Re: [PATCH v4 2/2] revision: implement `git log --merge` also for rebase/cherry-pick/revert

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Philippe

On 13/02/2024 13:27, Philippe Blain wrote:
> Le 2024-02-12 à 06:02, Phillip Wood a écrit :
>> Hi Philippe
>> On 10/02/2024 23:35, Philippe Blain wrote:
>>> From: Michael Lohmann <mi.al.lohmann@gmail.com>
>>> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
>>> index 2bf239ff03..5b4672c346 100644
>>> --- a/Documentation/rev-list-options.txt
>>> +++ b/Documentation/rev-list-options.txt
>>> @@ -341,8 +341,10 @@ See also linkgit:git-reflog[1].
>>>    Under `--pretty=reference`, this information will not be shown at all.
>>>      --merge::
>>> -    After a failed merge, show refs that touch files having a
>>> -    conflict and don't exist on all heads to merge.
>>> +    Show commits touching conflicted paths in the range `HEAD...$OTHER`,
>>> +    where `$OTHER` is the first existing pseudoref in `MERGE_HEAD`,
>>> +    `CHERRY_PICK_HEAD`, `REVERT_HEAD` or `REBASE_HEAD`. Only works
>>> +    when the index has unmerged entries.
>>
>> Do you know what "and don't exist on all heads to merge" in the original 
> is referring to? The new text doesn't mention anything that sounds like 
>that but I don't understand what the original was trying to say.
> 
> Yes, it took me a while to understand what that meant. I think it is simply
> describing the range of commits shown. If we substitute "refs" for "commits"
> and switch the order of the sentence, it reads:
> 
>      After a failed merge, show commits that don't exist on all heads to merge
>      and that touch files having a conflict.
> 
> So it's just describing (a bit awkwardly) the HEAD...MERGE_HEAD range.

Ah, that makes sense, thanks for explaining


>>> +    for (i = 0; i < ARRAY_SIZE(other_head); i++)
>>> +        if (!read_ref_full(other_head[i],
>>> +                RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
>>> +                oid, NULL)) {
>>> +            if (is_null_oid(oid))
>>> +                die("%s is a symbolic ref???", other_head[i]);
>>
>> This would benefit from being translated and I think one '?' would suffice 
>> (I'm not sure we even need that - are there other possible causes of a null 
>> oid here?)
> 
> This bit was suggested by Junio upthread in <xmqqzfxa9usx.fsf@gitster.g>.
> I'm not sure if the are other causes of null oid, as I don't know well this
> part of the code.
> I agree that a single '?' would be enough, but I'm not sure about marking
> this for translation, I think maybe this situation would be best handled with
> BUG() ?

I think it would be a bug for git to create MERGE_HEAD as a symbolic ref 
but when we read MERGE_HEAD and find it is a symbolic ref we don't know 
if git created it or some third-party script so I think we should just 
report an error.

Best Wishes

Phillip