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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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 <ethomson@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