"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Omar Polo <op@omarpolo.com>
Subject:
Re: got patch: diff3 merge for git diffs
To:
Omar Polo <op@omarpolo.com>
Cc:
gameoftrees@openbsd.org
Date:
Mon, 27 Jun 2022 15:01:51 +0200

Download raw body.

Thread
Omar Polo <op@omarpolo.com> wrote:
> the idea is to parse also the
> 
> 	index oldblobid...newblobid
> 
> field in git diffs for the diff3 merge machinery.  Yep, those are blob
> ids and not commit nor tree ids, I got it wrong initially too.
> 
> The diff seems to work (only tested thru the added regress) but could
> use some help to tidy it a bit.  One annoying thing is that git uses
> abbreviated blob ids, unlike got diff which prints the full blob id, at
> least with a default config.  The change in `blobid' (got-read-patch.c)
> is to accomodate for it, got_parse_sha1_digest fails if the given blob
> id is abbreviated.

I forgot to mention, there's another place I'm not so sure about:

> [...]

> -	if (!got_parse_sha1_digest(id.sha1, blobid))
> -		return got_error(GOT_ERR_BAD_OBJ_ID_STR);
> +	if (strlen(blobid) != 40) {

looking at the length of the blobid to judge if it's abbreviated or not
is fine?

> +		err = got_repo_match_object_id(&matched_id, NULL, blobid,
> +		    GOT_OBJ_TYPE_BLOB, NULL /* do not resolve tags */,
> +		    repo);
> +		if (err)
> +			return err;
> +		idptr = matched_id;
> +	} else {
> +		if (!got_parse_sha1_digest(id.sha1, blobid))
> +			return got_error(GOT_ERR_BAD_OBJ_ID_STR);
> +		idptr = &id;
> +	}

slightly revised diff with a more "git-looking" diff in
test_patch_merge_unknown_blob

diff refs/heads/main refs/heads/gitdiff
commit - 3d589bee0bbbe812bb91f3b0284fbf2596304132
commit + 943bdc96d99adddfe53e7cb9b793c495ff6645eb
blob - 965f8606f5dc0febfa47fceeba356519258f1021
blob + efe7f1a611a60b4528c47d210297d944f7e1fd38
--- lib/patch.c
+++ lib/patch.c
@@ -192,10 +192,11 @@ recv_patch(struct imsgbuf *ibuf, int *done, struct got
 		goto done;
 	}
 
-	if (*patch.cid != '\0' && *patch.blob != '\0') {
+	if (*patch.cid != '\0')
 		strlcpy(p->cid, patch.cid, sizeof(p->cid));
+
+	if (*patch.blob != '\0')
 		strlcpy(p->blob, patch.blob, sizeof(p->blob));
-	}
 
 	/* automatically set strip=1 for git-style diffs */
 	if (strip == -1 && patch.git &&
@@ -586,15 +587,25 @@ open_blob(char **path, FILE **fp, const char *blobid,
 {
 	const struct got_error *err = NULL;
 	struct got_blob_object *blob = NULL;
-	struct got_object_id id;
+	struct got_object_id id, *idptr, *matched_id = NULL;
 
 	*fp = NULL;
 	*path = NULL;
 
-	if (!got_parse_sha1_digest(id.sha1, blobid))
-		return got_error(GOT_ERR_BAD_OBJ_ID_STR);
+	if (strlen(blobid) != 40) {
+		err = got_repo_match_object_id(&matched_id, NULL, blobid,
+		    GOT_OBJ_TYPE_BLOB, NULL /* do not resolve tags */,
+		    repo);
+		if (err)
+			return err;
+		idptr = matched_id;
+	} else {
+		if (!got_parse_sha1_digest(id.sha1, blobid))
+			return got_error(GOT_ERR_BAD_OBJ_ID_STR);
+		idptr = &id;
+	}
 
-	err = got_object_open_as_blob(&blob, repo, &id, 8192);
+	err = got_object_open_as_blob(&blob, repo, idptr, 8192);
 	if (err)
 		goto done;
 
@@ -609,6 +620,8 @@ open_blob(char **path, FILE **fp, const char *blobid,
 done:
 	if (blob)
 		got_object_blob_close(blob);
+	if (matched_id != NULL)
+		free(matched_id);
 	if (err) {
 		if (*fp != NULL)
 			fclose(*fp);
@@ -647,7 +660,8 @@ apply_patch(int *overlapcnt, struct got_worktree *work
 		 * ignore failures to open this blob, we might have
 		 * parsed gibberish.
 		 */
-		if (err && !(err->code == GOT_ERR_ERRNO && errno == ENOENT))
+		if (err && !(err->code == GOT_ERR_ERRNO && errno == ENOENT) &&
+		    err->code != GOT_ERR_NO_OBJ)
 			return err;
 		else if (err == NULL)
 			do_merge = 1;
@@ -689,6 +703,8 @@ apply_patch(int *overlapcnt, struct got_worktree *work
 		goto done;
 
 	if (do_merge) {
+		const char *type, *id;
+
 		if (fseeko(afile, 0, SEEK_SET) == -1 ||
 		    fseeko(oldfile, 0, SEEK_SET) == -1 ||
 		    fseeko(tmpfile, 0, SEEK_SET) == -1) {
@@ -708,7 +724,15 @@ apply_patch(int *overlapcnt, struct got_worktree *work
 			goto done;
 		}
 
-		if (asprintf(&anclabel, "commit %s", p->cid) == -1) {
+		if (*p->cid != '\0') {
+			type = "commit";
+			id = p->cid;
+		} else {
+			type = "blob";
+			id = p->blob;
+		}
+
+		if (asprintf(&anclabel, "%s %s", type, id) == -1) {
 			err = got_error_from_errno("asprintf");
 			anclabel = NULL;
 			goto done;
blob - 05415ddde579ebfde446284ba961406bc37a4255
blob + d6eea51f8d3203ae05b110adaac04ee330bd4d28
--- libexec/got-read-patch/got-read-patch.c
+++ libexec/got-read-patch/got-read-patch.c
@@ -74,10 +74,11 @@ send_patch(const char *oldname, const char *newname, c
 	if (newname != NULL)
 		strlcpy(p.new, newname, sizeof(p.new));
 
-	if (commitid != NULL && blob != NULL) {
+	if (commitid != NULL)
 		strlcpy(p.cid, commitid, sizeof(p.cid));
+
+	if (blob != NULL)
 		strlcpy(p.blob, blob, sizeof(p.blob));
-	}
 
 	p.git = git;
 	if (imsg_compose(&ibuf, GOT_IMSG_PATCH, 0, 0, -1, &p, sizeof(p)) == -1)
@@ -129,7 +130,7 @@ filename(const char *at, char **name)
 }
 
 static const struct got_error *
-blobid(const char *line, char **blob)
+blobid(const char *line, char **blob, int git)
 {
 	uint8_t digest[SHA1_DIGEST_LENGTH];
 	size_t len;
@@ -140,7 +141,7 @@ blobid(const char *line, char **blob)
 	if ((*blob = strndup(line, len)) == NULL)
 		return got_error_from_errno("strndup");
 
-	if (!got_parse_sha1_digest(digest, *blob)) {
+	if (!git && !got_parse_sha1_digest(digest, *blob)) {
 		/* silently ignore invalid blob ids */
 		free(*blob);
 		*blob = NULL;
@@ -176,13 +177,16 @@ find_patch(int *done, FILE *fp)
 			err = filename(line+4, &new);
 		} else if (!git && !strncmp(line, "blob - ", 7)) {
 			free(blob);
-			err = blobid(line + 7, &blob);
+			err = blobid(line + 7, &blob, git);
 		} else if (rename && !strncmp(line, "rename to ", 10)) {
 			free(new);
 			err = filename(line + 10, &new);
 		} else if (git && !strncmp(line, "similarity index 100%", 21))
 			rename = 1;
-		else if (!strncmp(line, "diff --git a/", 13)) {
+		else if (git && !strncmp(line, "index ", 6)) {
+			free(blob);
+			err = blobid(line + 6, &blob, git);
+		} else if (!strncmp(line, "diff --git a/", 13)) {
 			git = 1;
 			free(commitid);
 			commitid = NULL;
@@ -190,10 +194,10 @@ find_patch(int *done, FILE *fp)
 			blob = NULL;
 		} else if (!git && !strncmp(line, "diff ", 5)) {
 			free(commitid);
-			err = blobid(line + 5, &commitid);
+			err = blobid(line + 5, &commitid, git);
 		} else if (!git && !strncmp(line, "commit - ", 9)) {
 			free(commitid);
-			err = blobid(line + 9, &commitid);
+			err = blobid(line + 9, &commitid, git);
 		}
 
 		if (err)
blob - 9fb1dd39304f1cd716283dd5684a1162ee8f745c
blob + 51903218ccbe2f14d00ba02be9bbc8ead7c2f1e4
--- regress/cmdline/patch.sh
+++ regress/cmdline/patch.sh
@@ -1507,6 +1507,62 @@ test_patch_merge_simple() {
 	test_done $testroot $ret
 }
 
+test_patch_merge_gitdiff() {
+	local testroot=`test_init patch_merge_gitdiff`
+
+	jot 10 > $testroot/repo/numbers
+	(cd $testroot/repo && git add numbers && \
+		git_commit $testroot/repo -m "nums")
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done $testroot $ret
+		return 1
+	fi
+
+	jot 10 | sed 's/4/four/g' > $testroot/repo/numbers
+	(cd $testroot/repo && git diff > $testroot/old.diff)
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done $testroot $ret
+		return 1
+	fi
+
+	# restore numbers
+	jot 10 > $testroot/repo/numbers
+
+	jot 10 | sed 's/6/six/g' > $testroot/repo/numbers
+	(cd $testroot/repo && git add numbers && \
+		git_commit $testroot/repo -m "edit")
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done $testroot $ret
+		return 1
+	fi
+
+	# now work with got:
+	got checkout $testroot/repo $testroot/wt > /dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done $testroot $ret
+		return 1
+	fi
+
+	(cd $testroot/wt && got patch $testroot/old.diff) > $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done $testroot $ret
+		return 1
+	fi
+
+	echo 'G  numbers' > $testroot/stdout.expected
+	cmp -s $testroot/stdout $testroot/stdout.expected
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout $testroot/stdout.expected
+	fi
+	test_done $testroot $ret
+}
+
 test_patch_merge_conflict() {
 	local testroot=`test_init patch_merge_conflict`
 
@@ -1665,7 +1721,36 @@ EOF
 	ret=$?
 	if [ $ret -ne 0 ]; then
 		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done $testroot $ret
+		return 1
 	fi
+
+	# try again with a git-style diff
+
+	cat <<EOF > $testroot/wt/patch
+diff --git a/alpha b/alpha
+index 0123456789ab..abcdef012345 100644
+--- a/alpha
++++ b/alpha
+@@ -1 +1 @@
+-alpha
++ALPHA
+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
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+	fi
 	test_done $testroot $ret
 }
 
@@ -1695,5 +1780,6 @@ run_test test_patch_with_path_prefix
 run_test test_patch_relpath_with_path_prefix
 run_test test_patch_reverse
 run_test test_patch_merge_simple
+run_test test_patch_merge_gitdiff
 run_test test_patch_merge_conflict
 run_test test_patch_merge_unknown_blob