Re: [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation
On Mon, 2017-11-06 at 11:24 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam 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
Re: [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation
Kaartic Sivaraam 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.
Re: [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation
On Friday 03 November 2017 12:12 AM, Stefan Beller wrote: On Thu, Nov 2, 2017 at 1:39 AM, Kaartic Sivaraam wrote: On Thursday 02 November 2017 12:24 PM, Kaartic Sivaraam wrote: Signed-off-by: Kaartic Sivaraam Signed-off-by: Kaartic Sivaraam 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
Re: [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation
On Thu, Nov 2, 2017 at 1:39 AM, Kaartic Sivaraam wrote: > On Thursday 02 November 2017 12:24 PM, Kaartic Sivaraam wrote: >> >> Signed-off-by: Kaartic Sivaraam >> Signed-off-by: Kaartic Sivaraam > > > 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.
Re: [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation
On Thursday 02 November 2017 12:24 PM, Kaartic Sivaraam wrote: Signed-off-by: Kaartic Sivaraam Signed-off-by: Kaartic Sivaraam 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
[RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation
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
Signed-off-by: Kaartic Sivaraam
---
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 refna
[RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation
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
Signed-off-by: Kaartic Sivaraam
---
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 refna
