Re: [PATCH v2] branch: reject --show-current with -v
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 22, 2026, 21:42 UTC
- Message-ID
- <xmqqwlz37d7g.fsf@gitster.g>
- In-Reply-To
- <bf5b0901-fc44-4fbc-b66d-5eb062ccd324@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
Show 19 quoted lines
> 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.
Show 13 quoted lines
>> 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.
Show 11 quoted lines
>> +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.