You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Jeff Bellegarde <be...@peak.org> on 2002/09/25 14:38:00 UTC
Re: [PATCH] [Issue 870] import should set svn:executable automatically
Dang. The same thing I was strating to work on. I knew I should have announced
I was working on it earlier. Oh well.
My solution had the same basic concept but a few differences I'ld like to
explore.
I put the code into 'svn_wc_add' in 'adm_ops.c'. I believe this does the work
when the 'svn add' command is given which would allow the user to change
'svn:executable' before commiting. If it's done at commit time the user will
have to make another change to fix it.
I'm not sure it's worthwile to check the actual user running svn. Generally
the same command on the same file should behave the same regardless of who
ran the command. If a file is marked as user executable it should be
considered user executable regardless of the user involved. The code is also
smaller and I tend to favor small solutions where possible.
Last I checked Windows considered *.com, *.exe, and *.bat to be executable and
everything else as data. It's been a few years since I cared so I could be
completely wrong by now. Any windows executable is unlikely to work on any
unix and vice versa. We may be better off making all files added on Windows
data and only checking on Unix.
I'm still looking at the testing situation. I think I see how to do it now.
I'll try to get something to Brian if I can figure it out.
On Wednesday 25 September 2002 03:20 pm, Brian Denny wrote:
> just cuz it seemed like a good one to get my feet wet with. :)
>
> notes/questions:
> - i assume that correct behavior is to set the svn:executable if
> the file is actually executable by the user running svn. is
> this correct? (as opposed to, for example, if *any* of the
> executable bits are set).
>
> - i created a helper method "is_executable" to determine if given
> file is executable by current user. it seems like a pretty
> general-purpose utility; but it's not currently used anywhere
> other than this one place. should (things like) this go in the
> public interface?
>
> - it seems like new tests should be written for this functionality,
> but i don't yet grok the existing test code well enough to be able
> to work this in. maybe in a few days. let me know if you wish to
> wait for the tests before accepting the patch.
>
> - there is a note in the issue tracker that this mechanism should
> defer to the mechanism described in issue 869 where applicable.
> as issue 869 has not been done, this is not addressed by the patch.
>
> - please critique. :)
>
> :brian
--
-- Jeff Bellegarde
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: updated [PATCH] [Issue 870] import should set svn:executable
automatically
Posted by Branko Čibej <br...@xbc.nu>.
Brian Denny wrote:
>This version of the patch incorporates several of the ideas/requests
>put forth on this list.
>
>Tests are still absent. (Working on it; have some specific questions
>which will go in a separate e-mail).
>
>Also, I haven't exercised this code on Windows as I am not yet set up
>to build there. What it *should* do on Windows is: nothing different
>from before (i.e., do not set svn:executable automatically).
>
>
This patch looks quite nice and correct. I'd cange the body of
svn_io_is_file_executable a bit, something like this:
{
#if defined(APR_HAS_USER) && !defined(SVN_WIN32)
<all the stuff that's conditional now>
#else
*executable = FALSE;
#endif
return SVN_NON_ERROR;
}
The less #ifdefs there are, the better I like it. :-) Otherwise I think it's cool.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
updated [PATCH] [Issue 870] import should set svn:executable
automatically
Posted by Brian Denny <br...@briandenny.net>.
This version of the patch incorporates several of the ideas/requests
put forth on this list.
Tests are still absent. (Working on it; have some specific questions
which will go in a separate e-mail).
Also, I haven't exercised this code on Windows as I am not yet set up
to build there. What it *should* do on Windows is: nothing different
from before (i.e., do not set svn:executable automatically).
-brian
Addresses issue 870: on import (and add), sets the svn:executable property
of files according to whether the file is actually executable by the
current user.
Thanks to the many people who contributed suggestions for this patch.
* subversion/include/svn_io.h : add header for new function
* subversion/libsvn_subr/io.c : add function 'svn_io_is_file_executable'
* subversion/libsvn_client/commit.c
(import_file) : Check if file to be imported is executable;
if so, set svn:executable property.
* subversion/libsvn_wc/adm_ops.c
(svn_wc_add) : Check if file to be added is executable;
if so, set svn:executable property.
Jeff Bellegarde suggested checking on add.
Index: subversion/include/svn_io.h
===================================================================
--- subversion/include/svn_io.h
+++ subversion/include/svn_io.h Thu Sep 26 21:48:06 2002
@@ -193,6 +193,16 @@
svn_boolean_t ignore_enoent,
apr_pool_t *pool);
+/* Determine whether a file is executable by the current user.
+ * Set *EXECUTABLE to TRUE if the file PATH is executable by the
+ * current user, otherwise set it to FALSE.
+ *
+ * On Windows and on platforms without userids, always returns FALSE.
+ */
+svn_error_t *svn_io_is_file_executable(svn_boolean_t *executable,
+ const char *path,
+ apr_pool_t *pool);
+
/* Read a line from FILE into BUF, but not exceeding *LIMIT bytes.
* Does not include newline, instead '\0' is put there.
Index: subversion/libsvn_wc/adm_ops.c
===================================================================
--- subversion/libsvn_wc/adm_ops.c
+++ subversion/libsvn_wc/adm_ops.c Thu Sep 26 21:30:03 2002
@@ -722,6 +722,7 @@
enum svn_node_kind kind;
apr_uint32_t modify_flags = 0;
const char *mimetype = NULL;
+ svn_boolean_t executable = FALSE;
svn_error_t *err;
svn_wc_adm_access_t *adm_access;
@@ -838,6 +839,17 @@
mt_str.data = mimetype;
mt_str.len = strlen(mimetype);
SVN_ERR (svn_wc_prop_set (SVN_PROP_MIME_TYPE, &mt_str, path,
+ parent_access, pool));
+ }
+
+ /* Set svn:executable if the new addition is executable. */
+ SVN_ERR (svn_io_is_file_executable (&executable, path, pool));
+ if (executable)
+ {
+ svn_string_t emptystr;
+ emptystr.data = "";
+ emptystr.len = 0;
+ SVN_ERR (svn_wc_prop_set (SVN_PROP_EXECUTABLE, &emptystr, path,
parent_access, pool));
}
}
Index: subversion/libsvn_subr/io.c
===================================================================
--- subversion/libsvn_subr/io.c
+++ subversion/libsvn_subr/io.c Thu Sep 26 21:29:31 2002
@@ -641,6 +641,51 @@
}
+svn_error_t *
+svn_io_is_file_executable(svn_boolean_t *executable,
+ const char *path,
+ apr_pool_t *pool)
+{
+#ifdef APR_HAS_USER
+#ifndef SVN_WIN32
+ apr_finfo_t file_info;
+ apr_status_t apr_err;
+ apr_uid_t uid;
+ apr_gid_t gid;
+ svn_boolean_t is_user;
+ svn_boolean_t is_group;
+#endif
+#endif
+
+ *executable = FALSE;
+
+#ifdef APR_HAS_USER
+#ifndef SVN_WIN32
+ /* Get file and user info. */
+ SVN_ERR (svn_io_stat (&file_info, path,
+ (APR_FINFO_PROT | APR_FINFO_OWNER),
+ pool));
+ apr_err = apr_current_userid (&uid, &gid, pool);
+
+ if (apr_err)
+ return svn_error_create(apr_err, 0, NULL, pool,
+ "Error getting UID of process.");
+
+ /* Check executable bit for current user. */
+ if (apr_compare_users(uid, file_info.user) == APR_SUCCESS)
+ *executable = (file_info.protection & APR_UEXECUTE);
+
+ else if (apr_compare_groups(gid, file_info.group) == APR_SUCCESS)
+ *executable = (file_info.protection & APR_GEXECUTE);
+
+ else
+ *executable = (file_info.protection & APR_WEXECUTE);
+#endif
+#endif
+ return SVN_NO_ERROR;
+}
+
+
/*** Generic streams. ***/
Index: subversion/libsvn_client/commit.c
===================================================================
--- subversion/libsvn_client/commit.c
+++ subversion/libsvn_client/commit.c Wed Sep 25 19:14:01 2002
@@ -112,6 +112,7 @@
apr_pool_t *subpool = svn_pool_create (hash_pool);
const char *filepath = apr_pstrdup (hash_pool, path);
struct imported_file *value = apr_palloc (hash_pool, sizeof (*value));
+ svn_boolean_t executable;
/* Add the file, using the pool from the FILES hash. */
SVN_ERR (editor->add_file (edit_path, dir_baton, NULL, SVN_INVALID_REVNUM,
@@ -123,6 +124,13 @@
if (mimetype)
SVN_ERR (editor->change_file_prop (file_baton, SVN_PROP_MIME_TYPE,
svn_string_create (mimetype, pool),
+ pool));
+
+ /* If the file is executable, add that as a property to the file. */
+ SVN_ERR (svn_io_is_file_executable (&executable, path, pool));
+ if (executable)
+ SVN_ERR (editor->change_file_prop (file_baton, SVN_PROP_EXECUTABLE,
+ svn_string_create ("", pool),
pool));
if (notify_func)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] [Issue 870] import should set svn:executable automatically
Posted by Greg Stein <gs...@lyra.org>.
On Wed, Sep 25, 2002 at 04:14:03PM -0700, Brian Denny wrote:
>...
> > completely wrong by now. Any windows executable is unlikely to work on any
> > unix and vice versa. We may be better off making all files added on Windows
> > data and only checking on Unix.
>
> a couple people have said this now; it sounds like a nice easy solution
> to me. :)
Yup. It is quite valid to simply say, "it will get automatically set on Unix
unless/until somebody figures out the Right Behavior(tm) for Windows and
gets the code update."
IOW, we get better functionality [on Unix] today, and we have a possibility
in the future for other platforms (essentially, a framework for Windows,
Mac, whatever, to add specific code).
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
Re: [PATCH] [Issue 870] import should set svn:executable automatically
Posted by Brian Denny <br...@briandenny.net>.
On 25 Sep 2002, Ben Collins-Sussman wrote:
> Jeff Bellegarde <be...@peak.org> writes:
>
> > I put the code into 'svn_wc_add' in 'adm_ops.c'. I believe this does
> > the work when the 'svn add' command is given which would allow the
> > user to change 'svn:executable' before commiting. If it's done at
> > commit time the user will have to make another change to fix it.
>
> Actually, this may be a better solution. It means that we get the
> property automatically set not just at 'svn import' time, but when the
> user runs plain old 'svn add'. This would mirror our current
> binary-detection behavior, whereby we (possibly) set the
> svn:mime-type.
AFAICT code needs to go in both places ('import_file' in commit.c
as well as 'svn_wc_add' in adm_ops.c) in order to affect import
as well as add. this seems to be what happens with mime-type.
-brian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] [Issue 870] import should set svn:executable automatically
Posted by Ben Collins-Sussman <su...@collab.net>.
Jeff Bellegarde <be...@peak.org> writes:
> I put the code into 'svn_wc_add' in 'adm_ops.c'. I believe this does
> the work when the 'svn add' command is given which would allow the
> user to change 'svn:executable' before commiting. If it's done at
> commit time the user will have to make another change to fix it.
Actually, this may be a better solution. It means that we get the
property automatically set not just at 'svn import' time, but when the
user runs plain old 'svn add'. This would mirror our current
binary-detection behavior, whereby we (possibly) set the
svn:mime-type.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] [Issue 870] import should set svn:executable automatically
Posted by Brian Denny <br...@briandenny.net>.
[replying to self]
>
> another possible use case is:
> - you have a file already under svn control, not executable
> % svn propset svn:exectuable "" foo.sh
> now is foo.txt being exectuable in wc
> (without having to chmod it also)?
> this makes sense to me, because if you do a commit and an update,
> it will be executable. but OTOH maybe it's too weird for an svn propset
> to be tweaking your wc.
>
oops, apparently it already does this! silly me.
-brian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] [Issue 870] import should set svn:executable automatically
Posted by Brian Denny <br...@briandenny.net>.
> I put the code into 'svn_wc_add' in 'adm_ops.c'. I believe this does the work
> when the 'svn add' command is given which would allow the user to change
> 'svn:executable' before commiting. If it's done at commit time the user will
> have to make another change to fix it.
oh yeah, i hadn't thought of that one. i think it sounds good.
but does this take care of the import case?
i'd thought of this use case, forgot to ask about it, but personally lean
against it:
- add and commit a file, not executable.
chmod the file in the working copy to be
executable. commit.
should this set svn:executable?
another possible use case is:
- you have a file already under svn control, not executable
% svn propset svn:exectuable "" foo.sh
now is foo.txt being exectuable in wc
(without having to chmod it also)?
this makes sense to me, because if you do a commit and an update,
it will be executable. but OTOH maybe it's too weird for an svn propset
to be tweaking your wc.
> I'm not sure it's worthwile to check the actual user running svn. Generally
> the same command on the same file should behave the same regardless of who
> ran the command. If a file is marked as user executable it should be
> considered user executable regardless of the user involved.
why? if a file is marked as user executable, that doesn't mean it's
exectuable by the user involved... idunno -- when i check out a
file with svn:executable set, i get all the executable bits set (not
sure if/how this interacts with umask). so there's some asymmetry
anyway. i have no strong opinion on this.
> Last I checked Windows considered *.com, *.exe, and *.bat to be executable and
> everything else as data. It's been a few years since I cared so I could be
> completely wrong by now. Any windows executable is unlikely to work on any
> unix and vice versa. We may be better off making all files added on Windows
> data and only checking on Unix.
a couple people have said this now; it sounds like a nice easy solution
to me. :)
> I'm still looking at the testing situation. I think I see how to do it now.
> I'll try to get something to Brian if I can figure it out.
cool. :)
:) brian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org