Re: [PATCH] pull: allow dirty tree when rebase.autostash enabled

[Paul Tan <pyokagan@gmail.com>]

Hi,

Some comments which may not necessarily be correct.

On Wed, Jun 3, 2015 at 5:55 AM, Kevin Daudt <me@ikke.info> wrote:
> rebase learned to stash changes when it encounters a dirty work tree, but
> git pull --rebase does not.
>
> Only verify if the working tree is dirty when rebase.autostash is not
> enabled.
> ---

Missing sign-off.

>  git-pull.sh     |  5 ++++-
>  t/t5520-pull.sh | 17 +++++++++++++++++
>  2 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/git-pull.sh b/git-pull.sh
> index 0917d0d..6b9e8a3 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -239,7 +239,10 @@ test true = "$rebase" && {
>                         die "$(gettext "updating an unborn branch with changes added to the index")"
>                 fi
>         else
> -               require_clean_work_tree "pull with rebase" "Please commit or stash them."
> +               if [ $(git config --bool --get rebase.autostash || echo false) = "false" ]

"false" doesn't need to be quoted.

> +               then
> +                       require_clean_work_tree "pull with rebase" "Please commit or stash them."
> +               fi
>         fi
>         oldremoteref= &&
>         test -n "$curr_branch" &&
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 7efd45b..d849a19 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -297,6 +297,23 @@ test_expect_success 'pull --rebase dies early with dirty working directory' '
>
>  '
>
> +test_expect_success 'pull --rebase succeeds with dirty working directory and rebase.autostash set' '
> +

I know the surrounding old tests use a newline, but I think that all
new tests should use the modern style of not having a newline, since
t5520 already consists of a mix of old and modern styles anyway.

> +       test_when_finished "git rm -f file4" &&

There is trailing whitespace here.

Furthermore, git rm -f will fail if "file4" does not exist in the
index. Perhaps it should be moved below the "git add" below.

> +       git checkout to-rebase &&
> +       git update-ref refs/remotes/me/copy copy^ &&
> +       COPY=$(git rev-parse --verify me/copy) &&

$COPY is not used anywhere in the test.

> +       git rebase --onto $COPY copy &&
> +       test_config branch.to-rebase.remote me &&
> +       test_config branch.to-rebase.merge refs/heads/copy &&
> +       test_config branch.to-rebase.rebase true &&
> +       test_config rebase.autostash true &&
> +       echo dirty >> file4 &&

file4 does not exist, so we don't need to append to it. I know the
above few tests do not adhere to it, but CodingGuidelines says that
redirection operators do not have a space after

> +       git add file4 &&
> +       git pull

I think we should check for file contents to ensure that
git-pull/git-stash/git-rebase is doing its job properly.

> +

Same as above, no need the newline.

> +'
> +

With all that said, I wonder if this test, and the test above ("pull
--rebase dies early with dirty working directory") could be vastly
simplified, since we are not testing if we can handle a rebased
upstream.

E.g., my simplified version for the above test would be something like:

    git checkout -f to-rebase &&
    git rebase --onto copy^ copy &&
    test_config rebase.autostash true &&
    echo dirty >file4 &&
    git add file4 &&
    test_when_finished "git rm -f file4" &&
    git pull --rebase . me/copy &&
    test "$(cat file4)" = dirty &&
    test "$(cat file2)" = file

It's still confusing though, because we cannot take advantage of the
'before-rebase' tag introduced in the above tests. I would much prefer
if this test and the ("pull --rebase dies with dirty working
directory") test could be moved to the --rebase tests at lines 214+.
Also, this section in the t5520 test suite always gives me a headache
trying to decipher what it is trying to do ><

Thanks,
Paul