Re: Merge with git-pasky II.

PB> Bah, you outran me. ;-)

Just being in a different timezone, I guess.

PB> I'll change it to use the cool git-pasky stuff (commit-id etc) and its PB> style of committing - that is, it will merely record the update-caches PB> to be done upon commit, and it will read-tree the branch we are merging PB> to instead of the ancestor. (So that git diff gives useful output.)

Sorry, I have not seen what you have been doing since pasky 0.3, and I have not even started to understand the mental model of the world your tool is building. That said, my gut feeling is that telling this script about git-pasky's world model might be a mistake. I'd rather see you consider the script as mere "part of the plumbing". Maybe adding an extra parameter to the script to let the user explicitly specify the common ancestor to use would be needed, but I would prefer git-pasky-merge to do its own magic (converting symbolic commit names into raw commit names and such) before calling this low level script.

That way people like me who have not migrated to your framework can still keep using it. All the script currently needs is a bare git object database; i.e., nothing other than what is in .git/objects and a couple of commit record SHA1s as its parameters. No .git/heads/, no .git/HEAD.local, no .git/tags, are involved for it to work, and I would prefer to keep things that way if possible.

PB> -r traditionally means recursive - what's the reasoning behind the PB> choice of this letter?

Well, '-r' is not necessarily recursive. "ls -r" is reverse, "sort -r" is reverse. "less -r" is raw. "cat -r" is reversible. "nethack -r" is race ;-). You are thinking as an SCM person so it may look that way. "diff -r" is recursive. "darcs add -r" is recursive. But even in the SCM world, "cvs add -r" is not (it means read-only) neither "co -r" (explicit revision) ;-).

I would rather pick '-q' if I were doing the patch today, but I was too tired and did not think of a letter when I wrote it. I guess '-r' stood for removed, but I agree it is a bad choice. Any objections to '-q'?

PB> use strict?

Not in this iteration but eventually yes.

PB> It'd be simpler to do just

PB> my @common = (map { $common{$_} } PB> sort { $b <=> $a } PB> keys %common)

Well, actually you spotted a bug between the implementation and what I wanted to do. It should have been:

map { $_->[0] }
    sort { $b->[1] <=> $a->[1] }
        map { [ $common{$_} => $_ ] } keys %common

That is, sort [anscestor => number of times it appears] tuple by the "number of times it appears" in decreasing order, and project the resulting list to a list of ancestors. It is trying to deal with the following pattern in rev-tree output:

TIMESTAMP1 EDGE1:1 ANCESTOR1:3 ANCESTOR2:3 TIMESTAMP2 EDGE2:2 ANCESTOR1:3

and when the above happens I wanted to pick up ANCESTOR1, but that was without no sound reason.

PB> But I really think this is a horrible heuristic. I believe you should PB> take the latest commit in the --edges output, and from that choose the PB> base whose rev-tree --edges the_base merged_branch has the least lines PB> on output. (That is, the path to it is shortest - ideally it's already PB> part of the merged_branch.)

I'll try something along that line. Honestly the ancestor selection part was what I had most trouble with. Thanks.

PB> What about

PB> sub OLDMODE { 0 } PB> sub NEWMODE { 1 } PB> sub NEWSHA { 2 }

PB> and then using that when accessing the tuple? Would make the code PB> much more readable.

Totally agreed; readability cleanup is needed, just as "use strict" you mentioned, before it is ready for public consumption. Remember, however, the primary purpose of the message was to share it with Linus so that I can ask his opinion while the script was still slushy; the contents that array contained was still changing then and was too early for symbolic constants. I'll do that in the next round.

PB> It is a good idea to check merge's exit code and give a notice at the PB> end if there were any conflicts.

In principle yes, but I noticed that merge already gave me a nice warning message when it found conflicts, so there was no need to do so myself in this case. See sample output:

    $ perl ./git-merge.perl \
        71796686221a0a56ccc25b02386ed8ea648da14d \
        bb95843a5a0f397270819462812735ee29796fb4 
    Common ancestor: 9f02d4d233223462d3f6217b5837b786e6286ba4
    O - COPYING
    O - README
    ...
    O - write-tree.c
    A M write-blob.c
    A M show-diff.c
    ...
    A M update-cache.c
    A M git-merge.perl
    B M merge-tree.c
    MRG Makefile
    merge: warning: conflicts during merge
    $ 

PB> Can't we call it just ,,merge?

I'd rather have a command line option '-o' (scrapping the current '-o' and renaming it to something else; as you can see I am terrible at picking option names ;-)) to mean "output to this directory". I am not really an Arch person so I do not particulary care about /^,,/. How about "git~merge~$$"?

PB> Huh, what are you trying to do here? I think you should just record PB> remove, no? (And I wouldn't do anything with my read-tree. ;-)

At this moment in the script, we have run "read-tree" the ancestor so the dircache has the original. %tree0 and %tree1 both did not touch the path ($_ here) so it is the same as ancestor. When '-f' is specified we are populating the output working tree with the merge result so that is what that 'checkout-cache' is about. "O - $path" means "we took the original".

The idea is to populate the dircache of merge-temp with the merge result and leave uncertain stuff as in the common ancestor state, so that the user can fix them starting from there.

Maybe it is a good time for me to summarize the output somewhere in a document.

    O - $path	Tree-A and tree-B did not touch this; the result
                is taken from the ancestor (O for original).

    A D $path	Only tree-A (or tree-B) deleted this and the other
    B D $path   branch did not touch this; the result is to delete.

    A M $path	Only tree-A (or tree-B) modified this and the other
    B M $path   branch did not touch this; the result is to use one
                from tree-A (or tree-B).  This includes file
                creation case.

    *DD $path	Both tree-A and tree-B deleted this; the result
                is to delete.

    *DM $path   Tree-A deleted while tree-B modified this (or
    *MD $path   vice versa), and manual conflict resolution is
                needed; dircache is left as in the ancestor, and
                the modified file is saved as $path~A~ in the
                working directory.  The user can rename it to $path
                and run show-diff to see what Tree-A wanted to do
                and decide before running update-cache.

    *MM $path   Tree-A and tree-B did the exact same
                modification; the result is to use that.

    MRG $path   Tree-A and tree-B have different modifications;
                run "merge" and the merge result is left as
                $path in the working directory.

In cases other than *DM, *MD, and MRG, the result is trivial and is recorded in the dircache. Without '-o' (to be renamed ;-) nor '-f' there will not be a file checked out in the working directory for them. The three merge cases need human attention. The dircache is not touched in these cases and left as the ancestor version, and the working directory gets some file as described above.

NOTE NOTE NOTE: I am not dealing with a case where both branches create the same file but with different contents. In such a case the current code falls into MRG path without having a common ancestor, which is nonsense---I can use /dev/null as the common ancestor, I guess. Also NOTE NOTE NOTE I need to detect the case where one branch creates a directory while the other creates a file. There is nothing an automated tool can do in that case but it needs to be detected and be told the user loudly.