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

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 [off-list ref] writes:

quoted

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

quoted

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!

quoted

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

quoted

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

quoted

@@ -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 [off-list ref] writes:

quoted

quoted

+{
+	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