Re: [PATCH v3 05/13] parse the -L options

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thomas Rast <trast@xxxxxxxxxxxxxxx> writes:

>> I would consider it similar to "blame --incremental"; you can choose a
>> very different output format driven by a single switch to change the mode
>> of operation.
>
> For me (and Bo seemed to agree off-list) the main interest is in the
> *history*.  Yes, it is an important feature that the diffs are also
> limited to the ranges you are scanning, but that is the secondary
> feature.
>
> [If you think that is unfair, consider:...

We are not saying very different things, I think.

I guess I shouldn't have mentined "blame --incremental"; I was referring
to a quick-hack experiment we did to show an early output when we pass
blame to parent.  You can imagine that inside pass_blame_to_parent() we
can detect if the lines around the range we are passing the blame to
parent were touched, we could show pretty-printed commit log and relevant
diff-tree output at that point, which would give you pretty much the same
history we are talking about.

> If pathspec limiting wasn't broken in the sense that it does not
> support --follow (properly, anyway), they would be exactly equivalent...

Well, --follow is from "content-id" camp, while pathspec is a revision
limiter that follows quite different philosophy, so they do not mix very
well.  "log --follow -- $path" is an unfortunate cross between these two
philosophies and spelled that way only because that was the easiest way to
cheat in the command line parser.

As you alluded to, "log --follow HEAD -- $path" would mean exactly the
same as "log -L<range>,HEAD:$path HEAD" in the mock syntax I gave you
earlier in the thread, but that does not change the fact that it is
conceptually very different from the normal "git log".

>> E.g. if you have a history of this shape where commit C is at the tip:
>> 
>>     ---o---A---x---B---o---C
>> 
>> you _could_ ask "log -p -L1,10,B:Makefile A..C" (I am not proposing this
>> syntax at all, by the way) to browse the history between A and C, looking
>> for commits that touch the region of Makefile that appeared as lines 1..10
>> in revision B.  Between B and C, some new lines might have been introduced
>> inside the range and you would dig in reverse enlarging the range to show
>> what have been added to it.
>
> [I think this would be entirely possible to do for the line-log case,

I am sure it is "entirely possible", even though it might be quite
expensive (both development wise and also the end result).

A funny thing is, I do not think it is so "hard to explain and rarely
used" case.  Imagine that you have this history:

                 y---z---D maint
                /
    ---o---A---x---B---o---C master

Wouldn't it be nice if you can ask this question?

    I have these lines in frotz.c at 'master' version, and I recently made
    fixes that I need to backport to 'maint'.  How does the corresponding
    region look like in 'maint', and what changes that may be necessary
    for and interfere with my backport are there since these two branches
    forked?

In the ugly mock syntax, you would write it like this:

    log -p -L<range>,master:frotz.c master...maint

In this example, you have two positive revisions (notice three-dots for
symmetric difference), but you can easily imagine that this will work well
when you have more than two branches you are interested in.

To produce a useful output, you would need to dig from C down to x while
listing the commit that touch the specified range and adjusting it, then
dig the other way from x through y, z and D, again mapping the corresponding
range.

I am not saying that what -L does is a bad thing, though.  All I am saying
is that it is conceptually very different, and trying to coax it into
existing command line parsing rules of "git log" might not work well.

You could use a few rules to allow something like this which might be more
friendly to the eyes that are used to looking at the input to
setup_revisions() parser:

    log -p -L<range1> <path1> -L<range2> <revs>...

The short-hand rules may look something like this:

    - -L<range> with missing <path> is the same as -L<range>,<path> for
       the last <path> we have seen used with -L (it would be cleaner to
       use a single string given to -L i.e. "-L<range>,<path>" instead of
       a rule "-L takes one or two parameters" that is totally odd-ball
       from the point of view of normal POSIX command line rules, though);

    - If there is no <path> anywhere in the first use of -L, we take the
      first pathspec element as if it were <path>.

    - -L<range>,<path> is the same as -L<range>,<rev>:<path> where <rev>
       is the first positive revision given on the command line.

No matter what syntax sugarcoating is done, conceptually when -L says
"these lines in this path", it has to talk about the range in one
revision.  -L<range>,<rev>:<path> instructs us: "you are to follow this
block of lines in that path in this particular revision---figure out how
that migrated while you traverse the history and show its evolution".
There is no room for the normal pathspec rules to take effect---as we are
operating in the "content-id" domain, we would ideally want to
automatically even do --find-copies-harder, just like --follow and blame
automatically runs rename detection (that is another reason I think the
traversal semantics is very different from the regular "log", in addition
to the need to traverse forward in addition to backward which I already
mentioned).  We may want to error out when we have any <pathspec> (after
taking the second short-hand rule above into account, that is).

In short, I don't really have a very deep preference between "log -L" and
"blame -L".

But as a very special case of allowing only one single positive rev and no
forward-walking of the history, what "log -L" gives us is much closer to
"blame -L".  That is where my suggestion came from.  If we were to
eventually have that "log -p -L<range>,master:frotz.c master...maint", not
the syntax but the capability, I am all for it to be part of the "log"
command.

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


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]