Re: [PATCH] add: add --chmod=+x / --chmod=-x options

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.