Re: [RFC PATCH 1/7] add --chmod: don't update index when --dry-run is used

Matheus Tavares [off-list ref] 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.

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.

OK, this side is straight-forward.  We know if we are doing --dry-run;
we have to pass it down.

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.

On Wed, Feb 17, 2021 at 6:46 PM Junio C Hamano [off-list ref]
wrote:

Matheus Tavares [off-list ref] writes:

quoted

`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.

Makes sense, should I send this as a standalone patch, after applying
the suggested changes?

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.

Hmm, when I read your snippet I thought that because this is a small fix
it wouldn't be bad to include the internationalization in the same patch
(with a "While we are here ..." note in the commit message). But are
there other reasons why it is better to do this as a follow-up step? 

quoted

-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.

And that's easier to read too. Thanks!

Also, in a following patch, should we make chmod_pathspec() return `err`
so that we can do:

	exit_status |= chmod_pathspec(&pathspec, chmod_arg[0], show_only);

and have the chmod error reflected in `add`s exit code?