[RFC] bisect: Honor log.date

Commit Message

Peter Krefting March 28, 2024, 8:53 p.m. UTC

When bisect finds the target commit to display, it calls git diff-tree
to do so. This is a plumbing command that is not affected by the user's
log.date setting. Switch to instead use "git show", which does honor
it.

Reported-by: Michael Osipov <[e-mail]>
Signed-off-By: Peter Krefting <[e-mail]>
---
  bisect.c | 26 +++++++++++---------------
  1 file changed, 11 insertions(+), 15 deletions(-)

Junio C Hamano:

> Instead of focusing too narrowly on the log.date option, that would 
> only tweak the date format, it may be a more fruitful way to invest 
> brainwaves in to consider the feasibility of switching to use "git 
> show" there.

Indeed.

Here is a patch that does exactly that.

This is my first patch to the actual codebase in Git, so it might be 
bit rough; improvements are welcome. I might need to change something 
in the test suite as well?

With this patch applied, running with log.date=iso and 
log.decorate=short, I get this output:

  $ ./git-bisect start
  [...]
  $ ./git-bisect good v2.43.2
  [...]
  $ ./git-bisect bad v2.43.3
  [...]
  $ ./git-bisect good
  0d464a4e6a5a19bd8fbea1deae22d48d14dccb01 is the first bad commit
  commit 0d464a4e6a5a19bd8fbea1deae22d48d14dccb01 (tag: v2.43.3)
  Author: Junio C Hamano <[e-mail]>
  Date:   2024-02-22 16:13:38 -0800

      Git 2.43.3

  Signed-off-by: Junio C Hamano <[e-mail]>

which is the format I expect.

Comments

Eric Sunshine March 28, 2024, 9:38 p.m. UTC | #1

On Thu, Mar 28, 2024 at 4:54 PM Peter Krefting <[e-mail]> wrote:
> When bisect finds the target commit to display, it calls git diff-tree
> to do so. This is a plumbing command that is not affected by the user's
> log.date setting. Switch to instead use "git show", which does honor
> it.
>
> Reported-by: Michael Osipov <[e-mail]>
> Signed-off-By: Peter Krefting <[e-mail]>
> ---
> diff --git a/bisect.c b/bisect.c
> @@ -959,23 +959,19 @@ static enum bisect_error check_good_are_ancestors_of_bad(struct repository *r,
> +       /* Invoke "git show --pretty=medium --shortstat --no-abbrev-commit --no-patch $object" */
> +       strvec_pushl(&show.args, "show", "--pretty=medium", "--shortstat", "--no-abbrev-commit", "--no-patch",
> +                    oid_to_hex(&commit->object.oid), NULL);
> +       show.git_cmd = 1;

Nit: The comment doesn't tell the reader anything that the code itself
isn't already clearly telling the reader, thus the comment is
redundant and unnecessary. Moreover, the comment is likely to become
outdated when people adjust the code but forget to update the comment.
As such, I'd recommend dropping the comment altogether.

Peter Krefting March 28, 2024, 11:18 p.m. UTC | #2

2024-03-28 22:38 skrev Eric Sunshine:

> Nit: The comment doesn't tell the reader anything that the code itself
> isn't already clearly telling the reader, thus the comment is
> redundant and unnecessary. Moreover, the comment is likely to become
> outdated when people adjust the code but forget to update the comment.
> As such, I'd recommend dropping the comment altogether.

Yes, that does make sense. I copied the code for invoking "git show" 
from builtin/notes.c, which does have that type of redundant comment.

I'll remove it.

Patch

diff --git a/bisect.c b/bisect.c
index 8487f8cd1b..0f7126c32b 100644
--- a/bisect.c
+++ b/bisect.c
@@ -959,23 +959,19 @@  static enum bisect_error check_good_are_ancestors_of_bad(struct repository *r,
  }

  /*
- * This does "git diff-tree --pretty COMMIT" without one fork+exec.
+ * Runs "git show" to display a commit
   */
-static void show_diff_tree(struct repository *r,
-			   const char *prefix,
-			   struct commit *commit)
+static void show_commit(struct commit *commit)
  {
-	const char *argv[] = {
-		"diff-tree", "--pretty", "--stat", "--summary", "--cc", NULL
-	};
-	struct rev_info opt;
+	struct child_process show = CHILD_PROCESS_INIT;

-	git_config(git_diff_ui_config, NULL);
-	repo_init_revisions(r, &opt, prefix);
-
-	setup_revisions(ARRAY_SIZE(argv) - 1, argv, &opt, NULL);
-	log_tree_commit(&opt, commit);
-	release_revisions(&opt);
+	/* Invoke "git show --pretty=medium --shortstat --no-abbrev-commit --no-patch $object" */
+	strvec_pushl(&show.args, "show", "--pretty=medium", "--shortstat", "--no-abbrev-commit", "--no-patch",
+		     oid_to_hex(&commit->object.oid), NULL);
+	show.git_cmd = 1;
+	if (run_command(&show))
+		die(_("unable to start 'show' for object '%s'"),
+		    oid_to_hex(&commit->object.oid));
  }

  /*
@@ -1092,7 +1088,7 @@  enum bisect_error bisect_next_all(struct repository *r, const char *prefix)
  		printf("%s is the first %s commit\n", oid_to_hex(bisect_rev),
  			term_bad);

-		show_diff_tree(r, prefix, revs.commits->item);
+		show_commit(revs.commits->item);
  		/*
  		 * This means the bisection process succeeded.
  		 * Using BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND (-10)