* [PATCH 00/13] Improve "refs" module encapsulation
@ 2015-06-08 11:45 Michael Haggerty
2015-06-08 11:45 ` [PATCH 01/13] delete_ref(): move declaration to refs.h Michael Haggerty
` (14 more replies)
0 siblings, 15 replies; 30+ messages in thread
From: Michael Haggerty @ 2015-06-08 11:45 UTC (permalink / raw
To: Junio C Hamano; +Cc: Stefan Beller, Jeff King, git, Michael Haggerty
Add functions to the reference API to
* Delete a bunch of references at once, but *without* failing the
whole transaction if one of the deletions fails. This functionality
is used by `git remote remove` and `git remote prune`.
* Create initial references during `git clone`. (During clone,
references are written directly to the `packed-refs` file without
any locking.)
Also move the remaining "refs" function declarations from `cache.h` to
`refs.h`.
This improves the encapsulation of the refs module. Especially, it
means that code outside of the refs module should no longer need to
care about the distinction between loose and packed references.
These patches are also available from my GitHub account [1] as branch
"init-delete-refs-api".
[1] https://github.com/mhagger/git
Michael Haggerty (13):
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(): improve error message
delete_refs(): convert error message to lower case
prune_remote(): 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
archive.c | 1 +
builtin/blame.c | 1 +
builtin/clone.c | 19 ++++-
builtin/fast-export.c | 1 +
builtin/fmt-merge-msg.c | 1 +
builtin/init-db.c | 1 +
builtin/log.c | 1 +
builtin/remote.c | 33 +-------
cache.h | 68 ----------------
refs.c | 167 +++++++++++++++++++++++++++++++++++---
refs.h | 210 +++++++++++++++++++++++++++++++-----------------
remote-testsvn.c | 1 +
12 files changed, 316 insertions(+), 188 deletions(-)
--
2.1.4
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 01/13] delete_ref(): move declaration to refs.h
2015-06-08 11:45 [PATCH 00/13] Improve "refs" module encapsulation Michael Haggerty
@ 2015-06-08 11:45 ` Michael Haggerty
2015-06-08 16:43 ` Stefan Beller
2015-06-08 11:45 ` [PATCH 02/13] remove_branches(): remove temporary Michael Haggerty
` (13 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Michael Haggerty @ 2015-06-08 11:45 UTC (permalink / raw
To: Junio C Hamano; +Cc: Stefan Beller, Jeff King, git, Michael Haggerty
Also
* Add a docstring
* Rename the second parameter to "old_sha1", to be consistent with the
convention used elsewhere in the refs module
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
cache.h | 2 --
refs.c | 5 +++--
refs.h | 9 +++++++++
3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/cache.h b/cache.h
index 571c98f..be92121 100644
--- a/cache.h
+++ b/cache.h
@@ -585,8 +585,6 @@ extern void update_index_if_able(struct index_state *, struct lock_file *);
extern int hold_locked_index(struct lock_file *, int);
extern void set_alternate_index_output(const char *);
-extern int delete_ref(const char *, const unsigned char *sha1, unsigned int flags);
-
/* Environment bits from configuration mechanism */
extern int trust_executable_bit;
extern int trust_ctime;
diff --git a/refs.c b/refs.c
index a742d79..b575bb8 100644
--- a/refs.c
+++ b/refs.c
@@ -2789,7 +2789,8 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)
return 0;
}
-int delete_ref(const char *refname, const unsigned char *sha1, unsigned int flags)
+int delete_ref(const char *refname, const unsigned char *old_sha1,
+ unsigned int flags)
{
struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
@@ -2797,7 +2798,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, unsigned int flag
transaction = ref_transaction_begin(&err);
if (!transaction ||
ref_transaction_delete(transaction, refname,
- (sha1 && !is_null_sha1(sha1)) ? sha1 : NULL,
+ (old_sha1 && !is_null_sha1(old_sha1)) ? old_sha1 : NULL,
flags, NULL, &err) ||
ref_transaction_commit(transaction, &err)) {
error("%s", err.buf);
diff --git a/refs.h b/refs.h
index 8c3d433..8785bca 100644
--- a/refs.h
+++ b/refs.h
@@ -202,6 +202,15 @@ extern int read_ref_at(const char *refname, unsigned int flags,
/** Check if a particular reflog exists */
extern int reflog_exists(const char *refname);
+/*
+ * Delete the specified reference. If old_sha1 is non-NULL and not
+ * NULL_SHA1, then verify that the current value of the reference is
+ * old_sha1 before deleting it. flags is passed to
+ * ref_transaction_delete().
+ */
+extern int delete_ref(const char *refname, const unsigned char *old_sha1,
+ unsigned int flags);
+
/** Delete a reflog */
extern int delete_reflog(const char *refname);
--
2.1.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 02/13] remove_branches(): remove temporary
2015-06-08 11:45 [PATCH 00/13] Improve "refs" module encapsulation Michael Haggerty
2015-06-08 11:45 ` [PATCH 01/13] delete_ref(): move declaration to refs.h Michael Haggerty
@ 2015-06-08 11:45 ` Michael Haggerty
2015-06-08 16:45 ` Stefan Beller
2015-06-08 11:45 ` [PATCH 03/13] delete_ref(): handle special case more explicitly Michael Haggerty
` (12 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Michael Haggerty @ 2015-06-08 11:45 UTC (permalink / raw
To: Junio C Hamano; +Cc: Stefan Beller, Jeff King, git, Michael Haggerty
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
builtin/remote.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index f4a6ec9..53b8e13 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -756,8 +756,7 @@ static int remove_branches(struct string_list *branches)
strbuf_release(&err);
for (i = 0; i < branches->nr; i++) {
- struct string_list_item *item = branches->items + i;
- const char *refname = item->string;
+ const char *refname = branches->items[i].string;
if (delete_ref(refname, NULL, 0))
result |= error(_("Could not remove branch %s"), refname);
--
2.1.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 03/13] delete_ref(): handle special case more explicitly
2015-06-08 11:45 [PATCH 00/13] Improve "refs" module encapsulation Michael Haggerty
2015-06-08 11:45 ` [PATCH 01/13] delete_ref(): move declaration to refs.h Michael Haggerty
2015-06-08 11:45 ` [PATCH 02/13] remove_branches(): remove temporary Michael Haggerty
@ 2015-06-08 11:45 ` Michael Haggerty
2015-06-08 16:48 ` Stefan Beller
2015-06-08 11:45 ` [PATCH 04/13] delete_refs(): new function for the refs API Michael Haggerty
` (11 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Michael Haggerty @ 2015-06-08 11:45 UTC (permalink / raw
To: Junio C Hamano; +Cc: Stefan Beller, Jeff King, git, Michael Haggerty
delete_ref() uses a different convention for its old_sha1 parameter
than, say, ref_transaction_delete(): NULL_SHA1 means not to check the
old value. Make this fact a little bit clearer in the code by handling
it in explicit, commented code rather than burying it in a conditional
expression.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index b575bb8..f9d87b6 100644
--- a/refs.c
+++ b/refs.c
@@ -2795,10 +2795,13 @@ int delete_ref(const char *refname, const unsigned char *old_sha1,
struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
+ /* Treat NULL_SHA1 as "don't care" */
+ if (old_sha1 && is_null_sha1(old_sha1))
+ old_sha1 = NULL;
+
transaction = ref_transaction_begin(&err);
if (!transaction ||
- ref_transaction_delete(transaction, refname,
- (old_sha1 && !is_null_sha1(old_sha1)) ? old_sha1 : NULL,
+ ref_transaction_delete(transaction, refname, old_sha1,
flags, NULL, &err) ||
ref_transaction_commit(transaction, &err)) {
error("%s", err.buf);
--
2.1.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 04/13] delete_refs(): new function for the refs API
2015-06-08 11:45 [PATCH 00/13] Improve "refs" module encapsulation Michael Haggerty
` (2 preceding siblings ...)
2015-06-08 11:45 ` [PATCH 03/13] delete_ref(): handle special case more explicitly Michael Haggerty
@ 2015-06-08 11:45 ` Michael Haggerty
2015-06-08 11:45 ` [PATCH 05/13] delete_refs(): improve error message Michael Haggerty
` (10 subsequent siblings)
14 siblings, 0 replies; 30+ messages in thread
From: Michael Haggerty @ 2015-06-08 11:45 UTC (permalink / raw
To: Junio C Hamano; +Cc: Stefan Beller, Jeff King, git, Michael Haggerty
Move the function remove_branches() from builtin/remote.c to refs.c,
rename it to delete_refs(), and make it public.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
builtin/remote.c | 21 +--------------------
refs.c | 19 +++++++++++++++++++
refs.h | 7 +++++++
3 files changed, 27 insertions(+), 20 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index 53b8e13..c8dc724 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -746,25 +746,6 @@ static int mv(int argc, const char **argv)
return 0;
}
-static int remove_branches(struct string_list *branches)
-{
- struct strbuf err = STRBUF_INIT;
- int i, result = 0;
-
- if (repack_without_refs(branches, &err))
- result |= error("%s", err.buf);
- strbuf_release(&err);
-
- for (i = 0; i < branches->nr; i++) {
- const char *refname = branches->items[i].string;
-
- if (delete_ref(refname, NULL, 0))
- result |= error(_("Could not remove branch %s"), refname);
- }
-
- return result;
-}
-
static int rm(int argc, const char **argv)
{
struct option options[] = {
@@ -821,7 +802,7 @@ static int rm(int argc, const char **argv)
strbuf_release(&buf);
if (!result)
- result = remove_branches(&branches);
+ result = delete_refs(&branches);
string_list_clear(&branches, 0);
if (skipped.nr) {
diff --git a/refs.c b/refs.c
index f9d87b6..c413282 100644
--- a/refs.c
+++ b/refs.c
@@ -2814,6 +2814,25 @@ int delete_ref(const char *refname, const unsigned char *old_sha1,
return 0;
}
+int delete_refs(struct string_list *refnames)
+{
+ struct strbuf err = STRBUF_INIT;
+ int i, result = 0;
+
+ if (repack_without_refs(refnames, &err))
+ result |= error("%s", err.buf);
+ strbuf_release(&err);
+
+ for (i = 0; i < refnames->nr; i++) {
+ const char *refname = refnames->items[i].string;
+
+ if (delete_ref(refname, NULL, 0))
+ result |= error(_("Could not remove branch %s"), refname);
+ }
+
+ return result;
+}
+
/*
* People using contrib's git-new-workdir have .git/logs/refs ->
* /some/other/path/.git/logs/refs, and that may live on another device.
diff --git a/refs.h b/refs.h
index 8785bca..9b75b9f 100644
--- a/refs.h
+++ b/refs.h
@@ -211,6 +211,13 @@ extern int reflog_exists(const char *refname);
extern int delete_ref(const char *refname, const unsigned char *old_sha1,
unsigned int flags);
+/*
+ * Delete the specified references. If there are any problems, emit
+ * errors but attempt to keep going (i.e., the deletes are not done in
+ * an all-or-nothing transaction).
+ */
+extern int delete_refs(struct string_list *refnames);
+
/** Delete a reflog */
extern int delete_reflog(const char *refname);
--
2.1.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 05/13] delete_refs(): improve error message
2015-06-08 11:45 [PATCH 00/13] Improve "refs" module encapsulation Michael Haggerty
` (3 preceding siblings ...)
2015-06-08 11:45 ` [PATCH 04/13] delete_refs(): new function for the refs API Michael Haggerty
@ 2015-06-08 11:45 ` Michael Haggerty
2015-06-09 18:47 ` Junio C Hamano
2015-06-08 11:45 ` [PATCH 06/13] delete_refs(): convert error message to lower case Michael Haggerty
` (9 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Michael Haggerty @ 2015-06-08 11:45 UTC (permalink / raw
To: Junio C Hamano; +Cc: Stefan Beller, Jeff King, git, Michael Haggerty
Change the error message from
Could not remove branch %s
to
Could not remove reference %s
This change makes sense even for the existing caller, which uses the
function to delete remote-tracking branches.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index c413282..2a2a06d 100644
--- a/refs.c
+++ b/refs.c
@@ -2827,7 +2827,7 @@ int delete_refs(struct string_list *refnames)
const char *refname = refnames->items[i].string;
if (delete_ref(refname, NULL, 0))
- result |= error(_("Could not remove branch %s"), refname);
+ result |= error(_("Could not remove reference %s"), refname);
}
return result;
--
2.1.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 06/13] delete_refs(): convert error message to lower case
2015-06-08 11:45 [PATCH 00/13] Improve "refs" module encapsulation Michael Haggerty
` (4 preceding siblings ...)
2015-06-08 11:45 ` [PATCH 05/13] delete_refs(): improve error message Michael Haggerty
@ 2015-06-08 11:45 ` Michael Haggerty
2015-06-08 16:51 ` Stefan Beller
2015-06-08 11:45 ` [PATCH 07/13] prune_remote(): use delete_refs() Michael Haggerty
` (8 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Michael Haggerty @ 2015-06-08 11:45 UTC (permalink / raw
To: Junio C Hamano; +Cc: Stefan Beller, Jeff King, git, Michael Haggerty
This string is going to have to be re-internationalized anyway because
of the previous commit. So while we're at it, we might as well convert
it to lower case as per our usual practice.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 2a2a06d..a10aba8 100644
--- a/refs.c
+++ b/refs.c
@@ -2827,7 +2827,7 @@ int delete_refs(struct string_list *refnames)
const char *refname = refnames->items[i].string;
if (delete_ref(refname, NULL, 0))
- result |= error(_("Could not remove reference %s"), refname);
+ result |= error(_("could not remove reference %s"), refname);
}
return result;
--
2.1.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 07/13] prune_remote(): use delete_refs()
2015-06-08 11:45 [PATCH 00/13] Improve "refs" module encapsulation Michael Haggerty
` (5 preceding siblings ...)
2015-06-08 11:45 ` [PATCH 06/13] delete_refs(): convert error message to lower case Michael Haggerty
@ 2015-06-08 11:45 ` Michael Haggerty
2015-06-08 16:57 ` Stefan Beller
2015-06-08 11:45 ` [PATCH 08/13] repack_without_refs(): make function private Michael Haggerty
` (7 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Michael Haggerty @ 2015-06-08 11:45 UTC (permalink / raw
To: Junio C Hamano; +Cc: Stefan Beller, Jeff King, git, Michael Haggerty
This will result in errors being emitted for references that can't be
deleted, but that is a good thing.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
builtin/remote.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index c8dc724..cc3c741 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1314,19 +1314,12 @@ static int prune_remote(const char *remote, int dry_run)
string_list_append(&refs_to_prune, item->util);
string_list_sort(&refs_to_prune);
- if (!dry_run) {
- struct strbuf err = STRBUF_INIT;
- if (repack_without_refs(&refs_to_prune, &err))
- result |= error("%s", err.buf);
- strbuf_release(&err);
- }
+ if (!dry_run)
+ result |= delete_refs(&refs_to_prune);
for_each_string_list_item(item, &states.stale) {
const char *refname = item->util;
- if (!dry_run)
- result |= delete_ref(refname, NULL, 0);
-
if (dry_run)
printf_ln(_(" * [would prune] %s"),
abbrev_ref(refname, "refs/remotes/"));
--
2.1.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 08/13] repack_without_refs(): make function private
2015-06-08 11:45 [PATCH 00/13] Improve "refs" module encapsulation Michael Haggerty
` (6 preceding siblings ...)
2015-06-08 11:45 ` [PATCH 07/13] prune_remote(): use delete_refs() Michael Haggerty
@ 2015-06-08 11:45 ` Michael Haggerty
2015-06-08 11:45 ` [PATCH 09/13] initial_ref_transaction_commit(): function for initial ref creation Michael Haggerty
` (6 subsequent siblings)
14 siblings, 0 replies; 30+ messages in thread
From: Michael Haggerty @ 2015-06-08 11:45 UTC (permalink / raw
To: Junio C Hamano; +Cc: Stefan Beller, Jeff King, git, Michael Haggerty
It is no longer called from outside of the refs module. Also move its
docstring and change it to imperative voice.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs.c | 9 ++++++++-
refs.h | 11 -----------
2 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/refs.c b/refs.c
index a10aba8..dc17984 100644
--- a/refs.c
+++ b/refs.c
@@ -2724,7 +2724,14 @@ int pack_refs(unsigned int flags)
return 0;
}
-int repack_without_refs(struct string_list *refnames, struct strbuf *err)
+/*
+ * Rewrite the packed-refs file, omitting any refs listed in
+ * 'refnames'. On error, leave packed-refs unchanged, write an error
+ * message to 'err', and return a nonzero value.
+ *
+ * The refs in 'refnames' needn't be sorted. `err` must not be NULL.
+ */
+static int repack_without_refs(struct string_list *refnames, struct strbuf *err)
{
struct ref_dir *packed;
struct string_list_item *refname;
diff --git a/refs.h b/refs.h
index 9b75b9f..3420c98 100644
--- a/refs.h
+++ b/refs.h
@@ -154,17 +154,6 @@ extern void rollback_packed_refs(void);
*/
int pack_refs(unsigned int flags);
-/*
- * Rewrite the packed-refs file, omitting any refs listed in
- * 'refnames'. On error, packed-refs will be unchanged, the return
- * value is nonzero, and a message about the error is written to the
- * 'err' strbuf.
- *
- * The refs in 'refnames' needn't be sorted. `err` must not be NULL.
- */
-extern int repack_without_refs(struct string_list *refnames,
- struct strbuf *err);
-
extern int ref_exists(const char *);
extern int is_branch(const char *refname);
--
2.1.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 09/13] initial_ref_transaction_commit(): function for initial ref creation
2015-06-08 11:45 [PATCH 00/13] Improve "refs" module encapsulation Michael Haggerty
` (7 preceding siblings ...)
2015-06-08 11:45 ` [PATCH 08/13] repack_without_refs(): make function private Michael Haggerty
@ 2015-06-08 11:45 ` Michael Haggerty
2015-06-08 11:45 ` [PATCH 10/13] refs: remove some functions from the module's public interface Michael Haggerty
` (5 subsequent siblings)
14 siblings, 0 replies; 30+ messages in thread
From: Michael Haggerty @ 2015-06-08 11:45 UTC (permalink / raw
To: Junio C Hamano; +Cc: Stefan Beller, Jeff King, git, Michael Haggerty
"git clone" uses shortcuts when creating the initial set of
references:
* It writes them directly to packed-refs.
* It doesn't lock the individual references (though it does lock the
packed-refs file).
* It doesn't check for refname conflicts between two new references or
between one new reference and any hypothetical old ones.
* It doesn't create reflog entries for the reference creations.
This functionality was implemented in builtin/clone.c. But really that
file shouldn't have such intimate knowledge of how references are
stored. So provide a new function in the refs API,
initial_ref_transaction_commit(), which can be used for initial
reference creation. The new function is based on the ref_transaction
interface.
This means that we can make some other functions private to the refs
module. That will be done in a followup commit.
It would seem to make sense to add a test here that there are no
existing references, because that is how the function *should* be
used. But in fact, the "testgit" remote helper appears to call it
*after* having set up refs/remotes/<name>/HEAD and
refs/remotes/<name>/master, so we can't be so strict. For now, the
function trusts its caller to only call it when it makes sense. Future
commits will add some more limited sanity checks.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
builtin/clone.c | 19 +++++++++++++++----
refs.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
refs.h | 14 ++++++++++++++
3 files changed, 76 insertions(+), 4 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index b878252..bd2a50a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -500,16 +500,27 @@ static void write_remote_refs(const struct ref *local_refs)
{
const struct ref *r;
- lock_packed_refs(LOCK_DIE_ON_ERROR);
+ struct ref_transaction *t;
+ struct strbuf err = STRBUF_INIT;
+
+ t = ref_transaction_begin(&err);
+ if (!t)
+ die(err.buf);
for (r = local_refs; r; r = r->next) {
if (!r->peer_ref)
continue;
- add_packed_ref(r->peer_ref->name, r->old_sha1);
+ if (ref_transaction_create(t, r->peer_ref->name, r->old_sha1,
+ 0, NULL, &err))
+ die(err.buf);
+ }
+
+ if (initial_ref_transaction_commit(t, &err)) {
+ die(err.buf);
}
- if (commit_packed_refs())
- die_errno("unable to overwrite old ref-pack file");
+ strbuf_release(&err);
+ ref_transaction_free(t);
}
static void write_followtags(const struct ref *refs, const char *msg)
diff --git a/refs.c b/refs.c
index dc17984..ec4b8a0 100644
--- a/refs.c
+++ b/refs.c
@@ -4041,6 +4041,53 @@ cleanup:
return ret;
}
+int initial_ref_transaction_commit(struct ref_transaction *transaction,
+ struct strbuf *err)
+{
+ int ret = 0, i;
+ int n = transaction->nr;
+ struct ref_update **updates = transaction->updates;
+
+ assert(err);
+
+ if (transaction->state != REF_TRANSACTION_OPEN)
+ die("BUG: commit called for transaction that is not open");
+
+ for (i = 0; i < n; i++) {
+ struct ref_update *update = updates[i];
+
+ if ((update->flags & REF_HAVE_OLD) &&
+ !is_null_sha1(update->old_sha1))
+ die("BUG: initial ref transaction with old_sha1 set");
+ }
+
+ if (lock_packed_refs(0)) {
+ strbuf_addf(err, "unable to lock packed-refs file: %s",
+ strerror(errno));
+ ret = TRANSACTION_GENERIC_ERROR;
+ goto cleanup;
+ }
+
+ for (i = 0; i < n; i++) {
+ struct ref_update *update = updates[i];
+
+ if ((update->flags & REF_HAVE_NEW) &&
+ !is_null_sha1(update->new_sha1))
+ add_packed_ref(update->refname, update->new_sha1);
+ }
+
+ if (commit_packed_refs()) {
+ strbuf_addf(err, "unable to commit packed-refs file: %s",
+ strerror(errno));
+ ret = TRANSACTION_GENERIC_ERROR;
+ goto cleanup;
+ }
+
+cleanup:
+ transaction->state = REF_TRANSACTION_CLOSED;
+ return ret;
+}
+
char *shorten_unambiguous_ref(const char *refname, int strict)
{
int i;
diff --git a/refs.h b/refs.h
index 3420c98..fa600b5 100644
--- a/refs.h
+++ b/refs.h
@@ -365,6 +365,20 @@ int ref_transaction_commit(struct ref_transaction *transaction,
struct strbuf *err);
/*
+ * Like ref_transaction_commit(), but optimized for creating
+ * references when originally initializing a repository (e.g., by "git
+ * clone"). It writes the new references directly to packed-refs
+ * without locking the individual references.
+ *
+ * It is a bug to call this function when there might be other
+ * processes accessing the repository or if there are existing
+ * references that might conflict with the ones being created. All
+ * old_sha1 values must either be absent or NULL_SHA1.
+ */
+int initial_ref_transaction_commit(struct ref_transaction *transaction,
+ struct strbuf *err);
+
+/*
* Free an existing transaction and all associated data.
*/
void ref_transaction_free(struct ref_transaction *transaction);
--
2.1.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 10/13] refs: remove some functions from the module's public interface
2015-06-08 11:45 [PATCH 00/13] Improve "refs" module encapsulation Michael Haggerty
` (8 preceding siblings ...)
2015-06-08 11:45 ` [PATCH 09/13] initial_ref_transaction_commit(): function for initial ref creation Michael Haggerty
@ 2015-06-08 11:45 ` Michael Haggerty
2015-06-08 11:45 ` [PATCH 11/13] initial_ref_transaction_commit(): check for duplicate refs Michael Haggerty
` (4 subsequent siblings)
14 siblings, 0 replies; 30+ messages in thread
From: Michael Haggerty @ 2015-06-08 11:45 UTC (permalink / raw
To: Junio C Hamano; +Cc: Stefan Beller, Jeff King, git, Michael Haggerty
The following functions are no longer used from outside the refs
module:
* lock_packed_refs()
* add_packed_ref()
* commit_packed_refs()
* rollback_packed_refs()
So make these functions private.
This is an important step, because it means that nobody outside of the
refs module needs to know the difference between loose and packed
references.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs.c | 31 ++++++++++++++++++++++++-------
refs.h | 30 ------------------------------
2 files changed, 24 insertions(+), 37 deletions(-)
diff --git a/refs.c b/refs.c
index ec4b8a0..b103a4b 100644
--- a/refs.c
+++ b/refs.c
@@ -1314,7 +1314,13 @@ static struct ref_dir *get_packed_refs(struct ref_cache *refs)
return get_packed_ref_dir(get_packed_ref_cache(refs));
}
-void add_packed_ref(const char *refname, const unsigned char *sha1)
+/*
+ * Add a reference to the in-memory packed reference cache. This may
+ * only be called while the packed-refs file is locked (see
+ * lock_packed_refs()). To actually write the packed-refs file, call
+ * commit_packed_refs().
+ */
+static void add_packed_ref(const char *refname, const unsigned char *sha1)
{
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(&ref_cache);
@@ -2503,8 +2509,12 @@ static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)
return 0;
}
-/* This should return a meaningful errno on failure */
-int lock_packed_refs(int flags)
+/*
+ * Lock the packed-refs file for writing. Flags is passed to
+ * hold_lock_file_for_update(). Return 0 on success. On errors, set
+ * errno appropriately and return a nonzero value.
+ */
+static int lock_packed_refs(int flags)
{
static int timeout_configured = 0;
static int timeout_value = 1000;
@@ -2534,10 +2544,12 @@ int lock_packed_refs(int flags)
}
/*
- * Commit the packed refs changes.
- * On error we must make sure that errno contains a meaningful value.
+ * Write the current version of the packed refs cache from memory to
+ * disk. The packed-refs file must already be locked for writing (see
+ * lock_packed_refs()). Return zero on success. On errors, set errno
+ * and return a nonzero value
*/
-int commit_packed_refs(void)
+static int commit_packed_refs(void)
{
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(&ref_cache);
@@ -2566,7 +2578,12 @@ int commit_packed_refs(void)
return error;
}
-void rollback_packed_refs(void)
+/*
+ * Rollback the lockfile for the packed-refs file, and discard the
+ * in-memory packed reference cache. (The packed-refs file will be
+ * read anew if it is needed again after this function is called.)
+ */
+static void rollback_packed_refs(void)
{
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(&ref_cache);
diff --git a/refs.h b/refs.h
index fa600b5..cb56662 100644
--- a/refs.h
+++ b/refs.h
@@ -111,36 +111,6 @@ extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refn
extern void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct string_list *refnames);
/*
- * Lock the packed-refs file for writing. Flags is passed to
- * hold_lock_file_for_update(). Return 0 on success.
- * Errno is set to something meaningful on error.
- */
-extern int lock_packed_refs(int flags);
-
-/*
- * Add a reference to the in-memory packed reference cache. This may
- * only be called while the packed-refs file is locked (see
- * lock_packed_refs()). To actually write the packed-refs file, call
- * commit_packed_refs().
- */
-extern void add_packed_ref(const char *refname, const unsigned char *sha1);
-
-/*
- * Write the current version of the packed refs cache from memory to
- * disk. The packed-refs file must already be locked for writing (see
- * lock_packed_refs()). Return zero on success.
- * Sets errno to something meaningful on error.
- */
-extern int commit_packed_refs(void);
-
-/*
- * Rollback the lockfile for the packed-refs file, and discard the
- * in-memory packed reference cache. (The packed-refs file will be
- * read anew if it is needed again after this function is called.)
- */
-extern void rollback_packed_refs(void);
-
-/*
* Flags for controlling behaviour of pack_refs()
* PACK_REFS_PRUNE: Prune loose refs after packing
* PACK_REFS_ALL: Pack _all_ refs, not just tags and already packed refs
--
2.1.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 11/13] initial_ref_transaction_commit(): check for duplicate refs
2015-06-08 11:45 [PATCH 00/13] Improve "refs" module encapsulation Michael Haggerty
` (9 preceding siblings ...)
2015-06-08 11:45 ` [PATCH 10/13] refs: remove some functions from the module's public interface Michael Haggerty
@ 2015-06-08 11:45 ` Michael Haggerty
2015-06-08 11:45 ` [PATCH 12/13] initial_ref_transaction_commit(): check for ref D/F conflicts Michael Haggerty
` (3 subsequent siblings)
14 siblings, 0 replies; 30+ messages in thread
From: Michael Haggerty @ 2015-06-08 11:45 UTC (permalink / raw
To: Junio C Hamano; +Cc: Stefan Beller, Jeff King, git, Michael Haggerty
Error out if the ref_transaction includes more than one update for any
refname.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/refs.c b/refs.c
index b103a4b..ced4e53 100644
--- a/refs.c
+++ b/refs.c
@@ -4064,12 +4064,22 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,
int ret = 0, i;
int n = transaction->nr;
struct ref_update **updates = transaction->updates;
+ struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
assert(err);
if (transaction->state != REF_TRANSACTION_OPEN)
die("BUG: commit called for transaction that is not open");
+ /* Fail if a refname appears more than once in the transaction: */
+ for (i = 0; i < n; i++)
+ string_list_append(&affected_refnames, updates[i]->refname);
+ string_list_sort(&affected_refnames);
+ if (ref_update_reject_duplicates(&affected_refnames, err)) {
+ ret = TRANSACTION_GENERIC_ERROR;
+ goto cleanup;
+ }
+
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
@@ -4102,6 +4112,7 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,
cleanup:
transaction->state = REF_TRANSACTION_CLOSED;
+ string_list_clear(&affected_refnames, 0);
return ret;
}
--
2.1.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 12/13] initial_ref_transaction_commit(): check for ref D/F conflicts
2015-06-08 11:45 [PATCH 00/13] Improve "refs" module encapsulation Michael Haggerty
` (10 preceding siblings ...)
2015-06-08 11:45 ` [PATCH 11/13] initial_ref_transaction_commit(): check for duplicate refs Michael Haggerty
@ 2015-06-08 11:45 ` Michael Haggerty
2015-06-08 11:45 ` [PATCH 13/13] refs: move the remaining ref module declarations to refs.h Michael Haggerty
` (2 subsequent siblings)
14 siblings, 0 replies; 30+ messages in thread
From: Michael Haggerty @ 2015-06-08 11:45 UTC (permalink / raw
To: Junio C Hamano; +Cc: Stefan Beller, Jeff King, git, Michael Haggerty
In initial_ref_transaction_commit(), check for D/F conflicts (i.e.,
the type of conflict that exists between "refs/foo" and
"refs/foo/bar") among the references being created and between the
references being created and any hypothetical existing references.
Ideally, there shouldn't *be* any existing references when this
function is called. But, at least in the case of the "testgit" remote
helper, "clone" can be called after the remote-tracking "HEAD" and
"master" branches have already been created. So let's just do the
full-blown check.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
Notes (discussion):
I'm a bit torn on this commit. Part of me says that we should avoid
the overhead of the extra checks against loose_refs and packed_refs
because there shouldn't be any references there anyway. Instead, the
"testgit" remote helper should be fixed.
But these tests should be pretty cheap, and we would want to check for
ref D/F conflicts among the new references in any case, so it seems
safer to include these checks. Also, our documentation suggests that
users refer to git-remote-testgit(1) as an example when writing their
own remote helpers, so it could be that there is other code out there
that has imitated its light misuse of this functionality.
refs.c | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/refs.c b/refs.c
index ced4e53..2c4d9ab 100644
--- a/refs.c
+++ b/refs.c
@@ -4058,9 +4058,19 @@ cleanup:
return ret;
}
+static int ref_present(const char *refname,
+ const struct object_id *oid, int flags, void *cb_data)
+{
+ struct string_list *affected_refnames = cb_data;
+
+ return string_list_has_string(affected_refnames, refname);
+}
+
int initial_ref_transaction_commit(struct ref_transaction *transaction,
struct strbuf *err)
{
+ struct ref_dir *loose_refs = get_loose_refs(&ref_cache);
+ struct ref_dir *packed_refs = get_packed_refs(&ref_cache);
int ret = 0, i;
int n = transaction->nr;
struct ref_update **updates = transaction->updates;
@@ -4080,12 +4090,36 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,
goto cleanup;
}
+ /*
+ * It's really undefined to call this function in an active
+ * repository or when there are existing references: we are
+ * only locking and changing packed-refs, so (1) any
+ * simultaneous processes might try to change a reference at
+ * the same time we do, and (2) any existing loose versions of
+ * the references that we are setting would have precedence
+ * over our values. But some remote helpers create the remote
+ * "HEAD" and "master" branches before calling this function,
+ * so here we really only check that none of the references
+ * that we are creating already exists.
+ */
+ if (for_each_rawref(ref_present, &affected_refnames))
+ die("BUG: initial ref transaction called with existing refs");
+
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
if ((update->flags & REF_HAVE_OLD) &&
!is_null_sha1(update->old_sha1))
die("BUG: initial ref transaction with old_sha1 set");
+ if (verify_refname_available(update->refname,
+ &affected_refnames, NULL,
+ loose_refs, err) ||
+ verify_refname_available(update->refname,
+ &affected_refnames, NULL,
+ packed_refs, err)) {
+ ret = TRANSACTION_NAME_CONFLICT;
+ goto cleanup;
+ }
}
if (lock_packed_refs(0)) {
--
2.1.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 13/13] refs: move the remaining ref module declarations to refs.h
2015-06-08 11:45 [PATCH 00/13] Improve "refs" module encapsulation Michael Haggerty
` (11 preceding siblings ...)
2015-06-08 11:45 ` [PATCH 12/13] initial_ref_transaction_commit(): check for ref D/F conflicts Michael Haggerty
@ 2015-06-08 11:45 ` Michael Haggerty
2015-06-08 17:03 ` [PATCH 00/13] Improve "refs" module encapsulation Stefan Beller
2015-06-09 18:47 ` Junio C Hamano
14 siblings, 0 replies; 30+ messages in thread
From: Michael Haggerty @ 2015-06-08 11:45 UTC (permalink / raw
To: Junio C Hamano; +Cc: Stefan Beller, Jeff King, git, Michael Haggerty
Some functions from the refs module were still declared in cache.h.
Move them to refs.h.
Add some parameter names where they were missing.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
archive.c | 1 +
builtin/blame.c | 1 +
builtin/fast-export.c | 1 +
builtin/fmt-merge-msg.c | 1 +
builtin/init-db.c | 1 +
builtin/log.c | 1 +
cache.h | 66 -----------------------
refs.c | 6 ++-
refs.h | 139 +++++++++++++++++++++++++++++++++++++-----------
remote-testsvn.c | 1 +
10 files changed, 118 insertions(+), 100 deletions(-)
diff --git a/archive.c b/archive.c
index d37c41d..936a594 100644
--- a/archive.c
+++ b/archive.c
@@ -1,4 +1,5 @@
#include "cache.h"
+#include "refs.h"
#include "commit.h"
#include "tree-walk.h"
#include "attr.h"
diff --git a/builtin/blame.c b/builtin/blame.c
index b3e948e..1c998cb 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -6,6 +6,7 @@
*/
#include "cache.h"
+#include "refs.h"
#include "builtin.h"
#include "blob.h"
#include "commit.h"
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index b8182c2..d23f3be 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -5,6 +5,7 @@
*/
#include "builtin.h"
#include "cache.h"
+#include "refs.h"
#include "commit.h"
#include "object.h"
#include "tag.h"
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 05f4c26..4ba7f28 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -1,5 +1,6 @@
#include "builtin.h"
#include "cache.h"
+#include "refs.h"
#include "commit.h"
#include "diff.h"
#include "revision.h"
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 4335738..49df78d 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -4,6 +4,7 @@
* Copyright (C) Linus Torvalds, 2005
*/
#include "cache.h"
+#include "refs.h"
#include "builtin.h"
#include "exec_cmd.h"
#include "parse-options.h"
diff --git a/builtin/log.c b/builtin/log.c
index e67671e..3caa917 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -5,6 +5,7 @@
* 2006 Junio Hamano
*/
#include "cache.h"
+#include "refs.h"
#include "color.h"
#include "commit.h"
#include "diff.h"
diff --git a/cache.h b/cache.h
index be92121..1c00098 100644
--- a/cache.h
+++ b/cache.h
@@ -1009,76 +1009,10 @@ extern int get_oid_hex(const char *hex, struct object_id *sha1);
extern char *sha1_to_hex(const unsigned char *sha1); /* static buffer result! */
extern char *oid_to_hex(const struct object_id *oid); /* same static buffer as sha1_to_hex */
-extern int read_ref_full(const char *refname, int resolve_flags,
- unsigned char *sha1, int *flags);
-extern int read_ref(const char *refname, unsigned char *sha1);
-/*
- * Resolve a reference, recursively following symbolic refererences.
- *
- * Store the referred-to object's name in sha1 and return the name of
- * the non-symbolic reference that ultimately pointed at it. The
- * return value, if not NULL, is a pointer into either a static buffer
- * or the input ref.
- *
- * If the reference cannot be resolved to an object, the behavior
- * depends on the RESOLVE_REF_READING flag:
- *
- * - If RESOLVE_REF_READING is set, return NULL.
- *
- * - If RESOLVE_REF_READING is not set, clear sha1 and return the name of
- * the last reference name in the chain, which will either be a non-symbolic
- * reference or an undefined reference. If this is a prelude to
- * "writing" to the ref, the return value is the name of the ref
- * that will actually be created or changed.
- *
- * If the RESOLVE_REF_NO_RECURSE flag is passed, only resolves one
- * level of symbolic reference. The value stored in sha1 for a symbolic
- * reference will always be null_sha1 in this case, and the return
- * value is the reference that the symref refers to directly.
- *
- * If flags is non-NULL, set the value that it points to the
- * combination of REF_ISPACKED (if the reference was found among the
- * packed references), REF_ISSYMREF (if the initial reference was a
- * symbolic reference), REF_BAD_NAME (if the reference name is ill
- * formed --- see RESOLVE_REF_ALLOW_BAD_NAME below), and REF_ISBROKEN
- * (if the ref is malformed or has a bad name). See refs.h for more detail
- * on each flag.
- *
- * If ref is not a properly-formatted, normalized reference, return
- * NULL. If more than MAXDEPTH recursive symbolic lookups are needed,
- * give up and return NULL.
- *
- * RESOLVE_REF_ALLOW_BAD_NAME allows resolving refs even when their
- * name is invalid according to git-check-ref-format(1). If the name
- * is bad then the value stored in sha1 will be null_sha1 and the two
- * flags REF_ISBROKEN and REF_BAD_NAME will be set.
- *
- * Even with RESOLVE_REF_ALLOW_BAD_NAME, names that escape the refs/
- * directory and do not consist of all caps and underscores cannot be
- * resolved. The function returns NULL for such ref names.
- * Caps and underscores refers to the special refs, such as HEAD,
- * FETCH_HEAD and friends, that all live outside of the refs/ directory.
- */
-#define RESOLVE_REF_READING 0x01
-#define RESOLVE_REF_NO_RECURSE 0x02
-#define RESOLVE_REF_ALLOW_BAD_NAME 0x04
-extern const char *resolve_ref_unsafe(const char *ref, int resolve_flags, unsigned char *sha1, int *flags);
-extern char *resolve_refdup(const char *ref, int resolve_flags, unsigned char *sha1, int *flags);
-
-extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);
-extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);
extern int interpret_branch_name(const char *str, int len, struct strbuf *);
extern int get_sha1_mb(const char *str, unsigned char *sha1);
-/*
- * Return true iff abbrev_name is a possible abbreviation for
- * full_name according to the rules defined by ref_rev_parse_rules in
- * refs.c.
- */
-extern int refname_match(const char *abbrev_name, const char *full_name);
-
-extern int create_symref(const char *ref, const char *refs_heads_master, const char *logmsg);
extern int validate_headref(const char *ref);
extern int base_name_compare(const char *name1, int len1, int mode1, const char *name2, int len2, int mode2);
diff --git a/refs.c b/refs.c
index 2c4d9ab..30aa2a5 100644
--- a/refs.c
+++ b/refs.c
@@ -1732,9 +1732,11 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
return ret;
}
-char *resolve_refdup(const char *ref, int resolve_flags, unsigned char *sha1, int *flags)
+char *resolve_refdup(const char *refname, int resolve_flags,
+ unsigned char *sha1, int *flags)
{
- return xstrdup_or_null(resolve_ref_unsafe(ref, resolve_flags, sha1, flags));
+ return xstrdup_or_null(resolve_ref_unsafe(refname, resolve_flags,
+ sha1, flags));
}
/* The argument to filter_refs */
diff --git a/refs.h b/refs.h
index cb56662..33c2f76 100644
--- a/refs.h
+++ b/refs.h
@@ -2,6 +2,98 @@
#define REFS_H
/*
+ * Resolve a reference, recursively following symbolic refererences.
+ *
+ * Store the referred-to object's name in sha1 and return the name of
+ * the non-symbolic reference that ultimately pointed at it. The
+ * return value, if not NULL, is a pointer into either a static buffer
+ * or the input ref.
+ *
+ * If the reference cannot be resolved to an object, the behavior
+ * depends on the RESOLVE_REF_READING flag:
+ *
+ * - If RESOLVE_REF_READING is set, return NULL.
+ *
+ * - If RESOLVE_REF_READING is not set, clear sha1 and return the name of
+ * the last reference name in the chain, which will either be a non-symbolic
+ * reference or an undefined reference. If this is a prelude to
+ * "writing" to the ref, the return value is the name of the ref
+ * that will actually be created or changed.
+ *
+ * If the RESOLVE_REF_NO_RECURSE flag is passed, only resolves one
+ * level of symbolic reference. The value stored in sha1 for a symbolic
+ * reference will always be null_sha1 in this case, and the return
+ * value is the reference that the symref refers to directly.
+ *
+ * If flags is non-NULL, set the value that it points to the
+ * combination of REF_ISPACKED (if the reference was found among the
+ * packed references), REF_ISSYMREF (if the initial reference was a
+ * symbolic reference), REF_BAD_NAME (if the reference name is ill
+ * formed --- see RESOLVE_REF_ALLOW_BAD_NAME below), and REF_ISBROKEN
+ * (if the ref is malformed or has a bad name). See refs.h for more detail
+ * on each flag.
+ *
+ * If ref is not a properly-formatted, normalized reference, return
+ * NULL. If more than MAXDEPTH recursive symbolic lookups are needed,
+ * give up and return NULL.
+ *
+ * RESOLVE_REF_ALLOW_BAD_NAME allows resolving refs even when their
+ * name is invalid according to git-check-ref-format(1). If the name
+ * is bad then the value stored in sha1 will be null_sha1 and the two
+ * flags REF_ISBROKEN and REF_BAD_NAME will be set.
+ *
+ * Even with RESOLVE_REF_ALLOW_BAD_NAME, names that escape the refs/
+ * directory and do not consist of all caps and underscores cannot be
+ * resolved. The function returns NULL for such ref names.
+ * Caps and underscores refers to the special refs, such as HEAD,
+ * FETCH_HEAD and friends, that all live outside of the refs/ directory.
+ */
+#define RESOLVE_REF_READING 0x01
+#define RESOLVE_REF_NO_RECURSE 0x02
+#define RESOLVE_REF_ALLOW_BAD_NAME 0x04
+
+extern const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
+ unsigned char *sha1, int *flags);
+
+extern char *resolve_refdup(const char *refname, int resolve_flags,
+ unsigned char *sha1, int *flags);
+
+extern int read_ref_full(const char *refname, int resolve_flags,
+ unsigned char *sha1, int *flags);
+extern int read_ref(const char *refname, unsigned char *sha1);
+
+extern int ref_exists(const char *refname);
+
+extern int is_branch(const char *refname);
+
+/*
+ * If refname is a non-symbolic reference that refers to a tag object,
+ * and the tag can be (recursively) dereferenced to a non-tag object,
+ * store the SHA1 of the referred-to object to sha1 and return 0. If
+ * any of these conditions are not met, return a non-zero value.
+ * Symbolic references are considered unpeelable, even if they
+ * ultimately resolve to a peelable tag.
+ */
+extern int peel_ref(const char *refname, unsigned char *sha1);
+
+/**
+ * Resolve refname in the nested "gitlink" repository that is located
+ * at path. If the resolution is successful, return 0 and set sha1 to
+ * the name of the object; otherwise, return a non-zero value.
+ */
+extern int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sha1);
+
+/*
+ * Return true iff abbrev_name is a possible abbreviation for
+ * full_name according to the rules defined by ref_rev_parse_rules in
+ * refs.c.
+ */
+extern int refname_match(const char *abbrev_name, const char *full_name);
+
+extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);
+extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);
+
+/*
* A ref_transaction represents a collection of ref updates
* that should succeed or fail together.
*
@@ -78,15 +170,15 @@ typedef int each_ref_fn(const char *refname,
* modifies the reference also returns a nonzero value to immediately
* stop the iteration.
*/
-extern int head_ref(each_ref_fn, void *);
-extern int for_each_ref(each_ref_fn, void *);
-extern int for_each_ref_in(const char *, each_ref_fn, void *);
-extern int for_each_tag_ref(each_ref_fn, void *);
-extern int for_each_branch_ref(each_ref_fn, void *);
-extern int for_each_remote_ref(each_ref_fn, void *);
-extern int for_each_replace_ref(each_ref_fn, void *);
-extern int for_each_glob_ref(each_ref_fn, const char *pattern, void *);
-extern int for_each_glob_ref_in(each_ref_fn, const char *pattern, const char* prefix, void *);
+extern int head_ref(each_ref_fn fn, void *cb_data);
+extern int for_each_ref(each_ref_fn fn, void *cb_data);
+extern int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data);
+extern int for_each_tag_ref(each_ref_fn fn, void *cb_data);
+extern int for_each_branch_ref(each_ref_fn fn, void *cb_data);
+extern int for_each_remote_ref(each_ref_fn fn, void *cb_data);
+extern int for_each_replace_ref(each_ref_fn fn, void *cb_data);
+extern int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);
+extern int for_each_glob_ref_in(each_ref_fn fn, const char *pattern, const char* prefix, void *cb_data);
extern int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);
extern int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);
@@ -99,14 +191,14 @@ extern int for_each_remote_ref_submodule(const char *submodule, each_ref_fn fn,
extern int head_ref_namespaced(each_ref_fn fn, void *cb_data);
extern int for_each_namespaced_ref(each_ref_fn fn, void *cb_data);
+/* can be used to learn about broken ref and symref */
+extern int for_each_rawref(each_ref_fn fn, void *cb_data);
+
static inline const char *has_glob_specials(const char *pattern)
{
return strpbrk(pattern, "?*[");
}
-/* can be used to learn about broken ref and symref */
-extern int for_each_rawref(each_ref_fn, void *);
-
extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname);
extern void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct string_list *refnames);
@@ -124,20 +216,6 @@ extern void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct st
*/
int pack_refs(unsigned int flags);
-extern int ref_exists(const char *);
-
-extern int is_branch(const char *refname);
-
-/*
- * If refname is a non-symbolic reference that refers to a tag object,
- * and the tag can be (recursively) dereferenced to a non-tag object,
- * store the SHA1 of the referred-to object to sha1 and return 0. If
- * any of these conditions are not met, return a non-zero value.
- * Symbolic references are considered unpeelable, even if they
- * ultimately resolve to a peelable tag.
- */
-extern int peel_ref(const char *refname, unsigned char *sha1);
-
/*
* Flags controlling ref_transaction_update(), ref_transaction_create(), etc.
* REF_NODEREF: act on the ref directly, instead of dereferencing
@@ -205,17 +283,13 @@ extern int for_each_reflog(each_ref_fn, void *);
extern int check_refname_format(const char *refname, int flags);
extern const char *prettify_refname(const char *refname);
+
extern char *shorten_unambiguous_ref(const char *refname, int strict);
/** rename ref, return 0 on success **/
extern int rename_ref(const char *oldref, const char *newref, const char *logmsg);
-/**
- * Resolve refname in the nested "gitlink" repository that is located
- * at path. If the resolution is successful, return 0 and set sha1 to
- * the name of the object; otherwise, return a non-zero value.
- */
-extern int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sha1);
+extern int create_symref(const char *ref, const char *refs_heads_master, const char *logmsg);
enum action_on_err {
UPDATE_REFS_MSG_ON_ERR,
@@ -366,6 +440,7 @@ int update_ref(const char *msg, const char *refname,
unsigned int flags, enum action_on_err onerr);
extern int parse_hide_refs_config(const char *var, const char *value, const char *);
+
extern int ref_is_hidden(const char *);
enum expire_reflog_flags {
diff --git a/remote-testsvn.c b/remote-testsvn.c
index 48bf6eb..f599c37 100644
--- a/remote-testsvn.c
+++ b/remote-testsvn.c
@@ -1,4 +1,5 @@
#include "cache.h"
+#include "refs.h"
#include "remote.h"
#include "strbuf.h"
#include "url.h"
--
2.1.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 01/13] delete_ref(): move declaration to refs.h
2015-06-08 11:45 ` [PATCH 01/13] delete_ref(): move declaration to refs.h Michael Haggerty
@ 2015-06-08 16:43 ` Stefan Beller
2015-06-09 10:10 ` Michael Haggerty
0 siblings, 1 reply; 30+ messages in thread
From: Stefan Beller @ 2015-06-08 16:43 UTC (permalink / raw
To: Michael Haggerty; +Cc: Junio C Hamano, Jeff King, git@vger.kernel.org
On Mon, Jun 8, 2015 at 4:45 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Also
>
> * Add a docstring
>
> * Rename the second parameter to "old_sha1", to be consistent with the
> convention used elsewhere in the refs module
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> cache.h | 2 --
> refs.c | 5 +++--
> refs.h | 9 +++++++++
> 3 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 571c98f..be92121 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -585,8 +585,6 @@ extern void update_index_if_able(struct index_state *, struct lock_file *);
> extern int hold_locked_index(struct lock_file *, int);
> extern void set_alternate_index_output(const char *);
>
> -extern int delete_ref(const char *, const unsigned char *sha1, unsigned int flags);
> -
> /* Environment bits from configuration mechanism */
> extern int trust_executable_bit;
> extern int trust_ctime;
> diff --git a/refs.c b/refs.c
> index a742d79..b575bb8 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2789,7 +2789,8 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)
> return 0;
> }
>
> -int delete_ref(const char *refname, const unsigned char *sha1, unsigned int flags)
> +int delete_ref(const char *refname, const unsigned char *old_sha1,
> + unsigned int flags)
> {
> struct ref_transaction *transaction;
> struct strbuf err = STRBUF_INIT;
> @@ -2797,7 +2798,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, unsigned int flag
> transaction = ref_transaction_begin(&err);
> if (!transaction ||
> ref_transaction_delete(transaction, refname,
> - (sha1 && !is_null_sha1(sha1)) ? sha1 : NULL,
> + (old_sha1 && !is_null_sha1(old_sha1)) ? old_sha1 : NULL,
> flags, NULL, &err) ||
> ref_transaction_commit(transaction, &err)) {
> error("%s", err.buf);
> diff --git a/refs.h b/refs.h
> index 8c3d433..8785bca 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -202,6 +202,15 @@ extern int read_ref_at(const char *refname, unsigned int flags,
> /** Check if a particular reflog exists */
> extern int reflog_exists(const char *refname);
>
> +/*
> + * Delete the specified reference. If old_sha1 is non-NULL and not
> + * NULL_SHA1, then verify that the current value of the reference is
> + * old_sha1 before deleting it.
And here I wondered what the distinction between NULL and non-NULL,
but NULL_SHA1
is and digging into the code, there is none. (As you can also see in
this patch above with
(old_sha1 && !is_null_sha1(old_sha1)) ? old_sha1 : NULL,
but when digging deeper, the !is_null_sha1(old_sha1) is an arbitrary
limitation (i.e.
ref_transaction_delete and ref_transaction_update don't differ between
those two cases.)
The distinction comes in at lock_ref_sha1_basic, where I think we may
want to get rid of
the is_null_sha1 check and depend on NULL/non-NULL as the difference
for valid and invalid
sha1's?
That said, do we want to add another sentence to the doc here saying
non-NULL and not
NULL_SHA1 are treated the same or is it clear enough?
With or without this question addressed:
Reviewed-by: Stefan Beller <sbeller@google.com>
> flags is passed to
> + * ref_transaction_delete().
> + */
> +extern int delete_ref(const char *refname, const unsigned char *old_sha1,
> + unsigned int flags);
> +
> /** Delete a reflog */
> extern int delete_reflog(const char *refname);
>
> --
> 2.1.4
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 02/13] remove_branches(): remove temporary
2015-06-08 11:45 ` [PATCH 02/13] remove_branches(): remove temporary Michael Haggerty
@ 2015-06-08 16:45 ` Stefan Beller
0 siblings, 0 replies; 30+ messages in thread
From: Stefan Beller @ 2015-06-08 16:45 UTC (permalink / raw
To: Michael Haggerty; +Cc: Junio C Hamano, Jeff King, git@vger.kernel.org
On Mon, Jun 8, 2015 at 4:45 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Found-to-be-Obviously-Correct-by: Stefan Beller <sbeller@google.com> ;)
> ---
> builtin/remote.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/builtin/remote.c b/builtin/remote.c
> index f4a6ec9..53b8e13 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -756,8 +756,7 @@ static int remove_branches(struct string_list *branches)
> strbuf_release(&err);
>
> for (i = 0; i < branches->nr; i++) {
> - struct string_list_item *item = branches->items + i;
> - const char *refname = item->string;
> + const char *refname = branches->items[i].string;
>
> if (delete_ref(refname, NULL, 0))
> result |= error(_("Could not remove branch %s"), refname);
> --
> 2.1.4
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 03/13] delete_ref(): handle special case more explicitly
2015-06-08 11:45 ` [PATCH 03/13] delete_ref(): handle special case more explicitly Michael Haggerty
@ 2015-06-08 16:48 ` Stefan Beller
2015-06-09 10:17 ` Michael Haggerty
0 siblings, 1 reply; 30+ messages in thread
From: Stefan Beller @ 2015-06-08 16:48 UTC (permalink / raw
To: Michael Haggerty; +Cc: Junio C Hamano, Jeff King, git@vger.kernel.org
On Mon, Jun 8, 2015 at 4:45 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> delete_ref() uses a different convention for its old_sha1 parameter
> than, say, ref_transaction_delete(): NULL_SHA1 means not to check the
> old value. Make this fact a little bit clearer in the code by handling
> it in explicit, commented code rather than burying it in a conditional
> expression.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> refs.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index b575bb8..f9d87b6 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2795,10 +2795,13 @@ int delete_ref(const char *refname, const unsigned char *old_sha1,
> struct ref_transaction *transaction;
> struct strbuf err = STRBUF_INIT;
>
> + /* Treat NULL_SHA1 as "don't care" */
and by "don't care" you mean we still care about getting it deleted,
the part we don't care about is the particular sha1 (could be a bogus ref).
> + if (old_sha1 && is_null_sha1(old_sha1))
> + old_sha1 = NULL;
> +
> transaction = ref_transaction_begin(&err);
> if (!transaction ||
> - ref_transaction_delete(transaction, refname,
> - (old_sha1 && !is_null_sha1(old_sha1)) ? old_sha1 : NULL,
> + ref_transaction_delete(transaction, refname, old_sha1,
> flags, NULL, &err) ||
> ref_transaction_commit(transaction, &err)) {
> error("%s", err.buf);
> --
> 2.1.4
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 06/13] delete_refs(): convert error message to lower case
2015-06-08 11:45 ` [PATCH 06/13] delete_refs(): convert error message to lower case Michael Haggerty
@ 2015-06-08 16:51 ` Stefan Beller
2015-06-09 10:23 ` Michael Haggerty
0 siblings, 1 reply; 30+ messages in thread
From: Stefan Beller @ 2015-06-08 16:51 UTC (permalink / raw
To: Michael Haggerty; +Cc: Junio C Hamano, Jeff King, git@vger.kernel.org
On Mon, Jun 8, 2015 at 4:45 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> This string is going to have to be re-internationalized anyway because
> of the previous commit. So while we're at it, we might as well convert
> it to lower case as per our usual practice.
Although the previous patch and this are addressing two slightly
different things,
we may want to squash this into the previous without dropping any of
the commit message?
(It might make reviewing easier, I'd assume)
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> refs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/refs.c b/refs.c
> index 2a2a06d..a10aba8 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2827,7 +2827,7 @@ int delete_refs(struct string_list *refnames)
> const char *refname = refnames->items[i].string;
>
> if (delete_ref(refname, NULL, 0))
> - result |= error(_("Could not remove reference %s"), refname);
> + result |= error(_("could not remove reference %s"), refname);
> }
>
> return result;
> --
> 2.1.4
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 07/13] prune_remote(): use delete_refs()
2015-06-08 11:45 ` [PATCH 07/13] prune_remote(): use delete_refs() Michael Haggerty
@ 2015-06-08 16:57 ` Stefan Beller
2015-06-08 17:12 ` Jeff King
0 siblings, 1 reply; 30+ messages in thread
From: Stefan Beller @ 2015-06-08 16:57 UTC (permalink / raw
To: Michael Haggerty; +Cc: Junio C Hamano, Jeff King, git@vger.kernel.org
On Mon, Jun 8, 2015 at 4:45 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> This will result in errors being emitted for references that can't be
> deleted, but that is a good thing.
This sounds a bit like hand-waving to me. "Trust me, I'm an engineer!".
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> builtin/remote.c | 11 ++---------
> 1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/remote.c b/builtin/remote.c
> index c8dc724..cc3c741 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -1314,19 +1314,12 @@ static int prune_remote(const char *remote, int dry_run)
> string_list_append(&refs_to_prune, item->util);
> string_list_sort(&refs_to_prune);
>
> - if (!dry_run) {
> - struct strbuf err = STRBUF_INIT;
> - if (repack_without_refs(&refs_to_prune, &err))
> - result |= error("%s", err.buf);
> - strbuf_release(&err);
> - }
> + if (!dry_run)
> + result |= delete_refs(&refs_to_prune);
>
> for_each_string_list_item(item, &states.stale) {
> const char *refname = item->util;
>
> - if (!dry_run)
> - result |= delete_ref(refname, NULL, 0);
> -
> if (dry_run)
> printf_ln(_(" * [would prune] %s"),
> abbrev_ref(refname, "refs/remotes/"));
> --
> 2.1.4
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 00/13] Improve "refs" module encapsulation
2015-06-08 11:45 [PATCH 00/13] Improve "refs" module encapsulation Michael Haggerty
` (12 preceding siblings ...)
2015-06-08 11:45 ` [PATCH 13/13] refs: move the remaining ref module declarations to refs.h Michael Haggerty
@ 2015-06-08 17:03 ` Stefan Beller
2015-06-09 18:47 ` Junio C Hamano
14 siblings, 0 replies; 30+ messages in thread
From: Stefan Beller @ 2015-06-08 17:03 UTC (permalink / raw
To: Michael Haggerty; +Cc: Junio C Hamano, Jeff King, git@vger.kernel.org
On Mon, Jun 8, 2015 at 4:45 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Add functions to the reference API to
>
> * Delete a bunch of references at once, but *without* failing the
> whole transaction if one of the deletions fails. This functionality
> is used by `git remote remove` and `git remote prune`.
>
> * Create initial references during `git clone`. (During clone,
> references are written directly to the `packed-refs` file without
> any locking.)
>
> Also move the remaining "refs" function declarations from `cache.h` to
> `refs.h`.
>
> This improves the encapsulation of the refs module. Especially, it
> means that code outside of the refs module should no longer need to
> care about the distinction between loose and packed references.
>
> These patches are also available from my GitHub account [1] as branch
> "init-delete-refs-api".
>
> [1] https://github.com/mhagger/git
Thw whole series looks good to me.
>
> Michael Haggerty (13):
> 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(): improve error message
> delete_refs(): convert error message to lower case
> prune_remote(): 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
>
> archive.c | 1 +
> builtin/blame.c | 1 +
> builtin/clone.c | 19 ++++-
> builtin/fast-export.c | 1 +
> builtin/fmt-merge-msg.c | 1 +
> builtin/init-db.c | 1 +
> builtin/log.c | 1 +
> builtin/remote.c | 33 +-------
> cache.h | 68 ----------------
> refs.c | 167 +++++++++++++++++++++++++++++++++++---
> refs.h | 210 +++++++++++++++++++++++++++++++-----------------
> remote-testsvn.c | 1 +
> 12 files changed, 316 insertions(+), 188 deletions(-)
>
> --
> 2.1.4
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 07/13] prune_remote(): use delete_refs()
2015-06-08 16:57 ` Stefan Beller
@ 2015-06-08 17:12 ` Jeff King
2015-06-09 10:50 ` Michael Haggerty
0 siblings, 1 reply; 30+ messages in thread
From: Jeff King @ 2015-06-08 17:12 UTC (permalink / raw
To: Stefan Beller; +Cc: Michael Haggerty, Junio C Hamano, git@vger.kernel.org
On Mon, Jun 08, 2015 at 09:57:04AM -0700, Stefan Beller wrote:
> On Mon, Jun 8, 2015 at 4:45 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> > This will result in errors being emitted for references that can't be
> > deleted, but that is a good thing.
>
> This sounds a bit like hand-waving to me. "Trust me, I'm an engineer!".
I think the argument is "we failed to do that the user asked for, but
never reported the reason why".
But I don't see how that is the case. We already complain if
repack_without_refs fail, and AFAICT the original call to delete_ref()
would emit an error, as well.
-Peff
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 01/13] delete_ref(): move declaration to refs.h
2015-06-08 16:43 ` Stefan Beller
@ 2015-06-09 10:10 ` Michael Haggerty
2015-06-09 16:42 ` Stefan Beller
0 siblings, 1 reply; 30+ messages in thread
From: Michael Haggerty @ 2015-06-09 10:10 UTC (permalink / raw
To: Stefan Beller; +Cc: Junio C Hamano, Jeff King, git@vger.kernel.org
On 06/08/2015 06:43 PM, Stefan Beller wrote:
> On Mon, Jun 8, 2015 at 4:45 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> [...]
>> +/*
>> + * Delete the specified reference. If old_sha1 is non-NULL and not
>> + * NULL_SHA1, then verify that the current value of the reference is
>> + * old_sha1 before deleting it.
>
> And here I wondered what the distinction between NULL and non-NULL,
> but NULL_SHA1
> is and digging into the code, there is none. (As you can also see in
> this patch above with
> (old_sha1 && !is_null_sha1(old_sha1)) ? old_sha1 : NULL,
> but when digging deeper, the !is_null_sha1(old_sha1) is an arbitrary
> limitation (i.e.
> ref_transaction_delete and ref_transaction_update don't differ between
> those two cases.)
>
> The distinction comes in at lock_ref_sha1_basic, where I think we may
> want to get rid of
> the is_null_sha1 check and depend on NULL/non-NULL as the difference
> for valid and invalid
> sha1's?
I'm having a little trouble understanding your comment.
The convention for ref_transaction_update() and friends is that
* old_sha1 == NULL
We don't care whether the reference existed prior to the
update, nor what its value was.
* *old_sha1 is NULL_SHA1
(by which I mean that old_sha1 points at 20 zero bytes; I hope
that's clear even though NULL_SHA1 is not actually defined
anywhere): The reference must *not* have existed prior to the
update.
* old_sha1 has some other value
The reference must have had that value prior to the update.
lock_ref_sha1_basic() distinguishes between { NULL vs. NULL_SHA1 vs.
other values } in the same way that ref_transaction_update() does.
The delete_ref() function doesn't follow the same convention. It treats
NULL and NULL_SHA1 identically, as "don't care".
It probably makes sense to change delete_ref() use the same convention
as ref_transaction_update(), but there are quite a few callers and I
didn't have the energy to review them all as part of this patch series.
So I left it unchanged and just documented the status quo better.
> That said, do we want to add another sentence to the doc here saying
> non-NULL and not
> NULL_SHA1 are treated the same or is it clear enough?
> With or without this question addressed:
> Reviewed-by: Stefan Beller <sbeller@google.com>
In set notation,
"non-NULL" =
"non-NULL and not NULL_SHA1" ∪
"non-NULL and equal to NULL_SHA1"
The latter two are *not* treated the same, so I don't see how we can
claim that "non-NULL" and "not NULL_SHA1" are treated the same. I must
be misunderstanding you.
Would it help if I changed the comment to
Delete the specified reference. If old_sha1 is non-NULL and not
NULL_SHA1, then verify that the current value of the reference is
old_sha1 before deleting it. If old_sha1 is NULL or NULL_SHA1,
delete the reference it it exists, regardless of its old value.
?
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 03/13] delete_ref(): handle special case more explicitly
2015-06-08 16:48 ` Stefan Beller
@ 2015-06-09 10:17 ` Michael Haggerty
0 siblings, 0 replies; 30+ messages in thread
From: Michael Haggerty @ 2015-06-09 10:17 UTC (permalink / raw
To: Stefan Beller; +Cc: Junio C Hamano, Jeff King, git@vger.kernel.org
On 06/08/2015 06:48 PM, Stefan Beller wrote:
> On Mon, Jun 8, 2015 at 4:45 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> delete_ref() uses a different convention for its old_sha1 parameter
>> than, say, ref_transaction_delete(): NULL_SHA1 means not to check the
>> old value. Make this fact a little bit clearer in the code by handling
>> it in explicit, commented code rather than burying it in a conditional
>> expression.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>> refs.c | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/refs.c b/refs.c
>> index b575bb8..f9d87b6 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2795,10 +2795,13 @@ int delete_ref(const char *refname, const unsigned char *old_sha1,
>> struct ref_transaction *transaction;
>> struct strbuf err = STRBUF_INIT;
>>
>> + /* Treat NULL_SHA1 as "don't care" */
>
> and by "don't care" you mean we still care about getting it deleted,
> the part we don't care about is the particular sha1 (could be a bogus ref).
Correct. I will to change the comment to
/*
* Treat NULL_SHA1 and NULL alike, to mean "we don't care what
* the old value of the reference was (or even if it didn't
* exist)":
*/
to make that clearer.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 06/13] delete_refs(): convert error message to lower case
2015-06-08 16:51 ` Stefan Beller
@ 2015-06-09 10:23 ` Michael Haggerty
0 siblings, 0 replies; 30+ messages in thread
From: Michael Haggerty @ 2015-06-09 10:23 UTC (permalink / raw
To: Stefan Beller; +Cc: Junio C Hamano, Jeff King, git@vger.kernel.org
On 06/08/2015 06:51 PM, Stefan Beller wrote:
> On Mon, Jun 8, 2015 at 4:45 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> This string is going to have to be re-internationalized anyway because
>> of the previous commit. So while we're at it, we might as well convert
>> it to lower case as per our usual practice.
>
> Although the previous patch and this are addressing two slightly
> different things,
> we may want to squash this into the previous without dropping any of
> the commit message?
> (It might make reviewing easier, I'd assume)
OK, I will squash them together.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 07/13] prune_remote(): use delete_refs()
2015-06-08 17:12 ` Jeff King
@ 2015-06-09 10:50 ` Michael Haggerty
2015-06-09 11:53 ` Jeff King
0 siblings, 1 reply; 30+ messages in thread
From: Michael Haggerty @ 2015-06-09 10:50 UTC (permalink / raw
To: Jeff King, Stefan Beller; +Cc: Junio C Hamano, git@vger.kernel.org
On 06/08/2015 07:12 PM, Jeff King wrote:
> On Mon, Jun 08, 2015 at 09:57:04AM -0700, Stefan Beller wrote:
>
>> On Mon, Jun 8, 2015 at 4:45 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>> This will result in errors being emitted for references that can't be
>>> deleted, but that is a good thing.
>>
>> This sounds a bit like hand-waving to me. "Trust me, I'm an engineer!".
>
> I think the argument is "we failed to do that the user asked for, but
> never reported the reason why".
>
> But I don't see how that is the case. We already complain if
> repack_without_refs fail, and AFAICT the original call to delete_ref()
> would emit an error, as well.
The old and new code report errors that come from repack_without_refs()
the same way. The difference is how they report errors within delete_ref().
The old code only allowed delete_ref() to emit its error.
The new code (in delete_refs()) allows delete_ref() to emit its error,
but then follows it up with
error(_("could not remove reference %s"), refname)
The "could not remove reference" error originally came from a similar
message in remove_branches() (from builtin/remote.c).
I *think* this is an improvement, because the error from delete_ref()
(which usually comes from ref_transaction_commit()) can be pretty
low-level, like
Cannot lock ref '%s': unable to resolve reference %s: %s
where the last "%s" is the original strerror.
I would be happy to change the behavior if somebody has a concrete wish.
At the same time I don't think we need to sweat the details too much,
because these errors should only ever be seen in the case of a broken
repository or a race between two processes; i.e., only in pretty rare
and anomalous situations.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 07/13] prune_remote(): use delete_refs()
2015-06-09 10:50 ` Michael Haggerty
@ 2015-06-09 11:53 ` Jeff King
0 siblings, 0 replies; 30+ messages in thread
From: Jeff King @ 2015-06-09 11:53 UTC (permalink / raw
To: Michael Haggerty; +Cc: Stefan Beller, Junio C Hamano, git@vger.kernel.org
On Tue, Jun 09, 2015 at 12:50:13PM +0200, Michael Haggerty wrote:
> The new code (in delete_refs()) allows delete_ref() to emit its error,
> but then follows it up with
>
> error(_("could not remove reference %s"), refname)
>
> The "could not remove reference" error originally came from a similar
> message in remove_branches() (from builtin/remote.c).
>
> I *think* this is an improvement, because the error from delete_ref()
> (which usually comes from ref_transaction_commit()) can be pretty
> low-level, like
>
> Cannot lock ref '%s': unable to resolve reference %s: %s
>
> where the last "%s" is the original strerror.
>
> I would be happy to change the behavior if somebody has a concrete wish.
> At the same time I don't think we need to sweat the details too much,
> because these errors should only ever be seen in the case of a broken
> repository or a race between two processes; i.e., only in pretty rare
> and anomalous situations.
Thanks for the explanation. I agree it probably doesn't matter much
either way.
-Peff
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 01/13] delete_ref(): move declaration to refs.h
2015-06-09 10:10 ` Michael Haggerty
@ 2015-06-09 16:42 ` Stefan Beller
2015-06-13 14:30 ` Michael Haggerty
0 siblings, 1 reply; 30+ messages in thread
From: Stefan Beller @ 2015-06-09 16:42 UTC (permalink / raw
To: Michael Haggerty; +Cc: Junio C Hamano, Jeff King, git@vger.kernel.org
On Tue, Jun 9, 2015 at 3:10 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 06/08/2015 06:43 PM, Stefan Beller wrote:
>> On Mon, Jun 8, 2015 at 4:45 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>> [...]
>>> +/*
>>> + * Delete the specified reference. If old_sha1 is non-NULL and not
>>> + * NULL_SHA1, then verify that the current value of the reference is
>>> + * old_sha1 before deleting it.
>>
>> And here I wondered what the distinction between NULL and non-NULL,
>> but NULL_SHA1
>> is and digging into the code, there is none. (As you can also see in
>> this patch above with
>> (old_sha1 && !is_null_sha1(old_sha1)) ? old_sha1 : NULL,
>> but when digging deeper, the !is_null_sha1(old_sha1) is an arbitrary
>> limitation (i.e.
>> ref_transaction_delete and ref_transaction_update don't differ between
>> those two cases.)
>>
>> The distinction comes in at lock_ref_sha1_basic, where I think we may
>> want to get rid of
>> the is_null_sha1 check and depend on NULL/non-NULL as the difference
>> for valid and invalid
>> sha1's?
>
> I'm having a little trouble understanding your comment.
>
> The convention for ref_transaction_update() and friends is that
>
> * old_sha1 == NULL
>
> We don't care whether the reference existed prior to the
> update, nor what its value was.
>
> * *old_sha1 is NULL_SHA1
>
> (by which I mean that old_sha1 points at 20 zero bytes; I hope
> that's clear even though NULL_SHA1 is not actually defined
> anywhere): The reference must *not* have existed prior to the
> update.
Ok that's what I was missing.
>
> * old_sha1 has some other value
>
> The reference must have had that value prior to the update.
>
> lock_ref_sha1_basic() distinguishes between { NULL vs. NULL_SHA1 vs.
> other values } in the same way that ref_transaction_update() does.
>
> The delete_ref() function doesn't follow the same convention. It treats
> NULL and NULL_SHA1 identically, as "don't care".
>
> It probably makes sense to change delete_ref() use the same convention
> as ref_transaction_update(), but there are quite a few callers and I
> didn't have the energy to review them all as part of this patch series.
> So I left it unchanged and just documented the status quo better.
>
>> That said, do we want to add another sentence to the doc here saying
>> non-NULL and not
>> NULL_SHA1 are treated the same or is it clear enough?
>> With or without this question addressed:
>> Reviewed-by: Stefan Beller <sbeller@google.com>
>
> In set notation,
>
> "non-NULL" =
> "non-NULL and not NULL_SHA1" ∪
> "non-NULL and equal to NULL_SHA1"
>
> The latter two are *not* treated the same, so I don't see how we can
> claim that "non-NULL" and "not NULL_SHA1" are treated the same. I must
> be misunderstanding you.
>
> Would it help if I changed the comment to
>
> Delete the specified reference. If old_sha1 is non-NULL and not
> NULL_SHA1, then verify that the current value of the reference is
> old_sha1 before deleting it. If old_sha1 is NULL or NULL_SHA1,
> delete the reference it it exists, regardless of its old value.
>
> ?
This is very clear to me.
>
> Michael
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 00/13] Improve "refs" module encapsulation
2015-06-08 11:45 [PATCH 00/13] Improve "refs" module encapsulation Michael Haggerty
` (13 preceding siblings ...)
2015-06-08 17:03 ` [PATCH 00/13] Improve "refs" module encapsulation Stefan Beller
@ 2015-06-09 18:47 ` Junio C Hamano
14 siblings, 0 replies; 30+ messages in thread
From: Junio C Hamano @ 2015-06-09 18:47 UTC (permalink / raw
To: Michael Haggerty; +Cc: Stefan Beller, Jeff King, git
Michael Haggerty <mhagger@alum.mit.edu> writes:
> Add functions to the reference API to
>
> * Delete a bunch of references at once, but *without* failing the
> whole transaction if one of the deletions fails. This functionality
> is used by `git remote remove` and `git remote prune`.
>
> * Create initial references during `git clone`. (During clone,
> references are written directly to the `packed-refs` file without
> any locking.)
>
> Also move the remaining "refs" function declarations from `cache.h` to
> `refs.h`.
>
> This improves the encapsulation of the refs module. Especially, it
> means that code outside of the refs module should no longer need to
> care about the distinction between loose and packed references.
Yay. The subject of the series matches this "primary value" of the
topic, even though the cover text makes it sound as if it was just a
"while we are at adding two functions" with "Also move..." ;-)
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 05/13] delete_refs(): improve error message
2015-06-08 11:45 ` [PATCH 05/13] delete_refs(): improve error message Michael Haggerty
@ 2015-06-09 18:47 ` Junio C Hamano
0 siblings, 0 replies; 30+ messages in thread
From: Junio C Hamano @ 2015-06-09 18:47 UTC (permalink / raw
To: Michael Haggerty; +Cc: Stefan Beller, Jeff King, git
Michael Haggerty <mhagger@alum.mit.edu> writes:
> Change the error message from
>
> Could not remove branch %s
>
> to
>
> Could not remove reference %s
>
> This change makes sense even for the existing caller, which uses the
> function to delete remote-tracking branches.
I am 80% convinced ;-)
The existing caller never used this for removing tags, so 'could not
remove branch' was equally correct for it and was more specific than
'could not remove reference'. If you change it to 'could not remove
that thing %s', it would still be correct for the existing caller;
it would be even less specific for them, though ;-)
The new callers you will add in later patch of course cannot live
with 'could not remove branch', so I think that this is an
acceptable compromise we can live with. If somebody later wants to
make the message more specific, they can add code that switches on
the prefix of the ref when coming up with the error message (and use
that code consistently in other error messages e.g. 'could not add
reference').
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> refs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/refs.c b/refs.c
> index c413282..2a2a06d 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2827,7 +2827,7 @@ int delete_refs(struct string_list *refnames)
> const char *refname = refnames->items[i].string;
>
> if (delete_ref(refname, NULL, 0))
> - result |= error(_("Could not remove branch %s"), refname);
> + result |= error(_("Could not remove reference %s"), refname);
> }
>
> return result;
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 01/13] delete_ref(): move declaration to refs.h
2015-06-09 16:42 ` Stefan Beller
@ 2015-06-13 14:30 ` Michael Haggerty
0 siblings, 0 replies; 30+ messages in thread
From: Michael Haggerty @ 2015-06-13 14:30 UTC (permalink / raw
To: Stefan Beller; +Cc: Junio C Hamano, Jeff King, git@vger.kernel.org
On 06/09/2015 06:42 PM, Stefan Beller wrote:
> On Tue, Jun 9, 2015 at 3:10 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> In set notation,
>>
>> "non-NULL" =
>> "non-NULL and not NULL_SHA1" ∪
>> "non-NULL and equal to NULL_SHA1"
>>
>> The latter two are *not* treated the same, so I don't see how we can
>> claim that "non-NULL" and "not NULL_SHA1" are treated the same. I must
>> be misunderstanding you.
>>
>> Would it help if I changed the comment to
>>
>> Delete the specified reference. If old_sha1 is non-NULL and not
>> NULL_SHA1, then verify that the current value of the reference is
>> old_sha1 before deleting it. If old_sha1 is NULL or NULL_SHA1,
>> delete the reference it it exists, regardless of its old value.
>>
>> ?
>
> This is very clear to me.
OK, I will make the change in v2 (with s/it it/if it/).
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2015-06-13 14:31 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-06-08 11:45 [PATCH 00/13] Improve "refs" module encapsulation Michael Haggerty
2015-06-08 11:45 ` [PATCH 01/13] delete_ref(): move declaration to refs.h Michael Haggerty
2015-06-08 16:43 ` Stefan Beller
2015-06-09 10:10 ` Michael Haggerty
2015-06-09 16:42 ` Stefan Beller
2015-06-13 14:30 ` Michael Haggerty
2015-06-08 11:45 ` [PATCH 02/13] remove_branches(): remove temporary Michael Haggerty
2015-06-08 16:45 ` Stefan Beller
2015-06-08 11:45 ` [PATCH 03/13] delete_ref(): handle special case more explicitly Michael Haggerty
2015-06-08 16:48 ` Stefan Beller
2015-06-09 10:17 ` Michael Haggerty
2015-06-08 11:45 ` [PATCH 04/13] delete_refs(): new function for the refs API Michael Haggerty
2015-06-08 11:45 ` [PATCH 05/13] delete_refs(): improve error message Michael Haggerty
2015-06-09 18:47 ` Junio C Hamano
2015-06-08 11:45 ` [PATCH 06/13] delete_refs(): convert error message to lower case Michael Haggerty
2015-06-08 16:51 ` Stefan Beller
2015-06-09 10:23 ` Michael Haggerty
2015-06-08 11:45 ` [PATCH 07/13] prune_remote(): use delete_refs() Michael Haggerty
2015-06-08 16:57 ` Stefan Beller
2015-06-08 17:12 ` Jeff King
2015-06-09 10:50 ` Michael Haggerty
2015-06-09 11:53 ` Jeff King
2015-06-08 11:45 ` [PATCH 08/13] repack_without_refs(): make function private Michael Haggerty
2015-06-08 11:45 ` [PATCH 09/13] initial_ref_transaction_commit(): function for initial ref creation Michael Haggerty
2015-06-08 11:45 ` [PATCH 10/13] refs: remove some functions from the module's public interface Michael Haggerty
2015-06-08 11:45 ` [PATCH 11/13] initial_ref_transaction_commit(): check for duplicate refs Michael Haggerty
2015-06-08 11:45 ` [PATCH 12/13] initial_ref_transaction_commit(): check for ref D/F conflicts Michael Haggerty
2015-06-08 11:45 ` [PATCH 13/13] refs: move the remaining ref module declarations to refs.h Michael Haggerty
2015-06-08 17:03 ` [PATCH 00/13] Improve "refs" module encapsulation Stefan Beller
2015-06-09 18:47 ` Junio C Hamano
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.