diff mbox series

[v3,2/7] bisect--helper: reimplement `bisect_replay` shell function in C

Message ID 20210123154056.48234-3-mirucam@gmail.com (mailing list archive)
State Superseded
Headers show
Series Finish converting git bisect to C part 3 | expand

Commit Message

Miriam Rubio Jan. 23, 2021, 3:40 p.m. UTC
From: Pranit Bauva <pranit.bauva@gmail.com>

Reimplement the `bisect_replay` shell function in C and also add
`--bisect-replay` subcommand to `git bisect--helper` to call it from
git-bisect.sh

Using `--bisect-replay` subcommand is a temporary measure to port shell
function to C so as to use the existing test suite.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
 builtin/bisect--helper.c | 84 +++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 34 +---------------
 2 files changed, 84 insertions(+), 34 deletions(-)

Comments

Junio C Hamano Jan. 24, 2021, 12:22 a.m. UTC | #1
Miriam Rubio <mirucam@gmail.com> writes:

> From: Pranit Bauva <pranit.bauva@gmail.com>
>
> Reimplement the `bisect_replay` shell function in C and also add
> `--bisect-replay` subcommand to `git bisect--helper` to call it from
> git-bisect.sh
> ...
> +static int process_replay_line(struct bisect_terms *terms, struct strbuf *line)
> +{
> +	const char *p = line->buf + strspn(line->buf, " \t");
> +
> +	if ((!skip_prefix(p, "git bisect", &p) &&
> +	!skip_prefix(p, "git-bisect", &p)) || !isspace(*p))
> +		return 0;
> +	p += strspn(p, " \t");
> +
> +	char *word_end = (char*)p + strcspn(p, " \t");
> +	char *rev = word_end + strspn(word_end, " \t");

Are these lines new in this round?

These break builds with -Werror=declaration-after-statement.
Junio C Hamano Jan. 24, 2021, 5:13 a.m. UTC | #2
Junio C Hamano <gitster@pobox.com> writes:

> Miriam Rubio <mirucam@gmail.com> writes:
>
>> From: Pranit Bauva <pranit.bauva@gmail.com>
>>
>> Reimplement the `bisect_replay` shell function in C and also add
>> `--bisect-replay` subcommand to `git bisect--helper` to call it from
>> git-bisect.sh
>> ...
>> +static int process_replay_line(struct bisect_terms *terms, struct strbuf *line)
>> +{
>> +	const char *p = line->buf + strspn(line->buf, " \t");
>> +
>> +	if ((!skip_prefix(p, "git bisect", &p) &&
>> +	!skip_prefix(p, "git-bisect", &p)) || !isspace(*p))
>> +		return 0;
>> +	p += strspn(p, " \t");
>> +
>> +	char *word_end = (char*)p + strcspn(p, " \t");
>> +	char *rev = word_end + strspn(word_end, " \t");
>
> Are these lines new in this round?
>
> These break builds with -Werror=declaration-after-statement.


The following fix-up is tentatively queued at the tip of the topic
in tonight's pushout.  There might be better way to do what this
function is doing (especially I am skeptical about the way the code
casts const away), so please don't blindly trust it, but at least it
should please the compiler.

(char*) should be spelled (char *), by the way.

Thanks.


diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 3b0a6a7d0c..c673f87795 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -918,14 +918,15 @@ static enum bisect_error bisect_log(void)
 static int process_replay_line(struct bisect_terms *terms, struct strbuf *line)
 {
 	const char *p = line->buf + strspn(line->buf, " \t");
+	char *word_end, *rev;
 
 	if ((!skip_prefix(p, "git bisect", &p) &&
 	!skip_prefix(p, "git-bisect", &p)) || !isspace(*p))
 		return 0;
 	p += strspn(p, " \t");
 
-	char *word_end = (char*)p + strcspn(p, " \t");
-	char *rev = word_end + strspn(word_end, " \t");
+	word_end = (char*)p + strcspn(p, " \t");
+	rev = word_end + strspn(word_end, " \t");
 	*word_end = '\0'; /* NUL-terminate the word */
 
 	get_terms(terms);
Miriam Rubio Jan. 25, 2021, 7:18 p.m. UTC | #3
Hi Junio,

El dom, 24 ene 2021 a las 1:22, Junio C Hamano (<gitster@pobox.com>) escribió:
>
> Miriam Rubio <mirucam@gmail.com> writes:
>
> > From: Pranit Bauva <pranit.bauva@gmail.com>
> >
> > Reimplement the `bisect_replay` shell function in C and also add
> > `--bisect-replay` subcommand to `git bisect--helper` to call it from
> > git-bisect.sh
> > ...
> > +static int process_replay_line(struct bisect_terms *terms, struct strbuf *line)
> > +{
> > +     const char *p = line->buf + strspn(line->buf, " \t");
> > +
> > +     if ((!skip_prefix(p, "git bisect", &p) &&
> > +     !skip_prefix(p, "git-bisect", &p)) || !isspace(*p))
> > +             return 0;
> > +     p += strspn(p, " \t");
> > +
> > +     char *word_end = (char*)p + strcspn(p, " \t");
> > +     char *rev = word_end + strspn(word_end, " \t");
>
> Are these lines new in this round?
>
Yes, they are a reviewer's suggestion.

> These break builds with -Werror=declaration-after-statement.
>
Sorry about this, my compiler sometimes does not alert with some -Werror.
I have just sent a new version.
Best,
Miriam
diff mbox series

Patch

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index a65244a0f5..4d8dca3717 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -31,6 +31,7 @@  static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --bisect-auto-next"),
 	N_("git bisect--helper --bisect-state (bad|new) [<rev>]"),
 	N_("git bisect--helper --bisect-state (good|old) [<rev>...]"),
+	N_("git bisect--helper --bisect-replay <filename>"),
 	NULL
 };
 
@@ -916,6 +917,78 @@  static enum bisect_error bisect_log(void)
 	return status ? BISECT_FAILED : BISECT_OK;
 }
 
+static int process_replay_line(struct bisect_terms *terms, struct strbuf *line)
+{
+	const char *p = line->buf + strspn(line->buf, " \t");
+
+	if ((!skip_prefix(p, "git bisect", &p) &&
+	!skip_prefix(p, "git-bisect", &p)) || !isspace(*p))
+		return 0;
+	p += strspn(p, " \t");
+
+	char *word_end = (char*)p + strcspn(p, " \t");
+	char *rev = word_end + strspn(word_end, " \t");
+	*word_end = '\0'; /* NUL-terminate the word */
+
+	get_terms(terms);
+	if (check_and_set_terms(terms, p))
+		return -1;
+
+	if (!strcmp(p, "start")) {
+		struct strvec argv = STRVEC_INIT;
+		int res;
+		sq_dequote_to_strvec(rev, &argv);
+		res = bisect_start(terms, argv.v, argv.nr);
+		strvec_clear(&argv);
+		return res;
+	}
+
+	if (one_of(p, terms->term_good,
+	   terms->term_bad, "skip", NULL))
+		return bisect_write(p, rev, terms, 0);
+
+	if (!strcmp(p, "terms")) {
+		struct strvec argv = STRVEC_INIT;
+		int res;
+		sq_dequote_to_strvec(rev, &argv);
+		res = bisect_terms(terms, argv.nr == 1 ? argv.v[0] : NULL);
+		strvec_clear(&argv);
+		return res;
+	}
+	error(_("'%s'?? what are you talking about?"), p);
+
+	return -1;
+}
+
+static enum bisect_error bisect_replay(struct bisect_terms *terms, const char *filename)
+{
+	FILE *fp = NULL;
+	enum bisect_error res = BISECT_OK;
+	struct strbuf line = STRBUF_INIT;
+
+	if (is_empty_or_missing_file(filename))
+		return error(_("cannot read file '%s' for replaying"), filename);
+
+	if (bisect_reset(NULL))
+		return BISECT_FAILED;
+
+	fp = fopen(filename, "r");
+	if (!fp)
+		return BISECT_FAILED;
+
+	while ((strbuf_getline(&line, fp) != EOF) && !res){
+		res = process_replay_line(terms, &line);
+	}
+
+	strbuf_release(&line);
+	fclose(fp);
+
+	if (res)
+		return BISECT_FAILED;
+
+	return bisect_auto_next(terms, NULL);
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
@@ -929,7 +1002,8 @@  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		BISECT_NEXT,
 		BISECT_AUTO_NEXT,
 		BISECT_STATE,
-		BISECT_LOG
+		BISECT_LOG,
+		BISECT_REPLAY
 	} cmdmode = 0;
 	int res = 0, nolog = 0;
 	struct option options[] = {
@@ -953,6 +1027,8 @@  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("mark the state of ref (or refs)"), BISECT_STATE),
 		OPT_CMDMODE(0, "bisect-log", &cmdmode,
 			 N_("list the bisection steps so far"), BISECT_LOG),
+		OPT_CMDMODE(0, "bisect-replay", &cmdmode,
+			 N_("replay the bisection process from the given file"), BISECT_REPLAY),
 		OPT_BOOL(0, "no-log", &nolog,
 			 N_("no log for BISECT_WRITE")),
 		OPT_END()
@@ -1020,6 +1096,12 @@  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			return error(_("--bisect-log requires 0 arguments"));
 		res = bisect_log();
 		break;
+	case BISECT_REPLAY:
+		if (argc != 1)
+			return error(_("no logfile given"));
+		set_terms(&terms, "bad", "good");
+		res = bisect_replay(&terms, argv[0]);
+		break;
 	default:
 		BUG("unknown subcommand %d", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index c6149846ff..79bcd31bd7 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -77,38 +77,6 @@  bisect_visualize() {
 	eval '"$@"' --bisect -- $(cat "$GIT_DIR/BISECT_NAMES")
 }
 
-bisect_replay () {
-	file="$1"
-	test "$#" -eq 1 || die "$(gettext "No logfile given")"
-	test -r "$file" || die "$(eval_gettext "cannot read \$file for replaying")"
-	git bisect--helper --bisect-reset || exit
-	oIFS="$IFS" IFS="$IFS$(printf '\015')"
-	while read git bisect command rev tail
-	do
-		test "$git $bisect" = "git bisect" || test "$git" = "git-bisect" || continue
-		if test "$git" = "git-bisect"
-		then
-			rev="$command"
-			command="$bisect"
-		fi
-		get_terms
-		git bisect--helper --check-and-set-terms "$command" "$TERM_GOOD" "$TERM_BAD" || exit
-		get_terms
-		case "$command" in
-		start)
-			eval "git bisect--helper --bisect-start $rev $tail" ;;
-		"$TERM_GOOD"|"$TERM_BAD"|skip)
-			git bisect--helper --bisect-write "$command" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit;;
-		terms)
-			git bisect--helper --bisect-terms $rev || exit;;
-		*)
-			die "$(gettext "?? what are you talking about?")" ;;
-		esac
-	done <"$file"
-	IFS="$oIFS"
-	git bisect--helper --bisect-auto-next || exit
-}
-
 bisect_run () {
 	git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit
 
@@ -203,7 +171,7 @@  case "$#" in
 	reset)
 		git bisect--helper --bisect-reset "$@" ;;
 	replay)
-		bisect_replay "$@" ;;
+		git bisect--helper --bisect-replay "$@" || exit;;
 	log)
 		git bisect--helper --bisect-log || exit;;
 	run)