From: Matheus Tavares <matheus.bernardino@usp.br>
Date: Thu, 18 Feb 2021 01:33:30 GMT
Subject: Re: [RFC PATCH 1/7] add --chmod: don't update index when --dry-run is used
Message-ID: <20210218013330.7491-1-matheus.bernardino@usp.br>
In-Reply-To: <xmqqa6s2jrl9.fsf@gitster.g>

On Wed, Feb 17, 2021 at 6:46 PM Junio C Hamano <gitster@pobox.com>
wrote:
>
> Matheus Tavares <matheus.bernardino@usp.br> 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?