[PATCH, RFC] checkout: Attempt to checkout submodules

[PATCH, RFC] checkout: Attempt to checkout submodules

[Trevor Saunders]

If a user does git checkout HEAD -- path/to/submodule they'd expect the
submodule to be checked out to the commit that submodule is at in HEAD.
This is the most brute force possible way of try to do that, and so its
probably broken in some cases.  However I'm not terribly familiar with
git's internals and I'm not sure if this is even wanted so I'm starting
simple.  If people want this to work I can try and do something better.

Signed-off-by: Trevor Saunders <tbsaunde@tbsaunde.org>
---
 entry.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/entry.c b/entry.c
index 1eda8e9..2dbf5b9 100644
--- a/entry.c
+++ b/entry.c
@@ -1,6 +1,8 @@
 #include "cache.h"
+#include "argv-array.h"
 #include "blob.h"
 #include "dir.h"
+#include "run-command.h"
 #include "streaming.h"
 
 static void create_directories(const char *path, int path_len,
@@ -277,9 +279,25 @@ int checkout_entry(struct cache_entry *ce,
 		 * just do the right thing)
 		 */
 		if (S_ISDIR(st.st_mode)) {
-			/* If it is a gitlink, leave it alone! */
-			if (S_ISGITLINK(ce->ce_mode))
+			if (S_ISGITLINK(ce->ce_mode)) {
+				struct argv_array args = ARGV_ARRAY_INIT;
+				char sha1[41];
+
+				argv_array_push(&args, "checkout");
+
+				if (state->force)
+					argv_array_push(&args, "-f");
+
+				memcpy(sha1, sha1_to_hex(ce->sha1), 41);
+				argv_array_push(&args, sha1);
+				
+				run_command_v_opt_cd_env(args.argv,
+					       		 RUN_GIT_CMD, ce->name,
+							 NULL);
+				argv_array_clear(&args);
+
 				return 0;
+			}
 			if (!state->force)
 				return error("%s is a directory", path.buf);
 			remove_subtree(&path);
-- 
2.1.4

Re: [PATCH, RFC] checkout: Attempt to checkout submodules

[Junio C Hamano]

Trevor Saunders <tbsaunde@tbsaunde.org> writes:

> If a user does git checkout HEAD -- path/to/submodule they'd expect the
> submodule to be checked out to the commit that submodule is at in HEAD.

Hmmm.

Is it a good idea to do that unconditionally by hard-coding the
behaviour like this patch does?

Is it a good idea that hard-coded behaviour is "checkout [-f]"?

I think "git submodule update" is the command people use when they
want to "match" the working trees of submodules, and via the
configuration mechanism submodule.*.update, people can choose what
they mean by "match"ing.  Some people want to checkout the commit
specified in the superproject tree by detaching HEAD at it.  Some
people want to integrate by merging or rebasing.

> This is the most brute force possible way of try to do that, and so its
> probably broken in some cases.  However I'm not terribly familiar with
> git's internals and I'm not sure if this is even wanted so I'm starting
> simple.  If people want this to work I can try and do something better.
>
> Signed-off-by: Trevor Saunders <tbsaunde@tbsaunde.org>
> ---
>  entry.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/entry.c b/entry.c
> index 1eda8e9..2dbf5b9 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -1,6 +1,8 @@
>  #include "cache.h"
> +#include "argv-array.h"
>  #include "blob.h"
>  #include "dir.h"
> +#include "run-command.h"
>  #include "streaming.h"
>  
>  static void create_directories(const char *path, int path_len,
> @@ -277,9 +279,25 @@ int checkout_entry(struct cache_entry *ce,
>  		 * just do the right thing)
>  		 */
>  		if (S_ISDIR(st.st_mode)) {
> -			/* If it is a gitlink, leave it alone! */
> -			if (S_ISGITLINK(ce->ce_mode))
> +			if (S_ISGITLINK(ce->ce_mode)) {
> +				struct argv_array args = ARGV_ARRAY_INIT;
> +				char sha1[41];
> +
> +				argv_array_push(&args, "checkout");
> +
> +				if (state->force)
> +					argv_array_push(&args, "-f");
> +
> +				memcpy(sha1, sha1_to_hex(ce->sha1), 41);
> +				argv_array_push(&args, sha1);
> +				
> +				run_command_v_opt_cd_env(args.argv,
> +					       		 RUN_GIT_CMD, ce->name,
> +							 NULL);
> +				argv_array_clear(&args);
> +
>  				return 0;
> +			}
>  			if (!state->force)
>  				return error("%s is a directory", path.buf);
>  			remove_subtree(&path);

Re: [PATCH, RFC] checkout: Attempt to checkout submodules

[Trevor Saunders]

On Thu, Mar 19, 2015 at 11:53:10AM -0700, Junio C Hamano wrote:
> Trevor Saunders <tbsaunde@tbsaunde.org> writes:
> 
> > If a user does git checkout HEAD -- path/to/submodule they'd expect the
> > submodule to be checked out to the commit that submodule is at in HEAD.
> 
> Hmmm.
> 
> Is it a good idea to do that unconditionally by hard-coding the
> behaviour like this patch does?

if I was sure it was a good idea it wouldn't be an RFC :-)

> Is it a good idea that hard-coded behaviour is "checkout [-f]"?

I suspect it depends on how you end up in checkout_entry.  If you do git
checkout HEAD -- /some/file then you force over writing any changes to
/some/file so I think a user should probably expect when the path is to
a submodule the effect is the same, the path is forced to be in the state
it is in HEAD.

> I think "git submodule update" is the command people use when they
> want to "match" the working trees of submodules, and via the
> configuration mechanism submodule.*.update, people can choose what
> they mean by "match"ing.  Some people want to checkout the commit
> specified in the superproject tree by detaching HEAD at it.  Some
> people want to integrate by merging or rebasing.

 git submodule update is certainly the current way to deal with the
 situation that your checkout of the submodule is out of sync with what
 is in the containing repo.  However I think users who aren't familiar
 with submodules would expect to be able to use "standard" git tools to
 deal with them.  So if they see

diff --git a/git-core b/git-core
index bb85775..52cae64 160000
--- a/git-core
+++ b/git-core
@@ -1 +1 @@
-Subproject commit bb8577532add843833ebf8b5324f94f84cb71ca0
+Subproject commit 52cae643c5d49b7fa18a7a4c60c284f9ae2e2c71

I think they'd expect they could restore git-core to the state in head the same
way they could with any other file by running git checkout HEAD -- git-core,
and they'd be suprised when that sighlently did nothing.  I suppose its
an option to print a message saying that nothing is being done with the
submodule git submodule should be used, but that seems kind of
unhelpful.

On one hand it seems kind of user hostile to just toss out any changes
in the submodule that are uncommitted, on the other for any other path
it would seem weird to have git checkout trigger rebasing or merging.

Trev


> 
> > This is the most brute force possible way of try to do that, and so its
> > probably broken in some cases.  However I'm not terribly familiar with
> > git's internals and I'm not sure if this is even wanted so I'm starting
> > simple.  If people want this to work I can try and do something better.
> >
> > Signed-off-by: Trevor Saunders <tbsaunde@tbsaunde.org>
> > ---
> >  entry.c | 22 ++++++++++++++++++++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/entry.c b/entry.c
> > index 1eda8e9..2dbf5b9 100644
> > --- a/entry.c
> > +++ b/entry.c
> > @@ -1,6 +1,8 @@
> >  #include "cache.h"
> > +#include "argv-array.h"
> >  #include "blob.h"
> >  #include "dir.h"
> > +#include "run-command.h"
> >  #include "streaming.h"
> >  
> >  static void create_directories(const char *path, int path_len,
> > @@ -277,9 +279,25 @@ int checkout_entry(struct cache_entry *ce,
> >  		 * just do the right thing)
> >  		 */
> >  		if (S_ISDIR(st.st_mode)) {
> > -			/* If it is a gitlink, leave it alone! */
> > -			if (S_ISGITLINK(ce->ce_mode))
> > +			if (S_ISGITLINK(ce->ce_mode)) {
> > +				struct argv_array args = ARGV_ARRAY_INIT;
> > +				char sha1[41];
> > +
> > +				argv_array_push(&args, "checkout");
> > +
> > +				if (state->force)
> > +					argv_array_push(&args, "-f");
> > +
> > +				memcpy(sha1, sha1_to_hex(ce->sha1), 41);
> > +				argv_array_push(&args, sha1);
> > +				
> > +				run_command_v_opt_cd_env(args.argv,
> > +					       		 RUN_GIT_CMD, ce->name,
> > +							 NULL);
> > +				argv_array_clear(&args);
> > +
> >  				return 0;
> > +			}
> >  			if (!state->force)
> >  				return error("%s is a directory", path.buf);
> >  			remove_subtree(&path);
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH, RFC] checkout: Attempt to checkout submodules

[Junio C Hamano]

Trevor Saunders <tbsaunde@tbsaunde.org> writes:

> On one hand it seems kind of user hostile to just toss out any changes
> in the submodule that are uncommitted, on the other for any other path
> it would seem weird to have git checkout trigger rebasing or merging.

I think that is exactly why we do not do anything in this codepath.

I have a feeling that an optional feature that allows "git submodule
update" to happen automatically from this codepath might be
acceptable by the submodule folks, and they might even say it does
not even have to be optional but should be enabled by default.

But I do not think it would fly well to unconditionally run
"checkout -f" here.

Re: [PATCH, RFC] checkout: Attempt to checkout submodules

[Trevor Saunders]

On Thu, Mar 19, 2015 at 02:15:19PM -0700, Junio C Hamano wrote:
> Trevor Saunders <tbsaunde@tbsaunde.org> writes:
> 
> > On one hand it seems kind of user hostile to just toss out any changes
> > in the submodule that are uncommitted, on the other for any other path
> > it would seem weird to have git checkout trigger rebasing or merging.
> 
> I think that is exactly why we do not do anything in this codepath.

yeah, and not only is it weird, but git diff will still report that
there's a difference which I imagine people will find strange.

> I have a feeling that an optional feature that allows "git submodule
> update" to happen automatically from this codepath might be
> acceptable by the submodule folks, and they might even say it does
> not even have to be optional but should be enabled by default.

ok, that seems fairly reasonable.  I do kind of wonder though if it
shouldn't be 'git submodule update --checkout' but that would get us
kind of back to where we started.  I guess since the default is checkout
if you set the pref then you can be assumed to have some amount of idea
what your doing.

> But I do not think it would fly well to unconditionally run
> "checkout -f" here.

agreed

Trev

> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH, RFC] checkout: Attempt to checkout submodules

[Jens Lehmann]

Am 20.03.2015 um 01:13 schrieb Trevor Saunders:
> On Thu, Mar 19, 2015 at 02:15:19PM -0700, Junio C Hamano wrote:
>> Trevor Saunders <tbsaunde@tbsaunde.org> writes:
>> I have a feeling that an optional feature that allows "git submodule
>> update" to happen automatically from this codepath might be
>> acceptable by the submodule folks, and they might even say it does
>> not even have to be optional but should be enabled by default.
>
> ok, that seems fairly reasonable.  I do kind of wonder though if it
> shouldn't be 'git submodule update --checkout' but that would get us
> kind of back to where we started.  I guess since the default is checkout
> if you set the pref then you can be assumed to have some amount of idea
> what your doing.

Me thinks it should be "git checkout" for those submodules that have
their update setting set to 'checkout' (or not set at all). I'm not
sure yet if it makes sense to attempt a rebase or merge here, but that
can be added later if necessary.

>> But I do not think it would fly well to unconditionally run
>> "checkout -f" here.
>
> agreed

Using -f here is ok when you extend the appropriate verify functions
in unpack-trees.c to check that no modifications will be lost (unless
the original checkout is used with -f). See the commit 76dbdd62
("submodule: teach unpack_trees() to update submodules") in my github
repo at https://github.com/jlehmann/git-submod-enhancements for
the basic concept (There is already a fixup! for that a bit further
down the branch which handles submodule to file conversion, maybe one
or two other changes will be needed when the test suite covers all
relevant cases).

Re: [PATCH, RFC] checkout: Attempt to checkout submodules

[Trevor Saunders]

On Mon, Mar 23, 2015 at 09:01:48PM +0100, Jens Lehmann wrote:
> Am 20.03.2015 um 01:13 schrieb Trevor Saunders:
> >On Thu, Mar 19, 2015 at 02:15:19PM -0700, Junio C Hamano wrote:
> >>Trevor Saunders <tbsaunde@tbsaunde.org> writes:
> >>I have a feeling that an optional feature that allows "git submodule
> >>update" to happen automatically from this codepath might be
> >>acceptable by the submodule folks, and they might even say it does
> >>not even have to be optional but should be enabled by default.
> >
> >ok, that seems fairly reasonable.  I do kind of wonder though if it
> >shouldn't be 'git submodule update --checkout' but that would get us
> >kind of back to where we started.  I guess since the default is checkout
> >if you set the pref then you can be assumed to have some amount of idea
> >what your doing.
> 
> Me thinks it should be "git checkout" for those submodules that have
> their update setting set to 'checkout' (or not set at all). I'm not
> sure yet if it makes sense to attempt a rebase or merge here, but that
> can be added later if necessary.

sgtm

> >>But I do not think it would fly well to unconditionally run
> >>"checkout -f" here.
> >
> >agreed
> 
> Using -f here is ok when you extend the appropriate verify functions
> in unpack-trees.c to check that no modifications will be lost (unless
> the original checkout is used with -f). See the commit 76dbdd62
> ("submodule: teach unpack_trees() to update submodules") in my github
> repo at https://github.com/jlehmann/git-submod-enhancements for
> the basic concept (There is already a fixup! for that a bit further
> down the branch which handles submodule to file conversion, maybe one
> or two other changes will be needed when the test suite covers all
> relevant cases).

ah, I see your already working a more complete solution to this sort of
issue.  I'll get out of your way then unless you want help.

Trev

> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH, RFC] checkout: Attempt to checkout submodules

[Jens Lehmann]

Am 24.03.2015 um 19:30 schrieb Trevor Saunders:
> On Mon, Mar 23, 2015 at 09:01:48PM +0100, Jens Lehmann wrote:
>> Using -f here is ok when you extend the appropriate verify functions
>> in unpack-trees.c to check that no modifications will be lost (unless
>> the original checkout is used with -f). See the commit 76dbdd62
>> ("submodule: teach unpack_trees() to update submodules") in my github
>> repo at https://github.com/jlehmann/git-submod-enhancements for
>> the basic concept (There is already a fixup! for that a bit further
>> down the branch which handles submodule to file conversion, maybe one
>> or two other changes will be needed when the test suite covers all
>> relevant cases).
>
> ah, I see your already working a more complete solution to this sort of
> issue.  I'll get out of your way then unless you want help.

Help would be very much appreciated. I'm currently separating teaching
the builtin commands to recursively update submodules from my branch to
submit these changes first. The reason for that is not only that there
are current efforts to make pull and am builtin commands, but that we
need an extension to git diff for the scripted commands to work. If you
could help implementing "--ignore-submodules=noupdate" (which would only
ignore changes to those submodules that are not going to be updated)
while I'm working on the builtin commands, that would help a lot. This
would enable the scripted commands (e.g. rebase) to not ignore changes
to submodules that are supposed to be updated (like they still do in
the current version of my branch). And another pair of eyes on code and
tests would also be very good to have.