* [PATCH 0/2] fast-import: allow more operations on root directory @ 2011-08-16 10:08 Dmitry Ivankov 2011-08-16 10:08 ` [PATCH 1/2] fast-import: be saner with temporary trees Dmitry Ivankov 2011-08-16 10:08 ` [PATCH 2/2] fast-import: allow top directory as an argument for some commands Dmitry Ivankov 0 siblings, 2 replies; 4+ messages in thread From: Dmitry Ivankov @ 2011-08-16 10:08 UTC (permalink / raw) To: git; +Cc: Jonathan Nieder, David Barr, Shawn O. Pearce, Dmitry Ivankov [1/2] has been already reviewed in [1] where it didn't seem to have any effect, but for this series it does, for 'ls ""' command. So the change is about allowing to "ls" root directory and to copy or rename it to some subdirectory. The documentation doesn't deny these, but they don't work. Make them work. More details are in [2/2]. [1] http://thread.gmane.org/gmane.comp.version-control.git/178007/focus=178009 Dmitry Ivankov (2): fast-import: be saner with temporary trees fast-import: allow top directory as an argument for some commands fast-import.c | 78 +++++++++++++++++++++++++++++++++++++++++------ t/t9300-fast-import.sh | 58 +++++++++++++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 10 deletions(-) -- 1.7.3.4 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/2] fast-import: be saner with temporary trees 2011-08-16 10:08 [PATCH 0/2] fast-import: allow more operations on root directory Dmitry Ivankov @ 2011-08-16 10:08 ` Dmitry Ivankov 2012-03-08 19:12 ` Jonathan Nieder 2011-08-16 10:08 ` [PATCH 2/2] fast-import: allow top directory as an argument for some commands Dmitry Ivankov 1 sibling, 1 reply; 4+ messages in thread From: Dmitry Ivankov @ 2011-08-16 10:08 UTC (permalink / raw) To: git; +Cc: Jonathan Nieder, David Barr, Shawn O. Pearce, Dmitry Ivankov new_tree_entry/release_tree_entry manage a stack of tree_entry structs to use as temporaries. Initializing them is the responsibility of the caller, both after allocation with xmalloc() when existing temporaries are exhausted and after used entries are pushed with release_tree_entry. parse_ls doesn't set root->versions[0] fields, making root an invalid entry. The problem could arise if store_tree(root) is called. parse_ls calls store_tree on the node corresponding to the path it is called on. Do initialize entry->versions[0]. As of now, ls command can not list the topmost tree so this change is just to avoid surprises in case things will change around ls or tree_content_get. Signed-off-by: Dmitry Ivankov <divanorama@gmail.com> Acked-by: Jonathan Nieder <jrnieder@gmail.com> --- fast-import.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/fast-import.c b/fast-import.c index 7cc2262..3a0aaad 100644 --- a/fast-import.c +++ b/fast-import.c @@ -2951,6 +2951,9 @@ static void parse_ls(struct branch *b) } else { struct object_entry *e = parse_treeish_dataref(&p); root = new_tree_entry(); + hashclr(root->versions[0].sha1); + root->versions[0].mode = 0; + root->versions[1].mode = S_IFDIR; hashcpy(root->versions[1].sha1, e->idx.sha1); load_tree(root); if (*p++ != ' ') -- 1.7.3.4 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] fast-import: be saner with temporary trees 2011-08-16 10:08 ` [PATCH 1/2] fast-import: be saner with temporary trees Dmitry Ivankov @ 2012-03-08 19:12 ` Jonathan Nieder 0 siblings, 0 replies; 4+ messages in thread From: Jonathan Nieder @ 2012-03-08 19:12 UTC (permalink / raw) To: Dmitry Ivankov Cc: git, David Barr, Shawn O. Pearce, Andrew Sayers, Sverre Rabbelier Hi, Dmitry Ivankov wrote: > new_tree_entry/release_tree_entry manage a stack of tree_entry structs > to use as temporaries. Initializing them is the responsibility of the > caller, [...] > Do initialize entry->versions[0]. As of now, ls command can not list > the topmost tree so this change is just to avoid surprises in case > things will change around ls or tree_content_get. > > Signed-off-by: Dmitry Ivankov <divanorama@gmail.com> > Acked-by: Jonathan Nieder <jrnieder@gmail.com> [...] > +++ b/fast-import.c > @@ -2951,6 +2951,9 @@ static void parse_ls(struct branch *b) > } else { > struct object_entry *e = parse_treeish_dataref(&p); > root = new_tree_entry(); > + hashclr(root->versions[0].sha1); > + root->versions[0].mode = 0; > + root->versions[1].mode = S_IFDIR; > hashcpy(root->versions[1].sha1, e->idx.sha1); I don't remember what this patch was about or why I acked it, which is a bad sign. (If the above explanation doesn't make me want to ack it again, how is it going to help someone in the future understand the harm in reverting this patch if some other bug bisects to it?) I think part of the problem is that the explanation seems to be phrased in terms of implementation details (root->versions[0] fields, calling store_tree, initializing entry->versions[0]) instead of the higher-level logic that these implement. It sounds ("avoid surprises") like this is meant to avoid scary behavior from the programmer's perspective, so: what are the symptoms without this patch, and why is this a good way to eliminate them? On the other hand, if we squash this in with David's [1], it starts to make perfect sense. The explanation can say: There is a pathological Subversion operation that svn-fe handles incorrectly due to an unexpected response from fast-import: svn cp $SVN_ROOT $SVN_ROOT/subdirectory When the following command is sent to fast-import: 'ls' SP ':1' SP LF The expected output is: '040000' SP 'tree' SP <dataref> HT LF The actual output is: 'missing' SP LF This is because tree_content_get() is called but expects a non-empty path. Instead, copy the root entry and force the mode to S_IFDIR. In the 'ls <dataref> ""' case, this requires some care, because parse_ls currently uses an uninitialized temporary tree entry from the new_tree_entry/release_tree_entry stack in a quick and dirty way without initializing its fields well enough for functions like store_tree that might calculate its tree name. Sensible? Alternatively, we could rush in David's [1] with the tweak below to dodge the 'ls <dataref> ""' case, and include your fix as part of a patch that just addresses that case. Thanks very much for the patch and the reminder. Sincerely, Jonathan [1] http://thread.gmane.org/gmane.comp.version-control.git/192517/focus=192520 --- fast-import.c | 2 +- t/t9300-fast-import.sh | 66 +++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 66 insertions(+), 2 deletions(-) diff --git a/fast-import.c b/fast-import.c index 8dbfd4cc..2748ced2 100644 --- a/fast-import.c +++ b/fast-import.c @@ -3019,7 +3019,7 @@ static void parse_ls(struct branch *b) die("Garbage after path in: %s", command_buf.buf); p = uq.buf; } - if (*p) { + if (*p || root != &b->branch_tree) { tree_content_get(root, p, &leaf); } else { leaf = *root; diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh index 2558a2ed..2f411fbb 100755 --- a/t/t9300-fast-import.sh +++ b/t/t9300-fast-import.sh @@ -1400,8 +1400,72 @@ test_expect_success \ test_cmp expect.qux actual.qux && test_cmp expect.qux actual.quux' +test_expect_success 'N: root of unborn branch reads as present and empty' ' + empty_tree=$(git mktree </dev/null) && + echo "040000 tree $empty_tree " >expect && + git fast-import --cat-blob-fd=3 3>actual <<-EOF && + commit refs/heads/N-empty + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE + data <<COMMIT + read empty root directory via ls + COMMIT + + ls "" + EOF + test_cmp expect actual +' + +test_expect_success 'N: empty root reads as present and empty' ' + empty_tree=$(git mktree </dev/null) && + echo "040000 tree $empty_tree " >expect && + echo empty >msg && + cmit=$(git commit-tree "$empty_tree" -p refs/heads/branch^0 <msg) && + git fast-import --cat-blob-fd=3 3>actual <<-EOF && + commit refs/heads/N-empty-existing + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE + data <<COMMIT + read empty root directory via ls + COMMIT + + ls "" + EOF + test_cmp expect actual +' + +test_expect_success 'N: "ls" command can read subdir of named tree' ' + branch_cmit=$(git rev-parse --verify refs/heads/branch^0) && + subdir_tree=$(git rev-parse $branch_cmit:newdir) && + echo "040000 tree $subdir_tree newdir" >expect && + git fast-import --cat-blob-fd=3 3>actual <<-EOF && + commit refs/heads/N-subdir-of-named-tree + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE + data <<COMMIT + read from commit with ls + COMMIT + + ls $branch_cmit "newdir" + EOF + test_cmp expect actual +' + +test_expect_failure 'N: "ls" command can read root of named commit' ' + branch_cmit=$(git rev-parse --verify refs/heads/branch^0) && + branch_tree=$(git rev-parse --verify $branch_cmit:) && + echo "040000 tree $branch_tree " >expect && + git fast-import --cat-blob-fd=3 3>actual <<-EOF && + commit refs/heads/N-root-of-named-tree + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE + data <<COMMIT + read root of commit with ls + COMMIT + + ls $branch_cmit "" + EOF + test_cmp expect actual +' + test_expect_success PIPE 'N: read and copy root' ' - cat >expect <<-\EOF + cat >expect <<-\EOF && :100755 100755 f1fb5da718392694d0076d677d6d0e364c79b0bc f1fb5da718392694d0076d677d6d0e364c79b0bc C100 file2/newf file3/file2/newf :100644 100644 7123f7f44e39be127c5eb701e5968176ee9d78b1 7123f7f44e39be127c5eb701e5968176ee9d78b1 C100 file2/oldf file3/file2/oldf :100755 100755 85df50785d62d3b05ab03d9cbf7e4a0b49449730 85df50785d62d3b05ab03d9cbf7e4a0b49449730 C100 file4 file3/file4 -- 1.7.9.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] fast-import: allow top directory as an argument for some commands 2011-08-16 10:08 [PATCH 0/2] fast-import: allow more operations on root directory Dmitry Ivankov 2011-08-16 10:08 ` [PATCH 1/2] fast-import: be saner with temporary trees Dmitry Ivankov @ 2011-08-16 10:08 ` Dmitry Ivankov 1 sibling, 0 replies; 4+ messages in thread From: Dmitry Ivankov @ 2011-08-16 10:08 UTC (permalink / raw) To: git; +Cc: Jonathan Nieder, David Barr, Shawn O. Pearce, Dmitry Ivankov fast-import allows to operate on the toplevel directory. This includes setting the value by sha1/mark, copying/renaming some subdirectory to a toplevel one. But it's not possible to 'ls' it or copy/rename it so somewhere, or delete it with 'D' command. For ls it's quite reasonable input so just handle it. The special case is empty directory, for other empty directories we report 'missing' so stick with this behaviour with toplevel too. For copy/rename it looks like a normal operation too so handle it too. But if it's empty, do nothing - there is nothing to copy/rename and the destination is nonexistent because toplevel is empty. For 'D ""' we already have deleteall command so probably it's not worth adding one more way to do the same thing. Signed-off-by: Dmitry Ivankov <divanorama@gmail.com> --- fast-import.c | 75 +++++++++++++++++++++++++++++++++++++++++------ t/t9300-fast-import.sh | 58 +++++++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 10 deletions(-) diff --git a/fast-import.c b/fast-import.c index 3a0aaad..8643e23 100644 --- a/fast-import.c +++ b/fast-import.c @@ -1552,6 +1552,31 @@ static int tree_content_set( return 1; } +static int tree_content_clear( + struct tree_entry *root, + struct tree_entry *backup_leaf) +{ + if (backup_leaf) + memcpy(backup_leaf, root, sizeof(*backup_leaf)); + else if (root->tree) + release_tree_content_recursive(root->tree); + root->tree = NULL; + hashclr(root->versions[1].sha1); + return 1; +} + +static int is_empty_tree(struct tree_entry *root) +{ + int n; + if (!root->tree) + load_tree(root); + for (n = 0; n < root->tree->entry_count; n++) { + if (root->tree->entries[n]->versions[1].mode) + return 0; + } + return 1; +} + static int tree_content_remove( struct tree_entry *root, const char *p, @@ -1587,11 +1612,9 @@ static int tree_content_remove( if (!e->tree) load_tree(e); if (tree_content_remove(e, slash1 + 1, backup_leaf)) { - for (n = 0; n < e->tree->entry_count; n++) { - if (e->tree->entries[n]->versions[1].mode) { - hashclr(root->versions[1].sha1); - return 1; - } + if (!is_empty_tree(e)) { + hashclr(root->versions[1].sha1); + return 1; } backup_leaf = NULL; goto del_entry; @@ -1613,6 +1636,19 @@ del_entry: return 1; } +static int tree_content_copy_root( + struct tree_entry *root, + struct tree_entry *leaf +) +{ + memcpy(leaf, root, sizeof(*leaf)); + if (root->tree && is_null_sha1(root->versions[1].sha1)) + leaf->tree = dup_tree_content(root->tree); + else + leaf->tree = NULL; + return 1; +} + static int tree_content_get( struct tree_entry *root, const char *p, @@ -2329,10 +2365,17 @@ 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); - else - tree_content_get(&b->branch_tree, s, &leaf); + if (rename) { + if (!*s) + tree_content_clear(&b->branch_tree, &leaf); + else + tree_content_remove(&b->branch_tree, s, &leaf); + } else { + if (!*s) + tree_content_copy_root(&b->branch_tree, &leaf); + else + tree_content_get(&b->branch_tree, s, &leaf); + } if (!leaf.versions[1].mode) die("Path %s not in branch", s); if (!*d) { /* C "path/to/subdir" "" */ @@ -2342,6 +2385,14 @@ static void file_change_cr(struct branch *b, int rename) leaf.tree); return; } + /* + * Git does not track empty, non-toplevel directories. + * At this point destination is non-toplevel, so if source is + * toplevel and empty, leave it as is all empty. + */ + if (!*s && is_empty_tree(&leaf)) { + return; + } tree_content_set(&b->branch_tree, d, leaf.versions[1].sha1, leaf.versions[1].mode, @@ -2969,7 +3020,11 @@ 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); + if (!*p) { + if (!is_empty_tree(root)) + tree_content_copy_root(root, &leaf); + } else + tree_content_get(root, p, &leaf); /* * A directory in preparation would have a sha1 of zero * until it is saved. Save, for simplicity. diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh index f256475..0e46cf0 100755 --- a/t/t9300-fast-import.sh +++ b/t/t9300-fast-import.sh @@ -978,6 +978,64 @@ test_expect_success PIPE 'N: empty directory reads as missing' ' ' test_expect_success \ + 'N: ls root directory' \ + 'cat >expect <<-\EOF && + missing + 040000 + EOF + cat >input <<-EOF && + commit refs/heads/ls-root + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE + data <<COMMIT + On empty tree: ls add file ls. + COMMIT + ls "" + M 644 inline file1 + data 0 + ls "" + EOF + git fast-import --quiet <input >tmp && + cat tmp | cut -d " " -f 1 >actual && + test_cmp expect actual' + +test_expect_success \ + 'N: move root directory' \ + 'echo "root/a/b" >expect && + cat >input <<-EOF && + commit refs/heads/move-root + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE + data <<COMMIT + Add stuff and move it. + COMMIT + M 644 inline a/b + data 0 + R "" root + EOF + git fast-import <input && + git ls-tree -r --name-only refs/heads/move-root >actual && + test_cmp expect actual' + +test_expect_success \ + 'N: copy root directory' \ + 'cat <<-\EOF >expect && + a/b + a/root/a/b + EOF + cat >input <<-EOF && + commit refs/heads/copy-root + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE + data <<COMMIT + Add stuff and move it. + COMMIT + M 644 inline a/b + data 0 + C "" a/root + EOF + git fast-import <input && + git ls-tree -r --name-only refs/heads/copy-root >actual && + test_cmp expect actual' + +test_expect_success \ 'N: copy root directory by tree hash' \ 'cat >expect <<-\EOF && :100755 000000 f1fb5da718392694d0076d677d6d0e364c79b0bc 0000000000000000000000000000000000000000 D file3/newf -- 1.7.3.4 ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-03-08 19:12 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-08-16 10:08 [PATCH 0/2] fast-import: allow more operations on root directory Dmitry Ivankov 2011-08-16 10:08 ` [PATCH 1/2] fast-import: be saner with temporary trees Dmitry Ivankov 2012-03-08 19:12 ` Jonathan Nieder 2011-08-16 10:08 ` [PATCH 2/2] fast-import: allow top directory as an argument for some commands Dmitry Ivankov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).