{"thread":{"id":"44557","subject":"[PATCH] diff: handle --no-abbrev outside of repository","startedAt":"2016-11-28T19:02:42Z","lastAt":"2016-12-09T00:22:08Z","messageCount":13,"participants":["Jack Bates","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"306510","messageId":"20161128182508.10570-1-jack@nottheoilrig.com","threadId":"44557","inReplyTo":null,"subject":"[PATCH] diff: handle --no-abbrev outside of repository","fromName":"Jack Bates","fromEmail":"bk874k@nottheoilrig.com","sentAt":"2016-11-28T18:25:08Z","receivedAt":"2016-11-28T19:02:42Z","isPatch":true,"sender":{"key":"bk874k@nottheoilrig.com","avatar":null},"body":"The \"git diff --no-index\" codepath\ndoesn't handle the --no-abbrev option.\n\nSigned-off-by: Jack Bates <jack@nottheoilrig.com>\n---\n diff.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex ec87283..0447eff 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3106,7 +3106,8 @@ static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)\n \t\t\tabbrev = FALLBACK_DEFAULT_ABBREV;\n \t\tif (abbrev > GIT_SHA1_HEXSZ)\n \t\t\tdie(\"BUG: oid abbreviation out of range: %d\", abbrev);\n-\t\thex[abbrev] = '\\0';\n+\t\tif (abbrev)\n+\t\t\thex[abbrev] = '\\0';\n \t\treturn hex;\n \t}\n }\n@@ -4024,6 +4025,8 @@ int diff_opt_parse(struct diff_options *options,\n \t\t\t    offending, optarg);\n \t\treturn argcount;\n \t}\n+\telse if (!strcmp(arg, \"--no-abbrev\"))\n+\t\toptions->abbrev = 0;\n \telse if (!strcmp(arg, \"--abbrev\"))\n \t\toptions->abbrev = DEFAULT_ABBREV;\n \telse if (skip_prefix(arg, \"--abbrev=\", &arg)) {\n-- \n2.10.2\n"},{"id":"306523","messageId":"xmqqbmwzi4wa.fsf@gitster.mtv.corp.google.com","threadId":"44557","inReplyTo":"20161128182508.10570-1-jack@nottheoilrig.com","subject":"Re: [PATCH] diff: handle --no-abbrev outside of repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-28T23:03:17Z","receivedAt":"2016-11-28T23:03:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jack Bates <bk874k@nottheoilrig.com> writes:\n\n> The \"git diff --no-index\" codepath doesn't handle the --no-abbrev\n> option.\n>\n> Signed-off-by: Jack Bates <jack@nottheoilrig.com>\n> ---\n\nThis patch also needs a new test to protect the fix from future\nbreakages.\n\nIt is unfortunate that parsing of these options that are done in\ndiff_opt_parse() are not used by most of the codepaths; they instead\nrely on revision.c parser to parse them into revs->abbrev and then\ncopied to revs->diffopt.abbrev in setup_revisions().  We would want\nto rethink the structure of the code around this, and possibly move\ntowards using setup_revisions() more when appropriate and removing\ndiff_opt_parse() or something like that; the three-way fallback\ncodepath in builtin/am.c is the only other caller of this function\nand it uses it to parse a fixed \"--diff-filter=AM\" option into\nrev_info.diffopt and manually sets up rev_info as if revision parser\nwas given \"diff --cached HEAD\", which we should be able to replace\nwith a call to setup_revisions() of \"--diff-filter=AM --cached HEAD\",\nI would suspect.  But that is a much larger change.\n\nIn any case, for now, the fix in this patch is a single best step\nthat moves us forward.  \n\nThanks.\n\n>  diff.c | 5 ++++-\n>  1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/diff.c b/diff.c\n> index ec87283..0447eff 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -3106,7 +3106,8 @@ static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)\n>  \t\t\tabbrev = FALLBACK_DEFAULT_ABBREV;\n>  \t\tif (abbrev > GIT_SHA1_HEXSZ)\n>  \t\t\tdie(\"BUG: oid abbreviation out of range: %d\", abbrev);\n> -\t\thex[abbrev] = '\\0';\n> +\t\tif (abbrev)\n> +\t\t\thex[abbrev] = '\\0';\n>  \t\treturn hex;\n>  \t}\n>  }\n> @@ -4024,6 +4025,8 @@ int diff_opt_parse(struct diff_options *options,\n>  \t\t\t    offending, optarg);\n>  \t\treturn argcount;\n>  \t}\n> +\telse if (!strcmp(arg, \"--no-abbrev\"))\n> +\t\toptions->abbrev = 0;\n>  \telse if (!strcmp(arg, \"--abbrev\"))\n>  \t\toptions->abbrev = DEFAULT_ABBREV;\n>  \telse if (skip_prefix(arg, \"--abbrev=\", &arg)) {\n"},{"id":"306535","messageId":"20161129070637.eult6o3m34r2mima@sigill.intra.peff.net","threadId":"44557","inReplyTo":"20161128182508.10570-1-jack@nottheoilrig.com","subject":"Re: [PATCH] diff: handle --no-abbrev outside of repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-29T07:06:37Z","receivedAt":"2016-11-29T07:06:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 28, 2016 at 11:25:08AM -0700, Jack Bates wrote:\n\n> diff --git a/diff.c b/diff.c\n> index ec87283..0447eff 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -3106,7 +3106,8 @@ static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)\n>  \t\t\tabbrev = FALLBACK_DEFAULT_ABBREV;\n>  \t\tif (abbrev > GIT_SHA1_HEXSZ)\n>  \t\t\tdie(\"BUG: oid abbreviation out of range: %d\", abbrev);\n> -\t\thex[abbrev] = '\\0';\n> +\t\tif (abbrev)\n> +\t\t\thex[abbrev] = '\\0';\n>  \t\treturn hex;\n>  \t}\n\nThis hunk made me wonder if there is a regression in v2.11, as this\nfallback code is new in that version. But I don't think so.\n\nThis new code doesn't handle abbrev==0 the same as find_unique_abbrev()\ndoes, and that's clearly a bug. But I couldn't find any way to trigger\nit with the existing code.\n\nThe obvious way is with --no-abbrev, but that doesn't work without yet\nwithout your patch.\n\nA less obvious way is --abbrev=0, but that gets munged internally to\nMINIMUM_ABBREV.\n\nThe most obscure is \"git -c core.abbrev=0\", but that barfs completely on\na too-small abbreviation (without even giving a good error message,\nwhich is something we might want to fix).\n\nSo I think there is no regression, and we only have to worry about it as\npart of the feature you are adding here (it might be worth calling out\nthe bug fix specifically in the commit message, though, or even putting\nit in its own patch).\n\n-Peff\n"},{"id":"306802","messageId":"20161202184840.2158-1-jack@nottheoilrig.com","threadId":"44557","inReplyTo":"20161129070637.eult6o3m34r2mima@sigill.intra.peff.net","subject":"[PATCH v2] diff: handle --no-abbrev outside of repository","fromName":"Jack Bates","fromEmail":"bk874k@nottheoilrig.com","sentAt":"2016-12-02T18:48:40Z","receivedAt":"2016-12-02T18:49:58Z","isPatch":true,"sender":{"key":"bk874k@nottheoilrig.com","avatar":null},"body":"The \"git diff --no-index\" codepath didn't handle the --no-abbrev option.\nAlso it didn't behave the same as find_unique_abbrev()\nin the case where abbrev == 0.\nfind_unique_abbrev() returns the full, unabbreviated string in that\ncase, but the \"git diff --no-index\" codepath returned an empty string.\n\nSigned-off-by: Jack Bates <jack@nottheoilrig.com>\n---\n diff.c                                                  | 6 +++++-\n t/t4013-diff-various.sh                                 | 7 +++++++\n t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir  | 3 +++\n t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir | 3 +++\n t/t4013/diff.diff_--no-index_--raw_dir2_dir             | 3 +++\n t/t4013/diff.diff_--raw_--abbrev=4_initial              | 6 ++++++\n t/t4013/diff.diff_--raw_--no-abbrev_initial             | 6 ++++++\n t/t4013/diff.diff_--raw_initial                         | 6 ++++++\n 8 files changed, 39 insertions(+), 1 deletion(-)\n create mode 100644 t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir\n create mode 100644 t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir\n create mode 100644 t/t4013/diff.diff_--no-index_--raw_dir2_dir\n create mode 100644 t/t4013/diff.diff_--raw_--abbrev=4_initial\n create mode 100644 t/t4013/diff.diff_--raw_--no-abbrev_initial\n create mode 100644 t/t4013/diff.diff_--raw_initial\n\ndiff --git a/diff.c b/diff.c\nindex ec87283..84dba60 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3106,7 +3106,8 @@ static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)\n \t\t\tabbrev = FALLBACK_DEFAULT_ABBREV;\n \t\tif (abbrev > GIT_SHA1_HEXSZ)\n \t\t\tdie(\"BUG: oid abbreviation out of range: %d\", abbrev);\n-\t\thex[abbrev] = '\\0';\n+\t\tif (abbrev)\n+\t\t\thex[abbrev] = '\\0';\n \t\treturn hex;\n \t}\n }\n@@ -3364,6 +3365,7 @@ void diff_setup(struct diff_options *options)\n \n \toptions->file = stdout;\n \n+\toptions->abbrev = DEFAULT_ABBREV;\n \toptions->line_termination = '\\n';\n \toptions->break_opt = -1;\n \toptions->rename_limit = -1;\n@@ -4024,6 +4026,8 @@ int diff_opt_parse(struct diff_options *options,\n \t\t\t    offending, optarg);\n \t\treturn argcount;\n \t}\n+\telse if (!strcmp(arg, \"--no-abbrev\"))\n+\t\toptions->abbrev = 0;\n \telse if (!strcmp(arg, \"--abbrev\"))\n \t\toptions->abbrev = DEFAULT_ABBREV;\n \telse if (skip_prefix(arg, \"--abbrev=\", &arg)) {\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 566817e..d7b71a0 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -311,6 +311,13 @@ diff --line-prefix=abc master master^ side\n diff --dirstat master~1 master~2\n diff --dirstat initial rearrange\n diff --dirstat-by-file initial rearrange\n+# --abbrev and --no-abbrev outside of repository\n+diff --raw initial\n+diff --raw --abbrev=4 initial\n+diff --raw --no-abbrev initial\n+diff --no-index --raw dir2 dir\n+diff --no-index --raw --abbrev=4 dir2 dir\n+diff --no-index --raw --no-abbrev dir2 dir\n EOF\n \n test_expect_success 'log -S requires an argument' '\ndiff --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\nnew file mode 100644\nindex 0000000..a71b38a\n--- /dev/null\n+++ b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir\n@@ -0,0 +1,3 @@\n+$ git diff --no-index --raw --abbrev=4 dir2 dir\n+:000000 100644 0000... 0000... A\tdir/sub\n+$\ndiff --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\nnew file mode 100644\nindex 0000000..e0f0097\n--- /dev/null\n+++ b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir\n@@ -0,0 +1,3 @@\n+$ git diff --no-index --raw --no-abbrev dir2 dir\n+:000000 100644 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 A\tdir/sub\n+$\ndiff --git a/t/t4013/diff.diff_--no-index_--raw_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_dir2_dir\nnew file mode 100644\nindex 0000000..3cb4ee7\n--- /dev/null\n+++ b/t/t4013/diff.diff_--no-index_--raw_dir2_dir\n@@ -0,0 +1,3 @@\n+$ git diff --no-index --raw dir2 dir\n+:000000 100644 0000000... 0000000... A\tdir/sub\n+$\ndiff --git a/t/t4013/diff.diff_--raw_--abbrev=4_initial b/t/t4013/diff.diff_--raw_--abbrev=4_initial\nnew file mode 100644\nindex 0000000..c3641db\n--- /dev/null\n+++ b/t/t4013/diff.diff_--raw_--abbrev=4_initial\n@@ -0,0 +1,6 @@\n+$ git diff --raw --abbrev=4 initial\n+:100644 100644 35d2... 9929... M\tdir/sub\n+:100644 100644 01e7... 10a8... M\tfile0\n+:000000 100644 0000... b1e6... A\tfile1\n+:100644 000000 01e7... 0000... D\tfile2\n+$\ndiff --git a/t/t4013/diff.diff_--raw_--no-abbrev_initial b/t/t4013/diff.diff_--raw_--no-abbrev_initial\nnew file mode 100644\nindex 0000000..c87a125\n--- /dev/null\n+++ b/t/t4013/diff.diff_--raw_--no-abbrev_initial\n@@ -0,0 +1,6 @@\n+$ git diff --raw --no-abbrev initial\n+:100644 100644 35d242ba79ae89ac695e26b3d4c27a8e6f028f9e 992913c5aa0a5476d10c49ed0f21fc0c6d1aedf3 M\tdir/sub\n+:100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d 10a8a9f3657f91a156b9f0184ed79a20adef9f7f M\tfile0\n+:000000 100644 0000000000000000000000000000000000000000 b1e67221afe8461efd244b487afca22d46b95eb8 A\tfile1\n+:100644 000000 01e79c32a8c99c557f0757da7cb6d65b3414466d 0000000000000000000000000000000000000000 D\tfile2\n+$\ndiff --git a/t/t4013/diff.diff_--raw_initial b/t/t4013/diff.diff_--raw_initial\nnew file mode 100644\nindex 0000000..a3e9780\n--- /dev/null\n+++ b/t/t4013/diff.diff_--raw_initial\n@@ -0,0 +1,6 @@\n+$ git diff --raw initial\n+:100644 100644 35d242b... 992913c... M\tdir/sub\n+:100644 100644 01e79c3... 10a8a9f... M\tfile0\n+:000000 100644 0000000... b1e6722... A\tfile1\n+:100644 000000 01e79c3... 0000000... D\tfile2\n+$\n-- \n2.10.2\n"},{"id":"306911","messageId":"20161205060116.szy5ojetg3znu4w7@sigill.intra.peff.net","threadId":"44557","inReplyTo":"20161202184840.2158-1-jack@nottheoilrig.com","subject":"Re: [PATCH v2] diff: handle --no-abbrev outside of repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-05T06:01:16Z","receivedAt":"2016-12-05T06:02:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 02, 2016 at 11:48:40AM -0700, Jack Bates wrote:\n\n> The \"git diff --no-index\" codepath didn't handle the --no-abbrev option.\n> Also it didn't behave the same as find_unique_abbrev()\n> in the case where abbrev == 0.\n> find_unique_abbrev() returns the full, unabbreviated string in that\n> case, but the \"git diff --no-index\" codepath returned an empty string.\n\nIf you've dug into what's wrong, I think it's often good to add some\nnotes in the commit message in case somebody has to revisit this later.\n\nFor example, I'd have written something like:\n\n  The \"git diff --no-index\" codepath doesn't handle the --no-abbrev\n  option, because it relies on diff_opt_parse(). Normally that function\n  is called as part of handle_revision_opt(), which handles the abbrev\n  options itself. Adding the option directly to diff_opt_parse() fixes\n  this. We don't need to do the same for --abbrev, because it's already\n  handled there.\n\n  Note that setting abbrev to \"0\" outside of a repository was broken\n  recently by 4f03666ac (diff: handle sha1 abbreviations outside of\n  repository, 2016-10-20). It adds a special out-of-repo code path for\n  handling abbreviations which behaves differently than find_unique_abbrev()\n  by truly giving a zero-length sha1, rather than taking \"0\" to mean \"do\n  not abbreviate\".\n\n  That bug was not triggerable until now, because there was no way to\n  set the value to zero (using --abbrev=0 silently bumps it to the\n  MINIMUM_ABBREV).\n\n>  t/t4013-diff-various.sh                                 | 7 +++++++\n>  t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir  | 3 +++\n>  t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir | 3 +++\n>  t/t4013/diff.diff_--no-index_--raw_dir2_dir             | 3 +++\n>  t/t4013/diff.diff_--raw_--abbrev=4_initial              | 6 ++++++\n>  t/t4013/diff.diff_--raw_--no-abbrev_initial             | 6 ++++++\n>  t/t4013/diff.diff_--raw_initial                         | 6 ++++++\n\nI wondered if the tests without --no-index were redundant with earlier\nones, but I don't think so. --abbrev=4 is tested with diff-tree, but\n--no-abbrev is not covered at all, AFAICT.\n\n>  diff.c                                                  | 6 +++++-\n\nThe actual code changes look good to me.\n\nThanks.\n\n-Peff\n"},{"id":"306912","messageId":"20161205061500.dinyc3juedkpw6o3@sigill.intra.peff.net","threadId":"44557","inReplyTo":"20161205060116.szy5ojetg3znu4w7@sigill.intra.peff.net","subject":"Re: [PATCH v2] diff: handle --no-abbrev outside of repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-05T06:15:00Z","receivedAt":"2016-12-05T06:15:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 05, 2016 at 01:01:16AM -0500, Jeff King wrote:\n\n>   Note that setting abbrev to \"0\" outside of a repository was broken\n>   recently by 4f03666ac (diff: handle sha1 abbreviations outside of\n>   repository, 2016-10-20). It adds a special out-of-repo code path for\n>   handling abbreviations which behaves differently than find_unique_abbrev()\n>   by truly giving a zero-length sha1, rather than taking \"0\" to mean \"do\n>   not abbreviate\".\n> \n>   That bug was not triggerable until now, because there was no way to\n>   set the value to zero (using --abbrev=0 silently bumps it to the\n>   MINIMUM_ABBREV).\n\nActually, I take this last paragraph back. You _can_ trigger the bug\nwith just:\n\n  echo one >foo\n  echo two >bar\n  git diff --no-index --raw foo bar\n\nwhich prints only \"...\" for each entry.\n\nI didn't notice it before because without \"--raw\", we show the patch\nformat. That uses the --full-index option, and does not respect --abbrev\nat all (which seems kind of bizarre, but has been that way forever).\n\nSo I think there _is_ a regression in v2.11, and the second half of your\nchange fixes it.\n\n-Peff\n"},{"id":"306914","messageId":"20161205065823.c7qw6xtc2hqk3xgu@sigill.intra.peff.net","threadId":"44557","inReplyTo":"20161205061500.dinyc3juedkpw6o3@sigill.intra.peff.net","subject":"Re: [PATCH v2] diff: handle --no-abbrev outside of repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-05T06:58:23Z","receivedAt":"2016-12-05T06:59:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 05, 2016 at 01:15:00AM -0500, Jeff King wrote:\n\n> On Mon, Dec 05, 2016 at 01:01:16AM -0500, Jeff King wrote:\n> \n> >   Note that setting abbrev to \"0\" outside of a repository was broken\n> >   recently by 4f03666ac (diff: handle sha1 abbreviations outside of\n> >   repository, 2016-10-20). It adds a special out-of-repo code path for\n> >   handling abbreviations which behaves differently than find_unique_abbrev()\n> >   by truly giving a zero-length sha1, rather than taking \"0\" to mean \"do\n> >   not abbreviate\".\n> > \n> >   That bug was not triggerable until now, because there was no way to\n> >   set the value to zero (using --abbrev=0 silently bumps it to the\n> >   MINIMUM_ABBREV).\n> \n> Actually, I take this last paragraph back. You _can_ trigger the bug\n> with just:\n> \n>   echo one >foo\n>   echo two >bar\n>   git diff --no-index --raw foo bar\n> \n> which prints only \"...\" for each entry.\n> \n> I didn't notice it before because without \"--raw\", we show the patch\n> format. That uses the --full-index option, and does not respect --abbrev\n> at all (which seems kind of bizarre, but has been that way forever).\n> \n> So I think there _is_ a regression in v2.11, and the second half of your\n> change fixes it.\n\nSorry for the sequence of emails, but as usual with \"diff --no-index\",\nthe deeper I dig the more confusion I find. :)\n\nAfter digging into your related thread in:\n\n  http://public-inbox.org/git/20161205065523.yspqt34p3dp5g5fk@sigill.intra.peff.net/\n\nI'm not convinced that \"--no-index --raw\" output isn't generally\nnonsensical in the first place. So yes, there's a regression there (and\nit's not just \"oops, we didn't abbreviate correctly\", but rather that\nthe output format is broken). But I'm not sure it's something people are\nusing. So it should be fixed on the 'maint' track, but I don't think\nit's incredibly urgent.\n\n-Peff\n"},{"id":"306988","messageId":"20161206010134.21856-1-jack@nottheoilrig.com","threadId":"44557","inReplyTo":"20161205065823.c7qw6xtc2hqk3xgu@sigill.intra.peff.net","subject":"[PATCH v3] diff: handle --no-abbrev in no-index case","fromName":"Jack Bates","fromEmail":"bk874k@nottheoilrig.com","sentAt":"2016-12-06T01:01:34Z","receivedAt":"2016-12-06T01:02:30Z","isPatch":true,"sender":{"key":"bk874k@nottheoilrig.com","avatar":null},"body":"There are two different places where the --no-abbrev option is parsed,\nand two different places where SHA-1s are abbreviated. We normally parse\n--no-abbrev with setup_revisions(), but in the no-index case, \"git diff\"\ncalls diff_opt_parse() directly, and diff_opt_parse() didn't handle\n--no-abbrev until now. (It did handle --abbrev, however.) We normally\nabbreviate SHA-1s with find_unique_abbrev(), but commit 4f03666 (\"diff:\nhandle sha1 abbreviations outside of repository, 2016-10-20) recently\nintroduced a special case when you run \"git diff\" outside of a\nrepository.\n\nsetup_revisions() does also call diff_opt_parse(), but not for --abbrev\nor --no-abbrev, which it handles itself. setup_revisions() sets\nrev_info->abbrev, and later copies that to diff_options->abbrev. It\nhandles --no-abbrev by setting abbrev to zero. (This change doesn't\ntouch that.)\n\nSetting abbrev to zero was broken in the outside-of-a-repository special\ncase, which until now resulted in a truly zero-length SHA-1, rather than\ntaking zero to mean do not abbreviate. The only way to trigger this bug,\nhowever, was by running \"git diff --raw\" without either the --abbrev or\n--no-abbrev options, because 1) without --raw it doesn't respect abbrev\n(which is bizarre, but has been that way forever), 2) we silently clamp\n--abbrev=0 to MINIMUM_ABBREV, and 3) --no-abbrev wasn't handled until\nnow.\n\nThe outside-of-a-repository case is one of three no-index cases. The\nother two are when one of the files you're comparing is outside of the\nrepository you're in, and the --no-index option.\n\nSigned-off-by: Jack Bates <jack@nottheoilrig.com>\n---\n diff.c                                                  | 6 +++++-\n t/t4013-diff-various.sh                                 | 7 +++++++\n t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir  | 3 +++\n t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir | 3 +++\n t/t4013/diff.diff_--no-index_--raw_dir2_dir             | 3 +++\n t/t4013/diff.diff_--raw_--abbrev=4_initial              | 6 ++++++\n t/t4013/diff.diff_--raw_--no-abbrev_initial             | 6 ++++++\n t/t4013/diff.diff_--raw_initial                         | 6 ++++++\n 8 files changed, 39 insertions(+), 1 deletion(-)\n create mode 100644 t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir\n create mode 100644 t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir\n create mode 100644 t/t4013/diff.diff_--no-index_--raw_dir2_dir\n create mode 100644 t/t4013/diff.diff_--raw_--abbrev=4_initial\n create mode 100644 t/t4013/diff.diff_--raw_--no-abbrev_initial\n create mode 100644 t/t4013/diff.diff_--raw_initial\n\ndiff --git a/diff.c b/diff.c\nindex ec87283..84dba60 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3106,7 +3106,8 @@ static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)\n \t\t\tabbrev = FALLBACK_DEFAULT_ABBREV;\n \t\tif (abbrev > GIT_SHA1_HEXSZ)\n \t\t\tdie(\"BUG: oid abbreviation out of range: %d\", abbrev);\n-\t\thex[abbrev] = '\\0';\n+\t\tif (abbrev)\n+\t\t\thex[abbrev] = '\\0';\n \t\treturn hex;\n \t}\n }\n@@ -3364,6 +3365,7 @@ void diff_setup(struct diff_options *options)\n \n \toptions->file = stdout;\n \n+\toptions->abbrev = DEFAULT_ABBREV;\n \toptions->line_termination = '\\n';\n \toptions->break_opt = -1;\n \toptions->rename_limit = -1;\n@@ -4024,6 +4026,8 @@ int diff_opt_parse(struct diff_options *options,\n \t\t\t    offending, optarg);\n \t\treturn argcount;\n \t}\n+\telse if (!strcmp(arg, \"--no-abbrev\"))\n+\t\toptions->abbrev = 0;\n \telse if (!strcmp(arg, \"--abbrev\"))\n \t\toptions->abbrev = DEFAULT_ABBREV;\n \telse if (skip_prefix(arg, \"--abbrev=\", &arg)) {\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 566817e..d7b71a0 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -311,6 +311,13 @@ diff --line-prefix=abc master master^ side\n diff --dirstat master~1 master~2\n diff --dirstat initial rearrange\n diff --dirstat-by-file initial rearrange\n+# --abbrev and --no-abbrev outside of repository\n+diff --raw initial\n+diff --raw --abbrev=4 initial\n+diff --raw --no-abbrev initial\n+diff --no-index --raw dir2 dir\n+diff --no-index --raw --abbrev=4 dir2 dir\n+diff --no-index --raw --no-abbrev dir2 dir\n EOF\n \n test_expect_success 'log -S requires an argument' '\ndiff --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\nnew file mode 100644\nindex 0000000..a71b38a\n--- /dev/null\n+++ b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir\n@@ -0,0 +1,3 @@\n+$ git diff --no-index --raw --abbrev=4 dir2 dir\n+:000000 100644 0000... 0000... A\tdir/sub\n+$\ndiff --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\nnew file mode 100644\nindex 0000000..e0f0097\n--- /dev/null\n+++ b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir\n@@ -0,0 +1,3 @@\n+$ git diff --no-index --raw --no-abbrev dir2 dir\n+:000000 100644 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 A\tdir/sub\n+$\ndiff --git a/t/t4013/diff.diff_--no-index_--raw_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_dir2_dir\nnew file mode 100644\nindex 0000000..3cb4ee7\n--- /dev/null\n+++ b/t/t4013/diff.diff_--no-index_--raw_dir2_dir\n@@ -0,0 +1,3 @@\n+$ git diff --no-index --raw dir2 dir\n+:000000 100644 0000000... 0000000... A\tdir/sub\n+$\ndiff --git a/t/t4013/diff.diff_--raw_--abbrev=4_initial b/t/t4013/diff.diff_--raw_--abbrev=4_initial\nnew file mode 100644\nindex 0000000..c3641db\n--- /dev/null\n+++ b/t/t4013/diff.diff_--raw_--abbrev=4_initial\n@@ -0,0 +1,6 @@\n+$ git diff --raw --abbrev=4 initial\n+:100644 100644 35d2... 9929... M\tdir/sub\n+:100644 100644 01e7... 10a8... M\tfile0\n+:000000 100644 0000... b1e6... A\tfile1\n+:100644 000000 01e7... 0000... D\tfile2\n+$\ndiff --git a/t/t4013/diff.diff_--raw_--no-abbrev_initial b/t/t4013/diff.diff_--raw_--no-abbrev_initial\nnew file mode 100644\nindex 0000000..c87a125\n--- /dev/null\n+++ b/t/t4013/diff.diff_--raw_--no-abbrev_initial\n@@ -0,0 +1,6 @@\n+$ git diff --raw --no-abbrev initial\n+:100644 100644 35d242ba79ae89ac695e26b3d4c27a8e6f028f9e 992913c5aa0a5476d10c49ed0f21fc0c6d1aedf3 M\tdir/sub\n+:100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d 10a8a9f3657f91a156b9f0184ed79a20adef9f7f M\tfile0\n+:000000 100644 0000000000000000000000000000000000000000 b1e67221afe8461efd244b487afca22d46b95eb8 A\tfile1\n+:100644 000000 01e79c32a8c99c557f0757da7cb6d65b3414466d 0000000000000000000000000000000000000000 D\tfile2\n+$\ndiff --git a/t/t4013/diff.diff_--raw_initial b/t/t4013/diff.diff_--raw_initial\nnew file mode 100644\nindex 0000000..a3e9780\n--- /dev/null\n+++ b/t/t4013/diff.diff_--raw_initial\n@@ -0,0 +1,6 @@\n+$ git diff --raw initial\n+:100644 100644 35d242b... 992913c... M\tdir/sub\n+:100644 100644 01e79c3... 10a8a9f... M\tfile0\n+:000000 100644 0000000... b1e6722... A\tfile1\n+:100644 000000 01e79c3... 0000000... D\tfile2\n+$\n-- \n2.10.2\n"},{"id":"307016","messageId":"20161206165346.20880-1-jack@nottheoilrig.com","threadId":"44557","inReplyTo":"20161206010134.21856-1-jack@nottheoilrig.com","subject":"[PATCH v4] diff: handle --no-abbrev in no-index case","fromName":"Jack Bates","fromEmail":"bk874k@nottheoilrig.com","sentAt":"2016-12-06T16:53:46Z","receivedAt":"2016-12-06T16:54:15Z","isPatch":true,"sender":{"key":"bk874k@nottheoilrig.com","avatar":null},"body":"There are two different places where the --no-abbrev option is parsed,\nand two different places where SHA-1s are abbreviated. We normally parse\n--no-abbrev with setup_revisions(), but in the no-index case, \"git diff\"\ncalls diff_opt_parse() directly, and diff_opt_parse() didn't handle\n--no-abbrev until now. (It did handle --abbrev, however.) We normally\nabbreviate SHA-1s with find_unique_abbrev(), but commit 4f03666 (\"diff:\nhandle sha1 abbreviations outside of repository, 2016-10-20) recently\nintroduced a special case when you run \"git diff\" outside of a\nrepository.\n\nsetup_revisions() does also call diff_opt_parse(), but not for --abbrev\nor --no-abbrev, which it handles itself. setup_revisions() sets\nrev_info->abbrev, and later copies that to diff_options->abbrev. It\nhandles --no-abbrev by setting abbrev to zero. (This change doesn't\ntouch that.)\n\nSetting abbrev to zero was broken in the outside-of-a-repository special\ncase, which until now resulted in a truly zero-length SHA-1, rather than\ntaking zero to mean do not abbreviate. The only way to trigger this bug,\nhowever, was by running \"git diff --raw\" without either the --abbrev or\n--no-abbrev options, because 1) without --raw it doesn't respect abbrev\n(which is bizarre, but has been that way forever), 2) we silently clamp\n--abbrev=0 to MINIMUM_ABBREV, and 3) --no-abbrev wasn't handled until\nnow.\n\nThe outside-of-a-repository case is one of three no-index cases. The\nother two are when one of the files you're comparing is outside of the\nrepository you're in, and the --no-index option.\n\nSigned-off-by: Jack Bates <jack@nottheoilrig.com>\n---\n diff.c                                                  | 6 +++++-\n t/t4013-diff-various.sh                                 | 7 +++++++\n t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir  | 3 +++\n t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir | 3 +++\n t/t4013/diff.diff_--no-index_--raw_dir2_dir             | 3 +++\n t/t4013/diff.diff_--raw_--abbrev=4_initial              | 6 ++++++\n t/t4013/diff.diff_--raw_--no-abbrev_initial             | 6 ++++++\n t/t4013/diff.diff_--raw_initial                         | 6 ++++++\n 8 files changed, 39 insertions(+), 1 deletion(-)\n create mode 100644 t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir\n create mode 100644 t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir\n create mode 100644 t/t4013/diff.diff_--no-index_--raw_dir2_dir\n create mode 100644 t/t4013/diff.diff_--raw_--abbrev=4_initial\n create mode 100644 t/t4013/diff.diff_--raw_--no-abbrev_initial\n create mode 100644 t/t4013/diff.diff_--raw_initial\n\ndiff --git a/diff.c b/diff.c\nindex ec87283..84dba60 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3106,7 +3106,8 @@ static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)\n \t\t\tabbrev = FALLBACK_DEFAULT_ABBREV;\n \t\tif (abbrev > GIT_SHA1_HEXSZ)\n \t\t\tdie(\"BUG: oid abbreviation out of range: %d\", abbrev);\n-\t\thex[abbrev] = '\\0';\n+\t\tif (abbrev)\n+\t\t\thex[abbrev] = '\\0';\n \t\treturn hex;\n \t}\n }\n@@ -3364,6 +3365,7 @@ void diff_setup(struct diff_options *options)\n \n \toptions->file = stdout;\n \n+\toptions->abbrev = DEFAULT_ABBREV;\n \toptions->line_termination = '\\n';\n \toptions->break_opt = -1;\n \toptions->rename_limit = -1;\n@@ -4024,6 +4026,8 @@ int diff_opt_parse(struct diff_options *options,\n \t\t\t    offending, optarg);\n \t\treturn argcount;\n \t}\n+\telse if (!strcmp(arg, \"--no-abbrev\"))\n+\t\toptions->abbrev = 0;\n \telse if (!strcmp(arg, \"--abbrev\"))\n \t\toptions->abbrev = DEFAULT_ABBREV;\n \telse if (skip_prefix(arg, \"--abbrev=\", &arg)) {\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 566817e..d7b71a0 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -311,6 +311,13 @@ diff --line-prefix=abc master master^ side\n diff --dirstat master~1 master~2\n diff --dirstat initial rearrange\n diff --dirstat-by-file initial rearrange\n+# --abbrev and --no-abbrev outside of repository\n+diff --raw initial\n+diff --raw --abbrev=4 initial\n+diff --raw --no-abbrev initial\n+diff --no-index --raw dir2 dir\n+diff --no-index --raw --abbrev=4 dir2 dir\n+diff --no-index --raw --no-abbrev dir2 dir\n EOF\n \n test_expect_success 'log -S requires an argument' '\ndiff --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\nnew file mode 100644\nindex 0000000..a71b38a\n--- /dev/null\n+++ b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir\n@@ -0,0 +1,3 @@\n+$ git diff --no-index --raw --abbrev=4 dir2 dir\n+:000000 100644 0000... 0000... A\tdir/sub\n+$\ndiff --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\nnew file mode 100644\nindex 0000000..e0f0097\n--- /dev/null\n+++ b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir\n@@ -0,0 +1,3 @@\n+$ git diff --no-index --raw --no-abbrev dir2 dir\n+:000000 100644 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 A\tdir/sub\n+$\ndiff --git a/t/t4013/diff.diff_--no-index_--raw_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_dir2_dir\nnew file mode 100644\nindex 0000000..3cb4ee7\n--- /dev/null\n+++ b/t/t4013/diff.diff_--no-index_--raw_dir2_dir\n@@ -0,0 +1,3 @@\n+$ git diff --no-index --raw dir2 dir\n+:000000 100644 0000000... 0000000... A\tdir/sub\n+$\ndiff --git a/t/t4013/diff.diff_--raw_--abbrev=4_initial b/t/t4013/diff.diff_--raw_--abbrev=4_initial\nnew file mode 100644\nindex 0000000..c3641db\n--- /dev/null\n+++ b/t/t4013/diff.diff_--raw_--abbrev=4_initial\n@@ -0,0 +1,6 @@\n+$ git diff --raw --abbrev=4 initial\n+:100644 100644 35d2... 9929... M\tdir/sub\n+:100644 100644 01e7... 10a8... M\tfile0\n+:000000 100644 0000... b1e6... A\tfile1\n+:100644 000000 01e7... 0000... D\tfile2\n+$\ndiff --git a/t/t4013/diff.diff_--raw_--no-abbrev_initial b/t/t4013/diff.diff_--raw_--no-abbrev_initial\nnew file mode 100644\nindex 0000000..c87a125\n--- /dev/null\n+++ b/t/t4013/diff.diff_--raw_--no-abbrev_initial\n@@ -0,0 +1,6 @@\n+$ git diff --raw --no-abbrev initial\n+:100644 100644 35d242ba79ae89ac695e26b3d4c27a8e6f028f9e 992913c5aa0a5476d10c49ed0f21fc0c6d1aedf3 M\tdir/sub\n+:100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d 10a8a9f3657f91a156b9f0184ed79a20adef9f7f M\tfile0\n+:000000 100644 0000000000000000000000000000000000000000 b1e67221afe8461efd244b487afca22d46b95eb8 A\tfile1\n+:100644 000000 01e79c32a8c99c557f0757da7cb6d65b3414466d 0000000000000000000000000000000000000000 D\tfile2\n+$\ndiff --git a/t/t4013/diff.diff_--raw_initial b/t/t4013/diff.diff_--raw_initial\nnew file mode 100644\nindex 0000000..a3e9780\n--- /dev/null\n+++ b/t/t4013/diff.diff_--raw_initial\n@@ -0,0 +1,6 @@\n+$ git diff --raw initial\n+:100644 100644 35d242b... 992913c... M\tdir/sub\n+:100644 100644 01e79c3... 10a8a9f... M\tfile0\n+:000000 100644 0000000... b1e6722... A\tfile1\n+:100644 000000 01e79c3... 0000000... D\tfile2\n+$\n-- \n2.10.2\n"},{"id":"307017","messageId":"20161206165614.22921-1-jack@nottheoilrig.com","threadId":"44557","inReplyTo":"20161206010134.21856-1-jack@nottheoilrig.com","subject":"[PATCH v4] diff: handle --no-abbrev in no-index case","fromName":"Jack Bates","fromEmail":"bk874k@nottheoilrig.com","sentAt":"2016-12-06T16:56:14Z","receivedAt":"2016-12-06T16:56:32Z","isPatch":true,"sender":{"key":"bk874k@nottheoilrig.com","avatar":null},"body":"There are two different places where the --no-abbrev option is parsed,\nand two different places where SHA-1s are abbreviated. We normally parse\n--no-abbrev with setup_revisions(), but in the no-index case, \"git diff\"\ncalls diff_opt_parse() directly, and diff_opt_parse() didn't handle\n--no-abbrev until now. (It did handle --abbrev, however.) We normally\nabbreviate SHA-1s with find_unique_abbrev(), but commit 4f03666 (\"diff:\nhandle sha1 abbreviations outside of repository, 2016-10-20) recently\nintroduced a special case when you run \"git diff\" outside of a\nrepository.\n\nsetup_revisions() does also call diff_opt_parse(), but not for --abbrev\nor --no-abbrev, which it handles itself. setup_revisions() sets\nrev_info->abbrev, and later copies that to diff_options->abbrev. It\nhandles --no-abbrev by setting abbrev to zero. (This change doesn't\ntouch that.)\n\nSetting abbrev to zero was broken in the outside-of-a-repository special\ncase, which until now resulted in a truly zero-length SHA-1, rather than\ntaking zero to mean do not abbreviate. The only way to trigger this bug,\nhowever, was by running \"git diff --raw\" without either the --abbrev or\n--no-abbrev options, because 1) without --raw it doesn't respect abbrev\n(which is bizarre, but has been that way forever), 2) we silently clamp\n--abbrev=0 to MINIMUM_ABBREV, and 3) --no-abbrev wasn't handled until\nnow.\n\nThe outside-of-a-repository case is one of three no-index cases. The\nother two are when one of the files you're comparing is outside of the\nrepository you're in, and the --no-index option.\n\nSigned-off-by: Jack Bates <jack@nottheoilrig.com>\n---\n diff.c                                                  | 6 +++++-\n t/t4013-diff-various.sh                                 | 7 +++++++\n t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir  | 3 +++\n t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir | 3 +++\n t/t4013/diff.diff_--no-index_--raw_dir2_dir             | 3 +++\n t/t4013/diff.diff_--raw_--abbrev=4_initial              | 6 ++++++\n t/t4013/diff.diff_--raw_--no-abbrev_initial             | 6 ++++++\n t/t4013/diff.diff_--raw_initial                         | 6 ++++++\n 8 files changed, 39 insertions(+), 1 deletion(-)\n create mode 100644 t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir\n create mode 100644 t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir\n create mode 100644 t/t4013/diff.diff_--no-index_--raw_dir2_dir\n create mode 100644 t/t4013/diff.diff_--raw_--abbrev=4_initial\n create mode 100644 t/t4013/diff.diff_--raw_--no-abbrev_initial\n create mode 100644 t/t4013/diff.diff_--raw_initial\n\ndiff --git a/diff.c b/diff.c\nindex ec87283..84dba60 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3106,7 +3106,8 @@ static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)\n \t\t\tabbrev = FALLBACK_DEFAULT_ABBREV;\n \t\tif (abbrev > GIT_SHA1_HEXSZ)\n \t\t\tdie(\"BUG: oid abbreviation out of range: %d\", abbrev);\n-\t\thex[abbrev] = '\\0';\n+\t\tif (abbrev)\n+\t\t\thex[abbrev] = '\\0';\n \t\treturn hex;\n \t}\n }\n@@ -3364,6 +3365,7 @@ void diff_setup(struct diff_options *options)\n \n \toptions->file = stdout;\n \n+\toptions->abbrev = DEFAULT_ABBREV;\n \toptions->line_termination = '\\n';\n \toptions->break_opt = -1;\n \toptions->rename_limit = -1;\n@@ -4024,6 +4026,8 @@ int diff_opt_parse(struct diff_options *options,\n \t\t\t    offending, optarg);\n \t\treturn argcount;\n \t}\n+\telse if (!strcmp(arg, \"--no-abbrev\"))\n+\t\toptions->abbrev = 0;\n \telse if (!strcmp(arg, \"--abbrev\"))\n \t\toptions->abbrev = DEFAULT_ABBREV;\n \telse if (skip_prefix(arg, \"--abbrev=\", &arg)) {\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 566817e..d09acfe 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -311,6 +311,13 @@ diff --line-prefix=abc master master^ side\n diff --dirstat master~1 master~2\n diff --dirstat initial rearrange\n diff --dirstat-by-file initial rearrange\n+# No-index --abbrev and --no-abbrev\n+diff --raw initial\n+diff --raw --abbrev=4 initial\n+diff --raw --no-abbrev initial\n+diff --no-index --raw dir2 dir\n+diff --no-index --raw --abbrev=4 dir2 dir\n+diff --no-index --raw --no-abbrev dir2 dir\n EOF\n \n test_expect_success 'log -S requires an argument' '\ndiff --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\nnew file mode 100644\nindex 0000000..a71b38a\n--- /dev/null\n+++ b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir\n@@ -0,0 +1,3 @@\n+$ git diff --no-index --raw --abbrev=4 dir2 dir\n+:000000 100644 0000... 0000... A\tdir/sub\n+$\ndiff --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\nnew file mode 100644\nindex 0000000..e0f0097\n--- /dev/null\n+++ b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir\n@@ -0,0 +1,3 @@\n+$ git diff --no-index --raw --no-abbrev dir2 dir\n+:000000 100644 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 A\tdir/sub\n+$\ndiff --git a/t/t4013/diff.diff_--no-index_--raw_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_dir2_dir\nnew file mode 100644\nindex 0000000..3cb4ee7\n--- /dev/null\n+++ b/t/t4013/diff.diff_--no-index_--raw_dir2_dir\n@@ -0,0 +1,3 @@\n+$ git diff --no-index --raw dir2 dir\n+:000000 100644 0000000... 0000000... A\tdir/sub\n+$\ndiff --git a/t/t4013/diff.diff_--raw_--abbrev=4_initial b/t/t4013/diff.diff_--raw_--abbrev=4_initial\nnew file mode 100644\nindex 0000000..c3641db\n--- /dev/null\n+++ b/t/t4013/diff.diff_--raw_--abbrev=4_initial\n@@ -0,0 +1,6 @@\n+$ git diff --raw --abbrev=4 initial\n+:100644 100644 35d2... 9929... M\tdir/sub\n+:100644 100644 01e7... 10a8... M\tfile0\n+:000000 100644 0000... b1e6... A\tfile1\n+:100644 000000 01e7... 0000... D\tfile2\n+$\ndiff --git a/t/t4013/diff.diff_--raw_--no-abbrev_initial b/t/t4013/diff.diff_--raw_--no-abbrev_initial\nnew file mode 100644\nindex 0000000..c87a125\n--- /dev/null\n+++ b/t/t4013/diff.diff_--raw_--no-abbrev_initial\n@@ -0,0 +1,6 @@\n+$ git diff --raw --no-abbrev initial\n+:100644 100644 35d242ba79ae89ac695e26b3d4c27a8e6f028f9e 992913c5aa0a5476d10c49ed0f21fc0c6d1aedf3 M\tdir/sub\n+:100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d 10a8a9f3657f91a156b9f0184ed79a20adef9f7f M\tfile0\n+:000000 100644 0000000000000000000000000000000000000000 b1e67221afe8461efd244b487afca22d46b95eb8 A\tfile1\n+:100644 000000 01e79c32a8c99c557f0757da7cb6d65b3414466d 0000000000000000000000000000000000000000 D\tfile2\n+$\ndiff --git a/t/t4013/diff.diff_--raw_initial b/t/t4013/diff.diff_--raw_initial\nnew file mode 100644\nindex 0000000..a3e9780\n--- /dev/null\n+++ b/t/t4013/diff.diff_--raw_initial\n@@ -0,0 +1,6 @@\n+$ git diff --raw initial\n+:100644 100644 35d242b... 992913c... M\tdir/sub\n+:100644 100644 01e79c3... 10a8a9f... M\tfile0\n+:000000 100644 0000000... b1e6722... A\tfile1\n+:100644 000000 01e79c3... 0000000... D\tfile2\n+$\n-- \n2.10.2\n"},{"id":"307018","messageId":"968b011f-c4b6-47af-ca98-8b2eae9a11d0@nottheoilrig.com","threadId":"44557","inReplyTo":"20161206165614.22921-1-jack@nottheoilrig.com","subject":"Re: [PATCH v4] diff: handle --no-abbrev in no-index case","fromName":"Jack Bates","fromEmail":"bk874k@nottheoilrig.com","sentAt":"2016-12-06T17:00:34Z","receivedAt":"2016-12-06T17:00:40Z","isPatch":true,"sender":{"key":"bk874k@nottheoilrig.com","avatar":null},"body":"On 06/12/16 09:56 AM, Jack Bates wrote:\n> There are two different places where the --no-abbrev option is parsed,\n> and two different places where SHA-1s are abbreviated. We normally parse\n> --no-abbrev with setup_revisions(), but in the no-index case, \"git diff\"\n> calls diff_opt_parse() directly, and diff_opt_parse() didn't handle\n> --no-abbrev until now. (It did handle --abbrev, however.) We normally\n> abbreviate SHA-1s with find_unique_abbrev(), but commit 4f03666 (\"diff:\n> handle sha1 abbreviations outside of repository, 2016-10-20) recently\n> introduced a special case when you run \"git diff\" outside of a\n> repository.\n>\n> setup_revisions() does also call diff_opt_parse(), but not for --abbrev\n> or --no-abbrev, which it handles itself. setup_revisions() sets\n> rev_info->abbrev, and later copies that to diff_options->abbrev. It\n> handles --no-abbrev by setting abbrev to zero. (This change doesn't\n> touch that.)\n>\n> Setting abbrev to zero was broken in the outside-of-a-repository special\n> case, which until now resulted in a truly zero-length SHA-1, rather than\n> taking zero to mean do not abbreviate. The only way to trigger this bug,\n> however, was by running \"git diff --raw\" without either the --abbrev or\n> --no-abbrev options, because 1) without --raw it doesn't respect abbrev\n> (which is bizarre, but has been that way forever), 2) we silently clamp\n> --abbrev=0 to MINIMUM_ABBREV, and 3) --no-abbrev wasn't handled until\n> now.\n>\n> The outside-of-a-repository case is one of three no-index cases. The\n> other two are when one of the files you're comparing is outside of the\n> repository you're in, and the --no-index option.\n>\n> Signed-off-by: Jack Bates <jack@nottheoilrig.com>\n> ---\n>  diff.c                                                  | 6 +++++-\n>  t/t4013-diff-various.sh                                 | 7 +++++++\n>  t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir  | 3 +++\n>  t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir | 3 +++\n>  t/t4013/diff.diff_--no-index_--raw_dir2_dir             | 3 +++\n>  t/t4013/diff.diff_--raw_--abbrev=4_initial              | 6 ++++++\n>  t/t4013/diff.diff_--raw_--no-abbrev_initial             | 6 ++++++\n>  t/t4013/diff.diff_--raw_initial                         | 6 ++++++\n>  8 files changed, 39 insertions(+), 1 deletion(-)\n>  create mode 100644 t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir\n>  create mode 100644 t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir\n>  create mode 100644 t/t4013/diff.diff_--no-index_--raw_dir2_dir\n>  create mode 100644 t/t4013/diff.diff_--raw_--abbrev=4_initial\n>  create mode 100644 t/t4013/diff.diff_--raw_--no-abbrev_initial\n>  create mode 100644 t/t4013/diff.diff_--raw_initial\n>\n> diff --git a/diff.c b/diff.c\n> index ec87283..84dba60 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -3106,7 +3106,8 @@ static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)\n>  \t\t\tabbrev = FALLBACK_DEFAULT_ABBREV;\n>  \t\tif (abbrev > GIT_SHA1_HEXSZ)\n>  \t\t\tdie(\"BUG: oid abbreviation out of range: %d\", abbrev);\n> -\t\thex[abbrev] = '\\0';\n> +\t\tif (abbrev)\n> +\t\t\thex[abbrev] = '\\0';\n>  \t\treturn hex;\n>  \t}\n>  }\n> @@ -3364,6 +3365,7 @@ void diff_setup(struct diff_options *options)\n>\n>  \toptions->file = stdout;\n>\n> +\toptions->abbrev = DEFAULT_ABBREV;\n>  \toptions->line_termination = '\\n';\n>  \toptions->break_opt = -1;\n>  \toptions->rename_limit = -1;\n> @@ -4024,6 +4026,8 @@ int diff_opt_parse(struct diff_options *options,\n>  \t\t\t    offending, optarg);\n>  \t\treturn argcount;\n>  \t}\n> +\telse if (!strcmp(arg, \"--no-abbrev\"))\n> +\t\toptions->abbrev = 0;\n>  \telse if (!strcmp(arg, \"--abbrev\"))\n>  \t\toptions->abbrev = DEFAULT_ABBREV;\n>  \telse if (skip_prefix(arg, \"--abbrev=\", &arg)) {\n> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\n> index 566817e..d09acfe 100755\n> --- a/t/t4013-diff-various.sh\n> +++ b/t/t4013-diff-various.sh\n> @@ -311,6 +311,13 @@ diff --line-prefix=abc master master^ side\n>  diff --dirstat master~1 master~2\n>  diff --dirstat initial rearrange\n>  diff --dirstat-by-file initial rearrange\n> +# No-index --abbrev and --no-abbrev\n\nI updated this comment to be consistent with the no-index vs. \noutside-of-a-repository distinction in the commit message.\n\n> +diff --raw initial\n> +diff --raw --abbrev=4 initial\n> +diff --raw --no-abbrev initial\n> +diff --no-index --raw dir2 dir\n> +diff --no-index --raw --abbrev=4 dir2 dir\n> +diff --no-index --raw --no-abbrev dir2 dir\n>  EOF\n>\n>  test_expect_success 'log -S requires an argument' '\n> 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\n> new file mode 100644\n> index 0000000..a71b38a\n> --- /dev/null\n> +++ b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir\n> @@ -0,0 +1,3 @@\n> +$ git diff --no-index --raw --abbrev=4 dir2 dir\n> +:000000 100644 0000... 0000... A\tdir/sub\n> +$\n> 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\n> new file mode 100644\n> index 0000000..e0f0097\n> --- /dev/null\n> +++ b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir\n> @@ -0,0 +1,3 @@\n> +$ git diff --no-index --raw --no-abbrev dir2 dir\n> +:000000 100644 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 A\tdir/sub\n> +$\n> diff --git a/t/t4013/diff.diff_--no-index_--raw_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_dir2_dir\n> new file mode 100644\n> index 0000000..3cb4ee7\n> --- /dev/null\n> +++ b/t/t4013/diff.diff_--no-index_--raw_dir2_dir\n> @@ -0,0 +1,3 @@\n> +$ git diff --no-index --raw dir2 dir\n> +:000000 100644 0000000... 0000000... A\tdir/sub\n> +$\n> diff --git a/t/t4013/diff.diff_--raw_--abbrev=4_initial b/t/t4013/diff.diff_--raw_--abbrev=4_initial\n> new file mode 100644\n> index 0000000..c3641db\n> --- /dev/null\n> +++ b/t/t4013/diff.diff_--raw_--abbrev=4_initial\n> @@ -0,0 +1,6 @@\n> +$ git diff --raw --abbrev=4 initial\n> +:100644 100644 35d2... 9929... M\tdir/sub\n> +:100644 100644 01e7... 10a8... M\tfile0\n> +:000000 100644 0000... b1e6... A\tfile1\n> +:100644 000000 01e7... 0000... D\tfile2\n> +$\n> diff --git a/t/t4013/diff.diff_--raw_--no-abbrev_initial b/t/t4013/diff.diff_--raw_--no-abbrev_initial\n> new file mode 100644\n> index 0000000..c87a125\n> --- /dev/null\n> +++ b/t/t4013/diff.diff_--raw_--no-abbrev_initial\n> @@ -0,0 +1,6 @@\n> +$ git diff --raw --no-abbrev initial\n> +:100644 100644 35d242ba79ae89ac695e26b3d4c27a8e6f028f9e 992913c5aa0a5476d10c49ed0f21fc0c6d1aedf3 M\tdir/sub\n> +:100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d 10a8a9f3657f91a156b9f0184ed79a20adef9f7f M\tfile0\n> +:000000 100644 0000000000000000000000000000000000000000 b1e67221afe8461efd244b487afca22d46b95eb8 A\tfile1\n> +:100644 000000 01e79c32a8c99c557f0757da7cb6d65b3414466d 0000000000000000000000000000000000000000 D\tfile2\n> +$\n> diff --git a/t/t4013/diff.diff_--raw_initial b/t/t4013/diff.diff_--raw_initial\n> new file mode 100644\n> index 0000000..a3e9780\n> --- /dev/null\n> +++ b/t/t4013/diff.diff_--raw_initial\n> @@ -0,0 +1,6 @@\n> +$ git diff --raw initial\n> +:100644 100644 35d242b... 992913c... M\tdir/sub\n> +:100644 100644 01e79c3... 10a8a9f... M\tfile0\n> +:000000 100644 0000000... b1e6722... A\tfile1\n> +:100644 000000 01e79c3... 0000000... D\tfile2\n> +$\n>\n"},{"id":"307312","messageId":"xmqqtwaerq1x.fsf@gitster.mtv.corp.google.com","threadId":"44557","inReplyTo":"20161206165614.22921-1-jack@nottheoilrig.com","subject":"Re: [PATCH v4] diff: handle --no-abbrev in no-index case","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-08T22:53:14Z","receivedAt":"2016-12-08T22:53:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jack Bates <bk874k@nottheoilrig.com> writes:\n\n> There are two different places where the --no-abbrev option is parsed,\n> and two different places where SHA-1s are abbreviated. We normally parse\n> --no-abbrev with setup_revisions(), but in the no-index case, \"git diff\"\n> calls diff_opt_parse() directly, and diff_opt_parse() didn't handle\n> --no-abbrev until now. (It did handle --abbrev, however.) We normally\n> abbreviate SHA-1s with find_unique_abbrev(), but commit 4f03666 (\"diff:\n> handle sha1 abbreviations outside of repository, 2016-10-20) recently\n> introduced a special case when you run \"git diff\" outside of a\n> repository.\n>\n> setup_revisions() does also call diff_opt_parse(), but not for --abbrev\n> or --no-abbrev, which it handles itself. setup_revisions() sets\n> rev_info->abbrev, and later copies that to diff_options->abbrev. It\n> handles --no-abbrev by setting abbrev to zero. (This change doesn't\n> touch that.)\n>\n> Setting abbrev to zero was broken in the outside-of-a-repository special\n> case, which until now resulted in a truly zero-length SHA-1, rather than\n> taking zero to mean do not abbreviate. The only way to trigger this bug,\n> however, was by running \"git diff --raw\" without either the --abbrev or\n> --no-abbrev options, because 1) without --raw it doesn't respect abbrev\n> (which is bizarre, but has been that way forever), 2) we silently clamp\n> --abbrev=0 to MINIMUM_ABBREV, and 3) --no-abbrev wasn't handled until\n> now.\n>\n> The outside-of-a-repository case is one of three no-index cases. The\n> other two are when one of the files you're comparing is outside of the\n> repository you're in, and the --no-index option.\n\nNicely described.  \n\n> diff --git a/diff.c b/diff.c\n> index ec87283..84dba60 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -3106,7 +3106,8 @@ static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)\n>  \t\t\tabbrev = FALLBACK_DEFAULT_ABBREV;\n>  \t\tif (abbrev > GIT_SHA1_HEXSZ)\n>  \t\t\tdie(\"BUG: oid abbreviation out of range: %d\", abbrev);\n> -\t\thex[abbrev] = '\\0';\n> +\t\tif (abbrev)\n> +\t\t\thex[abbrev] = '\\0';\n>  \t\treturn hex;\n>  \t}\n>  }\n\nThis is the same since your earlier round and it is correct.  The\ncode before this part clamps abbrev to be between 0 and 40.\n\n> @@ -3364,6 +3365,7 @@ void diff_setup(struct diff_options *options)\n>  \n>  \toptions->file = stdout;\n>  \n> +\toptions->abbrev = DEFAULT_ABBREV;\n\nThis is a new change relative to your earlier one.  \n\nI looked at all the callers of diff_setup() and noticed that many of\nthem were initializing \"struct diff_options\" that is on-stack that\nis totally uninitialized, which means they were using a completely\nrandom value that happened to be on the stack.  \n\nWhich was surprising and made me wonder how the entire \"diff\" code\ncould have ever worked correctly for the past 10 years, as it's not\nlike all the users always passed --[no-]abbrev[=<value>] from the\ncommand line.\n\nIn any case, this cannot possibly be introducing a regression; these\ncallsites of diff_setup() were starting from a random garbage---now\nthey start with -1 in this field.  If they were doing the right\nthing by assigning their own abbrev to the field after diff_setup()\nreturned, they will continue to do the same, and otherwise they will\nkeep doing whatever random things they have been doing when the\nuninitialized field happened to contain -1 the same way.\n\nI didn't look carefully at the additional tests, but the code change\nlooks good.\n\nThanks.\n\n"},{"id":"307319","messageId":"763ddb21-f735-342b-45a4-2f22e03dd346@nottheoilrig.com","threadId":"44557","inReplyTo":"xmqqtwaerq1x.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4] diff: handle --no-abbrev in no-index case","fromName":"Jack Bates","fromEmail":"bk874k@nottheoilrig.com","sentAt":"2016-12-09T00:22:01Z","receivedAt":"2016-12-09T00:22:08Z","isPatch":true,"sender":{"key":"bk874k@nottheoilrig.com","avatar":null},"body":"On 08/12/16 03:53 PM, Junio C Hamano wrote:\n> Jack Bates <bk874k@nottheoilrig.com> writes:\n>> @@ -3364,6 +3365,7 @@ void diff_setup(struct diff_options *options)\n>>\n>>  \toptions->file = stdout;\n>>\n>> +\toptions->abbrev = DEFAULT_ABBREV;\n>\n> This is a new change relative to your earlier one.\n>\n> I looked at all the callers of diff_setup() and noticed that many of\n> them were initializing \"struct diff_options\" that is on-stack that\n> is totally uninitialized, which means they were using a completely\n> random value that happened to be on the stack.\n>\n> Which was surprising and made me wonder how the entire \"diff\" code\n> could have ever worked correctly for the past 10 years, as it's not\n> like all the users always passed --[no-]abbrev[=<value>] from the\n> command line.\n>\n> In any case, this cannot possibly be introducing a regression; these\n> callsites of diff_setup() were starting from a random garbage---now\n> they start with -1 in this field.  If they were doing the right\n> thing by assigning their own abbrev to the field after diff_setup()\n> returned, they will continue to do the same, and otherwise they will\n> keep doing whatever random things they have been doing when the\n> uninitialized field happened to contain -1 the same way.\n>\n> I didn't look carefully at the additional tests, but the code change\n> looks good.\n>\n> Thanks.\n\nGreat, thanks for reviewing it!\n"}]}