Hey Stephan,
On Tue, Nov 22, 2016 at 5:42 AM, Stephan Beyer <s-beyer@xxxxxxx> wrote:
> Hi,
>
> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
>> Reimplement the `bisect_state` shell function in C and also add a
>> subcommand `--bisect-state` to `git-bisect--helper` to call it from
>> git-bisect.sh .
>>
>> Using `--bisect-state` subcommand is a temporary 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 some
>> other methods.
>>
>> `bisect_head` is called from `bisect_state` thus its not required to
>> introduce another subcommand.
>
> Missing comma before "thus", and "it is" (or "it's") instead of "its" :)
Sure, will fix.
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index 1767916..1481c6d 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
>> @@ -784,6 +786,79 @@ static int bisect_autostart(struct bisect_terms *terms)
>> return 0;
>> }
>>
>> +static char *bisect_head(void)
>> +{
>> + if (is_empty_or_missing_file(git_path_bisect_head()))
>> + return "HEAD";
>> + else
>> + return "BISECT_HEAD";
>> +}
>
> This is very shellish.
> In C I'd expect something like
>
> static int bisect_head_sha1(unsigned char *sha)
> {
> int res;
> if (is_empty_or_missing_file(git_path_bisect_head()))
> res = get_sha1("HEAD", sha);
> else
> res = get_sha1("BISECT_HEAD", sha);
>
> if (res)
> return error(_("Could not find BISECT_HEAD or HEAD."));
>
> return 0;
> }
>
>> +
>> +static int bisect_state(struct bisect_terms *terms, const char **argv,
>> + int argc)
>> +{
>> + const char *state = argv[0];
>> +
>> + get_terms(terms);
>> + if (check_and_set_terms(terms, state))
>> + return -1;
>> +
>> + if (!argc)
>> + die(_("Please call `--bisect-state` with at least one argument"));
>
> I think this check should move to cmd_bisect__helper. There are also the
> other argument number checks.
Not really. After the whole conversion, the cmdmode will cease to
exists while bisect_state will be called directly, thus it is
important to check it here.
>> +
>> + if (argc == 1 && one_of(state, terms->term_good,
>> + terms->term_bad, "skip", NULL)) {
>> + const char *bisected_head = xstrdup(bisect_head());
>> + const char *hex[1];
>
> Maybe:
> const char *hex;
>
>> + unsigned char sha1[20];
>> +
>> + if (get_sha1(bisected_head, sha1))
>> + die(_("Bad rev input: %s"), bisected_head);
>
> And instead of...
>
>> + if (bisect_write(state, sha1_to_hex(sha1), terms, 0))
>> + return -1;
>> +
>> + *hex = xstrdup(sha1_to_hex(sha1));
>> + if (check_expected_revs(hex, 1))
>> + return -1;
>
> ... simply:
>
> hex = xstrdup(sha1_to_hex(sha1));
> if (set_state(terms, state, hex)) {
> free(hex);
> return -1;
> }
> free(hex);
>
> where:
Yes I am planning to convert all places with hex rather than the sha1
but not yet, maybe in an another patch series because currently a lot
of things revolve around sha1 and changing its behaviour wouldn't
really be a part of a porting patch series.
> static int set_state(struct bisect_terms *terms, const char *state,
> const char *hex)
> {
> if (bisect_write(state, hex, terms, 0))
> return -1;
> if (check_expected_revs(&hex, 1))
> return -1;
> return 0;
> }
>
>> + return bisect_auto_next(terms, NULL);
>> + }
>> +
>> + if ((argc == 2 && !strcmp(state, terms->term_bad)) ||
>> + one_of(state, terms->term_good, "skip", NULL)) {
>> + int i;
>> + struct string_list hex = STRING_LIST_INIT_DUP;
>> +
>> + for (i = 1; i < argc; i++) {
>> + unsigned char sha1[20];
>> +
>> + if (get_sha1(argv[i], sha1)) {
>> + string_list_clear(&hex, 0);
>> + die(_("Bad rev input: %s"), argv[i]);
>> + }
>> + string_list_append(&hex, sha1_to_hex(sha1));
>> + }
>> + for (i = 0; i < hex.nr; i++) {
>
> ... And replace this:
>
>> + const char **hex_string = (const char **) &hex.items[i].string;
>> + if(bisect_write(state, *hex_string, terms, 0)) {
>> + string_list_clear(&hex, 0);
>> + return -1;
>> + }
>> + if (check_expected_revs(hex_string, 1)) {
>> + string_list_clear(&hex, 0);
>> + return -1;
>> + }
>
> by:
>
> const char *hex_str = hex.items[i].string;
> if (set_state(terms, state, hex_string)) {
> string_list_clear(&hex, 0);
> return -1;
> }
>
>> + }
>> + string_list_clear(&hex, 0);
>> + return bisect_auto_next(terms, NULL);
>> + }
>> +
>> + if (!strcmp(state, terms->term_bad))
>> + die(_("'git bisect %s' can take only one argument."),
>> + terms->term_bad);
>> +
>> + return -1;
>> +}
>> +
>> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>> {
>> enum {
>> @@ -823,6 +899,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>> N_("verify the next bisection state then find the next bisection state"), BISECT_AUTO_NEXT),
>> OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
>> N_("start the bisection if BISECT_START empty or missing"), BISECT_AUTOSTART),
>> + OPT_CMDMODE(0, "bisect-state", &cmdmode,
>> + N_("mark the state of ref (or refs)"), BISECT_STATE),
>
> "mark the state of the given revs"
>
> Note that rev != ref
Might have been a typo. Will fix.
>> @@ -902,6 +980,14 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>> terms.term_bad = "bad";
>> res = bisect_autostart(&terms);
>> break;
>> + case BISECT_STATE:
>> + if (argc == 0)
>> + die(_("--bisect-state requires at least 1 argument"));
>
> "at least one revision"
Okay, that would make it more specific.
>> diff --git a/git-bisect.sh b/git-bisect.sh
>> index cd56551..a9eebbb 100755
>> --- a/git-bisect.sh
>> +++ b/git-bisect.sh
>> @@ -61,44 +51,7 @@ bisect_skip() {
>> esac
>> all="$all $revs"
>> done
>> - eval bisect_state 'skip' $all
> [...deleted lines...]
>> + eval git bisect--helper --bisect-state 'skip' $all
>
> I think you don't need "eval" here any longer.
Yes, I wouldn't
>> @@ -184,8 +137,8 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
>> state="$TERM_GOOD"
>> fi
>>
>> - # We have to use a subshell because "bisect_state" can exit.
>> - ( bisect_state $state >"$GIT_DIR/BISECT_RUN" )
>> + # We have to use a subshell because "--bisect-state" can exit.
>> + ( git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN" )
True, but right now I didn't want to modify that part of the source
code to remove the comment. I will remove the comment all together
when I port bisect_run()
Regards,
Pranit Bauva