git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] merge-ort: remove code obsoleted by other changes
@ 2022-08-19  4:45 Elijah Newren via GitGitGadget
  2022-08-19  9:22 ` Johannes Schindelin
  0 siblings, 1 reply; 3+ messages in thread
From: Elijah Newren via GitGitGadget @ 2022-08-19  4:45 UTC (permalink / raw)
  To: git; +Cc: Elijah Newren, Elijah Newren

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(-)

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

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] merge-ort: remove code obsoleted by other changes
  2022-08-19  4:45 [PATCH] merge-ort: remove code obsoleted by other changes Elijah Newren via GitGitGadget
@ 2022-08-19  9:22 ` Johannes Schindelin
  2022-08-20 23:01   ` Elijah Newren
  0 siblings, 1 reply; 3+ messages in thread
From: Johannes Schindelin @ 2022-08-19  9:22 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget; +Cc: git, Elijah Newren, Elijah Newren

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
>

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] merge-ort: remove code obsoleted by other changes
  2022-08-19  9:22 ` Johannes Schindelin
@ 2022-08-20 23:01   ` Elijah Newren
  0 siblings, 0 replies; 3+ messages in thread
From: Elijah Newren @ 2022-08-20 23:01 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Elijah Newren via GitGitGadget, Git Mailing List

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!

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-08-20 23:02 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-19  4:45 [PATCH] merge-ort: remove code obsoleted by other changes Elijah Newren via GitGitGadget
2022-08-19  9:22 ` Johannes Schindelin
2022-08-20 23:01   ` Elijah Newren

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).