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

[Jeff King <peff@peff.net>]

On Mon, Apr 01, 2024 at 04:50:32PM +0100, Peter Krefting wrote:

> I am trying to run some comparisons, but I'm not entirely certain what the
> parameters are that were passed to "ls-tree", as it doesn't actually run it
> through a command line. I tried the v1.0.0^0 and are seeing discrepancies in
> the line count. I need to check if it is my configuration that causes it, or
> something else:
> 
>   $ git diff-tree --pretty --stat --summary --cc v1.0.0^0 | grep clone-pack.c
>    clone-pack.c                                     | 153 ++----------------
>   $ git show --stat --summary --no-abbrev-commit v1.0.0^0 | grep clone-pack.c
>    clone-pack.c                                     | 151 ++----------------
> 
> (these are the options I've currently landed on)

Hmm, I get 153 for both. Presumably it's due to some config that only
git-show respects...

Aha. If I set diff.algorithm to "patience", I get 151. And I think
bisect would produce the same, because it loads diff_ui_config() before
running the internal diff-tree.

So I think this is fine.

> > $ git show -s --stat --summary --first-parent v1.0.0^0
> 
> Hmm, the git show manual page doesn't document supporting "--first-parent".

I think that's a documentation bug(-ish). We do not include all of the
traversal-related options that "git log" could use because "git show"
does not traverse by default. But it does also affect diffs, per the
comment added to git-log's documentation in e58142add4
(doc/rev-list-options: document --first-parent changes merges format,
2020-12-21).

But these days we have "--diff-merges=first-parent", which I think is a
more intuitive way to specify the same thing for git-show. And it is
documented. So I'd say we could probably continue to not mention
"--first-parent" itself for git-show.

> Jeff King:
> 
> > I guess that commit is what brought me into the cc. I have not been
> > following this topic too closely, but generally I'm in favor of using
> > "git show". I even suggested it back then, but I think Christian
> > preferred not using an external process if we could avoid it.
> 
> I saw the code that tried to avoid calling one. I don't know the internals
> well enough here to figure out if we can do without, even when using git
> show?

There's not really an easy way.

I think the only thing you could do is call cmd_show(), but I'm
skeptical of that approach in general. The builtin top-level commands
are not designed to be run from other spots. And while it will generally
work, there will be corner cases (e.g., loading config that touches
globals, affecting the calling command in unexpected ways). I suspect
you could largely get away with it here where showing the commit is the
last thing we do, but I don't think it's a good pattern to get into.

> That made me realize, if "git show" runs things through a pager, wouldn't it
> then lose the "%s is the first %s commit\n" message printed by
> bisect_next_all() before calling the function to show the contents?
> 
> Is that fixable?

Good catch. IMHO we should disable the pager entirely by sticking
"--no-pager" at the front of the child argv. But then, maybe somebody
would like the output to be paged? I wouldn't.

If we really wanted to keep the pager for git-show, I guess we'd need to
have it print the "%s is the first %s commit" message. The only way I
can think to do that is to pass it as a custom --format. But then we'd
need to additionally specify all of the usual "medium" format as a
custom format, too, which is quite ugly.

> I think I added it because the diff-tree command did something similar. I
> can drop that as well ("bisect" displays the full commit hash anyway). I
> guess it mostly is for merges where we show the parent hashes?

No, I don't think the "Merge:" lines are affected by it either way.
Those are always abbreviated, and looking at pretty.c's
add_merge_info(), I don't think there is any config that affects it.

-Peff