From: Ted Bullock <tbullock@comlore.com>
Subject: Re: vi struggling to save commit messages
To: "Todd C. Miller" <Todd.Miller@sudo.ws>
Cc: gameoftrees@openbsd.org
Date: Wed, 8 Mar 2023 13:19:39 -0700


On 2023-03-08 11:02 a.m., Todd C. Miller wrote:
> On Wed, 08 Mar 2023 10:42:07 -0700, Ted Bullock wrote:
> 
>> I thought that nfs v3 was the default? It does say so for mount_nfs(3)
>>
>> In my /etc/fstab I did just add a -3 to the options line and remounted
>> the nfs share, and didn't see a difference in behavior for what it's
>> worth. Maybe I'm missing something?
> 
> OK, sounds like that is not the problem then.  You could instrument
> vi (or just attach a debugger) to see whether it is the size or
> timestamp that is different from what it has cached.
> 
>  - todd

I can see why it triggers, it's comparing identical timestamps, not
relative. This is inconsistent with the error text too which explicitly
says "file modified more recently than this copy".

timespeccmp(&sb.st_mtim, &ep->mtim, !=)

should be

timespeccmp(&sb.st_mtim, &ep->mtim, <=)

from vi/common/exf.c

	if (stat(name, &sb))
		mtype = NEWFILE;
	else {
		if (noname && !LF_ISSET(FS_FORCE | FS_APPEND) &&
		    ((F_ISSET(ep, F_DEVSET) &&
		    (sb.st_dev != ep->mdev || sb.st_ino != ep->minode)) ||
		    timespeccmp(&sb.st_mtim, &ep->mtim, !=))) {
			msgq_str(sp, M_ERR, name, LF_ISSET(FS_POSSIBLE) ?
"%s: file modified more recently than this copy; use ! to override" :
"%s: file modified more recently than this copy");
			return (1);
		}

		mtype = OLDFILE;


Also, this is a hideously complex if statement, yikes, it took me 15
minutes to discombobulate it.

So there are a couple problems with the query as written above:

1. changes to file parameters fail with identical error text as a
timestamp difference
2. the timespec comparison is using an inconsistent comparator given
the error text.
3. there are WAY too many nested conditionals here. It's comical.

Further, there is some more subtlety to think about, this is exposed
because the clocks on the NFS server and the client vary at some amount
of nanoseconds, but really GOT shouldn't put vi in the position where it
won't work even if the timestamps are widely off.

Here is a diff to vi that fixes this issue that GOT triggers for me and
makes it readable by a mortal:

diff /usr/src
commit - c91a0f502346babf19c59a6b6d974a94c32ea1b1
path + /usr/src
blob - 1d966db18237b542256c9d0b13e2b95ae51328d8
file + usr.bin/vi/common/exf.c
--- usr.bin/vi/common/exf.c
+++ usr.bin/vi/common/exf.c
@@ -795,19 +795,35 @@ file_write(SCR *sp, MARK *fm, MARK *tm, char *name, in
 	 */
 	if (stat(name, &sb))
 		mtype = NEWFILE;
-	else {
-		if (noname && !LF_ISSET(FS_FORCE | FS_APPEND) &&
-		    ((F_ISSET(ep, F_DEVSET) &&
-		    (sb.st_dev != ep->mdev || sb.st_ino != ep->minode)) ||
-		    timespeccmp(&sb.st_mtim, &ep->mtim, !=))) {
-			msgq_str(sp, M_ERR, name, LF_ISSET(FS_POSSIBLE) ?
-"%s: file modified more recently than this copy; use ! to override" :
-"%s: file modified more recently than this copy");
+	else if (noname && !LF_ISSET(FS_FORCE | FS_APPEND)) {
+		char *msg;
+		/* check if the file has changed since */
+		if (F_ISSET(ep, F_DEVSET) &&
+		    (sb.st_dev != ep->mdev || sb.st_ino != ep->minode)) {
+			if (LF_ISSET(FS_POSSIBLE))
+				msg = "%s: file parameters have changed "
+				      "use ! to override";
+			else
+				msg = "%s: file parameters have changed";
+
+			msgq_str(sp, M_ERR, name, msg);
 			return (1);
+		} else if (timespeccmp(&sb.st_mtim, &ep->mtim, <=)) {
+			if (LF_ISSET(FS_POSSIBLE))
+				msg = "%s: file modified more recently "
+				      "than this copy; use ! to override";
+			else
+				msg = "%s: file modified more recently "
+				      "than this copy";
+
+			msgq_str(sp, M_ERR, name, msg);
+			return (1);
 		}
 
 		mtype = OLDFILE;
 	}
+	else
+		mtype = OLDFILE;
 
 	/* Set flags to create, write, and either append or truncate. */
 	oflags = O_CREAT | O_WRONLY |