Re: [PATCH 4/4] builtin/show: do not prune by pathspec

[Michael J Gruber <git@drmicha.warpmail.net>]

Junio C Hamano venit, vidit, dixit 04.04.2011 23:49:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> diff --git a/builtin/log.c b/builtin/log.c
>> index 916019c..474a76d 100644
>> --- a/builtin/log.c
>> +++ b/builtin/log.c
>> @@ -420,6 +420,8 @@ int cmd_show(int argc, const char **argv, const char *prefix)
>>  	opt.def = "HEAD";
>>  	opt.tweak = show_rev_tweak_rev;
>>  	cmd_log_init(argc, argv, prefix, &rev, &opt);
>> +	if (rev.no_walk)
>> +		rev.prune = 0;
> 
> This is not your fault, but I am somewhat disgusted by the reason why
> 
>     $ git show master..next [ -- Documentation ]
> 
> works by "walking" the history.  It takes completely different codepath
> from "git log" with the same set of arguments.
> 
>  * first log_init() grabs '^master' and 'next' into the rev.pending object
>    array;
> 
>  * we pop '^master' first, make it the only object in the rev.pending
>    object array, and let cmd_log_walk() call prepare_revision_walk() on it
>    to limit the list.  Since we don't have any positive ref, we get
>    nothing;
> 
>  * then we pop 'next', make it again _the only_ object in the rev.pending
>    object array; prepare_revision_walk() on the same codepath now limits
>    the list to exclude what is reachable from 'master', only because we
>    have processed '^master'.
> 
> Yikes.  In other words, the reason the current code works is only by
> accident.

That is super ugly, yes.

> 
> This is a tangent, but we _might_ want to (perhaps at git 2.0) update the
> revision parsing machinery so that we can clear the object flags more
> easily and have
> 
>     $ git log A..B C..D [ -- pathspec ]
> 
> compute the union of (commits reachable from B but not from A) and
> (commits reachable from D but not from C).  I say we _might_ because we
> would still want to compute what the current code computes in some cases,
> and we may be able to express it as "^A ^C B D", but I am not sure if we
> want to go that route and end up with a general set operation (with unions
> and intersections, perhaps even using parentheses to express precedence).
> 
> More general set operation is certainly doable, and at that point it
> probably does not matter that we cannot stream the output as we are doing
> something complex (IOW, we would be revs->limited, giving up the latency),
> but I don't know if it is useful in the first place to support such
> general case.  Because the most complex set operations that I run every
> day is
> 
>     $ git log ^master $topic1 $topic2 $topic3... -- $pathspec
> 
> and I don't recall cases in which I wished A..B C..D showed commits
> within A..B that are reachable from C.
> 

We seem to promise that one should think in terms of sets, and this
breaks with "A..B" and "C...D" unless one always thinks of them in the
resolved form "^A B" and "^E C D" (with E=$(git merge-base C D)).

I also think that "A...B C" treats C as the right of that symmetric
range (because anything non-left we treat as right), and "A...B ^C"
probably interacts badly with --cherry-pick (though I haven't tried).

Also, "A...B C...D" would be a candidate for union. Lot's to do for
after 1.7.5...

Michael