# [PATCH] Really fix git-merge-one-file-script this time. 6 messages from 2005-05-01 to 2005-05-01. Participants: Junio C Hamano, Linus Torvalds. Thread: https://gitlist.dev/t/415 ## Junio C Hamano, 2005-05-01 09:29 Subject: [PATCH] Really fix git-merge-one-file-script this time. Message-ID: <7vd5sbz436.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vd5sbz436.fsf%40assigned-by-dhcp.cox.net ``` 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 --- 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 ``` ## Junio C Hamano, 2005-05-01 18:38 Subject: Re: [PATCH] Really fix git-merge-one-file-script this time. Message-ID: <7vzmveu6zs.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vzmveu6zs.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <7vd5sbz436.fsf@assigned-by-dhcp.cox.net> ``` 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. ``` ## Linus Torvalds, 2005-05-01 20:29 Subject: Re: [PATCH] Really fix git-merge-one-file-script this time. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.58.0505011312080.2296%40ppc970.osdl.org In-Reply-To: <7vzmveu6zs.fsf@assigned-by-dhcp.cox.net> ``` 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 ``` ## Junio C Hamano, 2005-05-01 21:18 Subject: Re: [PATCH] Really fix git-merge-one-file-script this time. Message-ID: <7vsm16sl0q.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vsm16sl0q.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` >>>>> "LT" == Linus Torvalds 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. ``` ## Junio C Hamano, 2005-05-01 21:32 Subject: Re: [PATCH] Really fix git-merge-one-file-script this time. Message-ID: <7v64y2skdp.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v64y2skdp.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` >>>>> "LT" == Linus Torvalds 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. ``` ## Linus Torvalds, 2005-05-01 21:42 Subject: Re: [PATCH] Really fix git-merge-one-file-script this time. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.58.0505011441140.2296%40ppc970.osdl.org In-Reply-To: <7vsm16sl0q.fsf@assigned-by-dhcp.cox.net> ``` 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 ```