Skip to content

RFC range-diff: treat notes like `log`

Kristoffer Haugsbakk git 1 version In review

Get series with b4
b4 am [email protected]

Versions newest first

  1. v1 RFC Latest3 patches 4 repliesIn review

Patches in v1 No applies recorded yet

#Subject and filesSizeRepliesApplied
1/3range-diff: treat notes like `log`range-diff.c revision.c t/t3206-range-diff.sh+34 -32 replies
1/3range-diff: treat notes like `log`range-diff.c revision.c t/t3206-range-diff.sh+34 -30 replies
2/3doc: pretty-options: remove documentation for deprecated optionsDocumentation/pretty-options.txt+0 -10 replies
2/3doc: pretty-options: remove documentation for deprecated optionsDocumentation/pretty-options.txt+0 -10 replies
3/3revision: comment `--no-standard-notes` as deprecatedrevision.c+1 -00 replies
3/3revision: comment `--no-standard-notes` as deprecatedrevision.c+1 -02 replies

Reviews trailers per patch

None of the 6 patches has a Reviewed-by or Acked-by.

PatchJohannes Schindelin
SeriesCo-authored-by Co-authored-by Co-authored-by Co-authored-by
1/3
1/3
2/3
2/3
3/3
3/3

Trailers given in replies are filled; those carried in the patches from earlier versions are outlined with dashes.

Read the v1 thread

Cover letter [RFC PATCH 0/3] 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>]):

Quoted 1 line

This flag is passed to the git log program (see git-log(1)) that generates the patches.

This makes me (at least) think that range-format is supposed to work just like log with regards to notes.

§ format-patch and the difference between showing and sharing

format-patch has a different default: it shows no notes. This makes sense in my opinion since format-patch is meant to be used to share changes with others, and you might be surprised if your notes (which might have only been notes to yourself) are sent out in your emails (keep in mind that notes refs are *not* pushed by default).

But the slightly faulty behavior of range-diff bleeds through to format-patch since the latter calls the former; if you have default notes they can be shared in the range-diff on the cover letter, even though format-patch isn’t supposed to show them.

§ Code layout and approach to the problem

As I’ve said, my focus was on fixing format-patch, so I’ve considered how format-patch calls range-diff which in turn calls log.

format-patch is a command which is defined in builtin/log.c. For notes in particular it in fact has some explicit logic for handling notes based on the value of rev. (There seems to be no issues with this part of the code; only the code in range-diff.c which passes the default argument to log.) It then calls range-diff.c:show_range_diff. That function on master loads some default arguments, among them --notes. It then eventually calls log as a subprocess.

My change consists of co-opting the deprecated --standard-notes and changing its behavior so that it can be used in range-diff.c:show_range_diff.

Using a special switch/option was the only way I found in order to fix this problem.

I could have also created a new option but I thought that doing a smaller change initially would be better.

§ RFC

This is marked as RFC since I chose to co-opt a deprecated option in order to get range-format to work, in my opinion, as it should.

§ Carbon copy

Based on contrib/contacts/git-contacts master..@.

Kristoffer Haugsbakk (3):
range-diff: treat notes like log
doc: pretty-options: remove documentation for deprecated options
revision: comment --no-standard-notes as deprecated

 Documentation/pretty-options.txt |  1 -
 range-diff.c                     |  2 +-
 revision.c                       |  8 ++++++--
 t/t3206-range-diff.sh            | 28 ++++++++++++++++++++++++++++
 4 files changed, 35 insertions(+), 4 deletions(-)
Signature · 1 line
2.41.0.rc2