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

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Edward Thomson-2
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 <[hidden email]>
---
 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)

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Junio C Hamano
Edward Thomson <[hidden email]> 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 <[hidden email]>
> ---

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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Johannes Schindelin
In reply to this post by Edward Thomson-2
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 <[hidden email]>

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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Junio C Hamano
In reply to this post by Edward Thomson-2
Edward Thomson <[hidden email]> 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").

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Johannes Schindelin
In reply to this post by Junio C Hamano
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Junio C Hamano
In reply to this post by Junio C Hamano
Junio C Hamano <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Junio C Hamano
In reply to this post by Johannes Schindelin
Johannes Schindelin <[hidden email]> 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.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Johannes Schindelin
Hi Junio,

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

> Johannes Schindelin <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Loading...