range-diff: treat notes like `log`

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/[email protected]/

§ 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 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

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 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

---
 range-diff.c          |  2 +-
 revision.c            |  7 +++++--
 t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++
 3 files changed, 34 insertions(+), 3 deletions(-)

index 6a704e6f47..2c92af337f 100644
@@ -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)

index b33cc1d106..a0ab7fb784 100644
@@ -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")) {

index b5f4d6a653..b33afa1c6a 100755
@@ -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

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...

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

Hi Peff!

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:

So just like the default behavior of log.

However, the next version (v2) is about teaching range-diff to pass notes down to log:

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/[email protected]/

I guess it depends on your perspective. ;) I would say that they are so intertwined that fixing one also fixes the other.

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

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.

---
 Documentation/pretty-options.txt | 1 -
 1 file changed, 1 deletion(-)

index dc685be363..8c982609c9 100644
@@ -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

We still use --standard-notes but this option has no use and is no longer documented anywhere.

---
 revision.c | 1 +
 1 file changed, 1 insertion(+)

index a0ab7fb784..24219c741a 100644
@@ -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

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/[email protected]/

§ 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 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

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 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

---
 range-diff.c          |  2 +-
 revision.c            |  7 +++++--
 t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++
 3 files changed, 34 insertions(+), 3 deletions(-)

index 6a704e6f47..2c92af337f 100644
@@ -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)

index b33cc1d106..a0ab7fb784 100644
@@ -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")) {

index b5f4d6a653..b33afa1c6a 100755
@@ -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

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.

---
 Documentation/pretty-options.txt | 1 -
 1 file changed, 1 deletion(-)

index dc685be363..8c982609c9 100644
@@ -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

We still use --standard-notes but this option has no use and is no longer documented anywhere.

---
 revision.c | 1 +
 1 file changed, 1 insertion(+)

index a0ab7fb784..24219c741a 100644
@@ -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

Thanks, I’ll use that in the next version if this patch is still included.

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

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.

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

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/[email protected]/

§ 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 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

+
+ ## Notes (series) ##
+ • Move comment: https://lore.kernel.org/git/[email protected]/
+ • 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

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 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

---
 range-diff.c          |  2 +-
 revision.c            |  7 +++++--
 t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++
 3 files changed, 34 insertions(+), 3 deletions(-)

index 2e86063491..f070e4a4ce 100644
@@ -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)

index 2f4c53ea20..64aebc014b 100644
@@ -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")) {

index b5f4d6a653..b33afa1c6a 100755
@@ -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

Hi Kristoffer,

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

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 --

index 8c982609c99..dc685be363a 100644
@@ -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[]

index f070e4a4ceb..fbb81a92cc0 100644
@@ -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);

index 44a04004a70..2f4c53ea207 100644
@@ -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;

Hi Dscho

That's interesting. I didn't find that commit in my history spelunking.

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

Hi Kristoffer,

Please do! You're welcome.

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

Sounds very sensible.

Thanks, both. Will look forward to seeing an updated series.

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.

---
 Documentation/pretty-options.txt | 1 -
 1 file changed, 1 deletion(-)

index dc685be363..8c982609c9 100644
@@ -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

--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

---

Notes (series):
• Move comment: https://lore.kernel.org/git/[email protected]/
• 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(+)

index 64aebc014b..44a04004a7 100644
@@ -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

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/[email protected]/

§ 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 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/[email protected]/

§ 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 <[email protected]>

## 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

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 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:

§ 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).

---
 range-diff.c          | 13 +++++++++++--
 t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 2 deletions(-)

index 2e86063491..fbb81a92cc 100644
@@ -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);

index b5f4d6a653..b33afa1c6a 100755
@@ -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

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.

Hi Junio,

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 --

index fbb81a92cc0..56f6870ff91 100644
@@ -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);

index 2f4c53ea207..49d385257ac 100644
@@ -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;
 }
 

index 82ab400139d..50091bbd13f 100644
@@ -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,

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.

Looks good to me. It seems like an explicit argument is the only way to make this work.

Kristoffer Haugsbakk

Will one of you tie the loose ends?

Thanks.

I will this afternoon (CEST).

Cheers

Kristoffer Haugsbakk

Hi Kristoffer,

Thank you, this iteration looks good to me!

Ciao,
Johannes

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/[email protected]/

§ 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 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/[email protected]/

§ 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

- ## 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

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 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:

§ 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).

---
 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(-)

index dc685be363..dcd501ee50 100644
@@ -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

index 2e86063491..56f6870ff9 100644
@@ -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)

index 2f4c53ea20..49d385257a 100644
@@ -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;
 }
 

index 82ab400139..50091bbd13 100644
@@ -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,

index b5f4d6a653..b33afa1c6a 100755
@@ -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

Very well described. I think the rest of the proposed log message is redundant now we have quite a good write-up above.

    $ git log --show-notes-by-default --notes=amlog

    $ git log --notes=amlog --show-notes-by-default

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 --

index dcd501ee505..d2df1aad647 100644
@@ -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

Looking good, although I would say "unless other options are given" ("other" is optional) instead of "arguments" if I were writing this.

Thanks.

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

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.

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

## 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

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

---
 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(-)

index dc685be363a..335395b727f 100644
@@ -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

index ca5493984a5..c45b6d849cb 100644
@@ -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)

index 2f4c53ea207..49d385257ac 100644
@@ -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;
 }
 

index 82ab400139d..50091bbd13f 100644
@@ -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,

index b5f4d6a6530..b33afa1c6aa 100755
@@ -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

Hi Kristoffer,

this iteration looks good to me!

Thank you so much,
Johannes