On Fri, Jan 26, 2018 at 8:33 AM, Branko Čibej <br...@apache.org> wrote: > On 25.01.2018 19:47, Johan Corveleyn wrote: >> On Thu, Jan 25, 2018 at 1:41 PM, Julian Foad <julianf...@apache.org> wrote: >>> Branko Čibej wrote: >>>> On 24.01.2018 22:32, Julian Foad wrote: >>>>> When 'svn patch' applies an 'add file' patch onto a WC path whose >>>>> local schedule is 'delete', it changes the schedule to 'replace'. >>>>> [...] >>>>> stsp and I discussed on IRC ( >>>>> http://colabti.org/irclogger/irclogger_log/svn-dev?date=2017-12-15#l19 >>>>> ) and agreed that this is not what users would generally want or expect. >>>>> >>>>> I propose to make 'patch' always generate a 'modified' (or unmodified) >>>>> schedule when it applies an 'add file' diff (or reverse-applies a >>>>> 'delete file' diff) onto a schedule 'deleted' working copy file. >>>>> [...] >>>> >>>> Why is this not what users would expect? "Delete" + "add" has always >>>> been "replace" in Subversion. The only other reasonable option I can >>>> think of would be to generate a delete/add tree conflict and let the >>>> user decide what to do about it. Silently undoing an "svn rm" in the >>>> working copy is exactly what I would _not_ expect. Both 'svn rm' and >>>> 'svn patch' are explicit user operations and we can't just assume that >>>> one or the other were mistakes. >>> >>> I'm not assuming anything was a mistake. >>> >>> Stefan commented in the IRC chat, "replacements are causing more grief than >>> good in general, especially if they happen by accident. i've seen people >>> block replacements in pre-commit hooks entirely so if we're given a choice >>> between having the default behaviour be replacement or modification, then >>> i'd always argue for modification by default. note also that many other vcs >>> don't have a replacement concept unless the node kind has changed and nobody >>> complains about that." >>> >>> The theoretical rationale is this. The patch format does not carry ancestry >>> information, not even implicitly. Whether a pair of patch operations should >>> cause a break in Subversion ancestry is a completely free choice for >>> Subversion to make. >>> >>> 'svn delete' and 'svn add' are explicit *Subversion* operations which carry >>> implications about ancestry (and after a local delete the user is free to >>> choose "svn revert" instead of "svn add" to get the other result). The 'add' >>> and 'delete' operations in a patch file are not and do not. >>> >>> Formally, yes, it would be nice to offer both outcomes. However, raising a >>> conflict without having a nice framework for setting up an automatic >>> conflict handling policy is just another barrier to users. >> +1 for not producing replacements by default. Intentional replacements >> are extremely rare compared to wanting to keep the ancestry intact. >> >> This makes me think of these old issues: >> >> https://issues.apache.org/jira/browse/SVN-3429 ("svn mv A B; svn mv B >> A" generates replace without history) >> >> https://issues.apache.org/jira/browse/SVN-4302 (move and move back >> breaks nested moves) >> >> I'm glad I don't have to deal with those unintended replacements anymore. > > Choosing short-term convenience over correctness is the Git way, not the > Subversion way. The Subversion way is to attain convenient correctness. > This kind of thoughtless +1 is just intellectual laziness where instead > there should be discussion of tradeoffs and side effects.
Thanks, Brane. I should have expected such a reply from you, downplaying my answer as a "thoughtless +1" and "intellectual laziness". Very useful comment. Feel free to continue the discussion of tradeoffs and side effects, I didn't do anything to block that. > The issues you mention are actual bugs. Recording a replacement where a > replacement was intended is not. Making it impossible to record a > replacement when it was intended is a regression. Note that I said "+1 for not producing replacements *by default*". I didn't say we should make it impossible. I suppose in the 0,1 % of the cases where someone really wants a replacement, they should be able to use some option. -- Johan
