* [PATCH v10 1/2] git-pull.c: introduce git_pull_config()
2016-03-21 18:18 [PATCH v10 0/2] introduce --[no-]autostash command line flag Mehul Jain
@ 2016-03-21 18:18 ` Mehul Jain
2016-03-21 18:18 ` [PATCH v10 2/2] pull --rebase: add --[no-]autostash flag Mehul Jain
` (2 subsequent siblings)
3 siblings, 0 replies; 16+ messages in thread
From: Mehul Jain @ 2016-03-21 18:18 UTC (permalink / raw)
To: git; +Cc: gitster, Matthieu.Moy, pyokagan, sunshine, Mehul Jain
git-pull makes a seperate call to git_config_get_bool() to read the value
of "rebase.autostash". This can be reduced as a call to git_config() is
already there in the code.
Introduce a callback function git_pull_config() to read "rebase.autostash"
along with other variables.
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Paul Tan <pyokagan@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---
builtin/pull.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/builtin/pull.c b/builtin/pull.c
index 10eff03..c21897d 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -86,6 +86,7 @@ static char *opt_commit;
static char *opt_edit;
static char *opt_ff;
static char *opt_verify_signatures;
+static int config_autostash;
static struct argv_array opt_strategies = ARGV_ARRAY_INIT;
static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;
static char *opt_gpg_sign;
@@ -306,6 +307,18 @@ static enum rebase_type config_get_rebase(void)
}
/**
+ * Read config variables.
+ */
+static int git_pull_config(const char *var, const char *value, void *cb)
+{
+ if (!strcmp(var, "rebase.autostash")) {
+ config_autostash = git_config_bool(var, value);
+ return 0;
+ }
+ return git_default_config(var, value, cb);
+}
+
+/**
* Returns 1 if there are unstaged changes, 0 otherwise.
*/
static int has_unstaged_changes(const char *prefix)
@@ -823,7 +836,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
if (opt_rebase < 0)
opt_rebase = config_get_rebase();
- git_config(git_default_config, NULL);
+ git_config(git_pull_config, NULL);
if (read_cache_unmerged())
die_resolve_conflict("Pull");
@@ -835,12 +848,11 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
hashclr(orig_head);
if (opt_rebase) {
- int autostash = 0;
+ int autostash = config_autostash;
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)
die_on_unclean_work_tree(prefix);
--
2.7.1.340.g69eb491.dirty
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v10 2/2] pull --rebase: add --[no-]autostash flag
2016-03-21 18:18 [PATCH v10 0/2] introduce --[no-]autostash command line flag Mehul Jain
2016-03-21 18:18 ` [PATCH v10 1/2] git-pull.c: introduce git_pull_config() Mehul Jain
@ 2016-03-21 18:18 ` Mehul Jain
2016-03-21 18:39 ` Matthieu Moy
2016-03-21 20:12 ` Mehul Jain
2016-03-25 7:23 ` [PATCH v10 0/2] introduce --[no-]autostash command line flag Eric Sunshine
3 siblings, 1 reply; 16+ messages in thread
From: Mehul Jain @ 2016-03-21 18:18 UTC (permalink / raw)
To: git; +Cc: gitster, Matthieu.Moy, pyokagan, sunshine, Mehul Jain
If rebase.autoStash configuration variable is set, there is no way to
override it for "git pull --rebase" from the command line.
Teach "git pull --rebase" the --[no-]autostash command line flag which
overrides the current value of rebase.autoStash, if set. As "git rebase"
understands the --[no-]autostash option, it's just a matter of passing
the option to underlying "git rebase" when "git pull --rebase" is called.
Helped-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Paul Tan <pyokagan@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---
Documentation/git-pull.txt | 9 ++++++
builtin/pull.c | 12 ++++++++
t/t5520-pull.sh | 70 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 91 insertions(+)
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index a62a2a6..3914507 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -128,6 +128,15 @@ unless you have read linkgit:git-rebase[1] carefully.
--no-rebase::
Override earlier --rebase.
+--autostash::
+--no-autostash::
+ Before starting rebase, stash local modifications away (see
+ linkgit:git-stash.txt[1]) if needed, and apply the stash when
+ done. `--no-autostash` is useful to override the `rebase.autoStash`
+ configuration variable (see linkgit:git-config[1]).
++
+This option is only valid when "--rebase" is used.
+
Options related to fetching
~~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/builtin/pull.c b/builtin/pull.c
index c21897d..d98f481 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -86,6 +86,7 @@ static char *opt_commit;
static char *opt_edit;
static char *opt_ff;
static char *opt_verify_signatures;
+static int opt_autostash = -1;
static int config_autostash;
static struct argv_array opt_strategies = ARGV_ARRAY_INIT;
static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;
@@ -150,6 +151,8 @@ static struct option pull_options[] = {
OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
N_("verify that the named commit has a valid GPG signature"),
PARSE_OPT_NOARG),
+ OPT_BOOL(0, "autostash", &opt_autostash,
+ N_("automatically stash/stash pop before and after rebase")),
OPT_PASSTHRU_ARGV('s', "strategy", &opt_strategies, N_("strategy"),
N_("merge strategy to use"),
0),
@@ -802,6 +805,10 @@ static int run_rebase(const unsigned char *curr_head,
argv_array_pushv(&args, opt_strategy_opts.argv);
if (opt_gpg_sign)
argv_array_push(&args, opt_gpg_sign);
+ if (opt_autostash == 0)
+ argv_array_push(&args, "--no-autostash");
+ else if (opt_autostash == 1)
+ argv_array_push(&args, "--autostash");
argv_array_push(&args, "--onto");
argv_array_push(&args, sha1_to_hex(merge_head));
@@ -847,8 +854,13 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
if (get_sha1("HEAD", orig_head))
hashclr(orig_head);
+ if (!opt_rebase && opt_autostash != -1)
+ die(_("--[no-]autostash option is only valid with --rebase."));
+
if (opt_rebase) {
int autostash = config_autostash;
+ if (opt_autostash != -1)
+ autostash = opt_autostash;
if (is_null_sha1(orig_head) && !is_cache_unborn())
die(_("Updating an unborn branch with changes added to the index."));
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index c952d5e..745e59e 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -256,6 +256,76 @@ test_expect_success 'pull --rebase succeeds with dirty working directory and reb
test "$(cat file)" = "modified again"
'
+test_expect_success 'pull --rebase --autostash & rebase.autostash=true' '
+ test_config rebase.autostash true &&
+ git reset --hard before-rebase &&
+ echo dirty >new_file &&
+ git add new_file &&
+ git pull --rebase --autostash . copy &&
+ test_cmp_rev HEAD^ copy &&
+ test "$(cat new_file)" = dirty &&
+ test "$(cat file)" = "modified again"
+'
+
+test_expect_success 'pull --rebase --autostash & rebase.autoStash=false' '
+ test_config rebase.autostash false &&
+ git reset --hard before-rebase &&
+ echo dirty >new_file &&
+ git add new_file &&
+ git pull --rebase --autostash . copy &&
+ test_cmp_rev HEAD^ copy &&
+ test "$(cat new_file)" = dirty &&
+ test "$(cat file)" = "modified again"
+'
+
+test_expect_success 'pull --rebase: --autostash & rebase.autoStash unset' '
+ git reset --hard before-rebase &&
+ echo dirty >new_file &&
+ git add new_file &&
+ git pull --rebase --autostash . copy &&
+ test_cmp_rev HEAD^ copy &&
+ test "$(cat new_file)" = dirty &&
+ test "$(cat file)" = "modified again"
+'
+
+test_expect_success 'pull --rebase --no-autostash & rebase.autostash=true' '
+ 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 2>err &&
+ test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
+'
+
+test_expect_success 'pull --rebase --no-autostash & rebase.autostash=false' '
+ test_config rebase.autostash false &&
+ git reset --hard before-rebase &&
+ echo dirty >new_file &&
+ git add new_file &&
+ test_must_fail git pull --rebase --no-autostash . copy 2>err &&
+ test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
+'
+
+test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '
+ git reset --hard before-rebase &&
+ echo dirty >new_file &&
+ git add new_file &&
+ test_must_fail git pull --rebase --no-autostash . copy 2>err &&
+ test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
+'
+
+test_expect_success 'pull --autostash (without --rebase) should error out' '
+ test_must_fail git pull --autostash . copy 2>actual &&
+ echo "fatal: --[no-]autostash option is only valid with --rebase." >expect &&
+ test_i18ncmp actual expect
+'
+
+test_expect_success 'pull --no-autostash (without --rebase) should error out' '
+ test_must_fail git pull --no-autostash . copy 2>actual &&
+ echo "fatal: --[no-]autostash option is only valid with --rebase." >expect &&
+ test_i18ncmp actual expect
+'
+
test_expect_success 'pull.rebase' '
git reset --hard before-rebase &&
test_config pull.rebase true &&
--
2.7.1.340.g69eb491.dirty
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v10 2/2] pull --rebase: add --[no-]autostash flag
2016-03-21 18:18 [PATCH v10 0/2] introduce --[no-]autostash command line flag Mehul Jain
2016-03-21 18:18 ` [PATCH v10 1/2] git-pull.c: introduce git_pull_config() Mehul Jain
2016-03-21 18:18 ` [PATCH v10 2/2] pull --rebase: add --[no-]autostash flag Mehul Jain
@ 2016-03-21 20:12 ` Mehul Jain
2016-03-25 8:31 ` Eric Sunshine
2016-03-25 9:05 ` Matthieu Moy
2016-03-25 7:23 ` [PATCH v10 0/2] introduce --[no-]autostash command line flag Eric Sunshine
3 siblings, 2 replies; 16+ messages in thread
From: Mehul Jain @ 2016-03-21 20:12 UTC (permalink / raw)
To: git; +Cc: gitster, Matthieu.Moy, pyokagan, sunshine, Mehul Jain
If rebase.autoStash configuration variable is set, there is no way to
override it for "git pull --rebase" from the command line.
Teach "git pull --rebase" the --[no-]autostash command line flag which
overrides the current value of rebase.autoStash, if set. As "git rebase"
understands the --[no-]autostash option, it's just a matter of passing
the option to underlying "git rebase" when "git pull --rebase" is called.
Helped-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Paul Tan <pyokagan@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---
Documentation/git-pull.txt | 9 ++++++
builtin/pull.c | 12 ++++++++
t/t5520-pull.sh | 70 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 91 insertions(+)
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index a62a2a6..3914507 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -128,6 +128,15 @@ unless you have read linkgit:git-rebase[1] carefully.
--no-rebase::
Override earlier --rebase.
+--autostash::
+--no-autostash::
+ Before starting rebase, stash local modifications away (see
+ linkgit:git-stash[1]) if needed, and apply the stash when
+ done. `--no-autostash` is useful to override the `rebase.autoStash`
+ configuration variable (see linkgit:git-config[1]).
++
+This option is only valid when "--rebase" is used.
+
Options related to fetching
~~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/builtin/pull.c b/builtin/pull.c
index c21897d..d98f481 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -86,6 +86,7 @@ static char *opt_commit;
static char *opt_edit;
static char *opt_ff;
static char *opt_verify_signatures;
+static int opt_autostash = -1;
static int config_autostash;
static struct argv_array opt_strategies = ARGV_ARRAY_INIT;
static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;
@@ -150,6 +151,8 @@ static struct option pull_options[] = {
OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
N_("verify that the named commit has a valid GPG signature"),
PARSE_OPT_NOARG),
+ OPT_BOOL(0, "autostash", &opt_autostash,
+ N_("automatically stash/stash pop before and after rebase")),
OPT_PASSTHRU_ARGV('s', "strategy", &opt_strategies, N_("strategy"),
N_("merge strategy to use"),
0),
@@ -802,6 +805,10 @@ static int run_rebase(const unsigned char *curr_head,
argv_array_pushv(&args, opt_strategy_opts.argv);
if (opt_gpg_sign)
argv_array_push(&args, opt_gpg_sign);
+ if (opt_autostash == 0)
+ argv_array_push(&args, "--no-autostash");
+ else if (opt_autostash == 1)
+ argv_array_push(&args, "--autostash");
argv_array_push(&args, "--onto");
argv_array_push(&args, sha1_to_hex(merge_head));
@@ -847,8 +854,13 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
if (get_sha1("HEAD", orig_head))
hashclr(orig_head);
+ if (!opt_rebase && opt_autostash != -1)
+ die(_("--[no-]autostash option is only valid with --rebase."));
+
if (opt_rebase) {
int autostash = config_autostash;
+ if (opt_autostash != -1)
+ autostash = opt_autostash;
if (is_null_sha1(orig_head) && !is_cache_unborn())
die(_("Updating an unborn branch with changes added to the index."));
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index c952d5e..745e59e 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -256,6 +256,76 @@ test_expect_success 'pull --rebase succeeds with dirty working directory and reb
test "$(cat file)" = "modified again"
'
+test_expect_success 'pull --rebase --autostash & rebase.autostash=true' '
+ test_config rebase.autostash true &&
+ git reset --hard before-rebase &&
+ echo dirty >new_file &&
+ git add new_file &&
+ git pull --rebase --autostash . copy &&
+ test_cmp_rev HEAD^ copy &&
+ test "$(cat new_file)" = dirty &&
+ test "$(cat file)" = "modified again"
+'
+
+test_expect_success 'pull --rebase --autostash & rebase.autoStash=false' '
+ test_config rebase.autostash false &&
+ git reset --hard before-rebase &&
+ echo dirty >new_file &&
+ git add new_file &&
+ git pull --rebase --autostash . copy &&
+ test_cmp_rev HEAD^ copy &&
+ test "$(cat new_file)" = dirty &&
+ test "$(cat file)" = "modified again"
+'
+
+test_expect_success 'pull --rebase: --autostash & rebase.autoStash unset' '
+ git reset --hard before-rebase &&
+ echo dirty >new_file &&
+ git add new_file &&
+ git pull --rebase --autostash . copy &&
+ test_cmp_rev HEAD^ copy &&
+ test "$(cat new_file)" = dirty &&
+ test "$(cat file)" = "modified again"
+'
+
+test_expect_success 'pull --rebase --no-autostash & rebase.autostash=true' '
+ 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 2>err &&
+ test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
+'
+
+test_expect_success 'pull --rebase --no-autostash & rebase.autostash=false' '
+ test_config rebase.autostash false &&
+ git reset --hard before-rebase &&
+ echo dirty >new_file &&
+ git add new_file &&
+ test_must_fail git pull --rebase --no-autostash . copy 2>err &&
+ test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
+'
+
+test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '
+ git reset --hard before-rebase &&
+ echo dirty >new_file &&
+ git add new_file &&
+ test_must_fail git pull --rebase --no-autostash . copy 2>err &&
+ test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
+'
+
+test_expect_success 'pull --autostash (without --rebase) should error out' '
+ test_must_fail git pull --autostash . copy 2>actual &&
+ echo "fatal: --[no-]autostash option is only valid with --rebase." >expect &&
+ test_i18ncmp actual expect
+'
+
+test_expect_success 'pull --no-autostash (without --rebase) should error out' '
+ test_must_fail git pull --no-autostash . copy 2>actual &&
+ echo "fatal: --[no-]autostash option is only valid with --rebase." >expect &&
+ test_i18ncmp actual expect
+'
+
test_expect_success 'pull.rebase' '
git reset --hard before-rebase &&
test_config pull.rebase true &&
--
2.7.1.340.g69eb491.dirty
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v10 2/2] pull --rebase: add --[no-]autostash flag
2016-03-21 20:12 ` Mehul Jain
@ 2016-03-25 8:31 ` Eric Sunshine
2016-03-25 8:44 ` Eric Sunshine
2016-03-25 18:07 ` Mehul Jain
2016-03-25 9:05 ` Matthieu Moy
1 sibling, 2 replies; 16+ messages in thread
From: Eric Sunshine @ 2016-03-25 8:31 UTC (permalink / raw)
To: Mehul Jain; +Cc: Git List, Junio C Hamano, Matthieu Moy, Paul Tan
On Mon, Mar 21, 2016 at 4:12 PM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
> If rebase.autoStash configuration variable is set, there is no way to
> override it for "git pull --rebase" from the command line.
>
> Teach "git pull --rebase" the --[no-]autostash command line flag which
> overrides the current value of rebase.autoStash, if set. As "git rebase"
> understands the --[no-]autostash option, it's just a matter of passing
> the option to underlying "git rebase" when "git pull --rebase" is called.
This version of the patch (coupled with patch 1/2) is a pleasant
improvement over previous versions due to the cleaner structure, less
noisy diff, and general simplicity (thus easier to reason about and
review).
See below for a nit and some comments about the tests.
> Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
> ---
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> @@ -256,6 +256,76 @@ test_expect_success 'pull --rebase succeeds with dirty working directory and reb
> test "$(cat file)" = "modified again"
> '
>
> +test_expect_success 'pull --rebase --autostash & rebase.autostash=true' '
Nit: Some of the test titles spell this as "rebase.autostash" while
others use "rebase.autoStash".
> + test_config rebase.autostash true &&
> + git reset --hard before-rebase &&
> + echo dirty >new_file &&
> + git add new_file &&
> + git pull --rebase --autostash . copy &&
> + test_cmp_rev HEAD^ copy &&
> + test "$(cat new_file)" = dirty &&
> + test "$(cat file)" = "modified again"
> +'
> +
> +test_expect_success 'pull --rebase --autostash & rebase.autoStash=false' '
> + test_config rebase.autostash false &&
> + git reset --hard before-rebase &&
> + echo dirty >new_file &&
> + git add new_file &&
> + git pull --rebase --autostash . copy &&
> + test_cmp_rev HEAD^ copy &&
> + test "$(cat new_file)" = dirty &&
> + test "$(cat file)" = "modified again"
> +'
> +
> +test_expect_success 'pull --rebase: --autostash & rebase.autoStash unset' '
The title says that this is testing with rebase.autoStash unset,
however, the test itself doesn't take any action to ensure that it is
indeed unset. As with the two above tests which explicitly set
rebase.autoStash, this test should explicitly unset rebase.autoStash
to ensure consistent results even if some future change somehow
pollutes the configuration globally. Therefore:
test_unconfig rebase.autostash &&
> + git reset --hard before-rebase &&
> + echo dirty >new_file &&
> + git add new_file &&
> + git pull --rebase --autostash . copy &&
> + test_cmp_rev HEAD^ copy &&
> + test "$(cat new_file)" = dirty &&
> + test "$(cat file)" = "modified again"
> +'
With the addition of these three new tests, aside from the
introductory 'test_{un}config', this exact sequence of commands is now
repeated four times in the script. Such repetition suggests that the
common code should be moved to a function. For instance:
test_rebase_autostash () {
git reset --hard before-rebase &&
echo dirty >new_file &&
git add new_file &&
git pull --rebase . copy &&
test_cmp_rev HEAD^ copy &&
test "$(cat new_file)" = dirty &&
test "$(cat file)" = "modified again"
}
And, a caller would look like this:
test_expect_success 'pull ... rebase.autostash=true' '
test_config rebase.autostash true &&
test_rebase_autostash
'
Of course, you'd also update the original test, from which this code
was copied, to also call the new function. Factoring out the common
code into a function should probably be done as a separate preparatory
patch.
This suggestion isn't mandatory and doesn't demand a re-roll, but, if
you're feeling ambitious, it would make the code easier to digest and
review.
> +test_expect_success 'pull --rebase --no-autostash & rebase.autostash=true' '
> + 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 2>err &&
> + test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
I don't care strongly, but many tests consider test_must_fail() alone
sufficient to verify proper behavior and don't bother being more exact
by checking the precise error message (since error messages sometimes
get refined, thus requiring adjustments to the tests). If you do
retain the error message check, it's often sufficient to check for
just a fragment of the error string rather than the full message. For
instance, it might be fine to grep merely for "uncommitted changes".
> +'
> +
> +test_expect_success 'pull --rebase --no-autostash & rebase.autostash=false' '
> + test_config rebase.autostash false &&
> + git reset --hard before-rebase &&
> + echo dirty >new_file &&
> + git add new_file &&
> + test_must_fail git pull --rebase --no-autostash . copy 2>err &&
> + test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
> +'
> +
> +test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '
Same comment as above:
test_unconfig rebase.autostash &&
> + git reset --hard before-rebase &&
> + echo dirty >new_file &&
> + git add new_file &&
> + test_must_fail git pull --rebase --no-autostash . copy 2>err &&
> + test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
> +'
Same comment as above about the common code shared by these three new
test: moving it to a function is suggested.
> +test_expect_success 'pull --autostash (without --rebase) should error out' '
> + test_must_fail git pull --autostash . copy 2>actual &&
> + echo "fatal: --[no-]autostash option is only valid with --rebase." >expect &&
> + test_i18ncmp actual expect
Same comment as above about checking the exact error message (vs. just
trusting test_must_fail).
Also, you mentioned in your cover letter that you couldn't use
test_i18ngrep because grep was mistaking "--[no-]autostash" in the
above expression as a command-line option. If you were using the exact
string as above as an argument to test_i18ngrep, then it is more
likely that the problem was that grep was seeing "[no-]" as a
character class rather than as a literal pattern to match. You could
get around this either by escaping the [ and ] with a backslash (\) or
by passing -F to test_i18ngrep.
Alternately, as mentioned above, just grep for a fragment of the error
message, such as "only valid with --rebase", rather than the full
diagnostic.
> +'
> +
> +test_expect_success 'pull --no-autostash (without --rebase) should error out' '
> + test_must_fail git pull --no-autostash . copy 2>actual &&
> + echo "fatal: --[no-]autostash option is only valid with --rebase." >expect &&
> + test_i18ncmp actual expect
> +'
Same comment as above about code common to these two tests. However,
in this case, it might be easier simply to use a 'for' loop rather
than a function:
for i in --autostash --no-autostash
do
test_expect_success "pull $i (without --rebase) is illegal" "
test_must_fail git pull $i . copy 2>actual &&
test_i18ngrep 'only valid with --rebase' actual
"
done
Take special note of how use of double (") and single (') quotes
differ in this case from other tests since $i needs to be interpolated
into the test body.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v10 2/2] pull --rebase: add --[no-]autostash flag
2016-03-25 8:31 ` Eric Sunshine
@ 2016-03-25 8:44 ` Eric Sunshine
2016-03-25 16:37 ` Eric Sunshine
2016-03-25 18:07 ` Mehul Jain
1 sibling, 1 reply; 16+ messages in thread
From: Eric Sunshine @ 2016-03-25 8:44 UTC (permalink / raw)
To: Mehul Jain; +Cc: Git List, Junio C Hamano, Matthieu Moy, Paul Tan
On Fri, Mar 25, 2016 at 4:31 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> for i in --autostash --no-autostash
> do
> test_expect_success "pull $i (without --rebase) is illegal" "
> test_must_fail git pull $i . copy 2>actual &&
> test_i18ngrep 'only valid with --rebase' actual
> "
> done
>
> Take special note of how use of double (") and single (') quotes
> differ in this case from other tests since $i needs to be interpolated
> into the test body.
That's not accurate. Since $i will be visible when the test body is
actually evaluated, it will work correctly even with the body
single-quoted as usual (like all other tests), so swapping the quotes
around like this is unnecessary (and Junio would prefer[1] they not be
swapped).
[1]: http://article.gmane.org/gmane.comp.version-control.git/284769
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v10 2/2] pull --rebase: add --[no-]autostash flag
2016-03-25 8:44 ` Eric Sunshine
@ 2016-03-25 16:37 ` Eric Sunshine
0 siblings, 0 replies; 16+ messages in thread
From: Eric Sunshine @ 2016-03-25 16:37 UTC (permalink / raw)
To: Mehul Jain; +Cc: Git List, Junio C Hamano, Matthieu Moy, Paul Tan
On Fri, Mar 25, 2016 at 4:44 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, Mar 25, 2016 at 4:31 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> for i in --autostash --no-autostash
>> do
>> test_expect_success "pull $i (without --rebase) is illegal" "
>> test_must_fail git pull $i . copy 2>actual &&
>> test_i18ngrep 'only valid with --rebase' actual
>> "
>> done
>>
>> Take special note of how use of double (") and single (') quotes
>> differ in this case from other tests since $i needs to be interpolated
>> into the test body.
>
> That's not accurate. Since $i will be visible when the test body is
> actually evaluated, it will work correctly even with the body
> single-quoted as usual (like all other tests), so swapping the quotes
> around like this is unnecessary (and Junio would prefer[1] they not be
> swapped).
Junio pointed out to me privately that I forgot to mention explicitly
that you would need to use double quotes for the test title to ensure
that $i is interpolated, but the test body can continue using single
quotes, as explained above.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v10 2/2] pull --rebase: add --[no-]autostash flag
2016-03-25 8:31 ` Eric Sunshine
2016-03-25 8:44 ` Eric Sunshine
@ 2016-03-25 18:07 ` Mehul Jain
2016-03-25 22:29 ` Eric Sunshine
1 sibling, 1 reply; 16+ messages in thread
From: Mehul Jain @ 2016-03-25 18:07 UTC (permalink / raw)
To: Eric Sunshine, Junio C Hamano; +Cc: Git List, Matthieu Moy, Paul Tan
On Fri, Mar 25, 2016 at 2:01 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> +test_expect_success 'pull --rebase --autostash & rebase.autostash=true' '
>
> Nit: Some of the test titles spell this as "rebase.autostash" while
> others use "rebase.autoStash".
That's a mistake. All test titles must spell "rebase.autoStash".
>> +test_expect_success 'pull --rebase: --autostash & rebase.autoStash unset' '
>
> The title says that this is testing with rebase.autoStash unset,
> however, the test itself doesn't take any action to ensure that it is
> indeed unset.
Actually test_config unset the config variable once the test is complete.
Thus I felt that test_unconfig might not be needed.
>As with the two above tests which explicitly set
> rebase.autoStash, this test should explicitly unset rebase.autoStash
> to ensure consistent results even if some future change somehow
> pollutes the configuration globally. Therefore:
>
> test_unconfig rebase.autostash &&
>
But considering this point, I'm convinced that indeed test_unconfig
should have been used.
>> + git reset --hard before-rebase &&
>> + echo dirty >new_file &&
>> + git add new_file &&
>> + git pull --rebase --autostash . copy &&
>> + test_cmp_rev HEAD^ copy &&
>> + test "$(cat new_file)" = dirty &&
>> + test "$(cat file)" = "modified again"
>> +'
>
> With the addition of these three new tests, aside from the
> introductory 'test_{un}config', this exact sequence of commands is now
> repeated four times in the script. Such repetition suggests that the
> common code should be moved to a function. For instance:
>
> test_rebase_autostash () {
> git reset --hard before-rebase &&
> echo dirty >new_file &&
> git add new_file &&
> git pull --rebase . copy &&
> test_cmp_rev HEAD^ copy &&
> test "$(cat new_file)" = dirty &&
> test "$(cat file)" = "modified again"
> }
>
> And, a caller would look like this:
>
> test_expect_success 'pull ... rebase.autostash=true' '
> test_config rebase.autostash true &&
> test_rebase_autostash
> '
>
> Of course, you'd also update the original test, from which this code
> was copied, to also call the new function. Factoring out the common
> code into a function should probably be done as a separate preparatory
> patch.
>
> This suggestion isn't mandatory and doesn't demand a re-roll, but, if
> you're feeling ambitious, it would make the code easier to digest and
> review.
Nice. This will increase fluency of the code and also lead to significant
reduction in number of new lines introduced by this patch.
>> +test_expect_success 'pull --rebase --no-autostash & rebase.autostash=true' '
>> + 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 2>err &&
>> + test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
>
> I don't care strongly, but many tests consider test_must_fail() alone
> sufficient to verify proper behavior and don't bother being more exact
> by checking the precise error message (since error messages sometimes
> get refined, thus requiring adjustments to the tests). If you do
Main reason to use test_i18ngrep here and check for this specific
error is that in future if some developer make changes which might
trigger git-pull not to die at die_on_unclean_work_tree() check (if
work tree is dirty) but leads git-pull to die somewhere else then
basically he/she will not understand the bug introduced by him/her as
test "pull --rebase --no-autostash & rebase.autostash=true" might pass.
test_i18ngrep will make sure that this does not happen.
> retain the error message check, it's often sufficient to check for
> just a fragment of the error string rather than the full message. For
> instance, it might be fine to grep merely for "uncommitted changes".
Yes, that will work too as no other error messages for git-pull contain these
words.
>> +'
>> +
>> +test_expect_success 'pull --rebase --no-autostash & rebase.autostash=false' '
>> + test_config rebase.autostash false &&
>> + git reset --hard before-rebase &&
>> + echo dirty >new_file &&
>> + git add new_file &&
>> + test_must_fail git pull --rebase --no-autostash . copy 2>err &&
>> + test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
>> +'
>> +
>> +test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '
>
> Same comment as above:
>
> test_unconfig rebase.autostash &&
>
>> + git reset --hard before-rebase &&
>> + echo dirty >new_file &&
>> + git add new_file &&
>> + test_must_fail git pull --rebase --no-autostash . copy 2>err &&
>> + test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
>> +'
>
> Same comment as above about the common code shared by these three new
> test: moving it to a function is suggested.
>
>> +test_expect_success 'pull --autostash (without --rebase) should error out' '
>> + test_must_fail git pull --autostash . copy 2>actual &&
>> + echo "fatal: --[no-]autostash option is only valid with --rebase." >expect &&
>> + test_i18ncmp actual expect
>
> Same comment as above about checking the exact error message (vs. just
> trusting test_must_fail).
>
> Also, you mentioned in your cover letter that you couldn't use
> test_i18ngrep because grep was mistaking "--[no-]autostash" in the
> above expression as a command-line option. If you were using the exact
> string as above as an argument to test_i18ngrep, then it is more
> likely that the problem was that grep was seeing "[no-]" as a
> character class rather than as a literal pattern to match. You could
> get around this either by escaping the [ and ] with a backslash (\) or
> by passing -F to test_i18ngrep.
>
> Alternately, as mentioned above, just grep for a fragment of the error
> message, such as "only valid with --rebase", rather than the full
> diagnostic.
>
>> +'
>> +
>> +test_expect_success 'pull --no-autostash (without --rebase) should error out' '
>> + test_must_fail git pull --no-autostash . copy 2>actual &&
>> + echo "fatal: --[no-]autostash option is only valid with --rebase." >expect &&
>> + test_i18ncmp actual expect
>> +'
>
> Same comment as above about code common to these two tests. However,
> in this case, it might be easier simply to use a 'for' loop rather
> than a function:
>
> for i in --autostash --no-autostash
> do
> test_expect_success "pull $i (without --rebase) is illegal" "
> test_must_fail git pull $i . copy 2>actual &&
> test_i18ngrep 'only valid with --rebase' actual
> "
> done
>
> Take special note of how use of double (") and single (') quotes
> differ in this case from other tests since $i needs to be interpolated
> into the test body.
I agree with all of these comments. I will introduce two new function to
reduce the code and the above mention loop. Also the work on Matthieu's
comment.
I feel that most of your comments are necessary and should be there in
the next patch. But I have a doubt regarding the next patch. As Junio has
merged v10 of current series in next branch (as noticed from his mail),
sending a new patch should be based on the current patch (i.e. on next
branch) or master branch (i.e. continuing with this series)?
Thanks,
Mehul
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v10 2/2] pull --rebase: add --[no-]autostash flag
2016-03-25 18:07 ` Mehul Jain
@ 2016-03-25 22:29 ` Eric Sunshine
0 siblings, 0 replies; 16+ messages in thread
From: Eric Sunshine @ 2016-03-25 22:29 UTC (permalink / raw)
To: Mehul Jain; +Cc: Junio C Hamano, Git List, Matthieu Moy, Paul Tan
On Fri, Mar 25, 2016 at 2:07 PM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
> On Fri, Mar 25, 2016 at 2:01 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> Nit: Some of the test titles spell this as "rebase.autostash" while
>> others use "rebase.autoStash".
>> [...]
>> The title says that this is testing with rebase.autoStash unset,
>> however, the test itself doesn't take any action to ensure that it is
>> indeed unset. As with the two above tests which explicitly set
>> rebase.autoStash, this test should explicitly unset rebase.autoStash
>> to ensure consistent results even if some future change somehow
>> pollutes the configuration globally. Therefore:
>> [...]
>> With the addition of these three new tests, aside from the
>> introductory 'test_{un}config', this exact sequence of commands is now
>> repeated four times in the script. Such repetition suggests that the
>> common code should be moved to a function. For instance:
>
> I agree with all of these comments. I will introduce two new function to
> reduce the code and the above mention loop. Also the work on Matthieu's
> comment.
>
> I feel that most of your comments are necessary and should be there in
> the next patch. But I have a doubt regarding the next patch. As Junio has
> merged v10 of current series in next branch (as noticed from his mail),
> sending a new patch should be based on the current patch (i.e. on next
> branch) or master branch (i.e. continuing with this series)?
I hadn't noticed that v10 was already in 'next'. In this case, the
suggested changes should be a new patch series which makes incremental
changes to what is already in 'next'. Be sure to mention in the cover
letter that the new series should be applied atop
mj/pull-rebase-autostash.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v10 2/2] pull --rebase: add --[no-]autostash flag
2016-03-21 20:12 ` Mehul Jain
2016-03-25 8:31 ` Eric Sunshine
@ 2016-03-25 9:05 ` Matthieu Moy
2016-03-25 18:10 ` Mehul Jain
1 sibling, 1 reply; 16+ messages in thread
From: Matthieu Moy @ 2016-03-25 9:05 UTC (permalink / raw)
To: Mehul Jain; +Cc: git, gitster, pyokagan, sunshine
Mehul Jain <mehul.jain2029@gmail.com> writes:
> +--autostash::
> +--no-autostash::
> + Before starting rebase, stash local modifications away (see
> + linkgit:git-stash[1]) if needed, and apply the stash when
> + done. `--no-autostash` is useful to override the `rebase.autoStash`
> + configuration variable (see linkgit:git-config[1]).
> ++
> +This option is only valid when "--rebase" is used.
This does not have to be added to this series (I don't want to break
everything at v10 ...), but I think it would be nice to allow "git pull
--autostash" even without --rebase if pull.rebase=true.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v10 2/2] pull --rebase: add --[no-]autostash flag
2016-03-25 9:05 ` Matthieu Moy
@ 2016-03-25 18:10 ` Mehul Jain
2016-03-25 18:37 ` Matthieu Moy
0 siblings, 1 reply; 16+ messages in thread
From: Mehul Jain @ 2016-03-25 18:10 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Git Mailing List, Junio C Hamano, Paul Tan, Eric Sunshine
On Fri, Mar 25, 2016 at 2:35 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Mehul Jain <mehul.jain2029@gmail.com> writes:
>
>> +--autostash::
>> +--no-autostash::
>> + Before starting rebase, stash local modifications away (see
>> + linkgit:git-stash[1]) if needed, and apply the stash when
>> + done. `--no-autostash` is useful to override the `rebase.autoStash`
>> + configuration variable (see linkgit:git-config[1]).
>> ++
>> +This option is only valid when "--rebase" is used.
>
> This does not have to be added to this series (I don't want to break
> everything at v10 ...), but I think it would be nice to allow "git pull
> --autostash" even without --rebase if pull.rebase=true.
This is a nice observation. As current patch allow "git pull --autostash"
to be run without --rebase if pull.rebase=true, hence correct
documentation should be something like this
This option is only valid when "--rebase" is used or pull.rebase=true.
But OTOH users who knows about pull.rebase understands that
pull.rebase=true means "git pull --rebase ..." will be executed whenever
"git pull ..." is called, thus for those users it might be easy to deduce that
need of "--rebase" for validity of "--autostash" is not necessary if
pull.rebase=true.
I will correct it in the re-roll.
Thanks,
Mehul
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v10 2/2] pull --rebase: add --[no-]autostash flag
2016-03-25 18:10 ` Mehul Jain
@ 2016-03-25 18:37 ` Matthieu Moy
2016-03-25 19:07 ` Mehul Jain
0 siblings, 1 reply; 16+ messages in thread
From: Matthieu Moy @ 2016-03-25 18:37 UTC (permalink / raw)
To: Mehul Jain; +Cc: Git Mailing List, Junio C Hamano, Paul Tan, Eric Sunshine
Mehul Jain <mehul.jain2029@gmail.com> writes:
> On Fri, Mar 25, 2016 at 2:35 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Mehul Jain <mehul.jain2029@gmail.com> writes:
>>
>>> +--autostash::
>>> +--no-autostash::
>>> + Before starting rebase, stash local modifications away (see
>>> + linkgit:git-stash[1]) if needed, and apply the stash when
>>> + done. `--no-autostash` is useful to override the `rebase.autoStash`
>>> + configuration variable (see linkgit:git-config[1]).
>>> ++
>>> +This option is only valid when "--rebase" is used.
>>
>> This does not have to be added to this series (I don't want to break
>> everything at v10 ...), but I think it would be nice to allow "git pull
>> --autostash" even without --rebase if pull.rebase=true.
>
> This is a nice observation. As current patch allow "git pull --autostash"
> to be run without --rebase if pull.rebase=true,
OK, I misread the patch assuming that opt_rebase was only reflecting the
options, but it is also set by the config:
if (opt_rebase < 0)
opt_rebase = config_get_rebase();
> hence correct documentation should be something like this
>
> This option is only valid when "--rebase" is used or pull.rebase=true.
... or just "when pull is used in rebase mode", which is shorter and
still technically accurate. I don't think you need to be exhaustive in
this kind of documentation, the user will notice anyway if he tries to
use --autostash in a forbidden situation.
> But OTOH users who knows about pull.rebase understands that
> pull.rebase=true means "git pull --rebase ..." will be executed whenever
> "git pull ..." is called, thus for those users it might be easy to deduce that
> need of "--rebase" for validity of "--autostash" is not necessary if
> pull.rebase=true.
I'd rather have something technically correct.
I think you should also change one of the tests to use pull.resbase=true
so that this behavior is properly tested.
Thanks,
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v10 0/2] introduce --[no-]autostash command line flag
2016-03-21 18:18 [PATCH v10 0/2] introduce --[no-]autostash command line flag Mehul Jain
` (2 preceding siblings ...)
2016-03-21 20:12 ` Mehul Jain
@ 2016-03-25 7:23 ` Eric Sunshine
2016-03-25 18:01 ` Mehul Jain
3 siblings, 1 reply; 16+ messages in thread
From: Eric Sunshine @ 2016-03-25 7:23 UTC (permalink / raw)
To: Mehul Jain; +Cc: Git List, Junio C Hamano, Matthieu Moy, Paul Tan
On Mon, Mar 21, 2016 at 2:18 PM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
> Changes introduced w.r.t. previous patch:
> [...]
> * Two more tests are added to checkout for error when "git pull
> --[no-]autostash" is called. Here I'm forced to use "test_i18ncmp"
> instead of "test_i18ngrep" to compare the expected error message with
> the actual because grep was, unfortunately, reading "--[no-]autostash"
> as an option and thus leading to test failure.
Pass -e to grep to treat the next argument as an expression (even if
it happens to look like an option):
test_i18ngrep -e "--[no-]-autostash ..."
You may also need to escape the [ and ] with backslash (\) to force
grep to treat them as literal characters rather than as the character
set "[no-]". Alternately, rather than escaping, also pass the -F flag
to make it treat all characters as literals.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v10 0/2] introduce --[no-]autostash command line flag
2016-03-25 7:23 ` [PATCH v10 0/2] introduce --[no-]autostash command line flag Eric Sunshine
@ 2016-03-25 18:01 ` Mehul Jain
0 siblings, 0 replies; 16+ messages in thread
From: Mehul Jain @ 2016-03-25 18:01 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List, Junio C Hamano, Matthieu Moy, Paul Tan
On Fri, Mar 25, 2016 at 12:53 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Mar 21, 2016 at 2:18 PM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
>> Changes introduced w.r.t. previous patch:
>> [...]
>> * Two more tests are added to checkout for error when "git pull
>> --[no-]autostash" is called. Here I'm forced to use "test_i18ncmp"
>> instead of "test_i18ngrep" to compare the expected error message with
>> the actual because grep was, unfortunately, reading "--[no-]autostash"
>> as an option and thus leading to test failure.
>
> Pass -e to grep to treat the next argument as an expression (even if
> it happens to look like an option):
>
> test_i18ngrep -e "--[no-]-autostash ..."
>
> You may also need to escape the [ and ] with backslash (\) to force
> grep to treat them as literal characters rather than as the character
> set "[no-]". Alternately, rather than escaping, also pass the -F flag
> to make it treat all characters as literals.
Thanks for this. I tried it out
test_i18ngrep -F -e "--[no-]autostash ..." err
and worked fine.
Thanks,
Mehul
^ permalink raw reply [flat|nested] 16+ messages in thread