On Wed, Aug 1, 2018 at 2:17 PM Junio C Hamano <gits...@pobox.com> wrote:
> Han-Wen Nienhuys <han...@google.com> writes:
> > Sorry for being dense, but do you want me to send an updated patch or
> > not based on your and Eric's comments or not?
>
> It would help to see the comments responded with either "such a
> change is not needed for such and such reasons", "it may make sense
> but let's leave it to a follow-up patch later," etc., or with a
> "here is an updated patch, taking all the comments to the previous
> version into account---note that I rejected that particular comment
> because of such and such reasons".
Right. The way to know whether or not an updated patch is warranted is
to respond to review comments, saying that you agree or disagree with
various points raised (and why), and by answering the (genuine)
questions raised during review. The outcome of the dialogue with
reviewers will make it clear if an updated patch is necessary. (It's
also a courtesy to respond to review comments since reviewing is
time-consuming business and it's good to let reviewers know that the
time spent reviewing was not in vain.)
Regarding my question about whether load_sideband_colors() can be
moved below the '!want_color_stderr(sideband_use_color)' conditional,
after studying the code further, I see that it can't be, because
load_sideband_colors() is responsible for setting
'sideband_use_color'. The fact that this code confused or left
questions in the mind of a reviewer may indicate that responsibilities
are not partitioned in the best manner, and that the code could be
structured to be more clear.
For instance, it might make sense to rip all the 'sideband_use_color'
gunk out of load_sideband_colors() and move it to
maybe_colorize_sideband(), perhaps like this:
void maybe_colorize_sideband(...)
{
/* one-time initialization */
if (sideband_use_color < 0) {
if (!git_config_get_string(key, &value))
sideband_use_color = git_config_colorbool(key, value);
if (sideband_use_color > 0)
load_sideband_colors();
}
if (!want_color_stderr(sideband_use_color)) {
strbuf_add(dest, src, n);
return;
}
...as before...
}
You may or may not agree with the above suggestion; it's good to let
the reviewer know how you feel about it.
Your follow-on comment about how Gerrit has for years used "ERROR" is
exactly the sort of information which should be in the commit messages
since it saves reviewers (and future readers, months or years down the
road) from head-scratching, wondering why the code was written the way
it was (strcasecmp() vs. strcmp(), for instance).
The more pertinent information you can say up front in the commit
message, the less likely reviewers will be confused or wonder why you
made the choices you did. My question about whether
maybe_colorize_sideband() is fed lines one-by-one or whether its input
may contain embedded newlines is a good example of how a more complete
commit message could help. As the author of the patch, you have been
working in this code and likely know the answer, but reviewers won't
necessarily have this information at hand. If the commit message says
up front that this function processes lines one-by-one, then the
reviewer feels reassured that the patch author understands the
implications of the change (as opposed to the patch author perhaps not
having thought of the possibility of embedded newlines). So, it's a
genuine question (I still don't know the answer.)
