Re: [PATCH] Segmentation Fault on non-commit --branch clone

From: Jeff King <peff@peff.net>
Date: Nov 1, 2019, 19:35 UTC
Message-ID: <20191101193558.GA1169@sigill.intra.peff.net>
In-Reply-To: <nycvar.QRO.7.76.6.1911012000160.46@tvgsbejvaqbjf.bet>

On Fri, Nov 01, 2019 at 08:08:10PM +0100, Johannes Schindelin wrote:

Show 17 quoted lines

> > +static int fallback_on_noncommit(const struct ref *check,
> > +				 const struct ref *remote,
> > +				 const char *msg)
> > +{
> > +	if (check == NULL)
> > +		return 1;
> > +	struct commit *c = lookup_commit_reference_gently(the_repository,
> > +						   &check->old_oid, 1);
> > +	if (c == NULL) {
> > +		/* Fallback HEAD to fallback refs */
> > +		warning(_("%s is not a valid commit object, HEAD will fallback
> > to %s"),
> > +			check->name, FALLBACK_REF);
> 
> Quite honestly, I do not think that it is a good idea to fall back in
> this case. The user asked for something that cannot be accomplished, and
> the best way to handle this is to exit with an error, i.e. `die()`.

The main reason I proposed falling back here is that the user can correct the situation without having to redo the clone from scratch (which might have been very expensive). And we cannot just leave HEAD empty there; we have to put _something_ in it.

I do think it's important, though, that we don't just fall back; we should still report an error exit from the program (just as we do for the similar case when clone's checkout step fails). Otherwise something as simple as:

  git clone -b $url repo &&
  cd repo &&
  do_something

could have quite unexpected results.

I don't know how often this would actually help users, though. It _is_ a pretty rare situation to ask for a non-commit. So maybe it's all over-engineering, and we should start with just die(). If somebody comes along later and wants to enhance it, it should be pretty straightforward.

-Peff