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