diff mbox series

[v5,1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge

Message ID 20240225-ml-log-merge-with-cherry-pick-and-other-pseudo-heads-v5-1-af1ef2d9e44d@gmail.com (mailing list archive)
State Superseded
Headers show
Series Implement `git log --merge` also for rebase/cherry-pick/revert | expand

Commit Message

Philippe Blain Feb. 25, 2024, 9:56 p.m. UTC
From: Michael Lohmann <mi.al.lohmann@gmail.com>

This is done to
(1) ensure MERGE_HEAD is a ref,
(2) obtain the oid without any prefixing by refs.c:repo_dwim_ref()
(3) error out when MERGE_HEAD is a symref.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 revision.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Jean-Noël AVILA Feb. 26, 2024, 5:22 p.m. UTC | #1
Hello,

Le 25/02/2024 à 22:56, Philippe Blain a écrit :
> From: Michael Lohmann <mi.al.lohmann@gmail.com>
> 
> This is done to
> (1) ensure MERGE_HEAD is a ref,
> (2) obtain the oid without any prefixing by refs.c:repo_dwim_ref()
> (3) error out when MERGE_HEAD is a symref.
> 
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
>  revision.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/revision.c b/revision.c
> index 2424c9bd67..ee26988cc6 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1973,8 +1973,12 @@ static void prepare_show_merge(struct rev_info *revs)
>  	if (repo_get_oid(the_repository, "HEAD", &oid))
>  		die("--merge without HEAD?");
>  	head = lookup_commit_or_die(&oid, "HEAD");
> -	if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
> +	if (read_ref_full("MERGE_HEAD",
> +			RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> +			&oid, NULL))
>  		die("--merge without MERGE_HEAD?");
> +	if (is_null_oid(&oid))
> +		die(_("MERGE_HEAD is a symbolic ref?"));

Following the thread about being less passive-aggressive, maybe this
could be rephrased in an assertive mood.

By the way, this string is translatable, but not the one 2 lines above.
What is the policy around translation?

>  	other = lookup_commit_or_die(&oid, "MERGE_HEAD");
>  	add_pending_object(revs, &head->object, "HEAD");
>  	add_pending_object(revs, &other->object, "MERGE_HEAD");
> 

Thanks.
Philippe Blain Feb. 26, 2024, 5:54 p.m. UTC | #2
Hi Jean-Noël,

Le 2024-02-26 à 12:22, Jean-Noël Avila a écrit :
> Hello,
> 
> Le 25/02/2024 à 22:56, Philippe Blain a écrit :
>> From: Michael Lohmann <mi.al.lohmann@gmail.com>
>>
>> This is done to
>> (1) ensure MERGE_HEAD is a ref,
>> (2) obtain the oid without any prefixing by refs.c:repo_dwim_ref()
>> (3) error out when MERGE_HEAD is a symref.
>>
>> Helped-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
>> ---
>>  revision.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/revision.c b/revision.c
>> index 2424c9bd67..ee26988cc6 100644
>> --- a/revision.c
>> +++ b/revision.c
>> @@ -1973,8 +1973,12 @@ static void prepare_show_merge(struct rev_info *revs)
>>  	if (repo_get_oid(the_repository, "HEAD", &oid))
>>  		die("--merge without HEAD?");
>>  	head = lookup_commit_or_die(&oid, "HEAD");
>> -	if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
>> +	if (read_ref_full("MERGE_HEAD",
>> +			RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
>> +			&oid, NULL))
>>  		die("--merge without MERGE_HEAD?");
>> +	if (is_null_oid(&oid))
>> +		die(_("MERGE_HEAD is a symbolic ref?"));
> 
> Following the thread about being less passive-aggressive, maybe this
> could be rephrased in an assertive mood.

Yes, Junio suggested the same in <xmqqa5nnj10v.fsf@gitster.g>.

> 
> By the way, this string is translatable, but not the one 2 lines above.
> What is the policy around translation?

My understanding is that new error messages should be translated, but here the patch
is not touching the message "--merge without HEAD?" so I would think
it is OK to avoid changing these lines to mark it for translation.
But, I could make that change in a preparatory patch (and rephrase it 
at the same time). 

Thanks, 
Philippe.
diff mbox series

Patch

diff --git a/revision.c b/revision.c
index 2424c9bd67..ee26988cc6 100644
--- a/revision.c
+++ b/revision.c
@@ -1973,8 +1973,12 @@  static void prepare_show_merge(struct rev_info *revs)
 	if (repo_get_oid(the_repository, "HEAD", &oid))
 		die("--merge without HEAD?");
 	head = lookup_commit_or_die(&oid, "HEAD");
-	if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
+	if (read_ref_full("MERGE_HEAD",
+			RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
+			&oid, NULL))
 		die("--merge without MERGE_HEAD?");
+	if (is_null_oid(&oid))
+		die(_("MERGE_HEAD is a symbolic ref?"));
 	other = lookup_commit_or_die(&oid, "MERGE_HEAD");
 	add_pending_object(revs, &head->object, "HEAD");
 	add_pending_object(revs, &other->object, "MERGE_HEAD");