{"thread":{"id":"7582","subject":"[PATCH (resend)] Pass -C1 to git-apply in StGIT's apply_diff() and apply_patch().","startedAt":"2007-04-09T11:24:22Z","lastAt":"2007-04-11T07:51:33Z","messageCount":6,"participants":["Tomash Brechko","Catalin Marinas"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"38929","messageId":"20070409112422.GE11593@moonlight.home","threadId":"7582","inReplyTo":null,"subject":"[PATCH (resend)] Pass -C1 to git-apply in StGIT's apply_diff() and apply_patch().","fromName":"Tomash Brechko","fromEmail":"tomash.brechko@gmail.com","sentAt":"2007-04-09T11:24:22Z","receivedAt":"2007-04-09T11:24:22Z","isPatch":true,"sender":{"key":"tomash.brechko@gmail.com","avatar":null},"body":"Running git-apply without -C is too restrictive: when the patch has\nsome fuzz (it could have been applied upstream with the fuzz, or\ndifferent local branches have slightly different context), StGIT would\nstart manual merge because of the conflict in the context. Passing\n-C1 makes git-apply behave close to default mode of diff/patch: 'diff'\ngenerates 3 lines of context, and 'patch' allows 2 line mismatch,\ni.e. it requires the match of at least one context line.\n\nFix in apply_diff() relaxes the restriction in 'push --merged' and\n'rebase --merged' for detection of upstream merges, fix in\napply_patch() does relaxation 'import', 'fold' and 'sync' commands.\n\nThis patch is a quick hack, better solution would be to have a control\nover the value to -C option, or to have the option similar to\ngit-cvsexportcommit -p (pedantic mode).\n---\n stgit/git.py | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/stgit/git.py b/stgit/git.py\nindex f6d6b43..bbb41fe 100644\n--- a/stgit/git.py\n+++ b/stgit/git.py\n@@ -660,7 +660,7 @@ def apply_diff(rev1, rev2, check_index = True, files = None):\n diff_str = diff(files, rev1, rev2)\n if diff_str:\n try:\n- _input_str('git-apply %s' % index_opt, diff_str)\n+ _input_str('git-apply -C1 %s' % index_opt, diff_str)\n except GitException:\n return False\n \n@@ -930,7 +930,7 @@ def apply_patch(filename = None, diff = None, base = None,\n refresh_index()\n \n try:\n- _input_str('git-apply --index', diff)\n+ _input_str('git-apply -C1 --index', diff)\n except GitException:\n if base:\n switch(orig_head)\n-- \n1.5.1.82.g46af1-dirty\n"},{"id":"39033","messageId":"b0943d9e0704100948k2b505916w5485b99e72d36c10@mail.gmail.com","threadId":"7582","inReplyTo":"20070409112422.GE11593@moonlight.home","subject":"Re: [PATCH (resend)] Pass -C1 to git-apply in StGIT's apply_diff() and apply_patch().","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2007-04-10T16:48:29Z","receivedAt":"2007-04-10T16:48:29Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"On 09/04/07, Tomash Brechko wrote:\n> Running git-apply without -C is too restrictive: when the patch has\n> some fuzz (it could have been applied upstream with the fuzz, or\n> different local branches have slightly different context), StGIT would\n> start manual merge because of the conflict in the context. Passing\n> -C1 makes git-apply behave close to default mode of diff/patch: 'diff'\n> generates 3 lines of context, and 'patch' allows 2 line mismatch,\n> i.e. it requires the match of at least one context line.\n>\n> Fix in apply_diff() relaxes the restriction in 'push --merged' and\n> 'rebase --merged' for detection of upstream merges, fix in\n> apply_patch() does relaxation 'import', 'fold' and 'sync' commands.\n\nThanks for the patch. I'm OK with -C1 in apply_patch() but I'm a bit\nconcerned with the 'push/rebase --merged' logic being relaxed. There\nis also the reporting of patches being modified during 'push', i.e.\nthe push succeeded only after a three-way merge.\n\nI think I could add separate config options for both apply_diff and\napply_patch, only that it might confuse users not knowing the StGIT\ninternals.\n\n-- \nCatalin\n"},{"id":"39049","messageId":"20070410192130.GE4946@moonlight.home","threadId":"7582","inReplyTo":"b0943d9e0704100948k2b505916w5485b99e72d36c10@mail.gmail.com","subject":"Re: [PATCH (resend)] Pass -C1 to git-apply in StGIT's apply_diff() and apply_patch().","fromName":"Tomash Brechko","fromEmail":"tomash.brechko@gmail.com","sentAt":"2007-04-10T19:21:30Z","receivedAt":"2007-04-10T19:21:30Z","isPatch":true,"sender":{"key":"tomash.brechko@gmail.com","avatar":null},"body":"On Tue, Apr 10, 2007 at 17:48:29 +0100, Catalin Marinas wrote:\n> >Fix in apply_diff() relaxes the restriction in 'push --merged' and\n> >'rebase --merged' for detection of upstream merges, fix in\n> >apply_patch() does relaxation 'import', 'fold' and 'sync' commands.\n> \n> Thanks for the patch. I'm OK with -C1 in apply_patch() but I'm a bit\n> concerned with the 'push/rebase --merged' logic being relaxed. There\n> is also the reporting of patches being modified during 'push', i.e.\n> the push succeeded only after a three-way merge.\n> \n> I think I could add separate config options for both apply_diff and\n> apply_patch, only that it might confuse users not knowing the StGIT\n> internals.\n\nAha, I've made a mistake, I wanted to say 'pull --merged and rebase\n--merged', not 'push'. The idea was that StGIT should be liberal when\nit decides if the patch was applied upsteam, it should not force the\nuser to merge her own patch back because of different context\nupstream. Of course we can imagine the situation when during such\nmerge the user will realize that her patch was applied upstream\nincorrectly, but such cases will be rare, so better not to enforce the\nmerge.\n\nBut I see your point, and back then I didn't realize how it will\naffect the 'push' command.\n\nSo, I think the best would be to have 'pull'-like commands (pull,\nrebase, import, fold, sync) to be liberal by default (accept pathes\nwith -C1), while 'push'-like commands (push, any other?) to be\nconservative (require full context match). And both classes should\nprovide the way to explicitly control acceptance level.\n\n\n-- \n Tomash Brechko\n"},{"id":"39025","messageId":"20070410193214.GF4946@moonlight.home","threadId":"7582","inReplyTo":"20070410192130.GE4946@moonlight.home","subject":"Re: [PATCH (resend)] Pass -C1 to git-apply in StGIT's apply_diff() and apply_patch().","fromName":"Tomash Brechko","fromEmail":"tomash.brechko@gmail.com","sentAt":"2007-04-10T19:32:14Z","receivedAt":"2007-04-10T19:32:14Z","isPatch":true,"sender":{"key":"tomash.brechko@gmail.com","avatar":null},"body":"On Tue, Apr 10, 2007 at 23:21:30 +0400, Tomash Brechko wrote:\n> But I see your point, and back then I didn't realize how it will\n> affect the 'push' command.\n> \n> So, I think the best would be to have 'pull'-like commands (pull,\n> rebase, import, fold, sync) to be liberal by default (accept pathes\n> with -C1), while 'push'-like commands (push, any other?) to be\n> conservative (require full context match). And both classes should\n> provide the way to explicitly control acceptance level.\n\nPlease disregard this part. It's late here, and I mistook StGIT's\npush for GIT's push. Once we are talking about StGIT's push (push of\nthe patch back to the stack), why would we want to start tree-way\nmerge when the context has changed? My point was exactly that since I\nwant to keep my patches up-to-date with the main branch, I do rebase\nfrom time to time, and I'm not interested in doing the merge every\ntime just because something has changed upstream in surrounding code.\n\nThe same goes for patches that were already applied upstream.\nWhatever the current context around the code of my applied patch is, I\nhave to accept it, because the patch was applied. I'm going to throw\nit away locally, but currently I have to do the merge first.\n\nI think I didn't get you point.\n\n\n-- \n Tomash Brechko\n"},{"id":"39065","messageId":"b0943d9e0704101538p3de0bf56m7906cfe2f5fc157e@mail.gmail.com","threadId":"7582","inReplyTo":"20070410193214.GF4946@moonlight.home","subject":"Re: [PATCH (resend)] Pass -C1 to git-apply in StGIT's apply_diff() and apply_patch().","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2007-04-10T22:38:55Z","receivedAt":"2007-04-10T22:38:55Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"On 10/04/07, Tomash Brechko wrote:\n> Once we are talking about StGIT's push (push of\n> the patch back to the stack), why would we want to start tree-way\n> merge when the context has changed? My point was exactly that since I\n> want to keep my patches up-to-date with the main branch, I do rebase\n> from time to time, and I'm not interested in doing the merge every\n> time just because something has changed upstream in surrounding code.\n\nWhen something has changed in the surrounding code (not touched by\nyour patch), the automatic three-way merge should, in general, be able\nto solve the issue as it uses the ancestor information. Is the\nautomatic three-way merge failing as well in your case?\n\n> The same goes for patches that were already applied upstream.\n> Whatever the current context around the code of my applied patch is, I\n> have to accept it, because the patch was applied. I'm going to throw\n> it away locally, but currently I have to do the merge first.\n\nI think -C1 should be OK for merge detection (in most situations) and\nimporting patch files (via import, fold) but I personally don't like\nit when rebasing a patch. I still prefer a more precise context\nchecking, rather than the fuzzy one similar to the \"patch\" tool (as\nthe line numbers are usually volatile).\n\nI'm OK with the idea of this patch but I would prefer a config option\nand/or command line option rather than hard-coding it for people with\ndifferent views. A command line option could make sense for commands\nlike import/fold and a config option for the rest.\n\nThanks.\n\n-- \nCatalin\n"},{"id":"39093","messageId":"20070411075133.GA5329@moonlight.home","threadId":"7582","inReplyTo":"b0943d9e0704101538p3de0bf56m7906cfe2f5fc157e@mail.gmail.com","subject":"Re: [PATCH (resend)] Pass -C1 to git-apply in StGIT's apply_diff() and apply_patch().","fromName":"Tomash Brechko","fromEmail":"tomash.brechko@gmail.com","sentAt":"2007-04-11T07:51:33Z","receivedAt":"2007-04-11T07:51:33Z","isPatch":true,"sender":{"key":"tomash.brechko@gmail.com","avatar":null},"body":"On Tue, Apr 10, 2007 at 23:38:55 +0100, Catalin Marinas wrote:\n> When something has changed in the surrounding code (not touched by\n> your patch), the automatic three-way merge should, in general, be able\n> to solve the issue as it uses the ancestor information. Is the\n> automatic three-way merge failing as well in your case?\n\nOK, maybe I was pushing too much. Indeed, three-way merge will solve\nit, I just don't have a setup where it runs automatically, and I\nthought the mege could be avoided alltogether. But I agree the\nproblem is easily managable.\n\nAnd as you agree that the option would be nice we are on the same\npage. Thanks you!\n\n\n-- \n Tomash Brechko\n"}]}