Re: [Monotone-devel] Comments on mass_set?

[Nathaniel Smith]

On Wed, Aug 23, 2006 at 11:18:24PM -0400, Daniel Dickinson wrote:
> 
> Just wondering if anyone had any comments on the attr mass_set command
> patch I posted a while ago (and am attaching again).

Sorry about that!

> Also, I don't
> have a place to host a monotone database, so I was wondering how to
> make the modifications available.

Send me a key :-).


> #   set "examples/mtn-dosh"
> #  attr "group"
> # value "1001"
> # 
> #   set "examples/mtn-dosh"
> #  attr "perms"
> # value "664"
> # 
> #   set "examples/mtn-dosh"
> #  attr "user"
> # value "1001"
> # 
> #   set "examples/unix_attributes.lua"
> #  attr "group"
> # value "1001"
> # 
> #   set "examples/unix_attributes.lua"
> #  attr "perms"
> # value "664"
> # 
> #   set "examples/unix_attributes.lua"
> #  attr "user"
> # value "1001"

^^ is this really necessary?

> ============================================================
> --- examples/mtn-dosh 4b27481a2dc207c901c0acbbf761d5d26d2fdc69
> +++ examples/mtn-dosh 4b27481a2dc207c901c0acbbf761d5d26d2fdc69
> @@ -0,0 +1,4 @@
> +#!/bin/sh
> +TMPFILE=$1
> +shift
> +$1 $2 $3 $4 $5 $6 $7 $8 $9 > $TMPFILE

You can do execute("sh", "-c", ...), you know, if that's what you
really want :-).

Do note that you run the risk of people introducing shell
metacharacters into filenames, this way, which is a big security
issue... not necessarily important to fix this immediately, before
doing anything else, but there should at least be some big flashing
comments in the examples so people don't blindly start using them.

(Wouldn't that be better spelled as: "$@" > $TMPFILE?  My shell is a
bit rusty.)

> --- cmd_ws_commit.cc  f24529cd245deb0002fdaa15f0f8b6d1e9d0e338
> +++ cmd_ws_commit.cc  562517ec4d18604fd4a5e211aeb78895d131c196
> @@ -162,6 +162,8 @@ CMD(revert, N_("workspace"), N_("[PATH].
>  
>    // Race.
>    put_work_cset(excluded);
> +  // FIXME_ATTRS: All attributes are reverted, even if we specify
> +  // path to revert
>    update_any_attrs(app);
>    maybe_update_inodeprints(app);

Is this comment true?  update_any_attrs should make all attrs equal
the ones in the current work roster, and revert carefully leaves
behind any unreverted attr changes to that roster.

> -CMD(attr, N_("workspace"), N_("set PATH ATTR VALUE\nget PATH [ATTR]\ndrop 
> PATH [ATTR]"),
> -    N_("set, get or drop file attributes"),
> +CMD(attr, N_("workspace"), N_("set PATH ATTR VALUE\nget PATH [ATTR]\ndrop 
> PATH [ATTR]\nmass_set [PATH...]"),
> +    N_("set, get or drop file attributes\nor set recorded attributes to 
> match actual attributes on all\nfiles in PATH(s)."),
>      OPT_NONE)

Maybe "attr scan" would be a better name?

>  bool
> +lua_hooks::hook_get_avail_attr_update_functions(std::vector<std::string> & 
> avail_funcs)

Why do you make a distinction between attr update functions, and the
existing attr init functions?  They both seem to do exactly the same
thing -- look at a file on disk, and figure out what attrs it has?

> +bool
> +lua_hooks::hook_update_attribute(std::string const & inattr,
> +                                 file_path const & filename,
> +                                 std::string const & invalue,
> +                                 std::string & outvalue)

Likewise, how is this different from hook_apply_attribute?

>  void
>  update_current_roster_from_filesystem(roster_t & ros,
>                                        node_restriction const & mask,
> -                                      app_state & app);
> +                                      app_state & app,
> +                                      bool use_inodeprints_mode = true);

What's up with this?

>  void
> +perform_attr_update(std::vector<file_path> const & paths, app_state & app)

Likewise, perhaps this should be 'perform_attr_scan'?

> @@ -247,7 +333,7 @@ perform_additions(path_set const & paths
>    cset new_work;
>    make_cset(base_roster, new_roster, new_work);
>    put_work_cset(new_work);
> -  update_any_attrs(app);
> +  //  update_any_attrs(app);
>  }

If we're going to delete these lines, we should delete them, not just
comment them out :-).

> @@ -1041,7 +1127,8 @@ editable_working_tree::clear_attr(split_
>  editable_working_tree::clear_attr(split_path const & pth,
>                                    attr_key const & name)
>  {
> -  // FIXME_ROSTERS: call a lua hook
> +  file_path pth_unsplit(pth);
> +  app.lua.hook_apply_attribute(name(), pth_unsplit, "");  
>  }

We should make it possible for a hook to tell the difference between
an attr that is unset, and an attr which is merely set to the empty
string (since we work rather hard in the roster layer to preserve this
distinction, actually :-)).

At the lua level, the solution seems simple -- for an unset attr, call
the apply_attribute hook with the new attr_value of nil.  (Which in
lua is distinct from "".)  The hook system needs to expose that
capability a bit differently to C++, though.

-- Nathaniel

-- 
Details are all that matters; God dwells there, and you never get to
see Him if you don't struggle to get them right. -- Stephen Jay Gould