{"thread":{"id":"8245","subject":"[PATCH v2] Submodule merge support","startedAt":"2007-05-20T15:42:27Z","lastAt":"2007-05-27T14:58:05Z","messageCount":10,"participants":["Martin Waitz","Shawn O. Pearce","Junio C Hamano","Johannes Schindelin","Alex Riesen","Johan Herland","Morten Welinder"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"42718","messageId":"20070520154227.GG5412@admingilde.org","threadId":"8245","inReplyTo":null,"subject":"[PATCH v2] Submodule merge support","fromName":"Martin Waitz","fromEmail":"tali@admingilde.org","sentAt":"2007-05-20T15:42:27Z","receivedAt":"2007-05-20T15:42:27Z","isPatch":true,"sender":{"key":"tali@admingilde.org","avatar":"https://gravatar.com/avatar/3f89b03eee362187effabe257898735b475673a12265c398ea9161259ae91553?d=mp&s=160"},"body":"When merge-recursive gets to a dirlink, it starts an automatic submodule\nmerge and then uses the resulting merge commit for the top-level tree.\nThe submodule merge is done in another process to decouple object databases.\n\nSubmodule merges are done solely in the submodules' history, without taking\nthe supermodule (and it's merge base) into account.  If the submodule merge\nis successful then the new submodule version will be used in the merged\nsupermodule.\n\nIf one side of the merge removed any submodule commits (e.g. by switching to\na different branch) then the automatic merge is stopped so that the user can\ntake a closer look on what happened.\n\nSigned-off-by: Martin Waitz <tali@admingilde.org>\n---\n\nThis patch is based on my previous submodule checkout patch and the\nstart-commands-in-submodule patch.\n\nThis version takes index_only into account and does not need a new\nhelper script as all code is done in C now.\n\nThe entire ll_merge code in merge-recursive still should be moved to\nsome generic place, but that is for another patch.\n\n merge-recursive.c |  122 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 122 insertions(+), 0 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 8f72b2c..72562a8 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -11,6 +11,7 @@\n #include \"diff.h\"\n #include \"diffcore.h\"\n #include \"run-command.h\"\n+#include \"refs.h\"\n #include \"tag.h\"\n #include \"unpack-trees.h\"\n #include \"path-list.h\"\n@@ -574,6 +575,21 @@ static void update_file_flags(const unsigned char *sha,\n \t\tvoid *buf;\n \t\tunsigned long size;\n \n+\t\tif (S_ISDIRLNK(mode)) {\n+\t\t\t/* defer dirlinks to another process, don't try to */\n+\t\t\t/* read the object \"sha\" here */\n+\t\t\tconst char *dirlink_checkout[] = {\n+\t\t\t\t\"dirlink-checkout\", path, sha1_to_hex(sha), NULL\n+\t\t\t};\n+\t\t\tstruct child_process cmd = {\n+\t\t\t\t.argv = dirlink_checkout,\n+\t\t\t\t.git_cmd = 1,\n+\t\t\t};\n+\n+\t\t\trun_command(&cmd);\n+\t\t\tgoto update_index;\n+\t\t}\n+\n \t\tbuf = read_sha1_file(sha, &type, &size);\n \t\tif (!buf)\n \t\t\tdie(\"cannot read object %s '%s'\", sha1_to_hex(sha), path);\n@@ -1025,6 +1041,105 @@ static int ll_merge(mmbuffer_t *result_buf,\n \treturn merge_status;\n }\n \n+\n+static int ll_dirlink_merge_base(const char *path,\n+                            const unsigned char *a,\n+                            const unsigned char *b,\n+                            unsigned char *result)\n+{\n+\tconst char *merge_base[] = {\n+\t\t\"merge-base\",\n+\t\tsha1_to_hex(a),\n+\t\tsha1_to_hex(b),\n+\t\tNULL\n+\t};\n+\tstruct child_process cmd = {\n+\t\t.argv = merge_base,\n+\t\t.submodule = path,\n+\t\t.git_cmd = 1,\n+\t\t.out = -1,\n+\t};\n+\tchar hex[40];\n+\tint status;\n+\n+\tstatus = start_command(&cmd);\n+\tif (status) return status;\n+\n+\tstatus = read(cmd.out, hex, sizeof(hex));\n+\tif (status != 40) return status;\n+\n+\tstatus = finish_command(&cmd);\n+\tif (status) return status;\n+\n+\tstatus = get_sha1_hex(hex, result);\n+\n+\treturn status;\n+}\n+\n+static int ll_dirlink_merge(const char *path,\n+                            const unsigned char *o,\n+                            const unsigned char *a,\n+                            const unsigned char *b,\n+                            unsigned char *result)\n+{\n+\tchar b_hex[40+1];\n+\tconst char *merge[] = {\n+\t\t\"merge\", b_hex, NULL\n+\t};\n+\tstruct child_process cmd = {\n+\t\t.argv = merge,\n+\t\t.submodule = path,\n+\t\t.git_cmd = 1,\n+\t};\n+\tint status;\n+\tunsigned char base[20];\n+\tunsigned char test[20];\n+\n+\tif (index_only)  {\n+\t\t/* as submodules have their own history we don't have to   */\n+\t\t/* try to do the index_only intermediate merges.           */\n+\t\t/* however we still want to get a submodule version        */\n+\t\t/* which is suitable as merge-base, just to make sure that */\n+\t\t/* all merge parents contain this base.                    */\n+\t\t/* The real merge (below) aborts if this check fails       */\n+\t\treturn ll_dirlink_merge_base(path, a, b, result);\n+\t}\n+\n+\tstrcpy(b_hex, sha1_to_hex(b));\n+\toutput(3, \"merging submodule %s:\", path);\n+\toutput(3, \" o=%s\", sha1_to_hex(o));\n+\toutput(3, \" a=%s\", sha1_to_hex(a));\n+\toutput(3, \" b=%s\", sha1_to_hex(b));\n+\n+\t/* first check that the submodule is in the current state  */\n+\t/* so that it can be merged.                               */\n+\tstatus = resolve_gitlink_ref(path, \"HEAD\", test);\n+\tif (hashcmp(test, a)) {\n+\t\treturn error(\"can't merge submodule %s: not up to date.\", path);\n+\t}\n+\n+\t/* check that both sides of the superproject only did a    */\n+\t/* fast forward of the subproject so that it can be merged */\n+\t/* automatically.                                          */\n+\tstatus = ll_dirlink_merge_base(path, a, b, base);\n+\tif (status) return status;\n+\tstatus = ll_dirlink_merge_base(path, o, base, test);\n+\tif (status) return status;\n+\tif (hashcmp(test, o)) {\n+\t\treturn error(\"can't merge submodule %s: conflicting history\",\n+\t\t             path);\n+\t}\n+\n+\t/* now start another merge process for the submodule */\n+\tstatus = run_command(&cmd);\n+\tif (status) return status;\n+\n+\t/* get the new merged version */\n+\tstatus = resolve_gitlink_ref(path, \"HEAD\", result);\n+\n+\treturn status;\n+}\n+\n static struct merge_file_info merge_file(struct diff_filespec *o,\n \t\tstruct diff_filespec *a, struct diff_filespec *b,\n \t\tconst char *branch1, const char *branch2)\n@@ -1069,6 +1184,13 @@ static struct merge_file_info merge_file(struct diff_filespec *o,\n \n \t\t\tfree(result_buf.ptr);\n \t\t\tresult.clean = (merge_status == 0);\n+\t\t} else if (S_ISDIRLNK(a->mode)) {\n+\t\t\tint merge_status;\n+\n+\t\t\tmerge_status = ll_dirlink_merge(a->path,\n+\t\t\t\to->sha1, a->sha1, b->sha1, result.sha);\n+\n+\t\t\tresult.clean = (merge_status == 0);\n \t\t} else {\n \t\t\tif (!(S_ISLNK(a->mode) || S_ISLNK(b->mode)))\n \t\t\t\tdie(\"cannot merge modes?\");\n-- \n1.5.2.2.g081e\n\n\n-- \nMartin Waitz\n"},{"id":"42843","messageId":"20070521062005.GK3141@spearce.org","threadId":"8245","inReplyTo":"20070520154227.GG5412@admingilde.org","subject":"Re: [PATCH v2] Submodule merge support","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-05-21T06:20:05Z","receivedAt":"2007-05-21T06:20:05Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"> @@ -574,6 +575,21 @@ static void update_file_flags(const unsigned char *sha,\n>  \t\tvoid *buf;\n>  \t\tunsigned long size;\n>  \n> +\t\tif (S_ISDIRLNK(mode)) {\n> +\t\t\t/* defer dirlinks to another process, don't try to */\n> +\t\t\t/* read the object \"sha\" here */\n> +\t\t\tconst char *dirlink_checkout[] = {\n> +\t\t\t\t\"dirlink-checkout\", path, sha1_to_hex(sha), NULL\n> +\t\t\t};\n> +\t\t\tstruct child_process cmd = {\n> +\t\t\t\t.argv = dirlink_checkout,\n> +\t\t\t\t.git_cmd = 1,\n> +\t\t\t};\n\nMy Solaris 9 system cannot compile this syntax, even though it is\na clean way to initalize the child_process.  That's why I've always\nused something more like:\n\n\tstruct child_process cmd;\n\tmemset(&cmd, 0, sizeof(cmd));\n\tcmd.argv = dirlink_checkout;\n\tcmd.git_cmd = 1;\n\nand actually that raises another point, does the compiler 0 fill\nthe stack-allocated struct that is initalized like you write, or\ndoes it avoid filling the other fields that aren't mentioned in\nthe initialization?\n\n> +\tstatus = read(cmd.out, hex, sizeof(hex));\n> +\tif (status != 40) return status;\n\nOK, this is probably just never trusting the OS, but shouldn't that\nread be wrapped up in a loop, like our read_in_full?  We want 40\nbytes here, and expect it, and the read call is allowed to return\nas few as 1 byte....\n\n-- \nShawn.\n"},{"id":"42856","messageId":"20070521073253.GU5412@admingilde.org","threadId":"8245","inReplyTo":"20070521062005.GK3141@spearce.org","subject":"Re: [PATCH v2] Submodule merge support","fromName":"Martin Waitz","fromEmail":"tali@admingilde.org","sentAt":"2007-05-21T07:32:53Z","receivedAt":"2007-05-21T07:32:53Z","isPatch":true,"sender":{"key":"tali@admingilde.org","avatar":"https://gravatar.com/avatar/3f89b03eee362187effabe257898735b475673a12265c398ea9161259ae91553?d=mp&s=160"},"body":"hoi :)\n\nOn Mon, May 21, 2007 at 02:20:05AM -0400, Shawn O. Pearce wrote:\n> > @@ -574,6 +575,21 @@ static void update_file_flags(const unsigned char *sha,\n> >  \t\tvoid *buf;\n> >  \t\tunsigned long size;\n> >  \n> > +\t\tif (S_ISDIRLNK(mode)) {\n> > +\t\t\t/* defer dirlinks to another process, don't try to */\n> > +\t\t\t/* read the object \"sha\" here */\n> > +\t\t\tconst char *dirlink_checkout[] = {\n> > +\t\t\t\t\"dirlink-checkout\", path, sha1_to_hex(sha), NULL\n> > +\t\t\t};\n> > +\t\t\tstruct child_process cmd = {\n> > +\t\t\t\t.argv = dirlink_checkout,\n> > +\t\t\t\t.git_cmd = 1,\n> > +\t\t\t};\n> \n> My Solaris 9 system cannot compile this syntax, even though it is\n> a clean way to initalize the child_process.\n\nany special thing it does not like in the above code or does it just\nnot support structs that are initialized that way?\n\n> > +\tstatus = read(cmd.out, hex, sizeof(hex));\n> > +\tif (status != 40) return status;\n> \n> OK, this is probably just never trusting the OS, but shouldn't that\n> read be wrapped up in a loop, like our read_in_full?  We want 40\n> bytes here, and expect it, and the read call is allowed to return\n> as few as 1 byte....\n\nright.\n\n-- \nMartin Waitz\n"},{"id":"42859","messageId":"20070521073758.GP3141@spearce.org","threadId":"8245","inReplyTo":"20070521073253.GU5412@admingilde.org","subject":"Re: [PATCH v2] Submodule merge support","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-05-21T07:37:58Z","receivedAt":"2007-05-21T07:37:58Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Martin Waitz <tali@admingilde.org> wrote:\n> On Mon, May 21, 2007 at 02:20:05AM -0400, Shawn O. Pearce wrote:\n> > > @@ -574,6 +575,21 @@ static void update_file_flags(const unsigned char *sha,\n> > >  \t\tvoid *buf;\n> > >  \t\tunsigned long size;\n> > >  \n> > > +\t\tif (S_ISDIRLNK(mode)) {\n> > > +\t\t\t/* defer dirlinks to another process, don't try to */\n> > > +\t\t\t/* read the object \"sha\" here */\n> > > +\t\t\tconst char *dirlink_checkout[] = {\n> > > +\t\t\t\t\"dirlink-checkout\", path, sha1_to_hex(sha), NULL\n> > > +\t\t\t};\n> > > +\t\t\tstruct child_process cmd = {\n> > > +\t\t\t\t.argv = dirlink_checkout,\n> > > +\t\t\t\t.git_cmd = 1,\n> > > +\t\t\t};\n> > \n> > My Solaris 9 system cannot compile this syntax, even though it is\n> > a clean way to initalize the child_process.\n> \n> any special thing it does not like in the above code or does it just\n> not support structs that are initialized that way?\n\nIts a very old Sun C compiler, and it doesn't like structs to be\ninitialized that way.  Yes, newer compilers are better, and gcc is\nalso better, but I'm unable to get our UNIX admins to actually do\ntheir job and keep systems usable by the users.\n\n/me starts to wonder why he continues with this day-job thing...\n\n-- \nShawn.\n"},{"id":"42862","messageId":"7vabvyfw7n.fsf@assigned-by-dhcp.cox.net","threadId":"8245","inReplyTo":"20070521073253.GU5412@admingilde.org","subject":"Re: [PATCH v2] Submodule merge support","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-21T07:54:20Z","receivedAt":"2007-05-21T07:54:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Waitz <tali@admingilde.org> writes:\n\n>> > +\t\tif (S_ISDIRLNK(mode)) {\n>> > +\t\t\t/* defer dirlinks to another process, don't try to */\n>> > +\t\t\t/* read the object \"sha\" here */\n>> > +\t\t\tconst char *dirlink_checkout[] = {\n>> > +\t\t\t\t\"dirlink-checkout\", path, sha1_to_hex(sha), NULL\n>> > +\t\t\t};\n>> > +\t\t\tstruct child_process cmd = {\n>> > +\t\t\t\t.argv = dirlink_checkout,\n>> > +\t\t\t\t.git_cmd = 1,\n>> > +\t\t\t};\n>> \n>> My Solaris 9 system cannot compile this syntax, even though it is\n>> a clean way to initalize the child_process.\n>\n> any special thing it does not like in the above code or does it just\n> not support structs that are initialized that way?\n\nPortability rules:\n\n - We do not do C99 initializers;\n - We do not do decl-after-statement;\n\nReadability rules:\n\n - We always write NULL, not 0, for a NULL pointer.\n\nThere may be a handful more unwritten rules we use.\n\n>> > +\tstatus = read(cmd.out, hex, sizeof(hex));\n>> > +\tif (status != 40) return status;\n>> \n>> OK, this is probably just never trusting the OS, but shouldn't that\n>> read be wrapped up in a loop, like our read_in_full?  We want 40\n>> bytes here, and expect it, and the read call is allowed to return\n>> as few as 1 byte....\n>\n> right.\n\nI think we have read-in-full or something like that for this\nexact purpose.\n"},{"id":"42865","messageId":"7v646mfw6b.fsf@assigned-by-dhcp.cox.net","threadId":"8245","inReplyTo":"20070521073758.GP3141@spearce.org","subject":"Re: [PATCH v2] Submodule merge support","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-21T07:55:08Z","receivedAt":"2007-05-21T07:55:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> writes:\n\n> Its a very old Sun C compiler, and it doesn't like structs to be\n> initialized that way.  Yes, newer compilers are better, and gcc is\n> also better, but I'm unable to get our UNIX admins to actually do\n> their job and keep systems usable by the users.\n>\n> /me starts to wonder why he continues with this day-job thing...\n\nTime for a \"git company\" ;-)?\n"},{"id":"42890","messageId":"Pine.LNX.4.64.0705211347540.6410@racer.site","threadId":"8245","inReplyTo":"7vabvyfw7n.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH] SubmittingPatches: mention older C compiler compatibility","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-05-21T12:48:49Z","receivedAt":"2007-05-21T12:48:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nWe do not appreciate C99 initializers, declarations after statements,\nor \"0\" instead of \"NULL\".\n\nSigned-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n\n---\n\n\tOn Mon, 21 May 2007, Junio C Hamano wrote:\n\t\n\t> Portability rules:\n\t> \n\t>  - We do not do C99 initializers;\n\t>  - We do not do decl-after-statement;\n\t> \n\t> Readability rules:\n\t> \n\t>  - We always write NULL, not 0, for a NULL pointer.\n\t> \n\t> There may be a handful more unwritten rules we use.\n\n\t... so let's start with these 3.\n\n Documentation/SubmittingPatches |   13 +++++++++++++\n 1 files changed, 13 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches\nindex 6a4da2d..cc74b4b 100644\n--- a/Documentation/SubmittingPatches\n+++ b/Documentation/SubmittingPatches\n@@ -65,6 +65,19 @@ in templates/hooks--pre-commit.  To help ensure this does not happen,\n run git diff --check on your changes before you commit.\n \n \n+(1a) Try to be nice to older C compilers\n+\n+We pride ourselves with the wide range of C compilers you can compile\n+git with. That means that you should not use C99 initializers, even\n+if a lot of compilers grok it.\n+\n+Also, variables have to be declared at the beginning of the block\n+(you can check this with gcc, using the -Wdeclaration-after-statement\n+option).\n+\n+Another thing: NULL pointers shall be written as NULL, not as 0.\n+\n+\n (2) Generate your patch using git tools out of your commits.\n \n git based diff tools (git, Cogito, and StGIT included) generate\n"},{"id":"42898","messageId":"81b0412b0705210842jdbe2c97i6a89ca472e84bde6@mail.gmail.com","threadId":"8245","inReplyTo":"20070521073758.GP3141@spearce.org","subject":"Re: [PATCH v2] Submodule merge support","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-05-21T15:42:57Z","receivedAt":"2007-05-21T15:42:57Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 5/21/07, Shawn O. Pearce <spearce@spearce.org> wrote:\n> Its a very old Sun C compiler, and it doesn't like structs to be\n> initialized that way.  Yes, newer compilers are better, and gcc is\n> also better, but I'm unable to get our UNIX admins to actually do\n> their job and keep systems usable by the users.\n>\n> /me starts to wonder why he continues with this day-job thing...\n\nBecause the new job may involve windows\n"},{"id":"43431","messageId":"200705271639.35267.johan@herland.net","threadId":"8245","inReplyTo":"Pine.LNX.4.64.0705211347540.6410@racer.site","subject":"[PATCH] Add -Wdeclaration-after-statement to CFLAGS to help enforce the instructions in SubmittingPatches","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-05-27T14:39:35Z","receivedAt":"2007-05-27T14:39:35Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Signed-off-by: Johan Herland <johan@herland.net>\n---\nOn Monday 21 May 2007, Johannes Schindelin wrote:\n> \n> We do not appreciate C99 initializers, declarations after statements,\n> or \"0\" instead of \"NULL\".\n>\n> [...]\n> \n> +Also, variables have to be declared at the beginning of the block\n> +(you can check this with gcc, using the -Wdeclaration-after-statement\n> +option).\n\nWhy not automatically enforce it by putting -Wdeclaration-after-statement\nin the Makefile?\n\nIt should probably be protected by some GCC if-ery, but then again, so should this:\n\nCC = gcc\n\n\nHave fun!\n\n...Johan\n\n\n Makefile |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 29243c6..4e91516 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -135,7 +135,7 @@ uname_P := $(shell sh -c 'uname -p 2>/dev/null || echo not')\n \n # CFLAGS and LDFLAGS are for the users to override from the command line.\n \n-CFLAGS = -g -O2 -Wall\n+CFLAGS = -g -O2 -Wall -Wdeclaration-after-statement\n LDFLAGS =\n ALL_CFLAGS = $(CFLAGS)\n ALL_LDFLAGS = $(LDFLAGS)\n-- \n1.5.2.101.gee49f\n"},{"id":"43433","messageId":"118833cc0705270758h1979eea2sc21ce351da03c6d3@mail.gmail.com","threadId":"8245","inReplyTo":"200705271639.35267.johan@herland.net","subject":"Re: [PATCH] Add -Wdeclaration-after-statement to CFLAGS to help enforce the instructions in SubmittingPatches","fromName":"Morten Welinder","fromEmail":"mwelinder@gmail.com","sentAt":"2007-05-27T14:58:05Z","receivedAt":"2007-05-27T14:58:05Z","isPatch":true,"sender":{"key":"mwelinder@gmail.com","avatar":null},"body":"> Why not automatically enforce it by putting -Wdeclaration-after-statement\n> in the Makefile?\n\nBecause -Wdeclaration-after-statement regretably is a fairly new thing.\n\nM.\n"}]}