git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Kristoffer Haugsbakk <code@khaugsbakk.name>
To: git@vger.kernel.org
Cc: Kristoffer Haugsbakk <code@khaugsbakk.name>,
	Jeff King <peff@peff.net>, Junio C Hamano <gitster@pobox.com>,
	Denton Liu <liu.denton@gmail.com>
Subject: [RFC PATCH 0/3] range-diff: treat notes like `log`
Date: Tue, 30 May 2023 12:41:28 +0200	[thread overview]
Message-ID: <cover.1685441207.git.code@khaugsbakk.name> (raw)

Currently, `range-diff` shows the default notes if no notes-related
arguments are given. This is also how `log` behaves. But unlike
`range-diff`, `log` does *not* show the default notes if
`--notes=<custom>` are given.

These changes are supposed to make `format-range` behave like `log` with
regards to notes.

These changes also fixes an issue with notes being shared in the cover
letter via `range-diff`, and that’s really the main motivation for
making these changes.

§ How `log` works

`log` shows the default notes if no notes arguments are given. But if
you give it specific notes to show then it forgets about the default
notes. Further, there is the convenience `--notes` option which will
show the default notes again. These options are cumulative. For example:

    git log --notes --notes=custom

Will show the default notes as well as the `custom` notes.

See discussion in: https://lore.kernel.org/git/20110329143357.GA10771@sigill.intra.peff.net/

§ How `range-format` works

`range-format` passes `--notes` to `log`, which means that it does not
have the default behavior of `log` (forget the default logs if you say
e.g. `--notes=custom`). However, the man page says that (under
`--[no-]notes[=<ref>]`):

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

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

§ `format-patch` and the difference between showing and sharing

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

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

§ Code layout and approach to the problem

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

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

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

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

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

§ RFC

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

§ Carbon copy

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

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

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

-- 
2.41.0.rc2


             reply	other threads:[~2023-05-30 10:43 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-05-30 10:41 Kristoffer Haugsbakk [this message]
2023-05-30 10:41 ` [RFC PATCH 1/3] range-diff: treat notes like `log` Kristoffer Haugsbakk
2023-06-01 18:20   ` Jeff King
2023-06-02 10:06     ` Kristoffer Haugsbakk
2023-05-30 10:41 ` [RFC PATCH 2/3] doc: pretty-options: remove documentation for deprecated options Kristoffer Haugsbakk
2023-05-30 10:41 ` [RFC PATCH 3/3] revision: comment `--no-standard-notes` as deprecated Kristoffer Haugsbakk
2023-06-11 18:15 ` [PATCH v1 0/3] range-diff: treat notes like `log` Kristoffer Haugsbakk
2023-06-11 18:15   ` [PATCH v1 1/3] " Kristoffer Haugsbakk
2023-06-11 18:15   ` [PATCH v1 2/3] doc: pretty-options: remove documentation for deprecated options Kristoffer Haugsbakk
2023-06-11 18:15   ` [PATCH v1 3/3] revision: comment `--no-standard-notes` as deprecated Kristoffer Haugsbakk
2023-06-12 22:21     ` Junio C Hamano
2023-06-13  9:46       ` Kristoffer Haugsbakk
2023-06-12 22:25   ` [PATCH v1 0/3] range-diff: treat notes like `log` Junio C Hamano
2023-06-13  5:43     ` Kristoffer Haugsbakk
2023-09-01 16:18   ` [PATCH v2 " Kristoffer Haugsbakk
2023-09-01 16:19     ` [PATCH v2 1/3] " Kristoffer Haugsbakk
2023-09-03 12:17       ` Johannes Schindelin
2023-09-04 17:10         ` Kristoffer Haugsbakk
2023-09-05 10:56           ` Johannes Schindelin
2023-09-05 22:19             ` Junio C Hamano
2023-09-01 16:19     ` [PATCH v2 2/3] doc: pretty-options: remove documentation for deprecated options Kristoffer Haugsbakk
2023-09-01 16:19     ` [PATCH v2 3/3] revision: comment `--no-standard-notes` as deprecated Kristoffer Haugsbakk
2023-09-10 22:06     ` [PATCH v3 0/1] range-diff: treat notes like `log` Kristoffer Haugsbakk
2023-09-10 22:06       ` [PATCH v3 1/1] " Kristoffer Haugsbakk
2023-09-11 19:55         ` Junio C Hamano
2023-09-14  8:29           ` Johannes Schindelin
2023-09-14 16:18             ` Junio C Hamano
2023-09-14 20:25             ` Kristoffer Haugsbakk
2023-09-19  1:16               ` Junio C Hamano
2023-09-19  9:12                 ` Kristoffer Haugsbakk
2023-09-11 13:23       ` [PATCH v3 0/1] " Johannes Schindelin
2023-09-19 18:05       ` [PATCH v4 " Kristoffer Haugsbakk
2023-09-19 18:05         ` [PATCH v4 1/1] " Kristoffer Haugsbakk
2023-09-19 19:27           ` Junio C Hamano
2023-09-19 19:44             ` Kristoffer Haugsbakk
2023-09-19 19:51               ` Junio C Hamano
2023-09-19 19:27           ` Kristoffer Haugsbakk
2023-09-19 19:43             ` Junio C Hamano
2023-09-19 20:26         ` [PATCH v5 0/1] " Kristoffer Haugsbakk
2023-09-19 20:26           ` [PATCH v5 1/1] " Kristoffer Haugsbakk
2023-09-21 12:30             ` Johannes Schindelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cover.1685441207.git.code@khaugsbakk.name \
    --to=code@khaugsbakk.name \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=liu.denton@gmail.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).