"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Stefan Sperling <stsp@stsp.name>
Subject:
fix directory mode bits
To:
gameoftrees@openbsd.org
Date:
Tue, 5 May 2020 13:06:45 +0200

Download raw body.

Thread 
Not long ago I fixed a problem with file modes that was reported by semarie@.
See commit f7b97ccb29b3e414e360ff635f9bc114f8db7c2f:
https://git.gameoftrees.org/gitweb/?p=got.git;a=commit;h=f7b97ccb29b3e414e360ff635f9bc114f8db7c2f

In that commit I used the GOT_DEFAULT_DIR_MODE macro (400755) to set the
mode for tree entries which represent directories.
This mode is correct for on-disk directories, however Git writes
in-repository directory tree entries with permission bits clear:

A tree object written by Got uses 0040755 directory modes:

5002ac02445f9ab3b2fc89397a45f00b8b287fd2 0100644 Makefile
ad5d4f10860d6c9f9ad299711d6261bfcf4dbe65 0100644 Makefile.cross
db671be984664ce133430230838e0de270673f91 0040755 bin
d91fe01de2e53309008b9aefd443259c33c2b693 0040755 distrib
c7275d347bf7fdc406ada4b642b2f32124337bc8 0040755 etc
1353ec34f4982066b73c097656934b31d5d9c15c 0040755 games
d5184a3e240951f504c9bc9a29aa856e7071085f 0040755 gnu
c6e923dff3b6195349d54b86a77e6c9569323020 0040755 include
efa871620aaaf3e9784340794390059a3ce3a2eb 0040755 lib
78b5a2e9a5112d337d0552c9b37322d4d6101fac 0040755 libexec
f34b008a1ede0417c8f559e733a276cb8e9cba24 0040755 regress
84de4dfd4080b1f95a05fe749ea76f78be7253d8 0040755 sbin
3d5f9c6916d9c5cb4ed6d53acd87193199da80a0 0040755 share
f168f79190f2710aaeb90f89fbc517ef25913586 0040755 sys
9891c1e0b22c7d3d7c4e87291f1175c555ef339b 0040755 usr.bin
9ae249e2cb56dcaeafeddb46da62d50c916a4a6b 0040755 usr.sbin

A different tree object written by Git uses 0040000 directory modes:

5002ac02445f9ab3b2fc89397a45f00b8b287fd2 0100644 Makefile
ad5d4f10860d6c9f9ad299711d6261bfcf4dbe65 0100644 Makefile.cross
db671be984664ce133430230838e0de270673f91 0040000 bin
d91fe01de2e53309008b9aefd443259c33c2b693 0040000 distrib
c7275d347bf7fdc406ada4b642b2f32124337bc8 0040000 etc
1353ec34f4982066b73c097656934b31d5d9c15c 0040000 games
d5184a3e240951f504c9bc9a29aa856e7071085f 0040000 gnu
c6e923dff3b6195349d54b86a77e6c9569323020 0040000 include
efa871620aaaf3e9784340794390059a3ce3a2eb 0040000 lib
78b5a2e9a5112d337d0552c9b37322d4d6101fac 0040000 libexec
f34b008a1ede0417c8f559e733a276cb8e9cba24 0040000 regress
84de4dfd4080b1f95a05fe749ea76f78be7253d8 0040000 sbin
3d5f9c6916d9c5cb4ed6d53acd87193199da80a0 0040000 share
fd78ffee748bb4215f4593504c5b572800e56ee7 0040000 sys
9891c1e0b22c7d3d7c4e87291f1175c555ef339b 0040000 usr.bin
9ae249e2cb56dcaeafeddb46da62d50c916a4a6b 0040000 usr.sbin

I noticed this because this difference in mode bits caused unexpected
commits to show up in my logs. 
For example, my local 'nomimo' branch did not modify the /etc directory,
yet it shows up in the output of this command:

[[[
$ cd /usr/src
$ got log -l2 -c nomimo -P etc 
-----------------------------------------------
commit 16b77845b78e0c6e6e85bb3384fc5e11b1ca454e (nomimo)
from: Stefan Sperling <stsp@stsp.name>
date: Mon Apr 27 18:45:17 2020 UTC
 
 add nomimo nwflag
 
 M  sbin/ifconfig/ifconfig.8
 M  sys/dev/ic/athn.c
 M  sys/dev/pci/if_iwm.c
 M  sys/dev/pci/if_iwx.c
 M  sys/net80211/ieee80211_ioctl.h

-----------------------------------------------
commit 38cbc561f766c7b9bfe0441270fae3c3f3e96978
from: ratchov <ratchov@openbsd.org>
date: Fri Apr 24 20:09:30 2020 UTC
 
 regen
 
 M  etc/etc.alpha/MAKEDEV
 M  etc/etc.arm64/MAKEDEV
 M  etc/etc.hppa/MAKEDEV
 M  etc/etc.i386/MAKEDEV
 M  etc/etc.landisk/MAKEDEV
 M  etc/etc.loongson/MAKEDEV
 M  etc/etc.macppc/MAKEDEV
 M  etc/etc.octeon/MAKEDEV
 M  etc/etc.sgi/MAKEDEV
 M  etc/etc.sparc64/MAKEDEV
]]]

With the patch below, the same command has the expected output:

[[[
$ got log -l2 -c nomimo -P etc 
-----------------------------------------------
commit 38cbc561f766c7b9bfe0441270fae3c3f3e96978
from: ratchov <ratchov@openbsd.org>
date: Fri Apr 24 20:09:30 2020 UTC
 
 regen
 
 M  etc/etc.alpha/MAKEDEV
 M  etc/etc.arm64/MAKEDEV
 M  etc/etc.hppa/MAKEDEV
 M  etc/etc.i386/MAKEDEV
 M  etc/etc.landisk/MAKEDEV
 M  etc/etc.loongson/MAKEDEV
 M  etc/etc.macppc/MAKEDEV
 M  etc/etc.octeon/MAKEDEV
 M  etc/etc.sgi/MAKEDEV
 M  etc/etc.sparc64/MAKEDEV

-----------------------------------------------
commit 66992aee90df49b625ea82f263b7d91481c5eed0
from: ratchov <ratchov@openbsd.org>
date: Fri Apr 24 20:09:04 2020 UTC
 
 Bump audio devices count to 4
 
 ok deraadt
 
 M  etc/etc.alpha/MAKEDEV.md
 M  etc/etc.amd64/MAKEDEV
 M  etc/etc.amd64/MAKEDEV.md
 M  etc/etc.arm64/MAKEDEV.md
 M  etc/etc.hppa/MAKEDEV.md
 M  etc/etc.i386/MAKEDEV.md
 M  etc/etc.landisk/MAKEDEV.md
 M  etc/etc.loongson/MAKEDEV.md
 M  etc/etc.macppc/MAKEDEV.md
 M  etc/etc.octeon/MAKEDEV.md
 M  etc/etc.sgi/MAKEDEV.md
 M  etc/etc.sparc64/MAKEDEV.md
]]]

The patch makes two changes:

1) When comparing modes of tree entries, only check the bits which actually
   matter for our purposes: We want to detect files replaced with directories
   and vice-versa, and we want to detect the executable bit changing.
   None of the other mode bits are versioned by Git and Got from the user's
   perspective anyway. This prevents existing commits with 755 bits set from
   causing bogus log entries as shown above.

2) Write directory tree entry mode bits in the same way as Git does.

ok?

diff 0208f208304c36921fbcd86d33751b877aab1e96 /home/stsp/src/got
blob - ea5232abfd828a7f52894757aaa990fca8c07c9b
file + lib/object.c
--- lib/object.c
+++ lib/object.c
@@ -1624,6 +1624,25 @@ done:
 	return err;
 }
 
+/*
+ * Normalize file mode bits to avoid false positive tree entry differences
+ * in case tree entries have unexpected mode bits set.
+ */
+static mode_t
+normalize_mode_for_comparison(mode_t mode)
+{
+	/*
+	 * For directories, the only relevant bit is the IFDIR bit.
+	 * This allows us to detect paths changing from a directory
+	 * to a file and vice versa.
+	 */
+	if (S_ISDIR(mode))
+		return mode & S_IFDIR;
+
+	/* For files, the only change we care about is the executable bit. */
+	return mode & S_IXUSR;
+}
+
 const struct got_error *
 got_object_tree_path_changed(int *changed,
     struct got_tree_object *tree01, struct got_tree_object *tree02,
@@ -1650,6 +1669,7 @@ got_object_tree_path_changed(int *changed,
 	seglen = 0;
 	while (*s) {
 		struct got_tree_object *next_tree1, *next_tree2;
+		mode_t mode1, mode2;
 
 		if (*s != '/') {
 			s++;
@@ -1670,7 +1690,9 @@ got_object_tree_path_changed(int *changed,
 			goto done;
 		}
 
-		if (te1->mode != te2->mode) {
+		mode1 = normalize_mode_for_comparison(te1->mode);
+		mode2 = normalize_mode_for_comparison(te2->mode);
+		if (mode1 != mode2) {
 			*changed = 1;
 			goto done;
 		}
blob - de279d7ac5f87f20be6173c3f7932fb898e2254d
file + lib/object_create.c
--- lib/object_create.c
+++ lib/object_create.c
@@ -212,14 +212,14 @@ te_mode2str(char *buf, size_t len, mode_t te_mode)
 	 * For best compatibility we normalize the file/directory mode here.
 	 * Note that we do not support committing symlinks or submodules.
 	 */
-	if (S_ISREG(te_mode))
+	if (S_ISREG(te_mode)) {
 		mode = GOT_DEFAULT_FILE_MODE;
-	else if (S_ISDIR(te_mode))
-		mode = GOT_DEFAULT_DIR_MODE;
+		if (te_mode & (S_IXUSR | S_IXGRP | S_IXOTH))
+			mode |= S_IXUSR | S_IXGRP | S_IXOTH;
+	} else if (S_ISDIR(te_mode))
+		mode = S_IFDIR; /* Git leaves all the other bits unset. */
 	else
 		return got_error(GOT_ERR_BAD_FILETYPE);
-	if (te_mode & (S_IXUSR | S_IXGRP | S_IXOTH))
-		mode |= S_IXUSR | S_IXGRP | S_IXOTH;
 
 	ret = snprintf(buf, len, "%o ", mode);
 	if (ret == -1 || ret >= len)