Re: Diffs for submodule conflicts during rebase usually empty

[Jens Lehmann <Jens.Lehmann@web.de>]

Am 11.09.2014 um 19:50 schrieb ezyang:
> Hello all,
>
> In many situations, if you have a submodule conflict during a rebase,
> and you type 'git diff' to get a summary of the situation, you will get
> an empty diff.  Here's a simple transcript for one such case (I'm sorry
> I can't make it much shorter), tested on git version 2.0.3.693.g996b0fd:
>
>      git init
>      mkdir b
>      cd b
>      git init
>      git commit --allow-empty -m "submodule initial"
>      cd ..
>      git submodule add ./b
>      git commit -am "parent initial"
>      git branch dev
>      cd b
>      touch a
>      git add a
>      git commit -m "submodule master"
>      cd ..
>      git commit -am "parent master"
>      git checkout dev
>      git submodule update
>      cd b
>      touch b
>      git add b
>      git commit -m "submodule dev"
>      cd ..
>      git commit -am "parent dev"
>      git rebase master
>      git diff b
>
> The last output is:
>
>      diff --cc b
>      index 4b1b6c6,c423df2..0000000
>      --- a/b
>      +++ b/b

Thanks for providing a simple way to reproduce what you are seeing.

> As it turns out, this behavior is logical in a perverse sort of way.
>
>      - The rebase operation doesn't go about updating your submodule
>        checkouts, so whatever is in the file is what the submodule
>        was pointing to before your initiated the rebase.
>
>      - By default, 'git diff' on a merge conflict (implicitly
>        'git diff --cc') only will report if the submodule's HEAD
>        differs from all of the merge heads.  So if you only had
>        one commit which changed the submodule, you're probably
>        on that commit, and so the "current state" of the submodule
>
> However, just because behavior is logical, doesn't mean it is user
> friendly.  There are a few problems here:
>
>      1. Git is treating the lagging submodule HEAD as if it were
>      actually a resolution that you might want for the conflict.
>      Actually, it's basically almost always wrong (in the example
>      above, if you commit it you'll be discarding commits made on
>      master.)  There is a sorter of wider UI issue here where Git
>      can't tell if you've legitimately changed the HEAD pointer
>      of a submodule, or if you checked out a new revision with different
>      submodule pointers and forgot to run 'git submodule update'.
>      (But by the way, you can't even do that here, because this is
>      a merge!)
>
>      2. The behavior of not reporting the diff when the diff for one
>      branch is non-empty is illogical: for submodules (whose "file
>      contents" are so short), you basically always want some hashes,
>      and not an empty diff.  Doubly so when the "resolution" is
>      bogus (c.f. (1)).
>
> Of course, changing behavior in a backwards-incompatible way is never a
> good way, so it's not exactly obvious what should be done here. I would
> recommend tweaking the default combined diff behavior for submodules and
> adding an admonition to the user that the submodules have not been
> updated in the rebase message (I can submit a patch for this if people
> agree if it's a good idea), but maybe that's too much of a behavior
> change.
>
> By the way, the difference between 'git diff -c' and 'git diff --cc'
> does not seem to be documented anywhere, except for an oblique comment
> in diff-format.txt "Note that 'combined diff' lists only files which
> were modified from all parents." -- the user expected, of course, to
> figure out that 'combined diff' here refers to --cc, but not -c.

It looks to me like your confusion is because current Git isn't
terribly good at displaying merge conflicts in submodules. While
diff produces rather confusing output:

	$ git diff
	diff --cc b
	index fc12d34,33d9fa9..0000000
	--- a/b
	+++ b/b

Git does know what's going on, just fails to display it properly
in the diff, as the output of ls-files shows:

	$git ls-files -u
	160000 6a6e215138b7f343fba67ba1b6ffc152019c6085 1	b
	160000 fc12d3455b120916ec508c3ccd04f23957c08ea5 2	b
	160000 33d9fa9f9e25de2a85f84993d8f6c752f84c769a 3	b

I agree that this needs to be improved, but am currently lacking
the time to do it myself. But I believe this will get important
rather soonish when we recursively update submodules too ...