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

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

[Edward Thomson]

The executable bit will not be detected (and therefore will not be
set) for paths in a repository with `core.filemode` set to false,
though the users may still wish to add files as executable for
compatibility with other users who _do_ have `core.filemode`
functionality.  For example, Windows users adding shell scripts may
wish to add them as executable for compatibility with users on
non-Windows.

Although this can be done with a plumbing command
(`git update-index --add --chmod=+x foo`), teaching the `git-add`
command allows users to set a file executable with a command that
they're already familiar with.

Signed-off-by: Edward Thomson <ethomson@edwardthomson.com>
---
 builtin/add.c  | 12 +++++++++++-
 cache.h        |  2 ++
 read-cache.c   | 11 +++++++++--
 t/t3700-add.sh | 30 ++++++++++++++++++++++++++++++
 4 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 145f06e..44b6c97 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 *chmod_arg = NULL;
+
 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 */
@@ -263,6 +265,7 @@ 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")),
+	OPT_STRING( 0 , "chmod", &chmod_arg, N_("(+/-)x"), N_("override the executable bit of the listed files")),
 	OPT_END(),
 };
 
@@ -336,6 +339,11 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	if (!show_only && ignore_missing)
 		die(_("Option --ignore-missing can only be used together with --dry-run"));
 
+	if (chmod_arg) {
+		if (strcmp(chmod_arg, "-x") && strcmp(chmod_arg, "+x"))
+			die(_("--chmod param must be either -x or +x"));
+	}
+
 	add_new_files = !take_worktree_changes && !refresh_only;
 	require_pathspec = !(take_worktree_changes || (0 < addremove_explicit));
 
@@ -346,7 +354,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		 (intent_to_add ? ADD_CACHE_INTENT : 0) |
 		 (ignore_add_errors ? ADD_CACHE_IGNORE_ERRORS : 0) |
 		 (!(addremove || take_worktree_changes)
-		  ? ADD_CACHE_IGNORE_REMOVAL : 0));
+		  ? ADD_CACHE_IGNORE_REMOVAL : 0)) |
+		 (chmod_arg && *chmod_arg == '+' ? ADD_CACHE_FORCE_EXECUTABLE : 0) |
+		 (chmod_arg && *chmod_arg == '-' ? ADD_CACHE_FORCE_NOTEXECUTABLE : 0);
 
 	if (require_pathspec && argc == 0) {
 		fprintf(stderr, _("Nothing specified, nothing added.\n"));
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
 extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags);
 extern int add_file_to_index(struct index_state *, const char *path, int flags);
 extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, unsigned int refresh_options);
diff --git a/read-cache.c b/read-cache.c
index d9fb78b..d12d143 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -641,6 +641,8 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
 	int intent_only = flags & ADD_CACHE_INTENT;
 	int add_option = (ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE|
 			  (intent_only ? ADD_CACHE_NEW_ONLY : 0));
+	int force_executable = flags & ADD_CACHE_FORCE_EXECUTABLE;
+	int force_notexecutable = flags & ADD_CACHE_FORCE_NOTEXECUTABLE;
 
 	if (!S_ISREG(st_mode) && !S_ISLNK(st_mode) && !S_ISDIR(st_mode))
 		return error("%s: can only add regular files, symbolic links or git-directories", path);
@@ -659,9 +661,14 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
 	else
 		ce->ce_flags |= CE_INTENT_TO_ADD;
 
-	if (trust_executable_bit && has_symlinks)
+	if (S_ISREG(st_mode) && (force_executable || force_notexecutable)) {
+		if (force_executable)
+			ce->ce_mode = create_ce_mode(0777);
+		else
+			ce->ce_mode = create_ce_mode(0666);
+	} else if (trust_executable_bit && has_symlinks) {
 		ce->ce_mode = create_ce_mode(st_mode);
-	else {
+	} else {
 		/* If there is an existing entry, pick the mode bits and type
 		 * from it, otherwise assume unexecutable regular file.
 		 */
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index f14a665..4865304 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -332,4 +332,34 @@ test_expect_success 'git add --dry-run --ignore-missing of non-existing file out
 	test_i18ncmp expect.err actual.err
 '
 
+test_expect_success 'git add --chmod=+x stages a non-executable file with +x' '
+	echo foo >foo1 &&
+	git add --chmod=+x foo1 &&
+	case "$(git ls-files --stage foo1)" in
+	100755" "*foo1) echo pass;;
+	*) echo fail; git ls-files --stage foo1; (exit 1);;
+	esac
+'
+
+test_expect_success 'git add --chmod=-x stages an executable file with -x' '
+	echo foo >xfoo1 &&
+	chmod 755 xfoo1 &&
+	git add --chmod=-x xfoo1 &&
+	case "$(git ls-files --stage xfoo1)" in
+	100644" "*xfoo1) echo pass;;
+	*) echo fail; git ls-files --stage xfoo1; (exit 1);;
+	esac
+'
+
+test_expect_success POSIXPERM,SYMLINKS 'git add --chmod=+x with symlinks' '
+	git config core.filemode 1 &&
+	git config core.symlinks 1 &&
+	echo foo >foo2 &&
+	git add --chmod=+x foo2 &&
+	case "$(git ls-files --stage foo2)" in
+	100755" "*foo2) echo pass;;
+	*) echo fail; git ls-files --stage foo2; (exit 1);;
+	esac
+'
+
 test_done
-- 
2.7.4 (Apple Git-66)

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

[Junio C Hamano]

Edward Thomson <ethomson@edwardthomson.com> writes:

> +static char *chmod_arg = NULL;
> +

I'll drop " = NULL", as it is our convention to let BSS take care of
the zero initialization for global variables as much as possible.

Other than that, I did not see anything objectionable in this round,
but I did notice that you kept the "this takes two bits out of 32"
Dscho mentioned--I do not have a strong preference either way, so
I'll queue it (at least tentatively) on 'pu'.

> @@ -346,7 +354,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  		 (intent_to_add ? ADD_CACHE_INTENT : 0) |
>  		 (ignore_add_errors ? ADD_CACHE_IGNORE_ERRORS : 0) |
>  		 (!(addremove || take_worktree_changes)
> -		  ? ADD_CACHE_IGNORE_REMOVAL : 0));
> +		  ? ADD_CACHE_IGNORE_REMOVAL : 0)) |
> +		 (chmod_arg && *chmod_arg == '+' ? ADD_CACHE_FORCE_EXECUTABLE : 0) |
> +		 (chmod_arg && *chmod_arg == '-' ? ADD_CACHE_FORCE_NOTEXECUTABLE : 0);
>  
>  	if (require_pathspec && argc == 0) {
>  		fprintf(stderr, _("Nothing specified, nothing added.\n"));
> 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

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

[Johannes Schindelin]

Hi Junio & Ed,

On Tue, 31 May 2016, Junio C Hamano wrote:

> Edward Thomson <ethomson@edwardthomson.com> writes:
> 
> > +static char *chmod_arg = NULL;
> > +
> 
> I'll drop " = NULL", as it is our convention to let BSS take care of
> the zero initialization for global variables as much as possible.
> 
> Other than that, I did not see anything objectionable in this round,
> but I did notice that you kept the "this takes two bits out of 32"
> Dscho mentioned--I do not have a strong preference either way, so
> I'll queue it (at least tentatively) on 'pu'.

And here is an add-on patch (Ed, feel free to squash) that avoids those
two bits, and even saves one line overall:

-- snipsnap --
diff --git a/builtin/add.c b/builtin/add.c
index 44b6c97..b1dddb4 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -26,7 +26,7 @@ static int patch_interactive, add_interactive, edit_interactive;
 static int take_worktree_changes;
 
 struct update_callback_data {
-	int flags;
+	int flags, force_mode;
 	int add_errors;
 };
 
@@ -65,7 +65,8 @@ static void update_callback(struct diff_queue_struct *q,
 			die(_("unexpected diff status %c"), p->status);
 		case DIFF_STATUS_MODIFIED:
 		case DIFF_STATUS_TYPE_CHANGED:
-			if (add_file_to_index(&the_index, path, data->flags)) {
+			if (add_file_to_index(&the_index, path,
+					data->flags, data->force_mode)) {
 				if (!(data->flags & ADD_CACHE_IGNORE_ERRORS))
 					die(_("updating files failed"));
 				data->add_errors++;
@@ -83,14 +84,15 @@ static void update_callback(struct diff_queue_struct *q,
 	}
 }
 
-int add_files_to_cache(const char *prefix,
-		       const struct pathspec *pathspec, int flags)
+int add_files_to_cache(const char *prefix, const struct pathspec *pathspec,
+	int flags, int force_mode)
 {
 	struct update_callback_data data;
 	struct rev_info rev;
 
 	memset(&data, 0, sizeof(data));
 	data.flags = flags;
+	data.force_mode = force_mode;
 
 	init_revisions(&rev, prefix);
 	setup_revisions(0, NULL, &rev, NULL);
@@ -238,7 +240,7 @@ static int ignore_add_errors, intent_to_add, ignore_missing;
 static int addremove = ADDREMOVE_DEFAULT;
 static int addremove_explicit = -1; /* unspecified */
 
-static char *chmod_arg = NULL;
+static char *chmod_arg;
 
 static int ignore_removal_cb(const struct option *opt, const char *arg, int unset)
 {
@@ -279,7 +281,7 @@ static int add_config(const char *var, const char *value, void *cb)
 	return git_default_config(var, value, cb);
 }
 
-static int add_files(struct dir_struct *dir, int flags)
+static int add_files(struct dir_struct *dir, int flags, int force_mode)
 {
 	int i, exit_status = 0;
 
@@ -292,7 +294,8 @@ static int add_files(struct dir_struct *dir, int flags)
 	}
 
 	for (i = 0; i < dir->nr; i++)
-		if (add_file_to_cache(dir->entries[i]->name, flags)) {
+		if (add_file_to_index(&the_index, dir->entries[i]->name,
+				flags, force_mode)) {
 			if (!ignore_add_errors)
 				die(_("adding files failed"));
 			exit_status = 1;
@@ -305,7 +308,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	int exit_status = 0;
 	struct pathspec pathspec;
 	struct dir_struct dir;
-	int flags;
+	int flags, force_mode;
 	int add_new_files;
 	int require_pathspec;
 	char *seen = NULL;
@@ -339,10 +342,14 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	if (!show_only && ignore_missing)
 		die(_("Option --ignore-missing can only be used together with --dry-run"));
 
-	if (chmod_arg) {
-		if (strcmp(chmod_arg, "-x") && strcmp(chmod_arg, "+x"))
-			die(_("--chmod param must be either -x or +x"));
-	}
+	if (!chmod_arg)
+		force_mode = 0;
+	else if (!strcmp(chmod_arg, "-x"))
+		force_mode = 0666;
+	else if (!strcmp(chmod_arg, "+x"))
+		force_mode = 0777;
+	else
+		die(_("--chmod param '%s' must be either -x or +x"), chmod_arg);
 
 	add_new_files = !take_worktree_changes && !refresh_only;
 	require_pathspec = !(take_worktree_changes || (0 < addremove_explicit));
@@ -354,9 +361,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		 (intent_to_add ? ADD_CACHE_INTENT : 0) |
 		 (ignore_add_errors ? ADD_CACHE_IGNORE_ERRORS : 0) |
 		 (!(addremove || take_worktree_changes)
-		  ? ADD_CACHE_IGNORE_REMOVAL : 0)) |
-		 (chmod_arg && *chmod_arg == '+' ? ADD_CACHE_FORCE_EXECUTABLE : 0) |
-		 (chmod_arg && *chmod_arg == '-' ? ADD_CACHE_FORCE_NOTEXECUTABLE : 0);
+		  ? ADD_CACHE_IGNORE_REMOVAL : 0));
 
 	if (require_pathspec && argc == 0) {
 		fprintf(stderr, _("Nothing specified, nothing added.\n"));
@@ -436,10 +441,10 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	plug_bulk_checkin();
 
-	exit_status |= add_files_to_cache(prefix, &pathspec, flags);
+	exit_status |= add_files_to_cache(prefix, &pathspec, flags, force_mode);
 
 	if (add_new_files)
-		exit_status |= add_files(&dir, flags);
+		exit_status |= add_files(&dir, flags, force_mode);
 
 	unplug_bulk_checkin();
 
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 3398c61..c3486bd 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -548,7 +548,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
 			 * entries in the index.
 			 */
 
-			add_files_to_cache(NULL, NULL, 0);
+			add_files_to_cache(NULL, NULL, 0, 0);
 			/*
 			 * NEEDSWORK: carrying over local changes
 			 * when branches have different end-of-line
diff --git a/builtin/commit.c b/builtin/commit.c
index 443ff91..163dbca 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -386,7 +386,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 	 */
 	if (all || (also && pathspec.nr)) {
 		hold_locked_index(&index_lock, 1);
-		add_files_to_cache(also ? prefix : NULL, &pathspec, 0);
+		add_files_to_cache(also ? prefix : NULL, &pathspec, 0, 0);
 		refresh_cache_or_die(refresh_flags);
 		update_main_cache_tree(WRITE_TREE_SILENT);
 		if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))
diff --git a/cache.h b/cache.h
index da03cd9..c73becb 100644
--- a/cache.h
+++ b/cache.h
@@ -367,8 +367,8 @@ extern void free_name_hash(struct index_state *istate);
 #define rename_cache_entry_at(pos, new_name) rename_index_entry_at(&the_index, (pos), (new_name))
 #define remove_cache_entry_at(pos) remove_index_entry_at(&the_index, (pos))
 #define remove_file_from_cache(path) remove_file_from_index(&the_index, (path))
-#define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags))
-#define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags))
+#define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags), 0)
+#define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags), 0)
 #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL)
 #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options))
 #define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options))
@@ -581,10 +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
-extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags);
-extern int add_file_to_index(struct index_state *, const char *path, int flags);
+extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags, int force_mode);
+extern int add_file_to_index(struct index_state *, const char *path, int flags, int force_mode);
 extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, unsigned int refresh_options);
 extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);
 extern void set_object_name_for_intent_to_add_entry(struct cache_entry *ce);
@@ -1774,7 +1772,7 @@ void packet_trace_identity(const char *prog);
  * return 0 if success, 1 - if addition of a file failed and
  * ADD_FILES_IGNORE_ERRORS was specified in flags
  */
-int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int flags);
+int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int flags, int force_mode);
 
 /* diff.c */
 extern int diff_auto_refresh_index;
diff --git a/read-cache.c b/read-cache.c
index d12d143..db27766 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -630,7 +630,7 @@ void set_object_name_for_intent_to_add_entry(struct cache_entry *ce)
 	hashcpy(ce->sha1, sha1);
 }
 
-int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags)
+int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags, int force_mode)
 {
 	int size, namelen, was_same;
 	mode_t st_mode = st->st_mode;
@@ -641,8 +641,6 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
 	int intent_only = flags & ADD_CACHE_INTENT;
 	int add_option = (ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE|
 			  (intent_only ? ADD_CACHE_NEW_ONLY : 0));
-	int force_executable = flags & ADD_CACHE_FORCE_EXECUTABLE;
-	int force_notexecutable = flags & ADD_CACHE_FORCE_NOTEXECUTABLE;
 
 	if (!S_ISREG(st_mode) && !S_ISLNK(st_mode) && !S_ISDIR(st_mode))
 		return error("%s: can only add regular files, symbolic links or git-directories", path);
@@ -661,14 +659,11 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
 	else
 		ce->ce_flags |= CE_INTENT_TO_ADD;
 
-	if (S_ISREG(st_mode) && (force_executable || force_notexecutable)) {
-		if (force_executable)
-			ce->ce_mode = create_ce_mode(0777);
-		else
-			ce->ce_mode = create_ce_mode(0666);
-	} else if (trust_executable_bit && has_symlinks) {
+	if (S_ISREG(st_mode) && force_mode)
+		ce->ce_mode = create_ce_mode(force_mode);
+	else if (trust_executable_bit && has_symlinks)
 		ce->ce_mode = create_ce_mode(st_mode);
-	} else {
+	else {
 		/* If there is an existing entry, pick the mode bits and type
 		 * from it, otherwise assume unexecutable regular file.
 		 */
@@ -727,12 +722,13 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
 	return 0;
 }
 
-int add_file_to_index(struct index_state *istate, const char *path, int flags)
+int add_file_to_index(struct index_state *istate, const char *path,
+	int flags, int force_mode)
 {
 	struct stat st;
 	if (lstat(path, &st))
 		die_errno("unable to stat '%s'", path);
-	return add_to_index(istate, path, &st, flags);
+	return add_to_index(istate, path, &st, flags, force_mode);
 }
 
 struct cache_entry *make_cache_entry(unsigned int mode,

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

[Johannes Schindelin]

On Wed, 1 Jun 2016, Johannes Schindelin wrote:

> And here is an add-on patch (Ed, feel free to squash) that avoids those
> two bits, and even saves one line overall:
> 
> [...]

And here is a link for developers who prefer to work with Git directly
(as opposed to working with Git through a mailbox):

	https://github.com/git/git/compare/dscho:force-chmod

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

[Junio C Hamano]

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

> Hi Junio & Ed,
>
> On Tue, 31 May 2016, Junio C Hamano wrote:
>
>> Edward Thomson <ethomson@edwardthomson.com> writes:
>> 
>> > +static char *chmod_arg = NULL;
>> > +
>> 
>> I'll drop " = NULL", as it is our convention to let BSS take care of
>> the zero initialization for global variables as much as possible.
>> 
>> Other than that, I did not see anything objectionable in this round,
>> but I did notice that you kept the "this takes two bits out of 32"
>> Dscho mentioned--I do not have a strong preference either way, so
>> I'll queue it (at least tentatively) on 'pu'.
>
> And here is an add-on patch (Ed, feel free to squash) that avoids those
> two bits, and even saves one line overall:

Unlike the "something like this" we saw earlier, this draws the
boundary of responsibility between the caller and the API at a much
more sensible place.

The difference between the versions with and without this update
essentially is that the original had two bits in flags (among 32
total, 5 bits are currently used and the patch uses 2 more bits),
and this instead changes the function signature of functions that
took "flags" so that lal of them take one additional word.  So in
that sense, because we have enough bits to use, this is not a great
improvement.

However.

The filemode that we force is not two independent bits, but a
tristate: do not force, force executable and force non-executable.
And from that point of view, the two seemingly-independent bits
looked a bit strange, and I prefer the separation this update gives
us slightly more for that reason.  I didn't think carefully about
the change to add_to_index(), but doing it this way _may_ make it
easier for us to later extend it to force "this is file on
filesystem but record it as a symlink", in which case, even if we do
not plan to implement such an enhancement in a near future, I think
this is the right direction to go in.

Thanks.

>
> -- snipsnap --
> diff --git a/builtin/add.c b/builtin/add.c
> index 44b6c97..b1dddb4 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -26,7 +26,7 @@ static int patch_interactive, add_interactive, edit_interactive;
>  static int take_worktree_changes;
>  
>  struct update_callback_data {
> -	int flags;
> +	int flags, force_mode;
>  	int add_errors;
>  };
>  
> @@ -65,7 +65,8 @@ static void update_callback(struct diff_queue_struct *q,
>  			die(_("unexpected diff status %c"), p->status);
>  		case DIFF_STATUS_MODIFIED:
>  		case DIFF_STATUS_TYPE_CHANGED:
> -			if (add_file_to_index(&the_index, path, data->flags)) {
> +			if (add_file_to_index(&the_index, path,
> +					data->flags, data->force_mode)) {
>  				if (!(data->flags & ADD_CACHE_IGNORE_ERRORS))
>  					die(_("updating files failed"));
>  				data->add_errors++;
> @@ -83,14 +84,15 @@ static void update_callback(struct diff_queue_struct *q,
>  	}
>  }
>  
> -int add_files_to_cache(const char *prefix,
> -		       const struct pathspec *pathspec, int flags)
> +int add_files_to_cache(const char *prefix, const struct pathspec *pathspec,
> +	int flags, int force_mode)
>  {
>  	struct update_callback_data data;
>  	struct rev_info rev;
>  
>  	memset(&data, 0, sizeof(data));
>  	data.flags = flags;
> +	data.force_mode = force_mode;
>  
>  	init_revisions(&rev, prefix);
>  	setup_revisions(0, NULL, &rev, NULL);
> @@ -238,7 +240,7 @@ static int ignore_add_errors, intent_to_add, ignore_missing;
>  static int addremove = ADDREMOVE_DEFAULT;
>  static int addremove_explicit = -1; /* unspecified */
>  
> -static char *chmod_arg = NULL;
> +static char *chmod_arg;
>  
>  static int ignore_removal_cb(const struct option *opt, const char *arg, int unset)
>  {
> @@ -279,7 +281,7 @@ static int add_config(const char *var, const char *value, void *cb)
>  	return git_default_config(var, value, cb);
>  }
>  
> -static int add_files(struct dir_struct *dir, int flags)
> +static int add_files(struct dir_struct *dir, int flags, int force_mode)
>  {
>  	int i, exit_status = 0;
>  
> @@ -292,7 +294,8 @@ static int add_files(struct dir_struct *dir, int flags)
>  	}
>  
>  	for (i = 0; i < dir->nr; i++)
> -		if (add_file_to_cache(dir->entries[i]->name, flags)) {
> +		if (add_file_to_index(&the_index, dir->entries[i]->name,
> +				flags, force_mode)) {
>  			if (!ignore_add_errors)
>  				die(_("adding files failed"));
>  			exit_status = 1;
> @@ -305,7 +308,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  	int exit_status = 0;
>  	struct pathspec pathspec;
>  	struct dir_struct dir;
> -	int flags;
> +	int flags, force_mode;
>  	int add_new_files;
>  	int require_pathspec;
>  	char *seen = NULL;
> @@ -339,10 +342,14 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  	if (!show_only && ignore_missing)
>  		die(_("Option --ignore-missing can only be used together with --dry-run"));
>  
> -	if (chmod_arg) {
> -		if (strcmp(chmod_arg, "-x") && strcmp(chmod_arg, "+x"))
> -			die(_("--chmod param must be either -x or +x"));
> -	}
> +	if (!chmod_arg)
> +		force_mode = 0;
> +	else if (!strcmp(chmod_arg, "-x"))
> +		force_mode = 0666;
> +	else if (!strcmp(chmod_arg, "+x"))
> +		force_mode = 0777;
> +	else
> +		die(_("--chmod param '%s' must be either -x or +x"), chmod_arg);
>  
>  	add_new_files = !take_worktree_changes && !refresh_only;
>  	require_pathspec = !(take_worktree_changes || (0 < addremove_explicit));
> @@ -354,9 +361,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  		 (intent_to_add ? ADD_CACHE_INTENT : 0) |
>  		 (ignore_add_errors ? ADD_CACHE_IGNORE_ERRORS : 0) |
>  		 (!(addremove || take_worktree_changes)
> -		  ? ADD_CACHE_IGNORE_REMOVAL : 0)) |
> -		 (chmod_arg && *chmod_arg == '+' ? ADD_CACHE_FORCE_EXECUTABLE : 0) |
> -		 (chmod_arg && *chmod_arg == '-' ? ADD_CACHE_FORCE_NOTEXECUTABLE : 0);
> +		  ? ADD_CACHE_IGNORE_REMOVAL : 0));
>  
>  	if (require_pathspec && argc == 0) {
>  		fprintf(stderr, _("Nothing specified, nothing added.\n"));
> @@ -436,10 +441,10 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  
>  	plug_bulk_checkin();
>  
> -	exit_status |= add_files_to_cache(prefix, &pathspec, flags);
> +	exit_status |= add_files_to_cache(prefix, &pathspec, flags, force_mode);
>  
>  	if (add_new_files)
> -		exit_status |= add_files(&dir, flags);
> +		exit_status |= add_files(&dir, flags, force_mode);
>  
>  	unplug_bulk_checkin();
>  
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 3398c61..c3486bd 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -548,7 +548,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
>  			 * entries in the index.
>  			 */
>  
> -			add_files_to_cache(NULL, NULL, 0);
> +			add_files_to_cache(NULL, NULL, 0, 0);
>  			/*
>  			 * NEEDSWORK: carrying over local changes
>  			 * when branches have different end-of-line
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 443ff91..163dbca 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -386,7 +386,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
>  	 */
>  	if (all || (also && pathspec.nr)) {
>  		hold_locked_index(&index_lock, 1);
> -		add_files_to_cache(also ? prefix : NULL, &pathspec, 0);
> +		add_files_to_cache(also ? prefix : NULL, &pathspec, 0, 0);
>  		refresh_cache_or_die(refresh_flags);
>  		update_main_cache_tree(WRITE_TREE_SILENT);
>  		if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))
> diff --git a/cache.h b/cache.h
> index da03cd9..c73becb 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -367,8 +367,8 @@ extern void free_name_hash(struct index_state *istate);
>  #define rename_cache_entry_at(pos, new_name) rename_index_entry_at(&the_index, (pos), (new_name))
>  #define remove_cache_entry_at(pos) remove_index_entry_at(&the_index, (pos))
>  #define remove_file_from_cache(path) remove_file_from_index(&the_index, (path))
> -#define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags))
> -#define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags))
> +#define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags), 0)
> +#define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags), 0)
>  #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL)
>  #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options))
>  #define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options))
> @@ -581,10 +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
> -extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags);
> -extern int add_file_to_index(struct index_state *, const char *path, int flags);
> +extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags, int force_mode);
> +extern int add_file_to_index(struct index_state *, const char *path, int flags, int force_mode);
>  extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, unsigned int refresh_options);
>  extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);
>  extern void set_object_name_for_intent_to_add_entry(struct cache_entry *ce);
> @@ -1774,7 +1772,7 @@ void packet_trace_identity(const char *prog);
>   * return 0 if success, 1 - if addition of a file failed and
>   * ADD_FILES_IGNORE_ERRORS was specified in flags
>   */
> -int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int flags);
> +int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int flags, int force_mode);
>  
>  /* diff.c */
>  extern int diff_auto_refresh_index;
> diff --git a/read-cache.c b/read-cache.c
> index d12d143..db27766 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -630,7 +630,7 @@ void set_object_name_for_intent_to_add_entry(struct cache_entry *ce)
>  	hashcpy(ce->sha1, sha1);
>  }
>  
> -int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags)
> +int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags, int force_mode)
>  {
>  	int size, namelen, was_same;
>  	mode_t st_mode = st->st_mode;
> @@ -641,8 +641,6 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
>  	int intent_only = flags & ADD_CACHE_INTENT;
>  	int add_option = (ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE|
>  			  (intent_only ? ADD_CACHE_NEW_ONLY : 0));
> -	int force_executable = flags & ADD_CACHE_FORCE_EXECUTABLE;
> -	int force_notexecutable = flags & ADD_CACHE_FORCE_NOTEXECUTABLE;
>  
>  	if (!S_ISREG(st_mode) && !S_ISLNK(st_mode) && !S_ISDIR(st_mode))
>  		return error("%s: can only add regular files, symbolic links or git-directories", path);
> @@ -661,14 +659,11 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
>  	else
>  		ce->ce_flags |= CE_INTENT_TO_ADD;
>  
> -	if (S_ISREG(st_mode) && (force_executable || force_notexecutable)) {
> -		if (force_executable)
> -			ce->ce_mode = create_ce_mode(0777);
> -		else
> -			ce->ce_mode = create_ce_mode(0666);
> -	} else if (trust_executable_bit && has_symlinks) {
> +	if (S_ISREG(st_mode) && force_mode)
> +		ce->ce_mode = create_ce_mode(force_mode);
> +	else if (trust_executable_bit && has_symlinks)
>  		ce->ce_mode = create_ce_mode(st_mode);
> -	} else {
> +	else {
>  		/* If there is an existing entry, pick the mode bits and type
>  		 * from it, otherwise assume unexecutable regular file.
>  		 */
> @@ -727,12 +722,13 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
>  	return 0;
>  }
>  
> -int add_file_to_index(struct index_state *istate, const char *path, int flags)
> +int add_file_to_index(struct index_state *istate, const char *path,
> +	int flags, int force_mode)
>  {
>  	struct stat st;
>  	if (lstat(path, &st))
>  		die_errno("unable to stat '%s'", path);
> -	return add_to_index(istate, path, &st, flags);
> +	return add_to_index(istate, path, &st, flags, force_mode);
>  }
>  
>  struct cache_entry *make_cache_entry(unsigned int mode,

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

[Edward Thomson]

On Wed, Jun 01, 2016 at 09:00:34AM -0700, Junio C Hamano wrote:
> 
> Unlike the "something like this" we saw earlier, this draws the
> boundary of responsibility between the caller and the API at a much
> more sensible place.

This makes sense to me - Junio, are you taking (or have you already
taken) dscho's patch, or would you like me to squash it and resend?

Thanks-
-ed

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

[Junio C Hamano]

Edward Thomson <ethomson@edwardthomson.com> writes:

> On Wed, Jun 01, 2016 at 09:00:34AM -0700, Junio C Hamano wrote:
>> 
>> Unlike the "something like this" we saw earlier, this draws the
>> boundary of responsibility between the caller and the API at a much
>> more sensible place.
>
> This makes sense to me - Junio, are you taking (or have you already
> taken) dscho's patch, or would you like me to squash it and resend?

I didn't plan to unilaterally squash them into one patch without
hearing from you, so I haven't--Dscho's fix-up is queued directly
on top, ready to be squashed in.

So either is fine by me, either you send a final version to replace
the two patches on et/add-chmod-x topic, or you just tell me to go
ahead.

Well, you practically said the latter already, so I'll do the
squashing.  Thanks.

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

[Duy Nguyen]

On Wed, Jun 1, 2016 at 5:08 AM, Edward Thomson
<ethomson@edwardthomson.com> wrote:
> @@ -263,6 +265,7 @@ 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")),
> +       OPT_STRING( 0 , "chmod", &chmod_arg, N_("(+/-)x"), N_("override the executable bit of the listed files")),

If this is only about +/-x, would --[no-]executable be a better option name?
-- 
Duy

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

[Edward Thomson]

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>
---
 builtin/add.c  | 18 +++++++++++++++++-
 cache.h        |  2 ++
 read-cache.c   |  6 ++++++
 t/t3700-add.sh | 19 +++++++++++++++++++
 4 files changed, 44 insertions(+), 1 deletion(-)

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},
 	OPT_END(),
 };
 
@@ -346,7 +360,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		 (intent_to_add ? ADD_CACHE_INTENT : 0) |
 		 (ignore_add_errors ? ADD_CACHE_IGNORE_ERRORS : 0) |
 		 (!(addremove || take_worktree_changes)
-		  ? ADD_CACHE_IGNORE_REMOVAL : 0));
+		  ? ADD_CACHE_IGNORE_REMOVAL : 0)) |
+		 (should_chmod == '+' ? ADD_CACHE_FORCE_EXECUTABLE : 0) |
+		 (should_chmod == '-' ? ADD_CACHE_FORCE_NOTEXECUTABLE : 0);
 
 	if (require_pathspec && argc == 0) {
 		fprintf(stderr, _("Nothing specified, nothing added.\n"));
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
 extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags);
 extern int add_file_to_index(struct index_state *, const char *path, int flags);
 extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, unsigned int refresh_options);
diff --git a/read-cache.c b/read-cache.c
index d9fb78b..81bf186 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -641,6 +641,8 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
 	int intent_only = flags & ADD_CACHE_INTENT;
 	int add_option = (ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE|
 			  (intent_only ? ADD_CACHE_NEW_ONLY : 0));
+	int force_executable = flags & ADD_CACHE_FORCE_EXECUTABLE;
+	int force_notexecutable = flags & ADD_CACHE_FORCE_NOTEXECUTABLE;
 
 	if (!S_ISREG(st_mode) && !S_ISLNK(st_mode) && !S_ISDIR(st_mode))
 		return error("%s: can only add regular files, symbolic links or git-directories", path);
@@ -661,6 +663,10 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
 
 	if (trust_executable_bit && has_symlinks)
 		ce->ce_mode = create_ce_mode(st_mode);
+	else if (force_executable)
+		ce->ce_mode = create_ce_mode(0777);
+	else if (force_notexecutable)
+		ce->ce_mode = create_ce_mode(0666);
 	else {
 		/* If there is an existing entry, pick the mode bits and type
 		 * from it, otherwise assume unexecutable regular file.
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index f14a665..e551eaf 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -332,4 +332,23 @@ test_expect_success 'git add --dry-run --ignore-missing of non-existing file out
 	test_i18ncmp expect.err actual.err
 '
 
+test_expect_success 'git add --chmod=+x stages a non-executable file with +x' '
+	echo foo >foo1 &&
+	git add --chmod=+x foo1 &&
+	case "$(git ls-files --stage foo1)" in
+	100755" "*foo1) echo pass;;
+	*) echo fail; git ls-files --stage foo1; (exit 1);;
+	esac
+'
+
+test_expect_success 'git add --chmod=-x stages an executable file with -x' '
+	echo foo >xfoo1 &&
+	chmod 755 xfoo1 &&
+	git add --chmod=-x xfoo1 &&
+	case "$(git ls-files --stage xfoo1)" in
+	100644" "*xfoo1) echo pass;;
+	*) echo fail; git ls-files --stage xfoo1; (exit 1);;
+	esac
+'
+
 test_done
-- 
2.6.4 (Apple Git-63)

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

[Junio C Hamano]

Edward Thomson <ethomson@edwardthomson.com> writes:

> 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 think we should tone down the first sentence of the proposed
commit log message.  With "s/deficient //" the paragraph still reads
perfectly well.  Even when an underlying filesystem is capable of
expressing executable bit, we can set core.filemode to false to
emulate the behaviour on DOS, so perhaps

    The executable bit will not be set for paths in a repository
    with core.filemode set to false, but the users may still wish to
    add files to ...

or something like it?

Giving an easy to use single-command short-hand for a common thing
that takes two commands is a worthy goal.  Another way to do the
above is

	git update-index --add --chmod=+x foo

and from that point of view, we do not need this patch, but that is
still a mouthful ;-)  I think it is a good idea to teach "git add",
an end-user facing command, to be more helpful.

At the design level, I have a few comments.

 * Unlike the command "chmod", which is _only_ about changing modes,
   "add --chmod" updates both the contents and modes in the index,
   which may invite "I only want to change modes--how?"

	Note. We had an ancient regression at 227bdb18 (make
	update-index --chmod work with multiple files and --stdin,
	2006-04-23), where "update-index --chmod=+x foo" stopped
	being "only flip the executable bit without hashing the
	contents" and that was done purely by mistake.  There is no
	longer a good answer to that question, which makes the above
	worry less of an issue.

 * This is about a repository with core.filemode=0; I wonder if
   something for a repository with core.symlinks=0 would also help?
   That is, would it be a big help to users if they can prepare a
   text file that holds symbolic link contents and add it as if it
   were a symlink with "git add", instead of having to run two
   commands, "hash-objects && update-index --cacheinfo"?

 * I am not familiar with life on filesystems with core.filemode=0;
   do files people would want to be able to "add --chmod=+x" share
   common trait that can be expressed with .gitattributes mechanism?

   What I am wondering is if a scheme like the following would work
   well, in addition to your patch:

   1. Have these in .gitattributes:

      *		-executable
      *.bat	executable text
      *.exe	executable binary
      *.com	executable binary

      A path with Unset "executable" attribute is explicitly marked
      as "not executable"; a path with Set "executable" attribute is
      marked as "executable", i.e. "needing chmod=+x".

   2. Teach "git add" to take the above hint _only_ in a repository
      where core.filemode is false and _only_ when adding a new path
      to the index.

   If something like this works well enough, users do not have to
   type --chmod=+x too often when doing "git add"; your patch
   becomes an escape hatch that is only needed when the attributes
   system gets it wrong.


Now some comments on the actual code.

> +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;
> +}

I know you mimicked the command line parser of update-index, but you
didn't have to, and you shouldn't have.

The command line semantics of update-index is largely "we read one
option and prepare to make its effect immediately available", which
predates parse-options where its attitude for command line parsing
is "we first parse all options and figure out what to do, and then
we work on arguments according to these options".  Because of these
vastly different attitudes, the way builtin/update-index.c uses
parse-options API is atypical.  The only reason it uses callback is
because it wants to allow you to say this:

    git update-index --chmod=+x foo bar --chmod=-x baz

and register foo and bar as executable, while baz as non-executable.

The way update-index uses parse-options API is not something you
want to mimick when adding a similar option to a more modern command
like "git add", whose attitude toward command line parsing is quite
different.  Modern command line parsing typically takes "the last
one wins" semantics, i.e.

    git add --chmod=-x --chmod=+x foo

would make foo executable.

If I were doing this patch, I'd just allocate a file scope global
"static char *chmod_arg;" and OPT_STRING("chmod") to set it, After
parse_options() returns, I'd do something like:

	if (chmod_arg) {
        	if (strcmp(chmod_arg, "-x") && strcmp(chmod_arg, "+x"))
			die("--chmod param must be either -x or +x");
	}

> @@ -661,6 +663,10 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
>  
>  	if (trust_executable_bit && has_symlinks)
>  		ce->ce_mode = create_ce_mode(st_mode);
> +	else if (force_executable)
> +		ce->ce_mode = create_ce_mode(0777);
> +	else if (force_notexecutable)
> +		ce->ce_mode = create_ce_mode(0666);

This is an iffy design decision.

Even when you are in core.filemode=true repository, if you
explicitly said

	git add --chmod=+x READ.ME

wouldn't you expect that the path would have executable bit in the
index, whether it has it as executable in the filesystem?  The above
if/else cascade, because trust-executable-bit is tested first, will
ignore force_* flags altogether, won't it?  It also is strange that
the decision to honor or ignore force_* flags is also tied to
has_symlinks, which is a totally orthogonal concept.

> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> index f14a665..e551eaf 100755
> --- a/t/t3700-add.sh
> +++ b/t/t3700-add.sh
> @@ -332,4 +332,23 @@ test_expect_success 'git add --dry-run --ignore-missing of non-existing file out
>  	test_i18ncmp expect.err actual.err
>  '
>  
> +test_expect_success 'git add --chmod=+x stages a non-executable file with +x' '
> +	echo foo >foo1 &&
> +	git add --chmod=+x foo1 &&
> +	case "$(git ls-files --stage foo1)" in
> +	100755" "*foo1) echo pass;;
> +	*) echo fail; git ls-files --stage foo1; (exit 1);;
> +	esac
> +'
> +
> +test_expect_success 'git add --chmod=-x stages an executable file with -x' '
> +	echo foo >xfoo1 &&
> +	chmod 755 xfoo1 &&
> +	git add --chmod=-x xfoo1 &&
> +	case "$(git ls-files --stage xfoo1)" in
> +	100644" "*xfoo1) echo pass;;
> +	*) echo fail; git ls-files --stage xfoo1; (exit 1);;
> +	esac
> +'
> +
>  test_done

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

[Johannes Schindelin]

Hi Junio,

On Wed, 25 May 2016, Junio C Hamano wrote:

>  * I am not familiar with life on filesystems with core.filemode=0;
>    do files people would want to be able to "add --chmod=+x" share
>    common trait that can be expressed with .gitattributes mechanism?

I think it is safe to say that the biggest example of core.filemode == 0
is Windows. On that platform, there simply is no executable bit in the
sense of POSIX permissions. There are Access Control Lists that let you
permit or deny certain users from executing certain files (and it is files
only, directories are a never "executable" as in POSIX' scheme).

And on Windows, you certainly do not mark a file explicitly as executable
when creating it. The file extension determines whether it is executable
or not, and that's it.

In a sense, it is cleaner than POSIX' permission system: it separates
between the permissions and the classification "is it executable"?

Side note: shell scripts in Git for Windows are a special type of animal.
They *cannot* be made executable by Windows because there is just no
concept of free-form scripts that can choose whatever interpreter they
want to run in. In Git Bash, we have a special hack that is inherited
transitively from Cygwin, where scripts are automatically marked as
executable if their contents start with a shebang. This stops working as
soon as you leave the Bash, of course. Due to Git's heavy dependence on
shell scripting, we had to imitate that same concept in compat/mingw.c. It
is ugly, it is slow, and we have to live with it.

As a consequence of this very different concept of an "executable bit",
you will actually see quite a few Windows-only repositories that *never*
mark their executables with 0755. In Git for Windows' own repositories,
there are a couple of examples where scripts were introduced and only much
later did I realize that they were not marked executable (and then I ran
`git update-index --chmod=+x` on them).

All that means that the `--chmod` option is really useful only to
cross-platform projects, and only with careful developers. (And those
developers could use update-index, as you pointed out.)

I still like Ed's idea and would love to have it: it is murky waters to
require users to call plumbing only because our porcelain isn't up to par.

Ciao,
Dscho

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

[Junio C Hamano]

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

> On Wed, 25 May 2016, Junio C Hamano wrote:
>
>>  * I am not familiar with life on filesystems with core.filemode=0;
>>    do files people would want to be able to "add --chmod=+x" share
>>    common trait that can be expressed with .gitattributes mechanism?
>
> I think it is safe to say that the biggest example of core.filemode == 0
> is Windows. On that platform, there simply is no executable bit in the
> sense of POSIX permissions. ...
> ... I still like Ed's idea and would love to have it: it is murky waters to
> require users to call plumbing only because our porcelain isn't up to par.

I thought that I made it absolutely clear that I like the addition,
too.  If it wasn't clear enough, I can say it again, but I do not
think you need it ;-).

The "attribute" thing was an idea that was hoping to make the system
as a whole even more helpful; if pattern matching with paths is
sufficient for projects to hint desired permission bits per paths,
then those working on such a cross-platform project on Windows do
not have to even worry about "git cmd --chmod=+x", whether cmd is
add or update-index.  If they can just do "git add" and need to use
the new "--chmod=+x" option only when the patterns are not set up
correctly, wouldn't that be even more helpful?  In other words, it
wasn't "with this we can _eliminate_ need for 'add --chmod'".

The only thing I was unsure about that scheme was if "pattern
matching with paths" is sufficiently powerful (if not, such an
addition would not work as a mechanism to reduce the need for the
users to run "git add --chmod=+x").  And that was my inquiry.

Unfortunately, your answer does not help answer that question;
it was a question to Edward, so that's OK anyway.

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

[Johannes Schindelin]

Hi Junio,

On Wed, 25 May 2016, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Wed, 25 May 2016, Junio C Hamano wrote:
> >
> >>  * I am not familiar with life on filesystems with core.filemode=0;
> >>    do files people would want to be able to "add --chmod=+x" share
> >>    common trait that can be expressed with .gitattributes mechanism?
> >
> > I think it is safe to say that the biggest example of core.filemode == 0
> > is Windows. On that platform, there simply is no executable bit in the
> > sense of POSIX permissions. ...
> > ... I still like Ed's idea and would love to have it: it is murky waters to
> > require users to call plumbing only because our porcelain isn't up to par.
> 
> I thought that I made it absolutely clear that I like the addition,
> too.  If it wasn't clear enough, I can say it again, but I do not
> think you need it ;-).

Oh, I understood that you liked it, sorry if my mail looked accusatory.

> The "attribute" thing was an idea that was hoping to make the system
> as a whole even more helpful;

I understood that, too. My first impression was that it would not be.
However, as Git for Windows can set default attributes in
/mingw64/etc/gitconfig, I guess it would actually be helpful. We could
automatically mark all *.exe, *.com, *.bat, *.cmd files as executable. It
would then still be the users' responsibility to add their own attributes
for, say, *.js, *.rb, *.py, *.sh, and whatever else.

Ciao,
Dscho

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> @@ -661,6 +663,10 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
>>  
>>  	if (trust_executable_bit && has_symlinks)
>>  		ce->ce_mode = create_ce_mode(st_mode);
>> +	else if (force_executable)
>> +		ce->ce_mode = create_ce_mode(0777);
>> +	else if (force_notexecutable)
>> +		ce->ce_mode = create_ce_mode(0666);
>
> This is an iffy design decision.
>
> Even when you are in core.filemode=true repository, if you
> explicitly said
>
> 	git add --chmod=+x READ.ME
>
> wouldn't you expect that the path would have executable bit in the
> index, whether it has it as executable in the filesystem?  The above
> if/else cascade, because trust-executable-bit is tested first, will
> ignore force_* flags altogether, won't it?  It also is strange that
> the decision to honor or ignore force_* flags is also tied to
> has_symlinks, which is a totally orthogonal concept.

Here is an additional patch to your tests.  It repeats one of the
tests you added, but runs in a repository with core.filemode and
core.symlinks both enabled.  The test fails to force executable bit
on platforms where it runs.

It passes with your patch if you drop core.symlinks, which is a good
demonstration why letting has_symlinks decide if force* is to be
honored is iffy.

 t/t3700-add.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index e551eaf..2afcb74 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -351,4 +351,15 @@ test_expect_success 'git add --chmod=-x stages an executable file with -x' '
 	esac
 '
 
+test_expect_success POSIXPERM,SYMLINKS 'git add --chmod=+x' '
+	git config core.filemode 1 &&
+	git config core.symlinks 1 &&
+	echo foo >foo2 &&
+	git add --chmod=+x foo2 &&
+	case "$(git ls-files --stage foo2)" in
+	100755" "*foo2) echo pass;;
+	*) echo fail; git ls-files --stage foo2; (exit 1);;
+	esac
+'
+
 test_done

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

[Edward Thomson]

On Wed, May 25, 2016 at 12:36:55AM -0700, Junio C Hamano wrote:
> 
> At the design level, I have a few comments.

Thanks, I will submit a new patch that incorporates your (and dscho's)
comments.

>  * This is about a repository with core.filemode=0; I wonder if
>    something for a repository with core.symlinks=0 would also help?
>    That is, would it be a big help to users if they can prepare a
>    text file that holds symbolic link contents and add it as if it
>    were a symlink with "git add", instead of having to run two
>    commands, "hash-objects && update-index --cacheinfo"?

I think that this is much less common and - speaking only from personal
experience - nobody has ever asked me how to stage a symlink on a
Windows machine.  I think that this is due to the fact that symlinks on
Windows are basically impossible to use, so people doing cross-platform
development wouldn't even try.

On the other hand, it's quite common for cross-platform teams to use
some scripting language since those do work across platforms, and
Windows users would want to add new scripts as executable for the
benefit of their brethren on platforms with an executable bit.

>  * I am not familiar with life on filesystems with core.filemode=0;
>    do files people would want to be able to "add --chmod=+x" share
>    common trait that can be expressed with .gitattributes mechanism?

Perhaps...  It would not be things like `*.bat` or `*.exe` - Windows
gets those as executable "for free" and would not care about adding the
execute bit on those files (since they're not executable anywhere else).
It would be items like `*.sh` or `*.rb` that should be executable on
POSIX platforms.

However I do not think that this is a common enough action that it needs
to be made automatic such that when I `git add foo.rb` it is
automatically made executable.  I think that the reduced complexity of
having a single mechanism to control executability (that being the
execute mode in the index or a tree) is preferable to a gitattributes
based mechanism, at least until somebody else makes a cogent argument
that the gitattributes approach would be helpful for them.  :)

Thanks again for the comments, an updated patch is forthcoming.

-ed

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

[Mike Hommey]

On Thu, May 26, 2016 at 11:41:12PM -0500, Edward Thomson wrote:
> On Wed, May 25, 2016 at 12:36:55AM -0700, Junio C Hamano wrote:
> > 
> > At the design level, I have a few comments.
> 
> Thanks, I will submit a new patch that incorporates your (and dscho's)
> comments.
> 
> >  * This is about a repository with core.filemode=0; I wonder if
> >    something for a repository with core.symlinks=0 would also help?
> >    That is, would it be a big help to users if they can prepare a
> >    text file that holds symbolic link contents and add it as if it
> >    were a symlink with "git add", instead of having to run two
> >    commands, "hash-objects && update-index --cacheinfo"?
> 
> I think that this is much less common and - speaking only from personal
> experience - nobody has ever asked me how to stage a symlink on a
> Windows machine.  I think that this is due to the fact that symlinks on
> Windows are basically impossible to use, so people doing cross-platform
> development wouldn't even try.
> 
> On the other hand, it's quite common for cross-platform teams to use
> some scripting language since those do work across platforms, and
> Windows users would want to add new scripts as executable for the
> benefit of their brethren on platforms with an executable bit.
> 
> >  * I am not familiar with life on filesystems with core.filemode=0;
> >    do files people would want to be able to "add --chmod=+x" share
> >    common trait that can be expressed with .gitattributes mechanism?
> 
> Perhaps...  It would not be things like `*.bat` or `*.exe` - Windows
> gets those as executable "for free" and would not care about adding the
> execute bit on those files (since they're not executable anywhere else).
> It would be items like `*.sh` or `*.rb` that should be executable on
> POSIX platforms.
> 
> However I do not think that this is a common enough action that it needs
> to be made automatic such that when I `git add foo.rb` it is
> automatically made executable.

Moreover, *.sh, *.rb, etc. are not necessarily meant to be executables.
The files might be modules, included from executables or other modules.
There's an example of this right in the git tree: t/test-lib.sh. It's
even more typical for *.rb files (or *.py, etc.)

However, the common pattern that /might/ be interesting for automatic
executable bits is files starting with "#!".

Mike

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

[Junio C Hamano]

Edward Thomson <ethomson@edwardthomson.com> writes:

> However I do not think that this is a common enough action that it needs
> to be made automatic such that when I `git add foo.rb` it is
> automatically made executable.  I think that the reduced complexity of
> having a single mechanism to control executability (that being the
> execute mode in the index or a tree) is preferable to a gitattributes
> based mechanism, at least until somebody else makes a cogent argument
> that the gitattributes approach would be helpful for them.  :)

It wasn't a "having to specify it every time sucks; you must do this
way instead" at all.  I was just gauging if it would be a viable idea
for a follow-up series to complement your patch.

Thanks.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Edward Thomson <ethomson@edwardthomson.com> writes:
>
>> However I do not think that this is a common enough action that it needs
>> to be made automatic such that when I `git add foo.rb` it is
>> automatically made executable.  I think that the reduced complexity of
>> having a single mechanism to control executability (that being the
>> execute mode in the index or a tree) is preferable to a gitattributes
>> based mechanism, at least until somebody else makes a cogent argument
>> that the gitattributes approach would be helpful for them.  :)
>
> It wasn't a "having to specify it every time sucks; you must do this
> way instead" at all.  I was just gauging if it would be a viable idea
> for a follow-up series to complement your patch.
>
> Thanks.

Oh, having said all of that, the comments on the implementation
still stand.

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

[Edward Thomson]

On Fri, May 27, 2016 at 11:30:40AM -0700, Junio C Hamano wrote:
> 
> Oh, having said all of that, the comments on the implementation
> still stand.

Certainly; sorry for the delay.  I've squashed in your tests and applied
your recommendations.  Resending the patch momentarily.

Thanks-

-ed

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

[Johannes Schindelin]

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

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

[Junio C Hamano]

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()).

> 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;
> +		}

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

[Junio C Hamano]

Edward Thomson <ethomson@edwardthomson.com> writes:

>  	if (trust_executable_bit && has_symlinks)
>  		ce->ce_mode = create_ce_mode(st_mode);
> +	else if (force_executable)
> +		ce->ce_mode = create_ce_mode(0777);
> +	else if (force_notexecutable)
> +		ce->ce_mode = create_ce_mode(0666);
>  	else {
>  		/* If there is an existing entry, pick the mode bits and type
>  		 * from it, otherwise assume unexecutable regular file.

I would rather do this part more like:

	if (S_ISREG(st_mode) && (force_executable || force_nonexecuable)) {
        	if (force_executable)
			ce->ce_mode = create_ce_mode(0777);
		else
			ce->ce_mode = create_ce_mode(0666);
	} else if (trust_executable_bit && has_symlinks) {
        	ce->ce_mode = create_ce_mode(st_mode);
	} else {
        	... carry the existing mode over ...

which would make sure that the new code will not interfere with
symbolic links and that forcing will be honored even on filesystems
whose executable bit can be trusted (i.e. "can be trusted" does not
have to mean "must be trusted").