On Tue, Jun 21, 2022 at 11:37:01AM +0200, Omar Polo wrote:
> Stefan Sperling <stsp@stsp.name> wrote:
> > On Tue, Jun 21, 2022 at 09:35:14AM +0200, Omar Polo wrote:
> > > The diff attempts to fix both issue.  I'm still not too keen on looking
> > > for an ENOENT thought.
> > 
> > Catching an error with code == GOT_ERR_ERRNO is perfectly valid.
> > GOT_ERR_ERRNO + ENOENT is indeed the correct error condition to check for.
> > Your open_blob() function eventually ends up in got_object_open_loose_fd()
> > which runs open(2) and converts failures to got_error_from_errno2().
> > 
> > On IRC I was thinking of the packed object case, in which case the
> > error would be GOT_ERR_NO_OBJ. But it turns out all the got_object_open()
> > function hide this error from you and fall back on trying to open a loose
> > object instead. So if the object does not exist at all, neither in packed
> > nor loose form, you'll only ever see the open(2) failure.
> > 
> > > diff 6131ff18e81056001a823f913094a92c10580cba /home/op/w/got
> > > blob - 9209e54ce7111b54b69980a44a48a92a333e5888
> > > file + lib/patch.c
> > > --- lib/patch.c
> > > +++ lib/patch.c
> > > @@ -643,7 +643,11 @@ apply_patch(int *overlapcnt, struct got_worktree *work
> > >  	/* don't run the diff3 merge on creations/deletions */
> > >  	if (*p->blob != '\0' && p->old != NULL && p->new != NULL) {
> > >  		err = open_blob(&apath, &afile, p->blob, repo);
> > > -		if (err && err->code != GOT_ERR_NOT_REF)
> > > +		/*
> > > +		 * ignore failures to open this blob, we might have
> > > +		 * parsed gibberish.
> > > +		 */
> > > +		if (err && err->code != GOT_ERR_ERRNO && errno == ENOENT)
> > >  			return err;
> > >  		else if (err == NULL)
> > >  			do_merge = 1;
> > 
> > Above condition looks wrong. Did you mean this?
> > 
> > 		if (err && !(err->code == GOT_ERR_ERRNO && errno == ENOENT))
> > 
> > Which could of course also be expressed as below, if you prefer:
> > 
> > 		if (err && (err->code != GOT_ERR_ERRNO || errno != ENOENT))
> 
> yes...  thanks for spotting it!
> 
> diff below also tweaks the test case by using a dummy commit id instead
> of the real one.

Looks great. Ok

I have another suggestion:

> +	cat <<EOF > $testroot/wt/patch
> +I've got a
> +blob - aaaabbbbccccddddeeeeffff0000111122223333
> +and also a
> +blob + 0000111122223333444455556666777788889999
> +for this dummy diff
> +--- alpha
> ++++ alpha
> +@@ -1 +1 @@
> +-alpha
> ++ALPHA
> +will it work?
> +EOF
> +
> +	(cd $testroot/wt && got revert alpha > /dev/null && got patch patch) \
> +		> $testroot/stdout
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		test_done $testroot $ret
> +		return 1
> +	fi
> +
> +	echo 'M  alpha' > $testroot/stdout.expected

The test doesn't check whether 3-way merge or search/replace was used.
Which kind of defeats the point, doesn't it?

'got patch' 3-way merge has the same possible outcomes as 'got cherrypick',
Except merge is only used if the file already exists on disk, and add/delete
is done elsewhere, so we are left with only the following two status codes
as they appear in 'got cherrypick':

	       G      file was merged
	       C      file was merged and conflicts occurred during merge

I would propose to make 'got patch' use status code as follows:

	       M      file was modified
	       G      file was merged using a merge-base found in repository
	       C      file was merged and conflicts occurred during merge
	       D      file was deleted
	       A      file was added
	       #      failed to patch the file

And then our tests could check for M or G/C to see which algorithm
was used to apply changes to a file.  What do you think?