[PATCH] Stgit - gitmergeonefile.py: handle removal vs. changes

[PATCH] Stgit - gitmergeonefile.py: handle removal vs. changes

[Paolo 'Blaisorblade' Giarrusso]

I just got a "removal vs. changed" conflict, which is unhandled by StGit. That
is taken from git-merge-one-file resolver, but is bad, as stg resolved does not
handle unmerged entries (and probably it should be fixed too).

Sample patch included, but some thought must be done on it (see the comments I
left in).

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
---

 gitmergeonefile.py |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/gitmergeonefile.py b/gitmergeonefile.py
index 1cba193..9344d33 100755
--- a/gitmergeonefile.py
+++ b/gitmergeonefile.py
@@ -180,6 +180,30 @@ if orig_hash:
             os.remove(path)
         __remove_files()
         sys.exit(os.system('git-update-index --remove -- %s' % path))
+    # file deleted in one and changed in the other
+    else:
+        # Do something here - we must at least merge the entry in the cache,
+        # instead of leaving it in U(nmerged) state. In fact, stg resolved
+        # does not handle that.
+
+        # Do the same thing cogito does - remove the file in any case.
+        os.system('git-update-index --remove -- %s' % path)
+
+        #if file1_hash:
+            ## file deleted upstream and changed in the patch. The patch is
+            ## probably going to move the changes elsewhere.
+
+            #os.system('git-update-index --remove -- %s' % path)
+        #else:
+            ## file deleted in the patch and changed upstream. We could re-delete
+            ## it, but for now leave it there - and let the user check if he
+            ## still wants to remove the file.
+
+            ## reset the cache to the first branch
+            #os.system('git-update-index --cacheinfo %s %s %s'
+                      #% (file1_mode, file1_hash, path))
+        __conflict()
+
 # file does not exist in origin
 else:
     # file added in both

Re: [PATCH] Stgit - gitmergeonefile.py: handle removal vs. changes

[Catalin Marinas]

On 13/11/05, Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it> wrote:
> I just got a "removal vs. changed" conflict, which is unhandled by StGit. That
> is taken from git-merge-one-file resolver, but is bad, as stg resolved does not
> handle unmerged entries (and probably it should be fixed too).

I think it 'stg resolved' should be fixed as well (in case there are
unmerged entries for other reasons). My initial idea was to make
gitmergeonefile not to leave any unmerged entries in the index. As you
could see, there are cases where it failed.

I can see the following scenarios for a file:

1. deleted in the base and modified by the patch. It should leave the
file in the tree together with file.older. Another option would be to
remove the file and leave both file.older and file.remote in the tree
(here .remote means the version in the patch) but I would prefer the
first one.

2. changed in the base but deleted by the patch. It should remove the
file from the tree but leave file.older and file.local. The other
option is to leave the file in the tree but, as above, I prefer the
first one.

Maybe StGIT should try to track the renaming as well but I haven't
played with this feature in GIT at all.

--
Catalin

Re: [PATCH] Stgit - gitmergeonefile.py: handle removal vs. changes

[Blaisorblade]

On Tuesday 15 November 2005 10:54, Catalin Marinas wrote:
> On 13/11/05, Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it> wrote:
> > I just got a "removal vs. changed" conflict, which is unhandled by StGit.
> > That is taken from git-merge-one-file resolver, but is bad, as stg
> > resolved does not handle unmerged entries (and probably it should be
> > fixed too).

> I think it 'stg resolved' should be fixed as well (in case there are
> unmerged entries for other reasons).

Yep, I was thinking that too but was too lazy to implement.

Actually, with .git/commits we are reimplementing handling of "unmerged" 
entries... it could be better to use the "unmerged entry" stgit idea. So "stg 
resolved" should modify the entries by itself.

> My initial idea was to make 
> gitmergeonefile not to leave any unmerged entries in the index. As you
> could see, there are cases where it failed.

Yep... it seems you took examples from git-merge-one-file, but that's lacking 
(but it's low-level so it's appropriate for it - it must leave unmerged 
entries when there are conflicts).

> I can see the following scenarios for a file:

In both cases, we're going to have a conflict, so we leave file.
{older,remote,local} as appropriate and already done.

> 1. deleted in the base and modified by the patch. It should leave the
> file in the tree together with file.older.

Why not leaving file.remote? We already do that in general, so we have a 
duplicate, but it's easier to understand.

> Another option would be to 
> remove the file and leave both file.older and file.remote in the tree
> (here .remote means the version in the patch)

I remember that at times, but .remote is very confusing... I see that's the 
mishandling is induced by various sources, maybe including "merge" itself, 
but that program (and possibly others) supports changing the labels, and this 
should probably be done (using "original", "patched" and "upstream" 
probably).

> but I would prefer the 
> first one.

> 2. changed in the base but deleted by the patch. It should remove the
> file from the tree but leave file.older and file.local. The other
> option is to leave the file in the tree but, as above, I prefer the
> first one.

The policy about when to remove the file and when to leave it is very 
personal... the user must anyway solve the conflict in some smart way... 
about the defaults, anything would do, but if we really care we could leave 
the user the choice.

For the Linux kernel, my experience is that when a file is removed it's either 
because it's renamed, it's refactored, or it's removed. In all these cases, 
there's often little interest in reviving it in the patch... However it's 
just a slight preference.

> Maybe StGIT should try to track the renaming as well but I haven't
> played with this feature in GIT at all.

-- 
Inform me of my mistakes, so I can keep imitating Homer Simpson's "Doh!".
Paolo Giarrusso, aka Blaisorblade (Skype ID "PaoloGiarrusso", ICQ 215621894)
http://www.user-mode-linux.org/~blaisorblade

	

	
		
___________________________________ 
Yahoo! Mail: gratis 1GB per i messaggi e allegati da 10MB 
http://mail.yahoo.it

Re: [PATCH] Stgit - gitmergeonefile.py: handle removal vs. changes

[Catalin Marinas]

On 16/11/05, Blaisorblade <blaisorblade@yahoo.it> wrote:
> On Tuesday 15 November 2005 10:54, Catalin Marinas wrote:
> Actually, with .git/commits we are reimplementing handling of "unmerged"
> entries... it could be better to use the "unmerged entry" stgit idea. So "stg
> resolved" should modify the entries by itself.

But would a git-diff-tree still show the changes between the current
files and the index if there are unmerged entries? I haven't tried it.

> > My initial idea was to make
> > gitmergeonefile not to leave any unmerged entries in the index. As you
> > could see, there are cases where it failed.
>
> Yep... it seems you took examples from git-merge-one-file, but that's lacking
> (but it's low-level so it's appropriate for it - it must leave unmerged
> entries when there are conflicts).

When I started writing StGIT, my main thoughts were driven towards the
patch merging/commuting via the diff3 algorithm. I found it simpler to
copy the algorithm from git-merge-one-file since that wasn't my main
interest in StGIT. I also looked at how Cogito did it.

> > I can see the following scenarios for a file:
>
> In both cases, we're going to have a conflict, so we leave file.
> {older,remote,local} as appropriate and already done.
>
> > 1. deleted in the base and modified by the patch. It should leave the
> > file in the tree together with file.older.
>
> Why not leaving file.remote? We already do that in general, so we have a
> duplicate, but it's easier to understand.

I agree with this.

> > Another option would be to
> > remove the file and leave both file.older and file.remote in the tree
> > (here .remote means the version in the patch)
>
> I remember that at times, but .remote is very confusing... I see that's the
> mishandling is induced by various sources, maybe including "merge" itself,
> but that program (and possibly others) supports changing the labels, and this
> should probably be done (using "original", "patched" and "upstream"
> probably).

I know that diff3/merge support labels. I don't exactly remember my
reasons but I think that I chose those namings because StGIT was
supporting another type of merge where "patched" etc. did not apply.

I agree that we should change them. I would rather use "ancestor",
"patch" and "base" but I don't have a strong opinion.

> > 2. changed in the base but deleted by the patch. It should remove the
> > file from the tree but leave file.older and file.local. The other
> > option is to leave the file in the tree but, as above, I prefer the
> > first one.
>
> The policy about when to remove the file and when to leave it is very
> personal... the user must anyway solve the conflict in some smart way...
> about the defaults, anything would do, but if we really care we could leave
> the user the choice.

At the moment, the conflicts usually leave the index in the state
before pushing the patch. I think it should also leave the file and
just mark it as conflict in .git/conflicts.

--
Catalin

Re: [PATCH] Stgit - gitmergeonefile.py: handle removal vs. changes

[Chuck Lever]

[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]

Catalin Marinas wrote:
> On 16/11/05, Blaisorblade <blaisorblade@yahoo.it> wrote:
>>>Another option would be to
>>>remove the file and leave both file.older and file.remote in the tree
>>>(here .remote means the version in the patch)
>>
>>I remember that at times, but .remote is very confusing... I see that's the
>>mishandling is induced by various sources, maybe including "merge" itself,
>>but that program (and possibly others) supports changing the labels, and this
>>should probably be done (using "original", "patched" and "upstream"
>>probably).
> 
> 
> I know that diff3/merge support labels. I don't exactly remember my
> reasons but I think that I chose those namings because StGIT was
> supporting another type of merge where "patched" etc. did not apply.
> 
> I agree that we should change them. I would rather use "ancestor",
> "patch" and "base" but I don't have a strong opinion.

just a data point:

i use "original" "patch" and "older" (set up in .stgitrc) because i 
found the default labels to be confusing.

but "original" "patch" and "upstream" make sense to me.

[-- Attachment #2: cel.vcf --]
[-- Type: text/x-vcard, Size: 439 bytes --]

begin:vcard
fn:Chuck Lever
n:Lever;Charles
org:Network Appliance, Incorporated;Linux NFS Client Development
adr:535 West William Street, Suite 3100;;Center for Information Technology Integration;Ann Arbor;MI;48103-4943;USA
email;internet:cel@citi.umich.edu
title:Member of Technical Staff
tel;work:+1 734 763 4415
tel;fax:+1 734 763 4434
tel;home:+1 734 668 1089
x-mozilla-html:FALSE
url:http://www.monkey.org/~cel/
version:2.1
end:vcard

Re: [PATCH] Stgit - gitmergeonefile.py: handle removal vs. changes

[Catalin Marinas]

On 17/11/05, Chuck Lever <cel@citi.umich.edu> wrote:
> i use "original" "patch" and "older" (set up in .stgitrc) because i
> found the default labels to be confusing.
>
> but "original" "patch" and "upstream" make sense to me.

These names would need to have a meaning for the result of the 'fold
--threeway' and 'pick' commands. 'patch' and 'original' are ok but
'upstream' might not make sense since for these commands it can
represent the top of an existing patch.

--
Catalin

Re: [PATCH] Stgit - gitmergeonefile.py: handle removal vs. changes

[Blaisorblade]

On Tuesday 15 November 2005 10:54, Catalin Marinas wrote:
> On 13/11/05, Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it> wrote:
> > I just got a "removal vs. changed" conflict, which is unhandled by StGit.
> > That is taken from git-merge-one-file resolver, but is bad, as stg
> > resolved does not handle unmerged entries (and probably it should be
> > fixed too).
>
> I think it 'stg resolved' should be fixed as well (in case there are
> unmerged entries for other reasons). My initial idea was to make
> gitmergeonefile not to leave any unmerged entries in the index. As you
> could see, there are cases where it failed.

The original patch hasn't been merged, nor (for what I see) anything else to 
fix this problem has been done.

I assume the patch was lost waiting for the discussion to settle down, but the 
patch can be merged, even changing the default choices in any way (see 
below).

Also, another note: I just found Bruce Eckel mentioning pychecker, which is a 
static code checker for Python (to perform the checks a compiler would 
normally do). I've not the time to investigate more myself, but I hope it can 
be useful to you.

> I can see the following scenarios for a file:

> 1. deleted in the base and modified by the patch. It should leave the
> file in the tree together with file.older. Another option would be to
> remove the file and leave both file.older and file.remote in the tree
> (here .remote means the version in the patch) but I would prefer the
> first one.
>
> 2. changed in the base but deleted by the patch. It should remove the
> file from the tree but leave file.older and file.local. The other
> option is to leave the file in the tree but, as above, I prefer the
> first one.
>
> Maybe StGIT should try to track the renaming as well but I haven't
> played with this feature in GIT at all.

-- 
Inform me of my mistakes, so I can keep imitating Homer Simpson's "Doh!".
Paolo Giarrusso, aka Blaisorblade (Skype ID "PaoloGiarrusso", ICQ 215621894)
http://www.user-mode-linux.org/~blaisorblade

	

	
		
___________________________________ 
Yahoo! Mail: gratis 1GB per i messaggi e allegati da 10MB 
http://mail.yahoo.it

Re: [PATCH] Stgit - gitmergeonefile.py: handle removal vs. changes

[Catalin Marinas]

Blaisorblade wrote:

>The original patch hasn't been merged, nor (for what I see) anything else to 
>fix this problem has been done.
>  
>
Indeed, I forgot about it.

>I assume the patch was lost waiting for the discussion to settle down, but the 
>patch can be merged, even changing the default choices in any way (see 
>below).
>  
>
I merged it as it is. I will think about the default options once I get
some time to fix the .local, .older and .remote extensions (give them
some meaningful names).

>Also, another note: I just found Bruce Eckel mentioning pychecker, which is a 
>static code checker for Python (to perform the checks a compiler would 
>normally do). I've not the time to investigate more myself, but I hope it can 
>be useful to you.
>  
>
Thanks. I'll give it a try.

Catalin

Re: [PATCH] Stgit - gitmergeonefile.py: handle removal vs. changes

[Chuck Lever]

[-- Attachment #1: Type: text/plain, Size: 549 bytes --]

Catalin Marinas wrote:
>>Also, another note: I just found Bruce Eckel mentioning pychecker, which is a 
>>static code checker for Python (to perform the checks a compiler would 
>>normally do). I've not the time to investigate more myself, but I hope it can 
>>be useful to you.
>> 
>>
> 
> Thanks. I'll give it a try.

i already ran pychecker against everything but gitmergeonefile.py... it
found a few things that you have already integrated (like that weird
behavior around using empty lists as the default value for function
arguments).  fyi...

[-- Attachment #2: cel.vcf --]
[-- Type: text/x-vcard, Size: 466 bytes --]

begin:vcard
fn:Chuck Lever
n:Lever;Charles
org:Network Appliance, Incorporated;Open Source NFS Client Development
adr:535 West William Street, Suite 3100;;Center for Information Technology Integration;Ann Arbor;MI;48103-4943;USA
email;internet:cel@citi.umich.edu
title:Member of Technical Staff
tel;work:+1 734 763-4415
tel;fax:+1 734 763 4434
tel;home:+1 734 668-1089
x-mozilla-html:FALSE
url:http://troy.citi.umich.edu/u/cel/
version:2.1
end:vcard