* Re: What's cooking in git.git (Aug 2020, #07; Thu, 27)
From: Taylor Blau @ 2020-08-28 0:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Taylor Blau, git
In-Reply-To: <xmqqzh6foe44.fsf@gitster.c.googlers.com>
On Thu, Aug 27, 2020 at 04:36:43PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Thu, Aug 27, 2020 at 02:43:02PM -0700, Junio C Hamano wrote:
> >
> >> * tb/repack-clearing-midx (2020-08-26) 1 commit
> >> (merged to 'next' on 2020-08-27 at a465875cbb)
> >> + builtin/repack.c: invalidate MIDX only when necessary
> >>
> >> When a packfile is removed by "git repack", multi-pack-index gets
> >> cleared; the code was taught to do so less aggressively by first
> >> checking if the midx actually refers to a pack that no longer
> >> exists.
> >>
> >> Will merge to 'master'.
> >
> > This seems to break t7700 when run with midx support:
> >
> > $ git checkout e08f7bb093
> > HEAD is now at e08f7bb093 builtin/repack.c: invalidate MIDX only when necessary
> >
> > $ make && (cd t && GIT_TEST_MULTI_PACK_INDEX=1 ./t7700-repack.sh -i)
> > [...]
> > + git repack -a -d
> > Enumerating objects: 10, done.
> > Counting objects: 100% (10/10), done.
> > Delta compression using up to 16 threads
> > Compressing objects: 100% (5/5), done.
> > Writing objects: 100% (10/10), done.
> > Total 10 (delta 1), reused 10 (delta 1), pack-reused 0
> > fatal: error preparing packfile from multi-pack-index
> > error: last command exited with $?=128
> > not ok 6 - packed obs in alt ODB are repacked when local repo has packs
> > #
> > # rm -f .git/objects/pack/* &&
> > # echo new_content >>file1 &&
> > # git add file1 &&
> > # test_tick &&
> > # git commit -m more_content &&
> > # git repack &&
> > # git repack -a -d &&
> > # test_no_missing_in_packs
> >
> > I didn't look into whether it's a bug in the actual code, or just a
> > weird interaction with the way GIT_TEST_MULTI_PACK_INDEX triggers
> > git-repack to write a midx. But either way we should figure that out
> > before it graduates.
>
> Thanks for stopping us ;-)
Thanks indeed. I started looking into this tonight thinking that it'd be
an easy fix, but I think there is a deeper bug that is worth
investigating further.
Let's eject this for now. If the bug turns out to be easier than I
thought, I'll send the patch again, otherwise I'll send it with my
larger series when that's ready.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] send-email: do not prompt for In-Reply-To
From: Junio C Hamano @ 2020-08-28 1:02 UTC (permalink / raw)
To: Drew DeVault; +Cc: Carlo Marcelo Arenas Belón, git
In-Reply-To: <C585M18ZSR87.2CMIKK2VKMC1S@homura>
"Drew DeVault" <sir@cmpwn.com> writes:
> Oh, and I should mention the fact that this breaking change is less
> severe than we initially thought, only breaking an edge case - when --to
> is not specified - so it's likely to have a pretty small impact. We'll
> find out when someone emails us to complain, I suppose.
I agree that it is likely that only very small population even gets
bittn by this "in-reply-to gets prompted" issue, because it is
unlikely for users not to give "to:" address in one way or another
and let the prompt logic to ask them.
Which means that it is OK to stop at step (0) in the long transition
sequence I outlined in a previous message from me.
That is, introduce a configuration variable that can be set to 'no'
by those who do not want to be prompted for "in-reply-to:" when they
did not give "to:" and let the prompt logic to ask "to:", document
the variable well [*1*], and make sure the default behaviour when
the variable is not set is the same as now.
That way, we do not even need to debate if it is true that most
users do not want in-reply-to (i.e. step (1)). That's the kind of
thing that is very hard to establish.
The minority who wants to use an updated behaviour can just set the
configuration once and the problem is behind them. The minority who
wants to keep the current behaviour do not have to do anything. And
there is no impact to the majority of people either way.
I hate having to go through a multi cycle compatibility-breaking
transition, because it would consume unnecessary resources from this
list (i.e. developer attention is the most scarce common resource
that must be shared across topics here), and we would avoid that
cost altogether because the affected population seems to be small
enough that it is not worth going through the rigmarole of flipping
the default. That alone is a big plus.
It seems like we managed to cut down the scope quite a bit.
Thanks.
[Footnote]
*1* As a part of "document the variable well", we can include a
message tweak for the prompt that asks for "in-reply-to:" to say
something like "if you never want to be prompted for in-reply-to:,
you can set this variable" when the variable is not set at all.
Those who did set the variable but still are getting the prompt are
explicitly opting into keeping the current behaviour by setting it
to 'yes', so they do not have to see that extra part of the message.
^ permalink raw reply
* Re: [PATCH] send-email: do not prompt for In-Reply-To
From: Drew DeVault @ 2020-08-28 1:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Carlo Marcelo Arenas Belón, git
In-Reply-To: <xmqqsgc7oa5s.fsf@gitster.c.googlers.com>
On Thu Aug 27, 2020 at 9:02 PM EDT, Junio C Hamano wrote:
> The minority who wants to use an updated behaviour can just set the
> configuration once and the problem is behind them. The minority who
> wants to keep the current behaviour do not have to do anything. And
> there is no impact to the majority of people either way.
Eh, this would defeat the purpose. I hate the multi-cycle
compatibility-breaking dance as much as the next guy, but the whole
reason I'm here is to solve a real problem which noobs are encountering.
I on-board a lot of people with git-send-email and I've heard questions
and confusions over this prompt at least 10 times in the past few years.
Compatibility breakage is annoying but this is a real problem which real
users are having real difficulty with, and the demographic of affected
user may be particularly ill-equipped to deal with it on their own.
^ permalink raw reply
* Re: [PATCH] send-email: do not prompt for In-Reply-To
From: Raymond E. Pasco @ 2020-08-28 1:44 UTC (permalink / raw)
To: Junio C Hamano, Drew DeVault; +Cc: Carlo Marcelo Arenas Belón, git
In-Reply-To: <xmqqsgc7oa5s.fsf@gitster.c.googlers.com>
On Thu Aug 27, 2020 at 9:02 PM EDT, Junio C Hamano wrote:
> I agree that it is likely that only very small population even gets
> bittn by this "in-reply-to gets prompted" issue, because it is
> unlikely for users not to give "to:" address in one way or another
> and let the prompt logic to ask them.
The merits of this thread's proposal aside, send-email's prompting has
always seemed half-baked to me - it often doesn't even show up, and
doesn't prompt for some things that would be useful like additional CCs
(and if you're setting In-Reply-To, there are probably additional CCs).
The prompts have helped me not flub emails before, but arguably that's
not good since they're often not even shown and someone may come to rely
on their presence only to later accidentally turn them off.
Anyway, it's worth noting that, because the prompts *do* exist,
half-baked as they are, it's not really safe to assume that nobody sees
them. There might be a sizable population out there never specifying
"--to" because they know they'll be prompted for it.
^ permalink raw reply
* Re: [PATCH] send-email: do not prompt for In-Reply-To
From: Drew DeVault @ 2020-08-28 1:56 UTC (permalink / raw)
To: Raymond E. Pasco, Junio C Hamano; +Cc: Carlo Marcelo Arenas Belón, git
In-Reply-To: <C588ZA2QAFBS.23DYVMBK3E0X6@ziyou.local>
On Thu Aug 27, 2020 at 9:44 PM EDT, Raymond E. Pasco wrote:
> Anyway, it's worth noting that, because the prompts *do* exist,
> half-baked as they are, it's not really safe to assume that nobody sees
> them. There might be a sizable population out there never specifying
> "--to" because they know they'll be prompted for it.
I hardly ever use --to for one-off patches or repositories which I'm not
regularly contributing to, and I expect that prompt. In-Reply-To has a
much less compelling argument, though, as Junio pointed out, given that
it's often not necessary or desirable, whereas for "to", every email has
to have at least one recipient to make sense.
^ permalink raw reply
* Re: What's cooking in git.git (Aug 2020, #07; Thu, 27)
From: Elijah Newren @ 2020-08-28 2:16 UTC (permalink / raw)
To: Matheus Tavares Bernardino, Jonathan Nieder
Cc: Git Mailing List, Junio C Hamano
In-Reply-To: <xmqqh7snpxy1.fsf@gitster.c.googlers.com>
On Thu, Aug 27, 2020 at 2:46 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> * mt/grep-sparse-checkout (2020-06-12) 6 commits
> - config: add setting to ignore sparsity patterns in some cmds
> - grep: honor sparse checkout patterns
> - config: correctly read worktree configs in submodules
> - t/helper/test-config: facilitate addition of new cli options
> - t/helper/test-config: return exit codes consistently
> - doc: grep: unify info on configuration variables
>
> "git grep" has been tweaked to be limited to the sparse checkout
> paths.
>
> Review needed on 4/6; otherwise looking sane.
> cf. <CABPp-BGdEyEeajYZj_rdxp=MyEQdszuyjVTax=hhYj3fOtRQUQ@mail.gmail.com>
Kinda disappointed to see this stalled out; especially after so much
work put into it. This spawned lots of other side discussions and a
topic or two outside this series as well. I really like the overall
result we came up with out of this series and would like to see it
land. If we can't get more reviewers, maybe we just merge it down
anyway? But, in an attempt to prod some review...
Jonathan: I pinged you in chat about this series some time ago and you
said you'd take a look and comment. I can't find a reference
anywhere, but maybe you talked with Matheus in IRC somewhere? Do you
recall?
Matheus: Do you have any outstanding items for this series or is it
good as far as you know?
^ permalink raw reply
* Re: What's cooking in git.git (Aug 2020, #07; Thu, 27)
From: Jeff King @ 2020-08-28 6:26 UTC (permalink / raw)
To: Taylor Blau; +Cc: Derrick Stolee, Junio C Hamano, git
In-Reply-To: <20200828003940.GA80266@syl.lan>
On Thu, Aug 27, 2020 at 08:39:40PM -0400, Taylor Blau wrote:
> > >> * tb/repack-clearing-midx (2020-08-26) 1 commit
> > >> (merged to 'next' on 2020-08-27 at a465875cbb)
> > >> + builtin/repack.c: invalidate MIDX only when necessary
> [...]
> Thanks indeed. I started looking into this tonight thinking that it'd be
> an easy fix, but I think there is a deeper bug that is worth
> investigating further.
I imagine this is part of it:
diff --git a/builtin/repack.c b/builtin/repack.c
index f10f52779c..2cc05f968a 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -134,7 +134,7 @@ static void remove_redundant_pack(const char *dir_name, const char *base_name)
{
struct strbuf buf = STRBUF_INIT;
struct multi_pack_index *m = get_multi_pack_index(the_repository);
- strbuf_addf(&buf, "%s.pack", base_name);
+ strbuf_addf(&buf, "%s.idx", base_name);
if (m && midx_contains_pack(m, buf.buf))
clear_midx_file(the_repository);
strbuf_insertf(&buf, 0, "%s/", dir_name);
but maybe that is just the "easy" part you meant. Several tests still
seem to fail, which I guess is the "deeper" part. :)
If I'm understanding midx_contains_pack() correctly, then the code
looking for ".pack" could never have matched, and we would never have
deleted a midx here. Which makes me wonder why the "repack removes
multi-pack-index when deleting packs" test ever succeeded.
> Let's eject this for now. If the bug turns out to be easier than I
> thought, I'll send the patch again, otherwise I'll send it with my
> larger series when that's ready.
It's in next, so the preferred path forward is generally to do a fix on
top. Unless it's so unsalvageable that we have to revert, but I doubt
that here.
-Peff
^ permalink raw reply related
* Git in Outreachy?
From: Jeff King @ 2020-08-28 6:56 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Johannes Schindelin
Are we interested in participating in the December 2020 round of
Outreachy? The community application period is now open.
I can look into lining up funding, but we'd also need:
- volunteers to act as mentors
- updates to our applicant materials (proposed projects, but also
microproject / patch suggestions)
-Peff
^ permalink raw reply
* Re: [PATCH 0/5] Add struct strmap and associated utility functions
From: Jeff King @ 2020-08-28 7:03 UTC (permalink / raw)
To: Elijah Newren; +Cc: Elijah Newren via GitGitGadget, Git Mailing List
In-Reply-To: <CABPp-BGYiphp-93Bf=2z-ZLd-Y=buTA0BCp6zuTJF39n1x3Rfw@mail.gmail.com>
On Fri, Aug 21, 2020 at 02:33:54PM -0700, Elijah Newren wrote:
> However, there's an important difference here between what I've done
> and what you've suggested for hashmap: my method did not deallocate
> hashmap->table in hashmap_clear() and then use lazy initialization.
> In fact, I think not deallocating the table was part of the charm --
> the table had already naturally grown to the right size, and because
> the repository has approximately the same number of paths in various
> commits, this provided me a way of getting a table preallocated to a
> reasonable size for all merges after the first (and there are multiple
> merges either when recursiveness is needed due to multiple merge
> bases, OR when rebasing or cherry-picking a sequence of commits).
> This prevented, as hashmap.h puts it, "expensive resizing".
>
> So, once again, my performance ideas might be clashing with some of
> your desires for the API. Any clever ideas for resolving that?
If the magic is in pre-sizing the hash, then it seems like the callers
ought to be feeding the size hint. That does make a little more work for
them, but I think there's real value in having consistent semantics for
"clear" across our data structures.
However, one cheat would be to free the memory but retain the size hint
after a clear. And then if we lazy-init, grow immediately to the hint
size. That's more expensive than a true reuse, because we do reallocate
the memory. But it avoids the repeated re-allocation during growth.
It may also be a sign that we should be growing the hash more
aggressively in the first place. Of course all of this is predicated
having some benchmarks. It would be useful to know which part actually
provided the speedup.
-Peff
^ permalink raw reply
* Re: [PATCH 4/5] strmap: add strdup_strings option
From: Jeff King @ 2020-08-28 7:08 UTC (permalink / raw)
To: Elijah Newren; +Cc: Elijah Newren via GitGitGadget, Git Mailing List
In-Reply-To: <CABPp-BE8tdpjx2RBGyZOYV4hsfjm5HF_dmehvX792x7TtWkLcA@mail.gmail.com>
On Fri, Aug 21, 2020 at 03:25:44PM -0700, Elijah Newren wrote:
> > - That sounds like a lot of maps. :) I guess you've looked at
> > compacting some of them into a single map-to-struct?
>
> Oh, map-to-struct is the primary use. But compacting them won't work,
> because the reason for the additional maps is that they have different
> sets of keys (this set of paths meet a certain condition...). Only
> one map contains all the paths involved in the merge.
OK, I guess I'm not surprised that you would not have missed such an
obvious optimization. :)
> Also, several of those maps don't even store a value; and are really
> just a set implemented via strmap (thus meaning the only bit of data I
> need for some conditions is whether any given path meets it). It
> seems slightly ugly to have to call strmap_put(map, string, NULL) for
> those. I wonder if I should have another strset type much like your
> suggesting for strintmap. Hmm...
FWIW, khash does have a "set" mode where it avoids allocating the value
array at all.
What's the easiest way to benchmark merge-ort? I suspect I could swap
out hashmap for khash (messily) in an hour or less.
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Aug 2020, #07; Thu, 27)
From: Jeff King @ 2020-08-28 8:31 UTC (permalink / raw)
To: Taylor Blau; +Cc: Derrick Stolee, Junio C Hamano, git
In-Reply-To: <20200828062619.GA2100989@coredump.intra.peff.net>
On Fri, Aug 28, 2020 at 02:26:19AM -0400, Jeff King wrote:
> On Thu, Aug 27, 2020 at 08:39:40PM -0400, Taylor Blau wrote:
>
> > > >> * tb/repack-clearing-midx (2020-08-26) 1 commit
> > > >> (merged to 'next' on 2020-08-27 at a465875cbb)
> > > >> + builtin/repack.c: invalidate MIDX only when necessary
> > [...]
> > Thanks indeed. I started looking into this tonight thinking that it'd be
> > an easy fix, but I think there is a deeper bug that is worth
> > investigating further.
>
> I imagine this is part of it:
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index f10f52779c..2cc05f968a 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -134,7 +134,7 @@ static void remove_redundant_pack(const char *dir_name, const char *base_name)
> {
> struct strbuf buf = STRBUF_INIT;
> struct multi_pack_index *m = get_multi_pack_index(the_repository);
> - strbuf_addf(&buf, "%s.pack", base_name);
> + strbuf_addf(&buf, "%s.idx", base_name);
> if (m && midx_contains_pack(m, buf.buf))
> clear_midx_file(the_repository);
> strbuf_insertf(&buf, 0, "%s/", dir_name);
>
> but maybe that is just the "easy" part you meant. Several tests still
> seem to fail, which I guess is the "deeper" part. :)
>
> If I'm understanding midx_contains_pack() correctly, then the code
> looking for ".pack" could never have matched, and we would never have
> deleted a midx here. Which makes me wonder why the "repack removes
> multi-pack-index when deleting packs" test ever succeeded.
Sorry, this is all nonsense.
I forgot about the hackery added in 013fd7ada3 (midx: check both pack
and index names for containment, 2019-04-05). And anyway, the patch
above is totally bogus because we need the ".pack" form in the buffer
when we call unlink_pack_path().
So there's definitely something more odd going on in that failing test.
-Peff
^ permalink raw reply
* Re: [PATCH] pack-redundant: gauge the usage before proposing its removal
From: Jeff King @ 2020-08-28 9:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Taylor Blau, git, dstolee
In-Reply-To: <xmqq1rjuz6n3.fsf_-_@gitster.c.googlers.com>
On Tue, Aug 25, 2020 at 03:45:52PM -0700, Junio C Hamano wrote:
> The subcommand is unusably slow and the reason why nobody reports it
> as a performance bug is suspected to be the absense of users. Let's
> show a big message that asks the user to tell us that they still
> care about the command when an attempt is made to run the command,
> with an escape hatch to override it with a command line option.
I was looking at the history here and noticed this topic, which I
somehow missed when it happened:
$ git show -s cf0879f7e98d2213503622f780d2ab0dd3f93477
commit cf0879f7e98d2213503622f780d2ab0dd3f93477
Merge: 3710f60a80 0e37abd2e8
Author: Junio C Hamano <gitster@pobox.com>
Date: Thu Mar 7 09:59:54 2019 +0900
Merge branch 'sc/pack-redundant'
Update the implementation of pack-redundant for performance in a
repository with many packfiles.
* sc/pack-redundant:
pack-redundant: consistent sort method
pack-redundant: rename pack_list.all_objects
pack-redundant: new algorithm to find min packs
pack-redundant: delete redundant code
pack-redundant: delay creation of unique_objects
t5323: test cases for git-pack-redundant
So it sounds like:
- somebody does care enough to use it
- it may not be horrifically slow anymore
So it may not be worth trying to follow through on the deprecation
(though the fact that neither of us realized this makes me worried for
the general state of maintenance of this code).
-Peff
^ permalink raw reply
* [PATCH 0/2] log_tree_diff: simplify
From: Sergey Organov @ 2020-08-28 11:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, Sergey Organov
These patches are pure code quality improvements, -- they should introduce no
changes in behavior.
Sergey Organov (2):
log_tree_diff: get rid of code duplication for first_parent_only
log_tree_diff: get rid of extra check for NULL
log-tree.c | 21 +++++----------------
1 file changed, 5 insertions(+), 16 deletions(-)
--
2.25.1
^ permalink raw reply
* [PATCH 2/2] log_tree_diff: get rid of extra check for NULL
From: Sergey Organov @ 2020-08-28 11:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, Sergey Organov
In-Reply-To: <20200828110526.21145-1-sorganov@gmail.com>
Get rid of needless check of 'parents' for NULL. The NULL case
is already handled right above, and 'parents' is dereferenced
without check below anyway.
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
log-tree.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/log-tree.c b/log-tree.c
index c01932fa72bf..8ac285a25af5 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -917,7 +917,7 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log
}
/* More than one parent? */
- if (parents && parents->next) {
+ if (parents->next) {
if (opt->ignore_merges)
return 0;
else if (opt->combine_merges)
--
2.25.1
^ permalink raw reply related
* [PATCH 1/2] log_tree_diff: get rid of code duplication for first_parent_only
From: Sergey Organov @ 2020-08-28 11:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, Sergey Organov
In-Reply-To: <20200828110526.21145-1-sorganov@gmail.com>
Handle first_parent_only by breaking from generic loop early
rather than by duplicating (part of) the loop body.
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
log-tree.c | 19 ++++---------------
1 file changed, 4 insertions(+), 15 deletions(-)
diff --git a/log-tree.c b/log-tree.c
index 55a68d0c6101..c01932fa72bf 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -922,21 +922,10 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log
return 0;
else if (opt->combine_merges)
return do_diff_combined(opt, commit);
- else if (opt->first_parent_only) {
- /*
- * Generate merge log entry only for the first
- * parent, showing summary diff of the others
- * we merged _in_.
- */
- parse_commit_or_die(parents->item);
- diff_tree_oid(get_commit_tree_oid(parents->item),
- oid, "", &opt->diffopt);
- log_tree_diff_flush(opt);
- return !opt->loginfo;
+ else if (!opt->first_parent_only) {
+ /* If we show multiple diffs, show the parent info */
+ log->parent = parents->item;
}
-
- /* If we show individual diffs, show the parent info */
- log->parent = parents->item;
}
showed_log = 0;
@@ -952,7 +941,7 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log
/* Set up the log info for the next parent, if any.. */
parents = parents->next;
- if (!parents)
+ if (!parents || opt->first_parent_only)
break;
log->parent = parents->item;
opt->loginfo = log;
--
2.25.1
^ permalink raw reply related
* [PATCH v6 00/13] Finish converting git bisect to C part 2
From: Miriam Rubio @ 2020-08-28 12:46 UTC (permalink / raw)
To: git; +Cc: Miriam Rubio
These patches correspond to a second part of patch series
of Outreachy project "Finish converting `git bisect` from shell to C"
started by Pranit Bauva and Tanushree Tumane
(https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) and
continued by me.
These patch series emails were generated from:
https://gitlab.com/mirucam/git/commits/git-bisect-work-part2-v6ps.
I would like to thank the reviewers for their help.
General changes
---------------
* Rebase on master branch: e9b77c84a0 (Tenth batch, 2020-08-24)
Specific changes
----------------
[1/13] bisect--helper: BUG() in cmd_*() on invalid subcommand
* Amend commit message.
* Remove casting to int.
---
[2/13] bisect--helper: use '-res' in 'cmd_bisect__helper' return
* Amend commit message.
---
[3/13] bisect--helper: introduce new `write_in_file()` function
* Use saved_errno variable.
---
[5/13] bisect--helper: reimplement `bisect_autostart` shell function in C
* Fix bug using empty_strvec instead of NULL in a `bisect_start()` call.
---
[6/13] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell
functions in C
* Change `file_exists(git_path_bisect_head())` to a refs API call.
---
Miriam Rubio (4):
bisect--helper: BUG() in cmd_*() on invalid subcommand
bisect--helper: use '-res' in 'cmd_bisect__helper' return
bisect--helper: introduce new `write_in_file()` function
bisect: call 'clear_commit_marks_all()' in 'bisect_next_all()'
Pranit Bauva (9):
bisect--helper: reimplement `bisect_autostart` shell function in C
bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell
functions in C
bisect--helper: finish porting `bisect_start()` to C
bisect--helper: retire `--bisect-clean-state` subcommand
bisect--helper: retire `--next-all` subcommand
bisect--helper: reimplement `bisect_state` & `bisect_head` shell
functions in C
bisect--helper: retire `--check-expected-revs` subcommand
bisect--helper: retire `--write-terms` subcommand
bisect--helper: retire `--bisect-autostart` subcommand
bisect.c | 8 +
builtin/bisect--helper.c | 445 ++++++++++++++++++++++++++++++++-------
git-bisect.sh | 145 +------------
3 files changed, 380 insertions(+), 218 deletions(-)
--
2.25.0
^ permalink raw reply
* [PATCH v6 01/13] bisect--helper: BUG() in cmd_*() on invalid subcommand
From: Miriam Rubio @ 2020-08-28 12:46 UTC (permalink / raw)
To: git; +Cc: Miriam Rubio, Christian Couder, Johannes Schindelin
In-Reply-To: <20200828124617.60618-1-mirucam@gmail.com>
In cmd_bisect__helper() function, if an invalid or no
subcommand is passed there is a BUG.
BUG() out instead of returning an error.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
builtin/bisect--helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index cdda279b23..f464e95792 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -720,7 +720,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
res = bisect_start(&terms, argv, argc);
break;
default:
- return error("BUG: unknown subcommand '%d'", cmdmode);
+ BUG("unknown subcommand %d", cmdmode);
}
free_terms(&terms);
--
2.25.0
^ permalink raw reply related
* [PATCH v6 02/13] bisect--helper: use '-res' in 'cmd_bisect__helper' return
From: Miriam Rubio @ 2020-08-28 12:46 UTC (permalink / raw)
To: git; +Cc: Miriam Rubio, Christian Couder, Junio C Hamano
In-Reply-To: <20200828124617.60618-1-mirucam@gmail.com>
Following 'enum bisect_error' vocabulary, return variable 'res' is
always non-positive.
Let's use '-res' instead of 'abs(res)' to make the code clearer.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
builtin/bisect--helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index f464e95792..b7345be3a5 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -731,5 +731,5 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE)
res = BISECT_OK;
- return abs(res);
+ return -res;
}
--
2.25.0
^ permalink raw reply related
* [PATCH v6 03/13] bisect--helper: introduce new `write_in_file()` function
From: Miriam Rubio @ 2020-08-28 12:46 UTC (permalink / raw)
To: git; +Cc: Miriam Rubio, Christian Couder, Johannes Schindelin
In-Reply-To: <20200828124617.60618-1-mirucam@gmail.com>
Let's refactor code adding a new `write_in_file()` function
that opens a file for writing a message and closes it and a
wrapper for writing mode.
This helper will be used in later steps and makes the code
simpler and easier to understand.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
builtin/bisect--helper.c | 43 +++++++++++++++++++++++++++++++++-------
1 file changed, 36 insertions(+), 7 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index b7345be3a5..bae09ce65d 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -74,6 +74,40 @@ static int one_of(const char *term, ...)
return res;
}
+static int write_in_file(const char *path, const char *mode, const char *format, va_list args)
+{
+ FILE *fp = NULL;
+ int res = 0;
+
+ if (strcmp(mode, "w"))
+ BUG("write-in-file does not support '%s' mode", mode);
+ fp = fopen(path, mode);
+ if (!fp)
+ return error_errno(_("cannot open file '%s' in mode '%s'"), path, mode);
+ res = vfprintf(fp, format, args);
+
+ if (res < 0) {
+ int saved_errno = errno;
+ fclose(fp);
+ errno = saved_errno;
+ return error_errno(_("could not write to file '%s'"), path);
+ }
+
+ return fclose(fp);
+}
+
+static int write_to_file(const char *path, const char *format, ...)
+{
+ int res;
+ va_list args;
+
+ va_start(args, format);
+ res = write_in_file(path, "w", format, args);
+ va_end(args);
+
+ return res;
+}
+
static int check_term_format(const char *term, const char *orig_term)
{
int res;
@@ -104,7 +138,6 @@ static int check_term_format(const char *term, const char *orig_term)
static int write_terms(const char *bad, const char *good)
{
- FILE *fp = NULL;
int res;
if (!strcmp(bad, good))
@@ -113,13 +146,9 @@ static int write_terms(const char *bad, const char *good)
if (check_term_format(bad, "bad") || check_term_format(good, "good"))
return -1;
- fp = fopen(git_path_bisect_terms(), "w");
- if (!fp)
- return error_errno(_("could not open the file BISECT_TERMS"));
+ res = write_to_file(git_path_bisect_terms(), "%s\n%s\n", bad, good);
- res = fprintf(fp, "%s\n%s\n", bad, good);
- res |= fclose(fp);
- return (res < 0) ? -1 : 0;
+ return res;
}
static int is_expected_rev(const char *expected_hex)
--
2.25.0
^ permalink raw reply related
* [PATCH v6 05/13] bisect: call 'clear_commit_marks_all()' in 'bisect_next_all()'
From: Miriam Rubio @ 2020-08-28 12:46 UTC (permalink / raw)
To: git; +Cc: Miriam Rubio, Christian Couder
In-Reply-To: <20200828124617.60618-1-mirucam@gmail.com>
As there can be other revision walks after bisect_next_all(),
let's add a call to a function to clear all the marks at the
end of bisect_next_all().
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
bisect.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/bisect.c b/bisect.c
index d42a3a3767..c6aba2b9f2 100644
--- a/bisect.c
+++ b/bisect.c
@@ -1082,6 +1082,8 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)
"Bisecting: %d revisions left to test after this %s\n",
nr), nr, steps_msg);
free(steps_msg);
+ /* Clean up objects used, as they will be reused. */
+ clear_commit_marks_all(ALL_REV_FLAGS);
return bisect_checkout(bisect_rev, no_checkout);
}
--
2.25.0
^ permalink raw reply related
* [PATCH v6 04/13] bisect--helper: reimplement `bisect_autostart` shell function in C
From: Miriam Rubio @ 2020-08-28 12:46 UTC (permalink / raw)
To: git
Cc: Pranit Bauva, Lars Schneider, Christian Couder,
Johannes Schindelin, Tanushree Tumane, Miriam Rubio
In-Reply-To: <20200828124617.60618-1-mirucam@gmail.com>
From: Pranit Bauva <pranit.bauva@gmail.com>
Reimplement the `bisect_autostart()` shell function in C and add the
C implementation from `bisect_next()` which was previously left
uncovered.
Add `--bisect-autostart` subcommand to be called from git-bisect.sh.
Using `--bisect-autostart` subcommand is a temporary measure to port
the shell function to C so as to use the existing test suite. As more
functions are ported, this subcommand will be retired and
bisect_autostart() will be called directly by `bisect_state()`.
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
builtin/bisect--helper.c | 44 +++++++++++++++++++++++++++++++++++++++-
git-bisect.sh | 25 ++---------------------
2 files changed, 45 insertions(+), 24 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index bae09ce65d..f71e8e54d2 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -29,6 +29,7 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]"),
N_("git bisect--helper --bisect-start [--term-{old,good}=<term> --term-{new,bad}=<term>]"
" [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]"),
+ N_("git bisect--helper --bisect-autostart"),
NULL
};
@@ -653,6 +654,38 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)
return res;
}
+static inline int file_is_not_empty(const char *path)
+{
+ return !is_empty_or_missing_file(path);
+}
+
+static int bisect_autostart(struct bisect_terms *terms)
+{
+ int res;
+ const char *yesno;
+
+ if (file_is_not_empty(git_path_bisect_start()))
+ return 0;
+
+ fprintf_ln(stderr, _("You need to start by \"git bisect "
+ "start\"\n"));
+
+ if (!isatty(STDIN_FILENO))
+ return 0;
+
+ /*
+ * TRANSLATORS: Make sure to include [Y] and [n] in your
+ * translation. The program will only accept English input
+ * at this point.
+ */
+ yesno = git_prompt(_("Do you want me to do it for you "
+ "[Y/n]? "), PROMPT_ECHO);
+ res = tolower(*yesno) == 'n' ?
+ -1 : bisect_start(terms, empty_strvec, 0);
+
+ return res;
+}
+
int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
{
enum {
@@ -665,7 +698,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
CHECK_AND_SET_TERMS,
BISECT_NEXT_CHECK,
BISECT_TERMS,
- BISECT_START
+ BISECT_START,
+ BISECT_AUTOSTART,
} cmdmode = 0;
int res = 0, nolog = 0;
struct option options[] = {
@@ -689,6 +723,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
N_("print out the bisect terms"), BISECT_TERMS),
OPT_CMDMODE(0, "bisect-start", &cmdmode,
N_("start the bisect session"), BISECT_START),
+ OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
+ N_("start the bisection if it has not yet been started"), BISECT_AUTOSTART),
OPT_BOOL(0, "no-log", &nolog,
N_("no log for BISECT_WRITE")),
OPT_END()
@@ -748,6 +784,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
set_terms(&terms, "bad", "good");
res = bisect_start(&terms, argv, argc);
break;
+ case BISECT_AUTOSTART:
+ if (argc)
+ return error(_("--bisect-autostart does not accept arguments"));
+ set_terms(&terms, "bad", "good");
+ res = bisect_autostart(&terms);
+ break;
default:
BUG("unknown subcommand %d", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index c7580e51a0..9ca583d964 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -49,27 +49,6 @@ bisect_head()
fi
}
-bisect_autostart() {
- test -s "$GIT_DIR/BISECT_START" || {
- gettextln "You need to start by \"git bisect start\"" >&2
- if test -t 0
- then
- # TRANSLATORS: Make sure to include [Y] and [n] in your
- # translation. The program will only accept English input
- # at this point.
- gettext "Do you want me to do it for you [Y/n]? " >&2
- read yesno
- case "$yesno" in
- [Nn]*)
- exit ;;
- esac
- bisect_start
- else
- exit 1
- fi
- }
-}
-
bisect_start() {
git bisect--helper --bisect-start $@ || exit
@@ -108,7 +87,7 @@ bisect_skip() {
}
bisect_state() {
- bisect_autostart
+ git bisect--helper --bisect-autostart
state=$1
git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit
get_terms
@@ -149,7 +128,7 @@ bisect_auto_next() {
bisect_next() {
case "$#" in 0) ;; *) usage ;; esac
- bisect_autostart
+ git bisect--helper --bisect-autostart
git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD $TERM_GOOD|| exit
# Perform all bisection computation, display and checkout
--
2.25.0
^ permalink raw reply related
* [PATCH v6 06/13] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C
From: Miriam Rubio @ 2020-08-28 12:46 UTC (permalink / raw)
To: git
Cc: Pranit Bauva, Lars Schneider, Christian Couder,
Johannes Schindelin, Tanushree Tumane, Miriam Rubio
In-Reply-To: <20200828124617.60618-1-mirucam@gmail.com>
From: Pranit Bauva <pranit.bauva@gmail.com>
Reimplement the `bisect_next()` and the `bisect_auto_next()` shell functions
in C and add the subcommands to `git bisect--helper` to call them from
git-bisect.sh .
bisect_auto_next() function returns an enum bisect_error type as whole
`git bisect` can exit with an error code when bisect_next() does.
Using `--bisect-next` and `--bisect-auto-next` subcommands is a
temporary measure to port shell function to C so as to use the existing
test suite. As more functions are ported, `--bisect-auto-next`
subcommand will be retired and will be called by some other methods.
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
bisect.c | 6 ++
builtin/bisect--helper.c | 183 ++++++++++++++++++++++++++++++++++++++-
git-bisect.sh | 47 +---------
3 files changed, 190 insertions(+), 46 deletions(-)
diff --git a/bisect.c b/bisect.c
index c6aba2b9f2..f0fca5c6f3 100644
--- a/bisect.c
+++ b/bisect.c
@@ -988,6 +988,12 @@ void read_bisect_terms(const char **read_bad, const char **read_good)
* the bisection process finished successfully.
* In this case the calling function or command should not turn a
* BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND return code into an error or a non zero exit code.
+ *
+ * Checking BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND
+ * in bisect_helper::bisect_next() and only transforming it to 0 at
+ * the end of bisect_helper::cmd_bisect__helper() helps bypassing
+ * all the code related to finding a commit to test.
+ *
* If no_checkout is non-zero, the bisection process does not
* checkout the trial commit but instead simply updates BISECT_HEAD.
*/
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index f71e8e54d2..5d443829e0 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -8,6 +8,7 @@
#include "run-command.h"
#include "prompt.h"
#include "quote.h"
+#include "revision.h"
static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
@@ -29,10 +30,17 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]"),
N_("git bisect--helper --bisect-start [--term-{old,good}=<term> --term-{new,bad}=<term>]"
" [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]"),
+ N_("git bisect--helper --bisect-next"),
+ N_("git bisect--helper --bisect-auto-next"),
N_("git bisect--helper --bisect-autostart"),
NULL
};
+struct add_bisect_ref_data {
+ struct rev_info *revs;
+ unsigned int object_flags;
+};
+
struct bisect_terms {
char *term_good;
char *term_bad;
@@ -56,6 +64,8 @@ static void set_terms(struct bisect_terms *terms, const char *bad,
static const char vocab_bad[] = "bad|new";
static const char vocab_good[] = "good|old";
+static int bisect_autostart(struct bisect_terms *terms);
+
/*
* Check whether the string `term` belongs to the set of strings
* included in the variable arguments.
@@ -80,7 +90,7 @@ static int write_in_file(const char *path, const char *mode, const char *format,
FILE *fp = NULL;
int res = 0;
- if (strcmp(mode, "w"))
+ if (strcmp(mode, "w") && strcmp(mode, "a"))
BUG("write-in-file does not support '%s' mode", mode);
fp = fopen(path, mode);
if (!fp)
@@ -109,6 +119,18 @@ static int write_to_file(const char *path, const char *format, ...)
return res;
}
+static int append_to_file(const char *path, const char *format, ...)
+{
+ int res;
+ va_list args;
+
+ va_start(args, format);
+ res = write_in_file(path, "a", format, args);
+ va_end(args);
+
+ return res;
+}
+
static int check_term_format(const char *term, const char *orig_term)
{
int res;
@@ -451,6 +473,143 @@ static int bisect_append_log_quoted(const char **argv)
return res;
}
+static int add_bisect_ref(const char *refname, const struct object_id *oid,
+ int flags, void *cb)
+{
+ struct add_bisect_ref_data *data = cb;
+
+ add_pending_oid(data->revs, refname, oid, data->object_flags);
+
+ return 0;
+}
+
+static int prepare_revs(struct bisect_terms *terms, struct rev_info *revs)
+{
+ int res = 0;
+ struct add_bisect_ref_data cb = { revs };
+ char *good = xstrfmt("%s-*", terms->term_good);
+
+ /*
+ * We cannot use terms->term_bad directly in
+ * for_each_glob_ref_in() and we have to append a '*' to it,
+ * otherwise for_each_glob_ref_in() will append '/' and '*'.
+ */
+ char *bad = xstrfmt("%s*", terms->term_bad);
+
+ /*
+ * It is important to reset the flags used by revision walks
+ * as the previous call to bisect_next_all() in turn
+ * sets up a revision walk.
+ */
+ reset_revision_walk();
+ init_revisions(revs, NULL);
+ setup_revisions(0, NULL, revs, NULL);
+ for_each_glob_ref_in(add_bisect_ref, bad, "refs/bisect/", &cb);
+ cb.object_flags = UNINTERESTING;
+ for_each_glob_ref_in(add_bisect_ref, good, "refs/bisect/", &cb);
+ if (prepare_revision_walk(revs))
+ res = error(_("revision walk setup failed\n"));
+
+ free(good);
+ free(bad);
+ return res;
+}
+
+static int bisect_skipped_commits(struct bisect_terms *terms)
+{
+ int res;
+ FILE *fp = NULL;
+ struct rev_info revs;
+ struct commit *commit;
+ struct pretty_print_context pp = {0};
+ struct strbuf commit_name = STRBUF_INIT;
+
+ res = prepare_revs(terms, &revs);
+ if (res)
+ return res;
+
+ fp = fopen(git_path_bisect_log(), "a");
+ if (!fp)
+ return error_errno(_("could not open '%s' for appending"),
+ git_path_bisect_log());
+
+ if (fprintf(fp, "# only skipped commits left to test\n") < 0)
+ return error_errno(_("failed to write to '%s'"), git_path_bisect_log());
+
+ while ((commit = get_revision(&revs)) != NULL) {
+ strbuf_reset(&commit_name);
+ format_commit_message(commit, "%s",
+ &commit_name, &pp);
+ fprintf(fp, "# possible first %s commit: [%s] %s\n",
+ terms->term_bad, oid_to_hex(&commit->object.oid),
+ commit_name.buf);
+ }
+
+ /*
+ * Reset the flags used by revision walks in case
+ * there is another revision walk after this one.
+ */
+ reset_revision_walk();
+
+ strbuf_release(&commit_name);
+ fclose(fp);
+ return 0;
+}
+
+static int bisect_successful(struct bisect_terms *terms)
+{
+ struct object_id oid;
+ struct commit *commit;
+ struct pretty_print_context pp = {0};
+ struct strbuf commit_name = STRBUF_INIT;
+ char *bad_ref = xstrfmt("refs/bisect/%s",terms->term_bad);
+ int res;
+
+ read_ref(bad_ref, &oid);
+ commit = lookup_commit_reference_by_name(bad_ref);
+ format_commit_message(commit, "%s", &commit_name, &pp);
+
+ res = append_to_file(git_path_bisect_log(), "# first %s commit: [%s] %s\n",
+ terms->term_bad, oid_to_hex(&commit->object.oid),
+ commit_name.buf);
+
+ strbuf_release(&commit_name);
+ free(bad_ref);
+ return res;
+}
+
+static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix)
+{
+ int no_checkout;
+ enum bisect_error res;
+
+ bisect_autostart(terms);
+ if (bisect_next_check(terms, terms->term_good))
+ return BISECT_FAILED;
+
+ no_checkout = ref_exists("BISECT_HEAD");
+
+ /* Perform all bisection computation */
+ res = bisect_next_all(the_repository, prefix);
+
+ if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {
+ res = bisect_successful(terms);
+ return res ? res : BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND;
+ } else if (res == BISECT_ONLY_SKIPPED_LEFT) {
+ res = bisect_skipped_commits(terms);
+ return res ? res : BISECT_ONLY_SKIPPED_LEFT;
+ }
+ return res;
+}
+
+static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix)
+{
+ if (bisect_next_check(terms, NULL))
+ return BISECT_OK;
+
+ return bisect_next(terms, prefix);
+}
+
static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)
{
int no_checkout = 0;
@@ -699,7 +858,9 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
BISECT_NEXT_CHECK,
BISECT_TERMS,
BISECT_START,
- BISECT_AUTOSTART,
+ BISECT_NEXT,
+ BISECT_AUTO_NEXT,
+ BISECT_AUTOSTART
} cmdmode = 0;
int res = 0, nolog = 0;
struct option options[] = {
@@ -723,6 +884,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
N_("print out the bisect terms"), BISECT_TERMS),
OPT_CMDMODE(0, "bisect-start", &cmdmode,
N_("start the bisect session"), BISECT_START),
+ OPT_CMDMODE(0, "bisect-next", &cmdmode,
+ N_("find the next bisection commit"), BISECT_NEXT),
+ OPT_CMDMODE(0, "bisect-auto-next", &cmdmode,
+ N_("verify the next bisection state then checkout the next bisection commit"), BISECT_AUTO_NEXT),
OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
N_("start the bisection if it has not yet been started"), BISECT_AUTOSTART),
OPT_BOOL(0, "no-log", &nolog,
@@ -784,6 +949,18 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
set_terms(&terms, "bad", "good");
res = bisect_start(&terms, argv, argc);
break;
+ case BISECT_NEXT:
+ if (argc)
+ return error(_("--bisect-next requires 0 arguments"));
+ get_terms(&terms);
+ res = bisect_next(&terms, prefix);
+ break;
+ case BISECT_AUTO_NEXT:
+ if (argc)
+ return error(_("--bisect-auto-next requires 0 arguments"));
+ get_terms(&terms);
+ res = bisect_auto_next(&terms, prefix);
+ break;
case BISECT_AUTOSTART:
if (argc)
return error(_("--bisect-autostart does not accept arguments"));
@@ -799,7 +976,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
* Handle early success
* From check_merge_bases > check_good_are_ancestors_of_bad > bisect_next_all
*/
- if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE)
+ if ((res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) || (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND))
res = BISECT_OK;
return -res;
diff --git a/git-bisect.sh b/git-bisect.sh
index 9ca583d964..59424f5b37 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -65,8 +65,7 @@ bisect_start() {
#
# Check if we can proceed to the next bisect state.
#
- get_terms
- bisect_auto_next
+ git bisect--helper --bisect-auto-next || exit
trap '-' 0
}
@@ -119,45 +118,7 @@ bisect_state() {
*)
usage ;;
esac
- bisect_auto_next
-}
-
-bisect_auto_next() {
- git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD && bisect_next || :
-}
-
-bisect_next() {
- case "$#" in 0) ;; *) usage ;; esac
- git bisect--helper --bisect-autostart
- git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD $TERM_GOOD|| exit
-
- # Perform all bisection computation, display and checkout
- git bisect--helper --next-all
- res=$?
-
- # Check if we should exit because bisection is finished
- if test $res -eq 10
- then
- bad_rev=$(git show-ref --hash --verify refs/bisect/$TERM_BAD)
- bad_commit=$(git show-branch $bad_rev)
- echo "# first $TERM_BAD commit: $bad_commit" >>"$GIT_DIR/BISECT_LOG"
- exit 0
- elif test $res -eq 2
- then
- echo "# only skipped commits left to test" >>"$GIT_DIR/BISECT_LOG"
- good_revs=$(git for-each-ref --format="%(objectname)" "refs/bisect/$TERM_GOOD-*")
- for skipped in $(git rev-list refs/bisect/$TERM_BAD --not $good_revs)
- do
- skipped_commit=$(git show-branch $skipped)
- echo "# possible first $TERM_BAD commit: $skipped_commit" >>"$GIT_DIR/BISECT_LOG"
- done
- exit $res
- fi
-
- # Check for an error in the bisection process
- test $res -ne 0 && exit $res
-
- return 0
+ git bisect--helper --bisect-auto-next
}
bisect_visualize() {
@@ -213,7 +174,7 @@ bisect_replay () {
esac
done <"$file"
IFS="$oIFS"
- bisect_auto_next
+ git bisect--helper --bisect-auto-next
}
bisect_run () {
@@ -310,7 +271,7 @@ case "$#" in
bisect_skip "$@" ;;
next)
# Not sure we want "next" at the UI level anymore.
- bisect_next "$@" ;;
+ git bisect--helper --bisect-next "$@" || exit ;;
visualize|view)
bisect_visualize "$@" ;;
reset)
--
2.25.0
^ permalink raw reply related
* [PATCH v6 07/13] bisect--helper: finish porting `bisect_start()` to C
From: Miriam Rubio @ 2020-08-28 12:46 UTC (permalink / raw)
To: git
Cc: Pranit Bauva, Christian Couder, Johannes Schindelin,
Tanushree Tumane, Miriam Rubio
In-Reply-To: <20200828124617.60618-1-mirucam@gmail.com>
From: Pranit Bauva <pranit.bauva@gmail.com>
Add the subcommand to `git bisect--helper` and call it from
git-bisect.sh.
With the conversion of `bisect_auto_next()` from shell to C in a
previous commit, `bisect_start()` can now be fully ported to C.
So let's complete the `--bisect-start` subcommand of
`git bisect--helper` so that it fully implements `bisect_start()`,
and let's use this subcommand in `git-bisect.sh` instead of
`bisect_start()`.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
builtin/bisect--helper.c | 56 ++++++++++++++++++++++++++++------------
git-bisect.sh | 26 ++-----------------
2 files changed, 41 insertions(+), 41 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 5d443829e0..56da407871 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -85,6 +85,19 @@ static int one_of(const char *term, ...)
return res;
}
+/*
+ * return code BISECT_INTERNAL_SUCCESS_MERGE_BASE
+ * and BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND are codes
+ * that indicate special success.
+ */
+
+static int is_bisect_success(enum bisect_error res)
+{
+ return !res ||
+ res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND ||
+ res == BISECT_INTERNAL_SUCCESS_MERGE_BASE;
+}
+
static int write_in_file(const char *path, const char *mode, const char *format, va_list args)
{
FILE *fp = NULL;
@@ -610,12 +623,13 @@ static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char
return bisect_next(terms, prefix);
}
-static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)
+static enum bisect_error bisect_start(struct bisect_terms *terms, const char **argv, int argc)
{
int no_checkout = 0;
int first_parent_only = 0;
int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
- int flags, pathspec_pos, res = 0;
+ int flags, pathspec_pos;
+ enum bisect_error res = BISECT_OK;
struct string_list revs = STRING_LIST_INIT_DUP;
struct string_list states = STRING_LIST_INIT_DUP;
struct strbuf start_head = STRBUF_INIT;
@@ -675,9 +689,12 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)
return error(_("unrecognized option: '%s'"), arg);
} else {
char *commit_id = xstrfmt("%s^{commit}", arg);
- if (get_oid(commit_id, &oid) && has_double_dash)
- die(_("'%s' does not appear to be a valid "
- "revision"), arg);
+ if (get_oid(commit_id, &oid) && has_double_dash) {
+ error(_("'%s' does not appear to be a valid "
+ "revision"), arg);
+ free(commit_id);
+ return BISECT_FAILED;
+ }
string_list_append(&revs, oid_to_hex(&oid));
free(commit_id);
@@ -755,14 +772,7 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)
* Get rid of any old bisect state.
*/
if (bisect_clean_state())
- return -1;
-
- /*
- * In case of mistaken revs or checkout error, or signals received,
- * "bisect_auto_next" below may exit or misbehave.
- * We have to trap this to be able to clean up using
- * "bisect_clean_state".
- */
+ return BISECT_FAILED;
/*
* Write new start state
@@ -779,7 +789,7 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)
}
if (update_ref(NULL, "BISECT_HEAD", &oid, NULL, 0,
UPDATE_REFS_MSG_ON_ERR)) {
- res = -1;
+ res = BISECT_FAILED;
goto finish;
}
}
@@ -791,25 +801,37 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)
for (i = 0; i < states.nr; i++)
if (bisect_write(states.items[i].string,
revs.items[i].string, terms, 1)) {
- res = -1;
+ res = BISECT_FAILED;
goto finish;
}
if (must_write_terms && write_terms(terms->term_bad,
terms->term_good)) {
- res = -1;
+ res = BISECT_FAILED;
goto finish;
}
res = bisect_append_log_quoted(argv);
if (res)
- res = -1;
+ res = BISECT_FAILED;
finish:
string_list_clear(&revs, 0);
string_list_clear(&states, 0);
strbuf_release(&start_head);
strbuf_release(&bisect_names);
+ if (res)
+ return res;
+
+ res = bisect_auto_next(terms, NULL);
+ /*
+ * In case of mistaken revs or checkout error,
+ * "bisect_auto_next" above may exit or misbehave.
+ * We have to handle this to be able to clean up using
+ * "bisect_clean_state".
+ */
+ if (!is_bisect_success(res))
+ bisect_clean_state();
return res;
}
diff --git a/git-bisect.sh b/git-bisect.sh
index 59424f5b37..356264caf0 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -49,27 +49,6 @@ bisect_head()
fi
}
-bisect_start() {
- git bisect--helper --bisect-start $@ || exit
-
- #
- # Change state.
- # In case of mistaken revs or checkout error, or signals received,
- # "bisect_auto_next" below may exit or misbehave.
- # We have to trap this to be able to clean up using
- # "bisect_clean_state".
- #
- trap 'git bisect--helper --bisect-clean-state' 0
- trap 'exit 255' 1 2 3 15
-
- #
- # Check if we can proceed to the next bisect state.
- #
- git bisect--helper --bisect-auto-next || exit
-
- trap '-' 0
-}
-
bisect_skip() {
all=''
for arg in "$@"
@@ -163,8 +142,7 @@ bisect_replay () {
get_terms
case "$command" in
start)
- cmd="bisect_start $rev $tail"
- eval "$cmd" ;;
+ eval "git bisect--helper --bisect-start $rev $tail" ;;
"$TERM_GOOD"|"$TERM_BAD"|skip)
git bisect--helper --bisect-write "$command" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit;;
terms)
@@ -264,7 +242,7 @@ case "$#" in
help)
git bisect -h ;;
start)
- bisect_start "$@" ;;
+ git bisect--helper --bisect-start "$@" ;;
bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD")
bisect_state "$cmd" "$@" ;;
skip)
--
2.25.0
^ permalink raw reply related
* [PATCH v6 08/13] bisect--helper: retire `--bisect-clean-state` subcommand
From: Miriam Rubio @ 2020-08-28 12:46 UTC (permalink / raw)
To: git
Cc: Pranit Bauva, Lars Schneider, Christian Couder, Tanushree Tumane,
Miriam Rubio
In-Reply-To: <20200828124617.60618-1-mirucam@gmail.com>
From: Pranit Bauva <pranit.bauva@gmail.com>
The `--bisect-clean-state` subcommand is no longer used from the
git-bisect.sh shell script. Instead the function
`bisect_clean_state()` is directly called from the C
implementation.
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
builtin/bisect--helper.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 56da407871..106e5b788e 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -22,7 +22,6 @@ static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT")
static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --next-all"),
N_("git bisect--helper --write-terms <bad_term> <good_term>"),
- N_("git bisect--helper --bisect-clean-state"),
N_("git bisect--helper --bisect-reset [<commit>]"),
N_("git bisect--helper --bisect-write [--no-log] <state> <revision> <good_term> <bad_term>"),
N_("git bisect--helper --bisect-check-and-set-terms <command> <good_term> <bad_term>"),
@@ -872,7 +871,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
enum {
NEXT_ALL = 1,
WRITE_TERMS,
- BISECT_CLEAN_STATE,
CHECK_EXPECTED_REVS,
BISECT_RESET,
BISECT_WRITE,
@@ -890,8 +888,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
N_("perform 'git bisect next'"), NEXT_ALL),
OPT_CMDMODE(0, "write-terms", &cmdmode,
N_("write the terms to .git/BISECT_TERMS"), WRITE_TERMS),
- OPT_CMDMODE(0, "bisect-clean-state", &cmdmode,
- N_("cleanup the bisection state"), BISECT_CLEAN_STATE),
OPT_CMDMODE(0, "check-expected-revs", &cmdmode,
N_("check for expected revs"), CHECK_EXPECTED_REVS),
OPT_CMDMODE(0, "bisect-reset", &cmdmode,
@@ -933,10 +929,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
if (argc != 2)
return error(_("--write-terms requires two arguments"));
return write_terms(argv[0], argv[1]);
- case BISECT_CLEAN_STATE:
- if (argc != 0)
- return error(_("--bisect-clean-state requires no arguments"));
- return bisect_clean_state();
case CHECK_EXPECTED_REVS:
check_expected_revs(argv, argc);
return 0;
--
2.25.0
^ permalink raw reply related
* [PATCH v6 09/13] bisect--helper: retire `--next-all` subcommand
From: Miriam Rubio @ 2020-08-28 12:46 UTC (permalink / raw)
To: git
Cc: Pranit Bauva, Lars Schneider, Christian Couder, Tanushree Tumane,
Miriam Rubio
In-Reply-To: <20200828124617.60618-1-mirucam@gmail.com>
From: Pranit Bauva <pranit.bauva@gmail.com>
The `--next-all` subcommand is no longer used from the git-bisect.sh
shell script. Instead the function `bisect_next_all()` is called from
the C implementation of `bisect_next()`.
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
builtin/bisect--helper.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 106e5b788e..0a32dbb68f 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -20,7 +20,6 @@ static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT")
static const char * const git_bisect_helper_usage[] = {
- N_("git bisect--helper --next-all"),
N_("git bisect--helper --write-terms <bad_term> <good_term>"),
N_("git bisect--helper --bisect-reset [<commit>]"),
N_("git bisect--helper --bisect-write [--no-log] <state> <revision> <good_term> <bad_term>"),
@@ -869,8 +868,7 @@ static int bisect_autostart(struct bisect_terms *terms)
int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
{
enum {
- NEXT_ALL = 1,
- WRITE_TERMS,
+ WRITE_TERMS = 1,
CHECK_EXPECTED_REVS,
BISECT_RESET,
BISECT_WRITE,
@@ -884,8 +882,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
} cmdmode = 0;
int res = 0, nolog = 0;
struct option options[] = {
- OPT_CMDMODE(0, "next-all", &cmdmode,
- N_("perform 'git bisect next'"), NEXT_ALL),
OPT_CMDMODE(0, "write-terms", &cmdmode,
N_("write the terms to .git/BISECT_TERMS"), WRITE_TERMS),
OPT_CMDMODE(0, "check-expected-revs", &cmdmode,
@@ -922,9 +918,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
usage_with_options(git_bisect_helper_usage, options);
switch (cmdmode) {
- case NEXT_ALL:
- res = bisect_next_all(the_repository, prefix);
- break;
case WRITE_TERMS:
if (argc != 2)
return error(_("--write-terms requires two arguments"));
--
2.25.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox