On 26.01.2018 09:14, Johan Corveleyn wrote:
> On Fri, Jan 26, 2018 at 8:33 AM, Branko Čibej <brane_at_apache.org> wrote:
>> On 25.01.2018 19:47, Johan Corveleyn wrote:
>>> On Thu, Jan 25, 2018 at 1:41 PM, Julian Foad <julianfoad_at_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.
Yup. You're welcome.
> 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.
That's the same as saying that "svn merge" should solve any particular
tree conflict a given way by default and only start asking questions if
given some option. After all, the solution to the vast majority of tree
(and even text) conflicts is usually obvious.
The case for "svn patch" not doing something silently by default is the
same as for "svn merge". Granted that a silent replacement may not be
what the user wants, it is what the user *said*. If there's a conflict
the logical thing to do is to invoke the conflict resolver. That implies
providing the --accept option for default choices.
In this thread we've also ignored a subtle distinction between two cases:
Case A:
$ svn rm foo
$ svn patch x.patch
where x.patch contains a hunk that represents a modification of the
contents of file foo
Case B:
$ svn rm foo
$ svn patch x.patch
where x.patch contains a hunk that represents the creation of file foo
The original post is about case B, but case A is also important. Case A
is a delete/modify conflict whereas case B is a delete/add conflict.
Regardless of what "svn patch" actually does today, I propose that we
should be treating these cases differently. Using --accept=theirs
should, in case A: revert the deletion and apply the edit; in case B:
record a replacement. If the conflict resolver were invoked, it should
offer both options in both cases (and again, I don't know what it
actually does in equivalent cases during merge).
-- Brane
Received on 2018-01-26 09:36:37 CET