From: Junio C Hamano <gitster@pobox.com>
To: Thomas Gummerer <t.gummerer@gmail.com>
Cc: git@vger.kernel.org,
"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
"Jeff King" <peff@peff.net>, "Jan Keromnes" <janx@linux.com>,
"Ingo Brückl" <ib@wupperonline.de>,
"Edward Thomson" <ethomson@edwardthomson.com>
Subject: Re: [PATCH v3 2/4] update-index: use the same structure for chmod as add
Date: Mon, 12 Sep 2016 14:59:43 -0700 [thread overview]
Message-ID: <xmqq37l4zsmo.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20160912210818.26282-3-t.gummerer@gmail.com> (Thomas Gummerer's message of "Mon, 12 Sep 2016 22:08:16 +0100")
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.
next prev parent reply other threads:[~2016-09-12 21:59 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-01 16:08 `make profile-install` fails in 2.9.3 Jan Keromnes
2016-09-01 16:25 ` Dennis Kaarsemaker
2016-09-01 20:07 ` Thomas Gummerer
2016-09-01 21:58 ` Jeff King
2016-09-01 22:16 ` Junio C Hamano
2016-09-01 22:20 ` Jeff King
2016-09-01 22:38 ` Junio C Hamano
2016-09-04 11:39 ` [PATCH 0/4] git add --chmod: always change the file Thomas Gummerer
2016-09-04 11:39 ` [PATCH 1/4] add: document the chmod option Thomas Gummerer
2016-09-05 7:44 ` Johannes Schindelin
2016-09-05 19:22 ` Thomas Gummerer
2016-09-07 16:44 ` Junio C Hamano
2016-09-04 11:39 ` [PATCH 2/4] update-index: use the same structure for chmod as add Thomas Gummerer
2016-09-04 11:39 ` [PATCH 3/4] read-cache: introduce chmod_index_entry Thomas Gummerer
2016-09-04 11:39 ` [PATCH 4/4] add: modify already added files when --chmod is given Thomas Gummerer
2016-09-11 10:30 ` [PATCH v2 0/4] git add --chmod: always change the file Thomas Gummerer
2016-09-11 10:30 ` [PATCH v2 1/4] add: document the chmod option Thomas Gummerer
2016-09-11 10:30 ` [PATCH v2 2/4] update-index: use the same structure for chmod as add Thomas Gummerer
2016-09-11 22:28 ` Junio C Hamano
2016-09-12 19:30 ` Thomas Gummerer
2016-09-11 10:30 ` [PATCH v2 3/4] read-cache: introduce chmod_index_entry Thomas Gummerer
2016-09-11 10:30 ` [PATCH v2 4/4] add: modify already added files when --chmod is given Thomas Gummerer
2016-09-12 21:08 ` [PATCH v3 0/4] git add --chmod: always change the file Thomas Gummerer
2016-09-12 21:08 ` [PATCH v3 1/4] add: document the chmod option Thomas Gummerer
2016-09-12 21:08 ` [PATCH v3 2/4] update-index: use the same structure for chmod as add Thomas Gummerer
2016-09-12 21:59 ` Junio C Hamano [this message]
2016-09-12 21:08 ` [PATCH v3 3/4] read-cache: introduce chmod_index_entry Thomas Gummerer
2016-09-12 21:08 ` [PATCH v3 4/4] add: modify already added files when --chmod is given Thomas Gummerer
2016-09-12 22:23 ` Junio C Hamano
2016-09-14 21:07 ` [PATCH v4 0/4] git add --chmod: always change the file Thomas Gummerer
2016-09-14 21:07 ` [PATCH v4 1/4] add: document the chmod option Thomas Gummerer
2016-09-14 21:07 ` [PATCH v4 2/4] update-index: add test for chmod flags Thomas Gummerer
2016-09-14 21:07 ` [PATCH v4 3/4] read-cache: introduce chmod_index_entry Thomas Gummerer
2016-09-14 21:46 ` Junio C Hamano
2016-09-14 22:54 ` Junio C Hamano
2016-09-15 18:49 ` Thomas Gummerer
2016-09-14 21:07 ` [PATCH v4 4/4] add: modify already added files when --chmod is given Thomas Gummerer
2016-09-14 21:54 ` Junio C Hamano
2017-08-07 21:40 ` René Scharfe
2017-08-12 12:30 ` Thomas Gummerer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq37l4zsmo.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=ethomson@edwardthomson.com \
--cc=git@vger.kernel.org \
--cc=ib@wupperonline.de \
--cc=janx@linux.com \
--cc=peff@peff.net \
--cc=t.gummerer@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).