RFC patch, 3 partsrange-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/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 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(-)

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

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

On Tue, May 30, 2023 at 12:41:29PM +0200, Kristoffer Haugsbakk wrote:

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!

On Thu, Jun 1, 2023, at 20:20, Jeff King wrote:

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/cover.1574125554.git.liu.denton@gmail.com/

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

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

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

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

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

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

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

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?

On Tue, Jun 13, 2023, at 00:21, Junio C Hamano wrote:

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

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

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

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

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.

On Tue, Jun 13, 2023, at 00:25, Junio C Hamano wrote:

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

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

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

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

Hi Kristoffer,

On Fri, 1 Sep 2023, Kristoffer Haugsbakk wrote:

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

Hi Dscho

On Sun, Sep 3, 2023, at 14:17, Johannes Schindelin wrote:

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

Hi Kristoffer,

On Mon, 4 Sep 2023, Kristoffer Haugsbakk wrote:

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 Documentation/pretty-options.txt | 1 -
 1 file changed, 1 deletion(-)

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

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

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

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

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

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,

On Mon, 11 Sep 2023, Junio C Hamano 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):

-- snipsnap --

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Clever. I was wondering if the option parsing done by the revision machinery was readily available for us to use from range-diff code, but because we are forking out to "log" anyway, giving it the new "--show-notes-by-default" option does sound like a much easier way out. It would help something like

	[alias] ln = !"git log --show-notes-by-default"

as well.  Nicely designed.

On Thu, Sep 14, 2023, at 10:29, Johannes Schindelin wrote:

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

"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:

Will one of you tie the loose ends?

Thanks.

On Tue, Sep 19, 2023, at 03:16, Junio C Hamano wrote:

I will this afternoon (CEST).

Cheers

Hi Kristoffer,

On Mon, 11 Sep 2023, Kristoffer Haugsbakk wrote:

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

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

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

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

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

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:

Other than the above minor nits, looks very good.

Thanks.

On Tue, Sep 19, 2023, at 21:27, Junio C Hamano wrote:

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

"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:

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" <code@khaugsbakk.name> writes:

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

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

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

Hi Kristoffer,

this iteration looks good to me!

Thank you so much, Johannes

On Tue, 19 Sep 2023, Kristoffer Haugsbakk wrote: