{"thread":{"id":"45395","subject":"sequencer \"edit\" command always issues a warning","startedAt":"2017-03-16T13:16:14Z","lastAt":"2017-03-17T00:19:50Z","messageCount":4,"participants":["Jeff King","Junio C Hamano","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"314217","messageId":"20170316131534.4vpbub3vmde7maua@sigill.intra.peff.net","threadId":"45395","inReplyTo":null,"subject":"sequencer \"edit\" command always issues a warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-16T13:15:34Z","receivedAt":"2017-03-16T13:16:14Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I was rebasing with the new built-in sequencer code today, and I was\nsurprised to see the use of warning() here:\n\n  $ git rebase -i\n  [set one commit to 'edit']\n  warning: stopped at 6ce6b914a... odb_pack_keep(): stop generating keepfile name\n  You can amend the commit now, with\n    [...more instructions...]\n\nIt alarmed me for a minute until I realized that no, this is nothing to\nbe alarmed about, but just git doing exactly what I told it to do.\n\nThe original just wrote:\n\n  Stopped at 6ce6b914a... odb_pack_keep(): stop generating keepfile name\n\nIt would be easy to switch back:\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 1f729b053..8183a83c1 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1997,7 +1997,8 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)\n \t\t\tif (item->command == TODO_EDIT) {\n \t\t\t\tstruct commit *commit = item->commit;\n \t\t\t\tif (!res)\n-\t\t\t\t\twarning(_(\"stopped at %s... %.*s\"),\n+\t\t\t\t\tfprintf(stderr,\n+\t\t\t\t\t\t_(\"Stopped at %s...  %.*s\"),\n \t\t\t\t\t\tshort_commit_name(commit),\n \t\t\t\t\t\titem->arg_len, item->arg);\n \t\t\t\treturn error_with_patch(commit,\n\nand that would match most of the other messages that the command issues,\nwhich use a bare fprintf() and start with a capital letter. But I'm not\nsure if there was some reason to treat this one differently.\n\n-Peff\n"},{"id":"314270","messageId":"xmqqy3w4ama4.fsf@gitster.mtv.corp.google.com","threadId":"45395","inReplyTo":"20170316131534.4vpbub3vmde7maua@sigill.intra.peff.net","subject":"Re: sequencer \"edit\" command always issues a warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-16T22:29:07Z","receivedAt":"2017-03-16T22:30:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I was rebasing with the new built-in sequencer code today, and I was\n> surprised to see the use of warning() here:\n>\n>   $ git rebase -i\n>   [set one commit to 'edit']\n>   warning: stopped at 6ce6b914a... odb_pack_keep(): stop generating keepfile name\n>   You can amend the commit now, with\n>     [...more instructions...]\n>\n> It alarmed me for a minute until I realized that no, this is nothing to\n> be alarmed about, but just git doing exactly what I told it to do.\n>\n> The original just wrote:\n>\n>   Stopped at 6ce6b914a... odb_pack_keep(): stop generating keepfile name\n>\n> It would be easy to switch back:\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 1f729b053..8183a83c1 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -1997,7 +1997,8 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)\n>  \t\t\tif (item->command == TODO_EDIT) {\n>  \t\t\t\tstruct commit *commit = item->commit;\n>  \t\t\t\tif (!res)\n> -\t\t\t\t\twarning(_(\"stopped at %s... %.*s\"),\n> +\t\t\t\t\tfprintf(stderr,\n> +\t\t\t\t\t\t_(\"Stopped at %s...  %.*s\"),\n>  \t\t\t\t\t\tshort_commit_name(commit),\n>  \t\t\t\t\t\titem->arg_len, item->arg);\n>  \t\t\t\treturn error_with_patch(commit,\n>\n> and that would match most of the other messages that the command issues,\n> which use a bare fprintf() and start with a capital letter. But I'm not\n> sure if there was some reason to treat this one differently.\n\nI doubt there was.  At least, I didn't think I read any rationale\nfor switching in logs or in-code comments.\n\nIf we stop due to conflicting changes during a \"rebase -i\" or a\nrange \"cherry-pick/revert\" session, warning() might make sense, but\nI agree with you that stopping at \"edit\" is an expected thing.  If\nwe had info(\"stopped at...\") that may be appropriate, but writing it\nout to stderr is just fine, I would think.\n\nThanks for spotting.\n"},{"id":"314288","messageId":"alpine.DEB.2.20.1703162358520.3767@virtualbox","threadId":"45395","inReplyTo":"20170316131534.4vpbub3vmde7maua@sigill.intra.peff.net","subject":"Re: sequencer \"edit\" command always issues a warning","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-16T22:59:35Z","receivedAt":"2017-03-16T22:59:47Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 16 Mar 2017, Jeff King wrote:\n\n> I was rebasing with the new built-in sequencer code today, and I was\n> surprised to see the use of warning() here:\n> \n>   $ git rebase -i\n>   [set one commit to 'edit']\n>   warning: stopped at 6ce6b914a... odb_pack_keep(): stop generating keepfile name\n>   You can amend the commit now, with\n>     [...more instructions...]\n> \n> It alarmed me for a minute until I realized that no, this is nothing to\n> be alarmed about, but just git doing exactly what I told it to do.\n> \n> The original just wrote:\n> \n>   Stopped at 6ce6b914a... odb_pack_keep(): stop generating keepfile name\n> \n> It would be easy to switch back:\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 1f729b053..8183a83c1 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -1997,7 +1997,8 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)\n>  \t\t\tif (item->command == TODO_EDIT) {\n>  \t\t\t\tstruct commit *commit = item->commit;\n>  \t\t\t\tif (!res)\n> -\t\t\t\t\twarning(_(\"stopped at %s... %.*s\"),\n> +\t\t\t\t\tfprintf(stderr,\n> +\t\t\t\t\t\t_(\"Stopped at %s...  %.*s\"),\n>  \t\t\t\t\t\tshort_commit_name(commit),\n>  \t\t\t\t\t\titem->arg_len, item->arg);\n>  \t\t\t\treturn error_with_patch(commit,\n> \n> and that would match most of the other messages that the command issues,\n> which use a bare fprintf() and start with a capital letter. But I'm not\n> sure if there was some reason to treat this one differently.\n\nI do not recall why I chose warning(); probably just an oversight on my\npart. Your patch looks good.\n\nCiao,\nDscho\n"},{"id":"314295","messageId":"20170317001942.2lndsi47kdyzhane@sigill.intra.peff.net","threadId":"45395","inReplyTo":"alpine.DEB.2.20.1703162358520.3767@virtualbox","subject":"Re: sequencer \"edit\" command always issues a warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-17T00:19:42Z","receivedAt":"2017-03-17T00:19:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 16, 2017 at 11:59:35PM +0100, Johannes Schindelin wrote:\n\n> I do not recall why I chose warning(); probably just an oversight on my\n> part. Your patch looks good.\n\nThanks. Here it is with a commit message.\n\nI agree with Junio that if we had \"info()\" it would perhaps be the right\nthing to use. We can go down that road later if we choose to.\n\n-- >8 --\nSubject: [PATCH] sequencer: drop \"warning:\" when stopping for edit\n\nSince the conversion from shell to C in 56dc3ab04 (sequencer\n(rebase -i): implement the 'edit' command, 2017-01-02),\nstopping at an \"edit\" instruction went from:\n\n  $ git rebase -i\n  Stopped at 6ce6b914a... odb_pack_keep(): stop generating keepfile name\n  You can amend the commit now, with\n    [...more instructions...]\n\nto:\n\n  $ git rebase -i\n  warning: stopped at 6ce6b914a... odb_pack_keep(): stop generating keepfile name\n  You can amend the commit now, with\n    [...more instructions...]\n\nThe \"warning\" implies that it's something unexpected, but\nit's not. Let's switch back to the original message.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sequencer.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 1f729b053..8183a83c1 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1997,7 +1997,8 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)\n \t\t\tif (item->command == TODO_EDIT) {\n \t\t\t\tstruct commit *commit = item->commit;\n \t\t\t\tif (!res)\n-\t\t\t\t\twarning(_(\"stopped at %s... %.*s\"),\n+\t\t\t\t\tfprintf(stderr,\n+\t\t\t\t\t\t_(\"Stopped at %s...  %.*s\"),\n \t\t\t\t\t\tshort_commit_name(commit),\n \t\t\t\t\t\titem->arg_len, item->arg);\n \t\t\t\treturn error_with_patch(commit,\n-- \n2.12.0.623.g86ec6c963\n\n"}]}