* [PATCH] branch: description for non-existent branch errors @ 2022-09-22 22:37 Rubén Justo 2022-09-24 22:52 ` Rubén Justo 2022-09-30 22:47 ` [PATCH v2] " Rubén Justo 0 siblings, 2 replies; 16+ messages in thread From: Rubén Justo @ 2022-09-22 22:37 UTC (permalink / raw) To: git 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(-) 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH] branch: description for non-existent branch errors 2022-09-22 22:37 [PATCH] branch: description for non-existent branch errors Rubén Justo @ 2022-09-24 22:52 ` Rubén Justo 2022-09-26 18:12 ` Junio C Hamano 2022-09-30 22:47 ` [PATCH v2] " Rubén Justo 1 sibling, 1 reply; 16+ messages in thread From: Rubén Justo @ 2022-09-24 22:52 UTC (permalink / raw) To: git 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(-) 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] branch: description for non-existent branch errors 2022-09-24 22:52 ` Rubén Justo @ 2022-09-26 18:12 ` Junio C Hamano 2022-09-26 23:35 ` Rubén Justo 0 siblings, 1 reply; 16+ messages in thread From: Junio C Hamano @ 2022-09-26 18:12 UTC (permalink / raw) To: Rubén Justo; +Cc: git 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. > 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] branch: description for non-existent branch errors 2022-09-26 18:12 ` Junio C Hamano @ 2022-09-26 23:35 ` Rubén Justo 2022-09-27 22:24 ` Junio C Hamano 0 siblings, 1 reply; 16+ messages in thread From: Rubén Justo @ 2022-09-26 23:35 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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.. >> + 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] branch: description for non-existent branch errors 2022-09-26 23:35 ` Rubén Justo @ 2022-09-27 22:24 ` Junio C Hamano 2022-09-28 17:41 ` Junio C Hamano 2022-09-28 17:50 ` Junio C Hamano 0 siblings, 2 replies; 16+ messages in thread From: Junio C Hamano @ 2022-09-27 22:24 UTC (permalink / raw) To: Rubén Justo; +Cc: git 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 --- 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")); ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] branch: description for non-existent branch errors 2022-09-27 22:24 ` Junio C Hamano @ 2022-09-28 17:41 ` Junio C Hamano 2022-09-28 17:50 ` Junio C Hamano 1 sibling, 0 replies; 16+ messages in thread From: Junio C Hamano @ 2022-09-28 17:41 UTC (permalink / raw) To: Rubén Justo; +Cc: git Junio C Hamano <gitster@pobox.com> writes: > 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] branch: description for non-existent branch errors 2022-09-27 22:24 ` Junio C Hamano 2022-09-28 17:41 ` Junio C Hamano @ 2022-09-28 17:50 ` Junio C Hamano 2022-09-28 23:59 ` Rubén Justo 1 sibling, 1 reply; 16+ messages in thread From: Junio C Hamano @ 2022-09-28 17:50 UTC (permalink / raw) To: Rubén Justo; +Cc: git Junio C Hamano <gitster@pobox.com> writes: > 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] branch: description for non-existent branch errors 2022-09-28 17:50 ` Junio C Hamano @ 2022-09-28 23:59 ` Rubén Justo 2022-09-29 1:56 ` Junio C Hamano 0 siblings, 1 reply; 16+ messages in thread From: Rubén Justo @ 2022-09-28 23:59 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On 28/9/22 19:50, Junio C Hamano wrote: > 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] branch: description for non-existent branch errors 2022-09-28 23:59 ` Rubén Justo @ 2022-09-29 1:56 ` Junio C Hamano 0 siblings, 0 replies; 16+ messages in thread From: Junio C Hamano @ 2022-09-29 1:56 UTC (permalink / raw) To: Rubén Justo; +Cc: git 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2] branch: description for non-existent branch errors 2022-09-22 22:37 [PATCH] branch: description for non-existent branch errors Rubén Justo 2022-09-24 22:52 ` Rubén Justo @ 2022-09-30 22:47 ` Rubén Justo 2022-10-01 12:43 ` Bagas Sanjaya 2022-10-08 0:39 ` [PATCH v3] " Rubén Justo 1 sibling, 2 replies; 16+ messages in thread From: Rubén Justo @ 2022-09-30 22:47 UTC (permalink / raw) To: git 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(-) 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2] branch: description for non-existent branch errors 2022-09-30 22:47 ` [PATCH v2] " Rubén Justo @ 2022-10-01 12:43 ` Bagas Sanjaya 2022-10-02 21:28 ` Rubén Justo 2022-10-08 0:39 ` [PATCH v3] " Rubén Justo 1 sibling, 1 reply; 16+ messages in thread From: Bagas Sanjaya @ 2022-10-01 12:43 UTC (permalink / raw) To: Rubén Justo, git 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] branch: description for non-existent branch errors 2022-10-01 12:43 ` Bagas Sanjaya @ 2022-10-02 21:28 ` Rubén Justo 0 siblings, 0 replies; 16+ messages in thread From: Rubén Justo @ 2022-10-02 21:28 UTC (permalink / raw) To: Bagas Sanjaya, git On 1/10/22 14:43, Bagas Sanjaya wrote: > 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3] branch: description for non-existent branch errors 2022-09-30 22:47 ` [PATCH v2] " Rubén Justo 2022-10-01 12:43 ` Bagas Sanjaya @ 2022-10-08 0:39 ` Rubén Justo 2022-10-08 3:27 ` Eric Sunshine 1 sibling, 1 reply; 16+ messages in thread From: Rubén Justo @ 2022-10-08 0:39 UTC (permalink / raw) To: git; +Cc: Bagas Sanjaya, Junio C Hamano 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(-) 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v3] branch: description for non-existent branch errors 2022-10-08 0:39 ` [PATCH v3] " Rubén Justo @ 2022-10-08 3:27 ` Eric Sunshine 2022-10-08 8:54 ` Rubén Justo 0 siblings, 1 reply; 16+ messages in thread From: Eric Sunshine @ 2022-10-08 3:27 UTC (permalink / raw) To: Rubén Justo; +Cc: Git List, Bagas Sanjaya, Junio C Hamano 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. > +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. > + 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] branch: description for non-existent branch errors 2022-10-08 3:27 ` Eric Sunshine @ 2022-10-08 8:54 ` Rubén Justo 2022-10-09 5:05 ` Junio C Hamano 0 siblings, 1 reply; 16+ messages in thread From: Rubén Justo @ 2022-10-08 8:54 UTC (permalink / raw) To: Eric Sunshine; +Cc: Git List, Bagas Sanjaya, Junio C Hamano On 8/10/22 5:27, Eric Sunshine wrote: > 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.. :-/ ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] branch: description for non-existent branch errors 2022-10-08 8:54 ` Rubén Justo @ 2022-10-09 5:05 ` Junio C Hamano 0 siblings, 0 replies; 16+ messages in thread From: Junio C Hamano @ 2022-10-09 5:05 UTC (permalink / raw) To: Rubén Justo; +Cc: Eric Sunshine, Git List, Bagas Sanjaya Rubén Justo <rjusto@gmail.com> writes: > 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-10-09 5:12 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-22 22:37 [PATCH] branch: description for non-existent branch errors Rubén Justo 2022-09-24 22:52 ` Rubén Justo 2022-09-26 18:12 ` Junio C Hamano 2022-09-26 23:35 ` Rubén Justo 2022-09-27 22:24 ` Junio C Hamano 2022-09-28 17:41 ` Junio C Hamano 2022-09-28 17:50 ` Junio C Hamano 2022-09-28 23:59 ` Rubén Justo 2022-09-29 1:56 ` Junio C Hamano 2022-09-30 22:47 ` [PATCH v2] " Rubén Justo 2022-10-01 12:43 ` Bagas Sanjaya 2022-10-02 21:28 ` Rubén Justo 2022-10-08 0:39 ` [PATCH v3] " Rubén Justo 2022-10-08 3:27 ` Eric Sunshine 2022-10-08 8:54 ` Rubén Justo 2022-10-09 5:05 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).