[v3,0/4] Introduce --first-parent flag for git bisect

Message

Aaron Lipman Aug. 1, 2020, 5:58 p.m. UTC

> 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(-)

Comments

Junio C Hamano Aug. 1, 2020, 7:06 p.m. UTC | #1

Aaron Lipman <[e-mail]> writes:

>> 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?

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.

> 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.

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.