[RFC PATCH 0/2] submodule update continue

[RFC PATCH 0/2] submodule update continue

[Fredrik Gustafsson]

When running submodule update git dies after the first error. This is
inconvenient if a submodule is unavailable. Therefore this patch series
makes git continue with the next submodule if a submodule is
unavailable.

This does only apply to checkout errors, rebase and merge errors will
result with a direct exit with code 2.

A checkout error makes git continue to the next submodule and when done
with all submodules exit with code 1.

Still missing are three tests (recursive checkout, merge and recurive rebase).

This is the second iteration of this patch series. The first can be
found on:
http://thread.gmane.org/gmane.comp.version-control.git/171895

Fredrik Gustafsson (2):
  sh-setup: Make die take the error code as param.
  submodule update: continue when a checkout fails

 git-sh-setup.sh             |    6 +++++-
 git-submodule.sh            |   42 ++++++++++++++++++++++++++++++++++++------
 t/t7406-submodule-update.sh |   29 +++++++++++++++++++++++++++++
 3 files changed, 70 insertions(+), 7 deletions(-)

-- 
1.7.5.1.219.ge2152.dirty

[RFC PATCH 1/2] sh-setup: Make die take the error code as param.

[Fredrik Gustafsson]

die used to print all parameters and then exit with code 1. Now die
prints the first parameter and uses the optional second parameter as the
exit code. The default exit code is 1.

This allows scripts to control the exit code when they call die.

All current git-code only uses the first parameter of die today so this
change has no impact on them.

Signed-off-by: Fredrik Gustafsson <iveqy@iveqy.com>
Mentored-by: Jens Lehmann <Jens.Lehmann@web.de>
Mentored-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 git-sh-setup.sh |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index aa16b83..6aa6c59 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -40,7 +40,11 @@ git_broken_path_fix () {
 # @@BROKEN_PATH_FIX@@
 
 die() {
-	echo >&2 "$@"
+	echo >&2 "$1"
+	if test $2
+	then
+		exit $2
+	fi
 	exit 1
 }
 
-- 
1.7.5.1.219.ge2152.dirty

Re: [RFC PATCH 1/2] sh-setup: Make die take the error code as param.

[Junio C Hamano]

Fredrik Gustafsson <iveqy@iveqy.com> writes:

> die used to print all parameters and then exit with code 1. Now die
> prints the first parameter and uses the optional second parameter as the
> exit code. The default exit code is 1.
>
> This allows scripts to control the exit code when they call die.
>
> All current git-code only uses the first parameter of die today so this
> change has no impact on them.

There are two issues in that logic.

The git-sh-setup API is a published interface that third-party Porcelains
can use, so it is insufficient to audit only the in-tree code.

Besides, your audit of in-tree code is insufficient. See output from this:

    $ git grep -A1 -n -e 'die .*\\$' -- '*.sh'

The hits in git-bisect.sh and git-rebase--merge.sh both pass two strings.

Can't you instead introduce a new "die_with_status" function and use it?

[RFC PATCH 2/2] submodule update: continue when a checkout fails

[Fredrik Gustafsson]

When running git submodule update the submodules are checked out in
alphabetic order. When an update of a submodule fails because of a
checkout error, continue to the next submodule and when done with all
submodules, exit with an error.

We only do this for 'safe' case when a submodule is not marked as
rebase or merge. When the update of a submodule fails because of a merge
or rebase, the update will still die immediately to give the user an
opportunity to resolve any conflicts before continuing.

Since submodule 'b' does not necessarily need to be dependent on
submodule 'a' this behavior is helpful if we have a lot of submodules.
For example if some submodules currently experience network problems
we can securely continue with the other submodules and the user can
revisit the failed one later on.

It also is helpful if a checkout fails because a submodules working
directory is dirty. Now the user can cleanup the submodule in question
and another git submodule update will just update the failed submodule
instead of all submodules that are ordered alphabetically afterwards.

Signed-off-by: Fredrik Gustafsson <iveqy@iveqy.com>
Mentored-by: Jens Lehmann <Jens.Lehmann@web.de>
Mentored-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 git-submodule.sh            |   42 ++++++++++++++++++++++++++++++++++++------
 t/t7406-submodule-update.sh |   29 +++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 6 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index bf110e9..02c41c7 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -444,7 +444,8 @@ cmd_update()
 	fi
 
 	cloned_modules=
-	module_list "$@" |
+	module_list "$@" | {
+	err=
 	while read mode sha1 stage path
 	do
 		if test "$stage" = U
@@ -525,17 +526,46 @@ cmd_update()
 				;;
 			esac
 
-			(clear_local_git_env; cd "$path" && $command "$sha1") ||
-			die "Unable to $action '$sha1' in submodule path '$path'"
-			say "Submodule path '$path': $msg '$sha1'"
+			if (clear_local_git_env; cd "$path" && $command "$sha1")
+			then
+				say "Submodule path '$path': $msg '$sha1'"
+			else
+				case $action in
+				rebase|merge)
+					die "Unable to $action '$sha1' in submodule path '$path'" 2
+					;;
+				*)
+					say "Unable to $action '$sha1' in submodule path '$path'"
+					err="Failed to $action one or more submodule(s)"
+					continue
+					;;
+				esac
+			fi
 		fi
 
 		if test -n "$recursive"
 		then
-			(clear_local_git_env; cd "$path" && eval cmd_update "$orig_flags") ||
-			die "Failed to recurse into submodule path '$path'"
+			(clear_local_git_env; cd "$path" && eval cmd_update "$orig_flags")
+			res=$?
+			if test $res -gt 0
+			then
+				if test $res -eq 1
+				then
+					say "Failed to recurse into submodule path '$path'"
+					continue
+				else
+					die "Failed to recurse into submodule path '$path'" $res
+				fi
+			fi
 		fi
 	done
+
+	if test -n "$err"
+	then
+		die "$err"
+	fi
+
+	}
 }
 
 set_name_rev () {
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 4f16fcc..e79c4df 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -298,4 +298,33 @@ test_expect_success 'submodule update ignores update=rebase config for new submo
 	)
 '
 
+test_expect_success 'submodule update continues after checkout error' '
+	(cd super &&
+	 git reset --hard HEAD &&
+	 git submodule add ../submodule submodule2 &&
+	 git submodule init &&
+	 git commit -am "new_submodule" &&
+	 (cd submodule2 &&
+	  git rev-parse HEAD > ../expect
+	 ) &&
+	 (cd submodule &&
+	  test_commit "update_submodule" file
+	 ) &&
+	 (cd submodule2 &&
+	  test_commit "update_submodule2" file
+	 ) &&
+	 git add submodule &&
+	 git add submodule2 &&
+	 git commit -m "two_new_submodule_commits" &&
+	 (cd submodule &&
+	 	echo "" > file
+	 ) &&
+	 git checkout HEAD^ &&
+	 test_must_fail git submodule update &&
+	 (cd submodule2 &&
+	  git rev-parse HEAD > ../actual
+	 ) &&
+	 test_cmp expect actual
+	) 
+'
 test_done
-- 
1.7.5.1.219.ge2152.dirty

Re: [RFC PATCH 2/2] submodule update: continue when a checkout fails

[Junio C Hamano]

Fredrik Gustafsson <iveqy@iveqy.com> writes:

> When running git submodule update the submodules are checked out in
> alphabetic order. When an update of a submodule fails because of a
> checkout error, continue to the next submodule and when done with all
> submodules, exit with an error.
>
> We only do this for 'safe' case when a submodule is not marked as
> rebase or merge. When the update of a submodule fails because of a merge
> or rebase, the update will still die immediately to give the user an
> opportunity to resolve any conflicts before continuing.
>
> Since submodule 'b' does not necessarily need to be dependent on
> submodule 'a' this behavior is helpful if we have a lot of submodules.
> For example if some submodules currently experience network problems
> we can securely continue with the other submodules and the user can
> revisit the failed one later on.
>
> It also is helpful if a checkout fails because a submodules working
> directory is dirty. Now the user can cleanup the submodule in question
> and another git submodule update will just update the failed submodule
> instead of all submodules that are ordered alphabetically afterwards.
>
> Signed-off-by: Fredrik Gustafsson <iveqy@iveqy.com>
> Mentored-by: Jens Lehmann <Jens.Lehmann@web.de>
> Mentored-by: Heiko Voigt <hvoigt@hvoigt.net>

Thanks, even though I find that the logic does not flow as smoothly as it
could in the above description, it now has sufficient information. Compared
to the previous round, it is a vast improvement.

I would rephrase it differently though. The problem description at the
highest level should come first.

	"git submodule update" stops at the first error and gives control
	back to the user. Only after the user fixes the problematic
	submodule and runs "git submodule update" again, the second error
	is found. And the user needs to repeat until all the problems are
	found and fixed one by one. This is tedious.

Then give a high level description of the proposed approach.

	Instead, the command can remember which submodules it had trouble
	with, continue updating the ones it can, and report which ones had
	errors at the end. The user can run "git submodule update", find
	all the ones that need minor fixing (e.g. working tree was dirty)
	to fix them in a single pass. Then another "git submodule update"
	can be run to update all.

And follow up with minor details.

	Note that the problematic submodules are skipped only when they
	are to be integrated with a safer value of submodule.<name>.update
	option, namely "checkout". Fixing a failure in a submodule that
	uses "rebase" or "merge" may need an involved conflict resolution
	by the user, and leaving too many submodules in states that need
	resolution would not reduce the mental burden on the user.

Wouldn't that be easier to understand? After all "alphabetical" is not an
essential part of the problem, but "stopping at the first error, with an
arbitrary definition of order of the submodules (which happens to be
alphabetical)" is the problem you are addressing, no?

Expressed that way, it may become clearer that it is dubious that stopping
on "rebase/merge" is something you should unilaterally enforce; instead,
the user may want to be able to configure if the command should skip and
continue in such a case.

>  git-submodule.sh            |   42 ++++++++++++++++++++++++++++++++++++------
>  t/t7406-submodule-update.sh |   29 +++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+), 6 deletions(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index bf110e9..02c41c7 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -525,17 +526,46 @@ cmd_update()
>  				;;
>  			esac
>  
> -			(clear_local_git_env; cd "$path" && $command "$sha1") ||
> -			die "Unable to $action '$sha1' in submodule path '$path'"
> -			say "Submodule path '$path': $msg '$sha1'"
> +			if (clear_local_git_env; cd "$path" && $command "$sha1")
> +			then
> +				say "Submodule path '$path': $msg '$sha1'"
> +			else
> +				case $action in
> +				rebase|merge)
> +					die "Unable to $action '$sha1' in submodule path '$path'" 2

Per my comments to your 1/2, this would become

					die_with_status 2 "Unable to $action '$sha1' in" \
				        	"submodule path '$path'"

I think.  And 1/2 would result in something like this:

	die_with_status () {
		status=$1
                shift
                echo >&2 "$*"
                exit $status
	}

        die () {
        	die_with_status 1 "$@"
	}

> +					err="Failed to $action one or more submodule(s)"
> +					continue

See below...

>  		if test -n "$recursive"
>  		then
> +			(clear_local_git_env; cd "$path" && eval cmd_update "$orig_flags")
> +			res=$?
> +			if test $res -gt 0
> +			then
> +				if test $res -eq 1
> +				then
> +					say "Failed to recurse into submodule path '$path'"
> +					continue

Hmm, should this case be err="..."?

> +				else
> +					die "Failed to recurse into submodule path '$path'" $res
> +				fi
> +			fi
>  		fi
>  	done
> +
> +	if test -n "$err"
> +	then
> +		die "$err"

I am not sure if you are properly addressing the original problem that you
are trying to solve.

When the first error stops the user, the user needs to fix the situation
with that submodule and continue to get another error, and then needs to
continue until all the errors are fixed incrementally, which is tedious.
But at least in the current code, when the user gets control back, the
error message clearly says which submodule was troublesome, so the user
knows where to go to fix things to make progress.

But after your change, the user essentially gets "Many submodules were
updated but there were a few that were skipped. Scroll up and read reams
of output to find out which ones they were".  Aren't you converting one
form of tedium with another?

How about making $err variable to accumulate the names/paths of submodules
you had trouble updating:

	punted_submodules= ;# initialization
	...

	if (... $command "$sha1")
        then
        	case "$action" in
                $unsafe_actions)
                	die_with_status 2 "Unable to $action '$sha1' ..." ;;
		*)
                	punted_submodules="$punted_submodules$path "
                        continue ;;
		esac

and formulate the error message here to list all of them, perhaps like:

	if test -n "$punted_modules"
        then
		die "Failed to update some modules: $punted_modules"
	fi