From: Lucas Gabriel Vuotto <lucas@sexy.is>
Subject: got: refactor choose_patch
To: gameoftrees@openbsd.org
Date: Sun, 28 Apr 2024 15:10:18 +0000

As the topic reads. There is a regress that had to be changed now that a
got_error is returned on EOF case. As an extra upside, now regress
partially covers the interactive case too.

The diff is a bit more noisy than it could be, as it also covers for
easing adding more options in the future, namely "Q". If a more minimal
diff is preferred, Omar has one in his tree.

Thinking aloud about it, regress could fully cover it by means of file
redirection, in the same fashion as -F paths but removing the response
from the expected output file, right?

	Lucas

-----------------------------------------------
commit 8863fdd67195fdc4f2aafc4dd9eb41b06bc65749 (choose-patch-refactor)
from: Lucas Gabriel Vuotto <lucas@lgv5.net>
date: Sun Apr 28 14:51:40 2024 UTC
 
 got: refactor choose_patch
 
 Unify the handling of a response file and interactive use, handle EOF,
 fix a memleak and make it easier to extend in the future. In particular,
 interactive mode now doesn't loop forever on EOF.
 
 Joint work with op.
 
diff ebd4d9a0266bb7a7145bcd6138bf64a5b0c1d004 8863fdd67195fdc4f2aafc4dd9eb41b06bc65749
commit - ebd4d9a0266bb7a7145bcd6138bf64a5b0c1d004
commit + 8863fdd67195fdc4f2aafc4dd9eb41b06bc65749
blob - 855d9d97039be14a03f12d2f1a5712cfa8d7574b
blob + 3847037924689c032af3f9d5b1c39909cdc75bb2
--- got/got.c
+++ got/got.c
@@ -8636,76 +8636,77 @@ show_change(unsigned char status, const char *path, FI
 	return NULL;
 }
 
+#define CHOOSE_PATCH_VALID_RESPONSES	"ynq"
+
 static const struct got_error *
 choose_patch(int *choice, void *arg, unsigned char status, const char *path,
     FILE *patch_file, int n, int nchanges)
 {
 	const struct got_error *err = NULL;
-	char *line = NULL;
+	char *nl, *line = NULL;
+	FILE *fp = stdin;
 	size_t linesize = 0;
 	ssize_t linelen;
-	int resp = ' ';
+	int interactive = 1;
 	struct choose_patch_arg *a = arg;
 
 	*choice = GOT_PATCH_CHOICE_NONE;
 
 	if (a->patch_script_file) {
-		char *nl;
-		err = show_change(status, path, patch_file, n, nchanges,
-		    a->action);
-		if (err)
-			return err;
-		linelen = getline(&line, &linesize, a->patch_script_file);
-		if (linelen == -1) {
-			if (ferror(a->patch_script_file))
-				return got_error_from_errno("getline");
-			return NULL;
-		}
-		nl = strchr(line, '\n');
-		if (nl)
-			*nl = '\0';
-		if (strcmp(line, "y") == 0) {
-			*choice = GOT_PATCH_CHOICE_YES;
-			printf("y\n");
-		} else if (strcmp(line, "n") == 0) {
-			*choice = GOT_PATCH_CHOICE_NO;
-			printf("n\n");
-		} else if (strcmp(line, "q") == 0 &&
-		    status == GOT_STATUS_MODIFY) {
-			*choice = GOT_PATCH_CHOICE_QUIT;
-			printf("q\n");
-		} else
+		interactive = 0;
+		fp = a->patch_script_file;
+	}
+
+ prompt:
+	err = show_change(status, path, patch_file, n, nchanges,
+	    a->action);
+	if (err)
+		return err;
+
+	linelen = getline(&line, &linesize, fp);
+	if (linelen == -1) {
+		free(line);
+		if (ferror(fp))
+			return got_error_from_errno("getline");
+		if (feof(fp))
+			return got_error(GOT_ERR_EOF);
+		return NULL;
+	}
+
+	nl = strchr(line, '\n');
+	if (nl)
+		*nl = '\0';
+	if (strlen(line) != 1 ||
+	    !strchr(CHOOSE_PATCH_VALID_RESPONSES, line[0])) {
+		printf("invalid response '%s'\n", line);
+		if (interactive)
+			goto prompt;
+	}
+
+	if (interactive) {
+		if (status != GOT_STATUS_MODIFY && line[0] == 'q') {
 			printf("invalid response '%s'\n", line);
-		free(line);
-		return NULL;
-	}
-
-	while (resp != 'y' && resp != 'n' && resp != 'q') {
-		err = show_change(status, path, patch_file, n, nchanges,
-		    a->action);
-		if (err)
-			return err;
-		resp = getchar();
-		if (resp == '\n')
-			resp = getchar();
-		if (status == GOT_STATUS_MODIFY) {
-			if (resp != 'y' && resp != 'n' && resp != 'q') {
-				printf("invalid response '%c'\n", resp);
-				resp = ' ';
-			}
-		} else if (resp != 'y' && resp != 'n') {
-				printf("invalid response '%c'\n", resp);
-				resp = ' ';
+			goto prompt;
 		}
 	}
 
-	if (resp == 'y')
+	switch (line[0]) {
+	case 'y':
 		*choice = GOT_PATCH_CHOICE_YES;
-	else if (resp == 'n')
+		break;
+	case 'n':
 		*choice = GOT_PATCH_CHOICE_NO;
-	else if (resp == 'q' && status == GOT_STATUS_MODIFY)
-		*choice = GOT_PATCH_CHOICE_QUIT;
+		break;
+	case 'q':
+		if (status == GOT_STATUS_MODIFY)
+			*choice = GOT_PATCH_CHOICE_QUIT;
+		break;
+	}
 
+	if (!interactive)
+		printf("%c\n", line[0]);
+
+	free(line);
 	return NULL;
 }
 
blob - 3c2cfbb76859df180a86e0ad56d976c5f0327eb8
blob + bb8f0dfe722dc734a56ccff46a57c1cf44e3005b
--- regress/cmdline/stage.sh
+++ regress/cmdline/stage.sh
@@ -2396,7 +2396,7 @@ stage this change? [y/n/q] y
 M  numbers (change 2 of 3)
 EOF
 	echo -n "stage this change? [y/n/q] " >> $testroot/stdout.expected
-	echo "got: invalid patch choice" > $testroot/stderr.expected
+	echo "got: unexpected end of file" > $testroot/stderr.expected
 	cmp -s $testroot/stderr.expected $testroot/stderr
 	ret=$?
 	if [ $ret -ne 0 ]; then