git log -g bizarre behaviour

git log -g bizarre behaviour

[Dennis Kaarsemaker]

I'm attempting to understand the log [-g] / reflog code enough to
untangle them and make reflog walking work for more than just commit
objects [see gmane 283169]. I found something which I think is wrong,
and would break after my changes.

git log -g HEAD^ and git log -g v2.7.0^ give no output. This is
expected, as those are not things that have a reflog. But git log -g
v2.7.0 seems to ignore -g and gives the normal log. git reflog v2.7.0
does something even more bizarre:

$ GIT_PAGER= git reflog v2.7.0 
7548842 (tag: v2.7.0, seveas/master, origin/master, origin/HEAD) 3e9226a 833e482 (tag: v2.6.5, gitster/maint-2.6) e3073cf e002527 e54d0f5 06b5c93 34872f0 5863990 02103b3 503b1ef 28274d0 (tag: v2.7.0-rc3) aecb997 7195733 e929264 ce858c0 5fa9ab8

Yes, that's a humongous line (I've only copied parts of it).

I'd like to make git log -g / git reflog abort early when trying to
display a reflog of a ref that has no reflog. Objections?

-- 
Dennis Kaarsemaker
www.kaarsemaker.net

Re: git log -g bizarre behaviour

[Junio C Hamano]

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

> I'm attempting to understand the log [-g] / reflog code enough to
> untangle them and make reflog walking work for more than just commit
> objects [see gmane 283169]. I found something which I think is wrong,
> and would break after my changes.
>
> git log -g HEAD^ and git log -g v2.7.0^ give no output. This is
> expected, as those are not things that have a reflog.

OK.

> But git log -g v2.7.0 seems to ignore -g and gives the normal
> log.

That sounds clearly broken, and I think I see how that happens from
the hacky way the "-g" traversal was bolted onto the revision
traversal machinery.

I _think_ "git log -g" (and by extension "git reflog" which is just
a short-hand to giving a few more options to that command) ought to

 * Iterate over the _objects_ that used to be at the tip of the ref;
 * Show each of these objects as if they were fed to "git show".

This clearly is not possible without major surgery, including
ripping out the hacky "-g" traversal from the revision traversal
machinery and perhaps lifting it up a few levels in the callchain,
as many functions in that callchain want to work on commits.

Contrast these two:

    $ git log -1 v2.7.0
    $ git show v2.7.0

> I'd like to make git log -g / git reflog abort early when trying to
> display a reflog of a ref that has no reflog. Objections?

Do you mean

	$ git checkout -b testing
        $ rm -f .git/logs/refs/heads/testing
        $ git log -g testing

will be changed from a silent no-op to an abort with error?

I do not see a need for such a change--does that count as an
objection?

Thanks.

Re: git log -g bizarre behaviour

[Dennis Kaarsemaker]

On ma, 2016-02-01 at 15:37 -0800, Junio C Hamano wrote:
> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> 
> > I'm attempting to understand the log [-g] / reflog code enough to
> > untangle them and make reflog walking work for more than just
> > commit
> > objects [see gmane 283169]. I found something which I think is
> > wrong,
> > and would break after my changes.
> > 
> > git log -g HEAD^ and git log -g v2.7.0^ give no output. This is
> > expected, as those are not things that have a reflog.
> 
> OK.
> 
> > But git log -g v2.7.0 seems to ignore -g and gives the normal
> > log.
> 
> That sounds clearly broken, and I think I see how that happens from
> the hacky way the "-g" traversal was bolted onto the revision
> traversal machinery.
> 
> I _think_ "git log -g" (and by extension "git reflog" which is just
> a short-hand to giving a few more options to that command) ought to
> 
>  * Iterate over the _objects_ that used to be at the tip of the ref;
>  * Show each of these objects as if they were fed to "git show".

That's what I am trying to achieve. Though not quite like 'git show', I
want to emulate the --oneline putput for non-commit objects too.

> This clearly is not possible without major surgery, including
> ripping out the hacky "-g" traversal from the revision traversal
> machinery and perhaps lifting it up a few levels in the callchain,
> as many functions in that callchain want to work on commits.

Yup. I'm planning to either split cmd_log_walk or make its behaviour
depend on whether we're traversing the reflog (don't call get_revision,
but call a new get_reflog_entry function). And then rip out the reflog
handling from revision.c and redo (parts of) reflog-walk.c to
accomodate the cmd_log_walk (split|replacement) that deals with reflogs
better.

> Contrast these two:
> 
>     $ git log -1 v2.7.0
>     $ git show v2.7.0
> 
> > I'd like to make git log -g / git reflog abort early when trying to
> > display a reflog of a ref that has no reflog. Objections?
> 
> Do you mean
> 
> 	$ git checkout -b testing
>         $ rm -f .git/logs/refs/heads/testing
>         $ git log -g testing
> 
> will be changed from a silent no-op to an abort with error?
> 
> I do not see a need for such a change--does that count as an
> objection?

No, I'd like to change:

$ ls .git/logs/refs/tags/v2.7.0
ls: cannot access .git/logs/refs/tags/v2.7.0: No such file or directory
$ git (log -g|reflog) v2.7.0

>From the bizarre behaviour above to a silent noop. But before I do that
in a rewrite (by simply not implementing it), I'd like to have that
behavior now as well and add tests for it.

-- 
Dennis Kaarsemaker
http://www.kaarsemaker.net

Re: git log -g bizarre behaviour

[Junio C Hamano]

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

> On ma, 2016-02-01 at 15:37 -0800, Junio C Hamano wrote:
>
>> Do you mean
>> 
>> 	$ git checkout -b testing
>>         $ rm -f .git/logs/refs/heads/testing
>>         $ git log -g testing
>> 
>> will be changed from a silent no-op to an abort with error?
>> 
>> I do not see a need for such a change--does that count as an
>> objection?
>
> No, I'd like to change:
>
> $ ls .git/logs/refs/tags/v2.7.0
> ls: cannot access .git/logs/refs/tags/v2.7.0: No such file or directory
> $ git (log -g|reflog) v2.7.0
> From the bizarre behaviour above to a silent noop.

When there is nothing to show, we do not show anything, and that is
just like "git log v2.7.0..v2.7.0" is silent.

I do not find the silence bizarre at all.

Re: git log -g bizarre behaviour

[Dennis Kaarsemaker]

On di, 2016-02-02 at 11:32 -0800, Junio C Hamano wrote:
> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> 
> > On ma, 2016-02-01 at 15:37 -0800, Junio C Hamano wrote:
> > 
> > > Do you mean
> > > 
> > > 	$ git checkout -b testing
> > >         $ rm -f .git/logs/refs/heads/testing
> > >         $ git log -g testing
> > > 
> > > will be changed from a silent no-op to an abort with error?
> > > 
> > > I do not see a need for such a change--does that count as an
> > > objection?
> > 
> > No, I'd like to change:
> > 
> > $ ls .git/logs/refs/tags/v2.7.0
> > ls: cannot access .git/logs/refs/tags/v2.7.0: No such file or
> > directory
> > $ git (log -g|reflog) v2.7.0
> > From the bizarre behaviour above to a silent noop.
> 
> When there is nothing to show, we do not show anything, 

As I demonstrated in the text that you cut: that is not true.
git log -g v2.7.0 and git reflog v2.7.0 are *not* silent, but buggy. I
would like to make them silent.

> and that is just like "git log v2.7.0..v2.7.0" is silent.
> 
> I do not find the silence bizarre at all.

I'll take that as an agreement then :)
-- 
Dennis Kaarsemaker
www.kaarsemaker.net

Re: git log -g bizarre behaviour

[Junio C Hamano]

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

>> > $ git (log -g|reflog) v2.7.0
>> > From the bizarre behaviour above to a silent noop.
>
> As I demonstrated in the text that you cut: that is not true.
> git log -g v2.7.0 and git reflog v2.7.0 are *not* silent, but buggy. I
> would like to make them silent.

Ah, sorry, I totally misread your "bizarre".  Yes, "log -g" that
walks the history not the reflog is "bizarre" and wrong, which we
already agreed in the previous exchange.  A fixed behaviour that
walks only the reflog entries should become a "silent noop".

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

[Dennis Kaarsemaker]

git log -g (and by extension, git reflog) gets mightly confused when
trying to display the reflog of something that is not a ref that has a
reflog. We can help by teaching handle_revision_arg to check all
revision arguments for reflog existence if it's in reflog mode.

git log -g something-that-is-not-a ref makes no sense, so let's die when
the user is trying that. git log -g ref-that-has-no-reflog is perfectly
sensible, so we just ignore it.

Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
---
 revision.c             | 12 ++++++++++++
 t/t1411-reflog-show.sh | 10 ++++++++++
 2 files changed, 22 insertions(+)

diff --git a/revision.c b/revision.c
index 0a282f5..43182c6 100644
--- a/revision.c
+++ b/revision.c
@@ -1498,6 +1498,18 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi
 
 	flags = flags & UNINTERESTING ? flags | BOTTOM : flags & ~BOTTOM;
 
+	if (revs->reflog_info) {
+		/*
+		 * The reflog iterator gets confused when fed things that don't
+		 * have reflogs. Help it along a bit
+		 */
+		if (strchr(arg, '@') != arg &&
+		    !dwim_ref(arg, strchrnul(arg, '@')-arg, sha1, &dotdot))
+			die("only refs can have reflogs");
+		if(!reflog_exists(dotdot))
+			return 0;
+	}
+
 	dotdot = strstr(arg, "..");
 	if (dotdot) {
 		unsigned char from_sha1[20];
diff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh
index 6ac7734..e55518f 100755
--- a/t/t1411-reflog-show.sh
+++ b/t/t1411-reflog-show.sh
@@ -171,4 +171,14 @@ test_expect_success 'reflog exists works' '
 	! git reflog exists refs/heads/nonexistent
 '
 
+test_expect_success 'reflog against non-ref dies' '
+	test_must_fail git reflog HEAD^
+'
+
+test_expect_success 'reflog against ref with no log is empty' '
+	git tag nolog &&
+	git reflog nolog > actual &&
+	test_line_count = 0 actual
+'
+
 test_done
-- 
2.7.0-345-gadc6f59

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

[Junio C Hamano]

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

> +	if (revs->reflog_info) {
> +		/*
> +		 * The reflog iterator gets confused when fed things that don't
> +		 * have reflogs. Help it along a bit
> +		 */
> +		if (strchr(arg, '@') != arg &&

Is this merely an expensive way to write *arg != '@', or is there
something else I am missing?

> +		    !dwim_ref(arg, strchrnul(arg, '@')-arg, sha1, &dotdot))
> +			die("only refs can have reflogs");

Is "foo@23" a forbidden branch name?

Is this looking for a dotdot?  If you are introducing a new scope,
you can afford to invent a variable with a name that reflects its
purpose.

Style: a binary operation like '-' (subtract) have SP on both sides
of it.

> +		if(!reflog_exists(dotdot))

Style: one SP between a syntactic keyword like 'if' and opening
parenthesis is required.

I have a suspicion that in your final "fixed" code, it may be a
better design not to let the command line argument for "-g"
processing pass through this function at all.

For example, what should "git log -g master next" do?  Merge two
reflog entries in chronological order and show each of them as if
they are thrown at "git show" one by one?  Does that mesh well with
other options like "--date-order/--topo-order"?

For another example, what should "git log -g master..next" do?

Or "git log -g master^^^"?

These are merely a few example inputs I can think of off in 5
seconds and I think none of the above makes much sense, but parsing
these is the primary purpose of this function.

So, I dunno.  I gave a few "coding" comments, but I am not sure if
you are touching the right codepath in the first place.

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

[Dennis Kaarsemaker]

On di, 2016-02-02 at 16:21 -0800, Junio C Hamano wrote:
> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> 
> > +	if (revs->reflog_info) {
> > +		/*
> > +		 * The reflog iterator gets confused when fed
> > things that don't
> > +		 * have reflogs. Help it along a bit
> > +		 */
> > +		if (strchr(arg, '@') != arg &&
> 
> Is this merely an expensive way to write *arg != '@', or is there
> something else I am missing?

Doh. No, that's just my stupidity. I did the strchrnul bits below
first, then found out that it broke `git log -g @{0}` and came up with
the above.

> > +		    !dwim_ref(arg, strchrnul(arg, '@')-arg, sha1,
> > &dotdot))
> > +			die("only refs can have reflogs");
> 
> Is "foo@23" a forbidden branch name?

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

> Is this looking for a dotdot?  If you are introducing a new scope,
> you can afford to invent a variable with a name that reflects its
> purpose.

True. I just adhered to surrounding style (the dotdot variable is
abused below as well). Lame excuse, I know :)

> Style: a binary operation like '-' (subtract) have SP on both sides
> of it.
> 
> > +		if(!reflog_exists(dotdot))
> 
> Style: one SP between a syntactic keyword like 'if' and opening
> parenthesis is required.

Ack.

> I have a suspicion that in your final "fixed" code, it may be a
> better design not to let the command line argument for "-g"
> processing pass through this function at all.
>
> For example, what should "git log -g master next" do?  Merge two
> reflog entries in chronological order and show each of them as if
> they are thrown at "git show" one by one?  Does that mesh well with
> other options like "--date-order/--topo-order"?

I agree that option parsing is not the right place in the end. When -g
is given, only one ref argument should be accepted, and --date-order
etc. should cause it to barf as they don't make sense.

> For another example, what should "git log -g master..next" do?
> 
> Or "git log -g master^^^"?
>
> These are merely a few example inputs I can think of off in 5
> seconds and I think none of the above makes much sense, but parsing
> these is the primary purpose of this function.

With this patch they die with an error as they make no sense.

> So, I dunno.  I gave a few "coding" comments, but I am not sure if
> you are touching the right codepath in the first place.

I was trying to go for a minimal change to fix a bug without
introducing regressions. It feels weird to do it in the option parsing
code, but I didn't want to make this behaviour fix wait for a rewrite
of the log -g functionality, as I have no idea when I'll be able to
finish that. It already took me a few hours to come up with this, as I
had not touched the related code at all before :)

-- 
Dennis Kaarsemaker
www.kaarsemaker.net

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

[Junio C Hamano]

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.