problem with cherry-picking a commit which comes before introducing a new submodule

Hi GIT Gurus,

per quick IRC troubleshooting it seems to be some kind of an issue in git cherry-pick logic which tries to deal with existing .gitmodules structure?

In our repository we had some submodules, then there was a branch off (call new branch todonotloose) with a single commit. In the master we had some other commits and moved one of the subdirectories into a submodule.

Later on we decided to cherry pick todonotloose into master but cherry-pick fails despite the fact that 'git show todonotloose | patch -p1' applies just fine, ie there were no changes touching any of the submodules.

See http://pastebin.com/hpqbiB03 for output.

IMHO everything is legit and failure should have not occurred, or am I missing something?

(+cc: Elijah Newren, who has worked on some of this code)

Yaroslav Halchenko wrote:

[...]

As contrib/examples/git-revert.sh explains, the heart of "git cherry-pick" is

	base=todonotloose^
	next=todonotloose
	head=HEAD

	git merge-recursive $base -- $head $next

Could you try that, perhaps with GIT_MERGE_VERBOSITY=4 (or some other number from 1 to 5, larger is louder) in the environment? For context,

	git ls-files -u;	# after the merge
	git diff-tree todonotloose
	git diff-tree todonotloose^ HEAD

would also be interesting.

oy -- I thought that cherry-pick is primarily just application of the patch (without use of the merge clevernesses).

Here is the protocol: % export GIT_MERGE_VERBOSITY=4 % base=todonotloose^ % next=todonotloose % head=HEAD % % git merge-recursive $base -- $head $next Merging HEAD with todonotloose Merging: 855981d just placeholders in the abstract a00c497 Initial draft for HBM abstract. CONFLICT (file/directory): There is a directory with name frontiers/code in todonotloose. Adding frontiers/code as frontiers/code~HEAD % git ls-files -u; # after the merge 160000 a2b57871d2d79bef06ba6214739d82b9a63772a8 2 frontiers/code zsh: command not found: # % git diff-tree todonotloose a00c497fa399c00486c97121ed0b8fda72c7ce47 :040000 040000 40427e34a1ff89c458f2a5f262a108d46b4fa004 c7ba91028b1cef63f4f7eef70f0c4054b31e92b6 M poster-hbm2011_neurodebian % git diff-tree todonotloose^ HEAD :100644 100644 378e1379ec5ebb7abac59fec162b7238b5846525 c39ced763aeb5fd352cecd6fef1bfc40471f2246 M .gitmodules :000000 040000 0000000000000000000000000000000000000000 141dbc1bfe1be2eab77f04ca03f6f28feb372cca A challenge-execpapers :040000 040000 401fd66867de412b8653dc3a698bbaa45441bec1 ee190f09786f324abdda6e7a36e8278c201a20a0 M frontiers :040000 040000 26c884a67efb55bdf96d7453d9acd50cee36ae90 ad3e829d15b302c4342a6b2a9fb5dfede0ed77c9 M sty

On Fri, 07 Jan 2011, Jonathan Nieder wrote:

Yaroslav Halchenko wrote [abbreviated]:

One more piece of protocol: what git version are you using? The release notes mention a fix in this area in v1.7.3[1]:

 * "git merge -s recursive" (which is the default) did not handle cases
   where a directory becomes a file (or vice versa) very well.

Hopefully this is that.  In any case, sounds like a bug.

(Hopefully someone else can comment on why cherry-pick uses the merge machinery to notice conflicts that would not be clear from the patch alone.)

Thanks again. Jonathan

[1] There is an updated Debian source package at [2]. Or, probably faster: one can use the build result in bin-wrappers/ from a git.git clone in place. [2] http://mentors.debian.net/debian/pool/main/g/git/git_1.7.4~rc1-0.1.dsc

sorry -- lame me... 1.7.2.3 ... I will check with current version as soon as kids permit ;-)

% apt-cache policy git
git:
  Installed: 1:1.7.2.3-2.2
  Candidate: 1:1.7.2.3-2.2
  Version table:
 *** 1:1.7.2.3-2.2 0
        900 http://debian.lcs.mit.edu/debian/ squeeze/main amd64 Packages
        800 http://debian.lcs.mit.edu/debian/ sid/main amd64 Packages
        100 /var/lib/dpkg/status
% git --version
git version 1.7.2.3

On Fri, 07 Jan 2011, Jonathan Nieder wrote:

message is different -- result the same:
I: writing typescript to /home/yoh/.tmp/script.git-cherry-pick.17386.20110107.1857 ...
% git --version
git version 1.7.4.rc1
% git reset --hard
HEAD is now at 855981d just placeholders in the abstract
% export GIT_MERGE_VERBOSITY=5
% git cherry-pick todonotloose
CONFLICT (file/directory): There is a directory with name frontiers/code in a00c497... Initial draft for HBM abstract.. Adding frontiers/code as frontiers/code~HEAD
error: could not apply a00c497... Initial draft for HBM abstract.
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit -c a00c497'
% git status      
# On branch master
# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#
#   new file:   poster-hbm2011_neurodebian/abstract.txt
#   modified:   poster-hbm2011_neurodebian/jb.txt
#
# Unmerged paths:
#   (use "git reset HEAD <file>..." to unstage)
#   (use "git add/rm <file>..." as appropriate to mark resolution)
#
#   added by us:        frontiers/code
#
% git reset --hard
HEAD is now at 855981d just placeholders in the abstract
%         base=todonotloose^
%         next=todonotloose
%         head=HEAD
% 
%         git merge-recursive $base -- $head $next
Merging HEAD with todonotloose
Merging:
855981d just placeholders in the abstract
a00c497 Initial draft for HBM abstract.
found 1 common ancestor(s):
4708e24 minor moves around
CONFLICT (file/directory): There is a directory with name frontiers/code in todonotloose. Adding frontiers/code as frontiers/code~HEAD
%         git ls-files -u;        # after the merge
160000 a2b57871d2d79bef06ba6214739d82b9a63772a8 2   frontiers/code
zsh: command not found: #
%         git diff-tree todonotloose
a00c497fa399c00486c97121ed0b8fda72c7ce47
:040000 040000 40427e34a1ff89c458f2a5f262a108d46b4fa004 c7ba91028b1cef63f4f7eef70f0c4054b31e92b6 M  poster-hbm2011_neurodebian
%         git diff-tree todonotloose^ HEAD
:100644 100644 378e1379ec5ebb7abac59fec162b7238b5846525 c39ced763aeb5fd352cecd6fef1bfc40471f2246 M  .gitmodules
:000000 040000 0000000000000000000000000000000000000000 141dbc1bfe1be2eab77f04ca03f6f28feb372cca A  challenge-execpapers
:040000 040000 401fd66867de412b8653dc3a698bbaa45441bec1 ee190f09786f324abdda6e7a36e8278c201a20a0 M  frontiers
:040000 040000 26c884a67efb55bdf96d7453d9acd50cee36ae90 ad3e829d15b302c4342a6b2a9fb5dfede0ed77c9 M  sty

On Fri, 07 Jan 2011, Jonathan Nieder wrote:

Yaroslav Halchenko wrote [abbreviated]:

Here is what happens.

In the heart of merge_trees:

	/*
	 * If there are D/F conflicts, and the paths currently exist
	 * in the working copy as a file, remove them to make room
	 * for the corresponding directory.  Such paths will later be
	 * processed in process_df_entry() at the end.
	 *
	 * If the corresponding directory ends up being removed by the
	 * merge, then the file will be reinstated at that time;
	 * otherwise, if the file is not supposed to be removed by the
	 * merge, the contents of the file will be placed in another
	 * unique filename.
	 */
	make_room_for_directories_of_df_conflicts(o, entries);

In this case I suppose it is rather a directory/submodule conflict; in any case, there are no regular files involved, so this logic does not kick in and the directory is left alone.

Next comes rename handling, which is irrelevant for our purposes.

Next comes the per entry merge.

	/*
	 * Per entry merge.  D/F conflicts are deferred so files
	 * contained in such a directory can be resolved first.
	 */
	for (i = 0; i < entries->nr; i++) {
		const char *path = entries->items[i].string;
		struct stage_data *e = entries->items[i].util;
		if (!e->processed
			&& !process_entry(o, path, e))
			clean = 0;
	}

This is case B: "added in one" (like all directories, the frontiers/code directory does not have an index entry, while the submodule does have one). Since that path is in the current directory set, it is deferred for later processing.

Next comes the per entry merge for D/F conflicts (process_df_entry in merge-recursive.c). This is the case "directory -> (directory, file)". Unfortunately the check that the old and new directories match is not implemented. Even worse, git checks for a directory (which was not moved out of the way before) and does not realize that a submodule might be another reason for a directory in the worktree. In any event, we get a spurious conflict.

Thanks, that was interesting (no patch yet, alas).

On Tue, 11 Jan 2011, Jonathan Nieder wrote:

is there a way to memorize this issue somewhere (bug tracking/TODO/etc) where this issue could be recorded so it doesn't get forgotten? ;)

On 01/18/2011 05:02 PM, Yaroslav Halchenko wrote:

It will be stored in the hive-mind of the mailing list participants. It's quite a bit better than it sounds actually.

On Tue, 18 Jan 2011, Andreas Ericsson wrote:

ok -- offload completed then, I am cleaning up my memory bank