On Wednesday 20 September 2017 09:42 AM, Junio C Hamano wrote:
@@ -15,6 +15,11 @@
*
* - reflog creates a reflog for the branch
*
+ * - if 'force' is true, clobber_head indicates whether the branch could be
+ * the current branch; else it has no effect
Everybody else in this list begins with what it is describing for
easy eyeballing. Can you make this match that pattern?
Sure!
Also, what does "could be" mean in that sentence? Is the caller
telling the function "how, I do not exactly know if that is the
case, but the branch I am asking to create you might be the same
branch as what is currently checked out, so be extra careful"?
Or is the comment telling a potential caller that it can pass true
to signal that create_branch() is allowed to (re)"create" the branch
that is already checked out (hence it already exists)?
Thanks. I didn't anticipate the possibility of misinterpretation.
I think the confusing statement above arises because an assumption
is unstated there. If the reader knows "Even with force, calling
create_branch() on the currently checked out branch is normally
forbidden", then the reader can guess your "could" mean the latter.
- clobber_head_ok allows the currently checked out (hence
existing) branch to be overwritten; without force, it has
no effect.
perhaps?
Sounds better. Will use it.
... As the underlying helper calls it clobber_head_ok, and
that name is more clearly expresses that this is a permission than
the current name, I chose to add _ok to the above example, but if
you are to take the suggestion, you'd need to also update the names
in the declaration, too.
Yes.
One thing I haven't done suspecting it wouldn't be encouraged is,
rearranging the order of parameters to group the related ones
together i.e., something like,
diff --git a/branch.c b/branch.c
index 703ded69c..0ea105b55 100644
--- a/branch.c
+++ b/branch.c
@@ -229,7 +229,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 clobber_head_ok, int reflog,
int quiet, enum branch_track track)
{
struct commit *commit;
@@ -245,7 +245,7 @@ void create_branch(const char *name, const char *start_name,
if (validate_new_branchname(name, &ref, force,
track == BRANCH_TRACK_OVERRIDE ||
- clobber_head)) {
+ clobber_head_ok)) {
if (!force)
dont_change_ref = 1;
else
diff --git a/branch.h b/branch.h
index 33b7f5d88..c62763ac9 100644
--- a/branch.h
+++ b/branch.h
@@ -13,10 +13,10 @@
*
* - force enables overwriting an existing (non-head) branch
*
- * - 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.
*
- * - if 'force' is true, clobber_head indicates whether the branch could be
- * the current branch; else it has no effect
+ * - reflog creates a reflog for the branch
*
* - quiet suppresses tracking information
*
@@ -24,8 +24,8 @@
* 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 force, int clobber_head_ok,
+ int reflog, int quiet, enum branch_track track);
/*
* Validates that the requested branch may be created, returning the
diff --git a/builtin/branch.c b/builtin/branch.c
index 355f9ef5d..62c311478 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -773,7 +773,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);
+ force, 0, reflog, quiet, track);
} else
usage_with_options(builtin_branch_usage, options);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 76859da9d..116d4709d 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -641,8 +641,8 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
else
create_branch(opts->new_branch, new->name,
opts->new_branch_force ? 1 : 0,
- opts->new_branch_log,
opts->new_branch_force ? 1 : 0,
+ opts->new_branch_log,
opts->quiet,
opts->track);
new->name = opts->new_branch;
Can I do this or should I keep the patch focused around the
documentation part alone?
---
Kaartic
