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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

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

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

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

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

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