On Sun, Oct 22, 2023 at 09:08:32PM +0200, Omar Polo wrote:
> I'd like to resume this to eventually make some more progress on the
> patch code.
> 
> > 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.
> 
> I've dropped this part and the regress test.  To be fair I don't like
> the idea of silently switching from diff3 merge to search/replace for
> binary files.  Since the diff3 code is not ready to deal with NULs, it's
> guarded and will return an error.  If we ever fix it, patch will
> immediately start to work with binary files too.
> 
> thoughts/oks?

I am fine with this. Binary files in patches are rare, so we don't need
to try too hard to make them work.

I still believe falling back on search/replace would really not be that bad,
provided that we disable any fuzzy matching, i.e. only patch binary files
which match 100% in order to avoid mangled results. This would probably result
in more user-friendly behaviour than just raising an error. As a worst-case
fallback 'got patch' could do what lib/worktree.c:merge_binary_file() does.

In any case, what you have now is good enough to go in. We can always
tweak things later.

Just one nit:

> @@ -392,14 +402,15 @@ copy(FILE *tmp, FILE *orig, off_t copypos, off_t pos)
>  	return NULL;
>  }
>  
> -static int linecmp(const char *, const char *, int *);
> +static int lineq(struct got_patch_line *, const char *, size_t, int *);

This function name is a little too dense for my taste.
I see it is a portmanteau of "line" and "equal" but could we spell this
out in a more obvious way? For example, lines_eq() or lines_equal()?