Re: [PATCH v3 2/4] update-index: use the same structure for chmod as add
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 12, 2016, 21:59 UTC
- Message-ID
- <xmqq37l4zsmo.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20160912210818.26282-3-t.gummerer@gmail.com>
Thomas Gummerer <t.gummerer@gmail.com> writes:
Show 17 quoted lines
> + 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.
Show 20 quoted lines
> 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.