Re: [PATCH] [Issue 870] import should set svn:executable automatically

Brian Denny <brian@briandenny.net> writes:

> - please critique. :)

Nice patch, can we have a log message please :)

> Index: subversion/libsvn_client/commit.c
> ===================================================================
> --- subversion/libsvn_client/commit.c
> +++ subversion/libsvn_client/commit.c Mon Sep 23 22:07:22 2002
[snip]
> + /* Set executable flag, depending on permissions for those
> + statuses user holds (owner, group, or other). */
> + *executable = FALSE;
> + if (! (is_user || is_group))
> + {
> + *executable = (file_info.protection & APR_WEXECUTE);
> + }
> + else
> + {
> + /* We must check both "owner" and "group" independently. */
> + if (is_user)
> + *executable = (file_info.protection & APR_UEXECUTE);
> +
> + if (is_group)
> + *executable |= (file_info.protection & APR_GEXECUTE);
> + }

Hmm, on Unix I think the logic is

   if (is_user)
     *executable = (file_info.protection & APR_UEXECUTE);
   else if (is_group)
     *executable = (file_info.protection & APR_GEXECUTE);
   else
     *executable = (file_info.protection & APR_WEXECUTE);

i.e. if there is group execute, but not user execute and I am both
user and group then I cannot execute. Your code does it differently,
and while there is no reason for Subversion to slavishly follow Unix
is there any reason for the algorithm you chose?

According to the APR documentation all this user/group stuff is only
available if APR_HAS_USER is defined, so we probably need

#ifdef APR_HAS_USER
#endif

around this code.

[snip]

> + /* If the file is executable, add that as a property to the file. */
> + SVN_ERR (is_executable (&executable, path, pool));
> + if (executable)
> + SVN_ERR (editor->change_file_prop (file_baton, SVN_PROP_EXECUTABLE,
> + svn_string_create ("true", pool),
> pool));

The value of the executable property is not relevant, only it's
presence or absence matters. I think "" would be better here, since
by setting it "true" we are encouraging the user to try setting it
"false".

-- 
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org