* [RFC PATCH 0/3] range-diff: treat notes like `log`
@ 2023-05-30 10:41 Kristoffer Haugsbakk
2023-05-30 10:41 ` [RFC PATCH 1/3] " Kristoffer Haugsbakk
` (3 more replies)
0 siblings, 4 replies; 41+ messages in thread
From: Kristoffer Haugsbakk @ 2023-05-30 10:41 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, Jeff King, Junio C Hamano, Denton Liu
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.
These changes are supposed to make `format-range` behave like `log` with
regards to notes.
These changes also fixes an issue with notes being shared in the cover
letter via `range-diff`, and that’s really the main motivation for
making these changes.
§ How `log` works
`log` shows the default notes if no notes arguments are given. But if
you give it specific notes to show then it forgets about the default
notes. Further, there is the convenience `--notes` option which will
show the default notes again. These options are cumulative. For example:
git log --notes --notes=custom
Will show the default notes as well as the `custom` notes.
See discussion in: https://lore.kernel.org/git/20110329143357.GA10771@sigill.intra.peff.net/
§ How `range-format` works
`range-format` passes `--notes` to `log`, which means that it does not
have the default behavior of `log` (forget the default logs if you say
e.g. `--notes=custom`). However, the man page says that (under
`--[no-]notes[=<ref>]`):
> This flag is passed to the git log program (see git-log(1)) that generates the patches.
This makes me (at least) think that `range-format` is supposed to work
just like `log` with regards to notes.
§ `format-patch` and the difference between showing and sharing
`format-patch` has a different default: it shows no notes. This makes
sense in my opinion since `format-patch` is meant to be used to share
changes with others, and you might be surprised if your notes (which
might have only been notes to yourself) are sent out in your emails
(keep in mind that notes refs are *not* pushed by default).
But the slightly faulty behavior of `range-diff` bleeds through to
`format-patch` since the latter calls the former; if you have default
notes they can be shared in the range-diff on the cover letter, even
though `format-patch` isn’t supposed to show them.
§ Code layout and approach to the problem
As I’ve said, my focus was on fixing `format-patch`, so I’ve considered
how `format-patch` calls `range-diff` which in turn calls `log`.
`format-patch` is a command which is defined in `builtin/log.c`. For
notes in particular it in fact has some explicit logic for handling
notes based on the value of `rev`. (There seems to be no issues with
this part of the code; only the code in `range-diff.c` which passes the
default argument to `log`.) It then calls
`range-diff.c:show_range_diff`. That function on `master` loads some
default arguments, among them `--notes`. It then eventually calls `log`
as a subprocess.
My change consists of co-opting the deprecated `--standard-notes` and
changing its behavior so that it can be used in
`range-diff.c:show_range_diff`.
Using a special switch/option was the only way I found in order to fix
this problem.
I could have also created a new option but I thought that doing a
smaller change initially would be better.
§ RFC
This is marked as RFC since I chose to co-opt a deprecated option in
order to get `range-format` to work, in my opinion, as it should.
§ Carbon copy
Based on `contrib/contacts/git-contacts master..@`.
Kristoffer Haugsbakk (3):
range-diff: treat notes like `log`
doc: pretty-options: remove documentation for deprecated options
revision: comment `--no-standard-notes` as deprecated
Documentation/pretty-options.txt | 1 -
range-diff.c | 2 +-
revision.c | 8 ++++++--
t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++
4 files changed, 35 insertions(+), 4 deletions(-)
--
2.41.0.rc2
^ permalink raw reply [flat|nested] 41+ messages in thread* [RFC PATCH 1/3] range-diff: treat notes like `log` 2023-05-30 10:41 [RFC PATCH 0/3] range-diff: treat notes like `log` Kristoffer Haugsbakk @ 2023-05-30 10:41 ` Kristoffer Haugsbakk 2023-06-01 18:20 ` Jeff King 2023-05-30 10:41 ` [RFC PATCH 2/3] doc: pretty-options: remove documentation for deprecated options Kristoffer Haugsbakk ` (2 subsequent siblings) 3 siblings, 1 reply; 41+ messages in thread From: Kristoffer Haugsbakk @ 2023-05-30 10:41 UTC (permalink / raw) To: git; +Cc: Kristoffer Haugsbakk, Jeff King, Junio C Hamano, Denton Liu 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 co-opting the `--standard-notes` option which has been deprecated since ab18b2c0df[2] and which is currently only documented in `pretty-options`. † 1: See e.g. 66b2ed09c2 (Fix "log" family not to be too agressive about showing notes, 2010-01-20). † 2: log/pretty-options: Document --[no-]notes and deprecate old notes options, 2011-03-30 Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> --- range-diff.c | 2 +- revision.c | 7 +++++-- t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++ 3 files changed, 34 insertions(+), 3 deletions(-) diff --git a/range-diff.c b/range-diff.c index 6a704e6f47..2c92af337f 100644 --- a/range-diff.c +++ b/range-diff.c @@ -59,7 +59,7 @@ static int read_patches(const char *range, struct string_list *list, "--output-indicator-context=#", "--no-abbrev-commit", "--pretty=medium", - "--notes", + "--standard-notes", NULL); strvec_push(&cp.args, range); if (other_arg) diff --git a/revision.c b/revision.c index b33cc1d106..a0ab7fb784 100644 --- a/revision.c +++ b/revision.c @@ -2524,8 +2524,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg disable_display_notes(&revs->notes_opt, &revs->show_notes); revs->show_notes_given = 1; } else if (!strcmp(arg, "--standard-notes")) { - revs->show_notes_given = 1; - revs->notes_opt.use_default_notes = 1; + disable_display_notes(&revs->notes_opt, &revs->show_notes); + revs->show_notes_given = 0; + enable_default_display_notes(&revs->notes_opt, + &revs->show_notes); + revs->notes_opt.use_default_notes = -1; } else if (!strcmp(arg, "--no-standard-notes")) { revs->notes_opt.use_default_notes = 0; } else if (!strcmp(arg, "--oneline")) { 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 && -- 2.41.0.rc2 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [RFC PATCH 1/3] range-diff: treat notes like `log` 2023-05-30 10:41 ` [RFC PATCH 1/3] " Kristoffer Haugsbakk @ 2023-06-01 18:20 ` Jeff King 2023-06-02 10:06 ` Kristoffer Haugsbakk 0 siblings, 1 reply; 41+ messages in thread From: Jeff King @ 2023-06-01 18:20 UTC (permalink / raw) To: Kristoffer Haugsbakk; +Cc: git, Junio C Hamano, Denton Liu On Tue, May 30, 2023 at 12:41:29PM +0200, Kristoffer Haugsbakk wrote: > 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. Yeah, I certainly agree that the behavior of range-diff is surprising, and that this is a bug. I'd have expected the solution here would be for range-diff to stop passing "--notes", and let "log" decide whether to show notes (based on specific --notes=foo it gets from other_arg). But... > 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 co-opting the `--standard-notes` option which has been > deprecated since ab18b2c0df[2] and which is currently only documented in > `pretty-options`. I'm not clear on whether you're actually fixing two separate bugs here, or if they need to be intertwined. It seems like passing --standard-notes means that format-patch's range-diff will still show the standard notes by default. Maybe I'm misunderstanding the problem, though. -Peff ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RFC PATCH 1/3] range-diff: treat notes like `log` 2023-06-01 18:20 ` Jeff King @ 2023-06-02 10:06 ` Kristoffer Haugsbakk 0 siblings, 0 replies; 41+ messages in thread From: Kristoffer Haugsbakk @ 2023-06-02 10:06 UTC (permalink / raw) To: Jeff King; +Cc: git, Junio C Hamano, Denton Liu Hi Peff! On Thu, Jun 1, 2023, at 20:20, Jeff King wrote: >> 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. > > Yeah, I certainly agree that the behavior of range-diff is surprising, > and that this is a bug. > > I'd have expected the solution here would be for range-diff to stop > passing "--notes", and let "log" decide whether to show notes (based on > specific --notes=foo it gets from other_arg). I tried that (removing `--notes`) but it didn’t work out for me. I’d had hoped that `--notes` was just a holdover from a previous change that wasn’t needed any more, but it seems according to my testing that `range diff` needs to pass *some* notes-related argument to `log` in order for the test suite to pass. Thinking about it again now it doesn’t really make sense in the isolated case of `range-diff`. But maybe it’s needed in conjunction with `format-patch`. A relevant series is “range-diff: learn `--notes`” by Denton Liu (f3c7bfdde2 (Merge branch 'dl/range-diff-with-notes', 2019-12-05)).[1] On the commit before that merge, `range-diff.c` does not pass `--notes` to `log`. Denton describes the behavior in the first cover letter: > When I was using range-diff at $DAYJOB earlier, I realised that it > includes commit notes as part of the commit message comparison. So just like the default behavior of `log`. However, the next version (v2) is about teaching `range-diff` to pass notes down to `log`: > This patchset teaches range-diff and format-patch to pass `--notes` > options down to the `git log` machinery. This should make the > behaviour more configurable for users who either don't want notes to > be displayed or want multiple notes refs to be displayed. So apparently `range-diff` couldn’t pass on/down `--notes=<ref>` before that. That series also teaches `format-patch` to handle notes in 5b583e6a09 (format-patch: pass notes configuration to range-diff, 2019-11-20). [1] https://lore.kernel.org/git/cover.1574125554.git.liu.denton@gmail.com/ > > But... > >> 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 co-opting the `--standard-notes` option which has been >> deprecated since ab18b2c0df[2] and which is currently only documented in >> `pretty-options`. > > I'm not clear on whether you're actually fixing two separate bugs here, > or if they need to be intertwined. I guess it depends on your perspective. ;) I would say that they are so intertwined that fixing one also fixes the other. > It seems like passing --standard-notes means that format-patch's > range-diff will still show the standard notes by default. Maybe I'm > misunderstanding the problem, though. No, that should still work. See the test `format-patch --range-diff does not compare notes by default`. That kind of thing seems to be handled by `log.c:get_notes_args`; `--no-notes` is sent to `log` if no notes should be shown. Cheers -- Kristoffer Haugsbakk ^ permalink raw reply [flat|nested] 41+ messages in thread
* [RFC PATCH 2/3] doc: pretty-options: remove documentation for deprecated options 2023-05-30 10:41 [RFC PATCH 0/3] range-diff: treat notes like `log` Kristoffer Haugsbakk 2023-05-30 10:41 ` [RFC PATCH 1/3] " Kristoffer Haugsbakk @ 2023-05-30 10:41 ` Kristoffer Haugsbakk 2023-05-30 10:41 ` [RFC PATCH 3/3] revision: comment `--no-standard-notes` as deprecated Kristoffer Haugsbakk 2023-06-11 18:15 ` [PATCH v1 0/3] range-diff: treat notes like `log` Kristoffer Haugsbakk 3 siblings, 0 replies; 41+ messages in thread From: Kristoffer Haugsbakk @ 2023-05-30 10:41 UTC (permalink / raw) To: git; +Cc: Kristoffer Haugsbakk, Jeff King, Junio C Hamano, Denton Liu Remove documentation for options which were deprecated in ab18b2c0df (log/pretty-options: Document --[no-]notes and deprecate old notes options, 2011-03-30). We now use `--standard-notes` internally for `range-diff` (see previous commit). Leave `--show-notes` as-is since we aren’t changing anything about it. Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> --- Documentation/pretty-options.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt index dc685be363..8c982609c9 100644 --- a/Documentation/pretty-options.txt +++ b/Documentation/pretty-options.txt @@ -88,7 +88,6 @@ being displayed. Examples: "--notes=foo" will show only notes from from "refs/notes/bar". --show-notes[=<ref>]:: ---[no-]standard-notes:: These options are deprecated. Use the above --notes/--no-notes options instead. endif::git-rev-list[] -- 2.41.0.rc2 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* [RFC PATCH 3/3] revision: comment `--no-standard-notes` as deprecated 2023-05-30 10:41 [RFC PATCH 0/3] range-diff: treat notes like `log` Kristoffer Haugsbakk 2023-05-30 10:41 ` [RFC PATCH 1/3] " Kristoffer Haugsbakk 2023-05-30 10:41 ` [RFC PATCH 2/3] doc: pretty-options: remove documentation for deprecated options Kristoffer Haugsbakk @ 2023-05-30 10:41 ` Kristoffer Haugsbakk 2023-06-11 18:15 ` [PATCH v1 0/3] range-diff: treat notes like `log` Kristoffer Haugsbakk 3 siblings, 0 replies; 41+ messages in thread From: Kristoffer Haugsbakk @ 2023-05-30 10:41 UTC (permalink / raw) To: git; +Cc: Kristoffer Haugsbakk, Jeff King, Junio C Hamano, Denton Liu We still use `--standard-notes` but this option has no use and is no longer documented anywhere. Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> --- revision.c | 1 + 1 file changed, 1 insertion(+) diff --git a/revision.c b/revision.c index a0ab7fb784..24219c741a 100644 --- a/revision.c +++ b/revision.c @@ -2529,6 +2529,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg enable_default_display_notes(&revs->notes_opt, &revs->show_notes); revs->notes_opt.use_default_notes = -1; + /* Deprecated */ } else if (!strcmp(arg, "--no-standard-notes")) { revs->notes_opt.use_default_notes = 0; } else if (!strcmp(arg, "--oneline")) { -- 2.41.0.rc2 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v1 0/3] range-diff: treat notes like `log` 2023-05-30 10:41 [RFC PATCH 0/3] range-diff: treat notes like `log` Kristoffer Haugsbakk ` (2 preceding siblings ...) 2023-05-30 10:41 ` [RFC PATCH 3/3] revision: comment `--no-standard-notes` as deprecated Kristoffer Haugsbakk @ 2023-06-11 18:15 ` Kristoffer Haugsbakk 2023-06-11 18:15 ` [PATCH v1 1/3] " Kristoffer Haugsbakk ` (4 more replies) 3 siblings, 5 replies; 41+ messages in thread From: Kristoffer Haugsbakk @ 2023-06-11 18:15 UTC (permalink / raw) To: git; +Cc: Kristoffer Haugsbakk, Jeff King, Junio C Hamano, Denton Liu This is the first normal (not RFC) version of this series. The following cover letter is identical to the previous one up to but not including section “RFC and version 1”. Cheers 🙛 🙙 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. These changes are supposed to make `format-range` behave like `log` with regards to notes. These changes also fixes an issue with notes being shared in the cover letter via `range-diff`, and that’s really the main motivation for making these changes. § How `log` works `log` shows the default notes if no notes arguments are given. But if you give it specific notes to show then it forgets about the default notes. Further, there is the convenience `--notes` option which will show the default notes again. These options are cumulative. For example: git log --notes --notes=custom Will show the default notes as well as the `custom` notes. See discussion in: https://lore.kernel.org/git/20110329143357.GA10771@sigill.intra.peff.net/ § How `range-format` works `range-format` passes `--notes` to `log`, which means that it does not have the default behavior of `log` (forget the default logs if you say e.g. `--notes=custom`). However, the man page says that (under `--[no-]notes[=<ref>]`): > This flag is passed to the git log program (see git-log(1)) that generates the patches. This makes me (at least) think that `range-format` is supposed to work just like `log` with regards to notes. § `format-patch` and the difference between showing and sharing `format-patch` has a different default: it shows no notes. This makes sense in my opinion since `format-patch` is meant to be used to share changes with others, and you might be surprised if your notes (which might have only been notes to yourself) are sent out in your emails (keep in mind that notes refs are *not* pushed by default). But the slightly faulty behavior of `range-diff` bleeds through to `format-patch` since the latter calls the former; if you have default notes they can be shared in the range-diff on the cover letter, even though `format-patch` isn’t supposed to show them. § Code layout and approach to the problem As I’ve said, my focus was on fixing `format-patch`, so I’ve considered how `format-patch` calls `range-diff` which in turn calls `log`. `format-patch` is a command which is defined in `builtin/log.c`. For notes in particular it in fact has some explicit logic for handling notes based on the value of `rev`. (There seems to be no issues with this part of the code; only the code in `range-diff.c` which passes the default argument to `log`.) It then calls `range-diff.c:show_range_diff`. That function on `master` loads some default arguments, among them `--notes`. It then eventually calls `log` as a subprocess. My change consists of co-opting the deprecated `--standard-notes` and changing its behavior so that it can be used in `range-diff.c:show_range_diff`. Using a special switch/option was the only way I found in order to fix this problem. I could have also created a new option but I thought that doing a smaller change initially would be better. § RFC and version 1 The previous sendout was an RFC. There are no changes since the RFC. The series was rebased on `v2.41.0`. Kristoffer Haugsbakk (3): range-diff: treat notes like `log` doc: pretty-options: remove documentation for deprecated options revision: comment `--no-standard-notes` as deprecated Documentation/pretty-options.txt | 1 - range-diff.c | 2 +- revision.c | 8 ++++++-- t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++ 4 files changed, 35 insertions(+), 4 deletions(-) -- 2.41.0 ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v1 1/3] range-diff: treat notes like `log` 2023-06-11 18:15 ` [PATCH v1 0/3] range-diff: treat notes like `log` Kristoffer Haugsbakk @ 2023-06-11 18:15 ` Kristoffer Haugsbakk 2023-06-11 18:15 ` [PATCH v1 2/3] doc: pretty-options: remove documentation for deprecated options Kristoffer Haugsbakk ` (3 subsequent siblings) 4 siblings, 0 replies; 41+ messages in thread From: Kristoffer Haugsbakk @ 2023-06-11 18:15 UTC (permalink / raw) To: git; +Cc: Kristoffer Haugsbakk, Jeff King, Junio C Hamano, Denton Liu 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 co-opting the `--standard-notes` option which has been deprecated since ab18b2c0df[2] and which is currently only documented in `pretty-options`. † 1: See e.g. 66b2ed09c2 (Fix "log" family not to be too agressive about showing notes, 2010-01-20). † 2: log/pretty-options: Document --[no-]notes and deprecate old notes options, 2011-03-30 Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> --- range-diff.c | 2 +- revision.c | 7 +++++-- t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++ 3 files changed, 34 insertions(+), 3 deletions(-) diff --git a/range-diff.c b/range-diff.c index 6a704e6f47..2c92af337f 100644 --- a/range-diff.c +++ b/range-diff.c @@ -59,7 +59,7 @@ static int read_patches(const char *range, struct string_list *list, "--output-indicator-context=#", "--no-abbrev-commit", "--pretty=medium", - "--notes", + "--standard-notes", NULL); strvec_push(&cp.args, range); if (other_arg) diff --git a/revision.c b/revision.c index b33cc1d106..a0ab7fb784 100644 --- a/revision.c +++ b/revision.c @@ -2524,8 +2524,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg disable_display_notes(&revs->notes_opt, &revs->show_notes); revs->show_notes_given = 1; } else if (!strcmp(arg, "--standard-notes")) { - revs->show_notes_given = 1; - revs->notes_opt.use_default_notes = 1; + disable_display_notes(&revs->notes_opt, &revs->show_notes); + revs->show_notes_given = 0; + enable_default_display_notes(&revs->notes_opt, + &revs->show_notes); + revs->notes_opt.use_default_notes = -1; } else if (!strcmp(arg, "--no-standard-notes")) { revs->notes_opt.use_default_notes = 0; } else if (!strcmp(arg, "--oneline")) { 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 && -- 2.41.0 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v1 2/3] doc: pretty-options: remove documentation for deprecated options 2023-06-11 18:15 ` [PATCH v1 0/3] range-diff: treat notes like `log` Kristoffer Haugsbakk 2023-06-11 18:15 ` [PATCH v1 1/3] " Kristoffer Haugsbakk @ 2023-06-11 18:15 ` Kristoffer Haugsbakk 2023-06-11 18:15 ` [PATCH v1 3/3] revision: comment `--no-standard-notes` as deprecated Kristoffer Haugsbakk ` (2 subsequent siblings) 4 siblings, 0 replies; 41+ messages in thread From: Kristoffer Haugsbakk @ 2023-06-11 18:15 UTC (permalink / raw) To: git; +Cc: Kristoffer Haugsbakk, Jeff King, Junio C Hamano, Denton Liu Remove documentation for options which were deprecated in ab18b2c0df (log/pretty-options: Document --[no-]notes and deprecate old notes options, 2011-03-30). We now use `--standard-notes` internally for `range-diff` (see previous commit). Leave `--show-notes` as-is since we aren’t changing anything about it. Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> --- Documentation/pretty-options.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt index dc685be363..8c982609c9 100644 --- a/Documentation/pretty-options.txt +++ b/Documentation/pretty-options.txt @@ -88,7 +88,6 @@ being displayed. Examples: "--notes=foo" will show only notes from from "refs/notes/bar". --show-notes[=<ref>]:: ---[no-]standard-notes:: These options are deprecated. Use the above --notes/--no-notes options instead. endif::git-rev-list[] -- 2.41.0 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v1 3/3] revision: comment `--no-standard-notes` as deprecated 2023-06-11 18:15 ` [PATCH v1 0/3] range-diff: treat notes like `log` Kristoffer Haugsbakk 2023-06-11 18:15 ` [PATCH v1 1/3] " Kristoffer Haugsbakk 2023-06-11 18:15 ` [PATCH v1 2/3] doc: pretty-options: remove documentation for deprecated options Kristoffer Haugsbakk @ 2023-06-11 18:15 ` Kristoffer Haugsbakk 2023-06-12 22:21 ` Junio C Hamano 2023-06-12 22:25 ` [PATCH v1 0/3] range-diff: treat notes like `log` Junio C Hamano 2023-09-01 16:18 ` [PATCH v2 " Kristoffer Haugsbakk 4 siblings, 1 reply; 41+ messages in thread From: Kristoffer Haugsbakk @ 2023-06-11 18:15 UTC (permalink / raw) To: git; +Cc: Kristoffer Haugsbakk, Jeff King, Junio C Hamano, Denton Liu We still use `--standard-notes` but this option has no use and is no longer documented anywhere. Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> --- revision.c | 1 + 1 file changed, 1 insertion(+) diff --git a/revision.c b/revision.c index a0ab7fb784..24219c741a 100644 --- a/revision.c +++ b/revision.c @@ -2529,6 +2529,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg enable_default_display_notes(&revs->notes_opt, &revs->show_notes); revs->notes_opt.use_default_notes = -1; + /* Deprecated */ } else if (!strcmp(arg, "--no-standard-notes")) { revs->notes_opt.use_default_notes = 0; } else if (!strcmp(arg, "--oneline")) { -- 2.41.0 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v1 3/3] revision: comment `--no-standard-notes` as deprecated 2023-06-11 18:15 ` [PATCH v1 3/3] revision: comment `--no-standard-notes` as deprecated Kristoffer Haugsbakk @ 2023-06-12 22:21 ` Junio C Hamano 2023-06-13 9:46 ` Kristoffer Haugsbakk 0 siblings, 1 reply; 41+ messages in thread From: Junio C Hamano @ 2023-06-12 22:21 UTC (permalink / raw) To: Kristoffer Haugsbakk; +Cc: git, Jeff King, Denton Liu Kristoffer Haugsbakk <code@khaugsbakk.name> writes: > We still use `--standard-notes` but this option has no use and is no > longer documented anywhere. > > Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> > --- > revision.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/revision.c b/revision.c > index a0ab7fb784..24219c741a 100644 > --- a/revision.c > +++ b/revision.c > @@ -2529,6 +2529,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg > enable_default_display_notes(&revs->notes_opt, > &revs->show_notes); > revs->notes_opt.use_default_notes = -1; > + /* Deprecated */ > } else if (!strcmp(arg, "--no-standard-notes")) { > revs->notes_opt.use_default_notes = 0; > } else if (!strcmp(arg, "--oneline")) { With the placement of this new comment, it is unclear which one between "--standard-notes" and "--no-standard-notes" is getting deprecated (actually, the comment is placed inside the block for the former, so it may be more natural to interpret that the comment marks the former as deprecated). } else if (!strcmp(arg, "--no-standard-notes")) { + /* Deprecated */ revs->notes_opt.use_default_notes = 0; } I am not commenting if it makes sense to declare that the option is deprecated here---I'll leave it to others to argue for/against it. The usual reasoning to add/maintain "--no-foo" is so that we can get "cmd --foo --no-foo" to naturally work, but it does not apply here? ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v1 3/3] revision: comment `--no-standard-notes` as deprecated 2023-06-12 22:21 ` Junio C Hamano @ 2023-06-13 9:46 ` Kristoffer Haugsbakk 0 siblings, 0 replies; 41+ messages in thread From: Kristoffer Haugsbakk @ 2023-06-13 9:46 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Denton Liu, Johannes Schindelin On Tue, Jun 13, 2023, at 00:21, Junio C Hamano wrote: >> [snip] >> diff --git a/revision.c b/revision.c >> index a0ab7fb784..24219c741a 100644 >> --- a/revision.c >> +++ b/revision.c >> @@ -2529,6 +2529,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg >> enable_default_display_notes(&revs->notes_opt, >> &revs->show_notes); >> revs->notes_opt.use_default_notes = -1; >> + /* Deprecated */ >> } else if (!strcmp(arg, "--no-standard-notes")) { >> revs->notes_opt.use_default_notes = 0; >> } else if (!strcmp(arg, "--oneline")) { > > With the placement of this new comment, it is unclear which one > between "--standard-notes" and "--no-standard-notes" is getting > deprecated (actually, the comment is placed inside the block for the > former, so it may be more natural to interpret that the comment > marks the former as deprecated). > > } else if (!strcmp(arg, "--no-standard-notes")) { > + /* Deprecated */ > revs->notes_opt.use_default_notes = 0; > } Thanks, I’ll use that in the next version if this patch is still included. > I am not commenting if it makes sense to declare that the option is > deprecated here---I'll leave it to others to argue for/against it. > The usual reasoning to add/maintain "--no-foo" is so that we can get > "cmd --foo --no-foo" to naturally work, but it does not apply here? Yeah, because I decided to piggyback on a deprecated command in order to make the change. So this change moves `--standard-notes` from “deprecated” to “for internal use”, while `--no-standard-notes`’s status remains the same, which is “deprecated”. Unless the symmetry of `--[no-]standard-notes` turns out to be useful. Like I’ve said before, I would have preferred to find a way to solve this issue without using an internal switch. But I wasn’t able to. Cheers -- Kristoffer Haugsbakk ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v1 0/3] range-diff: treat notes like `log` 2023-06-11 18:15 ` [PATCH v1 0/3] range-diff: treat notes like `log` Kristoffer Haugsbakk ` (2 preceding siblings ...) 2023-06-11 18:15 ` [PATCH v1 3/3] revision: comment `--no-standard-notes` as deprecated Kristoffer Haugsbakk @ 2023-06-12 22:25 ` Junio C Hamano 2023-06-13 5:43 ` Kristoffer Haugsbakk 2023-09-01 16:18 ` [PATCH v2 " Kristoffer Haugsbakk 4 siblings, 1 reply; 41+ messages in thread From: Junio C Hamano @ 2023-06-12 22:25 UTC (permalink / raw) To: Kristoffer Haugsbakk; +Cc: git, Jeff King, Denton Liu, Johannes Schindelin Kristoffer Haugsbakk <code@khaugsbakk.name> writes: > This is the first normal (not RFC) version of this series. The following > cover letter is identical to the previous one up to but not including > section “RFC and version 1”. I have no strong opinion on the topic and did not comment on the RFC version, either, so I am puzzled to be CC'ed, especially when the primary author of the feature is not. Dscho, how does this series look? Thanks. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v1 0/3] range-diff: treat notes like `log` 2023-06-12 22:25 ` [PATCH v1 0/3] range-diff: treat notes like `log` Junio C Hamano @ 2023-06-13 5:43 ` Kristoffer Haugsbakk 0 siblings, 0 replies; 41+ messages in thread From: Kristoffer Haugsbakk @ 2023-06-13 5:43 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Denton Liu, Johannes Schindelin On Tue, Jun 13, 2023, at 00:25, Junio C Hamano wrote: > Kristoffer Haugsbakk <code@khaugsbakk.name> writes: > >> This is the first normal (not RFC) version of this series. The following >> cover letter is identical to the previous one up to but not including >> section “RFC and version 1”. > > I have no strong opinion on the topic and did not comment on the RFC > version, either, so I am puzzled to be CC'ed, especially when the > primary author of the feature is not. The CC list is the output of `contrib/contacts/git-contacts master..HEAD`. I’ll include Dscho if I make another series that touches this part of the tree. Cheers -- Kristoffer Haugsbakk ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v2 0/3] range-diff: treat notes like `log` 2023-06-11 18:15 ` [PATCH v1 0/3] range-diff: treat notes like `log` Kristoffer Haugsbakk ` (3 preceding siblings ...) 2023-06-12 22:25 ` [PATCH v1 0/3] range-diff: treat notes like `log` Junio C Hamano @ 2023-09-01 16:18 ` Kristoffer Haugsbakk 2023-09-01 16:19 ` [PATCH v2 1/3] " Kristoffer Haugsbakk ` (3 more replies) 4 siblings, 4 replies; 41+ messages in thread From: Kristoffer Haugsbakk @ 2023-09-01 16:18 UTC (permalink / raw) To: git; +Cc: Johannes Schindelin, Denton Liu, Jeff King, Kristoffer Haugsbakk The following cover letter is identical to the previous one up to but not including section “Changes since version 1”. Cheers 🙛 🙙 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. These changes are supposed to make `format-range` behave like `log` with regards to notes. These changes also fixes an issue with notes being shared in the cover letter via `range-diff`, and that’s really the main motivation for making these changes. § How `log` works `log` shows the default notes if no notes arguments are given. But if you give it specific notes to show then it forgets about the default notes. Further, there is the convenience `--notes` option which will show the default notes again. These options are cumulative. For example: git log --notes --notes=custom Will show the default notes as well as the `custom` notes. See discussion in: https://lore.kernel.org/git/20110329143357.GA10771@sigill.intra.peff.net/ § How `range-format` works `range-format` passes `--notes` to `log`, which means that it does not have the default behavior of `log` (forget the default logs if you say e.g. `--notes=custom`). However, the man page says that (under `--[no-]notes[=<ref>]`): > This flag is passed to the git log program (see git-log(1)) that generates the patches. This makes me (at least) think that `range-format` is supposed to work just like `log` with regards to notes. § `format-patch` and the difference between showing and sharing `format-patch` has a different default: it shows no notes. This makes sense in my opinion since `format-patch` is meant to be used to share changes with others, and you might be surprised if your notes (which might have only been notes to yourself) are sent out in your emails (keep in mind that notes refs are *not* pushed by default). But the slightly faulty behavior of `range-diff` bleeds through to `format-patch` since the latter calls the former; if you have default notes they can be shared in the range-diff on the cover letter, even though `format-patch` isn’t supposed to show them. § Code layout and approach to the problem As I’ve said, my focus was on fixing `format-patch`, so I’ve considered how `format-patch` calls `range-diff` which in turn calls `log`. `format-patch` is a command which is defined in `builtin/log.c`. For notes in particular it in fact has some explicit logic for handling notes based on the value of `rev`. (There seems to be no issues with this part of the code; only the code in `range-diff.c` which passes the default argument to `log`.) It then calls `range-diff.c:show_range_diff`. That function on `master` loads some default arguments, among them `--notes`. It then eventually calls `log` as a subprocess. My change consists of co-opting the deprecated `--standard-notes` and changing its behavior so that it can be used in `range-diff.c:show_range_diff`. Using a special switch/option was the only way I found in order to fix this problem. I could have also created a new option but I thought that doing a smaller change initially would be better. § Changes since version 1 Patch 3: change comment placement based on feedback. § Rebase on upstream Rebased on `master` (fc6bba66bc (Merge branch 'js/allow-t4000-to-be-indented-with-spaces', 2023-08-14)). § CI https://github.com/LemmingAvalanche/git/actions/runs/5868463450 Kristoffer Haugsbakk (3): range-diff: treat notes like `log` doc: pretty-options: remove documentation for deprecated options revision: comment `--no-standard-notes` as deprecated Documentation/pretty-options.txt | 1 - range-diff.c | 2 +- revision.c | 8 ++++++-- t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++ 4 files changed, 35 insertions(+), 4 deletions(-) Range-diff against v1: 1: 6a4fe606cf = 1: e9a5910831 range-diff: treat notes like `log` 2: fbc1c47b92 = 2: f7308b7abf doc: pretty-options: remove documentation for deprecated options 3: 9141f5a86e ! 3: 80245bbb7e revision: comment `--no-standard-notes` as deprecated @@ Metadata ## Commit message ## revision: comment `--no-standard-notes` as deprecated - We still use `--standard-notes` but this option has no use and is no - longer documented anywhere. + `--standard-notes` used to be deprecated but is now (since 6a4fe606cf[1]) + used internally. Its negation `--no-standard-notes`, however, is still + deprecated even for internal use. + + Mark this option as such. + + † 1: range-diff: treat notes like `log`, 2023-05-19 Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> + + ## Notes (series) ## + • Move comment: https://lore.kernel.org/git/xmqqbkhk724x.fsf@gitster.g/ + • Tweak commit message so that it's clearer why we are only commenting + “no” as deprecated and not the other option as well + ## revision.c ## @@ revision.c: static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg - enable_default_display_notes(&revs->notes_opt, &revs->show_notes); revs->notes_opt.use_default_notes = -1; -+ /* Deprecated */ } else if (!strcmp(arg, "--no-standard-notes")) { ++ /* Deprecated */ revs->notes_opt.use_default_notes = 0; } else if (!strcmp(arg, "--oneline")) { + revs->verbose_header = 1; -- 2.41.0 ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v2 1/3] range-diff: treat notes like `log` 2023-09-01 16:18 ` [PATCH v2 " Kristoffer Haugsbakk @ 2023-09-01 16:19 ` Kristoffer Haugsbakk 2023-09-03 12:17 ` Johannes Schindelin 2023-09-01 16:19 ` [PATCH v2 2/3] doc: pretty-options: remove documentation for deprecated options Kristoffer Haugsbakk ` (2 subsequent siblings) 3 siblings, 1 reply; 41+ messages in thread From: Kristoffer Haugsbakk @ 2023-09-01 16:19 UTC (permalink / raw) To: git; +Cc: Johannes Schindelin, Denton Liu, Jeff King, Kristoffer Haugsbakk 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 co-opting the `--standard-notes` option which has been deprecated since ab18b2c0df[2] and which is currently only documented in `pretty-options`. † 1: See e.g. 66b2ed09c2 (Fix "log" family not to be too agressive about showing notes, 2010-01-20). † 2: log/pretty-options: Document --[no-]notes and deprecate old notes options, 2011-03-30 Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> --- range-diff.c | 2 +- revision.c | 7 +++++-- t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++ 3 files changed, 34 insertions(+), 3 deletions(-) diff --git a/range-diff.c b/range-diff.c index 2e86063491..f070e4a4ce 100644 --- a/range-diff.c +++ b/range-diff.c @@ -60,7 +60,7 @@ static int read_patches(const char *range, struct string_list *list, "--output-indicator-context=#", "--no-abbrev-commit", "--pretty=medium", - "--notes", + "--standard-notes", NULL); strvec_push(&cp.args, range); if (other_arg) diff --git a/revision.c b/revision.c index 2f4c53ea20..64aebc014b 100644 --- a/revision.c +++ b/revision.c @@ -2495,8 +2495,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg disable_display_notes(&revs->notes_opt, &revs->show_notes); revs->show_notes_given = 1; } else if (!strcmp(arg, "--standard-notes")) { - revs->show_notes_given = 1; - revs->notes_opt.use_default_notes = 1; + disable_display_notes(&revs->notes_opt, &revs->show_notes); + revs->show_notes_given = 0; + enable_default_display_notes(&revs->notes_opt, + &revs->show_notes); + revs->notes_opt.use_default_notes = -1; } else if (!strcmp(arg, "--no-standard-notes")) { revs->notes_opt.use_default_notes = 0; } else if (!strcmp(arg, "--oneline")) { 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 && -- 2.41.0 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v2 1/3] range-diff: treat notes like `log` 2023-09-01 16:19 ` [PATCH v2 1/3] " Kristoffer Haugsbakk @ 2023-09-03 12:17 ` Johannes Schindelin 2023-09-04 17:10 ` Kristoffer Haugsbakk 0 siblings, 1 reply; 41+ messages in thread From: Johannes Schindelin @ 2023-09-03 12:17 UTC (permalink / raw) To: Kristoffer Haugsbakk; +Cc: git, Denton Liu, Jeff King [-- Attachment #1: Type: text/plain, Size: 4720 bytes --] Hi Kristoffer, On Fri, 1 Sep 2023, Kristoffer Haugsbakk wrote: > 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. Very well explained. The root cause for this is 8cf51561d1e (range-diff: fix a crash in parsing git-log output, 2020-04-15) which added the `--notes` argument in `read_patches()`' call. The commit message explains why this is needed: the (necessary) `--pretty=medium` would turn off the notes, therefore `--notes` had to be added to reinstate the original behavior (except, as you pointed out, in the case `--notes=<ref>` was specified). > Remedy this by co-opting the `--standard-notes` option which has been > deprecated since ab18b2c0df[2] and which is currently only documented in > `pretty-options`. This sounds a bit less desirable, though, than passing the `--notes` argument only as needed. This patch (on top of `notes-range-diff` in your fork) lets the new test still pass while leaving `--standard-notes` deprecated: -- snipsnap -- diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt index 8c982609c99..dc685be363a 100644 --- a/Documentation/pretty-options.txt +++ b/Documentation/pretty-options.txt @@ -88,6 +88,7 @@ being displayed. Examples: "--notes=foo" will show only notes from from "refs/notes/bar". --show-notes[=<ref>]:: +--[no-]standard-notes:: These options are deprecated. Use the above --notes/--no-notes options instead. endif::git-rev-list[] diff --git a/range-diff.c b/range-diff.c index f070e4a4ceb..fbb81a92cc0 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", - "--standard-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/revision.c b/revision.c index 44a04004a70..2f4c53ea207 100644 --- a/revision.c +++ b/revision.c @@ -2495,13 +2495,9 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg disable_display_notes(&revs->notes_opt, &revs->show_notes); revs->show_notes_given = 1; } else if (!strcmp(arg, "--standard-notes")) { - disable_display_notes(&revs->notes_opt, &revs->show_notes); - revs->show_notes_given = 0; - enable_default_display_notes(&revs->notes_opt, - &revs->show_notes); - revs->notes_opt.use_default_notes = -1; + revs->show_notes_given = 1; + revs->notes_opt.use_default_notes = 1; } else if (!strcmp(arg, "--no-standard-notes")) { - /* Deprecated */ revs->notes_opt.use_default_notes = 0; } else if (!strcmp(arg, "--oneline")) { revs->verbose_header = 1; ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v2 1/3] range-diff: treat notes like `log` 2023-09-03 12:17 ` Johannes Schindelin @ 2023-09-04 17:10 ` Kristoffer Haugsbakk 2023-09-05 10:56 ` Johannes Schindelin 0 siblings, 1 reply; 41+ messages in thread From: Kristoffer Haugsbakk @ 2023-09-04 17:10 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git, Denton Liu, Jeff King Hi Dscho On Sun, Sep 3, 2023, at 14:17, Johannes Schindelin wrote: > Hi Kristoffer, > > On Fri, 1 Sep 2023, Kristoffer Haugsbakk wrote: > > > [snip] > > Very well explained. > > The root cause for this is 8cf51561d1e (range-diff: fix a crash in parsing > git-log output, 2020-04-15) which added the `--notes` argument in > `read_patches()`' call. The commit message explains why this is needed: > the (necessary) `--pretty=medium` would turn off the notes, therefore > `--notes` had to be added to reinstate the original behavior (except, as > you pointed out, in the case `--notes=<ref>` was specified). That's interesting. I didn't find that commit in my history spelunking. >> Remedy this by co-opting the `--standard-notes` option which has been >> deprecated since ab18b2c0df[2] and which is currently only documented in >> `pretty-options`. > > This sounds a bit less desirable, though, than passing the `--notes` > argument only as needed. This patch (on top of `notes-range-diff` in your > fork) lets the new test still pass while leaving `--standard-notes` > deprecated: > > [snip] I like it. I didn't like my solution but it was the only thing that I could get to work. I would like to use your solution instead. Thank you. Maybe you could supply a commit message for v3? v3 would then consist of two commits: 1. Your fix 2. Those two tests Or should it be handled in some other way? Cheers -- Kristoffer Haugsbakk ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 1/3] range-diff: treat notes like `log` 2023-09-04 17:10 ` Kristoffer Haugsbakk @ 2023-09-05 10:56 ` Johannes Schindelin 2023-09-05 22:19 ` Junio C Hamano 0 siblings, 1 reply; 41+ messages in thread From: Johannes Schindelin @ 2023-09-05 10:56 UTC (permalink / raw) To: Kristoffer Haugsbakk; +Cc: git, Denton Liu, Jeff King Hi Kristoffer, On Mon, 4 Sep 2023, Kristoffer Haugsbakk wrote: > I would like to use your solution instead. Thank you. Please do! You're welcome. > Maybe you could supply a commit message for v3? v3 would then consist of > two commits: > > 1. Your fix > 2. Those two tests > > Or should it be handled in some other way? Personally, I'd prefer it if you just squashed the changes into your patch. If you want, I'd be delighted about a Helped-by (or even Co-authored-by) footer, but taking ownership would be fine with me, too. Ciao, Johannes ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 1/3] range-diff: treat notes like `log` 2023-09-05 10:56 ` Johannes Schindelin @ 2023-09-05 22:19 ` Junio C Hamano 0 siblings, 0 replies; 41+ messages in thread From: Junio C Hamano @ 2023-09-05 22:19 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Kristoffer Haugsbakk, git, Denton Liu, Jeff King Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Hi Kristoffer, > > On Mon, 4 Sep 2023, Kristoffer Haugsbakk wrote: > >> I would like to use your solution instead. Thank you. > > Please do! You're welcome. > >> Maybe you could supply a commit message for v3? v3 would then consist of >> two commits: >> >> 1. Your fix >> 2. Those two tests >> >> Or should it be handled in some other way? > > Personally, I'd prefer it if you just squashed the changes into your > patch. If you want, I'd be delighted about a Helped-by (or even > Co-authored-by) footer, but taking ownership would be fine with me, too. Sounds very sensible. Thanks, both. Will look forward to seeing an updated series. ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v2 2/3] doc: pretty-options: remove documentation for deprecated options 2023-09-01 16:18 ` [PATCH v2 " Kristoffer Haugsbakk 2023-09-01 16:19 ` [PATCH v2 1/3] " Kristoffer Haugsbakk @ 2023-09-01 16:19 ` Kristoffer Haugsbakk 2023-09-01 16:19 ` [PATCH v2 3/3] revision: comment `--no-standard-notes` as deprecated Kristoffer Haugsbakk 2023-09-10 22:06 ` [PATCH v3 0/1] range-diff: treat notes like `log` Kristoffer Haugsbakk 3 siblings, 0 replies; 41+ messages in thread From: Kristoffer Haugsbakk @ 2023-09-01 16:19 UTC (permalink / raw) To: git; +Cc: Johannes Schindelin, Denton Liu, Jeff King, Kristoffer Haugsbakk Remove documentation for options which were deprecated in ab18b2c0df (log/pretty-options: Document --[no-]notes and deprecate old notes options, 2011-03-30). We now use `--standard-notes` internally for `range-diff` (see previous commit). Leave `--show-notes` as-is since we aren’t changing anything about it. Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> --- Documentation/pretty-options.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt index dc685be363..8c982609c9 100644 --- a/Documentation/pretty-options.txt +++ b/Documentation/pretty-options.txt @@ -88,7 +88,6 @@ being displayed. Examples: "--notes=foo" will show only notes from from "refs/notes/bar". --show-notes[=<ref>]:: ---[no-]standard-notes:: These options are deprecated. Use the above --notes/--no-notes options instead. endif::git-rev-list[] -- 2.41.0 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v2 3/3] revision: comment `--no-standard-notes` as deprecated 2023-09-01 16:18 ` [PATCH v2 " Kristoffer Haugsbakk 2023-09-01 16:19 ` [PATCH v2 1/3] " Kristoffer Haugsbakk 2023-09-01 16:19 ` [PATCH v2 2/3] doc: pretty-options: remove documentation for deprecated options Kristoffer Haugsbakk @ 2023-09-01 16:19 ` Kristoffer Haugsbakk 2023-09-10 22:06 ` [PATCH v3 0/1] range-diff: treat notes like `log` Kristoffer Haugsbakk 3 siblings, 0 replies; 41+ messages in thread From: Kristoffer Haugsbakk @ 2023-09-01 16:19 UTC (permalink / raw) To: git; +Cc: Johannes Schindelin, Denton Liu, Jeff King, Kristoffer Haugsbakk `--standard-notes` used to be deprecated but is now (since 6a4fe606cf[1]) used internally. Its negation `--no-standard-notes`, however, is still deprecated even for internal use. Mark this option as such. † 1: range-diff: treat notes like `log`, 2023-05-19 Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> --- Notes (series): • Move comment: https://lore.kernel.org/git/xmqqbkhk724x.fsf@gitster.g/ • Tweak commit message so that it's clearer why we are only commenting “no” as deprecated and not the other option as well revision.c | 1 + 1 file changed, 1 insertion(+) diff --git a/revision.c b/revision.c index 64aebc014b..44a04004a7 100644 --- a/revision.c +++ b/revision.c @@ -2501,6 +2501,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg &revs->show_notes); revs->notes_opt.use_default_notes = -1; } else if (!strcmp(arg, "--no-standard-notes")) { + /* Deprecated */ revs->notes_opt.use_default_notes = 0; } else if (!strcmp(arg, "--oneline")) { revs->verbose_header = 1; -- 2.41.0 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v3 0/1] range-diff: treat notes like `log` 2023-09-01 16:18 ` [PATCH v2 " Kristoffer Haugsbakk ` (2 preceding siblings ...) 2023-09-01 16:19 ` [PATCH v2 3/3] revision: comment `--no-standard-notes` as deprecated Kristoffer Haugsbakk @ 2023-09-10 22:06 ` Kristoffer Haugsbakk 2023-09-10 22:06 ` [PATCH v3 1/1] " Kristoffer Haugsbakk ` (2 more replies) 3 siblings, 3 replies; 41+ messages in thread From: Kristoffer Haugsbakk @ 2023-09-10 22:06 UTC (permalink / raw) To: git; +Cc: Johannes Schindelin, Denton Liu, Jeff King, Kristoffer Haugsbakk Hi The cover letter up until the “Changes” section is mostly the same except I deleted the justification for my approach to the problem since we don't use that approach any more. Cheers 🙛 🙙 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. These changes are supposed to make `format-range` behave like `log` with regards to notes. These changes also fixes an issue with notes being shared in the cover letter via `range-diff`, and that’s really the main motivation for making these changes. § How `log` works `log` shows the default notes if no notes arguments are given. But if you give it specific notes to show then it forgets about the default notes. Further, there is the convenience `--notes` option which will show the default notes again. These options are cumulative. For example: git log --notes --notes=custom Will show the default notes as well as the `custom` notes. See discussion in: https://lore.kernel.org/git/20110329143357.GA10771@sigill.intra.peff.net/ § How `range-format` works `range-format` passes `--notes` to `log`, which means that it does not have the default behavior of `log` (forget the default logs if you say e.g. `--notes=custom`). However, the man page says that (under `--[no-]notes[=<ref>]`): > This flag is passed to the git log program (see git-log(1)) that generates the patches. This makes me (at least) think that `range-format` is supposed to work just like `log` with regards to notes. § `format-patch` and the difference between showing and sharing `format-patch` has a different default: it shows no notes. This makes sense in my opinion since `format-patch` is meant to be used to share changes with others, and you might be surprised if your notes (which might have only been notes to yourself) are sent out in your emails (keep in mind that notes refs are *not* pushed by default). But the slightly faulty behavior of `range-diff` bleeds through to `format-patch` since the latter calls the former; if you have default notes they can be shared in the range-diff on the cover letter, even though `format-patch` isn’t supposed to show them. § Changes since version 2 Dscho provided an [alternative] solution. My three patches and his patch have been squashed into one. 🔗 alternative: https://lore.kernel.org/git/94b9535b-8c2a-eb8f-90fb-cd0f998ec57e@gmx.de/ § CI https://github.com/LemmingAvalanche/git/actions/runs/6139150031 Kristoffer Haugsbakk (1): range-diff: treat notes like `log` range-diff.c | 13 +++++++++++-- t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) Range-diff against v2: 1: e9a5910831 ! 1: a37dfb3748 range-diff: treat notes like `log` @@ Commit message 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: + man page for `range-diff` under `--[no-]notes[=<ref>]` says: - > This flag is passed to the git log program (see git-log(1)) that + > 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 @@ Commit message to say about the changes to the default notes, since that will be shown in the cover letter. - Remedy this by co-opting the `--standard-notes` option which has been - deprecated since ab18b2c0df[2] and which is currently only documented in - `pretty-options`. + 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). - † 2: log/pretty-options: Document --[no-]notes and deprecate old notes - options, 2011-03-30 + Co-authored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> ## range-diff.c ## +@@ range-diff.c: 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", @@ range-diff.c: static int read_patches(const char *range, struct string_list *list, "--output-indicator-context=#", "--no-abbrev-commit", "--pretty=medium", - "--notes", -+ "--standard-notes", NULL); ++ if (implicit_notes_arg) ++ strvec_push(&cp.args, "--notes"); strvec_push(&cp.args, range); if (other_arg) - - ## revision.c ## -@@ revision.c: static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg - disable_display_notes(&revs->notes_opt, &revs->show_notes); - revs->show_notes_given = 1; - } else if (!strcmp(arg, "--standard-notes")) { -- revs->show_notes_given = 1; -- revs->notes_opt.use_default_notes = 1; -+ disable_display_notes(&revs->notes_opt, &revs->show_notes); -+ revs->show_notes_given = 0; -+ enable_default_display_notes(&revs->notes_opt, -+ &revs->show_notes); -+ revs->notes_opt.use_default_notes = -1; - } else if (!strcmp(arg, "--no-standard-notes")) { - revs->notes_opt.use_default_notes = 0; - } else if (!strcmp(arg, "--oneline")) { + strvec_pushv(&cp.args, other_arg->v); ## t/t3206-range-diff.sh ## @@ t/t3206-range-diff.sh: test_expect_success 'range-diff with multiple --notes' ' 2: f7308b7abf < -: ---------- doc: pretty-options: remove documentation for deprecated options 3: 80245bbb7e < -: ---------- revision: comment `--no-standard-notes` as deprecated -- 2.42.0 ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v3 1/1] range-diff: treat notes like `log` 2023-09-10 22:06 ` [PATCH v3 0/1] range-diff: treat notes like `log` Kristoffer Haugsbakk @ 2023-09-10 22:06 ` Kristoffer Haugsbakk 2023-09-11 19:55 ` Junio C Hamano 2023-09-11 13:23 ` [PATCH v3 0/1] " Johannes Schindelin 2023-09-19 18:05 ` [PATCH v4 " Kristoffer Haugsbakk 2 siblings, 1 reply; 41+ messages in thread From: Kristoffer Haugsbakk @ 2023-09-10 22:06 UTC (permalink / raw) To: git Cc: Johannes Schindelin, Denton Liu, Jeff King, Kristoffer Haugsbakk, Johannes Schindelin 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(-) 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 && -- 2.42.0 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v3 1/1] range-diff: treat notes like `log` 2023-09-10 22:06 ` [PATCH v3 1/1] " Kristoffer Haugsbakk @ 2023-09-11 19:55 ` Junio C Hamano 2023-09-14 8:29 ` Johannes Schindelin 0 siblings, 1 reply; 41+ messages in thread From: Junio C Hamano @ 2023-09-11 19:55 UTC (permalink / raw) To: Kristoffer Haugsbakk; +Cc: git, Johannes Schindelin, Denton Liu, Jeff King 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 && ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 1/1] range-diff: treat notes like `log` 2023-09-11 19:55 ` Junio C Hamano @ 2023-09-14 8:29 ` Johannes Schindelin 2023-09-14 16:18 ` Junio C Hamano 2023-09-14 20:25 ` Kristoffer Haugsbakk 0 siblings, 2 replies; 41+ messages in thread From: Johannes Schindelin @ 2023-09-14 8:29 UTC (permalink / raw) To: Junio C Hamano; +Cc: Kristoffer Haugsbakk, git, Denton Liu, Jeff King [-- Attachment #1: Type: text/plain, Size: 4717 bytes --] 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, ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v3 1/1] range-diff: treat notes like `log` 2023-09-14 8:29 ` Johannes Schindelin @ 2023-09-14 16:18 ` Junio C Hamano 2023-09-14 20:25 ` Kristoffer Haugsbakk 1 sibling, 0 replies; 41+ messages in thread From: Junio C Hamano @ 2023-09-14 16:18 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Kristoffer Haugsbakk, git, Denton Liu, Jeff King 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. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 1/1] range-diff: treat notes like `log` 2023-09-14 8:29 ` Johannes Schindelin 2023-09-14 16:18 ` Junio C Hamano @ 2023-09-14 20:25 ` Kristoffer Haugsbakk 2023-09-19 1:16 ` Junio C Hamano 1 sibling, 1 reply; 41+ messages in thread From: Kristoffer Haugsbakk @ 2023-09-14 20:25 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git, Denton Liu, Jeff King, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 1/1] range-diff: treat notes like `log` 2023-09-14 20:25 ` Kristoffer Haugsbakk @ 2023-09-19 1:16 ` Junio C Hamano 2023-09-19 9:12 ` Kristoffer Haugsbakk 0 siblings, 1 reply; 41+ messages in thread From: Junio C Hamano @ 2023-09-19 1:16 UTC (permalink / raw) To: Kristoffer Haugsbakk; +Cc: Johannes Schindelin, git, Denton Liu, Jeff King "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. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 1/1] range-diff: treat notes like `log` 2023-09-19 1:16 ` Junio C Hamano @ 2023-09-19 9:12 ` Kristoffer Haugsbakk 0 siblings, 0 replies; 41+ messages in thread From: Kristoffer Haugsbakk @ 2023-09-19 9:12 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Schindelin, git, Denton Liu, Jeff King 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 -- Kristoffer Haugsbakk ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 0/1] range-diff: treat notes like `log` 2023-09-10 22:06 ` [PATCH v3 0/1] range-diff: treat notes like `log` Kristoffer Haugsbakk 2023-09-10 22:06 ` [PATCH v3 1/1] " Kristoffer Haugsbakk @ 2023-09-11 13:23 ` Johannes Schindelin 2023-09-19 18:05 ` [PATCH v4 " Kristoffer Haugsbakk 2 siblings, 0 replies; 41+ messages in thread From: Johannes Schindelin @ 2023-09-11 13:23 UTC (permalink / raw) To: Kristoffer Haugsbakk; +Cc: git, Denton Liu, Jeff King [-- Attachment #1: Type: text/plain, Size: 374 bytes --] Hi Kristoffer, On Mon, 11 Sep 2023, Kristoffer Haugsbakk wrote: > § Changes since version 2 > > Dscho provided an [alternative] solution. My three patches and his patch > have been squashed into one. > > 🔗 alternative: https://lore.kernel.org/git/94b9535b-8c2a-eb8f-90fb-cd0f998ec57e@gmx.de/ Thank you, this iteration looks good to me! Ciao, Johannes ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v4 0/1] range-diff: treat notes like `log` 2023-09-10 22:06 ` [PATCH v3 0/1] range-diff: treat notes like `log` Kristoffer Haugsbakk 2023-09-10 22:06 ` [PATCH v3 1/1] " Kristoffer Haugsbakk 2023-09-11 13:23 ` [PATCH v3 0/1] " Johannes Schindelin @ 2023-09-19 18:05 ` Kristoffer Haugsbakk 2023-09-19 18:05 ` [PATCH v4 1/1] " Kristoffer Haugsbakk 2023-09-19 20:26 ` [PATCH v5 0/1] " Kristoffer Haugsbakk 2 siblings, 2 replies; 41+ messages in thread From: Kristoffer Haugsbakk @ 2023-09-19 18:05 UTC (permalink / raw) To: git; +Cc: Johannes Schindelin, Denton Liu, Jeff King, Kristoffer Haugsbakk Hi Same cover letter up until but not including “Changes ...”. Cheers 🙛 🙙 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. These changes are supposed to make `format-range` behave like `log` with regards to notes. These changes also fixes an issue with notes being shared in the cover letter via `range-diff`, and that’s really the main motivation for making these changes. § How `log` works `log` shows the default notes if no notes arguments are given. But if you give it specific notes to show then it forgets about the default notes. Further, there is the convenience `--notes` option which will show the default notes again. These options are cumulative. For example: git log --notes --notes=custom Will show the default notes as well as the `custom` notes. See discussion in: https://lore.kernel.org/git/20110329143357.GA10771@sigill.intra.peff.net/ § How `range-format` works `range-format` passes `--notes` to `log`, which means that it does not have the default behavior of `log` (forget the default logs if you say e.g. `--notes=custom`). However, the man page says that (under `--[no-]notes[=<ref>]`): > This flag is passed to the git log program (see git-log(1)) that generates the patches. This makes me (at least) think that `range-format` is supposed to work just like `log` with regards to notes. § `format-patch` and the difference between showing and sharing `format-patch` has a different default: it shows no notes. This makes sense in my opinion since `format-patch` is meant to be used to share changes with others, and you might be surprised if your notes (which might have only been notes to yourself) are sent out in your emails (keep in mind that notes refs are *not* pushed by default). But the slightly faulty behavior of `range-diff` bleeds through to `format-patch` since the latter calls the former; if you have default notes they can be shared in the range-diff on the cover letter, even though `format-patch` isn’t supposed to show them. § Changes since version 3 Dscho [rewrote] the fix: introduce a new option to pass to `git log`. 🔗 rewrote: https://lore.kernel.org/git/dd2958c5-58bf-86dd-b666-9033259a8e1a@gmx.de/ § CI (WIP) https://github.com/LemmingAvalanche/git/actions/runs/6238806624 Kristoffer Haugsbakk (1): range-diff: treat notes like `log` Documentation/pretty-options.txt | 4 ++++ range-diff.c | 2 +- revision.c | 7 +++++++ revision.h | 1 + t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++ 5 files changed, 41 insertions(+), 1 deletion(-) Range-diff against v3: 1: a37dfb3748 ! 1: 244e102cc4 range-diff: treat notes like `log` @@ Commit message 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`. + Remedy this by introducing `--show-notes-by-default` that `range-diff` can + use to tell the `log` subprocess what to do. § Root cause @@ Commit message Co-authored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> - ## range-diff.c ## -@@ range-diff.c: 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; + ## Documentation/pretty-options.txt ## +@@ Documentation/pretty-options.txt: message by 4 spaces (i.e. 'medium', which is the default, 'full', + and 'fuller'). -+ 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; -+ } + ifndef::git-rev-list[] ++--show-notes-by-default:: ++ Show the default notes (see `--notes`) unless subsequent arguments ++ are used to display specific notes. + - strvec_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges", - "--reverse", "--date-order", "--decorate=no", - "--no-prefix", "--submodule=short", + --notes[=<ref>]:: + Show the notes (see linkgit:git-notes[1]) that annotate the + commit, when showing the commit log message. This is the default + + ## range-diff.c ## @@ range-diff.c: static int read_patches(const char *range, struct string_list *list, "--output-indicator-context=#", "--no-abbrev-commit", "--pretty=medium", - "--notes", ++ "--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); + + ## revision.c ## +@@ revision.c: 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=") && +@@ revision.c: 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; + } + + + ## revision.h ## +@@ revision.h: 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, ## t/t3206-range-diff.sh ## @@ t/t3206-range-diff.sh: test_expect_success 'range-diff with multiple --notes' ' -- 2.42.0 ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v4 1/1] range-diff: treat notes like `log` 2023-09-19 18:05 ` [PATCH v4 " Kristoffer Haugsbakk @ 2023-09-19 18:05 ` Kristoffer Haugsbakk 2023-09-19 19:27 ` Junio C Hamano 2023-09-19 19:27 ` Kristoffer Haugsbakk 2023-09-19 20:26 ` [PATCH v5 0/1] " Kristoffer Haugsbakk 1 sibling, 2 replies; 41+ messages in thread From: Kristoffer Haugsbakk @ 2023-09-19 18:05 UTC (permalink / raw) To: git Cc: Johannes Schindelin, Denton Liu, Jeff King, Kristoffer Haugsbakk, Johannes Schindelin 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 introducing `--show-notes-by-default` that `range-diff` can use to tell the `log` subprocess what to do. § 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> --- Documentation/pretty-options.txt | 4 ++++ range-diff.c | 2 +- revision.c | 7 +++++++ revision.h | 1 + t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++ 5 files changed, 41 insertions(+), 1 deletion(-) diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt index dc685be363..dcd501ee50 100644 --- a/Documentation/pretty-options.txt +++ b/Documentation/pretty-options.txt @@ -59,6 +59,10 @@ message by 4 spaces (i.e. 'medium', which is the default, 'full', and 'fuller'). ifndef::git-rev-list[] +--show-notes-by-default:: + Show the default notes (see `--notes`) unless subsequent arguments + are used to display specific notes. + --notes[=<ref>]:: Show the notes (see linkgit:git-notes[1]) that annotate the commit, when showing the commit log message. This is the default diff --git a/range-diff.c b/range-diff.c index 2e86063491..56f6870ff9 100644 --- a/range-diff.c +++ b/range-diff.c @@ -60,7 +60,7 @@ static int read_patches(const char *range, struct string_list *list, "--output-indicator-context=#", "--no-abbrev-commit", "--pretty=medium", - "--notes", + "--show-notes-by-default", NULL); strvec_push(&cp.args, range); if (other_arg) diff --git a/revision.c b/revision.c index 2f4c53ea20..49d385257a 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 82ab400139..50091bbd13 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, 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 && -- 2.42.0 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v4 1/1] range-diff: treat notes like `log` 2023-09-19 18:05 ` [PATCH v4 1/1] " Kristoffer Haugsbakk @ 2023-09-19 19:27 ` Junio C Hamano 2023-09-19 19:44 ` Kristoffer Haugsbakk 2023-09-19 19:27 ` Kristoffer Haugsbakk 1 sibling, 1 reply; 41+ messages in thread From: Junio C Hamano @ 2023-09-19 19:27 UTC (permalink / raw) To: Kristoffer Haugsbakk; +Cc: git, Johannes Schindelin, Denton Liu, Jeff King Kristoffer Haugsbakk <code@khaugsbakk.name> writes: > 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 introducing `--show-notes-by-default` that `range-diff` can > use to tell the `log` subprocess what to do. Very well described. I think the rest of the proposed log message is redundant now we have quite a good write-up above. > ifndef::git-rev-list[] > +--show-notes-by-default:: > + Show the default notes (see `--notes`) unless subsequent arguments > + are used to display specific notes. > + > --notes[=<ref>]:: > Show the notes (see linkgit:git-notes[1]) that annotate the > commit, when showing the commit log message. This is the default I think the new entry should come after the description of `--notes`, which is the primary option around the "notes" feature. In the description, I think "subsequent" is misphrased. It makes it sound as if $ git log --show-notes-by-default --notes=amlog would stop showing the notes from the default notes tree (because the notes from the .git/refs/notes/amlog is explicitly asked for), while $ git log --notes=amlog --show-notes-by-default would show both the default and the custom notes, which is not what the code does, I think, in this hunk: > @@ -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; > } Other than the above minor nits, looks very good. Thanks. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v4 1/1] range-diff: treat notes like `log` 2023-09-19 19:27 ` Junio C Hamano @ 2023-09-19 19:44 ` Kristoffer Haugsbakk 2023-09-19 19:51 ` Junio C Hamano 0 siblings, 1 reply; 41+ messages in thread From: Kristoffer Haugsbakk @ 2023-09-19 19:44 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Johannes Schindelin, Denton Liu, Jeff King On Tue, Sep 19, 2023, at 21:27, Junio C Hamano wrote: > Kristoffer Haugsbakk <code@khaugsbakk.name> writes: > >> [snip] > > Very well described. I think the rest of the proposed log message > is redundant now we have quite a good write-up above. Thanks! > >> ifndef::git-rev-list[] >> +--show-notes-by-default:: >> + Show the default notes (see `--notes`) unless subsequent arguments >> + are used to display specific notes. >> + >> --notes[=<ref>]:: >> Show the notes (see linkgit:git-notes[1]) that annotate the >> commit, when showing the commit log message. This is the default > > I think the new entry should come after the description of `--notes`, > which is the primary option around the "notes" feature. > > In the description, I think "subsequent" is misphrased. It makes it > sound as if > > $ git log --show-notes-by-default --notes=amlog > > would stop showing the notes from the default notes tree (because > the notes from the .git/refs/notes/amlog is explicitly asked for), > while > > $ git log --notes=amlog --show-notes-by-default > > would show both the default and the custom notes, which is not what > the code does, I think, in this hunk: > >> @@ -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; >> } > > Other than the above minor nits, looks very good. > > Thanks. Okay I think I understand. With that in mind I would change it to the patch below. I can make a new version if that looks okay. -- >8 -- diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt index dcd501ee505..d2df1aad647 100644 --- a/Documentation/pretty-options.txt +++ b/Documentation/pretty-options.txt @@ -59,10 +59,6 @@ message by 4 spaces (i.e. 'medium', which is the default, 'full', and 'fuller'). ifndef::git-rev-list[] ---show-notes-by-default:: - Show the default notes (see `--notes`) unless subsequent arguments - are used to display specific notes. - --notes[=<ref>]:: Show the notes (see linkgit:git-notes[1]) that annotate the commit, when showing the commit log message. This is the default @@ -91,6 +87,10 @@ being displayed. Examples: "--notes=foo" will show only notes from "--notes --notes=foo --no-notes --notes=bar" will only show notes from "refs/notes/bar". +--show-notes-by-default:: + Show the default notes unless arguments are given for displaying + specific notes. + --show-notes[=<ref>]:: --[no-]standard-notes:: These options are deprecated. Use the above --notes/--no-notes ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v4 1/1] range-diff: treat notes like `log` 2023-09-19 19:44 ` Kristoffer Haugsbakk @ 2023-09-19 19:51 ` Junio C Hamano 0 siblings, 0 replies; 41+ messages in thread From: Junio C Hamano @ 2023-09-19 19:51 UTC (permalink / raw) To: Kristoffer Haugsbakk; +Cc: git, Johannes Schindelin, Denton Liu, Jeff King "Kristoffer Haugsbakk" <code@khaugsbakk.name> writes: > Okay I think I understand. With that in mind I would change it to the > patch below. > > I can make a new version if that looks okay. Looking good, although I would say "unless other options are given" ("other" is optional) instead of "arguments" if I were writing this. Thanks. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v4 1/1] range-diff: treat notes like `log` 2023-09-19 18:05 ` [PATCH v4 1/1] " Kristoffer Haugsbakk 2023-09-19 19:27 ` Junio C Hamano @ 2023-09-19 19:27 ` Kristoffer Haugsbakk 2023-09-19 19:43 ` Junio C Hamano 1 sibling, 1 reply; 41+ messages in thread From: Kristoffer Haugsbakk @ 2023-09-19 19:27 UTC (permalink / raw) To: git; +Cc: Johannes Schindelin, Denton Liu, Jeff King The original CI failed.[1] Then I rebased on `master` (d4a83d07b8c (The tenth batch, 2023-09-18)) which also failed.[2] Problem in t5559. So: fair warning. :) [1] https://github.com/LemmingAvalanche/git/actions/runs/6238806624 [2] https://github.com/LemmingAvalanche/git/actions/runs/6239585493 -- Kristoffer Haugsbakk ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v4 1/1] range-diff: treat notes like `log` 2023-09-19 19:27 ` Kristoffer Haugsbakk @ 2023-09-19 19:43 ` Junio C Hamano 0 siblings, 0 replies; 41+ messages in thread From: Junio C Hamano @ 2023-09-19 19:43 UTC (permalink / raw) To: Kristoffer Haugsbakk; +Cc: git, Johannes Schindelin, Denton Liu, Jeff King "Kristoffer Haugsbakk" <code@khaugsbakk.name> writes: > The original CI failed.[1] Then I rebased on `master` (d4a83d07b8c (The > tenth batch, 2023-09-18)) which also failed.[2] Problem in t5559. > > So: fair warning. :) > > [1] https://github.com/LemmingAvalanche/git/actions/runs/6238806624 > [2] https://github.com/LemmingAvalanche/git/actions/runs/6239585493 I think you can ignore these macOS ones that spew == Info: [HTTP/2] [1] ... in the log. The adjustments necessary for the redaction code to deal with these new messages from cURL are known and are cooking. ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v5 0/1] range-diff: treat notes like `log` 2023-09-19 18:05 ` [PATCH v4 " Kristoffer Haugsbakk 2023-09-19 18:05 ` [PATCH v4 1/1] " Kristoffer Haugsbakk @ 2023-09-19 20:26 ` Kristoffer Haugsbakk 2023-09-19 20:26 ` [PATCH v5 1/1] " Kristoffer Haugsbakk 1 sibling, 1 reply; 41+ messages in thread From: Kristoffer Haugsbakk @ 2023-09-19 20:26 UTC (permalink / raw) To: git; +Cc: Johannes Schindelin, Denton Liu, Jeff King, Kristoffer Haugsbakk Hi See the previous cover letter for context. § Changes since version 4 • Remove “Root cause” section from commit message • Doc change according to feedback Kristoffer Haugsbakk (1): range-diff: treat notes like `log` Documentation/pretty-options.txt | 4 ++++ range-diff.c | 2 +- revision.c | 7 +++++++ revision.h | 1 + t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++ 5 files changed, 41 insertions(+), 1 deletion(-) Range-diff against v4: 1: 244e102cc46 ! 1: 6e114271a2e range-diff: treat notes like `log` @@ Commit message Remedy this by introducing `--show-notes-by-default` that `range-diff` can use to tell the `log` subprocess what to do. - § 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 @@ Commit message Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> ## Documentation/pretty-options.txt ## -@@ Documentation/pretty-options.txt: message by 4 spaces (i.e. 'medium', which is the default, 'full', - and 'fuller'). +@@ Documentation/pretty-options.txt: being displayed. Examples: "--notes=foo" will show only notes from + "--notes --notes=foo --no-notes --notes=bar" will only show notes + from "refs/notes/bar". - ifndef::git-rev-list[] +--show-notes-by-default:: -+ Show the default notes (see `--notes`) unless subsequent arguments -+ are used to display specific notes. ++ Show the default notes unless options for displaying specific ++ notes are given. + - --notes[=<ref>]:: - Show the notes (see linkgit:git-notes[1]) that annotate the - commit, when showing the commit log message. This is the default + --show-notes[=<ref>]:: + --[no-]standard-notes:: + These options are deprecated. Use the above --notes/--no-notes ## range-diff.c ## @@ range-diff.c: static int read_patches(const char *range, struct string_list *list, -- 2.42.0 ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v5 1/1] range-diff: treat notes like `log` 2023-09-19 20:26 ` [PATCH v5 0/1] " Kristoffer Haugsbakk @ 2023-09-19 20:26 ` Kristoffer Haugsbakk 2023-09-21 12:30 ` Johannes Schindelin 0 siblings, 1 reply; 41+ messages in thread From: Kristoffer Haugsbakk @ 2023-09-19 20:26 UTC (permalink / raw) To: git Cc: Johannes Schindelin, Denton Liu, Jeff King, Kristoffer Haugsbakk, Johannes Schindelin 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 introducing `--show-notes-by-default` that `range-diff` can use to tell the `log` subprocess what to do. § 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> --- Documentation/pretty-options.txt | 4 ++++ range-diff.c | 2 +- revision.c | 7 +++++++ revision.h | 1 + t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++ 5 files changed, 41 insertions(+), 1 deletion(-) diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt index dc685be363a..335395b727f 100644 --- a/Documentation/pretty-options.txt +++ b/Documentation/pretty-options.txt @@ -87,6 +87,10 @@ being displayed. Examples: "--notes=foo" will show only notes from "--notes --notes=foo --no-notes --notes=bar" will only show notes from "refs/notes/bar". +--show-notes-by-default:: + Show the default notes unless options for displaying specific + notes are given. + --show-notes[=<ref>]:: --[no-]standard-notes:: These options are deprecated. Use the above --notes/--no-notes diff --git a/range-diff.c b/range-diff.c index ca5493984a5..c45b6d849cb 100644 --- a/range-diff.c +++ b/range-diff.c @@ -60,7 +60,7 @@ static int read_patches(const char *range, struct string_list *list, "--output-indicator-context=#", "--no-abbrev-commit", "--pretty=medium", - "--notes", + "--show-notes-by-default", NULL); strvec_push(&cp.args, range); if (other_arg) 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, diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh index b5f4d6a6530..b33afa1c6aa 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 && -- 2.42.0 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v5 1/1] range-diff: treat notes like `log` 2023-09-19 20:26 ` [PATCH v5 1/1] " Kristoffer Haugsbakk @ 2023-09-21 12:30 ` Johannes Schindelin 0 siblings, 0 replies; 41+ messages in thread From: Johannes Schindelin @ 2023-09-21 12:30 UTC (permalink / raw) To: Kristoffer Haugsbakk; +Cc: git, Denton Liu, Jeff King [-- Attachment #1: Type: text/plain, Size: 6670 bytes --] Hi Kristoffer, this iteration looks good to me! Thank you so much, Johannes On Tue, 19 Sep 2023, Kristoffer Haugsbakk wrote: > 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 introducing `--show-notes-by-default` that `range-diff` can > use to tell the `log` subprocess what to do. > > § 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> > --- > Documentation/pretty-options.txt | 4 ++++ > range-diff.c | 2 +- > revision.c | 7 +++++++ > revision.h | 1 + > t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++ > 5 files changed, 41 insertions(+), 1 deletion(-) > > diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt > index dc685be363a..335395b727f 100644 > --- a/Documentation/pretty-options.txt > +++ b/Documentation/pretty-options.txt > @@ -87,6 +87,10 @@ being displayed. Examples: "--notes=foo" will show only notes from > "--notes --notes=foo --no-notes --notes=bar" will only show notes > from "refs/notes/bar". > > +--show-notes-by-default:: > + Show the default notes unless options for displaying specific > + notes are given. > + > --show-notes[=<ref>]:: > --[no-]standard-notes:: > These options are deprecated. Use the above --notes/--no-notes > diff --git a/range-diff.c b/range-diff.c > index ca5493984a5..c45b6d849cb 100644 > --- a/range-diff.c > +++ b/range-diff.c > @@ -60,7 +60,7 @@ static int read_patches(const char *range, struct string_list *list, > "--output-indicator-context=#", > "--no-abbrev-commit", > "--pretty=medium", > - "--notes", > + "--show-notes-by-default", > NULL); > strvec_push(&cp.args, range); > if (other_arg) > 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, > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh > index b5f4d6a6530..b33afa1c6aa 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 && > -- > 2.42.0 > > ^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2023-09-21 19:39 UTC | newest] Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-05-30 10:41 [RFC PATCH 0/3] range-diff: treat notes like `log` Kristoffer Haugsbakk 2023-05-30 10:41 ` [RFC PATCH 1/3] " Kristoffer Haugsbakk 2023-06-01 18:20 ` Jeff King 2023-06-02 10:06 ` Kristoffer Haugsbakk 2023-05-30 10:41 ` [RFC PATCH 2/3] doc: pretty-options: remove documentation for deprecated options Kristoffer Haugsbakk 2023-05-30 10:41 ` [RFC PATCH 3/3] revision: comment `--no-standard-notes` as deprecated Kristoffer Haugsbakk 2023-06-11 18:15 ` [PATCH v1 0/3] range-diff: treat notes like `log` Kristoffer Haugsbakk 2023-06-11 18:15 ` [PATCH v1 1/3] " Kristoffer Haugsbakk 2023-06-11 18:15 ` [PATCH v1 2/3] doc: pretty-options: remove documentation for deprecated options Kristoffer Haugsbakk 2023-06-11 18:15 ` [PATCH v1 3/3] revision: comment `--no-standard-notes` as deprecated Kristoffer Haugsbakk 2023-06-12 22:21 ` Junio C Hamano 2023-06-13 9:46 ` Kristoffer Haugsbakk 2023-06-12 22:25 ` [PATCH v1 0/3] range-diff: treat notes like `log` Junio C Hamano 2023-06-13 5:43 ` Kristoffer Haugsbakk 2023-09-01 16:18 ` [PATCH v2 " Kristoffer Haugsbakk 2023-09-01 16:19 ` [PATCH v2 1/3] " Kristoffer Haugsbakk 2023-09-03 12:17 ` Johannes Schindelin 2023-09-04 17:10 ` Kristoffer Haugsbakk 2023-09-05 10:56 ` Johannes Schindelin 2023-09-05 22:19 ` Junio C Hamano 2023-09-01 16:19 ` [PATCH v2 2/3] doc: pretty-options: remove documentation for deprecated options Kristoffer Haugsbakk 2023-09-01 16:19 ` [PATCH v2 3/3] revision: comment `--no-standard-notes` as deprecated Kristoffer Haugsbakk 2023-09-10 22:06 ` [PATCH v3 0/1] range-diff: treat notes like `log` Kristoffer Haugsbakk 2023-09-10 22:06 ` [PATCH v3 1/1] " Kristoffer Haugsbakk 2023-09-11 19:55 ` Junio C Hamano 2023-09-14 8:29 ` Johannes Schindelin 2023-09-14 16:18 ` Junio C Hamano 2023-09-14 20:25 ` Kristoffer Haugsbakk 2023-09-19 1:16 ` Junio C Hamano 2023-09-19 9:12 ` Kristoffer Haugsbakk 2023-09-11 13:23 ` [PATCH v3 0/1] " Johannes Schindelin 2023-09-19 18:05 ` [PATCH v4 " Kristoffer Haugsbakk 2023-09-19 18:05 ` [PATCH v4 1/1] " Kristoffer Haugsbakk 2023-09-19 19:27 ` Junio C Hamano 2023-09-19 19:44 ` Kristoffer Haugsbakk 2023-09-19 19:51 ` Junio C Hamano 2023-09-19 19:27 ` Kristoffer Haugsbakk 2023-09-19 19:43 ` Junio C Hamano 2023-09-19 20:26 ` [PATCH v5 0/1] " Kristoffer Haugsbakk 2023-09-19 20:26 ` [PATCH v5 1/1] " Kristoffer Haugsbakk 2023-09-21 12:30 ` Johannes Schindelin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).