{"thread":{"id":"415","subject":"[PATCH] Really fix git-merge-one-file-script this time.","startedAt":"2005-05-01T09:29:49Z","lastAt":"2005-05-01T21:42:51Z","messageCount":6,"participants":["Junio C Hamano","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"2286","messageId":"7vd5sbz436.fsf@assigned-by-dhcp.cox.net","threadId":"415","inReplyTo":null,"subject":"[PATCH] Really fix git-merge-one-file-script this time.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-05-01T09:29:49Z","receivedAt":"2005-05-01T09:29:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The merge-cache program was updated to pass executable bits when\ncalling git-merge-one-file-script, but the called script\nsupplied as an example were not using them carefully.\n\nThis patch fixes the following problems in the script:\n\n * When a new file is created in a directory, which is a file in\n   the work tree, it tried to create leading directory but did\n   not check for failure from the \"mkdir -p\" command.\n\n * The script did not check the exit status from the\n   git-update-cache command at all.\n\n * The parameter \"$4\" to the script is a file name that can\n   contain almost any characters, so it must be quoted with\n   double quotes and also needs to be preceded with -- to mark\n   it as a non-option when passed to certain commands.\n\n * The chmod command was used with parameter \"$6\" or \"$7\" to set\n   the mode bits.  This contradicts with the strategy taken by\n   checkout-cache, where we honor user's umask and force only\n   the executable bits.  With this patch, it creates a new file\n   by redirecting into it (thus honoring user's default umask),\n   and then uses \"chmod +x\" if we want the resulting file\n   executable.  Without this fix, the merge result becomes 0644\n   or 0755 for users whose umask is 002 for whom it should\n   become 0664 or 0775.\n\n * When \"$1 -> $2 -> $3\" case was not handled, the script did\n   not say which path it was working on, which was not so useful\n   when used with the -a option of git-merge-cache.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\n\ngit-merge-one-file-script |   41 ++++++++++++++++++-----------------------\n1 files changed, 18 insertions(+), 23 deletions(-)\n\n# - [PATCH] Resurrect diff-tree-helper -R\n# + [PATCH] Really fix git-merge-one-file-script this time.\n--- k/git-merge-one-file-script\n+++ l/git-merge-one-file-script\n@@ -16,7 +16,7 @@\n # if the directory is newly added in a branch, it might not exist\n # in the current tree\n dir=$(dirname \"$4\")\n-mkdir -p \"$dir\"\n+mkdir -p \"$dir\" || exit\n \n case \"${1:-.}${2:-.}${3:-.}\" in\n #\n@@ -30,22 +30,18 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n # deleted in one and unchanged in the other\n #\n \"$1..\" | \"$1.$1\" | \"$1$1.\")\n-\trm -f -- \"$4\"\n \techo \"Removing $4\"\n-\tgit-update-cache --remove -- \"$4\"\n-\texit 0\n-\t;;\n-\n+\trm -f -- \"$4\" && exec git-update-cache --remove -- \"$4\" ;;\n #\n # added in one\n #\n \".$2.\" | \"..$3\" )\n-\techo \"Adding $4 with perm $6$7\"\n-\tmv $(git-unpack-file \"$2$3\") $4\n-\tchmod \"$6$7\" $4\n-\tgit-update-cache --add -- $4\n-\texit 0\n-\t;;\n+\tcase \"$6$7\" in *7??) mode=+x;; *) mode=-x;; esac\n+\techo \"Adding $4 with perm $mode\"\n+\trm -f -- \"$4\" &&\n+\tgit-cat-file blob \"$2$3\" >\"$4\" &&\n+\tchmod \"$mode\" -- \"$4\" &&\n+\texec git-update-cache --add -- \"$4\" ;;\n #\n # Added in both (check for same permissions)\n #\n@@ -54,11 +50,12 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \t\techo \"ERROR: File $4 added in both branches, permissions conflict $6->$7\"\n \t\texit 1\n \tfi\n-\techo \"Adding $4 with perm $6\"\n-\tmv $(git-unpack-file \"$2\") $4\n-\tchmod \"$6\" $4\n-\tgit-update-cache --add -- $4\n-\texit 0;;\n+\tcase \"$6\" in *7??) mode=+x;; *) mode=-x;; esac\n+\techo \"Adding $4 with perm $mode\"\n+\trm -f -- \"$4\" &&\n+\tgit-cat-file blob \"$2\" >\"$4\" &&\n+\tchmod \"$mode\" -- \"$4\" &&\n+\texec git-update-cache --add -- \"$4\" ;;\n #\n # Modified in both, but differently ;(\n #\n@@ -76,16 +73,14 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \t\tfi\n \t\texit 1\n \tfi\n-\tchmod -- \"$6\" \"$src2\"\n \tif [ $ret -ne 0 ]; then\n \t\techo \"ERROR: Leaving conflict merge in $src2\"\n \t\texit 1\n \tfi\n-\tcp -- \"$src2\" \"$4\" && chmod -- \"$6\" \"$4\" &&  git-update-cache --add -- \"$4\" && exit 0\n-\t;;\n-\n+\tcase \"$6\" in *7??) mode=+x;; *) mode=-x;; esac\n+\trm -f -- \"$4\" && cat \"$src2\" >\"$4\" && chmod \"$mode\" -- \"$4\" &&\n+\texec git-update-cache --add -- \"$4\" ;;\n *)\n-\techo \"Not handling case $1 -> $2 -> $3\"\n-\t;;\n+\techo \"Not handling case $4: $1 -> $2 -> $3\" ;;\n esac\n exit 1\n\n"},{"id":"2305","messageId":"7vzmveu6zs.fsf@assigned-by-dhcp.cox.net","threadId":"415","inReplyTo":"7vd5sbz436.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Really fix git-merge-one-file-script this time.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-05-01T18:38:15Z","receivedAt":"2005-05-01T18:38:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Following up to my own message...\n\nLinus, have you decided to like or dislike the behaviour of\ngit-merge-one-file-script touching the work tree in some cases\nbut not in other cases?  A straightforward merge implementation\nthat does a \"git-read-tree -m\" followed by a \"git-merge-cache\ngit-merge-one-file-script\" does the following to your work tree\nand the cache:\n\n - Paths merged unsuccessfully makes git-merge-cache phase fail\n   and the work tree is not affected for such paths.\n\n - Paths merged \"git-read-tree -m\" trivially does not change the\n   work tree and \"git-read-tree -m\" result is kept in the cache.\n\n - Paths merged by \"merge\" successfully, and paths chosen from a\n   single side by \"git-merge-one-file-script\" change the work\n   tree, possibly checking out the file if you started out from\n   an empty work tree.\n \nI am not worried about the first case where you will have to\nmanually examine and resolve anyway. I am wondering if the rest\nis the desired behavior for _your_ way of using the GIT merge.\nAfter a successful merge, what kind of verification would you\ntypically do?\n\nFirst of all, would you usually do the merge in an empty work\ntree, or in a populated work tree?  Secondly, would you care\nabout the distinction between \"git-read-tree -m\" trivial merges\nand \"merge\" trivial merges when reviewing the result?\n\nIf you work in an empty work tree, and never review the merge\nresult while in that tree, then not touching the work tree in\ngit-merge-one-file-script at all may be desirable, especially if\nyou really want to keep things only in the cache.  On the other\nhand, if you do review there, leaving the merge result in the\nwork tree is desirable.  Especially, if you want to verify the\nresulting files that are \"merge\" trivial but not are\n\"git-read-tree -m\" trivial, the files you see in the work tree\nare the only ones you need to check.\n\nIf you do your merge in a populated work tree, and assuming your\nstarting work tree matches one of the commits being merged\n[*1*], it becomes harder to review the changes to the \"merge\"\ntrivial but not \"git-read-tree -m\" trivial files.  The cache\ndoes not tell you which ones are which with the current\nimplementation of \"git-merge-one-file-script\".  \"git-diff-cache\"\nagainst the tree before the merge would report all merges,\nincluding \"git-read-tree -m\" trivial ones, so you end up needing\nto save the output from git-merge-one-file-script and decide\nwhich paths to check.\n\nI am wondering if the following changes would make sense and\nmake things easier for you:\n\n * git-merge-one-file-script is changed to register the path\n   with --cacheinfo using magic SHA1 0{40} instead of using the\n   resulting file on the filesystem.  Do keep the current\n   behaviour of leaving the merge results of trivial merges\n   (both kind) in the work tree.\n\n * git-write-tree is changed to refuse to write from a cache\n   that records the magic SHA1.\n\n * git-ls-files acquires a new option --merged to notice the\n   magic SHA1 and shows the paths that have such SHA1.\n\n * git-update-cache acquires a new option --resolve to notice\n   the magic SHA1 and:\n\n   - if the named path is not in the work tree anymore, delete\n     the entry.\n\n   - if the named path exists in the work tree, compute the\n     latest SHA1 for that file and update the entry.\n\nChanges other than the first two listed above are purely\noptional, since the Porcelain layer can implement them without\nthe Plumbing support.  Not doing them would keep the Plumbing\nsomewhat cleaner by not having to know about this magic SHA1\nconvention.  On the other hand, we already use that convention\nin git-diff-cache, so it may even be a consistent change to make\nthese optional changes.  Essentially, the magic SHA1 in the\ncache means \"I know the user wants me to keep an eye on this\npath when it matters\" [*2*].\n\nPlease veto if these changes would not help _your_ use pattern.\n\n[Footnotes]\n\n*1* That is, you do \"read-tree -m O A B\" and your work tree\nbefore the merge matches A (e.g. linux-2.6.git or your\nyet-to-be-published descendant of it), B is a subsystem tree\n(e.g. rmk/linux-2.6-serial.git) and O is the common ancestor.\n\n*2* This convention would also make an implementation of \"SCM\nadd\" in the Porcelain layer a bit more efficient.  A typical\nworkflow without such a convention would consist of:\n\n * Create a file and start editing.\n * \"SCM add\" file, causing \"git-update-cache --add -- file\".\n * Do more changes, and review.\n * \"SCM commit\" which does\"git-update-cache\" changed files,\n   \"git-write-tree\" and \"git-commit-tree\" to commit.\n\nwhich wastes one extra blob object per \"SCM add\".  My gut\nfeeling is that more than 80% of the time \"SCM add\" is followed\nby some edit to the added file before \"SCM commit\", unless it is\nthe initial import.  If we adopt that convention, \"SCM add\"\nwould register with --cacheinfo with the magic SHA1 without\ncreating the useless blob, and \"SCM commit\" will be written to\nlazily pick things up from the work tree.\n\n\n\n"},{"id":"2313","messageId":"Pine.LNX.4.58.0505011312080.2296@ppc970.osdl.org","threadId":"415","inReplyTo":"7vzmveu6zs.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Really fix git-merge-one-file-script this time.","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-05-01T20:29:59Z","receivedAt":"2005-05-01T20:29:59Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 1 May 2005, Junio C Hamano wrote:\n> \n> Linus, have you decided to like or dislike the behaviour of\n> git-merge-one-file-script touching the work tree in some cases\n> but not in other cases?  A straightforward merge implementation\n> that does a \"git-read-tree -m\" followed by a \"git-merge-cache\n> git-merge-one-file-script\" does the following to your work tree\n> and the cache:\n> \n>  - Paths merged unsuccessfully makes git-merge-cache phase fail\n>    and the work tree is not affected for such paths.\n> \n>  - Paths merged \"git-read-tree -m\" trivially does not change the\n>    work tree and \"git-read-tree -m\" result is kept in the cache.\n> \n>  - Paths merged by \"merge\" successfully, and paths chosen from a\n>    single side by \"git-merge-one-file-script\" change the work\n>    tree, possibly checking out the file if you started out from\n>    an empty work tree.\n>  \n> I am not worried about the first case where you will have to\n> manually examine and resolve anyway. I am wondering if the rest\n> is the desired behavior for _your_ way of using the GIT merge.\n> After a successful merge, what kind of verification would you\n> typically do?\n\nI don't care about the _successful_ merge, since a successful merge is \nbasically always followed by a \"git-checkout-cache -f -a\" anyway (and \nupdate-cache + remove now-stale files etc).\n\nSo let's totally ignore the case of \"the tree was up-to-date before, and \nthe merge is successful\". It's not an interesting case.\n\nNo, the reason I'd prefer to be consistent is for the _strange_ cases, \nwhere the merge fails. There's two of those:\n - we had local modifications that weren't checked in\n - we had a real conflict that wasn't automatically merged.\n\nand in both of these cases we end up having to fix things up, and I\ngenerally think that we're better off if we do _not_ update the working\ntree.\n\nIn particular, the \"local modifications\" case is much nicer to handle if\nwe can just do the merge totally (and successfully) in the index, and then\nhandle the \"local modifications\" as a failure case of \"git-checkout-cache\"\ninstead.\n\nIn particular, I think the \"apply the patch forward\" (that cogito does) is\nas wrong with the \"local modifications\" as it is for the merge itself, and\nthat a truly good merge would actually have _another_ three-way merge on\nthe working file - the \"original\" is the version in our old HEAD branch,\nwith the two branches being merged are \"working copy before the merge\" and\n\"merge results\".\n\nNotice? See how this _nice_ handling of the local modifications actually\nmeant that our merge itself should never have touched the working tree\nfile. We'd actually commit the merge, and then do the \"checkout-cache -f\n-a\", adn leave the dirty files with the result of being merged with the\nnew (which may, of course, have a merge clash: the user sees that very\nclearly from the output of \"git-diff-cache\").\n\nThe other case is the \"real conflict\" case, and that's the case where I\nagain don't like modifying the working tree, because I think it's a\nperfectly natural thing to do to say \"ok, the merge didn't work out this\nway\", so let's not do it at all. Again, that means that the working tree\nshould not have been modified, and we should _not_ have written out the\nconflict file to the same file that was conflicting. We'd be much better\noff if we left _all_ checked-out files in the original state instead.\n\nSo my personal preference is still that if we actually have a real \nconflict, we don't actually \"consummate\" the merge at all, and that very \nmuch means that we don't write out some partially merged state. We'd leave \nthe working directory alone, and now we can fairly easily create a MERGE \ndirectory which has it's .git file as a symlink to ../.git, and which \ncontains all the files that had conflicts in them.\n\nThen, if you decide to not go forwared with the merge, just doing\n\n\tread-tree $(cat .git/HEAD)\n\trm -rf MERGE\n\ndoes exactly that. Boom, it's gone.\n\nSee? THAT is good behaviour, I think.\n\n> I am wondering if the following changes would make sense and\n> make things easier for you:\n> \n>  * git-merge-one-file-script is changed to register the path\n>    with --cacheinfo using magic SHA1 0{40} instead of using the\n>    resulting file on the filesystem.\n\nThis sounds fine.\n\n>\t\t\t\t\t  Do keep the current\n>    behaviour of leaving the merge results of trivial merges\n>    (both kind) in the work tree.\n\nI'd actually prefer not to. Exactly because it fails _both_ the \"dirty\nfiles\" case _and_ the \"merge didn't complete\" case.\n\nBut if the \"magic SHA1\" meant that we look for it in a special merge \ndirectory, that would work.\n\n>  * git-write-tree is changed to refuse to write from a cache\n>    that records the magic SHA1.\n> \n>  * git-ls-files acquires a new option --merged to notice the\n>    magic SHA1 and shows the paths that have such SHA1.\n> \n>  * git-update-cache acquires a new option --resolve to notice\n>    the magic SHA1 and:\n> \n>    - if the named path is not in the work tree anymore, delete\n>      the entry.\n> \n>    - if the named path exists in the work tree, compute the\n>      latest SHA1 for that file and update the entry.\n\nSounds sane.\n\nOn the other hand, I think it would actually be easier to just make your \n\"magic SHA1\" be just another \"stage\".\n\n\t\tLinus\n"},{"id":"2318","messageId":"7vsm16sl0q.fsf@assigned-by-dhcp.cox.net","threadId":"415","inReplyTo":"Pine.LNX.4.58.0505011312080.2296@ppc970.osdl.org","subject":"Re: [PATCH] Really fix git-merge-one-file-script this time.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-05-01T21:18:13Z","receivedAt":"2005-05-01T21:18:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":">>>>> \"LT\" == Linus Torvalds <torvalds@osdl.org> writes:\n\n>> Linus, have you decided to like or dislike the behaviour of\n>> git-merge-one-file-script touching the work tree in some cases\n>> but not in other cases?\n\nLT> I don't care about the _successful_ merge, since a successful merge is \nLT> basically always followed by a \"git-checkout-cache -f -a\" anyway (and \nLT> update-cache + remove now-stale files etc).\n\nLet me make sure I understand you correctly before I go\nfurther.  In the above sentence, do you mean a merge that is\n_successful_ by merges that:\n\n    (1) \"git-remove-tree -m\" considers them trivial;\n\nor\n    (2) in addition to (1), \"git-merge-one-file-script\"\n        considers them trivial.  That is, only one-side changes\n        or removes or adds it, or both sides adds it\n        identically, or merge/diff3 merges without conflict.\n\nWhich one?\n\nLT> Sounds sane.\n\nLT> On the other hand, I think it would actually be easier to just make your \nLT> \"magic SHA1\" be just another \"stage\".\n\nI am undecided on this one but having worked on the diff side, I\nthink the magic SHA1 of 0{40} would match better with what diff\nneeds to do.  It would mean \"cache is stale, look in the work\ntree\" to them.\n\n"},{"id":"2320","messageId":"7v64y2skdp.fsf@assigned-by-dhcp.cox.net","threadId":"415","inReplyTo":"Pine.LNX.4.58.0505011312080.2296@ppc970.osdl.org","subject":"Re: [PATCH] Really fix git-merge-one-file-script this time.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-05-01T21:32:02Z","receivedAt":"2005-05-01T21:32:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":">>>>> \"LT\" == Linus Torvalds <torvalds@osdl.org> writes:\n\nLT> In particular, I think the \"apply the patch forward\" (that cogito does) is\nLT> as wrong with the \"local modifications\" as it is for the merge itself, and\nLT> that a truly good merge would actually have _another_ three-way merge on\nLT> the working file - the \"original\" is the version in our old HEAD branch,\nLT> with the two branches being merged are \"working copy before the merge\" and\nLT> \"merge results\".\n\nFunny that I realize that is exactly how I've been doing my\nmerges with you.\n\n"},{"id":"2321","messageId":"Pine.LNX.4.58.0505011441140.2296@ppc970.osdl.org","threadId":"415","inReplyTo":"7vsm16sl0q.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Really fix git-merge-one-file-script this time.","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-05-01T21:42:51Z","receivedAt":"2005-05-01T21:42:51Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 1 May 2005, Junio C Hamano wrote:\n> \n> Let me make sure I understand you correctly before I go\n> further.  In the above sentence, do you mean a merge that is\n> _successful_ by merges that:\n> \n>     (1) \"git-remove-tree -m\" considers them trivial;\n> \n> or\n>     (2) in addition to (1), \"git-merge-one-file-script\"\n>         considers them trivial.  That is, only one-side changes\n>         or removes or adds it, or both sides adds it\n>         identically, or merge/diff3 merges without conflict.\n\n(2).\n\nBut in the more general case _any_ automated merge is the \"uninteresting\" \ncase (except for the fact that we hope for them ;)\n\nIn other words, I want this case really designed for the situation where \nautomation (_any_ kind) breaks down. \n\n\t\t\tLinus\n"}]}