[v2,2/7] merge-ort: add some more explanations in collect_merge_info_callback()

Commit Message

Elijah Newren July 13, 2021, 7:32 p.m. UTC

From: Elijah Newren <[e-mail]>

The previous patch possibly raises some questions about whether
additional cases in collect_merge_info_callback() can be handled early.
Add some explanations in the form of comments to help explain these
better.  While we're at it, add a few comments to denote what a few
boolean '0' or '1' values stand for.

Signed-off-by: Elijah Newren <[e-mail]>
---
 merge-ort.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

Comments

Bagas Sanjaya July 13, 2021, 11:34 p.m. UTC | #1

On 14/07/21 02.32, Elijah Newren via GitGitGadget wrote:
> @@ -1018,8 +1018,8 @@ static int collect_merge_info_callback(int n,
>   	if (side1_matches_mbase && side2_matches_mbase) {
>   		/* mbase, side1, & side2 all match; use mbase as resolution */
>   		setup_path_info(opt, &pi, dirname, info->pathlen, fullpath,
> -				names, names+0, mbase_null, 0,
> -				filemask, dirmask, 1);
> +				names, names+0, mbase_null, 0 /* df_conflict */,
> +				filemask, dirmask, 1 /* resolved */);
>   		return mask;
>   	}
>


Is df_conflict stands for directory-file conflict?

>   	/*
> -	 * Record information about the path so we can resolve later in
> -	 * process_entries.
> +	 * None of the special cases above matched, so we have a
> +	 * provisional conflict.  (Rename detection might allow us to
> +	 * unconflict some more cases, but that comes later so all we can
> +	 * do now is record the different non-null file hashes.)
>   	 */
>   	setup_path_info(opt, &pi, dirname, info->pathlen, fullpath,
>   			names, NULL, 0, df_conflict, filemask, dirmask, 0);
> 

So when none of special cases matched, we assumed there's conflict 
(although provisional), right?

Elijah Newren July 14, 2021, 12:19 a.m. UTC | #2

On Tue, Jul 13, 2021 at 4:34 PM Bagas Sanjaya <[e-mail]> wrote:
>
> On 14/07/21 02.32, Elijah Newren via GitGitGadget wrote:
> > @@ -1018,8 +1018,8 @@ static int collect_merge_info_callback(int n,
> >       if (side1_matches_mbase && side2_matches_mbase) {
> >               /* mbase, side1, & side2 all match; use mbase as resolution */
> >               setup_path_info(opt, &pi, dirname, info->pathlen, fullpath,
> > -                             names, names+0, mbase_null, 0,
> > -                             filemask, dirmask, 1);
> > +                             names, names+0, mbase_null, 0 /* df_conflict */,
> > +                             filemask, dirmask, 1 /* resolved */);
> >               return mask;
> >       }
> >
>
>
> Is df_conflict stands for directory-file conflict?

Yes, eventually propagating up to conflict_info->df_conflict, defined as:

    /* Whether this path is/was involved in a directory/file conflict */
    unsigned df_conflict:1;

> >       /*
> > -      * Record information about the path so we can resolve later in
> > -      * process_entries.
> > +      * None of the special cases above matched, so we have a
> > +      * provisional conflict.  (Rename detection might allow us to
> > +      * unconflict some more cases, but that comes later so all we can
> > +      * do now is record the different non-null file hashes.)
> >        */
> >       setup_path_info(opt, &pi, dirname, info->pathlen, fullpath,
> >                       names, NULL, 0, df_conflict, filemask, dirmask, 0);
> >
>
> So when none of special cases matched, we assumed there's conflict
> (although provisional), right?

Yes, the "provisional" adjective in particular is there because we
revisit each case in process_entries(), and resolve those that can be
via content merges, renormalization, etc.  The content merges need to
wait until after rename detection allows us to pair up different
files.  If the different file versions are still not resolved after
process_entries() then it becomes an actual conflict rather than just
a provisional conflict.

Patch

diff --git a/merge-ort.c b/merge-ort.c
index 6299b4f9413..843fa693145 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -1018,8 +1018,8 @@  static int collect_merge_info_callback(int n,
 	if (side1_matches_mbase && side2_matches_mbase) {
 		/* mbase, side1, & side2 all match; use mbase as resolution */
 		setup_path_info(opt, &pi, dirname, info->pathlen, fullpath,
-				names, names+0, mbase_null, 0,
-				filemask, dirmask, 1);
+				names, names+0, mbase_null, 0 /* df_conflict */,
+				filemask, dirmask, 1 /* resolved */);
 		return mask;
 	}
 
@@ -1061,14 +1061,24 @@  static int collect_merge_info_callback(int n,
 	}
 
 	/*
-	 * Gather additional information used in rename detection.
+	 * Sometimes we can tell that a source path need not be included in
+	 * rename detection -- namely, whenever either
+	 *    side1_matches_mbase && side2_null
+	 * or
+	 *    side2_matches_mbase && side1_null
+	 * However, we call collect_rename_info() even in those cases,
+	 * because exact renames are cheap and would let us remove both a
+	 * source and destination path.  We'll cull the unneeded sources
+	 * later.
 	 */
 	collect_rename_info(opt, names, dirname, fullpath,
 			    filemask, dirmask, match_mask);
 
 	/*
-	 * Record information about the path so we can resolve later in
-	 * process_entries.
+	 * None of the special cases above matched, so we have a
+	 * provisional conflict.  (Rename detection might allow us to
+	 * unconflict some more cases, but that comes later so all we can
+	 * do now is record the different non-null file hashes.)
 	 */
 	setup_path_info(opt, &pi, dirname, info->pathlen, fullpath,
 			names, NULL, 0, df_conflict, filemask, dirmask, 0);