Introduce --first-parent flag for git bisect #686
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add first_parent_only parameter to find_bisection(), removing the barrier that prevented combining the --bisect and --first-parent flags when using git rev-list Signed-off-by: Aaron Lipman <[email protected]> Based-on-patch-by: Tiago Botelho <[email protected]>
Welcome to GitGitGadgetHi @alipman88, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that your Pull Request has a good description, as it will be used as cover letter. Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join [email protected], where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
alipman88
force-pushed
the
bisect_first_parent_final_draft
branch
from
fb8391d to
06436e9
Compare
Upon seeing a merge commit when bisecting, this option may be used to follow only the first parent. In detecting regressions introduced through the merging of a branch, the merge commit will be identified as introduction of the bug and its ancestors will be ignored. This option is particularly useful in avoiding false positives when a merged branch contained broken or non-buildable commits, but the merge itself was OK. Signed-off-by: Aaron Lipman <[email protected]>
Now that find_bisection() accepts multiple boolean arguments, these may be combined into a single unsigned integer in order to declutter some of the code in bisect.c Signed-off-by: Aaron Lipman <[email protected]> Based-on-patch-by: Harald Nordgren <[email protected]>
alipman88
force-pushed
the
bisect_first_parent_final_draft
branch
from
06436e9 to
24d1168
Compare
|
/allow |
|
User alipman88 is now allowed to use GitGitGadget. WARNING: alipman88 has no public email address set on GitHub |
|
/preview |
|
Preview email sent as [email protected] |
|
/submit |
|
Submitted as [email protected] |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -128,8 +128,7 @@ parents) and `--max-parents=-1` (negative numbers denote no upper limit). | |||
| because merges into a topic branch tend to be only about | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Aaron Lipman via GitGitGadget" <[email protected]> writes:
> From: Aaron Lipman <[email protected]>
>
> Add first_parent_only parameter to find_bisection(), removing the
> barrier that prevented combining the --bisect and --first-parent flags
> when using git rev-list
>
> Signed-off-by: Aaron Lipman <[email protected]>
> Based-on-patch-by: Tiago Botelho <[email protected]>
In chronological order, please.
> diff --git a/bisect.c b/bisect.c
> index d5e830410f..762f68c8e9 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -37,7 +37,7 @@ static const char *term_good;
> * We care just barely enough to avoid recursing for
> * non-merge entries.
> */
> -static int count_distance(struct commit_list *entry)
> +static int count_distance(struct commit_list *entry, int first_parent_only)
> {
> int nr = 0;
>
> @@ -52,10 +52,10 @@ static int count_distance(struct commit_list *entry)
> commit->object.flags |= COUNTED;
> p = commit->parents;
> entry = p;
> - if (p) {
> + if (p && !first_parent_only) {
> p = p->next;
> while (p) {
> - nr += count_distance(p);
> + nr += count_distance(p, first_parent_only);
> p = p->next;
> }
> }
If you are running a first-parent-only bisection, by default, each
commit in the "goodness not yet known" region is treated as a single
parent commit for the purpose of bisection. As do_find_bisection()
avoids the cost of calling stupid-and-expensive count_distance() by
treating such a single-strand-of-pearls specially, wouldn't it be a
BUG() if this funtion is called under first_parent_only mode in the
first place?
> @@ -88,15 +88,16 @@ static inline void weight_set(struct commit_list *elem, int weight)
> **commit_weight_at(&commit_weight, elem->item) = weight;
> }
> -static int count_interesting_parents(struct commit *commit)
> +static int count_interesting_parents(struct commit *commit, int first_parent_only)
> {
> struct commit_list *p;
> int count;
>
> for (count = 0, p = commit->parents; p; p = p->next) {
> - if (p->item->object.flags & UNINTERESTING)
> - continue;
> - count++;
> + if (!(p->item->object.flags & UNINTERESTING))
> + count++;
> + if (first_parent_only)
> + break;
> }
> return count;
> }
Any merge will be checked for interesting-ness of its first parent;
there is either zero or one interesting parent and no other case
under first-parent-only. OK.
> @@ -259,7 +260,7 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n
> */
> static struct commit_list *do_find_bisection(struct commit_list *list,
> int nr, int *weights,
> - int find_all)
> + int find_all, int first_parent_only)
> {
> int n, counted;
> struct commit_list *p;
> @@ -271,7 +272,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
> unsigned flags = commit->object.flags;
>
> *commit_weight_at(&commit_weight, p->item) = &weights[n++];
> - switch (count_interesting_parents(commit)) {
> + switch (count_interesting_parents(commit, first_parent_only)) {
> case 0:
> if (!(flags & TREESAME)) {
> weight_set(p, 1);
> @@ -314,7 +315,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
> continue;
> if (weight(p) != -2)
> continue;
If count-interesting-parents can never be 2 or more, you will never
see weight(p) == -2 (see the switch statement in the previous hunk).
So it your first-parent-only mode allowed this continue not to
trigger, you have a BUG(), I think.
> - weight_set(p, count_distance(p));
> + weight_set(p, count_distance(p, first_parent_only));
Hence, you do not need to touch count_distance() at all, no?
> @@ -330,13 +331,21 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
> struct commit_list *q;
> unsigned flags = p->item->object.flags;
>
> + /* if commit p has already been weighted, continue. */
> if (0 <= weight(p))
> continue;
> +
> + /*
> + * otherwise, find the first interesting
> + * already-weighted parent of p and store as q.
> + */
Unnecessary changes above.
> for (q = p->item->parents; q; q = q->next) {
> - if (q->item->object.flags & UNINTERESTING)
> - continue;
> - if (0 <= weight(q))
> + if (!(q->item->object.flags & UNINTERESTING) && 0 <= weight(q)) {
> + break;
Overlong line. Fold it after "&&", like this:
if (!(q->item->object.flags & UNINTERESTING) &&
0 <= weight(q)) {
What is this change about?
Ah, if the first parent is uninteresting, we do not want to
continue, but with the original loop, we may go on and check
q->next. We just want to break out of the loop instead.
I strongly suspect that
for (q = p->item->parents;
q;
q = first_parent_only ? NULL : q->next) {
without any change inside the loop is much easier to reason about.
We follow exactly the same logic as the usual case. We just pretend
that all commits have only one single parent which is the first one.
Thanks.
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Taking all the suggestions and speculations I made in the message I
am responding to, I think squashing the following changes into the
[PATCH 1/3] would make the code simpler and much easier to follow.
The first two hunks revert the changes made to count_distance(); the
third hunk makes sure that count_distance() is not called under the
first-parent mode, since the "single-strand-of-pearls" optimization
is the only counting method needed in the first-parent mode. The
last hunk is to revert the changes to the single-strand-of-pearls
optimized counting and tweak the loop control to take only the first
parent into account.
Your new tests in 6000 and 6002 of course still pass with this
clean-up.
bisect.c | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/bisect.c b/bisect.c
index 762f68c8e9..a11fdb1473 100644
--- a/bisect.c
+++ b/bisect.c
@@ -37,7 +37,7 @@ static const char *term_good;
* We care just barely enough to avoid recursing for
* non-merge entries.
*/
-static int count_distance(struct commit_list *entry, int first_parent_only)
+static int count_distance(struct commit_list *entry)
{
int nr = 0;
@@ -52,10 +52,10 @@ static int count_distance(struct commit_list *entry, int first_parent_only)
commit->object.flags |= COUNTED;
p = commit->parents;
entry = p;
- if (p && !first_parent_only) {
+ if (p) {
p = p->next;
while (p) {
- nr += count_distance(p, first_parent_only);
+ nr += count_distance(p);
p = p->next;
}
}
@@ -315,7 +315,9 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
continue;
if (weight(p) != -2)
continue;
- weight_set(p, count_distance(p, first_parent_only));
+ if (first_parent_only)
+ BUG("shouldn't be calling count-distance in fp mode");
+ weight_set(p, count_distance(p));
clear_distance(list);
/* Does it happen to be at exactly half-way? */
@@ -331,21 +333,16 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
struct commit_list *q;
unsigned flags = p->item->object.flags;
- /* if commit p has already been weighted, continue. */
if (0 <= weight(p))
continue;
- /*
- * otherwise, find the first interesting
- * already-weighted parent of p and store as q.
- */
- for (q = p->item->parents; q; q = q->next) {
- if (!(q->item->object.flags & UNINTERESTING) && 0 <= weight(q)) {
- break;
- } else if (first_parent_only) {
- q = NULL;
+ for (q = p->item->parents;
+ q;
+ q = first_parent_only ? NULL : q->next) {
+ if (q->item->object.flags & UNINTERESTING)
+ continue;
+ if (0 <= weight(q))
break;
- }
}
if (!q)
continue;
|
On the Git mailing list, Aaron Lipman wrote (reply to this): |
|
On the Git mailing list, Aaron Lipman wrote (reply to this): |
|
On the Git mailing list, Aaron Lipman wrote (reply to this): |
|
On the Git mailing list, Aaron Lipman wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
This branch is now known as |
|
This patch series was integrated into seen via git@99b937c. |
|
This patch series was integrated into seen via git@2356be2. |
|
This patch series was integrated into seen via git@2a87a35. |
|
This patch series was integrated into seen via git@852feb8. |
|
This patch series was integrated into seen via git@a008ded. |
|
On the Git mailing list, Aaron Lipman wrote (reply to this): |
|
On the Git mailing list, Aaron Lipman wrote (reply to this): |
|
On the Git mailing list, Aaron Lipman wrote (reply to this): |
|
This patch series was integrated into seen via git@f2c8345. |
|
This patch series was integrated into seen via git@efee6c0. |
|
On the Git mailing list, Aaron Lipman wrote (reply to this): |
|
On the Git mailing list, Aaron Lipman wrote (reply to this): |
|
On the Git mailing list, Aaron Lipman wrote (reply to this): |
|
On the Git mailing list, Aaron Lipman wrote (reply to this): |
|
On the Git mailing list, Aaron Lipman wrote (reply to this): |
|
On the Git mailing list, Aaron Lipman wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On the Git mailing list, Eric Sunshine wrote (reply to this): |
|
On the Git mailing list, Christian Couder wrote (reply to this): |
|
On the Git mailing list, Christian Couder wrote (reply to this): |
|
On the Git mailing list, Martin Ågren wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On the Git mailing list, Eric Sunshine wrote (reply to this): |
|
On the Git mailing list, Aaron Lipman wrote (reply to this): |
|
On the Git mailing list, Aaron Lipman wrote (reply to this): |
|
On the Git mailing list, Aaron Lipman wrote (reply to this): |
|
On the Git mailing list, Aaron Lipman wrote (reply to this): |
|
On the Git mailing list, Aaron Lipman wrote (reply to this): |
|
On the Git mailing list, Aaron Lipman wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On the Git mailing list, Eric Sunshine wrote (reply to this): |
|
This was actually merged via b9ccc5e, and has been released as part of v2.29.0. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I've always wished git bisect had a first-parent flag, e.g. "git bisect start --first-parent"
This option would be particularly useful in avoiding false positives when bisecting, if a merged branch contained broken or non-buildable commits, but the merge itself was OK.
There have been a couple attempts/iterations towards this functionality in the past two years or so, and I'd like to get this across the finish line.
The previous iterations have focussed on the preliminary step of editing functions in bisect.c to accept a first_parent_only parameter, enabling the --bisect and --first-parent flags to be used in conjunction with git rev-list. In addition to updating git rev-list, I intend to enable the --first-parent flag on git bisect as well.
I've taken some of the feedback from previous iterations into account, specifically tidying up the code that assigns weights to commits in do_find_bisection(), utilizing existing commit graphs when running tests, and clarifying the test for "git rev-list --bisect-all --first-parent"
Previous two iterations (most recent first):
https://lore.kernel.org/git/[email protected]/
https://lore.kernel.org/git/[email protected]/
Related discussion about combining multiple boolean params passed to find_bisection() into a single unsigned integer:
https://lore.kernel.org/git/[email protected]/