From: Jonathan Nieder <jrnieder@gmail.com>
To: Michael Haggerty <mhagger@alum.mit.edu>
Cc: Stefan Beller <sbeller@google.com>,
ronniesahlberg@gmail.com, gitster@pobox.com, git@vger.kernel.org
Subject: Re: [PATCHv3 00/13] the refs-transactions-reflog series
Date: Thu, 4 Dec 2014 10:14:04 -0800 [thread overview]
Message-ID: <20141204181404.GD9992@google.com> (raw)
In-Reply-To: <54809577.4080302@alum.mit.edu>
Michael Haggerty wrote:
> I am still unhappy with the approach of this series, for the reasons
> that I explained earlier [1]. In short, I think that the abstraction
> level is wrong. In my opinion, consumers of the refs API should barely
> even have to *know* about reflogs, let alone implement reflog expiration
> themselves.
Okay, now returning to the substance of this comment. This is
revisiting themes from [3], so my opinions are probably not a
surprise.
I think that the API changes that you and Stefan are proposing are
consistent and could both go in.
You suggested refactoring expire_reflogs() to use a callback that
decides what to expire. Then it doesn't have to care that the
expiration happens by creating a new reflog and copying over the
reflog entries that are not being expired. The result is a clean
reflog expiration API.
The ref-transaction-reflog series allows those low-level steps to be
part of a ref transaction. Any ref backend (the current files-based
backend or a future other one) would get a chance to reimplement those
low-level steps, which are part of what happens during ref updates and
reflog deletion. The goal is for all reflog updates to use the
transaction API, so that new ref/reflog backends only need to
implement the transaction functions.
So *both* are making good changes, with different goals.
The implementation of the reflog expiration API can use the ref
transaction API.
> Of course, reflog expiration *should* be done atomically. But that is
> the business of the refs module; callers shouldn't have to do all the
> complicated work of building the transaction themselves.
I don't understand this comment. After the ref-transaction-reflog
series, a transaction_update_ref() call still takes care of the
corresponding reflog update without the caller having to worry about
it.
Thanks for looking it over,
Jonathan
> [1] http://thread.gmane.org/gmane.comp.version-control.git/259712/focus=259770
[3] http://thread.gmane.org/gmane.comp.version-control.git/259939/focus=259967
next prev parent reply other threads:[~2014-12-04 18:14 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-12-04 8:29 [PATCHv3 00/13] the refs-transactions-reflog series Stefan Beller
2014-12-04 8:29 ` [PATCH 01/13] refs.c: make ref_transaction_create a wrapper for ref_transaction_update Stefan Beller
2014-12-04 8:29 ` [PATCH 02/13] refs.c: make ref_transaction_delete " Stefan Beller
2014-12-04 8:29 ` [PATCH 03/13] refs.c: add a function to append a reflog entry to a fd Stefan Beller
2014-12-04 8:29 ` [PATCH 04/13] refs.c: rename the transaction functions Stefan Beller
2014-12-04 8:29 ` [PATCH 05/13] refs.c: rename transaction.updates to transaction.ref_updates Stefan Beller
2014-12-04 8:29 ` [PATCH 06/13] refs.c: add a transaction function to truncate or append a reflog entry Stefan Beller
2014-12-04 8:29 ` [PATCH 07/13] reflog.c: use a reflog transaction when writing during expire Stefan Beller
2014-12-04 8:29 ` [PATCH 08/13] refs.c: rename log_ref_setup to create_reflog Stefan Beller
2014-12-04 8:29 ` [PATCH 09/13] refs.c: remove unlock_ref/close_ref/commit_ref from the refs api Stefan Beller
2014-12-04 8:29 ` [PATCH 10/13] refs.c: remove lock_any_ref_for_update Stefan Beller
2014-12-04 8:29 ` [PATCH 11/13] refs.c: don't expose the internal struct ref_lock in the header file Stefan Beller
2014-12-04 8:29 ` [PATCH 12/13] refs.c: use a bit for ref_update have_old Stefan Beller
2014-12-04 16:10 ` Torsten Bögershausen
2014-12-04 17:00 ` Andreas Schwab
2014-12-04 8:29 ` [PATCH 13/13] refs.c: allow deleting refs with a broken sha1 Stefan Beller
2014-12-04 17:10 ` [PATCHv3 00/13] the refs-transactions-reflog series Michael Haggerty
2014-12-04 17:53 ` Jonathan Nieder
2014-12-04 18:14 ` Jonathan Nieder [this message]
2014-12-04 18:32 ` Stefan Beller
2014-12-04 21:13 ` Michael Haggerty
2014-12-04 18:37 ` Junio C Hamano
2014-12-04 18:41 ` Junio C Hamano
2014-12-04 18:49 ` Stefan Beller
2014-12-04 19:27 ` Jonathan Nieder
Reply instructions:
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:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20141204181404.GD9992@google.com \
--to=jrnieder@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=mhagger@alum.mit.edu \
--cc=ronniesahlberg@gmail.com \
--cc=sbeller@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* 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.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.