From fb70a06da2f10e8b3255abd76cd6ec1926bd9a40 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sun, 28 Jun 2015 14:35:13 -0700 Subject: rerere: fix an off-by-one non-bug 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 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) { -- cgit v0.10.2-6-g49f6 From 5eda906b2873c986fa61406dafb6acd99e70d540 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 24 Jul 2015 15:08:03 -0700 Subject: rerere: handle conflicts with multiple stage #1 entries A conflicted index can have multiple stage #1 entries when dealing with a criss-cross merge and using the "resolve" merge strategy. Signed-off-by: Junio C Hamano diff --git a/rerere.c b/rerere.c index e307711..b453a80 100644 --- a/rerere.c +++ b/rerere.c @@ -369,7 +369,7 @@ static int check_one_conflict(int i, int *type) } *type = PUNTED; - if (ce_stage(e) == 1) + while (ce_stage(active_cache[i]) == 1) i++; /* Only handle regular files with both stages #2 and #3 */ -- cgit v0.10.2-6-g49f6 From 8d9b5a4ada8b8e187af7dbdc7bc24f6ed774df80 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 30 Jun 2015 13:03:36 -0700 Subject: rerere: plug conflict ID leaks 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 diff --git a/rerere.c b/rerere.c index b453a80..0f40f89 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; } -- cgit v0.10.2-6-g49f6 From f5800f6ad8b8cbf41a252f7ca0ae465217174c60 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sun, 28 Jun 2015 15:51:59 -0700 Subject: rerere: lift PATH_MAX limitation 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 diff --git a/rerere.c b/rerere.c index 0f40f89..576361f 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); } -- cgit v0.10.2-6-g49f6 From e2cb6a950b4617edc3d07b372063b624b2b5c420 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sun, 28 Jun 2015 16:28:00 -0700 Subject: rerere: write out each record of MERGE_RR in one go 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 diff --git a/rerere.c b/rerere.c index 576361f..d8518a4 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"); -- cgit v0.10.2-6-g49f6 From a14c7ab8f58f3b2aea99e65a74c9f9ab4f955a40 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sun, 28 Jun 2015 21:13:24 -0700 Subject: rerere: report autoupdated paths only after actually updating them Signed-off-by: Junio C Hamano diff --git a/rerere.c b/rerere.c index d8518a4..7fa58cc 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. */ -- cgit v0.10.2-6-g49f6 From 67711cdc399203ec1d70cde2c3cd71e37e43da70 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 29 Jun 2015 15:05:24 -0700 Subject: rerere: drop want_sp parameter from is_cmarker() As the nature of the conflict marker line determines if there should be a SP and label after it, the caller shouldn't have to pass the parameter redundantly. Signed-off-by: Junio C Hamano diff --git a/rerere.c b/rerere.c index 7fa58cc..2bce1c8 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) -- cgit v0.10.2-6-g49f6 From 74444d4ec4c23d254040de7b2637660b7f141110 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 30 Jun 2015 16:10:10 -0700 Subject: rerere: stop looping unnecessarily 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 diff --git a/rerere.c b/rerere.c index 2bce1c8..e1d527c 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 -- cgit v0.10.2-6-g49f6 From 7d4053b69b37927c0ccd98eac41f32707227eca0 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 24 Jul 2015 15:10:52 -0700 Subject: rerere: do not leak mmfile[] for a path with multiple stage #1 entries A conflicted index can have multiple stage #1 entries when dealing with a criss-cross merge and using the "resolve" merge strategy. Plug the leak by reading only the first one of the same stage entries. Strictly speaking, this fix 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 is not a grave backward compatibility breakage. Either way, we are arbitrarily picking one of multiple stage #1 entries and using it, ignoring others, and there is no meaning in the ordering of these stage #1 entries. Signed-off-by: Junio C Hamano diff --git a/rerere.c b/rerere.c index e1d527c..d665deb 100644 --- a/rerere.c +++ b/rerere.c @@ -337,8 +337,10 @@ static int handle_cache(const char *path, unsigned char *sha1, const char *outpu if (ce_namelen(ce) != len || memcmp(ce->name, path, len)) break; i = ce_stage(ce) - 1; - mmfile[i].ptr = read_sha1_file(ce->sha1, &type, &size); - mmfile[i].size = size; + if (!mmfile[i].ptr) { + mmfile[i].ptr = read_sha1_file(ce->sha1, &type, &size); + mmfile[i].size = size; + } } for (i = 0; i < 3; i++) if (!mmfile[i].ptr && !mmfile[i].size) -- cgit v0.10.2-6-g49f6 From a96847cc1691840bd95cc56549d7c00b35f6d5a0 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 30 Jun 2015 22:33:19 -0700 Subject: rerere: explain the rerere I/O abstraction 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 diff --git a/rerere.c b/rerere.c index d665deb..7db5b54 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_; -- cgit v0.10.2-6-g49f6 From d3c2749def9563798cea3486f3793ad36d9c1030 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 24 Jul 2015 16:01:48 -0700 Subject: rerere: fix benign off-by-one non-bug and clarify code rerere_io_putconflict() wants to use a limited fixed-sized buf[] on stack repeatedly to formulate a longer string, but its implementation is doubly confusing: * When it knows that the whole thing fits in buf[], it wants to fill early part of buf[] with conflict marker characters, followed by a LF and a NUL. It miscounts the size of the buffer by 1 and does not use the last byte of buf[]. * When it needs to show only the early part of a long conflict marker string (because the whole thing does not fit in buf[]), it adjusts the number of bytes shown in the current round in a strange-looking way. It makes sure that this round does not emit all bytes and leaves at least one byte to the next round, so that "it all fits" case will pick up the rest and show the terminating LF. While this is correct, one needs to stop and think for a while to realize why it is correct without an explanation. Fix the benign off-by-one, and add comments to explain the strange-looking size adjustment. Signed-off-by: Junio C Hamano diff --git a/rerere.c b/rerere.c index 7db5b54..6fd8c5d 100644 --- a/rerere.c +++ b/rerere.c @@ -125,13 +125,20 @@ static void rerere_io_putconflict(int ch, int size, struct rerere_io *io) char buf[64]; while (size) { - if (size < sizeof(buf) - 2) { + if (size <= sizeof(buf) - 2) { memset(buf, ch, size); buf[size] = '\n'; buf[size + 1] = '\0'; size = 0; } else { int sz = sizeof(buf) - 1; + + /* + * Make sure we will not write everything out + * in this round by leaving at least 1 byte + * for the next round, giving the next round + * a chance to add the terminating LF. Yuck. + */ if (size <= sz) sz -= (sz - size) + 1; memset(buf, ch, sz); -- cgit v0.10.2-6-g49f6 From 4b68c2a0877b18d1fe591dd7aee55aa1158fada0 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 30 Jun 2015 22:36:35 -0700 Subject: rerere: explain MERGE_RR management helpers 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 diff --git a/rerere.c b/rerere.c index 6fd8c5d..1e68b8a 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; @@ -403,6 +410,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]; @@ -434,6 +449,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; @@ -450,6 +476,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; -- cgit v0.10.2-6-g49f6 From cc899eca552a9b93788e6bca34aa0e4d86b251a0 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 30 Jun 2015 22:40:35 -0700 Subject: rerere: explain the primary codepath 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 diff --git a/rerere.c b/rerere.c index 1e68b8a..6961c17 100644 --- a/rerere.c +++ b/rerere.c @@ -206,6 +206,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; @@ -276,6 +291,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; @@ -515,29 +534,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, @@ -590,41 +634,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; @@ -638,13 +702,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; @@ -698,6 +762,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; -- cgit v0.10.2-6-g49f6 From 963ec003560765bde56a880d89c75056ebe9e990 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 30 Jun 2015 22:42:34 -0700 Subject: rerere: explain "rerere forget" codepath 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 diff --git a/rerere.c b/rerere.c index 6961c17..c3e34d1 100644 --- a/rerere.c +++ b/rerere.c @@ -422,6 +422,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) @@ -786,9 +790,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)) @@ -796,9 +806,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; @@ -817,6 +836,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++) { -- cgit v0.10.2-6-g49f6 From e828de826bd0b852337b9625354c7c73d8de20c0 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 30 Jun 2015 22:43:37 -0700 Subject: rerere: explain the remainder 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 diff --git a/rerere.c b/rerere.c index c3e34d1..e167670 100644 --- a/rerere.c +++ b/rerere.c @@ -853,6 +853,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; @@ -865,11 +868,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)); } @@ -889,6 +900,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; @@ -906,11 +918,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; -- cgit v0.10.2-6-g49f6 From 8e7768b2de8bfdf82cde565d2f42e8d7f91e74e0 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 30 Jun 2015 19:36:24 -0700 Subject: rerere: refactor "replay" part of do_plain_rerere() 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 diff --git a/rerere.c b/rerere.c index e167670..1d0f69d 100644 --- a/rerere.c +++ b/rerere.c @@ -629,6 +629,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; @@ -682,41 +720,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); -- cgit v0.10.2-6-g49f6 From c7a25d3790bdbc486362084238db5a773f728570 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sat, 4 Jul 2015 17:17:38 -0700 Subject: rerere: further de-dent do_plain_rerere() It's just easier to follow this way. Signed-off-by: Junio C Hamano diff --git a/rerere.c b/rerere.c index 1d0f69d..7b4028c 100644 --- a/rerere.c +++ b/rerere.c @@ -682,42 +682,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 (string_list_has_string(rr, path)) + continue; - /* - * 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; + /* + * 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; - /* - * 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 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++) -- cgit v0.10.2-6-g49f6 From 925d73c4217388838e36bfed85553132c458c7d0 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 6 Jul 2015 14:18:09 -0700 Subject: rerere: further clarify do_rerere_one_path() Signed-off-by: Junio C Hamano diff --git a/rerere.c b/rerere.c index 7b4028c..80be303 100644 --- a/rerere.c +++ b/rerere.c @@ -653,16 +653,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; } -- cgit v0.10.2-6-g49f6 From 18bb99342fdb4b612ae45be3fef084ceebd498a0 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 6 Jul 2015 14:45:55 -0700 Subject: rerere: call conflict-ids IDs 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 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 80be303..2865aea 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); } /* @@ -539,7 +539,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. @@ -548,7 +548,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}; @@ -558,12 +558,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; } @@ -580,9 +580,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 */ @@ -640,11 +640,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) @@ -655,7 +655,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; @@ -680,7 +680,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; @@ -695,8 +695,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 @@ -706,7 +706,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; /* @@ -714,7 +714,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); } @@ -788,7 +788,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; @@ -802,8 +802,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) @@ -814,7 +814,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); /* @@ -823,7 +823,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; } @@ -859,32 +859,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) @@ -939,9 +939,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 *); -- cgit v0.10.2-6-g49f6 From 1d51eced103f1c3e36beb6c1e01a413660910a50 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sat, 4 Jul 2015 17:38:34 -0700 Subject: rerere: use "struct rerere_id" instead of "char *" for conflict ID 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 presence 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 for a given conflict ID. Signed-off-by: Junio C Hamano 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 2865aea..93fdeec 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", rerere_id_hex(id)); + + return git_path("rr-cache/%s/%s", rerere_id_hex(id), file); } -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"); @@ -530,7 +560,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; } } @@ -548,7 +578,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}; @@ -582,8 +612,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"); @@ -640,7 +670,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)) { @@ -660,7 +690,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; } @@ -679,10 +709,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; @@ -695,7 +725,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; /* @@ -706,7 +737,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; /* @@ -788,7 +819,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; @@ -802,7 +833,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 @@ -822,7 +853,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; @@ -859,22 +890,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")); @@ -884,7 +931,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) @@ -923,7 +970,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); } @@ -939,7 +986,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 *); -- cgit v0.10.2-6-g49f6 From 15ed07d532db743a2a397a38bacc1f20e54b2c80 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 6 Jul 2015 15:32:53 -0700 Subject: rerere: un-nest merge() further By consistently using "upon failure, set 'ret' and jump to out" pattern, flatten the function further. Signed-off-by: Junio C Hamano diff --git a/rerere.c b/rerere.c index 93fdeec..9bef24f 100644 --- a/rerere.c +++ b/rerere.c @@ -580,6 +580,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}; @@ -588,8 +589,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")) || @@ -603,29 +606,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); -- cgit v0.10.2-6-g49f6