| Message ID | 858edf12-67b1-5e2c-dd2e-3eb476530803@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | branch: description for non-existent branch errors | expand |
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.
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.
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"));
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.
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.
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.
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.
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,
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(-)