[PATCH] diff: remove dead code that flips arguments order

[PATCH] diff: remove dead code that flips arguments order

[Junio C Hamano]

In the olden days, "git cmd A..B" used to push ^A and then B into the list
of parsed revisions. The output from "git rev-list A..B" still does this,
and shows "B" first and then "^A" for backward compatibility.

The command line parser for "git diff A..B" was written originally to take
this swappage into account; when it internally sees an uninteresting
commit as its second input argument, it swapped the arguments to make it
the origin of the comparison, and the other one the destination.

These days, there is no need for this swappage, as the internal machinery
feeds ^A and then B to builtin_diff_tree().  Remove the dead code.

The funny thing is, I think this was a dead code from day one when it
was first written 6505602 (built-in diff., 2006-04-28); the author of
that patch must have known the implementation of the scripted version
that needed to deal with the swapped output from rev-parse too deeply
and didn't realize that the gotcha the dead code attempts to protect
him from didn't even apply to this codepath.

This change _will_ regress if somebody is insane enough to manually call
"git diff B ^A", but that is simply too insane for me to care.

There is exactly the same tree swapping code in diff-tree; that is not to
be changed not to break "git diff-tree B ^A" issued by Porcelain scripts,
but reword the comment to explain why we swap trees a bit better.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 builtin/diff-tree.c |    4 ++--
 builtin/diff.c      |   13 +------------
 2 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 0d2a3e9..c843546 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -133,8 +133,8 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 	}
 
 	/*
-	 * NOTE! We expect "a ^b" to be equal to "a..b", so we
-	 * reverse the order of the objects if the second one
+	 * NOTE! We expect "b ^a" to have come from "rev-parse a..b",
+	 * so we reverse the order of the objects if the second one
 	 * is marked UNINTERESTING.
 	 */
 	nr_sha1 = opt->pending.nr;
diff --git a/builtin/diff.c b/builtin/diff.c
index 4c9deb2..86614d4 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -146,20 +146,9 @@ static int builtin_diff_tree(struct rev_info *revs,
 			     int argc, const char **argv,
 			     struct object_array_entry *ent)
 {
-	const unsigned char *(sha1[2]);
-	int swap = 0;
-
 	if (argc > 1)
 		usage(builtin_diff_usage);
-
-	/* We saw two trees, ent[0] and ent[1].
-	 * if ent[1] is uninteresting, they are swapped
-	 */
-	if (ent[1].item->flags & UNINTERESTING)
-		swap = 1;
-	sha1[swap] = ent[0].item->sha1;
-	sha1[1-swap] = ent[1].item->sha1;
-	diff_tree_sha1(sha1[0], sha1[1], "", &revs->diffopt);
+	diff_tree_sha1(ent[0].item->sha1, ent[1].item->sha1, "", &revs->diffopt);
 	log_tree_diff_flush(revs);
 	return 0;
 }

[PATCH] warn use of "git diff A..B"

[Junio C Hamano]

"git diff" (and "diff-tree") accepts a range notation "A..B" from the
command line to specify the two endpoints to be compared; the right way to
spell this would be "git diff A B".  This is merely a historical accident
that comes from the fact that "git log" family of commands and "git diff"
happens to share some code in their command line parsers.

It was fine for people who are used to seeing "git diff A..B" and silently
translating it to "git diff A B" in their head, but it made things hard
for new people.  It is easy to mistakenly think that "git diff A..B" has
some similarity with "git log A..B" (there isn't).

Indeed, "git log A..B" computes for the range "A..B" a set of commits that
are in B but not in A, and if one misunderstands "git diff" to somehow
magically work with ranges (it doesn't), it is more natural to expect that
"git diff A..B" might show a cumulative changes since B forked from A.

But that is not what the command gives (it gives the difference between
two endpoints and there is no history cosideration such as "B forked from
A").

New people can be trained not to say "git diff A..B" when they mean to
compare two endpoints with "git diff A B", and that would reduce the
confusion greatly.

Warn the use of "git diff A..B" syntax when "git diff A B" equally works
well, is shorter, and is much more clear what the command is comparing
(i.e. two endpoints).

The new code does not issue a warning against "git diff ..B" that is used
as a shorthand for "git diff HEAD B", and "git diff A.." that is used as a
shorthand for "git diff A HEAD", respectively.  These are shorter to type
and are often useful.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Cumulative effect can be had with "git diff $(git merge-base A B) B"
   and there is a short-hand for it "git diff A...B", but this change is
   about the two-dot notation, not three-dots.

 builtin/diff.c |   16 ++++++++++++++++
 revision.c     |    6 ++++--
 revision.h     |    1 +
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index 86614d4..6d8028b 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -142,12 +142,28 @@ static int builtin_diff_index(struct rev_info *revs,
 	return run_diff_index(revs, cached);
 }
 
+static void check_useless_use_of_range(struct object_array_entry *ent)
+{
+	if (!(ent[0].item->flags & UNINTERESTING) ||
+	    ent[1].item->flags & UNINTERESTING)
+		return; /* not a range made by "A..B" notation */
+
+	if ((ent[0].name == dotdot_default_HEAD) ||
+	    (ent[1].name == dotdot_default_HEAD))
+		return;	/* "A.." or "..B" */
+
+	warning("Do not write 'git diff A..B' but write 'git diff A B'");
+	warning("diff is about two endpoints!");
+}
+
 static int builtin_diff_tree(struct rev_info *revs,
 			     int argc, const char **argv,
 			     struct object_array_entry *ent)
 {
 	if (argc > 1)
 		usage(builtin_diff_usage);
+
+	check_useless_use_of_range(ent);
 	diff_tree_sha1(ent[0].item->sha1, ent[1].item->sha1, "", &revs->diffopt);
 	log_tree_diff_flush(revs);
 	return 0;
diff --git a/revision.c b/revision.c
index e96c281..48a0a44 100644
--- a/revision.c
+++ b/revision.c
@@ -1008,6 +1008,8 @@ static void prepare_show_merge(struct rev_info *revs)
 	revs->limited = 1;
 }
 
+const char dotdot_default_HEAD[] = "HEAD";
+
 int handle_revision_arg(const char *arg, struct rev_info *revs,
 			int flags,
 			int cant_be_filename)
@@ -1030,9 +1032,9 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,
 		next += symmetric;
 
 		if (!*next)
-			next = "HEAD";
+			next = dotdot_default_HEAD;
 		if (dotdot == arg)
-			this = "HEAD";
+			this = dotdot_default_HEAD;
 		if (!get_sha1(this, from_sha1) &&
 		    !get_sha1(next, sha1)) {
 			struct commit *a, *b;
diff --git a/revision.h b/revision.h
index ae94860..27a233c 100644
--- a/revision.h
+++ b/revision.h
@@ -151,6 +151,7 @@ struct rev_info {
 /* revision.c */
 typedef void (*show_early_output_fn_t)(struct rev_info *, struct commit_list *);
 extern volatile show_early_output_fn_t show_early_output;
+extern const char dotdot_default_HEAD[];
 
 struct setup_revision_opt {
 	const char *def;

Re: [PATCH] warn use of "git diff A..B"

[Jakub Narebski]

Junio C Hamano <gitster@pobox.com> writes:

> "git diff" (and "diff-tree") accepts a range notation "A..B" from the
> command line to specify the two endpoints to be compared; the right way to
> spell this would be "git diff A B".  This is merely a historical accident
> that comes from the fact that "git log" family of commands and "git diff"
> happens to share some code in their command line parsers.

I think it is quite useful to acept this notation to allow for
copy'n'paste from e.g. "git fetch" output:

     5e839c8..cd3065f  master     -> origin

On can simply paste "git diff 5e839c8..cd3065f".
                              ^^^^^^^^^^^^^^^^
-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH] warn use of "git diff A..B"

[Johannes Sixt]

Am 3/24/2011 16:44, schrieb Jakub Narebski:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> "git diff" (and "diff-tree") accepts a range notation "A..B" from the
>> command line to specify the two endpoints to be compared; the right way to
>> spell this would be "git diff A B".  This is merely a historical accident
>> that comes from the fact that "git log" family of commands and "git diff"
>> happens to share some code in their command line parsers.
> 
> I think it is quite useful to acept this notation to allow for
> copy'n'paste from e.g. "git fetch" output:
> 
>      5e839c8..cd3065f  master     -> origin
> 
> On can simply paste "git diff 5e839c8..cd3065f".
>                               ^^^^^^^^^^^^^^^^

Good point! Of the inconsistencies that still remain in git, IMO, this is
the most bearable one, because it's very easy to explain.

-- Hannes

Re: [PATCH] warn use of "git diff A..B"

[Jay Soffian]

On Wed, Mar 23, 2011 at 5:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> New people can be trained not to say "git diff A..B" when they mean to
> compare two endpoints with "git diff A B", and that would reduce the
> confusion greatly.

I think this is ill-advised because...

> Warn the use of "git diff A..B" syntax when "git diff A B" equally works
> well, is shorter, and is much more clear what the command is comparing
> (i.e. two endpoints).

... this is not consistent ...

> The new code does not issue a warning against "git diff ..B" that is used
> as a shorthand for "git diff HEAD B", and "git diff A.." that is used as a
> shorthand for "git diff A HEAD", respectively.  These are shorter to type
> and are often useful.

...with this.

I don't think telling newbies "Use diff A B instead of diff A..B, but
A.. and ..B are okay" reduces confusion any. i.e., this makes more
sense to me:

(1) One sided '..' implies HEAD on the other side with both git log and git diff
(2) "diff A..B" is the same as "diff A B"

Therefor:

- "diff A.." is the same as "diff A..HEAD" (1) is the same as "diff A HEAD" (2).
- "diff ..B" is the same as "diff HEAD..B" (1) is the same as "diff HEAD B" (2).

Also, the fact that a one-sided '..' implies HEAD on the other side


> +static void check_useless_use_of_range(struct object_array_entry *ent)
> +{
> +       if (!(ent[0].item->flags & UNINTERESTING) ||
> +           ent[1].item->flags & UNINTERESTING)
> +               return; /* not a range made by "A..B" notation */
> +
> +       if ((ent[0].name == dotdot_default_HEAD) ||
> +           (ent[1].name == dotdot_default_HEAD))
> +               return; /* "A.." or "..B" */
> +
> +       warning("Do not write 'git diff A..B' but write 'git diff A B'");
> +       warning("diff is about two endpoints!");
> +}
> +

I use diff A..B all the time, because I've often just used log A..B,
or I'm about to, and it's one less part of the command line to change.
Please make this warning squelch-able at least.

j.

Re: [PATCH] warn use of "git diff A..B"

[Jay Soffian]

On Thu, Mar 24, 2011 at 2:11 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
> Also, the fact that a one-sided '..' implies HEAD on the other side

Sorry, stray sentence. Ignore it.

But also, context matters. I don't get confused that Perl has a ".."
operator which can be both a range operator and a flip-flop operator.

Of all the things that git overloads (reset, checkout, ...), this
seems the least confusing to explain.

$0.03. :-)

j.

Re: [PATCH] warn use of "git diff A..B"

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> On Wed, Mar 23, 2011 at 5:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> ... this is not consistent ...
>
>> The new code does not issue a warning against "git diff ..B" that is used
>> as a shorthand for "git diff HEAD B", and "git diff A.." that is used as a
>> shorthand for "git diff A HEAD", respectively.  These are shorter to type
>> and are often useful.
>
> ...with this.

I already _know_ it is not consistent.  My verdict is that shoter-to-type
and being useful trumps that minor consistency argument and that is why
the patch is like so, and that is why the proposed commit log message
clearly describes the choice.

> I use diff A..B all the time, because I've often just used log A..B,
> or I'm about to, and it's one less part of the command line to change.

OK.  I hereby designate you as the official cluebat wearer responsible for
explaining things whenever new people get confused and say that the output
from "git diff A..B" does not match what they expect from "git log A..B",
(and "git diff A...B" vs "git log A...B") on this list.

Thanks ;-).

Re: [PATCH] warn use of "git diff A..B"

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 23, 2011 at 22:45, Junio C Hamano <gitster@pobox.com> wrote:

I agree with Jakub. I often write something like this:

    $ git log origin/master..origin/pu
    $ ^log^diff
    git diff origin/master..origin/pu

It would be annoying to have that start to warn.

> +       warning("Do not write 'git diff A..B' but write 'git diff A B'");
> +       warning("diff is about two endpoints!");

And in any case this warning is way too terse to give the user an idea
of what this is about.

Re: [PATCH] warn use of "git diff A..B"

[Alex Riesen]

On Wed, Mar 23, 2011 at 22:45, Junio C Hamano <gitster@pobox.com> wrote:
> "git diff" (and "diff-tree") accepts a range notation "A..B" from the
> command line to specify the two endpoints to be compared; the right way to
> spell this would be "git diff A B".  This is merely a historical accident
> that comes from the fact that "git log" family of commands and "git diff"
> happens to share some code in their command line parsers.
>
> It was fine for people who are used to seeing "git diff A..B" and silently
> translating it to "git diff A B" in their head, but it made things hard
> for new people.  It is easy to mistakenly think that "git diff A..B" has
> some similarity with "git log A..B" (there isn't).

I like the dot-dot notation a lot.

Re: [PATCH] warn use of "git diff A..B"

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> I like the dot-dot notation a lot.

Thanks to be the third volunteer.  I don't need any more volunteers ;-)