git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v3 1/1] range-diff: treat notes like `log`

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Sep 14, 2023, 08:29 UTC
Message-ID
<dd2958c5-58bf-86dd-b666-9033259a8e1a@gmx.de>
In-Reply-To
<xmqqzg1strgx.fsf@gitster.g>
Hi Junio,
On Mon, 11 Sep 2023, Junio C Hamano wrote:
Show 38 quoted lines
> Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
>
> >> 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).
> >
> > Co-authored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> > Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> > ---
>
> 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.
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 --
diff --git a/range-diff.c b/range-diff.c
index fbb81a92cc0..56f6870ff91 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -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);
diff --git a/revision.c b/revision.c
index 2f4c53ea207..49d385257ac 100644
--- a/revision.c
+++ b/revision.c
@@ -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;
 }

diff --git a/revision.h b/revision.h
index 82ab400139d..50091bbd13f 100644
--- a/revision.h
+++ b/revision.h
@@ -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,
Previous: Junio C HamanoNext: Junio C Hamano
Message 26 of 41 in “range-diff: treat notes like `log`”
  1. 0/3 range-diff: treat notes like `log`Kristoffer Haugsbakk, May 30, 2023
  2. 1/3 range-diff: treat notes like `log`Kristoffer Haugsbakk, May 30, 2023
  3. Jeff KingJun 1, 2023
  4. Kristoffer HaugsbakkJun 2, 2023
  5. 2/3 doc: pretty-options: remove documentation for deprecated optionsKristoffer Haugsbakk, May 30, 2023
  6. 3/3 revision: comment `--no-standard-notes` as deprecatedKristoffer Haugsbakk, May 30, 2023
  7. 0/3 range-diff: treat notes like `log`Kristoffer Haugsbakk, Jun 11, 2023
  8. 2/3 doc: pretty-options: remove documentation for deprecated optionsKristoffer Haugsbakk, Jun 11, 2023
  9. 3/3 revision: comment `--no-standard-notes` as deprecatedKristoffer Haugsbakk, Jun 11, 2023
  10. Junio C HamanoJun 12, 2023
  11. Kristoffer HaugsbakkJun 13, 2023
  12. 1/3 range-diff: treat notes like `log`Kristoffer Haugsbakk, Jun 11, 2023
  13. Junio C HamanoJun 12, 2023
  14. Kristoffer HaugsbakkJun 13, 2023
  15. 0/3 range-diff: treat notes like `log`Kristoffer Haugsbakk, Sep 1, 2023
  16. 1/3 range-diff: treat notes like `log`Kristoffer Haugsbakk, Sep 1, 2023
  17. Johannes SchindelinSep 3, 2023
  18. Kristoffer HaugsbakkSep 4, 2023
  19. Johannes SchindelinSep 5, 2023
  20. Junio C HamanoSep 5, 2023
  21. 2/3 doc: pretty-options: remove documentation for deprecated optionsKristoffer Haugsbakk, Sep 1, 2023
  22. 3/3 revision: comment `--no-standard-notes` as deprecatedKristoffer Haugsbakk, Sep 1, 2023
  23. 0/1 range-diff: treat notes like `log`Kristoffer Haugsbakk, Sep 10, 2023
  24. 1/1 range-diff: treat notes like `log`Kristoffer Haugsbakk, Sep 10, 2023
  25. Junio C HamanoSep 11, 2023
  26. Johannes SchindelinSep 14, 2023
  27. Junio C HamanoSep 14, 2023
  28. Kristoffer HaugsbakkSep 14, 2023
  29. Junio C HamanoSep 19, 2023
  30. Kristoffer HaugsbakkSep 19, 2023
  31. Johannes SchindelinSep 11, 2023
  32. 0/1 range-diff: treat notes like `log`Kristoffer Haugsbakk, Sep 19, 2023
  33. 1/1 range-diff: treat notes like `log`Kristoffer Haugsbakk, Sep 19, 2023
  34. Junio C HamanoSep 19, 2023
  35. Kristoffer HaugsbakkSep 19, 2023
  36. Junio C HamanoSep 19, 2023
  37. Kristoffer HaugsbakkSep 19, 2023
  38. Junio C HamanoSep 19, 2023
  39. 0/1 range-diff: treat notes like `log`Kristoffer Haugsbakk, Sep 19, 2023
  40. 1/1 range-diff: treat notes like `log`Kristoffer Haugsbakk, Sep 19, 2023
  41. Johannes SchindelinSep 21, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.