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!
