Matheus Tavares <matheus.bernardino@xxxxxx> writes:
> `git add --chmod` applies the mode changes even when `--dry-run` is
> used. Fix that and add some tests for this option combination.
Well spotted. I hope we can split this out of the series and fast
track, as it is an obvious bugfix.
I by mistake wrote error(_("...")) in the snippet below, but as a
bugfix, we should stick to the existing fprintf(stderr, "...") without
_(). i18n should be left outside the "bugfix" change.
> -static void chmod_pathspec(struct pathspec *pathspec, char flip)
> +static void chmod_pathspec(struct pathspec *pathspec, char flip, int show_only)
> {
> int i;
>
> @@ -48,7 +48,8 @@ static void chmod_pathspec(struct pathspec *pathspec, char flip)
> if (pathspec && !ce_path_match(&the_index, ce, pathspec, NULL))
> continue;
>
> - if (chmod_cache_entry(ce, flip) < 0)
> + if ((show_only && !S_ISREG(ce->ce_mode)) ||
> + (!show_only && chmod_cache_entry(ce, flip) < 0))
> fprintf(stderr, "cannot chmod %cx '%s'\n", flip, ce->name);
> }
> }
This is a bit dense, especially when the reader does not know by
heart that chmod_cache_entry() refuses to chmod anything that is not
a regular file.
Even when dry-run, we know chmod will fail when the thing is not a
regular file. When not dry-run, we will try chmod and it will
report an failure. And we report an error under these conditions.
if (show_only
? !S_ISREG(ce->ce_mode)
: chmod_cache_entry(ce, flip) < 0)
error(_("cannot chmod ..."), ...);
may express the same idea in a way that is a bit easier to follow.
In any case, that "idea", while it is not wrong per-se, makes it as
if the primary purpose of this code is to give an error message,
which smells a bit funny.
if (!show_only)
err = chmod_cache_entry(ce, flip);
else
err = S_ISREG(ce->ce_mode) ? 0 : -1;
if (err < 0)
error(_("cannot chmod ..."), ...);
would waste one extra variable, but may make the primary point
(i.e. we call chmod_cache_entry() unless dry-run) more clear.
> @@ -609,7 +610,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();
OK, this side is straight-forward. We know if we are doing --dry-run;
we have to pass it down.
> 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
> +'
Nice that test_ln_s_add lets write this without SYMLINKS
prerequisite, as all that matters is what is in the index
and not in the working tree.
> +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 &&
Thanks.
