[PATCH (resend)] Pass -C1 to git-apply in StGIT's apply_diff() and apply_patch().

[PATCH (resend)] Pass -C1 to git-apply in StGIT's apply_diff() and apply_patch().

[Tomash Brechko]

Running git-apply without -C is too restrictive: when the patch has
some fuzz (it could have been applied upstream with the fuzz, or
different local branches have slightly different context), StGIT would
start manual merge because of the conflict in the context.  Passing
-C1 makes git-apply behave close to default mode of diff/patch: 'diff'
generates 3 lines of context, and 'patch' allows 2 line mismatch,
i.e. it requires the match of at least one context line.

Fix in apply_diff() relaxes the restriction in 'push --merged' and
'rebase --merged' for detection of upstream merges, fix in
apply_patch() does relaxation 'import', 'fold' and 'sync' commands.

This patch is a quick hack, better solution would be to have a control
over the value to -C option, or to have the option similar to
git-cvsexportcommit -p (pedantic mode).
---
 stgit/git.py |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/stgit/git.py b/stgit/git.py
index f6d6b43..bbb41fe 100644
--- a/stgit/git.py
+++ b/stgit/git.py
@@ -660,7 +660,7 @@ def apply_diff(rev1, rev2, check_index = True, files = None):
     diff_str = diff(files, rev1, rev2)
     if diff_str:
         try:
-            _input_str('git-apply %s' % index_opt, diff_str)
+            _input_str('git-apply -C1 %s' % index_opt, diff_str)
         except GitException:
             return False
 
@@ -930,7 +930,7 @@ def apply_patch(filename = None, diff = None, base = None,
         refresh_index()
 
     try:
-        _input_str('git-apply --index', diff)
+        _input_str('git-apply -C1 --index', diff)
     except GitException:
         if base:
             switch(orig_head)
-- 
1.5.1.82.g46af1-dirty

Re: [PATCH (resend)] Pass -C1 to git-apply in StGIT's apply_diff() and apply_patch().

[Catalin Marinas]

On 09/04/07, Tomash Brechko <tomash.brechko@gmail.com> wrote:
> Running git-apply without -C is too restrictive: when the patch has
> some fuzz (it could have been applied upstream with the fuzz, or
> different local branches have slightly different context), StGIT would
> start manual merge because of the conflict in the context.  Passing
> -C1 makes git-apply behave close to default mode of diff/patch: 'diff'
> generates 3 lines of context, and 'patch' allows 2 line mismatch,
> i.e. it requires the match of at least one context line.
>
> Fix in apply_diff() relaxes the restriction in 'push --merged' and
> 'rebase --merged' for detection of upstream merges, fix in
> apply_patch() does relaxation 'import', 'fold' and 'sync' commands.

Thanks for the patch. I'm OK with -C1 in apply_patch() but I'm a bit
concerned with the 'push/rebase --merged' logic being relaxed. There
is also the reporting of patches being modified during 'push', i.e.
the push succeeded only after a three-way merge.

I think I could add separate config options for both apply_diff and
apply_patch, only that it might confuse users not knowing the StGIT
internals.

-- 
Catalin

Re: [PATCH (resend)] Pass -C1 to git-apply in StGIT's apply_diff() and apply_patch().

[Tomash Brechko]

On Tue, Apr 10, 2007 at 17:48:29 +0100, Catalin Marinas wrote:
> >Fix in apply_diff() relaxes the restriction in 'push --merged' and
> >'rebase --merged' for detection of upstream merges, fix in
> >apply_patch() does relaxation 'import', 'fold' and 'sync' commands.
> 
> Thanks for the patch. I'm OK with -C1 in apply_patch() but I'm a bit
> concerned with the 'push/rebase --merged' logic being relaxed. There
> is also the reporting of patches being modified during 'push', i.e.
> the push succeeded only after a three-way merge.
> 
> I think I could add separate config options for both apply_diff and
> apply_patch, only that it might confuse users not knowing the StGIT
> internals.

Aha, I've made a mistake, I wanted to say 'pull --merged and rebase
--merged', not 'push'.  The idea was that StGIT should be liberal when
it decides if the patch was applied upsteam, it should not force the
user to merge her own patch back because of different context
upstream.  Of course we can imagine the situation when during such
merge the user will realize that her patch was applied upstream
incorrectly, but such cases will be rare, so better not to enforce the
merge.

But I see your point, and back then I didn't realize how it will
affect the 'push' command.

So, I think the best would be to have 'pull'-like commands (pull,
rebase, import, fold, sync) to be liberal by default (accept pathes
with -C1), while 'push'-like commands (push, any other?) to be
conservative (require full context match).  And both classes should
provide the way to explicitly control acceptance level.


-- 
   Tomash Brechko

Re: [PATCH (resend)] Pass -C1 to git-apply in StGIT's apply_diff() and apply_patch().

[Tomash Brechko]

On Tue, Apr 10, 2007 at 23:21:30 +0400, Tomash Brechko wrote:
> But I see your point, and back then I didn't realize how it will
> affect the 'push' command.
> 
> So, I think the best would be to have 'pull'-like commands (pull,
> rebase, import, fold, sync) to be liberal by default (accept pathes
> with -C1), while 'push'-like commands (push, any other?) to be
> conservative (require full context match).  And both classes should
> provide the way to explicitly control acceptance level.

Please disregard this part.  It's late here, and I mistook StGIT's
push for GIT's push.  Once we are talking about StGIT's push (push of
the patch back to the stack), why would we want to start tree-way
merge when the context has changed?  My point was exactly that since I
want to keep my patches up-to-date with the main branch, I do rebase
from time to time, and I'm not interested in doing the merge every
time just because something has changed upstream in surrounding code.

The same goes for patches that were already applied upstream.
Whatever the current context around the code of my applied patch is, I
have to accept it, because the patch was applied.  I'm going to throw
it away locally, but currently I have to do the merge first.

I think I didn't get you point.


-- 
   Tomash Brechko

Re: [PATCH (resend)] Pass -C1 to git-apply in StGIT's apply_diff() and apply_patch().

[Catalin Marinas]

On 10/04/07, Tomash Brechko <tomash.brechko@gmail.com> wrote:
> Once we are talking about StGIT's push (push of
> the patch back to the stack), why would we want to start tree-way
> merge when the context has changed?  My point was exactly that since I
> want to keep my patches up-to-date with the main branch, I do rebase
> from time to time, and I'm not interested in doing the merge every
> time just because something has changed upstream in surrounding code.

When something has changed in the surrounding code (not touched by
your patch), the automatic three-way merge should, in general, be able
to solve the issue as it uses the ancestor information. Is the
automatic three-way merge failing as well in your case?

> The same goes for patches that were already applied upstream.
> Whatever the current context around the code of my applied patch is, I
> have to accept it, because the patch was applied.  I'm going to throw
> it away locally, but currently I have to do the merge first.

I think -C1 should be OK for merge detection (in most situations) and
importing patch files (via import, fold) but I personally don't like
it when rebasing a patch. I still prefer a more precise context
checking, rather than the fuzzy one similar to the "patch" tool (as
the line numbers are usually volatile).

I'm OK with the idea of this patch but I would prefer a config option
and/or command line option rather than hard-coding it for people with
different views. A command line option could make sense for commands
like import/fold and a config option for the rest.

Thanks.

-- 
Catalin

Re: [PATCH (resend)] Pass -C1 to git-apply in StGIT's apply_diff() and apply_patch().

[Tomash Brechko]

On Tue, Apr 10, 2007 at 23:38:55 +0100, Catalin Marinas wrote:
> When something has changed in the surrounding code (not touched by
> your patch), the automatic three-way merge should, in general, be able
> to solve the issue as it uses the ancestor information. Is the
> automatic three-way merge failing as well in your case?

OK, maybe I was pushing too much.  Indeed, three-way merge will solve
it, I just don't have a setup where it runs automatically, and I
thought the mege could be avoided alltogether.  But I agree the
problem is easily managable.

And as you agree that the option would be nice we are on the same
page.  Thanks you!


-- 
   Tomash Brechko