Re: [PATCH 3/8] merge-ort: record the reason that we want a rename for a directory

Elijah Newren [off-list ref] writes:

On Mon, Mar 15, 2021 at 7:31 AM Derrick Stolee [off-list ref] wrote:

quoted

On 3/13/2021 5:22 PM, Elijah Newren via GitGitGadget wrote:

quoted

From: Elijah Newren <redacted>

When one side of history renames a directory, and the other side of
history added files to the old directory, directory rename detection is
used to warn about the location of the added files so the user can
move them to the old directory or keep them with the new one.

This sets up three different types of directories:
  * directories that had new files added to them
  * directories underneath a directory that had new files added to them
  * directories where no new files were added to it or any leading path

Save this information in dirs_removed; the next several commits will
make use of this information.

...

quoted

+/* dir_rename_relevance: the reason we want rename information for a dir */
+enum dir_rename_relevance {
+     NOT_RELEVANT = 0,
+     RELEVANT_FOR_ANCESTOR = 1,
+     RELEVANT_FOR_SELF = 2
+};

Is this potentially a flag list? It's hard to tell because we don't
have another item (3 or 4?).

quoted

              unsigned sides = (0x07 - dirmask)/2;
+             unsigned relevance = (renames->dir_rename_mask == 0x07) ?
+                                     RELEVANT_FOR_ANCESTOR : NOT_RELEVANT;
+             /*
+              * Record relevance of this directory.  However, note that
+              * when collect_merge_info_callback() recurses into this
+              * directory and calls collect_rename_info() on paths
+              * within that directory, if we find a path that was added
+              * to this directory on the other side of history, we will
+              * upgrade this value to RELEVANT_FOR_SELF; see below.
+              */

This comment seems to imply that RELEVANT_FOR_SELF is "more important"
than RELEVANT_FOR_ANCESTOR, so the value will just be changed (not a
flag).

Yes.

quoted

quoted

+     /*
+      * Here's the block that potentially upgrades to RELEVANT_FOR_SELF.
+      * When we run across a file added to a directory.  In such a case,
+      * find the directory of the file and upgrade its relevance.
+      */
+     if (renames->dir_rename_mask == 0x07 &&
+         (filemask == 2 || filemask == 4)) {
+             /*
+              * Need directory rename for parent directory on other side
+              * of history from added file.  Thus
+              *    side = (~filemask & 0x06) >> 1
+              * or
+              *    side = 3 - (filemask/2).
+              */
+             unsigned side = 3 - (filemask >> 1);
+             strintmap_set(&renames->dirs_removed[side], dirname,
+                           RELEVANT_FOR_SELF);

Yes, using "RELEVANT_FOR_SELF" here, not "relevance | RELEVANT_FOR_SELF".
OK. This should make the later consumers simpler.

Yep, indeed.  Would it make it clearer earlier if I were to stop
assigning the explicit values in the enum?  Would adding a comment
when I introduce the enum be easier?  Or was it just "thinking out
loud"?

You are not asking me, but if you were, I'd say not using enum for
bitmask would be a good discipline to follow, and an enum like this
one that is used only for uniqueness of the values would benefit from
not having explicit value assignments.