| Message ID | a37dfb3748e23b4f5081bc9a3c80a5c546101f1d.1694383248.git.code@khaugsbakk.name (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | range-diff: treat notes like `log` | expand |
Kristoffer Haugsbakk <code@khaugsbakk.name> writes: >> To fix this explicitly set the output format of the internally executed >> `git log` with `--pretty=medium`. Because that cancels `--notes`, add >> explicitly `--notes` at the end. > > § Authors > > • Fix by Johannes > • Tests by Kristoffer > > † 1: See e.g. 66b2ed09c2 (Fix "log" family not to be too agressive about > showing notes, 2010-01-20). > > Co-authored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> > Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> > --- OK, Dscho, does this round look acceptable to you? It feels UGLY to iterate over args _without_ actually parsing them, at least to me. Such a non-parsing look breaks at least in two ways over time. (1) a mechanism may be introduced laster, similar to "--", that allows other_arg->v[] array to mark "here is where the dashed options end". Now the existing loop keeps reading to the end and finds "--notes" that is not a dashed option but is part of the normal command line arguments in "other arg". (2) Among the dashed options passed in the other_arg->v[], there may be an option that takes a string value, and a value that happens to be "--notes" is mistaken as asking for "notes" (iow "git log -G --notes" is looking for commits with changes that contain "double dash followed by en oh tee ee es"). I think "git range-diff -G --notes" (or "-S --notes") shows that this new non-parsing loop is already broken. It looks for a change that has "--notes" correctly, but at the same time, triggers that "ah, we have an explicit --notes so drop the implicit_notes_arg flag" logic. > range-diff.c | 13 +++++++++++-- > t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++ > 2 files changed, 39 insertions(+), 2 deletions(-) > > diff --git a/range-diff.c b/range-diff.c > index 2e86063491..fbb81a92cc 100644 > --- a/range-diff.c > +++ b/range-diff.c > @@ -41,12 +41,20 @@ static int read_patches(const char *range, struct string_list *list, > struct child_process cp = CHILD_PROCESS_INIT; > struct strbuf buf = STRBUF_INIT, contents = STRBUF_INIT; > struct patch_util *util = NULL; > - int in_header = 1; > + int i, implicit_notes_arg = 1, in_header = 1; > char *line, *current_filename = NULL; > ssize_t len; > size_t size; > int ret = -1; > > + for (i = 0; other_arg && i < other_arg->nr; i++) > + if (!strcmp(other_arg->v[i], "--notes") || > + starts_with(other_arg->v[i], "--notes=") || > + !strcmp(other_arg->v[i], "--no-notes")) { > + implicit_notes_arg = 0; > + break; > + } > + > strvec_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges", > "--reverse", "--date-order", "--decorate=no", > "--no-prefix", "--submodule=short", > @@ -60,8 +68,9 @@ static int read_patches(const char *range, struct string_list *list, > "--output-indicator-context=#", > "--no-abbrev-commit", > "--pretty=medium", > - "--notes", > NULL); > + if (implicit_notes_arg) > + strvec_push(&cp.args, "--notes"); > strvec_push(&cp.args, range); > if (other_arg) > strvec_pushv(&cp.args, other_arg->v); > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh > index b5f4d6a653..b33afa1c6a 100755 > --- a/t/t3206-range-diff.sh > +++ b/t/t3206-range-diff.sh > @@ -662,6 +662,20 @@ test_expect_success 'range-diff with multiple --notes' ' > test_cmp expect actual > ' > > +# `range-diff` should act like `log` with regards to notes > +test_expect_success 'range-diff with --notes=custom does not show default notes' ' > + git notes add -m "topic note" topic && > + git notes add -m "unmodified note" unmodified && > + git notes --ref=custom add -m "topic note" topic && > + git notes --ref=custom add -m "unmodified note" unmodified && > + test_when_finished git notes remove topic unmodified && > + test_when_finished git notes --ref=custom remove topic unmodified && > + git range-diff --notes=custom main..topic main..unmodified \ > + >actual && > + ! grep "## Notes ##" actual && > + grep "## Notes (custom) ##" actual > +' > + > test_expect_success 'format-patch --range-diff does not compare notes by default' ' > git notes add -m "topic note" topic && > git notes add -m "unmodified note" unmodified && > @@ -679,6 +693,20 @@ test_expect_success 'format-patch --range-diff does not compare notes by default > ! grep "note" 0000-* > ' > > +test_expect_success 'format-patch --notes=custom --range-diff only compares custom notes' ' > + git notes add -m "topic note" topic && > + git notes --ref=custom add -m "topic note (custom)" topic && > + git notes add -m "unmodified note" unmodified && > + git notes --ref=custom add -m "unmodified note (custom)" unmodified && > + test_when_finished git notes remove topic unmodified && > + test_when_finished git notes --ref=custom remove topic unmodified && > + git format-patch --notes=custom --cover-letter --range-diff=$prev \ > + main..unmodified >actual && > + test_when_finished "rm 000?-*" && > + grep "## Notes (custom) ##" 0000-* && > + ! grep "## Notes ##" 0000-* > +' > + > test_expect_success 'format-patch --range-diff with --no-notes' ' > git notes add -m "topic note" topic && > git notes add -m "unmodified note" unmodified &&
Hi Junio, On Mon, 11 Sep 2023, Junio C Hamano wrote: > Kristoffer Haugsbakk <code@khaugsbakk.name> writes: > > >> To fix this explicitly set the output format of the internally executed > >> `git log` with `--pretty=medium`. Because that cancels `--notes`, add > >> explicitly `--notes` at the end. > > > > § Authors > > > > • Fix by Johannes > > • Tests by Kristoffer > > > > † 1: See e.g. 66b2ed09c2 (Fix "log" family not to be too agressive about > > showing notes, 2010-01-20). > > > > Co-authored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> > > Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> > > --- > > OK, Dscho, does this round look acceptable to you? > > It feels UGLY to iterate over args _without_ actually parsing them, > at least to me. Such a non-parsing look breaks at least in two ways > over time. (1) a mechanism may be introduced laster, similar to > "--", that allows other_arg->v[] array to mark "here is where the > dashed options end". Now the existing loop keeps reading to the end > and finds "--notes" that is not a dashed option but is part of the > normal command line arguments in "other arg". (2) Among the dashed > options passed in the other_arg->v[], there may be an option that > takes a string value, and a value that happens to be "--notes" is > mistaken as asking for "notes" (iow "git log -G --notes" is looking > for commits with changes that contain "double dash followed by en oh > tee ee es"). > > I think "git range-diff -G --notes" (or "-S --notes") shows that > this new non-parsing loop is already broken. It looks for a change > that has "--notes" correctly, but at the same time, triggers that > "ah, we have an explicit --notes so drop the implicit_notes_arg > flag" logic. Right, `-G --notes` is a good argument to rethink this. A much more surgical way to address the issue at hand might be this (Kristoffer, what do you think? It's missing documentation for the newly-introduced `--show-notes-by-default` option, but you get the idea): -- snipsnap -- diff --git a/range-diff.c b/range-diff.c index fbb81a92cc0..56f6870ff91 100644 --- a/range-diff.c +++ b/range-diff.c @@ -41,20 +41,12 @@ static int read_patches(const char *range, struct string_list *list, struct child_process cp = CHILD_PROCESS_INIT; struct strbuf buf = STRBUF_INIT, contents = STRBUF_INIT; struct patch_util *util = NULL; - int i, implicit_notes_arg = 1, in_header = 1; + int in_header = 1; char *line, *current_filename = NULL; ssize_t len; size_t size; int ret = -1; - for (i = 0; other_arg && i < other_arg->nr; i++) - if (!strcmp(other_arg->v[i], "--notes") || - starts_with(other_arg->v[i], "--notes=") || - !strcmp(other_arg->v[i], "--no-notes")) { - implicit_notes_arg = 0; - break; - } - strvec_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges", "--reverse", "--date-order", "--decorate=no", "--no-prefix", "--submodule=short", @@ -68,9 +60,8 @@ static int read_patches(const char *range, struct string_list *list, "--output-indicator-context=#", "--no-abbrev-commit", "--pretty=medium", + "--show-notes-by-default", NULL); - if (implicit_notes_arg) - strvec_push(&cp.args, "--notes"); strvec_push(&cp.args, range); if (other_arg) strvec_pushv(&cp.args, other_arg->v); diff --git a/revision.c b/revision.c index 2f4c53ea207..49d385257ac 100644 --- a/revision.c +++ b/revision.c @@ -2484,6 +2484,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg revs->break_bar = xstrdup(optarg); revs->track_linear = 1; revs->track_first_time = 1; + } else if (!strcmp(arg, "--show-notes-by-default")) { + revs->show_notes_by_default = 1; } else if (skip_prefix(arg, "--show-notes=", &optarg) || skip_prefix(arg, "--notes=", &optarg)) { if (starts_with(arg, "--show-notes=") && @@ -3054,6 +3056,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s if (revs->expand_tabs_in_log < 0) revs->expand_tabs_in_log = revs->expand_tabs_in_log_default; + if (!revs->show_notes_given && revs->show_notes_by_default) { + enable_default_display_notes(&revs->notes_opt, &revs->show_notes); + revs->show_notes_given = 1; + } + return left; } diff --git a/revision.h b/revision.h index 82ab400139d..50091bbd13f 100644 --- a/revision.h +++ b/revision.h @@ -253,6 +253,7 @@ struct rev_info { shown_dashes:1, show_merge:1, show_notes_given:1, + show_notes_by_default:1, show_signature:1, pretty_given:1, abbrev_commit:1,
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > A much more surgical way to address the issue at hand might be this > (Kristoffer, what do you think? It's missing documentation for the > newly-introduced `--show-notes-by-default` option, but you get the idea): Clever. I was wondering if the option parsing done by the revision machinery was readily available for us to use from range-diff code, but because we are forking out to "log" anyway, giving it the new "--show-notes-by-default" option does sound like a much easier way out. It would help something like [alias] ln = !"git log --show-notes-by-default" as well. Nicely designed.
On Thu, Sep 14, 2023, at 10:29, Johannes Schindelin wrote: >> [...] > > Right, `-G --notes` is a good argument to rethink this. > > A much more surgical way to address the issue at hand might be this > (Kristoffer, what do you think? It's missing documentation for the > newly-introduced `--show-notes-by-default` option, but you get the idea): Looks good to me. It seems like an explicit argument is the only way to make this work.
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes: > On Thu, Sep 14, 2023, at 10:29, Johannes Schindelin wrote: >>> [...] >> >> Right, `-G --notes` is a good argument to rethink this. >> >> A much more surgical way to address the issue at hand might be this >> (Kristoffer, what do you think? It's missing documentation for the >> newly-introduced `--show-notes-by-default` option, but you get the idea): > > Looks good to me. It seems like an explicit argument is the only way to > make this work. Will one of you tie the loose ends? Thanks.
On Tue, Sep 19, 2023, at 03:16, Junio C Hamano wrote: > Will one of you tie the loose ends? > > Thanks. I will this afternoon (CEST). Cheers
diff --git a/range-diff.c b/range-diff.c index 2e86063491..fbb81a92cc 100644 --- a/range-diff.c +++ b/range-diff.c @@ -41,12 +41,20 @@ static int read_patches(const char *range, struct string_list *list, struct child_process cp = CHILD_PROCESS_INIT; struct strbuf buf = STRBUF_INIT, contents = STRBUF_INIT; struct patch_util *util = NULL; - int in_header = 1; + int i, implicit_notes_arg = 1, in_header = 1; char *line, *current_filename = NULL; ssize_t len; size_t size; int ret = -1; + for (i = 0; other_arg && i < other_arg->nr; i++) + if (!strcmp(other_arg->v[i], "--notes") || + starts_with(other_arg->v[i], "--notes=") || + !strcmp(other_arg->v[i], "--no-notes")) { + implicit_notes_arg = 0; + break; + } + strvec_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges", "--reverse", "--date-order", "--decorate=no", "--no-prefix", "--submodule=short", @@ -60,8 +68,9 @@ static int read_patches(const char *range, struct string_list *list, "--output-indicator-context=#", "--no-abbrev-commit", "--pretty=medium", - "--notes", NULL); + if (implicit_notes_arg) + strvec_push(&cp.args, "--notes"); strvec_push(&cp.args, range); if (other_arg) strvec_pushv(&cp.args, other_arg->v); diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh index b5f4d6a653..b33afa1c6a 100755 --- a/t/t3206-range-diff.sh +++ b/t/t3206-range-diff.sh @@ -662,6 +662,20 @@ test_expect_success 'range-diff with multiple --notes' ' test_cmp expect actual ' +# `range-diff` should act like `log` with regards to notes +test_expect_success 'range-diff with --notes=custom does not show default notes' ' + git notes add -m "topic note" topic && + git notes add -m "unmodified note" unmodified && + git notes --ref=custom add -m "topic note" topic && + git notes --ref=custom add -m "unmodified note" unmodified && + test_when_finished git notes remove topic unmodified && + test_when_finished git notes --ref=custom remove topic unmodified && + git range-diff --notes=custom main..topic main..unmodified \ + >actual && + ! grep "## Notes ##" actual && + grep "## Notes (custom) ##" actual +' + test_expect_success 'format-patch --range-diff does not compare notes by default' ' git notes add -m "topic note" topic && git notes add -m "unmodified note" unmodified && @@ -679,6 +693,20 @@ test_expect_success 'format-patch --range-diff does not compare notes by default ! grep "note" 0000-* ' +test_expect_success 'format-patch --notes=custom --range-diff only compares custom notes' ' + git notes add -m "topic note" topic && + git notes --ref=custom add -m "topic note (custom)" topic && + git notes add -m "unmodified note" unmodified && + git notes --ref=custom add -m "unmodified note (custom)" unmodified && + test_when_finished git notes remove topic unmodified && + test_when_finished git notes --ref=custom remove topic unmodified && + git format-patch --notes=custom --cover-letter --range-diff=$prev \ + main..unmodified >actual && + test_when_finished "rm 000?-*" && + grep "## Notes (custom) ##" 0000-* && + ! grep "## Notes ##" 0000-* +' + test_expect_success 'format-patch --range-diff with --no-notes' ' git notes add -m "topic note" topic && git notes add -m "unmodified note" unmodified &&
Currently, `range-diff` shows the default notes if no notes-related arguments are given. This is also how `log` behaves. But unlike `range-diff`, `log` does *not* show the default notes if `--notes=<custom>` are given. In other words, this: git log --notes=custom is equivalent to this: git log --no-notes --notes=custom While: git range-diff --notes=custom acts like this: git log --notes --notes-custom This can’t be how the user expects `range-diff` to behave given that the man page for `range-diff` under `--[no-]notes[=<ref>]` says: > This flag is passed to the `git log` program (see git-log(1)) that > generates the patches. This behavior also affects `format-patch` since it uses `range-diff` for the cover letter. Unlike `log`, though, `format-patch` is not supposed to show the default notes if no notes-related arguments are given.[1] But this promise is broken when the range-diff happens to have something to say about the changes to the default notes, since that will be shown in the cover letter. Remedy this by only conditionally passing in `--notes` to `range-diff`. § Root cause 8cf51561d1e (range-diff: fix a crash in parsing git-log output, 2020-04-15) added `--notes` in order to deal with a side-effect of `--pretty=medium`: > To fix this explicitly set the output format of the internally executed > `git log` with `--pretty=medium`. Because that cancels `--notes`, add > explicitly `--notes` at the end. § Authors • Fix by Johannes • Tests by Kristoffer † 1: See e.g. 66b2ed09c2 (Fix "log" family not to be too agressive about showing notes, 2010-01-20). Co-authored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> --- range-diff.c | 13 +++++++++++-- t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-)