Re: [PATCH v2] diff: handle --no-abbrev outside of repository

From: Jeff King <peff@peff.net>
Date: Dec 5, 2016, 06:01 UTC
Message-ID: <20161205060116.szy5ojetg3znu4w7@sigill.intra.peff.net>
In-Reply-To: <20161202184840.2158-1-jack@nottheoilrig.com>

On Fri, Dec 02, 2016 at 11:48:40AM -0700, Jack Bates wrote:

Show 5 quoted lines

> 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).

Show 7 quoted lines

>  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