{"thread":{"id":"14528","subject":"[RFC PATCH] Support gitlinks in fast-import/export.","startedAt":"2008-07-18T17:03:36Z","lastAt":"2008-07-19T01:22:16Z","messageCount":6,"participants":["Alexander Gavrilov","Johannes Schindelin","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"83893","messageId":"200807182103.37272.angavrilov@gmail.com","threadId":"14528","inReplyTo":null,"subject":"[RFC PATCH] Support gitlinks in fast-import/export.","fromName":"Alexander Gavrilov","fromEmail":"angavrilov@gmail.com","sentAt":"2008-07-18T17:03:36Z","receivedAt":"2008-07-18T17:03:36Z","isPatch":true,"sender":{"key":"angavrilov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42666?v=4"},"body":"Currently fast-import/export cannot be used for\nrepositories with submodules. This patch extends\nthe relevant programs to make them correctly\nprocess gitlinks.\n\nSigned-off-by: Alexander Gavrilov <angavrilov@gmail.com>\n---\n\n\tI noticed that fast-export & fast-import cannot work with\n\trepositories using submodules: import complains about\n\tan invalid mode, and export fails while trying to open the SHA\n\tas a blob.\n\n\tAs I didn't see any particular reason for it to be so, I tried to\n\timplement support for gitlinks.\n\n\tWhat I'm unsure of is, should fast-export try to reuse commit\n\tmarks for gitlinks where it happened to recognize the object,\n\tor always output the SHA as it is stored in the tree?\n\n\t-- Alexander\n\n Documentation/git-fast-import.txt |    3 +++\n builtin-fast-export.c             |   18 +++++++++++++++---\n fast-import.c                     |   12 +++++++++++-\n 3 files changed, 29 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\nindex 395c055..80c591a 100644\n--- a/Documentation/git-fast-import.txt\n+++ b/Documentation/git-fast-import.txt\n@@ -481,6 +481,9 @@ in octal.  Git only supports the following modes:\n   what you want.\n * `100755` or `755`: A normal, but executable, file.\n * `120000`: A symlink, the content of the file will be the link target.\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 implements submodules.\n \n In both formats `<path>` is the complete path of the file to be added\n (if not already existing) or modified (if already existing).\ndiff --git a/builtin-fast-export.c b/builtin-fast-export.c\nindex d0a462f..14b1549 100644\n--- a/builtin-fast-export.c\n+++ b/builtin-fast-export.c\n@@ -123,8 +123,19 @@ static void show_filemodify(struct diff_queue_struct *q,\n \t\t\tprintf(\"D %s\\n\", spec->path);\n \t\telse {\n \t\t\tstruct object *object = lookup_object(spec->sha1);\n-\t\t\tprintf(\"M %06o :%d %s\\n\", spec->mode,\n-\t\t\t       get_object_mark(object), spec->path);\n+\t\t\tint mark = object ? get_object_mark(object) : 0;\n+\n+\t\t\tif (mark)\n+\t\t\t\tprintf(\"M %06o :%d %s\\n\", spec->mode,\n+\t\t\t\t       mark, spec->path);\n+\t\t\telse {\n+\t\t\t\tif (!S_ISGITLINK(spec->mode))\n+\t\t\t\t\tdie(\"Unknown object added: %s at %s\",\n+\t\t\t\t\t    sha1_to_hex(spec->sha1), spec->path);\n+\n+\t\t\t\tprintf(\"M %06o %s %s\\n\", spec->mode,\n+\t\t\t\t       sha1_to_hex(spec->sha1), spec->path);\n+\t\t\t}\n \t\t}\n \t}\n }\n@@ -183,7 +194,8 @@ static void handle_commit(struct commit *commit, struct rev_info *rev)\n \t\t\t\t    \"\", &rev->diffopt);\n \n \tfor (i = 0; i < diff_queued_diff.nr; i++)\n-\t\thandle_object(diff_queued_diff.queue[i]->two->sha1);\n+\t\tif (!S_ISGITLINK(diff_queued_diff.queue[i]->two->mode))\n+\t\t\thandle_object(diff_queued_diff.queue[i]->two->sha1);\n \n \tmark_object(&commit->object);\n \tif (!is_encoding_utf8(encoding))\ndiff --git a/fast-import.c b/fast-import.c\nindex e72b286..e7977c1 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1868,6 +1868,7 @@ static void file_change_m(struct branch *b)\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@@ -1900,7 +1901,16 @@ static void file_change_m(struct branch *b)\n \t\tp = uq.buf;\n \t}\n \n-\tif (inline_data) {\n+\tif (S_ISGITLINK(mode)) {\n+\t\tif (inline_data)\n+\t\t\tdie(\"Git links cannot be specified 'inline': %s\",\n+\t\t\t\tcommand_buf.buf);\n+\t\telse if (oe) {\n+\t\t\tif (oe->type != OBJ_COMMIT)\n+\t\t\t\tdie(\"Not a commit (actually a %s): %s\",\n+\t\t\t\t\ttypename(oe->type), command_buf.buf);\n+\t\t}\n+\t} else if (inline_data) {\n \t\tstatic struct strbuf buf = STRBUF_INIT;\n \n \t\tif (p != uq.buf) {\n-- \n1.5.6.3.18.gfe82\n"},{"id":"83897","messageId":"alpine.DEB.1.00.0807181810400.8986@racer","threadId":"14528","inReplyTo":"200807182103.37272.angavrilov@gmail.com","subject":"Re: [RFC PATCH] Support gitlinks in fast-import/export.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-18T17:36:26Z","receivedAt":"2008-07-18T17:36:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 18 Jul 2008, Alexander Gavrilov wrote:\n\n> \tWhat I'm unsure of is, should fast-export try to reuse commit\n> \tmarks for gitlinks where it happened to recognize the object,\n> \tor always output the SHA as it is stored in the tree?\n\nAre they commit marks?  No.  So they should be handled as marks, just as \nthose for blobs and trees.\n\n(They are commit marks in the _submodule_, but that does not matter here.)\n\n> diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\n> index 395c055..80c591a 100644\n> --- a/Documentation/git-fast-import.txt\n> +++ b/Documentation/git-fast-import.txt\n> @@ -481,6 +481,9 @@ in octal.  Git only supports the following modes:\n>    what you want.\n>  * `100755` or `755`: A normal, but executable, file.\n>  * `120000`: A symlink, the content of the file will be the link target.\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 implements submodules.\n\ns/\\(implement\\)s/\\1/\n\n> diff --git a/builtin-fast-export.c b/builtin-fast-export.c\n> index d0a462f..14b1549 100644\n> --- a/builtin-fast-export.c\n> +++ b/builtin-fast-export.c\n> @@ -123,8 +123,19 @@ static void show_filemodify(struct diff_queue_struct *q,\n>  \t\t\tprintf(\"D %s\\n\", spec->path);\n>  \t\telse {\n>  \t\t\tstruct object *object = lookup_object(spec->sha1);\n> -\t\t\tprintf(\"M %06o :%d %s\\n\", spec->mode,\n> -\t\t\t       get_object_mark(object), spec->path);\n> +\t\t\tint mark = object ? get_object_mark(object) : 0;\n\nAs I said, that looks wrong.  Maybe we have to fake objects for the \ngitlinks.\n\n> @@ -183,7 +194,8 @@ static void handle_commit(struct commit *commit, struct rev_info *rev)\n>  \t\t\t\t    \"\", &rev->diffopt);\n>  \n>  \tfor (i = 0; i < diff_queued_diff.nr; i++)\n> -\t\thandle_object(diff_queued_diff.queue[i]->two->sha1);\n> +\t\tif (!S_ISGITLINK(diff_queued_diff.queue[i]->two->mode))\n> +\t\t\thandle_object(diff_queued_diff.queue[i]->two->sha1);\n\nWhy?  You do not want to export changes in the submodules?\n\n> diff --git a/fast-import.c b/fast-import.c\n> index e72b286..e7977c1 100644\n> --- a/fast-import.c\n> +++ b/fast-import.c\n\nI'll let Shawn comment on that.  Oh, wait, it's his last day in work \ntoday.  Maybe I have something useful to say about this part of the patch, \nthen, to save Shawn some work.\n\n> @@ -1900,7 +1901,16 @@ static void file_change_m(struct branch *b)\n>  \t\tp = uq.buf;\n>  \t}\n>  \n> -\tif (inline_data) {\n> +\tif (S_ISGITLINK(mode)) {\n> +\t\tif (inline_data)\n> +\t\t\tdie(\"Git links cannot be specified 'inline': %s\",\n> +\t\t\t\tcommand_buf.buf);\n> +\t\telse if (oe) {\n> +\t\t\tif (oe->type != OBJ_COMMIT)\n> +\t\t\t\tdie(\"Not a commit (actually a %s): %s\",\n> +\t\t\t\t\ttypename(oe->type), command_buf.buf);\n\nHow is that supposed to work?  Do I understand correctly that you require \nthe user to construct a commit object for the gitlink?  That would be \nactively wrong.\n\nOh, and your patch lacks test cases that demonstrate how you envisage the \nwhole thing to work.\n\nCiao,\nDscho\n"},{"id":"83907","messageId":"200807182234.47363.angavrilov@gmail.com","threadId":"14528","inReplyTo":"alpine.DEB.1.00.0807181810400.8986@racer","subject":"Re: [RFC PATCH] Support gitlinks in fast-import/export.","fromName":"Alexander Gavrilov","fromEmail":"angavrilov@gmail.com","sentAt":"2008-07-18T18:34:46Z","receivedAt":"2008-07-18T18:34:46Z","isPatch":true,"sender":{"key":"angavrilov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42666?v=4"},"body":"Hello,\n\nOn Friday 18 July 2008 21:36:26 Johannes Schindelin wrote:\n> > \tWhat I'm unsure of is, should fast-export try to reuse commit\n> > \tmarks for gitlinks where it happened to recognize the object,\n> > \tor always output the SHA as it is stored in the tree?\n \n> Are they commit marks?  No.  So they should be handled as marks, just as \n> those for blobs and trees.\n> \n> (They are commit marks in the _submodule_, but that does not matter here.)\n\nWell, I was thinking of that unlikely case when the master module and the submodule\nare in the same repository and are exported together. But probably it is best to just\noutput the SHA after all.\n\n> >  \tfor (i = 0; i < diff_queued_diff.nr; i++)\n> > -\t\thandle_object(diff_queued_diff.queue[i]->two->sha1);\n> > +\t\tif (!S_ISGITLINK(diff_queued_diff.queue[i]->two->mode))\n> > +\t\t\thandle_object(diff_queued_diff.queue[i]->two->sha1);\n> \n> Why?  You do not want to export changes in the submodules?\n\nhandle_object opens the sha as a blob and outputs it. As gitlinks aren't blobs, it won't work.\nAnd if the submodule is in a separate repository, there is nothing to open anyway.\n\nSimultaneously reading commits from the submodule repository is a whole different level\nof complexity. I'm afraid I'm not up to it yet.\n\n> > diff --git a/builtin-fast-export.c b/builtin-fast-export.c\n> > index d0a462f..14b1549 100644\n> > --- a/builtin-fast-export.c\n> > +++ b/builtin-fast-export.c\n> > @@ -123,8 +123,19 @@ static void show_filemodify(struct diff_queue_struct *q,\n> >  \t\t\tprintf(\"D %s\\n\", spec->path);\n> >  \t\telse {\n> >  \t\t\tstruct object *object = lookup_object(spec->sha1);\n> > -\t\t\tprintf(\"M %06o :%d %s\\n\", spec->mode,\n> > -\t\t\t       get_object_mark(object), spec->path);\n> > +\t\t\tint mark = object ? get_object_mark(object) : 0;\n> \n> As I said, that looks wrong.  Maybe we have to fake objects for the \n> gitlinks.\n\nI tried to avoid faking blobs and stick to the interface of the M command itself.\n\n> > @@ -1900,7 +1901,16 @@ static void file_change_m(struct branch *b)\n> >  \t\tp = uq.buf;\n> >  \t}\n> >  \n> > -\tif (inline_data) {\n> > +\tif (S_ISGITLINK(mode)) {\n> > +\t\tif (inline_data)\n> > +\t\t\tdie(\"Git links cannot be specified 'inline': %s\",\n> > +\t\t\t\tcommand_buf.buf);\n> > +\t\telse if (oe) {\n> > +\t\t\tif (oe->type != OBJ_COMMIT)\n> > +\t\t\t\tdie(\"Not a commit (actually a %s): %s\",\n> > +\t\t\t\t\ttypename(oe->type), command_buf.buf);\n> \n> How is that supposed to work?  Do I understand correctly that you require \n> the user to construct a commit object for the gitlink?  That would be \n> actively wrong.\n\nThere are three forms of the M command:\n\nM mode inline some/path\n...some data...\n\nM mode :mark some/path\n\nM mode SHA some/path\n\nFor usual files the mark must be created by the 'blob' command,\nand the SHA must refer to an existing blob. This hunk makes fast-import\nrequire for gitlinks a commit mark instead, and accept the SHA without\nchecking (as it is expected to be in another repository).\n\n> Oh, and your patch lacks test cases that demonstrate how you envisage the \n> whole thing to work.\n\nOk, I'll make some tests tomorrow. For now, this is an example of output from\nmy test repository:\n\n...\nblob\nmark :16\ndata 41\n[submodule \"sub\"]\n        path = sub\n        url = sub\n\ncommit refs/heads/master\nmark :17\nauthor Alexander Gavrilov <angavrilov@gmail.com> 1216360728 +0400\ncommitter Alexander Gavrilov <angavrilov@gmail.com> 1216360728 +0400\ndata 4\nsub\nfrom :15\nM 100644 :16 .gitmodules\nM 160000 d6317bc6e2597bf74ae199514a54e25775b0d20d sub\n\n\nAlexander\n"},{"id":"83914","messageId":"20080718204338.GB27528@spearce.org","threadId":"14528","inReplyTo":"200807182103.37272.angavrilov@gmail.com","subject":"Re: [RFC PATCH] Support gitlinks in fast-import/export.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-07-18T20:43:38Z","receivedAt":"2008-07-18T20:43:38Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Alexander Gavrilov <angavrilov@gmail.com> wrote:\n> Currently fast-import/export cannot be used for\n> repositories with submodules. This patch extends\n> the relevant programs to make them correctly\n> process gitlinks.\n> \n> Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>\n\nAcked-by: Shawn O. Pearce <spearce@spearce.org>\n\n> \tI noticed that fast-export & fast-import cannot work with\n> \trepositories using submodules: import complains about\n> \tan invalid mode, and export fails while trying to open the SHA\n> \tas a blob.\n> \n> \tAs I didn't see any particular reason for it to be so, I tried to\n> \timplement support for gitlinks.\n> \n> \tWhat I'm unsure of is, should fast-export try to reuse commit\n> \tmarks for gitlinks where it happened to recognize the object,\n> \tor always output the SHA as it is stored in the tree?\n\nIts unlikely that the commit objects are in the repository the\nexport is running in, so its unlikely you can use a mark.  Its\nfine to just always output the SHA-1 I think.\n\n-- \nShawn.\n"},{"id":"83923","messageId":"alpine.DEB.1.00.0807190109570.3064@eeepc-johanness","threadId":"14528","inReplyTo":"200807182234.47363.angavrilov@gmail.com","subject":"Re: [RFC PATCH] Support gitlinks in fast-import/export.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-18T23:21:53Z","receivedAt":"2008-07-18T23:21:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 18 Jul 2008, Alexander Gavrilov wrote:\n\n> On Friday 18 July 2008 21:36:26 Johannes Schindelin wrote:\n> > > \tWhat I'm unsure of is, should fast-export try to reuse commit \n> > > \tmarks for gitlinks where it happened to recognize the object, or \n> > > \talways output the SHA as it is stored in the tree?\n>  \n> > Are they commit marks?  No.  So they should be handled as marks, just \n> > as those for blobs and trees.\n> > \n> > (They are commit marks in the _submodule_, but that does not matter \n> > here.)\n> \n> Well, I was thinking of that unlikely case when the master module and \n> the submodule are in the same repository and are exported together. But \n> probably it is best to just output the SHA after all.\n\nExactly.\n\nWhat you describe is a _possibility_.  Requiring it would be a serious \nmistake.\n\n> > >  \tfor (i = 0; i < diff_queued_diff.nr; i++)\n> > > -\t\thandle_object(diff_queued_diff.queue[i]->two->sha1);\n> > > +\t\tif (!S_ISGITLINK(diff_queued_diff.queue[i]->two->mode))\n> > > +\t\t\thandle_object(diff_queued_diff.queue[i]->two->sha1);\n> > \n> > Why?  You do not want to export changes in the submodules?\n> \n> handle_object opens the sha as a blob and outputs it. As gitlinks aren't \n> blobs, it won't work. And if the submodule is in a separate repository, \n> there is nothing to open anyway.\n\nSo what should happen to them instead?  Ignoring them?\n\nPossible.\n\nMy earlier remark about the marks was this: you might want to mark SHA-1s \nof gitlinks with a (shorter) number, but maybe that is just bullocks.  At \nthe same time, it might be not.\n\n> Simultaneously reading commits from the submodule repository is a whole \n> different level of complexity. I'm afraid I'm not up to it yet.\n\nGood news for you:  I think it would be a serious mistake to read commits \nfrom the submodule.\n\n> > >  \t\telse {\n> > >  \t\t\tstruct object *object = lookup_object(spec->sha1);\n> > > -\t\t\tprintf(\"M %06o :%d %s\\n\", spec->mode,\n> > > -\t\t\t       get_object_mark(object), spec->path);\n> > > +\t\t\tint mark = object ? get_object_mark(object) : 0;\n> > \n> > As I said, that looks wrong.  Maybe we have to fake objects for the \n> > gitlinks.\n> \n> I tried to avoid faking blobs and stick to the interface of the M \n> command itself.\n\nMy point was: I do not see gitlinks handled _at all_.\n\nI agree that they should not be handled the same as blobs, but i did not \nhave time to check that gitlinks are not just ignored by your patch.  \nWhich would be wrong: you want the exported commits to contain them.\n\n> > > @@ -1900,7 +1901,16 @@ static void file_change_m(struct branch *b)\n> > >  \t\tp = uq.buf;\n> > >  \t}\n> > >  \n> > > -\tif (inline_data) {\n> > > +\tif (S_ISGITLINK(mode)) {\n> > > +\t\tif (inline_data)\n> > > +\t\t\tdie(\"Git links cannot be specified 'inline': %s\",\n> > > +\t\t\t\tcommand_buf.buf);\n> > > +\t\telse if (oe) {\n> > > +\t\t\tif (oe->type != OBJ_COMMIT)\n> > > +\t\t\t\tdie(\"Not a commit (actually a %s): %s\",\n> > > +\t\t\t\t\ttypename(oe->type), command_buf.buf);\n> > \n> > How is that supposed to work?  Do I understand correctly that you \n> > require the user to construct a commit object for the gitlink?  That \n> > would be actively wrong.\n> \n> There are three forms of the M command:\n> \n> M mode inline some/path\n> ...some data...\n> \n> M mode :mark some/path\n> \n> M mode SHA some/path\n> \n> For usual files the mark must be created by the 'blob' command,\n> and the SHA must refer to an existing blob. This hunk makes fast-import\n> require for gitlinks a commit mark instead, and accept the SHA without\n> checking (as it is expected to be in another repository).\n\nWhat's this commit mark thing?  I hope you do not mean to ask the user to \nconstruct a commit object in the _superproject's_ repository...\n\n> > Oh, and your patch lacks test cases that demonstrate how you envisage \n> > the whole thing to work.\n> \n> Ok, I'll make some tests tomorrow.\n\nMaybe I should enhance on my point, to drive it home:\n\nIf you do _not_ include automated tests, other people have less reason to \ntrust that your patch does what you insist it does.\n\nIf you do not include tests, and have a sparse commit message (which you \ndo), people are left to guess what your patch tries to do, and do not even \nhave the chance to see something you wanted to work.\n\nIf you do not include tests, and your patch does something a reviewer \nfeels it should not to, or omits something a reviewer feels it _should_ \ndo, there is less of an opportunity to see if this was intended: a \ncomprehensive test script would show what the submitter expects to \nwork alright.\n\nSo in a very real sense, it is advisable to write the test _first_, and \nthen the patch.\n\nNot everything XP brought to this world is evil.  (Oh yes, you guessed it, \nI was talking about eXtreme Programming...)\n\nCiao,\nDscho\n"},{"id":"83932","messageId":"alpine.DEB.1.00.0807190321040.3064@eeepc-johanness","threadId":"14528","inReplyTo":"20080718204338.GB27528@spearce.org","subject":"Re: [RFC PATCH] Support gitlinks in fast-import/export.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-19T01:22:16Z","receivedAt":"2008-07-19T01:22:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 18 Jul 2008, Shawn O. Pearce wrote:\n\n> Alexander Gavrilov <angavrilov@gmail.com> wrote:\n> \n> > \tI noticed that fast-export & fast-import cannot work with \n> > \trepositories using submodules: import complains about an invalid \n> > \tmode, and export fails while trying to open the SHA as a blob.\n> > \n> > \tAs I didn't see any particular reason for it to be so, I tried to \n> > \timplement support for gitlinks.\n> > \n> > \tWhat I'm unsure of is, should fast-export try to reuse commit \n> > \tmarks for gitlinks where it happened to recognize the object, or \n> > \talways output the SHA as it is stored in the tree?\n> \n> Its unlikely that the commit objects are in the repository the export is \n> running in, so its unlikely you can use a mark.  Its fine to just always \n> output the SHA-1 I think.\n\nOh, I just realized that fast-import expects changes, instead of full \nsnapshots.\n\nSo strike all my suggestions regarding gitlink marks.\n\nSorry,\nDscho\n"}]}