On Tue, Nov 26, 2024 at 11:15 PM Daniel Sahlberg <
daniel.l.sahlb...@gmail.com> wrote:
> Hi,
>
> Thanks for the detailed explanation.
>
> Offering some small inputs, but basically I think Nathan has made very
> good comments.
>
> Den tis 26 nov. 2024 kl 19:11 skrev Nathan Hartman <
> hartman.nat...@gmail.com>:
>
>> On Tue, Nov 26, 2024 at 12:23 PM Timofei Zhakov <t...@chemodax.net>
>> wrote:
>> >
>> > On Sun, Nov 24, 2024 at 5:25 PM Daniel Shahaf <d...@daniel.shahaf.name>
>> wrote:
>> > > Don't leak implementation details into the API/CLI. In CLI terms,
>> > > applying a patch isn't a kind of merge, notwithstanding the
>> libsvn_client
>> > > implementation details.
>> > >
>> > > In general, don't add --options that make a command do something
>> > > entirely different; rather, have each subcommand "do one thing and do
>> it
>> > > well".
>> > >
>> > > The standard example for that is «svn switch --relocate», now known as
>> > > «svn relocate».
>> >
>> > Hi,
>> >
>> > I'd like to discuss the command-line interface for working with
>> xpatches in
>> > detail. Firstly, I'm going to explain the CLI I was initially
>> suggesting:
>> >
>> > We have two basic operations in this area; Creation of xpatch files and
>> > their application onto a working copy.
>> >
>> > The creation step seems to be very similar to the plain `svn diff`
>> command,
>> > because `svn diff` handles different targets greatly. I mean that it can
>> > differentiate either the working copy changes, changes made in any
>> revision,
>> > two different trees, and any combination of the points above. I think
>> that
>> > it's very familiar to the user to use the `svn diff` command for such
>> an action,
>> > but changing the output format. I may also say that this command already
>> > accepts a bunch of different options, that change the output format. For
>> > example, `svn diff --summarize` and `svn diff --git`. In my opinion,
>> adding
>> > another command that does everything the same as `svn diff`, but
>> outputs the
>> > differences in xpatch format is a nice idea. I'll provide a list of
>> different
>> > invocations options of `svn diff` that we will have:
>> >
>> > - svn diff > ../patches/something.patch
>> > - svn diff --summarize
>> > - svn diff --git
>> > - svn diff --git
>> > - svn diff --xpatch
>> > - svn diff --xpatch > ../patches/something.xpatch
>> >
>> > This seems to be a great and structured interface.
>> >
>> > Afterwards, I'm going to dig into the application of the xpatch files
>> onto a
>> > working copy. This seems to be a bit more controversial than the
>> creation step.
>> > I'm currently suggesting to use the `svn merge` command for this. If
>> > so, to apply
>> > an xpatch, the user should use commands like `svn merge --xpatch
>> > FILENAME [WCPATH]`.
>> > Then SVN will read that xpatch file from FILENAME and apply it over a
>> working
>> > copy at WCPATH, or onto the current directory, if no WCPATH is given.
>> For now,
>> > we haven't changed any behaviour from `svn merge`, except the
>> > `--xpatch FILENAME`
>> > argument added. Additionally, we are keeping most of merge's options
>> such
>> > `--diff3-cmd`, `--force`, and we are using the same conflict resolution
>> as
>> > one's in the merge. I mean we can do merge from different source,
>> either URL
>> > in the same repository, URL in another repository, working copy path, or
>> > xpatch file, which is going to be added.
>> >
>> > I had another idea for such a command; I thought about simply adding
>> commands
>> > such as `svn patch --xpatch`, but I didn't like it. It seems to be a bit
>> > confusing for me, because the word "patch" is duplicated. Also we would
>> have
>> > to add a list of new options, which we are already having in `svn
>> merge`.
>> > I'd add that these options may also confuse someone using `svn patch` to
>> > apply unidiff patches.
>> >
>> > I was also thinking about adding commands to view xpatch files, for
>> example,
>> > viewing the summary or previewing them as unidiff, but I haven't
>> designed it
>> > yet. Also, I think that GUI clients like TortoiseSVN may add dialog
>> that will
>> > appear when the user opens an xpatch file. Then, they will be provided
>> a list
>> > of changed paths (like in 'check for modifications' dialog), and by
>> > double-clicking to each file, they will get into diff-viewer. I think
>> they
>> > will do it pretty well, if we implement it great and provide useful
>> APIs.
>> >
>> > This is my point. If you or anyone else want to suggest any other ideas
>> for the
>> > command-line interface, or leave a comment about one's I am suggesting,
>> feel
>> > free to express them. It is very important to make a correct decision
>> about it
>> > and have it accepted by the whole community.
>> >
>> > Want do add a few words about the leakage of implementation details
>> into the
>> > API/CLI. I think it's great when the API functions works similarly to
>> the
>> > command line interface. Of course they should not be exactly the same,
>> since
>> > the command-line interface is made to be useful, while API should do
>> exactly
>> > what it should do, so there could be several simplifications in the
>> part of CLI,
>> > but if the differences we have between svn.exe and libsvn_client are
>> getting
>> > minimal, the whole system would be simpler.
>> >
>> > > > 3. Optionally, we can also implement several commands to browse
>> xpatch
>> > > > files using the command-line interface. For example, previewing them
>> > > > as unidiff or showing the diff-summary.
>> > > >
>> > > > In my opinion, the best way to save the changes is to write them to
>> an
>> > > > XML file/stream (btw, this is why I would name this feature as
>> > > > "[x]patch").
>> > >
>> > > Again, leaky abstraction. Please name the thing based on its API
>> rather
>> > > than based on its implementation. There's a reason Air Force One's
>> > > callsign isn't "Biden One", you know :-)
>> >
>> > I had several candidates for the feature's name. Firstly, I was looking
>> > forward to name it as `bpatch`, because they are "binary" files, and
>> can track
>> > "binary" files, but 1) git style patches are already named as `bpatch`
>> somewhere
>> > and 2) it didn't sound pretty good for me. Then, I finally had a look
>> > on `xpatch`, due to XML format. Generally, it looks nice for me and I
>> decided
>> > to keep it simple, without going deeply into naming, in order to save
>> time.
>> >
>> > I am happy to hear any other voice about the name of the feature. This
>> is an
>> > important part of it and this decision has to be done and accepted by
>> the
>> > whole community.
>> >
>> > --
>> > Timofei Zhakov
>>
>> Thanks for describing your vision for this feature. Some thoughts
>> below... I would like to emphasize that I'm only offering input, but
>> as always I'm perfectly open to different ideas.
>>
>> Regarding the name 'xpatch':
>>
>> Admittedly at first I felt we should find another name, but the more I
>> think about it, the more the name "xpatch" appeals to me. In
>> additional to x standing for XML and binary, it could also mean
>> "eXtended patch."
>>
>
> eXtended patch sounds like a better "name" for the feature to use in
> documentation, even if we end up using XML format.
>
>
>>
>> Also:
>>
>> On Sun, Nov 24, 2024 at 11:25 AM Daniel Shahaf <d...@daniel.shahaf.name>
>> wrote:
>> (snipping most of [1] to focus on the naming)
>> > Moreover, we have a bunch of cmdline options named --x-*, which
>> > are "experimental". (Coincidentally, most of these are related to
>> > shelving.) --xpatch could be taken for experimental as well, if so
>> > named.
>>
>> True, we have 'svn x-shelve' etc., where 'x'=experimental, but I don't
>> think we'll have confusion here. We have a precedent for 'x' to mean
>> "eXtended": e.g., 'svn diff -x-p'. So I think 'svn diff --xpatch'
>> should be okay.
>>
>
> I also don't think this is a big problem. The --xpatch option would be
> shown directly by svn help and not require -v "to show experimental
> options", also the experimental options are marked as such.
>
>
>> Regarding command line usage:
>>
>> Thoughts in favor of using 'svn patch':
>>
>> * Until now, patches have been applied with 'svn patch', rather than
>> 'svn merge'. If I consider myself as a user with a patch, I just want
>> to apply the patch without knowing what happens under the hood or
>> thinking too much about which command to use.
>>
>> * 'svn patch' accepts 2 kinds of patches: unidiff (+ svn properties),
>> and git extended unidiff (+ svn properties). IMHO adding xpatch as a
>> 3rd kind feels like a logical next step (from user's point of view).
>>
>> * The 'svn help patch' text is not too long and the explanation of
>> 'xpatch' can be added without too many changes. I experimented with
>> this a little and I'll share a rough draft further below... In
>> contrast 'svn help merge' is very long and we'd have to make it longer
>> to explain the new xpatch mode.
>>
>> * 'svn patch' has a '--strip ARG' to remove leading path components.
>> e.g., a patch could have paths like subversion/trunk/subversion but
>> maybe your WC is rooted at trunk, so you may need 'svn patch --strip
>> 2'.
>>
>> Thoughts in favor of using 'svn merge':
>>
>> * It's possible that 'svn patch' was always kept "dumb" intentionally
>> to keep 'svn diff' and 'svn patch' close to plain Unix 'diff' and
>> 'patch'. That might be why 'svn patch' doesn't attempt to use a 3-way
>> merge, but just goes hunk-by-hunk and rejects everything that doesn't
>> fit. Maybe 'svn patch' subcommand would be too far from its original
>> purpose if it applies xpatches with merge and conflict resolution.
>>
>> * You mentioned 'svn merge' has other options that would be used with
>> xpatch. Not sure if things like '--record-only' or '--ignore-ancestry'
>> make sense, but certainly '--accept ARG' does, because we'll use
>> conflict resolution.
>>
>
> svn merge (as far as I understand) always operate on a URL (if the source
> is a WC, "the corresponding URL of the path is used").
> svn merge also updates mergeinfo, which I assume xpatch wouldn't do.
>
> Both of these make xpatch more similar to svn patch. The argument about
> having to add options to the help text of svn patch apply in reverse for
> svn merge - some options would have to be marked as "this option doesn't
> apply for merging an xpatch file").
>
>
>>
>> * GUI clients may be more amenable to support 'xpatch' if it fits
>> better with the merge APIs.
>>
>
> I don't think that would matter, at least not to TortoiseSVN. There is
> already a precedence for naming things differently ("svn shelve
> --keep-local" is called "checkpointing") and I don't see a big problem
> detecting an xpatch file and switching APIs.
>
>
>>
>> Idea for a third alternative:
>>
>> If neither 'svn patch' nor 'svn merge' are the best fit, we could take
>> inspiration from Git, which uses 'git apply' to apply patches, and add
>> a new 'svn apply' subcommand. It would be like 'svn patch', but
>> smarter, with merging and conflict resolution. Perhaps it could
>> support xpatch at first, and later add support for unidiff and git
>> unidiff. The idea here is to leave 'svn patch' as similar to plain
>> Unix patch... but it means we'd have 2 patch application commands and
>> potential confusion about which one to use, so, maybe this isn't such
>> a good idea. :-)
>>
>
> Maybe: svn xpatch patchfile.xpatch
>
> But I think the risk of confusion is too large.
>
>
>
>>
>> Returning to 'svn patch' for a moment:
>>
>> What would the help text look like if 'svn patch' handles xpatches? I
>> experimented a little and came up with this UNTESTED rough draft
>> patch:
>>
>> [[[
>>
>> Index: subversion/svn/svn.c
>> ===================================================================
>> --- subversion/svn/svn.c (revision 1921955)
>> +++ subversion/svn/svn.c (working copy)
>> @@ -1418,28 +1421,36 @@
>> " first @ as a peg revision indicator. This does not apply to
>> DST.\n"
>> )},
>> {'q', opt_force, opt_parents, opt_allow_mixed_revisions,
>> SVN_CL__LOG_MSG_OPTIONS, 'r'},
>> {{'r', "deprecated and ignored"}} },
>>
>> { "patch", svn_cl__patch, {0}, {N_(
>> "Apply a patch to a working copy.\n"
>> "usage: patch PATCHFILE [WCPATH[@]]\n"
>> "\n"), N_(
>> - " Apply a unidiff patch in PATCHFILE to the working copy WCPATH.\n"
>> + " Apply a patch in PATCHFILE to the working copy WCPATH.\n"
>> " If WCPATH is omitted, '.' is assumed.\n"
>> "\n"), N_(
>> + " The patch may be a unidiff, git extended unidiff, or xpatch
>> file. Its\n"
>> + " type is detected automatically.\n"
>> + "\n"), N_(
>> " A unidiff patch suitable for application to a working copy can
>> be\n"
>> " produced with the 'svn diff' command or third-party diffing
>> tools.\n"
>> " Any non-unidiff content of PATCHFILE is ignored, except for
>> Subversion\n"
>> " property diffs as produced by 'svn diff'.\n"
>> "\n"), N_(
>> + " An xpatch file can be produced with the 'svn diff --xpatch'
>> command.\n"
>> + " This patch format is specific to Subversion and can represent
>> all\n"
>> + " types of working copy changes. It is applied with three-way
>> merging\n"
>> + " and conflict resolution.\n"
>> + "\n"), N_(
>> " Changes listed in the patch will either be applied or
>> rejected.\n"
>> " If a change does not match at its exact line offset, it may
>> be applied\n"
>> " earlier or later in the file if a match is found elsewhere for
>> the\n"
>> " surrounding lines of context provided by the patch.\n"
>> " A change may also be applied with fuzz, which means that one\n"
>> " or more lines of context are ignored when matching the change.\n"
>> " If no matching context can be found for a change, the change
>> conflicts\n"
>> " and will be written to a reject file with the extension
>> .svnpatch.rej.\n"
>> "\n"), N_(
>> " For each patched file a line will be printed with characters
>> reporting\n"
>>
>> ]]]
>>
>> The last part of 'svn help patch' (not shown) is only applicable to
>> unidiff (and git unidiff) patches so needs a small edit to clarify
>> that. I tried various phrasings but wasn't happy with any of them so
>> far.
>>
>
> Makes sense to me!
>
>
>>
>> I didn't do it for 'svn help merge' text yet.
>>
>> If you'd like, I'll be happy to draft 'svn help' text, for 'svn diff'
>> and whichever of 'svn patch' or 'svn merge' ends up being the command.
>>
>> Thoughts, corrections, alternative ideas welcome!
>>
>> References:
>>
>> [1] danielsh's message to dev@ on 24 Nov 2024:
>> https://lists.apache.org/thread/gywjpqyxvryo08wtq1t0232v6vdf94c1
>
>
Hi,
My few addition points regarding the name:
I think it's nice to have an association with an "extension", like an
"eXtended" patch, and suggest considering it as something modern. In my
thoughts it'd be great to keep it, of course if nobody would suggest any
other idea.
As I understood that, we agreed using the `svn diff --xpatch` command for
creating xpatches.
However, I'm changing my opinion about the CLI of xpatches'
application/merging onto a WC. I realised that `svn merge --xpatch` is not
the best and a little bit confusing for the users. In my opinion, the
reason for that is that we should not rely the format detection onto the
users. They should not even realise that they are going to apply an xpatch
instead of a plaintext patch. So, I think that it's much simpler for the
users to always invoke `svn patch`, for all kinds of patches, even if it
would be a completely new format XYZ.
I also thought about how this would be implemented in the GUI clients, and
didn't find anything better than automatic detection of the patch format,
and utilising either unidif or xpatch applier.
Finalising the points above, I'd suggest to do the xpatch application
through the `svn patch patchfile.xpatch` command. I think this would be
best for the users.
However, this approach might cause a few technical problems; For example,
how to detect the format?
I think that the xpatch files should begin with a line that describes it.
For example, there is a standard tag in XML called "Processing Instruction"
[1]. It specifies that below an XML stream is going. Usually it begins with
a line like this:
[[[
<?xml version="1.0" encoding="UTF-8" ?>
]]]
However, the wiki-page at [1], explains that it is allowed to use any tag
as for the processing instruction. I think we may begin each xpatch with a
line presented below:
[[[
<?xml-patch encoding="UTF-8" version="42" ?>
...or...
<?xpatch encoding="UTF-8" version="42" ?>
]]]
Here the 'encoding' attribute specifies the encoding used for the tags, and
the 'version' will let us know whether we support it or not. We can add any
other attribute, as needed.
So to detect if it is an xpatch or not we can just compare the first line.
Isn't it?
The problem is that a unidiff patch can begin with it. For example, this is
a good practice to write a log-message before the patch itself, and it
should not be interpreted by the client as a content. This means that each
plain and unidiff patch, beginning with this line, as a log-message, will
be rejected by the client.
Probably we can deal with that and interpret those patches as 'xpatch',
since it's very unusual to begin a log-message with this line, but we
should take it in mind.
Additionally, we may add an argument to the `svn patch` command, so users
will be able to specify the kind explicitly, for example, `svn patch
patchfile.patch --format unidiff` will apply it as a unidiff patch, even if
it begins with the xpatch marker.
Thoughts?
Thanks to Nathan for writing the "help" docstrings. They add much more
sense to the feature in general. Are you going to describe xpatch in the
`svn patch` command?
[1] https://en.wikipedia.org/wiki/Processing_Instruction
--
Timofei Zhakov
