On Sun, Mar 27, 2016 at 8:15 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva <pranit.bauva@xxxxxxxxx> wrote:
>> parse-options.c: make OPTION__COUNTUP understand "unspecified" values
>
> A bit clearer: s/understand/respect/
> Also: s/__/_/
Sure.
>> The reason to make it understand negative values or more specifically
>> "unspecified" values is to give the ability to differentiate whether
>> `--option` or `--no-option` was specified at all.
>
> The word "negative" shows up far too early in this paragraph, even
> before "unspecified". It also fails utterly to explain what
> "unspecified" means and how to use it. It does vaguely explain why
> respecting "unspecified" is desirable (to know if --[no-]option was
> seen), but otherwise leaves the reader quite clueless.
>
>> Many uses of COUNTUP have now been replaced with BOOL and what remains
>> are verbose/quiet/force. This change will not affect existing users of
>> COUNTUP at all, as long as they use the initial value of 0 (or more), as
>> there is no mechanism to decrement. The only thing the command line can
>> do is to reset it to zero with "--no-foo".
>
> Copying this paragraph verbatim from Junio's email[1] isn't
> necessarily the best way to compose a commit message. Junio was
> "thinking out loud" while justifying the change to himself as being
> safe, but you ought to reframe this reasoning into a more concise form
> which flows with the rest of the commit message, keeping the important
> bits and dropping others.
I will drop off some initial parts.
>> Verbose or quiet don't use negative values before this commit but force
>> uses it in a different way to handle its config and then munges the "-1"
>> into 0 before we get to parse_options.
>
> Presumably, you're talking about a very specific instance of -1 in
> conjunction with OPT__FORCE in builtin/clean.c, but that information
> is entirely missing from this paragraph, thus this explanation serves
> only to confuse rather than clarify. It is a good idea to cite this
> specific unusual case when justifying that this change is safe, but it
> must be accompanied by sufficient context for the reader to understand
> what is being said.
I will mention about OPT__FORCE in builtin/clean.c clearly.
>> There are uses of COUNTUP in cmd_hash_object() and test-parse-options.c
>> and they are both fine.
>
> What does "fine" mean? I know from reading the code that "fine" means
> that these clients initialize the values to 0, thus should see no
> behavioral difference. But why are these two cases called out
> specially when they are already covered by the above "as long as they
> use initial value of 0" explanation? As a reader, having these two
> cases mentioned specially makes me wonder if I'm missing something
> non-obvious about them.
Its good to drop off this paragraph
> It would probably be better to avoid mentioning cmd_hash_object() and
> test-parse-options.c at all, and instead explain generally that, with
> one exception (builtin/clean.c), all current clients of OPTION_COUNTUP
> use an initial value of zero, thus are not impacted by this change.
> And, then go on to explain builtin/clean.c's special case and why it
> is safe, as well.
>
>
> Sorry for seeming to be very picky, but if I hadn't already been
> following this topic (and hadn't in fact suggested this particular
> change), as a reader, I think I would find this commit message utterly
> baffling, and wouldn't have a clue what this change is about or why it
> is desirable.
>
> Perhaps it would be a good idea to re-read "(2) Describe your changes
> well" in Documentation/SubmittingPatches for hints about writing a
> good commit message, as well as Junio's recent re-stating[2] of those
> hints.
I don't disagree to your point and it is a bit difficult for me to
explain stuff to other people especially when I know what's going on
but other might not.
> Try to write the commit message as if you were speaking to someone who
> wasn't already familiar with the issue you're trying to solve.
> Specifically, explain the problem ("no way to distinguish between
> --[no-]option being seen or not"); explain the solution ("introduce an
> 'unspecified' value"); the implementation of the "unspecified" value
> ("any negative number" plus an example of how to make use of it);
> justify that the change is safe ("existing clients of OPTION_COUNTUP
> are not impacted because ...").
I will try and do this in the commit message.
>> Signed-off-by: Pranit Bauva <pranit.bauva@xxxxxxxxx>
>> ---
>> The discussion about this patch:
>> [1] : http://thread.gmane.org/gmane.comp.version-control.git/289027
>>
>> Previous version of the patch:
>> [v1] : http://thread.gmane.org/gmane.comp.version-control.git/289061
>
> Unless I'm mistaken, the previous version was v9[3], not v1.
>
>> Changes introduced:
>> Use a different language in commit message to make the change and its
>> utility more clear. Also added some points as to where this patch could
>> break but it doesn't.
>
> This version forgets to add the new tests to t0040-parse-options.sh
> which SZEDER requested[4] to ensure that the new behavior works as
> expected.
>
>> ---
>> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
>> @@ -144,8 +144,12 @@ There are some macros to easily define options:
>>
>> `OPT_COUNTUP(short, long, &int_var, description)`::
>> Introduce a count-up option.
>> - `int_var` is incremented on each use of `--option`, and
>> - reset to zero with `--no-option`.
>> + Each use of `--option` increments `int_var`, starting from zero
>> + (even if initially negative), and `--no-option` resets it to
>> + zero. To determine if `--option` or `--no-option` was set at
>> + all, set `int_var` to a negative value, and if it is still
>
> I realize that you copied this text verbatim from the example I
> gave[5], but in retrospect, I think s/set/initialize/ would be a bit
> more clear:
>
> all, initialize `int_var` to a negative value, ...
>
>> + negative after parse_options(), then neither `--option` nor
>> + `--no-option` was seen.
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/289264/
> [2]: http://article.gmane.org/gmane.comp.version-control.git/289757/
> [3]: http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=289724
> [4]: http://article.gmane.org/gmane.comp.version-control.git/289733
> [5]: http://article.gmane.org/gmane.comp.version-control.git/289822
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html