Re: [RFC PATCH 2/5] branch: document the usage of certain parameters

[Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>]

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