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

From: Aaron Lipman <alipman88@gmail.com>
Date: Aug 1, 2020, 17:58 UTC
Message-ID: <20200801175840.1877-1-alipman88@gmail.com>
In-Reply-To: <20200730002735.87655-1-alipman88@gmail.com>

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

Show 5 quoted lines

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

Show 6 quoted 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.

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

Show 11 quoted lines

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