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