On Wed, Feb 17, 2021 at 6:46 PM Junio C Hamano <gitster@xxxxxxxxx>
wrote:
>
> 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.
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?
> > -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?
