problem with merging notes.

problem with merging notes.

[David Bremner]

Hi;

It seems to me there is a bug (or at least some missing documentation)
for git notes merge --commit. If the working directory is
.git/NOTES_MERGE_WORKTREE, then all notes for the conflicting commit are
silently dropped.

A test script follows. Have I missed something?  

David

P.S. Please CC me, I'm not subscribed.


tmpdir=$(mktemp -d)
cd $tmpdir
git init
git commit --allow-empty -m'empty commit'
git notes add -m 'foo' HEAD
git notes --ref=other add -m 'bar' HEAD 
git notes merge refs/notes/other
cd .git/NOTES_MERGE_WORKTREE
echo "foo\nbar\n"> $(git rev-parse HEAD)
git notes merge --commit
cd ../..
git notes list | wc -l

Re: problem with merging notes.

[Michael Schubert]

On 03/11/2012 06:17 PM, David Bremner wrote:
> tmpdir=$(mktemp -d)
> cd $tmpdir
> git init
> git commit --allow-empty -m'empty commit'
> git notes add -m 'foo' HEAD
> git notes --ref=other add -m 'bar' HEAD 
> git notes merge refs/notes/other
> cd .git/NOTES_MERGE_WORKTREE
> echo "foo\nbar\n"> $(git rev-parse HEAD)
> git notes merge --commit

cd back to your worktree *before* you call
"git notes merge --commit" and it will work as expected.

Apparently, there's some path issue and git notes just
continues doing half of its job. I'll check later.

> cd ../..
> git notes list | wc -l

Thanks.

[PATCH 2/2] notes-merge: use opendir/readdir instead of using read_directory()

[Johan Herland]

notes_merge_commit() only needs to list all entries (non-recursively)
under a directory, which can be easily accomplished with
opendir/readdir and would be more lightweight than read_directory().

read_directory() is designed to list paths inside a working
directory. Using it outside of its scope may lead to undesired effects.

Apparently, one of the undesired effects of read_directory() is that it
doesn't deal with being given absolute paths. This creates problems for
notes_merge_commit() when git_path() returns an absolute path, which
happens when the current working directory is in a subdirectory of the
.git directory.

Originally-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Updated-by:  Johan Herland <johan@herland.net>
Signed-off-by: Johan Herland <johan@herland.net>
---

This is a resurrection of pclouds' patch 2/11 in a patch series sent
last October for rewriting read_directory(). This patch doesn't
actually touch read_directory(), but instead rewrites
notes_merge_commit() to use opendir()/readdir() instead of
read_directory(). Since the usage of read_directory() is what caused
the bug that David found (in the previous patch), this rewrite happens
to fix that bug as well.


Have fun! :)

...Johan

 notes-merge.c                         |   50 ++++++++++++++++++++-------------
 t/t3310-notes-merge-manual-resolve.sh |    2 +-
 2 files changed, 31 insertions(+), 21 deletions(-)

diff --git a/notes-merge.c b/notes-merge.c
index fb0832f..3a16af2 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -687,51 +687,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,7 +749,8 @@ 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;
 }

diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index 0c531c3..d6d6ac6 100755
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -558,7 +558,7 @@ foo
 bar
 EOF

-test_expect_failure 'switch cwd before committing notes merge' '
+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 &&
--
1.7.9.2

Re: [PATCH 2/2] notes-merge: use opendir/readdir instead of using read_directory()

[Nguyen Thai Ngoc Duy]

On Mon, Mar 12, 2012 at 9:47 PM, Johan Herland <johan@herland.net> wrote:
> This is a resurrection of pclouds' patch 2/11 in a patch series sent
> last October for rewriting read_directory(). This patch doesn't
> actually touch read_directory(), but instead rewrites
> notes_merge_commit() to use opendir()/readdir() instead of
> read_directory(). Since the usage of read_directory() is what caused
> the bug that David found (in the previous patch), this rewrite happens
> to fix that bug as well.

Happy to help. And that reminds me I've got to revive the
read_directory() rewrite soon. Got stuck at git-add because I aimed
too high. Gaah...
-- 
Duy

[PATCH 1/2] t3310: Add testcase demonstrating failure to --commit from within another dir

[Johan Herland]

Found-by: David Bremner <david@tethera.net>
Signed-off-by: Johan Herland <johan@herland.net>
---

(sending again in the correct thread. Sorry for the screwup.)

This is a transcription of David's test script into a git test case.

Thanks to David for finding this issue.


Have fun! :)

...Johan

 t/t3310-notes-merge-manual-resolve.sh |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index 4367197..0c531c3 100755
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -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_failure '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
--
1.7.9.2

[PATCH 2/2] notes-merge: use opendir/readdir instead of using read_directory()

[Johan Herland]

notes_merge_commit() only needs to list all entries (non-recursively)
under a directory, which can be easily accomplished with
opendir/readdir and would be more lightweight than read_directory().

read_directory() is designed to list paths inside a working
directory. Using it outside of its scope may lead to undesired effects.

Apparently, one of the undesired effects of read_directory() is that it
doesn't deal with being given absolute paths. This creates problems for
notes_merge_commit() when git_path() returns an absolute path, which
happens when the current working directory is in a subdirectory of the
.git directory.

Originally-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Updated-by:  Johan Herland <johan@herland.net>
Signed-off-by: Johan Herland <johan@herland.net>
---

(sending again in the correct thread. Sorry for the screwup.)

This is a resurrection of pclouds' patch 2/11 in a patch series sent
last October for rewriting read_directory(). This patch doesn't
actually touch read_directory(), but instead rewrites
notes_merge_commit() to use opendir()/readdir() instead of
read_directory(). Since the usage of read_directory() is what caused
the bug that David found (in the previous patch), this rewrite happens
to fix that bug as well.


Have fun! :)

...Johan

 notes-merge.c                         |   50 ++++++++++++++++++++-------------
 t/t3310-notes-merge-manual-resolve.sh |    2 +-
 2 files changed, 31 insertions(+), 21 deletions(-)

diff --git a/notes-merge.c b/notes-merge.c
index fb0832f..3a16af2 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -687,51 +687,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,7 +749,8 @@ 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;
 }

diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index 0c531c3..d6d6ac6 100755
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -558,7 +558,7 @@ foo
 bar
 EOF

-test_expect_failure 'switch cwd before committing notes merge' '
+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 &&
--
1.7.9.2

Re: [PATCH 1/2] t3310: Add testcase demonstrating failure to --commit from within another dir

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> Found-by: David Bremner <david@tethera.net>
> Signed-off-by: Johan Herland <johan@herland.net>

Could you clarify what "from within another dir" means on the subject?

What was the expected usage?

	The 'git notes merge' command expected to be run from the
        working tree of the project being annotated, and did not
        anticipate getting run inside $GIT_DIR/.  However, because
        we use $GIT_DIR/NOTES_MERGE_WORKTREE as a temporary working
        space for the user to work on resolving conflicts, it is not
        unreasonable for a user to run "git notes merge --commit"
        there.

Is that the issue?

> ---
>
> (sending again in the correct thread. Sorry for the screwup.)
>
> This is a transcription of David's test script into a git test case.
>
> Thanks to David for finding this issue.
>
>
> Have fun! :)
>
> ...Johan
>
>  t/t3310-notes-merge-manual-resolve.sh |   19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
> index 4367197..0c531c3 100755
> --- a/t/t3310-notes-merge-manual-resolve.sh
> +++ b/t/t3310-notes-merge-manual-resolve.sh
> @@ -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_failure '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
> --
> 1.7.9.2

Re: [PATCH 1/2] t3310: Add testcase demonstrating failure to --commit from within another dir

[Johan Herland]

On Mon, Mar 12, 2012 at 19:21, Junio C Hamano <gitster@pobox.com> wrote:
> Johan Herland <johan@herland.net> writes:
>
>> Found-by: David Bremner <david@tethera.net>
>> Signed-off-by: Johan Herland <johan@herland.net>
>
> Could you clarify what "from within another dir" means on the subject?
>
> What was the expected usage?
>
>        The 'git notes merge' command expected to be run from the
>        working tree of the project being annotated, and did not
>        anticipate getting run inside $GIT_DIR/.  However, because
>        we use $GIT_DIR/NOTES_MERGE_WORKTREE as a temporary working
>        space for the user to work on resolving conflicts, it is not
>        unreasonable for a user to run "git notes merge --commit"
>        there.
>
> Is that the issue?

That is exactly the issue. Thanks for the clear wording.

Feel free to update the commit message accordingly.


...Johan

>> ---
>>
>> (sending again in the correct thread. Sorry for the screwup.)
>>
>> This is a transcription of David's test script into a git test case.
>>
>> Thanks to David for finding this issue.
>>
>>
>> Have fun! :)
>>
>> ...Johan
>>
>>  t/t3310-notes-merge-manual-resolve.sh |   19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
>> index 4367197..0c531c3 100755
>> --- a/t/t3310-notes-merge-manual-resolve.sh
>> +++ b/t/t3310-notes-merge-manual-resolve.sh
>> @@ -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_failure '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
>> --
>> 1.7.9.2



-- 
Johan Herland, <johan@herland.net>
www.herland.net