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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 2, 2019, 09:18 UTC
Message-ID: <xmqqtv7m1vrf.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <nycvar.QRO.7.76.6.1911012000160.46@tvgsbejvaqbjf.bet>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 6 quoted lines

> Hi Davide,
>
> I wonder whether you might want to reword the Subject: such that it
> reflects what the patch does instead of what the problem is that
> motivated you to work on the patch (the patch does not cause the
> segmentation fault, after all, but it fixes it).

Good point.

    Subject: clone: do not segfault on "clone -b B" where B is a non-commit

    The code in "git clone -b B" to decide what branch to check out
    assumed that B points at a commit object without checking,
    leading to dereferencing a NULL pointer and causing a segfault.

    Just aborting the operation when it happens is not a very
    attractive option because we would be left with a directory
    without .git/HEAD that cannot be used as a valid repository the
    user can attempt to recover from the situation by checking out
    something.

    Fall back to use the 'master' branch, which is what we use when
    the command without the "-b" option cannot figure out what
    branch the remote side points with its HEAD.

or something like that, perhaps?

I am not sure if the existing code is careful enough setting up the resulting local 'master' branch, or needs more changes associated with this patch, though. For example, does it want to be set to integrate with the 'master' branch on the remote side by setting "branch.master.remote" and "branch.master.merge" configuration, or do we want to turn them off? I _think_ the answer is that we want to behave as if the user said "-b master" instead of "-b B" (with B that does not point at a commit), but I am not sure. Also, don't we try to be a bit noisier when the fallback fails? For example, if the user said "clone -b master" and the 'master' points at an object that is not a commit, falling back and writing refs/heads/master in the HEAD would leave us in the same position as we did not have any fallback.

I skimmed your review and I think I agree with most (if not all) of them. Thanks.