merge-ll: expose revision names to custom drivers

PATCH Superseded Antonin Delpeuch via GitGitGadget 5 participants 15 messages Jan 18, 2024 – Jan 24, 2024 Documentation

lore

Share & tools

lore

Antonin Delpeuch via GitGitGadget

author

2024-01-18 14:26 UTC #0 lore raw

PATCH 1/1 merge-ll: expose revision names to custom drivers

From: Antonin Delpeuch <[email protected]>

Custom merge drivers need access to the names of the revisions they are working on, so that the merge conflict markers they introduce can refer to those revisions. The placeholders '%S', '%X' and '%Y' are introduced to this end.

CC Junio C Hamano <[email protected]>

Signed-off-by Antonin Delpeuch <[email protected]>

---
merge-ll: expose revision names to custom drivers

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1648%2Fwetneb%2Fmerge_driver_pathnames-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1648/wetneb/merge_driver_pathnames-v1
Pull-Request: https://github.com/git/git/pull/1648

 Documentation/gitattributes.txt | 10 +++++++---
 merge-ll.c                      | 17 ++++++++++++++---
 t/t6406-merge-attr.sh           | 16 +++++++++++-----
 3 files changed, 32 insertions(+), 11 deletions(-)

Documentation/gitattributes.txt +7 -3

index 201bdf5edbd..108c2e23baa 100644
@@ -1141,7 +1141,7 @@ command to run to merge ancestor's version (`%O`), current
 version (`%A`) and the other branches' version (`%B`).  These
 three tokens are replaced with the names of temporary files that
 hold the contents of these versions when the command line is
-built. Additionally, %L will be replaced with the conflict marker
+built. Additionally, `%L` will be replaced with the conflict marker
 size (see below).
 
 The merge driver is expected to leave the result of the merge in

@@ -1159,8 +1159,12 @@ When left unspecified, the driver itself is used for both
 internal merge and the final merge.
 
 The merge driver can learn the pathname in which the merged result
-will be stored via placeholder `%P`.
-
+will be stored via placeholder `%P`. Additionally, the names of the
+merge ancestor revision (`%S`), of the current revision (`%X`) and
+of the other branch (`%Y`) can also be supplied. Those are short
+revision names, optionally joined with the paths of the file in each
+revision. Those paths are only present if they differ and are separated
+from the revision by a colon.
 
 `conflict-marker-size`
 ^^^^^^^^^^^^^^^^^^^^^^

merge-ll.c +14 -3

index 1df58ebaac0..ae9eb69be14 100644
@@ -185,9 +185,9 @@ static void create_temp(mmfile_t *src, char *path, size_t len)
 static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 			mmbuffer_t *result,
 			const char *path,
-			mmfile_t *orig, const char *orig_name UNUSED,
-			mmfile_t *src1, const char *name1 UNUSED,
-			mmfile_t *src2, const char *name2 UNUSED,
+			mmfile_t *orig, const char *orig_name,
+			mmfile_t *src1, const char *name1,
+			mmfile_t *src2, const char *name2,
 			const struct ll_merge_options *opts,
 			int marker_size)
 {

@@ -222,6 +222,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 			strbuf_addf(&cmd, "%d", marker_size);
 		else if (skip_prefix(format, "P", &format))
 			sq_quote_buf(&cmd, path);
+		else if (skip_prefix(format, "S", &format))
+		    sq_quote_buf(&cmd, orig_name);
+		else if (skip_prefix(format, "X", &format))
+			sq_quote_buf(&cmd, name1);
+		else if (skip_prefix(format, "Y", &format))
+			sq_quote_buf(&cmd, name2);
 		else
 			strbuf_addch(&cmd, '%');
 	}

@@ -315,7 +321,12 @@ static int read_merge_config(const char *var, const char *value,
 		 *    %B - temporary file name for the other branches' version.
 		 *    %L - conflict marker length
 		 *    %P - the original path (safely quoted for the shell)
+		 *    %S - the revision for the merge base
+		 *    %X - the revision for our version
+		 *    %Y - the revision for their version
 		 *
+		 * If the file is not named indentically in all versions, then each
+		 * revision is joined with the corresponding path, separated by a colon.
 		 * The external merge driver should write the results in the
 		 * file named by %A, and signal that it has done with zero exit
 		 * status.

t/t6406-merge-attr.sh +11 -5

index 72f8c1722ff..156a1efacfe 100755
@@ -42,11 +42,15 @@ test_expect_success setup '
 	#!/bin/sh
 
 	orig="$1" ours="$2" theirs="$3" exit="$4" path=$5
+	orig_name="$6" our_name="$7" their_name="$8"
 	(
 		echo "orig is $orig"
 		echo "ours is $ours"
 		echo "theirs is $theirs"
 		echo "path is $path"
+		echo "orig_name is $orig_name"
+		echo "our_name is $our_name"
+		echo "their_name is $their_name"
 		echo "=== orig ==="
 		cat "$orig"
 		echo "=== ours ==="

@@ -121,7 +125,7 @@ test_expect_success 'custom merge backend' '
 
 	git reset --hard anchor &&
 	git config --replace-all \
-	merge.custom.driver "./custom-merge %O %A %B 0 %P" &&
+	merge.custom.driver "./custom-merge %O %A %B 0 %P %S %X %Y" &&
 	git config --replace-all \
 	merge.custom.name "custom merge driver for testing" &&

@@ -132,7 +136,8 @@ test_expect_success 'custom merge backend' '
 	o=$(git unpack-file main^:text) &&
 	a=$(git unpack-file side^:text) &&
 	b=$(git unpack-file main:text) &&
-	sh -c "./custom-merge $o $a $b 0 text" &&
+	base_revid=$(git rev-parse --short main^) &&
+	sh -c "./custom-merge $o $a $b 0 text $base_revid HEAD main" &&
 	sed -e 1,3d $a >check-2 &&
 	cmp check-1 check-2 &&
 	rm -f $o $a $b

@@ -142,7 +147,7 @@ test_expect_success 'custom merge backend' '
 
 	git reset --hard anchor &&
 	git config --replace-all \
-	merge.custom.driver "./custom-merge %O %A %B 1 %P" &&
+	merge.custom.driver "./custom-merge %O %A %B 1 %P %S %X %Y" &&
 	git config --replace-all \
 	merge.custom.name "custom merge driver for testing" &&

@@ -159,7 +164,8 @@ test_expect_success 'custom merge backend' '
 	o=$(git unpack-file main^:text) &&
 	a=$(git unpack-file anchor:text) &&
 	b=$(git unpack-file main:text) &&
-	sh -c "./custom-merge $o $a $b 0 text" &&
+	base_revid=$(git rev-parse --short main^) &&
+	sh -c "./custom-merge $o $a $b 0 text $base_revid HEAD main" &&
 	sed -e 1,3d $a >check-2 &&
 	cmp check-1 check-2 &&
 	sed -e 1,3d -e 4q $a >check-3 &&

@@ -173,7 +179,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'
 
 	git reset --hard anchor &&
 	git config --replace-all \
-	merge.custom.driver "./custom-merge %O %A %B 0 %P" &&
+	merge.custom.driver "./custom-merge %O %A %B 0 %P %S %X %Y" &&
 	git config --replace-all \
 	merge.custom.name "custom merge driver for testing" &&

base-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5

Signature · 1 line

gitgitgadget

Kristoffer Haugsbakk

2024-01-18 15:25 UTC #1 lore raw

Quoted 8 lines

are working on, so that the merge conflict markers they introduce can refer to those revisions. The placeholders '%S', '%X' and '%Y' are introduced to this end.
CC Junio C Hamano <[email protected]>
Signed-off-by Antonin Delpeuch <[email protected]>
---

This is gitgitgadget but git send-email would probably not pick up the CC here since it is not part of the trailer section (because there is a blank line between the CC line and the signed-off-by line). Maybe gitgitgadget works the same way. Also the space between CC: and the value is a no-break space but I don’t know if that matters.[1]

(Also see [2] for what it’s worth.)

† 1: On Linux at least this often happens by accident by pressing AltGr+Space. 🔗 2: https://lore.kernel.org/git/[email protected]/

Signature · 1 line

Kristoffer Haugsbakk

Antonin Delpeuch

2024-01-18 15:43 UTC #2 lore raw

Quoted 5 lines · Kristoffer Haugsbakk

This is gitgitgadget but git send-email would probably not pick up the CC here since it is not part of the trailer section (because there is a blank line between the CC line and the signed-off-by line). Maybe gitgitgadget works the same way. Also the space between CC: and the value is a no-break space but I don’t know if that matters.[1]

Thanks a lot for catching that! Let me fix and re-submit.

Antonin

Antonin Delpeuch via GitGitGadget

author

2024-01-18 15:43 UTC #3 lore raw

PATCH v2 1/1 merge-ll: expose revision names to custom drivers

In reply to Antonin Delpeuch via GitGitGadget

From: Antonin Delpeuch <[email protected]>

Signed-off-by Antonin Delpeuch <[email protected]>

---
merge-ll: expose revision names to custom drivers

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1648%2Fwetneb%2Fmerge_driver_pathnames-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1648/wetneb/merge_driver_pathnames-v2
Pull-Request: https://github.com/git/git/pull/1648

Range-diff vs v1:

1: e648f9f0696 ! 1: 6dec70529c0 merge-ll: expose revision names to custom drivers
@@ Commit message
can refer to those revisions. The placeholders '%S', '%X' and '%Y' are introduced to this end.

- CC: Junio C Hamano <[email protected]>
-

Signed-off-by Antonin Delpeuch <[email protected]>

## Documentation/gitattributes.txt ##

 Documentation/gitattributes.txt | 10 +++++++---
 merge-ll.c                      | 17 ++++++++++++++---
 t/t6406-merge-attr.sh           | 16 +++++++++++-----
 3 files changed, 32 insertions(+), 11 deletions(-)

Documentation/gitattributes.txt +7 -3

index 201bdf5edbd..108c2e23baa 100644
@@ -1141,7 +1141,7 @@ command to run to merge ancestor's version (`%O`), current
 version (`%A`) and the other branches' version (`%B`).  These
 three tokens are replaced with the names of temporary files that
 hold the contents of these versions when the command line is
-built. Additionally, %L will be replaced with the conflict marker
+built. Additionally, `%L` will be replaced with the conflict marker
 size (see below).
 
 The merge driver is expected to leave the result of the merge in

@@ -1159,8 +1159,12 @@ When left unspecified, the driver itself is used for both
 internal merge and the final merge.
 
 The merge driver can learn the pathname in which the merged result
-will be stored via placeholder `%P`.
-
+will be stored via placeholder `%P`. Additionally, the names of the
+merge ancestor revision (`%S`), of the current revision (`%X`) and
+of the other branch (`%Y`) can also be supplied. Those are short
+revision names, optionally joined with the paths of the file in each
+revision. Those paths are only present if they differ and are separated
+from the revision by a colon.
 
 `conflict-marker-size`
 ^^^^^^^^^^^^^^^^^^^^^^

merge-ll.c +14 -3

index 1df58ebaac0..ae9eb69be14 100644
@@ -185,9 +185,9 @@ static void create_temp(mmfile_t *src, char *path, size_t len)
 static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 			mmbuffer_t *result,
 			const char *path,
-			mmfile_t *orig, const char *orig_name UNUSED,
-			mmfile_t *src1, const char *name1 UNUSED,
-			mmfile_t *src2, const char *name2 UNUSED,
+			mmfile_t *orig, const char *orig_name,
+			mmfile_t *src1, const char *name1,
+			mmfile_t *src2, const char *name2,
 			const struct ll_merge_options *opts,
 			int marker_size)
 {

@@ -222,6 +222,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 			strbuf_addf(&cmd, "%d", marker_size);
 		else if (skip_prefix(format, "P", &format))
 			sq_quote_buf(&cmd, path);
+		else if (skip_prefix(format, "S", &format))
+		    sq_quote_buf(&cmd, orig_name);
+		else if (skip_prefix(format, "X", &format))
+			sq_quote_buf(&cmd, name1);
+		else if (skip_prefix(format, "Y", &format))
+			sq_quote_buf(&cmd, name2);
 		else
 			strbuf_addch(&cmd, '%');
 	}

@@ -315,7 +321,12 @@ static int read_merge_config(const char *var, const char *value,
 		 *    %B - temporary file name for the other branches' version.
 		 *    %L - conflict marker length
 		 *    %P - the original path (safely quoted for the shell)
+		 *    %S - the revision for the merge base
+		 *    %X - the revision for our version
+		 *    %Y - the revision for their version
 		 *
+		 * If the file is not named indentically in all versions, then each
+		 * revision is joined with the corresponding path, separated by a colon.
 		 * The external merge driver should write the results in the
 		 * file named by %A, and signal that it has done with zero exit
 		 * status.

t/t6406-merge-attr.sh +11 -5

index 72f8c1722ff..156a1efacfe 100755
@@ -42,11 +42,15 @@ test_expect_success setup '
 	#!/bin/sh
 
 	orig="$1" ours="$2" theirs="$3" exit="$4" path=$5
+	orig_name="$6" our_name="$7" their_name="$8"
 	(
 		echo "orig is $orig"
 		echo "ours is $ours"
 		echo "theirs is $theirs"
 		echo "path is $path"
+		echo "orig_name is $orig_name"
+		echo "our_name is $our_name"
+		echo "their_name is $their_name"
 		echo "=== orig ==="
 		cat "$orig"
 		echo "=== ours ==="

@@ -121,7 +125,7 @@ test_expect_success 'custom merge backend' '
 
 	git reset --hard anchor &&
 	git config --replace-all \
-	merge.custom.driver "./custom-merge %O %A %B 0 %P" &&
+	merge.custom.driver "./custom-merge %O %A %B 0 %P %S %X %Y" &&
 	git config --replace-all \
 	merge.custom.name "custom merge driver for testing" &&

@@ -132,7 +136,8 @@ test_expect_success 'custom merge backend' '
 	o=$(git unpack-file main^:text) &&
 	a=$(git unpack-file side^:text) &&
 	b=$(git unpack-file main:text) &&
-	sh -c "./custom-merge $o $a $b 0 text" &&
+	base_revid=$(git rev-parse --short main^) &&
+	sh -c "./custom-merge $o $a $b 0 text $base_revid HEAD main" &&
 	sed -e 1,3d $a >check-2 &&
 	cmp check-1 check-2 &&
 	rm -f $o $a $b

@@ -142,7 +147,7 @@ test_expect_success 'custom merge backend' '
 
 	git reset --hard anchor &&
 	git config --replace-all \
-	merge.custom.driver "./custom-merge %O %A %B 1 %P" &&
+	merge.custom.driver "./custom-merge %O %A %B 1 %P %S %X %Y" &&
 	git config --replace-all \
 	merge.custom.name "custom merge driver for testing" &&

@@ -159,7 +164,8 @@ test_expect_success 'custom merge backend' '
 	o=$(git unpack-file main^:text) &&
 	a=$(git unpack-file anchor:text) &&
 	b=$(git unpack-file main:text) &&
-	sh -c "./custom-merge $o $a $b 0 text" &&
+	base_revid=$(git rev-parse --short main^) &&
+	sh -c "./custom-merge $o $a $b 0 text $base_revid HEAD main" &&
 	sed -e 1,3d $a >check-2 &&
 	cmp check-1 check-2 &&
 	sed -e 1,3d -e 4q $a >check-3 &&

@@ -173,7 +179,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'
 
 	git reset --hard anchor &&
 	git config --replace-all \
-	merge.custom.driver "./custom-merge %O %A %B 0 %P" &&
+	merge.custom.driver "./custom-merge %O %A %B 0 %P %S %X %Y" &&
 	git config --replace-all \
 	merge.custom.name "custom merge driver for testing" &&

base-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5

Signature · 1 line

gitgitgadget

Junio C Hamano

2024-01-18 20:16 UTC #4 lore raw

Quoted 21 lines · Antonin Delpeuch via GitGitGadget

From: Antonin Delpeuch <[email protected]>
Custom merge drivers need access to the names of the revisions they are working on, so that the merge conflict markers they introduce can refer to those revisions. The placeholders '%S', '%X' and '%Y' are introduced to this end.
Signed-off-by Antonin Delpeuch <[email protected]>
---
merge-ll: expose revision names to custom drivers
 Documentation/gitattributes.txt | 10 +++++++---
 merge-ll.c                      | 17 ++++++++++++++---
 t/t6406-merge-attr.sh           | 16 +++++++++++-----
 3 files changed, 32 insertions(+), 11 deletions(-)
Documentation/gitattributes.txt +0 -0
index 201bdf5edbd..108c2e23baa 100644

Documentation/gitattributes.txt command to run to merge ancestor's version (`%O`), current · line 1145

Quoted 6 lines

Documentation/gitattributes.txt +1 -1

@@ -1141,7 +1141,7 @@ command to run to merge ancestor's version (`%O`), current
 version (`%A`) and the other branches' version (`%B`).  These
 three tokens are replaced with the names of temporary files that
 hold the contents of these versions when the command line is
-built. Additionally, %L will be replaced with the conflict marker
+built. Additionally, `%L` will be replaced with the conflict marker
 size (see below).

Good eyes. Nice to fix it while we are in the vicinity.

Documentation/gitattributes.txt When left unspecified, the driver itself is used for both · line 1163

Quoted 8 lines

Documentation/gitattributes.txt +2 -2

@@ -1159,8 +1159,12 @@ When left unspecified, the driver itself is used for both
 internal merge and the final merge.
 
 The merge driver can learn the pathname in which the merged result
-will be stored via placeholder `%P`.
-
+will be stored via placeholder `%P`. Additionally, the names of the
+merge ancestor revision (`%S`), of the current revision (`%X`) and

The phrase already existsin the description of '%O', but the correct word for that is "the common ancestor", not "the merge ancestor". At least this one is consistent with the existing error, so we can fix them together in a clean-up patch after the dust settles.

Or you could fix %O's description "while at it" and use the right term from the get-go for %S.

Quoted 20 lines

merge-ll.c +3 -3

index 1df58ebaac0..ae9eb69be14 100644
@@ -185,9 +185,9 @@ static void create_temp(mmfile_t *src, char *path, size_t len)
 static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 			mmbuffer_t *result,
 			const char *path,
-			mmfile_t *orig, const char *orig_name UNUSED,
-			mmfile_t *src1, const char *name1 UNUSED,
-			mmfile_t *src2, const char *name2 UNUSED,
+			mmfile_t *orig, const char *orig_name,
+			mmfile_t *src1, const char *name1,
+			mmfile_t *src2, const char *name2,
 			const struct ll_merge_options *opts,
 			int marker_size)
 {

@@ -222,6 +222,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 			strbuf_addf(&cmd, "%d", marker_size);
 		else if (skip_prefix(format, "P", &format))

merge-ll.c static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, · line 233

Quoted 10 lines

merge-ll.c +6 -0

 			sq_quote_buf(&cmd, path);
+		else if (skip_prefix(format, "S", &format))
+		    sq_quote_buf(&cmd, orig_name);
+		else if (skip_prefix(format, "X", &format))
+			sq_quote_buf(&cmd, name1);
+		else if (skip_prefix(format, "Y", &format))
+			sq_quote_buf(&cmd, name2);
 		else
 			strbuf_addch(&cmd, '%');
 	}

I see some funny indentation for "S" here.

Quoted 30 lines

t/t6406-merge-attr.sh +5 -1

index 72f8c1722ff..156a1efacfe 100755
@@ -42,11 +42,15 @@ test_expect_success setup '
 	#!/bin/sh
 
 	orig="$1" ours="$2" theirs="$3" exit="$4" path=$5
+	orig_name="$6" our_name="$7" their_name="$8"
 	(
 		echo "orig is $orig"
 		echo "ours is $ours"
 		echo "theirs is $theirs"
 		echo "path is $path"
+		echo "orig_name is $orig_name"
+		echo "our_name is $our_name"
+		echo "their_name is $their_name"
 		echo "=== orig ==="
 		cat "$orig"
 		echo "=== ours ==="

@@ -121,7 +125,7 @@ test_expect_success 'custom merge backend' '
 
 	git reset --hard anchor &&
 	git config --replace-all \
-	merge.custom.driver "./custom-merge %O %A %B 0 %P" &&
+	merge.custom.driver "./custom-merge %O %A %B 0 %P %S %X %Y" &&
 	git config --replace-all \
 	merge.custom.name "custom merge driver for testing" &&

t/t6406-merge-attr.sh test_expect_success 'custom merge backend' ' · line 143

Quoted 9 lines

t/t6406-merge-attr.sh +2 -1

@@ -132,7 +136,8 @@ test_expect_success 'custom merge backend' '
 	o=$(git unpack-file main^:text) &&
 	a=$(git unpack-file side^:text) &&
 	b=$(git unpack-file main:text) &&
-	sh -c "./custom-merge $o $a $b 0 text" &&
+	base_revid=$(git rev-parse --short main^) &&
+	sh -c "./custom-merge $o $a $b 0 text $base_revid HEAD main" &&
 	sed -e 1,3d $a >check-2 &&
 	cmp check-1 check-2 &&
 	rm -f $o $a $b

OK.

Quoted 10 lines

t/t6406-merge-attr.sh +1 -1

@@ -142,7 +147,7 @@ test_expect_success 'custom merge backend' '
 
 	git reset --hard anchor &&
 	git config --replace-all \
-	merge.custom.driver "./custom-merge %O %A %B 1 %P" &&
+	merge.custom.driver "./custom-merge %O %A %B 1 %P %S %X %Y" &&
 	git config --replace-all \
 	merge.custom.name "custom merge driver for testing" &&

t/t6406-merge-attr.sh test_expect_success 'custom merge backend' ' · line 171

Quoted 9 lines

t/t6406-merge-attr.sh +2 -1

@@ -159,7 +164,8 @@ test_expect_success 'custom merge backend' '
 	o=$(git unpack-file main^:text) &&
 	a=$(git unpack-file anchor:text) &&
 	b=$(git unpack-file main:text) &&
-	sh -c "./custom-merge $o $a $b 0 text" &&
+	base_revid=$(git rev-parse --short main^) &&
+	sh -c "./custom-merge $o $a $b 0 text $base_revid HEAD main" &&
 	sed -e 1,3d $a >check-2 &&
 	cmp check-1 check-2 &&
 	sed -e 1,3d -e 4q $a >check-3 &&

OK.

t/t6406-merge-attr.sh test_expect_success !WINDOWS 'custom merge driver that is killed with a signal' · line 184

Quoted 8 lines

t/t6406-merge-attr.sh +1 -1

@@ -173,7 +179,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'
 
 	git reset --hard anchor &&
 	git config --replace-all \
-	merge.custom.driver "./custom-merge %O %A %B 0 %P" &&
+	merge.custom.driver "./custom-merge %O %A %B 0 %P %S %X %Y" &&
 	git config --replace-all \
 	merge.custom.name "custom merge driver for testing" &&

;-)

This one is expected to die and not produce meaningful output; I was wondering why this does not need to make corresponding changes to the expected output pattern like the earlier tests.

Antonin Delpeuch

2024-01-18 21:09 UTC #5 lore raw

Hi Junio,

Thanks a lot for your review! (and many apologies for the double sending as HTML…)

Quoted 2 lines · Junio C Hamano

Or you could fix %O's description "while at it" and use the right term from the get-go for %S.

Agreed, I'll do that, it also feels more fitting to me.

Quoted 1 line

I see some funny indentation for "S" here.

Oops, sorry about that.

Quoted 10 lines

t/t6406-merge-attr.sh test_expect_success !WINDOWS 'custom merge driver that is killed with a signal' · line 184

Quoted 8 lines

t/t6406-merge-attr.sh +1 -1

@@ -173,7 +179,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'
 
  	git reset --hard anchor &&
  	git config --replace-all \
-	merge.custom.driver "./custom-merge %O %A %B 0 %P" &&
+	merge.custom.driver "./custom-merge %O %A %B 0 %P %S %X %Y" &&
  	git config --replace-all \
  	merge.custom.name "custom merge driver for testing" &&

;-)

Quoted 3 lines

This one is expected to die and not produce meaningful output; I was wondering why this does not need to make corresponding changes to the expected output pattern like the earlier tests.

As far as I can tell, this test does not compare the result of the merge to the expected merge driver output, because that output is expected to be disregarded by git given that the merge driver died (see the last two lines). So it seems normal to me that we don't need to adapt the expected output: the repository files are still left unchanged.

I'll submit a new version of the patch with the two changes above.

Antonin

Antonin Delpeuch via GitGitGadget

author

2024-01-18 22:09 UTC #6 lore raw

PATCH v3 1/1 merge-ll: expose revision names to custom drivers

In reply to Antonin Delpeuch via GitGitGadget

From: Antonin Delpeuch <[email protected]>

Signed-off-by Antonin Delpeuch <[email protected]>

---
merge-ll: expose revision names to custom drivers

Changes since v2:

* change the documentation to use "common ancestor" rather than "merge ancestor"
* fix indentation issue

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1648%2Fwetneb%2Fmerge_driver_pathnames-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1648/wetneb/merge_driver_pathnames-v3
Pull-Request: https://github.com/git/git/pull/1648

Range-diff vs v2:

1: 6dec70529c0 ! 1: aebd26711fe merge-ll: expose revision names to custom drivers
@@ Commit message

Signed-off-by Antonin Delpeuch <[email protected]>

## Documentation/gitattributes.txt ##
-@@ Documentation/gitattributes.txt: command to run to merge ancestor's version (%O), current +@@ Documentation/gitattributes.txt: The merge.*.name variable gives the driver a human-readable
+ name.
+
+ The merge.*.driver variable's value is used to construct a
+-command to run to merge ancestor's version (%O), current
++command to run to common ancestor's version (%O), current
version (%A) and the other branches' version (%B). These
three tokens are replaced with the names of temporary files that
hold the contents of these versions when the command line is
@@ Documentation/gitattributes.txt: When left unspecified, the driver itself is use
-will be stored via placeholder %P.
-
+will be stored via placeholder %P. Additionally, the names of the
-+merge ancestor revision (%S), of the current revision (%X) and
++common ancestor revision (%S), of the current revision (%X) and
+of the other branch (%Y) can also be supplied. Those are short
+revision names, optionally joined with the paths of the file in each
+revision. Those paths are only present if they differ and are separated
@@ merge-ll.c: static enum ll_merge_result ll_ext_merge(const struct ll_merge_drive
else if (skip_prefix(format, "P", &format))
sq_quote_buf(&cmd, path);
+ else if (skip_prefix(format, "S", &format))
-+ sq_quote_buf(&cmd, orig_name);
++ sq_quote_buf(&cmd, orig_name);
+ else if (skip_prefix(format, "X", &format))
+ sq_quote_buf(&cmd, name1);
+ else if (skip_prefix(format, "Y", &format))

 Documentation/gitattributes.txt | 12 ++++++++----
 merge-ll.c                      | 17 ++++++++++++++---
 t/t6406-merge-attr.sh           | 16 +++++++++++-----
 3 files changed, 33 insertions(+), 12 deletions(-)

Documentation/gitattributes.txt +8 -4

index 201bdf5edbd..86a0946bb9e 100644
@@ -1137,11 +1137,11 @@ The `merge.*.name` variable gives the driver a human-readable
 name.
 
 The `merge.*.driver` variable's value is used to construct a
-command to run to merge ancestor's version (`%O`), current
+command to run to common ancestor's version (`%O`), current
 version (`%A`) and the other branches' version (`%B`).  These
 three tokens are replaced with the names of temporary files that
 hold the contents of these versions when the command line is
-built. Additionally, %L will be replaced with the conflict marker
+built. Additionally, `%L` will be replaced with the conflict marker
 size (see below).
 
 The merge driver is expected to leave the result of the merge in

@@ -1159,8 +1159,12 @@ When left unspecified, the driver itself is used for both
 internal merge and the final merge.
 
 The merge driver can learn the pathname in which the merged result
-will be stored via placeholder `%P`.
-
+will be stored via placeholder `%P`. Additionally, the names of the
+common ancestor revision (`%S`), of the current revision (`%X`) and
+of the other branch (`%Y`) can also be supplied. Those are short
+revision names, optionally joined with the paths of the file in each
+revision. Those paths are only present if they differ and are separated
+from the revision by a colon.
 
 `conflict-marker-size`
 ^^^^^^^^^^^^^^^^^^^^^^

merge-ll.c +14 -3

index 1df58ebaac0..13e0713fe82 100644
@@ -185,9 +185,9 @@ static void create_temp(mmfile_t *src, char *path, size_t len)
 static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 			mmbuffer_t *result,
 			const char *path,
-			mmfile_t *orig, const char *orig_name UNUSED,
-			mmfile_t *src1, const char *name1 UNUSED,
-			mmfile_t *src2, const char *name2 UNUSED,
+			mmfile_t *orig, const char *orig_name,
+			mmfile_t *src1, const char *name1,
+			mmfile_t *src2, const char *name2,
 			const struct ll_merge_options *opts,
 			int marker_size)
 {

@@ -222,6 +222,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 			strbuf_addf(&cmd, "%d", marker_size);
 		else if (skip_prefix(format, "P", &format))
 			sq_quote_buf(&cmd, path);
+		else if (skip_prefix(format, "S", &format))
+			sq_quote_buf(&cmd, orig_name);
+		else if (skip_prefix(format, "X", &format))
+			sq_quote_buf(&cmd, name1);
+		else if (skip_prefix(format, "Y", &format))
+			sq_quote_buf(&cmd, name2);
 		else
 			strbuf_addch(&cmd, '%');
 	}

@@ -315,7 +321,12 @@ static int read_merge_config(const char *var, const char *value,
 		 *    %B - temporary file name for the other branches' version.
 		 *    %L - conflict marker length
 		 *    %P - the original path (safely quoted for the shell)
+		 *    %S - the revision for the merge base
+		 *    %X - the revision for our version
+		 *    %Y - the revision for their version
 		 *
+		 * If the file is not named indentically in all versions, then each
+		 * revision is joined with the corresponding path, separated by a colon.
 		 * The external merge driver should write the results in the
 		 * file named by %A, and signal that it has done with zero exit
 		 * status.

t/t6406-merge-attr.sh +11 -5

index 72f8c1722ff..156a1efacfe 100755
@@ -42,11 +42,15 @@ test_expect_success setup '
 	#!/bin/sh
 
 	orig="$1" ours="$2" theirs="$3" exit="$4" path=$5
+	orig_name="$6" our_name="$7" their_name="$8"
 	(
 		echo "orig is $orig"
 		echo "ours is $ours"
 		echo "theirs is $theirs"
 		echo "path is $path"
+		echo "orig_name is $orig_name"
+		echo "our_name is $our_name"
+		echo "their_name is $their_name"
 		echo "=== orig ==="
 		cat "$orig"
 		echo "=== ours ==="

@@ -121,7 +125,7 @@ test_expect_success 'custom merge backend' '
 
 	git reset --hard anchor &&
 	git config --replace-all \
-	merge.custom.driver "./custom-merge %O %A %B 0 %P" &&
+	merge.custom.driver "./custom-merge %O %A %B 0 %P %S %X %Y" &&
 	git config --replace-all \
 	merge.custom.name "custom merge driver for testing" &&

@@ -132,7 +136,8 @@ test_expect_success 'custom merge backend' '
 	o=$(git unpack-file main^:text) &&
 	a=$(git unpack-file side^:text) &&
 	b=$(git unpack-file main:text) &&
-	sh -c "./custom-merge $o $a $b 0 text" &&
+	base_revid=$(git rev-parse --short main^) &&
+	sh -c "./custom-merge $o $a $b 0 text $base_revid HEAD main" &&
 	sed -e 1,3d $a >check-2 &&
 	cmp check-1 check-2 &&
 	rm -f $o $a $b

@@ -142,7 +147,7 @@ test_expect_success 'custom merge backend' '
 
 	git reset --hard anchor &&
 	git config --replace-all \
-	merge.custom.driver "./custom-merge %O %A %B 1 %P" &&
+	merge.custom.driver "./custom-merge %O %A %B 1 %P %S %X %Y" &&
 	git config --replace-all \
 	merge.custom.name "custom merge driver for testing" &&

@@ -159,7 +164,8 @@ test_expect_success 'custom merge backend' '
 	o=$(git unpack-file main^:text) &&
 	a=$(git unpack-file anchor:text) &&
 	b=$(git unpack-file main:text) &&
-	sh -c "./custom-merge $o $a $b 0 text" &&
+	base_revid=$(git rev-parse --short main^) &&
+	sh -c "./custom-merge $o $a $b 0 text $base_revid HEAD main" &&
 	sed -e 1,3d $a >check-2 &&
 	cmp check-1 check-2 &&
 	sed -e 1,3d -e 4q $a >check-3 &&

@@ -173,7 +179,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'
 
 	git reset --hard anchor &&
 	git config --replace-all \
-	merge.custom.driver "./custom-merge %O %A %B 0 %P" &&
+	merge.custom.driver "./custom-merge %O %A %B 0 %P %S %X %Y" &&
 	git config --replace-all \
 	merge.custom.name "custom merge driver for testing" &&

base-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5

Signature · 1 line

gitgitgadget

Antonin Delpeuch

2024-01-19 20:02 UTC #7 lore raw

Hi Junio,

After more testing (combining custom merge drivers with rerere) I realized that my patch can lead to a segmentation error. Many apologies for not having caught that earlier!

merge-ll.c static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, · line 230

Quoted 10 lines · Antonin Delpeuch via GitGitGadget

merge-ll.c +6 -0

@@ -222,6 +222,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
  			strbuf_addf(&cmd, "%d", marker_size);
  		else if (skip_prefix(format, "P", &format))
  			sq_quote_buf(&cmd, path);
+		else if (skip_prefix(format, "S", &format))
+			sq_quote_buf(&cmd, orig_name);
+		else if (skip_prefix(format, "X", &format))
+			sq_quote_buf(&cmd, name1);
+		else if (skip_prefix(format, "Y", &format))
+			sq_quote_buf(&cmd, name2);

The "orig_name", "name1" and "name2" pointers can be NULL at this stage. This can happen when the merge is invoked from rerere, to resolve a conflict using a previous resolution.

I wonder what the appropriate fallback would be in such a case. I am tempted to use the temporary filenames of the files to merge instead, so that the merge driver can rely on those names being non-empty and being the best string to use to identify the files. Passing an empty string seems dangerous to me, as it is likely to change the index of arguments passed to the merge driver. Passing fixed strings such as "base", "ours" and "theirs" could perhaps work too.

Let me know if you have any preference about this.

Best,

Antonin

Phillip Wood

2024-01-20 14:13 UTC #9 lore raw

In reply to Antonin Delpeuch via GitGitGadget

Hi Antonin

Quoted 6 lines · Antonin Delpeuch via GitGitGadget

From: Antonin Delpeuch <[email protected]>
Custom merge drivers need access to the names of the revisions they are working on, so that the merge conflict markers they introduce can refer to those revisions. The placeholders '%S', '%X' and '%Y' are introduced to this end.

Thanks for working on this, I think it is a useful improvement. I guess '%X' and '%Y' are no worse than the existing '%A' and '%B' but I do wonder if we want to take the opportunity to switch to more descriptive names for the various parameters passed to the custom merge strategy. We do do this by supporting %(label:ours) modeled after the format specifiers used by other commands such as "git log" and "git for-each-ref".

Documentation/gitattributes.txt When left unspecified, the driver itself is used for both · line 1167

Quoted 6 lines

[...]

Documentation/gitattributes.txt +5 -0

+will be stored via placeholder `%P`. Additionally, the names of the
+common ancestor revision (`%S`), of the current revision (`%X`) and
+of the other branch (`%Y`) can also be supplied. Those are short > +revision names, optionally joined with the paths of the file in each
+revision. Those paths are only present if they differ and are separated
+from the revision by a colon.

It might be simpler to just call these the "conflict marker labels" without tying ourselves to a particular format. Something like

The conflict labels to be used for the common ancestor, local head and other head can be passed by using '%(label:base)', '%(label:ours)' and '%(label:theirs) respectively.

Review static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,

Quoted 1 line

(quoted diff) +0 -0

@@ -222,6 +222,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,

Not part of this patch but I noticed that we're passing the filenames for '%A' etc. unquoted which is a bit scary.

merge-ll.c static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, · line 226

Quoted 5 lines

merge-ll.c +2 -0

  			strbuf_addf(&cmd, "%d", marker_size);
  		else if (skip_prefix(format, "P", &format))
  			sq_quote_buf(&cmd, path);
+		else if (skip_prefix(format, "S", &format))
+			sq_quote_buf(&cmd, orig_name);

I think you can avoid the SIGSEV problem you mentioned in your other email by changing this to

	sq_quote_buf(&cmd, orig_name ? orig_name, "");

That would make sure the labels we pass match the ones used by the internal merge.

Best Wishes

Phillip

Junio C Hamano

2024-01-20 17:25 UTC #8 lore raw

In reply to Antonin Delpeuch

Quoted 3 lines · Antonin Delpeuch

After more testing (combining custom merge drivers with rerere) I realized that my patch can lead to a segmentation error. Many apologies for not having caught that earlier!

Ah, understandable. The 3-way merge machinery may not even have to work on commit objects (it can merge two trees, using another tree as the "common ancestor" tree, just fine).

And in such a case, it is perfectly possible there is no "human readable name"; all there is may be a tree object name.

Quoted 7 lines · Antonin Delpeuch via GitGitGadget

Quoted 6 lines

merge-ll.c +2 -0

@@ -222,6 +222,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
  			strbuf_addf(&cmd, "%d", marker_size);
  		else if (skip_prefix(format, "P", &format))
  			sq_quote_buf(&cmd, path);
+		else if (skip_prefix(format, "S", &format))
+			sq_quote_buf(&cmd, orig_name);

Quoted 8 lines

merge-ll.c static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, · line 230
Quoted 4 lines
merge-ll.c +4 -0
+		else if (skip_prefix(format, "X", &format))
+			sq_quote_buf(&cmd, name1);
+		else if (skip_prefix(format, "Y", &format))
+			sq_quote_buf(&cmd, name2);
The "orig_name", "name1" and "name2" pointers can be NULL at this stage. This can happen when the merge is invoked from rerere, to resolve a conflict using a previous resolution.

	sq_quote_buf(&cmd, name1 ? name1 : "(ours)");

or something like that, perhaps.

Junio C Hamano

2024-01-20 17:37 UTC #10 lore raw

In reply to Phillip Wood

Quoted 2 lines · Phillip Wood

Not part of this patch but I noticed that we're passing the filenames for '%A' etc. unquoted which is a bit scary.

May be scary but safe, as long as create_temp() gives a reasonable temporary filename. We pass ".merge_file_XXXXXX" to xmkstemp(), which calls into mkstemp(), which should give us a shell safe name?

It also should be a safe conversion to change strbuf_addstr() used for these three to sq_quote_buf(), as the string with these %[OAB] placeholders are passed to the shell that eats the quoting before invoking the end-user supplied external merge driver, which means the merge driver would not notice any difference.

Thanks for being careful ;-)

Phillip Wood

2024-01-20 18:23 UTC #11 lore raw

Hi Junio

Quoted 8 lines · Junio C Hamano

Quoted 2 lines · Phillip Wood
Not part of this patch but I noticed that we're passing the filenames for '%A' etc. unquoted which is a bit scary.
May be scary but safe, as long as create_temp() gives a reasonable temporary filename. We pass ".merge_file_XXXXXX" to xmkstemp(), which calls into mkstemp(), which should give us a shell safe name?

Yes. I'd mis-read create_temp() and thought we were appending ".merge_file_XXXXX" to the path being merged but looking at it again it is safe.

Quoted 5 lines

It also should be a safe conversion to change strbuf_addstr() used for these three to sq_quote_buf(), as the string with these %[OAB] placeholders are passed to the shell that eats the quoting before invoking the end-user supplied external merge driver, which means the merge driver would not notice any difference.

I agree that would be a safe conversion , but I'm not sure it is worth it.

Best Wishes

Phillip

Junio C Hamano

2024-01-20 22:49 UTC #12 lore raw

In reply to Phillip Wood

Quoted 7 lines · Phillip Wood

Thanks for working on this, I think it is a useful improvement. I guess '%X' and '%Y' are no worse than the existing '%A' and '%B' but I do wonder if we want to take the opportunity to switch to more descriptive names for the various parameters passed to the custom merge strategy. We do do this by supporting %(label:ours) modeled after the format specifiers used by other commands such as "git log" and "git for-each-ref".

Perhaps. Unlike the --format option these commands take, the placeholders are never typed from the command line (they always are taken from the configuration file), so mnemonic value longer version gives over the current single-letter ones is not as valuable, while making the total line length longer. So I dunno.

Quoted 10 lines

Documentation/gitattributes.txt When left unspecified, the driver itself is used for both · line 1167
Quoted 6 lines
[...]
Documentation/gitattributes.txt +5 -0
+will be stored via placeholder `%P`. Additionally, the names of the
+common ancestor revision (`%S`), of the current revision (`%X`) and
+of the other branch (`%Y`) can also be supplied. Those are short > +revision names, optionally joined with the paths of the file in each
+revision. Those paths are only present if they differ and are separated
+from the revision by a colon.
It might be simpler to just call these the "conflict marker labels" without tying ourselves to a particular format. Something like

Quoted 3 lines

The conflict labels to be used for the common ancestor, local head and other head can be passed by using '%(label:base)', '%(label:ours)' and '%(label:theirs) respectively.

Yeah, that sounds like a good improvement, even if we did not use the longhand placeholders and replaced %(label:{base,ours,theirs}) with %S, %X, and %Y.

Quoted 14 lines

Review static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
Quoted 1 line
(quoted diff) +0 -0
@@ -222,6 +222,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
Not part of this patch but I noticed that we're passing the filenames for '%A' etc. unquoted which is a bit scary.
merge-ll.c static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, · line 226
Quoted 5 lines
merge-ll.c +2 -0
  			strbuf_addf(&cmd, "%d", marker_size);
  		else if (skip_prefix(format, "P", &format))
  			sq_quote_buf(&cmd, path);
+		else if (skip_prefix(format, "S", &format))
+			sq_quote_buf(&cmd, orig_name);
I think you can avoid the SIGSEV problem you mentioned in your other email by changing this to

Quoted 4 lines

	sq_quote_buf(&cmd, orig_name ? orig_name, "");
That would make sure the labels we pass match the ones used by the internal merge.

Makes sense. That would be much better than using hardcoded string "ours", "theirs", etc.

Antonin Delpeuch via GitGitGadget

author

2024-01-24 20:09 UTC #13 lore raw

PATCH v4 1/1 merge-ll: expose revision names to custom drivers

In reply to Antonin Delpeuch via GitGitGadget

From: Antonin Delpeuch <[email protected]>

Signed-off-by Antonin Delpeuch <[email protected]>

---
merge-ll: expose revision names to custom drivers

Changes since v3:

* simplify the documentation as suggested by Phillip, keeping single-letter placeholders as suggested by Junio
* add fallback to empty string when the collision marker names are NULL, as suggested by Phillip

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1648%2Fwetneb%2Fmerge_driver_pathnames-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1648/wetneb/merge_driver_pathnames-v4
Pull-Request: https://github.com/git/git/pull/1648

Range-diff vs v3:

1: aebd26711fe ! 1: 47aca4fc8f6 merge-ll: expose revision names to custom drivers

     @@ Documentation/gitattributes.txt: When left unspecified, the driver itself is use
       The merge driver can learn the pathname in which the merged result
      -will be stored via placeholder `%P`.
      -
     -+will be stored via placeholder `%P`. Additionally, the names of the
     -+common ancestor revision (`%S`), of the current revision (`%X`) and
     -+of the other branch (`%Y`) can also be supplied. Those are short
     -+revision names, optionally joined with the paths of the file in each
     -+revision. Those paths are only present if they differ and are separated
     -+from the revision by a colon.
     ++will be stored via placeholder `%P`. The conflict labels to be used
     ++for the common ancestor, local head and other head can be passed by
     ++using '%S', '%X' and '%Y` respectively.
       
       `conflict-marker-size`
       ^^^^^^^^^^^^^^^^^^^^^^
     @@ merge-ll.c: static enum ll_merge_result ll_ext_merge(const struct ll_merge_drive
       		else if (skip_prefix(format, "P", &format))
       			sq_quote_buf(&cmd, path);
      +		else if (skip_prefix(format, "S", &format))
     -+			sq_quote_buf(&cmd, orig_name);
     ++			sq_quote_buf(&cmd, orig_name ? orig_name : "");
      +		else if (skip_prefix(format, "X", &format))
     -+			sq_quote_buf(&cmd, name1);
     ++			sq_quote_buf(&cmd, name1 ? name1 : "");
      +		else if (skip_prefix(format, "Y", &format))
     -+			sq_quote_buf(&cmd, name2);
     ++			sq_quote_buf(&cmd, name2 ? name2 : "");
       		else
       			strbuf_addch(&cmd, '%');
       	}

 Documentation/gitattributes.txt |  9 +++++----
 merge-ll.c                      | 17 ++++++++++++++---
 t/t6406-merge-attr.sh           | 16 +++++++++++-----
 3 files changed, 30 insertions(+), 12 deletions(-)

Documentation/gitattributes.txt +5 -4

index 201bdf5edbd..4338d023d9a 100644
@@ -1137,11 +1137,11 @@ The `merge.*.name` variable gives the driver a human-readable
 name.
 
 The `merge.*.driver` variable's value is used to construct a
-command to run to merge ancestor's version (`%O`), current
+command to run to common ancestor's version (`%O`), current
 version (`%A`) and the other branches' version (`%B`).  These
 three tokens are replaced with the names of temporary files that
 hold the contents of these versions when the command line is
-built. Additionally, %L will be replaced with the conflict marker
+built. Additionally, `%L` will be replaced with the conflict marker
 size (see below).
 
 The merge driver is expected to leave the result of the merge in

@@ -1159,8 +1159,9 @@ When left unspecified, the driver itself is used for both
 internal merge and the final merge.
 
 The merge driver can learn the pathname in which the merged result
-will be stored via placeholder `%P`.
-
+will be stored via placeholder `%P`. The conflict labels to be used
+for the common ancestor, local head and other head can be passed by
+using '%S', '%X' and '%Y` respectively.
 
 `conflict-marker-size`
 ^^^^^^^^^^^^^^^^^^^^^^

merge-ll.c +14 -3

index 1df58ebaac0..5ffb045efb9 100644
@@ -185,9 +185,9 @@ static void create_temp(mmfile_t *src, char *path, size_t len)
 static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 			mmbuffer_t *result,
 			const char *path,
-			mmfile_t *orig, const char *orig_name UNUSED,
-			mmfile_t *src1, const char *name1 UNUSED,
-			mmfile_t *src2, const char *name2 UNUSED,
+			mmfile_t *orig, const char *orig_name,
+			mmfile_t *src1, const char *name1,
+			mmfile_t *src2, const char *name2,
 			const struct ll_merge_options *opts,
 			int marker_size)
 {

@@ -222,6 +222,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 			strbuf_addf(&cmd, "%d", marker_size);
 		else if (skip_prefix(format, "P", &format))
 			sq_quote_buf(&cmd, path);
+		else if (skip_prefix(format, "S", &format))
+			sq_quote_buf(&cmd, orig_name ? orig_name : "");
+		else if (skip_prefix(format, "X", &format))
+			sq_quote_buf(&cmd, name1 ? name1 : "");
+		else if (skip_prefix(format, "Y", &format))
+			sq_quote_buf(&cmd, name2 ? name2 : "");
 		else
 			strbuf_addch(&cmd, '%');
 	}

@@ -315,7 +321,12 @@ static int read_merge_config(const char *var, const char *value,
 		 *    %B - temporary file name for the other branches' version.
 		 *    %L - conflict marker length
 		 *    %P - the original path (safely quoted for the shell)
+		 *    %S - the revision for the merge base
+		 *    %X - the revision for our version
+		 *    %Y - the revision for their version
 		 *
+		 * If the file is not named indentically in all versions, then each
+		 * revision is joined with the corresponding path, separated by a colon.
 		 * The external merge driver should write the results in the
 		 * file named by %A, and signal that it has done with zero exit
 		 * status.

t/t6406-merge-attr.sh +11 -5

index 72f8c1722ff..156a1efacfe 100755
@@ -42,11 +42,15 @@ test_expect_success setup '
 	#!/bin/sh
 
 	orig="$1" ours="$2" theirs="$3" exit="$4" path=$5
+	orig_name="$6" our_name="$7" their_name="$8"
 	(
 		echo "orig is $orig"
 		echo "ours is $ours"
 		echo "theirs is $theirs"
 		echo "path is $path"
+		echo "orig_name is $orig_name"
+		echo "our_name is $our_name"
+		echo "their_name is $their_name"
 		echo "=== orig ==="
 		cat "$orig"
 		echo "=== ours ==="

@@ -121,7 +125,7 @@ test_expect_success 'custom merge backend' '
 
 	git reset --hard anchor &&
 	git config --replace-all \
-	merge.custom.driver "./custom-merge %O %A %B 0 %P" &&
+	merge.custom.driver "./custom-merge %O %A %B 0 %P %S %X %Y" &&
 	git config --replace-all \
 	merge.custom.name "custom merge driver for testing" &&

@@ -132,7 +136,8 @@ test_expect_success 'custom merge backend' '
 	o=$(git unpack-file main^:text) &&
 	a=$(git unpack-file side^:text) &&
 	b=$(git unpack-file main:text) &&
-	sh -c "./custom-merge $o $a $b 0 text" &&
+	base_revid=$(git rev-parse --short main^) &&
+	sh -c "./custom-merge $o $a $b 0 text $base_revid HEAD main" &&
 	sed -e 1,3d $a >check-2 &&
 	cmp check-1 check-2 &&
 	rm -f $o $a $b

@@ -142,7 +147,7 @@ test_expect_success 'custom merge backend' '
 
 	git reset --hard anchor &&
 	git config --replace-all \
-	merge.custom.driver "./custom-merge %O %A %B 1 %P" &&
+	merge.custom.driver "./custom-merge %O %A %B 1 %P %S %X %Y" &&
 	git config --replace-all \
 	merge.custom.name "custom merge driver for testing" &&

@@ -159,7 +164,8 @@ test_expect_success 'custom merge backend' '
 	o=$(git unpack-file main^:text) &&
 	a=$(git unpack-file anchor:text) &&
 	b=$(git unpack-file main:text) &&
-	sh -c "./custom-merge $o $a $b 0 text" &&
+	base_revid=$(git rev-parse --short main^) &&
+	sh -c "./custom-merge $o $a $b 0 text $base_revid HEAD main" &&
 	sed -e 1,3d $a >check-2 &&
 	cmp check-1 check-2 &&
 	sed -e 1,3d -e 4q $a >check-3 &&

@@ -173,7 +179,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'
 
 	git reset --hard anchor &&
 	git config --replace-all \
-	merge.custom.driver "./custom-merge %O %A %B 0 %P" &&
+	merge.custom.driver "./custom-merge %O %A %B 0 %P %S %X %Y" &&
 	git config --replace-all \
 	merge.custom.name "custom merge driver for testing" &&

base-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5

Signature · 1 line

gitgitgadget

Junio C Hamano

2024-01-24 21:17 UTC #14 lore raw

Quoted 9 lines · Antonin Delpeuch via GitGitGadget

From: Antonin Delpeuch <[email protected]>
Custom merge drivers need access to the names of the revisions they are working on, so that the merge conflict markers they introduce can refer to those revisions. The placeholders '%S', '%X' and '%Y' are introduced to this end.
Signed-off-by Antonin Delpeuch <[email protected]>
---

This looks all good.

Let's declare a victory and merge it down to 'next' soonish.

Thanks for sticking to the topic.