{"thread":{"id":"6955","subject":"[PATCH] Add a testcase for the safety of pull-policy='pull'.","startedAt":"2007-02-25T22:11:33Z","lastAt":"2007-03-06T22:37:34Z","messageCount":7,"participants":["Yann Dirson","Catalin Marinas"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"35460","messageId":"20070225220853.31361.7201.stgit@gandelf.nowhere.earth","threadId":"6955","inReplyTo":null,"subject":"[PATCH] Add a testcase for the safety of pull-policy='pull'.","fromName":"Yann Dirson","fromEmail":"ydirson@altern.org","sentAt":"2007-02-25T22:11:33Z","receivedAt":"2007-02-25T22:11:33Z","isPatch":true,"sender":{"key":"ydirson@altern.org","avatar":"https://avatars.githubusercontent.com/u/1190950?v=4"},"body":"\nThis testcase demonstrates a long-standing problem with the handling\nof conflicts on a rewinding branch, when \"stg pull\" calls git-pull.\n\nSigned-off-by: Yann Dirson \n---\n\nThis is precisely the problem that made me believe using \"git-pull\"\nwas a wrong idea to start with. Since it seems there are uses for the\n\"git-pull\" mode, this particular issue has to be addressed - I'm\nhowever not sure how.\n\n t/t2101-pull-policy-pull.sh | 60 ++++++++++++++++++++++++++++++++++\n t/t2102-pull-policy-rebase.sh | 72 +++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 132 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t2101-pull-policy-pull.sh b/t/t2101-pull-policy-pull.sh\nnew file mode 100755\nindex 0000000..368d7d4\n--- /dev/null\n+++ b/t/t2101-pull-policy-pull.sh\n@@ -0,0 +1,60 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2007 Yann Dirson\n+#\n+\n+test_description='Excercise pull-policy \"fetch-rebase\".'\n+\n+. ./test-lib.sh\n+\n+# don't need this repo, but better not drop it, see t1100\n+#rm -rf .git\n+\n+# Need a repo to clone\n+test_create_repo upstream\n+\n+test_expect_success \\\n+ 'Setup upstream repo, clone it, and add patches to the clone' \\\n+ '\n+ (cd upstream && stg init) &&\n+ stg clone upstream clone &&\n+ (cd clone &&\n+ git repo-config branch.master.stgit.pull-policy pull &&\n+ git repo-config --list &&\n+ stg new c1 -m c1 &&\n+ echo a > file && stg add file && stg refresh\n+ )\n+ '\n+\n+test_expect_success \\\n+ 'Add non-rewinding commit upstream and pull it from clone' \\\n+ '\n+ (cd upstream && stg new u1 -m u1 &&\n+ echo a > file2 && stg add file2 && stg refresh) &&\n+ (cd clone && stg pull) &&\n+ test -e clone/file2\n+ '\n+\n+# note: with pre-1.5 Git the clone is not automatically recorded\n+# as rewinding, and thus heads/origin is not moved, but the stack\n+# is still correctly rebased\n+\n+test_expect_failure \\\n+ 'Rewind/rewrite upstream commit and pull it from clone, without --merged' \\\n+ '\n+ (cd upstream && echo b >> file2 && stg refresh) &&\n+ (cd clone && stg pull)\n+ '\n+\n+test_expect_success \\\n+ 'Undo the conflicted pull' \\\n+ '(cd clone && stg push --undo)'\n+\n+test_expect_success \\\n+ 'Pull the rewinded commit, with --merged' \\\n+ '\n+ (cd clone && stg pull --merged) &&\n+ test `wc -l file && stg add file && stg refresh\n+ )\n+ '\n+\n+test_expect_success \\\n+ 'Add non-rewinding commit upstream and pull it from clone' \\\n+ '\n+ (cd upstream && stg new u1 -m u1 &&\n+ echo a > file2 && stg add file2 && stg refresh) &&\n+ (cd clone && stg pull) &&\n+ test -e clone/file2\n+ '\n+\n+# note: with pre-1.5 Git the clone is not automatically recorded\n+# as rewinding, and thus heads/origin is not moved, but the stack\n+# is still correctly rebased\n+test_expect_success \\\n+ 'Rewind/rewrite upstream commit and pull it from clone' \\\n+ '\n+ (cd upstream && echo b >> file2 && stg refresh) &&\n+ (cd clone && stg pull) &&\n+ test `wc -l > file2 && stg refresh) &&\n+ (cd clone && git fetch && stg pull) &&\n+ test `wc -l file3 && stg add file3 && stg refresh) &&\n+ (cd clone && stg commit && stg new c2 -m c2 &&\n+ echo a >> file && stg refresh)\n+ '\n+test_expect_failure \\\n+ 'Try to and commit a patch in clone' \\\n+ '(cd clone && stg pull)'\n+\n+test_done\n"},{"id":"35618","messageId":"b0943d9e0702270625o5a9ef8b4xd0fa5df68e2b805a@mail.gmail.com","threadId":"6955","inReplyTo":"20070225220853.31361.7201.stgit@gandelf.nowhere.earth","subject":"Re: [PATCH] Add a testcase for the safety of pull-policy='pull'.","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2007-02-27T14:25:57Z","receivedAt":"2007-02-27T14:25:57Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"On 25/02/07, Yann Dirson wrote:\n> This testcase demonstrates a long-standing problem with the handling\n> of conflicts on a rewinding branch, when \"stg pull\" calls git-pull.\n[...]\n> diff --git a/t/t2101-pull-policy-pull.sh b/t/t2101-pull-policy-pull.sh\n[...]\n> +test_expect_failure \\\n> + 'Rewind/rewrite upstream commit and pull it from clone, without --merged' \\\n> + '\n> + (cd upstream && echo b >> file2 && stg refresh) &&\n> + (cd clone && stg pull)\n> + '\n\nThis fails (with git 1.5), as expected, but probably not for the same\nreason. See below.\n\n> +test_expect_success \\\n> + 'Undo the conflicted pull' \\\n> + '(cd clone && stg push --undo)'\n\nThis actually fails in my tests because the git-pull failed previously\n(and not the patch pushing) and there is no patch on the stack to\nundo. BTW, push --undo now requires a status --reset beforehand.\n\nI can merge the patch as it is and you can send me another one for this issue.\n\nThanks.\n\n-- \nCatalin\n"},{"id":"35686","messageId":"20070227210906.GH4045@nan92-1-81-57-214-146.fbx.proxad.net","threadId":"6955","inReplyTo":"b0943d9e0702270625o5a9ef8b4xd0fa5df68e2b805a@mail.gmail.com","subject":"Re: [PATCH] Add a testcase for the safety of pull-policy='pull'.","fromName":"Yann Dirson","fromEmail":"ydirson@altern.org","sentAt":"2007-02-27T21:09:06Z","receivedAt":"2007-02-27T21:09:06Z","isPatch":true,"sender":{"key":"ydirson@altern.org","avatar":"https://avatars.githubusercontent.com/u/1190950?v=4"},"body":"On Tue, Feb 27, 2007 at 02:25:57PM +0000, Catalin Marinas wrote:\n> On 25/02/07, Yann Dirson wrote:\n> >This testcase demonstrates a long-standing problem with the handling\n> >of conflicts on a rewinding branch, when \"stg pull\" calls git-pull.\n> [...]\n> >diff --git a/t/t2101-pull-policy-pull.sh b/t/t2101-pull-policy-pull.sh\n> [...]\n> >+test_expect_failure \\\n> >+ 'Rewind/rewrite upstream commit and pull it from clone, without \n> >--merged' \\\n> >+ '\n> >+ (cd upstream && echo b >> file2 && stg refresh) &&\n> >+ (cd clone && stg pull)\n> >+ '\n> \n> This fails (with git 1.5), as expected, but probably not for the same\n> reason. See below.\n\nThat would demonstrate the usefulness of a python-based testsuite as I\nsuggested the other day :)\n\nBut I see the test failing with both 1.4.4.4 and 1.5.0.1:\n\nAuto-merged file2\nCONFLICT (add/add): Merge conflict in file2\nAutomatic merge failed; fix conflicts and then commit the result.\nstg pull: Failed \"git-pull origin\"\nTraceback (most recent call last):\n File \"/export/work/yann/git/stgit/t/../stg\", line 43, in ?\n main()\n File \"/export/work/yann/git/stgit/stgit/main.py\", line 280, in main\n command.func(parser, options, args)\n File \"/export/work/yann/git/stgit/stgit/commands/pull.py\", line 84, in func\n git.pull(repository)\n File \"/export/work/yann/git/stgit/stgit/git.py\", line 839, in pull\n raise GitException, 'Failed \"%s %s\"' % (command, repository)\nstgit.git.GitException: Failed \"git-pull origin\"\n\n\n> >+test_expect_success \\\n> >+ 'Undo the conflicted pull' \\\n> >+ '(cd clone && stg push --undo)'\n> \n> This actually fails in my tests because the git-pull failed previously\n> (and not the patch pushing) and there is no patch on the stack to\n> undo.\n\nRight, I was a bit confused. I first checked whether there was a\n\"pull --undo\" and did not catch correctly the FlagNotFoundException :)\n\n\n> BTW, push --undo now requires a status --reset beforehand.\n\nOh, I had not noticed that. Why so ? It's not like if \"push --undo\"\nwould lose any valuable data...\n\n\nWhat strikes me most in this case is the difference in behaviour\nbetween this type of conflict (conflict marked in index, so needs an\noperation outside the current scope of stgit to proceed), with a\nregular stgit conflict that occurs during a push (index clean,\nconflict info not available to tools written for regular GIT).\n\nThat reminds me I already wondered why stgit has to deal with conflict\nhandling itself. I've not digged much into the GIT merge mechanism,\nbut I'd think it would be great if we were able to use it.\n\n\n> I can merge the patch as it is and you can send me another one for this \n> issue.\n\nI have prepared the followin patchlet (you may want to food it into my\nprevious patch), but I am still a bit unsure what to do - read on.\n\n====================\n--- a/t/t2101-pull-policy-pull.sh\n+++ b/t/t2101-pull-policy-pull.sh\n@@ -47,14 +47,12 @@ test_expect_failure \\\n '\n \n test_expect_success \\\n- 'Undo the conflicted pull' \\\n- '(cd clone && stg push --undo)'\n+ '\"Solve\" the conflict (pretend there is none)' \\\n+ '(cd clone &&\n+ git add file2 && EDITOR=cat git commit)'\n \n test_expect_success \\\n- 'Pull the rewinded commit, with --merged' \\\n- '\n- (cd clone && stg pull --merged) &&\n- test `wc -l wrote:\n> On Tue, Feb 27, 2007 at 02:25:57PM +0000, Catalin Marinas wrote:\n> > This fails (with git 1.5), as expected, but probably not for the same\n> > reason. See below.\n>\n> That would demonstrate the usefulness of a python-based testsuite as I\n> suggested the other day :)\n\nYes, it would be useful, but probably in addition to the functional\ntesting we currently do (which might be simplified).\n\n> > BTW, push --undo now requires a status --reset beforehand.\n>\n> Oh, I had not noticed that. Why so ? It's not like if \"push --undo\"\n> would lose any valuable data...\n\nI added it so that one cannot remove the local changes by doing a\n\"push --undo\" in error. You would have to explicitly ask for local\nchanges removing with status --reset.\n\n> What strikes me most in this case is the difference in behaviour\n> between this type of conflict (conflict marked in index, so needs an\n> operation outside the current scope of stgit to proceed), with a\n> regular stgit conflict that occurs during a push (index clean,\n> conflict info not available to tools written for regular GIT).\n\nI think this is a valid GIT conflict as the upstream tree re-wrote the\nhistory and git-pull will trigger a 3-way merge. If you would always\nuse git-fetch + stgit-rebase, there wouldn't be any problem.\n\n> That reminds me I already wondered why stgit has to deal with conflict\n> handling itself. I've not digged much into the GIT merge mechanism,\n> but I'd think it would be great if we were able to use it.\n\nThat's probably for historical reasons. A year or so ago, porcelain\ntools had to handle the merging themselves (if they needed more than\nwhat git-read-tree -m). The only merge algorithm I could easily\nunderstand was the one in Cogito, so I implemented it in StGIT.\n\nThings has changed since then and now GIT can handle the merging\npretty well. StGIT even uses it transparently via git-merge-recursive\ncalled during the 'push' operation ('sync' still uses git-read-tree\nbecause of the better performance). The gitmergeonefile.py still has\nall the conflict cases but most of them are handled by\ngit-merge-recursive and never get here. If this fail, StGIT tries to\nuse a configured 3-way merge (diff3 by default) and maybe the\ninteractive merge if the this fails (BTW, I just added a patch for a\n2-way interactive merge as well).\n\nAn advantage of using the diff3 over the git's 3-way merge is the\nconfigurable labels attached to the in-file conflict markers but I\ndon't use this feature much anyway as they get overridden by the\ninteractive merge tool.\n\nWe can drop most of this at some point but it is currently still\nneeded for the 'sync' command using git-read-tree -m. StGIT also\nleaves a clean index (i.e. only one-stage files) and marks the\nconflict in .git/conflicts. Maybe post 1.0 we can leave the unmerged\nentries in the index and re-work this part.\n\n> What happenned is our \"stg pull\" could not complete, so we had to\n> complete it by hand (instead of attempting to abort it, as my initial\n> testcase wanted to do). This makes it look like an \"unsafe base\n> change\" (since post_rebase was never called to update \"orig-base\",\n> after our manual conflict resolution was done), so any subsequent pull\n> or rebase will require a --force.\n>\n> I don't like that either. I'm still quite unsure how the \"git-pull\"\n> model of pulling ought to work, that obviously does not help :)\n> Any idea ?\n\nI think StGIT behaves correctly since, as I said above, you are\npulling from a tree that has a re-written history. People using GIT\nshould be able to fix this themselves. For people using StGIT-only, we\nshould default to the fetch+rebase strategy.\n\n-- \nCatalin\n"},{"id":"35890","messageId":"20070228214851.GC4149@nan92-1-81-57-214-146.fbx.proxad.net","threadId":"6955","inReplyTo":"b0943d9e0702271538q32e6a9d5y6ac73adfc4854ab6@mail.gmail.com","subject":"Re: [PATCH] Add a testcase for the safety of pull-policy='pull'.","fromName":"Yann Dirson","fromEmail":"ydirson@altern.org","sentAt":"2007-02-28T21:48:51Z","receivedAt":"2007-02-28T21:48:51Z","isPatch":true,"sender":{"key":"ydirson@altern.org","avatar":"https://avatars.githubusercontent.com/u/1190950?v=4"},"body":"On Tue, Feb 27, 2007 at 11:38:45PM +0000, Catalin Marinas wrote:\n> >> BTW, push --undo now requires a status --reset beforehand.\n> >\n> >Oh, I had not noticed that. Why so ? It's not like if \"push --undo\"\n> >would lose any valuable data...\n> \n> I added it so that one cannot remove the local changes by doing a\n> \"push --undo\" in error. You would have to explicitly ask for local\n> changes removing with status --reset.\n\nAt least, the message in that case should probably be made better,\nwhen undoing to avoid having to resolve a conflict: the message says I\ncannot undo because I have a conflict, whereas it is the exact reason\nwhy I want to undo.\nEspecially, since the conflict markers are now auto-recorded, we could\nsimply allow undo in that case.\n\n> >What strikes me most in this case is the difference in behaviour\n> >between this type of conflict (conflict marked in index, so needs an\n> >operation outside the current scope of stgit to proceed), with a\n> >regular stgit conflict that occurs during a push (index clean,\n> >conflict info not available to tools written for regular GIT).\n> \n> I think this is a valid GIT conflict as the upstream tree re-wrote the\n> history and git-pull will trigger a 3-way merge.\n\nI do not discuss the validity of the conflict. I'm just emphasizing\nthat it makes it hard to handle things in stgit, with *any*\npost-rebase processing being skipped. For now we seem to have nothing\ncritical for that workflow (I'll surely end up with deactivating the\norig-base check for pull-policy=pull), but that may cause more trouble\nlater, eg. when implementing transactions.\n\n\n> If you would always use git-fetch + stgit-rebase, there wouldn't be\n> any problem.\n\nSure, that's why I'm lobbying for this policy to be the default :)\n\n\n> (BTW, I just added a patch for a 2-way interactive merge as well).\n\nSounds great - I just had an add/add conflict today on 0.12.1 when\ntrying to play with \"resolved -i\", and it loudly complained it had no\nancestor to play with. Sounds like a perfect usage for 2-way merges.\n\nBest regards,\n-- \nYann.\n"},{"id":"36003","messageId":"20070301201047.GD4149@nan92-1-81-57-214-146.fbx.proxad.net","threadId":"6955","inReplyTo":"20070228214851.GC4149@nan92-1-81-57-214-146.fbx.proxad.net","subject":"Re: [PATCH] Add a testcase for the safety of pull-policy='pull'.","fromName":"Yann Dirson","fromEmail":"ydirson@altern.org","sentAt":"2007-03-01T20:10:47Z","receivedAt":"2007-03-01T20:10:47Z","isPatch":true,"sender":{"key":"ydirson@altern.org","avatar":"https://avatars.githubusercontent.com/u/1190950?v=4"},"body":"On Wed, Feb 28, 2007 at 10:48:51PM +0100, Yann Dirson wrote:\n> On Tue, Feb 27, 2007 at 11:38:45PM +0000, Catalin Marinas wrote:\n> > >> BTW, push --undo now requires a status --reset beforehand.\n> > >\n> > >Oh, I had not noticed that. Why so ? It's not like if \"push --undo\"\n> > >would lose any valuable data...\n> > \n> > I added it so that one cannot remove the local changes by doing a\n> > \"push --undo\" in error. You would have to explicitly ask for local\n> > changes removing with status --reset.\n> \n> At least, the message in that case should probably be made better,\n> when undoing to avoid having to resolve a conflict: the message says I\n> cannot undo because I have a conflict, whereas it is the exact reason\n> why I want to undo.\n> Especially, since the conflict markers are now auto-recorded, we could\n> simply allow undo in that case.\n\nActually, I find the behaviour to be much worse than before: forcing\nthe user to run \"status --reset\" before \"push --undo\" indeed brings\nthe patch that conflicted in a state where it is partly committed.\nThat is, if the user is interrupted in her work after resetting, and\nlater comes back, it is not unlikely that she forget that an operation\nhas not been finished, and eg. run \"stg pop\", in which case the patch\nwould be left in an unwanted state. Eg, I just caused the problem\nwhen pushing those patches of mine you integrated: the \"editor\" patch,\nwhich is later modified by one of yours, caused a conflict on puch,\nwhich causes an empty patch to be recorded (no idea why there was no\nconflict markers recorded, BTW).\n\nIndeed, the problem may well be that we should not commit the\nunresolved conflict. Why it has some value, recording it as if the\nuser had refreshed the patch is probably not a good idea at all,\nprecisely because we're mid-way in the push, and that the operation\ncan be aborted without stgit knowing.\n\nMaybe with transactions we could manage to make something sensible\nhere, but even then, I'm not sure if we would be able to detect that\nthe user's actions should abort the push transaction and rollback to\nthe previous state. Sounds really too much DWIM in the end.\n\nMaybe instead we should write such a commit, but not as if it was a\nrefresh. Maybe under some .../patches/name.conflict ref, leaving the\npatch itself untouched, but causing the stack to be blocked until the\npush gets undone (in which case things look simple), or the patch gets\nresolved+refreshed (in which case, both the conflict and its\nresolution can appear in the patchlog, but things should be made so\n\"push --undo\" would rollback the 2 ones as a whole).\n\nDoes that sound sensible ?\n\nBest regards,\n-- \nYann.\n"},{"id":"36483","messageId":"b0943d9e0703061437t67f3be3s3358b4fe3ecb2b0a@mail.gmail.com","threadId":"6955","inReplyTo":"20070301201047.GD4149@nan92-1-81-57-214-146.fbx.proxad.net","subject":"Re: [PATCH] Add a testcase for the safety of pull-policy='pull'.","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2007-03-06T22:37:34Z","receivedAt":"2007-03-06T22:37:34Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"On 01/03/07, Yann Dirson wrote:\n> On Wed, Feb 28, 2007 at 10:48:51PM +0100, Yann Dirson wrote:\n> > On Tue, Feb 27, 2007 at 11:38:45PM +0000, Catalin Marinas wrote:\n> > > I added it so that one cannot remove the local changes by doing a\n> > > \"push --undo\" in error. You would have to explicitly ask for local\n> > > changes removing with status --reset.\n> >\n> > At least, the message in that case should probably be made better,\n> > when undoing to avoid having to resolve a conflict: the message says I\n> > cannot undo because I have a conflict, whereas it is the exact reason\n> > why I want to undo.\n\nThat's because of the order of the safety checks.\n\n> > Especially, since the conflict markers are now auto-recorded, we could\n> > simply allow undo in that case.\n>\n> Actually, I find the behaviour to be much worse than before: forcing\n> the user to run \"status --reset\" before \"push --undo\" indeed brings\n> the patch that conflicted in a state where it is partly committed.\n\nI don't have a strong opinion on forcing status --reset before push\n--undo anyway, so I can revert that commit (it bothers me a bit as\nwell :-)).\n\n> Indeed, the problem may well be that we should not commit the\n> unresolved conflict. Why it has some value, recording it as if the\n> user had refreshed the patch is probably not a good idea at all,\n> precisely because we're mid-way in the push, and that the operation\n> can be aborted without stgit knowing.\n\nThe idea of commit 0f4eba6a37c1a5454560b097873e5a22bfcde908 was to\nonly show the conflict files with 'stg diff' and also allow me to\nlater see how I fixed a conflict via the 'stg log (-p|-g)' command.\nBut this interim refresh shouldn't affect the backup information\n(backup = False in Series.refresh_patch). Do you have a simple test\nshowing this problem?\n\nRegards.\n\n-- \nCatalin\n"}]}