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

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

 



Thomas Rast <trast@xxxxxxxxxxxxxxx> writes:

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

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

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