[PATCH v2 2/3] builtin-status: submodule summary support

[PATCH v2 2/3] builtin-status: submodule summary support

[Ping Yin]

This commit teaches 'git commit/status' show 'Modified submodules'
section given by

git submodule summary --cached --for-status --summary-limit <limit>

just before 'Untracked files' section.

The <limit> is given by the config variable status.submodulesummary
to limit the submodule summary size. status.submodulesummary is 0 by
default which disables the summary.

Also mention status.submodulesummary in the documentation.

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 Documentation/git-status.txt |    4 ++++
 wt-status.c                  |   39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 3ea269a..32b6660 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -52,6 +52,10 @@ If the config variable `status.relativePaths` is set to false, then all
 paths shown are relative to the repository root, not to the current
 directory.
 
+If 'status.submodulesummary' is set to a non zero number, the submodule
+summary will be enabled and a summary of commits for modified submodules
+will be shown (see --summary-limit option of linkgit:git-submodule[1]).
+
 See Also
 --------
 linkgit:gitignore[5]
diff --git a/wt-status.c b/wt-status.c
index b3fd57b..369c519 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -8,9 +8,11 @@
 #include "revision.h"
 #include "diffcore.h"
 #include "quote.h"
+#include "run-command.h"
 
 int wt_status_relative_paths = 1;
 int wt_status_use_color = -1;
+int wt_status_submodule_summary = 0;
 static char wt_status_colors[][COLOR_MAXLEN] = {
 	"",         /* WT_STATUS_HEADER: normal */
 	"\033[32m", /* WT_STATUS_UPDATED: green */
@@ -219,6 +221,38 @@ static void wt_status_print_changed(struct wt_status *s)
 	rev.diffopt.format_callback_data = s;
 	run_diff_files(&rev, 0);
 }
+static void wt_status_print_submodule_summary(struct wt_status *s)
+{
+	struct child_process sm_summary;
+	char summary_limit[64];
+	char index[PATH_MAX];
+	const char *env[] = { index, NULL };
+	const char *argv[] = {
+		"submodule",
+		"summary",
+		"--cached",
+		"--for-status",
+		"--summary-limit",
+		summary_limit,
+		s->amend ? "HEAD^" : "HEAD",
+		NULL
+	};
+
+	if (!wt_status_submodule_summary)
+		return;
+
+	sprintf(summary_limit, "%d", wt_status_submodule_summary);
+	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", s->index_file);
+
+	memset(&sm_summary, 0, sizeof(sm_summary));
+	sm_summary.argv = argv;
+	sm_summary.env = env;
+	sm_summary.git_cmd = 1;
+	sm_summary.no_stdin = 1;
+	fflush(s->fp);
+	sm_summary.out = dup(fileno(s->fp));    /* run_command closes it */
+	run_command(&sm_summary);
+}
 
 static void wt_status_print_untracked(struct wt_status *s)
 {
@@ -308,6 +342,7 @@ void wt_status_print(struct wt_status *s)
 	}
 
 	wt_status_print_changed(s);
+	wt_status_print_submodule_summary(s);
 	wt_status_print_untracked(s);
 
 	if (s->verbose && !s->is_initial)
@@ -330,6 +365,10 @@ void wt_status_print(struct wt_status *s)
 
 int git_status_config(const char *k, const char *v)
 {
+	if (!strcmp(k, "status.submodulesummary")) {
+		wt_status_submodule_summary = atoi(v);
+		return 0;
+	}
 	if (!strcmp(k, "status.color") || !strcmp(k, "color.status")) {
 		wt_status_use_color = git_config_colorbool(k, v, -1);
 		return 0;
-- 
1.5.5.23.g2a5f

[PATCH/RFC] submodule: fallback to .gitmodules and multiple level module definition

[Ping Yin]

This patch series has two functional improvements for submodule

  - Fall back on .gitmodules if info not found in $GIT_DIR/config
  - multi-level module definition

Patches 1,2,4 is mainly code refactor but the second one also
has some semantic change.

The other patches do the real functional changes.

Ping Yin (7):
      git-submodule: Extract functions module_info and module_url
      git-submodule: Extract absolute_url & move absolute url logic to module_clone
      git-submodule: Fall back on .gitmodules if info not found in $GIT_DIR/config
      git-submodule: Extract module_add from cmd_add
      git-submodule: multi-level module definition
      git-submodule: Don't die when command fails for one submodule
      git-submodule: "update --force" to enforce cloning non-submodule

 git-submodule.sh           |  326 ++++++++++++++++++++++++++++++++------------
 t/t7400-submodule-basic.sh |   31 ++++-
 2 files changed, 267 insertions(+), 90 deletions(-)

[PATCH/RFC 1/7] git-submodule: Extract functions module_info and module_url

[Ping Yin]

module_info is extracted to remove the logic redundance which acquires
module names and urls by path filter in several places.

module_url is also extracted to prepare for an alternative logic to get url by
module name.

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 git-submodule.sh |   40 ++++++++++++++++++++++++++++------------
 1 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 56ec353..a5002e1 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -82,6 +82,25 @@ module_name()
        echo "$name"
 }
 
+module_url() {
+	git config submodule.$1.url
+}
+
+module_info() {
+	git ls-files --stage -- "$@" | grep -e '^160000 ' |
+	while read mode sha1 stage path
+	do
+		name=$(module_name "$path")
+		if test -n "$name"
+		then
+			url=$(module_url "$name")
+			echo "$sha1	$path	$name	$url"
+		else
+			echo "$sha1	$path		"
+		fi
+	done
+}
+
 #
 # Clone a submodule
 #
@@ -232,12 +251,11 @@ cmd_init()
 		shift
 	done
 
-	git ls-files --stage -- "$@" | grep '^160000 ' |
-	while read mode sha1 stage path
+	module_info "$@" |
+	while read sha1 path name url
 	do
+		test -n "$name" || exit
 		# Skip already registered paths
-		name=$(module_name "$path") || exit
-		url=$(git config submodule."$name".url)
 		test -z "$url" || continue
 
 		url=$(GIT_CONFIG=.gitmodules git config submodule."$name".url)
@@ -286,11 +304,10 @@ cmd_update()
 		shift
 	done
 
-	git ls-files --stage -- "$@" | grep '^160000 ' |
-	while read mode sha1 stage path
+	module_info "$@" |
+	while read sha1 path name url
 	do
-		name=$(module_name "$path") || exit
-		url=$(git config submodule."$name".url)
+		test -n "$name" || exit
 		if test -z "$url"
 		then
 			# Only mention uninitialized submodules when its
@@ -537,11 +554,10 @@ cmd_status()
 		shift
 	done
 
-	git ls-files --stage -- "$@" | grep '^160000 ' |
-	while read mode sha1 stage path
+	module_info "$@" |
+	while read sha1 path name url
 	do
-		name=$(module_name "$path") || exit
-		url=$(git config submodule."$name".url)
+		test -n "$name" || exit
 		if test -z "$url" || ! test -d "$path"/.git
 		then
 			say "-$sha1 $path"
-- 
1.5.5.23.g2a5f

[PATCH v2 1/3] git-submodule summary: --for-status option

[Ping Yin]

The --for-status option is mainly used by builtin-status/commit.
It adds 'Modified submodules:' line at top and  '# ' prefix to all
following lines.

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 git-submodule.sh             |   17 ++++++++++++++++-
 t/t7401-submodule-summary.sh |   13 +++++++++++++
 2 files changed, 29 insertions(+), 1 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 56ec353..d7937a5 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -342,6 +342,7 @@ set_name_rev () {
 #
 cmd_summary() {
 	summary_limit=-1
+	for_status=
 
 	# parse $args after "submodule ... summary".
 	while test $# -ne 0
@@ -350,6 +351,9 @@ cmd_summary() {
 		--cached)
 			cached="$1"
 			;;
+		--for-status)
+			for_status="$1"
+			;;
 		-n|--summary-limit)
 			if summary_limit=$(($2 + 0)) 2>/dev/null && test "$summary_limit" = "$2"
 			then
@@ -398,6 +402,12 @@ cmd_summary() {
 	)
 
 	test -n "$modules" &&
+	if test -n "$for_status"; then
+		echo "# Modified submodules:"
+		echo "#"
+	else
+		true
+	fi &&
 	git diff-index $cached --raw $head -- $modules |
 	grep -e '^:160000' -e '^:[0-7]* 160000' |
 	cut -c2- |
@@ -499,7 +509,12 @@ cmd_summary() {
 			echo
 		fi
 		echo
-	done
+	done |
+	if test -n "$for_status"; then
+		sed -e "s|^|# |" -e 's|^# $|#|'
+	else
+		cat
+	fi
 }
 #
 # List all submodules, prefixed with:
diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
index 0f3c42a..1dbb39d 100755
--- a/t/t7401-submodule-summary.sh
+++ b/t/t7401-submodule-summary.sh
@@ -192,4 +192,17 @@ test_expect_success 'given commit' "
 EOF
 "
 
+test_expect_success '--for-status' "
+    git submodule summary --for-status HEAD^ >actual &&
+    diff actual - <<-EOF
+# Modified submodules:
+#
+# * sm1 $head6...0000000:
+#
+# * sm2 0000000...$head7 (2):
+#   > Add foo9
+#
+EOF
+"
+
 test_done
-- 
1.5.5.23.g2a5f

[PATCH v2 2/3] builtin-status: submodule summary support

[Ping Yin]

This commit teaches 'git commit/status' show 'Modified submodules'
section given by

git submodule summary --cached --for-status --summary-limit <limit>

just before 'Untracked files' section.

The <limit> is given by the config variable status.submodulesummary
to limit the submodule summary size. status.submodulesummary is 0 by
default which disables the summary.

Also mention status.submodulesummary in the documentation.

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 Documentation/git-status.txt |    4 ++++
 wt-status.c                  |   39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 3ea269a..32b6660 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -52,6 +52,10 @@ If the config variable `status.relativePaths` is set to false, then all
 paths shown are relative to the repository root, not to the current
 directory.
 
+If 'status.submodulesummary' is set to a non zero number, the submodule
+summary will be enabled and a summary of commits for modified submodules
+will be shown (see --summary-limit option of linkgit:git-submodule[1]).
+
 See Also
 --------
 linkgit:gitignore[5]
diff --git a/wt-status.c b/wt-status.c
index b3fd57b..369c519 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -8,9 +8,11 @@
 #include "revision.h"
 #include "diffcore.h"
 #include "quote.h"
+#include "run-command.h"
 
 int wt_status_relative_paths = 1;
 int wt_status_use_color = -1;
+int wt_status_submodule_summary = 0;
 static char wt_status_colors[][COLOR_MAXLEN] = {
 	"",         /* WT_STATUS_HEADER: normal */
 	"\033[32m", /* WT_STATUS_UPDATED: green */
@@ -219,6 +221,38 @@ static void wt_status_print_changed(struct wt_status *s)
 	rev.diffopt.format_callback_data = s;
 	run_diff_files(&rev, 0);
 }
+static void wt_status_print_submodule_summary(struct wt_status *s)
+{
+	struct child_process sm_summary;
+	char summary_limit[64];
+	char index[PATH_MAX];
+	const char *env[] = { index, NULL };
+	const char *argv[] = {
+		"submodule",
+		"summary",
+		"--cached",
+		"--for-status",
+		"--summary-limit",
+		summary_limit,
+		s->amend ? "HEAD^" : "HEAD",
+		NULL
+	};
+
+	if (!wt_status_submodule_summary)
+		return;
+
+	sprintf(summary_limit, "%d", wt_status_submodule_summary);
+	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", s->index_file);
+
+	memset(&sm_summary, 0, sizeof(sm_summary));
+	sm_summary.argv = argv;
+	sm_summary.env = env;
+	sm_summary.git_cmd = 1;
+	sm_summary.no_stdin = 1;
+	fflush(s->fp);
+	sm_summary.out = dup(fileno(s->fp));    /* run_command closes it */
+	run_command(&sm_summary);
+}
 
 static void wt_status_print_untracked(struct wt_status *s)
 {
@@ -308,6 +342,7 @@ void wt_status_print(struct wt_status *s)
 	}
 
 	wt_status_print_changed(s);
+	wt_status_print_submodule_summary(s);
 	wt_status_print_untracked(s);
 
 	if (s->verbose && !s->is_initial)
@@ -330,6 +365,10 @@ void wt_status_print(struct wt_status *s)
 
 int git_status_config(const char *k, const char *v)
 {
+	if (!strcmp(k, "status.submodulesummary")) {
+		wt_status_submodule_summary = atoi(v);
+		return 0;
+	}
 	if (!strcmp(k, "status.color") || !strcmp(k, "color.status")) {
 		wt_status_use_color = git_config_colorbool(k, v, -1);
 		return 0;
-- 
1.5.5.23.g2a5f

[PATCH/RFC 2/7] git-submodule: Extract absolute_url & move absolute url logic to module_clone

[Ping Yin]

Extract function absolute_url to remove code redundance and inconsistence in
cmd_init and cmd_add when resolving relative url/path to absolute one.

Also move resolving absolute url logic from cmd_add to module_clone which
results in a litte behaviour change: cmd_update originally doesn't
resolve absolute url but now it will.

This behaviour change breaks t7400 which uses relative url './.subrepo'.
However, this test originally doesn't mean to test relative url, so fix
the url as '.subrepo'.

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 git-submodule.sh           |   41 ++++++++++++++++++-----------------------
 t/t7400-submodule-basic.sh |    2 +-
 2 files changed, 19 insertions(+), 24 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index a5002e1..4d56135 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -65,6 +65,21 @@ resolve_relative_url ()
 	echo "$remoteurl/$url"
 }
 
+# Resolve relative url/path to absolute one
+absolute_url () {
+	case "$1" in
+	./*|../*)
+		# dereference source url relative to parent's url
+		url="$(resolve_relative_url $1)" ;;
+	*)
+		# Turn the source into an absolute path if it is local
+		url=$(get_repo_base "$1") ||
+		url=$1
+		;;
+	esac
+	echo "$url"
+}
+
 #
 # Map submodule path to submodule name
 #
@@ -112,7 +127,7 @@ module_info() {
 module_clone()
 {
 	path=$1
-	url=$2
+	url=$(absolute_url "$2")
 
 	# If there already is a directory at the submodule path,
 	# expect it to be empty (since that is the default checkout
@@ -195,21 +210,7 @@ cmd_add()
 			die "'$path' already exists and is not a valid git repo"
 		fi
 	else
-		case "$repo" in
-		./*|../*)
-			# dereference source url relative to parent's url
-			realrepo="$(resolve_relative_url $repo)" ;;
-		*)
-			# Turn the source into an absolute path if
-			# it is local
-			if base=$(get_repo_base "$repo"); then
-				repo="$base"
-			fi
-			realrepo=$repo
-			;;
-		esac
-
-		module_clone "$path" "$realrepo" || exit
+		module_clone "$path" "$repo" || exit
 		(unset GIT_DIR; cd "$path" && git checkout -q ${branch:+-b "$branch" "origin/$branch"}) ||
 		die "Unable to checkout submodule '$path'"
 	fi
@@ -262,13 +263,7 @@ cmd_init()
 		test -z "$url" &&
 		die "No url found for submodule path '$path' in .gitmodules"
 
-		# Possibly a url relative to parent
-		case "$url" in
-		./*|../*)
-			url="$(resolve_relative_url "$url")"
-			;;
-		esac
-
+		url=$(absolute_url "$url")
 		git config submodule."$name".url "$url" ||
 		die "Failed to register url for submodule path '$path'"
 
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 2ef85a8..e5d59b8 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -75,7 +75,7 @@ test_expect_success 'init should register submodule url in .git/config' '
 	then
 		echo "[OOPS] init succeeded but submodule url is wrong"
 		false
-	elif ! git config submodule.example.url ./.subrepo
+	elif ! git config submodule.example.url .subrepo
 	then
 		echo "[OOPS] init succeeded but update of url failed"
 		false
-- 
1.5.5.23.g2a5f

[PATCH v2 3/3] buitin-status: Add tests for submodule summary

[Ping Yin]

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 t/t7502-status.sh |  134 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 134 insertions(+), 0 deletions(-)

diff --git a/t/t7502-status.sh b/t/t7502-status.sh
index cd08516..33882c9 100755
--- a/t/t7502-status.sh
+++ b/t/t7502-status.sh
@@ -149,4 +149,138 @@ test_expect_success 'status of partial commit excluding new file in index' '
 	test_cmp expect output
 '
 
+test_expect_success "setup status submodule summary" '
+	test_create_repo sm &&
+	cd sm &&
+	: >foo &&
+	git add foo &&
+	git commit -m "Add foo" &&
+	cd .. &&
+	git add sm
+'
+
+cat > expect <<EOF
+# On branch master
+# Changes to be committed:
+#   (use "git reset HEAD <file>..." to unstage)
+#
+#	new file:   dir2/added
+#	new file:   sm
+#
+# Changed but not updated:
+#   (use "git add <file>..." to update what will be committed)
+#
+#	modified:   dir1/modified
+#
+# Untracked files:
+#   (use "git add <file>..." to include in what will be committed)
+#
+#	dir1/untracked
+#	dir2/modified
+#	dir2/untracked
+#	expect
+#	output
+#	untracked
+EOF
+test_expect_success "status submodule summary is disabled by default" '
+	git status > output &&
+	git diff expect output
+'
+
+head=$(cd sm && git rev-parse --short=7 --verify HEAD)
+
+cat > expect <<EOF
+# On branch master
+# Changes to be committed:
+#   (use "git reset HEAD <file>..." to unstage)
+#
+#	new file:   dir2/added
+#	new file:   sm
+#
+# Changed but not updated:
+#   (use "git add <file>..." to update what will be committed)
+#
+#	modified:   dir1/modified
+#
+# Modified submodules:
+#
+# * sm 0000000...$head (1):
+#   > Add foo
+#
+# Untracked files:
+#   (use "git add <file>..." to include in what will be committed)
+#
+#	dir1/untracked
+#	dir2/modified
+#	dir2/untracked
+#	expect
+#	output
+#	untracked
+EOF
+test_expect_success "status submodule summary" '
+	git config status.submodulesummary 10 &&
+	git status > output &&
+	git diff expect output
+'
+
+
+cat > expect <<EOF
+# On branch master
+# Changed but not updated:
+#   (use "git add <file>..." to update what will be committed)
+#
+#	modified:   dir1/modified
+#
+# Untracked files:
+#   (use "git add <file>..." to include in what will be committed)
+#
+#	dir1/untracked
+#	dir2/modified
+#	dir2/untracked
+#	expect
+#	output
+#	untracked
+no changes added to commit (use "git add" and/or "git commit -a")
+EOF
+test_expect_success "status submodule summary (clean submodule)" '
+	git commit -m "commit submodule" &&
+	git config status.submodulesummary 10 &&
+	! git status > output &&
+	git diff expect output
+'
+
+cat > expect <<EOF
+# On branch master
+# Changes to be committed:
+#   (use "git reset HEAD^1 <file>..." to unstage)
+#
+#	new file:   dir2/added
+#	new file:   sm
+#
+# Changed but not updated:
+#   (use "git add <file>..." to update what will be committed)
+#
+#	modified:   dir1/modified
+#
+# Modified submodules:
+#
+# * sm 0000000...$head (1):
+#   > Add foo
+#
+# Untracked files:
+#   (use "git add <file>..." to include in what will be committed)
+#
+#	dir1/untracked
+#	dir2/modified
+#	dir2/untracked
+#	expect
+#	output
+#	untracked
+EOF
+test_expect_success "status submodule summary (--amend)" '
+	git config status.submodulesummary 10 &&
+	git status --amend > output &&
+	git diff expect output
+'
+
 test_done
-- 
1.5.5.23.g2a5f

[PATCH/RFC 3/7] git-submodule: Fall back on .gitmodules if info not found in $GIT_DIR/config

[Ping Yin]

Originally, the submodule workflow enforces 'git init' in the beginning
which copies submodule config info from .gitmodules to $GIT_DIR/config.
Then all subcommands except 'init' and 'add' fetch submodule info from
$GIT_DIR/config and .gitmodules can be discarded.

However, there may be inconsistence between .git/config and .gitmodules
when always using 'git init' at first. If upstream .gitmodules changes,
it is not easy to sync the changes to $GIT_DIR/config.

Running 'git init' again may not help much in this case.  Since .git/config
has a whole copy of .gitmodules, the user has no easy way to know which
entries should follow the upstream changes and which entires shouldn't.

Actually, .gitmodules which formly only acted as info hints can and should
play a more important and essential role.

As an analogy to .gitignore and .git/info/excludes which are for colleagues'
and individual wishes separately, .gitmodules is for common requirements and
$GIT_DIR/config is for special requirements.

This patch implements a fall back strategy to satisfy both common and
special requirements as follows.

$GIT_DIR/config only keeps submodule info different from .gitmodules.
And the info from $GIT_DIR/config take higher precedence. The code first
consults $GIT_DIR/config and then fall back on in-tree .gitmodules file.

With this patch, init subcommand becomes not forcefull and less meaningful.
And now it is just a tool to help users copy info to $GIT_DIR/config
(and may modify it later) only when they need.

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 git-submodule.sh           |    9 ++++-----
 t/t7400-submodule-basic.sh |   29 +++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 4d56135..d88e3c3 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -98,7 +98,8 @@ module_name()
 }
 
 module_url() {
-	git config submodule.$1.url
+	git config submodule.$1.url ||
+	GIT_CONFIG=.gitmodules git config submodule.$1.url
 }
 
 module_info() {
@@ -256,12 +257,10 @@ cmd_init()
 	while read sha1 path name url
 	do
 		test -n "$name" || exit
-		# Skip already registered paths
-		test -z "$url" || continue
-
-		url=$(GIT_CONFIG=.gitmodules git config submodule."$name".url)
 		test -z "$url" &&
 		die "No url found for submodule path '$path' in .gitmodules"
+		# Skip already registered paths
+		git config submodule.$name.url && continue
 
 		url=$(absolute_url "$url")
 		git config submodule."$name".url "$url" ||
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index e5d59b8..d9b48f7 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -174,6 +174,35 @@ test_expect_success 'status should be "up-to-date" after update' '
 	git-submodule status | grep "^ $rev1"
 '
 
+test_expect_success 'status should be "modified" after submodule reset --hard HEAD@{1}' '
+	cd init &&
+	git reset --hard HEAD@{1}
+	rev2=$(git rev-parse HEAD) &&
+	cd .. &&
+	if test -z "$rev2"
+	then
+		echo "[OOPS] submodule git rev-parse returned nothing"
+		false
+	fi &&
+	git-submodule status | grep "^+$rev2"
+'
+
+test_expect_success 'update should checkout rev1 when fall back' '
+	git-config --unset submodule.example.url &&
+	GIT_CONFIG=.gitmodules git config submodule.example.url .subrepo &&
+	git-submodule update init &&
+	head=$(cd init && git rev-parse HEAD) &&
+	if test -z "$head"
+	then
+		echo "[OOPS] submodule git rev-parse returned nothing"
+		false
+	elif test "$head" != "$rev1"
+	then
+		echo "[OOPS] init did not checkout correct head"
+		false
+	fi
+'
+
 test_expect_success 'checkout superproject with subproject already present' '
 	git-checkout initial &&
 	git-checkout master
-- 
1.5.5.23.g2a5f

[PATCH/RFC 4/7] git-submodule: Extract module_add from cmd_add

[Ping Yin]

cmd_add will later handle the case adding multiple modules, so extract
module_add to add a single module.

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 git-submodule.sh |   67 +++++++++++++++++++++++++++++++----------------------
 1 files changed, 39 insertions(+), 28 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index d88e3c3..996bf2c 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -155,34 +155,7 @@ module_clone()
 #
 # optional branch is stored in global branch variable
 #
-cmd_add()
-{
-	# parse $args after "submodule ... add".
-	while test $# -ne 0
-	do
-		case "$1" in
-		-b | --branch)
-			case "$2" in '') usage ;; esac
-			branch=$2
-			shift
-			;;
-		-q|--quiet)
-			quiet=1
-			;;
-		--)
-			shift
-			break
-			;;
-		-*)
-			usage
-			;;
-		*)
-			break
-			;;
-		esac
-		shift
-	done
-
+module_add() {
 	repo=$1
 	path=$2
 
@@ -226,6 +199,44 @@ cmd_add()
 }
 
 #
+# Add a new submodule to the working tree, .gitmodules and the index
+#
+# $@ = repo [path]
+#
+# optional branch is stored in global branch variable
+#
+cmd_add()
+{
+	# parse $args after "submodule ... add".
+	while test $# -ne 0
+	do
+		case "$1" in
+		-b | --branch)
+			case "$2" in '') usage ;; esac
+			branch=$2
+			shift
+			;;
+		-q|--quiet)
+			quiet=1
+			;;
+		--)
+			shift
+			break
+			;;
+		-*)
+			usage
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done
+
+	module_add "$1" "$2"
+}
+
+#
 # Register submodules in .git/config
 #
 # $@ = requested paths (default to all)
-- 
1.5.5.23.g2a5f

[PATCH/RFC 5/7] git-submodule: multi-level module definition

[Ping Yin]

This patch introduces multi-level module definition and '--module-name'
option to designate submodules by logical names instead of path filters.
Then the init/update/status/add subcommand is enhanced combined with
this option.

The multi-level module definition in .gitmodules was first suggested by
Linus and etc. in mails "Let .git/config specify the url for submodules"
(http://article.gmane.org/gmane.comp.version-control.git/48939).

Following shows an example of such a .gitmodules which finally comes
from the group notation of 'git remote' which is suggested by Johannes
Schindelin.

.gitmodules with multiple level of indirection
------------------------------------------------------
[submodules]
	service = crawler search
	crawler = util imcrawter
	search = util imsearch
[submodule "util"]
	url = git://xyzzy/util.git
[submodule "imsearch"]
	path = search/imsearch
	url = git://xyzzy/imsearch.git
[submodule "imcrawler"]
	path = crawler/imcrawter
	url = git://xyzzy/imcrawter.git
------------------------------------------------------

By adding the 'submodules' section, we can define multi-level modules
in an infinite levels of indirection.

The "-m|--module-name" option is introduced with which submodules are
designated by logical names instead of real paths as following shows.

Identical commands forms with/without "--module-name"
---------------------------------------------------
$ git submodule XXX util imcrawler              (1)
$ git submodule XXX -n crawler                  (2)
$ git submodule XXX util imcrawler imsearch     (3)
$ git submodule XXX -n service                  (4)
$ git submodule XXX -n crawler search           (5)
---------------------------------------------------
* XXX represents status, update or init, but not add
* (1) and (2) are identical conditionally (explained below)
* (3), (4) and (5) are identical conditionally

There are still minor difference between these two forms.

In the no "--module-name" form, the path parameter may be not the real
submodule path, and it just acts as the filter for real submodule paths.
While in the "--module-name" form, the name parameter must be the logical
name, and the real paths corresponding to the logical name may be neither
a submodule path nor even existent.

This patch handles such a path for different subcommands as follows.

 - status: Output 0{40} as the sha1. Doing this can remind the user to
   add the path as submodule or delete the path from .gitmodules.
 - update: Skip that path and issue a "Not a submodule" warning
 - init: Also init for that path

So in the example above, commands (1) and (2) are identical only when
util and imcrawler are already submodules.

The add subcommand is also enhanced.

The former workflow to add submodules is adding one by one with
"git submodule add url path" which then modifies .gitmodules. However,
sometimes it is more convenient to work in the reverse way: edit
.gitmodules first and then add submodules in batch.

Now "git submodule add --module-name modulename" can help us to do that.
It will find all submodules corresponding to the logical name and add them
in batch by using the paths and urls from .gitmodules. Of course, it will
skip the paths which have already been submodules.

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 git-submodule.sh |   86 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 81 insertions(+), 5 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 996bf2c..f1c164c 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -4,7 +4,7 @@
 #
 # Copyright (c) 2007 Lars Hjemli
 
-USAGE="[--quiet] [--cached] \
+USAGE="[--quiet] [--cached] [--module-name] \
 [add <repo> [-b branch]|status|init|update|summary [-n|--summary-limit <n>] [<commit>]] \
 [--] [<path>...]"
 OPTIONS_SPEC=
@@ -15,6 +15,7 @@ command=
 branch=
 quiet=
 cached=
+use_module_name=
 
 #
 # print stuff on stdout unless -q was specified
@@ -97,12 +98,23 @@ module_name()
        echo "$name"
 }
 
+module_path() {
+	git config submodule.$1.path ||
+	GIT_CONFIG=.gitmodules git config submodule.$1.path ||
+	echo "$1"
+}
+
 module_url() {
 	git config submodule.$1.url ||
 	GIT_CONFIG=.gitmodules git config submodule.$1.url
 }
 
 module_info() {
+	if test -n "$use_module_name"
+	then
+		module_info_by_name "$@"
+		return
+	fi
 	git ls-files --stage -- "$@" | grep -e '^160000 ' |
 	while read mode sha1 stage path
 	do
@@ -118,6 +130,50 @@ module_info() {
 }
 
 #
+# List all submodule info line by line by given names as follows
+# sha1<tab>path<tab>name<tab>url
+#
+# Here we assume that module names and paths don't contain space
+# characters
+#
+# $@ = module names
+# When names is not given, list all module info
+#
+module_info_by_name() {
+	if test $# = 0
+	then
+		names=$(
+		{
+			git config --get-regexp 'submodule.*.url'
+			git config -f .gitmodules --get-regexp 'submodule.*.url'
+		} | sed 's/submodule.\(.*\).url .*$/\1/' 2>/dev/null
+		)
+	else
+		names=$(module_children_names "$@")
+	fi
+	for name in $names
+	do
+		url=$(module_url "$name") || continue
+		path=$(module_path "$name")
+		sha1=$(git ls-files --stage "$path" |
+			grep "$path$" | grep '^160000' | awk '{print $2}')
+		test -z "$sha1" && sha1=0000000000000000000000000000000000000000
+		echo "$sha1	$path	$name	$url"
+	done
+}
+
+module_children_names() {
+	for name
+	do
+		echo "$name"
+		module_children_names $(
+			git config "submodules.$name"
+			git config -f .gitmodules "submodules.$name"
+		)
+	done | sort -u
+}
+
+#
 # Clone a submodule
 #
 # Prior to calling, cmd_update checks that a possibly existing
@@ -233,7 +289,17 @@ cmd_add()
 		shift
 	done
 
-	module_add "$1" "$2"
+	if test -n "$use_module_name"
+	then
+		module_info "$@" |
+		while read sha1 path name url
+		do
+			module_add "$url" "$path"
+		done
+	else
+		module_add "$1" "$2"
+	fi
+
 }
 
 #
@@ -313,7 +379,11 @@ cmd_update()
 	while read sha1 path name url
 	do
 		test -n "$name" || exit
-		if test -z "$url"
+		if test $sha1 = 0000000000000000000000000000000000000000
+		then
+			say "Not a submodule: $name @ $path"
+			continue
+		elif test -z "$url"
 		then
 			# Only mention uninitialized submodules when its
 			# path have been specified
@@ -558,12 +628,15 @@ cmd_status()
 		esac
 		shift
 	done
-
 	module_info "$@" |
 	while read sha1 path name url
 	do
 		test -n "$name" || exit
-		if test -z "$url" || ! test -d "$path"/.git
+		if test $sha1 = 0000000000000000000000000000000000000000
+		then
+			say "*$sha1 $path"
+			continue
+		elif test -z "$url" || ! test -d "$path"/.git
 		then
 			say "-$sha1 $path"
 			continue;
@@ -609,6 +682,9 @@ do
 	--cached)
 		cached="$1"
 		;;
+	-m|--module-name)
+		use_module_name=1
+		;;
 	--)
 		break
 		;;
-- 
1.5.5.23.g2a5f

[PATCH/RFC 6/7] git-submodule: Don't die when command fails for one submodule

[Ping Yin]

When handling multiple modules, init/update/status/add subcommand will
exit when it fails for one submodule. This patch makes the subcommand
continue bypassing the failure and keep right exit status.

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 git-submodule.sh |   87 +++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 63 insertions(+), 24 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index f1c164c..b4db676 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -193,15 +193,19 @@ module_clone()
 	# succeed but the rmdir will fail. We might want to fix this.
 	if test -d "$path"
 	then
-		rmdir "$path" 2>/dev/null ||
-		die "Directory '$path' exist, but is neither empty nor a git repository"
+		! rmdir "$path" 2>/dev/null &&
+		say "Directory '$path' exist, but is neither empty nor a git repository" &&
+		return 1
 	fi
 
 	test -e "$path" &&
-	die "A file already exist at path '$path'"
+	say "A file already exist at path '$path'" &&
+	return 1
 
-	git-clone -n "$url" "$path" ||
-	die "Clone of '$url' into submodule path '$path' failed"
+	! git-clone -n "$url" "$path" &&
+	say "Clone of '$url' into submodule path '$path' failed" &&
+	return 1
+	:
 }
 
 #
@@ -227,7 +231,8 @@ module_add() {
 	fi
 
 	git ls-files --error-unmatch "$path" > /dev/null 2>&1 &&
-	die "'$path' already exists in the index"
+	say "'$path' already exists in the index" &&
+	return 1
 
 	# perhaps the path exists and is already a git repo, else clone it
 	if test -e "$path"
@@ -237,21 +242,26 @@ module_add() {
 		then
 			echo "Adding existing repo at '$path' to the index"
 		else
-			die "'$path' already exists and is not a valid git repo"
+			say "'$path' already exists and is not a valid git repo"
+			return 1
 		fi
 	else
-		module_clone "$path" "$repo" || exit
-		(unset GIT_DIR; cd "$path" && git checkout -q ${branch:+-b "$branch" "origin/$branch"}) ||
-		die "Unable to checkout submodule '$path'"
+		module_clone "$path" "$repo" || return 1
+		! (unset GIT_DIR; cd "$path" && git checkout -q ${branch:+-b "$branch" "origin/$branch"}) &&
+		say "Unable to checkout submodule '$path'" &&
+		return 1
 	fi
 
-	git add "$path" ||
-	die "Failed to add submodule '$path'"
+	! git add "$path" &&
+	say "Failed to add submodule '$path'" &&
+	return 1
 
 	GIT_CONFIG=.gitmodules git config submodule."$path".path "$path" &&
 	GIT_CONFIG=.gitmodules git config submodule."$path".url "$repo" &&
-	git add .gitmodules ||
-	die "Failed to register submodule '$path'"
+	! git add .gitmodules &&
+	say "Failed to register submodule '$path'" &&
+	return 1
+	:
 }
 
 #
@@ -292,14 +302,17 @@ cmd_add()
 	if test -n "$use_module_name"
 	then
 		module_info "$@" |
+		{
+		exit_status=0
 		while read sha1 path name url
 		do
-			module_add "$url" "$path"
+			module_add "$url" "$path" || exit_status=1
 		done
+		test $exit_status = 0
+		}
 	else
 		module_add "$1" "$2"
 	fi
-
 }
 
 #
@@ -331,20 +344,30 @@ cmd_init()
 	done
 
 	module_info "$@" |
+	{
+	exit_status=0
 	while read sha1 path name url
 	do
-		test -n "$name" || exit
+		test -z "$name" && exit_status=1 && continue
 		test -z "$url" &&
-		die "No url found for submodule path '$path' in .gitmodules"
+		say "No url found for submodule path '$path' in .gitmodules" &&
+		exit_status=1 &&
+		continue
 		# Skip already registered paths
 		git config submodule.$name.url && continue
 
 		url=$(absolute_url "$url")
 		git config submodule."$name".url "$url" ||
-		die "Failed to register url for submodule path '$path'"
+		{
+		say "Failed to register url for submodule path '$path'"
+		exit_status=1
+		continue
+		}
 
 		say "Submodule '$name' ($url) registered for path '$path'"
 	done
+	exit $exit_status
+	}
 }
 
 #
@@ -376,9 +399,11 @@ cmd_update()
 	done
 
 	module_info "$@" |
+	{
+	exit_status=0
 	while read sha1 path name url
 	do
-		test -n "$name" || exit
+		test -z "$name" && exit_status=1 && continue
 		if test $sha1 = 0000000000000000000000000000000000000000
 		then
 			say "Not a submodule: $name @ $path"
@@ -394,23 +419,33 @@ cmd_update()
 
 		if ! test -d "$path"/.git
 		then
-			module_clone "$path" "$url" || exit
+			! module_clone "$path" "$url" && exit_status=1 && continue
 			subsha1=
 		else
 			subsha1=$(unset GIT_DIR; cd "$path" &&
 				git rev-parse --verify HEAD) ||
-			die "Unable to find current revision in submodule path '$path'"
+			{
+				say "Unable to find current revision in submodule path '$path'"
+				exit_status=1
+				continue
+			}
 		fi
 
 		if test "$subsha1" != "$sha1"
 		then
 			(unset GIT_DIR; cd "$path" && git-fetch &&
 				git-checkout -q "$sha1") ||
-			die "Unable to checkout '$sha1' in submodule path '$path'"
+			{
+				say "Unable to checkout '$sha1' in submodule path '$path'"
+				exit_status=1
+				continue
+			}
 
 			say "Submodule path '$path': checked out '$sha1'"
 		fi
 	done
+	exit $exit_status
+	}
 }
 
 set_name_rev () {
@@ -629,9 +664,11 @@ cmd_status()
 		shift
 	done
 	module_info "$@" |
+	{
+	exit_status=0
 	while read sha1 path name url
 	do
-		test -n "$name" || exit
+		test -z "$name" && exit_status=1 && continue
 		if test $sha1 = 0000000000000000000000000000000000000000
 		then
 			say "*$sha1 $path"
@@ -654,6 +691,8 @@ cmd_status()
 			say "+$sha1 $path$revname"
 		fi
 	done
+	exit $exit_status
+	}
 }
 
 # This loop parses the command line arguments to find the
-- 
1.5.5.23.g2a5f

[PATCH/RFC 7/7] git-submodule: "update --force" to enforce cloning non-submodule

[Ping Yin]

If the update subcommand combines with --force, non-submodules
(i.e. modules existing in .gitmodules or $GIT_DIR/config but not added
to the super module) will be cloned and the master branch will be
checked out.

However, if a non-submodule has already been cloned before, the update
will be rejected since we don't know what the update means.

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 git-submodule.sh |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index b4db676..153b61e 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -384,6 +384,9 @@ cmd_update()
 		-q|--quiet)
 			quiet=1
 			;;
+		-f | --force)
+			force="$1"
+			;;
 		--)
 			shift
 			break
@@ -406,7 +409,8 @@ cmd_update()
 		test -z "$name" && exit_status=1 && continue
 		if test $sha1 = 0000000000000000000000000000000000000000
 		then
-			say "Not a submodule: $name @ $path"
+			test -z "$force" &&
+			say "Not a submodule: $name @ $path" &&
 			continue
 		elif test -z "$url"
 		then
@@ -420,8 +424,15 @@ cmd_update()
 		if ! test -d "$path"/.git
 		then
 			! module_clone "$path" "$url" && exit_status=1 && continue
+			test "$sha1" = 0000000000000000000000000000000000000000 &&
+			(unset GIT_DIR; cd "$path" && git checkout -q master) &&
+			say "non-submodule cloned and master checked out: $name @ $path" &&
+			continue
 			subsha1=
 		else
+			test "$sha1" = 0000000000000000000000000000000000000000 &&
+			say "non-submodule already cloned: $name @ $path" &&
+			continue
 			subsha1=$(unset GIT_DIR; cd "$path" &&
 				git rev-parse --verify HEAD) ||
 			{
@@ -431,6 +442,7 @@ cmd_update()
 			}
 		fi
 
+
 		if test "$subsha1" != "$sha1"
 		then
 			(unset GIT_DIR; cd "$path" && git-fetch &&
-- 
1.5.5.23.g2a5f

Re: [PATCH v2 1/3] git-submodule summary: --for-status option

[Ping Yin]

On Thu, Apr 10, 2008 at 11:50 PM, Ping Yin <pkufranky@gmail.com> wrote:
>
> The --for-status option is mainly used by builtin-status/commit.
>  It adds 'Modified submodules:' line at top and  '# ' prefix to all
>  following lines.
>

Sorry for the repeated mail. I don't know how it happened. Maybe
send-email bug or my misoperation.

So just Ignore the second one.




-- 
Ping Yin

Re: [PATCH/RFC 2/7] git-submodule: Extract absolute_url & move absolute url logic to module_clone

[Junio C Hamano]

Ping Yin <pkufranky@gmail.com> writes:

> This behaviour change breaks t7400 which uses relative url './.subrepo'.
> However, this test originally doesn't mean to test relative url, so fix
> the url as '.subrepo'.

Hmmm.  Doesn't "foo" generally mean the same thing as "./foo" in the sense
both are relative to the current directory?

Re: [PATCH/RFC 1/7] git-submodule: Extract functions module_info and module_url

[Junio C Hamano]

Ping Yin <pkufranky@gmail.com> writes:

> @@ -232,12 +251,11 @@ cmd_init()
>  		shift
>  	done
>  
> -	git ls-files --stage -- "$@" | grep '^160000 ' |
> -	while read mode sha1 stage path
> +	module_info "$@" |
> +	while read sha1 path name url
>  	do
> +		test -n "$name" || exit
>  		# Skip already registered paths
> -		name=$(module_name "$path") || exit
> -		url=$(git config submodule."$name".url)
>  		test -z "$url" || continue

This is not a new bug in this round of patch (i.e. the original code
already did the same), but I have to wonder if exiting the loop silently
when $name is not set (i.e. .gitmodules does not have an entry to describe
the submodule yet) is a good idea.

If an entry with mode 160000 in the index is an error if it does not have
a corresponding entry in .gitmodules, then this code should say so when
exiting the loop prematurely.  If it is not, I think it should silently
continue just like missing URL case.

The user may be right in the process of manually adding a new submodule,
has done "git add" of the submodule path already but hasn't yet decided
what the name of the submodule or where the final published URL would be.
In such a case, you would have 160000 entry in the index that does not
have a corresponding entry in .gitmodules and that is perfectly valid.

So I tend to think that you should treat "missing name" and "missing url"
as an non-error case.

cmd_init would obviously need to _notice_ "missing url" and refrain from
adding the missing remote URL to the config, but I do not think it should
error out.  Warning might be appropriate in cases, but I dunno.

Same comment applies to cmd_update() and cmd_status().  I would strongly
suspect that status may want to ignore missing name/url and show the usual
diff, as it does not even have to require the submodule to interact with
any remote repository at all.  The user may be privately using the
submodule and does not even need to push it out nor pull it from
elsewhere, and in such a case, .gitmodules may not even be populated with
an entry for that submodule, ever, not just as a "right in the middle of
adding" status.

When the command should be usable without .gitmodules entry, it should not
require one.

Re: [PATCH v2 2/3] builtin-status: submodule summary support

[Junio C Hamano]

Ping Yin <pkufranky@gmail.com> writes:

> This commit teaches 'git commit/status' show 'Modified submodules'
> section given by
>
> git submodule summary --cached --for-status --summary-limit <limit>
>
> just before 'Untracked files' section.
>
> The <limit> is given by the config variable status.submodulesummary
> to limit the submodule summary size. status.submodulesummary is 0 by
> default which disables the summary.

This begs an obvious question "What if I want to have an unlimited summary
in the status output?"

> diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
> index 3ea269a..32b6660 100644
> --- a/Documentation/git-status.txt
> +++ b/Documentation/git-status.txt
> @@ -52,6 +52,10 @@ If the config variable `status.relativePaths` is set to false, then all
>  paths shown are relative to the repository root, not to the current
>  directory.
>  
> +If 'status.submodulesummary' is set to a non zero number, the submodule
> +summary will be enabled and a summary of commits for modified submodules
> +will be shown (see --summary-limit option of linkgit:git-submodule[1]).
> +

The docs quote configuration variables in typewriter face by using
backticks (pay attention to how status.relativePaths is quoted in your
hunk header comment).

> diff --git a/wt-status.c b/wt-status.c
> index b3fd57b..369c519 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -8,9 +8,11 @@
>  #include "revision.h"
>  #include "diffcore.h"
>  #include "quote.h"
> +#include "run-command.h"
>  
>  int wt_status_relative_paths = 1;
>  int wt_status_use_color = -1;
> +int wt_status_submodule_summary = 0;

Don't initialize to 0.  BSS takes care of it.

>  static char wt_status_colors[][COLOR_MAXLEN] = {
>  	"",         /* WT_STATUS_HEADER: normal */
>  	"\033[32m", /* WT_STATUS_UPDATED: green */
> @@ -219,6 +221,38 @@ static void wt_status_print_changed(struct wt_status *s)
>  	rev.diffopt.format_callback_data = s;
>  	run_diff_files(&rev, 0);
>  }
> +static void wt_status_print_submodule_summary(struct wt_status *s)
> +{
> ...
> +	memset(&sm_summary, 0, sizeof(sm_summary));
> +	sm_summary.argv = argv;
> +	sm_summary.env = env;
> +	sm_summary.git_cmd = 1;
> +	sm_summary.no_stdin = 1;
> +	fflush(s->fp);
> +	sm_summary.out = dup(fileno(s->fp));    /* run_command closes it */
> +	run_command(&sm_summary);

I recall we had some clean-up on how file descriptors are inherited and
duped around when run_command() runs a subprocess.  Hannes, is this dup()
consistent with how the things ought to be these days?

> @@ -330,6 +365,10 @@ void wt_status_print(struct wt_status *s)
>  
>  int git_status_config(const char *k, const char *v)
>  {
> +	if (!strcmp(k, "status.submodulesummary")) {
> +		wt_status_submodule_summary = atoi(v);

With this code, you would feed a NULL to atoi() with:

        [status]
                submoduleSummary

How about making this a bool-or-number, and boolean true gives whatever
the default limit (may be unlimited) when no --summary-limit is given?

We may want to have git_config_bool_or_int() in config.c so that the
caller can tell if the value is boolean true or integer 1, which would be
a reasonable improvement independent from this submodule summary
codepath.

int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
{
        *is_bool = 1;
	if (!value)
		return 1;
	if (!*value)
		return 0;
	if (!strcasecmp(value, "true") || !strcasecmp(value, "yes"))
		return 1;
	if (!strcasecmp(value, "false") || !strcasecmp(value, "no"))
		return 0;
	*is_bool = 0;
	return git_config_int(name, value) != 0;
}

With that, your configuration parser can do something like:

	if (!strcmp(k, "status.submodulesummary")) {
		int is_bool, val;
		val = git_config_bool_or_int(k, v, &b);
		if (is_bool || val <= 0) {
                	wt_status_submodule_summary_enabled = val;
		} else {
                	wt_status_submodule_summary = val;
                	wt_status_submodule_summary_enabled = 1;
		}
	}

and skip the call to wt_status_print_submodule_summary() when not
enabled.

>  int wt_status_relative_paths = 1;
>  int wt_status_use_color = -1;
> +int wt_status_submodule_summary = -1; /* unspecified */
  +int wt_status_submodule_summary_enabled;

The call site in wt_status_print() would look like: 

	...
 	wt_status_print_changed(s);
	if (wt_status_submodule_summary_enabled > 0)
		wt_status_print_submodule_summary(s);
 	wt_status_print_untracked(s);
	...

and the wt_status_print_submodule_summary() would not add --summary-limit
parameter if wt_status_submodule_summary is left unspecified (i.e. -1).

Re: [PATCH/RFC 3/7] git-submodule: Fall back on .gitmodules if info not found in $GIT_DIR/config

[Junio C Hamano]

Ping Yin <pkufranky@gmail.com> writes:

> Originally, the submodule workflow enforces 'git init' in the beginning
> which copies submodule config info from .gitmodules to $GIT_DIR/config.
> Then all subcommands except 'init' and 'add' fetch submodule info from
> $GIT_DIR/config and .gitmodules can be discarded.
>
> However, there may be inconsistence between .git/config and .gitmodules
> when always using 'git init' at first. If upstream .gitmodules changes,
> it is not easy to sync the changes to $GIT_DIR/config.

Maybe you missed an earlier thread with Roman Shaposhnik where this issue
was discussed and a solution more in line with the original intent of the
design of the submodule system was mentioned (actually I should not take
credit for that suggestion as it was not mine but somebody else mentioned
it back when git-submodule command was initially being designed.  I only
recalled there was that one issue in the discussion)?

Re: [PATCH/RFC 3/7] git-submodule: Fall back on .gitmodules if info not found in $GIT_DIR/config

[Junio C Hamano]

Ping Yin <pkufranky@gmail.com> writes:

> Originally, the submodule workflow enforces 'git init' in the beginning
> which copies submodule config info from .gitmodules to $GIT_DIR/config.
> Then all subcommands except 'init' and 'add' fetch submodule info from
> $GIT_DIR/config and .gitmodules can be discarded.
>
> However, there may be inconsistence between .git/config and .gitmodules
> when always using 'git init' at first. If upstream .gitmodules changes,
> it is not easy to sync the changes to $GIT_DIR/config.

Maybe you missed an earlier thread with Roman Shaposhnik where this issue
was discussed and a solution more in line with the original intent of the
design of the submodule system was mentioned (actually I should not take
credit for that suggestion as it was not mine but somebody else mentioned
it back when git-submodule command was initially being designed.  I only
recalled there was that one issue in the old discussion but there might
have been others)?

Re: [PATCH/RFC 2/7] git-submodule: Extract absolute_url & move absolute url logic to module_clone

[Ping Yin]

On Sat, Apr 12, 2008 at 5:56 AM, Junio C Hamano <junio@pobox.com> wrote:
> Ping Yin <pkufranky@gmail.com> writes:
>
>  > This behaviour change breaks t7400 which uses relative url './.subrepo'.
>  > However, this test originally doesn't mean to test relative url, so fix
>  > the url as '.subrepo'.
>
>  Hmmm.  Doesn't "foo" generally mean the same thing as "./foo" in the sense
>  both are relative to the current directory?

There is a little inconsistence in current logic

1. git submodule add ./foo will expand foo with remote.origin.url and
init an entry in .gitmodules as
"submodule.foo.url=$remoteoriginurl/foo"
2. git submodule update will not expand ./foo if  there is an entry
"submodule.foo.url=./foo"  in $GIT_DIR/config

I tend to add the url as is when "git submodule add", and then expand
the url when running "git submodule update". So this will result that
the second case expands './foo' as "$remoteoriginurl/foo" instead of
"foo"





-- 
Ping Yin

Re: [PATCH/RFC 1/7] git-submodule: Extract functions module_info and module_url

[Ping Yin]

On Sat, Apr 12, 2008 at 6:30 AM, Junio C Hamano <junio@pobox.com> wrote:
> Ping Yin <pkufranky@gmail.com> writes:
>
>  > @@ -232,12 +251,11 @@ cmd_init()
>  >               shift
>  >       done
>  >
>  > -     git ls-files --stage -- "$@" | grep '^160000 ' |
>  > -     while read mode sha1 stage path
>  > +     module_info "$@" |
>  > +     while read sha1 path name url
>  >       do
>  > +             test -n "$name" || exit
>  >               # Skip already registered paths
>  > -             name=$(module_name "$path") || exit
>  > -             url=$(git config submodule."$name".url)
>  >               test -z "$url" || continue
>
>  This is not a new bug in this round of patch (i.e. the original code
>  already did the same), but I have to wonder if exiting the loop silently
>  when $name is not set (i.e. .gitmodules does not have an entry to describe
>  the submodule yet) is a good idea.
>
>  If an entry with mode 160000 in the index is an error if it does not have
>  a corresponding entry in .gitmodules, then this code should say so when
>  exiting the loop prematurely.  If it is not, I think it should silently
>  continue just like missing URL case.
>
>  The user may be right in the process of manually adding a new submodule,
>  has done "git add" of the submodule path already but hasn't yet decided
>  what the name of the submodule or where the final published URL would be.
>  In such a case, you would have 160000 entry in the index that does not
>  have a corresponding entry in .gitmodules and that is perfectly valid.
>
>  So I tend to think that you should treat "missing name" and "missing url"
>  as an non-error case.
>
>  cmd_init would obviously need to _notice_ "missing url" and refrain from
>  adding the missing remote URL to the config, but I do not think it should
>  error out.  Warning might be appropriate in cases, but I dunno.
>
>  Same comment applies to cmd_update() and cmd_status().  I would strongly
>  suspect that status may want to ignore missing name/url and show the usual
>  diff, as it does not even have to require the submodule to interact with
>  any remote repository at all.  The user may be privately using the
>  submodule and does not even need to push it out nor pull it from
>  elsewhere, and in such a case, .gitmodules may not even be populated with
>  an entry for that submodule, ever, not just as a "right in the middle of
>  adding" status.

This patch just does refactor, i do this in my sixth patch "Don't die
when command fails for one submodule"


-- 
Ping Yin

Re: [PATCH/RFC 3/7] git-submodule: Fall back on .gitmodules if info not found in $GIT_DIR/config

[Ping Yin]

On Sat, Apr 12, 2008 at 7:24 AM, Junio C Hamano <junio@pobox.com> wrote:
> Ping Yin <pkufranky@gmail.com> writes:
>
>  > Originally, the submodule workflow enforces 'git init' in the beginning
>  > which copies submodule config info from .gitmodules to $GIT_DIR/config.
>  > Then all subcommands except 'init' and 'add' fetch submodule info from
>  > $GIT_DIR/config and .gitmodules can be discarded.
>  >
>  > However, there may be inconsistence between .git/config and .gitmodules
>  > when always using 'git init' at first. If upstream .gitmodules changes,
>  > it is not easy to sync the changes to $GIT_DIR/config.
>
>
> Maybe you missed an earlier thread with Roman Shaposhnik where this issue
>  was discussed and a solution more in line with the original intent of the
>  design of the submodule system was mentioned (actually I should not take
>  credit for that suggestion as it was not mine but somebody else mentioned
>  it back when git-submodule command was initially being designed.  I only
>  recalled there was that one issue in the old discussion but there might
>  have been others)?

You mean use "hooks" to update $GIT_DIR/config with user interaction
when .gitmodules changes? Or give user hints when "git submodule
update" fails?

What you said in that thread is that the url in $GIT_DIR/config is
different from the one in .gitmodules (with protocol change perhaps)
originally, and then the url in .gitmodules changes. So when "git
submodule update" fails, it notices this change and tell the user.

What i mean here is another case. The url in $GIT_DIR/config is the
same as the one in .gitmodules, and then the url in .gitmodules
change. So this change can be synced automatically to $GIT_DIR/config.

However, when both cases happen in the same time, there is no way to
differentiate these two cases. So the command  can't do something
automatically and has to leave all choice to the user.

In an environment with central repositories, all submodule urls will
be the same between $GIT_DIR/config and .gitmodules. It is a little
annoying to give so many users this kind of uneccessary choice if the
submodule url changes in .gitmodules.



-- 
Ping Yin

Re: [PATCH/RFC 1/7] git-submodule: Extract functions module_info and module_url

[Junio C Hamano]

"Ping Yin" <pkufranky@gmail.com> writes:

> On Sat, Apr 12, 2008 at 6:30 AM, Junio C Hamano <junio@pobox.com> wrote:
>> ...
>>  Same comment applies to cmd_update() and cmd_status().  I would strongly
>>  suspect that status may want to ignore missing name/url and show the usual
>>  diff, as it does not even have to require the submodule to interact with
>>  any remote repository at all.  The user may be privately using the
>>  submodule and does not even need to push it out nor pull it from
>>  elsewhere, and in such a case, .gitmodules may not even be populated with
>>  an entry for that submodule, ever, not just as a "right in the middle of
>>  adding" status.
>
> This patch just does refactor, i do this in my sixth patch "Don't die
> when command fails for one submodule"

Fair enough.

As usual, the preferred order is to first refector, fix without
enhancement as necessary, then finally build on more solidified base.

Re: [PATCH v2 2/3] builtin-status: submodule summary support

[Ping Yin]

On Sat, Apr 12, 2008 at 6:31 AM, Junio C Hamano <junio@pobox.com> wrote:

>  int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
>  {
>         *is_bool = 1;
>         if (!value)
>                 return 1;
>         if (!*value)
>                 return 0;
>         if (!strcasecmp(value, "true") || !strcasecmp(value, "yes"))
>                 return 1;
>         if (!strcasecmp(value, "false") || !strcasecmp(value, "no"))
>                 return 0;
>         *is_bool = 0;
>         return git_config_int(name, value) != 0;
>  }
>
>  With that, your configuration parser can do something like:
>
>
>         if (!strcmp(k, "status.submodulesummary")) {
>                 int is_bool, val;
>                 val = git_config_bool_or_int(k, v, &b);
>                 if (is_bool || val <= 0) {
>                         wt_status_submodule_summary_enabled = val;
>                 } else {
>                         wt_status_submodule_summary = val;
>                         wt_status_submodule_summary_enabled = 1;
>                 }
>         }
>
>  and skip the call to wt_status_print_submodule_summary() when not
>  enabled.
>
>
>  >  int wt_status_relative_paths = 1;
>  >  int wt_status_use_color = -1;
>  > +int wt_status_submodule_summary = -1; /* unspecified */
>   +int wt_status_submodule_summary_enabled;
>
>  The call site in wt_status_print() would look like:
>
>         ...
>
>         wt_status_print_changed(s);
>         if (wt_status_submodule_summary_enabled > 0)
>
>                 wt_status_print_submodule_summary(s);
>         wt_status_print_untracked(s);
>         ...
>

How about the following?

diff --git a/config.c b/config.c
index 0624494..e614456 100644
--- a/config.c
+++ b/config.c
@@ -316,6 +316,21 @@ int git_config_bool(const char *name, const char *value)
 	return git_config_int(name, value) != 0;
 }

+int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
+{
+	*is_bool = 1;
+	if (!value)
+		return 1;
+	if (!*value)
+		return 0;
+	if (!strcasecmp(value, "true") || !strcasecmp(value, "yes"))
+		return 1;
+	if (!strcasecmp(value, "false") || !strcasecmp(value, "no"))
+		return 0;
+	*is_bool = 0;
+	return git_config_int(name, value);
+}
+
 int git_config_string(const char **dest, const char *var, const char *value)
 {
 	if (!value)
diff --git a/wt-status.c b/wt-status.c
index 22385f5..3baa128 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -366,7 +366,10 @@ void wt_status_print(struct wt_status *s)
 int git_status_config(const char *k, const char *v)
 {
 	if (!strcmp(k, "status.submodulesummary")) {
-		wt_status_submodule_summary = atoi(v);
+		int is_bool;
+		wt_status_submodule_summary = git_config_bool_or_int(k, v, &is_bool);
+		if (is_bool && wt_status_submodule_summary)
+			wt_status_submodule_summary = -1;
 		return 0;
 	}
 	if (!strcmp(k, "status.color") || !strcmp(k, "color.status")) {



-- 
Ping Yin

Re: [PATCH v2 2/3] builtin-status: submodule summary support

[Ping Yin]

On Sat, Apr 12, 2008 at 6:31 AM, Junio C Hamano <junio@pobox.com> wrote:
>  > +static void wt_status_print_submodule_summary(struct wt_status *s)
>  > +{
>  > ...
>
> > +     memset(&sm_summary, 0, sizeof(sm_summary));
>  > +     sm_summary.argv = argv;
>  > +     sm_summary.env = env;
>  > +     sm_summary.git_cmd = 1;
>  > +     sm_summary.no_stdin = 1;
>  > +     fflush(s->fp);
>  > +     sm_summary.out = dup(fileno(s->fp));    /* run_command closes it */
>  > +     run_command(&sm_summary);
>
>  I recall we had some clean-up on how file descriptors are inherited and
>  duped around when run_command() runs a subprocess.  Hannes, is this dup()
>  consistent with how the things ought to be these days?

I think dup is needed, as commit c20181e3a3 says, run_command still
will close passed in positive descripors.

    start_command(), if .in/.out > 0, closes file descriptors, not the callers

    Callers of start_command() can set the members .in and .out of struct
    child_process to a value > 0 to specify that this descriptor is used as
    the stdin or stdout of the child process.

    Previously, if start_command() was successful, this descriptor was closed
    upon return. Here we now make sure that the descriptor is also closed in
    case of failures. All callers are updated not to close the file descriptor
    themselves after start_command() was called.


-- 
Ping Yin

Re: [PATCH v2 2/3] builtin-status: submodule summary support

[Johannes Sixt]

On Saturday 12 April 2008 00:31, Junio C Hamano wrote:
> Ping Yin <pkufranky@gmail.com> writes:
> > +static void wt_status_print_submodule_summary(struct wt_status *s)
> > +{
> > ...
> > +	memset(&sm_summary, 0, sizeof(sm_summary));
> > +	sm_summary.argv = argv;
> > +	sm_summary.env = env;
> > +	sm_summary.git_cmd = 1;
> > +	sm_summary.no_stdin = 1;
> > +	fflush(s->fp);
> > +	sm_summary.out = dup(fileno(s->fp));    /* run_command closes it */
> > +	run_command(&sm_summary);
>
> I recall we had some clean-up on how file descriptors are inherited and
> duped around when run_command() runs a subprocess.  Hannes, is this dup()
> consistent with how the things ought to be these days?

Yes, this dup() is required and correct.

-- Hannes