From: Omar Polo <op@omarpolo.com>
Subject: Re: got patch vs binary files
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Mon, 29 May 2023 08:52:06 +0200

ping

On 2023/05/12 21:06:15 +0200, Omar Polo <op@omarpolo.com> wrote:
> back in february naddy reported that got patch choked on the perl
> update.
> 
> https://marc.gameoftrees.org/thread/1675780522.55927_0.html
> 
> The problem was two-fold:
>   1. the "Result too large" for very long lines
>   2. NUL bytes in the middle of the lines
> 
> Here I'm adressing the most interesting one, the sencond.  I'll handle
> the very long lines case next.
> 
> got patch assumes lines can be encoded as strings, so embedded NUL
> bytes effectively truncates the line.  diff below changes this
> assumption, by considering lines just binary strings LF-delimited.
> 
> While doing this, I took the chance to ship some other minor changes.
> 
> The type of the line (i.e. '+', '-' or ' ' aka context) now is a
> separate field instead of being encoded in the first byte of each
> line, this simplifies the various +1/-1 that were needed otherwise.
> 
> I've renamed linecmp into lineq and made it boolean: it's simpler to
> write, to use and we'll never be interested in sorting lines anyway.
> 
> One quirk is that I'm still allocating the space for a NUL terminator.
> This is not required, and in fact makes pushline() maybe not obvious,
> but it's useful for debugging so it's worth it I think.
> 
> The diff is not committable as-is however.  I had to disable a check
> in diff3 to allow got_merge_diff3 to proceed on binary files.  I'm not
> sure what we want to do in for this, but I believe that even without
> the will to support "binary patches" most of the diff still makes
> sense, as it propagate lengths rather than recomputing strlen() here
> and there and avoids some extra useless allocations in got-read-patch.
> 
> 
> diff /home/op/w/got
> commit - ba0bed23e515b14cd3fe877c6847785c8c29971e
> path + /home/op/w/got
> blob - 5cd79150cc6476f78c90b997d76b00eaa99fc341
> file + lib/diff3.c
> --- lib/diff3.c
> +++ lib/diff3.c
> @@ -231,6 +231,7 @@ diffreg(BUF **d, const char *path1, const char *path2,
>  	if (err)
>  		goto done;
>  
> +#if 0
>  	if (diffreg_result) {
>  		struct diff_result *diff_result = diffreg_result->result;
>  		int atomizer_flags = (diff_result->left->atomizer_flags |
> @@ -240,6 +241,7 @@ diffreg(BUF **d, const char *path1, const char *path2,
>  			goto done;
>  		}
>  	}
> +#endif
>  
>  	err = got_diffreg_output(NULL, NULL, diffreg_result, 1, 1, "", "",
>  	    GOT_DIFF_OUTPUT_PLAIN, 0, outfile);
> blob - 9f524a6bfc0deae40498090e43fe7f9238eb4e64
> file + lib/patch.c
> --- lib/patch.c
> +++ lib/patch.c
> @@ -59,6 +59,12 @@ struct got_patch_hunk {
>  #define MIN(a, b) ((a) < (b) ? (a) : (b))
>  #endif
>  
> +struct got_patch_line {
> +	char	 mode;
> +	char	*line;
> +	size_t	 len;
> +};
> +
>  struct got_patch_hunk {
>  	STAILQ_ENTRY(got_patch_hunk) entries;
>  	const struct got_error *err;
> @@ -72,7 +78,7 @@ struct got_patch_hunk {
>  	int	new_lines;
>  	size_t	len;
>  	size_t	cap;
> -	char	**lines;
> +	struct got_patch_line *lines;
>  };
>  
>  STAILQ_HEAD(got_patch_hunk_head, got_patch_hunk);
> @@ -128,7 +134,7 @@ patch_free(struct got_patch *p)
>  		STAILQ_REMOVE_HEAD(&p->head, entries);
>  
>  		for (i = 0; i < h->len; ++i)
> -			free(h->lines[i]);
> +			free(h->lines[i].line);
>  		free(h->lines);
>  		free(h);
>  	}
> @@ -141,7 +147,7 @@ pushline(struct got_patch_hunk *h, const char *line)
>  }
>  
>  static const struct got_error *
> -pushline(struct got_patch_hunk *h, const char *line)
> +pushline(struct got_patch_hunk *h, const char *line, size_t len)
>  {
>  	void 	*t;
>  	size_t	 newcap;
> @@ -157,10 +163,14 @@ pushline(struct got_patch_hunk *h, const char *line)
>  		h->cap = newcap;
>  	}
>  
> -	if ((t = strdup(line)) == NULL)
> -		return got_error_from_errno("strdup");
> +	if ((t = malloc(len - 1)) == NULL)
> +		return got_error_from_errno("malloc");
> +	memcpy(t, line + 1, len - 1);	/* skip the line type */
>  
> -	h->lines[h->len++] = t;
> +	h->lines[h->len].mode = *line;
> +	h->lines[h->len].line = t;
> +	h->lines[h->len].len = len - 2;	/* skip line type and trailing NUL */
> +	h->len++;
>  	return NULL;
>  }
>  
> @@ -301,7 +311,7 @@ recv_patch(struct imsgbuf *ibuf, int *done, struct got
>  			}
>  
>  			if (*t != '\\')
> -				err = pushline(h, t);
> +				err = pushline(h, t, datalen);
>  			else if (lastmode == '-')
>  				h->old_nonl = 1;
>  			else if (lastmode == '+')
> @@ -351,10 +361,10 @@ reverse_patch(struct got_patch *p)
>  		h->new_nonl = tmp;
>  
>  		for (i = 0; i < h->len; ++i) {
> -			if (*h->lines[i] == '+')
> -				*h->lines[i] = '-';
> -			else if (*h->lines[i] == '-')
> -				*h->lines[i] = '+';
> +			if (h->lines[i].mode == '+')
> +				h->lines[i].mode = '-';
> +			else if (h->lines[i].mode == '-')
> +				h->lines[i].mode = '+';
>  		}
>  	}
>  }
> @@ -392,14 +402,15 @@ static int linecmp(const char *, const char *, int *);
>  	return NULL;
>  }
>  
> -static int linecmp(const char *, const char *, int *);
> +static int lineq(struct got_patch_line *, const char *, size_t, int *);
>  
>  static const struct got_error *
>  locate_hunk(FILE *orig, struct got_patch_hunk *h, off_t *pos, int *lineno)
>  {
>  	const struct got_error *err = NULL;
> +	struct got_patch_line *l = &h->lines[0];
>  	char *line = NULL;
> -	char mode = *h->lines[0];
> +	char mode = l->mode;
>  	size_t linesize = 0;
>  	ssize_t linelen;
>  	off_t match = -1;
> @@ -414,12 +425,10 @@ locate_hunk(FILE *orig, struct got_patch_hunk *h, off_
>  				err = got_error(GOT_ERR_HUNK_FAILED);
>  			break;
>  		}
> -		if (line[linelen - 1] == '\n')
> -			line[linelen - 1] = '\0';
>  		(*lineno)++;
>  
> -		if ((mode == ' ' && !linecmp(h->lines[0] + 1, line, &mangled)) ||
> -		    (mode == '-' && !linecmp(h->lines[0] + 1, line, &mangled)) ||
> +		if ((mode == ' ' && lineq(l, line, linelen, &mangled)) ||
> +		    (mode == '-' && lineq(l, line, linelen, &mangled)) ||
>  		    (mode == '+' && *lineno == h->old_from)) {
>  			match = ftello(orig);
>  			if (match == -1) {
> @@ -449,27 +458,34 @@ linecmp(const char *a, const char *b, int *mangled)
>  }
>  
>  static int
> -linecmp(const char *a, const char *b, int *mangled)
> +lineq(struct got_patch_line *l, const char *b, size_t len, int *mangled)
>  {
> -	int c;
> +	char *a = l->line;
> +	size_t i, j;
>  
> +	if (b[len - 1] == '\n')
> +		len--;
> +
>  	*mangled = 0;
> -	c = strcmp(a, b);
> -	if (c == 0)
> -		return c;
> +	if (l->len == len && !memcmp(a, b, len))
> +		return 1;
>  
>  	*mangled = 1;
> +
> +	i = j = 0;
>  	for (;;) {
> -		while (*a == '\t' || *a == ' ' || *a == '\f')
> -			a++;
> -		while (*b == '\t' || *b == ' ' || *b == '\f')
> -			b++;
> -		if (*a == '\0' || *a != *b)
> +		while (i < l->len &&
> +		    (a[i] == '\t' || a[i] == ' ' || a[i] == '\f'))
> +			i++;
> +		while (j < len &&
> +		    (b[j] == '\t' || b[j] == ' ' || b[j] == '\f'))
> +			j++;
> +		if (i == l->len || j == len || a[i] != b[j])
>  			break;
> -		a++, b++;
> +		i++, j++;
>  	}
>  
> -	return *a - *b;
> +	return (i == l->len && j == len);
>  }
>  
>  static const struct got_error *
> @@ -482,7 +498,7 @@ test_hunk(FILE *orig, struct got_patch_hunk *h)
>  	int mangled;
>  
>  	for (i = 0; i < h->len; ++i) {
> -		switch (*h->lines[i]) {
> +		switch (h->lines[i].mode) {
>  		case '+':
>  			continue;
>  		case ' ':
> @@ -496,9 +512,7 @@ test_hunk(FILE *orig, struct got_patch_hunk *h)
>  					    GOT_ERR_HUNK_FAILED);
>  				goto done;
>  			}
> -			if (line[linelen - 1] == '\n')
> -				line[linelen - 1] = '\0';
> -			if (linecmp(h->lines[i] + 1, line, &mangled)) {
> +			if (!lineq(&h->lines[i], line, linelen, &mangled)) {
>  				err = got_error(GOT_ERR_HUNK_FAILED);
>  				goto done;
>  			}
> @@ -522,13 +536,14 @@ apply_hunk(FILE *orig, FILE *tmp, struct got_patch_hun
>  	size_t linesize = 0, i, new = 0;
>  	char *line = NULL;
>  	char mode;
> +	size_t l;
>  	ssize_t linelen;
>  
>  	if (orig != NULL && fseeko(orig, from, SEEK_SET) == -1)
>  		return got_error_from_errno("fseeko");
>  
>  	for (i = 0; i < h->len; ++i) {
> -		switch (mode = *h->lines[i]) {
> +		switch (mode = h->lines[i].mode) {
>  		case '-':
>  		case ' ':
>  			(*lineno)++;
> @@ -541,18 +556,24 @@ apply_hunk(FILE *orig, FILE *tmp, struct got_patch_hun
>  				if (line[linelen - 1] == '\n')
>  					line[linelen - 1] = '\0';
>  				t = line;
> -			} else
> -				t = h->lines[i] + 1;
> +				l = linelen - 1;
> +			} else {
> +				t = h->lines[i].line;
> +				l = h->lines[i].len;
> +			}
>  			if (mode == '-')
>  				continue;
> -			if (fprintf(tmp, "%s\n", t) < 0) {
> +			if (fwrite(t, 1, l, tmp) != l ||
> +			    fputc('\n', tmp) == EOF) {
>  				err = got_error_from_errno("fprintf");
>  				goto done;
>  			}
>  			break;
>  		case '+':
>  			new++;
> -			if (fprintf(tmp, "%s", h->lines[i] + 1) < 0) {
> +			t = h->lines[i].line;
> +			l = h->lines[i].len;
> +			if (fwrite(t, 1, l, tmp) != l) {
>  				err = got_error_from_errno("fprintf");
>  				goto done;
>  			}
> blob - a5fe8c2470856e960745214cc536ddbc804c7ef0
> file + libexec/got-read-patch/got-read-patch.c
> --- libexec/got-read-patch/got-read-patch.c
> +++ libexec/got-read-patch/got-read-patch.c
> @@ -445,23 +445,29 @@ send_line(const char *line)
>  }
>  
>  static const struct got_error *
> -send_line(const char *line)
> +send_line(const char *line, size_t len)
>  {
>  	static const struct got_error *err = NULL;
> -	char *p = NULL;
> +	struct iovec iov[2];
> +	int iovcnt = 0;
>  
> +	memset(&iov, 0, sizeof(iov));
> +
>  	if (*line != '+' && *line != '-' && *line != ' ' && *line != '\\') {
> -		if (asprintf(&p, " %s", line) == -1)
> -			return got_error_from_errno("asprintf");
> -		line = p;
> +		iov[iovcnt].iov_base = (void *)" ";
> +		iov[iovcnt].iov_len = 1;
> +		iovcnt++;
>  	}
>  
> -	if (imsg_compose(&ibuf, GOT_IMSG_PATCH_LINE, 0, 0, -1,
> -	    line, strlen(line) + 1) == -1)
> +	iov[iovcnt].iov_base = (void *)line;
> +	iov[iovcnt].iov_len = len;
> +	iovcnt++;
> +
> +	if (imsg_composev(&ibuf, GOT_IMSG_PATCH_LINE, 0, 0, -1,
> +	    iov, iovcnt) == -1)
>  		err = got_error_from_errno(
>  		    "imsg_compose GOT_IMSG_PATCH_LINE");
>  
> -	free(p);
>  	return err;
>  }
>  
> @@ -478,7 +484,7 @@ peek_special_line(FILE *fp)
>  	}
>  
>  	if (ch == '\\') {
> -		err = send_line("\\");
> +		err = send_line("\\", 2);
>  		if (err)
>  			return err;
>  	}
> @@ -568,7 +574,7 @@ parse_hunk(FILE *fp, int *done)
>  			goto done;
>  		}
>  
> -		err = send_line(line);
> +		err = send_line(line, linelen);
>  		if (err)
>  			goto done;
>  
> blob - 17db471e5c3aa81e0eb9233b406861cdccd08fcd
> file + regress/cmdline/patch.sh
> --- regress/cmdline/patch.sh
> +++ regress/cmdline/patch.sh
> @@ -1956,6 +1956,57 @@ test_parseargs "$@"
>  	test_done $testroot 0
>  }
>  
> +test_patch_binary() {
> +	local testroot=`test_init patch_binary`
> +
> +	if ! got checkout "$testroot/repo" "$testroot/wt" >/dev/null; then
> +		test_done "$testroot" $ret
> +		return 1
> +	fi
> +
> +	printf 'e\0ta' > $testroot/wt/eta
> +	(cd "$testroot/wt" && got add eta && got commit -m '+eta') >/dev/null
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got add failed unexpectedly" >&2
> +		test_done "$testroot" $ret
> +		return 1
> +	fi
> +
> +	printf 'et\0a' | tee "$testroot/wt/eta.expected" > $testroot/wt/eta
> +
> +	(cd "$testroot/wt" && got diff -a > patch && got revert eta) >/dev/null
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got diff or revert failed unexpectedly" >&2
> +		test_done "$testroot" $ret
> +		return 1
> +	fi
> +
> +	(cd "$testroot/wt" && got patch <patch) > $testroot/stdout
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got patch failed unexpectedly" >&2
> +		test_done "$testroot" $ret
> +		return 1
> +	fi
> +
> +	echo 'G  eta' > $testroot/stdout.expected
> +	if ! cmp -s "$testroot/stdout.expected" "$testroot/stdout"; then
> +		diff -u "$testroot/stdout.expected" "$testroot/stdout"
> +		test_done "$testroot" 1
> +		return 1
> +	fi
> +
> +	if ! cmp -s "$testroot/wt/eta.expected" "$testroot/wt/eta"; then
> +		diff -u "$testroot/wt/eta.expected" "$testroot/wt/eta"
> +		test_done "$testroot" 1
> +		return 1
> +	fi
> +
> +	test_done "$testroot" 0
> +}
> +
>  test_parseargs "$@"
>  run_test test_patch_basic
>  run_test test_patch_dont_apply
> @@ -1986,3 +2037,4 @@ run_test test_patch_remove_binary_file
>  run_test test_patch_newfile_xbit_git_diff
>  run_test test_patch_umask
>  run_test test_patch_remove_binary_file
> +run_test test_patch_binary