[PATCH] Add a testcase for the safety of pull-policy='pull'.

[PATCH] Add a testcase for the safety of pull-policy='pull'.

[Yann Dirson]

This testcase demonstrates a long-standing problem with the handling
of conflicts on a rewinding branch, when "stg pull" calls git-pull.

Signed-off-by: Yann Dirson <ydirson@altern.org>
---

This is precisely the problem that made me believe using "git-pull"
was a wrong idea to start with.  Since it seems there are uses for the
"git-pull" mode, this particular issue has to be addressed - I'm
however not sure how.

 t/t2101-pull-policy-pull.sh   |   60 ++++++++++++++++++++++++++++++++++
 t/t2102-pull-policy-rebase.sh |   72 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 132 insertions(+), 0 deletions(-)

diff --git a/t/t2101-pull-policy-pull.sh b/t/t2101-pull-policy-pull.sh
new file mode 100755
index 0000000..368d7d4
--- /dev/null
+++ b/t/t2101-pull-policy-pull.sh
@@ -0,0 +1,60 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 Yann Dirson
+#
+
+test_description='Excercise pull-policy "fetch-rebase".'
+
+. ./test-lib.sh
+
+# don't need this repo, but better not drop it, see t1100
+#rm -rf .git
+
+# Need a repo to clone
+test_create_repo upstream
+
+test_expect_success \
+    'Setup upstream repo, clone it, and add patches to the clone' \
+    '
+    (cd upstream && stg init) &&
+    stg clone upstream clone &&
+    (cd clone &&
+     git repo-config branch.master.stgit.pull-policy pull &&
+     git repo-config --list &&
+     stg new c1 -m c1 &&
+     echo a > file && stg add file && stg refresh
+    )
+    '
+
+test_expect_success \
+    'Add non-rewinding commit upstream and pull it from clone' \
+    '
+    (cd upstream && stg new u1 -m u1 &&
+     echo a > file2 && stg add file2 && stg refresh) &&
+    (cd clone && stg pull) &&
+     test -e clone/file2
+    '
+
+# note: with pre-1.5 Git the clone is not automatically recorded
+# as rewinding, and thus heads/origin is not moved, but the stack
+# is still correctly rebased
+
+test_expect_failure \
+    'Rewind/rewrite upstream commit and pull it from clone, without --merged' \
+    '
+    (cd upstream && echo b >> file2 && stg refresh) &&
+    (cd clone && stg pull)
+    '
+
+test_expect_success \
+    'Undo the conflicted pull' \
+    '(cd clone && stg push --undo)'
+
+test_expect_success \
+    'Pull the rewinded commit, with --merged' \
+    '
+    (cd clone && stg pull --merged) &&
+    test `wc -l <clone/file2` = 2
+    '
+
+test_done
diff --git a/t/t2102-pull-policy-rebase.sh b/t/t2102-pull-policy-rebase.sh
new file mode 100755
index 0000000..e1398a3
--- /dev/null
+++ b/t/t2102-pull-policy-rebase.sh
@@ -0,0 +1,72 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 Yann Dirson
+#
+
+test_description='Excercise pull-policy "fetch-rebase".'
+
+. ./test-lib.sh
+
+# don't need this repo, but better not drop it, see t1100
+#rm -rf .git
+
+# Need a repo to clone
+test_create_repo upstream
+
+test_expect_success \
+    'Setup upstream repo, clone it, and add patches to the clone' \
+    '
+    (cd upstream && stg init) &&
+    stg clone upstream clone &&
+    (cd clone &&
+     git repo-config branch.master.stgit.pull-policy fetch-rebase &&
+     git repo-config --list &&
+     stg new c1 -m c1 &&
+     echo a > file && stg add file && stg refresh
+    )
+    '
+
+test_expect_success \
+    'Add non-rewinding commit upstream and pull it from clone' \
+    '
+    (cd upstream && stg new u1 -m u1 &&
+     echo a > file2 && stg add file2 && stg refresh) &&
+    (cd clone && stg pull) &&
+    test -e clone/file2
+    '
+
+# note: with pre-1.5 Git the clone is not automatically recorded
+# as rewinding, and thus heads/origin is not moved, but the stack
+# is still correctly rebased
+test_expect_success \
+    'Rewind/rewrite upstream commit and pull it from clone' \
+    '
+    (cd upstream && echo b >> file2 && stg refresh) &&
+    (cd clone && stg pull) &&
+    test `wc -l <clone/file2` = 2
+    '
+
+# this one ensures the guard against commits does not unduly trigger
+test_expect_success \
+    'Rewind/rewrite upstream commit and fetch it from clone before pulling' \
+    '
+    (cd upstream && echo c >> file2 && stg refresh) &&
+    (cd clone && git fetch && stg pull) &&
+    test `wc -l <clone/file2` = 3
+    '
+
+# this one exercises the guard against commits
+# (use a new file to avoid mistaking a conflict for a success)
+test_expect_success \
+    'New upstream commit and commit a patch in clone' \
+    '
+    (cd upstream && stg new u2 -m u2 &&
+     echo a > file3 && stg add file3 && stg refresh) &&
+    (cd clone && stg commit && stg new c2 -m c2 &&
+     echo a >> file && stg refresh)
+    '
+test_expect_failure \
+    'Try to  and commit a patch in clone' \
+    '(cd clone && stg pull)'
+
+test_done

Re: [PATCH] Add a testcase for the safety of pull-policy='pull'.

[Catalin Marinas]

On 25/02/07, Yann Dirson <ydirson@altern.org> wrote:
> This testcase demonstrates a long-standing problem with the handling
> of conflicts on a rewinding branch, when "stg pull" calls git-pull.
[...]
> diff --git a/t/t2101-pull-policy-pull.sh b/t/t2101-pull-policy-pull.sh
[...]
> +test_expect_failure \
> +    'Rewind/rewrite upstream commit and pull it from clone, without --merged' \
> +    '
> +    (cd upstream && echo b >> file2 && stg refresh) &&
> +    (cd clone && stg pull)
> +    '

This fails (with git 1.5), as expected, but probably not for the same
reason. See below.

> +test_expect_success \
> +    'Undo the conflicted pull' \
> +    '(cd clone && stg push --undo)'

This actually fails in my tests because the git-pull failed previously
(and not the patch pushing) and there is no patch on the stack to
undo. BTW, push --undo now requires a status --reset beforehand.

I can merge the patch as it is and you can send me another one for this issue.

Thanks.

-- 
Catalin

Re: [PATCH] Add a testcase for the safety of pull-policy='pull'.

[Yann Dirson]

On Tue, Feb 27, 2007 at 02:25:57PM +0000, Catalin Marinas wrote:
> On 25/02/07, Yann Dirson <ydirson@altern.org> wrote:
> >This testcase demonstrates a long-standing problem with the handling
> >of conflicts on a rewinding branch, when "stg pull" calls git-pull.
> [...]
> >diff --git a/t/t2101-pull-policy-pull.sh b/t/t2101-pull-policy-pull.sh
> [...]
> >+test_expect_failure \
> >+    'Rewind/rewrite upstream commit and pull it from clone, without 
> >--merged' \
> >+    '
> >+    (cd upstream && echo b >> file2 && stg refresh) &&
> >+    (cd clone && stg pull)
> >+    '
> 
> This fails (with git 1.5), as expected, but probably not for the same
> reason. See below.

That would demonstrate the usefulness of a python-based testsuite as I
suggested the other day :)

But I see the test failing with both 1.4.4.4 and 1.5.0.1:

Auto-merged file2
CONFLICT (add/add): Merge conflict in file2
Automatic merge failed; fix conflicts and then commit the result.
stg pull: Failed "git-pull origin"
Traceback (most recent call last):
  File "/export/work/yann/git/stgit/t/../stg", line 43, in ?
    main()
  File "/export/work/yann/git/stgit/stgit/main.py", line 280, in main
    command.func(parser, options, args)
  File "/export/work/yann/git/stgit/stgit/commands/pull.py", line 84, in func
    git.pull(repository)
  File "/export/work/yann/git/stgit/stgit/git.py", line 839, in pull
    raise GitException, 'Failed "%s %s"' % (command, repository)
stgit.git.GitException: Failed "git-pull origin"


> >+test_expect_success \
> >+    'Undo the conflicted pull' \
> >+    '(cd clone && stg push --undo)'
> 
> This actually fails in my tests because the git-pull failed previously
> (and not the patch pushing) and there is no patch on the stack to
> undo.

Right, I was a bit confused.  I first checked whether there was a
"pull --undo" and did not catch correctly the FlagNotFoundException :)


> BTW, push --undo now requires a status --reset beforehand.

Oh, I had not noticed that.  Why so ?  It's not like if "push --undo"
would lose any valuable data...


What strikes me most in this case is the difference in behaviour
between this type of conflict (conflict marked in index, so needs an
operation outside the current scope of stgit to proceed), with a
regular stgit conflict that occurs during a push (index clean,
conflict info not available to tools written for regular GIT).

That reminds me I already wondered why stgit has to deal with conflict
handling itself.  I've not digged much into the GIT merge mechanism,
but I'd think it would be great if we were able to use it.


> I can merge the patch as it is and you can send me another one for this 
> issue.

I have prepared the followin patchlet (you may want to food it into my
previous patch), but I am still a bit unsure what to do - read on.

====================
--- a/t/t2101-pull-policy-pull.sh
+++ b/t/t2101-pull-policy-pull.sh
@@ -47,14 +47,12 @@ test_expect_failure \
     '
 
 test_expect_success \
-    'Undo the conflicted pull' \
-    '(cd clone && stg push --undo)'
+    '"Solve" the conflict (pretend there is none)' \
+    '(cd clone &&
+      git add file2 && EDITOR=cat git commit)'
 
 test_expect_success \
-    'Pull the rewinded commit, with --merged' \
-    '
-    (cd clone && stg pull --merged) &&
-    test `wc -l <clone/file2` = 2
-    '
+    'Push the stack back' \
+    '(cd clone && stg push -a)'
 
 test_done
====================


The testcase now still ends in a bad situation.

What happenned is our "stg pull" could not complete, so we had to
complete it by hand (instead of attempting to abort it, as my initial
testcase wanted to do).  This makes it look like an "unsafe base
change" (since post_rebase was never called to update "orig-base",
after our manual conflict resolution was done), so any subsequent pull
or rebase will require a --force.

I don't like that either.  I'm still quite unsure how the "git-pull"
model of pulling ought to work, that obviously does not help :)
Any idea ?

Best regards,
-- 
Yann.

Re: [PATCH] Add a testcase for the safety of pull-policy='pull'.

[Catalin Marinas]

On 27/02/07, Yann Dirson <ydirson@altern.org> wrote:
> On Tue, Feb 27, 2007 at 02:25:57PM +0000, Catalin Marinas wrote:
> > This fails (with git 1.5), as expected, but probably not for the same
> > reason. See below.
>
> That would demonstrate the usefulness of a python-based testsuite as I
> suggested the other day :)

Yes, it would be useful, but probably in addition to the functional
testing we currently do (which might be simplified).

> > BTW, push --undo now requires a status --reset beforehand.
>
> Oh, I had not noticed that.  Why so ?  It's not like if "push --undo"
> would lose any valuable data...

I added it so that one cannot remove the local changes by doing a
"push --undo" in error. You would have to explicitly ask for local
changes removing with status --reset.

> What strikes me most in this case is the difference in behaviour
> between this type of conflict (conflict marked in index, so needs an
> operation outside the current scope of stgit to proceed), with a
> regular stgit conflict that occurs during a push (index clean,
> conflict info not available to tools written for regular GIT).

I think this is a valid GIT conflict as the upstream tree re-wrote the
history and git-pull will trigger a 3-way merge. If you would always
use git-fetch + stgit-rebase, there wouldn't be any problem.

> That reminds me I already wondered why stgit has to deal with conflict
> handling itself.  I've not digged much into the GIT merge mechanism,
> but I'd think it would be great if we were able to use it.

That's probably for historical reasons. A year or so ago, porcelain
tools had to handle the merging themselves (if they needed more than
what git-read-tree -m). The only merge algorithm I could easily
understand was the one in Cogito, so I implemented it in StGIT.

Things has changed since then and now GIT can handle the merging
pretty well. StGIT even uses it transparently via git-merge-recursive
called during the 'push' operation ('sync' still uses git-read-tree
because of the better performance). The gitmergeonefile.py still has
all the conflict cases but most of them are handled by
git-merge-recursive and never get here. If this fail, StGIT tries to
use a configured 3-way merge (diff3 by default) and maybe the
interactive merge if the this fails (BTW, I just added a patch for a
2-way interactive merge as well).

An advantage of using the diff3 over the git's 3-way merge is the
configurable labels attached to the in-file conflict markers but I
don't use this feature much anyway as they get overridden by the
interactive merge tool.

We can drop most of this at some point but it is currently still
needed for the 'sync' command using git-read-tree -m. StGIT also
leaves a clean index (i.e. only one-stage files) and marks the
conflict in .git/conflicts. Maybe post 1.0 we can leave the unmerged
entries in the index and re-work this part.

> What happenned is our "stg pull" could not complete, so we had to
> complete it by hand (instead of attempting to abort it, as my initial
> testcase wanted to do).  This makes it look like an "unsafe base
> change" (since post_rebase was never called to update "orig-base",
> after our manual conflict resolution was done), so any subsequent pull
> or rebase will require a --force.
>
> I don't like that either.  I'm still quite unsure how the "git-pull"
> model of pulling ought to work, that obviously does not help :)
> Any idea ?

I think StGIT behaves correctly since, as I said above, you are
pulling from a tree that has a re-written history. People using GIT
should be able to fix this themselves. For people using StGIT-only, we
should default to the fetch+rebase strategy.

-- 
Catalin

Re: [PATCH] Add a testcase for the safety of pull-policy='pull'.

[Yann Dirson]

On Tue, Feb 27, 2007 at 11:38:45PM +0000, Catalin Marinas wrote:
> >> BTW, push --undo now requires a status --reset beforehand.
> >
> >Oh, I had not noticed that.  Why so ?  It's not like if "push --undo"
> >would lose any valuable data...
> 
> I added it so that one cannot remove the local changes by doing a
> "push --undo" in error. You would have to explicitly ask for local
> changes removing with status --reset.

At least, the message in that case should probably be made better,
when undoing to avoid having to resolve a conflict: the message says I
cannot undo because I have a conflict, whereas it is the exact reason
why I want to undo.
Especially, since the conflict markers are now auto-recorded, we could
simply allow undo in that case.

> >What strikes me most in this case is the difference in behaviour
> >between this type of conflict (conflict marked in index, so needs an
> >operation outside the current scope of stgit to proceed), with a
> >regular stgit conflict that occurs during a push (index clean,
> >conflict info not available to tools written for regular GIT).
> 
> I think this is a valid GIT conflict as the upstream tree re-wrote the
> history and git-pull will trigger a 3-way merge.

I do not discuss the validity of the conflict.  I'm just emphasizing
that it makes it hard to handle things in stgit, with *any*
post-rebase processing being skipped.  For now we seem to have nothing
critical for that workflow (I'll surely end up with deactivating the
orig-base check for pull-policy=pull), but that may cause more trouble
later, eg. when implementing transactions.


> If you would always use git-fetch + stgit-rebase, there wouldn't be
> any problem.

Sure, that's why I'm lobbying for this policy to be the default :)


> (BTW, I just added a patch for a 2-way interactive merge as well).

Sounds great - I just had an add/add conflict today on 0.12.1 when
trying to play with "resolved -i", and it loudly complained it had no
ancestor to play with.  Sounds like a perfect usage for 2-way merges.

Best regards,
-- 
Yann.

Re: [PATCH] Add a testcase for the safety of pull-policy='pull'.

[Yann Dirson]

On Wed, Feb 28, 2007 at 10:48:51PM +0100, Yann Dirson wrote:
> On Tue, Feb 27, 2007 at 11:38:45PM +0000, Catalin Marinas wrote:
> > >> BTW, push --undo now requires a status --reset beforehand.
> > >
> > >Oh, I had not noticed that.  Why so ?  It's not like if "push --undo"
> > >would lose any valuable data...
> > 
> > I added it so that one cannot remove the local changes by doing a
> > "push --undo" in error. You would have to explicitly ask for local
> > changes removing with status --reset.
> 
> At least, the message in that case should probably be made better,
> when undoing to avoid having to resolve a conflict: the message says I
> cannot undo because I have a conflict, whereas it is the exact reason
> why I want to undo.
> Especially, since the conflict markers are now auto-recorded, we could
> simply allow undo in that case.

Actually, I find the behaviour to be much worse than before: forcing
the user to run "status --reset" before "push --undo" indeed brings
the patch that conflicted in a state where it is partly committed.
That is, if the user is interrupted in her work after resetting, and
later comes back, it is not unlikely that she forget that an operation
has not been finished, and eg. run "stg pop", in which case the patch
would be left in an unwanted state.  Eg, I just caused the problem
when pushing those patches of mine you integrated: the "editor" patch,
which is later modified by one of yours, caused a conflict on puch,
which causes an empty patch to be recorded (no idea why there was no
conflict markers recorded, BTW).

Indeed, the problem may well be that we should not commit the
unresolved conflict.  Why it has some value, recording it as if the
user had refreshed the patch is probably not a good idea at all,
precisely because we're mid-way in the push, and that the operation
can be aborted without stgit knowing.

Maybe with transactions we could manage to make something sensible
here, but even then, I'm not sure if we would be able to detect that
the user's actions should abort the push transaction and rollback to
the previous state.  Sounds really too much DWIM in the end.

Maybe instead we should write such a commit, but not as if it was a
refresh.  Maybe under some .../patches/name.conflict ref, leaving the
patch itself untouched, but causing the stack to be blocked until the
push gets undone (in which case things look simple), or the patch gets
resolved+refreshed (in which case, both the conflict and its
resolution can appear in the patchlog, but things should be made so
"push --undo" would rollback the 2 ones as a whole).

Does that sound sensible ?

Best regards,
-- 
Yann.

Re: [PATCH] Add a testcase for the safety of pull-policy='pull'.

[Catalin Marinas]

On 01/03/07, Yann Dirson <ydirson@altern.org> wrote:
> On Wed, Feb 28, 2007 at 10:48:51PM +0100, Yann Dirson wrote:
> > On Tue, Feb 27, 2007 at 11:38:45PM +0000, Catalin Marinas wrote:
> > > I added it so that one cannot remove the local changes by doing a
> > > "push --undo" in error. You would have to explicitly ask for local
> > > changes removing with status --reset.
> >
> > At least, the message in that case should probably be made better,
> > when undoing to avoid having to resolve a conflict: the message says I
> > cannot undo because I have a conflict, whereas it is the exact reason
> > why I want to undo.

That's because of the order of the safety checks.

> > Especially, since the conflict markers are now auto-recorded, we could
> > simply allow undo in that case.
>
> Actually, I find the behaviour to be much worse than before: forcing
> the user to run "status --reset" before "push --undo" indeed brings
> the patch that conflicted in a state where it is partly committed.

I don't have a strong opinion on forcing status --reset before push
--undo anyway, so I can revert that commit (it bothers me a bit as
well :-)).

> Indeed, the problem may well be that we should not commit the
> unresolved conflict.  Why it has some value, recording it as if the
> user had refreshed the patch is probably not a good idea at all,
> precisely because we're mid-way in the push, and that the operation
> can be aborted without stgit knowing.

The idea of commit 0f4eba6a37c1a5454560b097873e5a22bfcde908 was to
only show the conflict files with 'stg diff' and also allow me to
later see how I fixed a conflict via the 'stg log (-p|-g)' command.
But this interim refresh shouldn't affect the backup information
(backup = False in Series.refresh_patch). Do you have a simple test
showing this problem?

Regards.

-- 
Catalin