merge-ll: expose revision names to custom drivers #1648
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/preview |
|
Preview email sent as pull.1648.git.git.1705587854932.gitgitgadget@gmail.com |
|
/submit |
|
Submitted as pull.1648.git.git.1705587974840.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, "Kristoffer Haugsbakk" wrote (reply to this): Hi
> 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 <gitster@pobox.com>
>
> Signed-off-by: Antonin Delpeuch <antonin@delpeuch.eu>
> ---
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/xmqqttwzded6.fsf@gitster.g/
--
Kristoffer Haugsbakk |
|
User |
wetneb
force-pushed
the
merge_driver_pathnames
branch
from
e648f9f
to
6dec705
Compare
|
/submit |
|
Submitted as pull.1648.v2.git.git.1705592581272.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Antonin Delpeuch wrote (reply to this): On 18/01/2024 16:25, Kristoffer Haugsbakk wrote:
> 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
|
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Antonin Delpeuch via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Antonin Delpeuch <antonin@delpeuch.eu>
>
> 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 <antonin@delpeuch.eu>
> ---
> 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(-)
>
> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
> index 201bdf5edbd..108c2e23baa 100644
> --- a/Documentation/gitattributes.txt
> +++ b/Documentation/gitattributes.txt
> @@ -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.
> @@ -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.
> diff --git a/merge-ll.c b/merge-ll.c
> index 1df58ebaac0..ae9eb69be14 100644
> --- a/merge-ll.c
> +++ b/merge-ll.c
> @@ -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, '%');
> }
I see some funny indentation for "S" here.
> diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
> index 72f8c1722ff..156a1efacfe 100755
> --- a/t/t6406-merge-attr.sh
> +++ b/t/t6406-merge-attr.sh
> @@ -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
OK.
> @@ -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 &&
OK.
> @@ -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.
|
wetneb
force-pushed
the
merge_driver_pathnames
branch
from
6dec705
to
aebd267
Compare
|
On the Git mailing list, Antonin Delpeuch wrote (reply to this): Hi Junio,
Thanks a lot for your review! (and many apologies for the double sending as HTML…)
On 18/01/2024 21:16, Junio C Hamano wrote:
> 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.
> I see some funny indentation for "S" here.
Oops, sorry about that.
>> @@ -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.
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 |
|
This branch is now known as |
|
This patch series was integrated into seen via 4893b29. |
|
/submit |
|
Submitted as pull.1648.v3.git.git.1705615794307.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
wetneb
force-pushed
the
merge_driver_pathnames
branch
from
aebd267
to
65128c3
Compare
|
On the Git mailing list, Antonin Delpeuch wrote (reply to this): 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!
On 18/01/2024 23:09, Antonin Delpeuch via GitGitGadget wrote:
> @@ -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
|
|
This patch series was integrated into seen via 19c5bfa. |
|
There was a status update in the "New Topics" section about the branch source: <pull.1648.v3.git.git.1705615794307.gitgitgadget@gmail.com> |
|
On the Git mailing list, Phillip Wood wrote (reply to this): Hi Antonin
On 18/01/2024 22:09, Antonin Delpeuch via GitGitGadget wrote:
> From: Antonin Delpeuch <antonin@delpeuch.eu>
> > 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".
> [...]
> +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.
> @@ -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.
> 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
|
|
User |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Antonin Delpeuch <antonin@delpeuch.eu> writes:
> 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.
> On 18/01/2024 23:09, Antonin Delpeuch via GitGitGadget wrote:
>> @@ -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.
sq_quote_buf(&cmd, name1 ? name1 : "(ours)");
or something like that, perhaps.
|
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Phillip Wood <phillip.wood123@gmail.com> writes:
> 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 ;-)
|
|
On the Git mailing list, Phillip Wood wrote (reply to this): Hi Junio
On 20/01/2024 17:37, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> >> 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.
> 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
|
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Phillip Wood <phillip.wood123@gmail.com> writes:
> 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.
>> [...]
>> +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.
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.
>> @@ -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.
>
>> 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.
Makes sense. That would be much better than using hardcoded string
"ours", "theirs", etc. |
|
This patch series was integrated into seen via 3c84fa7. |
|
This patch series was integrated into seen via 767bcaf. |
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 <antonin@delpeuch.eu>
wetneb
force-pushed
the
merge_driver_pathnames
branch
from
65128c3
to
47aca4f
Compare
|
/preview |
|
Preview email sent as pull.1648.v4.git.git.1706126716942.gitgitgadget@gmail.com |
|
/submit |
|
Submitted as pull.1648.v4.git.git.1706126951879.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Antonin Delpeuch via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Antonin Delpeuch <antonin@delpeuch.eu>
>
> 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 <antonin@delpeuch.eu>
> ---
This looks all good.
Let's declare a victory and merge it down to 'next' soonish.
Thanks for sticking to the topic. |
|
This patch series was integrated into seen via 8ca3a35. |
|
This patch series was integrated into seen via ed35445. |
|
This patch series was integrated into next via d9cf4e2. |
|
This patch series was integrated into seen via 68131bf. |
|
There was a status update in the "Cooking" section about the branch The labels on conflict markers for the common ancestor, our version, and the other version are available to custom 3-way merge driver via %S, %X, and %Y placeholders. Will merge to 'master'. source: <pull.1648.v4.git.git.1706126951879.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via 444755b. |
|
This patch series was integrated into seen via 3ddf313. |
|
There was a status update in the "Cooking" section about the branch The labels on conflict markers for the common ancestor, our version, and the other version are available to custom 3-way merge driver via %S, %X, and %Y placeholders. Will merge to 'master'. source: <pull.1648.v4.git.git.1706126951879.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via 963af59. |
|
This patch series was integrated into seen via 18b29c3. |
|
This patch series was integrated into seen via 3a14edc. |
|
This patch series was integrated into seen via 985170d. |
|
This patch series was integrated into seen via d9cf4e2. |
|
This patch series was integrated into seen via 985170d. |
|
This patch series was integrated into seen via 55bc84b. |
|
This patch series was integrated into seen via 6ae51a3. |
|
This patch series was integrated into seen via 9e189a0. |
|
This patch series was integrated into master via 9e189a0. |
|
This patch series was integrated into next via 9e189a0. |
|
Closed via 9e189a0. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes since v3:
cc: Junio C Hamano gitster@pobox.com
cc: Phillip Wood phillip.wood123@gmail.com