[PATCH v2 1/2] pull --rebase: add --[no-]autostash flag

classic Classic list List threaded Threaded
44 messages Options
123
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 1/2] pull --rebase: add --[no-]autostash flag

Mehul Jain
git pull --rebase understands --[no-]autostash flag.

This flag overrides config variable "rebase.autoStash"
if set (default is false).

When calling "git pull --rebase" with "--autostash",
pull passes the "--autostash" option to rebase,
which then runs rebase on a dirty worktree.

With "--no-autostash" option, the command will die
if the worktree is dirty, before calling rebase.

Signed-off-by: Mehul Jain <[hidden email]>
---

Thanks to Paul and Matthieu for comments on previous round.
Changes:
  - --autostash flag added
        - OPT_COLOR_FLAG replaced by OPT_BOOL
        - Default value of opt_rebase changed
        - Few changes in code
        - Commit message improvements
        - Documentation added
        - Few tests removed as suggested by Paul
        - Added test for --autostash flag
All tests passed: https://travis-ci.org/mehul2029/git 

 builtin/pull.c          | 13 ++++++++-----
 t/t5520-pull.sh         | 19 +++++++++++++++++++
 t/t5521-pull-options.sh | 16 ++++++++++++++++
 3 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 10eff03..60b320e 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 = 0;
 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_BOOL(0,"autostash",&opt_autostash,
+ N_("automatically stash/stash pop before and after rebase")),
  OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
  N_("verify that the named commit has a valid GPG signature"),
  PARSE_OPT_NOARG),
@@ -789,7 +792,8 @@ 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)
+ argv_array_push(&args,"--autostash");
  argv_array_push(&args, "--onto");
  argv_array_push(&args, sha1_to_hex(merge_head));
 
@@ -813,6 +817,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
  if (!getenv("GIT_REFLOG_ACTION"))
  set_reflog_message(argc, argv);
 
+ git_config_get_bool("rebase.autostash",&opt_autostash);
+
  argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);
 
  parse_repo_refspecs(argc, argv, &repo, &refspecs);
@@ -835,13 +841,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 (!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..d80b6cc 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -245,6 +245,25 @@ test_expect_success '--rebase fails with multiple branches' '
  test modified = "$(git show HEAD:file)"
 '
 
+test_expect_success 'pull --rebase --no-autostash fails with dirty working directory and rebase.autstash set false' '
+ 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 --autostash succeeds with dirty working directory and rebase.autstash set 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 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..3930e45 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -62,6 +62,22 @@ test_expect_success 'git pull -v --rebase' '
  test_must_be_empty out)
 '
 
+test_expect_success 'git pull --rebase --autostash' '
+ mkdir clonedrbas &&
+ (cd clonedrbas  && git init &&
+ git pull --rebase --autostash "../parent" >out 2>err &&
+ test -s err &&
+ test_must_be_empty out)
+'
+
+test_expect_success 'git pull --rebase --no-autostash' '
+ mkdir clonedrbnas &&
+ (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 -v -q' '
  mkdir clonedvq &&
  (cd clonedvq && git init &&
--
2.7.1.340.g69eb491.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 2/2] Documentation/git-pull.txt: teach 'git pull --rebase' the --[no-]autostash option.

Mehul Jain
git pull --rebase understands --[no-]autostash flag.

This flag overrides config variable "rebase.autoStash"
if set (default is false).

When calling "git pull --rebase" with "--autostash",
pull passes the "--autostash" option to rebase,
which then runs rebase on a dirty worktree.

With "--no-autostash" option, the command will die
if the worktree is dirty, before calling rebase.
       
Signed-off-by: Mehul Jain <[hidden email]>
---
 Documentation/git-pull.txt | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index a62a2a6..ce22c52 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -128,6 +128,22 @@ unless you have read linkgit:git-rebase[1] carefully.
 --no-rebase::
  Override earlier --rebase.
 
+--autostash::
+ Automatically create a temporary stash before the operation
+ begins, and apply it after the operation ends. This means
+ that you can run rebase on a dirty worktree.
++
+This option is only valid when '--rebase' option is used.
++
+[NOTE]
+Use with care: the final stash application after a successful
+rebase might result in non-trivial conflicts.
+
+--no-autostash::
+ If the '--autostash' option is enabled by default using the
+ configuration variable `rebase.autoStash`, this option can be
+ used to override this setting.
+
 Options related to fetching
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
--
2.7.1.340.g69eb491.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 1/2] pull --rebase: add --[no-]autostash flag

Junio C Hamano
In reply to this post by Mehul Jain
Mehul Jain <[hidden email]> writes:

> git pull --rebase understands --[no-]autostash flag.
>
> This flag overrides config variable "rebase.autoStash"
> if set (default is false).

Is that a statement of a fact?  If so, is it true before this patch
is applied, or after?

Each project has local convention for log messages, and we do too.
A long log message typically start by explaining how the world is,
why that is not desirable, present a description of a possible world
that is better, and then give commands to somebody who is updating
the code telling him what to do to make that better world a reality
(and optionally how).

So perhaps (I am totally making this up; you need to fact check and
adjust):

    If you enable rebase.autoStash option in your repository, there
    is no way to override it for "git pull --rebase" from the
    command line.

    Teach "git pull" a new "--[no-]autostash" option so that a
    rebase.autoStash configuration can be overridden.  As "git
    rebase" already knows "--[no-]autostash" option, it is just the
    matter of passing one when we spawn the command as necessary.

or something.  The first one gives the readers how the current world
works, and why it is not ideal.  The proposed better world in this
case is too simple--the first paragraph complained that "we cannot
do X" and X is something reader would likely to agree is a good
thing to do, so it can be left unsaid that a better world is one in
which X can be done.

> When calling "git pull --rebase" with "--autostash",
> pull passes the "--autostash" option to rebase,
> which then runs rebase on a dirty worktree.
>
> With "--no-autostash" option, the command will die
> if the worktree is dirty, before calling rebase.

These two paragraphs are too obvious and probably are better left
unsaid.  Especially the latter--you are changing "pull" and do not
control what "rebase" would do in the future.  It could be that a
better rebase in the future may be able to do its job in a dirty
worktree without doing an autostash.

> Signed-off-by: Mehul Jain <[hidden email]>
> ---
>
> Thanks to Paul and Matthieu for comments on previous round.
> Changes:
>   - --autostash flag added
> - OPT_COLOR_FLAG replaced by OPT_BOOL
> - Default value of opt_rebase changed
> - Few changes in code
> - Commit message improvements
> - Documentation added
> - Few tests removed as suggested by Paul
> - Added test for --autostash flag
> All tests passed: https://travis-ci.org/mehul2029/git 
>
>  builtin/pull.c          | 13 ++++++++-----
>  t/t5520-pull.sh         | 19 +++++++++++++++++++
>  t/t5521-pull-options.sh | 16 ++++++++++++++++
>  3 files changed, 43 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 10eff03..60b320e 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 = 0;

Do not explicitly initialize a static to 0 (or NULL).

>  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_BOOL(0,"autostash",&opt_autostash,
> + N_("automatically stash/stash pop before and after rebase")),
>   OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
>   N_("verify that the named commit has a valid GPG signature"),
>   PARSE_OPT_NOARG),
> @@ -789,7 +792,8 @@ 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)

Style: control keywords are followed by a single SP before the next '('.

> + argv_array_push(&args,"--autostash");

Style: a single SP after a comma.

How would --no-autostash defeat a configured rebase.autostash with this?

By the way, how would this affect "git pull --autostash" that is run
without "--rebase"?  If this is an option to "git pull", shouldn't
the stashing done even when you are not doing a rebase but making a
merge?

>   argv_array_push(&args, "--onto");
>   argv_array_push(&args, sha1_to_hex(merge_head));
>  
> @@ -813,6 +817,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>   if (!getenv("GIT_REFLOG_ACTION"))
>   set_reflog_message(argc, argv);
>  
> + git_config_get_bool("rebase.autostash",&opt_autostash);
> +

Why is this change even necessary?

>   argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);
>  
>   parse_repo_refspecs(argc, argv, &repo, &refspecs);
> @@ -835,13 +841,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 (!opt_autostash)
>   die_on_unclean_work_tree(prefix);

I would have expected that

 * a global opt_autostash is initialized to -1 (unspecified);

 * opt_bool() would flip it to either 0 or 1 with --[no-]autostash;

 * existing "rebase.autostash" configuration check inside "git pull"
   code  gets removed;

 * and the code that builds "git rebase" invocation command line
   will do

    if (opt_autostash < 0)
        ; /* do nothing */
        else if (opt_autostash == 0)
        argv_array_push(&args, "--no-autostash");
        else
        argv_array_push(&args, "--autostash");
       
Then when "git pull --rebase" is run without "--[no-]autostash", the
underlying "git rebase" would be run without that option, and does its
usual thing, including reading rebase.autostash and deciding to do
"git stash".  And when "git pull" is run with "--[no-]autostash",
the underlying "git rebase" would be given the same option, and
would do what it was told to do, ignoring rebase.autostash setting.

So why does "git pull" still need to look at rebase.autostash
configuration after this change?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 1/2] pull --rebase: add --[no-]autostash flag

Mehul Jain
On Sun, Feb 28, 2016 at 12:56 AM, Junio C Hamano <[hidden email]> wrote:

> Mehul Jain <[hidden email]> writes:
>
>> git pull --rebase understands --[no-]autostash flag.
>>
>> This flag overrides config variable "rebase.autoStash"
>> if set (default is false).
>
> Is that a statement of a fact?  If so, is it true before this patch
> is applied, or after?
>
> Each project has local convention for log messages, and we do too.
> A long log message typically start by explaining how the world is,
> why that is not desirable, present a description of a possible world
> that is better, and then give commands to somebody who is updating
> the code telling him what to do to make that better world a reality
> (and optionally how).
>
> So perhaps (I am totally making this up; you need to fact check and
> adjust):
>
>     If you enable rebase.autoStash option in your repository, there
>     is no way to override it for "git pull --rebase" from the
>     command line.
>
>     Teach "git pull" a new "--[no-]autostash" option so that a
>     rebase.autoStash configuration can be overridden.  As "git
>     rebase" already knows "--[no-]autostash" option, it is just the
>     matter of passing one when we spawn the command as necessary.
>
> or something.  The first one gives the readers how the current world
> works, and why it is not ideal.  The proposed better world in this
> case is too simple--the first paragraph complained that "we cannot
> do X" and X is something reader would likely to agree is a good
> thing to do, so it can be left unsaid that a better world is one in
> which X can be done.
>
>> When calling "git pull --rebase" with "--autostash",
>> pull passes the "--autostash" option to rebase,
>> which then runs rebase on a dirty worktree.
>>
>> With "--no-autostash" option, the command will die
>> if the worktree is dirty, before calling rebase.
>
> These two paragraphs are too obvious and probably are better left
> unsaid.  Especially the latter--you are changing "pull" and do not
> control what "rebase" would do in the future.  It could be that a
> better rebase in the future may be able to do its job in a dirty
> worktree without doing an autostash.

OK. I will follow up with a better commit message.

>> Signed-off-by: Mehul Jain <[hidden email]>
>> ---
>>
>> Thanks to Paul and Matthieu for comments on previous round.
>> Changes:
>>       - --autostash flag added
>>       - OPT_COLOR_FLAG replaced by OPT_BOOL
>>       - Default value of opt_rebase changed
>>       - Few changes in code
>>       - Commit message improvements
>>       - Documentation added
>>       - Few tests removed as suggested by Paul
>>       - Added test for --autostash flag
>> All tests passed: https://travis-ci.org/mehul2029/git
>>
>>  builtin/pull.c          | 13 ++++++++-----
>>  t/t5520-pull.sh         | 19 +++++++++++++++++++
>>  t/t5521-pull-options.sh | 16 ++++++++++++++++
>>  3 files changed, 43 insertions(+), 5 deletions(-)
>>
>> diff --git a/builtin/pull.c b/builtin/pull.c
>> index 10eff03..60b320e 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 = 0;
>
> Do not explicitly initialize a static to 0 (or NULL).
>
>>  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_BOOL(0,"autostash",&opt_autostash,
>> +             N_("automatically stash/stash pop before and after rebase")),
>>       OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
>>               N_("verify that the named commit has a valid GPG signature"),
>>               PARSE_OPT_NOARG),
>> @@ -789,7 +792,8 @@ 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)
>
> Style: control keywords are followed by a single SP before the next '('.
>
>> +             argv_array_push(&args,"--autostash");
>
> Style: a single SP after a comma.
>
> How would --no-autostash defeat a configured rebase.autostash with this?
>
> By the way, how would this affect "git pull --autostash" that is run
> without "--rebase"?  If this is an option to "git pull", shouldn't
> the stashing done even when you are not doing a rebase but making a
> merge?

As "git rebase" takes "--[no-]autostash" as an option whereas "git
merge" does not,
hence "--[no-]autostash" option is only valid when "--rebase" option is used in
"git pull".

If user uses "git pull --[no-]autostash" then two possible things can be done:

           * Either "git pull" ignores "--[no-]autostash" and calls
underlying "git merge",
             as merge stashes the untracked files by itself. Thus
ignoring --no-autostash
             flag given by user.

           * Or "git pull" dies with the following error:

                    "--[no-]autostash is only valid when --rebase is used.
                     Example: git pull --rebase --[no-]autostash"

I suggest that the latter option should be used in this case as user should know
that stashing is performed by "git merge" anyway and "--no-autostash"
flag is not
a way to tell "git merge" to not to do stashing. Also this error will
fit perfectly with the
documentation of "--[no-]autostash" option given in the [PATCH v2 2/2].

Please suggest what are your views on this.

>>       argv_array_push(&args, "--onto");
>>       argv_array_push(&args, sha1_to_hex(merge_head));
>>
>> @@ -813,6 +817,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>>       if (!getenv("GIT_REFLOG_ACTION"))
>>               set_reflog_message(argc, argv);
>>
>> +     git_config_get_bool("rebase.autostash",&opt_autostash);
>> +
>
> Why is this change even necessary?
>
>>       argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);
>>
>>       parse_repo_refspecs(argc, argv, &repo, &refspecs);
>> @@ -835,13 +841,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 (!opt_autostash)
>>                       die_on_unclean_work_tree(prefix);
>
> I would have expected that
>
>  * a global opt_autostash is initialized to -1 (unspecified);
>
>  * opt_bool() would flip it to either 0 or 1 with --[no-]autostash;
>
>  * existing "rebase.autostash" configuration check inside "git pull"
>    code  gets removed;
>
>  * and the code that builds "git rebase" invocation command line
>    will do
>
>         if (opt_autostash < 0)
>                 ; /* do nothing */
>         else if (opt_autostash == 0)
>                 argv_array_push(&args, "--no-autostash");
>         else
>                 argv_array_push(&args, "--autostash");
>
> Then when "git pull --rebase" is run without "--[no-]autostash", the
> underlying "git rebase" would be run without that option, and does its
> usual thing, including reading rebase.autostash and deciding to do
> "git stash".  And when "git pull" is run with "--[no-]autostash",
> the underlying "git rebase" would be given the same option, and
> would do what it was told to do, ignoring rebase.autostash setting.
>
> So why does "git pull" still need to look at rebase.autostash
> configuration after this change?

I agree with your point that future rebase might be able to do it's job on
dirty working tree without  autostash option, so it's always better to
let rebase
check for dirty working tree and do the task of looking at rebase.autostash.

Thanks for a thorough review.

Cheers,
Mehul Jain
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 1/2] pull --rebase: add --[no-]autostash flag

Matthieu Moy-2
Mehul Jain <[hidden email]> writes:

> If user uses "git pull --[no-]autostash" then two possible things can be done:
>
>            * Either "git pull" ignores "--[no-]autostash" and calls
>              underlying "git merge",
>              as merge stashes the untracked files by itself. Thus
>              ignoring --no-autostash
>              flag given by user.
>
>            * Or "git pull" dies with the following error:
>
>                     "--[no-]autostash is only valid when --rebase is used.
>                      Example: git pull --rebase --[no-]autostash"
>
> I suggest that the latter option should be used in this case as user should know
> that stashing is performed by "git merge" anyway

Not exactly. "git merge" doesn't do a stash, but it accepts to run if
local changes do not touch the same files as the merge. So, --autostash
is usually not needed for merge.

But I agree that erroring out is better. Silently accepting the option
and doing nothing with it would be confusing for users. This would mean
that "git pull --autostash" would work, but still sometimes show the error:

  error: Your local changes to the following files would be overwritten by merge:

The situation is different for command-line options and for config
variables: the config variable wasn't explicitly specified for each run
of "git pull", hence it's acceptable to ignore its value when we have
nothing to do with it.

--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 1/2] pull --rebase: add --[no-]autostash flag

Paul Tan
In reply to this post by Junio C Hamano
Hi Junio,

On Sun, Feb 28, 2016 at 3:26 AM, Junio C Hamano <[hidden email]> wrote:

> Mehul Jain <[hidden email]> writes:
>> @@ -835,13 +841,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 (!opt_autostash)
>>                       die_on_unclean_work_tree(prefix);
>
> I would have expected that
>
>  * a global opt_autostash is initialized to -1 (unspecified);
>
>  * opt_bool() would flip it to either 0 or 1 with --[no-]autostash;
>
>  * existing "rebase.autostash" configuration check inside "git pull"
>    code  gets removed;

Removing the "rebase.autostash" configuration check would bring back
the problem which 53c76dc (pull: allow dirty tree when
rebase.autostash enabled, 2015-07-04) fixed.

>  * and the code that builds "git rebase" invocation command line
>    will do
>
>         if (opt_autostash < 0)
>                 ; /* do nothing */
>         else if (opt_autostash == 0)
>                 argv_array_push(&args, "--no-autostash");
>         else
>                 argv_array_push(&args, "--autostash");
>
> Then when "git pull --rebase" is run without "--[no-]autostash", the
> underlying "git rebase" would be run without that option, and does its
> usual thing, including reading rebase.autostash and deciding to do
> "git stash".  And when "git pull" is run with "--[no-]autostash",
> the underlying "git rebase" would be given the same option, and
> would do what it was told to do, ignoring rebase.autostash setting.
>
> So why does "git pull" still need to look at rebase.autostash
> configuration after this change?

Ultimately, git-pull needs to be aware of whether autostash is active
or not (and this means rebase.autostash needs to be looked at as well)
because if autostash is disabled, git-pull needs to perform the
"worktree is clean" check. And this "worktree is clean" check needs to
be done *before* git-fetch and git-rebase is run.

See f9189cf (pull --rebase: exit early when the working directory is
dirty, 2008-05-21).

Regards,
Paul
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 1/2] pull --rebase: add --[no-]autostash flag

Junio C Hamano
Paul Tan <[hidden email]> writes:

> Ultimately, git-pull needs to be aware of whether autostash is active
> or not (and this means rebase.autostash needs to be looked at as well)
> because if autostash is disabled, git-pull needs to perform the
> "worktree is clean" check. And this "worktree is clean" check needs to
> be done *before* git-fetch and git-rebase is run.

Ahh, right. I forgot about that discussion.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v3 1/3] pull --rebase: add --[no-]autostash flag

Mehul Jain
In reply to this post by 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 <[hidden email]>
Helped-by: Junio C Hamano <[hidden email]>
Helped-by: Paul Tan <[hidden email]>
Signed-off-by: Mehul Jain <[hidden email]>
---
Sorry for a late follow up. I had my mid-semester
examination going on.

Previous patch: $gname287709

Changes:
        * error message is produced when "git pull
         --[no]autostash" is called.
  * opt_autostash intialized with -1.

 builtin/pull.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 10eff03..b338b83 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_BOOL(0, "autostash", &opt_autostash,
+ N_("automatically stash/stash pop before and after rebase")),
  OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
  N_("verify that the named commit has a valid GPG signature"),
  PARSE_OPT_NOARG),
@@ -789,7 +792,8 @@ 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)
+ argv_array_push(&args, "--autostash");
  argv_array_push(&args, "--onto");
  argv_array_push(&args, sha1_to_hex(merge_head));
 
@@ -835,18 +839,25 @@ 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 == -1)
+ git_config_get_bool("rebase.autostash", &opt_autostash);
+
+ if (opt_autostash == 0 || opt_autostash == -1)
  die_on_unclean_work_tree(prefix);
 
  if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
  hashclr(rebase_fork_point);
  }
+ else {
+ /* If --[no-]autostash option is called without --rebase */
+ if (opt_autostash == 0)
+ die(_("--no-autostash option is only valid with --rebase."));
+ else if (opt_autostash == 1)
+ die(_("--autostash option is only valid with --rebase."));
+ }
 
  if (run_fetch(repo, refspecs))
  return 1;
--
2.7.1.340.g69eb491.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v3 2/3] test: add test for --[no-]autostash flag

Mehul Jain
Signed-off-by: Mehul Jain <[hidden email]>
---
 t/t5520-pull.sh         | 19 +++++++++++++++++++
 t/t5521-pull-options.sh | 16 ++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index c952d5e..f5d1d31 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -245,6 +245,25 @@ test_expect_success '--rebase fails with multiple branches' '
  test modified = "$(git show HEAD:file)"
 '
 
+test_expect_success 'pull --rebase --no-autostash fails with dirty working directory and rebase.autstash set 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
+'
+
+test_expect_success 'pull --rebase --autostash succeeds with dirty working directory and rebase.autstash set 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 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..3930e45 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -62,6 +62,22 @@ test_expect_success 'git pull -v --rebase' '
  test_must_be_empty out)
 '
 
+test_expect_success 'git pull --rebase --autostash' '
+ mkdir clonedrbas &&
+ (cd clonedrbas  && git init &&
+ git pull --rebase --autostash "../parent" >out 2>err &&
+ test -s err &&
+ test_must_be_empty out)
+'
+
+test_expect_success 'git pull --rebase --no-autostash' '
+ mkdir clonedrbnas &&
+ (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 -v -q' '
  mkdir clonedvq &&
  (cd clonedvq && git init &&
--
2.7.1.340.g69eb491.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v3 3/3] Documentation/git-pull: document --[no-]autostash option

Mehul Jain
In reply to this post by Mehul Jain
Signed-off-by: Mehul Jain <[hidden email]>
---
 Documentation/git-pull.txt | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index a62a2a6..a593972 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -128,6 +128,21 @@ unless you have read linkgit:git-rebase[1] carefully.
 --no-rebase::
  Override earlier --rebase.
 
+--autostash::
+--no-autostash::
+ Automatically create a temporary stash before the operation
+ begins, and apply it after the operation ends. This means
+ that you can run rebase on a dirty worktree.
++
+This option is only valid when '--rebase' option is used.
++
+The default is --no-autostash, unless rebase.autoStash configuration
+is set.
++
+[NOTE]
+Use with care: the final stash application after a successful
+rebase might result in non-trivial conflicts.
+
 Options related to fetching
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
--
2.7.1.340.g69eb491.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 3/3] Documentation/git-pull: document --[no-]autostash option

Junio C Hamano
Mehul Jain <[hidden email]> writes:

> Signed-off-by: Mehul Jain <[hidden email]>
> ---
>  Documentation/git-pull.txt | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> index a62a2a6..a593972 100644
> --- a/Documentation/git-pull.txt
> +++ b/Documentation/git-pull.txt
> @@ -128,6 +128,21 @@ unless you have read linkgit:git-rebase[1] carefully.
>  --no-rebase::
>   Override earlier --rebase.
>  
> +--autostash::
> +--no-autostash::
> + Automatically create a temporary stash before the operation
> + begins, and apply it after the operation ends. This means
> + that you can run rebase on a dirty worktree.
> ++
> +This option is only valid when '--rebase' option is used.
> ++
> +The default is --no-autostash, unless rebase.autoStash configuration
> +is set.
> ++
> +[NOTE]
> +Use with care: the final stash application after a successful
> +rebase might result in non-trivial conflicts.
> +

Should this entry this verbose?

 - Is there a non-temporary stash?

 - I think "This means that ..." is totally unnecessary.

 - It probably makes sense to have "This option is only valid..." as
   a separate second paragraph as you did.

 - "The default is..." is misleading.  Even if rebase.autostash is
   set to false, we won't autostash, but that is different from the
   default being "--no-autostash".

   Think of "--[no-]autostash" option as *ONE* way to affect the
   auto-stashing behaviour, and treat "options" and "behaviours" two
   different things.

There is no default "option" for this.  It is that "autostash"
behaviour defaults to what is given to rebase.autostash if
exists, and can be explicitly set by --[no-]autostash if given.

But that is the norm for any configuration and option that overrides
the configuration, so it probably is a better use of the ink to say
something like this perhaps?

        --autostash::
        --no-autostash::
                Before starting "pull --rebase", create a stash to save
                local modifications, and apply the stash when done (this
                option is only valid when "--rebase" is used).
        +
        '--no-autostash' is useful to override the 'rebase.autoStash'
        configuration variable (see linkgit:git-config[1]).

By the way, some other patches in this series say --noautostash
without a dash after --no, which you would want to correct.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 1/3] pull --rebase: add --[no-]autostash flag

Matthieu Moy-2
In reply to this post by Mehul Jain
Mehul Jain <[hidden email]> writes:

> 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.

We normally wrap text with a bit less than 80 columns. Yours is wrappet
at 50 columns which makes it look weird.

> --- 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;

Instead of going through this 3-valued "true/false/unset", I would have
let opt_autostash = 0 by default, and read the configuration before the
call to parse_options (the usual way to apply precedence: read from low
precedence to high precedence).

But this is a bit less easy than it seems, since the code currently
checks the configuration variable only when --rebase is given, so my
version would do a useless call to git_config_get_bool() when --rebase
is not given. So I think your version is OK.

> + else {
> + /* If --[no-]autostash option is called without --rebase */
> + if (opt_autostash == 0)
> + die(_("--no-autostash option is only valid with --rebase."));
> + else if (opt_autostash == 1)

The else is not needed since the other branch dies.

--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 2/3] test: add test for --[no-]autostash flag

Matthieu Moy-2
In reply to this post by Mehul Jain
Mehul Jain <[hidden email]> writes:

> Signed-off-by: Mehul Jain <[hidden email]>
> ---
>  t/t5520-pull.sh         | 19 +++++++++++++++++++
>  t/t5521-pull-options.sh | 16 ++++++++++++++++

There's no need to split code/test/doc into separate patches, except if
your patches are getting really huge. As a reviewer, I like having tests
and doc in the same patch because they describe the intention of the
programmer.

We try to have a history where each commit is equally good, and with
your version there are two commits where --autostash exists and is
undocumented (which is not catastrophic, though).

> +test_expect_success 'pull --rebase --no-autostash fails with dirty working directory and rebase.autstash set 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
> +'
> +
> +test_expect_success 'pull --rebase --autostash succeeds with dirty working directory and rebase.autstash set 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"
> +'

Sounds good.

> --- a/t/t5521-pull-options.sh
> +++ b/t/t5521-pull-options.sh
> @@ -62,6 +62,22 @@ test_expect_success 'git pull -v --rebase' '
>   test_must_be_empty out)
>  '
>  
> +test_expect_success 'git pull --rebase --autostash' '
> + mkdir clonedrbas &&
> + (cd clonedrbas  && git init &&
> + git pull --rebase --autostash "../parent" >out 2>err &&
> + test -s err &&
> + test_must_be_empty out)
> +'
> +
> +test_expect_success 'git pull --rebase --no-autostash' '
> + mkdir clonedrbnas &&
> + (cd clonedrbnas  && git init &&
> + git pull --rebase --no-autostash "../parent" >out 2>err &&
> + test -s err &&
> + test_must_be_empty out)
> +'

Not sure these tests are needed if you have the ones in t/t5520-pull.sh.
More tests means more time to run so testing twice the same thing has a
cost.

--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 1/3] pull --rebase: add --[no-]autostash flag

Eric Sunshine
In reply to this post by Matthieu Moy-2
On Thu, Mar 3, 2016 at 12:24 PM, Matthieu Moy
<[hidden email]> wrote:
> Mehul Jain <[hidden email]> writes:
>> +     else {
>> +             /* If --[no-]autostash option is called without --rebase */
>> +             if (opt_autostash == 0)
>> +                     die(_("--no-autostash option is only valid with --rebase."));
>> +             else if (opt_autostash == 1)
>
> The else is not needed since the other branch dies.

A couple other minor comments (to be considered or ignored):

The comment "/* If --[no-]autostash ... */" merely repeats what the
code itself already says, thus is not really helpful and can be
dropped.

It would be reasonable to combine the two cases into one:

    if (opt_autostash != -1)
        die(_("--[no]-autostash option is only valid with --rebase."));
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 3/3] Documentation/git-pull: document --[no-]autostash option

Mehul Jain
In reply to this post by Junio C Hamano
On Thu, Mar 3, 2016 at 10:44 PM, Junio C Hamano <[hidden email]> wrote:

> Should this entry this verbose?
>
>  - Is there a non-temporary stash?
>
>  - I think "This means that ..." is totally unnecessary.
>
>  - It probably makes sense to have "This option is only valid..." as
>    a separate second paragraph as you did.
>
>  - "The default is..." is misleading.  Even if rebase.autostash is
>    set to false, we won't autostash, but that is different from the
>    default being "--no-autostash".
>
>    Think of "--[no-]autostash" option as *ONE* way to affect the
>    auto-stashing behaviour, and treat "options" and "behaviours" two
>    different things.
>
> There is no default "option" for this.  It is that "autostash"
> behaviour defaults to what is given to rebase.autostash if
> exists, and can be explicitly set by --[no-]autostash if given.
>
> But that is the norm for any configuration and option that overrides
> the configuration, so it probably is a better use of the ink to say
> something like this perhaps?
>
>         --autostash::
>         --no-autostash::
>                 Before starting "pull --rebase", create a stash to save
>                 local modifications, and apply the stash when done (this
>                 option is only valid when "--rebase" is used).

OK, but according to the definition of --[no-]autostash given in
git-rebase documentation (https://git-scm.com/docs/git-rebase),
temporary stash is created. So shouldn't we be consistent with this
definition by writing "... , create a temporary stash ..." instead of
"... , create a stash ...".

>         +
>         '--no-autostash' is useful to override the 'rebase.autoStash'
>         configuration variable (see linkgit:git-config[1]).

Thanks,
Mehul
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 1/3] pull --rebase: add --[no-]autostash flag

Mehul Jain
In reply to this post by Matthieu Moy-2
On Thu, Mar 3, 2016 at 10:54 PM, Matthieu Moy
<[hidden email]> wrote:

> Mehul Jain <[hidden email]> writes:
>
>> 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.
>
> We normally wrap text with a bit less than 80 columns. Yours is wrappet
> at 50 columns which makes it look weird.

OK. I will change it.

>> +     else {
>> +             /* If --[no-]autostash option is called without --rebase */
>> +             if (opt_autostash == 0)
>> +                     die(_("--no-autostash option is only valid with --rebase."));
>> +             else if (opt_autostash == 1)
>
> The else is not needed since the other branch dies.

I'm bit confused here. Which "else" you are talking about. I think both the
"else" and "else if" are needed here because:

- for the first "else", it is necessary that the case is only executed
when --rebase option is not given. If "else" is removed then in some case
where user calls "git pull --rebase --autostash" will lead to the execution of
"else if (opt_autostash == 1)"  case.

- Also removal of  "else if (opt_autostash == 1)" is not the right thing. As
the possibility of opt_autostash = -1 is there and this change may lead to
the execution of "die(_("--no-autostash ... "));" in case user calls "git pull".

Though I agree with Eric on combining the "if and else if" cases.

Thanks,
Mehul
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 2/3] test: add test for --[no-]autostash flag

Mehul Jain
In reply to this post by Matthieu Moy-2
On Thu, Mar 3, 2016 at 11:01 PM, Matthieu Moy
<[hidden email]> wrote:
> There's no need to split code/test/doc into separate patches, except if
> your patches are getting really huge. As a reviewer, I like having tests
> and doc in the same patch because they describe the intention of the
> programmer.
>
> We try to have a history where each commit is equally good, and with
> your version there are two commits where --autostash exists and is
> undocumented (which is not catastrophic, though).

Alright, I will make a single patch for the next version.

>> --- a/t/t5521-pull-options.sh
>> +++ b/t/t5521-pull-options.sh
>> @@ -62,6 +62,22 @@ test_expect_success 'git pull -v --rebase' '
>>       test_must_be_empty out)
>>  '
>>
>> +test_expect_success 'git pull --rebase --autostash' '
>> +     mkdir clonedrbas &&
>> +     (cd clonedrbas  && git init &&
>> +     git pull --rebase --autostash "../parent" >out 2>err &&
>> +     test -s err &&
>> +     test_must_be_empty out)
>> +'
>> +
>> +test_expect_success 'git pull --rebase --no-autostash' '
>> +     mkdir clonedrbnas &&
>> +     (cd clonedrbnas  && git init &&
>> +     git pull --rebase --no-autostash "../parent" >out 2>err &&
>> +     test -s err &&
>> +     test_must_be_empty out)
>> +'
>
> Not sure these tests are needed if you have the ones in t/t5520-pull.sh.
> More tests means more time to run so testing twice the same thing has a
> cost.

I agree with you. This test may not be needed as t/t5520-pull.sh already test
this option.

Thanks,
Mehul
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 1/3] pull --rebase: add --[no-]autostash flag

Matthieu Moy-2
In reply to this post by Eric Sunshine
Eric Sunshine <[hidden email]> writes:

> It would be reasonable to combine the two cases into one:
>
>     if (opt_autostash != -1)
>         die(_("--[no]-autostash option is only valid with --rebase."));

Nit (in case Mehul copy/paste this): that would be --[no-], not --[no]-.

--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 1/3] pull --rebase: add --[no-]autostash flag

Matthieu Moy-2
In reply to this post by Mehul Jain
Mehul Jain <[hidden email]> writes:

> On Thu, Mar 3, 2016 at 10:54 PM, Matthieu Moy
> <[hidden email]> wrote:
>> Mehul Jain <[hidden email]> writes:
>>> +     else {
>>> +             /* If --[no-]autostash option is called without --rebase */
>>> +             if (opt_autostash == 0)
>>> +                     die(_("--no-autostash option is only valid with --rebase."));
>>> +             else if (opt_autostash == 1)
>>
>> The else is not needed since the other branch dies.
>
> I'm bit confused here. Which "else" you are talking about.

The last "else" keyword. Not the "else" branch. It would just work like
this, and be a bit more pleasing to my eyes:

if (...)
        die(...);
if (...)
        die(...);

--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 3/3] Documentation/git-pull: document --[no-]autostash option

Matthieu Moy-2
In reply to this post by Mehul Jain
Mehul Jain <[hidden email]> writes:

> On Thu, Mar 3, 2016 at 10:44 PM, Junio C Hamano <[hidden email]> wrote:
>> Should this entry this verbose?
>>
>>  - Is there a non-temporary stash?
>>
>>  - I think "This means that ..." is totally unnecessary.
>>
>>  - It probably makes sense to have "This option is only valid..." as
>>    a separate second paragraph as you did.
>>
>>  - "The default is..." is misleading.  Even if rebase.autostash is
>>    set to false, we won't autostash, but that is different from the
>>    default being "--no-autostash".
>>
>>    Think of "--[no-]autostash" option as *ONE* way to affect the
>>    auto-stashing behaviour, and treat "options" and "behaviours" two
>>    different things.
>>
>> There is no default "option" for this.  It is that "autostash"
>> behaviour defaults to what is given to rebase.autostash if
>> exists, and can be explicitly set by --[no-]autostash if given.
>>
>> But that is the norm for any configuration and option that overrides
>> the configuration, so it probably is a better use of the ink to say
>> something like this perhaps?
>>
>>         --autostash::
>>         --no-autostash::
>>                 Before starting "pull --rebase", create a stash to save
>>                 local modifications, and apply the stash when done (this
>>                 option is only valid when "--rebase" is used).
>
> OK, but according to the definition of --[no-]autostash given in
> git-rebase documentation (https://git-scm.com/docs/git-rebase),
> temporary stash is created.

You shouldn't justify your change by copy-paste from another place. If
the other place is wrong, then replicating it is the worse thing to do
because this would mean we end up with two suboptimal pieces of
documentation instead of one (keep in mind: maintaining stuff is usually
harder than writing it in the first place).

I agree with Junio that "temporary stash" is pleonasm. OTOH, for someone
not familiar with "git stash", the explanation makes no sense anyway.
So, I'd drop the "temporary", and instead add a link to the doc of git
stash. Also, this is not really before starting "pull --rebase" but
after the user calls "pull" and before the actual rebase starst. I'd
write something like this:

        Before starting rebase, stash local modifications away (see
        linkgit:git-stash.txt[1]) if needed, and apply the stash when
        done

(I added "if needed" which makes sense technically since we don't create
an empty stash if not needed)

--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
123