Page MenuHomePhabricator

morestatus: move fb extension to core by plugging to `hg status --verbose`
ClosedPublic

Authored by pulkit on Aug 3 2017, 12:07 AM.

Details

Summary

morestatus extension in fbext use to show more context about the state of the
repo like the repository is in a unfinished merge state, or a rebase is going
on, or histedit is going on, listing the files which need to be resolved and
also suggesting ways to handle the situation.

This patch moves the extension directly to core by plugging it into the
--verbose flag of the status command. So now if you are in any unfinished state
and you do hg status -v, it will show you details and help related to the state.

The extension in fbext also shows context about unfinished update state
which is not ported to core as that plug in hooks to update command which need
to be tackled somewhat differently.

The following configuration will turn the behaviour on by default

[commands]
status.verbose = 1

You can also skip considering some states like bisect as follows:

[commands]
status.skipstates=bisect

This patch also adds test for the feature.

.. feature::

``hg status -v`` can now show unfinished state. For example, when in
an unfinished rebase state, ``hg status -v`` might show::

# The repository is in an unfinished *rebase* state.
# No unresolved merge conflicts.
# To continue:                hg rebase --continue
# To abort:                   hg rebase --abort

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pulkit created this revision.Aug 3 2017, 12:07 AM
pulkit added a subscriber: durham.Aug 3 2017, 12:10 AM

@durham and I thought on the flag name and had '--more' and '--repo-state' in our mind. Since '--repo-state' is more explicit, I went with that. Need suggestions for a better flag name. :)

In D219#3558, @pulkit wrote:

@durham and I thought on the flag name and had '--more' and '--repo-state' in our mind. Since '--repo-state' is more explicit, I went with that. Need suggestions for a better flag name. :)

Do we need a flag? Why not just a config option (whose value is ignored with HGPLAIN)?

So what does '--verbose' do for status currently? Didn't seem to do anything. Can this output be for verbose?

In D219#3562, @akushner wrote:

So what does '--verbose' do for status currently? Didn't seem to do anything. Can this output be for verbose?

Good question. From https://www.mercurial-scm.org/wiki/CompatibilityRules#Commands: "Adding messages at the verbose level is also usually acceptable."

I think it makes sense for this output to be part of the verbose form.

It would still be nice to have a config option for it. I don't like "morestatus" for that. I would think it should be in commands.status (along with ".relative"). Maybe commands.status.verbose=1 to enable it if we decide to use the --verbose flag for it.

@martinvonz - I like the commands.status.verbose=1 option. I think the verbose form of status makes sense when there's a large body of unsophisticated source control users and the more details on how to get out of the current predicament, the better.

pulkit added a comment.Aug 3 2017, 2:49 AM

@akushner @martinvonz : thanks for suggestions. I also like the --verbose option, I will send a new version with that.
@martinvonz: Since you don't like 'morestatus' much, shall I place the skipstates option which skips some states under some different config name, maybe status.skipstates?

In D219#3566, @pulkit wrote:

@akushner @martinvonz : thanks for suggestions. I also like the --verbose option, I will send a new version with that.
@martinvonz: Since you don't like 'morestatus' much, shall I place the skipstates option which skips some states under some different config name, maybe status.skipstates?

I'm not 100% sure, but I think we're trying to put command-specific configs under command.<command>, so it would probably be commands.status.skipstates. Also, shouldn't you be putting it in configitems.py?

quark added a subscriber: quark.Aug 3 2017, 8:42 PM

Maybe this is a good case for the new release note extension. According to https://www.mercurial-scm.org/wiki/ReleasenotesExtension having the following in commit message would be helpful:

.. feature::

   ``hg status -v`` can now show unfinished state. For example, when in
   an unfinished rebase state, ``hg status -v`` might show::

     # The repository is in an unfinished *rebase* state.
     # No unresolved merge conflicts.
     # To continue:                hg rebase --continue
     # To abort:                   hg rebase --abort

I'm thinking about the difference between the deprecated [defaults] and [commands] (and also [alias]) but don't quite get it. It seems people wanting the behavior might just set alias.status = status -v, or use [defaults]. What's the advantage of using a [commands] option?

In D219#3661, @quark wrote:

I'm thinking about the difference between the deprecated [defaults] and [commands] (and also [alias]) but don't quite get it. It seems people wanting the behavior might just set alias.status = status -v, or use [defaults]. What's the advantage of using a [commands] option?

Good point, there's probably no good reason to use [commands] for this. [defaults] has been discouraged, but I don't know if there's a good reason to discourage it (I think it's already ignored when HGPLAIN is in effect). [commands] make sense when there's no command line flag for the behavior, but we do have that here. So if we agree on using --verbose for this, I don't think I see a reason to a special config for it and we can use [defaults] instead.

Hmm, I just realized there is a somewhat good reason to avoid [defaults]: it applies to *all* options at once. Let me clarify with an example. Let's say your sysadmin has set "defaults.status = --relative" (hypothetical option -- it's currently a config in [commands]) and you want to keep that *and* get verbose status output, you now have to copy the default config from the system hgrc. I don't think I've heard that argument against [defaults] before, but that seems more relevant than the scripting argument (which, again, goes away with HGPLAIN). Or am I mistaken?

durin42 requested changes to this revision.Aug 4 2017, 5:55 PM
durin42 added a subscriber: durin42.
In D219#3661, @quark wrote:

I'm thinking about the difference between the deprecated [defaults] and [commands] (and also [alias]) but don't quite get it. It seems people wanting the behavior might just set alias.status = status -v, or use [defaults]. What's the advantage of using a [commands] option?

Good point, there's probably no good reason to use [commands] for this. [defaults] has been discouraged, but I don't know if there's a good reason to discourage it (I think it's already ignored when HGPLAIN is in effect).

The discouragement is twofold: we didn't used to have HGPLAIN, and it used to be there was no way to un-do the setting of a boolean flag in [defaults]. Both of those limitations are now fixed, so I'm less worried about it overall. That said, I would like a config knob for this, so it can be part of ui.tweakdefaults.

[commands] make sense when there's no command line flag for the behavior, but we do have that here. So if we agree on using --verbose for this, I don't think I see a reason to a special config for it and we can use [defaults] instead.
Hmm, I just realized there is a somewhat good reason to avoid [defaults]: it applies to *all* options at once. Let me clarify with an example. Let's say your sysadmin has set "defaults.status = --relative" (hypothetical option -- it's currently a config in [commands]) and you want to keep that *and* get verbose status output, you now have to copy the default config from the system hgrc. I don't think I've heard that argument against [defaults] before, but that seems more relevant than the scripting argument (which, again, goes away with HGPLAIN). Or am I mistaken?

That's another argument in favor of a config knob IMO.

I'm not sure if --verbose should be (ab)used for this, given that we've got --terse. --verbose almost sounds like the opposite of --terse, so it might be confusing. Meditate on that, I guess.

(Marking as "Request changes" for now, since we seem largely agreed that --verbose is a decent way to go, and if nobody else is worried about the conceptual overlap with --terse I'm fine with it, especially since I more or less expect this to be on in tweakdefaults soon.)

This revision now requires changes to proceed.Aug 4 2017, 5:55 PM
pulkit edited edge metadata.Aug 5 2017, 4:26 AM
pulkit edited the summary of this revision. (Show Details)
pulkit retitled this revision from morestatus: move fb extension to core as '--repo-state' option to status to morestatus: move fb extension to core by plugging to `hg status --verbose`.
pulkit updated this revision to Diff 577.
durin42 accepted this revision as: durin42.Aug 8 2017, 2:15 PM

I'm still a tiny bit worried about potential confusion between --terse and --verbose (in that they're not opposites), but I'm also fine with this. I'll take it in a couple of days if I don't hear any objections.

pulkit added a comment.EditedAug 9 2017, 9:17 AM
In D219#4343, @durin42 wrote:

I'm still a tiny bit worried about potential confusion between --terse and --verbose (in that they're not opposites), but I'm also fine with this. I'll take it in a couple of days if I don't hear any objections.

Currently if we do hg status --terse u --verbose, it will still terse the output. I think the best way is to document that --verbose and --terse are not opposites.

durin42 accepted this revision.Aug 11 2017, 2:43 PM

I've heard no objections, but also the suggestion that --terse be something like --terse-dirs or similar so it's obviously not the antonym of --verbose.

This revision is now accepted and ready to land.Aug 11 2017, 2:43 PM
This revision was automatically updated to reflect the committed changes.
martinvonz added inline comments.Aug 19 2017, 1:31 AM
tests/test-bisect.t
190

What does None mean here?

pulkit added inline comments.Aug 19 2017, 7:00 PM
tests/test-bisect.t
190

Oh, _conflictsmsg() is returning None which is getting printed. I will send a follow-up for this.