Re: [PATCH] add: add --chmod=+x / --chmod=-x options
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- May 25, 2016, 07:46 UTC
- Message-ID
- <alpine.DEB.2.20.1605250923120.4449@virtualbox>
- In-Reply-To
- <20160525020609.GA20123@zoidberg>
Hi Ed,
On Tue, 24 May 2016, Edward Thomson wrote:
Show 8 quoted lines
> Users on deficient filesystems that lack an execute bit may still > wish to add files to the repository with the appropriate execute > bit set (or not). Although this can be done in two steps > (`git add foo && git update-index --chmod=+x foo`), providing the > `--chmod=+x` option to the add command allows users to set a file > executable in a single command that they're already familiar with. > > Signed-off-by: Edward Thomson <ethomson@edwardthomson.com>
I like it! Some comments below:
Show 36 quoted lines
> diff --git a/builtin/add.c b/builtin/add.c
> index 145f06e..2a9abf7 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -238,6 +238,8 @@ static int ignore_add_errors, intent_to_add, ignore_missing;
> static int addremove = ADDREMOVE_DEFAULT;
> static int addremove_explicit = -1; /* unspecified */
>
> +static char should_chmod = 0;
> +
> static int ignore_removal_cb(const struct option *opt, const char *arg, int unset)
> {
> /* if we are told to ignore, we are not adding removals */
> @@ -245,6 +247,15 @@ static int ignore_removal_cb(const struct option *opt, const char *arg, int unse
> return 0;
> }
>
> +static int chmod_cb(const struct option *opt, const char *arg, int unset)
> +{
> + char *flip = opt->value;
> + if ((arg[0] != '-' && arg[0] != '+') || arg[1] != 'x' || arg[2])
> + return error("option 'chmod' expects \"+x\" or \"-x\"");
> + *flip = arg[0];
> + return 0;
> +}
> +
> static struct option builtin_add_options[] = {
> OPT__DRY_RUN(&show_only, N_("dry run")),
> OPT__VERBOSE(&verbose, N_("be verbose")),
> @@ -263,6 +274,9 @@ static struct option builtin_add_options[] = {
> OPT_BOOL( 0 , "refresh", &refresh_only, N_("don't add, only refresh the index")),
> OPT_BOOL( 0 , "ignore-errors", &ignore_add_errors, N_("just skip files which cannot be added because of errors")),
> OPT_BOOL( 0 , "ignore-missing", &ignore_missing, N_("check if - even missing - files are ignored in dry run")),
> + { OPTION_CALLBACK, 0, "chmod", &should_chmod, N_("(+/-)x"),
> + N_("override the executable bit of the listed files"),
> + PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP, chmod_cb},I wonder, however, whether it would be "cleaner" to simply make this an OPT_STRING and perform the validation after the option parsing. Something like:
const char *chmod_string = NULL;
...
OPT_STRING( 0 , "chmod", &chmod_string, N_("( +x | -x )"),
N_("override the executable bit of the listed files")),
...
flags = ...
if (chmod_string) {
if (!strcmp("+x", chmod_string))
flags |= ADD_CACHE_FORCE_EXECUTABLE;
else if (!strcmp("-x", chmod_string))
flags |= ADD_CACHE_FORCE_NOTEXECUTABLE;
else
die(_("invalid --chmod value: %s"), chmod_string);
}Show 10 quoted lines
> diff --git a/cache.h b/cache.h > index 6049f86..da03cd9 100644 > --- a/cache.h > +++ b/cache.h > @@ -581,6 +581,8 @@ extern int remove_file_from_index(struct index_state *, const char *path); > #define ADD_CACHE_IGNORE_ERRORS 4 > #define ADD_CACHE_IGNORE_REMOVAL 8 > #define ADD_CACHE_INTENT 16 > +#define ADD_CACHE_FORCE_EXECUTABLE 32 > +#define ADD_CACHE_FORCE_NOTEXECUTABLE 64
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(). 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;
+ }This would not only contain the changes in builtin/add.c, it would also force the mode change when core.filemode = true and core.symlinks = true (which your version would handle in a surprising way, I believe).
> 2.6.4 (Apple Git-63)
Time to upgrade? ;-)
Ciao, Dscho