Re: [PATCH v3 2/4] update-index: use the same structure for chmod as add

[Junio C Hamano <gitster@pobox.com>]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> +	char flip = force_mode == 0777 ? '+' : '-';
>  
>  	pos = cache_name_pos(path, strlen(path));
>  	if (pos < 0)
> @@ -432,17 +433,11 @@ static void chmod_path(int flip, const char *path)
>  	mode = ce->ce_mode;
>  	if (!S_ISREG(mode))
>  		goto fail;
> -	switch (flip) {
> -	case '+':
> -		ce->ce_mode |= 0111; break;
> -	case '-':
> -		ce->ce_mode &= ~0111; break;
> -	default:
> -		goto fail;
> -	}
> +	ce->ce_mode = create_ce_mode(force_mode);

create_ce_mode() is supposed to take the st_mode taken from a
"struct stat", but here force_mode is 0777 or something else.  The
above to feed only 0777 or 0666 may happen to work with the way how
create_ce_mode() is implemented now, but it is a time-bomb waiting
to go off.

Instead of using force_mode that is unsigned, keep the 'flip' as
argument, and do:

	if (!S_ISREG(mode))
        	goto fail;
+	if (flip == '+')
+		mode |= 0111;
+	else
+		mode &= ~0111;
	ce->ce_mode = create_ce_mode(mode);

perhaps, as you do not need and are not using the full 9 bits in
force_mode anyway.

That would mean chmod_index_entry() introduced in 3/4 and its user
in 4/4 need to be updated to pass '+' or '-' down the callchain, but
I think that is a good change for the same reason.  We do not allow
you to set full 9 bits; let's not pretend as if we do so by passing
just a single bit '+' or '-' around.

I think 3/4 needs to be fixed where it calls create_ce_mode() with
only the 0666/0777 in chmod_index_entry() anyway, regardless of the
above suggested change.

> diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh
> index dfe02f4..32ac6e0 100755
> --- a/t/t2107-update-index-basic.sh
> +++ b/t/t2107-update-index-basic.sh
> @@ -80,4 +80,17 @@ test_expect_success '.lock files cleaned up' '
>  	)
>  '
>  
> +test_expect_success '--chmod=+x and chmod=-x in the same argument list' '
> +	>A &&
> +	>B &&
> +	git add A B &&
> +	git update-index --chmod=+x A --chmod=-x B &&
> +	cat >expect <<-\EOF &&
> +	100755 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	A
> +	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	B
> +	EOF
> +	git ls-files --stage A B >actual &&
> +	test_cmp expect actual
> +'

Thanks for adding this test.  We may want to cross the b/c bridge in
a later version bump, but until then it is good to make sure we know
what the currently expected behaviour is.