Re: [PATCH v3 1/3] factor out refresh_and_write_cache function

[Thomas Gummerer <t.gummerer@gmail.com>]

On 09/11, Johannes Schindelin wrote:
> Hi Thomas,
> 
> On Fri, 6 Sep 2019, Thomas Gummerer wrote:
> > Oops, I didn't realize there was another series in flight that also
> > introduces 'repo_refresh_and_write_index'.  Probably should have done
> > a test merge of this with pu.
> 
> Yep, our patches clash. I would not mind placing my patch series on top
> of yours, provided that you can make a few changes that I need ;-)

Sounds good.  Looking ahead further I don't mind these changes at all!

> > Right, and if gentle is set to false, it avoids writing the index,
> > which seems fine from my perspective.
> 
> This also suggests that it would make sense to avoid
> `LOCK_DIE_ON_ERROR`, _in particular_ because this is supposed to be a
> library function, not just a helper function for a one-shot built-in
> (don't you like how this idea "it is okay to use exit() to clean up
> after us, we don't care" comes back to bite us?).

Yup, returning an error for this definitely makes sense, especially
for future proofing.

> > >  - This version allows to pass pathspec, seen and header_msg, while
> > >    the one in builtin-add-i cannot limit the part of the index
> > >    getting refreshed with pathspec.  It wouldn't be a brain surgery
> > >    to use this version and adjust the caller (there only is one) in
> > >    the builtin-add-i topic.
> >
> > 'pathspec', 'seen' and 'header_msg' are not used in my version either,
> > I just implemented it for completeness and compatibility.  So I'd be
> > fine to do without them.
> 
> Oh, why not keep them? I'd rather keep them and adjust the caller in
> `builtin-add-i`.

Great, I'm happy to keep them.

> > There's two more differences between the versions:
> >
> >  - The version in my series allows passing in write_flags to be passed
> >    to write_locked_index, which is required to convert the callers in
> >    builtin/merge.c.
> 
> I can always pass in 0 as `write_flags`.
> 
> >  - Dscho's version also calls 'repo_read_index_preload()', which I
> >    don't do in mine.  Some callers don't need to do that, so I think it
> >    would be nice to keep that outside of the
> >    'repo_refresh_and_write_index()' function.
> 
> Agreed.
> 
> > I can think of a few ways forward here:
> >
> >  - I incorporate features that are needed for the builtin-add-i series
> >    here, and that is rebased on top of this series.
> 
> I'd prefer this way forward. The `builtin-add-i` patch series is
> evolving more slowly than yours.

Great!  I'll send an updated version of my series soon.  Thanks!