Merge branch 'tg/rerere'
authorJunio C Hamano <gitster@pobox.com>
Mon, 17 Sep 2018 20:53:51 +0000 (13:53 -0700)
committerJunio C Hamano <gitster@pobox.com>
Mon, 17 Sep 2018 20:53:51 +0000 (13:53 -0700)
Fixes to "git rerere" corner cases, especially when conflict
markers cannot be parsed in the file.

* tg/rerere:
rerere: recalculate conflict ID when unresolved conflict is committed
rerere: teach rerere to handle nested conflicts
rerere: return strbuf from handle path
rerere: factor out handle_conflict function
rerere: only return whether a path has conflicts or not
rerere: fix crash with files rerere can't handle
rerere: add documentation for conflict normalization
rerere: mark strings for translation
rerere: wrap paths in output in sq
rerere: lowercase error messages
rerere: unify error messages when read_cache fails

1  2 
rerere.c
t/t4200-rerere.sh
diff --combined rerere.c
index c7787aa07f80f00589e3c4f4e4e3cc825c43044e,dd81d09e1915c6581eb4381ff9d8ca5592557aad..2fd3181f7beb7e2b19e4ca7ae26a50d703b60728
+++ b/rerere.c
@@@ -9,7 -9,6 +9,7 @@@
  #include "ll-merge.h"
  #include "attr.h"
  #include "pathspec.h"
 +#include "object-store.h"
  #include "sha1-lookup.h"
  
  #define RESOLVED 0
@@@ -201,7 -200,7 +201,7 @@@ static struct rerere_id *new_rerere_id(
  static void read_rr(struct string_list *rr)
  {
        struct strbuf buf = STRBUF_INIT;
 -      FILE *in = fopen_or_warn(git_path_merge_rr(), "r");
 +      FILE *in = fopen_or_warn(git_path_merge_rr(the_repository), "r");
  
        if (!in)
                return;
  
                /* 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");
+                       die(_("corrupt MERGE_RR"));
  
                if (buf.buf[40] != '.') {
                        variant = 0;
                        errno = 0;
                        variant = strtol(buf.buf + 41, &path, 10);
                        if (errno)
-                               die("corrupt MERGE_RR");
+                               die(_("corrupt MERGE_RR"));
                }
                if (*(path++) != '\t')
-                       die("corrupt MERGE_RR");
+                       die(_("corrupt MERGE_RR"));
                buf.buf[40] = '\0';
                id = new_rerere_id_hex(buf.buf);
                id->variant = variant;
@@@ -260,12 -259,12 +260,12 @@@ static int write_rr(struct string_list 
                                    rr->items[i].string, 0);
  
                if (write_in_full(out_fd, buf.buf, buf.len) < 0)
-                       die("unable to write rerere record");
+                       die(_("unable to write rerere record"));
  
                strbuf_release(&buf);
        }
        if (commit_lock_file(&write_lock) != 0)
-               die("unable to write rerere record");
+               die(_("unable to write rerere record"));
        return 0;
  }
  
@@@ -303,38 -302,6 +303,6 @@@ static void rerere_io_putstr(const cha
                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];
-       while (size) {
-               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);
-                       buf[sz] = '\0';
-                       size -= sz;
-               }
-               rerere_io_putstr(buf, io);
-       }
- }
  static void rerere_io_putmem(const char *mem, size_t sz, struct rerere_io *io)
  {
        if (io->output)
@@@ -385,89 -352,109 +353,109 @@@ static int is_cmarker(char *buf, int ma
        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)
+ static void rerere_strbuf_putconflict(struct strbuf *buf, int ch, size_t size)
+ {
+       strbuf_addchars(buf, ch, size);
+       strbuf_addch(buf, '\n');
+ }
+ static int handle_conflict(struct strbuf *out, struct rerere_io *io,
+                          int marker_size, git_SHA_CTX *ctx)
  {
-       git_SHA_CTX ctx;
-       int hunk_no = 0;
        enum {
-               RR_CONTEXT = 0, RR_SIDE_1, RR_SIDE_2, RR_ORIGINAL
-       } hunk = RR_CONTEXT;
+               RR_SIDE_1 = 0, RR_SIDE_2, RR_ORIGINAL
+       } hunk = RR_SIDE_1;
        struct strbuf one = STRBUF_INIT, two = STRBUF_INIT;
-       struct strbuf buf = STRBUF_INIT;
-       if (sha1)
-               git_SHA1_Init(&ctx);
+       struct strbuf buf = STRBUF_INIT, conflict = STRBUF_INIT;
+       int has_conflicts = -1;
  
        while (!io->getline(&buf, io)) {
                if (is_cmarker(buf.buf, '<', marker_size)) {
-                       if (hunk != RR_CONTEXT)
-                               goto bad;
-                       hunk = RR_SIDE_1;
+                       if (handle_conflict(&conflict, io, marker_size, NULL) < 0)
+                               break;
+                       if (hunk == RR_SIDE_1)
+                               strbuf_addbuf(&one, &conflict);
+                       else
+                               strbuf_addbuf(&two, &conflict);
+                       strbuf_release(&conflict);
                } else if (is_cmarker(buf.buf, '|', marker_size)) {
                        if (hunk != RR_SIDE_1)
-                               goto bad;
+                               break;
                        hunk = RR_ORIGINAL;
                } else if (is_cmarker(buf.buf, '=', marker_size)) {
                        if (hunk != RR_SIDE_1 && hunk != RR_ORIGINAL)
-                               goto bad;
+                               break;
                        hunk = RR_SIDE_2;
                } else if (is_cmarker(buf.buf, '>', marker_size)) {
                        if (hunk != RR_SIDE_2)
-                               goto bad;
+                               break;
                        if (strbuf_cmp(&one, &two) > 0)
                                strbuf_swap(&one, &two);
-                       hunk_no++;
-                       hunk = RR_CONTEXT;
-                       rerere_io_putconflict('<', marker_size, io);
-                       rerere_io_putmem(one.buf, one.len, io);
-                       rerere_io_putconflict('=', marker_size, io);
-                       rerere_io_putmem(two.buf, two.len, io);
-                       rerere_io_putconflict('>', marker_size, io);
-                       if (sha1) {
-                               git_SHA1_Update(&ctx, one.buf ? one.buf : "",
+                       has_conflicts = 1;
+                       rerere_strbuf_putconflict(out, '<', marker_size);
+                       strbuf_addbuf(out, &one);
+                       rerere_strbuf_putconflict(out, '=', marker_size);
+                       strbuf_addbuf(out, &two);
+                       rerere_strbuf_putconflict(out, '>', marker_size);
+                       if (ctx) {
+                               git_SHA1_Update(ctx, one.buf ? one.buf : "",
                                            one.len + 1);
-                               git_SHA1_Update(&ctx, two.buf ? two.buf : "",
+                               git_SHA1_Update(ctx, two.buf ? two.buf : "",
                                            two.len + 1);
                        }
-                       strbuf_reset(&one);
-                       strbuf_reset(&two);
+                       break;
                } else if (hunk == RR_SIDE_1)
                        strbuf_addbuf(&one, &buf);
                else if (hunk == RR_ORIGINAL)
                        ; /* discard */
                else if (hunk == RR_SIDE_2)
                        strbuf_addbuf(&two, &buf);
-               else
-                       rerere_io_putstr(buf.buf, io);
-               continue;
-       bad:
-               hunk = 99; /* force error exit */
-               break;
        }
        strbuf_release(&one);
        strbuf_release(&two);
        strbuf_release(&buf);
  
+       return has_conflicts;
+ }
+ /*
+  * 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 1 if conflict hunks are found, 0 if there are no conflict
+  * hunks and -1 if an error occured.
+  */
+ static int handle_path(unsigned char *sha1, struct rerere_io *io, int marker_size)
+ {
+       git_SHA_CTX ctx;
+       struct strbuf buf = STRBUF_INIT, out = STRBUF_INIT;
+       int has_conflicts = 0;
+       if (sha1)
+               git_SHA1_Init(&ctx);
+       while (!io->getline(&buf, io)) {
+               if (is_cmarker(buf.buf, '<', marker_size)) {
+                       has_conflicts = handle_conflict(&out, io, marker_size,
+                                                       sha1 ? &ctx : NULL);
+                       if (has_conflicts < 0)
+                               break;
+                       rerere_io_putmem(out.buf, out.len, io);
+                       strbuf_reset(&out);
+               } else
+                       rerere_io_putstr(buf.buf, io);
+       }
+       strbuf_release(&buf);
+       strbuf_release(&out);
        if (sha1)
                git_SHA1_Final(sha1, &ctx);
-       if (hunk != RR_CONTEXT)
-               return -1;
-       return hunk_no;
+       return has_conflicts;
  }
  
  /*
   */
  static int handle_file(const char *path, unsigned char *sha1, const char *output)
  {
-       int hunk_no = 0;
+       int has_conflicts = 0;
        struct rerere_io_file io;
        int marker_size = ll_merge_marker_size(path);
  
        io.input = fopen(path, "r");
        io.io.wrerror = 0;
        if (!io.input)
-               return error_errno("Could not open %s", path);
+               return error_errno(_("could not open '%s'"), path);
  
        if (output) {
                io.io.output = fopen(output, "w");
                if (!io.io.output) {
-                       error_errno("Could not write %s", output);
+                       error_errno(_("could not write '%s'"), output);
                        fclose(io.input);
                        return -1;
                }
        }
  
-       hunk_no = handle_path(sha1, (struct rerere_io *)&io, marker_size);
+       has_conflicts = handle_path(sha1, (struct rerere_io *)&io, marker_size);
  
        fclose(io.input);
        if (io.io.wrerror)
-               error("There were errors while writing %s (%s)",
+               error(_("there were errors while writing '%s' (%s)"),
                      path, strerror(io.io.wrerror));
        if (io.io.output && fclose(io.io.output))
-               io.io.wrerror = error_errno("Failed to flush %s", path);
+               io.io.wrerror = error_errno(_("failed to flush '%s'"), path);
  
-       if (hunk_no < 0) {
+       if (has_conflicts < 0) {
                if (output)
                        unlink_or_warn(output);
-               return error("Could not parse conflict hunks in %s", path);
+               return error(_("could not parse conflict hunks in '%s'"), path);
        }
        if (io.io.wrerror)
                return -1;
-       return hunk_no;
+       return has_conflicts;
  }
  
  /*
@@@ -569,7 -556,7 +557,7 @@@ static int find_conflict(struct string_
  {
        int i;
        if (read_cache() < 0)
-               return error("Could not read index");
+               return error(_("index file corrupt"));
  
        for (i = 0; i < active_nr;) {
                int conflict_type;
@@@ -602,7 -589,7 +590,7 @@@ int rerere_remaining(struct string_lis
        if (setup_rerere(merge_rr, RERERE_READONLY))
                return 0;
        if (read_cache() < 0)
-               return error("Could not read index");
+               return error(_("index file corrupt"));
  
        for (i = 0; i < active_nr;) {
                int conflict_type;
@@@ -685,17 -672,17 +673,17 @@@ static int merge(const struct rerere_i
         * Mark that "postimage" was used to help gc.
         */
        if (utime(rerere_path(id, "postimage"), NULL) < 0)
-               warning_errno("failed utime() on %s",
+               warning_errno(_("failed utime() on '%s'"),
                              rerere_path(id, "postimage"));
  
        /* Update "path" with the resolution */
        f = fopen(path, "w");
        if (!f)
-               return error_errno("Could not open %s", path);
+               return error_errno(_("could not open '%s'"), path);
        if (fwrite(result.ptr, result.size, 1, f) != 1)
-               error_errno("Could not write %s", path);
+               error_errno(_("could not write '%s'"), path);
        if (fclose(f))
-               return error_errno("Writing %s failed", path);
+               return error_errno(_("writing '%s' failed"), path);
  
  out:
        free(cur.ptr);
@@@ -715,13 -702,13 +703,13 @@@ static void update_paths(struct string_
                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",
+               fprintf_ln(stderr, _("Staged '%s' using previous resolution."),
                        item->string);
        }
  
        if (write_locked_index(&the_index, &index_lock,
                               COMMIT_LOCK | SKIP_IF_UNCHANGED))
-               die("Unable to write new index file");
+               die(_("unable to write new index file"));
  }
  
  static void remove_variant(struct rerere_id *id)
@@@ -753,7 -740,7 +741,7 @@@ static void do_rerere_one_path(struct s
                if (!handle_file(path, NULL, NULL)) {
                        copy_file(rerere_path(id, "postimage"), path, 0666);
                        id->collection->status[variant] |= RR_HAS_POSTIMAGE;
-                       fprintf(stderr, "Recorded resolution for '%s'.\n", path);
+                       fprintf_ln(stderr, _("Recorded resolution for '%s'."), path);
                        free_rerere_id(rr_item);
                        rr_item->util = NULL;
                        return;
                if (rerere_autoupdate)
                        string_list_insert(update, path);
                else
-                       fprintf(stderr,
-                               "Resolved '%s' using previous resolution.\n",
-                               path);
+                       fprintf_ln(stderr,
+                                  _("Resolved '%s' using previous resolution."),
+                                  path);
                free_rerere_id(rr_item);
                rr_item->util = NULL;
                return;
        if (id->collection->status[variant] & RR_HAS_POSTIMAGE) {
                const char *path = rerere_path(id, "postimage");
                if (unlink(path))
-                       die_errno("cannot unlink stray '%s'", path);
+                       die_errno(_("cannot unlink stray '%s'"), path);
                id->collection->status[variant] &= ~RR_HAS_POSTIMAGE;
        }
        id->collection->status[variant] |= RR_HAS_PREIMAGE;
-       fprintf(stderr, "Recorded preimage for '%s'\n", path);
+       fprintf_ln(stderr, _("Recorded preimage for '%s'"), path);
  }
  
  static int do_plain_rerere(struct string_list *rr, int fd)
                const char *path = conflict.items[i].string;
                int ret;
  
-               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 != 0 && string_list_has_string(rr, path)) {
+                       remove_variant(string_list_lookup(rr, path)->util);
+                       string_list_remove(rr, path, 1);
+               }
                if (ret < 1)
                        continue;
  
@@@ -879,7 -867,7 +868,7 @@@ static int is_rerere_enabled(void
                return rr_cache_exists;
  
        if (!rr_cache_exists && mkdir_in_gitdir(git_path_rr_cache()))
-               die("Could not create directory %s", git_path_rr_cache());
+               die(_("could not create directory '%s'"), git_path_rr_cache());
        return 1;
  }
  
@@@ -896,8 -884,7 +885,8 @@@ int setup_rerere(struct string_list *me
        if (flags & RERERE_READONLY)
                fd = 0;
        else
 -              fd = hold_lock_file_for_update(&write_lock, git_path_merge_rr(),
 +              fd = hold_lock_file_for_update(&write_lock,
 +                                             git_path_merge_rr(the_repository),
                                               LOCK_DIE_ON_ERROR);
        read_rr(merge_rr);
        return fd;
@@@ -958,7 -945,7 +947,7 @@@ static int handle_cache(const char *pat
        mmfile_t mmfile[3] = {{NULL}};
        mmbuffer_t result = {NULL, 0};
        const struct cache_entry *ce;
-       int pos, len, i, hunk_no;
+       int pos, len, i, has_conflicts;
        struct rerere_io_mem io;
        int marker_size = ll_merge_marker_size(path);
  
         * 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);
+       has_conflicts = handle_path(sha1, (struct rerere_io *)&io, marker_size);
        strbuf_release(&io.input);
        if (io.io.output)
                fclose(io.io.output);
-       return hunk_no;
+       return has_conflicts;
  }
  
  static int rerere_forget_one_path(const char *path, struct string_list *rr)
         */
        ret = handle_cache(path, sha1, NULL);
        if (ret < 1)
-               return error("Could not parse conflict hunks in '%s'", path);
+               return error(_("could not parse conflict hunks in '%s'"), path);
  
        /* Nuke the recorded resolution for the conflict */
        id = new_rerere_id(sha1);
                handle_cache(path, sha1, rerere_path(id, "thisimage"));
                if (read_mmfile(&cur, rerere_path(id, "thisimage"))) {
                        free(cur.ptr);
-                       error("Failed to update conflicted state in '%s'", path);
+                       error(_("failed to update conflicted state in '%s'"), path);
                        goto fail_exit;
                }
                cleanly_resolved = !try_merge(id, path, &cur, &result);
        }
  
        if (id->collection->status_nr <= id->variant) {
-               error("no remembered resolution for '%s'", path);
+               error(_("no remembered resolution for '%s'"), path);
                goto fail_exit;
        }
  
        filename = rerere_path(id, "postimage");
        if (unlink(filename)) {
                if (errno == ENOENT)
-                       error("no remembered resolution for %s", path);
+                       error(_("no remembered resolution for '%s'"), path);
                else
-                       error_errno("cannot unlink %s", filename);
+                       error_errno(_("cannot unlink '%s'"), filename);
                goto fail_exit;
        }
  
         * the postimage.
         */
        handle_cache(path, sha1, rerere_path(id, "preimage"));
-       fprintf(stderr, "Updated preimage for '%s'\n", path);
+       fprintf_ln(stderr, _("Updated preimage for '%s'"), path);
  
        /*
         * And remember that we can record resolution for this
        item = string_list_insert(rr, path);
        free_rerere_id(item);
        item->util = id;
-       fprintf(stderr, "Forgot resolution for %s\n", path);
+       fprintf(stderr, _("Forgot resolution for '%s'\n"), path);
        return 0;
  
  fail_exit:
@@@ -1105,7 -1092,7 +1094,7 @@@ int rerere_forget(struct pathspec *path
        struct string_list merge_rr = STRING_LIST_INIT_DUP;
  
        if (read_cache() < 0)
-               return error("Could not read index");
+               return error(_("index file corrupt"));
  
        fd = setup_rerere(&merge_rr, RERERE_NOAUTOUPDATE);
        if (fd < 0)
        find_conflict(&conflict);
        for (i = 0; i < conflict.nr; i++) {
                struct string_list_item *it = &conflict.items[i];
 -              if (!match_pathspec(pathspec, it->string,
 +              if (!match_pathspec(&the_index, pathspec, it->string,
                                    strlen(it->string), 0, NULL, 0))
                        continue;
                rerere_forget_one_path(it->string, &merge_rr);
@@@ -1193,7 -1180,7 +1182,7 @@@ void rerere_gc(struct string_list *rr
        git_config(git_default_config, NULL);
        dir = opendir(git_path("rr-cache"));
        if (!dir)
-               die_errno("unable to open rr-cache directory");
+               die_errno(_("unable to open rr-cache directory"));
        /* Collect stale conflict IDs ... */
        while ((e = readdir(dir))) {
                struct rerere_dir *rr_dir;
@@@ -1247,6 -1234,6 +1236,6 @@@ void rerere_clear(struct string_list *m
                        rmdir(rerere_path(id, NULL));
                }
        }
 -      unlink_or_warn(git_path_merge_rr());
 +      unlink_or_warn(git_path_merge_rr(the_repository));
        rollback_lock_file(&write_lock);
  }
diff --combined t/t4200-rerere.sh
index 65da74c76683c0ccd109fdec63766ab6bdfb08f0,819f6dd67291146a8fbc047399573ed151f8f465..428b3c1e9e85a25dab044ae833bf469158c28057
@@@ -267,7 -267,8 +267,7 @@@ rerere_gc_custom_expiry_test () 
                git -c "gc.rerereresolved=$right_now" \
                    -c "gc.rerereunresolved=$right_now" rerere gc &&
                find .git/rr-cache -type f | sort >actual &&
 -              >expect &&
 -              test_cmp expect actual
 +              test_must_be_empty actual
        '
  }
  
@@@ -535,8 -536,9 +535,8 @@@ test_expect_success 'multiple identica
  
        # We resolved file1 and file2
        git rerere &&
 -      >expect &&
        git rerere remaining >actual &&
 -      test_cmp expect actual &&
 +      test_must_be_empty actual &&
  
        # We must have recorded both of them
        count_pre_post 2 2 &&
        test_must_fail git merge six.1 &&
        git rerere &&
  
 -      >expect &&
        git rerere remaining >actual &&
 -      test_cmp expect actual &&
 +      test_must_be_empty actual &&
  
        concat_insert short 6.1 6.2 >file1.expect &&
        concat_insert long 6.1 6.2 >file2.expect &&
        count_pre_post 0 0
  '
  
+ test_expect_success 'rerere with unexpected conflict markers does not crash' '
+       git reset --hard &&
+       git checkout -b branch-1 master &&
+       echo "bar" >test &&
+       git add test &&
+       git commit -q -m two &&
+       git reset --hard &&
+       git checkout -b branch-2 master &&
+       echo "foo" >test &&
+       git add test &&
+       git commit -q -a -m one &&
+       test_must_fail git merge branch-1 &&
+       echo "<<<<<<< a" >test &&
+       git rerere &&
+       git rerere clear
+ '
+ test_expect_success 'rerere with inner conflict markers' '
+       git reset --hard &&
+       git checkout -b A master &&
+       echo "bar" >test &&
+       git add test &&
+       git commit -q -m two &&
+       echo "baz" >test &&
+       git add test &&
+       git commit -q -m three &&
+       git reset --hard &&
+       git checkout -b B master &&
+       echo "foo" >test &&
+       git add test &&
+       git commit -q -a -m one &&
+       test_must_fail git merge A~ &&
+       git add test &&
+       git commit -q -m "will solve conflicts later" &&
+       test_must_fail git merge A &&
+       echo "resolved" >test &&
+       git add test &&
+       git commit -q -m "solved conflict" &&
+       echo "resolved" >expect &&
+       git reset --hard HEAD~~ &&
+       test_must_fail git merge A~ &&
+       git add test &&
+       git commit -q -m "will solve conflicts later" &&
+       test_must_fail git merge A &&
+       cat test >actual &&
+       test_cmp expect actual &&
+       git add test &&
+       git commit -m "rerere solved conflict" &&
+       git reset --hard HEAD~ &&
+       test_must_fail git merge A &&
+       cat test >actual &&
+       test_cmp expect actual
+ '
  test_done