Regulator updates for 3.3

Regulator updates for 3.3

[Mark Brown]

Please pull from the tree below to merge regulator API updates for
version 3.3  The major update here is the addition of device tree
support by Rajendra Nayak, and we also have a new driver for the
regulators on the DA9052 PMIC.  Otherwise most of the changes are
bug fixes and cleanups of one form or another.

The following changes since commit caca6a03d365883564885f2c1da3e88dcf65d139:

  Linux 3.2-rc3 (2011-11-23 20:20:28 -0800)

are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-linus

Ashish Jangam (2):
      regulator: DA9052/53 Regulator support
      regulator: Replace kzalloc with devm_kzalloc and if-else with a switch-case for da9052-regulator

Axel Lin (10):
      regulator: export of_get_regulator_init_data
      regulator: Checking return value of of_get_regulator_init_data
      regulator: Staticise of_get_fixed_voltage_config()
      regulator: Avoid potential NULL dereference in reg_fixed_voltage_probe()
      regulator: aat2870: Fix the logic of checking if no id is matched in aat2870_get_regulator
      regulator: aat2870: Remove a redundant bitwise and operation
      regulator: Convert virtual and userspace regulator consumer drivers to use module_platform_driver()
      regulator: da9052: Staticize non-exported symbols
      regulator: Fix the error handling if create_regulator fails
      regulator: Fix checking return value of create_regulator

Donggeun Kim (1):
      regulator: add regulator_bulk_force_disable function

Fabio Estevam (2):
      regulator: mc13783: Convert to devm_kzalloc()
      regulator: mc13892: Convert to devm_kzalloc()

Jonghwan Choi (2):
      regulator: Use regmap_read/write(), regmap_update_bits functions directly
      regulator: max8649 Convert max8649 to use regmap api

Kevin Liu (1):
      regulator: max8925: fix enabled/disabled judgement mistake

Kyle Manna (2):
      regulator: TPS65910: Move regulator defs to header
      regulator: TPS65910: Create an array for init data

Laxman Dewangan (1):
      regulator: Enable supply regulator if child rail is enabled.

Lothar Waßmann (1):
      regulator: fix use after free bug

Mark Brown (16):
      regulator: Don't report zero volts for the fixed voltage regulator
      regulator: Don't create voltage sysfs entries if we can't read voltage
      Merge branch 'for-linus' of git://git.kernel.org/.../broonie/regulator into regulator-next
      Merge remote-tracking branch 'regulator/topic/dt' into regulator-next
      Merge remote-tracking branch 'regulator/topic/dt' into regulator-next
      Merge remote-tracking branch 'regulator/topic/dt' into regulator-next
      Merge remote-tracking branch 'regulator/topic/dt' into regulator-next
      Merge remote-tracking branch 'regulator/for-linus' into regulator-next
      Merge remote-tracking branch 'regulator/for-linus' into regulator-next
      Merge remote-tracking branches 'regulator/topic/dt' and 'regulator/for-linus' into regulator-next
      regulator: Allow regulators to register with no init_data
      regulator: If a single voltage is set with device tree then set apply_uV
      Merge branch 'topic/dt' of git://git.kernel.org/.../broonie/regulator into regulator-next
      regulator: Update da9052-regulator for DT changes
      regulator: Convert wm831x regulator drivers to devm_kzalloc()
      regulator: Export regulator_is_supported_voltage()

MyungJoo Ham (1):
      regulator: add regulator_force_disable() definition for !CONFIG_REGULATOR

Rajendra Nayak (6):
      regulator: helper routine to extract regulator_init_data
      regulator: adapt fixed regulator driver to dt
      regulator: pass additional of_node to regulator_register()
      regulator: map consumer regulator based on device tree
      regulator: Fix compile break due to missing arguments to regulator_register
      regulator: Fix regulator_register() API signature in Documentation

Richard Zhao (2):
      regulator: pass regulator_register of_node in fixed voltage driver
      regulator: set constraints.apply_uV to 0 in of_get_fixed_voltage_config

Shawn Guo (4):
      regulator: fix label names used in device tree bindings
      regulator: pass device_node to of_get_regulator_init_data()
      regulator: mc13892: remove the unnecessary prefix from regulator name
      regulator: mc13892: add device tree probe support

Tero Kristo (1):
      regulator: twl: fix twl4030 support for smps regulators

 .../bindings/regulator/fixed-regulator.txt         |   29 +
 .../devicetree/bindings/regulator/regulator.txt    |   54 ++
 Documentation/power/regulator/regulator.txt        |    2 +-
 drivers/regulator/88pm8607.c                       |    2 +-
 drivers/regulator/Kconfig                          |    8 +
 drivers/regulator/Makefile                         |    2 +
 drivers/regulator/aat2870-regulator.c              |    6 +-
 drivers/regulator/ab3100.c                         |    2 +-
 drivers/regulator/ab8500.c                         |    2 +-
 drivers/regulator/ad5398.c                         |    2 +-
 drivers/regulator/bq24022.c                        |    2 +-
 drivers/regulator/core.c                           |  180 +++++--
 drivers/regulator/da903x.c                         |    2 +-
 drivers/regulator/da9052-regulator.c               |  606 ++++++++++++++++++++
 drivers/regulator/db8500-prcmu.c                   |    2 +-
 drivers/regulator/dummy.c                          |    2 +-
 drivers/regulator/fixed.c                          |   85 +++-
 drivers/regulator/gpio-regulator.c                 |    2 +-
 drivers/regulator/isl6271a-regulator.c             |    2 +-
 drivers/regulator/lp3971.c                         |    2 +-
 drivers/regulator/lp3972.c                         |    2 +-
 drivers/regulator/max1586.c                        |    2 +-
 drivers/regulator/max8649.c                        |  157 ++----
 drivers/regulator/max8660.c                        |    2 +-
 drivers/regulator/max8925-regulator.c              |   35 +-
 drivers/regulator/max8952.c                        |    2 +-
 drivers/regulator/max8997.c                        |    2 +-
 drivers/regulator/max8998.c                        |    2 +-
 drivers/regulator/mc13783-regulator.c              |    7 +-
 drivers/regulator/mc13892-regulator.c              |   47 ++-
 drivers/regulator/mc13xxx-regulator-core.c         |   57 ++
 drivers/regulator/mc13xxx.h                        |   26 +-
 drivers/regulator/of_regulator.c                   |   87 +++
 drivers/regulator/pcap-regulator.c                 |    2 +-
 drivers/regulator/pcf50633-regulator.c             |    2 +-
 drivers/regulator/tps6105x-regulator.c             |    3 +-
 drivers/regulator/tps65023-regulator.c             |   89 ++--
 drivers/regulator/tps6507x-regulator.c             |    2 +-
 drivers/regulator/tps6524x-regulator.c             |    2 +-
 drivers/regulator/tps6586x-regulator.c             |    2 +-
 drivers/regulator/tps65910-regulator.c             |   39 +-
 drivers/regulator/tps65912-regulator.c             |    2 +-
 drivers/regulator/twl-regulator.c                  |   48 ++-
 drivers/regulator/userspace-consumer.c             |   13 +-
 drivers/regulator/virtual.c                        |   12 +-
 drivers/regulator/wm831x-dcdc.c                    |   18 +-
 drivers/regulator/wm831x-isink.c                   |    7 +-
 drivers/regulator/wm831x-ldo.c                     |   18 +-
 drivers/regulator/wm8350-regulator.c               |    2 +-
 drivers/regulator/wm8400-regulator.c               |    2 +-
 drivers/regulator/wm8994-regulator.c               |    2 +-
 include/linux/mfd/mc13xxx.h                        |    1 +
 include/linux/mfd/tps65910.h                       |   30 +-
 include/linux/regulator/consumer.h                 |   13 +
 include/linux/regulator/driver.h                   |    4 +-
 include/linux/regulator/of_regulator.h             |   22 +
 sound/soc/codecs/sgtl5000.c                        |    2 +-
 57 files changed, 1414 insertions(+), 345 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/regulator/fixed-regulator.txt
 create mode 100644 Documentation/devicetree/bindings/regulator/regulator.txt
 create mode 100644 drivers/regulator/da9052-regulator.c
 create mode 100644 drivers/regulator/of_regulator.c
 create mode 100644 include/linux/regulator/of_regulator.h

Re: Regulator updates for 3.3

[Linus Torvalds]

On Sun, Jan 8, 2012 at 11:37 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> Please pull from the tree below to merge regulator API updates for
> version 3.3  The major update here is the addition of device tree
> support by Rajendra Nayak, and we also have a new driver for the
> regulators on the DA9052 PMIC.  Otherwise most of the changes are
> bug fixes and cleanups of one form or another.

Mark, I pulled this, but I was *this* close to unpulling it because
it's such an unholy mess.

You seem to do the crazy "daily pull" crap that NOBODY IS EVER SUPPOSED TO DO.

There are lots of totally pointless

   Merge remote-tracking branch 'regulator/for-linus' into regulator-next

merge commits, and no, that's not ok. It makes the history really hard
to read, and has some bad practical impact outside of that
visualization issue too (bisection becomes slower and much more of a
pain, for example).

Just don't do it. There's no excuse. The *only* time you should merge
is when a sub lieutenant asks you to - and if you have people who work
with you and ask you to do pointless merges almost every day, just
tell them to shut the f*ck up already!

If you want to have a special linux-next branch that you want to merge
crap into (for linux-next), go right ahead. But dammit, don't then do
development on top of that kind of crap - use that branch *only* for
linux-next, not for anything else, and don't ask me to pull that
polluted piece of sh*t.

Do your development in a real branch, and do sane things in that real
branch - like pulling from the people who work with you, but only when
they ask, and only when they are ready.  And applying patches. But
never *ever* have those stupid pointless "Merge remote-tracking branch
'regulator/for-linus' into regulator-next" in the branch you actually
use for development, and the branch you send to me.

                       Linus

Re: Regulator updates for 3.3

[Mark Brown]

On Tue, Jan 10, 2012 at 10:27:01AM -0800, Linus Torvalds wrote:

> Just don't do it. There's no excuse. The *only* time you should merge
> is when a sub lieutenant asks you to - and if you have people who work
> with you and ask you to do pointless merges almost every day, just
> tell them to shut the f*ck up already!

Hrm, OK.  These merges are all merges up of bug fixes for -rc from my
own tree into the development code which I tend to do constantly to make
it easier to work directly on the development branch.  What's the best
practice here - push things to you a bit more aggressively and wait
until you've tagged a -rc and then merge that back up into the
development branch?

> Do your development in a real branch, and do sane things in that real
> branch - like pulling from the people who work with you, but only when
> they ask, and only when they are ready.  And applying patches. But
> never *ever* have those stupid pointless "Merge remote-tracking branch
> 'regulator/for-linus' into regulator-next" in the branch you actually
> use for development, and the branch you send to me.

The -next branch is the branch used for development here unless there's
anything that's actually a topic that might want to get viewed
separately (like the device tree stuff this time), and all the merges
are from my own trees.  The reason they showed up as merges from remotes
is an oddity of my workflow here.

Re: Regulator updates for 3.3

[Linus Torvalds]

On Tue, Jan 10, 2012 at 10:45 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
>
> Hrm, OK.  These merges are all merges up of bug fixes for -rc from my
> own tree into the development code which I tend to do constantly to make
> it easier to work directly on the development branch.  What's the best
> practice here - push things to you a bit more aggressively and wait
> until you've tagged a -rc and then merge that back up into the
> development branch?

No. Just don't do the merges. If your development tree isn't stable on
its own, then there is something seriously wrong in what you do.

Doing *one* or two merges during the cycle might be ok just to not get
too far out, but you have way way WAY too many of them.

Just do this:

    gitk d52739c62e00..269d430131b6

to see what I actually got, and then look at what you did on Nov 27,
for example. You did *two* of the merges within hours of each other!
And then you had two more the very next day, without even having any
actual development in between! That's just crazy. The fact that you
then say that you have some kind of *excuse* for that craziness is
just sad.

Stop doing that. It's stupid. It just makes it harder for everybody to
see what you are doing. You didn't even do any *development* in that
tree, you just did a few merges in a row because *some other tree* had
done development! Followed up by a three-way merge where one of the
points was *again* that useless one. Can't you see how crazy that is?

And the "two consecutive merges with no development in between" is
just the extreme case of that. It's wrong in general, that extra crazy
case is just the extreme example of what kind of crap happens when you
think that you should track another unrelated branch in your
"for-next" branch.

You shouldn't do that. It's your development branch. You shouldn't
track the "for-linus" branch in it.

                          Linus

Re: Regulator updates for 3.3

[Mark Brown]

On Tue, Jan 10, 2012 at 11:18:44AM -0800, Linus Torvalds wrote:
> On Tue, Jan 10, 2012 at 10:45 AM, Mark Brown

> > Hrm, OK.  These merges are all merges up of bug fixes for -rc from my
> > own tree into the development code which I tend to do constantly to make
> > it easier to work directly on the development branch.  What's the best
> > practice here - push things to you a bit more aggressively and wait
> > until you've tagged a -rc and then merge that back up into the
> > development branch?

> No. Just don't do the merges. If your development tree isn't stable on
> its own, then there is something seriously wrong in what you do.

Just to be clear I'm not disagreeing that most if not all of these
merges aren't needed, I'm just trying to figure out in general how to
get the bug fixes into the development code when it is useful to do
that.  The merges are happening because I've been in the habit of
merging fixes up as a matter of routine rather than due to an actual
immediate need to get the fixes.

The major reason why the merges might be needed is that often enough to
be interesting the stuff getting pushed into -rc is the result of
fixing bugs that get discovered in the course of doing the new work and
interfere with that new work.  Either the testing needed to verify that
work makes some existing issue (that might also be triggered in some
other way) obvious or the bug got discovered by code inspection as
someone was looking at a feature and so the two changes end up colliding
with each other.

Especially in the cases where the lack of the bug fix breaks the new
code it sems sensible enough to want to do the merges so that the
history includes things that actually work.  This was a lot easier back
when there was no question of most embedded boards booting with vanilla
mainline and you couldn't actually directly develop or test usefully
with upstream code and working history wasn't an issue :/

> Just do this:
> 
>     gitk d52739c62e00..269d430131b6
>
> to see what I actually got, and then look at what you did on Nov 27,
> for example. You did *two* of the merges within hours of each other!
> And then you had two more the very next day, without even having any
> actual development in between! That's just crazy. The fact that you

Yes, it's excessive.  I also made the above graph much more confusing by
discarding my for-linus branch every time you merged it and making new
ones based off new -rcs which means that gitk can't just show a
for-linus branch that gets repeatedly merged into the development branch
but instead shows a bunch of random isolated commits which get merged
in.  I'm not saying that this would solve the problem you see, just that
it's an issue.

Clearly even if I did nothing beyond defer the merges until there was
some need for them that'd help though it wouldn't fully address your
issue.

Re: Regulator updates for 3.3

[Linus Torvalds]

On Tue, Jan 10, 2012 at 2:27 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
>
> Especially in the cases where the lack of the bug fix breaks the new
> code it sems sensible enough to want to do the merges so that the
> history includes things that actually work.

So I don't mind merges if they have a lear reason for existing.

This is actually one of my major gripes with the git UI, and one of
the few areas where I really think I screwed up: I made merging *too*
easy by default. I should have made it always start up an editor for a
merge message, the way it does for a commit - rather than just do a
trivial pointless merge without even asking the user for a reason for
the merge.

So looking at that almost two months of regulator history in

   gitk d52739c62e00..269d430131b6

I would not have reacted badly at all if there were one or two of
those merges, and they actually had a reason associated with them.
Sadly, due to that git UI mess-up, that's harder to do than it should
be. Oh, it's easy enough with "git merge --no-commit" followed by just
"git commit", and then you get the normal git editor window.

So right now "git merge" (and "git pull") make it too easy to make
those meaningless merge commits. If instead of seven pointless merges
you had (say) had two merges that had messages about *why* they
weren't pointless, I'd be perfectly happy.

Addid junio and git to the cc just to bring up this issue of bad UI
once again. I realize it could break old scripts to start up an editor
window, but still..

                       Linus

Re: Regulator updates for 3.3

[Mark Brown]

On Tue, Jan 10, 2012 at 02:54:27PM -0800, Linus Torvalds wrote:
> On Tue, Jan 10, 2012 at 2:27 PM, Mark Brown

> > Especially in the cases where the lack of the bug fix breaks the new
> > code it sems sensible enough to want to do the merges so that the
> > history includes things that actually work.

> So I don't mind merges if they have a lear reason for existing.

OK, good - I figured that was the case but wanted to make sure as you
were stating things rather more strongly than that.

Just to warn you there's also a whole stack of similar merges going to
come in via the sound tree too due to the same workflow, I *could* try
to rebuild the history and ask Takashi to redo his tree using that but
there's a lot of history there and it'd be hard to figure out which of
the merges was actually important.  Is it OK to leave things as they are
for this release?

> So right now "git merge" (and "git pull") make it too easy to make
> those meaningless merge commits. If instead of seven pointless merges
> you had (say) had two merges that had messages about *why* they
> weren't pointless, I'd be perfectly happy.

> Addid junio and git to the cc just to bring up this issue of bad UI
> once again. I realize it could break old scripts to start up an editor
> window, but still..

I'd use a configuration option that popped up an editor by default, even
if I did have to manually enable it.

Re: Regulator updates for 3.3

[Junio C Hamano]

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

> Addid junio and git to the cc just to bring up this issue of bad UI
> once again. I realize it could break old scripts to start up an editor
> window, but still..

It is a non-starter to unconditionally start an editor. We would need a
good way for users to conveniently say "I am doing this unusual merge that
needs to be justified, and I want an editor to write my justification".

Obviously, "git merge -e regulator/for-linus" would work and is just three
keystrokes, which can be said "convenient enough" once the user gets used
to, but I think this is still inadequate as a solution, as the real
problem is it is _too_ easy to forget to give the option.  Until the user
becomes _aware_ of the issues, it will not even occur to the user that
s/he _has_ to justify a merge (or not create a merge at all) in certain
circumstances and directions.  After all, you have been repeating the "do
not make meaningless merges" for the past five years on the list. UI tweak
alone will not fix that.

If we are to rely on user's conscious action, I think it may be something
like a set of configurations that say things like:

 - This branch is for advancing a specific topic, and not for merging
   random development that happen elsewhere;

 - This branch is for merging works by people downstream from me;

 - This remote tracking branch (and by extension that branch at that
   remote that uses this as its remote tracking branch) is my upstream and
   I should not be merging back from it; and

 - This remote tracking branch is my downstream, and I should freely merge
   it when I heard it is ready.

and depending on the combination of what is being merged into what, toggle
the --edit option by default for "git merge" when neither "--edit" nor
"--no-edit" is given, just like "git merge" defaults to "--edit" when
merging an annotated tag.

Re: Regulator updates for 3.3

[Linus Torvalds]

On Tue, Jan 10, 2012 at 6:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> It is a non-starter to unconditionally start an editor.

I really wonder. Because not being default will always lead to really
odd ways of saying "it should have been default, so we'll make up
these complex and arbitrary special rules" (like the ones you were
starting to outline).

So I really suspect it would be easier and more straightforward to
instead just bite the bullet, and say:

 (a) start an editor by default if both stdin/stdout matched in fstat
and were istty().

 (b) have some trivial way to disable that default behavior for people
who really want the legacy behavior. And by "trivial" I mean "set the
GIT_LEGACY_MERGE environment variable" or something.

 (c) have a "--no-editor" command line switch so that scripts and/or
users that want to make it explicit (rather than rely on the hacky
legacy workaround) can do so (and a explicit "--editor" switch to
enable people to use a GUI editor even if they aren't on a terminal -
think something IDE environment, whatever).

Where (a) is so that people will always get the editor if they aren't
aware of it, and (b) is so that existing scripting environments can
then *trivially* work around the fact that we changed semantics,
including on a site-wide basis. With (c) being for future users. Of
course, just a "git merge < /dev/null" would also do it, but sounds
ridiculously hacky (and doesn't allow the "--editor" version), so that
"--no-editor" flag sounds saner and much more powerful.

Of course, if you use "-m", no editor would fire up anyway, exactly
like with "git commit", so that's one way to avoid the issue forever
(and be backwards compatible). But if you actually *want* to get the
auto-generated message and no editor, that would need that new switch.

Yes, git has been very good about not breaking semantics. But it's
happened before too when it needed to happen. We've had much bigger
breaks (like the whole "git-xyz" to "git xyz" transition, for example,
which broke a lot of scripts).

                       Linus

Re: Regulator updates for 3.3

[Junio C Hamano]

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

> On Tue, Jan 10, 2012 at 6:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> It is a non-starter to unconditionally start an editor.
>
> I really wonder. Because not being default will always lead to really
> odd ways of saying "it should have been default, so we'll make up
> these complex and arbitrary special rules" (like the ones you were
> starting to outline).
>
> So I really suspect it would be easier and more straightforward to
> instead just bite the bullet, and say:
>
>  (a) start an editor by default if both stdin/stdout matched in fstat
> and were istty().
>
>  (b) have some trivial way to disable that default behavior for people
> who really want the legacy behavior. And by "trivial" I mean "set the
> GIT_LEGACY_MERGE environment variable" or something.
>
>  (c) have a "--no-editor" command line switch so that scripts and/or
> users that want to make it explicit (rather than rely on the hacky
> legacy workaround) can do so (and a explicit "--editor" switch to
> enable people to use a GUI editor even if they aren't on a terminal -
> think something IDE environment, whatever).

Hrm. Lack of any quoted line other than the first line from my message,
together with (c) above, makes me suspect that you did not read beyond the
first line before composing this message you are responding to.

> Yes, git has been very good about not breaking semantics. But it's
> happened before too when it needed to happen. We've had much bigger
> breaks (like the whole "git-xyz" to "git xyz" transition, for example,
> which broke a lot of scripts).

Yes, I am learning from the experience to be cautious ;-)

I dunno. You just scrapped the plan for 1.7.10; it may have to be called 2.0
instead.

Re: Regulator updates for 3.3

[Linus Torvalds]

On Tue, Jan 10, 2012 at 7:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> I really wonder. Because not being default will always lead to really
>> odd ways of saying "it should have been default, so we'll make up
>> these complex and arbitrary special rules" (like the ones you were
>> starting to outline).
>
> Hrm. Lack of any quoted line other than the first line from my message,
> together with (c) above, makes me suspect that you did not read beyond the
> first line before composing this message you are responding to.

No. See again. I did read your suggestion, and that's where the "we'll
make up these complex and arbitrary special rules" comment comes from.
Did I mis-understand it?

I think it's a *horrible* idea to go down the road or some
branch-specific configurations and then, and I quote:

  "depending on the combination of what is being merged into what,
toggle the --edit option by default"

THAT is the kind of design that sounds crazy.

Instead, just make editing the default. No ifs, buts, or maybe. No
configuration, no complexities - just make it act the same way our
pager logic acts (ie redirecting stdin/stdout obviously shuts down the
pager, and equally obviously needs to shut down the editor).

Then, the --edit/--no-edit flags are for future users that want to
make it explicit. But they aren't about rules, they are about just
making very explicit statements of "I don't want the editor".

The (b) thing I suggested was for "work around for people who have
legacy cases that they don't want to make explicit". I guess you could
count that as some rule, but I really think it's more of a "ok, we had
bad legacy behavior, and now we have scripts that depended on that bad
legacy".

But the notion of complex rules? That sounds really really bad. I'd
much rather get *rid* of the one complex rule we have (the "merging a
tag implies --edit"). That rule is already a hack.

                   Linus

Re* Regulator updates for 3.3

[Junio C Hamano]

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

> The (b) thing I suggested was for "work around for people who have
> legacy cases that they don't want to make explicit". I guess you could
> count that as some rule, but I really think it's more of a "ok, we had
> bad legacy behavior, and now we have scripts that depended on that bad
> legacy".

I would think that would solve the issue for scripts like the one used to
rebuild the linux-next tree.  I also have such a script to rebuild 'pu' I
use three or four times a day, but admittedly the standard input of "git
merge" in that script is connected to a here document that feeds the list
of branches to be merged to the loop that drives the "git merge", so your
heuristic (a) will kick in and I wouldn't need the GIT_MERGE_NO_EDIT
environment myself.

What makes me uneasy about the idea of running the editor by default is
that many people still use Git as a better CVS/SVN. Their workflow is to
build randomly on their 'master', attempt to push and get rejected, pull
only so that they can push out, and then push the merge result out. Such
merges are done without any consideration on the cohesiveness of the
branch (the segment of the history that records their work since they last
pulled from the central repository). Having to justify the backmerge is
nothing but a nuisance for them, as they do not have any justification
better than "I am done for the day, and my commuter shuttle will leave in
15 minutes, so I tried to push what I've done so far, but it was rejected
due to non fast-forward, and I am merging random things others did so that
I can push back". They won't be saying "This merges the great work I
completed and have been testing privately for a few days to the trunk", as
the direction of their merge is backwards.

With that caveat, the patch should look like this.

-- >8 --
Subject: [PATCH] merge: use editor by default in interactive sessions

Traditionally, a cleanly resolved merge was committed by "git merge" using
the auto-generated merge commit log message with invoking the editor.

After 5 years of use in the field, it turns out that many people perform
too many unjustified backmerges of the upstream history into their topic
branches. These merges are not just useless, but they are more often than
not explained and making the end result unreadable when it gets time for
merging their history back to their upstream.

Earlier we added the "--edit" option to the command, so that people can
edit the log message to explain and justify their merge commits. Let's
take it one step further and spawn the editor by default when we are in an
interactive session (i.e. the standard input and the standard output are
pointing at the same tty device).

There may be existing scripts that leave the standard input and the
standard output of the "git merge" connected to whatever environment the
scripts were started, and such invocation might trigger the above
"interactive session" heuristics. Such scripts can export GIT_MERGE_LEGACY
environment variable set to "yes" to force the traditional behaviour.

Suggested-by: Linus Torvalds
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/merge.c |   34 ++++++++++++++++++++++++++++++----
 t/test-lib.sh   |    3 ++-
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 99f1429..6a80e1e 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -46,7 +46,7 @@ static const char * const builtin_merge_usage[] = {
 
 static int show_diffstat = 1, shortlog_len, squash;
 static int option_commit = 1, allow_fast_forward = 1;
-static int fast_forward_only, option_edit;
+static int fast_forward_only, option_edit = -1;
 static int allow_trivial = 1, have_message;
 static struct strbuf merge_msg;
 static struct commit_list *remoteheads;
@@ -189,7 +189,7 @@ static struct option builtin_merge_options[] = {
 		"create a single commit instead of doing a merge"),
 	OPT_BOOLEAN(0, "commit", &option_commit,
 		"perform a commit if the merge succeeds (default)"),
-	OPT_BOOLEAN('e', "edit", &option_edit,
+	OPT_BOOL('e', "edit", &option_edit,
 		"edit message before committing"),
 	OPT_BOOLEAN(0, "ff", &allow_fast_forward,
 		"allow fast-forward (default)"),
@@ -877,12 +877,12 @@ static void prepare_to_commit(void)
 	write_merge_msg(&msg);
 	run_hook(get_index_file(), "prepare-commit-msg",
 		 git_path("MERGE_MSG"), "merge", NULL, NULL);
-	if (option_edit) {
+	if (0 < option_edit) {
 		if (launch_editor(git_path("MERGE_MSG"), NULL, NULL))
 			abort_commit(NULL);
 	}
 	read_merge_msg(&msg);
-	stripspace(&msg, option_edit);
+	stripspace(&msg, 0 < option_edit);
 	if (!msg.len)
 		abort_commit(_("Empty commit message."));
 	strbuf_release(&merge_msg);
@@ -1076,6 +1076,29 @@ static void write_merge_state(void)
 	close(fd);
 }
 
+static int default_edit_option(void)
+{
+	static const char name[] = "GIT_MERGE_LEGACY";
+	const char *e = getenv(name);
+	struct stat st_stdin, st_stdout;
+
+	if (e) {
+		int v = git_config_maybe_bool(name, e);
+		if (v < 0)
+			die("Bad value '%s' in environment '%s'", e, name);
+		return !v;
+	}
+
+	/* Use editor if stdin and stdout are the same and is a tty */
+	return (!fstat(0, &st_stdin) &&
+		!fstat(1, &st_stdout) &&
+		isatty(0) &&
+		st_stdin.st_dev == st_stdout.st_dev &&
+		st_stdin.st_ino == st_stdout.st_ino &&
+		st_stdin.st_rdev == st_stdout.st_rdev);
+}
+
+
 int cmd_merge(int argc, const char **argv, const char *prefix)
 {
 	unsigned char result_tree[20];
@@ -1261,6 +1284,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		}
 	}
 
+	if (option_edit < 0)
+		option_edit = default_edit_option();
+
 	if (!use_strategies) {
 		if (!remoteheads->next)
 			add_strategies(pull_twohead, DEFAULT_TWOHEAD);
diff --git a/t/test-lib.sh b/t/test-lib.sh
index bdd9513..439f192 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -63,7 +63,8 @@ GIT_AUTHOR_NAME='A U Thor'
 GIT_COMMITTER_EMAIL=committer@example.com
 GIT_COMMITTER_NAME='C O Mitter'
 GIT_MERGE_VERBOSITY=5
-export GIT_MERGE_VERBOSITY
+GIT_MERGE_LEGACY=yes
+export GIT_MERGE_VERBOSITY GIT_MERGE_LEGACY
 export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME
 export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME
 export EDITOR
-- 
1.7.9.rc0.39.ged86b

Re: Re* Regulator updates for 3.3

[Linus Torvalds]

On Tue, Jan 10, 2012 at 10:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> What makes me uneasy about the idea of running the editor by default is
> that many people still use Git as a better CVS/SVN. Their workflow is to
> build randomly on their 'master', attempt to push and get rejected, pull
> only so that they can push out, and then push the merge result out.

Sure. And I don't think we can do much about it. They'll either set
the legacy flag, or they'll just exit the editor without adding
anything useful (if you come from a CVS background in particular, you
probably never learnt to do good commit logs anyway).

So it will be a bit more work for the bad workflow, I agree - although
if it really irritates people, they can just set that GIT_MERGE_LEGACY
in their .bashrc files or something. But we can *hope* that even those
people might sometimes actually talk about what/why they are doing
things, or maybe even learn about that whole "distributed" thing.

I agree that is unlikely to ever happen, though. It's more likely that
they will change their aliases so that their "update" command just
adds the --no-edit flag. Regardless, it doesn't sound *too* onerous to
work around.

Patch looks good to me. I would personally have compared "st_mode"
instead of (or in addition to) "st_rdev", but I don't think it matters
all that much.

                                 Linus

Re: Re* Regulator updates for 3.3

[Pete Harlan]

On 01/10/2012 10:59 PM, Junio C Hamano wrote:
> There may be existing scripts that leave the standard input and the
> standard output of the "git merge" connected to whatever environment the
> scripts were started, and such invocation might trigger the above
> "interactive session" heuristics. Such scripts can export GIT_MERGE_LEGACY
> environment variable set to "yes" to force the traditional behaviour.

The name GIT_MERGE_LEGACY gives no clue about what flavor of legacy
merge behavior is being enabled.  Something like GIT_MERGE_LEGACY_EDIT
might be clearer, or perhaps just have GIT_MERGE_EDIT=0 to get the old
behavior without reference to whether or not that behavior is
considered legacy.

--Pete

Re: Re* Regulator updates for 3.3

[Junio C Hamano]

Pete Harlan <pgit@pcharlan.com> writes:

> On 01/10/2012 10:59 PM, Junio C Hamano wrote:
>> There may be existing scripts that leave the standard input and the
>> standard output of the "git merge" connected to whatever environment the
>> scripts were started, and such invocation might trigger the above
>> "interactive session" heuristics. Such scripts can export GIT_MERGE_LEGACY
>> environment variable set to "yes" to force the traditional behaviour.
>
> The name GIT_MERGE_LEGACY gives no clue about what flavor of legacy
> merge behavior is being enabled.  Something like GIT_MERGE_LEGACY_EDIT
> might be clearer, or perhaps just have GIT_MERGE_EDIT=0 to get the old
> behavior without reference to whether or not that behavior is
> considered legacy.

Hrm.

The only case your suggestion may make a difference would be when we find
another earlier UI mistake we would want to correct in a backward
incompatible way that affects _existing_ scripts.

With your suggestion, they need to export "GIT_MERGE_EDIT=0" today, and
they will need to update again to export "GIT_MERGE_SOMETHINGELSE=0" when
such an incompatible change comes.

With a single "GIT_MERGE_LEGACY=YesPlease", they can be future-proofed today
and will not be affected when we make another incompatible change.

So I am not sure why separating the big-red-switch into smaller pieces
would be an improvement, especially wnen the scripts that want to specify
finer-grained control of features can use "--[no-]edit" options to
explicitly ask for it.

Re: Re* Regulator updates for 3.3

[Martin Fick]

On Monday, January 16, 2012 04:33:00 pm Junio C Hamano 
wrote:
> With your suggestion, they need to export
> "GIT_MERGE_EDIT=0" today, and they will need to update
> again to export "GIT_MERGE_SOMETHINGELSE=0" when such an
> incompatible change comes.
> 
> With a single "GIT_MERGE_LEGACY=YesPlease", they can be
> future-proofed today and will not be affected when we
> make another incompatible change.
> 
> So I am not sure why separating the big-red-switch into
> smaller pieces would be an improvement, especially wnen
> the scripts that want to specify finer-grained control
> of features can use "--[no-]edit" options to explicitly
> ask for it.


Then, what would I do if I write a script which uses the new 
edit functionality (without even being aware that there was 
an old way) and you introduce a new incompatibility?  I 
can't turn on GIT_MERGE_LEGACY then since it would revert to 
behavior which my script would not expect (since it was 
written after the current incompatibility, but before the 
new one)!

-Martin

Re: Re* Regulator updates for 3.3

[Pete Harlan]

On 01/16/2012 03:33 PM, Junio C Hamano wrote:
> Pete Harlan <pgit@pcharlan.com> writes:
> 
>> On 01/10/2012 10:59 PM, Junio C Hamano wrote:
>>> There may be existing scripts that leave the standard input and the
>>> standard output of the "git merge" connected to whatever environment the
>>> scripts were started, and such invocation might trigger the above
>>> "interactive session" heuristics. Such scripts can export GIT_MERGE_LEGACY
>>> environment variable set to "yes" to force the traditional behaviour.
>>
>> The name GIT_MERGE_LEGACY gives no clue about what flavor of legacy
>> merge behavior is being enabled.  Something like GIT_MERGE_LEGACY_EDIT
>> might be clearer, or perhaps just have GIT_MERGE_EDIT=0 to get the old
>> behavior without reference to whether or not that behavior is
>> considered legacy.
> 
> Hrm.
> 
> The only case your suggestion may make a difference would be when we find
> another earlier UI mistake we would want to correct in a backward
> incompatible way that affects _existing_ scripts.
> 
> With your suggestion, they need to export "GIT_MERGE_EDIT=0" today, and
> they will need to update again to export "GIT_MERGE_SOMETHINGELSE=0" when
> such an incompatible change comes.

Which is a good thing, because maybe they started using Git after the
current proposed change (which they like), and what you see as new
becomes their "legacy" behavior.  If you change something after that,
you can't use GIT_MERGE_LEGACY=yes for that one also because which
legacy is it preserving?

In general, naming configuration variables "DO_IT_<THIS_WAY>" instead
of "DO_IT_THE_OLD_WAY" is better because it's self-documenting.  The
only time I think I'd prefer "LEGACY" is if you're planning on
deprecating and removing it eventually and you want to indicate
something to that effect in the name.

--
Pete Harlan
pgit@pcharlan.com

Re: Re* Regulator updates for 3.3

[Junio C Hamano]

Pete Harlan <pgit@pcharlan.com> writes:

> ... The
> only time I think I'd prefer "LEGACY" is if you're planning on
> deprecating and removing it eventually and you want to indicate
> something to that effect in the name.

The discussion that led to the naming of that LEGACY token needs to be
re-read, then. The kind of "LEGACY" you prefer is exactly why the
environment variable is called LEGACY in the patch you are commenting on,
written in response to Linus's suggestion to switch the default, even
though I am not 100% buying it.

Having said that, I think I am wasting my time responding to this thread
during the feature-freeze period for v1.7.9, as I am not a big fan of
switching the default without adequate warning and transition plans, after
getting burned by the "'git-foo' vs 'git foo'" flames back in the v1.6.0
release. We would likely to take a gradual and smoother migration route to
transition, e.g. v1.7.9 to introduce "merge --edit", v1.7.10 to introduce
a configuration variable merge.edit (lack of which gives a warning and an
advice message while defaulting to 'no' to preserve the traditional
behaviour), and finally v1.8.0 (or v2.0) to flip the default to 'yes'
(while the configuration still giving a warning and an advice message)
that "merge --no-edit" can still countermand.

So you have until v1.7.10 to decide a good name for the overriding
environment variable.

Re: Regulator updates for 3.3

[Linus Torvalds]

On Tue, Jan 10, 2012 at 7:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> I dunno. You just scrapped the plan for 1.7.10; it may have to be called 2.0
> instead.

Btw, version numbers are cheap. I already argued for updating to 2.0
just because of the new signed tag pulling, which I think is a much
bigger issue.

I don't think a small change like "start the editor by default for
merge messages" is nearly as worthy of a version number. But I
wouldn't argue against it either, exactly because those major numbers
are cheap.

It took the kernel until 2.6.39 to learn that, I think git could learn
to use its major number more freely much earlier.

                   Linus

Re: Regulator updates for 3.3

[Paul Gortmaker]

[Re: Regulator updates for 3.3] On 10/01/2012 (Tue 14:54) Linus Torvalds wrote:

[...]

> So right now "git merge" (and "git pull") make it too easy to make
> those meaningless merge commits. If instead of seven pointless merges

It looks like the editor-by-default solution is a go, but there still
might be value in increasing the visibility of the pointless merges
via. the patch below.

Paul.

> you had (say) had two merges that had messages about *why* they
> weren't pointless, I'd be perfectly happy.
> 
> Addid junio and git to the cc just to bring up this issue of bad UI
> once again. I realize it could break old scripts to start up an editor
> window, but still..
> 
>                        Linus


>From 1a548fa97b78cebcded15d2b00ee3d826f731abd Mon Sep 17 00:00:00 2001
From: Paul Gortmaker <paul.gortmaker@windriver.com>
Date: Wed, 11 Jan 2012 10:33:45 -0500
Subject: [PATCH] merge: Make merge strategy message follow the diffstat

One of the common problems I've seen with people who are
somewhat new to git is that they don't realize that a pull
is a fetch+merge.  They simply decide they want all the
latest stuff and issue a git pull without really thinking
if they are on a branch with local commits or on master,
where a fast forward can take place.

But the one line message that tells you whether you got a fast
forward or a real merge commit is usually pushed off the
screen by all the diffstat information.  So these users won't
even know that their pull has created a merge, and chances
are they will never change their workflow.

By moving the message after the diffstat, there is a better
chance that people will be aware they've done a pointless
merge commit.

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>

diff --git a/builtin/merge.c b/builtin/merge.c
index 3a45172..9471588 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -370,12 +370,12 @@ static void finish(struct commit *head_commit,
 {
 	struct strbuf reflog_message = STRBUF_INIT;
 	const unsigned char *head = head_commit->object.sha1;
+	int automsg = 0;
 
-	if (!msg)
+	if (!msg) {
+		automsg = 1;
 		strbuf_addstr(&reflog_message, getenv("GIT_REFLOG_ACTION"));
-	else {
-		if (verbosity >= 0)
-			printf("%s\n", msg);
+	} else {
 		strbuf_addf(&reflog_message, "%s: %s",
 			getenv("GIT_REFLOG_ACTION"), msg);
 	}
@@ -409,6 +409,9 @@ static void finish(struct commit *head_commit,
 		diff_flush(&opts);
 	}
 
+	if (!automsg && verbosity >= 0)
+		printf("%s\n", msg);
+
 	/* Run a post-merge hook */
 	run_hook(NULL, "post-merge", squash ? "1" : "0", NULL);
 
-- 
1.7.4.4

Re: [PATCH] merge: Make merge strategy message follow the diffstat

[Junio C Hamano]

Paul Gortmaker <paul.gortmaker@windriver.com> writes:

> By moving the message after the diffstat, there is a better chance that
> people will be aware they've done a pointless merge commit.
>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>

I think the goal of the change may be worthy, but a few points:

 - What does "automsg" mean? Is "auto" in contrast to "manual"? Even
   better, wouldn't it be far simpler to just use

	if (msg && verbosity >= 0)
		printf("%s\n", msg);

   and get rid of this mysteriously named variable altogether?

 - Wouldn't it make more sense to move "No merge message -- not updating
   HEAD" also to the end?

 - After applying this patch, does the tests still pass?

Thanks.

Re: [PATCH] merge: Make merge strategy message follow the diffstat

[Nguyen Thai Ngoc Duy]

On Sat, Jan 14, 2012 at 2:12 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Paul Gortmaker <paul.gortmaker@windriver.com> writes:
>
>> By moving the message after the diffstat, there is a better chance that
>> people will be aware they've done a pointless merge commit.
>>
>> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
>
> I think the goal of the change may be worthy

Still, diffstat from a fetch/pull is sometimes too verbose. It'd be
better if we have something that fit in one screen (dirstat or maybe
just a first few lines from diffstat then ellipsis) then refer users
to "git diff --stat HEAD@{1}" for more detail stat.
-- 
Duy

Re: [PATCH] merge: Make merge strategy message follow the diffstat

[Linus Torvalds]

On Fri, Jan 13, 2012 at 11:27 AM, Nguyen Thai Ngoc Duy
<pclouds@gmail.com> wrote:
>
> Still, diffstat from a fetch/pull is sometimes too verbose. It'd be
> better if we have something that fit in one screen (dirstat or maybe
> just a first few lines from diffstat then ellipsis) then refer users
> to "git diff --stat HEAD@{1}" for more detail stat.

Yeah, I've wanted that. Show the beginning, the end, and the summary
line of the diffstat would be lovely.

It would be lovely in "git commit" too. Not just

    Modified: filename

but a diffstat that shows now many lines.

And what I've *really* wanted is to actually see the diff itself if it
is small. So some kind of "dynamic summary": for one-liners (or
ten-liners), show the whole diff. For medium-sized changes, show the
whole diffstat. And for really big changes, show an outline and the
"768 files changed, 179851 lines added, 7630 lines removed" stats.

IOW, whatever fits in, say, 50 lines or less.

That would be absolutely lovely if somebody were to do it.

                  Linus

Re: [PATCH] merge: Make merge strategy message follow the diffstat

[Miles Bader]

So ... "--shortish-diffthingy"

-miles

-- 
Zeal, n. A certain nervous disorder afflicting the young and inexperienced.