Re: [PATCH v3 4/4] tmp-objdir: merge loose object index when migrating tmpdir

[Justin Tobler <jltobler@gmail.com>]

On 26/10/30 08:55PM, Karthik Nayak wrote:
> Justin Tobler <jltobler@gmail.com> writes:
> 
> > diff --git a/loose.c b/loose.c
> > index f2e111c25e..0ce9c7486e 100644
> > --- a/loose.c
> > +++ b/loose.c
> > @@ -311,3 +311,50 @@ void loose_object_map_clear(struct loose_object_map **map)
> >  	free(m);
> >  	*map = NULL;
> >  }
> > +
> > +int loose_object_idx_merge(struct repository *repo, const char *src,
> > +			   const char *dst)
> >
> 
> Nit: should we name this `loose_object_map_merge()` to stay consistent
> with the other `loose_object_map...()` functions?

I specifically did not call it `loose_object_map_merge()` because,
unlike the other similarly named functions, this one does not update the
in-memory loose object map for the object source. Here we are only
merging the "loose-object-idx" files.

> > +	if (loose_object_map_write(repo, map, fd) < 0) {
> > +		ret = error_errno(_("unable to write %s"),
> > +				  get_lock_file_path(&lock));
> > +		goto out;
> > +	}
> > +
> > +	if (commit_lock_file(&lock) < 0) {
> > +		ret = error_errno(_("unable to write %s"), dst);
> 
> Nit: Shouldn't this be unable to commit/rename? This will differentiate
> from the above write error

I choose "write" here because logically we are trying atomically write
the final "loose-object-idx" file. IMO, I think the difference in
filepath should be sufficient to differentiate between the two as well.
Ultimately though, I don't feel strongly either way about this and am
not sure it's worth a reroll, so I'm inclined to leave it as-is.

-Justin