Re: [Monotone-devel] Comments on mass_set?

[Daniel Dickinson]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Fri, 25 Aug 2006 04:56:59 -0700
Nathaniel Smith <address@hidden> wrote:

> On Wed, Aug 23, 2006 at 11:18:24PM -0400, Daniel Dickinson wrote:
[snip]
> > 
> > #   set "examples/unix_attributes.lua"
> > #  attr "user"
> > # value "1001"
> 
> ^^ is this really necessary?

Erp, no that was an accident; I have these attr_init_functions defined
in my homedir monotonerc so the attributes are picked up when I add
files and I didn't clean up mtn diff.  I'm thinking that tracking
filesystem attributes might be best done on a per-workspace fashion
using _MTN/monotonerc because most of the time they're probably not
needed (and using the _MTN/monotonerc structure already present gives
the flexibility of doing this when it does matter).
> 
> > ============================================================
> > --- 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 :-).

When I first started working on this I didn't know lua, so I did this.
Now I think I could get the parameters into a string the way I need,
but this is just a stopgap until execute_out as a lua function is
available (which I intend to do again using Timothy's suggestion now
that spawn_redir is available).

> 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.

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

shift doesn't affect $@ that I can see (I did try that first).

> 
> > --- 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.

I think that's me mixing my terminology.  By revert I meant that
whatever is in monotone (the current roster) is applied to all files
in the roster using update_attr_functions, even if we specify a path.
This conflicts with my usage of attributes as another piece of data
(more like the contents of a file, rather than like a filename which
is what the current usage seems to be).

> > -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?

I like it; will do.
 
> >  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?

attr_update_functions is given the current value of the attribute while
attr_init_functions doesn't care and just gets the filename.  I'm not
sure whether that really matters as I can't think of a specific use
case where that is important.

> > +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?

This is the inverse operation (basically hook_get_attribute, only it
passes the current value as well, if invalue was dropped it'd be like
a single file version of hook_init_attributes, which I wanted to be
able to make it easier to know when an attribute was cleared
(using an hook_init_attributes style map a missing attribute may
never have been set, or maybe it was cleared).

> 
> >  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?

AIUI inodeprints aren't affected by attribute changes (at the
filesystem level), therefore if inodeprints are in use, attr scan might
fail to pick up changes to files not otherwise changed.

> >  void
> > +perform_attr_update(std::vector<file_path> const & paths,
> > app_state & app)
> 
> Likewise, perhaps this should be 'perform_attr_scan'?

Ok.
 
> > @@ -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 :-).

Oh, I just wanted to make sure it was right first.

> > @@ -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 :-)).

Hmmm...okay I'll have to check my other code too; I didn't realize
there was a distinction.  I'm not sure, for example,
that attr_init_functions makes that distinction.  I'll review that in a bit.

> 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.

I noticed you had full_attr_map_t in the roster, but it doesn't seem to
get exposed to the hooks.  Should we just make full_attr_map_t
available to hooks & work, or is something else preferred?

Thanks !

Daniel

- -- 
GnuPG Key Fingerprint 86 F5 81 A5 D4 2E 1F 1C      http://gnupg.org
And that's my crabbing done for the day.  Got it out of the way early, 
now I have the rest of the afternoon to sniff fragrant tea-roses or 
strangle cute bunnies or something.   -- Michael Devore
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD8DBQFE75SHhvWBpdQuHxwRAjcbAJ9B3iWwwQPvaLBitBs0BSFhzolCNQCfU7fo
d4M+qZ9qL1Bnne2B7Nxb+XI=
=B1Ip
-----END PGP SIGNATURE-----