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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 1, 2024, 17:03 UTC
Message-ID: <xmqqmsqd9fse.fsf@gitster.g>
In-Reply-To: <20240401163209.GB3120568@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 12 quoted lines

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

It's one of the "show is a command in the log family, so some of the options that are appropriate to log applies there". The ones that are not useful are the ones about commit walking (e.g., "git show --no-merges seen" would probably show nothing), but many are still relevant. After all "git show" is a "git log --no-walk --cc" in disguise. The "--first-parent" option affects both traversal (which is useless in the context of "git show" that does not walk) and also diff generation (which does make it show the diffstat/summary/patch relative to the first parent), as you two saw.

Show 5 quoted lines

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

True, but this is "we show the single commit we found before exiting"; executing "git show" as an external program is fine and not worth "optimizing out" the cost of starting another process.

Show 7 quoted lines

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

Exactly. Anybody who turns run_command("foo") into blindly calling cmd_foo() should be shot, twice ;-). The right way to turn run_command("foo") into an internal call is not to call cmd_foo(), but to refactor cmd_foo() into the part that sets up the global state and the part that does the "foo" thing, and make the latter a reusable function.

In the longer run, if we had infinite engineering resources, it would be nice to have everything callable by everything else internally, is it worth doing for this case? I dunno.

Show 9 quoted lines

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

Hardcoded --no-pager is a good workaround. But if the output is long and needs paging, wouldn't we see what was shown before we spawned "less" on the screen when we quit it? Running

    $ (echo message here ; git log --help)

and then saying 'q' to exit the pager leaves me "message" after that command line.

Show 5 quoted lines

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

;-)  Ugly but fun.

I wonder how hard it is to add %(default-output) placeholder for the pretty machinery.

Thanks.