{"thread":{"id":"58325","subject":"[PATCH] merge-ort: remove code obsoleted by other changes","startedAt":"2022-08-19T04:46:02Z","lastAt":"2022-08-20T23:02:10Z","messageCount":3,"participants":["Elijah Newren via GitGitGadget","Johannes Schindelin","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"461506","messageId":"pull.1302.git.git.1660884355643.gitgitgadget@gmail.com","threadId":"58325","inReplyTo":null,"subject":"[PATCH] merge-ort: remove code obsoleted by other changes","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-19T04:45:55Z","receivedAt":"2022-08-19T04:46:02Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nCommit 66b209b86a (\"merge-ort: implement CE_SKIP_WORKTREE handling with\nconflicted entries\", 2021-03-20) added some code for merge-ort to handle\nconflicted and skip_worktree entries in general.  Included in this was\nan ugly hack for dealing with present-despite-skipped entries and a\ntestcase (t6428.2) specific to that hack, since at that time users could\naccidentally get files into that state when using a sparse checkout.\n\nHowever, with the merging of 82386b4496 (\"Merge branch\n'en/present-despite-skipped'\", 2022-03-09), that class of problems was\naddressed globally and in a much cleaner way.  As such, the\npresent-despite-skipped hack in merge-ort is no longer needed and can\nsimply be removed.\n\nNo additional testcase is needed here; t6428.2 was written to test the\nnecessary functionality and is being kept.  The fact that this test\ncontinues to pass despite the code being removed shows that the extra\ncode is no longer necessary.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n    merge-ort: remove code obsoleted by other changes\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1302%2Fnewren%2Fnuke-present-despite-skipped-hack-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1302/newren/nuke-present-despite-skipped-hack-v1\nPull-Request: https://github.com/git/git/pull/1302\n\n merge-ort.c | 22 ++--------------------\n 1 file changed, 2 insertions(+), 20 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 8b7de0fbd8e..a6a3ab839a0 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -491,7 +491,6 @@ enum conflict_and_info_types {\n \tCONFLICT_FILE_DIRECTORY,\n \tCONFLICT_DISTINCT_MODES,\n \tCONFLICT_MODIFY_DELETE,\n-\tCONFLICT_PRESENT_DESPITE_SKIPPED,\n \n \t/* Regular rename */\n \tCONFLICT_RENAME_RENAME,   /* same file renamed differently */\n@@ -536,8 +535,6 @@ static const char *type_short_descriptions[] = {\n \t[CONFLICT_FILE_DIRECTORY] = \"CONFLICT (file/directory)\",\n \t[CONFLICT_DISTINCT_MODES] = \"CONFLICT (distinct modes)\",\n \t[CONFLICT_MODIFY_DELETE] = \"CONFLICT (modify/delete)\",\n-\t[CONFLICT_PRESENT_DESPITE_SKIPPED] =\n-\t\t\"CONFLICT (upgrade your version of git)\",\n \n \t/*** Regular rename ***/\n \t[CONFLICT_RENAME_RENAME] = \"CONFLICT (rename/rename)\",\n@@ -748,8 +745,7 @@ static void path_msg(struct merge_options *opt,\n \t/* Sanity checks */\n \tassert(omittable_hint ==\n \t       !starts_with(type_short_descriptions[type], \"CONFLICT\") ||\n-\t       type == CONFLICT_DIR_RENAME_SUGGESTED ||\n-\t       type == CONFLICT_PRESENT_DESPITE_SKIPPED);\n+\t       type == CONFLICT_DIR_RENAME_SUGGESTED);\n \tif (opt->record_conflict_msgs_as_headers && omittable_hint)\n \t\treturn; /* Do not record mere hints in headers */\n \tif (opt->priv->call_depth && opt->verbosity < 5)\n@@ -4377,22 +4373,8 @@ static int record_conflicted_index_entries(struct merge_options *opt)\n \t\t\t * the CE_SKIP_WORKTREE bit and manually write those\n \t\t\t * files to the working disk here.\n \t\t\t */\n-\t\t\tif (ce_skip_worktree(ce)) {\n-\t\t\t\tstruct stat st;\n-\n-\t\t\t\tif (!lstat(path, &st)) {\n-\t\t\t\t\tchar *new_name = unique_path(opt,\n-\t\t\t\t\t\t\t\t     path,\n-\t\t\t\t\t\t\t\t     \"cruft\");\n-\n-\t\t\t\t\tpath_msg(opt, CONFLICT_PRESENT_DESPITE_SKIPPED, 1,\n-\t\t\t\t\t\t path, NULL, NULL, NULL,\n-\t\t\t\t\t\t _(\"Note: %s not up to date and in way of checking out conflicted version; old copy renamed to %s\"),\n-\t\t\t\t\t\t path, new_name);\n-\t\t\t\t\terrs |= rename(path, new_name);\n-\t\t\t\t}\n+\t\t\tif (ce_skip_worktree(ce))\n \t\t\t\terrs |= checkout_entry(ce, &state, NULL, NULL);\n-\t\t\t}\n \n \t\t\t/*\n \t\t\t * Mark this cache entry for removal and instead add\n\nbase-commit: 6a475b71f8c4ce708d69fdc9317aefbde3769e25\n-- \ngitgitgadget\n"},{"id":"461520","messageId":"596s400n-s6qs-0s08-s31s-99rq4o016647@tzk.qr","threadId":"58325","inReplyTo":"pull.1302.git.git.1660884355643.gitgitgadget@gmail.com","subject":"Re: [PATCH] merge-ort: remove code obsoleted by other changes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-19T09:22:44Z","receivedAt":"2022-08-19T09:22:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Fri, 19 Aug 2022, Elijah Newren via GitGitGadget wrote:\n\n> From: Elijah Newren <newren@gmail.com>\n>\n> Commit 66b209b86a (\"merge-ort: implement CE_SKIP_WORKTREE handling with\n> conflicted entries\", 2021-03-20) added some code for merge-ort to handle\n> conflicted and skip_worktree entries in general.  Included in this was\n> an ugly hack for dealing with present-despite-skipped entries and a\n> testcase (t6428.2) specific to that hack, since at that time users could\n> accidentally get files into that state when using a sparse checkout.\n>\n> However, with the merging of 82386b4496 (\"Merge branch\n> 'en/present-despite-skipped'\", 2022-03-09), that class of problems was\n> addressed globally and in a much cleaner way.  As such, the\n> present-despite-skipped hack in merge-ort is no longer needed and can\n> simply be removed.\n>\n> No additional testcase is needed here; t6428.2 was written to test the\n> necessary functionality and is being kept.  The fact that this test\n> continues to pass despite the code being removed shows that the extra\n> code is no longer necessary.\n>\n> Signed-off-by: Elijah Newren <newren@gmail.com>\n> ---\n>     merge-ort: remove code obsoleted by other changes\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1302%2Fnewren%2Fnuke-present-despite-skipped-hack-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1302/newren/nuke-present-despite-skipped-hack-v1\n> Pull-Request: https://github.com/git/git/pull/1302\n>\n>  merge-ort.c | 22 ++--------------------\n>  1 file changed, 2 insertions(+), 20 deletions(-)\n\nNice!\n\nSince I have been in the `merge-ort` code quite a bit as of late, I deem\nmyself familiar enough with the code to dare offering my ACK.\n\nWhat is _particularly_ nice is that this patch removes an `lstat()` call\nthat was a bit of a concern for me when using `merge-ort` in a\nworktree-less scenario. After some reasoning about the code, it turned out\nthat it is not hit in that use case, nevertheless it is much easier to\nreason about `lstat()` calls that simply are not in the code.\n\nThank you!\nDscho\n\n>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index 8b7de0fbd8e..a6a3ab839a0 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -491,7 +491,6 @@ enum conflict_and_info_types {\n>  \tCONFLICT_FILE_DIRECTORY,\n>  \tCONFLICT_DISTINCT_MODES,\n>  \tCONFLICT_MODIFY_DELETE,\n> -\tCONFLICT_PRESENT_DESPITE_SKIPPED,\n>\n>  \t/* Regular rename */\n>  \tCONFLICT_RENAME_RENAME,   /* same file renamed differently */\n> @@ -536,8 +535,6 @@ static const char *type_short_descriptions[] = {\n>  \t[CONFLICT_FILE_DIRECTORY] = \"CONFLICT (file/directory)\",\n>  \t[CONFLICT_DISTINCT_MODES] = \"CONFLICT (distinct modes)\",\n>  \t[CONFLICT_MODIFY_DELETE] = \"CONFLICT (modify/delete)\",\n> -\t[CONFLICT_PRESENT_DESPITE_SKIPPED] =\n> -\t\t\"CONFLICT (upgrade your version of git)\",\n>\n>  \t/*** Regular rename ***/\n>  \t[CONFLICT_RENAME_RENAME] = \"CONFLICT (rename/rename)\",\n> @@ -748,8 +745,7 @@ static void path_msg(struct merge_options *opt,\n>  \t/* Sanity checks */\n>  \tassert(omittable_hint ==\n>  \t       !starts_with(type_short_descriptions[type], \"CONFLICT\") ||\n> -\t       type == CONFLICT_DIR_RENAME_SUGGESTED ||\n> -\t       type == CONFLICT_PRESENT_DESPITE_SKIPPED);\n> +\t       type == CONFLICT_DIR_RENAME_SUGGESTED);\n>  \tif (opt->record_conflict_msgs_as_headers && omittable_hint)\n>  \t\treturn; /* Do not record mere hints in headers */\n>  \tif (opt->priv->call_depth && opt->verbosity < 5)\n> @@ -4377,22 +4373,8 @@ static int record_conflicted_index_entries(struct merge_options *opt)\n>  \t\t\t * the CE_SKIP_WORKTREE bit and manually write those\n>  \t\t\t * files to the working disk here.\n>  \t\t\t */\n> -\t\t\tif (ce_skip_worktree(ce)) {\n> -\t\t\t\tstruct stat st;\n> -\n> -\t\t\t\tif (!lstat(path, &st)) {\n> -\t\t\t\t\tchar *new_name = unique_path(opt,\n> -\t\t\t\t\t\t\t\t     path,\n> -\t\t\t\t\t\t\t\t     \"cruft\");\n> -\n> -\t\t\t\t\tpath_msg(opt, CONFLICT_PRESENT_DESPITE_SKIPPED, 1,\n> -\t\t\t\t\t\t path, NULL, NULL, NULL,\n> -\t\t\t\t\t\t _(\"Note: %s not up to date and in way of checking out conflicted version; old copy renamed to %s\"),\n> -\t\t\t\t\t\t path, new_name);\n> -\t\t\t\t\terrs |= rename(path, new_name);\n> -\t\t\t\t}\n> +\t\t\tif (ce_skip_worktree(ce))\n>  \t\t\t\terrs |= checkout_entry(ce, &state, NULL, NULL);\n> -\t\t\t}\n>\n>  \t\t\t/*\n>  \t\t\t * Mark this cache entry for removal and instead add\n>\n> base-commit: 6a475b71f8c4ce708d69fdc9317aefbde3769e25\n> --\n> gitgitgadget\n>\n"},{"id":"461671","messageId":"CABPp-BH9-k+LiikRLHx8JsfNkihvXazxLPzjQ6-jG-bdDKD5XQ@mail.gmail.com","threadId":"58325","inReplyTo":"596s400n-s6qs-0s08-s31s-99rq4o016647@tzk.qr","subject":"Re: [PATCH] merge-ort: remove code obsoleted by other changes","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-08-20T23:01:54Z","receivedAt":"2022-08-20T23:02:10Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Aug 19, 2022 at 2:22 AM Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n>\n> Hi Elijah,\n>\n> On Fri, 19 Aug 2022, Elijah Newren via GitGitGadget wrote:\n>\n> > From: Elijah Newren <newren@gmail.com>\n> >\n> > Commit 66b209b86a (\"merge-ort: implement CE_SKIP_WORKTREE handling with\n> > conflicted entries\", 2021-03-20) added some code for merge-ort to handle\n> > conflicted and skip_worktree entries in general.  Included in this was\n> > an ugly hack for dealing with present-despite-skipped entries and a\n> > testcase (t6428.2) specific to that hack, since at that time users could\n> > accidentally get files into that state when using a sparse checkout.\n> >\n> > However, with the merging of 82386b4496 (\"Merge branch\n> > 'en/present-despite-skipped'\", 2022-03-09), that class of problems was\n> > addressed globally and in a much cleaner way.  As such, the\n> > present-despite-skipped hack in merge-ort is no longer needed and can\n> > simply be removed.\n> >\n> > No additional testcase is needed here; t6428.2 was written to test the\n> > necessary functionality and is being kept.  The fact that this test\n> > continues to pass despite the code being removed shows that the extra\n> > code is no longer necessary.\n> >\n> > Signed-off-by: Elijah Newren <newren@gmail.com>\n> > ---\n> >     merge-ort: remove code obsoleted by other changes\n> >\n> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1302%2Fnewren%2Fnuke-present-despite-skipped-hack-v1\n> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1302/newren/nuke-present-despite-skipped-hack-v1\n> > Pull-Request: https://github.com/git/git/pull/1302\n> >\n> >  merge-ort.c | 22 ++--------------------\n> >  1 file changed, 2 insertions(+), 20 deletions(-)\n>\n> Nice!\n>\n> Since I have been in the `merge-ort` code quite a bit as of late, I deem\n> myself familiar enough with the code to dare offering my ACK.\n\nThanks for taking a look.\n\n> What is _particularly_ nice is that this patch removes an `lstat()` call\n> that was a bit of a concern for me when using `merge-ort` in a\n> worktree-less scenario. After some reasoning about the code, it turned out\n> that it is not hit in that use case, nevertheless it is much easier to\n> reason about `lstat()` calls that simply are not in the code.\n\nOh cool, glad there are other benefits too!\n"}]}