Hi,
Thank you for the suggestions.
On Fri, Feb 26, 2016 at 6:17 PM, Paul Tan <[email protected]> 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.
>> 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.
>> 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.
>>
>> + 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.'
>> +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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
