Re: [PATCH] mergetool: Teach about submodules

[Junio C Hamano <gitster@pobox.com>]

Jonathon Mah <me@JonathonMah.com> writes:

> Mergetool mildly clobbered submodules, attempting to move and copy their
> directories. It now recognizes submodules and offers a resolution:
> Submodule merge conflict for 'Shared':
>   {local}: commit ad9f12e3e6205381bf2163a793d1e596a9e211d0
>   {remote}: commit f5893fb70ec5646efcd9aa643c5136753ac89253
> Use (l)ocal or (r)emote, or (a)bort?

I was confused when I first read the first sentence, because "when/under
what condition" was missing.  I also suspect that we don't even have to
say "mildly".  The reviewers can judge the severity themselves.

When you can, please make the statement of the problem and the description
of the solution into separate paragraphs.  It also makes it easier to read
if you indent illustration (e.g. sample transcript) from your description.

Perhaps like this:

    When a merge in the superproject results in conflict at a submodule,
    mergetool used to mildly clobber submodules, attempting to move and
    copy their directories.

    Recognize submodules and offer a resolution instead:

      Submodule merge conflict for 'Shared':
        {local}: commit ad9f12e3e6205381bf2163a793d1e596a9e211d0
        {remote}: commit f5893fb70ec5646efcd9aa643c5136753ac89253
      Use (l)ocal or (r)emote, or (a)bort?

> Selecting a commit will stage it, but not update the submodule (as it
> would had there been no conflict). Type changes are also supported,
> should the path be a submodule on one side, and a file on the other.
>
> Signed-off-by: Jonathon Mah <me@JonathonMah.com>
> ...
> +resolve_submodule_merge () {
> +    while true; do
> +	printf "Use (l)ocal or (r)emote, or (a)bort? "
> +	read ans
> +	case "$ans" in
> +	    [lL]*)
> +		local_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $1;}')
> +		if is_submodule "$local_mode"; then
> +		    stage_submodule "$MERGED" $(git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $2;}')

If the version we had checked out and merging into has a submodule at the
path, use that.  This part of the logic seem sensible.

Don't you already have local_mode from the caller here?  For that matter,
don't you also have access to local_sha1 the caller already has computed?

> +		else
> +		    git checkout-index -f --stage=2 -- "$MERGED"
> +		    git add -- "$MERGED"

If what we had is not a submodule, then do a checkout-index.  Here you
assume that we _must_ have a stage #2 entry, but is that always the case?

Can we be in delete/modify conflict, where we had a submodule at the
common ancestor, we removed the submodule while the other branch modified
it?  What does this "else" clause do in such a case?

The same comment applies symmetrically to the "remote" case, of course.

> +stage_submodule () {
> +    path="$1"
> +    submodule_sha1="$2"
> +
> +    submodule_basename=$(basename "$path")
> +    tree_with_module=$(echo "160000 commit $submodule_sha1	\"$submodule_basename\"" | git mktree --missing 2>/dev/null)
> +    if test -z "$tree_with_module" ; then
> +	echo "$path: unable to stage commit $sha1"
> +	return 1
> +    fi
> +    git checkout $tree_with_module -- "$path"

Are you looking for "git update-index --cacheinfo 160000 $sha1 $name"
here, or is there something deeper going on?

If not, please don't use the "primarily for debugging and hacking" command
mktree for something like this to create a garbage tree object.

>  checkout_staged_file () {
>      tmpfile=$(expr "$(git checkout-index --temp --stage="$1" "$2")" : '\([^	]*\)	')
>  
> +    if is_submodule "$local_mode" || is_submodule "$remote_mode"; then
> +	echo "Submodule merge conflict for '$MERGED':"
> +	local_sha1=$(git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $2;}')
> +	remote_sha1=$(git ls-files -u -- "$MERGED" | awk '{if ($3==3) print $2;}')
> +	describe_file "$local_mode" "local" "$local_sha1"
> +	describe_file "$remote_mode" "remote" "$remote_sha1"
> +	resolve_submodule_merge
> +	return

I really hate these repeated "awk" invocations, here and then inside the
callee.  As the script seems to use these as global variables, the callee
shouldn't have to recompute local/remote-mode/sha1, no?

Thanks.