[darcs-devel] darcs patch: added postget to prefs (and 2 more)

David Roundy droundy at abridgegame.org
Wed Aug 3 05:19:44 PDT 2005


On Tue, Aug 02, 2005 at 09:47:03AM -0700, dagit at eecs.oregonstate.edu wrote:
> ... But I was under the impression that to get the postget hook working
> we would need to read the per-repository defaults of the exising repo
> which is possibly remote.

Right, I was thinking about the other sorts of posthooks (which are
easier).

> This is where I got stuck, the existing code for inspecting a default
> flag assumes two things,
[my reformatting...]
> 1) the defaults can be read inside the ReadableDirectory monad which as
> near as I can tell assumes that the files are connected to the local file
> system,

> 2) that the defaults come from both the local repo and ~/.darcs/defaults.

> Both (1) & (2) make it difficult to reuse the existing code to fetch what
> I'm calling 'remote defaults'.  I would like to finish implementing the
> posget hook since I've already come so far with it.  With the code I have
> since sending in my first patch I could stop and have a local, user
> defined default for postget.  BTW, I added the commandline switch
> --postget-command=COMMAND.  But, I don't think postget is actually useful
> until it can read the remote defaults and use them.
> 
> So I guess my email was asking for advice on how to change assumptions
> (1)&(2) so that implementing this will be acceptable. I could also just
> copy, paste, and edit the existing code, creating two copies of almost
> identical functionality that would then run the risk of diverging, but
> that's a last resort.  That is the path I started down when I mimiced
> show_motd to fetch the remote defaults file.  I don't think just adding a
> parameter for the specification of the remote repo's path will be enough,
> as it looks like ReadableDirectory needs a path which is local.  Perhaps
> I'm wrong here?

Yeah, the ReadableDirectory monad requires that you can implement
mDoesDirectoryExist, which isn't really possible over http.  The reason
this stuff is in ReadableDirectory is because we need to use it when we
apply patches (which may be applied to something that *isn't* a local
directory (but isn't on-disk either).  But that's only needed for
"prefs/prefs" itself, not for everything else in the prefs/ directory.

I think we could perhaps implement a modified

get_default_flag :: [Repository] -> String -> DarcsOption -> IO [DarcsFlag]

where the first argument would be a list of repositories to check for
defaults (in order).  Under normal circumstances, it would have one
element, which is the current repository.  For get, it might have one
element, which would be the remote repository.  For pull (if we decided to
implement this) it might have two elements, the local followed by the
remote repository.

Repository.prefsUrl gives us the location of the prefs directory on that
repository (which will vary depending if it's darcs format or git format),
which we can use together with fetchFilePS (or whatever it's called) to
grab the defaults file.

The trick will be in how to decide what repositories to search, and in
ensuring that we have moderate security--we don't ever want to silently use
remote defaults that could be possibly dangerous.

But the reworking of get_default_flag to not use RepoPrefs, but instead get
as its argument a list of repositories should be safe and an improvement
anyways.

Perhaps we could use do the "prompting" at a higher level, by doing
something like

get_default_flag :: [Repository] -> [Repository]
                 -> String -> DarcsOption -> IO [DarcsFlag]

where the first [Repository] is "safe" repositories, from which we silently
accept defaults, and the second list is "risky" (i.e. remote) repositories,
and we prompt the user for each default that we might want to accept from
there.  The user prompt would ask if the user would like to add that
default into the local repository (and never get asked about it again), or
ignore it in the future (which would require a bit of tricky code), or
perhaps ignore all defaults from that repository in the future, or accept
it this time, etc.  This would have the advantage that we'd be pretty well
ensured against security risks caused by forgetting to check for a specific
flag type, and would have the advantage that all flags could be
automatically handled transparently.  The downside is that it might lead to
excessive prompting.

There's also the question of how (and when) to decide on which remote
repositories to check for prefs.  I'm imagining that we'd add either a
function or a flag to the DarcsCommand that would handle this.  It needs to
be done before we actually execute the command, since the defaults need to
be passed to the command as arguments.

> > In this case, I'd implement it at a higher level, in
> > DarcsCommands.consider_running.
> 
> Working at this higher level will let me use the defaults from the remote
> repository?  I had not examined consider_running, perhaps I'll have to do
> that tonight.

No, working at a higher level won't address the postget "remote defaults"
issue, what it will do is to make your run_posthook function unnecesary,
which would mean that adding posthook support to a command will only
require that the command add the posthook flag to its list of DarcsOptions,
which seems like it would be nice, and the posthook would be guaranteed to
always run on command success (i.e. if the command exits with ExitSuccess).
It would mean that we'd have to be careful about when the darcs exits with
ExitSuccess... i.e. the question of whether to exit successfully when there
are no patches to pull/apply could become significant.

Actually, I'm thinking that if we implemented posthooks in this manner, we
could avoid modifying the commands at all, and add the DarcsOption like we
do the --help and --list-options flags in DarcsCommands itself.  Then every
command would support --posthook, and they'd all behave in the same way.

The trick, by the way, would be to use (in consider_running) code something
like

  do (command_command cmd) (FixFilePath fix_path:specops) extra
           `Control.Exception.catch` foo specops
     foo (ExitSuccess) specops
  where foo (ExitException ExitSuccess) opts = run_posthook opts
        foo e = Control.Exception.throwIO e

which would mean that if the command either doesn't call exitWith (and just
returns) or calls "exitWith ExitSuccess", the posthook will get run.
-- 
David Roundy
http://www.darcs.net




More information about the darcs-devel mailing list