[BUG] `git branch --contains ID name` creates branch "name"

[BUG] `git branch --contains ID name` creates branch "name"

[Peter Wu]

Hi,

I was trying to check whether a certain branch contained a commit and ran:

     git branch --contains ddc150f7a33ae0c9cb16eaac3641abc00f56316f master

This resulted in:

    fatal: A branch named 'master' already exists.
When "name" does not exist, this command creates a branch. I expect this 
command to search the mentioned branch, not trying to create it. The manual 
page of git-branch(1) does not mention such special behavior either.

Git version 1.8.1.2

Regards,
Peter

[PATCH 0/2] improve "git branch --contains=<commit> <pattern>"

[Jeff King]

On Wed, Jan 30, 2013 at 07:57:03PM +0100, Peter Wu wrote:

> Hi,
> 
> I was trying to check whether a certain branch contained a commit and ran:
> 
>      git branch --contains ddc150f7a33ae0c9cb16eaac3641abc00f56316f master
> 
> This resulted in:
> 
>     fatal: A branch named 'master' already exists.
> When "name" does not exist, this command creates a branch. I expect this 
> command to search the mentioned branch, not trying to create it. The manual 
> page of git-branch(1) does not mention such special behavior either.

Yeah, it's sort-of a bug. It is a syntactic ambiguity that an argument
to git-branch could be a listing pattern or a new branch name. When
using a listing pattern, you need to explicitly specify that you want
list mode with `--list`. This is documented in git-branch under --list,
but it should be more prominent, in the section that covers the various
invocation modes. The first patch below fixes that.

That being said, we could be much more helpful. It seems like --contains
should imply listing mode, since it is nonsensical in other modes. The
second patch below adjusts that, and makes the command above do what you
expect.

  [1/2]: docs: clarify git-branch --list behavior
  [2/2]: branch: let branch filters imply --list

-Peff

[PATCH 1/2] docs: clarify git-branch --list behavior

[Jeff King]

It was not clear from the "description" section of
git-branch(1) that using a <pattern> meant that you _had_ to
use the --list option. Let's clarify that, and while we're
at it, reword some clunky and ambiguous sentences.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-branch.txt | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 45a225e..01aa87f 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -22,13 +22,15 @@ DESCRIPTION
 DESCRIPTION
 -----------
 
-With no arguments, existing branches are listed and the current branch will
-be highlighted with an asterisk.  Option `-r` causes the remote-tracking
-branches to be listed, and option `-a` shows both. This list mode is also
-activated by the `--list` option (see below).
-<pattern> restricts the output to matching branches, the pattern is a shell
-wildcard (i.e., matched using fnmatch(3)).
-Multiple patterns may be given; if any of them matches, the branch is shown.
+If `--list` is given, or if there are no non-option arguments, existing
+branches are listed; the current branch will be highlighted with an
+asterisk.  Option `-r` causes the remote-tracking branches to be listed,
+and option `-a` shows both local and remote branches. If a `<pattern>`
+is given, it is used as a shell wildcard to restrict the output to
+matching branches. If multiple patterns are given, a branch is shown if
+any it is matched by any of the patterns.  Note that when providing a
+`<pattern>`, you must use `--list`; otherwise the command is interpreted
+as branch creation.
 
 With `--contains`, shows only the branches that contain the named commit
 (in other words, the branches whose tip commits are descendants of the
-- 
1.8.1.2.5.g1cb3f73

[PATCH 2/2] branch: let branch filters imply --list

[Jeff King]

Currently, a branch filter like `--contains`, `--merged`, or
`--no-merged` is ignored when we are not in listing mode.
For example:

  git branch --contains=foo bar

will create the branch "bar" from the current HEAD, ignoring
the `--contains` argument entirely. This is not very
helpful. There are two reasonable behaviors for git here:

  1. Flag an error; the arguments do not make sense.

  2. Implicitly go into `--list` mode

This patch chooses the latter, as it is more convenient, and
there should not be any ambiguity with attempting to create
a branch; using `--contains` and not wanting to list is
nonsensical.

That leaves the case where an explicit modification option
like `-d` is given.  We already catch the case where
`--list` is given alongside `-d` and flag an error. With
this patch, we will also catch the use of `--contains` and
other filter options alongside `-d`.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-branch.txt |  6 +++---
 builtin/branch.c             |  3 +++
 t/t3201-branch-contains.sh   | 35 +++++++++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 01aa87f..07ef5af 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -195,15 +195,15 @@ start-point is either a local or remote-tracking branch.
 
 --contains [<commit>]::
 	Only list branches which contain the specified commit (HEAD
-	if not specified).
+	if not specified). Implies `--list`.
 
 --merged [<commit>]::
 	Only list branches whose tips are reachable from the
-	specified commit (HEAD if not specified).
+	specified commit (HEAD if not specified). Implies `--list`.
 
 --no-merged [<commit>]::
 	Only list branches whose tips are not reachable from the
-	specified commit (HEAD if not specified).
+	specified commit (HEAD if not specified). Implies `--list`.
 
 <branchname>::
 	The name of the branch to create or delete.
diff --git a/builtin/branch.c b/builtin/branch.c
index 873f624..4aa3d4e 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -825,6 +825,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0)
 		list = 1;
 
+	if (with_commit || merge_filter != NO_FILTER)
+		list = 1;
+
 	if (!!delete + !!rename + !!force_create + !!list + !!new_upstream + !!unset_upstream > 1)
 		usage_with_options(builtin_branch_usage, options);
 
diff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh
index f86f4bc..141b061 100755
--- a/t/t3201-branch-contains.sh
+++ b/t/t3201-branch-contains.sh
@@ -55,6 +55,16 @@ test_expect_success 'branch --contains=side' '
 
 '
 
+test_expect_success 'branch --contains with pattern implies --list' '
+
+	git branch --contains=master master >actual &&
+	{
+		echo "  master"
+	} >expect &&
+	test_cmp expect actual
+
+'
+
 test_expect_success 'side: branch --merged' '
 
 	git branch --merged >actual &&
@@ -66,6 +76,16 @@ test_expect_success 'side: branch --merged' '
 
 '
 
+test_expect_success 'branch --merged with pattern implies --list' '
+
+	git branch --merged=side master >actual &&
+	{
+		echo "  master"
+	} >expect &&
+	test_cmp expect actual
+
+'
+
 test_expect_success 'side: branch --no-merged' '
 
 	git branch --no-merged >actual &&
@@ -95,4 +115,19 @@ test_expect_success 'master: branch --no-merged' '
 
 '
 
+test_expect_success 'branch --no-merged with pattern implies --list' '
+
+	git branch --no-merged=master master >actual &&
+	>expect &&
+	test_cmp expect actual
+
+'
+
+test_expect_success 'implicit --list conflicts with modification options' '
+
+	test_must_fail git branch --contains=master -d &&
+	test_must_fail git branch --contains=master -m foo
+
+'
+
 test_done
-- 
1.8.1.2.5.g1cb3f73

Re: [PATCH 0/2] improve "git branch --contains=<commit> <pattern>"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> That being said, we could be much more helpful. It seems like --contains
> should imply listing mode, since it is nonsensical in other modes. The
> second patch below adjusts that, and makes the command above do what you
> expect.
>
>   [1/2]: docs: clarify git-branch --list behavior
>   [2/2]: branch: let branch filters imply --list

Thanks.  Looks good.

Re: [PATCH 2/2] branch: let branch filters imply --list

[Peter Wu]

On Thursday 31 January 2013 01:46:11 Jeff King wrote:
> Currently, a branch filter like `--contains`, `--merged`, or
> `--no-merged` is ignored when we are not in listing mode.
> For example:
> 
>   git branch --contains=foo bar
> 
> will create the branch "bar" from the current HEAD, ignoring
> the `--contains` argument entirely. This is not very
> helpful. There are two reasonable behaviors for git here:
> 
>   1. Flag an error; the arguments do not make sense.
> 
>   2. Implicitly go into `--list` mode
> 
> This patch chooses the latter, as it is more convenient, and
> there should not be any ambiguity with attempting to create
> a branch; using `--contains` and not wanting to list is
> nonsensical.
> 
> That leaves the case where an explicit modification option
> like `-d` is given.  We already catch the case where
> `--list` is given alongside `-d` and flag an error. With
> this patch, we will also catch the use of `--contains` and
> other filter options alongside `-d`.
> 
> Signed-off-by: Jeff King <peff@peff.net>
Tested-by: Peter Wu <lekensteyn@gmail.com>

I have tested this patch on top of 1.8.1.2 and it seems to work.

One note, the following command spits out master without complaining about the 
non-existing branch name:

    git branch --contains <id> master <non-existant branch name>

(the order of branches doesn't affect the result.)

Regards,
Peter

Re: [PATCH 2/2] branch: let branch filters imply --list

[Junio C Hamano]

Peter Wu <lekensteyn@gmail.com> writes:

> One note, the following command spits out master without complaining about the 
> non-existing branch name:
>
>     git branch --contains <id> master <non-existant branch name>
>
> (the order of branches doesn't affect the result.)

That is perfectly normal.

What you gave after "--contains <id>" are *not* branch names.  They
are patterns against branch names that fits the given criteria (in
this case "--contains <id>") are matched, and the branches that do
not match any of the patterns will not appear in the result.

Re: [PATCH 1/2] docs: clarify git-branch --list behavior

[Eric Sunshine]

On Thu, Jan 31, 2013 at 1:45 AM, Jeff King <peff@peff.net> wrote:
> +If `--list` is given, or if there are no non-option arguments, existing
> +branches are listed; the current branch will be highlighted with an
> +asterisk.  Option `-r` causes the remote-tracking branches to be listed,
> +and option `-a` shows both local and remote branches. If a `<pattern>`
> +is given, it is used as a shell wildcard to restrict the output to
> +matching branches. If multiple patterns are given, a branch is shown if
> +any it is matched by any of the patterns.

s/if any it is/if it is/

-- ES

Re: [PATCH 1/2] docs: clarify git-branch --list behavior

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Thu, Jan 31, 2013 at 1:45 AM, Jeff King <peff@peff.net> wrote:
>> +If `--list` is given, or if there are no non-option arguments, existing
>> +branches are listed; the current branch will be highlighted with an
>> +asterisk.  Option `-r` causes the remote-tracking branches to be listed,
>> +and option `-a` shows both local and remote branches. If a `<pattern>`
>> +is given, it is used as a shell wildcard to restrict the output to
>> +matching branches. If multiple patterns are given, a branch is shown if
>> +any it is matched by any of the patterns.
>
> s/if any it is/if it is/

Thanks; I'll squash this in.  Peff, no need to resend.

 Documentation/git-branch.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 01aa87f..2635dee 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -28,7 +28,7 @@ asterisk.  Option `-r` causes the remote-tracking branches to be listed,
 and option `-a` shows both local and remote branches. If a `<pattern>`
 is given, it is used as a shell wildcard to restrict the output to
 matching branches. If multiple patterns are given, a branch is shown if
-any it is matched by any of the patterns.  Note that when providing a
+it matches any of the patterns.  Note that when providing a
 `<pattern>`, you must use `--list`; otherwise the command is interpreted
 as branch creation.
 

Re: [PATCH 1/2] docs: clarify git-branch --list behavior

[Jeff King]

On Thu, Jan 31, 2013 at 04:37:01PM -0800, Junio C Hamano wrote:

> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> > On Thu, Jan 31, 2013 at 1:45 AM, Jeff King <peff@peff.net> wrote:
> >> +If `--list` is given, or if there are no non-option arguments, existing
> >> +branches are listed; the current branch will be highlighted with an
> >> +asterisk.  Option `-r` causes the remote-tracking branches to be listed,
> >> +and option `-a` shows both local and remote branches. If a `<pattern>`
> >> +is given, it is used as a shell wildcard to restrict the output to
> >> +matching branches. If multiple patterns are given, a branch is shown if
> >> +any it is matched by any of the patterns.
> >
> > s/if any it is/if it is/
> 
> Thanks; I'll squash this in.  Peff, no need to resend.

Thanks. No matter how many times I proofread a doc change, I always
manage to slip an error into the final version. Hooray for many eyes.

-Peff

Re: [PATCH 1/2] docs: clarify git-branch --list behavior

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Thanks. No matter how many times I proofread a doc change, I always
> manage to slip an error into the final version.

I think everybody shares that trait.

> Hooray for many eyes.

Indeed.