threads / patch / 60153

patch, 4 partscode clean-up for rerere

Subject: [PATCH 0/4] code clean-up for rerere

## tl;dr

5 messages between Aug 24, 2023 and Aug 24, 2023. Diffs are folded; open one to read it.

replies: 4people: 1as markdown or json

Junio C Hamano· Aug 24, 2023, 20:54 UTC · lore

These are to clean up some odd code and in-code comment that invited "Huh?" while I was reviewing the code paths involved in "git rerere" recently. None of them is intended to change any behaviour.

Junio C Hamano (4):
  rerere: simplify check_one_conflict() helper function
  rerere: fix comment on handle_file() helper
  rerere: try_merge() should use LL_MERGE_ERROR when it means an error
  rerere: modernize use of empty strbuf

 rerere.c | 46 +++++++++++++++++++---------------------------
 1 file changed, 19 insertions(+), 27 deletions(-)

-- 
2.42.0-29-gcd9da15a85

Junio C Hamano· Aug 24, 2023, 20:54 UTC · re: Junio C Hamano · lore

[PATCH 3/4] rerere: try_merge() should use LL_MERGE_ERROR when it means an error

When the preimage or the postimage files cannot be read, the try_merge() helper function returns LL_MERGE_CONFLICT. To all of its callers, this does not make them do wrong things per-se, as they are only checking if the result is 0 and LL_MERGE_CONFLICT is not 0.

But it is an error if we fail to read the input we expect to be able to read; return LL_MERGE_ERROR instead. This does not change any behaviour---it just makes the code use the "correct" constant to signal an error.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 rerere.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Show changes to rerere.c +1 −1

diff --git a/rerere.c b/rerere.c
index 4ce1270a94..6bc3c54d3b 100644
--- a/rerere.c
+++ b/rerere.c
@@ -617,7 +617,7 @@ static int try_merge(struct index_state *istate,
 
 	if (read_mmfile(&base, rerere_path(id, "preimage")) ||
 	    read_mmfile(&other, rerere_path(id, "postimage"))) {
-		ret = LL_MERGE_CONFLICT;
+		ret = LL_MERGE_ERROR;
 	} else {
 		/*
 		 * A three-way merge. Note that this honors user-customizable

-- 
2.42.0-29-gcd9da15a85

Junio C Hamano· Aug 24, 2023, 20:54 UTC · re: Junio C Hamano · lore

[PATCH 1/4] rerere: simplify check_one_conflict() helper function

The helper function is responsible for inspecting the index and deciding if the path is merged, is conflicted in a way that we would want to handle, or is conflicted in a way that we cannot handle.

Currently, only conflicts with both stage #2 and stage #3 are handled, but eventually we would want to be able to deal with delete-modify conflicts (i.e. only one of stages #2 and #3 exist). Streamline the implementation of the function to make it easier to extend.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 rerere.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

Show changes to rerere.c +15 −19

diff --git a/rerere.c b/rerere.c
index b525dd9230..73cdc8392f 100644
--- a/rerere.c
+++ b/rerere.c
@@ -499,9 +499,11 @@ static int handle_file(struct index_state *istate,
 }
 
 /*
- * Look at a cache entry at "i" and see if it is not conflicting,
- * conflicting and we are willing to handle, or conflicting and
- * we are unable to handle, and return the determination in *type.
+ * Look at a cache entry at "i" and see if it is not conflicting
+ * (RESOLVED), conflicting and we are willing to handle (THREE_STAGED),
+ * or conflicting and we are unable to handle (PUNTED), and return the
+ * determination in *type.
+ *
  * Return the cache index to be looked at next, by skipping the
  * stages we have already looked at in this invocation of this
  * function.
@@ -509,6 +511,7 @@ static int handle_file(struct index_state *istate,
 static int check_one_conflict(struct index_state *istate, int i, int *type)
 {
 	const struct cache_entry *e = istate->cache[i];
+	unsigned int seen_stages = 0;
 
 	if (!ce_stage(e)) {
 		*type = RESOLVED;
@@ -516,24 +519,17 @@ static int check_one_conflict(struct index_state *istate, int i, int *type)
 	}
 
 	*type = PUNTED;
-	while (i < istate->cache_nr && ce_stage(istate->cache[i]) == 1)
-		i++;
-
-	/* Only handle regular files with both stages #2 and #3 */
-	if (i + 1 < istate->cache_nr) {
-		const struct cache_entry *e2 = istate->cache[i];
-		const struct cache_entry *e3 = istate->cache[i + 1];
-		if (ce_stage(e2) == 2 &&
-		    ce_stage(e3) == 3 &&
-		    ce_same_name(e, e3) &&
-		    S_ISREG(e2->ce_mode) &&
-		    S_ISREG(e3->ce_mode))
-			*type = THREE_STAGED;
+	for (; i < istate->cache_nr; i++) {
+		const struct cache_entry *n = istate->cache[i];
+		if (!ce_same_name(n, e))
+			break;
+		if (S_ISREG(n->ce_mode))
+			seen_stages |= 1u << (ce_stage(n) - 1);
 	}
 
-	/* Skip the entries with the same name */
-	while (i < istate->cache_nr && ce_same_name(e, istate->cache[i]))
-		i++;
+	if ((seen_stages & 6) == 6)
+		*type = THREE_STAGED; /* has both stages #2 and #3 */
+
 	return i;
 }

-- 
2.42.0-29-gcd9da15a85

Junio C Hamano· Aug 24, 2023, 20:54 UTC · re: Junio C Hamano · lore

[PATCH 4/4] rerere: modernize use of empty strbuf

Back when the code in the handle_conflict() helper function that hashes the contents stored in the strbuf "one" and "two", including its terminating NUL, was written, a freshly initialized strbuf had NULL in its .buf member, so it was an error to say

    update(one.buf, one.len + 1)

which was corrected by b4833a2c (rerere: Fix use of an empty strbuf.buf, 2007-09-26).

But soon after that, b315c5c0 (strbuf change: be sure ->buf is never ever NULL., 2007-09-27) introduced strbuf_slopbuf mechanism that ensures that .buf member of a strbuf is *never* NULL. A freshly initialized and empty strbuf uses a static piece of memory that has NUL in it, with its .len member set to 0, so we can always safely use from offset 0 of .buf[] array for (one.len + 1) bytes.

Simplify the code by essentially reverting the b4833a2c, whose fix is no longer necessary in the modern world order.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 rerere.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Show changes to rerere.c +2 −6

diff --git a/rerere.c b/rerere.c
index 6bc3c54d3b..69a61aac93 100644
--- a/rerere.c
+++ b/rerere.c
@@ -390,12 +390,8 @@ static int handle_conflict(struct strbuf *out, struct rerere_io *io,
 			strbuf_addbuf(out, &two);
 			rerere_strbuf_putconflict(out, '>', marker_size);
 			if (ctx) {
-				the_hash_algo->update_fn(ctx, one.buf ?
-							 one.buf : "",
-							 one.len + 1);
-				the_hash_algo->update_fn(ctx, two.buf ?
-							 two.buf : "",
-							 two.len + 1);
+				the_hash_algo->update_fn(ctx, one.buf, one.len + 1);
+				the_hash_algo->update_fn(ctx, two.buf, two.len + 1);
 			}
 			break;
 		} else if (hunk == RR_SIDE_1)

-- 
2.42.0-29-gcd9da15a85

Junio C Hamano· Aug 24, 2023, 20:54 UTC · re: Junio C Hamano · lore

[PATCH 2/4] rerere: fix comment on handle_file() helper

The return value from handle_path() is returned to the caller of handle_file() in the normal cases, and it is not the number of hunks. It is just a normal C Boolean, "do we (!=0) or do we not (0) have conflict?" plus "a negative return value signals an error".

And all the callers of handle_file() understands its return value as such. Update the comment to match the reality after 221444f5 (rerere: only return whether a path has conflicts or not, 2018-08-05), which apparently forgot to update this comment when it turned the returned value of this function from the number of conflict hunks to a boolean plus error.

Signed-off-by: Junio C Hamano <gitster@pobox.com>

---
cc: Thomas Gummerer <t.gummerer@gmail.com>
---
 rerere.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Show changes to rerere.c +1 −1

diff --git a/rerere.c b/rerere.c
index 73cdc8392f..4ce1270a94 100644
--- a/rerere.c
+++ b/rerere.c
@@ -454,7 +454,7 @@ static int handle_path(unsigned char *hash, struct rerere_io *io, int marker_siz
 
 /*
  * Scan the path for conflicts, do the "handle_path()" thing above, and
- * return the number of conflict hunks found.
+ * return true iff conflict hunks were found.
  */
 static int handle_file(struct index_state *istate,
 		       const char *path, unsigned char *hash, const char *output)

-- 
2.42.0-29-gcd9da15a85