Newline substitution behaviour

[GoogleCodeExporter]

Since patch 7.4.232 (http://markmail.org/message/vo7ruair5raccawp), vim 
recognizes 's/\n//' as a special case, and transforms it into a join command.

The generated join command involves lines in command line-range. Thus, 
'1,3s/\n//' joins lines 1, 2, and 3, which implies it only deletes 2 newlines 
(not 3). Also, for the particular case of just one line, 's/\n//' does nothing. 
I mean, do_join gets invoked, but it does nothing in that case.

Now, another interpretation would be possible, in which the generated join 
should involve lines in rage plus one. This is, '1,3s/\n//' would join 4 lines, 
effectively deleting 3 newlines, and 's/\n//' would join current line with the 
next one, deleting 1 newline.

Then, my question is: Is current behaviour the canonical one, or is it a bug? 
Div vim previous to 7.4.232 behave the same?

Thanks.

Original issue reported on code.google.com by eliseoma...@gmail.com on 27 Nov 2014 at 6:34

[GoogleCodeExporter]

I would say it is a bug and should behave as before the patch. 

Original comment by chrisbr...@googlemail.com on 27 Nov 2014 at 8:05

[GoogleCodeExporter]

But, are we sure about how it behaved before the patch? I don't have an old vim 
here to check.
Did it behave as in the second interpretation I gave above?

Thanks.

Original comment by eliseoma...@gmail.com on 27 Nov 2014 at 9:00

[GoogleCodeExporter]

Here is a patch.

Original comment by chrisbr...@googlemail.com on 28 Nov 2014 at 1:26

Attachments:

• issue_287.diff

[GoogleCodeExporter]

I've got some issues with that patch:
- I don't like modifying eap content (can have undesirable side-effects down 
the road).
- Edge case of single-line-range in last-line-of-file still calls do_join, with 
count == 1. This should be better avoided, IMO, because: 
  1) do_join precondition states it should be called with count >= 2.
  2) if called with count == 1, it does nothing useful anyway, so it can be saved.
- Reported subsitutions/lines are still wrong. If you join n lines, you should 
report n - 1 substitutions in n - 1 lines.

I attach an alternative patch fixing all that.

Original comment by eliseoma...@gmail.com on 28 Nov 2014 at 3:10

Attachments:

• issue_287_alt.patch

[GoogleCodeExporter]

I just built 7.4.231 and checked the behavior of '1,3s/\n//'.  The input (text 
between the triple-tick marks):

this is a test

and after running ':1,3s/\n//' :

thisisatest

That is, the three newlines within the range are deleted.  This means the 
correct optimization of the old :s behavior as a join is to join 4 lines.  In 
light of this, I would suggest the post-7.4.232 should be viewed as a bug.

For reference, the output in vim 7.4.488 is now:

thisisa test

Original comment by bangpath...@gmail.com on 28 Nov 2014 at 8:15

[GoogleCodeExporter]

I've noticed another strange behaviour of previous code, that is also fixed by 
my patch:

Previously (in the bugged code since 7.4.232), the single-line case ':s/\n//' 
was a no-op for what regards text changes. But it wasn't a complete no-op, as 
it still modified the last-change position, and reported 1 substitution in 1 
line. In other words, it introduced an "empty" change, that was visible with 
the :changes command, and undoable through undo.

After my patch, single-line case isn't no-op anymore, except in the very last 
line.
But in that case, our approach behaves correctly, resulting in a complete 
no-op. This is, no change is recorded, nor listed, nor undoable, which seems 
the correct thing to do.

Now, I don't know how vim pre-7.4.232 behaved regarding that. 
bangpath, could you please check that case?
Was an empty change recorded before? How many substitutions/lines were reported?

Thanks.

Original comment by eliseoma...@gmail.com on 28 Nov 2014 at 8:51

[GoogleCodeExporter]

eliseoma, 

Using my sample text above and running ':s/\n//' on the last line ("test"), I 
get:

<input the sample text in an empty buffer w/o final newline>
:changes
change line  col text
    1     4    3 test
>

In 7.4.231:

:s/\n//
:changes
change line  col text
    1     4    0 test
>

In 7.4.488:

:s/\n//
:changes
change line  col text
    1     4    4 test
>

The column position is updated by :s/\n// in each of pre- and post- 7.4.232, 
but to different values as shown.

Cheers,
John

Original comment by bangpath...@gmail.com on 28 Nov 2014 at 11:13

[GoogleCodeExporter]

Ok, bangpath. Thanks.

Personally, I think ':s/\n//' should be a complete no-op, when in the last 
line, so I'll leave my patch as-is, if nobody opposes.

Original comment by eliseoma...@gmail.com on 29 Nov 2014 at 8:43

[GoogleCodeExporter]

Okay, that works as well.

However your patch does not compile because of the lowercase true. 

Secondly, I personally do not think, changing the message about the changed 
lines is correct. Vim 7.4 reported "<addresscount> substitutions on 1 line", 
vim 7.4.232 reports "<addresscount> substitutions on <addresscount> lines" 
Which I personally think is correct, since this is the number of substitions 
performed.

But if we think about changing that regression, it should be changed as well 
back to the Vim 7.4. 

And since we are skipping the join command for the :$s/\n// anyhow, Vim should 
also skip the message reporting the number of substititutions.

So here is a final patch addressing all those remarks.

Original comment by chrisbr...@googlemail.com on 29 Nov 2014 at 12:02

Attachments:

• issue_287.diff

[GoogleCodeExporter]

> However your patch does not compile because of the lowercase true.

Sorry for that. I'm a contributor to the neovim project, and I compiled / 
tested my patch there (where that  parameter has been refactored into a bool), 
before transplanting it onto the last version of vim's corresponding file, just 
to generate the patch.

> Vim 7.4 reported "<addresscount> substitutions on 1 line", vim 7.4.232 
reports "<addresscount> substitutions on <addresscount> lines" Which I 
personally think is correct, since this is the number of substitions performed.

If by <addresscount> you mean the number of lines within the range, please 
notice it's not always true that the number of substitutions is equal to that. 
That's only true when range doesn't include last line. When it does, number of 
substitutions equals <addresscount> - 1. As that difference is already included 
in the definition of joined_lines_count, what's always true is that the number 
of subsitutions is equal to joined_lines_count - 1. Both last two patches are 
correct in this sense.

Regarding the number of lines affected, I think what should be considered 
correct depends on the intended meaning of the message. I mean, whether it 
refers to lines as they were before the change, or after it. I can only make 
sense of vim 7.4's behaviour if the "after" interpretation is taken. On the 
other hand, I personally think the "before" interpretation is more logical. And 
in that case, reported lines should be equal to reported substitutions. So, I 
think message meaning should be clarified to reach an agreement on this.

> And since we are skipping the join command for the :$s/\n// anyhow, Vim 
should also skip the message reporting the number of substitutions.

I think reporting user in the no-op edge case would be in fact good ui 
practice, as that can signal a user misunderstanding of the intended behaviour. 
But I do agree that, with current code, call to do_sub_msg with sub_nlines and 
sub_nsubs being 0, no output gets generated anyway, so it can be saved.

Not so sure about not updating sub_nlines and sub_nsubs, though:
- On one hand, given the operation is a no-op, it should have no side-effects. 
So, it would be ok leaving those globals reflecting the last no-op substitution 
result.
- On the other hand, I don't know if any other code can depend on those globals 
being updated to reflect the result of the last substitution, be it a no-op or 
not. 
I suppose if you opt for not updating them, it's because no such reliance on 
those globals exists.

So, in essence, I'm ok with the last version of the patch, except for the 
sub_nlines = 1 thing, until meaning of message is clarified.

Original comment by eliseoma...@gmail.com on 29 Nov 2014 at 8:35

[GoogleCodeExporter]

>> Vim 7.4 reported "<addresscount> substitutions on 1 line", vim 7.4.232
>> reports "<addresscount> substitutions on <addresscount> lines" Which I
>> personally think is correct, since this is the number of substitions
>> performed.

> If by <addresscount> you mean the number of lines within the range,
> please notice it's not always true that the number of substitutions is
> equal to that. That's only true when range doesn't include last line.

Sure, but I was just mentioning the pre 7.4.232 patch behaviour. And
that was independent of whether there was a linebreak substituted in the
last line. In fact, it could have even been true, that previous Vim did
replace that linebreak and it would be recreated when writing that buffer.

> Regarding the number of lines affected, I think what should be
> considered correct depends on the intended meaning of the message. I
> mean, whether it refers to lines as they were before the change, or
> after it. I can only make sense of vim 7.4's behaviour if the "after"
> interpretation is taken. On the other hand, I personally think the
> "before" interpretation is more logical. And in that case, reported
> lines should be equal to reported substitutions. So, I think message
> meaning should be clarified to reach an agreement on this.

While that is true, ':%s/\n//' will report the number of substitutions
as the number of lines affected, while with slightly different patterns it
will report "on 1 line" (e.g. :%s/\(.\)\n/\1/) and this is even more
confusing. That's why I was resetting the sub_nlines counter to 1 in my
patch.

Original comment by chrisbr...@googlemail.com on 1 Dec 2014 at 7:41

[GoogleCodeExporter]

Okay. I've made some other tests and it seems the "after" interpretation is the 
one taken elsewhere.
We agree on your last patch as the definitive one, then.
Thanks!

Original comment by eliseoma...@gmail.com on 2 Dec 2014 at 11:47

[GoogleCodeExporter]

Patch included as 7.4.543.  Plus fixing the search pattern history and 
highlighting.

Original comment by brammool...@gmail.com on 13 Dec 2014 at 2:18

• Changed state: Fixed