{"thread":{"id":"48047","subject":"[PATCH] filter-branch: return 2 when nothing to rewrite","startedAt":"2018-03-15T13:04:16Z","lastAt":"2018-03-15T18:00:50Z","messageCount":12,"participants":["Michele Locati","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"341745","messageId":"20180315130359.6108-1-michele@locati.it","threadId":"48047","inReplyTo":null,"subject":"[PATCH] filter-branch: return 2 when nothing to rewrite","fromName":"Michele Locati","fromEmail":"michele@locati.it","sentAt":"2018-03-15T13:03:59Z","receivedAt":"2018-03-15T13:04:16Z","isPatch":true,"sender":{"key":"michele@locati.it","avatar":"https://avatars.githubusercontent.com/u/928116?v=4"},"body":"Using the --state-branch option allows us to perform incremental filtering.\nThis may lead to having nothing to rewrite in subsequent filtering, so we need\na way to recognize this case.\nSo, let's exit with 2 instead of 1 when this \"error\" occurs.\n\nSigned-off-by: Michele Locati \n---\n git-filter-branch.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-filter-branch.sh b/git-filter-branch.sh\nindex 1b7e4b2cd..c285fdb90 100755\n--- a/git-filter-branch.sh\n+++ b/git-filter-branch.sh\n@@ -310,7 +310,7 @@ git rev-list --reverse --topo-order --default HEAD \\\n \tdie \"Could not get the commits\"\n commits=$(wc -l <../revs | tr -d \" \")\n \n-test $commits -eq 0 && die \"Found nothing to rewrite\"\n+test $commits -eq 0 && die_with_status 2 \"Found nothing to rewrite\"\n \n # Rewrite the commits\n report_progress ()\n-- \n2.16.2.windows.1\n\n"},{"id":"341747","messageId":"20180315141220.GB27748@sigill.intra.peff.net","threadId":"48047","inReplyTo":"20180315130359.6108-1-michele@locati.it","subject":"Re: [PATCH] filter-branch: return 2 when nothing to rewrite","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-15T14:12:21Z","receivedAt":"2018-03-15T14:12:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 15, 2018 at 02:03:59PM +0100, Michele Locati wrote:\n\n> Using the --state-branch option allows us to perform incremental filtering.\n> This may lead to having nothing to rewrite in subsequent filtering, so we need\n> a way to recognize this case.\n> So, let's exit with 2 instead of 1 when this \"error\" occurs.\n\nThat sounds like a good feature. It doesn't look like we use \"2\" for\nanything else currently.\n\n> ---\n> git-filter-branch.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n\nThis should probably get a mention in the manpage at\nDocumentation/git-filter-branch.txt, too.\n\nThanks.\n\n-Peff\n"},{"id":"341748","messageId":"CAGen01iZTs1FC3tsuMF9SAS0QcKxN0Sk1CPeZ+YNyh5X8sdgtg@mail.gmail.com","threadId":"48047","inReplyTo":"20180315141220.GB27748@sigill.intra.peff.net","subject":"Re: [PATCH] filter-branch: return 2 when nothing to rewrite","fromName":"Michele Locati","fromEmail":"michele@locati.it","sentAt":"2018-03-15T14:57:15Z","receivedAt":"2018-03-15T14:57:23Z","isPatch":true,"sender":{"key":"michele@locati.it","avatar":"https://avatars.githubusercontent.com/u/928116?v=4"},"body":"2018-03-15 15:12 GMT+01:00 Jeff King :\n> On Thu, Mar 15, 2018 at 02:03:59PM +0100, Michele Locati wrote:\n>\n>> Using the --state-branch option allows us to perform incremental filtering.\n>> This may lead to having nothing to rewrite in subsequent filtering, so we need\n>> a way to recognize this case.\n>> So, let's exit with 2 instead of 1 when this \"error\" occurs.\n>\n> That sounds like a good feature. It doesn't look like we use \"2\" for\n> anything else currently.\n>\n>> ---\n>> git-filter-branch.sh | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> This should probably get a mention in the manpage at\n> Documentation/git-filter-branch.txt, too.\n\n\nYes, I agree it would be useful. What about this addition right after the\n\"Remap to ancestor\" section?\n\nEXIT CODE\n---------\n\nIn general, this command will fail with an exit status of `1` in case of errors.\nWhen the filter can't fine anything to rewrite, the exit status is `2`.\n\n\n--\nMichele\n"},{"id":"341751","messageId":"20180315153525.GA29265@sigill.intra.peff.net","threadId":"48047","inReplyTo":"CAGen01iZTs1FC3tsuMF9SAS0QcKxN0Sk1CPeZ+YNyh5X8sdgtg@mail.gmail.com","subject":"Re: [PATCH] filter-branch: return 2 when nothing to rewrite","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-15T15:35:25Z","receivedAt":"2018-03-15T15:35:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 15, 2018 at 03:57:15PM +0100, Michele Locati wrote:\n\n> >> git-filter-branch.sh | 2 +-\n> >> 1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > This should probably get a mention in the manpage at\n> > Documentation/git-filter-branch.txt, too.\n> \n> Yes, I agree it would be useful. What about this addition right after the\n> \"Remap to ancestor\" section?\n> \n> EXIT CODE\n> ---------\n\nThat seems like a good place (for those just reading on the list, it's\nright before the \"examples\" section).\n\nIt looks like we don't have many similar sections, but when we do we\ncall them \"EXIT STATUS\" (which seems to match other projects like\n\"grep\").\n\n> In general, this command will fail with an exit status of `1` in case of errors.\n> When the filter can't fine anything to rewrite, the exit status is `2`.\n\ns/fine/find/\n\nDo we want to commit to status `1` for everything else? Most of the\nC code that dies does so with 128, and I wonder if that could propagate\nin some cases. IOW, could we leave room for that and for future changes\nwith something like:\n\n On success, the exit status is `0`. If the filter can't find any\n commits to rewrite, the exit status is `2`. On any other error,\n the exit status may be any other non-zero value.\n\n-Peff\n\nPS I think this is your first patch to Git. I forgot to say: welcome to\n the list!\n"},{"id":"341752","messageId":"xmqqa7v973b5.fsf@gitster-ct.c.googlers.com","threadId":"48047","inReplyTo":"20180315141220.GB27748@sigill.intra.peff.net","subject":"Re: [PATCH] filter-branch: return 2 when nothing to rewrite","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-15T15:42:54Z","receivedAt":"2018-03-15T15:43:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Mar 15, 2018 at 02:03:59PM +0100, Michele Locati wrote:\n>\n>> Using the --state-branch option allows us to perform incremental filtering.\n>> This may lead to having nothing to rewrite in subsequent filtering, so we need\n>> a way to recognize this case.\n>> So, let's exit with 2 instead of 1 when this \"error\" occurs.\n>\n> That sounds like a good feature. It doesn't look like we use \"2\" for\n> anything else currently.\n\nI do not want to sound overly negative against the first\ncontribution from a new contributor, but I am not sure if this is a\ngood idea. While I do agree that the caller of filter-branch would\nwant _some_ way to tell if the call\n\n - got some new stuff,\n - got no error but did not get anything new, or\n - failed\n\nand act accordingly, changing the exit code to a non-zero value for\nthe second case above would mean that existing scripts that have\nhappily been working would suddenly start failing. Due to the lack\nof an easy way to tell the first two cases apart, they may have been\ndoing _extra_ work after calling filter-branch when it found no new\ndevelopment (resulting in an expensive no-op), or perhaps they\nimplemented their own way to tell the second case apart from the\nfirst one and efficiently omitting extra work in the second case\nalready. In either case, these scripts will get broken with this\nchange.\n\nSo I'd respond with a mild \"no\" with \"can't we allow callers to tell\nthe first two cases apart in some other way so that we do not have\nto break existing scripts?\".\n\n>> ---\n>> git-filter-branch.sh | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> This should probably get a mention in the manpage at\n> Documentation/git-filter-branch.txt, too.\n\nWhatever solution we eventually end up with, it needs to be\ndocumented.\n\nThanks.\n"},{"id":"341753","messageId":"20180315154815.GA29874@sigill.intra.peff.net","threadId":"48047","inReplyTo":"xmqqa7v973b5.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] filter-branch: return 2 when nothing to rewrite","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-15T15:48:15Z","receivedAt":"2018-03-15T15:48:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 15, 2018 at 08:42:54AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Thu, Mar 15, 2018 at 02:03:59PM +0100, Michele Locati wrote:\n> >\n> >> Using the --state-branch option allows us to perform incremental filtering.\n> >> This may lead to having nothing to rewrite in subsequent filtering, so we need\n> >> a way to recognize this case.\n> >> So, let's exit with 2 instead of 1 when this \"error\" occurs.\n> >\n> > That sounds like a good feature. It doesn't look like we use \"2\" for\n> > anything else currently.\n> \n> I do not want to sound overly negative against the first\n> contribution from a new contributor, but I am not sure if this is a\n> good idea. While I do agree that the caller of filter-branch would\n> want _some_ way to tell if the call\n> \n> - got some new stuff,\n> - got no error but did not get anything new, or\n> - failed\n> \n> and act accordingly, changing the exit code to a non-zero value for\n> the second case above would mean that existing scripts that have\n> happily been working would suddenly start failing. Due to the lack\n> of an easy way to tell the first two cases apart, they may have been\n> doing _extra_ work after calling filter-branch when it found no new\n> development (resulting in an expensive no-op), or perhaps they\n> implemented their own way to tell the second case apart from the\n> first one and efficiently omitting extra work in the second case\n> already. In either case, these scripts will get broken with this\n> change.\n\nHrm. I took the goal to mean that we used to exit with a failing \"1\" in\nthis case, and now we would switch to a more-specific \"2\". And I think\nthat matches the behavior of the patch:\n\n-test $commits -eq 0 && die \"Found nothing to rewrite\"\n+test $commits -eq 0 && die_with_status 2 \"Found nothing to rewrite\"\n\nAm I missing something?\n\n-Peff\n"},{"id":"341755","messageId":"xmqq605x72qs.fsf@gitster-ct.c.googlers.com","threadId":"48047","inReplyTo":"20180315154815.GA29874@sigill.intra.peff.net","subject":"Re: [PATCH] filter-branch: return 2 when nothing to rewrite","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-15T15:55:07Z","receivedAt":"2018-03-15T15:55:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Hrm. I took the goal to mean that we used to exit with a failing \"1\" in\n> this case, and now we would switch to a more-specific \"2\". And I think\n> that matches the behavior of the patch:\n>\n> -test $commits -eq 0 && die \"Found nothing to rewrite\"\n> +test $commits -eq 0 && die_with_status 2 \"Found nothing to rewrite\"\n>\n> Am I missing something?\n\nNo, other than that I wrote my response before sufficiently\ncaffeinated ;-)\n\nThanks, then other than the lack of doc updates, I do not see an\nissue.\n"},{"id":"341756","messageId":"CAGen01hodC=z_74z+7fSSrx2kvPnSbOQaML9kBb9iO6xCvWHQA@mail.gmail.com","threadId":"48047","inReplyTo":"xmqq605x72qs.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] filter-branch: return 2 when nothing to rewrite","fromName":"Michele Locati","fromEmail":"michele@locati.it","sentAt":"2018-03-15T16:18:59Z","receivedAt":"2018-03-15T16:19:07Z","isPatch":true,"sender":{"key":"michele@locati.it","avatar":"https://avatars.githubusercontent.com/u/928116?v=4"},"body":"2018-03-15 16:55 GMT+01:00 Junio C Hamano :\n> Jeff King writes:\n>\n>> Hrm. I took the goal to mean that we used to exit with a failing \"1\" in\n>> this case, and now we would switch to a more-specific \"2\". And I think\n>> that matches the behavior of the patch:\n>>\n>> -test $commits -eq 0 && die \"Found nothing to rewrite\"\n>> +test $commits -eq 0 && die_with_status 2 \"Found nothing to rewrite\"\n>>\n>> Am I missing something?\n>\n> No, other than that I wrote my response before sufficiently\n> caffeinated ;-)\n>\n> Thanks, then other than the lack of doc updates, I do not see an\n> issue.\n\n\nGreat! So, I'm ready to update the patch, including the doc changes,\nwhich will be\nthe one suggested by Jeff:\n\n\nEXIT STATUS\n-----------\n\nOn success, the exit status is `0`. If the filter can't find any commits to\nrewrite, the exit status is `2`. On any other error, the exit status may be\nany other non-zero value.\n\n\nAnd yes, I'm a brand new contributor, so here's my question: how should I\nsend an updated patch? I can't find anything related to this in\nhttps://github.com/git/git/blob/master/Documentation/SubmittingPatches\n\nPS: nice community!\n\n--\nMichele\n"},{"id":"341757","messageId":"20180315162457.GA31351@sigill.intra.peff.net","threadId":"48047","inReplyTo":"CAGen01hodC=z_74z+7fSSrx2kvPnSbOQaML9kBb9iO6xCvWHQA@mail.gmail.com","subject":"Re: [PATCH] filter-branch: return 2 when nothing to rewrite","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-15T16:24:58Z","receivedAt":"2018-03-15T16:25:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 15, 2018 at 05:18:59PM +0100, Michele Locati wrote:\n\n> Great! So, I'm ready to update the patch, including the doc changes,\n> which will be\n> the one suggested by Jeff:\n> [...]\n\nSounds good.\n\n> And yes, I'm a brand new contributor, so here's my question: how should I\n> send an updated patch? I can't find anything related to this in\n> https://github.com/git/git/blob/master/Documentation/SubmittingPatches\n\nUsually you'd just send it in reply to the original thread with \"[PATCH\nv2]\" instead of just \"[PATCH]\" in the subject line. If you're using\nformat-patch or send-email, you should be able to just add \"-v2\" (and\n--in-reply-to if you want to join the existing thread).\n\nI thought SubmittingPatches discussed patch \"re-rolls\" like this, but I\ndon't see any mention of it from a quick grep.\n\n-Peff\n"},{"id":"341771","messageId":"20180315170918.1984-1-michele@locati.it","threadId":"48047","inReplyTo":"20180315130359.6108-1-michele@locati.it","subject":"[PATCH v2] filter-branch: return 2 when nothing to rewrite","fromName":"Michele Locati","fromEmail":"michele@locati.it","sentAt":"2018-03-15T17:09:18Z","receivedAt":"2018-03-15T17:09:37Z","isPatch":true,"sender":{"key":"michele@locati.it","avatar":"https://avatars.githubusercontent.com/u/928116?v=4"},"body":"Using the --state-branch option allows us to perform incremental filtering.\nThis may lead to having nothing to rewrite in subsequent filtering, so we need\na way to recognize this case.\nSo, let's exit with 2 instead of 1 when this \"error\" occurs.\n\nSigned-off-by: Michele Locati \n---\n Documentation/git-filter-branch.txt | 8 ++++++++\n git-filter-branch.sh | 2 +-\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-filter-branch.txt b/Documentation/git-filter-branch.txt\nindex 3a52e4dce..b63404318 100644\n--- a/Documentation/git-filter-branch.txt\n+++ b/Documentation/git-filter-branch.txt\n@@ -222,6 +222,14 @@ this purpose, they are instead rewritten to point at the nearest ancestor that\n was not excluded.\n \n \n+EXIT STATUS\n+-----------\n+\n+On success, the exit status is `0`. If the filter can't find any commits to\n+rewrite, the exit status is `2`. On any other error, the exit status may be\n+any other non-zero value.\n+\n+\n Examples\n --------\n \ndiff --git a/git-filter-branch.sh b/git-filter-branch.sh\nindex 1b7e4b2cd..c285fdb90 100755\n--- a/git-filter-branch.sh\n+++ b/git-filter-branch.sh\n@@ -310,7 +310,7 @@ git rev-list --reverse --topo-order --default HEAD \\\n \tdie \"Could not get the commits\"\n commits=$(wc -l <../revs | tr -d \" \")\n \n-test $commits -eq 0 && die \"Found nothing to rewrite\"\n+test $commits -eq 0 && die_with_status 2 \"Found nothing to rewrite\"\n \n # Rewrite the commits\n report_progress ()\n-- \n2.16.2.windows.1\n\n"},{"id":"341816","messageId":"xmqqbmfp5ifw.fsf@gitster-ct.c.googlers.com","threadId":"48047","inReplyTo":"20180315170918.1984-1-michele@locati.it","subject":"Re: [PATCH v2] filter-branch: return 2 when nothing to rewrite","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-15T17:58:59Z","receivedAt":"2018-03-15T18:00:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michele Locati writes:\n\n> Using the --state-branch option allows us to perform incremental filtering.\n> This may lead to having nothing to rewrite in subsequent filtering, so we need\n> a way to recognize this case.\n> So, let's exit with 2 instead of 1 when this \"error\" occurs.\n>\n> Signed-off-by: Michele Locati \n> ---\n> Documentation/git-filter-branch.txt | 8 ++++++++\n> git-filter-branch.sh | 2 +-\n> 2 files changed, 9 insertions(+), 1 deletion(-)\n\nThanks. Will queue.\n\n>\n> diff --git a/Documentation/git-filter-branch.txt b/Documentation/git-filter-branch.txt\n> index 3a52e4dce..b63404318 100644\n> --- a/Documentation/git-filter-branch.txt\n> +++ b/Documentation/git-filter-branch.txt\n> @@ -222,6 +222,14 @@ this purpose, they are instead rewritten to point at the nearest ancestor that\n> was not excluded.\n> \n> \n> +EXIT STATUS\n> +-----------\n> +\n> +On success, the exit status is `0`. If the filter can't find any commits to\n> +rewrite, the exit status is `2`. On any other error, the exit status may be\n> +any other non-zero value.\n> +\n> +\n> Examples\n> --------\n> \n> diff --git a/git-filter-branch.sh b/git-filter-branch.sh\n> index 1b7e4b2cd..c285fdb90 100755\n> --- a/git-filter-branch.sh\n> +++ b/git-filter-branch.sh\n> @@ -310,7 +310,7 @@ git rev-list --reverse --topo-order --default HEAD \\\n> \tdie \"Could not get the commits\"\n> commits=$(wc -l <../revs | tr -d \" \")\n> \n> -test $commits -eq 0 && die \"Found nothing to rewrite\"\n> +test $commits -eq 0 && die_with_status 2 \"Found nothing to rewrite\"\n> \n> # Rewrite the commits\n> report_progress ()\n"},{"id":"341817","messageId":"20180315175935.GA8752@sigill.intra.peff.net","threadId":"48047","inReplyTo":"20180315170918.1984-1-michele@locati.it","subject":"Re: [PATCH v2] filter-branch: return 2 when nothing to rewrite","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-15T17:59:35Z","receivedAt":"2018-03-15T18:00:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 15, 2018 at 06:09:18PM +0100, Michele Locati wrote:\n\n> Using the --state-branch option allows us to perform incremental filtering.\n> This may lead to having nothing to rewrite in subsequent filtering, so we need\n> a way to recognize this case.\n> So, let's exit with 2 instead of 1 when this \"error\" occurs.\n\nThanks, this looks good to me.\n\nI did have one other thought while reading this, but I think it's OK to\nleave as-is:\n\n> +EXIT STATUS\n> +-----------\n> +\n> +On success, the exit status is `0`. If the filter can't find any commits to\n> +rewrite, the exit status is `2`. On any other error, the exit status may be\n> +any other non-zero value.\n\nI wondered if people might take \"any commits to rewrite\" to also mean\nthe case where the filters do not actually change any commits (e.g, and\nindex filter which removes a path that does not exist). That's currently\na successful outcome but does issue a warning; it's not changed by this\npatch at all (and nor should it be).\n\nIf we wanted to make that more clear, we could perhaps mention the\n--state-branch option here explicitly. Not sure if it's worth it.\n\n-Peff\n"}]}