Re: [PATCH v3 5/8] submodule: return target of submodule symref

From: Glen Choo <chooglen@google.com>
Date: Oct 28, 2022, 23:11 UTC
Message-ID: <kl6l8rkzczlt.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqqfsf7hb56.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 29 quoted lines

> "Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>>  int resolve_gitlink_ref(const char *submodule, const char *refname,
>> -			struct object_id *oid)
>> +			struct object_id *oid, const char **target_out)
>>  {
>>  	struct ref_store *refs;
>>  	int flags;
>> +	const char *target;
>>  
>>  	refs = get_submodule_ref_store(submodule);
>>  
>>  	if (!refs)
>>  		return -1;
>> -
>> -	if (!refs_resolve_ref_unsafe(refs, refname, 0, oid, &flags) ||
>> -	    is_null_oid(oid))
>> +	target = refs_resolve_ref_unsafe(refs, refname, 0, oid, &flags);
>> +	if (!target || is_null_oid(oid))
>>  		return -1;
>> +	if (target_out)
>> +		*target_out = target;
>>  	return 0;
>>  }
>
> Please remind me why we call this underlying helper _unsafe()?
>
> Isn't it because we return a temporary buffer  (static strbuf),
> whose contents is not permanent?

8cad4744ee (Rename resolve_ref() to resolve_ref_unsafe(), 2011-12-12) seems to suggest so.

(For some reason, I thought it was *_unsafe() because it would die(), but it obviously doesn't.)

> I am wondering if we should force the callers who care enough to
> pass the optional **target_out to xstrdup() the result.

Yes, makes sense. We even have the *_refdup() helper for that.