Re: [PATCH 2/2] remote.c: reject 0-length branch names

From: Jeff King <peff@peff.net>
Date: Jun 8, 2022, 23:24 UTC
Message-ID: <YqEvoB3Bskpykh1w@coredump.intra.peff.net>
In-Reply-To: <kl6lilpke31e.fsf@chooglen-macbookpro.roam.corp.google.com>

On Wed, Jun 01, 2022 at 09:55:57AM -0700, Glen Choo wrote:

Show 12 quoted lines

> > Are we confident that this is just bizarro config that nobody would have
> > had in practice? In that case I think it's fine to start dying on it.
> >
> > But as I understand we previously just ignored this, then if there's any
> > doubt about that perhaps we should start with a warning?
> >
> > Or are we really confident that this is an edge case not worth worrying
> > about in that way, and that we can go straight to die()?
> 
> The case I want to make is even stronger than that - this is an edge
> case that _we_ shouldn't worry about, and we should tell the _user_ that
> their config is bogus.

It's a tradeoff, isn't it? We don't know how the user ended up with this config, what they were trying to do, nor how common it is. Clearly the config makes no sense and is broken, but by alerting the user, we are:

  - maybe doing some good, because now they know that whatever they were
    trying to do didn't work, and can clean up the broken config

  - maybe doing some bad, because it was not (and is not) hurting
    anything to have config that nobody bothers to do anything with. But
    if we die, now the user is presented with a situation that they know
    nothing about, and must resolve it before continuing with their
    unrelated work.

-Peff