> Ah, I was referring to the order of sign-off, helped-by, etc.
Whoops, re-ordered correctly.
> 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.
Happy to pick that up next!
> > +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
> > 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.
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;
> > +# 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?
> Let's not revert back to ancient line-folding style.
Thank you for the template. I've updated my test accordingly.
> 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.
> - 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.)
> 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.
> 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?
> > + 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.
OK, removed.
> 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)
> // ...
>
> 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(-)
--
2.24.3 (Apple Git-128)
