Re: [PATCH] Add --no-autostash flag to git pull --rebase

From: Mehul Jain <mehul.jain2029@gmail.com>
Date: Feb 26, 2016, 17:09 UTC
Message-ID: <CA+DCAeTAAn-xA=3kHBcURzv_1eJRKd7uMZRwaPNbCZbGS2eFLw@mail.gmail.com>
In-Reply-To: <CACRoPnTKRwGmhQOrJsA2ys66RwRVcY1nu+hs_5_YMqgiSUbNiw@mail.gmail.com>

Hi,

Thank you for the suggestions. On Fri, Feb 26, 2016 at 6:17 PM, Paul Tan <pyokagan@gmail.com> wrote:

> Some grammatical/spelling nits below:

Many apologies for my English.

> I think git-pull's documentation should be updated as well to talk
> about this new command-line switch.

OK.

Show 23 quoted lines

>> diff --git a/builtin/pull.c b/builtin/pull.c
>> index 10eff03..9d1a3d0 100644
>> --- a/builtin/pull.c
>> +++ b/builtin/pull.c
>> @@ -85,6 +85,7 @@ static char *opt_squash;
>>  static char *opt_commit;
>>  static char *opt_edit;
>>  static char *opt_ff;
>> +static int opt_autostash = -1;
>>  static char *opt_verify_signatures;
>>  static struct argv_array opt_strategies = ARGV_ARRAY_INIT;
>>  static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;
>> @@ -146,6 +147,8 @@ static struct option pull_options[] = {
>>         OPT_PASSTHRU(0, "ff-only", &opt_ff, NULL,
>>                 N_("abort if fast-forward is not possible"),
>>                 PARSE_OPT_NOARG | PARSE_OPT_NONEG),
>> +       OPT_COLOR_FLAG(0,"autostash",&opt_autostash,
>> +               N_("abort if tree is dirty")),
>
> Why OPT_COLOR_FLAG()? And --autostash is not just about aborting if
> the working tree is dirty. Why not just copy the help message from
> git-rebase? Something like:
> "automatically stash/stash pop before and after rebase"

Using OPT_COLOR_FLAG() is wrong, I agree. OPT_BOOL will be a better option. N_("automatically stash/stash pop before and after rebase") is better.

Show 21 quoted lines

>>         OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
>>                 N_("verify that the named commit has a valid GPG signature"),
>>                 PARSE_OPT_NOARG),
>> @@ -835,13 +838,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>>                 hashclr(orig_head);
>>
>>         if (opt_rebase) {
>> -               int autostash = 0;
>> -
>>                 if (is_null_sha1(orig_head) && !is_cache_unborn())
>>                         die(_("Updating an unborn branch with changes added to the index."));
>>
>> -               git_config_get_bool("rebase.autostash", &autostash);
>> -               if (!autostash)
>> +               if(opt_autostash < 0)
>> +                       if(git_config_get_bool("rebase.autostash",&opt_autostash))
>> +                               opt_autostash = 0;
>
> I wonder if this code could be shortened if we simply just called
> git_config_get_bool() just before parse_options(). That way, we don't
> need to check for the "-1" special value.

Definitely. This way opt_autostash can be initialized with 0, thus default will be false.

Show 20 quoted lines

>>
>> +               if (!opt_autostash)
>>                         die_on_unclean_work_tree(prefix);
>
> OK.
>
>>
>>                 if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
>> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
>> index c952d5e..512d3bf 100755
>> --- a/t/t5520-pull.sh
>> +++ b/t/t5520-pull.sh
>> @@ -245,6 +245,14 @@ test_expect_success '--rebase fails with multiple branches' '
>>         test modified = "$(git show HEAD:file)"
>>  '
>>
>> +test_expect_success '--rebase --no-autostash fails with dirty working directory' '
>
> Maybe add ..."and rebase.autostash set" to the test name? Describes
> the test better, and is consistent with the name of the test below.

Can be done. But which one of these will be more appropriate:
 "rebase.autostash set" or "rebase.autostash set true".
I prefer latter, as it will maintain consistence with the test name of
"--rebase --autostash", which will be
'--rebase --autostash succeeds with dirty working directory and
rebase.autostash set false.'

Show 20 quoted lines

>> +test_expect_success 'git pull -q --rebase --no-autostash' '
>> +       mkdir clonedqrbnas &&
>> +       (cd clonedqrbnas  && git init &&
>> +       git pull -q --rebase --no-autostash "../parent" >out 2>err &&
>> +       test_must_be_empty err &&
>> +       test_must_be_empty out)
>> +'
>> +
>> +test_expect_success 'git pull -v --rebase --no-autostash' '
>> +       mkdir clonedvrbnas &&
>> +       (cd clonedvrbnas && git init &&
>> +       git pull -v --rebase --no-autostash "../parent" >out 2>err &&
>> +       test -s err &&
>> +       test_must_be_empty out)
>> +'
>
> While more tests are always good, I don't think we need to test for
> "-q" and "-v" with --no-autostash, because it's already covered by the
> test for "git pull -q --rebase". Perhaps with --autostash, but even
> then I don't think we need a test for "-v".

OK then. I will only add tests for "git pull --rebase --no-autostash", "git pull --rebase --autostash" and "git pull -q --rebase --autostash" in t5521-pull-options.sh

Thanks, Mehul Jain