Would it make sense to break this patch in two? One patch could contain the implementation for :cfdo and :lfdo. The other patch could contain the implementation for :cdo and :ldo. I think that :cfdo is ready to merge. Whereas :cdo seems to be a bit more controversial and perhaps needs more work?
Drew On Wed, Jul 29, 2015 at 9:34 PM, Yegappan Lakshmanan <[email protected]> wrote: > Hi all, > > On Sun, Jul 26, 2015 at 12:22 PM, Yegappan Lakshmanan > <[email protected]> wrote: > > Hi, > > > > On Sun, Jul 26, 2015 at 4:53 AM, Drew Neil <[email protected]> > wrote: > >> I agree with h_east that if you’re planning to run the :substitute > command > >> across multiple files, it makes sense to use: > >> > >> :cfdo %s/pattern/replacement/g > >> > >> and not: > >> > >> :cdo s/pattern/replacement/g > >> > > > > Depending on the task, you can use either the ":cdo" or the ":cfdo" > commands. > > > > I haven't see any replies so far. I am not sure whether we are > agreeing to add both the > commands or only the cfdo/lfdo commands? Do you guys think that only > the cfdo/lfdo > commands will be useful? > > Regards, > Yegappan > > > > > If you want to perform text search/replace across all the files in the > quickfix > > list, then the ":cfdo" command with ":%s/.../g" is the correct option > (as it is > > more optimal). > > > > This is similar to using the "g" argument to the ":substitute" command to > > replace all the matching text in a single line. And using the "%" range > > to replace text in all the lines. If you want to replace multiple > matching > > text across all the lines in a file, then you have to pass both "%" and > "g". > > If you don't, then the text will not be correctly replaced. This is not a > > problem with the ":substitute" command. > > > >> > >> I can also see myself wanting to use the :cdo command in combination > with > >> :normal for certain types of task. But I’ve come across another problem. > >> Suppose that we have a text file containing these four lines of text: > >> > >> http://example.com > >> http://example.org > >> http://example.net > >> http://example.com http://example.org http://example.net > >> > >> Now let’s say that we want to turn each occurrence of ‘http’ to > ‘https’. (We > >> could use the :substitute command here, but let me use this to > demonstrate a > >> problem with using :normal). We’ll use :vimgrep to populate the quickfix > >> list with 6 matches: > >> > >> :vimgrep /http\zs:/g % > >> > >> Then we’ll insert the ’s’ character in front of the colon with this > command: > >> > >> :cdo normal is > >> > >> The resulting text looks like this: > >> > >> https://example.com > >> https://example.org > >> https://example.net > >> https://example.com httsp://example.org htstp://example.net > >> > >> In the last line, we get ‘https’, then ‘httsp’, then ‘htstp’. Not ideal! > >> > >> The problem here is that the quickfix list records line and column > numbers. > >> If characters are added or removed near the start of the line, the > column > >> numbers for later matches on that line will no longer line up with the > match > >> that created the original quickfix list entry. > >> > >> I’m not sure if this is a problem with the quickfix list, with :cdo, or > with > >> :normal. > >> > > > > This is a problem with the quickfix list functionality. Currently when a > line > > is added or removed, then the line numbers in the quickfix list entries > > are updated. But when a line is modified, the column numbers in the > > quickfix list entries are not updated. Refer to the qf_mark_adjust() > > function. > > > > In the above example, you should use ":s/../g" instead of the ":normal" > > command. > > > > Regards, > > Yegappan > > > >> > >> On Sat, Jul 25, 2015 at 6:22 PM, Yegappan Lakshmanan < > [email protected]> > >> wrote: > >>> > >>> Hi, > >>> > >>> On Sat, Jul 25, 2015 at 9:55 AM, h_east <[email protected]> wrote: > >>> > Hi Yegappan and Bram > >>> > > >>> > 2015-7-25(Sat) 12:27:56 UTC+9 [email protected]: > >>> > >>> >> Hi Hirohito, > >>> >> > >>> >> On Fri, Jul 24, 2015 at 3:42 PM, h_east wrote: > >>> >> > Hi Yegappan, Bram and List > >>> >> > > >>> >> >> >> > > >>> >> >> >> > Thanks for testing the patch. I will send out an updated > patch > >>> >> >> >> > in a few days. > >>> >> >> >> > Hopefully this time it will get included. This has been > >>> >> >> >> > outstanding for more > >>> >> >> >> > than two years. > >>> >> >> >> > > >>> >> >> >> > >>> >> >> >> The updated patch (against vim 7.4.796) is attached. > >>> >> >> > > >>> >> >> > Thanks. So now it's ready to include, right? > >>> >> >> > > >>> >> >> > >>> >> >> Yes. Of course :-) > >>> >> > > >>> >> > I confirmed this patch. > >>> >> > > >>> >> > I found unexpected behaviors. > >>> >> > > >>> >> > >>> >> Thanks for testing the patch and sending the bug report. I am > attaching > >>> >> an updated patch that fixes the two problems. Let me know if you see > >>> >> any > >>> >> issues with this attached patch. > >>> > > >>> > I confirmed that reported problem have been fixed. > >>> > Thank you for quickly fixes. > >>> > > >>> > I think it is better to discuss. > >>> >> > This is my opnion. > >>> >> > When the search pattern exists more in a row, I think :cdo/:ldo > >>> >> > confuse to use. > >>> >> > and the processing time tends to be long. > >>> > > >>> > Do you understand that the results of the following two commands are > >>> > different, > >>> > When the search pattern exists more in a row? > >>> > > >>> > >>> The ":cdo" command executes the supplied command for every valid entry > >>> in the quickfix list. It is upto the supplied command to perform the > >>> appropriate > >>> action for every entry. > >>> > >>> > > >>> > (1) :cdo s/\<cmdidx\>/ex_&/g | update > >>> > > >>> > >>> In this case, the supplied substitute command replaces all the > occurrences > >>> of > >>> cmdidx in the current line. > >>> > >>> > > >>> > (2) :exec "cdo norm!iex_\<Esc>:w\<CR>" > >>> > > >>> > >>> In this case, the supplied replaces only the first occurrence of > cmdidx. > >>> This is > >>> not a problem with the ":cdo" command. This is a problem with the user > >>> supplied > >>> command. > >>> > >>> > > >>> > The (1) is processed all search pattern. > >>> > But, The (2) is processed first search pattern in a row. > >>> > > >>> > >>> This is the expected behavior as this is a problem with the user > >>> supplied command. > >>> > >>> > > >>> > ':cdo' is not necessary, When use only :substitute. > >>> > > >>> > When we use the ':cfdo' command such as ':cdo', Speed is also faster. > >>> > > >>> > :cfdo %s/\<cmdidx\>/ex_&/g | update > >>> > > >>> > So I propose to including patch only ':cfdo' and ':lfdo'. > >>> > > >>> > How do you think? > >>> > > >>> > >>> No. In some cases the ":cdo/:ldo" commands are useful and in some > >>> other cases ":cfdo/:lfdo" commands are useful. > >>> > >>> You are assuming that the ":cdo/:cfdo" commands will only be used > >>> to perform substitutions and the results in the quickfix/location lists > >>> are > >>> from a search command (e.g. vimgrep). This is not always the case. > >>> You can populate the quickfix list with output from various tools > >>> (e.g. cscope, tags, lid, global, build output, static analysis output, > >>> etc.). > >>> > >>> Regards, > >>> Yegappan > >>> > >>> >> > > >>> >> > Case#1 > >>> >> > How to reproduce: > >>> >> > 1. cd to vim src dir. > >>> >> > $ cd (Vim clone dir)/vim/src > >>> >> > 2. Start Vim. (including this patch version Vim) > >>> >> > $ vim -N -u NONE > >>> >> > 3. Grep word "cmdidx" from source and header using vimgrep. > >>> >> > :vimgrep "\<cmdidx\>" **/*.[ch] > >>> >> > 4. Open quickfix window. > >>> >> > :copen > >>> >> > 5. Do :cdo command. (Intentionally forget the '| update') > >>> >> > :cdo s/\<cmdidx\>/ex_&/g > >>> >> > > >>> >> > Expect behavior: > >>> >> > - E37 occurs once. > >>> >> > > >>> >> > Actual behavior: > >>> >> > - E37 occurs continuously. > >>> >> > > >>> >> > > >>> >> > -------- > >>> >> > Case#2 > >>> >> > How to reproduce: > >>> >> > 1~4. (Same abobe.) > >>> >> > 5. Do :cdo command. (Intentionally forget the ":w\<CR>") > >>> >> > :exec "cdo norm!iex_\<Esc>" > >>> >> > > >>> >> > Expect behavior: > >>> >> > - E37 occurs once. > >>> >> > > >>> >> > Actual behavior: > >>> >> > - E37 occurs continuously. > >>> >> > > >>> >> > And, When press Ctrl-C after the '-- More --' display, buffer.c > was > >>> >> > modified unexpectedly. > >>> >> > > >>> >> > [original buffer.c:4901] > >>> >> > if (eap->cmdidx == CMD_unhide || eap->cmdidx == CMD_sunhide) > >>> >> > > >>> >> > [modified buffer.c:4901] > >>> >> > if > >>> >> > > (eap->exexexexexexexexexexexexexexexexexexexexexexexex___________________ > >>> >> > _____cmdidx == CMD_unhide || eap->cmdidx == CMD_sunhide) > >>> >> > > >>> >> > > >>> >> > -------- > >>> >> > This is my opnion. > >>> >> > When the search pattern exists more in a row, I think :cdo/:ldo > >>> >> > confuse to use. > >>> >> > and the processing time tends to be long. > >>> > > -- > -- > You received this message from the "vim_dev" maillist. > Do not top-post! Type your reply below the text you are replying to. > For more information, visit http://www.vim.org/maillist.php > > --- > You received this message because you are subscribed to a topic in the > Google Groups "vim_dev" group. > To unsubscribe from this topic, visit > https://groups.google.com/d/topic/vim_dev/dfyt-G6SMec/unsubscribe. > To unsubscribe from this group and all its topics, send an email to > [email protected]. > For more options, visit https://groups.google.com/d/optout. > -- -- You received this message from the "vim_dev" maillist. Do not top-post! Type your reply below the text you are replying to. For more information, visit http://www.vim.org/maillist.php --- You received this message because you are subscribed to the Google Groups "vim_dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
