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
>
> Cheers,
> Nathan
>
Cheers,
Daniel
