Changes in update-game-score.c

Changes in update-game-score.c

[Eli Zaretskii]

Can we please stop futzing with this silly program, while in feature
freeze?  Why do we suddenly care about it so much?  Those changes do
nothing useful, and risk breaking builds, e.g.:

  ./update-game-score.c: In function `write_scores':
  ./update-game-score.c:446: warning: implicit declaration of function `fchmod'
  gcc  -std=gnu99 -mtune=pentium4      -I. -I../src -I../lib  -I. -I./../src -I./../lib   -mtune=pentium4   -DUSE_CRT_DLL=1 -I /d/gnu/bzr/emacs/trunk/nt/inc -O0 -gdwarf-2 -g3 -o test-distrib.exe ./test-distrib.c
  d:/usr/tmp/cckhdaaa.o(.text+0xe24): In function `write_scores':
  d:\gnu\bzr\emacs\trunk\lib-src/./update-game-score.c:446: undefined reference to `fchmod'
  collect2: ld returned 1 exit status

TIA

Re: Changes in update-game-score.c

[David Kastrup]

Eli Zaretskii <eliz@gnu.org> writes:

> Can we please stop futzing with this silly program, while in feature
> freeze?  Why do we suddenly care about it so much?  Those changes do
> nothing useful, and risk breaking builds, e.g.:
>
>   ./update-game-score.c: In function `write_scores':
>   ./update-game-score.c:446: warning: implicit declaration of function `fchmod'
>   gcc -std=gnu99 -mtune=pentium4 -I. -I../src -I../lib -I. -I./../src
> -I./../lib -mtune=pentium4 -DUSE_CRT_DLL=1 -I
> /d/gnu/bzr/emacs/trunk/nt/inc -O0 -gdwarf-2 -g3 -o test-distrib.exe
> ./test-distrib.c
>   d:/usr/tmp/cckhdaaa.o(.text+0xe24): In function `write_scores':
>   d:\gnu\bzr\emacs\trunk\lib-src/./update-game-score.c:446: undefined
> reference to `fchmod'
>   collect2: ld returned 1 exit status

You fixed the breakage for Windows NT, but fchmod is not generally
available on a _lot_ of platforms.  Without a proper autoconf test, the
whole change should be reverted.

-- 
David Kastrup

Re: Changes in update-game-score.c

[Eli Zaretskii]

> From: David Kastrup <dak@gnu.org>
> Date: Wed, 22 Jan 2014 20:50:19 +0100
> 
> You fixed the breakage for Windows NT, but fchmod is not generally
> available on a _lot_ of platforms.  Without a proper autoconf test, the
> whole change should be reverted.

Go for it.  This program is broken in so many ways that I don't even
understand why we care to have it.

Re: Changes in update-game-score.c

[David Kastrup]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: David Kastrup <dak@gnu.org>
>> Date: Wed, 22 Jan 2014 20:50:19 +0100
>> 
>> You fixed the breakage for Windows NT, but fchmod is not generally
>> available on a _lot_ of platforms.  Without a proper autoconf test, the
>> whole change should be reverted.
>
> Go for it.

That would have to be done by an Emacs developer.

-- 
David Kastrup

Re: Changes in update-game-score.c

[Glenn Morris]

>> fchmod is not generally available on a _lot_ of platforms. Without a
>> proper autoconf test, the whole change should be reverted.

Fcopy_file seems to have managed ok for the past 8 years.

Deja vu:
http://lists.gnu.org/archive/html/emacs-devel/2005-06/msg01476.html

Re: Changes in update-game-score.c

[Paul Eggert]

Eli Zaretskii wrote:
> Why do we suddenly care about it so much?

We got a bug report about it, which I fixed, and I noticed some other 
bugs in the neighborhood, including undefined behavior and race 
conditions which are dicey in a setuid program (it's bad PR at the very 
least). A feature freeze shouldn't prevent us from fixing such bugs.

The main culprit here was the w32 port's definition of fchmod as a noop 
for Emacs, and its failure to define fchmod for non-Emacs executables. 
We can't expect non-w32 experts to anticipate that sort of tricky 
situation flawlessly every time.

Re: Changes in update-game-score.c

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 578 bytes --]

Glenn Morris wrote:
> Fcopy_file seems to have managed ok for the past 8 years.

Although all mainstream POSIXish platforms have fchmod these days, one 
oldish platform (BeOS) lacks it. It's easy enough to port to such 
platforms, so we might as well.

This problem isn't limited to update-game-score. I found three other 
instances in Emacs. The first was in qcopy_acl (called by Fcopy_file) 
and I fixed it in trunk bzr 116124. The other two are in fileio.c and 
filelock.c. The attached patch should fix it; I plan to install it 
unless someone points out a problem with it.

[-- Attachment #2: fnchmod.diff --]
[-- Type: text/x-patch, Size: 5905 bytes --]

=== modified file 'ChangeLog'
--- ChangeLog	2014-01-23 02:48:44 +0000
+++ ChangeLog	2014-01-23 03:11:08 +0000
@@ -1,5 +1,10 @@
 2014-01-23  Paul Eggert  <eggert@cs.ucla.edu>
 
+	Port better to platforms lacking fchmod.
+	See the thread starting at:
+	http://lists.gnu.org/archive/html/emacs-devel/2014-01/msg01899.html
+	* lib/fnchmod.h: New file.
+
 	Merge from gnulib, incorporating:
 	2014-01-22 qacl: check for fchmod
 	* m4/acl.m4: Update from gnulib.

=== modified file 'lib-src/ChangeLog'
--- lib-src/ChangeLog	2014-01-22 19:38:31 +0000
+++ lib-src/ChangeLog	2014-01-23 03:11:08 +0000
@@ -1,3 +1,13 @@
+2014-01-23  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Port better to platforms lacking fchmod.
+	* Makefile.in (update-game-score${EXEEXT}):
+	Depend on ${srcdir}/../lib/fnchmod.h.
+	* update-game-score.c (INLINE): New macro.
+	Include <fnchmod.h>.
+	(write_scores): Use fnchmod rather than chmod on WINDOWSNT and
+	fchmod elsewhere.
+
 2014-01-22  Eli Zaretskii  <eliz@gnu.org>
 
 	* update-game-score.c (write_scores) [WINDOWSNT]: Use chmod

=== modified file 'lib-src/Makefile.in'
--- lib-src/Makefile.in	2014-01-05 02:56:08 +0000
+++ lib-src/Makefile.in	2014-01-23 03:11:08 +0000
@@ -372,7 +372,8 @@
 hexl${EXEEXT}: ${srcdir}/hexl.c $(NTLIB) $(config_h)
 	$(CC) ${ALL_CFLAGS} ${srcdir}/hexl.c $(LOADLIBES) -o hexl${EXEEXT}
 
-update-game-score${EXEEXT}: ${srcdir}/update-game-score.c $(NTLIB) $(config_h)
+update-game-score${EXEEXT}: ${srcdir}/update-game-score.c \
+		${srcdir}/../lib/fnchmod.h $(NTLIB) $(config_h)
 	$(CC) ${ALL_CFLAGS} -DHAVE_SHARED_GAME_DIR="\"$(gamedir)\"" \
 	  ${srcdir}/update-game-score.c $(LOADLIBES) $(NTLIB) \
 	  -o update-game-score${EXEEXT}

=== modified file 'lib-src/update-game-score.c'
--- lib-src/update-game-score.c	2014-01-22 19:38:31 +0000
+++ lib-src/update-game-score.c	2014-01-23 03:11:08 +0000
@@ -31,6 +31,7 @@
    Created 2002/03/22.
 */
 
+#define INLINE EXTERN_INLINE
 #include <config.h>
 
 #include <unistd.h>
@@ -48,6 +49,8 @@
 #include <sys/stat.h>
 #include <getopt.h>
 
+#include <fnchmod.h>
+
 #ifdef WINDOWSNT
 #include "ntlib.h"
 #endif
@@ -443,10 +446,8 @@
   fd = mkostemp (tempfile, 0);
   if (fd < 0)
     return -1;
-#ifndef WINDOWSNT
-  if (fchmod (fd, 0644) != 0)
+  if (fnchmod (fd, filename, 0644) != 0)
     return -1;
-#endif
   f = fdopen (fd, "w");
   if (! f)
     return -1;
@@ -459,10 +460,6 @@
     return -1;
   if (rename (tempfile, filename) != 0)
     return -1;
-#ifdef WINDOWSNT
-  if (chmod (filename, 0644) < 0)
-    return -1;
-#endif
   return 0;
 }
 

=== added file 'lib/fnchmod.h'
--- lib/fnchmod.h	1970-01-01 00:00:00 +0000
+++ lib/fnchmod.h	2014-01-23 03:11:08 +0000
@@ -0,0 +1,45 @@
+/* Set file mode based on a file descriptor or name.
+
+   Copyright 2014 Free Software Foundation, Inc.
+
+   This program is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef FNCHMOD_H
+#define FNCHMOD_H
+
+#include <config.h>
+
+#include <sys/stat.h>
+
+INLINE_HEADER_BEGIN
+
+/* On systems that have fchmod, use it to change the mode of FD.
+   On other systems, use chmod on NAME instead; NAME should identify
+   the same file that FD does.  Typically fchmod is preferable, as it
+   can avoid some races.  MODE is the desired mode.  Return 0 if
+   successful.  Return -1 and set errno upon failure.  */
+
+INLINE int
+fnchmod (int fd, char const *name, mode_t mode)
+{
+#if HAVE_FCHMOD
+  return fchmod (fd, mode);
+#else
+  return chmod (name, mode);
+#endif
+}
+
+INLINE_HEADER_END
+
+#endif

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2014-01-22 10:29:23 +0000
+++ src/ChangeLog	2014-01-23 03:11:08 +0000
@@ -1,3 +1,10 @@
+2014-01-23  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Port better to platforms lacking fchmod.
+	* fileio.c, filelock.c: Include <fnchmod.h>.
+	* fileio.c (Fcopy_file):
+	* filelock.c (create_lock_file): Use fnchmod, not fchmod.
+
 2014-01-22  Martin Rudalics  <rudalics@gmx.at>
 
 	Fixes in window size functions around Bug#16430 and Bug#16470.

=== modified file 'src/fileio.c'
--- src/fileio.c	2014-01-03 06:47:27 +0000
+++ src/fileio.c	2014-01-23 03:11:08 +0000
@@ -41,6 +41,7 @@
 #endif
 
 #include <c-ctype.h>
+#include <fnchmod.h>
 
 #include "lisp.h"
 #include "intervals.h"
@@ -2113,7 +2114,7 @@
 	    : (already_exists
 	       || (new_mask & ~realmask) == default_permissions)
 	    ? 0
-	    : fchmod (ofd, default_permissions))
+	    : fnchmod (ofd, SSDATA (encoded_newname), default_permissions))
       {
       case -2: report_file_error ("Copying permissions from", file);
       case -1: report_file_error ("Copying permissions to", newname);

=== modified file 'src/filelock.c'
--- src/filelock.c	2014-01-01 07:43:34 +0000
+++ src/filelock.c	2014-01-23 03:11:08 +0000
@@ -39,6 +39,7 @@
 #include <errno.h>
 
 #include <c-ctype.h>
+#include <fnchmod.h>
 
 #include "lisp.h"
 #include "character.h"
@@ -424,7 +425,7 @@
 	  /* Use 'write', not 'emacs_write', as garbage collection
 	     might signal an error, which would leak FD.  */
 	  if (write (fd, lock_info_str, lock_info_len) != lock_info_len
-	      || fchmod (fd, S_IRUSR | S_IRGRP | S_IROTH) != 0)
+	      || fnchmod (fd, nonce, S_IRUSR | S_IRGRP | S_IROTH) != 0)
 	    err = errno;
 	  /* There is no need to call fsync here, as the contents of
 	     the lock file need not survive system crashes.  */

Re: Changes in update-game-score.c

[Eli Zaretskii]

> Date: Wed, 22 Jan 2014 19:32:01 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: emacs-devel@gnu.org
> 
> Eli Zaretskii wrote:
> > Why do we suddenly care about it so much?
> 
> We got a bug report about it, which I fixed, and I noticed some other 
> bugs in the neighborhood, including undefined behavior and race 
> conditions which are dicey in a setuid program (it's bad PR at the very 
> least). A feature freeze shouldn't prevent us from fixing such bugs.

The bugfix should have fixed the bug that was reported, and that's it.
Even the first bugfix was already too much (you said it yourself:
fancy).  The rest is just superfluous and unneeded, as the program is
not important enough for us to waste any time on it.  (Why isn't it
implemented in Lisp, anyway?)

> The main culprit here was the w32 port's definition of fchmod as a noop 
> for Emacs, and its failure to define fchmod for non-Emacs executables. 
> We can't expect non-w32 experts to anticipate that sort of tricky 
> situation flawlessly every time.

No one requested you to be the expert, you are missing the point.

You never explained why the switch from chmod to fchmod was needed
anyway.  Do we really care about race conditions here?  What, the same
user will be updating her score simultaneously from 2 sessions?  Come
on!

Feature freeze means restraint: do not try to "fix" that which has
been happily "broken" for ages.

Re: Changes in update-game-score.c

[Eli Zaretskii]

> Date: Wed, 22 Jan 2014 19:33:58 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> This problem isn't limited to update-game-score. I found three other 
> instances in Emacs. The first was in qcopy_acl (called by Fcopy_file) 
> and I fixed it in trunk bzr 116124. The other two are in fileio.c and 
> filelock.c. The attached patch should fix it; I plan to install it 
> unless someone points out a problem with it.

Please don't install this, we've been living with this "problem" long
enough to leave it for afterwards.

Re: Changes in update-game-score.c

[Glenn Morris]

Paul Eggert wrote:

> Although all mainstream POSIXish platforms have fchmod these days, one
> oldish platform (BeOS) lacks it. It's easy enough to port to such
> platforms, so we might as well.

?
But obviously this is irrelevant, since precisely no-one has ever
complained in the past ~ decade.

No-one apart from you has mentioned BeOS on emacs-devel, bug-gnu-emacs,
or help-gnu-emacs in 12+ years (apart from precisely one tangential
mention).

Re: Changes in update-game-score.c

[Glenn Morris]

Eli Zaretskii wrote:

> (Why isn't it implemented in Lisp, anyway?)

The only reason this program exists (AFAIK) is so it can be setuid, so
that there can be a shared game score file owned by a "games" user.

Re: Changes in update-game-score.c

[Glenn Morris]

Glenn Morris wrote:

> The only reason this program exists (AFAIK) is so it can be setuid, so
> that there can be a shared game score file owned by a "games" user.

BTW, in Debian testing and RHEL6 the default rpm/dpkg installed
update-game-score isn't setuid, so AFAICS this makes it pointless on
those platforms.

(Although on RHEL6 things games like same-gnome, mahjongg are setgid
games and have global score files.)

Re: Changes in update-game-score.c

[Glenn Morris]

Glenn Morris wrote:

> BTW, in Debian testing and RHEL6 the default rpm/dpkg installed
> update-game-score isn't setuid, so AFAICS this makes it pointless on
> those platforms.
>
> (Although on RHEL6 things games like same-gnome, mahjongg are setgid
> games and have global score files.)

And in Debian testing, things like gnome-robots are setgid games and
have global score files. Seems like a double standard...

Re: Changes in update-game-score.c

[Paul Eggert]

Eli Zaretskii wrote:
> The bugfix should have fixed the bug that was reported, and that's it.

I could have filed a half-dozen bug reports, one for each bug I fixed, 
and then installed patches one by one, marking each bug fixed as I did 
so. But that would have been overkill in this case; it would have been 
too much work for everybody (particularly for me :-) for too little benefit.

The bugs included undefined behavior the very nature of which could be 
changed by the original fix for the reported bug, and we are talking 
setuid here, so here was sufficient justification for fixing them. Now 
that they're fixed we can move on.

Re: Changes in update-game-score.c

[Eli Zaretskii]

> Date: Thu, 23 Jan 2014 05:53:47 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> > Date: Wed, 22 Jan 2014 19:33:58 -0800
> > From: Paul Eggert <eggert@cs.ucla.edu>
> > 
> > This problem isn't limited to update-game-score. I found three other 
> > instances in Emacs. The first was in qcopy_acl (called by Fcopy_file) 
> > and I fixed it in trunk bzr 116124. The other two are in fileio.c and 
> > filelock.c. The attached patch should fix it; I plan to install it 
> > unless someone points out a problem with it.
> 
> Please don't install this, we've been living with this "problem" long
> enough to leave it for afterwards.

And on top of that, it would break the Windows build, because gnulib
functions called by Emacs can no longer call libc functions that
accept file names, since we now pass UTF-8 encoded file names to them.
So where formerly fchmod was a no-op that always succeeded, fnchmod
will now be a no-op that always fails, and filelock.c will stop
working.

Re: Changes in update-game-score.c

[Eli Zaretskii]

> Date: Wed, 22 Jan 2014 20:56:13 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: emacs-devel@gnu.org
> 
> Eli Zaretskii wrote:
> > The bugfix should have fixed the bug that was reported, and that's it.
> 
> I could have filed a half-dozen bug reports, one for each bug I fixed, 
> and then installed patches one by one, marking each bug fixed as I did 
> so.

That is exactly what we all should do: each commit is one coherent
changeset, solving a problem that is independent of others.  I'm sure
I'm not saying anything you didn't know.

> But that would have been overkill in this case; it would have been 
> too much work for everybody (particularly for me :-) for too little benefit.

Sorry, I disagree.

> The bugs included undefined behavior the very nature of which could be 
> changed by the original fix for the reported bug, and we are talking 
> setuid here, so here was sufficient justification for fixing them. Now 
> that they're fixed we can move on.

Sorry, cannot move on, not yet.  Please at least document the reasons
for each part of that commit, as some of them remain unexplained in
the logs.  And since they are anything but self-explanatory, I don't
see how anyone else could do that, or guess.

Thanks.

Re: Changes in update-game-score.c

[Paul Eggert]

On 01/23/2014 08:07 AM, Eli Zaretskii wrote:
> Please at least document the reasons
> for each part of that commit

That was already done in its ChangeLog entry. In trunk bzr 116131 I 
added some text to that entry, to give more detail.

Re: Changes in update-game-score.c

[Juanma Barranquero]

On Thu, Jan 23, 2014 at 5:07 PM, Eli Zaretskii <eliz@gnu.org> wrote:

> That is exactly what we all should do: each commit is one coherent
> changeset, solving a problem that is independent of others.

Very emphatically agree. We should put that in writing in our coding
guidelines. That and making sure that the first line of a commit log
is its summary.

    J

Re: Changes in update-game-score.c

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 1096 bytes --]

On 01/23/2014 12:27 PM, Juanma Barranquero wrote:
> On Thu, Jan 23, 2014 at 5:07 PM, Eli Zaretskii<eliz@gnu.org>  wrote:
>
>> >That is exactly what we all should do: each commit is one coherent
>> >changeset, solving a problem that is independent of others.
> Very emphatically agree.

It's easy to nod one's head and say "yes, that's right! and whoever 
commits independent changes should be taken out and shot!". But in 
practice that's far too extreme. For example, if we look at a recent 
simple patch (attached), it's actually two changes, one change for each 
variable that was declared on WINDOWSNT platforms but not used there.

Now, suppose someone complained "Hey! Wait a minute! You should have 
broken that into two independent fixes and installed each fix 
separately!" I hope our response would be something like "Sure, we could 
have installed two separate patches. But that would have been overkill 
here; it would have been too much work for everybody for too little 
benefit." The situation for update-game-score was similar, and this is 
true of many changes we make to Emacs.

[-- Attachment #2: windowsnt.diff --]
[-- Type: text/x-patch, Size: 999 bytes --]

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2013-12-30 17:51:28 +0000
+++ src/ChangeLog	2013-12-30 22:36:44 +0000
@@ -1,3 +1,8 @@
+2013-12-30  Juanma Barranquero  <lekktu@gmail.com>
+
+	* fileio.c (Fcopy_file) [!WINDOWSNT]: Don't declare on Windows
+	variables not used there.
+
 2013-12-30  Eli Zaretskii  <eliz@gnu.org>
 
 	* w32.c (sys_umask): New function.  (Bug#16299)

=== modified file 'src/fileio.c'
--- src/fileio.c	2013-12-29 18:18:45 +0000
+++ src/fileio.c	2013-12-30 22:36:44 +0000
@@ -1943,15 +1943,15 @@
   struct gcpro gcpro1, gcpro2, gcpro3, gcpro4;
   ptrdiff_t count = SPECPDL_INDEX ();
   Lisp_Object encoded_file, encoded_newname;
+#if HAVE_LIBSELINUX
+  security_context_t con;
+  int conlength = 0;
+#endif
+#ifdef WINDOWSNT
+  int result;
+#else
   bool already_exists = false;
   mode_t new_mask;
-#if HAVE_LIBSELINUX
-  security_context_t con;
-  int conlength = 0;
-#endif
-#ifdef WINDOWSNT
-  int result;
-#else
   int ifd, ofd;
   int n;
   char buf[16 * 1024];

Re: Changes in update-game-score.c

[Juanma Barranquero]

On Thu, Jan 23, 2014 at 10:50 PM, Paul Eggert <eggert@cs.ucla.edu> wrote:

> It's easy to nod one's head and say "yes, that's right! and whoever commits
> independent changes should be taken out and shot!".

Yes, that is exactly what I was proposing. Good catch.

     J

Re: Changes in update-game-score.c

[Eli Zaretskii]

> Date: Thu, 23 Jan 2014 13:50:56 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> Cc: Emacs developers <emacs-devel@gnu.org>
> 
> For example, if we look at a recent simple patch (attached), it's
> actually two changes, one change for each variable that was declared
> on WINDOWSNT platforms but not used there.

Alternatively, one could say in the log something like "Avoid compiler
warnings due to unused variables."

In any case, the comparison is invalid because this change wasn't
fixing any bug, it's more like whitespace cleanup.

> The situation for update-game-score was similar, and this is true of
> many changes we make to Emacs.

True or false, sumo changesets make maintenance harder, e.g., because
they cannot be reverted easily.

Re: Changes in update-game-score.c

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 897 bytes --]

Eli Zaretskii wrote:
> sumo changesets make maintenance harder

Sure, but the change that prompted this thread (trunk bzr 116113) 
deleted 13 lines of code and added 8. That's a small changeset, not a 
sumo one.

> the comparison is invalid because this [other] change wasn't fixing any bug, it's more like whitespace cleanup.

It's easy to find bug-fixing changes that behave the same way. For 
example, the most recent bug-fixing patch that you installed (attached) 
actually consists of multiple independent changes. One change replaces 
fchmod with chmod on WINDOWSNT platforms, fixing a porting bug; but 
another change replaces "!=" with "<" on WINDOWSNT, and this change does 
not fix any bug.

This sort of thing happens all the time, and it's OK. It'd be 
unreasonable to insist that every patch to Emacs now must fix just one 
bug and must not make any changes that do not fix the bug.


[-- Attachment #2: w32.diff --]
[-- Type: text/x-patch, Size: 939 bytes --]

=== modified file 'lib-src/ChangeLog'
--- lib-src/ChangeLog	2014-01-22 19:02:41 +0000
+++ lib-src/ChangeLog	2014-01-22 19:38:31 +0000
@@ -1,3 +1,8 @@
+2014-01-22  Eli Zaretskii  <eliz@gnu.org>
+
+	* update-game-score.c (write_scores) [WINDOWSNT]: Use chmod
+	instead of fchmod.
+
 2014-01-22  Paul Eggert  <eggert@cs.ucla.edu>
 
 	Fix miscellaneous update-game-score bugs.

=== modified file 'lib-src/update-game-score.c'
--- lib-src/update-game-score.c	2014-01-22 19:02:41 +0000
+++ lib-src/update-game-score.c	2014-01-22 19:38:31 +0000
@@ -443,8 +443,10 @@
   fd = mkostemp (tempfile, 0);
   if (fd < 0)
     return -1;
+#ifndef WINDOWSNT
   if (fchmod (fd, 0644) != 0)
     return -1;
+#endif
   f = fdopen (fd, "w");
   if (! f)
     return -1;
@@ -457,6 +459,10 @@
     return -1;
   if (rename (tempfile, filename) != 0)
     return -1;
+#ifdef WINDOWSNT
+  if (chmod (filename, 0644) < 0)
+    return -1;
+#endif
   return 0;
 }
 

Re: Changes in update-game-score.c

[Eli Zaretskii]

> Date: Fri, 24 Jan 2014 00:42:35 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: emacs-devel@gnu.org
> 
> Eli Zaretskii wrote:
> > sumo changesets make maintenance harder
> 
> Sure, but the change that prompted this thread (trunk bzr 116113) 
> deleted 13 lines of code and added 8. That's a small changeset, not a 
> sumo one.

"Sumo" does not necessarily mean a lot of lines.  If there are several
unrelated issues taken care, it's a "sumo" change, for the purposes of
this discussion.  It means that reverting the commit gets rid of more
than just one issue.

> It's easy to find bug-fixing changes that behave the same way. For 
> example, the most recent bug-fixing patch that you installed (attached) 
> actually consists of multiple independent changes. One change replaces 
> fchmod with chmod on WINDOWSNT platforms, fixing a porting bug; but 
> another change replaces "!=" with "<" on WINDOWSNT, and this change does 
> not fix any bug.

No, you are mistaken: chmod is documented to be able to return both
negative and positive values, in addition to zero.  We care only about
the negative case.  By contrast, fchmod is documented to return only
zero or -1.  These two functions are different in this respect, so the
code cannot stay the same when one is replaced by the other.

Seen from another POV, I simply restored the previous code for
WINDOWSNT, i.e. the bug I fixed was that the code was changed in the
first place.

> This sort of thing happens all the time, and it's OK.

That it happens all the time doesn't yet make it OK.

> It'd be unreasonable to insist that every patch to Emacs now must
> fix just one bug and must not make any changes that do not fix the
> bug.

The only unrelated changes that can be done are whitespace changes,
comment changes, formatting changes, and other similar stuff.  It
might be hard to reach that ideal, but we should strive.

Re: Changes in update-game-score.c

[Jarek Czekalski]

W dniu 2014-01-24 10:17, Eli Zaretskii pisze:
> [...] it's a "sumo" change, for the purposes of
> this discussion.  [...]

If at least admins would not go off topic in mailing list threads, other 
people could also learn that. Is this still regarding update-game-score? 
It should be safe to skip this thread if one is not interested in game 
scores.

At some point someone should start a new thread with a good subject 
line. In the point where discussion starts to be general or is related 
to something very different (fchmod?). A new thread, not an answer to 
the existing one, so that archivers could notice it as a new thread.

These rules should be obeyed in all mailing lists, but in something as 
big as this, it should be obeyed strictly. I have ideas how to force 
this, but that's for a separate thread.

Jarek

Re: Changes in update-game-score.c

[Eli Zaretskii]

> Date: Fri, 24 Jan 2014 16:29:38 +0100
> From: Jarek Czekalski <jarekczek@poczta.onet.pl>
> 
> 
> W dniu 2014-01-24 10:17, Eli Zaretskii pisze:
> > [...] it's a "sumo" change, for the purposes of
> > this discussion.  [...]
> 
> If at least admins would not go off topic in mailing list threads, other 
> people could also learn that. Is this still regarding update-game-score? 

Yes.

> It should be safe to skip this thread if one is not interested in game 
> scores.

It is up to each one of us which threads to read and which to skip.
(I read all of them, FWIW.)

change the Subject line when you change topics [was: Changes in update-game-score.c]

[Drew Adams]

> If at least admins would not go off topic in mailing list threads, other
> people could also learn that. Is this still regarding update-game-score?
> It should be safe to skip this thread if one is not interested in game
> scores.
> 
> At some point someone should start a new thread with a good subject
                               ^^^^^^^^^^^^^^^^^^
> line. In the point where discussion starts to be general or is related
> to something very different (fchmod?). A new thread, not an answer to
> the existing one, so that archivers could notice it as a new thread.
> 
> These rules should be obeyed in all mailing lists, but in something as
> big as this, it should be obeyed strictly. I have ideas how to force
> this, but that's for a separate thread.

+1

Patches with independent changes

[Paul Eggert]

[retitling from "Changes in update-game-score.c"]

Eli Zaretskii wrote:

> chmod is documented to be able to return both negative and positive values

Really?  Where is it documented to do that?  It's not documented that 
way in the POSIX documentation for chmod, or in the Gnulib documentation 
of chmod porting hassles; see 
<http://pubs.opengroup.org/onlinepubs/9699919799/functions/chmod.html> 
and <http://www.gnu.org/software/gnulib/manual/html_node/chmod.html>, 
respectively.

Even if you're correct, which I doubt, trunk bzr 116116 would still be 
an example of what you're calling a "sumo" patch -- a patch containing 
multiple independent changes. One change addresses WINDOWSNT's lack of 
fchmod. Another addresses the perhaps-hypothetical issue that chmod can 
return positive values. So regardless, that patch does not follow the 
rule that patches should fix just one bug and should not make 
independent changes.

Insisting on such a rule would be silly, anyway. We don't follow that 
rule and we never have, and we shouldn't impose it now. Obviously there 
are advantages to putting independent changes in separate patches and in 
many cases we should do that, but an extremist insistence of this 
principle in all cases would significantly increase our maintenance 
burden for little or no benefit.

Re: Patches with independent changes

[Eli Zaretskii]

> Date: Fri, 24 Jan 2014 08:43:19 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: emacs-devel@gnu.org
> 
> [retitling from "Changes in update-game-score.c"]

[Why?]

> Eli Zaretskii wrote:
> 
> > chmod is documented to be able to return both negative and positive values
> 
> Really?  Where is it documented to do that?

I could have sworn that I saw it on MSDN, but I cannot find it now, so
apologies about that part.

> Even if you're correct, which I doubt, trunk bzr 116116 would still be 
> an example of what you're calling a "sumo" patch -- a patch containing 
> multiple independent changes. One change addresses WINDOWSNT's lack of 
> fchmod. Another addresses the perhaps-hypothetical issue that chmod can 
> return positive values.

No, they both address the fact that fchmod is not available on
Windows.  One without the other would break the build on Windows.
That I used < rather than != is immaterial, because both are correct
in this case.

Re: Patches with independent changes

[Paul Eggert]

On 01/24/2014 01:39 PM, Eli Zaretskii wrote:

> they both address the fact that fchmod is not available on Windows. 
> One without the other would break the build on Windows.

I wasn't referring to two hunks in the patch. I was referring to two of 
the changes installed by that patch. One change worked around 
WINDOWSNT's lack of fchmod, and the other replaced "!=" with "<".The 
changes were independent and only the first change was needed to fix the 
bug.

> That I used < rather than != is immaterial

You thought replacing "!=" with "<" was needed, so the intent of that 
patch was to install multiple independent fixes. Which was fine.

Here's another example. Trunk bzr 116064 fixes a file name handling 
failure on MS-Windows 9X. While it's in the neighborhood, it makes the 
independent changes of removing initialization code for the variable 
g_b_init_is_w9x, and of removing that variable's definition. These 
independent changes weren't needed to fix the bug.

This sort of thing is quite common, and there's nothing wrong with it.
>> [retitling from "Changes in update-game-score.c"]
> [Why?]

Others requested retitling and this seemed reasonable.

Re: Patches with independent changes

[Eli Zaretskii]

> Date: Fri, 24 Jan 2014 14:34:35 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: emacs-devel@gnu.org
> 
> On 01/24/2014 01:39 PM, Eli Zaretskii wrote:
> 
> > they both address the fact that fchmod is not available on Windows. 
> > One without the other would break the build on Windows.
> 
> I wasn't referring to two hunks in the patch. I was referring to two of 
> the changes installed by that patch. One change worked around 
> WINDOWSNT's lack of fchmod, and the other replaced "!=" with "<".The 
> changes were independent and only the first change was needed to fix the 
> bug.

There was no second change.  There were 2 hunks in a single change
that fixed a single problem:

=== modified file 'lib-src/update-game-score.c'
--- lib-src/update-game-score.c	2014-01-22 19:02:41 +0000
+++ lib-src/update-game-score.c	2014-01-22 19:38:31 +0000
@@ -443,8 +443,10 @@ write_scores (const char *filename, cons
   fd = mkostemp (tempfile, 0);
   if (fd < 0)
     return -1;
+#ifndef WINDOWSNT
   if (fchmod (fd, 0644) != 0)
     return -1;
+#endif
   f = fdopen (fd, "w");
   if (! f)
     return -1;
@@ -457,6 +459,10 @@ write_scores (const char *filename, cons
     return -1;
   if (rename (tempfile, filename) != 0)
     return -1;
+#ifdef WINDOWSNT
+  if (chmod (filename, 0644) < 0)
+    return -1;
+#endif
   return 0;
 }
 
I didn't change anything in the call to fchmod, except conditioning it
on non-Windows build.  The code that calls chmod instead was added,
and it used < from the get-go.  So there's no second change, the two
hunks were strictly required to make the code compile on Windows.

> > That I used < rather than != is immaterial
> 
> You thought replacing "!=" with "<" was needed, so the intent of that 
> patch was to install multiple independent fixes. Which was fine.

I didn't replace anything, the line with chmod was _added_ in its
entirety.

> Here's another example. Trunk bzr 116064 fixes a file name handling 
> failure on MS-Windows 9X. While it's in the neighborhood, it makes the 
> independent changes of removing initialization code for the variable 
> g_b_init_is_w9x, and of removing that variable's definition. These 
> independent changes weren't needed to fix the bug.

Yes, they were needed: that variable was no longer used after the
change.  Removal of unused code is the same as changing commentary or
whitespace: it is generally done when the related code changes.

> This sort of thing is quite common, and there's nothing wrong with it.

Indeed, there's nothing wrong with the changes you are trying to
represent as examples.  Your changes were different in kind, and that
is what this discussion is about.

> >> [retitling from "Changes in update-game-score.c"]
> > [Why?]
> 
> Others requested retitling and this seemed reasonable.

Others were wrong, there's no reason to retitle.  The discussion is
still about changes in update-game-score.

Re: change the Subject line when you change topics [was: Changes in update-game-score.c]

[Jarek Czekalski]

Drew,

This new subject is not a new thread, but continuation of game score:

http://lists.gnu.org/archive/html/emacs-devel/2014-01/threads.html#01899

That's what should be avoided: rewriting the subject. In Thunderbird you 
start a new thread by pressing "New message" or right-clicking on email 
address and "Send message". Quotations must be manuallly copied using 
for example "Paste as quotation".

Jarek

Re: Patches with independent changes

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 1316 bytes --]

Eli Zaretskii wrote:
> There was no second change.

Sure there was.  It would have been easy to apply a simpler patch that 
contained just the single change needed to fix the porting bug, namely 
to call chmod rather than fchmod (attached).  Instead, you applied a 
more-complicated patch that contained multiple independent changes.

Moving on to trunk bzr 116064:

>> These independent changes weren't needed to fix the bug.
>
> Yes, they were needed

Obviously they were not needed to fix the bug, as the bug would have 
been fixed without them.  They were "needed" only in the sense that it's 
nicer for maintainers and users if Emacs is simpler and smaller.  It was 
reasonable to apply those changes while you were in the neighborhood, 
but they could have been applied as separate and independent patches 
(not that I recommend this -- I think the patch was fine as-is).

> Your changes were different in kind

I don't see why.  We all install patches containing multiple independent 
changes, only some of which are needed to fix a bug.  And that's OK. 
The important thing is that changes in a patch should all be related, so 
that it makes sense to install them together.  It's not always essential 
or even advisable to separate changes into different patches merely 
because the changes are independent.

[-- Attachment #2: update.diff --]
[-- Type: text/x-patch, Size: 393 bytes --]

--- update-game-score.c-old	2014-01-25 08:31:13.065305429 -0800
+++ update-game-score.c	2014-01-25 08:31:54.337506871 -0800
@@ -443,8 +443,13 @@
   fd = mkostemp (tempfile, 0);
   if (fd < 0)
     return -1;
+#ifdef WINDOWSNT
+  if (chmod (tempfile, 0644) != 0)
+    return -1;
+#else
   if (fchmod (fd, 0644) != 0)
     return -1;
+#endif
   f = fdopen (fd, "w");
   if (! f)
     return -1;

Re: Patches with independent changes

[Eli Zaretskii]

> Date: Sat, 25 Jan 2014 09:01:21 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: emacs-devel@gnu.org
> 
> Eli Zaretskii wrote:
> > There was no second change.
> 
> Sure there was.  It would have been easy to apply a simpler patch that 
> contained just the single change needed to fix the porting bug, namely 
> to call chmod rather than fchmod (attached).  Instead, you applied a 
> more-complicated patch that contained multiple independent changes.

No, I applied a _single_ change, which in effect simply restored the
previous code, but only for WINDOWSNT.  The new fchmod code had no
influence on what I added, and wasn't touched.

> Moving on to trunk bzr 116064:
> 
> >> These independent changes weren't needed to fix the bug.
> >
> > Yes, they were needed
> 
> Obviously they were not needed to fix the bug, as the bug would have 
> been fixed without them.  They were "needed" only in the sense that it's 
> nicer for maintainers and users if Emacs is simpler and smaller.

No, it's not "nicer", it's necessary, because otherwise the build
would have failed when compiler warnings are turned on and used as
errors.

> > Your changes were different in kind
> 
> I don't see why.

I doubt that.

> We all install patches containing multiple independent changes, only
> some of which are needed to fix a bug.  And that's OK.  The
> important thing is that changes in a patch should all be related, so
> that it makes sense to install them together.

Then any number of patches can be installed in a single commit,
because they are all "related" -- after all, they are all about Emacs.

Re: Patches with independent changes

[Eli Zaretskii]

> Date: Sat, 25 Jan 2014 09:01:21 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> Cc: emacs-devel@gnu.org
> 
> --- update-game-score.c-old	2014-01-25 08:31:13.065305429 -0800
> +++ update-game-score.c	2014-01-25 08:31:54.337506871 -0800
> @@ -443,8 +443,13 @@
>    fd = mkostemp (tempfile, 0);
>    if (fd < 0)
>      return -1;
> +#ifdef WINDOWSNT
> +  if (chmod (tempfile, 0644) != 0)
> +    return -1;
> +#else
>    if (fchmod (fd, 0644) != 0)
>      return -1;
> +#endif
>    f = fdopen (fd, "w");
>    if (! f)
>      return -1;

This code is plain wrong: you cannot chmod a file that is open, if you
try, the chmod call will fail.

Again, I just restored the old code, but only for WINDOWSNT.  The
reason for that was that I wanted a safe change, since we are in a
feature freeze.  In any case, the fact that the problem could have
been solved in more than one way doesn't mean there are multiple
changes involved.

Re: Patches with independent changes

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 1569 bytes --]

Eli Zaretskii wrote:
> otherwise the build would have failed when compiler warnings are
 > turned on and used as errors.

That would have been odd, as other static variables in Emacs are 
declared and set but never used (e.g., category_table_version) and 
evidently these builds are not failing.  It's certainly OK to clean up 
the glitches, but such cleanups are independent changes.

> Then any number of patches can be installed in a single commit,
> because they are all "related" -- after all, they are all about Emacs.

No, that's too broad.  Patches should contain changes that are more 
closely-related than that.

 > you cannot chmod a file that is open ... the chmod call will fail.

Where is this documented?  Does the problem occur if any process has the 
file open, or only if the current process has it open?  What is errno 
after the failure?  This problem does not occur on POSIXish platforms; 
if it happens under Microsoft Windows the incompatibility should be 
documented (in Gnulib, if the problem is generic to GNU applications).

 > the fact that the problem could have been solved in more than
 > one way doesn't mean there are multiple changes involved.

True, but if the problem could have been solved in a simpler way that 
involved fewer changes, then multiple changes were most likely present.

Come to think of it, why is that chmod needed at all in WINDOWSNT?  The 
file is already readable and writeable, so as I understand it chmod 644 
is therefore a no-op on that platform.  If so, the attached patch would 
have been simpler yet, no?

[-- Attachment #2: update1.diff --]
[-- Type: text/x-patch, Size: 335 bytes --]

--- update-game-score.c-old	2014-01-25 08:31:13.065305429 -0800
+++ update-game-score.c	2014-01-25 11:56:15.570846522 -0800
@@ -443,8 +443,10 @@
   fd = mkostemp (tempfile, 0);
   if (fd < 0)
     return -1;
+#ifndef WINDOWSNT
   if (fchmod (fd, 0644) != 0)
     return -1;
+#endif
   f = fdopen (fd, "w");
   if (! f)
     return -1;

Re: Patches with independent changes

[Eli Zaretskii]

> Date: Sat, 25 Jan 2014 12:58:09 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: emacs-devel@gnu.org
> 
> Eli Zaretskii wrote:
> > otherwise the build would have failed when compiler warnings are
>  > turned on and used as errors.
> 
> That would have been odd, as other static variables in Emacs are 
> declared and set but never used (e.g., category_table_version) and 
> evidently these builds are not failing.  It's certainly OK to clean up 
> the glitches, but such cleanups are independent changes.

They are not independent.  A variable that becomes unused as result of
a change should be removed as part of that change.

> > Then any number of patches can be installed in a single commit,
> > because they are all "related" -- after all, they are all about Emacs.
> 
> No, that's too broad.  Patches should contain changes that are more 
> closely-related than that.

That's the gist of our disagreement: how close that relation should
be.  Using vague terminology, such as "related", doesn't solve it.

>  > you cannot chmod a file that is open ... the chmod call will fail.
> 
> Where is this documented?  Does the problem occur if any process has the 
> file open, or only if the current process has it open?  What is errno 
> after the failure?  This problem does not occur on POSIXish platforms; 
> if it happens under Microsoft Windows the incompatibility should be 
> documented (in Gnulib, if the problem is generic to GNU applications).

Then document it, please.  Gnulib documentation is not of interest
here.

>  > the fact that the problem could have been solved in more than
>  > one way doesn't mean there are multiple changes involved.
> 
> True, but if the problem could have been solved in a simpler way that 
> involved fewer changes, then multiple changes were most likely present.

It's not simpler.

> Come to think of it, why is that chmod needed at all in WINDOWSNT?  The 
> file is already readable and writeable, so as I understand it chmod 644 
> is therefore a no-op on that platform.  If so, the attached patch would 
> have been simpler yet, no?

Removal of this code _could_ have constituted an additional change
unrelated to the original bug.

In any case, we don't want to consider changes that aren't bugfixes at
this time.  The change I made restored code that was there in the
first place.  Any improvements can be considered at a later date.

Re: Patches with independent changes

[Paul Eggert]

Eli Zaretskii wrote:
> A variable that becomes unused as result of
> a change should be removed as part of that change.

That guideline makes sense to me too.  Some other software projects 
don't do that: their developers split such patches into pieces, one to 
remove the need for a variable, the other to remove the variable.  To my 
mind this is mostly make-work, and its costs exceed its benefits, and 
I'm glad we don't insist on such things with Emacs.

>>   > you cannot chmod a file that is open ... the chmod call will fail.
>>
>> Where is this documented?  Does the problem occur if any process has the
>> file open, or only if the current process has it open?  What is errno
>> after the failure?  This problem does not occur on POSIXish platforms;
>> if it happens under Microsoft Windows the incompatibility should be
>> documented (in Gnulib, if the problem is generic to GNU applications).
>
> Then document it, please.

We shouldn't document a statement that may be mistaken.

> Removal of this code _could_ have constituted an additional change
> unrelated to the original bug.

The patch I mentioned is simpler than the patch in trunk bzr 116116. 
It's not an additional change compared to 116116; on the contrary, 
116116 is the one containing an additional change.

That being said, I agree that we should let this code alone during the 
feature freeze.