git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: Michele Locati <michele@locati.it>, git@vger.kernel.org
Subject: Re: [PATCH] filter-branch: return 2 when nothing to rewrite
Date: Thu, 15 Mar 2018 08:42:54 -0700	[thread overview]
Message-ID: <xmqqa7v973b5.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20180315141220.GB27748@sigill.intra.peff.net> (Jeff King's message of "Thu, 15 Mar 2018 10:12:21 -0400")

Jeff King <peff@peff.net> writes:

> On Thu, Mar 15, 2018 at 02:03:59PM +0100, Michele Locati wrote:
>
>> Using the --state-branch option allows us to perform incremental filtering.
>> This may lead to having nothing to rewrite in subsequent filtering, so we need
>> a way to recognize this case.
>> So, let's exit with 2 instead of 1 when this "error" occurs.
>
> That sounds like a good feature. It doesn't look like we use "2" for
> anything else currently.

I do not want to sound overly negative against the first
contribution from a new contributor, but I am not sure if this is a
good idea.  While I do agree that the caller of filter-branch would
want _some_ way to tell if the call

 - got some new stuff,
 - got no error but did not get anything new, or
 - failed

and act accordingly, changing the exit code to a non-zero value for
the second case above would mean that existing scripts that have
happily been working would suddenly start failing.  Due to the lack
of an easy way to tell the first two cases apart, they may have been
doing _extra_ work after calling filter-branch when it found no new
development (resulting in an expensive no-op), or perhaps they
implemented their own way to tell the second case apart from the
first one and efficiently omitting extra work in the second case
already.  In either case, these scripts will get broken with this
change.

So I'd respond with a mild "no" with "can't we allow callers to tell
the first two cases apart in some other way so that we do not have
to break existing scripts?".

>> ---
>>  git-filter-branch.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> This should probably get a mention in the manpage at
> Documentation/git-filter-branch.txt, too.

Whatever solution we eventually end up with, it needs to be
documented.

Thanks.

  parent reply	other threads:[~2018-03-15 15:43 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-15 13:03 [PATCH] filter-branch: return 2 when nothing to rewrite Michele Locati
2018-03-15 14:12 ` Jeff King
2018-03-15 14:57   ` Michele Locati
2018-03-15 15:35     ` Jeff King
2018-03-15 15:42   ` Junio C Hamano [this message]
2018-03-15 15:48     ` Jeff King
2018-03-15 15:55       ` Junio C Hamano
2018-03-15 16:18         ` Michele Locati
2018-03-15 16:24           ` Jeff King
2018-03-15 17:09 ` [PATCH v2] " Michele Locati
2018-03-15 17:58   ` Junio C Hamano
2018-03-15 17:59   ` 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=xmqqa7v973b5.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=michele@locati.it \
    --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 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 NNTP newsgroup(s).