Kaartic Sivaraam <kaartic.sivaraam@xxxxxxxxx> writes:
> From: Kaartic Sivaraam <kaarticsivaraam91196@xxxxxxxxx>
>
> 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.
> Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@xxxxxxxxx>
> ---
> 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);
> }
