Merge branch 'jh/notes-merge-in-git-dir-worktree'

Running "notes merge --commit" failed to perform correctly when run
from any directory inside $GIT_DIR/.  When "notes merge" stops with
conflicts, $GIT_DIR/NOTES_MERGE_WORKTREE is the place a user edits
to resolve it.

By Johan Herland (3) and Junio C Hamano (1)
* jh/notes-merge-in-git-dir-worktree:
  notes-merge: Don't remove .git/NOTES_MERGE_WORKTREE; it may be the user's cwd
  notes-merge: use opendir/readdir instead of using read_directory()
  t3310: illustrate failure to "notes merge --commit" inside $GIT_DIR/
  remove_dir_recursively(): Add flag for skipping removal of toplevel dir

diff --git a/dir.c b/dir.c
index 0a78d00b545ac4f302ea89b6393773669907599e..2087d23b6bce5280e54770923cb7b7274361d667 100644 (file)
--- a/dir.c
+++ b/dir.c
@@ -1178,6 +1178,7 @@ int remove_dir_recursively(struct strbuf *path, int flag)
        struct dirent *e;
        int ret = 0, original_len = path->len, len;
        int only_empty = (flag & REMOVE_DIR_EMPTY_ONLY);
+       int keep_toplevel = (flag & REMOVE_DIR_KEEP_TOPLEVEL);
        unsigned char submodule_head[20];
 
        if ((flag & REMOVE_DIR_KEEP_NESTED_GIT) &&
@@ -1185,9 +1186,14 @@ int remove_dir_recursively(struct strbuf *path, int flag)
                /* Do not descend and nuke a nested git work tree. */
                return 0;
 
+       flag &= ~(REMOVE_DIR_KEEP_TOPLEVEL|REMOVE_DIR_KEEP_NESTED_GIT);
        dir = opendir(path->buf);
-       if (!dir)
-               return rmdir(path->buf);
+       if (!dir) {
+               if (!keep_toplevel)
+                       return rmdir(path->buf);
+               else
+                       return -1;
+       }
        if (path->buf[original_len - 1] != '/')
                strbuf_addch(path, '/');
 
@@ -1202,7 +1208,7 @@ int remove_dir_recursively(struct strbuf *path, int flag)
                if (lstat(path->buf, &st))
                        ; /* fall thru */
                else if (S_ISDIR(st.st_mode)) {
-                       if (!remove_dir_recursively(path, only_empty))
+                       if (!remove_dir_recursively(path, flag))
                                continue; /* happy */
                } else if (!only_empty && !unlink(path->buf))
                        continue; /* happy, too */
@@ -1214,7 +1220,7 @@ int remove_dir_recursively(struct strbuf *path, int flag)
        closedir(dir);
 
        strbuf_setlen(path, original_len);
-       if (!ret)
+       if (!ret && !keep_toplevel)
                ret = rmdir(path->buf);
        return ret;
 }

diff --git a/dir.h b/dir.h
index dd6947e1d46098732ff1d8c3f23087c1e7163fe9..58b6fc7c86df1bd5ac6f072672027a1474732ac6 100644 (file)
--- a/dir.h
+++ b/dir.h
@@ -102,6 +102,7 @@ extern void setup_standard_excludes(struct dir_struct *dir);
 
 #define REMOVE_DIR_EMPTY_ONLY 01
 #define REMOVE_DIR_KEEP_NESTED_GIT 02
+#define REMOVE_DIR_KEEP_TOPLEVEL 04
 extern int remove_dir_recursively(struct strbuf *path, int flag);
 
 /* tries to remove the path with empty directories along it, ignores ENOENT */

diff --git a/notes-merge.c b/notes-merge.c
index fb0832f97d218ecd1812361721800d6288935c06..74aa77ce4be2bf23387a32e42cbdc72154c529c2 100644 (file)
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -267,7 +267,8 @@ static void check_notes_merge_worktree(struct notes_merge_options *o)
                 * Must establish NOTES_MERGE_WORKTREE.
                 * Abort if NOTES_MERGE_WORKTREE already exists
                 */
-               if (file_exists(git_path(NOTES_MERGE_WORKTREE))) {
+               if (file_exists(git_path(NOTES_MERGE_WORKTREE)) &&
+                   !is_empty_dir(git_path(NOTES_MERGE_WORKTREE))) {
                        if (advice_resolve_conflict)
                                die("You have not concluded your previous "
                                    "notes merge (%s exists).\nPlease, use "
@@ -687,51 +688,60 @@ int notes_merge_commit(struct notes_merge_options *o,
 {
        /*
         * Iterate through files in .git/NOTES_MERGE_WORKTREE and add all
-        * found notes to 'partial_tree'. Write the updates notes tree to
+        * found notes to 'partial_tree'. Write the updated notes tree to
         * the DB, and commit the resulting tree object while reusing the
         * commit message and parents from 'partial_commit'.
         * Finally store the new commit object SHA1 into 'result_sha1'.
         */
-       struct dir_struct dir;
-       char *path = xstrdup(git_path(NOTES_MERGE_WORKTREE "/"));
-       int path_len = strlen(path), i;
+       DIR *dir;
+       struct dirent *e;
+       struct strbuf path = STRBUF_INIT;
        char *msg = strstr(partial_commit->buffer, "\n\n");
        struct strbuf sb_msg = STRBUF_INIT;
+       int baselen;
 
+       strbuf_addstr(&path, git_path(NOTES_MERGE_WORKTREE));
        if (o->verbosity >= 3)
-               printf("Committing notes in notes merge worktree at %.*s\n",
-                       path_len - 1, path);
+               printf("Committing notes in notes merge worktree at %s\n",
+                       path.buf);
 
        if (!msg || msg[2] == '\0')
                die("partial notes commit has empty message");
        msg += 2;
 
-       memset(&dir, 0, sizeof(dir));
-       read_directory(&dir, path, path_len, NULL);
-       for (i = 0; i < dir.nr; i++) {
-               struct dir_entry *ent = dir.entries[i];
+       dir = opendir(path.buf);
+       if (!dir)
+               die_errno("could not open %s", path.buf);
+
+       strbuf_addch(&path, '/');
+       baselen = path.len;
+       while ((e = readdir(dir)) != NULL) {
                struct stat st;
-               const char *relpath = ent->name + path_len;
                unsigned char obj_sha1[20], blob_sha1[20];
 
-               if (ent->len - path_len != 40 || get_sha1_hex(relpath, obj_sha1)) {
+               if (is_dot_or_dotdot(e->d_name))
+                       continue;
+
+               if (strlen(e->d_name) != 40 || get_sha1_hex(e->d_name, obj_sha1)) {
                        if (o->verbosity >= 3)
-                               printf("Skipping non-SHA1 entry '%s'\n",
-                                                               ent->name);
+                               printf("Skipping non-SHA1 entry '%s%s'\n",
+                                       path.buf, e->d_name);
                        continue;
                }
 
+               strbuf_addstr(&path, e->d_name);
                /* write file as blob, and add to partial_tree */
-               if (stat(ent->name, &st))
-                       die_errno("Failed to stat '%s'", ent->name);
-               if (index_path(blob_sha1, ent->name, &st, HASH_WRITE_OBJECT))
-                       die("Failed to write blob object from '%s'", ent->name);
+               if (stat(path.buf, &st))
+                       die_errno("Failed to stat '%s'", path.buf);
+               if (index_path(blob_sha1, path.buf, &st, HASH_WRITE_OBJECT))
+                       die("Failed to write blob object from '%s'", path.buf);
                if (add_note(partial_tree, obj_sha1, blob_sha1, NULL))
                        die("Failed to add resolved note '%s' to notes tree",
-                           ent->name);
+                           path.buf);
                if (o->verbosity >= 4)
                        printf("Added resolved note for object %s: %s\n",
                                sha1_to_hex(obj_sha1), sha1_to_hex(blob_sha1));
+               strbuf_setlen(&path, baselen);
        }
 
        strbuf_attach(&sb_msg, msg, strlen(msg), strlen(msg) + 1);
@@ -740,20 +750,25 @@ int notes_merge_commit(struct notes_merge_options *o,
        if (o->verbosity >= 4)
                printf("Finalized notes merge commit: %s\n",
                        sha1_to_hex(result_sha1));
-       free(path);
+       strbuf_release(&path);
+       closedir(dir);
        return 0;
 }
 
 int notes_merge_abort(struct notes_merge_options *o)
 {
-       /* Remove .git/NOTES_MERGE_WORKTREE directory and all files within */
+       /*
+        * Remove all files within .git/NOTES_MERGE_WORKTREE. We do not remove
+        * the .git/NOTES_MERGE_WORKTREE directory itself, since it might be
+        * the current working directory of the user.
+        */
        struct strbuf buf = STRBUF_INIT;
        int ret;
 
        strbuf_addstr(&buf, git_path(NOTES_MERGE_WORKTREE));
        if (o->verbosity >= 3)
-               printf("Removing notes merge worktree at %s\n", buf.buf);
-       ret = remove_dir_recursively(&buf, 0);
+               printf("Removing notes merge worktree at %s/*\n", buf.buf);
+       ret = remove_dir_recursively(&buf, REMOVE_DIR_KEEP_TOPLEVEL);
        strbuf_release(&buf);
        return ret;
 }

diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index 436719795376f78e3a32a441e9e7e0a4606ac2f5..195bb97f859d6a4990c292da46b9674be0f6153f 100755 (executable)
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -324,7 +324,7 @@ y and z notes on 4th commit
 EOF
        git notes merge --commit &&
        # No .git/NOTES_MERGE_* files left
-       test_must_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
+       test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
        test_cmp /dev/null output &&
        # Merge commit has pre-merge y and pre-merge z as parents
        test "$(git rev-parse refs/notes/m^1)" = "$(cat pre_merge_y)" &&
@@ -386,7 +386,7 @@ test_expect_success 'redo merge of z into m (== y) with default ("manual") resol
 test_expect_success 'abort notes merge' '
        git notes merge --abort &&
        # No .git/NOTES_MERGE_* files left
-       test_must_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
+       test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
        test_cmp /dev/null output &&
        # m has not moved (still == y)
        test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)" &&
@@ -453,7 +453,7 @@ EOF
        # Finalize merge
        git notes merge --commit &&
        # No .git/NOTES_MERGE_* files left
-       test_must_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
+       test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
        test_cmp /dev/null output &&
        # Merge commit has pre-merge y and pre-merge z as parents
        test "$(git rev-parse refs/notes/m^1)" = "$(cat pre_merge_y)" &&
@@ -542,7 +542,7 @@ EOF
 test_expect_success 'resolve situation by aborting the notes merge' '
        git notes merge --abort &&
        # No .git/NOTES_MERGE_* files left
-       test_must_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
+       test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
        test_cmp /dev/null output &&
        # m has not moved (still == w)
        test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)" &&
@@ -553,4 +553,23 @@ test_expect_success 'resolve situation by aborting the notes merge' '
        verify_notes z
 '
 
+cat >expect_notes <<EOF
+foo
+bar
+EOF
+
+test_expect_success 'switch cwd before committing notes merge' '
+       git notes add -m foo HEAD &&
+       git notes --ref=other add -m bar HEAD &&
+       test_must_fail git notes merge refs/notes/other &&
+       (
+               cd .git/NOTES_MERGE_WORKTREE &&
+               echo "foo" > $(git rev-parse HEAD) &&
+               echo "bar" >> $(git rev-parse HEAD) &&
+               git notes merge --commit
+       ) &&
+       git notes show HEAD > actual_notes &&
+       test_cmp expect_notes actual_notes
+'
+
 test_done