Re: [PATCH 1/4] merge-recursive: silence -Wxor-used-as-pow warning

From: Jeff King <peff@peff.net>
Date: Jan 25, 2020, 23:57 UTC
Message-ID: <20200125235745.GA447121@coredump.intra.peff.net>
In-Reply-To: <CABPp-BFp3VrYjD8KTqnOff0-CDY7qO3Au7GeUDCuA46ofv1GSg@mail.gmail.com>

On Sat, Jan 25, 2020 at 12:50:38PM -0800, Elijah Newren wrote:

Show 9 quoted lines

> Interesting.  In merge-ort (my in-development attempt at a replacement
> for merge-recursive), I'm currently storing the stages in an array
> with indices 0-2 rather than the 1-3 used by merge-recursive.  This
> removes the empty/unused array entry at the beginning, and also works
> a bit better with the masks that traverse_trees() returns (as 1<<index
> corresponds to the bit in the mask from the traverse_trees()
> callback).  In that scheme, bitflip won't work, but the subtraction
> idea still does.  So, I'd tend to agree with Junio, but I think the
> helper you added here is probably the more important improvement.

OK, that's three people who think the subtraction is more obvious. I agree it's not that big a deal now that it's hidden in the helper (and the code may eventually go away anyway), but it's easy enough to fix it while we're thinking about it.

Patch is below (as an incremental on top attributed to Junio, who proposed it; but it would be fine to squash it in with a Helped-by, too).

Show 8 quoted lines

> [1] merge-ort still isn't functional yet other than in extremely
> narrow circumstances, I'm still experimenting with the data
> structures, and I've written several hundred lines of code and then
> thrown it all away at least once -- and may do so again.  Whenever I
> find a useful patch I can separate and submit upstream, I have been
> doing so, but until the risk of another complete rewrite goes down,
> there's no point in me sending my half-baked ideas in for review.
> They need to be at least three-quarters baked first.  :-)

Thank you, by the way, for all of the work you have done on merge-recursive. I generally find it one of the scariest bits of the code to look at, so I am happy to be able to punt it off to you. :)

-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Subject: [PATCH] merge-recursive: use subtraction to flip stage

The flip_stage() helper uses a bit-flipping xor to switch between "2" and "3". While clever, this relies on a property of those two numbers that is mostly coincidence. Let's write it as a subtraction; that's more clear and would extend to other numbers if somebody copies the logic.

Signed-off-by: Jeff King <peff@peff.net>
---
 merge-recursive.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index e6aedd3cab..aee1769a7a 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1713,12 +1713,11 @@ static char *find_path_for_conflict(struct merge_options *opt,
 }
 
 /*
- * Toggle the stage number between "ours" and "theirs" (2 and 3) by flipping
- * the 1-bit.
+ * Toggle the stage number between "ours" and "theirs" (2 and 3).
  */
 static inline int flip_stage(int stage)
 {
-	return stage ^ 1;
+	return (2 + 3) - stage;
 }
 
 static int handle_rename_rename_1to2(struct merge_options *opt,

-- 
2.25.0.430.g8dfc7de6f7