{"thread":{"id":"41294","subject":"git log -g bizarre behaviour","startedAt":"2016-01-31T11:52:24Z","lastAt":"2016-02-03T18:32:22Z","messageCount":10,"participants":["Dennis Kaarsemaker","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"277112","messageId":"1454241144.2822.7.camel@kaarsemaker.net","threadId":"41294","inReplyTo":null,"subject":"git log -g bizarre behaviour","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2016-01-31T11:52:24Z","receivedAt":"2016-01-31T11:52:24Z","isPatch":false,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"I'm attempting to understand the log [-g] / reflog code enough to\nuntangle them and make reflog walking work for more than just commit\nobjects [see gmane 283169]. I found something which I think is wrong,\nand would break after my changes.\n\ngit log -g HEAD^ and git log -g v2.7.0^ give no output. This is\nexpected, as those are not things that have a reflog. But git log -g\nv2.7.0 seems to ignore -g and gives the normal log. git reflog v2.7.0\ndoes something even more bizarre:\n\n$ GIT_PAGER= git reflog v2.7.0 \n7548842 (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\n\nYes, that's a humongous line (I've only copied parts of it).\n\nI'd like to make git log -g / git reflog abort early when trying to\ndisplay a reflog of a ref that has no reflog. Objections?\n\n-- \nDennis Kaarsemaker\nwww.kaarsemaker.net\n"},{"id":"277196","messageId":"xmqqegcwt32j.fsf@gitster.mtv.corp.google.com","threadId":"41294","inReplyTo":"1454241144.2822.7.camel@kaarsemaker.net","subject":"Re: git log -g bizarre behaviour","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-01T23:37:24Z","receivedAt":"2016-02-01T23:37:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n\n> I'm attempting to understand the log [-g] / reflog code enough to\n> untangle them and make reflog walking work for more than just commit\n> objects [see gmane 283169]. I found something which I think is wrong,\n> and would break after my changes.\n>\n> git log -g HEAD^ and git log -g v2.7.0^ give no output. This is\n> expected, as those are not things that have a reflog.\n\nOK.\n\n> But git log -g v2.7.0 seems to ignore -g and gives the normal\n> log.\n\nThat sounds clearly broken, and I think I see how that happens from\nthe hacky way the \"-g\" traversal was bolted onto the revision\ntraversal machinery.\n\nI _think_ \"git log -g\" (and by extension \"git reflog\" which is just\na short-hand to giving a few more options to that command) ought to\n\n * Iterate over the _objects_ that used to be at the tip of the ref;\n * Show each of these objects as if they were fed to \"git show\".\n\nThis clearly is not possible without major surgery, including\nripping out the hacky \"-g\" traversal from the revision traversal\nmachinery and perhaps lifting it up a few levels in the callchain,\nas many functions in that callchain want to work on commits.\n\nContrast these two:\n\n    $ git log -1 v2.7.0\n    $ git show v2.7.0\n\n> I'd like to make git log -g / git reflog abort early when trying to\n> display a reflog of a ref that has no reflog. Objections?\n\nDo you mean\n\n\t$ git checkout -b testing\n        $ rm -f .git/logs/refs/heads/testing\n        $ git log -g testing\n\nwill be changed from a silent no-op to an abort with error?\n\nI do not see a need for such a change--does that count as an\nobjection?\n\nThanks.\n"},{"id":"277214","messageId":"1454401738.32711.7.camel@kaarsemaker.net","threadId":"41294","inReplyTo":"xmqqegcwt32j.fsf@gitster.mtv.corp.google.com","subject":"Re: git log -g bizarre behaviour","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2016-02-02T08:28:58Z","receivedAt":"2016-02-02T08:28:58Z","isPatch":false,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On ma, 2016-02-01 at 15:37 -0800, Junio C Hamano wrote:\n> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n> \n> > I'm attempting to understand the log [-g] / reflog code enough to\n> > untangle them and make reflog walking work for more than just\n> > commit\n> > objects [see gmane 283169]. I found something which I think is\n> > wrong,\n> > and would break after my changes.\n> > \n> > git log -g HEAD^ and git log -g v2.7.0^ give no output. This is\n> > expected, as those are not things that have a reflog.\n> \n> OK.\n> \n> > But git log -g v2.7.0 seems to ignore -g and gives the normal\n> > log.\n> \n> That sounds clearly broken, and I think I see how that happens from\n> the hacky way the \"-g\" traversal was bolted onto the revision\n> traversal machinery.\n> \n> I _think_ \"git log -g\" (and by extension \"git reflog\" which is just\n> a short-hand to giving a few more options to that command) ought to\n> \n>  * Iterate over the _objects_ that used to be at the tip of the ref;\n>  * Show each of these objects as if they were fed to \"git show\".\n\nThat's what I am trying to achieve. Though not quite like 'git show', I\nwant to emulate the --oneline putput for non-commit objects too.\n\n> This clearly is not possible without major surgery, including\n> ripping out the hacky \"-g\" traversal from the revision traversal\n> machinery and perhaps lifting it up a few levels in the callchain,\n> as many functions in that callchain want to work on commits.\n\nYup. I'm planning to either split cmd_log_walk or make its behaviour\ndepend on whether we're traversing the reflog (don't call get_revision,\nbut call a new get_reflog_entry function). And then rip out the reflog\nhandling from revision.c and redo (parts of) reflog-walk.c to\naccomodate the cmd_log_walk (split|replacement) that deals with reflogs\nbetter.\n\n> Contrast these two:\n> \n>     $ git log -1 v2.7.0\n>     $ git show v2.7.0\n> \n> > I'd like to make git log -g / git reflog abort early when trying to\n> > display a reflog of a ref that has no reflog. Objections?\n> \n> Do you mean\n> \n> \t$ git checkout -b testing\n>         $ rm -f .git/logs/refs/heads/testing\n>         $ git log -g testing\n> \n> will be changed from a silent no-op to an abort with error?\n> \n> I do not see a need for such a change--does that count as an\n> objection?\n\nNo, I'd like to change:\n\n$ ls .git/logs/refs/tags/v2.7.0\nls: cannot access .git/logs/refs/tags/v2.7.0: No such file or directory\n$ git (log -g|reflog) v2.7.0\n\n>From the bizarre behaviour above to a silent noop. But before I do that\nin a rewrite (by simply not implementing it), I'd like to have that\nbehavior now as well and add tests for it.\n\n-- \nDennis Kaarsemaker\nhttp://www.kaarsemaker.net\n"},{"id":"277269","messageId":"xmqqsi1asyai.fsf@gitster.mtv.corp.google.com","threadId":"41294","inReplyTo":"1454401738.32711.7.camel@kaarsemaker.net","subject":"Re: git log -g bizarre behaviour","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-02T19:32:53Z","receivedAt":"2016-02-02T19:32:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n\n> On ma, 2016-02-01 at 15:37 -0800, Junio C Hamano wrote:\n>\n>> Do you mean\n>> \n>> \t$ git checkout -b testing\n>>         $ rm -f .git/logs/refs/heads/testing\n>>         $ git log -g testing\n>> \n>> will be changed from a silent no-op to an abort with error?\n>> \n>> I do not see a need for such a change--does that count as an\n>> objection?\n>\n> No, I'd like to change:\n>\n> $ ls .git/logs/refs/tags/v2.7.0\n> ls: cannot access .git/logs/refs/tags/v2.7.0: No such file or directory\n> $ git (log -g|reflog) v2.7.0\n> From the bizarre behaviour above to a silent noop.\n\nWhen there is nothing to show, we do not show anything, and that is\njust like \"git log v2.7.0..v2.7.0\" is silent.\n\nI do not find the silence bizarre at all.\n"},{"id":"277272","messageId":"1454444532.2713.1.camel@kaarsemaker.net","threadId":"41294","inReplyTo":"xmqqsi1asyai.fsf@gitster.mtv.corp.google.com","subject":"Re: git log -g bizarre behaviour","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2016-02-02T20:22:12Z","receivedAt":"2016-02-02T20:22:12Z","isPatch":false,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On di, 2016-02-02 at 11:32 -0800, Junio C Hamano wrote:\n> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n> \n> > On ma, 2016-02-01 at 15:37 -0800, Junio C Hamano wrote:\n> > \n> > > Do you mean\n> > > \n> > > \t$ git checkout -b testing\n> > >         $ rm -f .git/logs/refs/heads/testing\n> > >         $ git log -g testing\n> > > \n> > > will be changed from a silent no-op to an abort with error?\n> > > \n> > > I do not see a need for such a change--does that count as an\n> > > objection?\n> > \n> > No, I'd like to change:\n> > \n> > $ ls .git/logs/refs/tags/v2.7.0\n> > ls: cannot access .git/logs/refs/tags/v2.7.0: No such file or\n> > directory\n> > $ git (log -g|reflog) v2.7.0\n> > From the bizarre behaviour above to a silent noop.\n> \n> When there is nothing to show, we do not show anything, \n\nAs I demonstrated in the text that you cut: that is not true.\ngit log -g v2.7.0 and git reflog v2.7.0 are *not* silent, but buggy. I\nwould like to make them silent.\n\n> and that is just like \"git log v2.7.0..v2.7.0\" is silent.\n> \n> I do not find the silence bizarre at all.\n\nI'll take that as an agreement then :)\n-- \nDennis Kaarsemaker\nwww.kaarsemaker.net\n"},{"id":"277274","messageId":"xmqqa8nisv2r.fsf@gitster.mtv.corp.google.com","threadId":"41294","inReplyTo":"1454444532.2713.1.camel@kaarsemaker.net","subject":"Re: git log -g bizarre behaviour","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-02T20:42:20Z","receivedAt":"2016-02-02T20:42:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n\n>> > $ git (log -g|reflog) v2.7.0\n>> > From the bizarre behaviour above to a silent noop.\n>\n> As I demonstrated in the text that you cut: that is not true.\n> git log -g v2.7.0 and git reflog v2.7.0 are *not* silent, but buggy. I\n> would like to make them silent.\n\nAh, sorry, I totally misread your \"bizarre\".  Yes, \"log -g\" that\nwalks the history not the reflog is \"bizarre\" and wrong, which we\nalready agreed in the previous exchange.  A fixed behaviour that\nwalks only the reflog entries should become a \"silent noop\".\n"},{"id":"277284","messageId":"1454455961-10640-1-git-send-email-dennis@kaarsemaker.net","threadId":"41294","inReplyTo":"1454241144.2822.7.camel@kaarsemaker.net","subject":"[PATCH] log -g: ignore revision parameters that have no reflog","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2016-02-02T23:32:41Z","receivedAt":"2016-02-02T23:32:41Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"git log -g (and by extension, git reflog) gets mightly confused when\ntrying to display the reflog of something that is not a ref that has a\nreflog. We can help by teaching handle_revision_arg to check all\nrevision arguments for reflog existence if it's in reflog mode.\n\ngit log -g something-that-is-not-a ref makes no sense, so let's die when\nthe user is trying that. git log -g ref-that-has-no-reflog is perfectly\nsensible, so we just ignore it.\n\nSigned-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>\n---\n revision.c             | 12 ++++++++++++\n t/t1411-reflog-show.sh | 10 ++++++++++\n 2 files changed, 22 insertions(+)\n\ndiff --git a/revision.c b/revision.c\nindex 0a282f5..43182c6 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1498,6 +1498,18 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \n \tflags = flags & UNINTERESTING ? flags | BOTTOM : flags & ~BOTTOM;\n \n+\tif (revs->reflog_info) {\n+\t\t/*\n+\t\t * The reflog iterator gets confused when fed things that don't\n+\t\t * have reflogs. Help it along a bit\n+\t\t */\n+\t\tif (strchr(arg, '@') != arg &&\n+\t\t    !dwim_ref(arg, strchrnul(arg, '@')-arg, sha1, &dotdot))\n+\t\t\tdie(\"only refs can have reflogs\");\n+\t\tif(!reflog_exists(dotdot))\n+\t\t\treturn 0;\n+\t}\n+\n \tdotdot = strstr(arg, \"..\");\n \tif (dotdot) {\n \t\tunsigned char from_sha1[20];\ndiff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh\nindex 6ac7734..e55518f 100755\n--- a/t/t1411-reflog-show.sh\n+++ b/t/t1411-reflog-show.sh\n@@ -171,4 +171,14 @@ test_expect_success 'reflog exists works' '\n \t! git reflog exists refs/heads/nonexistent\n '\n \n+test_expect_success 'reflog against non-ref dies' '\n+\ttest_must_fail git reflog HEAD^\n+'\n+\n+test_expect_success 'reflog against ref with no log is empty' '\n+\tgit tag nolog &&\n+\tgit reflog nolog > actual &&\n+\ttest_line_count = 0 actual\n+'\n+\n test_done\n-- \n2.7.0-345-gadc6f59\n"},{"id":"277289","messageId":"xmqqegcuprrw.fsf@gitster.mtv.corp.google.com","threadId":"41294","inReplyTo":"1454455961-10640-1-git-send-email-dennis@kaarsemaker.net","subject":"Re: [PATCH] log -g: ignore revision parameters that have no reflog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-03T00:21:55Z","receivedAt":"2016-02-03T00:21:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n\n> +\tif (revs->reflog_info) {\n> +\t\t/*\n> +\t\t * The reflog iterator gets confused when fed things that don't\n> +\t\t * have reflogs. Help it along a bit\n> +\t\t */\n> +\t\tif (strchr(arg, '@') != arg &&\n\nIs this merely an expensive way to write *arg != '@', or is there\nsomething else I am missing?\n\n> +\t\t    !dwim_ref(arg, strchrnul(arg, '@')-arg, sha1, &dotdot))\n> +\t\t\tdie(\"only refs can have reflogs\");\n\nIs \"foo@23\" a forbidden branch name?\n\nIs this looking for a dotdot?  If you are introducing a new scope,\nyou can afford to invent a variable with a name that reflects its\npurpose.\n\nStyle: a binary operation like '-' (subtract) have SP on both sides\nof it.\n\n> +\t\tif(!reflog_exists(dotdot))\n\nStyle: one SP between a syntactic keyword like 'if' and opening\nparenthesis is required.\n\nI have a suspicion that in your final \"fixed\" code, it may be a\nbetter design not to let the command line argument for \"-g\"\nprocessing pass through this function at all.\n\nFor example, what should \"git log -g master next\" do?  Merge two\nreflog entries in chronological order and show each of them as if\nthey are thrown at \"git show\" one by one?  Does that mesh well with\nother options like \"--date-order/--topo-order\"?\n\nFor another example, what should \"git log -g master..next\" do?\n\nOr \"git log -g master^^^\"?\n\nThese are merely a few example inputs I can think of off in 5\nseconds and I think none of the above makes much sense, but parsing\nthese is the primary purpose of this function.\n\nSo, I dunno.  I gave a few \"coding\" comments, but I am not sure if\nyou are touching the right codepath in the first place.\n"},{"id":"277335","messageId":"1454502958.2713.13.camel@kaarsemaker.net","threadId":"41294","inReplyTo":"xmqqegcuprrw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] log -g: ignore revision parameters that have no reflog","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2016-02-03T12:35:58Z","receivedAt":"2016-02-03T12:35:58Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On di, 2016-02-02 at 16:21 -0800, Junio C Hamano wrote:\n> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n> \n> > +\tif (revs->reflog_info) {\n> > +\t\t/*\n> > +\t\t * The reflog iterator gets confused when fed\n> > things that don't\n> > +\t\t * have reflogs. Help it along a bit\n> > +\t\t */\n> > +\t\tif (strchr(arg, '@') != arg &&\n> \n> Is this merely an expensive way to write *arg != '@', or is there\n> something else I am missing?\n\nDoh. No, that's just my stupidity. I did the strchrnul bits below\nfirst, then found out that it broke `git log -g @{0}` and came up with\nthe above.\n\n> > +\t\t    !dwim_ref(arg, strchrnul(arg, '@')-arg, sha1,\n> > &dotdot))\n> > +\t\t\tdie(\"only refs can have reflogs\");\n> \n> Is \"foo@23\" a forbidden branch name?\n\nIt is not, the code should look for @{, not @.\n\n> Is this looking for a dotdot?  If you are introducing a new scope,\n> you can afford to invent a variable with a name that reflects its\n> purpose.\n\nTrue. I just adhered to surrounding style (the dotdot variable is\nabused below as well). Lame excuse, I know :)\n\n> Style: a binary operation like '-' (subtract) have SP on both sides\n> of it.\n> \n> > +\t\tif(!reflog_exists(dotdot))\n> \n> Style: one SP between a syntactic keyword like 'if' and opening\n> parenthesis is required.\n\nAck.\n\n> I have a suspicion that in your final \"fixed\" code, it may be a\n> better design not to let the command line argument for \"-g\"\n> processing pass through this function at all.\n>\n> For example, what should \"git log -g master next\" do?  Merge two\n> reflog entries in chronological order and show each of them as if\n> they are thrown at \"git show\" one by one?  Does that mesh well with\n> other options like \"--date-order/--topo-order\"?\n\nI agree that option parsing is not the right place in the end. When -g\nis given, only one ref argument should be accepted, and --date-order\netc. should cause it to barf as they don't make sense.\n\n> For another example, what should \"git log -g master..next\" do?\n> \n> Or \"git log -g master^^^\"?\n>\n> These are merely a few example inputs I can think of off in 5\n> seconds and I think none of the above makes much sense, but parsing\n> these is the primary purpose of this function.\n\nWith this patch they die with an error as they make no sense.\n\n> So, I dunno.  I gave a few \"coding\" comments, but I am not sure if\n> you are touching the right codepath in the first place.\n\nI was trying to go for a minimal change to fix a bug without\nintroducing regressions. It feels weird to do it in the option parsing\ncode, but I didn't want to make this behaviour fix wait for a rewrite\nof the log -g functionality, as I have no idea when I'll be able to\nfinish that. It already took me a few hours to come up with this, as I\nhad not touched the related code at all before :)\n\n-- \nDennis Kaarsemaker\nwww.kaarsemaker.net\n"},{"id":"277339","messageId":"xmqqd1sdodah.fsf@gitster.mtv.corp.google.com","threadId":"41294","inReplyTo":"1454502958.2713.13.camel@kaarsemaker.net","subject":"Re: [PATCH] log -g: ignore revision parameters that have no reflog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-03T18:32:22Z","receivedAt":"2016-02-03T18:32:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n\n> It is not, the code should look for @{, not @.\n\nNot exactly.\n\n    $ git show -s --format='%h %s' 'HEAD^{/@{3}}' --\n    55d5d5b combine-diff.c: fix performance problem when folding ...\n\nThe commit has a line with a string \"@@@\" on it and the regular\nexpression asked for 3 '@', which shows that scanning for \"@{\" is\nnot a good way forward--it merely opens another can of worms.\n\nHopefully by now you have realized that a band-aid to add an ad-hoc\ncode that second-guesses what the existing code does for real while\nparsing the command line is not a good way forward.\n\nPerhaps we may want to step back a bit.\n\nWhere is the book-keeping information used for \"-g\" processing\nhandled in the codechain?  Upon seeing \"-g\", the parser calls\ninit_reflog_walk() to make revs->reflog_info non-NULL.\n\nWhat are the codepaths that use this field?\n\nWe can see the function add_pending_object_with_path() refers to\nthis revs->reflog_info field, when it calls add_reflog_for_walk(),\nwith the \"name\" it receives, after some mangling.\n\nThe called function, add_reflog_for_walk(), finds, from an object\nand its name, the ref whose log is going to be enumerated.  It looks\nat the name, optionally finds '@' in it, and eventually calls the\nfunction read_complete_reflog() [*1*].\n\nWe can infer that, by the time it does so, it must have figured out\nof which ref it wants to read the reflog.\n\nAnd it already has calls to die() and a few \"return -1\" to signal a\nnon-fatal error to the caller.  Perhaps instead of letting it to\npunt and resort to the normal history walking, the code should\nrealize that some refs do not have reflog (and no non-refs has\nreflog) and diagnose it as an error and die()?\n\nPerhaps one of these two functions is a much better place to do your\nimprovement?  The caller, add_pending_object_with_path(), does some\nmangling of \"name\" that is given by its caller before calling into\nthe callee, add_reflog_for_walk().  It could be that name mangling\nthat is leading to a wrong result.  More likely, the way the callee\nfigures out which ref it needs to read the reflog for given the\n\"name\" may be what you need to fix.\n\nUNLESS we are losing the information we got directly from the user\nat the command line before the control is passed down through the\ncallchain to reach these two functions, that is.  In such a case,\nI'd agree that we would need additional checks much closer to the\ninput.\n\nBut I think the callers of this callchain are passing what the user\ngave us pretty much verbatim down this callchain, so I would expect\nthat the leaf callee in this discussion, add_reflog_for_walk(),\nwould have enough information.\n\n\n[Footnote]\n\n*1* Yuck.\n"}]}