Re: [RFC/PATCH v11 11/13] bisect--helper: `bisect_next_check` shell function in C

[Pranit Bauva]

Hey Junio,

On Wed, Aug 3, 2016 at 12:47 AM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> +static int mark_good(const char *refname, const struct object_id *oid,
>> +  int flag, void *cb_data)
>> +{
>> + int *m_good = (int *)cb_data;
>> + *m_good = 0;
>> + return 0;
>> +}
>
> See below.
>
>> +static int bisect_next_check(const struct bisect_terms *terms,
>> +  const char *current_term)
>> +{
>> + int missing_good = 1, missing_bad = 1;
>
> It is somewhat unusual to start with "assume we are OK" and then
> "it turns out that we are not".
>
>> + char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad.buf);
>> + char *good_glob = xstrfmt("%s*", terms->term_good.buf);
>
> The original runs
>
> git for-each-ref "refs/bisect/$TERM_GOOD-*
>
> but this one lacks the final dash.

My bad. Will include it.

>> + if (ref_exists(bad_ref))
>> + missing_bad = 0;
>> + free(bad_ref);
>> +
>> + for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
>> +  (void *) _good);
>> + free(good_glob);
>
> The for-each helper does not return until it iterates over all the
> matching refs, but you are only interested in seeing if at least one
> exists.  It may make sense to return 1 from mark_good() to terminate
> the traversal early.

Seems a better way. Thanks!

>> + if (!missing_good && !missing_bad)
>> + return 0;
>> +
>> + if (!current_term)
>> + return -1;
>> +
>> + if (missing_good && !missing_bad && current_term &&
>> + !strcmp(current_term, terms->term_good.buf)) {
>> + char *yesno;
>> + /*
>> +  * have bad (or new) but not good (or old). We could bisect
>> +  * although this is less optimum.
>> +  */
>> + fprintf(stderr, "Warning: bisecting only with a %s commit\n",
>> + terms->term_bad.buf);
>
> In the original, this message goes through gettext.

Will do.

>> + if (!isatty(0))
>> + 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(_("Are you sure [Y/n]? "), PROMPT_ECHO);
>> + if (starts_with(yesno, "N") || starts_with(yesno, "n"))
>> + return -1;
>> + return 0;
>> + }
>
> When the control falls into the above if(){} block, the function
> will always return.  It will clarify that this is the end of such a
> logical block to have a blank line here.

Will do.

>> + if (!is_empty_or_missing_file(git_path_bisect_start()))
>> + return error(_("You need to give me at least one good|old and "
>> + "bad|new revision. You can use \"git bisect "
>> + "bad|new\" and \"git bisect good|old\" for "
>> + "that. \n"));
>> + else
>> + return error(_("You need to start by \"git bisect start\". "
>> + "You then need to give me at least one good|"
>> + "old and bad|new revision. You can use \"git "
>> + "bisect bad|new\" and \"git bisect good|old\" "
>> + " for that.\n"));
>
> The i18n on these two messages seem to be different from the
> original, which asks bisect_voc to learn what 'bad' and 'good' are
> called and attempts to use these words from the vocabulary.

I have little idea about i18n. Will look more into it.

Regards,
Pranit Bauva
--
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: [RFC/PATCH v11 11/13] bisect--helper: `bisect_next_check` shell function in C

[Junio C Hamano]

Pranit Bauva  writes:

> +static int mark_good(const char *refname, const struct object_id *oid,
> +  int flag, void *cb_data)
> +{
> + int *m_good = (int *)cb_data;
> + *m_good = 0;
> + return 0;
> +}

See below.

> +static int bisect_next_check(const struct bisect_terms *terms,
> +  const char *current_term)
> +{
> + int missing_good = 1, missing_bad = 1;

It is somewhat unusual to start with "assume we are OK" and then
"it turns out that we are not".

> + char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad.buf);
> + char *good_glob = xstrfmt("%s*", terms->term_good.buf);

The original runs

git for-each-ref "refs/bisect/$TERM_GOOD-*

but this one lacks the final dash.

> + if (ref_exists(bad_ref))
> + missing_bad = 0;
> + free(bad_ref);
> +
> + for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
> +  (void *) _good);
> + free(good_glob);

The for-each helper does not return until it iterates over all the
matching refs, but you are only interested in seeing if at least one
exists.  It may make sense to return 1 from mark_good() to terminate
the traversal early.

> + if (!missing_good && !missing_bad)
> + return 0;
> +
> + if (!current_term)
> + return -1;
> +
> + if (missing_good && !missing_bad && current_term &&
> + !strcmp(current_term, terms->term_good.buf)) {
> + char *yesno;
> + /*
> +  * have bad (or new) but not good (or old). We could bisect
> +  * although this is less optimum.
> +  */
> + fprintf(stderr, "Warning: bisecting only with a %s commit\n",
> + terms->term_bad.buf);

In the original, this message goes through gettext.

> + if (!isatty(0))
> + 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(_("Are you sure [Y/n]? "), PROMPT_ECHO);
> + if (starts_with(yesno, "N") || starts_with(yesno, "n"))
> + return -1;
> + return 0;
> + }

When the control falls into the above if(){} block, the function
will always return.  It will clarify that this is the end of such a
logical block to have a blank line here.

> + if (!is_empty_or_missing_file(git_path_bisect_start()))
> + return error(_("You need to give me at least one good|old and "
> + "bad|new revision. You can use \"git bisect "
> + "bad|new\" and \"git bisect good|old\" for "
> + "that. \n"));
> + else
> + return error(_("You need to start by \"git bisect start\". "
> + "You then need to give me at least one good|"
> + "old and bad|new revision. You can use \"git "
> + "bisect bad|new\" and \"git bisect good|old\" "
> + " for that.\n"));

The i18n on these two messages seem to be different from the
original, which asks bisect_voc to learn what 'bad' and 'good' are
called and attempts to use these words from the vocabulary.
--
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

[RFC/PATCH v11 11/13] bisect--helper: `bisect_next_check` shell function in C

[Pranit Bauva]

Reimplement `bisect_next_check` shell function in C and add
`bisect-next-check` subcommand to `git bisect--helper` to call it from
git-bisect.sh .

Using `--bisect-next-check` 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_voc() is removed as it is redundant and does not serve any useful
purpose. We are better off specifying "bad|new" "good|old" as and when
we require in bisect_next_check().

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 79 +++-
 git-bisect.sh| 60 +++-
 2 files changed, 82 insertions(+), 57 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 99c9f90..71f4cf0 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -6,6 +6,7 @@
 #include "dir.h"
 #include "argv-array.h"
 #include "run-command.h"
+#include "prompt.h"
 
 static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
 static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
@@ -24,6 +25,7 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-reset []"),
N_("git bisect--helper --bisect-write
 []"),
N_("git bisect--helper --bisect-check-and-set-terms  
 "),
+   N_("git bisect--helper --bisect-next-check []  
term_bad.buf);
+   char *good_glob = xstrfmt("%s*", terms->term_good.buf);
+
+   if (ref_exists(bad_ref))
+   missing_bad = 0;
+   free(bad_ref);
+
+   for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
+(void *) _good);
+   free(good_glob);
+
+   if (!missing_good && !missing_bad)
+   return 0;
+
+   if (!current_term)
+   return -1;
+
+   if (missing_good && !missing_bad && current_term &&
+   !strcmp(current_term, terms->term_good.buf)) {
+   char *yesno;
+   /*
+* have bad (or new) but not good (or old). We could bisect
+* although this is less optimum.
+*/
+   fprintf(stderr, "Warning: bisecting only with a %s commit\n",
+   terms->term_bad.buf);
+   if (!isatty(0))
+   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(_("Are you sure [Y/n]? "), PROMPT_ECHO);
+   if (starts_with(yesno, "N") || starts_with(yesno, "n"))
+   return -1;
+   return 0;
+   }
+   if (!is_empty_or_missing_file(git_path_bisect_start()))
+   return error(_("You need to give me at least one good|old and "
+   "bad|new revision. You can use \"git bisect "
+   "bad|new\" and \"git bisect good|old\" for "
+   "that. \n"));
+   else
+   return error(_("You need to start by \"git bisect start\". "
+   "You then need to give me at least one good|"
+   "old and bad|new revision. You can use \"git "
+   "bisect bad|new\" and \"git bisect good|old\" "
+   " for that.\n"));
+
+   return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
@@ -301,7 +368,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
BISECT_RESET,
CHECK_EXPECTED_REVS,
BISECT_WRITE,
-   CHECK_AND_SET_TERMS
+   CHECK_AND_SET_TERMS,
+   BISECT_NEXT_CHECK
} cmdmode = 0;
int no_checkout = 0, res = 0;
struct option options[] = {
@@ -319,6 +387,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("write out the bisection state in BISECT_LOG"),