git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Davide Berardi <berardi.dav@gmail.com>
Cc: Jeff King <peff@peff.net>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	git@vger.kernel.org
Subject: Re: [PATCH] Segmentation Fault on non-commit --branch clone
Date: Mon, 04 Nov 2019 12:55:09 +0900	[thread overview]
Message-ID: <xmqq36f41ejm.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20191103181657.GA89185@carpenter.lan> (Davide Berardi's message of "Sun, 3 Nov 2019 19:16:57 +0100")

Davide Berardi <berardi.dav@gmail.com> writes:

> The problem with the proposed approach was that the code was
> incompatible with some tests (specifically the tests which specifies an
> empty .git directory would fail and fallback to the unborn master
> branch).
>
> The lookup commit have two error-paths:
>
> 1. the commit cannot be found;
> 2. the commit is found and cannot be casted to a commit (whoops!).
>
> so, I've returned the second condition using an auxiliary variable and
> declaring a new lookup_commit function keeping compatibility with the
> old one.

It's more like three, I think.

  1a. The given object name is a sentinel, "no such object", value.

  1b. The given object name is meant to name a real object, but
      there is no object with such a name in the repository.

  2.  The given object name names an existing object, but it does
      not peel to a commit.

Traditionally, we had only lookup_commit() and died on any of the
above.  Later, we added the _gently() variant for callers that want
to use a commit and also want to handle an error case where the
object name they are handed by their callers does not peel to a
commit.

In the "unborn repository, empty .git" case, are you getting a
random object name, or null_oid (aka case 1a. above)?  If that is
the case, then your solution to introduce another variant of
lookup_commit() that takes *err parameter is a wrong approach would
not differenciate between 1a. and 1b., which would not help, as I
suspect that we still do want to treat 1b. as an error.

Wouldn't it be cleaner to catch 1a. upfront, e.g.

	if (our)
		tip = &our->old_oid;
	else if (remote)
		tip = &remote->old_oid;
	else
		return NULL;

	if (is_null_oid(tip))
        	return NULL;

	tip_commit = lookup_commit_reference_gently(the_repository, tip, 1);
	if (!tip_commit) {
		warning(...);
		create_symref(...);
		return NULL;
	}

I am not offhand sure if the places we return NULL upfront want to
also create HEAD symref, or that is something automatically happens
for us in the downstream of this function, though.

Thanks.

  reply	other threads:[~2019-11-04  3:55 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-01  0:24 Davide Berardi
2019-11-01 19:08 ` Johannes Schindelin
2019-11-01 19:35   ` Jeff King
2019-11-02 10:16     ` Junio C Hamano
2019-11-03 18:16       ` Davide Berardi
2019-11-04  3:55         ` Junio C Hamano [this message]
2019-11-02  9:18   ` Junio C Hamano
2019-11-01 19:43 ` Jeff King
2019-11-02 10:07 ` Junio C Hamano
2019-11-03 18:07 ` [PATCH v2] clone: Don't segfault on -b specifing a non-commit Davide Berardi
2019-11-05  4:37   ` Jeff King
2019-11-06  1:36     ` Junio C Hamano
2019-11-06  4:05       ` Jeff King
2019-11-06  9:53         ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq36f41ejm.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=berardi.dav@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    --subject='Re: [PATCH] Segmentation Fault on non-commit --branch clone' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).