{"thread":{"id":"42490","subject":"[RFC/PATCH] bisect--helper: `bisect_clean_state` shell function in C","startedAt":"2016-05-30T18:21:48Z","lastAt":"2016-05-31T06:40:58Z","messageCount":5,"participants":["Pranit Bauva","Christian Couder","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"287838","messageId":"20160530182148.18801-1-pranit.bauva@gmail.com","threadId":"42490","inReplyTo":null,"subject":"[RFC/PATCH] bisect--helper: `bisect_clean_state` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-05-30T18:21:48Z","receivedAt":"2016-05-30T18:21:48Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Reimplement `bisect_clean_state` shell function in C and add a\n`bisect-clean-state` subcommand to `git bisect--helper` to call it from\ngit-bisect.sh .\n\nUsing `bisect_clean_state` subcommand is a measure to port shell\nfunction to C so as to use the existing test suite. As more functions\nare ported, this subcommand will be retired and will be called by\nbisect_reset() and bisect_start().\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nSigned-off-by: Pranit Bauva \n---\nThis patch contains a bug. I have tried to identify the bug and I suppose it\nexists in do_for_each_entry_in_dir(). I have reproduced the debugging session\nat this link[1]. I have seen that some patches in mailing list regarding\niterating over refs. Will those affect this? Or is this bug fixed in those\npatches?\n\n[1]: http://paste.ubuntu.com/16830752/\n\n builtin/bisect--helper.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++-\n git-bisect.sh | 26 +++----------------------\n 2 files changed, 52 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 2b21c02..a20c9a5 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -3,10 +3,21 @@\n #include \"parse-options.h\"\n #include \"bisect.h\"\n #include \"refs.h\"\n+#include \"dir.h\"\n+\n+static GIT_PATH_FUNC(git_path_bisect_expected_rev, \"BISECT_EXPECTED_REV\")\n+static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, \"BISECT_ANCESTORS_OK\")\n+static GIT_PATH_FUNC(git_path_bisect_log, \"BISECT_LOG\")\n+static GIT_PATH_FUNC(git_path_bisect_names, \"BISECT_NAMES\")\n+static GIT_PATH_FUNC(git_path_bisect_run, \"BISECT_RUN\")\n+static GIT_PATH_FUNC(git_path_bisect_terms, \"BISECT_TERMS\")\n+static GIT_PATH_FUNC(git_path_head_name, \"head-name\")\n+static GIT_PATH_FUNC(git_path_bisect_start, \"BISECT_START\")\n \n static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --next-all [--no-checkout]\"),\n \tN_(\"git bisect--helper --write-terms \"),\n+\tN_(\"git bisect--helper --bisect-clean-state\"),\n \tNULL\n };\n \n@@ -79,11 +90,42 @@ int write_terms(const char *bad, const char *good)\n \tstrbuf_release(&content);\n \treturn (res < 0) ? -1 : 0;\n }\n+\n+int remove_bisect_ref(const char *refname, const struct object_id *oid,\n+\t\t int flag, void *cb_data)\n+{\n+\tchar *ref;\n+\tref = xstrfmt(\"refs/bisect/%s\", refname);\n+\tif (delete_ref(ref, oid->hash, flag))\n+\t\treturn error(_(\"couldn't delete the ref %s\\n\"), ref);\n+\treturn 0;\n+}\n+\n+int bisect_clean_state(void)\n+{\n+\tfor_each_ref_in(\"refs/bisect/\", remove_bisect_ref, NULL);\n+\n+\tremove_path(git_path_bisect_expected_rev());\n+\tremove_path(git_path_bisect_ancestors_ok());\n+\tremove_path(git_path_bisect_log());\n+\tremove_path(git_path_bisect_names());\n+\tremove_path(git_path_bisect_run());\n+\tremove_path(git_path_bisect_terms());\n+\t/* Cleanup head-name if it got left by an old version of git-bisect */\n+\tremove_path(git_path_head_name());\n+\tdelete_ref(\"BISECT_HEAD\", NULL, 0);\n+\t/* Cleanup BISECT_START last */\n+\tremove_path(git_path_bisect_start());\n+\n+\treturn 0;\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n \t\tNEXT_ALL = 1,\n-\t\tWRITE_TERMS\n+\t\tWRITE_TERMS,\n+\t\tBISECT_CLEAN_STATE\n \t} cmdmode = 0;\n \tint no_checkout = 0;\n \tstruct option options[] = {\n@@ -91,6 +133,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"perform 'git bisect next'\"), NEXT_ALL),\n \t\tOPT_CMDMODE(0, \"write-terms\", &cmdmode,\n \t\t\t N_(\"write the terms to .git/BISECT_TERMS\"), WRITE_TERMS),\n+\t\tOPT_CMDMODE(0, \"bisect-clean-state\", &cmdmode,\n+\t\t\t N_(\"cleanup the bisection state\"), BISECT_CLEAN_STATE),\n \t\tOPT_BOOL(0, \"no-checkout\", &no_checkout,\n \t\t\t N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n \t\tOPT_END()\n@@ -109,6 +153,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tif (argc != 2)\n \t\t\tdie(_(\"--write-terms requires two arguments\"));\n \t\treturn write_terms(argv[0], argv[1]);\n+\tcase BISECT_CLEAN_STATE:\n+\t\tif (argc != 0)\n+\t\t\tdie(_(\"--bisect-clean-state requires no arguments\"));\n+\t\treturn bisect_clean_state();\n \tdefault:\n \t\tdie(\"BUG: unknown subcommand '%d'\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 2dd7ec5..a937d1c 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -187,7 +187,7 @@ bisect_start() {\n \t#\n \t# Get rid of any old bisect state.\n \t#\n-\tbisect_clean_state || exit\n+\tgit bisect--helper --bisect-clean-state || exit\n \n \t#\n \t# Change state.\n@@ -196,7 +196,7 @@ bisect_start() {\n \t# We have to trap this to be able to clean up using\n \t# \"bisect_clean_state\".\n \t#\n-\ttrap 'bisect_clean_state' 0\n+\ttrap 'git bisect--helper --bisect-clean-state' 0\n \ttrap 'exit 255' 1 2 3 15\n \n \t#\n@@ -430,27 +430,7 @@ bisect_reset() {\n \t\tdie \"$(eval_gettext \"Could not check out original HEAD '\\$branch'.\n Try 'git bisect reset '.\")\"\n \tfi\n-\tbisect_clean_state\n-}\n-\n-bisect_clean_state() {\n-\t# There may be some refs packed during bisection.\n-\tgit for-each-ref --format='%(refname) %(objectname)' refs/bisect/\\* |\n-\twhile read ref hash\n-\tdo\n-\t\tgit update-ref -d $ref $hash || exit\n-\tdone\n-\trm -f \"$GIT_DIR/BISECT_EXPECTED_REV\" &&\n-\trm -f \"$GIT_DIR/BISECT_ANCESTORS_OK\" &&\n-\trm -f \"$GIT_DIR/BISECT_LOG\" &&\n-\trm -f \"$GIT_DIR/BISECT_NAMES\" &&\n-\trm -f \"$GIT_DIR/BISECT_RUN\" &&\n-\trm -f \"$GIT_DIR/BISECT_TERMS\" &&\n-\t# Cleanup head-name if it got left by an old version of git-bisect\n-\trm -f \"$GIT_DIR/head-name\" &&\n-\tgit update-ref -d --no-deref BISECT_HEAD &&\n-\t# clean up BISECT_START last\n-\trm -f \"$GIT_DIR/BISECT_START\"\n+\tgit bisect--helper --bisect-clean-state || exit\n }\n \n bisect_replay () {\n-- \n2.8.3\n"},{"id":"287854","messageId":"CAP8UFD2wAKN2CsfndLNeCb1dAEH5H-wj_6spAm+d_AtBQDZfhQ@mail.gmail.com","threadId":"42490","inReplyTo":"20160530182148.18801-1-pranit.bauva@gmail.com","subject":"Re: [RFC/PATCH] bisect--helper: `bisect_clean_state` shell function in C","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-05-30T21:48:17Z","receivedAt":"2016-05-30T21:48:17Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, May 30, 2016 at 8:21 PM, Pranit Bauva wrote:\n> ---\n> This patch contains a bug. I have tried to identify the bug and I suppose it\n> exists in do_for_each_entry_in_dir(). I have reproduced the debugging session\n> at this link[1]. I have seen that some patches in mailing list regarding\n> iterating over refs. Will those affect this? Or is this bug fixed in those\n> patches?\n>\n> [1]: http://paste.ubuntu.com/16830752/\n\nThe debug session seems to use code source from a previous version of\nthis patch.\nAlso it is not cear in which context you run git under gdb. What have\nyou done before?\nAnd we don't see a crash. Could you show the crash and run the \"bt\"\ncommand in gdb to get a backtrace?\n\n> @@ -79,11 +90,42 @@ int write_terms(const char *bad, const char *good)\n> strbuf_release(&content);\n> return (res < 0) ? -1 : 0;\n> }\n> +\n> +int remove_bisect_ref(const char *refname, const struct object_id *oid,\n> + int flag, void *cb_data)\n> +{\n> + char *ref;\n> + ref = xstrfmt(\"refs/bisect/%s\", refname);\n\nYou could save one line by concatenating the 2 above lines.\n\n> + if (delete_ref(ref, oid->hash, flag))\n> + return error(_(\"couldn't delete the ref %s\\n\"), ref);\n> + return 0;\n\nYou need to free \"ref\".\n\n> +}\n\nThanks,\nChristian.\n"},{"id":"287870","messageId":"574D122F.7080608@alum.mit.edu","threadId":"42490","inReplyTo":"20160530182148.18801-1-pranit.bauva@gmail.com","subject":"Re: [RFC/PATCH] bisect--helper: `bisect_clean_state` shell function in C","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-31T04:25:19Z","receivedAt":"2016-05-31T04:25:19Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/30/2016 08:21 PM, Pranit Bauva wrote:\n> Reimplement `bisect_clean_state` shell function in C and add a\n> `bisect-clean-state` subcommand to `git bisect--helper` to call it from\n> git-bisect.sh .\n> \n> Using `bisect_clean_state` subcommand is a measure to port shell\n> function to C so as to use the existing test suite. As more functions\n> are ported, this subcommand will be retired and will be called by\n> bisect_reset() and bisect_start().\n> \n> Mentored-by: Lars Schneider \n> Mentored-by: Christian Couder \n> Signed-off-by: Pranit Bauva \n> ---\n> This patch contains a bug. I have tried to identify the bug and I suppose it\n> exists in do_for_each_entry_in_dir(). I have reproduced the debugging session\n> at this link[1]. I have seen that some patches in mailing list regarding\n> iterating over refs. Will those affect this? Or is this bug fixed in those\n> patches?\n\nThe problem is that it is not legal to modify references while iterating\nover them. See [1]. Your remove_bisect_ref() callback function deletes\nreferences, which modifies the reference cache that is being iterated over.\n\nInstead I suggest that your remove_bisect_ref() add the references to a\nstring_list, then call delete_refs() *after* the iteration is over.\nAlternatively, you can change remove_bisect_ref() to call\nref_transaction_delete() to add reference deletions to a\nref_transaction, then call ref_transaction_commit() after the iteration\nis over. See the rm() function in builtin/remote.c [2] for an example.\n\n[1]\nhttps://github.com/git/git/blob/f3913c2d03abc660140678a9e14dac399f847647/refs.h#L176-L184\n[2]\nhttps://github.com/git/git/blob/f3913c2d03abc660140678a9e14dac399f847647/builtin/remote.c#L738\n\n> [...]\n\nMichael\n"},{"id":"287877","messageId":"CAFZEwPOhGuTkZcP+hmnCrNjvHGM_FKUOsQaGwWKoUzokCN5OFg@mail.gmail.com","threadId":"42490","inReplyTo":"CAP8UFD2wAKN2CsfndLNeCb1dAEH5H-wj_6spAm+d_AtBQDZfhQ@mail.gmail.com","subject":"Re: [RFC/PATCH] bisect--helper: `bisect_clean_state` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-05-31T06:33:09Z","receivedAt":"2016-05-31T06:33:09Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Christian,\n\nOn Tue, May 31, 2016 at 3:18 AM, Christian Couder\n wrote:\n> On Mon, May 30, 2016 at 8:21 PM, Pranit Bauva wrote:\n>> ---\n>> This patch contains a bug. I have tried to identify the bug and I suppose it\n>> exists in do_for_each_entry_in_dir(). I have reproduced the debugging session\n>> at this link[1]. I have seen that some patches in mailing list regarding\n>> iterating over refs. Will those affect this? Or is this bug fixed in those\n>> patches?\n>>\n>> [1]: http://paste.ubuntu.com/16830752/\n>\n> The debug session seems to use code source from a previous version of\n> this patch.\n> Also it is not cear in which context you run git under gdb. What have\n> you done before?\n> And we don't see a crash. Could you show the crash and run the \"bt\"\n> command in gdb to get a backtrace?\n\nThe segmentation fault will occur if I move to the next step ie.\nevaluate the value entry->flag.\n\n>> @@ -79,11 +90,42 @@ int write_terms(const char *bad, const char *good)\n>> strbuf_release(&content);\n>> return (res < 0) ? -1 : 0;\n>> }\n>> +\n>> +int remove_bisect_ref(const char *refname, const struct object_id *oid,\n>> + int flag, void *cb_data)\n>> +{\n>> + char *ref;\n>> + ref = xstrfmt(\"refs/bisect/%s\", refname);\n>\n> You could save one line by concatenating the 2 above lines.\n\nSure!\n\n>> + if (delete_ref(ref, oid->hash, flag))\n>> + return error(_(\"couldn't delete the ref %s\\n\"), ref);\n>> + return 0;\n>\n> You need to free \"ref\".\n\nSure!\n\nRegards,\nPranit Bauva\n"},{"id":"287879","messageId":"CAFZEwPMKQ6JtaNvoexzGqWRphqEgXBXn-PSrkUaPLzs6phM4Ww@mail.gmail.com","threadId":"42490","inReplyTo":"574D122F.7080608@alum.mit.edu","subject":"Re: [RFC/PATCH] bisect--helper: `bisect_clean_state` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-05-31T06:40:58Z","receivedAt":"2016-05-31T06:40:58Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Michael,\n\nOn Tue, May 31, 2016 at 9:55 AM, Michael Haggerty wrote:\n> On 05/30/2016 08:21 PM, Pranit Bauva wrote:\n>> Reimplement `bisect_clean_state` shell function in C and add a\n>> `bisect-clean-state` subcommand to `git bisect--helper` to call it from\n>> git-bisect.sh .\n>>\n>> Using `bisect_clean_state` subcommand is a measure to port shell\n>> function to C so as to use the existing test suite. As more functions\n>> are ported, this subcommand will be retired and will be called by\n>> bisect_reset() and bisect_start().\n>>\n>> Mentored-by: Lars Schneider \n>> Mentored-by: Christian Couder \n>> Signed-off-by: Pranit Bauva \n>> ---\n>> This patch contains a bug. I have tried to identify the bug and I suppose it\n>> exists in do_for_each_entry_in_dir(). I have reproduced the debugging session\n>> at this link[1]. I have seen that some patches in mailing list regarding\n>> iterating over refs. Will those affect this? Or is this bug fixed in those\n>> patches?\n>\n> The problem is that it is not legal to modify references while iterating\n> over them. See [1]. Your remove_bisect_ref() callback function deletes\n> references, which modifies the reference cache that is being iterated over.\n\nThanks for explaining this. I wasn't aware about this.\n\n> Instead I suggest that your remove_bisect_ref() add the references to a\n> string_list, then call delete_refs() *after* the iteration is over.\n> Alternatively, you can change remove_bisect_ref() to call\n> ref_transaction_delete() to add reference deletions to a\n> ref_transaction, then call ref_transaction_commit() after the iteration\n> is over. See the rm() function in builtin/remote.c [2] for an example.\n\nI have gone through the links and I think I will stick with the first\none because I don't see how the latter one would be better.\n\n> [1]\n> https://github.com/git/git/blob/f3913c2d03abc660140678a9e14dac399f847647/refs.h#L176-L184\n> [2]\n> https://github.com/git/git/blob/f3913c2d03abc660140678a9e14dac399f847647/builtin/remote.c#L738\n>\n>> [...]\n>\n\nRegards,\nPranit Bauva\n"}]}