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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 4, 2011, 21:49 UTC
Message-ID: <7v1v1h8z5k.fsf@alter.siamese.dyndns.org>
In-Reply-To: <6509d23c538dfabd4fac8870d29a69b1dd11a516.1301649372.git.git@drmicha.warpmail.net>

Michael J Gruber <git@drmicha.warpmail.net> writes:

Show 10 quoted lines

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

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.