Re: [RFC PATCH 1/7] add --chmod: don't update index when --dry-run is used

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 17, 2021, 21:45 UTC
Message-ID: <xmqqa6s2jrl9.fsf@gitster.g>
In-Reply-To: <2256132f9de01cc06a001aa8c44e29dc5a218441.1613593946.git.matheus.bernardino@usp.br>

Matheus Tavares <matheus.bernardino@usp.br> writes:

> `git add --chmod` applies the mode changes even when `--dry-run` is
> used. Fix that and add some tests for this option combination.

Well spotted. I hope we can split this out of the series and fast track, as it is an obvious bugfix.

I by mistake wrote error(_("...")) in the snippet below, but as a bugfix, we should stick to the existing fprintf(stderr, "...") without _(). i18n should be left outside the "bugfix" change.

Show 15 quoted lines

> -static void chmod_pathspec(struct pathspec *pathspec, char flip)
> +static void chmod_pathspec(struct pathspec *pathspec, char flip, int show_only)
>  {
>  	int i;
>  
> @@ -48,7 +48,8 @@ static void chmod_pathspec(struct pathspec *pathspec, char flip)
>  		if (pathspec && !ce_path_match(&the_index, ce, pathspec, NULL))
>  			continue;
>  
> -		if (chmod_cache_entry(ce, flip) < 0)
> +		if ((show_only && !S_ISREG(ce->ce_mode)) ||
> +		    (!show_only && chmod_cache_entry(ce, flip) < 0))
>  			fprintf(stderr, "cannot chmod %cx '%s'\n", flip, ce->name);
>  	}
>  }

This is a bit dense, especially when the reader does not know by heart that chmod_cache_entry() refuses to chmod anything that is not a regular file.

Even when dry-run, we know chmod will fail when the thing is not a regular file. When not dry-run, we will try chmod and it will report an failure. And we report an error under these conditions.

	if (show_only
	    ? !S_ISREG(ce->ce_mode)
	    : chmod_cache_entry(ce, flip) < 0)
		error(_("cannot chmod ..."), ...);

may express the same idea in a way that is a bit easier to follow.

In any case, that "idea", while it is not wrong per-se, makes it as if the primary purpose of this code is to give an error message, which smells a bit funny.

	if (!show_only)
        	err = chmod_cache_entry(ce, flip);
	else 
        	err = S_ISREG(ce->ce_mode) ? 0 : -1;

	if (err < 0)
		error(_("cannot chmod ..."), ...);

would waste one extra variable, but may make the primary point (i.e. we call chmod_cache_entry() unless dry-run) more clear.

Show 7 quoted lines

> @@ -609,7 +610,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  		exit_status |= add_files(&dir, flags);
>  
>  	if (chmod_arg && pathspec.nr)
> -		chmod_pathspec(&pathspec, chmod_arg[0]);
> +		chmod_pathspec(&pathspec, chmod_arg[0], show_only);
>  	unplug_bulk_checkin();

OK, this side is straight-forward. We know if we are doing --dry-run; we have to pass it down.

Show 22 quoted lines

> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> index b7d4ba608c..fc81f2ef00 100755
> --- a/t/t3700-add.sh
> +++ b/t/t3700-add.sh
> @@ -386,6 +386,26 @@ test_expect_success POSIXPERM 'git add --chmod=[+-]x does not change the working
>  	! test -x foo4
>  '
>  
> +test_expect_success 'git add --chmod honors --dry-run' '
> +	git reset --hard &&
> +	echo foo >foo4 &&
> +	git add foo4 &&
> +	git add --chmod=+x --dry-run foo4 &&
> +	test_mode_in_index 100644 foo4
> +'
> +
> +test_expect_success 'git add --chmod --dry-run reports error for non regular files' '
> +	git reset --hard &&
> +	test_ln_s_add foo foo4 &&
> +	git add --chmod=+x --dry-run foo4 2>stderr &&
> +	grep "cannot chmod +x .foo4." stderr
> +'

Nice that test_ln_s_add lets write this without SYMLINKS prerequisite, as all that matters is what is in the index and not in the working tree.

Show 8 quoted lines

> +test_expect_success 'git add --chmod --dry-run reports error for unmatched pathspec' '
> +	test_must_fail git add --chmod=+x --dry-run nonexistent 2>stderr &&
> +	test_i18ngrep "pathspec .nonexistent. did not match any files" stderr
> +'
> +
>  test_expect_success 'no file status change if no pathspec is given' '
>  	>foo5 &&
>  	>foo6 &&

Thanks.