From: Junio C Hamano <gitster@pobox•com>
To: Lars Schneider <larsxschneider@gmail•com>
Cc: Luke Diamand <luke@diamand•org>, git@vger•kernel.org
Subject: Re: [PATCH v2 1/4] git-p4: add optional type specifier to gitConfig reader
Date: Thu, 03 Sep 2015 14:31:26 -0700 [thread overview]
Message-ID: <xmqqio7ri40h.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <6FAAE139-9010-4C68-AA97-2739E9A09564@gmail.com> (Lars Schneider's message of "Thu, 3 Sep 2015 22:53:47 +0200")
Lars Schneider <larsxschneider@gmail•com> writes:
> In case I agree with a reviewer. What is the more appropriate action?
> A response like the one above or a new role that includes the change
> right away? I don’t want to spam the list with lots of tiny changes…
Responding to review comment like you did with "will do" is
perfectly fine.
When you do think you will (eventually) want to send an updated
patch, there are things other than "will do" that you can say. "I
understand what you said, but I am not sure how exactly to make this
better. Perhaps lend me a help?" is good, too.
An explanation, followed by "ok?", in response to "it is unclear
what you are doing here" (commenting on code) or to "I cannot
understand what this is trying to say" (commenting on log message),
is problematic because your intention becomes ambiguous.
The reviewers are rarely saying "I do not understand; educate me."
but "I do not understand, and it is likely many others don't, too.
Make it more easily understandable." is what they mean.
An explanation with "ok?" can be taken as a sign that you mistook
the review comment as "educate me".
What I meant was .... Do you think commenting the code here
with the above description is good enough? Or do you think
of a way to restructure the code itself to be more self
evident?
or something like that may be a way to avoid the ambiguity.
Thanks.
next prev parent reply other threads:[~2015-09-03 21:31 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-09-03 16:35 [PATCH v2] git-p4: add support for large file systems larsxschneider
2015-09-03 16:35 ` [PATCH v2 1/4] git-p4: add optional type specifier to gitConfig reader larsxschneider
2015-09-03 19:55 ` Luke Diamand
2015-09-03 20:14 ` Lars Schneider
2015-09-03 20:18 ` Junio C Hamano
2015-09-03 20:53 ` Lars Schneider
2015-09-03 21:31 ` Junio C Hamano [this message]
2015-09-04 7:49 ` Lars Schneider
2015-09-03 16:35 ` [PATCH v2 2/4] git-p4: add gitConfigInt reader larsxschneider
2015-09-03 19:57 ` Luke Diamand
2015-09-03 20:17 ` Lars Schneider
2015-09-03 20:20 ` Luke Diamand
2015-09-03 16:35 ` [PATCH v2 3/4] git-p4: return an empty list if a list config has no values larsxschneider
2015-09-03 16:35 ` [PATCH v2 4/4] git-p4: add support for large file systems larsxschneider
2015-09-03 20:03 ` Luke Diamand
2015-09-03 20:49 ` Lars Schneider
2015-09-04 8:58 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqio7ri40h.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=larsxschneider@gmail$(echo .)com \
--cc=luke@diamand$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox