Re: [PATCH 2/2] pull --rebase: add --[no-]autostash flag

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 18, 2016, 03:53 UTC
Message-ID: <CAPig+cRmA5qQQxrHhE=3b7QPKWtSQq6ks2qPRc5v0yrE20pRsg@mail.gmail.com>
In-Reply-To: <CA+DCAeSoyKyO+SVYuY7oYF-2YK4pgvQEKpe7FpU6=5Ag+KeT-Q@mail.gmail.com>

On Thu, Mar 17, 2016 at 4:17 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:

Show 28 quoted lines

> I tried out this approach and here's the result.
>
> +       if(!opt_rebase && opt_autostash != -1)
> +               die(_("--[no-]autostash option is only valid with --rebase."));
> +
>         if (opt_rebase) {
>                 int autostash = config_autostash;
>
>                 if (is_null_sha1(orig_head) && !is_cache_unborn())
>                         die(_("Updating an unborn branch with changes
> added to the index."));
>
> +               if (opt_autostash != -1)
> +                       autostash = opt_autostash;
> +               else
> +                       opt_autostash = config_autostash;
>                 if (!autostash)
>                         die_on_unclean_work_tree(prefix);
>
> This way of implementation looks a bit less clean to me than
> the previous one because we are using "opt_autostash" to pass
> the "--[no-]autostash"  flag to git-rebase, thus if user does not
> specify anything about stashing in command line then  config_autostash
> value has to be used ( i.e. opt_autostash = config_autostash).
> To do this an "else" case has to be introduced in the code. This
> might effect the readability of the code because the reader might
> wonder why "opt_autostash" is used to assign value to "autostash"
> in one case, and opt_autostash = config_autostash in other case.

That's pretty ugly. Since cmd_pull() is the only caller of run_rebase(), an alternative would be to pass 'autostash' as an argument to run_rebase(). However, since run_rebase() is already accessing other 'opt_foo' globals, it wouldn't make sense to make an exception of 'autostash' by passing it as an argument. So, in the end, the original approach is indeed probably cleaner.

> Also I made a mistake in patch 1/2 which I will correct in the next
> version along with other changes suggested by you.

Which mistake would that be?