On Mon, Dec 16, 2024 at 1:17 PM Timofei Zhakov <t...@chemodax.net> wrote:
(snip)
> Hi Nathan,
>
> Thanks for clarifying your thoughts.
>
> As I am understanding that, you are looking forward to make the xpatches
> applying through the svn_client_patch() function, which is currently working
> with unidiff and git style patches.
Yes, that was how I approached it.
>From the user's perspective, I think it's correct that we're
simplifying usage by putting xpatch with the 'svn patch' subcommand.
My thinking was that the API could be (somewhat) a parallel of the
cmdline interface. If that were possible, GUI clients that already
know how to apply patches might gain support for applying xpatches
"for free" (without having to modify their code, and, without having
to know what type of patch they have).
However, with your explanation and a bit more thinking, I can see that
my idea wouldn't work at all.
So:
> However, I think that this approach is completely incorrect in terms of the
> abstraction. The svn_client_patch() function currently handles only
> plain-text patches (including both git-style and unidiff). I am not sure, but
> I think it reads each next patch one-by-one, detecting whether it's a
> git-style patch or a plain undiff. It could do so, since they have just minor
> differences, such as binary file support. Additionally, each unidiff patch is
> valid in git-style as well.
>
> I think that it would be better to separate format detection,
> unidiff/git-style patch application, and xpatch application into different
> API methods. Let me provide you with declaration of such functions, that I've
> just wrote and explained with the corresponding docstrings:
Agreed: separate APIs to detect patch format, apply unidiff patches,
and apply xpatches...
> [[[
> /* Describes the patch format. */
> typedef enum svn_client_patch_format_t
> {
> /* Unrecognized or unknown format.
> *
> * ### Do we actually need this?
> */
> unknown = -1,
>
> /* Either unidiff or git style patch */
> unidiff = 0,
>
> /* Extended Subversion patch -- xpatch
> *
> * ### Better description?
> */
> xpatch,
> };
(I know it's just a rough draft, but just a nit: if the enum needs to
become published API, the enum items would need namespace
decorations...)
More below...
> /**
> * Apply an xpatch, described in readable @a xpatch_file stream, onto a
> * working at @a target_wcpath.
> *
> * Use @a pool for any temporary allocation.
> */
> svn_error_t *
> svn_client_xpatch_apply(svn_stream_t *xpatch_file,
> const char *target_wcpath,
> /* TODO: other arguments from the
> * svn_client__apply_processor_create() function */
> svn_client_ctx_t *ctx,
> apr_pool_t *pool);
>
> /**
> * Detect the format of @a patch_file, setting @a format with the detected
> * format.
> */
> svn_error_t *
> svn_client_detect_patch_format(apr_file_t *patch_file,
> svn_client_patch_format_t *format);
> ]]]
>
> Additionally, I think that it's much simpler to implement the xpatch
> applier/parser in a separate function than the svn_client_patch(), as for our
> client library (libsvn_client), probably in svn.exe, and may simplify it a
> lot for the GUI clients developers, since they might want to detect the
> format before to show different dialog for xpatch, as an example.
>
> Moreover, there are semantic differences in patches and xpatches, because
> unidiff patches internally work with the file pointer, using apr_file* API,
> while xpatches could be implemented in a push-parser, backed by svn_stream
> and Expat library.
>
> Yeah, we could create a stream from a file, but in this case, it's better to
> leave this responsibility to a higher level, for example, cmdline or any
> other SVN client.
I agree that's a lot cleaner.
I was trying to encapsulate the file handling and avoid opening the
patch file twice.
But nothing stops the cmdline or GUI client from opening the file once
and creating a stream from the file.
(Also with a stream, clients might come up with other sources for
xpatch content besides files, which could give interesting
possibilities...)
> In summary, I'd say that we should separate the APIs for "merging" of
> xpatches and undiff "patching", but leave a client function for the format
> detection. However, the current patch API prohibits so, due to the source,
> that we get the patches from.
>
> Hope this now makes some sense...
+1. Thanks for explaining it, and thanks for being mindful of keeping
a clean abstraction.
Also I'll include my reply to the follow-up below:
On Mon, Dec 16, 2024 at 3:09 PM Timofei Zhakov <t...@chemodax.net> wrote:
(snip)
> Hi again,
>
> I have an alternative suggestion for the format-detection API:
>
> Instead of svn_client_detect_patch_format(), a function that checks whether
> it's xpatch or not. I mean that we won't provide a function for detecting the
> exact format, but we'll have a function like svn_client_patch_is_xpatch(),
> returning a boolean value (probably as an output argument, since the
> filesystem API may throw an exception). The drafted declaration can be
> accessed below:
>
> [[[
> /**
> * Detect whether the format of @a patch_file is xpatch or not. The result
> * should be available at @a is_xpatch.
> *
> * Use @a scratch_pool for any temporary allocation.
> */
> svn_error_t *
> svn_client_patch_is_xpatch(apr_file_t *patch_file,
> svn_boolean_t *is_xpatch,
> apr_pool_t *scratch_pool);
> ]]]
I agree with this idea because it is simpler.
(If we ever want a svn_client_detect_patch_format(), we would still
need svn_client_patch_is_xpatch() and we would also need
svn_client_patch_is_unidiff(), because
svn_client_detect_patch_format() would have to call each one until it
finds a match, or returns 'unknown'... Having
svn_client_patch_is_xpatch() might be enough for now and more
functions can always be added in the future.)
> Additionally, I'd like to add that a single function for applying unidiff
> patches and xpatches would not deal with a switch for forcing the format that
> we wanted to add to the command-line (I mean something like `svn patch
> --format xpatch`). That's because it's a bit incorrect to add such an
> argument to the svn_client API. It's svn-exe's responsibility, to handle this
> switch, over the format detected automatically. Let's not overcomplicate our
> life!
+1 to that!
I'm confident that the direction here is correct.
Cheers,
Nathan
