[PATCH] completing the data flow of tmp text base paths in commit

[Julian Foad <ju...@wandisco.com>]

The attached patch is an attempt to close the gap between "transmit text
deltas" and commit post-processing, by passing the temporary new text
base file paths around.  It succeeds in that, at least it looks right
and passes the test suite.

The part of this patch that I haven't finished is with back-compat of
svn_wc__process_committed_internal(), and what is the difference between
its 'queue' parameter and its checksum/recurse/no_unlock/etc.
parameters, being values which are alternatively available in the queue.

svn_wc__process_committed_internal() is called by
svn_wc_process_committed_queue2() which passes the 'queue' param, and
also by the deprecated svn_wc_process_committed4() with QUEUE=NULL.  I
had been assuming that if QUEUE==NULL then all the parameters that are
available in the queue (checksum for one) are not available, but that's
not how the back-compat wrapper wants to work.  I'll need to fix that.
I think the right thing to do is to re-write the wrapper
(svn_wc_process_committed4()) to construct a new queue with one item and
pass that, and stop having the other parameters (checksum etc.) passed
as separate parameters.  I'll look at that tomorrow.  I may already have
committed changes that break that back-compat; I'll check.

The internal recursion of svn_wc__process_committed_internal() is
confusing me: it passes NO_UNLOCK=TRUE to itself when recursing,
regardless of what no_unlock value was passed in or what is in the queue
item.  Yet it passes the received value of KEEP_CHANGELIST.  For the
NEW_DAV_CACHE and CHECKSUM arguments it passes NULL, which is half
explained by them only having meaning in connection with a single node:
the same ones cannot be applicable to a child node.

Comments?

- Julian

Re: [PATCH] completing the data flow of tmp text base paths in commit

[Julian Foad <ju...@wandisco.com>]

On Mon, 2010-03-29, Greg Stein wrote:
> On Mon, Mar 29, 2010 at 13:10, Julian Foad <ju...@wandisco.com> wrote:
> > The attached patch is an attempt to close the gap between "transmit text
> > deltas" and commit post-processing, by passing the temporary new text
> > base file paths around.  It succeeds in that, at least it looks right
> > and passes the test suite.
> 
> except for that printf() :-P
> 
> > The part of this patch that I haven't finished is with back-compat of
> > svn_wc__process_committed_internal(), and what is the difference between
> > its 'queue' parameter and its checksum/recurse/no_unlock/etc.
> > parameters, being values which are alternatively available in the queue.
> >
> > svn_wc__process_committed_internal() is called by
> > svn_wc_process_committed_queue2() which passes the 'queue' param, and
> > also by the deprecated svn_wc_process_committed4() with QUEUE=NULL.  I
> > had been assuming that if QUEUE==NULL then all the parameters that are
> > available in the queue (checksum for one) are not available, but that's
> > not how the back-compat wrapper wants to work.  I'll need to fix that.
> > I think the right thing to do is to re-write the wrapper
> > (svn_wc_process_committed4()) to construct a new queue with one item and
> > pass that, and stop having the other parameters (checksum etc.) passed
> > as separate parameters.  I'll look at that tomorrow.  I may already have
> > committed changes that break that back-compat; I'll check.

Well, at least I checked that I don't seem to have made it worse by
anything I've done recently.

Still digging in history.

- Julian

Re: [PATCH] completing the data flow of tmp text base paths in commit

[Greg Stein <gs...@gmail.com>]

On Mon, Mar 29, 2010 at 13:10, Julian Foad <ju...@wandisco.com> wrote:
> The attached patch is an attempt to close the gap between "transmit text
> deltas" and commit post-processing, by passing the temporary new text
> base file paths around.  It succeeds in that, at least it looks right
> and passes the test suite.

except for that printf() :-P

> The part of this patch that I haven't finished is with back-compat of
> svn_wc__process_committed_internal(), and what is the difference between
> its 'queue' parameter and its checksum/recurse/no_unlock/etc.
> parameters, being values which are alternatively available in the queue.
>
> svn_wc__process_committed_internal() is called by
> svn_wc_process_committed_queue2() which passes the 'queue' param, and
> also by the deprecated svn_wc_process_committed4() with QUEUE=NULL.  I
> had been assuming that if QUEUE==NULL then all the parameters that are
> available in the queue (checksum for one) are not available, but that's
> not how the back-compat wrapper wants to work.  I'll need to fix that.
> I think the right thing to do is to re-write the wrapper
> (svn_wc_process_committed4()) to construct a new queue with one item and
> pass that, and stop having the other parameters (checksum etc.) passed
> as separate parameters.  I'll look at that tomorrow.  I may already have
> committed changes that break that back-compat; I'll check.

I think the answer for above may be to call internal functions with a
single cqi, rather than a full queue. Would that work well?

I do agree that passing QUEUE==NULL is troublesome, given all the
checks in this patch.

> The internal recursion of svn_wc__process_committed_internal() is
> confusing me: it passes NO_UNLOCK=TRUE to itself when recursing,
> regardless of what no_unlock value was passed in or what is in the queue
> item.  Yet it passes the received value of KEEP_CHANGELIST.  For the
> NEW_DAV_CACHE and CHECKSUM arguments it passes NULL, which is half
> explained by them only having meaning in connection with a single node:
> the same ones cannot be applicable to a child node.

Right. I have no idea why the NO_UNLOCK=TRUE is present. I would
suggest a bit of archeology to try and determine the reason. The
cache/checksum as NULL are valid, tho I don't know how we *do* get a
valid checksum for those committed children (since we don't,
necessarily, transmit any text for them). We kinda need a checksum...

>...

Cheers,
-g