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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Mar 17, 2016 at 4:17 AM, Mehul Jain <mehul.jain2029@xxxxxxxxx> wrote:
> 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?
--
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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]