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

Thomas Gummerer [off-list ref] writes:

On 09/11, Johannes Schindelin wrote:

quoted

Hi Thomas,

On Fri, 6 Sep 2019, Thomas Gummerer wrote:

quoted

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!

quoted

quoted

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.

quoted

quoted

quoted

 - 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.

quoted

quoted

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`.

quoted

 - 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.

quoted

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!

I just read the conclusion you two reached (after being down and
offline for two days) and found the reasoning totally sensible.

Thanks, both of you, for working well together.