Thomas Gummerer <t.gummerer@xxxxxxxxx> 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.
