[PATCH v2 00/10] rerere: handle nested conflicts

[Thomas Gummerer <t.gummerer@gmail.com>]

The previous round was at
<20180520211210.1248-1-t.gummerer@gmail.com>.

Thanks Junio for the comments on the previous round.

Changes since v2:
 - lowercase the first letter in some error/warning messages before
   marking them for translation
 - wrap paths in output in single quotes, for consistency, and to make
   some of the messages the same as ones that are already translated
 - mark messages in builtin/rerere.c for translation as well, which I
   had previously forgotten.
 - expanded the technical documentation on rerere.  The entire
   document is basically rewritten.
 - changed the test in 6/10 to just fake a conflict marker inside of
   one of the hunks instead of using an inner conflict created by a
   merge.  This is to make sure the codepath is still hit after we
   handle inner conflicts properly.
 - added tests for handling inner conflict markers
 - added one commit to recalculate the conflict ID when an unresolved
   conflict is committed, and the subsequent operation conflicts again
   in the same file.  More explanation in the commit message of that
   commit.

range-diff below.  A few commits changed enough for range-diff
to give up showing the differences in those, they are probably best
reviewed as the whole patch anyway:

1:  901b638400 ! 1:  2825342cc2 rerere: unify error message when read_cache fails
    @@ -1,6 +1,6 @@
     Author: Thomas Gummerer <t.gummerer@gmail.com>
     
    -    rerere: unify error message when read_cache fails
    +    rerere: unify error messages when read_cache fails
     
         We have multiple different variants of the error message we show to
         the user if 'read_cache' fails.  The "Could not read index" variant we
-:  ---------- > 2:  d1500028aa rerere: lowercase error messages
-:  ---------- > 3:  ed3601ee71 rerere: wrap paths in output in sq
2:  c48ffededd ! 4:  6ead84a199 rerere: mark strings for translation
    @@ -9,6 +9,28 @@
     
         Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
     
    +diff --git a/builtin/rerere.c b/builtin/rerere.c
    +--- a/builtin/rerere.c
    ++++ b/builtin/rerere.c
    +@@
    + 	if (!strcmp(argv[0], "forget")) {
    + 		struct pathspec pathspec;
    + 		if (argc < 2)
    +-			warning("'git rerere forget' without paths is deprecated");
    ++			warning(_("'git rerere forget' without paths is deprecated"));
    + 		parse_pathspec(&pathspec, 0, PATHSPEC_PREFER_CWD,
    + 			       prefix, argv + 1);
    + 		return rerere_forget(&pathspec);
    +@@
    + 			const char *path = merge_rr.items[i].string;
    + 			const struct rerere_id *id = merge_rr.items[i].util;
    + 			if (diff_two(rerere_path(id, "preimage"), path, path, path))
    +-				die("unable to generate diff for '%s'", rerere_path(id, NULL));
    ++				die(_("unable to generate diff for '%s'"), rerere_path(id, NULL));
    + 		}
    + 	} else
    + 		usage_with_options(rerere_usage, options);
    +
     diff --git a/rerere.c b/rerere.c
     --- a/rerere.c
     +++ b/rerere.c
    @@ -53,14 +75,14 @@
      	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);
    +-		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);
    +-			error_errno("could not write '%s'", output);
    ++			error_errno(_("could not write '%s'"), output);
      			fclose(io.input);
      			return -1;
      		}
    @@ -68,18 +90,18 @@
      
      	fclose(io.input);
      	if (io.io.wrerror)
    --		error("There were errors while writing %s (%s)",
    -+		error(_("There were errors while writing %s (%s)"),
    +-		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);
    +-		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 (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);
    +-		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;
    @@ -105,21 +127,21 @@
      	 * 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"),
    +-		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);
    +-		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);
    +-		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);
    +-		return error_errno("writing '%s' failed", path);
    ++		return error_errno(_("writing '%s' failed"), path);
      
      out:
      	free(cur.ptr);
    @@ -134,8 +156,8 @@
      
      	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"));
    +-		die("unable to write new index file");
    ++		die(_("unable to write new index file"));
      }
      
      static void remove_variant(struct rerere_id *id)
    @@ -179,8 +201,8 @@
      		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());
    +-		die("could not create directory '%s'", git_path_rr_cache());
    ++		die(_("could not create directory '%s'"), git_path_rr_cache());
      	return 1;
      }
      
    @@ -188,8 +210,8 @@
      	 */
      	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);
    +-		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);
    @@ -214,11 +236,11 @@
      	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);
    +-			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);
    +-			error_errno("cannot unlink '%s'", filename);
    ++			error_errno(_("cannot unlink '%s'"), filename);
      		goto fail_exit;
      	}
      
    @@ -235,8 +257,8 @@
      	item = string_list_insert(rr, path);
      	free_rerere_id(item);
      	item->util = id;
    --	fprintf(stderr, "Forgot resolution for %s\n", path);
    -+	fprintf_ln(stderr, _("Forgot resolution for %s"), path);
    +-	fprintf(stderr, "Forgot resolution for '%s'\n", path);
    ++	fprintf(stderr, _("Forgot resolution for '%s'\n"), path);
      	return 0;
      
      fail_exit:
3:  e29449406f < -:  ---------- rerere: add some documentation
-:  ---------- > 5:  caad276aca rerere: add some documentation
4:  3b41520b28 ! 6:  ad88a6b8a8 rerere: fix crash when conflict goes unresolved
    @@ -23,14 +23,18 @@
         Now when 'rerere clear' for example is run, it will segfault in
         'has_rerere_resolution', because status is NULL.
     
    -    To fix this, remove the rerere ID from the MERGE_RR file in case we
    -    can't handle it, and remove the folder for the ID.  Removing it
    -    unconditionally is fine here, because if the user would have resolved
    -    the conflict and ran rerere, the entry would no longer be in the
    -    MERGE_RR file, so we wouldn't have this problem in the first place,
    -    while if the conflict was not resolved, the only thing that's left in
    -    the folder is the 'preimage', which by itself will be regenerated by
    -    git if necessary, so the user won't loose any work.
    +    To fix this, remove the rerere ID from the MERGE_RR file in the case
    +    when we can't handle it, and remove the corresponding variant from
    +    .git/rr-cache/.  Removing it unconditionally is fine here, because if
    +    the user would have resolved the conflict and ran rerere, the entry
    +    would no longer be in the MERGE_RR file, so we wouldn't have this
    +    problem in the first place, while if the conflict was not resolved,
    +    the only thing that's left in the folder is the 'preimage', which by
    +    itself will be regenerated by git if necessary, so the user won't
    +    loose any work.
    +
    +    Note that other variants that have the same conflict ID will not be
    +    touched.
     
         Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
     
    @@ -71,16 +75,13 @@
      	count_pre_post 0 0
      '
      
    -+test_expect_success 'rerere with extra conflict markers keeps working' '
    ++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 &&
    -+	echo "baz" >test &&
    -+	git add test &&
    -+	git commit -q -m three &&
     +
     +	git reset --hard &&
     +	git checkout -b branch-2 master &&
    @@ -88,10 +89,10 @@
     +	git add test &&
     +	git commit -q -a -m one &&
     +
    -+	test_must_fail git merge branch-1~ &&
    -+	git add test &&
    -+	git commit -q -m "will solve conflicts later" &&
     +	test_must_fail git merge branch-1 &&
    ++	sed "s/bar/>>>>>>> a/" >test.tmp <test &&
    ++	mv test.tmp test &&
    ++	git rerere &&
     +
     +	git rerere clear
     +'
5:  411a4ee37e ! 7:  15f9efcba6 rerere: only return whether a path has conflicts or not
    @@ -67,13 +67,13 @@
      	if (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;
6:  fc9f715913 = 8:  1490efaad3 rerere: factor out handle_conflict function
7:  f7dea09a0a < -:  ---------- rerere: teach rerere to handle nested conflicts
-:  ---------- > 9:  6619650c42 rerere: teach rerere to handle nested conflicts
-:  ---------- > 10:  4b11dce7dd rerere: recalculate conflict ID when unresolved conflict is committed

Thomas Gummerer (10):
  rerere: unify error messages when read_cache fails
  rerere: lowercase error messages
  rerere: wrap paths in output in sq
  rerere: mark strings for translation
  rerere: add some documentation
  rerere: fix crash when conflict goes unresolved
  rerere: only return whether a path has conflicts or not
  rerere: factor out handle_conflict function
  rerere: teach rerere to handle nested conflicts
  rerere: recalculate conflict ID when unresolved conflict is committed

 Documentation/technical/rerere.txt | 182 +++++++++++++++++++++
 builtin/rerere.c                   |   4 +-
 rerere.c                           | 246 ++++++++++++++---------------
 t/t4200-rerere.sh                  |  67 ++++++++
 4 files changed, 372 insertions(+), 127 deletions(-)
 create mode 100644 Documentation/technical/rerere.txt

-- 
2.18.0.rc1.242.g61856ae69