From: Erick Mattos <erick.mattos@gmail.com>
Date: Mon, 21 Jun 2010 23:04:30 GMT
Subject: Re: [PATCH 2/3] builtin/checkout: change -b from an OPTION_STRING to a OPTION_SET_INT
Message-ID: <AANLkTikyugBxwyCu0GY4dTLW6n_OQhIPNnPfd0_i6k0t@mail.gmail.com>
In-Reply-To: <1277140782-4064-3-git-send-email-rctay89@gmail.com>

Hi,

2010/6/21 Tay Ray Chuan <rctay89@gmail.com>:
> 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