{"thread":{"id":"55429","subject":"[PATCH 0/1] git-apply: Allow simultaneous --cached and --3way options","startedAt":"2021-04-03T01:34:34Z","lastAt":"2021-04-12T18:27:16Z","messageCount":29,"participants":["Jerry Zhang","Elijah Newren","Junio C Hamano","Bagas Sanjaya"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"420913","messageId":"20210403013410.32064-1-jerry@skydio.com","threadId":"55429","inReplyTo":null,"subject":"[PATCH 0/1] git-apply: Allow simultaneous --cached and --3way options","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-04-03T01:34:09Z","receivedAt":"2021-04-03T01:34:34Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"I'm creating a script/tool that will be able to cherry-pick \nmultiple commits from a single branch, rebase them onto a\nbase commit, and push those references to a remote. \n\nEx. with a branch like \"origin/master -> A -> B -> C\"\nThe tool will create \"master -> A\", \"master -> B\",\n\"master -> C\" and either make local branches or\npush them to a remote. This can be useful since code\nreview tools like github use branches as the basis\nfor pull requests.\n\nA key feature here is that the above happens without\nany changes to the user's working directory or cache.\nThis is important since those operations will add\ntime and generate build churn. We use these steps\nfor synthesizing a \"cherry-pick\" of B to master.\n\n1. cp .git/index index.temp\n2. set GIT_INDEX_FILE=index.temp\n3. git reset master -- . (git read-tree also works here, but is a bit slower)\n4. git format-patch --full-index B~..B\n5. git apply --cached B.patch\n6. git write-tree\n7. git commit-tree {output of 6} -p master -m \"message\"\n8. either `git symbolic-ref` to make a branch or `git push` to remote\n\nI'm looking to improve the git apply step in #5. \nCurrently we can't use --cached in combination with\n--3way, which limits some of the usefulness of this method.\nThere are many diffs that will block applying a patch\nthat a 3 way merge can resolve without conflicts. Even\nin the case where there are real conflicts, performing\na 3 way merge will allow us to show the user the lines\nwhere the conflict occurred. \n\nWith the above in mind, I've created a small patch that\nimplements the behavior I'd like. Rather than disallow\nthe cached and 3way flags to be combined, we allow them,\nbut write any conflicts directly to the cached file. Since \nwe're unable to modify the working directory, it seems\nreasonable in this case to not actually present the user\nwith any options to resolve conflicts. Instead, a script\nor tool using this command can diff the temporary cache\nto get the source of the conflict.\n\nHappy to address any feedback. After I address any major\nchanges I will add new tests for this path.\n\nAll tests passed locally.\n\nJerry Zhang (1):\n git-apply: Allow simultaneous --cached and --3way options\n\n Documentation/git-apply.txt | 4 +++-\n apply.c | 13 +++++++------\n 2 files changed, 10 insertions(+), 7 deletions(-)\n\n-- \n2.29.0\n\n"},{"id":"420914","messageId":"20210403013410.32064-2-jerry@skydio.com","threadId":"55429","inReplyTo":"20210403013410.32064-1-jerry@skydio.com","subject":"[PATCH 1/1] git-apply: Allow simultaneous --cached and --3way options","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-04-03T01:34:10Z","receivedAt":"2021-04-03T01:34:35Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"Previously, --cached and --3way were not\nallowed to be used together, since --3way\nwrote conflict markers into the working tree.\n\nThese changes change semantics so that if\nthese flags are given together and there is\na conflict, the conflict markers are added\ndirectly to cache. If there is no conflict,\nthe patch is applied directly to cache as\nexpected.\n\nSigned-off-by: Jerry Zhang \nSigned-off-by: Jerry Zhang \n---\n Documentation/git-apply.txt | 4 +++-\n apply.c | 13 +++++++------\n 2 files changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt\nindex 91d9a8601c..3dc0085066 100644\n--- a/Documentation/git-apply.txt\n+++ b/Documentation/git-apply.txt\n@@ -89,7 +89,9 @@ OPTIONS\n \tand we have those blobs available locally, possibly leaving the\n \tconflict markers in the files in the working tree for the user to\n \tresolve. This option implies the `--index` option, and is incompatible\n-\twith the `--reject` and the `--cached` options.\n+\twith the `--reject` option. When used with the --cached option, any\n+\tconflict markers are added directly to the cache rather than the\n+\tworking tree.\n \n --build-fake-ancestor=::\n \tNewer 'git diff' output has embedded 'index information'\ndiff --git a/apply.c b/apply.c\nindex 6695a931e9..fc94ca0e99 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -133,8 +133,6 @@ int check_apply_state(struct apply_state *state, int force_apply)\n \n \tif (state->apply_with_reject && state->threeway)\n \t\treturn error(_(\"--reject and --3way cannot be used together.\"));\n-\tif (state->cached && state->threeway)\n-\t\treturn error(_(\"--cached and --3way cannot be used together.\"));\n \tif (state->threeway) {\n \t\tif (is_not_gitdir)\n \t\t\treturn error(_(\"--3way outside a repository\"));\n@@ -4490,13 +4488,16 @@ static int create_file(struct apply_state *state, struct patch *patch)\n \n \tif (!mode)\n \t\tmode = S_IFREG | 0644;\n-\tif (create_one_file(state, path, mode, buf, size))\n-\t\treturn -1;\n+\tif (!state->cached) {\n+\t\tif (create_one_file(state, path, mode, buf, size))\n+\t\t\treturn -1;\n+\t}\n \n-\tif (patch->conflicted_threeway)\n+\tif (patch->conflicted_threeway && !state->cached)\n \t\treturn add_conflicted_stages_file(state, patch);\n-\telse if (state->update_index)\n+\telse if (state->update_index) {\n \t\treturn add_index_file(state, path, mode, buf, size);\n+\t}\n \treturn 0;\n }\n \n-- \n2.29.0\n\n"},{"id":"420915","messageId":"CABPp-BGSgyAH0w21Vrv_bdPaLg+rCPViktbUmM6fMbmxaK70qA@mail.gmail.com","threadId":"55429","inReplyTo":"20210403013410.32064-1-jerry@skydio.com","subject":"Re: [PATCH 0/1] git-apply: Allow simultaneous --cached and --3way options","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-04-03T03:04:44Z","receivedAt":"2021-04-03T03:04:58Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Apr 2, 2021 at 6:36 PM Jerry Zhang wrote:\n>\n> I'm creating a script/tool that will be able to cherry-pick\n> multiple commits from a single branch, rebase them onto a\n> base commit, and push those references to a remote.\n>\n> Ex. with a branch like \"origin/master -> A -> B -> C\"\n> The tool will create \"master -> A\", \"master -> B\",\n> \"master -> C\" and either make local branches or\n> push them to a remote. This can be useful since code\n> review tools like github use branches as the basis\n> for pull requests.\n\nNot sure I understand the \"master -> A\", \"master -> B\" syntax. What\ndo you mean here?\n\n> A key feature here is that the above happens without\n> any changes to the user's working directory or cache.\n> This is important since those operations will add\n> time and generate build churn. We use these steps\n> for synthesizing a \"cherry-pick\" of B to master.\n>\n> 1. cp .git/index index.temp\n> 2. set GIT_INDEX_FILE=index.temp\n> 3. git reset master -- . (git read-tree also works here, but is a bit slower)\n> 4. git format-patch --full-index B~..B\n> 5. git apply --cached B.patch\n> 6. git write-tree\n> 7. git commit-tree {output of 6} -p master -m \"message\"\n> 8. either `git symbolic-ref` to make a branch or `git push` to remote\n\nYeah, folks have resorted to various variants of this kind of thing in\nthe past. It is a clever way to handle some basic cases, but it does\noften fall short. It's unfortunate that cherry-pick and rebase cannot\nyet just provide this functionality (more on that below).\n\nIt may also interest you that rebase has two different backends, one\nbuilt on am (which in turn is built on format-patch + apply), and one\nbuilt on the merge machinery (which the am --3way also uses when it\nneeds to). We deprecated the format-patch + apply backend in part\nbecause it sometimes results in misapplied patches; see the \"Context\"\nsubsection of the \"BEHAVIORAL DIFFERENCES\" section of the git-rebase\nmanpage. However, the am version would at least handle basic renames,\nwhich I believe might cause problems for a direct format-patch + apply\ninvocation like yours (I'll also discuss this more below).\n\n> I'm looking to improve the git apply step in #5.\n> Currently we can't use --cached in combination with\n> --3way, which limits some of the usefulness of this method.\n> There are many diffs that will block applying a patch\n> that a 3 way merge can resolve without conflicts. Even\n> in the case where there are real conflicts, performing\n> a 3 way merge will allow us to show the user the lines\n> where the conflict occurred.\n>\n> With the above in mind, I've created a small patch that\n> implements the behavior I'd like. Rather than disallow\n> the cached and 3way flags to be combined, we allow them,\n> but write any conflicts directly to the cached file. Since\n> we're unable to modify the working directory, it seems\n> reasonable in this case to not actually present the user\n> with any options to resolve conflicts. Instead, a script\n> or tool using this command can diff the temporary cache\n> to get the source of the conflict.\n\nLooks like you're focusing on content conflicts. What about path\nconflicts? For example, apply's --3way just uses a per-file\nll_merge() call, meaning it won't handle renames, so your method would\nalso often get spurious modify/delete conflicts when renames occur.\nHow does your plan to just \"cache\" conflicts work with these\nmodify/delete files? Will users just look for conflict markers and\nignore the fact that both modified newfile and modified oldfile are\npresent? I'm also curious how e.g. directory/file conflicts would be\nhandled by your scheme; these seem somewhat problematic to me from\nyour description.\n\n> Happy to address any feedback. After I address any major\n> changes I will add new tests for this path.\n\nDon't know the timeframe you're looking at, but I'm looking to modify\ncherry-pick and rebase to be able to operate on branches that are not\nchecked out, or in bare repositories. The blocker to that\ntraditionally has been that the merge machinery required a working\ndirectory. The good news is that I wrote a new merge backend that\ndoesn't require a working directory. The bad news is I'm still trying\nto get that new merge backend through the review process, and no\ncurrent release of git has a complete version of that new backend yet.\nFurther, the changes to cherry-pick and rebase have not yet been\nstarted. There were some decisions to make too, such as how to handle\nthe case with conflicts -- just report them and tell the user to retry\nwith a checkout? Provide some kind of basic information about the\nconflicts? What'd be useful to you?\n"},{"id":"420917","messageId":"CABPp-BGhvQF9k1Jw9NPbZWMkNSffqR777-4S-y-Sh=Etvw-SAA@mail.gmail.com","threadId":"55429","inReplyTo":"20210403013410.32064-2-jerry@skydio.com","subject":"Re: [PATCH 1/1] git-apply: Allow simultaneous --cached and --3way options","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-04-03T03:46:44Z","receivedAt":"2021-04-03T03:46:59Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"I'm not that familiar with apply.c, but let me attempt to take a look...\n\nOn Fri, Apr 2, 2021 at 6:36 PM Jerry Zhang wrote:\n>\n> Previously, --cached and --3way were not\n> allowed to be used together, since --3way\n> wrote conflict markers into the working tree.\n>\n> These changes change semantics so that if\n> these flags are given together and there is\n> a conflict, the conflict markers are added\n> directly to cache. If there is no conflict,\n> the patch is applied directly to cache as\n> expected.\n>\n> Signed-off-by: Jerry Zhang \n> Signed-off-by: Jerry Zhang \n> ---\n> Documentation/git-apply.txt | 4 +++-\n> apply.c | 13 +++++++------\n> 2 files changed, 10 insertions(+), 7 deletions(-)\n>\n> diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt\n> index 91d9a8601c..3dc0085066 100644\n> --- a/Documentation/git-apply.txt\n> +++ b/Documentation/git-apply.txt\n> @@ -89,7 +89,9 @@ OPTIONS\n> and we have those blobs available locally, possibly leaving the\n> conflict markers in the files in the working tree for the user to\n> resolve. This option implies the `--index` option, and is incompatible\n> - with the `--reject` and the `--cached` options.\n> + with the `--reject` option. When used with the --cached option, any\n> + conflict markers are added directly to the cache rather than the\n> + working tree.\n>\n> --build-fake-ancestor=::\n> Newer 'git diff' output has embedded 'index information'\n> diff --git a/apply.c b/apply.c\n> index 6695a931e9..fc94ca0e99 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -133,8 +133,6 @@ int check_apply_state(struct apply_state *state, int force_apply)\n>\n> if (state->apply_with_reject && state->threeway)\n> return error(_(\"--reject and --3way cannot be used together.\"));\n> - if (state->cached && state->threeway)\n> - return error(_(\"--cached and --3way cannot be used together.\"));\n> if (state->threeway) {\n> if (is_not_gitdir)\n> return error(_(\"--3way outside a repository\"));\n> @@ -4490,13 +4488,16 @@ static int create_file(struct apply_state *state, struct patch *patch)\n>\n> if (!mode)\n> mode = S_IFREG | 0644;\n> - if (create_one_file(state, path, mode, buf, size))\n> - return -1;\n> + if (!state->cached) {\n\nWhy add this check? create_one_file() already has an early return if\nstate->cached is true.\n\n> + if (create_one_file(state, path, mode, buf, size))\n> + return -1;\n> + }\n>\n> - if (patch->conflicted_threeway)\n> + if (patch->conflicted_threeway && !state->cached)\n> return add_conflicted_stages_file(state, patch);\n> - else if (state->update_index)\n> + else if (state->update_index) {\n> return add_index_file(state, path, mode, buf, size);\n\nSo if something had conflicts, you ignore the various conflicted\nmodes, and just add it to the index as it stands. What if it was\ndeleted upstream and modified locally? Doesn't that just ignore the\nconflict, make it invisible to the user, and add the locally modified\nversion? Similarly if it was renamed upstream and modified locally,\ndoesn't that end up in both files being present? And if there's a\ndirectory/file conflict, due to the lack of ADD_CACHE_SKIP_DFCHECK (or\nwhatever it's called), the add is just going to fail, but perhaps\nthat's the most reasonable case as it'd print an error message and\nreturn -1, I think.\n\nAgain, I didn't test any of this out and I'm not so familiar with this\ncode, so I'm guessing at these scenarios. If I'm wrong about how this\nworks, the commit message probably deserves an explanation about why\nthey work, and we'd definitely need a few testcases for these types of\nscenarios. If I'm right, the current implementation is problematic at\nleast if not the idea of using these options together.\n"},{"id":"420918","messageId":"xmqqy2e00zaf.fsf@gitster.g","threadId":"55429","inReplyTo":"CABPp-BGhvQF9k1Jw9NPbZWMkNSffqR777-4S-y-Sh=Etvw-SAA@mail.gmail.com","subject":"Re: [PATCH 1/1] git-apply: Allow simultaneous --cached and --3way options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-03T04:26:00Z","receivedAt":"2021-04-03T04:26:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> I'm not that familiar with apply.c, but let me attempt to take a look...\n\nI am (well, at least I was the one who invented 3way and added the\n\"index\" line to the diff output format) ;-)\n\n> scenarios. If I'm right, the current implementation is problematic at\n> least if not the idea of using these options together.\n\nYes, the conflicted case cannot sanely be handled _without_ leaving\nhigher stage entries in the index, and that is exactly why I made it\nincompatible with the \"--cached\" mode.\n\nIt might be OK to only allow the combination when everything auto\nresolves cleanly and fail the operation without touching either the\nindex or the working tree. Pretending there was no delete/modify\nconflicts or adding contents with unresolved conflicts as if nothing\nbad happened as stage 0 entries would never be acceptable.\n\nPerhaps\n\n * Error out if the index does not match HEAD.\n\n * Try applying to the contents in the index. If there are any\n structural conflicts, leave these paths at higher stage and do\n not touch their contents.\n\n * For paths without structural conflict but need content merges,\n attempt ll-merge of the contents. If autoresolves cleanly,\n register the result at stage 0. Otherwise, discard the failed\n conflicted merge, and leave stages 1, 2 and 3 as they are.\n\n * Exit with status 0 if and only if everything has resolved\n cleanly. Otherwise, exit with non-zero status.\n\nwould be the minimally-acceptably-safe behaviour.\n\n\n"},{"id":"420923","messageId":"0803a702-ae9c-da9a-c168-d534fe2aab58@gmail.com","threadId":"55429","inReplyTo":"20210403013410.32064-1-jerry@skydio.com","subject":"Re: [PATCH 0/1] git-apply: Allow simultaneous --cached and --3way options","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-04-03T05:24:12Z","receivedAt":"2021-04-03T05:25:08Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On 03/04/21 08.34, Jerry Zhang wrote:\n> I'm creating a script/tool that will be able to cherry-pick\n> multiple commits from a single branch, rebase them onto a\n> base commit, and push those references to a remote.\n> \n> Ex. with a branch like \"origin/master -> A -> B -> C\"\n> The tool will create \"master -> A\", \"master -> B\",\n> \"master -> C\" and either make local branches or\n> push them to a remote. This can be useful since code\n> review tools like github use branches as the basis\n> for pull requests.\n> \n> A key feature here is that the above happens without\n> any changes to the user's working directory or cache.\n> This is important since those operations will add\n> time and generate build churn. We use these steps\n> for synthesizing a \"cherry-pick\" of B to master.\n> \n> 1. cp .git/index index.temp\n> 2. set GIT_INDEX_FILE=index.temp\n> 3. git reset master -- . (git read-tree also works here, but is a bit slower)\n> 4. git format-patch --full-index B~..B\n> 5. git apply --cached B.patch\n> 6. git write-tree\n> 7. git commit-tree {output of 6} -p master -m \"message\"\n> 8. either `git symbolic-ref` to make a branch or `git push` to remote\n> \n> I'm looking to improve the git apply step in #5.\n> Currently we can't use --cached in combination with\n> --3way, which limits some of the usefulness of this method.\n> There are many diffs that will block applying a patch\n> that a 3 way merge can resolve without conflicts. Even\n> in the case where there are real conflicts, performing\n> a 3 way merge will allow us to show the user the lines\n> where the conflict occurred.\n> \n> With the above in mind, I've created a small patch that\n> implements the behavior I'd like. Rather than disallow\n> the cached and 3way flags to be combined, we allow them,\n> but write any conflicts directly to the cached file. Since\n> we're unable to modify the working directory, it seems\n> reasonable in this case to not actually present the user\n> with any options to resolve conflicts. Instead, a script\n> or tool using this command can diff the temporary cache\n> to get the source of the conflict.\n> \n> Happy to address any feedback. After I address any major\n> changes I will add new tests for this path.\n> \n> All tests passed locally.\n> \n> Jerry Zhang (1):\n> git-apply: Allow simultaneous --cached and --3way options\n> \n> Documentation/git-apply.txt | 4 +++-\n> apply.c | 13 +++++++------\n> 2 files changed, 10 insertions(+), 7 deletions(-)\n> \nI know this patch series only have one patch, so why add the\ncover letter (PATCH 0/1)? Customarily, single-patch series just\nbegin with `[PATCH] some title`, then patch message, and actual\ndiff; all without cover letter.\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"420925","messageId":"dea7dd84-0763-cd1b-7996-541a5cde3a06@gmail.com","threadId":"55429","inReplyTo":"CAMKO5CtiW84E4XjnPRf-yOPp+ua_u07LsAu=BB0YhmP3+3kYiw@mail.gmail.com","subject":"Re: [PATCH 0/1] git-apply: Allow simultaneous --cached and --3way options","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-04-03T08:05:36Z","receivedAt":"2021-04-03T08:05:44Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On 03/04/21 13.57, Jerry Zhang wrote:\n> I wanted to provide extra context to explain the background of what I'm\n> trying to accomplish,\n> but didn't want it all merged as commit text in the tree ;-)\n> \nBut you can add these contexts to your patch, just put them between\n--- and `diff` line. (after patch message and before actual diff).\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"420945","messageId":"xmqq1rbq276g.fsf@gitster.g","threadId":"55429","inReplyTo":"xmqqy2e00zaf.fsf@gitster.g","subject":"Re: [PATCH 1/1] git-apply: Allow simultaneous --cached and --3way options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-04T01:02:31Z","receivedAt":"2021-04-04T01:02:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> It might be OK to only allow the combination when everything auto\n> resolves cleanly and fail the operation without touching either the\n> index or the working tree. Pretending there was no delete/modify\n> conflicts or adding contents with unresolved conflicts as if nothing\n> bad happened as stage 0 entries would never be acceptable.\n>\n> Perhaps\n>\n> * Error out if the index does not match HEAD.\n>\n> * Try applying to the contents in the index. If there are any\n> structural conflicts, leave these paths at higher stage and do\n> not touch their contents.\n>\n> * For paths without structural conflict but need content merges,\n> attempt ll-merge of the contents. If autoresolves cleanly,\n> register the result at stage 0. Otherwise, discard the failed\n> conflicted merge, and leave stages 1, 2 and 3 as they are.\n>\n> * Exit with status 0 if and only if everything has resolved\n> cleanly. Otherwise, exit with non-zero status.\n>\n> would be the minimally-acceptably-safe behaviour.\n\nNote that, while a lot unsatisfactory than the above, the following\nwould also be acceptable.\n\n * Error out if the index does not match HEAD.\n\n * Try applying to the contents in the index. If there are any\n structural conflicts, abort without touching the index (or the\n working tree --- but that is best left unsaid as we all know we\n are talking about '--cached').\n\n * For paths without structural conflict but need content merges,\n attempt ll-merge of the contents. If ALL SUCh PATHS autoresolve\n cleanly, register their result at stage 0. Otherwise, abort\n without touching the index (or the working tree).\n\n * Exit with status 0 if and only if everything has resolved\n cleanly. Otherwise, exit with non-zero status (and never touch\n the index or the working tree).\n\nThe version I earlier gave would give a good starting point to\nmanually resolve the conflicts in the index and when resolved fully,\nit is safely recorded as the result of applying the patch on top of\nHEAD, because the non-final results are all in higher stages, and\nall the paths at stage 0 are either from the HEAD and unaffected by\nthe merge, or the ones that cleanly resolved. The \"the index must\nmatch HEAD\" upfront is to ensure that. Otherwise it would make it\nvery tempting, after spending all that time to resolve the conflicts\nonly in the higher stages of the index, to commit the index as-is to\nmake a child commit of HEAD and record that it is the result of\napplying the patch. But if the starting condition had a change\nunrelated to the change the patch brings in already in the index,\nthe resulting commit would be _more_ than what the patch did to the\ncodebase.\n\nThe simplified version would let the user proceed only when the\nconflicts can mechanically resolved, but it still has the \"make sure\nwhat is recorded is only from the incoming patch\" safety.\n\nOf course, if the user is trying to cherry-pick parts of multiple\npatches and combine them to create a new single commit, the second\nand subsequent applycation of the patches would be thwarted by the\n\"the index must match HEAD\" rule, but it is far safer to make each\nstep into its own snapshot commit during such a workflow to combine\nmultiple patch pieces and then squash them together after finishing,\nthan carrying an intermediate result only in the index and risk\nlosing work you did in the previous step(s) to incorrect resolution\nin later step(s).\n"},{"id":"421016","messageId":"CAMKO5CsN+J_30vhJTo5PYj_9SNJVh_y33APUviG2P4bir29RjQ@mail.gmail.com","threadId":"55429","inReplyTo":"CABPp-BGSgyAH0w21Vrv_bdPaLg+rCPViktbUmM6fMbmxaK70qA@mail.gmail.com","subject":"Re: [PATCH 0/1] git-apply: Allow simultaneous --cached and --3way options","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-04-05T22:05:34Z","receivedAt":"2021-04-05T22:05:49Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"On Fri, Apr 2, 2021 at 8:04 PM Elijah Newren wrote:\n>\n> On Fri, Apr 2, 2021 at 6:36 PM Jerry Zhang wrote:\n> >\n> > I'm creating a script/tool that will be able to cherry-pick\n> > multiple commits from a single branch, rebase them onto a\n> > base commit, and push those references to a remote.\n> >\n> > Ex. with a branch like \"origin/master -> A -> B -> C\"\n> > The tool will create \"master -> A\", \"master -> B\",\n> > \"master -> C\" and either make local branches or\n> > push them to a remote. This can be useful since code\n> > review tools like github use branches as the basis\n> > for pull requests.\n>\n> Not sure I understand the \"master -> A\", \"master -> B\" syntax. What\n> do you mean here?\nAh yeah my syntax wasn't super clear here.\nI mean a branch \"dev\" pointing to commit \"C\", which is on top of \"B\",\nwhich is on top of \"A\", which is on top of \"master\".\nMy tool would fake \"cherry-pick\" each of A, B, and C on top of master.\n>\n> > A key feature here is that the above happens without\n> > any changes to the user's working directory or cache.\n> > This is important since those operations will add\n> > time and generate build churn. We use these steps\n> > for synthesizing a \"cherry-pick\" of B to master.\n> >\n> > 1. cp .git/index index.temp\n> > 2. set GIT_INDEX_FILE=index.temp\n> > 3. git reset master -- . (git read-tree also works here, but is a bit slower)\n> > 4. git format-patch --full-index B~..B\n> > 5. git apply --cached B.patch\n> > 6. git write-tree\n> > 7. git commit-tree {output of 6} -p master -m \"message\"\n> > 8. either `git symbolic-ref` to make a branch or `git push` to remote\n>\n> Yeah, folks have resorted to various variants of this kind of thing in\n> the past. It is a clever way to handle some basic cases, but it does\n> often fall short. It's unfortunate that cherry-pick and rebase cannot\n> yet just provide this functionality (more on that below).\n>\n> It may also interest you that rebase has two different backends, one\n> built on am (which in turn is built on format-patch + apply), and one\n> built on the merge machinery (which the am --3way also uses when it\n> needs to). We deprecated the format-patch + apply backend in part\n> because it sometimes results in misapplied patches; see the \"Context\"\n> subsection of the \"BEHAVIORAL DIFFERENCES\" section of the git-rebase\n> manpage. However, the am version would at least handle basic renames,\n> which I believe might cause problems for a direct format-patch + apply\n> invocation like yours (I'll also discuss this more below).\nThanks -- I was able to repro a case where am machinery applied a patch\nincorrectly but 3way applied it correctly. This actually brings up\nanother point,\nbecause am doesn't report errors when applying a patch incorrectly in this\ncase, we don't end up falling back to 3way. There also is no user flag\nto force 3way, so the user can't do anything to ensure the correct\napplication here. Maybe it would be better for --3way to directly invoke\nthe 3way merge rather than causing it to fallback? (Junio might also\nhave some input here).\n>\n> > I'm looking to improve the git apply step in #5.\n> > Currently we can't use --cached in combination with\n> > --3way, which limits some of the usefulness of this method.\n> > There are many diffs that will block applying a patch\n> > that a 3 way merge can resolve without conflicts. Even\n> > in the case where there are real conflicts, performing\n> > a 3 way merge will allow us to show the user the lines\n> > where the conflict occurred.\n> >\n> > With the above in mind, I've created a small patch that\n> > implements the behavior I'd like. Rather than disallow\n> > the cached and 3way flags to be combined, we allow them,\n> > but write any conflicts directly to the cached file. Since\n> > we're unable to modify the working directory, it seems\n> > reasonable in this case to not actually present the user\n> > with any options to resolve conflicts. Instead, a script\n> > or tool using this command can diff the temporary cache\n> > to get the source of the conflict.\n>\n> Looks like you're focusing on content conflicts. What about path\n> conflicts? For example, apply's --3way just uses a per-file\n> ll_merge() call, meaning it won't handle renames, so your method would\n> also often get spurious modify/delete conflicts when renames occur.\n> How does your plan to just \"cache\" conflicts work with these\n> modify/delete files? Will users just look for conflict markers and\n> ignore the fact that both modified newfile and modified oldfile are\n> present? I'm also curious how e.g. directory/file conflicts would be\n> handled by your scheme; these seem somewhat problematic to me from\n> your description.\n>\n> > Happy to address any feedback. After I address any major\n> > changes I will add new tests for this path.\n>\n> Don't know the timeframe you're looking at, but I'm looking to modify\n> cherry-pick and rebase to be able to operate on branches that are not\n> checked out, or in bare repositories. The blocker to that\nThat functionality would be great. I initially did look at what it would\ntake to modify sequencer to get what I wanted, but I quickly realized\nit would be a big refactor.\n> traditionally has been that the merge machinery required a working\n> directory. The good news is that I wrote a new merge backend that\n> doesn't require a working directory. The bad news is I'm still trying\n> to get that new merge backend through the review process, and no\n> current release of git has a complete version of that new backend yet.\n> Further, the changes to cherry-pick and rebase have not yet been\n> started. There were some decisions to make too, such as how to handle\n> the case with conflicts -- just report them and tell the user to retry\n> with a checkout? Provide some kind of basic information about the\n> conflicts? What'd be useful to you?\nAfter thinking some more I'd generally agree with comments to leave\nthe conflicts at higher stages rather than check in the conflict markers.\nThis should result in less issues with path conflicts as well (or at least\nbe similar to 3way by itself). This is probably ok, because the conflict\nmarkers can always be generated from the higher stage files (git diff or\ngit checkout -m -- .), but the reverse isn't true.\nOverall 90% of the functionality comes from being able to do the 3-way\nat all, since it's able to handle more cases correctly. Having *any* output\nto tell the user why their operation failed would just be a bonus.\n\nI'd envision flags similar to these, for cherry-pick\n\n--cached : Do not touch the working tree to apply conflict markers.\nInstead conflicts are left at a higher order in the cache.\n--cached-parent : Checkout the index to the given commit, then\napply the cherry-pick with the given commit as a parent. Print out\nthe new commit. Warning: index will be left unsynchronized with\nHEAD after this operation. Intended to be used with a temporary index\nrather than the main one.\n\nIn the end I'm not sure how to still accomplish the desired functionality\nwithout using a temporary index -- this would always result in\ndesyncing the user's index / working dir afterwards. Maybe error or\nwarn if the user isn't using a temporary index?\n"},{"id":"421017","messageId":"CAMKO5Ct89R=Ls61H-q4ArhmWSokRQKS_uc93+=6_Gnxnjnk80A@mail.gmail.com","threadId":"55429","inReplyTo":"CABPp-BGhvQF9k1Jw9NPbZWMkNSffqR777-4S-y-Sh=Etvw-SAA@mail.gmail.com","subject":"Re: [PATCH 1/1] git-apply: Allow simultaneous --cached and --3way options","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-04-05T22:08:01Z","receivedAt":"2021-04-05T22:08:17Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"On Fri, Apr 2, 2021 at 8:46 PM Elijah Newren wrote:\n>\n> I'm not that familiar with apply.c, but let me attempt to take a look...\n>\n> On Fri, Apr 2, 2021 at 6:36 PM Jerry Zhang wrote:\n> >\n> > Previously, --cached and --3way were not\n> > allowed to be used together, since --3way\n> > wrote conflict markers into the working tree.\n> >\n> > These changes change semantics so that if\n> > these flags are given together and there is\n> > a conflict, the conflict markers are added\n> > directly to cache. If there is no conflict,\n> > the patch is applied directly to cache as\n> > expected.\n> >\n> > Signed-off-by: Jerry Zhang \n> > Signed-off-by: Jerry Zhang \n> > ---\n> > Documentation/git-apply.txt | 4 +++-\n> > apply.c | 13 +++++++------\n> > 2 files changed, 10 insertions(+), 7 deletions(-)\n> >\n> > diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt\n> > index 91d9a8601c..3dc0085066 100644\n> > --- a/Documentation/git-apply.txt\n> > +++ b/Documentation/git-apply.txt\n> > @@ -89,7 +89,9 @@ OPTIONS\n> > and we have those blobs available locally, possibly leaving the\n> > conflict markers in the files in the working tree for the user to\n> > resolve. This option implies the `--index` option, and is incompatible\n> > - with the `--reject` and the `--cached` options.\n> > + with the `--reject` option. When used with the --cached option, any\n> > + conflict markers are added directly to the cache rather than the\n> > + working tree.\n> >\n> > --build-fake-ancestor=::\n> > Newer 'git diff' output has embedded 'index information'\n> > diff --git a/apply.c b/apply.c\n> > index 6695a931e9..fc94ca0e99 100644\n> > --- a/apply.c\n> > +++ b/apply.c\n> > @@ -133,8 +133,6 @@ int check_apply_state(struct apply_state *state, int force_apply)\n> >\n> > if (state->apply_with_reject && state->threeway)\n> > return error(_(\"--reject and --3way cannot be used together.\"));\n> > - if (state->cached && state->threeway)\n> > - return error(_(\"--cached and --3way cannot be used together.\"));\n> > if (state->threeway) {\n> > if (is_not_gitdir)\n> > return error(_(\"--3way outside a repository\"));\n> > @@ -4490,13 +4488,16 @@ static int create_file(struct apply_state *state, struct patch *patch)\n> >\n> > if (!mode)\n> > mode = S_IFREG | 0644;\n> > - if (create_one_file(state, path, mode, buf, size))\n> > - return -1;\n> > + if (!state->cached) {\n>\n> Why add this check? create_one_file() already has an early return if\n> state->cached is true.\n>\nremoved in latest patch\n> > + if (create_one_file(state, path, mode, buf, size))\n> > + return -1;\n> > + }\n> >\n> > - if (patch->conflicted_threeway)\n> > + if (patch->conflicted_threeway && !state->cached)\n> > return add_conflicted_stages_file(state, patch);\n> > - else if (state->update_index)\n> > + else if (state->update_index) {\n> > return add_index_file(state, path, mode, buf, size);\n>\n> So if something had conflicts, you ignore the various conflicted\n> modes, and just add it to the index as it stands. What if it was\n> deleted upstream and modified locally? Doesn't that just ignore the\n> conflict, make it invisible to the user, and add the locally modified\n> version? Similarly if it was renamed upstream and modified locally,\n> doesn't that end up in both files being present? And if there's a\n> directory/file conflict, due to the lack of ADD_CACHE_SKIP_DFCHECK (or\n> whatever it's called), the add is just going to fail, but perhaps\n> that's the most reasonable case as it'd print an error message and\n> return -1, I think.\n>\n> Again, I didn't test any of this out and I'm not so familiar with this\n> code, so I'm guessing at these scenarios. If I'm wrong about how this\n> works, the commit message probably deserves an explanation about why\n> they work, and we'd definitely need a few testcases for these types of\n> scenarios. If I'm right, the current implementation is problematic at\n> least if not the idea of using these options together.\nEchoing my other reply, I think I will give up on the conflict markers and\njust leave the files at the higher stages. I think that will address any\nsafety concerns since that's what --3way does without the cached\nflag\n"},{"id":"421018","messageId":"CAMKO5CtCk_sJsFFiKKFR1wCSyY226CbxPtN6=p6JRzocSuv8jQ@mail.gmail.com","threadId":"55429","inReplyTo":"xmqq1rbq276g.fsf@gitster.g","subject":"Re: [PATCH 1/1] git-apply: Allow simultaneous --cached and --3way options","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-04-05T22:12:43Z","receivedAt":"2021-04-05T22:12:58Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"On Sat, Apr 3, 2021 at 6:02 PM Junio C Hamano wrote:\n>\n> Junio C Hamano writes:\n>\n> > It might be OK to only allow the combination when everything auto\n> > resolves cleanly and fail the operation without touching either the\n> > index or the working tree. Pretending there was no delete/modify\n> > conflicts or adding contents with unresolved conflicts as if nothing\n> > bad happened as stage 0 entries would never be acceptable.\nYep I've changed it to just leave the higher stages in the index\n> >\n> > Perhaps\n> >\n> > * Error out if the index does not match HEAD.\n> >\n> > * Try applying to the contents in the index. If there are any\n> > structural conflicts, leave these paths at higher stage and do\n> > not touch their contents.\n> >\n> > * For paths without structural conflict but need content merges,\n> > attempt ll-merge of the contents. If autoresolves cleanly,\n> > register the result at stage 0. Otherwise, discard the failed\n> > conflicted merge, and leave stages 1, 2 and 3 as they are.\n> >\n> > * Exit with status 0 if and only if everything has resolved\n> > cleanly. Otherwise, exit with non-zero status.\n> >\n> > would be the minimally-acceptably-safe behaviour.\n>\n> Note that, while a lot unsatisfactory than the above, the following\n> would also be acceptable.\n>\n> * Error out if the index does not match HEAD.\n>\n> * Try applying to the contents in the index. If there are any\n> structural conflicts, abort without touching the index (or the\n> working tree --- but that is best left unsaid as we all know we\n> are talking about '--cached').\n>\n> * For paths without structural conflict but need content merges,\n> attempt ll-merge of the contents. If ALL SUCh PATHS autoresolve\n> cleanly, register their result at stage 0. Otherwise, abort\n> without touching the index (or the working tree).\n>\n> * Exit with status 0 if and only if everything has resolved\n> cleanly. Otherwise, exit with non-zero status (and never touch\n> the index or the working tree).\n>\n> The version I earlier gave would give a good starting point to\n> manually resolve the conflicts in the index and when resolved fully,\n> it is safely recorded as the result of applying the patch on top of\n> HEAD, because the non-final results are all in higher stages, and\n> all the paths at stage 0 are either from the HEAD and unaffected by\n> the merge, or the ones that cleanly resolved. The \"the index must\n> match HEAD\" upfront is to ensure that. Otherwise it would make it\n> very tempting, after spending all that time to resolve the conflicts\n> only in the higher stages of the index, to commit the index as-is to\n> make a child commit of HEAD and record that it is the result of\n> applying the patch. But if the starting condition had a change\n> unrelated to the change the patch brings in already in the index,\n> the resulting commit would be _more_ than what the patch did to the\n> codebase.\nI can see what you mean about the user safety issue. However,\nmy specific use case (see cover letter) involves an index that does not\nmatch HEAD, and wouldn't be possible at all if we forced the index to\nmatch HEAD. Furthermore git-apply --cached even without --3way\ndoesn't force the index to match HEAD either, so why force it now?\n\n>\n> The simplified version would let the user proceed only when the\n> conflicts can mechanically resolved, but it still has the \"make sure\n> what is recorded is only from the incoming patch\" safety.\n>\n> Of course, if the user is trying to cherry-pick parts of multiple\n> patches and combine them to create a new single commit, the second\n> and subsequent applycation of the patches would be thwarted by the\n> \"the index must match HEAD\" rule, but it is far safer to make each\n> step into its own snapshot commit during such a workflow to combine\n> multiple patch pieces and then squash them together after finishing,\n> than carrying an intermediate result only in the index and risk\n> losing work you did in the previous step(s) to incorrect resolution\n> in later step(s).\n"},{"id":"421020","messageId":"20210405221902.27998-1-jerry@skydio.com","threadId":"55429","inReplyTo":"20210403013410.32064-2-jerry@skydio.com","subject":"[PATCH V2] git-apply: Allow simultaneous --cached and --3way options","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-04-05T22:19:02Z","receivedAt":"2021-04-05T22:19:14Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"Previously, --cached and --3way were not\nallowed to be used together, since --3way\nwrote conflict markers into the working tree.\n\nThese changes change semantics so that if\nthese flags are given together and there is\na conflict, the conflicting objects are left\nat a higher order in the cache, and the command\nwill return non-zero. If there is no conflict,\nthe patch is applied directly to cache as\nexpected and the command will return 0.\n\nThe user can use `git diff` to view the contents\nof the conflict, or `git checkout -m -- .` to\nregenerate the conflict markers in the working\ndirectory.\n\nWith the combined --3way and --cached flags,\nThe conflict markers won't be written to the\nworking directory, so there is no point in\nattempting rerere.\n\nSigned-off-by: Jerry Zhang \nSigned-off-by: Jerry Zhang \n---\n Documentation/git-apply.txt | 3 ++-\n apply.c | 5 ++---\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt\nindex 91d9a8601c..392882d9a5 100644\n--- a/Documentation/git-apply.txt\n+++ b/Documentation/git-apply.txt\n@@ -89,7 +89,8 @@ OPTIONS\n \tand we have those blobs available locally, possibly leaving the\n \tconflict markers in the files in the working tree for the user to\n \tresolve. This option implies the `--index` option, and is incompatible\n-\twith the `--reject` and the `--cached` options.\n+\twith the `--reject` option. When used with the --cached option, any conflicts\n+ are left at higher stages in the cache.\n \n --build-fake-ancestor=::\n \tNewer 'git diff' output has embedded 'index information'\ndiff --git a/apply.c b/apply.c\nindex 6695a931e9..e59c77a1b7 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -133,8 +133,6 @@ int check_apply_state(struct apply_state *state, int force_apply)\n \n \tif (state->apply_with_reject && state->threeway)\n \t\treturn error(_(\"--reject and --3way cannot be used together.\"));\n-\tif (state->cached && state->threeway)\n-\t\treturn error(_(\"--cached and --3way cannot be used together.\"));\n \tif (state->threeway) {\n \t\tif (is_not_gitdir)\n \t\t\treturn error(_(\"--3way outside a repository\"));\n@@ -4646,7 +4644,8 @@ static int write_out_results(struct apply_state *state, struct patch *list)\n \t\t}\n \t\tstring_list_clear(&cpath, 0);\n \n-\t\trepo_rerere(state->repo, 0);\n+\t\tif (!state->cached)\n+\t\t\trepo_rerere(state->repo, 0);\n \t}\n \n \treturn errs;\n-- \n2.29.0\n\n"},{"id":"421021","messageId":"xmqqy2dw4bh3.fsf@gitster.g","threadId":"55429","inReplyTo":"CAMKO5CtCk_sJsFFiKKFR1wCSyY226CbxPtN6=p6JRzocSuv8jQ@mail.gmail.com","subject":"Re: [PATCH 1/1] git-apply: Allow simultaneous --cached and --3way options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-05T22:23:36Z","receivedAt":"2021-04-05T22:23:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jerry Zhang writes:\n\n> I can see what you mean about the user safety issue. However,\n> my specific use case (see cover letter) involves an index that does not\n> match HEAD, and wouldn't be possible at all if we forced the index to\n> match HEAD. Furthermore git-apply --cached even without --3way\n> doesn't force the index to match HEAD either, so why force it now?\n\nPrimarily because we tend to be extra careful before mergy operation\nthan any other operation. Especially without --3way, apply (with or\nwithout --cached/--index) is extra careful to make itself all-or-none\noperation to be safe, so that there is no mixed mess that requires\nmanual intervention (which would further increase the risk of mistakes).\n\nIt is OK to introduce a new option to allow a dirty index, and your\ntool can pass that option when it calls \"apply --cached --3way\", but\nit would be safe to require a clean index (it does not matter how\ndirty the working tree is ;-) by default.\n\n"},{"id":"421023","messageId":"xmqqr1jo4aex.fsf@gitster.g","threadId":"55429","inReplyTo":"20210405221902.27998-1-jerry@skydio.com","subject":"Re: [PATCH V2] git-apply: Allow simultaneous --cached and --3way options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-05T22:46:30Z","receivedAt":"2021-04-05T22:46:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jerry Zhang writes:\n\n> Subject: Re: [PATCH V2] git-apply: Allow simultaneous --cached and --3way options\n\ns/Allow/allow/ (cf. \"git shortlog --no-merged\" output for recent examples)\n\n> Previously, --cached and --3way were not\n> allowed to be used together, since --3way\n> wrote conflict markers into the working tree.\n\nHint that you are talking about the \"git apply\" command by\nmentioning the name somewhere.\n\nDrop \"previously\"; we talk about the status quo in the present tense\nin our proposed commit log messages to set the stage, and then describe\nwhat the patch author percieves as a problem, before describing the\nproposed solution to the problem.\n\ncf. Documentation/SubmittingPatches[[describe-changes]] (the whole section)\n\n> These changes change semantics so that if\n> these flags are given together and there is\n> a conflict, the conflicting objects are left\n> at a higher order in the cache, and the command\n> will return non-zero. If there is no conflict,\n> the patch is applied directly to cache as\n> expected and the command will return 0.\n\nGive an order to the codebase to \"be like so\". Here is my attempt.\n\n Teach \"git apply\" to accept \"--cached\" and \"--3way\" at the same\n time. Only when all changes to all paths involved in the\n application auto-resolve cleanly, the result is placed in the\n index at stage #0 and the command exits with 0 status. If there\n is any path whose conflict cannot be cleanly auto-resolved, the\n original contents from common ancestor (stage #1), our version\n (stage #2) and the contents from the patch (stage #3) for the\n conflicted paths are left at separate stages without any attempt\n to resolve the conflict at the content level, and the command\n exists with non-zero status, because there is no place (like the\n working tree files) to leave a half-resolved conflicted merge\n result to ask the end-user to resolve.\n\n> The user can use `git diff` to view the contents\n> of the conflict, or `git checkout -m -- .` to\n> regenerate the conflict markers in the working\n> directory.\n\nNice.\n\n> With the combined --3way and --cached flags,\n> The conflict markers won't be written to the\n> working directory, so there is no point in\n> attempting rerere.\n\nI am not sure what this paragraph is trying to convey here.\n\nI agree that when a *new* conflict is encountered in this new mode,\nwriting out a rerere pre-image, in preparation for accepting the\npost-image the end-user gives us after the conflicts are resolved,\ndoes not make sense, because we are not giving the end-user the\nconflicted state and asking to help resolve it for us.\n\nBut if a rerere database entry records a previous merge result in\nwhich conflicts were resolved by the end user, it would make sense\nto try reusing the resolution, I would think. I offhand do not know\nhow involved it would be to do so, so punting on that is fine, but\nthat is \"there is no point\", but it is \"we are not trying\".\n\nPerhaps\n\n When there are conflicts, theoretically, it would be nice to be\n able to replay an existing entry in the rerere database that\n records already resolved conflict that match the current one,\n but that would be too much work, so let's not try it for now.\n\nwould be a good explanation why we are not doing (i.e. we made a\ntrade-off) and recording that is important, as it will allow others\nin the future to try building on the change we are proposing here\n(it is not like we decided that it is fundamentally wrong to try to\nuse rerere in this situation).\n\n> Signed-off-by: Jerry Zhang \n> Signed-off-by: Jerry Zhang \n\nUnless we are interacting with two people with the same name, please\nsign-off with the same name/address as the name/address that will be\nrecorded as the author of this change. I am guessing that dropping\nthe latter should be sufficient?\n\nThanks.\n"},{"id":"421025","messageId":"CAMKO5CuYE1VA2h2zDo-b77WQDgj1LriwifruziPA30Yb7uS=6A@mail.gmail.com","threadId":"55429","inReplyTo":"xmqqy2dw4bh3.fsf@gitster.g","subject":"Re: [PATCH 1/1] git-apply: Allow simultaneous --cached and --3way options","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-04-05T23:29:15Z","receivedAt":"2021-04-05T23:29:30Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"On Mon, Apr 5, 2021 at 3:23 PM Junio C Hamano wrote:\n>\n> Jerry Zhang writes:\n>\n> > I can see what you mean about the user safety issue. However,\n> > my specific use case (see cover letter) involves an index that does not\n> > match HEAD, and wouldn't be possible at all if we forced the index to\n> > match HEAD. Furthermore git-apply --cached even without --3way\n> > doesn't force the index to match HEAD either, so why force it now?\n>\n> Primarily because we tend to be extra careful before mergy operation\n> than any other operation. Especially without --3way, apply (with or\n> without --cached/--index) is extra careful to make itself all-or-none\n> operation to be safe, so that there is no mixed mess that requires\n> manual intervention (which would further increase the risk of mistakes).\n>\n> It is OK to introduce a new option to allow a dirty index, and your\n> tool can pass that option when it calls \"apply --cached --3way\", but\n> it would be safe to require a clean index (it does not matter how\n> dirty the working tree is ;-) by default.\n>\nSure adding the staged files will definitely clobber whatever the user\nhad in the cache at stage 0. This will probably be unexpected. But\nthe normal invocation of --3way also does this without warning, since\nit touches the cache as well. It just seems odd to me to be adding a safety\ncheck on some paths that aren't there on other very similar ones. Maybe\nanother option would be to add a very stern warning for users of --3way?\n\nUnrelatedly would you have context on why --3way falls back on 3way\nrather than trying 3way first then falling back on apply_fragments if\nblobs don't exist? I see some cases where the normal patch application\nwill succeed but apply the patch incorrectly, while 3way will apply the\npatch correctly. In these cases it's impossible for the user to force 3way.\nAre there downsides to 3way that aren't solved by falling back on\napply_fragments?\n"},{"id":"421029","messageId":"xmqqblas46ja.fsf@gitster.g","threadId":"55429","inReplyTo":"CAMKO5CuYE1VA2h2zDo-b77WQDgj1LriwifruziPA30Yb7uS=6A@mail.gmail.com","subject":"Re: [PATCH 1/1] git-apply: Allow simultaneous --cached and --3way options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-06T00:10:17Z","receivedAt":"2021-04-06T00:10:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jerry Zhang writes:\n\n> Unrelatedly would you have context on why --3way falls back on 3way\n> rather than trying 3way first then falling back on apply_fragments if\n> blobs don't exist?\n\nHistorical accident, following the order in which these features\nwere invented, plus applying a single patch straight has been faster\nthan doing a 3-way.\n\nI tend to agree with what you are hinting at, though, as I do not\noffhand think of a situation in which a successful 3way merge would\nbe less correct than a straight patch application, and if the user\nexplicitly has told us to do \"--3way\", that is a sign that it is\nacceptable to try 3way first even if it costs more cycles.\n\nThose who has been giving \"--3way\" from inertia would notice if the\nperformance difference is large enough and may complain, though.\n"},{"id":"421036","messageId":"20210406024931.24355-1-jerry@skydio.com","threadId":"55429","inReplyTo":"20210405221902.27998-1-jerry@skydio.com","subject":"[PATCH v3] git-apply: allow simultaneous --cached and --3way options","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-04-06T02:49:31Z","receivedAt":"2021-04-06T02:49:41Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"\"git apply\" does not allow \"--cached\" and\n\"--3way\" to be used together, since \"--3way\"\nwrites conflict markers into the working tree.\n\nAllow \"git apply\" to accept \"--cached\" and\n\"--3way\" at the same time. When all changes\nauto-resolve cleanly, the result is placed in the\nindex at stage #0 and the command exits with 0\nstatus. If there is any path whose conflict\ncannot be cleanly auto-resolved, the original\ncontents from common ancestor (stage #1), our\nversion (stage #2) and the contents from the\npatch (stage #3) are left at separate stages.\nNo attempt is made to resolve the conflict at\nthe content level, and the command exists with\n non-zero status, because there is no place\n(like the working tree) to leave a half-resolved\n merge for the user to resolve.\n\nThe user can use `git diff` to view the contents\nof the conflict, or `git checkout -m -- .` to\nregenerate the conflict markers in the working\ndirectory.\n\nSince rerere depends on conflict markers written\nto file for its database storage and lookup, don't\nattempt it in this case. This could be fixable\nif the in memory conflict markers from the ll_merge\nresult could be passed to the rerere api.\n\nSigned-off-by: Jerry Zhang \n---\n Documentation/git-apply.txt | 6 ++++--\n apply.c | 7 +++----\n t/t4108-apply-threeway.sh | 24 ++++++++++++++++++++++++\n 3 files changed, 31 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt\nindex 91d9a8601c8c316d4649c405af42e531c39991a8..9c48863c47287208850e8376f43453ecec595444 100644\n--- a/Documentation/git-apply.txt\n+++ b/Documentation/git-apply.txt\n@@ -88,8 +88,10 @@ OPTIONS\n \tthe patch records the identity of blobs it is supposed to apply to,\n \tand we have those blobs available locally, possibly leaving the\n \tconflict markers in the files in the working tree for the user to\n-\tresolve. This option implies the `--index` option, and is incompatible\n-\twith the `--reject` and the `--cached` options.\n+\tresolve. This option implies the `--index` option unless the\n+\t`--cached` option is used, and is incompatible with the `--reject` option.\n+\tWhen used with the `--cached` option, any conflicts are left at higher stages\n+\tin the cache.\n \n --build-fake-ancestor=::\n \tNewer 'git diff' output has embedded 'index information'\ndiff --git a/apply.c b/apply.c\nindex 6695a931e979a968b28af88d425d0c76ba17d0d4..02d13ea6db7f9a4066dec3d33d7ddbe11b616f33 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -133,8 +133,6 @@ int check_apply_state(struct apply_state *state, int force_apply)\n \n \tif (state->apply_with_reject && state->threeway)\n \t\treturn error(_(\"--reject and --3way cannot be used together.\"));\n-\tif (state->cached && state->threeway)\n-\t\treturn error(_(\"--cached and --3way cannot be used together.\"));\n \tif (state->threeway) {\n \t\tif (is_not_gitdir)\n \t\t\treturn error(_(\"--3way outside a repository\"));\n@@ -4645,8 +4643,9 @@ static int write_out_results(struct apply_state *state, struct patch *list)\n \t\t\t\tfprintf(stderr, \"U %s\\n\", item->string);\n \t\t}\n \t\tstring_list_clear(&cpath, 0);\n-\n-\t\trepo_rerere(state->repo, 0);\n+\t\t/* rerere relies on conflict markers which aren't written with --cached */\n+\t\tif (!state->cached)\n+\t\t\trepo_rerere(state->repo, 0);\n \t}\n \n \treturn errs;\ndiff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh\nindex d62db3fbe16f35a625a4a14eebb70034f695d3eb..75eb34b13d0024046fd2a510c00c5af1f7bfc52d 100755\n--- a/t/t4108-apply-threeway.sh\n+++ b/t/t4108-apply-threeway.sh\n@@ -160,4 +160,28 @@ test_expect_success 'apply -3 with add/add conflict (dirty working tree)' '\n \ttest_cmp three.save three\n '\n \n+test_expect_success 'apply with --3way --cached' '\n+\t# Merging side should be similar to applying this patch\n+\tgit diff ...side >P.diff &&\n+\n+\t# The corresponding conflicted merge\n+\tgit reset --hard &&\n+\tgit checkout main^0 &&\n+\ttest_must_fail git merge --no-commit side &&\n+\tgit ls-files -s >expect.ls &&\n+\n+\t# should fail to apply\n+\tgit reset --hard &&\n+\tgit checkout main^0 &&\n+\ttest_must_fail git apply --cached --3way P.diff &&\n+\tgit ls-files -s >actual.ls &&\n+\tprint_sanitized_conflicted_diff >actual.diff &&\n+\n+\t# The cache should resemble the corresponding merge\n+\ttest_cmp expect.ls actual.ls &&\n+\t# However the working directory should not change\n+\t>expect.diff &&\n+\ttest_cmp expect.diff actual.diff\n+'\n+\n test_done\n-- \n2.29.0\n\n"},{"id":"421037","messageId":"CAMKO5CuLpa9Sn_oXMpgP6oGE9NFA8aLeTfeyaW6TOTErE0KgEg@mail.gmail.com","threadId":"55429","inReplyTo":"xmqqr1jo4aex.fsf@gitster.g","subject":"Re: [PATCH V2] git-apply: Allow simultaneous --cached and --3way options","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-04-06T02:52:06Z","receivedAt":"2021-04-06T02:52:20Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"Thanks for the comments! I've updated v3 with the changes. Let me know\nif you have any\nmore thoughts on whether to block / warn the user before clobbering their cache.\n\nOn Mon, Apr 5, 2021 at 3:46 PM Junio C Hamano wrote:\n>\n> Jerry Zhang writes:\n>\n> > Subject: Re: [PATCH V2] git-apply: Allow simultaneous --cached and --3way options\n>\n> s/Allow/allow/ (cf. \"git shortlog --no-merged\" output for recent examples)\n>\n> > Previously, --cached and --3way were not\n> > allowed to be used together, since --3way\n> > wrote conflict markers into the working tree.\n>\n> Hint that you are talking about the \"git apply\" command by\n> mentioning the name somewhere.\n>\n> Drop \"previously\"; we talk about the status quo in the present tense\n> in our proposed commit log messages to set the stage, and then describe\n> what the patch author percieves as a problem, before describing the\n> proposed solution to the problem.\n>\n> cf. Documentation/SubmittingPatches[[describe-changes]] (the whole section)\n>\n> > These changes change semantics so that if\n> > these flags are given together and there is\n> > a conflict, the conflicting objects are left\n> > at a higher order in the cache, and the command\n> > will return non-zero. If there is no conflict,\n> > the patch is applied directly to cache as\n> > expected and the command will return 0.\n>\n> Give an order to the codebase to \"be like so\". Here is my attempt.\n>\n> Teach \"git apply\" to accept \"--cached\" and \"--3way\" at the same\n> time. Only when all changes to all paths involved in the\n> application auto-resolve cleanly, the result is placed in the\n> index at stage #0 and the command exits with 0 status. If there\n> is any path whose conflict cannot be cleanly auto-resolved, the\n> original contents from common ancestor (stage #1), our version\n> (stage #2) and the contents from the patch (stage #3) for the\n> conflicted paths are left at separate stages without any attempt\n> to resolve the conflict at the content level, and the command\n> exists with non-zero status, because there is no place (like the\n> working tree files) to leave a half-resolved conflicted merge\n> result to ask the end-user to resolve.\n>\n> > The user can use `git diff` to view the contents\n> > of the conflict, or `git checkout -m -- .` to\n> > regenerate the conflict markers in the working\n> > directory.\n>\n> Nice.\n>\n> > With the combined --3way and --cached flags,\n> > The conflict markers won't be written to the\n> > working directory, so there is no point in\n> > attempting rerere.\n>\n> I am not sure what this paragraph is trying to convey here.\n>\n> I agree that when a *new* conflict is encountered in this new mode,\n> writing out a rerere pre-image, in preparation for accepting the\n> post-image the end-user gives us after the conflicts are resolved,\n> does not make sense, because we are not giving the end-user the\n> conflicted state and asking to help resolve it for us.\n>\n> But if a rerere database entry records a previous merge result in\n> which conflicts were resolved by the end user, it would make sense\n> to try reusing the resolution, I would think. I offhand do not know\n> how involved it would be to do so, so punting on that is fine, but\n> that is \"there is no point\", but it is \"we are not trying\".\n>\n> Perhaps\n>\n> When there are conflicts, theoretically, it would be nice to be\n> able to replay an existing entry in the rerere database that\n> records already resolved conflict that match the current one,\n> but that would be too much work, so let's not try it for now.\n>\n> would be a good explanation why we are not doing (i.e. we made a\n> trade-off) and recording that is important, as it will allow others\n> in the future to try building on the change we are proposing here\n> (it is not like we decided that it is fundamentally wrong to try to\n> use rerere in this situation).\n>\n> > Signed-off-by: Jerry Zhang \n> > Signed-off-by: Jerry Zhang \n>\n> Unless we are interacting with two people with the same name, please\n> sign-off with the same name/address as the name/address that will be\n> recorded as the author of this change. I am guessing that dropping\n> the latter should be sufficient?\n>\n> Thanks.\n"},{"id":"421044","messageId":"xmqqh7kk2c49.fsf@gitster.g","threadId":"55429","inReplyTo":"CAMKO5CuLpa9Sn_oXMpgP6oGE9NFA8aLeTfeyaW6TOTErE0KgEg@mail.gmail.com","subject":"Re: [PATCH V2] git-apply: Allow simultaneous --cached and --3way options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-06T05:52:38Z","receivedAt":"2021-04-06T05:52:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jerry Zhang writes:\n\n> Thanks for the comments! I've updated v3 with the changes. Let me know\n> if you have any\n> more thoughts on whether to block / warn the user before clobbering their cache.\n\nPlease do not top-post on this list.\n\nI've already said that I think we should ensure the index is clean\nby default, because, unlike the case where the application is done\non the working tree files, the use of \"--cached\" is a sign that the\nnext step is likely to write a tree out. As I've already said so in\nearlier reviews, there is nothing more from me to add on that issue.\n\n>> Give an order to the codebase to \"be like so\". Here is my attempt.\n>>\n>> Teach \"git apply\" to accept \"--cached\" and \"--3way\" at the same\n>> time. Only when all changes to all paths involved in the\n>> application auto-resolve cleanly, the result is placed in the\n>> index at stage #0 and the command exits with 0 status. If there\n>> is any path whose conflict cannot be cleanly auto-resolved, the\n>> original contents from common ancestor (stage #1), our version\n>> (stage #2) and the contents from the patch (stage #3) for the\n>> conflicted paths are left at separate stages without any attempt\n>> to resolve the conflict at the content level, and the command\n>> exists with non-zero status, because there is no place (like the\n>> working tree files) to leave a half-resolved conflicted merge\n>> result to ask the end-user to resolve.\n\nI wrote the above as an example to illustrate the tone and the level\nof details expected in our proposed commit log message. The\nbehaviour it describes may not necessarily match what you have\nimplemented in the patch.\n\nFor example, imagine that we are applying a patch for two paths,\nwhere one auto-resolves cleanly and the other does not. The above\ndescription expects both paths will leave the higher stages (instead\nof recording the auto-resolved path at stage #0, and leaving the\nother path that cannot be auto-resolved at higher stages) and the\ncommand exits with non-zero status, which may not be what you\nimplemented. As an illustration, I didn't necessarily mean such an\nall-or-none behaviour wrt resolving should be what we implement---I\ndo not want to choose, as this is your itch and I want _you_ with\nthe itch to think long and hard before deciding what the best design\nfor end-users would be, and present it as a proposed solution. An\nobvious alternative is to record auto-resolved paths at stage #0 and\nleave only the paths for which auto-resolution failed in conflicted\nstate.\n\nThanks.\n"},{"id":"421096","messageId":"CAMKO5Ctoa8cf9T0reE9DduC7oX8QgQw-sQH315mQN=KiLDS8ag@mail.gmail.com","threadId":"55429","inReplyTo":"xmqqh7kk2c49.fsf@gitster.g","subject":"Re: [PATCH V2] git-apply: Allow simultaneous --cached and --3way options","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-04-06T21:56:42Z","receivedAt":"2021-04-06T21:57:03Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"On Mon, Apr 5, 2021 at 10:52 PM Junio C Hamano wrote:\n>\n> Jerry Zhang writes:\n>\n> > Thanks for the comments! I've updated v3 with the changes. Let me know\n> > if you have any\n> > more thoughts on whether to block / warn the user before clobbering their cache.\n>\n> Please do not top-post on this list.\n>\n> I've already said that I think we should ensure the index is clean\n> by default, because, unlike the case where the application is done\n> on the working tree files, the use of \"--cached\" is a sign that the\n> next step is likely to write a tree out. As I've already said so in\n> earlier reviews, there is nothing more from me to add on that issue.\nUnderstood, but please bear with me to explain the risks a bit more. I'm\nhaving some difficulty coming up with a name and explanation for flags\nfor this case, because I don't completely understand the safety issue\nwe are trying to mitigate.\n\nLet me enumerate some behaviors in 3 different cases where the user\nhas \"file.txt\" changes staged in the index, so index differs from HEAD.\n\n\"git apply --cached\" would either 1. combine the patch and cached version\nand put that in the cache or 2. do nothing (patch failed). In 2 nothing happened\nso the user's changes are safe. In 1 the user's changes may be gone, but\nsince the user was forewarned, this is presumably what they wanted.\n\n\"git apply --3way\" would either 1. apply cleanly to working dir or\n2. conflict, in which case user's changes would be moved to stage #2\nin cache. For 1 the user's changes are in the cache, so they can check that out\nto restore the original state, since this invocation requires the cache\nand working dir to match. For 2, the user's changes are moved to cache\nin stage #2. Although the changes are preserved, there doesn't seem to\nbe any atomic way to move a cache entry from stage #2 to stage #0.\nSomething like \"git restore --staged --ours file.txt\" seems like it should\nwork, but \"git restore\" doesn't allow combining those flags.\nThe non atomic way we can do is \"git checkout --ours file.txt &&\ngit add file.txt\", this is ok in this case since we've required the index\nand working tree to match.\n\n\"git apply --3way --cached\" would either 1. apply cleanly to the cache or\n2. conflict, and the user's changes are moved to stage #2. In 1, the user's\nchanges are lost because they're combined with the patch, but this is\nthe same as the \"--cached\" case by itself. In 2, the user's changes are\npreserved in stage #2 similar to \"--3way\" by itself. What's somewhat\ntricky here is restoring it to stage #0 since we can't use the working\ntree, but I think that is more of a limitation in \"git restore\", since moving\na cache entry from stage #2 to stage #0 is a conceptually possible and\nsimple operation.\n\nIn summary it seems to me that merge or no merge, the safety semantics\nfor \"--3way\" + \"--cached\" as it is are pretty similar to the existing semantics\nfor those options individually. The user could be preparing to write\na tree out in either the \"--cached\" or the \"--cached --3way\" operation\nso I don't understand why those must differ in safety. In addition, the\nboth \"--3way\" and \"--3way --cached\" perform mergey operations that\nchanges the stages of a file in cache, so I don't understand why those\nmust differ in safety either.\n\n>\n> >> Give an order to the codebase to \"be like so\". Here is my attempt.\n> >>\n> >> Teach \"git apply\" to accept \"--cached\" and \"--3way\" at the same\n> >> time. Only when all changes to all paths involved in the\n> >> application auto-resolve cleanly, the result is placed in the\n> >> index at stage #0 and the command exits with 0 status. If there\n> >> is any path whose conflict cannot be cleanly auto-resolved, the\n> >> original contents from common ancestor (stage #1), our version\n> >> (stage #2) and the contents from the patch (stage #3) for the\n> >> conflicted paths are left at separate stages without any attempt\n> >> to resolve the conflict at the content level, and the command\n> >> exists with non-zero status, because there is no place (like the\n> >> working tree files) to leave a half-resolved conflicted merge\n> >> result to ask the end-user to resolve.\n>\n> I wrote the above as an example to illustrate the tone and the level\n> of details expected in our proposed commit log message. The\n> behaviour it describes may not necessarily match what you have\n> implemented in the patch.\n>\n> For example, imagine that we are applying a patch for two paths,\n> where one auto-resolves cleanly and the other does not. The above\n> description expects both paths will leave the higher stages (instead\n> of recording the auto-resolved path at stage #0, and leaving the\n> other path that cannot be auto-resolved at higher stages) and the\n> command exits with non-zero status, which may not be what you\n> implemented. As an illustration, I didn't necessarily mean such an\n> all-or-none behaviour wrt resolving should be what we implement---I\n> do not want to choose, as this is your itch and I want _you_ with\n> the itch to think long and hard before deciding what the best design\n> for end-users would be, and present it as a proposed solution. An\n> obvious alternative is to record auto-resolved paths at stage #0 and\n> leave only the paths for which auto-resolution failed in conflicted\n> state.\nI missed the \"all changes to all paths\" requirement in that description,\nI'll update it to be more consistent with what it actually does. As you say,\nthe leaving entries at higher orders behavior only happens for conflicting\npaths, not for all paths.\n>\n> Thanks.\n"},{"id":"421106","messageId":"CAMKO5Cv01vFde5XqrM0Niu1jSrhMe=bdmc15KN1Uo8WP9cNKhw@mail.gmail.com","threadId":"55429","inReplyTo":"CAMKO5Ctoa8cf9T0reE9DduC7oX8QgQw-sQH315mQN=KiLDS8ag@mail.gmail.com","subject":"Re: [PATCH V2] git-apply: Allow simultaneous --cached and --3way options","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-04-07T02:25:41Z","receivedAt":"2021-04-07T02:25:57Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"On Tue, Apr 6, 2021 at 2:56 PM Jerry Zhang wrote:\n>\n> On Mon, Apr 5, 2021 at 10:52 PM Junio C Hamano wrote:\n> >\n> > Jerry Zhang writes:\n> >\n> > > Thanks for the comments! I've updated v3 with the changes. Let me know\n> > > if you have any\n> > > more thoughts on whether to block / warn the user before clobbering their cache.\n> >\n> > Please do not top-post on this list.\n> >\n> > I've already said that I think we should ensure the index is clean\n> > by default, because, unlike the case where the application is done\n> > on the working tree files, the use of \"--cached\" is a sign that the\n> > next step is likely to write a tree out. As I've already said so in\n> > earlier reviews, there is nothing more from me to add on that issue.\n> Understood, but please bear with me to explain the risks a bit more. I'm\n> having some difficulty coming up with a name and explanation for flags\n> for this case, because I don't completely understand the safety issue\n> we are trying to mitigate.\n>\n> Let me enumerate some behaviors in 3 different cases where the user\n> has \"file.txt\" changes staged in the index, so index differs from HEAD.\n>\n> \"git apply --cached\" would either 1. combine the patch and cached version\n> and put that in the cache or 2. do nothing (patch failed). In 2 nothing happened\n> so the user's changes are safe. In 1 the user's changes may be gone, but\n> since the user was forewarned, this is presumably what they wanted.\n>\n> \"git apply --3way\" would either 1. apply cleanly to working dir or\n> 2. conflict, in which case user's changes would be moved to stage #2\n> in cache. For 1 the user's changes are in the cache, so they can check that out\n> to restore the original state, since this invocation requires the cache\n> and working dir to match. For 2, the user's changes are moved to cache\n> in stage #2. Although the changes are preserved, there doesn't seem to\n> be any atomic way to move a cache entry from stage #2 to stage #0.\n> Something like \"git restore --staged --ours file.txt\" seems like it should\n> work, but \"git restore\" doesn't allow combining those flags.\n> The non atomic way we can do is \"git checkout --ours file.txt &&\n> git add file.txt\", this is ok in this case since we've required the index\n> and working tree to match.\n>\n> \"git apply --3way --cached\" would either 1. apply cleanly to the cache or\n> 2. conflict, and the user's changes are moved to stage #2. In 1, the user's\n> changes are lost because they're combined with the patch, but this is\n> the same as the \"--cached\" case by itself. In 2, the user's changes are\n> preserved in stage #2 similar to \"--3way\" by itself. What's somewhat\n> tricky here is restoring it to stage #0 since we can't use the working\n> tree, but I think that is more of a limitation in \"git restore\", since moving\n> a cache entry from stage #2 to stage #0 is a conceptually possible and\n> simple operation.\nUpdate: I found out that this can be done through \"git update-index\"\nSay you start with\n```\n100755 adc1032303de8d262868c0a1b85a00fa3b97e9d8 1 file.sh\n100755 d0bf2e33594ea7d9d7352df5511db562de819518 2 file.sh\n100755 e9bf5dfdea804f4f1bcf24988bbc7c3f99991a09 3 file.sh\n```\nPass into \"git update-index --index-info the following\n```\n100755 d0bf2e33594ea7d9d7352df5511db562de819518 0 file.sh\n0 0 1 file.sh\n0 0 2 file.sh\n0 0 3 file.sh\n```\nand you will have atomically moved the \"ours\" stage of file.sh into\nstage #0. This is sort of what I'd expect \"git checkout --ours file.sh\"\nto do, but it doesn't seem to do that.\n>\n> In summary it seems to me that merge or no merge, the safety semantics\n> for \"--3way\" + \"--cached\" as it is are pretty similar to the existing semantics\n> for those options individually. The user could be preparing to write\n> a tree out in either the \"--cached\" or the \"--cached --3way\" operation\n> so I don't understand why those must differ in safety. In addition, the\n> both \"--3way\" and \"--3way --cached\" perform mergey operations that\n> changes the stages of a file in cache, so I don't understand why those\n> must differ in safety either.\n>\n> >\n> > >> Give an order to the codebase to \"be like so\". Here is my attempt.\n> > >>\n> > >> Teach \"git apply\" to accept \"--cached\" and \"--3way\" at the same\n> > >> time. Only when all changes to all paths involved in the\n> > >> application auto-resolve cleanly, the result is placed in the\n> > >> index at stage #0 and the command exits with 0 status. If there\n> > >> is any path whose conflict cannot be cleanly auto-resolved, the\n> > >> original contents from common ancestor (stage #1), our version\n> > >> (stage #2) and the contents from the patch (stage #3) for the\n> > >> conflicted paths are left at separate stages without any attempt\n> > >> to resolve the conflict at the content level, and the command\n> > >> exists with non-zero status, because there is no place (like the\n> > >> working tree files) to leave a half-resolved conflicted merge\n> > >> result to ask the end-user to resolve.\n> >\n> > I wrote the above as an example to illustrate the tone and the level\n> > of details expected in our proposed commit log message. The\n> > behaviour it describes may not necessarily match what you have\n> > implemented in the patch.\n> >\n> > For example, imagine that we are applying a patch for two paths,\n> > where one auto-resolves cleanly and the other does not. The above\n> > description expects both paths will leave the higher stages (instead\n> > of recording the auto-resolved path at stage #0, and leaving the\n> > other path that cannot be auto-resolved at higher stages) and the\n> > command exits with non-zero status, which may not be what you\n> > implemented. As an illustration, I didn't necessarily mean such an\n> > all-or-none behaviour wrt resolving should be what we implement---I\n> > do not want to choose, as this is your itch and I want _you_ with\n> > the itch to think long and hard before deciding what the best design\n> > for end-users would be, and present it as a proposed solution. An\n> > obvious alternative is to record auto-resolved paths at stage #0 and\n> > leave only the paths for which auto-resolution failed in conflicted\n> > state.\n> I missed the \"all changes to all paths\" requirement in that description,\n> I'll update it to be more consistent with what it actually does. As you say,\n> the leaving entries at higher orders behavior only happens for conflicting\n> paths, not for all paths.\n> >\n> > Thanks.\n"},{"id":"421133","messageId":"20210407180349.10173-1-jerry@skydio.com","threadId":"55429","inReplyTo":"20210406024931.24355-1-jerry@skydio.com","subject":"[PATCH v4] git-apply: allow simultaneous --cached and --3way options","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-04-07T18:03:49Z","receivedAt":"2021-04-07T18:03:55Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"\"git apply\" does not allow \"--cached\" and\n\"--3way\" to be used together, since \"--3way\"\nwrites conflict markers into the working tree.\n\nAllow \"git apply\" to accept \"--cached\" and\n\"--3way\" at the same time. When a single file\nauto-resolves cleanly, the result is placed in the\nindex at stage #0 and the command exits with 0\nstatus. For a file that has a conflict which\ncannot be cleanly auto-resolved, the original\ncontents from common ancestor (stage #1), our\nversion (stage #2) and the contents from the\npatch (stage #3) are left at separate stages.\nNo attempt is made to resolve the conflict at\nthe content level, and the command exists with\nnon-zero status, because there is no place\n(like the working tree) to leave a half-resolved\nmerge for the user to resolve.\n\nThe user can use `git diff` to view the contents\nof the conflict, or `git checkout -m -- .` to\nregenerate the conflict markers in the working\ndirectory.\n\nDon't attempt rerere in this case since it depends\non conflict markers written to file for its database\nstorage and lookup. There would be two main changes\nrequired to get rerere working:\n1. Allow the rerere api to accept in memory object\nrather than files, which would allow us to pass in\nthe conflict markers contained in the result from\nll_merge().\n2. Rerere can't write to the working directory, so\nit would have to apply the result to cache stage #0\ndirectly. A flag would be needed to control this.\n\nSigned-off-by: Jerry Zhang \n---\n Documentation/git-apply.txt | 6 ++++--\n apply.c | 7 +++----\n t/t4108-apply-threeway.sh | 24 ++++++++++++++++++++++++\n 3 files changed, 31 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt\nindex 9144575299c264dd299b542b7b5948eef35f211c..aa1ae56a25e0428cabcfa2539900ef2a09abcb7c 100644\n--- a/Documentation/git-apply.txt\n+++ b/Documentation/git-apply.txt\n@@ -87,8 +87,10 @@ OPTIONS\n \tAttempt 3-way merge if the patch records the identity of blobs it is supposed\n \tto apply to and we have those blobs available locally, possibly leaving the\n \tconflict markers in the files in the working tree for the user to\n-\tresolve. This option implies the `--index` option, and is incompatible\n-\twith the `--reject` and the `--cached` options.\n+\tresolve. This option implies the `--index` option unless the\n+\t`--cached` option is used, and is incompatible with the `--reject` option.\n+\tWhen used with the `--cached` option, any conflicts are left at higher stages\n+\tin the cache.\n \n --build-fake-ancestor=::\n \tNewer 'git diff' output has embedded 'index information'\ndiff --git a/apply.c b/apply.c\nindex 9bd4efcbced842d2c5c030a0f2178ddb36114600..0d1e91c88986433052e9b6e67c0dcbd04e6eb703 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -133,8 +133,6 @@ int check_apply_state(struct apply_state *state, int force_apply)\n \n \tif (state->apply_with_reject && state->threeway)\n \t\treturn error(_(\"--reject and --3way cannot be used together.\"));\n-\tif (state->cached && state->threeway)\n-\t\treturn error(_(\"--cached and --3way cannot be used together.\"));\n \tif (state->threeway) {\n \t\tif (is_not_gitdir)\n \t\t\treturn error(_(\"--3way outside a repository\"));\n@@ -4644,8 +4642,9 @@ static int write_out_results(struct apply_state *state, struct patch *list)\n \t\t\t\tfprintf(stderr, \"U %s\\n\", item->string);\n \t\t}\n \t\tstring_list_clear(&cpath, 0);\n-\n-\t\trepo_rerere(state->repo, 0);\n+\t\t/* rerere relies on conflict markers which aren't written with --cached */\n+\t\tif (!state->cached)\n+\t\t\trepo_rerere(state->repo, 0);\n \t}\n \n \treturn errs;\ndiff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh\nindex 9ff313f976422f9c12dc8032d14567b54cfe3765..37ba4f6fa201c49a4bf2882d6b8345c1c2bedf0c 100755\n--- a/t/t4108-apply-threeway.sh\n+++ b/t/t4108-apply-threeway.sh\n@@ -180,4 +180,28 @@ test_expect_success 'apply -3 with ambiguous repeating file' '\n \ttest_cmp expect one_two_repeat\n '\n \n+test_expect_success 'apply with --3way --cached' '\n+\t# Merging side should be similar to applying this patch\n+\tgit diff ...side >P.diff &&\n+\n+\t# The corresponding conflicted merge\n+\tgit reset --hard &&\n+\tgit checkout main^0 &&\n+\ttest_must_fail git merge --no-commit side &&\n+\tgit ls-files -s >expect.ls &&\n+\n+\t# should fail to apply\n+\tgit reset --hard &&\n+\tgit checkout main^0 &&\n+\ttest_must_fail git apply --cached --3way P.diff &&\n+\tgit ls-files -s >actual.ls &&\n+\tprint_sanitized_conflicted_diff >actual.diff &&\n+\n+\t# The cache should resemble the corresponding merge\n+\ttest_cmp expect.ls actual.ls &&\n+\t# However the working directory should not change\n+\t>expect.diff &&\n+\ttest_cmp expect.diff actual.diff\n+'\n+\n test_done\n-- \n2.29.0\n\n"},{"id":"421136","messageId":"xmqqzgy9zzr0.fsf@gitster.g","threadId":"55429","inReplyTo":"20210407180349.10173-1-jerry@skydio.com","subject":"Re: [PATCH v4] git-apply: allow simultaneous --cached and --3way options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-07T19:00:19Z","receivedAt":"2021-04-07T19:00:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jerry Zhang writes:\n\n> \"git apply\" does not allow \"--cached\" and\n> \"--3way\" to be used together, since \"--3way\"\n> writes conflict markers into the working tree.\n>\n> Allow \"git apply\" to accept \"--cached\" and\n> \"--3way\" at the same time. When a single file\n> auto-resolves cleanly, the result is placed in the\n> index at stage #0 and the command exits with 0\n> status. For a file that has a conflict which\n> cannot be cleanly auto-resolved, the original\n> contents from common ancestor (stage #1), our\n> version (stage #2) and the contents from the\n> patch (stage #3) are left at separate stages.\n> No attempt is made to resolve the conflict at\n> the content level, and the command exists with\n> non-zero status, because there is no place\n> (like the working tree) to leave a half-resolved\n> merge for the user to resolve.\n>\n> The user can use `git diff` to view the contents\n> of the conflict, or `git checkout -m -- .` to\n> regenerate the conflict markers in the working\n> directory.\n>\n> Don't attempt rerere in this case since it depends\n> on conflict markers written to file for its database\n> storage and lookup. There would be two main changes\n> required to get rerere working:\n> 1. Allow the rerere api to accept in memory object\n> rather than files, which would allow us to pass in\n> the conflict markers contained in the result from\n> ll_merge().\n> 2. Rerere can't write to the working directory, so\n> it would have to apply the result to cache stage #0\n> directly. A flag would be needed to control this.\n>\n> Signed-off-by: Jerry Zhang \n> ---\n\nFor future reference, please summarize what changed between v3 and\nv4 in this space immediately after the three-dash line. This is\nespecially helpful when sending v4 so soon after v3 that nobody had\na chance to review and respond to v3, as it helps reviewers to\ndecide if it is safe to skip v3 and jump directly to v4 to start\nreading.\n\n> Documentation/git-apply.txt | 6 ++++--\n> apply.c | 7 +++----\n> t/t4108-apply-threeway.sh | 24 ++++++++++++++++++++++++\n> 3 files changed, 31 insertions(+), 6 deletions(-)\n>\n> diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt\n> index 9144575299c264dd299b542b7b5948eef35f211c..aa1ae56a25e0428cabcfa2539900ef2a09abcb7c 100644\n> --- a/Documentation/git-apply.txt\n> +++ b/Documentation/git-apply.txt\n> @@ -87,8 +87,10 @@ OPTIONS\n> \tAttempt 3-way merge if the patch records the identity of blobs it is supposed\n> \tto apply to and we have those blobs available locally, possibly leaving the\n> \tconflict markers in the files in the working tree for the user to\n> -\tresolve. This option implies the `--index` option, and is incompatible\n> -\twith the `--reject` and the `--cached` options.\n> +\tresolve. This option implies the `--index` option unless the\n> +\t`--cached` option is used, and is incompatible with the `--reject` option.\n> +\tWhen used with the `--cached` option, any conflicts are left at higher stages\n> +\tin the cache.\n\nAlso for future reference.\n\nIt is clear to me (from the pre-context lines of the above hunk)\nthat this change wants to depend on the other \"3way-first\" topic,\nbecause I reviewed the other topic.\n\nBut it would not be too much trouble to say \"this builds on the\njz/apply-run-3way-first topic 923cd87a (git-apply: try threeway\nfirst when \"--3way\" is used, 2021-04-06)\". When potential reviewers\nare tempted to apply this and try it out while reviewing, such a\nnote would help them. And as a patch author, you would want to\nincrease the chance that your patch gets reviewed, so any help you\ngive to potential reviewers would help you.\n\nThe space between the three-dash line and the diffstat is the place\nto write it.\n\n> diff --git a/apply.c b/apply.c\n> index 9bd4efcbced842d2c5c030a0f2178ddb36114600..0d1e91c88986433052e9b6e67c0dcbd04e6eb703 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -133,8 +133,6 @@ int check_apply_state(struct apply_state *state, int force_apply)\n> \n> \tif (state->apply_with_reject && state->threeway)\n> \t\treturn error(_(\"--reject and --3way cannot be used together.\"));\n> -\tif (state->cached && state->threeway)\n> -\t\treturn error(_(\"--cached and --3way cannot be used together.\"));\n> \tif (state->threeway) {\n> \t\tif (is_not_gitdir)\n> \t\t\treturn error(_(\"--3way outside a repository\"));\n> @@ -4644,8 +4642,9 @@ static int write_out_results(struct apply_state *state, struct patch *list)\n> \t\t\t\tfprintf(stderr, \"U %s\\n\", item->string);\n> \t\t}\n> \t\tstring_list_clear(&cpath, 0);\n> -\n> -\t\trepo_rerere(state->repo, 0);\n> +\t\t/* rerere relies on conflict markers which aren't written with --cached */\n\nA minor nit. It is not just \"conflict markers\" that rerere wants.\nIt wants a intermediate half-merged result \"in the working tree\",\nbecause it does not work with in-core copy. So\n\n /*\n * With --cached, we do not write conflicted file to the\n * working tree, so cannot use rerere to reuse previous\n\t\t * resolution.\n */\n\nor something, perhaps.\n\n> +\t\tif (!state->cached)\n> +\t\t\trepo_rerere(state->repo, 0);\n> \t}\n> \n> \treturn errs;\n> diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh\n> index 9ff313f976422f9c12dc8032d14567b54cfe3765..37ba4f6fa201c49a4bf2882d6b8345c1c2bedf0c 100755\n> --- a/t/t4108-apply-threeway.sh\n> +++ b/t/t4108-apply-threeway.sh\n> @@ -180,4 +180,28 @@ test_expect_success 'apply -3 with ambiguous repeating file' '\n> \ttest_cmp expect one_two_repeat\n> '\n> \n> +test_expect_success 'apply with --3way --cached' '\n> +\t# Merging side should be similar to applying this patch\n> +\tgit diff ...side >P.diff &&\n> +\n> +\t# The corresponding conflicted merge\n> +\tgit reset --hard &&\n> +\tgit checkout main^0 &&\n> +\ttest_must_fail git merge --no-commit side &&\n> +\tgit ls-files -s >expect.ls &&\n> +\n> +\t# should fail to apply\n> +\tgit reset --hard &&\n> +\tgit checkout main^0 &&\n> +\ttest_must_fail git apply --cached --3way P.diff &&\n> +\tgit ls-files -s >actual.ls &&\n> +\tprint_sanitized_conflicted_diff >actual.diff &&\n> +\n> +\t# The cache should resemble the corresponding merge\n> +\ttest_cmp expect.ls actual.ls &&\n> +\t# However the working directory should not change\n> +\t>expect.diff &&\n> +\ttest_cmp expect.diff actual.diff\n> +'\n\nInteresting. I would have expected \"ls-files -u\" would be used, but\nusing \"-s\" to see the stage #0 entries is more thorough.\n\nThe above is only about a failing case, which is of course an\nimportant case to validate, but don't we also want to check a\nsuccessful case, and a case where two paths are touched, and one\napplies cleanly while the other conflicts?\n\nThanks.\n"},{"id":"421192","messageId":"20210408021344.8053-1-jerry@skydio.com","threadId":"55429","inReplyTo":"20210407180349.10173-1-jerry@skydio.com","subject":"[PATCH v5] git-apply: allow simultaneous --cached and --3way options","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2021-04-08T02:13:44Z","receivedAt":"2021-04-08T02:13:50Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"\"git apply\" does not allow \"--cached\" and\n\"--3way\" to be used together, since \"--3way\"\nwrites conflict markers into the working tree.\n\nAllow \"git apply\" to accept \"--cached\" and\n\"--3way\" at the same time. When a single file\nauto-resolves cleanly, the result is placed in the\nindex at stage #0 and the command exits with 0\nstatus. For a file that has a conflict which\ncannot be cleanly auto-resolved, the original\ncontents from common ancestor (stage #1), our\nversion (stage #2) and the contents from the\npatch (stage #3) are left at separate stages.\nNo attempt is made to resolve the conflict at\nthe content level, and the command exists with\nnon-zero status, because there is no place\n(like the working tree) to leave a half-resolved\nmerge for the user to resolve.\n\nThe user can use `git diff` to view the contents\nof the conflict, or `git checkout -m -- .` to\nregenerate the conflict markers in the working\ndirectory.\n\nDon't attempt rerere in this case since it depends\non conflict markers written to file for its database\nstorage and lookup. There would be two main changes\nrequired to get rerere working:\n1. Allow the rerere api to accept in memory object\nrather than files, which would allow us to pass in\nthe conflict markers contained in the result from\nll_merge().\n2. Rerere can't write to the working directory, so\nit would have to apply the result to cache stage #0\ndirectly. A flag would be needed to control this.\n\nSigned-off-by: Jerry Zhang \n---\nPatch applies on top of\n\"[PATCH v2] git-apply: try threeway first when \"--3way\"\"\nMain merge conflict is the addition of multiple\ntests at the bottom of the file.\n\nv4->v5:\n\nUpdated in file comment about rerere\nAdded test for cleanly applying patch (should return 0)\nPrevious test captured case where patch fails to apply\ndue to 1 conflicting file and 1 cleanly applying file\n(returns 1).\n\n Documentation/git-apply.txt | 6 +++--\n apply.c | 9 ++++---\n t/t4108-apply-threeway.sh | 50 +++++++++++++++++++++++++++++++++++++\n 3 files changed, 59 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt\nindex 9144575299c264dd299b542b7b5948eef35f211c..aa1ae56a25e0428cabcfa2539900ef2a09abcb7c 100644\n--- a/Documentation/git-apply.txt\n+++ b/Documentation/git-apply.txt\n@@ -87,8 +87,10 @@ OPTIONS\n \tAttempt 3-way merge if the patch records the identity of blobs it is supposed\n \tto apply to and we have those blobs available locally, possibly leaving the\n \tconflict markers in the files in the working tree for the user to\n-\tresolve. This option implies the `--index` option, and is incompatible\n-\twith the `--reject` and the `--cached` options.\n+\tresolve. This option implies the `--index` option unless the\n+\t`--cached` option is used, and is incompatible with the `--reject` option.\n+\tWhen used with the `--cached` option, any conflicts are left at higher stages\n+\tin the cache.\n \n --build-fake-ancestor=::\n \tNewer 'git diff' output has embedded 'index information'\ndiff --git a/apply.c b/apply.c\nindex 9bd4efcbced842d2c5c030a0f2178ddb36114600..dadab80ec967357b031657d4e3d0ae52fac11411 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -133,8 +133,6 @@ int check_apply_state(struct apply_state *state, int force_apply)\n \n \tif (state->apply_with_reject && state->threeway)\n \t\treturn error(_(\"--reject and --3way cannot be used together.\"));\n-\tif (state->cached && state->threeway)\n-\t\treturn error(_(\"--cached and --3way cannot be used together.\"));\n \tif (state->threeway) {\n \t\tif (is_not_gitdir)\n \t\t\treturn error(_(\"--3way outside a repository\"));\n@@ -4644,8 +4642,11 @@ static int write_out_results(struct apply_state *state, struct patch *list)\n \t\t\t\tfprintf(stderr, \"U %s\\n\", item->string);\n \t\t}\n \t\tstring_list_clear(&cpath, 0);\n-\n-\t\trepo_rerere(state->repo, 0);\n+\t\t/* Rerere relies on the partially merged result being in the working tree\n+\t\t * with conflict markers, but that isn't written with --cached.\n+\t\t */\n+\t\tif (!state->cached)\n+\t\t\trepo_rerere(state->repo, 0);\n \t}\n \n \treturn errs;\ndiff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh\nindex 9ff313f976422f9c12dc8032d14567b54cfe3765..65147efdea9a00e30d156e6f4d5d72a3987f230d 100755\n--- a/t/t4108-apply-threeway.sh\n+++ b/t/t4108-apply-threeway.sh\n@@ -180,4 +180,54 @@ test_expect_success 'apply -3 with ambiguous repeating file' '\n \ttest_cmp expect one_two_repeat\n '\n \n+test_expect_success 'apply with --3way --cached clean apply' '\n+\t# Merging side should be similar to applying this patch\n+\tgit diff ...side >P.diff &&\n+\n+\t# The corresponding cleanly applied merge\n+\tgit reset --hard &&\n+\tgit checkout main~ &&\n+\tgit merge --no-commit side &&\n+\tgit ls-files -s >expect.ls &&\n+\n+\t# should succeed\n+\tgit reset --hard &&\n+\tgit checkout main~ &&\n+\tgit apply --cached --3way P.diff &&\n+\tgit ls-files -s >actual.ls &&\n+\tprint_sanitized_conflicted_diff >actual.diff &&\n+\n+\t# The cache should resemble the corresponding merge\n+\t# (both files at stage #0)\n+\ttest_cmp expect.ls actual.ls &&\n+\t# However the working directory should not change\n+\t>expect.diff &&\n+\ttest_cmp expect.diff actual.diff\n+'\n+\n+test_expect_success 'apply with --3way --cached and conflicts' '\n+\t# Merging side should be similar to applying this patch\n+\tgit diff ...side >P.diff &&\n+\n+\t# The corresponding conflicted merge\n+\tgit reset --hard &&\n+\tgit checkout main^0 &&\n+\ttest_must_fail git merge --no-commit side &&\n+\tgit ls-files -s >expect.ls &&\n+\n+\t# should fail to apply\n+\tgit reset --hard &&\n+\tgit checkout main^0 &&\n+\ttest_must_fail git apply --cached --3way P.diff &&\n+\tgit ls-files -s >actual.ls &&\n+\tprint_sanitized_conflicted_diff >actual.diff &&\n+\n+\t# The cache should resemble the corresponding merge\n+\t# (one file at stage #0, one file at stages #1 #2 #3)\n+\ttest_cmp expect.ls actual.ls &&\n+\t# However the working directory should not change\n+\t>expect.diff &&\n+\ttest_cmp expect.diff actual.diff\n+'\n+\n test_done\n-- \n2.29.0\n\n"},{"id":"421211","messageId":"xmqqh7kgvr3i.fsf@gitster.g","threadId":"55429","inReplyTo":"20210408021344.8053-1-jerry@skydio.com","subject":"Re: [PATCH v5] git-apply: allow simultaneous --cached and --3way options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-08T13:33:05Z","receivedAt":"2021-04-08T13:33:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jerry Zhang writes:\n\n> Documentation/git-apply.txt | 6 +++--\n> apply.c | 9 ++++---\n> t/t4108-apply-threeway.sh | 50 +++++++++++++++++++++++++++++++++++++\n> 3 files changed, 59 insertions(+), 6 deletions(-)\n\nNicely done. Will queue.\n\nElijah, how does this round look to you?\n\n> diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt\n> index 9144575299c264dd299b542b7b5948eef35f211c..aa1ae56a25e0428cabcfa2539900ef2a09abcb7c 100644\n> --- a/Documentation/git-apply.txt\n> +++ b/Documentation/git-apply.txt\n> @@ -87,8 +87,10 @@ OPTIONS\n> \tAttempt 3-way merge if the patch records the identity of blobs it is supposed\n> \tto apply to and we have those blobs available locally, possibly leaving the\n> \tconflict markers in the files in the working tree for the user to\n> -\tresolve. This option implies the `--index` option, and is incompatible\n> -\twith the `--reject` and the `--cached` options.\n> +\tresolve. This option implies the `--index` option unless the\n> +\t`--cached` option is used, and is incompatible with the `--reject` option.\n> +\tWhen used with the `--cached` option, any conflicts are left at higher stages\n> +\tin the cache.\n> \n> --build-fake-ancestor=::\n> \tNewer 'git diff' output has embedded 'index information'\n> diff --git a/apply.c b/apply.c\n> index 9bd4efcbced842d2c5c030a0f2178ddb36114600..dadab80ec967357b031657d4e3d0ae52fac11411 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -133,8 +133,6 @@ int check_apply_state(struct apply_state *state, int force_apply)\n> \n> \tif (state->apply_with_reject && state->threeway)\n> \t\treturn error(_(\"--reject and --3way cannot be used together.\"));\n> -\tif (state->cached && state->threeway)\n> -\t\treturn error(_(\"--cached and --3way cannot be used together.\"));\n> \tif (state->threeway) {\n> \t\tif (is_not_gitdir)\n> \t\t\treturn error(_(\"--3way outside a repository\"));\n> @@ -4644,8 +4642,11 @@ static int write_out_results(struct apply_state *state, struct patch *list)\n> \t\t\t\tfprintf(stderr, \"U %s\\n\", item->string);\n> \t\t}\n> \t\tstring_list_clear(&cpath, 0);\n> -\n> -\t\trepo_rerere(state->repo, 0);\n> +\t\t/* Rerere relies on the partially merged result being in the working tree\n> +\t\t * with conflict markers, but that isn't written with --cached.\n> +\t\t */\n> +\t\tif (!state->cached)\n> +\t\t\trepo_rerere(state->repo, 0);\n> \t}\n> \n> \treturn errs;\n> diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh\n> index 9ff313f976422f9c12dc8032d14567b54cfe3765..65147efdea9a00e30d156e6f4d5d72a3987f230d 100755\n> --- a/t/t4108-apply-threeway.sh\n> +++ b/t/t4108-apply-threeway.sh\n> @@ -180,4 +180,54 @@ test_expect_success 'apply -3 with ambiguous repeating file' '\n> \ttest_cmp expect one_two_repeat\n> '\n> \n> +test_expect_success 'apply with --3way --cached clean apply' '\n> +\t# Merging side should be similar to applying this patch\n> +\tgit diff ...side >P.diff &&\n> +\n> +\t# The corresponding cleanly applied merge\n> +\tgit reset --hard &&\n> +\tgit checkout main~ &&\n> +\tgit merge --no-commit side &&\n> +\tgit ls-files -s >expect.ls &&\n> +\n> +\t# should succeed\n> +\tgit reset --hard &&\n> +\tgit checkout main~ &&\n> +\tgit apply --cached --3way P.diff &&\n> +\tgit ls-files -s >actual.ls &&\n> +\tprint_sanitized_conflicted_diff >actual.diff &&\n> +\n> +\t# The cache should resemble the corresponding merge\n> +\t# (both files at stage #0)\n> +\ttest_cmp expect.ls actual.ls &&\n> +\t# However the working directory should not change\n> +\t>expect.diff &&\n> +\ttest_cmp expect.diff actual.diff\n> +'\n> +\n> +test_expect_success 'apply with --3way --cached and conflicts' '\n> +\t# Merging side should be similar to applying this patch\n> +\tgit diff ...side >P.diff &&\n> +\n> +\t# The corresponding conflicted merge\n> +\tgit reset --hard &&\n> +\tgit checkout main^0 &&\n> +\ttest_must_fail git merge --no-commit side &&\n> +\tgit ls-files -s >expect.ls &&\n> +\n> +\t# should fail to apply\n> +\tgit reset --hard &&\n> +\tgit checkout main^0 &&\n> +\ttest_must_fail git apply --cached --3way P.diff &&\n> +\tgit ls-files -s >actual.ls &&\n> +\tprint_sanitized_conflicted_diff >actual.diff &&\n> +\n> +\t# The cache should resemble the corresponding merge\n> +\t# (one file at stage #0, one file at stages #1 #2 #3)\n> +\ttest_cmp expect.ls actual.ls &&\n> +\t# However the working directory should not change\n> +\t>expect.diff &&\n> +\ttest_cmp expect.diff actual.diff\n> +'\n> +\n> test_done\n"},{"id":"421701","messageId":"CABPp-BGFjZajiEMcJ7-WMPNaHJd3_eA3g1Wc-5HzBZMuA_7h+Q@mail.gmail.com","threadId":"55429","inReplyTo":"20210408021344.8053-1-jerry@skydio.com","subject":"Re: [PATCH v5] git-apply: allow simultaneous --cached and --3way options","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-04-12T15:40:52Z","receivedAt":"2021-04-12T15:41:07Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Apr 7, 2021 at 7:13 PM Jerry Zhang wrote:\n>\n> \"git apply\" does not allow \"--cached\" and\n> \"--3way\" to be used together, since \"--3way\"\n> writes conflict markers into the working tree.\n>\n> Allow \"git apply\" to accept \"--cached\" and\n> \"--3way\" at the same time. When a single file\n> auto-resolves cleanly, the result is placed in the\n> index at stage #0 and the command exits with 0\n> status.\n\nShould this instead read:\n \"...placed in the index at stage #0. If all files auto-resolve\ncleanly, the command exits with 0 status.\"\nor something like that?\n\n> For a file that has a conflict which\n> cannot be cleanly auto-resolved, the original\n> contents from common ancestor (stage #1), our\n> version (stage #2) and the contents from the\n> patch (stage #3) are left at separate stages.\n> No attempt is made to resolve the conflict at\n> the content level, and the command exists with\n\ns/exists/exits/\n\n> non-zero status, because there is no place\n> (like the working tree) to leave a half-resolved\n> merge for the user to resolve.\n>\n> The user can use `git diff` to view the contents\n> of the conflict, or `git checkout -m -- .` to\n> regenerate the conflict markers in the working\n> directory.\n>\n> Don't attempt rerere in this case since it depends\n> on conflict markers written to file for its database\n> storage and lookup. There would be two main changes\n> required to get rerere working:\n> 1. Allow the rerere api to accept in memory object\n> rather than files, which would allow us to pass in\n> the conflict markers contained in the result from\n> ll_merge().\n> 2. Rerere can't write to the working directory, so\n> it would have to apply the result to cache stage #0\n> directly. A flag would be needed to control this.\n>\n> Signed-off-by: Jerry Zhang \n> ---\n> Patch applies on top of\n> \"[PATCH v2] git-apply: try threeway first when \"--3way\"\"\n> Main merge conflict is the addition of multiple\n> tests at the bottom of the file.\n>\n> v4->v5:\n>\n> Updated in file comment about rerere\n> Added test for cleanly applying patch (should return 0)\n> Previous test captured case where patch fails to apply\n> due to 1 conflicting file and 1 cleanly applying file\n> (returns 1).\n>\n> Documentation/git-apply.txt | 6 +++--\n> apply.c | 9 ++++---\n> t/t4108-apply-threeway.sh | 50 +++++++++++++++++++++++++++++++++++++\n> 3 files changed, 59 insertions(+), 6 deletions(-)\n>\n> diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt\n> index 9144575299c264dd299b542b7b5948eef35f211c..aa1ae56a25e0428cabcfa2539900ef2a09abcb7c 100644\n> --- a/Documentation/git-apply.txt\n> +++ b/Documentation/git-apply.txt\n> @@ -87,8 +87,10 @@ OPTIONS\n> Attempt 3-way merge if the patch records the identity of blobs it is supposed\n> to apply to and we have those blobs available locally, possibly leaving the\n> conflict markers in the files in the working tree for the user to\n> - resolve. This option implies the `--index` option, and is incompatible\n> - with the `--reject` and the `--cached` options.\n> + resolve. This option implies the `--index` option unless the\n> + `--cached` option is used, and is incompatible with the `--reject` option.\n> + When used with the `--cached` option, any conflicts are left at higher stages\n> + in the cache.\n>\n> --build-fake-ancestor=::\n> Newer 'git diff' output has embedded 'index information'\n> diff --git a/apply.c b/apply.c\n> index 9bd4efcbced842d2c5c030a0f2178ddb36114600..dadab80ec967357b031657d4e3d0ae52fac11411 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -133,8 +133,6 @@ int check_apply_state(struct apply_state *state, int force_apply)\n>\n> if (state->apply_with_reject && state->threeway)\n> return error(_(\"--reject and --3way cannot be used together.\"));\n> - if (state->cached && state->threeway)\n> - return error(_(\"--cached and --3way cannot be used together.\"));\n> if (state->threeway) {\n> if (is_not_gitdir)\n> return error(_(\"--3way outside a repository\"));\n> @@ -4644,8 +4642,11 @@ static int write_out_results(struct apply_state *state, struct patch *list)\n> fprintf(stderr, \"U %s\\n\", item->string);\n> }\n> string_list_clear(&cpath, 0);\n> -\n> - repo_rerere(state->repo, 0);\n> + /* Rerere relies on the partially merged result being in the working tree\n> + * with conflict markers, but that isn't written with --cached.\n> + */\n> + if (!state->cached)\n> + repo_rerere(state->repo, 0);\n> }\n>\n> return errs;\n> diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh\n> index 9ff313f976422f9c12dc8032d14567b54cfe3765..65147efdea9a00e30d156e6f4d5d72a3987f230d 100755\n> --- a/t/t4108-apply-threeway.sh\n> +++ b/t/t4108-apply-threeway.sh\n> @@ -180,4 +180,54 @@ test_expect_success 'apply -3 with ambiguous repeating file' '\n> test_cmp expect one_two_repeat\n> '\n>\n> +test_expect_success 'apply with --3way --cached clean apply' '\n> + # Merging side should be similar to applying this patch\n> + git diff ...side >P.diff &&\n> +\n> + # The corresponding cleanly applied merge\n> + git reset --hard &&\n> + git checkout main~ &&\n> + git merge --no-commit side &&\n> + git ls-files -s >expect.ls &&\n> +\n> + # should succeed\n> + git reset --hard &&\n> + git checkout main~ &&\n> + git apply --cached --3way P.diff &&\n> + git ls-files -s >actual.ls &&\n> + print_sanitized_conflicted_diff >actual.diff &&\n> +\n> + # The cache should resemble the corresponding merge\n> + # (both files at stage #0)\n> + test_cmp expect.ls actual.ls &&\n> + # However the working directory should not change\n> + >expect.diff &&\n> + test_cmp expect.diff actual.diff\n> +'\n> +\n> +test_expect_success 'apply with --3way --cached and conflicts' '\n> + # Merging side should be similar to applying this patch\n> + git diff ...side >P.diff &&\n> +\n> + # The corresponding conflicted merge\n> + git reset --hard &&\n> + git checkout main^0 &&\n> + test_must_fail git merge --no-commit side &&\n> + git ls-files -s >expect.ls &&\n> +\n> + # should fail to apply\n> + git reset --hard &&\n> + git checkout main^0 &&\n> + test_must_fail git apply --cached --3way P.diff &&\n> + git ls-files -s >actual.ls &&\n> + print_sanitized_conflicted_diff >actual.diff &&\n> +\n> + # The cache should resemble the corresponding merge\n> + # (one file at stage #0, one file at stages #1 #2 #3)\n> + test_cmp expect.ls actual.ls &&\n> + # However the working directory should not change\n> + >expect.diff &&\n> + test_cmp expect.diff actual.diff\n> +'\n> +\n> test_done\n> --\n> 2.29.0\n\nOtherwise, looks good to me.\n"},{"id":"421702","messageId":"CABPp-BEmZrK9ambLHL=ryjRM22zqGn6vzc+2aGoy=x-Z3mwUdQ@mail.gmail.com","threadId":"55429","inReplyTo":"xmqqh7kgvr3i.fsf@gitster.g","subject":"Re: [PATCH v5] git-apply: allow simultaneous --cached and --3way options","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-04-12T15:45:31Z","receivedAt":"2021-04-12T15:45:47Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Apr 8, 2021 at 6:33 AM Junio C Hamano wrote:\n>\n> Jerry Zhang writes:\n>\n> > Documentation/git-apply.txt | 6 +++--\n> > apply.c | 9 ++++---\n> > t/t4108-apply-threeway.sh | 50 +++++++++++++++++++++++++++++++++++++\n> > 3 files changed, 59 insertions(+), 6 deletions(-)\n>\n> Nicely done. Will queue.\n>\n> Elijah, how does this round look to you?\n\nSorry for the delay; modulo two minor issues with the commit message\nit looks good to me.\n\nThis change won't allow git-apply to handle upstream renames, and\nmakes me wonder if we should lift the fall_back_threeway() logic out\nof builtin/am.c and use it here. This change also makes me wonder if\nwe should change git-am's --3way flag to make it not be treated as a\nfallback to be consistent with what we are doing here. But neither of\nthose changes need to be part of this patch.\n\n> > diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt\n> > index 9144575299c264dd299b542b7b5948eef35f211c..aa1ae56a25e0428cabcfa2539900ef2a09abcb7c 100644\n> > --- a/Documentation/git-apply.txt\n> > +++ b/Documentation/git-apply.txt\n> > @@ -87,8 +87,10 @@ OPTIONS\n> > Attempt 3-way merge if the patch records the identity of blobs it is supposed\n> > to apply to and we have those blobs available locally, possibly leaving the\n> > conflict markers in the files in the working tree for the user to\n> > - resolve. This option implies the `--index` option, and is incompatible\n> > - with the `--reject` and the `--cached` options.\n> > + resolve. This option implies the `--index` option unless the\n> > + `--cached` option is used, and is incompatible with the `--reject` option.\n> > + When used with the `--cached` option, any conflicts are left at higher stages\n> > + in the cache.\n> >\n> > --build-fake-ancestor=::\n> > Newer 'git diff' output has embedded 'index information'\n> > diff --git a/apply.c b/apply.c\n> > index 9bd4efcbced842d2c5c030a0f2178ddb36114600..dadab80ec967357b031657d4e3d0ae52fac11411 100644\n> > --- a/apply.c\n> > +++ b/apply.c\n> > @@ -133,8 +133,6 @@ int check_apply_state(struct apply_state *state, int force_apply)\n> >\n> > if (state->apply_with_reject && state->threeway)\n> > return error(_(\"--reject and --3way cannot be used together.\"));\n> > - if (state->cached && state->threeway)\n> > - return error(_(\"--cached and --3way cannot be used together.\"));\n> > if (state->threeway) {\n> > if (is_not_gitdir)\n> > return error(_(\"--3way outside a repository\"));\n> > @@ -4644,8 +4642,11 @@ static int write_out_results(struct apply_state *state, struct patch *list)\n> > fprintf(stderr, \"U %s\\n\", item->string);\n> > }\n> > string_list_clear(&cpath, 0);\n> > -\n> > - repo_rerere(state->repo, 0);\n> > + /* Rerere relies on the partially merged result being in the working tree\n> > + * with conflict markers, but that isn't written with --cached.\n> > + */\n> > + if (!state->cached)\n> > + repo_rerere(state->repo, 0);\n> > }\n> >\n> > return errs;\n> > diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh\n> > index 9ff313f976422f9c12dc8032d14567b54cfe3765..65147efdea9a00e30d156e6f4d5d72a3987f230d 100755\n> > --- a/t/t4108-apply-threeway.sh\n> > +++ b/t/t4108-apply-threeway.sh\n> > @@ -180,4 +180,54 @@ test_expect_success 'apply -3 with ambiguous repeating file' '\n> > test_cmp expect one_two_repeat\n> > '\n> >\n> > +test_expect_success 'apply with --3way --cached clean apply' '\n> > + # Merging side should be similar to applying this patch\n> > + git diff ...side >P.diff &&\n> > +\n> > + # The corresponding cleanly applied merge\n> > + git reset --hard &&\n> > + git checkout main~ &&\n> > + git merge --no-commit side &&\n> > + git ls-files -s >expect.ls &&\n> > +\n> > + # should succeed\n> > + git reset --hard &&\n> > + git checkout main~ &&\n> > + git apply --cached --3way P.diff &&\n> > + git ls-files -s >actual.ls &&\n> > + print_sanitized_conflicted_diff >actual.diff &&\n> > +\n> > + # The cache should resemble the corresponding merge\n> > + # (both files at stage #0)\n> > + test_cmp expect.ls actual.ls &&\n> > + # However the working directory should not change\n> > + >expect.diff &&\n> > + test_cmp expect.diff actual.diff\n> > +'\n> > +\n> > +test_expect_success 'apply with --3way --cached and conflicts' '\n> > + # Merging side should be similar to applying this patch\n> > + git diff ...side >P.diff &&\n> > +\n> > + # The corresponding conflicted merge\n> > + git reset --hard &&\n> > + git checkout main^0 &&\n> > + test_must_fail git merge --no-commit side &&\n> > + git ls-files -s >expect.ls &&\n> > +\n> > + # should fail to apply\n> > + git reset --hard &&\n> > + git checkout main^0 &&\n> > + test_must_fail git apply --cached --3way P.diff &&\n> > + git ls-files -s >actual.ls &&\n> > + print_sanitized_conflicted_diff >actual.diff &&\n> > +\n> > + # The cache should resemble the corresponding merge\n> > + # (one file at stage #0, one file at stages #1 #2 #3)\n> > + test_cmp expect.ls actual.ls &&\n> > + # However the working directory should not change\n> > + >expect.diff &&\n> > + test_cmp expect.diff actual.diff\n> > +'\n> > +\n> > test_done\n"},{"id":"421745","messageId":"xmqq5z0re4vc.fsf@gitster.g","threadId":"55429","inReplyTo":"CABPp-BEmZrK9ambLHL=ryjRM22zqGn6vzc+2aGoy=x-Z3mwUdQ@mail.gmail.com","subject":"Re: [PATCH v5] git-apply: allow simultaneous --cached and --3way options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-12T18:26:31Z","receivedAt":"2021-04-12T18:26:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> Sorry for the delay; modulo two minor issues with the commit message\n> it looks good to me.\n\nThanks.\n"},{"id":"421746","messageId":"xmqq1rbfe4u9.fsf@gitster.g","threadId":"55429","inReplyTo":"CABPp-BGFjZajiEMcJ7-WMPNaHJd3_eA3g1Wc-5HzBZMuA_7h+Q@mail.gmail.com","subject":"Re: [PATCH v5] git-apply: allow simultaneous --cached and --3way options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-12T18:27:10Z","receivedAt":"2021-04-12T18:27:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> On Wed, Apr 7, 2021 at 7:13 PM Jerry Zhang wrote:\n>>\n>> \"git apply\" does not allow \"--cached\" and\n>> \"--3way\" to be used together, since \"--3way\"\n>> writes conflict markers into the working tree.\n>>\n>> Allow \"git apply\" to accept \"--cached\" and\n>> \"--3way\" at the same time. When a single file\n>> auto-resolves cleanly, the result is placed in the\n>> index at stage #0 and the command exits with 0\n>> status.\n>\n> Should this instead read:\n> \"...placed in the index at stage #0. If all files auto-resolve\n> cleanly, the command exits with 0 status.\"\n> or something like that?\n\nPerhaps.\n\n>> For a file that has a conflict which\n>> cannot be cleanly auto-resolved, the original\n>> contents from common ancestor (stage #1), our\n>> version (stage #2) and the contents from the\n>> patch (stage #3) are left at separate stages.\n>> No attempt is made to resolve the conflict at\n>> the content level, and the command exists with\n>\n> s/exists/exits/\n\nWill squash it in.\n"}]}