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

[Trevor Saunders <tbsaunde@tbsaunde.org>]

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