[PATCH] fast-import: properly fanout notes when tree is imported

[PATCH] fast-import: properly fanout notes when tree is imported

[Mike Hommey]

In typical uses of fast-import, trees are inherited from a parent
commit. In that case, the tree_entry for the branch looks like:

  .versions[1].sha1 = $some_sha1
  .tree = <tree structure loaded from $some_sha1>

However, when trees are imported, rather than inherited, that is not the
case. One can import a tree with a filemodify command, replacing the
root tree object.

e.g.
  "M 040000 $some_sha1 \n"

In this case, the tree_entry for the branch looks like:

  .versions[1].sha1 = $some_sha1
  .tree = NULL

When adding new notes with the notemodify command, do_change_note_fanout
is called to get a notes count, and to do so, it loops over the
tree_entry->tree, but doesn't do anything when the tree is NULL.

In the latter case above, it means do_change_note_fanout thinks the tree
contains no note, and new notes are added with no fanout.

Interestingly, do_change_note_fanout does check whether subdirectories
have a NULL .tree, in which case it uses load_tree(). Which means the
right behaviour happens when using the filemodify command to import
subdirectories.

This change makes do_change_note_fanount call load_tree() whenever the
tree_entry it is given has no tree loaded, making all cases handled
equally.

Signed-off-by: Mike Hommey <mh@glandium.org>
---

This is something I should have submitted a patch for a long time ago, back
when this was discussed in the thread starting from
https://www.spinics.net/lists/git/msg242426.html. The message most relevant to
this patch in the thread is https://www.spinics.net/lists/git/msg242448.html.

 fast-import.c                |  8 +++++---
 t/t9301-fast-import-notes.sh | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index cb545d7df5..5e528b1999 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2220,13 +2220,17 @@ static uintmax_t do_change_note_fanout(
 		char *fullpath, unsigned int fullpath_len,
 		unsigned char fanout)
 {
-	struct tree_content *t = root->tree;
+	struct tree_content *t;
 	struct tree_entry *e, leaf;
 	unsigned int i, tmp_hex_sha1_len, tmp_fullpath_len;
 	uintmax_t num_notes = 0;
 	unsigned char sha1[20];
 	char realpath[60];
 
+	if (!root->tree)
+		load_tree(root);
+	t = root->tree;
+
 	for (i = 0; t && i < t->entry_count; i++) {
 		e = t->entries[i];
 		tmp_hex_sha1_len = hex_sha1_len + e->name->str_len;
@@ -2278,8 +2282,6 @@ static uintmax_t do_change_note_fanout(
 				leaf.tree);
 		} else if (S_ISDIR(e->versions[1].mode)) {
 			/* This is a subdir that may contain note entries */
-			if (!e->tree)
-				load_tree(e);
 			num_notes += do_change_note_fanout(orig_root, e,
 				hex_sha1, tmp_hex_sha1_len,
 				fullpath, tmp_fullpath_len, fanout);
diff --git a/t/t9301-fast-import-notes.sh b/t/t9301-fast-import-notes.sh
index 83acf68bc3..b408b2b32d 100755
--- a/t/t9301-fast-import-notes.sh
+++ b/t/t9301-fast-import-notes.sh
@@ -483,6 +483,47 @@ test_expect_success 'verify that lots of notes trigger a fanout scheme' '
 
 '
 
+# Create another notes tree from the one above
+cat >>input <<INPUT_END
+commit refs/heads/other_commits
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+commit #$(($num_commit + 1))
+COMMIT
+
+from refs/heads/many_commits
+M 644 inline file
+data <<EOF
+file contents in commit #$(($num_commit + 1))
+EOF
+
+commit refs/notes/other_notes
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+committing one more note on a tree imported from a previous notes tree
+COMMIT
+
+M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) 
+N inline :$(($num_commit + 1))
+data <<EOF
+note for commit #$(($num_commit + 1))
+EOF
+INPUT_END
+
+test_expect_success 'verify that importing a notes tree respects the fanout scheme' '
+	git fast-import <input &&
+
+	# None of the entries in the top-level notes tree should be a full SHA1
+	git ls-tree --name-only refs/notes/other_notes |
+	while read path
+	do
+		if test $(expr length "$path") -ge 40
+		then
+			return 1
+		fi
+	done
+'
+
 cat >>expect_non-note1 << EOF
 This is not a note, but rather a regular file residing in a notes tree
 EOF
-- 
2.11.0.dirty

Re: [PATCH] fast-import: properly fanout notes when tree is imported

[Johan Herland]

On Mon, Dec 19, 2016 at 3:12 AM, Mike Hommey <mh@glandium.org> wrote:
> In typical uses of fast-import, trees are inherited from a parent
> commit. In that case, the tree_entry for the branch looks like:
>
>   .versions[1].sha1 = $some_sha1
>   .tree = <tree structure loaded from $some_sha1>
>
> However, when trees are imported, rather than inherited, that is not the
> case. One can import a tree with a filemodify command, replacing the
> root tree object.
>
> e.g.
>   "M 040000 $some_sha1 \n"
>
> In this case, the tree_entry for the branch looks like:
>
>   .versions[1].sha1 = $some_sha1
>   .tree = NULL
>
> When adding new notes with the notemodify command, do_change_note_fanout
> is called to get a notes count, and to do so, it loops over the
> tree_entry->tree, but doesn't do anything when the tree is NULL.
>
> In the latter case above, it means do_change_note_fanout thinks the tree
> contains no note, and new notes are added with no fanout.

s/note,/notes,/

>
> Interestingly, do_change_note_fanout does check whether subdirectories
> have a NULL .tree, in which case it uses load_tree(). Which means the
> right behaviour happens when using the filemodify command to import
> subdirectories.
>
> This change makes do_change_note_fanount call load_tree() whenever the
> tree_entry it is given has no tree loaded, making all cases handled
> equally.
>
> Signed-off-by: Mike Hommey <mh@glandium.org>

Acked-by: Johan Herland <johan@herland.net>

Re: [PATCH] fast-import: properly fanout notes when tree is imported

[Junio C Hamano]

Mike Hommey <mh@glandium.org> writes:

> In typical uses of fast-import, trees are inherited from a parent
> commit. In that case, the tree_entry for the branch looks like:
> ...
> +# Create another notes tree from the one above
> +cat >>input <<INPUT_END
> +...
> +M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) 

There is a trailing SP that cannot be seen by anybody.

Don't do this.  It makes it very easy to miss what is going on and
wastes reviewers' time.

Protect it by doing something like:

	sed -e 's/Z$//' >>input <<INPUT_END
	...
	M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) Z

Thanks.

Re: [PATCH] fast-import: properly fanout notes when tree is imported

[Mike Hommey]

On Tue, Dec 20, 2016 at 11:34:04AM -0800, Junio C Hamano wrote:
> Mike Hommey <mh@glandium.org> writes:
> 
> > In typical uses of fast-import, trees are inherited from a parent
> > commit. In that case, the tree_entry for the branch looks like:
> > ...
> > +# Create another notes tree from the one above
> > +cat >>input <<INPUT_END
> > +...
> > +M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) 
> 
> There is a trailing SP that cannot be seen by anybody.
> 
> Don't do this.  It makes it very easy to miss what is going on and
> wastes reviewers' time.
> 
> Protect it by doing something like:
> 
> 	sed -e 's/Z$//' >>input <<INPUT_END
> 	...
> 	M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) Z

How about
EMPTY=
...
M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) $EMPTY

?

Mike

Re: [PATCH] fast-import: properly fanout notes when tree is imported

[Junio C Hamano]

Mike Hommey <mh@glandium.org> writes:

> On Tue, Dec 20, 2016 at 11:34:04AM -0800, Junio C Hamano wrote:
>> Mike Hommey <mh@glandium.org> writes:
>> 
>> > In typical uses of fast-import, trees are inherited from a parent
>> > commit. In that case, the tree_entry for the branch looks like:
>> > ...
>> > +# Create another notes tree from the one above
>> > +cat >>input <<INPUT_END
>> > +...
>> > +M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) 
>> 
>> There is a trailing SP that cannot be seen by anybody.
>> 
>> Don't do this.  It makes it very easy to miss what is going on and
>> wastes reviewers' time.
>> 
>> Protect it by doing something like:
>> 
>> 	sed -e 's/Z$//' >>input <<INPUT_END
>> 	...
>> 	M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) Z
>
> How about
> EMPTY=
> ...
> M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) $EMPTY
>
> ?

Notice I said "something like" ;-)

I think you are bringing that up to avoid sed, but if you want to go
that route, the long string $EMPTY is distracting, and makes readers
wonder why something that is loud but expands to nothing has to be
there.  It hides the true intention, which is that the SP that comes
before it is the most important thing on that line.

I would think a lot more understandable variant would be to do this
instead:

	SP=" "
	...
	M a lot of garbage $(and command)$SP

[PATCH] fast-import: properly fanout notes when tree is imported

[Mike Hommey]

In typical uses of fast-import, trees are inherited from a parent
commit. In that case, the tree_entry for the branch looks like:

  .versions[1].sha1 = $some_sha1
  .tree = <tree structure loaded from $some_sha1>

However, when trees are imported, rather than inherited, that is not the
case. One can import a tree with a filemodify command, replacing the
root tree object.

e.g.
  "M 040000 $some_sha1 \n"

In this case, the tree_entry for the branch looks like:

  .versions[1].sha1 = $some_sha1
  .tree = NULL

When adding new notes with the notemodify command, do_change_note_fanout
is called to get a notes count, and to do so, it loops over the
tree_entry->tree, but doesn't do anything when the tree is NULL.

In the latter case above, it means do_change_note_fanout thinks the tree
contains no notes, and new notes are added with no fanout.

Interestingly, do_change_note_fanout does check whether subdirectories
have a NULL .tree, in which case it uses load_tree(). Which means the
right behaviour happens when using the filemodify command to import
subdirectories.

This change makes do_change_note_fanount call load_tree() whenever the
tree_entry it is given has no tree loaded, making all cases handled
equally.

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 fast-import.c                |  8 +++++---
 t/t9301-fast-import-notes.sh | 42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index cb545d7df5..5e528b1999 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2220,13 +2220,17 @@ static uintmax_t do_change_note_fanout(
 		char *fullpath, unsigned int fullpath_len,
 		unsigned char fanout)
 {
-	struct tree_content *t = root->tree;
+	struct tree_content *t;
 	struct tree_entry *e, leaf;
 	unsigned int i, tmp_hex_sha1_len, tmp_fullpath_len;
 	uintmax_t num_notes = 0;
 	unsigned char sha1[20];
 	char realpath[60];
 
+	if (!root->tree)
+		load_tree(root);
+	t = root->tree;
+
 	for (i = 0; t && i < t->entry_count; i++) {
 		e = t->entries[i];
 		tmp_hex_sha1_len = hex_sha1_len + e->name->str_len;
@@ -2278,8 +2282,6 @@ static uintmax_t do_change_note_fanout(
 				leaf.tree);
 		} else if (S_ISDIR(e->versions[1].mode)) {
 			/* This is a subdir that may contain note entries */
-			if (!e->tree)
-				load_tree(e);
 			num_notes += do_change_note_fanout(orig_root, e,
 				hex_sha1, tmp_hex_sha1_len,
 				fullpath, tmp_fullpath_len, fanout);
diff --git a/t/t9301-fast-import-notes.sh b/t/t9301-fast-import-notes.sh
index 83acf68bc3..dadc70b7d5 100755
--- a/t/t9301-fast-import-notes.sh
+++ b/t/t9301-fast-import-notes.sh
@@ -483,6 +483,48 @@ test_expect_success 'verify that lots of notes trigger a fanout scheme' '
 
 '
 
+# Create another notes tree from the one above
+SP=" "
+cat >>input <<INPUT_END
+commit refs/heads/other_commits
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+commit #$(($num_commit + 1))
+COMMIT
+
+from refs/heads/many_commits
+M 644 inline file
+data <<EOF
+file contents in commit #$(($num_commit + 1))
+EOF
+
+commit refs/notes/other_notes
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+committing one more note on a tree imported from a previous notes tree
+COMMIT
+
+M 040000 $(git log --no-walk --format=%T refs/notes/many_notes)$SP
+N inline :$(($num_commit + 1))
+data <<EOF
+note for commit #$(($num_commit + 1))
+EOF
+INPUT_END
+
+test_expect_success 'verify that importing a notes tree respects the fanout scheme' '
+	git fast-import <input &&
+
+	# None of the entries in the top-level notes tree should be a full SHA1
+	git ls-tree --name-only refs/notes/other_notes |
+	while read path
+	do
+		if test $(expr length "$path") -ge 40
+		then
+			return 1
+		fi
+	done
+'
+
 cat >>expect_non-note1 << EOF
 This is not a note, but rather a regular file residing in a notes tree
 EOF
-- 
2.11.0

Re: [PATCH] fast-import: properly fanout notes when tree is imported

[Mike Hommey]

Sorry, I forgot the v2 in the subject.

Mike

Re: [PATCH] fast-import: properly fanout notes when tree is imported

[Junio C Hamano]

Mike Hommey <mh@glandium.org> writes:

> In typical uses of fast-import, trees are inherited from a parent
> commit. In that case, the tree_entry for the branch looks like:
> ...
> This change makes do_change_note_fanount call load_tree() whenever the
> tree_entry it is given has no tree loaded, making all cases handled
> equally.

Thanks.