fast-import: handle empty paths better
cover fast-import: handle empty paths better
This series addressed Dave Abraham's recent bug report [1] about using fast-import's "ls" command with an empty path. I also found a couple of other places that do not handle the empty path when it can reasonably be interpreted as meaning the root tree object, which are also fixed here.
[1] http://article.gmane.org/gmane.comp.version-control.git/228586
John Keeping (4):
t9300: document fast-import empty path issues
fast-import: set valid mode on root tree in "ls"
fast-import: allow ls or filecopy of the root tree
fast-import: allow moving the root tree
fast-import.c | 58 ++++++++++++++++++++++++++++---------------- t/t9300-fast-import.sh | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 20 deletions(-)
Signature · 1 line
1.8.3.1.676.gaae6535
PATCH 1/4 t9300: document fast-import empty path issues
When given an empty path, fast-import sometimes reports "missing" instead of using the root tree object. On top of this, for "ls" and file copy (but not move) it dies with "Empty path component found in input".
Document this behaviour with failing test cases.
--- t/t9300-fast-import.sh | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+)
t/t9300-fast-import.sh +65 -0
@@ -1031,6 +1031,32 @@ test_expect_success \ git diff-tree -M -r M3^ M3 >actual && compare_diff_raw expect actual' +cat >input <<INPUT_END +commit refs/heads/M4 +committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE +data <<COMMIT +rename root +COMMIT + +from refs/heads/M2^0 +R "" sub + +INPUT_END + +cat >expect <<EOF +:100644 100644 7123f7f44e39be127c5eb701e5968176ee9d78b1 7123f7f44e39be127c5eb701e5968176ee9d78b1 R100 file2/oldf sub/file2/oldf +:100755 100755 85df50785d62d3b05ab03d9cbf7e4a0b49449730 85df50785d62d3b05ab03d9cbf7e4a0b49449730 R100 file4 sub/file4 +:100755 100755 f1fb5da718392694d0076d677d6d0e364c79b0bc f1fb5da718392694d0076d677d6d0e364c79b0bc R100 i/am/new/to/you sub/i/am/new/to/you +:100755 100755 e74b7d465e52746be2b4bae983670711e6e66657 e74b7d465e52746be2b4bae983670711e6e66657 R100 newdir/exec.sh sub/newdir/exec.sh +:100644 100644 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 R100 newdir/interesting sub/newdir/interesting +EOF +test_expect_failure \ + 'M: rename root to subdirectory' \ + 'git fast-import <input && + git diff-tree -M -r M4^ M4 >actual && + cat actual && + compare_diff_raw expect actual' + ### ### series N ###
@@ -1227,6 +1253,29 @@ test_expect_success \ git diff-tree -C --find-copies-harder -r N4 N6 >actual && compare_diff_raw expect actual' +test_expect_failure \ + 'N: copy root by path' \ + 'cat >expect <<-\EOF && + :100755 100755 f1fb5da718392694d0076d677d6d0e364c79b0bc f1fb5da718392694d0076d677d6d0e364c79b0bc C100 file2/newf oldroot/file2/newf + :100644 100644 7123f7f44e39be127c5eb701e5968176ee9d78b1 7123f7f44e39be127c5eb701e5968176ee9d78b1 C100 file2/oldf oldroot/file2/oldf + :100755 100755 85df50785d62d3b05ab03d9cbf7e4a0b49449730 85df50785d62d3b05ab03d9cbf7e4a0b49449730 C100 file4 oldroot/file4 + :100755 100755 e74b7d465e52746be2b4bae983670711e6e66657 e74b7d465e52746be2b4bae983670711e6e66657 C100 newdir/exec.sh oldroot/newdir/exec.sh + :100644 100644 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 C100 newdir/interesting oldroot/newdir/interesting + EOF + cat >input <<-INPUT_END && + commit refs/heads/N-copy-root-path + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE + data <<COMMIT + copy root directory by (empty) path + COMMIT + + from refs/heads/branch^0 + C "" oldroot + INPUT_END + git fast-import <input && + git diff-tree -C --find-copies-harder -r branch N-copy-root-path >actual && + compare_diff_raw expect actual' + test_expect_success \ 'N: delete directory by copying' \ 'cat >expect <<-\EOF &&
@@ -2934,4 +2983,20 @@ test_expect_success 'S: ls with garbage after sha1 must fail' ' test_i18ngrep "space after tree-ish" err ' +### +### series T (ls) +### +# Setup is carried over from series S. + +test_expect_failure 'T: ls root tree' ' + sed -e "s/Z\$//" >expect <<-EOF && + 040000 tree $(git rev-parse S^{tree}) Z + EOF + sha1=$(git rev-parse --verify S) && + git fast-import --import-marks=marks <<-EOF >actual && + ls $sha1 "" + EOF + test_cmp expect actual +' + test_done
Signature · 1 line
1.8.3.1.676.gaae6535
PATCH 2/4 fast-import: set valid mode on root tree in "ls"
In reply to John KeepingThis prevents a failure later when we lift the restriction on ls with the empty path.
--- fast-import.c | 2 ++ 1 file changed, 2 insertions(+)
fast-import.c +2 -0
@@ -3051,6 +3051,8 @@ static void parse_ls(struct branch *b)struct object_entry *e =parse_treeish_dataref (&p); root =new_tree_entry ();hashcpy (root->versions[1 ].sha1, e->idx.sha1); +if (!is_null_sha1 (root->versions[1 ].sha1)) + root->versions[1 ].mode =S_IFDIR ;load_tree (root);if (*p++ !=' ' )die ("Missing space after tree-ish: %s" , command_buf.buf);
Signature · 1 line
1.8.3.1.676.gaae6535
PATCH 3/4 fast-import: allow ls or filecopy of the root tree
In reply to John KeepingCommit 178e1de (fast-import: don't allow 'ls' of path with empty components, 2012-03-09) restricted paths which:
. contain an empty directory component (e.g. foo//bar is invalid), . end with a directory separator (e.g. foo/ is invalid), . start with a directory separator (e.g. /foo is invalid).
However, the implementation also caught the empty path, which should represent the root tree. Relax this restriction so that the empty path is explicitly allowed and refers to the root tree.
--- fast-import.c | 35 ++++++++++++++++++++++------------- t/t9300-fast-import.sh | 4 ++-- 2 files changed, 24 insertions(+), 15 deletions(-)
fast-import.c +22 -13
@@ -1629,7 +1629,8 @@ del_entry:static int tree_content_get (struct tree_entry *root,const char *p, -struct tree_entry *leaf) +struct tree_entry *leaf, +int allow_root) {struct tree_content *t;const char *slash1;
@@ -1641,31 +1642,39 @@ static int tree_content_get( n = slash1 - p; else n =strlen (p); -if (!n) +if (!n && !allow_root)die ("Empty path component found in input" );if (!root->tree)load_tree (root); + +if (!n) { + e = root; + goto found_entry; + } + t = root->tree;for (i =0 ; i < t->entry_count; i++) { e = t->entries[i];if (e->name->str_len == n && !strncmp_icase (p, e->name->str_dat, n)) { -if (!slash1) { -memcpy (leaf, e,sizeof (*leaf)); -if (e->tree &&is_null_sha1 (e->versions[1 ].sha1)) - leaf->tree =dup_tree_content (e->tree); -else - leaf->tree =NULL ; -return 1 ; - } +if (!slash1) + goto found_entry;if (!S_ISDIR (e->versions[1 ].mode))return 0 ;if (!e->tree)load_tree (e); -return tree_content_get (e, slash1 +1 , leaf); +return tree_content_get (e, slash1 +1 , leaf,0 ); } }return 0 ; + +found_entry: +memcpy (leaf, e,sizeof (*leaf)); +if (e->tree &&is_null_sha1 (e->versions[1 ].sha1)) + leaf->tree =dup_tree_content (e->tree); +else + leaf->tree =NULL ; +return 1 ; }static int update_branch (struct branch *b)
@@ -2415,7 +2424,7 @@ static void file_change_cr(struct branch *b, int rename)if (rename)tree_content_remove (&b->branch_tree, s, &leaf);else -tree_content_get (&b->branch_tree, s, &leaf); +tree_content_get (&b->branch_tree, s, &leaf,1 );if (!leaf.versions[1 ].mode)die ("Path %s not in branch" , s);if (!*d) {/* C "path/to/subdir" "" */
@@ -3067,7 +3076,7 @@ static void parse_ls(struct branch *b)die ("Garbage after path in: %s" , command_buf.buf); p = uq.buf; } -tree_content_get (root, p, &leaf); +tree_content_get (root, p, &leaf,1 );/* * A directory in preparation would have a sha1 of zero * until it is saved. Save, for simplicity.
t/t9300-fast-import.sh +2 -2
@@ -1253,7 +1253,7 @@ test_expect_success \ git diff-tree -C --find-copies-harder -r N4 N6 >actual && compare_diff_raw expect actual' -test_expect_failure \ +test_expect_success \ 'N: copy root by path' \ 'cat >expect <<-\EOF && :100755 100755 f1fb5da718392694d0076d677d6d0e364c79b0bc f1fb5da718392694d0076d677d6d0e364c79b0bc C100 file2/newf oldroot/file2/newf
@@ -2988,7 +2988,7 @@ test_expect_success 'S: ls with garbage after sha1 must fail' ' ### # Setup is carried over from series S. -test_expect_failure 'T: ls root tree' ' +test_expect_success 'T: ls root tree' ' sed -e "s/Z\$//" >expect <<-EOF && 040000 tree $(git rev-parse S^{tree}) Z EOF
Signature · 1 line
1.8.3.1.676.gaae6535
PATCH 4/4 fast-import: allow moving the root tree
In reply to John KeepingBecause fast-import.c::tree_content_remove does not check for the empty path, it is not possible to move the root tree to a subdirectory. Instead the error "Path not in branch" is produced (note the double space where the empty path has been inserted).
Fix this by explicitly checking for the empty path and handling it.
--- fast-import.c | 21 ++++++++++++++------- t/t9300-fast-import.sh | 2 +- 2 files changed, 15 insertions(+), 8 deletions(-)
fast-import.c +14 -7
@@ -1568,7 +1568,8 @@ static int tree_content_set(static int tree_content_remove (struct tree_entry *root,const char *p, -struct tree_entry *backup_leaf) +struct tree_entry *backup_leaf, +int allow_root) {struct tree_content *t;const char *slash1;
@@ -1583,6 +1584,12 @@ static int tree_content_remove(if (!root->tree)load_tree (root); + +if (!*p && allow_root) { + e = root; + goto del_entry; + } + t = root->tree;for (i =0 ; i < t->entry_count; i++) { e = t->entries[i];
@@ -1599,7 +1606,7 @@ static int tree_content_remove( goto del_entry;if (!e->tree)load_tree (e); -if (tree_content_remove (e, slash1 +1 , backup_leaf)) { +if (tree_content_remove (e, slash1 +1 , backup_leaf,0 )) {for (n =0 ; n < e->tree->entry_count; n++) {if (e->tree->entries[n]->versions[1 ].mode) {hashclr (root->versions[1 ].sha1);
@@ -2188,7 +2195,7 @@ static uintmax_t do_change_note_fanout( }/* Rename fullpath to realpath */ -if (!tree_content_remove (orig_root, fullpath, &leaf)) +if (!tree_content_remove (orig_root, fullpath, &leaf,0 ))die ("Failed to remove path %s" , fullpath); tree_content_set(orig_root, realpath, leaf.versions[1].sha1,
@@ -2323,7 +2330,7 @@ static void file_change_m(struct branch *b)/* Git does not track empty, non-toplevel directories. */ if (S_ISDIR (mode) && !memcmp (sha1,EMPTY_TREE_SHA1_BIN ,20 ) && *p) { -tree_content_remove (&b->branch_tree, p,NULL ); +tree_content_remove (&b->branch_tree, p,NULL ,0 );return ; }
@@ -2384,7 +2391,7 @@ static void file_change_d(struct branch *b)die ("Garbage after path in: %s" , command_buf.buf); p = uq.buf; } -tree_content_remove (&b->branch_tree, p,NULL ); +tree_content_remove (&b->branch_tree, p,NULL ,1 ); }static void file_change_cr (struct branch *b,int rename)
@@ -2422,7 +2429,7 @@ static void file_change_cr(struct branch *b, int rename)memset (&leaf,0 ,sizeof (leaf));if (rename) -tree_content_remove (&b->branch_tree, s, &leaf); +tree_content_remove (&b->branch_tree, s, &leaf,1 );else tree_content_get (&b->branch_tree, s, &leaf,1 );if (!leaf.versions[1 ].mode)
@@ -2530,7 +2537,7 @@ static void note_change_n(struct branch *b, unsigned char *old_fanout) }construct_path_with_fanout (sha1_to_hex (commit_sha1), *old_fanout, path); -if (tree_content_remove (&b->branch_tree, path,NULL )) +if (tree_content_remove (&b->branch_tree, path,NULL ,0 )) b->num_notes--;if (is_null_sha1 (sha1))
t/t9300-fast-import.sh +1 -1
@@ -1050,7 +1050,7 @@ cat >expect <<EOF :100755 100755 e74b7d465e52746be2b4bae983670711e6e66657 e74b7d465e52746be2b4bae983670711e6e66657 R100 newdir/exec.sh sub/newdir/exec.sh :100644 100644 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 R100 newdir/interesting sub/newdir/interesting EOF -test_expect_failure \ +test_expect_success \ 'M: rename root to subdirectory' \ 'git fast-import <input && git diff-tree -M -r M4^ M4 >actual &&
Signature · 1 line
1.8.3.1.676.gaae6535
Quoted 13 lines · John Keeping
This series addressed Dave Abraham's recent bug report [1] about using fast-import's "ls" command with an empty path. I also found a couple of other places that do not handle the empty path when it can reasonably be interpreted as meaning the root tree object, which are also fixed here.
[1] http://article.gmane.org/gmane.comp.version-control.git/228586
John Keeping (4):
t9300: document fast-import empty path issues
fast-import: set valid mode on root tree in "ls"
fast-import: allow ls or filecopy of the root tree
fast-import: allow moving the root tree
Quoted 3 lines
fast-import.c | 58 ++++++++++++++++++++++++++++---------------- t/t9300-fast-import.sh | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 20 deletions(-)
Ping? Reviews, please?
No need to hurry, but just to make sure this didn't disappear from everybody's radar.
Thanks.