bug#21699: 24.5; Bug in backup-buffer-copy and/or set-file-extended-attributes etc [set-file-extended-attributes]

[Eli Zaretskii]

> Date: Mon, 19 Oct 2015 05:47:36 -0400
> From: Eli Barzilay <eli@barzilay.org>
> Cc: 21699@debbugs.gnu.org
> 
> > I don't know if this is important, but it does change the semantics of
> > an interface that was already released.  So I preferred a fix that
> > didn't involve such changes.
> 
> OK, here's a version that does the decision in
> `set-file-extended-attributes', making it succeed if all of the given
> attributes were set but ignoring the "null" values.  (Again, I verified
> that it works in my case.)
> 
> -------------------------------------------------------------------------------
> (defun set-file-extended-attributes (filename attributes)
>   "Set extended attributes of file FILENAME to ATTRIBUTES.
> 
> ATTRIBUTES must be an alist of file attributes as returned by
> `file-extended-attributes'.  Value is t if the function succeeds
> in setting all of the given attributes excluding ones that
> indicate \"no information\"."
>   (let ((result t))
>     (dolist (elt attributes)
>       (let ((attr (car elt))
>             (val (cdr elt)))
>         (unless (cond ((eq attr 'acl)
>                        (or (equal val nil)
>                            (set-file-acl filename val)))
>                       ((eq attr 'selinux-context)
>                        (or (equal val '(nil nil nil nil))
>                            (set-file-selinux-context filename val))))
>           (setq result nil))))
>     result))
> -------------------------------------------------------------------------------

Thanks.  I'll let others to express opinions on this alternative vs
the one I committed.  The difference is what happens when all the
attribute values are "null" values: your version returns t in that
case, and I'm not sure that's correct, see below.

> >> With my fix, `file-extended-attributes' would just return nil in
> >> that case, and `set-file-extended-attributes' will succeed
> >> trivially.
> >
> > Why should set-file-extended-attributes succeed in this case?  It
> > didn't set any extended attributes, right?
> 
> Well, it did set all of the specified attributes, since there were none
> of them.  My new fix above will succeed in this case because it will
> ignore all of them.
> 
> 
> > And if neither ACLs nor SELinux is supported, we should definitely
> > fall back on chmod for the backup files, shouldn't we?
> 
> But chmod is not done on backup files other than copy the original bits
> to the backup.

Yes, I'm talking specifically about that scenario.  We should fall
back on chmod in that scenario, shouldn't we?  And if chmod fails, as
it did for you, shouldn't we tell the user about that?