Re: [PATCH/RFC 4/4] Add interactive mode to git-shell for user-friendliness

From: Greg Brockman <gdb@mit.edu>
Date: Jul 17, 2010, 04:12 UTC
Message-ID: <AANLkTikiOgV1iE7dwPUkLpWTb_zXSFdEuOYvyqJ1eDCo@mail.gmail.com>
In-Reply-To: <AANLkTikEjMeKPkyY4RdRq-ESkmmq4PvqCFPgp8yvLVBz@mail.gmail.com>

It looks like git@ got dropped from the CC at some point, but I had written a few days ago:

>> Is there a reason that you duplicate functionality offered by run_command()?
> No--I hadn't realized that existed.  I'll switch over to that in V2 of
> this patch series.

Today, I inspected run_command() in more detail.  Unfortunately, I'm
not sure of the best way to use it in this situation.  In particular,
run_command() uses execvp, meaning that PATH is invoked.  However, the
user should only be able to run commands in the git-shell-commands
directory.   I do have a few ideas for approaches here; maybe others
see more?  Anyway:
- Set PATH to just $HOME/git-shell-commands.  But then the helper
scripts have to restore PATH to a sane value themselves, and it's not
really clear to me what that value should be.
- Under the hood, exec a different script, which processes the user's
command on its own.  (So if the user types 'help' at the git shell
prompt, actually exec 'git-shell-wrapper help'.)  The
git-shell-wrapper could be a dumb wrapper that just execs
$HOME/git-shell-commands/help or similar.
- Extend run_command to optionally use execv.  Would any other code
actually want this functionality though?  If not, it's probably an
excessively large code change for little benefit.
- Continue using the one-off run() method that I wrote here.

Do people have opinions on the most elegant way to handle this?

Thanks!

Greg

Show 34 quoted lines

> On Wed, Jul 14, 2010 at 12:07 PM, Bernhard R. Link <brlink@debian.org> wrote:
>> * Thomas Rast <trast@student.ethz.ch> [100714 17:41]:
>>> [Please don't trim the Cc list without good reason.]
>>
>> The mail I answered to had only git@vger.kernel.org in CC and some
>> syntax errors in To.
>>
>>> Bernhard R. Link wrote:
>>> > To be more specific: If NULL is (void *)0 then it does not need to be
>>> > cast. Sadly the standard allows to define it as 0, and so it is on
>>> > some systems. So to be portable it needs to be cast to be a pointer,
>>> > otherwise the varargs argument is assumed to be an int.
>>>
>>> Worse, the pointer representations need not be the same between types,
>>> even though that is a fairly exotic idea:
>>>
>>>   http://c-faq.com/null/machexamp.html
>>>
>>> So it seems execl() must always have an explicitly-cast (char*)NULL
>>> sentinel.
>>
>> There is a difference between ugly operating systems where everything
>> else works and you need to cast it and things too exotic to have any
>> chance to get the rest of the code to work without big changes.
>>
>> Machines where you do not get a NULL pointer by a memset(,0,), calloc
>> or the like will have bigger problems anyway. (have not looked at git,
>> but I'd be suprised if at every place there is an explicit assignment
>> for the pointers).
>> Note that in the other examples, char * and void * are the same anyway.
>>
>>        Bernhard R. Link
>>
>