threads / patch / 32722

patchbranch: reject -D/-d without branch name

Subject: [PATCH] branch: reject -D/-d without branch name

## tl;dr

14 messages between Jan 25, 2013 and Jan 28, 2013. Diffs are folded; open one to read it.

replies: 13people: 4as markdown or json

Nguyễn Thái Ngọc Duy· Jan 25, 2013, 08:26 UTC · lore
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/branch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to builtin/branch.c +1 −1
diff --git a/builtin/branch.c b/builtin/branch.c
index 873f624..1d3e842 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -837,7 +837,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		colopts = 0;
 	}
 
-	if (delete)
+	if (delete && argc)
 		return delete_branches(argc, argv, delete > 1, kinds, quiet);
 	else if (list) {
 		int ret = print_ref_list(kinds, detached, verbose, abbrev,

-- 
1.8.1.1.380.g782aa97
Matthieu Moy· Jan 25, 2013, 08:45 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH] branch: reject -D/-d without branch name

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 13 quoted lines
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 873f624..1d3e842 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -837,7 +837,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  		colopts = 0;
>  	}
>  
> -	if (delete)
> +	if (delete && argc)
>  		return delete_branches(argc, argv, delete > 1, kinds, quiet);
>  	else if (list) {
>  		int ret = print_ref_list(kinds, detached, verbose, abbrev,

Shouldn't this error out with a clean error message ("branch name expected" or so)?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Duy Nguyen· Jan 25, 2013, 08:56 UTC · re: Matthieu Moy · lore

Re: [PATCH] branch: reject -D/-d without branch name

On Fri, Jan 25, 2013 at 3:45 PM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Show 18 quoted lines
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> diff --git a/builtin/branch.c b/builtin/branch.c
>> index 873f624..1d3e842 100644
>> --- a/builtin/branch.c
>> +++ b/builtin/branch.c
>> @@ -837,7 +837,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>>               colopts = 0;
>>       }
>>
>> -     if (delete)
>> +     if (delete && argc)
>>               return delete_branches(argc, argv, delete > 1, kinds, quiet);
>>       else if (list) {
>>               int ret = print_ref_list(kinds, detached, verbose, abbrev,
>
> Shouldn't this error out with a clean error message ("branch name
> expected" or so)?

Yeah. But on the other hand, this command seems to prefer to print help usage when incorrect number of arguments is given (checkout blocks "if (edit_description)" and "if (rename)" in cmd_branch). Should those be fixed too?

-- 
Duy
Nguyễn Thái Ngọc Duy· Jan 25, 2013, 12:50 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 1/3] branch: reject -D/-d without branch name

---
 builtin/branch.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Show changes to builtin/branch.c +4 −2
diff --git a/builtin/branch.c b/builtin/branch.c
index 873f624..50fcacc 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -837,9 +837,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		colopts = 0;
 	}
 
-	if (delete)
+	if (delete) {
+		if (!argc)
+			die(_("branch name required"));
 		return delete_branches(argc, argv, delete > 1, kinds, quiet);
-	else if (list) {
+	} else if (list) {
 		int ret = print_ref_list(kinds, detached, verbose, abbrev,
 					 with_commit, argv);
 		print_columns(&output, colopts, NULL);

-- 
1.8.0.rc2.23.g1fb49df
Nguyễn Thái Ngọc Duy· Jan 25, 2013, 12:50 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 2/3] branch: give a more helpful message on redundant arguments

While at there, do not stop user from editing a branch description
when the unrelated HEAD is detached.
---
 builtin/branch.c  | 12 ++++++------
 t/t3200-branch.sh |  4 ++--
 2 files changed, 8 insertions(+), 8 deletions(-)
Show changes to 2 files +8 −8

builtin/branch.c, t/t3200-branch.sh

diff --git a/builtin/branch.c b/builtin/branch.c
index 50fcacc..ca61c5b 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -852,14 +852,14 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		const char *branch_name;
 		struct strbuf branch_ref = STRBUF_INIT;
 
-		if (detached)
-			die("Cannot give description to detached HEAD");
-		if (!argc)
+		if (!argc) {
+			if (detached)
+				die("Cannot give description to detached HEAD");
 			branch_name = head;
-		else if (argc == 1)
+		} else if (argc == 1)
 			branch_name = argv[0];
 		else
-			usage_with_options(builtin_branch_usage, options);
+			die(_("cannot edit description of more than one branch"));
 
 		strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
 		if (!ref_exists(branch_ref.buf)) {
@@ -881,7 +881,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		else if (argc == 2)
 			rename_branch(argv[0], argv[1], rename > 1);
 		else
-			usage_with_options(builtin_branch_usage, options);
+			die(_("too many branches for a rename operation"));
 	} else if (new_upstream) {
 		struct branch *branch = branch_get(argv[0]);
 
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 80e6be3..f3e0e4a 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -73,8 +73,8 @@ test_expect_success \
 
 test_expect_success \
     'git branch -m dumps usage' \
-       'test_expect_code 129 git branch -m 2>err &&
-	test_i18ngrep "[Uu]sage: git branch" err'
+       'test_expect_code 128 git branch -m 2>err &&
+	test_i18ngrep "too many branches for a rename operation" err'
 
 test_expect_success \
     'git branch -m m m/m should work' \

-- 
1.8.0.rc2.23.g1fb49df
Jonathan Nieder· Jan 27, 2013, 11:58 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v2 2/3] branch: give a more helpful message on redundant arguments

Nguyễn Thái Ngọc Duy wrote:
Show 12 quoted lines
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -852,14 +852,14 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  		const char *branch_name;
>  		struct strbuf branch_ref = STRBUF_INIT;
>  
> -		if (detached)
> -			die("Cannot give description to detached HEAD");
> -		if (!argc)
> +		if (!argc) {
> +			if (detached)
> +				die("Cannot give description to detached HEAD");

Good catch. Shouldn't this bugfix be a separate patch, so it can also be included in maint?

Nguyễn Thái Ngọc Duy· Jan 25, 2013, 12:50 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 3/3] branch: mark more strings for translation

---
 builtin/branch.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
Show changes to builtin/branch.c +6 −5
diff --git a/builtin/branch.c b/builtin/branch.c
index ca61c5b..597b578 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -466,7 +466,7 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,
 			     int verbose, int abbrev)
 {
 	struct strbuf subject = STRBUF_INIT, stat = STRBUF_INIT;
-	const char *sub = " **** invalid ref ****";
+	const char *sub = _(" **** invalid ref ****");
 	struct commit *commit = item->commit;
 
 	if (commit && !parse_commit(commit)) {
@@ -590,7 +590,7 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
 		struct commit *filter;
 		filter = lookup_commit_reference_gently(merge_filter_ref, 0);
 		if (!filter)
-			die("object '%s' does not point to a commit",
+			die(_("object '%s' does not point to a commit"),
 			    sha1_to_hex(merge_filter_ref));
 
 		filter->object.flags |= UNINTERESTING;
@@ -854,7 +854,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 
 		if (!argc) {
 			if (detached)
-				die("Cannot give description to detached HEAD");
+				die(_("Cannot give description to detached HEAD"));
 			branch_name = head;
 		} else if (argc == 1)
 			branch_name = argv[0];
@@ -866,10 +866,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			strbuf_release(&branch_ref);
 
 			if (!argc)
-				return error("No commit on branch '%s' yet.",
+				return error(_("No commit on branch '%s' yet."),
 					     branch_name);
 			else
-				return error("No such branch '%s'.", branch_name);
+				return error(_("No branch named '%s'."),
+					     branch_name);
 		}
 		strbuf_release(&branch_ref);
 

-- 
1.8.0.rc2.23.g1fb49df
Jonathan Nieder· Jan 27, 2013, 11:55 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v2 3/3] branch: mark more strings for translation

Nguyễn Thái Ngọc Duy wrote:
Show 8 quoted lines
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -466,7 +466,7 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,
>  			     int verbose, int abbrev)
>  {
>  	struct strbuf subject = STRBUF_INIT, stat = STRBUF_INIT;
> -	const char *sub = " **** invalid ref ****";
> +	const char *sub = _(" **** invalid ref ****");

This worried me for a second --- is it an actual message that gets emitted, a placeholder used only in code, or some combination of the two?

Luckily it really is just a message (or rather, a value for the commit message column in the " f7c0c00 [ahead 58, behind 197] vcs-svn: drop obj_pool.h" message).

For what it's worth, assuming this passes tests,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Junio C Hamano· Jan 25, 2013, 19:04 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v2 1/3] branch: reject -D/-d without branch name

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> ---
>  builtin/branch.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
Forgot to sign-off?
Is this a real problem?

I do not see it particularly wrong to succeed after deleting 0 or more given branch names.

Show 19 quoted lines
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 873f624..50fcacc 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -837,9 +837,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  		colopts = 0;
>  	}
>  
> -	if (delete)
> +	if (delete) {
> +		if (!argc)
> +			die(_("branch name required"));
>  		return delete_branches(argc, argv, delete > 1, kinds, quiet);
> -	else if (list) {
> +	} else if (list) {
>  		int ret = print_ref_list(kinds, detached, verbose, abbrev,
>  					 with_commit, argv);
>  		print_columns(&output, colopts, NULL);
Duy Nguyen· Jan 26, 2013, 03:12 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 1/3] branch: reject -D/-d without branch name

On Sat, Jan 26, 2013 at 2:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 7 quoted lines
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> ---
>>  builtin/branch.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> Forgot to sign-off?
Yes
> Is this a real problem?
Yes. My thoughts yesterday when I happened to type "git branch -D":

"Wait, I just entered a delete command without a branch name. It did not report anything, which in UNIX world usually means successful operation. _What_ did it delete??"

I knew this code so I just went check. If I did not know, I might as well list all branches I had and see if something was missing. A short message would have saved me the trouble.

> I do not see it particularly wrong to succeed after deleting 0 or
> more given branch names.
-- 
Duy
Nguyễn Thái Ngọc Duy· Jan 28, 2013, 01:18 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 1/4] branch: no detached HEAD check when editing another branch's description

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Split out from v3's 2/3 as Jonathan suggested.
 builtin/branch.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to builtin/branch.c +4 −4
diff --git a/builtin/branch.c b/builtin/branch.c
index 873f624..ea6498b 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -850,11 +850,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		const char *branch_name;
 		struct strbuf branch_ref = STRBUF_INIT;
 
-		if (detached)
-			die("Cannot give description to detached HEAD");
-		if (!argc)
+		if (!argc) {
+			if (detached)
+				die("Cannot give description to detached HEAD");
 			branch_name = head;
-		else if (argc == 1)
+		} else if (argc == 1)
 			branch_name = argv[0];
 		else
 			usage_with_options(builtin_branch_usage, options);

-- 
1.8.1.1.459.g5970e58
Nguyễn Thái Ngọc Duy· Jan 28, 2013, 01:18 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 2/4] branch: reject -D/-d without branch name

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/branch.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Show changes to builtin/branch.c +4 −2
diff --git a/builtin/branch.c b/builtin/branch.c
index ea6498b..30c4545 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -837,9 +837,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		colopts = 0;
 	}
 
-	if (delete)
+	if (delete) {
+		if (!argc)
+			die(_("branch name required"));
 		return delete_branches(argc, argv, delete > 1, kinds, quiet);
-	else if (list) {
+	} else if (list) {
 		int ret = print_ref_list(kinds, detached, verbose, abbrev,
 					 with_commit, argv);
 		print_columns(&output, colopts, NULL);

-- 
1.8.1.1.459.g5970e58
Nguyễn Thái Ngọc Duy· Jan 28, 2013, 01:18 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 3/4] branch: give a more helpful message on redundant arguments

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/branch.c  | 4 ++--
 t/t3200-branch.sh | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)
Show changes to 2 files +4 −4

builtin/branch.c, t/t3200-branch.sh

diff --git a/builtin/branch.c b/builtin/branch.c
index 30c4545..ca61c5b 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -859,7 +859,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		} else if (argc == 1)
 			branch_name = argv[0];
 		else
-			usage_with_options(builtin_branch_usage, options);
+			die(_("cannot edit description of more than one branch"));
 
 		strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
 		if (!ref_exists(branch_ref.buf)) {
@@ -881,7 +881,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		else if (argc == 2)
 			rename_branch(argv[0], argv[1], rename > 1);
 		else
-			usage_with_options(builtin_branch_usage, options);
+			die(_("too many branches for a rename operation"));
 	} else if (new_upstream) {
 		struct branch *branch = branch_get(argv[0]);
 
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 80e6be3..f3e0e4a 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -73,8 +73,8 @@ test_expect_success \
 
 test_expect_success \
     'git branch -m dumps usage' \
-       'test_expect_code 129 git branch -m 2>err &&
-	test_i18ngrep "[Uu]sage: git branch" err'
+       'test_expect_code 128 git branch -m 2>err &&
+	test_i18ngrep "too many branches for a rename operation" err'
 
 test_expect_success \
     'git branch -m m m/m should work' \

-- 
1.8.1.1.459.g5970e58
Nguyễn Thái Ngọc Duy· Jan 28, 2013, 01:18 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 4/4] branch: mark more strings for translation

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 On Sun, Jan 27, 2013 at 6:55 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
 > For what it's worth, assuming this passes tests,
 It passes the tests. Although I doubt the tests are written to catch
 this.
 builtin/branch.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
Show changes to builtin/branch.c +6 −5
diff --git a/builtin/branch.c b/builtin/branch.c
index ca61c5b..597b578 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -466,7 +466,7 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,
 			     int verbose, int abbrev)
 {
 	struct strbuf subject = STRBUF_INIT, stat = STRBUF_INIT;
-	const char *sub = " **** invalid ref ****";
+	const char *sub = _(" **** invalid ref ****");
 	struct commit *commit = item->commit;
 
 	if (commit && !parse_commit(commit)) {
@@ -590,7 +590,7 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
 		struct commit *filter;
 		filter = lookup_commit_reference_gently(merge_filter_ref, 0);
 		if (!filter)
-			die("object '%s' does not point to a commit",
+			die(_("object '%s' does not point to a commit"),
 			    sha1_to_hex(merge_filter_ref));
 
 		filter->object.flags |= UNINTERESTING;
@@ -854,7 +854,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 
 		if (!argc) {
 			if (detached)
-				die("Cannot give description to detached HEAD");
+				die(_("Cannot give description to detached HEAD"));
 			branch_name = head;
 		} else if (argc == 1)
 			branch_name = argv[0];
@@ -866,10 +866,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			strbuf_release(&branch_ref);
 
 			if (!argc)
-				return error("No commit on branch '%s' yet.",
+				return error(_("No commit on branch '%s' yet."),
 					     branch_name);
 			else
-				return error("No such branch '%s'.", branch_name);
+				return error(_("No branch named '%s'."),
+					     branch_name);
 		}
 		strbuf_release(&branch_ref);
 

-- 
1.8.1.1.459.g5970e58

← back to recent threads