Re: [PATCH 06/15] sequencer: lib'ify read_populate_todo()
Johannes Schindelin writes: > In short: I would really appreciate it if you could cut quoted text after > your last response. I think you are referring to the patch part in this case. As I was not making point-by-point comments on the proposed commit log message, quoting only that part and cutting the patch text would have been pointless. I could have cut the proposed log message and left the patch in, though, because the comments were not about what was in the proposed log message, but about what was not in it [*1*]. I left the patch part for other people's use, to make it easy for them to see what I was saying was correct and appropriate for what the patch does. Removing that would not have made much sense. But that is only true in this case. I try to see if I can trim quote more aggressively, but I would still err on the side of over-quoting than under-quoting [*2*]. [Footnote] *1* As to what was IN the proposed log message, I have one comment. I do not think "To be truly useful" adds any value, as there is nothing "truly" about what this series does. The original was "truly" useful for the purpose of the sequencer machinery and its use in the current callers, just like with this series it becomes "truly" useful for envisioned new callers that want to handle error conditions themselves. The change is making it "more useful" for one different use case. It may not be "truly" useful for other use cases that sequencer machinery could be used even with the "eradicate die and exit" change and other people may bring a new use case that wants it to be even "more useful". The fact that it may not be directly usable by a new use case without further change does not make the result of applying this proposed series less than "truly useful". The "truly" adjective implies absolute, but there is nothing absolute in incremental improvements. It is always relative to the context within which the machinery was designed to be used. "To make it usable for callers that want to handle errors themselves (instead of just dying and the calling process handle it), let's turn die's and exit's to returning negative values." is probably closer to what I would have expected. *2* As I read the quoted part before sending my response out, erring on the side not to underquote tends to avoid a mistake that invites: "I think you misunderstood what I wrote in the part you snipped from your quote; perhaps you skimmed it without fully reading." -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/15] sequencer: lib'ify read_populate_todo()
Hi Junio, On Thu, 25 Aug 2016, Junio C Hamano wrote: > Johannes Schindelin writes: > > > To be truly useful, the sequencer should never die() but always return > > an error. > > > > Signed-off-by: Johannes Schindelin > > --- > > sequencer.c | 14 +- > > 1 file changed, 9 insertions(+), 5 deletions(-) > > Instead of dying there, you let the caller high up in the callchain > to notice the error and handle it (by dying). > > The only caller of read_populate_todo(), sequencer_continue() can > already return errors, so its caller must be already prepared to > handle error returns, and with this step, you make it notice an > error return from this function. So this is a safe conversion to > make read_populate_todo() callable from new callers that want it not > to die, without changing the external behaviour of anything > existing. > > Good. > > By the way, I am writing these as review comments because I do not > want to keep repeating this kind of analysis as a reviewer. I am > demonstrating what should have been in the commit log message > instead, so that the reviewer does not have to spend extra time, if > the reviewer trusts the author's diligence well enough, to see if > the conversion makes sense. > > Please follow the example when/if you have to reroll. I want the > patches to show the evidence of careful analysis to reviewers so > that they can gauge the trustworthiness of the patches. With this > round of patches, honestly, I cannot tell if it is a mechanical > substitution alone, or such a substitution followed by a careful > verification of the callers. Duly noted. I fixed up the patch order as well as the commit messages. Now on to a request of my own: I am once again reminded that I have *no* good mail client to serve me. (Before you suggest it: no, emacs won't cut it for me, nor mutt. Thank you very much for your suggestion.) So it is really annoying for me to scroll through quoted text, page after page, only to find that none of it got a response after all. In short: I would really appreciate it if you could cut quoted text after your last response. Thanks, Dscho P.S.: It's this mailing list thing again. I would *love* to switch to a mail client more to my liking, but the ones I like all won't respect white space in patch files that are pasted verbatim into the mail body. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/15] sequencer: lib'ify read_populate_todo()
Johannes Schindelin writes: > To be truly useful, the sequencer should never die() but always return > an error. > > Signed-off-by: Johannes Schindelin > --- > sequencer.c | 14 +- > 1 file changed, 9 insertions(+), 5 deletions(-) Instead of dying there, you let the caller high up in the callchain to notice the error and handle it (by dying). The only caller of read_populate_todo(), sequencer_continue() can already return errors, so its caller must be already prepared to handle error returns, and with this step, you make it notice an error return from this function. So this is a safe conversion to make read_populate_todo() callable from new callers that want it not to die, without changing the external behaviour of anything existing. Good. By the way, I am writing these as review comments because I do not want to keep repeating this kind of analysis as a reviewer. I am demonstrating what should have been in the commit log message instead, so that the reviewer does not have to spend extra time, if the reviewer trusts the author's diligence well enough, to see if the conversion makes sense. Please follow the example when/if you have to reroll. I want the patches to show the evidence of careful analysis to reviewers so that they can gauge the trustworthiness of the patches. With this round of patches, honestly, I cannot tell if it is a mechanical substitution alone, or such a substitution followed by a careful verification of the callers. > diff --git a/sequencer.c b/sequencer.c > index a8c3a48..5f6b020 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -746,7 +746,7 @@ static int parse_insn_buffer(char *buf, struct > commit_list **todo_list, > return 0; > } > > -static void read_populate_todo(struct commit_list **todo_list, > +static int read_populate_todo(struct commit_list **todo_list, > struct replay_opts *opts) > { > struct strbuf buf = STRBUF_INIT; > @@ -754,18 +754,21 @@ static void read_populate_todo(struct commit_list > **todo_list, > > fd = open(git_path_todo_file(), O_RDONLY); > if (fd < 0) > - die_errno(_("Could not open %s"), git_path_todo_file()); > + return error(_("Could not open %s (%s)"), > + git_path_todo_file(), strerror(errno)); > if (strbuf_read(&buf, fd, 0) < 0) { > close(fd); > strbuf_release(&buf); > - die(_("Could not read %s."), git_path_todo_file()); > + return error(_("Could not read %s."), git_path_todo_file()); > } > close(fd); > > res = parse_insn_buffer(buf.buf, todo_list, opts); > strbuf_release(&buf); > if (res) > - die(_("Unusable instruction sheet: %s"), git_path_todo_file()); > + return error(_("Unusable instruction sheet: %s"), > + git_path_todo_file()); > + return 0; > } > > static int populate_opts_cb(const char *key, const char *value, void *data) > @@ -1015,7 +1018,8 @@ static int sequencer_continue(struct replay_opts *opts) > if (!file_exists(git_path_todo_file())) > return continue_single_pick(); > read_populate_opts(&opts); > - read_populate_todo(&todo_list, opts); > + if (read_populate_todo(&todo_list, opts)) > + return -1; > > /* Verify that the conflict has been resolved */ > if (file_exists(git_path_cherry_pick_head()) || -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/15] sequencer: lib'ify read_populate_todo()
Hi Eric, On Wed, 24 Aug 2016, Eric Sunshine wrote: > On Tue, Aug 23, 2016 at 12:07 PM, Johannes Schindelin > wrote: > > To be truly useful, the sequencer should never die() but always return > > an error. > > > > Signed-off-by: Johannes Schindelin > > --- > > diff --git a/sequencer.c b/sequencer.c > > @@ -754,18 +754,21 @@ static void read_populate_todo(struct commit_list > > **todo_list, > > > > fd = open(git_path_todo_file(), O_RDONLY); > > if (fd < 0) > > - die_errno(_("Could not open %s"), git_path_todo_file()); > > + return error(_("Could not open %s (%s)"), > > + git_path_todo_file(), strerror(errno)); > > error_errno() perhaps? Absolutely! Thanks, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/15] sequencer: lib'ify read_populate_todo()
On Tue, Aug 23, 2016 at 12:07 PM, Johannes Schindelin wrote: > To be truly useful, the sequencer should never die() but always return > an error. > > Signed-off-by: Johannes Schindelin > --- > diff --git a/sequencer.c b/sequencer.c > @@ -754,18 +754,21 @@ static void read_populate_todo(struct commit_list > **todo_list, > > fd = open(git_path_todo_file(), O_RDONLY); > if (fd < 0) > - die_errno(_("Could not open %s"), git_path_todo_file()); > + return error(_("Could not open %s (%s)"), > + git_path_todo_file(), strerror(errno)); error_errno() perhaps? > if (strbuf_read(&buf, fd, 0) < 0) { > close(fd); > strbuf_release(&buf); > - die(_("Could not read %s."), git_path_todo_file()); > + return error(_("Could not read %s."), git_path_todo_file()); > } > close(fd); > > res = parse_insn_buffer(buf.buf, todo_list, opts); > strbuf_release(&buf); > if (res) > - die(_("Unusable instruction sheet: %s"), > git_path_todo_file()); > + return error(_("Unusable instruction sheet: %s"), > + git_path_todo_file()); Neither 'fd' nor 'buf' are leaked by these two new error returns. Good. > + return 0; > } -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html