Submodule support for git mv and git rm

+static int gitmodules_worker(const char *key, const char *value, void *info_)

submodule.c new file +15 -0

+}
+
+char *submodule_by_path(const char *path)
+{
+	struct gitmodules_info info = { path, NULL };
+
+	config_exclusive_filename = ".gitmodules";
+	if (git_config(gitmodules_worker, &info))
+		die("cannot process .gitmodules");
+	if (!info.key)
+		die("the submodule of `%s' not found in .gitmodules", path);
+	config_exclusive_filename = NULL;
+
+	return info.key;
+}

submodule.h new file +0 -0

new file mode 100644
index 0000000..bc74fa0

submodule.h new file +3 -0

+extern char *submodule_by_path(const char *path);
+
+#endif

Quoted 1 line · Petr Baudis

+static int gitmodules_worker(const char *key, const char *value, void *info_)

Won't you ever have different kind of work in the future? find_submodule_by_path(), perhaps?

submodule.c line 20

Quoted 10 lines

submodule.c new file +10 -0

+{
+	struct gitmodules_info *info = info_;
+	const char *subkey;
+
+	if (prefixcmp(key, "submodule."))
+		return 0;
+
+	subkey = strrchr(key, '.');
+	if (!subkey)
+		return 0;

This cannot happen; you made sure the thing begins with "submodule." already.

Review

Quoted 2 lines

(quoted diff) +2 -0

+	if (strcmp(subkey, ".path"))
+		return 0;

This will miss a misconfigured "submodule.path" (two level).

I can understand if this part were:

	subkey = strrchr(key, '.');
        if (!subkey || subkey == key + strlen("submodule.") - 1)
        	return 0;

Review

Quoted 2 lines

(quoted diff) +2 -0

+	if (strcmp(value, info->path))
+		return 0;

This will segfault on a misconfigured:

[submodule "xyzzy"]
path

submodule.c line 30

Quoted 3 lines

submodule.c new file +3 -0

+	/* Found the key to change. */
+	if (info->key) {
+		error("multiple submodules live at path `%s'", info->path);

Why is this "error()", not "warning()"?

submodule.c line 35

Quoted 5 lines

submodule.c new file +5 -0

+		/* The last one is supposed to win. */
+		free(info->key);
+	}
+	info->key = xstrdup(key);
+	return 0;

Have to wonder if it makes easier for the users if this function kept only "xyzzy" out of "submodule.xyzzy.path", not the whole thing. Cannot judge without actual callers.

Quoted 20 lines

submodule.c new file +15 -0

+}
+
+char *submodule_by_path(const char *path)
+{
+	struct gitmodules_info info = { path, NULL };
+
+	config_exclusive_filename = ".gitmodules";
+	if (git_config(gitmodules_worker, &info))
+		die("cannot process .gitmodules");
+	if (!info.key)
+		die("the submodule of `%s' not found in .gitmodules", path);
+	config_exclusive_filename = NULL;
+
+	return info.key;
+}

submodule.h new file +0 -0

new file mode 100644
index 0000000..bc74fa0

submodule.h line 5

Quoted 6 lines

submodule.h new file +5 -0

@@ -0,0 +1,8 @@
+#ifndef SUBMODULE_H
+#define SUBMODULE_H
+
+/* Find submodule living at given path in .gitmodules and return the key
+ * of its path config variable (dynamically allocated). */

Style?

submodule.c line 20

Quoted 10 lines · Petr Baudis

submodule.c new file +10 -0

+{
+	struct gitmodules_info *info = info_;
+	const char *subkey;
+
+	if (prefixcmp(key, "submodule."))
+		return 0;
+
+	subkey = strrchr(key, '.');
+	if (!subkey)
+		return 0;

This cannot happen; you made sure the thing begins with "submodule." already.

builtin-rm.c static int check_local_mod(unsigned char *head, int index_only) · line 144

Quoted 6 lines

builtin-rm.c +6 -0

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

Documentation/git-rm.txt and no updates to their contents can be staged in the index, · line 24

Quoted 7 lines · Petr Baudis

Documentation/git-rm.txt +2 -1

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

Quoted 7 lines

builtin-rm.c +1 -1

index 6bd8211..7475de2 100644
...
-static void add_list(const char *name)
+static void add_list(const char *name, int is_gitlink)

builtin-rm.c static const char * const builtin_rm_usage[] = { · line 32

Quoted 6 lines

{

builtin-rm.c +1 -1

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

Quoted 5 lines

builtin-rm.c +1 -0

@@ -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) {

builtin-rm.c static int remove_file(const char *name) · line 50

Quoted 3 lines

builtin-rm.c +3 -0

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

builtin-rm.c static int check_local_mod(unsigned char *head, int index_only) · line 144

Quoted 7 lines

builtin-rm.c +7 -0

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

builtin-rm.c static struct option builtin_rm_options[] = { · line 172

Quoted 6 lines

builtin-rm.c +1 -1

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

Quoted 8 lines · Junio C Hamano

builtin-rm.c +0 -0

 > +{
 > +	char *key = submodule_by_path(name);
 > +	char *sectend = strrchr(key, '.');

builtin-rm.c static int check_local_mod(unsigned char *head, int index_only) · line 144

Quoted 3 lines

builtin-rm.c +3 -0

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

Quoted 9 lines

Quoted 8 lines

builtin-rm.c static int check_local_mod(unsigned char *head, int index_only) · line 144

Quoted 6 lines

builtin-rm.c +6 -0

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

Quoted 2 lines

Another thing --- can submodule_by_path() ever return NULL saying "I do not see one in the configuration"?

No, it would rather die().

builtin-mv.c +1 -0

index 4f65b5a..2970acc 100644
@@ -9,6 +9,7 @@
 #include "cache-tree.h"
 #include "string-list.h"
 #include "parse-options.h"
+#include "submodule.h"
 
 static const char * const builtin_mv_usage[] = {
 	"git mv [options] <source>... <destination>",

This patch fixes git submodule add behaviour when we add submodule living at a same path as logical name of existing submodule. This can happen e.g. in case the user git mv's the previous submodule away and then git submodule add's another under the same name.

 git-submodule.sh           |   15 ++++++++++++---
 t/t7400-submodule-basic.sh |   11 +++++++++++
 2 files changed, 23 insertions(+), 3 deletions(-)

git-submodule.sh +5 -2

index 1c39b59..3e4d839 100755
@@ -201,10 +201,19 @@ cmd_add()
 	git add "$path" ||
 	die "Failed to add submodule '$path'"
 
-	git config -f .gitmodules submodule."$path".path "$path" &&
-	git config -f .gitmodules submodule."$path".url "$repo" &&
+	name="$path"
+	if git config -f .gitmodules submodule."$name".path; then
+		name="$path~"; i=1;
+		while git config -f .gitmodules submodule."$name".path; do
+			name="$path~$i"

Quoted 4 lines · Petr Baudis

This patch fixes git submodule add behaviour when we add submodule living at a same path as logical name of existing submodule. This can happen e.g. in case the user git mv's the previous submodule away and then git submodule add's another under the same name.

In short, name "example" was used to name the submodule bound at path "init" in earlier tests, and the new one adds another submodule whose name and path are both "example" and makes sure they do not get mixed up (and the original code did mix them up).

Quoted 19 lines

 git-submodule.sh           |   15 ++++++++++++---
 t/t7400-submodule-basic.sh |   11 +++++++++++
 2 files changed, 23 insertions(+), 3 deletions(-)

git-submodule.sh +5 -2

index 1c39b59..3e4d839 100755
@@ -201,10 +201,19 @@ cmd_add()
      git add "$path" ||
      die "Failed to add submodule '$path'"
 
-     git config -f .gitmodules submodule."$path".path "$path" &&
-     git config -f .gitmodules submodule."$path".url "$repo" &&
+     name="$path"
+     if git config -f .gitmodules submodule."$name".path; then
+             name="$path~"; i=1;
+             while git config -f .gitmodules submodule."$name".path; do
+                     name="$path~$i"

git-submodule.sh cmd_add() · line 217

Quoted 10 lines

git-submodule.sh +7 -1

+                     i=$((i+1))
+             done
+     fi
+
+     git config -f .gitmodules submodule."$name".path "$path" &&
+     git config -f .gitmodules submodule."$name".url "$repo" &&
      git add .gitmodules ||
-     die "Failed to register submodule '$path'"
+     die "Failed to register submodule '$path' (name '$name')"
 }

The logic of the fix seems to be correct, but shouldn't the test be like this instead?

if git config -f .gitmodules "submodule.$name.path" >/dev/null then

The same thing for "git config" used in the "while" loop.

Also I am not sure if name="$path~" is a good idea for two reasons:

- name suffixed with tilde and number looks too much like revision expression.

- A, A~, A~1, A~2... looks ugly; A, A-0, A-1, A-2,... (or start counting from 1 or 2) I would understand.

By the way, I noticed that cmd_add does not seem to cd_to_toplevel, and accesses .gitmodules in the current working directory. Isn't this a bug?