Re: [PATCH] for-each-repo: do nothing on empty config
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Jan 6, 2021, 04:20 UTC
- Message-ID
- <CAPig+cRGWzz5n_PZ=_OiHy2hkmeru3=fo2zX3_hnsEhnPq+giQ@mail.gmail.com>
- In-Reply-To
- <db7bf751-7541-59b9-a3a0-ec07e28eb9de@gmail.com>
On Tue, Jan 5, 2021 at 9:20 PM Derrick Stolee <stolee@gmail.com> wrote:
Show 18 quoted lines
> On 1/5/2021 12:55 PM, Eric Sunshine wrote:
> > On Tue, Jan 5, 2021 at 9:44 AM Derrick Stolee via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> > Probably not worth a re-roll, but the above has higher cognitive load than:
> >
> > if (!value)
> > return 0;
> >
> > which indicates clearly that the command succeeded, whereas `return
> > result` makes the reader scan all the code above the conditional to
> > figure out what values `result` could possibly hold.
>
> True. Looking at this again, it might be better to just update the
> loop to be
>
> for (i = 0; values && i < values->nr; i++) {
>
> which would run the loop zero times when there are zero results.I find the explicit `return 0` easier to reason about since I can see immediately that it handles a particular boundary condition, after which I no longer have to think about that condition as I continue reading the code.
That said, I don't think it matters greatly one way or the other whether you use `return result`, `return 0`, or adjust the loop condition. It might matter if more functionality is added later, but we don't have to worry about it now, and it's not worth re-rolling just for this, or spending a lot of time discussing it.
Show 15 quoted lines
> >> + git for-each-repo --config=bogus.config -- these args would fail > > > > The `these args would fail` arguments add mystery to the test, > > prompting the reader to wonder what exactly is going on: "Fail how?", > > "Is it supposed to fail?". It might be less confusing to use something > > more direct like `nonexistent` or `does not exist`. > > I guess the point is that if we actually did try running a subcommand > on a repo (for instance, accidentally running it on the current repo) > then the command would fail when running "git these args would fail". > > To me, "nonexistent" or "does not exist" doesn't adequately describe > this (hypothetical) situation. > > Perhaps "fail-subcommand" might be better?
I had never even looked at git-for-each-repo before, so I took the time to read the documentation and the source code before writing this reply. Now that I understand what is supposed to happen, I might be tempted to suggest `this-command-wont-be-run` as an argument, but that's a mouthful. If you want to be really explicit that it should fail if a bug gets re-introduced which causes the argument to be run even though the config is empty, then perhaps use `false`:
git for-each-repo --config=bogus.config -- false
By the way, when reading the documentation, some questions came to mind.
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? That might be legitimate in automation situations. If legitimate to have an empty config, then would it be helpful to somehow distinguish between an empty config variable and a non-existent one (assuming we can even do that)?
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.