threads / patch / 7582

patchPass -C1 to git-apply in StGIT's apply_diff() and apply_patch().

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

## tl;dr

6 messages between Apr 9, 2007 and Apr 11, 2007. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Tomash Brechko· Apr 9, 2007, 11:24 UTC · lore

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(-)

Show changes to stgit/git.py +2 −2

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

Catalin Marinas· Apr 10, 2007, 16:48 UTC · re: Tomash Brechko · lore

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

On 09/04/07, Tomash Brechko <tomash.brechko@gmail.com> wrote:

Show 11 quoted lines

> 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

Tomash Brechko· Apr 10, 2007, 19:21 UTC · re: Catalin Marinas · lore

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

On Tue, Apr 10, 2007 at 17:48:29 +0100, Catalin Marinas wrote:

Show 12 quoted lines

> >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

Tomash Brechko· Apr 10, 2007, 19:32 UTC · re: Tomash Brechko · lore

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

On Tue, Apr 10, 2007 at 23:21:30 +0400, Tomash Brechko wrote:

Show 8 quoted lines

> 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

Catalin Marinas· Apr 10, 2007, 22:38 UTC · re: Tomash Brechko · lore

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

On 10/04/07, Tomash Brechko <tomash.brechko@gmail.com> wrote:

Show 6 quoted lines

> 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

Tomash Brechko· Apr 11, 2007, 07:51 UTC · re: Catalin Marinas · lore

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

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