Greg Stein <gstein@lyra.org> writes:
> On Thu, Apr 25, 2002 at 04:21:49PM -0500, sussman@tigris.org wrote:
> >...
> > +++ trunk/subversion/libsvn_wc/adm_crawler.c Thu Apr 25 16:21:44 2002
> >...
> > @@ -77,6 +78,15 @@
> > pool));
> >
> > SVN_ERR (svn_io_remove_file (tmp_text_base_path->data, pool));
> > +
> > + /* If necessary, tweak the new working file's executable bit. */
> > + SVN_ERR (svn_wc__maybe_toggle_working_executable_bit
> > + (&toggled, file_path->data, pool));
> > +
> > + /* ### hey guys, shouldn't we recording the 'restored'
> > + working-file's timestamp in its entry? Right now, every time we
> > + restore a file, the front-line-timestamp-check-for-modifiedness
> > + is being destroyed. */
>
> The restored file might be different from what used to be in there (for
> example, somebody had local mods, typed 'rm foo' and then the file got
> restored from the pristine). Thus, it needs a "new" timestamp so that it
> would get rebuilt.
It *will* have a new timestamp, by virtue of being copied back out
into the working. The issue here is that we should probably record
that new text-time in .svn/entries. If we don't, then a future
modified-check on the file might take longer.
Are we misunderstanding each other?
> > + SVN_ERR (svn_wc_copy_and_translate (full_from_path->data,
> > + full_dest_path->data,
> > + eol_str,
> > + TRUE,
> > + keywords,
> > + TRUE,
> > + pool));
> > +
> > + /* After copying, toggle the executable bit if props dictate. */
> > + return svn_wc__maybe_toggle_working_executable_bit
> > + (&toggled,
> > + full_dest_path->data,
> > + pool);
>
> Um, does this mean that a failure between the copy and the toggle could
> leave you with a file missing its exec bit?
>
> Shouldn't you set the exec bit *first*, and then preserve the bits when you
> do the copy? That way, you'll never end up with an inconsistent state.
>
> [ or is this before it even gets moved to be the WC file? ]
These are all being executed as log commands. If a crash occurs
between the copy and the toggle, the logfile still exists, and will be
re-run when the user does 'svn cleanup'.
But actually, your point *holds* in the first example -- in
svn_revert_file, which is *not* a loggy operation. I better fix that!
> > + else if ((! apr_strnatcasecmp (propval->data, "false"))
> > + || (! apr_strnatcasecmp (propval->data, "off")))
>
> What in the hell?
>
> strnatcasecmp is NOT for doing comparisons like this. Those functions are
> for *SORTING*. This came up before with a checkin from cmpilato. You want to
> use #include apr_general, and then use strcasecmp().
Yikes! I naively copied that function from other places in the same C
file, being used for the same purpose.
I better go fix them both! Thanks.
>
> > + {
> > + *wants_exec = apr_pcalloc (pool, sizeof (*wants_exec));
> > + **wants_exec = FALSE;
> > + }
>
> Why does this function return an *allocated* boolean? This function should
> just set a boolean, provided by the caller. If the *caller* wants it on the
> heap, then they can allocate it there. But your lowest level function has no
> reason to go and allocate this thing. Crazy...
I wanted the function to return *three* states of the executable
property: on, off, or non-existent. They correspond to the actions of
"do a +x", "do a -x", or "do nothing".
I could have returned the actual property value, but I thought it was
simpler to either return NULL, or return an *allocated* TRUE or FALSE.
You don't think that's simpler for a caller to parse? I did. Simpler
than having a caller parse an svn_string_t value, I think.
Not like it matters much -- everyone calls the function
svn_wc__maybe_toggle_working_executable_bit anyway, which has a
"normal" interface. Under the hood, it's currently the sole caller of
the "weird" function.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Apr 26 03:22:34 2002