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. > 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: parse-options does not recognize "unspecified" behavior
Quoting Pranit Bauva : On Sat, Mar 19, 2016 at 4:55 PM, SZEDER Gábor 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: parse-options does not recognize "unspecified" behavior
On Wed, Mar 16, 2016 at 02:53:17PM -0700, Stefan Beller wrote: > On Wed, Mar 16, 2016 at 2:44 PM, Jeff King 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: parse-options does not recognize "unspecified" behavior
On Wed, Mar 16, 2016 at 2:44 PM, Jeff King 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. > > -Peff Is there any command which needs more than one --no-verbose? (as an abuse to stacking --quiet multiple times)? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: parse-options does not recognize "unspecified" behavior
On Thu, Mar 17, 2016 at 01:32:41AM -0400, Eric Sunshine wrote: > On Wed, Mar 16, 2016 at 9:43 PM, Jeff King 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: parse-options does not recognize "unspecified" behavior
On Thu, Mar 17, 2016 at 2:53 AM, Jeff King wrote: > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: parse-options does not recognize "unspecified" behavior
On Wed, Mar 16, 2016 at 4:16 PM, Jeff King wrote:
> On Wed, Mar 16, 2016 at 02:53:17PM -0700, Stefan Beller wrote:
>
>> On Wed, Mar 16, 2016 at 2:44 PM, Jeff King 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 -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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: parse-options does not recognize "unspecified" behavior
On Wed, Mar 16, 2016 at 5:23 PM, Jeff King wrote: > 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= 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: parse-options does not recognize "unspecified" behavior
On Thu, Mar 17, 2016 at 3:07 AM, Eric Sunshine wrote: > The goal comes from his GSoC microproject. Specifically, Pranit wants > an "unspecified" value. The reason is that he is adding a > commit.verbose= 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: parse-options does not recognize "unspecified" behavior
On Sat, Mar 19, 2016 at 4:55 PM, SZEDER Gábor 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. Plus, this edit would not be required. What are your suggestions? Regards, Pranit Bauva -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: parse-options does not recognize "unspecified" behavior
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: parse-options does not recognize "unspecified" behavior
> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: parse-options does not recognize "unspecified" behavior
On Wed, Mar 16, 2016 at 9:43 PM, Jeff King 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: parse-options does not recognize "unspecified" behavior
On Thu, Mar 17, 2016 at 02:36:51AM +0530, Pranit Bauva wrote: > > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: parse-options does not recognize "unspecified" behavior
On Thu, Mar 17, 2016 at 2:19 AM, Jeff King wrote: > 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? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: parse-options does not recognize "unspecified" behavior
On Wed, Mar 16, 2016 at 04:33:03PM -0700, Stefan Beller wrote:
> 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).
> 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).
> In case of a command where this is tracked in one variable,
>
> git -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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
