[PATCH v2] format-patch --signature-file
Added feature that allows a signature file to be used with format-patch.
$ git format-patch --signature-file ~/.signature -1
Now signatures with newlines and other special characters can be
easily included.
Signed-off-by: Jeremiah Mahler
---
Documentation/git-format-patch.txt | 7 +++
builtin/log.c | 24
t/t4014-format-patch.sh| 13 +
3 files changed, 44 insertions(+)
diff --git a/Documentation/git-format-patch.txt
b/Documentation/git-format-patch.txt
index 5c0a4ab..dd57841 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -14,6 +14,7 @@ SYNOPSIS
[(--attach|--inline)[=] | --no-attach]
[-s | --signoff]
[--signature= | --no-signature]
+ [--signature-file=]
[-n | --numbered | -N | --no-numbered]
[--start-number ] [--numbered-files]
[--in-reply-to=Message-Id] [--suffix=.]
@@ -233,6 +234,12 @@ configuration options in linkgit:git-notes[1] to use this
workflow).
signature option is omitted the signature defaults to the Git version
number.
+--signature-file=::
+ Add a signature, by including the contents of a file, to each message
+ produced. Per RFC 3676 the signature is separated from the body by a
+ line with '-- ' on it. If the signature option is omitted the signature
+ defaults to the Git version number.
+
--suffix=.::
Instead of using `.patch` as the suffix for generated
filenames, use specified suffix. A common alternative is
diff --git a/builtin/log.c b/builtin/log.c
index 39e8836..4a7308d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1147,6 +1147,27 @@ static int from_callback(const struct option *opt, const
char *arg, int unset)
return 0;
}
+static int signature_file_callback(const struct option *opt, const char *arg,
+ int unset)
+{
+ const char **signature = opt->value;
+ static char buf[1024];
+ size_t sz;
+ FILE *fd;
+
+ fd = fopen(arg, "r");
+ if (fd) {
+ sz = sizeof(buf);
+ sz = fread(buf, 1, sz - 1, fd);
+ if (sz) {
+ buf[sz] = '\0';
+ *signature = buf;
+ }
+ fclose(fd);
+ }
+ return 0;
+}
+
int cmd_format_patch(int argc, const char **argv, const char *prefix)
{
struct commit *commit;
@@ -1230,6 +1251,9 @@ int cmd_format_patch(int argc, const char **argv, const
char *prefix)
PARSE_OPT_OPTARG, thread_callback },
OPT_STRING(0, "signature", &signature, N_("signature"),
N_("add a signature")),
+ { OPTION_CALLBACK, 0, "signature-file", &signature,
N_("signature-file"),
+ N_("add a signature from contents of a file"),
+ PARSE_OPT_NONEG, signature_file_callback },
OPT__QUIET(&quiet, N_("don't print the patch filenames")),
OPT_END()
};
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 9c80633..19b67e3 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -762,6 +762,19 @@ test_expect_success 'format-patch --signature=""
suppresses signatures' '
! grep "^-- \$" output
'
+cat > expect << EOF
+Test User
+http://git.kernel.org/cgit/git/git.git
+git.kernel.org/?p=git/git.git;a=summary
+EOF
+
+test_expect_success 'format-patch --signature-file file' '
+ git format-patch --stdout --signature-file expect -1 >output &&
+ check_patch output &&
+ fgrep -x -f output expect >output2 &&
+ diff expect output2
+'
+
test_expect_success TTY 'format-patch --stdout paginates' '
rm -f pager_used &&
test_terminal env GIT_PAGER="wc >pager_used" git format-patch --stdout
--all &&
--
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] format-patch --signature-file
On Tue, May 13, 2014 at 09:07:12AM -0700, Jonathan Nieder wrote: > Hi, > > Jeremiah Mahler wrote: > > > # from a string > > $ git format-patch --signature "from a string" origin > > > > # or from a file > > $ git format-patch --signature ~/.signature origin > > Interesting. But... what if I want my patch to end with > > -- > /home/jrnieder/.signature > > ? It seems safer to introduce a separate --signature-file option. > It is probably smarter to avoid that corner case entirely. Good idea. > [...] > > builtin/log.c | 26 -- > > 1 file changed, 24 insertions(+), 2 deletions(-) > > Tests? > I added a test which checks that a valid patch is produced and that the signature from the file appears in the output. > Thanks and hope that helps, > Jonathan In addition to addressing the suggestions from Jonathan I also updated the Documentation. This solution uses a static buffer to store the signature which does create a size limitation (1024 bytes). I considered a solution using malloc but I could not figure out a clean way of determining when to free the memory. Thanks for the help and suggestions. Jeremiah Mahler (1): format-patch --signature-file Documentation/git-format-patch.txt | 7 +++ builtin/log.c | 24 t/t4014-format-patch.sh| 13 + 3 files changed, 44 insertions(+) -- Jeremiah Mahler jmmah...@gmail.com http://github.com/jmahler -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] Gitweb: Convert UTF-8 encoded file names
Jakub Narębski writes: > On Thu, May 15, 2014 at 9:38 PM, Junio C Hamano wrote: >> Jakub Narębski writes: >> >>> Writing test for this would not be easy, and require some HTML >>> parser (WWW::Mechanize, Web::Scraper, HTML::Query, pQuery, >>> ... or low level HTML::TreeBuilder, or other low level parser). >> >> Hmph. Is it more than just looking for a specific run of %xx we >> would expect to see in the output of the tree view for a repository >> in which there is one tree with non-ASCII name? > > There is if we want to check (in non-fragile way) that said > specific run is in 'href' *attribute* of 'a' element (link target). Correct, but is "where does it appear" the question we are primarily interested in, wrt this breakage and its fix? If gitweb output has some volatile parts that do not depend on the contents of the Git test repository (e.g. showing contents of /etc/motd, date/time of when the test was run, or the full pathname leading to the trash directory), then preparing a tree whose name is äéìõû and making sure that the properly encoded version of äéìõû appears anywhere in the output may not be sufficient to validate that we got the encoding right, as that string may appear in the parts that are totally unrelated to the contents being shown and not under our control. But is that really the case? Also we may introduce a bug and misspell the attr name and produce an anchor element with hpef attribute with the properly encoded URL in it, and your "parse HTML properly" approach would catch it, but is that the kind of breakage under discussion? You hinted at new tests for UTF-8 encoding in the other message in the thread earlier, and I've been assuming that we were talking about the encoding test, not a test to catch s/href/hpef/ kind of breakage. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 21/44] refs.c: ref_transaction_commit should not free the transaction
Ronnie Sahlberg wrote: > Change ref_transaction_commit so that it does not free the transaction. > Instead require that a caller will end a transaction by either calling > ref_transaction_rollback or ref_transaction_free. Can I always use ref_transaction_rollback instead of ref_transaction_free? It would be convenient if my cleanup code could always call _rollback instead of having to do something different for success and errors. Another way to ask the question: what is it valid to do with a transaction after commiting? Thanks, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GUILT v2 15/29] Produce legal patch names in guilt-import-commit.
On Tue, May 13, 2014 at 10:30:51PM +0200, Per Cederqvist wrote: > Try harder to create patch names that adhere to the rules in > git-check-ref-format(1) when deriving a patch name from the commit > message. Verify that the derived name using "git check-ref-format", > and as a final fallback simply use the patch name "x" (to ensure that > the code is future-proof in case new rules are added in the future). > > Always append a ".patch" suffix to the patch name. > > Added test cases. Also very nice. Thanks, Jeff. Signed-off-by: Josef 'Jeff' Sipek -- We have joy, we have fun, we have Linux on a Sun... -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 10/44] refs.c: change ref_transaction_update() to do error checking and return status
Update ref_transaction_update() do some basic error checking and return
non-zero on error. Update all callers to check ref_transaction_update() for
error. There are currently no conditions in _update that will return error but
there will be in the future. Add an err argument that will be updated on
failure.
Also check for BUGs during update and die(BUG:...) if we are calling
_update with have_old but the old_sha1 pointer is NULL.
Signed-off-by: Ronnie Sahlberg
---
builtin/update-ref.c | 12 +++-
refs.c | 18 --
refs.h | 14 +-
3 files changed, 28 insertions(+), 16 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 2bef2a0..9f328b2 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -16,6 +16,7 @@ static struct ref_transaction *transaction;
static char line_termination = '\n';
static int update_flags;
+static struct strbuf err = STRBUF_INIT;
/*
* Parse one whitespace- or NUL-terminated, possibly C-quoted argument
@@ -197,8 +198,9 @@ static const char *parse_cmd_update(struct strbuf *input,
const char *next)
if (*next != line_termination)
die("update %s: extra input: %s", refname, next);
- ref_transaction_update(transaction, refname, new_sha1, old_sha1,
- update_flags, have_old);
+ if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+ update_flags, have_old, &err))
+ die("update %s: failed: %s", refname, err.buf);
update_flags = 0;
free(refname);
@@ -286,8 +288,9 @@ static const char *parse_cmd_verify(struct strbuf *input,
const char *next)
if (*next != line_termination)
die("verify %s: extra input: %s", refname, next);
- ref_transaction_update(transaction, refname, new_sha1, old_sha1,
- update_flags, have_old);
+ if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+ update_flags, have_old, &err))
+ die("verify %s: %s", refname, err.buf);
update_flags = 0;
free(refname);
@@ -342,7 +345,6 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
const char *refname, *oldval, *msg = NULL;
unsigned char sha1[20], oldsha1[20];
int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;
- struct strbuf err = STRBUF_INIT;
struct option options[] = {
OPT_STRING( 'm', NULL, &msg, N_("reason"), N_("reason of the
update")),
OPT_BOOL('d', NULL, &delete, N_("delete the reference")),
diff --git a/refs.c b/refs.c
index 3eb73a3..3c9b981 100644
--- a/refs.c
+++ b/refs.c
@@ -3381,19 +3381,25 @@ static struct ref_update *add_update(struct
ref_transaction *transaction,
return update;
}
-void ref_transaction_update(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *new_sha1,
- const unsigned char *old_sha1,
- int flags, int have_old)
+int ref_transaction_update(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *new_sha1,
+ const unsigned char *old_sha1,
+ int flags, int have_old,
+ struct strbuf *err)
{
- struct ref_update *update = add_update(transaction, refname);
+ struct ref_update *update;
+
+ if (have_old && !old_sha1)
+ die("BUG: have_old is true but old_sha1 is NULL");
+ update = add_update(transaction, refname);
hashcpy(update->new_sha1, new_sha1);
update->flags = flags;
update->have_old = have_old;
if (have_old)
hashcpy(update->old_sha1, old_sha1);
+ return 0;
}
void ref_transaction_create(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index 555ee59..11d07ee 100644
--- a/refs.h
+++ b/refs.h
@@ -242,12 +242,16 @@ void ref_transaction_rollback(struct ref_transaction
*transaction);
* be deleted. If have_old is true, then old_sha1 holds the value
* that the reference should have had before the update, or zeros if
* it must not have existed beforehand.
+ * Function returns 0 on success and non-zero on failure. A failure to update
+ * means that the transaction as a whole has failed and will need to be
+ * rolled back. On failure the err buffer will be updated.
*/
-void ref_transaction_update(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *new_sha1,
- const unsigned char *old_sha1,
- int flags, int have_old);
+int ref_transaction_update(struct ref_transaction *t
[PATCH v9 32/44] refs.c: remove the update_ref_write function
Since we only call update_ref_write from a single place and we only call it
with onerr==QUIET_ON_ERR we can just as well get rid of it and just call
write_ref_sha1 directly.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 35 +--
1 file changed, 9 insertions(+), 26 deletions(-)
diff --git a/refs.c b/refs.c
index 3b0d574..1857e61 100644
--- a/refs.c
+++ b/refs.c
@@ -3283,25 +3283,6 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
return retval;
}
-static int update_ref_write(const char *action, const char *refname,
- const unsigned char *sha1, struct ref_lock *lock,
- struct strbuf *err, enum action_on_err onerr)
-{
- if (write_ref_sha1(lock, sha1, action) < 0) {
- const char *str = "Cannot update the ref '%s'.";
- if (err)
- strbuf_addf(err, str, refname);
-
- switch (onerr) {
- case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
- case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
- case UPDATE_REFS_QUIET_ON_ERR: break;
- }
- return 1;
- }
- return 0;
-}
-
/**
* Information needed for a single ref update. Set new_sha1 to the
* new value or to zero to delete the ref. To check the old value
@@ -3549,14 +3530,16 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (!is_null_sha1(update->new_sha1)) {
- ret = update_ref_write(msg,
- update->refname,
- update->new_sha1,
- update->lock, err,
- UPDATE_REFS_QUIET_ON_ERR);
- update->lock = NULL; /* freed by update_ref_write */
- if (ret)
+ ret = write_ref_sha1(update->lock, update->new_sha1,
+msg);
+ update->lock = NULL; /* freed by write_ref_sha1 */
+ if (ret) {
+ const char *str = "Cannot update the ref '%s'.";
+
+ if (err)
+ strbuf_addf(err, str, update->refname);
goto cleanup;
+ }
}
}
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 19/44] refs.c: change update_ref to use a transaction
Change the update_ref helper function to use a ref transaction internally.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 25 +
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index 7276b9f..5e82695 100644
--- a/refs.c
+++ b/refs.c
@@ -3447,11 +3447,28 @@ int update_ref(const char *action, const char *refname,
const unsigned char *sha1, const unsigned char *oldval,
int flags, enum action_on_err onerr)
{
- struct ref_lock *lock;
- lock = update_ref_lock(refname, oldval, flags, NULL, onerr);
- if (!lock)
+ struct ref_transaction *t;
+ struct strbuf err = STRBUF_INIT;
+
+ t = ref_transaction_begin();
+ if ((!t ||
+ ref_transaction_update(t, refname, sha1, oldval, flags,
+ !!oldval, &err)) ||
+ (ref_transaction_commit(t, action, &err) && !(t = NULL))) {
+ const char *str = "update_ref failed for ref '%s': %s";
+
+ ref_transaction_rollback(t);
+ switch (onerr) {
+ case UPDATE_REFS_MSG_ON_ERR:
+ error(str, refname, err.buf); break;
+ case UPDATE_REFS_DIE_ON_ERR:
+ die(str, refname, err.buf); break;
+ case UPDATE_REFS_QUIET_ON_ERR: break;
+ }
+ strbuf_release(&err);
return 1;
- return update_ref_write(action, refname, sha1, lock, NULL, onerr);
+ }
+ return 0;
}
static int ref_update_compare(const void *r1, const void *r2)
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 20/44] refs.c: free the transaction before returning when number of updates is 0
We have to free the transaction before returning in the early check for
'return early if number of updates == 0' or else the following code would
create a memory leak with the transaction never being freed :
t = ref_transaction_begin()
ref_transaction_commit(t)
Signed-off-by: Ronnie Sahlberg
---
refs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 5e82695..b4c7d04 100644
--- a/refs.c
+++ b/refs.c
@@ -3502,8 +3502,10 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
int n = transaction->nr;
struct ref_update **updates = transaction->updates;
- if (!n)
+ if (!n) {
+ ref_transaction_free(transaction);
return 0;
+ }
/* Allocate work space */
delnames = xmalloc(sizeof(*delnames) * n);
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 41/44] refs.c: add a new flag for transaction delete for refs we know are packed only
Add a new flag REF_ISPACKONLY that we can use in ref_transaction_delete.
This flag indicates that the ref does not exist as a loose ref andf only as
a packed ref. If this is the case we then change the commit code so that
we skip taking out a lock file and we skip calling delete_ref_loose.
Check for this flag and die(BUG:...) if used with _update or _create.
At the start of the transaction, before we even start locking any refs,
we add all such REF_ISPACKONLY refs to delnames so that we have a list of
all pack only refs that we will be deleting during this transaction.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 23 +++
1 file changed, 23 insertions(+)
diff --git a/refs.c b/refs.c
index 87113af..6299b3d 100644
--- a/refs.c
+++ b/refs.c
@@ -33,6 +33,10 @@ static inline int bad_ref_char(int ch)
* pruned.
*/
#define REF_ISPRUNING 0x0100
+/** Deletion of a ref that only exists as a packed ref in which case we do not
+ * need to lock the loose ref during the transaction.
+ */
+#define REF_ISPACKONLY 0x0200
/*
* Try to read one refname component from the front of refname. Return
@@ -3376,6 +3380,9 @@ int ref_transaction_update(struct ref_transaction
*transaction,
if (transaction->status != REF_TRANSACTION_OPEN)
die("BUG: update on transaction that is not open");
+ if (flags & REF_ISPACKONLY)
+ die("BUG: REF_ISPACKONLY can not be used with updates");
+
update = add_update(transaction, refname);
hashcpy(update->new_sha1, new_sha1);
update->flags = flags;
@@ -3402,6 +3409,9 @@ int ref_transaction_create(struct ref_transaction
*transaction,
if (transaction->status != REF_TRANSACTION_OPEN)
die("BUG: create on transaction that is not open");
+ if (flags & REF_ISPACKONLY)
+ die("BUG: REF_ISPACKONLY can not be used with creates");
+
update = add_update(transaction, refname);
hashcpy(update->new_sha1, new_sha1);
@@ -3517,10 +3527,20 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
if (ret)
goto cleanup;
+ for (i = 0; i < n; i++) {
+ struct ref_update *update = updates[i];
+
+ if (update->flags & REF_ISPACKONLY)
+ delnames[delnum++] = update->refname;
+ }
+
/* Acquire all locks while verifying old values */
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
+ if (update->flags & REF_ISPACKONLY)
+ continue;
+
update->lock = lock_ref_sha1_basic(update->refname,
(update->have_old ?
update->old_sha1 :
@@ -3558,6 +3578,9 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
+ if (update->flags & REF_ISPACKONLY)
+ continue;
+
if (update->lock) {
ret |= delete_ref_loose(update->lock, update->type,
err);
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 15/44] commit.c: use ref transactions for updates
Change commit.c to use ref transactions for all ref updates.
Make sure we pass a NULL pointer to ref_transaction_update if have_old
is false.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
builtin/commit.c | 23 ++-
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index d28505a..c429216 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1581,11 +1581,12 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
const char *index_file, *reflog_msg;
char *nl;
unsigned char sha1[20];
- struct ref_lock *ref_lock;
struct commit_list *parents = NULL, **pptr = &parents;
struct stat statbuf;
struct commit *current_head = NULL;
struct commit_extra_header *extra = NULL;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
if (argc == 2 && !strcmp(argv[1], "-h"))
usage_with_options(builtin_commit_usage,
builtin_commit_options);
@@ -1707,16 +1708,6 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
strbuf_release(&author_ident);
free_commit_extra_headers(extra);
- ref_lock = lock_any_ref_for_update("HEAD",
- !current_head
- ? NULL
- : current_head->object.sha1,
- 0, NULL);
- if (!ref_lock) {
- rollback_index_files();
- die(_("cannot lock HEAD ref"));
- }
-
nl = strchr(sb.buf, '\n');
if (nl)
strbuf_setlen(&sb, nl + 1 - sb.buf);
@@ -1725,9 +1716,15 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));
strbuf_insert(&sb, strlen(reflog_msg), ": ", 2);
- if (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_update(transaction, "HEAD", sha1,
+ current_head ?
+ current_head->object.sha1 : NULL,
+ 0, !!current_head, &err) ||
+ ref_transaction_commit(transaction, sb.buf, &err)) {
rollback_index_files();
- die(_("cannot update HEAD ref"));
+ die("%s", err.buf);
}
unlink(git_path("CHERRY_PICK_HEAD"));
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 35/44] refs.c: make delete_ref use a transaction
Change delete_ref to use a ref transaction for the deletion. At the same time
since we no longer have any callers of repack_without_ref we can now delete
this function.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 31 ++-
1 file changed, 10 insertions(+), 21 deletions(-)
diff --git a/refs.c b/refs.c
index f88263e..7efb6bb 100644
--- a/refs.c
+++ b/refs.c
@@ -2495,11 +2495,6 @@ static int repack_without_refs(const char **refnames,
int n, struct strbuf *err)
return ret;
}
-static int repack_without_ref(const char *refname)
-{
- return repack_without_refs(&refname, 1, NULL);
-}
-
static int add_err_if_unremovable(const char *op, const char *file,
struct strbuf *e, int rc)
{
@@ -2543,24 +2538,18 @@ static int delete_ref_loose(struct ref_lock *lock, int
flag, struct strbuf *err)
int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
{
- struct ref_lock *lock;
- int ret = 0, flag = 0;
+ struct ref_transaction *transaction;
- lock = lock_ref_sha1_basic(refname, sha1, delopt, &flag);
- if (!lock)
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_delete(transaction, refname, sha1, delopt,
+ sha1 && !is_null_sha1(sha1), NULL) ||
+ ref_transaction_commit(transaction, NULL, NULL)) {
+ ref_transaction_rollback(transaction);
return 1;
- ret |= delete_ref_loose(lock, flag, NULL);
-
- /* removing the loose one could have resurrected an earlier
-* packed one. Also, if it was not loose we need to repack
-* without it.
-*/
- ret |= repack_without_ref(lock->ref_name);
-
- unlink_or_warn(git_path("logs/%s", lock->ref_name));
- clear_loose_ref_cache(&ref_cache);
- unlock_ref(lock);
- return ret;
+ }
+ ref_transaction_free(transaction);
+ return 0;
}
/*
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 12/44] refs.c: ref_transaction_delete to check for error and return status
Change ref_transaction_delete() to do basic error checking and return
non-zero of error. Update all callers to check the return for
ref_transaction_delete(). There are currently no conditions in _delete that
will return error but there will be in the future. Add an err argument that
will be updated on failure.
Signed-off-by: Ronnie Sahlberg
---
builtin/update-ref.c | 5 +++--
refs.c | 16 +++-
refs.h | 12
3 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index e9c216e..cdb71a8 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -258,8 +258,9 @@ static const char *parse_cmd_delete(struct strbuf *input,
const char *next)
if (*next != line_termination)
die("delete %s: extra input: %s", refname, next);
- ref_transaction_delete(transaction, refname, old_sha1,
- update_flags, have_old);
+ if (ref_transaction_delete(transaction, refname, old_sha1,
+ update_flags, have_old, NULL))
+ die("failed transaction delete for %s", refname);
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index 344e27a..7276b9f 100644
--- a/refs.c
+++ b/refs.c
@@ -3422,19 +3422,25 @@ int ref_transaction_create(struct ref_transaction
*transaction,
return 0;
}
-void ref_transaction_delete(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *old_sha1,
- int flags, int have_old)
+int ref_transaction_delete(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *old_sha1,
+ int flags, int have_old,
+ struct strbuf *err)
{
- struct ref_update *update = add_update(transaction, refname);
+ struct ref_update *update;
+ if (have_old && !old_sha1)
+ die("BUG: have_old is true but old_sha1 is NULL");
+
+ update = add_update(transaction, refname);
update->flags = flags;
update->have_old = have_old;
if (have_old) {
assert(!is_null_sha1(old_sha1));
hashcpy(update->old_sha1, old_sha1);
}
+ return 0;
}
int update_ref(const char *action, const char *refname,
diff --git a/refs.h b/refs.h
index 0af5ba7..2c2e694 100644
--- a/refs.h
+++ b/refs.h
@@ -272,11 +272,15 @@ int ref_transaction_create(struct ref_transaction
*transaction,
* Add a reference deletion to transaction. If have_old is true, then
* old_sha1 holds the value that the reference should have had before
* the update (which must not be the null SHA-1).
+ * Function returns 0 on success and non-zero on failure. A failure to delete
+ * means that the transaction as a whole has failed and will need to be
+ * rolled back. On failure the err buffer will be updated.
*/
-void ref_transaction_delete(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *old_sha1,
- int flags, int have_old);
+int ref_transaction_delete(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *old_sha1,
+ int flags, int have_old,
+ struct strbuf *err);
/*
* Commit all of the changes that have been queued in transaction, as
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 30/44] refs.c: add transaction.status and track OPEN/CLOSED/ERROR
Track the status of a transaction in a new status field. Check the field for
sanity, i.e. that status must be OPEN when _commit/_create/_delete or
_update is called or else die(BUG:...)
Signed-off-by: Ronnie Sahlberg
---
refs.c | 31 ++-
1 file changed, 30 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 733dfb3..3fb5cb8 100644
--- a/refs.c
+++ b/refs.c
@@ -3336,6 +3336,12 @@ struct ref_update {
const char refname[FLEX_ARRAY];
};
+enum ref_transaction_status {
+ REF_TRANSACTION_OPEN = 0,
+ REF_TRANSACTION_CLOSED = 1,
+ REF_TRANSACTION_ERROR = 2,
+};
+
/*
* Data structure for holding a reference transaction, which can
* consist of checks and updates to multiple references, carried out
@@ -3345,6 +3351,7 @@ struct ref_transaction {
struct ref_update **updates;
size_t alloc;
size_t nr;
+ enum ref_transaction_status status;
};
struct ref_transaction *ref_transaction_begin(void)
@@ -3368,6 +3375,11 @@ void ref_transaction_free(struct ref_transaction
*transaction)
void ref_transaction_rollback(struct ref_transaction *transaction)
{
+ if (!transaction)
+ return;
+
+ transaction->status = REF_TRANSACTION_ERROR;
+
ref_transaction_free(transaction);
}
@@ -3395,6 +3407,9 @@ int ref_transaction_update(struct ref_transaction
*transaction,
if (have_old && !old_sha1)
die("BUG: have_old is true but old_sha1 is NULL");
+ if (transaction->status != REF_TRANSACTION_OPEN)
+ die("BUG: update on transaction that is not open");
+
update = add_update(transaction, refname);
hashcpy(update->new_sha1, new_sha1);
update->flags = flags;
@@ -3415,6 +3430,9 @@ int ref_transaction_create(struct ref_transaction
*transaction,
if (!new_sha1 || is_null_sha1(new_sha1))
die("BUG: create ref with null new_sha1");
+ if (transaction->status != REF_TRANSACTION_OPEN)
+ die("BUG: create on transaction that is not open");
+
update = add_update(transaction, refname);
hashcpy(update->new_sha1, new_sha1);
@@ -3435,6 +3453,9 @@ int ref_transaction_delete(struct ref_transaction
*transaction,
if (have_old && !old_sha1)
die("BUG: have_old is true but old_sha1 is NULL");
+ if (transaction->status != REF_TRANSACTION_OPEN)
+ die("BUG: delete on transaction that is not open");
+
update = add_update(transaction, refname);
update->flags = flags;
update->have_old = have_old;
@@ -3505,8 +3526,13 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
int n = transaction->nr;
struct ref_update **updates = transaction->updates;
- if (!n)
+ if (transaction->status != REF_TRANSACTION_OPEN)
+ die("BUG: commit on transaction that is not open");
+
+ if (!n) {
+ transaction->status = REF_TRANSACTION_CLOSED;
return 0;
+ }
/* Allocate work space */
delnames = xmalloc(sizeof(*delnames) * n);
@@ -3569,6 +3595,9 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
clear_loose_ref_cache(&ref_cache);
cleanup:
+ transaction->status = ret ? REF_TRANSACTION_ERROR
+ : REF_TRANSACTION_CLOSED;
+
for (i = 0; i < n; i++)
if (updates[i]->lock)
unlock_ref(updates[i]->lock);
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 07/44] refs.c: make update_ref_write update a strbuf on failure
Change update_ref_write to also update an error strbuf on failure.
This makes the error available to ref_transaction_commit callers if the
transaction failed due to update_ref_sha1/write_ref_sha1 failures.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
refs.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/refs.c b/refs.c
index e4d35c9..e8756fd 100644
--- a/refs.c
+++ b/refs.c
@@ -3301,10 +3301,13 @@ static struct ref_lock *update_ref_lock(const char
*refname,
static int update_ref_write(const char *action, const char *refname,
const unsigned char *sha1, struct ref_lock *lock,
- enum action_on_err onerr)
+ struct strbuf *err, enum action_on_err onerr)
{
if (write_ref_sha1(lock, sha1, action) < 0) {
const char *str = "Cannot update the ref '%s'.";
+ if (err)
+ strbuf_addf(err, str, refname);
+
switch (onerr) {
case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
@@ -3430,7 +3433,7 @@ int update_ref(const char *action, const char *refname,
lock = update_ref_lock(refname, oldval, flags, NULL, onerr);
if (!lock)
return 1;
- return update_ref_write(action, refname, sha1, lock, onerr);
+ return update_ref_write(action, refname, sha1, lock, NULL, onerr);
}
static int ref_update_compare(const void *r1, const void *r2)
@@ -3512,7 +3515,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
ret = update_ref_write(msg,
update->refname,
update->new_sha1,
- update->lock, onerr);
+ update->lock, err, onerr);
update->lock = NULL; /* freed by update_ref_write */
if (ret)
goto cleanup;
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 31/44] refs.c: remove the update_ref_lock function
Since we now only call update_ref_lock with onerr==QUIET_ON_ERR we no longer
need this function and can replace it with just calling lock_any_ref_for_update
directly.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 30 ++
1 file changed, 6 insertions(+), 24 deletions(-)
diff --git a/refs.c b/refs.c
index 3fb5cb8..3b0d574 100644
--- a/refs.c
+++ b/refs.c
@@ -3283,24 +3283,6 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
return retval;
}
-static struct ref_lock *update_ref_lock(const char *refname,
- const unsigned char *oldval,
- int flags, int *type_p,
- enum action_on_err onerr)
-{
- struct ref_lock *lock;
- lock = lock_any_ref_for_update(refname, oldval, flags, type_p);
- if (!lock) {
- const char *str = "Cannot lock the ref '%s'.";
- switch (onerr) {
- case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
- case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
- case UPDATE_REFS_QUIET_ON_ERR: break;
- }
- }
- return lock;
-}
-
static int update_ref_write(const char *action, const char *refname,
const unsigned char *sha1, struct ref_lock *lock,
struct strbuf *err, enum action_on_err onerr)
@@ -3547,12 +3529,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
- update->lock = update_ref_lock(update->refname,
- (update->have_old ?
- update->old_sha1 : NULL),
- update->flags,
- &update->type,
- UPDATE_REFS_QUIET_ON_ERR);
+ update->lock = lock_any_ref_for_update(update->refname,
+ (update->have_old ?
+ update->old_sha1 :
+ NULL),
+ update->flags,
+ &update->type);
if (!update->lock) {
if (err)
strbuf_addf(err, "Cannot lock the ref '%s'.",
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 29/44] refs.c: make lock_ref_sha1 static
No external callers reference lock_ref_sha1 any more so lets declare it static.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 2 +-
refs.h | 3 ---
2 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index 6151c3c..733dfb3 100644
--- a/refs.c
+++ b/refs.c
@@ -2126,7 +2126,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
return NULL;
}
-struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char
*old_sha1)
+static struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char
*old_sha1)
{
char refpath[PATH_MAX];
if (check_refname_format(refname, 0))
diff --git a/refs.h b/refs.h
index a45712c..832d81b 100644
--- a/refs.h
+++ b/refs.h
@@ -132,9 +132,6 @@ extern int ref_exists(const char *);
*/
extern int peel_ref(const char *refname, unsigned char *sha1);
-/** Locks a "refs/" ref returning the lock on success and NULL on failure. **/
-extern struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char
*old_sha1);
-
/** Locks any ref (for 'HEAD' type refs). */
#define REF_NODEREF0x01
extern struct ref_lock *lock_any_ref_for_update(const char *refname,
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 08/44] update-ref.c: log transaction error from the update_ref
Call ref_transaction_commit with QUIET_ON_ERR and use the strbuf that is
returned to print a log message if/after the transaction fails.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
builtin/update-ref.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index aaa06aa..207e24d 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -342,6 +342,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
const char *refname, *oldval, *msg = NULL;
unsigned char sha1[20], oldsha1[20];
int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;
+ struct strbuf err = STRBUF_INIT;
struct option options[] = {
OPT_STRING( 'm', NULL, &msg, N_("reason"), N_("reason of the
update")),
OPT_BOOL('d', NULL, &delete, N_("delete the reference")),
@@ -359,17 +360,16 @@ int cmd_update_ref(int argc, const char **argv, const
char *prefix)
die("Refusing to perform update with empty message.");
if (read_stdin) {
- int ret;
transaction = ref_transaction_begin();
-
if (delete || no_deref || argc > 0)
usage_with_options(git_update_ref_usage, options);
if (end_null)
line_termination = '\0';
update_refs_stdin();
- ret = ref_transaction_commit(transaction, msg, NULL,
-UPDATE_REFS_DIE_ON_ERR);
- return ret;
+ if (ref_transaction_commit(transaction, msg, &err,
+ UPDATE_REFS_QUIET_ON_ERR))
+ die("%s", err.buf);
+ return 0;
}
if (end_null)
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 25/44] receive-pack.c: use a reference transaction for updating the refs
Wrap all the ref updates inside a transaction to make the update atomic.
Signed-off-by: Ronnie Sahlberg
---
builtin/receive-pack.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c323081..5534138 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -46,6 +46,8 @@ static void *head_name_to_free;
static int sent_capabilities;
static int shallow_update;
static const char *alt_shallow_file;
+static struct strbuf err = STRBUF_INIT;
+static struct ref_transaction *transaction;
static enum deny_action parse_deny_action(const char *var, const char *value)
{
@@ -475,7 +477,6 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
const char *namespaced_name;
unsigned char *old_sha1 = cmd->old_sha1;
unsigned char *new_sha1 = cmd->new_sha1;
- struct ref_lock *lock;
/* only refs/... are allowed */
if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
@@ -580,15 +581,9 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
update_shallow_ref(cmd, si))
return "shallow error";
- lock = lock_any_ref_for_update(namespaced_name, old_sha1,
- 0, NULL);
- if (!lock) {
- rp_error("failed to lock %s", name);
- return "failed to lock";
- }
- if (write_ref_sha1(lock, new_sha1, "push")) {
- return "failed to write"; /* error() already called */
- }
+ if (ref_transaction_update(transaction, namespaced_name,
+ new_sha1, old_sha1, 0, 1, &err))
+ return "failed to update";
return NULL; /* good */
}
}
@@ -812,6 +807,7 @@ static void execute_commands(struct command *commands,
head_name = head_name_to_free = resolve_refdup("HEAD", sha1, 0, NULL);
checked_connectivity = 1;
+ transaction = ref_transaction_begin();
for (cmd = commands; cmd; cmd = cmd->next) {
if (cmd->error_string)
continue;
@@ -827,6 +823,10 @@ static void execute_commands(struct command *commands,
checked_connectivity = 0;
}
}
+ if (ref_transaction_commit(transaction, "push", &err))
+ error("%s", err.buf);
+ ref_transaction_free(transaction);
+ strbuf_release(&err);
if (shallow_update && !checked_connectivity)
error("BUG: run 'git fsck' for safety.\n"
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 02/44] refs.c: allow passing NULL to ref_transaction_free
Allow ref_transaction_free(NULL) and hence ref_transaction_rollback(NULL)
as no-ops. This makes ref_transaction_rollback easier to use and more similar
to plain 'free'.
In particular, it lets us rollback unconditionally as part of cleanup code
after setting 'transaction = NULL' if a transaction has been committed or
rolled back already.
This allows us to write code like
if ( (!transaction ||
ref_transaction_update(...)) ||
(ref_transaction_commit(...) && !(transaction = NULL)) {
ref_transaction_rollback(transaction);
...
}
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
refs.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/refs.c b/refs.c
index 2114748..88d73c8 100644
--- a/refs.c
+++ b/refs.c
@@ -3312,6 +3312,9 @@ static void ref_transaction_free(struct ref_transaction
*transaction)
{
int i;
+ if (!transaction)
+ return;
+
for (i = 0; i < transaction->nr; i++)
free(transaction->updates[i]);
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 13/44] tag.c: use ref transactions when doing updates
Change tag.c to use ref transactions for all ref updates.
Signed-off-by: Ronnie Sahlberg
---
builtin/tag.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index c6e8a71..b05f9a5 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -548,7 +548,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
struct strbuf ref = STRBUF_INIT;
unsigned char object[20], prev[20];
const char *object_ref, *tag;
- struct ref_lock *lock;
struct create_tag_options opt;
char *cleanup_arg = NULL;
int annotate = 0, force = 0, lines = -1;
@@ -556,6 +555,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
const char *msgfile = NULL, *keyid = NULL;
struct msg_arg msg = { 0, STRBUF_INIT };
struct commit_list *with_commit = NULL;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
struct option options[] = {
OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
{ OPTION_INTEGER, 'n', NULL, &lines, N_("n"),
@@ -701,11 +702,12 @@ int cmd_tag(int argc, const char **argv, const char
*prefix)
if (annotate)
create_tag(object, tag, &buf, &opt, prev, object);
- lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL);
- if (!lock)
- die(_("%s: cannot lock the ref"), ref.buf);
- if (write_ref_sha1(lock, object, NULL) < 0)
- die(_("%s: cannot update the ref"), ref.buf);
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_update(transaction, ref.buf, object, prev,
+ 0, !is_null_sha1(prev), &err) ||
+ ref_transaction_commit(transaction, NULL, &err))
+ die("%s", err.buf);
if (force && !is_null_sha1(prev) && hashcmp(prev, object))
printf(_("Updated tag '%s' (was %s)\n"), tag,
find_unique_abbrev(prev, DEFAULT_ABBREV));
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 44/44] refs.c: remove forward declaration of write_ref_sha1
Signed-off-by: Ronnie Sahlberg
---
refs.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/refs.c b/refs.c
index 7ea3f42..8e30511 100644
--- a/refs.c
+++ b/refs.c
@@ -260,8 +260,6 @@ struct ref_entry {
};
static void read_loose_refs(const char *dirname, struct ref_dir *dir);
-static int write_ref_sha1(struct ref_lock *lock,
- const unsigned char *sha1, const char *logmsg);
static struct ref_dir *get_ref_dir(struct ref_entry *entry)
{
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 05/44] refs.c: make ref_update_reject_duplicates take a strbuf argument for errors
Make ref_update_reject_duplicates return any error that occurs through a
new strbuf argument. This means that when a transaction commit fails in
this function we will now be able to pass a helpful error message back to the
caller.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
refs.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 66938d7..4ebf130 100644
--- a/refs.c
+++ b/refs.c
@@ -3415,6 +3415,7 @@ static int ref_update_compare(const void *r1, const void
*r2)
}
static int ref_update_reject_duplicates(struct ref_update **updates, int n,
+ struct strbuf *err,
enum action_on_err onerr)
{
int i;
@@ -3422,6 +3423,9 @@ static int ref_update_reject_duplicates(struct ref_update
**updates, int n,
if (!strcmp(updates[i - 1]->refname, updates[i]->refname)) {
const char *str =
"Multiple updates for ref '%s' not allowed.";
+ if (err)
+ strbuf_addf(err, str, updates[i]->refname);
+
switch (onerr) {
case UPDATE_REFS_MSG_ON_ERR:
error(str, updates[i]->refname); break;
@@ -3452,7 +3456,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Copy, sort, and reject duplicate refs */
qsort(updates, n, sizeof(*updates), ref_update_compare);
- ret = ref_update_reject_duplicates(updates, n, onerr);
+ ret = ref_update_reject_duplicates(updates, n, err, onerr);
if (ret)
goto cleanup;
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 23/44] fetch.c: change s_update_ref to use a ref transaction
Change s_update_ref to use a ref transaction for the ref update.
Signed-off-by: Ronnie Sahlberg
---
builtin/fetch.c | 17 +
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index a93c893..c9b5954 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -375,7 +375,7 @@ static int s_update_ref(const char *action,
{
char msg[1024];
char *rla = getenv("GIT_REFLOG_ACTION");
- static struct ref_lock *lock;
+ struct ref_transaction *transaction;
if (dry_run)
return 0;
@@ -384,15 +384,16 @@ static int s_update_ref(const char *action,
snprintf(msg, sizeof(msg), "%s: %s", rla, action);
errno = 0;
- lock = lock_any_ref_for_update(ref->name,
- check_old ? ref->old_sha1 : NULL,
- 0, NULL);
- if (!lock)
- return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
- STORE_REF_ERROR_OTHER;
- if (write_ref_sha1(lock, ref->new_sha1, msg) < 0)
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_update(transaction, ref->name, ref->new_sha1,
+ ref->old_sha1, 0, check_old, NULL) ||
+ ref_transaction_commit(transaction, msg, NULL)) {
+ ref_transaction_rollback(transaction);
return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
STORE_REF_ERROR_OTHER;
+ }
+ ref_transaction_free(transaction);
return 0;
}
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 03/44] refs.c: add a strbuf argument to ref_transaction_commit for error logging
Add a strbuf argument to _commit so that we can pass an error string back to
the caller. So that we can do error logging from the caller instead of from
_commit.
Longer term plan is to first convert all callers to use onerr==QUIET_ON_ERR
and craft any log messages from the callers themselves and finally remove the
onerr argument completely.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
builtin/update-ref.c | 2 +-
refs.c | 6 +-
refs.h | 5 -
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 405267f..aaa06aa 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -367,7 +367,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
if (end_null)
line_termination = '\0';
update_refs_stdin();
- ret = ref_transaction_commit(transaction, msg,
+ ret = ref_transaction_commit(transaction, msg, NULL,
UPDATE_REFS_DIE_ON_ERR);
return ret;
}
diff --git a/refs.c b/refs.c
index 88d73c8..1a7f9d9 100644
--- a/refs.c
+++ b/refs.c
@@ -3423,7 +3423,8 @@ static int ref_update_reject_duplicates(struct ref_update
**updates, int n,
}
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, enum action_on_err onerr)
+ const char *msg, struct strbuf *err,
+ enum action_on_err onerr)
{
int ret = 0, delnum = 0, i;
const char **delnames;
@@ -3452,6 +3453,9 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
update->flags,
&update->type, onerr);
if (!update->lock) {
+ if (err)
+ strbuf_addf(err, "Cannot lock the ref '%s'.",
+ update->refname);
ret = 1;
goto cleanup;
}
diff --git a/refs.h b/refs.h
index 6d97700..25ae110 100644
--- a/refs.h
+++ b/refs.h
@@ -274,9 +274,12 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
* Commit all of the changes that have been queued in transaction, as
* atomically as possible. Return a nonzero value if there is a
* problem. The ref_transaction is freed by this function.
+ * If err is non-NULL we will add an error string to it to explain why
+ * the transaction failed. The string does not end in newline.
*/
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, enum action_on_err onerr);
+ const char *msg, struct strbuf *err,
+ enum action_on_err onerr);
/** Lock a ref and then write its file */
int update_ref(const char *action, const char *refname,
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 14/44] replace.c: use the ref transaction functions for updates
Update replace.c to use ref transactions for updates.
Signed-off-by: Ronnie Sahlberg
---
builtin/replace.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/builtin/replace.c b/builtin/replace.c
index 3da1bae..e8932cd 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -133,7 +133,8 @@ static int replace_object_sha1(const char *object_ref,
unsigned char prev[20];
enum object_type obj_type, repl_type;
char ref[PATH_MAX];
- struct ref_lock *lock;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
if (snprintf(ref, sizeof(ref),
"refs/replace/%s",
@@ -156,11 +157,12 @@ static int replace_object_sha1(const char *object_ref,
else if (!force)
die("replace ref '%s' already exists", 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)
- die("%s: cannot update the ref", ref);
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_update(transaction, ref, repl, prev,
+ 0, !is_null_sha1(prev), &err) ||
+ ref_transaction_commit(transaction, NULL, &err))
+ die("%s", err.buf);
return 0;
}
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 26/44] fast-import.c: use a ref transaction when dumping tags
Signed-off-by: Ronnie Sahlberg
---
fast-import.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 6b186ce..6ef5cec 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1735,15 +1735,22 @@ static void dump_tags(void)
{
static const char *msg = "fast-import";
struct tag *t;
- struct ref_lock *lock;
char ref_name[PATH_MAX];
+ struct strbuf err = STRBUF_INIT;
+ struct ref_transaction *transaction;
+ transaction = ref_transaction_begin();
for (t = first_tag; t; t = t->next_tag) {
- sprintf(ref_name, "tags/%s", t->name);
- lock = lock_ref_sha1(ref_name, NULL);
- if (!lock || write_ref_sha1(lock, t->sha1, msg) < 0)
- failure |= error("Unable to update %s", ref_name);
+ sprintf(ref_name, "refs/tags/%s", t->name);
+
+ if (ref_transaction_update(transaction, ref_name, t->sha1,
+ NULL, 0, 0, &err))
+ failure |= error("%s", err.buf);
}
+ if (failure || ref_transaction_commit(transaction, msg, &err))
+ failure |= error("%s", err.buf);
+ ref_transaction_free(transaction);
+ strbuf_release(&err);
}
static void dump_marks_helper(FILE *f,
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 28/44] refs.c: make write_ref_sha1 static
No external users call write_ref_sha1 any more so lets declare it static.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 4 +++-
refs.h | 3 ---
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index 016206b..6151c3c 100644
--- a/refs.c
+++ b/refs.c
@@ -251,6 +251,8 @@ struct ref_entry {
};
static void read_loose_refs(const char *dirname, struct ref_dir *dir);
+static int write_ref_sha1(struct ref_lock *lock,
+ const unsigned char *sha1, const char *logmsg);
static struct ref_dir *get_ref_dir(struct ref_entry *entry)
{
@@ -2833,7 +2835,7 @@ static int is_branch(const char *refname)
return !strcmp(refname, "HEAD") || starts_with(refname, "refs/heads/");
}
-int write_ref_sha1(struct ref_lock *lock,
+static int write_ref_sha1(struct ref_lock *lock,
const unsigned char *sha1, const char *logmsg)
{
static char term = '\n';
diff --git a/refs.h b/refs.h
index 608ca10..a45712c 100644
--- a/refs.h
+++ b/refs.h
@@ -150,9 +150,6 @@ extern int commit_ref(struct ref_lock *lock);
/** Release any lock taken but not written. **/
extern void unlock_ref(struct ref_lock *lock);
-/** Writes sha1 into the ref specified by the lock. **/
-extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
const char *msg);
-
/** Setup reflog before using. **/
int log_ref_setup(const char *refname, char *logfile, int bufsize);
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 00/44] Use ref transactions for all ref updates
This patch series can also be found at https://github.com/rsahlberg/git/tree/ref-transactions This patch series is based on next and expands on the transaction API. It converts all ref updates, inside refs.c as well as external, to use the transaction API for updates. This makes most of the ref updates to become atomic when there are failures locking or writing to a ref. This version completes the work to convert all ref updates to use transactions. Now that all updates are through transactions I will start working on cleaning up the reading of refs and to create an api for managing reflogs but all that will go in a different patch series. Version 9: - Lots of updates to error messages based on JN's review. Version 8: - Updates after review by JN - Improve commit messages - Add a patch that adds an err argument to repack_without_refs - Add a patch that adds an err argument to delete_loose_ref - Better document that a _update/_delete/_create failure means the whole transaction has failed and needs rollback. Version 7: - Updated commit messages per JNs review comments. - Changed REF_ISPRUNING and REF_ISPACKONLY to be private flags and not exposed through refs.h Version 6: - Convert all updates in refs.c to use transactions too. Version 5: - Reword commit messages for having _create/_delete/_update returning success/failure. There are no conditions yet that return an error from these failures but there will be in the future. So we still check the return from these functions in the callers in preparation for this. - Don't leak memory by just passing a strbuf_detach() pointer to functions. Use .buf and explicitely strbuf_release the data afterwards. - Remove the function update_ref_lock. - Remove the function update_ref_write. - Track transaction status and die(BUG:) if we call _create/_delete/_update/ _commit for a transaction that is not OPEN. Version 4: - Rename patch series from "Use ref transactions from most callers" to "Use ref transactions for all ref updates". - Convert all external ref writes to use transactions and make write_ref_sha1 and lock_ref_sha1 static functions. - Change the ref commit and free handling so we no longer pass pointer to pointer to _commit. _commit no longer frees the transaction. The caller MUST call _free itself. - Change _commit to take a strbuf pointer instead of a char* for error reporting back to the caller. - Re-add the walker patch after fixing it. Version 3: - Remove the walker patch for now. Walker needs more complex solution so defer it until the basics are done. - Remove the onerr argument to ref_transaction_commit(). All callers that need to die() on error now have to do this explicitely. - Pass an error string from ref_transaction_commit() back to the callers so that they can craft a nice error message upon failures. - Make ref_transaction_rollback() accept NULL as argument. - Change ref_transaction_commit() to take a pointer to pointer argument for the transaction and have it clear the callers pointer to NULL when invoked. This allows for much nicer handling of transaction rollback on failure. Version 2: - Add a patch to ref_transaction_commit to make it honor onerr even if the error triggered in ref_Transaction_commit itself rather than in a call to other functions (that already honor onerr). - Add a patch to make the update_ref() helper function use transactions internally. - Change ref_transaction_update to die() instead of error() if we pass if a NULL old_sha1 but have have_old == true. - Change ref_transaction_create to die() instead of error() if new_sha1 is false but we pass it a null_sha1. - Change ref_transaction_delete die() instead of error() if we pass if a NULL old_sha1 but have have_old == true. - Change several places to do if(!transaction || ref_transaction_update() || ref_Transaction_commit()) die(generic-message) instead of checking each step separately and having a different message for each failure. Most users are likely not interested in what step of the transaction failed and only whether it failed or not. - Change commit.c to only pass a pointer to ref_transaction_update iff current_head is non-NULL. The previous patch used to compute a garbage pointer for current_head->object.sha1 and relied on the fact that ref_transaction_update would not try to dereference this pointer if !!current_head was 0. - Updated commit message for the walker_fetch change to try to justify why the change in locking semantics should not be harmful. Ronnie Sahlberg (44): refs.c: constify the sha arguments for ref_transaction_create|delete|update refs.c: allow passing NULL to ref_transaction_free refs.c: add a strbuf argument to ref_transaction_commit for error logging refs.c: add an err argument to repack_without_refs refs.c: make ref_update_reject_duplicates take a strbuf argument for errors refs.c:
[PATCH v9 43/44] refs.c: make rename_ref use a transaction
Change rename_ref to use a single transaction to perform the ref rename.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 73 ++
1 file changed, 20 insertions(+), 53 deletions(-)
diff --git a/refs.c b/refs.c
index c471811..7ea3f42 100644
--- a/refs.c
+++ b/refs.c
@@ -2630,9 +2630,10 @@ static int rename_tmp_log(const char *newrefname)
int rename_ref(const char *oldrefname, const char *newrefname, const char
*logmsg)
{
- unsigned char sha1[20], orig_sha1[20];
- int flag = 0, logmoved = 0;
- struct ref_lock *lock;
+ unsigned char sha1[20];
+ int flag = 0;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
struct stat loginfo;
int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
const char *symref = NULL;
@@ -2643,7 +2644,7 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
if (log && S_ISLNK(loginfo.st_mode))
return error("reflog for %s is a symlink", oldrefname);
- symref = resolve_ref_unsafe(oldrefname, orig_sha1, 1, &flag);
+ symref = resolve_ref_unsafe(oldrefname, sha1, 1, &flag);
if (flag & REF_ISSYMREF)
return error("refname %s is a symbolic ref, renaming it is not
supported",
oldrefname);
@@ -2665,62 +2666,28 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
if (pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE))
return error("unable to pack refs");
- if (delete_ref(oldrefname, orig_sha1, REF_NODEREF)) {
- error("unable to delete old %s", oldrefname);
- goto rollback;
- }
-
- if (!read_ref_full(newrefname, sha1, 1, NULL) &&
- delete_ref(newrefname, sha1, REF_NODEREF)) {
- if (errno==EISDIR) {
- if (remove_empty_directories(git_path("%s",
newrefname))) {
- error("Directory not empty: %s", newrefname);
- goto rollback;
- }
- } else {
- error("unable to delete existing %s", newrefname);
- goto rollback;
- }
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_delete(transaction, oldrefname, sha1,
+ REF_NODEREF | REF_ISPACKONLY,
+ 1, NULL, &err) ||
+ ref_transaction_update(transaction, newrefname, sha1,
+ NULL, 0, 0, logmsg, &err) ||
+ ref_transaction_commit(transaction, &err)) {
+ ref_transaction_rollback(transaction);
+ error("rename_ref failed: %s", err.buf);
+ strbuf_release(&err);
+ goto rollbacklog;
}
+ ref_transaction_free(transaction);
if (log && rename_tmp_log(newrefname))
- goto rollback;
-
- logmoved = log;
-
- lock = lock_ref_sha1_basic(newrefname, NULL, 0, NULL, NULL, 0);
- if (!lock) {
- error("unable to lock %s for update", newrefname);
- goto rollback;
- }
- lock->force_write = 1;
- hashcpy(lock->old_sha1, orig_sha1);
- if (write_ref_sha1(lock, orig_sha1, logmsg)) {
- error("unable to write current sha1 into %s", newrefname);
- goto rollback;
- }
-
- return 0;
-
- rollback:
- lock = lock_ref_sha1_basic(oldrefname, NULL, 0, NULL, NULL, 0);
- if (!lock) {
- error("unable to lock %s for rollback", oldrefname);
goto rollbacklog;
- }
- lock->force_write = 1;
- flag = log_all_ref_updates;
- log_all_ref_updates = 0;
- if (write_ref_sha1(lock, orig_sha1, NULL))
- error("unable to write current sha1 into %s", oldrefname);
- log_all_ref_updates = flag;
+ return 0;
rollbacklog:
- if (logmoved && rename(git_path("logs/%s", newrefname),
git_path("logs/%s", oldrefname)))
- error("unable to restore logfile %s from %s: %s",
- oldrefname, newrefname, strerror(errno));
- if (!logmoved && log &&
+ if (log &&
rename(git_path(TMP_RENAMED_LOG), git_path("logs/%s", oldrefname)))
error("unable to restore logfile %s from "TMP_RENAMED_LOG": %s",
oldrefname, strerror(errno));
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 24/44] fetch.c: use a single ref transaction for all ref updates
Change store_updated_refs to use a single ref transaction for all refs that
are updated during the fetch. This makes the fetch more atomic when update
failures occur.
Since ref update failures will now no longer occur in the code path for
updating a single ref in s_update_ref, we no longer have as detailed error
message logging the exact reference and the ref log action as in the old cod
Instead since we fail the entire transaction we log a much more generic
message. But since we commit the transaction using MSG_ON_ERR we will log
an error containing the ref name if either locking of writing the ref would
so the regression in the log message is minor.
This will also change the order in which errors are checked for and logged
which may alter which error will be logged if there are multiple errors
occuring during a fetch.
For example, assume we have a fetch for two refs that both would fail.
Where the first ref would fail with ENOTDIR due to a directory in the ref
path not existing, and the second ref in the fetch would fail due to
the check in update_logical_ref():
if (current_branch &&
!strcmp(ref->name, current_branch->name) &&
!(update_head_ok || is_bare_repository()) &&
!is_null_sha1(ref->old_sha1)) {
/*
* If this is the head, and it's not okay to update
* the head, and the old value of the head isn't empty...
*/
In the old code since we would update the refs one ref at a time we would
first fail the ENOTDIR and then fail the second update of HEAD as well.
But since the first ref failed with ENOTDIR we would eventually fail the who
fetch with STORE_REF_ERROR_DF_CONFLICT
In the new code, since we defer committing the transaction until all refs
have been processed, we would now detect that the second ref was bad and
rollback the transaction before we would even try start writing the update t
disk and thus we would not return STORE_REF_ERROR_DF_CONFLICT for this case.
I think this new behaviour is more correct, since if there was a problem
we would not even try to commit the transaction but need to highlight this
change in how/what errors are reported.
This change in what error is returned only occurs if there are multiple
refs that fail to update and only some, but not all, of them fail due to
ENOTDIR.
Signed-off-by: Ronnie Sahlberg
---
builtin/fetch.c | 34 --
1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index c9b5954..5b0cc31 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -45,6 +45,7 @@ static struct transport *gsecondary;
static const char *submodule_prefix = "";
static const char *recurse_submodules_default;
static int shown_url;
+static struct ref_transaction *transaction;
static int option_parse_recurse_submodules(const struct option *opt,
const char *arg, int unset)
@@ -373,27 +374,13 @@ static int s_update_ref(const char *action,
struct ref *ref,
int check_old)
{
- char msg[1024];
- char *rla = getenv("GIT_REFLOG_ACTION");
- struct ref_transaction *transaction;
-
if (dry_run)
return 0;
- if (!rla)
- rla = default_rla.buf;
- snprintf(msg, sizeof(msg), "%s: %s", rla, action);
- errno = 0;
- transaction = ref_transaction_begin();
- if (!transaction ||
- ref_transaction_update(transaction, ref->name, ref->new_sha1,
- ref->old_sha1, 0, check_old, NULL) ||
- ref_transaction_commit(transaction, msg, NULL)) {
- ref_transaction_rollback(transaction);
- return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
- STORE_REF_ERROR_OTHER;
- }
- ref_transaction_free(transaction);
+ if (ref_transaction_update(transaction, ref->name, ref->new_sha1,
+ ref->old_sha1, 0, check_old, NULL))
+ return STORE_REF_ERROR_OTHER;
+
return 0;
}
@@ -565,6 +552,13 @@ static int store_updated_refs(const char *raw_url, const
char *remote_name,
goto abort;
}
+ errno = 0;
+ transaction = ref_transaction_begin();
+ if (!transaction) {
+ rc = error(_("cannot start ref transaction\n"));
+ goto abort;
+ }
+
/*
* We do a pass for each fetch_head_status type in their enum order, so
* merged entries are written before not-for-merge. That lets readers
@@ -676,6 +670,10 @@ static int store_updated_refs(const char *raw_url, const
char *remote_name,
}
}
}
+ if (ref_transaction_commit(transaction, "fetch_ref transaction", NULL))
+ rc |= errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
+
[PATCH v9 27/44] walker.c: use ref transaction for ref updates
Switch to using ref transactions in walker_fetch(). As part of the refactoring
to use ref transactions we also fix a potential memory leak where in the
original code if write_ref_sha1() would fail we would end up returning from
the function without free()ing the msg string.
This changes the locking slightly for walker_fetch. Previously the code would
lock all refs before writing them but now we do not lock the refs until the
commit stage. There is thus a very short window where changes could be done
locally during the fetch which would be overwritten when the fetch completes
and commits its transaction. But this window should be reasonably short.
Even if this race does trigger, since both the old code and the new code
just overwrites the refs to the new values without checking or comparing
them with the previous value, this is not too dissimilar to a similar scenario
where you first do a ref change locally and then later do a fetch that
overwrites the local change. With this in mind I do not see the change in
locking semantics to be critical.
Note that this function is only called when fetching from a remote HTTP
repository onto the local (most of the time single-user) repository which
likely means that the type of collissions that the previous locking would
protect against and cause the fetch to fail for to be even more rare.
Signed-off-by: Ronnie Sahlberg
---
walker.c | 51 ++-
1 file changed, 26 insertions(+), 25 deletions(-)
diff --git a/walker.c b/walker.c
index 1dd86b8..cbe5686 100644
--- a/walker.c
+++ b/walker.c
@@ -251,24 +251,18 @@ void walker_targets_free(int targets, char **target,
const char **write_ref)
int walker_fetch(struct walker *walker, int targets, char **target,
const char **write_ref, const char *write_ref_log_details)
{
- struct ref_lock **lock = xcalloc(targets, sizeof(struct ref_lock *));
+ char ref_name[PATH_MAX];
+ struct strbuf err = STRBUF_INIT;
+ struct ref_transaction *transaction;
unsigned char *sha1 = xmalloc(targets * 20);
- char *msg;
- int ret;
+ char *msg = NULL;
int i;
save_commit_buffer = 0;
- for (i = 0; i < targets; i++) {
- if (!write_ref || !write_ref[i])
- continue;
-
- lock[i] = lock_ref_sha1(write_ref[i], NULL);
- if (!lock[i]) {
- error("Can't lock ref %s", write_ref[i]);
- goto unlock_and_fail;
- }
- }
+ transaction = ref_transaction_begin();
+ if (!transaction)
+ return -1;
if (!walker->get_recover)
for_each_ref(mark_complete, NULL);
@@ -276,14 +270,14 @@ int walker_fetch(struct walker *walker, int targets, char
**target,
for (i = 0; i < targets; i++) {
if (interpret_target(walker, target[i], &sha1[20 * i])) {
error("Could not interpret response from server '%s' as
something to pull", target[i]);
- goto unlock_and_fail;
+ goto rollback_and_fail;
}
if (process(walker, lookup_unknown_object(&sha1[20 * i])))
- goto unlock_and_fail;
+ goto rollback_and_fail;
}
if (loop(walker))
- goto unlock_and_fail;
+ goto rollback_and_fail;
if (write_ref_log_details) {
msg = xmalloc(strlen(write_ref_log_details) + 12);
@@ -294,19 +288,26 @@ int walker_fetch(struct walker *walker, int targets, char
**target,
for (i = 0; i < targets; i++) {
if (!write_ref || !write_ref[i])
continue;
- ret = write_ref_sha1(lock[i], &sha1[20 * i], msg ? msg : "fetch
(unknown)");
- lock[i] = NULL;
- if (ret)
- goto unlock_and_fail;
+ sprintf(ref_name, "refs/%s", write_ref[i]);
+ if (ref_transaction_update(transaction, ref_name,
+ &sha1[20 * i], NULL,
+ 0, 0, &err))
+ goto rollback_and_fail;
+ }
+
+ if (ref_transaction_commit(transaction, msg ? msg : "fetch (unknown)",
+ &err)) {
+ error("%s", err.buf);
+ goto rollback_and_fail;
}
- free(msg);
+ free(msg);
return 0;
-unlock_and_fail:
- for (i = 0; i < targets; i++)
- if (lock[i])
- unlock_ref(lock[i]);
+rollback_and_fail:
+ free(msg);
+ strbuf_release(&err);
+ ref_transaction_free(transaction);
return -1;
}
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info
[PATCH v9 16/44] sequencer.c: use ref transactions for all ref updates
Change to use ref transactions for all updates to refs.
Signed-off-by: Ronnie Sahlberg
---
sequencer.c | 24
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 0a80c58..e6b3bbc 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -272,23 +272,31 @@ static int error_dirty_index(struct replay_opts *opts)
static int fast_forward_to(const unsigned char *to, const unsigned char *from,
int unborn, struct replay_opts *opts)
{
- struct ref_lock *ref_lock;
+ struct ref_transaction *transaction;
struct strbuf sb = STRBUF_INIT;
- int ret;
+ struct strbuf err = STRBUF_INIT;
read_cache();
if (checkout_fast_forward(from, to, 1))
exit(1); /* the callee should have complained already */
- ref_lock = lock_any_ref_for_update("HEAD", unborn ? null_sha1 : from,
- 0, NULL);
- if (!ref_lock)
- return error(_("Failed to lock HEAD during fast_forward_to"));
strbuf_addf(&sb, "%s: fast-forward", action_name(opts));
- ret = write_ref_sha1(ref_lock, to, sb.buf);
+
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_update(transaction, "HEAD", to, from,
+ 0, !unborn, &err) ||
+ (ref_transaction_commit(transaction, sb.buf, &err) &&
+!(transaction = NULL))) {
+ ref_transaction_rollback(transaction);
+ error("%s", err.buf);
+ strbuf_release(&sb);
+ strbuf_release(&err);
+ return -1;
+ }
strbuf_release(&sb);
- return ret;
+ return 0;
}
static int do_recursive_merge(struct commit *base, struct commit *next,
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 18/44] branch.c: use ref transaction for all ref updates
Change create_branch to use a ref transaction when creating the new branch.
ref_transaction_create will check that the ref does not already exist and fail
otherwise meaning that we no longer need to keep a lock on the ref during the
setup_tracking. This simplifies the code since we can now do the transaction
in one single step.
If the forcing flag is false then use ref_transaction_create since this will
fail if the ref already exist. Otherwise use ref_transaction_update.
This also fixes a race condition in the old code where two concurrent
create_branch could race since the lock_any_ref_for_update/write_ref_sha1
did not protect against the ref already existing. I.e. one thread could end up
overwriting a branch even if the forcing flag is false.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
branch.c | 29 +++--
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/branch.c b/branch.c
index 660097b..8bf7588 100644
--- a/branch.c
+++ b/branch.c
@@ -226,7 +226,6 @@ void create_branch(const char *head,
int force, int reflog, int clobber_head,
int quiet, enum branch_track track)
{
- struct ref_lock *lock = NULL;
struct commit *commit;
unsigned char sha1[20];
char *real_ref, msg[PATH_MAX + 20];
@@ -285,15 +284,6 @@ void create_branch(const char *head,
die(_("Not a valid branch point: '%s'."), start_name);
hashcpy(sha1, commit->object.sha1);
- if (!dont_change_ref) {
- lock = lock_any_ref_for_update(ref.buf, NULL, 0, NULL);
- if (!lock)
- die_errno(_("Failed to lock ref for update"));
- }
-
- if (reflog)
- log_all_ref_updates = 1;
-
if (forcing)
snprintf(msg, sizeof msg, "branch: Reset to %s",
start_name);
@@ -301,13 +291,24 @@ void create_branch(const char *head,
snprintf(msg, sizeof msg, "branch: Created from %s",
start_name);
+ if (reflog)
+ log_all_ref_updates = 1;
+
+ if (!dont_change_ref) {
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
+
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_update(transaction, ref.buf, sha1,
+ null_sha1, 0, !forcing, &err) ||
+ ref_transaction_commit(transaction, msg, &err))
+ die("%s", err.buf);
+ }
+
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)
- die_errno(_("Failed to write ref"));
-
strbuf_release(&ref);
free(real_ref);
}
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 17/44] fast-import.c: change update_branch to use ref transactions
Change update_branch() to use ref transactions for updates.
Signed-off-by: Ronnie Sahlberg
---
fast-import.c | 25 +++--
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 6707a66..bac799c 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1679,39 +1679,44 @@ found_entry:
static int update_branch(struct branch *b)
{
static const char *msg = "fast-import";
- struct ref_lock *lock;
+ struct ref_transaction *transaction;
unsigned char old_sha1[20];
+ struct strbuf err = STRBUF_INIT;
if (read_ref(b->name, old_sha1))
hashclr(old_sha1);
+
if (is_null_sha1(b->sha1)) {
if (b->delete)
delete_ref(b->name, old_sha1, 0);
return 0;
}
- lock = lock_any_ref_for_update(b->name, old_sha1, 0, NULL);
- if (!lock)
- return error("Unable to lock %s", b->name);
if (!force_update && !is_null_sha1(old_sha1)) {
struct commit *old_cmit, *new_cmit;
old_cmit = lookup_commit_reference_gently(old_sha1, 0);
new_cmit = lookup_commit_reference_gently(b->sha1, 0);
- if (!old_cmit || !new_cmit) {
- unlock_ref(lock);
+ if (!old_cmit || !new_cmit)
return error("Branch %s is missing commits.", b->name);
- }
if (!in_merge_bases(old_cmit, new_cmit)) {
- unlock_ref(lock);
warning("Not updating %s"
" (new tip %s does not contain %s)",
b->name, sha1_to_hex(b->sha1),
sha1_to_hex(old_sha1));
return -1;
}
}
- if (write_ref_sha1(lock, b->sha1, msg) < 0)
- return error("Unable to update %s", b->name);
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_update(transaction, b->name, b->sha1, old_sha1,
+ 0, 1, &err) ||
+ (ref_transaction_commit(transaction, msg, &err) &&
+!(transaction = NULL))) {
+ ref_transaction_rollback(transaction);
+ error("%s", err.buf);
+ strbuf_release(&err);
+ return -1;
+ }
return 0;
}
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 34/44] refs.c: make prune_ref use a transaction to delete the ref
Change prune_ref to delete the ref using a ref transaction. To do this we also
need to add a new flag REF_ISPRUNING that will tell the transaction that we
do not want to delete this ref from the packed refs. This flag is private to
refs.c and not exposed to external callers.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 27 ---
1 file changed, 20 insertions(+), 7 deletions(-)
diff --git a/refs.c b/refs.c
index 99b110f..f88263e 100644
--- a/refs.c
+++ b/refs.c
@@ -29,6 +29,11 @@ static inline int bad_ref_char(int ch)
return 0;
}
+/** Used as a flag to ref_transaction_delete when a loose ref is beeing
+ * pruned.
+ */
+#define REF_ISPRUNING 0x0100
+
/*
* Try to read one refname component from the front of refname. Return
* the length of the component found, or -1 if the component is not
@@ -2330,17 +2335,24 @@ static void try_remove_empty_parents(char *name)
/* make sure nobody touched the ref, and unlink */
static void prune_ref(struct ref_to_prune *r)
{
- struct ref_lock *lock;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
if (check_refname_format(r->name + 5, 0))
return;
- lock = lock_ref_sha1_basic(r->name, r->sha1, 0, NULL);
- if (lock) {
- unlink_or_warn(git_path("%s", r->name));
- unlock_ref(lock);
- try_remove_empty_parents(r->name);
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_delete(transaction, r->name, r->sha1,
+ REF_ISPRUNING, 1, &err) ||
+ ref_transaction_commit(transaction, NULL, &err)) {
+ ref_transaction_rollback(transaction);
+ warning("prune_ref: %s", err.buf);
+ strbuf_release(&err);
+ return;
}
+ ref_transaction_free(transaction);
+ try_remove_empty_parents(r->name);
}
static void prune_refs(struct ref_to_prune *r)
@@ -3543,9 +3555,10 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (update->lock) {
- delnames[delnum++] = update->lock->ref_name;
ret |= delete_ref_loose(update->lock, update->type,
err);
+ if (!(update->flags & REF_ISPRUNING))
+ delnames[delnum++] = update->lock->ref_name;
}
}
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 22/44] fetch.c: clear errno before calling functions that might set it
In s_update_ref there are two calls that when they fail we return an error based on the errno value. In particular we want to return a specific error if ENOTDIR happened. Both these functions do have failure modes where they may return an error without updating errno, in which case a previous and unrelated ENOTDIR may cause us to return the wrong error. Clear errno before calling any functions if we check errno afterwards. Also skip initializing a static variable to 0. Statics live in .bss and are all automatically initialized to 0. Signed-off-by: Ronnie Sahlberg --- builtin/fetch.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 55f457c..a93c893 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -44,7 +44,7 @@ static struct transport *gtransport; static struct transport *gsecondary; static const char *submodule_prefix = ""; static const char *recurse_submodules_default; -static int shown_url = 0; +static int shown_url; static int option_parse_recurse_submodules(const struct option *opt, const char *arg, int unset) @@ -382,6 +382,8 @@ static int s_update_ref(const char *action, if (!rla) rla = default_rla.buf; snprintf(msg, sizeof(msg), "%s: %s", rla, action); + + errno = 0; lock = lock_any_ref_for_update(ref->name, check_old ? ref->old_sha1 : NULL, 0, NULL); -- 2.0.0.rc3.477.gffe78a2 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 37/44] refs.c: pass NULL as *flags to read_ref_full
We call read_ref_full with a pointer to flags from rename_ref but since
we never actually use the returned flags we can just pass NULL here instead.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 5369a39..e25345c 100644
--- a/refs.c
+++ b/refs.c
@@ -2640,7 +2640,7 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
goto rollback;
}
- if (!read_ref_full(newrefname, sha1, 1, &flag) &&
+ if (!read_ref_full(newrefname, sha1, 1, NULL) &&
delete_ref(newrefname, sha1, REF_NODEREF)) {
if (errno==EISDIR) {
if (remove_empty_directories(git_path("%s",
newrefname))) {
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 19/44] refs.c: change update_ref to use a transaction
Ronnie Sahlberg wrote:
> Change the update_ref helper function to use a ref transaction internally.
>
> Signed-off-by: Ronnie Sahlberg
> ---
> refs.c | 25 +
> 1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 6e5e940..0476892 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3416,11 +3416,28 @@ int update_ref(const char *action, const char
> *refname,
> const unsigned char *sha1, const unsigned char *oldval,
> int flags, enum action_on_err onerr)
> {
> - struct ref_lock *lock;
> - lock = update_ref_lock(refname, oldval, flags, NULL, onerr);
> - if (!lock)
> + struct ref_transaction *t;
> + struct strbuf err = STRBUF_INIT;
> +
> + t = ref_transaction_begin();
> + if ((!t ||
> + ref_transaction_update(t, refname, sha1, oldval, flags,
> +!!oldval)) ||
> + (ref_transaction_commit(t, action, &err) && !(t = NULL))) {
> + const char *str = "update_ref failed for ref '%s': %s";
> +
> + ref_transaction_rollback(t);
> + switch (onerr) {
> + case UPDATE_REFS_MSG_ON_ERR:
> + error(str, refname, err.buf); break;
> + case UPDATE_REFS_DIE_ON_ERR:
> + die(str, refname, err.buf); break;
> + case UPDATE_REFS_QUIET_ON_ERR: break;
> + }
The error string already contains the refname, so it would make sense to
do
case UPDATE_REFS_MSG_ON_ERR:
error("%s", err.buf); break;
etc here.
It occurs to me now that if ref_transaction_begin fails, presumably
the error string would not contain the refname. I want to say that
the refname wouldn't help much in diagnosing or recovering from the
problem in that case so it would still be okay to just do error("%s",
err.buf). What do you think?
Thanks,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 06/44] refs.c: add an err argument to delete_ref_loose
Add an err argument to delete_loose_ref so that we can pass a descriptive
error string back to the caller. Pass the err argument from transaction
commit to this function so that transaction users will have a nice error
string if the transaction failed due to delete_loose_ref.
Add a new function unlink_or_err that we can call from delete_ref_loose. This
function is similar to unlink_or_warn except that we can pass it an err
argument. If err is non-NULL the function will populate err instead of
printing a warning().
Signed-off-by: Ronnie Sahlberg
---
refs.c | 39 +--
1 file changed, 33 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index 4ebf130..e4d35c9 100644
--- a/refs.c
+++ b/refs.c
@@ -2491,17 +2491,43 @@ static int repack_without_ref(const char *refname)
return repack_without_refs(&refname, 1, NULL);
}
-static int delete_ref_loose(struct ref_lock *lock, int flag)
+static int add_err_if_unremovable(const char *op, const char *file,
+ struct strbuf *e, int rc)
+{
+ int err = errno;
+ if (rc < 0 && err != ENOENT) {
+ strbuf_addf(e, "unable to %s %s: %s",
+ op, file, strerror(errno));
+ errno = err;
+ }
+ return rc;
+}
+
+static int unlink_or_err(const char *file, struct strbuf *err)
+{
+ if (err)
+ return add_err_if_unremovable("unlink", file, err,
+ unlink(file));
+ else
+ return unlink_or_warn(file);
+}
+
+static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf
*err)
{
if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
/* loose */
- int err, i = strlen(lock->lk->filename) - 5; /* .lock */
+ int res, i = strlen(lock->lk->filename) - 5; /* .lock */
lock->lk->filename[i] = 0;
- err = unlink_or_warn(lock->lk->filename);
+ res = unlink_or_err(lock->lk->filename, err);
lock->lk->filename[i] = '.';
- if (err && errno != ENOENT)
+ if (res && errno != ENOENT) {
+ if (err)
+ strbuf_addf(err,
+ "failed to delete loose ref '%s'",
+ lock->lk->filename);
return 1;
+ }
}
return 0;
}
@@ -2514,7 +2540,7 @@ int delete_ref(const char *refname, const unsigned char
*sha1, int delopt)
lock = lock_ref_sha1_basic(refname, sha1, delopt, &flag);
if (!lock)
return 1;
- ret |= delete_ref_loose(lock, flag);
+ ret |= delete_ref_loose(lock, flag, NULL);
/* removing the loose one could have resurrected an earlier
* packed one. Also, if it was not loose we need to repack
@@ -3499,7 +3525,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
if (update->lock) {
delnames[delnum++] = update->lock->ref_name;
- ret |= delete_ref_loose(update->lock, update->type);
+ ret |= delete_ref_loose(update->lock, update->type,
+ err);
}
}
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 39/44] refs.c: move the check for valid refname to lock_ref_sha1_basic
Move the check for check_refname_format from lock_any_ref_for_update
to lock_ref_sha1_basic. At some later stage we will get rid of
lock_any_ref_for_update completely.
This leaves lock_any_ref_for_updates as a no-op wrapper which could be removed.
But this wrapper is also called from an external caller and we will soon
make changes to the signature to lock_ref_sha1_basic that we do not want to
expose to that caller.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index fb36cba..baa620f 100644
--- a/refs.c
+++ b/refs.c
@@ -2044,6 +2044,9 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
int missing = 0;
int attempts_remaining = 3;
+ if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
+ return NULL;
+
lock = xcalloc(1, sizeof(struct ref_lock));
lock->lock_fd = -1;
@@ -2135,8 +2138,6 @@ struct ref_lock *lock_any_ref_for_update(const char
*refname,
const unsigned char *old_sha1,
int flags, int *type_p)
{
- if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
- return NULL;
return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
}
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 21/44] refs.c: ref_transaction_commit should not free the transaction
Change ref_transaction_commit so that it does not free the transaction.
Instead require that a caller will end a transaction by either calling
ref_transaction_rollback or ref_transaction_free.
By having the transaction object remaining valid after _commit returns allows
us to write much nicer code and still be able to call ref_transaction_rollback
safely. Instead of this horribleness
t = ref_transaction_begin();
if ((!t ||
ref_transaction_update(t, refname, sha1, oldval, flags,
!!oldval)) ||
(ref_transaction_commit(t, action, &err) && !(t = NULL))) {
ref_transaction_rollback(t);
we can now just do the much nicer
t = ref_transaction_begin();
if (!t ||
ref_transaction_update(t, refname, sha1, oldval, flags,
!!oldval) ||
ref_transaction_commit(&t, action, &err)) {
ref_transaction_rollback(t);
... die/return ...
ref_transaction_free(transaction);
Signed-off-by: Ronnie Sahlberg
---
branch.c | 1 +
builtin/commit.c | 1 +
builtin/replace.c| 1 +
builtin/tag.c| 1 +
builtin/update-ref.c | 1 +
fast-import.c| 4 ++--
refs.c | 14 ++
refs.h | 14 +-
sequencer.c | 4 ++--
9 files changed, 24 insertions(+), 17 deletions(-)
diff --git a/branch.c b/branch.c
index 8bf7588..f78a28b 100644
--- a/branch.c
+++ b/branch.c
@@ -304,6 +304,7 @@ void create_branch(const char *head,
null_sha1, 0, !forcing, &err) ||
ref_transaction_commit(transaction, msg, &err))
die("%s", err.buf);
+ ref_transaction_free(transaction);
}
if (real_ref && track)
diff --git a/builtin/commit.c b/builtin/commit.c
index c429216..6b888f2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1726,6 +1726,7 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
rollback_index_files();
die("%s", err.buf);
}
+ ref_transaction_free(transaction);
unlink(git_path("CHERRY_PICK_HEAD"));
unlink(git_path("REVERT_HEAD"));
diff --git a/builtin/replace.c b/builtin/replace.c
index e8932cd..af7f72d 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -164,6 +164,7 @@ static int replace_object_sha1(const char *object_ref,
ref_transaction_commit(transaction, NULL, &err))
die("%s", err.buf);
+ ref_transaction_free(transaction);
return 0;
}
diff --git a/builtin/tag.c b/builtin/tag.c
index b05f9a5..30b471c 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -708,6 +708,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
0, !is_null_sha1(prev), &err) ||
ref_transaction_commit(transaction, NULL, &err))
die("%s", err.buf);
+ ref_transaction_free(transaction);
if (force && !is_null_sha1(prev) && hashcmp(prev, object))
printf(_("Updated tag '%s' (was %s)\n"), tag,
find_unique_abbrev(prev, DEFAULT_ABBREV));
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index cdb71a8..4c2145e 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -373,6 +373,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
update_refs_stdin();
if (ref_transaction_commit(transaction, msg, &err))
die("%s", err.buf);
+ ref_transaction_free(transaction);
return 0;
}
diff --git a/fast-import.c b/fast-import.c
index bac799c..6b186ce 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1710,13 +1710,13 @@ static int update_branch(struct branch *b)
if (!transaction ||
ref_transaction_update(transaction, b->name, b->sha1, old_sha1,
0, 1, &err) ||
- (ref_transaction_commit(transaction, msg, &err) &&
-!(transaction = NULL))) {
+ ref_transaction_commit(transaction, msg, &err)) {
ref_transaction_rollback(transaction);
error("%s", err.buf);
strbuf_release(&err);
return -1;
}
+ ref_transaction_free(transaction);
return 0;
}
diff --git a/refs.c b/refs.c
index b4c7d04..016206b 100644
--- a/refs.c
+++ b/refs.c
@@ -3350,7 +3350,7 @@ struct ref_transaction *ref_transaction_begin(void)
return xcalloc(1, sizeof(struct ref_transaction));
}
-static void ref_transaction_free(struct ref_transaction *transaction)
+void ref_transaction_free(struct ref_transaction *transaction)
{
int i;
@@ -3451,10 +3451,10 @@ int update_ref(const char *action, const char *refname,
struct strbuf err = STRBUF_INIT;
t = ref_transaction_begin();
-
[PATCH v9 42/44] refs.c: pass a skip list to name_conflict_fn
Allow passing a list of refs to skip checking to name_conflict_fn.
There are some conditions where we want to allow a temporary conflict and skip
checking those refs. For example if we have a transaction that
1, guarantees that m is a packed refs and there is no loose ref for m
2, the transaction will delete m from the packed ref
3, the transaction will create conflicting m/m
For this case we want to be able to lock and create m/m since we know that the
conflict is only transient. I.e. the conflict will be automatically resolved
by the transaction when it deletes m.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 43 +--
1 file changed, 33 insertions(+), 10 deletions(-)
diff --git a/refs.c b/refs.c
index 6299b3d..c471811 100644
--- a/refs.c
+++ b/refs.c
@@ -798,11 +798,19 @@ struct name_conflict_cb {
const char *refname;
const char *oldrefname;
const char *conflicting_refname;
+ const char **skip;
+ int skipnum;
};
static int name_conflict_fn(struct ref_entry *entry, void *cb_data)
{
struct name_conflict_cb *data = (struct name_conflict_cb *)cb_data;
+ int i;
+ for(i = 0; i < data->skipnum; i++) {
+ if (!strcmp(entry->name, data->skip[i])) {
+ return 0;
+ }
+ }
if (data->oldrefname && !strcmp(data->oldrefname, entry->name))
return 0;
if (names_conflict(data->refname, entry->name)) {
@@ -817,15 +825,21 @@ static int name_conflict_fn(struct ref_entry *entry, void
*cb_data)
* conflicting with the name of an existing reference in dir. If
* oldrefname is non-NULL, ignore potential conflicts with oldrefname
* (e.g., because oldrefname is scheduled for deletion in the same
- * operation).
+ * operation). skip contains a list of refs we want to skip checking for
+ * conflicts with. Refs may be skipped due to us knowing that it will
+ * be deleted later during a transaction that deletes one reference and then
+ * creates a new conflicting reference. For example a rename from m to m/m.
*/
static int is_refname_available(const char *refname, const char *oldrefname,
- struct ref_dir *dir)
+ struct ref_dir *dir,
+ const char **skip, int skipnum)
{
struct name_conflict_cb data;
data.refname = refname;
data.oldrefname = oldrefname;
data.conflicting_refname = NULL;
+ data.skip = skip;
+ data.skipnum = skipnum;
sort_ref_dir(dir);
if (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, &data)) {
@@ -2037,7 +2051,8 @@ int dwim_log(const char *str, int len, unsigned char
*sha1, char **log)
static struct ref_lock *lock_ref_sha1_basic(const char *refname,
const unsigned char *old_sha1,
- int flags, int *type_p)
+ int flags, int *type_p,
+ const char **skip, int skipnum)
{
char *ref_file;
const char *orig_refname = refname;
@@ -2084,7 +2099,9 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
* name is a proper prefix of our refname.
*/
if (missing &&
-!is_refname_available(refname, NULL, get_packed_refs(&ref_cache)))
{
+!is_refname_available(refname, NULL,
+ get_packed_refs(&ref_cache),
+ skip, skipnum)) {
last_errno = ENOTDIR;
goto error_return;
}
@@ -2142,7 +2159,7 @@ struct ref_lock *lock_any_ref_for_update(const char
*refname,
const unsigned char *old_sha1,
int flags, int *type_p)
{
- return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
+ return lock_ref_sha1_basic(refname, old_sha1, flags, type_p, NULL, 0);
}
/*
@@ -2620,6 +2637,9 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
const char *symref = NULL;
+ if (!strcmp(oldrefname, newrefname))
+ return 0;
+
if (log && S_ISLNK(loginfo.st_mode))
return error("reflog for %s is a symlink", oldrefname);
@@ -2630,10 +2650,12 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
if (!symref)
return error("refname %s not found", oldrefname);
- if (!is_refname_available(newrefname, oldrefname,
get_packed_refs(&ref_cache)))
+ if (!is_refname_available(newrefname, oldrefname,
+ get_packed_refs(&ref_cache), NULL, 0))
return 1;
- if (!is_refname_available(newrefname, oldrefname,
get_loose_re
[PATCH v9 11/44] refs.c: change ref_transaction_create to do error checking and return status
Do basic error checking in ref_transaction_create() and make it return
non-zero on error. Update all callers to check the result of
ref_transaction_create(). There are currently no conditions in _create that
will return error but there will be in the future. Add an err argument that
will be updated on failure.
Signed-off-by: Ronnie Sahlberg
---
builtin/update-ref.c | 4 +++-
refs.c | 18 --
refs.h | 12
3 files changed, 23 insertions(+), 11 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 9f328b2..e9c216e 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -226,7 +226,9 @@ static const char *parse_cmd_create(struct strbuf *input,
const char *next)
if (*next != line_termination)
die("create %s: extra input: %s", refname, next);
- ref_transaction_create(transaction, refname, new_sha1, update_flags);
+ if (ref_transaction_create(transaction, refname, new_sha1,
+ update_flags, NULL))
+ die("cannot create ref '%s'", refname);
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index 3c9b981..344e27a 100644
--- a/refs.c
+++ b/refs.c
@@ -3402,18 +3402,24 @@ int ref_transaction_update(struct ref_transaction
*transaction,
return 0;
}
-void ref_transaction_create(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *new_sha1,
- int flags)
+int ref_transaction_create(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *new_sha1,
+ int flags,
+ struct strbuf *err)
{
- struct ref_update *update = add_update(transaction, refname);
+ struct ref_update *update;
+
+ if (!new_sha1 || is_null_sha1(new_sha1))
+ die("BUG: create ref with null new_sha1");
+
+ update = add_update(transaction, refname);
- assert(!is_null_sha1(new_sha1));
hashcpy(update->new_sha1, new_sha1);
hashclr(update->old_sha1);
update->flags = flags;
update->have_old = 1;
+ return 0;
}
void ref_transaction_delete(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index 11d07ee..0af5ba7 100644
--- a/refs.h
+++ b/refs.h
@@ -258,11 +258,15 @@ int ref_transaction_update(struct ref_transaction
*transaction,
* that the reference should have after the update; it must not be the
* null SHA-1. It is verified that the reference does not exist
* already.
+ * Function returns 0 on success and non-zero on failure. A failure to create
+ * means that the transaction as a whole has failed and will need to be
+ * rolled back. On failure the err buffer will be updated.
*/
-void ref_transaction_create(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *new_sha1,
- int flags);
+int ref_transaction_create(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *new_sha1,
+ int flags,
+ struct strbuf *err);
/*
* Add a reference deletion to transaction. If have_old is true, then
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 38/44] refs.c: pack all refs before we start to rename a ref
This means that most loose refs will no longer be present after the rename
which triggered a test failure since it assumes the file for an unrelated
ref would still be present after the rename.
Signed-off-by: Ronnie Sahlberg
---
refs.c| 3 +++
t/t3200-branch.sh | 2 +-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index e25345c..fb36cba 100644
--- a/refs.c
+++ b/refs.c
@@ -2635,6 +2635,9 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
return error("unable to move logfile logs/%s to
"TMP_RENAMED_LOG": %s",
oldrefname, strerror(errno));
+ if (pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE))
+ return error("unable to pack refs");
+
if (delete_ref(oldrefname, orig_sha1, REF_NODEREF)) {
error("unable to delete old %s", oldrefname);
goto rollback;
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index ac31b71..fafd38a 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -289,7 +289,7 @@ test_expect_success 'renaming a symref is not allowed' '
git symbolic-ref refs/heads/master2 refs/heads/master &&
test_must_fail git branch -m master2 master3 &&
git symbolic-ref refs/heads/master2 &&
- test_path_is_file .git/refs/heads/master &&
+ test_path_is_missing .git/refs/heads/master &&
test_path_is_missing .git/refs/heads/master3
'
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 36/44] refs.c: pass the ref log message to _create/delete/update instead of _commit
Change the reference transactions so that we pass the reflog message
through to the create/delete/update function instead of the commit message.
This allows for individual messages for each change in a multi ref
transaction.
Signed-off-by: Ronnie Sahlberg
---
branch.c | 4 ++--
builtin/commit.c | 4 ++--
builtin/fetch.c| 10 --
builtin/receive-pack.c | 5 +++--
builtin/replace.c | 4 ++--
builtin/tag.c | 4 ++--
builtin/update-ref.c | 13 +++--
fast-import.c | 8
refs.c | 31 +--
refs.h | 5 -
sequencer.c| 4 ++--
walker.c | 6 +++---
12 files changed, 60 insertions(+), 38 deletions(-)
diff --git a/branch.c b/branch.c
index f78a28b..6dfdc2e 100644
--- a/branch.c
+++ b/branch.c
@@ -301,8 +301,8 @@ void create_branch(const char *head,
transaction = ref_transaction_begin();
if (!transaction ||
ref_transaction_update(transaction, ref.buf, sha1,
- null_sha1, 0, !forcing, &err) ||
- ref_transaction_commit(transaction, msg, &err))
+ null_sha1, 0, !forcing, msg, &err) ||
+ ref_transaction_commit(transaction, &err))
die("%s", err.buf);
ref_transaction_free(transaction);
}
diff --git a/builtin/commit.c b/builtin/commit.c
index 6b888f2..b361a13 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1721,8 +1721,8 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
ref_transaction_update(transaction, "HEAD", sha1,
current_head ?
current_head->object.sha1 : NULL,
- 0, !!current_head, &err) ||
- ref_transaction_commit(transaction, sb.buf, &err)) {
+ 0, !!current_head, sb.buf, &err) ||
+ ref_transaction_commit(transaction, &err)) {
rollback_index_files();
die("%s", err.buf);
}
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 5b0cc31..4603cb6 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -374,11 +374,17 @@ static int s_update_ref(const char *action,
struct ref *ref,
int check_old)
{
+ char msg[1024];
+ char *rla = getenv("GIT_REFLOG_ACTION");
+
if (dry_run)
return 0;
+ if (!rla)
+ rla = default_rla.buf;
+ snprintf(msg, sizeof(msg), "%s: %s", rla, action);
if (ref_transaction_update(transaction, ref->name, ref->new_sha1,
- ref->old_sha1, 0, check_old, NULL))
+ ref->old_sha1, 0, check_old, msg, NULL))
return STORE_REF_ERROR_OTHER;
return 0;
@@ -670,7 +676,7 @@ static int store_updated_refs(const char *raw_url, const
char *remote_name,
}
}
}
- if (ref_transaction_commit(transaction, "fetch_ref transaction", NULL))
+ if (ref_transaction_commit(transaction, NULL))
rc |= errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
STORE_REF_ERROR_OTHER;
ref_transaction_free(transaction);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 5534138..324a220 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -582,7 +582,8 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
return "shallow error";
if (ref_transaction_update(transaction, namespaced_name,
- new_sha1, old_sha1, 0, 1, &err))
+ new_sha1, old_sha1, 0, 1, "push",
+ &err))
return "failed to update";
return NULL; /* good */
}
@@ -823,7 +824,7 @@ static void execute_commands(struct command *commands,
checked_connectivity = 0;
}
}
- if (ref_transaction_commit(transaction, "push", &err))
+ if (ref_transaction_commit(transaction, &err))
error("%s", err.buf);
ref_transaction_free(transaction);
strbuf_release(&err);
diff --git a/builtin/replace.c b/builtin/replace.c
index af7f72d..4fa74c1 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -160,8 +160,8 @@ static int replace_object_sha1(const char *object_ref,
transaction = ref_transaction_begin();
if (!transaction ||
ref_transaction_update(transaction, ref, repl, prev,
- 0, !is_null_sha1(prev), &err) ||
- ref_transaction_commit(transaction, NULL, &err))
+
[PATCH v9 33/44] refs.c: remove lock_ref_sha1
lock_ref_sha1 was only called from one place in refc.c and only provided
a check that the refname was sane before adding back the initial "refs/"
part of the ref path name, the initial "refs/" that this caller had already
stripped off before calling lock_ref_sha1.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 15 +--
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/refs.c b/refs.c
index 1857e61..99b110f 100644
--- a/refs.c
+++ b/refs.c
@@ -2126,15 +2126,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
return NULL;
}
-static struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char
*old_sha1)
-{
- char refpath[PATH_MAX];
- if (check_refname_format(refname, 0))
- return NULL;
- strcpy(refpath, mkpath("refs/%s", refname));
- return lock_ref_sha1_basic(refpath, old_sha1, 0, NULL);
-}
-
struct ref_lock *lock_any_ref_for_update(const char *refname,
const unsigned char *old_sha1,
int flags, int *type_p)
@@ -2339,8 +2330,12 @@ static void try_remove_empty_parents(char *name)
/* make sure nobody touched the ref, and unlink */
static void prune_ref(struct ref_to_prune *r)
{
- struct ref_lock *lock = lock_ref_sha1(r->name + 5, r->sha1);
+ struct ref_lock *lock;
+
+ if (check_refname_format(r->name + 5, 0))
+ return;
+ lock = lock_ref_sha1_basic(r->name, r->sha1, 0, NULL);
if (lock) {
unlink_or_warn(git_path("%s", r->name));
unlock_ref(lock);
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 09/44] refs.c: remove the onerr argument to ref_transaction_commit
Since all callers now use QUIET_ON_ERR we no longer need to provide an onerr
argument any more. Remove the onerr argument from the ref_transaction_commit
signature.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
builtin/update-ref.c | 3 +--
refs.c | 22 +++---
refs.h | 3 +--
3 files changed, 9 insertions(+), 19 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 207e24d..2bef2a0 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -366,8 +366,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
if (end_null)
line_termination = '\0';
update_refs_stdin();
- if (ref_transaction_commit(transaction, msg, &err,
- UPDATE_REFS_QUIET_ON_ERR))
+ if (ref_transaction_commit(transaction, msg, &err))
die("%s", err.buf);
return 0;
}
diff --git a/refs.c b/refs.c
index e8756fd..3eb73a3 100644
--- a/refs.c
+++ b/refs.c
@@ -3444,8 +3444,7 @@ static int ref_update_compare(const void *r1, const void
*r2)
}
static int ref_update_reject_duplicates(struct ref_update **updates, int n,
- struct strbuf *err,
- enum action_on_err onerr)
+ struct strbuf *err)
{
int i;
for (i = 1; i < n; i++)
@@ -3455,22 +3454,13 @@ static int ref_update_reject_duplicates(struct
ref_update **updates, int n,
if (err)
strbuf_addf(err, str, updates[i]->refname);
- switch (onerr) {
- case UPDATE_REFS_MSG_ON_ERR:
- error(str, updates[i]->refname); break;
- case UPDATE_REFS_DIE_ON_ERR:
- die(str, updates[i]->refname); break;
- case UPDATE_REFS_QUIET_ON_ERR:
- break;
- }
return 1;
}
return 0;
}
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, struct strbuf *err,
- enum action_on_err onerr)
+ const char *msg, struct strbuf *err)
{
int ret = 0, delnum = 0, i;
const char **delnames;
@@ -3485,7 +3475,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Copy, sort, and reject duplicate refs */
qsort(updates, n, sizeof(*updates), ref_update_compare);
- ret = ref_update_reject_duplicates(updates, n, err, onerr);
+ ret = ref_update_reject_duplicates(updates, n, err);
if (ret)
goto cleanup;
@@ -3497,7 +3487,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
(update->have_old ?
update->old_sha1 : NULL),
update->flags,
- &update->type, onerr);
+ &update->type,
+ UPDATE_REFS_QUIET_ON_ERR);
if (!update->lock) {
if (err)
strbuf_addf(err, "Cannot lock the ref '%s'.",
@@ -3515,7 +3506,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
ret = update_ref_write(msg,
update->refname,
update->new_sha1,
- update->lock, err, onerr);
+ update->lock, err,
+ UPDATE_REFS_QUIET_ON_ERR);
update->lock = NULL; /* freed by update_ref_write */
if (ret)
goto cleanup;
diff --git a/refs.h b/refs.h
index 25ae110..555ee59 100644
--- a/refs.h
+++ b/refs.h
@@ -278,8 +278,7 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
* the transaction failed. The string does not end in newline.
*/
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, struct strbuf *err,
- enum action_on_err onerr);
+ const char *msg, struct strbuf *err);
/** Lock a ref and then write its file */
int update_ref(const char *action, const char *refname,
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 40/44] refs.c: call lock_ref_sha1_basic directly from commit
Skip using the lock_any_ref_for_update wrapper and call lock_ref_sha1_basic
directly from the commit function.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index baa620f..87113af 100644
--- a/refs.c
+++ b/refs.c
@@ -3521,12 +3521,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
- update->lock = lock_any_ref_for_update(update->refname,
- (update->have_old ?
- update->old_sha1 :
- NULL),
- update->flags,
- &update->type);
+ update->lock = lock_ref_sha1_basic(update->refname,
+ (update->have_old ?
+ update->old_sha1 :
+ NULL),
+ update->flags,
+ &update->type);
if (!update->lock) {
if (err)
strbuf_addf(err, "Cannot lock the ref '%s'.",
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 01/44] refs.c: constify the sha arguments for ref_transaction_create|delete|update
ref_transaction_create|delete|update has no need to modify the sha1
arguments passed to it so it should use const unsigned char* instead
of unsigned char*.
Some functions, such as fast_forward_to(), already have its old/new
sha1 arguments as consts. This function will at some point need to
use ref_transaction_update() in which case this change is required.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
refs.c | 7 ---
refs.h | 7 ---
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index 6898263..2114748 100644
--- a/refs.c
+++ b/refs.c
@@ -3338,7 +3338,8 @@ static struct ref_update *add_update(struct
ref_transaction *transaction,
void ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1, unsigned char *old_sha1,
+ const unsigned char *new_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old)
{
struct ref_update *update = add_update(transaction, refname);
@@ -3352,7 +3353,7 @@ void ref_transaction_update(struct ref_transaction
*transaction,
void ref_transaction_create(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1,
+ const unsigned char *new_sha1,
int flags)
{
struct ref_update *update = add_update(transaction, refname);
@@ -3366,7 +3367,7 @@ void ref_transaction_create(struct ref_transaction
*transaction,
void ref_transaction_delete(struct ref_transaction *transaction,
const char *refname,
- unsigned char *old_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old)
{
struct ref_update *update = add_update(transaction, refname);
diff --git a/refs.h b/refs.h
index 09ff483..6d97700 100644
--- a/refs.h
+++ b/refs.h
@@ -245,7 +245,8 @@ void ref_transaction_rollback(struct ref_transaction
*transaction);
*/
void ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1, unsigned char *old_sha1,
+ const unsigned char *new_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old);
/*
@@ -256,7 +257,7 @@ void ref_transaction_update(struct ref_transaction
*transaction,
*/
void ref_transaction_create(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1,
+ const unsigned char *new_sha1,
int flags);
/*
@@ -266,7 +267,7 @@ void ref_transaction_create(struct ref_transaction
*transaction,
*/
void ref_transaction_delete(struct ref_transaction *transaction,
const char *refname,
- unsigned char *old_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old);
/*
--
2.0.0.rc3.477.gffe78a2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 04/44] refs.c: add an err argument to repack_without_refs
Update repack_without_refs to take an err argument and update it if there
is a failure. Pass the err variable from ref_transaction_commit to this
function so that callers can print a meaningful error message if _commit
fails due to a problem in repack_without_refs.
Signed-off-by: Ronnie Sahlberg
---
cache.h| 2 ++
lockfile.c | 17 ++---
refs.c | 25 +++--
3 files changed, 31 insertions(+), 13 deletions(-)
diff --git a/cache.h b/cache.h
index 8c6cdc2..48548d6 100644
--- a/cache.h
+++ b/cache.h
@@ -559,6 +559,8 @@ struct lock_file {
#define LOCK_DIE_ON_ERROR 1
#define LOCK_NODEREF 2
extern int unable_to_lock_error(const char *path, int err);
+extern void unable_to_lock_strbuf(const char *path, int err,
+ struct strbuf *buf);
extern NORETURN void unable_to_lock_index_die(const char *path, int err);
extern int hold_lock_file_for_update(struct lock_file *, const char *path,
int);
extern int hold_lock_file_for_append(struct lock_file *, const char *path,
int);
diff --git a/lockfile.c b/lockfile.c
index 8fbcb6a..67e9bb5 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -157,9 +157,8 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
return lk->fd;
}
-static char *unable_to_lock_message(const char *path, int err)
+void unable_to_lock_strbuf(const char *path, int err, struct strbuf *buf)
{
- struct strbuf buf = STRBUF_INIT;
if (err == EEXIST) {
strbuf_addf(&buf, "Unable to create '%s.lock': %s.\n\n"
@@ -170,20 +169,24 @@ static char *unable_to_lock_message(const char *path, int
err)
} else
strbuf_addf(&buf, "Unable to create '%s.lock': %s",
absolute_path(path), strerror(err));
- return strbuf_detach(&buf, NULL);
}
int unable_to_lock_error(const char *path, int err)
{
- char *msg = unable_to_lock_message(path, err);
- error("%s", msg);
- free(msg);
+ struct strbuf buf = STRBUF_INIT;
+
+ unable_to_lock_strbuf(path, err, &buf);
+ error("%s", buf.buf);
+ strbuf_release(&buf);
return -1;
}
NORETURN void unable_to_lock_index_die(const char *path, int err)
{
- die("%s", unable_to_lock_message(path, err));
+ struct strbuf buf = STRBUF_INIT;
+
+ unable_to_lock_strbuf(path, err, &buf);
+ die("%s", buf.buf);
}
int hold_lock_file_for_update(struct lock_file *lk, const char *path, int
flags)
diff --git a/refs.c b/refs.c
index 1a7f9d9..66938d7 100644
--- a/refs.c
+++ b/refs.c
@@ -2208,6 +2208,7 @@ int commit_packed_refs(void)
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(&ref_cache);
int error = 0;
+ int save_errno;
if (!packed_ref_cache->lock)
die("internal error: packed-refs not locked");
@@ -2217,10 +2218,13 @@ int commit_packed_refs(void)
do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),
0, write_packed_entry_fn,
&packed_ref_cache->lock->fd);
- if (commit_lock_file(packed_ref_cache->lock))
+ if (commit_lock_file(packed_ref_cache->lock)) {
+ save_errno = errno;
error = -1;
+ }
packed_ref_cache->lock = NULL;
release_packed_ref_cache(packed_ref_cache);
+ errno = save_errno;
return error;
}
@@ -2427,12 +2431,12 @@ static int curate_packed_ref_fn(struct ref_entry
*entry, void *cb_data)
return 0;
}
-static int repack_without_refs(const char **refnames, int n)
+static int repack_without_refs(const char **refnames, int n, struct strbuf
*err)
{
struct ref_dir *packed;
struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
struct string_list_item *ref_to_delete;
- int i, removed = 0;
+ int i, ret, removed = 0;
/* Look for a packed ref */
for (i = 0; i < n; i++)
@@ -2444,6 +2448,11 @@ static int repack_without_refs(const char **refnames,
int n)
return 0; /* no refname exists in packed refs */
if (lock_packed_refs(0)) {
+ if (err) {
+ unable_to_lock_strbuf(git_path("packed-refs"), errno,
+ err);
+ return 1;
+ }
unable_to_lock_error(git_path("packed-refs"), errno);
return error("cannot delete '%s' from packed refs",
refnames[i]);
}
@@ -2470,12 +2479,16 @@ static int repack_without_refs(const char **refnames,
int n)
}
/* Write what remains */
- return commit_packed_refs();
+ ret = commit_packed_refs();
+ if (ret && err)
+ strbuf_addf(err, "unable to overwrite old ref-pack file: %s",
+ strerror(errno));
+ return ret;
}
static int repack_without_ref(const char *refname
Re: [PATCH v8 04/44] refs.c: add an err argument to repack_without_refs
On Thu, May 15, 2014 at 11:38 AM, Jonathan Nieder wrote:
> Ronnie Sahlberg wrote:
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2427,12 +2427,12 @@ static int curate_packed_ref_fn(struct ref_entry
>> *entry, void *cb_data)
>> return 0;
>> }
>>
>> -static int repack_without_refs(const char **refnames, int n)
>> +static int repack_without_refs(const char **refnames, int n, struct strbuf
>> *err)
>
> Should this also get an onerr flag to suppress the message to stderr,
> or unconditionally suppress the message to stderr when err != NULL?
Unconditionally suppress the message I think. And is what I did.
>
> [...]
>> @@ -2445,6 +2445,9 @@ static int repack_without_refs(const char **refnames,
>> int n)
>>
>> if (lock_packed_refs(0)) {
>> unable_to_lock_error(git_path("packed-refs"), errno);
>> + if (err)
>> + strbuf_addf(err, "cannot delete '%s' from packed refs",
>> + refnames[i]);
>
> unable_to_lock_error is able to come up with a message with more
> detail (path so the sysadmin can hunt down the problem even if this
> was run e.g. from a cronjob where the path is not obvious, errno
> hinting at the nature of the problem).
I changed it so that if err is non-NULL we call a new function that updates the
strbuf with a nice message explaining the failure and then return 1
without writing anything to stderr.
For the NULL case I left it to continue logging two lines of errors
"Unable to create '%s.lock': %s.\n\n"
"cannot delete '%s' from packed refs"
That second line should probably go away since the error at this stage
is related to locking the packed
refs file itself and has nothing really to do with a particular ref
such as refnames[i]
>
> [...]
>> @@ -2470,12 +2473,15 @@ static int repack_without_refs(const char
>> **refnames, int n)
>> }
>>
>> /* Write what remains */
>> - return commit_packed_refs();
>> + ret = commit_packed_refs();
>> + if (ret && err)
>> + strbuf_addf(err, "unable to overwrite old ref-pack file");
>
> After commit_lock_file sets errno, amazingly no one clobbers it
> until we get to this point. The only calls in between are to
> free().
>
> It would be nice to make that more explicit in commit_packed_refs:
>
> int save_errno;
>
> ...
> if (commit_lock_file(packed_ref_cache->lock)) {
> save_errno = errno;
> error = -1;
> }
>
> packed_ref_cache->lock = NULL;
> release_packed_ref_cache(packed_ref_cache);
>
> errno = save_errno;
> return error;
>
> Even without that, this message could include strerror(errno).
Done and Done.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GUILT v2 14/29] Use "git check-ref-format" to validate patch names.
On Tue, May 13, 2014 at 10:30:50PM +0200, Per Cederqvist wrote:
> The valid_patchname now lets "git check-ref-format" do its job instead
> of trying (and failing) to implement the same rules. See
> git-check-ref-format(1) for a list of the rules.
>
> Refer to the git-check-ref-format(1) man page in the error messages
> produced when valid_patchname indicates that the name is bad.
>
> Added testcases that breaks most of the rules in that man-page.
>
> Git version 1.8.5 no longer allows the single character "@" as a
> branch name. Guilt always rejects that name, for increased
> compatibility.
Very nice.
Signed-off-by: Josef 'Jeff' Sipek
> Signed-off-by: Per Cederqvist
> ---
> guilt| 21 ++-
> guilt-fork | 2 +-
> guilt-import | 2 +-
> guilt-new| 2 +-
> regression/t-025.out | 426
> +--
> regression/t-025.sh | 12 +-
> regression/t-032.out | 4 +-
> 7 files changed, 446 insertions(+), 23 deletions(-)
>
> diff --git a/guilt b/guilt
> index 3fc524e..23cc2da 100755
> --- a/guilt
> +++ b/guilt
> @@ -132,14 +132,19 @@ fi
> # usage: valid_patchname
> valid_patchname()
> {
> - case "$1" in
> - /*|./*|../*|*/./*|*/../*|*/.|*/..|*/|*\ *|*\*)
> - return 1;;
> - *:*)
> - return 1;;
> - *)
> - return 0;;
> - esac
> + if git check-ref-format --allow-onelevel "$1"; then
> + # Starting with Git version 1.8.5, a branch cannot be
> + # the single character "@". Make sure guilt rejects
> + # that name even if we are currently using an older
> + # version of Git. This ensures that the test suite
> + # runs fine using any version of Git.
> + if [ "$1" = "@" ]; then
> + return 1
> + fi
> + return 0
> + else
> + return 1
> + fi
> }
>
> get_branch()
> diff --git a/guilt-fork b/guilt-fork
> index a85d391..6447e55 100755
> --- a/guilt-fork
> +++ b/guilt-fork
> @@ -37,7 +37,7 @@ else
> fi
>
> if ! valid_patchname "$newpatch"; then
> - die "The specified patch name contains invalid characters (:)."
> + die "The specified patch name is invalid according to
> git-check-ref-format(1)."
> fi
>
> if [ -e "$GUILT_DIR/$branch/$newpatch" ]; then
> diff --git a/guilt-import b/guilt-import
> index 3e9b3bb..928e325 100755
> --- a/guilt-import
> +++ b/guilt-import
> @@ -40,7 +40,7 @@ if [ -e "$GUILT_DIR/$branch/$newname" ]; then
> fi
>
> if ! valid_patchname "$newname"; then
> - die "The specified patch name contains invalid characters (:)."
> + die "The specified patch name is invalid according to
> git-check-ref-format(1)."
> fi
>
> # create any directories as needed
> diff --git a/guilt-new b/guilt-new
> index 9528438..9f7fa44 100755
> --- a/guilt-new
> +++ b/guilt-new
> @@ -64,7 +64,7 @@ fi
>
> if ! valid_patchname "$patch"; then
> disp "Patchname is invalid." >&2
> - die "it cannot begin with '/', './' or '../', or contain /./, /../, or
> whitespace"
> + die "It must follow the rules in git-check-ref-format(1)."
> fi
>
> # create any directories as needed
> diff --git a/regression/t-025.out b/regression/t-025.out
> index 7811ab1..01bc406 100644
> --- a/regression/t-025.out
> +++ b/regression/t-025.out
> @@ -141,7 +141,7 @@ f da39a3ee5e6b4b0d3255bfef95601890afd80709
> .git/patches/master/prepend
> r acdeef96ee30eb34bbbf65d11de5cf7da4b5fee8 .git/refs/patches/master/prepend
> % guilt new white space
> Patchname is invalid.
> -it cannot begin with '/', './' or '../', or contain /./, /../, or whitespace
> +It must follow the rules in git-check-ref-format(1).
> % list_files
> d .git/patches
> d .git/patches/master
> @@ -211,7 +211,7 @@ f da39a3ee5e6b4b0d3255bfef95601890afd80709
> .git/patches/master/prepend
> r acdeef96ee30eb34bbbf65d11de5cf7da4b5fee8 .git/refs/patches/master/prepend
> % guilt new /abc
> Patchname is invalid.
> -it cannot begin with '/', './' or '../', or contain /./, /../, or whitespace
> +It must follow the rules in git-check-ref-format(1).
> % list_files
> d .git/patches
> d .git/patches/master
> @@ -235,7 +235,7 @@ f da39a3ee5e6b4b0d3255bfef95601890afd80709
> .git/patches/master/prepend
> r acdeef96ee30eb34bbbf65d11de5cf7da4b5fee8 .git/refs/patches/master/prepend
> % guilt new ./blah
> Patchname is invalid.
> -it cannot begin with '/', './' or '../', or contain /./, /../, or whitespace
> +It must follow the rules in git-check-ref-format(1).
> % list_files
> d .git/patches
> d .git/patches/master
> @@ -259,7 +259,7 @@ f da39a3ee5e6b4b0d3255bfef95601890afd80709
> .git/patches/master/prepend
> r acdeef96ee30eb34bbbf65d11de5cf7da4b5fee8 .git/refs/patches/master/prepend
> % guilt new ../blah
> Patchname is invalid.
> -it cannot begin with '/', './' or '../', or contain /./,
Re: [PATCH v8 18/44] branch.c: use ref transaction for all ref updates
Ronnie Sahlberg wrote:
> Change create_branch to use a ref transaction when creating the new branch.
> ref_transaction_create will check that the ref does not already exist and fail
> otherwise meaning that we no longer need to keep a lock on the ref during the
> setup_tracking. This simplifies the code since we can now do the transaction
> in one single step.
Previously the ref lock was also held during setup_tracking, which
sets up configuration for the branch in .git/config. So in principle
they were one transaction and this patch would change that.
The race:
checkout -B master origin/master
update-ref -d refs/heads/master
---
checkout -B master
other/master
---
create ref
delete ref
create ref
configure tracking configure tracking
Since setup_tracking is not a single transaction, if the two processes
are lucky enough to not try to write to .git/config at the same time
then the resulting configuration could have branch.master.merge set
by the first checkout -b and branch.master.remote set by the second.
But trying to "checkout -b" the same branch in two terminals
concurrently is a somewhat insane thing to do, so I don't mind
breaking it.
[...]
> This also fixes a race condition in the old code where two concurrent
> create_branch could race since the lock_any_ref_for_update/write_ref_sha1
> did not protect against the ref already existsing. I.e. one thread could end
> up
s/existsing/existing/
> overwriting a branch even if the forcing flag is false.
Good catch.
> --- a/branch.c
> +++ b/branch.c
[...]
> @@ -301,13 +291,25 @@ void create_branch(const char *head,
[...]
> + if (!transaction ||
> + ref_transaction_update(transaction, ref.buf, sha1,
> +null_sha1, 0, !forcing) ||
> + ref_transaction_commit(transaction, msg, &err))
> + die_errno(_("%s: failed to write ref: %s"),
> + ref.buf, err.buf);
errno is not guaranteed valid here. The usual
die("%s", err.buf);
should do the trick.
With the changes mentioned above (commit message typofix, die()
message),
Reviewed-by: Jonathan Nieder
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] remote-helpers: point at their upstream repositories
Two announcements for their version 0.2 on the list archive are not quite enough to advertise them to their users. Signed-off-by: Junio C Hamano --- * I am inclined to queue this for 2.0, and we would also need an update to the release notes as well. I am undecided about the revert I sent earlier in $gmane/248937; with or without it, that is just a contrib/ thing that is not well maintained inside our tree anyway. contrib/remote-helpers/README | 11 +++ 1 file changed, 11 insertions(+) create mode 100644 contrib/remote-helpers/README diff --git a/contrib/remote-helpers/README b/contrib/remote-helpers/README new file mode 100644 index 000..72a2df4 --- /dev/null +++ b/contrib/remote-helpers/README @@ -0,0 +1,11 @@ +The remote-helper bridges to access data stored in Hg and bzr will be +maintained outside the git.git tree in the repositories of its +primary author at: + +https://github.com/felipec/git-remote-hg +https://github.com/felipec/git-remote-bzr + +As a convenience, copies of the last-bundled version of these two +remote-helper bridges are kept here, but they may left stale. Users +are encouraged to visit the above authoritative repositories for the +latest versions to get involved in its further developments. -- 2.0.0-rc3-431-gba4c34e -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 01/17] contrib: remove outdated README
Felipe Contreras writes: > Junio C Hamano wrote: >> Felipe Contreras writes: >> > Junio never explained his *TECHNICAL* reason, and Michael Haggerty >> > simply said "there are good technical arguments for and against >> > moving git-remote-hg out of contrib", that was all his explanation for >> > the *TECHNICAL* reason. > >> I am not interested in distinction between technical and social that >> much. The points that were raised in the thread started by John >> Keeping, and some of the points that came to my mind while on that >> thread, even though I may not have mentioned in *that* thread, that >> affected the way *I* thought about the issue are these (not >> exhaustive): > > Let's be clear; the discussion in that thread was contrib vs. core. Most > of the points you mention below are not related to that. > > == contrib vs. core == > > This is the only point relevant to contrib vs. core: > > > - We may be painted in a hard place if remote-hg or remote-bzr take > >us to a position where the Git as a whole is blocked while it is > >incompatible with the other side. > > It will never happen. I already argued against it[1], multiple times. > Essentially making the tests optional removes any possibility of > blockage (pluse many more arguments). I already said that your "It will never happen" is a handwaving, and I already saw you "argued against it". There is no point repeating that exchange. We both know, and bystanders with popcorns in their hands also know, that we have different opinions. You may have been interested in contrib/core in the thread, but that does not prevent others from considering other aspects of the issue and different and possibly better solutions, which was to unbundle. I was very confident back in that thread that the remote Hg bridge would not merely survive but would serve its users well as a standalone project, and the level of confidence was actually a lot higher than for a hypothetical case where other "already in-core" bridges like git-svn/p4 are somehow forced to become standalone, simply because of the difference in the depths of involvement of respective area maintainers. Without meaning any disrespect to Eric or Pete, I think my prodding them (by forwarding issues and proposed patches by others to them when I see them on the list) has been helping the area maintainers who have other time and attention obligations to help us, by drawing their attention and making their workload smaller (they can only Ack and have me apply, instead of maintaining a separate tree). There is a greater risk for these bridges to become unmaintained if we do not have them in my tree, and that would only hurt our users. In the ideal world, it may be better if it weren't that way, but at the same time, new issues that changing time brings to them seem to be handled more or less OK, so we have to be content with the status quo. But I did not see that particular risk at all for the remote Hg bridge. You have been very interested in maintaining it, and I don't think I ever had to prod you to respond to an issue raised on the list. It is an apples-and-oranges comparison to bring up git-svn/p4. Besides, I want to see the "git-core has the best thing among competing implementations for a specific niche subtask" perception changed in the longer term so that it becomes natural for users to say something like "For this particular task, there is no support in what comes with core (or there is a tool that comes with core to address similar issues but in a way different from what you want), and the go-to tool these days for that kind of task is to use this third-party plugin", because it is simply unrealistic to expect my tree to forever be the be-all destination for everything. Things like "git imerge" are helping us to go in that direction. I was hoping that the remote Hg bridge to be capable of becoming the first demonstration to graduate out of contrib/ that shows the users that is the case, not with just talk but with a specific example. Anyway, the above is only within the discussion theme of John Keeping's thread [*1*]. You seem to be adamant that you do not consider other people's opinions that came in later threads you started [*2*], and I do not think that is a sensible way to discuss things. After seeing these discussions, it tells me that the code is not fit for the core (also [*3*]), and I think there no longer is any reason for us to still talk only about contrib vs core. As you already said, you do not want to see them in contrib/, and as you already saw, everybody other than you do not want to see them in core and some of them do not want to even see them in contrib/ for that matter. I do not see that there is any direction other than out. [Reference] *1* http://thread.gmane.org/gmane.comp.version-control.git/247660/focus=248167 *2* http://thread.gmane.org/gmane.comp.version-control.git/248705 *3* http://thread.gmane.org/gmane.comp.version-control.git/248063/
Re: [GUILT v2 13/29] Check that "guilt header '.*'" fails.
Signed-off-by: Josef 'Jeff' Sipek On Tue, May 13, 2014 at 10:30:49PM +0200, Per Cederqvist wrote: > Signed-off-by: Per Cederqvist > --- > regression/t-028.out | 7 +++ > regression/t-028.sh | 4 > 2 files changed, 11 insertions(+) > > diff --git a/regression/t-028.out b/regression/t-028.out > index 1564c09..ea72a3a 100644 > --- a/regression/t-028.out > +++ b/regression/t-028.out > @@ -49,3 +49,10 @@ Signed-off-by: Commiter Name > > % guilt header non-existant > Patch non-existant is not in the series > +% guilt header .* > +.* does not uniquely identify a patch. Did you mean any of these? > + modify > + add > + remove > + mode > + patch-with-some-desc > diff --git a/regression/t-028.sh b/regression/t-028.sh > index 88e9adb..2ce0378 100755 > --- a/regression/t-028.sh > +++ b/regression/t-028.sh > @@ -31,4 +31,8 @@ done > > shouldfail guilt header non-existant > > +# This is an evil variant of a non-existant patch. However, this > +# patch name is a regexp that just happens to match an existing patch. > +shouldfail guilt header '.*' > + > # FIXME: how do we check that -e works? > -- > 1.8.3.1 > -- Si hoc legere scis nimium eruditionis habes. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GUILT v2 12/29] "guilt header": more robust header selection.
On Tue, May 13, 2014 at 10:30:48PM +0200, Per Cederqvist wrote: > If you run something like "guilt header '.*'" the command would crash, > because the grep comand that tries to ensure that the patch exist > would detect a match, but the later code expected the match to be > exact. > > Fixed by comparing exact strings. > > And as a creeping feature "guilt header" will now try to use the > supplied patch name as an unachored regexp if no exact match was > found. If the regexp yields a unique match, it is used; if more than > one patch matches, the names of all patches are listed and the command > fails. (Exercise left to the reader: generalized this so that "guilt > push" also accepts a unique regular expression.) > > Signed-off-by: Per Cederqvist > --- > guilt-header | 28 +--- > 1 file changed, 25 insertions(+), 3 deletions(-) > > diff --git a/guilt-header b/guilt-header > index 41e00cc..4701b31 100755 > --- a/guilt-header > +++ b/guilt-header > @@ -45,10 +45,32 @@ esac > [ -z "$patch" ] && die "No patches applied." > > # check that patch exists in the series > -ret=`get_full_series | grep -e "^$patch\$" | wc -l` > -if [ $ret -eq 0 ]; then > - die "Patch $patch is not in the series" > +full_series=`get_tmp_file series` > +get_full_series > "$full_series" > +found_patch= > +while read x; do > + if [ "$x" = "$patch" ]; then > + found_patch="$patch" > + break > + fi > +done < "$full_series" We have to use a temp file instead of a 'get_full_series | while read x; do ...' because that'd create a subshell, correct? > +if [ -z "$found_patch" ]; then > + TMP_MATCHES=`get_tmp_file series` > + grep "$patch" < "$full_series" > "$TMP_MATCHES" > + nr=`wc -l < $TMP_MATCHES` > + if [ $nr -gt 1 ]; then > + echo "$patch does not uniquely identify a patch. Did you mean > any of these?" >&2 > + sed 's/^/ /' "$TMP_MATCHES" >&2 > + rm -f "$TMP_MATCHES" > + exit 1 > + elif [ $nr -eq 0 ]; then > + rm -f "$TMP_MATCHES" > + die "Patch $patch is not in the series" > + fi > + found_patch=`cat $TMP_MATCHES` > + rm -f "$TMP_MATCHES" > fi > +patch="$found_patch" Do we not delete $full_series? > > # FIXME: warn if we're editing an applied patch > > -- > 1.8.3.1 > -- OpenIndiana ibdm: 8 cores, 12 GB RAM -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 14/44] replace.c: use the ref transaction functions for updates
On Thu, May 15, 2014 at 2:18 PM, Jonathan Nieder wrote:
> Ronnie Sahlberg wrote:
>
>> --- a/builtin/replace.c
>> +++ b/builtin/replace.c
> [...]
>> @@ -156,11 +157,12 @@ static int replace_object_sha1(const char *object_ref,
>> else if (!force)
>> die("replace ref '%s' already exists", 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)
>> - die("%s: cannot update the ref", ref);
>> + transaction = ref_transaction_begin();
>> + if (!transaction ||
>> + ref_transaction_update(transaction, ref, repl, prev,
>> +0, !is_null_sha1(prev)) ||
>> + ref_transaction_commit(transaction, NULL, &err))
>> + die("%s: failed to replace ref: %s", ref, err.buf);
>
> This would write
>
> fatal: refs/replace/09c779943364d893c190066c385e6112af421db3: failed
> to replace ref: Cannot lock the ref
> 'refs/replace/09c779943364d893c190066c385e6112af421db3'.
>
> Perhaps something like
>
> $ git replace foo bar
> fatal: replace foo: Cannot lock the ref
> 'refs/replace/09c779943364d893c190066c385e6112af421db3'.
>
> would make sense (die("replace %s: %s", object_ref, err.buf)). Plain
>
> $ git replace foo bar
> fatal: Cannot lock the ref
> 'refs/replace/09c779943364d893c190066c385e6112af421db3'.
>
> also seems fine (die("%s", err.buf)).
Done.
I used : die("%s", err.buf)
since this is consistent with most of the other if-transaction-failed-die()
>
> Thanks,
> Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 13/44] tag.c: use ref transactions when doing updates
On Thu, May 15, 2014 at 2:11 PM, Jonathan Nieder wrote:
> Ronnie Sahlberg wrote:
>
>> --- a/builtin/tag.c
>> +++ b/builtin/tag.c
>> @@ -701,11 +702,12 @@ int cmd_tag(int argc, const char **argv, const char
>> *prefix)
>> if (annotate)
>> create_tag(object, tag, &buf, &opt, prev, object);
>>
>> - lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL);
>> - if (!lock)
>> - die(_("%s: cannot lock the ref"), ref.buf);
>> - if (write_ref_sha1(lock, object, NULL) < 0)
>> - die(_("%s: cannot update the ref"), ref.buf);
>> + transaction = ref_transaction_begin();
>> + if (!transaction ||
>> + ref_transaction_update(transaction, ref.buf, object, prev,
>> +0, !is_null_sha1(prev)) ||
>> + ref_transaction_commit(transaction, NULL, &err))
>> + die(_("%s: cannot update the ref: %s"), ref.buf, err.buf);
>
> The error string says what ref it was trying to update, so
>
> die("%s", err.buf);
>
> should be enough. (E.g.,
>
> fatal: refs/tags/v1.0: cannot lock the ref
>
> would become
>
> fatal: Cannot lock the ref 'refs/tags/v1.0'.
>
> instead of
>
> fatal: refs/tags/v1.0: cannot update the ref: Cannot lock the ref
> 'refs/tags/v1.0'.
>
> .)
>
Done.
Thanks!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 17/44] fast-import.c: change update_branch to use ref transactions
On Thu, May 15, 2014 at 2:47 PM, Jonathan Nieder wrote:
> Ronnie Sahlberg wrote:
>
>> --- a/fast-import.c
>> +++ b/fast-import.c
>> @@ -1679,39 +1679,45 @@ found_entry:
>> static int update_branch(struct branch *b)
>> {
>> static const char *msg = "fast-import";
>> - struct ref_lock *lock;
>> + struct ref_transaction *transaction;
>> unsigned char old_sha1[20];
>> + struct strbuf err = STRBUF_INIT;
>>
>> if (read_ref(b->name, old_sha1))
>> hashclr(old_sha1);
>> +
>> if (is_null_sha1(b->sha1)) {
>> if (b->delete)
>> delete_ref(b->name, old_sha1, 0);
>> return 0;
>> }
>> - lock = lock_any_ref_for_update(b->name, old_sha1, 0, NULL);
>> - if (!lock)
>> - return error("Unable to lock %s", b->name);
>> if (!force_update && !is_null_sha1(old_sha1)) {
>> struct commit *old_cmit, *new_cmit;
>>
>> old_cmit = lookup_commit_reference_gently(old_sha1, 0);
>> new_cmit = lookup_commit_reference_gently(b->sha1, 0);
>> if (!old_cmit || !new_cmit) {
>> - unlock_ref(lock);
>> return error("Branch %s is missing commits.", b->name);
>> }
>
> (style) Now that there's only one line in the "if" body, we can
> drop the braces.
>
Done.
>>
>> if (!in_merge_bases(old_cmit, new_cmit)) {
>> - unlock_ref(lock);
>> warning("Not updating %s"
>> " (new tip %s does not contain %s)",
>> b->name, sha1_to_hex(b->sha1),
>> sha1_to_hex(old_sha1));
>> return -1;
>
> (not about this patch, feel free to ignore) This could
> return warning("...")
Ignored.
>
>> }
>> - if (write_ref_sha1(lock, b->sha1, msg) < 0)
>> - return error("Unable to update %s", b->name);
>> + transaction = ref_transaction_begin();
>> + if ((!transaction ||
>> + ref_transaction_update(transaction, b->name, b->sha1, old_sha1,
>> +0, 1)) ||
>
> Would be more idiomatic to drop a layer of parentheses:
>
> if (!transaction ||
> ref_transaction_update(...) ||
>
>> + (ref_transaction_commit(transaction, msg, &err) &&
>> + !(transaction = NULL))) {
>
Done.
> Would be clearer if ref_transaction_commit didn't take care of the
> rollback (or in other words if patch 21 were earlier in the series).
True, but first making it correct, but ugly, here makes one appreciate
patch 21 more. That patch makes it correct and not ugly :-)
Leaving as is.
>
>> + ref_transaction_rollback(transaction);
>> + error("Unable to update branch %s: %s", b->name, err.buf);
>> + strbuf_release(&err);
>> + return -1;
>> + }
>
> Example old message:
>
> error: Unable to lock refs/heads/master
>
> New message:
>
> error: Unable to update branch refs/heads/master: Cannot lock the ref
> 'refs/heads/master'.
>
> So 'error("%s", err.buf)' would probably work better.
Done.
>
> The only call site is dump_branches:
>
> for (i = 0; i < branch_table_sz; i++) {
> for (b = branch_table[i]; b; b = b->table_next_branch)
> failure |= update_branch(b);
> }
>
> Should these happen in a single transaction? I haven't thought
> through whether it would be a good idea, if it should be optional, or
> what.
>
> Anyway, that would be a bigger behavior change, but it's interesting
> to think about.
I will try to add it to the next patch series.
>
> Thanks,
> Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: regression: request-pull with signed tag lacks tags/ in master
"Michael S. Tsirkin" writes: >> My reading of the earlier parts of the series is that Linus wanted >> us never dwim "for-upstream" to "tags/for-upstream" or any other ref >> that happens to point at the same commit as for-upstream you have. >> The changes done for that purpose covered various cases a bit too >> widely, and "request-pull ... tags/for_upstream" were incorrectly >> stripped to a request to pull "for_upstream", which was fixed by >> 5aae66bd (request-pull: resurrect "pretty refname" feature, >> 2014-02-25). >> >> But that fix does not resurrect the dwimming forbid by the earlier >> parts of the series to turn "for_upstream" into "tags/for_upstream". >> >> What would you get if you do this? >> >> $ git request-pull origin/master \ >> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git \ >> tags/for_upstream | grep git.kernel.org > > > I get > git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream Thanks for double-checking. Let's close this as working as intended, then. I personally feel that the "intention" tightened the logic a bit too much [*1*], and with the updates mentioned in [*2*], existing users may find it still too tight, but that is what we have today. [References] *1* http://thread.gmane.org/gmane.comp.version-control.git/240860 *2* http://thread.gmane.org/gmane.comp.version-control.git/240886 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 10/44] refs.c: change ref_transaction_update() to do error checking and return status
On Thu, May 15, 2014 at 12:34 PM, Jonathan Nieder wrote:
> Ronnie Sahlberg wrote:
>
>> Update ref_transaction_update() do some basic error checking and return
>> non-zero on error. Update all callers to check ref_transaction_update() for
>> error. There are currently no conditions in _update that will return error
>> but
>> there will be in the future.
>
> Probably worth passing a 'struct strbuf *err' argument. Then callers
> can do
>
> die("%s", err.buf);
>
Done.
> and the error message can say which ref and whether we were trying to
> create a ref, or delete one, or whatever.
>
>> --- a/builtin/update-ref.c
>> +++ b/builtin/update-ref.c
>> @@ -197,8 +197,9 @@ static const char *parse_cmd_update(struct strbuf
>> *input, const char *next)
>> if (*next != line_termination)
>> die("update %s: extra input: %s", refname, next);
>>
>> - ref_transaction_update(transaction, refname, new_sha1, old_sha1,
>> -update_flags, have_old);
>> + if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
>> +update_flags, have_old))
>> + die("update %s: failed", refname);
>
> This could say
>
> die("update %s: %s", refname, err.buf);
Done.
>
> to give context about which command it was trying to execute.
>
> [...]
>> @@ -286,8 +287,9 @@ static const char *parse_cmd_verify(struct strbuf
>> *input, const char *next)
>> if (*next != line_termination)
>> die("verify %s: extra input: %s", refname, next);
>>
>> - ref_transaction_update(transaction, refname, new_sha1, old_sha1,
>> -update_flags, have_old);
>> + if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
>> +update_flags, have_old))
>> + die("failed transaction update for %s", refname);
>
> And this could say
>
> die("verify %s: %s", refname, err.buf);
Done.
>
> [...]
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -242,12 +242,15 @@ void ref_transaction_rollback(struct ref_transaction
>> *transaction);
>> * be deleted. If have_old is true, then old_sha1 holds the value
>> * that the reference should have had before the update, or zeros if
>> * it must not have existed beforehand.
>> + * Function returns 0 on success and non-zero on failure. A failure to
>> update
>> + * means that the transaction as a whole has failed and will need to be
>> + * rolled back.
>> + */
>
> Thanks for this documentation.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 11/44] refs.c: change ref_transaction_create to do error checking and return status
On Thu, May 15, 2014 at 12:44 PM, Jonathan Nieder wrote: > Ronnie Sahlberg wrote: > >> Do basic error checking in ref_transaction_create() and make it return >> non-zero on error. > > Same thoughts as _update(). Basic idea is good but would be nice to > have a 'struct strbuf *err' parameter. Done. > > Thanks, > Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 12/44] refs.c: ref_transaction_delete to check for error and return status
On Thu, May 15, 2014 at 12:51 PM, Jonathan Nieder wrote: > Ronnie Sahlberg wrote: > >> Change ref_transaction_delete() to do basic error checking and return >> non-zero of error. > > Likewise: a 'struct strbuf *err' would make nicer error messages > possible. Done. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 16/44] sequencer.c: use ref transactions for all ref updates
Ronnie Sahlberg wrote:
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -272,23 +272,31 @@ static int fast_forward_to(const unsigned char *to,
> const unsigned char *from,
[...]
> + error(_("HEAD: Could not fast-forward: %s\n"), err.buf);
As with 'git commit', since err.buf already mentions that HEAD was the
problematic ref I think it has enough information for the user to
diagnose the problem and figure out what to do next.
error("%s", err.buf);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 17/44] fast-import.c: change update_branch to use ref transactions
Ronnie Sahlberg wrote:
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -1679,39 +1679,45 @@ found_entry:
> static int update_branch(struct branch *b)
> {
> static const char *msg = "fast-import";
> - struct ref_lock *lock;
> + struct ref_transaction *transaction;
> unsigned char old_sha1[20];
> + struct strbuf err = STRBUF_INIT;
>
> if (read_ref(b->name, old_sha1))
> hashclr(old_sha1);
> +
> if (is_null_sha1(b->sha1)) {
> if (b->delete)
> delete_ref(b->name, old_sha1, 0);
> return 0;
> }
> - lock = lock_any_ref_for_update(b->name, old_sha1, 0, NULL);
> - if (!lock)
> - return error("Unable to lock %s", b->name);
> if (!force_update && !is_null_sha1(old_sha1)) {
> struct commit *old_cmit, *new_cmit;
>
> old_cmit = lookup_commit_reference_gently(old_sha1, 0);
> new_cmit = lookup_commit_reference_gently(b->sha1, 0);
> if (!old_cmit || !new_cmit) {
> - unlock_ref(lock);
> return error("Branch %s is missing commits.", b->name);
> }
(style) Now that there's only one line in the "if" body, we can
drop the braces.
>
> if (!in_merge_bases(old_cmit, new_cmit)) {
> - unlock_ref(lock);
> warning("Not updating %s"
> " (new tip %s does not contain %s)",
> b->name, sha1_to_hex(b->sha1),
> sha1_to_hex(old_sha1));
> return -1;
(not about this patch, feel free to ignore) This could
return warning("...")
> }
> - if (write_ref_sha1(lock, b->sha1, msg) < 0)
> - return error("Unable to update %s", b->name);
> + transaction = ref_transaction_begin();
> + if ((!transaction ||
> + ref_transaction_update(transaction, b->name, b->sha1, old_sha1,
> +0, 1)) ||
Would be more idiomatic to drop a layer of parentheses:
if (!transaction ||
ref_transaction_update(...) ||
> + (ref_transaction_commit(transaction, msg, &err) &&
> + !(transaction = NULL))) {
Would be clearer if ref_transaction_commit didn't take care of the
rollback (or in other words if patch 21 were earlier in the series).
> + ref_transaction_rollback(transaction);
> + error("Unable to update branch %s: %s", b->name, err.buf);
> + strbuf_release(&err);
> + return -1;
> + }
Example old message:
error: Unable to lock refs/heads/master
New message:
error: Unable to update branch refs/heads/master: Cannot lock the ref
'refs/heads/master'.
So 'error("%s", err.buf)' would probably work better.
The only call site is dump_branches:
for (i = 0; i < branch_table_sz; i++) {
for (b = branch_table[i]; b; b = b->table_next_branch)
failure |= update_branch(b);
}
Should these happen in a single transaction? I haven't thought
through whether it would be a good idea, if it should be optional, or
what.
Anyway, that would be a bigger behavior change, but it's interesting
to think about.
Thanks,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: regression: request-pull with signed tag lacks tags/ in master
On Thu, May 15, 2014 at 12:13:18PM -0700, Junio C Hamano wrote: > Junio C Hamano writes: > > > "Michael S. Tsirkin" writes: > > > >> looks like pull requests with signed git got broken in git master: > >> > >> [mst@robin qemu]$ /usr/bin/git --version > >> git version 1.8.3.1 > >> [mst@robin qemu]$ git --version > >> git version 2.0.0.rc1.18.gac53fc6.dirty > >> [mst@robin qemu]$ > >> [mst@robin qemu]$ /usr/bin/git request-pull origin/master > >> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git for_upstream |grep > >> git.kernel.org > >> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream > >> > >> > >> [mst@robin qemu]$ git request-pull origin/master > >> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git for_upstream |grep > >> git.kernel.org > >> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git for_upstream > >> > >> this for_upstream syntax is a problem because it does not work > >> for older git clients who might get this request. > >> > >> Didn't bisect yet - anyone knows what broke this? > > > > Linus ;-) The series that ends with ec44507, I think. > > My reading of the earlier parts of the series is that Linus wanted > us never dwim "for-upstream" to "tags/for-upstream" or any other ref > that happens to point at the same commit as for-upstream you have. > The changes done for that purpose covered various cases a bit too > widely, and "request-pull ... tags/for_upstream" were incorrectly > stripped to a request to pull "for_upstream", which was fixed by > 5aae66bd (request-pull: resurrect "pretty refname" feature, > 2014-02-25). > > But that fix does not resurrect the dwimming forbid by the earlier > parts of the series to turn "for_upstream" into "tags/for_upstream". > > What would you get if you do this? > > $ git request-pull origin/master \ > git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git \ > tags/for_upstream | grep git.kernel.org I get git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 15/44] commit.c: use ref transactions for updates
Ronnie Sahlberg wrote: > Change commit.c to use ref transactions for all ref updates. > Make sure we pass a NULL pointer to ref_transaction_update if have_old > is false. Once ref_transaction_begin() and ref_transaction_update() set err, this will always have a reasonable message to print in die(). Reviewed-by: Jonathan Nieder -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 14/44] replace.c: use the ref transaction functions for updates
Ronnie Sahlberg wrote:
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
[...]
> @@ -156,11 +157,12 @@ static int replace_object_sha1(const char *object_ref,
> else if (!force)
> die("replace ref '%s' already exists", 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)
> - die("%s: cannot update the ref", ref);
> + transaction = ref_transaction_begin();
> + if (!transaction ||
> + ref_transaction_update(transaction, ref, repl, prev,
> +0, !is_null_sha1(prev)) ||
> + ref_transaction_commit(transaction, NULL, &err))
> + die("%s: failed to replace ref: %s", ref, err.buf);
This would write
fatal: refs/replace/09c779943364d893c190066c385e6112af421db3: failed to
replace ref: Cannot lock the ref
'refs/replace/09c779943364d893c190066c385e6112af421db3'.
Perhaps something like
$ git replace foo bar
fatal: replace foo: Cannot lock the ref
'refs/replace/09c779943364d893c190066c385e6112af421db3'.
would make sense (die("replace %s: %s", object_ref, err.buf)). Plain
$ git replace foo bar
fatal: Cannot lock the ref
'refs/replace/09c779943364d893c190066c385e6112af421db3'.
also seems fine (die("%s", err.buf)).
Thanks,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 13/44] tag.c: use ref transactions when doing updates
Ronnie Sahlberg wrote:
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -701,11 +702,12 @@ int cmd_tag(int argc, const char **argv, const char
> *prefix)
> if (annotate)
> create_tag(object, tag, &buf, &opt, prev, object);
>
> - lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL);
> - if (!lock)
> - die(_("%s: cannot lock the ref"), ref.buf);
> - if (write_ref_sha1(lock, object, NULL) < 0)
> - die(_("%s: cannot update the ref"), ref.buf);
> + transaction = ref_transaction_begin();
> + if (!transaction ||
> + ref_transaction_update(transaction, ref.buf, object, prev,
> +0, !is_null_sha1(prev)) ||
> + ref_transaction_commit(transaction, NULL, &err))
> + die(_("%s: cannot update the ref: %s"), ref.buf, err.buf);
The error string says what ref it was trying to update, so
die("%s", err.buf);
should be enough. (E.g.,
fatal: refs/tags/v1.0: cannot lock the ref
would become
fatal: Cannot lock the ref 'refs/tags/v1.0'.
instead of
fatal: refs/tags/v1.0: cannot update the ref: Cannot lock the ref
'refs/tags/v1.0'.
.)
Thanks,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] Gitweb: Convert UTF-8 encoded file names
On Thu, May 15, 2014 at 9:38 PM, Junio C Hamano wrote: > Jakub Narębski writes: > >> Writing test for this would not be easy, and require some HTML >> parser (WWW::Mechanize, Web::Scraper, HTML::Query, pQuery, >> ... or low level HTML::TreeBuilder, or other low level parser). > > Hmph. Is it more than just looking for a specific run of %xx we > would expect to see in the output of the tree view for a repository > in which there is one tree with non-ASCII name? There is if we want to check (in non-fragile way) that said specific run is in 'href' *attribute* of 'a' element (link target). -- Jakub Narebski -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 06/44] refs.c: add an err argument ro delete_loose_ref
On Thu, May 15, 2014 at 12:04 PM, Jonathan Nieder wrote:
> Ronnie Sahlberg wrote:
>
>> [Subject: refs.c: add an err argument ro delete_loose_ref]
>
> s/ro/to/
> s/delete_loose_ref/delete_ref_loose/
>
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2484,17 +2484,22 @@ static int repack_without_ref(const char *refname)
>> return repack_without_refs(&refname, 1, NULL);
>> }
>>
>> -static int delete_ref_loose(struct ref_lock *lock, int flag)
>> +static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf
>> *err)
>
> Should this get an onerr flag to suppress the message to stderr
> or unconditionally suppress it when err != NULL?
>
Fixed.
I added a new function unlink_or_err that will update err if non-NULL
and unse warning() otherwise.
> [...]
>> lock->lk->filename[i] = 0;
>> - err = unlink_or_warn(lock->lk->filename);
>> + res = unlink_or_warn(lock->lk->filename);
>
> It seems like in the new error handling scheme there should be a new
> variant on wrapper.c's warn_if_unremovable:
>
> static int add_err_if_unremovable(const char *op, const char *file,
> struct strbuf *err, int rc)
> {
> int err = errno;
> if (rc < 0 && err != ENOENT) {
> strbuf_addf(err, "unable to %s %s: %s",
> op, file, strerror(errno));
> errno = err;
> }
> return rc;
> }
>
> static int unlink_or_err(const char *file, struct strbuf *err)
> {
> return add_err_if_unremovable("unlink", file, err,
> unlink(file));
> }
>
> res = unlink_or_err(lock->lk->filename, err);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git grep -O -i: if the pager is 'less', pass the '-i' option
Johannes Schindelin writes:
>> Jonathan Nieder writes:
>>
>> >>> +if (opt.ignore_case && !strcmp("less", pager))
>> >>> +string_list_append(&path_list, "-i");
>> >>
>> >> I have a feeling that this goes against the recent trend of not
>> >> mucking with the expectation of the users on their pagers, if I
>> >> recall correctly the arguments for dropping S from the default given
>> >> to an unconfigured LESS environment variable.
>> >
>> > It's just missing an explanation.
>> > ...
>> > (That's -I, not -i, because it ought to work even when the pattern
>> > contains capital letters.)
>>
>> Spot on. The change, especially with "-I", makes sense.
>
> Except that it was not tested with -I. If you change it that way and it
> stops working on Windows, it's useless to me.
That is all true, and I didn't test on Windows, but it seems that
the feature is very old in the upstream that we can rely on, so
let's take Jonathan's explanation and queue somethink like this.
-- >8 --
From: Johannes Schindelin
Date: Tue, 8 Feb 2011 00:17:24 -0600
Subject: [PATCH] git grep -O -i: if the pager is 'less', pass the '-I' option
When happens to be the magic string "less", today
git grep -O -e
helpfully passes +/ to less so you can navigate through
the results within a file using the n and shift+n keystrokes.
Alas, that doesn't do the right thing for a case-insensitive match,
i.e.
git grep -i -O -e
For that case we should pass --IGNORE-CASE to "less" so that n and
shift+n can move between results ignoring case in the pattern.
The original patch came from msysgit and used "-i", but that was not
due to lack of support for "-I" but it merely overlooked that it
ought to work even when the pattern contains capital letters.
Signed-off-by: Johannes Schindelin
Signed-off-by: Stepan Kasal
Helped-by: Jonathan Nieder
Signed-off-by: Junio C Hamano
---
builtin/grep.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/builtin/grep.c b/builtin/grep.c
index 63f8603..c0573d0 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -876,6 +876,9 @@ int cmd_grep(int argc, const char **argv, const char
*prefix)
if (len > 4 && is_dir_sep(pager[len - 5]))
pager += len - 4;
+ if (opt.ignore_case && !strcmp("less", pager))
+ string_list_append(&path_list, "-I");
+
if (!strcmp("less", pager) || !strcmp("vi", pager)) {
struct strbuf buf = STRBUF_INIT;
strbuf_addf(&buf, "+/%s%s",
--
2.0.0-rc3-419-gdb851f2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [git-users] worlds slowest git repo- what to do?
On 05/15/2014 12:06 PM, Philip Oakley wrote: > From: "John Fisher" >> I assert based on one piece of evidence ( a post from a facebook dev) >> that I now have the worlds biggest and slowest git >> repository, and I am not a happy guy. I used to have the worlds >> biggest CVS repository, but CVS can't handle multi-G >> sized files. So I moved the repo to git, because we are using that >> for our new projects. >> >> goal: >> keep 150 G of files (mostly binary) from tiny sized to over 8G in a >> version-control system. >> >> problem: >> git is absurdly slow, think hours, on fast hardware. >> >> question: >> any suggestions beyond these- >> http://git-annex.branchable.com/ >> https://github.com/jedbrown/git-fat >> https://github.com/schacon/git-media >> http://code.google.com/p/boar/ >> subversion >> You could shard. Break the problem up into smaller repositories, eg via submodules. Try ~128 shards and I'd expect that 129 small clones should complete faster than a single 150G clone, as well as being resumable etc. The first challenge will be figuring out what to shard on, and how to lay out the repository. You could have all of the large files in their own directory, and then the main repository just has symlinks into the sharded area. In that case, I would recommend sharding by date of the introduced blob, so that there's a good chance you won't need to clone everything forever; as shards with not many files for the current version could in theory be retired. Or, if the directory structure already suits it, you could "directly" use submodules. The second challenge will be writing the filter-branch script for this :-) Good luck, Sam -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 12/44] refs.c: ref_transaction_delete to check for error and return status
Ronnie Sahlberg wrote: > Change ref_transaction_delete() to do basic error checking and return > non-zero of error. Likewise: a 'struct strbuf *err' would make nicer error messages possible. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] Gitweb: Convert UTF-8 encoded file names
Jakub Narębski writes:
> Writing test for this would not be easy, and require some HTML
> parser (WWW::Mechanize, Web::Scraper, HTML::Query, pQuery,
> ... or low level HTML::TreeBuilder, or other low level parser).
Hmph. Is it more than just looking for a specific run of %xx we
would expect to see in the output of the tree view for a repository
in which there is one tree with non-ASCII name?
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index a9f57d6..f1414e1 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -7138,7 +7138,7 @@ sub git_tree {
>> my @entries = ();
>> {
>> local $/ = "\0";
>> - open my $fd, "-|", git_cmd(), "ls-tree", '-z',
>> + open my $fd, "-|encoding(UTF-8)", git_cmd(), "ls-tree", '-z',
>> ($show_sizes ? '-l' : ()), @extra_options, $hash
>> or die_error(500, "Open git-ls-tree failed");
>
> Or put
>
>binmode $fd, ':utf8';
>
> like in the rest of the code.
I expect a patch to do so and can forget about this thread myself,
then, OK?
Thanks all for digging this to the root.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 11/44] refs.c: change ref_transaction_create to do error checking and return status
Ronnie Sahlberg wrote: > Do basic error checking in ref_transaction_create() and make it return > non-zero on error. Same thoughts as _update(). Basic idea is good but would be nice to have a 'struct strbuf *err' parameter. Thanks, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Watchman support for git
On Wed, 2014-05-14 at 17:36 +0700, Duy Nguyen wrote: > >> With that in > >> mind, I think you don't need to keep a fs cache on disk at all. All > >> you need to store (in the index) is the clock value from watchman. > >> After we parse the index, we perform a "since" query to get path names > >> (and perhaps "exists" and "mode" for later). Then we set CE_VALID bit > >> on entries that are _not_ in the query result. The remaining items > >> will be lstat'd by git (see [1] and read-cache.c changes on the next > >> few patches). Assuming the number of updated files is reasonably > >> small, we won't be punished by lookup time. > > > > I considered this, but rejected it for a few reasons: > > 1. We still need to know about the untracked files. I guess we could > > do an index extension for just that, like your untracked cache. > > Yes. But consider that the number of untracked files is usually small > (otherwise 'git status' would look very messy). And your fscache would > need to store excluded file list too, which could be a lot bigger (one > pair of .[ch] -> one .o). _But_ yours would make 'git status > --ignored' work. I don't consider that a major use case for > optimization, but people may have different opinions. I don't think --ignored is a major use case. But I do think it's nice to get as much mileage as possible out of a change like this. > > 2. That doesn't eliminate opendir/readdir, I think. Those are a major > > cost. I did not thoroughly review your patches from January/February, > > but I didn't notice anything in there about this part of dir.c. > > Also the cost of open(nonexistent .gitignore) to do ignore processing. > > Assuming untracked cache is in use, opendir/readdir is replaced with > lstat. And cheap lstat can be solved with watchman without storing > anything extra. I solve open(.gitignore) too by checking its stat data > with the one in index. If matches, I reuse the version in tree. This > does not necessarily make it cheaper, but it increases locality so it > might be. _Modified_ .gitignore files have to go through > open(.gitignore), but people don't update .gitignore often. Interesting -- if all that actually works, then it's probably the right approach. > > 3. Changing a file in the index (e.g. git add) requires clearing > > the CE_VALID bit; this means that third-party libraries (e.g. jgit) > > that change the git repo need to understand this extension for correct > > operation. Maybe that's just the nature of extensions, but it's not > > something that my present patch set requires. > > I don't store CE_VALID on disk. Instead I store a new bit CE_WATCHED. > Only after loading and validating against watchman that I turn > CE_WATCHED to CE_VALID in memory. So JGit, libgit2 are not confused. > > I assume you won't change your mind about this. Which is fine to me. I > will still try out my approach with your libwatchman though. Just > curious about its performance and complexity, compared to your > approach. I am open-minded here. This code is really the first time I have looked at git's internals, and I accept that your way might be better. If you're going to try the watchman version of your approach, then we do a direct comparison. Let me know if there is something I can do to help out on that. > A bit off topic, but msys fork has another "fscache" in compat/win32. > If you could come up with a different name, maybe it'll be less > confusing for them after merging. But this is not a big deal, as this > fscache won't work on windows anyway. Does wtcache sounds like a better name? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] Gitweb: Convert UTF-8 encoded file names
On Thu, May 15, 2014 at 9:28 PM, Jakub Narębski wrote:
> On Thu, May 15, 2014 at 8:48 PM, Michael Wagner wrote:
[...]
>> The subroutine "git tree" generates the tree view. It stores the output
>> of "git ls-tree -z ..." in an array named "@entries". Printing the content
>> of this array yields the following result:
>>
>> 00644 blob 6419cd06a9461c38d4f94d9705d97eaaa887156a 520
>> Gütekriterien.txt
>>
>> This leads to the "doubled" encoding. Declaring the encoding in the call
>> to open yields the following result:
>>
>> 100644 blob 6419cd06a9461c38d4f94d9705d97eaaa887156a 520
>> Gütekriterien.txt
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index a9f57d6..f1414e1 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -7138,7 +7138,7 @@ sub git_tree {
>> my @entries = ();
>> {
>> local $/ = "\0";
>> - open my $fd, "-|", git_cmd(), "ls-tree", '-z',
>> + open my $fd, "-|encoding(UTF-8)", git_cmd(), "ls-tree", '-z',
>> ($show_sizes ? '-l' : ()), @extra_options, $hash
>> or die_error(500, "Open git-ls-tree failed");
>
> Or put
>
>binmode $fd, ':utf8';
>
> like in the rest of the code.
>
>> @entries = map { chomp; $_ } <$fd>;
Though to be exact there isn't any mechanism that ensures that
filenames in tree objects use utf-8 encoding, so perhaps a safer
solution would be to use
to_utf8($file_name)
(which respects $fallback_encoding) in appropriate places.
--
Jakub Narębski
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 10/44] refs.c: change ref_transaction_update() to do error checking and return status
Ronnie Sahlberg wrote:
> Update ref_transaction_update() do some basic error checking and return
> non-zero on error. Update all callers to check ref_transaction_update() for
> error. There are currently no conditions in _update that will return error but
> there will be in the future.
Probably worth passing a 'struct strbuf *err' argument. Then callers
can do
die("%s", err.buf);
and the error message can say which ref and whether we were trying to
create a ref, or delete one, or whatever.
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -197,8 +197,9 @@ static const char *parse_cmd_update(struct strbuf *input,
> const char *next)
> if (*next != line_termination)
> die("update %s: extra input: %s", refname, next);
>
> - ref_transaction_update(transaction, refname, new_sha1, old_sha1,
> -update_flags, have_old);
> + if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
> +update_flags, have_old))
> + die("update %s: failed", refname);
This could say
die("update %s: %s", refname, err.buf);
to give context about which command it was trying to execute.
[...]
> @@ -286,8 +287,9 @@ static const char *parse_cmd_verify(struct strbuf *input,
> const char *next)
> if (*next != line_termination)
> die("verify %s: extra input: %s", refname, next);
>
> - ref_transaction_update(transaction, refname, new_sha1, old_sha1,
> -update_flags, have_old);
> + if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
> +update_flags, have_old))
> + die("failed transaction update for %s", refname);
And this could say
die("verify %s: %s", refname, err.buf);
[...]
> --- a/refs.h
> +++ b/refs.h
> @@ -242,12 +242,15 @@ void ref_transaction_rollback(struct ref_transaction
> *transaction);
> * be deleted. If have_old is true, then old_sha1 holds the value
> * that the reference should have had before the update, or zeros if
> * it must not have existed beforehand.
> + * Function returns 0 on success and non-zero on failure. A failure to update
> + * means that the transaction as a whole has failed and will need to be
> + * rolled back.
> + */
Thanks for this documentation.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] Gitweb: Convert UTF-8 encoded file names
On Thu, May 15, 2014 at 8:48 PM, Michael Wagner wrote:
> On Thu, May 15, 2014 at 10:04:24AM +0100, Peter Krefting wrote:
>> Michael Wagner:
>>
>>>Decoding the UTF-8 encoded file name (again with an additional print
>>>statement):
>>>
>>>$ REQUEST_METHOD=GET
>>>QUERY_STRING='p=notes.git;a=blob_plain;f=work/G%C3%83%C2%BCtekriterien.txt;hb=HEAD'
>>> ./gitweb.cgi
>>>
>>>work/Gütekriterien.txt
>>>Content-disposition: inline; filename="work/Gütekriterien.txt"
>>
>> You should fix the code path that created that URI, though, as it is not
>> what you expected.
>>
>> %C3%83 decodes to U+00C3 Latin Capital Letter A With Tilde
>> %C2%BC decodes to U+00BC Vulgar Graction One Quarter
>>
>> The proper UTF-8 encoding for ü (U+00FC) is, as you can probably guess from
>> looking at which two characters the sequence above yielded, C3 BC, which in
>> a URI is represented as %C3%BC.
>>
>> Your QUERY_STRING should thus be
>>
>> p=notes.git;a=blob_plain;f=work/G%C3%BCtekriterien.txt;hb=HEAD
>>
>> which probably works as expected.
>>
>> What is happening is that whatever is generating the URI us UTF-8-encoding
>> the string twice (i.e., it generates a string with the proper C3 BC in it,
>> and then interprets it as iso-8859-1 data and runs that through a UTF-8
>> encoder again, yielding the C3 83 C2 BC sequence you see above).
>
> The subroutine "git tree" generates the tree view. It stores the output
> of "git ls-tree -z ..." in an array named "@entries". Printing the content
> of this array yields the following result:
>
> 00644 blob 6419cd06a9461c38d4f94d9705d97eaaa887156a 520 Gütekriterien.txt
>
> This leads to the "doubled" encoding. Declaring the encoding in the call
> to open yields the following result:
>
> 100644 blob 6419cd06a9461c38d4f94d9705d97eaaa887156a 520 Gütekriterien.txt
Good catch.
Writing test for this would not be easy, and require some HTML
parser (WWW::Mechanize, Web::Scraper, HTML::Query, pQuery,
... or low level HTML::TreeBuilder, or other low level parser).
> ---
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index a9f57d6..f1414e1 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -7138,7 +7138,7 @@ sub git_tree {
> my @entries = ();
> {
> local $/ = "\0";
> - open my $fd, "-|", git_cmd(), "ls-tree", '-z',
> + open my $fd, "-|encoding(UTF-8)", git_cmd(), "ls-tree", '-z',
> ($show_sizes ? '-l' : ()), @extra_options, $hash
> or die_error(500, "Open git-ls-tree failed");
Or put
binmode $fd, ':utf8';
like in the rest of the code.
> @entries = map { chomp; $_ } <$fd>;
>
Even better solution would be to use
use open IN => ':encoding(utf-8)';
at the beginning of gitweb.perl, once and for all.
Unfortunately the output equivalent requires creating Perl
module for gitweb, to be able to use
use open OUT => ':encoding(utf-8-with-fallback)';
--
Jakub Narebski
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 08/44] update-ref.c: log transaction error from the update_ref
Ronnie Sahlberg wrote: > Call ref_transaction_commit with QUIET_ON_ERR and use the strbuf that is > returned to print a log message if/after the transaction fails. > > Signed-off-by: Ronnie Sahlberg All error paths in _commit add to the error string now, so the only effect should be to add the missing error message when commiting packed-refs fails (thanks for fixing that) and to change some one-line errors to two-line. Reviewed-by: Jonathan Nieder -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] grep -I: do not bother to read known-binary files
On Thu, May 15, 2014 at 07:42:00PM +0200, Johannes Schindelin wrote: > > Hrm. Is this patch still necessary? In the time since this patch was > > written, we did 0826579 (grep: load file data after checking > > binary-ness, 2012-02-02) > > I have no time to test this but I trust that you made sure that it works > as advertised. In my case, there were about 500 gigabytes of image data > intermixed with code, and waiting for 'git grep' was not funny at all (and > I did not have time back then to go through a full code submission cycle > on the Git mailing list, either). > > So I guess we can drop my patch. Certainly I tested it at the time (those commits I referenced contain timing information), and it should have improved the workload you describe. I did not test before/after the patch in this thread, but only read it and noticed that it was trying to do the same thing (that is why I said "I suspect..."). As the person who is proposing the patch for git.git, I would hope Stepan would follow up on such review and confirm whether or not it is still needed. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git grep -O -i: if the pager is 'less', pass the '-i' option
On Thu, May 15, 2014 at 07:46:49PM +0200, Johannes Schindelin wrote: > > We've already found the lines of interest to the user. It would be nice > > if we could somehow point the pager at them by number, rather than > > repeating the (slightly incompatible) search. > > FWIW it is exactly that type of "I want your patch to do more than you do" > type of comments that makes it impossible for myself to contribute patches > anymore. It just does not fit inside my Git time budget anymore. I'm sorry you took it that way. What I meant to say was "it would be nice if we could do it this other way, which is more robust, but I don't think it is easy. Let's take the patch". I.e., when I later said: >>> But as is, it's an improvement, so (except that "-i" should be >>> replaced by "-I") it seems like a good change. >> >>Agreed. Thanks for the list of problematic options. the "agreed" there is with "it seems like a good change". And I also wanted to point people to existing work, if they did want to explore doing it that other way. I really didn't expect anything from you (beyond s/i/I/, as Jonathan suggested). > Besides, it breaks exactly the intended usage. My intent is not just to > see the matching lines in the pager, but to be able to adjust the search > pattern further if needed. Your suggestion completely breaks that usage. Thanks, this is a point I hadn't considered. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: regression: request-pull with signed tag lacks tags/ in master
Junio C Hamano writes: > "Michael S. Tsirkin" writes: > >> looks like pull requests with signed git got broken in git master: >> >> [mst@robin qemu]$ /usr/bin/git --version >> git version 1.8.3.1 >> [mst@robin qemu]$ git --version >> git version 2.0.0.rc1.18.gac53fc6.dirty >> [mst@robin qemu]$ >> [mst@robin qemu]$ /usr/bin/git request-pull origin/master >> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git for_upstream |grep >> git.kernel.org >> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream >> >> >> [mst@robin qemu]$ git request-pull origin/master >> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git for_upstream |grep >> git.kernel.org >> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git for_upstream >> >> this for_upstream syntax is a problem because it does not work >> for older git clients who might get this request. >> >> Didn't bisect yet - anyone knows what broke this? > > Linus ;-) The series that ends with ec44507, I think. My reading of the earlier parts of the series is that Linus wanted us never dwim "for-upstream" to "tags/for-upstream" or any other ref that happens to point at the same commit as for-upstream you have. The changes done for that purpose covered various cases a bit too widely, and "request-pull ... tags/for_upstream" were incorrectly stripped to a request to pull "for_upstream", which was fixed by 5aae66bd (request-pull: resurrect "pretty refname" feature, 2014-02-25). But that fix does not resurrect the dwimming forbid by the earlier parts of the series to turn "for_upstream" into "tags/for_upstream". What would you get if you do this? $ git request-pull origin/master \ git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git \ tags/for_upstream | grep git.kernel.org -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] open_sha1_file: report "most interesting" errno
On Thu, May 15, 2014 at 10:02:06AM -0700, Junio C Hamano wrote: > > $ chmod 0 .git/objects/??/* > > $ git rev-list --all > > fatal: loose object b2d6fab18b92d49eac46dc3c5a0bcafabda20131 (stored in > > .git/objects/b2/d6fab18b92d49eac46dc3c5a0bcafabda20131) is corrupt > > H. So we keep track of a more interesting errno we get from > some other place than what we get for this local loose object, and > we no longer give this message pointing at the local loose > object---is that the idea? Yes, though my main goal was to stop saying "corrupt" when that is not the problem at all. Not pointing to the wrong object was a secondary consideration. :) I would also be happy to just show the error for the local object, even if it is exists somewhere else. The main thing I am changing here is that we currently _never_ show the errno from the main odb. We either show the errno from the last alternate we looked at, or we show ENOENT (because we explicitly set ENOENT right before looking at the alternates). I think it's a separate problem that the "stored in..." is sometimes wrong. That comes when we get ENOENT, and we check has_loose_object(). IOW, we guess "we couldn't find it, but we claim to have it, so it must have been corrupt". But that does not say _where_ we found it, and our call to sha1_file_name is a guess that may be wrong. I'm actually not sure if we can even trigger that code path now. It depended on returning ENOENT from read_object, which we used to frequently do erroneously. Now we will only do it when the object truly does not exist, which means has_loose_object should generally not return true. I'm also a bit surprised that errno actually survives here. That clearly was the intent, so I don't think my patch is making anything worse. But it's possible that we would prepare_packed_git or open/mmap packfiles between the call to open_sha1_file and when read_sha1_file_extended looks at errno. > What I am wondering is that this report we give in the new code > > > $ git rev-list --all > > fatal: failed to read object b2d6fab18b92d49eac46dc3c5a0bcafabda20131: > > Permission denied > > may want to say which of the various possible places we saw this > most interesting errno, which I think was the original motivation > came from e8b15e61 (sha1_file: Show the the type and path to corrupt > objects, 2010-06-10) that added "(stored in ...)". > > But that may involve a larger surgery, and I definitely do not want > to add unnecessary logic in the common-case codepath to keep track > of pieces of information that are only used in the error codepath, > so it smells like that this is the best fix to the issue the commit > message describes. Yes, I think doing this right would involve a lot more surgery, and I don't know if it is worth the effort. But in addition to the problems above, I note that we simply open the first object we can find, and do not loop if mmap or checksums fail. So unlike packed objects, which are resilient to corruption, we would fail immediately. So I think the right thing to do would be: 1. Don't loop across alternates in open_sha1_file. Loop in read_object (which means looping in _other_ calls to map_sha1_file, like in sha1_object_info). 2. Fail quickly, since the common case is that we will find the object elsewhere. But when we do have an error, take time to go back and actually find the location of the object and the real error (i.e., have a diagnose_object or something). Neither is a particularly high priority to me, though, so I don't plan on working on them anytime soon. The only reason I went this far was that I saw the "loose object is corrupt" / EPERM confusion in the real world. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [git-users] worlds slowest git repo- what to do?
From: "John Fisher" I assert based on one piece of evidence ( a post from a facebook dev) that I now have the worlds biggest and slowest git repository, and I am not a happy guy. I used to have the worlds biggest CVS repository, but CVS can't handle multi-G sized files. So I moved the repo to git, because we are using that for our new projects. goal: keep 150 G of files (mostly binary) from tiny sized to over 8G in a version-control system. problem: git is absurdly slow, think hours, on fast hardware. question: any suggestions beyond these- http://git-annex.branchable.com/ https://github.com/jedbrown/git-fat https://github.com/schacon/git-media http://code.google.com/p/boar/ subversion ? At the moment some of the developers are looking to speed up some of the code on very large repos, though I think they are looking at code repos, rather than large file repos. They were looking for large repos to test some of the code upon ;-) I've copied the Git list should they want to make any suggestions. Thanks. -- Philip -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 06/44] refs.c: add an err argument ro delete_loose_ref
Ronnie Sahlberg wrote:
> [Subject: refs.c: add an err argument ro delete_loose_ref]
s/ro/to/
s/delete_loose_ref/delete_ref_loose/
> --- a/refs.c
> +++ b/refs.c
> @@ -2484,17 +2484,22 @@ static int repack_without_ref(const char *refname)
> return repack_without_refs(&refname, 1, NULL);
> }
>
> -static int delete_ref_loose(struct ref_lock *lock, int flag)
> +static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf
> *err)
Should this get an onerr flag to suppress the message to stderr
or unconditionally suppress it when err != NULL?
[...]
> lock->lk->filename[i] = 0;
> - err = unlink_or_warn(lock->lk->filename);
> + res = unlink_or_warn(lock->lk->filename);
It seems like in the new error handling scheme there should be a new
variant on wrapper.c's warn_if_unremovable:
static int add_err_if_unremovable(const char *op, const char *file,
struct strbuf *err, int rc)
{
int err = errno;
if (rc < 0 && err != ENOENT) {
strbuf_addf(err, "unable to %s %s: %s",
op, file, strerror(errno));
errno = err;
}
return rc;
}
static int unlink_or_err(const char *file, struct strbuf *err)
{
return add_err_if_unremovable("unlink", file, err,
unlink(file));
}
res = unlink_or_err(lock->lk->filename, err);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git grep -O -i: if the pager is 'less', pass the '-i' option
Hi, On Thu, 15 May 2014, Jonathan Nieder wrote: > Johannes Schindelin wrote: > > On Wed, 14 May 2014, Junio C Hamano wrote: > > >> Spot on. The change, especially with "-I", makes sense. > > > > Except that it was not tested with -I. If you change it that way and it > > stops working on Windows, it's useless to me. > > Are you saying that less on Windows doesn't have an "-I" option? I did not say that. > version.c tells me it was added in v266 (1994-12-26). Thanks. That was the thing I was really looking for: an indication that -I is not a new-fangled thing but a well-tested option that even old greps have. Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 00/44] Use ref transactions for all ref updates
Jonathan Nieder writes:
> Ronnie Sahlberg wrote:
>
>> This patch series is based on next and expands on the transaction API.
>
> Thanks. Will pick up in v8 where I left off with v6.
>
> Applies with just one minor conflict on top of a merge of
> mh/ref-transaction, rs/ref-update-check-errors-early, and
> rs/reflog-exists. Here's an interdiff against version 6 for those
> following along.
The interdiffs look mostly sensible. Thanks.
>> Version 8:
>> - Updates after review by JN
>> - Improve commit messages
>> - Add a patch that adds an err argument to repack_without_refs
>> - Add a patch that adds an err argument to delete_loose_ref
>> - Better document that a _update/_delete/_create failure means the whole
>>transaction has failed and needs rollback.
>>
>> Version 7:
>> - Updated commit messages per JNs review comments.
>> - Changed REF_ISPRUNING and REF_ISPACKONLY to be private flags and not
>>exposed through refs.h
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 0f4e1fc..07ccc97 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1684,7 +1684,7 @@ int cmd_commit(int argc, const char **argv, const char
> *prefix)
> 0, !!current_head, sb.buf) ||
> ref_transaction_commit(transaction, &err)) {
> rollback_index_files();
> - die(_("HEAD: cannot update ref: %s"), err.buf);
> + die("%s", err.buf);
> }
> ref_transaction_free(transaction);
>
> diff --git a/builtin/replace.c b/builtin/replace.c
> index 47c360c..952b589 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -163,7 +163,7 @@ static int replace_object(const char *object_ref, const
> char *replace_ref,
> ref_transaction_update(transaction, ref, repl, prev,
> 0, !is_null_sha1(prev), NULL) ||
> ref_transaction_commit(transaction, &err))
> - die(_("%s: failed to replace ref: %s"), ref, err.buf);
> + die("%s: failed to replace ref: %s", ref, err.buf);
>
> ref_transaction_free(transaction);
> return 0;
> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index c5aff92..bd7e96f 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -228,7 +228,7 @@ static const char *parse_cmd_create(struct strbuf *input,
> const char *next)
>
> if (ref_transaction_create(transaction, refname, new_sha1,
> update_flags, msg))
> - die("failed transaction create for %s", refname);
> + die("cannot create ref '%s'", refname);
>
> update_flags = 0;
> free(refname);
> diff --git a/refs.c b/refs.c
> index 302a2b3..ed93b75 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -29,6 +29,15 @@ static inline int bad_ref_char(int ch)
> return 0;
> }
>
> +/** Used as a flag to ref_transaction_delete when a loose ref is beeing
> + * pruned.
> + */
> +#define REF_ISPRUNING0x0100
> +/** Deletion of a ref that only exists as a packed ref in which case we do
> not
> + * need to lock the loose ref during the transaction.
> + */
> +#define REF_ISPACKONLY 0x0200
> +
> /*
> * Try to read one refname component from the front of refname. Return
> * the length of the component found, or -1 if the component is not
> @@ -2447,12 +2456,12 @@ static int curate_packed_ref_fn(struct ref_entry
> *entry, void *cb_data)
> return 0;
> }
>
> -static int repack_without_refs(const char **refnames, int n)
> +static int repack_without_refs(const char **refnames, int n, struct strbuf
> *err)
> {
> struct ref_dir *packed;
> struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
> struct string_list_item *ref_to_delete;
> - int i, removed = 0;
> + int i, ret, removed = 0;
>
> /* Look for a packed ref */
> for (i = 0; i < n; i++)
> @@ -2465,6 +2474,9 @@ static int repack_without_refs(const char **refnames,
> int n)
>
> if (lock_packed_refs(0)) {
> unable_to_lock_error(git_path("packed-refs"), errno);
> + if (err)
> + strbuf_addf(err, "cannot delete '%s' from packed refs",
> + refnames[i]);
> return error("cannot delete '%s' from packed refs",
> refnames[i]);
> }
> packed = get_packed_refs(&ref_cache);
> @@ -2490,20 +2502,28 @@ static int repack_without_refs(const char **refnames,
> int n)
> }
>
> /* Write what remains */
> - return commit_packed_refs();
> + ret = commit_packed_refs();
> + if (ret && err)
> + strbuf_addf(err, "unable to overwrite old ref-pack file");
> + return ret;
> }
>
> -static int delete_ref_loose(struct ref_lock *lock, int flag)
> +static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf
> *err)
> {
> if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
> /* loose */
> - int e
Re: regression: request-pull with signed tag lacks tags/ in master
"Michael S. Tsirkin" writes: > looks like pull requests with signed git got broken in git master: > > [mst@robin qemu]$ /usr/bin/git --version > git version 1.8.3.1 > [mst@robin qemu]$ git --version > git version 2.0.0.rc1.18.gac53fc6.dirty > [mst@robin qemu]$ > [mst@robin qemu]$ /usr/bin/git request-pull origin/master > git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git for_upstream |grep > git.kernel.org > git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream > > > [mst@robin qemu]$ git request-pull origin/master > git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git for_upstream |grep > git.kernel.org > git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git for_upstream > > this for_upstream syntax is a problem because it does not work > for older git clients who might get this request. > > Didn't bisect yet - anyone knows what broke this? Linus ;-) The series that ends with ec44507, I think. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] Gitweb: Convert UTF-8 encoded file names
On Thu, May 15, 2014 at 10:04:24AM +0100, Peter Krefting wrote:
> Michael Wagner:
>
> >Decoding the UTF-8 encoded file name (again with an additional print
> >statement):
> >
> >$ REQUEST_METHOD=GET
> >QUERY_STRING='p=notes.git;a=blob_plain;f=work/G%C3%83%C2%BCtekriterien.txt;hb=HEAD'
> > ./gitweb.cgi
> >
> >work/Gütekriterien.txt
> >Content-disposition: inline; filename="work/Gütekriterien.txt"
>
> You should fix the code path that created that URI, though, as it is not
> what you expected.
>
> %C3%83 decodes to U+00C3 Latin Capital Letter A With Tilde
> %C2%BC decodes to U+00BC Vulgar Graction One Quarter
>
> The proper UTF-8 encoding for ü (U+00FC) is, as you can probably guess from
> looking at which two characters the sequence above yielded, C3 BC, which in
> a URI is represented as %C3%BC.
>
> Your QUERY_STRING should thus be
>
> p=notes.git;a=blob_plain;f=work/G%C3%BCtekriterien.txt;hb=HEAD
>
> which probably works as expected.
Obviously, you are right, thanks.
>
> What is happening is that whatever is generating the URI us UTF-8-encoding
> the string twice (i.e., it generates a string with the proper C3 BC in it,
> and then interprets it as iso-8859-1 data and runs that through a UTF-8
> encoder again, yielding the C3 83 C2 BC sequence you see above).
>
The subroutine "git tree" generates the tree view. It stores the output
of "git ls-tree -z ..." in an array named "@entries". Printing the content
of this array yields the following result:
00644 blob 6419cd06a9461c38d4f94d9705d97eaaa887156a 520 Gütekriterien.txt
This leads to the "doubled" encoding. Declaring the encoding in the call
to open yields the following result:
100644 blob 6419cd06a9461c38d4f94d9705d97eaaa887156a 520 Gütekriterien.txt
---
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a9f57d6..f1414e1 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -7138,7 +7138,7 @@ sub git_tree {
my @entries = ();
{
local $/ = "\0";
- open my $fd, "-|", git_cmd(), "ls-tree", '-z',
+ open my $fd, "-|encoding(UTF-8)", git_cmd(), "ls-tree", '-z',
($show_sizes ? '-l' : ()), @extra_options, $hash
or die_error(500, "Open git-ls-tree failed");
@entries = map { chomp; $_ } <$fd>;
> --
> \\// Peter - http://www.softwolves.pp.se/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 04/44] refs.c: add an err argument to repack_without_refs
Ronnie Sahlberg wrote:
> --- a/refs.c
> +++ b/refs.c
> @@ -2427,12 +2427,12 @@ static int curate_packed_ref_fn(struct ref_entry
> *entry, void *cb_data)
> return 0;
> }
>
> -static int repack_without_refs(const char **refnames, int n)
> +static int repack_without_refs(const char **refnames, int n, struct strbuf
> *err)
Should this also get an onerr flag to suppress the message to stderr,
or unconditionally suppress the message to stderr when err != NULL?
[...]
> @@ -2445,6 +2445,9 @@ static int repack_without_refs(const char **refnames,
> int n)
>
> if (lock_packed_refs(0)) {
> unable_to_lock_error(git_path("packed-refs"), errno);
> + if (err)
> + strbuf_addf(err, "cannot delete '%s' from packed refs",
> + refnames[i]);
unable_to_lock_error is able to come up with a message with more
detail (path so the sysadmin can hunt down the problem even if this
was run e.g. from a cronjob where the path is not obvious, errno
hinting at the nature of the problem).
[...]
> @@ -2470,12 +2473,15 @@ static int repack_without_refs(const char **refnames,
> int n)
> }
>
> /* Write what remains */
> - return commit_packed_refs();
> + ret = commit_packed_refs();
> + if (ret && err)
> + strbuf_addf(err, "unable to overwrite old ref-pack file");
After commit_lock_file sets errno, amazingly no one clobbers it
until we get to this point. The only calls in between are to
free().
It would be nice to make that more explicit in commit_packed_refs:
int save_errno;
...
if (commit_lock_file(packed_ref_cache->lock)) {
save_errno = errno;
error = -1;
}
packed_ref_cache->lock = NULL;
release_packed_ref_cache(packed_ref_cache);
errno = save_errno;
return error;
Even without that, this message could include strerror(errno).
> + return ret;
> }
Thanks,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH] transport-helper: add trailing --
Hi, On Thu, 15 May 2014, Stepan Kasal wrote: > From: Sverre Rabbelier > Date: Sat, 28 Aug 2010 20:49:01 -0500 > > [PT: ensure we add an additional element to the argv array] IIRC we had problems with refs vs file names. Ciao, Johannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 02/44] refs.c: allow passing NULL to ref_transaction_free
Thanks.
I used your improved text in the commit message.
On Thu, May 15, 2014 at 11:15 AM, Jonathan Nieder wrote:
> Ronnie Sahlberg wrote:
>
>> Allow ref_transaction_free to be called with NULL and as a result allow
>> ref_transaction_rollback to be called for a NULL transaction.
>>
>> This allows us to write code that will
>> if ( (!transaction ||
>> ref_transaction_update(...)) ||
>> (ref_transaction_commit(...) && !(transaction = NULL)) {
>> ref_transaction_rollback(transaction);
>> ...
>> }
>>
>> In this case transaction is reset to NULL IFF ref_transaction_commit() was
>> invoked and thus the rollback becomes ref_transaction_rollback(NULL) which
>> is safe. IF the conditional triggered prior to ref_transaction_commit()
>> then transaction is untouched and then ref_transaction_rollback(transaction)
>> will rollback the failed transaction.
>
> I still think these last two paragraphs confuse more than enlighten
> here. There's plenty of time to explain them in the patch that uses
> that code.
>
> I'd just say something like
>
> Allow ref_transaction_free(NULL) and hence
> ref_transaction_rollback(NULL)
> as no-ops.
>
> This makes ref_transaction_rollback easier to use and more similar to
> plain 'free'.
>
> And maybe:
>
> In particular, it lets us rollback unconditionally as part of cleanup
> code after setting 'transaction = NULL' if a transaction has been
> committed or rolled back already.
>
> Thanks,
> Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 02/44] refs.c: allow passing NULL to ref_transaction_free
Ronnie Sahlberg wrote:
> Allow ref_transaction_free to be called with NULL and as a result allow
> ref_transaction_rollback to be called for a NULL transaction.
>
> This allows us to write code that will
> if ( (!transaction ||
> ref_transaction_update(...)) ||
> (ref_transaction_commit(...) && !(transaction = NULL)) {
> ref_transaction_rollback(transaction);
> ...
> }
>
> In this case transaction is reset to NULL IFF ref_transaction_commit() was
> invoked and thus the rollback becomes ref_transaction_rollback(NULL) which
> is safe. IF the conditional triggered prior to ref_transaction_commit()
> then transaction is untouched and then ref_transaction_rollback(transaction)
> will rollback the failed transaction.
I still think these last two paragraphs confuse more than enlighten
here. There's plenty of time to explain them in the patch that uses
that code.
I'd just say something like
Allow ref_transaction_free(NULL) and hence
ref_transaction_rollback(NULL)
as no-ops.
This makes ref_transaction_rollback easier to use and more similar to
plain 'free'.
And maybe:
In particular, it lets us rollback unconditionally as part of cleanup
code after setting 'transaction = NULL' if a transaction has been
committed or rolled back already.
Thanks,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 01/44] refs.c: constify the sha arguments for ref_transaction_create|delete|update
Ronnie Sahlberg wrote: > ref_transaction_create|delete|update has no need to modify the sha1 > arguments passed to it so it should use const unsigned char* instead > of unsigned char*. Obviously good. Reviewed-by: Jonathan Nieder -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 00/44] Use ref transactions for all ref updates
Ronnie Sahlberg wrote:
> This patch series is based on next and expands on the transaction API.
Thanks. Will pick up in v8 where I left off with v6.
Applies with just one minor conflict on top of a merge of
mh/ref-transaction, rs/ref-update-check-errors-early, and
rs/reflog-exists. Here's an interdiff against version 6 for those
following along.
> Version 8:
> - Updates after review by JN
> - Improve commit messages
> - Add a patch that adds an err argument to repack_without_refs
> - Add a patch that adds an err argument to delete_loose_ref
> - Better document that a _update/_delete/_create failure means the whole
>transaction has failed and needs rollback.
>
> Version 7:
> - Updated commit messages per JNs review comments.
> - Changed REF_ISPRUNING and REF_ISPACKONLY to be private flags and not
>exposed through refs.h
diff --git a/builtin/commit.c b/builtin/commit.c
index 0f4e1fc..07ccc97 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1684,7 +1684,7 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
0, !!current_head, sb.buf) ||
ref_transaction_commit(transaction, &err)) {
rollback_index_files();
- die(_("HEAD: cannot update ref: %s"), err.buf);
+ die("%s", err.buf);
}
ref_transaction_free(transaction);
diff --git a/builtin/replace.c b/builtin/replace.c
index 47c360c..952b589 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -163,7 +163,7 @@ static int replace_object(const char *object_ref, const
char *replace_ref,
ref_transaction_update(transaction, ref, repl, prev,
0, !is_null_sha1(prev), NULL) ||
ref_transaction_commit(transaction, &err))
- die(_("%s: failed to replace ref: %s"), ref, err.buf);
+ die("%s: failed to replace ref: %s", ref, err.buf);
ref_transaction_free(transaction);
return 0;
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index c5aff92..bd7e96f 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -228,7 +228,7 @@ static const char *parse_cmd_create(struct strbuf *input,
const char *next)
if (ref_transaction_create(transaction, refname, new_sha1,
update_flags, msg))
- die("failed transaction create for %s", refname);
+ die("cannot create ref '%s'", refname);
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index 302a2b3..ed93b75 100644
--- a/refs.c
+++ b/refs.c
@@ -29,6 +29,15 @@ static inline int bad_ref_char(int ch)
return 0;
}
+/** Used as a flag to ref_transaction_delete when a loose ref is beeing
+ * pruned.
+ */
+#define REF_ISPRUNING 0x0100
+/** Deletion of a ref that only exists as a packed ref in which case we do not
+ * need to lock the loose ref during the transaction.
+ */
+#define REF_ISPACKONLY 0x0200
+
/*
* Try to read one refname component from the front of refname. Return
* the length of the component found, or -1 if the component is not
@@ -2447,12 +2456,12 @@ static int curate_packed_ref_fn(struct ref_entry
*entry, void *cb_data)
return 0;
}
-static int repack_without_refs(const char **refnames, int n)
+static int repack_without_refs(const char **refnames, int n, struct strbuf
*err)
{
struct ref_dir *packed;
struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
struct string_list_item *ref_to_delete;
- int i, removed = 0;
+ int i, ret, removed = 0;
/* Look for a packed ref */
for (i = 0; i < n; i++)
@@ -2465,6 +2474,9 @@ static int repack_without_refs(const char **refnames, int
n)
if (lock_packed_refs(0)) {
unable_to_lock_error(git_path("packed-refs"), errno);
+ if (err)
+ strbuf_addf(err, "cannot delete '%s' from packed refs",
+ refnames[i]);
return error("cannot delete '%s' from packed refs",
refnames[i]);
}
packed = get_packed_refs(&ref_cache);
@@ -2490,20 +2502,28 @@ static int repack_without_refs(const char **refnames,
int n)
}
/* Write what remains */
- return commit_packed_refs();
+ ret = commit_packed_refs();
+ if (ret && err)
+ strbuf_addf(err, "unable to overwrite old ref-pack file");
+ return ret;
}
-static int delete_ref_loose(struct ref_lock *lock, int flag)
+static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf
*err)
{
if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
/* loose */
- int err, i = strlen(lock->lk->filename) - 5; /* .lock */
+ int res, i = strlen(lock->lk->filename) - 5; /* .lock */
lock->lk->filename[i] = 0;
- err = unlink_or_warn(lock->lk->filename);
+
Re: [PATCH] git grep -O -i: if the pager is 'less', pass the '-i' option
Johannes Schindelin wrote: > On Wed, 14 May 2014, Jeff King wrote: >> We've already found the lines of interest to the user. It would be nice >> if we could somehow point the pager at them by number, rather than >> repeating the (slightly incompatible) search. > > FWIW it is exactly that type of "I want your patch to do more than you do" > type of comments that makes it impossible for myself to contribute patches > anymore. I think you're overreacting to Peff's "It would be nice". It is a way of talking about where this lies in a design space that also includes the git-jump tool that Peff worked on. Maybe the tools can learn from each other. It is not a reason not to apply the patch. Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
