git/builtin/diff.c, branch v2.46.2

builtin/diff: explicitly set hash algo when there is no repo

2024-05-07T05:50:49Z

The git-diff(1) command can be used outside repositories to diff two files with each other. But even if there is no repository we will end up hashing the files that we are diffing so that we can print the "index" line: ``` diff --git a/a b/b index 7898192..6178079 100644 --- a/a +++ b/b @@ -1 +1 @@ -a +b ``` We implicitly use SHA1 to calculate the hash here, which is because `the_repository` gets initialized with SHA1 during the startup routine. We are about to stop doing this though such that `the_repository` only ever has a hash function when it was properly initialized via a repo's configuration. To give full control to our users, we would ideally add a new switch to git-diff(1) that allows them to specify the hash function when executed outside of a repository. But for now, we only convert the code to make this explicit such that we can stop setting the default hash algorithm for `the_repository`. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano

builtin: stop using `the_index`

2024-04-18T19:30:42Z

Convert builtins to use `the_repository->index` instead of `the_index`. Signed-off-by: Patrick Steinhardt 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 --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

diff: drop useless "status" parameter from diff_result_code()

2023-08-21T22:33:24Z

Many programs use diff_result_code() to get a user-visible program exit code from a diff result (e.g., checking opts.found_changes if --exit-code was requested). This function also takes a "status" parameter, which seems at first glance that it could be used to propagate an error encountered when computing the diff. But it doesn't work that way: - negative values are passed through as-is, but are not appropriate as program exit codes - when --exit-code or --check is in effect, we _ignore_ the passed-in status completely. So a failed diff which did not have a chance to set opts.found_changes would erroneously report "success, no changes" instead of propagating the error. After recent cleanups, neither of these bugs is possible to trigger, as every caller just passes in "0". So rather than fixing them, we can simply drop the useless parameter instead. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

diff: drop useless return values in git-diff helpers

2023-08-21T22:33:24Z

Since git-diff has many diff modes, it dispatches to many helpers to perform each one. But every helper simply returns "0", as it exits directly if there are serious errors (and options like --exit-code are handled afterwards). So let's get rid of these useless return values, which makes the code flow more clear. There's very little chance that we'd later want to propagate errors instead of dying immediately. These are all static-local helpers for the git-diff program implementing its various modes. More "lib-ified" code would directly call the underlying functions. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

diff: drop useless return from run_diff_{files,index} functions

2023-08-21T22:33:24Z

Neither of these functions ever returns a value other than zero. Instead, they expect unrecoverable errors to exit immediately, and things like "--exit-code" are stored inside the diff_options struct to be handled later via diff_result_code(). Some callers do check the return values, but many don't bother. Let's drop the useless return values, which are misleading callers about how the functions work. This could be seen as a step in the wrong direction, as we might want to eventually "lib-ify" these to more cleanly return errors up the stack, in which case we'd have to add the return values back in. But there are some benefits to doing this now: 1. In the current code, somebody could accidentally add a "return -1" to one of the functions, which would be erroneously ignored by many callers. By removing the return code, the compiler can notice the mismatch and force the developer to decide what to do. Obviously the other option here is that we could start consistently checking the error code in every caller. But it would be dead code, and we wouldn't get any compile-time help in catching new cases. 2. It communicates the situation to callers, who may want to choose a different function. These functions are really thin wrappers for doing git-diff-files and git-diff-index within the process. But callers who care about recovering from an error here are probably better off using the underlying library functions, many of which do return errors. If somebody eventually wants to teach these functions to propagate errors, they'll have to switch back to returning a value, effectively reverting this patch. But at least then they will be starting with a level playing field: they know that they will need to inspect each caller to see how it should handle the error. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

diff: die when failing to read index in git-diff builtin

2023-08-21T22:33:24Z

When the git-diff program fails to read the index in its diff-files or diff-index helper functions, it propagates the error up the stack. This eventually lands in diff_result_code(), which does not handle it well (as discussed in the previous patch). Since the only sensible thing here is to exit with an error code (and what we were expecting the propagated error code to cause), let's just do that directly. There's no test here, as I'm not even sure this case can be triggered. The index-reading functions tend to die() themselves when encountering any errors, and the return value is just the number of entries in the file (and so always 0 or positive). But let's err on the conservative side and keep checking the return value. It may be worth digging into as a separate topic (though index-reading is low-level enough that we probably want to eventually teach it to propagate errors anyway for lib-ification purposes, at which point this code would already be doing the right thing). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

diff: show usage for unknown builtin_diff_files() options

2023-08-21T22:33:24Z

The git-diff command has many modes (comparing worktree to index, index to HEAD, individual blobs, etc). As a result, it dispatches to many helper functions and cannot completely parse its options until we're in those helper functions. Most of them, when seeing an unknown option, exit immediately by calling usage(). But builtin_diff_files(), which is the default if no revision or blob arguments are given, instead prints an error() and returns -1. One obvious shortcoming here is that the user doesn't get to see the usual usage message. But there's a much more important bug: the -1 return is fed to diff_result_code(), which is not ready to handle it. By default, it passes the code along as an exit code. We try to avoid negative exit codes because they get converted to unsigned values, but it should at least consistently show up as non-zero (i.e., a failure). But much worse is that when --exit-code is in effect, diff_result_code() will _ignore_ the status passed in by the caller, and instead only report on whether the diff found changes. It didn't, of course, because we never ran the diff, and the program unexpectedly exits with success! We can fix this bug by just calling usage(), like the other helpers do. Another option would of course be to teach diff_result_code() to handle this value. But as we'll see in the next few patches, it can be cleaned up even further. Let's just fix this bug directly to start with. Reported-by: Romain Chossart Signed-off-by: Jeff King Signed-off-by: Junio C Hamano