Re: linux-next: unneeded merge in the security tree

Re: linux-next: unneeded merge in the security tree

[Linus Torvalds]

[ Added Junio and git to the recipients, and leaving a lot of stuff
quoted due to that... ]

On Mon, Mar 11, 2013 at 9:16 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Tue, Mar 12, 2013 at 03:10:53PM +1100, James Morris wrote:
>> On Tue, 12 Mar 2013, Stephen Rothwell wrote:
>> > The top commit in the security tree today is a merge of v3.9-rc2.  This
>> > is a completely unnecessary merge as the tree before the merge was a
>> > subset of v3.9-rc1 and so if the merge had been done using anything but
>> > the tag, it would have just been a fast forward.  I know that this is now
>> > deliberate behaviour on git's behalf, but isn't there some way we can
>> > make this easier on maintainers who are just really just trying to pick a
>> > new starting point for their trees after a release?  (at least I assume
>> > that is what James was trying to do)
>>
>> Yes, and I was merging to a tag as required by Linus.

Now, quite frankly, I'd prefer people not merge -rc tags either, just
real releases. -rc tags are certainly *much* better than merging
random daily stuff, but the basic rule should be "don't back-merge AT
ALL" rather than "back-merge tags".

That said, you didn't really want a merge at all, you just wanted to
sync up and start development. Which is different (but should still
prefer real releases, and only use rc tags if it's fixing stuff that
happened in the merge window - which may be the case here).

> Why not just force the head of the security tree to be v3.9-rc2?  Then
> you don't end up creating a completely unnecessary merge commit, and
> users who were at the previous head of the security tree will
> experience a fast forward when they pull your new head.

So I think that may *technically* be the right solution, but it's a
rather annoying UI issue, partly because you can't just do it in a
single operation (you can't do a pull of the tag to both fetch and
fast-forward it), but partly because "git reset --hard" is also an
operation that can lose history, so it's something that people should
be nervous about, and shouldn't use as some kind of standard "let's
just fast-forward to Linus' tree" thing.

At the same time, it's absolutely true that when *I* pull a signed tag
from a downstream developer, I don't want a fast-forward, because then
I'd lose the signature. So when a maintainer pulls a submaintainer
tree, you want the signature to come upstream, but when a
submaintainer wants to just sync up with upstream, you don't want to
generate the pointless signed merge commit, because the signature is
already upstream because it's a public tag. So gthe behavior of "git
pull" is fundamentally ambiguous.

But git doesn't know the difference between "official public upstream
tag" and "signed tag used to verify the pull request".

I'm adding the git list just to get this issue out there and see if
people have any ideas. I've got a couple of workarounds, but they
aren't wonderful..

One is simple:

    git config alias.sync="pull --ff-only"

which works fine, but forces submaintainers to be careful when doing
things like this, and using a special command to do back-merges.

And maybe that's the right thing to do? Back-merges *are* special,
after all. But the above alias is particularly fragile, in that
there's both "pull" and "merge" that people want to use this for, and
it doesn't really handle both. And --ff-only will obviously fail if
you actually have some work in your tree, and want to do a real merge,
so then you have to do that differently. So I'm mentioning this as a
better model than "git reset", but not really a *solution*.

That said, the fact that "--ff-only" errors out if you have local
development may actually be a big bonus - because you really shouldn't
do merges at all if you have local development, but syncing up to my
tree if you don't have it (and are going to start it) may be something
reasonable.

Now, the other approach - and perhaps preferable, but requiring actual
changes to git itself - is to do the non-fast-forward merge *only* for
FETCH_HEAD, which already has magic semantics in other ways. So if
somebody does

    git fetch linus
    git merge v3.8

to sync with me, they would *not* get a merge commit with a signature,
just a fast-forward. But if you do

    git pull linus v3.8

or a

    git fetch linus v3.8
    git merge FETCH_HEAD

it would look like a "maintainer merge" and stash the signature in the
merge commit rather than fast-forward. It would probably work in
practice.

The final approach might be to make it like the "merge summary" and
simply make it configurable _and_ have a command line flag for it,
defaulting to our current behavior or to the above suggested "default
on for FETCH_HEAD, off for anything else".

Hmm?

            Linus

Re: linux-next: unneeded merge in the security tree

[Geert Uytterhoeven]

On Tue, Mar 12, 2013 at 6:13 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>> Why not just force the head of the security tree to be v3.9-rc2?  Then
>> you don't end up creating a completely unnecessary merge commit, and
>> users who were at the previous head of the security tree will
>> experience a fast forward when they pull your new head.
>
> So I think that may *technically* be the right solution, but it's a
> rather annoying UI issue, partly because you can't just do it in a
> single operation (you can't do a pull of the tag to both fetch and
> fast-forward it), but partly because "git reset --hard" is also an
> operation that can lose history, so it's something that people should
> be nervous about, and shouldn't use as some kind of standard "let's
> just fast-forward to Linus' tree" thing.

In many cases, "git rebase x" does the exact same thing as
"git reset --hard x", with an added safeguard: if you forgot to upstream
something, it'll boil up on top of "x".

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: linux-next: unneeded merge in the security tree

[Junio C Hamano]

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

> One is simple:
>
>     git config alias.sync="pull --ff-only"

Heh, I just wrote that myself after reading the early part of this
message ;-)

> which works fine, but forces submaintainers to be careful when doing
> things like this, and using a special command to do back-merges.

> And maybe that's the right thing to do? Back-merges *are* special,

Yes.

> after all. But the above alias is particularly fragile, in that
> there's both "pull" and "merge" that people want to use this for, and
> it doesn't really handle both. And --ff-only will obviously fail if
> you actually have some work in your tree, and want to do a real merge,
> so then you have to do that differently. So I'm mentioning this as a
> better model than "git reset", but not really a *solution*.

> That said, the fact that "--ff-only" errors out if you have local
> development may actually be a big bonus - because you really shouldn't
> do merges at all if you have local development, but syncing up to my
> tree if you don't have it (and are going to start it) may be something
> reasonable.

Yes, that's the reasoning behind all the behaviours you described
above.

> Now, the other approach - and perhaps preferable, but requiring actual
> changes to git itself - is to do the non-fast-forward merge *only* for
> FETCH_HEAD, which already has magic semantics in other ways. So if
> somebody does
>
>     git fetch linus
>     git merge v3.8
>
> to sync with me, they would *not* get a merge commit with a signature,
> just a fast-forward. But if you do
>
>     git pull linus v3.8
>
> or a
>
>     git fetch linus v3.8
>     git merge FETCH_HEAD
>
> it would look like a "maintainer merge"....

I am not sure I follow.  Are you solving the real problem, the
pointeless merge in the "security tree" that started this thread?

I would imagine it was made by somebody thinking that pulling a
tagged stable point from you is a good idea, like this:

	git pull linus v3.9-rc2

which under your FETCH_HEAD rule would look like a maintainer merge,
no?

An alternative may be to activate the magic "mergetag" thing only
when you give "--no-ff" explicitly; otherwise merge would unwrap the
tag, whether it comes from FETCH_HEAD.

The following examples all assume that your HEAD is somewhere
behind v3.9-rc2, perhaps done by

	git checkout -b test v3.8^0

Then under the "--no-ff activates the magic" rule:

	git merge v3.9-rc2

will fast-forward, but this

	git merge --no-ff v3.9-rc2

creates a real merge with the "mergetag" signature block.  The one
that caused trouble in the "security tree", i.e.

        git pull linus v3.9-rc2

or its equivalent

        git fetch linus v3.9-rc2
        git merge FETCH_HEAD

would still fast-forward under this rule.  The maintainer needs to
do

	git pull --no-ff git://git.kernel.org/... for-linus

if the pull could fast-forward under this rule, though.

Having thought this up to this point, I am not sure it generally is
a good change.  It feels that "pull --ff-only" that prevents people
from creating pointless back-merges may still be a better mechanism.

I dunno.

Re: linux-next: unneeded merge in the security tree

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Then under the "--no-ff activates the magic" rule:
>
> 	git merge v3.9-rc2
>
> will fast-forward, but this
>
> 	git merge --no-ff v3.9-rc2
>
> creates a real merge with the "mergetag" signature block.  The one
> that caused trouble in the "security tree", i.e.
>
>         git pull linus v3.9-rc2
>
> or its equivalent
>
>         git fetch linus v3.9-rc2
>         git merge FETCH_HEAD
>
> would still fast-forward under this rule.  The maintainer needs to
> do
>
> 	git pull --no-ff git://git.kernel.org/... for-linus
>
> if the pull could fast-forward under this rule, though.

Scratch the last sentence.  It should have been

"whether the pull fast-forwards or not".  You'd always need to.

Re: linux-next: unneeded merge in the security tree

[Theodore Ts'o]

What if we added the ability to do something like this:

[remote "origin"]
	url = git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
	fetch = +refs/heads/master:refs/heads/master
	mergeoptions = --ff-only

This would be an analog to branch.<name>.mergeoptions, but it would
apply to the source of the pull request, instead of the destination.

That way, people who do a "git pull" from Linus's tree would get the
protection of --ff-only, while pulls from submaintainer trees would
automatically get a merge commit, which is what we want.

It doesn't handle the case of a submaintainer pulling from a
maintainer in a back-merge scenario, but that should be a pretty rare
case, so maybe that's OK.

      	     	       	   	- Ted

Re: linux-next: unneeded merge in the security tree

[Linus Torvalds]

On Tue, Mar 12, 2013 at 2:20 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> What if we added the ability to do something like this:
>
> [remote "origin"]
>         url = git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
>         fetch = +refs/heads/master:refs/heads/master
>         mergeoptions = --ff-only

Hmm. Something like this could be interesting for other things:

 - use "--rebase" when pulling (this is common for people who maintain
a set of patches and do *not* export their git tree - I use it for
projects like git and subsurface where there is an upstream maintainer
and I usually send patches by email rather than git)

 - "--no-summary". As a maintainer, you people probably do want to
enable summaries for people they pull from, but *not* from upstream.
So this might even make sense to do by default when you clone a new
repository.

 - I do think that we might want a "--no-signatures" for the specific
case of merging signed tags without actually taking the signature
(because it's a "upstream" repo). The "--ff-only" thing is *too*
strict. Sometimes you really do want to merge in new code, disallowing
it entirely is tough.

Of course, I'm not really sure if we want to list the flags. Maybe
it's better to just introduce the notion of "upstream" directly, and
make that a flag, and make "origin" default to that when you clone.
And then have git use different heurstics for pulling upstream (like
warning by default when doing a back-merge, perhaps?)

                   Linus

Re: linux-next: unneeded merge in the security tree

[Junio C Hamano]

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

>  - I do think that we might want a "--no-signatures" for the specific
> case of merging signed tags without actually taking the signature
> (because it's a "upstream" repo). The "--ff-only" thing is *too*
> strict. Sometimes you really do want to merge in new code, disallowing
> it entirely is tough.

I agree that "--ff-only" thing is too strict and sometimes you would
want to allow back-merges, but when you do allow such a back-merge,
is there a reason you want it to be --no-signatures merge?  When a
subtree maintainer decides to merge a stable release point from you
with a good reason, I do not see anything wrong in recording that
the resulting commit _did_ merge what you released with a signature.

Re: linux-next: unneeded merge in the security tree

[Linus Torvalds]

On Tue, Mar 12, 2013 at 2:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> I agree that "--ff-only" thing is too strict and sometimes you would
> want to allow back-merges, but when you do allow such a back-merge,
> is there a reason you want it to be --no-signatures merge?  When a
> subtree maintainer decides to merge a stable release point from you
> with a good reason, I do not see anything wrong in recording that
> the resulting commit _did_ merge what you released with a signature.

No, there's nothing really bad with adding the signature to the merge
commit if you do make a merge. It's the fact that it currently makes a
non-ff merge when that is pointless that hurts.

That said, adding the signature from an upstream tag doesn't really
seem to be hugely useful. I'm not seeing much of an upside, in other
words. I'd *expect* that people would pick up upstream tags
regardless, no?

           Linus

Re: linux-next: unneeded merge in the security tree

[Junio C Hamano]

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

> That said, adding the signature from an upstream tag doesn't really
> seem to be hugely useful. I'm not seeing much of an upside, in other
> words. I'd *expect* that people would pick up upstream tags
> regardless, no?

Yes, their "git fetch" will auto-follow, but mergetag embedded in
the commit objects will give the history auditable trail the same
way as the merges you make your downstream.  You of course could
match out-of-line tags against back-merges and verify your merges
with mergetags, but you do not have to.

Re: linux-next: unneeded merge in the security tree

[Theodore Ts'o]

On Tue, Mar 12, 2013 at 02:30:04PM -0700, Junio C Hamano wrote:
> Theodore Ts'o <tytso@mit.edu> writes:
> 
> > [remote "origin"]
> > 	url = git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
> > 	fetch = +refs/heads/master:refs/heads/master
> > 	mergeoptions = --ff-only
> >
> 
> Is there an escape hatch for that rare case?  IOW, how does a
> submaintainer who configured the above to override --ff-only?

Hmm, maybe we would need to add a --no-ff-only?  Or they could just
do:

	git fetch origin
	git merge FETCH_HEAD

On Tue, Mar 12, 2013 at 02:28:39PM -0700, Linus Torvalds wrote:
>
> Of course, I'm not really sure if we want to list the flags. Maybe
> it's better to just introduce the notion of "upstream" directly, and
> make that a flag, and make "origin" default to that when you clone.
> And then have git use different heurstics for pulling upstream (like
> warning by default when doing a back-merge, perhaps?)

What if git automaticallly set up the origin branch to have a certain
set of mergeoptions by default?  That would probably be right for most
users, but it makes it obvious what's going on when they take a look
at the .git/config file, and doesn't make the remote that happens to
have the name "origin" as having certain magic properties.  Using a
set of mergeoptions would also be bit more general, and might have
applications in the future.

     	      	       	  	 	       - Ted

Re: linux-next: unneeded merge in the security tree

[Junio C Hamano]

Theodore Ts'o <tytso@mit.edu> writes:

> On Tue, Mar 12, 2013 at 02:30:04PM -0700, Junio C Hamano wrote:
>> Theodore Ts'o <tytso@mit.edu> writes:
>> 
>> > [remote "origin"]
>> > 	url = git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
>> > 	fetch = +refs/heads/master:refs/heads/master
>> > 	mergeoptions = --ff-only
>> >
>> 
>> Is there an escape hatch for that rare case?  IOW, how does a
>> submaintainer who configured the above to override --ff-only?
>
> Hmm, maybe we would need to add a --no-ff-only?  Or they could just
> do:
>
> 	git fetch origin
> 	git merge FETCH_HEAD

Hmm, neither feel quite nice.

I haven't heard any comments on my alternative proposal, by the
way.  Did the message get lost?

Re: linux-next: unneeded merge in the security tree

[Junio C Hamano]

Theodore Ts'o <tytso@mit.edu> writes:

> What if we added the ability to do something like this:
>
> [remote "origin"]
> 	url = git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
> 	fetch = +refs/heads/master:refs/heads/master
> 	mergeoptions = --ff-only
>
> This would be an analog to branch.<name>.mergeoptions, but it would
> apply to the source of the pull request, instead of the destination.
>
> That way, people who do a "git pull" from Linus's tree would get the
> protection of --ff-only, while pulls from submaintainer trees would
> automatically get a merge commit, which is what we want.
>
> It doesn't handle the case of a submaintainer pulling from a
> maintainer in a back-merge scenario, but that should be a pretty rare
> case, so maybe that's OK.

Is there an escape hatch for that rare case?  IOW, how does a
submaintainer who configured the above to override --ff-only?