Re: [PATCH 2/6] git rm: Support for removing submodules

[Petr Baudis <pasky@suse.cz>]

I will collect your feedback on the whole series, then resend it from
scratch - sounds good?

On Fri, Sep 12, 2008 at 02:49:56PM -0700, Junio C Hamano wrote:
> Petr Baudis <pasky@suse.cz> writes:
> 
> > @@ -20,7 +20,8 @@ and no updates to their contents can be staged in the index,
> >  though that default behavior can be overridden with the `-f` option.
> >  When '--cached' is given, the staged content has to
> >  match either the tip of the branch or the file on disk,
> > -allowing the file to be removed from just the index.
> > +allowing the file to be removed from just the index;
> > +this is always the case when removing submodules.
> 
> Sorry, I read this three times but "this" is unclear to me.  Different and
> mutually incompatible interpretations I tried to understand it are:
> 
>  (1) When removing submodules, whether --cached or not, the index can
>      match either HEAD or the work tree; this is different from removing
>      regular blobs where the index must match with HEAD without --cached
>      nor -f;
> 
>  (2) When removing submodules with --cached, the index can match either
>      HEAD or the work tree and it is removed only from the index.  You
>      cannot remove submodules without --cached;
> 
>  (3) When removing submodules, the index can match either HEAD or the work
>      tree and it is removed only from the index, even if you did not give
>      --cached;
> 
> It later becomes clear that you meant (3) in the second hunk, but the
> first time reader of the resulting document (not this patch) won't be
> reading from bottom to top.
> 
> This is a leftover issue from ealier documentation 25dc720 (Clarify and
> fix English in "git-rm" documentation, 2008-04-16), but the description is
> unclear what should happen while working towards the initial commit
> (i.e. no HEAD yet).  I think check_local_mod() allows removal in such a
> case.  Perhaps you can clarify the point while at it, please?

I will have a look.

> > diff --git a/builtin-rm.c b/builtin-rm.c
> > index 6bd8211..7475de2 100644
> > --- a/builtin-rm.c
> > +++ b/builtin-rm.c
> > ...
> > -static void add_list(const char *name)
> > +static void add_list(const char *name, int is_gitlink)
> >  {
> >  	if (list.nr >= list.alloc) {
> >  		list.alloc = alloc_nr(list.alloc);
> > -		list.name = xrealloc(list.name, list.alloc * sizeof(const char *));
> > +		list.info = xrealloc(list.info, list.alloc * sizeof(*list.info));
> >  	}
> 
> ALLOC_GROW()?

Neat thing!

> > @@ -38,6 +44,13 @@ static int remove_file(const char *name)
> >  	if (ret && errno == ENOENT)
> >  		/* The user has removed it from the filesystem by hand */
> >  		ret = errno = 0;
> > +	if (ret && errno == EISDIR) {
> > +		/* This is a gitlink entry; try to remove at least the
> > +		 * directory if the submodule is not checked out; we always
> > +		 * leave the checked out ones as they are */
> 
> 	/*
> 	 * Style?
>          * for a multi-line comment.
>          */

Right - I will have to get used to this. ;-)

> > +static void remove_submodule(const char *name)
> > +{
> > +	char *key = submodule_by_path(name);
> > +	char *sectend = strrchr(key, '.');
> > +
> > +	assert(sectend);
> > +	*sectend = 0;
> 
> Here is one caller I questioned in my comments on [1/6].  It is clear this
> caller wants to use "submodule.xyzzy" out of "submodule.xyzzy.path".  The
> function returning "submodule.xyzzy.path" does not feel like a clean and
> reusable interface to me.  I'd suggest either returning "submodule.xyzzy"
> (that's too specialized only for this caller to my taste, though), or just
> "xyzzy" and have the caller synthesize whatever string it wants to use
> (yes, it loses microoptimization but do we really care about it in this
> codepath?), if you have other callers that want different strings around
> "xyzzy".

This is still easier to use than explicit snprintf(), but in the long
run, you're right that returning just the submodule name is the right
thing. I will change the API.

> > @@ -140,7 +169,7 @@ static struct option builtin_rm_options[] = {
> >  
> >  int cmd_rm(int argc, const char **argv, const char *prefix)
> >  {
> > -	int i, newfd;
> > +	int i, newfd, subs;
> 
> Perhaps hoist "int removed" up one scope level and reuse it?  I misread
> that you are counting the number of gitlinks in the index, not the number
> of gitlinks that is being removed, on my first read.  The variable is used
> for the latter.

Sensible idea.

On Fri, Sep 12, 2008 at 02:59:12PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> >> +{
> >> +	char *key = submodule_by_path(name);
> >> +	char *sectend = strrchr(key, '.');
> >> +
> >> +	assert(sectend);
> >> +	*sectend = 0;
> >
> > Here is one caller I questioned in my comments on [1/6]...
> 
> Another thing --- can submodule_by_path() ever return NULL saying "I do
> not see one in the configuration"?

No, it would rather die().

-- 
				Petr "Pasky" Baudis
The next generation of interesting software will be done
on the Macintosh, not the IBM PC.  -- Bill Gates