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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 12, 2008, 21:49 UTC
Message-ID: <7v8wtxniez.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20080912210902.31628.7325.stgit@localhost>

Petr Baudis <pasky@suse.cz> writes:

Show 7 quoted lines

> @@ -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?

Show 13 quoted lines

> 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()?

Show 8 quoted lines

> @@ -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.
         */

Show 7 quoted lines

> +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".

Show 6 quoted lines

> @@ -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.

Other than that I did not see anything questionable in [2/6].