Re: [PATCH v2] branch: reject --show-current with -v

[Junio C Hamano <gitster@pobox.com>]

Phillip Wood <phillip.wood123@gmail.com> writes:

> On 22/03/2026 06:07, K Jayatheerth wrote:
>> The --show-current option doesn't know how to handle verbose
>> logic. In such a case, we want the program to die when both flags
>> are used together.
>
> Is there any reason why --show-current couldn't be made to work with 
> --verbose instead of dying?
>
>> 
>> Acked-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
>> ---
>> I only changed the title of the commit message.
>> The previous one (i.e remove unnecessary verbose flag) sounded
>> like we are nuking the whole verbose flag.
>> 
>> The acked by tag exists here because this was supposed to be queued,
>
> That's not what the Acked-by: tag means - you should remove it

I was about to write "Yes, I do not even recall seeing this patch",
noticed the "v2" label, but couldn't find corresponding v1.  If the
message had "In-reply-to: <xmqqldiplvyd.fsf@gitster.g>", that would
not have happened.  FWIW, Sergey, who gave the "why" to start the
thread, deserves much more credit than I would, I would have to say.

I did say "This is certainly an improvement over status quo.", and
"is good enough for now", so it is not completely unwarranted to
have my Ack there.  Without any context it is hard to see, though.

I _think_ what happened was later, after that "Ack" was given,
Sergey brought up a good point, "why not consider what should -v
mean in the context of --show-current before proceeding", which is
more or less the same as your point above, so either I did not pick
up the patch right there (expecting some form of response), or
picked it up but discarded (after seeing no response came), or
something like that.

Since there are two people who independently questioned the wisdom
of erroring out on "-v" before thinking things through to see if we
can come up with a good behaviour for "--show-current -v", perhaps
we should do so before proceeding.

So, I am not picking this iteration up, at least not yet.

>> diff --git a/builtin/branch.c b/builtin/branch.c
>> index a1a43380d0..cab22e1538 100644
>> --- a/builtin/branch.c
>> +++ b/builtin/branch.c
>> @@ -861,6 +861,8 @@ int cmd_branch(int argc,
>>   		ret = delete_branches(argc, argv, delete > 1, filter.kind, quiet);
>>   		goto out;
>>   	} else if (show_current) {
>> +		if (filter.verbose)
>> +			die(_("options '%s' and '%s' cannot be used together"), "--show-current", "-v");
>
> It would be better to use die_for_incompatible_opt2() here so that the 
> message is consistent with other commands.

Yes.  That would be much better.

>> +test_expect_success 'git branch --show-current rejects -v' '
>> +	test_must_fail git branch --show-current -v
>
> This checks that the command fails but does not check _why_ in failed. 
> It would be better to redirect stderr to a file and use test_grep to 
> check the error message matches what we expect as well checking that the 
> command fails.
>
> Thanks
>
> Phillip

Good suggestion.