Re: [RFC PATCH v3 2/4] branch: re-order function arguments to group related arguments

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 6, 2017, 02:06 UTC
Message-ID: <xmqqbmkgjh32.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171102065407.25404-3-kaartic.sivaraam@gmail.com>

Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:

Show 6 quoted lines

> 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.

I do not get what this wants to say. "I am sending this ad-hoc patch that scrambles the order of parameters for no real reason" is certainly not how you are selling this step.

> So, re-order the arguments to keep the related arguments close to each
> other.

This sentence (without "So,", obviously, because I do not get the previous paragraph) I do understand and agree with as a goal.

I think the only two things that should be kept together are "force" and "clobber_head_ok" because the previous 1/4 changed the meaning of "clobber_head" to "it is OK if I am renaming the currently checked-out branch", i.e. closer to what "force" means.

I certainly would not mind the order used in the result of this patch (in other words, if somebody posted a patch to add create_branch() function to the codebase that lacked it, with its parameters listed in the order this patch uses, I wouldn't complain), but it would have equally been OK if "reflog" and "force" were swapped without making any other change this patch makes.

I dunno.

Show 97 quoted lines

> 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);
>  	}