{"thread":{"id":"43977","subject":"`make profile-install` fails in 2.9.3","startedAt":"2016-09-01T16:08:09Z","lastAt":"2017-08-12T12:30:13Z","messageCount":40,"participants":["Jan Keromnes","Dennis Kaarsemaker","Thomas Gummerer","Jeff King","Junio C Hamano","Johannes Schindelin","René Scharfe"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"300813","messageId":"CAA6PgK7C18F1WGyZMTEUAWEVsUWqiZND5Ne_0SH-rUEm8u5dNg@mail.gmail.com","threadId":"43977","inReplyTo":null,"subject":"`make profile-install` fails in 2.9.3","fromName":"Jan Keromnes","fromEmail":"janx@linux.com","sentAt":"2016-09-01T16:08:02Z","receivedAt":"2016-09-01T16:08:09Z","isPatch":false,"sender":{"key":"janx@linux.com","avatar":"https://gravatar.com/avatar/2d842511ba2875cc54f5434fb9c6a6ef813eb53bd511b6fe05b645f6c151e344?d=mp&s=160"},"body":"Hello,\n\nI'm trying to `profile-install` Git from source on Ubuntu 16.04, to\nhave the latest stable Git optimized for my machine.\n\nHowever, this fails (and has failed in previous versions), because it\nruns the whole test-suite to get the profile, but bails out if there\nwere test failures (which happens often).\n\nProblem: Is there a way to `make profile-install` but ignore\noccasional test failures, as they're not problematic to get a useful\nprofile?\n\nRelated problem: `t3700-add.sh` currently fails in 2.9.3. I can\nprovide more debug information if you don't already know this problem.\n\nThanks,\nJan Keromnes\n\n---\n\nSteps to reproduce:\n\n curl https://www.kernel.org/pub/software/scm/git/git-2.9.3.tar.xz | tar xJ \\\n && cd git-2.9.3 \\\n && make prefix=/usr profile-install install-man -j18\n\nExpected result:\n\n - runs all tests to get a profile (ignoring occasional failures)\n - rebuilds Git with the profile\n - installs Git\n\nActual result:\n\n - runs all tests to get a profile\n - at least one test fails, interrupting the whole process\n - Git is not installed\n\nFailure log:\n\n # failed 1 among 40 test(s)\n 1..40\n Makefile:43: recipe for target 't3700-add.sh' failed\n make[3]: *** [t3700-add.sh] Error 1\n make[3]: Leaving directory '/tmp/git/git-2.9.3/t'\n Makefile:36: recipe for target 'test' failed\n make[2]: Leaving directory '/tmp/git/git-2.9.3/t'\n make[2]: *** [test] Error 2\n Makefile:2221: recipe for target 'test' failed\n make[1]: *** [test] Error 2\n make[1]: Leaving directory '/tmp/git/git-2.9.3'\n Makefile:1633: recipe for target 'profile' failed\n make: *** [profile] Error 2\n The command '/bin/sh -c mkdir /tmp/git && cd /tmp/git && curl\nhttps://www.kernel.org/pub/software/scm/git/git-2.9.3.tar.xz | tar xJ\n&& cd git-2.9.3 && make prefix=/usr profile-install install-man -j18\n&& rm -rf /tmp/git' returned a non-zero code: 2\n"},{"id":"300816","messageId":"1472747125.4680.38.camel@kaarsemaker.net","threadId":"43977","inReplyTo":"CAA6PgK7C18F1WGyZMTEUAWEVsUWqiZND5Ne_0SH-rUEm8u5dNg@mail.gmail.com","subject":"Re: `make profile-install` fails in 2.9.3","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2016-09-01T16:25:25Z","receivedAt":"2016-09-01T16:25:33Z","isPatch":false,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On do, 2016-09-01 at 18:08 +0200, Jan Keromnes wrote:\n\n> However, this fails (and has failed in previous versions), because it\n> runs the whole test-suite to get the profile, but bails out if there\n> were test failures (which happens often).\n\nWorking around failing tests is fixing a symptom, not the root cause. I\nrun the testsuite for master, next and pu very regularly and test\nfailures on master are extremely rare. I think I've seen one or so in\nthe last year, might even be 0. So let's focus on that instead.\n\n> Related problem: `t3700-add.sh` currently fails in 2.9.3. I can\n> provide more debug information if you don't already know this problem.\n\nIt should not fail, and for me does not fail on ubuntu 16.04. Please\nrun that test in verbose mode and share its output, as well as your\nbuild configuration.\n\nD.\n"},{"id":"300824","messageId":"20160901200700.GA8254@hank","threadId":"43977","inReplyTo":"CAA6PgK7C18F1WGyZMTEUAWEVsUWqiZND5Ne_0SH-rUEm8u5dNg@mail.gmail.com","subject":"Re: `make profile-install` fails in 2.9.3","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-09-01T20:07:00Z","receivedAt":"2016-09-01T20:59:54Z","isPatch":false,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"[+cc Edward Thomson, Ingo Brückl]\n\nHi,\n\nOn 09/01, Jan Keromnes wrote:\n\n[.. snip the parts others are more qualified to answer ..]\n> \n> Related problem: `t3700-add.sh` currently fails in 2.9.3. I can\n> provide more debug information if you don't already know this problem.\n\nI noticed this problem as well, when I'm compiling with USE_NSEC = 1\nin my config.mak.\n\nTracking this problem down a bit, it happens because the --chmod=[+-]x\noption introduced in 4e55ed32 (\"add: add --chmod=+x / --chmod=-x\noptions\") only works if the file on disk is modified. When the test\nwas changed to work on one single file, instead of doing chmod=+x on\none file and chmod=-x on another file in b38ab197c (\"t3700: merge two\ntests into one\"), this test started breaking when the mtime of the\nfile and the index file weren't the same (in other words, if the file\nwas not racily clean and thus was not smudged).\n\nWhen the file is racily clean, git detects that the contents changed,\nand everything is happy, but if it isn't, git incorectly thinks the\nfile wasn't modified (which it wasn't, but from gits view it should be\nviewed as modified because the mode does not match up anymore).\n\nOne possible fix for the test is to smudge the entry as showed below,\nthough I'm not sure it's the right fix. The other way I can think of\nis to change the file in the index regardless of whether the file was\nchanged in some other way before issuing the git add command, as that\nmight fit the user expectation better. Thoughts?\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 491e52d..f2e7986 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -656,11 +656,13 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \telse\n \t\tce->ce_flags |= CE_INTENT_TO_ADD;\n \n-\tif (S_ISREG(st_mode) && force_mode)\n+\tif (S_ISREG(st_mode) && force_mode) {\n \t\tce->ce_mode = create_ce_mode(force_mode);\n-\telse if (trust_executable_bit && has_symlinks)\n+\t\tce->ce_stat_data.sd_size = 0;\n+\t} else if (trust_executable_bit && has_symlinks) {\n \t\tce->ce_mode = create_ce_mode(st_mode);\n-\telse {\n+\t} else {\n \t\t/* If there is an existing entry, pick the mode bits and type\n \t\t * from it, otherwise assume unexecutable regular file.\n \t\t */\n\n\t\t\t\t\t\t\t\t \n\n\n\n> Thanks,\n> Jan Keromnes\n> \n> ---\n> \n> Steps to reproduce:\n> \n> curl https://www.kernel.org/pub/software/scm/git/git-2.9.3.tar.xz | tar xJ \\\n> && cd git-2.9.3 \\\n> && make prefix=/usr profile-install install-man -j18\n> \n> Expected result:\n> \n> - runs all tests to get a profile (ignoring occasional failures)\n> - rebuilds Git with the profile\n> - installs Git\n> \n> Actual result:\n> \n> - runs all tests to get a profile\n> - at least one test fails, interrupting the whole process\n> - Git is not installed\n> \n> Failure log:\n> \n> # failed 1 among 40 test(s)\n> 1..40\n> Makefile:43: recipe for target 't3700-add.sh' failed\n> make[3]: *** [t3700-add.sh] Error 1\n> make[3]: Leaving directory '/tmp/git/git-2.9.3/t'\n> Makefile:36: recipe for target 'test' failed\n> make[2]: Leaving directory '/tmp/git/git-2.9.3/t'\n> make[2]: *** [test] Error 2\n> Makefile:2221: recipe for target 'test' failed\n> make[1]: *** [test] Error 2\n> make[1]: Leaving directory '/tmp/git/git-2.9.3'\n> Makefile:1633: recipe for target 'profile' failed\n> make: *** [profile] Error 2\n> The command '/bin/sh -c mkdir /tmp/git && cd /tmp/git && curl\n> https://www.kernel.org/pub/software/scm/git/git-2.9.3.tar.xz | tar xJ\n> && cd git-2.9.3 && make prefix=/usr profile-install install-man -j18\n> && rm -rf /tmp/git' returned a non-zero code: 2\n\n-- \nThomas\n"},{"id":"300842","messageId":"20160901215810.ez47lqwmfmahyvc7@sigill.intra.peff.net","threadId":"43977","inReplyTo":"20160901200700.GA8254@hank","subject":"Re: `make profile-install` fails in 2.9.3","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-01T21:58:10Z","receivedAt":"2016-09-01T21:58:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 01, 2016 at 09:07:00PM +0100, Thomas Gummerer wrote:\n\n> > Related problem: `t3700-add.sh` currently fails in 2.9.3. I can\n> > provide more debug information if you don't already know this problem.\n> \n> I noticed this problem as well, when I'm compiling with USE_NSEC = 1\n> in my config.mak.\n\nI can replicate this even without USE_NSEC with my stress-tester[1].\nThat makes sense why it would show up with the profiling run; git runs\nslower and therefore increases the chances of crossing the 1-second\nboundary and losing the race.\n\n[1] https://github.com/peff/git/blob/meta/stress\n\n> Tracking this problem down a bit, it happens because the --chmod=[+-]x\n> option introduced in 4e55ed32 (\"add: add --chmod=+x / --chmod=-x\n> options\") only works if the file on disk is modified. When the test\n> was changed to work on one single file, instead of doing chmod=+x on\n> one file and chmod=-x on another file in b38ab197c (\"t3700: merge two\n> tests into one\"), this test started breaking when the mtime of the\n> file and the index file weren't the same (in other words, if the file\n> was not racily clean and thus was not smudged).\n\nThat certainly sounds buggy. A less racy way of verifying this is just:\n\n # guarantee not-racy state\n echo content >file\n test-chmtime -60 file\n git add file\n\n # now check --chmod; file will still be 100644!\n git add --chmod=+x file\n git ls-files -s\n\n> One possible fix for the test is to smudge the entry as showed below,\n> though I'm not sure it's the right fix. The other way I can think of\n> is to change the file in the index regardless of whether the file was\n> changed in some other way before issuing the git add command, as that\n> might fit the user expectation better. Thoughts?\n\nYeah, I think we should _always_ act on the --chmod, no matter if the\nfile is racy or not, or whether it has a content change or not. I.e.,\nthe race is not the problem, but rather the behavior of 4e55ed32. Your\nsecond proposal there sounds more like the right approach.\n\n-Peff\n"},{"id":"300846","messageId":"xmqqh99zuuyh.fsf@gitster.mtv.corp.google.com","threadId":"43977","inReplyTo":"20160901215810.ez47lqwmfmahyvc7@sigill.intra.peff.net","subject":"Re: `make profile-install` fails in 2.9.3","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-01T22:16:38Z","receivedAt":"2016-09-01T22:16:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah, I think we should _always_ act on the --chmod, no matter if the\n> file is racy or not, or whether it has a content change or not. I.e.,\n> the race is not the problem, but rather the behavior of 4e55ed32. Your\n> second proposal there sounds more like the right approach.\n\nYeah, you two are absolutely right. The second \"git add --chmod=+x\"\nin\n\n $ git add .\n $ git add --chmod=+x .\n\nshould still find _all_ the non-executable paths and flip their\nexecutable bit in the index, making them all up-to-date in the\nindex.\n\nWhich means that piggybacking this on the \"run 'git diff' limited to\nthe pathspec to find the paths that needs updating\" logic usually\ndone in \"git add\" can not be reused [*1*].\n\nWhat was I thinking while reviewing the patch X-<. Sigh.\n\n\n[Footnote]\n\n*1* I guess we _could_, by first flipping all the regular file\n blob's executable bit for paths that are inside the pathspec and\n then by running \"git diff\" against that modified index, limited\n to the pathspec, to find the paths that need to be added.\n\n It sounds ugly, but may conceptually be cleaner. We first start\n from an ideal end-result, and then re-hash what needs to be\n updated to match the ideal.\n\n\n"},{"id":"300847","messageId":"20160901222015.wqhrerudnwddbyhz@sigill.intra.peff.net","threadId":"43977","inReplyTo":"xmqqh99zuuyh.fsf@gitster.mtv.corp.google.com","subject":"Re: `make profile-install` fails in 2.9.3","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-01T22:20:15Z","receivedAt":"2016-09-01T22:20:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 01, 2016 at 03:16:38PM -0700, Junio C Hamano wrote:\n\n> Which means that piggybacking this on the \"run 'git diff' limited to\n> the pathspec to find the paths that needs updating\" logic usually\n> done in \"git add\" can not be reused [*1*].\n> \n> What was I thinking while reviewing the patch X-<. Sigh.\n> \n> \n> [Footnote]\n> \n> *1* I guess we _could_, by first flipping all the regular file\n> blob's executable bit for paths that are inside the pathspec and\n> then by running \"git diff\" against that modified index, limited\n> to the pathspec, to find the paths that need to be added.\n> \n> It sounds ugly, but may conceptually be cleaner. We first start\n> from an ideal end-result, and then re-hash what needs to be\n> updated to match the ideal.\n\nYeah, I had a similar thought, but it just feels so hacky. Is there\nanything wrong with making this completely separate from the content\nupdate. I.e., just applying the pathspec to the index as a separate step\nand adding \"+x\" to each entry?\n\nThis really is just a more convenient interface around \"update-index\n--chmod\", isn't it? We should be able to do the same thing it does.\n\n-Peff\n"},{"id":"300851","messageId":"xmqq4m5zutyc.fsf@gitster.mtv.corp.google.com","threadId":"43977","inReplyTo":"20160901222015.wqhrerudnwddbyhz@sigill.intra.peff.net","subject":"Re: `make profile-install` fails in 2.9.3","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-01T22:38:19Z","receivedAt":"2016-09-01T22:38:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah, I had a similar thought, but it just feels so hacky. Is there\n> anything wrong with making this completely separate from the content\n> update. I.e., just applying the pathspec to the index as a separate step\n> and adding \"+x\" to each entry?\n>\n> This really is just a more convenient interface around \"update-index\n> --chmod\", isn't it? We should be able to do the same thing it does.\n\nSure, the simplest and the most straight-forward way may look dumb,\nbut it would be the safest.\n\n\n"},{"id":"300983","messageId":"20160904113954.21697-1-t.gummerer@gmail.com","threadId":"43977","inReplyTo":"xmqq4m5zutyc.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 0/4] git add --chmod: always change the file","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-09-04T11:39:50Z","receivedAt":"2016-09-04T11:40:13Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Thanks to Peff and Junio for your inputs on the best way to solve this\nproblem.\n\nThe patch series is made up as follows:\n\n[1/4]: Documentation for the chmod option\n[2,3/4]: Small refactoring to simplify the final step\n[4/4]: The actual change that introduces the new behaviour.\n\nThomas Gummerer (4):\n add: document the chmod option\n update-index: use the same structure for chmod as add\n read-cache: introduce chmod_index_entry\n add: modify already added files when --chmod is given\n\n Documentation/git-add.txt | 7 +++++-\n builtin/add.c | 36 +++++++++++++++++++++----------\n builtin/checkout.c | 2 +-\n builtin/commit.c | 2 +-\n builtin/update-index.c | 55 ++++++++++++++++++-----------------------------\n cache.h | 12 ++++++-----\n read-cache.c | 33 +++++++++++++++++++++-------\n t/t3700-add.sh | 21 ++++++++++++++++++\n 8 files changed, 107 insertions(+), 61 deletions(-)\n\n-- \n2.10.0.304.gf2ff484\n\n"},{"id":"300984","messageId":"20160904113954.21697-2-t.gummerer@gmail.com","threadId":"43977","inReplyTo":"20160904113954.21697-1-t.gummerer@gmail.com","subject":"[PATCH 1/4] add: document the chmod option","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-09-04T11:39:51Z","receivedAt":"2016-09-04T11:40:22Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"The git add --chmod option was introduced in 4e55ed3 (\"add: add\n--chmod=+x / --chmod=-x options\", 2016-05-31), but was never\ndocumented. Document the feature.\n\nSigned-off-by: Thomas Gummerer \n---\n Documentation/git-add.txt | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-add.txt b/Documentation/git-add.txt\nindex 6a96a66..8caa691 100644\n--- a/Documentation/git-add.txt\n+++ b/Documentation/git-add.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n 'git add' [--verbose | -v] [--dry-run | -n] [--force | -f] [--interactive | -i] [--patch | -p]\n \t [--edit | -e] [--[no-]all | --[no-]ignore-removal | [--update | -u]]\n \t [--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing]\n-\t [--] [...]\n+\t [--chmod=(+|-)x] [--] [...]\n \n DESCRIPTION\n -----------\n@@ -165,6 +165,11 @@ for \"git add --no-all ...\", i.e. ignored removed files.\n \tbe ignored, no matter if they are already present in the work\n \ttree or not.\n \n+--chmod=(+|-)::\n+\tOverride the executable bit of the added files. The executable\n+\tbit is only changed in the index, the files on disk are left\n+\tunchanged.\n+\n \\--::\n \tThis option can be used to separate command-line options from\n \tthe list of files, (useful when filenames might be mistaken\n-- \n2.10.0.304.gf2ff484\n\n"},{"id":"300985","messageId":"20160904113954.21697-4-t.gummerer@gmail.com","threadId":"43977","inReplyTo":"20160904113954.21697-1-t.gummerer@gmail.com","subject":"[PATCH 3/4] read-cache: introduce chmod_index_entry","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-09-04T11:39:53Z","receivedAt":"2016-09-04T11:40:24Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"As there are chmod options for both add and update-index, introduce a\nnew chmod_index_entry function to do the work. Use it in update-index,\nwhile it will be used in add in the next patch.\n\nSigned-off-by: Thomas Gummerer \n---\n builtin/update-index.c | 8 +-------\n cache.h | 2 ++\n read-cache.c | 19 +++++++++++++++++++\n 3 files changed, 22 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 85a57db..1569c81 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -423,20 +423,14 @@ static void chmod_path(int force_mode, const char *path)\n {\n \tint pos;\n \tstruct cache_entry *ce;\n-\tunsigned int mode;\n \tchar flip = force_mode == 0777 ? '+' : '-';\n \n \tpos = cache_name_pos(path, strlen(path));\n \tif (pos < 0)\n \t\tgoto fail;\n \tce = active_cache[pos];\n-\tmode = ce->ce_mode;\n-\tif (!S_ISREG(mode))\n+\tif (chmod_cache_entry(ce, force_mode) < 0)\n \t\tgoto fail;\n-\tce->ce_mode = create_ce_mode(force_mode);\n-\tcache_tree_invalidate_path(&the_index, path);\n-\tce->ce_flags |= CE_UPDATE_IN_BASE;\n-\tactive_cache_changed |= CE_ENTRY_CHANGED;\n \n \treport(\"chmod %cx '%s'\", flip, path);\n \treturn;\ndiff --git a/cache.h b/cache.h\nindex b780a91..44a4f76 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -369,6 +369,7 @@ extern void free_name_hash(struct index_state *istate);\n #define remove_file_from_cache(path) remove_file_from_index(&the_index, (path))\n #define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags), 0)\n #define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags), 0)\n+#define chmod_cache_entry(ce, force_mode) chmod_index_entry(&the_index, (ce), (force_mode))\n #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL)\n #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options))\n #define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options))\n@@ -584,6 +585,7 @@ extern int remove_file_from_index(struct index_state *, const char *path);\n extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags, int force_mode);\n extern int add_file_to_index(struct index_state *, const char *path, int flags, int force_mode);\n extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, unsigned int refresh_options);\n+extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, int force_mode);\n extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);\n extern void set_object_name_for_intent_to_add_entry(struct cache_entry *ce);\n extern int index_name_is_other(const struct index_state *, const char *, int);\ndiff --git a/read-cache.c b/read-cache.c\nindex 491e52d..367be57 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -756,6 +756,25 @@ struct cache_entry *make_cache_entry(unsigned int mode,\n \treturn ret;\n }\n \n+/*\n+ * Change the mode of an index entry to force_mode, where force_mode can\n+ * either be 0777 or 0666.\n+ * Returns -1 if the chmod for the particular cache entry failed (if it's\n+ * not a regular file), 0 otherwise.\n+ */\n+int chmod_index_entry(struct index_state *istate, struct cache_entry *ce,\n+\t\t int force_mode)\n+{\n+\tif (!S_ISREG(ce->ce_mode))\n+\t\treturn -1;\n+\tce->ce_mode = create_ce_mode(force_mode);\n+\tcache_tree_invalidate_path(istate, ce->name);\n+\tce->ce_flags |= CE_UPDATE_IN_BASE;\n+\tistate->cache_changed |= CE_ENTRY_CHANGED;\n+\n+\treturn 0;\n+}\n+\n int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)\n {\n \tint len = ce_namelen(a);\n-- \n2.10.0.304.gf2ff484\n\n"},{"id":"300986","messageId":"20160904113954.21697-3-t.gummerer@gmail.com","threadId":"43977","inReplyTo":"20160904113954.21697-1-t.gummerer@gmail.com","subject":"[PATCH 2/4] update-index: use the same structure for chmod as add","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-09-04T11:39:52Z","receivedAt":"2016-09-04T11:40:26Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"While the chmod options for update-index and the add have the same\nfunctionality, they are using different ways to parse and handle the\noption internally. Unify these modes in order to make further\nrefactoring simpler.\n\nSigned-off-by: Thomas Gummerer \n---\n builtin/update-index.c | 49 +++++++++++++++++++++----------------------------\n 1 file changed, 21 insertions(+), 28 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex ba04b19..85a57db 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -419,11 +419,12 @@ static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,\n \treturn 0;\n }\n \n-static void chmod_path(int flip, const char *path)\n+static void chmod_path(int force_mode, const char *path)\n {\n \tint pos;\n \tstruct cache_entry *ce;\n \tunsigned int mode;\n+\tchar flip = force_mode == 0777 ? '+' : '-';\n \n \tpos = cache_name_pos(path, strlen(path));\n \tif (pos < 0)\n@@ -432,17 +433,11 @@ static void chmod_path(int flip, const char *path)\n \tmode = ce->ce_mode;\n \tif (!S_ISREG(mode))\n \t\tgoto fail;\n-\tswitch (flip) {\n-\tcase '+':\n-\t\tce->ce_mode |= 0111; break;\n-\tcase '-':\n-\t\tce->ce_mode &= ~0111; break;\n-\tdefault:\n-\t\tgoto fail;\n-\t}\n+\tce->ce_mode = create_ce_mode(force_mode);\n \tcache_tree_invalidate_path(&the_index, path);\n \tce->ce_flags |= CE_UPDATE_IN_BASE;\n \tactive_cache_changed |= CE_ENTRY_CHANGED;\n+\n \treport(\"chmod %cx '%s'\", flip, path);\n \treturn;\n fail:\n@@ -788,16 +783,6 @@ static int really_refresh_callback(const struct option *opt,\n \treturn refresh(opt->value, REFRESH_REALLY);\n }\n \n-static int chmod_callback(const struct option *opt,\n-\t\t\t\tconst char *arg, int unset)\n-{\n-\tchar *flip = opt->value;\n-\tif ((arg[0] != '-' && arg[0] != '+') || arg[1] != 'x' || arg[2])\n-\t\treturn error(\"option 'chmod' expects \\\"+x\\\" or \\\"-x\\\"\");\n-\t*flip = arg[0];\n-\treturn 0;\n-}\n-\n static int resolve_undo_clear_callback(const struct option *opt,\n \t\t\t\tconst char *arg, int unset)\n {\n@@ -917,7 +902,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \tint read_from_stdin = 0;\n \tint prefix_length = prefix ? strlen(prefix) : 0;\n \tint preferred_index_format = 0;\n-\tchar set_executable_bit = 0;\n+\tchar *chmod_arg = 0;\n+\tint force_mode = 0;\n \tstruct refresh_params refresh_args = {0, &has_errors};\n \tint lock_error = 0;\n \tint split_index = -1;\n@@ -955,10 +941,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\t\tPARSE_OPT_NOARG | /* disallow --cacheinfo= form */\n \t\t\tPARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n \t\t\t(parse_opt_cb *) cacheinfo_callback},\n-\t\t{OPTION_CALLBACK, 0, \"chmod\", &set_executable_bit, N_(\"(+/-)x\"),\n-\t\t\tN_(\"override the executable bit of the listed files\"),\n-\t\t\tPARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n-\t\t\tchmod_callback},\n+\t\tOPT_STRING( 0, \"chmod\", &chmod_arg, N_(\"(+/-)x\"),\n+\t\t\tN_(\"override the executable bit of the listed files\")),\n \t\t{OPTION_SET_INT, 0, \"assume-unchanged\", &mark_valid_only, NULL,\n \t\t\tN_(\"mark files as \\\"not changing\\\"\"),\n \t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, MARK_FLAG},\n@@ -1018,6 +1002,15 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage_with_options(update_index_usage, options);\n \n+\tif (!chmod_arg)\n+\t\tforce_mode = 0;\n+\telse if (!strcmp(chmod_arg, \"-x\"))\n+\t\tforce_mode = 0666;\n+\telse if (!strcmp(chmod_arg, \"+x\"))\n+\t\tforce_mode = 0777;\n+\telse\n+\t\tdie(_(\"option 'chmod' expects \\\"+x\\\" or \\\"-x\\\"\"));\n+\n \tgit_config(git_default_config, NULL);\n \n \t/* We can't free this memory, it becomes part of a linked list parsed atexit() */\n@@ -1055,8 +1048,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\t\tsetup_work_tree();\n \t\t\tp = prefix_path(prefix, prefix_length, path);\n \t\t\tupdate_one(p);\n-\t\t\tif (set_executable_bit)\n-\t\t\t\tchmod_path(set_executable_bit, p);\n+\t\t\tif (force_mode)\n+\t\t\t\tchmod_path(force_mode, p);\n \t\t\tfree(p);\n \t\t\tctx.argc--;\n \t\t\tctx.argv++;\n@@ -1100,8 +1093,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\t\t}\n \t\t\tp = prefix_path(prefix, prefix_length, buf.buf);\n \t\t\tupdate_one(p);\n-\t\t\tif (set_executable_bit)\n-\t\t\t\tchmod_path(set_executable_bit, p);\n+\t\t\tif (force_mode)\n+\t\t\t\tchmod_path(force_mode, p);\n \t\t\tfree(p);\n \t\t}\n \t\tstrbuf_release(&unquoted);\n-- \n2.10.0.304.gf2ff484\n\n"},{"id":"300987","messageId":"20160904113954.21697-5-t.gummerer@gmail.com","threadId":"43977","inReplyTo":"20160904113954.21697-1-t.gummerer@gmail.com","subject":"[PATCH 4/4] add: modify already added files when --chmod is given","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-09-04T11:39:54Z","receivedAt":"2016-09-04T11:40:30Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"When the chmod option was added to git add, it was hooked up to the diff\nmachinery, meaning that it only works when the version in the index\ndiffers from the version on disk.\n\nAs the option was supposed to mirror the chmod option in update-index,\nwhich always changes the mode in the index, regardless of the status of\nthe file, make sure the option behaves the same way in git add.\n\nReported-by: Jan Keromnes \nSigned-off-by: Thomas Gummerer \n---\n builtin/add.c | 36 +++++++++++++++++++++++++-----------\n builtin/checkout.c | 2 +-\n builtin/commit.c | 2 +-\n cache.h | 10 +++++-----\n read-cache.c | 14 ++++++--------\n t/t3700-add.sh | 21 +++++++++++++++++++++\n 6 files changed, 59 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex b1dddb4..892198a 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -26,10 +26,25 @@ static int patch_interactive, add_interactive, edit_interactive;\n static int take_worktree_changes;\n \n struct update_callback_data {\n-\tint flags, force_mode;\n+\tint flags;\n \tint add_errors;\n };\n \n+static void chmod_pathspec(struct pathspec *pathspec, int force_mode)\n+{\n+\tint i;\n+\t\n+\tfor (i = 0; i < active_nr; i++) {\n+\t\tstruct cache_entry *ce = active_cache[i];\n+\n+\t\tif (pathspec && !ce_path_match(ce, pathspec, NULL))\n+\t\t\tcontinue;\n+\n+\t\tif (chmod_cache_entry(ce, force_mode) < 0)\n+\t\t\tfprintf(stderr, \"cannot chmod '%s'\", ce->name);\n+\t}\n+}\n+\n static int fix_unmerged_status(struct diff_filepair *p,\n \t\t\t struct update_callback_data *data)\n {\n@@ -65,8 +80,7 @@ static void update_callback(struct diff_queue_struct *q,\n \t\t\tdie(_(\"unexpected diff status %c\"), p->status);\n \t\tcase DIFF_STATUS_MODIFIED:\n \t\tcase DIFF_STATUS_TYPE_CHANGED:\n-\t\t\tif (add_file_to_index(&the_index, path,\n-\t\t\t\t\tdata->flags, data->force_mode)) {\n+\t\t\tif (add_file_to_index(&the_index, path,\tdata->flags)) {\n \t\t\t\tif (!(data->flags & ADD_CACHE_IGNORE_ERRORS))\n \t\t\t\t\tdie(_(\"updating files failed\"));\n \t\t\t\tdata->add_errors++;\n@@ -84,15 +98,14 @@ static void update_callback(struct diff_queue_struct *q,\n \t}\n }\n \n-int add_files_to_cache(const char *prefix, const struct pathspec *pathspec,\n-\tint flags, int force_mode)\n+int add_files_to_cache(const char *prefix,\n+\t\t const struct pathspec *pathspec, int flags)\n {\n \tstruct update_callback_data data;\n \tstruct rev_info rev;\n \n \tmemset(&data, 0, sizeof(data));\n \tdata.flags = flags;\n-\tdata.force_mode = force_mode;\n \n \tinit_revisions(&rev, prefix);\n \tsetup_revisions(0, NULL, &rev, NULL);\n@@ -281,7 +294,7 @@ static int add_config(const char *var, const char *value, void *cb)\n \treturn git_default_config(var, value, cb);\n }\n \n-static int add_files(struct dir_struct *dir, int flags, int force_mode)\n+static int add_files(struct dir_struct *dir, int flags)\n {\n \tint i, exit_status = 0;\n \n@@ -294,8 +307,7 @@ static int add_files(struct dir_struct *dir, int flags, int force_mode)\n \t}\n \n \tfor (i = 0; i < dir->nr; i++)\n-\t\tif (add_file_to_index(&the_index, dir->entries[i]->name,\n-\t\t\t\tflags, force_mode)) {\n+\t\tif (add_file_to_index(&the_index, dir->entries[i]->name, flags)) {\n \t\t\tif (!ignore_add_errors)\n \t\t\t\tdie(_(\"adding files failed\"));\n \t\t\texit_status = 1;\n@@ -441,11 +453,13 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \n \tplug_bulk_checkin();\n \n-\texit_status |= add_files_to_cache(prefix, &pathspec, flags, force_mode);\n+\texit_status |= add_files_to_cache(prefix, &pathspec, flags);\n \n \tif (add_new_files)\n-\t\texit_status |= add_files(&dir, flags, force_mode);\n+\t\texit_status |= add_files(&dir, flags);\n \n+\tif (force_mode)\n+\t\tchmod_pathspec(&pathspec, force_mode);\n \tunplug_bulk_checkin();\n \n finish:\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 8672d07..a83c78f 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -548,7 +548,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t * entries in the index.\n \t\t\t */\n \n-\t\t\tadd_files_to_cache(NULL, NULL, 0, 0);\n+\t\t\tadd_files_to_cache(NULL, NULL, 0);\n \t\t\t/*\n \t\t\t * NEEDSWORK: carrying over local changes\n \t\t\t * when branches have different end-of-line\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 77e3dc8..7a1ade0 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -387,7 +387,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t */\n \tif (all || (also && pathspec.nr)) {\n \t\thold_locked_index(&index_lock, 1);\n-\t\tadd_files_to_cache(also ? prefix : NULL, &pathspec, 0, 0);\n+\t\tadd_files_to_cache(also ? prefix : NULL, &pathspec, 0);\n \t\trefresh_cache_or_die(refresh_flags);\n \t\tupdate_main_cache_tree(WRITE_TREE_SILENT);\n \t\tif (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))\ndiff --git a/cache.h b/cache.h\nindex 44a4f76..eb6d6b8 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -367,8 +367,8 @@ extern void free_name_hash(struct index_state *istate);\n #define rename_cache_entry_at(pos, new_name) rename_index_entry_at(&the_index, (pos), (new_name))\n #define remove_cache_entry_at(pos) remove_index_entry_at(&the_index, (pos))\n #define remove_file_from_cache(path) remove_file_from_index(&the_index, (path))\n-#define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags), 0)\n-#define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags), 0)\n+#define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags))\n+#define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags))\n #define chmod_cache_entry(ce, force_mode) chmod_index_entry(&the_index, (ce), (force_mode))\n #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL)\n #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options))\n@@ -582,8 +582,8 @@ extern int remove_file_from_index(struct index_state *, const char *path);\n #define ADD_CACHE_IGNORE_ERRORS\t4\n #define ADD_CACHE_IGNORE_REMOVAL 8\n #define ADD_CACHE_INTENT 16\n-extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags, int force_mode);\n-extern int add_file_to_index(struct index_state *, const char *path, int flags, int force_mode);\n+extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags);\n+extern int add_file_to_index(struct index_state *, const char *path, int flags);\n extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, unsigned int refresh_options);\n extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, int force_mode);\n extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);\n@@ -1820,7 +1820,7 @@ void packet_trace_identity(const char *prog);\n * return 0 if success, 1 - if addition of a file failed and\n * ADD_FILES_IGNORE_ERRORS was specified in flags\n */\n-int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int flags, int force_mode);\n+int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int flags);\n \n /* diff.c */\n extern int diff_auto_refresh_index;\ndiff --git a/read-cache.c b/read-cache.c\nindex 367be57..b92d72e 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -627,7 +627,7 @@ void set_object_name_for_intent_to_add_entry(struct cache_entry *ce)\n \thashcpy(ce->sha1, sha1);\n }\n \n-int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags, int force_mode)\n+int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags)\n {\n \tint size, namelen, was_same;\n \tmode_t st_mode = st->st_mode;\n@@ -656,11 +656,10 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \telse\n \t\tce->ce_flags |= CE_INTENT_TO_ADD;\n \n-\tif (S_ISREG(st_mode) && force_mode)\n-\t\tce->ce_mode = create_ce_mode(force_mode);\n-\telse if (trust_executable_bit && has_symlinks)\n+\n+\tif (trust_executable_bit && has_symlinks) {\n \t\tce->ce_mode = create_ce_mode(st_mode);\n-\telse {\n+\t} else {\n \t\t/* If there is an existing entry, pick the mode bits and type\n \t\t * from it, otherwise assume unexecutable regular file.\n \t\t */\n@@ -719,13 +718,12 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \treturn 0;\n }\n \n-int add_file_to_index(struct index_state *istate, const char *path,\n-\tint flags, int force_mode)\n+int add_file_to_index(struct index_state *istate, const char *path, int flags)\n {\n \tstruct stat st;\n \tif (lstat(path, &st))\n \t\tdie_errno(\"unable to stat '%s'\", path);\n-\treturn add_to_index(istate, path, &st, flags, force_mode);\n+\treturn add_to_index(istate, path, &st, flags);\n }\n \n struct cache_entry *make_cache_entry(unsigned int mode,\ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex 2978cb9..62c7f71 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -349,4 +349,25 @@ test_expect_success POSIXPERM,SYMLINKS 'git add --chmod=+x with symlinks' '\n \ttest_mode_in_index 100755 foo2\n '\n \n+test_expect_success 'git add --chmod=[+-]x changes index with already added file' '\n+\techo foo >foo3 &&\n+\tgit add foo3 &&\n+\tgit add --chmod=+x foo3 &&\n+\ttest_mode_in_index 100755 foo3 &&\n+\techo foo >xfoo3 &&\n+\tchmod 755 xfoo3 &&\n+\tgit add xfoo3 &&\n+\tgit add --chmod=-x xfoo3 &&\n+\ttest_mode_in_index 100644 xfoo3\n+'\n+\n+test_expect_success 'file status is changed after git add --chmod=+x' '\n+\techo \"AM foo4\" >expected &&\n+\techo foo >foo4 &&\n+\tgit add foo4 &&\n+\tgit add --chmod=+x foo4 &&\n+\tgit status -s foo4 >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n2.10.0.304.gf2ff484\n\n"},{"id":"301081","messageId":"alpine.DEB.2.20.1609050944070.129229@virtualbox","threadId":"43977","inReplyTo":"20160904113954.21697-2-t.gummerer@gmail.com","subject":"Re: [PATCH 1/4] add: document the chmod option","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-05T07:44:25Z","receivedAt":"2016-09-05T07:44:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Thomas,\n\nOn Sun, 4 Sep 2016, Thomas Gummerer wrote:\n\n> +--chmod=(+|-)::\n\nThere is an \"x\" missing ;-)\n\nCiao,\nDscho\n"},{"id":"301113","messageId":"20160905192202.GB8254@hank","threadId":"43977","inReplyTo":"alpine.DEB.2.20.1609050944070.129229@virtualbox","subject":"Re: [PATCH 1/4] add: document the chmod option","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-09-05T19:22:02Z","receivedAt":"2016-09-05T19:22:03Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Hi Johannes,\n\nOn 09/05, Johannes Schindelin wrote:\n> Hi Thomas,\n> \n> On Sun, 4 Sep 2016, Thomas Gummerer wrote:\n> \n> > +--chmod=(+|-)::\n> \n> There is an \"x\" missing ;-)\n\nUgh, thanks for catching. I'll wait a few days for more comments and\naddress it in a re-roll.\n\n> Ciao,\n> Dscho\n"},{"id":"301290","messageId":"xmqq1t0vpsl7.fsf@gitster.mtv.corp.google.com","threadId":"43977","inReplyTo":"20160905192202.GB8254@hank","subject":"Re: [PATCH 1/4] add: document the chmod option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-07T16:44:52Z","receivedAt":"2016-09-07T16:45:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer writes:\n\n> Ugh, thanks for catching. I'll wait a few days for more comments and\n> address it in a re-roll.\n\nThanks for starting this. I agree with you and Peff that running\n\"chmod\" on the resulting index after other \"add\" operations took\nplace is the right approach, as we want to flip the executable bit\nfor paths that hasn't otherwise changed.\n\n"},{"id":"301626","messageId":"20160911103028.5492-1-t.gummerer@gmail.com","threadId":"43977","inReplyTo":"20160904113954.21697-1-t.gummerer@gmail.com","subject":"[PATCH v2 0/4] git add --chmod: always change the file","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-09-11T10:30:24Z","receivedAt":"2016-09-11T10:30:32Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Changes since v1:\nAdded missing x in the documentation.\n\nThe changes since v1 are only minor, but as it's been a week, this is\nas much a ping as a fixup of my error :)\n\nThomas Gummerer (4):\n add: document the chmod option\n update-index: use the same structure for chmod as add\n read-cache: introduce chmod_index_entry\n add: modify already added files when --chmod is given\n\n Documentation/git-add.txt | 7 +++++-\n builtin/add.c | 36 +++++++++++++++++++++----------\n builtin/checkout.c | 2 +-\n builtin/commit.c | 2 +-\n builtin/update-index.c | 55 ++++++++++++++++++-----------------------------\n cache.h | 12 ++++++-----\n read-cache.c | 33 +++++++++++++++++++++-------\n t/t3700-add.sh | 21 ++++++++++++++++++\n 8 files changed, 107 insertions(+), 61 deletions(-)\n\n-- \n2.10.0.304.gf2ff484\n\n"},{"id":"301627","messageId":"20160911103028.5492-2-t.gummerer@gmail.com","threadId":"43977","inReplyTo":"20160911103028.5492-1-t.gummerer@gmail.com","subject":"[PATCH v2 1/4] add: document the chmod option","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-09-11T10:30:25Z","receivedAt":"2016-09-11T10:30:35Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"The git add --chmod option was introduced in 4e55ed3 (\"add: add\n--chmod=+x / --chmod=-x options\", 2016-05-31), but was never\ndocumented. Document the feature.\n\nSigned-off-by: Thomas Gummerer \n---\n Documentation/git-add.txt | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-add.txt b/Documentation/git-add.txt\nindex 6a96a66..7ed63dc 100644\n--- a/Documentation/git-add.txt\n+++ b/Documentation/git-add.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n 'git add' [--verbose | -v] [--dry-run | -n] [--force | -f] [--interactive | -i] [--patch | -p]\n \t [--edit | -e] [--[no-]all | --[no-]ignore-removal | [--update | -u]]\n \t [--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing]\n-\t [--] [...]\n+\t [--chmod=(+|-)x] [--] [...]\n \n DESCRIPTION\n -----------\n@@ -165,6 +165,11 @@ for \"git add --no-all ...\", i.e. ignored removed files.\n \tbe ignored, no matter if they are already present in the work\n \ttree or not.\n \n+--chmod=(+|-)x::\n+\tOverride the executable bit of the added files. The executable\n+\tbit is only changed in the index, the files on disk are left\n+\tunchanged.\n+\n \\--::\n \tThis option can be used to separate command-line options from\n \tthe list of files, (useful when filenames might be mistaken\n-- \n2.10.0.304.gf2ff484\n\n"},{"id":"301628","messageId":"20160911103028.5492-3-t.gummerer@gmail.com","threadId":"43977","inReplyTo":"20160911103028.5492-1-t.gummerer@gmail.com","subject":"[PATCH v2 2/4] update-index: use the same structure for chmod as add","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-09-11T10:30:26Z","receivedAt":"2016-09-11T10:30:39Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"While the chmod options for update-index and the add have the same\nfunctionality, they are using different ways to parse and handle the\noption internally. Unify these modes in order to make further\nrefactoring simpler.\n\nSigned-off-by: Thomas Gummerer \n---\n builtin/update-index.c | 49 +++++++++++++++++++++----------------------------\n 1 file changed, 21 insertions(+), 28 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex ba04b19..85a57db 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -419,11 +419,12 @@ static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,\n \treturn 0;\n }\n \n-static void chmod_path(int flip, const char *path)\n+static void chmod_path(int force_mode, const char *path)\n {\n \tint pos;\n \tstruct cache_entry *ce;\n \tunsigned int mode;\n+\tchar flip = force_mode == 0777 ? '+' : '-';\n \n \tpos = cache_name_pos(path, strlen(path));\n \tif (pos < 0)\n@@ -432,17 +433,11 @@ static void chmod_path(int flip, const char *path)\n \tmode = ce->ce_mode;\n \tif (!S_ISREG(mode))\n \t\tgoto fail;\n-\tswitch (flip) {\n-\tcase '+':\n-\t\tce->ce_mode |= 0111; break;\n-\tcase '-':\n-\t\tce->ce_mode &= ~0111; break;\n-\tdefault:\n-\t\tgoto fail;\n-\t}\n+\tce->ce_mode = create_ce_mode(force_mode);\n \tcache_tree_invalidate_path(&the_index, path);\n \tce->ce_flags |= CE_UPDATE_IN_BASE;\n \tactive_cache_changed |= CE_ENTRY_CHANGED;\n+\n \treport(\"chmod %cx '%s'\", flip, path);\n \treturn;\n fail:\n@@ -788,16 +783,6 @@ static int really_refresh_callback(const struct option *opt,\n \treturn refresh(opt->value, REFRESH_REALLY);\n }\n \n-static int chmod_callback(const struct option *opt,\n-\t\t\t\tconst char *arg, int unset)\n-{\n-\tchar *flip = opt->value;\n-\tif ((arg[0] != '-' && arg[0] != '+') || arg[1] != 'x' || arg[2])\n-\t\treturn error(\"option 'chmod' expects \\\"+x\\\" or \\\"-x\\\"\");\n-\t*flip = arg[0];\n-\treturn 0;\n-}\n-\n static int resolve_undo_clear_callback(const struct option *opt,\n \t\t\t\tconst char *arg, int unset)\n {\n@@ -917,7 +902,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \tint read_from_stdin = 0;\n \tint prefix_length = prefix ? strlen(prefix) : 0;\n \tint preferred_index_format = 0;\n-\tchar set_executable_bit = 0;\n+\tchar *chmod_arg = 0;\n+\tint force_mode = 0;\n \tstruct refresh_params refresh_args = {0, &has_errors};\n \tint lock_error = 0;\n \tint split_index = -1;\n@@ -955,10 +941,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\t\tPARSE_OPT_NOARG | /* disallow --cacheinfo= form */\n \t\t\tPARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n \t\t\t(parse_opt_cb *) cacheinfo_callback},\n-\t\t{OPTION_CALLBACK, 0, \"chmod\", &set_executable_bit, N_(\"(+/-)x\"),\n-\t\t\tN_(\"override the executable bit of the listed files\"),\n-\t\t\tPARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n-\t\t\tchmod_callback},\n+\t\tOPT_STRING( 0, \"chmod\", &chmod_arg, N_(\"(+/-)x\"),\n+\t\t\tN_(\"override the executable bit of the listed files\")),\n \t\t{OPTION_SET_INT, 0, \"assume-unchanged\", &mark_valid_only, NULL,\n \t\t\tN_(\"mark files as \\\"not changing\\\"\"),\n \t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, MARK_FLAG},\n@@ -1018,6 +1002,15 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage_with_options(update_index_usage, options);\n \n+\tif (!chmod_arg)\n+\t\tforce_mode = 0;\n+\telse if (!strcmp(chmod_arg, \"-x\"))\n+\t\tforce_mode = 0666;\n+\telse if (!strcmp(chmod_arg, \"+x\"))\n+\t\tforce_mode = 0777;\n+\telse\n+\t\tdie(_(\"option 'chmod' expects \\\"+x\\\" or \\\"-x\\\"\"));\n+\n \tgit_config(git_default_config, NULL);\n \n \t/* We can't free this memory, it becomes part of a linked list parsed atexit() */\n@@ -1055,8 +1048,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\t\tsetup_work_tree();\n \t\t\tp = prefix_path(prefix, prefix_length, path);\n \t\t\tupdate_one(p);\n-\t\t\tif (set_executable_bit)\n-\t\t\t\tchmod_path(set_executable_bit, p);\n+\t\t\tif (force_mode)\n+\t\t\t\tchmod_path(force_mode, p);\n \t\t\tfree(p);\n \t\t\tctx.argc--;\n \t\t\tctx.argv++;\n@@ -1100,8 +1093,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\t\t}\n \t\t\tp = prefix_path(prefix, prefix_length, buf.buf);\n \t\t\tupdate_one(p);\n-\t\t\tif (set_executable_bit)\n-\t\t\t\tchmod_path(set_executable_bit, p);\n+\t\t\tif (force_mode)\n+\t\t\t\tchmod_path(force_mode, p);\n \t\t\tfree(p);\n \t\t}\n \t\tstrbuf_release(&unquoted);\n-- \n2.10.0.304.gf2ff484\n\n"},{"id":"301629","messageId":"20160911103028.5492-4-t.gummerer@gmail.com","threadId":"43977","inReplyTo":"20160911103028.5492-1-t.gummerer@gmail.com","subject":"[PATCH v2 3/4] read-cache: introduce chmod_index_entry","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-09-11T10:30:27Z","receivedAt":"2016-09-11T10:30:42Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"As there are chmod options for both add and update-index, introduce a\nnew chmod_index_entry function to do the work. Use it in update-index,\nwhile it will be used in add in the next patch.\n\nSigned-off-by: Thomas Gummerer \n---\n builtin/update-index.c | 8 +-------\n cache.h | 2 ++\n read-cache.c | 19 +++++++++++++++++++\n 3 files changed, 22 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 85a57db..1569c81 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -423,20 +423,14 @@ static void chmod_path(int force_mode, const char *path)\n {\n \tint pos;\n \tstruct cache_entry *ce;\n-\tunsigned int mode;\n \tchar flip = force_mode == 0777 ? '+' : '-';\n \n \tpos = cache_name_pos(path, strlen(path));\n \tif (pos < 0)\n \t\tgoto fail;\n \tce = active_cache[pos];\n-\tmode = ce->ce_mode;\n-\tif (!S_ISREG(mode))\n+\tif (chmod_cache_entry(ce, force_mode) < 0)\n \t\tgoto fail;\n-\tce->ce_mode = create_ce_mode(force_mode);\n-\tcache_tree_invalidate_path(&the_index, path);\n-\tce->ce_flags |= CE_UPDATE_IN_BASE;\n-\tactive_cache_changed |= CE_ENTRY_CHANGED;\n \n \treport(\"chmod %cx '%s'\", flip, path);\n \treturn;\ndiff --git a/cache.h b/cache.h\nindex b780a91..44a4f76 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -369,6 +369,7 @@ extern void free_name_hash(struct index_state *istate);\n #define remove_file_from_cache(path) remove_file_from_index(&the_index, (path))\n #define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags), 0)\n #define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags), 0)\n+#define chmod_cache_entry(ce, force_mode) chmod_index_entry(&the_index, (ce), (force_mode))\n #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL)\n #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options))\n #define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options))\n@@ -584,6 +585,7 @@ extern int remove_file_from_index(struct index_state *, const char *path);\n extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags, int force_mode);\n extern int add_file_to_index(struct index_state *, const char *path, int flags, int force_mode);\n extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, unsigned int refresh_options);\n+extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, int force_mode);\n extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);\n extern void set_object_name_for_intent_to_add_entry(struct cache_entry *ce);\n extern int index_name_is_other(const struct index_state *, const char *, int);\ndiff --git a/read-cache.c b/read-cache.c\nindex 491e52d..367be57 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -756,6 +756,25 @@ struct cache_entry *make_cache_entry(unsigned int mode,\n \treturn ret;\n }\n \n+/*\n+ * Change the mode of an index entry to force_mode, where force_mode can\n+ * either be 0777 or 0666.\n+ * Returns -1 if the chmod for the particular cache entry failed (if it's\n+ * not a regular file), 0 otherwise.\n+ */\n+int chmod_index_entry(struct index_state *istate, struct cache_entry *ce,\n+\t\t int force_mode)\n+{\n+\tif (!S_ISREG(ce->ce_mode))\n+\t\treturn -1;\n+\tce->ce_mode = create_ce_mode(force_mode);\n+\tcache_tree_invalidate_path(istate, ce->name);\n+\tce->ce_flags |= CE_UPDATE_IN_BASE;\n+\tistate->cache_changed |= CE_ENTRY_CHANGED;\n+\n+\treturn 0;\n+}\n+\n int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)\n {\n \tint len = ce_namelen(a);\n-- \n2.10.0.304.gf2ff484\n\n"},{"id":"301630","messageId":"20160911103028.5492-5-t.gummerer@gmail.com","threadId":"43977","inReplyTo":"20160911103028.5492-1-t.gummerer@gmail.com","subject":"[PATCH v2 4/4] add: modify already added files when --chmod is given","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-09-11T10:30:28Z","receivedAt":"2016-09-11T10:30:52Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"When the chmod option was added to git add, it was hooked up to the diff\nmachinery, meaning that it only works when the version in the index\ndiffers from the version on disk.\n\nAs the option was supposed to mirror the chmod option in update-index,\nwhich always changes the mode in the index, regardless of the status of\nthe file, make sure the option behaves the same way in git add.\n\nSigned-off-by: Thomas Gummerer \n---\n builtin/add.c | 36 +++++++++++++++++++++++++-----------\n builtin/checkout.c | 2 +-\n builtin/commit.c | 2 +-\n cache.h | 10 +++++-----\n read-cache.c | 14 ++++++--------\n t/t3700-add.sh | 21 +++++++++++++++++++++\n 6 files changed, 59 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex b1dddb4..892198a 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -26,10 +26,25 @@ static int patch_interactive, add_interactive, edit_interactive;\n static int take_worktree_changes;\n \n struct update_callback_data {\n-\tint flags, force_mode;\n+\tint flags;\n \tint add_errors;\n };\n \n+static void chmod_pathspec(struct pathspec *pathspec, int force_mode)\n+{\n+\tint i;\n+\t\n+\tfor (i = 0; i < active_nr; i++) {\n+\t\tstruct cache_entry *ce = active_cache[i];\n+\n+\t\tif (pathspec && !ce_path_match(ce, pathspec, NULL))\n+\t\t\tcontinue;\n+\n+\t\tif (chmod_cache_entry(ce, force_mode) < 0)\n+\t\t\tfprintf(stderr, \"cannot chmod '%s'\", ce->name);\n+\t}\n+}\n+\n static int fix_unmerged_status(struct diff_filepair *p,\n \t\t\t struct update_callback_data *data)\n {\n@@ -65,8 +80,7 @@ static void update_callback(struct diff_queue_struct *q,\n \t\t\tdie(_(\"unexpected diff status %c\"), p->status);\n \t\tcase DIFF_STATUS_MODIFIED:\n \t\tcase DIFF_STATUS_TYPE_CHANGED:\n-\t\t\tif (add_file_to_index(&the_index, path,\n-\t\t\t\t\tdata->flags, data->force_mode)) {\n+\t\t\tif (add_file_to_index(&the_index, path,\tdata->flags)) {\n \t\t\t\tif (!(data->flags & ADD_CACHE_IGNORE_ERRORS))\n \t\t\t\t\tdie(_(\"updating files failed\"));\n \t\t\t\tdata->add_errors++;\n@@ -84,15 +98,14 @@ static void update_callback(struct diff_queue_struct *q,\n \t}\n }\n \n-int add_files_to_cache(const char *prefix, const struct pathspec *pathspec,\n-\tint flags, int force_mode)\n+int add_files_to_cache(const char *prefix,\n+\t\t const struct pathspec *pathspec, int flags)\n {\n \tstruct update_callback_data data;\n \tstruct rev_info rev;\n \n \tmemset(&data, 0, sizeof(data));\n \tdata.flags = flags;\n-\tdata.force_mode = force_mode;\n \n \tinit_revisions(&rev, prefix);\n \tsetup_revisions(0, NULL, &rev, NULL);\n@@ -281,7 +294,7 @@ static int add_config(const char *var, const char *value, void *cb)\n \treturn git_default_config(var, value, cb);\n }\n \n-static int add_files(struct dir_struct *dir, int flags, int force_mode)\n+static int add_files(struct dir_struct *dir, int flags)\n {\n \tint i, exit_status = 0;\n \n@@ -294,8 +307,7 @@ static int add_files(struct dir_struct *dir, int flags, int force_mode)\n \t}\n \n \tfor (i = 0; i < dir->nr; i++)\n-\t\tif (add_file_to_index(&the_index, dir->entries[i]->name,\n-\t\t\t\tflags, force_mode)) {\n+\t\tif (add_file_to_index(&the_index, dir->entries[i]->name, flags)) {\n \t\t\tif (!ignore_add_errors)\n \t\t\t\tdie(_(\"adding files failed\"));\n \t\t\texit_status = 1;\n@@ -441,11 +453,13 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \n \tplug_bulk_checkin();\n \n-\texit_status |= add_files_to_cache(prefix, &pathspec, flags, force_mode);\n+\texit_status |= add_files_to_cache(prefix, &pathspec, flags);\n \n \tif (add_new_files)\n-\t\texit_status |= add_files(&dir, flags, force_mode);\n+\t\texit_status |= add_files(&dir, flags);\n \n+\tif (force_mode)\n+\t\tchmod_pathspec(&pathspec, force_mode);\n \tunplug_bulk_checkin();\n \n finish:\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 8672d07..a83c78f 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -548,7 +548,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t * entries in the index.\n \t\t\t */\n \n-\t\t\tadd_files_to_cache(NULL, NULL, 0, 0);\n+\t\t\tadd_files_to_cache(NULL, NULL, 0);\n \t\t\t/*\n \t\t\t * NEEDSWORK: carrying over local changes\n \t\t\t * when branches have different end-of-line\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 77e3dc8..7a1ade0 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -387,7 +387,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t */\n \tif (all || (also && pathspec.nr)) {\n \t\thold_locked_index(&index_lock, 1);\n-\t\tadd_files_to_cache(also ? prefix : NULL, &pathspec, 0, 0);\n+\t\tadd_files_to_cache(also ? prefix : NULL, &pathspec, 0);\n \t\trefresh_cache_or_die(refresh_flags);\n \t\tupdate_main_cache_tree(WRITE_TREE_SILENT);\n \t\tif (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))\ndiff --git a/cache.h b/cache.h\nindex 44a4f76..eb6d6b8 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -367,8 +367,8 @@ extern void free_name_hash(struct index_state *istate);\n #define rename_cache_entry_at(pos, new_name) rename_index_entry_at(&the_index, (pos), (new_name))\n #define remove_cache_entry_at(pos) remove_index_entry_at(&the_index, (pos))\n #define remove_file_from_cache(path) remove_file_from_index(&the_index, (path))\n-#define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags), 0)\n-#define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags), 0)\n+#define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags))\n+#define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags))\n #define chmod_cache_entry(ce, force_mode) chmod_index_entry(&the_index, (ce), (force_mode))\n #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL)\n #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options))\n@@ -582,8 +582,8 @@ extern int remove_file_from_index(struct index_state *, const char *path);\n #define ADD_CACHE_IGNORE_ERRORS\t4\n #define ADD_CACHE_IGNORE_REMOVAL 8\n #define ADD_CACHE_INTENT 16\n-extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags, int force_mode);\n-extern int add_file_to_index(struct index_state *, const char *path, int flags, int force_mode);\n+extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags);\n+extern int add_file_to_index(struct index_state *, const char *path, int flags);\n extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, unsigned int refresh_options);\n extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, int force_mode);\n extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);\n@@ -1820,7 +1820,7 @@ void packet_trace_identity(const char *prog);\n * return 0 if success, 1 - if addition of a file failed and\n * ADD_FILES_IGNORE_ERRORS was specified in flags\n */\n-int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int flags, int force_mode);\n+int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int flags);\n \n /* diff.c */\n extern int diff_auto_refresh_index;\ndiff --git a/read-cache.c b/read-cache.c\nindex 367be57..b92d72e 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -627,7 +627,7 @@ void set_object_name_for_intent_to_add_entry(struct cache_entry *ce)\n \thashcpy(ce->sha1, sha1);\n }\n \n-int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags, int force_mode)\n+int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags)\n {\n \tint size, namelen, was_same;\n \tmode_t st_mode = st->st_mode;\n@@ -656,11 +656,10 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \telse\n \t\tce->ce_flags |= CE_INTENT_TO_ADD;\n \n-\tif (S_ISREG(st_mode) && force_mode)\n-\t\tce->ce_mode = create_ce_mode(force_mode);\n-\telse if (trust_executable_bit && has_symlinks)\n+\n+\tif (trust_executable_bit && has_symlinks) {\n \t\tce->ce_mode = create_ce_mode(st_mode);\n-\telse {\n+\t} else {\n \t\t/* If there is an existing entry, pick the mode bits and type\n \t\t * from it, otherwise assume unexecutable regular file.\n \t\t */\n@@ -719,13 +718,12 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \treturn 0;\n }\n \n-int add_file_to_index(struct index_state *istate, const char *path,\n-\tint flags, int force_mode)\n+int add_file_to_index(struct index_state *istate, const char *path, int flags)\n {\n \tstruct stat st;\n \tif (lstat(path, &st))\n \t\tdie_errno(\"unable to stat '%s'\", path);\n-\treturn add_to_index(istate, path, &st, flags, force_mode);\n+\treturn add_to_index(istate, path, &st, flags);\n }\n \n struct cache_entry *make_cache_entry(unsigned int mode,\ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex 2978cb9..62c7f71 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -349,4 +349,25 @@ test_expect_success POSIXPERM,SYMLINKS 'git add --chmod=+x with symlinks' '\n \ttest_mode_in_index 100755 foo2\n '\n \n+test_expect_success 'git add --chmod=[+-]x changes index with already added file' '\n+\techo foo >foo3 &&\n+\tgit add foo3 &&\n+\tgit add --chmod=+x foo3 &&\n+\ttest_mode_in_index 100755 foo3 &&\n+\techo foo >xfoo3 &&\n+\tchmod 755 xfoo3 &&\n+\tgit add xfoo3 &&\n+\tgit add --chmod=-x xfoo3 &&\n+\ttest_mode_in_index 100644 xfoo3\n+'\n+\n+test_expect_success 'file status is changed after git add --chmod=+x' '\n+\techo \"AM foo4\" >expected &&\n+\techo foo >foo4 &&\n+\tgit add foo4 &&\n+\tgit add --chmod=+x foo4 &&\n+\tgit status -s foo4 >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n2.10.0.304.gf2ff484\n\n"},{"id":"301675","messageId":"xmqqinu26ph0.fsf@gitster.mtv.corp.google.com","threadId":"43977","inReplyTo":"20160911103028.5492-3-t.gummerer@gmail.com","subject":"Re: [PATCH v2 2/4] update-index: use the same structure for chmod as add","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-11T22:28:27Z","receivedAt":"2016-09-11T22:28:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer writes:\n\n> @@ -955,10 +941,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n> \t\t\tPARSE_OPT_NOARG | /* disallow --cacheinfo= form */\n> \t\t\tPARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n> \t\t\t(parse_opt_cb *) cacheinfo_callback},\n> -\t\t{OPTION_CALLBACK, 0, \"chmod\", &set_executable_bit, N_(\"(+/-)x\"),\n> -\t\t\tN_(\"override the executable bit of the listed files\"),\n> -\t\t\tPARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n> -\t\t\tchmod_callback},\n> +\t\tOPT_STRING( 0, \"chmod\", &chmod_arg, N_(\"(+/-)x\"),\n> +\t\t\tN_(\"override the executable bit of the listed files\")),\n> \t\t{OPTION_SET_INT, 0, \"assume-unchanged\", &mark_valid_only, NULL,\n> \t\t\tN_(\"mark files as \\\"not changing\\\"\"),\n> \t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, MARK_FLAG},\n> @@ -1018,6 +1002,15 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n> \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n> \t\tusage_with_options(update_index_usage, options);\n> \n> +\tif (!chmod_arg)\n> +\t\tforce_mode = 0;\n> +\telse if (!strcmp(chmod_arg, \"-x\"))\n> +\t\tforce_mode = 0666;\n> +\telse if (!strcmp(chmod_arg, \"+x\"))\n> +\t\tforce_mode = 0777;\n> +\telse\n> +\t\tdie(_(\"option 'chmod' expects \\\"+x\\\" or \\\"-x\\\"\"));\n> +\n\nI am afraid that this changes the behaviour drastically.\n\n\"git update-index\" is an oddball command that takes options and then\nprocesses them immediately, exactly because it was designed to take\n\n\tgit update-index --chmod=-x A --chmod=+x B --add C\n\nand say things like \"A and B are not in the index and you are\nattempting to add them before giving me --add option\".\n\n\tgit update-index --add --chmod=-x A --chmod=+x B C\n\nis expected to add A as non-executable, and B and C as executable.\nMany exotic parse-options callback mechanisms used in this command\nwere invented exactly to support its quirky way of not doing \"get a\nlist of options and use the last one\". And this patch breaks it for\nonly one option without changing the others.\n\nIf we were willing to take such a big backward compatiblity hit in\nthe upcoming release (which I personally won't be affected, but old\nscripts by others need to be audited and adjusted, which I won't\nvolunteer to do myself), we should make such a change consistently,\ne.g. \"git update-index A --add --remove B\" should no longer error\nout when it sees A and it is not yet in the index because \"--add\"\nhasn't been given yet, or A is in the index but is missing from the\nworking tree because \"--remove\" hasn't been given yet. Then it may\nbe more justifiable if \"update-index --chmod=-x A --chmod=+x B\"\nadded A as an executable. With the current form of this patch, it\nis not.\n\nCan we do this \"fix\" without this change?\n\n\n\n\n"},{"id":"301746","messageId":"20160912193016.GD8254@hank","threadId":"43977","inReplyTo":"xmqqinu26ph0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/4] update-index: use the same structure for chmod as add","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-09-12T19:30:16Z","receivedAt":"2016-09-12T19:30:31Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 09/11, Junio C Hamano wrote:\n> Thomas Gummerer writes:\n> \n> > @@ -955,10 +941,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n> > \t\t\tPARSE_OPT_NOARG | /* disallow --cacheinfo= form */\n> > \t\t\tPARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n> > \t\t\t(parse_opt_cb *) cacheinfo_callback},\n> > -\t\t{OPTION_CALLBACK, 0, \"chmod\", &set_executable_bit, N_(\"(+/-)x\"),\n> > -\t\t\tN_(\"override the executable bit of the listed files\"),\n> > -\t\t\tPARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n> > -\t\t\tchmod_callback},\n> > +\t\tOPT_STRING( 0, \"chmod\", &chmod_arg, N_(\"(+/-)x\"),\n> > +\t\t\tN_(\"override the executable bit of the listed files\")),\n> > \t\t{OPTION_SET_INT, 0, \"assume-unchanged\", &mark_valid_only, NULL,\n> > \t\t\tN_(\"mark files as \\\"not changing\\\"\"),\n> > \t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, MARK_FLAG},\n> > @@ -1018,6 +1002,15 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n> > \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n> > \t\tusage_with_options(update_index_usage, options);\n> > \n> > +\tif (!chmod_arg)\n> > +\t\tforce_mode = 0;\n> > +\telse if (!strcmp(chmod_arg, \"-x\"))\n> > +\t\tforce_mode = 0666;\n> > +\telse if (!strcmp(chmod_arg, \"+x\"))\n> > +\t\tforce_mode = 0777;\n> > +\telse\n> > +\t\tdie(_(\"option 'chmod' expects \\\"+x\\\" or \\\"-x\\\"\"));\n> > +\n> \n> I am afraid that this changes the behaviour drastically.\n> \n> \"git update-index\" is an oddball command that takes options and then\n> processes them immediately, exactly because it was designed to take\n> \n> \tgit update-index --chmod=-x A --chmod=+x B --add C\n> \n> and say things like \"A and B are not in the index and you are\n> attempting to add them before giving me --add option\".\n> \n> \tgit update-index --add --chmod=-x A --chmod=+x B C\n> \n> is expected to add A as non-executable, and B and C as executable.\n> Many exotic parse-options callback mechanisms used in this command\n> were invented exactly to support its quirky way of not doing \"get a\n> list of options and use the last one\". And this patch breaks it for\n> only one option without changing the others.\n> \n> If we were willing to take such a big backward compatiblity hit in\n> the upcoming release (which I personally won't be affected, but old\n> scripts by others need to be audited and adjusted, which I won't\n> volunteer to do myself), we should make such a change consistently,\n> e.g. \"git update-index A --add --remove B\" should no longer error\n> out when it sees A and it is not yet in the index because \"--add\"\n> hasn't been given yet, or A is in the index but is missing from the\n> working tree because \"--remove\" hasn't been given yet. Then it may\n> be more justifiable if \"update-index --chmod=-x A --chmod=+x B\"\n> added A as an executable. With the current form of this patch, it\n> is not.\n\nThanks for the explanation, this change in backwards compatibility is\ncertainly not what I intended, but rather something I missed while\ncooking up this patch.\n\n> Can we do this \"fix\" without this change?\n\nYeah, let me see what I can come up with in a re-roll.\n\nThanks,\nThomas\n"},{"id":"301762","messageId":"20160912210818.26282-1-t.gummerer@gmail.com","threadId":"43977","inReplyTo":"20160911103028.5492-1-t.gummerer@gmail.com","subject":"[PATCH v3 0/4] git add --chmod: always change the file","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-09-12T21:08:14Z","receivedAt":"2016-09-12T21:08:50Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Thanks to Junio on setting me straight on the change of behaviour in\nthe previous round.\n\nThis round includes a similar change, which does however not change\nthe behaviour of update-index with repeated arguments. I still think\nthe unification of the way git add and git update-index handle chmod\nis useful, when we can keep the behaviour with multiple arguments in\nupdate-index the same.\n\nThomas Gummerer (4):\n add: document the chmod option\n update-index: use the same structure for chmod as add\n read-cache: introduce chmod_index_entry\n add: modify already added files when --chmod is given\n\n Documentation/git-add.txt | 7 ++++++-\n builtin/add.c | 36 +++++++++++++++++++++++-----------\n builtin/checkout.c | 2 +-\n builtin/commit.c | 2 +-\n builtin/update-index.c | 45 ++++++++++++++++++-------------------------\n cache.h | 12 +++++++-----\n read-cache.c | 33 +++++++++++++++++++++++--------\n t/t2107-update-index-basic.sh | 13 +++++++++++++\n t/t3700-add.sh | 21 ++++++++++++++++++++\n 9 files changed, 118 insertions(+), 53 deletions(-)\n\n-- \n2.10.0.304.gf2ff484\n\n"},{"id":"301763","messageId":"20160912210818.26282-2-t.gummerer@gmail.com","threadId":"43977","inReplyTo":"20160912210818.26282-1-t.gummerer@gmail.com","subject":"[PATCH v3 1/4] add: document the chmod option","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-09-12T21:08:15Z","receivedAt":"2016-09-12T21:08:54Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"The git add --chmod option was introduced in 4e55ed3 (\"add: add\n--chmod=+x / --chmod=-x options\", 2016-05-31), but was never\ndocumented. Document the feature.\n\nSigned-off-by: Thomas Gummerer \n---\n Documentation/git-add.txt | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-add.txt b/Documentation/git-add.txt\nindex 6a96a66..7ed63dc 100644\n--- a/Documentation/git-add.txt\n+++ b/Documentation/git-add.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n 'git add' [--verbose | -v] [--dry-run | -n] [--force | -f] [--interactive | -i] [--patch | -p]\n \t [--edit | -e] [--[no-]all | --[no-]ignore-removal | [--update | -u]]\n \t [--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing]\n-\t [--] [...]\n+\t [--chmod=(+|-)x] [--] [...]\n \n DESCRIPTION\n -----------\n@@ -165,6 +165,11 @@ for \"git add --no-all ...\", i.e. ignored removed files.\n \tbe ignored, no matter if they are already present in the work\n \ttree or not.\n \n+--chmod=(+|-)x::\n+\tOverride the executable bit of the added files. The executable\n+\tbit is only changed in the index, the files on disk are left\n+\tunchanged.\n+\n \\--::\n \tThis option can be used to separate command-line options from\n \tthe list of files, (useful when filenames might be mistaken\n-- \n2.10.0.304.gf2ff484\n\n"},{"id":"301764","messageId":"20160912210818.26282-3-t.gummerer@gmail.com","threadId":"43977","inReplyTo":"20160912210818.26282-1-t.gummerer@gmail.com","subject":"[PATCH v3 2/4] update-index: use the same structure for chmod as add","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-09-12T21:08:16Z","receivedAt":"2016-09-12T21:09:01Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"While the chmod options for update-index and the add have the same\nfunctionality, they are using different ways to parse and handle the\noption internally. Unify these modes in order to make further\nrefactoring simpler.\n\nSigned-off-by: Thomas Gummerer \n---\n builtin/update-index.c | 39 +++++++++++++++++++--------------------\n t/t2107-update-index-basic.sh | 13 +++++++++++++\n 2 files changed, 32 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex ba04b19..6d6cddd 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -419,11 +419,12 @@ static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,\n \treturn 0;\n }\n \n-static void chmod_path(int flip, const char *path)\n+static void chmod_path(int force_mode, const char *path)\n {\n \tint pos;\n \tstruct cache_entry *ce;\n \tunsigned int mode;\n+\tchar flip = force_mode == 0777 ? '+' : '-';\n \n \tpos = cache_name_pos(path, strlen(path));\n \tif (pos < 0)\n@@ -432,17 +433,11 @@ static void chmod_path(int flip, const char *path)\n \tmode = ce->ce_mode;\n \tif (!S_ISREG(mode))\n \t\tgoto fail;\n-\tswitch (flip) {\n-\tcase '+':\n-\t\tce->ce_mode |= 0111; break;\n-\tcase '-':\n-\t\tce->ce_mode &= ~0111; break;\n-\tdefault:\n-\t\tgoto fail;\n-\t}\n+\tce->ce_mode = create_ce_mode(force_mode);\n \tcache_tree_invalidate_path(&the_index, path);\n \tce->ce_flags |= CE_UPDATE_IN_BASE;\n \tactive_cache_changed |= CE_ENTRY_CHANGED;\n+\n \treport(\"chmod %cx '%s'\", flip, path);\n \treturn;\n fail:\n@@ -789,12 +784,16 @@ static int really_refresh_callback(const struct option *opt,\n }\n \n static int chmod_callback(const struct option *opt,\n-\t\t\t\tconst char *arg, int unset)\n+\t\t\t const char *arg, int unset)\n {\n-\tchar *flip = opt->value;\n-\tif ((arg[0] != '-' && arg[0] != '+') || arg[1] != 'x' || arg[2])\n-\t\treturn error(\"option 'chmod' expects \\\"+x\\\" or \\\"-x\\\"\");\n-\t*flip = arg[0];\n+\tint *force_mode = opt->value;\n+\tif (!strcmp(arg, \"-x\"))\n+\t\t*force_mode = 0666;\n+\telse if (!strcmp(arg, \"+x\"))\n+\t\t*force_mode = 0777;\n+\telse\n+\t\tdie(_(\"option 'chmod' expects \\\"+x\\\" or \\\"-x\\\"\"));\n+\n \treturn 0;\n }\n \n@@ -917,7 +916,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \tint read_from_stdin = 0;\n \tint prefix_length = prefix ? strlen(prefix) : 0;\n \tint preferred_index_format = 0;\n-\tchar set_executable_bit = 0;\n+\tint force_mode = 0;\n \tstruct refresh_params refresh_args = {0, &has_errors};\n \tint lock_error = 0;\n \tint split_index = -1;\n@@ -955,7 +954,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\t\tPARSE_OPT_NOARG | /* disallow --cacheinfo= form */\n \t\t\tPARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n \t\t\t(parse_opt_cb *) cacheinfo_callback},\n-\t\t{OPTION_CALLBACK, 0, \"chmod\", &set_executable_bit, N_(\"(+/-)x\"),\n+\t\t{OPTION_CALLBACK, 0, \"chmod\", &force_mode, N_(\"(+/-)x\"),\n \t\t\tN_(\"override the executable bit of the listed files\"),\n \t\t\tPARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n \t\t\tchmod_callback},\n@@ -1055,8 +1054,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\t\tsetup_work_tree();\n \t\t\tp = prefix_path(prefix, prefix_length, path);\n \t\t\tupdate_one(p);\n-\t\t\tif (set_executable_bit)\n-\t\t\t\tchmod_path(set_executable_bit, p);\n+\t\t\tif (force_mode)\n+\t\t\t\tchmod_path(force_mode, p);\n \t\t\tfree(p);\n \t\t\tctx.argc--;\n \t\t\tctx.argv++;\n@@ -1100,8 +1099,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\t\t}\n \t\t\tp = prefix_path(prefix, prefix_length, buf.buf);\n \t\t\tupdate_one(p);\n-\t\t\tif (set_executable_bit)\n-\t\t\t\tchmod_path(set_executable_bit, p);\n+\t\t\tif (force_mode)\n+\t\t\t\tchmod_path(force_mode, p);\n \t\t\tfree(p);\n \t\t}\n \t\tstrbuf_release(&unquoted);\ndiff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh\nindex dfe02f4..32ac6e0 100755\n--- a/t/t2107-update-index-basic.sh\n+++ b/t/t2107-update-index-basic.sh\n@@ -80,4 +80,17 @@ test_expect_success '.lock files cleaned up' '\n \t)\n '\n \n+test_expect_success '--chmod=+x and chmod=-x in the same argument list' '\n+\t>A &&\n+\t>B &&\n+\tgit add A B &&\n+\tgit update-index --chmod=+x A --chmod=-x B &&\n+\tcat >expect <<-\\EOF &&\n+\t100755 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0\tA\n+\t100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0\tB\n+\tEOF\n+\tgit ls-files --stage A B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.10.0.304.gf2ff484\n\n"},{"id":"301765","messageId":"20160912210818.26282-4-t.gummerer@gmail.com","threadId":"43977","inReplyTo":"20160912210818.26282-1-t.gummerer@gmail.com","subject":"[PATCH v3 3/4] read-cache: introduce chmod_index_entry","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-09-12T21:08:17Z","receivedAt":"2016-09-12T21:09:04Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"As there are chmod options for both add and update-index, introduce a\nnew chmod_index_entry function to do the work. Use it in update-index,\nwhile it will be used in add in the next patch.\n\nSigned-off-by: Thomas Gummerer \n---\n builtin/update-index.c | 8 +-------\n cache.h | 2 ++\n read-cache.c | 19 +++++++++++++++++++\n 3 files changed, 22 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 6d6cddd..809ce97 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -423,20 +423,14 @@ static void chmod_path(int force_mode, const char *path)\n {\n \tint pos;\n \tstruct cache_entry *ce;\n-\tunsigned int mode;\n \tchar flip = force_mode == 0777 ? '+' : '-';\n \n \tpos = cache_name_pos(path, strlen(path));\n \tif (pos < 0)\n \t\tgoto fail;\n \tce = active_cache[pos];\n-\tmode = ce->ce_mode;\n-\tif (!S_ISREG(mode))\n+\tif (chmod_cache_entry(ce, force_mode) < 0)\n \t\tgoto fail;\n-\tce->ce_mode = create_ce_mode(force_mode);\n-\tcache_tree_invalidate_path(&the_index, path);\n-\tce->ce_flags |= CE_UPDATE_IN_BASE;\n-\tactive_cache_changed |= CE_ENTRY_CHANGED;\n \n \treport(\"chmod %cx '%s'\", flip, path);\n \treturn;\ndiff --git a/cache.h b/cache.h\nindex b780a91..44a4f76 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -369,6 +369,7 @@ extern void free_name_hash(struct index_state *istate);\n #define remove_file_from_cache(path) remove_file_from_index(&the_index, (path))\n #define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags), 0)\n #define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags), 0)\n+#define chmod_cache_entry(ce, force_mode) chmod_index_entry(&the_index, (ce), (force_mode))\n #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL)\n #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options))\n #define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options))\n@@ -584,6 +585,7 @@ extern int remove_file_from_index(struct index_state *, const char *path);\n extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags, int force_mode);\n extern int add_file_to_index(struct index_state *, const char *path, int flags, int force_mode);\n extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, unsigned int refresh_options);\n+extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, int force_mode);\n extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);\n extern void set_object_name_for_intent_to_add_entry(struct cache_entry *ce);\n extern int index_name_is_other(const struct index_state *, const char *, int);\ndiff --git a/read-cache.c b/read-cache.c\nindex 491e52d..367be57 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -756,6 +756,25 @@ struct cache_entry *make_cache_entry(unsigned int mode,\n \treturn ret;\n }\n \n+/*\n+ * Change the mode of an index entry to force_mode, where force_mode can\n+ * either be 0777 or 0666.\n+ * Returns -1 if the chmod for the particular cache entry failed (if it's\n+ * not a regular file), 0 otherwise.\n+ */\n+int chmod_index_entry(struct index_state *istate, struct cache_entry *ce,\n+\t\t int force_mode)\n+{\n+\tif (!S_ISREG(ce->ce_mode))\n+\t\treturn -1;\n+\tce->ce_mode = create_ce_mode(force_mode);\n+\tcache_tree_invalidate_path(istate, ce->name);\n+\tce->ce_flags |= CE_UPDATE_IN_BASE;\n+\tistate->cache_changed |= CE_ENTRY_CHANGED;\n+\n+\treturn 0;\n+}\n+\n int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)\n {\n \tint len = ce_namelen(a);\n-- \n2.10.0.304.gf2ff484\n\n"},{"id":"301766","messageId":"20160912210818.26282-5-t.gummerer@gmail.com","threadId":"43977","inReplyTo":"20160912210818.26282-1-t.gummerer@gmail.com","subject":"[PATCH v3 4/4] add: modify already added files when --chmod is given","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-09-12T21:08:18Z","receivedAt":"2016-09-12T21:09:09Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"When the chmod option was added to git add, it was hooked up to the diff\nmachinery, meaning that it only works when the version in the index\ndiffers from the version on disk.\n\nAs the option was supposed to mirror the chmod option in update-index,\nwhich always changes the mode in the index, regardless of the status of\nthe file, make sure the option behaves the same way in git add.\n\nSigned-off-by: Thomas Gummerer \n---\n builtin/add.c | 36 +++++++++++++++++++++++++-----------\n builtin/checkout.c | 2 +-\n builtin/commit.c | 2 +-\n cache.h | 10 +++++-----\n read-cache.c | 14 ++++++--------\n t/t3700-add.sh | 21 +++++++++++++++++++++\n 6 files changed, 59 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex b1dddb4..892198a 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -26,10 +26,25 @@ static int patch_interactive, add_interactive, edit_interactive;\n static int take_worktree_changes;\n \n struct update_callback_data {\n-\tint flags, force_mode;\n+\tint flags;\n \tint add_errors;\n };\n \n+static void chmod_pathspec(struct pathspec *pathspec, int force_mode)\n+{\n+\tint i;\n+\t\n+\tfor (i = 0; i < active_nr; i++) {\n+\t\tstruct cache_entry *ce = active_cache[i];\n+\n+\t\tif (pathspec && !ce_path_match(ce, pathspec, NULL))\n+\t\t\tcontinue;\n+\n+\t\tif (chmod_cache_entry(ce, force_mode) < 0)\n+\t\t\tfprintf(stderr, \"cannot chmod '%s'\", ce->name);\n+\t}\n+}\n+\n static int fix_unmerged_status(struct diff_filepair *p,\n \t\t\t struct update_callback_data *data)\n {\n@@ -65,8 +80,7 @@ static void update_callback(struct diff_queue_struct *q,\n \t\t\tdie(_(\"unexpected diff status %c\"), p->status);\n \t\tcase DIFF_STATUS_MODIFIED:\n \t\tcase DIFF_STATUS_TYPE_CHANGED:\n-\t\t\tif (add_file_to_index(&the_index, path,\n-\t\t\t\t\tdata->flags, data->force_mode)) {\n+\t\t\tif (add_file_to_index(&the_index, path,\tdata->flags)) {\n \t\t\t\tif (!(data->flags & ADD_CACHE_IGNORE_ERRORS))\n \t\t\t\t\tdie(_(\"updating files failed\"));\n \t\t\t\tdata->add_errors++;\n@@ -84,15 +98,14 @@ static void update_callback(struct diff_queue_struct *q,\n \t}\n }\n \n-int add_files_to_cache(const char *prefix, const struct pathspec *pathspec,\n-\tint flags, int force_mode)\n+int add_files_to_cache(const char *prefix,\n+\t\t const struct pathspec *pathspec, int flags)\n {\n \tstruct update_callback_data data;\n \tstruct rev_info rev;\n \n \tmemset(&data, 0, sizeof(data));\n \tdata.flags = flags;\n-\tdata.force_mode = force_mode;\n \n \tinit_revisions(&rev, prefix);\n \tsetup_revisions(0, NULL, &rev, NULL);\n@@ -281,7 +294,7 @@ static int add_config(const char *var, const char *value, void *cb)\n \treturn git_default_config(var, value, cb);\n }\n \n-static int add_files(struct dir_struct *dir, int flags, int force_mode)\n+static int add_files(struct dir_struct *dir, int flags)\n {\n \tint i, exit_status = 0;\n \n@@ -294,8 +307,7 @@ static int add_files(struct dir_struct *dir, int flags, int force_mode)\n \t}\n \n \tfor (i = 0; i < dir->nr; i++)\n-\t\tif (add_file_to_index(&the_index, dir->entries[i]->name,\n-\t\t\t\tflags, force_mode)) {\n+\t\tif (add_file_to_index(&the_index, dir->entries[i]->name, flags)) {\n \t\t\tif (!ignore_add_errors)\n \t\t\t\tdie(_(\"adding files failed\"));\n \t\t\texit_status = 1;\n@@ -441,11 +453,13 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \n \tplug_bulk_checkin();\n \n-\texit_status |= add_files_to_cache(prefix, &pathspec, flags, force_mode);\n+\texit_status |= add_files_to_cache(prefix, &pathspec, flags);\n \n \tif (add_new_files)\n-\t\texit_status |= add_files(&dir, flags, force_mode);\n+\t\texit_status |= add_files(&dir, flags);\n \n+\tif (force_mode)\n+\t\tchmod_pathspec(&pathspec, force_mode);\n \tunplug_bulk_checkin();\n \n finish:\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 8672d07..a83c78f 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -548,7 +548,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t * entries in the index.\n \t\t\t */\n \n-\t\t\tadd_files_to_cache(NULL, NULL, 0, 0);\n+\t\t\tadd_files_to_cache(NULL, NULL, 0);\n \t\t\t/*\n \t\t\t * NEEDSWORK: carrying over local changes\n \t\t\t * when branches have different end-of-line\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 77e3dc8..7a1ade0 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -387,7 +387,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t */\n \tif (all || (also && pathspec.nr)) {\n \t\thold_locked_index(&index_lock, 1);\n-\t\tadd_files_to_cache(also ? prefix : NULL, &pathspec, 0, 0);\n+\t\tadd_files_to_cache(also ? prefix : NULL, &pathspec, 0);\n \t\trefresh_cache_or_die(refresh_flags);\n \t\tupdate_main_cache_tree(WRITE_TREE_SILENT);\n \t\tif (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))\ndiff --git a/cache.h b/cache.h\nindex 44a4f76..eb6d6b8 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -367,8 +367,8 @@ extern void free_name_hash(struct index_state *istate);\n #define rename_cache_entry_at(pos, new_name) rename_index_entry_at(&the_index, (pos), (new_name))\n #define remove_cache_entry_at(pos) remove_index_entry_at(&the_index, (pos))\n #define remove_file_from_cache(path) remove_file_from_index(&the_index, (path))\n-#define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags), 0)\n-#define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags), 0)\n+#define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags))\n+#define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags))\n #define chmod_cache_entry(ce, force_mode) chmod_index_entry(&the_index, (ce), (force_mode))\n #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL)\n #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options))\n@@ -582,8 +582,8 @@ extern int remove_file_from_index(struct index_state *, const char *path);\n #define ADD_CACHE_IGNORE_ERRORS\t4\n #define ADD_CACHE_IGNORE_REMOVAL 8\n #define ADD_CACHE_INTENT 16\n-extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags, int force_mode);\n-extern int add_file_to_index(struct index_state *, const char *path, int flags, int force_mode);\n+extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags);\n+extern int add_file_to_index(struct index_state *, const char *path, int flags);\n extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, unsigned int refresh_options);\n extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, int force_mode);\n extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);\n@@ -1820,7 +1820,7 @@ void packet_trace_identity(const char *prog);\n * return 0 if success, 1 - if addition of a file failed and\n * ADD_FILES_IGNORE_ERRORS was specified in flags\n */\n-int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int flags, int force_mode);\n+int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int flags);\n \n /* diff.c */\n extern int diff_auto_refresh_index;\ndiff --git a/read-cache.c b/read-cache.c\nindex 367be57..b92d72e 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -627,7 +627,7 @@ void set_object_name_for_intent_to_add_entry(struct cache_entry *ce)\n \thashcpy(ce->sha1, sha1);\n }\n \n-int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags, int force_mode)\n+int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags)\n {\n \tint size, namelen, was_same;\n \tmode_t st_mode = st->st_mode;\n@@ -656,11 +656,10 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \telse\n \t\tce->ce_flags |= CE_INTENT_TO_ADD;\n \n-\tif (S_ISREG(st_mode) && force_mode)\n-\t\tce->ce_mode = create_ce_mode(force_mode);\n-\telse if (trust_executable_bit && has_symlinks)\n+\n+\tif (trust_executable_bit && has_symlinks) {\n \t\tce->ce_mode = create_ce_mode(st_mode);\n-\telse {\n+\t} else {\n \t\t/* If there is an existing entry, pick the mode bits and type\n \t\t * from it, otherwise assume unexecutable regular file.\n \t\t */\n@@ -719,13 +718,12 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \treturn 0;\n }\n \n-int add_file_to_index(struct index_state *istate, const char *path,\n-\tint flags, int force_mode)\n+int add_file_to_index(struct index_state *istate, const char *path, int flags)\n {\n \tstruct stat st;\n \tif (lstat(path, &st))\n \t\tdie_errno(\"unable to stat '%s'\", path);\n-\treturn add_to_index(istate, path, &st, flags, force_mode);\n+\treturn add_to_index(istate, path, &st, flags);\n }\n \n struct cache_entry *make_cache_entry(unsigned int mode,\ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex 2978cb9..62c7f71 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -349,4 +349,25 @@ test_expect_success POSIXPERM,SYMLINKS 'git add --chmod=+x with symlinks' '\n \ttest_mode_in_index 100755 foo2\n '\n \n+test_expect_success 'git add --chmod=[+-]x changes index with already added file' '\n+\techo foo >foo3 &&\n+\tgit add foo3 &&\n+\tgit add --chmod=+x foo3 &&\n+\ttest_mode_in_index 100755 foo3 &&\n+\techo foo >xfoo3 &&\n+\tchmod 755 xfoo3 &&\n+\tgit add xfoo3 &&\n+\tgit add --chmod=-x xfoo3 &&\n+\ttest_mode_in_index 100644 xfoo3\n+'\n+\n+test_expect_success 'file status is changed after git add --chmod=+x' '\n+\techo \"AM foo4\" >expected &&\n+\techo foo >foo4 &&\n+\tgit add foo4 &&\n+\tgit add --chmod=+x foo4 &&\n+\tgit status -s foo4 >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n2.10.0.304.gf2ff484\n\n"},{"id":"301773","messageId":"xmqq37l4zsmo.fsf@gitster.mtv.corp.google.com","threadId":"43977","inReplyTo":"20160912210818.26282-3-t.gummerer@gmail.com","subject":"Re: [PATCH v3 2/4] update-index: use the same structure for chmod as add","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-12T21:59:43Z","receivedAt":"2016-09-12T21:59:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer writes:\n\n> +\tchar flip = force_mode == 0777 ? '+' : '-';\n> \n> \tpos = cache_name_pos(path, strlen(path));\n> \tif (pos < 0)\n> @@ -432,17 +433,11 @@ static void chmod_path(int flip, const char *path)\n> \tmode = ce->ce_mode;\n> \tif (!S_ISREG(mode))\n> \t\tgoto fail;\n> -\tswitch (flip) {\n> -\tcase '+':\n> -\t\tce->ce_mode |= 0111; break;\n> -\tcase '-':\n> -\t\tce->ce_mode &= ~0111; break;\n> -\tdefault:\n> -\t\tgoto fail;\n> -\t}\n> +\tce->ce_mode = create_ce_mode(force_mode);\n\ncreate_ce_mode() is supposed to take the st_mode taken from a\n\"struct stat\", but here force_mode is 0777 or something else. The\nabove to feed only 0777 or 0666 may happen to work with the way how\ncreate_ce_mode() is implemented now, but it is a time-bomb waiting\nto go off.\n\nInstead of using force_mode that is unsigned, keep the 'flip' as\nargument, and do:\n\n\tif (!S_ISREG(mode))\n \tgoto fail;\n+\tif (flip == '+')\n+\t\tmode |= 0111;\n+\telse\n+\t\tmode &= ~0111;\n\tce->ce_mode = create_ce_mode(mode);\n\nperhaps, as you do not need and are not using the full 9 bits in\nforce_mode anyway.\n\nThat would mean chmod_index_entry() introduced in 3/4 and its user\nin 4/4 need to be updated to pass '+' or '-' down the callchain, but\nI think that is a good change for the same reason. We do not allow\nyou to set full 9 bits; let's not pretend as if we do so by passing\njust a single bit '+' or '-' around.\n\nI think 3/4 needs to be fixed where it calls create_ce_mode() with\nonly the 0666/0777 in chmod_index_entry() anyway, regardless of the\nabove suggested change.\n\n> diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh\n> index dfe02f4..32ac6e0 100755\n> --- a/t/t2107-update-index-basic.sh\n> +++ b/t/t2107-update-index-basic.sh\n> @@ -80,4 +80,17 @@ test_expect_success '.lock files cleaned up' '\n> \t)\n> '\n> \n> +test_expect_success '--chmod=+x and chmod=-x in the same argument list' '\n> +\t>A &&\n> +\t>B &&\n> +\tgit add A B &&\n> +\tgit update-index --chmod=+x A --chmod=-x B &&\n> +\tcat >expect <<-\\EOF &&\n> +\t100755 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0\tA\n> +\t100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0\tB\n> +\tEOF\n> +\tgit ls-files --stage A B >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nThanks for adding this test. We may want to cross the b/c bridge in\na later version bump, but until then it is good to make sure we know\nwhat the currently expected behaviour is.\n"},{"id":"301776","messageId":"xmqqy42wycz5.fsf@gitster.mtv.corp.google.com","threadId":"43977","inReplyTo":"20160912210818.26282-5-t.gummerer@gmail.com","subject":"Re: [PATCH v3 4/4] add: modify already added files when --chmod is given","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-12T22:23:10Z","receivedAt":"2016-09-12T22:23:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer writes:\n\n> When the chmod option was added to git add, it was hooked up to the diff\n> machinery, meaning that it only works when the version in the index\n> differs from the version on disk.\n>\n> As the option was supposed to mirror the chmod option in update-index,\n> which always changes the mode in the index, regardless of the status of\n> the file, make sure the option behaves the same way in git add.\n>\n> Signed-off-by: Thomas Gummerer \n> ---\n> builtin/add.c | 36 +++++++++++++++++++++++++-----------\n> builtin/checkout.c | 2 +-\n> builtin/commit.c | 2 +-\n> cache.h | 10 +++++-----\n> read-cache.c | 14 ++++++--------\n> t/t3700-add.sh | 21 +++++++++++++++++++++\n> 6 files changed, 59 insertions(+), 26 deletions(-)\n\nThe change looks quite large but this in essense reverts what Edward\ndid in the first round by hooking into \"we add only modified files\nhere\" and \"we add new files here\", both of which are made unnecessary,\nbecause this adds the final \"now we finished adding things, let's\nfix modes of paths that match the pathspec\".\n\nWhich makes sense.\n\n> +static void chmod_pathspec(struct pathspec *pathspec, int force_mode)\n> +{\n> +\tint i;\n> +\t\n> +\tfor (i = 0; i < active_nr; i++) {\n> +\t\tstruct cache_entry *ce = active_cache[i];\n> +\n> +\t\tif (pathspec && !ce_path_match(ce, pathspec, NULL))\n> +\t\t\tcontinue;\n> +\n> +\t\tif (chmod_cache_entry(ce, force_mode) < 0)\n> +\t\t\tfprintf(stderr, \"cannot chmod '%s'\", ce->name);\n> +\t}\n> +}\n\nIf pathspec is NULL, this will chmod all paths in the index, which\nis probably not very useful and quite risky operation at the same\ntime.\n\nHowever ...\n\n> +\tif (force_mode)\n> +\t\tchmod_pathspec(&pathspec, force_mode);\n\n... the caller never passes a NULL as pathspec.\n\nIn any case, I somehow expected to see\n\n\tif (force_mode && pathspec.nr)\n \tchmod_pathspec(&pathspec, force_mode);\n\nbecause it would make it very easy to see in the caller that\n\n\tgit add --chmod=+x ;# no pathspec\n cd subdir && git add --chmod=+x ;# no pathspec\n\nwill be a no-op, which is what we want, if I am not mistaken. Of\ncourse, if somebody really wants to drop executable bit from\neverything, she can do\n\n\tgit add --chmod=-x .\n\npretty easily.\n\nAbove three may want to be added as new tests. The first two should\nbe a no-op, while the last one should drop executable bits from\neverywhere.\n\nThanks.\n\n\n\n"},{"id":"301928","messageId":"20160914210747.15485-1-t.gummerer@gmail.com","threadId":"43977","inReplyTo":"20160912210818.26282-1-t.gummerer@gmail.com","subject":"[PATCH v4 0/4] git add --chmod: always change the file","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-09-14T21:07:43Z","receivedAt":"2016-09-14T21:07:39Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Thanks Junio for the review of my last round.\n\nChanges since then:\n[1/4]: patch unchanged\n[2/4]: Only adds a test now, and corrects the type of the argument of\n chmod_path, but leaves the rest of the patch unchanged.\n[3/4]: chmod_index_entry now takes a char as argument which can either\n be + or -, and changes the mode based on that, instead of using\n the 0777 or 0666 mode that was passed in from the outside.\n[4/4]: Adapted to the different behaviour of chmod_index_entry and\n added tests as suggested by Junio.\n\nThomas Gummerer (4):\n add: document the chmod option\n update-index: add test for chmod flags\n read-cache: introduce chmod_index_entry\n add: modify already added files when --chmod is given\n\n Documentation/git-add.txt | 7 +++++-\n builtin/add.c | 47 ++++++++++++++++++++++++----------------\n builtin/checkout.c | 2 +-\n builtin/commit.c | 2 +-\n builtin/update-index.c | 18 +++-------------\n cache.h | 12 ++++++-----\n read-cache.c | 43 ++++++++++++++++++++++++++++++-------\n t/t2107-update-index-basic.sh | 13 +++++++++++\n t/t3700-add.sh | 50 +++++++++++++++++++++++++++++++++++++++++++\n 9 files changed, 144 insertions(+), 50 deletions(-)\n\n-- \n2.10.0.304.gf2ff484\n\n"},{"id":"301929","messageId":"20160914210747.15485-2-t.gummerer@gmail.com","threadId":"43977","inReplyTo":"20160914210747.15485-1-t.gummerer@gmail.com","subject":"[PATCH v4 1/4] add: document the chmod option","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-09-14T21:07:44Z","receivedAt":"2016-09-14T21:07:48Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"The git add --chmod option was introduced in 4e55ed3 (\"add: add\n--chmod=+x / --chmod=-x options\", 2016-05-31), but was never\ndocumented. Document the feature.\n\nSigned-off-by: Thomas Gummerer \n---\n Documentation/git-add.txt | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-add.txt b/Documentation/git-add.txt\nindex 6a96a66..7ed63dc 100644\n--- a/Documentation/git-add.txt\n+++ b/Documentation/git-add.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n 'git add' [--verbose | -v] [--dry-run | -n] [--force | -f] [--interactive | -i] [--patch | -p]\n \t [--edit | -e] [--[no-]all | --[no-]ignore-removal | [--update | -u]]\n \t [--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing]\n-\t [--] [...]\n+\t [--chmod=(+|-)x] [--] [...]\n \n DESCRIPTION\n -----------\n@@ -165,6 +165,11 @@ for \"git add --no-all ...\", i.e. ignored removed files.\n \tbe ignored, no matter if they are already present in the work\n \ttree or not.\n \n+--chmod=(+|-)x::\n+\tOverride the executable bit of the added files. The executable\n+\tbit is only changed in the index, the files on disk are left\n+\tunchanged.\n+\n \\--::\n \tThis option can be used to separate command-line options from\n \tthe list of files, (useful when filenames might be mistaken\n-- \n2.10.0.304.gf2ff484\n\n"},{"id":"301930","messageId":"20160914210747.15485-3-t.gummerer@gmail.com","threadId":"43977","inReplyTo":"20160914210747.15485-1-t.gummerer@gmail.com","subject":"[PATCH v4 2/4] update-index: add test for chmod flags","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-09-14T21:07:45Z","receivedAt":"2016-09-14T21:07:53Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Currently there is no test checking the expected behaviour when multiple\nchmod flags with different arguments are passed. As argument handling\nis not in line with other git commands it's easy to miss and\naccidentally change the current behaviour.\n\nWhile there, fix the argument type of chmod_path, which takes an int,\nbut had a char passed in.\n\nSigned-off-by: Thomas Gummerer \n---\n builtin/update-index.c | 2 +-\n t/t2107-update-index-basic.sh | 13 +++++++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex ba04b19..bbdf0d9 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -419,7 +419,7 @@ static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,\n \treturn 0;\n }\n \n-static void chmod_path(int flip, const char *path)\n+static void chmod_path(char flip, const char *path)\n {\n \tint pos;\n \tstruct cache_entry *ce;\ndiff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh\nindex dfe02f4..32ac6e0 100755\n--- a/t/t2107-update-index-basic.sh\n+++ b/t/t2107-update-index-basic.sh\n@@ -80,4 +80,17 @@ test_expect_success '.lock files cleaned up' '\n \t)\n '\n \n+test_expect_success '--chmod=+x and chmod=-x in the same argument list' '\n+\t>A &&\n+\t>B &&\n+\tgit add A B &&\n+\tgit update-index --chmod=+x A --chmod=-x B &&\n+\tcat >expect <<-\\EOF &&\n+\t100755 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0\tA\n+\t100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0\tB\n+\tEOF\n+\tgit ls-files --stage A B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.10.0.304.gf2ff484\n\n"},{"id":"301931","messageId":"20160914210747.15485-4-t.gummerer@gmail.com","threadId":"43977","inReplyTo":"20160914210747.15485-1-t.gummerer@gmail.com","subject":"[PATCH v4 3/4] read-cache: introduce chmod_index_entry","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-09-14T21:07:46Z","receivedAt":"2016-09-14T21:07:56Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"As there are chmod options for both add and update-index, introduce a\nnew chmod_index_entry function to do the work. Use it in update-index,\nwhile it will be used in add in the next patch.\n\nSigned-off-by: Thomas Gummerer \n---\n builtin/update-index.c | 16 ++--------------\n cache.h | 2 ++\n read-cache.c | 29 +++++++++++++++++++++++++++++\n 3 files changed, 33 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex bbdf0d9..9e9e040 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -423,26 +423,14 @@ static void chmod_path(char flip, const char *path)\n {\n \tint pos;\n \tstruct cache_entry *ce;\n-\tunsigned int mode;\n \n \tpos = cache_name_pos(path, strlen(path));\n \tif (pos < 0)\n \t\tgoto fail;\n \tce = active_cache[pos];\n-\tmode = ce->ce_mode;\n-\tif (!S_ISREG(mode))\n-\t\tgoto fail;\n-\tswitch (flip) {\n-\tcase '+':\n-\t\tce->ce_mode |= 0111; break;\n-\tcase '-':\n-\t\tce->ce_mode &= ~0111; break;\n-\tdefault:\n+\tif (chmod_cache_entry(ce, flip) < 0)\n \t\tgoto fail;\n-\t}\n-\tcache_tree_invalidate_path(&the_index, path);\n-\tce->ce_flags |= CE_UPDATE_IN_BASE;\n-\tactive_cache_changed |= CE_ENTRY_CHANGED;\n+\n \treport(\"chmod %cx '%s'\", flip, path);\n \treturn;\n fail:\ndiff --git a/cache.h b/cache.h\nindex 6738050..35c8d1c 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -369,6 +369,7 @@ extern void free_name_hash(struct index_state *istate);\n #define remove_file_from_cache(path) remove_file_from_index(&the_index, (path))\n #define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags), 0)\n #define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags), 0)\n+#define chmod_cache_entry(ce, flip) chmod_index_entry(&the_index, (ce), (flip))\n #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL)\n #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options))\n #define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options))\n@@ -584,6 +585,7 @@ extern int remove_file_from_index(struct index_state *, const char *path);\n extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags, int force_mode);\n extern int add_file_to_index(struct index_state *, const char *path, int flags, int force_mode);\n extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, unsigned int refresh_options);\n+extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, char flip);\n extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);\n extern void set_object_name_for_intent_to_add_entry(struct cache_entry *ce);\n extern int index_name_is_other(const struct index_state *, const char *, int);\ndiff --git a/read-cache.c b/read-cache.c\nindex 491e52d..8924f2e 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -756,6 +756,35 @@ struct cache_entry *make_cache_entry(unsigned int mode,\n \treturn ret;\n }\n \n+/*\n+ * Chmod an index entry with either +x or -x.\n+ *\n+ * Returns -1 if the chmod for the particular cache entry failed (if it's\n+ * not a regular file), -2 if an invalid flip argument is passed in, 0\n+ * otherwise.\n+ */\n+int chmod_index_entry(struct index_state *istate, struct cache_entry *ce,\n+\t\t char flip)\n+{\n+\tif (!S_ISREG(ce->ce_mode))\n+\t\treturn -1;\n+\tswitch (flip) {\n+\tcase '+':\n+\t\tce->ce_mode |= 0111;\n+\t\tbreak;\n+\tcase '-':\n+\t\tce->ce_mode &= ~0111;\n+\t\tbreak;\n+\tdefault:\n+\t\treturn -2;\n+\t}\n+\tcache_tree_invalidate_path(&the_index, ce->name);\n+\tce->ce_flags |= CE_UPDATE_IN_BASE;\n+\tistate->cache_changed |= CE_ENTRY_CHANGED;\n+\n+\treturn 0;\n+}\n+\n int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)\n {\n \tint len = ce_namelen(a);\n-- \n2.10.0.304.gf2ff484\n\n"},{"id":"301932","messageId":"20160914210747.15485-5-t.gummerer@gmail.com","threadId":"43977","inReplyTo":"20160914210747.15485-1-t.gummerer@gmail.com","subject":"[PATCH v4 4/4] add: modify already added files when --chmod is given","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-09-14T21:07:47Z","receivedAt":"2016-09-14T21:07:59Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"When the chmod option was added to git add, it was hooked up to the diff\nmachinery, meaning that it only works when the version in the index\ndiffers from the version on disk.\n\nAs the option was supposed to mirror the chmod option in update-index,\nwhich always changes the mode in the index, regardless of the status of\nthe file, make sure the option behaves the same way in git add.\n\nSigned-off-by: Thomas Gummerer \n---\n builtin/add.c | 47 ++++++++++++++++++++++++++++-------------------\n builtin/checkout.c | 2 +-\n builtin/commit.c | 2 +-\n cache.h | 10 +++++-----\n read-cache.c | 14 ++++++--------\n t/t3700-add.sh | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 91 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex b1dddb4..595a0b2 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -26,10 +26,25 @@ static int patch_interactive, add_interactive, edit_interactive;\n static int take_worktree_changes;\n \n struct update_callback_data {\n-\tint flags, force_mode;\n+\tint flags;\n \tint add_errors;\n };\n \n+static void chmod_pathspec(struct pathspec *pathspec, int force_mode)\n+{\n+\tint i;\n+\t\n+\tfor (i = 0; i < active_nr; i++) {\n+\t\tstruct cache_entry *ce = active_cache[i];\n+\n+\t\tif (pathspec && !ce_path_match(ce, pathspec, NULL))\n+\t\t\tcontinue;\n+\n+\t\tif (chmod_cache_entry(ce, force_mode) < 0)\n+\t\t\tfprintf(stderr, \"cannot chmod '%s'\", ce->name);\n+\t}\n+}\n+\n static int fix_unmerged_status(struct diff_filepair *p,\n \t\t\t struct update_callback_data *data)\n {\n@@ -65,8 +80,7 @@ static void update_callback(struct diff_queue_struct *q,\n \t\t\tdie(_(\"unexpected diff status %c\"), p->status);\n \t\tcase DIFF_STATUS_MODIFIED:\n \t\tcase DIFF_STATUS_TYPE_CHANGED:\n-\t\t\tif (add_file_to_index(&the_index, path,\n-\t\t\t\t\tdata->flags, data->force_mode)) {\n+\t\t\tif (add_file_to_index(&the_index, path,\tdata->flags)) {\n \t\t\t\tif (!(data->flags & ADD_CACHE_IGNORE_ERRORS))\n \t\t\t\t\tdie(_(\"updating files failed\"));\n \t\t\t\tdata->add_errors++;\n@@ -84,15 +98,14 @@ static void update_callback(struct diff_queue_struct *q,\n \t}\n }\n \n-int add_files_to_cache(const char *prefix, const struct pathspec *pathspec,\n-\tint flags, int force_mode)\n+int add_files_to_cache(const char *prefix,\n+\t\t const struct pathspec *pathspec, int flags)\n {\n \tstruct update_callback_data data;\n \tstruct rev_info rev;\n \n \tmemset(&data, 0, sizeof(data));\n \tdata.flags = flags;\n-\tdata.force_mode = force_mode;\n \n \tinit_revisions(&rev, prefix);\n \tsetup_revisions(0, NULL, &rev, NULL);\n@@ -281,7 +294,7 @@ static int add_config(const char *var, const char *value, void *cb)\n \treturn git_default_config(var, value, cb);\n }\n \n-static int add_files(struct dir_struct *dir, int flags, int force_mode)\n+static int add_files(struct dir_struct *dir, int flags)\n {\n \tint i, exit_status = 0;\n \n@@ -294,8 +307,7 @@ static int add_files(struct dir_struct *dir, int flags, int force_mode)\n \t}\n \n \tfor (i = 0; i < dir->nr; i++)\n-\t\tif (add_file_to_index(&the_index, dir->entries[i]->name,\n-\t\t\t\tflags, force_mode)) {\n+\t\tif (add_file_to_index(&the_index, dir->entries[i]->name, flags)) {\n \t\t\tif (!ignore_add_errors)\n \t\t\t\tdie(_(\"adding files failed\"));\n \t\t\texit_status = 1;\n@@ -308,7 +320,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \tint exit_status = 0;\n \tstruct pathspec pathspec;\n \tstruct dir_struct dir;\n-\tint flags, force_mode;\n+\tint flags;\n \tint add_new_files;\n \tint require_pathspec;\n \tchar *seen = NULL;\n@@ -342,13 +354,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \tif (!show_only && ignore_missing)\n \t\tdie(_(\"Option --ignore-missing can only be used together with --dry-run\"));\n \n-\tif (!chmod_arg)\n-\t\tforce_mode = 0;\n-\telse if (!strcmp(chmod_arg, \"-x\"))\n-\t\tforce_mode = 0666;\n-\telse if (!strcmp(chmod_arg, \"+x\"))\n-\t\tforce_mode = 0777;\n-\telse\n+\tif (chmod_arg && ((chmod_arg[0] != '-' && chmod_arg[0] != '+') ||\n+\t\t\t chmod_arg[1] != 'x' || chmod_arg[2]))\n \t\tdie(_(\"--chmod param '%s' must be either -x or +x\"), chmod_arg);\n \n \tadd_new_files = !take_worktree_changes && !refresh_only;\n@@ -441,11 +448,13 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \n \tplug_bulk_checkin();\n \n-\texit_status |= add_files_to_cache(prefix, &pathspec, flags, force_mode);\n+\texit_status |= add_files_to_cache(prefix, &pathspec, flags);\n \n \tif (add_new_files)\n-\t\texit_status |= add_files(&dir, flags, force_mode);\n+\t\texit_status |= add_files(&dir, flags);\n \n+\tif (chmod_arg && pathspec.nr)\n+\t\tchmod_pathspec(&pathspec, chmod_arg[0]);\n \tunplug_bulk_checkin();\n \n finish:\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 8672d07..a83c78f 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -548,7 +548,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t * entries in the index.\n \t\t\t */\n \n-\t\t\tadd_files_to_cache(NULL, NULL, 0, 0);\n+\t\t\tadd_files_to_cache(NULL, NULL, 0);\n \t\t\t/*\n \t\t\t * NEEDSWORK: carrying over local changes\n \t\t\t * when branches have different end-of-line\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex bb9f79b..1cba3b7 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -397,7 +397,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t */\n \tif (all || (also && pathspec.nr)) {\n \t\thold_locked_index(&index_lock, 1);\n-\t\tadd_files_to_cache(also ? prefix : NULL, &pathspec, 0, 0);\n+\t\tadd_files_to_cache(also ? prefix : NULL, &pathspec, 0);\n \t\trefresh_cache_or_die(refresh_flags);\n \t\tupdate_main_cache_tree(WRITE_TREE_SILENT);\n \t\tif (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))\ndiff --git a/cache.h b/cache.h\nindex 35c8d1c..cd8e9fe 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -367,8 +367,8 @@ extern void free_name_hash(struct index_state *istate);\n #define rename_cache_entry_at(pos, new_name) rename_index_entry_at(&the_index, (pos), (new_name))\n #define remove_cache_entry_at(pos) remove_index_entry_at(&the_index, (pos))\n #define remove_file_from_cache(path) remove_file_from_index(&the_index, (path))\n-#define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags), 0)\n-#define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags), 0)\n+#define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags))\n+#define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags))\n #define chmod_cache_entry(ce, flip) chmod_index_entry(&the_index, (ce), (flip))\n #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL)\n #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options))\n@@ -582,8 +582,8 @@ extern int remove_file_from_index(struct index_state *, const char *path);\n #define ADD_CACHE_IGNORE_ERRORS\t4\n #define ADD_CACHE_IGNORE_REMOVAL 8\n #define ADD_CACHE_INTENT 16\n-extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags, int force_mode);\n-extern int add_file_to_index(struct index_state *, const char *path, int flags, int force_mode);\n+extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags);\n+extern int add_file_to_index(struct index_state *, const char *path, int flags);\n extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, unsigned int refresh_options);\n extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, char flip);\n extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);\n@@ -1821,7 +1821,7 @@ void packet_trace_identity(const char *prog);\n * return 0 if success, 1 - if addition of a file failed and\n * ADD_FILES_IGNORE_ERRORS was specified in flags\n */\n-int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int flags, int force_mode);\n+int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int flags);\n \n /* diff.c */\n extern int diff_auto_refresh_index;\ndiff --git a/read-cache.c b/read-cache.c\nindex 8924f2e..016bbcb 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -627,7 +627,7 @@ void set_object_name_for_intent_to_add_entry(struct cache_entry *ce)\n \thashcpy(ce->sha1, sha1);\n }\n \n-int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags, int force_mode)\n+int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags)\n {\n \tint size, namelen, was_same;\n \tmode_t st_mode = st->st_mode;\n@@ -656,11 +656,10 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \telse\n \t\tce->ce_flags |= CE_INTENT_TO_ADD;\n \n-\tif (S_ISREG(st_mode) && force_mode)\n-\t\tce->ce_mode = create_ce_mode(force_mode);\n-\telse if (trust_executable_bit && has_symlinks)\n+\n+\tif (trust_executable_bit && has_symlinks) {\n \t\tce->ce_mode = create_ce_mode(st_mode);\n-\telse {\n+\t} else {\n \t\t/* If there is an existing entry, pick the mode bits and type\n \t\t * from it, otherwise assume unexecutable regular file.\n \t\t */\n@@ -719,13 +718,12 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \treturn 0;\n }\n \n-int add_file_to_index(struct index_state *istate, const char *path,\n-\tint flags, int force_mode)\n+int add_file_to_index(struct index_state *istate, const char *path, int flags)\n {\n \tstruct stat st;\n \tif (lstat(path, &st))\n \t\tdie_errno(\"unable to stat '%s'\", path);\n-\treturn add_to_index(istate, path, &st, flags, force_mode);\n+\treturn add_to_index(istate, path, &st, flags);\n }\n \n struct cache_entry *make_cache_entry(unsigned int mode,\ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex 2978cb9..0a962a6 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -349,4 +349,54 @@ test_expect_success POSIXPERM,SYMLINKS 'git add --chmod=+x with symlinks' '\n \ttest_mode_in_index 100755 foo2\n '\n \n+test_expect_success 'git add --chmod=[+-]x changes index with already added file' '\n+\techo foo >foo3 &&\n+\tgit add foo3 &&\n+\tgit add --chmod=+x foo3 &&\n+\ttest_mode_in_index 100755 foo3 &&\n+\techo foo >xfoo3 &&\n+\tchmod 755 xfoo3 &&\n+\tgit add xfoo3 &&\n+\tgit add --chmod=-x xfoo3 &&\n+\ttest_mode_in_index 100644 xfoo3\n+'\n+\n+test_expect_success 'file status is changed after git add --chmod=+x' '\n+\techo \"AM foo4\" >expected &&\n+\techo foo >foo4 &&\n+\tgit add foo4 &&\n+\tgit add --chmod=+x foo4 &&\n+\tgit status -s foo4 >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'no file status change if no pathspec is given' '\n+\t>foo5 &&\n+\t>foo6 &&\n+\tgit add foo5 foo6 &&\n+\tgit add --chmod=+x &&\n+\ttest_mode_in_index 100644 foo5 &&\n+\ttest_mode_in_index 100644 foo6\n+'\n+\n+test_expect_success 'no file status change if no pathspec is given in subdir' '\n+\tmkdir sub &&\n+\t(\n+\t\tcd sub &&\n+\t\t>sub-foo1 &&\n+\t\t>sub-foo2 &&\n+\t\tgit add . &&\n+\t\tgit add --chmod=+x &&\n+\t\ttest_mode_in_index 100644 sub-foo1 &&\n+\t\ttest_mode_in_index 100644 sub-foo2\n+\t)\n+'\n+\n+test_expect_success 'all statuses changed in folder if . is given' '\n+\tgit add --chmod=+x . &&\n+\ttest $(git ls-files --stage | grep ^100644 | wc -l) -eq 0 &&\n+\tgit add --chmod=-x . &&\n+\ttest $(git ls-files --stage | grep ^100755 | wc -l) -eq 0\n+'\n+\n test_done\n-- \n2.10.0.304.gf2ff484\n\n"},{"id":"301933","messageId":"xmqqbmzqqhm7.fsf@gitster.mtv.corp.google.com","threadId":"43977","inReplyTo":"20160914210747.15485-4-t.gummerer@gmail.com","subject":"Re: [PATCH v4 3/4] read-cache: introduce chmod_index_entry","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-14T21:46:56Z","receivedAt":"2016-09-14T21:47:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer writes:\n\n> As there are chmod options for both add and update-index, introduce a\n> new chmod_index_entry function to do the work. Use it in update-index,\n> while it will be used in add in the next patch.\n>\n> Signed-off-by: Thomas Gummerer \n> ---\n> builtin/update-index.c | 16 ++--------------\n> cache.h | 2 ++\n> read-cache.c | 29 +++++++++++++++++++++++++++++\n> 3 files changed, 33 insertions(+), 14 deletions(-)\n>\n> diff --git a/builtin/update-index.c b/builtin/update-index.c\n> index bbdf0d9..9e9e040 100644\n> --- a/builtin/update-index.c\n> +++ b/builtin/update-index.c\n> @@ -423,26 +423,14 @@ static void chmod_path(char flip, const char *path)\n> {\n> ...\n> -\tmode = ce->ce_mode;\n> -\tif (!S_ISREG(mode))\n> -\t\tgoto fail;\n> -\tswitch (flip) {\n> -\tcase '+':\n> -\t\tce->ce_mode |= 0111; break;\n> -\tcase '-':\n> -\t\tce->ce_mode &= ~0111; break;\n> -\tdefault:\n> +\tif (chmod_cache_entry(ce, flip) < 0)\n> \t\tgoto fail;\n> -\t}\n> -\tcache_tree_invalidate_path(&the_index, path);\n\nThis used to always work on the default index, hence the_index\nreference is here, but ...\n\n> +int chmod_index_entry(struct index_state *istate, struct cache_entry *ce,\n> +\t\t char flip)\n> +{\n> +\tif (!S_ISREG(ce->ce_mode))\n> +\t\treturn -1;\n> +\tswitch (flip) {\n> +\tcase '+':\n> +\t\tce->ce_mode |= 0111;\n> +\t\tbreak;\n> +\tcase '-':\n> +\t\tce->ce_mode &= ~0111;\n> +\t\tbreak;\n> +\tdefault:\n> +\t\treturn -2;\n> +\t}\n> +\tcache_tree_invalidate_path(&the_index, ce->name);\n\n... this one takes istate, so you need to use it, instead of the\nhard-coded the_index reference.\n\n> +\tce->ce_flags |= CE_UPDATE_IN_BASE;\n> +\tistate->cache_changed |= CE_ENTRY_CHANGED;\n> +\n> +\treturn 0;\n> +}\n> +\n> int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)\n> {\n> \tint len = ce_namelen(a);\n\nOther than that, this looks good to me.\n"},{"id":"301934","messageId":"xmqq4m5iqh99.fsf@gitster.mtv.corp.google.com","threadId":"43977","inReplyTo":"20160914210747.15485-5-t.gummerer@gmail.com","subject":"Re: [PATCH v4 4/4] add: modify already added files when --chmod is given","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-14T21:54:42Z","receivedAt":"2016-09-14T21:54:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer writes:\n\n> When the chmod option was added to git add, it was hooked up to the diff\n> machinery, meaning that it only works when the version in the index\n> differs from the version on disk.\n>\n> As the option was supposed to mirror the chmod option in update-index,\n> which always changes the mode in the index, regardless of the status of\n> the file, make sure the option behaves the same way in git add.\n>\n> Signed-off-by: Thomas Gummerer \n> ---\n\nThis change essentially reverts most of what 4e55ed32 (\"add: add\n--chmod=+x / --chmod=-x options\", 2016-05-31) did, except that it\nkeeps the command line option and adjusts its validation, and adds a\nseparate phase to \"fix-up\" the executable bits for all paths that\nmatch the given pathspec, whether they were new or modified or\nunchanged.\n\nThe patch makes sense to me. Thanks.\n"},{"id":"301937","messageId":"xmqqsht2ozwx.fsf@gitster.mtv.corp.google.com","threadId":"43977","inReplyTo":"xmqqbmzqqhm7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 3/4] read-cache: introduce chmod_index_entry","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-14T22:54:38Z","receivedAt":"2016-09-14T22:54:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I've queued this trivial SQUASH??? on top, which I think should be\nsquashed into 3/4.\n\nThanks.\n\n\n read-cache.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 2445e30..c2b2e97 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -779,7 +779,7 @@ int chmod_index_entry(struct index_state *istate, struct cache_entry *ce,\n \tdefault:\n \t\treturn -2;\n \t}\n-\tcache_tree_invalidate_path(&the_index, ce->name);\n+\tcache_tree_invalidate_path(istate, ce->name);\n \tce->ce_flags |= CE_UPDATE_IN_BASE;\n \tistate->cache_changed |= CE_ENTRY_CHANGED;\n \n-- \n2.10.0-458-g8cce42d\n\n"},{"id":"301997","messageId":"20160915184906.GE8254@hank","threadId":"43977","inReplyTo":"xmqqsht2ozwx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 3/4] read-cache: introduce chmod_index_entry","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-09-15T18:49:06Z","receivedAt":"2016-09-15T18:48:57Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 09/14, Junio C Hamano wrote:\n> I've queued this trivial SQUASH??? on top, which I think should be\n> squashed into 3/4.\n\nYeah, I missed this. The SQUASH??? definitely makes sense, would be\ngreat if you could just squash that in.\n\n> Thanks.\n\nThanks the reviews and helping me getting the series in a good shape!\n\n> \n> \n> read-cache.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/read-cache.c b/read-cache.c\n> index 2445e30..c2b2e97 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -779,7 +779,7 @@ int chmod_index_entry(struct index_state *istate, struct cache_entry *ce,\n> \tdefault:\n> \t\treturn -2;\n> \t}\n> -\tcache_tree_invalidate_path(&the_index, ce->name);\n> +\tcache_tree_invalidate_path(istate, ce->name);\n> \tce->ce_flags |= CE_UPDATE_IN_BASE;\n> \tistate->cache_changed |= CE_ENTRY_CHANGED;\n> \n> -- \n> 2.10.0-458-g8cce42d\n> \n\n-- \nThomas\n"},{"id":"325763","messageId":"3c61d9f6-e0fd-22a4-68e0-89fd9ce9b944@web.de","threadId":"43977","inReplyTo":"20160914210747.15485-5-t.gummerer@gmail.com","subject":"Re: [PATCH v4 4/4] add: modify already added files when --chmod is given","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-08-07T21:40:30Z","receivedAt":"2017-08-07T21:40:58Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 14.09.2016 um 23:07 schrieb Thomas Gummerer:\n> When the chmod option was added to git add, it was hooked up to the diff\n> machinery, meaning that it only works when the version in the index\n> differs from the version on disk.\n> \n> As the option was supposed to mirror the chmod option in update-index,\n> which always changes the mode in the index, regardless of the status of\n> the file, make sure the option behaves the same way in git add.\n> \n> Signed-off-by: Thomas Gummerer \n\nSorry for replying almost a year late, hopefully you're still interested.\n\n> ---\n> builtin/add.c | 47 ++++++++++++++++++++++++++++-------------------\n> builtin/checkout.c | 2 +-\n> builtin/commit.c | 2 +-\n> cache.h | 10 +++++-----\n> read-cache.c | 14 ++++++--------\n> t/t3700-add.sh | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++\n> 6 files changed, 91 insertions(+), 34 deletions(-)\n> \n> diff --git a/builtin/add.c b/builtin/add.c\n> index b1dddb4..595a0b2 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -26,10 +26,25 @@ static int patch_interactive, add_interactive, edit_interactive;\n> static int take_worktree_changes;\n> \n> struct update_callback_data {\n> -\tint flags, force_mode;\n> +\tint flags;\n> \tint add_errors;\n> };\n> \n> +static void chmod_pathspec(struct pathspec *pathspec, int force_mode)\n\n\"int force_mode\" looks like a binary (or perhaps ternary) flag, but\nactually it is a character and can only have the values '-' or '+'.\nIn builtin/update-index.c it's called \"char flip\" and we probably should\ndefine it like this here as well.\n\n> +{\n> +\tint i;\n> +\t\n> +\tfor (i = 0; i < active_nr; i++) {\n> +\t\tstruct cache_entry *ce = active_cache[i];\n> +\n> +\t\tif (pathspec && !ce_path_match(ce, pathspec, NULL))\n> +\t\t\tcontinue;\n> +\n> +\t\tif (chmod_cache_entry(ce, force_mode) < 0)\n> +\t\t\tfprintf(stderr, \"cannot chmod '%s'\", ce->name);\n\nThis error message is missing a newline. In builtin/update-index.c we\nalso show the attempted change (-x or +x); perhaps we want to do that\nhere as well.\n\nCurrently chmod_cache_entry() can only fail if ce is not a regular\nfile or it's other parameter is neither '-' nor '+'. We rule out the\nlatter already in the argument parsing code. The former can happen if\nwe add a symlink, either explicitly or because it's in a directory\nwe're specified.\n\nI wonder if we even need to report anything, or under which conditions.\nIf you have a file named dir/file and a symlink named dir/symlink then\nthe interesting cases are:\n\n\tgit add --chmod=.. dir/symlink\n\tgit add --chmod=.. dir/file dir/symlink\n\tgit add --chmod=.. dir\n\nWarning about each case may be the most cautious thing to do, but\ndocumenting that --chmod has no effect on symlinks and keeping silent\nmight be less annoying, especially in the last case. What do you\nthink?\n\n> @@ -342,13 +354,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n> \tif (!show_only && ignore_missing)\n> \t\tdie(_(\"Option --ignore-missing can only be used together with --dry-run\"));\n> \n> -\tif (!chmod_arg)\n> -\t\tforce_mode = 0;\n> -\telse if (!strcmp(chmod_arg, \"-x\"))\n> -\t\tforce_mode = 0666;\n> -\telse if (!strcmp(chmod_arg, \"+x\"))\n> -\t\tforce_mode = 0777;\n> -\telse\n> +\tif (chmod_arg && ((chmod_arg[0] != '-' && chmod_arg[0] != '+') ||\n> +\t\t\t chmod_arg[1] != 'x' || chmod_arg[2]))\n> \t\tdie(_(\"--chmod param '%s' must be either -x or +x\"), chmod_arg);\n\nThat's the argument parsing code mentioned above. The strcmp-based\nchecks look nicer to me btw. How about this?\n\n\tif (chmod_arg && strcmp(chmod_arg, \"-x\") && strcmp(chmod_arg, \"+x\"))\n\nBut that's just nitpicking.\n\nRené\n"},{"id":"326213","messageId":"20170812123010.GA4304@hank","threadId":"43977","inReplyTo":"3c61d9f6-e0fd-22a4-68e0-89fd9ce9b944@web.de","subject":"Re: [PATCH v4 4/4] add: modify already added files when --chmod is given","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2017-08-12T12:30:10Z","receivedAt":"2017-08-12T12:30:13Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 08/07, René Scharfe wrote:\n> Am 14.09.2016 um 23:07 schrieb Thomas Gummerer:\n> > When the chmod option was added to git add, it was hooked up to the diff\n> > machinery, meaning that it only works when the version in the index\n> > differs from the version on disk.\n> > \n> > As the option was supposed to mirror the chmod option in update-index,\n> > which always changes the mode in the index, regardless of the status of\n> > the file, make sure the option behaves the same way in git add.\n> > \n> > Signed-off-by: Thomas Gummerer \n> \n> Sorry for replying almost a year late, hopefully you're still interested.\n\nThanks for still replying :)\n\n> > ---\n> > builtin/add.c | 47 ++++++++++++++++++++++++++++-------------------\n> > builtin/checkout.c | 2 +-\n> > builtin/commit.c | 2 +-\n> > cache.h | 10 +++++-----\n> > read-cache.c | 14 ++++++--------\n> > t/t3700-add.sh | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++\n> > 6 files changed, 91 insertions(+), 34 deletions(-)\n> > \n> > diff --git a/builtin/add.c b/builtin/add.c\n> > index b1dddb4..595a0b2 100644\n> > --- a/builtin/add.c\n> > +++ b/builtin/add.c\n> > @@ -26,10 +26,25 @@ static int patch_interactive, add_interactive, edit_interactive;\n> > static int take_worktree_changes;\n> > \n> > struct update_callback_data {\n> > -\tint flags, force_mode;\n> > +\tint flags;\n> > \tint add_errors;\n> > };\n> > \n> > +static void chmod_pathspec(struct pathspec *pathspec, int force_mode)\n> \n> \"int force_mode\" looks like a binary (or perhaps ternary) flag, but\n> actually it is a character and can only have the values '-' or '+'.\n> In builtin/update-index.c it's called \"char flip\" and we probably should\n> define it like this here as well.\n> \n> > +{\n> > +\tint i;\n> > +\t\n> > +\tfor (i = 0; i < active_nr; i++) {\n> > +\t\tstruct cache_entry *ce = active_cache[i];\n> > +\n> > +\t\tif (pathspec && !ce_path_match(ce, pathspec, NULL))\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tif (chmod_cache_entry(ce, force_mode) < 0)\n> > +\t\t\tfprintf(stderr, \"cannot chmod '%s'\", ce->name);\n> \n> This error message is missing a newline. In builtin/update-index.c we\n> also show the attempted change (-x or +x); perhaps we want to do that\n> here as well.\n\nThanks for catching this, both this and the above are worth changing\nimo. I see Ramsay already provided a patch for this in\nhttps://public-inbox.org/git/aa004526-3e0d-66d4-287f-30abd29758fc@ramsayjones.plus.com/.\nThanks Ramsay!\n\n> Currently chmod_cache_entry() can only fail if ce is not a regular\n> file or it's other parameter is neither '-' nor '+'. We rule out the\n> latter already in the argument parsing code. The former can happen if\n> we add a symlink, either explicitly or because it's in a directory\n> we're specified.\n> \n> I wonder if we even need to report anything, or under which conditions.\n> If you have a file named dir/file and a symlink named dir/symlink then\n> the interesting cases are:\n> \n> \tgit add --chmod=.. dir/symlink\n> \tgit add --chmod=.. dir/file dir/symlink\n> \tgit add --chmod=.. dir\n> \n> Warning about each case may be the most cautious thing to do, but\n> documenting that --chmod has no effect on symlinks and keeping silent\n> might be less annoying, especially in the last case. What do you\n> think?\n\nI'm not sure about this. While I do agree that it could be quite\nannoying in the last case, it could potentially be a bit confusing to\nnot get any warning in the first case. I don't have a strong opinion\neither way.\n\n> > @@ -342,13 +354,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n> > \tif (!show_only && ignore_missing)\n> > \t\tdie(_(\"Option --ignore-missing can only be used together with --dry-run\"));\n> > \n> > -\tif (!chmod_arg)\n> > -\t\tforce_mode = 0;\n> > -\telse if (!strcmp(chmod_arg, \"-x\"))\n> > -\t\tforce_mode = 0666;\n> > -\telse if (!strcmp(chmod_arg, \"+x\"))\n> > -\t\tforce_mode = 0777;\n> > -\telse\n> > +\tif (chmod_arg && ((chmod_arg[0] != '-' && chmod_arg[0] != '+') ||\n> > +\t\t\t chmod_arg[1] != 'x' || chmod_arg[2]))\n> > \t\tdie(_(\"--chmod param '%s' must be either -x or +x\"), chmod_arg);\n> \n> That's the argument parsing code mentioned above. The strcmp-based\n> checks look nicer to me btw. How about this?\n> \n> \tif (chmod_arg && strcmp(chmod_arg, \"-x\") && strcmp(chmod_arg, \"+x\"))\n> \n> But that's just nitpicking.\n\nI think this looks nicer indeed, thanks! But it's probably not worth\na patch for just this unless we decide to change to not warn as you\nmentioned above, and can fix this at the same time?\n\n> René\n"}]}