{"thread":{"id":"39521","subject":"[RFC] git-am: handling unborn branches","startedAt":"2015-06-04T10:34:13Z","lastAt":"2015-06-05T16:33:33Z","messageCount":8,"participants":["Paul Tan","Junio C Hamano","Stefan Beller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"262944","messageId":"CACRoPnSmF0ym7ONnLAfL=o5ouSrP2Ucxdh40k6Ps-hnTsSUx4w@mail.gmail.com","threadId":"39521","inReplyTo":null,"subject":"[RFC] git-am: handling unborn branches","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-04T10:34:13Z","receivedAt":"2015-06-04T10:34:13Z","isPatch":false,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Hi,\n\ngit-am generally supports applying patches to unborn branches.\nHowever, there are 2 cases where git-am does not handle unborn\nbranches which I would like to address before the git-am rewrite to C:\n\n1. am --skip\n\nFor git am --skip, git-am.sh does a fast-forward checkout from HEAD to\nHEAD, discarding unmerged entries, and then resets the index to HEAD\nso that the index is not dirty.\n\n        git read-tree --reset -u HEAD HEAD\n        orig_head=$(cat \"$GIT_DIR/ORIG_HEAD\")\n        git reset HEAD\n        git update-ref ORIG_HEAD $orig_head\n\nThis requires a valid HEAD. Since git-am requires an empty index for\nunborn branches in the patch application stage anyway, I think we\nshould discard all entires in the index if we are on an unborn branch?\n\nOr, the current behavior of git-am.sh will print some scary errors\nabout the missing HEAD, but will then continue on to the next patch.\nIf the index is not clean, it will then error out. Should we preserve\nthis behavior? (without the errors about the missing HEAD)\n\n2. am --abort\n\nFor git am --abort, git-am.sh does something similar. It does a\nfast-forward checkout from HEAD to ORIG_HEAD, discarding unmerged\nentries, and then resets the index to ORIG_HEAD so that local changes\nwill be unstaged.\n\n        if safe_to_abort\n        then\n            git read-tree --reset -u HEAD ORIG_HEAD\n            git reset ORIG_HEAD\n        fi\n        rm -fr \"$dotest\"\n\nThis, however, requires a valid HEAD and ORIG_HEAD. If we don't have a\nHEAD or ORIG_HEAD (because we were on an unborn branch when we started\ngit am), what should we do? (Note: safe_to_abort returns true if we\ngit am with no HEAD because $dotest/abort-safety will not exist)\nShould we discard all entires in the index as well? (Since we might\nthink of the \"original HEAD\" as an empty tree?)\n\nOr, the current behavior of git-am.sh will print some scary errors\nabout the missing HEAD and ORIG_HEAD, but will not touch the index at\nall, and still delete the rebase-apply directory. Should we preserve\nthis behavior (without the errors)?\n\nThanks,\nPaul\n"},{"id":"262966","messageId":"xmqqpp5bv0z9.fsf@gitster.dls.corp.google.com","threadId":"39521","inReplyTo":"CACRoPnSmF0ym7ONnLAfL=o5ouSrP2Ucxdh40k6Ps-hnTsSUx4w@mail.gmail.com","subject":"Re: [RFC] git-am: handling unborn branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-04T17:26:34Z","receivedAt":"2015-06-04T17:26:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan <pyokagan@gmail.com> writes:\n\n> git-am generally supports applying patches to unborn branches.\n> However, there are 2 cases where git-am does not handle unborn\n> branches which I would like to address before the git-am rewrite to C:\n\n> 1. am --skip\n>\n> For git am --skip, git-am.sh does a fast-forward checkout from HEAD to\n> HEAD, discarding unmerged entries, and then resets the index to HEAD\n> so that the index is not dirty.\n>\n>         git read-tree --reset -u HEAD HEAD\n>         orig_head=$(cat \"$GIT_DIR/ORIG_HEAD\")\n>         git reset HEAD\n>         git update-ref ORIG_HEAD $orig_head\n>\n> This requires a valid HEAD. Since git-am requires an empty index for\n> unborn branches in the patch application stage anyway, I think we\n> should discard all entires in the index if we are on an unborn branch?\n\nYes, and it should also remove the new files the failed application\nbrought in to the working tree, if any, to match the \"--skip\" done\nin the normal case (i.e. when we already have a history to apply\npatches to), I would think.\n\n> 2. am --abort\n>\n> For git am --abort, git-am.sh does something similar. It does a\n> fast-forward checkout from HEAD to ORIG_HEAD, discarding unmerged\n> entries, and then resets the index to ORIG_HEAD so that local changes\n> will be unstaged.\n\nIn general, the \"apply to nothing\" is more or less an afterthought\nand was not done as carefully as the rest of the program, so view\nwhenever you see a strange behaviour as not a \"strange spec\" but\nlikely to be a bug.  You would do OK if you imagine what should\nhappen if you were doing the same operation on top of a commit that\nrecords an empty tree and try to match the behaviour to that case.\n\nThanks.\n"},{"id":"262967","messageId":"CAGZ79kbvWjoCx0VFJS+B-mvxE62_PaRixMjUSWXwsO-c+LonVQ@mail.gmail.com","threadId":"39521","inReplyTo":"CACRoPnSmF0ym7ONnLAfL=o5ouSrP2Ucxdh40k6Ps-hnTsSUx4w@mail.gmail.com","subject":"Re: [RFC] git-am: handling unborn branches","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-06-04T17:27:17Z","receivedAt":"2015-06-04T17:27:17Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jun 4, 2015 at 3:34 AM, Paul Tan <pyokagan@gmail.com> wrote:\n> Hi,\n>\n> git-am generally supports applying patches to unborn branches.\n> However, there are 2 cases where git-am does not handle unborn\n> branches which I would like to address before the git-am rewrite to C:\n>\n> 1. am --skip\n>\n> For git am --skip, git-am.sh does a fast-forward checkout from HEAD to\n> HEAD, discarding unmerged entries, and then resets the index to HEAD\n> so that the index is not dirty.\n>\n>         git read-tree --reset -u HEAD HEAD\n>         orig_head=$(cat \"$GIT_DIR/ORIG_HEAD\")\n>         git reset HEAD\n>         git update-ref ORIG_HEAD $orig_head\n>\n> This requires a valid HEAD. Since git-am requires an empty index for\n> unborn branches in the patch application stage anyway, I think we\n> should discard all entires in the index if we are on an unborn branch?\n\nThat makes sense.\n\n>\n> Or, the current behavior of git-am.sh will print some scary errors\n> about the missing HEAD, but will then continue on to the next patch.\n> If the index is not clean, it will then error out. Should we preserve\n> this behavior? (without the errors about the missing HEAD)\n>\n> 2. am --abort\n>\n> For git am --abort, git-am.sh does something similar. It does a\n> fast-forward checkout from HEAD to ORIG_HEAD, discarding unmerged\n> entries, and then resets the index to ORIG_HEAD so that local changes\n> will be unstaged.\n>\n>         if safe_to_abort\n>         then\n>             git read-tree --reset -u HEAD ORIG_HEAD\n>             git reset ORIG_HEAD\n>         fi\n>         rm -fr \"$dotest\"\n>\n> This, however, requires a valid HEAD and ORIG_HEAD. If we don't have a\n> HEAD or ORIG_HEAD (because we were on an unborn branch when we started\n> git am), what should we do? (Note: safe_to_abort returns true if we\n> git am with no HEAD because $dotest/abort-safety will not exist)\n> Should we discard all entires in the index as well? (Since we might\n> think of the \"original HEAD\" as an empty tree?)\n>\n> Or, the current behavior of git-am.sh will print some scary errors\n> about the missing HEAD and ORIG_HEAD, but will not touch the index at\n> all, and still delete the rebase-apply directory. Should we preserve\n> this behavior (without the errors)?\n\nI guess so, looking at the documentation\n       --abort\n           Restore the original branch and abort the patching operation.\n\na user may want to not go to the unborn branch, but rather to the previous\nHEAD?\n\n\n>\n> Thanks,\n> Paul\n"},{"id":"262993","messageId":"CACRoPnS9UK0ebD2JdSfun8+F0MXj6Loza4Y07whqbO6tV4_APQ@mail.gmail.com","threadId":"39521","inReplyTo":"xmqqpp5bv0z9.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC] git-am: handling unborn branches","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-05T06:37:24Z","receivedAt":"2015-06-05T06:37:24Z","isPatch":false,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Fri, Jun 5, 2015 at 1:26 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Paul Tan <pyokagan@gmail.com> writes:\n>\n>> git-am generally supports applying patches to unborn branches.\n>> However, there are 2 cases where git-am does not handle unborn\n>> branches which I would like to address before the git-am rewrite to C:\n>\n>> 1. am --skip\n>>\n>> For git am --skip, git-am.sh does a fast-forward checkout from HEAD to\n>> HEAD, discarding unmerged entries, and then resets the index to HEAD\n>> so that the index is not dirty.\n>>\n>>         git read-tree --reset -u HEAD HEAD\n>>         orig_head=$(cat \"$GIT_DIR/ORIG_HEAD\")\n>>         git reset HEAD\n>>         git update-ref ORIG_HEAD $orig_head\n>>\n>> This requires a valid HEAD. Since git-am requires an empty index for\n>> unborn branches in the patch application stage anyway, I think we\n>> should discard all entires in the index if we are on an unborn branch?\n>\n> Yes, and it should also remove the new files the failed application\n> brought in to the working tree, if any, to match the \"--skip\" done\n> in the normal case (i.e. when we already have a history to apply\n> patches to), I would think.\n\nHmm, actually git-am.sh doesn't seem to do that even when we have a\nhistory to apply patches to. This is okay in the non-3way case, as\ngit-apply will check to see if the patch applies before it modifies\nthe index, but if we fall back on 3-way merge, any new files the\nfailed merge added will not be deleted in the \"git read-tree --reset\n-u HEAD HEAD\".\n\nShould we do that? I dunno, but if we want to introduce this feature,\nI think a \"git read-tree --reset HEAD HEAD && git read-tree -m -u\n$(git write-tree) HEAD\" will do the trick? (We discard all unmerged\nentries, then fast-forward from the tree the failed merged left us\nwith back to HEAD).\n\nClearing the index in the unborn branch case seems to be the most\nconsistent thing to do for now (for the purpose of the git-am\nrewrite).\n\nThanks,\nPaul\n"},{"id":"262997","messageId":"CACRoPnTf+4_=B7CWY_XBOaK0vWXit9tDf2pbO-T=q7JoWjhtLA@mail.gmail.com","threadId":"39521","inReplyTo":"CAGZ79kbvWjoCx0VFJS+B-mvxE62_PaRixMjUSWXwsO-c+LonVQ@mail.gmail.com","subject":"Re: [RFC] git-am: handling unborn branches","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-05T08:32:18Z","receivedAt":"2015-06-05T08:32:18Z","isPatch":false,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Fri, Jun 5, 2015 at 1:27 AM, Stefan Beller <sbeller@google.com> wrote:\n> On Thu, Jun 4, 2015 at 3:34 AM, Paul Tan <pyokagan@gmail.com> wrote:\n>> Or, the current behavior of git-am.sh will print some scary errors\n>> about the missing HEAD, but will then continue on to the next patch.\n>> If the index is not clean, it will then error out. Should we preserve\n>> this behavior? (without the errors about the missing HEAD)\n>>\n>> 2. am --abort\n>>\n>> For git am --abort, git-am.sh does something similar. It does a\n>> fast-forward checkout from HEAD to ORIG_HEAD, discarding unmerged\n>> entries, and then resets the index to ORIG_HEAD so that local changes\n>> will be unstaged.\n>>\n>>         if safe_to_abort\n>>         then\n>>             git read-tree --reset -u HEAD ORIG_HEAD\n>>             git reset ORIG_HEAD\n>>         fi\n>>         rm -fr \"$dotest\"\n>>\n>> This, however, requires a valid HEAD and ORIG_HEAD. If we don't have a\n>> HEAD or ORIG_HEAD (because we were on an unborn branch when we started\n>> git am), what should we do? (Note: safe_to_abort returns true if we\n>> git am with no HEAD because $dotest/abort-safety will not exist)\n>> Should we discard all entires in the index as well? (Since we might\n>> think of the \"original HEAD\" as an empty tree?)\n>>\n>> Or, the current behavior of git-am.sh will print some scary errors\n>> about the missing HEAD and ORIG_HEAD, but will not touch the index at\n>> all, and still delete the rebase-apply directory. Should we preserve\n>> this behavior (without the errors)?\n>\n> I guess so, looking at the documentation\n>        --abort\n>            Restore the original branch and abort the patching operation.\n>\n> a user may want to not go to the unborn branch, but rather to the previous\n> HEAD?\n\nI think we need to be consistent with the non-unborn-branch behavior\nof git-am. In normal use am --abort would reset the branch back to\nORIG_HEAD, which is the HEAD when am was first run, thus losing all\nthe applied commits. However, since f79d4c8 (git-am: teach git-am to\napply a patch to an unborn branch, 2009-04-10) if git-am is run\nwithout a HEAD, no ORIG_HEAD would be created.\n\nI guess if we are to follow this normal behavior, we need to destroy\nthe current branch as well.\n\nSo the abort logic would be something like:\n\n1. If we are still on an unborn branch, we clear the index.\n\n2. If we are not on an unborn branch, but we have no ORIG_HEAD because\nwe started from an unborn branch, then we destroy the current branch.\n\n3. If we are not on an unborn branch, and we have an ORIG_HEAD, then\nwe do the normal behavior of fast-forwarding and resetting the index\nto ORIG_HEAD.\n\nWe also need to consider safe_to_abort() as well though, which was\nintroduced in 7b3b7e3 (am --abort: keep unrelated commits since the\nlast failure and warn, 2010-12-21). Specifically,\n\nsafe_to_abort () {\n...\n    if ! test -s \"$dotest/abort-safety\"\n    then\n        return 0\n    fi\n...\n}\n\nwhere we are allowed to reset HEAD if the abort-safety file is empty\nor does not exist. If patch application failed while we are on an\nunborn branch, we will have no abort-safety file. However, the user\nmay have made commits since then, and we should not make the user lose\nthose commits. As such, we should probably not reset HEAD if there is\nno abort-safety file.\n\nThanks,\nPaul\n"},{"id":"263022","messageId":"xmqqfv66tbei.fsf@gitster.dls.corp.google.com","threadId":"39521","inReplyTo":"CACRoPnS9UK0ebD2JdSfun8+F0MXj6Loza4Y07whqbO6tV4_APQ@mail.gmail.com","subject":"Re: [RFC] git-am: handling unborn branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-05T15:36:37Z","receivedAt":"2015-06-05T15:36:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan <pyokagan@gmail.com> writes:\n\n> Hmm, actually git-am.sh doesn't seem to do that even when we have a\n> history to apply patches to. This is okay in the non-3way case, as\n> git-apply will check to see if the patch applies before it modifies\n> the index, but if we fall back on 3-way merge, any new files the\n> failed merge added will not be deleted in the \"git read-tree --reset\n> -u HEAD HEAD\".\n>\n> Should we do that?\n\nThat sounds like the right thing to do; I agree that fixing it may\nor may not be outside the scope of the immediate series.\n"},{"id":"263029","messageId":"CACRoPnTjqWrhS0UArSevjp6kUbQL8YYEvvobmykssoo05wjbow@mail.gmail.com","threadId":"39521","inReplyTo":"xmqqfv66tbei.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC] git-am: handling unborn branches","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-05T16:26:43Z","receivedAt":"2015-06-05T16:26:43Z","isPatch":false,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Fri, Jun 5, 2015 at 11:36 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Paul Tan <pyokagan@gmail.com> writes:\n>\n>> Hmm, actually git-am.sh doesn't seem to do that even when we have a\n>> history to apply patches to. This is okay in the non-3way case, as\n>> git-apply will check to see if the patch applies before it modifies\n>> the index, but if we fall back on 3-way merge, any new files the\n>> failed merge added will not be deleted in the \"git read-tree --reset\n>> -u HEAD HEAD\".\n>>\n>> Should we do that?\n>\n> That sounds like the right thing to do; I agree that fixing it may\n> or may not be outside the scope of the immediate series.\n\nHmm, thinking about it, the equivalent C code would be greatly\naffected by whatever behavior we go with, so I think we should try\nfixing the behavior first.\n\nThis was done really quickly, but I think this may fix it:\n\ndiff --git a/git-am.sh b/git-am.sh\nindex 761befb..f7d54bf 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -502,7 +502,9 @@ then\n         ;;\n     t,)\n         git rerere clear\n-        git read-tree --reset -u HEAD HEAD\n+        git read-tree --reset HEAD HEAD &&\n+        our_tree=$(git write-tree) &&\n+        git read-tree -m -u $our_tree HEAD\n         orig_head=$(cat \"$GIT_DIR/ORIG_HEAD\")\n         git reset HEAD\n         git update-ref ORIG_HEAD $orig_head\ndiff --git a/t/t4153-am-clean-index.sh b/t/t4153-am-clean-index.sh\nnew file mode 100755\nindex 0000000..6d696db\n--- /dev/null\n+++ b/t/t4153-am-clean-index.sh\n@@ -0,0 +1,30 @@\n+#!/bin/sh\n+\n+test_description='test clean index with am'\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+    test_commit initial file &&\n+    test_commit master-commit file &&\n+    git checkout -b conflict master^ &&\n+    echo conflict-commit >file &&\n+    echo newfile >newfile &&\n+    git add newfile &&\n+    test_tick &&\n+    git commit -a -m conflict-commit &&\n+    git format-patch --stdout initial >conflict.patch &&\n+    git checkout master\n+'\n+\n+test_expect_success 'am -3 pauses on conflict' '\n+    test_must_fail git am -3 conflict.patch &&\n+    echo newfile >expected &&\n+    test_cmp newfile expected\n+'\n+\n+test_expect_success 'am --skip removes newfile' '\n+    git am --skip &&\n+    test_path_is_missing newfile\n+'\n+\n+test_done\n\nHowever, it assumes that the contents of the index are from the failed\nmerge. If the user modified the index before running git-am --skip,\ne.g. the user added a file, then that file would be deleted, which may\nnot be desired...\n\nThanks,\nPaul\n"},{"id":"263031","messageId":"xmqq3826t8rm.fsf@gitster.dls.corp.google.com","threadId":"39521","inReplyTo":"CACRoPnTjqWrhS0UArSevjp6kUbQL8YYEvvobmykssoo05wjbow@mail.gmail.com","subject":"Re: [RFC] git-am: handling unborn branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-05T16:33:33Z","receivedAt":"2015-06-05T16:33:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan <pyokagan@gmail.com> writes:\n\n> Hmm, thinking about it, the equivalent C code would be greatly\n> affected by whatever behavior we go with, so I think we should try\n> fixing the behavior first.\n\nI am glad to see that sometimes people see the light when I say\none of the greatest strength in scripted Porcelains is that they are\nfar easier to modify than those writtein in C ;-)\n\n> This was done really quickly, but I think this may fix it:\n> ...\n> However, it assumes that the contents of the index are from the failed\n> merge. If the user modified the index before running git-am --skip,\n> e.g. the user added a file, then that file would be deleted, which may\n> not be desired...\n\nI do not think that is worth worrying about; if users made changes\nunrelated to what \"git am\" asked them to help it apply by modifying\nthe working tree and updating the index, don't they \"lose\" their\nchanges the same way anyway, whether it is a new file or an existing\none?\n"}]}