Re: [PATCH 04/11] submodule.c: convert submodule_move_head new argument to object id

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> All callers use oid_to_hex to convert the desired oid to a string before
> calling submodule_move_head. Defer the conversion to the
> submodule_move_head as it will turn out to be useful in a bit.

I would think this is a good change even without "as it will turn
out..." which readers do not have enough information to judge for
themselves at this point.

> diff --git a/submodule.c b/submodule.c
> index 50cbf5f13ed..da2ed8696f5 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1597,9 +1597,9 @@ static void submodule_reset_index(const char *path)
>   * pass NULL for old or new respectively.
>   */
>  int submodule_move_head(const char *path,
> -			 const char *old_head,
> -			 const char *new_head,
> -			 unsigned flags)
> +			const char *old_head,
> +			const struct object_id *new_oid,
> +			unsigned flags)

The new calling convention feels somewhat uneven, though, that "new"
(does it mean "switching to this commit object"?) takes an object
id, but "old" ("switching from this commit object"?) still wants a
textual representation.

So, I no longer am sure that this is a good change by itself.  It
would be a good change by itself if we deferred _both_ to keep them
in sync (otherwise those who write (or read) callers will be forced
to wonder which one takes the object id and which one takes the
textual representation and why they need to remember the
difference).

Perhaps not all callers use oid_to_hex() to come up with old_head,
and some may even use branch names, etc., which is passed through
from this function to its callee, to convey more information than
mere object names?  If that is the case, then converting old_head to
old_oid would be a bad move as it can lose information and we'd need
to reconsider the strategy used here (e.g. perhaps we may be better
off sending both textual name and object id down the callchain, and
a caller without a meaningful textual name may indicate that by
passing NULL, or something like that).

> @@ -1865,8 +1863,7 @@ static int merged_entry(const struct cache_entry *ce,
>  
>  		if (submodule_from_ce(ce)) {
>  			int ret = check_submodule_move_head(ce, oid_to_hex(&old->oid),
> -							    oid_to_hex(&ce->oid),
> -							    o);
> +							    &ce->oid, o);
>  			if (ret)
>  				return ret;
>  		}