On di, 2016-02-02 at 16:21 -0800, Junio C Hamano wrote:
> Dennis Kaarsemaker <dennis@xxxxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html