Re: [PATCH v3] branch: segfault fixes and validation

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 23, 2013, 20:01 UTC
Message-ID: <7vtxp27qa3.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1361622147-20921-1-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

Show 9 quoted lines

>  > What you can do is to have a single helper function that can explain
>  > why branch_get() returned NULL (or extend branch_get() to serve that
>  > purpose as well); then you do not have to duplicate the logic twice
>  > on the caller's side (and there may be other callers that want to do
>  > the same).
>
>  The explanation mentions about the failed operation, which makes a
>  helper less useful. We could still do the helper, but it may lead to
>  i18n legos. So no helper in this version.

Who said explanation has to be conveyed in human language to the caller of the helper? Since there are only two possible cases, at least for now, and I do not think there will be countless many in the future, for the branch_get() function to fail, you can still do:

    	int explain_error;
	struct branch *branch = branch_get(argv[0], &explain_error);

        switch (explain_error) {
	case DEFAULT_HEAD_DETACHED:
        case GIVEN_HEAD_DETACHED:
		die(_("could not make %s upstream of the current branch "
			"because you do not have one"), new_upstream);
		break;
	default:
		break;
        }

and we could even fold the !ref_exists() check that is there for typo-detection purposes into the framework, e.g.

	case GIVEN_BRANCH_DOES_NOT_EXIST:
		die(_("you told me to make %s upstream of %s "
			"but the latter does not exist yet"),
			new_upstream, argv[0]);

if we wanted to preserve what the current test does, no?

Show 7 quoted lines

>  > The existing test might be wrong, by the way.  Your HEAD may point
>  > at a branch Y but you may not have any commit on it yet, and you may
>  > want to allow setting the upstream of that to-be-born branch to
>  > another branch X with "branch --set-upstream-to=X [Y|HEAD]".
>
>  It sounds complicated. I think we can revisit it when a user actually
>  complains about it.

Yeah, will replace the previous one and queue this version.

Thanks.