| Message ID | 8d627a2c-923f-181f-a03b-15f370c4dd0f@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | bcfc82bd48f518c8939090748516ad21bb079720 |
| Headers | show |
| Series | [v3] branch: description for non-existent branch errors | expand |
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.
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.. :-/
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.
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
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(-)