Re: [PATCH 0/3] More add_submodule_odb() cleanup in merge code

[Jonathan Tan <jonathantanmy@google.com>]

> On Wed, Sep 8, 2021 at 11:18 AM Jonathan Tan <jonathantanmy@google.com> wrote:
> >
> > (CC-ing Elijah in case he has insight into the merge-ort part.)
> 
> All the submodule merging related functions were lifted from
> merge-recursive.c and minimally modified to fit the new structure.
> The only substantive change I made was to fix the merge result for
> virtual merge bases, but that's like one or two lines of code.  In
> particular, everything relating to submodule objects was totally
> untouched...and I think that's reflected in the fact that your PATCH 2
> basically is the same patch twice, once for merge-recursive and once
> for merge-ort.
> 
> I read over PATCH 2 and I didn't find anything that looked
> problematic, but I'm not up-to-speed on the add_submodule_odb and repo
> handling bits of the codebase so I'm not sure I would catch anything.
> But I am encouraged by the fact that it looks like you did the same
> stuff to merge-recursive and merge-ort; I'd be worried you missed
> something if that weren't the case.

Thanks for taking a look.

> As a sidenote, though...
> 
> This does remind me that I noticed that the following functions from
> object-store.h do not take an explicit repository:
> 
> write_object_file()
> hash_object_file()
> hash_object_file_literally()
> force_object_loose()
> 
> I have a patch sitting around somewhere (possibly only still
> referenced in my 'temp' branch) to make repo_*() variants of the above
> functions, and turn the above into simple wrappers of the repo_*()
> variants which just pass the_repository (much like someone else did
> with read_object_file() and repo_read_object_file()).  It also updates
> merge-ort to use the new repo_*() functions.  However, I ended up
> excluding it from my merge-ort submissions since it wasn't necessary.
> Would this be of interest in your submodule work, though?  I guess
> it'd only matter if we started doing real merges of submodules as part
> of a parent repo merge.  (As opposed to the fast-forward-only merges
> that merge-recursive/merge-ort do right now for submodules.)

I think that these functions would be useful if we were to write into
submodules (for example, in a real merge of submodules, as you said).
Right now my submodule work is just making it support partial clones, so
I'm not planning to introduce any new submodule writes. (Well, besides
the fact that a lazy-fetch in a submodule would end up with new objects,
but that's already handled because we do the fetch in a separate process
in the submodule's gitdir.)