`make profile-install` fails in 2.9.3

`make profile-install` fails in 2.9.3

[Jan Keromnes]

Hello,

I'm trying to `profile-install` Git from source on Ubuntu 16.04, to
have the latest stable Git optimized for my machine.

However, this fails (and has failed in previous versions), because it
runs the whole test-suite to get the profile, but bails out if there
were test failures (which happens often).

Problem: Is there a way to `make profile-install` but ignore
occasional test failures, as they're not problematic to get a useful
profile?

Related problem: `t3700-add.sh` currently fails in 2.9.3. I can
provide more debug information if you don't already know this problem.

Thanks,
Jan Keromnes

---

Steps to reproduce:

    curl https://www.kernel.org/pub/software/scm/git/git-2.9.3.tar.xz | tar xJ \
     && cd git-2.9.3 \
     && make prefix=/usr profile-install install-man -j18

Expected result:

    - runs all tests to get a profile (ignoring occasional failures)
    - rebuilds Git with the profile
    - installs Git

Actual result:

    - runs all tests to get a profile
    - at least one test fails, interrupting the whole process
    - Git is not installed

Failure log:

    # failed 1 among 40 test(s)
    1..40
    Makefile:43: recipe for target 't3700-add.sh' failed
    make[3]: *** [t3700-add.sh] Error 1
    make[3]: Leaving directory '/tmp/git/git-2.9.3/t'
    Makefile:36: recipe for target 'test' failed
    make[2]: Leaving directory '/tmp/git/git-2.9.3/t'
    make[2]: *** [test] Error 2
    Makefile:2221: recipe for target 'test' failed
    make[1]: *** [test] Error 2
    make[1]: Leaving directory '/tmp/git/git-2.9.3'
    Makefile:1633: recipe for target 'profile' failed
    make: *** [profile] Error 2
    The command '/bin/sh -c mkdir /tmp/git  && cd /tmp/git  && curl
https://www.kernel.org/pub/software/scm/git/git-2.9.3.tar.xz | tar xJ
&& cd git-2.9.3  && make prefix=/usr profile-install install-man -j18
&& rm -rf /tmp/git' returned a non-zero code: 2

Re: `make profile-install` fails in 2.9.3

[Dennis Kaarsemaker]

On do, 2016-09-01 at 18:08 +0200, Jan Keromnes wrote:

> However, this fails (and has failed in previous versions), because it
> runs the whole test-suite to get the profile, but bails out if there
> were test failures (which happens often).

Working around failing tests is fixing a symptom, not the root cause. I
run the testsuite for master, next and pu very regularly and test
failures on master are extremely rare. I think I've seen one or so in
the last year, might even be 0. So let's focus on that instead.

> Related problem: `t3700-add.sh` currently fails in 2.9.3. I can
> provide more debug information if you don't already know this problem.

It should not fail, and for me does not fail on ubuntu 16.04. Please
run that test in verbose mode and share its output, as well as your
build configuration.

D.

Re: `make profile-install` fails in 2.9.3

[Thomas Gummerer]

[+cc Edward Thomson, Ingo Brückl]

Hi,

On 09/01, Jan Keromnes wrote:

[.. snip the parts others are more qualified to answer ..]
> 
> Related problem: `t3700-add.sh` currently fails in 2.9.3. I can
> provide more debug information if you don't already know this problem.

I noticed this problem as well, when I'm compiling with USE_NSEC = 1
in my config.mak.

Tracking this problem down a bit, it happens because the --chmod=[+-]x
option introduced in 4e55ed32 ("add: add --chmod=+x / --chmod=-x
options") only works if the file on disk is modified.  When the test
was changed to work on one single file, instead of doing chmod=+x on
one file and chmod=-x on another file in b38ab197c ("t3700: merge two
tests into one"), this test started breaking when the mtime of the
file and the index file weren't the same (in other words, if the file
was not racily clean and thus was not smudged).

When the file is racily clean, git detects that the contents changed,
and everything is happy, but if it isn't, git incorectly thinks the
file wasn't modified (which it wasn't, but from gits view it should be
viewed as modified because the mode does not match up anymore).

One possible fix for the test is to smudge the entry as showed below,
though I'm not sure it's the right fix.  The other way I can think of
is to change the file in the index regardless of whether the file was
changed in some other way before issuing the git add command, as that
might fit the user expectation better.  Thoughts?

diff --git a/read-cache.c b/read-cache.c
index 491e52d..f2e7986 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -656,11 +656,13 @@ 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_mode)
+	if (S_ISREG(st_mode) && force_mode) {
 		ce->ce_mode = create_ce_mode(force_mode);
-	else if (trust_executable_bit && has_symlinks)
+		ce->ce_stat_data.sd_size = 0;
+	} 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.
 		 */

								           



> Thanks,
> Jan Keromnes
> 
> ---
> 
> Steps to reproduce:
> 
>     curl https://www.kernel.org/pub/software/scm/git/git-2.9.3.tar.xz | tar xJ \
>      && cd git-2.9.3 \
>      && make prefix=/usr profile-install install-man -j18
> 
> Expected result:
> 
>     - runs all tests to get a profile (ignoring occasional failures)
>     - rebuilds Git with the profile
>     - installs Git
> 
> Actual result:
> 
>     - runs all tests to get a profile
>     - at least one test fails, interrupting the whole process
>     - Git is not installed
> 
> Failure log:
> 
>     # failed 1 among 40 test(s)
>     1..40
>     Makefile:43: recipe for target 't3700-add.sh' failed
>     make[3]: *** [t3700-add.sh] Error 1
>     make[3]: Leaving directory '/tmp/git/git-2.9.3/t'
>     Makefile:36: recipe for target 'test' failed
>     make[2]: Leaving directory '/tmp/git/git-2.9.3/t'
>     make[2]: *** [test] Error 2
>     Makefile:2221: recipe for target 'test' failed
>     make[1]: *** [test] Error 2
>     make[1]: Leaving directory '/tmp/git/git-2.9.3'
>     Makefile:1633: recipe for target 'profile' failed
>     make: *** [profile] Error 2
>     The command '/bin/sh -c mkdir /tmp/git  && cd /tmp/git  && curl
> https://www.kernel.org/pub/software/scm/git/git-2.9.3.tar.xz | tar xJ
> && cd git-2.9.3  && make prefix=/usr profile-install install-man -j18
> && rm -rf /tmp/git' returned a non-zero code: 2

-- 
Thomas

Re: `make profile-install` fails in 2.9.3

[Jeff King]

On Thu, Sep 01, 2016 at 09:07:00PM +0100, Thomas Gummerer wrote:

> > Related problem: `t3700-add.sh` currently fails in 2.9.3. I can
> > provide more debug information if you don't already know this problem.
> 
> I noticed this problem as well, when I'm compiling with USE_NSEC = 1
> in my config.mak.

I can replicate this even without USE_NSEC with my stress-tester[1].
That makes sense why it would show up with the profiling run; git runs
slower and therefore increases the chances of crossing the 1-second
boundary and losing the race.

[1] https://github.com/peff/git/blob/meta/stress

> Tracking this problem down a bit, it happens because the --chmod=[+-]x
> option introduced in 4e55ed32 ("add: add --chmod=+x / --chmod=-x
> options") only works if the file on disk is modified.  When the test
> was changed to work on one single file, instead of doing chmod=+x on
> one file and chmod=-x on another file in b38ab197c ("t3700: merge two
> tests into one"), this test started breaking when the mtime of the
> file and the index file weren't the same (in other words, if the file
> was not racily clean and thus was not smudged).

That certainly sounds buggy. A less racy way of verifying this is just:

  # guarantee not-racy state
  echo content >file
  test-chmtime -60 file
  git add file

  # now check --chmod; file will still be 100644!
  git add --chmod=+x file
  git ls-files -s

> One possible fix for the test is to smudge the entry as showed below,
> though I'm not sure it's the right fix.  The other way I can think of
> is to change the file in the index regardless of whether the file was
> changed in some other way before issuing the git add command, as that
> might fit the user expectation better.  Thoughts?

Yeah, I think we should _always_ act on the --chmod, no matter if the
file is racy or not, or whether it has a content change or not. I.e.,
the race is not the problem, but rather the behavior of 4e55ed32. Your
second proposal there sounds more like the right approach.

-Peff

Re: `make profile-install` fails in 2.9.3

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yeah, I think we should _always_ act on the --chmod, no matter if the
> file is racy or not, or whether it has a content change or not. I.e.,
> the race is not the problem, but rather the behavior of 4e55ed32. Your
> second proposal there sounds more like the right approach.

Yeah, you two are absolutely right.  The second "git add --chmod=+x"
in

    $ git add .
    $ git add --chmod=+x .

should still find _all_ the non-executable paths and flip their
executable bit in the index, making them all up-to-date in the
index.

Which means that piggybacking this on the "run 'git diff' limited to
the pathspec to find the paths that needs updating" logic usually
done in "git add" can not be reused [*1*].

What was I thinking while reviewing the patch X-<.  Sigh.


[Footnote]

*1* I guess we _could_, by first flipping all the regular file
    blob's executable bit for paths that are inside the pathspec and
    then by running "git diff" against that modified index, limited
    to the pathspec, to find the paths that need to be added.

    It sounds ugly, but may conceptually be cleaner.  We first start
    from an ideal end-result, and then re-hash what needs to be
    updated to match the ideal.

Re: `make profile-install` fails in 2.9.3

[Jeff King]

On Thu, Sep 01, 2016 at 03:16:38PM -0700, Junio C Hamano wrote:

> Which means that piggybacking this on the "run 'git diff' limited to
> the pathspec to find the paths that needs updating" logic usually
> done in "git add" can not be reused [*1*].
> 
> What was I thinking while reviewing the patch X-<.  Sigh.
> 
> 
> [Footnote]
> 
> *1* I guess we _could_, by first flipping all the regular file
>     blob's executable bit for paths that are inside the pathspec and
>     then by running "git diff" against that modified index, limited
>     to the pathspec, to find the paths that need to be added.
> 
>     It sounds ugly, but may conceptually be cleaner.  We first start
>     from an ideal end-result, and then re-hash what needs to be
>     updated to match the ideal.

Yeah, I had a similar thought, but it just feels so hacky. Is there
anything wrong with making this completely separate from the content
update. I.e., just applying the pathspec to the index as a separate step
and adding "+x" to each entry?

This really is just a more convenient interface around "update-index
--chmod", isn't it? We should be able to do the same thing it does.

-Peff

Re: `make profile-install` fails in 2.9.3

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yeah, I had a similar thought, but it just feels so hacky. Is there
> anything wrong with making this completely separate from the content
> update. I.e., just applying the pathspec to the index as a separate step
> and adding "+x" to each entry?
>
> This really is just a more convenient interface around "update-index
> --chmod", isn't it? We should be able to do the same thing it does.

Sure, the simplest and the most straight-forward way may look dumb,
but it would be the safest.

[PATCH 0/4] git add --chmod: always change the file

[Thomas Gummerer]

Thanks to Peff and Junio for your inputs on the best way to solve this
problem.

The patch series is made up as follows:

[1/4]: Documentation for the chmod option
[2,3/4]: Small refactoring to simplify the final step
[4/4]: The actual change that introduces the new behaviour.

Thomas Gummerer (4):
  add: document the chmod option
  update-index: use the same structure for chmod as add
  read-cache: introduce chmod_index_entry
  add: modify already added files when --chmod is given

 Documentation/git-add.txt |  7 +++++-
 builtin/add.c             | 36 +++++++++++++++++++++----------
 builtin/checkout.c        |  2 +-
 builtin/commit.c          |  2 +-
 builtin/update-index.c    | 55 ++++++++++++++++++-----------------------------
 cache.h                   | 12 ++++++-----
 read-cache.c              | 33 +++++++++++++++++++++-------
 t/t3700-add.sh            | 21 ++++++++++++++++++
 8 files changed, 107 insertions(+), 61 deletions(-)

-- 
2.10.0.304.gf2ff484

[PATCH 1/4] add: document the chmod option

[Thomas Gummerer]

The git add --chmod option was introduced in 4e55ed3 ("add: add
--chmod=+x / --chmod=-x options", 2016-05-31), but was never
documented.  Document the feature.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 Documentation/git-add.txt | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 6a96a66..8caa691 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 'git add' [--verbose | -v] [--dry-run | -n] [--force | -f] [--interactive | -i] [--patch | -p]
 	  [--edit | -e] [--[no-]all | --[no-]ignore-removal | [--update | -u]]
 	  [--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing]
-	  [--] [<pathspec>...]
+	  [--chmod=(+|-)x] [--] [<pathspec>...]
 
 DESCRIPTION
 -----------
@@ -165,6 +165,11 @@ for "git add --no-all <pathspec>...", i.e. ignored removed files.
 	be ignored, no matter if they are already present in the work
 	tree or not.
 
+--chmod=(+|-)::
+	Override the executable bit of the added files.  The executable
+	bit is only changed in the index, the files on disk are left
+	unchanged.
+
 \--::
 	This option can be used to separate command-line options from
 	the list of files, (useful when filenames might be mistaken
-- 
2.10.0.304.gf2ff484

Re: [PATCH 1/4] add: document the chmod option

[Johannes Schindelin]

Hi Thomas,

On Sun, 4 Sep 2016, Thomas Gummerer wrote:

> +--chmod=(+|-)::

There is an "x" missing ;-)

Ciao,
Dscho

Re: [PATCH 1/4] add: document the chmod option

[Thomas Gummerer]

Hi Johannes,

On 09/05, Johannes Schindelin wrote:
> Hi Thomas,
> 
> On Sun, 4 Sep 2016, Thomas Gummerer wrote:
> 
> > +--chmod=(+|-)::
> 
> There is an "x" missing ;-)

Ugh, thanks for catching.  I'll wait a few days for more comments and
address it in a re-roll.

> Ciao,
> Dscho

Re: [PATCH 1/4] add: document the chmod option

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> Ugh, thanks for catching.  I'll wait a few days for more comments and
> address it in a re-roll.

Thanks for starting this.  I agree with you and Peff that running
"chmod" on the resulting index after other "add" operations took
place is the right approach, as we want to flip the executable bit
for paths that hasn't otherwise changed.

[PATCH 2/4] update-index: use the same structure for chmod as add

[Thomas Gummerer]

While the chmod options for update-index and the add have the same
functionality, they are using different ways to parse and handle the
option internally.  Unify these modes in order to make further
refactoring simpler.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/update-index.c | 49 +++++++++++++++++++++----------------------------
 1 file changed, 21 insertions(+), 28 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index ba04b19..85a57db 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -419,11 +419,12 @@ static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,
 	return 0;
 }
 
-static void chmod_path(int flip, const char *path)
+static void chmod_path(int force_mode, const char *path)
 {
 	int pos;
 	struct cache_entry *ce;
 	unsigned int mode;
+	char flip = force_mode == 0777 ? '+' : '-';
 
 	pos = cache_name_pos(path, strlen(path));
 	if (pos < 0)
@@ -432,17 +433,11 @@ static void chmod_path(int flip, const char *path)
 	mode = ce->ce_mode;
 	if (!S_ISREG(mode))
 		goto fail;
-	switch (flip) {
-	case '+':
-		ce->ce_mode |= 0111; break;
-	case '-':
-		ce->ce_mode &= ~0111; break;
-	default:
-		goto fail;
-	}
+	ce->ce_mode = create_ce_mode(force_mode);
 	cache_tree_invalidate_path(&the_index, path);
 	ce->ce_flags |= CE_UPDATE_IN_BASE;
 	active_cache_changed |= CE_ENTRY_CHANGED;
+
 	report("chmod %cx '%s'", flip, path);
 	return;
  fail:
@@ -788,16 +783,6 @@ static int really_refresh_callback(const struct option *opt,
 	return refresh(opt->value, REFRESH_REALLY);
 }
 
-static int chmod_callback(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 int resolve_undo_clear_callback(const struct option *opt,
 				const char *arg, int unset)
 {
@@ -917,7 +902,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	int read_from_stdin = 0;
 	int prefix_length = prefix ? strlen(prefix) : 0;
 	int preferred_index_format = 0;
-	char set_executable_bit = 0;
+	char *chmod_arg = 0;
+	int force_mode = 0;
 	struct refresh_params refresh_args = {0, &has_errors};
 	int lock_error = 0;
 	int split_index = -1;
@@ -955,10 +941,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 			PARSE_OPT_NOARG | /* disallow --cacheinfo=<mode> form */
 			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
 			(parse_opt_cb *) cacheinfo_callback},
-		{OPTION_CALLBACK, 0, "chmod", &set_executable_bit, N_("(+/-)x"),
-			N_("override the executable bit of the listed files"),
-			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
-			chmod_callback},
+		OPT_STRING( 0, "chmod", &chmod_arg, N_("(+/-)x"),
+			N_("override the executable bit of the listed files")),
 		{OPTION_SET_INT, 0, "assume-unchanged", &mark_valid_only, NULL,
 			N_("mark files as \"not changing\""),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, MARK_FLAG},
@@ -1018,6 +1002,15 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(update_index_usage, options);
 
+	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(_("option 'chmod' expects \"+x\" or \"-x\""));
+
 	git_config(git_default_config, NULL);
 
 	/* We can't free this memory, it becomes part of a linked list parsed atexit() */
@@ -1055,8 +1048,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 			setup_work_tree();
 			p = prefix_path(prefix, prefix_length, path);
 			update_one(p);
-			if (set_executable_bit)
-				chmod_path(set_executable_bit, p);
+			if (force_mode)
+				chmod_path(force_mode, p);
 			free(p);
 			ctx.argc--;
 			ctx.argv++;
@@ -1100,8 +1093,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 			}
 			p = prefix_path(prefix, prefix_length, buf.buf);
 			update_one(p);
-			if (set_executable_bit)
-				chmod_path(set_executable_bit, p);
+			if (force_mode)
+				chmod_path(force_mode, p);
 			free(p);
 		}
 		strbuf_release(&unquoted);
-- 
2.10.0.304.gf2ff484

[PATCH 3/4] read-cache: introduce chmod_index_entry

[Thomas Gummerer]

As there are chmod options for both add and update-index, introduce a
new chmod_index_entry function to do the work.  Use it in update-index,
while it will be used in add in the next patch.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/update-index.c |  8 +-------
 cache.h                |  2 ++
 read-cache.c           | 19 +++++++++++++++++++
 3 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 85a57db..1569c81 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -423,20 +423,14 @@ static void chmod_path(int force_mode, const char *path)
 {
 	int pos;
 	struct cache_entry *ce;
-	unsigned int mode;
 	char flip = force_mode == 0777 ? '+' : '-';
 
 	pos = cache_name_pos(path, strlen(path));
 	if (pos < 0)
 		goto fail;
 	ce = active_cache[pos];
-	mode = ce->ce_mode;
-	if (!S_ISREG(mode))
+	if (chmod_cache_entry(ce, force_mode) < 0)
 		goto fail;
-	ce->ce_mode = create_ce_mode(force_mode);
-	cache_tree_invalidate_path(&the_index, path);
-	ce->ce_flags |= CE_UPDATE_IN_BASE;
-	active_cache_changed |= CE_ENTRY_CHANGED;
 
 	report("chmod %cx '%s'", flip, path);
 	return;
diff --git a/cache.h b/cache.h
index b780a91..44a4f76 100644
--- a/cache.h
+++ b/cache.h
@@ -369,6 +369,7 @@ extern void free_name_hash(struct index_state *istate);
 #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), 0)
 #define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags), 0)
+#define chmod_cache_entry(ce, force_mode) chmod_index_entry(&the_index, (ce), (force_mode))
 #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))
@@ -584,6 +585,7 @@ extern int remove_file_from_index(struct index_state *, const char *path);
 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 chmod_index_entry(struct index_state *, struct cache_entry *ce, int force_mode);
 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);
 extern int index_name_is_other(const struct index_state *, const char *, int);
diff --git a/read-cache.c b/read-cache.c
index 491e52d..367be57 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -756,6 +756,25 @@ struct cache_entry *make_cache_entry(unsigned int mode,
 	return ret;
 }
 
+/*
+ * Change the mode of an index entry to force_mode, where force_mode can
+ * either be 0777 or 0666.
+ * Returns -1 if the chmod for the particular cache entry failed (if it's
+ * not a regular file), 0 otherwise.
+ */
+int chmod_index_entry(struct index_state *istate, struct cache_entry *ce,
+		       int force_mode)
+{
+	if (!S_ISREG(ce->ce_mode))
+		return -1;
+	ce->ce_mode = create_ce_mode(force_mode);
+	cache_tree_invalidate_path(istate, ce->name);
+	ce->ce_flags |= CE_UPDATE_IN_BASE;
+	istate->cache_changed |= CE_ENTRY_CHANGED;
+
+	return 0;
+}
+
 int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)
 {
 	int len = ce_namelen(a);
-- 
2.10.0.304.gf2ff484

[PATCH 4/4] add: modify already added files when --chmod is given

[Thomas Gummerer]

When the chmod option was added to git add, it was hooked up to the diff
machinery, meaning that it only works when the version in the index
differs from the version on disk.

As the option was supposed to mirror the chmod option in update-index,
which always changes the mode in the index, regardless of the status of
the file, make sure the option behaves the same way in git add.

Reported-by: Jan Keromnes <janx@linux.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/add.c      | 36 +++++++++++++++++++++++++-----------
 builtin/checkout.c |  2 +-
 builtin/commit.c   |  2 +-
 cache.h            | 10 +++++-----
 read-cache.c       | 14 ++++++--------
 t/t3700-add.sh     | 21 +++++++++++++++++++++
 6 files changed, 59 insertions(+), 26 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index b1dddb4..892198a 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -26,10 +26,25 @@ static int patch_interactive, add_interactive, edit_interactive;
 static int take_worktree_changes;
 
 struct update_callback_data {
-	int flags, force_mode;
+	int flags;
 	int add_errors;
 };
 
+static void chmod_pathspec(struct pathspec *pathspec, int force_mode)
+{
+	int i;
+	
+	for (i = 0; i < active_nr; i++) {
+		struct cache_entry *ce = active_cache[i];
+
+		if (pathspec && !ce_path_match(ce, pathspec, NULL))
+			continue;
+
+		if (chmod_cache_entry(ce, force_mode) < 0)
+			fprintf(stderr, "cannot chmod '%s'", ce->name);
+	}
+}
+
 static int fix_unmerged_status(struct diff_filepair *p,
 			       struct update_callback_data *data)
 {
@@ -65,8 +80,7 @@ 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, data->force_mode)) {
+			if (add_file_to_index(&the_index, path,	data->flags)) {
 				if (!(data->flags & ADD_CACHE_IGNORE_ERRORS))
 					die(_("updating files failed"));
 				data->add_errors++;
@@ -84,15 +98,14 @@ static void update_callback(struct diff_queue_struct *q,
 	}
 }
 
-int add_files_to_cache(const char *prefix, const struct pathspec *pathspec,
-	int flags, int force_mode)
+int add_files_to_cache(const char *prefix,
+		       const struct pathspec *pathspec, int flags)
 {
 	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);
@@ -281,7 +294,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, int force_mode)
+static int add_files(struct dir_struct *dir, int flags)
 {
 	int i, exit_status = 0;
 
@@ -294,8 +307,7 @@ static int add_files(struct dir_struct *dir, int flags, int force_mode)
 	}
 
 	for (i = 0; i < dir->nr; i++)
-		if (add_file_to_index(&the_index, dir->entries[i]->name,
-				flags, force_mode)) {
+		if (add_file_to_index(&the_index, dir->entries[i]->name, flags)) {
 			if (!ignore_add_errors)
 				die(_("adding files failed"));
 			exit_status = 1;
@@ -441,11 +453,13 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	plug_bulk_checkin();
 
-	exit_status |= add_files_to_cache(prefix, &pathspec, flags, force_mode);
+	exit_status |= add_files_to_cache(prefix, &pathspec, flags);
 
 	if (add_new_files)
-		exit_status |= add_files(&dir, flags, force_mode);
+		exit_status |= add_files(&dir, flags);
 
+	if (force_mode)
+		chmod_pathspec(&pathspec, force_mode);
 	unplug_bulk_checkin();
 
 finish:
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8672d07..a83c78f 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, 0);
+			add_files_to_cache(NULL, NULL, 0);
 			/*
 			 * NEEDSWORK: carrying over local changes
 			 * when branches have different end-of-line
diff --git a/builtin/commit.c b/builtin/commit.c
index 77e3dc8..7a1ade0 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -387,7 +387,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, 0);
+		add_files_to_cache(also ? prefix : NULL, &pathspec, 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 44a4f76..eb6d6b8 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), 0)
-#define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags), 0)
+#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 chmod_cache_entry(ce, force_mode) chmod_index_entry(&the_index, (ce), (force_mode))
 #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))
@@ -582,8 +582,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
-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 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);
 extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, int force_mode);
 extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);
@@ -1820,7 +1820,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 force_mode);
+int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int flags);
 
 /* diff.c */
 extern int diff_auto_refresh_index;
diff --git a/read-cache.c b/read-cache.c
index 367be57..b92d72e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -627,7 +627,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 force_mode)
+int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags)
 {
 	int size, namelen, was_same;
 	mode_t st_mode = st->st_mode;
@@ -656,11 +656,10 @@ 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_mode)
-		ce->ce_mode = create_ce_mode(force_mode);
-	else if (trust_executable_bit && has_symlinks)
+
+	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.
 		 */
@@ -719,13 +718,12 @@ 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 force_mode)
+int add_file_to_index(struct index_state *istate, const char *path, int flags)
 {
 	struct stat st;
 	if (lstat(path, &st))
 		die_errno("unable to stat '%s'", path);
-	return add_to_index(istate, path, &st, flags, force_mode);
+	return add_to_index(istate, path, &st, flags);
 }
 
 struct cache_entry *make_cache_entry(unsigned int mode,
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 2978cb9..62c7f71 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -349,4 +349,25 @@ test_expect_success POSIXPERM,SYMLINKS 'git add --chmod=+x with symlinks' '
 	test_mode_in_index 100755 foo2
 '
 
+test_expect_success 'git add --chmod=[+-]x changes index with already added file' '
+	echo foo >foo3 &&
+	git add foo3 &&
+	git add --chmod=+x foo3 &&
+	test_mode_in_index 100755 foo3 &&
+	echo foo >xfoo3 &&
+	chmod 755 xfoo3 &&
+	git add xfoo3 &&
+	git add --chmod=-x xfoo3 &&
+	test_mode_in_index 100644 xfoo3
+'
+
+test_expect_success 'file status is changed after git add --chmod=+x' '
+	echo "AM foo4" >expected &&
+	echo foo >foo4 &&
+	git add foo4 &&
+	git add --chmod=+x foo4 &&
+	git status -s foo4 >actual &&
+	test_cmp expected actual
+'
+
 test_done
-- 
2.10.0.304.gf2ff484

[PATCH v2 0/4] git add --chmod: always change the file

[Thomas Gummerer]

Changes since v1:
Added missing x in the documentation.

The changes since v1 are only minor, but as it's been a week, this is
as much a ping as a fixup of my error :)

Thomas Gummerer (4):
  add: document the chmod option
  update-index: use the same structure for chmod as add
  read-cache: introduce chmod_index_entry
  add: modify already added files when --chmod is given

 Documentation/git-add.txt |  7 +++++-
 builtin/add.c             | 36 +++++++++++++++++++++----------
 builtin/checkout.c        |  2 +-
 builtin/commit.c          |  2 +-
 builtin/update-index.c    | 55 ++++++++++++++++++-----------------------------
 cache.h                   | 12 ++++++-----
 read-cache.c              | 33 +++++++++++++++++++++-------
 t/t3700-add.sh            | 21 ++++++++++++++++++
 8 files changed, 107 insertions(+), 61 deletions(-)

-- 
2.10.0.304.gf2ff484

[PATCH v2 1/4] add: document the chmod option

[Thomas Gummerer]

The git add --chmod option was introduced in 4e55ed3 ("add: add
--chmod=+x / --chmod=-x options", 2016-05-31), but was never
documented.  Document the feature.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 Documentation/git-add.txt | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 6a96a66..7ed63dc 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 'git add' [--verbose | -v] [--dry-run | -n] [--force | -f] [--interactive | -i] [--patch | -p]
 	  [--edit | -e] [--[no-]all | --[no-]ignore-removal | [--update | -u]]
 	  [--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing]
-	  [--] [<pathspec>...]
+	  [--chmod=(+|-)x] [--] [<pathspec>...]
 
 DESCRIPTION
 -----------
@@ -165,6 +165,11 @@ for "git add --no-all <pathspec>...", i.e. ignored removed files.
 	be ignored, no matter if they are already present in the work
 	tree or not.
 
+--chmod=(+|-)x::
+	Override the executable bit of the added files.  The executable
+	bit is only changed in the index, the files on disk are left
+	unchanged.
+
 \--::
 	This option can be used to separate command-line options from
 	the list of files, (useful when filenames might be mistaken
-- 
2.10.0.304.gf2ff484

[PATCH v2 2/4] update-index: use the same structure for chmod as add

[Thomas Gummerer]

While the chmod options for update-index and the add have the same
functionality, they are using different ways to parse and handle the
option internally.  Unify these modes in order to make further
refactoring simpler.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/update-index.c | 49 +++++++++++++++++++++----------------------------
 1 file changed, 21 insertions(+), 28 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index ba04b19..85a57db 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -419,11 +419,12 @@ static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,
 	return 0;
 }
 
-static void chmod_path(int flip, const char *path)
+static void chmod_path(int force_mode, const char *path)
 {
 	int pos;
 	struct cache_entry *ce;
 	unsigned int mode;
+	char flip = force_mode == 0777 ? '+' : '-';
 
 	pos = cache_name_pos(path, strlen(path));
 	if (pos < 0)
@@ -432,17 +433,11 @@ static void chmod_path(int flip, const char *path)
 	mode = ce->ce_mode;
 	if (!S_ISREG(mode))
 		goto fail;
-	switch (flip) {
-	case '+':
-		ce->ce_mode |= 0111; break;
-	case '-':
-		ce->ce_mode &= ~0111; break;
-	default:
-		goto fail;
-	}
+	ce->ce_mode = create_ce_mode(force_mode);
 	cache_tree_invalidate_path(&the_index, path);
 	ce->ce_flags |= CE_UPDATE_IN_BASE;
 	active_cache_changed |= CE_ENTRY_CHANGED;
+
 	report("chmod %cx '%s'", flip, path);
 	return;
  fail:
@@ -788,16 +783,6 @@ static int really_refresh_callback(const struct option *opt,
 	return refresh(opt->value, REFRESH_REALLY);
 }
 
-static int chmod_callback(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 int resolve_undo_clear_callback(const struct option *opt,
 				const char *arg, int unset)
 {
@@ -917,7 +902,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	int read_from_stdin = 0;
 	int prefix_length = prefix ? strlen(prefix) : 0;
 	int preferred_index_format = 0;
-	char set_executable_bit = 0;
+	char *chmod_arg = 0;
+	int force_mode = 0;
 	struct refresh_params refresh_args = {0, &has_errors};
 	int lock_error = 0;
 	int split_index = -1;
@@ -955,10 +941,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 			PARSE_OPT_NOARG | /* disallow --cacheinfo=<mode> form */
 			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
 			(parse_opt_cb *) cacheinfo_callback},
-		{OPTION_CALLBACK, 0, "chmod", &set_executable_bit, N_("(+/-)x"),
-			N_("override the executable bit of the listed files"),
-			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
-			chmod_callback},
+		OPT_STRING( 0, "chmod", &chmod_arg, N_("(+/-)x"),
+			N_("override the executable bit of the listed files")),
 		{OPTION_SET_INT, 0, "assume-unchanged", &mark_valid_only, NULL,
 			N_("mark files as \"not changing\""),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, MARK_FLAG},
@@ -1018,6 +1002,15 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(update_index_usage, options);
 
+	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(_("option 'chmod' expects \"+x\" or \"-x\""));
+
 	git_config(git_default_config, NULL);
 
 	/* We can't free this memory, it becomes part of a linked list parsed atexit() */
@@ -1055,8 +1048,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 			setup_work_tree();
 			p = prefix_path(prefix, prefix_length, path);
 			update_one(p);
-			if (set_executable_bit)
-				chmod_path(set_executable_bit, p);
+			if (force_mode)
+				chmod_path(force_mode, p);
 			free(p);
 			ctx.argc--;
 			ctx.argv++;
@@ -1100,8 +1093,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 			}
 			p = prefix_path(prefix, prefix_length, buf.buf);
 			update_one(p);
-			if (set_executable_bit)
-				chmod_path(set_executable_bit, p);
+			if (force_mode)
+				chmod_path(force_mode, p);
 			free(p);
 		}
 		strbuf_release(&unquoted);
-- 
2.10.0.304.gf2ff484

Re: [PATCH v2 2/4] update-index: use the same structure for chmod as add

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> @@ -955,10 +941,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>  			PARSE_OPT_NOARG | /* disallow --cacheinfo=<mode> form */
>  			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
>  			(parse_opt_cb *) cacheinfo_callback},
> -		{OPTION_CALLBACK, 0, "chmod", &set_executable_bit, N_("(+/-)x"),
> -			N_("override the executable bit of the listed files"),
> -			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
> -			chmod_callback},
> +		OPT_STRING( 0, "chmod", &chmod_arg, N_("(+/-)x"),
> +			N_("override the executable bit of the listed files")),
>  		{OPTION_SET_INT, 0, "assume-unchanged", &mark_valid_only, NULL,
>  			N_("mark files as \"not changing\""),
>  			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, MARK_FLAG},
> @@ -1018,6 +1002,15 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>  	if (argc == 2 && !strcmp(argv[1], "-h"))
>  		usage_with_options(update_index_usage, options);
>  
> +	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(_("option 'chmod' expects \"+x\" or \"-x\""));
> +

I am afraid that this changes the behaviour drastically.

"git update-index" is an oddball command that takes options and then
processes them immediately, exactly because it was designed to take

	git update-index --chmod=-x A --chmod=+x B --add C

and say things like "A and B are not in the index and you are
attempting to add them before giving me --add option".

	git update-index --add --chmod=-x A --chmod=+x B C

is expected to add A as non-executable, and B and C as executable.
Many exotic parse-options callback mechanisms used in this command
were invented exactly to support its quirky way of not doing "get a
list of options and use the last one".  And this patch breaks it for
only one option without changing the others.

If we were willing to take such a big backward compatiblity hit in
the upcoming release (which I personally won't be affected, but old
scripts by others need to be audited and adjusted, which I won't
volunteer to do myself), we should make such a change consistently,
e.g. "git update-index A --add --remove B" should no longer error
out when it sees A and it is not yet in the index because "--add"
hasn't been given yet, or A is in the index but is missing from the
working tree because "--remove" hasn't been given yet.  Then it may
be more justifiable if "update-index --chmod=-x A --chmod=+x B"
added A as an executable.  With the current form of this patch, it
is not.

Can we do this "fix" without this change?

Re: [PATCH v2 2/4] update-index: use the same structure for chmod as add

[Thomas Gummerer]

On 09/11, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
> > @@ -955,10 +941,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
> >  			PARSE_OPT_NOARG | /* disallow --cacheinfo=<mode> form */
> >  			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
> >  			(parse_opt_cb *) cacheinfo_callback},
> > -		{OPTION_CALLBACK, 0, "chmod", &set_executable_bit, N_("(+/-)x"),
> > -			N_("override the executable bit of the listed files"),
> > -			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
> > -			chmod_callback},
> > +		OPT_STRING( 0, "chmod", &chmod_arg, N_("(+/-)x"),
> > +			N_("override the executable bit of the listed files")),
> >  		{OPTION_SET_INT, 0, "assume-unchanged", &mark_valid_only, NULL,
> >  			N_("mark files as \"not changing\""),
> >  			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, MARK_FLAG},
> > @@ -1018,6 +1002,15 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
> >  	if (argc == 2 && !strcmp(argv[1], "-h"))
> >  		usage_with_options(update_index_usage, options);
> >  
> > +	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(_("option 'chmod' expects \"+x\" or \"-x\""));
> > +
> 
> I am afraid that this changes the behaviour drastically.
> 
> "git update-index" is an oddball command that takes options and then
> processes them immediately, exactly because it was designed to take
> 
> 	git update-index --chmod=-x A --chmod=+x B --add C
> 
> and say things like "A and B are not in the index and you are
> attempting to add them before giving me --add option".
> 
> 	git update-index --add --chmod=-x A --chmod=+x B C
> 
> is expected to add A as non-executable, and B and C as executable.
> Many exotic parse-options callback mechanisms used in this command
> were invented exactly to support its quirky way of not doing "get a
> list of options and use the last one".  And this patch breaks it for
> only one option without changing the others.
> 
> If we were willing to take such a big backward compatiblity hit in
> the upcoming release (which I personally won't be affected, but old
> scripts by others need to be audited and adjusted, which I won't
> volunteer to do myself), we should make such a change consistently,
> e.g. "git update-index A --add --remove B" should no longer error
> out when it sees A and it is not yet in the index because "--add"
> hasn't been given yet, or A is in the index but is missing from the
> working tree because "--remove" hasn't been given yet.  Then it may
> be more justifiable if "update-index --chmod=-x A --chmod=+x B"
> added A as an executable.  With the current form of this patch, it
> is not.

Thanks for the explanation, this change in backwards compatibility is
certainly not what I intended, but rather something I missed while
cooking up this patch.

> Can we do this "fix" without this change?

Yeah, let me see what I can come up with in a re-roll.

Thanks,
Thomas

[PATCH v2 3/4] read-cache: introduce chmod_index_entry

[Thomas Gummerer]

As there are chmod options for both add and update-index, introduce a
new chmod_index_entry function to do the work.  Use it in update-index,
while it will be used in add in the next patch.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/update-index.c |  8 +-------
 cache.h                |  2 ++
 read-cache.c           | 19 +++++++++++++++++++
 3 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 85a57db..1569c81 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -423,20 +423,14 @@ static void chmod_path(int force_mode, const char *path)
 {
 	int pos;
 	struct cache_entry *ce;
-	unsigned int mode;
 	char flip = force_mode == 0777 ? '+' : '-';
 
 	pos = cache_name_pos(path, strlen(path));
 	if (pos < 0)
 		goto fail;
 	ce = active_cache[pos];
-	mode = ce->ce_mode;
-	if (!S_ISREG(mode))
+	if (chmod_cache_entry(ce, force_mode) < 0)
 		goto fail;
-	ce->ce_mode = create_ce_mode(force_mode);
-	cache_tree_invalidate_path(&the_index, path);
-	ce->ce_flags |= CE_UPDATE_IN_BASE;
-	active_cache_changed |= CE_ENTRY_CHANGED;
 
 	report("chmod %cx '%s'", flip, path);
 	return;
diff --git a/cache.h b/cache.h
index b780a91..44a4f76 100644
--- a/cache.h
+++ b/cache.h
@@ -369,6 +369,7 @@ extern void free_name_hash(struct index_state *istate);
 #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), 0)
 #define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags), 0)
+#define chmod_cache_entry(ce, force_mode) chmod_index_entry(&the_index, (ce), (force_mode))
 #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))
@@ -584,6 +585,7 @@ extern int remove_file_from_index(struct index_state *, const char *path);
 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 chmod_index_entry(struct index_state *, struct cache_entry *ce, int force_mode);
 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);
 extern int index_name_is_other(const struct index_state *, const char *, int);
diff --git a/read-cache.c b/read-cache.c
index 491e52d..367be57 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -756,6 +756,25 @@ struct cache_entry *make_cache_entry(unsigned int mode,
 	return ret;
 }
 
+/*
+ * Change the mode of an index entry to force_mode, where force_mode can
+ * either be 0777 or 0666.
+ * Returns -1 if the chmod for the particular cache entry failed (if it's
+ * not a regular file), 0 otherwise.
+ */
+int chmod_index_entry(struct index_state *istate, struct cache_entry *ce,
+		       int force_mode)
+{
+	if (!S_ISREG(ce->ce_mode))
+		return -1;
+	ce->ce_mode = create_ce_mode(force_mode);
+	cache_tree_invalidate_path(istate, ce->name);
+	ce->ce_flags |= CE_UPDATE_IN_BASE;
+	istate->cache_changed |= CE_ENTRY_CHANGED;
+
+	return 0;
+}
+
 int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)
 {
 	int len = ce_namelen(a);
-- 
2.10.0.304.gf2ff484

[PATCH v2 4/4] add: modify already added files when --chmod is given

[Thomas Gummerer]

When the chmod option was added to git add, it was hooked up to the diff
machinery, meaning that it only works when the version in the index
differs from the version on disk.

As the option was supposed to mirror the chmod option in update-index,
which always changes the mode in the index, regardless of the status of
the file, make sure the option behaves the same way in git add.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/add.c      | 36 +++++++++++++++++++++++++-----------
 builtin/checkout.c |  2 +-
 builtin/commit.c   |  2 +-
 cache.h            | 10 +++++-----
 read-cache.c       | 14 ++++++--------
 t/t3700-add.sh     | 21 +++++++++++++++++++++
 6 files changed, 59 insertions(+), 26 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index b1dddb4..892198a 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -26,10 +26,25 @@ static int patch_interactive, add_interactive, edit_interactive;
 static int take_worktree_changes;
 
 struct update_callback_data {
-	int flags, force_mode;
+	int flags;
 	int add_errors;
 };
 
+static void chmod_pathspec(struct pathspec *pathspec, int force_mode)
+{
+	int i;
+	
+	for (i = 0; i < active_nr; i++) {
+		struct cache_entry *ce = active_cache[i];
+
+		if (pathspec && !ce_path_match(ce, pathspec, NULL))
+			continue;
+
+		if (chmod_cache_entry(ce, force_mode) < 0)
+			fprintf(stderr, "cannot chmod '%s'", ce->name);
+	}
+}
+
 static int fix_unmerged_status(struct diff_filepair *p,
 			       struct update_callback_data *data)
 {
@@ -65,8 +80,7 @@ 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, data->force_mode)) {
+			if (add_file_to_index(&the_index, path,	data->flags)) {
 				if (!(data->flags & ADD_CACHE_IGNORE_ERRORS))
 					die(_("updating files failed"));
 				data->add_errors++;
@@ -84,15 +98,14 @@ static void update_callback(struct diff_queue_struct *q,
 	}
 }
 
-int add_files_to_cache(const char *prefix, const struct pathspec *pathspec,
-	int flags, int force_mode)
+int add_files_to_cache(const char *prefix,
+		       const struct pathspec *pathspec, int flags)
 {
 	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);
@@ -281,7 +294,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, int force_mode)
+static int add_files(struct dir_struct *dir, int flags)
 {
 	int i, exit_status = 0;
 
@@ -294,8 +307,7 @@ static int add_files(struct dir_struct *dir, int flags, int force_mode)
 	}
 
 	for (i = 0; i < dir->nr; i++)
-		if (add_file_to_index(&the_index, dir->entries[i]->name,
-				flags, force_mode)) {
+		if (add_file_to_index(&the_index, dir->entries[i]->name, flags)) {
 			if (!ignore_add_errors)
 				die(_("adding files failed"));
 			exit_status = 1;
@@ -441,11 +453,13 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	plug_bulk_checkin();
 
-	exit_status |= add_files_to_cache(prefix, &pathspec, flags, force_mode);
+	exit_status |= add_files_to_cache(prefix, &pathspec, flags);
 
 	if (add_new_files)
-		exit_status |= add_files(&dir, flags, force_mode);
+		exit_status |= add_files(&dir, flags);
 
+	if (force_mode)
+		chmod_pathspec(&pathspec, force_mode);
 	unplug_bulk_checkin();
 
 finish:
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8672d07..a83c78f 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, 0);
+			add_files_to_cache(NULL, NULL, 0);
 			/*
 			 * NEEDSWORK: carrying over local changes
 			 * when branches have different end-of-line
diff --git a/builtin/commit.c b/builtin/commit.c
index 77e3dc8..7a1ade0 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -387,7 +387,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, 0);
+		add_files_to_cache(also ? prefix : NULL, &pathspec, 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 44a4f76..eb6d6b8 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), 0)
-#define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags), 0)
+#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 chmod_cache_entry(ce, force_mode) chmod_index_entry(&the_index, (ce), (force_mode))
 #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))
@@ -582,8 +582,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
-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 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);
 extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, int force_mode);
 extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);
@@ -1820,7 +1820,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 force_mode);
+int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int flags);
 
 /* diff.c */
 extern int diff_auto_refresh_index;
diff --git a/read-cache.c b/read-cache.c
index 367be57..b92d72e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -627,7 +627,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 force_mode)
+int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags)
 {
 	int size, namelen, was_same;
 	mode_t st_mode = st->st_mode;
@@ -656,11 +656,10 @@ 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_mode)
-		ce->ce_mode = create_ce_mode(force_mode);
-	else if (trust_executable_bit && has_symlinks)
+
+	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.
 		 */
@@ -719,13 +718,12 @@ 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 force_mode)
+int add_file_to_index(struct index_state *istate, const char *path, int flags)
 {
 	struct stat st;
 	if (lstat(path, &st))
 		die_errno("unable to stat '%s'", path);
-	return add_to_index(istate, path, &st, flags, force_mode);
+	return add_to_index(istate, path, &st, flags);
 }
 
 struct cache_entry *make_cache_entry(unsigned int mode,
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 2978cb9..62c7f71 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -349,4 +349,25 @@ test_expect_success POSIXPERM,SYMLINKS 'git add --chmod=+x with symlinks' '
 	test_mode_in_index 100755 foo2
 '
 
+test_expect_success 'git add --chmod=[+-]x changes index with already added file' '
+	echo foo >foo3 &&
+	git add foo3 &&
+	git add --chmod=+x foo3 &&
+	test_mode_in_index 100755 foo3 &&
+	echo foo >xfoo3 &&
+	chmod 755 xfoo3 &&
+	git add xfoo3 &&
+	git add --chmod=-x xfoo3 &&
+	test_mode_in_index 100644 xfoo3
+'
+
+test_expect_success 'file status is changed after git add --chmod=+x' '
+	echo "AM foo4" >expected &&
+	echo foo >foo4 &&
+	git add foo4 &&
+	git add --chmod=+x foo4 &&
+	git status -s foo4 >actual &&
+	test_cmp expected actual
+'
+
 test_done
-- 
2.10.0.304.gf2ff484

[PATCH v3 0/4] git add --chmod: always change the file

[Thomas Gummerer]

Thanks to Junio on setting me straight on the change of behaviour in
the previous round.

This round includes a similar change, which does however not change
the behaviour of update-index with repeated arguments.  I still think
the unification of the way git add and git update-index handle chmod
is useful, when we can keep the behaviour with multiple arguments in
update-index the same.

Thomas Gummerer (4):
  add: document the chmod option
  update-index: use the same structure for chmod as add
  read-cache: introduce chmod_index_entry
  add: modify already added files when --chmod is given

 Documentation/git-add.txt     |  7 ++++++-
 builtin/add.c                 | 36 +++++++++++++++++++++++-----------
 builtin/checkout.c            |  2 +-
 builtin/commit.c              |  2 +-
 builtin/update-index.c        | 45 ++++++++++++++++++-------------------------
 cache.h                       | 12 +++++++-----
 read-cache.c                  | 33 +++++++++++++++++++++++--------
 t/t2107-update-index-basic.sh | 13 +++++++++++++
 t/t3700-add.sh                | 21 ++++++++++++++++++++
 9 files changed, 118 insertions(+), 53 deletions(-)

-- 
2.10.0.304.gf2ff484

[PATCH v3 1/4] add: document the chmod option

[Thomas Gummerer]

The git add --chmod option was introduced in 4e55ed3 ("add: add
--chmod=+x / --chmod=-x options", 2016-05-31), but was never
documented.  Document the feature.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 Documentation/git-add.txt | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 6a96a66..7ed63dc 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 'git add' [--verbose | -v] [--dry-run | -n] [--force | -f] [--interactive | -i] [--patch | -p]
 	  [--edit | -e] [--[no-]all | --[no-]ignore-removal | [--update | -u]]
 	  [--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing]
-	  [--] [<pathspec>...]
+	  [--chmod=(+|-)x] [--] [<pathspec>...]
 
 DESCRIPTION
 -----------
@@ -165,6 +165,11 @@ for "git add --no-all <pathspec>...", i.e. ignored removed files.
 	be ignored, no matter if they are already present in the work
 	tree or not.
 
+--chmod=(+|-)x::
+	Override the executable bit of the added files.  The executable
+	bit is only changed in the index, the files on disk are left
+	unchanged.
+
 \--::
 	This option can be used to separate command-line options from
 	the list of files, (useful when filenames might be mistaken
-- 
2.10.0.304.gf2ff484

[PATCH v3 2/4] update-index: use the same structure for chmod as add

[Thomas Gummerer]

While the chmod options for update-index and the add have the same
functionality, they are using different ways to parse and handle the
option internally.  Unify these modes in order to make further
refactoring simpler.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/update-index.c        | 39 +++++++++++++++++++--------------------
 t/t2107-update-index-basic.sh | 13 +++++++++++++
 2 files changed, 32 insertions(+), 20 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index ba04b19..6d6cddd 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -419,11 +419,12 @@ static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,
 	return 0;
 }
 
-static void chmod_path(int flip, const char *path)
+static void chmod_path(int force_mode, const char *path)
 {
 	int pos;
 	struct cache_entry *ce;
 	unsigned int mode;
+	char flip = force_mode == 0777 ? '+' : '-';
 
 	pos = cache_name_pos(path, strlen(path));
 	if (pos < 0)
@@ -432,17 +433,11 @@ static void chmod_path(int flip, const char *path)
 	mode = ce->ce_mode;
 	if (!S_ISREG(mode))
 		goto fail;
-	switch (flip) {
-	case '+':
-		ce->ce_mode |= 0111; break;
-	case '-':
-		ce->ce_mode &= ~0111; break;
-	default:
-		goto fail;
-	}
+	ce->ce_mode = create_ce_mode(force_mode);
 	cache_tree_invalidate_path(&the_index, path);
 	ce->ce_flags |= CE_UPDATE_IN_BASE;
 	active_cache_changed |= CE_ENTRY_CHANGED;
+
 	report("chmod %cx '%s'", flip, path);
 	return;
  fail:
@@ -789,12 +784,16 @@ static int really_refresh_callback(const struct option *opt,
 }
 
 static int chmod_callback(const struct option *opt,
-				const char *arg, int unset)
+			  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];
+	int *force_mode = opt->value;
+	if (!strcmp(arg, "-x"))
+		*force_mode = 0666;
+	else if (!strcmp(arg, "+x"))
+		*force_mode = 0777;
+	else
+		die(_("option 'chmod' expects \"+x\" or \"-x\""));
+
 	return 0;
 }
 
@@ -917,7 +916,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	int read_from_stdin = 0;
 	int prefix_length = prefix ? strlen(prefix) : 0;
 	int preferred_index_format = 0;
-	char set_executable_bit = 0;
+	int force_mode = 0;
 	struct refresh_params refresh_args = {0, &has_errors};
 	int lock_error = 0;
 	int split_index = -1;
@@ -955,7 +954,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 			PARSE_OPT_NOARG | /* disallow --cacheinfo=<mode> form */
 			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
 			(parse_opt_cb *) cacheinfo_callback},
-		{OPTION_CALLBACK, 0, "chmod", &set_executable_bit, N_("(+/-)x"),
+		{OPTION_CALLBACK, 0, "chmod", &force_mode, N_("(+/-)x"),
 			N_("override the executable bit of the listed files"),
 			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
 			chmod_callback},
@@ -1055,8 +1054,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 			setup_work_tree();
 			p = prefix_path(prefix, prefix_length, path);
 			update_one(p);
-			if (set_executable_bit)
-				chmod_path(set_executable_bit, p);
+			if (force_mode)
+				chmod_path(force_mode, p);
 			free(p);
 			ctx.argc--;
 			ctx.argv++;
@@ -1100,8 +1099,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 			}
 			p = prefix_path(prefix, prefix_length, buf.buf);
 			update_one(p);
-			if (set_executable_bit)
-				chmod_path(set_executable_bit, p);
+			if (force_mode)
+				chmod_path(force_mode, p);
 			free(p);
 		}
 		strbuf_release(&unquoted);
diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh
index dfe02f4..32ac6e0 100755
--- a/t/t2107-update-index-basic.sh
+++ b/t/t2107-update-index-basic.sh
@@ -80,4 +80,17 @@ test_expect_success '.lock files cleaned up' '
 	)
 '
 
+test_expect_success '--chmod=+x and chmod=-x in the same argument list' '
+	>A &&
+	>B &&
+	git add A B &&
+	git update-index --chmod=+x A --chmod=-x B &&
+	cat >expect <<-\EOF &&
+	100755 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	A
+	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	B
+	EOF
+	git ls-files --stage A B >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.10.0.304.gf2ff484

Re: [PATCH v3 2/4] update-index: use the same structure for chmod as add

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> +	char flip = force_mode == 0777 ? '+' : '-';
>  
>  	pos = cache_name_pos(path, strlen(path));
>  	if (pos < 0)
> @@ -432,17 +433,11 @@ static void chmod_path(int flip, const char *path)
>  	mode = ce->ce_mode;
>  	if (!S_ISREG(mode))
>  		goto fail;
> -	switch (flip) {
> -	case '+':
> -		ce->ce_mode |= 0111; break;
> -	case '-':
> -		ce->ce_mode &= ~0111; break;
> -	default:
> -		goto fail;
> -	}
> +	ce->ce_mode = create_ce_mode(force_mode);

create_ce_mode() is supposed to take the st_mode taken from a
"struct stat", but here force_mode is 0777 or something else.  The
above to feed only 0777 or 0666 may happen to work with the way how
create_ce_mode() is implemented now, but it is a time-bomb waiting
to go off.

Instead of using force_mode that is unsigned, keep the 'flip' as
argument, and do:

	if (!S_ISREG(mode))
        	goto fail;
+	if (flip == '+')
+		mode |= 0111;
+	else
+		mode &= ~0111;
	ce->ce_mode = create_ce_mode(mode);

perhaps, as you do not need and are not using the full 9 bits in
force_mode anyway.

That would mean chmod_index_entry() introduced in 3/4 and its user
in 4/4 need to be updated to pass '+' or '-' down the callchain, but
I think that is a good change for the same reason.  We do not allow
you to set full 9 bits; let's not pretend as if we do so by passing
just a single bit '+' or '-' around.

I think 3/4 needs to be fixed where it calls create_ce_mode() with
only the 0666/0777 in chmod_index_entry() anyway, regardless of the
above suggested change.

> diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh
> index dfe02f4..32ac6e0 100755
> --- a/t/t2107-update-index-basic.sh
> +++ b/t/t2107-update-index-basic.sh
> @@ -80,4 +80,17 @@ test_expect_success '.lock files cleaned up' '
>  	)
>  '
>  
> +test_expect_success '--chmod=+x and chmod=-x in the same argument list' '
> +	>A &&
> +	>B &&
> +	git add A B &&
> +	git update-index --chmod=+x A --chmod=-x B &&
> +	cat >expect <<-\EOF &&
> +	100755 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	A
> +	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	B
> +	EOF
> +	git ls-files --stage A B >actual &&
> +	test_cmp expect actual
> +'

Thanks for adding this test.  We may want to cross the b/c bridge in
a later version bump, but until then it is good to make sure we know
what the currently expected behaviour is.

[PATCH v3 3/4] read-cache: introduce chmod_index_entry

[Thomas Gummerer]

As there are chmod options for both add and update-index, introduce a
new chmod_index_entry function to do the work.  Use it in update-index,
while it will be used in add in the next patch.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/update-index.c |  8 +-------
 cache.h                |  2 ++
 read-cache.c           | 19 +++++++++++++++++++
 3 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 6d6cddd..809ce97 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -423,20 +423,14 @@ static void chmod_path(int force_mode, const char *path)
 {
 	int pos;
 	struct cache_entry *ce;
-	unsigned int mode;
 	char flip = force_mode == 0777 ? '+' : '-';
 
 	pos = cache_name_pos(path, strlen(path));
 	if (pos < 0)
 		goto fail;
 	ce = active_cache[pos];
-	mode = ce->ce_mode;
-	if (!S_ISREG(mode))
+	if (chmod_cache_entry(ce, force_mode) < 0)
 		goto fail;
-	ce->ce_mode = create_ce_mode(force_mode);
-	cache_tree_invalidate_path(&the_index, path);
-	ce->ce_flags |= CE_UPDATE_IN_BASE;
-	active_cache_changed |= CE_ENTRY_CHANGED;
 
 	report("chmod %cx '%s'", flip, path);
 	return;
diff --git a/cache.h b/cache.h
index b780a91..44a4f76 100644
--- a/cache.h
+++ b/cache.h
@@ -369,6 +369,7 @@ extern void free_name_hash(struct index_state *istate);
 #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), 0)
 #define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags), 0)
+#define chmod_cache_entry(ce, force_mode) chmod_index_entry(&the_index, (ce), (force_mode))
 #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))
@@ -584,6 +585,7 @@ extern int remove_file_from_index(struct index_state *, const char *path);
 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 chmod_index_entry(struct index_state *, struct cache_entry *ce, int force_mode);
 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);
 extern int index_name_is_other(const struct index_state *, const char *, int);
diff --git a/read-cache.c b/read-cache.c
index 491e52d..367be57 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -756,6 +756,25 @@ struct cache_entry *make_cache_entry(unsigned int mode,
 	return ret;
 }
 
+/*
+ * Change the mode of an index entry to force_mode, where force_mode can
+ * either be 0777 or 0666.
+ * Returns -1 if the chmod for the particular cache entry failed (if it's
+ * not a regular file), 0 otherwise.
+ */
+int chmod_index_entry(struct index_state *istate, struct cache_entry *ce,
+		       int force_mode)
+{
+	if (!S_ISREG(ce->ce_mode))
+		return -1;
+	ce->ce_mode = create_ce_mode(force_mode);
+	cache_tree_invalidate_path(istate, ce->name);
+	ce->ce_flags |= CE_UPDATE_IN_BASE;
+	istate->cache_changed |= CE_ENTRY_CHANGED;
+
+	return 0;
+}
+
 int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)
 {
 	int len = ce_namelen(a);
-- 
2.10.0.304.gf2ff484

[PATCH v3 4/4] add: modify already added files when --chmod is given

[Thomas Gummerer]

When the chmod option was added to git add, it was hooked up to the diff
machinery, meaning that it only works when the version in the index
differs from the version on disk.

As the option was supposed to mirror the chmod option in update-index,
which always changes the mode in the index, regardless of the status of
the file, make sure the option behaves the same way in git add.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/add.c      | 36 +++++++++++++++++++++++++-----------
 builtin/checkout.c |  2 +-
 builtin/commit.c   |  2 +-
 cache.h            | 10 +++++-----
 read-cache.c       | 14 ++++++--------
 t/t3700-add.sh     | 21 +++++++++++++++++++++
 6 files changed, 59 insertions(+), 26 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index b1dddb4..892198a 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -26,10 +26,25 @@ static int patch_interactive, add_interactive, edit_interactive;
 static int take_worktree_changes;
 
 struct update_callback_data {
-	int flags, force_mode;
+	int flags;
 	int add_errors;
 };
 
+static void chmod_pathspec(struct pathspec *pathspec, int force_mode)
+{
+	int i;
+	
+	for (i = 0; i < active_nr; i++) {
+		struct cache_entry *ce = active_cache[i];
+
+		if (pathspec && !ce_path_match(ce, pathspec, NULL))
+			continue;
+
+		if (chmod_cache_entry(ce, force_mode) < 0)
+			fprintf(stderr, "cannot chmod '%s'", ce->name);
+	}
+}
+
 static int fix_unmerged_status(struct diff_filepair *p,
 			       struct update_callback_data *data)
 {
@@ -65,8 +80,7 @@ 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, data->force_mode)) {
+			if (add_file_to_index(&the_index, path,	data->flags)) {
 				if (!(data->flags & ADD_CACHE_IGNORE_ERRORS))
 					die(_("updating files failed"));
 				data->add_errors++;
@@ -84,15 +98,14 @@ static void update_callback(struct diff_queue_struct *q,
 	}
 }
 
-int add_files_to_cache(const char *prefix, const struct pathspec *pathspec,
-	int flags, int force_mode)
+int add_files_to_cache(const char *prefix,
+		       const struct pathspec *pathspec, int flags)
 {
 	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);
@@ -281,7 +294,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, int force_mode)
+static int add_files(struct dir_struct *dir, int flags)
 {
 	int i, exit_status = 0;
 
@@ -294,8 +307,7 @@ static int add_files(struct dir_struct *dir, int flags, int force_mode)
 	}
 
 	for (i = 0; i < dir->nr; i++)
-		if (add_file_to_index(&the_index, dir->entries[i]->name,
-				flags, force_mode)) {
+		if (add_file_to_index(&the_index, dir->entries[i]->name, flags)) {
 			if (!ignore_add_errors)
 				die(_("adding files failed"));
 			exit_status = 1;
@@ -441,11 +453,13 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	plug_bulk_checkin();
 
-	exit_status |= add_files_to_cache(prefix, &pathspec, flags, force_mode);
+	exit_status |= add_files_to_cache(prefix, &pathspec, flags);
 
 	if (add_new_files)
-		exit_status |= add_files(&dir, flags, force_mode);
+		exit_status |= add_files(&dir, flags);
 
+	if (force_mode)
+		chmod_pathspec(&pathspec, force_mode);
 	unplug_bulk_checkin();
 
 finish:
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8672d07..a83c78f 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, 0);
+			add_files_to_cache(NULL, NULL, 0);
 			/*
 			 * NEEDSWORK: carrying over local changes
 			 * when branches have different end-of-line
diff --git a/builtin/commit.c b/builtin/commit.c
index 77e3dc8..7a1ade0 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -387,7 +387,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, 0);
+		add_files_to_cache(also ? prefix : NULL, &pathspec, 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 44a4f76..eb6d6b8 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), 0)
-#define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags), 0)
+#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 chmod_cache_entry(ce, force_mode) chmod_index_entry(&the_index, (ce), (force_mode))
 #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))
@@ -582,8 +582,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
-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 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);
 extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, int force_mode);
 extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);
@@ -1820,7 +1820,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 force_mode);
+int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int flags);
 
 /* diff.c */
 extern int diff_auto_refresh_index;
diff --git a/read-cache.c b/read-cache.c
index 367be57..b92d72e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -627,7 +627,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 force_mode)
+int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags)
 {
 	int size, namelen, was_same;
 	mode_t st_mode = st->st_mode;
@@ -656,11 +656,10 @@ 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_mode)
-		ce->ce_mode = create_ce_mode(force_mode);
-	else if (trust_executable_bit && has_symlinks)
+
+	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.
 		 */
@@ -719,13 +718,12 @@ 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 force_mode)
+int add_file_to_index(struct index_state *istate, const char *path, int flags)
 {
 	struct stat st;
 	if (lstat(path, &st))
 		die_errno("unable to stat '%s'", path);
-	return add_to_index(istate, path, &st, flags, force_mode);
+	return add_to_index(istate, path, &st, flags);
 }
 
 struct cache_entry *make_cache_entry(unsigned int mode,
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 2978cb9..62c7f71 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -349,4 +349,25 @@ test_expect_success POSIXPERM,SYMLINKS 'git add --chmod=+x with symlinks' '
 	test_mode_in_index 100755 foo2
 '
 
+test_expect_success 'git add --chmod=[+-]x changes index with already added file' '
+	echo foo >foo3 &&
+	git add foo3 &&
+	git add --chmod=+x foo3 &&
+	test_mode_in_index 100755 foo3 &&
+	echo foo >xfoo3 &&
+	chmod 755 xfoo3 &&
+	git add xfoo3 &&
+	git add --chmod=-x xfoo3 &&
+	test_mode_in_index 100644 xfoo3
+'
+
+test_expect_success 'file status is changed after git add --chmod=+x' '
+	echo "AM foo4" >expected &&
+	echo foo >foo4 &&
+	git add foo4 &&
+	git add --chmod=+x foo4 &&
+	git status -s foo4 >actual &&
+	test_cmp expected actual
+'
+
 test_done
-- 
2.10.0.304.gf2ff484

Re: [PATCH v3 4/4] add: modify already added files when --chmod is given

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> When the chmod option was added to git add, it was hooked up to the diff
> machinery, meaning that it only works when the version in the index
> differs from the version on disk.
>
> As the option was supposed to mirror the chmod option in update-index,
> which always changes the mode in the index, regardless of the status of
> the file, make sure the option behaves the same way in git add.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
>  builtin/add.c      | 36 +++++++++++++++++++++++++-----------
>  builtin/checkout.c |  2 +-
>  builtin/commit.c   |  2 +-
>  cache.h            | 10 +++++-----
>  read-cache.c       | 14 ++++++--------
>  t/t3700-add.sh     | 21 +++++++++++++++++++++
>  6 files changed, 59 insertions(+), 26 deletions(-)

The change looks quite large but this in essense reverts what Edward
did in the first round by hooking into "we add only modified files
here" and "we add new files here", both of which are made unnecessary,
because this adds the final "now we finished adding things, let's
fix modes of paths that match the pathspec".

Which makes sense.

> +static void chmod_pathspec(struct pathspec *pathspec, int force_mode)
> +{
> +	int i;
> +	
> +	for (i = 0; i < active_nr; i++) {
> +		struct cache_entry *ce = active_cache[i];
> +
> +		if (pathspec && !ce_path_match(ce, pathspec, NULL))
> +			continue;
> +
> +		if (chmod_cache_entry(ce, force_mode) < 0)
> +			fprintf(stderr, "cannot chmod '%s'", ce->name);
> +	}
> +}

If pathspec is NULL, this will chmod all paths in the index, which
is probably not very useful and quite risky operation at the same
time.

However ...

> +	if (force_mode)
> +		chmod_pathspec(&pathspec, force_mode);

... the caller never passes a NULL as pathspec.

In any case, I somehow expected to see

	if (force_mode && pathspec.nr)
        	chmod_pathspec(&pathspec, force_mode);

because it would make it very easy to see in the caller that

	git add --chmod=+x              ;# no pathspec
        cd subdir && git add --chmod=+x ;# no pathspec

will be a no-op, which is what we want, if I am not mistaken.  Of
course, if somebody really wants to drop executable bit from
everything, she can do

	git add --chmod=-x .

pretty easily.

Above three may want to be added as new tests.  The first two should
be a no-op, while the last one should drop executable bits from
everywhere.

Thanks.

[PATCH v4 0/4] git add --chmod: always change the file

[Thomas Gummerer]

Thanks Junio for the review of my last round.

Changes since then:
[1/4]: patch unchanged
[2/4]: Only adds a test now, and corrects the type of the argument of
       chmod_path, but leaves the rest of the patch unchanged.
[3/4]: chmod_index_entry now takes a char as argument which can either
       be + or -, and changes the mode based on that, instead of using
       the 0777 or 0666 mode that was passed in from the outside.
[4/4]: Adapted to the different behaviour of chmod_index_entry and
       added tests as suggested by Junio.

Thomas Gummerer (4):
  add: document the chmod option
  update-index: add test for chmod flags
  read-cache: introduce chmod_index_entry
  add: modify already added files when --chmod is given

 Documentation/git-add.txt     |  7 +++++-
 builtin/add.c                 | 47 ++++++++++++++++++++++++----------------
 builtin/checkout.c            |  2 +-
 builtin/commit.c              |  2 +-
 builtin/update-index.c        | 18 +++-------------
 cache.h                       | 12 ++++++-----
 read-cache.c                  | 43 ++++++++++++++++++++++++++++++-------
 t/t2107-update-index-basic.sh | 13 +++++++++++
 t/t3700-add.sh                | 50 +++++++++++++++++++++++++++++++++++++++++++
 9 files changed, 144 insertions(+), 50 deletions(-)

-- 
2.10.0.304.gf2ff484

[PATCH v4 1/4] add: document the chmod option

[Thomas Gummerer]

The git add --chmod option was introduced in 4e55ed3 ("add: add
--chmod=+x / --chmod=-x options", 2016-05-31), but was never
documented.  Document the feature.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 Documentation/git-add.txt | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 6a96a66..7ed63dc 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 'git add' [--verbose | -v] [--dry-run | -n] [--force | -f] [--interactive | -i] [--patch | -p]
 	  [--edit | -e] [--[no-]all | --[no-]ignore-removal | [--update | -u]]
 	  [--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing]
-	  [--] [<pathspec>...]
+	  [--chmod=(+|-)x] [--] [<pathspec>...]
 
 DESCRIPTION
 -----------
@@ -165,6 +165,11 @@ for "git add --no-all <pathspec>...", i.e. ignored removed files.
 	be ignored, no matter if they are already present in the work
 	tree or not.
 
+--chmod=(+|-)x::
+	Override the executable bit of the added files.  The executable
+	bit is only changed in the index, the files on disk are left
+	unchanged.
+
 \--::
 	This option can be used to separate command-line options from
 	the list of files, (useful when filenames might be mistaken
-- 
2.10.0.304.gf2ff484

[PATCH v4 2/4] update-index: add test for chmod flags

[Thomas Gummerer]

Currently there is no test checking the expected behaviour when multiple
chmod flags with different arguments are passed.  As argument handling
is not in line with other git commands it's easy to miss and
accidentally change the current behaviour.

While there, fix the argument type of chmod_path, which takes an int,
but had a char passed in.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/update-index.c        |  2 +-
 t/t2107-update-index-basic.sh | 13 +++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index ba04b19..bbdf0d9 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -419,7 +419,7 @@ static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,
 	return 0;
 }
 
-static void chmod_path(int flip, const char *path)
+static void chmod_path(char flip, const char *path)
 {
 	int pos;
 	struct cache_entry *ce;
diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh
index dfe02f4..32ac6e0 100755
--- a/t/t2107-update-index-basic.sh
+++ b/t/t2107-update-index-basic.sh
@@ -80,4 +80,17 @@ test_expect_success '.lock files cleaned up' '
 	)
 '
 
+test_expect_success '--chmod=+x and chmod=-x in the same argument list' '
+	>A &&
+	>B &&
+	git add A B &&
+	git update-index --chmod=+x A --chmod=-x B &&
+	cat >expect <<-\EOF &&
+	100755 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	A
+	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	B
+	EOF
+	git ls-files --stage A B >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.10.0.304.gf2ff484

[PATCH v4 3/4] read-cache: introduce chmod_index_entry

[Thomas Gummerer]

As there are chmod options for both add and update-index, introduce a
new chmod_index_entry function to do the work.  Use it in update-index,
while it will be used in add in the next patch.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/update-index.c | 16 ++--------------
 cache.h                |  2 ++
 read-cache.c           | 29 +++++++++++++++++++++++++++++
 3 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index bbdf0d9..9e9e040 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -423,26 +423,14 @@ static void chmod_path(char flip, const char *path)
 {
 	int pos;
 	struct cache_entry *ce;
-	unsigned int mode;
 
 	pos = cache_name_pos(path, strlen(path));
 	if (pos < 0)
 		goto fail;
 	ce = active_cache[pos];
-	mode = ce->ce_mode;
-	if (!S_ISREG(mode))
-		goto fail;
-	switch (flip) {
-	case '+':
-		ce->ce_mode |= 0111; break;
-	case '-':
-		ce->ce_mode &= ~0111; break;
-	default:
+	if (chmod_cache_entry(ce, flip) < 0)
 		goto fail;
-	}
-	cache_tree_invalidate_path(&the_index, path);
-	ce->ce_flags |= CE_UPDATE_IN_BASE;
-	active_cache_changed |= CE_ENTRY_CHANGED;
+
 	report("chmod %cx '%s'", flip, path);
 	return;
  fail:
diff --git a/cache.h b/cache.h
index 6738050..35c8d1c 100644
--- a/cache.h
+++ b/cache.h
@@ -369,6 +369,7 @@ extern void free_name_hash(struct index_state *istate);
 #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), 0)
 #define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags), 0)
+#define chmod_cache_entry(ce, flip) chmod_index_entry(&the_index, (ce), (flip))
 #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))
@@ -584,6 +585,7 @@ extern int remove_file_from_index(struct index_state *, const char *path);
 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 chmod_index_entry(struct index_state *, struct cache_entry *ce, char flip);
 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);
 extern int index_name_is_other(const struct index_state *, const char *, int);
diff --git a/read-cache.c b/read-cache.c
index 491e52d..8924f2e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -756,6 +756,35 @@ struct cache_entry *make_cache_entry(unsigned int mode,
 	return ret;
 }
 
+/*
+ * Chmod an index entry with either +x or -x.
+ *
+ * Returns -1 if the chmod for the particular cache entry failed (if it's
+ * not a regular file), -2 if an invalid flip argument is passed in, 0
+ * otherwise.
+ */
+int chmod_index_entry(struct index_state *istate, struct cache_entry *ce,
+		      char flip)
+{
+	if (!S_ISREG(ce->ce_mode))
+		return -1;
+	switch (flip) {
+	case '+':
+		ce->ce_mode |= 0111;
+		break;
+	case '-':
+		ce->ce_mode &= ~0111;
+		break;
+	default:
+		return -2;
+	}
+	cache_tree_invalidate_path(&the_index, ce->name);
+	ce->ce_flags |= CE_UPDATE_IN_BASE;
+	istate->cache_changed |= CE_ENTRY_CHANGED;
+
+	return 0;
+}
+
 int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)
 {
 	int len = ce_namelen(a);
-- 
2.10.0.304.gf2ff484

Re: [PATCH v4 3/4] read-cache: introduce chmod_index_entry

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> As there are chmod options for both add and update-index, introduce a
> new chmod_index_entry function to do the work.  Use it in update-index,
> while it will be used in add in the next patch.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
>  builtin/update-index.c | 16 ++--------------
>  cache.h                |  2 ++
>  read-cache.c           | 29 +++++++++++++++++++++++++++++
>  3 files changed, 33 insertions(+), 14 deletions(-)
>
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index bbdf0d9..9e9e040 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -423,26 +423,14 @@ static void chmod_path(char flip, const char *path)
>  {
> ...
> -	mode = ce->ce_mode;
> -	if (!S_ISREG(mode))
> -		goto fail;
> -	switch (flip) {
> -	case '+':
> -		ce->ce_mode |= 0111; break;
> -	case '-':
> -		ce->ce_mode &= ~0111; break;
> -	default:
> +	if (chmod_cache_entry(ce, flip) < 0)
>  		goto fail;
> -	}
> -	cache_tree_invalidate_path(&the_index, path);

This used to always work on the default index, hence the_index
reference is here, but ...

> +int chmod_index_entry(struct index_state *istate, struct cache_entry *ce,
> +		      char flip)
> +{
> +	if (!S_ISREG(ce->ce_mode))
> +		return -1;
> +	switch (flip) {
> +	case '+':
> +		ce->ce_mode |= 0111;
> +		break;
> +	case '-':
> +		ce->ce_mode &= ~0111;
> +		break;
> +	default:
> +		return -2;
> +	}
> +	cache_tree_invalidate_path(&the_index, ce->name);

... this one takes istate, so you need to use it, instead of the
hard-coded the_index reference.

> +	ce->ce_flags |= CE_UPDATE_IN_BASE;
> +	istate->cache_changed |= CE_ENTRY_CHANGED;
> +
> +	return 0;
> +}
> +
>  int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)
>  {
>  	int len = ce_namelen(a);

Other than that, this looks good to me.

Re: [PATCH v4 3/4] read-cache: introduce chmod_index_entry

[Junio C Hamano]

I've queued this trivial SQUASH??? on top, which I think should be
squashed into 3/4.

Thanks.


 read-cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/read-cache.c b/read-cache.c
index 2445e30..c2b2e97 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -779,7 +779,7 @@ int chmod_index_entry(struct index_state *istate, struct cache_entry *ce,
 	default:
 		return -2;
 	}
-	cache_tree_invalidate_path(&the_index, ce->name);
+	cache_tree_invalidate_path(istate, ce->name);
 	ce->ce_flags |= CE_UPDATE_IN_BASE;
 	istate->cache_changed |= CE_ENTRY_CHANGED;
 
-- 
2.10.0-458-g8cce42d

Re: [PATCH v4 3/4] read-cache: introduce chmod_index_entry

[Thomas Gummerer]

On 09/14, Junio C Hamano wrote:
> I've queued this trivial SQUASH??? on top, which I think should be
> squashed into 3/4.

Yeah, I missed this.  The SQUASH??? definitely makes sense, would be
great if you could just squash that in.

> Thanks.

Thanks the reviews and helping me getting the series in a good shape!

> 
> 
>  read-cache.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/read-cache.c b/read-cache.c
> index 2445e30..c2b2e97 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -779,7 +779,7 @@ int chmod_index_entry(struct index_state *istate, struct cache_entry *ce,
>  	default:
>  		return -2;
>  	}
> -	cache_tree_invalidate_path(&the_index, ce->name);
> +	cache_tree_invalidate_path(istate, ce->name);
>  	ce->ce_flags |= CE_UPDATE_IN_BASE;
>  	istate->cache_changed |= CE_ENTRY_CHANGED;
>  
> -- 
> 2.10.0-458-g8cce42d
> 

-- 
Thomas

[PATCH v4 4/4] add: modify already added files when --chmod is given

[Thomas Gummerer]

When the chmod option was added to git add, it was hooked up to the diff
machinery, meaning that it only works when the version in the index
differs from the version on disk.

As the option was supposed to mirror the chmod option in update-index,
which always changes the mode in the index, regardless of the status of
the file, make sure the option behaves the same way in git add.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/add.c      | 47 ++++++++++++++++++++++++++++-------------------
 builtin/checkout.c |  2 +-
 builtin/commit.c   |  2 +-
 cache.h            | 10 +++++-----
 read-cache.c       | 14 ++++++--------
 t/t3700-add.sh     | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 91 insertions(+), 34 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index b1dddb4..595a0b2 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -26,10 +26,25 @@ static int patch_interactive, add_interactive, edit_interactive;
 static int take_worktree_changes;
 
 struct update_callback_data {
-	int flags, force_mode;
+	int flags;
 	int add_errors;
 };
 
+static void chmod_pathspec(struct pathspec *pathspec, int force_mode)
+{
+	int i;
+	
+	for (i = 0; i < active_nr; i++) {
+		struct cache_entry *ce = active_cache[i];
+
+		if (pathspec && !ce_path_match(ce, pathspec, NULL))
+			continue;
+
+		if (chmod_cache_entry(ce, force_mode) < 0)
+			fprintf(stderr, "cannot chmod '%s'", ce->name);
+	}
+}
+
 static int fix_unmerged_status(struct diff_filepair *p,
 			       struct update_callback_data *data)
 {
@@ -65,8 +80,7 @@ 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, data->force_mode)) {
+			if (add_file_to_index(&the_index, path,	data->flags)) {
 				if (!(data->flags & ADD_CACHE_IGNORE_ERRORS))
 					die(_("updating files failed"));
 				data->add_errors++;
@@ -84,15 +98,14 @@ static void update_callback(struct diff_queue_struct *q,
 	}
 }
 
-int add_files_to_cache(const char *prefix, const struct pathspec *pathspec,
-	int flags, int force_mode)
+int add_files_to_cache(const char *prefix,
+		       const struct pathspec *pathspec, int flags)
 {
 	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);
@@ -281,7 +294,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, int force_mode)
+static int add_files(struct dir_struct *dir, int flags)
 {
 	int i, exit_status = 0;
 
@@ -294,8 +307,7 @@ static int add_files(struct dir_struct *dir, int flags, int force_mode)
 	}
 
 	for (i = 0; i < dir->nr; i++)
-		if (add_file_to_index(&the_index, dir->entries[i]->name,
-				flags, force_mode)) {
+		if (add_file_to_index(&the_index, dir->entries[i]->name, flags)) {
 			if (!ignore_add_errors)
 				die(_("adding files failed"));
 			exit_status = 1;
@@ -308,7 +320,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, force_mode;
+	int flags;
 	int add_new_files;
 	int require_pathspec;
 	char *seen = NULL;
@@ -342,13 +354,8 @@ 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)
-		force_mode = 0;
-	else if (!strcmp(chmod_arg, "-x"))
-		force_mode = 0666;
-	else if (!strcmp(chmod_arg, "+x"))
-		force_mode = 0777;
-	else
+	if (chmod_arg && ((chmod_arg[0] != '-' && chmod_arg[0] != '+') ||
+			  chmod_arg[1] != 'x' || chmod_arg[2]))
 		die(_("--chmod param '%s' must be either -x or +x"), chmod_arg);
 
 	add_new_files = !take_worktree_changes && !refresh_only;
@@ -441,11 +448,13 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	plug_bulk_checkin();
 
-	exit_status |= add_files_to_cache(prefix, &pathspec, flags, force_mode);
+	exit_status |= add_files_to_cache(prefix, &pathspec, flags);
 
 	if (add_new_files)
-		exit_status |= add_files(&dir, flags, force_mode);
+		exit_status |= add_files(&dir, flags);
 
+	if (chmod_arg && pathspec.nr)
+		chmod_pathspec(&pathspec, chmod_arg[0]);
 	unplug_bulk_checkin();
 
 finish:
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8672d07..a83c78f 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, 0);
+			add_files_to_cache(NULL, NULL, 0);
 			/*
 			 * NEEDSWORK: carrying over local changes
 			 * when branches have different end-of-line
diff --git a/builtin/commit.c b/builtin/commit.c
index bb9f79b..1cba3b7 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -397,7 +397,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, 0);
+		add_files_to_cache(also ? prefix : NULL, &pathspec, 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 35c8d1c..cd8e9fe 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), 0)
-#define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags), 0)
+#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 chmod_cache_entry(ce, flip) chmod_index_entry(&the_index, (ce), (flip))
 #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))
@@ -582,8 +582,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
-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 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);
 extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, char flip);
 extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);
@@ -1821,7 +1821,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 force_mode);
+int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int flags);
 
 /* diff.c */
 extern int diff_auto_refresh_index;
diff --git a/read-cache.c b/read-cache.c
index 8924f2e..016bbcb 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -627,7 +627,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 force_mode)
+int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags)
 {
 	int size, namelen, was_same;
 	mode_t st_mode = st->st_mode;
@@ -656,11 +656,10 @@ 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_mode)
-		ce->ce_mode = create_ce_mode(force_mode);
-	else if (trust_executable_bit && has_symlinks)
+
+	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.
 		 */
@@ -719,13 +718,12 @@ 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 force_mode)
+int add_file_to_index(struct index_state *istate, const char *path, int flags)
 {
 	struct stat st;
 	if (lstat(path, &st))
 		die_errno("unable to stat '%s'", path);
-	return add_to_index(istate, path, &st, flags, force_mode);
+	return add_to_index(istate, path, &st, flags);
 }
 
 struct cache_entry *make_cache_entry(unsigned int mode,
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 2978cb9..0a962a6 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -349,4 +349,54 @@ test_expect_success POSIXPERM,SYMLINKS 'git add --chmod=+x with symlinks' '
 	test_mode_in_index 100755 foo2
 '
 
+test_expect_success 'git add --chmod=[+-]x changes index with already added file' '
+	echo foo >foo3 &&
+	git add foo3 &&
+	git add --chmod=+x foo3 &&
+	test_mode_in_index 100755 foo3 &&
+	echo foo >xfoo3 &&
+	chmod 755 xfoo3 &&
+	git add xfoo3 &&
+	git add --chmod=-x xfoo3 &&
+	test_mode_in_index 100644 xfoo3
+'
+
+test_expect_success 'file status is changed after git add --chmod=+x' '
+	echo "AM foo4" >expected &&
+	echo foo >foo4 &&
+	git add foo4 &&
+	git add --chmod=+x foo4 &&
+	git status -s foo4 >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'no file status change if no pathspec is given' '
+	>foo5 &&
+	>foo6 &&
+	git add foo5 foo6 &&
+	git add --chmod=+x &&
+	test_mode_in_index 100644 foo5 &&
+	test_mode_in_index 100644 foo6
+'
+
+test_expect_success 'no file status change if no pathspec is given in subdir' '
+	mkdir sub &&
+	(
+		cd sub &&
+		>sub-foo1 &&
+		>sub-foo2 &&
+		git add . &&
+		git add --chmod=+x &&
+		test_mode_in_index 100644 sub-foo1 &&
+		test_mode_in_index 100644 sub-foo2
+	)
+'
+
+test_expect_success 'all statuses changed in folder if . is given' '
+	git add --chmod=+x . &&
+	test $(git ls-files --stage | grep ^100644 | wc -l) -eq 0 &&
+	git add --chmod=-x . &&
+	test $(git ls-files --stage | grep ^100755 | wc -l) -eq 0
+'
+
 test_done
-- 
2.10.0.304.gf2ff484

Re: [PATCH v4 4/4] add: modify already added files when --chmod is given

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> When the chmod option was added to git add, it was hooked up to the diff
> machinery, meaning that it only works when the version in the index
> differs from the version on disk.
>
> As the option was supposed to mirror the chmod option in update-index,
> which always changes the mode in the index, regardless of the status of
> the file, make sure the option behaves the same way in git add.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---

This change essentially reverts most of what 4e55ed32 ("add: add
--chmod=+x / --chmod=-x options", 2016-05-31) did, except that it
keeps the command line option and adjusts its validation, and adds a
separate phase to "fix-up" the executable bits for all paths that
match the given pathspec, whether they were new or modified or
unchanged.

The patch makes sense to me.  Thanks.

Re: [PATCH v4 4/4] add: modify already added files when --chmod is given

[René Scharfe]

Am 14.09.2016 um 23:07 schrieb Thomas Gummerer:
> When the chmod option was added to git add, it was hooked up to the diff
> machinery, meaning that it only works when the version in the index
> differs from the version on disk.
> 
> As the option was supposed to mirror the chmod option in update-index,
> which always changes the mode in the index, regardless of the status of
> the file, make sure the option behaves the same way in git add.
> 
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>

Sorry for replying almost a year late, hopefully you're still interested.

> ---
>   builtin/add.c      | 47 ++++++++++++++++++++++++++++-------------------
>   builtin/checkout.c |  2 +-
>   builtin/commit.c   |  2 +-
>   cache.h            | 10 +++++-----
>   read-cache.c       | 14 ++++++--------
>   t/t3700-add.sh     | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   6 files changed, 91 insertions(+), 34 deletions(-)
> 
> diff --git a/builtin/add.c b/builtin/add.c
> index b1dddb4..595a0b2 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -26,10 +26,25 @@ static int patch_interactive, add_interactive, edit_interactive;
>   static int take_worktree_changes;
>   
>   struct update_callback_data {
> -	int flags, force_mode;
> +	int flags;
>   	int add_errors;
>   };
>   
> +static void chmod_pathspec(struct pathspec *pathspec, int force_mode)

"int force_mode" looks like a binary (or perhaps ternary) flag, but
actually it is a character and can only have the values '-' or '+'.
In builtin/update-index.c it's called "char flip" and we probably should
define it like this here as well.

> +{
> +	int i;
> +	
> +	for (i = 0; i < active_nr; i++) {
> +		struct cache_entry *ce = active_cache[i];
> +
> +		if (pathspec && !ce_path_match(ce, pathspec, NULL))
> +			continue;
> +
> +		if (chmod_cache_entry(ce, force_mode) < 0)
> +			fprintf(stderr, "cannot chmod '%s'", ce->name);

This error message is missing a newline.  In builtin/update-index.c we
also show the attempted change (-x or +x); perhaps we want to do that
here as well.

Currently chmod_cache_entry() can only fail if ce is not a regular
file or it's other parameter is neither '-' nor '+'.  We rule out the
latter already in the argument parsing code.  The former can happen if
we add a symlink, either explicitly or because it's in a directory
we're specified.

I wonder if we even need to report anything, or under which conditions.
If you have a file named dir/file and a symlink named dir/symlink then
the interesting cases are:

	git add --chmod=.. dir/symlink
	git add --chmod=.. dir/file dir/symlink
	git add --chmod=.. dir

Warning about each case may be the most cautious thing to do, but
documenting that --chmod has no effect on symlinks and keeping silent
might be less annoying, especially in the last case.  What do you
think?

> @@ -342,13 +354,8 @@ 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)
> -		force_mode = 0;
> -	else if (!strcmp(chmod_arg, "-x"))
> -		force_mode = 0666;
> -	else if (!strcmp(chmod_arg, "+x"))
> -		force_mode = 0777;
> -	else
> +	if (chmod_arg && ((chmod_arg[0] != '-' && chmod_arg[0] != '+') ||
> +			  chmod_arg[1] != 'x' || chmod_arg[2]))
>   		die(_("--chmod param '%s' must be either -x or +x"), chmod_arg);

That's the argument parsing code mentioned above.  The strcmp-based
checks look nicer to me btw.  How about this?

	if (chmod_arg && strcmp(chmod_arg, "-x") && strcmp(chmod_arg, "+x"))

But that's just nitpicking.

René

Re: [PATCH v4 4/4] add: modify already added files when --chmod is given

[Thomas Gummerer]

On 08/07, René Scharfe wrote:
> Am 14.09.2016 um 23:07 schrieb Thomas Gummerer:
> > When the chmod option was added to git add, it was hooked up to the diff
> > machinery, meaning that it only works when the version in the index
> > differs from the version on disk.
> > 
> > As the option was supposed to mirror the chmod option in update-index,
> > which always changes the mode in the index, regardless of the status of
> > the file, make sure the option behaves the same way in git add.
> > 
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> 
> Sorry for replying almost a year late, hopefully you're still interested.

Thanks for still replying :)

> > ---
> >   builtin/add.c      | 47 ++++++++++++++++++++++++++++-------------------
> >   builtin/checkout.c |  2 +-
> >   builtin/commit.c   |  2 +-
> >   cache.h            | 10 +++++-----
> >   read-cache.c       | 14 ++++++--------
> >   t/t3700-add.sh     | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >   6 files changed, 91 insertions(+), 34 deletions(-)
> > 
> > diff --git a/builtin/add.c b/builtin/add.c
> > index b1dddb4..595a0b2 100644
> > --- a/builtin/add.c
> > +++ b/builtin/add.c
> > @@ -26,10 +26,25 @@ static int patch_interactive, add_interactive, edit_interactive;
> >   static int take_worktree_changes;
> >   
> >   struct update_callback_data {
> > -	int flags, force_mode;
> > +	int flags;
> >   	int add_errors;
> >   };
> >   
> > +static void chmod_pathspec(struct pathspec *pathspec, int force_mode)
> 
> "int force_mode" looks like a binary (or perhaps ternary) flag, but
> actually it is a character and can only have the values '-' or '+'.
> In builtin/update-index.c it's called "char flip" and we probably should
> define it like this here as well.
> 
> > +{
> > +	int i;
> > +	
> > +	for (i = 0; i < active_nr; i++) {
> > +		struct cache_entry *ce = active_cache[i];
> > +
> > +		if (pathspec && !ce_path_match(ce, pathspec, NULL))
> > +			continue;
> > +
> > +		if (chmod_cache_entry(ce, force_mode) < 0)
> > +			fprintf(stderr, "cannot chmod '%s'", ce->name);
> 
> This error message is missing a newline.  In builtin/update-index.c we
> also show the attempted change (-x or +x); perhaps we want to do that
> here as well.

Thanks for catching this, both this and the above are worth changing
imo.  I see Ramsay already provided a patch for this in
https://public-inbox.org/git/aa004526-3e0d-66d4-287f-30abd29758fc@ramsayjones.plus.com/.
Thanks Ramsay!

> Currently chmod_cache_entry() can only fail if ce is not a regular
> file or it's other parameter is neither '-' nor '+'.  We rule out the
> latter already in the argument parsing code.  The former can happen if
> we add a symlink, either explicitly or because it's in a directory
> we're specified.
> 
> I wonder if we even need to report anything, or under which conditions.
> If you have a file named dir/file and a symlink named dir/symlink then
> the interesting cases are:
> 
> 	git add --chmod=.. dir/symlink
> 	git add --chmod=.. dir/file dir/symlink
> 	git add --chmod=.. dir
> 
> Warning about each case may be the most cautious thing to do, but
> documenting that --chmod has no effect on symlinks and keeping silent
> might be less annoying, especially in the last case.  What do you
> think?

I'm not sure about this.  While I do agree that it could be quite
annoying in the last case, it could potentially be a bit confusing to
not get any warning in the first case.  I don't have a strong opinion
either way.

> > @@ -342,13 +354,8 @@ 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)
> > -		force_mode = 0;
> > -	else if (!strcmp(chmod_arg, "-x"))
> > -		force_mode = 0666;
> > -	else if (!strcmp(chmod_arg, "+x"))
> > -		force_mode = 0777;
> > -	else
> > +	if (chmod_arg && ((chmod_arg[0] != '-' && chmod_arg[0] != '+') ||
> > +			  chmod_arg[1] != 'x' || chmod_arg[2]))
> >   		die(_("--chmod param '%s' must be either -x or +x"), chmod_arg);
> 
> That's the argument parsing code mentioned above.  The strcmp-based
> checks look nicer to me btw.  How about this?
> 
> 	if (chmod_arg && strcmp(chmod_arg, "-x") && strcmp(chmod_arg, "+x"))
> 
> But that's just nitpicking.

I think this looks nicer indeed, thanks!  But it's probably not worth
a patch for just this unless we decide to change to not warn as you
mentioned above, and can fix this at the same time?

> René