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

[Jeff King <peff@peff.net>]

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

> > +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