* [PATCH v6] Implement --first-parent for git rev-list --bisect @ 2018-08-28 12:32 Tiago Botelho 2018-08-28 13:21 ` Johannes Schindelin 2018-08-28 16:45 ` Junio C Hamano 0 siblings, 2 replies; 12+ messages in thread From: Tiago Botelho @ 2018-08-28 12:32 UTC (permalink / raw) To: git Cc: christian.couder, johannes.schindelin, haraldnordgren, gitster, Tiago Botelho This will enable users to implement bisecting on first parents which can be useful for when the commits from a feature branch that we want to merge are not always tested. Signed-off-by: Tiago Botelho <tiagonbotelho@hotmail.com> --- This patch refactors **only** the tests according to the suggestions made by Junio in v5 more specifically `https://public-inbox.org/git/xmqqa7rhi40f.fsf@gitster-ct.c.googlers.com/`. The discussion between Junio, Christian and Johannes got pretty confusing in the sense that I am unsure if this is the actual problem I am supposed to solve in order to get the patch accepted, if it is not I will keep iterating on it until it is good enough to be merged bisect.c | 43 ++++++++++++++++++++++------------ bisect.h | 1 + builtin/rev-list.c | 3 +++ revision.c | 3 --- t/t6002-rev-list-bisect.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 90 insertions(+), 18 deletions(-) diff --git a/bisect.c b/bisect.c index 4eafc8262..cb80f29c5 100644 --- a/bisect.c +++ b/bisect.c @@ -82,15 +82,16 @@ static inline void weight_set(struct commit_list *elem, int weight) *((int*)(elem->item->util)) = weight; } -static int count_interesting_parents(struct commit *commit) +static int count_interesting_parents(struct commit *commit, unsigned bisect_flags) { 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 (bisect_flags & BISECT_FIRST_PARENT) + break; } return count; } @@ -117,10 +118,10 @@ static inline int halfway(struct commit_list *p, int nr) } #if !DEBUG_BISECT -#define show_list(a,b,c,d) do { ; } while (0) +#define show_list(a,b,c,d,e) do { ; } while (0) #else static void show_list(const char *debug, int counted, int nr, - struct commit_list *list) + struct commit_list *list, unsigned bisect_flags) { struct commit_list *p; @@ -146,10 +147,14 @@ static void show_list(const char *debug, int counted, int nr, else fprintf(stderr, "---"); fprintf(stderr, " %.*s", 8, oid_to_hex(&commit->object.oid)); - for (pp = commit->parents; pp; pp = pp->next) + for (pp = commit->parents; pp; pp = pp->next) { fprintf(stderr, " %.*s", 8, oid_to_hex(&pp->item->object.oid)); + if (bisect_flags & BISECT_FIRST_PARENT) + break; + } + subject_len = find_commit_subject(buf, &subject_start); if (subject_len) fprintf(stderr, " %.*s", subject_len, subject_start); @@ -267,13 +272,13 @@ static struct commit_list *do_find_bisection(struct commit_list *list, unsigned flags = commit->object.flags; p->item->util = &weights[n++]; - switch (count_interesting_parents(commit)) { + switch (count_interesting_parents(commit, bisect_flags)) { case 0: if (!(flags & TREESAME)) { weight_set(p, 1); counted++; show_list("bisection 2 count one", - counted, nr, list); + counted, nr, list, bisect_flags); } /* * otherwise, it is known not to reach any @@ -289,7 +294,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list, } } - show_list("bisection 2 initialize", counted, nr, list); + show_list("bisection 2 initialize", counted, nr, list, bisect_flags); /* * If you have only one parent in the resulting set @@ -319,7 +324,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list, counted++; } - show_list("bisection 2 count_distance", counted, nr, list); + show_list("bisection 2 count_distance", counted, nr, list, bisect_flags); while (counted < nr) { for (p = list; p; p = p->next) { @@ -329,6 +334,11 @@ 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) { + if ((bisect_flags & BISECT_FIRST_PARENT)) { + if (weight(q) < 0) + q = NULL; + break; + } if (q->item->object.flags & UNINTERESTING) continue; if (0 <= weight(q)) @@ -346,7 +356,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list, weight_set(p, weight(q)+1); counted++; show_list("bisection 2 count one", - counted, nr, list); + counted, nr, list, bisect_flags); } else weight_set(p, weight(q)); @@ -357,7 +367,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list, } } - show_list("bisection 2 counted all", counted, nr, list); + show_list("bisection 2 counted all", counted, nr, list, bisect_flags); if (!find_all) return best_bisection(list, nr); @@ -372,7 +382,7 @@ void find_bisection(struct commit_list **commit_list, int *reaches, struct commit_list *list, *p, *best, *next, *last; int *weights; - show_list("bisection 2 entry", 0, 0, *commit_list); + show_list("bisection 2 entry", 0, 0, *commit_list, bisect_flags); /* * Count the number of total and tree-changing items on the @@ -395,7 +405,7 @@ void find_bisection(struct commit_list **commit_list, int *reaches, on_list++; } list = last; - show_list("bisection 2 sorted", 0, nr, list); + show_list("bisection 2 sorted", 0, nr, list, bisect_flags); *all = nr; weights = xcalloc(on_list, sizeof(*weights)); @@ -962,6 +972,9 @@ int bisect_next_all(const char *prefix, int no_checkout) if (skipped_revs.nr) bisect_flags |= BISECT_FIND_ALL; + if (revs.first_parent_only) + bisect_flags |= BISECT_FIRST_PARENT; + find_bisection(&revs.commits, &reaches, &all, bisect_flags); revs.commits = managed_skipped(revs.commits, &tried); diff --git a/bisect.h b/bisect.h index 1d40a33ad..e445567c2 100644 --- a/bisect.h +++ b/bisect.h @@ -2,6 +2,7 @@ #define BISECT_H #define BISECT_FIND_ALL (1u<<0) +#define BISECT_FIRST_PARENT (1u<<1) /* * Find bisection. If something is found, `reaches` will be the number of diff --git a/builtin/rev-list.c b/builtin/rev-list.c index 8752f5bbe..66439e1b3 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -538,6 +538,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) if (bisect_list) { int reaches, all; + if (revs.first_parent_only) + bisect_flags |= BISECT_FIRST_PARENT; + find_bisection(&revs.commits, &reaches, &all, bisect_flags); if (bisect_show_vars) diff --git a/revision.c b/revision.c index 4e0e193e5..b9d063805 100644 --- a/revision.c +++ b/revision.c @@ -2474,9 +2474,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s if (!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->expand_tabs_in_log < 0) revs->expand_tabs_in_log = revs->expand_tabs_in_log_default; diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh index a66140803..1bc297de5 100755 --- a/t/t6002-rev-list-bisect.sh +++ b/t/t6002-rev-list-bisect.sh @@ -263,4 +263,62 @@ test_expect_success 'rev-parse --bisect can default to good/bad refs' ' test_cmp expect.sorted actual.sorted ' +# We generate the following commit graph: +# +# B ------ C +# / \ +# A FX +# \ / +# D - CC - EX + +test_expect_success 'setup' ' + test_commit A && + test_commit B && + test_commit C && + git reset --hard A && + test_commit D && + test_commit CC && + test_commit EX && + test_merge FX C +' + +test_output_expect_success "--bisect --first-parent" 'git rev-list --bisect --first-parent FX ^A' <<EOF +$(git rev-parse CC) +EOF + +test_output_expect_success "--first-parent" 'git rev-list --first-parent FX ^A' <<EOF +$(git rev-parse FX) +$(git rev-parse EX) +$(git rev-parse CC) +$(git rev-parse D) +EOF + +test_output_expect_success "--bisect-vars --first-parent" 'git rev-list --bisect-vars --first-parent FX ^A' <<EOF +bisect_rev='$(git rev-parse CC)' +bisect_nr=1 +bisect_good=1 +bisect_bad=1 +bisect_all=4 +bisect_steps=1 +EOF + +test_expect_success "--bisect-all --first-parent" ' +cat >expect <<EOF && +$(git rev-parse CC) (dist=2) +$(git rev-parse EX) (dist=1) +$(git rev-parse D) (dist=1) +$(git rev-parse FX) (dist=0) +EOF + +# Make sure we have the same entries, nothing more, nothing less +git rev-list --bisect-all --first-parent FX ^A >actual && + sort actual >actual.sorted && + sort expect >expect.sorted && + test_cmp expect.sorted actual.sorted && + # Make sure the entries are sorted in the dist order + sed -e "s/.*(dist=\([1-9]*[0-9]\)).*/\1/" actual >actual.dists && + sort -r actual.dists >actual.dists.sorted && + test_cmp actual.dists.sorted actual.dists +' + test_done -- 2.16.3 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v6] Implement --first-parent for git rev-list --bisect 2018-08-28 12:32 [PATCH v6] Implement --first-parent for git rev-list --bisect Tiago Botelho @ 2018-08-28 13:21 ` Johannes Schindelin 2018-08-28 18:39 ` Junio C Hamano 2018-08-28 16:45 ` Junio C Hamano 1 sibling, 1 reply; 12+ messages in thread From: Johannes Schindelin @ 2018-08-28 13:21 UTC (permalink / raw) To: Tiago Botelho Cc: git, christian.couder, haraldnordgren, gitster, Tiago Botelho Hi Tiago, On Tue, 28 Aug 2018, Tiago Botelho wrote: > This will enable users to implement bisecting on first parents > which can be useful for when the commits from a feature branch > that we want to merge are not always tested. This message is still lacking the explanation I asked for, namely for the lines: @@ -329,6 +334,11 @@ 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) { + if ((bisect_flags & BISECT_FIRST_PARENT)) { + if (weight(q) < 0) + q = NULL; + break; + } if (q->item->object.flags & UNINTERESTING) continue; if (0 <= weight(q)) > Signed-off-by: Tiago Botelho <tiagonbotelho@hotmail.com> > --- > > This patch refactors **only** the tests according to the suggestions made by Junio in v5 > more specifically `https://public-inbox.org/git/xmqqa7rhi40f.fsf@gitster-ct.c.googlers.com/`. > > The discussion between Junio, Christian and Johannes got pretty confusing in the sense > that I am unsure if this is the actual problem I am supposed to solve in order to get the > patch accepted, if it is not I will keep iterating on it until it is good enough to be > merged I would have preferred to reuse the already existing commits generated in the `setup` phase rather than generating yet another batch, and to not introduce an inconsistent way to present a commit graph (compare your diagram with the one in https://github.com/git/git/blob/v2.18.0/t/t6002-rev-list-bisect.sh#L64-L90 i.e. *in the same file*), and I mentioned this at least twice before, but maybe it got lost (and I could imagine that Christian's insistence on pushing the patch through with as little changes as he can get away with does not help), but at least now the added tests look slightly more understandable than before (despite funny indentation and too-long lines), so I'll stop imitating a broken record at this point. Ciao, Johannes > > bisect.c | 43 ++++++++++++++++++++++------------ > bisect.h | 1 + > builtin/rev-list.c | 3 +++ > revision.c | 3 --- > t/t6002-rev-list-bisect.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 90 insertions(+), 18 deletions(-) > > diff --git a/bisect.c b/bisect.c > index 4eafc8262..cb80f29c5 100644 > --- a/bisect.c > +++ b/bisect.c > @@ -82,15 +82,16 @@ static inline void weight_set(struct commit_list *elem, int weight) > *((int*)(elem->item->util)) = weight; > } > > -static int count_interesting_parents(struct commit *commit) > +static int count_interesting_parents(struct commit *commit, unsigned bisect_flags) > { > 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 (bisect_flags & BISECT_FIRST_PARENT) > + break; > } > return count; > } > @@ -117,10 +118,10 @@ static inline int halfway(struct commit_list *p, int nr) > } > > #if !DEBUG_BISECT > -#define show_list(a,b,c,d) do { ; } while (0) > +#define show_list(a,b,c,d,e) do { ; } while (0) > #else > static void show_list(const char *debug, int counted, int nr, > - struct commit_list *list) > + struct commit_list *list, unsigned bisect_flags) > { > struct commit_list *p; > > @@ -146,10 +147,14 @@ static void show_list(const char *debug, int counted, int nr, > else > fprintf(stderr, "---"); > fprintf(stderr, " %.*s", 8, oid_to_hex(&commit->object.oid)); > - for (pp = commit->parents; pp; pp = pp->next) > + for (pp = commit->parents; pp; pp = pp->next) { > fprintf(stderr, " %.*s", 8, > oid_to_hex(&pp->item->object.oid)); > > + if (bisect_flags & BISECT_FIRST_PARENT) > + break; > + } > + > subject_len = find_commit_subject(buf, &subject_start); > if (subject_len) > fprintf(stderr, " %.*s", subject_len, subject_start); > @@ -267,13 +272,13 @@ static struct commit_list *do_find_bisection(struct commit_list *list, > unsigned flags = commit->object.flags; > > p->item->util = &weights[n++]; > - switch (count_interesting_parents(commit)) { > + switch (count_interesting_parents(commit, bisect_flags)) { > case 0: > if (!(flags & TREESAME)) { > weight_set(p, 1); > counted++; > show_list("bisection 2 count one", > - counted, nr, list); > + counted, nr, list, bisect_flags); > } > /* > * otherwise, it is known not to reach any > @@ -289,7 +294,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list, > } > } > > - show_list("bisection 2 initialize", counted, nr, list); > + show_list("bisection 2 initialize", counted, nr, list, bisect_flags); > > /* > * If you have only one parent in the resulting set > @@ -319,7 +324,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list, > counted++; > } > > - show_list("bisection 2 count_distance", counted, nr, list); > + show_list("bisection 2 count_distance", counted, nr, list, bisect_flags); > > while (counted < nr) { > for (p = list; p; p = p->next) { > @@ -329,6 +334,11 @@ 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) { > + if ((bisect_flags & BISECT_FIRST_PARENT)) { > + if (weight(q) < 0) > + q = NULL; > + break; > + } > if (q->item->object.flags & UNINTERESTING) > continue; > if (0 <= weight(q)) > @@ -346,7 +356,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list, > weight_set(p, weight(q)+1); > counted++; > show_list("bisection 2 count one", > - counted, nr, list); > + counted, nr, list, bisect_flags); > } > else > weight_set(p, weight(q)); > @@ -357,7 +367,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list, > } > } > > - show_list("bisection 2 counted all", counted, nr, list); > + show_list("bisection 2 counted all", counted, nr, list, bisect_flags); > > if (!find_all) > return best_bisection(list, nr); > @@ -372,7 +382,7 @@ void find_bisection(struct commit_list **commit_list, int *reaches, > struct commit_list *list, *p, *best, *next, *last; > int *weights; > > - show_list("bisection 2 entry", 0, 0, *commit_list); > + show_list("bisection 2 entry", 0, 0, *commit_list, bisect_flags); > > /* > * Count the number of total and tree-changing items on the > @@ -395,7 +405,7 @@ void find_bisection(struct commit_list **commit_list, int *reaches, > on_list++; > } > list = last; > - show_list("bisection 2 sorted", 0, nr, list); > + show_list("bisection 2 sorted", 0, nr, list, bisect_flags); > > *all = nr; > weights = xcalloc(on_list, sizeof(*weights)); > @@ -962,6 +972,9 @@ int bisect_next_all(const char *prefix, int no_checkout) > if (skipped_revs.nr) > bisect_flags |= BISECT_FIND_ALL; > > + if (revs.first_parent_only) > + bisect_flags |= BISECT_FIRST_PARENT; > + > find_bisection(&revs.commits, &reaches, &all, bisect_flags); > revs.commits = managed_skipped(revs.commits, &tried); > > diff --git a/bisect.h b/bisect.h > index 1d40a33ad..e445567c2 100644 > --- a/bisect.h > +++ b/bisect.h > @@ -2,6 +2,7 @@ > #define BISECT_H > > #define BISECT_FIND_ALL (1u<<0) > +#define BISECT_FIRST_PARENT (1u<<1) > > /* > * Find bisection. If something is found, `reaches` will be the number of > diff --git a/builtin/rev-list.c b/builtin/rev-list.c > index 8752f5bbe..66439e1b3 100644 > --- a/builtin/rev-list.c > +++ b/builtin/rev-list.c > @@ -538,6 +538,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) > if (bisect_list) { > int reaches, all; > > + if (revs.first_parent_only) > + bisect_flags |= BISECT_FIRST_PARENT; > + > find_bisection(&revs.commits, &reaches, &all, bisect_flags); > > if (bisect_show_vars) > diff --git a/revision.c b/revision.c > index 4e0e193e5..b9d063805 100644 > --- a/revision.c > +++ b/revision.c > @@ -2474,9 +2474,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s > if (!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->expand_tabs_in_log < 0) > revs->expand_tabs_in_log = revs->expand_tabs_in_log_default; > > diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh > index a66140803..1bc297de5 100755 > --- a/t/t6002-rev-list-bisect.sh > +++ b/t/t6002-rev-list-bisect.sh > @@ -263,4 +263,62 @@ test_expect_success 'rev-parse --bisect can default to good/bad refs' ' > test_cmp expect.sorted actual.sorted > ' > > +# We generate the following commit graph: > +# > +# B ------ C > +# / \ > +# A FX > +# \ / > +# D - CC - EX > + > +test_expect_success 'setup' ' > + test_commit A && > + test_commit B && > + test_commit C && > + git reset --hard A && > + test_commit D && > + test_commit CC && > + test_commit EX && > + test_merge FX C > +' > + > +test_output_expect_success "--bisect --first-parent" 'git rev-list --bisect --first-parent FX ^A' <<EOF > +$(git rev-parse CC) > +EOF > + > +test_output_expect_success "--first-parent" 'git rev-list --first-parent FX ^A' <<EOF > +$(git rev-parse FX) > +$(git rev-parse EX) > +$(git rev-parse CC) > +$(git rev-parse D) > +EOF > + > +test_output_expect_success "--bisect-vars --first-parent" 'git rev-list --bisect-vars --first-parent FX ^A' <<EOF > +bisect_rev='$(git rev-parse CC)' > +bisect_nr=1 > +bisect_good=1 > +bisect_bad=1 > +bisect_all=4 > +bisect_steps=1 > +EOF > + > +test_expect_success "--bisect-all --first-parent" ' > +cat >expect <<EOF && > +$(git rev-parse CC) (dist=2) > +$(git rev-parse EX) (dist=1) > +$(git rev-parse D) (dist=1) > +$(git rev-parse FX) (dist=0) > +EOF > + > +# Make sure we have the same entries, nothing more, nothing less > +git rev-list --bisect-all --first-parent FX ^A >actual && > + sort actual >actual.sorted && > + sort expect >expect.sorted && > + test_cmp expect.sorted actual.sorted && > + # Make sure the entries are sorted in the dist order > + sed -e "s/.*(dist=\([1-9]*[0-9]\)).*/\1/" actual >actual.dists && > + sort -r actual.dists >actual.dists.sorted && > + test_cmp actual.dists.sorted actual.dists > +' > + > test_done > -- > 2.16.3 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v6] Implement --first-parent for git rev-list --bisect 2018-08-28 13:21 ` Johannes Schindelin @ 2018-08-28 18:39 ` Junio C Hamano 2018-08-28 20:45 ` Junio C Hamano 0 siblings, 1 reply; 12+ messages in thread From: Junio C Hamano @ 2018-08-28 18:39 UTC (permalink / raw) To: Johannes Schindelin Cc: Tiago Botelho, git, christian.couder, haraldnordgren, Tiago Botelho Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Hi Tiago, > > On Tue, 28 Aug 2018, Tiago Botelho wrote: > >> This will enable users to implement bisecting on first parents >> which can be useful for when the commits from a feature branch >> that we want to merge are not always tested. > > This message is still lacking the explanation I asked for, namely for the > lines: > > @@ -329,6 +334,11 @@ 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) { > + if ((bisect_flags & BISECT_FIRST_PARENT)) { > + if (weight(q) < 0) > + q = NULL; > + break; > + } > if (q->item->object.flags & UNINTERESTING) > continue; > if (0 <= weight(q)) I've just finished scanning the discussion thread on public-inbox for v5, v4, v3, v2 and the initial round of this series, but found your comments only on the tests. If you have a pointer that would be great; it also is OK to say what kind of explanation is needed for that addition again. FWIW I too was puzzled about the correctness of the added logic above, especially the part that reads weight(q) before checking if it is not UNINTERESTING, but I covered it on a separate message. > I would have preferred to reuse the already existing commits generated in > the `setup` phase rather than generating yet another batch, and to not > introduce an inconsistent way to present a commit graph (compare your > diagram with the one in > https://github.com/git/git/blob/v2.18.0/t/t6002-rev-list-bisect.sh#L64-L90 > i.e. *in the same file*) As I already said in the previous round, I do agree with these. That is, ... >> diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh >> index a66140803..1bc297de5 100755 >> --- a/t/t6002-rev-list-bisect.sh >> +++ b/t/t6002-rev-list-bisect.sh >> @@ -263,4 +263,62 @@ test_expect_success 'rev-parse --bisect can default to good/bad refs' ' >> test_cmp expect.sorted actual.sorted >> ' >> >> +# We generate the following commit graph: >> +# >> +# B ------ C >> +# / \ >> +# A FX >> +# \ / >> +# D - CC - EX >> + >> +test_expect_success 'setup' ' >> + test_commit A && >> + test_commit B && >> + test_commit C && >> + git reset --hard A && >> + test_commit D && >> + test_commit CC && >> + test_commit EX && >> + test_merge FX C >> +' ... the above graph construction should not be necessary. An earlier part of t6002 would have already created a history of suitable shape to use for writing the following tests. >> +test_output_expect_success "--bisect --first-parent" 'git rev-list --bisect --first-parent FX ^A' <<EOF >> +$(git rev-parse CC) >> +EOF >> + >> +test_output_expect_success "--first-parent" 'git rev-list --first-parent FX ^A' <<EOF >> +$(git rev-parse FX) >> +$(git rev-parse EX) >> +$(git rev-parse CC) >> +$(git rev-parse D) >> +EOF >> + >> +test_output_expect_success "--bisect-vars --first-parent" 'git rev-list --bisect-vars --first-parent FX ^A' <<EOF >> +bisect_rev='$(git rev-parse CC)' >> +bisect_nr=1 >> +bisect_good=1 >> +bisect_bad=1 >> +bisect_all=4 >> +bisect_steps=1 >> +EOF >> + >> +test_expect_success "--bisect-all --first-parent" ' >> +cat >expect <<EOF && >> +$(git rev-parse CC) (dist=2) >> +$(git rev-parse EX) (dist=1) >> +$(git rev-parse D) (dist=1) >> +$(git rev-parse FX) (dist=0) >> +EOF >> + >> +# Make sure we have the same entries, nothing more, nothing less >> +git rev-list --bisect-all --first-parent FX ^A >actual && >> + sort actual >actual.sorted && >> + sort expect >expect.sorted && >> + test_cmp expect.sorted actual.sorted && >> + # Make sure the entries are sorted in the dist order >> + sed -e "s/.*(dist=\([1-9]*[0-9]\)).*/\1/" actual >actual.dists && >> + sort -r actual.dists >actual.dists.sorted && >> + test_cmp actual.dists.sorted actual.dists >> +' >> + >> test_done >> -- >> 2.16.3 >> >> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v6] Implement --first-parent for git rev-list --bisect 2018-08-28 18:39 ` Junio C Hamano @ 2018-08-28 20:45 ` Junio C Hamano 2018-08-28 21:24 ` Junio C Hamano 0 siblings, 1 reply; 12+ messages in thread From: Junio C Hamano @ 2018-08-28 20:45 UTC (permalink / raw) To: Tiago Botelho Cc: Johannes Schindelin, git, christian.couder, haraldnordgren, Tiago Botelho Junio C Hamano <gitster@pobox.com> writes: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > >> I would have preferred to reuse the already existing commits generated in >> the `setup` phase rather than generating yet another batch, and to not >> introduce an inconsistent way to present a commit graph (compare your >> diagram with the one in >> https://github.com/git/git/blob/v2.18.0/t/t6002-rev-list-bisect.sh#L64-L90 >> i.e. *in the same file*) > > As I already said in the previous round, I do agree with these. > That is, ... > >>> >>> +# We generate the following commit graph: >>> +# >>> +# B ------ C > ... the above graph construction should not be necessary. An > earlier part of t6002 would have already created a history of > suitable shape to use for writing the following tests. Something like the following, perhaps? I have a feeling that use of test-output-expect-success is trying to be too cute without making the result easier to read, and also makes it harder to debug the test script when it does not work as expected (e.g. you need to see where the output from the actual command is stored by going to the definition of the shell function), and would have preferred to see these three tests written without it (and instead using explicit 'expect' vs 'actual' comparison), but at least the patch below shows what I meant when I suggested updating the tests to reuse the existing history (I do not speak for Johannes, but I am guessing we are on the same page on this one). Thanks. t/t6002-rev-list-bisect.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh index a661408038..d27d0087d6 100755 --- a/t/t6002-rev-list-bisect.sh +++ b/t/t6002-rev-list-bisect.sh @@ -263,4 +263,50 @@ test_expect_success 'rev-parse --bisect can default to good/bad refs' ' test_cmp expect.sorted actual.sorted ' +# See the drawing near the top --- e4 is in the middle of the first parent chain +printf "%s\n" e4 | +test_output_expect_success '--bisect --first-parent' ' + git rev-list --bisect --first-parent E ^F +' + +printf "%s\n" E e1 e2 e3 e4 e5 e6 e7 e8 | +test_output_expect_success "--first-parent" ' + git rev-list --first-parent E ^F +' + +test_output_expect_success '--bisect-vars --first-parent' ' + git rev-list --bisect-vars --first-parent E ^F +' <<-EOF + bisect_rev='e5' + bisect_nr=4 + bisect_good=4 + bisect_bad=3 + bisect_all=9 + bisect_steps=2 +EOF + +test_expect_success '--bisect-all --first-parent' ' + cat >expect <<-EOF && + $(git rev-parse tags/e5) (dist=4) + $(git rev-parse tags/e4) (dist=4) + $(git rev-parse tags/e6) (dist=3) + $(git rev-parse tags/e3) (dist=3) + $(git rev-parse tags/e7) (dist=2) + $(git rev-parse tags/e2) (dist=2) + $(git rev-parse tags/e8) (dist=1) + $(git rev-parse tags/e1) (dist=1) + $(git rev-parse tags/E) (dist=0) + EOF + + # Make sure we have the same entries, nothing more, nothing less + git rev-list --bisect-all --first-parent E ^F >actual && + sort actual >actual.sorted && + sort expect >expect.sorted && + test_cmp expect.sorted actual.sorted && + # Make sure the entries are sorted in the dist order + sed -e "s/.*(dist=\([1-9]*[0-9]\)).*/\1/" actual >actual.dists && + sort -r -n actual.dists >actual.dists.sorted && + test_cmp actual.dists.sorted actual.dists +' + test_done ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v6] Implement --first-parent for git rev-list --bisect 2018-08-28 20:45 ` Junio C Hamano @ 2018-08-28 21:24 ` Junio C Hamano 0 siblings, 0 replies; 12+ messages in thread From: Junio C Hamano @ 2018-08-28 21:24 UTC (permalink / raw) To: Tiago Botelho Cc: Johannes Schindelin, git, christian.couder, haraldnordgren, Tiago Botelho Junio C Hamano <gitster@pobox.com> writes: > Something like the following, perhaps? Having said all that. > +# See the drawing near the top --- e4 is in the middle of the first parent chain > +printf "%s\n" e4 | > +test_output_expect_success '--bisect --first-parent' ' > + git rev-list --bisect --first-parent E ^F > +' > + > +printf "%s\n" E e1 e2 e3 e4 e5 e6 e7 e8 | > +test_output_expect_success "--first-parent" ' > + git rev-list --first-parent E ^F > +' The above two are probably not controversial. > +test_output_expect_success '--bisect-vars --first-parent' ' > + git rev-list --bisect-vars --first-parent E ^F > +' <<-EOF > + bisect_rev='e5' > + bisect_nr=4 > + bisect_good=4 > + bisect_bad=3 > + bisect_all=9 > + bisect_steps=2 > +EOF Perhaps this one isn't either. > +test_expect_success '--bisect-all --first-parent' ' > + cat >expect <<-EOF && > + $(git rev-parse tags/e5) (dist=4) > + $(git rev-parse tags/e4) (dist=4) > + $(git rev-parse tags/e6) (dist=3) > + $(git rev-parse tags/e3) (dist=3) > + $(git rev-parse tags/e7) (dist=2) > + $(git rev-parse tags/e2) (dist=2) > + $(git rev-parse tags/e8) (dist=1) > + $(git rev-parse tags/e1) (dist=1) > + $(git rev-parse tags/E) (dist=0) > + EOF > + > + # Make sure we have the same entries, nothing more, nothing less > + git rev-list --bisect-all --first-parent E ^F >actual && > + sort actual >actual.sorted && > + sort expect >expect.sorted && > + test_cmp expect.sorted actual.sorted && By sorting both, we attempt allow them to come out in any random order when sorting done by --bisect-all gets tiebroken differently between commits with the same dist value. As Johannes said, this is a bit too strict to insist that E *must* get dist 0, when the only thing we may care about are e2 and e7 get the same dist value, which is larger than the dist value given to E, etc. If we wanted to ease the over-strictness, we could omit " (dist=N)" from the 'expect' file, and then sed -e 's/ (dist=[0-9]*)$//' actual | sort >actual.sorted && sort expect >expect.sorted && test_cmp expect.sorted actual.sorted && to compare only the object names. This over-strictness would have caught a bug in Git if we reverted this new feature (i.e. "rev-list --first-parent --bisect-all" does not refuse to work---it just does not do what we expect), which gives distance of -1 in the output (!). From that point of view, I think it is also OK for the test to be spelling the values out like the above. > + # Make sure the entries are sorted in the dist order > + sed -e "s/.*(dist=\([0-9]*\)).*/\1/" actual >actual.dists && > + sort -r -n actual.dists >actual.dists.sorted && I forgot to mention but I added "-n" here to make sure we numerically sort the list of distance values. Also you had a bogus regexp to catch digits inherited from "something like this" I showed in an older discussion (I think it is sufficient to say [0-9]* here). The above makes sure that commits that have larger distance number are listed earlier in the output, and we do not care if which one of e4 or e5, both of which have distant number 4, is shown first---we'd be happy as long as all the ones at distance 4 are shown before the others at smaller distance number. So with this in place, I think we can omit the exact distance number from the earlier part of this test, but as I said, it would have caught the bug that produces a negative distance value in the output, so I am still on the fence, leaning a bit toward being stricter than necessary. > + test_cmp actual.dists.sorted actual.dists > +' > + > test_done ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v6] Implement --first-parent for git rev-list --bisect 2018-08-28 12:32 [PATCH v6] Implement --first-parent for git rev-list --bisect Tiago Botelho 2018-08-28 13:21 ` Johannes Schindelin @ 2018-08-28 16:45 ` Junio C Hamano 2018-09-02 7:34 ` Duy Nguyen 1 sibling, 1 reply; 12+ messages in thread From: Junio C Hamano @ 2018-08-28 16:45 UTC (permalink / raw) To: Tiago Botelho Cc: git, christian.couder, johannes.schindelin, haraldnordgren, Tiago Botelho, Nguyễn Thái Ngọc Duy Tiago Botelho <tiagonbotelho@gmail.com> writes: > diff --git a/bisect.c b/bisect.c > index 4eafc8262..cb80f29c5 100644 > --- a/bisect.c > +++ b/bisect.c > @@ -82,15 +82,16 @@ static inline void weight_set(struct commit_list *elem, int weight) > *((int*)(elem->item->util)) = weight; > } > > -static int count_interesting_parents(struct commit *commit) > +static int count_interesting_parents(struct commit *commit, unsigned bisect_flags) > { > 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 (bisect_flags & BISECT_FIRST_PARENT) > + break; > } > return count; > } We either say 0 ("the first parent is uninteresting") or 1 ("it is") under BISECT_FIRST_PARENT mode. OK. > @@ -117,10 +118,10 @@ static inline int halfway(struct commit_list *p, int nr) > } > > #if !DEBUG_BISECT > -#define show_list(a,b,c,d) do { ; } while (0) > +#define show_list(a,b,c,d,e) do { ; } while (0) > #else > static void show_list(const char *debug, int counted, int nr, > - struct commit_list *list) > + struct commit_list *list, unsigned bisect_flags) > { > struct commit_list *p; > > @@ -146,10 +147,14 @@ static void show_list(const char *debug, int counted, int nr, An unrelated tangent, but I think I just spotted a bug in the existing code on the line immediately before this hunk, which reads if (commit->util) fprintf(stderr, "%3d", weight(p)); I think this was a bug introduced at bb408ac9 ("bisect.c: use commit-slab for commit weight instead of commit->util", 2018-05-19) where the internal implementation of weight() was changed not to touch commit->util but instead to use a separate commit-slab storage Looking at the code before that conversion, it seems that we were using ->util to store a pointer to an integer, so we had the ability to differenciate non-negative weight (i.e. weight already computed for the commit), negative weight (i.e. not computed yet, but will be), and commits to which the concept of weight is not applicable. When we went to the commit-slab with the change, we lost the ability to represent the third case. I am offhand not sure what the best remedy would be. Perhaps stuff a so-far unused value like -3 to the weight() and use weight(p) == -3 instead of the old !commit->util or something like that? (Duy CC'ed to help checking my sanity on this point). In any case, this is an existing bug in a debug helper, and the focus of this patch is not about fixing that bug, so you can and should leave it as-is, until this patch successfully adds the "bisection following only the first parent" feature. > else > fprintf(stderr, "---"); > fprintf(stderr, " %.*s", 8, oid_to_hex(&commit->object.oid)); > - for (pp = commit->parents; pp; pp = pp->next) > + for (pp = commit->parents; pp; pp = pp->next) { > fprintf(stderr, " %.*s", 8, > oid_to_hex(&pp->item->object.oid)); > > + if (bisect_flags & BISECT_FIRST_PARENT) > + break; > + } > + I am not sure if we want to do this. This is a debugging aid that allows us to peek into the internal state of the commit history graph being worked on; don't we rather want to make sure that tips of merged side branches are not participating (e.g. commit->util that records "weight" is used differently between the first and other parent commits)? > subject_len = find_commit_subject(buf, &subject_start); > if (subject_len) > fprintf(stderr, " %.*s", subject_len, subject_start); > @@ -267,13 +272,13 @@ static struct commit_list *do_find_bisection(struct commit_list *list, > unsigned flags = commit->object.flags; > > p->item->util = &weights[n++]; > - switch (count_interesting_parents(commit)) { > + switch (count_interesting_parents(commit, bisect_flags)) { > case 0: > if (!(flags & TREESAME)) { > weight_set(p, 1); > counted++; > show_list("bisection 2 count one", > - counted, nr, list); > + counted, nr, list, bisect_flags); > } > /* > * otherwise, it is known not to reach any Because count_interesting_parents() returns either 0 or 1 under BISECT_FIRST_PARENT mode, we will never set -2 in this switch() statement. We either paint the bottom boundary with the correct weight, or -1 (i.e. "its weight is one plus its first parent's weight" marker). OK. > @@ -289,7 +294,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list, > } > } > > - show_list("bisection 2 initialize", counted, nr, list); > + show_list("bisection 2 initialize", counted, nr, list, bisect_flags); > > /* > * If you have only one parent in the resulting set > @@ -319,7 +324,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list, > counted++; > } > > - show_list("bisection 2 count_distance", counted, nr, list); > + show_list("bisection 2 count_distance", counted, nr, list, bisect_flags); The loop before this part (not shown) is about counting weight for merges the hard way, which does not apply to us under the new BISECT_FIRST_PARENT mode. IOW, "initialize" and "count_distance" should show exactly the same output from show_list(). > while (counted < nr) { > for (p = list; p; p = p->next) { > @@ -329,6 +334,11 @@ static struct commit_list *do_find_bisection(struct commit_list *list, > if (0 <= weight(p)) > continue; If we know the weight of 'p' already, then we are already done with it. In the normal code, that means we are done with all the merges, so the loop immediately below deals only with a single strand of pearls. However, under BISECT_FIRST_PARENT mode, that is different. Let's see. > for (q = p->item->parents; q; q = q->next) { > + if ((bisect_flags & BISECT_FIRST_PARENT)) { > + if (weight(q) < 0) > + q = NULL; > + break; > + } Under first-parent mode, we uncondtionally break out of the loop after inspecting the first parent, even if it is a merge. The code after this loop (outside the context) says if (!q) continue; which is to say "we are not ready to set the weight of p based on the weight of q, because the latter is unknown". So that is done by setting q to NULL, which makes sort of sense. > if (q->item->object.flags & UNINTERESTING) > continue; > ... > if (0 <= weight(q)) Post context of this hunk is "break;", meaning that if we find an interesting parent What the above original tells us is that when we are doing a full bisection, we do not look at weight of uninteresting parent and continue. In that case we know p is not a merge so we will exit this for() loop with q set to NULL. Shouldn't we keep that property even under BISECT_FIRST_PARENT mode? What I am wondering is the block added to the top of this for() loop. Shouldn't it be more like this? if ((bisect_flags & BISECT_FIRST_PARENT)) { if ((q->item->object.flags & UNINTERESTING) || (weight(q) < 0)) q = NULL; break; } I do not even recall if weight(q) is defined for an uninteresting q offhand; your patch requires weight(q) is defined and is negative for such a 'q' to be correct. Code would be more trivially correct with an explicit check for UNINTERESTING, I would think. In any case, after finding a parent (or not finding, signalled by being NULL) whose weight is either the same or one less than our weight, we leave the above for() loop and then ... > @@ -346,7 +356,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list, > weight_set(p, weight(q)+1); > counted++; > show_list("bisection 2 count one", > - counted, nr, list); > + counted, nr, list, bisect_flags); > } > else > weight_set(p, weight(q)); ... we set our weight based on its weight. OK. The remainder of the change looked OK (I didn't look at the the test). Thanks. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v6] Implement --first-parent for git rev-list --bisect 2018-08-28 16:45 ` Junio C Hamano @ 2018-09-02 7:34 ` Duy Nguyen 2018-09-02 7:42 ` [PATCH] bisect.c: make show_list() build again Nguyễn Thái Ngọc Duy 2018-09-04 19:32 ` [PATCH v6] Implement --first-parent for git rev-list --bisect Junio C Hamano 0 siblings, 2 replies; 12+ messages in thread From: Duy Nguyen @ 2018-09-02 7:34 UTC (permalink / raw) To: Junio C Hamano Cc: tiagonbotelho, Git Mailing List, Christian Couder, Johannes Schindelin, haraldnordgren, tiagonbotelho On Tue, Aug 28, 2018 at 6:45 PM Junio C Hamano <gitster@pobox.com> wrote: > > @@ -146,10 +147,14 @@ static void show_list(const char *debug, int counted, int nr, > > An unrelated tangent, but I think I just spotted a bug in the > existing code on the line immediately before this hunk, which reads > > if (commit->util) > fprintf(stderr, "%3d", weight(p)); > > I think this was a bug introduced at bb408ac9 ("bisect.c: use > commit-slab for commit weight instead of commit->util", 2018-05-19) > where the internal implementation of weight() was changed not to > touch commit->util but instead to use a separate commit-slab storage > > Looking at the code before that conversion, it seems that we were > using ->util to store a pointer to an integer, so we had the ability > to differenciate non-negative weight (i.e. weight already computed > for the commit), negative weight (i.e. not computed yet, but will > be), and commits to which the concept of weight is not applicable. > When we went to the commit-slab with the change, we lost the ability > to represent the third case. I am offhand not sure what the best > remedy would be. Perhaps stuff a so-far unused value like -3 to the > weight() and use weight(p) == -3 instead of the old !commit->util or > something like that? Hmm.. no? the commit-slab stores the pointer to the weight, not the weight itself, so we still have the ability to check the third case, I think. > (Duy CC'ed to help checking my sanity on this point). > > In any case, this is an existing bug in a debug helper, and the > focus of this patch is not about fixing that bug, so you can and > should leave it as-is, until this patch successfully adds the > "bisection following only the first parent" feature. Yes. I'll post a patch soon to fix this "commit->util" leftover. -- Duy ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] bisect.c: make show_list() build again 2018-09-02 7:34 ` Duy Nguyen @ 2018-09-02 7:42 ` Nguyễn Thái Ngọc Duy 2018-09-02 7:57 ` Christian Couder 2018-09-04 19:32 ` [PATCH v6] Implement --first-parent for git rev-list --bisect Junio C Hamano 1 sibling, 1 reply; 12+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2018-09-02 7:42 UTC (permalink / raw) To: pclouds Cc: christian.couder, git, gitster, haraldnordgren, johannes.schindelin, tiagonbotelho, tiagonbotelho This function only compiles when DEBUG_BISECT is 1, which is often not the case. As a result there are two commits [1] [2] that break it but the breakages went unnoticed because the code did not compile by default. Update the function and include the new header file to make this function build again. In order to stop this from happening again, the function is now compiled unconditionally but exits early unless DEBUG_BISECT is non-zero. A smart compiler generates no extra code (not even a function call). But even if it does not, this function does not seem to be in a hot path that the extra cost becomes a big problem. [1] bb408ac95d (bisect.c: use commit-slab for commit weight instead of commit->util - 2018-05-19) [2] cbd53a2193 (object-store: move object access functions to object-store.h - 2018-05-15) Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- bisect.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/bisect.c b/bisect.c index e1275ba79e..e65f6821b8 100644 --- a/bisect.c +++ b/bisect.c @@ -13,6 +13,7 @@ #include "sha1-array.h" #include "argv-array.h" #include "commit-slab.h" +#include "object-store.h" static struct oid_array good_revs; static struct oid_array skipped_revs; @@ -120,14 +121,14 @@ static inline int halfway(struct commit_list *p, int nr) } } -#if !DEBUG_BISECT -#define show_list(a,b,c,d) do { ; } while (0) -#else static void show_list(const char *debug, int counted, int nr, struct commit_list *list) { struct commit_list *p; + if (!DEBUG_BISECT) + return; + fprintf(stderr, "%s (%d/%d)\n", debug, counted, nr); for (p = list; p; p = p->next) { @@ -145,7 +146,7 @@ static void show_list(const char *debug, int counted, int nr, (flags & TREESAME) ? ' ' : 'T', (flags & UNINTERESTING) ? 'U' : ' ', (flags & COUNTED) ? 'C' : ' '); - if (commit->util) + if (*commit_weight_at(&commit_weight, p->item)) fprintf(stderr, "%3d", weight(p)); else fprintf(stderr, "---"); @@ -160,7 +161,6 @@ static void show_list(const char *debug, int counted, int nr, fprintf(stderr, "\n"); } } -#endif /* DEBUG_BISECT */ static struct commit_list *best_bisection(struct commit_list *list, int nr) { -- 2.19.0.rc0.337.ge906d732e7 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] bisect.c: make show_list() build again 2018-09-02 7:42 ` [PATCH] bisect.c: make show_list() build again Nguyễn Thái Ngọc Duy @ 2018-09-02 7:57 ` Christian Couder 2018-09-03 17:31 ` Duy Nguyen 0 siblings, 1 reply; 12+ messages in thread From: Christian Couder @ 2018-09-02 7:57 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy Cc: git, Junio C Hamano, Harald Nordgren, Johannes Schindelin, Tiago Botelho, Tiago Botelho On Sun, Sep 2, 2018 at 9:42 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote: > In order to stop this from happening again, the function is now > compiled unconditionally but exits early unless DEBUG_BISECT is > non-zero. Thanks for going the extra mile and doing this! I wonder if we should also try to make the show_list() function part of the trace_*() functions to make it even more regular. This can be a separate patch or topic though. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] bisect.c: make show_list() build again 2018-09-02 7:57 ` Christian Couder @ 2018-09-03 17:31 ` Duy Nguyen 2018-09-04 11:13 ` Christian Couder 0 siblings, 1 reply; 12+ messages in thread From: Duy Nguyen @ 2018-09-03 17:31 UTC (permalink / raw) To: Christian Couder Cc: Git Mailing List, Junio C Hamano, haraldnordgren, Johannes Schindelin, tiagonbotelho, tiagonbotelho On Sun, Sep 2, 2018 at 9:57 AM Christian Couder <christian.couder@gmail.com> wrote: > > On Sun, Sep 2, 2018 at 9:42 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote: > > > In order to stop this from happening again, the function is now > > compiled unconditionally but exits early unless DEBUG_BISECT is > > non-zero. > > Thanks for going the extra mile and doing this! > > I wonder if we should also try to make the show_list() function part > of the trace_*() functions to make it even more regular. This can be a > separate patch or topic though. Yeah that's probably a good idea (though I'm not familiar with bisect.c enough to take that step). -- Duy ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] bisect.c: make show_list() build again 2018-09-03 17:31 ` Duy Nguyen @ 2018-09-04 11:13 ` Christian Couder 0 siblings, 0 replies; 12+ messages in thread From: Christian Couder @ 2018-09-04 11:13 UTC (permalink / raw) To: Duy Nguyen Cc: Git Mailing List, Junio C Hamano, Harald Nordgren, Johannes Schindelin, Tiago Botelho, Tiago Botelho On Mon, Sep 3, 2018 at 7:31 PM, Duy Nguyen <pclouds@gmail.com> wrote: > On Sun, Sep 2, 2018 at 9:57 AM Christian Couder > <christian.couder@gmail.com> wrote: >> >> Thanks for going the extra mile and doing this! >> >> I wonder if we should also try to make the show_list() function part >> of the trace_*() functions to make it even more regular. This can be a >> separate patch or topic though. > > Yeah that's probably a good idea (though I'm not familiar with > bisect.c enough to take that step). Maybe this could be a GSoC micro project or a left over bit. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v6] Implement --first-parent for git rev-list --bisect 2018-09-02 7:34 ` Duy Nguyen 2018-09-02 7:42 ` [PATCH] bisect.c: make show_list() build again Nguyễn Thái Ngọc Duy @ 2018-09-04 19:32 ` Junio C Hamano 1 sibling, 0 replies; 12+ messages in thread From: Junio C Hamano @ 2018-09-04 19:32 UTC (permalink / raw) To: Duy Nguyen Cc: tiagonbotelho, Git Mailing List, Christian Couder, Johannes Schindelin, haraldnordgren, tiagonbotelho Duy Nguyen <pclouds@gmail.com> writes: > Hmm.. no? the commit-slab stores the pointer to the weight, not the > weight itself, so we still have the ability to check the third case, I > think. Good, thanks. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-09-04 19:32 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-28 12:32 [PATCH v6] Implement --first-parent for git rev-list --bisect Tiago Botelho 2018-08-28 13:21 ` Johannes Schindelin 2018-08-28 18:39 ` Junio C Hamano 2018-08-28 20:45 ` Junio C Hamano 2018-08-28 21:24 ` Junio C Hamano 2018-08-28 16:45 ` Junio C Hamano 2018-09-02 7:34 ` Duy Nguyen 2018-09-02 7:42 ` [PATCH] bisect.c: make show_list() build again Nguyễn Thái Ngọc Duy 2018-09-02 7:57 ` Christian Couder 2018-09-03 17:31 ` Duy Nguyen 2018-09-04 11:13 ` Christian Couder 2018-09-04 19:32 ` [PATCH v6] Implement --first-parent for git rev-list --bisect Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).