[PATCH 1/2] fast-import: add special mode; copy from parent.

[PATCH 1/2] fast-import: add special mode; copy from parent.

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-fast-import.txt |    1 +
 fast-import.c                     |   41 +++++++++++++++++++++---------------
 2 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index c2f483a..9d4231e 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -484,6 +484,7 @@ in octal.  Git only supports the following modes:
 * `160000`: A gitlink, SHA-1 of the object refers to a commit in
   another repository. Git links can only be specified by SHA or through
   a commit mark. They are used to implement submodules.
+* `-`: A special mode; copy the parent's mode.
 
 In both formats `<path>` is the complete path of the file to be added
 (if not already existing) or modified (if already existing).
diff --git a/fast-import.c b/fast-import.c
index 3c035a5..a00d3fe 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1356,7 +1356,7 @@ static int tree_content_set(
 	struct tree_entry *root,
 	const char *p,
 	const unsigned char *sha1,
-	const uint16_t mode,
+	uint16_t mode,
 	struct tree_content *subtree)
 {
 	struct tree_content *t = root->tree;
@@ -1382,7 +1382,9 @@ static int tree_content_set(
 						&& e->versions[1].mode == mode
 						&& !hashcmp(e->versions[1].sha1, sha1))
 					return 0;
-				e->versions[1].mode = mode;
+				if (mode == S_IFREG)
+					mode = e->versions[0].mode;
+				e->versions[1].mode = (mode == S_IFREG) ? (S_IFREG | 0644) : mode;
 				hashcpy(e->versions[1].sha1, sha1);
 				if (e->tree)
 					release_tree_content_recursive(e->tree);
@@ -1417,7 +1419,7 @@ static int tree_content_set(
 		tree_content_set(e, slash1 + 1, sha1, mode, subtree);
 	} else {
 		e->tree = subtree;
-		e->versions[1].mode = mode;
+		e->versions[1].mode = (mode == S_IFREG) ? (S_IFREG | 0644) : mode;
 		hashcpy(e->versions[1].sha1, sha1);
 	}
 	hashclr(root->versions[1].sha1);
@@ -1862,20 +1864,25 @@ static void file_change_m(struct branch *b)
 	unsigned char sha1[20];
 	uint16_t mode, inline_data = 0;
 
-	p = get_mode(p, &mode);
-	if (!p)
-		die("Corrupt mode: %s", command_buf.buf);
-	switch (mode) {
-	case S_IFREG | 0644:
-	case S_IFREG | 0755:
-	case S_IFLNK:
-	case S_IFGITLINK:
-	case 0644:
-	case 0755:
-		/* ok */
-		break;
-	default:
-		die("Corrupt mode: %s", command_buf.buf);
+	if (!prefixcmp(p, "- ")) {
+		mode = 0;
+		p += 2;
+	} else {
+		p = get_mode(p, &mode);
+		if (!p)
+			die("Corrupt mode: %s", command_buf.buf);
+		switch (mode) {
+		case S_IFREG | 0644:
+		case S_IFREG | 0755:
+		case S_IFLNK:
+		case S_IFGITLINK:
+		case 0644:
+		case 0755:
+			/* ok */
+			break;
+		default:
+			die("Corrupt mode: %s", command_buf.buf);
+		}
 	}
 
 	if (*p == ':') {
-- 
1.6.0.6.3.g7ccbd

[PATCH 2/2] fast-import: add special '-' blob reference to use the previous one.

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-fast-import.txt |    6 +++---
 fast-import.c                     |   13 ++++++++++---
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 9d4231e..6fce41f 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -457,9 +457,9 @@ External data format::
 	'M' SP <mode> SP <dataref> SP <path> LF
 ....
 +
-Here `<dataref>` can be either a mark reference (`:<idnum>`)
-set by a prior `blob` command, or a full 40-byte SHA-1 of an
-existing Git blob object.
+Here `<dataref>` can be either a mark reference (`:<idnum>`) set by a prior
+`blob` command, a special `-` reference to use the one of the ancestor, or
+a full 40-byte SHA-1 of an existing Git blob object.
 
 Inline data format::
 	The data content for the file has not been supplied yet.
diff --git a/fast-import.c b/fast-import.c
index a00d3fe..228c474 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1385,7 +1385,8 @@ static int tree_content_set(
 				if (mode == S_IFREG)
 					mode = e->versions[0].mode;
 				e->versions[1].mode = (mode == S_IFREG) ? (S_IFREG | 0644) : mode;
-				hashcpy(e->versions[1].sha1, sha1);
+				hashcpy(e->versions[1].sha1,
+					is_null_sha1(sha1) ? e->versions[0].sha1 : sha1);
 				if (e->tree)
 					release_tree_content_recursive(e->tree);
 				e->tree = subtree;
@@ -1420,7 +1421,8 @@ static int tree_content_set(
 	} else {
 		e->tree = subtree;
 		e->versions[1].mode = (mode == S_IFREG) ? (S_IFREG | 0644) : mode;
-		hashcpy(e->versions[1].sha1, sha1);
+		hashcpy(e->versions[1].sha1,
+			is_null_sha1(sha1) ? e->versions[0].sha1 : sha1);
 	}
 	hashclr(root->versions[1].sha1);
 	return 1;
@@ -1862,7 +1864,7 @@ static void file_change_m(struct branch *b)
 	const char *endp;
 	struct object_entry *oe = oe;
 	unsigned char sha1[20];
-	uint16_t mode, inline_data = 0;
+	uint16_t mode, inline_data = 0, empty_blob = 0;
 
 	if (!prefixcmp(p, "- ")) {
 		mode = 0;
@@ -1893,6 +1895,10 @@ static void file_change_m(struct branch *b)
 	} else if (!prefixcmp(p, "inline")) {
 		inline_data = 1;
 		p += 6;
+	} else if (!prefixcmp(p, "- ")) {
+		hashclr(sha1);
+		empty_blob = 1;
+		p += 1;
 	} else {
 		if (get_sha1_hex(p, sha1))
 			die("Invalid SHA1: %s", command_buf.buf);
@@ -1936,6 +1942,7 @@ static void file_change_m(struct branch *b)
 		if (oe->type != OBJ_BLOB)
 			die("Not a blob (actually a %s): %s",
 				typename(oe->type), command_buf.buf);
+	} else if (empty_blob) {
 	} else {
 		enum object_type type = sha1_object_info(sha1, NULL);
 		if (type < 0)
-- 
1.6.0.6.3.g7ccbd

Re: [PATCH 2/2] fast-import: add special '-' blob reference to use the previous one.

[Shawn O. Pearce]

Felipe Contreras <felipe.contreras@gmail.com> wrote:
> @@ -1862,7 +1864,7 @@ static void file_change_m(struct branch *b)
>  	const char *endp;
>  	struct object_entry *oe = oe;
>  	unsigned char sha1[20];
> -	uint16_t mode, inline_data = 0;
> +	uint16_t mode, inline_data = 0, empty_blob = 0;

Its not the empty blob, its the inherited/assumed blob...
  
> @@ -1893,6 +1895,10 @@ static void file_change_m(struct branch *b)
>  	} else if (!prefixcmp(p, "inline")) {
>  		inline_data = 1;
>  		p += 6;
> +	} else if (!prefixcmp(p, "- ")) {
> +		hashclr(sha1);
> +		empty_blob = 1;
> +		p += 1;

Hmph, so if create a new path with a blob of "-" the repository
will be corrupt because the zero id was used and error was produced.

Actually I think you have the same bug in the prior patch with the
mode being inherited.  I wonder if we shouldn't put error checking
in too to validate that versions[0] describes a file entry.

-- 
Shawn.

Re: [PATCH 2/2] fast-import: add special '-' blob reference to use the previous one.

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Hmph, so if create a new path with a blob of "-" the repository
> will be corrupt because the zero id was used and error was produced.
>
> Actually I think you have the same bug in the prior patch with the
> mode being inherited.  I wonder if we shouldn't put error checking
> in too to validate that versions[0] describes a file entry.

Why are these patches necessary?

The proposed commit message describes what it does, but does not give hint
to even guess being able to use this new feature helps in what situation.
As far as I can see, these changes allow the exporter to say "this aspect
of the new data is the same as the previous one", but I thought that the
way in which fast-import works already revolves around "you have this
tree, and the next tree is different from it in this and that way."  Why
does one need be able to mention "this is the same as the previous one"
explicitly in the first place?

Re: [PATCH 2/2] fast-import: add special '-' blob reference to use the previous one.

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> 
> > Hmph, so if create a new path with a blob of "-" the repository
> > will be corrupt because the zero id was used and error was produced.
> >
> > Actually I think you have the same bug in the prior patch with the
> > mode being inherited.  I wonder if we shouldn't put error checking
> > in too to validate that versions[0] describes a file entry.
> 
> Why are these patches necessary?
> 
> The proposed commit message describes what it does, but does not give hint
> to even guess being able to use this new feature helps in what situation.
> As far as I can see, these changes allow the exporter to say "this aspect
> of the new data is the same as the previous one", but I thought that the
> way in which fast-import works already revolves around "you have this
> tree, and the next tree is different from it in this and that way."  Why
> does one need be able to mention "this is the same as the previous one"
> explicitly in the first place?

Hmm.  Actually, imagine you were dumping from git-diff output style
stream into a fast-import stream.

If a file changes only content, the mode is shown in the index line.
Yay us.  But what if the index line wasn't present in the diff? You
don't know the prior mode of the file, but you do have its content.

If a file changes only mode, we get no content hints in the diff.
How do you send that into fast-import without making the frontend
keep track of every path's current mode?

Though I agree, these details should be described in the commit
messages, not left as an exercise for the maintainer to make up.

-- 
Shawn.

Re: [PATCH 2/2] fast-import: add special '-' blob reference to use the previous one.

[Felipe Contreras]

On Mon, Dec 22, 2008 at 12:33 AM, Shawn O. Pearce <spearce@spearce.org> wrote:
> Junio C Hamano <gitster@pobox.com> wrote:
>> "Shawn O. Pearce" <spearce@spearce.org> writes:
>>
>> > Hmph, so if create a new path with a blob of "-" the repository
>> > will be corrupt because the zero id was used and error was produced.
>> >
>> > Actually I think you have the same bug in the prior patch with the
>> > mode being inherited.  I wonder if we shouldn't put error checking
>> > in too to validate that versions[0] describes a file entry.
>>
>> Why are these patches necessary?

Yeah, I realized I didn't explain that after sending the patches.

>> The proposed commit message describes what it does, but does not give hint
>> to even guess being able to use this new feature helps in what situation.
>> As far as I can see, these changes allow the exporter to say "this aspect
>> of the new data is the same as the previous one", but I thought that the
>> way in which fast-import works already revolves around "you have this
>> tree, and the next tree is different from it in this and that way."  Why
>> does one need be able to mention "this is the same as the previous one"
>> explicitly in the first place?
>
> Hmm.  Actually, imagine you were dumping from git-diff output style
> stream into a fast-import stream.
>
> If a file changes only content, the mode is shown in the index line.
> Yay us.  But what if the index line wasn't present in the diff? You
> don't know the prior mode of the file, but you do have its content.
>
> If a file changes only mode, we get no content hints in the diff.
> How do you send that into fast-import without making the frontend
> keep track of every path's current mode?
>
> Though I agree, these details should be described in the commit
> messages, not left as an exercise for the maintainer to make up.

Exactly. That's what happens with monotone; you usually have the
contents or the new mode, but not both at the same time.

-- 
Felipe Contreras

Re: [PATCH 2/2] fast-import: add special '-' blob reference to use the previous one.

[Felipe Contreras]

On Mon, Dec 22, 2008 at 12:11 AM, Shawn O. Pearce <spearce@spearce.org> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> wrote:
>> @@ -1862,7 +1864,7 @@ static void file_change_m(struct branch *b)
>>       const char *endp;
>>       struct object_entry *oe = oe;
>>       unsigned char sha1[20];
>> -     uint16_t mode, inline_data = 0;
>> +     uint16_t mode, inline_data = 0, empty_blob = 0;
>
> Its not the empty blob, its the inherited/assumed blob...

Right. I thought: in order to use the inherited blob, you should not
specify any blob (leave it empty, or blank).

But yeah, 'inherited' does the job too.

>> @@ -1893,6 +1895,10 @@ static void file_change_m(struct branch *b)
>>       } else if (!prefixcmp(p, "inline")) {
>>               inline_data = 1;
>>               p += 6;
>> +     } else if (!prefixcmp(p, "- ")) {
>> +             hashclr(sha1);
>> +             empty_blob = 1;
>> +             p += 1;
>
> Hmph, so if create a new path with a blob of "-" the repository
> will be corrupt because the zero id was used and error was produced.
>
> Actually I think you have the same bug in the prior patch with the
> mode being inherited.  I wonder if we shouldn't put error checking
> in too to validate that versions[0] describes a file entry.

Yes, in my tests I found that issue in the previous patch and I have a
fix for that (set a default mode), but I haven't fixed this one. Do
you know what should be the behavior? I think it should 'die'.

-- 
Felipe Contreras

Re: [PATCH 1/2] fast-import: add special mode; copy from parent.

[Shawn O. Pearce]

Felipe Contreras <felipe.contreras@gmail.com> wrote:
> +	if (!prefixcmp(p, "- ")) {
> +		mode = 0;
> +		p += 2;

This part made me wonder, why are we always doing "S_IFREG | mode"
further down?

> +	} else {
> +		p = get_mode(p, &mode);
> +		if (!p)
> +			die("Corrupt mode: %s", command_buf.buf);
> +		switch (mode) {
> +		case S_IFREG | 0644:
> +		case S_IFREG | 0755:
> +		case S_IFLNK:
> +		case S_IFGITLINK:
> +		case 0644:
> +		case 0755:
> +			/* ok */
> +			break;
> +		default:
> +			die("Corrupt mode: %s", command_buf.buf);
> +		}

-- 
Shawn.

Re: [PATCH 1/2] fast-import: add special mode; copy from parent.

[Felipe Contreras]

On Mon, Dec 22, 2008 at 12:07 AM, Shawn O. Pearce <spearce@spearce.org> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> wrote:
>> +     if (!prefixcmp(p, "- ")) {
>> +             mode = 0;
>> +             p += 2;
>
> This part made me wonder, why are we always doing "S_IFREG | mode"
> further down?

My guess is because 0644 and 0755; doing S_IFREG | mode doesn't
achieve anything for the other modes.

I just sent a patch that I hope makes that more visible.

-- 
Felipe Contreras

Re: [PATCH 1/2] fast-import: add special mode; copy from parent.

[Felipe Contreras]

On Sun, Dec 21, 2008 at 4:11 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  Documentation/git-fast-import.txt |    1 +
>  fast-import.c                     |   41 +++++++++++++++++++++---------------
>  2 files changed, 25 insertions(+), 17 deletions(-)

Please disregard this patch, it doesn't work as I expected. I'll send
an updated one soon.

-- 
Felipe Contreras