On 3/13/07, Peter Lundblad <plundblad@google.com> wrote:
> dionisos@tigris.org writes:
> > Log:
> > Make the 'working file has changed' heuristic more robust:
> > check for the size of the file too.
> >
> Hi, Erik,
>
> This is much appreciated!
Thanks :-)
>Also, why is the difference between unknown value and "value not recorded
>because the file is modified" important. I'd say these two boils down to the
>same. If the file is modified compared to the base, you catually *don't know*
>the real value. I'd say we need to distinguish between present and not
>present. If present, use it, else fall back on other means. If we know the
>value and know we can use it, write it, else make it not present.
Well, the difference between present and not-present or
known-modified/present/not-present is that in the case of
known-modified the actual size and the value which would have been in
the entries file may be equal. So, I think I improved the detection
heuristic though I may have 'abused' one of the fields for it. I think
it would be a shame to discard the fact that a file was changed and
needs close inspection.
> > Modified: trunk/subversion/include/svn_wc.h
> > URL: http://svn.collab.net/viewvc/svn/trunk/subversion/include/svn_wc.h?pathrev=23767&r1=23766&r2=23767
> > ==============================================================================
> > --- trunk/subversion/include/svn_wc.h (original)
> > +++ trunk/subversion/include/svn_wc.h Sat Mar 10 03:22:25 2007
> > @@ -1384,6 +1384,12 @@
> > */
> > const char *changelist;
> >
> > + /** Size of the file after being translated into local representation,
> > + * or 0 if unknown.
> > + * @since New in 1.5.
> > + */
> > + apr_off_t working_size;
>
> This was already mentioned, but I'm still very uneasy about using a valid
> value as "size unknown".
Ok. I'll change that then. Karl wasn't very thrilled about it either.
> > +
> > /** Whether a local copy of this entry should be kept in the working copy
> > * after a deletion has been committed, Only valid for the this-dir entry
> > * when it is scheduled for deletion.
> >
> > Modified: trunk/subversion/libsvn_wc/entries.c
> > URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_wc/entries.c?pathrev=23767&r1=23766&r2=23767
> > ==============================================================================
> > --- trunk/subversion/libsvn_wc/entries.c (original)
> > +++ trunk/subversion/libsvn_wc/entries.c Sat Mar 10 03:22:25 2007
> > @@ -2,7 +2,7 @@
> > * entries.c : manipulating the administrative `entries' file.
> > *
> > * ====================================================================
> > - * Copyright (c) 2000-2006 CollabNet. All rights reserved.
> > + * Copyright (c) 2000-2007 CollabNet. All rights reserved.
> > *
> > * This software is licensed as described in the file COPYING, which
> > * you should have received as part of this distribution. The terms
> > @@ -454,6 +454,18 @@
> > /* Keep entry in working copy after deletion? */
> > SVN_ERR(read_bool(&entry->keep_local, SVN_WC__ENTRY_ATTR_KEEP_LOCAL,
> > buf, end));
> > + MAYBE_DONE;
> > +
> > + /* Translated size */
> > + {
> > + const char *val;
> > +
> > + SVN_ERR(read_str(&val, buf, end, pool));
> > +
>
> read_str should be read_val because the value cannot include escape sequences.
Ah. ok. Done.
> > + entry->working_size =
> > + (apr_off_t)apr_strtoi64(val, NULL, 0);
> > + }
> > + MAYBE_DONE;
> >
> > done:
> > *new_entry = entry;
> > @@ -847,6 +859,27 @@
> > entry->present_props = apr_pstrdup(pool, entry->present_props);
> > }
> >
> > + /* Translated size */
> > + {
> > + const char *val
> > + = apr_hash_get(atts,
> > + SVN_WC__ENTRY_ATTR_WORKING_SIZE,
> > + APR_HASH_KEY_STRING);
> > + if (val)
> > + {
> > + if (! strcmp(val, SVN_WC__WORKING_SIZE_WC))
> > + {
> > + /* Special case (same as the timestamps); ignore here
> > + these will be handled elsewhere */
> > + }
> > + else
> > + /* Cast to off_t; it's safe: we put in an off_t to start with... */
> > + entry->working_size = (apr_off_t)apr_strtoi64(val, NULL, 0);
> > +
> > + *modify_flags |= SVN_WC__ENTRY_MODIFY_WORKING_SIZE;
> > + }
> > + }
> > +
> > *new_entry = entry;
> > return SVN_NO_ERROR;
> > }
> > @@ -1554,6 +1587,12 @@
> > /* Keep in working copy flag. */
> > write_bool(buf, SVN_WC__ENTRY_ATTR_KEEP_LOCAL, entry->keep_local);
> >
> > + /* Translated size */
> > + write_str(buf,
> > + entry->working_size
> > + ? apr_off_t_toa(pool, entry->working_size) : "",
> > + pool);
>
> Should be write_val, mostly for symmetric reasons...
ok. Done.
> > Modified: trunk/subversion/libsvn_wc/log.h
> > URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_wc/log.h?pathrev=23767&r1=23766&r2=23767
> > ==============================================================================
> > --- trunk/subversion/libsvn_wc/log.h (original)
> > +++ trunk/subversion/libsvn_wc/log.h Sat Mar 10 03:22:25 2007
> > @@ -219,6 +219,20 @@
> > const char *time_prop,
> > apr_pool_t *pool);
> >
> > +
> > +/* Extend **LOG_ACCUM with log instructions to set the timestamp of PATH
> > + in the entry field with name TIME_PROP.
> > +
> > + Use SVN_WC__ENTRY_ATTR_* values for TIME_PROP.
> > +*/
> > +
> > +svn_error_t *
> > +svn_wc__loggy_set_entry_working_size_from_wc(svn_stringbuf_t **log_accum,
> > + svn_wc_adm_access_t *adm_access,
> > + const char *path,
> > + apr_pool_t *pool);
>
> Indentation got screwed up here. Also, docstrings talks about a parameter
> TIME_PROP which is not present. Also, what does the log instructions set
> the timestamp *to*?
Indentation has already been fixed. Doing the docstring now.
> > Modified: trunk/subversion/libsvn_wc/questions.c
> > URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_wc/questions.c?pathrev=23767&r1=23766&r2=23767
> > ==============================================================================
> > --- trunk/subversion/libsvn_wc/questions.c (original)
> > +++ trunk/subversion/libsvn_wc/questions.c Sat Mar 10 03:22:25 2007
> > @@ -163,6 +163,41 @@
> > mark it for removal?
> > */
> >
> > +
> > +/*
> > + */
>
> Nice docstring, maybe a bit short, thoug:-)
Ok. I've come up with one a bit less terse.
> > +static svn_error_t *
> > +localfile_size_changed_p(svn_boolean_t *changed_p,
> > + const char *path,
> > + svn_wc_adm_access_t *adm_access,
> > + apr_pool_t *pool)
> > @@ -393,18 +428,30 @@
> >
> > if (! force_comparison)
> > {
> > + /* See if the local file's size is different from when we created it */
> > +
> > + /*### The number of stat() calls can be reduced by integrating
> > + localfile_size_changed_p() and svn_wc__timestamps_equal_p() here. */
> > +
>
> I'd say that this is pretty important. This function is called for every
> versioned file in svn status and we've worked quite hard to eliminate
> unnecesary stat calls in the common codepath of that command. It would be
> really sad if this work was partially undone by this change, and there's
> actually no real reason to keep it this way, is there?
Yes, there's no reason not to do that, but I just didn't want to in
the same commit as that would have clouded the actual change. I'll
change this after I put right most of the other comments I've received
(mostly yours).
> > Modified: trunk/subversion/libsvn_wc/update_editor.c
> > URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_wc/update_editor.c?pathrev=23767&r1=23766&r2=23767
> > ==============================================================================
> > --- trunk/subversion/libsvn_wc/update_editor.c (original)
> > +++ trunk/subversion/libsvn_wc/update_editor.c Sat Mar 10 03:22:25 2007
> > @@ -2052,13 +2052,15 @@
> > apr_uint64_t modify_flags = SVN_WC__ENTRY_MODIFY_KIND
> > | SVN_WC__ENTRY_MODIFY_REVISION
> > | SVN_WC__ENTRY_MODIFY_DELETED
> > - | SVN_WC__ENTRY_MODIFY_ABSENT;
> > + | SVN_WC__ENTRY_MODIFY_ABSENT
> > + | SVN_WC__ENTRY_MODIFY_WORKING_SIZE;
> >
> >
> > tmp_entry.revision = new_revision;
> > tmp_entry.kind = svn_node_file;
> > tmp_entry.deleted = FALSE;
> > tmp_entry.absent = FALSE;
> > + tmp_entry.working_size = 0;
> >
> > /* Possibly install a *non*-inherited URL in the entry. */
> > if (new_URL)
>
> Maybe it doesn't hurt, but why do we modify the working size here, but not the
> text timestamps? I'd imagine they should be treated quite similarly.
Heh. Yea, I guess we should. I didn't think about that. Added.
> As I said before, the robustness improvement is appreciated.
Thanks again.
bye,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Mar 14 23:58:06 2007