Re: [RFC PATCH v3 2/4] branch: re-order function arguments to group related arguments
On Mon, 2017-11-13 at 11:32 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam writes:
>
> > I've tried to improve it, does the following paragraph sound clear
> > enough?
> >
> > branch: group related arguments of create_branch()
> >
> > New arguments were added to create_branch() whenever the need
> > arised and they were added to tail of the argument list. This
> > resulted in the related arguments not being close to each other.
>
> OK, I understand what you wanted to say. But I do not think that is
> based on a true history.
>
> - f9a482e6 ("checkout: suppress tracking message with "-q"",
>2012-03-26) adds 'quiet' just after 'clobber_head', exactly
>because they are related, and leaves 'track' at the end.
>
> - 39bd6f72 ("Allow checkout -B to update the
>current branch", 2011-11-26) adds 'clobber_head' not at the end but
>before 'track', which is left at the end.
>
> - c847f537 ("Detached HEAD (experimental)", 2007-01-01) split 'start'
>into 'start_name' and 'start_sha1' (the latter was laster removed)
>and this was not a mindless "add at the end", either.
>
> - 0746d19a ("git-branch, git-checkout: autosetup for remote branch
>tracking", 2007-03-08) did add track at the end, but that is
>justifiable, as it has no relation to any other parameter.
>
Seems I wasn't careful enough in noticing how the arguments were added.
I seemed to have overlooked the fact that 39bd6f72 added 'clobber_head'
"before" track which resulted in the vague commit message. Anyways,
thanks for taking the time to dig into this.
> You could call 39bd6f72 somewhat questionable as 'clobber_head' is
> related to 'force' more strongly than it is to 'reflog' [*1*], but
> it is unfair to blame anything else having done a mindless "add at
> the end".
>
Yep, you're right. How does the following sound?
branch: group related arguments of create_branch()
39bd6f726 (Allow checkout -B to update the current
branch, 2011-11-26) added 'clobber_head' (now, 'clobber_head_ok')
"before" 'track' as 'track' was closely related 'clobber_head' for
the purpose the commit wanted to achieve. Looking from the perspective
of how the arguments are used it turns out that 'clobber_head' is
more related to 'force' than it is to 'track'.
So, re-order the arguments to keep the related arguments close
to each other.
--
Kaartic
Re: [RFC PATCH v3 2/4] branch: re-order function arguments to group related arguments
Kaartic Sivaraam writes:
> I've tried to improve it, does the following paragraph sound clear
> enough?
>
> branch: group related arguments of create_branch()
>
> New arguments were added to create_branch() whenever the need
> arised and they were added to tail of the argument list. This
> resulted in the related arguments not being close to each other.
OK, I understand what you wanted to say. But I do not think that is
based on a true history.
- f9a482e6 ("checkout: suppress tracking message with "-q"",
2012-03-26) adds 'quiet' just after 'clobber_head', exactly
because they are related, and leaves 'track' at the end.
- 39bd6f72 ("Allow checkout -B to update the
current branch", 2011-11-26) adds 'clobber_head' not at the end but
before 'track', which is left at the end.
- c847f537 ("Detached HEAD (experimental)", 2007-01-01) split 'start'
into 'start_name' and 'start_sha1' (the latter was laster removed)
and this was not a mindless "add at the end", either.
- 0746d19a ("git-branch, git-checkout: autosetup for remote branch
tracking", 2007-03-08) did add track at the end, but that is
justifiable, as it has no relation to any other parameter.
You could call 39bd6f72 somewhat questionable as 'clobber_head' is
related to 'force' more strongly than it is to 'reflog' [*1*], but
it is unfair to blame anything else having done a mindless "add at
the end".
[Footnote]
*1* I actually think the commit added 'clobber_head' because for the
purpose of what the commit did, it closely was related to 'track',
turning into .
Arguably, it could have done instead.
Re: [RFC PATCH v3 2/4] branch: re-order function arguments to group related arguments
On Mon, 2017-11-06 at 11:06 +0900, Junio C Hamano wrote: > Kaartic Sivaraam writes: > > > From: Kaartic Sivaraam > > > > 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've tried to improve it, does the following paragraph sound clear enough? branch: group related arguments of create_branch() New arguments were added to create_branch() whenever the need arised and they were added to tail of the argument list. This 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 is not the case. So, re-order the arguments to keep the related arguments close to each other. > 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. > Makes sense. The unwanted shuffling was a consequence of my attempt to see if the signature of the function did change when the position of the 'enum' was changed. It seems there isn't change in its signature as it is possible to use integers for enums and vice versa due to liberal checking for misuses. I've changed the signature back to keep alone "force" and "clobber_head_ok" together. Thanks, Kaartic
Re: [RFC PATCH v3 2/4] branch: re-order function arguments to group related arguments
Kaartic Sivaraam writes:
> From: Kaartic Sivaraam
>
> 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
> ---
> 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
[RFC PATCH v3 2/4] branch: re-order function arguments to group related arguments
From: Kaartic Sivaraam
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.
So, re-order the arguments to keep the related arguments close to each
other.
Signed-off-by: Kaartic Sivaraam
---
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);
}
--
2.15.0.461.gf957c703b.dirty
[RFC PATCH v3 2/4] branch: re-order function arguments to group related arguments
From: Kaartic Sivaraam
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.
So, re-order the arguments to keep the related arguments close to each
other.
Signed-off-by: Kaartic Sivaraam
---
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);
}
--
2.15.0.461.gf957c703b.dirty
