patchadd: add --chmod=+x / --chmod=-x options

Users on deficient filesystems that lack an execute bit may still wish to add files to the repository with the appropriate execute bit set (or not). Although this can be done in two steps (`git add foo && git update-index --chmod=+x foo`), providing the `--chmod=+x` option to the add command allows users to set a file executable in a single command that they're already familiar with.

Signed-off-by: Edward Thomson <ethomson@edwardthomson.com>
---
 builtin/add.c  | 18 +++++++++++++++++-
 cache.h        |  2 ++
 read-cache.c   |  6 ++++++
 t/t3700-add.sh | 19 +++++++++++++++++++
 4 files changed, 44 insertions(+), 1 deletion(-)

Edward Thomson <ethomson@edwardthomson.com> writes:

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.

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");
	}

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.

Hi Junio,

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

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

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

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.

Hi Junio,

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

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

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

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

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

On Wed, May 25, 2016 at 12:36:55AM -0700, Junio C Hamano wrote:

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

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.

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

On Thu, May 26, 2016 at 11:41:12PM -0500, Edward Thomson wrote:

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

Edward Thomson <ethomson@edwardthomson.com> writes:

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 <gitster@pobox.com> writes:

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

On Fri, May 27, 2016 at 11:30:40AM -0700, Junio C Hamano wrote:

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

Thanks-

-ed

Hi Ed,

On Tue, 24 May 2016, Edward Thomson wrote:

I like it! Some comments below:

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

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

Time to upgrade? ;-)

Ciao, Dscho

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

Yes, I think I touched on this in my comments in a bit more detail.

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

Edward Thomson <ethomson@edwardthomson.com> writes:

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