Re: [PATCH] git-submodule: Remove duplicate entries during merge with conflict

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Junio C Hamano <gitster@xxxxxxxxx> writes:

> Nicolas Morey-Chaisemartin <devel-git@xxxxxxxxxxxxxxxxxxxxxx> writes:
>
>> During a merge with conflict on a submodule, the submodule appears 3
>> times in git ls-files (stage 1,2,3) which causes the submodule to be
>> used 3 times in git submodule update or status command.  This patch
>> filters the results of git ls-files and only shows submodule in stage 0
>> or 1 thus removing the duplicates.
>
> That is a wrong thing to do.  What if both sides added a submodule at the
> same directory after forked from an ancestor that did not have it?  You
> will have only stage #2 and stage #3, no?

It is not very friendly to say a solution is wrong without hinting what
the right thing to do, so let's try.

There are 5 callers of module_list; they all read (mode, sha1, stage,
path) tuple, and most of them care only about path.  As a first level
approximation, it should be Ok (in the sense that it does not make things
worse than it currently is) to filter the duplicate paths from module_list
output.

But a bigger question is, shouldn't some callers _care_ when the
superproject is still unmerged?  What does it mean to say "submodule
update" when your superproject is unmerged, you haven't resolved it so you
don't know if you are going to take the commit your branch points at, or
the commit their branch points at, or a commit completely different from
either?  If submodule.$name.update is set to detach and checkout the
commit registred at the superproject, which stage do you want to get the
commit to update the submodule with?  If it is set to "rebase" or "merge",
which stage do you take the commit to rebase/merge your history onto?

I think "update" should not touch the submodule repository in such a case.
"status" might want to recurse into the submodule repository, but there is
no information at the superproject level to compare the submodule status
against, so we should at least skip that part for submodule entries that
are unmerged in the superproject.

Perhaps something along the lines of (as usual, totally untested, just to
illustrate the concept) the attached patch?

The idea is to notice the higher-stage entries, and emit only one record
from module_list, but while doing so, mark the entry with "U" (not [0-3])
in $stage field and null out the SHA-1 part, as the object name for the
lowest stage does not give any useful information to the caller, and this
way any caller that uses the object name would hopefully barf.

Fine points:

 - The command called by foreach may want to do whatever it wants to do by
   noticing the merged status in the superproject itself, so I made no
   change to the function;

 - Init is an unlikely thing to do, but as long as a submodule is there in
   $path, there is no point skipping it. It might however want to take the
   merged status of .gitmodules into account, but that is outside of the
   scope of this topic;

 - Update should refrain from touching the submodule itself. It may want
   to recurse into the submodule of the unmerged one, but people involved
   in submodule work should think things through;

 - Status does not have anything to report for an unmerged submodule. It
   may want to recurse into the submodule of the unmerged one, but people
   involved in submodule work should think things through; and

 - Sync should be Ok for the same reason as Init.



 git-submodule.sh |   29 ++++++++++++++++++++++++++++-
 1 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 3a13397..8ecd311 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -72,7 +72,24 @@ resolve_relative_url ()
 #
 module_list()
 {
-	git ls-files --error-unmatch --stage -- "$@" | sane_grep '^160000 '
+	git ls-files --error-unmatch --stage -- "$@" |
+	perl -e '
+	my %unmerged = ();
+	my ($null_sha1) = ("0" x 40);
+	while (<STDIN>) {
+		chomp;
+		my ($mode, $sha1, $stage, $path) =
+			/^([0-7]+) ([0-9a-f]{40}) ([0-3])\t(.*)$/;
+		next unless $mode eq "160000";
+		if ($stage ne "0") {
+			if (!$unmerged{$path}++) {
+				print "$mode $null_sha1 U\t$path\n";
+			}
+			next;
+		}
+		print "$_\n";
+	}
+	'
 }
 
 #
@@ -427,6 +444,11 @@ cmd_update()
 	module_list "$@" |
 	while read mode sha1 stage path
 	do
+		if test "$stage" = U
+		then
+			echo >&2 "Skipping unmerged submodule $path"
+			continue
+		fi
 		name=$(module_name "$path") || exit
 		url=$(git config submodule."$name".url)
 		update_module=$(git config submodule."$name".update)
@@ -767,6 +789,11 @@ cmd_status()
 	module_list "$@" |
 	while read mode sha1 stage path
 	do
+		if test "$stage" = U
+		then
+			echo >&2 "Skipping unmerged submodule $path"
+			continue
+		fi
 		name=$(module_name "$path") || exit
 		url=$(git config submodule."$name".url)
 		displaypath="$prefix$path"
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]