{"thread":{"id":"60038","subject":"[PATCH 1/2] checkout/restore: refuse unmerging paths unless checking out of the index","startedAt":"2023-07-28T22:06:35Z","lastAt":"2023-07-28T22:07:45Z","messageCount":2,"participants":["Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"479976","messageId":"xmqqedkr4rod.fsf@gitster.g","threadId":"60038","inReplyTo":null,"subject":"[PATCH 1/2] checkout/restore: refuse unmerging paths unless checking out of the index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-28T22:06:26Z","receivedAt":"2023-07-28T22:06:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Recreating unmerged index entries using resolve-undo data,\nrecreating conflicted working tree files using unmerged index\nentries, and writing data out of unmerged index entries, make\nsense only when we are checking paths out of the index and not when\nwe are checking paths out of a tree-ish.\n\nAdd an extra check to make sure \"--merge\" and \"--ours/--theirs\"\noptions are rejected when checking out from a tree-ish, update the\ndocument (especially the SYNOPSIS section) to highlight that they\nare incompatible, and add a few tests to make sure the combination\nfails.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * We do have proper description of the --merge option in\n   Documentation/git-checkout.txt to cover the operation in both\n   modes of \"checkout\" (i.e. checking out a branch vs checking out\n   paths), but the parse_options help text only talks about the one\n   that happens when checking out a branch.  We might want to do\n   something about it, but I am not sure what the right phrase\n   should be.  The options[] array there is created by concatenating\n   common ones, switch-related ones, and restore-related ones, and\n   ideally how the option works in each mode should be decribed in\n   the latter two, but we cannot have duplicate entries in the\n   resulting options[] array, of course, so such an approach would\n   not work well.\n\n Documentation/git-checkout.txt | 9 ++++++---\n Documentation/git-restore.txt  | 4 ++++\n builtin/checkout.c             | 9 +++++++++\n t/t2070-restore.sh             | 7 +++++--\n t/t7201-co.sh                  | 5 +++++\n 5 files changed, 29 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt\nindex 4af0904f47..a30e3ebc51 100644\n--- a/Documentation/git-checkout.txt\n+++ b/Documentation/git-checkout.txt\n@@ -12,8 +12,10 @@ SYNOPSIS\n 'git checkout' [-q] [-f] [-m] --detach [<branch>]\n 'git checkout' [-q] [-f] [-m] [--detach] <commit>\n 'git checkout' [-q] [-f] [-m] [[-b|-B|--orphan] <new-branch>] [<start-point>]\n-'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <pathspec>...\n-'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] --pathspec-from-file=<file> [--pathspec-file-nul]\n+'git checkout' [-f] <tree-ish> [--] <pathspec>...\n+'git checkout' [-f] <tree-ish> --pathspec-from-file=<file> [--pathspec-file-nul]\n+'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [--] <pathspec>...\n+'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] --pathspec-from-file=<file> [--pathspec-file-nul]\n 'git checkout' (-p|--patch) [<tree-ish>] [--] [<pathspec>...]\n \n DESCRIPTION\n@@ -260,7 +262,8 @@ and mark the resolved paths with `git add` (or `git rm` if the merge\n should result in deletion of the path).\n +\n When checking out paths from the index, this option lets you recreate\n-the conflicted merge in the specified paths.\n+the conflicted merge in the specified paths.  This option cannot be\n+used when checking out paths from a tree-ish.\n +\n When switching branches with `--merge`, staged changes may be lost.\n \ndiff --git a/Documentation/git-restore.txt b/Documentation/git-restore.txt\nindex 5964810caa..c70444705b 100644\n--- a/Documentation/git-restore.txt\n+++ b/Documentation/git-restore.txt\n@@ -78,6 +78,8 @@ all modified paths.\n --theirs::\n \tWhen restoring files in the working tree from the index, use\n \tstage #2 ('ours') or #3 ('theirs') for unmerged paths.\n+\tThis option cannot be used when checking out paths from a\n+\ttree-ish (i.e. with the `--source` option).\n +\n Note that during `git rebase` and `git pull --rebase`, 'ours' and\n 'theirs' may appear swapped. See the explanation of the same options\n@@ -87,6 +89,8 @@ in linkgit:git-checkout[1] for details.\n --merge::\n \tWhen restoring files on the working tree from the index,\n \trecreate the conflicted merge in the unmerged paths.\n+\tThis option cannot be used when checking out paths from a\n+\ttree-ish (i.e. with the `--source` option).\n \n --conflict=<style>::\n \tThe same as `--merge` option above, but changes the way the\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 715eeb5048..b8dfba57c6 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -520,6 +520,15 @@ static int checkout_paths(const struct checkout_opts *opts,\n \t\t\t    \"--merge\", \"--conflict\", \"--staged\");\n \t}\n \n+\t/*\n+\t * recreating unmerged index entries and writing out data from\n+\t * unmerged index entries would make no sense when checking out\n+\t * of a tree-ish.\n+\t */\n+\tif ((opts->merge || opts->writeout_stage) && opts->source_tree)\n+\t\tdie(_(\"'%s', '%s', or '%s' cannot be used when checking out of a tree\"),\n+\t\t    \"--merge\", \"--ours\", \"--theirs\");\n+\n \tif (opts->patch_mode) {\n \t\tenum add_p_mode patch_mode;\n \t\tconst char *rev = new_branch_info->name;\ndiff --git a/t/t2070-restore.sh b/t/t2070-restore.sh\nindex c5d19dd973..fd775807e7 100755\n--- a/t/t2070-restore.sh\n+++ b/t/t2070-restore.sh\n@@ -137,11 +137,14 @@ test_expect_success 'restore --staged invalidates cache tree for deletions' '\n \ttest_must_fail git rev-parse HEAD:new1\n '\n \n-test_expect_success 'restore with merge options rejects --staged' '\n+test_expect_success 'restore with merge options are incompatible with certain options' '\n \tfor opts in \\\n \t\t\"--staged --ours\" \\\n \t\t\"--staged --theirs\" \\\n \t\t\"--staged --merge\" \\\n+\t\t\"--source=HEAD --ours\" \\\n+\t\t\"--source=HEAD --theirs\" \\\n+\t\t\"--source=HEAD --merge\" \\\n \t\t\"--staged --conflict=diff3\" \\\n \t\t\"--staged --worktree --ours\" \\\n \t\t\"--staged --worktree --theirs\" \\\n@@ -149,7 +152,7 @@ test_expect_success 'restore with merge options rejects --staged' '\n \t\t\"--staged --worktree --conflict=zdiff3\"\n \tdo\n \t\ttest_must_fail git restore $opts . 2>err &&\n-\t\tgrep \"cannot be used with --staged\" err || return\n+\t\tgrep \"cannot be used\" err || return\n \tdone\n '\n \ndiff --git a/t/t7201-co.sh b/t/t7201-co.sh\nindex 61ad47b0c1..23d4dadbcc 100755\n--- a/t/t7201-co.sh\n+++ b/t/t7201-co.sh\n@@ -497,6 +497,11 @@ test_expect_success 'checkout unmerged stage' '\n \ttest ztheirside = \"z$(cat file)\"\n '\n \n+test_expect_success 'checkout path with --merge from tree-ish is a no-no' '\n+\tsetup_conflicting_index &&\n+\ttest_must_fail git checkout -m HEAD -- file\n+'\n+\n test_expect_success 'checkout with --merge' '\n \tsetup_conflicting_index &&\n \techo \"none of the above\" >sample &&\n-- \n2.41.0-478-gee48e70a82\n\n"},{"id":"479977","messageId":"xmqq7cqj4rme.fsf@gitster.g","threadId":"60038","inReplyTo":"xmqqedkr4rod.fsf@gitster.g","subject":"[PATCH 2/2] checkout/restore: add basic tests for --merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-28T22:07:37Z","receivedAt":"2023-07-28T22:07:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Even though \"checkout --merge -- paths\" had some tests, we never\nmade sure it worked to recreate the conflicted state _after_ the\nresolution was recorded in the index.  Also \"restore --merge\" did\nnot even have any tests.\n\nCurrently these commands use the unmerge_marked_index() interface\nthat cannot handle paths that have been resolved as removal, and\ntests for that case are marked with test_expect_failure; these\nshould eventually be fixed, but not in this patch.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t2070-restore.sh | 64 ++++++++++++++++++++++++++++++++++++++++++++++\n t/t7201-co.sh      | 42 ++++++++++++++++++++++++++++++\n 2 files changed, 106 insertions(+)\n\ndiff --git a/t/t2070-restore.sh b/t/t2070-restore.sh\nindex fd775807e7..d97ecc2483 100755\n--- a/t/t2070-restore.sh\n+++ b/t/t2070-restore.sh\n@@ -137,6 +137,70 @@ test_expect_success 'restore --staged invalidates cache tree for deletions' '\n \ttest_must_fail git rev-parse HEAD:new1\n '\n \n+test_expect_success 'restore --merge to unresolve' '\n+\tO=$(echo original | git hash-object -w --stdin) &&\n+\tA=$(echo ourside | git hash-object -w --stdin) &&\n+\tB=$(echo theirside | git hash-object -w --stdin) &&\n+\t{\n+\t\techo \"100644 $O 1\tfile\" &&\n+\t\techo \"100644 $A 2\tfile\" &&\n+\t\techo \"100644 $B 3\tfile\"\n+\t} | git update-index --index-info &&\n+\techo nothing >file &&\n+\tgit restore --worktree --merge file &&\n+\tcat >expect <<-\\EOF &&\n+\t<<<<<<< ours\n+\tourside\n+\t=======\n+\ttheirside\n+\t>>>>>>> theirs\n+\tEOF\n+\ttest_cmp expect file\n+'\n+\n+test_expect_success 'restore --merge to unresolve after (mistaken) resolution' '\n+\tO=$(echo original | git hash-object -w --stdin) &&\n+\tA=$(echo ourside | git hash-object -w --stdin) &&\n+\tB=$(echo theirside | git hash-object -w --stdin) &&\n+\t{\n+\t\techo \"100644 $O 1\tfile\" &&\n+\t\techo \"100644 $A 2\tfile\" &&\n+\t\techo \"100644 $B 3\tfile\"\n+\t} | git update-index --index-info &&\n+\techo nothing >file &&\n+\tgit add file &&\n+\tgit restore --worktree --merge file &&\n+\tcat >expect <<-\\EOF &&\n+\t<<<<<<< ours\n+\tourside\n+\t=======\n+\ttheirside\n+\t>>>>>>> theirs\n+\tEOF\n+\ttest_cmp expect file\n+'\n+\n+test_expect_failure 'restore --merge to unresolve after (mistaken) resolution' '\n+\tO=$(echo original | git hash-object -w --stdin) &&\n+\tA=$(echo ourside | git hash-object -w --stdin) &&\n+\tB=$(echo theirside | git hash-object -w --stdin) &&\n+\t{\n+\t\techo \"100644 $O 1\tfile\" &&\n+\t\techo \"100644 $A 2\tfile\" &&\n+\t\techo \"100644 $B 3\tfile\"\n+\t} | git update-index --index-info &&\n+\tgit rm -f file &&\n+\tgit restore --worktree --merge file &&\n+\tcat >expect <<-\\EOF &&\n+\t<<<<<<< ours\n+\tourside\n+\t=======\n+\ttheirside\n+\t>>>>>>> theirs\n+\tEOF\n+\ttest_cmp expect file\n+'\n+\n test_expect_success 'restore with merge options are incompatible with certain options' '\n \tfor opts in \\\n \t\t\"--staged --ours\" \\\ndiff --git a/t/t7201-co.sh b/t/t7201-co.sh\nindex 23d4dadbcc..4b07a26c14 100755\n--- a/t/t7201-co.sh\n+++ b/t/t7201-co.sh\n@@ -522,6 +522,48 @@ test_expect_success 'checkout with --merge' '\n \ttest_cmp merged file\n '\n \n+test_expect_success 'checkout -m works after (mistaken) resolution' '\n+\tsetup_conflicting_index &&\n+\techo \"none of the above\" >sample &&\n+\tcat sample >fild &&\n+\tcat sample >file &&\n+\tcat sample >filf &&\n+\t# resolve to something\n+\tgit add file &&\n+\tgit checkout --merge -- fild file filf &&\n+\t{\n+\t\techo \"<<<<<<< ours\" &&\n+\t\techo ourside &&\n+\t\techo \"=======\" &&\n+\t\techo theirside &&\n+\t\techo \">>>>>>> theirs\"\n+\t} >merged &&\n+\ttest_cmp expect fild &&\n+\ttest_cmp expect filf &&\n+\ttest_cmp merged file\n+'\n+\n+test_expect_failure 'checkout -m works after (mistaken) resolution to remove' '\n+\tsetup_conflicting_index &&\n+\techo \"none of the above\" >sample &&\n+\tcat sample >fild &&\n+\tcat sample >file &&\n+\tcat sample >filf &&\n+\t# resolve to remove\n+\tgit rm file &&\n+\tgit checkout --merge -- fild file filf &&\n+\t{\n+\t\techo \"<<<<<<< ours\" &&\n+\t\techo ourside &&\n+\t\techo \"=======\" &&\n+\t\techo theirside &&\n+\t\techo \">>>>>>> theirs\"\n+\t} >merged &&\n+\ttest_cmp expect fild &&\n+\ttest_cmp expect filf &&\n+\ttest_cmp merged file\n+'\n+\n test_expect_success 'checkout with --merge, in diff3 -m style' '\n \tgit config merge.conflictstyle diff3 &&\n \tsetup_conflicting_index &&\n-- \n2.41.0-478-gee48e70a82\n\n"}]}