Introduce --first-parent flag for git bisect
cover Introduce --first-parent flag for git bisect
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]/
Aaron Lipman (3):
rev-list: allow bisect and first-parent flags
bisect: introduce first-parent flag
bisect: combine args passed to find_bisection()
Documentation/git-bisect.txt | 13 +++++- Documentation/rev-list-options.txt | 7 ++-- bisect.c | 66 +++++++++++++++++++++--------- bisect.h | 7 +++- builtin/bisect--helper.c | 16 ++++++-- builtin/rev-list.c | 9 +++- revision.c | 3 -- t/t6000-rev-list-misc.sh | 4 +- t/t6002-rev-list-bisect.sh | 45 ++++++++++++++++++++ t/t6030-bisect-porcelain.sh | 17 ++++++++ 10 files changed, 151 insertions(+), 36 deletions(-)
base-commit: 00a7a21b97a47889cf66de32f8defed023c14c2c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-686%2Falipman88%2Fbisect_first_parent_final_draft-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-686/alipman88/bisect_first_parent_final_draft-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/686
Signature · 1 line
gitgitgadget
PATCH 1/3 rev-list: allow bisect and first-parent flags
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
--- Documentation/rev-list-options.txt | 7 ++--- bisect.c | 43 +++++++++++++++++----------- bisect.h | 2 +- builtin/rev-list.c | 2 +- revision.c | 3 -- t/t6000-rev-list-misc.sh | 4 +-- t/t6002-rev-list-bisect.sh | 45 ++++++++++++++++++++++++++++++ 7 files changed, 79 insertions(+), 27 deletions(-)
Documentation/rev-list-options.txt +3 -4
@@ -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 adjusting to updated upstream from time to time, and this option allows you to ignore the individual commits - brought in to your history by such a merge. Cannot be - combined with --bisect. + brought in to your history by such a merge. --not:: Reverses the meaning of the '{caret}' prefix (or lack thereof)
@@ -207,7 +206,7 @@ ifndef::git-rev-list[] Pretend as if the bad bisection ref `refs/bisect/bad` was listed and as if it was followed by `--not` and the good bisection refs `refs/bisect/good-*` on the command - line. Cannot be combined with --first-parent. + line. endif::git-rev-list[] --stdin::
@@ -743,7 +742,7 @@ outputs 'midpoint', the output of the two commands would be of roughly the same length. Finding the change which introduces a regression is thus reduced to a binary search: repeatedly generate and test new 'midpoint's until the commit chain is of length -one. Cannot be combined with --first-parent. +one. --bisect-vars:: This calculates the same as `--bisect`, except that refs in
bisect.c +27 -16
124 lines not on this page · raw message
bisect.h +1 -1
@@ -12,7 +12,7 @@ struct repository;* best commit, as chosen by `find_all`. */ void find_bisection (struct commit_list **list,int *reaches,int *all, -int find_all); +int find_all,int first_parent_only); struct commit_list *filter_skipped(struct commit_list *list, struct commit_list **tried,
builtin/rev-list.c +1 -1
@@ -638,7 +638,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, revs.first_parent_only);if (bisect_show_vars)return show_bisect_vars (&info, reaches, all);
revision.c +0 -3
@@ -2869,9 +2869,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct sif (!revs->reflog_info && revs->grep_filter.use_reflog_filter)die ("cannot use --grep-reflog without --walk-reflogs" ); -if (revs->first_parent_only && revs->bisect) -die (_ ("--first-parent is incompatible with --bisect" )); -if (revs->line_level_traverse && (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH |DIFF_FORMAT_NO_OUTPUT )))die (_ ("-L does not yet support diff formats besides -p and -s" ));
t/t6000-rev-list-misc.sh +2 -2
@@ -125,8 +125,8 @@ test_expect_success 'rev-list can negate index objects' ' test_cmp expect actual ' -test_expect_success '--bisect and --first-parent can not be combined' ' - test_must_fail git rev-list --bisect --first-parent HEAD +test_expect_success '--bisect and --first-parent can be combined' ' + git rev-list --bisect --first-parent HEAD ' test_expect_success '--header shows a NUL after each commit' '
t/t6002-rev-list-bisect.sh +45 -0
49 lines not on this page · raw message
Signature · 1 line
gitgitgadget
PATCH 2/3 bisect: introduce first-parent flag
In reply to Aaron Lipman via GitGitGadgetFrom: Aaron Lipman <[email protected]>
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.
--- Documentation/git-bisect.txt | 13 ++++++++++++- bisect.c | 10 +++++++++- bisect.h | 2 ++ builtin/bisect--helper.c | 16 ++++++++++++---- t/t6030-bisect-porcelain.sh | 17 +++++++++++++++++ 5 files changed, 52 insertions(+), 6 deletions(-)
Documentation/git-bisect.txt +12 -1
@@ -17,7 +17,7 @@ The command takes various subcommands, and different options depending on the subcommand: git bisect start [--term-{old,good}=<term> --term-{new,bad}=<term>] - [--no-checkout] [<bad> [<good>...]] [--] [<paths>...] + [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...] git bisect (bad|new|<term-new>) [<rev>] git bisect (good|old|<term-old>) [<rev>...] git bisect terms [--term-good | --term-bad]
@@ -365,6 +365,17 @@ does not require a checked out tree. + If the repository is bare, `--no-checkout` is assumed. +--first-parent:: ++ +Follow only the first parent commit upon seeing a merge commit. ++ +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. + EXAMPLES --------
bisect.c +9 -1
@@ -463,6 +463,7 @@ static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN") static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START") static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG") static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS") +static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT") static GIT_PATH_FUNC(git_path_head_name, "head-name") static void read_bisect_paths(struct argv_array *array)
@@ -984,6 +985,12 @@ void read_bisect_terms(const char **read_bad, const char **read_good)fclose (fp); } +int read_first_parent_option (void ) +{ +const char *filename =git_path_bisect_first_parent (); +return !access (filename,F_OK ); +} +/* * We use the convention that return BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND (-10) means * the bisection process finished successfully.
@@ -1000,7 +1007,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, intenum bisect_error res =BISECT_OK ;struct object_id *bisect_rev;char *steps_msg; -int first_parent_only =0 ;/* TODO: pass --first-parent flag from git bisect start */ +int first_parent_only =read_first_parent_option ();read_bisect_terms (&term_bad, &term_good);if (read_bisect_refs ())
@@ -1144,6 +1151,7 @@ int bisect_clean_state(void)unlink_or_warn (git_path_bisect_names ());unlink_or_warn (git_path_bisect_run ());unlink_or_warn (git_path_bisect_terms ()); +unlink_or_warn (git_path_bisect_first_parent ());/* Cleanup head-name if it got left by an old version of git-bisect */ unlink_or_warn (git_path_head_name ());/*
bisect.h +2 -0
@@ -66,6 +66,8 @@ int estimate_bisect_steps(int all);void read_bisect_terms (const char **bad,const char **good); +int read_first_parent_option (void ); +int bisect_clean_state (void );#endif
builtin/bisect--helper.c +12 -4
@@ -17,6 +17,7 @@ static GIT_PATH_FUNC(git_path_bisect_head, "BISECT_HEAD") static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG") static GIT_PATH_FUNC(git_path_head_name, "head-name") static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES") +static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT") static const char * const git_bisect_helper_usage[] = {N_ ("git bisect--helper --next-all [--no-checkout]" ),
@@ -28,7 +29,7 @@ static const char * const git_bisect_helper_usage[] = {N_ ("git bisect--helper --bisect-next-check <good_term> <bad_term> [<term>]" ),N_ ("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]" ),N_ ("git bisect--helper --bisect-start [--term-{old,good}=<term> --term-{new,bad}=<term>]" -"[--no-checkout] [<bad> [<good>...]] [--] [<paths>...]" ), +" [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]" ), NULL };
@@ -422,7 +423,7 @@ static int bisect_append_log_quoted(const char **argv) }static int bisect_start (struct bisect_terms *terms,int no_checkout, -const char **argv,int argc) +int first_parent_only,const char **argv,int argc) {int i, has_double_dash =0 , must_write_terms =0 , bad_seen =0 ;int flags, pathspec_pos, res =0 ;
@@ -453,6 +454,8 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,break ; }else if (!strcmp (arg,"--no-checkout" )) { no_checkout =1 ; + }else if (!strcmp (arg,"--first-parent" )) { + first_parent_only =1 ; }else if (!strcmp (arg,"--term-good" ) || !strcmp (arg,"--term-old" )) { i++;
@@ -577,6 +580,9 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,*/ write_file (git_path_bisect_start (),"%s\n" , start_head.buf); +if (first_parent_only) +write_file (git_path_bisect_first_parent (),"\n" ); +if (no_checkout) {if (get_oid (start_head.buf, &oid) <0 ) { res =error (_ ("invalid ref: '%s'" ), start_head.buf);
@@ -632,7 +638,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)BISECT_TERMS , BISECT_START } cmdmode =0 ; -int no_checkout =0 , res =0 , nolog =0 ; +int no_checkout =0 , first_parent_only =0 , res =0 , nolog =0 ;struct option options[] = {OPT_CMDMODE (0 ,"next-all" , &cmdmode,N_ ("perform 'git bisect next'" ),NEXT_ALL ),
@@ -656,6 +662,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) N_("start the bisect session"), BISECT_START),OPT_BOOL (0 ,"no-checkout" , &no_checkout,N_ ("update BISECT_HEAD instead of checking out the current commit" )), +OPT_BOOL (0 ,"first-parent" , &first_parent_only, +N_ ("only trace the first parent of merge commits" )),OPT_BOOL (0 ,"no-log" , &nolog,N_ ("no log for BISECT_WRITE" )),OPT_END ()
@@ -713,7 +721,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)break ;case BISECT_START :set_terms (&terms,"bad" ,"good" ); - res =bisect_start (&terms, no_checkout, argv, argc); + res =bisect_start (&terms, no_checkout, first_parent_only, argv, argc);break ;default :return error ("BUG: unknown subcommand '%d'" , cmdmode);
t/t6030-bisect-porcelain.sh +17 -0
@@ -458,6 +458,23 @@ test_expect_success 'many merge bases creation' ' grep "$SIDE_HASH5" merge_bases.txt ' +# We want to automatically find the merge that +# introduced "line" into hello. +test_expect_success \ + '"git bisect run --first-parent" simple case' \ + 'echo "#"\!"/bin/sh" > test_script.sh && + echo "grep line hello > /dev/null" >> test_script.sh && + echo "test \$? -ne 0" >> test_script.sh && + chmod +x test_script.sh && + git bisect start --first-parent && + test_path_is_file ".git/BISECT_FIRST_PARENT" && + git bisect good $HASH4 && + git bisect bad $B_HASH && + git bisect run ./test_script.sh > my_bisect_log.txt && + grep "$B_HASH is the first bad commit" my_bisect_log.txt && + git bisect reset && + test_path_is_missing ".git/BISECT_FIRST_PARENT"' + test_expect_success 'good merge bases when good and bad are siblings' ' git bisect start "$B_HASH" "$A_HASH" > my_bisect_log.txt && test_i18ngrep "merge base must be tested" my_bisect_log.txt &&
Signature · 1 line
gitgitgadget
PATCH 3/3 bisect: combine args passed to find_bisection()
In reply to Aaron Lipman via GitGitGadgetFrom: 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
--- bisect.c | 43 +++++++++++++++++++++++++------------------ bisect.h | 5 ++++- builtin/rev-list.c | 9 ++++++++- 3 files changed, 37 insertions(+), 20 deletions(-)
bisect.c +25 -18
@@ -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,unsigned bisect_flags) {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 && !(bisect_flags &BISECT_FIRST_PARENT )) { p = p->next;while (p) { - nr +=count_distance (p, first_parent_only); + nr +=count_distance (p, bisect_flags); p = p->next; } }
@@ -88,7 +88,7 @@ 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,int first_parent_only) +static int count_interesting_parents (struct commit *commit,unsigned bisect_flags) {struct commit_list *p;int count;
@@ -96,7 +96,7 @@ static int count_interesting_parents(struct commit *commit, int first_parent_onlfor (count =0 , p = commit->parents; p; p = p->next) {if (!(p->item->object.flags &UNINTERESTING )) count++; -if (first_parent_only) +if (bisect_flags &BISECT_FIRST_PARENT )break ; }return count;
@@ -260,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 first_parent_only) +unsigned bisect_flags) {int n, counted;struct commit_list *p;
@@ -272,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, first_parent_only)) { +switch (count_interesting_parents (commit, bisect_flags)) {case 0 :if (!(flags &TREESAME )) {weight_set (p,1 );
@@ -315,11 +315,11 @@ 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)); +weight_set (p,count_distance (p, bisect_flags));clear_distance (list);/* Does it happen to be at exactly half-way? */ -if (!find_all &&halfway (p, nr)) +if (!(bisect_flags &BISECT_FIND_ALL ) &&halfway (p, nr))return p; counted++; }
@@ -342,7 +342,7 @@ 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 ) &&0 <=weight (q)) {break ; - }else if (first_parent_only) { + }else if (bisect_flags &BISECT_FIRST_PARENT ) { q =NULL ;break ; }
@@ -365,21 +365,21 @@ static struct commit_list *do_find_bisection(struct commit_list *list,weight_set (p,weight (q));/* Does it happen to be at exactly half-way? */ -if (!find_all &&halfway (p, nr)) +if (!(bisect_flags &BISECT_FIND_ALL ) &&halfway (p, nr))return p; } }show_list ("bisection 2 counted all" , counted, nr, list); -if (!find_all) +if (!(bisect_flags &BISECT_FIND_ALL ))return best_bisection (list, nr);else return best_bisection_sorted (list, nr); }void find_bisection (struct commit_list **commit_list,int *reaches, -int *all,int find_all,int first_parent_only) +int *all,unsigned bisect_flags) {int nr, on_list;struct commit_list *list, *p, *best, *next, *last;
@@ -415,9 +415,9 @@ 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, first_parent_only); + best =do_find_bisection (list, nr, weights, bisect_flags);if (best) { -if (!find_all) { +if (!(bisect_flags &BISECT_FIND_ALL )) { list->item = best->item;free_commit_list (list->next); best = list;
@@ -1007,23 +1007,30 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, intenum bisect_error res =BISECT_OK ;struct object_id *bisect_rev;char *steps_msg; -int first_parent_only =read_first_parent_option (); +unsigned bisect_flags =0 ;read_bisect_terms (&term_bad, &term_good);if (read_bisect_refs ())die (_ ("reading bisect refs failed" )); +if (read_first_parent_option ()) + bisect_flags |=BISECT_FIRST_PARENT ; + +if (!!skipped_revs.nr) + bisect_flags |=BISECT_FIND_ALL ; + res =check_good_are_ancestors_of_bad (r, prefix, no_checkout);if (res)return res;bisect_rev_setup (r, &revs, prefix,"%s" ,"^%s" ,1 ); - revs.first_parent_only = first_parent_only; + + revs.first_parent_only = !!(bisect_flags &BISECT_FIRST_PARENT ); revs.limited =1 ;bisect_common (&revs); -find_bisection (&revs.commits, &reaches, &all, !!skipped_revs.nr, first_parent_only); +find_bisection (&revs.commits, &reaches, &all, bisect_flags); revs.commits =managed_skipped (revs.commits, &tried);if (!revs.commits) {
bisect.h +4 -1
@@ -12,7 +12,7 @@ struct repository;* best commit, as chosen by `find_all`. */ void find_bisection (struct commit_list **list,int *reaches,int *all, -int find_all,int first_parent_only); +unsigned bisect_flags); struct commit_list *filter_skipped(struct commit_list *list, struct commit_list **tried,
@@ -23,6 +23,9 @@ struct commit_list *filter_skipped(struct commit_list *list,#define BISECT_SHOW_ALL (1<<0)#define REV_LIST_QUIET (1<<1) +#define BISECT_FIND_ALL (1u<<0) +#define BISECT_FIRST_PARENT (1u<<1) +struct rev_list_info {struct rev_info *revs;int flags;
builtin/rev-list.c +8 -1
@@ -637,8 +637,15 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)if (bisect_list) {int reaches, all; +unsigned bisect_flags =0 ; -find_bisection (&revs.commits, &reaches, &all, bisect_find_all, revs.first_parent_only); +if (bisect_find_all) + bisect_flags |=BISECT_FIND_ALL ; + +if (revs.first_parent_only) + bisect_flags |=BISECT_FIRST_PARENT ; + +find_bisection (&revs.commits, &reaches, &all, bisect_flags);if (bisect_show_vars)return show_bisect_vars (&info, reaches, all);
Signature · 1 line
gitgitgadget
Re: [PATCH 1/3] rev-list: allow bisect and first-parent flags
In reply to Aaron Lipman via GitGitGadgetQuoted 8 lines · Aaron Lipman via GitGitGadget
From: Aaron Lipman <[email protected]>
Add
first_parent_onlyparameter tofind_bisection(), removing the barrier that prevented combining the --bisect and --first-parent flags when using git rev-listSigned-off-by Aaron Lipman <[email protected]>Based-on-patch-by Tiago Botelho <[email protected]>
In chronological order, please.
Quoted 16 lines
bisect.c +1
-1@@ -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;
bisect.c static int count_distance(struct commit_list *entry) · line 61
Quoted 10 lines
bisect.c +2
-2entry = 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?
Quoted 21 lines
bisect.c +5
-4@@ -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;
Quoted 1 line
}
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.
Quoted 19 lines
bisect.c +2
-2@@ -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 );
bisect.c static struct commit_list *do_find_bisection(struct commit_list *list, · line 317
Quoted 3 lines
bisect.c +0
-0@@ -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.
bisect.c static struct commit_list *do_find_bisection(struct commit_list *list, · line 318
Quoted 2 lines
bisect.c +1
-1-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?
bisect.c static struct commit_list *do_find_bisection(struct commit_list *list, · line 341
Quoted 12 lines
bisect.c +6
-0@@ -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.
bisect.c static struct commit_list *do_find_bisection(struct commit_list *list, · line 344
Quoted 6 lines
bisect.c +2
-3for (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.
Re: [PATCH 1/3] rev-list: allow bisect and first-parent flags
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(-)
bisect.c +12 -15
@@ -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 ;
v2 cover Introduce --first-parent flag for git bisect
In reply to Aaron Lipman via GitGitGadgetJunio, thank you for the feedback and the explanation regarding count_distance(). I've gone ahead and implemented all your suggestions.
Quoted 1 line
In chronological order, please.
I submitted my initial patch through GitGitGadget. I don't see a way to control the order GitGitGadget sends emails, so I'm switching to git send-email for this iteration. Please continue to let me know if I'm missing a convention.
Aaron Lipman (3):
rev-list: allow bisect and first-parent flags
bisect: introduce first-parent flag
bisect: combine args passed to find_bisection()
Documentation/git-bisect.txt | 13 +++++++- Documentation/rev-list-options.txt | 7 ++-- bisect.c | 51 ++++++++++++++++++++++-------- bisect.h | 7 +++- builtin/bisect--helper.c | 16 +++++++--- builtin/rev-list.c | 9 +++++- revision.c | 3 -- t/t6000-rev-list-misc.sh | 4 +-- t/t6002-rev-list-bisect.sh | 45 ++++++++++++++++++++++++++ t/t6030-bisect-porcelain.sh | 17 ++++++++++ 10 files changed, 142 insertions(+), 30 deletions(-)
Signature · 1 line
2.24.3 (Apple Git-128)
PATCH v2 1/3 rev-list: allow bisect and first-parent flags
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
--- Documentation/rev-list-options.txt | 7 ++--- bisect.c | 28 ++++++++++++------- bisect.h | 2 +- builtin/rev-list.c | 2 +- revision.c | 3 -- t/t6000-rev-list-misc.sh | 4 +-- t/t6002-rev-list-bisect.sh | 45 ++++++++++++++++++++++++++++++ 7 files changed, 70 insertions(+), 21 deletions(-)
Documentation/rev-list-options.txt +3 -4
@@ -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 adjusting to updated upstream from time to time, and this option allows you to ignore the individual commits - brought in to your history by such a merge. Cannot be - combined with --bisect. + brought in to your history by such a merge. --not:: Reverses the meaning of the '{caret}' prefix (or lack thereof)
@@ -207,7 +206,7 @@ ifndef::git-rev-list[] Pretend as if the bad bisection ref `refs/bisect/bad` was listed and as if it was followed by `--not` and the good bisection refs `refs/bisect/good-*` on the command - line. Cannot be combined with --first-parent. + line. endif::git-rev-list[] --stdin::
@@ -743,7 +742,7 @@ outputs 'midpoint', the output of the two commands would be of roughly the same length. Finding the change which introduces a regression is thus reduced to a binary search: repeatedly generate and test new 'midpoint's until the commit chain is of length -one. Cannot be combined with --first-parent. +one. --bisect-vars:: This calculates the same as `--bisect`, except that refs in
bisect.c +18 -10
@@ -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; }
@@ -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,6 +315,8 @@ static struct commit_list *do_find_bisection(struct commit_list *list,continue ;if (weight (p) !=-2 )continue ; +if (first_parent_only) +BUG ("shouldn't be calling count-distance in fp mode" );weight_set (p,count_distance (p));clear_distance (list);
@@ -332,7 +335,10 @@ static struct commit_list *do_find_bisection(struct commit_list *list,if (0 <=weight (p))continue ; -for (q = p->item->parents; q; q = q->next) { + +for (q = p->item->parents; + q; + q = first_parent_only ?NULL : q->next) {if (q->item->object.flags &UNINTERESTING )continue ;if (0 <=weight (q))
@@ -370,7 +376,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list, }void find_bisection (struct commit_list **commit_list,int *reaches, -int *all,int find_all) +int *all,int find_all,int first_parent_only) {int nr, on_list;struct commit_list *list, *p, *best, *next, *last;
@@ -406,7 +412,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, first_parent_only);if (best) {if (!find_all) { list->item = best->item;
@@ -991,6 +997,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, intenum bisect_error res =BISECT_OK ;struct object_id *bisect_rev;char *steps_msg; +int first_parent_only =0 ;/* TODO: pass --first-parent flag from git bisect start */ read_bisect_terms (&term_bad, &term_good);if (read_bisect_refs ())
@@ -1001,11 +1008,12 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, intreturn res;bisect_rev_setup (r, &revs, prefix,"%s" ,"^%s" ,1 ); + revs.first_parent_only = first_parent_only; revs.limited =1 ;bisect_common (&revs); -find_bisection (&revs.commits, &reaches, &all, !!skipped_revs.nr); +find_bisection (&revs.commits, &reaches, &all, !!skipped_revs.nr, first_parent_only); revs.commits =managed_skipped (revs.commits, &tried);if (!revs.commits) {
bisect.h +1 -1
@@ -12,7 +12,7 @@ struct repository;* best commit, as chosen by `find_all`. */ void find_bisection (struct commit_list **list,int *reaches,int *all, -int find_all); +int find_all,int first_parent_only); struct commit_list *filter_skipped(struct commit_list *list, struct commit_list **tried,
builtin/rev-list.c +1 -1
@@ -638,7 +638,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, revs.first_parent_only);if (bisect_show_vars)return show_bisect_vars (&info, reaches, all);
revision.c +0 -3
@@ -2869,9 +2869,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct sif (!revs->reflog_info && revs->grep_filter.use_reflog_filter)die ("cannot use --grep-reflog without --walk-reflogs" ); -if (revs->first_parent_only && revs->bisect) -die (_ ("--first-parent is incompatible with --bisect" )); -if (revs->line_level_traverse && (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH |DIFF_FORMAT_NO_OUTPUT )))die (_ ("-L does not yet support diff formats besides -p and -s" ));
t/t6000-rev-list-misc.sh +2 -2
@@ -125,8 +125,8 @@ test_expect_success 'rev-list can negate index objects' ' test_cmp expect actual ' -test_expect_success '--bisect and --first-parent can not be combined' ' - test_must_fail git rev-list --bisect --first-parent HEAD +test_expect_success '--bisect and --first-parent can be combined' ' + git rev-list --bisect --first-parent HEAD ' test_expect_success '--header shows a NUL after each commit' '
t/t6002-rev-list-bisect.sh +45 -0
49 lines not on this page · raw message
Signature · 1 line
2.24.3 (Apple Git-128)
PATCH v2 2/3 bisect: introduce first-parent flag
In reply to Aaron LipmanUpon 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.
--- Documentation/git-bisect.txt | 13 ++++++++++++- bisect.c | 10 +++++++++- bisect.h | 2 ++ builtin/bisect--helper.c | 16 ++++++++++++---- t/t6030-bisect-porcelain.sh | 17 +++++++++++++++++ 5 files changed, 52 insertions(+), 6 deletions(-)
Documentation/git-bisect.txt +12 -1
@@ -17,7 +17,7 @@ The command takes various subcommands, and different options depending on the subcommand: git bisect start [--term-{old,good}=<term> --term-{new,bad}=<term>] - [--no-checkout] [<bad> [<good>...]] [--] [<paths>...] + [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...] git bisect (bad|new|<term-new>) [<rev>] git bisect (good|old|<term-old>) [<rev>...] git bisect terms [--term-good | --term-bad]
@@ -365,6 +365,17 @@ does not require a checked out tree. + If the repository is bare, `--no-checkout` is assumed. +--first-parent:: ++ +Follow only the first parent commit upon seeing a merge commit. ++ +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. + EXAMPLES --------
bisect.c +9 -1
@@ -460,6 +460,7 @@ static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN") static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START") static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG") static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS") +static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT") static GIT_PATH_FUNC(git_path_head_name, "head-name") static void read_bisect_paths(struct argv_array *array)
@@ -981,6 +982,12 @@ void read_bisect_terms(const char **read_bad, const char **read_good)fclose (fp); } +int read_first_parent_option (void ) +{ +const char *filename =git_path_bisect_first_parent (); +return !access (filename,F_OK ); +} +/* * We use the convention that return BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND (-10) means * the bisection process finished successfully.
@@ -997,7 +1004,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, intenum bisect_error res =BISECT_OK ;struct object_id *bisect_rev;char *steps_msg; -int first_parent_only =0 ;/* TODO: pass --first-parent flag from git bisect start */ +int first_parent_only =read_first_parent_option ();read_bisect_terms (&term_bad, &term_good);if (read_bisect_refs ())
@@ -1141,6 +1148,7 @@ int bisect_clean_state(void)unlink_or_warn (git_path_bisect_names ());unlink_or_warn (git_path_bisect_run ());unlink_or_warn (git_path_bisect_terms ()); +unlink_or_warn (git_path_bisect_first_parent ());/* Cleanup head-name if it got left by an old version of git-bisect */ unlink_or_warn (git_path_head_name ());/*
bisect.h +2 -0
@@ -66,6 +66,8 @@ int estimate_bisect_steps(int all);void read_bisect_terms (const char **bad,const char **good); +int read_first_parent_option (void ); +int bisect_clean_state (void );#endif
builtin/bisect--helper.c +12 -4
@@ -17,6 +17,7 @@ static GIT_PATH_FUNC(git_path_bisect_head, "BISECT_HEAD") static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG") static GIT_PATH_FUNC(git_path_head_name, "head-name") static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES") +static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT") static const char * const git_bisect_helper_usage[] = {N_ ("git bisect--helper --next-all [--no-checkout]" ),
@@ -28,7 +29,7 @@ static const char * const git_bisect_helper_usage[] = {N_ ("git bisect--helper --bisect-next-check <good_term> <bad_term> [<term>]" ),N_ ("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]" ),N_ ("git bisect--helper --bisect-start [--term-{old,good}=<term> --term-{new,bad}=<term>]" -"[--no-checkout] [<bad> [<good>...]] [--] [<paths>...]" ), +" [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]" ), NULL };
@@ -422,7 +423,7 @@ static int bisect_append_log_quoted(const char **argv) }static int bisect_start (struct bisect_terms *terms,int no_checkout, -const char **argv,int argc) +int first_parent_only,const char **argv,int argc) {int i, has_double_dash =0 , must_write_terms =0 , bad_seen =0 ;int flags, pathspec_pos, res =0 ;
@@ -453,6 +454,8 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,break ; }else if (!strcmp (arg,"--no-checkout" )) { no_checkout =1 ; + }else if (!strcmp (arg,"--first-parent" )) { + first_parent_only =1 ; }else if (!strcmp (arg,"--term-good" ) || !strcmp (arg,"--term-old" )) { i++;
@@ -577,6 +580,9 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,*/ write_file (git_path_bisect_start (),"%s\n" , start_head.buf); +if (first_parent_only) +write_file (git_path_bisect_first_parent (),"\n" ); +if (no_checkout) {if (get_oid (start_head.buf, &oid) <0 ) { res =error (_ ("invalid ref: '%s'" ), start_head.buf);
@@ -632,7 +638,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)BISECT_TERMS , BISECT_START } cmdmode =0 ; -int no_checkout =0 , res =0 , nolog =0 ; +int no_checkout =0 , first_parent_only =0 , res =0 , nolog =0 ;struct option options[] = {OPT_CMDMODE (0 ,"next-all" , &cmdmode,N_ ("perform 'git bisect next'" ),NEXT_ALL ),
@@ -656,6 +662,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) N_("start the bisect session"), BISECT_START),OPT_BOOL (0 ,"no-checkout" , &no_checkout,N_ ("update BISECT_HEAD instead of checking out the current commit" )), +OPT_BOOL (0 ,"first-parent" , &first_parent_only, +N_ ("only trace the first parent of merge commits" )),OPT_BOOL (0 ,"no-log" , &nolog,N_ ("no log for BISECT_WRITE" )),OPT_END ()
@@ -713,7 +721,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)break ;case BISECT_START :set_terms (&terms,"bad" ,"good" ); - res =bisect_start (&terms, no_checkout, argv, argc); + res =bisect_start (&terms, no_checkout, first_parent_only, argv, argc);break ;default :return error ("BUG: unknown subcommand '%d'" , cmdmode);
t/t6030-bisect-porcelain.sh +17 -0
@@ -458,6 +458,23 @@ test_expect_success 'many merge bases creation' ' grep "$SIDE_HASH5" merge_bases.txt ' +# We want to automatically find the merge that +# introduced "line" into hello. +test_expect_success \ + '"git bisect run --first-parent" simple case' \ + 'echo "#"\!"/bin/sh" > test_script.sh && + echo "grep line hello > /dev/null" >> test_script.sh && + echo "test \$? -ne 0" >> test_script.sh && + chmod +x test_script.sh && + git bisect start --first-parent && + test_path_is_file ".git/BISECT_FIRST_PARENT" && + git bisect good $HASH4 && + git bisect bad $B_HASH && + git bisect run ./test_script.sh > my_bisect_log.txt && + grep "$B_HASH is the first bad commit" my_bisect_log.txt && + git bisect reset && + test_path_is_missing ".git/BISECT_FIRST_PARENT"' + test_expect_success 'good merge bases when good and bad are siblings' ' git bisect start "$B_HASH" "$A_HASH" > my_bisect_log.txt && test_i18ngrep "merge base must be tested" my_bisect_log.txt &&
Signature · 1 line
2.24.3 (Apple Git-128)
PATCH v2 3/3 bisect: combine args passed to find_bisection()
In reply to Aaron LipmanNow 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
--- bisect.c | 37 ++++++++++++++++++++++--------------- bisect.h | 5 ++++- builtin/rev-list.c | 9 ++++++++- 3 files changed, 34 insertions(+), 17 deletions(-)
bisect.c +22 -15
@@ -88,7 +88,7 @@ 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,int first_parent_only) +static int count_interesting_parents (struct commit *commit,unsigned bisect_flags) {struct commit_list *p;int count;
@@ -96,7 +96,7 @@ static int count_interesting_parents(struct commit *commit, int first_parent_onlfor (count =0 , p = commit->parents; p; p = p->next) {if (!(p->item->object.flags &UNINTERESTING )) count++; -if (first_parent_only) +if (bisect_flags &BISECT_FIRST_PARENT )break ; }return count;
@@ -260,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 first_parent_only) +unsigned bisect_flags) {int n, counted;struct commit_list *p;
@@ -272,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, first_parent_only)) { +switch (count_interesting_parents (commit, bisect_flags)) {case 0 :if (!(flags &TREESAME )) {weight_set (p,1 );
@@ -315,13 +315,13 @@ static struct commit_list *do_find_bisection(struct commit_list *list,continue ;if (weight (p) !=-2 )continue ; -if (first_parent_only) +if (bisect_flags &BISECT_FIRST_PARENT )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? */ -if (!find_all &&halfway (p, nr)) +if (!(bisect_flags &BISECT_FIND_ALL ) &&halfway (p, nr))return p; counted++; }
@@ -338,7 +338,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,for (q = p->item->parents; q; - q = first_parent_only ?NULL : q->next) { + q = bisect_flags &BISECT_FIRST_PARENT ?NULL : q->next) {if (q->item->object.flags &UNINTERESTING )continue ;if (0 <=weight (q))
@@ -362,21 +362,21 @@ static struct commit_list *do_find_bisection(struct commit_list *list,weight_set (p,weight (q));/* Does it happen to be at exactly half-way? */ -if (!find_all &&halfway (p, nr)) +if (!(bisect_flags &BISECT_FIND_ALL ) &&halfway (p, nr))return p; } }show_list ("bisection 2 counted all" , counted, nr, list); -if (!find_all) +if (!(bisect_flags &BISECT_FIND_ALL ))return best_bisection (list, nr);else return best_bisection_sorted (list, nr); }void find_bisection (struct commit_list **commit_list,int *reaches, -int *all,int find_all,int first_parent_only) +int *all,unsigned bisect_flags) {int nr, on_list;struct commit_list *list, *p, *best, *next, *last;
@@ -412,9 +412,9 @@ 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, first_parent_only); + best =do_find_bisection (list, nr, weights, bisect_flags);if (best) { -if (!find_all) { +if (!(bisect_flags &BISECT_FIND_ALL )) { list->item = best->item;free_commit_list (list->next); best = list;
@@ -1004,23 +1004,30 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, intenum bisect_error res =BISECT_OK ;struct object_id *bisect_rev;char *steps_msg; -int first_parent_only =read_first_parent_option (); +unsigned bisect_flags =0 ;read_bisect_terms (&term_bad, &term_good);if (read_bisect_refs ())die (_ ("reading bisect refs failed" )); +if (read_first_parent_option ()) + bisect_flags |=BISECT_FIRST_PARENT ; + +if (!!skipped_revs.nr) + bisect_flags |=BISECT_FIND_ALL ; + res =check_good_are_ancestors_of_bad (r, prefix, no_checkout);if (res)return res;bisect_rev_setup (r, &revs, prefix,"%s" ,"^%s" ,1 ); - revs.first_parent_only = first_parent_only; + + revs.first_parent_only = !!(bisect_flags &BISECT_FIRST_PARENT ); revs.limited =1 ;bisect_common (&revs); -find_bisection (&revs.commits, &reaches, &all, !!skipped_revs.nr, first_parent_only); +find_bisection (&revs.commits, &reaches, &all, bisect_flags); revs.commits =managed_skipped (revs.commits, &tried);if (!revs.commits) {
bisect.h +4 -1
@@ -12,7 +12,7 @@ struct repository;* best commit, as chosen by `find_all`. */ void find_bisection (struct commit_list **list,int *reaches,int *all, -int find_all,int first_parent_only); +unsigned bisect_flags); struct commit_list *filter_skipped(struct commit_list *list, struct commit_list **tried,
@@ -23,6 +23,9 @@ struct commit_list *filter_skipped(struct commit_list *list,#define BISECT_SHOW_ALL (1<<0)#define REV_LIST_QUIET (1<<1) +#define BISECT_FIND_ALL (1u<<0) +#define BISECT_FIRST_PARENT (1u<<1) +struct rev_list_info {struct rev_info *revs;int flags;
builtin/rev-list.c +8 -1
@@ -637,8 +637,15 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)if (bisect_list) {int reaches, all; +unsigned bisect_flags =0 ; -find_bisection (&revs.commits, &reaches, &all, bisect_find_all, revs.first_parent_only); +if (bisect_find_all) + bisect_flags |=BISECT_FIND_ALL ; + +if (revs.first_parent_only) + bisect_flags |=BISECT_FIRST_PARENT ; + +find_bisection (&revs.commits, &reaches, &all, bisect_flags);if (bisect_show_vars)return show_bisect_vars (&info, reaches, all);
Signature · 1 line
2.24.3 (Apple Git-128)
Quoted 6 lines · Aaron Lipman
Junio, thank you for the feedback and the explanation regarding
count_distance(). I've gone ahead and implemented all your suggestions.Quoted 1 line
In chronological order, please.
I submitted my initial patch through GitGitGadget. I don't see a way to control the order GitGitGadget sends emails,...
Ah, I was referring to the order of sign-off, helped-by, etc. Somebody writes a patch that is left on the list, you pick it up and polish it for perfection, and send the result to the list.
So based-on would come first because it happened first, and then you worked further until the result deserves to be signed-off and finally you sent it out. So your sign-off comes the last.
Re: [PATCH v2 2/3] bisect: introduce first-parent flag
In reply to Aaron Lipmanbisect.c static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN") · line 464
Quoted 10 lines · Aaron Lipman
bisect.c +1
-0@@ -460,6 +460,7 @@ static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN") static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START") static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG") static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS") +static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT") static GIT_PATH_FUNC(git_path_head_name, "head-name")
This is not a new issue, but duplication of the above and the same set of PATH_FUNC in builtin/bisect-helper.c looks ugly. We may want to do something about it after this topic is done.
bisect.c void read_bisect_terms(const char **read_bad, const char **read_good) · line 985
Quoted 5 lines
bisect.c +1
-0@@ -981,6 +982,12 @@ void read_bisect_terms(const char **read_bad, const char **read_good)fclose (fp); } +int read_first_parent_option (void )
"static int", no? I do not think we need to be able to call this from anywhere outside this file.
Quoted 30 lines
bisect.c +7
-1+{ +const char *filename =git_path_bisect_first_parent (); +return !access (filename,F_OK ); +} +/* * We use the convention that return BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND (-10) means * the bisection process finished successfully. @@ -997,7 +1004,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, intenum bisect_error res =BISECT_OK ;struct object_id *bisect_rev;char *steps_msg; -int first_parent_only =0 ;/* TODO: pass --first-parent flag from git bisect start */ +int first_parent_only =read_first_parent_option ();read_bisect_terms (&term_bad, &term_good);if (read_bisect_refs ())@@ -1141,6 +1148,7 @@ int bisect_clean_state(void)unlink_or_warn (git_path_bisect_names ());unlink_or_warn (git_path_bisect_run ());unlink_or_warn (git_path_bisect_terms ()); +unlink_or_warn (git_path_bisect_first_parent ());/* Cleanup head-name if it got left by an old version of git-bisect */ unlink_or_warn (git_path_head_name ());/* bisect.h +0
-0
bisect.h int estimate_bisect_steps(int all); · line 70
Quoted 5 lines
bisect.h +2
-0@@ -66,6 +66,8 @@ int estimate_bisect_steps(int all);void read_bisect_terms (const char **bad,const char **good); +int read_first_parent_option (void ); +
So this won't be needed, right?
builtin/bisect--helper.c static int bisect_append_log_quoted(const char **argv) · line 426
Quoted 10 lines
builtin/bisect--helper.c +1
-1@@ -422,7 +423,7 @@ static int bisect_append_log_quoted(const char **argv) }static int bisect_start (struct bisect_terms *terms,int no_checkout, -const char **argv,int argc) +int first_parent_only,const char **argv,int argc)
Why do we need to pass this new parameter from cmd_bisect__helper(), when we are going to parse it out of argv/argc outselves anyway?
I suspect that you would ask the same question to whoever added no_checkout to this thing, and I wouldn't be surprised if we end up removing both of these parameters (and parsing of the options inside cmd_bisect__helper()) after thinking about them, but anyway, let's read on.
builtin/bisect--helper.c static int bisect_append_log_quoted(const char **argv) · line 428
Quoted 2 lines
builtin/bisect--helper.c +2
-0+# We want to automatically find the merge that +# introduced "line" into hello.
'introduced'?
builtin/bisect--helper.c static int bisect_append_log_quoted(const char **argv)
Quoted 2 lines
builtin/bisect--helper.c +2
-0+test_expect_success \ +'"git bisect run --first-parent" simple case' \
Let's not revert back to ancient line-folding style. The next test you can see in the postimage, i.e.
test_expect_success 'title in single quote' ' the first command && the second command && ... the last command '
is what we want to mimic.
builtin/bisect--helper.c
Quoted 4 lines
builtin/bisect--helper.c +4
-0+ 'echo "#"\!"/bin/sh" > test_script.sh && + echo "grep line hello > /dev/null" >> test_script.sh && + echo "test \$? -ne 0" >> test_script.sh && + chmod +x test_script.sh &&
So, we want to say "bad" if $? is 0, i.e. the file 'hello' has a string "line" in it and "good" if $? is not 0, i.e. the file 'hello' does not have such a line?
A few things:
- Use "write_script" to abstract away platform-specific details such as which $SHELL_PATH to use on "#!" line, and "chmod +x".
- There is no SP between a redirection operator and its target file.
i.e. Ssomething like this instead of the above four lines:
write_script test_script.sh <<-\EOF
! grep line hello >/dev/null
EOF
builtin/bisect--helper.c
Quoted 5 lines
builtin/bisect--helper.c +5
-0+ git bisect start --first-parent && + test_path_is_file".git/BISECT_FIRST_PARENT" && + git bisect good $HASH4 && + git bisect bad $B_HASH && + git bisect run ./test_script.sh > my_bisect_log.txt &&
Style: no SP between '>' and its target file.
builtin/bisect--helper.c
Quoted 3 lines
builtin/bisect--helper.c +3
-0+ grep"$B_HASH is the first bad commit" my_bisect_log.txt && + git bisect reset && + test_path_is_missing ".git/BISECT_FIRST_PARENT"'
The final blame must lie on a commit on the first-parent chain, which this test tries to ensure, but I wonder if it is also required that all commits offered to be tested by "git bisect" are on the first-parent chain, and if so, shouldn't we be make sure each and every time "test_script" is run, the commit that is at HEAD is on the first parent chain between the initial good..bad range?
builtin/bisect--helper.c
Quoted 3 lines
test_expect_success'good merge bases when good and bad are siblings' 'builtin/bisect--helper.c +0
-0git bisect start "$B_HASH" "$A_HASH" > my_bisect_log.txt && test_i18ngrep "merge base must be tested" my_bisect_log.txt &&
Other than that, overall this seems to be going in the right direction.
Thanks.
Re: [PATCH v2 3/3] bisect: combine args passed to find_bisection()
In reply to Aaron LipmanQuoted 3 lines · Aaron Lipman
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 inbisect.c
I'd rather call them "flags" without "bisect_". If we are passing two sets of flags, one set about "bisect" and the other set about something else, it would make quite a lot of sense to call the first set "bisect_flags", but what we are seeing here is not like that.
bisect.c enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int · line 1018
Quoted 6 lines
bisect.c +6
-0+if (read_first_parent_option ()) + bisect_flags |=BISECT_FIRST_PARENT ; + +if (!!skipped_revs.nr) + bisect_flags |=BISECT_FIND_ALL ; +
You do not care what kind of "true" you are feeding "if()", so I do not think you would want to keep !! prefix here. Older API into find_bisection() may wanted to say "pass 1 to find_all", in which case, normalizing with !! prefix may have made perfect sense, but that no longer holds here.
Review
Quoted 2 lines
(quoted diff) +2
-0+ + revs.first_parent_only = !!(bisect_flags & BISECT_FIRST_PARENT);
On the other hand, this !! may make sense; especially since .first_parent_only could just be a one-bit unsigned bitfield.
bisect.c struct commit_list *filter_skipped(struct commit_list *list, · line 27
Quoted 13 lines
bisect.c +3
-1revs.limited =1 ;bisect_common (&revs); -find_bisection (&revs.commits, &reaches, &all, !!skipped_revs.nr, first_parent_only); +find_bisection (&revs.commits, &reaches, &all, bisect_flags);@@ -23,6 +23,9 @@ struct commit_list *filter_skipped(struct commit_list *list,#define BISECT_SHOW_ALL (1<<0)#define REV_LIST_QUIET (1<<1) +#define BISECT_FIND_ALL (1u<<0) +#define BISECT_FIRST_PARENT (1u<<1)
The set of bits to go to your "bisect_flags" are only these two new ones, and the existing BISECT_SHOW_ALL does not belong to it (it is a bit in rev_list_info.flags), but it is not apparent. I wonder if there is a good way to help readers easily tell these two sets apart. These are flags passed to find_bisection(), so perhaps
#define FIND_BISECTION_ALL (1U<<0)
#define FIND_BISECTION_FIRST_PARENT_ONLY (1U<<1)
unsigned flags = 0;
if (first_parent)
flags |= FIND_BISECTION_FIRST_PARENT_ONLY;
...
find_bisection(..., flags);would be a reasonable compromise?
Thanks.
v3 cover Introduce --first-parent flag for git bisect
In reply to Aaron LipmanQuoted 1 line
Ah, I was referring to the order of sign-off, helped-by, etc.
Whoops, re-ordered correctly.
Quoted 3 lines
This is not a new issue, but duplication of the above and the same set of
PATH_FUNCinbuiltin/bisect-helper.clooks ugly. We may want to do something about it after this topic is done.
Happy to pick that up next!
Quoted 4 lines
Quoted 1 line
+int
read_first_parent_option(void)"static int", no? I do not think we need to be able to call this from anywhere outside this file.
Added static keyword and removed the redundant line from bisect.h
Quoted 4 lines
Review
Quoted 3 lines
static int bisect_start(struct
bisect_terms*terms, intno_checkout,(quoted diff) +1
-1- const char **argv, int argc) + int first_parent_only, const char **argv, int argc)
Quoted 8 lines
Why do we need to pass this new parameter from
cmd_bisect__helper(), when we are going to parse it out of argv/argc outselves anyway?I suspect that you would ask the same question to whoever added
no_checkoutto this thing, and I wouldn't be surprised if we end up removing both of these parameters (and parsing of the options insidecmd_bisect__helper()) after thinking about them, but anyway, let's read on.
Hmm. Is there a preferred way to to add a --first-parent line to the output of "git bisect start --help" via the parse-options API without removing the --first-parent option from argv in the process?
As long as we're capturing the --first-parent option in cmd_bisect__helper(), checking argv for a --first-parent flag in bisect_start() is pointless. So I've deleted the following lines from my patch inside bisect_start() (for the time being, at least):
git diff v2 v3 -- builtin/bisect--helper.c
- } else if (!strcmp(arg, "--first-parent")) {
- first_parent_only = 1;
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+# We want to automatically find the merge that +# introduced "line" into hello.'introduced'?
That's the language used by other "git bisect run" tests. If "introduced" sounds too clinical, perhaps "added" instead?
Quoted 1 line
Let's not revert back to ancient line-folding style.
Thank you for the template. I've updated my test accordingly.
Quoted 3 lines
So, we want to say "bad" if $? is 0, i.e. the file 'hello' has a string "line" in it and "good" if $? is not 0, i.e. the file 'hello' does not have such a line?
I think the other way around - an exit code of 0 from the script passed to "git bisect run" marks a commit as good, 1 as bad.
Quoted 5 lines
- Use "
write_script" to abstract away platform-specific details such as which $SHELL_PATH to use on "#!" line, and "chmod +x".- There is no SP between a redirection operator and its target file.
Noted. Can we rewrite the other "git bisect run" tests in t6030-bisect-porcelain.sh so that they're all consisent? That way, when someone adds another test for "git bisect run", they'll have a few more proper examples. (I've added a fourth commit that does this, if that's OK.)
Quoted 6 lines
The final blame must lie on a commit on the first-parent chain, which this test tries to ensure, but I wonder if it is also required that all commits offered to be tested by "git bisect" are on the first-parent chain, and if so, shouldn't we be make sure each and every time "
test_script" is run, the commit that is at HEAD is on the first parent chain between the initial good..bad range?
That is prudent. I've altered test_script.sh to use the special -1 exit code to abort the bisection process if it encounters a commit outside the first parent chain.
Althernatively, if we prefer not to depend on the special -1 exit code, we can append any tested commits outside the first parent chain to a file and ensure that it's empty after "git bisect run" finishes.
Quoted 4 lines
I'd rather call them "flags" without "
bisect_". If we are passing two sets of flags, one set about "bisect" and the other set about something else, it would make quite a lot of sense to call the first set "bisect_flags", but what we are seeing here is not like that.
bisect.c already uses a "flags" variable in several locations that would collide with this. Perhaps rename the existing "flags" variable to "commit_flags" to explicitly differentiate?
Quoted 6 lines
Review
Quoted 3 lines
+ if (!!skipped_revs.nr)
(quoted diff) +2
-0+ bisect_flags |= BISECT_FIND_ALL; +You do not care what kind of "true" you are feeding "
if()", so I do not think you would want to keep !! prefix here.
OK, removed.
Quoted 11 lines
The set of bits to go to your "
bisect_flags" are only these two new ones, and the existingBISECT_SHOW_ALLdoes not belong to it (it is a bit in rev_list_info.flags), but it is not apparent. I wonder if there is a good way to help readers easily tell these two sets apart. These are flags passed tofind_bisection(), so perhaps#define FIND_BISECTION_ALL (1U<<0) #define FIND_BISECTION_FIRST_PARENT_ONLY (1U<<1) // ...would be a reasonable compromise?
Sounds good, renamed.
Aaron Lipman (4):
rev-list: allow bisect and first-parent flags
bisect: introduce first-parent flag
bisect: combine args passed to find_bisection()
bisect: consistent style for git bisect run tests
Documentation/git-bisect.txt | 13 ++++- Documentation/rev-list-options.txt | 7 ++- bisect.c | 81 +++++++++++++++++++----------- bisect.h | 5 +- builtin/bisect--helper.c | 14 ++++-- builtin/rev-list.c | 9 +++- revision.c | 3 -- t/t6000-rev-list-misc.sh | 4 +- t/t6002-rev-list-bisect.sh | 45 +++++++++++++++++ t/t6030-bisect-porcelain.sh | 64 ++++++++++++++--------- 10 files changed, 176 insertions(+), 69 deletions(-)
Signature · 1 line
2.24.3 (Apple Git-128)
PATCH v3 1/4 rev-list: allow bisect and first-parent flags
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
--- Documentation/rev-list-options.txt | 7 ++--- bisect.c | 28 ++++++++++++------- bisect.h | 2 +- builtin/rev-list.c | 2 +- revision.c | 3 -- t/t6000-rev-list-misc.sh | 4 +-- t/t6002-rev-list-bisect.sh | 45 ++++++++++++++++++++++++++++++ 7 files changed, 70 insertions(+), 21 deletions(-)
Documentation/rev-list-options.txt +3 -4
@@ -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 adjusting to updated upstream from time to time, and this option allows you to ignore the individual commits - brought in to your history by such a merge. Cannot be - combined with --bisect. + brought in to your history by such a merge. --not:: Reverses the meaning of the '{caret}' prefix (or lack thereof)
@@ -207,7 +206,7 @@ ifndef::git-rev-list[] Pretend as if the bad bisection ref `refs/bisect/bad` was listed and as if it was followed by `--not` and the good bisection refs `refs/bisect/good-*` on the command - line. Cannot be combined with --first-parent. + line. endif::git-rev-list[] --stdin::
@@ -743,7 +742,7 @@ outputs 'midpoint', the output of the two commands would be of roughly the same length. Finding the change which introduces a regression is thus reduced to a binary search: repeatedly generate and test new 'midpoint's until the commit chain is of length -one. Cannot be combined with --first-parent. +one. --bisect-vars:: This calculates the same as `--bisect`, except that refs in
bisect.c +18 -10
@@ -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; }
@@ -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,6 +315,8 @@ static struct commit_list *do_find_bisection(struct commit_list *list,continue ;if (weight (p) !=-2 )continue ; +if (first_parent_only) +BUG ("shouldn't be calling count-distance in fp mode" );weight_set (p,count_distance (p));clear_distance (list);
@@ -332,7 +335,10 @@ static struct commit_list *do_find_bisection(struct commit_list *list,if (0 <=weight (p))continue ; -for (q = p->item->parents; q; q = q->next) { + +for (q = p->item->parents; + q; + q = first_parent_only ?NULL : q->next) {if (q->item->object.flags &UNINTERESTING )continue ;if (0 <=weight (q))
@@ -370,7 +376,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list, }void find_bisection (struct commit_list **commit_list,int *reaches, -int *all,int find_all) +int *all,int find_all,int first_parent_only) {int nr, on_list;struct commit_list *list, *p, *best, *next, *last;
@@ -406,7 +412,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, first_parent_only);if (best) {if (!find_all) { list->item = best->item;
@@ -991,6 +997,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, intenum bisect_error res =BISECT_OK ;struct object_id *bisect_rev;char *steps_msg; +int first_parent_only =0 ;/* TODO: pass --first-parent flag from git bisect start */ read_bisect_terms (&term_bad, &term_good);if (read_bisect_refs ())
@@ -1001,11 +1008,12 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, intreturn res;bisect_rev_setup (r, &revs, prefix,"%s" ,"^%s" ,1 ); + revs.first_parent_only = first_parent_only; revs.limited =1 ;bisect_common (&revs); -find_bisection (&revs.commits, &reaches, &all, !!skipped_revs.nr); +find_bisection (&revs.commits, &reaches, &all, !!skipped_revs.nr, first_parent_only); revs.commits =managed_skipped (revs.commits, &tried);if (!revs.commits) {
bisect.h +1 -1
@@ -12,7 +12,7 @@ struct repository;* best commit, as chosen by `find_all`. */ void find_bisection (struct commit_list **list,int *reaches,int *all, -int find_all); +int find_all,int first_parent_only); struct commit_list *filter_skipped(struct commit_list *list, struct commit_list **tried,
builtin/rev-list.c +1 -1
@@ -638,7 +638,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, revs.first_parent_only);if (bisect_show_vars)return show_bisect_vars (&info, reaches, all);
revision.c +0 -3
@@ -2869,9 +2869,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct sif (!revs->reflog_info && revs->grep_filter.use_reflog_filter)die ("cannot use --grep-reflog without --walk-reflogs" ); -if (revs->first_parent_only && revs->bisect) -die (_ ("--first-parent is incompatible with --bisect" )); -if (revs->line_level_traverse && (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH |DIFF_FORMAT_NO_OUTPUT )))die (_ ("-L does not yet support diff formats besides -p and -s" ));
t/t6000-rev-list-misc.sh +2 -2
@@ -125,8 +125,8 @@ test_expect_success 'rev-list can negate index objects' ' test_cmp expect actual ' -test_expect_success '--bisect and --first-parent can not be combined' ' - test_must_fail git rev-list --bisect --first-parent HEAD +test_expect_success '--bisect and --first-parent can be combined' ' + git rev-list --bisect --first-parent HEAD ' test_expect_success '--header shows a NUL after each commit' '
t/t6002-rev-list-bisect.sh +45 -0
49 lines not on this page · raw message
Signature · 1 line
2.24.3 (Apple Git-128)
PATCH v3 3/4 bisect: combine args passed to find_bisection()
In reply to Aaron LipmanNow 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
Also, rename existing the "flags" bitfield to "commit_flags", to explicitly differentiate between the new "bisect_flags" bitfield.
--- bisect.c | 67 +++++++++++++++++++++++++--------------------- bisect.h | 5 +++- builtin/rev-list.c | 9 ++++++- 3 files changed, 49 insertions(+), 32 deletions(-)
bisect.c +37 -30
@@ -88,7 +88,7 @@ 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,int first_parent_only) +static int count_interesting_parents (struct commit *commit,unsigned bisect_flags) {struct commit_list *p;int count;
@@ -96,7 +96,7 @@ static int count_interesting_parents(struct commit *commit, int first_parent_onlfor (count =0 , p = commit->parents; p; p = p->next) {if (!(p->item->object.flags &UNINTERESTING )) count++; -if (first_parent_only) +if (bisect_flags &FIND_BISECTION_FIRST_PARENT_ONLY )break ; }return count;
@@ -136,7 +136,7 @@ static void show_list(const char *debug, int counted, int nr,for (p = list; p; p = p->next) {struct commit_list *pp;struct commit *commit = p->item; -unsigned flags = commit->object.flags; +unsigned commit_flags = commit->object.flags;enum object_type type;unsigned long size; char *buf = read_object_file(&commit->object.oid, &type,
@@ -145,9 +145,9 @@ static void show_list(const char *debug, int counted, int nr,int subject_len;fprintf (stderr,"%c%c%c " , - (flags &TREESAME ) ?' ' :'T' , - (flags &UNINTERESTING ) ?'U' :' ' , - (flags &COUNTED ) ?'C' :' ' ); + (commit_flags &TREESAME ) ?' ' :'T' , + (commit_flags &UNINTERESTING ) ?'U' :' ' , + (commit_flags &COUNTED ) ?'C' :' ' );if (*commit_weight_at (&commit_weight, p->item))fprintf (stderr,"%3d" ,weight (p));else
@@ -172,9 +172,9 @@ static struct commit_list *best_bisection(struct commit_list *list, int nr) best = list;for (p = list; p; p = p->next) {int distance; -unsigned flags = p->item->object.flags; +unsigned commit_flags = p->item->object.flags; -if (flags &TREESAME ) +if (commit_flags &TREESAME )continue ; distance =weight (p);if (nr - distance < distance)
@@ -213,9 +213,9 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int nfor (p = list, cnt =0 ; p; p = p->next) {int distance; -unsigned flags = p->item->object.flags; +unsigned commit_flags = p->item->object.flags; -if (flags &TREESAME ) +if (commit_flags &TREESAME )continue ; distance =weight (p);if (nr - distance < distance)
@@ -260,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 first_parent_only) +unsigned bisect_flags) {int n, counted;struct commit_list *p;
@@ -269,12 +269,12 @@ static struct commit_list *do_find_bisection(struct commit_list *list,for (n =0 , p = list; p; p = p->next) {struct commit *commit = p->item; -unsigned flags = commit->object.flags; +unsigned commit_flags = commit->object.flags; *commit_weight_at (&commit_weight, p->item) = &weights[n++]; -switch (count_interesting_parents (commit, first_parent_only)) { +switch (count_interesting_parents (commit, bisect_flags)) {case 0 : -if (!(flags &TREESAME )) { +if (!(commit_flags &TREESAME )) {weight_set (p,1 ); counted++; show_list("bisection 2 count one",
@@ -315,13 +315,13 @@ static struct commit_list *do_find_bisection(struct commit_list *list,continue ;if (weight (p) !=-2 )continue ; -if (first_parent_only) +if (bisect_flags &FIND_BISECTION_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? */ -if (!find_all &&halfway (p, nr)) +if (!(bisect_flags &FIND_BISECTION_ALL ) &&halfway (p, nr))return p; counted++; }
@@ -331,14 +331,14 @@ static struct commit_list *do_find_bisection(struct commit_list *list,while (counted < nr) {for (p = list; p; p = p->next) {struct commit_list *q; -unsigned flags = p->item->object.flags; +unsigned commit_flags = p->item->object.flags;if (0 <=weight (p))continue ;for (q = p->item->parents; q; - q = first_parent_only ?NULL : q->next) { + q = bisect_flags &FIND_BISECTION_FIRST_PARENT_ONLY ?NULL : q->next) {if (q->item->object.flags &UNINTERESTING )continue ;if (0 <=weight (q))
@@ -352,7 +352,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,* add one for p itself if p is to be counted, * otherwise inherit it from q directly. */ -if (!(flags &TREESAME )) { +if (!(commit_flags &TREESAME )) {weight_set (p,weight (q)+1 ); counted++; show_list("bisection 2 count one",
@@ -362,21 +362,21 @@ static struct commit_list *do_find_bisection(struct commit_list *list,weight_set (p,weight (q));/* Does it happen to be at exactly half-way? */ -if (!find_all &&halfway (p, nr)) +if (!(bisect_flags &FIND_BISECTION_ALL ) &&halfway (p, nr))return p; } }show_list ("bisection 2 counted all" , counted, nr, list); -if (!find_all) +if (!(bisect_flags &FIND_BISECTION_ALL ))return best_bisection (list, nr);else return best_bisection_sorted (list, nr); }void find_bisection (struct commit_list **commit_list,int *reaches, -int *all,int find_all,int first_parent_only) +int *all,unsigned bisect_flags) {int nr, on_list;struct commit_list *list, *p, *best, *next, *last;
@@ -392,16 +392,16 @@ void find_bisection(struct commit_list **commit_list, int *reaches,for (nr = on_list =0 , last =NULL , p = *commit_list; p; p = next) { -unsigned flags = p->item->object.flags; +unsigned commit_flags = p->item->object.flags; next = p->next; -if (flags &UNINTERESTING ) { +if (commit_flags &UNINTERESTING ) {free (p);continue ; } p->next = last; last = p; -if (!(flags &TREESAME )) +if (!(commit_flags &TREESAME )) nr++; on_list++; }
@@ -412,9 +412,9 @@ 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, first_parent_only); + best =do_find_bisection (list, nr, weights, bisect_flags);if (best) { -if (!find_all) { +if (!(bisect_flags &FIND_BISECTION_ALL )) { list->item = best->item;free_commit_list (list->next); best = list;
@@ -1004,23 +1004,30 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, intenum bisect_error res =BISECT_OK ;struct object_id *bisect_rev;char *steps_msg; -int first_parent_only =read_first_parent_option (); +unsigned bisect_flags =0 ;read_bisect_terms (&term_bad, &term_good);if (read_bisect_refs ())die (_ ("reading bisect refs failed" )); +if (read_first_parent_option ()) + bisect_flags |=FIND_BISECTION_FIRST_PARENT_ONLY ; + +if (skipped_revs.nr) + bisect_flags |=FIND_BISECTION_ALL ; + res =check_good_are_ancestors_of_bad (r, prefix, no_checkout);if (res)return res;bisect_rev_setup (r, &revs, prefix,"%s" ,"^%s" ,1 ); - revs.first_parent_only = first_parent_only; + + revs.first_parent_only = !!(bisect_flags &FIND_BISECTION_FIRST_PARENT_ONLY ); revs.limited =1 ;bisect_common (&revs); -find_bisection (&revs.commits, &reaches, &all, !!skipped_revs.nr, first_parent_only); +find_bisection (&revs.commits, &reaches, &all, bisect_flags); revs.commits =managed_skipped (revs.commits, &tried);if (!revs.commits) {
bisect.h +4 -1
@@ -12,7 +12,7 @@ struct repository;* best commit, as chosen by `find_all`. */ void find_bisection (struct commit_list **list,int *reaches,int *all, -int find_all,int first_parent_only); +unsigned bisect_flags); struct commit_list *filter_skipped(struct commit_list *list, struct commit_list **tried,
@@ -23,6 +23,9 @@ struct commit_list *filter_skipped(struct commit_list *list,#define BISECT_SHOW_ALL (1<<0)#define REV_LIST_QUIET (1<<1) +#define FIND_BISECTION_ALL (1u<<0) +#define FIND_BISECTION_FIRST_PARENT_ONLY (1u<<1) +struct rev_list_info {struct rev_info *revs;int flags;
builtin/rev-list.c +8 -1
@@ -637,8 +637,15 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)if (bisect_list) {int reaches, all; +unsigned bisect_flags =0 ; -find_bisection (&revs.commits, &reaches, &all, bisect_find_all, revs.first_parent_only); +if (bisect_find_all) + bisect_flags |=FIND_BISECTION_ALL ; + +if (revs.first_parent_only) + bisect_flags |=FIND_BISECTION_FIRST_PARENT_ONLY ; + +find_bisection (&revs.commits, &reaches, &all, bisect_flags);if (bisect_show_vars)return show_bisect_vars (&info, reaches, all);
Signature · 1 line
2.24.3 (Apple Git-128)
PATCH v3 2/4 bisect: introduce first-parent flag
In reply to Aaron LipmanUpon 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.
--- Documentation/git-bisect.txt | 13 ++++++++++++- bisect.c | 10 +++++++++- builtin/bisect--helper.c | 14 ++++++++++---- t/t6030-bisect-porcelain.sh | 18 ++++++++++++++++++ 4 files changed, 49 insertions(+), 6 deletions(-)
Documentation/git-bisect.txt +12 -1
@@ -17,7 +17,7 @@ The command takes various subcommands, and different options depending on the subcommand: git bisect start [--term-{old,good}=<term> --term-{new,bad}=<term>] - [--no-checkout] [<bad> [<good>...]] [--] [<paths>...] + [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...] git bisect (bad|new|<term-new>) [<rev>] git bisect (good|old|<term-old>) [<rev>...] git bisect terms [--term-good | --term-bad]
@@ -365,6 +365,17 @@ does not require a checked out tree. + If the repository is bare, `--no-checkout` is assumed. +--first-parent:: ++ +Follow only the first parent commit upon seeing a merge commit. ++ +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. + EXAMPLES --------
bisect.c +9 -1
@@ -460,6 +460,7 @@ static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN") static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START") static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG") static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS") +static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT") static GIT_PATH_FUNC(git_path_head_name, "head-name") static void read_bisect_paths(struct argv_array *array)
@@ -981,6 +982,12 @@ void read_bisect_terms(const char **read_bad, const char **read_good)fclose (fp); } +static int read_first_parent_option (void ) +{ +const char *filename =git_path_bisect_first_parent (); +return !access (filename,F_OK ); +} +/* * We use the convention that return BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND (-10) means * the bisection process finished successfully.
@@ -997,7 +1004,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, intenum bisect_error res =BISECT_OK ;struct object_id *bisect_rev;char *steps_msg; -int first_parent_only =0 ;/* TODO: pass --first-parent flag from git bisect start */ +int first_parent_only =read_first_parent_option ();read_bisect_terms (&term_bad, &term_good);if (read_bisect_refs ())
@@ -1141,6 +1148,7 @@ int bisect_clean_state(void)unlink_or_warn (git_path_bisect_names ());unlink_or_warn (git_path_bisect_run ());unlink_or_warn (git_path_bisect_terms ()); +unlink_or_warn (git_path_bisect_first_parent ());/* Cleanup head-name if it got left by an old version of git-bisect */ unlink_or_warn (git_path_head_name ());/*
builtin/bisect--helper.c +10 -4
@@ -17,6 +17,7 @@ static GIT_PATH_FUNC(git_path_bisect_head, "BISECT_HEAD") static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG") static GIT_PATH_FUNC(git_path_head_name, "head-name") static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES") +static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT") static const char * const git_bisect_helper_usage[] = {N_ ("git bisect--helper --next-all [--no-checkout]" ),
@@ -28,7 +29,7 @@ static const char * const git_bisect_helper_usage[] = {N_ ("git bisect--helper --bisect-next-check <good_term> <bad_term> [<term>]" ),N_ ("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]" ),N_ ("git bisect--helper --bisect-start [--term-{old,good}=<term> --term-{new,bad}=<term>]" -"[--no-checkout] [<bad> [<good>...]] [--] [<paths>...]" ), +" [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]" ), NULL };
@@ -422,7 +423,7 @@ static int bisect_append_log_quoted(const char **argv) }static int bisect_start (struct bisect_terms *terms,int no_checkout, -const char **argv,int argc) +int first_parent_only,const char **argv,int argc) {int i, has_double_dash =0 , must_write_terms =0 , bad_seen =0 ;int flags, pathspec_pos, res =0 ;
@@ -577,6 +578,9 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,*/ write_file (git_path_bisect_start (),"%s\n" , start_head.buf); +if (first_parent_only) +write_file (git_path_bisect_first_parent (),"\n" ); +if (no_checkout) {if (get_oid (start_head.buf, &oid) <0 ) { res =error (_ ("invalid ref: '%s'" ), start_head.buf);
@@ -632,7 +636,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)BISECT_TERMS , BISECT_START } cmdmode =0 ; -int no_checkout =0 , res =0 , nolog =0 ; +int no_checkout =0 , first_parent_only =0 , res =0 , nolog =0 ;struct option options[] = {OPT_CMDMODE (0 ,"next-all" , &cmdmode,N_ ("perform 'git bisect next'" ),NEXT_ALL ),
@@ -656,6 +660,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) N_("start the bisect session"), BISECT_START),OPT_BOOL (0 ,"no-checkout" , &no_checkout,N_ ("update BISECT_HEAD instead of checking out the current commit" )), +OPT_BOOL (0 ,"first-parent" , &first_parent_only, +N_ ("only trace the first parent of merge commits" )),OPT_BOOL (0 ,"no-log" , &nolog,N_ ("no log for BISECT_WRITE" )),OPT_END ()
@@ -713,7 +719,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)break ;case BISECT_START :set_terms (&terms,"bad" ,"good" ); - res =bisect_start (&terms, no_checkout, argv, argc); + res =bisect_start (&terms, no_checkout, first_parent_only, argv, argc);break ;default :return error ("BUG: unknown subcommand '%d'" , cmdmode);
t/t6030-bisect-porcelain.sh +18 -0
@@ -458,6 +458,24 @@ test_expect_success 'many merge bases creation' ' grep "$SIDE_HASH5" merge_bases.txt ' +# We want to automatically find the merge that +# added "line" into hello. +test_expect_success '"git bisect run --first-parent" simple case' ' + git rev-list --first-parent $B_HASH ^$HASH4 >first_parent_chain.txt && + write_script test_script.sh <<-\EOF && + grep $(git rev-parse HEAD) first_parent_chain.txt || exit -1 + ! grep line hello >/dev/null + EOF + git bisect start --first-parent && + test_path_is_file ".git/BISECT_FIRST_PARENT" && + git bisect good $HASH4 && + git bisect bad $B_HASH && + git bisect run ./test_script.sh >my_bisect_log.txt && + grep "$B_HASH is the first bad commit" my_bisect_log.txt && + git bisect reset && + test_path_is_missing .git/BISECT_FIRST_PARENT +' + test_expect_success 'good merge bases when good and bad are siblings' ' git bisect start "$B_HASH" "$A_HASH" > my_bisect_log.txt && test_i18ngrep "merge base must be tested" my_bisect_log.txt &&
Signature · 1 line
2.24.3 (Apple Git-128)
PATCH v3 4/4 bisect: consistent style for git bisect run tests
In reply to Aaron LipmanEnforce consistent styling for tests on "git bisect run":
- Use "write_script" to abstract away platform-specific details.
- Favor current whitespace conventions.
--- t/t6030-bisect-porcelain.sh | 46 ++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 24 deletions(-)
t/t6030-bisect-porcelain.sh +22 -24
@@ -243,32 +243,30 @@ test_expect_success 'bisect skip: with commit both bad and skipped' ' ' # We want to automatically find the commit that -# introduced "Another" into hello. -test_expect_success \ - '"git bisect run" simple case' \ - 'echo "#"\!"/bin/sh" > test_script.sh && - echo "grep Another hello > /dev/null" >> test_script.sh && - echo "test \$? -ne 0" >> test_script.sh && - chmod +x test_script.sh && - git bisect start && - git bisect good $HASH1 && - git bisect bad $HASH4 && - git bisect run ./test_script.sh > my_bisect_log.txt && - grep "$HASH3 is the first bad commit" my_bisect_log.txt && - git bisect reset' +# added "Another" into hello. +test_expect_success '"git bisect run" simple case' ' + write_script test_script.sh <<-\EOF && + ! grep Another hello >/dev/null + EOF + git bisect start && + git bisect good $HASH1 && + git bisect bad $HASH4 && + git bisect run ./test_script.sh >my_bisect_log.txt && + grep "$HASH3 is the first bad commit" my_bisect_log.txt && + git bisect reset +' # We want to automatically find the commit that -# introduced "Ciao" into hello. -test_expect_success \ - '"git bisect run" with more complex "git bisect start"' \ - 'echo "#"\!"/bin/sh" > test_script.sh && - echo "grep Ciao hello > /dev/null" >> test_script.sh && - echo "test \$? -ne 0" >> test_script.sh && - chmod +x test_script.sh && - git bisect start $HASH4 $HASH1 && - git bisect run ./test_script.sh > my_bisect_log.txt && - grep "$HASH4 is the first bad commit" my_bisect_log.txt && - git bisect reset' +# added "Ciao" into hello. +test_expect_success '"git bisect run" with more complex "git bisect start"' ' + write_script test_script.sh <<-\EOF && + ! grep Ciao hello >/dev/null + EOF + git bisect start $HASH4 $HASH1 && + git bisect run ./test_script.sh >my_bisect_log.txt && + grep "$HASH4 is the first bad commit" my_bisect_log.txt && + git bisect reset +' # $HASH1 is good, $HASH5 is bad, we skip $HASH3 # but $HASH4 is good,
Signature · 1 line
2.24.3 (Apple Git-128)
Quoted 9 lines · Aaron Lipman
Quoted 5 lines
I suspect that you would ask the same question to whoever added
no_checkoutto this thing, and I wouldn't be surprised if we end up removing both of these parameters (and parsing of the options insidecmd_bisect__helper()) after thinking about them, but anyway, let's read on.Hmm. Is there a preferred way to to add a --first-parent line to the output of "git bisect start --help" via the parse-options API without removing the --first-parent option from argv in the process?
I'd rather not to see the code made ugly if the only reason why you have duplicated command line parsing is to support "git bisect start -h" while cmd_bisect__helper() still exists.
That function is supposed to be a thin shim layer whose only reason of its existence is to serve as an interface with the scripted version of "git bisect". When everything is migrated from the shell script, cmd_bisect__helper() should disappear.
In its place, cmd_bisect() written in C would become a dispatcher that only looks at the first token on the command line that comes after ["git", "bisect"] and calls "bisect_start()", "bisect_next()", etc. with the remainder of the command line with options such as "--first-parent" and "--no-checkout", which will be parsed by bisect_start() etc. parsing its argc/argv[] passed by cmd_bisect(). It is likely that cmd_bisect() would not have any call to parse_options(); instead cmd_start() etc. would call parse_options() with their own option[] table.
So I am not sure if the change between v2 and v3 is going in the right direction.
Quoted 3 lines
As long as we're capturing the --first-parent option in
cmd_bisect__helper(), checking argv for a --first-parent flag inbisect_start()is pointless.
This is going backwards, as far as I can tell. If anything, I'd rather see cmd_bisect__helper() get fixed so that it does not parse "--first-parent" (and similarly, "--no-checkout" that you imitated) into first_parent_only (and no_checkout) variables and passed as parameters to bisect_start(). cmd_bisect__helper() should instead just let these flags (and there may be others) sit in argc/argv[] and have bisect_start() parse them, together with all other options bisect_start() already has its parser for.
Thanks.
Re: [PATCH v3 4/4] bisect: consistent style for git bisect run tests
In reply to Aaron LipmanQuoted 1 line · Aaron Lipman
Subject: bisect: consistent style for git bisect run tests
Nit: this could be 't6030: modernize "git bisect run" tests' to tighten the area a bit and to begin with a verb.
Quoted 4 lines
Enforce consistent styling for tests on "git bisect run":
- Use "write_script" to abstract away platform-specific details.
- Favor current whitespace conventions.
---
Missing sign-off.
I think this should come earlier in the series, probably already as patch 1/4. Some reasons for that:
* If this effort doesn't go any further (let's hope it does!), at least the initial cleanup can be merged, so that later endeavours can find a slightly cleaner table to start with.
* Revisiting your other patches in git log after they've been merged, it will be obvious that they follow other tests, so one can focus on what they do and not on how they differ.
* Let's first tidy up, then build shiny new things. ;-)
Quoted 5 lines
# We want to automatically find the commit that
(quoted diff) +0
-4-# introduced "Another" into hello. -test_expect_success \ - '"git bisect run" simple case' \ - 'echo "#"\!"/bin/sh" > test_script.sh &&
[...]
Review
Quoted 3 lines
(quoted diff) +3
-0+# added "Another" into hello. +test_expect_success '"git bisect run" simple case' ' + write_script test_script.sh <<-\EOF &&
[...]
All of these look like correct, worthwhile modifications. The s/introduced/added/ changes aren't mentioned in the proposed log message. They have been discussed up-thread, but it might still be good to add something like
While at it, change "introduced" to "added" in the comments to make them read better.
FWIW, I guess there's some minor added value in doing that change and since you're doing other changes anyway, it might actually be worth the churn.
You change two tests like this, but there are other "git bisect run" tests that look like they're in need of some modernizing. Looking for "chmod" I spotted the ones I'm tweaking below. Looking for "git bisect run" you might find more that may contain some funny indentation. I'll leave it to you where to stop going down that rabbit-hole but if you do re-roll, it might make sense to squash in the diff below, maybe with some modifications to the commit message. (Signed-off-by: Martin Ågren <[email protected]>, FWIW.)
Martin
-- >8 --
t/t6030-bisect-porcelain.sh +16 -24
@@ -293,24 +293,17 @@ HASH6= test_expect_success 'bisect run & skip: cannot tell between 2' ' add_line_into_file "6: Yet a line." hello && HASH6=$(git rev-parse --verify HEAD) && - echo "#"\!"/bin/sh" > test_script.sh && - echo "sed -ne \\\$p hello | grep Ciao > /dev/null && exit 125" >> test_script.sh && - echo "grep line hello > /dev/null" >> test_script.sh && - echo "test \$? -ne 0" >> test_script.sh && - chmod +x test_script.sh && + write_script test_script.sh <<-\EOF && + sed -ne \$p hello | grep Ciao >/dev/null && exit 125 + ! grep line hello >/dev/null + EOF git bisect start $HASH6 $HASH1 && - if git bisect run ./test_script.sh > my_bisect_log.txt - then - echo Oops, should have failed. - false - else - test $? -eq 2 && - grep "first bad commit could be any of" my_bisect_log.txt && - ! grep $HASH3 my_bisect_log.txt && - ! grep $HASH6 my_bisect_log.txt && - grep $HASH4 my_bisect_log.txt && - grep $HASH5 my_bisect_log.txt - fi + test_expect_code 2 git bisect run ./test_script.sh >my_bisect_log.txt && + grep "first bad commit could be any of" my_bisect_log.txt && + ! grep $HASH3 my_bisect_log.txt && + ! grep $HASH6 my_bisect_log.txt && + grep $HASH4 my_bisect_log.txt && + grep $HASH5 my_bisect_log.txt ' HASH7=
@@ -318,14 +311,13 @@ test_expect_success 'bisect run & skip: find first bad' ' git bisect reset && add_line_into_file "7: Should be the last line." hello && HASH7=$(git rev-parse --verify HEAD) && - echo "#"\!"/bin/sh" > test_script.sh && - echo "sed -ne \\\$p hello | grep Ciao > /dev/null && exit 125" >> test_script.sh && - echo "sed -ne \\\$p hello | grep day > /dev/null && exit 125" >> test_script.sh && - echo "grep Yet hello > /dev/null" >> test_script.sh && - echo "test \$? -ne 0" >> test_script.sh && - chmod +x test_script.sh && + write_script test_script.sh <<-\EOF && + sed -ne \$p hello | grep Ciao >/dev/null && exit 125 + sed -ne \$p hello | grep day >/dev/null && exit 125 + ! grep Yet hello >/dev/null + EOF git bisect start $HASH7 $HASH1 && - git bisect run ./test_script.sh > my_bisect_log.txt && + git bisect run ./test_script.sh >my_bisect_log.txt && grep "$HASH6 is the first bad commit" my_bisect_log.txt '
Signature · 1 line
2.28.0.81.ge8ab941b67
Re: [PATCH v3 3/4] bisect: combine args passed to find_bisection()
In reply to Aaron LipmanQuoted 2 lines · Aaron Lipman
Also, rename existing the "flags" bitfield to "
commit_flags", to explicitly differentiate between the new "bisect_flags" bitfield.
s/existing the/the existing/
Also, does "separate between <one thing>" make grammatical sense? Should it be "separate between <two things>" or "separate [it] from <one thing>"? Dunno..
Martin
v4 cover Introduce --first-parent flag for git bisect
In reply to Aaron LipmanOK, here's take 4! Responding to Junio's feedback, first:
Quoted 10 lines
That function [
cmd_bisect__helper()] is supposed to be a thin shim layer whose only reason of its existence is to serve as an interface with the scripted version of "git bisect". When everything is migrated from the shell script,cmd_bisect__helper()should disappear.
...
This is going backwards, as far as I can tell. If anything, I'd rather seecmd_bisect__helper()get fixed so that it does not parse "--first-parent" (and similarly, "--no-checkout" that you imitated) intofirst_parent_only(andno_checkout) variables and passed as parameters tobisect_start().
Thanks for the explanation, Junio. (And for bearing with me while I gain familiarity with git's codebase.) Now that I've taken some time to examine how git-bisect.sh and cmd_bisect__helper() fit together, the correct approach is much more clear.
I've added a commit (3/5 in this iteration) that updates cmd_bisect__helper() to defer parsing the --no-checkout flag.
As no_checkout was also passed to bisect_next_all() [via implicitly checking for the existence of .git/BISECT_HEAD when calling bisect_next() in git-bisect.sh], I've removed that parameter and instead check for .git/BISECT_HEAD in bisect_next_all() to determine whether no-checkout mode is on.
This means no-checkout mode can no longer be enabled by "git bisect--helper --next-all --no-checkout" - it can now only be turned on when running "git bisect start". But this doesn't change git bisect's public API, so this should be OK.
Other than the changes suggested by Martin (summarized below), the only new change is removing the read_first_parent_option() wrapper function in favor of file_exists() from dir.h.
---
Martin, thanks for your suggestions - I've moved the commit updating "git bisect run" tests to 1/5, updated the commit message, and included the changes you provided. (I held off on additional indentation corrections, as it kinda felt like unnecessary code churn/outside the scope of abstracting platform-specific details.)
Quoted 3 lines
Also, does "separate between <one thing>" make grammatical sense? Should it be "separate between <two things>" or "separate [it] from <one thing>"? Dunno..
I think "separate [it] from <one thing>" is correct, good catch!
Quoted 1 line
(Signed-off-by: Martin Ågren <[email protected]>, FWIW.)
I'm still getting used to the conventions - should I add your name as a signed-off-by tag, a thanks-to tag, or both?
Aaron Lipman (5):
t6030: modernize "git bisect run" tests
rev-list: allow bisect and first-parent flags
cmd_bisect__helper: defer parsing no-checkout flag
bisect: introduce first-parent flag
bisect: combine args passed to find_bisection()
Documentation/git-bisect.txt | 13 +++- Documentation/rev-list-options.txt | 7 +- bisect.c | 79 +++++++++++++--------- bisect.h | 9 +-- builtin/bisect--helper.c | 23 ++++--- builtin/rev-list.c | 9 ++- git-bisect.sh | 2 +- revision.c | 3 - t/t6000-rev-list-misc.sh | 4 +- t/t6002-rev-list-bisect.sh | 45 +++++++++++++ t/t6030-bisect-porcelain.sh | 104 ++++++++++++++++------------- 11 files changed, 195 insertions(+), 103 deletions(-)
Signature · 1 line
2.24.3 (Apple Git-128)
PATCH v4 1/5 t6030: modernize "git bisect run" tests
Enforce consistent styling for tests on "git bisect run":
- Use "write_script" to abstract away platform-specific details.
- Favor current whitespace conventions.
- While at it, change "introduced" to "added" in the comments to make them read better.
Thanks-to: Martin Ågren <[email protected]>
--- t/t6030-bisect-porcelain.sh | 86 ++++++++++++++++--------------------- 1 file changed, 38 insertions(+), 48 deletions(-)
t/t6030-bisect-porcelain.sh +38 -48
@@ -243,32 +243,30 @@ test_expect_success 'bisect skip: with commit both bad and skipped' ' ' # We want to automatically find the commit that -# introduced "Another" into hello. -test_expect_success \ - '"git bisect run" simple case' \ - 'echo "#"\!"/bin/sh" > test_script.sh && - echo "grep Another hello > /dev/null" >> test_script.sh && - echo "test \$? -ne 0" >> test_script.sh && - chmod +x test_script.sh && - git bisect start && - git bisect good $HASH1 && - git bisect bad $HASH4 && - git bisect run ./test_script.sh > my_bisect_log.txt && - grep "$HASH3 is the first bad commit" my_bisect_log.txt && - git bisect reset' +# added "Another" into hello. +test_expect_success '"git bisect run" simple case' ' + write_script test_script.sh <<-\EOF && + ! grep Another hello >/dev/null + EOF + git bisect start && + git bisect good $HASH1 && + git bisect bad $HASH4 && + git bisect run ./test_script.sh >my_bisect_log.txt && + grep "$HASH3 is the first bad commit" my_bisect_log.txt && + git bisect reset +' # We want to automatically find the commit that -# introduced "Ciao" into hello. -test_expect_success \ - '"git bisect run" with more complex "git bisect start"' \ - 'echo "#"\!"/bin/sh" > test_script.sh && - echo "grep Ciao hello > /dev/null" >> test_script.sh && - echo "test \$? -ne 0" >> test_script.sh && - chmod +x test_script.sh && - git bisect start $HASH4 $HASH1 && - git bisect run ./test_script.sh > my_bisect_log.txt && - grep "$HASH4 is the first bad commit" my_bisect_log.txt && - git bisect reset' +# added "Ciao" into hello. +test_expect_success '"git bisect run" with more complex "git bisect start"' ' + write_script test_script.sh <<-\EOF && + ! grep Ciao hello >/dev/null + EOF + git bisect start $HASH4 $HASH1 && + git bisect run ./test_script.sh >my_bisect_log.txt && + grep "$HASH4 is the first bad commit" my_bisect_log.txt && + git bisect reset +' # $HASH1 is good, $HASH5 is bad, we skip $HASH3 # but $HASH4 is good,
@@ -295,24 +293,17 @@ HASH6= test_expect_success 'bisect run & skip: cannot tell between 2' ' add_line_into_file "6: Yet a line." hello && HASH6=$(git rev-parse --verify HEAD) && - echo "#"\!"/bin/sh" > test_script.sh && - echo "sed -ne \\\$p hello | grep Ciao > /dev/null && exit 125" >> test_script.sh && - echo "grep line hello > /dev/null" >> test_script.sh && - echo "test \$? -ne 0" >> test_script.sh && - chmod +x test_script.sh && + write_script test_script.sh <<-\EOF && + sed -ne \$p hello | grep Ciao >/dev/null && exit 125 + ! grep line hello >/dev/null + EOF git bisect start $HASH6 $HASH1 && - if git bisect run ./test_script.sh > my_bisect_log.txt - then - echo Oops, should have failed. - false - else - test $? -eq 2 && - grep "first bad commit could be any of" my_bisect_log.txt && - ! grep $HASH3 my_bisect_log.txt && - ! grep $HASH6 my_bisect_log.txt && - grep $HASH4 my_bisect_log.txt && - grep $HASH5 my_bisect_log.txt - fi + test_expect_code 2 git bisect run ./test_script.sh >my_bisect_log.txt && + grep "first bad commit could be any of" my_bisect_log.txt && + ! grep $HASH3 my_bisect_log.txt && + ! grep $HASH6 my_bisect_log.txt && + grep $HASH4 my_bisect_log.txt && + grep $HASH5 my_bisect_log.txt ' HASH7=
@@ -320,14 +311,13 @@ test_expect_success 'bisect run & skip: find first bad' ' git bisect reset && add_line_into_file "7: Should be the last line." hello && HASH7=$(git rev-parse --verify HEAD) && - echo "#"\!"/bin/sh" > test_script.sh && - echo "sed -ne \\\$p hello | grep Ciao > /dev/null && exit 125" >> test_script.sh && - echo "sed -ne \\\$p hello | grep day > /dev/null && exit 125" >> test_script.sh && - echo "grep Yet hello > /dev/null" >> test_script.sh && - echo "test \$? -ne 0" >> test_script.sh && - chmod +x test_script.sh && + write_script test_script.sh <<-\EOF && + sed -ne \$p hello | grep Ciao >/dev/null && exit 125 + sed -ne \$p hello | grep day >/dev/null && exit 125 + ! grep Yet hello >/dev/null + EOF git bisect start $HASH7 $HASH1 && - git bisect run ./test_script.sh > my_bisect_log.txt && + git bisect run ./test_script.sh >my_bisect_log.txt && grep "$HASH6 is the first bad commit" my_bisect_log.txt '
Signature · 1 line
2.24.3 (Apple Git-128)
PATCH v4 2/5 rev-list: allow bisect and first-parent flags
In reply to Aaron LipmanAdd first_parent_only parameter to find_bisection(), removing the barrier that prevented combining the --bisect and --first-parent flags when using git rev-list
--- Documentation/rev-list-options.txt | 7 ++--- bisect.c | 28 ++++++++++++------- bisect.h | 2 +- builtin/rev-list.c | 2 +- revision.c | 3 -- t/t6000-rev-list-misc.sh | 4 +-- t/t6002-rev-list-bisect.sh | 45 ++++++++++++++++++++++++++++++ 7 files changed, 70 insertions(+), 21 deletions(-)
Documentation/rev-list-options.txt +3 -4
@@ -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 adjusting to updated upstream from time to time, and this option allows you to ignore the individual commits - brought in to your history by such a merge. Cannot be - combined with --bisect. + brought in to your history by such a merge. --not:: Reverses the meaning of the '{caret}' prefix (or lack thereof)
@@ -207,7 +206,7 @@ ifndef::git-rev-list[] Pretend as if the bad bisection ref `refs/bisect/bad` was listed and as if it was followed by `--not` and the good bisection refs `refs/bisect/good-*` on the command - line. Cannot be combined with --first-parent. + line. endif::git-rev-list[] --stdin::
@@ -743,7 +742,7 @@ outputs 'midpoint', the output of the two commands would be of roughly the same length. Finding the change which introduces a regression is thus reduced to a binary search: repeatedly generate and test new 'midpoint's until the commit chain is of length -one. Cannot be combined with --first-parent. +one. --bisect-vars:: This calculates the same as `--bisect`, except that refs in
bisect.c +18 -10
@@ -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; }
@@ -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,6 +315,8 @@ static struct commit_list *do_find_bisection(struct commit_list *list,continue ;if (weight (p) !=-2 )continue ; +if (first_parent_only) +BUG ("shouldn't be calling count-distance in fp mode" );weight_set (p,count_distance (p));clear_distance (list);
@@ -332,7 +335,10 @@ static struct commit_list *do_find_bisection(struct commit_list *list,if (0 <=weight (p))continue ; -for (q = p->item->parents; q; q = q->next) { + +for (q = p->item->parents; + q; + q = first_parent_only ?NULL : q->next) {if (q->item->object.flags &UNINTERESTING )continue ;if (0 <=weight (q))
@@ -370,7 +376,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list, }void find_bisection (struct commit_list **commit_list,int *reaches, -int *all,int find_all) +int *all,int find_all,int first_parent_only) {int nr, on_list;struct commit_list *list, *p, *best, *next, *last;
@@ -406,7 +412,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, first_parent_only);if (best) {if (!find_all) { list->item = best->item;
@@ -991,6 +997,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, intenum bisect_error res =BISECT_OK ;struct object_id *bisect_rev;char *steps_msg; +int first_parent_only =0 ;/* TODO: pass --first-parent flag from git bisect start */ read_bisect_terms (&term_bad, &term_good);if (read_bisect_refs ())
@@ -1001,11 +1008,12 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, intreturn res;bisect_rev_setup (r, &revs, prefix,"%s" ,"^%s" ,1 ); + revs.first_parent_only = first_parent_only; revs.limited =1 ;bisect_common (&revs); -find_bisection (&revs.commits, &reaches, &all, !!skipped_revs.nr); +find_bisection (&revs.commits, &reaches, &all, !!skipped_revs.nr, first_parent_only); revs.commits =managed_skipped (revs.commits, &tried);if (!revs.commits) {
bisect.h +1 -1
@@ -12,7 +12,7 @@ struct repository;* best commit, as chosen by `find_all`. */ void find_bisection (struct commit_list **list,int *reaches,int *all, -int find_all); +int find_all,int first_parent_only); struct commit_list *filter_skipped(struct commit_list *list, struct commit_list **tried,
builtin/rev-list.c +1 -1
@@ -638,7 +638,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, revs.first_parent_only);if (bisect_show_vars)return show_bisect_vars (&info, reaches, all);
revision.c +0 -3
@@ -2891,9 +2891,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct sif (!revs->reflog_info && revs->grep_filter.use_reflog_filter)die ("cannot use --grep-reflog without --walk-reflogs" ); -if (revs->first_parent_only && revs->bisect) -die (_ ("--first-parent is incompatible with --bisect" )); -if (revs->line_level_traverse && (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH |DIFF_FORMAT_NO_OUTPUT )))die (_ ("-L does not yet support diff formats besides -p and -s" ));
t/t6000-rev-list-misc.sh +2 -2
@@ -128,8 +128,8 @@ test_expect_success 'rev-list can negate index objects' ' test_cmp expect actual ' -test_expect_success '--bisect and --first-parent can not be combined' ' - test_must_fail git rev-list --bisect --first-parent HEAD +test_expect_success '--bisect and --first-parent can be combined' ' + git rev-list --bisect --first-parent HEAD ' test_expect_success '--header shows a NUL after each commit' '
t/t6002-rev-list-bisect.sh +45 -0
49 lines not on this page · raw message
Signature · 1 line
2.24.3 (Apple Git-128)
PATCH v4 3/5 cmd_bisect__helper: defer parsing no-checkout flag
In reply to Aaron Lipmancmd_bisect__helper() is intended as a temporary shim layer serving as an interface for git-bisect.sh. This function and git-bisect.sh should eventually be replaced by a C implementation, cmd_bisect(), serving as an entrypoint for all "git bisect ..." shell commands: cmd_bisect() will only parse the first token following "git bisect", and dispatch the remaining args to the appropriate function ["bisect_start()", "bisect_next()", etc.].
Thus, cmd_bisect__helper() should not be responsible for parsing flags like --no-checkout. Instead, let the --no-checkout flag remain in the argv array, so it may be evaluated alongside the other options already parsed by bisect_start().
--- bisect.c | 3 ++- bisect.h | 4 +--- builtin/bisect--helper.c | 14 ++++++-------- git-bisect.sh | 2 +- 4 files changed, 10 insertions(+), 13 deletions(-)
bisect.c +2 -1
@@ -989,7 +989,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. */ -enum bisect_error bisect_next_all (struct repository *r,const char *prefix,int no_checkout) +enum bisect_error bisect_next_all (struct repository *r,const char *prefix) {struct rev_info revs;struct commit_list *tried;
@@ -997,6 +997,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, intenum bisect_error res =BISECT_OK ;struct object_id *bisect_rev;char *steps_msg; +int no_checkout =ref_exists ("BISECT_HEAD" );int first_parent_only =0 ;/* TODO: pass --first-parent flag from git bisect start */ read_bisect_terms (&term_bad, &term_good);
bisect.h +1 -3
@@ -58,9 +58,7 @@ enum bisect_error { BISECT_INTERNAL_SUCCESS_MERGE_BASE = -11 }; -enum bisect_error bisect_next_all (struct repository *r, -const char *prefix, -int no_checkout); +enum bisect_error bisect_next_all (struct repository *r,const char *prefix);int estimate_bisect_steps (int all);
builtin/bisect--helper.c +6 -8
@@ -18,7 +18,7 @@ static GIT_PATH_FUNC(git_path_head_name, "head-name") static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES") static const char * const git_bisect_helper_usage[] = { -N_ ("git bisect--helper --next-all [--no-checkout]" ), +N_ ("git bisect--helper --next-all" ),N_ ("git bisect--helper --write-terms <bad_term> <good_term>" ),N_ ("git bisect--helper --bisect-clean-state" ),N_ ("git bisect--helper --bisect-reset [<commit>]" ),
@@ -420,9 +420,9 @@ static int bisect_append_log_quoted(const char **argv)return res; } -static int bisect_start (struct bisect_terms *terms,int no_checkout, -const char **argv,int argc) +static int bisect_start (struct bisect_terms *terms,const char **argv,int argc) { +int no_checkout =0 ;int i, has_double_dash =0 , must_write_terms =0 , bad_seen =0 ;int flags, pathspec_pos, res =0 ;struct string_list revs =STRING_LIST_INIT_DUP ;
@@ -631,7 +631,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)BISECT_TERMS , BISECT_START } cmdmode =0 ; -int no_checkout =0 , res =0 , nolog =0 ; +int res =0 , nolog =0 ;struct option options[] = {OPT_CMDMODE (0 ,"next-all" , &cmdmode,N_ ("perform 'git bisect next'" ),NEXT_ALL ),
@@ -653,8 +653,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) N_("print out the bisect terms"), BISECT_TERMS),OPT_CMDMODE (0 ,"bisect-start" , &cmdmode,N_ ("start the bisect session" ),BISECT_START ), -OPT_BOOL (0 ,"no-checkout" , &no_checkout, -N_ ("update BISECT_HEAD instead of checking out the current commit" )),OPT_BOOL (0 ,"no-log" , &nolog,N_ ("no log for BISECT_WRITE" )),OPT_END ()
@@ -670,7 +668,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)switch (cmdmode) {case NEXT_ALL : - res =bisect_next_all (the_repository, prefix, no_checkout); + res =bisect_next_all (the_repository, prefix);break ;case WRITE_TERMS :if (argc !=2 )
@@ -712,7 +710,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)break ;case BISECT_START :set_terms (&terms,"bad" ,"good" ); - res =bisect_start (&terms, no_checkout, argv, argc); + res =bisect_start (&terms, argv, argc);break ;default :return error ("BUG: unknown subcommand '%d'" , cmdmode);
git-bisect.sh +1 -1
@@ -153,7 +153,7 @@ bisect_next() { git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD $TERM_GOOD|| exit # Perform all bisection computation, display and checkout - git bisect--helper --next-all $(git rev-parse --verify -q BISECT_HEAD > /dev/null && echo --no-checkout) + git bisect--helper --next-all res=$? # Check if we should exit because bisection is finished
Signature · 1 line
2.24.3 (Apple Git-128)
PATCH v4 4/5 bisect: introduce first-parent flag
In reply to Aaron LipmanUpon 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.
--- Documentation/git-bisect.txt | 13 ++++++++++++- bisect.c | 5 ++++- builtin/bisect--helper.c | 9 ++++++++- t/t6030-bisect-porcelain.sh | 18 ++++++++++++++++++ 4 files changed, 42 insertions(+), 3 deletions(-)
Documentation/git-bisect.txt +12 -1
@@ -17,7 +17,7 @@ The command takes various subcommands, and different options depending on the subcommand: git bisect start [--term-{old,good}=<term> --term-{new,bad}=<term>] - [--no-checkout] [<bad> [<good>...]] [--] [<paths>...] + [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...] git bisect (bad|new|<term-new>) [<rev>] git bisect (good|old|<term-old>) [<rev>...] git bisect terms [--term-good | --term-bad]
@@ -365,6 +365,17 @@ does not require a checked out tree. + If the repository is bare, `--no-checkout` is assumed. +--first-parent:: ++ +Follow only the first parent commit upon seeing a merge commit. ++ +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. + EXAMPLES --------
bisect.c +4 -1
@@ -15,6 +15,7 @@#include "commit-slab.h" #include "commit-reach.h" #include "object-store.h" +#include "dir.h" static struct oid_array good_revs;static struct oid_array skipped_revs;
@@ -460,6 +461,7 @@ static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN") static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START") static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG") static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS") +static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT") static GIT_PATH_FUNC(git_path_head_name, "head-name") static void read_bisect_paths(struct argv_array *array)
@@ -998,7 +1000,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)struct object_id *bisect_rev;char *steps_msg;int no_checkout =ref_exists ("BISECT_HEAD" ); -int first_parent_only =0 ;/* TODO: pass --first-parent flag from git bisect start */ +int first_parent_only =file_exists (git_path_bisect_first_parent ());read_bisect_terms (&term_bad, &term_good);if (read_bisect_refs ())
@@ -1142,6 +1144,7 @@ int bisect_clean_state(void)unlink_or_warn (git_path_bisect_names ());unlink_or_warn (git_path_bisect_run ());unlink_or_warn (git_path_bisect_terms ()); +unlink_or_warn (git_path_bisect_first_parent ());/* Cleanup head-name if it got left by an old version of git-bisect */ unlink_or_warn (git_path_head_name ());/*
builtin/bisect--helper.c +8 -1
@@ -16,6 +16,7 @@ static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START") static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG") static GIT_PATH_FUNC(git_path_head_name, "head-name") static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES") +static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT") static const char * const git_bisect_helper_usage[] = {N_ ("git bisect--helper --next-all" ),
@@ -27,7 +28,7 @@ static const char * const git_bisect_helper_usage[] = {N_ ("git bisect--helper --bisect-next-check <good_term> <bad_term> [<term>]" ),N_ ("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]" ),N_ ("git bisect--helper --bisect-start [--term-{old,good}=<term> --term-{new,bad}=<term>]" -"[--no-checkout] [<bad> [<good>...]] [--] [<paths>...]" ), +" [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]" ), NULL };
@@ -423,6 +424,7 @@ static int bisect_append_log_quoted(const char **argv)static int bisect_start (struct bisect_terms *terms,const char **argv,int argc) {int no_checkout =0 ; +int first_parent_only =0 ;int i, has_double_dash =0 , must_write_terms =0 , bad_seen =0 ;int flags, pathspec_pos, res =0 ;struct string_list revs =STRING_LIST_INIT_DUP ;
@@ -452,6 +454,8 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)break ; }else if (!strcmp (arg,"--no-checkout" )) { no_checkout =1 ; + }else if (!strcmp (arg,"--first-parent" )) { + first_parent_only =1 ; }else if (!strcmp (arg,"--term-good" ) || !strcmp (arg,"--term-old" )) { i++;
@@ -576,6 +580,9 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)*/ write_file (git_path_bisect_start (),"%s\n" , start_head.buf); +if (first_parent_only) +write_file (git_path_bisect_first_parent (),"\n" ); +if (no_checkout) {if (get_oid (start_head.buf, &oid) <0 ) { res =error (_ ("invalid ref: '%s'" ), start_head.buf);
t/t6030-bisect-porcelain.sh +18 -0
@@ -448,6 +448,24 @@ test_expect_success 'many merge bases creation' ' grep "$SIDE_HASH5" merge_bases.txt ' +# We want to automatically find the merge that +# added "line" into hello. +test_expect_success '"git bisect run --first-parent" simple case' ' + git rev-list --first-parent $B_HASH ^$HASH4 >first_parent_chain.txt && + write_script test_script.sh <<-\EOF && + grep $(git rev-parse HEAD) first_parent_chain.txt || exit -1 + ! grep line hello >/dev/null + EOF + git bisect start --first-parent && + test_path_is_file ".git/BISECT_FIRST_PARENT" && + git bisect good $HASH4 && + git bisect bad $B_HASH && + git bisect run ./test_script.sh >my_bisect_log.txt && + grep "$B_HASH is the first bad commit" my_bisect_log.txt && + git bisect reset && + test_path_is_missing .git/BISECT_FIRST_PARENT +' + test_expect_success 'good merge bases when good and bad are siblings' ' git bisect start "$B_HASH" "$A_HASH" > my_bisect_log.txt && test_i18ngrep "merge base must be tested" my_bisect_log.txt &&
Signature · 1 line
2.24.3 (Apple Git-128)
PATCH v4 5/5 bisect: combine args passed to find_bisection()
In reply to Aaron LipmanNow 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
Also, rename the existing "flags" bitfield to "commit_flags", to explicitly differentiate it from the new "bisect_flags" bitfield.
--- bisect.c | 67 +++++++++++++++++++++++++--------------------- bisect.h | 5 +++- builtin/rev-list.c | 9 ++++++- 3 files changed, 49 insertions(+), 32 deletions(-)
bisect.c +37 -30
@@ -89,7 +89,7 @@ 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,int first_parent_only) +static int count_interesting_parents (struct commit *commit,unsigned bisect_flags) {struct commit_list *p;int count;
@@ -97,7 +97,7 @@ static int count_interesting_parents(struct commit *commit, int first_parent_onlfor (count =0 , p = commit->parents; p; p = p->next) {if (!(p->item->object.flags &UNINTERESTING )) count++; -if (first_parent_only) +if (bisect_flags &FIND_BISECTION_FIRST_PARENT_ONLY )break ; }return count;
@@ -137,7 +137,7 @@ static void show_list(const char *debug, int counted, int nr,for (p = list; p; p = p->next) {struct commit_list *pp;struct commit *commit = p->item; -unsigned flags = commit->object.flags; +unsigned commit_flags = commit->object.flags;enum object_type type;unsigned long size; char *buf = read_object_file(&commit->object.oid, &type,
@@ -146,9 +146,9 @@ static void show_list(const char *debug, int counted, int nr,int subject_len;fprintf (stderr,"%c%c%c " , - (flags &TREESAME ) ?' ' :'T' , - (flags &UNINTERESTING ) ?'U' :' ' , - (flags &COUNTED ) ?'C' :' ' ); + (commit_flags &TREESAME ) ?' ' :'T' , + (commit_flags &UNINTERESTING ) ?'U' :' ' , + (commit_flags &COUNTED ) ?'C' :' ' );if (*commit_weight_at (&commit_weight, p->item))fprintf (stderr,"%3d" ,weight (p));else
@@ -173,9 +173,9 @@ static struct commit_list *best_bisection(struct commit_list *list, int nr) best = list;for (p = list; p; p = p->next) {int distance; -unsigned flags = p->item->object.flags; +unsigned commit_flags = p->item->object.flags; -if (flags &TREESAME ) +if (commit_flags &TREESAME )continue ; distance =weight (p);if (nr - distance < distance)
@@ -214,9 +214,9 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int nfor (p = list, cnt =0 ; p; p = p->next) {int distance; -unsigned flags = p->item->object.flags; +unsigned commit_flags = p->item->object.flags; -if (flags &TREESAME ) +if (commit_flags &TREESAME )continue ; distance =weight (p);if (nr - distance < distance)
@@ -261,7 +261,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 first_parent_only) +unsigned bisect_flags) {int n, counted;struct commit_list *p;
@@ -270,12 +270,12 @@ static struct commit_list *do_find_bisection(struct commit_list *list,for (n =0 , p = list; p; p = p->next) {struct commit *commit = p->item; -unsigned flags = commit->object.flags; +unsigned commit_flags = commit->object.flags; *commit_weight_at (&commit_weight, p->item) = &weights[n++]; -switch (count_interesting_parents (commit, first_parent_only)) { +switch (count_interesting_parents (commit, bisect_flags)) {case 0 : -if (!(flags &TREESAME )) { +if (!(commit_flags &TREESAME )) {weight_set (p,1 ); counted++; show_list("bisection 2 count one",
@@ -316,13 +316,13 @@ static struct commit_list *do_find_bisection(struct commit_list *list,continue ;if (weight (p) !=-2 )continue ; -if (first_parent_only) +if (bisect_flags &FIND_BISECTION_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? */ -if (!find_all &&halfway (p, nr)) +if (!(bisect_flags &FIND_BISECTION_ALL ) &&halfway (p, nr))return p; counted++; }
@@ -332,14 +332,14 @@ static struct commit_list *do_find_bisection(struct commit_list *list,while (counted < nr) {for (p = list; p; p = p->next) {struct commit_list *q; -unsigned flags = p->item->object.flags; +unsigned commit_flags = p->item->object.flags;if (0 <=weight (p))continue ;for (q = p->item->parents; q; - q = first_parent_only ?NULL : q->next) { + q = bisect_flags &FIND_BISECTION_FIRST_PARENT_ONLY ?NULL : q->next) {if (q->item->object.flags &UNINTERESTING )continue ;if (0 <=weight (q))
@@ -353,7 +353,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,* add one for p itself if p is to be counted, * otherwise inherit it from q directly. */ -if (!(flags &TREESAME )) { +if (!(commit_flags &TREESAME )) {weight_set (p,weight (q)+1 ); counted++; show_list("bisection 2 count one",
@@ -363,21 +363,21 @@ static struct commit_list *do_find_bisection(struct commit_list *list,weight_set (p,weight (q));/* Does it happen to be at exactly half-way? */ -if (!find_all &&halfway (p, nr)) +if (!(bisect_flags &FIND_BISECTION_ALL ) &&halfway (p, nr))return p; } }show_list ("bisection 2 counted all" , counted, nr, list); -if (!find_all) +if (!(bisect_flags &FIND_BISECTION_ALL ))return best_bisection (list, nr);else return best_bisection_sorted (list, nr); }void find_bisection (struct commit_list **commit_list,int *reaches, -int *all,int find_all,int first_parent_only) +int *all,unsigned bisect_flags) {int nr, on_list;struct commit_list *list, *p, *best, *next, *last;
@@ -393,16 +393,16 @@ void find_bisection(struct commit_list **commit_list, int *reaches,for (nr = on_list =0 , last =NULL , p = *commit_list; p; p = next) { -unsigned flags = p->item->object.flags; +unsigned commit_flags = p->item->object.flags; next = p->next; -if (flags &UNINTERESTING ) { +if (commit_flags &UNINTERESTING ) {free (p);continue ; } p->next = last; last = p; -if (!(flags &TREESAME )) +if (!(commit_flags &TREESAME )) nr++; on_list++; }
@@ -413,9 +413,9 @@ 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, first_parent_only); + best =do_find_bisection (list, nr, weights, bisect_flags);if (best) { -if (!find_all) { +if (!(bisect_flags &FIND_BISECTION_ALL )) { list->item = best->item;free_commit_list (list->next); best = list;
@@ -1000,23 +1000,30 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)struct object_id *bisect_rev;char *steps_msg;int no_checkout =ref_exists ("BISECT_HEAD" ); -int first_parent_only =file_exists (git_path_bisect_first_parent ()); +unsigned bisect_flags =0 ;read_bisect_terms (&term_bad, &term_good);if (read_bisect_refs ())die (_ ("reading bisect refs failed" )); +if (file_exists (git_path_bisect_first_parent ())) + bisect_flags |=FIND_BISECTION_FIRST_PARENT_ONLY ; + +if (skipped_revs.nr) + bisect_flags |=FIND_BISECTION_ALL ; + res =check_good_are_ancestors_of_bad (r, prefix, no_checkout);if (res)return res;bisect_rev_setup (r, &revs, prefix,"%s" ,"^%s" ,1 ); - revs.first_parent_only = first_parent_only; + + revs.first_parent_only = !!(bisect_flags &FIND_BISECTION_FIRST_PARENT_ONLY ); revs.limited =1 ;bisect_common (&revs); -find_bisection (&revs.commits, &reaches, &all, !!skipped_revs.nr, first_parent_only); +find_bisection (&revs.commits, &reaches, &all, bisect_flags); revs.commits =managed_skipped (revs.commits, &tried);if (!revs.commits) {
bisect.h +4 -1
@@ -12,7 +12,7 @@ struct repository;* best commit, as chosen by `find_all`. */ void find_bisection (struct commit_list **list,int *reaches,int *all, -int find_all,int first_parent_only); +unsigned bisect_flags); struct commit_list *filter_skipped(struct commit_list *list, struct commit_list **tried,
@@ -23,6 +23,9 @@ struct commit_list *filter_skipped(struct commit_list *list,#define BISECT_SHOW_ALL (1<<0)#define REV_LIST_QUIET (1<<1) +#define FIND_BISECTION_ALL (1u<<0) +#define FIND_BISECTION_FIRST_PARENT_ONLY (1u<<1) +struct rev_list_info {struct rev_info *revs;int flags;
builtin/rev-list.c +8 -1
@@ -637,8 +637,15 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)if (bisect_list) {int reaches, all; +unsigned bisect_flags =0 ; -find_bisection (&revs.commits, &reaches, &all, bisect_find_all, revs.first_parent_only); +if (bisect_find_all) + bisect_flags |=FIND_BISECTION_ALL ; + +if (revs.first_parent_only) + bisect_flags |=FIND_BISECTION_FIRST_PARENT_ONLY ; + +find_bisection (&revs.commits, &reaches, &all, bisect_flags);if (bisect_show_vars)return show_bisect_vars (&info, reaches, all);
Signature · 1 line
2.24.3 (Apple Git-128)
Quoted 13 lines · Aaron Lipman
OK, here's take 4! Responding to Junio's feedback, first:
Quoted 10 lines
That function [
cmd_bisect__helper()] is supposed to be a thin shim layer whose only reason of its existence is to serve as an interface with the scripted version of "git bisect". When everything is migrated from the shell script,cmd_bisect__helper()should disappear.
...
This is going backwards, as far as I can tell. If anything, I'd rather seecmd_bisect__helper()get fixed so that it does not parse "--first-parent" (and similarly, "--no-checkout" that you imitated) intofirst_parent_only(andno_checkout) variables and passed as parameters tobisect_start().
Quoted 4 lines
Thanks for the explanation, Junio. (And for bearing with me while I gain familiarity with git's codebase.) Now that I've taken some time to examine how
git-bisect.shandcmd_bisect__helper()fit together, the correct approach is much more clear.
I did notice that no_checkout variable in the top-level cmd_bisect__helper() was convenient to have in the current code structure, and I didn't think how it should be fixed deeply enough, so for the purpose of this series, I do not mind if it is left untouched. It's just that it was disturbing to see that addition of "--first-parent" wanted to add yet another variable to the top-level, and that the new variable was totally unneeded.
Quoted 8 lines
As
no_checkoutwas also passed tobisect_next_all()[via implicitly checking for the existence of .git/BISECT_HEAD when callingbisect_next()ingit-bisect.sh], I've removed that parameter and instead check for .git/BISECT_HEAD inbisect_next_all()to determine whether no-checkout mode is on.This means no-checkout mode can no longer be enabled by "git bisect--helper --next-all --no-checkout"
Unlike "start", "next-all" is not really a command in the end-user's mind ("git bisect next" is, though). It merely is just an interim "convenience" interface for the remaining part of the scripted "git bisect" to call into. As long as the caller can do the same thing as before, it should be OK, I would think.
Re: [PATCH v4 2/5] rev-list: allow bisect and first-parent flags
In reply to Aaron Lipmant/t6002-rev-list-bisect.sh test_expect_success 'rev-parse --bisect can default to good/bad refs' ' · line 263
Quoted 9 lines · Aaron Lipman
Add
first_parent_onlyparameter tofind_bisection(), removing the barrier that prevented combining the --bisect and --first-parent flags when using git rev-listSigned-off-by Aaron Lipman <[email protected]>---
t/t6002-rev-list-bisect.sh +1
-0@@ -263,4 +263,49 @@ test_expect_success 'rev-parse --bisect can default to good/bad refs' ' +test_output_expect_success "--bisect --first-parent" 'git rev-list --bisect --first-parent E ^F' <<EOF
Custom is to use single quotes around the test title, not double quotes (unless you need to interpolate a variable). Same comment applies to other tests introduced by this patch.
t/t6002-rev-list-bisect.sh test_expect_success 'rev-parse --bisect can default to good/bad refs' ' · line 309
Quoted 13 lines
t/t6002-rev-list-bisect.sh +13
-0+test_expect_success "--bisect-all --first-parent" ' +cat > expect.unsorted <<EOF && +$(git rev-parse E) (tag: E, dist=0) +[...] +$(git rev-parse e8) (tag: e8, dist=1) +EOF + +# expect results to be ordered by distance (descending), +# commit hash (ascending) +sort -k4,4r -k1,1 expect.unsorted > expect && +git rev-list --bisect-all --first-parent E ^F > actual && +test_cmp actual expect +'
Indent this test body with TAB rather than having it sit at the left margin. You can use <<-EOF for the here-doc to allow its content to be indented, as well.
Swap the arguments to test_cmp.
Also, for style consistency, no whitespace following redirect:
cat >expect.unsorted <<-EOF
...
EOF
...
sort ... >expect &&
git rev-list ... >actual &&
test_cmp expect actualQuoted 2 lines · Aaron Lipman
OK, here's take 4! Responding to Junio's feedback, first:
[...]
Quoted 1 line
Martin, thanks for your suggestions
[...]
It's better to have the people you are replying to as recipients of your emails (in the "To:" field). I have added them into "Cc:".
Quoted 4 lines
Quoted 1 line
(Signed-off-by: Martin Ågren <[email protected]>, FWIW.)
I'm still getting used to the conventions - should I add your name as a signed-off-by tag, a thanks-to tag, or both?
We often use the following trailers:
- "Helped-by:" when someone helped you
- "Suggested-by:" when someone suggested the main idea in the patch
- "Reported-by:" when someone reported an issue fixed by the patch
- "Acked-by:" when someone explicitly acked the patch
- "Reviewed-by:" when someone explicitly gave their "Reviewed-by:"
If your patch is based on a patch from someone else, you can also keep the "Signed-off-by:" and other trailers that the person already put in the commit message. If you haven't made a lot of changes to a patch initially from someone else you can also keep them as the author.
Thanks,
Christian.
Re: [PATCH v4 1/5] t6030: modernize "git bisect run" tests
In reply to Aaron LipmanQuoted 8 lines · Aaron Lipman
Enforce consistent styling for tests on "git bisect run":
- Use "write_script" to abstract away platform-specific details.
- Favor current whitespace conventions.
- While at it, change "introduced" to "added" in the comments to make them read better.Thanks-to: Martin Ågren <[email protected]>
'Helped-by:' and 'Suggested-by:' are much more often used than 'Thanks-to:'. There are approximately 1182, 247 and 69 occurrences of them respectively.
Quoted 1 line
Signed-off-by Aaron Lipman <[email protected]>
Anyway the patch looks good to me.
Thanks,
Christian.
Hi Aaron,
By the way, welcome to the list!
Quoted 6 lines · Aaron Lipman
Martin, thanks for your suggestions - I've moved the commit updating "git bisect run" tests to 1/5, updated the commit message, and included the changes you provided. (I held off on additional indentation corrections, as it kinda felt like unnecessary code churn/outside the scope of abstracting platform-specific details.)
Ok, great, I think it's fine to stop there. Those spots really are in a much better shape now, thanks.
Quoted 4 lines
Quoted 1 line
(Signed-off-by: Martin Ågren <[email protected]>, FWIW.)
I'm still getting used to the conventions - should I add your name as a signed-off-by tag, a thanks-to tag, or both?
What I meant was "oh, and here's my Signed-off-by for the record" so that we wouldn't need a round-trip of "cool, can I have your S-o-b on that?". Of course, I expressed that in such a lousy, compact way that we needed a round-trip *anyway*. :-/
And I'm not even sure we need a "Signed-off-by". I just posted the patch form of "If you look for chmod, you'll find a few more spots where you can do the exact same transformation, and you might want to look up test_expect_code."
I see you added "Thanks-to" and I would have been fine with no mention at all. I think that patch looks good now, thanks. No need to think about the trailers for that patch now, I think you can safely focus on the later patches from now on. ;-)
Martin
Quoted 26 lines · Christian Couder
Quoted 2 lines · Aaron Lipman
OK, here's take 4! Responding to Junio's feedback, first:
[...]
Quoted 1 line
Martin, thanks for your suggestions
[...]
It's better to have the people you are replying to as recipients of your emails (in the "To:" field). I have added them into "Cc:".
Quoted 4 lines
Quoted 1 line
(Signed-off-by: Martin Ågren <[email protected]>, FWIW.)
I'm still getting used to the conventions - should I add your name as a signed-off-by tag, a thanks-to tag, or both?
We often use the following trailers:
- "Helped-by:" when someone helped you
- "Suggested-by:" when someone suggested the main idea in the patch
- "Reported-by:" when someone reported an issue fixed by the patch
- "Acked-by:" when someone explicitly acked the patch
- "Reviewed-by:" when someone explicitly gave their "Reviewed-by:"
Quoted 4 lines
If your patch is based on a patch from someone else, you can also keep the "Signed-off-by:" and other trailers that the person already put in the commit message. If you haven't made a lot of changes to a patch initially from someone else you can also keep them as the author.
OK. It appears that the patches themselves do not have fundamental issues, perhaps other than test style updates for [2/5]?
Aaron, Eric, can we have the hopefully final update to close this topic, if that is the case?
Thanks.
Quoted 5 lines · Junio C Hamano
OK. It appears that the patches themselves do not have fundamental issues, perhaps other than test style updates for [2/5]?
Aaron, Eric, can we have the hopefully final update to close this topic, if that is the case?
I have not personally reviewed any of these patches, nor have I been following the topic. My one contribution to this thread[1] was merely to point out minor style violations in [2/5] which I noticed while quickly scrolling through the email. Consequently, I'm not in a position to say whether or not there are any fundamental issues with the changes made by any of these patches.
[1]: https://lore.kernel.org/git/CAPig+cQumRSCQA3Et5=h7SD7zqMm_Z6LJVUTonkewR=hNR55Ug@mail.gmail.com/
v5 cover Introduce --first-parent flag for git bisect
In reply to Aaron LipmanThanks for catching those style violations, Eric. (And to everyone else who reviewed!)
Junio, here is the (hopefully) final update with Eric's style corrections applied to 2/5.
Aaron Lipman (5):
t6030: modernize "git bisect run" tests
rev-list: allow bisect and first-parent flags
cmd_bisect__helper: defer parsing no-checkout flag
bisect: introduce first-parent flag
bisect: combine args passed to find_bisection()
Documentation/git-bisect.txt | 13 +++- Documentation/rev-list-options.txt | 7 +- bisect.c | 79 +++++++++++++--------- bisect.h | 9 +-- builtin/bisect--helper.c | 23 ++++--- builtin/rev-list.c | 9 ++- git-bisect.sh | 2 +- revision.c | 3 - t/t6000-rev-list-misc.sh | 4 +- t/t6002-rev-list-bisect.sh | 45 +++++++++++++ t/t6030-bisect-porcelain.sh | 104 ++++++++++++++++------------- 11 files changed, 195 insertions(+), 103 deletions(-)
Signature · 1 line
2.24.3 (Apple Git-128)
PATCH v5 1/5 t6030: modernize "git bisect run" tests
Enforce consistent styling for tests on "git bisect run":
- Use "write_script" to abstract away platform-specific details.
- Favor current whitespace conventions.
- While at it, change "introduced" to "added" in the comments to make them read better.
--- t/t6030-bisect-porcelain.sh | 86 ++++++++++++++++--------------------- 1 file changed, 38 insertions(+), 48 deletions(-)
t/t6030-bisect-porcelain.sh +38 -48
@@ -243,32 +243,30 @@ test_expect_success 'bisect skip: with commit both bad and skipped' ' ' # We want to automatically find the commit that -# introduced "Another" into hello. -test_expect_success \ - '"git bisect run" simple case' \ - 'echo "#"\!"/bin/sh" > test_script.sh && - echo "grep Another hello > /dev/null" >> test_script.sh && - echo "test \$? -ne 0" >> test_script.sh && - chmod +x test_script.sh && - git bisect start && - git bisect good $HASH1 && - git bisect bad $HASH4 && - git bisect run ./test_script.sh > my_bisect_log.txt && - grep "$HASH3 is the first bad commit" my_bisect_log.txt && - git bisect reset' +# added "Another" into hello. +test_expect_success '"git bisect run" simple case' ' + write_script test_script.sh <<-\EOF && + ! grep Another hello >/dev/null + EOF + git bisect start && + git bisect good $HASH1 && + git bisect bad $HASH4 && + git bisect run ./test_script.sh >my_bisect_log.txt && + grep "$HASH3 is the first bad commit" my_bisect_log.txt && + git bisect reset +' # We want to automatically find the commit that -# introduced "Ciao" into hello. -test_expect_success \ - '"git bisect run" with more complex "git bisect start"' \ - 'echo "#"\!"/bin/sh" > test_script.sh && - echo "grep Ciao hello > /dev/null" >> test_script.sh && - echo "test \$? -ne 0" >> test_script.sh && - chmod +x test_script.sh && - git bisect start $HASH4 $HASH1 && - git bisect run ./test_script.sh > my_bisect_log.txt && - grep "$HASH4 is the first bad commit" my_bisect_log.txt && - git bisect reset' +# added "Ciao" into hello. +test_expect_success '"git bisect run" with more complex "git bisect start"' ' + write_script test_script.sh <<-\EOF && + ! grep Ciao hello >/dev/null + EOF + git bisect start $HASH4 $HASH1 && + git bisect run ./test_script.sh >my_bisect_log.txt && + grep "$HASH4 is the first bad commit" my_bisect_log.txt && + git bisect reset +' # $HASH1 is good, $HASH5 is bad, we skip $HASH3 # but $HASH4 is good,
@@ -295,24 +293,17 @@ HASH6= test_expect_success 'bisect run & skip: cannot tell between 2' ' add_line_into_file "6: Yet a line." hello && HASH6=$(git rev-parse --verify HEAD) && - echo "#"\!"/bin/sh" > test_script.sh && - echo "sed -ne \\\$p hello | grep Ciao > /dev/null && exit 125" >> test_script.sh && - echo "grep line hello > /dev/null" >> test_script.sh && - echo "test \$? -ne 0" >> test_script.sh && - chmod +x test_script.sh && + write_script test_script.sh <<-\EOF && + sed -ne \$p hello | grep Ciao >/dev/null && exit 125 + ! grep line hello >/dev/null + EOF git bisect start $HASH6 $HASH1 && - if git bisect run ./test_script.sh > my_bisect_log.txt - then - echo Oops, should have failed. - false - else - test $? -eq 2 && - grep "first bad commit could be any of" my_bisect_log.txt && - ! grep $HASH3 my_bisect_log.txt && - ! grep $HASH6 my_bisect_log.txt && - grep $HASH4 my_bisect_log.txt && - grep $HASH5 my_bisect_log.txt - fi + test_expect_code 2 git bisect run ./test_script.sh >my_bisect_log.txt && + grep "first bad commit could be any of" my_bisect_log.txt && + ! grep $HASH3 my_bisect_log.txt && + ! grep $HASH6 my_bisect_log.txt && + grep $HASH4 my_bisect_log.txt && + grep $HASH5 my_bisect_log.txt ' HASH7=
@@ -320,14 +311,13 @@ test_expect_success 'bisect run & skip: find first bad' ' git bisect reset && add_line_into_file "7: Should be the last line." hello && HASH7=$(git rev-parse --verify HEAD) && - echo "#"\!"/bin/sh" > test_script.sh && - echo "sed -ne \\\$p hello | grep Ciao > /dev/null && exit 125" >> test_script.sh && - echo "sed -ne \\\$p hello | grep day > /dev/null && exit 125" >> test_script.sh && - echo "grep Yet hello > /dev/null" >> test_script.sh && - echo "test \$? -ne 0" >> test_script.sh && - chmod +x test_script.sh && + write_script test_script.sh <<-\EOF && + sed -ne \$p hello | grep Ciao >/dev/null && exit 125 + sed -ne \$p hello | grep day >/dev/null && exit 125 + ! grep Yet hello >/dev/null + EOF git bisect start $HASH7 $HASH1 && - git bisect run ./test_script.sh > my_bisect_log.txt && + git bisect run ./test_script.sh >my_bisect_log.txt && grep "$HASH6 is the first bad commit" my_bisect_log.txt '
Signature · 1 line
2.24.3 (Apple Git-128)
PATCH v5 2/5 rev-list: allow bisect and first-parent flags
In reply to Aaron LipmanAdd first_parent_only parameter to find_bisection(), removing the barrier that prevented combining the --bisect and --first-parent flags when using git rev-list
--- Documentation/rev-list-options.txt | 7 ++--- bisect.c | 28 ++++++++++++------- bisect.h | 2 +- builtin/rev-list.c | 2 +- revision.c | 3 -- t/t6000-rev-list-misc.sh | 4 +-- t/t6002-rev-list-bisect.sh | 45 ++++++++++++++++++++++++++++++ 7 files changed, 70 insertions(+), 21 deletions(-)
Documentation/rev-list-options.txt +3 -4
@@ -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 adjusting to updated upstream from time to time, and this option allows you to ignore the individual commits - brought in to your history by such a merge. Cannot be - combined with --bisect. + brought in to your history by such a merge. --not:: Reverses the meaning of the '{caret}' prefix (or lack thereof)
@@ -207,7 +206,7 @@ ifndef::git-rev-list[] Pretend as if the bad bisection ref `refs/bisect/bad` was listed and as if it was followed by `--not` and the good bisection refs `refs/bisect/good-*` on the command - line. Cannot be combined with --first-parent. + line. endif::git-rev-list[] --stdin::
@@ -743,7 +742,7 @@ outputs 'midpoint', the output of the two commands would be of roughly the same length. Finding the change which introduces a regression is thus reduced to a binary search: repeatedly generate and test new 'midpoint's until the commit chain is of length -one. Cannot be combined with --first-parent. +one. --bisect-vars:: This calculates the same as `--bisect`, except that refs in
bisect.c +18 -10
@@ -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; }
@@ -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,6 +315,8 @@ static struct commit_list *do_find_bisection(struct commit_list *list,continue ;if (weight (p) !=-2 )continue ; +if (first_parent_only) +BUG ("shouldn't be calling count-distance in fp mode" );weight_set (p,count_distance (p));clear_distance (list);
@@ -332,7 +335,10 @@ static struct commit_list *do_find_bisection(struct commit_list *list,if (0 <=weight (p))continue ; -for (q = p->item->parents; q; q = q->next) { + +for (q = p->item->parents; + q; + q = first_parent_only ?NULL : q->next) {if (q->item->object.flags &UNINTERESTING )continue ;if (0 <=weight (q))
@@ -370,7 +376,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list, }void find_bisection (struct commit_list **commit_list,int *reaches, -int *all,int find_all) +int *all,int find_all,int first_parent_only) {int nr, on_list;struct commit_list *list, *p, *best, *next, *last;
@@ -406,7 +412,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, first_parent_only);if (best) {if (!find_all) { list->item = best->item;
@@ -991,6 +997,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, intenum bisect_error res =BISECT_OK ;struct object_id *bisect_rev;char *steps_msg; +int first_parent_only =0 ;/* TODO: pass --first-parent flag from git bisect start */ read_bisect_terms (&term_bad, &term_good);if (read_bisect_refs ())
@@ -1001,11 +1008,12 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, intreturn res;bisect_rev_setup (r, &revs, prefix,"%s" ,"^%s" ,1 ); + revs.first_parent_only = first_parent_only; revs.limited =1 ;bisect_common (&revs); -find_bisection (&revs.commits, &reaches, &all, !!skipped_revs.nr); +find_bisection (&revs.commits, &reaches, &all, !!skipped_revs.nr, first_parent_only); revs.commits =managed_skipped (revs.commits, &tried);if (!revs.commits) {
bisect.h +1 -1
@@ -12,7 +12,7 @@ struct repository;* best commit, as chosen by `find_all`. */ void find_bisection (struct commit_list **list,int *reaches,int *all, -int find_all); +int find_all,int first_parent_only); struct commit_list *filter_skipped(struct commit_list *list, struct commit_list **tried,
builtin/rev-list.c +1 -1
@@ -638,7 +638,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, revs.first_parent_only);if (bisect_show_vars)return show_bisect_vars (&info, reaches, all);
revision.c +0 -3
@@ -2891,9 +2891,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct sif (!revs->reflog_info && revs->grep_filter.use_reflog_filter)die ("cannot use --grep-reflog without --walk-reflogs" ); -if (revs->first_parent_only && revs->bisect) -die (_ ("--first-parent is incompatible with --bisect" )); -if (revs->line_level_traverse && (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH |DIFF_FORMAT_NO_OUTPUT )))die (_ ("-L does not yet support diff formats besides -p and -s" ));
t/t6000-rev-list-misc.sh +2 -2
@@ -128,8 +128,8 @@ test_expect_success 'rev-list can negate index objects' ' test_cmp expect actual ' -test_expect_success '--bisect and --first-parent can not be combined' ' - test_must_fail git rev-list --bisect --first-parent HEAD +test_expect_success '--bisect and --first-parent can be combined' ' + git rev-list --bisect --first-parent HEAD ' test_expect_success '--header shows a NUL after each commit' '
t/t6002-rev-list-bisect.sh +45 -0
49 lines not on this page · raw message
Signature · 1 line
2.24.3 (Apple Git-128)
PATCH v5 3/5 cmd_bisect__helper: defer parsing no-checkout flag
In reply to Aaron Lipmancmd_bisect__helper() is intended as a temporary shim layer serving as an interface for git-bisect.sh. This function and git-bisect.sh should eventually be replaced by a C implementation, cmd_bisect(), serving as an entrypoint for all "git bisect ..." shell commands: cmd_bisect() will only parse the first token following "git bisect", and dispatch the remaining args to the appropriate function ["bisect_start()", "bisect_next()", etc.].
Thus, cmd_bisect__helper() should not be responsible for parsing flags like --no-checkout. Instead, let the --no-checkout flag remain in the argv array, so it may be evaluated alongside the other options already parsed by bisect_start().
--- bisect.c | 3 ++- bisect.h | 4 +--- builtin/bisect--helper.c | 14 ++++++-------- git-bisect.sh | 2 +- 4 files changed, 10 insertions(+), 13 deletions(-)
bisect.c +2 -1
@@ -989,7 +989,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. */ -enum bisect_error bisect_next_all (struct repository *r,const char *prefix,int no_checkout) +enum bisect_error bisect_next_all (struct repository *r,const char *prefix) {struct rev_info revs;struct commit_list *tried;
@@ -997,6 +997,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, intenum bisect_error res =BISECT_OK ;struct object_id *bisect_rev;char *steps_msg; +int no_checkout =ref_exists ("BISECT_HEAD" );int first_parent_only =0 ;/* TODO: pass --first-parent flag from git bisect start */ read_bisect_terms (&term_bad, &term_good);
bisect.h +1 -3
@@ -58,9 +58,7 @@ enum bisect_error { BISECT_INTERNAL_SUCCESS_MERGE_BASE = -11 }; -enum bisect_error bisect_next_all (struct repository *r, -const char *prefix, -int no_checkout); +enum bisect_error bisect_next_all (struct repository *r,const char *prefix);int estimate_bisect_steps (int all);
builtin/bisect--helper.c +6 -8
@@ -18,7 +18,7 @@ static GIT_PATH_FUNC(git_path_head_name, "head-name") static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES") static const char * const git_bisect_helper_usage[] = { -N_ ("git bisect--helper --next-all [--no-checkout]" ), +N_ ("git bisect--helper --next-all" ),N_ ("git bisect--helper --write-terms <bad_term> <good_term>" ),N_ ("git bisect--helper --bisect-clean-state" ),N_ ("git bisect--helper --bisect-reset [<commit>]" ),
@@ -420,9 +420,9 @@ static int bisect_append_log_quoted(const char **argv)return res; } -static int bisect_start (struct bisect_terms *terms,int no_checkout, -const char **argv,int argc) +static int bisect_start (struct bisect_terms *terms,const char **argv,int argc) { +int no_checkout =0 ;int i, has_double_dash =0 , must_write_terms =0 , bad_seen =0 ;int flags, pathspec_pos, res =0 ;struct string_list revs =STRING_LIST_INIT_DUP ;
@@ -631,7 +631,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)BISECT_TERMS , BISECT_START } cmdmode =0 ; -int no_checkout =0 , res =0 , nolog =0 ; +int res =0 , nolog =0 ;struct option options[] = {OPT_CMDMODE (0 ,"next-all" , &cmdmode,N_ ("perform 'git bisect next'" ),NEXT_ALL ),
@@ -653,8 +653,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) N_("print out the bisect terms"), BISECT_TERMS),OPT_CMDMODE (0 ,"bisect-start" , &cmdmode,N_ ("start the bisect session" ),BISECT_START ), -OPT_BOOL (0 ,"no-checkout" , &no_checkout, -N_ ("update BISECT_HEAD instead of checking out the current commit" )),OPT_BOOL (0 ,"no-log" , &nolog,N_ ("no log for BISECT_WRITE" )),OPT_END ()
@@ -670,7 +668,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)switch (cmdmode) {case NEXT_ALL : - res =bisect_next_all (the_repository, prefix, no_checkout); + res =bisect_next_all (the_repository, prefix);break ;case WRITE_TERMS :if (argc !=2 )
@@ -712,7 +710,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)break ;case BISECT_START :set_terms (&terms,"bad" ,"good" ); - res =bisect_start (&terms, no_checkout, argv, argc); + res =bisect_start (&terms, argv, argc);break ;default :return error ("BUG: unknown subcommand '%d'" , cmdmode);
git-bisect.sh +1 -1
@@ -153,7 +153,7 @@ bisect_next() { git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD $TERM_GOOD|| exit # Perform all bisection computation, display and checkout - git bisect--helper --next-all $(git rev-parse --verify -q BISECT_HEAD > /dev/null && echo --no-checkout) + git bisect--helper --next-all res=$? # Check if we should exit because bisection is finished
Signature · 1 line
2.24.3 (Apple Git-128)
PATCH v5 5/5 bisect: combine args passed to find_bisection()
In reply to Aaron LipmanNow 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
Also, rename the existing "flags" bitfield to "commit_flags", to explicitly differentiate it from the new "bisect_flags" bitfield.
--- bisect.c | 67 +++++++++++++++++++++++++--------------------- bisect.h | 5 +++- builtin/rev-list.c | 9 ++++++- 3 files changed, 49 insertions(+), 32 deletions(-)
bisect.c +37 -30
@@ -89,7 +89,7 @@ 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,int first_parent_only) +static int count_interesting_parents (struct commit *commit,unsigned bisect_flags) {struct commit_list *p;int count;
@@ -97,7 +97,7 @@ static int count_interesting_parents(struct commit *commit, int first_parent_onlfor (count =0 , p = commit->parents; p; p = p->next) {if (!(p->item->object.flags &UNINTERESTING )) count++; -if (first_parent_only) +if (bisect_flags &FIND_BISECTION_FIRST_PARENT_ONLY )break ; }return count;
@@ -137,7 +137,7 @@ static void show_list(const char *debug, int counted, int nr,for (p = list; p; p = p->next) {struct commit_list *pp;struct commit *commit = p->item; -unsigned flags = commit->object.flags; +unsigned commit_flags = commit->object.flags;enum object_type type;unsigned long size; char *buf = read_object_file(&commit->object.oid, &type,
@@ -146,9 +146,9 @@ static void show_list(const char *debug, int counted, int nr,int subject_len;fprintf (stderr,"%c%c%c " , - (flags &TREESAME ) ?' ' :'T' , - (flags &UNINTERESTING ) ?'U' :' ' , - (flags &COUNTED ) ?'C' :' ' ); + (commit_flags &TREESAME ) ?' ' :'T' , + (commit_flags &UNINTERESTING ) ?'U' :' ' , + (commit_flags &COUNTED ) ?'C' :' ' );if (*commit_weight_at (&commit_weight, p->item))fprintf (stderr,"%3d" ,weight (p));else
@@ -173,9 +173,9 @@ static struct commit_list *best_bisection(struct commit_list *list, int nr) best = list;for (p = list; p; p = p->next) {int distance; -unsigned flags = p->item->object.flags; +unsigned commit_flags = p->item->object.flags; -if (flags &TREESAME ) +if (commit_flags &TREESAME )continue ; distance =weight (p);if (nr - distance < distance)
@@ -214,9 +214,9 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int nfor (p = list, cnt =0 ; p; p = p->next) {int distance; -unsigned flags = p->item->object.flags; +unsigned commit_flags = p->item->object.flags; -if (flags &TREESAME ) +if (commit_flags &TREESAME )continue ; distance =weight (p);if (nr - distance < distance)
@@ -261,7 +261,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 first_parent_only) +unsigned bisect_flags) {int n, counted;struct commit_list *p;
@@ -270,12 +270,12 @@ static struct commit_list *do_find_bisection(struct commit_list *list,for (n =0 , p = list; p; p = p->next) {struct commit *commit = p->item; -unsigned flags = commit->object.flags; +unsigned commit_flags = commit->object.flags; *commit_weight_at (&commit_weight, p->item) = &weights[n++]; -switch (count_interesting_parents (commit, first_parent_only)) { +switch (count_interesting_parents (commit, bisect_flags)) {case 0 : -if (!(flags &TREESAME )) { +if (!(commit_flags &TREESAME )) {weight_set (p,1 ); counted++; show_list("bisection 2 count one",
@@ -316,13 +316,13 @@ static struct commit_list *do_find_bisection(struct commit_list *list,continue ;if (weight (p) !=-2 )continue ; -if (first_parent_only) +if (bisect_flags &FIND_BISECTION_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? */ -if (!find_all &&halfway (p, nr)) +if (!(bisect_flags &FIND_BISECTION_ALL ) &&halfway (p, nr))return p; counted++; }
@@ -332,14 +332,14 @@ static struct commit_list *do_find_bisection(struct commit_list *list,while (counted < nr) {for (p = list; p; p = p->next) {struct commit_list *q; -unsigned flags = p->item->object.flags; +unsigned commit_flags = p->item->object.flags;if (0 <=weight (p))continue ;for (q = p->item->parents; q; - q = first_parent_only ?NULL : q->next) { + q = bisect_flags &FIND_BISECTION_FIRST_PARENT_ONLY ?NULL : q->next) {if (q->item->object.flags &UNINTERESTING )continue ;if (0 <=weight (q))
@@ -353,7 +353,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,* add one for p itself if p is to be counted, * otherwise inherit it from q directly. */ -if (!(flags &TREESAME )) { +if (!(commit_flags &TREESAME )) {weight_set (p,weight (q)+1 ); counted++; show_list("bisection 2 count one",
@@ -363,21 +363,21 @@ static struct commit_list *do_find_bisection(struct commit_list *list,weight_set (p,weight (q));/* Does it happen to be at exactly half-way? */ -if (!find_all &&halfway (p, nr)) +if (!(bisect_flags &FIND_BISECTION_ALL ) &&halfway (p, nr))return p; } }show_list ("bisection 2 counted all" , counted, nr, list); -if (!find_all) +if (!(bisect_flags &FIND_BISECTION_ALL ))return best_bisection (list, nr);else return best_bisection_sorted (list, nr); }void find_bisection (struct commit_list **commit_list,int *reaches, -int *all,int find_all,int first_parent_only) +int *all,unsigned bisect_flags) {int nr, on_list;struct commit_list *list, *p, *best, *next, *last;
@@ -393,16 +393,16 @@ void find_bisection(struct commit_list **commit_list, int *reaches,for (nr = on_list =0 , last =NULL , p = *commit_list; p; p = next) { -unsigned flags = p->item->object.flags; +unsigned commit_flags = p->item->object.flags; next = p->next; -if (flags &UNINTERESTING ) { +if (commit_flags &UNINTERESTING ) {free (p);continue ; } p->next = last; last = p; -if (!(flags &TREESAME )) +if (!(commit_flags &TREESAME )) nr++; on_list++; }
@@ -413,9 +413,9 @@ 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, first_parent_only); + best =do_find_bisection (list, nr, weights, bisect_flags);if (best) { -if (!find_all) { +if (!(bisect_flags &FIND_BISECTION_ALL )) { list->item = best->item;free_commit_list (list->next); best = list;
@@ -1000,23 +1000,30 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)struct object_id *bisect_rev;char *steps_msg;int no_checkout =ref_exists ("BISECT_HEAD" ); -int first_parent_only =file_exists (git_path_bisect_first_parent ()); +unsigned bisect_flags =0 ;read_bisect_terms (&term_bad, &term_good);if (read_bisect_refs ())die (_ ("reading bisect refs failed" )); +if (file_exists (git_path_bisect_first_parent ())) + bisect_flags |=FIND_BISECTION_FIRST_PARENT_ONLY ; + +if (skipped_revs.nr) + bisect_flags |=FIND_BISECTION_ALL ; + res =check_good_are_ancestors_of_bad (r, prefix, no_checkout);if (res)return res;bisect_rev_setup (r, &revs, prefix,"%s" ,"^%s" ,1 ); - revs.first_parent_only = first_parent_only; + + revs.first_parent_only = !!(bisect_flags &FIND_BISECTION_FIRST_PARENT_ONLY ); revs.limited =1 ;bisect_common (&revs); -find_bisection (&revs.commits, &reaches, &all, !!skipped_revs.nr, first_parent_only); +find_bisection (&revs.commits, &reaches, &all, bisect_flags); revs.commits =managed_skipped (revs.commits, &tried);if (!revs.commits) {
bisect.h +4 -1
@@ -12,7 +12,7 @@ struct repository;* best commit, as chosen by `find_all`. */ void find_bisection (struct commit_list **list,int *reaches,int *all, -int find_all,int first_parent_only); +unsigned bisect_flags); struct commit_list *filter_skipped(struct commit_list *list, struct commit_list **tried,
@@ -23,6 +23,9 @@ struct commit_list *filter_skipped(struct commit_list *list,#define BISECT_SHOW_ALL (1<<0)#define REV_LIST_QUIET (1<<1) +#define FIND_BISECTION_ALL (1u<<0) +#define FIND_BISECTION_FIRST_PARENT_ONLY (1u<<1) +struct rev_list_info {struct rev_info *revs;int flags;
builtin/rev-list.c +8 -1
@@ -637,8 +637,15 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)if (bisect_list) {int reaches, all; +unsigned bisect_flags =0 ; -find_bisection (&revs.commits, &reaches, &all, bisect_find_all, revs.first_parent_only); +if (bisect_find_all) + bisect_flags |=FIND_BISECTION_ALL ; + +if (revs.first_parent_only) + bisect_flags |=FIND_BISECTION_FIRST_PARENT_ONLY ; + +find_bisection (&revs.commits, &reaches, &all, bisect_flags);if (bisect_show_vars)return show_bisect_vars (&info, reaches, all);
Signature · 1 line
2.24.3 (Apple Git-128)
PATCH v5 4/5 bisect: introduce first-parent flag
In reply to Aaron LipmanUpon 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.
--- Documentation/git-bisect.txt | 13 ++++++++++++- bisect.c | 5 ++++- builtin/bisect--helper.c | 9 ++++++++- t/t6030-bisect-porcelain.sh | 18 ++++++++++++++++++ 4 files changed, 42 insertions(+), 3 deletions(-)
Documentation/git-bisect.txt +12 -1
@@ -17,7 +17,7 @@ The command takes various subcommands, and different options depending on the subcommand: git bisect start [--term-{old,good}=<term> --term-{new,bad}=<term>] - [--no-checkout] [<bad> [<good>...]] [--] [<paths>...] + [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...] git bisect (bad|new|<term-new>) [<rev>] git bisect (good|old|<term-old>) [<rev>...] git bisect terms [--term-good | --term-bad]
@@ -365,6 +365,17 @@ does not require a checked out tree. + If the repository is bare, `--no-checkout` is assumed. +--first-parent:: ++ +Follow only the first parent commit upon seeing a merge commit. ++ +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. + EXAMPLES --------
bisect.c +4 -1
@@ -15,6 +15,7 @@#include "commit-slab.h" #include "commit-reach.h" #include "object-store.h" +#include "dir.h" static struct oid_array good_revs;static struct oid_array skipped_revs;
@@ -460,6 +461,7 @@ static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN") static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START") static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG") static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS") +static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT") static GIT_PATH_FUNC(git_path_head_name, "head-name") static void read_bisect_paths(struct argv_array *array)
@@ -998,7 +1000,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)struct object_id *bisect_rev;char *steps_msg;int no_checkout =ref_exists ("BISECT_HEAD" ); -int first_parent_only =0 ;/* TODO: pass --first-parent flag from git bisect start */ +int first_parent_only =file_exists (git_path_bisect_first_parent ());read_bisect_terms (&term_bad, &term_good);if (read_bisect_refs ())
@@ -1142,6 +1144,7 @@ int bisect_clean_state(void)unlink_or_warn (git_path_bisect_names ());unlink_or_warn (git_path_bisect_run ());unlink_or_warn (git_path_bisect_terms ()); +unlink_or_warn (git_path_bisect_first_parent ());/* Cleanup head-name if it got left by an old version of git-bisect */ unlink_or_warn (git_path_head_name ());/*
builtin/bisect--helper.c +8 -1
@@ -16,6 +16,7 @@ static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START") static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG") static GIT_PATH_FUNC(git_path_head_name, "head-name") static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES") +static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT") static const char * const git_bisect_helper_usage[] = {N_ ("git bisect--helper --next-all" ),
@@ -27,7 +28,7 @@ static const char * const git_bisect_helper_usage[] = {N_ ("git bisect--helper --bisect-next-check <good_term> <bad_term> [<term>]" ),N_ ("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]" ),N_ ("git bisect--helper --bisect-start [--term-{old,good}=<term> --term-{new,bad}=<term>]" -"[--no-checkout] [<bad> [<good>...]] [--] [<paths>...]" ), +" [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]" ), NULL };
@@ -423,6 +424,7 @@ static int bisect_append_log_quoted(const char **argv)static int bisect_start (struct bisect_terms *terms,const char **argv,int argc) {int no_checkout =0 ; +int first_parent_only =0 ;int i, has_double_dash =0 , must_write_terms =0 , bad_seen =0 ;int flags, pathspec_pos, res =0 ;struct string_list revs =STRING_LIST_INIT_DUP ;
@@ -452,6 +454,8 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)break ; }else if (!strcmp (arg,"--no-checkout" )) { no_checkout =1 ; + }else if (!strcmp (arg,"--first-parent" )) { + first_parent_only =1 ; }else if (!strcmp (arg,"--term-good" ) || !strcmp (arg,"--term-old" )) { i++;
@@ -576,6 +580,9 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)*/ write_file (git_path_bisect_start (),"%s\n" , start_head.buf); +if (first_parent_only) +write_file (git_path_bisect_first_parent (),"\n" ); +if (no_checkout) {if (get_oid (start_head.buf, &oid) <0 ) { res =error (_ ("invalid ref: '%s'" ), start_head.buf);
t/t6030-bisect-porcelain.sh +18 -0
@@ -448,6 +448,24 @@ test_expect_success 'many merge bases creation' ' grep "$SIDE_HASH5" merge_bases.txt ' +# We want to automatically find the merge that +# added "line" into hello. +test_expect_success '"git bisect run --first-parent" simple case' ' + git rev-list --first-parent $B_HASH ^$HASH4 >first_parent_chain.txt && + write_script test_script.sh <<-\EOF && + grep $(git rev-parse HEAD) first_parent_chain.txt || exit -1 + ! grep line hello >/dev/null + EOF + git bisect start --first-parent && + test_path_is_file ".git/BISECT_FIRST_PARENT" && + git bisect good $HASH4 && + git bisect bad $B_HASH && + git bisect run ./test_script.sh >my_bisect_log.txt && + grep "$B_HASH is the first bad commit" my_bisect_log.txt && + git bisect reset && + test_path_is_missing .git/BISECT_FIRST_PARENT +' + test_expect_success 'good merge bases when good and bad are siblings' ' git bisect start "$B_HASH" "$A_HASH" > my_bisect_log.txt && test_i18ngrep "merge base must be tested" my_bisect_log.txt &&
Signature · 1 line
2.24.3 (Apple Git-128)
Quoted 3 lines · Eric Sunshine
Consequently, I'm not in a
position to say whether or not there are any fundamental issues with the changes made by any of these patches.
Sorry, that wasn't what I meant. A reroll would be done with an updated test, so I was asking your eyeball once again on that.
Quoted 7 lines · Junio C Hamano
Quoted 3 lines · Eric Sunshine
Consequently, I'm not in a
position to say whether or not there are any fundamental issues with the changes made by any of these patches.Sorry, that wasn't what I meant. A reroll would be done with an updated test, so I was asking your eyeball once again on that.
I eyeballed the test in the re-rolled [2/5] from a purely style perspective (not a content perspective), and it looks fine. I could say (and could previously have said) something about the lost exit codes from the $(git rev-parse ...) invocations within the here-doc in order to save Denton the trouble of eradicating them, but that seems relatively minor. Re-thinking the unindented here-docs on the other tests, I wonder now if that they might be easier to read if indented with \-EOF (rather than using plain EOF), as well:
test_output_expect_success '--first-parent' 'git rev-list ...' <<-\EOF
E
...
e8
EOFbut I doubt that such a minor style change is worth a re-roll (and I wouldn't ask Aaron to re-roll just for that).