[PATCH] fast-import: Allow filemodify to set the root

[PATCH] fast-import: Allow filemodify to set the root

[David Barr]

Most git commands do their writing to the object db via the index and
loose objects.  When you just have a pile of trees you want to convert
into commits, this is wasteful; for performance-critical operations
like filter-branch --subdirectory-filter, one might want a sort of
hash-object --batch-to-pack to write a pack directly.

Fortunately we have fast-import (which is one of the only git commands
that will write to a pack directly) but there is not an advertised way
to tell fast-import to use a given tree for its commits.  So in
current git, one has the unpleasant choice of writing loose objects
without parsing the trees or writing straight to pack but having to
parse trees to do it.

This patch changes that, by allowing

	M 040000 <tree id> ""

as a filemodify line in a commit to reset to a particular tree without
any need to unpack it.  For example,

	M 040000 4b825dc642cb6eb9a060e54bf8d69288fbee4904 ""

is a synonym for the deleteall command.

Commit-message-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: David Barr <david.barr@cordelta.com>
---
 fast-import.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 2317b0f..8f68a89 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1454,6 +1454,15 @@ static int tree_content_set(
 		n = slash1 - p;
 	else
 		n = strlen(p);
+	if (!slash1 && !n) {
+		if (!S_ISDIR(mode))
+			die("Root cannot be a non-directory");
+		hashcpy(root->versions[1].sha1, sha1);
+		if (root->tree)
+			release_tree_content_recursive(root->tree);
+		root->tree = subtree;
+		return 1;
+	}
 	if (!n)
 		die("Empty path component found in input");
 	if (!slash1 && !S_ISDIR(mode) && subtree)
-- 
1.7.3

Re: [PATCH] fast-import: Allow filemodify to set the root

[Sverre Rabbelier]

Heya,

On Thu, Oct 7, 2010 at 12:55, David Barr <david.barr@cordelta.com> wrote:
> Most git commands do their writing to the object db via the index and
> loose objects.  When you just have a pile of trees you want to convert
> into commits, this is wasteful; for performance-critical operations
> like filter-branch --subdirectory-filter, one might want a sort of
> hash-object --batch-to-pack to write a pack directly.

This means nothing to me, but perhaps I'm not the target audience of
this paragraph.

> Fortunately we have fast-import (which is one of the only git commands
> that will write to a pack directly) but there is not an advertised way
> to tell fast-import to use a given tree for its commits.

I'm with you up to "use a given tree for its commits".

> This patch changes that, by allowing
>
>        M 040000 <tree id> ""
>
> as a filemodify line in a commit to reset to a particular tree without
> any need to unpack it.  For example,
>
>        M 040000 4b825dc642cb6eb9a060e54bf8d69288fbee4904 ""
>
> is a synonym for the deleteall command.

Ok, so maybe I do understand, is it basically 'git read-tree
4b825dc642cb6eb9a060e54bf8d69288fbee4904' for fast-import?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] fast-import: Allow filemodify to set the root

[Jonathan Nieder]

Sverre Rabbelier wrote:

> This means nothing to me

Sorry for the lack of clarity.  Probably I should have just said:

| For a command (like filter-branch --subdirectory-filter) that wants
| to commit a lot of trees that already exist in the object db, writing
| undeltified objects as loose files only to repack them later can
| involve a significant amount[*] of overhead.
|
| Fortunately we have fast-import (which is one of the only git commands
| that will write to a pack directly) but there is not an advertised way
| to tell fast-import to commit a given tree without unpacking it.
|
| This patch changes that, by allowing
|
|        M 040000 <tree id> ""
|
| as a filemodify line in a commit to reset to a particular tree without
| any need to parse it.  For example,
|
|	M 040000 4b825dc642cb6eb9a060e54bf8d69288fbee4904 ""
|
| is a synonym for the deleteall command.

[*] how significant?  Numbers are always nice. :)

> Ok, so maybe I do understand, is it basically 'git read-tree
> 4b825dc642cb6eb9a060e54bf8d69288fbee4904' for fast-import?

Yep.

Thanks.

Re: [PATCH] fast-import: Allow filemodify to set the root

[Sverre Rabbelier]

Heya,

On Thu, Oct 7, 2010 at 22:28, Jonathan Nieder <jrnieder@gmail.com> wrote:
> | is a synonym for the deleteall command.

Thanks, the updated doc is more understandable.

> [*] how significant?  Numbers are always nice. :)

Yes, numbers please! :)

>> Ok, so maybe I do understand, is it basically 'git read-tree
>> 4b825dc642cb6eb9a060e54bf8d69288fbee4904' for fast-import?
>
> Yep.

Perhaps mention that in the commit message as well then. Of course,
the fast-import doc needs updating, and it needs test.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] fast-import: Allow filemodify to set the root

[David Barr]

Hi,

> | For a command (like filter-branch --subdirectory-filter) that wants
> | to commit a lot of trees that already exist in the object db, writing
> | undeltified objects as loose files only to repack them later can
> | involve a significant amount[*] of overhead.

> [*] how significant?  Numbers are always nice. :)

Using git hash-object:
real    2m51.748s
user    0m25.460s
sys     0m23.890s

Using git fast-import:
real    2m13.337s
user    0m28.010s
sys     0m18.100s

> 
> > Ok, so maybe I do understand, is it basically 'git read-tree
> > 4b825dc642cb6eb9a060e54bf8d69288fbee4904' for fast-import?

> Perhaps mention that in the commit message as well then. Of course,
> the fast-import doc needs updating, and it needs test.

| is a synonym for the deleteall command and the fast-import equivalent of
| 
|         git read-tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904

--
David Barr

[PATCH] fast-import: Allow filemodify to set the root

[David Barr]

For a command (like filter-branch --subdirectory-filter) that wants
to commit a lot of trees that already exist in the object db, writing
undeltified objects as loose files only to repack them later can
involve a significant amount of overhead.
(23% slow-down observed on Linux 2.6.35, worse on Mac OS X 10.6)

Fortunately we have fast-import (which is one of the only git commands
that will write to a pack directly) but there is not an advertised way
to tell fast-import to commit a given tree without unpacking it.

This patch changes that, by allowing

	M 040000 <tree id> ""

as a filemodify line in a commit to reset to a particular tree without
any need to parse it.  For example,

	M 040000 4b825dc642cb6eb9a060e54bf8d69288fbee4904 ""

is a synonym for the deleteall command and the fast-import equivalent of

	git read-tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904

Commit-message-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: David Barr <david.barr@cordelta.com>
---
 fast-import.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 2317b0f..8f68a89 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1454,6 +1454,15 @@ static int tree_content_set(
 		n = slash1 - p;
 	else
 		n = strlen(p);
+	if (!slash1 && !n) {
+		if (!S_ISDIR(mode))
+			die("Root cannot be a non-directory");
+		hashcpy(root->versions[1].sha1, sha1);
+		if (root->tree)
+			release_tree_content_recursive(root->tree);
+		root->tree = subtree;
+		return 1;
+	}
 	if (!n)
 		die("Empty path component found in input");
 	if (!slash1 && !S_ISDIR(mode) && subtree)
-- 
1.7.3.4.g45608.dirty

Re: [PATCH] fast-import: Allow filemodify to set the root

[Sverre Rabbelier]

Heya,

On Fri, Oct 8, 2010 at 01:46, David Barr <david.barr@cordelta.com> wrote:
> Commit-message-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: David Barr <david.barr@cordelta.com>

I like it, FWIW:

Acked-by: Sverre Rabbelier <srabbelier@gmail.com>

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] fast-import: Allow filemodify to set the root

[Johannes Sixt]

Am 10/7/2010 22:28, schrieb Jonathan Nieder:
> Sverre Rabbelier wrote:
> 
>> This means nothing to me
> 
> Sorry for the lack of clarity.  Probably I should have just said:
> 
> | For a command (like filter-branch --subdirectory-filter) that wants
> | to commit a lot of trees that already exist in the object db, writing
> | undeltified objects as loose files only to repack them later can
> | involve a significant amount[*] of overhead.

1. But when an object already exists in the db, it won't be written again,
will it?

2. Even though fast-import puts all (new) objects into a pack file, the
pack is heavily sub-optimal, and you should repack -f anyway. So what's
the point? Only to avoid a loose object?

(I'm not saying that the patch is unwanted, but only that the
justification is still not sufficiently complete.)

-- Hannes

Re: [PATCH] fast-import: Allow filemodify to set the root

[Jonathan Nieder]

Johannes Sixt wrote:
> Am 10/7/2010 22:28, schrieb Jonathan Nieder:

>> | For a command (like filter-branch --subdirectory-filter) that wants
>> | to commit a lot of trees that already exist in the object db, writing
>> | undeltified objects as loose files only to repack them later can
>> | involve a significant amount[*] of overhead.
>
> 1. But when an object already exists in the db, it won't be written again,
> will it?

In David's application, the trees already exist, but the commits are new.

> 2. Even though fast-import puts all (new) objects into a pack file, the
> pack is heavily sub-optimal, and you should repack -f anyway. So what's
> the point? Only to avoid a loose object?

To avoid thousands of loose objects.

> (I'm not saying that the patch is unwanted, but only that the
> justification is still not sufficiently complete.)

No problem - these questions are useful.  If the result is learning
that something else is responsible for the speedup David observed in
his script, that would not be a bad outcome after all.

I suppose supporting M 040000 <tree> "" and C <path> "" could still
be a good idea in that case anyway, for the convenience of front-end
authors.

Jonathan
who still hasn't reviewed the patch (sorry)

Re: [PATCH] fast-import: Allow filemodify to set the root

[Johannes Sixt]

Am 10/8/2010 9:05, schrieb Jonathan Nieder:
> Johannes Sixt wrote:
>> Am 10/7/2010 22:28, schrieb Jonathan Nieder:
> 
>>> | For a command (like filter-branch --subdirectory-filter) that wants
>>> | to commit a lot of trees that already exist in the object db, writing
>>> | undeltified objects as loose files only to repack them later can
>>> | involve a significant amount[*] of overhead.
>>
>> 1. But when an object already exists in the db, it won't be written again,
>> will it?
> 
> In David's application, the trees already exist, but the commits are new.

But then what has this to do with "allow filemodify to set the root"?

> I suppose supporting M 040000 <tree> "" and C <path> "" could still
> be a good idea in that case anyway, for the convenience of front-end
> authors.

What is the special new thing here? That "" means 'empty string' == 'tree
at the root'? If so:

1. Then this is the missing piece in the justification. Then I could buy
that the observed speed-up is due to the reuse of an existing tree object
(which avoids parsing it and re-constructing it from its pieces because
fast-imports syntax didn't allow it otherwise). But it has nothing to do
with new loose objects (the re-constructed object would be identical to an
existing one).

2. Without this patch, would this syntax create a tree object with a name
consisting of two double-quotes in the root? Or would it be a syntax
error? How would one construct such an entry with this patch?

-- Hannes

Re: [PATCH] fast-import: Allow filemodify to set the root

[Jonathan Nieder]

Some quick answers before I go to sleep.

Johannes Sixt wrote:

> What is the special new thing here? That "" means 'empty string' == 'tree
> at the root'? If so:

Yes.

> 1. Then this is the missing piece in the justification. Then I could buy
> that the observed speed-up is due to the reuse of an existing tree object
> (which avoids parsing it and re-constructing it from its pieces because
> fast-imports syntax didn't allow it otherwise). But it has nothing to do
> with new loose objects (the re-constructed object would be identical to an
> existing one).

If I remember correctly:

Version 1 of the script was similar to filter-branch and used
"git read-tree", "git write-tree", and "git commit-tree" (or something
equivalent).  Reconstructing the trees seemed to be a significant overhead,
leading to:

Version 2, which just called "git commit-tree" (or equivalent) repeatedly.
What this patch allows is

Version 3, which is a bit over 20% faster than version 2 and uses fast-import
to write the new commits directly to pack.

That speed-up could be due to avoiding the repeated fork() + exec() +
git startup cost, or avoiding loose objects, or any number of other
things.  Without this patch, one could write

Theoretical version 1', which uses cat-file --batch to unpack each
tree and uses fast-import to write commits with the same (recreated)
tree directly to pack.  But that would be kind of insane.

> 2. Without this patch, would this syntax create a tree object with a name
> consisting of two double-quotes in the root? Or would it be a syntax
> error?

Syntax error.

> How would one construct such an entry with this patch?

M 040000 <tree id> "\"\""

Thanks again.

Re: [PATCH] fast-import: Allow filemodify to set the root

[Ramkumar Ramachandra]

Hi David,

I'm sorry I didn't get the time to look at this earlier- just looked
at it now.

David Barr writes:
> Most git commands do their writing to the object db via the index and
> loose objects.  When you just have a pile of trees you want to convert
> into commits, this is wasteful; for performance-critical operations
> like filter-branch --subdirectory-filter, one might want a sort of
> hash-object --batch-to-pack to write a pack directly.
> 
> Fortunately we have fast-import (which is one of the only git commands
> that will write to a pack directly) but there is not an advertised way
> to tell fast-import to use a given tree for its commits.  So in
> current git, one has the unpleasant choice of writing loose objects
> without parsing the trees or writing straight to pack but having to
> parse trees to do it.
> 
> This patch changes that, by allowing
> 
> 	M 040000 <tree id> ""

It can be a <dataref> in general: either a SHA1 or a tree mark.

> as a filemodify line in a commit to reset to a particular tree without
> any need to unpack it.  For example,
> 
> 	M 040000 4b825dc642cb6eb9a060e54bf8d69288fbee4904 ""
> 
> is a synonym for the deleteall command.
> 
> Commit-message-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: David Barr <david.barr@cordelta.com>
> ---
>  fast-import.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)

I applied and tried it out- it works as expected. Here's a patch for
the documentation and a test. You might want to put the test in a
separate patch (in preparation for Jonathan's t9300 cleanup series).

@Jonathan: What happened to your series cleaning up t9300?

Tested-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

-- 8< --
diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 966ba4f..90a4666 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -524,6 +524,9 @@ start with double quote (`"`).
 If an `LF` or double quote must be encoded into `<path>` shell-style
 quoting should be used, e.g. `"path/with\n and \" in it"`.
 
+Additionally, in `040000` mode, `<path>` may also be an empty string
+(`""`) to specify the root of the tree.
+
 The value of `<path>` must be in canonical form. That is it must not:
 
 * contain an empty directory component (e.g. `foo//bar` is invalid),

diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 7c05920..3c0cf05 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -875,6 +875,27 @@ test_expect_success \
 	 compare_diff_raw expect actual'
 
 test_expect_success \
+	'N: copy root directory by tree hash' \
+	'cat >expect <<-\EOF &&
+	:100755 000000 f1fb5da718392694d0076d677d6d0e364c79b0bc 0000000000000000000000000000000000000000 D	file3/newf
+	:100644 000000 7123f7f44e39be127c5eb701e5968176ee9d78b1 0000000000000000000000000000000000000000 D	file3/oldf
+	EOF
+	 root=$(git rev-parse refs/heads/branch^0^{tree}) &&
+	 cat >input <<-INPUT_END &&
+	commit refs/heads/N6
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	copy root directory by tree hash
+	COMMIT
+
+	from refs/heads/branch^0
+	M 040000 $root ""
+	INPUT_END
+	 git fast-import <input &&
+	 git diff-tree -C --find-copies-harder -r N4 N6 >actual &&
+	 compare_diff_raw expect actual'
+
+test_expect_success \
 	'N: modify copied tree' \
 	'cat >expect <<-\EOF &&
 	:100644 100644 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 C100	newdir/interesting	file3/file5

> diff --git a/fast-import.c b/fast-import.c
> index 2317b0f..8f68a89 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -1454,6 +1454,15 @@ static int tree_content_set(
>  		n = slash1 - p;
>  	else
>  		n = strlen(p);
> +	if (!slash1 && !n) {
> +		if (!S_ISDIR(mode))
> +			die("Root cannot be a non-directory");
> +		hashcpy(root->versions[1].sha1, sha1);
> +		if (root->tree)
> +			release_tree_content_recursive(root->tree);
> +		root->tree = subtree;

Should there be a hashclr(root->versions[1].sha1) here? I saw it in
the other branches.

Looks good otherwise. I'm surprised fast-import didn't already have
this functionality.

-- Ram

Re: [PATCH] fast-import: Allow filemodify to set the root

[Gabriel Filion]

Hello,

On 2010-10-08 04:15, Ramkumar Ramachandra wrote:
> diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
> index 966ba4f..90a4666 100644
> --- a/Documentation/git-fast-import.txt
> +++ b/Documentation/git-fast-import.txt
> @@ -524,6 +524,9 @@ start with double quote (`"`).
>  If an `LF` or double quote must be encoded into `<path>` shell-style
>  quoting should be used, e.g. `"path/with\n and \" in it"`.
>  
> +Additionally, in `040000` mode, `<path>` may also be an empty string
> +(`""`) to specify the root of the tree.
> +
>  The value of `<path>` must be in canonical form. That is it must not:
>  
>  * contain an empty directory component (e.g. `foo//bar` is invalid),

Thanks for keeping documentation up to date :)

Although, I think the documentation should be a bit more precise about
what "specifying the root of the tree" means for a 'filemodify' command
and its implication on performance (i.e. why there's actually this
special case in the syntax)

-- 
Gabriel Filion

Re: [PATCH] fast-import: Allow filemodify to set the root

[David Michael Barr]

Hi,

>> diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
>> index 966ba4f..90a4666 100644
>> --- a/Documentation/git-fast-import.txt
>> +++ b/Documentation/git-fast-import.txt
>> @@ -524,6 +524,9 @@ start with double quote (`"`).
>> If an `LF` or double quote must be encoded into `<path>` shell-style
>> quoting should be used, e.g. `"path/with\n and \" in it"`.
>> 
>> +Additionally, in `040000` mode, `<path>` may also be an empty string
>> +(`""`) to specify the root of the tree.
>> +
>> The value of `<path>` must be in canonical form. That is it must not:
>> 
>> * contain an empty directory component (e.g. `foo//bar` is invalid),
> 
> Thanks for keeping documentation up to date :)
> 
> Although, I think the documentation should be a bit more precise about
> what "specifying the root of the tree" means for a 'filemodify' command
> and its implication on performance (i.e. why there's actually this
> special case in the syntax)

The way I perceive it, the patch simply removes the special treatment of
the root, allowing it to be modified just as any other path.
The only property that distinguishes the root from other paths is that the
endpoint must be a tree.

Re: [PATCH] fast-import: Allow filemodify to set the root

[Sverre Rabbelier]

Heya,

On Fri, Oct 8, 2010 at 10:58, David Michael Barr
<david.barr@cordelta.com> wrote:
> The way I perceive it, the patch simply removes the special treatment of
> the root, allowing it to be modified just as any other path.
> The only property that distinguishes the root from other paths is that the
> endpoint must be a tree.

So, are other trees allowed too? Can I set the contents of directory
"foo/bar/baz/" to an existing tree?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] fast-import: Allow filemodify to set the root

[Jonathan Nieder]

Sverre Rabbelier wrote:

> So, are other trees allowed too? Can I set the contents of directory
> "foo/bar/baz/" to an existing tree?

Yep, ever since v1.7.3-rc0~75^2 (Teach fast-import to import
subtrees named by tree id, 2010-06-30).

Re: [PATCH] fast-import: Allow filemodify to set the root

[David Michael Barr]

Should I resubmit this patch with the complete justification?

Add an extra paragraph like:

v1.7.3-rc0~75^2 (Teach fast-import to import
subtrees named by tree id, 2010-06-30)
has a shortcoming, it doesn't allow the root to be set.
Extend this behaviour by allowing the root to be
referenced as the empty path, "".

>  v1.7.3-rc0~75^2 (Teach fast-import to import
> subtrees named by tree id, 2010-06-30)

Re: [PATCH] fast-import: Allow filemodify to set the root

[Sverre Rabbelier]

Heya,

On Sun, Oct 10, 2010 at 00:11, David Michael Barr
<david.barr@cordelta.com> wrote:
> v1.7.3-rc0~75^2 (Teach fast-import to import
> subtrees named by tree id, 2010-06-30)
> has a shortcoming, it doesn't allow the root to be set.
> Extend this behaviour by allowing the root to be
> referenced as the empty path, "".

Yes, that would be a good addition.

-- 
Cheers,

Sverre Rabbelier

[PATCH] fast-import: Allow filemodify to set the root

[David Barr]

v1.7.3-rc0~75^2 (Teach fast-import to import subtrees named by tree id,
2010-06-30) has a shortcoming - it doesn't allow the root to be set.
Extend this behaviour by allowing the root to be referenced as the
empty path, "".

For a command (like filter-branch --subdirectory-filter) that wants
to commit a lot of trees that already exist in the object db, writing
undeltified objects as loose files only to repack them later can
involve a significant amount of overhead.
(23% slow-down observed on Linux 2.6.35, worse on Mac OS X 10.6)

Fortunately we have fast-import (which is one of the only git commands
that will write to a pack directly) but there is not an advertised way
to tell fast-import to commit a given tree without unpacking it.

This patch changes that, by allowing

	M 040000 <tree id> ""

as a filemodify line in a commit to reset to a particular tree without
any need to parse it.  For example,

	M 040000 4b825dc642cb6eb9a060e54bf8d69288fbee4904 ""

is a synonym for the deleteall command and the fast-import equivalent of

	git read-tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904

Signed-off-by: David Barr <david.barr@cordelta.com>
Commit-message-by: Jonathan Nieder <jrnieder@gmail.com>
Acked-by: Sverre Rabbelier <srabbelier@gmail.com>
Tested-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-fast-import.txt |    3 +++
 fast-import.c                     |    9 +++++++++
 t/t9300-fast-import.sh            |   21 +++++++++++++++++++++
 3 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 966ba4f..90a4666 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -524,6 +524,9 @@ start with double quote (`"`).
 If an `LF` or double quote must be encoded into `<path>` shell-style
 quoting should be used, e.g. `"path/with\n and \" in it"`.
 
+Additionally, in `040000` mode, `<path>` may also be an empty string
+(`""`) to specify the root of the tree.
+
 The value of `<path>` must be in canonical form. That is it must not:
 
 * contain an empty directory component (e.g. `foo//bar` is invalid),
diff --git a/fast-import.c b/fast-import.c
index 2317b0f..8f68a89 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1454,6 +1454,15 @@ static int tree_content_set(
 		n = slash1 - p;
 	else
 		n = strlen(p);
+	if (!slash1 && !n) {
+		if (!S_ISDIR(mode))
+			die("Root cannot be a non-directory");
+		hashcpy(root->versions[1].sha1, sha1);
+		if (root->tree)
+			release_tree_content_recursive(root->tree);
+		root->tree = subtree;
+		return 1;
+	}
 	if (!n)
 		die("Empty path component found in input");
 	if (!slash1 && !S_ISDIR(mode) && subtree)
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 7c05920..3c0cf05 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -875,6 +875,27 @@ test_expect_success \
 	 compare_diff_raw expect actual'
 
 test_expect_success \
+	'N: copy root directory by tree hash' \
+	'cat >expect <<-\EOF &&
+	:100755 000000 f1fb5da718392694d0076d677d6d0e364c79b0bc 0000000000000000000000000000000000000000 D	file3/newf
+	:100644 000000 7123f7f44e39be127c5eb701e5968176ee9d78b1 0000000000000000000000000000000000000000 D	file3/oldf
+	EOF
+	 root=$(git rev-parse refs/heads/branch^0^{tree}) &&
+	 cat >input <<-INPUT_END &&
+	commit refs/heads/N6
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	copy root directory by tree hash
+	COMMIT
+
+	from refs/heads/branch^0
+	M 040000 $root ""
+	INPUT_END
+	 git fast-import <input &&
+	 git diff-tree -C --find-copies-harder -r N4 N6 >actual &&
+	 compare_diff_raw expect actual'
+
+test_expect_success \
 	'N: modify copied tree' \
 	'cat >expect <<-\EOF &&
 	:100644 100644 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 C100	newdir/interesting	file3/file5
-- 
1.7.3

Re: [PATCH] fast-import: Allow filemodify to set the root

[Jonathan Nieder]

David Barr wrote:

> --- a/Documentation/git-fast-import.txt
> +++ b/Documentation/git-fast-import.txt
> @@ -524,6 +524,9 @@ start with double quote (`"`).
>  If an `LF` or double quote must be encoded into `<path>` shell-style
>  quoting should be used, e.g. `"path/with\n and \" in it"`.
>  
> +Additionally, in `040000` mode, `<path>` may also be an empty string
> +(`""`) to specify the root of the tree.
> +

Ideally this would be not so much "Additionally" as "For example".
Maybe just:

	An empty path ("") refers to the toplevel directory of
	the tracked tree.

> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -1454,6 +1454,15 @@ static int tree_content_set(
>  		n = slash1 - p;
>  	else
>  		n = strlen(p);
> +	if (!slash1 && !n) {
> +		if (!S_ISDIR(mode))
> +			die("Root cannot be a non-directory");
> +		hashcpy(root->versions[1].sha1, sha1);
> +		if (root->tree)
> +			release_tree_content_recursive(root->tree);
> +		root->tree = subtree;
> +		return 1;
> +	}
>  	if (!n)
>  		die("Empty path component found in input");

Background for the curious: tree_content_set() is a recursive function
to modify a tree-in-the-making by changing the entry at path p to
refer to some specified content with a given mode.  The recursion
works as one might expect:

	tree_content_set(root, "foo/bar/baz", ...) ->
	 tree_content_set(root:foo, "bar/baz", ...) ->
	  et c

The "if (!n)" check introduced in v1.5.1.3~11^2~1 (Don't allow empty
pathnames in fast-import, 2007-04-28) ensures fast-import doesn't
end up creating a subdirectory corresponding to an empty path
component in a pathname like "foo//bar/baz".

With this patch, an empty path component is allowed again, but only
as the last path component.  It is used to modify directories.  So,
for example,

	tree_content_set(root, "foo/bar/", sha1, S_IFDIR)

becomes an almost-synonym for

	tree_content_set(root, "foo/bar", sha1, S_IFDIR)

and

	tree_content_set(root, "foo/bar/", sha1, S_IFREG | 0644)

is rejected.

Why do I say almost-synonym?  Because as Ram pointed out, you are not
invalidating the parent tree hash, because there may not even _be_ a
parent tree.

In other words, with this patch, I worry that a

	M 040000 ...sha1... "foo/bar/"

line would be sometimes ignored and sometimes not.  Confusing.

Would it make sense to just handle the empty-path case in the callers
(file_change_m(), file_change_cr()) to avoid this?

[PATCH 0/2] Re: fast-import: Allow filemodify to set the root

[Jonathan Nieder]

Jonathan Nieder wrote:

> In other words, with this patch, I worry that a
> 
> 	M 040000 ...sha1... "foo/bar/"
> 
> line would be sometimes ignored and sometimes not.  Confusing.

Hey, my worries were unfounded!  Nice when that happens.

Patch 1 is an unrelated bugfix.

> Would it make sense to just handle the empty-path case in the callers
> (file_change_m(), file_change_cr()) to avoid this?

Patch 2 introduces this change anyway, to propose that change, to
avoid unnecessarily introducing a new "foo/bar/" syntax that does not
work with git 1.7.3.

I hope they are not too dull to read.
Jonathan Nieder (2):
  fast-import: filemodify after M 040000 <tree> "" crashes
  fast-import: tighten M 040000 syntax

 fast-import.c          |   54 +++++++++++++++++++++++++++++---------
 t/t9300-fast-import.sh |   68 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 109 insertions(+), 13 deletions(-)

-- 
1.7.2.3

[PATCH 1/2] fast-import: filemodify after M 040000 <tree> "" crashes

[Jonathan Nieder]

Until M 040000 <tree> "" syntax was introduced in commit 2794ad5
(fast-import: Allow filemodify to set the root, 2010-10-10), it
was impossible for the root entry to refer to an unloaded tree.
Update various functions to take that possibility into account.
Otherwise

	M 040000 <tree> ""
	M 100644 :1 "foo"

and similar commands (using D, C, or R after resetting the root
tree) segfault.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 fast-import.c          |   20 ++++++++++++++++----
 t/t9300-fast-import.sh |   38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 4 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 8f68a89..aaf47c5 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1444,7 +1444,7 @@ static int tree_content_set(
 	const uint16_t mode,
 	struct tree_content *subtree)
 {
-	struct tree_content *t = root->tree;
+	struct tree_content *t;
 	const char *slash1;
 	unsigned int i, n;
 	struct tree_entry *e;
@@ -1468,6 +1468,9 @@ static int tree_content_set(
 	if (!slash1 && !S_ISDIR(mode) && subtree)
 		die("Non-directories cannot have subtrees");
 
+	if (!root->tree)
+		load_tree(root);
+	t = root->tree;
 	for (i = 0; i < t->entry_count; i++) {
 		e = t->entries[i];
 		if (e->name->str_len == n && !strncmp(p, e->name->str_dat, n)) {
@@ -1523,7 +1526,7 @@ static int tree_content_remove(
 	const char *p,
 	struct tree_entry *backup_leaf)
 {
-	struct tree_content *t = root->tree;
+	struct tree_content *t;
 	const char *slash1;
 	unsigned int i, n;
 	struct tree_entry *e;
@@ -1534,6 +1537,9 @@ static int tree_content_remove(
 	else
 		n = strlen(p);
 
+	if (!root->tree)
+		load_tree(root);
+	t = root->tree;
 	for (i = 0; i < t->entry_count; i++) {
 		e = t->entries[i];
 		if (e->name->str_len == n && !strncmp(p, e->name->str_dat, n)) {
@@ -1581,7 +1587,7 @@ static int tree_content_get(
 	const char *p,
 	struct tree_entry *leaf)
 {
-	struct tree_content *t = root->tree;
+	struct tree_content *t;
 	const char *slash1;
 	unsigned int i, n;
 	struct tree_entry *e;
@@ -1592,6 +1598,9 @@ static int tree_content_get(
 	else
 		n = strlen(p);
 
+	if (!root->tree)
+		load_tree(root);
+	t = root->tree;
 	for (i = 0; i < t->entry_count; i++) {
 		e = t->entries[i];
 		if (e->name->str_len == n && !strncmp(p, e->name->str_dat, n)) {
@@ -2056,13 +2065,16 @@ 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;
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 3c0cf05..de95a6e 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -928,6 +928,43 @@ test_expect_success \
 	 git diff-tree -C --find-copies-harder -r N5^^ N5 >actual &&
 	 compare_diff_raw expect actual'
 
+test_expect_success \
+	'N: copy to root by id and modify' \
+	'echo "hello, world" >expect.foo &&
+	 echo hello >expect.bar &&
+	 git fast-import <<-SETUP_END &&
+	commit refs/heads/N7
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	hello, tree
+	COMMIT
+
+	deleteall
+	M 644 inline foo/bar
+	data <<EOF
+	hello
+	EOF
+	SETUP_END
+
+	 tree=$(git rev-parse --verify N7:) &&
+	 git fast-import <<-INPUT_END &&
+	commit refs/heads/N8
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	copy to root by id and modify
+	COMMIT
+
+	M 040000 $tree ""
+	M 644 inline foo/foo
+	data <<EOF
+	hello, world
+	EOF
+	INPUT_END
+	 git show N8:foo/foo >actual.foo &&
+	 git show N8:foo/bar >actual.bar &&
+	 test_cmp expect.foo actual.foo &&
+	 test_cmp expect.bar actual.bar'
+
 ###
 ### series O
 ###
-- 
1.7.2.3

Re: [PATCH 1/2] fast-import: filemodify after M 040000 <tree> "" crashes

[Sverre Rabbelier]

Heya,

On Sun, Oct 17, 2010 at 20:03, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Until M 040000 <tree> "" syntax was introduced in commit 2794ad5
> (fast-import: Allow filemodify to set the root, 2010-10-10), it
> was impossible for the root entry to refer to an unloaded tree.
> Update various functions to take that possibility into account.
> Otherwise
>
>        M 040000 <tree> ""
>        M 100644 :1 "foo"
>
> and similar commands (using D, C, or R after resetting the root
> tree) segfault.

I'm curious, how come this not found earlier? Would seem like a fairly
regular operation?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 1/2] fast-import: filemodify after M 040000 <tree> "" crashes

[Jonathan Nieder]

Sverre Rabbelier wrote:
> On Sun, Oct 17, 2010 at 20:03, Jonathan Nieder <jrnieder@gmail.com> wrote:

>>        M 040000 <tree> ""
>>        M 100644 :1 "foo"
>>
>> and similar commands (using D, C, or R after resetting the root
>> tree) segfault.
>
> I'm curious, how come this not found earlier? Would seem like a fairly
> regular operation?

Sure, though that isn't what the feature has been used for so far.

Hooray for tests. :)

-- 8< --
Subject: t9300 (fast-import): another test for the "replace root" feature

Another test for the replace root feature.  One can imagine an
implementation for which R "some/subdir" "" would free some state
associated to the subdir and leave fast-import confused.

Luckily, git's is not such an implementation.

While at it, change the previous test to use C "some/subdir" ""
instead of R (i.e., test both syntaxes).

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t9300-fast-import.sh |   43 ++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 42 insertions(+), 1 deletions(-)

diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index ce09457..dd90a09 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -990,11 +990,52 @@ test_expect_success \
 	COMMIT
 
 	M 040000 $branch ""
-	R "newdir" ""
+	C "newdir" ""
 	INPUT_END
 	 git fast-import <input &&
 	 git diff --exit-code branch:newdir N9'
 
+test_expect_success \
+	'N: modify subtree, extract it, and modify again' \
+	'echo hello >expect.baz &&
+	 echo hello, world >expect.qux &&
+	 git fast-import <<-SETUP_END &&
+	commit refs/heads/N10
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	hello, tree
+	COMMIT
+
+	deleteall
+	M 644 inline foo/bar/baz
+	data <<EOF
+	hello
+	EOF
+	SETUP_END
+
+	 tree=$(git rev-parse --verify N10:) &&
+	 git fast-import <<-INPUT_END &&
+	commit refs/heads/N11
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	copy to root by id and modify
+	COMMIT
+
+	M 040000 $tree ""
+	M 100644 inline foo/bar/qux
+	data <<EOF
+	hello, world
+	EOF
+	R "foo" ""
+	C "bar/qux" "bar/quux"
+	INPUT_END
+	 git show N11:bar/baz >actual.baz &&
+	 git show N11:bar/qux >actual.qux &&
+	 git show N11:bar/quux >actual.quux &&
+	 test_cmp expect.baz actual.baz &&
+	 test_cmp expect.qux actual.qux &&
+	 test_cmp expect.qux actual.quux'
+
 ###
 ### series O
 ###
-- 
1.7.2.3

[PATCH] fast-import: do not clear notes in do_change_note_fanout()

[Jonathan Nieder]

Commit 5edde51 (fast-import: filemodify after M 040000 <tree> ""
crashes, 2010-10-17) taught fast-import to load trees from the
object db as needed when it is time to access them.

But it went too far.  In change_note_fanout(), an empty,
not-loaded tree is not meant to destroy notes, so calling
load_tree() at that point is exactly the wrong thing to do.

Kudos to Johan Herland for t9301, which caught this failure.

Reported-by: Thomas Rast <trast@student.ethz.ch>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
[cleared cc list.]

Jonathan Nieder wrote:

> +++ b/fast-import.c
[...]
> @@ -2056,13 +2065,16 @@ 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++) {

Oops.  The !t case is normal here and certainly is not a request
to turn t into an empty tree.  Here's a minimal fix.

 fast-import.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index aaf47c5..d2458ea 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2065,16 +2065,13 @@ static uintmax_t do_change_note_fanout(
 		char *fullpath, unsigned int fullpath_len,
 		unsigned char fanout)
 {
-	struct tree_content *t;
+	struct tree_content *t = root->tree;
 	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;
-- 
1.7.2.3

[PATCH 2/2] fast-import: tighten M 040000 syntax

[Jonathan Nieder]

When tree_content_set() is asked to modify the path "foo/bar/",
it first recurses like so:

	tree_content_set(root, "foo/bar/", sha1, S_IFDIR) ->
	 tree_content_set(root:foo, "bar/", ...) ->
	  tree_content_set(root:foo/bar, "", ...)

And as a side-effect of 2794ad5 (fast-import: Allow filemodify to set
the root, 2010-10-10), this last call is accepted and changes
the tree entry for root:foo/bar to refer to the specified tree.

That seems safe enough but let's reject the new syntax (we never meant
to support it) and make it harder for frontends to introduce pointless
incompatibilities with git fast-import 1.7.3.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 fast-import.c          |   34 +++++++++++++++++++++++++---------
 t/t9300-fast-import.sh |   30 ++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 9 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index aaf47c5..cb947c1 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1437,6 +1437,20 @@ static void store_tree(struct tree_entry *root)
 	t->entry_count -= del;
 }
 
+static void tree_content_replace(
+	struct tree_entry *root,
+	const unsigned char *sha1,
+	const uint16_t mode,
+	struct tree_content *newtree)
+{
+	if (!S_ISDIR(mode))
+		die("Root cannot be a non-directory");
+	hashcpy(root->versions[1].sha1, sha1);
+	if (root->tree)
+		release_tree_content_recursive(root->tree);
+	root->tree = newtree;
+}
+
 static int tree_content_set(
 	struct tree_entry *root,
 	const char *p,
@@ -1454,15 +1468,6 @@ static int tree_content_set(
 		n = slash1 - p;
 	else
 		n = strlen(p);
-	if (!slash1 && !n) {
-		if (!S_ISDIR(mode))
-			die("Root cannot be a non-directory");
-		hashcpy(root->versions[1].sha1, sha1);
-		if (root->tree)
-			release_tree_content_recursive(root->tree);
-		root->tree = subtree;
-		return 1;
-	}
 	if (!n)
 		die("Empty path component found in input");
 	if (!slash1 && !S_ISDIR(mode) && subtree)
@@ -2230,6 +2235,10 @@ static void file_change_m(struct branch *b)
 				command_buf.buf);
 	}
 
+	if (!*p) {
+		tree_content_replace(&b->branch_tree, sha1, mode, NULL);
+		return;
+	}
 	tree_content_set(&b->branch_tree, p, sha1, mode, NULL);
 }
 
@@ -2288,6 +2297,13 @@ static void file_change_cr(struct branch *b, int rename)
 		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" "" */
+		tree_content_replace(&b->branch_tree,
+			leaf.versions[1].sha1,
+			leaf.versions[1].mode,
+			leaf.tree);
+		return;
+	}
 	tree_content_set(&b->branch_tree, d,
 		leaf.versions[1].sha1,
 		leaf.versions[1].mode,
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 1df11ad..ce09457 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -929,6 +929,20 @@ test_expect_success \
 	 compare_diff_raw expect actual'
 
 test_expect_success \
+	'N: reject foo/ syntax' \
+	'subdir=$(git rev-parse refs/heads/branch^0:file2) &&
+	 test_must_fail git fast-import <<-INPUT_END
+	commit refs/heads/N5B
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	copy with invalid syntax
+	COMMIT
+
+	from refs/heads/branch^0
+	M 040000 $subdir file3/
+	INPUT_END'
+
+test_expect_success \
 	'N: copy to root by id and modify' \
 	'echo "hello, world" >expect.foo &&
 	 echo hello >expect.bar &&
@@ -965,6 +979,22 @@ test_expect_success \
 	 test_cmp expect.foo actual.foo &&
 	 test_cmp expect.bar actual.bar'
 
+test_expect_success \
+	'N: extract subtree' \
+	'branch=$(git rev-parse --verify refs/heads/branch^{tree}) &&
+	 cat >input <<-INPUT_END &&
+	commit refs/heads/N9
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	extract subtree branch:newdir
+	COMMIT
+
+	M 040000 $branch ""
+	R "newdir" ""
+	INPUT_END
+	 git fast-import <input &&
+	 git diff --exit-code branch:newdir N9'
+
 ###
 ### series O
 ###
-- 
1.7.2.3

[PATCH] Documentation/fast-import: put explanation of M 040000 <dataref> "" in context

[Jonathan Nieder]

Omit needless words ("Additionally ... <path> may also" is redundant).
While at it, place the explanation of this special case after the
general rules for paths to provide the reader with some context.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
A small language tweak.

 Documentation/git-fast-import.txt |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index f56dfca..e2a46a5 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -534,9 +534,6 @@ start with double quote (`"`).
 If an `LF` or double quote must be encoded into `<path>` shell-style
 quoting should be used, e.g. `"path/with\n and \" in it"`.
 
-Additionally, in `040000` mode, `<path>` may also be an empty string
-(`""`) to specify the root of the tree.
-
 The value of `<path>` must be in canonical form. That is it must not:
 
 * contain an empty directory component (e.g. `foo//bar` is invalid),
@@ -545,6 +542,9 @@ The value of `<path>` must be in canonical form. That is it must not:
 * contain the special component `.` or `..` (e.g. `foo/./bar` and
   `foo/../bar` are invalid).
 
+In `040000` mode, `<path>` can be the empty string (`""`)
+to specify the root of the tree.
+
 It is recommended that `<path>` always be encoded using UTF-8.
 
 `filedelete`
-- 
1.7.4.rc2

Re: [PATCH] Documentation/fast-import: put explanation of M 040000 <dataref> "" in context

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Omit needless words ("Additionally ... <path> may also" is redundant).
> While at it, place the explanation of this special case after the
> general rules for paths to provide the reader with some context.
>
> diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
> index f56dfca..e2a46a5 100644
> --- a/Documentation/git-fast-import.txt
> +++ b/Documentation/git-fast-import.txt
> @@ -545,6 +542,9 @@ The value of `<path>` must be in canonical form. That is it must not:
>  * contain the special component `.` or `..` (e.g. `foo/./bar` and
>    `foo/../bar` are invalid).
>  
> +In `040000` mode, `<path>` can be the empty string (`""`)
> +to specify the root of the tree.
> +

Thanks, but is "In x mode" an appropriate wording here in the first place?
I would expect x to be some action (e.g. In navigation mode, typing many
keys just beep, and in edit mode, typing most keys just insert -- vi) but
the word "mode" doesn't fit that pattern (unless we take a bitpattern "x"
to mean "the act of creating a path with mode bits x").

Re: [PATCH] Documentation/fast-import: put explanation of M 040000 <dataref> "" in context

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> +++ b/Documentation/git-fast-import.txt
>> @@ -545,6 +542,9 @@ The value of `<path>` must be in canonical form. That is it must not:
>>  * contain the special component `.` or `..` (e.g. `foo/./bar` and
>>    `foo/../bar` are invalid).
>>  
>> +In `040000` mode, `<path>` can be the empty string (`""`)
>> +to specify the root of the tree.
>> +
>
> Thanks, but is "In x mode" an appropriate wording here in the first place?

Good point.  No, it isn't.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
For squashing.

 Documentation/git-fast-import.txt |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 43d2174..7068de3 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -542,7 +542,7 @@ The value of `<path>` must be in canonical form. That is it must not:
 * contain the special component `.` or `..` (e.g. `foo/./bar` and
   `foo/../bar` are invalid).
 
-In `040000` mode, `<path>` can be the empty string (`""`)
+If `<mode>` is `040000`, `<path>` can be the empty string (`""`)
 to specify the root of the tree.
 
 It is recommended that `<path>` always be encoded using UTF-8.
-- 
1.7.4.rc2

Re: [PATCH] Documentation/fast-import: put explanation of M 040000 <dataref> "" in context

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
> index 43d2174..7068de3 100644
> --- a/Documentation/git-fast-import.txt
> +++ b/Documentation/git-fast-import.txt
> @@ -542,7 +542,7 @@ The value of `<path>` must be in canonical form. That is it must not:
>  * contain the special component `.` or `..` (e.g. `foo/./bar` and
>    `foo/../bar` are invalid).
>  
> -In `040000` mode, `<path>` can be the empty string (`""`)
> +If `<mode>` is `040000`, `<path>` can be the empty string (`""`)
>  to specify the root of the tree.

For whom is this paragraph written?  People who produce g-f-i streams, or
people who interpret somebody else's g-f-i streams?

I am wondering if it is easier to read if we consistently target the
former, something like this:

        The root of the tree can be represented by `<mode> 040000` with an
        empty string as `<path>`.

because all the surrounding description seems to describe rules
(e.g. "quoting should be used", "The value of `<path>` must be ...").

Re: [PATCH] Documentation/fast-import: put explanation of M 040000 <dataref> "" in context

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> +++ b/Documentation/git-fast-import.txt
>> @@ -542,7 +542,7 @@ The value of `<path>` must be in canonical form. That is it must not:
>>  * contain the special component `.` or `..` (e.g. `foo/./bar` and
>>    `foo/../bar` are invalid).
>>  
>> -In `040000` mode, `<path>` can be the empty string (`""`)
>> +If `<mode>` is `040000`, `<path>` can be the empty string (`""`)
>>  to specify the root of the tree.
>
> For whom is this paragraph written?  People who produce g-f-i streams, or
> people who interpret somebody else's g-f-i streams?
> 
> I am wondering if it is easier to read if we consistently target the
> former, something like this:
> 
>         The root of the tree can be represented by `<mode> 040000` with an
>         empty string as `<path>`.

Sounds good to me.  I suppose the clause "by <mode> 040000" should just
be left out --- we already explained that directories must have mode
S_IFDIR a few lines earlier.