[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: Nasty double-replace copy-on-update bug

From: Philip Martin <philip_at_codematters.co.uk>
Date: 2007-10-19 15:13:11 CEST

"David Glasser" <glasser@davidglasser.net> writes:

> On 10/19/07, Philip Martin <philip@codematters.co.uk> wrote:
>> "Ben Collins-Sussman" <sussman@red-bean.com> writes:
>>
>> > The question is: why the wc-searching routine
>> > (update_editor.c:locate_copyfrom()) think that it's found the correct
>> > version of the file, when it's not? Answer: it seems to be getting
>> > stale svn_wc_entry_t information. It's reading a couple of entries
>> > files from disk, rather than from a more recent adm_access set still
>> > held in memory, and getting an incorrect (stale) last-changed-rev
>> > value. From poking around in gdb, it seems that the entry_t's which
>> > are write-cached in memory are correct -- they have the proper
>> > last-changed-rev values, but simply aren't flushed to disk yet.
>> >
>> > I attempted to solve this by passing in the edit_baton->access_set to
>> > the searching routine, but it's not fixing the problem. I've attached
>> > my patch-in-progress below. Can we get some more eyes on this
>> > problem?
>>
>> I think there are two bugs here. I believe it is wrong for
>> locate_copyfrom to use a read-only snapshot of the working copy, your
>> patch to pass an access baton looks like the right thing to do,
>> although I think that the _try calls should take write locks. However
>> that's not what is causing the current failure.
>
> Why is that? It sounds like a read-only operation...

Without a write lock some other Subversion process could change or
delete the file in the interval between the decision to use a file and
the moment it gets used.

>> The reason that locate_copyfrom thinks it has the right file is that
>> when alpha first gets added it has committed-rev=1 and that meets the
>> conditions required by locate_copyfrom. This happens because alpha
>> itself has copyfrom info and update_editor.c:add_file_with_history
>> calls eb->fetch_func to get gamma@1 from the repository and then there
>> is a bit of code that does:
>>
>> /* Loop over whatever base-props we have in memory, faking change_file_prop()
>> calls against the file baton. */
>> for (hi = apr_hash_first(pool, base_props); hi; hi = apr_hash_next(hi))
>> {
>> const void *key;
>> void *val;
>> const char *propname;
>> svn_string_t *propval;
>> apr_hash_this(hi, &key, NULL, &val);
>> propname = key;
>> propval = val;
>> SVN_ERR(change_file_prop(tfb, propname, propval, pool));
>> }
>>
>> which transfers the gamma@1 props to the new alpha, including
>> committed-rev=1. Later on alpha will get corrected to have
>> committed-rev=3 but before that something else happens to look for
>> alpha@1.
>
> Well, it's not really "before that". Based on the editor drive that's
> happening, the server has already told the client that commited-rev=3;
> this information is in a loggy that has not yet been executed. Should
> the cached entries in the baton be updated when the loggy is written
> or when it is executed?

How the logs and cache should interact is a difficult question to
answer. We have the current behaviour which is the result of
evolution rather than any grand unified design.

Why does add_file_with_history flush and run the log, the one with the
wrong committed-rev? I guess it's to enable a text-delta to be
applied, but it does seem to be sort of in the middle of things. Is
the working copy consistent if the process gets interrupted after the
log has been run? If there is no log file cleanup will simply remove
the locks and assume the working copy is OK.

> See Issue 2977 for a more detailed explanation of what events occur.

That might have saved me some time. I hate the issue tracker
diverting information from the dev list.

>> Perhaps properties like committed-rev should be filtered out of then
>> above loop? Perhaps only regular props should be transferred?
>
> Maybe, but that seems like sidestepping the problem, which is that the
> left hand of the library knows something (that the server has set
> committed-rev) that the right hand doesn't...

I think that transferring committed-rev from one file to another is a
bug, that's not sidestepping anything.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Oct 19 15:33:33 2007

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.