[PATCH] Really fix git-merge-one-file-script this time.

[PATCH] Really fix git-merge-one-file-script this time.

[Junio C Hamano]

The merge-cache program was updated to pass executable bits when
calling git-merge-one-file-script, but the called script
supplied as an example were not using them carefully.

This patch fixes the following problems in the script:

 * When a new file is created in a directory, which is a file in
   the work tree, it tried to create leading directory but did
   not check for failure from the "mkdir -p" command.

 * The script did not check the exit status from the
   git-update-cache command at all.

 * The parameter "$4" to the script is a file name that can
   contain almost any characters, so it must be quoted with
   double quotes and also needs to be preceded with -- to mark
   it as a non-option when passed to certain commands.

 * The chmod command was used with parameter "$6" or "$7" to set
   the mode bits.  This contradicts with the strategy taken by
   checkout-cache, where we honor user's umask and force only
   the executable bits.  With this patch, it creates a new file
   by redirecting into it (thus honoring user's default umask),
   and then uses "chmod +x" if we want the resulting file
   executable.  Without this fix, the merge result becomes 0644
   or 0755 for users whose umask is 002 for whom it should
   become 0664 or 0775.

 * When "$1 -> $2 -> $3" case was not handled, the script did
   not say which path it was working on, which was not so useful
   when used with the -a option of git-merge-cache.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

git-merge-one-file-script |   41 ++++++++++++++++++-----------------------
1 files changed, 18 insertions(+), 23 deletions(-)

# - [PATCH] Resurrect diff-tree-helper -R
# + [PATCH] Really fix git-merge-one-file-script this time.
--- k/git-merge-one-file-script
+++ l/git-merge-one-file-script
@@ -16,7 +16,7 @@
 # if the directory is newly added in a branch, it might not exist
 # in the current tree
 dir=$(dirname "$4")
-mkdir -p "$dir"
+mkdir -p "$dir" || exit
 
 case "${1:-.}${2:-.}${3:-.}" in
 #
@@ -30,22 +30,18 @@ case "${1:-.}${2:-.}${3:-.}" in
 # deleted in one and unchanged in the other
 #
 "$1.." | "$1.$1" | "$1$1.")
-	rm -f -- "$4"
 	echo "Removing $4"
-	git-update-cache --remove -- "$4"
-	exit 0
-	;;
-
+	rm -f -- "$4" && exec git-update-cache --remove -- "$4" ;;
 #
 # added in one
 #
 ".$2." | "..$3" )
-	echo "Adding $4 with perm $6$7"
-	mv $(git-unpack-file "$2$3") $4
-	chmod "$6$7" $4
-	git-update-cache --add -- $4
-	exit 0
-	;;
+	case "$6$7" in *7??) mode=+x;; *) mode=-x;; esac
+	echo "Adding $4 with perm $mode"
+	rm -f -- "$4" &&
+	git-cat-file blob "$2$3" >"$4" &&
+	chmod "$mode" -- "$4" &&
+	exec git-update-cache --add -- "$4" ;;
 #
 # Added in both (check for same permissions)
 #
@@ -54,11 +50,12 @@ case "${1:-.}${2:-.}${3:-.}" in
 		echo "ERROR: File $4 added in both branches, permissions conflict $6->$7"
 		exit 1
 	fi
-	echo "Adding $4 with perm $6"
-	mv $(git-unpack-file "$2") $4
-	chmod "$6" $4
-	git-update-cache --add -- $4
-	exit 0;;
+	case "$6" in *7??) mode=+x;; *) mode=-x;; esac
+	echo "Adding $4 with perm $mode"
+	rm -f -- "$4" &&
+	git-cat-file blob "$2" >"$4" &&
+	chmod "$mode" -- "$4" &&
+	exec git-update-cache --add -- "$4" ;;
 #
 # Modified in both, but differently ;(
 #
@@ -76,16 +73,14 @@ case "${1:-.}${2:-.}${3:-.}" in
 		fi
 		exit 1
 	fi
-	chmod -- "$6" "$src2"
 	if [ $ret -ne 0 ]; then
 		echo "ERROR: Leaving conflict merge in $src2"
 		exit 1
 	fi
-	cp -- "$src2" "$4" && chmod -- "$6" "$4" &&  git-update-cache --add -- "$4" && exit 0
-	;;
-
+	case "$6" in *7??) mode=+x;; *) mode=-x;; esac
+	rm -f -- "$4" && cat "$src2" >"$4" && chmod "$mode" -- "$4" &&
+	exec git-update-cache --add -- "$4" ;;
 *)
-	echo "Not handling case $1 -> $2 -> $3"
-	;;
+	echo "Not handling case $4: $1 -> $2 -> $3" ;;
 esac
 exit 1

Re: [PATCH] Really fix git-merge-one-file-script this time.

[Junio C Hamano]

Following up to my own message...

Linus, have you decided to like or dislike the behaviour of
git-merge-one-file-script touching the work tree in some cases
but not in other cases?  A straightforward merge implementation
that does a "git-read-tree -m" followed by a "git-merge-cache
git-merge-one-file-script" does the following to your work tree
and the cache:

 - Paths merged unsuccessfully makes git-merge-cache phase fail
   and the work tree is not affected for such paths.

 - Paths merged "git-read-tree -m" trivially does not change the
   work tree and "git-read-tree -m" result is kept in the cache.

 - Paths merged by "merge" successfully, and paths chosen from a
   single side by "git-merge-one-file-script" change the work
   tree, possibly checking out the file if you started out from
   an empty work tree.
 
I am not worried about the first case where you will have to
manually examine and resolve anyway. I am wondering if the rest
is the desired behavior for _your_ way of using the GIT merge.
After a successful merge, what kind of verification would you
typically do?

First of all, would you usually do the merge in an empty work
tree, or in a populated work tree?  Secondly, would you care
about the distinction between "git-read-tree -m" trivial merges
and "merge" trivial merges when reviewing the result?

If you work in an empty work tree, and never review the merge
result while in that tree, then not touching the work tree in
git-merge-one-file-script at all may be desirable, especially if
you really want to keep things only in the cache.  On the other
hand, if you do review there, leaving the merge result in the
work tree is desirable.  Especially, if you want to verify the
resulting files that are "merge" trivial but not are
"git-read-tree -m" trivial, the files you see in the work tree
are the only ones you need to check.

If you do your merge in a populated work tree, and assuming your
starting work tree matches one of the commits being merged
[*1*], it becomes harder to review the changes to the "merge"
trivial but not "git-read-tree -m" trivial files.  The cache
does not tell you which ones are which with the current
implementation of "git-merge-one-file-script".  "git-diff-cache"
against the tree before the merge would report all merges,
including "git-read-tree -m" trivial ones, so you end up needing
to save the output from git-merge-one-file-script and decide
which paths to check.

I am wondering if the following changes would make sense and
make things easier for you:

 * git-merge-one-file-script is changed to register the path
   with --cacheinfo using magic SHA1 0{40} instead of using the
   resulting file on the filesystem.  Do keep the current
   behaviour of leaving the merge results of trivial merges
   (both kind) in the work tree.

 * git-write-tree is changed to refuse to write from a cache
   that records the magic SHA1.

 * git-ls-files acquires a new option --merged to notice the
   magic SHA1 and shows the paths that have such SHA1.

 * git-update-cache acquires a new option --resolve to notice
   the magic SHA1 and:

   - if the named path is not in the work tree anymore, delete
     the entry.

   - if the named path exists in the work tree, compute the
     latest SHA1 for that file and update the entry.

Changes other than the first two listed above are purely
optional, since the Porcelain layer can implement them without
the Plumbing support.  Not doing them would keep the Plumbing
somewhat cleaner by not having to know about this magic SHA1
convention.  On the other hand, we already use that convention
in git-diff-cache, so it may even be a consistent change to make
these optional changes.  Essentially, the magic SHA1 in the
cache means "I know the user wants me to keep an eye on this
path when it matters" [*2*].

Please veto if these changes would not help _your_ use pattern.

[Footnotes]

*1* That is, you do "read-tree -m O A B" and your work tree
before the merge matches A (e.g. linux-2.6.git or your
yet-to-be-published descendant of it), B is a subsystem tree
(e.g. rmk/linux-2.6-serial.git) and O is the common ancestor.

*2* This convention would also make an implementation of "SCM
add" in the Porcelain layer a bit more efficient.  A typical
workflow without such a convention would consist of:

 * Create a file and start editing.
 * "SCM add" file, causing "git-update-cache --add -- file".
 * Do more changes, and review.
 * "SCM commit" which does"git-update-cache" changed files,
   "git-write-tree" and "git-commit-tree" to commit.

which wastes one extra blob object per "SCM add".  My gut
feeling is that more than 80% of the time "SCM add" is followed
by some edit to the added file before "SCM commit", unless it is
the initial import.  If we adopt that convention, "SCM add"
would register with --cacheinfo with the magic SHA1 without
creating the useless blob, and "SCM commit" will be written to
lazily pick things up from the work tree.

Re: [PATCH] Really fix git-merge-one-file-script this time.

[Linus Torvalds]

On Sun, 1 May 2005, Junio C Hamano wrote:
> 
> Linus, have you decided to like or dislike the behaviour of
> git-merge-one-file-script touching the work tree in some cases
> but not in other cases?  A straightforward merge implementation
> that does a "git-read-tree -m" followed by a "git-merge-cache
> git-merge-one-file-script" does the following to your work tree
> and the cache:
> 
>  - Paths merged unsuccessfully makes git-merge-cache phase fail
>    and the work tree is not affected for such paths.
> 
>  - Paths merged "git-read-tree -m" trivially does not change the
>    work tree and "git-read-tree -m" result is kept in the cache.
> 
>  - Paths merged by "merge" successfully, and paths chosen from a
>    single side by "git-merge-one-file-script" change the work
>    tree, possibly checking out the file if you started out from
>    an empty work tree.
>  
> I am not worried about the first case where you will have to
> manually examine and resolve anyway. I am wondering if the rest
> is the desired behavior for _your_ way of using the GIT merge.
> After a successful merge, what kind of verification would you
> typically do?

I don't care about the _successful_ merge, since a successful merge is 
basically always followed by a "git-checkout-cache -f -a" anyway (and 
update-cache + remove now-stale files etc).

So let's totally ignore the case of "the tree was up-to-date before, and 
the merge is successful". It's not an interesting case.

No, the reason I'd prefer to be consistent is for the _strange_ cases, 
where the merge fails. There's two of those:
 - we had local modifications that weren't checked in
 - we had a real conflict that wasn't automatically merged.

and in both of these cases we end up having to fix things up, and I
generally think that we're better off if we do _not_ update the working
tree.

In particular, the "local modifications" case is much nicer to handle if
we can just do the merge totally (and successfully) in the index, and then
handle the "local modifications" as a failure case of "git-checkout-cache"
instead.

In particular, I think the "apply the patch forward" (that cogito does) is
as wrong with the "local modifications" as it is for the merge itself, and
that a truly good merge would actually have _another_ three-way merge on
the working file - the "original" is the version in our old HEAD branch,
with the two branches being merged are "working copy before the merge" and
"merge results".

Notice? See how this _nice_ handling of the local modifications actually
meant that our merge itself should never have touched the working tree
file. We'd actually commit the merge, and then do the "checkout-cache -f
-a", adn leave the dirty files with the result of being merged with the
new (which may, of course, have a merge clash: the user sees that very
clearly from the output of "git-diff-cache").

The other case is the "real conflict" case, and that's the case where I
again don't like modifying the working tree, because I think it's a
perfectly natural thing to do to say "ok, the merge didn't work out this
way", so let's not do it at all. Again, that means that the working tree
should not have been modified, and we should _not_ have written out the
conflict file to the same file that was conflicting. We'd be much better
off if we left _all_ checked-out files in the original state instead.

So my personal preference is still that if we actually have a real 
conflict, we don't actually "consummate" the merge at all, and that very 
much means that we don't write out some partially merged state. We'd leave 
the working directory alone, and now we can fairly easily create a MERGE 
directory which has it's .git file as a symlink to ../.git, and which 
contains all the files that had conflicts in them.

Then, if you decide to not go forwared with the merge, just doing

	read-tree $(cat .git/HEAD)
	rm -rf MERGE

does exactly that. Boom, it's gone.

See? THAT is good behaviour, I think.

> I am wondering if the following changes would make sense and
> make things easier for you:
> 
>  * git-merge-one-file-script is changed to register the path
>    with --cacheinfo using magic SHA1 0{40} instead of using the
>    resulting file on the filesystem.

This sounds fine.

>					  Do keep the current
>    behaviour of leaving the merge results of trivial merges
>    (both kind) in the work tree.

I'd actually prefer not to. Exactly because it fails _both_ the "dirty
files" case _and_ the "merge didn't complete" case.

But if the "magic SHA1" meant that we look for it in a special merge 
directory, that would work.

>  * git-write-tree is changed to refuse to write from a cache
>    that records the magic SHA1.
> 
>  * git-ls-files acquires a new option --merged to notice the
>    magic SHA1 and shows the paths that have such SHA1.
> 
>  * git-update-cache acquires a new option --resolve to notice
>    the magic SHA1 and:
> 
>    - if the named path is not in the work tree anymore, delete
>      the entry.
> 
>    - if the named path exists in the work tree, compute the
>      latest SHA1 for that file and update the entry.

Sounds sane.

On the other hand, I think it would actually be easier to just make your 
"magic SHA1" be just another "stage".

		Linus

Re: [PATCH] Really fix git-merge-one-file-script this time.

[Junio C Hamano]

>>>>> "LT" == Linus Torvalds <torvalds@osdl.org> writes:

>> Linus, have you decided to like or dislike the behaviour of
>> git-merge-one-file-script touching the work tree in some cases
>> but not in other cases?

LT> I don't care about the _successful_ merge, since a successful merge is 
LT> basically always followed by a "git-checkout-cache -f -a" anyway (and 
LT> update-cache + remove now-stale files etc).

Let me make sure I understand you correctly before I go
further.  In the above sentence, do you mean a merge that is
_successful_ by merges that:

    (1) "git-remove-tree -m" considers them trivial;

or
    (2) in addition to (1), "git-merge-one-file-script"
        considers them trivial.  That is, only one-side changes
        or removes or adds it, or both sides adds it
        identically, or merge/diff3 merges without conflict.

Which one?

LT> Sounds sane.

LT> On the other hand, I think it would actually be easier to just make your 
LT> "magic SHA1" be just another "stage".

I am undecided on this one but having worked on the diff side, I
think the magic SHA1 of 0{40} would match better with what diff
needs to do.  It would mean "cache is stale, look in the work
tree" to them.

Re: [PATCH] Really fix git-merge-one-file-script this time.

[Linus Torvalds]

On Sun, 1 May 2005, Junio C Hamano wrote:
> 
> Let me make sure I understand you correctly before I go
> further.  In the above sentence, do you mean a merge that is
> _successful_ by merges that:
> 
>     (1) "git-remove-tree -m" considers them trivial;
> 
> or
>     (2) in addition to (1), "git-merge-one-file-script"
>         considers them trivial.  That is, only one-side changes
>         or removes or adds it, or both sides adds it
>         identically, or merge/diff3 merges without conflict.

(2).

But in the more general case _any_ automated merge is the "uninteresting" 
case (except for the fact that we hope for them ;)

In other words, I want this case really designed for the situation where 
automation (_any_ kind) breaks down. 

			Linus

Re: [PATCH] Really fix git-merge-one-file-script this time.

[Junio C Hamano]

>>>>> "LT" == Linus Torvalds <torvalds@osdl.org> writes:

LT> In particular, I think the "apply the patch forward" (that cogito does) is
LT> as wrong with the "local modifications" as it is for the merge itself, and
LT> that a truly good merge would actually have _another_ three-way merge on
LT> the working file - the "original" is the version in our old HEAD branch,
LT> with the two branches being merged are "working copy before the merge" and
LT> "merge results".

Funny that I realize that is exactly how I've been doing my
merges with you.