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/__/_/
> 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.
> 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.
> 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.
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.
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 ...").
> 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