{"thread":{"id":"55188","subject":"[PATCH 0/3] add: minor --chmod fixes","startedAt":"2021-02-23T01:11:41Z","lastAt":"2021-02-23T18:16:36Z","messageCount":6,"participants":["Matheus Tavares","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"417514","messageId":"cover.1614037664.git.matheus.bernardino@usp.br","threadId":"55188","inReplyTo":null,"subject":"[PATCH 0/3] add: minor --chmod fixes","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-02-23T01:10:32Z","receivedAt":"2021-02-23T01:11:41Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"The first patch was broken out from [1], to avoid holding it off while\nthere is still work to do on that series. The other two are minor\nrelated changes.\n\n[1]: https://lore.kernel.org/git/cover.1613593946.git.matheus.bernardino@usp.br/\n\nMatheus Tavares (3):\n add --chmod: don't update index when --dry-run is used\n add: mark --chmod error string for translation\n add: propagate --chmod errors to exit status\n\n builtin/add.c | 18 +++++++++++++-----\n t/t3700-add.sh | 46 +++++++++++++++++++++++++++++++++++++++++-----\n 2 files changed, 54 insertions(+), 10 deletions(-)\n\n-- \n2.30.1\n\n"},{"id":"417515","messageId":"8ca9ba0d7f1e6efb04cae5c2ff00c9c9af50362d.1614037664.git.matheus.bernardino@usp.br","threadId":"55188","inReplyTo":"cover.1614037664.git.matheus.bernardino@usp.br","subject":"[PATCH 1/3] add --chmod: don't update index when --dry-run is used","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-02-23T01:10:33Z","receivedAt":"2021-02-23T01:11:41Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"`git add --chmod` applies the mode changes even when `--dry-run` is\nused. Fix that and add some tests for this option combination.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Matheus Tavares \n---\n builtin/add.c | 12 +++++++++---\n t/t3700-add.sh | 20 ++++++++++++++++++++\n 2 files changed, 29 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex a825887c50..1e33ab81f2 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -38,17 +38,23 @@ struct update_callback_data {\n \tint add_errors;\n };\n \n-static void chmod_pathspec(struct pathspec *pathspec, char flip)\n+static void chmod_pathspec(struct pathspec *pathspec, char flip, int show_only)\n {\n \tint i;\n \n \tfor (i = 0; i < active_nr; i++) {\n \t\tstruct cache_entry *ce = active_cache[i];\n+\t\tint err;\n \n \t\tif (pathspec && !ce_path_match(&the_index, ce, pathspec, NULL))\n \t\t\tcontinue;\n \n-\t\tif (chmod_cache_entry(ce, flip) < 0)\n+\t\tif (!show_only)\n+\t\t\terr = chmod_cache_entry(ce, flip);\n+\t\telse\n+\t\t\terr = S_ISREG(ce->ce_mode) ? 0 : -1;\n+\n+\t\tif (err < 0)\n \t\t\tfprintf(stderr, \"cannot chmod %cx '%s'\\n\", flip, ce->name);\n \t}\n }\n@@ -609,7 +615,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\texit_status |= add_files(&dir, flags);\n \n \tif (chmod_arg && pathspec.nr)\n-\t\tchmod_pathspec(&pathspec, chmod_arg[0]);\n+\t\tchmod_pathspec(&pathspec, chmod_arg[0], show_only);\n \tunplug_bulk_checkin();\n \n finish:\ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex b7d4ba608c..fc81f2ef00 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -386,6 +386,26 @@ test_expect_success POSIXPERM 'git add --chmod=[+-]x does not change the working\n \t! test -x foo4\n '\n \n+test_expect_success 'git add --chmod honors --dry-run' '\n+\tgit reset --hard &&\n+\techo foo >foo4 &&\n+\tgit add foo4 &&\n+\tgit add --chmod=+x --dry-run foo4 &&\n+\ttest_mode_in_index 100644 foo4\n+'\n+\n+test_expect_success 'git add --chmod --dry-run reports error for non regular files' '\n+\tgit reset --hard &&\n+\ttest_ln_s_add foo foo4 &&\n+\tgit add --chmod=+x --dry-run foo4 2>stderr &&\n+\tgrep \"cannot chmod +x .foo4.\" stderr\n+'\n+\n+test_expect_success 'git add --chmod --dry-run reports error for unmatched pathspec' '\n+\ttest_must_fail git add --chmod=+x --dry-run nonexistent 2>stderr &&\n+\ttest_i18ngrep \"pathspec .nonexistent. did not match any files\" stderr\n+'\n+\n test_expect_success 'no file status change if no pathspec is given' '\n \t>foo5 &&\n \t>foo6 &&\n-- \n2.30.1\n\n"},{"id":"417516","messageId":"b65d3a1c3771617caea175830409a103f45e9c73.1614037664.git.matheus.bernardino@usp.br","threadId":"55188","inReplyTo":"cover.1614037664.git.matheus.bernardino@usp.br","subject":"[PATCH 2/3] add: mark --chmod error string for translation","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-02-23T01:10:34Z","receivedAt":"2021-02-23T01:11:58Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"This error message is intended for humans, so mark it for translation.\nAlso use error() instead of fprintf(stderr, ...), to make the\ncorresponding line a bit cleaner, and to display the \"error:\" prefix,\nwhich helps classifying the nature/severity of the message.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/add.c | 2 +-\n t/t3700-add.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 1e33ab81f2..0c5f53c0bb 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -55,7 +55,7 @@ static void chmod_pathspec(struct pathspec *pathspec, char flip, int show_only)\n \t\t\terr = S_ISREG(ce->ce_mode) ? 0 : -1;\n \n \t\tif (err < 0)\n-\t\t\tfprintf(stderr, \"cannot chmod %cx '%s'\\n\", flip, ce->name);\n+\t\t\terror(_(\"cannot chmod %cx '%s'\"), flip, ce->name);\n \t}\n }\n \ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex fc81f2ef00..6a8b94a4f7 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -398,7 +398,7 @@ test_expect_success 'git add --chmod --dry-run reports error for non regular fil\n \tgit reset --hard &&\n \ttest_ln_s_add foo foo4 &&\n \tgit add --chmod=+x --dry-run foo4 2>stderr &&\n-\tgrep \"cannot chmod +x .foo4.\" stderr\n+\ttest_i18ngrep \"cannot chmod +x .foo4.\" stderr\n '\n \n test_expect_success 'git add --chmod --dry-run reports error for unmatched pathspec' '\n-- \n2.30.1\n\n"},{"id":"417517","messageId":"f3fe47aa6d9aec39e521182120c732d12bdfe96c.1614037664.git.matheus.bernardino@usp.br","threadId":"55188","inReplyTo":"cover.1614037664.git.matheus.bernardino@usp.br","subject":"[PATCH 3/3] add: propagate --chmod errors to exit status","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-02-23T01:10:35Z","receivedAt":"2021-02-23T01:11:59Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"If `add` encounters an error while applying the --chmod changes, it\nprints a message to stderr, but exits with a success code. This might\nhave been an oversight, as the command does exit with a non-zero code in\nother situations where it cannot (or refuses to) update all of the\nrequested paths (e.g. when some of the given paths are ignored). So make\nthe exit behavior more consistent by also propagating --chmod errors to\nthe exit status.\n\nNote: the test \"all statuses changed in folder if . is given\" uses paths\nadded by previous test cases, some of which might be symbolic links.\nBecause `git add --chmod` will now fail with such paths, this test would\ndepend on whether all the previous tests were executed, or only some\nof them. Avoid that by running the test on a fresh repo with only\nregular files.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/add.c | 10 ++++++----\n t/t3700-add.sh | 28 ++++++++++++++++++++++------\n 2 files changed, 28 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 0c5f53c0bb..ea762a41e3 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -38,9 +38,9 @@ struct update_callback_data {\n \tint add_errors;\n };\n \n-static void chmod_pathspec(struct pathspec *pathspec, char flip, int show_only)\n+static int chmod_pathspec(struct pathspec *pathspec, char flip, int show_only)\n {\n-\tint i;\n+\tint i, ret = 0;\n \n \tfor (i = 0; i < active_nr; i++) {\n \t\tstruct cache_entry *ce = active_cache[i];\n@@ -55,8 +55,10 @@ static void chmod_pathspec(struct pathspec *pathspec, char flip, int show_only)\n \t\t\terr = S_ISREG(ce->ce_mode) ? 0 : -1;\n \n \t\tif (err < 0)\n-\t\t\terror(_(\"cannot chmod %cx '%s'\"), flip, ce->name);\n+\t\t\tret = error(_(\"cannot chmod %cx '%s'\"), flip, ce->name);\n \t}\n+\n+\treturn ret;\n }\n \n static int fix_unmerged_status(struct diff_filepair *p,\n@@ -615,7 +617,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\texit_status |= add_files(&dir, flags);\n \n \tif (chmod_arg && pathspec.nr)\n-\t\tchmod_pathspec(&pathspec, chmod_arg[0], show_only);\n+\t\texit_status |= chmod_pathspec(&pathspec, chmod_arg[0], show_only);\n \tunplug_bulk_checkin();\n \n finish:\ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex 6a8b94a4f7..d402c775c0 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -386,6 +386,16 @@ test_expect_success POSIXPERM 'git add --chmod=[+-]x does not change the working\n \t! test -x foo4\n '\n \n+test_expect_success 'git add --chmod fails with non regular files (but updates the other paths)' '\n+\tgit reset --hard &&\n+\ttest_ln_s_add foo foo3 &&\n+\ttouch foo4 &&\n+\ttest_must_fail git add --chmod=+x foo3 foo4 2>stderr &&\n+\ttest_i18ngrep \"cannot chmod +x .foo3.\" stderr &&\n+\ttest_mode_in_index 120000 foo3 &&\n+\ttest_mode_in_index 100755 foo4\n+'\n+\n test_expect_success 'git add --chmod honors --dry-run' '\n \tgit reset --hard &&\n \techo foo >foo4 &&\n@@ -397,7 +407,7 @@ test_expect_success 'git add --chmod honors --dry-run' '\n test_expect_success 'git add --chmod --dry-run reports error for non regular files' '\n \tgit reset --hard &&\n \ttest_ln_s_add foo foo4 &&\n-\tgit add --chmod=+x --dry-run foo4 2>stderr &&\n+\ttest_must_fail git add --chmod=+x --dry-run foo4 2>stderr &&\n \ttest_i18ngrep \"cannot chmod +x .foo4.\" stderr\n '\n \n@@ -429,11 +439,17 @@ test_expect_success 'no file status change if no pathspec is given in subdir' '\n '\n \n test_expect_success 'all statuses changed in folder if . is given' '\n-\trm -fr empty &&\n-\tgit add --chmod=+x . &&\n-\ttest $(git ls-files --stage | grep ^100644 | wc -l) -eq 0 &&\n-\tgit add --chmod=-x . &&\n-\ttest $(git ls-files --stage | grep ^100755 | wc -l) -eq 0\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tmkdir -p sub/dir &&\n+\t\ttouch x y z sub/a sub/dir/b &&\n+\t\tgit add -A &&\n+\t\tgit add --chmod=+x . &&\n+\t\ttest $(git ls-files --stage | grep ^100644 | wc -l) -eq 0 &&\n+\t\tgit add --chmod=-x . &&\n+\t\ttest $(git ls-files --stage | grep ^100755 | wc -l) -eq 0\n+\t)\n '\n \n test_expect_success CASE_INSENSITIVE_FS 'path is case-insensitive' '\n-- \n2.30.1\n\n"},{"id":"417568","messageId":"YDU2lzxPqsOwjv9N@nand.local","threadId":"55188","inReplyTo":"cover.1614037664.git.matheus.bernardino@usp.br","subject":"Re: [PATCH 0/3] add: minor --chmod fixes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-02-23T17:08:39Z","receivedAt":"2021-02-23T17:09:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Matheus,\n\nOn Mon, Feb 22, 2021 at 10:10:32PM -0300, Matheus Tavares wrote:\n> The first patch was broken out from [1], to avoid holding it off while\n> there is still work to do on that series. The other two are minor\n> related changes.\n>\n> [1]: https://lore.kernel.org/git/cover.1613593946.git.matheus.bernardino@usp.br/\n\nAll look quite reasonable to me, thanks.\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"417574","messageId":"xmqq35xm4pmz.fsf@gitster.g","threadId":"55188","inReplyTo":"YDU2lzxPqsOwjv9N@nand.local","subject":"Re: [PATCH 0/3] add: minor --chmod fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-23T18:15:16Z","receivedAt":"2021-02-23T18:16:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Hi Matheus,\n>\n> On Mon, Feb 22, 2021 at 10:10:32PM -0300, Matheus Tavares wrote:\n>> The first patch was broken out from [1], to avoid holding it off while\n>> there is still work to do on that series. The other two are minor\n>> related changes.\n>>\n>> [1]: https://lore.kernel.org/git/cover.1613593946.git.matheus.bernardino@usp.br/\n>\n> All look quite reasonable to me, thanks.\n>\n> Reviewed-by: Taylor Blau \n\nYeah, thanks, both.\n"}]}