{"thread":{"id":"16816","subject":"[PATCH 1/2] fast-import: add special mode; copy from parent.","startedAt":"2008-12-21T02:11:41Z","lastAt":"2008-12-22T02:25:11Z","messageCount":10,"participants":["Felipe Contreras","Shawn O. Pearce","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"98469","messageId":"1229825502-963-1-git-send-email-felipe.contreras@gmail.com","threadId":"16816","inReplyTo":null,"subject":"[PATCH 1/2] fast-import: add special mode; copy from parent.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2008-12-21T02:11:41Z","receivedAt":"2008-12-21T02:11:41Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Signed-off-by: Felipe Contreras \n---\n Documentation/git-fast-import.txt | 1 +\n fast-import.c | 41 +++++++++++++++++++++---------------\n 2 files changed, 25 insertions(+), 17 deletions(-)\n\ndiff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\nindex c2f483a..9d4231e 100644\n--- a/Documentation/git-fast-import.txt\n+++ b/Documentation/git-fast-import.txt\n@@ -484,6 +484,7 @@ in octal. Git only supports the following modes:\n * `160000`: A gitlink, SHA-1 of the object refers to a commit in\n another repository. Git links can only be specified by SHA or through\n a commit mark. They are used to implement submodules.\n+* `-`: A special mode; copy the parent's mode.\n \n In both formats `` is the complete path of the file to be added\n (if not already existing) or modified (if already existing).\ndiff --git a/fast-import.c b/fast-import.c\nindex 3c035a5..a00d3fe 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1356,7 +1356,7 @@ static int tree_content_set(\n \tstruct tree_entry *root,\n \tconst char *p,\n \tconst unsigned char *sha1,\n-\tconst uint16_t mode,\n+\tuint16_t mode,\n \tstruct tree_content *subtree)\n {\n \tstruct tree_content *t = root->tree;\n@@ -1382,7 +1382,9 @@ static int tree_content_set(\n \t\t\t\t\t\t&& e->versions[1].mode == mode\n \t\t\t\t\t\t&& !hashcmp(e->versions[1].sha1, sha1))\n \t\t\t\t\treturn 0;\n-\t\t\t\te->versions[1].mode = mode;\n+\t\t\t\tif (mode == S_IFREG)\n+\t\t\t\t\tmode = e->versions[0].mode;\n+\t\t\t\te->versions[1].mode = (mode == S_IFREG) ? (S_IFREG | 0644) : mode;\n \t\t\t\thashcpy(e->versions[1].sha1, sha1);\n \t\t\t\tif (e->tree)\n \t\t\t\t\trelease_tree_content_recursive(e->tree);\n@@ -1417,7 +1419,7 @@ static int tree_content_set(\n \t\ttree_content_set(e, slash1 + 1, sha1, mode, subtree);\n \t} else {\n \t\te->tree = subtree;\n-\t\te->versions[1].mode = mode;\n+\t\te->versions[1].mode = (mode == S_IFREG) ? (S_IFREG | 0644) : mode;\n \t\thashcpy(e->versions[1].sha1, sha1);\n \t}\n \thashclr(root->versions[1].sha1);\n@@ -1862,20 +1864,25 @@ static void file_change_m(struct branch *b)\n \tunsigned char sha1[20];\n \tuint16_t mode, inline_data = 0;\n \n-\tp = get_mode(p, &mode);\n-\tif (!p)\n-\t\tdie(\"Corrupt mode: %s\", command_buf.buf);\n-\tswitch (mode) {\n-\tcase S_IFREG | 0644:\n-\tcase S_IFREG | 0755:\n-\tcase S_IFLNK:\n-\tcase S_IFGITLINK:\n-\tcase 0644:\n-\tcase 0755:\n-\t\t/* ok */\n-\t\tbreak;\n-\tdefault:\n-\t\tdie(\"Corrupt mode: %s\", command_buf.buf);\n+\tif (!prefixcmp(p, \"- \")) {\n+\t\tmode = 0;\n+\t\tp += 2;\n+\t} else {\n+\t\tp = get_mode(p, &mode);\n+\t\tif (!p)\n+\t\t\tdie(\"Corrupt mode: %s\", command_buf.buf);\n+\t\tswitch (mode) {\n+\t\tcase S_IFREG | 0644:\n+\t\tcase S_IFREG | 0755:\n+\t\tcase S_IFLNK:\n+\t\tcase S_IFGITLINK:\n+\t\tcase 0644:\n+\t\tcase 0755:\n+\t\t\t/* ok */\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tdie(\"Corrupt mode: %s\", command_buf.buf);\n+\t\t}\n \t}\n \n \tif (*p == ':') {\n-- \n1.6.0.6.3.g7ccbd\n"},{"id":"98470","messageId":"1229825502-963-2-git-send-email-felipe.contreras@gmail.com","threadId":"16816","inReplyTo":"1229825502-963-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 2/2] fast-import: add special '-' blob reference to use the previous one.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2008-12-21T02:11:42Z","receivedAt":"2008-12-21T02:11:42Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Signed-off-by: Felipe Contreras \n---\n Documentation/git-fast-import.txt | 6 +++---\n fast-import.c | 13 ++++++++++---\n 2 files changed, 13 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\nindex 9d4231e..6fce41f 100644\n--- a/Documentation/git-fast-import.txt\n+++ b/Documentation/git-fast-import.txt\n@@ -457,9 +457,9 @@ External data format::\n \t'M' SP SP SP LF\n ....\n +\n-Here `` can be either a mark reference (`:`)\n-set by a prior `blob` command, or a full 40-byte SHA-1 of an\n-existing Git blob object.\n+Here `` can be either a mark reference (`:`) set by a prior\n+`blob` command, a special `-` reference to use the one of the ancestor, or\n+a full 40-byte SHA-1 of an existing Git blob object.\n \n Inline data format::\n \tThe data content for the file has not been supplied yet.\ndiff --git a/fast-import.c b/fast-import.c\nindex a00d3fe..228c474 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1385,7 +1385,8 @@ static int tree_content_set(\n \t\t\t\tif (mode == S_IFREG)\n \t\t\t\t\tmode = e->versions[0].mode;\n \t\t\t\te->versions[1].mode = (mode == S_IFREG) ? (S_IFREG | 0644) : mode;\n-\t\t\t\thashcpy(e->versions[1].sha1, sha1);\n+\t\t\t\thashcpy(e->versions[1].sha1,\n+\t\t\t\t\tis_null_sha1(sha1) ? e->versions[0].sha1 : sha1);\n \t\t\t\tif (e->tree)\n \t\t\t\t\trelease_tree_content_recursive(e->tree);\n \t\t\t\te->tree = subtree;\n@@ -1420,7 +1421,8 @@ static int tree_content_set(\n \t} else {\n \t\te->tree = subtree;\n \t\te->versions[1].mode = (mode == S_IFREG) ? (S_IFREG | 0644) : mode;\n-\t\thashcpy(e->versions[1].sha1, sha1);\n+\t\thashcpy(e->versions[1].sha1,\n+\t\t\tis_null_sha1(sha1) ? e->versions[0].sha1 : sha1);\n \t}\n \thashclr(root->versions[1].sha1);\n \treturn 1;\n@@ -1862,7 +1864,7 @@ static void file_change_m(struct branch *b)\n \tconst char *endp;\n \tstruct object_entry *oe = oe;\n \tunsigned char sha1[20];\n-\tuint16_t mode, inline_data = 0;\n+\tuint16_t mode, inline_data = 0, empty_blob = 0;\n \n \tif (!prefixcmp(p, \"- \")) {\n \t\tmode = 0;\n@@ -1893,6 +1895,10 @@ static void file_change_m(struct branch *b)\n \t} else if (!prefixcmp(p, \"inline\")) {\n \t\tinline_data = 1;\n \t\tp += 6;\n+\t} else if (!prefixcmp(p, \"- \")) {\n+\t\thashclr(sha1);\n+\t\tempty_blob = 1;\n+\t\tp += 1;\n \t} else {\n \t\tif (get_sha1_hex(p, sha1))\n \t\t\tdie(\"Invalid SHA1: %s\", command_buf.buf);\n@@ -1936,6 +1942,7 @@ static void file_change_m(struct branch *b)\n \t\tif (oe->type != OBJ_BLOB)\n \t\t\tdie(\"Not a blob (actually a %s): %s\",\n \t\t\t\ttypename(oe->type), command_buf.buf);\n+\t} else if (empty_blob) {\n \t} else {\n \t\tenum object_type type = sha1_object_info(sha1, NULL);\n \t\tif (type < 0)\n-- \n1.6.0.6.3.g7ccbd\n"},{"id":"98506","messageId":"20081221220757.GA17355@spearce.org","threadId":"16816","inReplyTo":"1229825502-963-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 1/2] fast-import: add special mode; copy from parent.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-12-21T22:07:57Z","receivedAt":"2008-12-21T22:07:57Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Felipe Contreras wrote:\n> +\tif (!prefixcmp(p, \"- \")) {\n> +\t\tmode = 0;\n> +\t\tp += 2;\n\nThis part made me wonder, why are we always doing \"S_IFREG | mode\"\nfurther down?\n\n> +\t} else {\n> +\t\tp = get_mode(p, &mode);\n> +\t\tif (!p)\n> +\t\t\tdie(\"Corrupt mode: %s\", command_buf.buf);\n> +\t\tswitch (mode) {\n> +\t\tcase S_IFREG | 0644:\n> +\t\tcase S_IFREG | 0755:\n> +\t\tcase S_IFLNK:\n> +\t\tcase S_IFGITLINK:\n> +\t\tcase 0644:\n> +\t\tcase 0755:\n> +\t\t\t/* ok */\n> +\t\t\tbreak;\n> +\t\tdefault:\n> +\t\t\tdie(\"Corrupt mode: %s\", command_buf.buf);\n> +\t\t}\n\n-- \nShawn.\n"},{"id":"98507","messageId":"20081221221149.GB17355@spearce.org","threadId":"16816","inReplyTo":"1229825502-963-2-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 2/2] fast-import: add special '-' blob reference to use the previous one.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-12-21T22:11:49Z","receivedAt":"2008-12-21T22:11:49Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Felipe Contreras wrote:\n> @@ -1862,7 +1864,7 @@ static void file_change_m(struct branch *b)\n> \tconst char *endp;\n> \tstruct object_entry *oe = oe;\n> \tunsigned char sha1[20];\n> -\tuint16_t mode, inline_data = 0;\n> +\tuint16_t mode, inline_data = 0, empty_blob = 0;\n\nIts not the empty blob, its the inherited/assumed blob...\n \n> @@ -1893,6 +1895,10 @@ static void file_change_m(struct branch *b)\n> \t} else if (!prefixcmp(p, \"inline\")) {\n> \t\tinline_data = 1;\n> \t\tp += 6;\n> +\t} else if (!prefixcmp(p, \"- \")) {\n> +\t\thashclr(sha1);\n> +\t\tempty_blob = 1;\n> +\t\tp += 1;\n\nHmph, so if create a new path with a blob of \"-\" the repository\nwill be corrupt because the zero id was used and error was produced.\n\nActually I think you have the same bug in the prior patch with the\nmode being inherited. I wonder if we shouldn't put error checking\nin too to validate that versions[0] describes a file entry.\n\n-- \nShawn.\n"},{"id":"98513","messageId":"7vlju9kvyg.fsf@gitster.siamese.dyndns.org","threadId":"16816","inReplyTo":"20081221221149.GB17355@spearce.org","subject":"Re: [PATCH 2/2] fast-import: add special '-' blob reference to use the previous one.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-21T22:24:55Z","receivedAt":"2008-12-21T22:24:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> Hmph, so if create a new path with a blob of \"-\" the repository\n> will be corrupt because the zero id was used and error was produced.\n>\n> Actually I think you have the same bug in the prior patch with the\n> mode being inherited. I wonder if we shouldn't put error checking\n> in too to validate that versions[0] describes a file entry.\n\nWhy are these patches necessary?\n\nThe proposed commit message describes what it does, but does not give hint\nto even guess being able to use this new feature helps in what situation.\nAs far as I can see, these changes allow the exporter to say \"this aspect\nof the new data is the same as the previous one\", but I thought that the\nway in which fast-import works already revolves around \"you have this\ntree, and the next tree is different from it in this and that way.\" Why\ndoes one need be able to mention \"this is the same as the previous one\"\nexplicitly in the first place?\n"},{"id":"98515","messageId":"20081221223335.GF17355@spearce.org","threadId":"16816","inReplyTo":"7vlju9kvyg.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] fast-import: add special '-' blob reference to use the previous one.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-12-21T22:33:35Z","receivedAt":"2008-12-21T22:33:35Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> \"Shawn O. Pearce\" writes:\n> \n> > Hmph, so if create a new path with a blob of \"-\" the repository\n> > will be corrupt because the zero id was used and error was produced.\n> >\n> > Actually I think you have the same bug in the prior patch with the\n> > mode being inherited. I wonder if we shouldn't put error checking\n> > in too to validate that versions[0] describes a file entry.\n> \n> Why are these patches necessary?\n> \n> The proposed commit message describes what it does, but does not give hint\n> to even guess being able to use this new feature helps in what situation.\n> As far as I can see, these changes allow the exporter to say \"this aspect\n> of the new data is the same as the previous one\", but I thought that the\n> way in which fast-import works already revolves around \"you have this\n> tree, and the next tree is different from it in this and that way.\" Why\n> does one need be able to mention \"this is the same as the previous one\"\n> explicitly in the first place?\n\nHmm. Actually, imagine you were dumping from git-diff output style\nstream into a fast-import stream.\n\nIf a file changes only content, the mode is shown in the index line.\nYay us. But what if the index line wasn't present in the diff? You\ndon't know the prior mode of the file, but you do have its content.\n\nIf a file changes only mode, we get no content hints in the diff.\nHow do you send that into fast-import without making the frontend\nkeep track of every path's current mode?\n\nThough I agree, these details should be described in the commit\nmessages, not left as an exercise for the maintainer to make up.\n\n-- \nShawn.\n"},{"id":"98525","messageId":"94a0d4530812211808r147cbd40na7266abe5ac0e6f3@mail.gmail.com","threadId":"16816","inReplyTo":"20081221221149.GB17355@spearce.org","subject":"Re: [PATCH 2/2] fast-import: add special '-' blob reference to use the previous one.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2008-12-22T02:08:28Z","receivedAt":"2008-12-22T02:08:28Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Dec 22, 2008 at 12:11 AM, Shawn O. Pearce wrote:\n> Felipe Contreras wrote:\n>> @@ -1862,7 +1864,7 @@ static void file_change_m(struct branch *b)\n>> const char *endp;\n>> struct object_entry *oe = oe;\n>> unsigned char sha1[20];\n>> - uint16_t mode, inline_data = 0;\n>> + uint16_t mode, inline_data = 0, empty_blob = 0;\n>\n> Its not the empty blob, its the inherited/assumed blob...\n\nRight. I thought: in order to use the inherited blob, you should not\nspecify any blob (leave it empty, or blank).\n\nBut yeah, 'inherited' does the job too.\n\n>> @@ -1893,6 +1895,10 @@ static void file_change_m(struct branch *b)\n>> } else if (!prefixcmp(p, \"inline\")) {\n>> inline_data = 1;\n>> p += 6;\n>> + } else if (!prefixcmp(p, \"- \")) {\n>> + hashclr(sha1);\n>> + empty_blob = 1;\n>> + p += 1;\n>\n> Hmph, so if create a new path with a blob of \"-\" the repository\n> will be corrupt because the zero id was used and error was produced.\n>\n> Actually I think you have the same bug in the prior patch with the\n> mode being inherited. I wonder if we shouldn't put error checking\n> in too to validate that versions[0] describes a file entry.\n\nYes, in my tests I found that issue in the previous patch and I have a\nfix for that (set a default mode), but I haven't fixed this one. Do\nyou know what should be the behavior? I think it should 'die'.\n\n-- \nFelipe Contreras\n"},{"id":"98526","messageId":"94a0d4530812211810x7d9772cbq9934dd6ea6e7f62a@mail.gmail.com","threadId":"16816","inReplyTo":"20081221223335.GF17355@spearce.org","subject":"Re: [PATCH 2/2] fast-import: add special '-' blob reference to use the previous one.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2008-12-22T02:10:34Z","receivedAt":"2008-12-22T02:10:34Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Dec 22, 2008 at 12:33 AM, Shawn O. Pearce wrote:\n> Junio C Hamano wrote:\n>> \"Shawn O. Pearce\" writes:\n>>\n>> > Hmph, so if create a new path with a blob of \"-\" the repository\n>> > will be corrupt because the zero id was used and error was produced.\n>> >\n>> > Actually I think you have the same bug in the prior patch with the\n>> > mode being inherited. I wonder if we shouldn't put error checking\n>> > in too to validate that versions[0] describes a file entry.\n>>\n>> Why are these patches necessary?\n\nYeah, I realized I didn't explain that after sending the patches.\n\n>> The proposed commit message describes what it does, but does not give hint\n>> to even guess being able to use this new feature helps in what situation.\n>> As far as I can see, these changes allow the exporter to say \"this aspect\n>> of the new data is the same as the previous one\", but I thought that the\n>> way in which fast-import works already revolves around \"you have this\n>> tree, and the next tree is different from it in this and that way.\" Why\n>> does one need be able to mention \"this is the same as the previous one\"\n>> explicitly in the first place?\n>\n> Hmm. Actually, imagine you were dumping from git-diff output style\n> stream into a fast-import stream.\n>\n> If a file changes only content, the mode is shown in the index line.\n> Yay us. But what if the index line wasn't present in the diff? You\n> don't know the prior mode of the file, but you do have its content.\n>\n> If a file changes only mode, we get no content hints in the diff.\n> How do you send that into fast-import without making the frontend\n> keep track of every path's current mode?\n>\n> Though I agree, these details should be described in the commit\n> messages, not left as an exercise for the maintainer to make up.\n\nExactly. That's what happens with monotone; you usually have the\ncontents or the new mode, but not both at the same time.\n\n-- \nFelipe Contreras\n"},{"id":"98528","messageId":"94a0d4530812211823l78d86e1en8e19872ee7efef23@mail.gmail.com","threadId":"16816","inReplyTo":"20081221220757.GA17355@spearce.org","subject":"Re: [PATCH 1/2] fast-import: add special mode; copy from parent.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2008-12-22T02:23:46Z","receivedAt":"2008-12-22T02:23:46Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Dec 22, 2008 at 12:07 AM, Shawn O. Pearce wrote:\n> Felipe Contreras wrote:\n>> + if (!prefixcmp(p, \"- \")) {\n>> + mode = 0;\n>> + p += 2;\n>\n> This part made me wonder, why are we always doing \"S_IFREG | mode\"\n> further down?\n\nMy guess is because 0644 and 0755; doing S_IFREG | mode doesn't\nachieve anything for the other modes.\n\nI just sent a patch that I hope makes that more visible.\n\n-- \nFelipe Contreras\n"},{"id":"98529","messageId":"94a0d4530812211825v452d0cccp710054ee082e1690@mail.gmail.com","threadId":"16816","inReplyTo":"1229825502-963-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 1/2] fast-import: add special mode; copy from parent.","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2008-12-22T02:25:11Z","receivedAt":"2008-12-22T02:25:11Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Dec 21, 2008 at 4:11 AM, Felipe Contreras\n wrote:\n> Signed-off-by: Felipe Contreras \n> ---\n> Documentation/git-fast-import.txt | 1 +\n> fast-import.c | 41 +++++++++++++++++++++---------------\n> 2 files changed, 25 insertions(+), 17 deletions(-)\n\nPlease disregard this patch, it doesn't work as I expected. I'll send\nan updated one soon.\n\n-- \nFelipe Contreras\n"}]}