[PATCH] filter-branch: return 2 when nothing to rewrite

[PATCH] filter-branch: return 2 when nothing to rewrite

[Michele Locati]

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(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 1b7e4b2cd..c285fdb90 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -310,7 +310,7 @@ git rev-list --reverse --topo-order --default HEAD \
 	die "Could not get the commits"
 commits=$(wc -l <../revs | tr -d " ")
 
-test $commits -eq 0 && die "Found nothing to rewrite"
+test $commits -eq 0 && die_with_status 2 "Found nothing to rewrite"
 
 # Rewrite the commits
 report_progress ()
-- 
2.16.2.windows.1

Re: [PATCH] filter-branch: return 2 when nothing to rewrite

[Jeff King]

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.

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

Thanks.

-Peff

Re: [PATCH] filter-branch: return 2 when nothing to rewrite

[Michele Locati]

2018-03-15 15:12 GMT+01:00 Jeff King <peff@peff.net>:
> 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.
>
>> ---
>>  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.


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

Re: [PATCH] filter-branch: return 2 when nothing to rewrite

[Jeff King]

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

> >>  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.
> 
> Yes, I agree it would be useful. What about this addition right after the
> "Remap to ancestor" section?
> 
> EXIT CODE
> ---------

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

> 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`.

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!

Re: [PATCH] filter-branch: return 2 when nothing to rewrite

[Junio C Hamano]

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.

Re: [PATCH] filter-branch: return 2 when nothing to rewrite

[Jeff King]

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

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

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

Re: [PATCH] filter-branch: return 2 when nothing to rewrite

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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.

Re: [PATCH] filter-branch: return 2 when nothing to rewrite

[Michele Locati]

2018-03-15 16:55 GMT+01:00 Junio C Hamano <gitster@pobox.com>:
> Jeff King <peff@peff.net> writes:
>
>> 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?
>
> 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.


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

Re: [PATCH] filter-branch: return 2 when nothing to rewrite

[Jeff King]

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

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

Sounds good.

> 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

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

[PATCH v2] filter-branch: return 2 when nothing to rewrite

[Michele Locati]

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(-)

diff --git a/Documentation/git-filter-branch.txt b/Documentation/git-filter-branch.txt
index 3a52e4dce..b63404318 100644
--- a/Documentation/git-filter-branch.txt
+++ b/Documentation/git-filter-branch.txt
@@ -222,6 +222,14 @@ this purpose, they are instead rewritten to point at the nearest ancestor that
 was not excluded.
 
 
+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.
+
+
 Examples
 --------
 
diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 1b7e4b2cd..c285fdb90 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -310,7 +310,7 @@ git rev-list --reverse --topo-order --default HEAD \
 	die "Could not get the commits"
 commits=$(wc -l <../revs | tr -d " ")
 
-test $commits -eq 0 && die "Found nothing to rewrite"
+test $commits -eq 0 && die_with_status 2 "Found nothing to rewrite"
 
 # Rewrite the commits
 report_progress ()
-- 
2.16.2.windows.1

Re: [PATCH v2] filter-branch: return 2 when nothing to rewrite

[Junio C Hamano]

Michele Locati <michele@locati.it> writes:

> 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(-)

Thanks.  Will queue.

>
> diff --git a/Documentation/git-filter-branch.txt b/Documentation/git-filter-branch.txt
> index 3a52e4dce..b63404318 100644
> --- a/Documentation/git-filter-branch.txt
> +++ b/Documentation/git-filter-branch.txt
> @@ -222,6 +222,14 @@ this purpose, they are instead rewritten to point at the nearest ancestor that
>  was not excluded.
>  
>  
> +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.
> +
> +
>  Examples
>  --------
>  
> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> index 1b7e4b2cd..c285fdb90 100755
> --- a/git-filter-branch.sh
> +++ b/git-filter-branch.sh
> @@ -310,7 +310,7 @@ git rev-list --reverse --topo-order --default HEAD \
>  	die "Could not get the commits"
>  commits=$(wc -l <../revs | tr -d " ")
>  
> -test $commits -eq 0 && die "Found nothing to rewrite"
> +test $commits -eq 0 && die_with_status 2 "Found nothing to rewrite"
>  
>  # Rewrite the commits
>  report_progress ()

Re: [PATCH v2] filter-branch: return 2 when nothing to rewrite

[Jeff King]

On Thu, Mar 15, 2018 at 06:09:18PM +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.

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:

> +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.

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