Re: [PATCH] Create '--merges-only' option for 'git bisect'
Hi Tiago, On Mon, 30 Apr 2018, Tiago Botelho wrote: > On Mon, Apr 30, 2018 at 12:31 PM, Johannes Schindelin > wrote: > > > >> > On Thu, 12 Apr 2018, Tiago Botelho wrote: > >> > > >> >> On Thu, Apr 12, 2018 at 9:58 AM, Christian Couder > >> >> wrote: > >> >> > On Thu, Apr 12, 2018 at 9:49 AM, Harald Nordgren > >> >> > wrote: > >> >> >> I think it looks similar. But if I'm reading that thread correctly > >> >> >> then there was never a patch created, right? > >> >> > > >> >> > (It is customary on this mailing list to reply after the sentences we > >> >> > reply to. We don't "top post".) > >> >> > > >> >> > On the GSoC idea pages (like https://git.github.io/SoC-2018-Ideas/) we > >> >> > have been suggesting "Implement git bisect --first-parent" and there > >> >> > are the following related links: > >> >> > > >> >> > https://public-inbox.org/git/2015030405.ga9...@peff.net/ > >> >> > https://public-inbox.org/git/4d3cddf9.6080...@intel.com/ > >> >> > > >> >> > Tiago in Cc also tried at a recent London hackathon to implement it > >> >> > and came up with the following: > >> >> > > >> >> > https://github.com/tiagonbotelho/git/pull/1/files > >> >> > > >> >> > I tried to help him by reworking his commit in the following branch: > >> >> > > >> >> > https://github.com/chriscool/git/commits/myfirstparent > >> >> > >> >> Thank you for the cc Christian, I’ve been quite busy and was not able > >> >> to work on the PR for quite some time. > >> >> > >> >> I intended to pick it back up again next week. If it is ok with > >> >> Harald I would love to finish the PR that I started, since it is > >> >> quite close to being finished (I think it was just specs missing if I > >> >> am not mistaken). > > > > It is now well after "next week". Are there any news? Or could you unblock > > Harald by stating that you won't come back to it any time soon (in > > particular since the PR is not quite as finished from my reading as you > > made it sound...)? > > > > Ciao, > > Johannes > > I've been working on the feature for the past week > https://github.com/tiagonbotelho/git/commit/709e2e248ebfb1deab12fd7d3da4611002dfaf86#diff-118df990fd68a0929bca5441fea06fc7 > > I have some comments sent by Christian I plan on fixing this week Okay, great! I was concerned because of the long silence. Ciao, Johannes
Re: [PATCH] Create '--merges-only' option for 'git bisect'
On Mon, Apr 30, 2018 at 12:31 PM, Johannes Schindelin wrote: > Hi Tiago, > >> > On Thu, 12 Apr 2018, Tiago Botelho wrote: >> > >> >> On Thu, Apr 12, 2018 at 9:58 AM, Christian Couder >> >> wrote: >> >> > On Thu, Apr 12, 2018 at 9:49 AM, Harald Nordgren >> >> > wrote: >> >> >> I think it looks similar. But if I'm reading that thread correctly >> >> >> then there was never a patch created, right? >> >> > >> >> > (It is customary on this mailing list to reply after the sentences we >> >> > reply to. We don't "top post".) >> >> > >> >> > On the GSoC idea pages (like https://git.github.io/SoC-2018-Ideas/) we >> >> > have been suggesting "Implement git bisect --first-parent" and there >> >> > are the following related links: >> >> > >> >> > https://public-inbox.org/git/2015030405.ga9...@peff.net/ >> >> > https://public-inbox.org/git/4d3cddf9.6080...@intel.com/ >> >> > >> >> > Tiago in Cc also tried at a recent London hackathon to implement it >> >> > and came up with the following: >> >> > >> >> > https://github.com/tiagonbotelho/git/pull/1/files >> >> > >> >> > I tried to help him by reworking his commit in the following branch: >> >> > >> >> > https://github.com/chriscool/git/commits/myfirstparent >> >> >> >> Thank you for the cc Christian, I’ve been quite busy and was not able >> >> to work on the PR for quite some time. >> >> >> >> I intended to pick it back up again next week. If it is ok with >> >> Harald I would love to finish the PR that I started, since it is >> >> quite close to being finished (I think it was just specs missing if I >> >> am not mistaken). > > It is now well after "next week". Are there any news? Or could you unblock > Harald by stating that you won't come back to it any time soon (in > particular since the PR is not quite as finished from my reading as you > made it sound...)? > > Ciao, > Johannes I've been working on the feature for the past week https://github.com/tiagonbotelho/git/commit/709e2e248ebfb1deab12fd7d3da4611002dfaf86#diff-118df990fd68a0929bca5441fea06fc7 I have some comments sent by Christian I plan on fixing this week Thank you, -- Tiago Botelho
Re: [PATCH] Create '--merges-only' option for 'git bisect'
Hi Tiago, > > On Thu, 12 Apr 2018, Tiago Botelho wrote: > > > >> On Thu, Apr 12, 2018 at 9:58 AM, Christian Couder > >> wrote: > >> > On Thu, Apr 12, 2018 at 9:49 AM, Harald Nordgren > >> > wrote: > >> >> I think it looks similar. But if I'm reading that thread correctly > >> >> then there was never a patch created, right? > >> > > >> > (It is customary on this mailing list to reply after the sentences we > >> > reply to. We don't "top post".) > >> > > >> > On the GSoC idea pages (like https://git.github.io/SoC-2018-Ideas/) we > >> > have been suggesting "Implement git bisect --first-parent" and there > >> > are the following related links: > >> > > >> > https://public-inbox.org/git/2015030405.ga9...@peff.net/ > >> > https://public-inbox.org/git/4d3cddf9.6080...@intel.com/ > >> > > >> > Tiago in Cc also tried at a recent London hackathon to implement it > >> > and came up with the following: > >> > > >> > https://github.com/tiagonbotelho/git/pull/1/files > >> > > >> > I tried to help him by reworking his commit in the following branch: > >> > > >> > https://github.com/chriscool/git/commits/myfirstparent > >> > >> Thank you for the cc Christian, I’ve been quite busy and was not able > >> to work on the PR for quite some time. > >> > >> I intended to pick it back up again next week. If it is ok with > >> Harald I would love to finish the PR that I started, since it is > >> quite close to being finished (I think it was just specs missing if I > >> am not mistaken). It is now well after "next week". Are there any news? Or could you unblock Harald by stating that you won't come back to it any time soon (in particular since the PR is not quite as finished from my reading as you made it sound...)? Ciao, Johannes
Re: [PATCH] Create '--merges-only' option for 'git bisect'
On Thu, Apr 12, 2018 at 12:53 PM, Johannes Schindelin wrote: > Hi Tiago, > > On Thu, 12 Apr 2018, Tiago Botelho wrote: > >> On Thu, Apr 12, 2018 at 9:58 AM, Christian Couder >> wrote: >> > On Thu, Apr 12, 2018 at 9:49 AM, Harald Nordgren >> > wrote: >> >> I think it looks similar. But if I'm reading that thread correctly >> >> then there was never a patch created, right? >> > >> > (It is customary on this mailing list to reply after the sentences we >> > reply to. We don't "top post".) >> > >> > On the GSoC idea pages (like https://git.github.io/SoC-2018-Ideas/) we >> > have been suggesting "Implement git bisect --first-parent" and there >> > are the following related links: >> > >> > https://public-inbox.org/git/2015030405.ga9...@peff.net/ >> > https://public-inbox.org/git/4d3cddf9.6080...@intel.com/ >> > >> > Tiago in Cc also tried at a recent London hackathon to implement it >> > and came up with the following: >> > >> > https://github.com/tiagonbotelho/git/pull/1/files >> > >> > I tried to help him by reworking his commit in the following branch: >> > >> > https://github.com/chriscool/git/commits/myfirstparent >> >> Thank you for the cc Christian, I’ve been quite busy and was not able >> to work on the PR for quite some time. >> >> I intended to pick it back up again next week. If it is ok with Harald >> I would love to finish the PR that I started, >> since it is quite close to being finished (I think it was just specs >> missing if I am not mistaken). > > That looks promising. Just like I suggested to Harald in another reply > [*1*] on this thread, you probably want to use `int flags` instead, and > turn `find_all` into BISECT_FIND_ALL in a preparatory commit. > > Also, you will definitely want to add a test. Again, my reply to Harald > [*1*] should give you a head start there... You will want to imitate the > test case I outlined there, maybe something like: > > # A - B - C - F > # \ \ / \ > # D - E - G - H > > [... 'setup' as in my mail to Harald ...] > > test_expect_success '--first-parent' ' > write_script find-e.sh <<-\EOF && > case "$(git show --format=%s -s)" in > B|C|F) ;; # first parent lineage: okay > *) git show -s --oneline HEAD >unexpected;; > esac > # detect whether we are "before" or "after" E > test ! -f E.t > EOF > > git bisect start --first-parent HEAD A && > git bisect run ./find-e.sh >actual && > test_path_is_missing unexpected && > grep "$(git rev-parse F) is the first bad commit" actual > ' > > Also, Tiago, reading through your patch (as on chriscool/git; do you have > your own fork? That would make it much easier to collaborate with you by > offering PRs), it looks more straight-forward than editing the commit_list > after the fact and adding magic weights ;-) > > Except for one thing. I wonder why `bisect_next_all()` does not set > revs.first_parent_only after calling `bisect_rev_setup()`? You would still > need the changes in `count_distance()`, as it performs its own commit > graph traversal, but there is no need to enumerate too many commits in the > first place, right? > > Harald, maybe --merges-only can be implemented on top of --first-parent, > with the `int flags` change I suggested? > > Ciao, > Johannes > > Footnote *1*: > https://public-inbox.org/git/nycvar.qro.7.76.6.1804121143120...@zvavag-6oxh6da.rhebcr.pbec.zvpebfbsg.pbz/ Hi Johannes, Thank you for the pointers! I do have my own fork, you can see it here: https://github.com/tiagonbotelho/git/pull/1/commits which applies the changes Chris made to my first draft of the solution. I still have to add him as co-author of that commit. Cheers, Tiago Botelho
Re: [PATCH] Create '--merges-only' option for 'git bisect'
Hi Tiago, On Thu, 12 Apr 2018, Tiago Botelho wrote: > On Thu, Apr 12, 2018 at 9:58 AM, Christian Couder > wrote: > > On Thu, Apr 12, 2018 at 9:49 AM, Harald Nordgren > > wrote: > >> I think it looks similar. But if I'm reading that thread correctly > >> then there was never a patch created, right? > > > > (It is customary on this mailing list to reply after the sentences we > > reply to. We don't "top post".) > > > > On the GSoC idea pages (like https://git.github.io/SoC-2018-Ideas/) we > > have been suggesting "Implement git bisect --first-parent" and there > > are the following related links: > > > > https://public-inbox.org/git/2015030405.ga9...@peff.net/ > > https://public-inbox.org/git/4d3cddf9.6080...@intel.com/ > > > > Tiago in Cc also tried at a recent London hackathon to implement it > > and came up with the following: > > > > https://github.com/tiagonbotelho/git/pull/1/files > > > > I tried to help him by reworking his commit in the following branch: > > > > https://github.com/chriscool/git/commits/myfirstparent > > Thank you for the cc Christian, I’ve been quite busy and was not able > to work on the PR for quite some time. > > I intended to pick it back up again next week. If it is ok with Harald > I would love to finish the PR that I started, > since it is quite close to being finished (I think it was just specs > missing if I am not mistaken). That looks promising. Just like I suggested to Harald in another reply [*1*] on this thread, you probably want to use `int flags` instead, and turn `find_all` into BISECT_FIND_ALL in a preparatory commit. Also, you will definitely want to add a test. Again, my reply to Harald [*1*] should give you a head start there... You will want to imitate the test case I outlined there, maybe something like: # A - B - C - F # \ \ / \ # D - E - G - H [... 'setup' as in my mail to Harald ...] test_expect_success '--first-parent' ' write_script find-e.sh <<-\EOF && case "$(git show --format=%s -s)" in B|C|F) ;; # first parent lineage: okay *) git show -s --oneline HEAD >unexpected;; esac # detect whether we are "before" or "after" E test ! -f E.t EOF git bisect start --first-parent HEAD A && git bisect run ./find-e.sh >actual && test_path_is_missing unexpected && grep "$(git rev-parse F) is the first bad commit" actual ' Also, Tiago, reading through your patch (as on chriscool/git; do you have your own fork? That would make it much easier to collaborate with you by offering PRs), it looks more straight-forward than editing the commit_list after the fact and adding magic weights ;-) Except for one thing. I wonder why `bisect_next_all()` does not set revs.first_parent_only after calling `bisect_rev_setup()`? You would still need the changes in `count_distance()`, as it performs its own commit graph traversal, but there is no need to enumerate too many commits in the first place, right? Harald, maybe --merges-only can be implemented on top of --first-parent, with the `int flags` change I suggested? Ciao, Johannes Footnote *1*: https://public-inbox.org/git/nycvar.qro.7.76.6.1804121143120...@zvavag-6oxh6da.rhebcr.pbec.zvpebfbsg.pbz/
Re: [PATCH] Create '--merges-only' option for 'git bisect'
Hi Stefan, On Wed, 11 Apr 2018, Stefan Beller wrote: > On Wed, Apr 11, 2018 at 3:55 PM, Harald Nordgren > wrote: > > When ran with '--merges-only', git bisect will only look at merge commits > > -- commits with 2 or more parents or the initial commit. > > There has been quite some talk on the mailing list, e.g. > https://public-inbox.org/git/20160427204551.GB4613@virgo.localdomain/ > which suggests a --first-parent mode instead. I like that mode, but I would love to have *both*. And from what I see, it should be relatively easy to add the --first-parent mode on top of Harald's patches. > For certain histories these are the same, but merges-only is more > restrictive for back-and-forth-cross merges. You mean merges-only tests *more* in back-and-forth-cross-merges scenarios? Ciao, Dscho
Re: [PATCH] Create '--merges-only' option for 'git bisect'
Hi Harald,
Thank you for working on this.
On Thu, 12 Apr 2018, Harald Nordgren wrote:
> When ran with '--merges-only', git bisect will only look at merge
> commits -- commits with 2 or more parents or the initial commit.
This is an excellent idea!
> Proof of concept of a feature that I have wanted in Git for a while.
> In my daily work my company uses GitHub, which creates lots of merge
> commits. In general, tests are only ran on the tip of a branch
> before merging, so the different commits within a merge commit are
> allowed not to be buildable. Therefore 'git bisect' often doesn't
> work since it will give lots of false positives for anything that is
> not a merge commit. If we could have a feature to only bisect merge
> commits then it would be easier to pinpoint which merge causes any
> particular issue. After that, a bisect could be done within the
> merge to pinpoint futher. As a follow-up to this patch -- we could
> potentially create a feature that automatically continues into
> regular bisect within the bad merge commit after completed
> '--merges-only' bisection.
It also helps when bisecting breakages in `pu` (mainly because the
branches in `pu` use branch points that are often insanely far in the
past).
> diff --git a/bisect.c b/bisect.c
> index a579b5088..e8a2f77c7 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -254,7 +254,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
> only_merge_commits)
How about `int flags`, and defining FIND_ALL and ONLY_MERGE_COMMITS?
That way, it will be easy to add ONLY_FIRST_PARENTS without changing the
signature of do_find_bisection().
Ideally, a preparatory commit would change find_all -> flags (adding
FIND_ALL), and the second commit would then add support for
ONLY_MERGE_COMMITS.
> @@ -266,6 +266,13 @@ static struct commit_list *do_find_bisection(struct
> commit_list *list,
> unsigned flags = commit->object.flags;
>
> p->item->util = &weights[n++];
> +
> + if (only_merge_commits) {
> + weight_set(p, -2);
> + counted++;
> + continue;
> + }
> +
This hunk is a little hard to understand when you come from elsewhere, as
I do. Could you maybe explain a little bit what the `weight_set(p, -2)`
does? This is probably good material for enhancing the commit message
(seeing as we understand the commit message to be kind of the "convince me
that this is a good change, and explain things that are not immediately
obvious from the diff" document).
Maybe it would be enough to describe the role of the weight, and what the
typical values look like.
> @@ -305,11 +312,17 @@ static struct commit_list *do_find_bisection(struct
> commit_list *list,
>* way, and then fill the blanks using cheaper algorithm.
>*/
> for (p = list; p; p = p->next) {
> + int distance;
> if (p->item->object.flags & UNINTERESTING)
> continue;
> if (weight(p) != -2)
> continue;
> - weight_set(p, count_distance(p));
> +
> + if (only_merge_commits)
> + distance = count_distance(p) - 1;
> + else
> + distance = count_distance(p);
> + weight_set(p, distance);
A shorter way:
weight_set(p, count_distance(p) - !!only_merge_commits);
Could you add a code comment above this line to explain why this is
needed? (I have to admit that I have no clue what the weights or the
distances are in this context, but I think that a 3-line explanation could
probably give me enough of an idea that I do not have to study an hour or
three to learn enough to understand this change.)
> clear_distance(list);
>
> /* Does it happen to be at exactly half-way? */
> @@ -330,11 +343,17 @@ static struct commit_list *do_find_bisection(struct
> commit_list *list,
> for (q = p->item->parents; q; q = q->next) {
> if (q->item->object.flags & UNINTERESTING)
> continue;
> + if (!q->item->util)
> + break;
So we use the `util` field now to do things, probably to flag some
property of the commit.
Maybe the commit message could prepare the reader for this, with a
paragraph starting with "We use the commits' `util` field to store the
information that [...]"?
Seeing as this loop iterates over the commit's parents, I guess we are
looking at merge commits, and drop out early if we find
Re: [PATCH] Create '--merges-only' option for 'git bisect'
On Thu, Apr 12, 2018 at 9:58 AM, Christian Couder wrote: > On Thu, Apr 12, 2018 at 9:49 AM, Harald Nordgren > wrote: >> I think it looks similar. But if I'm reading that thread correctly >> then there was never a patch created, right? > > (It is customary on this mailing list to reply after the sentences we > reply to. We don't "top post".) > > On the GSoC idea pages (like https://git.github.io/SoC-2018-Ideas/) we > have been suggesting "Implement git bisect --first-parent" and there > are the following related links: > > https://public-inbox.org/git/2015030405.ga9...@peff.net/ > https://public-inbox.org/git/4d3cddf9.6080...@intel.com/ > > Tiago in Cc also tried at a recent London hackathon to implement it > and came up with the following: > > https://github.com/tiagonbotelho/git/pull/1/files > > I tried to help him by reworking his commit in the following branch: > > https://github.com/chriscool/git/commits/myfirstparent Thank you for the cc Christian, I’ve been quite busy and was not able to work on the PR for quite some time. I intended to pick it back up again next week. If it is ok with Harald I would love to finish the PR that I started, since it is quite close to being finished (I think it was just specs missing if I am not mistaken). Kind regards, Tiago Botelho
Re: [PATCH] Create '--merges-only' option for 'git bisect'
On Thu, Apr 12, 2018 at 9:49 AM, Harald Nordgren wrote: > I think it looks similar. But if I'm reading that thread correctly > then there was never a patch created, right? (It is customary on this mailing list to reply after the sentences we reply to. We don't "top post".) On the GSoC idea pages (like https://git.github.io/SoC-2018-Ideas/) we have been suggesting "Implement git bisect --first-parent" and there are the following related links: https://public-inbox.org/git/2015030405.ga9...@peff.net/ https://public-inbox.org/git/4d3cddf9.6080...@intel.com/ Tiago in Cc also tried at a recent London hackathon to implement it and came up with the following: https://github.com/tiagonbotelho/git/pull/1/files I tried to help him by reworking his commit in the following branch: https://github.com/chriscool/git/commits/myfirstparent
Re: [PATCH] Create '--merges-only' option for 'git bisect'
I think it looks similar. But if I'm reading that thread correctly then there was never a patch created, right? On Thu, Apr 12, 2018 at 1:33 AM, Stefan Beller wrote: > On Wed, Apr 11, 2018 at 3:55 PM, Harald Nordgren > wrote: >> When ran with '--merges-only', git bisect will only look at merge commits -- >> commits with 2 or more parents or the initial commit. > > There has been quite some talk on the mailing list, e.g. > https://public-inbox.org/git/20160427204551.GB4613@virgo.localdomain/ > which suggests a --first-parent mode instead. For certain histories > these are the same, > but merges-only is more restrictive for back-and-forth-cross merges. > > > >> >> Signed-off-by: Harald Nordgren >> --- >> >> Notes: >> Proof of concept of a feature that I have wanted in Git for a while. In >> my daily work my company uses GitHub, which creates lots of merge commits. >> In general, tests are only ran on the tip of a branch before merging, so the >> different commits within a merge commit are allowed not to be buildable. >> Therefore 'git bisect' often doesn't work since it will give lots of false >> positives for anything that is not a merge commit. If we could have a >> feature to only bisect merge commits then it would be easier to pinpoint >> which merge causes any particular issue. After that, a bisect could be done >> within the merge to pinpoint futher. As a follow-up to this patch -- we >> could potentially create a feature that automatically continues into regular >> bisect within the bad merge commit after completed '--merges-only' bisection. > > The github workflow you mention sounds as if --first-parent would do, too?
Re: [PATCH] Create '--merges-only' option for 'git bisect'
On Wed, Apr 11, 2018 at 3:55 PM, Harald Nordgren wrote: > When ran with '--merges-only', git bisect will only look at merge commits -- > commits with 2 or more parents or the initial commit. There has been quite some talk on the mailing list, e.g. https://public-inbox.org/git/20160427204551.GB4613@virgo.localdomain/ which suggests a --first-parent mode instead. For certain histories these are the same, but merges-only is more restrictive for back-and-forth-cross merges. > > Signed-off-by: Harald Nordgren > --- > > Notes: > Proof of concept of a feature that I have wanted in Git for a while. In > my daily work my company uses GitHub, which creates lots of merge commits. In > general, tests are only ran on the tip of a branch before merging, so the > different commits within a merge commit are allowed not to be buildable. > Therefore 'git bisect' often doesn't work since it will give lots of false > positives for anything that is not a merge commit. If we could have a feature > to only bisect merge commits then it would be easier to pinpoint which merge > causes any particular issue. After that, a bisect could be done within the > merge to pinpoint futher. As a follow-up to this patch -- we could > potentially create a feature that automatically continues into regular bisect > within the bad merge commit after completed '--merges-only' bisection. The github workflow you mention sounds as if --first-parent would do, too?
