Re: [Monotone-devel] 0.43's revert is noisy: 'mtn: reverting mtn:execute on ...'

[Derek Scherger]

On Sun, Apr 5, 2009 at 10:10 PM, Derek Scherger <address@hidden> wrote:

On Wed, Apr 1, 2009 at 9:06 AM, Jack Lloyd <address@hidden> wrote:

I just noticed that in 0.43 the revert command has gotten quite noisy,
and reports reverting of mtn:execute and mtn:manual_merge metadata,
even if no actual reversion took place:

This is probably due to the changes I made to revert leading up to 0.43 to make it revert attributes more carefully. I haven't had a chance to look into it yet but I will in the next few days.

I finally had a minute to look into this and it is due to the changes I made to revert to do something more sensible with attributes. It's a trivial fix to change a P(F(... into an L(FL(... to silence this but I wonder if that's a bit too silent. i.e. do we never want to hear about attributes being changed?

Alternatively, what if we made the code that mucks with the execute bits print something when it actually makes a change? i.e. when called to set or clear the x bits only say something when they are actually being changed and otherwise be silent. This would affect revert and any other workspace modifying commands and might still be annoying but it might also be useful information.

Here's the change that I'm sitting on at the moment, comments welcome.

Cheers,
Derek

#
# old_revision [201865bdf90c29bd7320f4b177aedf69e18a0b6c]
#
# patch "cmd_ws_commit.cc"
#  from [1ccfaf790bb0726d0c797c230a6c85d67070d7ae]
#    to [b4c16e97c8c03598457fca84ccbae14bb0088468]
# 
# patch "unix/process.cc"
#  from [94aac0a3c86589223c8165020ddb0cae00633985]
#    to [a4fbbe13d86d9ef8427ed44416737385e3616968]
#
============================================================
--- cmd_ws_commit.cc    1ccfaf790bb0726d0c797c230a6c85d67070d7ae
+++ cmd_ws_commit.cc    b4c16e97c8c03598457fca84ccbae14bb0088468
@@ -314,7 +314,7 @@ CMD(revert, "revert", "", CMD_REF(worksp
       for (attr_map_t::const_iterator a = node->attrs.begin();
            a != node->attrs.end(); ++a)
         {
-          P(F("reverting %s on %s") % a->first() % path);
+          L(FL("reverting %s on %s") % a->first() % path);
           if (a->second.first)
             app.lua.hook_set_attribute(a->first(), path,
                                        a->second.second());
============================================================
--- unix/process.cc    94aac0a3c86589223c8165020ddb0cae00633985
+++ unix/process.cc    a4fbbe13d86d9ef8427ed44416737385e3616968
@@ -77,7 +77,7 @@ int change_xbits(const char *path, const
 
 int change_xbits(const char *path, const bool set)
 {
-  mode_t mode;
+  mode_t old_mode, new_mode;
   struct stat s;
   int fd = open(path, O_RDONLY);
   if (fd == -1)
@@ -88,21 +88,38 @@ int change_xbits(const char *path, const
     }
   if (fstat(fd, &s))
     return -1;
-  mode = s.st_mode;
+  old_mode = s.st_mode;
   if (set) {
-    mode |= ((S_IXUSR|S_IXGRP|S_IXOTH) & ~read_umask());
+    new_mode = old_mode | ((S_IXUSR|S_IXGRP|S_IXOTH) & ~read_umask());
   } else {
-    mode &= (~(S_IXUSR|S_IXGRP|S_IXOTH) & ~read_umask());
+    new_mode = old_mode & (~(S_IXUSR|S_IXGRP|S_IXOTH) & ~read_umask());
   }
-  L(FL("setting execute permission on path %s with mode %s") % path % mode);
-  int ret = fchmod(fd, mode);
+
+  int status = 0;
+  if (new_mode != old_mode)
+    {
+      if (set)
+        {
+          P(F("setting execute permission on %s") % path);
+          L(FL("setting execute permission on %s with mode %s") % path % new_mode);
+        }
+      else
+        {
+          P(F("clearing execute permission on %s") % path);
+          L(FL("clearing execute permission on %s with mode %s") % path % new_mode);
+        }
+
+      status = fchmod(fd, new_mode);
+    }
+
   if (close(fd) != 0)
     {
       const int err = errno;
       E(false, origin::system,
         F("error closing file %s: %s") % path % os_strerror(err));
     }
-  return ret;
+
+  return status;
 }
 
 int set_executable(const char *path)