Thomas Gummerer <t.gummerer@xxxxxxxxx> 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.