| Message ID | 20220808053459.184367-3-greenfoo@u92.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mergetools: vimdiff: regression fix (vimdiff3 mode) | expand |
On Mon, Aug 8, 2022 at 12:35 AM Fernando Ramos <greenfoo@u92.eu> wrote: > > vimdiff3 was introduced in 7c147b77d3 (mergetools: add vimdiff3 mode, > 2014-04-20) and then partially broken in 0041797449 (vimdiff: new > implementation with layout support, 2022-03-30) in two ways: > > - It does not show colors unless the user has "set hidden" in his > .vimrc file > > - It prompts the user to "Press ENTER" every time it runs. For the record, in my version these two issues are fixed in a much simpler way: --- a/mergetools/vimdiff +++ b/mergetools/vimdiff @@ -325,7 +325,7 @@ gen_cmd () { if ! echo "$LAYOUT" | grep ",\|/" >/dev/null then buf=$(get_buf "$LAYOUT") - FINAL_CMD="-c \"echo | ${buf}b | bufdo diffthis\" -c \"tabfirst\"" + FINAL_CMD="-c \"echo | set hidden | ${buf}b | silent bufdo diffthis\" -c \"tabfirst\"" return fi > # Add an extra "-c" option to move to the first tab (notice that we > # can't simply append the command to the previous "-c" string as > # explained here: https://github.com/vim/vim/issues/9076 > > - FINAL_CMD="-c \"$CMD\" -c \"tabfirst\"" > + FINAL_CMD="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"$CMD\" -c \"tabfirst\"" > } These diffopt settings look awfully familiar. https://lore.kernel.org/git/20220807024941.222018-7-felipe.contreras@gmail.com/
On 22/08/08 01:37AM, Felipe Contreras wrote: > On Mon, Aug 8, 2022 at 12:35 AM Fernando Ramos <greenfoo@u92.eu> wrote: > > > > vimdiff3 was introduced in 7c147b77d3 (mergetools: add vimdiff3 mode, > > 2014-04-20) and then partially broken in 0041797449 (vimdiff: new > > implementation with layout support, 2022-03-30) in two ways: > > > > - It does not show colors unless the user has "set hidden" in his > > .vimrc file > > > > - It prompts the user to "Press ENTER" every time it runs. > > For the record, in my version these two issues are fixed in a much simpler way: > Yes, it was simpler but remember it had two small issues: 1. In "vimdiff3" mode, if you switch to buffers #2 or #3, highlighting disappears. 2. It treats a single tab with a single window as a special case, when in fact it is just a subcase of a layout with many tabs where one of them contains just one window. The new patch series makes no distinction between them by keeping track of the number of windows opened on each tab which, as you noted, adds some extra complexity (but needed complexity nevertheless if we want to have highlighting enabled in all cases) > > # Add an extra "-c" option to move to the first tab (notice that we > > # can't simply append the command to the previous "-c" string as > > # explained here: https://github.com/vim/vim/issues/9076 > > > > - FINAL_CMD="-c \"$CMD\" -c \"tabfirst\"" > > + FINAL_CMD="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"$CMD\" -c \"tabfirst\"" > > } > > These diffopt settings look awfully familiar. > I would go as far as saying they are the same :) As you explained, it is better to keep these options explicitly set so that buffer diff'ing works in all cases. Notice that in this new patch series, however, these options apply to all layouts (and not just to "vimdiff3"), as we want highlighting to also be enabled in multi-tab single window layouts. PS: I have been testing many layouts today with and without an empty .vimrc and everything seems to work. But it would be great if others reading this did the same to make sure there are no other strange vim configuration options that affect the way diffs are displayed (as, unfortunately, we have found out in the past more than once!) Thanks!
On Mon, Aug 8, 2022 at 1:14 PM Fernando Ramos <greenfoo@u92.eu> wrote: > > On 22/08/08 01:37AM, Felipe Contreras wrote: > > On Mon, Aug 8, 2022 at 12:35 AM Fernando Ramos <greenfoo@u92.eu> wrote: > > > > > > vimdiff3 was introduced in 7c147b77d3 (mergetools: add vimdiff3 mode, > > > 2014-04-20) and then partially broken in 0041797449 (vimdiff: new > > > implementation with layout support, 2022-03-30) in two ways: > > > > > > - It does not show colors unless the user has "set hidden" in his > > > .vimrc file > > > > > > - It prompts the user to "Press ENTER" every time it runs. > > > > For the record, in my version these two issues are fixed in a much simpler way: > > > > Yes, it was simpler but remember it had two small issues: > > 1. In "vimdiff3" mode, if you switch to buffers #2 or #3, highlighting > disappears. No. That only happens in patch 9. In patch 5--which is where those two bugs are resolved--that problem doesn't exist yet. Also, I'm pretty sure that's a bug in vim (of which there are many). > 2. It treats a single tab with a single window as a special case, when in > fact it is just a subcase of a layout with many tabs where one of them > contains just one window. > The new patch series makes no distinction between them by keeping track > of the number of windows opened on each tab which, as you noted, adds > some extra complexity (but needed complexity nevertheless if we want to > have highlighting enabled in all cases) That's not necessarily true. You are assuming that is the only solution possible. Even supposing your solution was the only solution possible, nothing prevents us from applying your patch on top of mine. In git (and in many other endeavors) it behooves us to do one thing at a time for many reasons. There's no reason to try to do two things at the same time. We can fix the specific case now (which is urgent and needed), and explore the generic case later on (which few people would care about anyway). For the generic case, I took a look at your solution and noticed most of the complexity comes from trying to guess the number of windows per tab. There is no need to do that. I experimented with doing "bufdo diffthis" even in cases with multiple windows *before* doing anything else, and it works. There's no need to do "bufdo diffthis" afterwards, and there's no need for "windo diffthis" either. There's also no need to store the current buffer in a variable. It *also* has the added benefit that now multi-window tabs now show the diff colors for all the buffers, not just the visible ones (which is what I would have expected anyway). This solution is not only simpler than your solution, it's actually simpler than the current code. --- a/mergetools/vimdiff +++ b/mergetools/vimdiff @@ -68,7 +68,7 @@ gen_cmd_aux () { if test -z "$CMD" then - CMD="echo" # vim "nop" operator + CMD="silent execute 'bufdo diffthis'" fi start=0 @@ -221,7 +221,7 @@ gen_cmd_aux () { if ! test -z "$index_new_tab" then - before="-tabnew" + before="-tabnew | silent execute 'bufdo diffthis'" after="tabnext" index=$index_new_tab terminate="true" @@ -336,17 +336,6 @@ gen_cmd () { CMD=$(gen_cmd_aux "$LAYOUT") - # Adjust the just obtained script depending on whether more than one - # windows are visible or not - - if echo "$LAYOUT" | grep ",\|/" >/dev/null - then - CMD="$CMD | tabdo windo diffthis" - else - CMD="$CMD | bufdo diffthis" - fi - - # Add an extra "-c" option to move to the first tab (notice that we # can't simply append the command to the previous "-c" string as # explained here: https://github.com/vim/vim/issues/9076 > > > # Add an extra "-c" option to move to the first tab (notice that we > > > # can't simply append the command to the previous "-c" string as > > > # explained here: https://github.com/vim/vim/issues/9076 > > > > > > - FINAL_CMD="-c \"$CMD\" -c \"tabfirst\"" > > > + FINAL_CMD="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"$CMD\" -c \"tabfirst\"" > > > } > > > > These diffopt settings look awfully familiar. > > I would go as far as saying they are the same :) That's actually my point. You copied one of my fixes without mentioning it. Not only is it not nice to copy code without attribution, it's not a good practice to sneak in unrelated changes. If later on it turns out the diffopt stuff introduce a regression, people will have a harder time figuring out what in this patch triggered the issue especially since it's not mentioned. The diffopt fix is completely separate from what you are trying to do in this patch. It's good practice to do those kinds of fixes in a separate patch. My patch [1] does only one thing, and it explains why in the commit message. > As you explained, it is better to keep these options explicitly set so that > buffer diff'ing works in all cases. > > Notice that in this new patch series, however, these options apply to all > layouts (and not just to "vimdiff3"), as we want highlighting to also be > enabled in multi-tab single window layouts. Yes, but still: it should be done in a separate patch and explained why. Cheers. [1] https://lore.kernel.org/git/20220807024941.222018-7-felipe.contreras@gmail.com/
On 22/08/08 04:00PM, Felipe Contreras wrote: > > No. That only happens in patch 9. In patch 5--which is where those two > bugs are resolved--that problem doesn't exist yet. > Sure. I was referring to your whole patch series (which is what I was testing) not to a particular commit. If I only apply some of your patches it is true that (1) is no longer an issue, but we still have to deal with (2). > Also, I'm pretty sure that's a bug in vim (of which there are many). > In any case it doesn't happen with v3 :) > > 2. It treats a single tab with a single window as a special case, when in > > fact it is just a subcase of a layout with many tabs where one of them > > contains just one window. > > The new patch series makes no distinction between them by keeping track > > of the number of windows opened on each tab which, as you noted, adds > > some extra complexity (but needed complexity nevertheless if we want to > > have highlighting enabled in all cases) > > That's not necessarily true. You are assuming that is the only > solution possible. Only solution possible? Of course not! (I'm sorry if you got that impression) I'm just presenting one solution that works, but I'm sure there are many others (we could use "vim -d" in all cases or even implement a completely different parsing logic). Do you think we should try a different approach from the currently proposed one? > Even supposing your solution was the only solution possible, nothing > prevents us from applying your patch on top of mine. In git (and in > many other endeavors) it behooves us to do one thing at a time for > many reasons. > > There's no reason to try to do two things at the same time. We can fix > the specific case now (which is urgent and needed), and explore the > generic case later on (which few people would care about anyway). > You mean to apply your patch series and then apply mine? Sure, why not? But what do get get from that? (I'm just curious) I mean... v3 already works in all cases, right? Or am I missing something? > For the generic case, I took a look at your solution and noticed most > of the complexity comes from trying to guess the number of windows per > tab. There is no need to do that. > > I experimented with doing "bufdo diffthis" even in cases with multiple > windows *before* doing anything else, and it works. There's no need to > do "bufdo diffthis" afterwards, and there's no need for "windo > diffthis" either. There's also no need to store the current buffer in > a variable. > > It *also* has the added benefit that now multi-window tabs now show > the diff colors for all the buffers, not just the visible ones (which > is what I would have expected anyway). > Oh! Ok, now I see where the confusion comes from: showing the diff colors only for the visible buffers is *a desired property* from the original design and not something we want to avoid (except for the case where there is just one window, which is what we are fixing now). This is actually why all of this work started: we want to see, in one tab, only differences between BASE and LOCAL and, in another tab, only differences between BASE and REMOTE *without extra diff noise*. If we enable ":bufdo diffthis" in those tabs we get a mess (at least for complex merges). Imagine this: - Tab #1: classical four windows layout (LOCAL, BASE, REMOTE on top and MERGED on the bottom). - Tab #2: two windows: left BASE, right REMOTE - Tab #3: two windows: left BASE, right LOCAL We already see all highgliting and colors in the 4-way diff in tab #1, but we are only interested in the sepecific BASE vs REMOTE and BASE vs LOCAL in tabs #2 and #3. > > I would go as far as saying they are the same :) > > That's actually my point. You copied one of my fixes without > mentioning it. Not only is it not nice to copy code without > attribution, > You must excuse me here... I still find it extremely confusing to figure out which field to use for attribution each time. I added you to the "CC:" footer of the commit. Is this not the right procedure? Should I have done something different? > ...it's not a good practice to sneak in unrelated changes. Should we split the patch series in more commits, then? Something like this? 1. Comment fix 2. Keep track of windows opened per tab 3. Update generated string 4. Add "set" options 5. Update unit tests I mean... I'm right with that sure :) Note, however that 2, 3 and 4 are closely related (ie. "vimdiff3" won't work until the 3 of them are merged)
On Mon, Aug 8, 2022 at 4:51 PM Fernando Ramos <greenfoo@u92.eu> wrote: > > On 22/08/08 04:00PM, Felipe Contreras wrote: > > > > No. That only happens in patch 9. In patch 5--which is where those two > > bugs are resolved--that problem doesn't exist yet. > > > > Sure. I was referring to your whole patch series (which is what I was testing) > not to a particular commit. And I was referring to these changes: --- a/mergetools/vimdiff +++ b/mergetools/vimdiff @@ -325,7 +325,7 @@ gen_cmd () { if ! echo "$LAYOUT" | grep ",\|/" >/dev/null then buf=$(get_buf "$LAYOUT") - FINAL_CMD="-c \"echo | ${buf}b | bufdo diffthis\" -c \"tabfirst\"" + FINAL_CMD="-c \"echo | set hidden | ${buf}b | silent bufdo diffthis\" -c \"tabfirst\"" return fi Which do fix the two issues in a much simpler way. As I said. > If I only apply some of your patches it is true that (1) is no longer an issue, > but we still have to deal with (2). Do we? Fixing regressions should take precedence over adding new features, especially if nobody asked for them. > > Also, I'm pretty sure that's a bug in vim (of which there are many). > > In any case it doesn't happen with v3 :) Neither does it happen in my v2 patch series minus the last patch. > > > 2. It treats a single tab with a single window as a special case, when in > > > fact it is just a subcase of a layout with many tabs where one of them > > > contains just one window. > > > The new patch series makes no distinction between them by keeping track > > > of the number of windows opened on each tab which, as you noted, adds > > > some extra complexity (but needed complexity nevertheless if we want to > > > have highlighting enabled in all cases) > > > > That's not necessarily true. You are assuming that is the only > > solution possible. > > Only solution possible? Of course not! (I'm sorry if you got that impression) > > I'm just presenting one solution that works, but I'm sure there are many others > (we could use "vim -d" in all cases or even implement a completely different > parsing logic). > > Do you think we should try a different approach from the currently proposed one? Yes. The proposed solution is too complex, and it seems to be trying to workaround the current logic rather than fixing it. It should be pretty clear at this point that we want to do something either before, or after a tab is populated. So it would make perfect sense to have a function that receives all the string that defines a tab (text between "+"s). Instead, the logic treats "+" as other separators, which is not the case, for example, this clearly doesn't make sense: MERGED/(LOCAL+BASE+REMOTE) Yet the code acts as if it does. I wrote a patch that splits the tab parsing logic outside of gen_cmd_aux() and the result is much simpler: https://lore.kernel.org/git/20220809004549.123020-7-felipe.contreras@gmail.com/ > > Even supposing your solution was the only solution possible, nothing > > prevents us from applying your patch on top of mine. In git (and in > > many other endeavors) it behooves us to do one thing at a time for > > many reasons. > > > > There's no reason to try to do two things at the same time. We can fix > > the specific case now (which is urgent and needed), and explore the > > generic case later on (which few people would care about anyway). > > You mean to apply your patch series and then apply mine? Sure, why not? But what > do get get from that? (I'm just curious) We get a simple regression fix decoupled from complex new feature code. Additionally we get a little more readable code thanks to get_buf(), which still does make sense even in your patch. > I mean... v3 already works in all cases, right? Or am I missing something? "Work" is not the only metric. > > For the generic case, I took a look at your solution and noticed most > > of the complexity comes from trying to guess the number of windows per > > tab. There is no need to do that. > > > > I experimented with doing "bufdo diffthis" even in cases with multiple > > windows *before* doing anything else, and it works. There's no need to > > do "bufdo diffthis" afterwards, and there's no need for "windo > > diffthis" either. There's also no need to store the current buffer in > > a variable. > > > > It *also* has the added benefit that now multi-window tabs now show > > the diff colors for all the buffers, not just the visible ones (which > > is what I would have expected anyway). > > Oh! Ok, now I see where the confusion comes from: showing the diff colors only > for the visible buffers is *a desired property* from the original design and not > something we want to avoid (except for the case where there is just one window, > which is what we are fixing now). Who is "we"? I certainly don't want to avoid that, that's precisely what prevents me from using this layout feature. > This is actually why all of this work started: we want to see, in one tab, only > differences between BASE and LOCAL and, in another tab, only differences between > BASE and REMOTE You still will be able to see that. > *without extra diff noise*. I don't consider color hints due to a very real difference in the code "noise". This seems like a preference rather than something fundamental to the tool. > If we enable ":bufdo diffthis" in those tabs we get a mess (at least for complex > merges). > > Imagine this: > > - Tab #1: classical four windows layout (LOCAL, BASE, REMOTE on top and MERGED > on the bottom). > > - Tab #2: two windows: left BASE, right REMOTE > > - Tab #3: two windows: left BASE, right LOCAL > > We already see all highgliting and colors in the 4-way diff in tab #1, but we > are only interested in the sepecific BASE vs REMOTE and BASE vs LOCAL in tabs #2 > and #3. If "we" is you (plural), then sure, but *I* (and presumably other users) am not interested only in that. If we want to support both preferences the current code would not be amenable to that. If we move the tab parsing logic outside of gen_cmd_aux() as I proposed in my v3 patch, the logic is much simpler. > > > I would go as far as saying they are the same :) > > > > That's actually my point. You copied one of my fixes without > > mentioning it. Not only is it not nice to copy code without > > attribution, > > You must excuse me here... I still find it extremely confusing to figure out > which field to use for attribution each time. > > I added you to the "CC:" footer of the commit. Is this not the right procedure? > Should I have done something different? CC is just carbon copy, doesn't mean anything. You would have to ask the maintainer for what would be the "right procedure" but some people add "Helped-by". > > ...it's not a good practice to sneak in unrelated changes. > > Should we split the patch series in more commits, then? > Something like this? > > 1. Comment fix > 2. Keep track of windows opened per tab > 3. Update generated string > 4. Add "set" options > 5. Update unit tests > > I mean... I'm right with that sure :) Note, however that 2, 3 and 4 are closely > related (ie. "vimdiff3" won't work until the 3 of them are merged) Something like that, yeah. I've sent a new version of my patch series with a proposal. Tests generally should be updated along with the changes, it's tedious, but it forces you to rethink the changes you are making. It also helps other reviewers see what would be the actual result of your changes. Cheers. -- Felipe Contreras
diff --git a/mergetools/vimdiff b/mergetools/vimdiff index ea416adcaa..f8cd7a83f0 100644 --- a/mergetools/vimdiff +++ b/mergetools/vimdiff @@ -55,12 +55,40 @@ substring () { echo "$STRING" | cut -c$(( START + 1 ))-$(( START + $LEN )) } +enable_diff_mode () { + # Auxiliary function that appends extra vim commands at the end of each + # tab section to enable diff mode + + NUMBER_OF_WINDOWS_IN_TAB=$1 + + if test "$NUMBER_OF_WINDOWS_IN_TAB" -eq 1 + then + # Tabs that only contains one window will "diff" + # against all loaded/hidden buffers + + echo "let tmp=bufnr('%') | execute 'silent 1,4bufdo diffthis' | execute 'buffer '.tmp" + else + # Tabs that contain more than one window will + # only "diff" against those windows + + echo "execute 'windo diffthis'" + fi +} + gen_cmd_aux () { # Auxiliary function used from "gen_cmd()". # Read that other function documentation for more details. + # + # This function returns two items + # - STDOUT: The vim command to use + # - RETCODE: The number of windows opened in the current tab - LAYOUT=$1 - CMD=$2 # This is a second (hidden) argument used for recursion + WINDOWS_NR=$1 # Number of windows opened in the current tab after + # having parsed the provided "LAYOUT" + # If applicable, this variable will be updated and + # returned in RETCODE + LAYOUT=$2 # Substring (from the original LAYOUT) to process + CMD=$3 # This is a third (hidden) argument used for recursion debug_print debug_print "LAYOUT : $LAYOUT" @@ -232,6 +260,7 @@ gen_cmd_aux () { after="wincmd j" index=$index_horizontal_split terminate="true" + WINDOWS_NR=$(( WINDOWS_NR + 1 )) elif ! test -z "$index_vertical_split" then @@ -239,16 +268,27 @@ gen_cmd_aux () { after="wincmd l" index=$index_vertical_split terminate="true" + WINDOWS_NR=$(( WINDOWS_NR + 1 )) fi if test "$terminate" = "true" then CMD="$CMD | $before" - CMD=$(gen_cmd_aux "$(substring "$LAYOUT" "$start" "$(( index - start ))")" "$CMD") + CMD=$(gen_cmd_aux $WINDOWS_NR "$(substring "$LAYOUT" "$start" "$(( index - start ))")" "$CMD") + WINDOWS_NR=$? + + if ! test -z "$index_new_tab" + then + CMD="$CMD | $(enable_diff_mode $WINDOWS_NR)" + WINDOWS_NR=1 + fi + CMD="$CMD | $after" - CMD=$(gen_cmd_aux "$(substring "$LAYOUT" "$(( index + 1 ))" "$(( ${#LAYOUT} - index ))")" "$CMD") + CMD=$(gen_cmd_aux $WINDOWS_NR "$(substring "$LAYOUT" "$(( index + 1 ))" "$(( ${#LAYOUT} - index ))")" "$CMD") + WINDOWS_NR=$? + echo "$CMD" - return + return $WINDOWS_NR fi @@ -280,10 +320,9 @@ gen_cmd_aux () { fi echo "$CMD" - return + return $WINDOWS_NR } - gen_cmd () { # This function returns (in global variable FINAL_CMD) the string that # you can use when invoking "vim" (as shown next) to obtain a given @@ -333,25 +372,15 @@ gen_cmd () { # Obtain the first part of vim "-c" option to obtain the desired layout - CMD=$(gen_cmd_aux "$LAYOUT") - - - # Adjust the just obtained script depending on whether more than one - # windows are visible or not - - if echo "$LAYOUT" | grep ",\|/" >/dev/null - then - CMD="$CMD | tabdo windo diffthis" - else - CMD="$CMD | bufdo diffthis" - fi + CMD=$(gen_cmd_aux 1 "$LAYOUT") + CMD="$CMD | $(enable_diff_mode $?)" # Add an extra "-c" option to move to the first tab (notice that we # can't simply append the command to the previous "-c" string as # explained here: https://github.com/vim/vim/issues/9076 - FINAL_CMD="-c \"$CMD\" -c \"tabfirst\"" + FINAL_CMD="-c \"set hidden diffopt-=hiddenoff diffopt-=closeoff\" -c \"$CMD\" -c \"tabfirst\"" }
vimdiff3 was introduced in 7c147b77d3 (mergetools: add vimdiff3 mode, 2014-04-20) and then partially broken in 0041797449 (vimdiff: new implementation with layout support, 2022-03-30) in two ways: - It does not show colors unless the user has "set hidden" in his .vimrc file - It prompts the user to "Press ENTER" every time it runs. This patch fixes both issues and, in adition: - Unifies the previously "special" case where LAYOUT contained one single tab with one single window. - Fixes colors in tabs with just one window. Cc: Felipe Contreras <felipe.contreras@gmail.com> Signed-off-by: Fernando Ramos <greenfoo@u92.eu> --- mergetools/vimdiff | 69 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 49 insertions(+), 20 deletions(-)