Re: [PATCH] Add --no-autostash flag to git pull --rebase
On Fri, Feb 26, 2016 at 7:23 PM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
> Subject: [PATCH] Add --no-autostash flag to git pull --rebase
We usually don't capitalize the first word of the commit title. We also usually prefix the commit title with the relevant subsystem, file or command. So something like:
pull --rebase: add --[no-]autostash flag
Some grammatical/spelling nits below:
> git pull --rebase now understand --no-autostash flag.
s/understand/understands the/
> If directory is found to be dirty then command will die.
If the worktree is found to be dirty then the command will die.
(Perhaps state more clearly that the dirty worktree check is only performed on --no-autostash or rebase.autostash=false.)
> This flag override "rebase.autostash" configuration(if set).
s/override/overrides the/
> If this flag is not passed in command line then default behaviour is choosen,
> given by "rebase.autostash"(if "rebase.autostash"
> is not set then git pull --rebase will die if directory is dirty).
If "rebase.autostash" is not set or is false. Or you could shorten it by saying that "(default is false)".
Show 6 quoted lines
> Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
> ---
> builtin/pull.c | 12 ++++++++----
> t/t5520-pull.sh | 8 ++++++++
> t/t5521-pull-options.sh | 24 ++++++++++++++++++++++++
> 3 files changed, 40 insertions(+), 4 deletions(-)
I think git-pull's documentation should be updated as well to talk about this new command-line switch.
Show 18 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"
Show 17 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.
> +
> + if (!opt_autostash)
> die_on_unclean_work_tree(prefix);
Show 11 quoted lines
>
> 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.
Show 20 quoted lines
> + test_config rebase.autostash true &&
> + git reset --hard before-rebase &&
> + echo dirty >new_file &&
> + git add new_file &&
> + test_must_fail git pull --rebase --no-autostash . copy
> +'
> +
> test_expect_success 'pull --rebase succeeds with dirty working directory and rebase.autostash set' '
> test_config rebase.autostash true &&
> git reset --hard before-rebase &&
> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
> index 18372ca..22ff5d7 100755
> --- a/t/t5521-pull-options.sh
> +++ b/t/t5521-pull-options.sh
> @@ -62,6 +62,30 @@ test_expect_success 'git pull -v --rebase' '
> test_must_be_empty out)
> '
>
> +test_expect_success 'git pull --rebase --no-autostash' '
> + mkdir clonedrbnas &&
Took me some time to realize this directory name is actually "cloned"
+ "rb" (from --rebase) and "nas" (--no-autostash) ;-)
Show 21 quoted lines
> + (cd clonedrbnas && git init &&
> + git pull --rebase --no-autostash "../parent" >out 2>err &&
> + test -s err &&
> + test_must_be_empty out)
> +'
> +
> +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".
Show 6 quoted lines
> +
> test_expect_success 'git pull -v -q' '
> mkdir clonedvq &&
> (cd clonedvq && git init &&
> --
> 2.7.1.340.g69eb491.dirty
Mehul Jain
