From: Kristoffer Haugsbakk <code@khaugsbakk.name>
To: git@vger.kernel.org
Cc: Johannes Schindelin <johannes.schindelin@gmx.de>,
Denton Liu <liu.denton@gmail.com>, Jeff King <peff@peff.net>,
Kristoffer Haugsbakk <code@khaugsbakk.name>
Subject: [PATCH v3 0/1] range-diff: treat notes like `log`
Date: Mon, 11 Sep 2023 00:06:22 +0200 [thread overview]
Message-ID: <cover.1694383247.git.code@khaugsbakk.name> (raw)
In-Reply-To: <cover.1693584310.git.code@khaugsbakk.name>
Hi
The cover letter up until the “Changes” section is mostly the same except
I deleted the justification for my approach to the problem since we don't
use that approach any more.
Cheers
🙛 🙙
Currently, `range-diff` shows the default notes if no notes-related
arguments are given. This is also how `log` behaves. But unlike
`range-diff`, `log` does *not* show the default notes if
`--notes=<custom>` are given.
These changes are supposed to make `format-range` behave like `log` with
regards to notes.
These changes also fixes an issue with notes being shared in the cover
letter via `range-diff`, and that’s really the main motivation for
making these changes.
§ How `log` works
`log` shows the default notes if no notes arguments are given. But if
you give it specific notes to show then it forgets about the default
notes. Further, there is the convenience `--notes` option which will
show the default notes again. These options are cumulative. For example:
git log --notes --notes=custom
Will show the default notes as well as the `custom` notes.
See discussion in: https://lore.kernel.org/git/20110329143357.GA10771@sigill.intra.peff.net/
§ How `range-format` works
`range-format` passes `--notes` to `log`, which means that it does not
have the default behavior of `log` (forget the default logs if you say
e.g. `--notes=custom`). However, the man page says that (under
`--[no-]notes[=<ref>]`):
> This flag is passed to the git log program (see git-log(1)) that generates the patches.
This makes me (at least) think that `range-format` is supposed to work
just like `log` with regards to notes.
§ `format-patch` and the difference between showing and sharing
`format-patch` has a different default: it shows no notes. This makes
sense in my opinion since `format-patch` is meant to be used to share
changes with others, and you might be surprised if your notes (which
might have only been notes to yourself) are sent out in your emails
(keep in mind that notes refs are *not* pushed by default).
But the slightly faulty behavior of `range-diff` bleeds through to
`format-patch` since the latter calls the former; if you have default
notes they can be shared in the range-diff on the cover letter, even
though `format-patch` isn’t supposed to show them.
§ Changes since version 2
Dscho provided an [alternative] solution. My three patches and his patch
have been squashed into one.
🔗 alternative: https://lore.kernel.org/git/94b9535b-8c2a-eb8f-90fb-cd0f998ec57e@gmx.de/
§ CI
https://github.com/LemmingAvalanche/git/actions/runs/6139150031
Kristoffer Haugsbakk (1):
range-diff: treat notes like `log`
range-diff.c | 13 +++++++++++--
t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++
2 files changed, 39 insertions(+), 2 deletions(-)
Range-diff against v2:
1: e9a5910831 ! 1: a37dfb3748 range-diff: treat notes like `log`
@@ Commit message
git log --notes --notes-custom
This can’t be how the user expects `range-diff` to behave given that the
- man page for `range diff` under `--[no-]notes[=<ref>]` says:
+ man page for `range-diff` under `--[no-]notes[=<ref>]` says:
- > This flag is passed to the git log program (see git-log(1)) that
+ > This flag is passed to the `git log` program (see git-log(1)) that
> generates the patches.
This behavior also affects `format-patch` since it uses `range-diff` for
@@ Commit message
to say about the changes to the default notes, since that will be shown
in the cover letter.
- Remedy this by co-opting the `--standard-notes` option which has been
- deprecated since ab18b2c0df[2] and which is currently only documented in
- `pretty-options`.
+ Remedy this by only conditionally passing in `--notes` to `range-diff`.
+
+ § Root cause
+
+ 8cf51561d1e (range-diff: fix a crash in parsing git-log output,
+ 2020-04-15) added `--notes` in order to deal with a side-effect of
+ `--pretty=medium`:
+
+ > To fix this explicitly set the output format of the internally executed
+ > `git log` with `--pretty=medium`. Because that cancels `--notes`, add
+ > explicitly `--notes` at the end.
+
+ § Authors
+
+ • Fix by Johannes
+ • Tests by Kristoffer
† 1: See e.g. 66b2ed09c2 (Fix "log" family not to be too agressive about
showing notes, 2010-01-20).
- † 2: log/pretty-options: Document --[no-]notes and deprecate old notes
- options, 2011-03-30
+ Co-authored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
## range-diff.c ##
+@@ range-diff.c: static int read_patches(const char *range, struct string_list *list,
+ struct child_process cp = CHILD_PROCESS_INIT;
+ struct strbuf buf = STRBUF_INIT, contents = STRBUF_INIT;
+ struct patch_util *util = NULL;
+- int in_header = 1;
++ int i, implicit_notes_arg = 1, in_header = 1;
+ char *line, *current_filename = NULL;
+ ssize_t len;
+ size_t size;
+ int ret = -1;
+
++ for (i = 0; other_arg && i < other_arg->nr; i++)
++ if (!strcmp(other_arg->v[i], "--notes") ||
++ starts_with(other_arg->v[i], "--notes=") ||
++ !strcmp(other_arg->v[i], "--no-notes")) {
++ implicit_notes_arg = 0;
++ break;
++ }
++
+ strvec_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges",
+ "--reverse", "--date-order", "--decorate=no",
+ "--no-prefix", "--submodule=short",
@@ range-diff.c: static int read_patches(const char *range, struct string_list *list,
"--output-indicator-context=#",
"--no-abbrev-commit",
"--pretty=medium",
- "--notes",
-+ "--standard-notes",
NULL);
++ if (implicit_notes_arg)
++ strvec_push(&cp.args, "--notes");
strvec_push(&cp.args, range);
if (other_arg)
-
- ## revision.c ##
-@@ revision.c: static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
- disable_display_notes(&revs->notes_opt, &revs->show_notes);
- revs->show_notes_given = 1;
- } else if (!strcmp(arg, "--standard-notes")) {
-- revs->show_notes_given = 1;
-- revs->notes_opt.use_default_notes = 1;
-+ disable_display_notes(&revs->notes_opt, &revs->show_notes);
-+ revs->show_notes_given = 0;
-+ enable_default_display_notes(&revs->notes_opt,
-+ &revs->show_notes);
-+ revs->notes_opt.use_default_notes = -1;
- } else if (!strcmp(arg, "--no-standard-notes")) {
- revs->notes_opt.use_default_notes = 0;
- } else if (!strcmp(arg, "--oneline")) {
+ strvec_pushv(&cp.args, other_arg->v);
## t/t3206-range-diff.sh ##
@@ t/t3206-range-diff.sh: test_expect_success 'range-diff with multiple --notes' '
2: f7308b7abf < -: ---------- doc: pretty-options: remove documentation for deprecated options
3: 80245bbb7e < -: ---------- revision: comment `--no-standard-notes` as deprecated
--
2.42.0
next prev parent reply other threads:[~2023-09-10 22:07 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-30 10:41 [RFC PATCH 0/3] range-diff: treat notes like `log` Kristoffer Haugsbakk
2023-05-30 10:41 ` [RFC PATCH 1/3] " Kristoffer Haugsbakk
2023-06-01 18:20 ` Jeff King
2023-06-02 10:06 ` Kristoffer Haugsbakk
2023-05-30 10:41 ` [RFC PATCH 2/3] doc: pretty-options: remove documentation for deprecated options Kristoffer Haugsbakk
2023-05-30 10:41 ` [RFC PATCH 3/3] revision: comment `--no-standard-notes` as deprecated Kristoffer Haugsbakk
2023-06-11 18:15 ` [PATCH v1 0/3] range-diff: treat notes like `log` Kristoffer Haugsbakk
2023-06-11 18:15 ` [PATCH v1 1/3] " Kristoffer Haugsbakk
2023-06-11 18:15 ` [PATCH v1 2/3] doc: pretty-options: remove documentation for deprecated options Kristoffer Haugsbakk
2023-06-11 18:15 ` [PATCH v1 3/3] revision: comment `--no-standard-notes` as deprecated Kristoffer Haugsbakk
2023-06-12 22:21 ` Junio C Hamano
2023-06-13 9:46 ` Kristoffer Haugsbakk
2023-06-12 22:25 ` [PATCH v1 0/3] range-diff: treat notes like `log` Junio C Hamano
2023-06-13 5:43 ` Kristoffer Haugsbakk
2023-09-01 16:18 ` [PATCH v2 " Kristoffer Haugsbakk
2023-09-01 16:19 ` [PATCH v2 1/3] " Kristoffer Haugsbakk
2023-09-03 12:17 ` Johannes Schindelin
2023-09-04 17:10 ` Kristoffer Haugsbakk
2023-09-05 10:56 ` Johannes Schindelin
2023-09-05 22:19 ` Junio C Hamano
2023-09-01 16:19 ` [PATCH v2 2/3] doc: pretty-options: remove documentation for deprecated options Kristoffer Haugsbakk
2023-09-01 16:19 ` [PATCH v2 3/3] revision: comment `--no-standard-notes` as deprecated Kristoffer Haugsbakk
2023-09-10 22:06 ` Kristoffer Haugsbakk [this message]
2023-09-10 22:06 ` [PATCH v3 1/1] range-diff: treat notes like `log` 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.1694383247.git.code@khaugsbakk.name \
--to=code@khaugsbakk.name \
--cc=git@vger.kernel.org \
--cc=johannes.schindelin@gmx.de \
--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 read-only IMAP folder(s) and NNTP newsgroup(s).