* [RFC/PATCH] bisect--helper: `bisect_clean_state` shell function in C
@ 2016-05-30 18:21 Pranit Bauva
2016-05-30 21:48 ` Christian Couder
2016-05-31 4:25 ` Michael Haggerty
0 siblings, 2 replies; 5+ messages in thread
From: Pranit Bauva @ 2016-05-30 18:21 UTC (permalink / raw)
To: git
Cc: Pranit Bauva, larsxschneider, chriscool, christian.couder,
mhagger, peff
Reimplement `bisect_clean_state` shell function in C and add a
`bisect-clean-state` subcommand to `git bisect--helper` to call it from
git-bisect.sh .
Using `bisect_clean_state` subcommand is a measure to port shell
function to C so as to use the existing test suite. As more functions
are ported, this subcommand will be retired and will be called by
bisect_reset() and bisect_start().
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
This patch contains a bug. I have tried to identify the bug and I suppose it
exists in do_for_each_entry_in_dir(). I have reproduced the debugging session
at this link[1]. I have seen that some patches in mailing list regarding
iterating over refs. Will those affect this? Or is this bug fixed in those
patches?
[1]: http://paste.ubuntu.com/16830752/
builtin/bisect--helper.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++-
git-bisect.sh | 26 +++----------------------
2 files changed, 52 insertions(+), 24 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 2b21c02..a20c9a5 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -3,10 +3,21 @@
#include "parse-options.h"
#include "bisect.h"
#include "refs.h"
+#include "dir.h"
+
+static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
+static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
+static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
+static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
+static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
+static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
+static GIT_PATH_FUNC(git_path_head_name, "head-name")
+static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --next-all [--no-checkout]"),
N_("git bisect--helper --write-terms <bad_term> <good_term>"),
+ N_("git bisect--helper --bisect-clean-state"),
NULL
};
@@ -79,11 +90,42 @@ int write_terms(const char *bad, const char *good)
strbuf_release(&content);
return (res < 0) ? -1 : 0;
}
+
+int remove_bisect_ref(const char *refname, const struct object_id *oid,
+ int flag, void *cb_data)
+{
+ char *ref;
+ ref = xstrfmt("refs/bisect/%s", refname);
+ if (delete_ref(ref, oid->hash, flag))
+ return error(_("couldn't delete the ref %s\n"), ref);
+ return 0;
+}
+
+int bisect_clean_state(void)
+{
+ for_each_ref_in("refs/bisect/", remove_bisect_ref, NULL);
+
+ remove_path(git_path_bisect_expected_rev());
+ remove_path(git_path_bisect_ancestors_ok());
+ remove_path(git_path_bisect_log());
+ remove_path(git_path_bisect_names());
+ remove_path(git_path_bisect_run());
+ remove_path(git_path_bisect_terms());
+ /* Cleanup head-name if it got left by an old version of git-bisect */
+ remove_path(git_path_head_name());
+ delete_ref("BISECT_HEAD", NULL, 0);
+ /* Cleanup BISECT_START last */
+ remove_path(git_path_bisect_start());
+
+ return 0;
+}
+
int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
{
enum {
NEXT_ALL = 1,
- WRITE_TERMS
+ WRITE_TERMS,
+ BISECT_CLEAN_STATE
} cmdmode = 0;
int no_checkout = 0;
struct option options[] = {
@@ -91,6 +133,8 @@ 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_BOOL(0, "no-checkout", &no_checkout,
N_("update BISECT_HEAD instead of checking out the current commit")),
OPT_END()
@@ -109,6 +153,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
if (argc != 2)
die(_("--write-terms requires two arguments"));
return write_terms(argv[0], argv[1]);
+ case BISECT_CLEAN_STATE:
+ if (argc != 0)
+ die(_("--bisect-clean-state requires no arguments"));
+ return bisect_clean_state();
default:
die("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index 2dd7ec5..a937d1c 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -187,7 +187,7 @@ bisect_start() {
#
# Get rid of any old bisect state.
#
- bisect_clean_state || exit
+ git bisect--helper --bisect-clean-state || exit
#
# Change state.
@@ -196,7 +196,7 @@ bisect_start() {
# We have to trap this to be able to clean up using
# "bisect_clean_state".
#
- trap 'bisect_clean_state' 0
+ trap 'git bisect--helper --bisect-clean-state' 0
trap 'exit 255' 1 2 3 15
#
@@ -430,27 +430,7 @@ bisect_reset() {
die "$(eval_gettext "Could not check out original HEAD '\$branch'.
Try 'git bisect reset <commit>'.")"
fi
- bisect_clean_state
-}
-
-bisect_clean_state() {
- # There may be some refs packed during bisection.
- git for-each-ref --format='%(refname) %(objectname)' refs/bisect/\* |
- while read ref hash
- do
- git update-ref -d $ref $hash || exit
- done
- rm -f "$GIT_DIR/BISECT_EXPECTED_REV" &&
- rm -f "$GIT_DIR/BISECT_ANCESTORS_OK" &&
- rm -f "$GIT_DIR/BISECT_LOG" &&
- rm -f "$GIT_DIR/BISECT_NAMES" &&
- rm -f "$GIT_DIR/BISECT_RUN" &&
- rm -f "$GIT_DIR/BISECT_TERMS" &&
- # Cleanup head-name if it got left by an old version of git-bisect
- rm -f "$GIT_DIR/head-name" &&
- git update-ref -d --no-deref BISECT_HEAD &&
- # clean up BISECT_START last
- rm -f "$GIT_DIR/BISECT_START"
+ git bisect--helper --bisect-clean-state || exit
}
bisect_replay () {
--
2.8.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC/PATCH] bisect--helper: `bisect_clean_state` shell function in C
2016-05-30 18:21 [RFC/PATCH] bisect--helper: `bisect_clean_state` shell function in C Pranit Bauva
@ 2016-05-30 21:48 ` Christian Couder
2016-05-31 6:33 ` Pranit Bauva
2016-05-31 4:25 ` Michael Haggerty
1 sibling, 1 reply; 5+ messages in thread
From: Christian Couder @ 2016-05-30 21:48 UTC (permalink / raw)
To: Pranit Bauva
Cc: git, Lars Schneider, Christian Couder, Michael Haggerty,
Jeff King
On Mon, May 30, 2016 at 8:21 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> ---
> This patch contains a bug. I have tried to identify the bug and I suppose it
> exists in do_for_each_entry_in_dir(). I have reproduced the debugging session
> at this link[1]. I have seen that some patches in mailing list regarding
> iterating over refs. Will those affect this? Or is this bug fixed in those
> patches?
>
> [1]: http://paste.ubuntu.com/16830752/
The debug session seems to use code source from a previous version of
this patch.
Also it is not cear in which context you run git under gdb. What have
you done before?
And we don't see a crash. Could you show the crash and run the "bt"
command in gdb to get a backtrace?
> @@ -79,11 +90,42 @@ int write_terms(const char *bad, const char *good)
> strbuf_release(&content);
> return (res < 0) ? -1 : 0;
> }
> +
> +int remove_bisect_ref(const char *refname, const struct object_id *oid,
> + int flag, void *cb_data)
> +{
> + char *ref;
> + ref = xstrfmt("refs/bisect/%s", refname);
You could save one line by concatenating the 2 above lines.
> + if (delete_ref(ref, oid->hash, flag))
> + return error(_("couldn't delete the ref %s\n"), ref);
> + return 0;
You need to free "ref".
> +}
Thanks,
Christian.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC/PATCH] bisect--helper: `bisect_clean_state` shell function in C
2016-05-30 21:48 ` Christian Couder
@ 2016-05-31 6:33 ` Pranit Bauva
0 siblings, 0 replies; 5+ messages in thread
From: Pranit Bauva @ 2016-05-31 6:33 UTC (permalink / raw)
To: Christian Couder
Cc: git, Lars Schneider, Christian Couder, Michael Haggerty,
Jeff King
Hey Christian,
On Tue, May 31, 2016 at 3:18 AM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Mon, May 30, 2016 at 8:21 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> ---
>> This patch contains a bug. I have tried to identify the bug and I suppose it
>> exists in do_for_each_entry_in_dir(). I have reproduced the debugging session
>> at this link[1]. I have seen that some patches in mailing list regarding
>> iterating over refs. Will those affect this? Or is this bug fixed in those
>> patches?
>>
>> [1]: http://paste.ubuntu.com/16830752/
>
> The debug session seems to use code source from a previous version of
> this patch.
> Also it is not cear in which context you run git under gdb. What have
> you done before?
> And we don't see a crash. Could you show the crash and run the "bt"
> command in gdb to get a backtrace?
The segmentation fault will occur if I move to the next step ie.
evaluate the value entry->flag.
>> @@ -79,11 +90,42 @@ int write_terms(const char *bad, const char *good)
>> strbuf_release(&content);
>> return (res < 0) ? -1 : 0;
>> }
>> +
>> +int remove_bisect_ref(const char *refname, const struct object_id *oid,
>> + int flag, void *cb_data)
>> +{
>> + char *ref;
>> + ref = xstrfmt("refs/bisect/%s", refname);
>
> You could save one line by concatenating the 2 above lines.
Sure!
>> + if (delete_ref(ref, oid->hash, flag))
>> + return error(_("couldn't delete the ref %s\n"), ref);
>> + return 0;
>
> You need to free "ref".
Sure!
Regards,
Pranit Bauva
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC/PATCH] bisect--helper: `bisect_clean_state` shell function in C
2016-05-30 18:21 [RFC/PATCH] bisect--helper: `bisect_clean_state` shell function in C Pranit Bauva
2016-05-30 21:48 ` Christian Couder
@ 2016-05-31 4:25 ` Michael Haggerty
2016-05-31 6:40 ` Pranit Bauva
1 sibling, 1 reply; 5+ messages in thread
From: Michael Haggerty @ 2016-05-31 4:25 UTC (permalink / raw)
To: Pranit Bauva, git; +Cc: larsxschneider, chriscool, christian.couder, peff
On 05/30/2016 08:21 PM, Pranit Bauva wrote:
> Reimplement `bisect_clean_state` shell function in C and add a
> `bisect-clean-state` subcommand to `git bisect--helper` to call it from
> git-bisect.sh .
>
> Using `bisect_clean_state` subcommand is a measure to port shell
> function to C so as to use the existing test suite. As more functions
> are ported, this subcommand will be retired and will be called by
> bisect_reset() and bisect_start().
>
> Mentored-by: Lars Schneider <larsxschneider@gmail.com>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> This patch contains a bug. I have tried to identify the bug and I suppose it
> exists in do_for_each_entry_in_dir(). I have reproduced the debugging session
> at this link[1]. I have seen that some patches in mailing list regarding
> iterating over refs. Will those affect this? Or is this bug fixed in those
> patches?
The problem is that it is not legal to modify references while iterating
over them. See [1]. Your remove_bisect_ref() callback function deletes
references, which modifies the reference cache that is being iterated over.
Instead I suggest that your remove_bisect_ref() add the references to a
string_list, then call delete_refs() *after* the iteration is over.
Alternatively, you can change remove_bisect_ref() to call
ref_transaction_delete() to add reference deletions to a
ref_transaction, then call ref_transaction_commit() after the iteration
is over. See the rm() function in builtin/remote.c [2] for an example.
[1]
https://github.com/git/git/blob/f3913c2d03abc660140678a9e14dac399f847647/refs.h#L176-L184
[2]
https://github.com/git/git/blob/f3913c2d03abc660140678a9e14dac399f847647/builtin/remote.c#L738
> [...]
Michael
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC/PATCH] bisect--helper: `bisect_clean_state` shell function in C
2016-05-31 4:25 ` Michael Haggerty
@ 2016-05-31 6:40 ` Pranit Bauva
0 siblings, 0 replies; 5+ messages in thread
From: Pranit Bauva @ 2016-05-31 6:40 UTC (permalink / raw)
To: Michael Haggerty
Cc: Git List, Lars Schneider, Christian Couder, Christian Couder,
Jeff King
Hey Michael,
On Tue, May 31, 2016 at 9:55 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 05/30/2016 08:21 PM, Pranit Bauva wrote:
>> Reimplement `bisect_clean_state` shell function in C and add a
>> `bisect-clean-state` subcommand to `git bisect--helper` to call it from
>> git-bisect.sh .
>>
>> Using `bisect_clean_state` subcommand is a measure to port shell
>> function to C so as to use the existing test suite. As more functions
>> are ported, this subcommand will be retired and will be called by
>> bisect_reset() and bisect_start().
>>
>> Mentored-by: Lars Schneider <larsxschneider@gmail.com>
>> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> This patch contains a bug. I have tried to identify the bug and I suppose it
>> exists in do_for_each_entry_in_dir(). I have reproduced the debugging session
>> at this link[1]. I have seen that some patches in mailing list regarding
>> iterating over refs. Will those affect this? Or is this bug fixed in those
>> patches?
>
> The problem is that it is not legal to modify references while iterating
> over them. See [1]. Your remove_bisect_ref() callback function deletes
> references, which modifies the reference cache that is being iterated over.
Thanks for explaining this. I wasn't aware about this.
> Instead I suggest that your remove_bisect_ref() add the references to a
> string_list, then call delete_refs() *after* the iteration is over.
> Alternatively, you can change remove_bisect_ref() to call
> ref_transaction_delete() to add reference deletions to a
> ref_transaction, then call ref_transaction_commit() after the iteration
> is over. See the rm() function in builtin/remote.c [2] for an example.
I have gone through the links and I think I will stick with the first
one because I don't see how the latter one would be better.
> [1]
> https://github.com/git/git/blob/f3913c2d03abc660140678a9e14dac399f847647/refs.h#L176-L184
> [2]
> https://github.com/git/git/blob/f3913c2d03abc660140678a9e14dac399f847647/builtin/remote.c#L738
>
>> [...]
>
Regards,
Pranit Bauva
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-05-31 6:41 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-05-30 18:21 [RFC/PATCH] bisect--helper: `bisect_clean_state` shell function in C Pranit Bauva
2016-05-30 21:48 ` Christian Couder
2016-05-31 6:33 ` Pranit Bauva
2016-05-31 4:25 ` Michael Haggerty
2016-05-31 6:40 ` Pranit Bauva
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 NNTP newsgroup(s).