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

Re: svn commit: rev 1784 - trunk trunk/subversion/include trunk/subversion/libsvn_wc trunk/subversion/libsvn_subr

From: Ben Collins-Sussman <sussman_at_collab.net>
Date: 2002-04-26 03:18:11 CEST

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

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