Re: [PATCH] pull: allow dirty tree when rebase.autostash enabled
Hi,
Some comments which may not necessarily be correct.
On Wed, Jun 3, 2015 at 5:55 AM, Kevin Daudt <me@ikke.info> wrote:
Show 6 quoted lines
> 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.
> ---
Show 14 quoted lines
> 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.
Show 16 quoted lines
> + 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.
Show 6 quoted lines
> + 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)" = fileIt'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
Kevin Daudt
