Re: svn commit: r1555350-/subversion/trunk/subversion/libsvn_client/mtcc.c

On 05.01.2014 09:50, Bert Huijben wrote:
> While I agree that the mtcc code needs review and tweaks, many other arguments here would equally be valid to veto every wc-ng API added during 1.7 and 1.8 where we explicitly moved the context to the start of the argument list.

Noted.

> Making svnmucc a library is one of the most common feature requests on the SharpSvn list and I expect other users to benefit from this, for the sane reason as we added apis to svnversion and other tools in the past.

That raises the question: why is it not a common feature request here on
this list? And what's wrong with telling users to use the RA API instead
of writing myriad wrappers for it? The only "hard" part of that is
creating the RA session, and we already have svn_client_open_ra_session
that solves the problem.

Note that svnversion is basically just a recursive svn_client_info with
a bit of icing on top; what we're doing here is adding a whole layer
above svn_client and svn_ra, but putting it into svn_client. That does
not sound like a valid API architecture to me.

> Releasing editor v2 in its current unproven state in javahl looks like a bigger problem

There's nothing unproven about the Ev2 API. Given the choice of exposing
the delta editor API in JavaHL, or exposing an API that's actually
understandable to an average programmer, I chose Ev2. It's currently
implemented using the Ev2<->Ev1 shims, and only commit and status are
available; but they work just fine.

The only "unproven" thing about Ev2 does not in fact have anything to do
with Ev2 at all; it's the question of move semantics, which we haven't
solved yet; and we haven't implemented it, either.

> but lets not tie every problem to each other and try to solve them separately instead of vetoing everything in one batch.

I didn't veto anything; I did put "technical" in quotes, since obviously
my arguments cannot support an actual veto. But I feel strongly enough
about the issue to write "-1".

> The pool requirements for fs* need work for 1.9.

I'd rephrase that as: we have to document pool usage constraints on the
delta editor. IMO, the kind of lifetime overlap we saw in mtrr_commit is
not valid; but neither should an editor implementation assume anything
about pool contents or lifetime after close_edit or abort_edit. You're
correct that we have pool cleanups for solving the kind of
interdependencies that we see in this case.

> The decision whether we allow external api usage of the WIP editor v2 for 1.9 needs work.

N.B., it's not "external API usage"; it's used by libsvnjavahl, which is
allowed to use our private APIs as much as any other Subversion library.

> And opening up svnmucc as an api needs work.

Mark it experimental. I still maintain that it should not be in
svn_client, and given what I wrote it shouldn't exist at all. We might
consider creating a new library for "APIfied command-line tools", but
making svn_client the same kind of hodge-podge of different layers as
svn_subr is IMO the wrong approach.

> (If I had just implemented this as part of SharpSvn we would just have found the fsfs symptoms months from now when releasing 1.9)

That's irrelevant (on this list). I won't say a word about what the
above statement implies for your QA workflow. :)

-- Brane

> -----Original Message-----
> From: "Branko Čibej" <brane_at_wandisco.com>
> Sent: ‎05/‎01/‎2014 04:51
> To: "Subversion Development" <dev_at_subversion.apache.org>
> Subject: Re: svn commit: r1555350-/subversion/trunk/subversion/libsvn_client/mtcc.c
>
> On 05.01.2014 01:53, Stefan Fuhrmann wrote:
>
> Hi Bert,
>
> All your arguments would be valid and I would fully agree
> with them, if the FS layer was the root cause of the pool
> issue. However, IMO, that is not the case and r1555297
> merely accidentally caused free memory to be reused
> earlier than before.
>
> My understanding of mucc_commit is the following sequence:
>
> ra = open_session(mtcc->pool)
> editor = open_editor(ra, scratch_pool)
> destroy(mtcc->pool)
> ... // editor cannot be cleaned up safely anymore
> destroy(scratch_pool)
>
> This is clearly invalid (unless I'm completely misreading the code).
> I agree that it looks clearly invalid; however, the API documentation is unclear. It only says that the RA session may not be used during the lifetime of the editor.
>
> I would expect that "editor->close_edit()" (which you omit from that sequence and which happens before the "destroy(mtcc->pool)" call) to in fact kills the editor, and that destroying its containing pool is just a minor detail. It's obviously a bad idea for the editor implementation to assume anything about the RA session's pool after close_edit (or abort_edit).
>
> On the other hand, the mtcc code is decidedly weird when it comes to pool lifetime handling, which becomes clear when the sequence above is seen in the context of the surrounding API calls and pool creations:
>
> execute(..., pool):
> scratch_pool = create(pool)
> mtcc = mtcc_create(pool, scratch_pool):
> mtcc.pool = create(pool)
> mtcc.ra = open_session(pool, scratch_pool)
> mtcc_commit(mtcc, scratch_pool):
> editor = open_editor(mtcc.ra, scratch_pool)
> editor.close_edit()
> destroy(mtcc.pool) ## Oops ...
> destroy(scratch_pool)
>
> This makes the lifetime of the editor object (as opposed to the edit drive) overlap with the lifetime of the session. As I said, there's nothing in the API docs saying explicitly that this is wrong, but at the very least it's extremely unusual (and, FWIW, it has nothing to do with the single- vs. dual-pool pattern).
>
> While it may be true that we use this pattern elsewhere, there's absolutely no call for repeating it here and perpetuating the broken pool usage. I'll note that this kind of crud (and in new code, no less) will make switching to Ev2 even more of a nightmare.
>
>
>
>
> As an aside to all of the above: Why in blazes is svn_client_mtcc_* a public API? It is nothing but a wrapper for the RA commit editor API. It doesn't even hide the nasty details of getting file contents streams and checksums from the API consumer. As such, it's nothing but a maintenance burden. Yes, it hides the nasty details of Ev1 from the poor user, but that goal would be much better served by working on making Ev2 public (as I've already done in JavaHL). This svn_client_mtcc thing adds absolutely no benefit compared to Ev2, as far as I can see.
>
> (It also breaks our established API pattern by putting the API context (the svn_client_mtcc_t) last in the parameter list, instead of first, but given that the whole idea of making this a public API is silly, that's just a minor nit.)
>
> In other words, -1 to the whole svn_client_mtcc idea for the "technical" reason that it's a total waste of time.
>
> -- Brane
>
>
>

-- 
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane_at_wandisco.com