From: Junio C Hamano <firstname.lastname@example.org>
To: Thomas Gummerer <email@example.com>
Cc: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
firstname.lastname@example.org, "Joel Teichroeb" <email@example.com>,
"Jeff King" <firstname.lastname@example.org>,
"Martin Ågren" <email@example.com>
Subject: Re: [PATCH v3 1/3] factor out refresh_and_write_cache function
Date: Thu, 12 Sep 2019 09:46:37 -0700 [thread overview]
Message-ID: <firstname.lastname@example.org> (raw)
In-Reply-To: <20190911175201.GA11444@cat> (Thomas Gummerer's message of "Wed, 11 Sep 2019 18:52:01 +0100")
Thomas Gummerer <email@example.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
> 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.
>> > 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.
next prev parent reply other threads:[~2019-09-12 16:46 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-27 10:14 [PATCH 0/3] make sure stash refreshes the index properly Thomas Gummerer
2019-08-27 10:14 ` [PATCH 1/3] factor out refresh_and_write_cache function Thomas Gummerer
2019-08-28 15:49 ` Martin Ågren
2019-08-29 17:59 ` Thomas Gummerer
2019-08-27 10:14 ` [PATCH 2/3] merge: use refresh_and_write_cache Thomas Gummerer
2019-08-28 15:52 ` Martin Ågren
2019-08-29 18:00 ` Thomas Gummerer
2019-08-27 10:14 ` [PATCH 3/3] stash: make sure to write refreshed cache Thomas Gummerer
2019-08-29 18:27 ` [PATCH v2 0/3] make sure stash refreshes the index properly Thomas Gummerer
2019-08-29 18:27 ` [PATCH v2 1/3] factor out refresh_and_write_cache function Thomas Gummerer
2019-08-30 15:07 ` Martin Ågren
2019-08-30 17:06 ` Junio C Hamano
2019-09-02 17:15 ` Thomas Gummerer
2019-09-03 17:43 ` Junio C Hamano
2019-08-29 18:27 ` [PATCH v2 2/3] merge: use refresh_and_write_cache Thomas Gummerer
2019-08-29 18:27 ` [PATCH v2 3/3] stash: make sure to write refreshed cache Thomas Gummerer
2019-09-03 19:10 ` [PATCH v3 0/3] make sure stash refreshes the index properly Thomas Gummerer
2019-09-03 19:10 ` [PATCH v3 1/3] factor out refresh_and_write_cache function Thomas Gummerer
2019-09-05 22:00 ` Junio C Hamano
2019-09-06 14:18 ` Thomas Gummerer
2019-09-11 10:57 ` Johannes Schindelin
2019-09-11 17:52 ` Thomas Gummerer
2019-09-12 16:46 ` Junio C Hamano [this message]
2019-09-03 19:10 ` [PATCH v3 2/3] merge: use refresh_and_write_cache Thomas Gummerer
2019-09-03 19:10 ` [PATCH v3 3/3] stash: make sure to write refreshed cache Thomas Gummerer
2019-09-11 18:20 ` [PATCH v4 0/3] make sure stash refreshes the index properly Thomas Gummerer
2019-09-11 18:20 ` [PATCH v4 1/3] factor out refresh_and_write_cache function Thomas Gummerer
2019-09-11 18:20 ` [PATCH v4 2/3] merge: use refresh_and_write_cache Thomas Gummerer
2019-09-11 18:20 ` [PATCH v4 3/3] stash: make sure to write refreshed cache Thomas Gummerer
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).