[PATCH 0/6] Submodule support for git mv and git rm

[PATCH 0/6] Submodule support for git mv and git rm

[Petr Baudis]

This is a resend of the previously submitted series, with basically
no changes since the previous submission.

---

Petr Baudis (6):
      git submodule add: Fix naming clash handling
      t7400: Add short "git submodule add" testsuite
      t7403: Submodule git mv, git rm testsuite
      git mv: Support moving submodules
      git rm: Support for removing submodules
      submodule.*: Introduce simple C interface for submodule lookup by path


 Documentation/git-rm.txt   |    6 +
 Makefile                   |    2 
 builtin-mv.c               |   37 ++++++-
 builtin-rm.c               |   65 ++++++++++--
 git-submodule.sh           |   15 ++-
 submodule.c                |   50 +++++++++
 submodule.h                |    8 +
 t/t7400-submodule-basic.sh |   39 +++++++
 t/t7403-submodule-mvrm.sh  |  242 ++++++++++++++++++++++++++++++++++++++++++++
 9 files changed, 443 insertions(+), 21 deletions(-)
 create mode 100644 submodule.c
 create mode 100644 submodule.h
 create mode 100755 t/t7403-submodule-mvrm.sh

[PATCH 1/6] submodule.*: Introduce simple C interface for submodule lookup by path

[Petr Baudis]

The interface will be used for git-mv and git-rm submodule support.
So far, only the submodule_by_path() function is defined, however more
can be probably expected in the future if/when the git-submodule command
is ported from shell.

Signed-off-by: Petr Baudis <pasky@suse.cz>
---

 Makefile    |    2 ++
 submodule.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 submodule.h |    8 ++++++++
 3 files changed, 60 insertions(+), 0 deletions(-)
 create mode 100644 submodule.c
 create mode 100644 submodule.h

diff --git a/Makefile b/Makefile
index 247cd2d..c7c6478 100644
--- a/Makefile
+++ b/Makefile
@@ -384,6 +384,7 @@ LIB_H += run-command.h
 LIB_H += sha1-lookup.h
 LIB_H += sideband.h
 LIB_H += strbuf.h
+LIB_H += submodule.h
 LIB_H += tag.h
 LIB_H += transport.h
 LIB_H += tree.h
@@ -476,6 +477,7 @@ LIB_OBJS += sha1_name.o
 LIB_OBJS += shallow.o
 LIB_OBJS += sideband.o
 LIB_OBJS += strbuf.o
+LIB_OBJS += submodule.o
 LIB_OBJS += symlinks.o
 LIB_OBJS += tag.o
 LIB_OBJS += trace.o
diff --git a/submodule.c b/submodule.c
new file mode 100644
index 0000000..2883ae6
--- /dev/null
+++ b/submodule.c
@@ -0,0 +1,50 @@
+#include "cache.h"
+#include "submodule.h"
+
+
+struct gitmodules_info {
+	const char *path;
+	char *key;
+};
+
+static int gitmodules_worker(const char *key, const char *value, void *info_)
+{
+	struct gitmodules_info *info = info_;
+	const char *subkey;
+
+	if (prefixcmp(key, "submodule."))
+		return 0;
+
+	subkey = strrchr(key, '.');
+	if (!subkey)
+		return 0;
+
+	if (strcmp(subkey, ".path"))
+		return 0;
+
+	if (strcmp(value, info->path))
+		return 0;
+
+	/* Found the key to change. */
+	if (info->key) {
+		error("multiple submodules live at path `%s'", info->path);
+		/* The last one is supposed to win. */
+		free(info->key);
+	}
+	info->key = xstrdup(key);
+	return 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;
+}
diff --git a/submodule.h b/submodule.h
new file mode 100644
index 0000000..bc74fa0
--- /dev/null
+++ b/submodule.h
@@ -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). */
+extern char *submodule_by_path(const char *path);
+
+#endif

Re: [PATCH 1/6] submodule.*: Introduce simple C interface for submodule lookup by path

[Junio C Hamano]

Petr Baudis <pasky@suse.cz> writes:

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

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

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

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

This will segfault on a misconfigured:

	[submodule "xyzzy"]
        	path

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

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

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

> +}
> +
> +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;
> +}
> diff --git a/submodule.h b/submodule.h
> new file mode 100644
> index 0000000..bc74fa0
> --- /dev/null
> +++ b/submodule.h
> @@ -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?

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

Re: [PATCH 1/6] submodule.*: Introduce simple C interface for submodule lookup by path

[Jakub Narebski]

Junio C Hamano <gitster@pobox.com> writes:

> Petr Baudis <pasky@suse.cz> writes:

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

Are there any api to access and manipulate configuration,
including fqvn with the subsection part, instead of having
to do this again and again?

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH 1/6] submodule.*: Introduce simple C interface for submodule lookup by path

[Petr Baudis]

On Fri, Sep 12, 2008 at 02:23:51PM -0700, Junio C Hamano wrote:
> Petr Baudis <pasky@suse.cz> writes:
> 
> > +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?

Good point.

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

This looks strange, but I think I see what do you mean. I will use

if (strcmp(subkey, ".path") || subkey == key + strlen("submodule.") - 1)

> > +	if (strcmp(value, info->path))
> > +		return 0;
> 
> This will segfault on a misconfigured:
> 
> 	[submodule "xyzzy"]
>         	path

Thanks.

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

Matter of taste, I suppose. I have changed this to warning(), though it
is dubious configuration at best.

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

They follow up in the next patches. ;-)  They use the submodule name
only to access the configuration again, so this format is the most
convenient for them.

> > +}
> > +
> > +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;
> > +}
> > diff --git a/submodule.h b/submodule.h
> > new file mode 100644
> > index 0000000..bc74fa0
> > --- /dev/null
> > +++ b/submodule.h
> > @@ -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?

Would you seriously find it prettier with the newline?

$ git grep '^[^/]*[a-z][^/]*\*/$' *.c | wc -l
37

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

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

[Petr Baudis]

This patch adds support for removing submodules to 'git rm', including
appropriately editing the .gitmodules file to reflect this. Submodule
_checkouts_ are never removed; that can be potentially catastrophic
and the user should remove them manually, if really desired.

Signed-off-by: Petr Baudis <pasky@suse.cz>
---

 Documentation/git-rm.txt |    6 +++-
 builtin-rm.c             |   65 ++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 58 insertions(+), 13 deletions(-)

diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt
index 5afb1e7..0c92687 100644
--- a/Documentation/git-rm.txt
+++ b/Documentation/git-rm.txt
@@ -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.
 
 
 OPTIONS
@@ -57,7 +58,8 @@ OPTIONS
 --cached::
 	Use this option to unstage and remove paths only from the index.
 	Working tree files, whether modified or not, will be
-	left alone.
+	left alone.  Note that this is always assumed when removing
+	a checked-out submodule.
 
 --ignore-unmatch::
 	Exit with a zero status even if no files matched.
diff --git a/builtin-rm.c b/builtin-rm.c
index 6bd8211..7475de2 100644
--- a/builtin-rm.c
+++ b/builtin-rm.c
@@ -9,6 +9,7 @@
 #include "cache-tree.h"
 #include "tree-walk.h"
 #include "parse-options.h"
+#include "submodule.h"
 
 static const char * const builtin_rm_usage[] = {
 	"git rm [options] [--] <file>...",
@@ -17,16 +18,21 @@ static const char * const builtin_rm_usage[] = {
 
 static struct {
 	int nr, alloc;
-	const char **name;
+	struct {
+		const char *name;
+		int is_gitlink;
+	} *info;
 } list;
 
-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));
 	}
-	list.name[list.nr++] = name;
+	list.info[list.nr].name = name;
+	list.info[list.nr].is_gitlink = is_gitlink;
+	list.nr++;
 }
 
 static int remove_file(const char *name)
@@ -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 */
+		if (!rmdir(name) || errno == ENOTEMPTY)
+			ret = errno = 0;
+	}
 
 	if (!ret && (slash = strrchr(name, '/'))) {
 		char *n = xstrdup(name);
@@ -65,7 +78,7 @@ static int check_local_mod(unsigned char *head, int index_only)
 		struct stat st;
 		int pos;
 		struct cache_entry *ce;
-		const char *name = list.name[i];
+		const char *name = list.info[i].name;
 		unsigned char sha1[20];
 		unsigned mode;
 		int local_changes = 0;
@@ -83,7 +96,7 @@ static int check_local_mod(unsigned char *head, int index_only)
 			/* It already vanished from the working tree */
 			continue;
 		}
-		else if (S_ISDIR(st.st_mode)) {
+		else if (S_ISDIR(st.st_mode) && !S_ISGITLINK(ce->ce_mode)) {
 			/* if a file was removed and it is now a
 			 * directory, that is the same as ENOENT as
 			 * far as git is concerned; we do not track
@@ -122,6 +135,22 @@ static int check_local_mod(unsigned char *head, int index_only)
 	return errs;
 }
 
+static void remove_submodule(const char *name)
+{
+	char *key = submodule_by_path(name);
+	char *sectend = strrchr(key, '.');
+
+	assert(sectend);
+	*sectend = 0;
+
+	config_exclusive_filename = ".gitmodules";
+	if (git_config_rename_section(key, NULL) <= 0)
+		die("cannot remove section `%s' from .gitmodules", key);
+	config_exclusive_filename = NULL;
+
+	free(key);
+}
+
 static struct lock_file lock_file;
 
 static int show_only = 0, force = 0, index_only = 0, recursive = 0, quiet = 0;
@@ -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;
 	const char **pathspec;
 	char *seen;
 
@@ -168,7 +197,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 		struct cache_entry *ce = active_cache[i];
 		if (!match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, seen))
 			continue;
-		add_list(ce->name);
+		add_list(ce->name, S_ISGITLINK(ce->ce_mode));
 	}
 
 	if (pathspec) {
@@ -216,9 +245,11 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	 * the index unless all of them succeed.
 	 */
 	for (i = 0; i < list.nr; i++) {
-		const char *path = list.name[i];
+		const char *path = list.info[i].name;
 		if (!quiet)
-			printf("rm '%s'\n", path);
+			printf("rm%s '%s'\n",
+				list.info[i].is_gitlink ? "dir" : "",
+				path);
 
 		if (remove_file_from_cache(path))
 			die("git rm: unable to remove %s", path);
@@ -238,7 +269,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	if (!index_only) {
 		int removed = 0;
 		for (i = 0; i < list.nr; i++) {
-			const char *path = list.name[i];
+			const char *path = list.info[i].name;
 			if (!remove_file(path)) {
 				removed = 1;
 				continue;
@@ -248,6 +279,18 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 		}
 	}
 
+	/*
+	 * Get rid of stale submodule setup.
+	 */
+	subs = 0;
+	for (i = 0; i < list.nr; i++)
+		if (list.info[i].is_gitlink) {
+			remove_submodule(list.info[i].name);
+			subs++;
+		}
+	if (subs && add_file_to_cache(".gitmodules", 0))
+		die("cannot add new .gitmodules to the index");
+
 	if (active_cache_changed) {
 		if (write_cache(newfd, active_cache, active_nr) ||
 		    commit_locked_index(&lock_file))

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

[Junio C Hamano]

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?

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

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

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

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

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

[Junio C Hamano]

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

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

[Petr Baudis]

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

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

[Junio C Hamano]

Petr Baudis <pasky@suse.cz> 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().

Hmmmm.  If I did...

        $ git init
	$ create and add normal paths
        $ git clone git://..../gitk.git gitk
        $ git add gitk
        : heh, I changed my mind
        $ git rm gitk

the last step would die, because I changed my mind before fully
initializing gitk repository as a proper submodule?

How would I get rid of the index entry to recover from the mistake?

        $ rm -fr gitk
        $ git rm gitk

would presumably fail the same way, no?  I hope I am misreading the
code...

We need to be extremely careful not to break people who do not (yet) have
[submodule "xyzzy"] entries in config and/or .gitmodules when dealing with
the gitlink entries in the index.

[PATCH 3/6] git mv: Support moving submodules

[Petr Baudis]

This patch adds support for moving submodules to 'git mv', including
rewriting of the .gitmodules file to reflect the movement.

Signed-off-by: Petr Baudis <pasky@suse.cz>
---

 builtin-mv.c |   37 +++++++++++++++++++++++++++++++++----
 1 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/builtin-mv.c b/builtin-mv.c
index 4f65b5a..52e3ea5 100644
--- a/builtin-mv.c
+++ b/builtin-mv.c
@@ -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>",
@@ -49,6 +50,24 @@ static const char *add_slash(const char *path)
 	return path;
 }
 
+static int ce_is_gitlink(int i)
+{
+	return i < 0 ? 0 : S_ISGITLINK(active_cache[i]->ce_mode);
+}
+
+static void rename_submodule(struct string_list_item *i)
+{
+	char *key = submodule_by_path(i->path);
+
+	config_exclusive_filename = ".gitmodules";
+	if (git_config_set(key, (const char *) i->util))
+		die("cannot update .gitmodules");
+	config_exclusive_filename = NULL;
+
+	free(key);
+}
+
+
 static struct lock_file lock_file;
 
 int cmd_mv(int argc, const char **argv, const char *prefix)
@@ -65,6 +84,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;
 	struct stat st;
 	struct string_list src_for_dst = {NULL, 0, 0, 0};
+	/* .path is source path, .util is destination path */
+	struct string_list submodules = {NULL, 0, 0, 0};
 
 	git_config(git_default_config, NULL);
 
@@ -84,7 +105,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		/* special case: "." was normalized to "" */
 		destination = copy_pathspec(dest_path[0], argv, argc, 1);
 	else if (!lstat(dest_path[0], &st) &&
-			S_ISDIR(st.st_mode)) {
+			S_ISDIR(st.st_mode) &&
+			!ce_is_gitlink(cache_name_pos(dest_path[0], strlen(dest_path[0])))) {
 		dest_path[0] = add_slash(dest_path[0]);
 		destination = copy_pathspec(dest_path[0], argv, argc, 1);
 	} else {
@@ -96,7 +118,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	/* Checking */
 	for (i = 0; i < argc; i++) {
 		const char *src = source[i], *dst = destination[i];
-		int length, src_is_dir;
+		int length, src_is_dir, src_cache_pos;
 		const char *bad = NULL;
 
 		if (show_only)
@@ -111,7 +133,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		} else if ((src_is_dir = S_ISDIR(st.st_mode))
 				&& lstat(dst, &st) == 0)
 			bad = "cannot move directory over file";
-		else if (src_is_dir) {
+		else if ((src_cache_pos = cache_name_pos(src, length)) < 0 && src_is_dir) {
 			const char *src_w_slash = add_slash(src);
 			int len_w_slash = length + 1;
 			int first, last;
@@ -177,7 +199,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				} else
 					bad = "Cannot overwrite";
 			}
-		} else if (cache_name_pos(src, length) < 0)
+		} else if (src_cache_pos < 0)
 			bad = "not under version control";
 		else if (string_list_has_string(&src_for_dst, dst))
 			bad = "multiple sources for the same target";
@@ -214,10 +236,17 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 
 		pos = cache_name_pos(src, strlen(src));
 		assert(pos >= 0);
+		if (ce_is_gitlink(pos))
+			string_list_insert(src, &submodules)->util = (void *) dst;
 		if (!show_only)
 			rename_cache_entry_at(pos, dst);
 	}
 
+	for (i = 0; i < submodules.nr; i++)
+		rename_submodule(&submodules.items[i]);
+	if (submodules.nr > 0 && add_file_to_cache(".gitmodules", 0))
+		die("cannot add new .gitmodules to the index");
+
 	if (active_cache_changed) {
 		if (write_cache(newfd, active_cache, active_nr) ||
 		    commit_locked_index(&lock_file))

[PATCH] git mv: Support moving submodules

[Petr Baudis]

This patch adds support for moving submodules to 'git mv', including
rewriting of the .gitmodules file to reflect the movement.

Signed-off-by: Petr Baudis <pasky@suse.cz>
---

I'm sorry, I was careless - this is the correct patch version, referring to
string_list_item.string instead of .path.

Also, the testsuite in patch 4 is for some reason marked as (c)'d by Dscho.
Not that it would be a big deal.

 builtin-mv.c |   37 +++++++++++++++++++++++++++++++++----
 1 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/builtin-mv.c b/builtin-mv.c
index 4f65b5a..2970acc 100644
--- a/builtin-mv.c
+++ b/builtin-mv.c
@@ -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>",
@@ -49,6 +50,24 @@ static const char *add_slash(const char *path)
 	return path;
 }
 
+static int ce_is_gitlink(int i)
+{
+	return i < 0 ? 0 : S_ISGITLINK(active_cache[i]->ce_mode);
+}
+
+static void rename_submodule(struct string_list_item *i)
+{
+	char *key = submodule_by_path(i->string);
+
+	config_exclusive_filename = ".gitmodules";
+	if (git_config_set(key, (const char *) i->util))
+		die("cannot update .gitmodules");
+	config_exclusive_filename = NULL;
+
+	free(key);
+}
+
+
 static struct lock_file lock_file;
 
 int cmd_mv(int argc, const char **argv, const char *prefix)
@@ -65,6 +84,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;
 	struct stat st;
 	struct string_list src_for_dst = {NULL, 0, 0, 0};
+	/* .string is source path, .util is destination path */
+	struct string_list submodules = {NULL, 0, 0, 0};
 
 	git_config(git_default_config, NULL);
 
@@ -84,7 +105,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		/* special case: "." was normalized to "" */
 		destination = copy_pathspec(dest_path[0], argv, argc, 1);
 	else if (!lstat(dest_path[0], &st) &&
-			S_ISDIR(st.st_mode)) {
+			S_ISDIR(st.st_mode) &&
+			!ce_is_gitlink(cache_name_pos(dest_path[0], strlen(dest_path[0])))) {
 		dest_path[0] = add_slash(dest_path[0]);
 		destination = copy_pathspec(dest_path[0], argv, argc, 1);
 	} else {
@@ -96,7 +118,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	/* Checking */
 	for (i = 0; i < argc; i++) {
 		const char *src = source[i], *dst = destination[i];
-		int length, src_is_dir;
+		int length, src_is_dir, src_cache_pos;
 		const char *bad = NULL;
 
 		if (show_only)
@@ -111,7 +133,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		} else if ((src_is_dir = S_ISDIR(st.st_mode))
 				&& lstat(dst, &st) == 0)
 			bad = "cannot move directory over file";
-		else if (src_is_dir) {
+		else if ((src_cache_pos = cache_name_pos(src, length)) < 0 && src_is_dir) {
 			const char *src_w_slash = add_slash(src);
 			int len_w_slash = length + 1;
 			int first, last;
@@ -177,7 +199,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				} else
 					bad = "Cannot overwrite";
 			}
-		} else if (cache_name_pos(src, length) < 0)
+		} else if (src_cache_pos < 0)
 			bad = "not under version control";
 		else if (string_list_has_string(&src_for_dst, dst))
 			bad = "multiple sources for the same target";
@@ -214,10 +236,17 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 
 		pos = cache_name_pos(src, strlen(src));
 		assert(pos >= 0);
+		if (ce_is_gitlink(pos))
+			string_list_insert(src, &submodules)->util = (void *) dst;
 		if (!show_only)
 			rename_cache_entry_at(pos, dst);
 	}
 
+	for (i = 0; i < submodules.nr; i++)
+		rename_submodule(&submodules.items[i]);
+	if (submodules.nr > 0 && add_file_to_cache(".gitmodules", 0))
+		die("cannot add new .gitmodules to the index");
+
 	if (active_cache_changed) {
 		if (write_cache(newfd, active_cache, active_nr) ||
 		    commit_locked_index(&lock_file))

Re: [PATCH] git mv: Support moving submodules

[Junio C Hamano]

Petr Baudis <pasky@suse.cz> writes:

> diff --git a/builtin-mv.c b/builtin-mv.c
> index 4f65b5a..2970acc 100644
> --- a/builtin-mv.c
> +++ b/builtin-mv.c
> @@ -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>",
> @@ -49,6 +50,24 @@ static const char *add_slash(const char *path)
>  	return path;
>  }
>  
> +static int ce_is_gitlink(int i)
> +{
> +	return i < 0 ? 0 : S_ISGITLINK(active_cache[i]->ce_mode);
> +}

This interface itself is ugly (why should a caller pass "it is unmerged or
does not exist" without checking?), and it also makes the hunk that begins
at 84/105 ugly.  Why not "path_is_gitlink(const char*)" and run
cache_name_pos() here instead?

The interface, even if it is internal, should be done with a better taste
than that, even though I understand that you wanted to reuse the cache_pos
for the source one while you check.

Oh, by the way, what should happen when you have an unmerged path in the
index and say "git mv path elsewhere" (this question is *not* limited to
submodules).  Compared to that, what _does_ happen with the current code,
and with your patch?

[PATCH 4/6] t7403: Submodule git mv, git rm testsuite

[Petr Baudis]

The testsuite for newly added submodule support in git mv, git rm.

Signed-off-by: Petr Baudis <pasky@suse.cz>
---

 t/t7403-submodule-mvrm.sh |  242 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 242 insertions(+), 0 deletions(-)
 create mode 100755 t/t7403-submodule-mvrm.sh

diff --git a/t/t7403-submodule-mvrm.sh b/t/t7403-submodule-mvrm.sh
new file mode 100755
index 0000000..9b50d6a
--- /dev/null
+++ b/t/t7403-submodule-mvrm.sh
@@ -0,0 +1,242 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Johannes Schindelin
+#
+
+test_description='Test submodules support in git mv and git rm'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+
+	(mkdir sub-repo &&
+	 cd sub-repo &&
+	 git init &&
+	 echo file > file &&
+	 git add file &&
+	 git commit -m "sub initial") &&
+	(cp -r sub-repo sub2-repo &&
+	 cd sub2-repo &&
+	 echo file2 > file &&
+	 git add file &&
+	 git commit -m "sub commit2") &&
+	git submodule add "$(pwd)/sub-repo" sub &&
+	git submodule add "$(pwd)/sub2-repo" sub2 &&
+	git commit -m initial &&
+	test "$(git config -f .gitmodules submodule.sub.path)" = "sub" &&
+	test "$(git config -f .gitmodules submodule.sub2.path)" = "sub2"
+
+'
+
+test_expect_success 'git mv of a submodule' '
+
+	git mv sub sub.moved &&
+	! test -d sub &&
+	test -d sub.moved/.git &&
+	! git ls-files --error-unmatch sub &&
+	test "$(git ls-files --stage --error-unmatch sub.moved | cut -d " " -f 1)" = 160000 &&
+	test "$(git config -f .gitmodules submodule.sub.path)" = "sub.moved" &&
+	! git config -f .gitmodules submodule.sub.moved.path
+
+'
+
+test_expect_success 'git submodule add vs. git mv' '
+
+	! git submodule add "$(pwd)/sub2-repo" sub.moved &&
+	git submodule add "$(pwd)/sub2-repo" sub &&
+	test -d sub/.git &&
+	test "$(git config -f .gitmodules submodule.sub.url)" = "$(pwd)/sub-repo" &&
+	test "$(git config -f .gitmodules submodule.sub.path)" = "sub.moved" &&
+	test "$(git config -f .gitmodules submodule.sub~.path)" = "sub"
+
+'
+
+test_expect_success 'git mv onto existing file' '
+
+	echo file > file &&
+	git add file &&
+	! git mv sub.moved file &&
+	test -d sub.moved &&
+	! test -d file/.git &&
+	test "$(git ls-files --stage --error-unmatch file | cut -d " " -f 1)" = 100644 &&
+	test "$(git ls-files --stage --error-unmatch sub.moved | cut -d " " -f 1)" = 160000 &&
+	test "$(git config -f .gitmodules submodule.sub.path)" = "sub.moved"
+
+'
+
+test_expect_success 'git mv onto existing directory' '
+
+	mkdir -p dir &&
+	echo file > dir/file &&
+	git add dir/file &&
+	git mv sub.moved dir &&
+	! test -d sub.moved &&
+	test -d dir/sub.moved/.git &&
+	! git ls-files --error-unmatch sub.moved &&
+	test "$(git ls-files --stage --error-unmatch dir/sub.moved | cut -d " " -f 1)" = 160000 &&
+	test "$(git config -f .gitmodules submodule.sub.path)" = "dir/sub.moved" &&
+	git mv dir/sub.moved . &&
+	test "$(git config -f .gitmodules submodule.sub.path)" = "sub.moved"
+
+'
+
+test_expect_success 'git mv onto existing submodule' '
+
+	! git mv sub.moved sub2 &&
+	test -d sub.moved/.git &&
+	! test -d sub2/sub.moved &&
+	test "$(git ls-files --stage --error-unmatch sub2 | cut -d " " -f 1)" = 160000 &&
+	test "$(git ls-files --stage --error-unmatch sub.moved | cut -d " " -f 1)" = 160000 &&
+	test "$(git config -f .gitmodules submodule.sub.path)" = "sub.moved"
+
+'
+
+test_expect_success 'git mv of multiple submodules' '
+
+	mkdir -p dir &&
+	git mv sub.moved sub dir &&
+	! test -d sub.moved &&
+	! test -d sub &&
+	test -d dir/sub.moved/.git &&
+	test -d dir/sub/.git &&
+	! git ls-files --error-unmatch sub.moved sub &&
+	test "$(git ls-files --stage --error-unmatch dir/sub.moved dir/sub | cut -d " " -f 1 | uniq)" = 160000 &&
+	! git config -f .gitmodules submodule.dir.path &&
+	test "$(git config -f .gitmodules submodule.sub.path)" = "dir/sub.moved" &&
+	test "$(git config -f .gitmodules submodule.sub~.path)" = "dir/sub"
+
+'
+
+test_expect_success 'git mv of multiple submodules back from a subdir' '
+
+	(cd dir && git mv sub.moved sub .. && cd ..) &&
+	test -d sub.moved &&
+	test -d sub &&
+	! test -d dir/sub.moved/.git &&
+	! test -d dir/sub/.git &&
+	! git ls-files --error-unmatch dir/sub.moved dir/sub &&
+	test "$(git ls-files --stage --error-unmatch sub.moved sub | cut -d " " -f 1 | uniq)" = 160000 &&
+	test "$(git config -f .gitmodules submodule.sub.path)" = "sub.moved" &&
+	test "$(git config -f .gitmodules submodule.sub~.path)" = "sub"
+
+'
+
+test_expect_success 'git mv of non-checked-out submodules' '
+
+	git clone . clone &&
+	(cd clone &&
+	test -d sub &&
+	test -d sub2 &&
+	! test -d sub/.git &&
+	! test -d sub2/.git &&
+	git ls-files --stage --error-unmatch sub sub2 > ls-files.out &&
+	mkdir -p dir &&
+	git mv sub sub2 dir &&
+	! test -d sub &&
+	! test -d sub2 &&
+	test -d dir/sub &&
+	test -d dir/sub2 &&
+	! git ls-files --error-unmatch sub sub2 &&
+	test "$(git ls-files --stage --error-unmatch dir/sub dir/sub2 | cut -d " " -f 1 | uniq)" = 160000 &&
+	git ls-files --stage --error-unmatch dir/sub dir/sub2 | sed "s#dir/##g" | diff - ls-files.out &&
+	test "$(git config -f .gitmodules submodule.sub.path)" = "dir/sub" &&
+	test "$(git config -f .gitmodules submodule.sub2.path)" = "dir/sub2" &&
+	(cd dir && git mv sub2 .. && cd ..) &&
+	test -d sub2 &&
+	! test -d dir/sub2 &&
+	! git ls-files --error-unmatch dir/sub2 &&
+	test "$(git ls-files --stage --error-unmatch sub2 | cut -d " " -f 1)" = 160000 &&
+	test "$(git config -f .gitmodules submodule.sub2.path)" = "sub2")
+
+'
+
+test_expect_success 'checkpointing state with git commit' '
+
+	git commit -m"checkpoint" -a &&
+	(cd clone && git commit -m"clone checkpoint" -a)
+
+'
+
+test_expect_success 'git rm of a regular submodule' '
+
+	git rm sub2 &&
+	test -d sub2/.git &&
+	! git ls-files --error-unmatch sub2 &&
+	! git config -f .gitmodules submodule.sub2.path &&
+	! git config -f .gitmodules submodule.sub2.url
+
+'
+
+test_expect_success 'git rm of a submodule with name different from path' '
+
+	git rm sub.moved &&
+	test -d sub.moved/.git &&
+	! git ls-files --error-unmatch sub.moved &&
+	! git config -f .gitmodules submodule.sub.path &&
+	! git config -f .gitmodules submodule.sub.url
+
+'
+
+test_expect_success 'git rm of a modified submodule' '
+
+	git mv sub dir/sub && # more fun with richer path
+	(cd dir/sub &&
+	 echo mod > file &&
+	 git commit -m "sub mod" file) &&
+	git add dir/sub &&
+	! git rm dir/sub &&
+	test -d dir/sub/.git &&
+	test "$(git ls-files --stage --error-unmatch dir/sub | cut -d " " -f 1)" = "160000" &&
+	git config -f .gitmodules submodule.sub~.path &&
+	git config -f .gitmodules submodule.sub~.url &&
+	git rm -f dir/sub &&
+	test -d dir/sub/.git &&
+	! git ls-files --error-unmatch dir/sub &&
+	! git config -f .gitmodules submodule.sub~.path &&
+	! git config -f .gitmodules submodule.sub~.url
+
+'
+
+test_expect_success 'git rm of a submodule from within a subdirectory' '
+
+	git submodule add "$(pwd)/sub-repo" sub-torm &&
+	mkdir -p dir &&
+	# -f since we did not commit the submodule
+	(cd dir && git rm -f ../sub-torm && cd ..) &&
+	test -d sub-torm/.git &&
+	! git ls-files --error-unmatch sub-torm &&
+	! git config -f .gitmodules submodule.sub-torm.path &&
+	! git config -f .gitmodules submodule.sub-torm.url
+
+'
+
+test_expect_success 'git rm of a non-checked-out submodule' '
+
+	(cd clone &&
+	test -d dir/sub &&
+	! test -d dir/sub/.git &&
+	git rm dir/sub &&
+	! test -d dir/sub &&
+	! git ls-files --error-unmatch dir/sub &&
+	! git config -f .gitmodules submodule.sub.path &&
+	! git config -f .gitmodules submodule.sub.url)
+
+'
+
+test_expect_success 'git rm of a non-checked-out submodule w/ different working tree' '
+
+	(cd clone &&
+	rmdir sub2 &&
+	echo cunning > sub2 &&
+	! git rm sub2 &&
+	test -f sub2 &&
+	test "$(git ls-files --stage --error-unmatch sub2 | cut -d " " -f 1)" = "160000" &&
+	git rm -f sub2 &&
+	! test -e sub2 &&
+	! git ls-files --error-unmatch sub2 &&
+	! git config -f .gitmodules submodule.sub2.path &&
+	! git config -f .gitmodules submodule.sub2.url)
+
+'
+
+test_done

[PATCH 5/6] t7400: Add short "git submodule add" testsuite

[Petr Baudis]

This patch introduces basic tests for

	git submodule add

covering the basic functionality and the -b parameter.

A trivial update --init test fix freeloads on this commit as well.

Signed-off-by: Petr Baudis <pasky@suse.cz>
---

 t/t7400-submodule-basic.sh |   28 +++++++++++++++++++++++++++-
 1 files changed, 27 insertions(+), 1 deletions(-)

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index be73f7b..8a002bc 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -200,7 +200,7 @@ test_expect_success 'update --init' '
 
 	mv init init2 &&
 	git config -f .gitmodules submodule.example.url "$(pwd)/init2" &&
-	git config --remove-section submodule.example
+	git config --remove-section submodule.example &&
 	git submodule update init > update.out &&
 	grep "not initialized" update.out &&
 	test ! -d init/.git &&
@@ -209,4 +209,30 @@ test_expect_success 'update --init' '
 
 '
 
+test_expect_success 'submodule add' '
+
+	git submodule add "$(pwd)/init2" init-added &&
+	test -d init-added/.git &&
+	[ "$(git config -f .gitmodules submodule.init-added.url)" = "$(pwd)/init2" ] &&
+	[ "$(git config -f .gitmodules submodule.init-added.path)" = "init-added" ]
+
+'
+
+test_expect_success 'submodule add -b' '
+
+	(
+		cd init2 &&
+		git checkout -b branch &&
+		echo t >s &&
+		git add s &&
+		git commit -m "change branch" &&
+		git checkout master
+	) &&
+	git submodule add -b branch -- "$(pwd)/init2" init-added-b &&
+	test -d init-added-b/.git &&
+	[ "$(git config -f .gitmodules submodule.init-added-b.url)" = "$(pwd)/init2" ] &&
+	[ "$(cd init2 && git rev-parse branch)" = "$(cd init-added-b && git rev-parse HEAD)" ]
+
+'
+
 test_done

[PATCH 6/6] git submodule add: Fix naming clash handling

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

A test-case is obviously included.

This is not completely satisfactory since .git/config cross-commit
conflicts can still occur. A question is whether this is worth
handling, maybe it would be worth adding some kind of randomization
of the autogenerated submodule name, e.g. appending $$ or a timestamp.

Signed-off-by: Petr Baudis <pasky@suse.cz>
---

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

diff --git a/git-submodule.sh b/git-submodule.sh
index 1c39b59..3e4d839 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -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"
+			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')"
 }
 
 #
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 8a002bc..0d12a37 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -235,4 +235,15 @@ test_expect_success 'submodule add -b' '
 
 '
 
+test_expect_success 'submodule add auto-naming clash' '
+
+	git submodule add "$(pwd)/init2/.git" example &&
+	test -d example/.git &&
+	[ "$(git config -f .gitmodules submodule.example.url)" = "$(pwd)/init2" ] &&
+	[ "$(git config -f .gitmodules submodule.example.path)" = "init" ]
+	[ "$(git config -f .gitmodules submodule.example~.url)" = "$(pwd)/init2/.git" ] &&
+	[ "$(git config -f .gitmodules submodule.example~.path)" = "example" ]
+
+'
+
 test_done

Re: [PATCH 6/6] git submodule add: Fix naming clash handling

[Junio C Hamano]

Petr Baudis <pasky@suse.cz> writes:

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

>  git-submodule.sh           |   15 ++++++++++++---
>  t/t7400-submodule-basic.sh |   11 +++++++++++
>  2 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 1c39b59..3e4d839 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -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"
> +			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?
How should "git submodule add" work from inside a subdirectory?

Re: [PATCH 6/6] git submodule add: Fix naming clash handling

[Lars Hjemli]

On Sat, Sep 13, 2008 at 4:24 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Petr Baudis <pasky@suse.cz> writes:
>
>> 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).
>
>>  git-submodule.sh           |   15 ++++++++++++---
>>  t/t7400-submodule-basic.sh |   11 +++++++++++
>>  2 files changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index 1c39b59..3e4d839 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -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"
>> +                     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.

Yeah, redirecting to /dev/null seems like a good idea.


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

I'd just exit with an informative error if submodule.$name already
exists in .git/config.


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

Not really, since `git submodule` must be executed from the toplevel
($SUBDIRECTORY_OK is undefined, so the cd_to_toplevel in cmd_sync and
cmd_summary seems to be noops).

--
larsh