Deprecating git diff ..; dealing with other ranges

Deprecating git diff ..; dealing with other ranges

[Denton Liu]

Hello all,

I was in the process of deprecating `git diff <commit>..<commit>` as
discussed here[1]. However, I ran into a weird case that I'm not sure
how to deal with.

In t3430-rebase-merges.sh:382, we have the following test case which
invokes git diff:

	test_expect_success 'with --autosquash and --exec' '
		git checkout -b with-exec H &&
		echo Booh >B.t &&
		test_tick &&
		git commit --fixup B B.t &&
		write_script show.sh <<-\EOF &&
		subject="$(git show -s --format=%s HEAD)"
=>		content="$(git diff HEAD^! | tail -n 1)"
		echo "$subject: $content"
		EOF
		test_tick &&
		git rebase -ir --autosquash --exec ./show.sh A >actual &&
		grep "B: +Booh" actual &&
		grep "E: +Booh" actual &&
		grep "G: +G" actual
	'

It gets caught in my attempt to only deprecate ..'s. Technically, it's
undocumented behaviour and it only happens to work because git-diff
accept ranges but it doesn't operate in an intuitive way.

I was just wondering what we should do about this case? Should we
deprecate all invocations of `git diff <range>` except for the special
case of `git diff <commit>...<commit>`, or should we _only_ deprecate
`git diff <commit>..<commit>` and allow all other forms of ranges, even
though it was undocumented behaviour?

Thanks,

Denton

[1]: https://public-inbox.org/git/xmqqmumy6mxe.fsf@gitster-ct.c.googlers.com/

Re: Deprecating git diff ..; dealing with other ranges

[Johannes Schindelin]

Hi Denton,

On Mon, 11 Mar 2019, Denton Liu wrote:

> I was in the process of deprecating `git diff <commit>..<commit>` as
> discussed here[1]. However, I ran into a weird case that I'm not sure
> how to deal with.
> 
> In t3430-rebase-merges.sh:382, we have the following test case which
> invokes git diff:
> 
> 	test_expect_success 'with --autosquash and --exec' '
> 		git checkout -b with-exec H &&
> 		echo Booh >B.t &&
> 		test_tick &&
> 		git commit --fixup B B.t &&
> 		write_script show.sh <<-\EOF &&
> 		subject="$(git show -s --format=%s HEAD)"
> =>		content="$(git diff HEAD^! | tail -n 1)"
> 		echo "$subject: $content"
> 		EOF
> 		test_tick &&
> 		git rebase -ir --autosquash --exec ./show.sh A >actual &&
> 		grep "B: +Booh" actual &&
> 		grep "E: +Booh" actual &&
> 		grep "G: +G" actual
> 	'
> 
> It gets caught in my attempt to only deprecate ..'s. Technically, it's
> undocumented behaviour and it only happens to work because git-diff
> accept ranges but it doesn't operate in an intuitive way.

I beg to differ. `git diff <commit>^!` does exactly what I want: it shows
the diff between the commit's first parent and the commit.

And I would not necessarily call this a "range". It is a short-hand. Can't
you allow short-hands in `git diff`, and disallow only ranges that have an
explicit `..` in them? You might need to record that somewhere, but I
think that should be easy enough.

Ciao,
Johannes

> 
> I was just wondering what we should do about this case? Should we
> deprecate all invocations of `git diff <range>` except for the special
> case of `git diff <commit>...<commit>`, or should we _only_ deprecate
> `git diff <commit>..<commit>` and allow all other forms of ranges, even
> though it was undocumented behaviour?
> 
> Thanks,
> 
> Denton
> 
> [1]: https://public-inbox.org/git/xmqqmumy6mxe.fsf@gitster-ct.c.googlers.com/
> 

Re: Deprecating git diff ..; dealing with other ranges

[Elijah Newren]

Hi,

On Mon, Mar 11, 2019 at 2:37 AM Denton Liu <liu.denton@gmail.com> wrote:
>
> Hello all,
>
> I was in the process of deprecating `git diff <commit>..<commit>` as
> discussed here[1]. However, I ran into a weird case that I'm not sure
> how to deal with.
>
> In t3430-rebase-merges.sh:382, we have the following test case which
> invokes git diff:
>
>         test_expect_success 'with --autosquash and --exec' '
>                 git checkout -b with-exec H &&
>                 echo Booh >B.t &&
>                 test_tick &&
>                 git commit --fixup B B.t &&
>                 write_script show.sh <<-\EOF &&
>                 subject="$(git show -s --format=%s HEAD)"
> =>              content="$(git diff HEAD^! | tail -n 1)"
>                 echo "$subject: $content"
>                 EOF
>                 test_tick &&
>                 git rebase -ir --autosquash --exec ./show.sh A >actual &&
>                 grep "B: +Booh" actual &&
>                 grep "E: +Booh" actual &&
>                 grep "G: +G" actual
>         '
>
> It gets caught in my attempt to only deprecate ..'s. Technically, it's
> undocumented behaviour and it only happens to work because git-diff
> accept ranges but it doesn't operate in an intuitive way.
>
> I was just wondering what we should do about this case? Should we
> deprecate all invocations of `git diff <range>` except for the special
> case of `git diff <commit>...<commit>`, or should we _only_ deprecate
> `git diff <commit>..<commit>` and allow all other forms of ranges, even
> though it was undocumented behaviour?

There's a few angles I can think of to view this from:

First, commit^! is somewhat of a degenerate "range"; it includes only
one commit and for non-merge commits, is equal to both
commit~1..commit and commit~1...commit.  The fact that those ranges
are equal means that "git diff commit~1..commit" and "git diff
commit~1...commit" will also give equal results, i.e. that this is not
a case where confusion between '..' and '...' will cause any problems
for the user.  (Admittedly, I'm ignoring usage of ^! with a merge
commit here; I've never seen anyone use it in such a case.)

Second, ^! is unlikely to cause confusion for users the way '..' vs
'...' will, because the syntax makes no sense with diff anyway.  It's
totally magical, and when I came across it being used with diff
instead of log, I had to test it out to determine what it did.  (I do
now find it handy and use it occasionally.)  It's fairly difficult to
explain to beginners -- I tried once and quickly gave up and used
longer but more straightforward alternatives.  So, IMO, this is only a
convenience syntax for experts, and a syntax that won't be confused
with other syntax out there, so there's no need to deprecate it.

Third, we have good reason to deprecate explicit usage of '..' with
git diff, but even with those reasons some folks probably aren't
convinced it's worth the effort.  I want to avoid expanding scope, for
fear of moving some people from the sidelines of "not worth the
effort" to "this is a bad idea".  So if there are other range syntax
cases used in the wild (maybe 'git diff merge_commit^@' ?!?), we
should just leave them be and allow them to continue working.


Hope that helps,
Elijah

Re: Deprecating git diff ..; dealing with other ranges

[Junio C Hamano]

Denton Liu <liu.denton@gmail.com> writes:

> I was in the process of deprecating `git diff <commit>..<commit>` as
> discussed here[1].
>
> [1]: https://public-inbox.org/git/xmqqmumy6mxe.fsf@gitster-ct.c.googlers.com/

I didn't (and don't) advocate such a deprecation, FWIW, in that
message.  I simply do not think it is worth the cost.

I however think it may be worth making sure that our docs do not
encourage "diff A..B" and teach "diff A B" when comparing two
endpoints.  That can be done without changing anything in the code.

Re: Deprecating git diff ..; dealing with other ranges

[Junio C Hamano]

Denton Liu <liu.denton@gmail.com> writes:

> =>		content="$(git diff HEAD^! | tail -n 1)"
> ...
> It gets caught in my attempt to only deprecate ..'s. Technically, it's
> undocumented behaviour and it only happens to work because git-diff
> accept ranges but it doesn't operate in an intuitive way.

It reuses the same notation as that is used for ranges, but makes
the notation mean something entirely different from ranges, because
"diff" is about two endpoints.

And "git diff ^A B" (or "git diff B ^A") works like "git diff A B".
So does "git diff A^!", for a single-parent commit A, work like "git
diff A^ A".

Re: Deprecating git diff ..; dealing with other ranges

[Andreas Schwab]

On Mär 12 2019, Junio C Hamano <gitster@pobox.com> wrote:

> I however think it may be worth making sure that our docs do not
> encourage "diff A..B" and teach "diff A B" when comparing two
> endpoints.  That can be done without changing anything in the code.

The nice thing about "diff A..B" is that you can c&p the output from the
fetch run without the need to edit it.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: Deprecating git diff ..; dealing with other ranges

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 12 2019, Andreas Schwab wrote:

> On Mär 12 2019, Junio C Hamano <gitster@pobox.com> wrote:
>
>> I however think it may be worth making sure that our docs do not
>> encourage "diff A..B" and teach "diff A B" when comparing two
>> endpoints.  That can be done without changing anything in the code.
>
> The nice thing about "diff A..B" is that you can c&p the output from the
> fetch run without the need to edit it.

Not to shoot down this effort, just to add another similar thing I do
regularly for ff-branches:

 1. Copy/paste A..B fetch output
 2. git log A..B
 3. ^log^diff

I.e. I just need to tell my terminal to re-run the same "log" command
with "diff" instead of "log".

Of course as covered in the linked thread it doesn't work for some
(non-ff) cases, and I'll sometimes end up cursing it and swapping around
".." for "..." with log/diff.

Re: Deprecating git diff ..; dealing with other ranges

[Duy Nguyen]

On Wed, Mar 13, 2019 at 12:26 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> On Mär 12 2019, Junio C Hamano <gitster@pobox.com> wrote:
>
> > I however think it may be worth making sure that our docs do not
> > encourage "diff A..B" and teach "diff A B" when comparing two
> > endpoints.  That can be done without changing anything in the code.
>
> The nice thing about "diff A..B" is that you can c&p the output from the
> fetch run without the need to edit it.

I do this copy&paste too though I think in this case we could improve
something to not copy&paste in the first place.

A..B from fetch is the same as branch@{1}..branch. If we have some
shortcut similar to ^! but for reflog, that would be perfect (the
branch part does not require much typing with tab completion).
-- 
Duy

Re: Deprecating git diff ..; dealing with other ranges

[Johannes Sixt]

Am 12.03.19 um 22:01 schrieb Ævar Arnfjörð Bjarmason:
> 
> On Tue, Mar 12 2019, Andreas Schwab wrote:
> 
>> On Mär 12 2019, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>> I however think it may be worth making sure that our docs do not
>>> encourage "diff A..B" and teach "diff A B" when comparing two
>>> endpoints.  That can be done without changing anything in the code.
>>
>> The nice thing about "diff A..B" is that you can c&p the output from the
>> fetch run without the need to edit it.
> 
> Not to shoot down this effort, just to add another similar thing I do
> regularly for ff-branches:
> 
>  1. Copy/paste A..B fetch output
>  2. git log A..B
>  3. ^log^diff
> 
> I.e. I just need to tell my terminal to re-run the same "log" command
> with "diff" instead of "log".
> 
> Of course as covered in the linked thread it doesn't work for some
> (non-ff) cases, and I'll sometimes end up cursing it and swapping around
> ".." for "..." with log/diff.

Still, your and Andreas's use-case is reason enough *not* to deprecate
diff A..B, IMO.

We wouldn't be able to remove .. wholesale anyway because diff A.. and
diff ..B, where the missing endpoints are filled in with HEAD would have
to live on.

-- Hannes

Re: Deprecating git diff ..; dealing with other ranges

[Andreas Schwab]

On Mär 13 2019, Duy Nguyen <pclouds@gmail.com> wrote:

> A..B from fetch is the same as branch@{1}..branch. If we have some
> shortcut similar to ^! but for reflog, that would be perfect (the
> branch part does not require much typing with tab completion).

Tab completion is much slower than c&p, especially if the completion is
ambigous.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: Deprecating git diff ..; dealing with other ranges

[Elijah Newren]

On Tue, Mar 12, 2019 at 2:01 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Tue, Mar 12 2019, Andreas Schwab wrote:
> > On Mär 12 2019, Junio C Hamano <gitster@pobox.com> wrote:
> >
> >> I however think it may be worth making sure that our docs do not
> >> encourage "diff A..B" and teach "diff A B" when comparing two
> >> endpoints.  That can be done without changing anything in the code.
> >
> > The nice thing about "diff A..B" is that you can c&p the output from the
> > fetch run without the need to edit it.
>
> Not to shoot down this effort, just to add another similar thing I do
> regularly for ff-branches:
>
>  1. Copy/paste A..B fetch output
>  2. git log A..B
>  3. ^log^diff
>
> I.e. I just need to tell my terminal to re-run the same "log" command
> with "diff" instead of "log".
>
> Of course as covered in the linked thread it doesn't work for some
> (non-ff) cases, and I'll sometimes end up cursing it and swapping around
> ".." for "..." with log/diff.

Doesn't this somewhat imply that although you use diff A..B here for
convenience, that it's actually wrong since what you really want is
A...B?  Or said another way, the end goal of deprecating "diff "A..B"
then later reinstating "diff A..B" to mean the same thing as "diff
A...B" would actually be better even for your usecase?

Of course, switching to the removal period may just be too painful for
too many folks since there are obviously people that use it, but I
just want to see if I'm understanding correctly here.

Re: Deprecating git diff ..; dealing with other ranges

[Michal Suchánek]

On Mon, 18 Mar 2019 10:07:08 -0700
Elijah Newren <newren@gmail.com> wrote:

> On Tue, Mar 12, 2019 at 2:01 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
> > On Tue, Mar 12 2019, Andreas Schwab wrote:  
> > > On Mär 12 2019, Junio C Hamano <gitster@pobox.com> wrote:
> > >  
> > >> I however think it may be worth making sure that our docs do not
> > >> encourage "diff A..B" and teach "diff A B" when comparing two
> > >> endpoints.  That can be done without changing anything in the code.  
> > >
> > > The nice thing about "diff A..B" is that you can c&p the output from the
> > > fetch run without the need to edit it.  
> >
> > Not to shoot down this effort, just to add another similar thing I do
> > regularly for ff-branches:
> >
> >  1. Copy/paste A..B fetch output
> >  2. git log A..B
> >  3. ^log^diff
> >
> > I.e. I just need to tell my terminal to re-run the same "log" command
> > with "diff" instead of "log".
> >
> > Of course as covered in the linked thread it doesn't work for some
> > (non-ff) cases, and I'll sometimes end up cursing it and swapping around
> > ".." for "..." with log/diff.  
> 
> Doesn't this somewhat imply that although you use diff A..B here for
> convenience, that it's actually wrong since what you really want is
> A...B?  Or said another way, the end goal of deprecating "diff "A..B"
> then later reinstating "diff A..B" to mean the same thing as "diff
> A...B" would actually be better even for your usecase?

I usually mean diff A..B in this case.

Thanks

Michal

Re: Deprecating git diff ..; dealing with other ranges

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 18 2019, Elijah Newren wrote:

> On Tue, Mar 12, 2019 at 2:01 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> On Tue, Mar 12 2019, Andreas Schwab wrote:
>> > On Mär 12 2019, Junio C Hamano <gitster@pobox.com> wrote:
>> >
>> >> I however think it may be worth making sure that our docs do not
>> >> encourage "diff A..B" and teach "diff A B" when comparing two
>> >> endpoints.  That can be done without changing anything in the code.
>> >
>> > The nice thing about "diff A..B" is that you can c&p the output from the
>> > fetch run without the need to edit it.
>>
>> Not to shoot down this effort, just to add another similar thing I do
>> regularly for ff-branches:
>>
>>  1. Copy/paste A..B fetch output
>>  2. git log A..B
>>  3. ^log^diff
>>
>> I.e. I just need to tell my terminal to re-run the same "log" command
>> with "diff" instead of "log".
>>
>> Of course as covered in the linked thread it doesn't work for some
>> (non-ff) cases, and I'll sometimes end up cursing it and swapping around
>> ".." for "..." with log/diff.
>
> Doesn't this somewhat imply that although you use diff A..B here for
> convenience, that it's actually wrong since what you really want is
> A...B?

Yeah "..." would be more correct, but usually when I do this it's for
things where the LHS hasn't diverged.

> Or said another way, the end goal of deprecating "diff "A..B"
> then later reinstating "diff A..B" to mean the same thing as "diff
> A...B" would actually be better even for your usecase?

Yeah, it would be. Sometimes I need to go back and tweak it to be "..."
instead of ".." now. When the two have different behavior I usually want
the former.

> Of course, switching to the removal period may just be too painful for
> too many folks since there are obviously people that use it, but I
> just want to see if I'm understanding correctly here.

I think we'd ideally migrate, but the conversion might be too
painful. Junio doesn't seem excited in
<xmqqmum0h88n.fsf@gitster-ct.c.googlers.com>

I'm not claiming that this ^log^diff use-case is common or anything,
just noting a use-case I do use occasionally and mostly works now.

Re: Deprecating git diff ..; dealing with other ranges

[Elijah Newren]

On Mon, Mar 18, 2019 at 10:11 AM Michal Suchánek <msuchanek@suse.de> wrote:
>
> On Mon, 18 Mar 2019 10:07:08 -0700
> Elijah Newren <newren@gmail.com> wrote:
>
> > On Tue, Mar 12, 2019 at 2:01 PM Ævar Arnfjörð Bjarmason
> > <avarab@gmail.com> wrote:
> > > On Tue, Mar 12 2019, Andreas Schwab wrote:
> > > > On Mär 12 2019, Junio C Hamano <gitster@pobox.com> wrote:
> > > >
> > > >> I however think it may be worth making sure that our docs do not
> > > >> encourage "diff A..B" and teach "diff A B" when comparing two
> > > >> endpoints.  That can be done without changing anything in the code.
> > > >
> > > > The nice thing about "diff A..B" is that you can c&p the output from the
> > > > fetch run without the need to edit it.
> > >
> > > Not to shoot down this effort, just to add another similar thing I do
> > > regularly for ff-branches:
> > >
> > >  1. Copy/paste A..B fetch output
> > >  2. git log A..B
> > >  3. ^log^diff
> > >
> > > I.e. I just need to tell my terminal to re-run the same "log" command
> > > with "diff" instead of "log".
> > >
> > > Of course as covered in the linked thread it doesn't work for some
> > > (non-ff) cases, and I'll sometimes end up cursing it and swapping around
> > > ".." for "..." with log/diff.
> >
> > Doesn't this somewhat imply that although you use diff A..B here for
> > convenience, that it's actually wrong since what you really want is
> > A...B?  Or said another way, the end goal of deprecating "diff "A..B"
> > then later reinstating "diff A..B" to mean the same thing as "diff
> > A...B" would actually be better even for your usecase?
>
> I usually mean diff A..B in this case.

Thanks for the feedback; somewhat odd that you and Ævar have the same
usecase but want the opposite meaning, but good to know nonetheless.