Re: [PATCH v4 1/3] refs.c: split writing and commiting a ref into two separate functions
See my comment to your cover letter where I suggest using ref
transactions instead of making callers deal with even more of the
details of updating references. But I will comment on these patches
anyway, in case you'd rather leave them closer to the current form.
On 04/14/2014 08:29 PM, Ronnie Sahlberg wrote:
> Change the function write_ref_sha1() to just write the ref but not
> commit the ref or the lockfile.
> Add a new function commit_ref_lock() that will commit the change done by
> a previous write_ref_sha1().
> Update all callers of write_ref_sha1() to call commit_ref_lock().
>
> The new pattern for updating a ref is now :
>
> lock = lock_ref_sha1_basic() (or varient of)
s/varient/variant/
> write_ref_sha1(lock)
> unlock_ref(lock) | commit_ref_lock(lock)
>
> Once write_ref_sha1() returns, the new ref has been written and the lock
> file has been closed.
> At that stage we can then either call unlock_ref() which will abort the
> update and delete the lock file withouth applying it, or call
You need a comma after "unlock_ref()".
s/withouth/without/
> commit_ref_lock() which will rename the lock file onto the ref file.
The commit message would be easier to read with better formatting; maybe
---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---
refs.c: split writing and commiting a ref into two separate functions
* Change the function write_ref_sha1() to just write the ref but not
commit the ref or the lockfile.
* Add a new function commit_ref_lock() that will commit the change done by
a previous write_ref_sha1().
* Update all callers of write_ref_sha1() to call commit_ref_lock().
The new pattern for updating a ref is now :
lock = lock_ref_sha1_basic() (or variant of)
write_ref_sha1(lock)
unlock_ref(lock) | commit_ref_lock(lock)
Once write_ref_sha1() returns, the new ref has been written and the lock
file has been closed. At that stage we can then either call unlock_ref(),
which will abort the update and delete the lock file without applying it,
or call commit_ref_lock() which will rename the lock file onto the ref
file.
Signed-off-by: Ronnie Sahlberg
---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---
> Signed-off-by: Ronnie Sahlberg
> ---
> 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 | 18 --
> refs.c | 41 +++--
> refs.h | 4
> sequencer.c| 4
> walker.c | 4
> 11 files changed, 92 insertions(+), 17 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 660097b..903ea75 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -304,9 +304,15 @@ void create_branch(const char *head,
> if (real_ref && track)
> setup_tracking(ref.buf + 11, real_ref, track, quiet);
>
> - if (!dont_change_ref)
> - if (write_ref_sha1(lock, sha1, msg) < 0)
> + if (!dont_change_ref) {
> + if (write_ref_sha1(lock, sha1, msg) < 0) {
> + unlock_ref(lock);
> die_errno(_("Failed to write ref"));
> + }
> + if (commit_ref_lock(lock) < 0) {
> + die_errno(_("Failed to commit ref"));
> + }
> + }
>
> strbuf_release(&ref);
> free(real_ref);
There are a lot of changes like this with similar duplicated error
handling. Why not define a helper function like
write_ref_sha1_and_commit() that does what the old write_ref_sha1() used
to do (for the callers who don't care about updating multiple references
at once)?
In fact, I would recommend renaming the function in a preparatory
commit, to reduce the amount of code churn in the second commit where
you extract the two new separate functions.
> diff --git a/builtin/commit.c b/builtin/commit.c
> index d9550c5..3d8a3a8 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1686,9 +1686,14 @@ int cmd_commit(int argc, const char **argv, const char
> *prefix)
> die(_("cannot lock HEAD ref"));
> }
> if (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {
> + unlock_ref(ref_lock);
> rollback_index_files();
> die(_("cannot update HEAD ref"));
> }
> + if (commit_ref_lock(ref_lock) < 0) {
> + rollback_index_files();
> + die(_("cannot commit HEAD ref"));
> + }
>
> unlink(git_path("CHERRY_PICK_HEAD"));
> unlink(git_path("REVERT_HEAD"));
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 55f457c..ebfb854 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -388,7 +388,12 @@ static int s_update_ref(const char *action,
> if (!lock)
> return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
[PATCH v4 1/3] refs.c: split writing and commiting a ref into two separate functions
Change the function write_ref_sha1() to just write the ref but not
commit the ref or the lockfile.
Add a new function commit_ref_lock() that will commit the change done by
a previous write_ref_sha1().
Update all callers of write_ref_sha1() to call commit_ref_lock().
The new pattern for updating a ref is now :
lock = lock_ref_sha1_basic() (or varient of)
write_ref_sha1(lock)
unlock_ref(lock) | commit_ref_lock(lock)
Once write_ref_sha1() returns, the new ref has been written and the lock
file has been closed.
At that stage we can then either call unlock_ref() which will abort the
update and delete the lock file withouth applying it, or call
commit_ref_lock() which will rename the lock file onto the ref file.
Signed-off-by: Ronnie Sahlberg
---
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 | 18 --
refs.c | 41 +++--
refs.h | 4
sequencer.c| 4
walker.c | 4
11 files changed, 92 insertions(+), 17 deletions(-)
diff --git a/branch.c b/branch.c
index 660097b..903ea75 100644
--- a/branch.c
+++ b/branch.c
@@ -304,9 +304,15 @@ void create_branch(const char *head,
if (real_ref && track)
setup_tracking(ref.buf + 11, real_ref, track, quiet);
- if (!dont_change_ref)
- if (write_ref_sha1(lock, sha1, msg) < 0)
+ if (!dont_change_ref) {
+ if (write_ref_sha1(lock, sha1, msg) < 0) {
+ unlock_ref(lock);
die_errno(_("Failed to write ref"));
+ }
+ if (commit_ref_lock(lock) < 0) {
+ die_errno(_("Failed to commit ref"));
+ }
+ }
strbuf_release(&ref);
free(real_ref);
diff --git a/builtin/commit.c b/builtin/commit.c
index d9550c5..3d8a3a8 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1686,9 +1686,14 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
die(_("cannot lock HEAD ref"));
}
if (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {
+ unlock_ref(ref_lock);
rollback_index_files();
die(_("cannot update HEAD ref"));
}
+ if (commit_ref_lock(ref_lock) < 0) {
+ rollback_index_files();
+ die(_("cannot commit HEAD ref"));
+ }
unlink(git_path("CHERRY_PICK_HEAD"));
unlink(git_path("REVERT_HEAD"));
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 55f457c..ebfb854 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -388,7 +388,12 @@ static int s_update_ref(const char *action,
if (!lock)
return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
STORE_REF_ERROR_OTHER;
- if (write_ref_sha1(lock, ref->new_sha1, msg) < 0)
+ if (write_ref_sha1(lock, ref->new_sha1, msg) < 0) {
+ unlock_ref(lock);
+ return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
+ STORE_REF_ERROR_OTHER;
+ }
+ if (commit_ref_lock(lock) < 0)
return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
STORE_REF_ERROR_OTHER;
return 0;
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c323081..4760274 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -587,8 +587,12 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
return "failed to lock";
}
if (write_ref_sha1(lock, new_sha1, "push")) {
+ unlock_ref(lock);
return "failed to write"; /* error() already called */
}
+ if (commit_ref_lock(lock))
+ return "failed to commit"; /* error() already called */
+
return NULL; /* good */
}
}
diff --git a/builtin/replace.c b/builtin/replace.c
index b62420a..c09ff49 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -160,8 +160,12 @@ static int replace_object(const char *object_ref, const
char *replace_ref,
lock = lock_any_ref_for_update(ref, prev, 0, NULL);
if (!lock)
die("%s: cannot lock the ref", ref);
- if (write_ref_sha1(lock, repl, NULL) < 0)
+ if (write_ref_sha1(lock, repl, NULL) < 0) {
+ unlock_ref(lock);
die("%s: cannot update the ref", ref);
+ }
+ if (commit_ref_lock(lock) < 0)
+ die("%s: cannot commit the ref", ref);
return 0;
}
diff --git a/builtin/tag.c b/builtin/tag.c
index 40356e3..8653a64 100644
--- a/builtin/tag.c
+++ b/bu
