[PATCH v9 1/2] git-pull.c: introduce git_pull_config()

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 [off-list ref]
Helped-by: Paul Tan [off-list ref]
Signed-off-by: Mehul Jain <redacted>
---
previous patches: http://thread.gmane.org/gmane.comp.version-control.git/287709
 builtin/pull.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 10eff03..72f4475 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;

@@ -304,6 +305,17 @@ static enum rebase_type config_get_rebase(void)
 
 	return REBASE_FALSE;
 }
+/**
+ * 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.

@@ -823,7 +835,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,13 +847,10 @@ 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 (!config_autostash)
 			die_on_unclean_work_tree(prefix);
 
 		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))

-- 
2.7.1.340.g69eb491.dirty

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 [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Helped-by: Paul Tan [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Mehul Jain <redacted>
---
previous patches: http://thread.gmane.org/gmane.comp.version-control.git/287709

Changes:
	* Modified documentation
	* "git pull --[no-]autostash" case is handled bit early then before

 Documentation/git-pull.txt |  9 +++++++++
 builtin/pull.c             | 12 +++++++++++-
 t/t5520-pull.sh            | 39 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 1 deletion(-)

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 72f4475..671179b 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),

@@ -801,6 +804,7 @@ 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);
+	argv_array_push(&args, opt_autostash ? "--autostash" : "--no-autostash");
 
 	argv_array_push(&args, "--onto");
 	argv_array_push(&args, sha1_to_hex(merge_head));

@@ -846,11 +850,17 @@ 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) {
 		if (is_null_sha1(orig_head) && !is_cache_unborn())
 			die(_("Updating an unborn branch with changes added to the index."));
 
-		if (!config_autostash)
+		if (opt_autostash == -1)
+			opt_autostash = config_autostash;
+
+		if (!opt_autostash)
 			die_on_unclean_work_tree(prefix);
 
 		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index c952d5e..85d9bea 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh

@@ -255,6 +255,45 @@ test_expect_success 'pull --rebase succeeds with dirty working directory and reb
 	test "$(cat new_file)" = dirty &&
 	test "$(cat file)" = "modified again"
 '
+test_expect_success 'pull --rebase: --autostash overrides rebase.autostash' '
+	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 works with rebase.autostash set 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: --no-autostash overrides rebase.autostash' '
+	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 works with rebase.autostash set 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' '
 	git reset --hard before-rebase &&

-- 
2.7.1.340.g69eb491.dirty

On Thu, Mar 17, 2016 at 12:49 PM, Mehul Jain [off-list ref] 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.

Signed-off-by: Mehul Jain <redacted>
---
previous patches: http://thread.gmane.org/gmane.comp.version-control.git/287709

Changes:
        * Modified documentation

This would be more helpful for reviewers if it went into a bit more
detail; "modified" alone doesn't say much. For instance, "... to keep
the description of --autostash and --no-autostash together rather than
splitting them apart with a tangential comment" or something.

        * "git pull --[no-]autostash" case is handled bit early then before

Likewise, explaining why it's handled a bit earlier would help
reviewers. For instance, "... since getting the error handling case
out of the way early makes the following code easier to reason about"
or something.

Since this is now a patch series rather than a single patch, another
way to help reviewers is to use a cover letter (see git-format-patch
--cover-letter) where you'd explain the changes, and, importantly,
include an interdiff between the previous and current versions.

At this point, we know that opt_autostash can't be -1 (thus
incorrectly triggering use of --autostash) because the conditional in
cmd_pull() set it to the value of config_autostash (either 0 or 1) if
the user did not specify it on the command-line. Okay. Makes sense.

Would an assert(opt_autostash != -1) to document this be desirable? (I
don't feel strongly about it, and it's certainly not worth a re-roll.)

Why do titles of some of the new test titles have a ":" after "rebase"
while other don't?

Also, how about normalizing the titles so that the reader knows in
which tests rebase.autostash is 'true' and in which it is 'false'?
Presently, it's difficult to decipher what's being tested based only
on the titles.

Finally, shouldn't you also be testing --autostash and --no-autostash
when rebase.autostash is not set?

+       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 works with rebase.autostash set 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: --no-autostash overrides rebase.autostash' '
+       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 works with rebase.autostash set 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
+'

On Fri, Mar 18, 2016 at 12:24 AM, Eric Sunshine [off-list ref] wrote:

On Thu, Mar 17, 2016 at 12:49 PM, Mehul Jain [off-list ref] wrote:

quoted

@@ -801,6 +804,7 @@ 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);
+       argv_array_push(&args, opt_autostash ? "--autostash" : "--no-autostash");

At this point, we know that opt_autostash can't be -1 (thus
incorrectly triggering use of --autostash) because the conditional in
cmd_pull() set it to the value of config_autostash (either 0 or 1) if
the user did not specify it on the command-line. Okay. Makes sense.

Actually, this is going to pass --autostash or --no-autostash to
git-rebase unconditionally won't it? This seems kind of undesirable
due to the unnecessarily tight coupling it creates between the two
commands. I wasn't paying close attention to the earlier discussion,
but wasn't the idea that you should pass one of these two options
along to git-rebase only if the user explicitly asked to do by saying
so on the command line?

In other words:

* invoke "git-rebase --autostash" only if the user typed "git pull
--rebase --autostash"

* invoke "git-rebase --no-autostash" only if the user typed "git pull
--rebase --no-autostash"

* invoke "git rebase" if the user typed bare "git pull --rebase"

On Fri, Mar 18, 2016 at 9:54 AM, Eric Sunshine [off-list ref] wrote:

Since this is now a patch series rather than a single patch, another
way to help reviewers is to use a cover letter (see git-format-patch
--cover-letter) where you'd explain the changes, and, importantly,
include an interdiff between the previous and current versions.

Point noted for the future patches.

quoted

+test_expect_success 'pull --rebase: --autostash overrides rebase.autostash' '

Why do titles of some of the new test titles have a ":" after "rebase"
while other don't?

Also, how about normalizing the titles so that the reader knows in
which tests rebase.autostash is 'true' and in which it is 'false'?
Presently, it's difficult to decipher what's being tested based only
on the titles.

If it's so then how about the tests titles to be the following:

* pull --rebase: --autostash works with rebase.autoStash set true

* pull --rebase: --autostash works with rebase.autoStash set false

* pull --rebase: --no-autostash works with rebase.autoStash set true

* pull --rebase: --no-autostash works with rebase.autoStash set false

Earlier I tried to keep it as less verbose as possible (and probably
made it hard to decipher). Does the above titles seems short and
informative to you? If so then I will use them instead of earlier ones.

Finally, shouldn't you also be testing --autostash and --no-autostash
when rebase.autostash is not set?

If rebase.autoStash is not set then config.autostash will remain zero
through out the process. What I want to point out is that rebase.autoStash
, if not set, is equivalent to being set false. So adding tests regarding
"--[no-]autostash with rebase.autoStash unset" seems equivalent to tests
" pull --rebase: --autostash works with rebase.autoStash set false" and
"pull --rebase: --no-autostash works with rebase.autoStash set false".

Thanks,
Mehul

On Fri, Mar 18, 2016 at 10:09 AM, Eric Sunshine [off-list ref] wrote:

On Fri, Mar 18, 2016 at 12:24 AM, Eric Sunshine [off-list ref] wrote:

quoted

On Thu, Mar 17, 2016 at 12:49 PM, Mehul Jain [off-list ref] wrote:

quoted

@@ -801,6 +804,7 @@ 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);
+       argv_array_push(&args, opt_autostash ? "--autostash" : "--no-autostash");

At this point, we know that opt_autostash can't be -1 (thus
incorrectly triggering use of --autostash) because the conditional in
cmd_pull() set it to the value of config_autostash (either 0 or 1) if
the user did not specify it on the command-line. Okay. Makes sense.

Actually, this is going to pass --autostash or --no-autostash to
git-rebase unconditionally won't it? This seems kind of undesirable
due to the unnecessarily tight coupling it creates between the two
commands. I wasn't paying close attention to the earlier discussion,
but wasn't the idea that you should pass one of these two options
along to git-rebase only if the user explicitly asked to do by saying
so on the command line?

This is interesting. I checked out git-rebase.sh and found that it reads
rebase.autoStash if nothing is specified by user. So if user is not
specifying anything about stashing then it is the job of git-rebase
to decide whether or not to do stashing by reading rebase.autoStash.

Similarly if user doesn't specify the --[no-]autostash option to git-pull
then neither of --autostash and --no-autstash should be passed to the
git-rebase as it will decide on his own about what needs to be done.
Agreed. I made a unnecessary tight coupling between git-pull and
git-rebase. Instead of that the following changes can be done to
remove it.

...
        if (opt_gpg_sign)
                argv_array_push(&args, opt_gpg_sign);
-       argv_array_push(&args, opt_autostash ? "--autostash" :
"--no-autostash");
+       if (opt_autostash == 0)
+               argv_array_push(&args, "--no-autostash");
+       else if (opt_autostash == 1)
+               argv_array_push(&args, "--autostash");

...

...
        if (opt_rebase) {
+               int autostash = config_autostash;
                if (is_null_sha1(orig_head) && !is_cache_unborn())
                        die(_("... with changes added to the index."));

-               if (opt_autostash == -1)
-                       opt_autostash = config_autostash;
+               if (opt_autostash != -1)
+                       autostash = opt_autostash;

-               if (!opt_autostash)
+               if (!autostash)
                        die_on_unclean_work_tree(prefix);

                if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
                        hashclr(rebase_fork_point);
        }
...

Note that the above changes are suggest with respect to my patch, not
the current
code base.

This way there's no need to remove "autostash" from the current code
base and instead use it to write a much cleaner patch.  Something like
this (this is w.r.t. current code base)

...
        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");

...

...

if (opt_rebase) {
                 int autostash = config_autostash;
+               if (opt_autostash != -1)
+                       autostash = opt_autostash;

                if (is_null_sha1(orig_head) && !is_cache_unborn())
                        die(_("... with changes added to the index."));

                if (!autostash)
                        die_on_unclean_work_tree(prefix);

                if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
                        hashclr(rebase_fork_point);
        }
...


What are your views on this? Also this way I will not touch changes
introduced in patch 1/2.

In other words:

* invoke "git-rebase --autostash" only if the user typed "git pull
--rebase --autostash"

* invoke "git-rebase --no-autostash" only if the user typed "git pull
--rebase --no-autostash"

* invoke "git rebase" if the user typed bare "git pull --rebase"

Thanks,
Mehul

On Fri, Mar 18, 2016 at 11:17 AM, Mehul Jain [off-list ref] wrote:

On Fri, Mar 18, 2016 at 9:54 AM, Eric Sunshine [off-list ref] wrote:

quoted

quoted

+test_expect_success 'pull --rebase: --autostash overrides rebase.autostash' '

Why do titles of some of the new test titles have a ":" after "rebase"
while other don't?

Also, how about normalizing the titles so that the reader knows in
which tests rebase.autostash is 'true' and in which it is 'false'?
Presently, it's difficult to decipher what's being tested based only
on the titles.

If it's so then how about the tests titles to be the following:

* pull --rebase: --autostash works with rebase.autoStash set true

* pull --rebase: --autostash works with rebase.autoStash set false

* pull --rebase: --no-autostash works with rebase.autoStash set true

* pull --rebase: --no-autostash works with rebase.autoStash set false

Earlier I tried to keep it as less verbose as possible (and probably
made it hard to decipher). Does the above titles seems short and
informative to you? If so then I will use them instead of earlier ones.

Those are better. If I was doing it, I'd probably drop the unnecessary
":", "works with", and "set", so:

    pull --rebase --autostash & rebase.autoStash=true
    pull --rebase --autostash & rebase.autoStash=false
    pull --rebase --no-autostash & rebase.autoStash=true
    pull --rebase --no-autostash & rebase.autoStash=false

or something, but that's a very minor point.

quoted

Finally, shouldn't you also be testing --autostash and --no-autostash
when rebase.autostash is not set?

If rebase.autoStash is not set then config.autostash will remain zero
through out the process. What I want to point out is that rebase.autoStash
, if not set, is equivalent to being set false. So adding tests regarding
"--[no-]autostash with rebase.autoStash unset" seems equivalent to tests
" pull --rebase: --autostash works with rebase.autoStash set false" and
"pull --rebase: --no-autostash works with rebase.autoStash set false".

Yes, but what you've described is how the current *implementation*
works, whereas the tests should be checking expected *behavior*. So,
while we both know that under the current implementation, checking:

    pull --rebase --autostash & rebase.autoStash unset

is the same as checking:

    pull --rebase--autostash & rebase.autoStash=false

some future change to the implementation could (accidentally) break
this equivalence, and we want to protect against such breakage by
checking behavior, not implementation.


Also, I forgot to mention a couple other missing tests you should add:

    * pull --autostash (without --rebase) should error out
    * pull --no-autostash (without --rebase) should error out

On Fri, Mar 18, 2016 at 11:17 AM, Mehul Jain [off-list ref] wrote:

On Fri, Mar 18, 2016 at 10:09 AM, Eric Sunshine [off-list ref] wrote:

quoted

Actually, this is going to pass --autostash or --no-autostash to
git-rebase unconditionally won't it? This seems kind of undesirable
due to the unnecessarily tight coupling it creates between the two
commands. I wasn't paying close attention to the earlier discussion,
but wasn't the idea that you should pass one of these two options
along to git-rebase only if the user explicitly asked to do by saying
so on the command line?

This is interesting. I checked out git-rebase.sh and found that it reads
rebase.autoStash if nothing is specified by user. So if user is not
specifying anything about stashing then it is the job of git-rebase
to decide whether or not to do stashing by reading rebase.autoStash.

Similarly if user doesn't specify the --[no-]autostash option to git-pull
then neither of --autostash and --no-autstash should be passed to the
git-rebase as it will decide on his own about what needs to be done.
Agreed. I made a unnecessary tight coupling between git-pull and
git-rebase. Instead of that the following changes can be done to
remove it.

This way there's no need to remove "autostash" from the current code
base and instead use it to write a much cleaner patch.  Something like
this (this is w.r.t. current code base)
[...]

What are your views on this?

I think this makes the patches cleaner and the final code nicer, and
it eliminates the too-tight coupling between the two commands, so it
seems to be a win overall.