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. 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. I'm leaning towards setting this as a milestone=2.0 issue and for the time leaving it alone not to risk destabilising anything. Have you reviewed the patch from a coding style perspective? I would appreciate a +1 from someone before committing. Kind regards, Daniel
