{"thread":{"id":"42445","subject":"[PATCH] add: add --chmod=+x / --chmod=-x options","startedAt":"2016-05-25T02:06:09Z","lastAt":"2016-05-31T22:06:50Z","messageCount":14,"participants":["Edward Thomson","Junio C Hamano","Johannes Schindelin","Mike Hommey"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"287473","messageId":"20160525020609.GA20123@zoidberg","threadId":"42445","inReplyTo":null,"subject":"[PATCH] add: add --chmod=+x / --chmod=-x options","fromName":"Edward Thomson","fromEmail":"ethomson@edwardthomson.com","sentAt":"2016-05-25T02:06:09Z","receivedAt":"2016-05-25T02:06:09Z","isPatch":true,"sender":{"key":"ethomson@edwardthomson.com","avatar":"https://avatars.githubusercontent.com/u/1130014?v=4"},"body":"Users on deficient filesystems that lack an execute bit may still\nwish to add files to the repository with the appropriate execute\nbit set (or not).  Although this can be done in two steps\n(`git add foo && git update-index --chmod=+x foo`), providing the\n`--chmod=+x` option to the add command allows users to set a file\nexecutable in a single command that they're already familiar with.\n\nSigned-off-by: Edward Thomson <ethomson@edwardthomson.com>\n---\n builtin/add.c  | 18 +++++++++++++++++-\n cache.h        |  2 ++\n read-cache.c   |  6 ++++++\n t/t3700-add.sh | 19 +++++++++++++++++++\n 4 files changed, 44 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 145f06e..2a9abf7 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -238,6 +238,8 @@ static int ignore_add_errors, intent_to_add, ignore_missing;\n static int addremove = ADDREMOVE_DEFAULT;\n static int addremove_explicit = -1; /* unspecified */\n \n+static char should_chmod = 0;\n+\n static int ignore_removal_cb(const struct option *opt, const char *arg, int unset)\n {\n \t/* if we are told to ignore, we are not adding removals */\n@@ -245,6 +247,15 @@ static int ignore_removal_cb(const struct option *opt, const char *arg, int unse\n \treturn 0;\n }\n \n+static int chmod_cb(const struct option *opt, 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+\treturn 0;\n+}\n+\n static struct option builtin_add_options[] = {\n \tOPT__DRY_RUN(&show_only, N_(\"dry run\")),\n \tOPT__VERBOSE(&verbose, N_(\"be verbose\")),\n@@ -263,6 +274,9 @@ static struct option builtin_add_options[] = {\n \tOPT_BOOL( 0 , \"refresh\", &refresh_only, N_(\"don't add, only refresh the index\")),\n \tOPT_BOOL( 0 , \"ignore-errors\", &ignore_add_errors, N_(\"just skip files which cannot be added because of errors\")),\n \tOPT_BOOL( 0 , \"ignore-missing\", &ignore_missing, N_(\"check if - even missing - files are ignored in dry run\")),\n+\t{ OPTION_CALLBACK, 0, \"chmod\", &should_chmod, N_(\"(+/-)x\"),\n+\t  N_(\"override the executable bit of the listed files\"),\n+\t  PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP, chmod_cb},\n \tOPT_END(),\n };\n \n@@ -346,7 +360,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\t (intent_to_add ? ADD_CACHE_INTENT : 0) |\n \t\t (ignore_add_errors ? ADD_CACHE_IGNORE_ERRORS : 0) |\n \t\t (!(addremove || take_worktree_changes)\n-\t\t  ? ADD_CACHE_IGNORE_REMOVAL : 0));\n+\t\t  ? ADD_CACHE_IGNORE_REMOVAL : 0)) |\n+\t\t (should_chmod == '+' ? ADD_CACHE_FORCE_EXECUTABLE : 0) |\n+\t\t (should_chmod == '-' ? ADD_CACHE_FORCE_NOTEXECUTABLE : 0);\n \n \tif (require_pathspec && argc == 0) {\n \t\tfprintf(stderr, _(\"Nothing specified, nothing added.\\n\"));\ndiff --git a/cache.h b/cache.h\nindex 6049f86..da03cd9 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -581,6 +581,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+#define ADD_CACHE_FORCE_EXECUTABLE 32\n+#define ADD_CACHE_FORCE_NOTEXECUTABLE 64\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);\ndiff --git a/read-cache.c b/read-cache.c\nindex d9fb78b..81bf186 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -641,6 +641,8 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \tint intent_only = flags & ADD_CACHE_INTENT;\n \tint add_option = (ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE|\n \t\t\t  (intent_only ? ADD_CACHE_NEW_ONLY : 0));\n+\tint force_executable = flags & ADD_CACHE_FORCE_EXECUTABLE;\n+\tint force_notexecutable = flags & ADD_CACHE_FORCE_NOTEXECUTABLE;\n \n \tif (!S_ISREG(st_mode) && !S_ISLNK(st_mode) && !S_ISDIR(st_mode))\n \t\treturn error(\"%s: can only add regular files, symbolic links or git-directories\", path);\n@@ -661,6 +663,10 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \n \tif (trust_executable_bit && has_symlinks)\n \t\tce->ce_mode = create_ce_mode(st_mode);\n+\telse if (force_executable)\n+\t\tce->ce_mode = create_ce_mode(0777);\n+\telse if (force_notexecutable)\n+\t\tce->ce_mode = create_ce_mode(0666);\n \telse {\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.\ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex f14a665..e551eaf 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -332,4 +332,23 @@ test_expect_success 'git add --dry-run --ignore-missing of non-existing file out\n \ttest_i18ncmp expect.err actual.err\n '\n \n+test_expect_success 'git add --chmod=+x stages a non-executable file with +x' '\n+\techo foo >foo1 &&\n+\tgit add --chmod=+x foo1 &&\n+\tcase \"$(git ls-files --stage foo1)\" in\n+\t100755\" \"*foo1) echo pass;;\n+\t*) echo fail; git ls-files --stage foo1; (exit 1);;\n+\tesac\n+'\n+\n+test_expect_success 'git add --chmod=-x stages an executable file with -x' '\n+\techo foo >xfoo1 &&\n+\tchmod 755 xfoo1 &&\n+\tgit add --chmod=-x xfoo1 &&\n+\tcase \"$(git ls-files --stage xfoo1)\" in\n+\t100644\" \"*xfoo1) echo pass;;\n+\t*) echo fail; git ls-files --stage xfoo1; (exit 1);;\n+\tesac\n+'\n+\n test_done\n-- \n2.6.4 (Apple Git-63)\n"},{"id":"287488","messageId":"xmqqh9dm37xk.fsf@gitster.mtv.corp.google.com","threadId":"42445","inReplyTo":"20160525020609.GA20123@zoidberg","subject":"Re: [PATCH] add: add --chmod=+x / --chmod=-x options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-25T07:36:55Z","receivedAt":"2016-05-25T07:36:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Edward Thomson <ethomson@edwardthomson.com> writes:\n\n> Users on deficient filesystems that lack an execute bit may still\n> wish to add files to the repository with the appropriate execute\n> bit set (or not).  Although this can be done in two steps\n> (`git add foo && git update-index --chmod=+x foo`), providing the\n> `--chmod=+x` option to the add command allows users to set a file\n> executable in a single command that they're already familiar with.\n>\n> Signed-off-by: Edward Thomson <ethomson@edwardthomson.com>\n> ---\n\nI think we should tone down the first sentence of the proposed\ncommit log message.  With \"s/deficient //\" the paragraph still reads\nperfectly well.  Even when an underlying filesystem is capable of\nexpressing executable bit, we can set core.filemode to false to\nemulate the behaviour on DOS, so perhaps\n\n    The executable bit will not be set for paths in a repository\n    with core.filemode set to false, but the users may still wish to\n    add files to ...\n\nor something like it?\n\nGiving an easy to use single-command short-hand for a common thing\nthat takes two commands is a worthy goal.  Another way to do the\nabove is\n\n\tgit update-index --add --chmod=+x foo\n\nand from that point of view, we do not need this patch, but that is\nstill a mouthful ;-)  I think it is a good idea to teach \"git add\",\nan end-user facing command, to be more helpful.\n\nAt the design level, I have a few comments.\n\n * Unlike the command \"chmod\", which is _only_ about changing modes,\n   \"add --chmod\" updates both the contents and modes in the index,\n   which may invite \"I only want to change modes--how?\"\n\n\tNote. We had an ancient regression at 227bdb18 (make\n\tupdate-index --chmod work with multiple files and --stdin,\n\t2006-04-23), where \"update-index --chmod=+x foo\" stopped\n\tbeing \"only flip the executable bit without hashing the\n\tcontents\" and that was done purely by mistake.  There is no\n\tlonger a good answer to that question, which makes the above\n\tworry less of an issue.\n\n * This is about a repository with core.filemode=0; I wonder if\n   something for a repository with core.symlinks=0 would also help?\n   That is, would it be a big help to users if they can prepare a\n   text file that holds symbolic link contents and add it as if it\n   were a symlink with \"git add\", instead of having to run two\n   commands, \"hash-objects && update-index --cacheinfo\"?\n\n * I am not familiar with life on filesystems with core.filemode=0;\n   do files people would want to be able to \"add --chmod=+x\" share\n   common trait that can be expressed with .gitattributes mechanism?\n\n   What I am wondering is if a scheme like the following would work\n   well, in addition to your patch:\n\n   1. Have these in .gitattributes:\n\n      *\t\t-executable\n      *.bat\texecutable text\n      *.exe\texecutable binary\n      *.com\texecutable binary\n\n      A path with Unset \"executable\" attribute is explicitly marked\n      as \"not executable\"; a path with Set \"executable\" attribute is\n      marked as \"executable\", i.e. \"needing chmod=+x\".\n\n   2. Teach \"git add\" to take the above hint _only_ in a repository\n      where core.filemode is false and _only_ when adding a new path\n      to the index.\n\n   If something like this works well enough, users do not have to\n   type --chmod=+x too often when doing \"git add\"; your patch\n   becomes an escape hatch that is only needed when the attributes\n   system gets it wrong.\n\n\nNow some comments on the actual code.\n\n> +static int chmod_cb(const struct option *opt, 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> +\treturn 0;\n> +}\n\nI know you mimicked the command line parser of update-index, but you\ndidn't have to, and you shouldn't have.\n\nThe command line semantics of update-index is largely \"we read one\noption and prepare to make its effect immediately available\", which\npredates parse-options where its attitude for command line parsing\nis \"we first parse all options and figure out what to do, and then\nwe work on arguments according to these options\".  Because of these\nvastly different attitudes, the way builtin/update-index.c uses\nparse-options API is atypical.  The only reason it uses callback is\nbecause it wants to allow you to say this:\n\n    git update-index --chmod=+x foo bar --chmod=-x baz\n\nand register foo and bar as executable, while baz as non-executable.\n\nThe way update-index uses parse-options API is not something you\nwant to mimick when adding a similar option to a more modern command\nlike \"git add\", whose attitude toward command line parsing is quite\ndifferent.  Modern command line parsing typically takes \"the last\none wins\" semantics, i.e.\n\n    git add --chmod=-x --chmod=+x foo\n\nwould make foo executable.\n\nIf I were doing this patch, I'd just allocate a file scope global\n\"static char *chmod_arg;\" and OPT_STRING(\"chmod\") to set it, After\nparse_options() returns, I'd do something like:\n\n\tif (chmod_arg) {\n        \tif (strcmp(chmod_arg, \"-x\") && strcmp(chmod_arg, \"+x\"))\n\t\t\tdie(\"--chmod param must be either -x or +x\");\n\t}\n\n> @@ -661,6 +663,10 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n>  \n>  \tif (trust_executable_bit && has_symlinks)\n>  \t\tce->ce_mode = create_ce_mode(st_mode);\n> +\telse if (force_executable)\n> +\t\tce->ce_mode = create_ce_mode(0777);\n> +\telse if (force_notexecutable)\n> +\t\tce->ce_mode = create_ce_mode(0666);\n\nThis is an iffy design decision.\n\nEven when you are in core.filemode=true repository, if you\nexplicitly said\n\n\tgit add --chmod=+x READ.ME\n\nwouldn't you expect that the path would have executable bit in the\nindex, whether it has it as executable in the filesystem?  The above\nif/else cascade, because trust-executable-bit is tested first, will\nignore force_* flags altogether, won't it?  It also is strange that\nthe decision to honor or ignore force_* flags is also tied to\nhas_symlinks, which is a totally orthogonal concept.\n\n> diff --git a/t/t3700-add.sh b/t/t3700-add.sh\n> index f14a665..e551eaf 100755\n> --- a/t/t3700-add.sh\n> +++ b/t/t3700-add.sh\n> @@ -332,4 +332,23 @@ test_expect_success 'git add --dry-run --ignore-missing of non-existing file out\n>  \ttest_i18ncmp expect.err actual.err\n>  '\n>  \n> +test_expect_success 'git add --chmod=+x stages a non-executable file with +x' '\n> +\techo foo >foo1 &&\n> +\tgit add --chmod=+x foo1 &&\n> +\tcase \"$(git ls-files --stage foo1)\" in\n> +\t100755\" \"*foo1) echo pass;;\n> +\t*) echo fail; git ls-files --stage foo1; (exit 1);;\n> +\tesac\n> +'\n> +\n> +test_expect_success 'git add --chmod=-x stages an executable file with -x' '\n> +\techo foo >xfoo1 &&\n> +\tchmod 755 xfoo1 &&\n> +\tgit add --chmod=-x xfoo1 &&\n> +\tcase \"$(git ls-files --stage xfoo1)\" in\n> +\t100644\" \"*xfoo1) echo pass;;\n> +\t*) echo fail; git ls-files --stage xfoo1; (exit 1);;\n> +\tesac\n> +'\n> +\n>  test_done\n"},{"id":"287490","messageId":"alpine.DEB.2.20.1605250923120.4449@virtualbox","threadId":"42445","inReplyTo":"20160525020609.GA20123@zoidberg","subject":"Re: [PATCH] add: add --chmod=+x / --chmod=-x options","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-25T07:46:17Z","receivedAt":"2016-05-25T07:46:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ed,\n\nOn Tue, 24 May 2016, Edward Thomson wrote:\n\n> Users on deficient filesystems that lack an execute bit may still\n> wish to add files to the repository with the appropriate execute\n> bit set (or not).  Although this can be done in two steps\n> (`git add foo && git update-index --chmod=+x foo`), providing the\n> `--chmod=+x` option to the add command allows users to set a file\n> executable in a single command that they're already familiar with.\n> \n> Signed-off-by: Edward Thomson <ethomson@edwardthomson.com>\n\nI like it! Some comments below:\n\n> diff --git a/builtin/add.c b/builtin/add.c\n> index 145f06e..2a9abf7 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -238,6 +238,8 @@ static int ignore_add_errors, intent_to_add, ignore_missing;\n>  static int addremove = ADDREMOVE_DEFAULT;\n>  static int addremove_explicit = -1; /* unspecified */\n>  \n> +static char should_chmod = 0;\n> +\n>  static int ignore_removal_cb(const struct option *opt, const char *arg, int unset)\n>  {\n>  \t/* if we are told to ignore, we are not adding removals */\n> @@ -245,6 +247,15 @@ static int ignore_removal_cb(const struct option *opt, const char *arg, int unse\n>  \treturn 0;\n>  }\n>  \n> +static int chmod_cb(const struct option *opt, 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> +\treturn 0;\n> +}\n> +\n>  static struct option builtin_add_options[] = {\n>  \tOPT__DRY_RUN(&show_only, N_(\"dry run\")),\n>  \tOPT__VERBOSE(&verbose, N_(\"be verbose\")),\n> @@ -263,6 +274,9 @@ static struct option builtin_add_options[] = {\n>  \tOPT_BOOL( 0 , \"refresh\", &refresh_only, N_(\"don't add, only refresh the index\")),\n>  \tOPT_BOOL( 0 , \"ignore-errors\", &ignore_add_errors, N_(\"just skip files which cannot be added because of errors\")),\n>  \tOPT_BOOL( 0 , \"ignore-missing\", &ignore_missing, N_(\"check if - even missing - files are ignored in dry run\")),\n> +\t{ OPTION_CALLBACK, 0, \"chmod\", &should_chmod, N_(\"(+/-)x\"),\n> +\t  N_(\"override the executable bit of the listed files\"),\n> +\t  PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP, chmod_cb},\n\nI wonder, however, whether it would be \"cleaner\" to simply make this an\nOPT_STRING and perform the validation after the option parsing. Something\nlike:\n\n\tconst char *chmod_string = NULL;\n\t...\n\tOPT_STRING( 0 , \"chmod\", &chmod_string, N_(\"( +x | -x )\"),\n\t\tN_(\"override the executable bit of the listed files\")),\n\t...\n\tflags = ...\n\tif (chmod_string) {\n\t\tif (!strcmp(\"+x\", chmod_string))\n\t\t\tflags |= ADD_CACHE_FORCE_EXECUTABLE;\n\t\telse if (!strcmp(\"-x\", chmod_string))\n\t\t\tflags |= ADD_CACHE_FORCE_NOTEXECUTABLE;\n\t\telse\n\t\t\tdie(_(\"invalid --chmod value: %s\"), chmod_string);\n\t}\n\n> diff --git a/cache.h b/cache.h\n> index 6049f86..da03cd9 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -581,6 +581,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> +#define ADD_CACHE_FORCE_EXECUTABLE 32\n> +#define ADD_CACHE_FORCE_NOTEXECUTABLE 64\n\nHmm. This change uses up 2 out of 31 available bits. I wonder whether a\nbetter idea would be to extend struct update_callback_data to include a\n`force_mode` field, pass a parameter of the same name to\nadd_files_to_cache() and then handle that in the update_callback().\nSomething like this:\n\n                case DIFF_STATUS_MODIFIED:\n-               case DIFF_STATUS_TYPE_CHANGED:\n+               case DIFF_STATUS_TYPE_CHANGED: {\n+\t\t\tstruct stat st;\n+\t\t\tif (lstat(path, &st))\n+\t\t\t\tdie_errno(\"unable to stat '%s'\", path);\n+\t\t\tif (S_ISREG(&st.st_mode) && data->force_mode)\n+\t\t\t\tst.st_mode = data->force_mode;\n-                       if (add_file_to_index(&the_index, path, data->flags)) {\n+                       if (add_to_index(&the_index, path, &st, data->flags)) {\n                                if (!(data->flags & ADD_CACHE_IGNORE_ERRORS))\n                                        die(_(\"updating files failed\"));\n                                data->add_errors++;\n                        }\n                        break;\n+\t\t}\n\nThis would not only contain the changes in builtin/add.c, it would also\nforce the mode change when core.filemode = true and core.symlinks = true\n(which your version would handle in a surprising way, I believe).\n\n> 2.6.4 (Apple Git-63)\n\nTime to upgrade? ;-)\n\nCiao,\nDscho\n"},{"id":"287489","messageId":"xmqq1t4q378x.fsf@gitster.mtv.corp.google.com","threadId":"42445","inReplyTo":"20160525020609.GA20123@zoidberg","subject":"Re: [PATCH] add: add --chmod=+x / --chmod=-x options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-25T07:51:42Z","receivedAt":"2016-05-25T07:51:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Edward Thomson <ethomson@edwardthomson.com> writes:\n\n>  \tif (trust_executable_bit && has_symlinks)\n>  \t\tce->ce_mode = create_ce_mode(st_mode);\n> +\telse if (force_executable)\n> +\t\tce->ce_mode = create_ce_mode(0777);\n> +\telse if (force_notexecutable)\n> +\t\tce->ce_mode = create_ce_mode(0666);\n>  \telse {\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\nI would rather do this part more like:\n\n\tif (S_ISREG(st_mode) && (force_executable || force_nonexecuable)) {\n        \tif (force_executable)\n\t\t\tce->ce_mode = create_ce_mode(0777);\n\t\telse\n\t\t\tce->ce_mode = create_ce_mode(0666);\n\t} else if (trust_executable_bit && has_symlinks) {\n        \tce->ce_mode = create_ce_mode(st_mode);\n\t} else {\n        \t... carry the existing mode over ...\n\nwhich would make sure that the new code will not interfere with\nsymbolic links and that forcing will be honored even on filesystems\nwhose executable bit can be trusted (i.e. \"can be trusted\" does not\nhave to mean \"must be trusted\").\n"},{"id":"287493","messageId":"alpine.DEB.2.20.1605251406020.4449@virtualbox","threadId":"42445","inReplyTo":"xmqqh9dm37xk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] add: add --chmod=+x / --chmod=-x options","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-25T12:19:35Z","receivedAt":"2016-05-25T12:19:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 25 May 2016, Junio C Hamano wrote:\n\n>  * I am not familiar with life on filesystems with core.filemode=0;\n>    do files people would want to be able to \"add --chmod=+x\" share\n>    common trait that can be expressed with .gitattributes mechanism?\n\nI think it is safe to say that the biggest example of core.filemode == 0\nis Windows. On that platform, there simply is no executable bit in the\nsense of POSIX permissions. There are Access Control Lists that let you\npermit or deny certain users from executing certain files (and it is files\nonly, directories are a never \"executable\" as in POSIX' scheme).\n\nAnd on Windows, you certainly do not mark a file explicitly as executable\nwhen creating it. The file extension determines whether it is executable\nor not, and that's it.\n\nIn a sense, it is cleaner than POSIX' permission system: it separates\nbetween the permissions and the classification \"is it executable\"?\n\nSide note: shell scripts in Git for Windows are a special type of animal.\nThey *cannot* be made executable by Windows because there is just no\nconcept of free-form scripts that can choose whatever interpreter they\nwant to run in. In Git Bash, we have a special hack that is inherited\ntransitively from Cygwin, where scripts are automatically marked as\nexecutable if their contents start with a shebang. This stops working as\nsoon as you leave the Bash, of course. Due to Git's heavy dependence on\nshell scripting, we had to imitate that same concept in compat/mingw.c. It\nis ugly, it is slow, and we have to live with it.\n\nAs a consequence of this very different concept of an \"executable bit\",\nyou will actually see quite a few Windows-only repositories that *never*\nmark their executables with 0755. In Git for Windows' own repositories,\nthere are a couple of examples where scripts were introduced and only much\nlater did I realize that they were not marked executable (and then I ran\n`git update-index --chmod=+x` on them).\n\nAll that means that the `--chmod` option is really useful only to\ncross-platform projects, and only with careful developers. (And those\ndevelopers could use update-index, as you pointed out.)\n\nI still like Ed's idea and would love to have it: it is murky waters to\nrequire users to call plumbing only because our porcelain isn't up to par.\n\nCiao,\nDscho\n"},{"id":"287497","messageId":"xmqqshx6162k.fsf@gitster.mtv.corp.google.com","threadId":"42445","inReplyTo":"xmqqh9dm37xk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] add: add --chmod=+x / --chmod=-x options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-25T16:00:03Z","receivedAt":"2016-05-25T16:00:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> @@ -661,6 +663,10 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n>>  \n>>  \tif (trust_executable_bit && has_symlinks)\n>>  \t\tce->ce_mode = create_ce_mode(st_mode);\n>> +\telse if (force_executable)\n>> +\t\tce->ce_mode = create_ce_mode(0777);\n>> +\telse if (force_notexecutable)\n>> +\t\tce->ce_mode = create_ce_mode(0666);\n>\n> This is an iffy design decision.\n>\n> Even when you are in core.filemode=true repository, if you\n> explicitly said\n>\n> \tgit add --chmod=+x READ.ME\n>\n> wouldn't you expect that the path would have executable bit in the\n> index, whether it has it as executable in the filesystem?  The above\n> if/else cascade, because trust-executable-bit is tested first, will\n> ignore force_* flags altogether, won't it?  It also is strange that\n> the decision to honor or ignore force_* flags is also tied to\n> has_symlinks, which is a totally orthogonal concept.\n\nHere is an additional patch to your tests.  It repeats one of the\ntests you added, but runs in a repository with core.filemode and\ncore.symlinks both enabled.  The test fails to force executable bit\non platforms where it runs.\n\nIt passes with your patch if you drop core.symlinks, which is a good\ndemonstration why letting has_symlinks decide if force* is to be\nhonored is iffy.\n\n t/t3700-add.sh | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex e551eaf..2afcb74 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -351,4 +351,15 @@ test_expect_success 'git add --chmod=-x stages an executable file with -x' '\n \tesac\n '\n \n+test_expect_success POSIXPERM,SYMLINKS 'git add --chmod=+x' '\n+\tgit config core.filemode 1 &&\n+\tgit config core.symlinks 1 &&\n+\techo foo >foo2 &&\n+\tgit add --chmod=+x foo2 &&\n+\tcase \"$(git ls-files --stage foo2)\" in\n+\t100755\" \"*foo2) echo pass;;\n+\t*) echo fail; git ls-files --stage foo2; (exit 1);;\n+\tesac\n+'\n+\n test_done\n"},{"id":"287498","messageId":"xmqqoa7u15lq.fsf@gitster.mtv.corp.google.com","threadId":"42445","inReplyTo":"alpine.DEB.2.20.1605251406020.4449@virtualbox","subject":"Re: [PATCH] add: add --chmod=+x / --chmod=-x options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-25T16:10:09Z","receivedAt":"2016-05-25T16:10:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> On Wed, 25 May 2016, Junio C Hamano wrote:\n>\n>>  * I am not familiar with life on filesystems with core.filemode=0;\n>>    do files people would want to be able to \"add --chmod=+x\" share\n>>    common trait that can be expressed with .gitattributes mechanism?\n>\n> I think it is safe to say that the biggest example of core.filemode == 0\n> is Windows. On that platform, there simply is no executable bit in the\n> sense of POSIX permissions. ...\n> ... I still like Ed's idea and would love to have it: it is murky waters to\n> require users to call plumbing only because our porcelain isn't up to par.\n\nI thought that I made it absolutely clear that I like the addition,\ntoo.  If it wasn't clear enough, I can say it again, but I do not\nthink you need it ;-).\n\nThe \"attribute\" thing was an idea that was hoping to make the system\nas a whole even more helpful; if pattern matching with paths is\nsufficient for projects to hint desired permission bits per paths,\nthen those working on such a cross-platform project on Windows do\nnot have to even worry about \"git cmd --chmod=+x\", whether cmd is\nadd or update-index.  If they can just do \"git add\" and need to use\nthe new \"--chmod=+x\" option only when the patterns are not set up\ncorrectly, wouldn't that be even more helpful?  In other words, it\nwasn't \"with this we can _eliminate_ need for 'add --chmod'\".\n\nThe only thing I was unsure about that scheme was if \"pattern\nmatching with paths\" is sufficiently powerful (if not, such an\naddition would not work as a mechanism to reduce the need for the\nusers to run \"git add --chmod=+x\").  And that was my inquiry.\n\nUnfortunately, your answer does not help answer that question;\nit was a question to Edward, so that's OK anyway.\n"},{"id":"287501","messageId":"alpine.DEB.2.20.1605251844580.4449@virtualbox","threadId":"42445","inReplyTo":"xmqqoa7u15lq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] add: add --chmod=+x / --chmod=-x options","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-25T16:49:29Z","receivedAt":"2016-05-25T16:49:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 25 May 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> > On Wed, 25 May 2016, Junio C Hamano wrote:\n> >\n> >>  * I am not familiar with life on filesystems with core.filemode=0;\n> >>    do files people would want to be able to \"add --chmod=+x\" share\n> >>    common trait that can be expressed with .gitattributes mechanism?\n> >\n> > I think it is safe to say that the biggest example of core.filemode == 0\n> > is Windows. On that platform, there simply is no executable bit in the\n> > sense of POSIX permissions. ...\n> > ... I still like Ed's idea and would love to have it: it is murky waters to\n> > require users to call plumbing only because our porcelain isn't up to par.\n> \n> I thought that I made it absolutely clear that I like the addition,\n> too.  If it wasn't clear enough, I can say it again, but I do not\n> think you need it ;-).\n\nOh, I understood that you liked it, sorry if my mail looked accusatory.\n\n> The \"attribute\" thing was an idea that was hoping to make the system\n> as a whole even more helpful;\n\nI understood that, too. My first impression was that it would not be.\nHowever, as Git for Windows can set default attributes in\n/mingw64/etc/gitconfig, I guess it would actually be helpful. We could\nautomatically mark all *.exe, *.com, *.bat, *.cmd files as executable. It\nwould then still be the users' responsibility to add their own attributes\nfor, say, *.js, *.rb, *.py, *.sh, and whatever else.\n\nCiao,\nDscho\n"},{"id":"287651","messageId":"20160527044112.GA31742@zoidberg","threadId":"42445","inReplyTo":"xmqqh9dm37xk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] add: add --chmod=+x / --chmod=-x options","fromName":"Edward Thomson","fromEmail":"ethomson@edwardthomson.com","sentAt":"2016-05-27T04:41:12Z","receivedAt":"2016-05-27T04:41:12Z","isPatch":true,"sender":{"key":"ethomson@edwardthomson.com","avatar":"https://avatars.githubusercontent.com/u/1130014?v=4"},"body":"On Wed, May 25, 2016 at 12:36:55AM -0700, Junio C Hamano wrote:\n> \n> At the design level, I have a few comments.\n\nThanks, I will submit a new patch that incorporates your (and dscho's)\ncomments.\n\n>  * This is about a repository with core.filemode=0; I wonder if\n>    something for a repository with core.symlinks=0 would also help?\n>    That is, would it be a big help to users if they can prepare a\n>    text file that holds symbolic link contents and add it as if it\n>    were a symlink with \"git add\", instead of having to run two\n>    commands, \"hash-objects && update-index --cacheinfo\"?\n\nI think that this is much less common and - speaking only from personal\nexperience - nobody has ever asked me how to stage a symlink on a\nWindows machine.  I think that this is due to the fact that symlinks on\nWindows are basically impossible to use, so people doing cross-platform\ndevelopment wouldn't even try.\n\nOn the other hand, it's quite common for cross-platform teams to use\nsome scripting language since those do work across platforms, and\nWindows users would want to add new scripts as executable for the\nbenefit of their brethren on platforms with an executable bit.\n\n>  * I am not familiar with life on filesystems with core.filemode=0;\n>    do files people would want to be able to \"add --chmod=+x\" share\n>    common trait that can be expressed with .gitattributes mechanism?\n\nPerhaps...  It would not be things like `*.bat` or `*.exe` - Windows\ngets those as executable \"for free\" and would not care about adding the\nexecute bit on those files (since they're not executable anywhere else).\nIt would be items like `*.sh` or `*.rb` that should be executable on\nPOSIX platforms.\n\nHowever I do not think that this is a common enough action that it needs\nto be made automatic such that when I `git add foo.rb` it is\nautomatically made executable.  I think that the reduced complexity of\nhaving a single mechanism to control executability (that being the\nexecute mode in the index or a tree) is preferable to a gitattributes\nbased mechanism, at least until somebody else makes a cogent argument\nthat the gitattributes approach would be helpful for them.  :)\n\nThanks again for the comments, an updated patch is forthcoming.\n\n-ed\n"},{"id":"287653","messageId":"20160527051246.GA27092@glandium.org","threadId":"42445","inReplyTo":"20160527044112.GA31742@zoidberg","subject":"Re: [PATCH] add: add --chmod=+x / --chmod=-x options","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-05-27T05:12:46Z","receivedAt":"2016-05-27T05:12:46Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Thu, May 26, 2016 at 11:41:12PM -0500, Edward Thomson wrote:\n> On Wed, May 25, 2016 at 12:36:55AM -0700, Junio C Hamano wrote:\n> > \n> > At the design level, I have a few comments.\n> \n> Thanks, I will submit a new patch that incorporates your (and dscho's)\n> comments.\n> \n> >  * This is about a repository with core.filemode=0; I wonder if\n> >    something for a repository with core.symlinks=0 would also help?\n> >    That is, would it be a big help to users if they can prepare a\n> >    text file that holds symbolic link contents and add it as if it\n> >    were a symlink with \"git add\", instead of having to run two\n> >    commands, \"hash-objects && update-index --cacheinfo\"?\n> \n> I think that this is much less common and - speaking only from personal\n> experience - nobody has ever asked me how to stage a symlink on a\n> Windows machine.  I think that this is due to the fact that symlinks on\n> Windows are basically impossible to use, so people doing cross-platform\n> development wouldn't even try.\n> \n> On the other hand, it's quite common for cross-platform teams to use\n> some scripting language since those do work across platforms, and\n> Windows users would want to add new scripts as executable for the\n> benefit of their brethren on platforms with an executable bit.\n> \n> >  * I am not familiar with life on filesystems with core.filemode=0;\n> >    do files people would want to be able to \"add --chmod=+x\" share\n> >    common trait that can be expressed with .gitattributes mechanism?\n> \n> Perhaps...  It would not be things like `*.bat` or `*.exe` - Windows\n> gets those as executable \"for free\" and would not care about adding the\n> execute bit on those files (since they're not executable anywhere else).\n> It would be items like `*.sh` or `*.rb` that should be executable on\n> POSIX platforms.\n> \n> However I do not think that this is a common enough action that it needs\n> to be made automatic such that when I `git add foo.rb` it is\n> automatically made executable.\n\nMoreover, *.sh, *.rb, etc. are not necessarily meant to be executables.\nThe files might be modules, included from executables or other modules.\nThere's an example of this right in the git tree: t/test-lib.sh. It's\neven more typical for *.rb files (or *.py, etc.)\n\nHowever, the common pattern that /might/ be interesting for automatic\nexecutable bits is files starting with \"#!\".\n\nMike\n"},{"id":"287659","messageId":"xmqqoa7sroru.fsf@gitster.mtv.corp.google.com","threadId":"42445","inReplyTo":"20160527044112.GA31742@zoidberg","subject":"Re: [PATCH] add: add --chmod=+x / --chmod=-x options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-27T06:36:05Z","receivedAt":"2016-05-27T06:36:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Edward Thomson <ethomson@edwardthomson.com> writes:\n\n> However I do not think that this is a common enough action that it needs\n> to be made automatic such that when I `git add foo.rb` it is\n> automatically made executable.  I think that the reduced complexity of\n> having a single mechanism to control executability (that being the\n> execute mode in the index or a tree) is preferable to a gitattributes\n> based mechanism, at least until somebody else makes a cogent argument\n> that the gitattributes approach would be helpful for them.  :)\n\nIt wasn't a \"having to specify it every time sucks; you must do this\nway instead\" at all.  I was just gauging if it would be a viable idea\nfor a follow-up series to complement your patch.\n\nThanks.\n"},{"id":"287703","messageId":"xmqqmvnbqrov.fsf@gitster.mtv.corp.google.com","threadId":"42445","inReplyTo":"xmqqoa7sroru.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] add: add --chmod=+x / --chmod=-x options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-27T18:30:40Z","receivedAt":"2016-05-27T18:30:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Edward Thomson <ethomson@edwardthomson.com> writes:\n>\n>> However I do not think that this is a common enough action that it needs\n>> to be made automatic such that when I `git add foo.rb` it is\n>> automatically made executable.  I think that the reduced complexity of\n>> having a single mechanism to control executability (that being the\n>> execute mode in the index or a tree) is preferable to a gitattributes\n>> based mechanism, at least until somebody else makes a cogent argument\n>> that the gitattributes approach would be helpful for them.  :)\n>\n> It wasn't a \"having to specify it every time sucks; you must do this\n> way instead\" at all.  I was just gauging if it would be a viable idea\n> for a follow-up series to complement your patch.\n>\n> Thanks.\n\nOh, having said all of that, the comments on the implementation\nstill stand.\n"},{"id":"287704","messageId":"xmqqinxzqrhn.fsf@gitster.mtv.corp.google.com","threadId":"42445","inReplyTo":"alpine.DEB.2.20.1605250923120.4449@virtualbox","subject":"Re: [PATCH] add: add --chmod=+x / --chmod=-x options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-27T18:35:00Z","receivedAt":"2016-05-27T18:35:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> I wonder, however, whether it would be \"cleaner\" to simply make this an\n> OPT_STRING and perform the validation after the option parsing.\n\nYes, I think I touched on this in my comments in a bit more detail.\n\n> Hmm. This change uses up 2 out of 31 available bits. I wonder whether a\n> better idea would be to extend struct update_callback_data to include a\n> `force_mode` field, pass a parameter of the same name to\n> add_files_to_cache() and then handle that in the update_callback().\n\nMaybe.  I am not sure if it is a good idea to do lstat(2) on the\ncalling side, though.  Assuming it is, your \"something like this\"\nneeds to be duplicated for the codepath that adds a new file, which\nis separate from the one we see below (i.e. add_files()).\n\n> Something like this:\n>\n>                 case DIFF_STATUS_MODIFIED:\n> -               case DIFF_STATUS_TYPE_CHANGED:\n> +               case DIFF_STATUS_TYPE_CHANGED: {\n> +\t\t\tstruct stat st;\n> +\t\t\tif (lstat(path, &st))\n> +\t\t\t\tdie_errno(\"unable to stat '%s'\", path);\n> +\t\t\tif (S_ISREG(&st.st_mode) && data->force_mode)\n> +\t\t\t\tst.st_mode = data->force_mode;\n> -                       if (add_file_to_index(&the_index, path, data->flags)) {\n> +                       if (add_to_index(&the_index, path, &st, data->flags)) {\n>                                 if (!(data->flags & ADD_CACHE_IGNORE_ERRORS))\n>                                         die(_(\"updating files failed\"));\n>                                 data->add_errors++;\n>                         }\n>                         break;\n> +\t\t}\n"},{"id":"287939","messageId":"20160531220650.GA46739@zoidberg","threadId":"42445","inReplyTo":"xmqqmvnbqrov.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] add: add --chmod=+x / --chmod=-x options","fromName":"Edward Thomson","fromEmail":"ethomson@edwardthomson.com","sentAt":"2016-05-31T22:06:50Z","receivedAt":"2016-05-31T22:06:50Z","isPatch":true,"sender":{"key":"ethomson@edwardthomson.com","avatar":"https://avatars.githubusercontent.com/u/1130014?v=4"},"body":"On Fri, May 27, 2016 at 11:30:40AM -0700, Junio C Hamano wrote:\n> \n> Oh, having said all of that, the comments on the implementation\n> still stand.\n\nCertainly; sorry for the delay.  I've squashed in your tests and applied\nyour recommendations.  Resending the patch momentarily.\n\nThanks-\n\n-ed\n"}]}