From: Sebastien Marie <semarie@online.fr>
Subject: Re: fix directory mode bits
To: gameoftrees@openbsd.org
Date: Tue, 5 May 2020 17:58:46 +0200

On Tue, May 05, 2020 at 01:06:45PM +0200, Stefan Sperling wrote:
> 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
> 
> [...]
> 
> 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?

it makes sens, and read fine.

ok semarie@
 
> 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)
> 

-- 
Sebastien Marie