* [PATCH] Create '--merges-only' option for 'git bisect' @ 2018-04-11 22:55 Harald Nordgren 2018-04-11 23:33 ` Stefan Beller 2018-04-12 11:33 ` Johannes Schindelin 0 siblings, 2 replies; 12+ messages in thread From: Harald Nordgren @ 2018-04-11 22:55 UTC (permalink / raw) To: git; +Cc: Harald Nordgren When ran with '--merges-only', git bisect will only look at merge commits -- commits with 2 or more parents or the initial commit. Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com> --- 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. bisect.c | 87 ++++++++++++++++++++++++++---- bisect.h | 4 +- builtin/bisect--helper.c | 28 +++++++++- builtin/rev-list.c | 2 +- git-bisect.sh | 5 ++ t/t6070-bisect-merge-commits.sh | 116 ++++++++++++++++++++++++++++++++++++++++ 6 files changed, 228 insertions(+), 14 deletions(-) create mode 100755 t/t6070-bisect-merge-commits.sh 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) { int n, counted; struct commit_list *p; @@ -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; + } + switch (count_interesting_parents(commit)) { case 0: if (!(flags & TREESAME)) { @@ -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); 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; if (0 <= weight(q)) break; } if (!q) continue; + if (!q->item->util) { + counted++; + continue; + } /* * weight for p is unknown but q is known. @@ -364,8 +383,43 @@ static struct commit_list *do_find_bisection(struct commit_list *list, return best_bisection_sorted(list, nr); } +int merge_commit_or_root(const struct commit c) +{ + if (!c.parents) + return 1; + + return !!c.parents->next; +} + +void filter_non_merge_commits(struct commit_list **commit_list) +{ + struct commit_list *list1 = *commit_list; + struct commit_list *list2 = NULL; + *commit_list = NULL; + + for ( ; list1; list1 = list1->next) { + if (merge_commit_or_root(*list1->item)) { + list2 = list1; + list1 = list1->next; + list2->next = NULL; + list2->item->parents = NULL; + *commit_list = list2; + break; + } + } + + for ( ; list1; list1 = list1->next) { + list2->next = NULL; + if (merge_commit_or_root(*list1->item)) { + list2->next = list1; + list2 = list2->next; + list2->item->parents = list1; + } + } +} + void find_bisection(struct commit_list **commit_list, int *reaches, - int *all, int find_all) + int *all, int find_all, int only_merge_commits) { int nr, on_list; struct commit_list *list, *p, *best, *next, *last; @@ -373,6 +427,10 @@ void find_bisection(struct commit_list **commit_list, int *reaches, show_list("bisection 2 entry", 0, 0, *commit_list); + if (only_merge_commits) { + filter_non_merge_commits(commit_list); + } + /* * Count the number of total and tree-changing items on the * list, while reversing the list. @@ -400,7 +458,7 @@ void find_bisection(struct commit_list **commit_list, int *reaches, weights = xcalloc(on_list, sizeof(*weights)); /* Do the real work of finding bisection commit. */ - best = do_find_bisection(list, nr, weights, find_all); + best = do_find_bisection(list, nr, weights, find_all, only_merge_commits); if (best) { if (!find_all) { list->item = best->item; @@ -878,7 +936,7 @@ static void check_good_are_ancestors_of_bad(const char *prefix, int no_checkout) /* * This does "git diff-tree --pretty COMMIT" without one fork+exec. */ -static void show_diff_tree(const char *prefix, struct commit *commit) +static void show_diff_tree(const char *prefix, struct commit *commit, int only_merge_commits) { struct rev_info opt; @@ -893,6 +951,11 @@ static void show_diff_tree(const char *prefix, struct commit *commit) opt.use_terminator = 0; opt.commit_format = CMIT_FMT_DEFAULT; + if (only_merge_commits) { + opt.ignore_merges = 0; + opt.combine_merges = 1; + } + /* diff-tree init */ if (!opt.diffopt.output_format) opt.diffopt.output_format = DIFF_FORMAT_RAW; @@ -938,7 +1001,7 @@ void read_bisect_terms(const char **read_bad, const char **read_good) * If no_checkout is non-zero, the bisection process does not * checkout the trial commit but instead simply updates BISECT_HEAD. */ -int bisect_next_all(const char *prefix, int no_checkout) +int bisect_next_all(const char *prefix, int no_checkout, int only_merge_commits) { struct rev_info revs; struct commit_list *tried; @@ -957,7 +1020,7 @@ int bisect_next_all(const char *prefix, int no_checkout) bisect_common(&revs); - find_bisection(&revs.commits, &reaches, &all, !!skipped_revs.nr); + find_bisection(&revs.commits, &reaches, &all, !!skipped_revs.nr, only_merge_commits); revs.commits = managed_skipped(revs.commits, &tried); if (!revs.commits) { @@ -983,10 +1046,14 @@ int bisect_next_all(const char *prefix, int no_checkout) bisect_rev = &revs.commits->item->object.oid; if (!oidcmp(bisect_rev, current_bad_oid)) { + char *format_string = NULL; exit_if_skipped_commits(tried, current_bad_oid); - printf("%s is the first %s commit\n", oid_to_hex(bisect_rev), - term_bad); - show_diff_tree(prefix, revs.commits->item); + if (only_merge_commits) + format_string = "%s is the first %s merge\n"; + else + format_string = "%s is the first %s commit\n"; + printf(format_string, oid_to_hex(bisect_rev), term_bad); + show_diff_tree(prefix, revs.commits->item, only_merge_commits); /* This means the bisection process succeeded. */ exit(10); } diff --git a/bisect.h b/bisect.h index a5d9248a4..664ada180 100644 --- a/bisect.h +++ b/bisect.h @@ -9,7 +9,7 @@ * best commit, as chosen by `find_all`. */ extern void find_bisection(struct commit_list **list, int *reaches, int *all, - int find_all); + int find_all, int only_merge_commits); extern struct commit_list *filter_skipped(struct commit_list *list, struct commit_list **tried, @@ -28,7 +28,7 @@ struct rev_list_info { const char *header_prefix; }; -extern int bisect_next_all(const char *prefix, int no_checkout); +extern int bisect_next_all(const char *prefix, int no_checkout, int only_merge_commits); extern int estimate_bisect_steps(int all); diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 4b5fadcbe..9d7a1dd23 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -106,6 +106,32 @@ static void check_expected_revs(const char **revs, int rev_nr) } } +static GIT_PATH_FUNC(git_path_bisect_merges_only, "MERGES_ONLY_BISECT") + +static int merges_only(void) +{ + const char *filename = git_path_bisect_merges_only(); + struct stat st; + struct strbuf str = STRBUF_INIT; + FILE *fp; + int res = 0; + + if (stat(filename, &st) || !S_ISREG(st.st_mode)) + return 0; + + fp = fopen_or_warn(filename, "r"); + if (!fp) + return 0; + + if (strbuf_getline_lf(&str, fp) != EOF) + res = atoi(str.buf); + + strbuf_release(&str); + fclose(fp); + + return res; +} + int cmd_bisect__helper(int argc, const char **argv, const char *prefix) { enum { @@ -137,7 +163,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) switch (cmdmode) { case NEXT_ALL: - return bisect_next_all(prefix, no_checkout); + return bisect_next_all(prefix, no_checkout, merges_only()); case WRITE_TERMS: if (argc != 2) return error(_("--write-terms requires two arguments")); diff --git a/builtin/rev-list.c b/builtin/rev-list.c index fadd3ec14..cacffe6c6 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -538,7 +538,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) if (bisect_list) { int reaches, all; - find_bisection(&revs.commits, &reaches, &all, bisect_find_all); + find_bisection(&revs.commits, &reaches, &all, bisect_find_all, 0); if (bisect_show_vars) return show_bisect_vars(&info, reaches, all); diff --git a/git-bisect.sh b/git-bisect.sh index 54cbfecc5..730c983c5 100755 --- a/git-bisect.sh +++ b/git-bisect.sh @@ -82,6 +82,7 @@ bisect_start() { bad_seen=0 eval='' must_write_terms=0 + merges_only=0 revs='' if test "z$(git rev-parse --is-bare-repository)" != zfalse then @@ -96,6 +97,9 @@ bisect_start() { shift break ;; + --merges-only) + merges_only=1 + shift ;; --no-checkout) mode=--no-checkout shift ;; @@ -212,6 +216,7 @@ bisect_start() { git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD" || exit fi && echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit + echo "$merges_only" >"$GIT_DIR/MERGES_ONLY_BISECT" || exit # # Check if we can proceed to the next bisect state. # diff --git a/t/t6070-bisect-merge-commits.sh b/t/t6070-bisect-merge-commits.sh new file mode 100755 index 000000000..edfae0cfd --- /dev/null +++ b/t/t6070-bisect-merge-commits.sh @@ -0,0 +1,116 @@ +#!/bin/sh +# +# Copyright (c) 2018 Harald Nordgren +# +test_description='Tests git bisect merge commit functionality' + +exec </dev/null + +. ./test-lib.sh + +add_line_into_file() +{ + _line=$1 + _file=$2 + + if [ -f "$_file" ]; then + echo "$_line" >> $_file || return $? + MSG="Add <$_line> into <$_file>." + else + echo "$_line" > $_file || return $? + git add $_file || return $? + MSG="Create file <$_file> with <$_line> inside." + fi + + test_tick + git commit --quiet -m "$MSG" $_file +} + +create_merge_commit() +{ + _branch=$1 + _file=$2 + + git checkout -b $_branch && + add_line_into_file "hello" $_file && + git checkout master && + git merge $_branch --no-edit --no-ff +} + +HASH1= +HASH2= +HASH3= +HASH4= +HASH5= +HASH6= + +test_expect_success 'set up basic repo with 3 files and 3 merge commits' ' + add_line_into_file "hello" hello && + HASH1=$(git rev-parse --verify HEAD) && + + add_line_into_file "hello" hello && + git checkout -b branch1 && + add_line_into_file "hello" hello1 && + git checkout master && + git checkout -b branch2 && + add_line_into_file "hello" hello2 && + git checkout master && + git checkout -b branch3 && + add_line_into_file "hello" hello3 && + git checkout master && + git merge branch1 --no-edit --no-ff && + HASH2=$(git rev-parse --verify HEAD) && + + add_line_into_file "hello" hello && + add_line_into_file "hello" hello && + git merge branch2 --no-edit --no-ff && + git merge branch3 --no-edit --no-ff && + HASH3=$(git rev-parse --verify HEAD) && + + add_line_into_file "hello" hello && + HASH4=$(git rev-parse --verify HEAD) && + + create_merge_commit branch4 hello4 && + create_merge_commit branch5 hello5 && + create_merge_commit branch6 hello6 && + create_merge_commit branch7 hello7 && + create_merge_commit branch8 hello8 && + create_merge_commit branch9 hello9 && + create_merge_commit branch10 hello10 && + create_merge_commit branch11 hello11 && + create_merge_commit branch12 hello12 && + create_merge_commit branch13 hello13 && + create_merge_commit branch14 hello14 && + create_merge_commit branch15 hello15 && + create_merge_commit branch16 hello16 && + HASH5=$(git rev-parse --verify HEAD) && + + create_merge_commit branch17 hello17 && + create_merge_commit branch18 hello18 && + create_merge_commit branch19 hello19 && + create_merge_commit branch20 hello20 && + HASH6=$(git rev-parse --verify HEAD) +' + +test_expect_success 'bisect skip: successful result' ' + test_when_finished git bisect reset && + git bisect reset && + git bisect start $HASH4 $HASH1 --merges-only && + git bisect good && + git bisect good && + git bisect bad > my_bisect_log.txt && + grep "$HASH3 is the first bad merge" my_bisect_log.txt +' + +test_expect_success 'bisect skip: successful result' ' + test_when_finished git bisect reset && + git bisect reset && + git bisect start $HASH6 $HASH2 --merges-only && + git bisect good && + git bisect good && + git bisect bad && + git bisect bad > my_bisect_log.txt && + grep "$HASH5 is the first bad merge" my_bisect_log.txt +' + +test_done -- 2.14.3 (Apple Git-98) ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] Create '--merges-only' option for 'git bisect' 2018-04-11 22:55 [PATCH] Create '--merges-only' option for 'git bisect' Harald Nordgren @ 2018-04-11 23:33 ` Stefan Beller 2018-04-12 7:49 ` Harald Nordgren 2018-04-12 11:35 ` Johannes Schindelin 2018-04-12 11:33 ` Johannes Schindelin 1 sibling, 2 replies; 12+ messages in thread From: Stefan Beller @ 2018-04-11 23:33 UTC (permalink / raw) To: Harald Nordgren; +Cc: git On Wed, Apr 11, 2018 at 3:55 PM, Harald Nordgren <haraldnordgren@gmail.com> 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 <haraldnordgren@gmail.com> > --- > > 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? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Create '--merges-only' option for 'git bisect' 2018-04-11 23:33 ` Stefan Beller @ 2018-04-12 7:49 ` Harald Nordgren 2018-04-12 8:58 ` Christian Couder 2018-04-12 11:35 ` Johannes Schindelin 1 sibling, 1 reply; 12+ messages in thread From: Harald Nordgren @ 2018-04-12 7:49 UTC (permalink / raw) To: Stefan Beller; +Cc: git 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 <sbeller@google.com> wrote: > On Wed, Apr 11, 2018 at 3:55 PM, Harald Nordgren > <haraldnordgren@gmail.com> 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 <haraldnordgren@gmail.com> >> --- >> >> 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? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Create '--merges-only' option for 'git bisect' 2018-04-12 7:49 ` Harald Nordgren @ 2018-04-12 8:58 ` Christian Couder 2018-04-12 9:27 ` Tiago Botelho 0 siblings, 1 reply; 12+ messages in thread From: Christian Couder @ 2018-04-12 8:58 UTC (permalink / raw) To: Harald Nordgren; +Cc: Stefan Beller, git, Tiago Botelho On Thu, Apr 12, 2018 at 9:49 AM, Harald Nordgren <haraldnordgren@gmail.com> 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/20150304053333.GA9584@peff.net/ https://public-inbox.org/git/4D3CDDF9.6080405@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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Create '--merges-only' option for 'git bisect' 2018-04-12 8:58 ` Christian Couder @ 2018-04-12 9:27 ` Tiago Botelho 2018-04-12 11:53 ` Johannes Schindelin 0 siblings, 1 reply; 12+ messages in thread From: Tiago Botelho @ 2018-04-12 9:27 UTC (permalink / raw) To: Christian Couder; +Cc: Harald Nordgren, Stefan Beller, git On Thu, Apr 12, 2018 at 9:58 AM, Christian Couder <christian.couder@gmail.com> wrote: > On Thu, Apr 12, 2018 at 9:49 AM, Harald Nordgren > <haraldnordgren@gmail.com> 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/20150304053333.GA9584@peff.net/ > https://public-inbox.org/git/4D3CDDF9.6080405@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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Create '--merges-only' option for 'git bisect' 2018-04-12 9:27 ` Tiago Botelho @ 2018-04-12 11:53 ` Johannes Schindelin 2018-04-12 12:19 ` Tiago Botelho 0 siblings, 1 reply; 12+ messages in thread From: Johannes Schindelin @ 2018-04-12 11:53 UTC (permalink / raw) To: Tiago Botelho; +Cc: Christian Couder, Harald Nordgren, Stefan Beller, git [-- Attachment #1: Type: text/plain, Size: 3309 bytes --] Hi Tiago, On Thu, 12 Apr 2018, Tiago Botelho wrote: > On Thu, Apr 12, 2018 at 9:58 AM, Christian Couder > <christian.couder@gmail.com> wrote: > > On Thu, Apr 12, 2018 at 9:49 AM, Harald Nordgren > > <haraldnordgren@gmail.com> 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/20150304053333.GA9584@peff.net/ > > https://public-inbox.org/git/4D3CDDF9.6080405@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.65@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz/ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Create '--merges-only' option for 'git bisect' 2018-04-12 11:53 ` Johannes Schindelin @ 2018-04-12 12:19 ` Tiago Botelho 2018-04-30 11:31 ` Johannes Schindelin 0 siblings, 1 reply; 12+ messages in thread From: Tiago Botelho @ 2018-04-12 12:19 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Christian Couder, Harald Nordgren, Stefan Beller, git On Thu, Apr 12, 2018 at 12:53 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > Hi Tiago, > > On Thu, 12 Apr 2018, Tiago Botelho wrote: > >> On Thu, Apr 12, 2018 at 9:58 AM, Christian Couder >> <christian.couder@gmail.com> wrote: >> > On Thu, Apr 12, 2018 at 9:49 AM, Harald Nordgren >> > <haraldnordgren@gmail.com> 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/20150304053333.GA9584@peff.net/ >> > https://public-inbox.org/git/4D3CDDF9.6080405@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.65@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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Create '--merges-only' option for 'git bisect' 2018-04-12 12:19 ` Tiago Botelho @ 2018-04-30 11:31 ` Johannes Schindelin 2018-04-30 11:34 ` Tiago Botelho 0 siblings, 1 reply; 12+ messages in thread From: Johannes Schindelin @ 2018-04-30 11:31 UTC (permalink / raw) To: Tiago Botelho; +Cc: Christian Couder, Harald Nordgren, Stefan Beller, git [-- Attachment #1: Type: text/plain, Size: 1843 bytes --] Hi Tiago, > > On Thu, 12 Apr 2018, Tiago Botelho wrote: > > > >> On Thu, Apr 12, 2018 at 9:58 AM, Christian Couder > >> <christian.couder@gmail.com> wrote: > >> > On Thu, Apr 12, 2018 at 9:49 AM, Harald Nordgren > >> > <haraldnordgren@gmail.com> 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/20150304053333.GA9584@peff.net/ > >> > https://public-inbox.org/git/4D3CDDF9.6080405@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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Create '--merges-only' option for 'git bisect' 2018-04-30 11:31 ` Johannes Schindelin @ 2018-04-30 11:34 ` Tiago Botelho 2018-05-01 11:31 ` Johannes Schindelin 0 siblings, 1 reply; 12+ messages in thread From: Tiago Botelho @ 2018-04-30 11:34 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Christian Couder, Harald Nordgren, Stefan Beller, git On Mon, Apr 30, 2018 at 12:31 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > Hi Tiago, > >> > On Thu, 12 Apr 2018, Tiago Botelho wrote: >> > >> >> On Thu, Apr 12, 2018 at 9:58 AM, Christian Couder >> >> <christian.couder@gmail.com> wrote: >> >> > On Thu, Apr 12, 2018 at 9:49 AM, Harald Nordgren >> >> > <haraldnordgren@gmail.com> 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/20150304053333.GA9584@peff.net/ >> >> > https://public-inbox.org/git/4D3CDDF9.6080405@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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Create '--merges-only' option for 'git bisect' 2018-04-30 11:34 ` Tiago Botelho @ 2018-05-01 11:31 ` Johannes Schindelin 0 siblings, 0 replies; 12+ messages in thread From: Johannes Schindelin @ 2018-05-01 11:31 UTC (permalink / raw) To: Tiago Botelho; +Cc: Christian Couder, Harald Nordgren, Stefan Beller, git [-- Attachment #1: Type: text/plain, Size: 2461 bytes --] Hi Tiago, On Mon, 30 Apr 2018, Tiago Botelho wrote: > On Mon, Apr 30, 2018 at 12:31 PM, Johannes Schindelin > <Johannes.Schindelin@gmx.de> wrote: > > > >> > On Thu, 12 Apr 2018, Tiago Botelho wrote: > >> > > >> >> On Thu, Apr 12, 2018 at 9:58 AM, Christian Couder > >> >> <christian.couder@gmail.com> wrote: > >> >> > On Thu, Apr 12, 2018 at 9:49 AM, Harald Nordgren > >> >> > <haraldnordgren@gmail.com> 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/20150304053333.GA9584@peff.net/ > >> >> > https://public-inbox.org/git/4D3CDDF9.6080405@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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Create '--merges-only' option for 'git bisect' 2018-04-11 23:33 ` Stefan Beller 2018-04-12 7:49 ` Harald Nordgren @ 2018-04-12 11:35 ` Johannes Schindelin 1 sibling, 0 replies; 12+ messages in thread From: Johannes Schindelin @ 2018-04-12 11:35 UTC (permalink / raw) To: Stefan Beller; +Cc: Harald Nordgren, git Hi Stefan, On Wed, 11 Apr 2018, Stefan Beller wrote: > On Wed, Apr 11, 2018 at 3:55 PM, Harald Nordgren > <haraldnordgren@gmail.com> 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Create '--merges-only' option for 'git bisect' 2018-04-11 22:55 [PATCH] Create '--merges-only' option for 'git bisect' Harald Nordgren 2018-04-11 23:33 ` Stefan Beller @ 2018-04-12 11:33 ` Johannes Schindelin 1 sibling, 0 replies; 12+ messages in thread From: Johannes Schindelin @ 2018-04-12 11:33 UTC (permalink / raw) To: Harald Nordgren; +Cc: git 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 a parent that was not marked as interesting earlier? If this is the case, why do we not have to look further, at later parents, whether they *do* have a non-NULL `util` attribute? > if (0 <= weight(q)) > break; > } > if (!q) > continue; > + if (!q->item->util) { > + counted++; > + continue; > + } Okay, so here we skip the current commit if we found a parent commit that has the `util` field set to NULL... > @@ -364,8 +383,43 @@ static struct commit_list *do_find_bisection(struct commit_list *list, > return best_bisection_sorted(list, nr); > } > > +int merge_commit_or_root(const struct commit c) By value? Let's use `*c` instead of `c` here. > +{ > + if (!c.parents) > + return 1; > + > + return !!c.parents->next; Unless you need the return value to be 0 or 1 (instead of 0 or non-0), this is conciser: return !c.parents || c.parents->next; However, we should not detect root commits here. We are only interested in merges, according to the new option! So we should filter out root commits, too. So this is what you really want: int is_merge(const struct commit *c) { return c->parents && c->parents->next; } > +void filter_non_merge_commits(struct commit_list **commit_list) So does this keep non_merge_commits (similar to Makefile's $(filter ...) function?) Or does it remove them? Judging by the code, maybe "filter_out_non_merge_commits()" or "drop_regular_commits()"? Or "keep_only_root_and_merge_commits()"? > +{ > + struct commit_list *list1 = *commit_list; > + struct commit_list *list2 = NULL; > + *commit_list = NULL; > + > + for ( ; list1; list1 = list1->next) { > + if (merge_commit_or_root(*list1->item)) { > + list2 = list1; > + list1 = list1->next; > + list2->next = NULL; Since commit_lists are malloc()ed, they also need to be free()d when filtering something out. > + list2->item->parents = NULL; Why do we touch the parents here? This is a bit dangerous, as the item refers to the commit that `lookup_commit()` would produce again. And by setting parents = NULL, we pretend that those commits are root commits from now on. > + *commit_list = list2; > + break; Also, using the non-descriptive names `list1` and `list2` makes code a bit hard to understand. (And hence it makes it easy for bugs to creep in.) > + } > + } > + > + for ( ; list1; list1 = list1->next) { The previous for loop has the condition `list1` already. So at this point we know that `list1 == NULL`, and therefore this for loop is dead code. Did you mean `for (list1 = *commit_list; list1; list1 = list1->next)`? > + list2->next = NULL; Where is list2 set? Only in the previous loop, if it found a merge or root commit. Otherwise, list2 would still be NULL and dereferenced here! > + if (merge_commit_or_root(*list1->item)) { > + list2->next = list1; > + list2 = list2->next; > + list2->item->parents = list1; And here, we set the parents to list1... Again, this is messing with the commits that come back when `lookup_commit()` is called later. Changing the parents of a commit is something I imagine breaks too many assumption to be a safe thing. I'll read on for a little bit, maybe I get a better understanding what we need to do in this here function. > + } > + } > +} > + > void find_bisection(struct commit_list **commit_list, int *reaches, > - int *all, int find_all) > + int *all, int find_all, int only_merge_commits) Let's use a flag here, just as above. > { > int nr, on_list; > struct commit_list *list, *p, *best, *next, *last; > @@ -373,6 +427,10 @@ void find_bisection(struct commit_list **commit_list, int *reaches, > > show_list("bisection 2 entry", 0, 0, *commit_list); > > + if (only_merge_commits) { > + filter_non_merge_commits(commit_list); > + } > + > /* > * Count the number of total and tree-changing items on the > * list, while reversing the list. > @@ -400,7 +458,7 @@ void find_bisection(struct commit_list **commit_list, int *reaches, > weights = xcalloc(on_list, sizeof(*weights)); > > /* Do the real work of finding bisection commit. */ > - best = do_find_bisection(list, nr, weights, find_all); > + best = do_find_bisection(list, nr, weights, find_all, only_merge_commits); > if (best) { > if (!find_all) { > list->item = best->item; > @@ -878,7 +936,7 @@ static void check_good_are_ancestors_of_bad(const char *prefix, int no_checkout) > /* > * This does "git diff-tree --pretty COMMIT" without one fork+exec. > */ > -static void show_diff_tree(const char *prefix, struct commit *commit) > +static void show_diff_tree(const char *prefix, struct commit *commit, int only_merge_commits) > { > struct rev_info opt; > > @@ -893,6 +951,11 @@ static void show_diff_tree(const char *prefix, struct commit *commit) > opt.use_terminator = 0; > opt.commit_format = CMIT_FMT_DEFAULT; > > + if (only_merge_commits) { > + opt.ignore_merges = 0; > + opt.combine_merges = 1; > + } This is most likely what we want, whether --only-merge-commits was passed or not. Let's make it the default (in a separate commit)? > @@ -938,7 +1001,7 @@ void read_bisect_terms(const char **read_bad, const char **read_good) > * If no_checkout is non-zero, the bisection process does not > * checkout the trial commit but instead simply updates BISECT_HEAD. > */ > -int bisect_next_all(const char *prefix, int no_checkout) > +int bisect_next_all(const char *prefix, int no_checkout, int only_merge_commits) Let's use `flags` here, too. > { > struct rev_info revs; > struct commit_list *tried; > @@ -957,7 +1020,7 @@ int bisect_next_all(const char *prefix, int no_checkout) > > bisect_common(&revs); > > - find_bisection(&revs.commits, &reaches, &all, !!skipped_revs.nr); > + find_bisection(&revs.commits, &reaches, &all, !!skipped_revs.nr, only_merge_commits); With the `flags` change, we would probably just introduce a local variable `int flags = 0;` and set `flags |= !!skipped_revs.nr;` here at first, and only change the `flags` variable to a parameter when introducing the --only-merge-commits flag. > @@ -983,10 +1046,14 @@ int bisect_next_all(const char *prefix, int no_checkout) > bisect_rev = &revs.commits->item->object.oid; > > if (!oidcmp(bisect_rev, current_bad_oid)) { > + char *format_string = NULL; > exit_if_skipped_commits(tried, current_bad_oid); > - printf("%s is the first %s commit\n", oid_to_hex(bisect_rev), > - term_bad); > - show_diff_tree(prefix, revs.commits->item); > + if (only_merge_commits) > + format_string = "%s is the first %s merge\n"; > + else > + format_string = "%s is the first %s commit\n"; ... why? We can reference merge commits as "commits", too... Why not leave the message as-is? > diff --git a/bisect.h b/bisect.h > index a5d9248a4..664ada180 100644 > --- a/bisect.h > +++ b/bisect.h > @@ -9,7 +9,7 @@ > * best commit, as chosen by `find_all`. > */ > extern void find_bisection(struct commit_list **list, int *reaches, int *all, > - int find_all); > + int find_all, int only_merge_commits); Oh, I missed that this is a global function. So the flags should not be FIND_ALL and ONLY_MERGE_COMMITS but BISECT_FIND_ALL and BISECT_ONLY_MERGE_COMMITS, and `#define`d above this function. > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 4b5fadcbe..9d7a1dd23 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -106,6 +106,32 @@ static void check_expected_revs(const char **revs, int rev_nr) > } > } > > +static GIT_PATH_FUNC(git_path_bisect_merges_only, "MERGES_ONLY_BISECT") The other bisect-specific files start with BISECT_, let's do that here, too (i.e. "BISECT_ONLY_MERGES")? > +static int merges_only(void) > +{ > + const char *filename = git_path_bisect_merges_only(); > + struct stat st; > + struct strbuf str = STRBUF_INIT; > + FILE *fp; > + int res = 0; > + > + if (stat(filename, &st) || !S_ISREG(st.st_mode)) > + return 0; > + > + fp = fopen_or_warn(filename, "r"); > + if (!fp) > + return 0; > + > + if (strbuf_getline_lf(&str, fp) != EOF) > + res = atoi(str.buf); Why not use the presence of the file as flag, rather than its contents? Then the test becomes as simple as int merges_only = file_exists(git_path_bisect_merges_only()); > + > + strbuf_release(&str); > + fclose(fp); > + > + return res; > +} > + > int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > { > enum { > @@ -137,7 +163,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > > switch (cmdmode) { > case NEXT_ALL: > - return bisect_next_all(prefix, no_checkout); > + return bisect_next_all(prefix, no_checkout, merges_only()); > case WRITE_TERMS: > if (argc != 2) > return error(_("--write-terms requires two arguments")); > diff --git a/builtin/rev-list.c b/builtin/rev-list.c > index fadd3ec14..cacffe6c6 100644 > --- a/builtin/rev-list.c > +++ b/builtin/rev-list.c > @@ -538,7 +538,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) > if (bisect_list) { > int reaches, all; > > - find_bisection(&revs.commits, &reaches, &all, bisect_find_all); > + find_bisection(&revs.commits, &reaches, &all, bisect_find_all, 0); This would become bisect_flags in the preparatory patch. > > if (bisect_show_vars) > return show_bisect_vars(&info, reaches, all); > diff --git a/git-bisect.sh b/git-bisect.sh > index 54cbfecc5..730c983c5 100755 > --- a/git-bisect.sh > +++ b/git-bisect.sh > @@ -82,6 +82,7 @@ bisect_start() { > bad_seen=0 > eval='' > must_write_terms=0 > + merges_only=0 > revs='' > if test "z$(git rev-parse --is-bare-repository)" != zfalse > then > @@ -96,6 +97,9 @@ bisect_start() { > shift > break > ;; > + --merges-only) > + merges_only=1 > + shift ;; > --no-checkout) > mode=--no-checkout > shift ;; > @@ -212,6 +216,7 @@ bisect_start() { > git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD" || exit > fi && > echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit > + echo "$merges_only" >"$GIT_DIR/MERGES_ONLY_BISECT" || exit The git-bisect.sh script deviates from our coding convention, it seems. Typically, we would write merges_only= [...] --merges-only) merges_only=t ;; [...] test -z "$merges_only" || : >"$GIT_DIR/BISECT_ONLY_MERGES" || exit I would prefer that here, too. > diff --git a/t/t6070-bisect-merge-commits.sh b/t/t6070-bisect-merge-commits.sh > new file mode 100755 > index 000000000..edfae0cfd > --- /dev/null > +++ b/t/t6070-bisect-merge-commits.sh > @@ -0,0 +1,116 @@ > +#!/bin/sh > +# > +# Copyright (c) 2018 Harald Nordgren > +# > +test_description='Tests git bisect merge commit functionality' > + > +exec </dev/null This is unnecessary, as our test-lib.sh already executes `exec 6<&0`. Further, it is not only unnecessary, but also breaks debugging with the `debug` shell function defined in test-lib-function.sh. Let's just drop this statement. > + > +. ./test-lib.sh > + > +add_line_into_file() > +{ > + _line=$1 > + _file=$2 > + > + if [ -f "$_file" ]; then > + echo "$_line" >> $_file || return $? > + MSG="Add <$_line> into <$_file>." > + else > + echo "$_line" > $_file || return $? > + git add $_file || return $? > + MSG="Create file <$_file> with <$_line> inside." > + fi > + > + test_tick > + git commit --quiet -m "$MSG" $_file > +} It would appear that the contents of the files written using this function are never tested. Let's just use `test_commit` instead. > + > +create_merge_commit() > +{ > + _branch=$1 > + _file=$2 > + > + git checkout -b $_branch && > + add_line_into_file "hello" $_file && > + git checkout master && > + git merge $_branch --no-edit --no-ff > +} This not only creates a merge commit but also sets up a branch and adds a commit to it. I'd much rather have a function that performs the merge after calling `test_tick`, and then tags the result just like `test_commit` would do. > +HASH1= > +HASH2= > +HASH3= > +HASH4= > +HASH5= > +HASH6= These are too non-descriptive names to be useful. Remember that the purpose of a regression test is to help debugging regressions. If I encounter a regression while developing a patch series that breaks this test, I really need to understand the regression test as quickly as possible. So let's use HEAD~4 and/or the tags creates by `test_commit` instead. > +test_expect_success 'set up basic repo with 3 files and 3 merge commits' ' If we say 'setup' here, it is not only conciser, but also less prone to become incorrect in the future. > + add_line_into_file "hello" hello && > + HASH1=$(git rev-parse --verify HEAD) && > + > + add_line_into_file "hello" hello && > + git checkout -b branch1 && > + add_line_into_file "hello" hello1 && > + git checkout master && > + git checkout -b branch2 && > + add_line_into_file "hello" hello2 && > + git checkout master && > + git checkout -b branch3 && > + add_line_into_file "hello" hello3 && > + git checkout master && > + git merge branch1 --no-edit --no-ff && > + HASH2=$(git rev-parse --verify HEAD) && > + > + add_line_into_file "hello" hello && > + add_line_into_file "hello" hello && > + git merge branch2 --no-edit --no-ff && > + git merge branch3 --no-edit --no-ff && > + HASH3=$(git rev-parse --verify HEAD) && > + > + add_line_into_file "hello" hello && > + HASH4=$(git rev-parse --verify HEAD) && > + > + create_merge_commit branch4 hello4 && > + create_merge_commit branch5 hello5 && > + create_merge_commit branch6 hello6 && > + create_merge_commit branch7 hello7 && > + create_merge_commit branch8 hello8 && > + create_merge_commit branch9 hello9 && > + create_merge_commit branch10 hello10 && > + create_merge_commit branch11 hello11 && > + create_merge_commit branch12 hello12 && > + create_merge_commit branch13 hello13 && > + create_merge_commit branch14 hello14 && > + create_merge_commit branch15 hello15 && > + create_merge_commit branch16 hello16 && > + HASH5=$(git rev-parse --verify HEAD) && > + > + create_merge_commit branch17 hello17 && > + create_merge_commit branch18 hello18 && > + create_merge_commit branch19 hello19 && > + create_merge_commit branch20 hello20 && > + HASH6=$(git rev-parse --verify HEAD) > +' It is next to impossible to understand this function in an 80x25 terminal (which many of us old-timers still use). And it is not even necessary in order to test the functionality you introduced... So how about this strategy: first, paint a nice little diagram of the commit graph you want to achieve, and make it not as insanely complex. Then, set that up using `test_commit`, `git checkout -b` and `test_tick && git merge`. Something like this: # We generate the following commit graph: # # A - B - C - F # \ \ / \ # D - E - G - H test_merge () { test_tick && git merge "$@" && git tag "$1" } test_expect_success 'setup' ' test_commit A && test_commit B && test_commit C && git reset --hard A && test_commit D && test_merge -m E B && git reset --hard C && git merge -m F E && git reset --hard G && git merge -m H F ' > +test_expect_success 'bisect skip: successful result' ' I do not actually see that `bisect skip` is tested here. > + test_when_finished git bisect reset && > + git bisect reset && > + git bisect start $HASH4 $HASH1 --merges-only && > + git bisect good && > + git bisect good && > + git bisect bad > my_bisect_log.txt && > + grep "$HASH3 is the first bad merge" my_bisect_log.txt > +' What we want to verify here is that we only encounter merge commits, right? So that is what we should test: test_expect_success '--merges-only' ' write_script find-c.sh <<-\EOF && case "$(git show --format=%p -s)" in *\ *) ;; # merge commit: okay *) git rev-parse HEAD >non-merge-found;; esac # detect whether we are "before" or "after" C test ! -f C.t EOF git bisect start HEAD A && git bisect run ./find-c.sh >actual && test_path_is_missing non-merge-found && grep "$(git rev-parse F) is the first bad commit" actual ' > + > +test_expect_success 'bisect skip: successful result' ' > + test_when_finished git bisect reset && > + git bisect reset && > + git bisect start $HASH6 $HASH2 --merges-only && > + git bisect good && > + git bisect good && > + git bisect bad && > + git bisect bad > my_bisect_log.txt && > + grep "$HASH5 is the first bad merge" my_bisect_log.txt > +' > + > +test_done So after reading this, I am quite a bit more puzzled about that commit->util field we seem to use. Could you clarify that in the commit message, to give readers a head start? Okay, now let's come back to that function accepting a commit_list and dropping all non-merge commits from it. First of all, I do not think that we actually make use of the parents information after calling this function, right? If we did, I think it would be horribly broken because all non-merge commits would now be looking like root commits, and all the merge commits would pretend to have *all* of the previous merge commits as parents (the first merge commit would therefore look like a root commit, the second merge commit would look like a non-merge, and so on). So let's just drop the parent rewriting? If we do need to collapse the commit graph to the merge commit nodes, then a much better idea would be to use an oidset with commit_lists representing the collapsed vertices in that graph (i.e. for every commit, it would replace the non-merge parents with the respective closest ancestor that *is* a merge, and this list would be constructed in reverse topological order to give linear complexity). Under the assumption that we can leave the commits' parents alone, this is my proposed function to keep only merge commits in a commit_list: static void keep_only_merge_commits(struct commit_list **p) { struct commit_list *parents, *next; while (*p) { parents = (*p)->item->parents; if (parents && parents->next) { /* merge commit: keep */ p = &(*p)->next; continue; } /* non-merge commit: free() and skip */ next = (*p)->next; free(*p); *p = next; } } This makes use of the fact that both the original list of commits, as well as the `next` field in the commit_list items, is a pointer to a commit_list. So once we processed the first item of the list, we either free it and replace the list by the remainder of the list, or we advance to the "sub list" starting with the second item of the list (if there is any left). Ciao, Johannes ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-05-01 11:31 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-11 22:55 [PATCH] Create '--merges-only' option for 'git bisect' Harald Nordgren 2018-04-11 23:33 ` Stefan Beller 2018-04-12 7:49 ` Harald Nordgren 2018-04-12 8:58 ` Christian Couder 2018-04-12 9:27 ` Tiago Botelho 2018-04-12 11:53 ` Johannes Schindelin 2018-04-12 12:19 ` Tiago Botelho 2018-04-30 11:31 ` Johannes Schindelin 2018-04-30 11:34 ` Tiago Botelho 2018-05-01 11:31 ` Johannes Schindelin 2018-04-12 11:35 ` Johannes Schindelin 2018-04-12 11:33 ` Johannes Schindelin
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).