Re: [PATCH v3 4/4] add: modify already added files when --chmod is given

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 12, 2016, 22:23 UTC
Message-ID: <xmqqy42wycz5.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160912210818.26282-5-t.gummerer@gmail.com>

Thomas Gummerer <t.gummerer@gmail.com> writes:

Show 17 quoted lines

> When the chmod option was added to git add, it was hooked up to the diff
> machinery, meaning that it only works when the version in the index
> differs from the version on disk.
>
> As the option was supposed to mirror the chmod option in update-index,
> which always changes the mode in the index, regardless of the status of
> the file, make sure the option behaves the same way in git add.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
>  builtin/add.c      | 36 +++++++++++++++++++++++++-----------
>  builtin/checkout.c |  2 +-
>  builtin/commit.c   |  2 +-
>  cache.h            | 10 +++++-----
>  read-cache.c       | 14 ++++++--------
>  t/t3700-add.sh     | 21 +++++++++++++++++++++
>  6 files changed, 59 insertions(+), 26 deletions(-)

The change looks quite large but this in essense reverts what Edward did in the first round by hooking into "we add only modified files here" and "we add new files here", both of which are made unnecessary, because this adds the final "now we finished adding things, let's fix modes of paths that match the pathspec".

Which makes sense.

Show 14 quoted lines

> +static void chmod_pathspec(struct pathspec *pathspec, int force_mode)
> +{
> +	int i;
> +	
> +	for (i = 0; i < active_nr; i++) {
> +		struct cache_entry *ce = active_cache[i];
> +
> +		if (pathspec && !ce_path_match(ce, pathspec, NULL))
> +			continue;
> +
> +		if (chmod_cache_entry(ce, force_mode) < 0)
> +			fprintf(stderr, "cannot chmod '%s'", ce->name);
> +	}
> +}

If pathspec is NULL, this will chmod all paths in the index, which is probably not very useful and quite risky operation at the same time.

However ...

> +	if (force_mode)
> +		chmod_pathspec(&pathspec, force_mode);

... the caller never passes a NULL as pathspec.

In any case, I somehow expected to see

	if (force_mode && pathspec.nr)
        	chmod_pathspec(&pathspec, force_mode);

because it would make it very easy to see in the caller that

	git add --chmod=+x              ;# no pathspec
        cd subdir && git add --chmod=+x ;# no pathspec

will be a no-op, which is what we want, if I am not mistaken. Of course, if somebody really wants to drop executable bit from everything, she can do

	git add --chmod=-x .

pretty easily.

Above three may want to be added as new tests. The first two should be a no-op, while the last one should drop executable bits from everywhere.

Thanks.