Re: [PATCH v14 13/27] bisect--helper: `bisect_start` shell function partially in C
Pranit Bauva writes:
>> I wonder the whole thing above is better restructured to avoid
>> repeated checks of the same thing.
>>
>> if (is it 40-hex, i.e. detached?) {
>> stuff it to start_head;
>> } else if (skip_prefix(head, "refs/heads/", &branchname)) {
>> do the "cogito" check;
>> stuff it to start_head;
>> } else {
>> that's a strange symbolic ref HEAD you have there;
>> }
>
> I guess it changes the behaviour. Its a strange symbolic ref if it
> does not start with "refs/heads".
I did not think my suggestion would change the behaviour at all. It
would change the code structure a bit to make it more readable, though.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v14 13/27] bisect--helper: `bisect_start` shell function partially in C
Hey Junio,
On Fri, Aug 26, 2016 at 12:32 AM, Junio C Hamano wrote:
> Pranit Bauva writes:
>
>> +static int bisect_start(struct bisect_terms *terms, int no_checkout,
>> + const char **argv, int argc)
>> +{
>> + int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
>> + int flags, pathspec_pos;
>> + struct string_list revs = STRING_LIST_INIT_DUP;
>> + struct string_list states = STRING_LIST_INIT_DUP;
>
> The original has a single state, not states. Let's see if there is
> a reason behind the name change
>
>> + unsigned char sha1[20];
>> + struct object_id oid;
>
> More on these below...
>
>> + ...
>> + for (i = 0; i < argc; i++) {
>> + const char *commit_id = xstrfmt("%s^{commit}", argv[i]);
>> + if (!strcmp(argv[i], "--")) {
>> + has_double_dash = 1;
>> + break;
>> + } else if (!strcmp(argv[i], "--no-checkout"))
>> + no_checkout = 1;
>> + else if (!strcmp(argv[i], "--term-good") ||
>> + ...
>> + } else if (starts_with(argv[i], "--") &&
>> + !one_of(argv[i], "--term-good", "--term-bad", NULL)) {
>> + string_list_clear(&revs, 0);
>> + string_list_clear(&states, 0);
>> + die(_("unrecognised option: '%s'"), argv[i]);
>> + } else if (get_oid(commit_id, &oid) && has_double_dash) {
>> + string_list_clear(&revs, 0);
>> + string_list_clear(&states, 0);
>> + die(_("'%s' does not appear to be a valid revision"),
>> argv[i]);
>> + } else {
>> + string_list_append(&revs, oid_to_hex(&oid));
>> + }
>> + }
>
> What I do not understand is clearing the string list "states" inside
> this loop. It may have been INIT_DUPed, but nothing in this loop
> adds anything to it. Because "revs" does get extended in the loop,
> it is understandable if you wanted to clear it before dying, but "if
> you are dying anyway why bother clearing?" is also a valid stance to
> take.
I think I should probably use return here instead of die().
> The same "perhaps want to use the 'retval' with goto 'finish:' pattern?"
> comment applies here, too.
Okay sure could do that.
>> + pathspec_pos = i;
>> +
>> + /*
>> + * The user ran "git bisect start ", hence did not
>> + * explicitly specify the terms, but we are already starting to
>> + * set references named with the default terms, and won't be able
>> + * to change afterwards.
>> + */
>> + must_write_terms |= !!revs.nr;
>> + for (i = 0; i < revs.nr; i++) {
>> + if (bad_seen)
>> + string_list_append(&states, terms->term_good.buf);
>> + else {
>> + bad_seen = 1;
>> + string_list_append(&states, terms->term_bad.buf);
>> + }
>> + }
>
> This is certainly different from the original. We used to do one
> "bisect_write" per element in revs in the loop; we no longer do that
> and instead collect them in states list. Each element in these two
> lists, i.e. revs.item[i] and states.item[i], corresponds to each
> other.
>
> That explains why the variable is renamed to states. We haven't
> seen enough to say if this behaviour change is a good idea or not.
>
>> + /*
>> + * Verify HEAD
>> + */
>> + head = resolve_ref_unsafe("HEAD", 0, sha1, &flags);
>> + if (!head) {
>> + if (get_sha1("HEAD", sha1)) {
>> + string_list_clear(&revs, 0);
>> + string_list_clear(&states, 0);
>> + die(_("Bad HEAD - I need a HEAD"));
>> + }
>> + }
>> + if (!is_empty_or_missing_file(git_path_bisect_start())) {
>> + /* Reset to the rev from where we started */
>> + strbuf_read_file(&start_head, git_path_bisect_start(), 0);
>> + strbuf_trim(&start_head);
>> + if (!no_checkout) {
>> + struct argv_array argv = ARGV_ARRAY_INIT;
>> + argv_array_pushl(&argv, "checkout", start_head.buf,
>> + "--", NULL);
>> + if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
>> + error(_("checking out '%s' failed. Try 'git "
>> + "bisect start '."),
>> + start_head.buf);
>> + strbuf_release(&start_head);
>> + string_list_clear(&revs, 0);
>> + string_list_clear(&states, 0);
>> + return -1;
>
> The original died here, but you expect the caller to respond to a
> negative return. I haven't read enough to judge if that is a good
> idea, but doesn't it make sense to do the same through
Re: [PATCH v14 13/27] bisect--helper: `bisect_start` shell function partially in C
Junio C Hamano writes:
>> +for (i = 0; i < revs.nr; i++) {
>> +if (bad_seen)
>> +string_list_append(&states, terms->term_good.buf);
>> +else {
>> +bad_seen = 1;
>> +string_list_append(&states, terms->term_bad.buf);
>> +}
>> +}
>
> This is certainly different from the original. We used to do one
> "bisect_write" per element in revs in the loop; we no longer do that
> and instead collect them in states list. Each element in these two
> lists, i.e. revs.item[i] and states.item[i], corresponds to each
> other.
>
> That explains why the variable is renamed to states. We haven't
> seen enough to say if this behaviour change is a good idea or not.
Ahh, I misread the original. It accumulates the writes to be
executed in $eval and does so at the end. So there is no change in
behaviour.
So please ignore that point in the previous message. That leaves
only the following points:
* Perhaps 'retval' with 'goto finish' pattern?
* ref_exists()? Perhaps use skip_prefix(head, "refs/heads/, &branch)?
* if (clean-state) { return -1 }?
* Is comment on trap relevant here?
Sorry, and thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v14 13/27] bisect--helper: `bisect_start` shell function partially in C
Pranit Bauva writes:
> +static int bisect_start(struct bisect_terms *terms, int no_checkout,
> + const char **argv, int argc)
> +{
> + int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
> + int flags, pathspec_pos;
> + struct string_list revs = STRING_LIST_INIT_DUP;
> + struct string_list states = STRING_LIST_INIT_DUP;
The original has a single state, not states. Let's see if there is
a reason behind the name change
> + unsigned char sha1[20];
> + struct object_id oid;
More on these below...
> + ...
> + for (i = 0; i < argc; i++) {
> + const char *commit_id = xstrfmt("%s^{commit}", argv[i]);
> + if (!strcmp(argv[i], "--")) {
> + has_double_dash = 1;
> + break;
> + } else if (!strcmp(argv[i], "--no-checkout"))
> + no_checkout = 1;
> + else if (!strcmp(argv[i], "--term-good") ||
> + ...
> + } else if (starts_with(argv[i], "--") &&
> + !one_of(argv[i], "--term-good", "--term-bad", NULL)) {
> + string_list_clear(&revs, 0);
> + string_list_clear(&states, 0);
> + die(_("unrecognised option: '%s'"), argv[i]);
> + } else if (get_oid(commit_id, &oid) && has_double_dash) {
> + string_list_clear(&revs, 0);
> + string_list_clear(&states, 0);
> + die(_("'%s' does not appear to be a valid revision"),
> argv[i]);
> + } else {
> + string_list_append(&revs, oid_to_hex(&oid));
> + }
> + }
What I do not understand is clearing the string list "states" inside
this loop. It may have been INIT_DUPed, but nothing in this loop
adds anything to it. Because "revs" does get extended in the loop,
it is understandable if you wanted to clear it before dying, but "if
you are dying anyway why bother clearing?" is also a valid stance to
take.
The same "perhaps want to use the 'retval' with goto 'finish:' pattern?"
comment applies here, too.
> + pathspec_pos = i;
> +
> + /*
> + * The user ran "git bisect start ", hence did not
> + * explicitly specify the terms, but we are already starting to
> + * set references named with the default terms, and won't be able
> + * to change afterwards.
> + */
> + must_write_terms |= !!revs.nr;
> + for (i = 0; i < revs.nr; i++) {
> + if (bad_seen)
> + string_list_append(&states, terms->term_good.buf);
> + else {
> + bad_seen = 1;
> + string_list_append(&states, terms->term_bad.buf);
> + }
> + }
This is certainly different from the original. We used to do one
"bisect_write" per element in revs in the loop; we no longer do that
and instead collect them in states list. Each element in these two
lists, i.e. revs.item[i] and states.item[i], corresponds to each
other.
That explains why the variable is renamed to states. We haven't
seen enough to say if this behaviour change is a good idea or not.
> + /*
> + * Verify HEAD
> + */
> + head = resolve_ref_unsafe("HEAD", 0, sha1, &flags);
> + if (!head) {
> + if (get_sha1("HEAD", sha1)) {
> + string_list_clear(&revs, 0);
> + string_list_clear(&states, 0);
> + die(_("Bad HEAD - I need a HEAD"));
> + }
> + }
> + if (!is_empty_or_missing_file(git_path_bisect_start())) {
> + /* Reset to the rev from where we started */
> + strbuf_read_file(&start_head, git_path_bisect_start(), 0);
> + strbuf_trim(&start_head);
> + if (!no_checkout) {
> + struct argv_array argv = ARGV_ARRAY_INIT;
> + argv_array_pushl(&argv, "checkout", start_head.buf,
> + "--", NULL);
> + if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
> + error(_("checking out '%s' failed. Try 'git "
> + "bisect start '."),
> + start_head.buf);
> + strbuf_release(&start_head);
> + string_list_clear(&revs, 0);
> + string_list_clear(&states, 0);
> + return -1;
The original died here, but you expect the caller to respond to a
negative return. I haven't read enough to judge if that is a good
idea, but doesn't it make sense to do the same throughout the
function consistently? I saw a few die()'s already in the command
line parsing loop--shouldn't they also return -1?
The original has called bisect_write already when we attempt this
checkout and the user will see the states in the filesystem. This
rewrite does not. What effe
[PATCH v14 13/27] bisect--helper: `bisect_start` shell function partially in C
Reimplement the `bisect_start` shell function partially in C and add
`bisect-start` subcommand to `git bisect--helper` to call it from
git-bisect.sh .
The last part is not converted because it calls another shell function
`bisect_start` shell function will be completed after the `bisect_next`
shell function is ported in C.
Using `--bisect-start` subcommand is a temporary measure to port shell
function in 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.
Mentored-by: Lars Schneider
Mentored-by: Christian Couder
Signed-off-by: Pranit Bauva
---
builtin/bisect--helper.c | 238 ++-
git-bisect.sh| 133 +-
2 files changed, 238 insertions(+), 133 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 44adf6b..c64996a 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -7,6 +7,7 @@
#include "argv-array.h"
#include "run-command.h"
#include "prompt.h"
+#include "quote.h"
static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
@@ -14,6 +15,8 @@ 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_start, "BISECT_START")
static GIT_PATH_FUNC(git_path_bisect_head, "BISECT_HEAD")
+static GIT_PATH_FUNC(git_path_head_name, "head-name")
+static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --next-all [--no-checkout]"),
@@ -24,6 +27,8 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-check-and-set-terms
"),
N_("git bisect--helper --bisect-next-check []
--term-{new,bad}=]"
+ "[--no-checkout] [
[...]] [--] [...]"),
NULL
};
@@ -391,6 +396,226 @@ static int bisect_terms(struct bisect_terms *terms, const
char **argv, int argc)
return 0;
}
+static int bisect_start(struct bisect_terms *terms, int no_checkout,
+ const char **argv, int argc)
+{
+ int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
+ int flags, pathspec_pos;
+ struct string_list revs = STRING_LIST_INIT_DUP;
+ struct string_list states = STRING_LIST_INIT_DUP;
+ struct strbuf start_head = STRBUF_INIT;
+ struct strbuf bisect_names = STRBUF_INIT;
+ struct strbuf orig_args = STRBUF_INIT;
+ const char *head;
+ unsigned char sha1[20];
+ FILE *fp;
+ struct object_id oid;
+
+ if (is_bare_repository())
+ no_checkout = 1;
+
+ for (i = 0; i < argc; i++) {
+ if (!strcmp(argv[i], "--")) {
+ has_double_dash = 1;
+ break;
+ }
+ }
+
+ for (i = 0; i < argc; i++) {
+ const char *commit_id = xstrfmt("%s^{commit}", argv[i]);
+ if (!strcmp(argv[i], "--")) {
+ has_double_dash = 1;
+ break;
+ } else if (!strcmp(argv[i], "--no-checkout"))
+ no_checkout = 1;
+ else if (!strcmp(argv[i], "--term-good") ||
+!strcmp(argv[i], "--term-old")) {
+ must_write_terms = 1;
+ strbuf_reset(&terms->term_good);
+ strbuf_addstr(&terms->term_good, argv[++i]);
+ } else if (skip_prefix(argv[i], "--term-good=", &argv[i])) {
+ must_write_terms = 1;
+ strbuf_reset(&terms->term_good);
+ strbuf_addstr(&terms->term_good, argv[i]);
+ } else if (skip_prefix(argv[i], "--term-old=", &argv[i])) {
+ must_write_terms = 1;
+ strbuf_reset(&terms->term_good);
+ strbuf_addstr(&terms->term_good, argv[i]);
+ } else if (!strcmp(argv[i], "--term-bad") ||
+!strcmp(argv[i], "--term-new")) {
+ must_write_terms = 1;
+ strbuf_reset(&terms->term_bad);
+ strbuf_addstr(&terms->term_bad, argv[++i]);
+ } else if (skip_prefix(argv[i], "--term-bad=", &argv[i])) {
+ must_write_terms = 1;
+ strbuf_reset(&terms->term_bad);
+ strbuf_addstr(&terms->term_bad, argv[i]);
+ } else if (skip_prefix(argv[i], "--term-new=", &argv[i])) {
+ must_write_terms = 1;
+ strbuf_reset(&terms->term_good);
+ strbuf_addstr(&terms->term_good, argv[i]);
+ } else if (starts_with(ar
