Re: [PATCH] mergetool: Teach about submodules

From: Jonathon Mah <me@jonathonmah.com>
Date: Apr 13, 2011, 10:00 UTC
Message-ID: <C411FA8C-8E26-4DB8-BC2A-B95CDBB2CF38@jonathonmah.com>
In-Reply-To: <7v62qkwomk.fsf@alter.siamese.dyndns.org>

Hi Junio,

On 2011-04-11, at 12:53, Junio C Hamano wrote:

Show 15 quoted lines

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

Thanks for the feedback. I'm not particularly experienced with shell scripts, so I don't usually think of the lack of variable scope.

Show 6 quoted lines

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

Indeed, it may not have. A v2 patch is following that handles the submodule path without a stage 2 (either because the submodule was deleted, or turned into a regular directory).

Show 13 quoted lines

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

Yes, this is the command I was intending. I needed it a while back when I was converting a monolithic repo into a super/submodule configuration, but couldn't find a better way than the kludge above. I had hoped someone would show me the clean way to do it!

Jonathon Mah me@JonathonMah.com