Re: [PATCH 04/11] submodule.c: convert submodule_move_head new argument to object id
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 6, 2018, 17:31 UTC
- Message-ID
- <xmqqmusu1qj9.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20180904230149.180332-5-sbeller@google.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.
Show 14 quoted lines
> 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).
Show 10 quoted lines
> @@ -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;
> }