Re: [PATCH 1/3] describe: debug is incompatible with contains

From: Michael J Gruber <git@drmicha.warpmail.net>
Date: Mar 17, 2017, 10:54 UTC
Message-ID: <a76bd1ae-74a8-a85c-852a-e36514cd05d5@drmicha.warpmail.net>
In-Reply-To: <xmqqzigme474.fsf@gitster.mtv.corp.google.com>

Junio C Hamano venit, vidit, dixit 15.03.2017 20:21:

Show 12 quoted lines

> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> `git describe --contains` calls into `git name-rev` which does not have
>> any searching to do and thus does not display any debug information.
>>
>> Say so in the documentation and catch the incompatible arguments.
> 
> I am not sure if this is worth it.  Those who are really doing the
> debugging would be staring at the code while running it anyway, so
> it is not like this new error condition would help anybody from
> wasting time scratching her head before viewing the source and
> realize that the underlying name-rev does not honor the option.

The story was: I tried to understand why git describe --contains did not do what I expected. The documentation said that --debug would output candidates, but it did not do anything.

So, instead of learning how git describe --contains works, I got side-tracked into understanding why git describe --contains does not do what the documentation says. That was a waste of time that we can avoid.

"viewing the source" should not be necessary to understand what is going on, should it?

Show 8 quoted lines

> If "--debug" is truly valuable, "name-rev" can gain "--debug" later
> and then we can pass it down if we want.
> 
> Also, it is not like "--debug" is incompatible.  It is just the
> "--contains" codepath is overly silent and does not give any useful
> information when run in the debug mode.  "incompatible" is more like
> "would not work correctly when both are given", which is not the
> case here.

Well, thee notion of giving debug output is certainly not incompatible. But if the "--debug" does not output anything with "--contains" then it is not working, which I would call incompatible (implementation, not concept).

Show 33 quoted lines

>>
>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>> ---
>>  Documentation/git-describe.txt | 2 +-
>>  builtin/describe.c             | 2 ++
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
>> index 8755f3af7b..0f9adb6e9a 100644
>> --- a/Documentation/git-describe.txt
>> +++ b/Documentation/git-describe.txt
>> @@ -69,7 +69,7 @@ OPTIONS
>>  --debug::
>>  	Verbosely display information about the searching strategy
>>  	being employed to standard error.  The tag name will still
>> -	be printed to standard out.
>> +	be printed to standard out. This is incompatible with --contains.
>>  
>>  --long::
>>  	Always output the long format (the tag, the number of commits
>> diff --git a/builtin/describe.c b/builtin/describe.c
>> index 76c18059bf..01a6d159a0 100644
>> --- a/builtin/describe.c
>> +++ b/builtin/describe.c
>> @@ -462,6 +462,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
>>  
>>  	if (longformat && abbrev == 0)
>>  		die(_("--long is incompatible with --abbrev=0"));
>> +	if (contains && debug)
>> +		die(_("--debug is incompatible with --contains"));
>>  
>>  	if (contains) {
>>  		struct string_list_item *item;