# [PATCH] add: add --chmod=+x / --chmod=-x options 14 messages from 2016-05-25 to 2016-05-31. Participants: Edward Thomson, Junio C Hamano, Johannes Schindelin, Mike Hommey. Thread: https://gitlist.dev/t/42445 ## Edward Thomson, 2016-05-25 02:06 Subject: [PATCH] add: add --chmod=+x / --chmod=-x options Message-ID: <20160525020609.GA20123@zoidberg> URL: https://gitlist.dev/e/20160525020609.GA20123%40zoidberg ``` Users on deficient filesystems that lack an execute bit may still wish to add files to the repository with the appropriate execute bit set (or not). Although this can be done in two steps (`git add foo && git update-index --chmod=+x foo`), providing the `--chmod=+x` option to the add command allows users to set a file executable in a single command that they're already familiar with. Signed-off-by: Edward Thomson --- builtin/add.c | 18 +++++++++++++++++- cache.h | 2 ++ read-cache.c | 6 ++++++ t/t3700-add.sh | 19 +++++++++++++++++++ 4 files changed, 44 insertions(+), 1 deletion(-) diff --git a/builtin/add.c b/builtin/add.c index 145f06e..2a9abf7 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -238,6 +238,8 @@ static int ignore_add_errors, intent_to_add, ignore_missing; static int addremove = ADDREMOVE_DEFAULT; static int addremove_explicit = -1; /* unspecified */ +static char should_chmod = 0; + static int ignore_removal_cb(const struct option *opt, const char *arg, int unset) { /* if we are told to ignore, we are not adding removals */ @@ -245,6 +247,15 @@ static int ignore_removal_cb(const struct option *opt, const char *arg, int unse return 0; } +static int chmod_cb(const struct option *opt, const char *arg, int unset) +{ + char *flip = opt->value; + if ((arg[0] != '-' && arg[0] != '+') || arg[1] != 'x' || arg[2]) + return error("option 'chmod' expects \"+x\" or \"-x\""); + *flip = arg[0]; + return 0; +} + static struct option builtin_add_options[] = { OPT__DRY_RUN(&show_only, N_("dry run")), OPT__VERBOSE(&verbose, N_("be verbose")), @@ -263,6 +274,9 @@ static struct option builtin_add_options[] = { OPT_BOOL( 0 , "refresh", &refresh_only, N_("don't add, only refresh the index")), OPT_BOOL( 0 , "ignore-errors", &ignore_add_errors, N_("just skip files which cannot be added because of errors")), OPT_BOOL( 0 , "ignore-missing", &ignore_missing, N_("check if - even missing - files are ignored in dry run")), + { OPTION_CALLBACK, 0, "chmod", &should_chmod, N_("(+/-)x"), + N_("override the executable bit of the listed files"), + PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP, chmod_cb}, OPT_END(), }; @@ -346,7 +360,9 @@ int cmd_add(int argc, const char **argv, const char *prefix) (intent_to_add ? ADD_CACHE_INTENT : 0) | (ignore_add_errors ? ADD_CACHE_IGNORE_ERRORS : 0) | (!(addremove || take_worktree_changes) - ? ADD_CACHE_IGNORE_REMOVAL : 0)); + ? ADD_CACHE_IGNORE_REMOVAL : 0)) | + (should_chmod == '+' ? ADD_CACHE_FORCE_EXECUTABLE : 0) | + (should_chmod == '-' ? ADD_CACHE_FORCE_NOTEXECUTABLE : 0); if (require_pathspec && argc == 0) { fprintf(stderr, _("Nothing specified, nothing added.\n")); diff --git a/cache.h b/cache.h index 6049f86..da03cd9 100644 --- a/cache.h +++ b/cache.h @@ -581,6 +581,8 @@ extern int remove_file_from_index(struct index_state *, const char *path); #define ADD_CACHE_IGNORE_ERRORS 4 #define ADD_CACHE_IGNORE_REMOVAL 8 #define ADD_CACHE_INTENT 16 +#define ADD_CACHE_FORCE_EXECUTABLE 32 +#define ADD_CACHE_FORCE_NOTEXECUTABLE 64 extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags); extern int add_file_to_index(struct index_state *, const char *path, int flags); extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, unsigned int refresh_options); diff --git a/read-cache.c b/read-cache.c index d9fb78b..81bf186 100644 --- a/read-cache.c +++ b/read-cache.c @@ -641,6 +641,8 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st, int intent_only = flags & ADD_CACHE_INTENT; int add_option = (ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE| (intent_only ? ADD_CACHE_NEW_ONLY : 0)); + int force_executable = flags & ADD_CACHE_FORCE_EXECUTABLE; + int force_notexecutable = flags & ADD_CACHE_FORCE_NOTEXECUTABLE; if (!S_ISREG(st_mode) && !S_ISLNK(st_mode) && !S_ISDIR(st_mode)) return error("%s: can only add regular files, symbolic links or git-directories", path); @@ -661,6 +663,10 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st, if (trust_executable_bit && has_symlinks) ce->ce_mode = create_ce_mode(st_mode); + else if (force_executable) + ce->ce_mode = create_ce_mode(0777); + else if (force_notexecutable) + ce->ce_mode = create_ce_mode(0666); else { /* If there is an existing entry, pick the mode bits and type * from it, otherwise assume unexecutable regular file. diff --git a/t/t3700-add.sh b/t/t3700-add.sh index f14a665..e551eaf 100755 --- a/t/t3700-add.sh +++ b/t/t3700-add.sh @@ -332,4 +332,23 @@ test_expect_success 'git add --dry-run --ignore-missing of non-existing file out test_i18ncmp expect.err actual.err ' +test_expect_success 'git add --chmod=+x stages a non-executable file with +x' ' + echo foo >foo1 && + git add --chmod=+x foo1 && + case "$(git ls-files --stage foo1)" in + 100755" "*foo1) echo pass;; + *) echo fail; git ls-files --stage foo1; (exit 1);; + esac +' + +test_expect_success 'git add --chmod=-x stages an executable file with -x' ' + echo foo >xfoo1 && + chmod 755 xfoo1 && + git add --chmod=-x xfoo1 && + case "$(git ls-files --stage xfoo1)" in + 100644" "*xfoo1) echo pass;; + *) echo fail; git ls-files --stage xfoo1; (exit 1);; + esac +' + test_done -- 2.6.4 (Apple Git-63) ``` ## Junio C Hamano, 2016-05-25 07:36 Subject: Re: [PATCH] add: add --chmod=+x / --chmod=-x options Message-ID: URL: https://gitlist.dev/e/xmqqh9dm37xk.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20160525020609.GA20123@zoidberg> ``` Edward Thomson writes: > Users on deficient filesystems that lack an execute bit may still > wish to add files to the repository with the appropriate execute > bit set (or not). Although this can be done in two steps > (`git add foo && git update-index --chmod=+x foo`), providing the > `--chmod=+x` option to the add command allows users to set a file > executable in a single command that they're already familiar with. > > Signed-off-by: Edward Thomson > --- I think we should tone down the first sentence of the proposed commit log message. With "s/deficient //" the paragraph still reads perfectly well. Even when an underlying filesystem is capable of expressing executable bit, we can set core.filemode to false to emulate the behaviour on DOS, so perhaps The executable bit will not be set for paths in a repository with core.filemode set to false, but the users may still wish to add files to ... or something like it? Giving an easy to use single-command short-hand for a common thing that takes two commands is a worthy goal. Another way to do the above is git update-index --add --chmod=+x foo and from that point of view, we do not need this patch, but that is still a mouthful ;-) I think it is a good idea to teach "git add", an end-user facing command, to be more helpful. At the design level, I have a few comments. * Unlike the command "chmod", which is _only_ about changing modes, "add --chmod" updates both the contents and modes in the index, which may invite "I only want to change modes--how?" Note. We had an ancient regression at 227bdb18 (make update-index --chmod work with multiple files and --stdin, 2006-04-23), where "update-index --chmod=+x foo" stopped being "only flip the executable bit without hashing the contents" and that was done purely by mistake. There is no longer a good answer to that question, which makes the above worry less of an issue. * This is about a repository with core.filemode=0; I wonder if something for a repository with core.symlinks=0 would also help? That is, would it be a big help to users if they can prepare a text file that holds symbolic link contents and add it as if it were a symlink with "git add", instead of having to run two commands, "hash-objects && update-index --cacheinfo"? * I am not familiar with life on filesystems with core.filemode=0; do files people would want to be able to "add --chmod=+x" share common trait that can be expressed with .gitattributes mechanism? What I am wondering is if a scheme like the following would work well, in addition to your patch: 1. Have these in .gitattributes: * -executable *.bat executable text *.exe executable binary *.com executable binary A path with Unset "executable" attribute is explicitly marked as "not executable"; a path with Set "executable" attribute is marked as "executable", i.e. "needing chmod=+x". 2. Teach "git add" to take the above hint _only_ in a repository where core.filemode is false and _only_ when adding a new path to the index. If something like this works well enough, users do not have to type --chmod=+x too often when doing "git add"; your patch becomes an escape hatch that is only needed when the attributes system gets it wrong. Now some comments on the actual code. > +static int chmod_cb(const struct option *opt, const char *arg, int unset) > +{ > + char *flip = opt->value; > + if ((arg[0] != '-' && arg[0] != '+') || arg[1] != 'x' || arg[2]) > + return error("option 'chmod' expects \"+x\" or \"-x\""); > + *flip = arg[0]; > + return 0; > +} I know you mimicked the command line parser of update-index, but you didn't have to, and you shouldn't have. The command line semantics of update-index is largely "we read one option and prepare to make its effect immediately available", which predates parse-options where its attitude for command line parsing is "we first parse all options and figure out what to do, and then we work on arguments according to these options". Because of these vastly different attitudes, the way builtin/update-index.c uses parse-options API is atypical. The only reason it uses callback is because it wants to allow you to say this: git update-index --chmod=+x foo bar --chmod=-x baz and register foo and bar as executable, while baz as non-executable. The way update-index uses parse-options API is not something you want to mimick when adding a similar option to a more modern command like "git add", whose attitude toward command line parsing is quite different. Modern command line parsing typically takes "the last one wins" semantics, i.e. git add --chmod=-x --chmod=+x foo would make foo executable. If I were doing this patch, I'd just allocate a file scope global "static char *chmod_arg;" and OPT_STRING("chmod") to set it, After parse_options() returns, I'd do something like: if (chmod_arg) { if (strcmp(chmod_arg, "-x") && strcmp(chmod_arg, "+x")) die("--chmod param must be either -x or +x"); } > @@ -661,6 +663,10 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st, > > if (trust_executable_bit && has_symlinks) > ce->ce_mode = create_ce_mode(st_mode); > + else if (force_executable) > + ce->ce_mode = create_ce_mode(0777); > + else if (force_notexecutable) > + ce->ce_mode = create_ce_mode(0666); This is an iffy design decision. Even when you are in core.filemode=true repository, if you explicitly said git add --chmod=+x READ.ME wouldn't you expect that the path would have executable bit in the index, whether it has it as executable in the filesystem? The above if/else cascade, because trust-executable-bit is tested first, will ignore force_* flags altogether, won't it? It also is strange that the decision to honor or ignore force_* flags is also tied to has_symlinks, which is a totally orthogonal concept. > diff --git a/t/t3700-add.sh b/t/t3700-add.sh > index f14a665..e551eaf 100755 > --- a/t/t3700-add.sh > +++ b/t/t3700-add.sh > @@ -332,4 +332,23 @@ test_expect_success 'git add --dry-run --ignore-missing of non-existing file out > test_i18ncmp expect.err actual.err > ' > > +test_expect_success 'git add --chmod=+x stages a non-executable file with +x' ' > + echo foo >foo1 && > + git add --chmod=+x foo1 && > + case "$(git ls-files --stage foo1)" in > + 100755" "*foo1) echo pass;; > + *) echo fail; git ls-files --stage foo1; (exit 1);; > + esac > +' > + > +test_expect_success 'git add --chmod=-x stages an executable file with -x' ' > + echo foo >xfoo1 && > + chmod 755 xfoo1 && > + git add --chmod=-x xfoo1 && > + case "$(git ls-files --stage xfoo1)" in > + 100644" "*xfoo1) echo pass;; > + *) echo fail; git ls-files --stage xfoo1; (exit 1);; > + esac > +' > + > test_done ``` ## Johannes Schindelin, 2016-05-25 07:46 Subject: Re: [PATCH] add: add --chmod=+x / --chmod=-x options Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1605250923120.4449%40virtualbox In-Reply-To: <20160525020609.GA20123@zoidberg> ``` Hi Ed, On Tue, 24 May 2016, Edward Thomson wrote: > Users on deficient filesystems that lack an execute bit may still > wish to add files to the repository with the appropriate execute > bit set (or not). Although this can be done in two steps > (`git add foo && git update-index --chmod=+x foo`), providing the > `--chmod=+x` option to the add command allows users to set a file > executable in a single command that they're already familiar with. > > Signed-off-by: Edward Thomson I like it! Some comments below: > diff --git a/builtin/add.c b/builtin/add.c > index 145f06e..2a9abf7 100644 > --- a/builtin/add.c > +++ b/builtin/add.c > @@ -238,6 +238,8 @@ static int ignore_add_errors, intent_to_add, ignore_missing; > static int addremove = ADDREMOVE_DEFAULT; > static int addremove_explicit = -1; /* unspecified */ > > +static char should_chmod = 0; > + > static int ignore_removal_cb(const struct option *opt, const char *arg, int unset) > { > /* if we are told to ignore, we are not adding removals */ > @@ -245,6 +247,15 @@ static int ignore_removal_cb(const struct option *opt, const char *arg, int unse > return 0; > } > > +static int chmod_cb(const struct option *opt, const char *arg, int unset) > +{ > + char *flip = opt->value; > + if ((arg[0] != '-' && arg[0] != '+') || arg[1] != 'x' || arg[2]) > + return error("option 'chmod' expects \"+x\" or \"-x\""); > + *flip = arg[0]; > + return 0; > +} > + > static struct option builtin_add_options[] = { > OPT__DRY_RUN(&show_only, N_("dry run")), > OPT__VERBOSE(&verbose, N_("be verbose")), > @@ -263,6 +274,9 @@ static struct option builtin_add_options[] = { > OPT_BOOL( 0 , "refresh", &refresh_only, N_("don't add, only refresh the index")), > OPT_BOOL( 0 , "ignore-errors", &ignore_add_errors, N_("just skip files which cannot be added because of errors")), > OPT_BOOL( 0 , "ignore-missing", &ignore_missing, N_("check if - even missing - files are ignored in dry run")), > + { OPTION_CALLBACK, 0, "chmod", &should_chmod, N_("(+/-)x"), > + N_("override the executable bit of the listed files"), > + PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP, chmod_cb}, I wonder, however, whether it would be "cleaner" to simply make this an OPT_STRING and perform the validation after the option parsing. Something like: const char *chmod_string = NULL; ... OPT_STRING( 0 , "chmod", &chmod_string, N_("( +x | -x )"), N_("override the executable bit of the listed files")), ... flags = ... if (chmod_string) { if (!strcmp("+x", chmod_string)) flags |= ADD_CACHE_FORCE_EXECUTABLE; else if (!strcmp("-x", chmod_string)) flags |= ADD_CACHE_FORCE_NOTEXECUTABLE; else die(_("invalid --chmod value: %s"), chmod_string); } > diff --git a/cache.h b/cache.h > index 6049f86..da03cd9 100644 > --- a/cache.h > +++ b/cache.h > @@ -581,6 +581,8 @@ extern int remove_file_from_index(struct index_state *, const char *path); > #define ADD_CACHE_IGNORE_ERRORS 4 > #define ADD_CACHE_IGNORE_REMOVAL 8 > #define ADD_CACHE_INTENT 16 > +#define ADD_CACHE_FORCE_EXECUTABLE 32 > +#define ADD_CACHE_FORCE_NOTEXECUTABLE 64 Hmm. This change uses up 2 out of 31 available bits. I wonder whether a better idea would be to extend struct update_callback_data to include a `force_mode` field, pass a parameter of the same name to add_files_to_cache() and then handle that in the update_callback(). Something like this: case DIFF_STATUS_MODIFIED: - case DIFF_STATUS_TYPE_CHANGED: + case DIFF_STATUS_TYPE_CHANGED: { + struct stat st; + if (lstat(path, &st)) + die_errno("unable to stat '%s'", path); + if (S_ISREG(&st.st_mode) && data->force_mode) + st.st_mode = data->force_mode; - if (add_file_to_index(&the_index, path, data->flags)) { + if (add_to_index(&the_index, path, &st, data->flags)) { if (!(data->flags & ADD_CACHE_IGNORE_ERRORS)) die(_("updating files failed")); data->add_errors++; } break; + } This would not only contain the changes in builtin/add.c, it would also force the mode change when core.filemode = true and core.symlinks = true (which your version would handle in a surprising way, I believe). > 2.6.4 (Apple Git-63) Time to upgrade? ;-) Ciao, Dscho ``` ## Junio C Hamano, 2016-05-25 07:51 Subject: Re: [PATCH] add: add --chmod=+x / --chmod=-x options Message-ID: URL: https://gitlist.dev/e/xmqq1t4q378x.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20160525020609.GA20123@zoidberg> ``` Edward Thomson writes: > if (trust_executable_bit && has_symlinks) > ce->ce_mode = create_ce_mode(st_mode); > + else if (force_executable) > + ce->ce_mode = create_ce_mode(0777); > + else if (force_notexecutable) > + ce->ce_mode = create_ce_mode(0666); > else { > /* If there is an existing entry, pick the mode bits and type > * from it, otherwise assume unexecutable regular file. I would rather do this part more like: if (S_ISREG(st_mode) && (force_executable || force_nonexecuable)) { if (force_executable) ce->ce_mode = create_ce_mode(0777); else ce->ce_mode = create_ce_mode(0666); } else if (trust_executable_bit && has_symlinks) { ce->ce_mode = create_ce_mode(st_mode); } else { ... carry the existing mode over ... which would make sure that the new code will not interfere with symbolic links and that forcing will be honored even on filesystems whose executable bit can be trusted (i.e. "can be trusted" does not have to mean "must be trusted"). ``` ## Johannes Schindelin, 2016-05-25 12:19 Subject: Re: [PATCH] add: add --chmod=+x / --chmod=-x options Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1605251406020.4449%40virtualbox In-Reply-To: ``` Hi Junio, On Wed, 25 May 2016, Junio C Hamano wrote: > * I am not familiar with life on filesystems with core.filemode=0; > do files people would want to be able to "add --chmod=+x" share > common trait that can be expressed with .gitattributes mechanism? I think it is safe to say that the biggest example of core.filemode == 0 is Windows. On that platform, there simply is no executable bit in the sense of POSIX permissions. There are Access Control Lists that let you permit or deny certain users from executing certain files (and it is files only, directories are a never "executable" as in POSIX' scheme). And on Windows, you certainly do not mark a file explicitly as executable when creating it. The file extension determines whether it is executable or not, and that's it. In a sense, it is cleaner than POSIX' permission system: it separates between the permissions and the classification "is it executable"? Side note: shell scripts in Git for Windows are a special type of animal. They *cannot* be made executable by Windows because there is just no concept of free-form scripts that can choose whatever interpreter they want to run in. In Git Bash, we have a special hack that is inherited transitively from Cygwin, where scripts are automatically marked as executable if their contents start with a shebang. This stops working as soon as you leave the Bash, of course. Due to Git's heavy dependence on shell scripting, we had to imitate that same concept in compat/mingw.c. It is ugly, it is slow, and we have to live with it. As a consequence of this very different concept of an "executable bit", you will actually see quite a few Windows-only repositories that *never* mark their executables with 0755. In Git for Windows' own repositories, there are a couple of examples where scripts were introduced and only much later did I realize that they were not marked executable (and then I ran `git update-index --chmod=+x` on them). All that means that the `--chmod` option is really useful only to cross-platform projects, and only with careful developers. (And those developers could use update-index, as you pointed out.) I still like Ed's idea and would love to have it: it is murky waters to require users to call plumbing only because our porcelain isn't up to par. Ciao, Dscho ``` ## Junio C Hamano, 2016-05-25 16:00 Subject: Re: [PATCH] add: add --chmod=+x / --chmod=-x options Message-ID: URL: https://gitlist.dev/e/xmqqshx6162k.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Junio C Hamano writes: >> @@ -661,6 +663,10 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st, >> >> if (trust_executable_bit && has_symlinks) >> ce->ce_mode = create_ce_mode(st_mode); >> + else if (force_executable) >> + ce->ce_mode = create_ce_mode(0777); >> + else if (force_notexecutable) >> + ce->ce_mode = create_ce_mode(0666); > > This is an iffy design decision. > > Even when you are in core.filemode=true repository, if you > explicitly said > > git add --chmod=+x READ.ME > > wouldn't you expect that the path would have executable bit in the > index, whether it has it as executable in the filesystem? The above > if/else cascade, because trust-executable-bit is tested first, will > ignore force_* flags altogether, won't it? It also is strange that > the decision to honor or ignore force_* flags is also tied to > has_symlinks, which is a totally orthogonal concept. Here is an additional patch to your tests. It repeats one of the tests you added, but runs in a repository with core.filemode and core.symlinks both enabled. The test fails to force executable bit on platforms where it runs. It passes with your patch if you drop core.symlinks, which is a good demonstration why letting has_symlinks decide if force* is to be honored is iffy. t/t3700-add.sh | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/t/t3700-add.sh b/t/t3700-add.sh index e551eaf..2afcb74 100755 --- a/t/t3700-add.sh +++ b/t/t3700-add.sh @@ -351,4 +351,15 @@ test_expect_success 'git add --chmod=-x stages an executable file with -x' ' esac ' +test_expect_success POSIXPERM,SYMLINKS 'git add --chmod=+x' ' + git config core.filemode 1 && + git config core.symlinks 1 && + echo foo >foo2 && + git add --chmod=+x foo2 && + case "$(git ls-files --stage foo2)" in + 100755" "*foo2) echo pass;; + *) echo fail; git ls-files --stage foo2; (exit 1);; + esac +' + test_done ``` ## Junio C Hamano, 2016-05-25 16:10 Subject: Re: [PATCH] add: add --chmod=+x / --chmod=-x options Message-ID: URL: https://gitlist.dev/e/xmqqoa7u15lq.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Johannes Schindelin writes: > On Wed, 25 May 2016, Junio C Hamano wrote: > >> * I am not familiar with life on filesystems with core.filemode=0; >> do files people would want to be able to "add --chmod=+x" share >> common trait that can be expressed with .gitattributes mechanism? > > I think it is safe to say that the biggest example of core.filemode == 0 > is Windows. On that platform, there simply is no executable bit in the > sense of POSIX permissions. ... > ... I still like Ed's idea and would love to have it: it is murky waters to > require users to call plumbing only because our porcelain isn't up to par. I thought that I made it absolutely clear that I like the addition, too. If it wasn't clear enough, I can say it again, but I do not think you need it ;-). The "attribute" thing was an idea that was hoping to make the system as a whole even more helpful; if pattern matching with paths is sufficient for projects to hint desired permission bits per paths, then those working on such a cross-platform project on Windows do not have to even worry about "git cmd --chmod=+x", whether cmd is add or update-index. If they can just do "git add" and need to use the new "--chmod=+x" option only when the patterns are not set up correctly, wouldn't that be even more helpful? In other words, it wasn't "with this we can _eliminate_ need for 'add --chmod'". The only thing I was unsure about that scheme was if "pattern matching with paths" is sufficiently powerful (if not, such an addition would not work as a mechanism to reduce the need for the users to run "git add --chmod=+x"). And that was my inquiry. Unfortunately, your answer does not help answer that question; it was a question to Edward, so that's OK anyway. ``` ## Johannes Schindelin, 2016-05-25 16:49 Subject: Re: [PATCH] add: add --chmod=+x / --chmod=-x options Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1605251844580.4449%40virtualbox In-Reply-To: ``` Hi Junio, On Wed, 25 May 2016, Junio C Hamano wrote: > Johannes Schindelin writes: > > > On Wed, 25 May 2016, Junio C Hamano wrote: > > > >> * I am not familiar with life on filesystems with core.filemode=0; > >> do files people would want to be able to "add --chmod=+x" share > >> common trait that can be expressed with .gitattributes mechanism? > > > > I think it is safe to say that the biggest example of core.filemode == 0 > > is Windows. On that platform, there simply is no executable bit in the > > sense of POSIX permissions. ... > > ... I still like Ed's idea and would love to have it: it is murky waters to > > require users to call plumbing only because our porcelain isn't up to par. > > I thought that I made it absolutely clear that I like the addition, > too. If it wasn't clear enough, I can say it again, but I do not > think you need it ;-). Oh, I understood that you liked it, sorry if my mail looked accusatory. > The "attribute" thing was an idea that was hoping to make the system > as a whole even more helpful; I understood that, too. My first impression was that it would not be. However, as Git for Windows can set default attributes in /mingw64/etc/gitconfig, I guess it would actually be helpful. We could automatically mark all *.exe, *.com, *.bat, *.cmd files as executable. It would then still be the users' responsibility to add their own attributes for, say, *.js, *.rb, *.py, *.sh, and whatever else. Ciao, Dscho ``` ## Edward Thomson, 2016-05-27 04:41 Subject: Re: [PATCH] add: add --chmod=+x / --chmod=-x options Message-ID: <20160527044112.GA31742@zoidberg> URL: https://gitlist.dev/e/20160527044112.GA31742%40zoidberg In-Reply-To: ``` On Wed, May 25, 2016 at 12:36:55AM -0700, Junio C Hamano wrote: > > At the design level, I have a few comments. Thanks, I will submit a new patch that incorporates your (and dscho's) comments. > * This is about a repository with core.filemode=0; I wonder if > something for a repository with core.symlinks=0 would also help? > That is, would it be a big help to users if they can prepare a > text file that holds symbolic link contents and add it as if it > were a symlink with "git add", instead of having to run two > commands, "hash-objects && update-index --cacheinfo"? I think that this is much less common and - speaking only from personal experience - nobody has ever asked me how to stage a symlink on a Windows machine. I think that this is due to the fact that symlinks on Windows are basically impossible to use, so people doing cross-platform development wouldn't even try. On the other hand, it's quite common for cross-platform teams to use some scripting language since those do work across platforms, and Windows users would want to add new scripts as executable for the benefit of their brethren on platforms with an executable bit. > * I am not familiar with life on filesystems with core.filemode=0; > do files people would want to be able to "add --chmod=+x" share > common trait that can be expressed with .gitattributes mechanism? Perhaps... It would not be things like `*.bat` or `*.exe` - Windows gets those as executable "for free" and would not care about adding the execute bit on those files (since they're not executable anywhere else). It would be items like `*.sh` or `*.rb` that should be executable on POSIX platforms. However I do not think that this is a common enough action that it needs to be made automatic such that when I `git add foo.rb` it is automatically made executable. I think that the reduced complexity of having a single mechanism to control executability (that being the execute mode in the index or a tree) is preferable to a gitattributes based mechanism, at least until somebody else makes a cogent argument that the gitattributes approach would be helpful for them. :) Thanks again for the comments, an updated patch is forthcoming. -ed ``` ## Mike Hommey, 2016-05-27 05:12 Subject: Re: [PATCH] add: add --chmod=+x / --chmod=-x options Message-ID: <20160527051246.GA27092@glandium.org> URL: https://gitlist.dev/e/20160527051246.GA27092%40glandium.org In-Reply-To: <20160527044112.GA31742@zoidberg> ``` On Thu, May 26, 2016 at 11:41:12PM -0500, Edward Thomson wrote: > On Wed, May 25, 2016 at 12:36:55AM -0700, Junio C Hamano wrote: > > > > At the design level, I have a few comments. > > Thanks, I will submit a new patch that incorporates your (and dscho's) > comments. > > > * This is about a repository with core.filemode=0; I wonder if > > something for a repository with core.symlinks=0 would also help? > > That is, would it be a big help to users if they can prepare a > > text file that holds symbolic link contents and add it as if it > > were a symlink with "git add", instead of having to run two > > commands, "hash-objects && update-index --cacheinfo"? > > I think that this is much less common and - speaking only from personal > experience - nobody has ever asked me how to stage a symlink on a > Windows machine. I think that this is due to the fact that symlinks on > Windows are basically impossible to use, so people doing cross-platform > development wouldn't even try. > > On the other hand, it's quite common for cross-platform teams to use > some scripting language since those do work across platforms, and > Windows users would want to add new scripts as executable for the > benefit of their brethren on platforms with an executable bit. > > > * I am not familiar with life on filesystems with core.filemode=0; > > do files people would want to be able to "add --chmod=+x" share > > common trait that can be expressed with .gitattributes mechanism? > > Perhaps... It would not be things like `*.bat` or `*.exe` - Windows > gets those as executable "for free" and would not care about adding the > execute bit on those files (since they're not executable anywhere else). > It would be items like `*.sh` or `*.rb` that should be executable on > POSIX platforms. > > However I do not think that this is a common enough action that it needs > to be made automatic such that when I `git add foo.rb` it is > automatically made executable. Moreover, *.sh, *.rb, etc. are not necessarily meant to be executables. The files might be modules, included from executables or other modules. There's an example of this right in the git tree: t/test-lib.sh. It's even more typical for *.rb files (or *.py, etc.) However, the common pattern that /might/ be interesting for automatic executable bits is files starting with "#!". Mike ``` ## Junio C Hamano, 2016-05-27 06:36 Subject: Re: [PATCH] add: add --chmod=+x / --chmod=-x options Message-ID: URL: https://gitlist.dev/e/xmqqoa7sroru.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20160527044112.GA31742@zoidberg> ``` Edward Thomson writes: > However I do not think that this is a common enough action that it needs > to be made automatic such that when I `git add foo.rb` it is > automatically made executable. I think that the reduced complexity of > having a single mechanism to control executability (that being the > execute mode in the index or a tree) is preferable to a gitattributes > based mechanism, at least until somebody else makes a cogent argument > that the gitattributes approach would be helpful for them. :) It wasn't a "having to specify it every time sucks; you must do this way instead" at all. I was just gauging if it would be a viable idea for a follow-up series to complement your patch. Thanks. ``` ## Junio C Hamano, 2016-05-27 18:30 Subject: Re: [PATCH] add: add --chmod=+x / --chmod=-x options Message-ID: URL: https://gitlist.dev/e/xmqqmvnbqrov.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Junio C Hamano writes: > Edward Thomson writes: > >> However I do not think that this is a common enough action that it needs >> to be made automatic such that when I `git add foo.rb` it is >> automatically made executable. I think that the reduced complexity of >> having a single mechanism to control executability (that being the >> execute mode in the index or a tree) is preferable to a gitattributes >> based mechanism, at least until somebody else makes a cogent argument >> that the gitattributes approach would be helpful for them. :) > > It wasn't a "having to specify it every time sucks; you must do this > way instead" at all. I was just gauging if it would be a viable idea > for a follow-up series to complement your patch. > > Thanks. Oh, having said all of that, the comments on the implementation still stand. ``` ## Junio C Hamano, 2016-05-27 18:35 Subject: Re: [PATCH] add: add --chmod=+x / --chmod=-x options Message-ID: URL: https://gitlist.dev/e/xmqqinxzqrhn.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Johannes Schindelin writes: > I wonder, however, whether it would be "cleaner" to simply make this an > OPT_STRING and perform the validation after the option parsing. Yes, I think I touched on this in my comments in a bit more detail. > Hmm. This change uses up 2 out of 31 available bits. I wonder whether a > better idea would be to extend struct update_callback_data to include a > `force_mode` field, pass a parameter of the same name to > add_files_to_cache() and then handle that in the update_callback(). Maybe. I am not sure if it is a good idea to do lstat(2) on the calling side, though. Assuming it is, your "something like this" needs to be duplicated for the codepath that adds a new file, which is separate from the one we see below (i.e. add_files()). > Something like this: > > case DIFF_STATUS_MODIFIED: > - case DIFF_STATUS_TYPE_CHANGED: > + case DIFF_STATUS_TYPE_CHANGED: { > + struct stat st; > + if (lstat(path, &st)) > + die_errno("unable to stat '%s'", path); > + if (S_ISREG(&st.st_mode) && data->force_mode) > + st.st_mode = data->force_mode; > - if (add_file_to_index(&the_index, path, data->flags)) { > + if (add_to_index(&the_index, path, &st, data->flags)) { > if (!(data->flags & ADD_CACHE_IGNORE_ERRORS)) > die(_("updating files failed")); > data->add_errors++; > } > break; > + } ``` ## Edward Thomson, 2016-05-31 22:06 Subject: Re: [PATCH] add: add --chmod=+x / --chmod=-x options Message-ID: <20160531220650.GA46739@zoidberg> URL: https://gitlist.dev/e/20160531220650.GA46739%40zoidberg In-Reply-To: ``` On Fri, May 27, 2016 at 11:30:40AM -0700, Junio C Hamano wrote: > > Oh, having said all of that, the comments on the implementation > still stand. Certainly; sorry for the delay. I've squashed in your tests and applied your recommendations. Resending the patch momentarily. Thanks- -ed ```