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

STALE3748d

7 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Felipe Contreras <hidden>
Date: 2016-06-15 22:45:48

Signed-off-by: Felipe Contreras <redacted>
---
 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.

From: Felipe Contreras <hidden>
Date: 2016-06-15 22:45:48

Signed-off-by: Felipe Contreras <redacted>
---
 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 1/2] fast-import: add special mode; copy from parent.

From: Shawn O. Pearce <hidden>
Date: 2016-06-15 22:45:48

Felipe Contreras [off-list ref] 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 2/2] fast-import: add special '-' blob reference to use the previous one.

From: Shawn O. Pearce <hidden>
Date: 2016-06-15 22:45:48

Felipe Contreras [off-list ref] wrote:

quoted hunk

@@ -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...

quoted hunk

@@ -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.

From: Felipe Contreras <hidden>
Date: 2016-06-15 22:45:48

On Mon, Dec 22, 2008 at 12:11 AM, Shawn O. Pearce [off-list ref] wrote:

Felipe Contreras [off-list ref] wrote:

quoted

@@ -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.

quoted

@@ -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.

From: Felipe Contreras <hidden>
Date: 2016-06-15 22:45:49

On Mon, Dec 22, 2008 at 12:07 AM, Shawn O. Pearce [off-list ref] wrote:

Felipe Contreras [off-list ref] wrote:

quoted

+     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.

From: Felipe Contreras <hidden>
Date: 2016-06-15 22:45:49

On Sun, Dec 21, 2008 at 4:11 AM, Felipe Contreras
[off-list ref] wrote:

Signed-off-by: Felipe Contreras <redacted>
---
 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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help