Hi,
2010/6/21 Tay Ray Chuan <rctay89@xxxxxxxxx>:
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 1994be9..e794e1e 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -32,7 +32,8 @@ struct checkout_opts {
> int writeout_stage;
> int writeout_error;
>
> - const char *new_branch;
> + int new_branch;
> + const char *new_branch_name;
The change of name of the existent variable creates more hassle than
helps.
As you are adding a new option I suggest you to create a new
variable named new_branch_forced or whatever. This way you avoid
making a lot of changes as you did and minimize the possibility of
adding new bugs by not catching all the problems affected by the
name change.
I think you have chosen to do that just because of the variable
names then I think you should find a variable naming alternative to
satisfy you without changing existing ones.
My suggestion is to do the same I did with --orphan
(const char *new_orphan_branch):
* Create a C string variable that receives its data by a new
OPT_STRING.
* After making all tests needed, point new_branch to your created
variable.
* You will always know if your new option was used or not by
checking nullity of the just created C string.
This way any existing implementation remains untouched and thus it
is much more easy to avoid bugs.
> @@ -692,8 +694,17 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
> || opts.new_branch_log || opts.merge || opts.force))
> die ("--patch is incompatible with all other options");
>
> + if (opts.new_branch > 0) {
> + const char *arg = argv[0];
> + if (!argc || !strcmp(arg, "--"))
> + die ("Missing branch name");
> + opts.new_branch_name = arg;
> + argv++;
> + argc--;
> + }
You won't use this if you accept my previous suggestions.
To conclude, IMHO, I don't think this patch is a good thing to do.
/* I would like to point out that I am criticizing it but also
presenting suggestions! So it is a constructive critic! ;-) */
Best regards
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html