Re: [PATCH] merge-ort: remove code obsoleted by other changes
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Aug 19, 2022, 09:22 UTC
- Message-ID
- <596s400n-s6qs-0s08-s31s-99rq4o016647@tzk.qr>
- In-Reply-To
- <pull.1302.git.git.1660884355643.gitgitgadget@gmail.com>
Hi Elijah,
On Fri, 19 Aug 2022, Elijah Newren via GitGitGadget wrote:
Show 30 quoted lines
> 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
Show 61 quoted lines
>
> 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
>