On Sun, Apr 28, 2024 at 09:44:25PM GMT, Stefan Sperling wrote:
> Please try to avoid 'goto' for control-flow purposes, other than
> the usual 'goto error' cleanup case. It is very rare that such
> jumps cannot be expressed as a loop that reads better.

Fair. I avoided it as I tend to find loops with break in their body
rather confusing. Here's an updated and rebased version.


-----------------------------------------------
commit 2ef87f4739bf699634d5f1ba92d67d6dafa3c7d8 (choose-patch-refactor)
from: Lucas Gabriel Vuotto <lucas@lgv5.net>
date: Mon Apr 29 05:56:00 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 b2c9f618aa4d760f0edfc7dbf6bcd01ca38aca17 2ef87f4739bf699634d5f1ba92d67d6dafa3c7d8
commit - b2c9f618aa4d760f0edfc7dbf6bcd01ca38aca17
commit + 2ef87f4739bf699634d5f1ba92d67d6dafa3c7d8
blob - 855d9d97039be14a03f12d2f1a5712cfa8d7574b
blob + 92d7198e1d1093296cd4b3a1cc0d49a6de2c3de5
--- got/got.c
+++ got/got.c
@@ -8636,76 +8636,80 @@ 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;
+		interactive = 0;
+		fp = a->patch_script_file;
+	}
+
+	for (;;) {
 		err = show_change(status, path, patch_file, n, nchanges,
 		    a->action);
 		if (err)
 			return err;
-		linelen = getline(&line, &linesize, a->patch_script_file);
+
+		linelen = getline(&line, &linesize, fp);
 		if (linelen == -1) {
-			if (ferror(a->patch_script_file))
+			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 (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
+		if (strlen(line) != 1 ||
+		    !strchr(CHOOSE_PATCH_VALID_RESPONSES, line[0])) {
 			printf("invalid response '%s'\n", line);
-		free(line);
-		return NULL;
-	}
+			if (interactive)
+				continue;
+		}
 
-	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 = ' ';
+		if (interactive) {
+			if (status != GOT_STATUS_MODIFY && line[0] == 'q') {
+				printf("invalid response '%s'\n", line);
+				continue;
 			}
-		} else if (resp != 'y' && resp != 'n') {
-				printf("invalid response '%c'\n", resp);
-				resp = ' ';
 		}
+
+		break;
 	}
 
-	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