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

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

[Jack Bates]

The "git diff --no-index" codepath
doesn't handle the --no-abbrev option.

Signed-off-by: Jack Bates <jack@nottheoilrig.com>
---
 diff.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index ec87283..0447eff 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;
 	}
 }
@@ -4024,6 +4025,8 @@ int diff_opt_parse(struct diff_options *options,
 			    offending, optarg);
 		return argcount;
 	}
+	else if (!strcmp(arg, "--no-abbrev"))
+		options->abbrev = 0;
 	else if (!strcmp(arg, "--abbrev"))
 		options->abbrev = DEFAULT_ABBREV;
 	else if (skip_prefix(arg, "--abbrev=", &arg)) {
-- 
2.10.2

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

[Junio C Hamano]

Jack Bates <bk874k@nottheoilrig.com> writes:

> The "git diff --no-index" codepath doesn't handle the --no-abbrev
> option.
>
> Signed-off-by: Jack Bates <jack@nottheoilrig.com>
> ---

This patch also needs a new test to protect the fix from future
breakages.

It is unfortunate that parsing of these options that are done in
diff_opt_parse() are not used by most of the codepaths; they instead
rely on revision.c parser to parse them into revs->abbrev and then
copied to revs->diffopt.abbrev in setup_revisions().  We would want
to rethink the structure of the code around this, and possibly move
towards using setup_revisions() more when appropriate and removing
diff_opt_parse() or something like that; the three-way fallback
codepath in builtin/am.c is the only other caller of this function
and it uses it to parse a fixed "--diff-filter=AM" option into
rev_info.diffopt and manually sets up rev_info as if revision parser
was given "diff --cached HEAD", which we should be able to replace
with a call to setup_revisions() of "--diff-filter=AM --cached HEAD",
I would suspect.  But that is a much larger change.

In any case, for now, the fix in this patch is a single best step
that moves us forward.  

Thanks.

>  diff.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/diff.c b/diff.c
> index ec87283..0447eff 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;
>  	}
>  }
> @@ -4024,6 +4025,8 @@ int diff_opt_parse(struct diff_options *options,
>  			    offending, optarg);
>  		return argcount;
>  	}
> +	else if (!strcmp(arg, "--no-abbrev"))
> +		options->abbrev = 0;
>  	else if (!strcmp(arg, "--abbrev"))
>  		options->abbrev = DEFAULT_ABBREV;
>  	else if (skip_prefix(arg, "--abbrev=", &arg)) {

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

[Jeff King]

On Mon, Nov 28, 2016 at 11:25:08AM -0700, Jack Bates wrote:

> diff --git a/diff.c b/diff.c
> index ec87283..0447eff 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 hunk made me wonder if there is a regression in v2.11, as this
fallback code is new in that version. But I don't think so.

This new code doesn't handle abbrev==0 the same as find_unique_abbrev()
does, and that's clearly a bug. But I couldn't find any way to trigger
it with the existing code.

The obvious way is with --no-abbrev, but that doesn't work without yet
without your patch.

A less obvious way is --abbrev=0, but that gets munged internally to
MINIMUM_ABBREV.

The most obscure is "git -c core.abbrev=0", but that barfs completely on
a too-small abbreviation (without even giving a good error message,
which is something we might want to fix).

So I think there is no regression, and we only have to worry about it as
part of the feature you are adding here (it might be worth calling out
the bug fix specifically in the commit message, though, or even putting
it in its own patch).

-Peff

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

[Jack Bates]

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.

Signed-off-by: Jack Bates <jack@nottheoilrig.com>
---
 diff.c                                                  | 6 +++++-
 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 ++++++
 8 files changed, 39 insertions(+), 1 deletion(-)
 create mode 100644 t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
 create mode 100644 t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
 create mode 100644 t/t4013/diff.diff_--no-index_--raw_dir2_dir
 create mode 100644 t/t4013/diff.diff_--raw_--abbrev=4_initial
 create mode 100644 t/t4013/diff.diff_--raw_--no-abbrev_initial
 create mode 100644 t/t4013/diff.diff_--raw_initial

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;
 	}
 }
@@ -3364,6 +3365,7 @@ void diff_setup(struct diff_options *options)
 
 	options->file = stdout;
 
+	options->abbrev = DEFAULT_ABBREV;
 	options->line_termination = '\n';
 	options->break_opt = -1;
 	options->rename_limit = -1;
@@ -4024,6 +4026,8 @@ int diff_opt_parse(struct diff_options *options,
 			    offending, optarg);
 		return argcount;
 	}
+	else if (!strcmp(arg, "--no-abbrev"))
+		options->abbrev = 0;
 	else if (!strcmp(arg, "--abbrev"))
 		options->abbrev = DEFAULT_ABBREV;
 	else if (skip_prefix(arg, "--abbrev=", &arg)) {
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 566817e..d7b71a0 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -311,6 +311,13 @@ diff --line-prefix=abc master master^ side
 diff --dirstat master~1 master~2
 diff --dirstat initial rearrange
 diff --dirstat-by-file initial rearrange
+# --abbrev and --no-abbrev outside of repository
+diff --raw initial
+diff --raw --abbrev=4 initial
+diff --raw --no-abbrev initial
+diff --no-index --raw dir2 dir
+diff --no-index --raw --abbrev=4 dir2 dir
+diff --no-index --raw --no-abbrev dir2 dir
 EOF
 
 test_expect_success 'log -S requires an argument' '
diff --git a/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
new file mode 100644
index 0000000..a71b38a
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw --abbrev=4 dir2 dir
+:000000 100644 0000... 0000... A	dir/sub
+$
diff --git a/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
new file mode 100644
index 0000000..e0f0097
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw --no-abbrev dir2 dir
+:000000 100644 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 A	dir/sub
+$
diff --git a/t/t4013/diff.diff_--no-index_--raw_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
new file mode 100644
index 0000000..3cb4ee7
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw dir2 dir
+:000000 100644 0000000... 0000000... A	dir/sub
+$
diff --git a/t/t4013/diff.diff_--raw_--abbrev=4_initial b/t/t4013/diff.diff_--raw_--abbrev=4_initial
new file mode 100644
index 0000000..c3641db
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_--abbrev=4_initial
@@ -0,0 +1,6 @@
+$ git diff --raw --abbrev=4 initial
+:100644 100644 35d2... 9929... M	dir/sub
+:100644 100644 01e7... 10a8... M	file0
+:000000 100644 0000... b1e6... A	file1
+:100644 000000 01e7... 0000... D	file2
+$
diff --git a/t/t4013/diff.diff_--raw_--no-abbrev_initial b/t/t4013/diff.diff_--raw_--no-abbrev_initial
new file mode 100644
index 0000000..c87a125
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_--no-abbrev_initial
@@ -0,0 +1,6 @@
+$ git diff --raw --no-abbrev initial
+:100644 100644 35d242ba79ae89ac695e26b3d4c27a8e6f028f9e 992913c5aa0a5476d10c49ed0f21fc0c6d1aedf3 M	dir/sub
+:100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d 10a8a9f3657f91a156b9f0184ed79a20adef9f7f M	file0
+:000000 100644 0000000000000000000000000000000000000000 b1e67221afe8461efd244b487afca22d46b95eb8 A	file1
+:100644 000000 01e79c32a8c99c557f0757da7cb6d65b3414466d 0000000000000000000000000000000000000000 D	file2
+$
diff --git a/t/t4013/diff.diff_--raw_initial b/t/t4013/diff.diff_--raw_initial
new file mode 100644
index 0000000..a3e9780
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_initial
@@ -0,0 +1,6 @@
+$ git diff --raw initial
+:100644 100644 35d242b... 992913c... M	dir/sub
+:100644 100644 01e79c3... 10a8a9f... M	file0
+:000000 100644 0000000... b1e6722... A	file1
+:100644 000000 01e79c3... 0000000... D	file2
+$
-- 
2.10.2

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

[Jeff King]

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

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

[Jeff King]

On Mon, Dec 05, 2016 at 01:01:16AM -0500, Jeff King wrote:

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

Actually, I take this last paragraph back. You _can_ trigger the bug
with just:

  echo one >foo
  echo two >bar
  git diff --no-index --raw foo bar

which prints only "..." for each entry.

I didn't notice it before because without "--raw", we show the patch
format. That uses the --full-index option, and does not respect --abbrev
at all (which seems kind of bizarre, but has been that way forever).

So I think there _is_ a regression in v2.11, and the second half of your
change fixes it.

-Peff

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

[Jeff King]

On Mon, Dec 05, 2016 at 01:15:00AM -0500, Jeff King wrote:

> On Mon, Dec 05, 2016 at 01:01:16AM -0500, Jeff King wrote:
> 
> >   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).
> 
> Actually, I take this last paragraph back. You _can_ trigger the bug
> with just:
> 
>   echo one >foo
>   echo two >bar
>   git diff --no-index --raw foo bar
> 
> which prints only "..." for each entry.
> 
> I didn't notice it before because without "--raw", we show the patch
> format. That uses the --full-index option, and does not respect --abbrev
> at all (which seems kind of bizarre, but has been that way forever).
> 
> So I think there _is_ a regression in v2.11, and the second half of your
> change fixes it.

Sorry for the sequence of emails, but as usual with "diff --no-index",
the deeper I dig the more confusion I find. :)

After digging into your related thread in:

  http://public-inbox.org/git/20161205065523.yspqt34p3dp5g5fk@sigill.intra.peff.net/

I'm not convinced that "--no-index --raw" output isn't generally
nonsensical in the first place. So yes, there's a regression there (and
it's not just "oops, we didn't abbreviate correctly", but rather that
the output format is broken). But I'm not sure it's something people are
using. So it should be fixed on the 'maint' track, but I don't think
it's incredibly urgent.

-Peff

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

[Jack Bates]

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.

Signed-off-by: Jack Bates <jack@nottheoilrig.com>
---
 diff.c                                                  | 6 +++++-
 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 ++++++
 8 files changed, 39 insertions(+), 1 deletion(-)
 create mode 100644 t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
 create mode 100644 t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
 create mode 100644 t/t4013/diff.diff_--no-index_--raw_dir2_dir
 create mode 100644 t/t4013/diff.diff_--raw_--abbrev=4_initial
 create mode 100644 t/t4013/diff.diff_--raw_--no-abbrev_initial
 create mode 100644 t/t4013/diff.diff_--raw_initial

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;
 	}
 }
@@ -3364,6 +3365,7 @@ void diff_setup(struct diff_options *options)
 
 	options->file = stdout;
 
+	options->abbrev = DEFAULT_ABBREV;
 	options->line_termination = '\n';
 	options->break_opt = -1;
 	options->rename_limit = -1;
@@ -4024,6 +4026,8 @@ int diff_opt_parse(struct diff_options *options,
 			    offending, optarg);
 		return argcount;
 	}
+	else if (!strcmp(arg, "--no-abbrev"))
+		options->abbrev = 0;
 	else if (!strcmp(arg, "--abbrev"))
 		options->abbrev = DEFAULT_ABBREV;
 	else if (skip_prefix(arg, "--abbrev=", &arg)) {
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 566817e..d7b71a0 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -311,6 +311,13 @@ diff --line-prefix=abc master master^ side
 diff --dirstat master~1 master~2
 diff --dirstat initial rearrange
 diff --dirstat-by-file initial rearrange
+# --abbrev and --no-abbrev outside of repository
+diff --raw initial
+diff --raw --abbrev=4 initial
+diff --raw --no-abbrev initial
+diff --no-index --raw dir2 dir
+diff --no-index --raw --abbrev=4 dir2 dir
+diff --no-index --raw --no-abbrev dir2 dir
 EOF
 
 test_expect_success 'log -S requires an argument' '
diff --git a/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
new file mode 100644
index 0000000..a71b38a
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw --abbrev=4 dir2 dir
+:000000 100644 0000... 0000... A	dir/sub
+$
diff --git a/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
new file mode 100644
index 0000000..e0f0097
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw --no-abbrev dir2 dir
+:000000 100644 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 A	dir/sub
+$
diff --git a/t/t4013/diff.diff_--no-index_--raw_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
new file mode 100644
index 0000000..3cb4ee7
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw dir2 dir
+:000000 100644 0000000... 0000000... A	dir/sub
+$
diff --git a/t/t4013/diff.diff_--raw_--abbrev=4_initial b/t/t4013/diff.diff_--raw_--abbrev=4_initial
new file mode 100644
index 0000000..c3641db
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_--abbrev=4_initial
@@ -0,0 +1,6 @@
+$ git diff --raw --abbrev=4 initial
+:100644 100644 35d2... 9929... M	dir/sub
+:100644 100644 01e7... 10a8... M	file0
+:000000 100644 0000... b1e6... A	file1
+:100644 000000 01e7... 0000... D	file2
+$
diff --git a/t/t4013/diff.diff_--raw_--no-abbrev_initial b/t/t4013/diff.diff_--raw_--no-abbrev_initial
new file mode 100644
index 0000000..c87a125
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_--no-abbrev_initial
@@ -0,0 +1,6 @@
+$ git diff --raw --no-abbrev initial
+:100644 100644 35d242ba79ae89ac695e26b3d4c27a8e6f028f9e 992913c5aa0a5476d10c49ed0f21fc0c6d1aedf3 M	dir/sub
+:100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d 10a8a9f3657f91a156b9f0184ed79a20adef9f7f M	file0
+:000000 100644 0000000000000000000000000000000000000000 b1e67221afe8461efd244b487afca22d46b95eb8 A	file1
+:100644 000000 01e79c32a8c99c557f0757da7cb6d65b3414466d 0000000000000000000000000000000000000000 D	file2
+$
diff --git a/t/t4013/diff.diff_--raw_initial b/t/t4013/diff.diff_--raw_initial
new file mode 100644
index 0000000..a3e9780
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_initial
@@ -0,0 +1,6 @@
+$ git diff --raw initial
+:100644 100644 35d242b... 992913c... M	dir/sub
+:100644 100644 01e79c3... 10a8a9f... M	file0
+:000000 100644 0000000... b1e6722... A	file1
+:100644 000000 01e79c3... 0000000... D	file2
+$
-- 
2.10.2

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

[Jack Bates]

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.

Signed-off-by: Jack Bates <jack@nottheoilrig.com>
---
 diff.c                                                  | 6 +++++-
 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 ++++++
 8 files changed, 39 insertions(+), 1 deletion(-)
 create mode 100644 t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
 create mode 100644 t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
 create mode 100644 t/t4013/diff.diff_--no-index_--raw_dir2_dir
 create mode 100644 t/t4013/diff.diff_--raw_--abbrev=4_initial
 create mode 100644 t/t4013/diff.diff_--raw_--no-abbrev_initial
 create mode 100644 t/t4013/diff.diff_--raw_initial

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;
 	}
 }
@@ -3364,6 +3365,7 @@ void diff_setup(struct diff_options *options)
 
 	options->file = stdout;
 
+	options->abbrev = DEFAULT_ABBREV;
 	options->line_termination = '\n';
 	options->break_opt = -1;
 	options->rename_limit = -1;
@@ -4024,6 +4026,8 @@ int diff_opt_parse(struct diff_options *options,
 			    offending, optarg);
 		return argcount;
 	}
+	else if (!strcmp(arg, "--no-abbrev"))
+		options->abbrev = 0;
 	else if (!strcmp(arg, "--abbrev"))
 		options->abbrev = DEFAULT_ABBREV;
 	else if (skip_prefix(arg, "--abbrev=", &arg)) {
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 566817e..d7b71a0 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -311,6 +311,13 @@ diff --line-prefix=abc master master^ side
 diff --dirstat master~1 master~2
 diff --dirstat initial rearrange
 diff --dirstat-by-file initial rearrange
+# --abbrev and --no-abbrev outside of repository
+diff --raw initial
+diff --raw --abbrev=4 initial
+diff --raw --no-abbrev initial
+diff --no-index --raw dir2 dir
+diff --no-index --raw --abbrev=4 dir2 dir
+diff --no-index --raw --no-abbrev dir2 dir
 EOF
 
 test_expect_success 'log -S requires an argument' '
diff --git a/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
new file mode 100644
index 0000000..a71b38a
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw --abbrev=4 dir2 dir
+:000000 100644 0000... 0000... A	dir/sub
+$
diff --git a/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
new file mode 100644
index 0000000..e0f0097
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw --no-abbrev dir2 dir
+:000000 100644 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 A	dir/sub
+$
diff --git a/t/t4013/diff.diff_--no-index_--raw_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
new file mode 100644
index 0000000..3cb4ee7
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw dir2 dir
+:000000 100644 0000000... 0000000... A	dir/sub
+$
diff --git a/t/t4013/diff.diff_--raw_--abbrev=4_initial b/t/t4013/diff.diff_--raw_--abbrev=4_initial
new file mode 100644
index 0000000..c3641db
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_--abbrev=4_initial
@@ -0,0 +1,6 @@
+$ git diff --raw --abbrev=4 initial
+:100644 100644 35d2... 9929... M	dir/sub
+:100644 100644 01e7... 10a8... M	file0
+:000000 100644 0000... b1e6... A	file1
+:100644 000000 01e7... 0000... D	file2
+$
diff --git a/t/t4013/diff.diff_--raw_--no-abbrev_initial b/t/t4013/diff.diff_--raw_--no-abbrev_initial
new file mode 100644
index 0000000..c87a125
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_--no-abbrev_initial
@@ -0,0 +1,6 @@
+$ git diff --raw --no-abbrev initial
+:100644 100644 35d242ba79ae89ac695e26b3d4c27a8e6f028f9e 992913c5aa0a5476d10c49ed0f21fc0c6d1aedf3 M	dir/sub
+:100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d 10a8a9f3657f91a156b9f0184ed79a20adef9f7f M	file0
+:000000 100644 0000000000000000000000000000000000000000 b1e67221afe8461efd244b487afca22d46b95eb8 A	file1
+:100644 000000 01e79c32a8c99c557f0757da7cb6d65b3414466d 0000000000000000000000000000000000000000 D	file2
+$
diff --git a/t/t4013/diff.diff_--raw_initial b/t/t4013/diff.diff_--raw_initial
new file mode 100644
index 0000000..a3e9780
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_initial
@@ -0,0 +1,6 @@
+$ git diff --raw initial
+:100644 100644 35d242b... 992913c... M	dir/sub
+:100644 100644 01e79c3... 10a8a9f... M	file0
+:000000 100644 0000000... b1e6722... A	file1
+:100644 000000 01e79c3... 0000000... D	file2
+$
-- 
2.10.2

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

[Jack Bates]

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.

Signed-off-by: Jack Bates <jack@nottheoilrig.com>
---
 diff.c                                                  | 6 +++++-
 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 ++++++
 8 files changed, 39 insertions(+), 1 deletion(-)
 create mode 100644 t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
 create mode 100644 t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
 create mode 100644 t/t4013/diff.diff_--no-index_--raw_dir2_dir
 create mode 100644 t/t4013/diff.diff_--raw_--abbrev=4_initial
 create mode 100644 t/t4013/diff.diff_--raw_--no-abbrev_initial
 create mode 100644 t/t4013/diff.diff_--raw_initial

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;
 	}
 }
@@ -3364,6 +3365,7 @@ void diff_setup(struct diff_options *options)
 
 	options->file = stdout;
 
+	options->abbrev = DEFAULT_ABBREV;
 	options->line_termination = '\n';
 	options->break_opt = -1;
 	options->rename_limit = -1;
@@ -4024,6 +4026,8 @@ int diff_opt_parse(struct diff_options *options,
 			    offending, optarg);
 		return argcount;
 	}
+	else if (!strcmp(arg, "--no-abbrev"))
+		options->abbrev = 0;
 	else if (!strcmp(arg, "--abbrev"))
 		options->abbrev = DEFAULT_ABBREV;
 	else if (skip_prefix(arg, "--abbrev=", &arg)) {
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 566817e..d09acfe 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -311,6 +311,13 @@ diff --line-prefix=abc master master^ side
 diff --dirstat master~1 master~2
 diff --dirstat initial rearrange
 diff --dirstat-by-file initial rearrange
+# No-index --abbrev and --no-abbrev
+diff --raw initial
+diff --raw --abbrev=4 initial
+diff --raw --no-abbrev initial
+diff --no-index --raw dir2 dir
+diff --no-index --raw --abbrev=4 dir2 dir
+diff --no-index --raw --no-abbrev dir2 dir
 EOF
 
 test_expect_success 'log -S requires an argument' '
diff --git a/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
new file mode 100644
index 0000000..a71b38a
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw --abbrev=4 dir2 dir
+:000000 100644 0000... 0000... A	dir/sub
+$
diff --git a/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
new file mode 100644
index 0000000..e0f0097
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw --no-abbrev dir2 dir
+:000000 100644 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 A	dir/sub
+$
diff --git a/t/t4013/diff.diff_--no-index_--raw_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
new file mode 100644
index 0000000..3cb4ee7
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw dir2 dir
+:000000 100644 0000000... 0000000... A	dir/sub
+$
diff --git a/t/t4013/diff.diff_--raw_--abbrev=4_initial b/t/t4013/diff.diff_--raw_--abbrev=4_initial
new file mode 100644
index 0000000..c3641db
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_--abbrev=4_initial
@@ -0,0 +1,6 @@
+$ git diff --raw --abbrev=4 initial
+:100644 100644 35d2... 9929... M	dir/sub
+:100644 100644 01e7... 10a8... M	file0
+:000000 100644 0000... b1e6... A	file1
+:100644 000000 01e7... 0000... D	file2
+$
diff --git a/t/t4013/diff.diff_--raw_--no-abbrev_initial b/t/t4013/diff.diff_--raw_--no-abbrev_initial
new file mode 100644
index 0000000..c87a125
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_--no-abbrev_initial
@@ -0,0 +1,6 @@
+$ git diff --raw --no-abbrev initial
+:100644 100644 35d242ba79ae89ac695e26b3d4c27a8e6f028f9e 992913c5aa0a5476d10c49ed0f21fc0c6d1aedf3 M	dir/sub
+:100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d 10a8a9f3657f91a156b9f0184ed79a20adef9f7f M	file0
+:000000 100644 0000000000000000000000000000000000000000 b1e67221afe8461efd244b487afca22d46b95eb8 A	file1
+:100644 000000 01e79c32a8c99c557f0757da7cb6d65b3414466d 0000000000000000000000000000000000000000 D	file2
+$
diff --git a/t/t4013/diff.diff_--raw_initial b/t/t4013/diff.diff_--raw_initial
new file mode 100644
index 0000000..a3e9780
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_initial
@@ -0,0 +1,6 @@
+$ git diff --raw initial
+:100644 100644 35d242b... 992913c... M	dir/sub
+:100644 100644 01e79c3... 10a8a9f... M	file0
+:000000 100644 0000000... b1e6722... A	file1
+:100644 000000 01e79c3... 0000000... D	file2
+$
-- 
2.10.2

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

[Jack Bates]

On 06/12/16 09:56 AM, Jack Bates wrote:
> 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.
>
> Signed-off-by: Jack Bates <jack@nottheoilrig.com>
> ---
>  diff.c                                                  | 6 +++++-
>  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 ++++++
>  8 files changed, 39 insertions(+), 1 deletion(-)
>  create mode 100644 t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
>  create mode 100644 t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
>  create mode 100644 t/t4013/diff.diff_--no-index_--raw_dir2_dir
>  create mode 100644 t/t4013/diff.diff_--raw_--abbrev=4_initial
>  create mode 100644 t/t4013/diff.diff_--raw_--no-abbrev_initial
>  create mode 100644 t/t4013/diff.diff_--raw_initial
>
> 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;
>  	}
>  }
> @@ -3364,6 +3365,7 @@ void diff_setup(struct diff_options *options)
>
>  	options->file = stdout;
>
> +	options->abbrev = DEFAULT_ABBREV;
>  	options->line_termination = '\n';
>  	options->break_opt = -1;
>  	options->rename_limit = -1;
> @@ -4024,6 +4026,8 @@ int diff_opt_parse(struct diff_options *options,
>  			    offending, optarg);
>  		return argcount;
>  	}
> +	else if (!strcmp(arg, "--no-abbrev"))
> +		options->abbrev = 0;
>  	else if (!strcmp(arg, "--abbrev"))
>  		options->abbrev = DEFAULT_ABBREV;
>  	else if (skip_prefix(arg, "--abbrev=", &arg)) {
> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index 566817e..d09acfe 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -311,6 +311,13 @@ diff --line-prefix=abc master master^ side
>  diff --dirstat master~1 master~2
>  diff --dirstat initial rearrange
>  diff --dirstat-by-file initial rearrange
> +# No-index --abbrev and --no-abbrev

I updated this comment to be consistent with the no-index vs. 
outside-of-a-repository distinction in the commit message.

> +diff --raw initial
> +diff --raw --abbrev=4 initial
> +diff --raw --no-abbrev initial
> +diff --no-index --raw dir2 dir
> +diff --no-index --raw --abbrev=4 dir2 dir
> +diff --no-index --raw --no-abbrev dir2 dir
>  EOF
>
>  test_expect_success 'log -S requires an argument' '
> diff --git a/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
> new file mode 100644
> index 0000000..a71b38a
> --- /dev/null
> +++ b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
> @@ -0,0 +1,3 @@
> +$ git diff --no-index --raw --abbrev=4 dir2 dir
> +:000000 100644 0000... 0000... A	dir/sub
> +$
> diff --git a/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
> new file mode 100644
> index 0000000..e0f0097
> --- /dev/null
> +++ b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
> @@ -0,0 +1,3 @@
> +$ git diff --no-index --raw --no-abbrev dir2 dir
> +:000000 100644 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 A	dir/sub
> +$
> diff --git a/t/t4013/diff.diff_--no-index_--raw_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
> new file mode 100644
> index 0000000..3cb4ee7
> --- /dev/null
> +++ b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
> @@ -0,0 +1,3 @@
> +$ git diff --no-index --raw dir2 dir
> +:000000 100644 0000000... 0000000... A	dir/sub
> +$
> diff --git a/t/t4013/diff.diff_--raw_--abbrev=4_initial b/t/t4013/diff.diff_--raw_--abbrev=4_initial
> new file mode 100644
> index 0000000..c3641db
> --- /dev/null
> +++ b/t/t4013/diff.diff_--raw_--abbrev=4_initial
> @@ -0,0 +1,6 @@
> +$ git diff --raw --abbrev=4 initial
> +:100644 100644 35d2... 9929... M	dir/sub
> +:100644 100644 01e7... 10a8... M	file0
> +:000000 100644 0000... b1e6... A	file1
> +:100644 000000 01e7... 0000... D	file2
> +$
> diff --git a/t/t4013/diff.diff_--raw_--no-abbrev_initial b/t/t4013/diff.diff_--raw_--no-abbrev_initial
> new file mode 100644
> index 0000000..c87a125
> --- /dev/null
> +++ b/t/t4013/diff.diff_--raw_--no-abbrev_initial
> @@ -0,0 +1,6 @@
> +$ git diff --raw --no-abbrev initial
> +:100644 100644 35d242ba79ae89ac695e26b3d4c27a8e6f028f9e 992913c5aa0a5476d10c49ed0f21fc0c6d1aedf3 M	dir/sub
> +:100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d 10a8a9f3657f91a156b9f0184ed79a20adef9f7f M	file0
> +:000000 100644 0000000000000000000000000000000000000000 b1e67221afe8461efd244b487afca22d46b95eb8 A	file1
> +:100644 000000 01e79c32a8c99c557f0757da7cb6d65b3414466d 0000000000000000000000000000000000000000 D	file2
> +$
> diff --git a/t/t4013/diff.diff_--raw_initial b/t/t4013/diff.diff_--raw_initial
> new file mode 100644
> index 0000000..a3e9780
> --- /dev/null
> +++ b/t/t4013/diff.diff_--raw_initial
> @@ -0,0 +1,6 @@
> +$ git diff --raw initial
> +:100644 100644 35d242b... 992913c... M	dir/sub
> +:100644 100644 01e79c3... 10a8a9f... M	file0
> +:000000 100644 0000000... b1e6722... A	file1
> +:100644 000000 01e79c3... 0000000... D	file2
> +$
>

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

[Junio C Hamano]

Jack Bates <bk874k@nottheoilrig.com> writes:

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

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

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

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

[Jack Bates]

On 08/12/16 03:53 PM, Junio C Hamano wrote:
> Jack Bates <bk874k@nottheoilrig.com> writes:
>> @@ -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.

Great, thanks for reviewing it!