threads / patch / 44733

patchfast-import: properly fanout notes when tree is imported

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

## tl;dr

8 messages between Dec 19, 2016 and Dec 20, 2016. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Mike Hommey· Dec 19, 2016, 02:12 UTC · lore

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(-)
Show changes to 2 files +46 −3

fast-import.c, t/t9301-fast-import-notes.sh

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
Johan Herland· Dec 19, 2016, 22:05 UTC · re: Mike Hommey · lore

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

On Mon, Dec 19, 2016 at 3:12 AM, Mike Hommey <mh@glandium.org> wrote:
Show 24 quoted lines
> 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,/
Show 11 quoted lines
>
> 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>
Junio C Hamano· Dec 20, 2016, 19:34 UTC · re: Mike Hommey · lore

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

Mike Hommey <mh@glandium.org> writes:
Show 7 quoted lines
> 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.
Mike Hommey· Dec 20, 2016, 20:48 UTC · re: Junio C Hamano · lore

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

On Tue, Dec 20, 2016 at 11:34:04AM -0800, Junio C Hamano wrote:
Show 20 quoted lines
> 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
Junio C Hamano· Dec 20, 2016, 20:56 UTC · re: Mike Hommey · lore

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

Mike Hommey <mh@glandium.org> writes:
Show 28 quoted lines
> 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
Mike Hommey· Dec 20, 2016, 21:04 UTC · re: Junio C Hamano · lore

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(-)
Show changes to 2 files +47 −3

fast-import.c, t/t9301-fast-import-notes.sh

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
Junio C Hamano· Dec 20, 2016, 21:50 UTC · re: Mike Hommey · lore

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

Mike Hommey <mh@glandium.org> writes:
Show 6 quoted lines
> 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.

← back to recent threads