{"thread":{"id":"60761","subject":"[PATCH] merge-ll: expose revision names to custom drivers","startedAt":"2024-01-18T14:26:17Z","lastAt":"2024-01-24T21:17:07Z","messageCount":15,"participants":["Antonin Delpeuch via GitGitGadget","Kristoffer Haugsbakk","Antonin Delpeuch","Junio C Hamano","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"486978","messageId":"pull.1648.git.git.1705587974840.gitgitgadget@gmail.com","threadId":"60761","inReplyTo":null,"subject":"[PATCH] merge-ll: expose revision names to custom drivers","fromName":"Antonin Delpeuch via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-18T14:26:14Z","receivedAt":"2024-01-18T14:26:17Z","isPatch":true,"sender":{"key":"antonin@delpeuch.eu","avatar":"https://avatars.githubusercontent.com/u/309908?v=4"},"body":"From: Antonin Delpeuch <antonin@delpeuch.eu>\n\nCustom merge drivers need access to the names of the revisions they\nare working on, so that the merge conflict markers they introduce\ncan refer to those revisions. The placeholders '%S', '%X' and '%Y'\nare introduced to this end.\n\nCC: Junio C Hamano <gitster@pobox.com>\n\nSigned-off-by: Antonin Delpeuch <antonin@delpeuch.eu>\n---\n    merge-ll: expose revision names to custom drivers\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1648%2Fwetneb%2Fmerge_driver_pathnames-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1648/wetneb/merge_driver_pathnames-v1\nPull-Request: https://github.com/git/git/pull/1648\n\n Documentation/gitattributes.txt | 10 +++++++---\n merge-ll.c                      | 17 ++++++++++++++---\n t/t6406-merge-attr.sh           | 16 +++++++++++-----\n 3 files changed, 32 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 201bdf5edbd..108c2e23baa 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -1141,7 +1141,7 @@ command to run to merge ancestor's version (`%O`), current\n version (`%A`) and the other branches' version (`%B`).  These\n three tokens are replaced with the names of temporary files that\n hold the contents of these versions when the command line is\n-built. Additionally, %L will be replaced with the conflict marker\n+built. Additionally, `%L` will be replaced with the conflict marker\n size (see below).\n \n The merge driver is expected to leave the result of the merge in\n@@ -1159,8 +1159,12 @@ When left unspecified, the driver itself is used for both\n internal merge and the final merge.\n \n The merge driver can learn the pathname in which the merged result\n-will be stored via placeholder `%P`.\n-\n+will be stored via placeholder `%P`. Additionally, the names of the\n+merge ancestor revision (`%S`), of the current revision (`%X`) and\n+of the other branch (`%Y`) can also be supplied. Those are short\n+revision names, optionally joined with the paths of the file in each\n+revision. Those paths are only present if they differ and are separated\n+from the revision by a colon.\n \n `conflict-marker-size`\n ^^^^^^^^^^^^^^^^^^^^^^\ndiff --git a/merge-ll.c b/merge-ll.c\nindex 1df58ebaac0..ae9eb69be14 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -185,9 +185,9 @@ static void create_temp(mmfile_t *src, char *path, size_t len)\n static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \t\t\tmmbuffer_t *result,\n \t\t\tconst char *path,\n-\t\t\tmmfile_t *orig, const char *orig_name UNUSED,\n-\t\t\tmmfile_t *src1, const char *name1 UNUSED,\n-\t\t\tmmfile_t *src2, const char *name2 UNUSED,\n+\t\t\tmmfile_t *orig, const char *orig_name,\n+\t\t\tmmfile_t *src1, const char *name1,\n+\t\t\tmmfile_t *src2, const char *name2,\n \t\t\tconst struct ll_merge_options *opts,\n \t\t\tint marker_size)\n {\n@@ -222,6 +222,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \t\t\tstrbuf_addf(&cmd, \"%d\", marker_size);\n \t\telse if (skip_prefix(format, \"P\", &format))\n \t\t\tsq_quote_buf(&cmd, path);\n+\t\telse if (skip_prefix(format, \"S\", &format))\n+\t\t    sq_quote_buf(&cmd, orig_name);\n+\t\telse if (skip_prefix(format, \"X\", &format))\n+\t\t\tsq_quote_buf(&cmd, name1);\n+\t\telse if (skip_prefix(format, \"Y\", &format))\n+\t\t\tsq_quote_buf(&cmd, name2);\n \t\telse\n \t\t\tstrbuf_addch(&cmd, '%');\n \t}\n@@ -315,7 +321,12 @@ static int read_merge_config(const char *var, const char *value,\n \t\t *    %B - temporary file name for the other branches' version.\n \t\t *    %L - conflict marker length\n \t\t *    %P - the original path (safely quoted for the shell)\n+\t\t *    %S - the revision for the merge base\n+\t\t *    %X - the revision for our version\n+\t\t *    %Y - the revision for their version\n \t\t *\n+\t\t * If the file is not named indentically in all versions, then each\n+\t\t * revision is joined with the corresponding path, separated by a colon.\n \t\t * The external merge driver should write the results in the\n \t\t * file named by %A, and signal that it has done with zero exit\n \t\t * status.\ndiff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh\nindex 72f8c1722ff..156a1efacfe 100755\n--- a/t/t6406-merge-attr.sh\n+++ b/t/t6406-merge-attr.sh\n@@ -42,11 +42,15 @@ test_expect_success setup '\n \t#!/bin/sh\n \n \torig=\"$1\" ours=\"$2\" theirs=\"$3\" exit=\"$4\" path=$5\n+\torig_name=\"$6\" our_name=\"$7\" their_name=\"$8\"\n \t(\n \t\techo \"orig is $orig\"\n \t\techo \"ours is $ours\"\n \t\techo \"theirs is $theirs\"\n \t\techo \"path is $path\"\n+\t\techo \"orig_name is $orig_name\"\n+\t\techo \"our_name is $our_name\"\n+\t\techo \"their_name is $their_name\"\n \t\techo \"=== orig ===\"\n \t\tcat \"$orig\"\n \t\techo \"=== ours ===\"\n@@ -121,7 +125,7 @@ test_expect_success 'custom merge backend' '\n \n \tgit reset --hard anchor &&\n \tgit config --replace-all \\\n-\tmerge.custom.driver \"./custom-merge %O %A %B 0 %P\" &&\n+\tmerge.custom.driver \"./custom-merge %O %A %B 0 %P %S %X %Y\" &&\n \tgit config --replace-all \\\n \tmerge.custom.name \"custom merge driver for testing\" &&\n \n@@ -132,7 +136,8 @@ test_expect_success 'custom merge backend' '\n \to=$(git unpack-file main^:text) &&\n \ta=$(git unpack-file side^:text) &&\n \tb=$(git unpack-file main:text) &&\n-\tsh -c \"./custom-merge $o $a $b 0 text\" &&\n+\tbase_revid=$(git rev-parse --short main^) &&\n+\tsh -c \"./custom-merge $o $a $b 0 text $base_revid HEAD main\" &&\n \tsed -e 1,3d $a >check-2 &&\n \tcmp check-1 check-2 &&\n \trm -f $o $a $b\n@@ -142,7 +147,7 @@ test_expect_success 'custom merge backend' '\n \n \tgit reset --hard anchor &&\n \tgit config --replace-all \\\n-\tmerge.custom.driver \"./custom-merge %O %A %B 1 %P\" &&\n+\tmerge.custom.driver \"./custom-merge %O %A %B 1 %P %S %X %Y\" &&\n \tgit config --replace-all \\\n \tmerge.custom.name \"custom merge driver for testing\" &&\n \n@@ -159,7 +164,8 @@ test_expect_success 'custom merge backend' '\n \to=$(git unpack-file main^:text) &&\n \ta=$(git unpack-file anchor:text) &&\n \tb=$(git unpack-file main:text) &&\n-\tsh -c \"./custom-merge $o $a $b 0 text\" &&\n+\tbase_revid=$(git rev-parse --short main^) &&\n+\tsh -c \"./custom-merge $o $a $b 0 text $base_revid HEAD main\" &&\n \tsed -e 1,3d $a >check-2 &&\n \tcmp check-1 check-2 &&\n \tsed -e 1,3d -e 4q $a >check-3 &&\n@@ -173,7 +179,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'\n \n \tgit reset --hard anchor &&\n \tgit config --replace-all \\\n-\tmerge.custom.driver \"./custom-merge %O %A %B 0 %P\" &&\n+\tmerge.custom.driver \"./custom-merge %O %A %B 0 %P %S %X %Y\" &&\n \tgit config --replace-all \\\n \tmerge.custom.name \"custom merge driver for testing\" &&\n \n\nbase-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5\n-- \ngitgitgadget\n"},{"id":"486979","messageId":"549875d6-b9a8-4b0d-8eaf-e12b72a20e16@app.fastmail.com","threadId":"60761","inReplyTo":"pull.1648.git.git.1705587974840.gitgitgadget@gmail.com","subject":"Re: [PATCH] merge-ll: expose revision names to custom drivers","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-01-18T15:25:25Z","receivedAt":"2024-01-18T15:25:47Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Hi\n\n> are working on, so that the merge conflict markers they introduce\n> can refer to those revisions. The placeholders '%S', '%X' and '%Y'\n> are introduced to this end.\n>\n> CC: Junio C Hamano <gitster@pobox.com>\n>\n> Signed-off-by: Antonin Delpeuch <antonin@delpeuch.eu>\n> ---\n\nThis is gitgitgadget but `git send-email` would probably not pick up the\nCC here since it is not part of the trailer section (because there is a\nblank line between the CC line and the signed-off-by line). Maybe\ngitgitgadget works the same way. Also the space between `CC:` and the\nvalue is a no-break space but I don’t know if that matters.[1]\n\n(Also see [2] for what it’s worth.)\n\n† 1: On Linux at least this often happens by accident by pressing AltGr+Space.\n🔗 2: https://lore.kernel.org/git/xmqqttwzded6.fsf@gitster.g/\n\n-- \nKristoffer Haugsbakk\n"},{"id":"486980","messageId":"pull.1648.v2.git.git.1705592581272.gitgitgadget@gmail.com","threadId":"60761","inReplyTo":"pull.1648.git.git.1705587974840.gitgitgadget@gmail.com","subject":"[PATCH v2] merge-ll: expose revision names to custom drivers","fromName":"Antonin Delpeuch via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-18T15:43:01Z","receivedAt":"2024-01-18T15:43:05Z","isPatch":true,"sender":{"key":"antonin@delpeuch.eu","avatar":"https://avatars.githubusercontent.com/u/309908?v=4"},"body":"From: Antonin Delpeuch <antonin@delpeuch.eu>\n\nCustom merge drivers need access to the names of the revisions they\nare working on, so that the merge conflict markers they introduce\ncan refer to those revisions. The placeholders '%S', '%X' and '%Y'\nare introduced to this end.\n\nSigned-off-by: Antonin Delpeuch <antonin@delpeuch.eu>\n---\n    merge-ll: expose revision names to custom drivers\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1648%2Fwetneb%2Fmerge_driver_pathnames-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1648/wetneb/merge_driver_pathnames-v2\nPull-Request: https://github.com/git/git/pull/1648\n\nRange-diff vs v1:\n\n 1:  e648f9f0696 ! 1:  6dec70529c0 merge-ll: expose revision names to custom drivers\n     @@ Commit message\n          can refer to those revisions. The placeholders '%S', '%X' and '%Y'\n          are introduced to this end.\n      \n     -    CC: Junio C Hamano <gitster@pobox.com>\n     -\n          Signed-off-by: Antonin Delpeuch <antonin@delpeuch.eu>\n      \n       ## Documentation/gitattributes.txt ##\n\n\n Documentation/gitattributes.txt | 10 +++++++---\n merge-ll.c                      | 17 ++++++++++++++---\n t/t6406-merge-attr.sh           | 16 +++++++++++-----\n 3 files changed, 32 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 201bdf5edbd..108c2e23baa 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -1141,7 +1141,7 @@ command to run to merge ancestor's version (`%O`), current\n version (`%A`) and the other branches' version (`%B`).  These\n three tokens are replaced with the names of temporary files that\n hold the contents of these versions when the command line is\n-built. Additionally, %L will be replaced with the conflict marker\n+built. Additionally, `%L` will be replaced with the conflict marker\n size (see below).\n \n The merge driver is expected to leave the result of the merge in\n@@ -1159,8 +1159,12 @@ When left unspecified, the driver itself is used for both\n internal merge and the final merge.\n \n The merge driver can learn the pathname in which the merged result\n-will be stored via placeholder `%P`.\n-\n+will be stored via placeholder `%P`. Additionally, the names of the\n+merge ancestor revision (`%S`), of the current revision (`%X`) and\n+of the other branch (`%Y`) can also be supplied. Those are short\n+revision names, optionally joined with the paths of the file in each\n+revision. Those paths are only present if they differ and are separated\n+from the revision by a colon.\n \n `conflict-marker-size`\n ^^^^^^^^^^^^^^^^^^^^^^\ndiff --git a/merge-ll.c b/merge-ll.c\nindex 1df58ebaac0..ae9eb69be14 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -185,9 +185,9 @@ static void create_temp(mmfile_t *src, char *path, size_t len)\n static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \t\t\tmmbuffer_t *result,\n \t\t\tconst char *path,\n-\t\t\tmmfile_t *orig, const char *orig_name UNUSED,\n-\t\t\tmmfile_t *src1, const char *name1 UNUSED,\n-\t\t\tmmfile_t *src2, const char *name2 UNUSED,\n+\t\t\tmmfile_t *orig, const char *orig_name,\n+\t\t\tmmfile_t *src1, const char *name1,\n+\t\t\tmmfile_t *src2, const char *name2,\n \t\t\tconst struct ll_merge_options *opts,\n \t\t\tint marker_size)\n {\n@@ -222,6 +222,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \t\t\tstrbuf_addf(&cmd, \"%d\", marker_size);\n \t\telse if (skip_prefix(format, \"P\", &format))\n \t\t\tsq_quote_buf(&cmd, path);\n+\t\telse if (skip_prefix(format, \"S\", &format))\n+\t\t    sq_quote_buf(&cmd, orig_name);\n+\t\telse if (skip_prefix(format, \"X\", &format))\n+\t\t\tsq_quote_buf(&cmd, name1);\n+\t\telse if (skip_prefix(format, \"Y\", &format))\n+\t\t\tsq_quote_buf(&cmd, name2);\n \t\telse\n \t\t\tstrbuf_addch(&cmd, '%');\n \t}\n@@ -315,7 +321,12 @@ static int read_merge_config(const char *var, const char *value,\n \t\t *    %B - temporary file name for the other branches' version.\n \t\t *    %L - conflict marker length\n \t\t *    %P - the original path (safely quoted for the shell)\n+\t\t *    %S - the revision for the merge base\n+\t\t *    %X - the revision for our version\n+\t\t *    %Y - the revision for their version\n \t\t *\n+\t\t * If the file is not named indentically in all versions, then each\n+\t\t * revision is joined with the corresponding path, separated by a colon.\n \t\t * The external merge driver should write the results in the\n \t\t * file named by %A, and signal that it has done with zero exit\n \t\t * status.\ndiff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh\nindex 72f8c1722ff..156a1efacfe 100755\n--- a/t/t6406-merge-attr.sh\n+++ b/t/t6406-merge-attr.sh\n@@ -42,11 +42,15 @@ test_expect_success setup '\n \t#!/bin/sh\n \n \torig=\"$1\" ours=\"$2\" theirs=\"$3\" exit=\"$4\" path=$5\n+\torig_name=\"$6\" our_name=\"$7\" their_name=\"$8\"\n \t(\n \t\techo \"orig is $orig\"\n \t\techo \"ours is $ours\"\n \t\techo \"theirs is $theirs\"\n \t\techo \"path is $path\"\n+\t\techo \"orig_name is $orig_name\"\n+\t\techo \"our_name is $our_name\"\n+\t\techo \"their_name is $their_name\"\n \t\techo \"=== orig ===\"\n \t\tcat \"$orig\"\n \t\techo \"=== ours ===\"\n@@ -121,7 +125,7 @@ test_expect_success 'custom merge backend' '\n \n \tgit reset --hard anchor &&\n \tgit config --replace-all \\\n-\tmerge.custom.driver \"./custom-merge %O %A %B 0 %P\" &&\n+\tmerge.custom.driver \"./custom-merge %O %A %B 0 %P %S %X %Y\" &&\n \tgit config --replace-all \\\n \tmerge.custom.name \"custom merge driver for testing\" &&\n \n@@ -132,7 +136,8 @@ test_expect_success 'custom merge backend' '\n \to=$(git unpack-file main^:text) &&\n \ta=$(git unpack-file side^:text) &&\n \tb=$(git unpack-file main:text) &&\n-\tsh -c \"./custom-merge $o $a $b 0 text\" &&\n+\tbase_revid=$(git rev-parse --short main^) &&\n+\tsh -c \"./custom-merge $o $a $b 0 text $base_revid HEAD main\" &&\n \tsed -e 1,3d $a >check-2 &&\n \tcmp check-1 check-2 &&\n \trm -f $o $a $b\n@@ -142,7 +147,7 @@ test_expect_success 'custom merge backend' '\n \n \tgit reset --hard anchor &&\n \tgit config --replace-all \\\n-\tmerge.custom.driver \"./custom-merge %O %A %B 1 %P\" &&\n+\tmerge.custom.driver \"./custom-merge %O %A %B 1 %P %S %X %Y\" &&\n \tgit config --replace-all \\\n \tmerge.custom.name \"custom merge driver for testing\" &&\n \n@@ -159,7 +164,8 @@ test_expect_success 'custom merge backend' '\n \to=$(git unpack-file main^:text) &&\n \ta=$(git unpack-file anchor:text) &&\n \tb=$(git unpack-file main:text) &&\n-\tsh -c \"./custom-merge $o $a $b 0 text\" &&\n+\tbase_revid=$(git rev-parse --short main^) &&\n+\tsh -c \"./custom-merge $o $a $b 0 text $base_revid HEAD main\" &&\n \tsed -e 1,3d $a >check-2 &&\n \tcmp check-1 check-2 &&\n \tsed -e 1,3d -e 4q $a >check-3 &&\n@@ -173,7 +179,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'\n \n \tgit reset --hard anchor &&\n \tgit config --replace-all \\\n-\tmerge.custom.driver \"./custom-merge %O %A %B 0 %P\" &&\n+\tmerge.custom.driver \"./custom-merge %O %A %B 0 %P %S %X %Y\" &&\n \tgit config --replace-all \\\n \tmerge.custom.name \"custom merge driver for testing\" &&\n \n\nbase-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5\n-- \ngitgitgadget\n"},{"id":"486981","messageId":"e9ecb4c9-0854-4b8e-87a7-ba66b64b5b1a@delpeuch.eu","threadId":"60761","inReplyTo":"549875d6-b9a8-4b0d-8eaf-e12b72a20e16@app.fastmail.com","subject":"Re: [PATCH] merge-ll: expose revision names to custom drivers","fromName":"Antonin Delpeuch","fromEmail":"antonin@delpeuch.eu","sentAt":"2024-01-18T15:42:59Z","receivedAt":"2024-01-18T15:43:05Z","isPatch":true,"sender":{"key":"antonin@delpeuch.eu","avatar":"https://avatars.githubusercontent.com/u/309908?v=4"},"body":"On 18/01/2024 16:25, Kristoffer Haugsbakk wrote:\n> This is gitgitgadget but `git send-email` would probably not pick up the\n> CC here since it is not part of the trailer section (because there is a\n> blank line between the CC line and the signed-off-by line). Maybe\n> gitgitgadget works the same way. Also the space between `CC:` and the\n> value is a no-break space but I don’t know if that matters.[1]\n\nThanks a lot for catching that! Let me fix and re-submit.\n\nAntonin\n\n"},{"id":"486996","messageId":"xmqq1qaeqtw7.fsf@gitster.g","threadId":"60761","inReplyTo":"pull.1648.v2.git.git.1705592581272.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] merge-ll: expose revision names to custom drivers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-18T20:16:40Z","receivedAt":"2024-01-18T20:16:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Antonin Delpeuch via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Antonin Delpeuch <antonin@delpeuch.eu>\n>\n> Custom merge drivers need access to the names of the revisions they\n> are working on, so that the merge conflict markers they introduce\n> can refer to those revisions. The placeholders '%S', '%X' and '%Y'\n> are introduced to this end.\n>\n> Signed-off-by: Antonin Delpeuch <antonin@delpeuch.eu>\n> ---\n>     merge-ll: expose revision names to custom drivers\n>\n>  Documentation/gitattributes.txt | 10 +++++++---\n>  merge-ll.c                      | 17 ++++++++++++++---\n>  t/t6406-merge-attr.sh           | 16 +++++++++++-----\n>  3 files changed, 32 insertions(+), 11 deletions(-)\n>\n> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\n> index 201bdf5edbd..108c2e23baa 100644\n> --- a/Documentation/gitattributes.txt\n> +++ b/Documentation/gitattributes.txt\n> @@ -1141,7 +1141,7 @@ command to run to merge ancestor's version (`%O`), current\n>  version (`%A`) and the other branches' version (`%B`).  These\n>  three tokens are replaced with the names of temporary files that\n>  hold the contents of these versions when the command line is\n> -built. Additionally, %L will be replaced with the conflict marker\n> +built. Additionally, `%L` will be replaced with the conflict marker\n>  size (see below).\n\nGood eyes.  Nice to fix it while we are in the vicinity.\n\n> @@ -1159,8 +1159,12 @@ When left unspecified, the driver itself is used for both\n>  internal merge and the final merge.\n>  \n>  The merge driver can learn the pathname in which the merged result\n> -will be stored via placeholder `%P`.\n> -\n> +will be stored via placeholder `%P`. Additionally, the names of the\n> +merge ancestor revision (`%S`), of the current revision (`%X`) and\n\nThe phrase already existsin the description of '%O', but the correct\nword for that is \"the common ancestor\", not \"the merge ancestor\".\nAt least this one is consistent with the existing error, so we can\nfix them together in a clean-up patch after the dust settles.  \n\nOr you could fix %O's description \"while at it\" and use the right\nterm from the get-go for %S.\n\n> diff --git a/merge-ll.c b/merge-ll.c\n> index 1df58ebaac0..ae9eb69be14 100644\n> --- a/merge-ll.c\n> +++ b/merge-ll.c\n> @@ -185,9 +185,9 @@ static void create_temp(mmfile_t *src, char *path, size_t len)\n>  static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n>  \t\t\tmmbuffer_t *result,\n>  \t\t\tconst char *path,\n> -\t\t\tmmfile_t *orig, const char *orig_name UNUSED,\n> -\t\t\tmmfile_t *src1, const char *name1 UNUSED,\n> -\t\t\tmmfile_t *src2, const char *name2 UNUSED,\n> +\t\t\tmmfile_t *orig, const char *orig_name,\n> +\t\t\tmmfile_t *src1, const char *name1,\n> +\t\t\tmmfile_t *src2, const char *name2,\n>  \t\t\tconst struct ll_merge_options *opts,\n>  \t\t\tint marker_size)\n>  {\n> @@ -222,6 +222,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n>  \t\t\tstrbuf_addf(&cmd, \"%d\", marker_size);\n>  \t\telse if (skip_prefix(format, \"P\", &format))\n>  \t\t\tsq_quote_buf(&cmd, path);\n> +\t\telse if (skip_prefix(format, \"S\", &format))\n> +\t\t    sq_quote_buf(&cmd, orig_name);\n> +\t\telse if (skip_prefix(format, \"X\", &format))\n> +\t\t\tsq_quote_buf(&cmd, name1);\n> +\t\telse if (skip_prefix(format, \"Y\", &format))\n> +\t\t\tsq_quote_buf(&cmd, name2);\n>  \t\telse\n>  \t\t\tstrbuf_addch(&cmd, '%');\n>  \t}\n\nI see some funny indentation for \"S\" here.\n\n> diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh\n> index 72f8c1722ff..156a1efacfe 100755\n> --- a/t/t6406-merge-attr.sh\n> +++ b/t/t6406-merge-attr.sh\n> @@ -42,11 +42,15 @@ test_expect_success setup '\n>  \t#!/bin/sh\n>  \n>  \torig=\"$1\" ours=\"$2\" theirs=\"$3\" exit=\"$4\" path=$5\n> +\torig_name=\"$6\" our_name=\"$7\" their_name=\"$8\"\n>  \t(\n>  \t\techo \"orig is $orig\"\n>  \t\techo \"ours is $ours\"\n>  \t\techo \"theirs is $theirs\"\n>  \t\techo \"path is $path\"\n> +\t\techo \"orig_name is $orig_name\"\n> +\t\techo \"our_name is $our_name\"\n> +\t\techo \"their_name is $their_name\"\n>  \t\techo \"=== orig ===\"\n>  \t\tcat \"$orig\"\n>  \t\techo \"=== ours ===\"\n> @@ -121,7 +125,7 @@ test_expect_success 'custom merge backend' '\n>  \n>  \tgit reset --hard anchor &&\n>  \tgit config --replace-all \\\n> -\tmerge.custom.driver \"./custom-merge %O %A %B 0 %P\" &&\n> +\tmerge.custom.driver \"./custom-merge %O %A %B 0 %P %S %X %Y\" &&\n>  \tgit config --replace-all \\\n>  \tmerge.custom.name \"custom merge driver for testing\" &&\n>  \n> @@ -132,7 +136,8 @@ test_expect_success 'custom merge backend' '\n>  \to=$(git unpack-file main^:text) &&\n>  \ta=$(git unpack-file side^:text) &&\n>  \tb=$(git unpack-file main:text) &&\n> -\tsh -c \"./custom-merge $o $a $b 0 text\" &&\n> +\tbase_revid=$(git rev-parse --short main^) &&\n> +\tsh -c \"./custom-merge $o $a $b 0 text $base_revid HEAD main\" &&\n>  \tsed -e 1,3d $a >check-2 &&\n>  \tcmp check-1 check-2 &&\n>  \trm -f $o $a $b\n\nOK.\n\n> @@ -142,7 +147,7 @@ test_expect_success 'custom merge backend' '\n>  \n>  \tgit reset --hard anchor &&\n>  \tgit config --replace-all \\\n> -\tmerge.custom.driver \"./custom-merge %O %A %B 1 %P\" &&\n> +\tmerge.custom.driver \"./custom-merge %O %A %B 1 %P %S %X %Y\" &&\n>  \tgit config --replace-all \\\n>  \tmerge.custom.name \"custom merge driver for testing\" &&\n>  \n> @@ -159,7 +164,8 @@ test_expect_success 'custom merge backend' '\n>  \to=$(git unpack-file main^:text) &&\n>  \ta=$(git unpack-file anchor:text) &&\n>  \tb=$(git unpack-file main:text) &&\n> -\tsh -c \"./custom-merge $o $a $b 0 text\" &&\n> +\tbase_revid=$(git rev-parse --short main^) &&\n> +\tsh -c \"./custom-merge $o $a $b 0 text $base_revid HEAD main\" &&\n>  \tsed -e 1,3d $a >check-2 &&\n>  \tcmp check-1 check-2 &&\n>  \tsed -e 1,3d -e 4q $a >check-3 &&\n\nOK.\n\n> @@ -173,7 +179,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'\n>  \n>  \tgit reset --hard anchor &&\n>  \tgit config --replace-all \\\n> -\tmerge.custom.driver \"./custom-merge %O %A %B 0 %P\" &&\n> +\tmerge.custom.driver \"./custom-merge %O %A %B 0 %P %S %X %Y\" &&\n>  \tgit config --replace-all \\\n>  \tmerge.custom.name \"custom merge driver for testing\" &&\n\n;-)\n\nThis one is expected to die and not produce meaningful output;\nI was wondering why this does not need to make corresponding changes\nto the expected output pattern like the earlier tests.\n\n"},{"id":"487006","messageId":"00fd20e2-73f5-42ca-b9a8-1ee227150eff@delpeuch.eu","threadId":"60761","inReplyTo":"xmqq1qaeqtw7.fsf@gitster.g","subject":"Re: [PATCH v2] merge-ll: expose revision names to custom drivers","fromName":"Antonin Delpeuch","fromEmail":"antonin@delpeuch.eu","sentAt":"2024-01-18T20:56:01Z","receivedAt":"2024-01-18T21:09:13Z","isPatch":true,"sender":{"key":"antonin@delpeuch.eu","avatar":"https://avatars.githubusercontent.com/u/309908?v=4"},"body":"Hi Junio,\n\nThanks a lot for your review! (and many apologies for the double sending \nas HTML…)\n\nOn 18/01/2024 21:16, Junio C Hamano wrote:\n> Or you could fix %O's description \"while at it\" and use the right\n> term from the get-go for %S.\nAgreed, I'll do that, it also feels more fitting to me.\n> I see some funny indentation for \"S\" here.\nOops, sorry about that.\n>> @@ -173,7 +179,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'\n>>   \n>>   \tgit reset --hard anchor &&\n>>   \tgit config --replace-all \\\n>> -\tmerge.custom.driver \"./custom-merge %O %A %B 0 %P\" &&\n>> +\tmerge.custom.driver \"./custom-merge %O %A %B 0 %P %S %X %Y\" &&\n>>   \tgit config --replace-all \\\n>>   \tmerge.custom.name \"custom merge driver for testing\" &&\n> ;-)\n>\n> This one is expected to die and not produce meaningful output;\n> I was wondering why this does not need to make corresponding changes\n> to the expected output pattern like the earlier tests.\n\nAs far as I can tell, this test does not compare the result of the merge \nto the expected merge driver output, because that output is expected to \nbe disregarded by git given that the merge driver died (see the last two \nlines). So it seems normal to me that we don't need to adapt the \nexpected output: the repository files are still left unchanged.\n\nI'll submit a new version of the patch with the two changes above.\n\nAntonin\n"},{"id":"487010","messageId":"pull.1648.v3.git.git.1705615794307.gitgitgadget@gmail.com","threadId":"60761","inReplyTo":"pull.1648.v2.git.git.1705592581272.gitgitgadget@gmail.com","subject":"[PATCH v3] merge-ll: expose revision names to custom drivers","fromName":"Antonin Delpeuch via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-18T22:09:54Z","receivedAt":"2024-01-18T22:09:57Z","isPatch":true,"sender":{"key":"antonin@delpeuch.eu","avatar":"https://avatars.githubusercontent.com/u/309908?v=4"},"body":"From: Antonin Delpeuch <antonin@delpeuch.eu>\n\nCustom merge drivers need access to the names of the revisions they\nare working on, so that the merge conflict markers they introduce\ncan refer to those revisions. The placeholders '%S', '%X' and '%Y'\nare introduced to this end.\n\nSigned-off-by: Antonin Delpeuch <antonin@delpeuch.eu>\n---\n    merge-ll: expose revision names to custom drivers\n    \n    Changes since v2:\n    \n     * change the documentation to use \"common ancestor\" rather than \"merge\n       ancestor\"\n     * fix indentation issue\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1648%2Fwetneb%2Fmerge_driver_pathnames-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1648/wetneb/merge_driver_pathnames-v3\nPull-Request: https://github.com/git/git/pull/1648\n\nRange-diff vs v2:\n\n 1:  6dec70529c0 ! 1:  aebd26711fe merge-ll: expose revision names to custom drivers\n     @@ Commit message\n          Signed-off-by: Antonin Delpeuch <antonin@delpeuch.eu>\n      \n       ## Documentation/gitattributes.txt ##\n     -@@ Documentation/gitattributes.txt: command to run to merge ancestor's version (`%O`), current\n     +@@ Documentation/gitattributes.txt: The `merge.*.name` variable gives the driver a human-readable\n     + name.\n     + \n     + The `merge.*.driver` variable's value is used to construct a\n     +-command to run to merge ancestor's version (`%O`), current\n     ++command to run to common ancestor's version (`%O`), current\n       version (`%A`) and the other branches' version (`%B`).  These\n       three tokens are replaced with the names of temporary files that\n       hold the contents of these versions when the command line is\n     @@ Documentation/gitattributes.txt: When left unspecified, the driver itself is use\n      -will be stored via placeholder `%P`.\n      -\n      +will be stored via placeholder `%P`. Additionally, the names of the\n     -+merge ancestor revision (`%S`), of the current revision (`%X`) and\n     ++common ancestor revision (`%S`), of the current revision (`%X`) and\n      +of the other branch (`%Y`) can also be supplied. Those are short\n      +revision names, optionally joined with the paths of the file in each\n      +revision. Those paths are only present if they differ and are separated\n     @@ merge-ll.c: static enum ll_merge_result ll_ext_merge(const struct ll_merge_drive\n       \t\telse if (skip_prefix(format, \"P\", &format))\n       \t\t\tsq_quote_buf(&cmd, path);\n      +\t\telse if (skip_prefix(format, \"S\", &format))\n     -+\t\t    sq_quote_buf(&cmd, orig_name);\n     ++\t\t\tsq_quote_buf(&cmd, orig_name);\n      +\t\telse if (skip_prefix(format, \"X\", &format))\n      +\t\t\tsq_quote_buf(&cmd, name1);\n      +\t\telse if (skip_prefix(format, \"Y\", &format))\n\n\n Documentation/gitattributes.txt | 12 ++++++++----\n merge-ll.c                      | 17 ++++++++++++++---\n t/t6406-merge-attr.sh           | 16 +++++++++++-----\n 3 files changed, 33 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 201bdf5edbd..86a0946bb9e 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -1137,11 +1137,11 @@ The `merge.*.name` variable gives the driver a human-readable\n name.\n \n The `merge.*.driver` variable's value is used to construct a\n-command to run to merge ancestor's version (`%O`), current\n+command to run to common ancestor's version (`%O`), current\n version (`%A`) and the other branches' version (`%B`).  These\n three tokens are replaced with the names of temporary files that\n hold the contents of these versions when the command line is\n-built. Additionally, %L will be replaced with the conflict marker\n+built. Additionally, `%L` will be replaced with the conflict marker\n size (see below).\n \n The merge driver is expected to leave the result of the merge in\n@@ -1159,8 +1159,12 @@ When left unspecified, the driver itself is used for both\n internal merge and the final merge.\n \n The merge driver can learn the pathname in which the merged result\n-will be stored via placeholder `%P`.\n-\n+will be stored via placeholder `%P`. Additionally, the names of the\n+common ancestor revision (`%S`), of the current revision (`%X`) and\n+of the other branch (`%Y`) can also be supplied. Those are short\n+revision names, optionally joined with the paths of the file in each\n+revision. Those paths are only present if they differ and are separated\n+from the revision by a colon.\n \n `conflict-marker-size`\n ^^^^^^^^^^^^^^^^^^^^^^\ndiff --git a/merge-ll.c b/merge-ll.c\nindex 1df58ebaac0..13e0713fe82 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -185,9 +185,9 @@ static void create_temp(mmfile_t *src, char *path, size_t len)\n static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \t\t\tmmbuffer_t *result,\n \t\t\tconst char *path,\n-\t\t\tmmfile_t *orig, const char *orig_name UNUSED,\n-\t\t\tmmfile_t *src1, const char *name1 UNUSED,\n-\t\t\tmmfile_t *src2, const char *name2 UNUSED,\n+\t\t\tmmfile_t *orig, const char *orig_name,\n+\t\t\tmmfile_t *src1, const char *name1,\n+\t\t\tmmfile_t *src2, const char *name2,\n \t\t\tconst struct ll_merge_options *opts,\n \t\t\tint marker_size)\n {\n@@ -222,6 +222,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \t\t\tstrbuf_addf(&cmd, \"%d\", marker_size);\n \t\telse if (skip_prefix(format, \"P\", &format))\n \t\t\tsq_quote_buf(&cmd, path);\n+\t\telse if (skip_prefix(format, \"S\", &format))\n+\t\t\tsq_quote_buf(&cmd, orig_name);\n+\t\telse if (skip_prefix(format, \"X\", &format))\n+\t\t\tsq_quote_buf(&cmd, name1);\n+\t\telse if (skip_prefix(format, \"Y\", &format))\n+\t\t\tsq_quote_buf(&cmd, name2);\n \t\telse\n \t\t\tstrbuf_addch(&cmd, '%');\n \t}\n@@ -315,7 +321,12 @@ static int read_merge_config(const char *var, const char *value,\n \t\t *    %B - temporary file name for the other branches' version.\n \t\t *    %L - conflict marker length\n \t\t *    %P - the original path (safely quoted for the shell)\n+\t\t *    %S - the revision for the merge base\n+\t\t *    %X - the revision for our version\n+\t\t *    %Y - the revision for their version\n \t\t *\n+\t\t * If the file is not named indentically in all versions, then each\n+\t\t * revision is joined with the corresponding path, separated by a colon.\n \t\t * The external merge driver should write the results in the\n \t\t * file named by %A, and signal that it has done with zero exit\n \t\t * status.\ndiff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh\nindex 72f8c1722ff..156a1efacfe 100755\n--- a/t/t6406-merge-attr.sh\n+++ b/t/t6406-merge-attr.sh\n@@ -42,11 +42,15 @@ test_expect_success setup '\n \t#!/bin/sh\n \n \torig=\"$1\" ours=\"$2\" theirs=\"$3\" exit=\"$4\" path=$5\n+\torig_name=\"$6\" our_name=\"$7\" their_name=\"$8\"\n \t(\n \t\techo \"orig is $orig\"\n \t\techo \"ours is $ours\"\n \t\techo \"theirs is $theirs\"\n \t\techo \"path is $path\"\n+\t\techo \"orig_name is $orig_name\"\n+\t\techo \"our_name is $our_name\"\n+\t\techo \"their_name is $their_name\"\n \t\techo \"=== orig ===\"\n \t\tcat \"$orig\"\n \t\techo \"=== ours ===\"\n@@ -121,7 +125,7 @@ test_expect_success 'custom merge backend' '\n \n \tgit reset --hard anchor &&\n \tgit config --replace-all \\\n-\tmerge.custom.driver \"./custom-merge %O %A %B 0 %P\" &&\n+\tmerge.custom.driver \"./custom-merge %O %A %B 0 %P %S %X %Y\" &&\n \tgit config --replace-all \\\n \tmerge.custom.name \"custom merge driver for testing\" &&\n \n@@ -132,7 +136,8 @@ test_expect_success 'custom merge backend' '\n \to=$(git unpack-file main^:text) &&\n \ta=$(git unpack-file side^:text) &&\n \tb=$(git unpack-file main:text) &&\n-\tsh -c \"./custom-merge $o $a $b 0 text\" &&\n+\tbase_revid=$(git rev-parse --short main^) &&\n+\tsh -c \"./custom-merge $o $a $b 0 text $base_revid HEAD main\" &&\n \tsed -e 1,3d $a >check-2 &&\n \tcmp check-1 check-2 &&\n \trm -f $o $a $b\n@@ -142,7 +147,7 @@ test_expect_success 'custom merge backend' '\n \n \tgit reset --hard anchor &&\n \tgit config --replace-all \\\n-\tmerge.custom.driver \"./custom-merge %O %A %B 1 %P\" &&\n+\tmerge.custom.driver \"./custom-merge %O %A %B 1 %P %S %X %Y\" &&\n \tgit config --replace-all \\\n \tmerge.custom.name \"custom merge driver for testing\" &&\n \n@@ -159,7 +164,8 @@ test_expect_success 'custom merge backend' '\n \to=$(git unpack-file main^:text) &&\n \ta=$(git unpack-file anchor:text) &&\n \tb=$(git unpack-file main:text) &&\n-\tsh -c \"./custom-merge $o $a $b 0 text\" &&\n+\tbase_revid=$(git rev-parse --short main^) &&\n+\tsh -c \"./custom-merge $o $a $b 0 text $base_revid HEAD main\" &&\n \tsed -e 1,3d $a >check-2 &&\n \tcmp check-1 check-2 &&\n \tsed -e 1,3d -e 4q $a >check-3 &&\n@@ -173,7 +179,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'\n \n \tgit reset --hard anchor &&\n \tgit config --replace-all \\\n-\tmerge.custom.driver \"./custom-merge %O %A %B 0 %P\" &&\n+\tmerge.custom.driver \"./custom-merge %O %A %B 0 %P %S %X %Y\" &&\n \tgit config --replace-all \\\n \tmerge.custom.name \"custom merge driver for testing\" &&\n \n\nbase-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5\n-- \ngitgitgadget\n"},{"id":"487088","messageId":"386c0318-0138-47c9-9e7f-d1004277226c@delpeuch.eu","threadId":"60761","inReplyTo":"pull.1648.v3.git.git.1705615794307.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] merge-ll: expose revision names to custom drivers","fromName":"Antonin Delpeuch","fromEmail":"antonin@delpeuch.eu","sentAt":"2024-01-19T20:02:35Z","receivedAt":"2024-01-19T20:02:49Z","isPatch":true,"sender":{"key":"antonin@delpeuch.eu","avatar":"https://avatars.githubusercontent.com/u/309908?v=4"},"body":"Hi Junio,\n\nAfter more testing (combining custom merge drivers with rerere) I \nrealized that my patch can lead to a segmentation error. Many apologies \nfor not having caught that earlier!\n\nOn 18/01/2024 23:09, Antonin Delpeuch via GitGitGadget wrote:\n> @@ -222,6 +222,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n>   \t\t\tstrbuf_addf(&cmd, \"%d\", marker_size);\n>   \t\telse if (skip_prefix(format, \"P\", &format))\n>   \t\t\tsq_quote_buf(&cmd, path);\n> +\t\telse if (skip_prefix(format, \"S\", &format))\n> +\t\t\tsq_quote_buf(&cmd, orig_name);\n> +\t\telse if (skip_prefix(format, \"X\", &format))\n> +\t\t\tsq_quote_buf(&cmd, name1);\n> +\t\telse if (skip_prefix(format, \"Y\", &format))\n> +\t\t\tsq_quote_buf(&cmd, name2);\n\nThe \"orig_name\", \"name1\" and \"name2\" pointers can be NULL at this stage. \nThis can happen when the merge is invoked from rerere, to resolve a \nconflict using a previous resolution.\n\nI wonder what the appropriate fallback would be in such a case. I am \ntempted to use the temporary filenames of the files to merge instead, so \nthat the merge driver can rely on those names being non-empty and being \nthe best string to use to identify the files. Passing an empty string \nseems dangerous to me, as it is likely to change the index of arguments \npassed to the merge driver. Passing fixed strings such as \"base\", \"ours\" \nand \"theirs\" could perhaps work too.\n\nLet me know if you have any preference about this.\n\nBest,\n\nAntonin\n\n\n"},{"id":"487134","messageId":"82624802-aa7f-4856-b819-9a2990b25a69@gmail.com","threadId":"60761","inReplyTo":"pull.1648.v3.git.git.1705615794307.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] merge-ll: expose revision names to custom drivers","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-01-20T14:13:50Z","receivedAt":"2024-01-20T14:13:53Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Antonin\n\nOn 18/01/2024 22:09, Antonin Delpeuch via GitGitGadget wrote:\n> From: Antonin Delpeuch <antonin@delpeuch.eu>\n> \n> Custom merge drivers need access to the names of the revisions they\n> are working on, so that the merge conflict markers they introduce\n> can refer to those revisions. The placeholders '%S', '%X' and '%Y'\n> are introduced to this end.\n\nThanks for working on this, I think it is a useful improvement. I guess \n'%X' and '%Y' are no worse than the existing '%A' and '%B' but I do \nwonder if we want to take the opportunity to switch to more descriptive \nnames for the various parameters passed to the custom merge strategy. We \ndo do this by supporting %(label:ours) modeled after the format \nspecifiers used by other commands such as \"git log\" and \"git for-each-ref\".\n\n> [...]\n> +will be stored via placeholder `%P`. Additionally, the names of the\n> +common ancestor revision (`%S`), of the current revision (`%X`) and\n> +of the other branch (`%Y`) can also be supplied. Those are short > +revision names, optionally joined with the paths of the file in each\n> +revision. Those paths are only present if they differ and are separated\n> +from the revision by a colon.\n\nIt might be simpler to just call these the \"conflict marker labels\" \nwithout tying ourselves to a particular format. Something like\n\n     The conflict labels to be used for the common ancestor, local head\n     and other head can be passed by using '%(label:base)',\n     '%(label:ours)' and '%(label:theirs) respectively.\n\n\n> @@ -222,6 +222,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n\nNot part of this patch but I noticed that we're passing the filenames \nfor '%A' etc. unquoted which is a bit scary.\n\n>   \t\t\tstrbuf_addf(&cmd, \"%d\", marker_size);\n>   \t\telse if (skip_prefix(format, \"P\", &format))\n>   \t\t\tsq_quote_buf(&cmd, path);\n> +\t\telse if (skip_prefix(format, \"S\", &format))\n> +\t\t\tsq_quote_buf(&cmd, orig_name);\n\nI think you can avoid the SIGSEV problem you mentioned in your other \nemail by changing this to\n\n\tsq_quote_buf(&cmd, orig_name ? orig_name, \"\");\n\nThat would make sure the labels we pass match the ones used by the \ninternal merge.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"487139","messageId":"xmqqy1cjgbna.fsf@gitster.g","threadId":"60761","inReplyTo":"386c0318-0138-47c9-9e7f-d1004277226c@delpeuch.eu","subject":"Re: [PATCH v3] merge-ll: expose revision names to custom drivers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-20T17:25:29Z","receivedAt":"2024-01-20T17:25:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antonin Delpeuch <antonin@delpeuch.eu> writes:\n\n> After more testing (combining custom merge drivers with rerere) I\n> realized that my patch can lead to a segmentation error. Many\n> apologies for not having caught that earlier!\n\nAh, understandable.  The 3-way merge machinery may not even have to\nwork on commit objects (it can merge two trees, using another tree\nas the \"common ancestor\" tree, just fine).\n\nAnd in such a case, it is perfectly possible there is no \"human\nreadable name\"; all there is may be a tree object name.\n\n> On 18/01/2024 23:09, Antonin Delpeuch via GitGitGadget wrote:\n>> @@ -222,6 +222,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n>>   \t\t\tstrbuf_addf(&cmd, \"%d\", marker_size);\n>>   \t\telse if (skip_prefix(format, \"P\", &format))\n>>   \t\t\tsq_quote_buf(&cmd, path);\n>> +\t\telse if (skip_prefix(format, \"S\", &format))\n>> +\t\t\tsq_quote_buf(&cmd, orig_name);\n>> +\t\telse if (skip_prefix(format, \"X\", &format))\n>> +\t\t\tsq_quote_buf(&cmd, name1);\n>> +\t\telse if (skip_prefix(format, \"Y\", &format))\n>> +\t\t\tsq_quote_buf(&cmd, name2);\n>\n> The \"orig_name\", \"name1\" and \"name2\" pointers can be NULL at this\n> stage. This can happen when the merge is invoked from rerere, to\n> resolve a conflict using a previous resolution.\n\n\tsq_quote_buf(&cmd, name1 ? name1 : \"(ours)\");\n\nor something like that, perhaps.\n\n"},{"id":"487140","messageId":"xmqqsf2rgb39.fsf@gitster.g","threadId":"60761","inReplyTo":"82624802-aa7f-4856-b819-9a2990b25a69@gmail.com","subject":"Re: [PATCH v3] merge-ll: expose revision names to custom drivers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-20T17:37:30Z","receivedAt":"2024-01-20T17:37:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> Not part of this patch but I noticed that we're passing the filenames\n> for '%A' etc. unquoted which is a bit scary.\n\nMay be scary but safe, as long as create_temp() gives a reasonable\ntemporary filename.  We pass \".merge_file_XXXXXX\" to xmkstemp(),\nwhich calls into mkstemp(), which should give us a shell safe name?\n\nIt also should be a safe conversion to change strbuf_addstr() used\nfor these three to sq_quote_buf(), as the string with these %[OAB]\nplaceholders are passed to the shell that eats the quoting before\ninvoking the end-user supplied external merge driver, which means\nthe merge driver would not notice any difference.\n\nThanks for being careful ;-)\n\n"},{"id":"487143","messageId":"7dbe4006-bfc2-402e-8073-870d7ec38319@gmail.com","threadId":"60761","inReplyTo":"xmqqsf2rgb39.fsf@gitster.g","subject":"Re: [PATCH v3] merge-ll: expose revision names to custom drivers","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-01-20T18:23:16Z","receivedAt":"2024-01-20T18:23:19Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 20/01/2024 17:37, Junio C Hamano wrote:\n> Phillip Wood <phillip.wood123@gmail.com> writes:\n> \n>> Not part of this patch but I noticed that we're passing the filenames\n>> for '%A' etc. unquoted which is a bit scary.\n> \n> May be scary but safe, as long as create_temp() gives a reasonable\n> temporary filename.  We pass \".merge_file_XXXXXX\" to xmkstemp(),\n> which calls into mkstemp(), which should give us a shell safe name?\n\nYes. I'd mis-read create_temp() and thought we were appending \n\".merge_file_XXXXX\" to the path being merged but looking at it again it \nis safe.\n\n> It also should be a safe conversion to change strbuf_addstr() used\n> for these three to sq_quote_buf(), as the string with these %[OAB]\n> placeholders are passed to the shell that eats the quoting before\n> invoking the end-user supplied external merge driver, which means\n> the merge driver would not notice any difference.\n\nI agree that would be a safe conversion , but I'm not sure it is worth it.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"487160","messageId":"xmqqcytvei3c.fsf@gitster.g","threadId":"60761","inReplyTo":"82624802-aa7f-4856-b819-9a2990b25a69@gmail.com","subject":"Re: [PATCH v3] merge-ll: expose revision names to custom drivers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-20T22:49:11Z","receivedAt":"2024-01-20T22:49:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> Thanks for working on this, I think it is a useful improvement. I\n> guess '%X' and '%Y' are no worse than the existing '%A' and '%B' but I\n> do wonder if we want to take the opportunity to switch to more\n> descriptive names for the various parameters passed to the custom\n> merge strategy. We do do this by supporting %(label:ours) modeled\n> after the format specifiers used by other commands such as \"git log\"\n> and \"git for-each-ref\".\n\nPerhaps.  Unlike the --format option these commands take, the\nplaceholders are never typed from the command line (they always are\ntaken from the configuration file), so mnemonic value longer version\ngives over the current single-letter ones is not as valuable, while\nmaking the total line length longer.  So I dunno.\n\n>> [...]\n>> +will be stored via placeholder `%P`. Additionally, the names of the\n>> +common ancestor revision (`%S`), of the current revision (`%X`) and\n>> +of the other branch (`%Y`) can also be supplied. Those are short > +revision names, optionally joined with the paths of the file in each\n>> +revision. Those paths are only present if they differ and are separated\n>> +from the revision by a colon.\n>\n> It might be simpler to just call these the \"conflict marker labels\"\n> without tying ourselves to a particular format. Something like\n>\n>     The conflict labels to be used for the common ancestor, local head\n>     and other head can be passed by using '%(label:base)',\n>     '%(label:ours)' and '%(label:theirs) respectively.\n\nYeah, that sounds like a good improvement, even if we did not use\nthe longhand placeholders and replaced %(label:{base,ours,theirs})\nwith %S, %X, and %Y.\n\n>> @@ -222,6 +222,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n>\n> Not part of this patch but I noticed that we're passing the filenames\n> for '%A' etc. unquoted which is a bit scary.\n>\n>>   \t\t\tstrbuf_addf(&cmd, \"%d\", marker_size);\n>>   \t\telse if (skip_prefix(format, \"P\", &format))\n>>   \t\t\tsq_quote_buf(&cmd, path);\n>> +\t\telse if (skip_prefix(format, \"S\", &format))\n>> +\t\t\tsq_quote_buf(&cmd, orig_name);\n>\n> I think you can avoid the SIGSEV problem you mentioned in your other\n> email by changing this to\n>\n> \tsq_quote_buf(&cmd, orig_name ? orig_name, \"\");\n>\n> That would make sure the labels we pass match the ones used by the\n> internal merge.\n\nMakes sense.  That would be much better than using hardcoded string\n\"ours\", \"theirs\", etc.\n"},{"id":"487349","messageId":"pull.1648.v4.git.git.1706126951879.gitgitgadget@gmail.com","threadId":"60761","inReplyTo":"pull.1648.v3.git.git.1705615794307.gitgitgadget@gmail.com","subject":"[PATCH v4] merge-ll: expose revision names to custom drivers","fromName":"Antonin Delpeuch via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-24T20:09:11Z","receivedAt":"2024-01-24T20:09:16Z","isPatch":true,"sender":{"key":"antonin@delpeuch.eu","avatar":"https://avatars.githubusercontent.com/u/309908?v=4"},"body":"From: Antonin Delpeuch <antonin@delpeuch.eu>\n\nCustom merge drivers need access to the names of the revisions they\nare working on, so that the merge conflict markers they introduce\ncan refer to those revisions. The placeholders '%S', '%X' and '%Y'\nare introduced to this end.\n\nSigned-off-by: Antonin Delpeuch <antonin@delpeuch.eu>\n---\n    merge-ll: expose revision names to custom drivers\n    \n    Changes since v3:\n    \n     * simplify the documentation as suggested by Phillip, keeping\n       single-letter placeholders as suggested by Junio\n     * add fallback to empty string when the collision marker names are\n       NULL, as suggested by Phillip\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1648%2Fwetneb%2Fmerge_driver_pathnames-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1648/wetneb/merge_driver_pathnames-v4\nPull-Request: https://github.com/git/git/pull/1648\n\nRange-diff vs v3:\n\n 1:  aebd26711fe ! 1:  47aca4fc8f6 merge-ll: expose revision names to custom drivers\n     @@ Documentation/gitattributes.txt: When left unspecified, the driver itself is use\n       The merge driver can learn the pathname in which the merged result\n      -will be stored via placeholder `%P`.\n      -\n     -+will be stored via placeholder `%P`. Additionally, the names of the\n     -+common ancestor revision (`%S`), of the current revision (`%X`) and\n     -+of the other branch (`%Y`) can also be supplied. Those are short\n     -+revision names, optionally joined with the paths of the file in each\n     -+revision. Those paths are only present if they differ and are separated\n     -+from the revision by a colon.\n     ++will be stored via placeholder `%P`. The conflict labels to be used\n     ++for the common ancestor, local head and other head can be passed by\n     ++using '%S', '%X' and '%Y` respectively.\n       \n       `conflict-marker-size`\n       ^^^^^^^^^^^^^^^^^^^^^^\n     @@ merge-ll.c: static enum ll_merge_result ll_ext_merge(const struct ll_merge_drive\n       \t\telse if (skip_prefix(format, \"P\", &format))\n       \t\t\tsq_quote_buf(&cmd, path);\n      +\t\telse if (skip_prefix(format, \"S\", &format))\n     -+\t\t\tsq_quote_buf(&cmd, orig_name);\n     ++\t\t\tsq_quote_buf(&cmd, orig_name ? orig_name : \"\");\n      +\t\telse if (skip_prefix(format, \"X\", &format))\n     -+\t\t\tsq_quote_buf(&cmd, name1);\n     ++\t\t\tsq_quote_buf(&cmd, name1 ? name1 : \"\");\n      +\t\telse if (skip_prefix(format, \"Y\", &format))\n     -+\t\t\tsq_quote_buf(&cmd, name2);\n     ++\t\t\tsq_quote_buf(&cmd, name2 ? name2 : \"\");\n       \t\telse\n       \t\t\tstrbuf_addch(&cmd, '%');\n       \t}\n\n\n Documentation/gitattributes.txt |  9 +++++----\n merge-ll.c                      | 17 ++++++++++++++---\n t/t6406-merge-attr.sh           | 16 +++++++++++-----\n 3 files changed, 30 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 201bdf5edbd..4338d023d9a 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -1137,11 +1137,11 @@ The `merge.*.name` variable gives the driver a human-readable\n name.\n \n The `merge.*.driver` variable's value is used to construct a\n-command to run to merge ancestor's version (`%O`), current\n+command to run to common ancestor's version (`%O`), current\n version (`%A`) and the other branches' version (`%B`).  These\n three tokens are replaced with the names of temporary files that\n hold the contents of these versions when the command line is\n-built. Additionally, %L will be replaced with the conflict marker\n+built. Additionally, `%L` will be replaced with the conflict marker\n size (see below).\n \n The merge driver is expected to leave the result of the merge in\n@@ -1159,8 +1159,9 @@ When left unspecified, the driver itself is used for both\n internal merge and the final merge.\n \n The merge driver can learn the pathname in which the merged result\n-will be stored via placeholder `%P`.\n-\n+will be stored via placeholder `%P`. The conflict labels to be used\n+for the common ancestor, local head and other head can be passed by\n+using '%S', '%X' and '%Y` respectively.\n \n `conflict-marker-size`\n ^^^^^^^^^^^^^^^^^^^^^^\ndiff --git a/merge-ll.c b/merge-ll.c\nindex 1df58ebaac0..5ffb045efb9 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -185,9 +185,9 @@ static void create_temp(mmfile_t *src, char *path, size_t len)\n static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \t\t\tmmbuffer_t *result,\n \t\t\tconst char *path,\n-\t\t\tmmfile_t *orig, const char *orig_name UNUSED,\n-\t\t\tmmfile_t *src1, const char *name1 UNUSED,\n-\t\t\tmmfile_t *src2, const char *name2 UNUSED,\n+\t\t\tmmfile_t *orig, const char *orig_name,\n+\t\t\tmmfile_t *src1, const char *name1,\n+\t\t\tmmfile_t *src2, const char *name2,\n \t\t\tconst struct ll_merge_options *opts,\n \t\t\tint marker_size)\n {\n@@ -222,6 +222,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \t\t\tstrbuf_addf(&cmd, \"%d\", marker_size);\n \t\telse if (skip_prefix(format, \"P\", &format))\n \t\t\tsq_quote_buf(&cmd, path);\n+\t\telse if (skip_prefix(format, \"S\", &format))\n+\t\t\tsq_quote_buf(&cmd, orig_name ? orig_name : \"\");\n+\t\telse if (skip_prefix(format, \"X\", &format))\n+\t\t\tsq_quote_buf(&cmd, name1 ? name1 : \"\");\n+\t\telse if (skip_prefix(format, \"Y\", &format))\n+\t\t\tsq_quote_buf(&cmd, name2 ? name2 : \"\");\n \t\telse\n \t\t\tstrbuf_addch(&cmd, '%');\n \t}\n@@ -315,7 +321,12 @@ static int read_merge_config(const char *var, const char *value,\n \t\t *    %B - temporary file name for the other branches' version.\n \t\t *    %L - conflict marker length\n \t\t *    %P - the original path (safely quoted for the shell)\n+\t\t *    %S - the revision for the merge base\n+\t\t *    %X - the revision for our version\n+\t\t *    %Y - the revision for their version\n \t\t *\n+\t\t * If the file is not named indentically in all versions, then each\n+\t\t * revision is joined with the corresponding path, separated by a colon.\n \t\t * The external merge driver should write the results in the\n \t\t * file named by %A, and signal that it has done with zero exit\n \t\t * status.\ndiff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh\nindex 72f8c1722ff..156a1efacfe 100755\n--- a/t/t6406-merge-attr.sh\n+++ b/t/t6406-merge-attr.sh\n@@ -42,11 +42,15 @@ test_expect_success setup '\n \t#!/bin/sh\n \n \torig=\"$1\" ours=\"$2\" theirs=\"$3\" exit=\"$4\" path=$5\n+\torig_name=\"$6\" our_name=\"$7\" their_name=\"$8\"\n \t(\n \t\techo \"orig is $orig\"\n \t\techo \"ours is $ours\"\n \t\techo \"theirs is $theirs\"\n \t\techo \"path is $path\"\n+\t\techo \"orig_name is $orig_name\"\n+\t\techo \"our_name is $our_name\"\n+\t\techo \"their_name is $their_name\"\n \t\techo \"=== orig ===\"\n \t\tcat \"$orig\"\n \t\techo \"=== ours ===\"\n@@ -121,7 +125,7 @@ test_expect_success 'custom merge backend' '\n \n \tgit reset --hard anchor &&\n \tgit config --replace-all \\\n-\tmerge.custom.driver \"./custom-merge %O %A %B 0 %P\" &&\n+\tmerge.custom.driver \"./custom-merge %O %A %B 0 %P %S %X %Y\" &&\n \tgit config --replace-all \\\n \tmerge.custom.name \"custom merge driver for testing\" &&\n \n@@ -132,7 +136,8 @@ test_expect_success 'custom merge backend' '\n \to=$(git unpack-file main^:text) &&\n \ta=$(git unpack-file side^:text) &&\n \tb=$(git unpack-file main:text) &&\n-\tsh -c \"./custom-merge $o $a $b 0 text\" &&\n+\tbase_revid=$(git rev-parse --short main^) &&\n+\tsh -c \"./custom-merge $o $a $b 0 text $base_revid HEAD main\" &&\n \tsed -e 1,3d $a >check-2 &&\n \tcmp check-1 check-2 &&\n \trm -f $o $a $b\n@@ -142,7 +147,7 @@ test_expect_success 'custom merge backend' '\n \n \tgit reset --hard anchor &&\n \tgit config --replace-all \\\n-\tmerge.custom.driver \"./custom-merge %O %A %B 1 %P\" &&\n+\tmerge.custom.driver \"./custom-merge %O %A %B 1 %P %S %X %Y\" &&\n \tgit config --replace-all \\\n \tmerge.custom.name \"custom merge driver for testing\" &&\n \n@@ -159,7 +164,8 @@ test_expect_success 'custom merge backend' '\n \to=$(git unpack-file main^:text) &&\n \ta=$(git unpack-file anchor:text) &&\n \tb=$(git unpack-file main:text) &&\n-\tsh -c \"./custom-merge $o $a $b 0 text\" &&\n+\tbase_revid=$(git rev-parse --short main^) &&\n+\tsh -c \"./custom-merge $o $a $b 0 text $base_revid HEAD main\" &&\n \tsed -e 1,3d $a >check-2 &&\n \tcmp check-1 check-2 &&\n \tsed -e 1,3d -e 4q $a >check-3 &&\n@@ -173,7 +179,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'\n \n \tgit reset --hard anchor &&\n \tgit config --replace-all \\\n-\tmerge.custom.driver \"./custom-merge %O %A %B 0 %P\" &&\n+\tmerge.custom.driver \"./custom-merge %O %A %B 0 %P %S %X %Y\" &&\n \tgit config --replace-all \\\n \tmerge.custom.name \"custom merge driver for testing\" &&\n \n\nbase-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5\n-- \ngitgitgadget\n"},{"id":"487354","messageId":"xmqqfrymbfe7.fsf@gitster.g","threadId":"60761","inReplyTo":"pull.1648.v4.git.git.1706126951879.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] merge-ll: expose revision names to custom drivers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-24T21:17:04Z","receivedAt":"2024-01-24T21:17:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Antonin Delpeuch via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Antonin Delpeuch <antonin@delpeuch.eu>\n>\n> Custom merge drivers need access to the names of the revisions they\n> are working on, so that the merge conflict markers they introduce\n> can refer to those revisions. The placeholders '%S', '%X' and '%Y'\n> are introduced to this end.\n>\n> Signed-off-by: Antonin Delpeuch <antonin@delpeuch.eu>\n> ---\n\nThis looks all good.\n\nLet's declare a victory and merge it down to 'next' soonish.\n\nThanks for sticking to the topic.\n"}]}