Re: [PATCH] Make 'diff C^!' show the same diff as 'show C'

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 20, 2009, 23:31 UTC
Message-ID: <7vocqanjua.fsf@alter.siamese.dyndns.org>
In-Reply-To: <86d1201d8adf53c1f48c0f3526d8e81475b18244.1250806019.git.trast@student.ethz.ch>

Thomas Rast <trast@student.ethz.ch> writes:

Show 9 quoted lines

> * If C has no parents, setup_revision() turns C^! into simply C.  This
>   meant that 'git diff C^!' compared the current worktree to C, which
>   is certainly not what the user asked for.
>
> * Otherwise setup_revision puts C itself last, i.e., the rev.pending
>   are ^C^1 ... ^C^N C.  So the first revision is uninteresting and in
>   the case of exactly two parents, the symmetric difference revspec
>   (diff A...B) case fired, and compared C only to C^1 (instead of
>   showing a combined diff).

I actually have a vague recollection that this ugly syntax C^! (and I am entitled to call it ugly as it was my invention) was advertised as a way to get "diff C^..C", not the combined diff, iow, this could be deliberate and people may depend on it.

In that light, I would say the first one (showing the root commit as root) may be a good change, but the latter one is moderately iffy.

Show 15 quoted lines

> @@ -292,6 +310,17 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>  	/* Otherwise, we are doing the usual "git" diff */
>  	rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;
>  
> +	for (i = 1; i < argc; i++) {
> +		if (prefixcmp(argv[i], "--")) {
> +			if (strstr(argv[i], "..."))
> +				mode = DIFF_MODE_SYMMETRIC;
> +			else if (strstr(argv[i], "^!"))
> +				mode = DIFF_MODE_SHOW;
> +		} else if (!strcmp(argv[i], "--")) {
> +			break;
> +		}
> +	}
> +

This is too ugly beyond words.

We already mark the left side commit "..." with SYMMETRIC_LEFT bit, so you should be able to detect it from the setup_revisions() result. If we were to formerly add some special meaning (other than being a short-hand of ^C^n C) to the ugly C^! syntax, I would suggest marking the result of in a similar way to allow you to detect it from the result.

But I do mean moderately strong negativeness when I say "if we _were_" above.

As far as I recall, there were only two reasons C^! was invented for (actually, one that was invented for, and another that was found to be useful).

One was so that you can say "The traversal should stop around this commit, but I want the commit itself to be included in the result".

	$ git log v1.6.3^! v1.6.3.1

This is much less useful than it used to be back when C^! was invented, as we can ask for --boundary these days.

The other utility later found was "to view diff with its first parent". I think it is a useful mode of operation, and we should add a simpler way to ask for it to "git show", as people often ask to view a merge not in the combined way, but a simple diff relative to the merged-to commit, to get an overview of the work done by the side branch.

So if anything, I'm actually for deprecating C^! syntax and removing it before we hit 1.7.0.