From: Aaron Lipman <alipman88@gmail.com>
To: git@vger.kernel.org
Cc: Aaron Lipman <alipman88@gmail.com>
Subject: [PATCH v3 0/4] Introduce --first-parent flag for git bisect
Date: Sat, 1 Aug 2020 13:58:36 -0400 [thread overview]
Message-ID: <20200801175840.1877-1-alipman88@gmail.com> (raw)
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?
> 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)
next prev parent reply other threads:[~2020-08-01 17:58 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-28 15:44 [PATCH 0/3] Introduce --first-parent flag for git bisect Aaron Lipman via GitGitGadget
2020-07-28 15:44 ` [PATCH 1/3] rev-list: allow bisect and first-parent flags Aaron Lipman via GitGitGadget
2020-07-28 20:23 ` Junio C Hamano
2020-07-28 21:53 ` Junio C Hamano
2020-07-28 15:44 ` [PATCH 2/3] bisect: introduce first-parent flag Aaron Lipman via GitGitGadget
2020-07-28 15:44 ` [PATCH 3/3] bisect: combine args passed to find_bisection() Aaron Lipman via GitGitGadget
2020-07-30 0:27 ` [PATCH v2 0/3] Introduce --first-parent flag for git bisect Aaron Lipman
2020-07-30 0:27 ` [PATCH v2 1/3] rev-list: allow bisect and first-parent flags Aaron Lipman
2020-07-30 0:27 ` [PATCH v2 2/3] bisect: introduce first-parent flag Aaron Lipman
2020-07-30 4:17 ` Junio C Hamano
2020-07-30 0:27 ` [PATCH v2 3/3] bisect: combine args passed to find_bisection() Aaron Lipman
2020-07-30 4:32 ` Junio C Hamano
2020-07-30 0:48 ` [PATCH v2 0/3] Introduce --first-parent flag for git bisect Junio C Hamano
2020-08-01 17:58 ` Aaron Lipman [this message]
2020-08-01 17:58 ` [PATCH v3 1/4] rev-list: allow bisect and first-parent flags Aaron Lipman
2020-08-01 17:58 ` [PATCH v3 2/4] bisect: introduce first-parent flag Aaron Lipman
2020-08-01 17:58 ` [PATCH v3 3/4] bisect: combine args passed to find_bisection() Aaron Lipman
2020-08-01 19:30 ` Martin Ågren
2020-08-01 17:58 ` [PATCH v3 4/4] bisect: consistent style for git bisect run tests Aaron Lipman
2020-08-01 19:27 ` Martin Ågren
2020-08-01 19:06 ` [PATCH v3 0/4] Introduce --first-parent flag for git bisect Junio C Hamano
2020-08-04 22:01 ` [PATCH v4 0/5] " Aaron Lipman
2020-08-04 22:01 ` [PATCH v4 1/5] t6030: modernize "git bisect run" tests Aaron Lipman
2020-08-05 6:11 ` Christian Couder
2020-08-04 22:01 ` [PATCH v4 2/5] rev-list: allow bisect and first-parent flags Aaron Lipman
2020-08-05 0:38 ` Eric Sunshine
2020-08-04 22:01 ` [PATCH v4 3/5] cmd_bisect__helper: defer parsing no-checkout flag Aaron Lipman
2020-08-04 22:01 ` [PATCH v4 4/5] bisect: introduce first-parent flag Aaron Lipman
2020-08-04 22:01 ` [PATCH v4 5/5] bisect: combine args passed to find_bisection() Aaron Lipman
2020-08-04 22:19 ` [PATCH v4 0/5] Introduce --first-parent flag for git bisect Junio C Hamano
2020-08-05 5:55 ` Christian Couder
2020-08-07 21:05 ` Junio C Hamano
2020-08-07 21:17 ` Eric Sunshine
2020-08-07 22:07 ` Junio C Hamano
2020-08-07 22:20 ` Eric Sunshine
2020-08-05 6:18 ` Martin Ågren
2020-08-07 21:58 ` [PATCH v5 " Aaron Lipman
2020-08-07 21:58 ` [PATCH v5 1/5] t6030: modernize "git bisect run" tests Aaron Lipman
2020-08-07 21:58 ` [PATCH v5 2/5] rev-list: allow bisect and first-parent flags Aaron Lipman
2020-08-07 21:58 ` [PATCH v5 3/5] cmd_bisect__helper: defer parsing no-checkout flag Aaron Lipman
2020-08-07 21:58 ` [PATCH v5 4/5] bisect: introduce first-parent flag Aaron Lipman
2020-08-07 21:58 ` [PATCH v5 5/5] bisect: combine args passed to find_bisection() Aaron Lipman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200801175840.1877-1-alipman88@gmail.com \
--to=alipman88@gmail.com \
--cc=git@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).