All of lore.kernel.org
 help / color / mirror / Atom feed
From: Michael Haggerty <mhagger@alum.mit.edu>
To: Junio C Hamano <gitster@pobox.com>
Cc: Stefan Beller <sbeller@google.com>, Jeff King <peff@peff.net>,
	git@vger.kernel.org, Michael Haggerty <mhagger@alum.mit.edu>
Subject: [PATCH v3 00/19] Improve "refs" encapsulation and speed up deletes
Date: Mon, 22 Jun 2015 16:02:51 +0200	[thread overview]
Message-ID: <cover.1434980615.git.mhagger@alum.mit.edu> (raw)

This is v3 of the series. I think I have addressed all of the feedback
from v1 [1] and v2 [2]. Thanks to Stefan, Junio, and Peff for their
feedback about v2.

There are three significant changes since v2:

* Add a patch

      delete_refs(): bail early if the packed-refs file cannot be rewritten

  that changes delete_refs() to bail early if repack_without_refs()
  fails. See the log message for the explanation.

* Add a patch

      prune_refs(): use delete_refs()

  that changes "git fetch --prune" to use delete_refs() as well. This
  is analogous to the existing patch

      prune_remote(): use delete_refs()

  (Both of these changes remove quadratic behavior that can be
  extremely expensive in repositories with many packed refs.)

* Append four more commits that change how delete_ref() interprets its
  `old_sha1` parameter, to make it consistent with
  ref_transaction_delete() and friends:

      check_branch_commit(): make first parameter const
      update_ref(): don't read old reference value before delete
      cmd_update_ref(): make logic more straightforward
      delete_ref(): use the usual convention for old_sha1

  This change was suggested by Junio [3].

The last change is good for consistency, but less important than
expected in the real world. The reason is that the convention of
ref_transaction_delete() is that passing NULL_SHA1 as old_sha1 is a
bug (because why would somebody delete a reference that he knows not
to exist?) So we don't really gain a useful third case, though we do
gain a consistency check that might be useful someday. However, if you
don't like this change, the last four commits can be dropped
(actually, commits N-3 through N-1 are marginally useful cleanups
anyway so probably only the last commit should be dropped in this
case).

Other, minor changes since v2:

* Improve the commit message of "delete_refs(): make error message
  more generic".

* Better explain the change to error output caused by "prune_remote():
  use delete_refs()" in that commit's log message.

* Split the "add names for function parameters" changes into a
  separate patch.

* Fix how die() is invoked in "initial_ref_transaction_commit():
  function for initial ref creation" and remove some superfluous
  braces.

This series is also available from my GitHub account [4] as branch
"init-delete-refs-api".

[1] http://thread.gmane.org/gmane.comp.version-control.git/271017
[2] http://thread.gmane.org/gmane.comp.version-control.git/271552
[3] http://article.gmane.org/gmane.comp.version-control.git/271697
[4] https://github.com/mhagger/git

Michael Haggerty (19):
  delete_ref(): move declaration to refs.h
  remove_branches(): remove temporary
  delete_ref(): handle special case more explicitly
  delete_refs(): new function for the refs API
  delete_refs(): make error message more generic
  delete_refs(): bail early if the packed-refs file cannot be rewritten
  prune_remote(): use delete_refs()
  prune_refs(): use delete_refs()
  repack_without_refs(): make function private
  initial_ref_transaction_commit(): function for initial ref creation
  refs: remove some functions from the module's public interface
  initial_ref_transaction_commit(): check for duplicate refs
  initial_ref_transaction_commit(): check for ref D/F conflicts
  refs: move the remaining ref module declarations to refs.h
  refs.h: add some parameter names to function declarations
  check_branch_commit(): make first parameter const
  update_ref(): don't read old reference value before delete
  cmd_update_ref(): make logic more straightforward
  delete_ref(): use the usual convention for old_sha1

 archive.c               |   1 +
 builtin/blame.c         |   1 +
 builtin/branch.c        |   5 +-
 builtin/clone.c         |  18 ++++-
 builtin/fast-export.c   |   1 +
 builtin/fetch.c         |  25 ++++--
 builtin/fmt-merge-msg.c |   1 +
 builtin/init-db.c       |   1 +
 builtin/log.c           |   1 +
 builtin/remote.c        |  33 +-------
 builtin/update-ref.c    |  21 ++++-
 cache.h                 |  68 ----------------
 fast-import.c           |   6 +-
 refs.c                  | 182 ++++++++++++++++++++++++++++++++++++++---
 refs.h                  | 211 +++++++++++++++++++++++++++++++-----------------
 remote-testsvn.c        |   1 +
 16 files changed, 371 insertions(+), 205 deletions(-)

-- 
2.1.4

             reply	other threads:[~2015-06-22 14:03 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-06-22 14:02 Michael Haggerty [this message]
2015-06-22 14:02 ` [PATCH v3 01/19] delete_ref(): move declaration to refs.h Michael Haggerty
2015-06-22 14:02 ` [PATCH v3 02/19] remove_branches(): remove temporary Michael Haggerty
2015-06-22 14:02 ` [PATCH v3 03/19] delete_ref(): handle special case more explicitly Michael Haggerty
2015-06-22 14:02 ` [PATCH v3 04/19] delete_refs(): new function for the refs API Michael Haggerty
2015-06-22 14:02 ` [PATCH v3 05/19] delete_refs(): make error message more generic Michael Haggerty
2015-06-22 14:02 ` [PATCH v3 06/19] delete_refs(): bail early if the packed-refs file cannot be rewritten Michael Haggerty
2015-06-22 14:02 ` [PATCH v3 07/19] prune_remote(): use delete_refs() Michael Haggerty
2015-06-22 14:02 ` [PATCH v3 08/19] prune_refs(): " Michael Haggerty
2015-06-22 14:03 ` [PATCH v3 09/19] repack_without_refs(): make function private Michael Haggerty
2015-06-22 14:03 ` [PATCH v3 10/19] initial_ref_transaction_commit(): function for initial ref creation Michael Haggerty
2015-06-22 14:03 ` [PATCH v3 11/19] refs: remove some functions from the module's public interface Michael Haggerty
2015-06-22 14:03 ` [PATCH v3 12/19] initial_ref_transaction_commit(): check for duplicate refs Michael Haggerty
2015-06-22 21:06   ` Junio C Hamano
2015-06-23  7:11     ` Michael Haggerty
2015-06-23 17:44       ` Junio C Hamano
2015-06-22 14:03 ` [PATCH v3 13/19] initial_ref_transaction_commit(): check for ref D/F conflicts Michael Haggerty
2015-06-22 14:03 ` [PATCH v3 14/19] refs: move the remaining ref module declarations to refs.h Michael Haggerty
2015-06-22 14:03 ` [PATCH v3 15/19] refs.h: add some parameter names to function declarations Michael Haggerty
2015-06-22 14:03 ` [PATCH v3 16/19] check_branch_commit(): make first parameter const Michael Haggerty
2015-06-22 14:03 ` [PATCH v3 17/19] update_ref(): don't read old reference value before delete Michael Haggerty
2015-06-22 14:03 ` [PATCH v3 18/19] cmd_update_ref(): make logic more straightforward Michael Haggerty
2015-06-22 14:03 ` [PATCH v3 19/19] delete_ref(): use the usual convention for old_sha1 Michael Haggerty
2015-06-22 21:10   ` Junio C Hamano

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=cover.1434980615.git.mhagger@alum.mit.edu \
    --to=mhagger@alum.mit.edu \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    --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.