Re: [PATCH] for-each-repo: do nothing on empty config

On Wed, Jan 6, 2021 at 6:54 AM Derrick Stolee [off-list ref] wrote:

On 1/5/2021 11:20 PM, Eric Sunshine wrote:

quoted

Is the behavior implemented by this patch desirable? That is, if the
user mistypes the name of the configuration variable, would it be
better to let the user know about the problem by returning an error
code rather than succeeding silently? Or do you foresee people
intentionally running the command against an empty config variable?

As mentioned in the message, this is the situation the background
maintenance would see if a user used 'git maintenance unregister' on
all of their repos or removed the 'maintenance.repo' config values
from their global config. I think it would be better to not start
failing the background commands.

Even outside of that context, we have no way to specify an "existing
but empty" multi-valued config value over a non-existing config
value. I'd rather prefer the interpretation "I succeeded in doing
nothing" instead of "I think you made a mistake, because there's
nothing to do."

Could we meet in the middle and print a warning to stderr?

        warning(_("supplied config key '%s' has no values"));

That would present a useful message to users running this command
manually (or constructing automation) without breaking scripts
that parse the output.

I don't know. My knee-jerk response is that such a warning could get
annoying quickly if this is used mostly in an automated environment,
and an empty config value is likely to come up in practice. In that
case, I'm somewhat negative on adding the warning. (I could perhaps
see a --dry-run or --verbose mode issuing the above warning, but
that's outside the scope of this series.)

quoted

Is the API of this command ideal? It feels odd to force the user to
specify required input via a command-line option rather than just as a
positional argument. In other words, since the config variable name is
mandatory, an alternative invocation format would be:

    git for-each-repo <config-var> <cmd>

Or do you foresee future enhancements expanding the ways in which the
repo list can be specified (such as from a file or stdin or directly
via the command-line), in which case the `--config` option makes
sense.

I believe some voice interest in specifying a list of repositories
by providing a directory instead of a config value. That would
require scanning the immediate subdirectories to see if they are Git
repos.

A --stdin option would also be a good addition, if desired.

Since this command was intended for automation, not end-users, it
seemed that future-proofing the arguments in this way would be a good
idea. We want to remain flexible for future additions without
breaking compatibility.

Fair enough.