2011-03-10 23:34:09

by David Miller

[permalink] [raw]
Subject: [GIT] Networking


I think all the major known regressions are cured and we should be
ready to go for 2.6.38-final

1) Fix regression in IPV6 route lookups, cures BZ 29252 and 30462

2) We use ifa_address where we mean ifa_local in ipv4 stack,
reported by Julian Anastasov.

3) pktgen time reporting units are wrong, fix from Daniel Turull

4) IPV6=m/BRIDGE=y results in broken build, fix from Randy Dunlap.

5) Network modloading security fix needs to handle ip6 tunnel case,
fix from Stephen Hemminger.

6) bnx2x driver fixes from Dmitry Kravkov and Eilon Greenstein.

7) smsc911x drops full sized VLAN packets erroneously, fix from G?ran
Weinholt.

8) Fix Makefile logic for entering net/ipv6 directory for the case
where we are only building {exthdrs,addrconf}_core.o Fix from
Thomas Graf.

9) Multi-threaded signal handling is botched because we use plain
mutex_lock() to synchronize readers in recvmsg(), change to use
mutex_lock_interruptible(). Fix from Rainer Weikusat.

10) Bonding driver state machine locking doesn't cover enough code,
fix from Nils Carlson.

12) Fix BUG_ON trigger in RDS stack, fix from Neil Horman.

13) Multicase handling fixes to r6040 driver from Shawn Lin.

Please pull, thanks a lot!

The following changes since commit 9179746652faf0aba07b8b7f770dcf29892a24c6:

Merge branch 'media_fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-2.6 (2011-03-10 13:22:10 -0800)

are available in the git repository at:

master.kernel.org:/pub/scm/linux/kernel/git/davem/net-2.6.git master

Daniel Turull (1):
pktgen: fix errata in show results

David S. Miller (3):
ipv4: Fix erroneous uses of ifa_address.
ipv6: Don't create clones of host routes.
Merge branch 'master' of /home/davem/src/GIT/linux-2.6/

Dmitry Kravkov (4):
bnx2x: fix non-pmf device load flow
bnx2x: fix link notification
bnx2x: (NPAR) prevent HW access in D3 state
bnx2x: fix MaxBW configuration

Florian Fainelli (1):
r6040: bump to version 0.27 and date 23Feb2011

G?ran Weinholt (1):
net/smsc911x.c: Set the VLAN1 register to fix VLAN MTU problem

Jon Mason (1):
vxge: update MAINTAINERS

Neil Horman (1):
rds: prevent BUG_ON triggering on congestion map updates

Nicolas Kaiser (1):
drivers/net/macvtap: fix error check

Nils Carlson (2):
bonding 802.3ad: Fix the state machine locking v2
bonding 802.3ad: Rename rx_machine_lock to state_machine_lock

Rainer Weikusat (1):
net: fix multithreaded signal handling in unix recv routines

Randy Dunlap (1):
net: bridge builtin vs. ipv6 modular

Shawn Lin (1):
r6040: fix multicast operations

Thomas Graf (1):
net: Enter net/ipv6/ even if CONFIG_IPV6=n

[email protected] (1):
ariadne: remove redundant NULL check

stephen hemminger (1):
ip6ip6: autoload ip6 tunnel

MAINTAINERS | 5 +-
drivers/net/ariadne.c | 5 --
drivers/net/bnx2x/bnx2x.h | 5 +-
drivers/net/bnx2x/bnx2x_cmn.c | 22 +++++++
drivers/net/bnx2x/bnx2x_cmn.h | 9 +++
drivers/net/bnx2x/bnx2x_ethtool.c | 18 +++---
drivers/net/bnx2x/bnx2x_main.c | 19 +++---
drivers/net/bonding/bond_3ad.c | 32 ++++++----
drivers/net/bonding/bond_3ad.h | 3 +-
drivers/net/macvtap.c | 3 +-
drivers/net/r6040.c | 115 +++++++++++++++++++++----------------
drivers/net/smsc911x.c | 5 ++
net/Makefile | 4 +-
net/bridge/Kconfig | 1 +
net/core/pktgen.c | 2 +-
net/ipv4/devinet.c | 6 +-
net/ipv6/ip6_tunnel.c | 1 +
net/ipv6/route.c | 4 +-
net/rds/ib_send.c | 5 +-
net/rds/loop.c | 11 +++-
net/unix/af_unix.c | 17 ++++-
21 files changed, 182 insertions(+), 110 deletions(-)


2011-03-10 23:50:30

by Linus Torvalds

[permalink] [raw]
Subject: Re: [GIT] Networking

On Thu, Mar 10, 2011 at 3:34 PM, David Miller <[email protected]> wrote:
>
> David S. Miller (3):
> ? ? ?ipv4: Fix erroneous uses of ifa_address.
> ? ? ?ipv6: Don't create clones of host routes.
> ? ? ?Merge branch 'master' of /home/davem/src/GIT/linux-2.6/

David, why do you keep on doing those broken back-merges?

It's the _one_ thing you keep on doing wrong, and I don't understand
it. Just the merge message you get should tell you that you're doing
something total SH*T.

Look at that commit message:

Merge branch 'master' of /home/davem/src/GIT/linux-2.6/

That is literally the WHOLE message. Ask yourself: is that commit
doing anything useful? Does the commit message explain what it is
doing, and why you are doing it?

And if not (and if you came to some other conclusion than "no" to
either of those questions, you should explain it), then why do you do
it? It sure doesn't help me: it just makes it harder for me to
see/follow the history (and if it's harder for me to see, then it's
harder for _others_ to see). There was no conflict (I double-checked),
and there was no explanation for why it would be done.

Now, I admit that it's a git usability bug: for normal "git commit",
git will _force_ you to write a message, and sadly, for merges, I made
it instead just do the message automatically. My bad. I designed it
for the kind of merges I do, where the the automatic merge message
actually tells you what the merge is all about. But for back-merges,
the automatic message is totally worthless, and it is DOUBLY worthless
when you do it the way you do it, namely from some local directory of
your own.

That's just STUPID. Look at that message once more, and ponder. What does

"Merge branch 'master' of /home/davem/src/GIT/linux-2.6/"

tell anybody? It's not even pointing to my repository, and you have
actively BROKEN the small amount of smarts (as admitted above, not
enough) that git does do normally, which is at least tell you where
the merge comes from. You broke it by fetching my repository into your
own anonymous tree, and then merging it from there, and thus the
automatic merge message lost sight of the fact that it was my upstream
tree, because you had made it your own random repo.

Grr. This has been going on for too long. Don't do it. Don't do random
backwards merges without explanations, and with the actual source data
removed.

We're _really_ good at doing commit messages, and the kernel commit
log should be a great example to other projects. But in the last week
or so, I've now _twice_ had to flame core developers for making
totally useless commit messages.

So don't do back-merges. And if you DO do back-merges, don't make the
commit message totally useless.

You can use either 'git commit --amend" to fix the message afterwards
and explain WHY you did the stupid thing, or you can just do "git pull
--no-commit" to not actually commit the merge and then write your
commit message as you do it. But preferably you shouldn't do the
back-merges at all.

Linus

2011-03-10 23:55:20

by David Miller

[permalink] [raw]
Subject: Re: [GIT] Networking

From: Linus Torvalds <[email protected]>
Date: Thu, 10 Mar 2011 15:49:40 -0800

> Grr. This has been going on for too long. Don't do it. Don't do random
> backwards merges without explanations, and with the actual source data
> removed.
>
> We're _really_ good at doing commit messages, and the kernel commit
> log should be a great example to other projects. But in the last week
> or so, I've now _twice_ had to flame core developers for making
> totally useless commit messages.
>
> So don't do back-merges. And if you DO do back-merges, don't make the
> commit message totally useless.
>
> You can use either 'git commit --amend" to fix the message afterwards
> and explain WHY you did the stupid thing, or you can just do "git pull
> --no-commit" to not actually commit the merge and then write your
> commit message as you do it. But preferably you shouldn't do the
> back-merges at all.

I'm sorry about this, won't happen again.

I should have put:

Merge to get commit 8909c9ad8ff03611c9c96c9a92656213e4bb495b
("net: don't allow CAP_NET_ADMIN to load non-netdev kernel modules")
so that we can add Stephen Hemminger's fix to handle ip6 tunnels
as well, which uses the MODULE_ALIAS_NETDEV() macro created by
that change.

Again, sorry.

2011-03-11 00:02:47

by Linus Torvalds

[permalink] [raw]
Subject: Re: [GIT] Networking

On Thu, Mar 10, 2011 at 3:49 PM, Linus Torvalds
<[email protected]> wrote:
>
> Now, I admit that it's a git usability bug: for normal "git commit",
> git will _force_ you to write a message, and sadly, for merges, I made
> it instead just do the message automatically. My bad. I designed it
> for the kind of merges I do, where the the automatic merge message
> actually tells you what the merge is all about.

Btw, the reason I got really upset this time (and I've let it slide
before), is that this time your back-merge not only had that totally
useless merge message (that's happened before), but *because* you did
that back-merge, it also ends up making _my_ merge message totally
useless - the one that normally contains good and useful information
(a valid source of merging, and the abbreviated shortlog of what was
merged).

Why? Because you had done the back-merge very recently, my pull
request then ends up being a fast-forward, so your _useless_ merge
message basically entirely replaces the one that would have been
useful.

Yes, I could do it with "git pull --no-ff", but I have to admit to
hating adding more artificial merges into the tree just to get the
merge information. I didn't think the "--no-ff" flag was a good idea,
and I've never used it so far, but I have to say that now I'm
wavering.

I probably also should just talk to Junio and tell him that the whole
"totally automatic merge messages" was another horrible design
mistake. But if we do fix that in git, I suspect it will (a) break
scripts that expected the automatic silent merge and (b) take a long
time to percolate to people. So it might be one of those "we have to
live with it due to backwards compatibility reasons". Ugh.

Linus

2011-03-11 00:30:14

by Linus Torvalds

[permalink] [raw]
Subject: Re: [GIT] Networking

On Thu, Mar 10, 2011 at 3:55 PM, David Miller <[email protected]> wrote:
> I should have put:
>
> ? ? ? ?Merge to get commit 8909c9ad8ff03611c9c96c9a92656213e4bb495b
> ? ? ? ?("net: don't allow CAP_NET_ADMIN to load non-netdev kernel modules")
> ? ? ? ?so that we can add Stephen Hemminger's fix to handle ip6 tunnels
> ? ? ? ?as well, which uses the MODULE_ALIAS_NETDEV() macro created by
> ? ? ? ?that change.

Yeah, that would have explained it. That said, if you are merging for
something like that, may I suggest actually starting off with

git merge 8909c9ad8ff03611c9c96c9a92656213e4bb495b

that then actually makes the history itself also show the relationship
(you'd still have to write the commit message explaining why,
otherwise git will try to be "helpful" by making the merge commit
message be

Merge commit '8909c9ad8ff03611c9c96c9a92656213e4bb495b'

which while _technically_ more useful and indicative of what you
wanted to do isn't actually any more readable than the one you have
now.

But the reason it would have been better is that it would literally
have made the git commit parenthood point to the commit you actually
care about.

Of course, since 'gitk' ends up highlighting the SHA1 pointers in the
commit message, even just mentioning it in the message is often almost
equivalent to having the parenthood be explicit. But it would show in
the actual history graph too, which I think would have been a good
thing.

Oh well. Water under the bridge. I think I'll try --no-ff this once,
despite my misgivings about the concept.

Linus

[ The reason I don't like "--no-ff" is that it can cause endless "I'm
going to add my own merge message" wars where people want to write
their name in the snow, and then doing cross-merges doesn't ever
actually converge on a single end-result. Fast-forward merges means
that people merging back-and-forth will always end up converging.

But I guess the whole notion of "upstream" vs "downstream" is the
thing that should be considered to be the real way to avoid that, and
if a project cannot agree on what's upstream and what is downstream, I
suspect the project has deeper problems than a few extra merge
messages ;^) ]

2011-03-11 00:33:58

by David Miller

[permalink] [raw]
Subject: Re: [GIT] Networking

From: Linus Torvalds <[email protected]>
Date: Thu, 10 Mar 2011 16:29:30 -0800

> On Thu, Mar 10, 2011 at 3:55 PM, David Miller <[email protected]> wrote:
>> I should have put:
>>
>> ? ? ? ?Merge to get commit 8909c9ad8ff03611c9c96c9a92656213e4bb495b
>> ? ? ? ?("net: don't allow CAP_NET_ADMIN to load non-netdev kernel modules")
>> ? ? ? ?so that we can add Stephen Hemminger's fix to handle ip6 tunnels
>> ? ? ? ?as well, which uses the MODULE_ALIAS_NETDEV() macro created by
>> ? ? ? ?that change.
>
> Yeah, that would have explained it. That said, if you are merging for
> something like that, may I suggest actually starting off with
>
> git merge 8909c9ad8ff03611c9c96c9a92656213e4bb495b
>
> that then actually makes the history itself also show the relationship
> (you'd still have to write the commit message explaining why,
> otherwise git will try to be "helpful" by making the merge commit
> message be
>
> Merge commit '8909c9ad8ff03611c9c96c9a92656213e4bb495b'
>
> which while _technically_ more useful and indicative of what you
> wanted to do isn't actually any more readable than the one you have
> now.
>
> But the reason it would have been better is that it would literally
> have made the git commit parenthood point to the commit you actually
> care about.

So, this is like a cherry-pick of sorts that doesn't create new commits?
It just makes the merge commit, and that's where I explain why I need this
particular change in my tree.

Right?

2011-03-11 00:34:55

by Linus Torvalds

[permalink] [raw]
Subject: Re: [GIT] Networking

On Thu, Mar 10, 2011 at 4:29 PM, Linus Torvalds
<[email protected]> wrote:
>
> Oh well. Water under the bridge. I think I'll try --no-ff this once,
> despite my misgivings about the concept.

Oh wow. That really does end up looking odd. I know some other git
projects use --no-ff, but I don't think we've ever had them in the
kernel, and I've never see the graph look like that before.

But it did allow me to add an explanation for what happened, so maybe
it's worth it.

Linus

2011-03-11 00:40:27

by Dave Airlie

[permalink] [raw]
Subject: Re: [GIT] Networking

On Fri, Mar 11, 2011 at 10:34 AM, Linus Torvalds
<[email protected]> wrote:
> On Thu, Mar 10, 2011 at 4:29 PM, Linus Torvalds
> <[email protected]> wrote:
>>
>> Oh well. Water under the bridge. I think I'll try --no-ff this once,
>> despite my misgivings about the concept.
>
> Oh wow. That really does end up looking odd. I know some other git
> projects use --no-ff, but I don't think we've ever had them in the
> kernel, and I've never see the graph look like that before.
>
> But it did allow me to add an explanation for what happened, so maybe
> it's worth it.

I didn't realise we weren't meant to --no-ff, I've been lately using
--no-ff --log
so I can keep track of what I merged easier, when someone bases something on my
tree and I haven't moved it in a while.

Though I suppose author/committer info should tell me this I've found
having the logs
at least a bit useful.

Dave.

2011-03-11 00:52:28

by Linus Torvalds

[permalink] [raw]
Subject: Re: [GIT] Networking

On Thu, Mar 10, 2011 at 4:34 PM, David Miller <[email protected]> wrote:
>
> So, this is like a cherry-pick of sorts that doesn't create new commits?
> It just makes the merge commit, and that's where I explain why I need this
> particular change in my tree.
>
> Right?

No, it's really just a perfectly regular merge. It's just that instead
of "merge whatever random state that Linus' tree happened to be in, in
order to get that one commit I wanted", it instead does a "merge the
_particular_ history that led up to the one commit I want".

So it's still a back-merge, and in that sense it is technically
absolutely no different from your "merge master" thing. You'll be
merging not just that one commit, but all the parents of the commit
too. So it's not like a cherry-pick, that literally just takes that
one single commit. You do get history, the same way you get with any
merge.

The reason I would suggest doing the "pick explicitly" is just that it
- at least to me - makes it much clearer *what* you actually wanted to
do. It's really the explicitness of "I need that particular commit" vs
"I just want to get whatever Linus has, because he ended up merging a
thing I wanted".

Put another way, I want merging to be something people think about,
and have a clear reason for. Now, for a regular upstream merge, that
"clear reason" is so obvious that we never even discuss it: most of
the merges in the kernel tree have a very simple reason, that simply
goes as "submaintainer asked for his tree to be merged". That's such
an obvious thing that we don't write it out - and it's why I
(stupidly) thought that "git merge" and "git pull" can just generate
the merge messages automatically.

But in other cases, there are less obvious reasons for why a merge
happens, and it's usually a case of a back-merge (ie a submaintainer
merging from upstream, rather than upstream merging from a
submaintainer). Now, there are various good reasons for those kinds of
merges too, and the automated git messages are all usually totally
useless for that case.

The reasons can be any of:

- "I don't want to get too far away from upstream". This is very
understandable, but I have asked people to please _not_ merge "random
trees of the day". Please use major releases for this (or, if worst
comes to worst, -rc releases) rather than just do something else.

- "I need the functionality that went upstream, and I don't want to
duplicate it". This was your case, and again, it's entirely
understandable. It's just that I think it should be explained, so that
people who see the merge also see _why_ the merge was done. And again,
I don't like seeing "merge random tree of the day". There should
really be some explicit thought about _why_ the merge is needed, and
if you merge a tree because you need a particular state, then I think
you should merge _that_ tree, not just "whatever Linus happened to
have right then".

I basically think "merge random tree" is _always_ wrong.

And I do admit that it's a big design mistake in git to not force
people to think about things, and to make it much too easy to generate
useless merge messages. As mentioned, that whole automatic merge
message thing makes a _lot_ more sense to upstream than it does to
downstream: when you're merging from a submaintainer, the implicit
fact that downstream _asked_ you to merge is what makes the automatic
message actually worth something.

Put another way: the same way I think it's wrong when you merge some
random tree from me, I think it would be very wrong for me to merge
your random tree of the day. That would be crazy - who knows what the
heck you have in your tree, and unless you ask me to merge, I should
just assume that it's broken. You may be in the middle of some
development thing, and your tree may be totally broken. I shouldn't
pull it.

The same is true the other way around. Sure, especially late in the
-rc cycle, you can hopefully assume that my tree isn't crap, but even
so, I would really like people to think about _what_ they are merging.
And making it explicit is a good thing, I think.

Linus

2011-03-11 00:57:57

by Linus Torvalds

[permalink] [raw]
Subject: Re: [GIT] Networking

On Thu, Mar 10, 2011 at 4:40 PM, Dave Airlie <[email protected]> wrote:
>
> I didn't realise we weren't meant to --no-ff, I've been lately using
> --no-ff --log
> so I can keep track of what I merged easier, when someone bases something on my
> tree and I haven't moved it in a while.

Well, the whole "--no-ff --log" is really designed for exactly that.
It's the "I'm the upstream maintainer, and I want that nice merge
summary". And it really is useful for that.

So I don't know about "weren't meant to". It very much is why those
flags exist (I don't personally use "--log", but that's because since
I'm the very top maintainer I've just set the flag in my .git/config
flag that makes _all_ my merges logging merges).

It's just that when I started doing git, the fast-forward merge was
very much a design decision. But I have to admit that I really like my
merge summaries too, and the main reason I have never really
considered using --no-ff is that _my_ tree moves so often that
practically speaking, I never fast-forward. If I get a fast-forward
merge, it almost always means that some submaintainer did something
iffy (like rebase his tree), and that the commits from that
submaintainer haven't been around for very long.

So no, I don't think you've been wrong in using "--no-ff --log". It
does make sense.

I just have my own private hang-ups about why I'm not personally a
huge fan of --no-ff.

But now that I've done it once, maybe I'm hooked. It's like coke to
Charlie Sheen.

Linus

2011-03-11 03:34:11

by Steven Rostedt

[permalink] [raw]
Subject: Re: [GIT] Networking

On Thu, Mar 10, 2011 at 04:51:34PM -0800, Linus Torvalds wrote:
>
> The reasons can be any of:
>
> - "I don't want to get too far away from upstream". This is very
> understandable, but I have asked people to please _not_ merge "random
> trees of the day". Please use major releases for this (or, if worst
> comes to worst, -rc releases) rather than just do something else.
>

What are you throughts on starting work from a tree. Most of my work is
usually based off of some branch in tip, but sometimes when I'm pulling
in patches that are not really related to anything, I just simply grab
whatever the latest Linus branch is and start from there.

Is it preferable to instead start from one of the official releases?

-- Steve

2011-03-11 06:37:06

by Ingo Molnar

[permalink] [raw]
Subject: Re: [GIT] Networking


* Steven Rostedt <[email protected]> wrote:

> On Thu, Mar 10, 2011 at 04:51:34PM -0800, Linus Torvalds wrote:
> >
> > The reasons can be any of:
> >
> > - "I don't want to get too far away from upstream". This is very
> > understandable, but I have asked people to please _not_ merge "random
> > trees of the day". Please use major releases for this (or, if worst
> > comes to worst, -rc releases) rather than just do something else.
>
> What are you throughts on starting work from a tree. Most of my work is usually
> based off of some branch in tip, but sometimes when I'm pulling in patches that
> are not really related to anything, I just simply grab whatever the latest Linus
> branch is and start from there.
>
> Is it preferable to instead start from one of the official releases?

Yes, for clarity of merge history i'm generally asking all people who send pull
requests to -tip to use official -rc's as bases (or -tip branches), *not* some
random daily -git snapshot. If a -git snapshot has to be merged for a good reason
then please amend the merge commit with the Merge-Reason tag describing the good
reason you had.

Thanks,

Ingo

2011-03-11 19:54:48

by Greg KH

[permalink] [raw]
Subject: Re: [GIT] Networking

On Thu, Mar 10, 2011 at 04:57:04PM -0800, Linus Torvalds wrote:
> But now that I've done it once, maybe I'm hooked. It's like coke to
> Charlie Sheen.

Did you ever push this networking pull out? I don't seem to be seeing
it in your tree, but the --no-ff might have caused me to miss it.

thanks,

greg k-h

2011-03-11 20:48:28

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [GIT] Networking

On Thu, Mar 10, 2011 at 04:29:30PM -0800, Linus Torvalds wrote:
> On Thu, Mar 10, 2011 at 3:55 PM, David Miller <[email protected]> wrote:
> > I should have put:
> >
> >        Merge to get commit 8909c9ad8ff03611c9c96c9a92656213e4bb495b
> >        ("net: don't allow CAP_NET_ADMIN to load non-netdev kernel modules")
> >        so that we can add Stephen Hemminger's fix to handle ip6 tunnels
> >        as well, which uses the MODULE_ALIAS_NETDEV() macro created by
> >        that change.
>
> Yeah, that would have explained it. That said, if you are merging for
> something like that, may I suggest actually starting off with
>
> git merge 8909c9ad8ff03611c9c96c9a92656213e4bb495b
>
> that then actually makes the history itself also show the relationship
> (you'd still have to write the commit message explaining why,

By the way, I occasionally wonder whether it would make sense to make a
habit of committing bugfixes on top of the commit that introduced the
bug (at least in cases where there *is* a single commit that introduced
the bug).

As with the above, it'd make the history a little more self-documenting.
It might simplify life for backporters. (In theory, they could do
merges instead of a cherry-picks if they wanted to.) The set of "bad"
commits would be described by "fix^...fix".

But then I had some mental image if your saying "WTF?" the first time I
send you a post-rc1 pull request that looks like an octopus merge of a
dozen little 1- or 2- commit branches based all over the place.

I dunno, would it be annoying?

--b.

2011-03-11 21:00:53

by David Miller

[permalink] [raw]
Subject: Re: [GIT] Networking

From: "J. Bruce Fields" <[email protected]>
Date: Fri, 11 Mar 2011 15:48:23 -0500

> As with the above, it'd make the history a little more self-documenting.

Once there is even one single commit after the buggy one, this is
simply impossible since the hashes of subsequent commits depend upon
the precise contents of the original one.

2011-03-11 21:17:59

by Ben Hutchings

[permalink] [raw]
Subject: Re: [GIT] Networking

On Fri, 2011-03-11 at 13:01 -0800, David Miller wrote:
> From: "J. Bruce Fields" <[email protected]>
> Date: Fri, 11 Mar 2011 15:48:23 -0500
>
> > As with the above, it'd make the history a little more self-documenting.
>
> Once there is even one single commit after the buggy one, this is
> simply impossible since the hashes of subsequent commits depend upon
> the precise contents of the original one.

I think Bruce is suggesting that the fix is committed on a branch from
the broken commit, then merged into whatever branches need it. I'm not
sure how much that helps, though. The merge could then involve forward-
porting (as opposed to the current situation where fixes are cherry-
picked and possibly back-ported).

Ben.

--
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

2011-03-11 21:42:14

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [GIT] Networking

On Fri, Mar 11, 2011 at 09:17:54PM +0000, Ben Hutchings wrote:
> On Fri, 2011-03-11 at 13:01 -0800, David Miller wrote:
> > From: "J. Bruce Fields" <[email protected]>
> > Date: Fri, 11 Mar 2011 15:48:23 -0500
> >
> > > As with the above, it'd make the history a little more self-documenting.
> >
> > Once there is even one single commit after the buggy one, this is
> > simply impossible since the hashes of subsequent commits depend upon
> > the precise contents of the original one.
>
> I think Bruce is suggesting that the fix is committed on a branch from
> the broken commit, then merged into whatever branches need it. I'm not
> sure how much that helps, though. The merge could then involve forward-
> porting (as opposed to the current situation where fixes are cherry-
> picked and possibly back-ported).

Yeah, there could be merge conflicts.

But assume you only did this in cases where the merge was trivial.
Would it be worth it, or would people be annoyed by the additional
branching?

--b.

2011-03-12 00:29:44

by Junio C Hamano

[permalink] [raw]
Subject: Re: [GIT] Networking

Linus Torvalds <[email protected]> writes:

> On Thu, Mar 10, 2011 at 3:34 PM, David Miller <[email protected]> wrote:
>> ...
> Look at that commit message:
>
> Merge branch 'master' of /home/davem/src/GIT/linux-2.6/
>
> That is literally the WHOLE message. Ask yourself: is that commit
> doing anything useful? Does the commit message explain what it is
> doing, and why you are doing it?
> ...
> Now, I admit that it's a git usability bug: for normal "git commit",
> git will _force_ you to write a message, and sadly, for merges, I made
> it instead just do the message automatically. My bad. I designed it
> for the kind of merges I do, where the the automatic merge message
> actually tells you what the merge is all about. But for back-merges,
> the automatic message is totally worthless, and it is DOUBLY worthless
> when you do it the way you do it, namely from some local directory of
> your own.

I admit that I back-merged a few times my own master to a largish topic
branch, when updates that happened on the master front since the topic
forked from it helped to clean up the topic. When I did so, I knew better
to say "git commit --amend" to reword the merge message to say something
like:

Merge 'master' to 'jc/frotz' for xyzzy feature

so it wasn't a huge problem for me personally to keep the history useful,
but I agree that it would be better to make it harder for mortals to just
backmerge without doing the rewording.

The question is how. Perhaps when the merge is made from the default
upstream, i.e. with "git pull" (no parameters) or "git merge @{u}", we
should automatically give the user an editor?

2011-03-12 04:09:48

by Steven Rostedt

[permalink] [raw]
Subject: Re: [GIT] Networking

On Fri, Mar 11, 2011 at 04:42:09PM -0500, J. Bruce Fields wrote:
>
> But assume you only did this in cases where the merge was trivial.
> Would it be worth it, or would people be annoyed by the additional
> branching?
>

I would be annoyed by it ;)

It really is meaningless to do so, as all you are doing is documenting
what commit caused this bug, and producing more problems by branching
off of the broken commit. It wont matter till it is merged, but then if
there are a lot of simple bug fixes, then you will have a lot of single
merges of branches that fix those bugs.

It's just better to say in the change log of the fix:

"commit abcdef1234 foo: add bar"

Broke this, and this fixes it.

Having the broken commit SHA1 and description in the change log is just
as helpful as having the bug commit being its parent.

-- Steve

2011-03-15 07:20:54

by David Lang

[permalink] [raw]
Subject: Re: [GIT] Networking

On Fri, 11 Mar 2011, Steven Rostedt wrote:

> On Fri, Mar 11, 2011 at 04:42:09PM -0500, J. Bruce Fields wrote:
>>
>> But assume you only did this in cases where the merge was trivial.
>> Would it be worth it, or would people be annoyed by the additional
>> branching?
>>
>
> I would be annoyed by it ;)
>
> It really is meaningless to do so, as all you are doing is documenting
> what commit caused this bug, and producing more problems by branching
> off of the broken commit. It wont matter till it is merged, but then if
> there are a lot of simple bug fixes, then you will have a lot of single
> merges of branches that fix those bugs.

what effect would this have on bisecting? if this helped people avoid
bisecting in between the bad commit and the fix for it, it may be worth
it.

David Lang

> It's just better to say in the change log of the fix:
>
> "commit abcdef1234 foo: add bar"
>
> Broke this, and this fixes it.
>
> Having the broken commit SHA1 and description in the change log is just
> as helpful as having the bug commit being its parent.
>
> -- Steve
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>

2011-03-15 12:52:44

by Steven Rostedt

[permalink] [raw]
Subject: Re: [GIT] Networking

On Tue, 2011-03-15 at 00:20 -0700, [email protected] wrote:

> > It really is meaningless to do so, as all you are doing is documenting
> > what commit caused this bug, and producing more problems by branching
> > off of the broken commit. It wont matter till it is merged, but then if
> > there are a lot of simple bug fixes, then you will have a lot of single
> > merges of branches that fix those bugs.
>
> what effect would this have on bisecting? if this helped people avoid
> bisecting in between the bad commit and the fix for it, it may be worth
> it.
>

But it wont, as the fix will still be brought in at a later time. It has
nothing to do with being based off of the broken commit.

A +
+ merged in fix
B + \
| |
| |
| |
Lots of | |
stuff | |
| |
| |
| + - fix for bug
| /
Bug commit +
|

A bisect will still be testing lots of stuff without that fix. And if it
goes into the branch with the fix, we just brought the kernel way back
in time from point A. Then if it goes back to point B, then we zoom back
to the future and bounce the kernel all over the place.

I see no gain for having a fixed based off of the bug that it fixes.

Pros of doing this:

1) documents the point that things broke (can be done by commenting it
in the change log too)

2) probably good for back porting (but Con 3 may out weigh this)

Cons:

1) Adds many more branches and merges for no real good reason

2) Makes bisects even less linear than it already is

3) May cause more conflicts at the merge point as the broken code may
have changed.



Who will be doing the conflict resolutions? Linus? I doubt he would be
happy with that, but he can speak for himself.

-- Steve

2011-03-15 14:37:16

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [GIT] Networking

On Tue, Mar 15, 2011 at 08:52:40AM -0400, Steven Rostedt wrote:
> But it wont, as the fix will still be brought in at a later time. It has
> nothing to do with being based off of the broken commit.
>
> A +
> + merged in fix
> B + \
> | |
> | |
> | |
> Lots of | |
> stuff | |
> | |
> | |
> | + - fix for bug
> | /
> Bug commit +
> |
>
> A bisect will still be testing lots of stuff without that fix. And if it
> goes into the branch with the fix, we just brought the kernel way back
> in time from point A. Then if it goes back to point B, then we zoom back
> to the future and bounce the kernel all over the place.
>
> I see no gain for having a fixed based off of the bug that it fixes.

I suppose you'd test the intermediate ("bad") area by merging in "fix
for bug" instead of cherry-picking it. In theory perhaps that would
give the bisect algorithm a little more information. (Since it's seeing
the same "fix for bug" commit each time.)

> Pros of doing this:
>
> 1) documents the point that things broke (can be done by commenting it
> in the change log too)
>
> 2) probably good for back porting (but Con 3 may out weigh this)
>
> Cons:
>
> 1) Adds many more branches and merges for no real good reason
>
> 2) Makes bisects even less linear than it already is
>
> 3) May cause more conflicts at the merge point as the broken code may
> have changed.
>
>
>
> Who will be doing the conflict resolutions? Linus? I doubt he would be
> happy with that, but he can speak for himself.

No real change there: you still won't want to send in a pull request
every time you fix a bug, so you'd pull a bunch together, merge them
(and maybe a test merge with upstream to make sure it's reasonable),
then send a pull request for the result.

I dunno, I have no strong opinions here, just curiosity.

--b.

2011-03-15 15:43:33

by Linus Torvalds

[permalink] [raw]
Subject: Re: [GIT] Networking

On Tue, Mar 15, 2011 at 12:20 AM, <[email protected]> wrote:
>
> what effect would this have on bisecting? if this helped people avoid
> bisecting in between the bad commit and the fix for it, it may be worth it.

The biggest impact on bisecting would be to just make it slower due to
extra merge commits. It's probably not a hugely noticeable thing,
though - we have merges for lots of other reasons.

Merges make bisecting a bit less efficient not only because they are
an extra commit, but because merges - by not being linear - end up
making it harder to find the "perfect" half-way point. IOW, the
bisection algorithm may not be able to pick a commit exactly midway,
because if it picks a merge it will obviously include both branches
(which may be "too much"), but if it picks the last commit of either
branch it may be too little.

And not picking a good half-way point makes bisection less efficient
(it's no longer "log2()", but maybe "log1.7()").

In practice, though, I suspect it's not really a big issue.

The other issue with bisecting merges is obviously that it can be
really nasty to figure out a bug that is introduced by the merge
itself (which is usually due to some interaction between the things
being merged, rather than any real mis-merge itself, although
mis-merges can obviously happen).

Again, that's pretty rare, although it does happen (and is often quite
"interesting" when it happens).

But in practice, I think the biggest argument against trying to fix
things as patches on top of the original bug and then merging it in is
that it's just extra work for fairly small gain. In particular:

- not all bugs get bisected. In fact, it's the minority - most
bug-fixes are "obvious", and no-one even bothers bisecting them

- not all bugs are as clearly the result of a single commit. Many are
a commit series. Sure, you'd just make it the last one in the series,
but it would kind of defeat the "parent introduced the bug" point of
it all.

- putting the "commit xyz caused this problem" in the changelog (and
then just follow the chain that way) isn't really all that much worse.
The reason I talked about doing this for _merges_ is that I think
developers should think a lot more about merging, but I don't think
people need to think a lot about "let's fix this obvious bug".

- bugfixes are often sent out as emails etc. Just applying the
bug-fix would be a _lot_ more work if it involved "create new branch
at original bug, apply, merge" than just "apply". That's especially
true for series of patches, like the ones I get from Andrew and Al.

In particular, that last thing is a killer for me personally. It
(together with the first point) would mean that it would be done only
for a very small subset of bugs, which really defeats the whole
purpose, and then makes that "merge it on top of its parent" thing be
the unusual case rather than some general rule.

And merges do end up being more complicated than plain patches. Not
just in the "extra commit" sense, but also simply in the sense of
making the history more complex.

They do have advantages too. We could just merge the same branch into
multiple branches, for example (-stable comes to mind). That said,
once more, it would really complicate things for everybody (no longer
can the stable people just apply a series of patches), so...

So on the whole, I don't think it would be worth it. I don't think
it's _wrong_, mind you, I just think that the way we work, it's more
pain than gain.

For doing a backwards merge, I think the "more pain" is a _good_ thing
(it really shouldn't _ever_ be a mindless "let's merge whatever
upstream is today" and I definitely made that mindless action too easy
to do in git). For most bug-fixes, more pain is just that - more pain.

Linus