git/diff.c, branch v2.43.2

Merge branch 'jk/diff-external-with-no-index' into maint-2.43

2024-02-13T22:44:48Z

"git diff --no-index file1 file2" segfaulted while invoking the external diff driver, which has been corrected. * jk/diff-external-with-no-index: diff: handle NULL meta-info when spawning external diff

Merge branch 'en/header-cleanup' into maint-2.43

2024-02-09T00:22:10Z

Remove unused header "#include". * en/header-cleanup: treewide: remove unnecessary includes in source files treewide: add direct includes currently only pulled in transitively trace2/tr2_tls.h: remove unnecessary include submodule-config.h: remove unnecessary include pkt-line.h: remove unnecessary include line-log.h: remove unnecessary include http.h: remove unnecessary include fsmonitor--daemon.h: remove unnecessary includes blame.h: remove unnecessary includes archive.h: remove unnecessary include treewide: remove unnecessary includes in source files treewide: remove unnecessary includes from header files

diff: handle NULL meta-info when spawning external diff

2024-01-29T18:37:44Z

Running this: $ touch foo bar $ chmod +x foo $ git -c diff.external=echo diff --ext-diff --no-index foo bar results in a segfault. The issue is that run_diff_cmd() passes a NULL "xfrm_msg" variable to run_external_diff(), which feeds it to strvec_push(), causing the segfault. The bug dates back to 82fbf269b9 (run_external_diff: use an argv_array for the command line, 2014-04-19), though it mostly only ever worked accidentally. Before then, we just stuck the NULL pointer into a "const char **" array, so our NULL ended up acting as an extra end-of-argv sentinel (which was OK, because it was the last thing in the array). Curiously, though, this is only a problem with --no-index. We set up xfrm_msg by calling fill_metainfo(). This result may be empty, or may have text like "index 1234..5678\n", "rename from foo\nrename from bar\n", etc. In run_external_diff(), we only look at xfrm_msg if the "other" variable is not NULL. That variable is set when the paths of the two sides of the diff pair aren't the same (in which case the destination path becomes "other"). So normally it would kick in only for a rename, in which case xfrm_msg should not be NULL (it would have the rename information in it). But with a "--no-index" of two blobs, we of course have two different pathnames, and thus end up with a non-NULL "other" filename (which is always just a repeat of the file2-name), but possibly a NULL xfrm_msg. So how to fix it? I have a feeling that --no-index always passing "other" to the external diff command is probably a bug. There was no rename, and the name is always redundant with existing information we pass (and this may even cause us to pass a useless "xfrm_msg" that contains an "index 1234..5678" line). So one option would be to change that behavior. We don't seem to have ever documented the "other" or "xfrm_msg" parameters for external diffs. But I'm not sure what fallout we might have from changing that behavior now. So this patch takes the less-risky option, and simply teaches run_external_diff() to avoid passing xfrm_msg when it's NULL. That makes it agnostic to whether "other" and "xfrm_msg" always come as a pair. It fixes the segfault now, and if we want to change the --no-index "other" behavior on top, it will handle that, too. Reported-by: Wilfred Hughes Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

treewide: remove unnecessary includes in source files

2023-12-26T20:04:31Z

Each of these were checked with gcc -E -I. ${SOURCE_FILE} | grep ${HEADER_FILE} to ensure that removing the direct inclusion of the header actually resulted in that header no longer being included at all (i.e. that no other header pulled it in transitively). ...except for a few cases where we verified that although the header was brought in transitively, nothing from it was directly used in that source file. These cases were: * builtin/credential-cache.c * builtin/pull.c * builtin/send-pack.c Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano

diff: give more detailed messages for bogus diff.* config

2023-12-08T23:26:22Z

The config callbacks for a few diff.* variables simply return -1 when we encounter an error. The message you get mentions the offending location, like: fatal: bad config variable 'diff.algorithm' in file '.git/config' at line 7 but is vague about "bad" (as it must be, since the message comes from the generic config code). Most callbacks add their own messages here, so let's do the same. E.g.: error: unknown value for config 'diff.algorithm': foo fatal: bad config variable 'diff.algorithm' in file '.git/config' at line 7 I've written the string in a way that should be reusable for translators, and matches another similar message in transport.c (there doesn't yet seem to be a popular generic message to reuse here, so hopefully this will get the ball rolling). Note that in the case of diff.algorithm, our parse_algorithm_value() helper does detect a NULL value string. But it's still worth detecting it ourselves here, since we can give a more specific error message (and which is the usual one for unexpected implicit-bool values). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

config: handle NULL value when parsing non-bools

2023-12-08T23:24:39Z

When the config parser sees an "implicit" bool like: [core] someVariable it passes NULL to the config callback. Any callback code which expects a string must check for NULL. This usually happens via helpers like git_config_string(), etc, but some custom code forgets to do so and will segfault. These are all fairly vanilla cases where the solution is just the usual pattern of: if (!value) return config_error_nonbool(var); though note that in a few cases we have to split initializers like: int some_var = initializer(); into: int some_var; if (!value) return config_error_nonbool(var); some_var = initializer(); There are still some broken instances after this patch, which I'll address on their own in individual patches after this one. Reported-by: Carlos Andrés Ramírez Cataño Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

diff --stat: set the width defaults in a helper function

2023-09-29T22:46:06Z

Extract the commonly used initialization of the --stat-width=, --stat-name-width= and --stat-graph-with= parameters to their internal default values into a helper function, to avoid repeating the same initialization code in a few places. Add a couple of tests to additionally cover existing configuration options diff.statNameWidth= and diff.statGraphWidth= when used by git-merge to generate --stat outputs. This closes the gap that existed previously in the --stat tests, and reduces the chances for having any regressions introduced by this commit. While there, perform a small bunch of minor wording tweaks in the improved unit test, to improve its test-level consistency a bit. Signed-off-by: Dragan Simic Signed-off-by: Junio C Hamano

diff --stat: add config option to limit filename width

2023-09-18T16:39:07Z

Add new configuration option diff.statNameWidth= that is equivalent to the command-line option --stat-name-width=, but it is ignored by format-patch. This follows the logic established by the already existing configuration option diff.statGraphWidth=. Limiting the widths of names and graphs in the --stat output makes sense for interactive work on wide terminals with many columns, hence the support for these configuration options. They don't affect format-patch because it already adheres to the traditional 80-column standard. Update the documentation and add more tests to cover new configuration option diff.statNameWidth=. While there, perform a few minor code and whitespace cleanups here and there, as spotted. Signed-off-by: Dragan Simic Signed-off-by: Junio C Hamano

Merge branch 'jk/diff-result-code-cleanup'

2023-09-01T18:26:28Z

"git diff --no-such-option" and other corner cases around the exit status of the "diff" command has been corrected. * jk/diff-result-code-cleanup: diff: drop useless "status" parameter from diff_result_code() diff: drop useless return values in git-diff helpers diff: drop useless return from run_diff_{files,index} functions diff: die when failing to read index in git-diff builtin diff: show usage for unknown builtin_diff_files() options diff-files: avoid negative exit value diff: spell DIFF_INDEX_CACHED out when calling run_diff_index()

Merge branch 'jc/diff-exit-code-with-w-fixes'

2023-08-30T20:50:41Z

"git diff -w --exit-code" with various options did not work correctly, which is being addressed. * jc/diff-exit-code-with-w-fixes: diff: the -w option breaks --exit-code for --raw and other output modes t4040: remove test that succeeded for a wrong reason diff: teach "--stat -w --exit-code" to notice differences diff: mode-only change should be noticed by "--patch -w --exit-code" diff: move the fallback "--exit-code" code down