patchfilter-branch: return 2 when nothing to rewrite

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.

Signed-off-by: Michele Locati <michele@locati.it>
---
 git-filter-branch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Thu, Mar 15, 2018 at 02:03:59PM +0100, Michele Locati wrote:

That sounds like a good feature. It doesn't look like we use "2" for anything else currently.

This should probably get a mention in the manpage at Documentation/git-filter-branch.txt, too.

Thanks.

-Peff

2018-03-15 15:12 GMT+01:00 Jeff King <peff@peff.net>:

Yes, I agree it would be useful. What about this addition right after the "Remap to ancestor" section?

EXIT CODE ---------

In general, this command will fail with an exit status of `1` in case of errors. When the filter can't fine anything to rewrite, the exit status is `2`.

-- Michele

On Thu, Mar 15, 2018 at 03:57:15PM +0100, Michele Locati wrote:

That seems like a good place (for those just reading on the list, it's right before the "examples" section).

It looks like we don't have many similar sections, but when we do we call them "EXIT STATUS" (which seems to match other projects like "grep").

s/fine/find/

Do we want to commit to status `1` for everything else? Most of the C code that dies does so with 128, and I wonder if that could propagate in some cases. IOW, could we leave room for that and for future changes with something like:

  On success, the exit status is `0`.  If the filter can't find any
  commits to rewrite, the exit status is `2`. On any other error,
  the exit status may be any other non-zero value.

-Peff

PS I think this is your first patch to Git. I forgot to say: welcome to
   the list!

Jeff King <peff@peff.net> writes:

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?".

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

Thanks.

On Thu, Mar 15, 2018 at 08:42:54AM -0700, Junio C Hamano wrote:

Hrm. I took the goal to mean that we used to exit with a failing "1" in this case, and now we would switch to a more-specific "2". And I think that matches the behavior of the patch:

-test $commits -eq 0 && die "Found nothing to rewrite" +test $commits -eq 0 && die_with_status 2 "Found nothing to rewrite"

Am I missing something?

-Peff

Jeff King <peff@peff.net> writes:

No, other than that I wrote my response before sufficiently caffeinated ;-)

Thanks, then other than the lack of doc updates, I do not see an issue.

2018-03-15 16:55 GMT+01:00 Junio C Hamano <gitster@pobox.com>:

Great! So, I'm ready to update the patch, including the doc changes, which will be the one suggested by Jeff:

EXIT STATUS -----------

On success, the exit status is `0`. If the filter can't find any commits to rewrite, the exit status is `2`. On any other error, the exit status may be any other non-zero value.

And yes, I'm a brand new contributor, so here's my question: how should I send an updated patch? I can't find anything related to this in https://github.com/git/git/blob/master/Documentation/SubmittingPatches

PS: nice community!

-- Michele

On Thu, Mar 15, 2018 at 05:18:59PM +0100, Michele Locati wrote:

Sounds good.

Usually you'd just send it in reply to the original thread with "[PATCH v2]" instead of just "[PATCH]" in the subject line. If you're using format-patch or send-email, you should be able to just add "-v2" (and --in-reply-to if you want to join the existing thread).

I thought SubmittingPatches discussed patch "re-rolls" like this, but I don't see any mention of it from a quick grep.

-Peff

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.

Signed-off-by: Michele Locati <michele@locati.it>
---
 Documentation/git-filter-branch.txt | 8 ++++++++
 git-filter-branch.sh                | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

Michele Locati <michele@locati.it> writes:

Thanks.  Will queue.

On Thu, Mar 15, 2018 at 06:09:18PM +0100, Michele Locati wrote:

Thanks, this looks good to me.

I did have one other thought while reading this, but I think it's OK to leave as-is:

I wondered if people might take "any commits to rewrite" to also mean the case where the filters do not actually change any commits (e.g, and index filter which removes a path that does not exist). That's currently a successful outcome but does issue a warning; it's not changed by this patch at all (and nor should it be).

If we wanted to make that more clear, we could perhaps mention the --state-branch option here explicitly. Not sure if it's worth it.

-Peff