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.
>...
> +++ trunk/subversion/libsvn_wc/log.c Thu Apr 25 16:21:44 2002
>...
> @@ -106,18 +107,26 @@
> svn_wc_keywords_t *keywords;
> const char *eol_str;
> enum svn_wc__eol_style style;
> + svn_boolean_t toggled;
> +
> SVN_ERR (svn_wc__get_keywords (&keywords, full_dest_path->data,
> NULL, pool));
> SVN_ERR (svn_wc__get_eol_style (&style, &eol_str, full_dest_path->data,
> pool));
>
> - return svn_wc_copy_and_translate (full_from_path->data,
> - full_dest_path->data,
> - eol_str,
> - TRUE,
> - keywords,
> - TRUE,
> - pool);
> + 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? ]
>...
> +++ trunk/subversion/libsvn_wc/translate.c Thu Apr 25 16:21:44 2002
> @@ -1040,6 +1040,69 @@
> }
>
>
> +
> +svn_error_t *
> +svn_wc__get_executable_prop (svn_boolean_t **wants_exec,
> + const char *path,
> + apr_pool_t *pool)
> +{
> + const svn_string_t *propval;
> +
> + SVN_ERR (svn_wc_prop_get (&propval, SVN_PROP_EXECUTABLE, path, pool));
> +
> + if (propval == NULL)
> + *wants_exec = NULL;
> +
> + else if ((! apr_strnatcasecmp (propval->data, "true"))
> + || (! apr_strnatcasecmp (propval->data, "on")))
> + {
> + *wants_exec = apr_pcalloc (pool, sizeof (*wants_exec));
> + **wants_exec = TRUE;
> + }
> +
> + 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().
> + {
> + *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...
>...
> +svn_wc__maybe_toggle_working_executable_bit (svn_boolean_t *toggled,
> + const char *path,
> + apr_pool_t *pool)
> +{
> + svn_boolean_t *wants_exec;
Just plain old: svn_boolean_t wants_exec;
> + SVN_ERR (svn_wc__get_executable_prop (&wants_exec, path, pool));
> +
> + if (wants_exec != NULL)
> + {
> + if (*wants_exec)
Plain old: if (wants_exec)
Simplify! :-)
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Apr 26 02:48:07 2002