Re: [Monotone-devel] Comments on mass_set?

[Nathaniel Smith]

On Fri, Aug 25, 2006 at 08:23:25PM -0400, Daniel Dickinson wrote:
> > > ============================================================
> > > --- 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 ...

Okay.  Do remember the big flashing comment, though :-).

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

Okay.  Maybe we need some better terminology here, to distinguish the
idea of the status of the file in the filesystem (e.g., the +x bit),
and the actual attr (e.g., the mtn:execute attr)?  'attr scan' makes
the latter match the former; update_any_attrs makes the former match
the latter...

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

Well, it seems like one of them should go away, anyway.  If we can't
think of a use case, that's also often a hint to simplify things...
:-).

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

In general, it's correct that filesystem attrs don't trigger
inodeprint changes (though for some attrs, they will -- e.g.,
permission changes are included in the inodeprint).

However, my question was more related to the fact that attr scan
doesn't _use_ update_current_roster_from_filesystem :-).  The new
argument you add here is not used by your patch.

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

It's possible that attr_init_functions doesn't... the exact semantics
for attrs got nailed down a lot when rosters were added, but we didn't
necessarily re-review things like the attr_init_functions API.

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

full_attr_map_t is probably a bit more complex than hooks want to deal
with.

My feeling is that hooks should just be given key (as a string), and
value (as a string, with 'nil' to represent 'unset').  This seems most
idiomatic in lua, and glosses over some details that are in
full_attr_map_t but that hooks don't care about (e.g., the distinction
between "this attr has never been set" and "this attr was set at some
point, but is no longer", which is needed for merging but otherwise
uninteresting).

-- Nathaniel

-- 
"But suppose I am not willing to claim that.  For in fact pianos
are heavy, and very few persons can carry a piano all by themselves."