* [PATCH] diff: handle --no-abbrev outside of repository @ 2016-11-28 18:25 Jack Bates 2016-11-28 23:03 ` Junio C Hamano 2016-11-29 7:06 ` Jeff King 0 siblings, 2 replies; 13+ messages in thread From: Jack Bates @ 2016-11-28 18:25 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] diff: handle --no-abbrev outside of repository 2016-11-28 18:25 [PATCH] diff: handle --no-abbrev outside of repository Jack Bates @ 2016-11-28 23:03 ` Junio C Hamano 2016-11-29 7:06 ` Jeff King 1 sibling, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2016-11-28 23:03 UTC (permalink / raw) To: Jack Bates; +Cc: git, Jack Bates 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)) { ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] diff: handle --no-abbrev outside of repository 2016-11-28 18:25 [PATCH] diff: handle --no-abbrev outside of repository Jack Bates 2016-11-28 23:03 ` Junio C Hamano @ 2016-11-29 7:06 ` Jeff King 2016-12-02 18:48 ` [PATCH v2] " Jack Bates 1 sibling, 1 reply; 13+ messages in thread From: Jeff King @ 2016-11-29 7:06 UTC (permalink / raw) To: Jack Bates; +Cc: git, Jack Bates 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] diff: handle --no-abbrev outside of repository 2016-11-29 7:06 ` Jeff King @ 2016-12-02 18:48 ` Jack Bates 2016-12-05 6:01 ` Jeff King 0 siblings, 1 reply; 13+ messages in thread From: Jack Bates @ 2016-12-02 18:48 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2] diff: handle --no-abbrev outside of repository 2016-12-02 18:48 ` [PATCH v2] " Jack Bates @ 2016-12-05 6:01 ` Jeff King 2016-12-05 6:15 ` Jeff King 0 siblings, 1 reply; 13+ messages in thread From: Jeff King @ 2016-12-05 6:01 UTC (permalink / raw) To: Jack Bates; +Cc: git, Junio C Hamano, Jack Bates 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] diff: handle --no-abbrev outside of repository 2016-12-05 6:01 ` Jeff King @ 2016-12-05 6:15 ` Jeff King 2016-12-05 6:58 ` Jeff King 0 siblings, 1 reply; 13+ messages in thread From: Jeff King @ 2016-12-05 6:15 UTC (permalink / raw) To: Jack Bates; +Cc: git, Junio C Hamano, Jack Bates 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] diff: handle --no-abbrev outside of repository 2016-12-05 6:15 ` Jeff King @ 2016-12-05 6:58 ` Jeff King 2016-12-06 1:01 ` [PATCH v3] diff: handle --no-abbrev in no-index case Jack Bates 0 siblings, 1 reply; 13+ messages in thread From: Jeff King @ 2016-12-05 6:58 UTC (permalink / raw) To: Jack Bates; +Cc: git, Junio C Hamano, Jack Bates 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3] diff: handle --no-abbrev in no-index case 2016-12-05 6:58 ` Jeff King @ 2016-12-06 1:01 ` Jack Bates 2016-12-06 16:53 ` [PATCH v4] " Jack Bates 2016-12-06 16:56 ` Jack Bates 0 siblings, 2 replies; 13+ messages in thread From: Jack Bates @ 2016-12-06 1:01 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4] diff: handle --no-abbrev in no-index case 2016-12-06 1:01 ` [PATCH v3] diff: handle --no-abbrev in no-index case Jack Bates @ 2016-12-06 16:53 ` Jack Bates 2016-12-06 16:56 ` Jack Bates 1 sibling, 0 replies; 13+ messages in thread From: Jack Bates @ 2016-12-06 16:53 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4] diff: handle --no-abbrev in no-index case 2016-12-06 1:01 ` [PATCH v3] diff: handle --no-abbrev in no-index case Jack Bates 2016-12-06 16:53 ` [PATCH v4] " Jack Bates @ 2016-12-06 16:56 ` Jack Bates 2016-12-06 17:00 ` Jack Bates 2016-12-08 22:53 ` Junio C Hamano 1 sibling, 2 replies; 13+ messages in thread From: Jack Bates @ 2016-12-06 16:56 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v4] diff: handle --no-abbrev in no-index case 2016-12-06 16:56 ` Jack Bates @ 2016-12-06 17:00 ` Jack Bates 2016-12-08 22:53 ` Junio C Hamano 1 sibling, 0 replies; 13+ messages in thread From: Jack Bates @ 2016-12-06 17:00 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King 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 > +$ > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4] diff: handle --no-abbrev in no-index case 2016-12-06 16:56 ` Jack Bates 2016-12-06 17:00 ` Jack Bates @ 2016-12-08 22:53 ` Junio C Hamano 2016-12-09 0:22 ` Jack Bates 1 sibling, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2016-12-08 22:53 UTC (permalink / raw) To: Jack Bates; +Cc: git, Jeff King, Jack Bates 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4] diff: handle --no-abbrev in no-index case 2016-12-08 22:53 ` Junio C Hamano @ 2016-12-09 0:22 ` Jack Bates 0 siblings, 0 replies; 13+ messages in thread From: Jack Bates @ 2016-12-09 0:22 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King 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! ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-12-09 0:22 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-28 18:25 [PATCH] diff: handle --no-abbrev outside of repository Jack Bates 2016-11-28 23:03 ` Junio C Hamano 2016-11-29 7:06 ` Jeff King 2016-12-02 18:48 ` [PATCH v2] " Jack Bates 2016-12-05 6:01 ` Jeff King 2016-12-05 6:15 ` Jeff King 2016-12-05 6:58 ` Jeff King 2016-12-06 1:01 ` [PATCH v3] diff: handle --no-abbrev in no-index case Jack Bates 2016-12-06 16:53 ` [PATCH v4] " Jack Bates 2016-12-06 16:56 ` Jack Bates 2016-12-06 17:00 ` Jack Bates 2016-12-08 22:53 ` Junio C Hamano 2016-12-09 0:22 ` Jack Bates
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.