Loading ...
Sorry, an error occurred while loading the content.

Re: Login shell findings and beginning of implementation

Expand Messages
  • björn
    ... Ben, I ve looked over the patch a bit more carefully now. I moved all the work that was done in -[MMAppController launchVimProcessWithArguments:] into a
    Message 1 of 18 , Mar 2, 2008
    • 0 Attachment
      On 19/02/2008, björn <bjorn.winckler@...> wrote:
      > This looks very interesting...I will polish and merge it soon and then
      > let you look over my changes (if any). I can't say I understand the
      > stuff that goes on in MMLoginTask, so I leave it to other people to
      > comment on that part, but I get the gist of it.

      Ben, I've looked over the patch a bit more carefully now. I moved all
      the work that was done in -[MMAppController
      launchVimProcessWithArguments:] into a function executeInLoginShell().
      I managed to get rid of all the malloc calls as well...however I am
      wondering if the argv array needs to be alloc'ed? I would assume not,
      but you were malloc'ing everything and only freed argv0c which I
      assume was an unintentional memory leak? In any case, look over what
      I've done and tell me if it looks ok.

      I haven't implemented any proper error checking yet...that is on my
      todo list. I'm not really sure what to do if a new process fails to
      launch...pop up a dialog box? Surely it must only happen under very
      extreme circumstances.


      /Björn


      ----------------------------


      diff --git a/src/MacVim/MMAppController.m b/src/MacVim/MMAppController.m
      index 894db3b..68f6cdf 100644
      --- a/src/MacVim/MMAppController.m
      +++ b/src/MacVim/MMAppController.m
      @@ -54,6 +54,9 @@ typedef struct
      #pragma options align=reset


      +static int executeInLoginShell(NSString *path, NSArray *args);
      +
      +
      @interface MMAppController (MMServices)
      - (void)openSelection:(NSPasteboard *)pboard userData:(NSString *)userData
      error:(NSString **)error;
      @@ -116,6 +119,8 @@ typedef struct
      MMUntitledWindowKey,
      [NSNumber numberWithBool:NO], MMTexturedWindowKey,
      [NSNumber numberWithBool:NO], MMZoomBothKey,
      + @"", MMLoginShellCommandKey,
      + @"", MMLoginShellArgumentKey,
      nil];

      [[NSUserDefaults standardUserDefaults] registerDefaults:dict];
      @@ -736,61 +741,32 @@ typedef struct

      - (int)launchVimProcessWithArguments:(NSArray *)args
      {
      - NSString *taskPath = nil;
      - NSArray *taskArgs = nil;
      + int pid = -1;
      NSString *path = [[NSBundle mainBundle] pathForAuxiliaryExecutable:@"Vim"];

      if (!path) {
      NSLog(@"ERROR: Vim executable could not be found inside app bundle!");
      - return 0;
      + return -1;
      }

      + NSArray *taskArgs = [NSArray arrayWithObjects:@"-g", @"-f", nil];
      + if (args)
      + taskArgs = [taskArgs arrayByAddingObjectsFromArray:args];
      +
      if ([[NSUserDefaults standardUserDefaults] boolForKey:MMLoginShellKey]) {
      - // Run process with a login shell
      + // Run process with a login shell, roughly:
      // $SHELL -l -c "exec Vim -g -f args"
      - // (-g for GUI, -f for foreground, i.e. don't fork)
      -
      - NSMutableString *execArg = [NSMutableString
      - stringWithFormat:@"exec \"%@\" -g -f", path];
      - if (args) {
      - // Append all arguments while making sure that arguments containing
      - // spaces are enclosed in quotes.
      - NSCharacterSet *space = [NSCharacterSet whitespaceCharacterSet];
      - unsigned i, count = [args count];
      -
      - for (i = 0; i < count; ++i) {
      - NSString *arg = [args objectAtIndex:i];
      - if (NSNotFound != [arg rangeOfCharacterFromSet:space].location)
      - [execArg appendFormat:@" \"%@\"", arg];
      - else
      - [execArg appendFormat:@" %@", arg];
      - }
      - }
      -
      - // Launch the process with a login shell so that users environment
      - // settings get sourced. This does not always happen when MacVim is
      - // started.
      - taskArgs = [NSArray arrayWithObjects:@"-l", @"-c", execArg, nil];
      - taskPath = [[[NSProcessInfo processInfo] environment]
      - objectForKey:@"SHELL"];
      - if (!taskPath)
      - taskPath = @"/bin/sh";
      + pid = executeInLoginShell(path, taskArgs);
      } else {
      // Run process directly:
      // Vim -g -f args
      - // (-g for GUI, -f for foreground, i.e. don't fork)
      - taskPath = path;
      - taskArgs = [NSArray arrayWithObjects:@"-g", @"-f", nil];
      - if (args)
      - taskArgs = [taskArgs arrayByAddingObjectsFromArray:args];
      + NSTask *task = [NSTask launchedTaskWithLaunchPath:path
      + arguments:taskArgs];
      + pid = [task processIdentifier];
      }

      - NSTask *task =[NSTask launchedTaskWithLaunchPath:taskPath
      - arguments:taskArgs];
      - //NSLog(@"launch %@ with args=%@ (pid=%d)", taskPath, taskArgs,
      - // [task processIdentifier]);

      - int pid = [task processIdentifier];
      + // TODO: detect pid == -1

      // If the process has no arguments, then add a null argument to the
      // pidArguments dictionary. This is later used to detect that a process
      @@ -1099,3 +1075,103 @@ typedef struct
      return [self intValue];
      }
      @end // NSNumber (MMExtras)
      +
      +
      +
      +
      + static int
      +executeInLoginShell(NSString *path, NSArray *args)
      +{
      + // Start a login shell and execute the command 'path' with arguments 'args'
      + // in the shell. This ensures that user environment variables are set even
      + // when MacVim was started from the Finder.
      +
      + int pid = -1;
      + NSUserDefaults *ud = [NSUserDefaults standardUserDefaults];
      +
      + // Determine which shell to use to execute the command. The user
      + // may decide which shell to use by setting a user default or the
      + // $SHELL environment variable.
      + NSString *shell = [ud stringForKey:MMLoginShellCommandKey];
      + if (!shell || [shell length] == 0)
      + shell = [[[NSProcessInfo processInfo] environment]
      + objectForKey:@"SHELL"];
      + if (!shell)
      + shell = @"/bin/bash";
      +
      + //NSLog(@"shell = %@", shell);
      +
      + // Bash needs the '-l' flag to launch a login shell. The user may add
      + // flags by setting a user default.
      + NSString *shellArgument = [ud stringForKey:MMLoginShellArgumentKey];
      + if (!shellArgument || [shellArgument length] == 0) {
      + if ([[shell lastPathComponent] isEqual:@"bash"])
      + shellArgument = @"-l";
      + else
      + shellArgument = nil;
      + }
      +
      + //NSLog(@"shellArgument = %@", shellArgument);
      +
      + // Build input to send to the login shell.
      + NSMutableString *input = [NSMutableString stringWithFormat:
      + @"exec \"%@\"", path];
      + if (args) {
      + // Append all arguments while making sure that arguments containing
      + // spaces are enclosed in quotes.
      + NSEnumerator *e = [args objectEnumerator];
      + id obj;
      +
      + while ((obj = [e nextObject])) {
      + NSMutableString *arg = [NSMutableString stringWithString:obj];
      + [arg replaceOccurrencesOfString:@"'" withString:@"'\"'\"'"
      + options:NSLiteralSearch
      + range:NSMakeRange(0, [arg length])];
      + [input appendFormat:@" '%@'", arg];
      + }
      + }
      +
      + // Build the argument vector used to start the login shell.
      + NSString *shellArg0 = [NSString stringWithFormat:@"-%@",
      + [shell lastPathComponent]];
      + char *shellArgv[3] = { (char *)[shellArg0 UTF8String], NULL, NULL };
      + if (shellArgument)
      + shellArgv[1] = (char *)[shellArgument UTF8String];
      +
      + // Get the C string representation of the shell path before the fork since
      + // we must not call Foundation functions after a fork.
      + char *shellPath = [shell fileSystemRepresentation];
      +
      + // Fork and execute the process.
      + int ds[2];
      + if (pipe(ds)) return -1;
      +
      + pid = fork();
      + if (pid == -1) {
      + return -1;
      + } else if (pid == 0) {
      + // Child process
      + if (close(ds[1]) == -1) exit(255);
      + if (dup2(ds[0], 0) == -1) exit(255);
      +
      + execv(shellPath, shellArgv);
      +
      + // Never reached unless execv fails
      + exit(255);
      + } else {
      + // Parent process
      + close(ds[0]);
      +
      + // Send input to execute to the child process
      + [input appendString:@"\n"];
      + int bytes = [input lengthOfBytesUsingEncoding:NSUTF8StringEncoding];
      + write(ds[1], [input UTF8String], bytes);
      +
      + close(ds[1]);
      + }
      +
      + // TODO: error checking
      +
      + return pid;
      +}
      +
      diff --git a/src/MacVim/MacVim.h b/src/MacVim/MacVim.h
      index dee990b..07fb5cc 100644
      --- a/src/MacVim/MacVim.h
      +++ b/src/MacVim/MacVim.h
      @@ -222,6 +222,8 @@ extern NSString *MMUntitledWindowKey;
      extern NSString *MMTexturedWindowKey;
      extern NSString *MMZoomBothKey;
      extern NSString *MMCurrentPreferencePaneKey;
      +extern NSString *MMLoginShellCommandKey;
      +extern NSString *MMLoginShellArgumentKey;

      // Enum for MMUntitledWindowKey
      enum {
      diff --git a/src/MacVim/MacVim.m b/src/MacVim/MacVim.m
      index 10e9c47..b63e2b3 100644
      --- a/src/MacVim/MacVim.m
      +++ b/src/MacVim/MacVim.m
      @@ -102,6 +102,8 @@ NSString *MMUntitledWindowKey =
      @"MMUntitledWindow";
      NSString *MMTexturedWindowKey = @"MMTexturedWindow";
      NSString *MMZoomBothKey = @"MMZoomBoth";
      NSString *MMCurrentPreferencePaneKey = @"MMCurrentPreferencePane";
      +NSString *MMLoginShellCommandKey = @"MMLoginShellCommand";
      +NSString *MMLoginShellArgumentKey = @"MMLoginShellArgument";

      --~--~---------~--~----~------------~-------~--~----~
      You received this message from the "vim_mac" maillist.
      For more information, visit http://www.vim.org/maillist.php
      -~----------~----~----~----~------~----~------~--~---
    • Ben Schmidt
      Hi, Björn, ... That seems nice and neat. I originally made a separate class as it was acting as a drop-in replacement for NSTask, or very close to it, but
      Message 2 of 18 , Mar 5, 2008
      • 0 Attachment
        Hi, Björn,

        > Ben, I've looked over the patch a bit more carefully now. I moved all
        > the work that was done in -[MMAppController
        > launchVimProcessWithArguments:] into a function executeInLoginShell().

        That seems nice and neat. I originally made a separate class as it
        was acting as a drop-in replacement for NSTask, or very close to it,
        but then the calling code changed for some other reason and it wasn't an
        advantage any more.

        > I managed to get rid of all the malloc calls as well...however I am
        > wondering if the argv array needs to be alloc'ed? I would assume not,

        Yes, you're right. The only reason I was allocating dynamically was
        because I wrote the function generally, and since an argument vector
        could be passed in of arbitrary length it was necessary. Since you
        have integrated with the calling code, the argument list is now of
        bounded size so static allocation is fine. Exec will be just as happy
        with it.

        > but you were malloc'ing everything and only freed argv0c which I
        > assume was an unintentional memory leak?

        You are correct.

        > In any case, look over what I've done and tell me if it looks ok.

        On the whole, it does. Much nicer style than mine; OO and everything!
        A couple of code comments are wrong/misleading:

        Instead of

        // Run process with a login shell, roughly:
        // $SHELL -l -c "exec Vim -g -f args"

        a more accurate description of what is done is

        // Run process with a login shell, roughly:
        // echo "exec Vim -g -f args" | ARGV0=-`basename $SHELL` $SHELL [-l]

        (which is pseudo-code that won't work in any shell but communicates the gist
        of it, I think). You may still want to trim it down a bit, but it's better
        than the version with -c in it.

        Also the comment

        // Append all arguments while making sure that arguments containing
        // spaces are enclosed in quotes.

        is just wrong (my fault). Perhaps something like

        // Append all arguments, making sure they are properly quoted,
        // even when they contain single quotes.

        > I haven't implemented any proper error checking yet...that is on my
        > todo list.

        And this is the one real problem with the patch. The system calls in
        the parent process are not checked; they should at least return -1 so
        that later the pid==-1 can be detected if desired, and it doesn't
        try additional system calls after one has failed. I would recommend
        something like

        // Parent process
        if (close(ds[0])==-1) return -1;

        // Send input to execute to the child process
        [input appendString:@"\n"];
        int bytes = [input lengthOfBytesUsingEncoding:NSUTF8StringEncoding];
        if (write(ds[1], [input UTF8String], bytes)!=bytes) return -1;

        if (close(ds[1])==-1) return -1;

        (I just added the three 'return's).

        Also, shouldn't it #import <unistd.h> (for execv, fork, pipe, write, dup2,
        close)?

        > I'm not really sure what to do if a new process fails to
        > launch...pop up a dialog box? Surely it must only happen under very
        > extreme circumstances.

        Yeah. It surely can't happen often. It may be that it's fine to do nothing at
        all apart from return from the function at the earliest point that the error is
        detected, and just let the existing code pop up the 'process failed to
        launch/connect' dialog. (I seem to remember there is code that does this, but
        can't spot it with a quick grep, so maybe it was only something suggested on
        the mailing list? That would be useful, I think; detecting when no process
        connects after some time when it is expected, and directing the user to
        Console.app for possible messages.)

        At any rate, it's unlikely with the current code design that popping up a
        dialog via a different code path to one that would pick up any failed process
        launch would provide additional useful information (only if you were to
        implement it so that exceptions were thrown with more informative error
        messages would there be any point, I think, but as you say, the circumstances
        must surely be extreme when it happens, so I doubt it's worth the effort).

        Hope that helps, and isn't too much work. Sorry for not providing a
        revised patch. That's just a little more than I have time for right now,
        and the changes are minor (and wholly untested by me...but surely they
        couldn't go wrong?!...famous last words...you'd better at least do a test
        or two before merging it).

        O, and the new user defaults should be added to the documentation. You
        can use the text from my previous email as a starting point if it helps.

        Ben.



        Send instant messages to your online friends http://au.messenger.yahoo.com


        --~--~---------~--~----~------------~-------~--~----~
        You received this message from the "vim_mac" maillist.
        For more information, visit http://www.vim.org/maillist.php
        -~----------~----~----~----~------~----~------~--~---
      • björn
        ... Ok. Why did we abandon the -c? As far as I can remember it was because some shells (e.g. tcsh) couldn t cope with both -l and -c...but now that we put a
        Message 3 of 18 , Mar 5, 2008
        • 0 Attachment
          On 05/03/2008, Ben Schmidt <mail_ben_schmidt@...> wrote:
          >
          > On the whole, it does. Much nicer style than mine; OO and everything!
          > A couple of code comments are wrong/misleading:
          >
          > Instead of
          >
          >
          > // Run process with a login shell, roughly:
          > // $SHELL -l -c "exec Vim -g -f args"
          >
          >
          > a more accurate description of what is done is
          >
          >
          > // Run process with a login shell, roughly:
          >
          > // echo "exec Vim -g -f args" | ARGV0=-`basename $SHELL` $SHELL [-l]
          >
          > (which is pseudo-code that won't work in any shell but communicates the gist
          > of it, I think). You may still want to trim it down a bit, but it's better
          > than the version with -c in it.

          Ok. Why did we abandon the -c? As far as I can remember it was
          because some shells (e.g. tcsh) couldn't cope with both -l and
          -c...but now that we put a dash before argv[0] can't we go back to
          using -c instead of piping the 'exec ...' to the shell? (Bash seems
          to be able to cope with both -l and -c and that's the only shell which
          wanted the -l anyway.)

          > > I haven't implemented any proper error checking yet...that is on my
          > > todo list.
          >
          > And this is the one real problem with the patch. The system calls in
          > the parent process are not checked; they should at least return -1 so
          > that later the pid==-1 can be detected if desired, and it doesn't
          > try additional system calls after one has failed. I would recommend
          > something like
          >
          > // Parent process
          > if (close(ds[0])==-1) return -1;
          >
          >
          > // Send input to execute to the child process
          > [input appendString:@"\n"];
          > int bytes = [input lengthOfBytesUsingEncoding:NSUTF8StringEncoding];
          >
          > if (write(ds[1], [input UTF8String], bytes)!=bytes) return -1;
          >
          > if (close(ds[1])==-1) return -1;
          >
          > (I just added the three 'return's).

          Ok...I'll change that.

          >
          > Also, shouldn't it #import <unistd.h> (for execv, fork, pipe, write, dup2,
          > close)?

          It seems that Cocoa.h or something pulls this, but I added the #import
          anyway for clarity.

          > > I'm not really sure what to do if a new process fails to
          > > launch...pop up a dialog box? Surely it must only happen under very
          > > extreme circumstances.
          >
          > Yeah. It surely can't happen often. It may be that it's fine to do nothing at
          > all apart from return from the function at the earliest point that the error is
          > detected, and just let the existing code pop up the 'process failed to
          > launch/connect' dialog. (I seem to remember there is code that does this, but
          > can't spot it with a quick grep, so maybe it was only something suggested on
          > the mailing list? That would be useful, I think; detecting when no process
          > connects after some time when it is expected, and directing the user to
          > Console.app for possible messages.)
          >
          > At any rate, it's unlikely with the current code design that popping up a
          > dialog via a different code path to one that would pick up any failed process
          > launch would provide additional useful information (only if you were to
          > implement it so that exceptions were thrown with more informative error
          > messages would there be any point, I think, but as you say, the circumstances
          > must surely be extreme when it happens, so I doubt it's worth the effort).

          For now I think a simple log message to stderr will have to do.
          Hopefully we can iron out all possible cases where starting a process
          would fail so that we don't need such a dialog (it would be very
          technical and hence not very informative for the user anyway).


          > Hope that helps, and isn't too much work. Sorry for not providing a
          > revised patch. That's just a little more than I have time for right now,
          > and the changes are minor (and wholly untested by me...but surely they
          > couldn't go wrong?!...famous last words...you'd better at least do a test
          > or two before merging it).

          That does help, thanks! I don't mind looking over the code.


          > O, and the new user defaults should be added to the documentation. You
          > can use the text from my previous email as a starting point if it helps.

          Yeah, I'll do that too. :-)

          Now, one thing I noticed: if I set the MMLoginShellCommand user
          default to '/bin/tcsh' I get the following warning on stderr:

          Warning: no access to tty (Inappropriate ioctl for device).
          Thus no job control in this shell.

          The warning seems harmless (everything still works as far as I can
          tell) but it is a bit disconcerting. Any idea what can be done about
          it?

          I will create a proper patch and post it soon so anybody who so wishes
          can try it out.


          /Björn

          --~--~---------~--~----~------------~-------~--~----~
          You received this message from the "vim_mac" maillist.
          For more information, visit http://www.vim.org/maillist.php
          -~----------~----~----~----~------~----~------~--~---
        • björn
          ... Here s the patch as promised. I wrote some help, changed some comments, and improved the error checking. Ben, do you think it is ok now? Can some people
          Message 4 of 18 , Mar 6, 2008
          • 0 Attachment
            On 05/03/2008, björn <bjorn.winckler@...> wrote:
            > On 05/03/2008, Ben Schmidt <mail_ben_schmidt@...> wrote:
            > >
            > > On the whole, it does. Much nicer style than mine; OO and everything!
            > > A couple of code comments are wrong/misleading:
            > >
            > > Instead of
            > >
            > >
            > > // Run process with a login shell, roughly:
            > > // $SHELL -l -c "exec Vim -g -f args"
            > >
            > >
            > > a more accurate description of what is done is
            > >
            > >
            > > // Run process with a login shell, roughly:
            > >
            > > // echo "exec Vim -g -f args" | ARGV0=-`basename $SHELL` $SHELL [-l]
            > >
            > > (which is pseudo-code that won't work in any shell but communicates the gist
            > > of it, I think). You may still want to trim it down a bit, but it's better
            > > than the version with -c in it.
            >
            >
            > Ok. Why did we abandon the -c? As far as I can remember it was
            > because some shells (e.g. tcsh) couldn't cope with both -l and
            > -c...but now that we put a dash before argv[0] can't we go back to
            > using -c instead of piping the 'exec ...' to the shell? (Bash seems
            > to be able to cope with both -l and -c and that's the only shell which
            > wanted the -l anyway.)
            >
            >
            > > > I haven't implemented any proper error checking yet...that is on my
            > > > todo list.
            > >
            > > And this is the one real problem with the patch. The system calls in
            > > the parent process are not checked; they should at least return -1 so
            > > that later the pid==-1 can be detected if desired, and it doesn't
            > > try additional system calls after one has failed. I would recommend
            > > something like
            > >
            > > // Parent process
            > > if (close(ds[0])==-1) return -1;
            > >
            > >
            > > // Send input to execute to the child process
            > > [input appendString:@"\n"];
            > > int bytes = [input lengthOfBytesUsingEncoding:NSUTF8StringEncoding];
            > >
            > > if (write(ds[1], [input UTF8String], bytes)!=bytes) return -1;
            > >
            > > if (close(ds[1])==-1) return -1;
            > >
            > > (I just added the three 'return's).
            >
            >
            > Ok...I'll change that.
            >
            >
            > >
            > > Also, shouldn't it #import <unistd.h> (for execv, fork, pipe, write, dup2,
            > > close)?
            >
            >
            > It seems that Cocoa.h or something pulls this, but I added the #import
            > anyway for clarity.
            >
            >
            > > > I'm not really sure what to do if a new process fails to
            > > > launch...pop up a dialog box? Surely it must only happen under very
            > > > extreme circumstances.
            > >
            > > Yeah. It surely can't happen often. It may be that it's fine to do nothing at
            > > all apart from return from the function at the earliest point that the error is
            > > detected, and just let the existing code pop up the 'process failed to
            > > launch/connect' dialog. (I seem to remember there is code that does this, but
            > > can't spot it with a quick grep, so maybe it was only something suggested on
            > > the mailing list? That would be useful, I think; detecting when no process
            > > connects after some time when it is expected, and directing the user to
            > > Console.app for possible messages.)
            > >
            > > At any rate, it's unlikely with the current code design that popping up a
            > > dialog via a different code path to one that would pick up any failed process
            > > launch would provide additional useful information (only if you were to
            > > implement it so that exceptions were thrown with more informative error
            > > messages would there be any point, I think, but as you say, the circumstances
            > > must surely be extreme when it happens, so I doubt it's worth the effort).
            >
            >
            > For now I think a simple log message to stderr will have to do.
            > Hopefully we can iron out all possible cases where starting a process
            > would fail so that we don't need such a dialog (it would be very
            > technical and hence not very informative for the user anyway).
            >
            >
            >
            > > Hope that helps, and isn't too much work. Sorry for not providing a
            > > revised patch. That's just a little more than I have time for right now,
            > > and the changes are minor (and wholly untested by me...but surely they
            > > couldn't go wrong?!...famous last words...you'd better at least do a test
            > > or two before merging it).
            >
            >
            > That does help, thanks! I don't mind looking over the code.
            >
            >
            >
            > > O, and the new user defaults should be added to the documentation. You
            > > can use the text from my previous email as a starting point if it helps.
            >
            >
            > Yeah, I'll do that too. :-)
            >
            > Now, one thing I noticed: if I set the MMLoginShellCommand user
            > default to '/bin/tcsh' I get the following warning on stderr:
            >
            > Warning: no access to tty (Inappropriate ioctl for device).
            > Thus no job control in this shell.
            >
            > The warning seems harmless (everything still works as far as I can
            > tell) but it is a bit disconcerting. Any idea what can be done about
            > it?
            >
            > I will create a proper patch and post it soon so anybody who so wishes
            > can try it out.

            Here's the patch as promised. I wrote some help, changed some
            comments, and improved the error checking. Ben, do you think it is ok
            now?

            Can some people who use other shells than bash try it out and let me
            know how/if it works (don't forget to enable the "login shell" setting
            in the prefs). If there are problems applying the patch, let me know.

            Thanks,
            Björn

            --~--~---------~--~----~------------~-------~--~----~
            You received this message from the "vim_mac" maillist.
            For more information, visit http://www.vim.org/maillist.php
            -~----------~----~----~----~------~----~------~--~---
          • Ben Schmidt
            ... To be honest, I can t remember the full details, but I think it was because a number of shells simply don t support it. Either that, or they refused to be
            Message 5 of 18 , Mar 7, 2008
            • 0 Attachment
              > Ok. Why did we abandon the -c?

              To be honest, I can't remember the full details, but I think it was because a
              number of shells simply don't support it. Either that, or they refused to be login
              shells with -c even if they had the dash in argv[0]. I do know that I tried all
              combinations of using -l, using dash in argv[0], using a -c argument and piping
              the command in, and the one that worked best was the one in the patch (dash and
              piping, and an exception for bash).

              > For now I think a simple log message to stderr will have to do.
              > Hopefully we can iron out all possible cases where starting a process
              > would fail so that we don't need such a dialog (it would be very
              > technical and hence not very informative for the user anyway).

              I agree.

              > That does help, thanks! I don't mind looking over the code.

              Thanks a heap, Björn.

              >> O, and the new user defaults should be added to the documentation. You
              >> can use the text from my previous email as a starting point if it helps.
              >
              > Yeah, I'll do that too. :-)

              Great. :-)

              > Now, one thing I noticed: if I set the MMLoginShellCommand user
              > default to '/bin/tcsh' I get the following warning on stderr:
              >
              > Warning: no access to tty (Inappropriate ioctl for device).
              > Thus no job control in this shell.
              >
              > The warning seems harmless (everything still works as far as I can
              > tell) but it is a bit disconcerting. Any idea what can be done about
              > it?

              Yeah, it should be harmless, too--either tcsh is just trying to treat its end of
              the pipe as a terminal, which it isn't, or somehow trying to access a controlling
              terminal it doesn't have or something. I wonder why I didn't notice it before.
              Maybe it depends how you launch MacVim. I think it's probably a tcsh bug. I've had
              a bit of a Google which has revealed some things that can be investigated anyway
              as it seems a number of projects are faced with the same problem and work around
              it from outside of (t)csh. I don't think it's a showstopper, though, something we
              could investigate and make a second patch for.

              > I will create a proper patch and post it soon so anybody who so wishes
              > can try it out.

              Cool.

              Ben.





              Send instant messages to your online friends http://au.messenger.yahoo.com


              --~--~---------~--~----~------------~-------~--~----~
              You received this message from the "vim_mac" maillist.
              For more information, visit http://www.vim.org/maillist.php
              -~----------~----~----~----~------~----~------~--~---
            • Ben Schmidt
              [Note, this is my third reply in this thread tonight; sorry for the multiple posts. They are all different.] This is a quick and unimportant one, though. I
              Message 6 of 18 , Mar 7, 2008
              • 0 Attachment
                [Note, this is my third reply in this thread tonight; sorry for the multiple
                posts. They are all different.]

                This is a quick and unimportant one, though. I think it's a bit
                wrong/misleading/confusing having 'Launch Vim processes in a login shell' under
                'Whem MacVim launches' because it doesn't necessarily happen then, and probably
                happens a lot of other times besides.

                The window does look nice, though. I like it.

                Ben.




                Send instant messages to your online friends http://au.messenger.yahoo.com


                --~--~---------~--~----~------------~-------~--~----~
                You received this message from the "vim_mac" maillist.
                For more information, visit http://www.vim.org/maillist.php
                -~----------~----~----~----~------~----~------~--~---
              • Ben Schmidt
                ... The docs should note the behaviour that any shell whose basename is bash will automatically be given -l by MacVim, and that this can be counteracted by
                Message 7 of 18 , Mar 7, 2008
                • 0 Attachment
                  > Here's the patch as promised. I wrote some help, changed some
                  > comments, and improved the error checking. Ben, do you think it is ok
                  > now?

                  The docs should note the behaviour that any shell whose basename is 'bash' will
                  automatically be given '-l' by MacVim, and that this can be counteracted by an
                  MMLoginShellArgument of '--'. I don't see why anyone would want to change it, but
                  it should be documented, and definitely could be helpful if anyone ever uses the
                  docs to inform them about shell behaviour for another purpose or project.

                  There is also a typo in the docs patch: paramenter. :-)

                  Apart from that, all looks OK and I think once we get confirmation from a few more
                  people that it works, the default value of the 'use login shell' preference should
                  be changed to 'yes', too. Most users will want and expect that.

                  Ben.



                  Send instant messages to your online friends http://au.messenger.yahoo.com


                  --~--~---------~--~----~------------~-------~--~----~
                  You received this message from the "vim_mac" maillist.
                  For more information, visit http://www.vim.org/maillist.php
                  -~----------~----~----~----~------~----~------~--~---
                • björn
                  ... Done. I ve also pushed the patch to the public repo. ... I thought I :set spell -ed...ah well... :-) ... Yeah, I d like to enable it by default as soon
                  Message 8 of 18 , Mar 7, 2008
                  • 0 Attachment
                    On 07/03/2008, Ben Schmidt <mail_ben_schmidt@...> wrote:
                    >
                    > > Here's the patch as promised. I wrote some help, changed some
                    > > comments, and improved the error checking. Ben, do you think it is ok
                    > > now?
                    >
                    > The docs should note the behaviour that any shell whose basename is 'bash' will
                    > automatically be given '-l' by MacVim, and that this can be counteracted by an
                    > MMLoginShellArgument of '--'. I don't see why anyone would want to change it, but
                    > it should be documented, and definitely could be helpful if anyone ever uses the
                    > docs to inform them about shell behaviour for another purpose or project.

                    Done. I've also pushed the patch to the public repo.


                    > There is also a typo in the docs patch: paramenter. :-)

                    I thought I ":set spell"-ed...ah well... :-)


                    > Apart from that, all looks OK and I think once we get confirmation from a few more
                    > people that it works, the default value of the 'use login shell' preference should
                    > be changed to 'yes', too. Most users will want and expect that.

                    Yeah, I'd like to enable it by default as soon as possible (and get
                    rid of the "login shell..." option from the prefs panel). To
                    everybody using other shells than bash: please pull the latest version
                    and try it out (make sure to enable "login shell..." in the prefs).
                    Please let me know how it works.


                    /Björn

                    --~--~---------~--~----~------------~-------~--~----~
                    You received this message from the "vim_mac" maillist.
                    For more information, visit http://www.vim.org/maillist.php
                    -~----------~----~----~----~------~----~------~--~---
                  • Ben Schmidt
                    ... I think it might be wise to leave the option there. If ever someone shows up with a really wacky shell, they have something to try then. If it s not there,
                    Message 9 of 18 , Mar 7, 2008
                    • 0 Attachment
                      > Yeah, I'd like to enable it by default as soon as possible (and get
                      > rid of the "login shell..." option from the prefs panel).

                      I think it might be wise to leave the option there. If ever someone shows up with
                      a really wacky shell, they have something to try then. If it's not there, sure,
                      the thing to try is documented in the help, but if MacVim's not working, how can
                      you access the help?! It would make sense to me to, as a general rule, have
                      options that could prevent a Vim instance from starting kept in the prefs, even
                      though they should be rarely used.

                      Grins,

                      Ben.




                      Send instant messages to your online friends http://au.messenger.yahoo.com


                      --~--~---------~--~----~------------~-------~--~----~
                      You received this message from the "vim_mac" maillist.
                      For more information, visit http://www.vim.org/maillist.php
                      -~----------~----~----~----~------~----~------~--~---
                    Your message has been successfully submitted and would be delivered to recipients shortly.