Subject: Re: [RFC/PATCH v11 11/13] bisect--helper: `bisect_next_check` shell function in C

From: Junio C Hamano <[email-masked]>
Date: Tue, 02 Aug 2016 12:17:23 -0700

Pranit Bauva <[email-masked]> writes:

> +static int mark_good(const char *refname, const struct object_id *oid,
> +		     int flag, void *cb_data)
> +{
> +	int *m_good = (int *)cb_data;
> +	*m_good = 0;
> +	return 0;
> +}

See below.

> +static int bisect_next_check(const struct bisect_terms *terms,
> +			     const char *current_term)
> +{
> +	int missing_good = 1, missing_bad = 1;

It is somewhat unusual to start with "assume we are OK" and then
"it turns out that we are not".

> +	char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad.buf);
> +	char *good_glob = xstrfmt("%s*", terms->term_good.buf);

The original runs

    git for-each-ref "refs/bisect/$TERM_GOOD-*

but this one lacks the final dash.

> +	if (ref_exists(bad_ref))
> +		missing_bad = 0;
> +	free(bad_ref);
> +
> +	for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
> +			     (void *) &missing_good);
> +	free(good_glob);

The for-each helper does not return until it iterates over all the
matching refs, but you are only interested in seeing if at least one
exists.  It may make sense to return 1 from mark_good() to terminate
the traversal early.

> +	if (!missing_good && !missing_bad)
> +		return 0;
> +
> +	if (!current_term)
> +		return -1;
> +
> +	if (missing_good && !missing_bad && current_term &&
> +	    !strcmp(current_term, terms->term_good.buf)) {
> +		char *yesno;
> +		/*
> +		 * have bad (or new) but not good (or old). We could bisect
> +		 * although this is less optimum.
> +		 */
> +		fprintf(stderr, "Warning: bisecting only with a %s commit\n",
> +			terms->term_bad.buf);

In the original, this message goes through gettext.

> +		if (!isatty(0))
> +			return 0;
> +		/*
> +		 * TRANSLATORS: Make sure to include [Y] and [n] in your
> +		 * translation. The program will only accept English input
> +		 * at this point.
> +		 */
> +		yesno = git_prompt(_("Are you sure [Y/n]? "), PROMPT_ECHO);
> +		if (starts_with(yesno, "N") || starts_with(yesno, "n"))
> +			return -1;
> +		return 0;
> +	}

When the control falls into the above if(){} block, the function
will always return.  It will clarify that this is the end of such a
logical block to have a blank line here.

> +	if (!is_empty_or_missing_file(git_path_bisect_start()))
> +		return error(_("You need to give me at least one good|old and "
> +				"bad|new revision. You can use \"git bisect "
> +				"bad|new\" and \"git bisect good|old\" for "
> +				"that. \n"));
> +	else
> +		return error(_("You need to start by \"git bisect start\". "
> +				"You then need to give me at least one good|"
> +				"old and bad|new revision. You can use \"git "
> +				"bisect bad|new\" and \"git bisect good|old\" "
> +				" for that.\n"));

The i18n on these two messages seem to be different from the
original, which asks bisect_voc to learn what 'bad' and 'good' are
called and attempts to use these words from the vocabulary.