( )⚙ D2897 fix: new extension for automatically modifying file contents

This is an archive of the discontinued Mercurial Phabricator instance.

fix: new extension for automatically modifying file contents
ClosedPublic

Authored by hooper on Mar 19 2018, 3:40 PM.

Details

Summary

This change implements most of the corresponding proposal as discussed at the
4.4 and 4.6 sprints: https://www.mercurial-scm.org/wiki/AutomaticFormattingPlan

This change notably does not include parallel execution of the formatter/fixer
tools. It does allow for implementing that without affecting other areas of the
code.

I believe the test coverage to be good, but this is a hotbed of corner cases.

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

hooper created this revision.Mar 19 2018, 3:40 PM
pulkit added a subscriber: pulkit.Mar 20 2018, 3:12 AM

Haven't reviewed the patch but I think fix is too generic for this use case. What about format? Sorry for not having this idea when we had couple of discussion on it.

In D2897#46656, @pulkit wrote:

Haven't reviewed the patch but I think fix is too generic for this use case. What about format? Sorry for not having this idea when we had couple of discussion on it.

I will do a final pass to rename it if desired, but I would like to wait for any other feedback before doing so.

I think "format" is accurate for how we expect this to be used, but I also think it may be overly specific. I don't have any good alternatives in mind. "fix" is problematic because it is confusing in contexts like commit messages.

hooper updated this revision to Diff 7165.Mar 20 2018, 5:18 PM

Can you add a test where fixing of one parent commit leads to a child commit becoming empty. For example:

Commit 1: echo foo> a
Commit 2: echo Foo > a

And I run a lexer on commit 1 and 2, to change everything to uppercase.

hgext/fix.py
461

we can drop this if-else now since things are moved to core.

tests/test-fix-topology.t
19

In core we do '#if obsstore-on' or '#if obsstore-off'.

hooper updated this revision to Diff 7329.Mar 26 2018, 6:10 PM
hooper marked 2 inline comments as done.Mar 26 2018, 6:11 PM
In D2897#47695, @pulkit wrote:

Can you add a test where fixing of one parent commit leads to a child commit becoming empty. For example:
Commit 1: echo foo> a
Commit 2: echo Foo > a
And I run a lexer on commit 1 and 2, to change everything to uppercase.

Done.

The code except of minor nits looks good to me. I will like others to chime in on whether we should rename the command and extension to format because fix is too generic. I will push it once we get the naming and these comments sorted out.
cc: @yuja @indygreg @martinvonz @durin42 @krbullock

tests/test-fix.t
814

This should respect experimental.evolution.allowunstable and error out if set to False.

839

I am not sure how it should interact with ui.allowemptycommit. Maybe add a TODO here about this.

858

When the working directory parent get obsoleted because of fix, we should update to it's successor similar to what other commands do.

yuja added a comment.Mar 29 2018, 9:35 AM
In D2897#47875, @pulkit wrote:

The code except of minor nits looks good to me. I will like others to chime in on whether we should rename the command and extension to format because fix is too generic.

I agree the extension name fix is too generic, but I have no strong opinion.
It could be format, formatter, extfmt, extformatter, etc., but I guess
this isn't just for "formatting" in Google? And hg fix is easy to type. ;)

In D2897#47944, @yuja wrote:
In D2897#47875, @pulkit wrote:

The code except of minor nits looks good to me. I will like others to chime in on whether we should rename the command and extension to format because fix is too generic.

I agree the extension name fix is too generic, but I have no strong opinion.
It could be format, formatter, extfmt, extformatter, etc., but I guess
this isn't just for "formatting" in Google? And hg fix is easy to type. ;)

One possibility to consider is a config that runs a linter tool on the changed lines, but makes no edits. In the proposal I also gave the example of a config to sort the lines in a file. Another thought was spell checking/fixing. Implying that the command is only useful for source code formatting is selling the feature short, but still I wouldn't know what else to call it. We had picked "fix" because our users are familiar with the term.

hooper marked an inline comment as done.Mar 29 2018, 5:36 PM
hooper updated this revision to Diff 7366.
hooper added inline comments.Mar 29 2018, 5:36 PM
tests/test-fix.t
814

Maybe the comment is poorly worded. I mean that an incorrect implementation could create an orphan in this case, even though both commits were meant to be replaced.

I'm adding a separate test case for aborting if the user asks to create an orphan. I imitated an abort from histedit, which seems like it should be a utility function (checknodescendants).

858

Would it abort or merge if the working copy is dirty? Is that behavior expected to end up in cleanupnodes? It could also just force --working-dir. It's similar to creating an orphan if we don't fix the dirty working copy.

One of my goals is to avoid any need for merging or conflict resolution in this command.

pulkit accepted this revision.Mar 30 2018, 2:55 PM
pulkit added inline comments.
tests/test-fix.t
858

One of my goals is to avoid any need for merging or conflict resolution in this command.

That makes sense. I am good with current behavior then.

This revision was automatically updated to reflect the committed changes.