Re: [PATCH v7 0/5] git bisect old/new

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 23, 2015, 19:04 UTC
Message-ID: <xmqq381idz59.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1435064084-5554-1-git-send-email-Matthieu.Moy@imag.fr>

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> I fixed a few minor issues in v6. Patch between my version and v6 is
> below. I also pushed my branch here:
>
>   https://github.com/moy/git/tree/bisect-terms

It is somewhat confusing to see v3 yesterday and then this v7 next day. How did I miss v4 thru v6?

> Not visible in the patch below: I squashed PATCH 5 into PATCH 3 to
> avoid the pattern "break stuff and then repair it".

Good.

> The first two patches seem ready.

Yeah, the first one is obviously fine ;-), and I agree the second one looks more or less OK.

Regarding the second and third one, the messages they give when the user marked one tip of a side branch as old and the other new gave me a hiccup while reading them, though.

	if (!strcmp(name_bad, "bad")) {
		fprintf(stderr, "The merge base %s is bad.\n"
			"This means the bug has been fixed "
			"between %s and [%s].\n",
			bad_hex, bad_hex, good_hex);
	} else {
		fprintf(stderr, "The merge base %s is %s.\n"
			"This means the first commit marked %s is "
			"between %s and [%s].\n",
			bad_hex, name_bad, name_bad, bad_hex, good_hex);

The "bad" side is inherited from the original and not your fault, but it was already very hard to understand. The other side is not just unreadable, but I think is incorrect and confusing to say "first commit marked %(name_bad)s"; you know there are history segments whose oldest ends (i.e. merge base that is bad) are marked as 'bad', and the other ends are marked as 'good', and you haven't marked any of the commits in between yet. So there is no "first commit marked" either as bad or good there between these endpoints (yet).

Also I was somewhat puzzled and disappointed to still see name_{bad,good} not name_{new,old} used as variable names even in the endgame patch, though. Is that intended?

Show 17 quoted lines

> PATCH 4 (add old/new) is still buggy. When starting a bisection with
>
>   git bisect start $old $new
>
> the command "git bisect visualize" does not work (it shows no commit).
>
> I consider PATCH 5 as WIP, I think it would need a lot of polishing
> and testing to be mergeable. I think a reasonable objective for now it
> to get old/new working in the user-interface, and drop PATCH 5 from
> the series when it gets merged. The existance of PATCH 5 is a very
> good thing even if it doesn't get merged:
>
> * The fact that it's possible to do it on top of the series shows that
>   we make the code more generic. I think it's important that
>   regardless of features, the code moves in the right direction.
>
> * The patch can be taken over later by someone else.

Yeah, if I may rephrase to make sure we are on the same page, in order for 5/5 to be done sanely, 1-4/5 must be giving a good foundation to build on. I agree with that, I agree that including a polished 5/5 would be a good thing, and then I further agree that 1-4/5 could be delivered before 5/5 is ready.

Thanks.