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

Fredrik Gustafsson <iveqy@iveqy.com> writes:

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.

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 "$@"
	}

See below...

Hmm, should this case be 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