[darcs-devel] Exporting env vars to the posthook

[Jason Dagit]

On 8/2/06, Nathaniel Gray <n8gray at caltech.edu> wrote:
> Jason Dagit wrote:
> > On 8/2/06, Eric Y. Kow <eric.kow at gmail.com> wrote:
> >> Some initial comments:
> >>
> >> On Wed, Aug 02, 2006 at 18:14:40 -0700, Nathaniel Gray wrote:
> >> > Aside from some aesthetic objections, the only serious problem I see is
> >> > that changing the method used to execute the posthook from
> >> > System.Cmd.system to System.Process.runProcess will break any posthook
> >> > command that counts on being processed by the shell.
> >
> > I don't like this new restriction.  Every posthook I've ever written
> > depended on the shell.  It's not the end of the world obviously, but
> > I'd rather not add this restriction.  What is the technical reason for
> > this?  Is it because you couldn't pass the new environment with
> > System.Cmd.system?  If that's the case, could you try adding the
> > environment variables in darcs's process before calling
> > System.Cmd.system?  It may be the case that the command inherits the
> > environment of darcs.  This would also need to be checked by someone
> > on windows as it may not be portable.
>
> Yes, the issue is that system doesn't accept an environment.  There's
> the option of putting the variables into darcs' environment, but AFAIK
> the way to do that is System.Posix.setEnv, which is described as
> non-portable.

Sad sad sad.  Why is windows so painful? :)  Windows does have an
environment we can tap into somehow, but I'm no windows expert.

> >> On the other hand... maybe it would be useful in general if darcs
> >> commands returned some information instead of just exiting.
> >>
> >> 1. What would you all think if we had a mechanism like this, only
> >>    renamed from PostHookEnvironmentVariable to DarcsCmdStatus,
> >>    and implemented as a record (instead of an attribute value list)
> >>    ?  See below.
> >
> > I'm with you so far.  In sort of a "oh gee, that's really obvious, why
> > isn't it that way already..." way.
>
> You'll have to correct me if any of this is wrong, since this is my
> first experience with Haskell, but I don't think records are the way to
> go.  Using records means every command has to supply every field.  Why
> should "darcs add" need to care about patchesApplied?  It's bad enough
> that all the possible env vars have to be defined in one place -- it
> would be much nicer if common vars were defined together and vars that
> only one command will supply were defined with that command, but I'm not
> sure how to achieve that elegantly in Haskell.

It's not as bad as you might think.  The record update syntax only
makes you specify the parts you want to update.  Combine this with
maybe and you get nice results.

mkCmdStatus e = CmdStatus { cmdStatus'PatchesApplied = Nothing
                        , cmdStatus'ExitCode = e }

Then when a command returns it does something like:

(mkCmdStatus ExitSuccess) { cmdStatus'PatchesApplied = Just 3 }

Or perhaps even the exit code could be a maybe, in that case:
mkCmdStatus = { cmdStatus'PatchesApplied = Nothing
                      , cmdStatus'ExitCode = Nothing
                      }

mkCmdStatus { cmdStatus'ExitCode = Just ExitSuccess }

That would be the case when no patches were applied but the command
ran sucessfully.  It's not so bad, the unspecified fields get the
value Nothing and then we ignore them.

> > I'm not sure if the formatting will look right when I send this.
> > Basically, I line up the comma with the opening and closing bracket,
> > line up the type signatures and then use the funny names.
>
> I'll leave the discussions of indentation to those who have to live with
> the code.  Suffice to say it all looks weird to me.  ;^)

Give it some time :)  I find that some of the more 'odd' haskell
conventions become the ones I like best.

Jason