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

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Junio C Hamano <gitster@pobox.com>
Cc: Kristoffer Haugsbakk <code@khaugsbakk.name>,
	git@vger.kernel.org, Denton Liu <liu.denton@gmail.com>,
	Jeff King <peff@peff.net>
Subject: Re: [PATCH v3 1/1] range-diff: treat notes like `log`
Date: Thu, 14 Sep 2023 10:29:31 +0200 (CEST)	[thread overview]
Message-ID: <dd2958c5-58bf-86dd-b666-9033259a8e1a@gmx.de> (raw)
In-Reply-To: <xmqqzg1strgx.fsf@gitster.g>

[-- Attachment #1: Type: text/plain, Size: 4717 bytes --]

Hi Junio,

On Mon, 11 Sep 2023, Junio C Hamano wrote:

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