Re: [PATCH 06/15] sequencer: lib'ify read_populate_todo()

[Junio C Hamano]

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

[Johannes Schindelin]

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

[Junio C Hamano]

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(, fd, 0) < 0) {
>   close(fd);
>   strbuf_release();
> - 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();
>   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();
> - read_populate_todo(_list, opts);
> + if (read_populate_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()

[Johannes Schindelin]

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

[Eric Sunshine]

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(, fd, 0) < 0) {
> close(fd);
> strbuf_release();
> -   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();
> 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