linux-next: manual merge of the security-testing tree with the tree

linux-next: manual merge of the security-testing tree with the tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 3680 bytes --]

Hi James,

Today's linux-next merge of the security-testing tree produced a large
number of conflicts in files not modified by the security-testing tree.
I assume that this is a bug in "git merge" but I cannot complete the
merge as such.

I have used the security-testing tree from next-20110628 for today.

More information for the git experts:

The security-testing tree is at
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6.git
and I am trying to merge branch "next" into linux-next at commit
9fd8fab5e299a ("Merge remote-tracking branch 'voltage/for-next'").

I can merge commit 0e4ae0e0dec6 ("TOMOYO: Make several options
configurable") from the security testing tree without conflict and also
commit 25e75dff519b ("AppArmor: Fix masking of capabilities in complain
mode").

I cannot merge commit bcd05ca10420 ("Merge branch 'for-security' of
git://git.kernel.org/pub/scm/linux/kernel/git/jj/apparmor-dev into next")
which is the child of the above two commits.

The tree to commit 25e75dff519b only containes two simple commits
(modifying 2 files) and is based on v3.0-rc5.  The tree to commit
bcd05ca10420 containes several commits and is based on commit
06e86849cf40 ("Merge branch 'pm-fixes' of
git://git.kernel.org/pub/scm/linux/kernel/git/rafael/suspend-2.6") from
Linus' tree (between v3.0-rc2 and v3.0-rc2).

After attempting the merge I get this:

$ git status
# On branch master
# Changes to be committed:
#
#	deleted:    Documentation/ABI/testing/sysfs-class-backlight-driver-adp8870
#	modified:   Documentation/accounting/cgroupstats.txt
#	modified:   Documentation/cgroups/blkio-controller.txt
#	modified:   Documentation/cgroups/cgroups.txt
#	modified:   Documentation/cgroups/cpuacct.txt
#	modified:   Documentation/cgroups/cpusets.txt
	.
	.	[lots elided]
	.
#	modified:   tools/perf/util/PERF-VERSION-GEN
#	modified:   tools/perf/util/trace-event-parse.c
#
# Unmerged paths:
#   (use "git add/rm <file>..." as appropriate to mark resolution)
#
#	both modified:      arch/arm/mach-shmobile/board-ag5evm.c
#	both modified:      arch/arm/mm/context.c
#	both modified:      arch/arm/mm/proc-v7.S
#	both modified:      arch/arm/plat-mxc/devices/platform-imx-dma.c
#	both modified:      arch/arm/plat-s5p/include/plat/map-s5p.h
#	both modified:      arch/m68k/Kconfig.nommu
#	both modified:      block/blk-throttle.c
#	both modified:      drivers/gpu/drm/nouveau/nouveau_fence.c
#	deleted by them:    drivers/net/usb/kalmia.c
#	both modified:      drivers/net/wireless/iwlegacy/iwl-dev.h
#	both modified:      drivers/net/wireless/iwlegacy/iwl4965-base.c
#	both modified:      drivers/net/wireless/iwlwifi/iwl-agn-rxon.c
#	both modified:      drivers/net/wireless/iwlwifi/iwl-agn.c
#	both modified:      drivers/net/wireless/rtlwifi/pci.c
#	deleted by them:    drivers/video/backlight/adp8870_bl.c
#	both modified:      fs/namei.c
#	both modified:      fs/nfs/nfs4proc.c
#	both modified:      fs/nfs/pnfs.c
#	both modified:      fs/proc/base.c
#	both modified:      net/bluetooth/rfcomm/sock.c
#	both modified:      net/ipv4/ip_output.c
#	both modified:      net/netfilter/ipvs/ip_vs_core.c
#	both modified:      sound/pci/hda/patch_via.c
#	both modified:      sound/soc/codecs/ad1836.h
#	both modified:      sound/soc/soc-cache.c
#

None of the "Unmerged paths" are modified in the tree I am merging in.

The linux-next tree will be published later today and James' tree is
available already (I can publis my copy of it if James modifies his).

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: linux-next: manual merge of the security-testing tree with the tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 309 bytes --]

On Thu, 30 Jun 2011 14:29:10 +1000 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> More information for the git experts:
> 

$ git version
git version 1.7.5.4

I am running Debian unstable.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: linux-next: manual merge of the security-testing tree with the tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 677 bytes --]

On Thu, 30 Jun 2011 14:33:34 +1000 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> On Thu, 30 Jun 2011 14:29:10 +1000 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> >
> > More information for the git experts:
> > 
> 
> $ git version
> git version 1.7.5.4
> 
> I am running Debian unstable.

Today's linux-next is based on v3.0-rc5-76-gc017d0d (Linus' tree commit
c017d0d135 ("Merge branch 'kvm-updates/3.0' of
git://git.kernel.org/pub/scm/virt/kvm/kvm").

The point at which I tried to merge the security-testing tree is
v3.0-rc5-3666-g9fd8fab.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: linux-next: manual merge of the security-testing tree with the tree

[James Morris]

On Thu, 30 Jun 2011, Stephen Rothwell wrote:

> I cannot merge commit bcd05ca10420 ("Merge branch 'for-security' of
> git://git.kernel.org/pub/scm/linux/kernel/git/jj/apparmor-dev into next")
> which is the child of the above two commits.

Actually, I may have accidentally pulled upstream commits in via the 
above.  I'll rebase my tree to the TOMOYO merge.


- James
-- 
James Morris
<jmorris@namei.org>

Re: linux-next: manual merge of the security-testing tree with the tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 726 bytes --]

Hi James,

On Thu, 30 Jun 2011 15:22:33 +1000 (EST) James Morris <jmorris@namei.org> wrote:
>
> On Thu, 30 Jun 2011, Stephen Rothwell wrote:
> 
> > I cannot merge commit bcd05ca10420 ("Merge branch 'for-security' of
> > git://git.kernel.org/pub/scm/linux/kernel/git/jj/apparmor-dev into next")
> > which is the child of the above two commits.
> 
> Actually, I may have accidentally pulled upstream commits in via the 
> above.  I'll rebase my tree to the TOMOYO merge.

It does pull in v3.0-rc5, but that is already in my tree today, so I
still don't understand why the merge I did gets so messaed up.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: linux-next: manual merge of the security-testing tree with the tree

[Uwe Kleine-König]

On Thu, Jun 30, 2011 at 02:29:10PM +1000, Stephen Rothwell wrote:
> Hi James,
> 
> Today's linux-next merge of the security-testing tree produced a large
> number of conflicts in files not modified by the security-testing tree.
> I assume that this is a bug in "git merge" but I cannot complete the
> merge as such.
> 
> I have used the security-testing tree from next-20110628 for today.
> 
> More information for the git experts:
> 
> The security-testing tree is at
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6.git
> and I am trying to merge branch "next" into linux-next at commit
> 9fd8fab5e299a ("Merge remote-tracking branch 'voltage/for-next'").
> 
> I can merge commit 0e4ae0e0dec6 ("TOMOYO: Make several options
> configurable") from the security testing tree without conflict and also
> commit 25e75dff519b ("AppArmor: Fix masking of capabilities in complain
> mode").
> 
> I cannot merge commit bcd05ca10420 ("Merge branch 'for-security' of
> git://git.kernel.org/pub/scm/linux/kernel/git/jj/apparmor-dev into next")
> which is the child of the above two commits.
> 
> The tree to commit 25e75dff519b only containes two simple commits
> (modifying 2 files) and is based on v3.0-rc5.  The tree to commit
> bcd05ca10420 containes several commits and is based on commit
> 06e86849cf40 ("Merge branch 'pm-fixes' of
> git://git.kernel.org/pub/scm/linux/kernel/git/rafael/suspend-2.6") from
> Linus' tree (between v3.0-rc2 and v3.0-rc2).
> 
> After attempting the merge I get this:
> 
> $ git status
> # On branch master
> # Changes to be committed:
> #
> #	deleted:    Documentation/ABI/testing/sysfs-class-backlight-driver-adp8870
> #	modified:   Documentation/accounting/cgroupstats.txt
> #	modified:   Documentation/cgroups/blkio-controller.txt
> #	modified:   Documentation/cgroups/cgroups.txt
> #	modified:   Documentation/cgroups/cpuacct.txt
> #	modified:   Documentation/cgroups/cpusets.txt
> 	.
> 	.	[lots elided]
> 	.
> #	modified:   tools/perf/util/PERF-VERSION-GEN
> #	modified:   tools/perf/util/trace-event-parse.c
> #
> # Unmerged paths:
> #   (use "git add/rm <file>..." as appropriate to mark resolution)
> #
> #	both modified:      arch/arm/mach-shmobile/board-ag5evm.c
> #	both modified:      arch/arm/mm/context.c
> #	both modified:      arch/arm/mm/proc-v7.S
> #	both modified:      arch/arm/plat-mxc/devices/platform-imx-dma.c
> #	both modified:      arch/arm/plat-s5p/include/plat/map-s5p.h
> #	both modified:      arch/m68k/Kconfig.nommu
> #	both modified:      block/blk-throttle.c
> #	both modified:      drivers/gpu/drm/nouveau/nouveau_fence.c
> #	deleted by them:    drivers/net/usb/kalmia.c
> #	both modified:      drivers/net/wireless/iwlegacy/iwl-dev.h
> #	both modified:      drivers/net/wireless/iwlegacy/iwl4965-base.c
> #	both modified:      drivers/net/wireless/iwlwifi/iwl-agn-rxon.c
> #	both modified:      drivers/net/wireless/iwlwifi/iwl-agn.c
> #	both modified:      drivers/net/wireless/rtlwifi/pci.c
> #	deleted by them:    drivers/video/backlight/adp8870_bl.c
> #	both modified:      fs/namei.c
> #	both modified:      fs/nfs/nfs4proc.c
> #	both modified:      fs/nfs/pnfs.c
> #	both modified:      fs/proc/base.c
> #	both modified:      net/bluetooth/rfcomm/sock.c
> #	both modified:      net/ipv4/ip_output.c
> #	both modified:      net/netfilter/ipvs/ip_vs_core.c
> #	both modified:      sound/pci/hda/patch_via.c
> #	both modified:      sound/soc/codecs/ad1836.h
> #	both modified:      sound/soc/soc-cache.c
> #
> 
> None of the "Unmerged paths" are modified in the tree I am merging in.
Hmm, looking at bcd05ca10420 and the difference to its first parent:

	$ git diff --stat bcd05ca10420^ bcd05ca10420
	<void>

	$ git describe bcd05ca10420
	v3.0-rc5-28-gbcd05ca

	$ git describe bcd05ca10420^
	v3.0-rc2-221-g0e4ae0e

So commit bcd05ca10420 reverted many commits between v3.0-rc2 and v3.0-rc5.

If I redo what should have been done in bcd05ca10420 and compare with
bcd05ca10420:

	git checkout bcd05ca10420^
	git merge bcd05ca10420^2
	git diff --stat bcd05ca10420

I get the same list of touched files as you above.

Long history short: James probably used -s ours or similar and it's fine
not to merge that commit into next :-)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: linux-next: manual merge of the security-testing tree with the tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 420 bytes --]

Hi Uwe,

On Thu, 30 Jun 2011 09:25:59 +0200 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
>
> Long history short: James probably used -s ours or similar and it's fine
> not to merge that commit into next :-)

Ah ha!  Thanks for the explanation.  My mind was clearly not up to it
today. :-)

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: linux-next: manual merge of the security-testing tree with the tree

[Uwe Kleine-König]

On Thu, Jun 30, 2011 at 07:30:23PM +1000, Stephen Rothwell wrote:
> Hi Uwe,
> 
> On Thu, 30 Jun 2011 09:25:59 +0200 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> >
> > Long history short: James probably used -s ours or similar and it's fine
> > not to merge that commit into next :-)
> 
> Ah ha!  Thanks for the explanation.  My mind was clearly not up to it
> today. :-)
The uncomfortable issue here is that 

	git show bcd05ca10420

(or gitk or gitweb or <enteryourfavoritetoolhere>) doesn't indicate that
it's "strange". The patch shown is simply empty, as it would be if the
tree matched the other parent or if it were a clean merge.

A flag would be nice that does what I did: redo the merge and compare
bcd05ca10420^{tree} with the result?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: linux-next: manual merge of the security-testing tree with the tree

[Linus Torvalds]

2011/6/30 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>
> A flag would be nice that does what I did: redo the merge and compare
> bcd05ca10420^{tree} with the result?

The problem with that is that it's *way* too expensive an operation to
do for the normal "git log" kind of operations.

Also, truly re-doing the merge actually requires workspace access
and/or require new objects to be created, so it would be inappropriate
anyway: git log/show absolutely has to be a read-only operation,
anything else would be totally insane.

So there's no way - both for performance and 'fundamental' reasons -
to make the normal logging code truly re-do the merge and then compare
the end result of the merge with the end result that is in the tree.

That said, what the current "git show/log" does is to just compare the
end points with the merge result, which means that if the end result
matches either of the end-points, nothing will be shown. That works
for the common cases, but it absolutely doesn't work if somebody does
something crazy, and just picks one end-point over another without
doing a proper merge (ie "-s ours" or just a mis-merge). But the
reason it's done that way is that it's possible to do without re-doing
the merge.

It would be lovely if "git show" (and log operations) had some option
to do a "expensive merge check" and did actually figure out the common
ancestor and at least took that into account.

It would be doable to do it at least better than we do now - the
common ancestor is not cheap to compute, but it's much cheaper than a
full merge, and would at least allow us to flag dangerous merges. Of
course, it gets fun when there are multiple common ancestors and
renames. It's entirely possible that it's never going to be practical
to do anything but "re-do the merge and compare result".

                      Linus

Re: linux-next: manual merge of the security-testing tree with the tree

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> It would be lovely if "git show" (and log operations) had some option
> to do a "expensive merge check" and did actually figure out the common
> ancestor and at least took that into account.
>
> It would be doable to do it at least better than we do now - the
> common ancestor is not cheap to compute, but it's much cheaper than a
> full merge, and would at least allow us to flag dangerous merges. Of
> course, it gets fun when there are multiple common ancestors and
> renames. It's entirely possible that it's never going to be practical
> to do anything but "re-do the merge and compare result".

I would have to say that it would boil down to "re-do the merge" whichever
way we implement it, and it is not necessarily a bad thing.  

There are ideas to implement a mode of "git merge" that works entirely
in-core without touching the working tree (it may have to write temporary
blobs and possibly trees to the object store, though). It would let sites
like github to let its users accept a trivial pull request that can merge
cleanly on site in the browser without necessarily having to have a local
checkout used for conflict resolution.

If such an "in-core merge" feature is implemented cleanly in a reusable
way, it would be just the matter of comparing the output from it with the
actual committed result.

Of course, if the committed result was deliberately made by "-s ours",
comparison between an auto-merge result and the committed result would
produce a lot of noise, but that is really the point of "expensie merge
check", so the noise in that scenario is a feature, not a bug.

Re: linux-next: manual merge of the security-testing tree with the tree

[Jeff King]

On Thu, Jun 30, 2011 at 11:52:17AM -0700, Junio C Hamano wrote:

> I would have to say that it would boil down to "re-do the merge" whichever
> way we implement it, and it is not necessarily a bad thing.  
> 
> There are ideas to implement a mode of "git merge" that works entirely
> in-core without touching the working tree (it may have to write temporary
> blobs and possibly trees to the object store, though). It would let sites
> like github to let its users accept a trivial pull request that can merge
> cleanly on site in the browser without necessarily having to have a local
> checkout used for conflict resolution.
> 
> If such an "in-core merge" feature is implemented cleanly in a reusable
> way, it would be just the matter of comparing the output from it with the
> actual committed result.

Below is my unpolished, probably-buggy-as-hell patch to do the in-core
content merge.  But there are still two sticking points:

  1. This is a dirt-simple 3-way content merge. The actual merge would
     likely have used some more complex strategy. So you're going to see
     discrepancies between a real merge, even a correct one, and what
     this produces (e.g., in the face of renames detected by
     merge-recursive).

  2. This just makes read-tree do the content merge where it doesn't
     conflict, and leaves the conflicted cases unmerged in the index.
     Which is of course the only sane thing to put in the index.

     But what do you want to do about comparing entries with conflicts,
     which are the really interesting bits?  Compare the result to the
     version of the file with conflict markers? If so, where do you want
     to store the file with conflict markers?  I guess we could generate
     an in-core index with the conflict markers that we are just going
     to throw away. That seems pretty hack-ish.

-Peff

-- >8 --
Subject: [PATCH] teach read-tree to do content-level merges

Read-tree will resolve simple 3-way merges, such as a path
touched on one branch but not on the other. With
--aggressive, it will also do some more complex merges, like
both sides adding the same content. But it always stops
short of actually merging content, leaving the unmerged
paths in the index.

One can always use "git merge-index git-merge-one-file -a"
to do a content-level merge of these paths. However, that
has two disadvantages:

  1. It's slower, as we actually invoke merge-one-file for
     each unmerged path, which in turns writes temporary
     files to the filesystem.

  2. It requires a working directory to store the merged
     result. When working in a bare repository, this can be
     inconvenient.

Instead, let's have read-tree perform the content-level
merge in core. If it results in conflicts, read-tree can
simply punt and leave the unmerged entries in the index.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/read-tree.c |    2 +
 unpack-trees.c      |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++
 unpack-trees.h      |    1 +
 3 files changed, 72 insertions(+), 0 deletions(-)

diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index df6c4c8..392c378 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -117,6 +117,8 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 			    "3-way merge if no file level merging required", 1),
 		OPT_SET_INT(0, "aggressive", &opts.aggressive,
 			    "3-way merge in presence of adds and removes", 1),
+		OPT_SET_INT(0, "merge-content", &opts.file_level_merge,
+			    "3-way merge of non-conflicting file content", 1),
 		OPT_SET_INT(0, "reset", &opts.reset,
 			    "same as -m, but discard unmerged entries", 1),
 		{ OPTION_STRING, 0, "prefix", &opts.prefix, "<subdirectory>/",
diff --git a/unpack-trees.c b/unpack-trees.c
index 3a61d82..0443fcf 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -8,6 +8,8 @@
 #include "progress.h"
 #include "refs.h"
 #include "attr.h"
+#include "xdiff-interface.h"
+#include "blob.h"
 
 /*
  * Error messages expected by scripts out of plumbing commands such as
@@ -1515,6 +1517,45 @@ static void show_stage_entry(FILE *o,
 }
 #endif
 
+static int file_level_merge(unsigned char sha1[20],
+			    struct cache_entry *old,
+			    struct cache_entry *head,
+			    struct cache_entry *remote)
+{
+	mmfile_t old_data = {0}, head_data = {0}, remote_data = {0};
+	mmbuffer_t resolved = {0};
+	xmparam_t xmp = {{0}};
+	int ret = -1;
+
+	if (remote->ce_mode != head->ce_mode &&
+	    remote->ce_mode != old->ce_mode)
+		goto out;
+
+	read_mmblob(&old_data, old->sha1);
+	if (buffer_is_binary(old_data.ptr, old_data.size))
+		goto out;
+	read_mmblob(&head_data, head->sha1);
+	if (buffer_is_binary(head_data.ptr, head_data.size))
+		goto out;
+	read_mmblob(&remote_data, remote->sha1);
+	if (buffer_is_binary(remote_data.ptr, remote_data.size))
+		goto out;
+
+	xmp.level = XDL_MERGE_ZEALOUS_ALNUM;
+	if (xdl_merge(&old_data, &head_data, &remote_data, &xmp, &resolved))
+		goto out;
+	if (write_sha1_file(resolved.ptr, resolved.size, blob_type, sha1) < 0)
+		die("unable to write resolved blob object");
+	ret = 0;
+
+out:
+	free(old_data.ptr);
+	free(head_data.ptr);
+	free(remote_data.ptr);
+	free(resolved.ptr);
+	return ret;
+}
+
 int threeway_merge(struct cache_entry **stages, struct unpack_trees_options *o)
 {
 	struct cache_entry *index;
@@ -1653,6 +1694,34 @@ int threeway_merge(struct cache_entry **stages, struct unpack_trees_options *o)
 			return -1;
 	}
 
+	if (o->file_level_merge &&
+	    !no_anc_exists && head && remote && !head_match && !remote_match) {
+		int i;
+		struct cache_entry *old = NULL;
+		unsigned char sha1[20];
+
+		for (i = 1; i < o->head_idx; i++) {
+			if (stages[i] && stages[i] != o->df_conflict_entry) {
+				old = stages[i];
+				break;
+			}
+		}
+		if (!old)
+			die("BUG: file-level merge couldn't find ancestor");
+
+		if (file_level_merge(sha1, old, head, remote) == 0) {
+			/* ugh */
+			unsigned char tmp[20];
+			int r;
+
+			hashcpy(tmp, head->sha1);
+			hashcpy(head->sha1, sha1);
+			r = merged_entry(head, index, o);
+			hashcpy(head->sha1, tmp);
+			return r;
+		}
+	}
+
 	o->nontrivial_merge = 1;
 
 	/* #2, #3, #4, #6, #7, #9, #10, #11. */
diff --git a/unpack-trees.h b/unpack-trees.h
index 7998948..516c2f1 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -40,6 +40,7 @@ struct unpack_trees_options {
 		     trivial_merges_only,
 		     verbose_update,
 		     aggressive,
+		     file_level_merge,
 		     skip_unmerged,
 		     initial_checkout,
 		     diff_index_cached,
-- 
1.7.6.15.ga6419