Re: [PATCH] add: add --chmod=+x / --chmod=-x options

From: Junio C Hamano <gitster@pobox.com>
Date: May 27, 2016, 18:35 UTC
Message-ID: <xmqqinxzqrhn.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <alpine.DEB.2.20.1605250923120.4449@virtualbox>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> I wonder, however, whether it would be "cleaner" to simply make this an
> OPT_STRING and perform the validation after the option parsing.

Yes, I think I touched on this in my comments in a bit more detail.

> Hmm. This change uses up 2 out of 31 available bits. I wonder whether a
> better idea would be to extend struct update_callback_data to include a
> `force_mode` field, pass a parameter of the same name to
> add_files_to_cache() and then handle that in the update_callback().

Maybe. I am not sure if it is a good idea to do lstat(2) on the calling side, though. Assuming it is, your "something like this" needs to be duplicated for the codepath that adds a new file, which is separate from the one we see below (i.e. add_files()).

Show 18 quoted lines

> Something like this:
>
>                 case DIFF_STATUS_MODIFIED:
> -               case DIFF_STATUS_TYPE_CHANGED:
> +               case DIFF_STATUS_TYPE_CHANGED: {
> +			struct stat st;
> +			if (lstat(path, &st))
> +				die_errno("unable to stat '%s'", path);
> +			if (S_ISREG(&st.st_mode) && data->force_mode)
> +				st.st_mode = data->force_mode;
> -                       if (add_file_to_index(&the_index, path, data->flags)) {
> +                       if (add_to_index(&the_index, path, &st, data->flags)) {
>                                 if (!(data->flags & ADD_CACHE_IGNORE_ERRORS))
>                                         die(_("updating files failed"));
>                                 data->add_errors++;
>                         }
>                         break;
> +		}