I'm not sure how general the "blank lines" heuristic actually is. I think this should be an opt-in feature based on a per-formatter config variable. You can start with an entry in "FIXER_ATTRS" and a check around line 685. Whatever we end up with, it needs to be documented in the module docstring (which is the source for hg help -e fix)

I don't have a list of examples handy, but I wouldn't be surprised if this produces bad results in relatively common cases. This tends to be subjective, but we don't want to ship a foot gun :)

Your ":deletion" suggestion is related to the possibility of supporting "zero-length byte offset ranges" or "zero-length line ranges" in addition to "line ranges." In general, I would recommend avoiding the temptation to complicate the interface between fix.py and formatter tools. Most of the formatters I've had to integrate with did not support "formatting a deletion" anyway. There are some important counter examples like clang-format.

Your ":deletion" suggestion is related to the possibility of supporting "zero-length byte offset ranges" or "zero-length line ranges" in addition to "line ranges." In general, I would recommend avoiding the temptation to complicate the interface between fix.py and formatter tools. Most of the formatters I've had to integrate with did not support "formatting a deletion" anyway. There are some important counter examples like clang-format.

Turns out that was my own comment about :deletion. I see this change as a workaround for formatters that don't explicitly support formatting around deleted sections, regardless of whether we ever implement a way to integrate with formatters that do support it.

I oppose enabling this as default behavior. A plausible way this could go wrong is if formatting an unchanged blank line also implicitly formats an unchanged block of code on the other side. I don't think we can assume that tools will never do this. I do think it would be surprising in a bad way for users, even if we technically don't make any guarantees about the minimal-ness of the diffs used to determine incremental formatting. Not formatting deletions also sucks, but it's less destructive.

Alright so as discussed, this is now a per-fixer config option which I called "adjacentblanks" (not married to it). There's also the new issue of having to plumb the flag down through the necessary layers of abstraction. I figured doing so with a fixer instance would be the least complex way but I'm not sure how idiomatic that is in hg's codebase.

@hooper can you please have a look again. Thank you!

Consider this C++ example:

#include <foo>

#include "foo.h"

Deleting the blank line would ideally lead clang-format to add the blank line back. Implementing this kind of behavior on the basis of adjacent blank lines will never provide a complete solution.

Now consider this example:

void foo() {
  int y;
  if (true) { 
                 int x;
  } 
}

Delete int y;, then format the two adjacent lines unconditionally. The indentation of int x; will be fixed, even though it has nothing to do with the diff. This behavior counteracts the purpose of the feature in an avoidable way.

I don't think there is any middle ground here, unless it involves language-aware analysis of the diff. I don't think that belongs here. A more robust solution is to communicate deletions in a precise way to the tools. For example, I think clang-format can reasonably accept it as a zero-length range at a byte offset. How to best express this in configuration is an open question, but it would maintain the separation of concerns between version control and language tools.

I think this change is reducing the robustness of the established interface. It provides some utility, but it also over-promises on its ability to handle deletions. It might be more appropriate as a separate extension that wraps the difflineranges function.

I'm definitely sympathetic to your position.

As discussed offline, I think an improvement to generality would be to change this config option to relate to deletion diff policies instead and have adjacent blanks be a policy therein.

I'm flexible on naming here. I think "emitdeletedranges" is a bit clunky but I can't think of something that fits much better.

PTAL and thanks!

The config changes are a big improvement, but I'm still not sure if it's worth supporting. One thing to note is that this feature doesn't help users who have multiple formatters behind a wrapper script configured as a single fixer tool. Configuring it as multiple fixer tools is possible, but not always practical, since it may duplicate some logic between the hg config and something else (sorry to be vague about that).

Okay the implementation itself looks better to me.

As for the multi-formatter considerations, I just don't think it should play a big role in whether this feature is worthwhile. This clearly addresses a configurability knob that didn't previously exist and I think that alone should dictate the viability of this patch.

Expanded the doctests a bit and factored out some of the index arithmetic.

Bump

The patch looks fine now.

I still think we shouldn't go down the road of implementing formatting behavior abstractly on top of the actual formatters. Ideally, we would pass flags to the formatter that ask it to do the right language-aware thing at the byte/line offset of a deletion. It's going to be awkward to support/document this if/when we get a solution like that.

In D9056#147377, @hooper wrote:

The patch looks fine now.

Great!

I still think we shouldn't go down the road of implementing formatting behavior abstractly on top of the actual formatters. Ideally, we would pass flags to the formatter that ask it to do the right language-aware thing at the byte/line offset of a deletion. It's going to be awkward to support/document this if/when we get a solution like that.

I agree in principle but I think the lack of support for those features in actual formatters indicates that this config option will still provide meaningful value to users. If/when popular formatters support this sort of adjacency detection, it may make sense to sunset this sort of feature but I'm not confident that will happen soon.

What's the next step here? Is there still disagreement on the value of the feature or does there need to be additional review(s)?

In D9056#149670, @msuozzo wrote:

What's the next step here? Is there still disagreement on the value of the feature or does there need to be additional review(s)?

Sorry lost track of the discussion. While the code change looks good, like @hooper I am not sure whether it's a good idea to do so.

Others: what do you think about this?