On Tue, Dec 13, 2011 at 8:01 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > > - That "have_..._ions()" is too long and ugly. I half expected that one and I agree. I vaguely remember typing it, deleting it and typing it again when I started on that one. > > - The only thing you care about this callsite is if you have enough > permission to execute the "cmd". > > In fact, you should not unconditionally require read permissions here. > > $ chmod a-r $(type --path git) && /bin/ls -l $(type --path git) > --wx--x--x 109 junio junio 5126580 Dec 13 09:47 /home/junio/git-active/bin/git > $ /home/junio/git-active/bin/git --version > git version 1.7.8.249.gb1b73 > > You may need read permission when the file is a script (i.e. not binary > executable). [...] > When checking if you can run "foo/bar/baz", directories "foo/" and "foo/bar/" > do not have to be readable. They only have to have executable bit to allow > descending into them, and typically this is called "searchable" (see man chmod). > > $ mkdir -p /var/tmp/a/b && cp $(type --path git) /var/tmp/a/b/git > $ chmod 111 /var/tmp/a /var/tmp/a/b > $ /var/tmp/a/b/git --version > git version 1.7.8.249.gb1b73 > > I'd suggest having two helper functions, instead of the single one with > overlong "have...ions" name. > > - can_search_directory() checks with access(X_OK); > > - can_execute_file() checks with access(X_OK|R_OK), even though R_OK is > not always needed. On the whole I like the suggestion. We should probably take it a bit further. Since the x and r bits basically have nothing to do with each other, and we need +rx only on scripts, I could just rely on fopen() for the +r check. I will still add the can_execute_file() and can_search_dir() helpers to support readability, as access(path, X_OK) means different things in the different contexts. I would then probably go for is_searchable() and is_executable() as function names. is_executable then means "is file and has executable flag set", is_searchable means "is directory and has executable flag set". Basically files won't be searchable and directories won't be executable. If execvp fails on a command that is executable, but not readable, it is definitely a script and we can generate an error in that case. 1/2 would then probably use access(path, R_OK), while 2/2 would start using fopen. Since fopen() uses the effective uid/gid, it then makes sense to use eaccess(3) instead of access(2) if available. It would be stupid to have bugs arise just because of a mismatch between the [ug]ids used by the two access checks. I'm aware of the fact that eaccess isn't a standard function, so a #define HAVE... fallback to at least access() would probably be required. > > Use the former here where you check the directory that contains the > command, and use the latter up above where you check the command that is > supposed to be executable, and also down below after you checked sb.buf is > a path to a file that may be the command that is supposed to be > executable. > > Then patch 2/2 can extend can_execute() to enhance its support for scripts > by reading the hash-bang line and validating it, etc. I'd rather keep the hash-bang check outside of that function and use can_execute/is_executable for checking the interpreter as well, if only for keeping the possibility of easily promoting them into an API. I'd rather move check_interpreter into where it's called now, but pull out the logic to find the interpreter. This will keep the error text generation in diagnose_execvp_eacces. I think the code will make more sense this way. There's tons of more errors that can be caused by a faulty interpreter, and it'll be easier to cover more cases this way in the future. Thanks for the insightful reviews so far. Let me know what you think, Frans -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html