Daniel Sahlberg wrote on Thu, Aug 26, 2021 at 14:44:04 +0200: > Den lör 21 aug. 2021 kl 05:18 skrev Daniel Shahaf <d...@daniel.shahaf.name>: > > > Daniel Sahlberg wrote on Fri, 20 Aug 2021 10:30 +00:00: > > > Den fre 20 aug. 2021 kl 12:11 skrev Daniel Shahaf < > > d...@daniel.shahaf.name>: > > > > Daniel Sahlberg wrote on Thu, Aug 19, 2021 at 23:23:49 +0200: > > > > > [[[ > > > > ⋮ > > > > > > > ------------------------------------------------------------------------ > > > > > r4 | dsahlberg | 2021-08-19 21:58:28 +0200 (Thu, 19 Aug 2021) | 1 > > line > > > > > Merged via: r7, r5 > > > > > ]]] > > > > > As can be seen here, it is not visible in the plain text output that > > r7 was > > > > > actually a reverse merge. > > > > > > > > Hmm. There's an interesting question of whether we can amend the > > output > > > > to make that clear. The CLI output is an API, and generally we try not > > > > to change it unless the user opts in to new features (e.g., when > > > > changelists were added, the CLI output for users who don't use > > > > changelists wasn't changed), but there are exceptions (e.g., adding the > > > > seventh column to `svn status` non-XML output). > > > > > > The two options today are: > > > - Reverse merged via: r7 > > > or: > > > - Merged via: r7, r5 > > > > > > The code is looking at the the way r4 was merged and then prints the > > > revision numbers from the merge stack. With the patch we have enough > > > information on the merge stack to print "(reverse)" on each revision > > > that was subtractive, for example: > > > - Merged via: r7 (reverse), r5 > > > > > > Another option would be to print both (and each revision only in the > > > line where it belongs). > > > - Reverse merged via: r7 > > > - Merged via: r5 > > > To do this one would have to loop through the merge_stack twice (at > > > least) or keep a reasonably long buffer to store the list of revisions > > > in case of a long stack. > > > > > > > Hmm. In favour of the second option is that it doesn't require CLI- > > parsing tools to learn about new syntax, but only to learn about two > > existing syntaxes being able to appear concurrently. With a little > > luck, some of them will DTRT. > > > > Is there anything the first option can represent that the second one > > can't? Perhaps some tree-like cases (rN was a merge of both rFOO and > > rBAR), but those aren't representable in the plain output anyway. > > > > > For a buffer, we have svn_stringbuf_t that automatically reallocates > > itself as needed; and in any case, a two-pass algorithm would be easy to > > implement and to read and performant (it'd still be O(N)). > > > > > Both these changes might mess it up for someone consuming the CLI > > > output and expecting to find one or the other of Merged via: or Reverse > > > merged via:, or expecting to find just a simple list of revisions. I > > > can't judge the risk/reward for changing this. > > > > Would it be accurate to say that the incumbent output is wrong, > > misleading, a bug? > > > > It would at least be misleading, since it isn't possible to figure out if a > merge some way down the tree was a forward or reverse merge. But that would > be easy to discover when looking at the code or inspecting the merges more > closely.
I'm not sure if "that would be easy to discover" is good enough a reason. That's an argument could also be used to justify, say, deleting the «svn diff» command. Determining reverse merges is something we can do easily and would be useful, so why not do it? > If so, we could make the change, as a bugfix. The release notes could > > point it out and point to the XML output as a more-stable alternative. > > > > If not, we could record the idea as a milestone=2.0 issue. > > > > I have let this question soak for a few days hoping someone else with more > experience would chime in. > Personally, I think I'm leaning towards "it's misleading". I'd probably prefer this alternative — > > > Another option would be to print both (and each revision only in the > > > line where it belongs). > > > - Reverse merged via: r7 > > > - Merged via: r5 — since it requires the minimal change to parsers: only to be aware that if a "Reverse merged" line is found, there may be a "Merged via" line before the next blank line. I expect few parsers would need to be adjusted for this, and it would be more friendly to humans. However, this _would_ be an incompatible change for anyone parsing only the "Merged via" line, so I can certainly see the logic behind relegating this to 2.0… … except that there's nothing preventing us from adding _another_ line to the output, alongside the existing «Merged via: r7, r5» line, to indicate the merge directions with. > I'm leaning towards setting this as a milestone=2.0 issue and for the time > leaving it alone not to risk destabilising anything. That's an option. Or we could include this change in the next alpha or beta release to get feedback on it. (We might even be able to use a preprocessor timebomb so the change is automatically reverted between the last beta and rc1 unless we take positive action to keep it; cf. r1889012.) > Have you reviewed the patch from a coding style perspective? I would > appreciate a +1 from someone before committing. I have now. A few things: - It's useful to describe the issue (e.g., copy its title) rather than refer to it just by number. (subject line) - Indentation was mangled by emailing, so I didn't review it. - Some lines appeared to exceed the 80 columns limit. - I'd have declared «int i;» twice in the two relevant blocks, rather than moved it to function scope, so it would have the least possible scope. That would convert some bugs (using «i» in a wrong place) into compile errors (using an undeclared variable). + Order of elements of the new struct is good: wider types before narrower ones. (Matters for padding bytes.) That's a review for style. I didn't review correctness. Cheers, Daniel
