| Message ID | pull.1302.git.git.1660884355643.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ff033db7a8afbed5d2ba407ebd929e6898637194 |
| Headers | show |
| Series | merge-ort: remove code obsoleted by other changes | expand |
Hi Elijah, On Fri, 19 Aug 2022, Elijah Newren via GitGitGadget wrote: > From: Elijah Newren <newren@gmail.com> > > Commit 66b209b86a ("merge-ort: implement CE_SKIP_WORKTREE handling with > conflicted entries", 2021-03-20) added some code for merge-ort to handle > conflicted and skip_worktree entries in general. Included in this was > an ugly hack for dealing with present-despite-skipped entries and a > testcase (t6428.2) specific to that hack, since at that time users could > accidentally get files into that state when using a sparse checkout. > > However, with the merging of 82386b4496 ("Merge branch > 'en/present-despite-skipped'", 2022-03-09), that class of problems was > addressed globally and in a much cleaner way. As such, the > present-despite-skipped hack in merge-ort is no longer needed and can > simply be removed. > > No additional testcase is needed here; t6428.2 was written to test the > necessary functionality and is being kept. The fact that this test > continues to pass despite the code being removed shows that the extra > code is no longer necessary. > > Signed-off-by: Elijah Newren <newren@gmail.com> > --- > merge-ort: remove code obsoleted by other changes > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1302%2Fnewren%2Fnuke-present-despite-skipped-hack-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1302/newren/nuke-present-despite-skipped-hack-v1 > Pull-Request: https://github.com/git/git/pull/1302 > > merge-ort.c | 22 ++-------------------- > 1 file changed, 2 insertions(+), 20 deletions(-) Nice! Since I have been in the `merge-ort` code quite a bit as of late, I deem myself familiar enough with the code to dare offering my ACK. What is _particularly_ nice is that this patch removes an `lstat()` call that was a bit of a concern for me when using `merge-ort` in a worktree-less scenario. After some reasoning about the code, it turned out that it is not hit in that use case, nevertheless it is much easier to reason about `lstat()` calls that simply are not in the code. Thank you! Dscho > > diff --git a/merge-ort.c b/merge-ort.c > index 8b7de0fbd8e..a6a3ab839a0 100644 > --- a/merge-ort.c > +++ b/merge-ort.c > @@ -491,7 +491,6 @@ enum conflict_and_info_types { > CONFLICT_FILE_DIRECTORY, > CONFLICT_DISTINCT_MODES, > CONFLICT_MODIFY_DELETE, > - CONFLICT_PRESENT_DESPITE_SKIPPED, > > /* Regular rename */ > CONFLICT_RENAME_RENAME, /* same file renamed differently */ > @@ -536,8 +535,6 @@ static const char *type_short_descriptions[] = { > [CONFLICT_FILE_DIRECTORY] = "CONFLICT (file/directory)", > [CONFLICT_DISTINCT_MODES] = "CONFLICT (distinct modes)", > [CONFLICT_MODIFY_DELETE] = "CONFLICT (modify/delete)", > - [CONFLICT_PRESENT_DESPITE_SKIPPED] = > - "CONFLICT (upgrade your version of git)", > > /*** Regular rename ***/ > [CONFLICT_RENAME_RENAME] = "CONFLICT (rename/rename)", > @@ -748,8 +745,7 @@ static void path_msg(struct merge_options *opt, > /* Sanity checks */ > assert(omittable_hint == > !starts_with(type_short_descriptions[type], "CONFLICT") || > - type == CONFLICT_DIR_RENAME_SUGGESTED || > - type == CONFLICT_PRESENT_DESPITE_SKIPPED); > + type == CONFLICT_DIR_RENAME_SUGGESTED); > if (opt->record_conflict_msgs_as_headers && omittable_hint) > return; /* Do not record mere hints in headers */ > if (opt->priv->call_depth && opt->verbosity < 5) > @@ -4377,22 +4373,8 @@ static int record_conflicted_index_entries(struct merge_options *opt) > * the CE_SKIP_WORKTREE bit and manually write those > * files to the working disk here. > */ > - if (ce_skip_worktree(ce)) { > - struct stat st; > - > - if (!lstat(path, &st)) { > - char *new_name = unique_path(opt, > - path, > - "cruft"); > - > - path_msg(opt, CONFLICT_PRESENT_DESPITE_SKIPPED, 1, > - path, NULL, NULL, NULL, > - _("Note: %s not up to date and in way of checking out conflicted version; old copy renamed to %s"), > - path, new_name); > - errs |= rename(path, new_name); > - } > + if (ce_skip_worktree(ce)) > errs |= checkout_entry(ce, &state, NULL, NULL); > - } > > /* > * Mark this cache entry for removal and instead add > > base-commit: 6a475b71f8c4ce708d69fdc9317aefbde3769e25 > -- > gitgitgadget >
On Fri, Aug 19, 2022 at 2:22 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > > Hi Elijah, > > On Fri, 19 Aug 2022, Elijah Newren via GitGitGadget wrote: > > > From: Elijah Newren <newren@gmail.com> > > > > Commit 66b209b86a ("merge-ort: implement CE_SKIP_WORKTREE handling with > > conflicted entries", 2021-03-20) added some code for merge-ort to handle > > conflicted and skip_worktree entries in general. Included in this was > > an ugly hack for dealing with present-despite-skipped entries and a > > testcase (t6428.2) specific to that hack, since at that time users could > > accidentally get files into that state when using a sparse checkout. > > > > However, with the merging of 82386b4496 ("Merge branch > > 'en/present-despite-skipped'", 2022-03-09), that class of problems was > > addressed globally and in a much cleaner way. As such, the > > present-despite-skipped hack in merge-ort is no longer needed and can > > simply be removed. > > > > No additional testcase is needed here; t6428.2 was written to test the > > necessary functionality and is being kept. The fact that this test > > continues to pass despite the code being removed shows that the extra > > code is no longer necessary. > > > > Signed-off-by: Elijah Newren <newren@gmail.com> > > --- > > merge-ort: remove code obsoleted by other changes > > > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1302%2Fnewren%2Fnuke-present-despite-skipped-hack-v1 > > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1302/newren/nuke-present-despite-skipped-hack-v1 > > Pull-Request: https://github.com/git/git/pull/1302 > > > > merge-ort.c | 22 ++-------------------- > > 1 file changed, 2 insertions(+), 20 deletions(-) > > Nice! > > Since I have been in the `merge-ort` code quite a bit as of late, I deem > myself familiar enough with the code to dare offering my ACK. Thanks for taking a look. > What is _particularly_ nice is that this patch removes an `lstat()` call > that was a bit of a concern for me when using `merge-ort` in a > worktree-less scenario. After some reasoning about the code, it turned out > that it is not hit in that use case, nevertheless it is much easier to > reason about `lstat()` calls that simply are not in the code. Oh cool, glad there are other benefits too!
diff --git a/merge-ort.c b/merge-ort.c index 8b7de0fbd8e..a6a3ab839a0 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -491,7 +491,6 @@ enum conflict_and_info_types { CONFLICT_FILE_DIRECTORY, CONFLICT_DISTINCT_MODES, CONFLICT_MODIFY_DELETE, - CONFLICT_PRESENT_DESPITE_SKIPPED, /* Regular rename */ CONFLICT_RENAME_RENAME, /* same file renamed differently */ @@ -536,8 +535,6 @@ static const char *type_short_descriptions[] = { [CONFLICT_FILE_DIRECTORY] = "CONFLICT (file/directory)", [CONFLICT_DISTINCT_MODES] = "CONFLICT (distinct modes)", [CONFLICT_MODIFY_DELETE] = "CONFLICT (modify/delete)", - [CONFLICT_PRESENT_DESPITE_SKIPPED] = - "CONFLICT (upgrade your version of git)", /*** Regular rename ***/ [CONFLICT_RENAME_RENAME] = "CONFLICT (rename/rename)", @@ -748,8 +745,7 @@ static void path_msg(struct merge_options *opt, /* Sanity checks */ assert(omittable_hint == !starts_with(type_short_descriptions[type], "CONFLICT") || - type == CONFLICT_DIR_RENAME_SUGGESTED || - type == CONFLICT_PRESENT_DESPITE_SKIPPED); + type == CONFLICT_DIR_RENAME_SUGGESTED); if (opt->record_conflict_msgs_as_headers && omittable_hint) return; /* Do not record mere hints in headers */ if (opt->priv->call_depth && opt->verbosity < 5) @@ -4377,22 +4373,8 @@ static int record_conflicted_index_entries(struct merge_options *opt) * the CE_SKIP_WORKTREE bit and manually write those * files to the working disk here. */ - if (ce_skip_worktree(ce)) { - struct stat st; - - if (!lstat(path, &st)) { - char *new_name = unique_path(opt, - path, - "cruft"); - - path_msg(opt, CONFLICT_PRESENT_DESPITE_SKIPPED, 1, - path, NULL, NULL, NULL, - _("Note: %s not up to date and in way of checking out conflicted version; old copy renamed to %s"), - path, new_name); - errs |= rename(path, new_name); - } + if (ce_skip_worktree(ce)) errs |= checkout_entry(ce, &state, NULL, NULL); - } /* * Mark this cache entry for removal and instead add