* [RFC PATCH v3 0/4] give more useful error messages while renaming branch @ 2017-11-02 6:52 Kaartic Sivaraam 2017-11-02 6:52 ` [RFC PATCH v3 1/4] branch: improve documentation and naming of 'create_branch()' Kaartic Sivaraam ` (4 more replies) 0 siblings, 5 replies; 12+ messages in thread From: Kaartic Sivaraam @ 2017-11-02 6:52 UTC (permalink / raw) To: git In builtin/branch, the error messages weren't handled directly by the branch renaming function and was left to the other function. Though this avoids redundancy this gave unclear error messages in some cases. So, make builtin/branch give more useful error messages. Changes in v3: Incorporated suggestions from v2 to improve code and commit message. To be more precise about the code part, In 2/4 slightly re-ordered the parameters to move the flag parameters to the end. In 3/4, changed the return type of the branchname validation functions to be the enum (whose values they return) as suggested by Stefan. Dropped the PATCH 3/5 of v2 as there was another series[1] that did the refactor and got merged to 'next'. I have now re-rolled the series over 'next' [pointing at 273055501 (Sync with master, 2017-10-24)]. This has made the code in 3/4 a little clumsy (at least to me) as I tried to achieve to achieve what the previous patches did with the new validate*_branchname functionS. Let me know, if it looks too bad. So this could go on top of 'next' without any conflicts but in case I missed something, let me know. The series could be found in my fork[2]. Any feedback welcome. Thanks, Kaartic [1] : https://public-inbox.org/git/20171013051132.3973-1-gitster@pobox.com [2] : https://github.com/sivaraam/git/tree/work/branch-revamp Kaartic Sivaraam (4): branch: improve documentation and naming of 'create_branch()' branch: re-order function arguments to group related arguments branch: introduce dont_fail parameter for branchname validation builtin/branch: give more useful error messages when renaming branch.c | 63 ++++++++++++++++++++++++++++++------------------------ branch.h | 57 ++++++++++++++++++++++++++++++++++++++---------- builtin/branch.c | 49 ++++++++++++++++++++++++++++++++++-------- builtin/checkout.c | 11 +++++----- 4 files changed, 127 insertions(+), 53 deletions(-) -- 2.15.0.rc2.401.g3db9995f9 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC PATCH v3 1/4] branch: improve documentation and naming of 'create_branch()' 2017-11-02 6:52 [RFC PATCH v3 0/4] give more useful error messages while renaming branch Kaartic Sivaraam @ 2017-11-02 6:52 ` Kaartic Sivaraam 2017-11-02 6:52 ` [RFC PATCH v3 2/4] branch: re-order function arguments to group related arguments Kaartic Sivaraam ` (3 subsequent siblings) 4 siblings, 0 replies; 12+ messages in thread From: Kaartic Sivaraam @ 2017-11-02 6:52 UTC (permalink / raw) To: git; +Cc: Kaartic Sivaraam From: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> The documentation for 'create_branch()' was incomplete as it didn't say what certain parameters were used for. Further a parameter name wasn't very communicative. So, add missing documentation for the sake of completeness and easy reference. Also, rename the concerned parameter to make its name more communicative. Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> --- branch.c | 4 ++-- branch.h | 7 ++++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/branch.c b/branch.c index fe1e1c367..ea6e2b359 100644 --- a/branch.c +++ b/branch.c @@ -244,7 +244,7 @@ N_("\n" "\"git push -u\" to set the upstream config as you push."); void create_branch(const char *name, const char *start_name, - int force, int reflog, int clobber_head, + int force, int reflog, int clobber_head_ok, int quiet, enum branch_track track) { struct commit *commit; @@ -258,7 +258,7 @@ void create_branch(const char *name, const char *start_name, if (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE) explicit_tracking = 1; - if ((track == BRANCH_TRACK_OVERRIDE || clobber_head) + if ((track == BRANCH_TRACK_OVERRIDE || clobber_head_ok) ? validate_branchname(name, &ref) : validate_new_branchname(name, &ref, force)) { if (!force) diff --git a/branch.h b/branch.h index be5e5d130..1512b78d1 100644 --- a/branch.h +++ b/branch.h @@ -15,12 +15,17 @@ * * - reflog creates a reflog for the branch * + * - clobber_head_ok allows the currently checked out (hence existing) + * branch to be overwritten; without 'force', it has no effect. + * + * - quiet suppresses tracking information + * * - track causes the new branch to be configured to merge the remote branch * that start_name is a tracking branch for (if any). */ void create_branch(const char *name, const char *start_name, int force, int reflog, - int clobber_head, int quiet, enum branch_track track); + int clobber_head_ok, int quiet, enum branch_track track); /* * Check if 'name' can be a valid name for a branch; die otherwise. -- 2.15.0.461.gf957c703b.dirty ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [RFC PATCH v3 2/4] branch: re-order function arguments to group related arguments 2017-11-02 6:52 [RFC PATCH v3 0/4] give more useful error messages while renaming branch Kaartic Sivaraam 2017-11-02 6:52 ` [RFC PATCH v3 1/4] branch: improve documentation and naming of 'create_branch()' Kaartic Sivaraam @ 2017-11-02 6:52 ` Kaartic Sivaraam 2017-11-02 6:52 ` [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation Kaartic Sivaraam ` (2 subsequent siblings) 4 siblings, 0 replies; 12+ messages in thread From: Kaartic Sivaraam @ 2017-11-02 6:52 UTC (permalink / raw) To: git; +Cc: Kaartic Sivaraam From: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> The ad-hoc patches to add new arguments to a function when needed resulted in the related arguments not being close to each other. This misleads the person reading the code to believe that there isn't much relation between those arguments while it's not the case. So, re-order the arguments to keep the related arguments close to each other. Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> --- branch.c | 4 ++-- branch.h | 14 +++++++------- builtin/branch.c | 4 ++-- builtin/checkout.c | 6 +++--- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/branch.c b/branch.c index ea6e2b359..7c8093041 100644 --- a/branch.c +++ b/branch.c @@ -244,8 +244,8 @@ N_("\n" "\"git push -u\" to set the upstream config as you push."); void create_branch(const char *name, const char *start_name, - int force, int reflog, int clobber_head_ok, - int quiet, enum branch_track track) + enum branch_track track, int force, int clobber_head_ok, + int reflog, int quiet) { struct commit *commit; struct object_id oid; diff --git a/branch.h b/branch.h index 1512b78d1..85052628b 100644 --- a/branch.h +++ b/branch.h @@ -11,21 +11,21 @@ * - start_name is the name of the existing branch that the new branch should * start from * - * - force enables overwriting an existing (non-head) branch + * - track causes the new branch to be configured to merge the remote branch + * that start_name is a tracking branch for (if any). * - * - reflog creates a reflog for the branch + * - force enables overwriting an existing (non-head) branch * * - clobber_head_ok allows the currently checked out (hence existing) * branch to be overwritten; without 'force', it has no effect. * - * - quiet suppresses tracking information + * - reflog creates a reflog for the branch * - * - track causes the new branch to be configured to merge the remote branch - * that start_name is a tracking branch for (if any). + * - quiet suppresses tracking information */ void create_branch(const char *name, const char *start_name, - int force, int reflog, - int clobber_head_ok, int quiet, enum branch_track track); + enum branch_track track, int force, int clobber_head_ok, + int reflog, int quiet); /* * Check if 'name' can be a valid name for a branch; die otherwise. diff --git a/builtin/branch.c b/builtin/branch.c index 5be40b384..df06ac968 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -766,7 +766,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) * create_branch takes care of setting up the tracking * info and making sure new_upstream is correct */ - create_branch(branch->name, new_upstream, 0, 0, 0, quiet, BRANCH_TRACK_OVERRIDE); + create_branch(branch->name, new_upstream, BRANCH_TRACK_OVERRIDE, 0, 0, 0, quiet); } else if (unset_upstream) { struct branch *branch = branch_get(argv[0]); struct strbuf buf = STRBUF_INIT; @@ -806,7 +806,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead.")); create_branch(argv[0], (argc == 2) ? argv[1] : head, - force, reflog, 0, quiet, track); + track, force, 0, reflog, quiet); } else usage_with_options(builtin_branch_usage, options); diff --git a/builtin/checkout.c b/builtin/checkout.c index 8546d630b..5c34a9a0d 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -639,11 +639,11 @@ static void update_refs_for_switch(const struct checkout_opts *opts, } else create_branch(opts->new_branch, new->name, + opts->track, + opts->new_branch_force ? 1 : 0, opts->new_branch_force ? 1 : 0, opts->new_branch_log, - opts->new_branch_force ? 1 : 0, - opts->quiet, - opts->track); + opts->quiet); new->name = opts->new_branch; setup_branch_path(new); } -- 2.15.0.461.gf957c703b.dirty ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation 2017-11-02 6:52 [RFC PATCH v3 0/4] give more useful error messages while renaming branch Kaartic Sivaraam 2017-11-02 6:52 ` [RFC PATCH v3 1/4] branch: improve documentation and naming of 'create_branch()' Kaartic Sivaraam 2017-11-02 6:52 ` [RFC PATCH v3 2/4] branch: re-order function arguments to group related arguments Kaartic Sivaraam @ 2017-11-02 6:52 ` Kaartic Sivaraam 2017-11-02 6:52 ` [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming Kaartic Sivaraam 2017-11-02 6:55 ` [RFC PATCH v3 0/4] give more useful error messages while renaming branch Kaartic Sivaraam 4 siblings, 0 replies; 12+ messages in thread From: Kaartic Sivaraam @ 2017-11-02 6:52 UTC (permalink / raw) To: git; +Cc: Kaartic Sivaraam This parameter allows the branchname validation functions to optionally return a flag specifying the reason for failure, when requested. This allows the caller to know why it was about to die. This allows more useful error messages to be given to the user when trying to rename a branch. The flags are specified in the form of an enum and values for success flags have been assigned explicitly to clearly express that certain callers rely on those values and they cannot be arbitrary. Only the logic has been added but no caller has been made to use it, yet. So, no functional changes. Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> --- branch.c | 62 +++++++++++++++++++++++++++++++----------------------- branch.h | 60 ++++++++++++++++++++++++++++++++++++++++++---------- builtin/branch.c | 4 ++-- builtin/checkout.c | 5 +++-- 4 files changed, 90 insertions(+), 41 deletions(-) diff --git a/branch.c b/branch.c index 7c8093041..7db2e3296 100644 --- a/branch.c +++ b/branch.c @@ -178,41 +178,51 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name) return 0; } -/* - * Check if 'name' can be a valid name for a branch; die otherwise. - * Return 1 if the named branch already exists; return 0 otherwise. - * Fill ref with the full refname for the branch. - */ -int validate_branchname(const char *name, struct strbuf *ref) +enum branch_validation_result validate_branchname(const char *name, struct strbuf *ref, unsigned dont_fail) { - if (strbuf_check_branch_ref(ref, name)) - die(_("'%s' is not a valid branch name."), name); + if (strbuf_check_branch_ref(ref, name)) { + if (dont_fail) + return INVALID_BRANCH_NAME; + else + die(_("'%s' is not a valid branch name."), name); + } - return ref_exists(ref->buf); + if(ref_exists(ref->buf)) + return BRANCH_EXISTS; + else + return BRANCH_DOESNT_EXIST; } -/* - * Check if a branch 'name' can be created as a new branch; die otherwise. - * 'force' can be used when it is OK for the named branch already exists. - * Return 1 if the named branch already exists; return 0 otherwise. - * Fill ref with the full refname for the branch. - */ -int validate_new_branchname(const char *name, struct strbuf *ref, int force) +enum branch_validation_result validate_new_branchname(const char *name, struct strbuf *ref, int force, unsigned dont_fail) { const char *head; - if (!validate_branchname(name, ref)) - return 0; + if(dont_fail) { + enum branch_validation_result res = validate_branchname(name, ref, 1); + if (res == INVALID_BRANCH_NAME || res == BRANCH_DOESNT_EXIST) + return res; + } else { + if(validate_branchname(name, ref, 0) == BRANCH_DOESNT_EXIST) + return BRANCH_DOESNT_EXIST; + } - if (!force) - die(_("A branch named '%s' already exists."), - ref->buf + strlen("refs/heads/")); + if (!force) { + if (dont_fail) + return BRANCH_EXISTS_NO_FORCE; + else + die(_("A branch named '%s' already exists."), + ref->buf + strlen("refs/heads/")); + } head = resolve_ref_unsafe("HEAD", 0, NULL, NULL); - if (!is_bare_repository() && head && !strcmp(head, ref->buf)) - die(_("Cannot force update the current branch.")); + if (!is_bare_repository() && head && !strcmp(head, ref->buf)) { + if (dont_fail) + return CANNOT_FORCE_UPDATE_CURRENT_BRANCH; + else + die(_("Cannot force update the current branch.")); + } - return 1; + return BRANCH_EXISTS; } static int check_tracking_branch(struct remote *remote, void *cb_data) @@ -259,8 +269,8 @@ void create_branch(const char *name, const char *start_name, explicit_tracking = 1; if ((track == BRANCH_TRACK_OVERRIDE || clobber_head_ok) - ? validate_branchname(name, &ref) - : validate_new_branchname(name, &ref, force)) { + ? validate_branchname(name, &ref, 0) + : validate_new_branchname(name, &ref, force, 0)) { if (!force) dont_change_ref = 1; else diff --git a/branch.h b/branch.h index 85052628b..0c178ec5a 100644 --- a/branch.h +++ b/branch.h @@ -27,20 +27,58 @@ void create_branch(const char *name, const char *start_name, enum branch_track track, int force, int clobber_head_ok, int reflog, int quiet); -/* - * Check if 'name' can be a valid name for a branch; die otherwise. - * Return 1 if the named branch already exists; return 0 otherwise. - * Fill ref with the full refname for the branch. - */ -extern int validate_branchname(const char *name, struct strbuf *ref); +enum branch_validation_result { + /* Flags that convey that validation FAILED */ + BRANCH_EXISTS_NO_FORCE = -3, + CANNOT_FORCE_UPDATE_CURRENT_BRANCH, + INVALID_BRANCH_NAME, + /* Flags that convey that validation SUCCEEDED */ + BRANCH_DOESNT_EXIST = 0, + BRANCH_EXISTS = 1, +}; /* - * Check if a branch 'name' can be created as a new branch; die otherwise. - * 'force' can be used when it is OK for the named branch already exists. - * Return 1 if the named branch already exists; return 0 otherwise. - * Fill ref with the full refname for the branch. + * Check if 'name' can be a valid name for a branch; die otherwise. + * + * - name is the new branch name + * + * - ref is used to return the full refname for the branch + * + * The return values have the following meaning, + * + * - If dont_fail is 0, the function dies in case of failure and returns flags of + * 'branch_validation_result' that indicate status of the given branch. The positive + * non-zero flag implies that the branch exists. + * + * - If dont_fail is 1, the function doesn't die in case of failure but returns flags + * of 'branch_validaton_result' that specify the reason for failure. The behaviour in case of + * success is same as above. + * */ -extern int validate_new_branchname(const char *name, struct strbuf *ref, int force); +extern enum branch_validation_result validate_branchname(const char *name, struct strbuf *ref, unsigned dont_fail); + +/* + * Check if a branch 'name' can be created as a new branch. + * + * - name is the new branch name + * + * - ref is used to return the full refname for the branch + * + * - force can be used when it is OK if the named branch already exists. + * the currently checkout branch; with 'shouldnt_exist', it has no effect. + * + * The return values have the following meaning, + * + * - If dont_fail is 0, the function dies in case of failure and returns flags of + * 'branch_validation_result' that indicate that convey status of given branch. The positive + * non-zero flag implies that the branch can be force updated. + * + * - If dont_fail is 1, the function doesn't die in case of failure but returns flags + * of 'branch_validaton_result' that specify the reason for failure. The behaviour in case of + * success is same as above. + * + */ +extern enum branch_validation_result validate_new_branchname(const char *name, struct strbuf *ref, int force, unsigned dont_fail); /* * Remove information about the state of working on the current diff --git a/builtin/branch.c b/builtin/branch.c index df06ac968..7018e5d75 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -487,9 +487,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int * cause the worktree to become inconsistent with HEAD, so allow it. */ if (!strcmp(oldname, newname)) - validate_branchname(newname, &newref); + validate_branchname(newname, &newref, 0); else - validate_new_branchname(newname, &newref, force); + validate_new_branchname(newname, &newref, force, 0); reject_rebase_or_bisect_branch(oldref.buf); diff --git a/builtin/checkout.c b/builtin/checkout.c index 5c34a9a0d..4adab3814 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1288,10 +1288,11 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) struct strbuf buf = STRBUF_INIT; if (opts.new_branch_force) - opts.branch_exists = validate_branchname(opts.new_branch, &buf); + opts.branch_exists = validate_branchname(opts.new_branch, &buf, 0); else opts.branch_exists = - validate_new_branchname(opts.new_branch, &buf, 0); + validate_new_branchname(opts.new_branch, &buf, 0, 0); + strbuf_release(&buf); } -- 2.15.0.461.gf957c703b.dirty ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming 2017-11-02 6:52 [RFC PATCH v3 0/4] give more useful error messages while renaming branch Kaartic Sivaraam ` (2 preceding siblings ...) 2017-11-02 6:52 ` [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation Kaartic Sivaraam @ 2017-11-02 6:52 ` Kaartic Sivaraam 2017-11-02 6:55 ` [RFC PATCH v3 0/4] give more useful error messages while renaming branch Kaartic Sivaraam 4 siblings, 0 replies; 12+ messages in thread From: Kaartic Sivaraam @ 2017-11-02 6:52 UTC (permalink / raw) To: git; +Cc: Kaartic Sivaraam From: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> When trying to rename an inexistent branch to with a name of a branch that already exists the rename failed specifying the new branch name exists rather than specifying that the branch trying to be renamed doesn't exist. $ git branch -m tset master fatal: A branch named 'master' already exists. It's conventional to report that 'tset' doesn't exist rather than reporting that 'master' exists, the same way the 'mv' command does. (hypothetical) $ git branch -m tset master fatal: branch 'tset' doesn't exist. That has the problem that the error about an existing branch is shown only after the user corrects the error about inexistent branch. $ git branch -m test master fatal: A branch named 'master' already exists. This isn't useful either because the user would have corrected this error in a single go if he had been told this alongside the first error. So, give more useful error messages by giving errors about old branch name and new branch name at the same time. This is possible as the branch name validation functions now return the reason they were about to die, when requested. $ git branch -m tset master fatal: branch 'tset' doesn't exist, and branch 'master' already exists Note: Thanks to the strbuf API that made it possible to easily construct the composite error message strings! Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> --- builtin/branch.c | 49 ++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 42 insertions(+), 7 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index 7018e5d75..c2bbf8c3d 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -458,11 +458,42 @@ static void reject_rebase_or_bisect_branch(const char *target) free_worktrees(worktrees); } +static void get_error_msg(struct strbuf* error_msg, const char* oldname, unsigned old_branch_exists, + const char* newname, enum branch_validation_result res) +{ + const char* connector_string = _(", and "); + + if (!old_branch_exists) { + strbuf_addf(error_msg, _("branch '%s' doesn't exist"), oldname); + } + + switch (res) { + case BRANCH_EXISTS_NO_FORCE: + strbuf_addf(error_msg, "%s", (!old_branch_exists) ? connector_string : ""); + strbuf_addf(error_msg,_("branch '%s' already exists"), newname); + break; + case CANNOT_FORCE_UPDATE_CURRENT_BRANCH: + strbuf_addf(error_msg, "%s", (!old_branch_exists) ? connector_string : ""); + strbuf_addstr(error_msg, _("cannot force update the current branch")); + break; + case INVALID_BRANCH_NAME: + strbuf_addf(error_msg, "%s", (!old_branch_exists) ? connector_string : ""); + strbuf_addf(error_msg, _("branch name '%s' is invalid"), newname); + break; + /* not necessary to handle success cases */ + case BRANCH_EXISTS: + case BRANCH_DOESNT_EXIST: + break; + } +} + static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force) { struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = STRBUF_INIT; struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT; int recovery = 0; + struct strbuf error_msg = STRBUF_INIT, empty = STRBUF_INIT; + enum branch_validation_result res; if (!oldname) { if (copy) @@ -471,15 +502,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int die(_("cannot rename the current branch while not on any.")); } - if (strbuf_check_branch_ref(&oldref, oldname)) { + if (strbuf_check_branch_ref(&oldref, oldname) && ref_exists(oldref.buf)) + { /* * Bad name --- this could be an attempt to rename a * ref that we used to allow to be created by accident. */ - if (ref_exists(oldref.buf)) - recovery = 1; - else - die(_("Invalid branch name: '%s'"), oldname); + recovery = 1; } /* @@ -487,9 +516,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int * cause the worktree to become inconsistent with HEAD, so allow it. */ if (!strcmp(oldname, newname)) - validate_branchname(newname, &newref, 0); + res = validate_branchname(newname, &newref, 1); else - validate_new_branchname(newname, &newref, force, 0); + res = validate_new_branchname(newname, &newref, force, 1); + + get_error_msg(&error_msg, oldname, ref_exists(oldref.buf), newname, res); + if (strbuf_cmp(&error_msg, &empty)) + die("%s", error_msg.buf); reject_rebase_or_bisect_branch(oldref.buf); @@ -530,6 +563,8 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int die(_("Branch is copied, but update of config-file failed")); strbuf_release(&oldsection); strbuf_release(&newsection); + strbuf_release(&error_msg); + strbuf_release(&empty); } static GIT_PATH_FUNC(edit_description, "EDIT_DESCRIPTION") -- 2.15.0.461.gf957c703b.dirty ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [RFC PATCH v3 0/4] give more useful error messages while renaming branch 2017-11-02 6:52 [RFC PATCH v3 0/4] give more useful error messages while renaming branch Kaartic Sivaraam ` (3 preceding siblings ...) 2017-11-02 6:52 ` [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming Kaartic Sivaraam @ 2017-11-02 6:55 ` Kaartic Sivaraam 4 siblings, 0 replies; 12+ messages in thread From: Kaartic Sivaraam @ 2017-11-02 6:55 UTC (permalink / raw) To: git Sorry, ignore this mails. I actually have re-sent it to the correct thread. -- Kaartic ^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC PATCH v2 0/5] Give more useful error messages when renaming a branch @ 2017-09-25 8:20 Kaartic Sivaraam 2017-11-02 6:54 ` [RFC PATCH v3 0/4] give more useful error messages while renaming branch Kaartic Sivaraam 0 siblings, 1 reply; 12+ messages in thread From: Kaartic Sivaraam @ 2017-09-25 8:20 UTC (permalink / raw) To: gitster; +Cc: git, sbeller [1/5] and [2/5] - cleanup patches [3/5] - refactor of a function that seemed a little unintuitive [4/5] - just a preparatory step adding part of the logic [5/5] - main patch that tries to improve the error messages Changes in v2: - dropped [1/5] of v1 [1/5] - incroporated suggestions of Junio [2/5] - new in this series - just a little thing I thought could improve things [3/5] - used the name 'validate_branch_creation' for the refactor instead of 'validate_branch_update' - changed the parameter names [5/5] - In v1 I added the connector string and removed it when it wasn't necessary. It's not possible to do that as that string has to be translated too and it won't have the same number of characters in other languages. So, I switched to adding the connector only when needed. Though this seems to introduce some redundancy, I find it to be the only reliable way. - tweaked commit message a little as suggested by Stefan There is no change in the error messages themselves. So the sample input output interaction sent for v1 still holds. This series applies on top of 'master' and can be found in my fork[1]. In case you were wondering, the Travis-CI tests did pass. [1]: https://github.com/sivaraam/git/tree/work/branch-move-revamp [2]: https://travis-ci.org/sivaraam/git/builds/279199977 Kaartic Sivaraam (5): branch: improve documentation and naming of certain parameters branch: re-order function arguments to group related arguments branch: cleanup branch name validation branch: introduce dont_fail parameter for create validation builtin/branch: give more useful error messages when renaming branch.c | 69 +++++++++++++++++++++++++++++++++++++++--------------- branch.h | 53 ++++++++++++++++++++++++++++++----------- builtin/branch.c | 43 ++++++++++++++++++++++++++++------ builtin/checkout.c | 8 +++---- t/t3200-branch.sh | 4 ++++ 5 files changed, 133 insertions(+), 44 deletions(-) -- 2.14.1.935.ge2b2bcd8a ^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC PATCH v3 0/4] give more useful error messages while renaming branch 2017-09-25 8:20 [RFC PATCH v2 0/5] Give more useful error messages when renaming a branch Kaartic Sivaraam @ 2017-11-02 6:54 ` Kaartic Sivaraam 2017-11-02 6:54 ` [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation Kaartic Sivaraam 0 siblings, 1 reply; 12+ messages in thread From: Kaartic Sivaraam @ 2017-11-02 6:54 UTC (permalink / raw) To: git In builtin/branch, the error messages weren't handled directly by the branch renaming function and was left to the other function. Though this avoids redundancy this gave unclear error messages in some cases. So, make builtin/branch give more useful error messages. Changes in v3: Incorporated suggestions from v2 to improve code and commit message. To be more precise about the code part, In 2/4 slightly re-ordered the parameters to move the flag parameters to the end. In 3/4, changed the return type of the branchname validation functions to be the enum (whose values they return) as suggested by Stefan. Dropped the PATCH 3/5 of v2 as there was another series[1] that did the refactor and got merged to 'next'. I have now re-rolled the series over 'next' [pointing at 273055501 (Sync with master, 2017-10-24)]. This has made the code in 3/4 a little clumsy (at least to me) as I tried to achieve to achieve what the previous patches did with the new validate*_branchname functionS. Let me know, if it looks too bad. So this could go on top of 'next' without any conflicts but in case I missed something, let me know. The series could be found in my fork[2]. Any feedback welcome. Thanks, Kaartic [1] : https://public-inbox.org/git/20171013051132.3973-1-gitster@pobox.com [2] : https://github.com/sivaraam/git/tree/work/branch-revamp Kaartic Sivaraam (4): branch: improve documentation and naming of 'create_branch()' branch: re-order function arguments to group related arguments branch: introduce dont_fail parameter for branchname validation builtin/branch: give more useful error messages when renaming branch.c | 63 ++++++++++++++++++++++++++++++------------------------ branch.h | 57 ++++++++++++++++++++++++++++++++++++++---------- builtin/branch.c | 49 ++++++++++++++++++++++++++++++++++-------- builtin/checkout.c | 11 +++++----- 4 files changed, 127 insertions(+), 53 deletions(-) -- 2.15.0.rc2.401.g3db9995f9 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation 2017-11-02 6:54 ` [RFC PATCH v3 0/4] give more useful error messages while renaming branch Kaartic Sivaraam @ 2017-11-02 6:54 ` Kaartic Sivaraam 2017-11-02 8:39 ` Kaartic Sivaraam 2017-11-06 2:24 ` Junio C Hamano 0 siblings, 2 replies; 12+ messages in thread From: Kaartic Sivaraam @ 2017-11-02 6:54 UTC (permalink / raw) To: git; +Cc: Kaartic Sivaraam This parameter allows the branchname validation functions to optionally return a flag specifying the reason for failure, when requested. This allows the caller to know why it was about to die. This allows more useful error messages to be given to the user when trying to rename a branch. The flags are specified in the form of an enum and values for success flags have been assigned explicitly to clearly express that certain callers rely on those values and they cannot be arbitrary. Only the logic has been added but no caller has been made to use it, yet. So, no functional changes. Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> --- branch.c | 62 +++++++++++++++++++++++++++++++----------------------- branch.h | 60 ++++++++++++++++++++++++++++++++++++++++++---------- builtin/branch.c | 4 ++-- builtin/checkout.c | 5 +++-- 4 files changed, 90 insertions(+), 41 deletions(-) diff --git a/branch.c b/branch.c index 7c8093041..7db2e3296 100644 --- a/branch.c +++ b/branch.c @@ -178,41 +178,51 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name) return 0; } -/* - * Check if 'name' can be a valid name for a branch; die otherwise. - * Return 1 if the named branch already exists; return 0 otherwise. - * Fill ref with the full refname for the branch. - */ -int validate_branchname(const char *name, struct strbuf *ref) +enum branch_validation_result validate_branchname(const char *name, struct strbuf *ref, unsigned dont_fail) { - if (strbuf_check_branch_ref(ref, name)) - die(_("'%s' is not a valid branch name."), name); + if (strbuf_check_branch_ref(ref, name)) { + if (dont_fail) + return INVALID_BRANCH_NAME; + else + die(_("'%s' is not a valid branch name."), name); + } - return ref_exists(ref->buf); + if(ref_exists(ref->buf)) + return BRANCH_EXISTS; + else + return BRANCH_DOESNT_EXIST; } -/* - * Check if a branch 'name' can be created as a new branch; die otherwise. - * 'force' can be used when it is OK for the named branch already exists. - * Return 1 if the named branch already exists; return 0 otherwise. - * Fill ref with the full refname for the branch. - */ -int validate_new_branchname(const char *name, struct strbuf *ref, int force) +enum branch_validation_result validate_new_branchname(const char *name, struct strbuf *ref, int force, unsigned dont_fail) { const char *head; - if (!validate_branchname(name, ref)) - return 0; + if(dont_fail) { + enum branch_validation_result res = validate_branchname(name, ref, 1); + if (res == INVALID_BRANCH_NAME || res == BRANCH_DOESNT_EXIST) + return res; + } else { + if(validate_branchname(name, ref, 0) == BRANCH_DOESNT_EXIST) + return BRANCH_DOESNT_EXIST; + } - if (!force) - die(_("A branch named '%s' already exists."), - ref->buf + strlen("refs/heads/")); + if (!force) { + if (dont_fail) + return BRANCH_EXISTS_NO_FORCE; + else + die(_("A branch named '%s' already exists."), + ref->buf + strlen("refs/heads/")); + } head = resolve_ref_unsafe("HEAD", 0, NULL, NULL); - if (!is_bare_repository() && head && !strcmp(head, ref->buf)) - die(_("Cannot force update the current branch.")); + if (!is_bare_repository() && head && !strcmp(head, ref->buf)) { + if (dont_fail) + return CANNOT_FORCE_UPDATE_CURRENT_BRANCH; + else + die(_("Cannot force update the current branch.")); + } - return 1; + return BRANCH_EXISTS; } static int check_tracking_branch(struct remote *remote, void *cb_data) @@ -259,8 +269,8 @@ void create_branch(const char *name, const char *start_name, explicit_tracking = 1; if ((track == BRANCH_TRACK_OVERRIDE || clobber_head_ok) - ? validate_branchname(name, &ref) - : validate_new_branchname(name, &ref, force)) { + ? validate_branchname(name, &ref, 0) + : validate_new_branchname(name, &ref, force, 0)) { if (!force) dont_change_ref = 1; else diff --git a/branch.h b/branch.h index 85052628b..0c178ec5a 100644 --- a/branch.h +++ b/branch.h @@ -27,20 +27,58 @@ void create_branch(const char *name, const char *start_name, enum branch_track track, int force, int clobber_head_ok, int reflog, int quiet); -/* - * Check if 'name' can be a valid name for a branch; die otherwise. - * Return 1 if the named branch already exists; return 0 otherwise. - * Fill ref with the full refname for the branch. - */ -extern int validate_branchname(const char *name, struct strbuf *ref); +enum branch_validation_result { + /* Flags that convey that validation FAILED */ + BRANCH_EXISTS_NO_FORCE = -3, + CANNOT_FORCE_UPDATE_CURRENT_BRANCH, + INVALID_BRANCH_NAME, + /* Flags that convey that validation SUCCEEDED */ + BRANCH_DOESNT_EXIST = 0, + BRANCH_EXISTS = 1, +}; /* - * Check if a branch 'name' can be created as a new branch; die otherwise. - * 'force' can be used when it is OK for the named branch already exists. - * Return 1 if the named branch already exists; return 0 otherwise. - * Fill ref with the full refname for the branch. + * Check if 'name' can be a valid name for a branch; die otherwise. + * + * - name is the new branch name + * + * - ref is used to return the full refname for the branch + * + * The return values have the following meaning, + * + * - If dont_fail is 0, the function dies in case of failure and returns flags of + * 'branch_validation_result' that indicate status of the given branch. The positive + * non-zero flag implies that the branch exists. + * + * - If dont_fail is 1, the function doesn't die in case of failure but returns flags + * of 'branch_validaton_result' that specify the reason for failure. The behaviour in case of + * success is same as above. + * */ -extern int validate_new_branchname(const char *name, struct strbuf *ref, int force); +extern enum branch_validation_result validate_branchname(const char *name, struct strbuf *ref, unsigned dont_fail); + +/* + * Check if a branch 'name' can be created as a new branch. + * + * - name is the new branch name + * + * - ref is used to return the full refname for the branch + * + * - force can be used when it is OK if the named branch already exists. + * the currently checkout branch; with 'shouldnt_exist', it has no effect. + * + * The return values have the following meaning, + * + * - If dont_fail is 0, the function dies in case of failure and returns flags of + * 'branch_validation_result' that indicate that convey status of given branch. The positive + * non-zero flag implies that the branch can be force updated. + * + * - If dont_fail is 1, the function doesn't die in case of failure but returns flags + * of 'branch_validaton_result' that specify the reason for failure. The behaviour in case of + * success is same as above. + * + */ +extern enum branch_validation_result validate_new_branchname(const char *name, struct strbuf *ref, int force, unsigned dont_fail); /* * Remove information about the state of working on the current diff --git a/builtin/branch.c b/builtin/branch.c index df06ac968..7018e5d75 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -487,9 +487,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int * cause the worktree to become inconsistent with HEAD, so allow it. */ if (!strcmp(oldname, newname)) - validate_branchname(newname, &newref); + validate_branchname(newname, &newref, 0); else - validate_new_branchname(newname, &newref, force); + validate_new_branchname(newname, &newref, force, 0); reject_rebase_or_bisect_branch(oldref.buf); diff --git a/builtin/checkout.c b/builtin/checkout.c index 5c34a9a0d..4adab3814 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1288,10 +1288,11 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) struct strbuf buf = STRBUF_INIT; if (opts.new_branch_force) - opts.branch_exists = validate_branchname(opts.new_branch, &buf); + opts.branch_exists = validate_branchname(opts.new_branch, &buf, 0); else opts.branch_exists = - validate_new_branchname(opts.new_branch, &buf, 0); + validate_new_branchname(opts.new_branch, &buf, 0, 0); + strbuf_release(&buf); } -- 2.15.0.461.gf957c703b.dirty ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation 2017-11-02 6:54 ` [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation Kaartic Sivaraam @ 2017-11-02 8:39 ` Kaartic Sivaraam 2017-11-02 18:42 ` Stefan Beller 2017-11-06 2:24 ` Junio C Hamano 1 sibling, 1 reply; 12+ messages in thread From: Kaartic Sivaraam @ 2017-11-02 8:39 UTC (permalink / raw) To: git; +Cc: Stefan Beller, Junio C Hamano On Thursday 02 November 2017 12:24 PM, Kaartic Sivaraam wrote: > Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> > Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> I just now saw this small glitch as a consequence of recently changing my email address. I would prefer to keep the second one but as the other patches have the first one it's better to keep the first one for now. But wait, it seems that this commit also has a different author identity (the email adress part). If this is a big enough issue, I'll fix that and send a v4 (possibly with any other suggested changes) else I'll leave it as it is. BTW, I added the Ccs to this mail which I forgot to do when sending the patches, hope it's not an issue. --- Kaartic ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation 2017-11-02 8:39 ` Kaartic Sivaraam @ 2017-11-02 18:42 ` Stefan Beller 2017-11-03 2:58 ` Kaartic Sivaraam 0 siblings, 1 reply; 12+ messages in thread From: Stefan Beller @ 2017-11-02 18:42 UTC (permalink / raw) To: Kaartic Sivaraam; +Cc: git, Junio C Hamano On Thu, Nov 2, 2017 at 1:39 AM, Kaartic Sivaraam <kaartic.sivaraam@gmail.com> wrote: > On Thursday 02 November 2017 12:24 PM, Kaartic Sivaraam wrote: >> >> Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> >> Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> > > > I just now saw this small glitch as a consequence of recently > changing my email address. I would prefer to keep the second one > but as the other patches have the first one it's better to keep > the first one for now. If you prefer one of them, you may have incentive to add an entry into .mailmap file, otherwise I'd kindly ask you to. :) (c.f. `git log --no-merges -- .mailmap`) > But wait, it seems that this commit also has a different author > identity (the email adress part). If this is a big enough issue, > I'll fix that and send a v4 (possibly with any other suggested > changes) else I'll leave it as it is. > > BTW, I added the Ccs to this mail which I forgot to do when > sending the patches, hope it's not an issue. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation 2017-11-02 18:42 ` Stefan Beller @ 2017-11-03 2:58 ` Kaartic Sivaraam 0 siblings, 0 replies; 12+ messages in thread From: Kaartic Sivaraam @ 2017-11-03 2:58 UTC (permalink / raw) To: Stefan Beller; +Cc: git, Junio C Hamano On Friday 03 November 2017 12:12 AM, Stefan Beller wrote: > On Thu, Nov 2, 2017 at 1:39 AM, Kaartic Sivaraam > <kaartic.sivaraam@gmail.com> wrote: >> On Thursday 02 November 2017 12:24 PM, Kaartic Sivaraam wrote: >>> >>> Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> >>> Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> >> >> >> I just now saw this small glitch as a consequence of recently >> changing my email address. I would prefer to keep the second one >> but as the other patches have the first one it's better to keep >> the first one for now. > > If you prefer one of them, you may have incentive to > add an entry into .mailmap file, otherwise I'd kindly ask you > to. :) (c.f. `git log --no-merges -- .mailmap`) > Sure, I'll do that. My intuition says this doesn't remove the duplicated sign-off line. Anyways, there's for sure a v4 that's going to update the connector string in [4/4] and another update. I'll be careful to address these issues in v4. --- Kaartic ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation 2017-11-02 6:54 ` [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation Kaartic Sivaraam 2017-11-02 8:39 ` Kaartic Sivaraam @ 2017-11-06 2:24 ` Junio C Hamano 2017-11-12 13:33 ` Kaartic Sivaraam 1 sibling, 1 reply; 12+ messages in thread From: Junio C Hamano @ 2017-11-06 2:24 UTC (permalink / raw) To: Kaartic Sivaraam; +Cc: git, Kaartic Sivaraam Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes: > This parameter allows the branchname validation functions to > optionally return a flag specifying the reason for failure, when > requested. This allows the caller to know why it was about to die. > This allows more useful error messages to be given to the user when > trying to rename a branch. > > The flags are specified in the form of an enum and values for success > flags have been assigned explicitly to clearly express that certain > callers rely on those values and they cannot be arbitrary. > > Only the logic has been added but no caller has been made to use it, yet. > So, no functional changes. This step makes sense, and nicely done. We usually use the word "gently" to when we enhance an operation that used to always die on failure. When there are tons of callsite to the original operation F(), we introduce F_gently() variant and do something like F(...) { if (F_gently(...)) die(...); } so that the callers do not have to change. If there aren't that many, it is OK to change the function signature of F() to tell it not to die without adding a new F_gently() function, which is the approach more appropriate for this change. The extra parameter used for that purpose should be called "gently", perhaps. > + if(ref_exists(ref->buf)) > + return BRANCH_EXISTS; > + else > + return BRANCH_DOESNT_EXIST; Always have SP between "if" (and other keyword like "while") and its condition. For this one, however, this might be easier to follow: return ref_exists(ref->buf) ? BRANCH_EXISTS : BRANCH_DOESNT_EXIST; The names of the enum values may need further thought. They must clearly convey two things, in addition to what kind of status they represent; the current names only convey the status. From the names of these values, it must be clear that they are part of the same enum (e.g. by sharing a common prefix), and also from the names of these values, it must be clear which ones are error conditions and which are not, without knowing their actual values. A reader cannot immediately tell from "BRANCH_EXISTS" if that is a good thing or not. Other than that, looks fairly cleanly done. I like what this step wants to achieve. Thanks. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation 2017-11-06 2:24 ` Junio C Hamano @ 2017-11-12 13:33 ` Kaartic Sivaraam 0 siblings, 0 replies; 12+ messages in thread From: Kaartic Sivaraam @ 2017-11-12 13:33 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Mon, 2017-11-06 at 11:24 +0900, Junio C Hamano wrote: > Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes: > > We usually use the word "gently" to when we enhance an operation > that used to always die on failure. When there are tons of callsite > to the original operation F(), we introduce F_gently() variant and > do something like > > F(...) > { > if (F_gently(...)) > die(...); > } > > so that the callers do not have to change. If there aren't that > many, it is OK to change the function signature of F() to tell it > not to die without adding a new F_gently() function, which is the > approach more appropriate for this change. The extra parameter used > for that purpose should be called "gently", perhaps. > Good suggestion, wasn't aware of it before. Renamed it. > > + if(ref_exists(ref->buf)) > > + return BRANCH_EXISTS; > > + else > > + return BRANCH_DOESNT_EXIST; > > Always have SP between "if" (and other keyword like "while") and its > condition. > > For this one, however, this might be easier to follow: > > return ref_exists(ref->buf) ? BRANCH_EXISTS : BRANCH_DOESNT_EXIST; > Done. > The names of the enum values may need further thought. They must > clearly convey two things, in addition to what kind of status they > represent; the current names only convey the status. From the names > of these values, it must be clear that they are part of the same > enum (e.g. by sharing a common prefix), and also from the names of > these values, it must be clear which ones are error conditions and > which are not, without knowing their actual values. A reader cannot > immediately tell from "BRANCH_EXISTS" if that is a good thing or > not. > I've added the prefix of "VALIDATION_{FAIL,PASS}" as appropriate to the enum values. This made the names to have the form, VALIDATION_(kind of status)_(reason) This made the names a bit long but I couldn't come up with a better prefix for now. Any suggestions are welcome. Thanks for the detailed review! --- Kaartic ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-11-12 13:33 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-02 6:52 [RFC PATCH v3 0/4] give more useful error messages while renaming branch Kaartic Sivaraam 2017-11-02 6:52 ` [RFC PATCH v3 1/4] branch: improve documentation and naming of 'create_branch()' Kaartic Sivaraam 2017-11-02 6:52 ` [RFC PATCH v3 2/4] branch: re-order function arguments to group related arguments Kaartic Sivaraam 2017-11-02 6:52 ` [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation Kaartic Sivaraam 2017-11-02 6:52 ` [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming Kaartic Sivaraam 2017-11-02 6:55 ` [RFC PATCH v3 0/4] give more useful error messages while renaming branch Kaartic Sivaraam -- strict thread matches above, loose matches on Subject: below -- 2017-09-25 8:20 [RFC PATCH v2 0/5] Give more useful error messages when renaming a branch Kaartic Sivaraam 2017-11-02 6:54 ` [RFC PATCH v3 0/4] give more useful error messages while renaming branch Kaartic Sivaraam 2017-11-02 6:54 ` [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation Kaartic Sivaraam 2017-11-02 8:39 ` Kaartic Sivaraam 2017-11-02 18:42 ` Stefan Beller 2017-11-03 2:58 ` Kaartic Sivaraam 2017-11-06 2:24 ` Junio C Hamano 2017-11-12 13:33 ` Kaartic Sivaraam
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).