* [PATCH 1/3] add --chmod: don't update index when --dry-run is used
2021-02-23 1:10 [PATCH 0/3] add: minor --chmod fixes Matheus Tavares
@ 2021-02-23 1:10 ` Matheus Tavares
2021-02-23 1:10 ` [PATCH 2/3] add: mark --chmod error string for translation Matheus Tavares
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Matheus Tavares @ 2021-02-23 1:10 UTC (permalink / raw)
To: git; +Cc: gitster, newren
`git add --chmod` applies the mode changes even when `--dry-run` is
used. Fix that and add some tests for this option combination.
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
builtin/add.c | 12 +++++++++---
t/t3700-add.sh | 20 ++++++++++++++++++++
2 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index a825887c50..1e33ab81f2 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -38,17 +38,23 @@ struct update_callback_data {
int add_errors;
};
-static void chmod_pathspec(struct pathspec *pathspec, char flip)
+static void chmod_pathspec(struct pathspec *pathspec, char flip, int show_only)
{
int i;
for (i = 0; i < active_nr; i++) {
struct cache_entry *ce = active_cache[i];
+ int err;
if (pathspec && !ce_path_match(&the_index, ce, pathspec, NULL))
continue;
- if (chmod_cache_entry(ce, flip) < 0)
+ if (!show_only)
+ err = chmod_cache_entry(ce, flip);
+ else
+ err = S_ISREG(ce->ce_mode) ? 0 : -1;
+
+ if (err < 0)
fprintf(stderr, "cannot chmod %cx '%s'\n", flip, ce->name);
}
}
@@ -609,7 +615,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
exit_status |= add_files(&dir, flags);
if (chmod_arg && pathspec.nr)
- chmod_pathspec(&pathspec, chmod_arg[0]);
+ chmod_pathspec(&pathspec, chmod_arg[0], show_only);
unplug_bulk_checkin();
finish:
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index b7d4ba608c..fc81f2ef00 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -386,6 +386,26 @@ test_expect_success POSIXPERM 'git add --chmod=[+-]x does not change the working
! test -x foo4
'
+test_expect_success 'git add --chmod honors --dry-run' '
+ git reset --hard &&
+ echo foo >foo4 &&
+ git add foo4 &&
+ git add --chmod=+x --dry-run foo4 &&
+ test_mode_in_index 100644 foo4
+'
+
+test_expect_success 'git add --chmod --dry-run reports error for non regular files' '
+ git reset --hard &&
+ test_ln_s_add foo foo4 &&
+ git add --chmod=+x --dry-run foo4 2>stderr &&
+ grep "cannot chmod +x .foo4." stderr
+'
+
+test_expect_success 'git add --chmod --dry-run reports error for unmatched pathspec' '
+ test_must_fail git add --chmod=+x --dry-run nonexistent 2>stderr &&
+ test_i18ngrep "pathspec .nonexistent. did not match any files" stderr
+'
+
test_expect_success 'no file status change if no pathspec is given' '
>foo5 &&
>foo6 &&
--
2.30.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/3] add: mark --chmod error string for translation
2021-02-23 1:10 [PATCH 0/3] add: minor --chmod fixes Matheus Tavares
2021-02-23 1:10 ` [PATCH 1/3] add --chmod: don't update index when --dry-run is used Matheus Tavares
@ 2021-02-23 1:10 ` Matheus Tavares
2021-02-23 1:10 ` [PATCH 3/3] add: propagate --chmod errors to exit status Matheus Tavares
2021-02-23 17:08 ` [PATCH 0/3] add: minor --chmod fixes Taylor Blau
3 siblings, 0 replies; 6+ messages in thread
From: Matheus Tavares @ 2021-02-23 1:10 UTC (permalink / raw)
To: git; +Cc: gitster, newren
This error message is intended for humans, so mark it for translation.
Also use error() instead of fprintf(stderr, ...), to make the
corresponding line a bit cleaner, and to display the "error:" prefix,
which helps classifying the nature/severity of the message.
Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
builtin/add.c | 2 +-
t/t3700-add.sh | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index 1e33ab81f2..0c5f53c0bb 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -55,7 +55,7 @@ static void chmod_pathspec(struct pathspec *pathspec, char flip, int show_only)
err = S_ISREG(ce->ce_mode) ? 0 : -1;
if (err < 0)
- fprintf(stderr, "cannot chmod %cx '%s'\n", flip, ce->name);
+ error(_("cannot chmod %cx '%s'"), flip, ce->name);
}
}
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index fc81f2ef00..6a8b94a4f7 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -398,7 +398,7 @@ test_expect_success 'git add --chmod --dry-run reports error for non regular fil
git reset --hard &&
test_ln_s_add foo foo4 &&
git add --chmod=+x --dry-run foo4 2>stderr &&
- grep "cannot chmod +x .foo4." stderr
+ test_i18ngrep "cannot chmod +x .foo4." stderr
'
test_expect_success 'git add --chmod --dry-run reports error for unmatched pathspec' '
--
2.30.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/3] add: propagate --chmod errors to exit status
2021-02-23 1:10 [PATCH 0/3] add: minor --chmod fixes Matheus Tavares
2021-02-23 1:10 ` [PATCH 1/3] add --chmod: don't update index when --dry-run is used Matheus Tavares
2021-02-23 1:10 ` [PATCH 2/3] add: mark --chmod error string for translation Matheus Tavares
@ 2021-02-23 1:10 ` Matheus Tavares
2021-02-23 17:08 ` [PATCH 0/3] add: minor --chmod fixes Taylor Blau
3 siblings, 0 replies; 6+ messages in thread
From: Matheus Tavares @ 2021-02-23 1:10 UTC (permalink / raw)
To: git; +Cc: gitster, newren
If `add` encounters an error while applying the --chmod changes, it
prints a message to stderr, but exits with a success code. This might
have been an oversight, as the command does exit with a non-zero code in
other situations where it cannot (or refuses to) update all of the
requested paths (e.g. when some of the given paths are ignored). So make
the exit behavior more consistent by also propagating --chmod errors to
the exit status.
Note: the test "all statuses changed in folder if . is given" uses paths
added by previous test cases, some of which might be symbolic links.
Because `git add --chmod` will now fail with such paths, this test would
depend on whether all the previous tests were executed, or only some
of them. Avoid that by running the test on a fresh repo with only
regular files.
Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
builtin/add.c | 10 ++++++----
t/t3700-add.sh | 28 ++++++++++++++++++++++------
2 files changed, 28 insertions(+), 10 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index 0c5f53c0bb..ea762a41e3 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -38,9 +38,9 @@ struct update_callback_data {
int add_errors;
};
-static void chmod_pathspec(struct pathspec *pathspec, char flip, int show_only)
+static int chmod_pathspec(struct pathspec *pathspec, char flip, int show_only)
{
- int i;
+ int i, ret = 0;
for (i = 0; i < active_nr; i++) {
struct cache_entry *ce = active_cache[i];
@@ -55,8 +55,10 @@ static void chmod_pathspec(struct pathspec *pathspec, char flip, int show_only)
err = S_ISREG(ce->ce_mode) ? 0 : -1;
if (err < 0)
- error(_("cannot chmod %cx '%s'"), flip, ce->name);
+ ret = error(_("cannot chmod %cx '%s'"), flip, ce->name);
}
+
+ return ret;
}
static int fix_unmerged_status(struct diff_filepair *p,
@@ -615,7 +617,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
exit_status |= add_files(&dir, flags);
if (chmod_arg && pathspec.nr)
- chmod_pathspec(&pathspec, chmod_arg[0], show_only);
+ exit_status |= chmod_pathspec(&pathspec, chmod_arg[0], show_only);
unplug_bulk_checkin();
finish:
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 6a8b94a4f7..d402c775c0 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -386,6 +386,16 @@ test_expect_success POSIXPERM 'git add --chmod=[+-]x does not change the working
! test -x foo4
'
+test_expect_success 'git add --chmod fails with non regular files (but updates the other paths)' '
+ git reset --hard &&
+ test_ln_s_add foo foo3 &&
+ touch foo4 &&
+ test_must_fail git add --chmod=+x foo3 foo4 2>stderr &&
+ test_i18ngrep "cannot chmod +x .foo3." stderr &&
+ test_mode_in_index 120000 foo3 &&
+ test_mode_in_index 100755 foo4
+'
+
test_expect_success 'git add --chmod honors --dry-run' '
git reset --hard &&
echo foo >foo4 &&
@@ -397,7 +407,7 @@ test_expect_success 'git add --chmod honors --dry-run' '
test_expect_success 'git add --chmod --dry-run reports error for non regular files' '
git reset --hard &&
test_ln_s_add foo foo4 &&
- git add --chmod=+x --dry-run foo4 2>stderr &&
+ test_must_fail git add --chmod=+x --dry-run foo4 2>stderr &&
test_i18ngrep "cannot chmod +x .foo4." stderr
'
@@ -429,11 +439,17 @@ test_expect_success 'no file status change if no pathspec is given in subdir' '
'
test_expect_success 'all statuses changed in folder if . is given' '
- rm -fr empty &&
- 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
+ git init repo &&
+ (
+ cd repo &&
+ mkdir -p sub/dir &&
+ touch x y z sub/a sub/dir/b &&
+ git add -A &&
+ 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_expect_success CASE_INSENSITIVE_FS 'path is case-insensitive' '
--
2.30.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 0/3] add: minor --chmod fixes
2021-02-23 1:10 [PATCH 0/3] add: minor --chmod fixes Matheus Tavares
` (2 preceding siblings ...)
2021-02-23 1:10 ` [PATCH 3/3] add: propagate --chmod errors to exit status Matheus Tavares
@ 2021-02-23 17:08 ` Taylor Blau
2021-02-23 18:15 ` Junio C Hamano
3 siblings, 1 reply; 6+ messages in thread
From: Taylor Blau @ 2021-02-23 17:08 UTC (permalink / raw)
To: Matheus Tavares; +Cc: git, gitster, newren
Hi Matheus,
On Mon, Feb 22, 2021 at 10:10:32PM -0300, Matheus Tavares wrote:
> The first patch was broken out from [1], to avoid holding it off while
> there is still work to do on that series. The other two are minor
> related changes.
>
> [1]: https://lore.kernel.org/git/cover.1613593946.git.matheus.bernardino@usp.br/
All look quite reasonable to me, thanks.
Reviewed-by: Taylor Blau <me@ttaylorr.com>
Thanks,
Taylor
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 0/3] add: minor --chmod fixes
2021-02-23 17:08 ` [PATCH 0/3] add: minor --chmod fixes Taylor Blau
@ 2021-02-23 18:15 ` Junio C Hamano
0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2021-02-23 18:15 UTC (permalink / raw)
To: Taylor Blau; +Cc: Matheus Tavares, git, newren
Taylor Blau <me@ttaylorr.com> writes:
> Hi Matheus,
>
> On Mon, Feb 22, 2021 at 10:10:32PM -0300, Matheus Tavares wrote:
>> The first patch was broken out from [1], to avoid holding it off while
>> there is still work to do on that series. The other two are minor
>> related changes.
>>
>> [1]: https://lore.kernel.org/git/cover.1613593946.git.matheus.bernardino@usp.br/
>
> All look quite reasonable to me, thanks.
>
> Reviewed-by: Taylor Blau <me@ttaylorr.com>
Yeah, thanks, both.
^ permalink raw reply [flat|nested] 6+ messages in thread