git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Denton Liu <liu.denton@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>,
	Philip Oakley <philipoakley@iee.org>,
	Viresh Kumar <viresh.kumar@linaro.org>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: Re: Deprecating git diff ..; dealing with other ranges
Date: Mon, 11 Mar 2019 08:34:34 -0700	[thread overview]
Message-ID: <CABPp-BEfNa88bc6rb7xWxJNOiGdTpc-9+=_1od52kDxOfK3XEA@mail.gmail.com> (raw)
In-Reply-To: <20190311093751.GA31092@archbookpro.localdomain>

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

  parent reply	other threads:[~2019-03-11 15:34 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-11  9:37 Deprecating git diff ..; dealing with other ranges Denton Liu
2019-03-11 13:19 ` Johannes Schindelin
2019-03-11 15:34 ` Elijah Newren [this message]
2019-03-12  7:17 ` Junio C Hamano
2019-03-12 17:24   ` Andreas Schwab
2019-03-12 21:01     ` Ævar Arnfjörð Bjarmason
2019-03-13  7:01       ` Johannes Sixt
2019-03-18 17:07       ` Elijah Newren
2019-03-18 17:11         ` Michal Suchánek
2019-03-18 18:51           ` Elijah Newren
2019-03-18 17:59         ` Ævar Arnfjörð Bjarmason
2019-03-13  1:20     ` Duy Nguyen
2019-03-13 18:12       ` Andreas Schwab
2019-03-12  7:22 ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CABPp-BEfNa88bc6rb7xWxJNOiGdTpc-9+=_1od52kDxOfK3XEA@mail.gmail.com' \
    --to=newren@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=liu.denton@gmail.com \
    --cc=philipoakley@iee.org \
    --cc=vincent.guittot@linaro.org \
    --cc=viresh.kumar@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).