From 22433ce4617b6ff30c9e9bf03b85d4bb244c3dec Mon Sep 17 00:00:00 2001 From: Thomas Gummerer Date: Wed, 14 Sep 2016 22:07:45 +0100 Subject: [PATCH 1/5] update-index: add test for chmod flags Currently there is no test checking the expected behaviour when multiple chmod flags with different arguments are passed. As argument handling is not in line with other git commands it's easy to miss and accidentally change the current behaviour. While there, fix the argument type of chmod_path, which takes an int, but had a char passed in. Signed-off-by: Thomas Gummerer Signed-off-by: Junio C Hamano --- builtin/update-index.c | 2 +- t/t2107-update-index-basic.sh | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/builtin/update-index.c b/builtin/update-index.c index b8b8522249..57bd5af144 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -419,7 +419,7 @@ static int add_cacheinfo(unsigned int mode, const unsigned char *sha1, return 0; } -static void chmod_path(int flip, const char *path) +static void chmod_path(char flip, const char *path) { int pos; struct cache_entry *ce; diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh index dfe02f4818..32ac6e09bd 100755 --- a/t/t2107-update-index-basic.sh +++ b/t/t2107-update-index-basic.sh @@ -80,4 +80,17 @@ test_expect_success '.lock files cleaned up' ' ) ' +test_expect_success '--chmod=+x and chmod=-x in the same argument list' ' + >A && + >B && + git add A B && + git update-index --chmod=+x A --chmod=-x B && + cat >expect <<-\EOF && + 100755 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 A + 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 B + EOF + git ls-files --stage A B >actual && + test_cmp expect actual +' + test_done From d9d7096662122f6b82ad6e4c08397b75906da78d Mon Sep 17 00:00:00 2001 From: Thomas Gummerer Date: Wed, 14 Sep 2016 22:07:46 +0100 Subject: [PATCH 2/5] read-cache: introduce chmod_index_entry As there are chmod options for both add and update-index, introduce a new chmod_index_entry function to do the work. Use it in update-index, while it will be used in add in the next patch. Signed-off-by: Thomas Gummerer Signed-off-by: Junio C Hamano --- builtin/update-index.c | 16 ++-------------- cache.h | 2 ++ read-cache.c | 29 +++++++++++++++++++++++++++++ 3 files changed, 33 insertions(+), 14 deletions(-) diff --git a/builtin/update-index.c b/builtin/update-index.c index 57bd5af144..8ef21fedc8 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -423,26 +423,14 @@ static void chmod_path(char flip, const char *path) { int pos; struct cache_entry *ce; - unsigned int mode; pos = cache_name_pos(path, strlen(path)); if (pos < 0) goto fail; ce = active_cache[pos]; - mode = ce->ce_mode; - if (!S_ISREG(mode)) + if (chmod_cache_entry(ce, flip) < 0) goto fail; - switch (flip) { - case '+': - ce->ce_mode |= 0111; break; - case '-': - ce->ce_mode &= ~0111; break; - default: - goto fail; - } - cache_tree_invalidate_path(&the_index, path); - ce->ce_flags |= CE_UPDATE_IN_BASE; - active_cache_changed |= CE_ENTRY_CHANGED; + report("chmod %cx '%s'", flip, path); return; fail: diff --git a/cache.h b/cache.h index c73becbf2d..217d12b8d8 100644 --- a/cache.h +++ b/cache.h @@ -369,6 +369,7 @@ extern void free_name_hash(struct index_state *istate); #define remove_file_from_cache(path) remove_file_from_index(&the_index, (path)) #define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags), 0) #define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags), 0) +#define chmod_cache_entry(ce, flip) chmod_index_entry(&the_index, (ce), (flip)) #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL) #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options)) #define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options)) @@ -584,6 +585,7 @@ extern int remove_file_from_index(struct index_state *, const char *path); extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags, int force_mode); extern int add_file_to_index(struct index_state *, const char *path, int flags, int force_mode); extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, unsigned int refresh_options); +extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, char flip); extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b); extern void set_object_name_for_intent_to_add_entry(struct cache_entry *ce); extern int index_name_is_other(const struct index_state *, const char *, int); diff --git a/read-cache.c b/read-cache.c index db27766055..d11a43beb6 100644 --- a/read-cache.c +++ b/read-cache.c @@ -759,6 +759,35 @@ struct cache_entry *make_cache_entry(unsigned int mode, return ret; } +/* + * Chmod an index entry with either +x or -x. + * + * Returns -1 if the chmod for the particular cache entry failed (if it's + * not a regular file), -2 if an invalid flip argument is passed in, 0 + * otherwise. + */ +int chmod_index_entry(struct index_state *istate, struct cache_entry *ce, + char flip) +{ + if (!S_ISREG(ce->ce_mode)) + return -1; + switch (flip) { + case '+': + ce->ce_mode |= 0111; + break; + case '-': + ce->ce_mode &= ~0111; + break; + default: + return -2; + } + cache_tree_invalidate_path(istate, ce->name); + ce->ce_flags |= CE_UPDATE_IN_BASE; + istate->cache_changed |= CE_ENTRY_CHANGED; + + return 0; +} + int ce_same_name(const struct cache_entry *a, const struct cache_entry *b) { int len = ce_namelen(a); From 610d55af0f082f6b866dc858e144c03d8ed4424c Mon Sep 17 00:00:00 2001 From: Thomas Gummerer Date: Wed, 14 Sep 2016 22:07:47 +0100 Subject: [PATCH 3/5] add: modify already added files when --chmod is given When the chmod option was added to git add, it was hooked up to the diff machinery, meaning that it only works when the version in the index differs from the version on disk. As the option was supposed to mirror the chmod option in update-index, which always changes the mode in the index, regardless of the status of the file, make sure the option behaves the same way in git add. Signed-off-by: Thomas Gummerer Signed-off-by: Junio C Hamano --- builtin/add.c | 47 +++++++++++++++++++++++++------------------ builtin/checkout.c | 2 +- builtin/commit.c | 2 +- cache.h | 10 +++++----- read-cache.c | 14 ++++++------- t/t3700-add.sh | 50 ++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 91 insertions(+), 34 deletions(-) diff --git a/builtin/add.c b/builtin/add.c index b1dddb4ac6..e8fb80b36e 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -26,10 +26,25 @@ static int patch_interactive, add_interactive, edit_interactive; static int take_worktree_changes; struct update_callback_data { - int flags, force_mode; + int flags; int add_errors; }; +static void chmod_pathspec(struct pathspec *pathspec, int force_mode) +{ + int i; + + for (i = 0; i < active_nr; i++) { + struct cache_entry *ce = active_cache[i]; + + if (pathspec && !ce_path_match(ce, pathspec, NULL)) + continue; + + if (chmod_cache_entry(ce, force_mode) < 0) + fprintf(stderr, "cannot chmod '%s'", ce->name); + } +} + static int fix_unmerged_status(struct diff_filepair *p, struct update_callback_data *data) { @@ -65,8 +80,7 @@ static void update_callback(struct diff_queue_struct *q, die(_("unexpected diff status %c"), p->status); case DIFF_STATUS_MODIFIED: case DIFF_STATUS_TYPE_CHANGED: - if (add_file_to_index(&the_index, path, - data->flags, data->force_mode)) { + if (add_file_to_index(&the_index, path, data->flags)) { if (!(data->flags & ADD_CACHE_IGNORE_ERRORS)) die(_("updating files failed")); data->add_errors++; @@ -84,15 +98,14 @@ static void update_callback(struct diff_queue_struct *q, } } -int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, - int flags, int force_mode) +int add_files_to_cache(const char *prefix, + const struct pathspec *pathspec, int flags) { struct update_callback_data data; struct rev_info rev; memset(&data, 0, sizeof(data)); data.flags = flags; - data.force_mode = force_mode; init_revisions(&rev, prefix); setup_revisions(0, NULL, &rev, NULL); @@ -281,7 +294,7 @@ static int add_config(const char *var, const char *value, void *cb) return git_default_config(var, value, cb); } -static int add_files(struct dir_struct *dir, int flags, int force_mode) +static int add_files(struct dir_struct *dir, int flags) { int i, exit_status = 0; @@ -294,8 +307,7 @@ static int add_files(struct dir_struct *dir, int flags, int force_mode) } for (i = 0; i < dir->nr; i++) - if (add_file_to_index(&the_index, dir->entries[i]->name, - flags, force_mode)) { + if (add_file_to_index(&the_index, dir->entries[i]->name, flags)) { if (!ignore_add_errors) die(_("adding files failed")); exit_status = 1; @@ -308,7 +320,7 @@ int cmd_add(int argc, const char **argv, const char *prefix) int exit_status = 0; struct pathspec pathspec; struct dir_struct dir; - int flags, force_mode; + int flags; int add_new_files; int require_pathspec; char *seen = NULL; @@ -342,13 +354,8 @@ int cmd_add(int argc, const char **argv, const char *prefix) if (!show_only && ignore_missing) die(_("Option --ignore-missing can only be used together with --dry-run")); - if (!chmod_arg) - force_mode = 0; - else if (!strcmp(chmod_arg, "-x")) - force_mode = 0666; - else if (!strcmp(chmod_arg, "+x")) - force_mode = 0777; - else + if (chmod_arg && ((chmod_arg[0] != '-' && chmod_arg[0] != '+') || + chmod_arg[1] != 'x' || chmod_arg[2])) die(_("--chmod param '%s' must be either -x or +x"), chmod_arg); add_new_files = !take_worktree_changes && !refresh_only; @@ -441,11 +448,13 @@ int cmd_add(int argc, const char **argv, const char *prefix) plug_bulk_checkin(); - exit_status |= add_files_to_cache(prefix, &pathspec, flags, force_mode); + exit_status |= add_files_to_cache(prefix, &pathspec, flags); if (add_new_files) - exit_status |= add_files(&dir, flags, force_mode); + exit_status |= add_files(&dir, flags); + if (chmod_arg && pathspec.nr) + chmod_pathspec(&pathspec, chmod_arg[0]); unplug_bulk_checkin(); finish: diff --git a/builtin/checkout.c b/builtin/checkout.c index c3486bdec3..3398c61e9a 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -548,7 +548,7 @@ static int merge_working_tree(const struct checkout_opts *opts, * entries in the index. */ - add_files_to_cache(NULL, NULL, 0, 0); + add_files_to_cache(NULL, NULL, 0); /* * NEEDSWORK: carrying over local changes * when branches have different end-of-line diff --git a/builtin/commit.c b/builtin/commit.c index 163dbcabf3..443ff9196d 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -386,7 +386,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix */ if (all || (also && pathspec.nr)) { hold_locked_index(&index_lock, 1); - add_files_to_cache(also ? prefix : NULL, &pathspec, 0, 0); + add_files_to_cache(also ? prefix : NULL, &pathspec, 0); refresh_cache_or_die(refresh_flags); update_main_cache_tree(WRITE_TREE_SILENT); if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK)) diff --git a/cache.h b/cache.h index 217d12b8d8..009432f9a8 100644 --- a/cache.h +++ b/cache.h @@ -367,8 +367,8 @@ extern void free_name_hash(struct index_state *istate); #define rename_cache_entry_at(pos, new_name) rename_index_entry_at(&the_index, (pos), (new_name)) #define remove_cache_entry_at(pos) remove_index_entry_at(&the_index, (pos)) #define remove_file_from_cache(path) remove_file_from_index(&the_index, (path)) -#define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags), 0) -#define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags), 0) +#define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags)) +#define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags)) #define chmod_cache_entry(ce, flip) chmod_index_entry(&the_index, (ce), (flip)) #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL) #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options)) @@ -582,8 +582,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 -extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags, int force_mode); -extern int add_file_to_index(struct index_state *, const char *path, int flags, int force_mode); +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); extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, char flip); extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b); @@ -1774,7 +1774,7 @@ void packet_trace_identity(const char *prog); * return 0 if success, 1 - if addition of a file failed and * ADD_FILES_IGNORE_ERRORS was specified in flags */ -int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int flags, int force_mode); +int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int flags); /* diff.c */ extern int diff_auto_refresh_index; diff --git a/read-cache.c b/read-cache.c index d11a43beb6..c2b2e970bc 100644 --- a/read-cache.c +++ b/read-cache.c @@ -630,7 +630,7 @@ void set_object_name_for_intent_to_add_entry(struct cache_entry *ce) hashcpy(ce->sha1, sha1); } -int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags, int force_mode) +int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags) { int size, namelen, was_same; mode_t st_mode = st->st_mode; @@ -659,11 +659,10 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st, else ce->ce_flags |= CE_INTENT_TO_ADD; - if (S_ISREG(st_mode) && force_mode) - ce->ce_mode = create_ce_mode(force_mode); - else if (trust_executable_bit && has_symlinks) + + if (trust_executable_bit && has_symlinks) { ce->ce_mode = create_ce_mode(st_mode); - else { + } else { /* If there is an existing entry, pick the mode bits and type * from it, otherwise assume unexecutable regular file. */ @@ -722,13 +721,12 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st, return 0; } -int add_file_to_index(struct index_state *istate, const char *path, - int flags, int force_mode) +int add_file_to_index(struct index_state *istate, const char *path, int flags) { struct stat st; if (lstat(path, &st)) die_errno("unable to stat '%s'", path); - return add_to_index(istate, path, &st, flags, force_mode); + return add_to_index(istate, path, &st, flags); } struct cache_entry *make_cache_entry(unsigned int mode, diff --git a/t/t3700-add.sh b/t/t3700-add.sh index 2978cb9d64..0a962a60f2 100755 --- a/t/t3700-add.sh +++ b/t/t3700-add.sh @@ -349,4 +349,54 @@ test_expect_success POSIXPERM,SYMLINKS 'git add --chmod=+x with symlinks' ' test_mode_in_index 100755 foo2 ' +test_expect_success 'git add --chmod=[+-]x changes index with already added file' ' + echo foo >foo3 && + git add foo3 && + git add --chmod=+x foo3 && + test_mode_in_index 100755 foo3 && + echo foo >xfoo3 && + chmod 755 xfoo3 && + git add xfoo3 && + git add --chmod=-x xfoo3 && + test_mode_in_index 100644 xfoo3 +' + +test_expect_success 'file status is changed after git add --chmod=+x' ' + echo "AM foo4" >expected && + echo foo >foo4 && + git add foo4 && + git add --chmod=+x foo4 && + git status -s foo4 >actual && + test_cmp expected actual +' + +test_expect_success 'no file status change if no pathspec is given' ' + >foo5 && + >foo6 && + git add foo5 foo6 && + git add --chmod=+x && + test_mode_in_index 100644 foo5 && + test_mode_in_index 100644 foo6 +' + +test_expect_success 'no file status change if no pathspec is given in subdir' ' + mkdir sub && + ( + cd sub && + >sub-foo1 && + >sub-foo2 && + git add . && + git add --chmod=+x && + test_mode_in_index 100644 sub-foo1 && + test_mode_in_index 100644 sub-foo2 + ) +' + +test_expect_success 'all statuses changed in folder if . is given' ' + git add --chmod=+x . && + test $(git ls-files --stage | grep ^100644 | wc -l) -eq 0 && + git add --chmod=-x . && + test $(git ls-files --stage | grep ^100755 | wc -l) -eq 0 +' + test_done From b07ad46432aa4fd80e8ebbcc48d38ec94ae20b01 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Tue, 20 Sep 2016 08:16:11 +0200 Subject: [PATCH 4/5] t3700-add: create subdirectory gently The subdirectory 'sub' is created early in the test file. Later, a test case removes it during its clean-up actions. However, this test case is protected by POSIXPERM. Consequently, 'sub' remains when the POSIXPERM prerequisite is not satisfied. Later, a recently introduced test case creates 'sub' again. Use -p with mkdir so that it does not fail if 'sub' already exists. Signed-off-by: Johannes Sixt Signed-off-by: Junio C Hamano --- t/t3700-add.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t3700-add.sh b/t/t3700-add.sh index 0a962a60f2..16ab2da8a2 100755 --- a/t/t3700-add.sh +++ b/t/t3700-add.sh @@ -380,7 +380,7 @@ test_expect_success 'no file status change if no pathspec is given' ' ' test_expect_success 'no file status change if no pathspec is given in subdir' ' - mkdir sub && + mkdir -p sub && ( cd sub && >sub-foo1 && From 40e0dc17ce6f8b699c0f9426438362ed658293dc Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Tue, 20 Sep 2016 08:18:25 +0200 Subject: [PATCH 5/5] t3700-add: do not check working tree file mode without POSIXPERM A recently introduced test checks the result of 'git status' after setting the executable bit on a file. This check does not yield the expected result when the filesystem does not support the executable bit. What we care about is that a file added with "--chmod=+x" has executable bit in the index and that "--chmod=+x" (or any other options for that matter) does not muck with working tree files. The former is tested by other existing tests, so let's check the latter more explicitly and only under POSIXPERM prerequisite. Signed-off-by: Johannes Sixt Signed-off-by: Junio C Hamano --- t/t3700-add.sh | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/t/t3700-add.sh b/t/t3700-add.sh index 16ab2da8a2..924a266126 100755 --- a/t/t3700-add.sh +++ b/t/t3700-add.sh @@ -361,13 +361,11 @@ test_expect_success 'git add --chmod=[+-]x changes index with already added file test_mode_in_index 100644 xfoo3 ' -test_expect_success 'file status is changed after git add --chmod=+x' ' - echo "AM foo4" >expected && +test_expect_success POSIXPERM 'git add --chmod=[+-]x does not change the working tree' ' echo foo >foo4 && git add foo4 && git add --chmod=+x foo4 && - git status -s foo4 >actual && - test_cmp expected actual + ! test -x foo4 ' test_expect_success 'no file status change if no pathspec is given' '