threads / discuss / 41715

parse-options does not recognize "unspecified" behavior

Subject: parse-options does not recognize "unspecified" behavior

## tl;dr

18 messages between Mar 16, 2016 and Mar 25, 2016.

replies: 17people: 5as markdown or json

Pranit Bauva· Mar 16, 2016, 19:51 UTC · lore
Hey!

I noticed that parse-options does not recognize the variable which is set to -1 so as to denote the "unspecified" value.

I did the following changes in builtin/commit.c (in master branch not
the patch I am working on) :
 - static int verbose = -1
 - introduced a printf statement after parsing the options to print
the value of verbose.
When I ran `git commit` :
 I get the output that verbose is set to -1.

When I ran `git commit -v` : I get the output that verbose is set to 0.

When I ran `git commit -v -v` : I get the output that verbose is set to 1.

When I ran `git commit --no-verbose` : I get the out that verbose is set to 0.

Then I set verbose = -2.
And the output was obtained in a similar format.
 -2 with `git commit`
 -1 with `git commit -v`
  0 with `git commit -v -v`
  0 with `git commit --no-verbose`
  1 with `git commit -v -v -v`

It seems that parse-options just increments the value without considering the -1 flag to denote "unspecified value".

Is this a bug?
Jeff King· Mar 16, 2016, 20:49 UTC · re: Pranit Bauva · lore

Re: parse-options does not recognize "unspecified" behavior

On Thu, Mar 17, 2016 at 01:21:49AM +0530, Pranit Bauva wrote:
> I noticed that parse-options does not recognize the variable which is
> set to -1 so as to denote the "unspecified" value.

Right. Like all of the stock parse-options handlers, it does not ever read or understand the value passed to it by the caller. It only increments or decrements.

Show 22 quoted lines
> I did the following changes in builtin/commit.c (in master branch not
> the patch I am working on) :
>  - static int verbose = -1
>  - introduced a printf statement after parsing the options to print
> the value of verbose.
> 
> When I ran `git commit` :
>  I get the output that verbose is set to -1.
> 
> When I ran `git commit -v` :
> I get the output that verbose is set to 0.
> 
> When I ran `git commit -v -v` :
> I get the output that verbose is set to 1.
> 
> When I ran `git commit --no-verbose` :
> I get the out that verbose is set to 0.
> [...]
> It seems that parse-options just increments the value without
> considering the -1 flag to denote "unspecified value".
> 
> Is this a bug?

Not in parse-options, though I think setting verbose to "-1" in the first place is wrong.

In general, parse-options does not know or care about the default values that callers assign to variables; it just writes to them based on the option-type specified by the caller. So the behavior for "commit", "commit -v", and "commit -v -v" you show are perfectly reasonable.

But the one for "--no-verbose" is wrong. Parse-options has to write some "reset" value, and it does not know what the initial default was. So it writes 0. This is the same for options like OPT_SET_INT, and similar for string options (where we set it to NULL).

So I think the caller choosing "-1" here as the "not set" value is the bug.

-Peff
Pranit Bauva· Mar 16, 2016, 21:06 UTC · re: Jeff King · lore

Re: parse-options does not recognize "unspecified" behavior

On Thu, Mar 17, 2016 at 2:19 AM, Jeff King <peff@peff.net> wrote:
Show 49 quoted lines
> On Thu, Mar 17, 2016 at 01:21:49AM +0530, Pranit Bauva wrote:
>
>> I noticed that parse-options does not recognize the variable which is
>> set to -1 so as to denote the "unspecified" value.
>
> Right. Like all of the stock parse-options handlers, it does not ever
> read or understand the value passed to it by the caller. It only
> increments or decrements.
>
>> I did the following changes in builtin/commit.c (in master branch not
>> the patch I am working on) :
>>  - static int verbose = -1
>>  - introduced a printf statement after parsing the options to print
>> the value of verbose.
>>
>> When I ran `git commit` :
>>  I get the output that verbose is set to -1.
>>
>> When I ran `git commit -v` :
>> I get the output that verbose is set to 0.
>>
>> When I ran `git commit -v -v` :
>> I get the output that verbose is set to 1.
>>
>> When I ran `git commit --no-verbose` :
>> I get the out that verbose is set to 0.
>> [...]
>> It seems that parse-options just increments the value without
>> considering the -1 flag to denote "unspecified value".
>>
>> Is this a bug?
>
> Not in parse-options, though I think setting verbose to "-1" in the
> first place is wrong.
>
> In general, parse-options does not know or care about the default values
> that callers assign to variables; it just writes to them based on the
> option-type specified by the caller. So the behavior for "commit",
> "commit -v", and "commit -v -v" you show are perfectly reasonable.
>
> But the one for "--no-verbose" is wrong. Parse-options has to write some
> "reset" value, and it does not know what the initial default was. So it
> writes 0. This is the same for options like OPT_SET_INT, and similar for
> string options (where we set it to NULL).
>
> So I think the caller choosing "-1" here as the "not set" value is the
> bug.
>
> -Peff

I agree to you on the point that parse-options should not care about the value passed to it. But I think plainly incrementing the value of the variable is not a very nice way. I have an another approach to it. The parse-options will first store a temporary structure. If there is some changes (not the "--no-" ones) then it sets the respective variable in temporary structure to the set value. If "--no-" is passed then it writes the "reset" value to the respective variable in temporary structure. If nothing about that options is specified then it copies the respective variable from original to temporary. After completing the entire process, it can copy temporary structure to the original structure.

What are your opinions about this?
Jeff King· Mar 16, 2016, 21:23 UTC · re: Pranit Bauva · lore

Re: parse-options does not recognize "unspecified" behavior

On Thu, Mar 17, 2016 at 02:36:51AM +0530, Pranit Bauva wrote:
Show 18 quoted lines
> > So I think the caller choosing "-1" here as the "not set" value is the
> > bug.
> >
> > -Peff
> 
> I agree to you on the point that parse-options should not care about
> the value passed to it. But I think plainly incrementing the value of
> the variable is not a very nice way. I have an another approach to it.
> The parse-options will first store a temporary structure. If there is
> some changes (not the "--no-" ones) then it sets the respective
> variable in temporary structure to the set value. If "--no-" is passed
> then it writes the "reset" value to the respective variable in
> temporary structure. If nothing about that options is specified then
> it copies the respective variable from original to temporary. After
> completing the entire process, it can copy temporary structure to the
> original structure.
> 
> What are your opinions about this?

I don't think that would produce the wrong behavior, but it seems like a very complicated solution to a problem that can easily be solved by just following the usual conventions (that verbose starts at 0, options make it go up or down, and "--no-" resets it to zero).

Perhaps it would make more sense if I understood what your goal was in setting verbose to -1 in the first place.

-Peff
Eric Sunshine· Mar 16, 2016, 21:37 UTC · re: Jeff King · lore

Re: parse-options does not recognize "unspecified" behavior

On Wed, Mar 16, 2016 at 5:23 PM, Jeff King <peff@peff.net> wrote:
Show 19 quoted lines
> On Thu, Mar 17, 2016 at 02:36:51AM +0530, Pranit Bauva wrote:
>> I agree to you on the point that parse-options should not care about
>> the value passed to it. But I think plainly incrementing the value of
>> the variable is not a very nice way. I have an another approach to it.
>> The parse-options will first store a temporary structure. If there is
>> some changes (not the "--no-" ones) then it sets the respective
>> variable in temporary structure to the set value. If "--no-" is passed
>> then it writes the "reset" value to the respective variable in
>> temporary structure. If nothing about that options is specified then
>> it copies the respective variable from original to temporary. After
>> completing the entire process, it can copy temporary structure to the
>> original structure.
>>
>> What are your opinions about this?
>
> I don't think that would produce the wrong behavior, but it seems like a
> very complicated solution to a problem that can easily be solved by just
> following the usual conventions (that verbose starts at 0, options make
> it go up or down, and "--no-" resets it to zero).
I agree that this is overly complicated.
> Perhaps it would make more sense if I understood what your goal was in
> setting verbose to -1 in the first place.

The goal comes from his GSoC microproject. Specifically, Pranit wants an "unspecified" value. The reason is that he is adding a commit.verbose=<level> config variable to back the existing git-commit --verbose option. Any use of --verbose (one or more times) or --no-verbose should override the config.verbose value altogether, so he wants a way to know if --verbose or --no-verbose was used; hence the "unspecified" value. And, really, this issue isn't necessarily specific to git-commit. It could apply to any command that understands verbosity levels and wants to be able to get them from both a config variable and a command-line option.

A much easier solution would be to update OPT_VERBOSE() to understand that negative values are "unspecified", and then --verbose would (pseudocode):

    if (value < 0)
        value = 0
    value++;
and --no-verbose would:
    value = 0

That should be compatible with existing clients of OPT__VERBOSE() which initialize the value to 0, and should satisfy Pranit's case; he can initialize it to -1, and if it is still -1 when option parsing is done, then he knows that neither --verbose nor --no-verbose was seen.

Pranit Bauva· Mar 16, 2016, 21:41 UTC · re: Eric Sunshine · lore

Re: parse-options does not recognize "unspecified" behavior

On Thu, Mar 17, 2016 at 3:07 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 27 quoted lines
> The goal comes from his GSoC microproject. Specifically, Pranit wants
> an "unspecified" value. The reason is that he is adding a
> commit.verbose=<level> config variable to back the existing git-commit
> --verbose option. Any use of --verbose (one or more times) or
> --no-verbose should override the config.verbose value altogether, so
> he wants a way to know if --verbose or --no-verbose was used; hence
> the "unspecified" value. And, really, this issue isn't necessarily
> specific to git-commit. It could apply to any command that understands
> verbosity levels and wants to be able to get them from both a config
> variable and a command-line option.
>
> A much easier solution would be to update OPT_VERBOSE() to understand
> that negative values are "unspecified", and then --verbose would
> (pseudocode):
>
>     if (value < 0)
>         value = 0
>     value++;
>
> and --no-verbose would:
>
>     value = 0
>
> That should be compatible with existing clients of OPT__VERBOSE()
> which initialize the value to 0, and should satisfy Pranit's case; he
> can initialize it to -1, and if it is still -1 when option parsing is
> done, then he knows that neither --verbose nor --no-verbose was seen.

This is a much easier solution. I didn't think of this. This type of problem can only arise with only verbose, so it would be better to specifically change that part of code.

Jeff King· Mar 16, 2016, 21:44 UTC · re: Eric Sunshine · lore

Re: parse-options does not recognize "unspecified" behavior

On Wed, Mar 16, 2016 at 05:37:03PM -0400, Eric Sunshine wrote:
Show 16 quoted lines
> A much easier solution would be to update OPT_VERBOSE() to understand
> that negative values are "unspecified", and then --verbose would
> (pseudocode):
> 
>     if (value < 0)
>         value = 0
>     value++;
> 
> and --no-verbose would:
> 
>     value = 0
> 
> That should be compatible with existing clients of OPT__VERBOSE()
> which initialize the value to 0, and should satisfy Pranit's case; he
> can initialize it to -1, and if it is still -1 when option parsing is
> done, then he knows that neither --verbose nor --no-verbose was seen.
Yes, that makes much more sense to me. Thanks for the back-story.
-Peff
Stefan Beller· Mar 16, 2016, 21:53 UTC · re: Jeff King · lore

Re: parse-options does not recognize "unspecified" behavior

On Wed, Mar 16, 2016 at 2:44 PM, Jeff King <peff@peff.net> wrote:
Show 22 quoted lines
> On Wed, Mar 16, 2016 at 05:37:03PM -0400, Eric Sunshine wrote:
>
>> A much easier solution would be to update OPT_VERBOSE() to understand
>> that negative values are "unspecified", and then --verbose would
>> (pseudocode):
>>
>>     if (value < 0)
>>         value = 0
>>     value++;
>>
>> and --no-verbose would:
>>
>>     value = 0
>>
>> That should be compatible with existing clients of OPT__VERBOSE()
>> which initialize the value to 0, and should satisfy Pranit's case; he
>> can initialize it to -1, and if it is still -1 when option parsing is
>> done, then he knows that neither --verbose nor --no-verbose was seen.
>
> Yes, that makes much more sense to me. Thanks for the back-story.
>
> -Peff

Is there any command which needs more than one --no-verbose? (as an abuse to stacking --quiet multiple times)?

Jeff King· Mar 16, 2016, 23:16 UTC · re: Stefan Beller · lore

Re: parse-options does not recognize "unspecified" behavior

On Wed, Mar 16, 2016 at 02:53:17PM -0700, Stefan Beller wrote:
Show 24 quoted lines
> On Wed, Mar 16, 2016 at 2:44 PM, Jeff King <peff@peff.net> wrote:
> > On Wed, Mar 16, 2016 at 05:37:03PM -0400, Eric Sunshine wrote:
> >
> >> A much easier solution would be to update OPT_VERBOSE() to understand
> >> that negative values are "unspecified", and then --verbose would
> >> (pseudocode):
> >>
> >>     if (value < 0)
> >>         value = 0
> >>     value++;
> >>
> >> and --no-verbose would:
> >>
> >>     value = 0
> >>
> >> That should be compatible with existing clients of OPT__VERBOSE()
> >> which initialize the value to 0, and should satisfy Pranit's case; he
> >> can initialize it to -1, and if it is still -1 when option parsing is
> >> done, then he knows that neither --verbose nor --no-verbose was seen.
> >
> > Yes, that makes much more sense to me. Thanks for the back-story.
> 
> Is there any command which needs more than one --no-verbose?
> (as an abuse to stacking --quiet multiple times)?

I'm not sure I understand. "--no-verbose" is just about resetting the value. So you might get it multiple times in:

   git commit -v --no-verbose -v --no-verbose

but the caller would not care. Which makes me think I'm misunderstanding your question.

You also mention "--quiet", but that is not handled by OPT__VERBOSE, but rather by OPT__QUIET. And there I do not think the "-1 is undefined" trick works as well there, because presumably "-1" is the same as one "--quiet".

-Peff
Stefan Beller· Mar 16, 2016, 23:33 UTC · re: Jeff King · lore

Re: parse-options does not recognize "unspecified" behavior

On Wed, Mar 16, 2016 at 4:16 PM, Jeff King <peff@peff.net> wrote:
Show 39 quoted lines
> On Wed, Mar 16, 2016 at 02:53:17PM -0700, Stefan Beller wrote:
>
>> On Wed, Mar 16, 2016 at 2:44 PM, Jeff King <peff@peff.net> wrote:
>> > On Wed, Mar 16, 2016 at 05:37:03PM -0400, Eric Sunshine wrote:
>> >
>> >> A much easier solution would be to update OPT_VERBOSE() to understand
>> >> that negative values are "unspecified", and then --verbose would
>> >> (pseudocode):
>> >>
>> >>     if (value < 0)
>> >>         value = 0
>> >>     value++;
>> >>
>> >> and --no-verbose would:
>> >>
>> >>     value = 0
>> >>
>> >> That should be compatible with existing clients of OPT__VERBOSE()
>> >> which initialize the value to 0, and should satisfy Pranit's case; he
>> >> can initialize it to -1, and if it is still -1 when option parsing is
>> >> done, then he knows that neither --verbose nor --no-verbose was seen.
>> >
>> > Yes, that makes much more sense to me. Thanks for the back-story.
>>
>> Is there any command which needs more than one --no-verbose?
>> (as an abuse to stacking --quiet multiple times)?
>
> I'm not sure I understand. "--no-verbose" is just about resetting the
> value. So you might get it multiple times in:
>
>    git commit -v --no-verbose -v --no-verbose
>
> but the caller would not care. Which makes me think I'm misunderstanding
> your question.
>
> You also mention "--quiet", but that is not handled by OPT__VERBOSE, but
> rather by OPT__QUIET. And there I do not think the "-1 is undefined"
> trick works as well there, because presumably "-1" is the same as one
> "--quiet".
The way I understand verbosity is this:
* You can instruct a command to be more verbose if it is supported by
adding more -v
* --no-verbose tells the command to be not verbose, i.e. no additional output.

So my question was, if there is any command, which is verbose by default, and --no-verbose would make it "more quiet"? Such a case would be a UX bug, as a user would rather expect --quiet instead of --no-verbose IMO. Would such a case ever happen, that we'd want to give --no-verbose to decrease the amount said by the command?

IIRC some commands use one integer variable to determine the amount of output, i.e. --verbose increases that variable, --quiet decreases it. What happens for example with

  git commit -v --no-verbose -v -q -q --no-quiet

In case of commit, the quietness and verbosity is done in 2 variables, so these are orthogonal to each other, there are even no internal checks for (verbosity > 0 && quietness > 0) at the same time, so it seems to be a valid command.

In case of a command where this is tracked in one variable,
  git <foo> -v --no-verbose -v -q -q --no-quiet
you'd expect:
  verbosity++ // because of first -v
  verbosity = 0 // because of the reset with --no-verbose
  verbosity++ // because of second -v
  verbosity-- // because of first -q
  verbosity-- // because of second -q
  verbosity = 0 // because of the reset with --no-quiet

Having typed that, I think my comment was not adding value to the discussion, as --no-{verbose/quiet} would just reset it to 0 no matter if you track verbosity/quietness in one or two variables internally.

Stefan
Jeff King· Mar 17, 2016, 01:43 UTC · re: Stefan Beller · lore

Re: parse-options does not recognize "unspecified" behavior

On Wed, Mar 16, 2016 at 04:33:03PM -0700, Stefan Beller wrote:
Show 10 quoted lines
> The way I understand verbosity is this:
> * You can instruct a command to be more verbose if it is supported by
> adding more -v
> * --no-verbose tells the command to be not verbose, i.e. no additional output.
> 
> So my question was, if there is any command, which is verbose by
> default, and --no-verbose would make it "more quiet"? Such a case
> would be a UX bug, as a user would rather expect --quiet instead of
> --no-verbose IMO. Would such a case ever happen, that we'd want to
> give --no-verbose to decrease the amount said by the command?

Ah, I see. I agree that would be a bug, because --no-verbose is not "more quiet". It is "cancel all previous -v". The right way to spell that is "--quiet" (usually, see below).

Show 11 quoted lines
> IIRC some commands use one integer variable to determine
> the amount of output, i.e. --verbose increases that variable, --quiet
> decreases it.
> What happens for example with
> 
>   git commit -v --no-verbose -v -q -q --no-quiet
> 
> In case of commit, the quietness and verbosity is done in 2 variables,
> so these are orthogonal to each other, there are even no internal checks for
> (verbosity > 0 && quietness > 0) at the same time, so it seems to be a valid
> command.

Yes, I think in general, "-v" and "-q" should work as opposites. But that is not the case with commit, where "-v" and "-q" operate on totally separate messages. I think that is a UX mistake, and we would not do it that way if designing from scratch. But we're stuck with it for historical reasons (I'd probably name "--verbose" as "--show-diff" or something if writing it today).

Arguably cmd_commit() should be using OPT_BOOL instead of OPT__VERBOSE, as there is no such thing as "verbose > 1" here. But I don't think there is any real user-facing consequence of that (however, given Eric's suggestion, I suspect it would make Pranit's problem just go away, as it assigns rather than increments; IOW, it does the thing Eric was suggestion OPT__VERBOSE to do).

Show 16 quoted lines
> In case of a command where this is tracked in one variable,
> 
>   git <foo> -v --no-verbose -v -q -q --no-quiet
> 
> you'd expect:
> 
>   verbosity++ // because of first -v
>   verbosity = 0 // because of the reset with --no-verbose
>   verbosity++ // because of second -v
>   verbosity-- // because of first -q
>   verbosity-- // because of second -q
>   verbosity = 0 // because of the reset with --no-quiet
> 
> Having typed that, I think my comment was not adding value to
> the discussion, as --no-{verbose/quiet} would just reset it to 0 no matter
> if you track verbosity/quietness in one or two variables internally.

Right, in a command using OPT_VERBOSITY(), that is how it should (and does) work. I think "commit" is just funny for historical reasons.

-Peff
Eric Sunshine· Mar 17, 2016, 05:32 UTC · re: Jeff King · lore

Re: parse-options does not recognize "unspecified" behavior

On Wed, Mar 16, 2016 at 9:43 PM, Jeff King <peff@peff.net> wrote:
Show 6 quoted lines
> Arguably cmd_commit() should be using OPT_BOOL instead of OPT__VERBOSE,
> as there is no such thing as "verbose > 1" here. But I don't think there
> is any real user-facing consequence of that (however, given Eric's
> suggestion, I suspect it would make Pranit's problem just go away, as it
> assigns rather than increments; IOW, it does the thing Eric was
> suggestion OPT__VERBOSE to do).

Actually, Pranit's previous version of the patch did treat verbosity as a boolean, but then SZEDER pointed out this bit from git-commit.txt:

    --verbose::
        ...
        If specified twice, show in addition the unified diff between
        what would be committed and the worktree files, i.e. the unstaged
        changes to tracked files.

which is what led us to the current discussion about wanting an "unspecified" value for OPT__VERBOSE.

Jeff King· Mar 17, 2016, 05:40 UTC · re: Eric Sunshine · lore

Re: parse-options does not recognize "unspecified" behavior

On Thu, Mar 17, 2016 at 01:32:41AM -0400, Eric Sunshine wrote:
Show 20 quoted lines
> On Wed, Mar 16, 2016 at 9:43 PM, Jeff King <peff@peff.net> wrote:
> > Arguably cmd_commit() should be using OPT_BOOL instead of OPT__VERBOSE,
> > as there is no such thing as "verbose > 1" here. But I don't think there
> > is any real user-facing consequence of that (however, given Eric's
> > suggestion, I suspect it would make Pranit's problem just go away, as it
> > assigns rather than increments; IOW, it does the thing Eric was
> > suggestion OPT__VERBOSE to do).
> 
> Actually, Pranit's previous version of the patch did treat verbosity
> as a boolean, but then SZEDER pointed out this bit from
> git-commit.txt:
> 
>     --verbose::
>         ...
>         If specified twice, show in addition the unified diff between
>         what would be committed and the worktree files, i.e. the unstaged
>         changes to tracked files.
> 
> which is what led us to the current discussion about wanting an
> "unspecified" value for OPT__VERBOSE.

Ah, thanks. I looked for something like that in builtin/commit.c and didn't see us using verbose as anything but a boolean. But we pass it into wt_status, which does look at "s->verbose > 1".

Sorry for the noise (and probably I should stop participating in this discussion without having read all of the backstory!).

-Peff
SZEDER Gábor· Mar 19, 2016, 11:25 UTC · re: Jeff King · lore

Re: parse-options does not recognize "unspecified" behavior

Show 6 quoted lines
> Yes, I think in general, "-v" and "-q" should work as opposites. But
> that is not the case with commit, where "-v" and "-q" operate on totally
> separate messages. I think that is a UX mistake, and we would not do
> it that way if designing from scratch. But we're stuck with it for
> historical reasons (I'd probably name "--verbose" as "--show-diff" or
> something if writing it today).

Yeah, as a long-time 'git commit -v' user I never really thought about the name of the option, but when I pointed out the multiple verbose levels it struck me as a rather unfortunate name for this feature, too. Oh, well, we are stuck with it indeed.

However, that doesn't mean that we have to spread this badly chosen name from options to config variables, does it? I think that if we are going to define a new config variable today, then it should be named properly, and it's better not to call it 'commit.verbose', but 'commit.showDiff' or something. Perhaps we could even define two new config variables: 'commit.showDiff' for the diff of the changes to be committed (= '-v'), and 'commit.showUnstagedDiff' for the - wait for it! - unstaged changes (= '-v -v'). Not sure about the variable names, though, because "plain" 'git diff' shows unstaged changes, while 'git diff --cached' shows staged changes.

Furthermore, it doesn't mean that we can't add properly named command line option(s) and state that '-v|--verbose' is a synonym to '--show-diff' (maybe even deprecate '--verbose'), but I don't want to squeeze even more into a GSOC micro project.

Gábor
Pranit Bauva· Mar 19, 2016, 16:55 UTC · re: SZEDER Gábor · lore

Re: parse-options does not recognize "unspecified" behavior

On Sat, Mar 19, 2016 at 4:55 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
Show 22 quoted lines
>> Yes, I think in general, "-v" and "-q" should work as opposites. But
>> that is not the case with commit, where "-v" and "-q" operate on totally
>> separate messages. I think that is a UX mistake, and we would not do
>> it that way if designing from scratch. But we're stuck with it for
>> historical reasons (I'd probably name "--verbose" as "--show-diff" or
>> something if writing it today).
>
> Yeah, as a long-time 'git commit -v' user I never really thought about
> the name of the option, but when I pointed out the multiple verbose
> levels it struck me as a rather unfortunate name for this feature,
> too.  Oh, well, we are stuck with it indeed.
>
> However, that doesn't mean that we have to spread this badly chosen
> name from options to config variables, does it?  I think that if we
> are going to define a new config variable today, then it should be
> named properly, and it's better not to call it 'commit.verbose', but
> 'commit.showDiff' or something.  Perhaps we could even define two new
> config variables: 'commit.showDiff' for the diff of the changes to be
> committed (= '-v'), and 'commit.showUnstagedDiff' for the - wait for
> it! - unstaged changes (= '-v -v').  Not sure about the variable
> names, though, because "plain" 'git diff' shows unstaged changes,
> while 'git diff --cached' shows staged changes.
This seems a better way to go about.
Show 5 quoted lines
>
> Furthermore, it doesn't mean that we can't add properly named command
> line option(s) and state that '-v|--verbose' is a synonym to
> '--show-diff' (maybe even deprecate '--verbose'), but I don't want to
> squeeze even more into a GSOC micro project.

Its perfectly fine. It hardly a half an hour job. Though I like the idea that we should use both "-v|--verbose" and "--show-diff" instead of deprecating it. Plus, this edit would not be required. What are your suggestions?

Regards, Pranit Bauva

SZEDER Gábor· Mar 25, 2016, 14:58 UTC · re: Pranit Bauva · lore

Re: parse-options does not recognize "unspecified" behavior

Quoting Pranit Bauva <pranit.bauva@gmail.com>:
Show 35 quoted lines
> On Sat, Mar 19, 2016 at 4:55 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>>> Yes, I think in general, "-v" and "-q" should work as opposites. But
>>> that is not the case with commit, where "-v" and "-q" operate on totally
>>> separate messages. I think that is a UX mistake, and we would not do
>>> it that way if designing from scratch. But we're stuck with it for
>>> historical reasons (I'd probably name "--verbose" as "--show-diff" or
>>> something if writing it today).
>>
>> Yeah, as a long-time 'git commit -v' user I never really thought about
>> the name of the option, but when I pointed out the multiple verbose
>> levels it struck me as a rather unfortunate name for this feature,
>> too.  Oh, well, we are stuck with it indeed.
>>
>> However, that doesn't mean that we have to spread this badly chosen
>> name from options to config variables, does it?  I think that if we
>> are going to define a new config variable today, then it should be
>> named properly, and it's better not to call it 'commit.verbose', but
>> 'commit.showDiff' or something.  Perhaps we could even define two new
>> config variables: 'commit.showDiff' for the diff of the changes to be
>> committed (= '-v'), and 'commit.showUnstagedDiff' for the - wait for
>> it! - unstaged changes (= '-v -v').  Not sure about the variable
>> names, though, because "plain" 'git diff' shows unstaged changes,
>> while 'git diff --cached' shows staged changes.
>
> This seems a better way to go about.
>
>>
>> Furthermore, it doesn't mean that we can't add properly named command
>> line option(s) and state that '-v|--verbose' is a synonym to
>> '--show-diff' (maybe even deprecate '--verbose'), but I don't want to
>> squeeze even more into a GSOC micro project.
>
> Its perfectly fine. It hardly a half an hour job. Though I like the
> idea that we should use both "-v|--verbose" and "--show-diff" instead
> of deprecating it.

Deprecating doesn't mean "removing". It merely means that we add a note to the documentation stating that the option in question is deprecated, but we will keep supporting it for several years and releases to come.

This means that '--verbose' and '--show-diff' must coexist for quite a while, and 'git commit' should do the right thing even if both old and new options are combined. And, of course, in combination with the new config variable(s). I suspect that all this would be more than a half an hour job.

Pranit Bauva· Mar 25, 2016, 15:03 UTC · re: SZEDER Gábor · lore

Re: parse-options does not recognize "unspecified" behavior

> Deprecating doesn't mean "removing".  It merely means that we add a note
> to the documentation stating that the option in question is deprecated,
> but we will keep supporting it for several years and releases to come.
Okay. Didn't know.
Show 5 quoted lines
> This means that '--verbose' and '--show-diff' must coexist for quite a
> while, and 'git commit' should do the right thing even if both old and
> new options are combined.  And, of course, in combination with the new
> config variable(s).  I suspect that all this would be more than a half
> an hour job.

Now that I think of it again, I underestimated this. It will take a lot more time and create a lot of confusion on my side.

Pranit Bauva· Mar 16, 2016, 21:37 UTC · re: Jeff King · lore

Re: parse-options does not recognize "unspecified" behavior

On Thu, Mar 17, 2016 at 2:53 AM, Jeff King <peff@peff.net> wrote:
Show 9 quoted lines
> I don't think that would produce the wrong behavior, but it seems like a
> very complicated solution to a problem that can easily be solved by just
> following the usual conventions (that verbose starts at 0, options make
> it go up or down, and "--no-" resets it to zero).
>
> Perhaps it would make more sense if I understood what your goal was in
> setting verbose to -1 in the first place.
>
> -Peff

I am working on the micro project "configuration for commonly used command line options like "git commit -v". I sent out a final patch without considering the multiple verbosity behavior of `git commit`. To take consideration for that behavior, Junio suggested this approach [1]. I was trying to write code for this when I noticed this behavior. I did a `git grep " unspecified " just to find that using -1 is not that common. I think this problem can also arise in future if some more options are included. The question still remains whether its worth all this effort?

[1] : http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=288909

← back to recent threads