threads / patch / 58498

patchbranch: description for non-existent branch errors

Subject: [PATCH] branch: description for non-existent branch errors

## tl;dr

16 messages between Sep 22, 2022 and Oct 9, 2022. Diffs are folded; open one to read it.

replies: 15people: 4as markdown or json

Rubén Justo· Sep 22, 2022, 22:37 UTC · lore

When the repository does not yet has commits, some errors describe that there is no branch:

    $ git init -b first
    $ git --edit-description first
    fatal: branch 'first' does not exist
    $ git --set-upstream-to=upstream
    fatal: branch 'first' does not exist
    $ git branch -c second
    error: refname refs/heads/first not found
    fatal: Branch copy failed
That "first" branch is unborn but to say it doesn't exists is confusing.

Options "-c" (copy) and "-m" (rename) show the same error when the origin branch doesn't exists:

    $ git branch -c non-existent-branch second
    error: refname refs/heads/non-existent-branch not found
    fatal: Branch copy failed
    $ git branch -m non-existent-branch second
    error: refname refs/heads/non-existent-branch not found
    fatal: Branch rename failed

Note that "--edit-description" without an explicit argument is already considering the _empty repository_ circumstance in its error. Also note that "-m" on the initial branch it is an allowed operation.

This commit makes the error descriptions for those branch operations with unborn or non-existent branches, more informative.

This is the result of the change:
    $ git init -b first
    $ git --edit-description first
    fatal: No commit on branch 'first' yet.
    $ git --set-upstream-to=upstream
    fatal: No commit on branch 'first' yet.
    $ git -c second
    fatal: No commit on branch 'first' yet.
    $ git [-c/-m] non-existent-branch second
    fatal: No branch named 'non-existent-branch'.
---
Hi all.
I would appreciate some guidance here.

This patch is based on master but there is already in 'seen' a patch submitted for 'branch' that produces conflicts with this. I don't know if it is more convenient to submit this patch based on 'seen' considering that.

I submitted that other patch too and prefer to keep the changes separate but maybe it's better not to and do a rollup with this on that other patch.. I don't know how disturbing it is to submit a new patch when it is "waiting for review".

Another approach?
Thank you.
Un saludo.
 builtin/branch.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)
Show changes to builtin/branch.c +12 −2
diff --git a/builtin/branch.c b/builtin/branch.c
index 55cd9a6e99..5ca35064f3 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -538,6 +538,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 			die(_("Invalid branch name: '%s'"), oldname);
 	}
 
+	if (copy && !ref_exists(oldref.buf)) {
+		if (!strcmp(head, oldname))
+			die(_("No commit on branch '%s' yet."), oldname);
+		else
+			die(_("No branch named '%s'."), oldname);
+	}
+
 	/*
 	 * A command like "git branch -M currentbranch currentbranch" cannot
 	 * cause the worktree to become inconsistent with HEAD, so allow it.
@@ -805,7 +812,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		if (!ref_exists(branch_ref.buf)) {
 			strbuf_release(&branch_ref);
 
-			if (!argc)
+			if (!argc || !strcmp(head, branch_name))
 				return error(_("No commit on branch '%s' yet."),
 					     branch_name);
 			else
@@ -848,8 +855,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			die(_("no such branch '%s'"), argv[0]);
 		}
 
-		if (!ref_exists(branch->refname))
+		if (!ref_exists(branch->refname)) {
+			if (!argc || !strcmp(head, branch->name))
+				die(_("No commit on branch '%s' yet."), branch->name);
 			die(_("branch '%s' does not exist"), branch->name);
+		}
 
 		dwim_and_setup_tracking(the_repository, branch->name,
 					new_upstream, BRANCH_TRACK_OVERRIDE,
-- 
2.36.1
Rubén Justo· Sep 24, 2022, 22:52 UTC · re: Rubén Justo · lore

When the repository does not yet has commits, some errors describe that there is no branch:

    $ git init -b first
    $ git --edit-description first
    fatal: branch 'first' does not exist
    $ git --set-upstream-to=upstream
    fatal: branch 'first' does not exist
    $ git branch -c second
    error: refname refs/heads/first not found
    fatal: Branch copy failed
That "first" branch is unborn but to say it doesn't exists is confusing.

Options "-c" (copy) and "-m" (rename) show the same error when the origin branch doesn't exists:

    $ git branch -c non-existent-branch second
    error: refname refs/heads/non-existent-branch not found
    fatal: Branch copy failed
    $ git branch -m non-existent-branch second
    error: refname refs/heads/non-existent-branch not found
    fatal: Branch rename failed

Note that "--edit-description" without an explicit argument is already considering the _empty repository_ circumstance in its error. Also note that "-m" on the initial branch it is an allowed operation.

This commit makes the error descriptions for those branch operations with unborn or non-existent branches, more informative.

This is the result of the change:
    $ git init -b first
    $ git --edit-description first
    fatal: No commit on branch 'first' yet.
    $ git --set-upstream-to=upstream
    fatal: No commit on branch 'first' yet.
    $ git -c second
    fatal: No commit on branch 'first' yet.
    $ git [-c/-m] non-existent-branch second
    fatal: No branch named 'non-existent-branch'.
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
Just re-sending with the Signed-off-by label. Sorry.
 builtin/branch.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)
Show changes to builtin/branch.c +12 −2
diff --git a/builtin/branch.c b/builtin/branch.c
index 55cd9a6e99..5ca35064f3 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -538,6 +538,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 			die(_("Invalid branch name: '%s'"), oldname);
 	}
 
+	if (copy && !ref_exists(oldref.buf)) {
+		if (!strcmp(head, oldname))
+			die(_("No commit on branch '%s' yet."), oldname);
+		else
+			die(_("No branch named '%s'."), oldname);
+	}
+
 	/*
 	 * A command like "git branch -M currentbranch currentbranch" cannot
 	 * cause the worktree to become inconsistent with HEAD, so allow it.
@@ -805,7 +812,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		if (!ref_exists(branch_ref.buf)) {
 			strbuf_release(&branch_ref);
 
-			if (!argc)
+			if (!argc || !strcmp(head, branch_name))
 				return error(_("No commit on branch '%s' yet."),
 					     branch_name);
 			else
@@ -848,8 +855,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			die(_("no such branch '%s'"), argv[0]);
 		}
 
-		if (!ref_exists(branch->refname))
+		if (!ref_exists(branch->refname)) {
+			if (!argc || !strcmp(head, branch->name))
+				die(_("No commit on branch '%s' yet."), branch->name);
 			die(_("branch '%s' does not exist"), branch->name);
+		}
 
 		dwim_and_setup_tracking(the_repository, branch->name,
 					new_upstream, BRANCH_TRACK_OVERRIDE,
-- 
2.36.1
Junio C Hamano· Sep 26, 2022, 18:12 UTC · re: Rubén Justo · lore

Re: [PATCH] branch: description for non-existent branch errors

Rubén Justo <rjusto@gmail.com> writes:
> When the repository does not yet has commits, some errors describe that
"has" -> "have".
>  builtin/branch.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)

Tests to protect the new behaviour from getting broken by future developers are missing.

Show 14 quoted lines
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 55cd9a6e99..5ca35064f3 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -538,6 +538,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>  			die(_("Invalid branch name: '%s'"), oldname);
>  	}
>  
> +	if (copy && !ref_exists(oldref.buf)) {
> +		if (!strcmp(head, oldname))
> +			die(_("No commit on branch '%s' yet."), oldname);
> +		else
> +			die(_("No branch named '%s'."), oldname);
> +	}

Let's not make it worse by starting the die() message with capital letters, even though the existing "git branch" error messages are already mixture that they need to be cleaned up later.

Thanks.
Rubén Justo· Sep 26, 2022, 23:35 UTC · re: Junio C Hamano · lore

Re: [PATCH] branch: description for non-existent branch errors

On 26/9/22 20:12, Junio C Hamano wrote:
Thank you for the review. I will do a reroll with your comments, but about..
Show 11 quoted lines
>> +	if (copy && !ref_exists(oldref.buf)) {
>> +		if (!strcmp(head, oldname))
>> +			die(_("No commit on branch '%s' yet."), oldname);
>> +		else
>> +			die(_("No branch named '%s'."), oldname);
>> +	}
> 
> Let's not make it worse by starting the die() message with capital
> letters, even though the existing "git branch" error messages are
> already mixture that they need to be cleaned up later.
> 

I chose these literals for the errors because they are already translated, appear below in that same file. I thought that would make the patch easier to review and apply, for the translators too.

Maybe we can maintain those capitalized literals to have a simpler patch to merge and leave the "mixtures" for a posterior patch. I have already identified a leak in that command:

	static const char *head;
	...
	int cmd_branch()
	...
		head = resolve_refdup("HEAD", 0, &head_oid, NULL);
"head" is leaked but there is no easy free, because some exists are like:
		if (delete) {
			if (!argc)
				die(_("branch name required"));
			return delete_branches(argc, argv, delete > 1, filter.kind, quiet);

Without entering in this too much, maybe an atexit() approach, as in some others commands... but maybe a more clear flow.. and that would need some changes that can carry out the "mixtures".

Anyway, if you think there is no problem with the new literal in that file, I will add it to the reroll with the rest of the comments in your review.

I pointed out in the first mail of this thread, there is already a patch in 'seen' that touches builtin/branch.c [1]. I would like to keep the patches separated, but I don't know how to proceed: make the change from 'seen', keep it from 'master'... Maybe you can give me some guidance in this.

Thank you.
Un saludo.
Junio C Hamano· Sep 27, 2022, 22:24 UTC · re: Rubén Justo · lore

Re: [PATCH] branch: description for non-existent branch errors

Rubén Justo <rjusto@gmail.com> writes:
> I pointed out in the first mail of this thread, there is already a patch in
> 'seen' that touches builtin/branch.c [1].  I would like to keep the patches
> separated, but I don't know how to proceed: make the change from 'seen', keep
> it from 'master'... Maybe you can give me some guidance in this.

I do not see much problem in keeping them separated. My trial merge of the result of applying this patch on top of 'master', with the other topic that has the "branch description for nth prior checkout" patch does show a minor textual conflict, but the resolution does not look too bad.

Check near the topic branch of 'seen' after I push out today's integration result in a few hours and see if they look reasonable.

Thanks.

diff --cc builtin/branch.c index 5ca35064f3,13d1f028da..2b3884ce61

Show changes to builtin/branch.c +7 −8
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@@ -810,19 -807,18 +814,18 @@@ int cmd_branch(int argc, const char **a
  
  		strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
  		if (!ref_exists(branch_ref.buf)) {
- 			strbuf_release(&branch_ref);
- 
 -			if (!argc)
 +			if (!argc || !strcmp(head, branch_name))
- 				return error(_("No commit on branch '%s' yet."),
+ 				ret = error(_("No commit on branch '%s' yet."),
  					     branch_name);
  			else
- 				return error(_("No branch named '%s'."),
+ 				ret = error(_("No branch named '%s'."),
  					     branch_name);
- 		}
- 		strbuf_release(&branch_ref);
+ 		} else
+ 			ret = edit_branch_description(branch_name);
  
- 		if (edit_branch_description(branch_name))
- 			return 1;
+ 		strbuf_release(&branch_ref);
+ 		strbuf_release(&buf);
+ 		return -ret;
  	} else if (copy) {
  		if (!argc)
  			die(_("branch name required"));
Junio C Hamano· Sep 28, 2022, 17:41 UTC · re: Junio C Hamano · lore

Re: [PATCH] branch: description for non-existent branch errors

Junio C Hamano <gitster@pobox.com> writes:
Show 17 quoted lines
> Rubén Justo <rjusto@gmail.com> writes:
>
>> I pointed out in the first mail of this thread, there is already a patch in
>> 'seen' that touches builtin/branch.c [1].  I would like to keep the patches
>> separated, but I don't know how to proceed: make the change from 'seen', keep
>> it from 'master'... Maybe you can give me some guidance in this.
>
> I do not see much problem in keeping them separated.  My trial merge
> of the result of applying this patch on top of 'master', with the
> other topic that has the "branch description for nth prior checkout"
> patch does show a minor textual conflict, but the resolution does
> not look too bad.
>
> Check near the topic branch of 'seen' after I push out today's
> integration result in a few hours and see if they look reasonable.
>
> Thanks.

Ah, I forgot to mention. As to the error messages that begin with a capital letter, to be consistent with violating messages that are already there in builtin/branch.c, let's keep them as they are in your patch. We can and should clean them up later, perhaps soon after the patch under discussion matures, but I agree with you that it can be left outside the scope of this patch.

But stepping back a bit, in the longer term, we might want to change the behaviour of "git branch --edit-description", at least when no branch is specified on the command line and we are on an unborn branch. It is merely the matter of setting a variable in the configuration file, so there may not be a strong reason to forbid

    $ git init trash
    $ cd trash
    $ git branch --edit-description
    $ git commit --allow-empty -m initial
while allowing the same sequence with the last two commands reversed.

After all, renaming a branch with "branch -m" does not to require an existing ref that points at a commit, i.e.

    $ git init -b master trash
    $ cd trash
    $ git config branch.master.description "describes master"
    $ git branch -m master main

does work fine and you end up with branch.main.description at the end.

Junio C Hamano· Sep 28, 2022, 17:50 UTC · re: Junio C Hamano · lore

Re: [PATCH] branch: description for non-existent branch errors

Junio C Hamano <gitster@pobox.com> writes:
Show 17 quoted lines
> Rubén Justo <rjusto@gmail.com> writes:
>
>> I pointed out in the first mail of this thread, there is already a patch in
>> 'seen' that touches builtin/branch.c [1].  I would like to keep the patches
>> separated, but I don't know how to proceed: make the change from 'seen', keep
>> it from 'master'... Maybe you can give me some guidance in this.
>
> I do not see much problem in keeping them separated.  My trial merge
> of the result of applying this patch on top of 'master', with the
> other topic that has the "branch description for nth prior checkout"
> patch does show a minor textual conflict, but the resolution does
> not look too bad.
>
> Check near the topic branch of 'seen' after I push out today's
> integration result in a few hours and see if they look reasonable.
>
> Thanks.

Ah, I forgot to mention. As to the error messages that begin with a capital letter, to be consistent with violating messages that are already there in builtin/branch.c, let's keep them as they are in your patch. We can and should clean them up later, perhaps soon after the patch under discussion matures, but I agree with you that it can be left outside the scope of this patch.

But stepping back a bit, in the longer term, we might want to change the behaviour of "git branch --edit-description", at least when no branch is specified on the command line and we are on an unborn branch. It is merely the matter of setting a variable in the configuration file, so there may not be a strong reason to forbid

    $ git init trash
    $ cd trash
    $ git branch --edit-description
    $ git commit --allow-empty -m initial
while allowing the same sequence with the last two commands reversed.

After all, renaming a branch with "branch -m" does not to require an existing ref that points at a commit, i.e.

    $ git init -b master trash
    $ cd trash
    $ git config branch.master.description "describes master"
    $ git branch -m master main

does work fine and you end up with branch.main.description at the end.

Rubén Justo· Sep 28, 2022, 23:59 UTC · re: Junio C Hamano · lore

Re: [PATCH] branch: description for non-existent branch errors

On 28/9/22 19:50, Junio C Hamano wrote:
Show 23 quoted lines
> But stepping back a bit, in the longer term, we might want to change
> the behaviour of "git branch --edit-description", at least when no
> branch is specified on the command line and we are on an unborn
> branch.  It is merely the matter of setting a variable in the
> configuration file, so there may not be a strong reason to forbid
> 
>     $ git init trash
>     $ cd trash
>     $ git branch --edit-description
>     $ git commit --allow-empty -m initial
> 
> while allowing the same sequence with the last two commands reversed.
> 
> After all, renaming a branch with "branch -m" does not to require an
> existing ref that points at a commit, i.e.
> 
>     $ git init -b master trash
>     $ cd trash
>     $ git config branch.master.description "describes master"
>     $ git branch -m master main
> 
> does work fine and you end up with branch.main.description at the
> end.
 
Indeed. And "--set-upstream-to"? I haven't found any reason why we
shouldn't allow it for an unborn branch too. "--unset-upstream"
already works.

Those changes I think are worth doing along with the fix for the leak of 'static const char *head'.

And then the error descriptions. Not just the capitalization but some similar but different messages too.

I'll reroll this path with the test for the current errors.
Thank you.
Un saludo.
Junio C Hamano· Sep 29, 2022, 01:56 UTC · re: Rubén Justo · lore

Re: [PATCH] branch: description for non-existent branch errors

Rubén Justo <rjusto@gmail.com> writes:
> Those changes I think are worth doing along with the fix for the leak
> of 'static const char *head'.

Let's not grow the scope of the topic forever. It will increase the chance of new mistakes and throw us into endless iterations.

I think the posted patch plus tests for the new behaviour (i.e. no longer we give a misleading error message) is a good stopping point.

Extending the behaviour to allow some of these operations that currently fail on an unborn branch may or may not make sense, and that should be discussed separately, possibly for each option. After the dust from the current one settles.

Personally, I do not think a "static const char *" variable that holds onto an allocated piece of memory smaller than 1kB is something we should worry about. Leak checkers should also be smart enough not to warn about such a variable, shouldn't they?

> And then the error descriptions.  Not just the capitalization but
> some similar but different messages too.

Yup, and that is probably a separate patch after all of the above settles.

Thanks.
Rubén Justo· Sep 30, 2022, 22:47 UTC · re: Rubén Justo · lore

[PATCH v2] branch: description for non-existent branch errors

When the repository does not yet have commits, some errors describe that there is no branch:

    $ git init -b first
    $ git branch --edit-description first
    error: No branch named 'first'.
    $ git branch --set-upstream-to=upstream
    fatal: branch 'first' does not exist
    $ git branch -c second
    error: refname refs/heads/first not found
    fatal: Branch copy failed
That "first" branch is unborn but to say it doesn't exists is confusing.

Options "-c" (copy) and "-m" (rename) show the same error when the origin branch doesn't exists:

    $ git branch -c non-existent-branch second
    error: refname refs/heads/non-existent-branch not found
    fatal: Branch copy failed
    $ git branch -m non-existent-branch second
    error: refname refs/heads/non-existent-branch not found
    fatal: Branch rename failed

Note that "--edit-description" without an explicit argument is already considering the _empty repository_ circumstance in its error. Also note that "-m" on the initial branch it is an allowed operation.

This commit makes the error descriptions for those branch operations with unborn or non-existent branches, more informative.

This is the result of the change:
    $ git init -b first
    $ git branch --edit-description first
    error: No commit on branch 'first' yet.
    $ git branch --set-upstream-to=upstream
    fatal: No commit on branch 'first' yet.
    $ git branch -c second
    fatal: No commit on branch 'first' yet.
    $ git branch [-c/-m] non-existent-branch second
    fatal: No branch named 'non-existent-branch'.
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
Changes since V1:
 * Fixed the message, non precise descriptions.
 * Error renaming non-existent branch was not correct.
 * Tests for the new errors.
Range-diff:
1:  bba7096c4b ! 1:  096c443a29 branch: description for non-existent branch errors
    @@ Metadata
      ## Commit message ##
         branch: description for non-existent branch errors
     
    -    When the repository does not yet has commits, some errors describe that
    +    When the repository does not yet have commits, some errors describe that
         there is no branch:
     
             $ git init -b first
     
    -        $ git --edit-description first
    -        fatal: branch 'first' does not exist
    +        $ git branch --edit-description first
    +        error: No branch named 'first'.
     
    -        $ git --set-upstream-to=upstream
    +        $ git branch --set-upstream-to=upstream
             fatal: branch 'first' does not exist
     
             $ git branch -c second
    @@ Commit message
     
             $ git init -b first
     
    -        $ git --edit-description first
    -        fatal: No commit on branch 'first' yet.
    +        $ git branch --edit-description first
    +        error: No commit on branch 'first' yet.
     
    -        $ git --set-upstream-to=upstream
    +        $ git branch --set-upstream-to=upstream
             fatal: No commit on branch 'first' yet.
     
    -        $ git -c second
    +        $ git branch -c second
             fatal: No commit on branch 'first' yet.
     
    -        $ git [-c/-m] non-existent-branch second
    +        $ git branch [-c/-m] non-existent-branch second
             fatal: No branch named 'non-existent-branch'.
     
         Signed-off-by: Rubén Justo <rjusto@gmail.com>
    @@ builtin/branch.c: static void copy_or_rename_branch(const char *oldname, const c
      			die(_("Invalid branch name: '%s'"), oldname);
      	}
      
    -+	if (copy && !ref_exists(oldref.buf)) {
    -+		if (!strcmp(head, oldname))
    ++	if ((copy || strcmp(head, oldname)) && !ref_exists(oldref.buf)) {
    ++		if (copy && !strcmp(head, oldname))
     +			die(_("No commit on branch '%s' yet."), oldname);
     +		else
     +			die(_("No branch named '%s'."), oldname);
    @@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix
      
      		dwim_and_setup_tracking(the_repository, branch->name,
      					new_upstream, BRANCH_TRACK_OVERRIDE,
    +
    + ## t/t3202-show-branch.sh ##
    +@@ t/t3202-show-branch.sh: test_description='test show-branch'
    + # arbitrary reference time: 2009-08-30 19:20:00
    + GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW
    + 
    ++test_expect_success 'error descriptions on empty repository' '
    ++	current=$(git branch --show-current) &&
    ++	cat >expect <<-EOF &&
    ++	error: No commit on branch '\''$current'\'' yet.
    ++	EOF
    ++	test_must_fail git branch --edit-description 2>actual &&
    ++	test_cmp expect actual &&
    ++	test_must_fail git branch --edit-description $current 2>actual &&
    ++	test_cmp expect actual
    ++'
    ++
    ++test_expect_success 'fatal descriptions on empty repository' '
    ++	current=$(git branch --show-current) &&
    ++	cat >expect <<-EOF &&
    ++	fatal: No commit on branch '\''$current'\'' yet.
    ++	EOF
    ++	test_must_fail git branch --set-upstream-to=non-existent 2>actual &&
    ++	test_cmp expect actual &&
    ++	test_must_fail git branch -c new-branch 2>actual &&
    ++	test_cmp expect actual
    ++'
    ++
    + test_expect_success 'setup' '
    + 	test_commit initial &&
    + 	for i in $(test_seq 1 10)
    +@@ t/t3202-show-branch.sh: done <<\EOF
    + --reflog --current
    + EOF
    + 
    ++test_expect_success 'error descriptions on non-existent branch' '
    ++	cat >expect <<-EOF &&
    ++	error: No branch named '\''non-existent'\'.'
    ++	EOF
    ++	test_must_fail git branch --edit-description non-existent 2>actual &&
    ++	test_cmp expect actual
    ++'
    ++
    ++test_expect_success 'fatal descriptions on non-existent branch' '
    ++	cat >expect <<-EOF &&
    ++	fatal: branch '\''non-existent'\'' does not exist
    ++	EOF
    ++	test_must_fail git branch --set-upstream-to=non-existent non-existent 2>actual &&
    ++	test_cmp expect actual &&
    ++
    ++	cat >expect <<-EOF &&
    ++	fatal: No branch named '\''non-existent'\''.
    ++	EOF
    ++	test_must_fail git branch -c non-existent new-branch 2>actual &&
    ++	test_cmp expect actual &&
    ++	test_must_fail git branch -m non-existent new-branch 2>actual &&
    ++	test_cmp expect actual
    ++'
    ++
    + test_done
 builtin/branch.c       | 14 +++++++++++--
 t/t3202-show-branch.sh | 46 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 2 deletions(-)
Show changes to 2 files +58 −2

builtin/branch.c, t/t3202-show-branch.sh

diff --git a/builtin/branch.c b/builtin/branch.c
index 55cd9a6e99..499ebec99e 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -538,6 +538,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 			die(_("Invalid branch name: '%s'"), oldname);
 	}
 
+	if ((copy || strcmp(head, oldname)) && !ref_exists(oldref.buf)) {
+		if (copy && !strcmp(head, oldname))
+			die(_("No commit on branch '%s' yet."), oldname);
+		else
+			die(_("No branch named '%s'."), oldname);
+	}
+
 	/*
 	 * A command like "git branch -M currentbranch currentbranch" cannot
 	 * cause the worktree to become inconsistent with HEAD, so allow it.
@@ -805,7 +812,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		if (!ref_exists(branch_ref.buf)) {
 			strbuf_release(&branch_ref);
 
-			if (!argc)
+			if (!argc || !strcmp(head, branch_name))
 				return error(_("No commit on branch '%s' yet."),
 					     branch_name);
 			else
@@ -848,8 +855,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			die(_("no such branch '%s'"), argv[0]);
 		}
 
-		if (!ref_exists(branch->refname))
+		if (!ref_exists(branch->refname)) {
+			if (!argc || !strcmp(head, branch->name))
+				die(_("No commit on branch '%s' yet."), branch->name);
 			die(_("branch '%s' does not exist"), branch->name);
+		}
 
 		dwim_and_setup_tracking(the_repository, branch->name,
 					new_upstream, BRANCH_TRACK_OVERRIDE,
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index f2b9199007..ea7cfd1951 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -7,6 +7,28 @@ test_description='test show-branch'
 # arbitrary reference time: 2009-08-30 19:20:00
 GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW
 
+test_expect_success 'error descriptions on empty repository' '
+	current=$(git branch --show-current) &&
+	cat >expect <<-EOF &&
+	error: No commit on branch '\''$current'\'' yet.
+	EOF
+	test_must_fail git branch --edit-description 2>actual &&
+	test_cmp expect actual &&
+	test_must_fail git branch --edit-description $current 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'fatal descriptions on empty repository' '
+	current=$(git branch --show-current) &&
+	cat >expect <<-EOF &&
+	fatal: No commit on branch '\''$current'\'' yet.
+	EOF
+	test_must_fail git branch --set-upstream-to=non-existent 2>actual &&
+	test_cmp expect actual &&
+	test_must_fail git branch -c new-branch 2>actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'setup' '
 	test_commit initial &&
 	for i in $(test_seq 1 10)
@@ -175,4 +197,28 @@ done <<\EOF
 --reflog --current
 EOF
 
+test_expect_success 'error descriptions on non-existent branch' '
+	cat >expect <<-EOF &&
+	error: No branch named '\''non-existent'\'.'
+	EOF
+	test_must_fail git branch --edit-description non-existent 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'fatal descriptions on non-existent branch' '
+	cat >expect <<-EOF &&
+	fatal: branch '\''non-existent'\'' does not exist
+	EOF
+	test_must_fail git branch --set-upstream-to=non-existent non-existent 2>actual &&
+	test_cmp expect actual &&
+
+	cat >expect <<-EOF &&
+	fatal: No branch named '\''non-existent'\''.
+	EOF
+	test_must_fail git branch -c non-existent new-branch 2>actual &&
+	test_cmp expect actual &&
+	test_must_fail git branch -m non-existent new-branch 2>actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.36.1
Bagas Sanjaya· Oct 1, 2022, 12:43 UTC · re: Rubén Justo · lore

Re: [PATCH v2] branch: description for non-existent branch errors

On 10/1/22 05:47, Rubén Justo wrote:
> This commit makes the error descriptions for those branch operations
> with unborn or non-existent branches, more informative.
> 
s/This commit makes/Make/
-- 
An old man doll... just what I always wanted! - Clara
Rubén Justo· Oct 2, 2022, 21:28 UTC · re: Bagas Sanjaya · lore

Re: [PATCH v2] branch: description for non-existent branch errors

On 1/10/22 14:43, Bagas Sanjaya wrote:
Show 7 quoted lines
> On 10/1/22 05:47, Rubén Justo wrote:
>> This commit makes the error descriptions for those branch operations
>> with unborn or non-existent branches, more informative.
>>
> 
> s/This commit makes/Make/
> 
Thanks
Rubén Justo· Oct 8, 2022, 00:39 UTC · re: Rubén Justo · lore

[PATCH v3] branch: description for non-existent branch errors

When the repository does not yet have commits, some errors describe that there is no branch:

    $ git init -b first
    $ git branch --edit-description first
    error: No branch named 'first'.
    $ git branch --set-upstream-to=upstream
    fatal: branch 'first' does not exist
    $ git branch -c second
    error: refname refs/heads/first not found
    fatal: Branch copy failed
That "first" branch is unborn but to say it doesn't exists is confusing.

Options "-c" (copy) and "-m" (rename) show the same error when the origin branch doesn't exists:

    $ git branch -c non-existent-branch second
    error: refname refs/heads/non-existent-branch not found
    fatal: Branch copy failed
    $ git branch -m non-existent-branch second
    error: refname refs/heads/non-existent-branch not found
    fatal: Branch rename failed

Note that "--edit-description" without an explicit argument is already considering the _empty repository_ circumstance in its error. Also note that "-m" on the initial branch it is an allowed operation.

Make the error descriptions for those branch operations with unborn or non-existent branches, more informative.

This is the result of the change:
    $ git init -b first
    $ git branch --edit-description first
    error: No commit on branch 'first' yet.
    $ git branch --set-upstream-to=upstream
    fatal: No commit on branch 'first' yet.
    $ git branch -c second
    fatal: No commit on branch 'first' yet.
    $ git branch [-c/-m] non-existent-branch second
    fatal: No branch named 'non-existent-branch'.
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
Range-diff:
1:  d8c3242b31 ! 1:  180435ff15 branch: description for non-existent branch errors
    @@ Commit message
         considering the _empty repository_ circumstance in its error.  Also note
         that "-m" on the initial branch it is an allowed operation.
     
    -    This commit makes the error descriptions for those branch operations
    -    with unborn or non-existent branches, more informative.
    +    Make the error descriptions for those branch operations with unborn or
    +    non-existent branches, more informative.
     
         This is the result of the change:
     
 builtin/branch.c       | 14 +++++++++++--
 t/t3202-show-branch.sh | 46 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 2 deletions(-)
Show changes to 2 files +58 −2

builtin/branch.c, t/t3202-show-branch.sh

diff --git a/builtin/branch.c b/builtin/branch.c
index 55cd9a6e99..499ebec99e 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -538,6 +538,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 			die(_("Invalid branch name: '%s'"), oldname);
 	}
 
+	if ((copy || strcmp(head, oldname)) && !ref_exists(oldref.buf)) {
+		if (copy && !strcmp(head, oldname))
+			die(_("No commit on branch '%s' yet."), oldname);
+		else
+			die(_("No branch named '%s'."), oldname);
+	}
+
 	/*
 	 * A command like "git branch -M currentbranch currentbranch" cannot
 	 * cause the worktree to become inconsistent with HEAD, so allow it.
@@ -805,7 +812,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		if (!ref_exists(branch_ref.buf)) {
 			strbuf_release(&branch_ref);
 
-			if (!argc)
+			if (!argc || !strcmp(head, branch_name))
 				return error(_("No commit on branch '%s' yet."),
 					     branch_name);
 			else
@@ -848,8 +855,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			die(_("no such branch '%s'"), argv[0]);
 		}
 
-		if (!ref_exists(branch->refname))
+		if (!ref_exists(branch->refname)) {
+			if (!argc || !strcmp(head, branch->name))
+				die(_("No commit on branch '%s' yet."), branch->name);
 			die(_("branch '%s' does not exist"), branch->name);
+		}
 
 		dwim_and_setup_tracking(the_repository, branch->name,
 					new_upstream, BRANCH_TRACK_OVERRIDE,
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index f2b9199007..ea7cfd1951 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -7,6 +7,28 @@ test_description='test show-branch'
 # arbitrary reference time: 2009-08-30 19:20:00
 GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW
 
+test_expect_success 'error descriptions on empty repository' '
+	current=$(git branch --show-current) &&
+	cat >expect <<-EOF &&
+	error: No commit on branch '\''$current'\'' yet.
+	EOF
+	test_must_fail git branch --edit-description 2>actual &&
+	test_cmp expect actual &&
+	test_must_fail git branch --edit-description $current 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'fatal descriptions on empty repository' '
+	current=$(git branch --show-current) &&
+	cat >expect <<-EOF &&
+	fatal: No commit on branch '\''$current'\'' yet.
+	EOF
+	test_must_fail git branch --set-upstream-to=non-existent 2>actual &&
+	test_cmp expect actual &&
+	test_must_fail git branch -c new-branch 2>actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'setup' '
 	test_commit initial &&
 	for i in $(test_seq 1 10)
@@ -175,4 +197,28 @@ done <<\EOF
 --reflog --current
 EOF
 
+test_expect_success 'error descriptions on non-existent branch' '
+	cat >expect <<-EOF &&
+	error: No branch named '\''non-existent'\'.'
+	EOF
+	test_must_fail git branch --edit-description non-existent 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'fatal descriptions on non-existent branch' '
+	cat >expect <<-EOF &&
+	fatal: branch '\''non-existent'\'' does not exist
+	EOF
+	test_must_fail git branch --set-upstream-to=non-existent non-existent 2>actual &&
+	test_cmp expect actual &&
+
+	cat >expect <<-EOF &&
+	fatal: No branch named '\''non-existent'\''.
+	EOF
+	test_must_fail git branch -c non-existent new-branch 2>actual &&
+	test_cmp expect actual &&
+	test_must_fail git branch -m non-existent new-branch 2>actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.32.0
Eric Sunshine· Oct 8, 2022, 03:27 UTC · re: Rubén Justo · lore

Re: [PATCH v3] branch: description for non-existent branch errors

On Fri, Oct 7, 2022 at 8:49 PM Rubén Justo <rjusto@gmail.com> wrote:
Show 13 quoted lines
> [...]
> Make the error descriptions for those branch operations with unborn or
> non-existent branches, more informative.
> [...]
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
> diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
> @@ -7,6 +7,28 @@ test_description='test show-branch'
> +test_expect_success 'error descriptions on empty repository' '
> +       current=$(git branch --show-current) &&
> +       cat >expect <<-EOF &&
> +       error: No commit on branch '\''$current'\'' yet.
> +       EOF

It's a matter of taste, but leaving and re-entering the single-quoted context, along with escaping, can make for a difficult read. You could instead take advantage of the SQ variable already defined by the test scripts:

    echo "error: No commit on branch $SQ$current$SQ yet." >expect &&
Not worth a re-roll, of course.
Show 5 quoted lines
> +test_expect_success 'fatal descriptions on empty repository' '
> +       current=$(git branch --show-current) &&
> +       cat >expect <<-EOF &&
> +       fatal: No commit on branch '\''$current'\'' yet.
> +       EOF
Ditto.
> +test_expect_success 'error descriptions on non-existent branch' '
> +       cat >expect <<-EOF &&
> +       error: No branch named '\''non-existent'\'.'
> +       EOF
Likewise.
> +test_expect_success 'fatal descriptions on non-existent branch' '
> +       cat >expect <<-EOF &&
> +       fatal: branch '\''non-existent'\'' does not exist
> +       EOF
Same.
Show 6 quoted lines
> +       test_must_fail git branch --set-upstream-to=non-existent non-existent 2>actual &&
> +       test_cmp expect actual &&
> +
> +       cat >expect <<-EOF &&
> +       fatal: No branch named '\''non-existent'\''.
> +       EOF
Again.
Rubén Justo· Oct 8, 2022, 08:54 UTC · re: Eric Sunshine · lore

Re: [PATCH v3] branch: description for non-existent branch errors

On 8/10/22 5:27, Eric Sunshine wrote:
Show 31 quoted lines
> On Fri, Oct 7, 2022 at 8:49 PM Rubén Justo <rjusto@gmail.com> wrote:
>> [...]
>> Make the error descriptions for those branch operations with unborn or
>> non-existent branches, more informative.
>> [...]
>> Signed-off-by: Rubén Justo <rjusto@gmail.com>
>> ---
>> diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
>> @@ -7,6 +7,28 @@ test_description='test show-branch'
>> +test_expect_success 'error descriptions on empty repository' '
>> +       current=$(git branch --show-current) &&
>> +       cat >expect <<-EOF &&
>> +       error: No commit on branch '\''$current'\'' yet.
>> +       EOF
> 
> It's a matter of taste, but leaving and re-entering the single-quoted
> context, along with escaping, can make for a difficult read. You could
> instead take advantage of the SQ variable already defined by the test
> scripts:
> 
>     echo "error: No commit on branch $SQ$current$SQ yet." >expect &&
> 
> Not worth a re-roll, of course.
> 
>> +test_expect_success 'fatal descriptions on empty repository' '
>> +       current=$(git branch --show-current) &&
>> +       cat >expect <<-EOF &&
>> +       fatal: No commit on branch '\''$current'\'' yet.
>> +       EOF
> 
> Ditto.
Thanks, I didn't know about $SQ.
	'\''$current'\'' vs $SQ$current$SQ vs ${SQ}$current${SQ}

I also find ugly that escaping, but I think is harder to read and error prone to use $SQ here.. :-/

Junio C Hamano· Oct 9, 2022, 05:05 UTC · re: Rubén Justo · lore

Re: [PATCH v3] branch: description for non-existent branch errors

Rubén Justo <rjusto@gmail.com> writes:
Show 6 quoted lines
> Thanks, I didn't know about $SQ.
>
> 	'\''$current'\'' vs $SQ$current$SQ vs ${SQ}$current${SQ}
>
> I also find ugly that escaping, but I think is harder to read and
> error prone to use $SQ here.. :-/

The ONLY case when $SQ shines is when the string that comes inside the single-quote pair begins with a non-alnum. $SQ$current$SQ is semi-readable, but if the string begins with an alnum, then you'd be forced to say ${SQ}current${SQ} (the first one must be inside braces because you do not want to refer to a variable whose name is SQcurrent, the second one wants to be inside braces for symmetry), which is ugly.

The rhythm of '\'' is not so bad, once you get used to seeing them. ${SQ}...${SQ} is a bit too loud.

← back to recent threads