git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v4] diff: handle --no-abbrev in no-index case

From
Junio C Hamano <gitster@pobox.com>
Date
Dec 8, 2016, 22:53 UTC
Message-ID
<xmqqtwaerq1x.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<20161206165614.22921-1-jack@nottheoilrig.com>
Jack Bates <bk874k@nottheoilrig.com> writes:
Show 28 quoted lines
> There are two different places where the --no-abbrev option is parsed,
> and two different places where SHA-1s are abbreviated. We normally parse
> --no-abbrev with setup_revisions(), but in the no-index case, "git diff"
> calls diff_opt_parse() directly, and diff_opt_parse() didn't handle
> --no-abbrev until now. (It did handle --abbrev, however.) We normally
> abbreviate SHA-1s with find_unique_abbrev(), but commit 4f03666 ("diff:
> handle sha1 abbreviations outside of repository, 2016-10-20) recently
> introduced a special case when you run "git diff" outside of a
> repository.
>
> setup_revisions() does also call diff_opt_parse(), but not for --abbrev
> or --no-abbrev, which it handles itself. setup_revisions() sets
> rev_info->abbrev, and later copies that to diff_options->abbrev. It
> handles --no-abbrev by setting abbrev to zero. (This change doesn't
> touch that.)
>
> Setting abbrev to zero was broken in the outside-of-a-repository special
> case, which until now resulted in a truly zero-length SHA-1, rather than
> taking zero to mean do not abbreviate. The only way to trigger this bug,
> however, was by running "git diff --raw" without either the --abbrev or
> --no-abbrev options, because 1) without --raw it doesn't respect abbrev
> (which is bizarre, but has been that way forever), 2) we silently clamp
> --abbrev=0 to MINIMUM_ABBREV, and 3) --no-abbrev wasn't handled until
> now.
>
> The outside-of-a-repository case is one of three no-index cases. The
> other two are when one of the files you're comparing is outside of the
> repository you're in, and the --no-index option.
Nicely described.  
Show 14 quoted lines
> diff --git a/diff.c b/diff.c
> index ec87283..84dba60 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3106,7 +3106,8 @@ static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)
>  			abbrev = FALLBACK_DEFAULT_ABBREV;
>  		if (abbrev > GIT_SHA1_HEXSZ)
>  			die("BUG: oid abbreviation out of range: %d", abbrev);
> -		hex[abbrev] = '\0';
> +		if (abbrev)
> +			hex[abbrev] = '\0';
>  		return hex;
>  	}
>  }

This is the same since your earlier round and it is correct. The code before this part clamps abbrev to be between 0 and 40.

Show 5 quoted lines
> @@ -3364,6 +3365,7 @@ void diff_setup(struct diff_options *options)
>  
>  	options->file = stdout;
>  
> +	options->abbrev = DEFAULT_ABBREV;
This is a new change relative to your earlier one.  

I looked at all the callers of diff_setup() and noticed that many of them were initializing "struct diff_options" that is on-stack that is totally uninitialized, which means they were using a completely random value that happened to be on the stack.

Which was surprising and made me wonder how the entire "diff" code could have ever worked correctly for the past 10 years, as it's not like all the users always passed --[no-]abbrev[=<value>] from the command line.

In any case, this cannot possibly be introducing a regression; these callsites of diff_setup() were starting from a random garbage---now they start with -1 in this field. If they were doing the right thing by assigning their own abbrev to the field after diff_setup() returned, they will continue to do the same, and otherwise they will keep doing whatever random things they have been doing when the uninitialized field happened to contain -1 the same way.

I didn't look carefully at the additional tests, but the code change looks good.

Thanks.
Previous: Jack BatesNext: Jack Bates
Message 12 of 13 in “diff: handle --no-abbrev outside of repository”
  1. diff: handle --no-abbrev outside of repositoryJack Bates, Nov 28, 2016
  2. Junio C HamanoNov 28, 2016
  3. Jeff KingNov 29, 2016
  4. diff: handle --no-abbrev outside of repositoryJack Bates, Dec 2, 2016
  5. Jeff KingDec 5, 2016
  6. Jeff KingDec 5, 2016
  7. Jeff KingDec 5, 2016
  8. diff: handle --no-abbrev in no-index caseJack Bates, Dec 6, 2016
  9. diff: handle --no-abbrev in no-index caseJack Bates, Dec 6, 2016
  10. diff: handle --no-abbrev in no-index caseJack Bates, Dec 6, 2016
  11. Jack BatesDec 6, 2016
  12. Junio C HamanoDec 8, 2016
  13. Jack BatesDec 9, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.