Re: [PATCH] log -g: ignore revision parameters that have no reflog

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

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

> It is not, the code should look for @{, not @.

Not exactly.

    $ git show -s --format='%h %s' 'HEAD^{/@{3}}' --
    55d5d5b combine-diff.c: fix performance problem when folding ...

The commit has a line with a string "@@@" on it and the regular
expression asked for 3 '@', which shows that scanning for "@{" is
not a good way forward--it merely opens another can of worms.

Hopefully by now you have realized that a band-aid to add an ad-hoc
code that second-guesses what the existing code does for real while
parsing the command line is not a good way forward.

Perhaps we may want to step back a bit.

Where is the book-keeping information used for "-g" processing
handled in the codechain?  Upon seeing "-g", the parser calls
init_reflog_walk() to make revs->reflog_info non-NULL.

What are the codepaths that use this field?

We can see the function add_pending_object_with_path() refers to
this revs->reflog_info field, when it calls add_reflog_for_walk(),
with the "name" it receives, after some mangling.

The called function, add_reflog_for_walk(), finds, from an object
and its name, the ref whose log is going to be enumerated.  It looks
at the name, optionally finds '@' in it, and eventually calls the
function read_complete_reflog() [*1*].

We can infer that, by the time it does so, it must have figured out
of which ref it wants to read the reflog.

And it already has calls to die() and a few "return -1" to signal a
non-fatal error to the caller.  Perhaps instead of letting it to
punt and resort to the normal history walking, the code should
realize that some refs do not have reflog (and no non-refs has
reflog) and diagnose it as an error and die()?

Perhaps one of these two functions is a much better place to do your
improvement?  The caller, add_pending_object_with_path(), does some
mangling of "name" that is given by its caller before calling into
the callee, add_reflog_for_walk().  It could be that name mangling
that is leading to a wrong result.  More likely, the way the callee
figures out which ref it needs to read the reflog for given the
"name" may be what you need to fix.

UNLESS we are losing the information we got directly from the user
at the command line before the control is passed down through the
callchain to reach these two functions, that is.  In such a case,
I'd agree that we would need additional checks much closer to the
input.

But I think the callers of this callchain are passing what the user
gave us pretty much verbatim down this callchain, so I would expect
that the leaf callee in this discussion, add_reflog_for_walk(),
would have enough information.


[Footnote]

*1* Yuck.