Re: [PATCH 0/4] Make update_refs more atomic V2

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 10, 2014, 18:53 UTC
Message-ID: <xmqq38hlxbh3.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1397154625-11884-1-git-send-email-sahlberg@google.com>

CC'ing Michael who has been active in this area as an area expert.

Ronnie, please make it a habit to run something like

    $ git shortlog --no-merges --since=18.months <affected paths>...

to help you decide who your series may want to be reviewed by, before sending them.

Thanks.

Ronnie Sahlberg <sahlberg@google.com> writes:

Show 56 quoted lines

> refs.c:update_refs() intermingles doing updates and checks with actually
> applying changes to the refs in loops that abort on error.
> This is done one ref at a time and means that if an error is detected that
> will fail the operation after only some of the ref operations have been
> been updated on the disk.
>
> These patches change the update and delete functions to use a three
> call pattern of
>
> 1, lock
> 2, update, or flag for deletion
> 3, apply on disk
>
> In the final patch I change update_refs to perform these actions in three
> separate loops where the final loop to 'apply on disk' all the changes will
> only be performed if there were no error conditions detected during any of
> previous loops.
>
> This should make the changes of refs in update_refs slightly more atomic.
>
>
> This may overlap with other current patch series for making refs updates
> more atomic which may mean these patches become obsolete, but I would still
> like some review and feedback on these changes.
>
> Version 2:
> Updates and fixes based on Junio's feedback.
> * Fix the subject line for patches so they comply with the project standard.
> * Redo the update/delete loops so that we maintain the correct order of
>   operations. Perform all updates first, then perform the deletes.
> * Add an additional patch that allows us to do the update/delete in the correct
>   order from within a single loop by first sorting the refs so that deletes
>   are after all non-deletes.
>
>
> Ronnie Sahlberg (4):
>   refs.c: split writing and commiting a ref into two separate functions
>   refs.c: split delete_ref_loose() into a separate flag-for-deletion and
>     commit phase
>   refs.c: change update_refs to run the commit loops once all work is
>     finished
>   refs.c: sort the refs by new_sha1 and merge the two update/delete
>     loops into one
>
>  branch.c               |  10 ++++-
>  builtin/commit.c       |   5 +++
>  builtin/fetch.c        |   7 +++-
>  builtin/receive-pack.c |   4 ++
>  builtin/replace.c      |   6 ++-
>  builtin/tag.c          |   6 ++-
>  fast-import.c          |   7 +++-
>  refs.c                 | 102 +++++++++++++++++++++++++++++++++----------------
>  refs.h                 |   6 +++
>  sequencer.c            |   4 ++
>  walker.c               |   4 ++
>  11 files changed, 123 insertions(+), 38 deletions(-)