Re: [PATCH] pull: allow dirty tree when rebase.autostash enabled
- From
Paul Tan <pyokagan@gmail.com>
- Date
- Jun 3, 2015, 04:50 UTC
- Message-ID
- <CACRoPnQ=5sdWpnebv0OKPHPBh4iyMZ1H47cix6KFh7i5fYomNA@mail.gmail.com>
- In-Reply-To
- <1433282157-8171-1-git-send-email-me@ikke.info>
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. > ---
Missing sign-off.
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