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

[Junio C Hamano <gitster@pobox.com>]

Thomas Gummerer <t.gummerer@gmail.com> writes:

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

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.