All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jonathan Nieder <jrnieder@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Jeff King <peff@peff.net>, Duy Nguyen <pclouds@gmail.com>,
	Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH v2 0/3] rev-parse and "--"
Date: Mon, 9 Dec 2013 12:56:21 -0800	[thread overview]
Message-ID: <20131209205621.GW29959@google.com> (raw)
In-Reply-To: <xmqqob4pycmv.fsf@gitster.dls.corp.google.com>

Junio C Hamano wrote:

> I do not share the "with --verify is better hence no problem"
> reasoning.  My "not so much worth worrying about" comes solely from
> a hunch that nobody has "HEAD~3..HEAD" in their working directory,

That's what makes it a problem.  This change makes it very easy to
make a general-purpose script that breaks in an edge case that the
script's author is not likely to run into.  Then as soon as someone
adds a file with such a name to the test data in their repo, their
favorite general-purpose repo munger just breaks.

If we wanted to forbid git from tracking files named HEAD~3..HEAD
altogether, that would be a different story.

> and if somebody has one, then they must be using "--verify" (or a
> clarifying "--"), because their "git log" and whatever they use "git
> rev-parse HEAD~3..HEAD" for would behave very differently otherwise.

Isn't protecting against this kind of thing the reason we ask authors
of general-purpose scripts to use "simple, do what I say and not what
I mean" plumbing commands?

Another relevant detail is that using rev-parse with "--" is more
painful than without, since it includes the "--" in its output.
Without this change, it seems much more likely to me that someone
would do

	git rev-parse <commits> |
	while read commit
	do
		...
	done

than

	git rev-parse <commits> -- |
	while read commit
	do
		if test "$commit" = "--"
		then
			continue
		fi

		...
	done

> So it is not merely "--verify is better"---in a situation where the
> backward incompatibility matters, I doubt the existing behaves
> sensibly in the first place.

What in the former of the above two loops is broken?

> But if we cook it for a while, I suspect that we will find more and
> more breakages of expectations in the existing scripts in and out of
> the tree;

Alas, probably no, because nobody has "HEAD~3..HEAD" in their working
directory.  That's exactly the problem --- it creates an edge case
that nobody is likely to test until the once-in-a-few-years moment
when they need it.

Jonathan

  reply	other threads:[~2013-12-09 20:56 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-12-05 10:07 [BUG] redundant error message Duy Nguyen
2013-12-05 19:15 ` Jeff King
2013-12-05 20:00   ` Junio C Hamano
2013-12-05 20:03     ` Jeff King
2013-12-05 20:15       ` Junio C Hamano
2013-12-05 21:00         ` Jeff King
2013-12-05 21:28           ` Jeff King
2013-12-05 21:44             ` Junio C Hamano
2013-12-06 21:12               ` [PATCH 0/2] rev-parse and "--" Jeff King
2013-12-06 21:13                 ` [PATCH 1/2] rev-parse: correctly diagnose revision errors before "--" Jeff King
2013-12-06 21:15                 ` [PATCH 2/2] rev-parse: diagnose ambiguous revision/filename arguments Jeff King
2013-12-06 22:05                   ` [PATCH v2 0/3] rev-parse and "--" Jeff King
2013-12-06 22:05                     ` [PATCH v2 1/3] rev-parse: correctly diagnose revision errors before "--" Jeff King
2013-12-06 23:34                       ` Jonathan Nieder
2013-12-06 22:07                     ` [PATCH v2 2/3] rev-parse: be more careful with munging arguments Jeff King
2013-12-07  0:04                       ` Jonathan Nieder
2013-12-09 21:33                       ` Eric Sunshine
2013-12-06 22:08                     ` [PATCH v2 3/3] rev-parse: diagnose ambiguous revision/filename arguments Jeff King
2013-12-06 23:25                     ` [PATCH v2 0/3] rev-parse and "--" Jonathan Nieder
2013-12-06 23:30                       ` Jeff King
2013-12-09 19:05                     ` Junio C Hamano
2013-12-09 19:12                       ` Jonathan Nieder
2013-12-09 19:23                         ` Jonathan Nieder
2013-12-09 20:48                         ` Junio C Hamano
2013-12-09 20:56                           ` Jonathan Nieder [this message]
2013-12-09 21:10                             ` Junio C Hamano
2013-12-06  1:15             ` [BUG] redundant error message Duy Nguyen
2013-12-06 22:13               ` Jeff King

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

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

  git send-email \
    --in-reply-to=20131209205621.GW29959@google.com \
    --to=jrnieder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@gmail.com \
    --cc=peff@peff.net \
    /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.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.