[PATCH v2 00/13] "rerere" minor clean-up

[PATCH v2 00/13] "rerere" minor clean-up

[Junio C Hamano]

Here is an collection of various minor clean-ups in the
implementation of one of my most favourite feature, rerere.

This still hasn't reached the step to make the right refactoring to
allow me to fix a bug I wanted to fix, which prompted me to look at
this code, but it should give me a good starting point.

Junio C Hamano (13):
  rerere: fix an off-by-one non-bug
  rerere: plug conflict ID leaks
  rerere: lift PATH_MAX limitation
  rerere: write out each record of MERGE_RR in one go
  rerere: report autoupdated paths only after actually updating them
  rerere: drop want_sp parameter from is_cmarker()
  rerere: stop looping unnecessarily
  rerere: explain the rerere I/O abstraction
  rerere: explain MERGE_RR management helpers
  rerere: explain the primary codepath
  rerere: explain "rerere forget" codepath
  rerere: explain the remainder
  rerere: refactor "replay" part of do_plain_rerere()

 rerere.c | 389 +++++++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 293 insertions(+), 96 deletions(-)

-- 
2.5.0-rc0-209-g5e1f148

[PATCH v2 01/13] rerere: fix an off-by-one non-bug

[Junio C Hamano]

When ac49f5ca (rerere "remaining", 2011-02-16) split out a new
helper function check_one_conflict() out of find_conflict()
function, so that the latter will use the returned value from the
new helper to update the loop control variable that is an index into
active_cache[], the new variable incremented the index by one too
many when it found a path with only stage #1 entry at the very end
of active_cache[].

This "strange" return value does not have any effect on the loop
control of two callers of this function, as they all notice that
active_nr+2 is larger than active_nr just like active_nr+1 is, but
nevertheless it puzzles the readers when they are trying to figure
out what the function is trying to do.

In fact, there is no need to do an early return.  The code that
follows after skipping the stage #1 entry is fully prepared to
handle a case where the entry is at the very end of active_cache[].

Help future readers from unnecessary confusion by dropping an early
return.  We skip the stage #1 entry, and if there are stage #2 and
stage #3 entries for the same path, we diagnose the path as
THREE_STAGED (otherwise we say PUNTED), and then we skip all entries
for the same path.

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

diff --git a/rerere.c b/rerere.c
index 31644de..e307711 100644
--- a/rerere.c
+++ b/rerere.c
@@ -369,10 +369,8 @@ static int check_one_conflict(int i, int *type)
 	}
 
 	*type = PUNTED;
-	if (ce_stage(e) == 1) {
-		if (active_nr <= ++i)
-			return i + 1;
-	}
+	if (ce_stage(e) == 1)
+		i++;
 
 	/* Only handle regular files with both stages #2 and #3 */
 	if (i + 1 < active_nr) {
-- 
2.5.0-rc0-209-g5e1f148

[PATCH v2 02/13] rerere: plug conflict ID leaks

[Junio C Hamano]

The merge_rr string list stores the conflict ID (a hexadecimal
string that is used to index into $GIT_DIR/rr-cache) in the .util
field of its elements, and when do_plain_rerere() resolves a
conflict, the field is cleared.  Also, when rerere_forget()
recomputes the conflict ID to updates the preimage file, the
conflict ID for the path is updated.

We forgot to free the existing conflict ID when we did these two
operations.

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

diff --git a/rerere.c b/rerere.c
index e307711..3b9104d 100644
--- a/rerere.c
+++ b/rerere.c
@@ -559,6 +559,7 @@ static int do_plain_rerere(struct string_list *rr, int fd)
 		fprintf(stderr, "Recorded resolution for '%s'.\n", path);
 		copy_file(rerere_path(name, "postimage"), path, 0666);
 	mark_resolved:
+		free(rr->items[i].util);
 		rr->items[i].util = NULL;
 	}
 
@@ -627,6 +628,7 @@ static int rerere_forget_one_path(const char *path, struct string_list *rr)
 	char *hex;
 	unsigned char sha1[20];
 	int ret;
+	struct string_list_item *item;
 
 	ret = handle_cache(path, sha1, NULL);
 	if (ret < 1)
@@ -641,8 +643,9 @@ static int rerere_forget_one_path(const char *path, struct string_list *rr)
 	handle_cache(path, sha1, rerere_path(hex, "preimage"));
 	fprintf(stderr, "Updated preimage for '%s'\n", path);
 
-
-	string_list_insert(rr, path)->util = hex;
+	item = string_list_insert(rr, path);
+	free(item->util);
+	item->util = hex;
 	fprintf(stderr, "Forgot resolution for %s\n", path);
 	return 0;
 }
-- 
2.5.0-rc0-209-g5e1f148

[PATCH v2 03/13] rerere: lift PATH_MAX limitation

[Junio C Hamano]

The MERGE_RR file records a collection of NUL-terminated entries,
each of which consists of

 - a hash that identifies the conflict
 - a HT
 - the pathname

We used to read this piece-by-piece, and worse yet, read the
pathname part a byte at a time into a fixed buffer of size PATH_MAX.

Instead, read a whole entry using strbuf_getwholeline() and parse
out the fields.  This way, we issue fewer read(2) calls and more
importantly we do not have to limit the pathname to PATH_MAX.

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

diff --git a/rerere.c b/rerere.c
index 3b9104d..e854985 100644
--- a/rerere.c
+++ b/rerere.c
@@ -35,32 +35,27 @@ static int has_rerere_resolution(const char *hex)
 
 static void read_rr(struct string_list *rr)
 {
-	unsigned char sha1[20];
-	char buf[PATH_MAX];
+	struct strbuf buf = STRBUF_INIT;
 	FILE *in = fopen(merge_rr_path, "r");
+
 	if (!in)
 		return;
-	while (fread(buf, 40, 1, in) == 1) {
-		int i;
-		char *name;
-		if (get_sha1_hex(buf, sha1))
+	while (!strbuf_getwholeline(&buf, in, '\0')) {
+		char *path;
+		unsigned char sha1[20];
+
+		/* There has to be the hash, tab, path and then NUL */
+		if (buf.len < 42 || get_sha1_hex(buf.buf, sha1))
 			die("corrupt MERGE_RR");
-		buf[40] = '\0';
-		name = xstrdup(buf);
-		if (fgetc(in) != '\t')
+
+		if (buf.buf[40] != '\t')
 			die("corrupt MERGE_RR");
-		for (i = 0; i < sizeof(buf); i++) {
-			int c = fgetc(in);
-			if (c < 0)
-				die("corrupt MERGE_RR");
-			buf[i] = c;
-			if (c == 0)
-				 break;
-		}
-		if (i == sizeof(buf))
-			die("filename too long");
-		string_list_insert(rr, buf)->util = name;
+		buf.buf[40] = '\0';
+		path = buf.buf + 41;
+
+		string_list_insert(rr, path)->util = xstrdup(buf.buf);
 	}
+	strbuf_release(&buf);
 	fclose(in);
 }
 
-- 
2.5.0-rc0-209-g5e1f148

[PATCH v2 04/13] rerere: write out each record of MERGE_RR in one go

[Junio C Hamano]

Instead of writing the hash for a conflict, a HT, and the path
with three separate write_in_full() calls, format them into a
single record into a strbuf and write it out in one go.

As a more recent "rerere remaining" codepath abuses the .util field
of the merge_rr data to store a sentinel token, make sure that
codepath does not call into this function (of course, "remaining" is
a read-only operation and currently does not call it).

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

diff --git a/rerere.c b/rerere.c
index e854985..27b287d 100644
--- a/rerere.c
+++ b/rerere.c
@@ -65,16 +65,18 @@ static int write_rr(struct string_list *rr, int out_fd)
 {
 	int i;
 	for (i = 0; i < rr->nr; i++) {
-		const char *path;
-		int length;
+		struct strbuf buf = STRBUF_INIT;
+
+		assert(rr->items[i].util != RERERE_RESOLVED);
 		if (!rr->items[i].util)
 			continue;
-		path = rr->items[i].string;
-		length = strlen(path) + 1;
-		if (write_in_full(out_fd, rr->items[i].util, 40) != 40 ||
-		    write_str_in_full(out_fd, "\t") != 1 ||
-		    write_in_full(out_fd, path, length) != length)
+		strbuf_addf(&buf, "%s\t%s%c",
+			    (char *)rr->items[i].util,
+			    rr->items[i].string, 0);
+		if (write_in_full(out_fd, buf.buf, buf.len) != buf.len)
 			die("unable to write rerere record");
+
+		strbuf_release(&buf);
 	}
 	if (commit_lock_file(&write_lock) != 0)
 		die("unable to write rerere record");
-- 
2.5.0-rc0-209-g5e1f148

[PATCH v2 05/13] rerere: report autoupdated paths only after actually updating them

[Junio C Hamano]

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

diff --git a/rerere.c b/rerere.c
index 27b287d..304df02 100644
--- a/rerere.c
+++ b/rerere.c
@@ -482,6 +482,8 @@ static void update_paths(struct string_list *update)
 		struct string_list_item *item = &update->items[i];
 		if (add_file_to_cache(item->string, 0))
 			exit(128);
+		fprintf(stderr, "Staged '%s' using previous resolution.\n",
+			item->string);
 	}
 
 	if (active_cache_changed) {
@@ -536,16 +538,16 @@ static int do_plain_rerere(struct string_list *rr, int fd)
 		const char *name = (const char *)rr->items[i].util;
 
 		if (has_rerere_resolution(name)) {
-			if (!merge(name, path)) {
-				const char *msg;
-				if (rerere_autoupdate) {
-					string_list_insert(&update, path);
-					msg = "Staged '%s' using previous resolution.\n";
-				} else
-					msg = "Resolved '%s' using previous resolution.\n";
-				fprintf(stderr, msg, path);
-				goto mark_resolved;
-			}
+			if (merge(name, path))
+				continue;
+
+			if (rerere_autoupdate)
+				string_list_insert(&update, path);
+			else
+				fprintf(stderr,
+					"Resolved '%s' using previous resolution.\n",
+					path);
+			goto mark_resolved;
 		}
 
 		/* Let's see if we have resolved it. */
-- 
2.5.0-rc0-209-g5e1f148

[PATCH v2 06/13] rerere: drop want_sp parameter from is_cmarker()

[Junio C Hamano]

As the nature of the conflict marker line determies if there should
a SP and label after it, the caller shouldn't have to pass the
parameter redundantly.

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

diff --git a/rerere.c b/rerere.c
index 304df02..4c45f55 100644
--- a/rerere.c
+++ b/rerere.c
@@ -148,8 +148,25 @@ static int rerere_file_getline(struct strbuf *sb, struct rerere_io *io_)
 	return strbuf_getwholeline(sb, io->input, '\n');
 }
 
-static int is_cmarker(char *buf, int marker_char, int marker_size, int want_sp)
+/*
+ * Require the exact number of conflict marker letters, no more, no
+ * less, followed by SP or any whitespace
+ * (including LF).
+ */
+static int is_cmarker(char *buf, int marker_char, int marker_size)
 {
+	int want_sp;
+
+	/*
+	 * The beginning of our version and the end of their version
+	 * always are labeled like "<<<<< ours" or ">>>>> theirs",
+	 * hence we set want_sp for them.  Note that the version from
+	 * the common ancestor in diff3-style output is not always
+	 * labelled (e.g. "||||| common" is often seen but "|||||"
+	 * alone is also valid), so we do not set want_sp.
+	 */
+	want_sp = (marker_char == '<') || (marker_char == '>');
+
 	while (marker_size--)
 		if (*buf++ != marker_char)
 			return 0;
@@ -172,19 +189,19 @@ static int handle_path(unsigned char *sha1, struct rerere_io *io, int marker_siz
 		git_SHA1_Init(&ctx);
 
 	while (!io->getline(&buf, io)) {
-		if (is_cmarker(buf.buf, '<', marker_size, 1)) {
+		if (is_cmarker(buf.buf, '<', marker_size)) {
 			if (hunk != RR_CONTEXT)
 				goto bad;
 			hunk = RR_SIDE_1;
-		} else if (is_cmarker(buf.buf, '|', marker_size, 0)) {
+		} else if (is_cmarker(buf.buf, '|', marker_size)) {
 			if (hunk != RR_SIDE_1)
 				goto bad;
 			hunk = RR_ORIGINAL;
-		} else if (is_cmarker(buf.buf, '=', marker_size, 0)) {
+		} else if (is_cmarker(buf.buf, '=', marker_size)) {
 			if (hunk != RR_SIDE_1 && hunk != RR_ORIGINAL)
 				goto bad;
 			hunk = RR_SIDE_2;
-		} else if (is_cmarker(buf.buf, '>', marker_size, 1)) {
+		} else if (is_cmarker(buf.buf, '>', marker_size)) {
 			if (hunk != RR_SIDE_2)
 				goto bad;
 			if (strbuf_cmp(&one, &two) > 0)
-- 
2.5.0-rc0-209-g5e1f148

[PATCH v2 07/13] rerere: stop looping unnecessarily

[Junio C Hamano]

handle_cache() loops 3 times starting from an index entry that is
unmerged, while ignoring an entry for a path that is different from
what we are looking for.

As the index is sorted, once we see a different path, we know we saw
all stages for the path we are interested in.  Just loop while we
see the same path and then break, instead of continuing for 3 times.

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

diff --git a/rerere.c b/rerere.c
index 4c45f55..7b1419c 100644
--- a/rerere.c
+++ b/rerere.c
@@ -329,24 +329,21 @@ static int handle_cache(const char *path, unsigned char *sha1, const char *outpu
 		return -1;
 	pos = -pos - 1;
 
-	for (i = 0; i < 3; i++) {
+	while (pos < active_nr) {
 		enum object_type type;
 		unsigned long size;
-		int j;
 
-		if (active_nr <= pos)
-			break;
 		ce = active_cache[pos++];
 		if (ce_namelen(ce) != len || memcmp(ce->name, path, len))
-			continue;
-		j = ce_stage(ce) - 1;
-		mmfile[j].ptr = read_sha1_file(ce->sha1, &type, &size);
-		mmfile[j].size = size;
+			break;
+		i = ce_stage(ce) - 1;
+		mmfile[i].ptr = read_sha1_file(ce->sha1, &type, &size);
+		mmfile[i].size = size;
 	}
-	for (i = 0; i < 3; i++) {
+	for (i = 0; i < 3; i++)
 		if (!mmfile[i].ptr && !mmfile[i].size)
 			mmfile[i].ptr = xstrdup("");
-	}
+
 	/*
 	 * NEEDSWORK: handle conflicts from merges with
 	 * merge.renormalize set, too
-- 
2.5.0-rc0-209-g5e1f148

[PATCH v2 08/13] rerere: explain the rerere I/O abstraction

[Junio C Hamano]

Explain the internals of rerere as in-code comments.

This one covers our thin I/O abstraction to read from either
a file or a memory while optionally writing out to a file.

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

diff --git a/rerere.c b/rerere.c
index 7b1419c..7ed20f1 100644
--- a/rerere.c
+++ b/rerere.c
@@ -83,6 +83,21 @@ static int write_rr(struct string_list *rr, int out_fd)
 	return 0;
 }
 
+/*
+ * "rerere" interacts with conflicted file contents using this I/O
+ * abstraction.  It reads a conflicted contents from one place via
+ * "getline()" method, and optionally can write it out after
+ * normalizing the conflicted hunks to the "output".  Subclasses of
+ * rerere_io embed this structure at the beginning of their own
+ * rerere_io object.
+ */
+struct rerere_io {
+	int (*getline)(struct strbuf *, struct rerere_io *);
+	FILE *output;
+	int wrerror;
+	/* some more stuff */
+};
+
 static void ferr_write(const void *p, size_t count, FILE *fp, int *err)
 {
 	if (!count || *err)
@@ -96,19 +111,15 @@ static inline void ferr_puts(const char *s, FILE *fp, int *err)
 	ferr_write(s, strlen(s), fp, err);
 }
 
-struct rerere_io {
-	int (*getline)(struct strbuf *, struct rerere_io *);
-	FILE *output;
-	int wrerror;
-	/* some more stuff */
-};
-
 static void rerere_io_putstr(const char *str, struct rerere_io *io)
 {
 	if (io->output)
 		ferr_puts(str, io->output, &io->wrerror);
 }
 
+/*
+ * Write a conflict marker to io->output (if defined).
+ */
 static void rerere_io_putconflict(int ch, int size, struct rerere_io *io)
 {
 	char buf[64];
@@ -137,11 +148,17 @@ static void rerere_io_putmem(const char *mem, size_t sz, struct rerere_io *io)
 		ferr_write(mem, sz, io->output, &io->wrerror);
 }
 
+/*
+ * Subclass of rerere_io that reads from an on-disk file
+ */
 struct rerere_io_file {
 	struct rerere_io io;
 	FILE *input;
 };
 
+/*
+ * ... and its getline() method implementation
+ */
 static int rerere_file_getline(struct strbuf *sb, struct rerere_io *io_)
 {
 	struct rerere_io_file *io = (struct rerere_io_file *)io_;
@@ -286,11 +303,18 @@ static int handle_file(const char *path, unsigned char *sha1, const char *output
 	return hunk_no;
 }
 
+/*
+ * Subclass of rerere_io that reads from an in-core buffer that is a
+ * strbuf
+ */
 struct rerere_io_mem {
 	struct rerere_io io;
 	struct strbuf input;
 };
 
+/*
+ * ... and its getline() method implementation
+ */
 static int rerere_mem_getline(struct strbuf *sb, struct rerere_io *io_)
 {
 	struct rerere_io_mem *io = (struct rerere_io_mem *)io_;
-- 
2.5.0-rc0-209-g5e1f148

[PATCH v2 09/13] rerere: explain MERGE_RR management helpers

[Junio C Hamano]

Explain the internals of rerere as in-code comments, while
sprinkling "NEEDSWORK" comment to highlight iffy bits and
questionable assumptions.

This one covers the "$GIT_DIR/MERGE_RR" file and in-core merge_rr
that are used to keep track of the status of "rerere" session in
progress.

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

diff --git a/rerere.c b/rerere.c
index 7ed20f1..d54bdb2 100644
--- a/rerere.c
+++ b/rerere.c
@@ -33,6 +33,13 @@ static int has_rerere_resolution(const char *hex)
 	return !stat(rerere_path(hex, "postimage"), &st);
 }
 
+/*
+ * $GIT_DIR/MERGE_RR file is a collection of records, each of which is
+ * "conflict ID", a HT and pathname, terminated with a NUL, and is
+ * used to keep track of the set of paths that "rerere" may need to
+ * work on (i.e. what is left by the previous invocation of "git
+ * rerere" during the current conflict resolution session).
+ */
 static void read_rr(struct string_list *rr)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -394,6 +401,14 @@ static int handle_cache(const char *path, unsigned char *sha1, const char *outpu
 	return hunk_no;
 }
 
+/*
+ * 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.
+ * Return the cache index to be looked at next, by skipping the
+ * stages we have already looked at in this invocation of this
+ * function.
+ */
 static int check_one_conflict(int i, int *type)
 {
 	const struct cache_entry *e = active_cache[i];
@@ -425,6 +440,17 @@ static int check_one_conflict(int i, int *type)
 	return i;
 }
 
+/*
+ * Scan the index and find paths that have conflicts that rerere can
+ * handle, i.e. the ones that has both stages #2 and #3.
+ *
+ * NEEDSWORK: we do not record or replay a previous "resolve by
+ * deletion" for a delete-modify conflict, as that is inherently risky
+ * without knowing what modification is being discarded.  The only
+ * safe case, i.e. both side doing the deletion and modification that
+ * are identical to the previous round, might want to be handled,
+ * though.
+ */
 static int find_conflict(struct string_list *conflict)
 {
 	int i;
@@ -441,6 +467,21 @@ static int find_conflict(struct string_list *conflict)
 	return 0;
 }
 
+/*
+ * The merge_rr list is meant to hold outstanding conflicted paths
+ * that rerere could handle.  Abuse the list by adding other types of
+ * entries to allow the caller to show "rerere remaining".
+ *
+ * - Conflicted paths that rerere does not handle are added
+ * - Conflicted paths that have been resolved are marked as such
+ *   by storing RERERE_RESOLVED to .util field (where conflict ID
+ *   is expected to be stored).
+ *
+ * Do *not* write MERGE_RR file out after calling this function.
+ *
+ * NEEDSWORK: we may want to fix the caller that implements "rerere
+ * remaining" to do this without abusing merge_rr.
+ */
 int rerere_remaining(struct string_list *merge_rr)
 {
 	int i;
-- 
2.5.0-rc0-209-g5e1f148

[PATCH v2 10/13] rerere: explain the primary codepath

[Junio C Hamano]

Explain the internals of rerere as in-code comments, while
sprinkling "NEEDSWORK" comment to highlight iffy bits and
questionable assumptions.

This one covers the codepath reached from rerere(), the primary
interface to the subsystem.

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

diff --git a/rerere.c b/rerere.c
index d54bdb2..3d9c33b 100644
--- a/rerere.c
+++ b/rerere.c
@@ -199,6 +199,21 @@ static int is_cmarker(char *buf, int marker_char, int marker_size)
 	return isspace(*buf);
 }
 
+/*
+ * Read contents a file with conflicts, normalize the conflicts
+ * by (1) discarding the common ancestor version in diff3-style,
+ * (2) reordering our side and their side so that whichever sorts
+ * alphabetically earlier comes before the other one, while
+ * computing the "conflict ID", which is just an SHA-1 hash of
+ * one side of the conflict, NUL, the other side of the conflict,
+ * and NUL concatenated together.
+ *
+ * Return the number of conflict hunks found.
+ *
+ * NEEDSWORK: the logic and theory of operation behind this conflict
+ * normalization may deserve to be documented somewhere, perhaps in
+ * Documentation/technical/rerere.txt.
+ */
 static int handle_path(unsigned char *sha1, struct rerere_io *io, int marker_size)
 {
 	git_SHA_CTX ctx;
@@ -269,6 +284,10 @@ static int handle_path(unsigned char *sha1, struct rerere_io *io, int marker_siz
 	return hunk_no;
 }
 
+/*
+ * Scan the path for conflicts, do the "handle_path()" thing above, and
+ * return the number of conflict hunks found.
+ */
 static int handle_file(const char *path, unsigned char *sha1, const char *output)
 {
 	int hunk_no = 0;
@@ -506,29 +525,54 @@ int rerere_remaining(struct string_list *merge_rr)
 	return 0;
 }
 
+/*
+ * Find the conflict identified by "name"; the change between its
+ * "preimage" (i.e. a previous contents with conflict markers) and its
+ * "postimage" (i.e. the corresponding contents with conflicts
+ * resolved) may apply cleanly to the contents stored in "path", i.e.
+ * the conflict this time around.
+ *
+ * Returns 0 for successful replay of recorded resolution, or non-zero
+ * for failure.
+ */
 static int merge(const char *name, const char *path)
 {
 	int ret;
 	mmfile_t cur = {NULL, 0}, base = {NULL, 0}, other = {NULL, 0};
 	mmbuffer_t result = {NULL, 0};
 
+	/*
+	 * Normalize the conflicts in path and write it out to
+	 * "thisimage" temporary file.
+	 */
 	if (handle_file(path, NULL, rerere_path(name, "thisimage")) < 0)
 		return 1;
 
 	if (read_mmfile(&cur, rerere_path(name, "thisimage")) ||
-			read_mmfile(&base, rerere_path(name, "preimage")) ||
-			read_mmfile(&other, rerere_path(name, "postimage"))) {
+	    read_mmfile(&base, rerere_path(name, "preimage")) ||
+	    read_mmfile(&other, rerere_path(name, "postimage"))) {
 		ret = 1;
 		goto out;
 	}
+
+	/*
+	 * A three-way merge. Note that this honors user-customizable
+	 * low-level merge driver settings.
+	 */
 	ret = ll_merge(&result, path, &base, NULL, &cur, "", &other, "", NULL);
 	if (!ret) {
 		FILE *f;
 
+		/*
+		 * A successful replay of recorded resolution.
+		 * Mark that "postimage" was used to help gc.
+		 */
 		if (utime(rerere_path(name, "postimage"), NULL) < 0)
 			warning("failed utime() on %s: %s",
 					rerere_path(name, "postimage"),
 					strerror(errno));
+
+		/* Update "path" with the resolution */
 		f = fopen(path, "w");
 		if (!f)
 			return error("Could not open %s: %s", path,
@@ -581,41 +625,61 @@ static int do_plain_rerere(struct string_list *rr, int fd)
 	find_conflict(&conflict);
 
 	/*
-	 * MERGE_RR records paths with conflicts immediately after merge
-	 * failed.  Some of the conflicted paths might have been hand resolved
-	 * in the working tree since then, but the initial run would catch all
-	 * and register their preimages.
+	 * MERGE_RR records paths with conflicts immediately after
+	 * merge failed.  Some of the conflicted paths might have been
+	 * hand resolved in the working tree since then, but the
+	 * initial run would catch all and register their preimages.
 	 */
-
 	for (i = 0; i < conflict.nr; i++) {
 		const char *path = conflict.items[i].string;
 		if (!string_list_has_string(rr, path)) {
 			unsigned char sha1[20];
 			char *hex;
 			int ret;
+
+			/*
+			 * Ask handle_file() to scan and assign a
+			 * conflict ID.  No need to write anything out
+			 * yet.
+			 */
 			ret = handle_file(path, sha1, NULL);
 			if (ret < 1)
 				continue;
 			hex = xstrdup(sha1_to_hex(sha1));
 			string_list_insert(rr, path)->util = hex;
+
+			/*
+			 * If the directory does not exist, create
+			 * it.  mkdir_in_gitdir() will fail with
+			 * EEXIST if there already is one.
+			 *
+			 * NEEDSWORK: make sure "gc" does not remove
+			 * preimage without removing the directory.
+			 */
 			if (mkdir_in_gitdir(git_path("rr-cache/%s", hex)))
 				continue;
+
+			/*
+			 * We are the first to encounter this
+			 * conflict.  Ask handle_file() to write the
+			 * normalized contents to the "preimage" file.
+			 */
 			handle_file(path, NULL, rerere_path(hex, "preimage"));
 			fprintf(stderr, "Recorded preimage for '%s'\n", path);
 		}
 	}
 
 	/*
-	 * Now some of the paths that had conflicts earlier might have been
-	 * hand resolved.  Others may be similar to a conflict already that
-	 * was resolved before.
+	 * Some of the paths that had conflicts earlier might have
+	 * been resolved by the user.  Others may be similar to a
+	 * conflict already that was resolved before.
 	 */
-
 	for (i = 0; i < rr->nr; i++) {
 		int ret;
 		const char *path = rr->items[i].string;
 		const char *name = (const char *)rr->items[i].util;
 
+		/* Is there a recorded resolution we could attempt to apply? */
 		if (has_rerere_resolution(name)) {
 			if (merge(name, path))
 				continue;
@@ -629,13 +693,13 @@ static int do_plain_rerere(struct string_list *rr, int fd)
 			goto mark_resolved;
 		}
 
-		/* Let's see if we have resolved it. */
+		/* Let's see if the user has resolved it. */
 		ret = handle_file(path, NULL, NULL);
 		if (ret)
 			continue;
 
-		fprintf(stderr, "Recorded resolution for '%s'.\n", path);
 		copy_file(rerere_path(name, "postimage"), path, 0666);
+		fprintf(stderr, "Recorded resolution for '%s'.\n", path);
 	mark_resolved:
 		free(rr->items[i].util);
 		rr->items[i].util = NULL;
@@ -689,6 +753,11 @@ int setup_rerere(struct string_list *merge_rr, int flags)
 	return fd;
 }
 
+/*
+ * The main entry point that is called internally from codepaths that
+ * perform mergy operations, possibly leaving conflicted index entries
+ * and working tree files.
+ */
 int rerere(int flags)
 {
 	struct string_list merge_rr = STRING_LIST_INIT_DUP;
-- 
2.5.0-rc0-209-g5e1f148

[PATCH v2 11/13] rerere: explain "rerere forget" codepath

[Junio C Hamano]

Explain the internals of rerere as in-code comments, while
sprinkling "NEEDSWORK" comment to highlight iffy bits and
questionable assumptions.

This covers the codepath that implements "rerere forget".

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

diff --git a/rerere.c b/rerere.c
index 3d9c33b..3782be6 100644
--- a/rerere.c
+++ b/rerere.c
@@ -413,6 +413,10 @@ static int handle_cache(const char *path, unsigned char *sha1, const char *outpu
 	strbuf_init(&io.input, 0);
 	strbuf_attach(&io.input, result.ptr, result.size, result.size);
 
+	/*
+	 * Grab the conflict ID and optionally write the original
+	 * contents with conflict markers out.
+	 */
 	hunk_no = handle_path(sha1, (struct rerere_io *)&io, marker_size);
 	strbuf_release(&io.input);
 	if (io.io.output)
@@ -777,9 +781,15 @@ static int rerere_forget_one_path(const char *path, struct string_list *rr)
 	int ret;
 	struct string_list_item *item;
 
+	/*
+	 * Recreate the original conflict from the stages in the
+	 * index and compute the conflict ID
+	 */
 	ret = handle_cache(path, sha1, NULL);
 	if (ret < 1)
 		return error("Could not parse conflict hunks in '%s'", path);
+
+	/* Nuke the recorded resolution for the conflict */
 	hex = xstrdup(sha1_to_hex(sha1));
 	filename = rerere_path(hex, "postimage");
 	if (unlink(filename))
@@ -787,9 +797,18 @@ static int rerere_forget_one_path(const char *path, struct string_list *rr)
 			? error("no remembered resolution for %s", path)
 			: error("cannot unlink %s: %s", filename, strerror(errno)));
 
+	/*
+	 * Update the preimage so that the user can resolve the
+	 * conflict in the working tree, run us again to record
+	 * the postimage.
+	 */
 	handle_cache(path, sha1, rerere_path(hex, "preimage"));
 	fprintf(stderr, "Updated preimage for '%s'\n", path);
 
+	/*
+	 * And remember that we can record resolution for this
+	 * conflict when the user is done.
+	 */
 	item = string_list_insert(rr, path);
 	free(item->util);
 	item->util = hex;
@@ -808,6 +827,11 @@ int rerere_forget(struct pathspec *pathspec)
 
 	fd = setup_rerere(&merge_rr, RERERE_NOAUTOUPDATE);
 
+	/*
+	 * The paths may have been resolved (incorrectly);
+	 * recover the original conflicted state and then
+	 * find the conflicted paths.
+	 */
 	unmerge_cache(pathspec);
 	find_conflict(&conflict);
 	for (i = 0; i < conflict.nr; i++) {
-- 
2.5.0-rc0-209-g5e1f148

[PATCH v2 12/13] rerere: explain the remainder

[Junio C Hamano]

Explain the internals of rerere as in-code comments, while
sprinkling "NEEDSWORK" comment to highlight iffy bits and
questionable assumptions.

This covers the codepath that implements "rerere gc" and "rerere
clear".

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

diff --git a/rerere.c b/rerere.c
index 3782be6..7ef951e 100644
--- a/rerere.c
+++ b/rerere.c
@@ -844,6 +844,9 @@ int rerere_forget(struct pathspec *pathspec)
 	return write_rr(&merge_rr, fd);
 }
 
+/*
+ * Garbage collection support
+ */
 static time_t rerere_created_at(const char *name)
 {
 	struct stat st;
@@ -856,11 +859,19 @@ static time_t rerere_last_used_at(const char *name)
 	return stat(rerere_path(name, "postimage"), &st) ? (time_t) 0 : st.st_mtime;
 }
 
+/*
+ * Remove the recorded resolution for a given conflict ID
+ */
 static void unlink_rr_item(const char *name)
 {
 	unlink(rerere_path(name, "thisimage"));
 	unlink(rerere_path(name, "preimage"));
 	unlink(rerere_path(name, "postimage"));
+	/*
+	 * NEEDSWORK: what if this rmdir() fails?  Wouldn't we then
+	 * assume that we already have preimage recorded in
+	 * do_plain_rerere()?
+	 */
 	rmdir(git_path("rr-cache/%s", name));
 }
 
@@ -880,6 +891,7 @@ void rerere_gc(struct string_list *rr)
 	dir = opendir(git_path("rr-cache"));
 	if (!dir)
 		die_errno("unable to open rr-cache directory");
+	/* Collect stale conflict IDs ... */
 	while ((e = readdir(dir))) {
 		if (is_dot_or_dotdot(e->d_name))
 			continue;
@@ -897,11 +909,19 @@ void rerere_gc(struct string_list *rr)
 			string_list_append(&to_remove, e->d_name);
 	}
 	closedir(dir);
+	/* ... and then remove them one-by-one */
 	for (i = 0; i < to_remove.nr; i++)
 		unlink_rr_item(to_remove.items[i].string);
 	string_list_clear(&to_remove, 0);
 }
 
+/*
+ * During a conflict resolution, after "rerere" recorded the
+ * preimages, abandon them if the user did not resolve them or
+ * record their resolutions.  And drop $GIT_DIR/MERGE_RR.
+ *
+ * NEEDSWORK: shouldn't we be calling this from "reset --hard"?
+ */
 void rerere_clear(struct string_list *merge_rr)
 {
 	int i;
-- 
2.5.0-rc0-209-g5e1f148

[PATCH v2 13/13] rerere: refactor "replay" part of do_plain_rerere()

[Junio C Hamano]

Extract the body of a loop that attempts to replay recorded
resolution for each conflicted path into a helper function, not
because I want to call it from multiple places later, but because
the logic has become too deeply nested and hard to read.

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

diff --git a/rerere.c b/rerere.c
index 7ef951e..09b72ed 100644
--- a/rerere.c
+++ b/rerere.c
@@ -620,6 +620,44 @@ static void update_paths(struct string_list *update)
 		rollback_lock_file(&index_lock);
 }
 
+/*
+ * The path indicated by rr_item may still have conflict for which we
+ * have a recorded resolution, in which case replay it and optionally
+ * update it.  Or it may have been resolved by the user and we may
+ * only have the preimage for that conflict, in which case the result
+ * needs to be recorded as a resolution in a postimage file.
+ */
+static void do_rerere_one_path(struct string_list_item *rr_item,
+			       struct string_list *update)
+{
+	const char *path = rr_item->string;
+	const char *name = (const char *)rr_item->util;
+
+	/* Is there a recorded resolution we could attempt to apply? */
+	if (has_rerere_resolution(name)) {
+		if (merge(name, path))
+			return; /* failed to replay */
+
+		if (rerere_autoupdate)
+			string_list_insert(update, path);
+		else
+			fprintf(stderr,
+				"Resolved '%s' using previous resolution.\n",
+				path);
+		goto mark_resolved;
+	}
+
+	/* Let's see if the user has resolved it. */
+	if (handle_file(path, NULL, NULL))
+		return; /* not yet resolved */
+
+	copy_file(rerere_path(name, "postimage"), path, 0666);
+	fprintf(stderr, "Recorded resolution for '%s'.\n", path);
+mark_resolved:
+	free(rr_item->util);
+	rr_item->util = NULL;
+}
+
 static int do_plain_rerere(struct string_list *rr, int fd)
 {
 	struct string_list conflict = STRING_LIST_INIT_DUP;
@@ -673,41 +711,8 @@ static int do_plain_rerere(struct string_list *rr, int fd)
 		}
 	}
 
-	/*
-	 * Some of the paths that had conflicts earlier might have
-	 * been resolved by the user.  Others may be similar to a
-	 * conflict already that was resolved before.
-	 */
-	for (i = 0; i < rr->nr; i++) {
-		int ret;
-		const char *path = rr->items[i].string;
-		const char *name = (const char *)rr->items[i].util;
-
-		/* Is there a recorded resolution we could attempt to apply? */
-		if (has_rerere_resolution(name)) {
-			if (merge(name, path))
-				continue;
-
-			if (rerere_autoupdate)
-				string_list_insert(&update, path);
-			else
-				fprintf(stderr,
-					"Resolved '%s' using previous resolution.\n",
-					path);
-			goto mark_resolved;
-		}
-
-		/* Let's see if the user has resolved it. */
-		ret = handle_file(path, NULL, NULL);
-		if (ret)
-			continue;
-
-		copy_file(rerere_path(name, "postimage"), path, 0666);
-		fprintf(stderr, "Recorded resolution for '%s'.\n", path);
-	mark_resolved:
-		free(rr->items[i].util);
-		rr->items[i].util = NULL;
-	}
+	for (i = 0; i < rr->nr; i++)
+		do_rerere_one_path(&rr->items[i], &update);
 
 	if (update.nr)
 		update_paths(&update);
-- 
2.5.0-rc0-209-g5e1f148

[PATCH v3 00/18] "rerere" preparatory clean-up

[Junio C Hamano]

This is a resend of v2:

    http://thread.gmane.org/gmane.comp.version-control.git/273117

plus 5 new changes.  I have a few more real-fix changes that build
on top, but they are not sufficiently polished to be published yet
compared to these early clean-up bits.

Junio C Hamano (18):
  rerere: fix an off-by-one non-bug
  rerere: plug conflict ID leaks
  rerere: lift PATH_MAX limitation
  rerere: write out each record of MERGE_RR in one go
  rerere: report autoupdated paths only after actually updating them
  rerere: drop want_sp parameter from is_cmarker()
  rerere: stop looping unnecessarily
  rerere: explain the rerere I/O abstraction
  rerere: explain MERGE_RR management helpers
  rerere: explain the primary codepath
  rerere: explain "rerere forget" codepath
  rerere: explain the remainder
  rerere: refactor "replay" part of do_plain_rerere()
  rerere: further de-dent do_plain_rerere()
  rerere: further clarify do_rerere_one_path()
  rerere: call conflict-ids IDs
  rerere: use "struct rerere_id" instead of "char *" for conflict ID
  rerere: un-nest merge() further

 builtin/rerere.c |   4 +-
 rerere.c         | 544 ++++++++++++++++++++++++++++++++++++++++---------------
 rerere.h         |  12 +-
 3 files changed, 407 insertions(+), 153 deletions(-)

-- 
2.5.0-rc2-340-g0cccc16

[PATCH v3 01/18] rerere: fix an off-by-one non-bug

[Junio C Hamano]

When ac49f5ca (rerere "remaining", 2011-02-16) split out a new
helper function check_one_conflict() out of find_conflict()
function, so that the latter will use the returned value from the
new helper to update the loop control variable that is an index into
active_cache[], the new variable incremented the index by one too
many when it found a path with only stage #1 entry at the very end
of active_cache[].

This "strange" return value does not have any effect on the loop
control of two callers of this function, as they all notice that
active_nr+2 is larger than active_nr just like active_nr+1 is, but
nevertheless it puzzles the readers when they are trying to figure
out what the function is trying to do.

In fact, there is no need to do an early return.  The code that
follows after skipping the stage #1 entry is fully prepared to
handle a case where the entry is at the very end of active_cache[].

Help future readers from unnecessary confusion by dropping an early
return.  We skip the stage #1 entry, and if there are stage #2 and
stage #3 entries for the same path, we diagnose the path as
THREE_STAGED (otherwise we say PUNTED), and then we skip all entries
for the same path.

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

diff --git a/rerere.c b/rerere.c
index 31644de..e307711 100644
--- a/rerere.c
+++ b/rerere.c
@@ -369,10 +369,8 @@ static int check_one_conflict(int i, int *type)
 	}
 
 	*type = PUNTED;
-	if (ce_stage(e) == 1) {
-		if (active_nr <= ++i)
-			return i + 1;
-	}
+	if (ce_stage(e) == 1)
+		i++;
 
 	/* Only handle regular files with both stages #2 and #3 */
 	if (i + 1 < active_nr) {
-- 
2.5.0-rc2-340-g0cccc16

[PATCH v3 02/18] rerere: plug conflict ID leaks

[Junio C Hamano]

The merge_rr string list stores the conflict ID (a hexadecimal
string that is used to index into $GIT_DIR/rr-cache) in the .util
field of its elements, and when do_plain_rerere() resolves a
conflict, the field is cleared.  Also, when rerere_forget()
recomputes the conflict ID to updates the preimage file, the
conflict ID for the path is updated.

We forgot to free the existing conflict ID when we did these two
operations.

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

diff --git a/rerere.c b/rerere.c
index e307711..3b9104d 100644
--- a/rerere.c
+++ b/rerere.c
@@ -559,6 +559,7 @@ static int do_plain_rerere(struct string_list *rr, int fd)
 		fprintf(stderr, "Recorded resolution for '%s'.\n", path);
 		copy_file(rerere_path(name, "postimage"), path, 0666);
 	mark_resolved:
+		free(rr->items[i].util);
 		rr->items[i].util = NULL;
 	}
 
@@ -627,6 +628,7 @@ static int rerere_forget_one_path(const char *path, struct string_list *rr)
 	char *hex;
 	unsigned char sha1[20];
 	int ret;
+	struct string_list_item *item;
 
 	ret = handle_cache(path, sha1, NULL);
 	if (ret < 1)
@@ -641,8 +643,9 @@ static int rerere_forget_one_path(const char *path, struct string_list *rr)
 	handle_cache(path, sha1, rerere_path(hex, "preimage"));
 	fprintf(stderr, "Updated preimage for '%s'\n", path);
 
-
-	string_list_insert(rr, path)->util = hex;
+	item = string_list_insert(rr, path);
+	free(item->util);
+	item->util = hex;
 	fprintf(stderr, "Forgot resolution for %s\n", path);
 	return 0;
 }
-- 
2.5.0-rc2-340-g0cccc16

[PATCH v3 03/18] rerere: lift PATH_MAX limitation

[Junio C Hamano]

The MERGE_RR file records a collection of NUL-terminated entries,
each of which consists of

 - a hash that identifies the conflict
 - a HT
 - the pathname

We used to read this piece-by-piece, and worse yet, read the
pathname part a byte at a time into a fixed buffer of size PATH_MAX.

Instead, read a whole entry using strbuf_getwholeline() and parse
out the fields.  This way, we issue fewer read(2) calls and more
importantly we do not have to limit the pathname to PATH_MAX.

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

diff --git a/rerere.c b/rerere.c
index 3b9104d..e854985 100644
--- a/rerere.c
+++ b/rerere.c
@@ -35,32 +35,27 @@ static int has_rerere_resolution(const char *hex)
 
 static void read_rr(struct string_list *rr)
 {
-	unsigned char sha1[20];
-	char buf[PATH_MAX];
+	struct strbuf buf = STRBUF_INIT;
 	FILE *in = fopen(merge_rr_path, "r");
+
 	if (!in)
 		return;
-	while (fread(buf, 40, 1, in) == 1) {
-		int i;
-		char *name;
-		if (get_sha1_hex(buf, sha1))
+	while (!strbuf_getwholeline(&buf, in, '\0')) {
+		char *path;
+		unsigned char sha1[20];
+
+		/* There has to be the hash, tab, path and then NUL */
+		if (buf.len < 42 || get_sha1_hex(buf.buf, sha1))
 			die("corrupt MERGE_RR");
-		buf[40] = '\0';
-		name = xstrdup(buf);
-		if (fgetc(in) != '\t')
+
+		if (buf.buf[40] != '\t')
 			die("corrupt MERGE_RR");
-		for (i = 0; i < sizeof(buf); i++) {
-			int c = fgetc(in);
-			if (c < 0)
-				die("corrupt MERGE_RR");
-			buf[i] = c;
-			if (c == 0)
-				 break;
-		}
-		if (i == sizeof(buf))
-			die("filename too long");
-		string_list_insert(rr, buf)->util = name;
+		buf.buf[40] = '\0';
+		path = buf.buf + 41;
+
+		string_list_insert(rr, path)->util = xstrdup(buf.buf);
 	}
+	strbuf_release(&buf);
 	fclose(in);
 }
 
-- 
2.5.0-rc2-340-g0cccc16

[PATCH v3 04/18] rerere: write out each record of MERGE_RR in one go

[Junio C Hamano]

Instead of writing the hash for a conflict, a HT, and the path
with three separate write_in_full() calls, format them into a
single record into a strbuf and write it out in one go.

As a more recent "rerere remaining" codepath abuses the .util field
of the merge_rr data to store a sentinel token, make sure that
codepath does not call into this function (of course, "remaining" is
a read-only operation and currently does not call it).

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

diff --git a/rerere.c b/rerere.c
index e854985..27b287d 100644
--- a/rerere.c
+++ b/rerere.c
@@ -65,16 +65,18 @@ static int write_rr(struct string_list *rr, int out_fd)
 {
 	int i;
 	for (i = 0; i < rr->nr; i++) {
-		const char *path;
-		int length;
+		struct strbuf buf = STRBUF_INIT;
+
+		assert(rr->items[i].util != RERERE_RESOLVED);
 		if (!rr->items[i].util)
 			continue;
-		path = rr->items[i].string;
-		length = strlen(path) + 1;
-		if (write_in_full(out_fd, rr->items[i].util, 40) != 40 ||
-		    write_str_in_full(out_fd, "\t") != 1 ||
-		    write_in_full(out_fd, path, length) != length)
+		strbuf_addf(&buf, "%s\t%s%c",
+			    (char *)rr->items[i].util,
+			    rr->items[i].string, 0);
+		if (write_in_full(out_fd, buf.buf, buf.len) != buf.len)
 			die("unable to write rerere record");
+
+		strbuf_release(&buf);
 	}
 	if (commit_lock_file(&write_lock) != 0)
 		die("unable to write rerere record");
-- 
2.5.0-rc2-340-g0cccc16

[PATCH v3 05/18] rerere: report autoupdated paths only after actually updating them

[Junio C Hamano]

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

diff --git a/rerere.c b/rerere.c
index 27b287d..304df02 100644
--- a/rerere.c
+++ b/rerere.c
@@ -482,6 +482,8 @@ static void update_paths(struct string_list *update)
 		struct string_list_item *item = &update->items[i];
 		if (add_file_to_cache(item->string, 0))
 			exit(128);
+		fprintf(stderr, "Staged '%s' using previous resolution.\n",
+			item->string);
 	}
 
 	if (active_cache_changed) {
@@ -536,16 +538,16 @@ static int do_plain_rerere(struct string_list *rr, int fd)
 		const char *name = (const char *)rr->items[i].util;
 
 		if (has_rerere_resolution(name)) {
-			if (!merge(name, path)) {
-				const char *msg;
-				if (rerere_autoupdate) {
-					string_list_insert(&update, path);
-					msg = "Staged '%s' using previous resolution.\n";
-				} else
-					msg = "Resolved '%s' using previous resolution.\n";
-				fprintf(stderr, msg, path);
-				goto mark_resolved;
-			}
+			if (merge(name, path))
+				continue;
+
+			if (rerere_autoupdate)
+				string_list_insert(&update, path);
+			else
+				fprintf(stderr,
+					"Resolved '%s' using previous resolution.\n",
+					path);
+			goto mark_resolved;
 		}
 
 		/* Let's see if we have resolved it. */
-- 
2.5.0-rc2-340-g0cccc16

[PATCH v3 06/18] rerere: drop want_sp parameter from is_cmarker()

[Junio C Hamano]

As the nature of the conflict marker line determies if there should
a SP and label after it, the caller shouldn't have to pass the
parameter redundantly.

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

diff --git a/rerere.c b/rerere.c
index 304df02..4c45f55 100644
--- a/rerere.c
+++ b/rerere.c
@@ -148,8 +148,25 @@ static int rerere_file_getline(struct strbuf *sb, struct rerere_io *io_)
 	return strbuf_getwholeline(sb, io->input, '\n');
 }
 
-static int is_cmarker(char *buf, int marker_char, int marker_size, int want_sp)
+/*
+ * Require the exact number of conflict marker letters, no more, no
+ * less, followed by SP or any whitespace
+ * (including LF).
+ */
+static int is_cmarker(char *buf, int marker_char, int marker_size)
 {
+	int want_sp;
+
+	/*
+	 * The beginning of our version and the end of their version
+	 * always are labeled like "<<<<< ours" or ">>>>> theirs",
+	 * hence we set want_sp for them.  Note that the version from
+	 * the common ancestor in diff3-style output is not always
+	 * labelled (e.g. "||||| common" is often seen but "|||||"
+	 * alone is also valid), so we do not set want_sp.
+	 */
+	want_sp = (marker_char == '<') || (marker_char == '>');
+
 	while (marker_size--)
 		if (*buf++ != marker_char)
 			return 0;
@@ -172,19 +189,19 @@ static int handle_path(unsigned char *sha1, struct rerere_io *io, int marker_siz
 		git_SHA1_Init(&ctx);
 
 	while (!io->getline(&buf, io)) {
-		if (is_cmarker(buf.buf, '<', marker_size, 1)) {
+		if (is_cmarker(buf.buf, '<', marker_size)) {
 			if (hunk != RR_CONTEXT)
 				goto bad;
 			hunk = RR_SIDE_1;
-		} else if (is_cmarker(buf.buf, '|', marker_size, 0)) {
+		} else if (is_cmarker(buf.buf, '|', marker_size)) {
 			if (hunk != RR_SIDE_1)
 				goto bad;
 			hunk = RR_ORIGINAL;
-		} else if (is_cmarker(buf.buf, '=', marker_size, 0)) {
+		} else if (is_cmarker(buf.buf, '=', marker_size)) {
 			if (hunk != RR_SIDE_1 && hunk != RR_ORIGINAL)
 				goto bad;
 			hunk = RR_SIDE_2;
-		} else if (is_cmarker(buf.buf, '>', marker_size, 1)) {
+		} else if (is_cmarker(buf.buf, '>', marker_size)) {
 			if (hunk != RR_SIDE_2)
 				goto bad;
 			if (strbuf_cmp(&one, &two) > 0)
-- 
2.5.0-rc2-340-g0cccc16

[PATCH v3 07/18] rerere: stop looping unnecessarily

[Junio C Hamano]

handle_cache() loops 3 times starting from an index entry that is
unmerged, while ignoring an entry for a path that is different from
what we are looking for.

As the index is sorted, once we see a different path, we know we saw
all stages for the path we are interested in.  Just loop while we
see the same path and then break, instead of continuing for 3 times.

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

diff --git a/rerere.c b/rerere.c
index 4c45f55..7b1419c 100644
--- a/rerere.c
+++ b/rerere.c
@@ -329,24 +329,21 @@ static int handle_cache(const char *path, unsigned char *sha1, const char *outpu
 		return -1;
 	pos = -pos - 1;
 
-	for (i = 0; i < 3; i++) {
+	while (pos < active_nr) {
 		enum object_type type;
 		unsigned long size;
-		int j;
 
-		if (active_nr <= pos)
-			break;
 		ce = active_cache[pos++];
 		if (ce_namelen(ce) != len || memcmp(ce->name, path, len))
-			continue;
-		j = ce_stage(ce) - 1;
-		mmfile[j].ptr = read_sha1_file(ce->sha1, &type, &size);
-		mmfile[j].size = size;
+			break;
+		i = ce_stage(ce) - 1;
+		mmfile[i].ptr = read_sha1_file(ce->sha1, &type, &size);
+		mmfile[i].size = size;
 	}
-	for (i = 0; i < 3; i++) {
+	for (i = 0; i < 3; i++)
 		if (!mmfile[i].ptr && !mmfile[i].size)
 			mmfile[i].ptr = xstrdup("");
-	}
+
 	/*
 	 * NEEDSWORK: handle conflicts from merges with
 	 * merge.renormalize set, too
-- 
2.5.0-rc2-340-g0cccc16

[PATCH v3 08/18] rerere: explain the rerere I/O abstraction

[Junio C Hamano]

Explain the internals of rerere as in-code comments.

This one covers our thin I/O abstraction to read from either
a file or a memory while optionally writing out to a file.

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

diff --git a/rerere.c b/rerere.c
index 7b1419c..7ed20f1 100644
--- a/rerere.c
+++ b/rerere.c
@@ -83,6 +83,21 @@ static int write_rr(struct string_list *rr, int out_fd)
 	return 0;
 }
 
+/*
+ * "rerere" interacts with conflicted file contents using this I/O
+ * abstraction.  It reads a conflicted contents from one place via
+ * "getline()" method, and optionally can write it out after
+ * normalizing the conflicted hunks to the "output".  Subclasses of
+ * rerere_io embed this structure at the beginning of their own
+ * rerere_io object.
+ */
+struct rerere_io {
+	int (*getline)(struct strbuf *, struct rerere_io *);
+	FILE *output;
+	int wrerror;
+	/* some more stuff */
+};
+
 static void ferr_write(const void *p, size_t count, FILE *fp, int *err)
 {
 	if (!count || *err)
@@ -96,19 +111,15 @@ static inline void ferr_puts(const char *s, FILE *fp, int *err)
 	ferr_write(s, strlen(s), fp, err);
 }
 
-struct rerere_io {
-	int (*getline)(struct strbuf *, struct rerere_io *);
-	FILE *output;
-	int wrerror;
-	/* some more stuff */
-};
-
 static void rerere_io_putstr(const char *str, struct rerere_io *io)
 {
 	if (io->output)
 		ferr_puts(str, io->output, &io->wrerror);
 }
 
+/*
+ * Write a conflict marker to io->output (if defined).
+ */
 static void rerere_io_putconflict(int ch, int size, struct rerere_io *io)
 {
 	char buf[64];
@@ -137,11 +148,17 @@ static void rerere_io_putmem(const char *mem, size_t sz, struct rerere_io *io)
 		ferr_write(mem, sz, io->output, &io->wrerror);
 }
 
+/*
+ * Subclass of rerere_io that reads from an on-disk file
+ */
 struct rerere_io_file {
 	struct rerere_io io;
 	FILE *input;
 };
 
+/*
+ * ... and its getline() method implementation
+ */
 static int rerere_file_getline(struct strbuf *sb, struct rerere_io *io_)
 {
 	struct rerere_io_file *io = (struct rerere_io_file *)io_;
@@ -286,11 +303,18 @@ static int handle_file(const char *path, unsigned char *sha1, const char *output
 	return hunk_no;
 }
 
+/*
+ * Subclass of rerere_io that reads from an in-core buffer that is a
+ * strbuf
+ */
 struct rerere_io_mem {
 	struct rerere_io io;
 	struct strbuf input;
 };
 
+/*
+ * ... and its getline() method implementation
+ */
 static int rerere_mem_getline(struct strbuf *sb, struct rerere_io *io_)
 {
 	struct rerere_io_mem *io = (struct rerere_io_mem *)io_;
-- 
2.5.0-rc2-340-g0cccc16

[PATCH v3 09/18] rerere: explain MERGE_RR management helpers

[Junio C Hamano]

Explain the internals of rerere as in-code comments, while
sprinkling "NEEDSWORK" comment to highlight iffy bits and
questionable assumptions.

This one covers the "$GIT_DIR/MERGE_RR" file and in-core merge_rr
that are used to keep track of the status of "rerere" session in
progress.

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

diff --git a/rerere.c b/rerere.c
index 7ed20f1..d54bdb2 100644
--- a/rerere.c
+++ b/rerere.c
@@ -33,6 +33,13 @@ static int has_rerere_resolution(const char *hex)
 	return !stat(rerere_path(hex, "postimage"), &st);
 }
 
+/*
+ * $GIT_DIR/MERGE_RR file is a collection of records, each of which is
+ * "conflict ID", a HT and pathname, terminated with a NUL, and is
+ * used to keep track of the set of paths that "rerere" may need to
+ * work on (i.e. what is left by the previous invocation of "git
+ * rerere" during the current conflict resolution session).
+ */
 static void read_rr(struct string_list *rr)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -394,6 +401,14 @@ static int handle_cache(const char *path, unsigned char *sha1, const char *outpu
 	return hunk_no;
 }
 
+/*
+ * 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.
+ * Return the cache index to be looked at next, by skipping the
+ * stages we have already looked at in this invocation of this
+ * function.
+ */
 static int check_one_conflict(int i, int *type)
 {
 	const struct cache_entry *e = active_cache[i];
@@ -425,6 +440,17 @@ static int check_one_conflict(int i, int *type)
 	return i;
 }
 
+/*
+ * Scan the index and find paths that have conflicts that rerere can
+ * handle, i.e. the ones that has both stages #2 and #3.
+ *
+ * NEEDSWORK: we do not record or replay a previous "resolve by
+ * deletion" for a delete-modify conflict, as that is inherently risky
+ * without knowing what modification is being discarded.  The only
+ * safe case, i.e. both side doing the deletion and modification that
+ * are identical to the previous round, might want to be handled,
+ * though.
+ */
 static int find_conflict(struct string_list *conflict)
 {
 	int i;
@@ -441,6 +467,21 @@ static int find_conflict(struct string_list *conflict)
 	return 0;
 }
 
+/*
+ * The merge_rr list is meant to hold outstanding conflicted paths
+ * that rerere could handle.  Abuse the list by adding other types of
+ * entries to allow the caller to show "rerere remaining".
+ *
+ * - Conflicted paths that rerere does not handle are added
+ * - Conflicted paths that have been resolved are marked as such
+ *   by storing RERERE_RESOLVED to .util field (where conflict ID
+ *   is expected to be stored).
+ *
+ * Do *not* write MERGE_RR file out after calling this function.
+ *
+ * NEEDSWORK: we may want to fix the caller that implements "rerere
+ * remaining" to do this without abusing merge_rr.
+ */
 int rerere_remaining(struct string_list *merge_rr)
 {
 	int i;
-- 
2.5.0-rc2-340-g0cccc16

[PATCH v3 10/18] rerere: explain the primary codepath

[Junio C Hamano]

Explain the internals of rerere as in-code comments, while
sprinkling "NEEDSWORK" comment to highlight iffy bits and
questionable assumptions.

This one covers the codepath reached from rerere(), the primary
interface to the subsystem.

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

diff --git a/rerere.c b/rerere.c
index d54bdb2..3d9c33b 100644
--- a/rerere.c
+++ b/rerere.c
@@ -199,6 +199,21 @@ static int is_cmarker(char *buf, int marker_char, int marker_size)
 	return isspace(*buf);
 }
 
+/*
+ * Read contents a file with conflicts, normalize the conflicts
+ * by (1) discarding the common ancestor version in diff3-style,
+ * (2) reordering our side and their side so that whichever sorts
+ * alphabetically earlier comes before the other one, while
+ * computing the "conflict ID", which is just an SHA-1 hash of
+ * one side of the conflict, NUL, the other side of the conflict,
+ * and NUL concatenated together.
+ *
+ * Return the number of conflict hunks found.
+ *
+ * NEEDSWORK: the logic and theory of operation behind this conflict
+ * normalization may deserve to be documented somewhere, perhaps in
+ * Documentation/technical/rerere.txt.
+ */
 static int handle_path(unsigned char *sha1, struct rerere_io *io, int marker_size)
 {
 	git_SHA_CTX ctx;
@@ -269,6 +284,10 @@ static int handle_path(unsigned char *sha1, struct rerere_io *io, int marker_siz
 	return hunk_no;
 }
 
+/*
+ * Scan the path for conflicts, do the "handle_path()" thing above, and
+ * return the number of conflict hunks found.
+ */
 static int handle_file(const char *path, unsigned char *sha1, const char *output)
 {
 	int hunk_no = 0;
@@ -506,29 +525,54 @@ int rerere_remaining(struct string_list *merge_rr)
 	return 0;
 }
 
+/*
+ * Find the conflict identified by "name"; the change between its
+ * "preimage" (i.e. a previous contents with conflict markers) and its
+ * "postimage" (i.e. the corresponding contents with conflicts
+ * resolved) may apply cleanly to the contents stored in "path", i.e.
+ * the conflict this time around.
+ *
+ * Returns 0 for successful replay of recorded resolution, or non-zero
+ * for failure.
+ */
 static int merge(const char *name, const char *path)
 {
 	int ret;
 	mmfile_t cur = {NULL, 0}, base = {NULL, 0}, other = {NULL, 0};
 	mmbuffer_t result = {NULL, 0};
 
+	/*
+	 * Normalize the conflicts in path and write it out to
+	 * "thisimage" temporary file.
+	 */
 	if (handle_file(path, NULL, rerere_path(name, "thisimage")) < 0)
 		return 1;
 
 	if (read_mmfile(&cur, rerere_path(name, "thisimage")) ||
-			read_mmfile(&base, rerere_path(name, "preimage")) ||
-			read_mmfile(&other, rerere_path(name, "postimage"))) {
+	    read_mmfile(&base, rerere_path(name, "preimage")) ||
+	    read_mmfile(&other, rerere_path(name, "postimage"))) {
 		ret = 1;
 		goto out;
 	}
+
+	/*
+	 * A three-way merge. Note that this honors user-customizable
+	 * low-level merge driver settings.
+	 */
 	ret = ll_merge(&result, path, &base, NULL, &cur, "", &other, "", NULL);
 	if (!ret) {
 		FILE *f;
 
+		/*
+		 * A successful replay of recorded resolution.
+		 * Mark that "postimage" was used to help gc.
+		 */
 		if (utime(rerere_path(name, "postimage"), NULL) < 0)
 			warning("failed utime() on %s: %s",
 					rerere_path(name, "postimage"),
 					strerror(errno));
+
+		/* Update "path" with the resolution */
 		f = fopen(path, "w");
 		if (!f)
 			return error("Could not open %s: %s", path,
@@ -581,41 +625,61 @@ static int do_plain_rerere(struct string_list *rr, int fd)
 	find_conflict(&conflict);
 
 	/*
-	 * MERGE_RR records paths with conflicts immediately after merge
-	 * failed.  Some of the conflicted paths might have been hand resolved
-	 * in the working tree since then, but the initial run would catch all
-	 * and register their preimages.
+	 * MERGE_RR records paths with conflicts immediately after
+	 * merge failed.  Some of the conflicted paths might have been
+	 * hand resolved in the working tree since then, but the
+	 * initial run would catch all and register their preimages.
 	 */
-
 	for (i = 0; i < conflict.nr; i++) {
 		const char *path = conflict.items[i].string;
 		if (!string_list_has_string(rr, path)) {
 			unsigned char sha1[20];
 			char *hex;
 			int ret;
+
+			/*
+			 * Ask handle_file() to scan and assign a
+			 * conflict ID.  No need to write anything out
+			 * yet.
+			 */
 			ret = handle_file(path, sha1, NULL);
 			if (ret < 1)
 				continue;
 			hex = xstrdup(sha1_to_hex(sha1));
 			string_list_insert(rr, path)->util = hex;
+
+			/*
+			 * If the directory does not exist, create
+			 * it.  mkdir_in_gitdir() will fail with
+			 * EEXIST if there already is one.
+			 *
+			 * NEEDSWORK: make sure "gc" does not remove
+			 * preimage without removing the directory.
+			 */
 			if (mkdir_in_gitdir(git_path("rr-cache/%s", hex)))
 				continue;
+
+			/*
+			 * We are the first to encounter this
+			 * conflict.  Ask handle_file() to write the
+			 * normalized contents to the "preimage" file.
+			 */
 			handle_file(path, NULL, rerere_path(hex, "preimage"));
 			fprintf(stderr, "Recorded preimage for '%s'\n", path);
 		}
 	}
 
 	/*
-	 * Now some of the paths that had conflicts earlier might have been
-	 * hand resolved.  Others may be similar to a conflict already that
-	 * was resolved before.
+	 * Some of the paths that had conflicts earlier might have
+	 * been resolved by the user.  Others may be similar to a
+	 * conflict already that was resolved before.
 	 */
-
 	for (i = 0; i < rr->nr; i++) {
 		int ret;
 		const char *path = rr->items[i].string;
 		const char *name = (const char *)rr->items[i].util;
 
+		/* Is there a recorded resolution we could attempt to apply? */
 		if (has_rerere_resolution(name)) {
 			if (merge(name, path))
 				continue;
@@ -629,13 +693,13 @@ static int do_plain_rerere(struct string_list *rr, int fd)
 			goto mark_resolved;
 		}
 
-		/* Let's see if we have resolved it. */
+		/* Let's see if the user has resolved it. */
 		ret = handle_file(path, NULL, NULL);
 		if (ret)
 			continue;
 
-		fprintf(stderr, "Recorded resolution for '%s'.\n", path);
 		copy_file(rerere_path(name, "postimage"), path, 0666);
+		fprintf(stderr, "Recorded resolution for '%s'.\n", path);
 	mark_resolved:
 		free(rr->items[i].util);
 		rr->items[i].util = NULL;
@@ -689,6 +753,11 @@ int setup_rerere(struct string_list *merge_rr, int flags)
 	return fd;
 }
 
+/*
+ * The main entry point that is called internally from codepaths that
+ * perform mergy operations, possibly leaving conflicted index entries
+ * and working tree files.
+ */
 int rerere(int flags)
 {
 	struct string_list merge_rr = STRING_LIST_INIT_DUP;
-- 
2.5.0-rc2-340-g0cccc16

[PATCH v3 11/18] rerere: explain "rerere forget" codepath

[Junio C Hamano]

Explain the internals of rerere as in-code comments, while
sprinkling "NEEDSWORK" comment to highlight iffy bits and
questionable assumptions.

This covers the codepath that implements "rerere forget".

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

diff --git a/rerere.c b/rerere.c
index 3d9c33b..3782be6 100644
--- a/rerere.c
+++ b/rerere.c
@@ -413,6 +413,10 @@ static int handle_cache(const char *path, unsigned char *sha1, const char *outpu
 	strbuf_init(&io.input, 0);
 	strbuf_attach(&io.input, result.ptr, result.size, result.size);
 
+	/*
+	 * Grab the conflict ID and optionally write the original
+	 * contents with conflict markers out.
+	 */
 	hunk_no = handle_path(sha1, (struct rerere_io *)&io, marker_size);
 	strbuf_release(&io.input);
 	if (io.io.output)
@@ -777,9 +781,15 @@ static int rerere_forget_one_path(const char *path, struct string_list *rr)
 	int ret;
 	struct string_list_item *item;
 
+	/*
+	 * Recreate the original conflict from the stages in the
+	 * index and compute the conflict ID
+	 */
 	ret = handle_cache(path, sha1, NULL);
 	if (ret < 1)
 		return error("Could not parse conflict hunks in '%s'", path);
+
+	/* Nuke the recorded resolution for the conflict */
 	hex = xstrdup(sha1_to_hex(sha1));
 	filename = rerere_path(hex, "postimage");
 	if (unlink(filename))
@@ -787,9 +797,18 @@ static int rerere_forget_one_path(const char *path, struct string_list *rr)
 			? error("no remembered resolution for %s", path)
 			: error("cannot unlink %s: %s", filename, strerror(errno)));
 
+	/*
+	 * Update the preimage so that the user can resolve the
+	 * conflict in the working tree, run us again to record
+	 * the postimage.
+	 */
 	handle_cache(path, sha1, rerere_path(hex, "preimage"));
 	fprintf(stderr, "Updated preimage for '%s'\n", path);
 
+	/*
+	 * And remember that we can record resolution for this
+	 * conflict when the user is done.
+	 */
 	item = string_list_insert(rr, path);
 	free(item->util);
 	item->util = hex;
@@ -808,6 +827,11 @@ int rerere_forget(struct pathspec *pathspec)
 
 	fd = setup_rerere(&merge_rr, RERERE_NOAUTOUPDATE);
 
+	/*
+	 * The paths may have been resolved (incorrectly);
+	 * recover the original conflicted state and then
+	 * find the conflicted paths.
+	 */
 	unmerge_cache(pathspec);
 	find_conflict(&conflict);
 	for (i = 0; i < conflict.nr; i++) {
-- 
2.5.0-rc2-340-g0cccc16

[PATCH v3 12/18] rerere: explain the remainder

[Junio C Hamano]

Explain the internals of rerere as in-code comments, while
sprinkling "NEEDSWORK" comment to highlight iffy bits and
questionable assumptions.

This covers the codepath that implements "rerere gc" and "rerere
clear".

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

diff --git a/rerere.c b/rerere.c
index 3782be6..7ef951e 100644
--- a/rerere.c
+++ b/rerere.c
@@ -844,6 +844,9 @@ int rerere_forget(struct pathspec *pathspec)
 	return write_rr(&merge_rr, fd);
 }
 
+/*
+ * Garbage collection support
+ */
 static time_t rerere_created_at(const char *name)
 {
 	struct stat st;
@@ -856,11 +859,19 @@ static time_t rerere_last_used_at(const char *name)
 	return stat(rerere_path(name, "postimage"), &st) ? (time_t) 0 : st.st_mtime;
 }
 
+/*
+ * Remove the recorded resolution for a given conflict ID
+ */
 static void unlink_rr_item(const char *name)
 {
 	unlink(rerere_path(name, "thisimage"));
 	unlink(rerere_path(name, "preimage"));
 	unlink(rerere_path(name, "postimage"));
+	/*
+	 * NEEDSWORK: what if this rmdir() fails?  Wouldn't we then
+	 * assume that we already have preimage recorded in
+	 * do_plain_rerere()?
+	 */
 	rmdir(git_path("rr-cache/%s", name));
 }
 
@@ -880,6 +891,7 @@ void rerere_gc(struct string_list *rr)
 	dir = opendir(git_path("rr-cache"));
 	if (!dir)
 		die_errno("unable to open rr-cache directory");
+	/* Collect stale conflict IDs ... */
 	while ((e = readdir(dir))) {
 		if (is_dot_or_dotdot(e->d_name))
 			continue;
@@ -897,11 +909,19 @@ void rerere_gc(struct string_list *rr)
 			string_list_append(&to_remove, e->d_name);
 	}
 	closedir(dir);
+	/* ... and then remove them one-by-one */
 	for (i = 0; i < to_remove.nr; i++)
 		unlink_rr_item(to_remove.items[i].string);
 	string_list_clear(&to_remove, 0);
 }
 
+/*
+ * During a conflict resolution, after "rerere" recorded the
+ * preimages, abandon them if the user did not resolve them or
+ * record their resolutions.  And drop $GIT_DIR/MERGE_RR.
+ *
+ * NEEDSWORK: shouldn't we be calling this from "reset --hard"?
+ */
 void rerere_clear(struct string_list *merge_rr)
 {
 	int i;
-- 
2.5.0-rc2-340-g0cccc16

[PATCH v3 13/18] rerere: refactor "replay" part of do_plain_rerere()

[Junio C Hamano]

Extract the body of a loop that attempts to replay recorded
resolution for each conflicted path into a helper function, not
because I want to call it from multiple places later, but because
the logic has become too deeply nested and hard to read.

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

diff --git a/rerere.c b/rerere.c
index 7ef951e..09b72ed 100644
--- a/rerere.c
+++ b/rerere.c
@@ -620,6 +620,44 @@ static void update_paths(struct string_list *update)
 		rollback_lock_file(&index_lock);
 }
 
+/*
+ * The path indicated by rr_item may still have conflict for which we
+ * have a recorded resolution, in which case replay it and optionally
+ * update it.  Or it may have been resolved by the user and we may
+ * only have the preimage for that conflict, in which case the result
+ * needs to be recorded as a resolution in a postimage file.
+ */
+static void do_rerere_one_path(struct string_list_item *rr_item,
+			       struct string_list *update)
+{
+	const char *path = rr_item->string;
+	const char *name = (const char *)rr_item->util;
+
+	/* Is there a recorded resolution we could attempt to apply? */
+	if (has_rerere_resolution(name)) {
+		if (merge(name, path))
+			return; /* failed to replay */
+
+		if (rerere_autoupdate)
+			string_list_insert(update, path);
+		else
+			fprintf(stderr,
+				"Resolved '%s' using previous resolution.\n",
+				path);
+		goto mark_resolved;
+	}
+
+	/* Let's see if the user has resolved it. */
+	if (handle_file(path, NULL, NULL))
+		return; /* not yet resolved */
+
+	copy_file(rerere_path(name, "postimage"), path, 0666);
+	fprintf(stderr, "Recorded resolution for '%s'.\n", path);
+mark_resolved:
+	free(rr_item->util);
+	rr_item->util = NULL;
+}
+
 static int do_plain_rerere(struct string_list *rr, int fd)
 {
 	struct string_list conflict = STRING_LIST_INIT_DUP;
@@ -673,41 +711,8 @@ static int do_plain_rerere(struct string_list *rr, int fd)
 		}
 	}
 
-	/*
-	 * Some of the paths that had conflicts earlier might have
-	 * been resolved by the user.  Others may be similar to a
-	 * conflict already that was resolved before.
-	 */
-	for (i = 0; i < rr->nr; i++) {
-		int ret;
-		const char *path = rr->items[i].string;
-		const char *name = (const char *)rr->items[i].util;
-
-		/* Is there a recorded resolution we could attempt to apply? */
-		if (has_rerere_resolution(name)) {
-			if (merge(name, path))
-				continue;
-
-			if (rerere_autoupdate)
-				string_list_insert(&update, path);
-			else
-				fprintf(stderr,
-					"Resolved '%s' using previous resolution.\n",
-					path);
-			goto mark_resolved;
-		}
-
-		/* Let's see if the user has resolved it. */
-		ret = handle_file(path, NULL, NULL);
-		if (ret)
-			continue;
-
-		copy_file(rerere_path(name, "postimage"), path, 0666);
-		fprintf(stderr, "Recorded resolution for '%s'.\n", path);
-	mark_resolved:
-		free(rr->items[i].util);
-		rr->items[i].util = NULL;
-	}
+	for (i = 0; i < rr->nr; i++)
+		do_rerere_one_path(&rr->items[i], &update);
 
 	if (update.nr)
 		update_paths(&update);
-- 
2.5.0-rc2-340-g0cccc16

[PATCH v3 14/18] rerere: further de-dent do_plain_rerere()

[Junio C Hamano]

It's just easier to follow this way.

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

diff --git a/rerere.c b/rerere.c
index 09b72ed..1089a9c 100644
--- a/rerere.c
+++ b/rerere.c
@@ -673,42 +673,43 @@ static int do_plain_rerere(struct string_list *rr, int fd)
 	 * initial run would catch all and register their preimages.
 	 */
 	for (i = 0; i < conflict.nr; i++) {
+		unsigned char sha1[20];
+		char *hex;
+		int ret;
 		const char *path = conflict.items[i].string;
-		if (!string_list_has_string(rr, path)) {
-			unsigned char sha1[20];
-			char *hex;
-			int ret;
-
-			/*
-			 * Ask handle_file() to scan and assign a
-			 * conflict ID.  No need to write anything out
-			 * yet.
-			 */
-			ret = handle_file(path, sha1, NULL);
-			if (ret < 1)
-				continue;
-			hex = xstrdup(sha1_to_hex(sha1));
-			string_list_insert(rr, path)->util = hex;
-
-			/*
-			 * If the directory does not exist, create
-			 * it.  mkdir_in_gitdir() will fail with
-			 * EEXIST if there already is one.
-			 *
-			 * NEEDSWORK: make sure "gc" does not remove
-			 * preimage without removing the directory.
-			 */
-			if (mkdir_in_gitdir(git_path("rr-cache/%s", hex)))
-				continue;
 
-			/*
-			 * We are the first to encounter this
-			 * conflict.  Ask handle_file() to write the
-			 * normalized contents to the "preimage" file.
-			 */
-			handle_file(path, NULL, rerere_path(hex, "preimage"));
-			fprintf(stderr, "Recorded preimage for '%s'\n", path);
-		}
+		if (string_list_has_string(rr, path))
+			continue;
+
+		/*
+		 * Ask handle_file() to scan and assign a
+		 * conflict ID.  No need to write anything out
+		 * yet.
+		 */
+		ret = handle_file(path, sha1, NULL);
+		if (ret < 1)
+			continue;
+		hex = xstrdup(sha1_to_hex(sha1));
+		string_list_insert(rr, path)->util = hex;
+
+		/*
+		 * If the directory does not exist, create
+		 * it.  mkdir_in_gitdir() will fail with
+		 * EEXIST if there already is one.
+		 *
+		 * NEEDSWORK: make sure "gc" does not remove
+		 * preimage without removing the directory.
+		 */
+		if (mkdir_in_gitdir(git_path("rr-cache/%s", hex)))
+			continue;
+
+		/*
+		 * We are the first to encounter this
+		 * conflict.  Ask handle_file() to write the
+		 * normalized contents to the "preimage" file.
+		 */
+		handle_file(path, NULL, rerere_path(hex, "preimage"));
+		fprintf(stderr, "Recorded preimage for '%s'\n", path);
 	}
 
 	for (i = 0; i < rr->nr; i++)
-- 
2.5.0-rc2-340-g0cccc16

[PATCH v3 15/18] rerere: further clarify do_rerere_one_path()

[Junio C Hamano]

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

diff --git a/rerere.c b/rerere.c
index 1089a9c..30bdfeb 100644
--- a/rerere.c
+++ b/rerere.c
@@ -644,16 +644,13 @@ static void do_rerere_one_path(struct string_list_item *rr_item,
 			fprintf(stderr,
 				"Resolved '%s' using previous resolution.\n",
 				path);
-		goto mark_resolved;
+	} else if (!handle_file(path, NULL, NULL)) {
+		/* The user has resolved it. */
+		copy_file(rerere_path(name, "postimage"), path, 0666);
+		fprintf(stderr, "Recorded resolution for '%s'.\n", path);
+	} else {
+		return;
 	}
-
-	/* Let's see if the user has resolved it. */
-	if (handle_file(path, NULL, NULL))
-		return; /* not yet resolved */
-
-	copy_file(rerere_path(name, "postimage"), path, 0666);
-	fprintf(stderr, "Recorded resolution for '%s'.\n", path);
-mark_resolved:
 	free(rr_item->util);
 	rr_item->util = NULL;
 }
-- 
2.5.0-rc2-340-g0cccc16

[PATCH v3 16/18] rerere: call conflict-ids IDs

[Junio C Hamano]

Most places we call conflict IDs "name" and some others we call them
"hex"; update all of them to "id".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/rerere.c |  4 +--
 rerere.c         | 76 ++++++++++++++++++++++++++++----------------------------
 rerere.h         |  2 +-
 3 files changed, 41 insertions(+), 41 deletions(-)

diff --git a/builtin/rerere.c b/builtin/rerere.c
index 98eb8c5..81730bb 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c
@@ -103,8 +103,8 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
 	} else if (!strcmp(argv[0], "diff"))
 		for (i = 0; i < merge_rr.nr; i++) {
 			const char *path = merge_rr.items[i].string;
-			const char *name = (const char *)merge_rr.items[i].util;
-			diff_two(rerere_path(name, "preimage"), path, path, path);
+			const char *id = (const char *)merge_rr.items[i].util;
+			diff_two(rerere_path(id, "preimage"), path, path, path);
 		}
 	else
 		usage_with_options(rerere_usage, options);
diff --git a/rerere.c b/rerere.c
index 30bdfeb..e68469a 100644
--- a/rerere.c
+++ b/rerere.c
@@ -22,15 +22,15 @@ static int rerere_autoupdate;
 
 static char *merge_rr_path;
 
-const char *rerere_path(const char *hex, const char *file)
+const char *rerere_path(const char *id, const char *file)
 {
-	return git_path("rr-cache/%s/%s", hex, file);
+	return git_path("rr-cache/%s/%s", id, file);
 }
 
-static int has_rerere_resolution(const char *hex)
+static int has_rerere_resolution(const char *id)
 {
 	struct stat st;
-	return !stat(rerere_path(hex, "postimage"), &st);
+	return !stat(rerere_path(id, "postimage"), &st);
 }
 
 /*
@@ -530,7 +530,7 @@ int rerere_remaining(struct string_list *merge_rr)
 }
 
 /*
- * Find the conflict identified by "name"; the change between its
+ * Find the conflict identified by "id"; the change between its
  * "preimage" (i.e. a previous contents with conflict markers) and its
  * "postimage" (i.e. the corresponding contents with conflicts
  * resolved) may apply cleanly to the contents stored in "path", i.e.
@@ -539,7 +539,7 @@ int rerere_remaining(struct string_list *merge_rr)
  * Returns 0 for successful replay of recorded resolution, or non-zero
  * for failure.
  */
-static int merge(const char *name, const char *path)
+static int merge(const char *id, const char *path)
 {
 	int ret;
 	mmfile_t cur = {NULL, 0}, base = {NULL, 0}, other = {NULL, 0};
@@ -549,12 +549,12 @@ static int merge(const char *name, const char *path)
 	 * Normalize the conflicts in path and write it out to
 	 * "thisimage" temporary file.
 	 */
-	if (handle_file(path, NULL, rerere_path(name, "thisimage")) < 0)
+	if (handle_file(path, NULL, rerere_path(id, "thisimage")) < 0)
 		return 1;
 
-	if (read_mmfile(&cur, rerere_path(name, "thisimage")) ||
-	    read_mmfile(&base, rerere_path(name, "preimage")) ||
-	    read_mmfile(&other, rerere_path(name, "postimage"))) {
+	if (read_mmfile(&cur, rerere_path(id, "thisimage")) ||
+	    read_mmfile(&base, rerere_path(id, "preimage")) ||
+	    read_mmfile(&other, rerere_path(id, "postimage"))) {
 		ret = 1;
 		goto out;
 	}
@@ -571,9 +571,9 @@ static int merge(const char *name, const char *path)
 		 * A successful replay of recorded resolution.
 		 * Mark that "postimage" was used to help gc.
 		 */
-		if (utime(rerere_path(name, "postimage"), NULL) < 0)
+		if (utime(rerere_path(id, "postimage"), NULL) < 0)
 			warning("failed utime() on %s: %s",
-					rerere_path(name, "postimage"),
+					rerere_path(id, "postimage"),
 					strerror(errno));
 
 		/* Update "path" with the resolution */
@@ -631,11 +631,11 @@ static void do_rerere_one_path(struct string_list_item *rr_item,
 			       struct string_list *update)
 {
 	const char *path = rr_item->string;
-	const char *name = (const char *)rr_item->util;
+	const char *id = (const char *)rr_item->util;
 
 	/* Is there a recorded resolution we could attempt to apply? */
-	if (has_rerere_resolution(name)) {
-		if (merge(name, path))
+	if (has_rerere_resolution(id)) {
+		if (merge(id, path))
 			return; /* failed to replay */
 
 		if (rerere_autoupdate)
@@ -646,7 +646,7 @@ static void do_rerere_one_path(struct string_list_item *rr_item,
 				path);
 	} else if (!handle_file(path, NULL, NULL)) {
 		/* The user has resolved it. */
-		copy_file(rerere_path(name, "postimage"), path, 0666);
+		copy_file(rerere_path(id, "postimage"), path, 0666);
 		fprintf(stderr, "Recorded resolution for '%s'.\n", path);
 	} else {
 		return;
@@ -671,7 +671,7 @@ static int do_plain_rerere(struct string_list *rr, int fd)
 	 */
 	for (i = 0; i < conflict.nr; i++) {
 		unsigned char sha1[20];
-		char *hex;
+		char *id;
 		int ret;
 		const char *path = conflict.items[i].string;
 
@@ -686,8 +686,8 @@ static int do_plain_rerere(struct string_list *rr, int fd)
 		ret = handle_file(path, sha1, NULL);
 		if (ret < 1)
 			continue;
-		hex = xstrdup(sha1_to_hex(sha1));
-		string_list_insert(rr, path)->util = hex;
+		id = xstrdup(sha1_to_hex(sha1));
+		string_list_insert(rr, path)->util = id;
 
 		/*
 		 * If the directory does not exist, create
@@ -697,7 +697,7 @@ static int do_plain_rerere(struct string_list *rr, int fd)
 		 * NEEDSWORK: make sure "gc" does not remove
 		 * preimage without removing the directory.
 		 */
-		if (mkdir_in_gitdir(git_path("rr-cache/%s", hex)))
+		if (mkdir_in_gitdir(git_path("rr-cache/%s", id)))
 			continue;
 
 		/*
@@ -705,7 +705,7 @@ static int do_plain_rerere(struct string_list *rr, int fd)
 		 * conflict.  Ask handle_file() to write the
 		 * normalized contents to the "preimage" file.
 		 */
-		handle_file(path, NULL, rerere_path(hex, "preimage"));
+		handle_file(path, NULL, rerere_path(id, "preimage"));
 		fprintf(stderr, "Recorded preimage for '%s'\n", path);
 	}
 
@@ -779,7 +779,7 @@ int rerere(int flags)
 static int rerere_forget_one_path(const char *path, struct string_list *rr)
 {
 	const char *filename;
-	char *hex;
+	char *id;
 	unsigned char sha1[20];
 	int ret;
 	struct string_list_item *item;
@@ -793,8 +793,8 @@ static int rerere_forget_one_path(const char *path, struct string_list *rr)
 		return error("Could not parse conflict hunks in '%s'", path);
 
 	/* Nuke the recorded resolution for the conflict */
-	hex = xstrdup(sha1_to_hex(sha1));
-	filename = rerere_path(hex, "postimage");
+	id = xstrdup(sha1_to_hex(sha1));
+	filename = rerere_path(id, "postimage");
 	if (unlink(filename))
 		return (errno == ENOENT
 			? error("no remembered resolution for %s", path)
@@ -805,7 +805,7 @@ static int rerere_forget_one_path(const char *path, struct string_list *rr)
 	 * conflict in the working tree, run us again to record
 	 * the postimage.
 	 */
-	handle_cache(path, sha1, rerere_path(hex, "preimage"));
+	handle_cache(path, sha1, rerere_path(id, "preimage"));
 	fprintf(stderr, "Updated preimage for '%s'\n", path);
 
 	/*
@@ -814,7 +814,7 @@ static int rerere_forget_one_path(const char *path, struct string_list *rr)
 	 */
 	item = string_list_insert(rr, path);
 	free(item->util);
-	item->util = hex;
+	item->util = id;
 	fprintf(stderr, "Forgot resolution for %s\n", path);
 	return 0;
 }
@@ -850,32 +850,32 @@ int rerere_forget(struct pathspec *pathspec)
 /*
  * Garbage collection support
  */
-static time_t rerere_created_at(const char *name)
+static time_t rerere_created_at(const char *id)
 {
 	struct stat st;
-	return stat(rerere_path(name, "preimage"), &st) ? (time_t) 0 : st.st_mtime;
+	return stat(rerere_path(id, "preimage"), &st) ? (time_t) 0 : st.st_mtime;
 }
 
-static time_t rerere_last_used_at(const char *name)
+static time_t rerere_last_used_at(const char *id)
 {
 	struct stat st;
-	return stat(rerere_path(name, "postimage"), &st) ? (time_t) 0 : st.st_mtime;
+	return stat(rerere_path(id, "postimage"), &st) ? (time_t) 0 : st.st_mtime;
 }
 
 /*
  * Remove the recorded resolution for a given conflict ID
  */
-static void unlink_rr_item(const char *name)
+static void unlink_rr_item(const char *id)
 {
-	unlink(rerere_path(name, "thisimage"));
-	unlink(rerere_path(name, "preimage"));
-	unlink(rerere_path(name, "postimage"));
+	unlink(rerere_path(id, "thisimage"));
+	unlink(rerere_path(id, "preimage"));
+	unlink(rerere_path(id, "postimage"));
 	/*
 	 * NEEDSWORK: what if this rmdir() fails?  Wouldn't we then
 	 * assume that we already have preimage recorded in
 	 * do_plain_rerere()?
 	 */
-	rmdir(git_path("rr-cache/%s", name));
+	rmdir(git_path("rr-cache/%s", id));
 }
 
 void rerere_gc(struct string_list *rr)
@@ -930,9 +930,9 @@ void rerere_clear(struct string_list *merge_rr)
 	int i;
 
 	for (i = 0; i < merge_rr->nr; i++) {
-		const char *name = (const char *)merge_rr->items[i].util;
-		if (!has_rerere_resolution(name))
-			unlink_rr_item(name);
+		const char *id = (const char *)merge_rr->items[i].util;
+		if (!has_rerere_resolution(id))
+			unlink_rr_item(id);
 	}
 	unlink_or_warn(git_path("MERGE_RR"));
 }
diff --git a/rerere.h b/rerere.h
index 2956c2e..f998eba 100644
--- a/rerere.h
+++ b/rerere.h
@@ -17,7 +17,7 @@ extern void *RERERE_RESOLVED;
 
 extern int setup_rerere(struct string_list *, int);
 extern int rerere(int);
-extern const char *rerere_path(const char *hex, const char *file);
+extern const char *rerere_path(const char *id, const char *file);
 extern int rerere_forget(struct pathspec *);
 extern int rerere_remaining(struct string_list *);
 extern void rerere_clear(struct string_list *);
-- 
2.5.0-rc2-340-g0cccc16

[PATCH v3 17/18] rerere: use "struct rerere_id" instead of "char *" for conflict ID

[Junio C Hamano]

This gives a thin abstraction between the conflict ID that is a hash
value obtained by inspecting the conflicts and the name of the
directory under $GIT_DIR/rr-cache/, in which the previous resolution
is recorded to be replayed.  The plan is to make sure that the
presense of the directory does not imply the presense of a previous
resolution and vice-versa, and later allow us to have more than one
pair of <preimage, postimage> for a given conflict ID.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/rerere.c |  2 +-
 rerere.c         | 99 +++++++++++++++++++++++++++++++++++++++++---------------
 rerere.h         | 12 ++++++-
 3 files changed, 85 insertions(+), 28 deletions(-)

diff --git a/builtin/rerere.c b/builtin/rerere.c
index 81730bb..fd229a7 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c
@@ -103,7 +103,7 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
 	} else if (!strcmp(argv[0], "diff"))
 		for (i = 0; i < merge_rr.nr; i++) {
 			const char *path = merge_rr.items[i].string;
-			const char *id = (const char *)merge_rr.items[i].util;
+			const struct rerere_id *id = merge_rr.items[i].util;
 			diff_two(rerere_path(id, "preimage"), path, path, path);
 		}
 	else
diff --git a/rerere.c b/rerere.c
index e68469a..469d8a8 100644
--- a/rerere.c
+++ b/rerere.c
@@ -22,17 +22,43 @@ static int rerere_autoupdate;
 
 static char *merge_rr_path;
 
-const char *rerere_path(const char *id, const char *file)
+static void free_rerere_id(struct string_list_item *item)
 {
-	return git_path("rr-cache/%s/%s", id, file);
+	free(item->util);
+}
+
+static const char *rerere_id_hex(const struct rerere_id *id)
+{
+	return id->hex;
+}
+
+const char *rerere_path(const struct rerere_id *id, const char *file)
+{
+	if (file)
+		return git_path("rr-cache/%s/%s", rerere_id_hex(id), file);
+	else
+		return git_path("rr-cache/%s", rerere_id_hex(id));
 }
 
-static int has_rerere_resolution(const char *id)
+static int has_rerere_resolution(const struct rerere_id *id)
 {
 	struct stat st;
+
 	return !stat(rerere_path(id, "postimage"), &st);
 }
 
+static struct rerere_id *new_rerere_id_hex(char *hex)
+{
+	struct rerere_id *id = xmalloc(sizeof(*id));
+	strcpy(id->hex, hex);
+	return id;
+}
+
+static struct rerere_id *new_rerere_id(unsigned char *sha1)
+{
+	return new_rerere_id_hex(sha1_to_hex(sha1));
+}
+
 /*
  * $GIT_DIR/MERGE_RR file is a collection of records, each of which is
  * "conflict ID", a HT and pathname, terminated with a NUL, and is
@@ -50,6 +76,7 @@ static void read_rr(struct string_list *rr)
 	while (!strbuf_getwholeline(&buf, in, '\0')) {
 		char *path;
 		unsigned char sha1[20];
+		struct rerere_id *id;
 
 		/* There has to be the hash, tab, path and then NUL */
 		if (buf.len < 42 || get_sha1_hex(buf.buf, sha1))
@@ -59,8 +86,8 @@ static void read_rr(struct string_list *rr)
 			die("corrupt MERGE_RR");
 		buf.buf[40] = '\0';
 		path = buf.buf + 41;
-
-		string_list_insert(rr, path)->util = xstrdup(buf.buf);
+		id = new_rerere_id_hex(buf.buf);
+		string_list_insert(rr, path)->util = id;
 	}
 	strbuf_release(&buf);
 	fclose(in);
@@ -73,12 +100,15 @@ static int write_rr(struct string_list *rr, int out_fd)
 	int i;
 	for (i = 0; i < rr->nr; i++) {
 		struct strbuf buf = STRBUF_INIT;
+		struct rerere_id *id;
 
 		assert(rr->items[i].util != RERERE_RESOLVED);
-		if (!rr->items[i].util)
+
+		id = rr->items[i].util;
+		if (!id)
 			continue;
 		strbuf_addf(&buf, "%s\t%s%c",
-			    (char *)rr->items[i].util,
+			    rerere_id_hex(id),
 			    rr->items[i].string, 0);
 		if (write_in_full(out_fd, buf.buf, buf.len) != buf.len)
 			die("unable to write rerere record");
@@ -521,7 +551,7 @@ int rerere_remaining(struct string_list *merge_rr)
 			struct string_list_item *it;
 			it = string_list_lookup(merge_rr, (const char *)e->name);
 			if (it != NULL) {
-				free(it->util);
+				free_rerere_id(it);
 				it->util = RERERE_RESOLVED;
 			}
 		}
@@ -539,7 +569,7 @@ int rerere_remaining(struct string_list *merge_rr)
  * Returns 0 for successful replay of recorded resolution, or non-zero
  * for failure.
  */
-static int merge(const char *id, const char *path)
+static int merge(const struct rerere_id *id, const char *path)
 {
 	int ret;
 	mmfile_t cur = {NULL, 0}, base = {NULL, 0}, other = {NULL, 0};
@@ -573,8 +603,8 @@ static int merge(const char *id, const char *path)
 		 */
 		if (utime(rerere_path(id, "postimage"), NULL) < 0)
 			warning("failed utime() on %s: %s",
-					rerere_path(id, "postimage"),
-					strerror(errno));
+				rerere_path(id, "postimage"),
+				strerror(errno));
 
 		/* Update "path" with the resolution */
 		f = fopen(path, "w");
@@ -631,7 +661,7 @@ static void do_rerere_one_path(struct string_list_item *rr_item,
 			       struct string_list *update)
 {
 	const char *path = rr_item->string;
-	const char *id = (const char *)rr_item->util;
+	const struct rerere_id *id = rr_item->util;
 
 	/* Is there a recorded resolution we could attempt to apply? */
 	if (has_rerere_resolution(id)) {
@@ -651,7 +681,7 @@ static void do_rerere_one_path(struct string_list_item *rr_item,
 	} else {
 		return;
 	}
-	free(rr_item->util);
+	free_rerere_id(rr_item);
 	rr_item->util = NULL;
 }
 
@@ -670,10 +700,10 @@ static int do_plain_rerere(struct string_list *rr, int fd)
 	 * initial run would catch all and register their preimages.
 	 */
 	for (i = 0; i < conflict.nr; i++) {
+		struct rerere_id *id;
 		unsigned char sha1[20];
-		char *id;
-		int ret;
 		const char *path = conflict.items[i].string;
+		int ret;
 
 		if (string_list_has_string(rr, path))
 			continue;
@@ -686,7 +716,8 @@ static int do_plain_rerere(struct string_list *rr, int fd)
 		ret = handle_file(path, sha1, NULL);
 		if (ret < 1)
 			continue;
-		id = xstrdup(sha1_to_hex(sha1));
+
+		id = new_rerere_id(sha1);
 		string_list_insert(rr, path)->util = id;
 
 		/*
@@ -697,7 +728,7 @@ static int do_plain_rerere(struct string_list *rr, int fd)
 		 * NEEDSWORK: make sure "gc" does not remove
 		 * preimage without removing the directory.
 		 */
-		if (mkdir_in_gitdir(git_path("rr-cache/%s", id)))
+		if (mkdir_in_gitdir(rerere_path(id, NULL)))
 			continue;
 
 		/*
@@ -779,7 +810,7 @@ int rerere(int flags)
 static int rerere_forget_one_path(const char *path, struct string_list *rr)
 {
 	const char *filename;
-	char *id;
+	struct rerere_id *id;
 	unsigned char sha1[20];
 	int ret;
 	struct string_list_item *item;
@@ -793,7 +824,7 @@ static int rerere_forget_one_path(const char *path, struct string_list *rr)
 		return error("Could not parse conflict hunks in '%s'", path);
 
 	/* Nuke the recorded resolution for the conflict */
-	id = xstrdup(sha1_to_hex(sha1));
+	id = new_rerere_id(sha1);
 	filename = rerere_path(id, "postimage");
 	if (unlink(filename))
 		return (errno == ENOENT
@@ -813,7 +844,7 @@ static int rerere_forget_one_path(const char *path, struct string_list *rr)
 	 * conflict when the user is done.
 	 */
 	item = string_list_insert(rr, path);
-	free(item->util);
+	free_rerere_id(item);
 	item->util = id;
 	fprintf(stderr, "Forgot resolution for %s\n", path);
 	return 0;
@@ -850,22 +881,38 @@ int rerere_forget(struct pathspec *pathspec)
 /*
  * Garbage collection support
  */
-static time_t rerere_created_at(const char *id)
+
+/*
+ * Note that this is not reentrant but is used only one-at-a-time
+ * so it does not matter right now.
+ */
+static struct rerere_id *dirname_to_id(const char *name)
+{
+	static struct rerere_id id;
+	strcpy(id.hex, name);
+	return &id;
+}
+
+static time_t rerere_created_at(const char *dir_name)
 {
 	struct stat st;
+	struct rerere_id *id = dirname_to_id(dir_name);
+
 	return stat(rerere_path(id, "preimage"), &st) ? (time_t) 0 : st.st_mtime;
 }
 
-static time_t rerere_last_used_at(const char *id)
+static time_t rerere_last_used_at(const char *dir_name)
 {
 	struct stat st;
+	struct rerere_id *id = dirname_to_id(dir_name);
+
 	return stat(rerere_path(id, "postimage"), &st) ? (time_t) 0 : st.st_mtime;
 }
 
 /*
  * Remove the recorded resolution for a given conflict ID
  */
-static void unlink_rr_item(const char *id)
+static void unlink_rr_item(struct rerere_id *id)
 {
 	unlink(rerere_path(id, "thisimage"));
 	unlink(rerere_path(id, "preimage"));
@@ -875,7 +922,7 @@ static void unlink_rr_item(const char *id)
 	 * assume that we already have preimage recorded in
 	 * do_plain_rerere()?
 	 */
-	rmdir(git_path("rr-cache/%s", id));
+	rmdir(rerere_path(id, NULL));
 }
 
 void rerere_gc(struct string_list *rr)
@@ -914,7 +961,7 @@ void rerere_gc(struct string_list *rr)
 	closedir(dir);
 	/* ... and then remove them one-by-one */
 	for (i = 0; i < to_remove.nr; i++)
-		unlink_rr_item(to_remove.items[i].string);
+		unlink_rr_item(dirname_to_id(to_remove.items[i].string));
 	string_list_clear(&to_remove, 0);
 }
 
@@ -930,7 +977,7 @@ void rerere_clear(struct string_list *merge_rr)
 	int i;
 
 	for (i = 0; i < merge_rr->nr; i++) {
-		const char *id = (const char *)merge_rr->items[i].util;
+		struct rerere_id *id = merge_rr->items[i].util;
 		if (!has_rerere_resolution(id))
 			unlink_rr_item(id);
 	}
diff --git a/rerere.h b/rerere.h
index f998eba..ce545d0 100644
--- a/rerere.h
+++ b/rerere.h
@@ -15,9 +15,19 @@ struct pathspec;
  */
 extern void *RERERE_RESOLVED;
 
+struct rerere_id {
+	char hex[41];
+};
+
 extern int setup_rerere(struct string_list *, int);
 extern int rerere(int);
-extern const char *rerere_path(const char *id, const char *file);
+/*
+ * Given the conflict ID and the name of a "file" used for replaying
+ * the recorded resolution (e.g. "preimage", "postimage"), return the
+ * path to that filesystem entity.  With "file" specified with NULL,
+ * return the path to the directory that houses these files.
+ */
+extern const char *rerere_path(const struct rerere_id *, const char *file);
 extern int rerere_forget(struct pathspec *);
 extern int rerere_remaining(struct string_list *);
 extern void rerere_clear(struct string_list *);
-- 
2.5.0-rc2-340-g0cccc16

[PATCH v3 18/18] rerere: un-nest merge() further

[Junio C Hamano]

By consistently using "upon failure, set 'ret' and jump to out"
pattern, flatten the function further.

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

diff --git a/rerere.c b/rerere.c
index 469d8a8..e8d8c02 100644
--- a/rerere.c
+++ b/rerere.c
@@ -571,6 +571,7 @@ int rerere_remaining(struct string_list *merge_rr)
  */
 static int merge(const struct rerere_id *id, const char *path)
 {
+	FILE *f;
 	int ret;
 	mmfile_t cur = {NULL, 0}, base = {NULL, 0}, other = {NULL, 0};
 	mmbuffer_t result = {NULL, 0};
@@ -579,8 +580,10 @@ static int merge(const struct rerere_id *id, const char *path)
 	 * Normalize the conflicts in path and write it out to
 	 * "thisimage" temporary file.
 	 */
-	if (handle_file(path, NULL, rerere_path(id, "thisimage")) < 0)
-		return 1;
+	if (handle_file(path, NULL, rerere_path(id, "thisimage")) < 0) {
+		ret = 1;
+		goto out;
+	}
 
 	if (read_mmfile(&cur, rerere_path(id, "thisimage")) ||
 	    read_mmfile(&base, rerere_path(id, "preimage")) ||
@@ -594,29 +597,28 @@ static int merge(const struct rerere_id *id, const char *path)
 	 * low-level merge driver settings.
 	 */
 	ret = ll_merge(&result, path, &base, NULL, &cur, "", &other, "", NULL);
-	if (!ret) {
-		FILE *f;
+	if (ret)
+		goto out;
 
-		/*
-		 * A successful replay of recorded resolution.
-		 * Mark that "postimage" was used to help gc.
-		 */
-		if (utime(rerere_path(id, "postimage"), NULL) < 0)
-			warning("failed utime() on %s: %s",
-				rerere_path(id, "postimage"),
-				strerror(errno));
-
-		/* Update "path" with the resolution */
-		f = fopen(path, "w");
-		if (!f)
-			return error("Could not open %s: %s", path,
-				     strerror(errno));
-		if (fwrite(result.ptr, result.size, 1, f) != 1)
-			error("Could not write %s: %s", path, strerror(errno));
-		if (fclose(f))
-			return error("Writing %s failed: %s", path,
-				     strerror(errno));
-	}
+	/*
+	 * A successful replay of recorded resolution.
+	 * Mark that "postimage" was used to help gc.
+	 */
+	if (utime(rerere_path(id, "postimage"), NULL) < 0)
+		warning("failed utime() on %s: %s",
+			rerere_path(id, "postimage"),
+			strerror(errno));
+
+	/* Update "path" with the resolution */
+	f = fopen(path, "w");
+	if (!f)
+		return error("Could not open %s: %s", path,
+			     strerror(errno));
+	if (fwrite(result.ptr, result.size, 1, f) != 1)
+		error("Could not write %s: %s", path, strerror(errno));
+	if (fclose(f))
+		return error("Writing %s failed: %s", path,
+			     strerror(errno));
 
 out:
 	free(cur.ptr);
-- 
2.5.0-rc2-340-g0cccc16

Re: [PATCH v3 06/18] rerere: drop want_sp parameter from is_cmarker()

[Philip Oakley]

From: "Junio C Hamano" <gitster@pobox.com>
> As the nature of the conflict marker line determies if there should

"should be a"?
i.e. s/a /be a /    below

> a SP and label after it, the caller shouldn't have to pass the
> parameter redundantly.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> rerere.c | 27 ++++++++++++++++++++++-----
> 1 file changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/rerere.c b/rerere.c
> index 304df02..4c45f55 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -148,8 +148,25 @@ static int rerere_file_getline(struct strbuf *sb, 
> struct rerere_io *io_)
>  return strbuf_getwholeline(sb, io->input, '\n');
> }
>
> -static int is_cmarker(char *buf, int marker_char, int marker_size, 
> int want_sp)
> +/*
> + * Require the exact number of conflict marker letters, no more, no
> + * less, followed by SP or any whitespace
> + * (including LF).
> + */
> +static int is_cmarker(char *buf, int marker_char, int marker_size)
> {
> + int want_sp;
> +
> + /*
> + * The beginning of our version and the end of their version
> + * always are labeled like "<<<<< ours" or ">>>>> theirs",
> + * hence we set want_sp for them.  Note that the version from
> + * the common ancestor in diff3-style output is not always
> + * labelled (e.g. "||||| common" is often seen but "|||||"
> + * alone is also valid), so we do not set want_sp.
> + */
> + want_sp = (marker_char == '<') || (marker_char == '>');
> +
>  while (marker_size--)
>  if (*buf++ != marker_char)
>  return 0;
> @@ -172,19 +189,19 @@ static int handle_path(unsigned char *sha1, 
> struct rerere_io *io, int marker_siz
>  git_SHA1_Init(&ctx);
>
>  while (!io->getline(&buf, io)) {
> - if (is_cmarker(buf.buf, '<', marker_size, 1)) {
> + if (is_cmarker(buf.buf, '<', marker_size)) {
>  if (hunk != RR_CONTEXT)
>  goto bad;
>  hunk = RR_SIDE_1;
> - } else if (is_cmarker(buf.buf, '|', marker_size, 0)) {
> + } else if (is_cmarker(buf.buf, '|', marker_size)) {
>  if (hunk != RR_SIDE_1)
>  goto bad;
>  hunk = RR_ORIGINAL;
> - } else if (is_cmarker(buf.buf, '=', marker_size, 0)) {
> + } else if (is_cmarker(buf.buf, '=', marker_size)) {
>  if (hunk != RR_SIDE_1 && hunk != RR_ORIGINAL)
>  goto bad;
>  hunk = RR_SIDE_2;
> - } else if (is_cmarker(buf.buf, '>', marker_size, 1)) {
> + } else if (is_cmarker(buf.buf, '>', marker_size)) {
>  if (hunk != RR_SIDE_2)
>  goto bad;
>  if (strbuf_cmp(&one, &two) > 0)
> -- 
> 2.5.0-rc2-340-g0cccc16
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 06/18] rerere: drop want_sp parameter from is_cmarker()

[Eric Sunshine]

On Sat, Jul 18, 2015 at 4:24 AM, Philip Oakley <philipoakley@iee.org> wrote:
> From: "Junio C Hamano" <gitster@pobox.com>
>> As the nature of the conflict marker line determies if there should
>
> "should be a"?
> i.e. s/a /be a /    below

Also: s/determies/determines/

Re: [PATCH v3 17/18] rerere: use "struct rerere_id" instead of "char *" for conflict ID

[Eric Sunshine]

On Fri, Jul 17, 2015 at 6:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
> This gives a thin abstraction between the conflict ID that is a hash
> value obtained by inspecting the conflicts and the name of the
> directory under $GIT_DIR/rr-cache/, in which the previous resolution
> is recorded to be replayed.  The plan is to make sure that the
> presense of the directory does not imply the presense of a previous

s/presense/presence/g

> resolution and vice-versa, and later allow us to have more than one
> pair of <preimage, postimage> for a given conflict ID.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Re: [PATCH v3 01/18] rerere: fix an off-by-one non-bug

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> When ac49f5ca (rerere "remaining", 2011-02-16) split out a new
> helper function check_one_conflict() out of find_conflict()
> function, so that the latter will use the returned value from the
> new helper to update the loop control variable that is an index into
> active_cache[], the new variable incremented the index by one too
> many when it found a path with only stage #1 entry at the very end
> of active_cache[].
>
> This "strange" return value does not have any effect on the loop
> control of two callers of this function, as they all notice that
> active_nr+2 is larger than active_nr just like active_nr+1 is, but
> nevertheless it puzzles the readers when they are trying to figure
> out what the function is trying to do.
>
> In fact, there is no need to do an early return.  The code that
> follows after skipping the stage #1 entry is fully prepared to
> handle a case where the entry is at the very end of active_cache[].
>
> Help future readers from unnecessary confusion by dropping an early
> return.  We skip the stage #1 entry, and if there are stage #2 and
> stage #3 entries for the same path, we diagnose the path as
> THREE_STAGED (otherwise we say PUNTED), and then we skip all entries
> for the same path.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  rerere.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/rerere.c b/rerere.c
> index 31644de..e307711 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -369,10 +369,8 @@ static int check_one_conflict(int i, int *type)
>  	}
>  
>  	*type = PUNTED;
> -	if (ce_stage(e) == 1) {
> -		if (active_nr <= ++i)
> -			return i + 1;
> -	}
> +	if (ce_stage(e) == 1)
> +		i++;

As a conflicted index can have multiple stage #1 entries when
dealing with a criss-cross merge, this should probably be

	while (ce_stage(e) == 1)
        	i++;

instead.

>  	/* Only handle regular files with both stages #2 and #3 */
>  	if (i + 1 < active_nr) {

Re: [PATCH v3 07/18] rerere: stop looping unnecessarily

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> handle_cache() loops 3 times starting from an index entry that is
> unmerged, while ignoring an entry for a path that is different from
> what we are looking for.
>
> As the index is sorted, once we see a different path, we know we saw
> all stages for the path we are interested in.  Just loop while we
> see the same path and then break, instead of continuing for 3 times.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  rerere.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/rerere.c b/rerere.c
> index 4c45f55..7b1419c 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -329,24 +329,21 @@ static int handle_cache(const char *path, unsigned char *sha1, const char *outpu
>  		return -1;
>  	pos = -pos - 1;
>  
> -	for (i = 0; i < 3; i++) {
> +	while (pos < active_nr) {
>  		enum object_type type;
>  		unsigned long size;
> -		int j;
>  
> -		if (active_nr <= pos)
> -			break;
>  		ce = active_cache[pos++];
>  		if (ce_namelen(ce) != len || memcmp(ce->name, path, len))
> -			continue;
> -		j = ce_stage(ce) - 1;
> -		mmfile[j].ptr = read_sha1_file(ce->sha1, &type, &size);
> -		mmfile[j].size = size;
> +			break;
> +		i = ce_stage(ce) - 1;
> +		mmfile[i].ptr = read_sha1_file(ce->sha1, &type, &size);
> +		mmfile[i].size = size;

If the conflicted index has multiple stage #1 entries, this will leak
what was previously read.  As the array is initialized to NULLs, perhaps


	i = ce_stage(ce) - 1;
        if (!mmfile[i].ptr) {
		mmfile[i].ptr = read_sha1_file(ce->sha1, &type, &size);
                mmfile[i].size = size;
	}

should be sufficient.  This does change the semantics, in that we
used to use the last stage #1 entry as the common ancestor when
doing the plain-vanilla three-way merge, but with the leak fix, we
will use the first stage #1 entry.  But it does not change it in
any meaningful way---we are using only one of multiple stage #1
entries arbitrarily picked either way.

>  	}
> -	for (i = 0; i < 3; i++) {
> +	for (i = 0; i < 3; i++)
>  		if (!mmfile[i].ptr && !mmfile[i].size)
>  			mmfile[i].ptr = xstrdup("");
> -	}
> +
>  	/*
>  	 * NEEDSWORK: handle conflicts from merges with
>  	 * merge.renormalize set, too

Re: [PATCH v3 08/18] rerere: explain the rerere I/O abstraction

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> +/*
> + * Write a conflict marker to io->output (if defined).
> + */
>  static void rerere_io_putconflict(int ch, int size, struct rerere_io *io)
>  {
>  	char buf[64];

This is unrelated to the topic of this step, but this function seems
to be very poorly put together with duct tape.

> static void rerere_io_putconflict(int ch, int size, struct rerere_io *io)
> {
> 	char buf[64];
> 
> 	while (size) {
> 		if (size < sizeof(buf) - 2) {
> 			memset(buf, ch, size);
> 			buf[size] = '\n';
> 			buf[size + 1] = '\0';
> 			size = 0;

The if() guarding this block has an off-by-one error in the benign
direction.  When size is 62, the marker with terminating LF and NUL
should still fit within the buf[], so it should use "<=", not "<",
to compare.

> 		} else {
> 			int sz = sizeof(buf) - 1;
> 			if (size <= sz)
> 				sz -= (sz - size) + 1;
> 			memset(buf, ch, sz);
> 			buf[sz] = '\0';
> 			size -= sz;

This is an even more awkward construct.  sz is what we have to work
with the substring that we cannot emit with a single call (because
buf[] is too small), so naturally I would expect it to be more like

	int to_emit = size;
        if (sz <= size)
        	to_emit = sz;
	memset(buf, ch, to_emit);
        buf[to_emit] = '\0';
	size -= to_emit;

which makes the "if (size <= sz)" in the code look very suspicious.

But rewriting it to the "more natural" version would make it buggy.
At a right boundary condition, i.e. size == 63, we may find that the
conflict marker is too long with LF and NUL to fit in buf[] and come
here, and then fill the full conflict marker with NUL just fine in
buf[], decrementing size to 0, emit that 63-byte long marker.  The
next iteration will exit the loop without emitting the LF.

The unnatural is what is saving us from such a bug.

	if (size <= sz)
		sz -= (sz - size) + 1;

It ensures that subtraction of sz (i.e. "to_emit") from size before
the next iteration will never brings size down to zero by reducing
sz by one too much, forcing another iteration, which will then have
size smaller than "sizeof(buf) - 2" and have us emit the LF.

Not buggy, but ugly.

> 		}
> 		rerere_io_putstr(buf, io);
> 	}
> }