Re: [PATCH v3 2/4] update-index: use the same structure for chmod as add
From: Junio C Hamano <hidden>
Date: 2016-09-12 21:59:51
Thomas Gummerer [off-list ref] writes:
quoted hunk
+ 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.
quoted hunk
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.