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

Re: svn commit: r31196 - trunk/subversion/libsvn_client

From: Philip Martin <philip_at_codematters.co.uk>
Date: Thu, 15 May 2008 23:03:51 +0100

Karl Fogel <kfogel_at_red-bean.com> writes:

> Philip Martin <philip_at_codematters.co.uk> writes:
>>> Log:
>>> Fix issue #3198: don't commit an add of a missing item.
>>>
>>> * subversion/libsvn_client/commit_util.c
>>> (harvest_committables): Check for existence before adding.
>>>
>>> [...]
>>
>> Adding svn_io_check_path calls often introduces races; in this case if
>> the file is deleted afer the check then the original problem is still
>> present. Now modifying the wc during a commit is not really expected
>> to work, but it would be good if the client failed without breaking
>> the working copy. A more robust solution would be better error
>> handling when attempting to open the missing file (I'm assuming that's
>> possible but I haven't looked at the code).
>
> Thanks for the review.
>
> I agree with you, of course, though in practice r31196 will prevent the
> situation from coming up in almost all cases.
>
> Doing the Truly Right Thing is surprisingly hard here, in part because
> svn_wc_text_modified_p() is documented to claim "not modified" if the

Why do we need to check that for an added file?

> file is simply missing. I don't know why we do this, but presumably it
> was on purpose, since we took the trouble to document it. This means
> that by the time we get to commit_util.c:do_item_commit(), the file is
> already not thought to be modified, that is
>
> item->state_flags & SVN_CLIENT_COMMIT_ITEM_TEXT_MODS
>
> is false. This is because earlier, in harvest_committables(), we asked
> svn_wc_text_modified_p(), and the answer was "not modified".

Just assume that all added files are modified.

>
> Now, this is not insoluble: we could ask there if the file is missing,
> set the SVN_CLIENT_COMMIT_ITEM_TEXT_MODS bit if it *is* missing, so that
> later we'd try to open it to send the textdelta and error at that point.

The commit should fail when the open fails when attempting to send the
textdelta. From what you have written I guess the original problem
was that the file was marked as unmodified and so the commit didn't
try to send the textdelta, and so it didn't try to open the file and
so didn't get an error. Does marking added files as modified does that
fix the bug by causing the commit to fail?

> But although that would be correct, it would be counterintuitive and
> IMHO make the code less comprehensible.
>
> What's really called for is some code reorganization, and/or figuring
> out why svn_wc_text_modified_p() treats missing as unmodified and
> (maybe) changing that.
>
> But I didn't want to go down that road right now. r31196 solves most of
> the problem, and life is short. Thoughts?

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-05-16 00:04:22 CEST

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