On Fri, Dec 02, 2016 at 11:48:40AM -0700, Jack Bates wrote: > The "git diff --no-index" codepath didn't handle the --no-abbrev option. > Also it didn't behave the same as find_unique_abbrev() > in the case where abbrev == 0. > find_unique_abbrev() returns the full, unabbreviated string in that > case, but the "git diff --no-index" codepath returned an empty string. If you've dug into what's wrong, I think it's often good to add some notes in the commit message in case somebody has to revisit this later. For example, I'd have written something like: The "git diff --no-index" codepath doesn't handle the --no-abbrev option, because it relies on diff_opt_parse(). Normally that function is called as part of handle_revision_opt(), which handles the abbrev options itself. Adding the option directly to diff_opt_parse() fixes this. We don't need to do the same for --abbrev, because it's already handled there. Note that setting abbrev to "0" outside of a repository was broken recently by 4f03666ac (diff: handle sha1 abbreviations outside of repository, 2016-10-20). It adds a special out-of-repo code path for handling abbreviations which behaves differently than find_unique_abbrev() by truly giving a zero-length sha1, rather than taking "0" to mean "do not abbreviate". That bug was not triggerable until now, because there was no way to set the value to zero (using --abbrev=0 silently bumps it to the MINIMUM_ABBREV). > t/t4013-diff-various.sh | 7 +++++++ > t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir | 3 +++ > t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir | 3 +++ > t/t4013/diff.diff_--no-index_--raw_dir2_dir | 3 +++ > t/t4013/diff.diff_--raw_--abbrev=4_initial | 6 ++++++ > t/t4013/diff.diff_--raw_--no-abbrev_initial | 6 ++++++ > t/t4013/diff.diff_--raw_initial | 6 ++++++ I wondered if the tests without --no-index were redundant with earlier ones, but I don't think so. --abbrev=4 is tested with diff-tree, but --no-abbrev is not covered at all, AFAICT. > diff.c | 6 +++++- The actual code changes look good to me. Thanks. -Peff