{"thread":{"id":"38845","subject":"[PATCH, RFC] checkout: Attempt to checkout submodules","startedAt":"2015-03-18T12:27:23Z","lastAt":"2015-03-25T20:16:21Z","messageCount":8,"participants":["Trevor Saunders","Junio C Hamano","Jens Lehmann"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"257947","messageId":"1426681643-7516-1-git-send-email-tbsaunde@tbsaunde.org","threadId":"38845","inReplyTo":null,"subject":"[PATCH, RFC] checkout: Attempt to checkout submodules","fromName":"Trevor Saunders","fromEmail":"tbsaunde@tbsaunde.org","sentAt":"2015-03-18T12:27:23Z","receivedAt":"2015-03-18T12:27:23Z","isPatch":true,"sender":{"key":"tbsaunde@tbsaunde.org","avatar":null},"body":"If a user does git checkout HEAD -- path/to/submodule they'd expect the\nsubmodule to be checked out to the commit that submodule is at in HEAD.\nThis is the most brute force possible way of try to do that, and so its\nprobably broken in some cases. However I'm not terribly familiar with\ngit's internals and I'm not sure if this is even wanted so I'm starting\nsimple. If people want this to work I can try and do something better.\n\nSigned-off-by: Trevor Saunders \n---\n entry.c | 22 ++++++++++++++++++++--\n 1 file changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex 1eda8e9..2dbf5b9 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -1,6 +1,8 @@\n #include \"cache.h\"\n+#include \"argv-array.h\"\n #include \"blob.h\"\n #include \"dir.h\"\n+#include \"run-command.h\"\n #include \"streaming.h\"\n \n static void create_directories(const char *path, int path_len,\n@@ -277,9 +279,25 @@ int checkout_entry(struct cache_entry *ce,\n \t\t * just do the right thing)\n \t\t */\n \t\tif (S_ISDIR(st.st_mode)) {\n-\t\t\t/* If it is a gitlink, leave it alone! */\n-\t\t\tif (S_ISGITLINK(ce->ce_mode))\n+\t\t\tif (S_ISGITLINK(ce->ce_mode)) {\n+\t\t\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\t\t\t\tchar sha1[41];\n+\n+\t\t\t\targv_array_push(&args, \"checkout\");\n+\n+\t\t\t\tif (state->force)\n+\t\t\t\t\targv_array_push(&args, \"-f\");\n+\n+\t\t\t\tmemcpy(sha1, sha1_to_hex(ce->sha1), 41);\n+\t\t\t\targv_array_push(&args, sha1);\n+\t\t\t\t\n+\t\t\t\trun_command_v_opt_cd_env(args.argv,\n+\t\t\t\t\t \t\t RUN_GIT_CMD, ce->name,\n+\t\t\t\t\t\t\t NULL);\n+\t\t\t\targv_array_clear(&args);\n+\n \t\t\t\treturn 0;\n+\t\t\t}\n \t\t\tif (!state->force)\n \t\t\t\treturn error(\"%s is a directory\", path.buf);\n \t\t\tremove_subtree(&path);\n-- \n2.1.4\n"},{"id":"258039","messageId":"xmqqy4msizu1.fsf@gitster.dls.corp.google.com","threadId":"38845","inReplyTo":"1426681643-7516-1-git-send-email-tbsaunde@tbsaunde.org","subject":"Re: [PATCH, RFC] checkout: Attempt to checkout submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-19T18:53:10Z","receivedAt":"2015-03-19T18:53:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Trevor Saunders writes:\n\n> If a user does git checkout HEAD -- path/to/submodule they'd expect the\n> submodule to be checked out to the commit that submodule is at in HEAD.\n\nHmmm.\n\nIs it a good idea to do that unconditionally by hard-coding the\nbehaviour like this patch does?\n\nIs it a good idea that hard-coded behaviour is \"checkout [-f]\"?\n\nI think \"git submodule update\" is the command people use when they\nwant to \"match\" the working trees of submodules, and via the\nconfiguration mechanism submodule.*.update, people can choose what\nthey mean by \"match\"ing. Some people want to checkout the commit\nspecified in the superproject tree by detaching HEAD at it. Some\npeople want to integrate by merging or rebasing.\n\n> This is the most brute force possible way of try to do that, and so its\n> probably broken in some cases. However I'm not terribly familiar with\n> git's internals and I'm not sure if this is even wanted so I'm starting\n> simple. If people want this to work I can try and do something better.\n>\n> Signed-off-by: Trevor Saunders \n> ---\n> entry.c | 22 ++++++++++++++++++++--\n> 1 file changed, 20 insertions(+), 2 deletions(-)\n>\n> diff --git a/entry.c b/entry.c\n> index 1eda8e9..2dbf5b9 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -1,6 +1,8 @@\n> #include \"cache.h\"\n> +#include \"argv-array.h\"\n> #include \"blob.h\"\n> #include \"dir.h\"\n> +#include \"run-command.h\"\n> #include \"streaming.h\"\n> \n> static void create_directories(const char *path, int path_len,\n> @@ -277,9 +279,25 @@ int checkout_entry(struct cache_entry *ce,\n> \t\t * just do the right thing)\n> \t\t */\n> \t\tif (S_ISDIR(st.st_mode)) {\n> -\t\t\t/* If it is a gitlink, leave it alone! */\n> -\t\t\tif (S_ISGITLINK(ce->ce_mode))\n> +\t\t\tif (S_ISGITLINK(ce->ce_mode)) {\n> +\t\t\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n> +\t\t\t\tchar sha1[41];\n> +\n> +\t\t\t\targv_array_push(&args, \"checkout\");\n> +\n> +\t\t\t\tif (state->force)\n> +\t\t\t\t\targv_array_push(&args, \"-f\");\n> +\n> +\t\t\t\tmemcpy(sha1, sha1_to_hex(ce->sha1), 41);\n> +\t\t\t\targv_array_push(&args, sha1);\n> +\t\t\t\t\n> +\t\t\t\trun_command_v_opt_cd_env(args.argv,\n> +\t\t\t\t\t \t\t RUN_GIT_CMD, ce->name,\n> +\t\t\t\t\t\t\t NULL);\n> +\t\t\t\targv_array_clear(&args);\n> +\n> \t\t\t\treturn 0;\n> +\t\t\t}\n> \t\t\tif (!state->force)\n> \t\t\t\treturn error(\"%s is a directory\", path.buf);\n> \t\t\tremove_subtree(&path);\n"},{"id":"258046","messageId":"20150319201509.GB21536@tsaunders-iceball.corp.tor1.mozilla.com","threadId":"38845","inReplyTo":"xmqqy4msizu1.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH, RFC] checkout: Attempt to checkout submodules","fromName":"Trevor Saunders","fromEmail":"tbsaunde@tbsaunde.org","sentAt":"2015-03-19T20:15:09Z","receivedAt":"2015-03-19T20:15:09Z","isPatch":true,"sender":{"key":"tbsaunde@tbsaunde.org","avatar":null},"body":"On Thu, Mar 19, 2015 at 11:53:10AM -0700, Junio C Hamano wrote:\n> Trevor Saunders writes:\n> \n> > If a user does git checkout HEAD -- path/to/submodule they'd expect the\n> > submodule to be checked out to the commit that submodule is at in HEAD.\n> \n> Hmmm.\n> \n> Is it a good idea to do that unconditionally by hard-coding the\n> behaviour like this patch does?\n\nif I was sure it was a good idea it wouldn't be an RFC :-)\n\n> Is it a good idea that hard-coded behaviour is \"checkout [-f]\"?\n\nI suspect it depends on how you end up in checkout_entry. If you do git\ncheckout HEAD -- /some/file then you force over writing any changes to\n/some/file so I think a user should probably expect when the path is to\na submodule the effect is the same, the path is forced to be in the state\nit is in HEAD.\n\n> I think \"git submodule update\" is the command people use when they\n> want to \"match\" the working trees of submodules, and via the\n> configuration mechanism submodule.*.update, people can choose what\n> they mean by \"match\"ing. Some people want to checkout the commit\n> specified in the superproject tree by detaching HEAD at it. Some\n> people want to integrate by merging or rebasing.\n\n git submodule update is certainly the current way to deal with the\n situation that your checkout of the submodule is out of sync with what\n is in the containing repo. However I think users who aren't familiar\n with submodules would expect to be able to use \"standard\" git tools to\n deal with them. So if they see\n\ndiff --git a/git-core b/git-core\nindex bb85775..52cae64 160000\n--- a/git-core\n+++ b/git-core\n@@ -1 +1 @@\n-Subproject commit bb8577532add843833ebf8b5324f94f84cb71ca0\n+Subproject commit 52cae643c5d49b7fa18a7a4c60c284f9ae2e2c71\n\nI think they'd expect they could restore git-core to the state in head the same\nway they could with any other file by running git checkout HEAD -- git-core,\nand they'd be suprised when that sighlently did nothing. I suppose its\nan option to print a message saying that nothing is being done with the\nsubmodule git submodule should be used, but that seems kind of\nunhelpful.\n\nOn one hand it seems kind of user hostile to just toss out any changes\nin the submodule that are uncommitted, on the other for any other path\nit would seem weird to have git checkout trigger rebasing or merging.\n\nTrev\n\n\n> \n> > This is the most brute force possible way of try to do that, and so its\n> > probably broken in some cases. However I'm not terribly familiar with\n> > git's internals and I'm not sure if this is even wanted so I'm starting\n> > simple. If people want this to work I can try and do something better.\n> >\n> > Signed-off-by: Trevor Saunders \n> > ---\n> > entry.c | 22 ++++++++++++++++++++--\n> > 1 file changed, 20 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/entry.c b/entry.c\n> > index 1eda8e9..2dbf5b9 100644\n> > --- a/entry.c\n> > +++ b/entry.c\n> > @@ -1,6 +1,8 @@\n> > #include \"cache.h\"\n> > +#include \"argv-array.h\"\n> > #include \"blob.h\"\n> > #include \"dir.h\"\n> > +#include \"run-command.h\"\n> > #include \"streaming.h\"\n> > \n> > static void create_directories(const char *path, int path_len,\n> > @@ -277,9 +279,25 @@ int checkout_entry(struct cache_entry *ce,\n> > \t\t * just do the right thing)\n> > \t\t */\n> > \t\tif (S_ISDIR(st.st_mode)) {\n> > -\t\t\t/* If it is a gitlink, leave it alone! */\n> > -\t\t\tif (S_ISGITLINK(ce->ce_mode))\n> > +\t\t\tif (S_ISGITLINK(ce->ce_mode)) {\n> > +\t\t\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n> > +\t\t\t\tchar sha1[41];\n> > +\n> > +\t\t\t\targv_array_push(&args, \"checkout\");\n> > +\n> > +\t\t\t\tif (state->force)\n> > +\t\t\t\t\targv_array_push(&args, \"-f\");\n> > +\n> > +\t\t\t\tmemcpy(sha1, sha1_to_hex(ce->sha1), 41);\n> > +\t\t\t\targv_array_push(&args, sha1);\n> > +\t\t\t\t\n> > +\t\t\t\trun_command_v_opt_cd_env(args.argv,\n> > +\t\t\t\t\t \t\t RUN_GIT_CMD, ce->name,\n> > +\t\t\t\t\t\t\t NULL);\n> > +\t\t\t\targv_array_clear(&args);\n> > +\n> > \t\t\t\treturn 0;\n> > +\t\t\t}\n> > \t\t\tif (!state->force)\n> > \t\t\t\treturn error(\"%s is a directory\", path.buf);\n> > \t\t\tremove_subtree(&path);\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"258057","messageId":"xmqq3850it94.fsf@gitster.dls.corp.google.com","threadId":"38845","inReplyTo":"20150319201509.GB21536@tsaunders-iceball.corp.tor1.mozilla.com","subject":"Re: [PATCH, RFC] checkout: Attempt to checkout submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-19T21:15:19Z","receivedAt":"2015-03-19T21:15:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Trevor Saunders writes:\n\n> On one hand it seems kind of user hostile to just toss out any changes\n> in the submodule that are uncommitted, on the other for any other path\n> it would seem weird to have git checkout trigger rebasing or merging.\n\nI think that is exactly why we do not do anything in this codepath.\n\nI have a feeling that an optional feature that allows \"git submodule\nupdate\" to happen automatically from this codepath might be\nacceptable by the submodule folks, and they might even say it does\nnot even have to be optional but should be enabled by default.\n\nBut I do not think it would fly well to unconditionally run\n\"checkout -f\" here.\n"},{"id":"258076","messageId":"20150320001345.GC21536@tsaunders-iceball.corp.tor1.mozilla.com","threadId":"38845","inReplyTo":"xmqq3850it94.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH, RFC] checkout: Attempt to checkout submodules","fromName":"Trevor Saunders","fromEmail":"tbsaunde@tbsaunde.org","sentAt":"2015-03-20T00:13:45Z","receivedAt":"2015-03-20T00:13:45Z","isPatch":true,"sender":{"key":"tbsaunde@tbsaunde.org","avatar":null},"body":"On Thu, Mar 19, 2015 at 02:15:19PM -0700, Junio C Hamano wrote:\n> Trevor Saunders writes:\n> \n> > On one hand it seems kind of user hostile to just toss out any changes\n> > in the submodule that are uncommitted, on the other for any other path\n> > it would seem weird to have git checkout trigger rebasing or merging.\n> \n> I think that is exactly why we do not do anything in this codepath.\n\nyeah, and not only is it weird, but git diff will still report that\nthere's a difference which I imagine people will find strange.\n\n> I have a feeling that an optional feature that allows \"git submodule\n> update\" to happen automatically from this codepath might be\n> acceptable by the submodule folks, and they might even say it does\n> not even have to be optional but should be enabled by default.\n\nok, that seems fairly reasonable. I do kind of wonder though if it\nshouldn't be 'git submodule update --checkout' but that would get us\nkind of back to where we started. I guess since the default is checkout\nif you set the pref then you can be assumed to have some amount of idea\nwhat your doing.\n\n> But I do not think it would fly well to unconditionally run\n> \"checkout -f\" here.\n\nagreed\n\nTrev\n\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"258383","messageId":"5510712C.5090906@web.de","threadId":"38845","inReplyTo":"20150320001345.GC21536@tsaunders-iceball.corp.tor1.mozilla.com","subject":"Re: [PATCH, RFC] checkout: Attempt to checkout submodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2015-03-23T20:01:48Z","receivedAt":"2015-03-23T20:01:48Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 20.03.2015 um 01:13 schrieb Trevor Saunders:\n> On Thu, Mar 19, 2015 at 02:15:19PM -0700, Junio C Hamano wrote:\n>> Trevor Saunders writes:\n>> I have a feeling that an optional feature that allows \"git submodule\n>> update\" to happen automatically from this codepath might be\n>> acceptable by the submodule folks, and they might even say it does\n>> not even have to be optional but should be enabled by default.\n>\n> ok, that seems fairly reasonable. I do kind of wonder though if it\n> shouldn't be 'git submodule update --checkout' but that would get us\n> kind of back to where we started. I guess since the default is checkout\n> if you set the pref then you can be assumed to have some amount of idea\n> what your doing.\n\nMe thinks it should be \"git checkout\" for those submodules that have\ntheir update setting set to 'checkout' (or not set at all). I'm not\nsure yet if it makes sense to attempt a rebase or merge here, but that\ncan be added later if necessary.\n\n>> But I do not think it would fly well to unconditionally run\n>> \"checkout -f\" here.\n>\n> agreed\n\nUsing -f here is ok when you extend the appropriate verify functions\nin unpack-trees.c to check that no modifications will be lost (unless\nthe original checkout is used with -f). See the commit 76dbdd62\n(\"submodule: teach unpack_trees() to update submodules\") in my github\nrepo at https://github.com/jlehmann/git-submod-enhancements for\nthe basic concept (There is already a fixup! for that a bit further\ndown the branch which handles submodule to file conversion, maybe one\nor two other changes will be needed when the test suite covers all\nrelevant cases).\n"},{"id":"258440","messageId":"20150324183013.GA15658@tsaunders-iceball.corp.tor1.mozilla.com","threadId":"38845","inReplyTo":"5510712C.5090906@web.de","subject":"Re: [PATCH, RFC] checkout: Attempt to checkout submodules","fromName":"Trevor Saunders","fromEmail":"tbsaunde@tbsaunde.org","sentAt":"2015-03-24T18:30:13Z","receivedAt":"2015-03-24T18:30:13Z","isPatch":true,"sender":{"key":"tbsaunde@tbsaunde.org","avatar":null},"body":"On Mon, Mar 23, 2015 at 09:01:48PM +0100, Jens Lehmann wrote:\n> Am 20.03.2015 um 01:13 schrieb Trevor Saunders:\n> >On Thu, Mar 19, 2015 at 02:15:19PM -0700, Junio C Hamano wrote:\n> >>Trevor Saunders writes:\n> >>I have a feeling that an optional feature that allows \"git submodule\n> >>update\" to happen automatically from this codepath might be\n> >>acceptable by the submodule folks, and they might even say it does\n> >>not even have to be optional but should be enabled by default.\n> >\n> >ok, that seems fairly reasonable. I do kind of wonder though if it\n> >shouldn't be 'git submodule update --checkout' but that would get us\n> >kind of back to where we started. I guess since the default is checkout\n> >if you set the pref then you can be assumed to have some amount of idea\n> >what your doing.\n> \n> Me thinks it should be \"git checkout\" for those submodules that have\n> their update setting set to 'checkout' (or not set at all). I'm not\n> sure yet if it makes sense to attempt a rebase or merge here, but that\n> can be added later if necessary.\n\nsgtm\n\n> >>But I do not think it would fly well to unconditionally run\n> >>\"checkout -f\" here.\n> >\n> >agreed\n> \n> Using -f here is ok when you extend the appropriate verify functions\n> in unpack-trees.c to check that no modifications will be lost (unless\n> the original checkout is used with -f). See the commit 76dbdd62\n> (\"submodule: teach unpack_trees() to update submodules\") in my github\n> repo at https://github.com/jlehmann/git-submod-enhancements for\n> the basic concept (There is already a fixup! for that a bit further\n> down the branch which handles submodule to file conversion, maybe one\n> or two other changes will be needed when the test suite covers all\n> relevant cases).\n\nah, I see your already working a more complete solution to this sort of\nissue. I'll get out of your way then unless you want help.\n\nTrev\n\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"258517","messageId":"55131795.1050308@web.de","threadId":"38845","inReplyTo":"20150324183013.GA15658@tsaunders-iceball.corp.tor1.mozilla.com","subject":"Re: [PATCH, RFC] checkout: Attempt to checkout submodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2015-03-25T20:16:21Z","receivedAt":"2015-03-25T20:16:21Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 24.03.2015 um 19:30 schrieb Trevor Saunders:\n> On Mon, Mar 23, 2015 at 09:01:48PM +0100, Jens Lehmann wrote:\n>> Using -f here is ok when you extend the appropriate verify functions\n>> in unpack-trees.c to check that no modifications will be lost (unless\n>> the original checkout is used with -f). See the commit 76dbdd62\n>> (\"submodule: teach unpack_trees() to update submodules\") in my github\n>> repo at https://github.com/jlehmann/git-submod-enhancements for\n>> the basic concept (There is already a fixup! for that a bit further\n>> down the branch which handles submodule to file conversion, maybe one\n>> or two other changes will be needed when the test suite covers all\n>> relevant cases).\n>\n> ah, I see your already working a more complete solution to this sort of\n> issue. I'll get out of your way then unless you want help.\n\nHelp would be very much appreciated. I'm currently separating teaching\nthe builtin commands to recursively update submodules from my branch to\nsubmit these changes first. The reason for that is not only that there\nare current efforts to make pull and am builtin commands, but that we\nneed an extension to git diff for the scripted commands to work. If you\ncould help implementing \"--ignore-submodules=noupdate\" (which would only\nignore changes to those submodules that are not going to be updated)\nwhile I'm working on the builtin commands, that would help a lot. This\nwould enable the scripted commands (e.g. rebase) to not ignore changes\nto submodules that are supposed to be updated (like they still do in\nthe current version of my branch). And another pair of eyes on code and\ntests would also be very good to have.\n"}]}