Hi Ed,
On Tue, 24 May 2016, Edward Thomson wrote:
> 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 <ethom...@edwardthomson.com>
I like it! Some comments below:
> 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);
}
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
