bisect does not respect 'log.date'

bisect does not respect 'log.date'

[Osipov, Michael (IN IT IN)]

Folks,

I am running git version 2.43.0 and consider the following config:
> $ git config --system --list
> core.eol=native
> log.date=iso-strict
> color.ui=auto

So date output looks fine with 'log' and 'show':
> osipovmi@deblndw011x:~/var/Projekte/tomcat-native ((ba1454e15...)|BISECTING)
> $ git log
> commit ba1454e15619a44fe66d86f59c766c0cc25323eb (HEAD)
> Author: Mark Thomas <markt@apache.org>
> Date:   2024-02-13T08:27:43+00:00
> 
>     Fix link
> 
and
> osipovmi@deblndw011x:~/var/Projekte/tomcat-native ((ba1454e15...)|BISECTING)
> $ git show HEAD^
> commit ef40b6d00c4bdaa23960b5dc0eaac28cce758d29
> Author: Mark Thomas <markt@apache.org>
> Date:   2024-02-06T16:40:25+00:00
> 
>     Update minimum APR version in a few more places
> 

now let's bisect:
> osipovmi@deblndw011x:~/var/Projekte/tomcat-native (apache-1.3.x =)
> $ git bisect start HEAD HEAD~4
> Binäre Suche: danach noch 1 Commit zum Testen übrig (ungefähr 1 Schritt)
> [ef40b6d00c4bdaa23960b5dc0eaac28cce758d29] Update minimum APR version in a few more places
...fast forward
> osipovmi@deblndw011x:~/var/Projekte/tomcat-native ((ba1454e15...)|BISECTING)
> $ git bisect bad
> ba1454e15619a44fe66d86f59c766c0cc25323eb is the first bad commit
> commit ba1454e15619a44fe66d86f59c766c0cc25323eb
> Author: Mark Thomas <markt@apache.org>
> Date:   Tue Feb 13 08:27:43 2024 +0000
> 
>     Fix link
> 
>  xdocs/news/project.xml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

The config for date format is ignored. Is uses the default value.

An oversight or bug?

Michael

Re: bisect does not respect 'log.date'

[Junio C Hamano]

"Osipov, Michael (IN IT IN)" <michael.osipov@innomotics.com> writes:

> An oversight or bug?

Neither, i.e. WAI, I would say.

The configuration variable log.date is about "git log" and its
documentation makes no promises how "git bisect" may or may not be
affected.

Having said that, I think it is not unreasonable for you to make it
a feature request to add bisect.dateformat or whatever.  The only
interesting part from the output being the exact commit object name
the problem bisects to, I personally would not see it a high priority
feature request, though.

Thanks.

Re: bisect does not respect 'log.date'

[Osipov, Michael (IN IT IN)]

On 2024-03-13 19:24, Junio C Hamano wrote:
> "Osipov, Michael (IN IT IN)" <michael.osipov@innomotics.com> writes:
> 
>> An oversight or bug?
> 
> Neither, i.e. WAI, I would say.
> 
> The configuration variable log.date is about "git log" and its
> documentation makes no promises how "git bisect" may or may not be
> affected.
> 
> Having said that, I think it is not unreasonable for you to make it
> a feature request to add bisect.dateformat or whatever.  The only
> interesting part from the output being the exact commit object name
> the problem bisects to, I personally would not see it a high priority
> feature request, though.

Interesting thought, but that also means that "git show" misbehaves 
because it respects "log.date" and there is no "show.date". I still 
think that consistency shouldn't be obeyed.
I'd be happy if someone could consider this improvement.

Michael

Re: bisect does not respect 'log.date'

[Peter Krefting]

Osipov, Michael (IN IT IN):

> Interesting thought, but that also means that "git show" misbehaves because 
> it respects "log.date" and there is no "show.date". I still think that 
> consistency shouldn't be obeyed.
> I'd be happy if someone could consider this improvement.

I've also been annoyed at this. Everything else respects the date 
setting.

Bisect displays the commit through an invocation of "git diff-tree 
--pretty". This command does not respect the log.date setting, but it 
can be passed the --date parameter to format the date.

The question is what is the correct way of fixing this; is it to make 
"git diff-tree --pretty" respect the "log.date" option, or to make 
"git bisect" pass a --date pate parameter to the invocation of it?

Or perhaps everything should just be made support the "TIME_STYLE" that 
GNU tools use? GNU ls is so much nicer to use with "TIME_STYLE=long-iso" 
set.

-- 
\\// Peter - http://www.softwolves.pp.se/

Re: bisect does not respect 'log.date'

[Junio C Hamano]

Peter Krefting <peter@softwolves.pp.se> writes:

> The question is what is the correct way of fixing this; is it to make
> "git diff-tree --pretty" respect the "log.date" option, or to make

The "diff-tree" and other "plumbing" commands deliberately ignore
configuration and the point of doing so is to make sure their output
are stable without getting affected by the end-user configuration.

> "git bisect" pass a --date pate parameter to the invocation of it?

If we were to change how "bisect" reports the date of the commit,
this is a more reasonable route to go.

Are you sure that nobody is driving "git bisect" from a script and
scraping the output in such a way that a change in the output format
would break such a script?  I would say it is unlikely (they may be
scraping the output to find a commit by looking for 40-hex string,
though) that it would cause such a breakage.

But stepping back a bit.

If "git bisect" were written in the more modern era, I am reasonably
sure that it wouldn't have used "git diff-tree" when reporting the
"first bad commit".  It would have used "git show" instead, which is
at the Porcelain level and will pay attention to the configuration
variables.  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.

[RFC PATCH] bisect: Honor log.date

[Peter Krefting]

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 <michael.osipov@innomotics.com>
Signed-off-By: Peter Krefting <peter@softwolves.pp.se>
---
  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 <gitster@pobox.com>
  Date:   2024-02-22 16:13:38 -0800

      Git 2.43.3

  Signed-off-by: Junio C Hamano <gitster@pobox.com>

which is the format I expect.

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)
-- 
2.39.2

Re: [RFC PATCH] bisect: Honor log.date

[Eric Sunshine]

On Thu, Mar 28, 2024 at 4:54 PM Peter Krefting <peter@softwolves.pp.se> 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 <michael.osipov@innomotics.com>
> Signed-off-By: Peter Krefting <peter@softwolves.pp.se>
> ---
> 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.

Re: [RFC PATCH] bisect: Honor log.date

[Peter Krefting]

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.

-- 
\\// Peter - http://www.softwolves.pp.se/