Re: [PATCH] add: add --chmod=+x / --chmod=-x options

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

Junio C Hamano <gitster@pobox.com> writes:

>> @@ -661,6 +663,10 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
>>  
>>  	if (trust_executable_bit && has_symlinks)
>>  		ce->ce_mode = create_ce_mode(st_mode);
>> +	else if (force_executable)
>> +		ce->ce_mode = create_ce_mode(0777);
>> +	else if (force_notexecutable)
>> +		ce->ce_mode = create_ce_mode(0666);
>
> This is an iffy design decision.
>
> Even when you are in core.filemode=true repository, if you
> explicitly said
>
> 	git add --chmod=+x READ.ME
>
> wouldn't you expect that the path would have executable bit in the
> index, whether it has it as executable in the filesystem?  The above
> if/else cascade, because trust-executable-bit is tested first, will
> ignore force_* flags altogether, won't it?  It also is strange that
> the decision to honor or ignore force_* flags is also tied to
> has_symlinks, which is a totally orthogonal concept.

Here is an additional patch to your tests.  It repeats one of the
tests you added, but runs in a repository with core.filemode and
core.symlinks both enabled.  The test fails to force executable bit
on platforms where it runs.

It passes with your patch if you drop core.symlinks, which is a good
demonstration why letting has_symlinks decide if force* is to be
honored is iffy.

 t/t3700-add.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index e551eaf..2afcb74 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -351,4 +351,15 @@ test_expect_success 'git add --chmod=-x stages an executable file with -x' '
 	esac
 '
 
+test_expect_success POSIXPERM,SYMLINKS 'git add --chmod=+x' '
+	git config core.filemode 1 &&
+	git config core.symlinks 1 &&
+	echo foo >foo2 &&
+	git add --chmod=+x foo2 &&
+	case "$(git ls-files --stage foo2)" in
+	100755" "*foo2) echo pass;;
+	*) echo fail; git ls-files --stage foo2; (exit 1);;
+	esac
+'
+
 test_done