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 jrnie...@gmail.com -- 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: [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 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 jrnie...@gmail.com 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 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 sahlb...@google.com
---
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
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: [PATCH 1/2] config: be strict on core.commentChar
Nguyễn Thái Ngọc Duy wrote:
--- a/config.c
+++ b/config.c
@@ -826,8 +826,12 @@ static int git_default_core_config(const char *var,
const char *value)
if (!strcmp(var, core.commentchar)) {
const char *comment;
int ret = git_config_string(comment, var, value);
- if (!ret)
- comment_line_char = comment[0];
+ if (!ret) {
+ if (comment[0] !comment[1])
+ comment_line_char = comment[0];
+ else
+ return error(core.commentChar should only be
one character);
+ }
Perhaps, to decrease indentation a little:
if (ret)
return ret;
if (comment[0] !comment[1])
comment_line_char = comment[0];
else
return error(...);
return 0;
[...]
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1348,12 +1348,6 @@ test_expect_success status (core.commentchar with
submodule summary) '
test_i18ncmp expect output
'
-test_expect_success status (core.commentchar with two chars with submodule
summary) '
- test_config core.commentchar ;;
- git -c status.displayCommentPrefix=true status output
- test_i18ncmp expect output
Could keep the test to avoid regressions:
test_config core.commentchar ;;
test_must_fail git -c status.displayCommentPrefix=true status
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 2/2] commit: allow core.commentChar=auto for character auto selection
Nguyễn Thái Ngọc Duy wrote:
core.commentChar starts with '#' as in default but if it's already in
the prepared message, find another one among a small subset. This
should stop surprises because git strips some lines unexpectedly.
Probably worth mentioning this only kicks in if someone explicitly
configures [core] commentchar = auto.
Would it be a goal to make 'auto' the default eventually if people
turn out to like it?
[...]
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -594,6 +594,40 @@ static char *cut_ident_timestamp_part(char *string)
return ket;
}
+static void adjust_comment_line_char(const struct strbuf *sb)
+{
+ char candidates[] = @!#$%^|:;~;
This prefers '@' over '#'. Intended?
[...]
+ char *candidate;
+ const char *p;
+
+ if (!sb-len)
+ return;
+
+ if (!strchr(candidates, comment_line_char))
+ candidates[0] = comment_line_char;
Could do
if (!memchr(sb-buf, comment_line_char, sb-len))
return;
to solve the precedence problem. The comment_line_char not appearing
in the message is the usual case and handling it separately means it
gets handled faster.
[...]
--- a/config.c
+++ b/config.c
@@ -829,6 +829,8 @@ static int git_default_core_config(const char *var, const
char *value)
if (!ret) {
if (comment[0] !comment[1])
comment_line_char = comment[0];
+ else if (!strcasecmp(comment, auto))
+ auto_comment_line_char = 1;
Is there a way to disable 'auto' if 'auto' is already set?
comment_line_char still can be set and matters when 'auto' is set.
Should they be separate settings?
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -563,4 +563,29 @@ test_expect_success 'commit --status with custom comment
character' '
[...]
+ GIT_EDITOR=.git/FAKE_EDITOR test_must_fail \
Shells make it obnoxiously hard to set a one-shot envvar while
calling a function without the setting leaking into later commands.
(
test_set_editor .git/FAKE_EDITOR
test_must_fail ...
)
or
test_must_fail env GIT_EDITOR=.git/FAKE_EDITOR ...
should do the trick.
Thanks and hope that helps,
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 22/44] fetch.c: clear errno before calling functions that might set it
(cc-ing peff, who may remember this STORE_REF_ERROR_DF_CONFLICT case
from long ago)
Ronnie Sahlberg wrote:
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
That's a bug. Any function for which errno is supposed to be
meaningful when it returns an error should always set errno.
Otherwise errno may be set to ENOTDIR within the function by an
unrelated system call.
Functions that fail and lead callers to check errno, based on a quick
search with 'git grep -e 'errno *[!=]=':
fopen
lstat
builtin/apply.c::try_create_file (= mkdir, symlink, or open)
rmdir
open
mkdir
unlink
lock_any_ref_for_update
write_ref_sha1
strtol
kill
odb_pack_keep
opendir
fgets
read
poll
pread
strtoimax
strtoumax
git_parse_int
git_parse_int64
git_parse_ulong
write_in_full
credential-cache.c::send_request
fstat
run_command_v_opt
git.c::run_argv
readlink
resolve_ref_unsafe
hold_lock_file_for_update
unlink_or_warn
rename
execvp
waitpid
execv_git_cmd
execv_shell_cmd
sane_execvp
stat
read_object
git_mkstemp_mode
create_tmpfile
start_command
xwrite
iconv
fast_export_ls
fast_export_ls_rev
delete_ref
lock_any_ref_for_update has failure paths
* check_ref_format [!]
* lock_ref_sha1_basic
lock_ref_sha1_basic has failure paths
* remove_empty_directories
* resolve_ref_unsafe
* safe_create_leading_directories
* verify_lock
remove_empty_directories has one failure path
* remove_dir_recursively
but could be more explicit about the need to preserve errno.
errno from remove_dir_recursively is meaningful.
resolve_ref_unsafe has failure paths
* check_refname_format [!]
* too much recursion [!]
* lstat, readlink, open
* handle_missing_loose_ref
* read_in_full, but errno gets clobbered
* invalid ref [!]
* invalid symref [!]
verify_lock has failure paths
* read_ref_full, but errno gets clobbered
* old_sha1 didn't match [!]
write_ref_sha1 has failure paths
* !lock [!]
* missing object [!]
* non-commit object [!]
* write_in_full, close_ref, but errno gets clobbered
* log_ref_write
* commit_ref
log_ref_write has failure paths
* log_ref_setup
* write_in_full, close. errno gets clobbered
commit_ref just calls commit_lock_file.
log_ref_setup has failure paths
* safe_create_leading_directories, but errno gets clobbered
* remove_empty_directories, but errno gets clobbered
* open, but errno gets clobbered
safe_create_leading_directories doesn't set errno for SCLD_EXISTS
but otherwise its errno is useful.
That should cover the cases affecting the code path in fetch.c you
mentioned (I haven't checked the others).
How about something like this?
It's also tempting to teach vreportf and vwritef to save errno, which
would handle some of these cases automatically.
diff --git i/refs.c w/refs.c
index 82a8d4e..f98acf0 100644
--- i/refs.c
+++ w/refs.c
@@ -1333,8 +1333,10 @@ const char *resolve_ref_unsafe(const char *refname,
unsigned char *sha1, int rea
if (flag)
*flag = 0;
- if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
+ if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+ errno = EINVAL;
return NULL;
+ }
for (;;) {
char path[PATH_MAX];
@@ -1342,8 +1344,10 @@ const char *resolve_ref_unsafe(const char *refname,
unsigned char *sha1, int rea
char *buf;
int fd;
- if (--depth 0)
+ if (--depth 0) {
+ errno = ELOOP;
return NULL;
+ }
git_snpath(path, sizeof(path), %s, refname);
@@ -1405,9 +1409,13 @@ const char *resolve_ref_unsafe(const char *refname,
unsigned char *sha1, int rea
return NULL;
}
len = read_in_full(fd, buffer, sizeof(buffer)-1);
- close(fd);
- if (len 0)
+ if (len 0) {
+ int save_errno = errno;
+ close(fd);
+ errno = save_errno;
return NULL;
+ }
+ close(fd);
while (len isspace(buffer[len-1]))
len--;
buffer[len] = '\0';
@@ -1424,6 +1432,7 @@ const char *resolve_ref_unsafe(const char *refname,
unsigned char *sha1, int rea
(buffer[40] != '\0' !isspace(buffer[40]))) {
if (flag)
*flag |= REF_ISBROKEN;
+ errno = EINVAL;
return NULL;
}
return refname;
@@ -1436,6 +1445,7 @@ const char *resolve_ref_unsafe(const char
Re: [PATCH v10 00/44] Use ref transactions for all ref updates
Ronnie Sahlberg wrote: This patch series can also be found at https://github.com/rsahlberg/git/tree/ref-transactions I think the rerolls are coming too fast. I've been using your repository on github to check if I should not send any particular comment because you've already fixed it, so the extra mail is not needed for that purpose, at least. In general, I think it's best to stick with one version of a series, gather review comments on it, comment inline to solicit feedback about approaches to particular problems, point reviewers to an up-to-date branch which is more in flux, etc, and then only resend when reviewers have had a chance to get through it to make it easier to keep track of the review without getting lost. Thanks for the quick work so far, by the way. It's been fun watching the API evolve. My two cents, 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 23/44] fetch.c: change s_update_ref to use a ref transaction
(+cc: peff for STORE_REF_ERROR_DF_CONFLICT expertise)
Ronnie Sahlberg wrote:
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -375,7 +375,7 @@ static int s_update_ref(const char *action,
[...]
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_update(transaction, ref-name, ref-new_sha1,
+ref-old_sha1, 0, check_old) ||
+ ref_transaction_commit(transaction, msg, NULL)) {
+ ref_transaction_rollback(transaction);
return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
STORE_REF_ERROR_OTHER;
+ }
I'd rather not rely on errno here (see the previous patch for why).
Is there some other way to distinguish the case where a ref couldn't
be created because there was a prefix of that ref in the way?
For example, maybe ref_transaction_commit could return a different
negative integer in this case.
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 24/44] fetch.c: use a single ref transaction for all ref updates
Ronnie Sahlberg wrote:
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.
Fun.
[...]
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
[...]
@@ -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;
Previously fetch respected GIT_REFLOG_ACTION, and this is dropping
that support. Intended?
[...]
+ if (ref_transaction_update(transaction, ref-name, ref-new_sha1,
+ref-old_sha1, 0, check_old))
+ return STORE_REF_ERROR_OTHER;
Should pass a strbuf in to get a message back.
[...]
+
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;
error() appends a newline on its own, so the message shouldn't
end with newline.
More importantly, this message isn't helpful without more detail about
why _transaction_begin() failed. The user doesn't know what
error: cannot start ref transaction
means.
error: cannot connect to mysqld for ref storage: [etc]
would tell what they need to know. That is:
transaction = ref_transaction_begin(err);
if (!transaction) {
rc = error(%s, err.buf);
goto abort;
}
errno is not used here, so no need to set errno = 0.
[...]
@@ -676,6 +670,10 @@ static int store_updated_refs(const char *raw_url, const
char *remote_name,
}
}
}
+ if ((ret = ref_transaction_commit(transaction, NULL)))
+ rc |= ret == -2 ? STORE_REF_ERROR_DF_CONFLICT :
+ STORE_REF_ERROR_OTHER;
(I cheated and stole the newer code from your repo.)
Yay! Style nit: git avoids assignments in if conditionals.
ret = ref_tr...
if (ret == -2)
rc |= ...
else if (ret)
rc |= ...
Does _update need the too as futureproofing for backends that can
detect the name conflict sooner?
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 23/44] fetch.c: change s_update_ref to use a ref transaction
Ronnie Sahlberg wrote:
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
[...]
@@ -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) ||
+ ref_transaction_commit(transaction, msg, NULL)) {
Since 'err' is NULL, does that mean there's no message shown to the
user on error?
--
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] send-pack.c: Allow to disable side-band-64k
Hi, Thomas Braun wrote: pushing over the dump git protocol with a windows git client. I've never heard of the dump git protocol. Do you mean the git protocol that's used with git:// URLs? [...] Alternative approaches considered but deemed too invasive: - Rewrite read/write wrappers in mingw.c in order to distinguish between a file descriptor which has a socket behind and a file descriptor which has a file behind. I assume here too invasive means too much engineering effort? It sounds like a clean fix, not too invasive at all. But I can understand wanting a stopgap in the meantime. - Turning the capability side-band-64k off completely. This would remove a useful feature for users of non-affected transport protocols. Would it make sense to turn off sideband unconditionally on Windows when using the relevant protocols? I assume someone on the list wouldn't mind writing such a patch, so I don't think the engineering effort would be a problem for that. 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 00/19] convert test -a/-o to and || patch series
Hi, Elia Pinto wrote: These patch series convert test -a/-o to and ||. Should this come with a new check in t/check-non-portable-shell.pl so we don't regress in the future? Elia Pinto (19): [...] check_bindir|2 +- contrib/examples/git-clone.sh |2 +- contrib/examples/git-commit.sh |4 ++-- contrib/examples/git-merge.sh |4 ++-- contrib/examples/git-repack.sh |4 ++-- contrib/examples/git-resolve.sh |2 +- git-bisect.sh |2 +- git-mergetool.sh|4 ++-- git-rebase--interactive.sh |2 +- git-submodule.sh| 29 + t/t0025-crlf-auto.sh|6 +++--- t/t0026-eol-config.sh |8 t/t4102-apply-rename.sh |2 +- t/t5000-tar-tree.sh |2 +- t/t5403-post-checkout-hook.sh |8 t/t5537-fetch-shallow.sh|2 +- t/t5538-push-shallow.sh |2 +- t/t9814-git-p4-rename.sh|4 ++-- t/test-lib-functions.sh |4 ++-- 19 files changed, 49 insertions(+), 44 deletions(-) I still think one patch per file is too many patches for this. It would be easier to read with, e.g., whichever ones were most complex as separate patches and the rest (the easy ones) as a single patch. 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 25/44] receive-pack.c: use a reference transaction for updating the refs
Ronnie Sahlberg wrote:
Wrap all the ref updates inside a transaction to make the update atomic.
Interesting.
[...]
--- 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;
I think it would be cleaner for err to be local. It isn't used for
communication between functions.
[...]
@@ -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;
The original used rp_error to send an error message immediately via
sideband. This drops that --- intended?
The old error string shown on the push status line was was failed to
lock or failed to write which makes it clear that the cause is
contention or database problems or filesystem problems, respectively.
After this change it would say failed to update which is about as
clear as failed.
Would it be safe to send err.buf as-is over the wire, or does it
contain information or formatting that wouldn't be suitable for the
client? (I haven't thought this through completely yet.) Is there
some easy way to distinguish between failure to lock and failure to
write? Or is there some one-size-fits-all error for this case?
When the transaction fails, we need to make sure that all ref updates
emit 'ng' and not 'ok' in receive-pack.c::report (see the example at
the end of Documentation/technical/pack-protocol.txt for what this
means). What error string should they use? Is there some way to make
it clear to the user which ref was the culprit?
What should happen when checks outside the ref transaction system
cause a ref update to fail? I'm thinking of
* per-ref 'update' hook (see githooks(5))
* fast-forward check
* ref creation/deletion checks
* attempt to push to the currently checked out branch
I think the natural thing to do would be to put each ref update in its
own transaction to start so the semantics do not change right away.
If there are obvious answers to all these questions, then a separate
patch could combine all these into a single transaction; or if there
are no obvious answers, we could make the single-transaction-per-push
semantics optional (using a configuration variable or protocol
capability or something) to make it possible to experiment.
Hope that helps,
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 26/44] fast-import.c: use a ref transaction when dumping tags
Ronnie Sahlberg wrote:
[Subject: fast-import.c: use a ref transaction when dumping tags]
This seems like an odd thing to do: either it would make sense to have
a single transaction for all imported refs so all fail or succeed
together, or there would be separate transactions for each ref.
That said, I don't mind, particularly if it's a step on the way to
using a single transaction for everything being dumped.
[...]
--- a/fast-import.c
+++ b/fast-import.c
@@ -1736,15 +1736,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);
+ sprintf(ref_name, refs/tags/%s, t-name);
Can this overflow the buffer?
- 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);
+
+ if (ref_transaction_update(transaction, ref_name, t-sha1,
+NULL, 0, 0, err)) {
+ failure |= 1;
+ break;
+ }
}
+ if (failure || ref_transaction_commit(transaction, msg, err))
+ failure |= error(Unable to update %s, err.buf);
This 'if (failure || ...) failure |=' idiom seems strange.
Is err.buf always set if failure is nonzero?
Elsewhere failure is 0 or -1 but this introduces the possibility for
it to be temporarily 1.
dump_branches could have caused failure to be -1 before dump_tags
is called. I think the intent is
if (ref_transaction_update(...)) {
failure |= error(...);
goto out;
}
}
if (ref_transaction_commit(...))
failure |= error(...);
out:
ref_transaction_free(transaction);
...
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] t0000-*.sh: Fix the GIT_SKIP_TESTS sub-tests
Ramsay Jones wrote: --- a/t/t-basic.sh +++ b/t/t-basic.sh @@ -296,8 +296,9 @@ test_expect_success 'test --verbose-only' ' ' test_expect_success 'GIT_SKIP_TESTS' - GIT_SKIP_TESTS='git.2' \ - run_sub_test_lib_test git-skip-tests-basic \ + GIT_SKIP_TESTS='git.2' export GIT_SKIP_TESTS + test_when_finished sane_unset GIT_SKIP_TESTS Oof. Good catch. What should happen if I have set GIT_SKIP_TESTS explicitly to run only some of the tests in t-basic? 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] t0000-*.sh: Fix the GIT_SKIP_TESTS sub-tests
Hi, Ramsay Jones wrote: On 20/05/14 22:40, Jonathan Nieder wrote: What should happen if I have set GIT_SKIP_TESTS explicitly to run only some of the tests in t-basic? A quick test (with the above patch applied) shows that it works as I would expect: $ GIT_SKIP_TESTS=t.1[2-6] ./t-basic.sh ... ok 11 - test --verbose ok 12 # skip test --verbose-only (GIT_SKIP_TESTS) ok 13 # skip GIT_SKIP_TESTS (GIT_SKIP_TESTS) ok 14 # skip GIT_SKIP_TESTS several tests (GIT_SKIP_TESTS) ok 15 # skip GIT_SKIP_TESTS sh pattern (GIT_SKIP_TESTS) ok 16 # skip --run basic (GIT_SKIP_TESTS) ok 17 - --run with a range Try GIT_SKIP_TESTS=t.17 ./t-basic.sh: ok 13 - GIT_SKIP_TESTS ok 14 - GIT_SKIP_TESTS several tests ok 15 - GIT_SKIP_TESTS sh pattern ok 16 - --run basic ok 17 - --run with a range 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 27/44] walker.c: use ref transaction for ref updates
Ronnie Sahlberg wrote:
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.
Sounds scary. The usual approach is
old_sha1 = ...
... various checks ...
transaction = transaction_begin(err)
transaction_update(transaction, refname, new_sha1, old_sha1, ...);
transaction_commit(transaction, err);
which is not racy because _update checks against old_sha1.
If I understand correctly, you are saying 'have_old' is false here so
we don't have the usual protection. If the ... various checks ...
section shown above is empty, that should be fine and there is no
actual change in semantics. If the ... various checks ... section
shown above is nonempty then it could be a problem.
[...]
--- 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];
We tend to prefer strbuf instead of fixed-size buffers in new code.
[...]
- char *msg;
+ char *msg = NULL;
Needed? The existing code seems to set msg = NULL in the
!write_ref_log_details case already.
[...]
@@ -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));
Okay, so before this patch we do:
for each target in write_ref:
lock it (with no particular expectation about where it
points)
Then
unless http-fetch was passed --recover:
mark the objects pointed to by current refs as COMPLETE
Then we do HTTP GETs to grab the objects we need from a dumb HTTP
server. The COMPLETE objects tell us about objects we don't have to
bother trying to get.
When we're done, we come up with a reflog entry and write out refs
pointing to the requested commits.
This code has two callers:
git-remote-http (aka remote-curl.c::fetch_dumb)
git-http-fetch (aka http-fetch.c)
The codepath in git-remote-http gets wide use, though it's diminishing
as more people switch to smart http. It doesn't 't use the write
out some refs feature. It just wants the objects and then takes care
of writing refs on its own.
Perhaps it's worth avoiding beginning a transaction in the first place
in the !write_ref case.
The git-http-fetch command is a piece of plumbing that used to be used
by 'git clone' and 'git fetch' in the olden days when they were shell
scripts. I doubt anyone uses it. As you noticed, it doesn't have any
way to specify anything about the expected old values of the refs it
writes to. So this change doesn't introduce any race there.
+ sprintf(ref_name, refs/%s, write_ref[i]);
+ if (ref_transaction_update(transaction, ref_name,
+sha1[20 * i], NULL,
+0, 0))
+ goto rollback_and_fail;
+ }
Looks good.
+
+ if (ref_transaction_commit(transaction, msg ? msg : fetch (unknown),
+err)) {
+ error(%s, err.buf);
+ goto rollback_and_fail;
}
Also looks good.
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 28/44] refs.c: make write_ref_sha1 static
Ronnie Sahlberg wrote:
No external users call write_ref_sha1 any more so lets declare it static.
Yay!
[...]
+++ 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);
Is this forward declaration needed?
[...]
--- 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);
(nit) Would be nice to keep the documentation comment.
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 29/44] refs.c: make lock_ref_sha1 static
Ronnie Sahlberg wrote: No external callers reference lock_ref_sha1 any more so lets declare it static. Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- refs.c | 2 +- refs.h | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) Reviewed-by: Jonathan Nieder jrnie...@gmail.com -- 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 30/44] refs.c: add transaction.status and track OPEN/CLOSED/ERROR
Ronnie Sahlberg wrote:
--- a/refs.c
+++ b/refs.c
@@ -3308,6 +3308,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,
What is the difference between _TRANSACTION_CLOSED and
_TRANSACTION_ERROR?
[...]
@@ -3340,6 +3347,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);
Once the transaction is freed, transaction-status is not reachable any
more so no one can tell that you've set it to _ERROR. What is the
intended effect?
[...]
@@ -3366,6 +3378,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);
Ok.
[...]
@@ -3538,6 +3564,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;
Nit: odd use of whitespace.
Overall thoughts: I like the idea of enforcing the API more strictly
(after an error, the only permitted operations are...). The details
leave me a little confused because I don't think anything is
distinguishing between _CLOSED and _ERROR. Maybe the enum only needs
two states.
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 31/44] refs.c: remove the update_ref_lock function
Ronnie Sahlberg wrote: Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- refs.c | 30 ++ 1 file changed, 6 insertions(+), 24 deletions(-) Reviewed-by: Jonathan Nieder jrnie...@gmail.com -- 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 32/44] refs.c: remove the update_ref_write function
Ronnie Sahlberg wrote:
+++ b/refs.c
[...]
@@ -3518,14 +3499,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);
+ ret = write_ref_sha1(update-lock, update-new_sha1,
+ msg);
This changes the return value on error from 1 to -1. That seems like a
good change. It's probably worth mentioning in the commit message.
--
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] builtin/log.c: fix minor memory leak
Matthieu Moy wrote:
Signed-off-by: Matthieu Moy matthieu@imag.fr
---
Valgrind confirms, one less unreachable block ;-).
This belongs in the commit message.
[...]
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -857,20 +857,21 @@ static void add_branch_description(struct strbuf *buf,
const char *branch_name)
{
struct strbuf desc = STRBUF_INIT;
if (!branch_name || !*branch_name)
return;
read_branch_desc(desc, branch_name);
if (desc.len) {
strbuf_addch(buf, '\n');
strbuf_addbuf(buf, desc);
strbuf_addch(buf, '\n');
}
+ strbuf_release(desc);
This is an old one. The leak was introduced by v1.7.9-rc1~1^2~12
(format-patch: use branch description in cover letter, 2011-09-21).
I was a little scared to see a leak in 'git log' code, since most of
what log does involves looping over many commits. Luckily this one is
only used in make_cover_letter to create a cover letter describing the
single branch on the command line, making it is a small, one-time
leak.
Less noise from static and dynamic analysis tools is still worthwhile,
so for what it's worth,
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
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: possibly a spurious conflict in a three way merge
Hi, meanlo...@gmail.com wrote: 2. commit test.txt to master: line1 line1 3. branch and checkout branch1 4. make and commit the following change to branch1: #line1 #line2 5. checkout master 6. make and commit the following change to master: line1 #line2 7. merge branch1, git sees a conflict: HEAD line1 === #line1 branch1 #line2 Why? Thanks for a clear example. On branch1 you made the following change: (a) modify lines 1 and 2 On master, you made a different change: (b) just modify line 2 The changes touch the same chunk of lines and don't produce the same result there. Git is being conservative and letting you know that the two branches didn't agree about what line 1 should say. On the other hand, if you had a larger files and on branch1 made the following change: (a) modify lines 1 and 101 and on master, you made a different change: (b) just modify line 101 then the changes are touching different parts of the code and are merged independently. The result would be a clean merge where lines 1 and 101 are both modified. Git's conservatism can be helpful when working with code, where adjacent lines are likely to be affecting a single behavior and the conflict can help the operator to know to be extra careful. It makes less sense for line-oriented input where every line is independent. If you are often making changes to a line-oriented file, it may make sense to set up a custom merge driver to override git's merge behavior for that file. See 'Defining a custom merge driver' in gitattributes(5) for details about how that works. Hope that helps, 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] Update hard-coded header dependencies
The fall-back rules used when compilers don't support the -MMD switch to generate makefile rules based on #includes have been out of date since v1.7.12.1~22^2~8 (move git_version_string into version.c, 2012-06-02). Checked with 'make CHECK_HEADER_DEPENDENCIES=yes'. Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- Maybe it's worth switching to plain LIB_H += $(wildcard *.h) ? People using ancient compilers that never change headers wouldn't be hurt, people using modern compilers that do change headers also wouldn't be hurt, and we could stop pretending to maintain an up-to-date list. Makefile | 8 1 file changed, 8 insertions(+) diff --git a/Makefile b/Makefile index 2320de5..18f0fad 100644 --- a/Makefile +++ b/Makefile @@ -646,15 +646,19 @@ LIB_H += cache.h LIB_H += color.h LIB_H += column.h LIB_H += commit.h +LIB_H += commit-slab.h +LIB_H += compat/apple-common-crypto.h LIB_H += compat/bswap.h LIB_H += compat/mingw.h LIB_H += compat/obstack.h LIB_H += compat/poll/poll.h LIB_H += compat/precompose_utf8.h LIB_H += compat/terminal.h +LIB_H += compat/win32/alloca.h LIB_H += compat/win32/dirent.h LIB_H += compat/win32/pthread.h LIB_H += compat/win32/syslog.h +LIB_H += connect.h LIB_H += connected.h LIB_H += convert.h LIB_H += credential.h @@ -678,6 +682,7 @@ LIB_H += grep.h LIB_H += hashmap.h LIB_H += help.h LIB_H += http.h +LIB_H += khash.h LIB_H += kwset.h LIB_H += levenshtein.h LIB_H += line-log.h @@ -721,6 +726,7 @@ LIB_H += sha1-lookup.h LIB_H += shortlog.h LIB_H += sideband.h LIB_H += sigchain.h +LIB_H += split-index.h LIB_H += strbuf.h LIB_H += streaming.h LIB_H += string-list.h @@ -728,6 +734,7 @@ LIB_H += submodule.h LIB_H += tag.h LIB_H += tar.h LIB_H += thread-utils.h +LIB_H += trace.h LIB_H += transport.h LIB_H += tree-walk.h LIB_H += tree.h @@ -744,6 +751,7 @@ LIB_H += vcs-svn/repo_tree.h LIB_H += vcs-svn/sliding_window.h LIB_H += vcs-svn/svndiff.h LIB_H += vcs-svn/svndump.h +LIB_H += version.h LIB_H += walker.h LIB_H += wildmatch.h LIB_H += wt-status.h -- -- 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] mailsplit.c: remove dead code
Stefan Beller wrote: This was found by coverity. (Id: 290001) [...] Signed-off-by: Stefan Beller stefanbel...@gmail.com Helped-by: René Scharfe l@web.de --- builtin/mailsplit.c | 17 ++--- 1 file changed, 6 insertions(+), 11 deletions(-) The idea is to simplify error handling (removing cleanup of the 'output' var) by making it more obvious that there is only one kind of error, right? For what it's worth, it looks good to me (and much clearer than the last version). It's always possible to reintroduce goto-based error handling later if another kind of error is introduced, and in the meantime this is simpler and does not change behavior. Reviewed-by: Jonathan Nieder jrnie...@gmail.com 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 0/3] Re: [PATCH] unpack-tree.c: remove dead code
Stefan Beller wrote:
In line 1763 of unpack-tree.c we have a condition on the current tree
[...]
The description is describing why the patch is *correct* (i.e., not
going to introduce a bug), while what the reader wants to know is why
the change is *desirable*.
Is this about making the code more readable, or robust, or suppressing
a static analysis error, or something else? What did the user or
reader want to do that they couldn't do before and now can after this
patch?
[...]
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1789,15 +1789,11 @@ int twoway_merge(const struct cache_entry * const
*src,
/* 20 or 21 */
return merged_entry(newtree, current, o);
}
+ else if (o-gently) {
+ return -1 ;
+ }
(not about this patch) Elsewhere git uses the 'cuddled else':
if (foo) {
...
} else if (bar) {
...
} else {
...
}
That stylefix would be a topic for a different patch, though.
else {
- /* all other failures */
- if (oldtree)
- return o-gently ? -1 : reject_merge(oldtree,
o);
- if (current)
- return o-gently ? -1 : reject_merge(current,
o);
- if (newtree)
- return o-gently ? -1 : reject_merge(newtree,
o);
- return -1;
Does the static analysis tool support comments like
if (oldtree)
...
if (current)
...
...
/* not reached */
return -1;
? That might be the simplest minimally invasive fix for what coverity
pointed out.
Now that we're looking there, though, it's worth understanding why we
do the 'if oldtree exists, use it, else fall back to, etc' thing. Was
this meant as futureproofing in case commands like 'git checkout' want
to do rename detection some day?
Everywhere else in the file that reject_merge is used, it is as
return o-gently ? -1 : reject_merge(..., o);
The one exception is
!current
oldtree
newtree
oldtree != newtree
!initial_checkout
(#17), which seems like a bug (it should have the same check). Would
it make sense to inline the o-gently check into reject_merge so callers
don't have to care?
In that spirit, I suspect the simplest fix would be
else
return o-gently ? -1 : reject_merge(current, o);
and then all calls could be replaced in a followup patch.
Sensible?
Thanks,
Jonathan Nieder (2):
unpack-trees: use 'cuddled' style for if-else cascade
checkout -m: attempt merge when deletion of path was staged
Stefan Beller (1):
unpack-trees: simplify 'all other failures' case
unpack-trees.c | 31 ++-
1 file changed, 10 insertions(+), 21 deletions(-)
--
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 1/3] unpack-trees: simplify 'all other failures' case
From: Stefan Beller stefanbel...@gmail.com
In the 'if (current)' block of twoway_merge, we handle the boring
errors by checking if the entry from the old tree, current index, and
new tree are present, to get a pathname for the error message from one
of them:
if (oldtree)
return o-gently ? -1 : reject_merge(oldtree, o);
if (current)
return o-gently ? -1 : reject_merge(current, o);
if (newtree)
return o-gently ? -1 : reject_merge(newtree, o);
return -1;
Since this is guarded by 'if (current)', the second test is guaranteed
to succeed. Moreover, any of the three entries, if present, would
have the same path because there is no rename detection in this code
path. Even if some day in the future the entries' paths differ, the
'current' path used in the index and worktree would presumably be the
most recognizable for the end user.
Simplify by just using 'current'.
Noticed by coverity, Id:290002
Signed-off-by: Stefan Beller stefanbel...@gmail.com
Improved-by: Junio C Hamano gits...@pobox.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
unpack-trees.c | 12 ++--
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index ad3e9a0..f4a9aa9 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1791,16 +1791,8 @@ int twoway_merge(const struct cache_entry * const *src,
/* 20 or 21 */
return merged_entry(newtree, current, o);
}
- else {
- /* all other failures */
- if (oldtree)
- return o-gently ? -1 : reject_merge(oldtree,
o);
- if (current)
- return o-gently ? -1 : reject_merge(current,
o);
- if (newtree)
- return o-gently ? -1 : reject_merge(newtree,
o);
- return -1;
- }
+ else
+ return o-gently ? -1 : reject_merge(current, o);
}
else if (newtree) {
if (oldtree !o-initial_checkout) {
--
--
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 2/3] unpack-trees: use 'cuddled' style for if-else cascade
Match the predominant style in git by following KR style for if/else
cascades. Documentation/CodingStyle from linux.git explains:
Note that the closing brace is empty on a line of its own, _except_ in
the cases where it is followed by a continuation of the same statement,
ie a while in a do-statement or an else in an if-statement, like
this:
if (x == y) {
..
} else if (x y) {
...
} else {
}
Rationale: KR.
Also, note that this brace-placement also minimizes the number of empty
(or almost empty) lines, without any loss of readability.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
unpack-trees.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index f4a9aa9..187b15b 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1771,8 +1771,7 @@ int twoway_merge(const struct cache_entry * const *src,
return merged_entry(newtree, current,
o);
}
return o-gently ? -1 : reject_merge(current, o);
- }
- else if ((!oldtree !newtree) || /* 4 and 5 */
+ } else if ((!oldtree !newtree) || /* 4 and 5 */
(!oldtree newtree
same(current, newtree)) || /* 6 and 7 */
(oldtree newtree
@@ -1781,17 +1780,14 @@ int twoway_merge(const struct cache_entry * const *src,
!same(oldtree, newtree) /* 18 and 19 */
same(current, newtree))) {
return keep_entry(current, o);
- }
- else if (oldtree !newtree same(current, oldtree)) {
+ } else if (oldtree !newtree same(current, oldtree)) {
/* 10 or 11 */
return deleted_entry(oldtree, current, o);
- }
- else if (oldtree newtree
+ } else if (oldtree newtree
same(current, oldtree) !same(current, newtree)) {
/* 20 or 21 */
return merged_entry(newtree, current, o);
- }
- else
+ } else
return o-gently ? -1 : reject_merge(current, o);
}
else if (newtree) {
--
--
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 3/3] checkout -m: attempt merge when deletion of path was staged
twoway_merge() is missing an o-gently check in the case where a file
that needs to be modified is missing from the index but present in the
old and new trees. As a result, in this case 'git checkout -m' errors
out instead of trying to perform a merge.
Fix it by checking o-gently. While at it, inline the o-gently check
into reject_merge to prevent future call sites from making the same
mistake.
Noticed by code inspection. The motivating case hasn't been tested.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
This is the most iffy of the three patches, mostly because I was too
lazy to write a test. I believe it's safe as-is nonetheless.
Thanks for reading.
unpack-trees.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index 187b15b..6c45af7 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1178,7 +1178,8 @@ return_failed:
static int reject_merge(const struct cache_entry *ce,
struct unpack_trees_options *o)
{
- return add_rejected_path(o, ERROR_WOULD_OVERWRITE, ce-name);
+ return o-gently ? -1 :
+ add_rejected_path(o, ERROR_WOULD_OVERWRITE, ce-name);
}
static int same(const struct cache_entry *a, const struct cache_entry *b)
@@ -1633,7 +1634,7 @@ int threeway_merge(const struct cache_entry * const
*stages,
/* #14, #14ALT, #2ALT */
if (remote !df_conflict_head head_match !remote_match) {
if (index !same(index, remote) !same(index, head))
- return o-gently ? -1 : reject_merge(index, o);
+ return reject_merge(index, o);
return merged_entry(remote, index, o);
}
/*
@@ -1641,7 +1642,7 @@ int threeway_merge(const struct cache_entry * const
*stages,
* make sure that it matches head.
*/
if (index !same(index, head))
- return o-gently ? -1 : reject_merge(index, o);
+ return reject_merge(index, o);
if (head) {
/* #5ALT, #15 */
@@ -1770,7 +1771,7 @@ int twoway_merge(const struct cache_entry * const *src,
else
return merged_entry(newtree, current,
o);
}
- return o-gently ? -1 : reject_merge(current, o);
+ return reject_merge(current, o);
} else if ((!oldtree !newtree) || /* 4 and 5 */
(!oldtree newtree
same(current, newtree)) || /* 6 and 7 */
@@ -1788,7 +1789,7 @@ int twoway_merge(const struct cache_entry * const *src,
/* 20 or 21 */
return merged_entry(newtree, current, o);
} else
- return o-gently ? -1 : reject_merge(current, o);
+ return reject_merge(current, o);
}
else if (newtree) {
if (oldtree !o-initial_checkout) {
--
--
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] git-imap-send: use libcurl for implementation
Bernhard Reiter wrote:
Use libcurl's high-level API functions to implement git-imap-send
instead of the previous low-level OpenSSL-based functions.
Wow! This sounds lovely. Thanks for working on this.
[...]
Since version 7.30.0, libcurl's API has been able to communicate with
IMAP servers. Using those high-level functions instead of the current
ones reduces imap-send.c by some 1200 lines of code.
As I don't have access to that many IMAP servers, I haven't been able to
test a variety of parameter combinations. I did test both secure and
insecure (imaps:// and imap://) connections -- this e-mail was generated
that way -- but could e.g. neither test the authMethod nor the tunnel
parameter.
The above two paragraphs belong in the commit message, too, since
they'll be just as useful for someone looking back through the history
as for someone reading the patch on-list today.
[...]
--- a/INSTALL
+++ b/INSTALL
[...]
- - libcurl library is used by git-http-fetch and git-fetch. You
+ - libcurl library is used by git-http-fetch, git-fetch, and
+ git-impap-send. You might also want the curl executable for
Typo: s/impap-send/imap-send/
--- a/Makefile
+++ b/Makefile
@@ -2067,9 +2067,9 @@ endif
git-%$X: %.o GIT-LDFLAGS $(GITLIBS)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^)
$(LIBS)
-git-imap-send$X: imap-send.o GIT-LDFLAGS $(GITLIBS)
+git-imap-send$X: imap-send.o http.o GIT-LDFLAGS $(GITLIBS)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
- $(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)
+ $(LIBS) $(CURL_LIBCURL)
7.30.0 is only ~1 year old. Does this mean users would need to update
curl in order to build imap-send?
For example, Debian 7.6 ships curl 7.26.0 and CentOS 7 has curl 7.29.0.
Ideally this could be done as an optional feature:
1. Introduce a USE_CURL_FOR_IMAP_SEND makefile variable to take
advantage of the nice new API.
2. (optional) Use the curl_check makefile variable to turn on
USE_CURL_FOR_IMAP_SEND automatically when appropriate.
3. In a few years, when everyone has upgraded, we could simplify by
getting rid of the USE_OPENSSL_FOR_IMAP_SEND code path.
What do you think?
[...]
--- a/imap-send.c
+++ b/imap-send.c
@@ -22,47 +22,13 @@
* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
*/
-#include cache.h
Usual style is to start with a #include of cache.h or
git-compat-util.h. http.h including cache.h for itself was an
old mistake. (I'll reply with a patch to fix that.)
[...]
+#include curl/curl.h
http.h already #includes this. Do you use other helpers from
http.h/http.c or do you use libcurl directly? (Just curious.)
Some style nits:
[...]
+static curl_socket_t opensocket(void *clientp, curlsocktype purpose,
+ struct
curl_sockaddr *address)
Long line. Do you have ts=4? (Git uses 8-space tabs. There's some
emacs magic in Documentation/CodingGuidelines. It should be possible
to add similar hints there for other editors if they don't do the
right thing by default.)
+{
+ curl_socket_t sockfd;
+ (void)purpose;
+ (void)address;
Elsewhere git lets unused parameters be. The unused parameter warning
is too noisy in callback-heavy code (e.g., for_each_ref) so we don't
turn it on.
+ sockfd = *(curl_socket_t *)clientp;
+ /* the actual externally set socket is passed in via the OPENSOCKETDATA
+option */
(style nit) Comments in git look like this:
/*
* The actual, externally set socket is passed in via the
* OPENSOCKETDATA option.
*/
return sockfd;
[...]
+static int sockopt_callback(void *clientp, curl_socket_t curlfd,
+ curlsocktype purpose)
+{
+ /* This return code was added in libcurl 7.21.5 */
+ return CURL_SOCKOPT_ALREADY_CONNECTED;
I'd drop the comment, unless there's some subtlety involved. (E.g.,
is there some other return code that would be more appropriate but was
introudced later or something?)
[...]
@@ -1368,12 +218,14 @@ static int git_imap_config(const char *key, const char
*val, void *cb)
int main(int argc, char **argv)
{
struct strbuf all_msgs = STRBUF_INIT;
- struct strbuf msg = STRBUF_INIT;
+ struct buffer msg = { STRBUF_INIT, 0 };
Ah, ok --- we do use http.c stuff.
[...]
+ char path[8192];
+ int pathlen;
I realize the old code only had 8192 for the IMAP command buffer,
but could this be a strbuf now, or is there some underlying limit
somewhere else?
[...]
@@ -1417,31 +269,89 @@ int main(int argc, char **argv)
return 1;
}
+ curl_global_init(CURL_GLOBAL_ALL);
http.c seems to make the same mistake, but should the return value
from this be checked?
- /* write it to the imap server */
-
Re: Git on Mac OS X 10.4.10
Hi Markus, export NO_APPLE_COMMON_CRYPTO=yes make configure CFLAGS=-O2 ./configure --without-tcltk --prefix=/usr/global make all compiles fine on 10.4.10. Would a configure patch checking for the existence of CommonHMAC.h and, if not found, defining this variable, be acceptable? Yes, that sounds useful. Orthogonal to that: a patch to the Darwin section of config.mak.uname so you can build without the 'make configure' would be even more welcome. :) See Documentation/SubmittingPatches for details about how to contribute to git. (Or if you're short on time, see section 5 of that file, Sign your work, and then push your code somewhere and tell us about it.) 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: Relative submodule URLs
Hi,
Robert Dailey wrote:
The documentation wasn't 100% clear on this, but I'm assuming by
remote origin, it says that the relative URL is relative to the
actual remote *named* origin (and it is not using origin as just a
general terminology).
Thanks for reporting. The remote used is the default remote that git
fetch without further arguments would use:
get_default_remote () {
curr_branch=$(git symbolic-ref -q HEAD)
curr_branch=${curr_branch#refs/heads/}
origin=$(git config --get branch.$curr_branch.remote)
echo ${origin:-origin}
}
The documentation is wrong. git-fetch(1) doesn't provide a name for
this thing. Any ideas for wording?
Is there a way to specify (on a per-clone basis) which named remote
will be used to calculate the URL for submodules?
Currently there isn't, short of reconfiguring the remote used by
default by git fetch.
Various co-workers use the remote named central instead of
upstream and fork instead of origin (because that just makes
more sense to them and it's perfectly valid).
However if relative submodules require 'origin' to exist AND also
represent the upstream repository (in triangle workflow), then this
breaks on several levels.
Can you explain further? In a triangle workflow, git fetch will
pull from the 'origin' remote by default and will push to the remote
named in the '[remote] pushdefault' setting (see remote.pushdefault
in git-config(1)). So you can do
[remote]
pushDefault = whereishouldpush
and then 'git fetch' and 'git fetch --recurse-submodules' will fetch
from origin and 'git push' will push to the whereishouldpush remote.
It might make sense to introduce a new
[remote]
default = whereishouldfetch
setting to allow the name origin above to be replaced, too. Is that
what you mean?
Meanwhile it is hard to fork a project that uses relative submodule
URLs without also forking the submodules (or, conversely, to fork some
of the submodules of a project that uses absolute submodule URLs).
That's a real and serious problem but I'm not sure how it relates to
the names of remotes. My preferred fix involves teaching git to read
a refs/meta/git (or similarly named) ref when cloning a project with
submodules and let settings from .gitmodules in that ref override
.gitmodules in other branches. Is that what you were referring to?
Curious,
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 v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic
Hi, Ronnie Sahlberg wrote: On Wed, Aug 20, 2014 at 7:52 AM, Michael Haggerty mhag...@alum.mit.edu wrote: I'm a little worried that abandoning *all* refname checks could open us up to somehow trying to delete a reference with a name like ../../../../etc/passwd. Either such names have to be prohibited somehow, or we have to be very sure that they can only come from trusted sources. I only set this flag from builtin/branch.c so it should only be used when a user runs 'git branch -D' from the command line. All other places where we delete branches we should still be checking the rename for badness. Right, this should be safe for 'git branch -D' and 'git update-ref -d'. But if we wanted to open it up in the future for 'git push --delete', too, then it would be a way to break out of the repository on hosts where people use git-shell instead of relying on filesystem permissions. And that wouldn't be good. I think elsewhere git has some checks for does this pathname fall in this directory. Could that be reused here, too, to make sure the resolved path is under the resolved $GIT_DIR/refs directory? Alternatively, when a ref being deleted doesn't meet the 'check-ref-format' checks, would it make sense to check that it is one of the refs you can get by iteration? 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: Transaction patch series overview
Hi, Ronnie Sahlberg wrote: List, please see here an overview and ordering of the ref transaction patch series. Thanks much for this. [...] rs/ref-transaction-0 [...] Has been merged into next. This is even part of master now, so if people have review comments then they can make them most easily by submitting new patches on top. ref-transaction-1 (2014-07-16) 20 commits - Second batch of ref transactions - refs.c: make delete_ref use a transaction - refs.c: make prune_ref use a transaction to delete the ref - refs.c: remove lock_ref_sha1 - refs.c: remove the update_ref_write function - refs.c: remove the update_ref_lock function - refs.c: make lock_ref_sha1 static - walker.c: use ref transaction for ref updates - fast-import.c: use a ref transaction when dumping tags - receive-pack.c: use a reference transaction for updating the refs - refs.c: change update_ref to use a transaction - branch.c: use ref transaction for all ref updates - fast-import.c: change update_branch to use ref transactions - sequencer.c: use ref transactions for all ref updates - commit.c: use ref transactions for updates - replace.c: use the ref transaction functions for updates - tag.c: use ref transactions when doing updates - refs.c: add transaction.status and track OPEN/CLOSED/ERROR - refs.c: make ref_transaction_begin take an err argument - refs.c: update ref_transaction_delete to check for error and return status - refs.c: change ref_transaction_create to do error checking and return status (this branch is used by rs/ref-transaction, rs/ref-transaction-multi, rs/ref-transaction-reflog and rs/ref-transaction-rename.) The second batch of the transactional ref update series. Has been merged into pu Last reviewed at http://thread.gmane.org/gmane.comp.version-control.git/252226, if I am using gmane search correctly. Are there any pending questions for this part? I've having trouble keeping track of how patches change, which patches have been reviewed and which haven't, unaddressed comments, and so on, so as an experiment I've pushed this part of the series to the Gerrit server at https://code-review.googlesource.com/#/q/topic:ref-transaction-1 It's running a copy of gerrit code review (https://code.google.com/p/gerrit). Maybe this can be useful as a way of keeping track of the state of long and long-lived series like this one. It works something like this: Preparation --- Get a password from https://code-review.googlesource.com/new-password and put it in netrc. Install the hook from https://code-review.googlesource.com/tools/hooks/commit-msg to .git/hooks/commit-msg. This automatically populates a Change-Id line in the commit message if none is present so gerrit can tell when you are uploading a new version of an existing patch. (The commit-msg hook can be annoying when not working with gerrit code review. To turn it off, use 'git config gerrit.createChangeId false'.) Uploading patches - Write a patch series against 'maint' or 'master' as usual, then push: git push https://code.googlesource.com/git \ HEAD:refs/for/master; # or refs/for/maint There can be a parameter '%topic=name' after the refs/for/branch (e.g., refs/for/master%topic=ref-transaction-1) if the series should be labelled with a topic name, or that can be set in the web UI or using the HTTP API after the fact. Commenting on patches - Visit https://code-review.googlesource.com. Patches can be found under All - Open. Patches you're involved with somehow are at My - Changes. To prepare inline comments, choose a file, highlight the text to comment on or click a line number, and comment. To publish comments, go back up to the change overview screen (using the up arrow button or by pressing u), click Reply, and say something. '?' brings up keyboard shortcuts. Downloading patches --- In the change overview screen, there's a 'Download' menu in the top-right corner with commands to download the patch. Revising patches After modifying a patch locally using the usual tools such as rebase --interactive, push again: git push https://code.googlesource.com/git \ HEAD:refs/for/master; # or refs/for/maint When a patch seems polished --- Get rid of the Change-Id lines --- they shouldn't be needed any more. Send the patches or a request to pull from a public git repository, as usual. A link (in the top-left corner where it says 'Change 1000', the 1000 is a link to the change) can be helpful for letting people on-list see how the patch got into the form it's in today. Maybe it can be useful. 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 1/2] Makefile: use find to determine static header dependencies
Hi, Jeff King wrote: However, we do always define $(LIB_H) as a dependency of po/git.pot. Even though we do not actually try to build that target, make will still evaluate the dependencies when reading the Makefile, and expand the variable. This is not ideal Would the following work? The current dependencies for po/git.pot are not correct anyway --- they include LOCALIZED_C but not LOCALIZED_SH or LOCALIZED_PERL, so someone hacking on shell scripts and then trying 'make po/git.pot' could end up with the pot file not being regenerated. -- 8 -- Subject: i18n: treat make pot as an explicitly-invoked target po/git.pot is normally used as-is and not regenerated by people building git, so it is okay if an explicit make po/git.pot always automatically regenerates it. Depend on the magic FORCE target instead of explicitly keeping track of dependencies. This simplifies the makefile, in particular preparing for a moment when $(LIB_H), which is part of $(LOCALIZED_C), can be computed on the fly. We still need a dependency on GENERATED_H, to force those files to be built when regenerating git.pot. Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 2320de5..cf0ccdf 100644 --- a/Makefile +++ b/Makefile @@ -2138,7 +2138,7 @@ LOCALIZED_SH += t/t0200/test.sh LOCALIZED_PERL += t/t0200/test.perl endif -po/git.pot: $(LOCALIZED_C) +po/git.pot: $(GENERATED_H) FORCE $(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ $(XGETTEXT_FLAGS_C) $(LOCALIZED_C) $(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ --join-existing $(XGETTEXT_FLAGS_SH) \ $(LOCALIZED_SH) -- -- 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: Hook post-merge does not get executed in case of confilicts
Hi, Bertram Scharpf wrote: today I wrote a port-merge hook. Then I just detected that it only gets executed when the merge is immediately successful. In case there is a conflict, I have to finish the merge using the command git commit. This will not call the post-merge hook. I think the hook should be reliable to be executed on _every_ non-failed merge. Therefore I propose the below extension. I agree that at first glance this sounds like a good thing. A manual conflict resolution is not so different from a very smart merge strategy, after all. Nits: Bertram Sign-off? (See Documentation/SubmittingPatches, section 5 Sign your work for what this means. --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1783,6 +1783,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix) rerere(0); run_commit_hook(use_editor, get_index_file(), post-commit, NULL); + if (whence == FROM_MERGE) + run_hook_le(NULL, post-merge, 0, NULL); git merge doesn't run the post-commit hook, so there's a new asymmetry being introduced here. Should git merge run the post-commit hook? Should a git commit that means git merge --continue avoid running it? Also if doing this for real, the documentation should be updated and tests introduced to make sure the behavior doesn't get broken in the future. Documentation/githooks.txt currently says This hook cannot affect the outcome of git merge and is not executed if the merge failed due to conflicts. which would need to be updated to say that the hook will run later in that case, when the merge is finally committed. Thanks and hope that helps, 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
check-ref-format: include refs/ in the argument or to strip it?
Hi,
Michael Haggerty wrote[1]:
Jonathan Nieder wrote:
The check-ref-format documentation is pretty unclear, but the
intent is that it would be used like
git check-ref-format heads/master
(see the surviving examples in contrib/examples/). That way, it can
enforce the rule (from git-check-ref-format(1))
2. They must contain at least one /. This enforces the presence
of a category like heads/, tags/ etc. but the actual names
are not restricted.
[...]
Thanks for the explanation and the pointer.
I suppose that was a usage that was more popular in the past than
now. I can hardly remember anyone talking about references like
heads/master or tags/v1. It seems to me that when somebody wants
to be unambiguous they usually use the whole reference name
refs/heads/master or refs/tags/v1. When they want to be succinct
they usually use just master or v1.
To me it seems incongruous to have the heads/master syntax
supported at this deep level of plumbing. In most cases, the caller
could get the same effect by prepending refs/ to the string and
then calling check_refname_format with a new
REFNAME_REQUIRE_CATEGORY option that requires both a refs/ prefix
and a total of at least *three* levels.
I agree that this piece of UI is pretty weird. Worse, it's
underdocumented.
I wonder if it would make sense to have the check-ref-format commandline
utility require two slashes when its argument begins with refs/ (still
requiring only one slash when it doesn't, for backward compatibility)
and to start encouraging people to pass refnames with refs/ to it.
The alternative would be something like the following, which doesn't
seem too appealing.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
[1] https://code-review.googlesource.com/1017
Documentation/git-check-ref-format.txt | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/Documentation/git-check-ref-format.txt
b/Documentation/git-check-ref-format.txt
index fc02959..447e9fb 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -15,8 +15,8 @@ SYNOPSIS
DESCRIPTION
---
-Checks if a given 'refname' is acceptable, and exits with a non-zero
-status if it is not.
+Checks if refs/refname is an acceptable ref name, and exits
+with a non-zero status if it is not.
A reference is used in Git to specify branches and tags. A
branch head is stored in the `refs/heads` hierarchy, while
@@ -91,14 +91,14 @@ OPTIONS
components). The default is `--no-allow-onelevel`.
--refspec-pattern::
- Interpret refname as a reference name pattern for a refspec
- (as used with remote repositories). If this option is
- enabled, refname is allowed to contain a single `*`
- in place of a one full pathname component (e.g.,
- `foo/*/bar` but not `foo/bar*`).
+ Interpret refs/refname as a reference name pattern
+ for a refspec (as used with remote repositories).
+ If this option is enabled, refname is allowed
+ to contain a single `*` in place of a one full pathname
+ component (e.g., `foo/*/bar` but not `foo/bar*`).
--normalize::
- Normalize 'refname' by removing any leading slash (`/`)
+ Normalize refname by removing any leading slash (`/`)
characters and collapsing runs of adjacent slashes between
name components into a single slash. Iff the normalized
refname is valid then print it to standard output and exit
@@ -118,7 +118,7 @@ $ git check-ref-format --branch @{-1}
* Determine the reference name to use for a new branch:
+
-$ ref=$(git check-ref-format --normalize refs/heads/$newbranch) ||
+$ ref=$(git check-ref-format --normalize heads/$newbranch) ||
die we do not like '$newbranch' as a branch 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: check-ref-format: include refs/ in the argument or to strip it?
Junio C Hamano wrote: implication of which is that the 'at least one slash' rule was to expect things are 'refs/anything' so there will be at least one. Even back then, that anything alone had at least one slash (e.g. heads/master), but the intention was *never* that we would forbid anything that does not have a slash by feeding anything part alone to check-ref-format, i.e. things like refs/stash were designed to be allowed. Now I'm more confused. Until 5f7b202a (2008-01-01), there was a comment if (level 2) return -2; /* at least of form heads/blah */ and that behavior has been preserved since the beginning. Why do most old callers pass a string that doesn't start with refs/ (e.g., see the callers in 03feddd6, 2005-10-13)? Has the intent been to relax the requirement since then? 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: check-ref-format: include refs/ in the argument or to strip it?
Junio C Hamano wrote: Michael Haggerty wrote[1]: Jonathan Nieder wrote: The check-ref-format documentation is pretty unclear, but the intent is that it would be used like git check-ref-format heads/master (see the surviving examples in contrib/examples/). That way, it can enforce the rule (from git-check-ref-format(1)) [...] Thanks for the explanation and the pointer. I wanted to follow this discussion, especially the ellided [...] pointer, but had a hart time finding what pointer was. I missed this question before. The discussion happened at https://code-review.googlesource.com/1017. It's easier to see after clicking the 'Expand All' button, but even then it's hard to see the signal in the 'Patch Set n was rebased' noise. The pointer Michael mentioned was to the git-check-ref-format(1) manpage and old 'check-ref-format' callers that can be found in contrib/examples/. Sorry for the lack of context. 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: check-ref-format: include refs/ in the argument or to strip it?
Ronnie Sahlberg wrote: On Fri, Aug 22, 2014 at 10:46 PM, Jeff King p...@peff.net wrote: Yeah, this weird do not allow refs/foo behavior has continually confused me. Coincidentally I just noticed a case today where pack-refs treats refs/foo specially for no good reason: http://thread.gmane.org/gmane.comp.version-control.git/255729 After much head scratching over the years, I am of the opinion that nobody every really _meant_ to prevent refs/foo, and that code comments like the one you quote above were an attempt to document existing buggy behavior that was really trying to differentiate HEAD from refs/*. That's just my opinion, though. :) It's still very puzzling to me. The comment came at the same time as the behavior, in v0.99.9~120 (git-check-ref-format: reject funny ref names, 2005-10-13). Before that, the behavior was even stranger --- it checked that there was exactly one slash in the argument. I'm willing to believe we might not want that check any more, though. [...] There are also a lot of places where we assume that a refs will start with refs/heads/ and not just refs/ for_each_branch_ref(), log_ref_setup() (so no reflogs) is_branch() to name a few. for_each_branch_ref is for iterating over local branches, which are defined as refs that start with refs/heads/*. Likewise, the only point of is_branch is to check whether a ref is under refs/heads/*. That's not an assumption about all refs. log_ref_setup implements the policy that there are only reflogs for: * refs where the reflog was explicitly created (git branch --create-reflog does this, but for some reason there's no corresponding git update-ref --create-reflog so people have to use mkdir directly for other refs), plus * if the '[core] logallrefupdates' configuration is enabled (and it is by default for non-bare repositories), then HEAD, refs/heads/*, refs/notes/*, and refs/remotes/*. This is documented in git-config(1) --- see core.logAllRefUpdates. That way, when tools internally use other refs (e.g., FETCH_HEAD), git doesn't have to automatically incur the cost of maintaining the reflog for those. What other refs should there be reflogs for? I haven't thought carefully about this. It definitely isn't an assumption that *all* refs will match that pattern. But it might be worth changing for other reasons. 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 2/3] Makefile: use `find` to determine static header dependencies
Jeff King wrote: -LOCALIZED_C := $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H) +LOCALIZED_C = $(C_OBJ:o=c) $(GENERATED_H) Why is LIB_H dropped here? This would mean that po/git.pot stops including strings from macros and static inline functions in headers (e.g., in parse-options.h). The rest looks good. 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 2/3] Makefile: use `find` to determine static header dependencies
Jeff King wrote: It is unfortunately easy for our static header list to grow stale, as none of the regular developers make use of it. Instead of trying to keep it up to date, let's invoke find to generate the list dynamically. Yep, I like this. It does mean that people who run make pot have to be a little more vigilant about not keeping around extra .h files by mistake. But I trust them. [...] +LIB_H = $(shell $(FIND) . \ + -name .git -prune -o \ + -name t -prune -o \ + -name Documentation -prune -o \ + -name '*.h' -print) Tiny nit: I might use $(shell $(FIND) * \ -name . -o -name '.*' -prune -o \ -name t -prune -o \ -name Documentation -prune -o \ -name '*.h' -print) or $(shell $(FIND) * \ -name '.?*' -prune -o \ -name t -prune -o \ -name Documentation -prune -o \ -name '*.h' -print) to avoid spending time looking in other dot-directories like .svn, .hg, or .snapshot. With or without such a change, Reviewed-by: Jonathan Nieder jrnie...@gmail.com -- 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 2/3] Makefile: use `find` to determine static header dependencies
Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: Tiny nit: I might use $(shell $(FIND) * \ -name . -o -name '.*' -prune -o \ -name t -prune -o \ -name Documentation -prune -o \ -name '*.h' -print) or $(shell $(FIND) * \ -name '.?*' -prune -o \ -name t -prune -o \ -name Documentation -prune -o \ -name '*.h' -print) to avoid spending time looking in other dot-directories like .svn, .hg, or .snapshot. Wouldn't it be sufficient to start digging not from * but from ??*? Gah, the * was supposed to be . in my examples (though it doesn't hurt). find ??* \( -name Documentation -o -name .\?\* \) -prune -o -name \*.h Heh. Yeah, that would work. ;-) -- 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: Transaction patch series overview
Jonathan Nieder wrote:
Ronnie Sahlberg wrote:
ref-transaction-1 (2014-07-16) 20 commits
-
Second batch of ref transactions
- refs.c: make delete_ref use a transaction
- refs.c: make prune_ref use a transaction to delete the ref
- refs.c: remove lock_ref_sha1
- refs.c: remove the update_ref_write function
- refs.c: remove the update_ref_lock function
- refs.c: make lock_ref_sha1 static
- walker.c: use ref transaction for ref updates
- fast-import.c: use a ref transaction when dumping tags
- receive-pack.c: use a reference transaction for updating the refs
- refs.c: change update_ref to use a transaction
- branch.c: use ref transaction for all ref updates
- fast-import.c: change update_branch to use ref transactions
- sequencer.c: use ref transactions for all ref updates
- commit.c: use ref transactions for updates
- replace.c: use the ref transaction functions for updates
- tag.c: use ref transactions when doing updates
- refs.c: add transaction.status and track OPEN/CLOSED/ERROR
- refs.c: make ref_transaction_begin take an err argument
- refs.c: update ref_transaction_delete to check for error and return status
- refs.c: change ref_transaction_create to do error checking and return
status
(this branch is used by rs/ref-transaction, rs/ref-transaction-multi,
rs/ref-transaction-reflog and rs/ref-transaction-rename.)
[...]
I've having trouble keeping track of how patches change, which patches
have been reviewed and which haven't, unaddressed comments, and so on,
so as an experiment I've pushed this part of the series to the Gerrit
server at
https://code-review.googlesource.com/#/q/topic:ref-transaction-1
Outcome of the experiment: patches gained some minor changes and then
1-12
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
13
Reviewed-by: Michael Haggerty mhag...@alum.mit.edu
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
14
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
15-16
Reviewed-by: Michael Haggerty mhag...@alum.mit.edu
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
17
Reviewed-by: Michael Haggerty mhag...@alum.mit.edu
18
Reviewed-by: Michael Haggerty mhag...@alum.mit.edu
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
19
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
20
Reviewed-by: Michael Haggerty mhag...@alum.mit.edu
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
I found the web UI helpful in seeing how each patch evolved since I
last looked at it. Interdiff relative to the version in pu is below.
I'm still hoping for a tweak in response to a minor comment and then I
can put up a copy of the updated series to pull.
The next series from Ronnie's collection is available at
https://code-review.googlesource.com/#/q/topic:ref-transaction in case
someone wants a fresh series to look at.
diff --git a/branch.c b/branch.c
index c1eae00..37ac555 100644
--- a/branch.c
+++ b/branch.c
@@ -305,6 +305,7 @@ void create_branch(const char *head,
ref_transaction_commit(transaction, msg, err))
die(%s, err.buf);
ref_transaction_free(transaction);
+ strbuf_release(err);
}
if (real_ref track)
diff --git a/builtin/commit.c b/builtin/commit.c
index 668fa6a..9bf1003 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1765,8 +1765,8 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
transaction = ref_transaction_begin(err);
if (!transaction ||
ref_transaction_update(transaction, HEAD, sha1,
- current_head ?
- current_head-object.sha1 : NULL,
+ current_head
+ ? current_head-object.sha1 : NULL,
0, !!current_head, err) ||
ref_transaction_commit(transaction, sb.buf, err)) {
rollback_index_files();
@@ -1801,5 +1801,6 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
if (!quiet)
print_summary(prefix, sha1, !current_head);
+ strbuf_release(err);
return 0;
}
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 91099ad..224fadc 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -194,7 +194,7 @@ static void write_head_info(void)
struct command {
struct command *next;
- char *error_string;
+ const char *error_string;
unsigned int skip_update:1,
did_not_exist:1;
int index;
@@ -468,7 +468,7 @@ static int update_shallow_ref(struct command *cmd, struct
shallow_info *si)
return 0;
}
-static char *update(struct command *cmd, struct shallow_info *si)
+static const char *update(struct command *cmd, struct
Re: Transaction patch series overview
Hi again,
Junio C Hamano wrote:
It seems that I can hold the topic in 'pu' a bit longer and expect a
reroll from this effort before merging it to 'next'?
Sorry for the delay. The following changes on top of master
(refs.c: change ref_transaction_update() to do error checking and
return status, 2014-07-14) are available in the git repository at
git://repo.or.cz/git/jrn.git tags/rs/ref-transaction-1
for you to fetch changes up to 432ad1f39fd773b37757dd8f10b3075dc3d0d0a2:
refs.c: make delete_ref use a transaction (2014-08-26 14:22:53 -0700)
Use ref transactions, early part
Ronnie explains:
This patch series expands on the transaction API. It converts
ref updates, inside refs.c as well as external, to use the
transaction API for updates. This makes most ref updates
atomic when there are failures locking or writing to a ref.
This series teaches the tag, replace, commit, cherry-pick,
fast-import, checkout -b, branch, receive-pack, and http-fetch
commands and all update_ref and delete_ref callers to use the ref
transaction API instead of lock_ref_sha1.
The main user-visible change should be some cosmetic changes to error
messages. The series also combines multiple ref updates into a single
transaction in 'git http-fetch -w' and when writing tags in
fast-import but otherwise doesn't change the granularity of
transactions.
Reviewed at https://code-review.googlesource.com/#/q/topic:ref-transaction-1
Ronnie Sahlberg (20):
refs.c: change ref_transaction_create to do error checking and return
status
refs.c: update ref_transaction_delete to check for error and return status
refs.c: make ref_transaction_begin take an err argument
refs.c: add transaction.status and track OPEN/CLOSED
tag.c: use ref transactions when doing updates
replace.c: use the ref transaction functions for updates
commit.c: use ref transactions for updates
sequencer.c: use ref transactions for all ref updates
fast-import.c: change update_branch to use ref transactions
branch.c: use ref transaction for all ref updates
refs.c: change update_ref to use a transaction
receive-pack.c: use a reference transaction for updating the refs
fast-import.c: use a ref transaction when dumping tags
walker.c: use ref transaction for ref updates
refs.c: make lock_ref_sha1 static
refs.c: remove the update_ref_lock function
refs.c: remove the update_ref_write function
refs.c: remove lock_ref_sha1
refs.c: make prune_ref use a transaction to delete the ref
refs.c: make delete_ref use a transaction
branch.c | 31 ---
builtin/commit.c | 25 +++--
builtin/receive-pack.c | 25 +++--
builtin/replace.c | 14 +--
builtin/tag.c | 16 ++--
builtin/update-ref.c | 14 ++-
fast-import.c | 54 +++
refs.c | 244 -
refs.h | 77
sequencer.c| 26 --
walker.c | 70 --
11 files changed, 367 insertions(+), 229 deletions(-)
[...]
Jonathan Nieder jrnie...@gmail.com writes:
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -580,18 +580,18 @@ static char *update(struct command *cmd, struct
shallow_info *si)
[...]
if (!transaction ||
ref_transaction_update(transaction, namespaced_name,
new_sha1, old_sha1, 0, 1, err) ||
ref_transaction_commit(transaction, push, err)) {
-char *str = strbuf_detach(err, NULL);
ref_transaction_free(transaction);
-rp_error(%s, str);
-return str;
+rp_error(%s, err.buf);
+strbuf_release(err);
+return failed to update ref;
}
I am guessing that the purpose of this change is to make sure that
cmd-error_string is taken from a fixed set of strings, not an
arbitrary collection of errors from the transaction subsystem, but
what is the significance of doing so? Do the other side i18n while
running print_ref_status() or something?
It's about keeping the message in parentheses on the ! rejected
master - master line short and simple, since the more detailed
diagnosis is redundant next to the full message that is sent over
sideband earlier and gets a line to itself.
Side effects:
* a less invasive patch --- no more need to change the calling
convention to return a dynamically allocated string, with
assertions throughout the file that check for leaks
* using the same all lowercase and brief convention makes the
message less jarring next to other statuses for ! rejected lines,
like non
[PATCH 0/20] rs/ref-transaction-1 (Re: Transaction patch series overview)
Jonathan Nieder wrote: This series teaches the tag, replace, commit, cherry-pick, fast-import, checkout -b, branch, receive-pack, and http-fetch commands and all update_ref and delete_ref callers to use the ref transaction API instead of lock_ref_sha1. The main user-visible change should be some cosmetic changes to error messages. The series also combines multiple ref updates into a single transaction in 'git http-fetch -w' and when writing tags in fast-import but otherwise doesn't change the granularity of transactions. Reviewed at https://code-review.googlesource.com/#/q/topic:ref-transaction-1 Ronnie Sahlberg (20): refs.c: change ref_transaction_create to do error checking and return status refs.c: update ref_transaction_delete to check for error and return status refs.c: make ref_transaction_begin take an err argument refs.c: add transaction.status and track OPEN/CLOSED tag.c: use ref transactions when doing updates replace.c: use the ref transaction functions for updates commit.c: use ref transactions for updates sequencer.c: use ref transactions for all ref updates fast-import.c: change update_branch to use ref transactions branch.c: use ref transaction for all ref updates refs.c: change update_ref to use a transaction receive-pack.c: use a reference transaction for updating the refs fast-import.c: use a ref transaction when dumping tags walker.c: use ref transaction for ref updates refs.c: make lock_ref_sha1 static refs.c: remove the update_ref_lock function refs.c: remove the update_ref_write function refs.c: remove lock_ref_sha1 refs.c: make prune_ref use a transaction to delete the ref refs.c: make delete_ref use a transaction And here are the patches. -- 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 02/20] refs.c: update ref_transaction_delete to check for error and return status
From: Ronnie Sahlberg sahlb...@google.com
Date: Wed, 16 Apr 2014 15:27:45 -0700
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 sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
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 41121fa..7c9c248 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, err))
+ die(%s, err.buf);
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index c49f1c6..40f04f4 100644
--- a/refs.c
+++ b/refs.c
@@ -3469,19 +3469,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 b648819..71389a1 100644
--- a/refs.h
+++ b/refs.h
@@ -308,11 +308,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.
*/
-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.1.0.rc2.206.gedb03e5
--
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 01/20] refs.c: change ref_transaction_create to do error checking and return status
From: Ronnie Sahlberg sahlb...@google.com
Date: Wed, 16 Apr 2014 15:26:44 -0700
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 sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
builtin/update-ref.c | 4 +++-
refs.c | 18 --
refs.h | 48 +---
3 files changed, 56 insertions(+), 14 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 3067b11..41121fa 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, err))
+ die(%s, err.buf);
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index 3f05e88..c49f1c6 100644
--- a/refs.c
+++ b/refs.c
@@ -3449,18 +3449,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 c5376ce..b648819 100644
--- a/refs.h
+++ b/refs.h
@@ -10,6 +10,38 @@ struct ref_lock {
int force_write;
};
+/*
+ * A ref_transaction represents a collection of ref updates
+ * that should succeed or fail together.
+ *
+ * Calling sequence
+ *
+ * - Allocate and initialize a `struct ref_transaction` by calling
+ * `ref_transaction_begin()`.
+ *
+ * - List intended ref updates by calling functions like
+ * `ref_transaction_update()` and `ref_transaction_create()`.
+ *
+ * - Call `ref_transaction_commit()` to execute the transaction.
+ * If this succeeds, the ref updates will have taken place and
+ * the transaction cannot be rolled back.
+ *
+ * - At any time call `ref_transaction_free()` to discard the
+ * transaction and free associated resources. In particular,
+ * this rolls back the transaction if it has not been
+ * successfully committed.
+ *
+ * Error handling
+ * --
+ *
+ * On error, transaction functions append a message about what
+ * went wrong to the 'err' argument. The message mentions what
+ * ref was being updated (if any) when the error occurred so it
+ * can be passed to 'die' or 'error' as-is.
+ *
+ * The message is appended to err without first clearing err.
+ * err will not be '\n' terminated.
+ */
struct ref_transaction;
/*
@@ -248,7 +280,7 @@ struct ref_transaction *ref_transaction_begin(void);
* 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.
+ * rolled back.
*/
int ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
@@ -262,11 +294,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.
*/
-void ref_transaction_create(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *new_sha1,
- int flags);
+int
[PATCH 03/20] refs.c: make ref_transaction_begin take an err argument
From: Ronnie Sahlberg sahlb...@google.com
Date: Mon, 19 May 2014 10:42:34 -0700
Add an err argument to _begin so that on non-fatal failures in future ref
backends we can report a nice error back to the caller.
While _begin can currently never fail for other reasons than OOM, in which
case we die() anyway, we may add other types of backends in the future.
For example, a hypothetical MySQL backend could fail in _begin with
Can not connect to MySQL server. No route to host.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
builtin/update-ref.c | 5 -
refs.c | 2 +-
refs.h | 2 +-
3 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 7c9c248..96a53b9 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -365,7 +365,9 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
die(Refusing to perform update with empty message.);
if (read_stdin) {
- transaction = ref_transaction_begin();
+ transaction = ref_transaction_begin(err);
+ if (!transaction)
+ die(%s, err.buf);
if (delete || no_deref || argc 0)
usage_with_options(git_update_ref_usage, options);
if (end_null)
@@ -374,6 +376,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
if (ref_transaction_commit(transaction, msg, err))
die(%s, err.buf);
ref_transaction_free(transaction);
+ strbuf_release(err);
return 0;
}
diff --git a/refs.c b/refs.c
index 40f04f4..9cb7908 100644
--- a/refs.c
+++ b/refs.c
@@ -3397,7 +3397,7 @@ struct ref_transaction {
size_t nr;
};
-struct ref_transaction *ref_transaction_begin(void)
+struct ref_transaction *ref_transaction_begin(struct strbuf *err)
{
return xcalloc(1, sizeof(struct ref_transaction));
}
diff --git a/refs.h b/refs.h
index 71389a1..3f37c65 100644
--- a/refs.h
+++ b/refs.h
@@ -262,7 +262,7 @@ enum action_on_err {
* Begin a reference transaction. The reference transaction must
* be freed by calling ref_transaction_free().
*/
-struct ref_transaction *ref_transaction_begin(void);
+struct ref_transaction *ref_transaction_begin(struct strbuf *err);
/*
* The following functions add a reference check or update to a
--
2.1.0.rc2.206.gedb03e5
--
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 04/20] refs.c: add transaction.status and track OPEN/CLOSED
From: Ronnie Sahlberg sahlb...@google.com
Date: Tue, 29 Apr 2014 12:06:19 -0700
Track the state of a transaction in a new state field. Check the field for
sanity, i.e. that state must be OPEN when _commit/_create/_delete or
_update is called or else die(BUG:...)
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
refs.c | 34 +-
1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 9cb7908..cc63056 100644
--- a/refs.c
+++ b/refs.c
@@ -3387,6 +3387,21 @@ struct ref_update {
};
/*
+ * Transaction states.
+ * OPEN: The transaction is in a valid state and can accept new updates.
+ * An OPEN transaction can be committed.
+ * CLOSED: A closed transaction is no longer active and no other operations
+ * than free can be used on it in this state.
+ * A transaction can either become closed by successfully committing
+ * an active transaction or if there is a failure while building
+ * the transaction thus rendering it failed/inactive.
+ */
+enum ref_transaction_state {
+ REF_TRANSACTION_OPEN = 0,
+ REF_TRANSACTION_CLOSED = 1
+};
+
+/*
* Data structure for holding a reference transaction, which can
* consist of checks and updates to multiple references, carried out
* as atomically as possible. This structure is opaque to callers.
@@ -3395,6 +3410,7 @@ struct ref_transaction {
struct ref_update **updates;
size_t alloc;
size_t nr;
+ enum ref_transaction_state state;
};
struct ref_transaction *ref_transaction_begin(struct strbuf *err)
@@ -3437,6 +3453,9 @@ int ref_transaction_update(struct ref_transaction
*transaction,
{
struct ref_update *update;
+ if (transaction-state != REF_TRANSACTION_OPEN)
+ die(BUG: update called for transaction that is not open);
+
if (have_old !old_sha1)
die(BUG: have_old is true but old_sha1 is NULL);
@@ -3457,6 +3476,9 @@ int ref_transaction_create(struct ref_transaction
*transaction,
{
struct ref_update *update;
+ if (transaction-state != REF_TRANSACTION_OPEN)
+ die(BUG: create called for transaction that is not open);
+
if (!new_sha1 || is_null_sha1(new_sha1))
die(BUG: create ref with null new_sha1);
@@ -3477,6 +3499,9 @@ int ref_transaction_delete(struct ref_transaction
*transaction,
{
struct ref_update *update;
+ if (transaction-state != REF_TRANSACTION_OPEN)
+ die(BUG: delete called for transaction that is not open);
+
if (have_old !old_sha1)
die(BUG: have_old is true but old_sha1 is NULL);
@@ -3532,8 +3557,13 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
int n = transaction-nr;
struct ref_update **updates = transaction-updates;
- if (!n)
+ if (transaction-state != REF_TRANSACTION_OPEN)
+ die(BUG: commit called for transaction that is not open);
+
+ if (!n) {
+ transaction-state = REF_TRANSACTION_CLOSED;
return 0;
+ }
/* Allocate work space */
delnames = xmalloc(sizeof(*delnames) * n);
@@ -3595,6 +3625,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
clear_loose_ref_cache(ref_cache);
cleanup:
+ transaction-state = REF_TRANSACTION_CLOSED;
+
for (i = 0; i n; i++)
if (updates[i]-lock)
unlock_ref(updates[i]-lock);
--
2.1.0.rc2.206.gedb03e5
--
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 05/20] tag.c: use ref transactions when doing updates
From: Ronnie Sahlberg sahlb...@google.com
Date: Wed, 16 Apr 2014 15:30:41 -0700
Change tag.c to use ref transactions for all ref updates.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
builtin/tag.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index c6e8a71..f3f172f 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,14 +702,17 @@ 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(err);
+ if (!transaction ||
+ ref_transaction_update(transaction, ref.buf, object, prev,
+ 0, 1, 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));
+ strbuf_release(err);
strbuf_release(buf);
strbuf_release(ref);
return 0;
--
2.1.0.rc2.206.gedb03e5
--
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 06/20] replace.c: use the ref transaction functions for updates
From: Ronnie Sahlberg sahlb...@google.com Date: Wed, 16 Apr 2014 15:32:29 -0700 Update replace.c to use ref transactions for updates. Signed-off-by: Ronnie Sahlberg sahlb...@google.com Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- builtin/replace.c | 14 -- 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/builtin/replace.c b/builtin/replace.c index 1bb491d..1fcd06d 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -153,7 +153,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; obj_type = sha1_object_info(object, NULL); repl_type = sha1_object_info(repl, NULL); @@ -166,12 +167,13 @@ static int replace_object_sha1(const char *object_ref, check_ref_valid(object, prev, ref, sizeof(ref), force); - 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(err); + if (!transaction || + ref_transaction_update(transaction, ref, repl, prev, 0, 1, err) || + ref_transaction_commit(transaction, NULL, err)) + die(%s, err.buf); + ref_transaction_free(transaction); return 0; } -- 2.1.0.rc2.206.gedb03e5 -- 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 07/20] commit.c: use ref transactions for updates
From: Ronnie Sahlberg sahlb...@google.com
Date: Wed, 16 Apr 2014 15:34:19 -0700
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.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
builtin/commit.c | 25 -
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 5e2221c..9bf1003 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1627,11 +1627,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);
@@ -1753,16 +1754,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);
@@ -1771,10 +1762,17 @@ 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(err);
+ 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);
}
+ ref_transaction_free(transaction);
unlink(git_path(CHERRY_PICK_HEAD));
unlink(git_path(REVERT_HEAD));
@@ -1803,5 +1801,6 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
if (!quiet)
print_summary(prefix, sha1, !current_head);
+ strbuf_release(err);
return 0;
}
--
2.1.0.rc2.206.gedb03e5
--
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 08/20] sequencer.c: use ref transactions for all ref updates
From: Ronnie Sahlberg sahlb...@google.com
Date: Wed, 16 Apr 2014 15:37:45 -0700
Change to use ref transactions for all updates to refs.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
sequencer.c | 26 ++
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 0a80c58..5e93b6a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -272,23 +272,33 @@ 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(err);
+ if (!transaction ||
+ ref_transaction_update(transaction, HEAD,
+ to, unborn ? null_sha1 : from,
+ 0, 1, err) ||
+ ref_transaction_commit(transaction, sb.buf, err)) {
+ ref_transaction_free(transaction);
+ error(%s, err.buf);
+ strbuf_release(sb);
+ strbuf_release(err);
+ return -1;
+ }
strbuf_release(sb);
- return ret;
+ strbuf_release(err);
+ ref_transaction_free(transaction);
+ return 0;
}
static int do_recursive_merge(struct commit *base, struct commit *next,
--
2.1.0.rc2.206.gedb03e5
--
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 10/20] branch.c: use ref transaction for all ref updates
From: Ronnie Sahlberg sahlb...@google.com
Date: Wed, 16 Apr 2014 16:21:53 -0700
Change create_branch to use a ref transaction when creating the new branch.
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.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
branch.c | 31 +--
1 file changed, 17 insertions(+), 14 deletions(-)
diff --git a/branch.c b/branch.c
index 660097b..37ac555 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,26 @@ 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(err);
+ if (!transaction ||
+ ref_transaction_update(transaction, ref.buf, sha1,
+ null_sha1, 0, !forcing, err) ||
+ ref_transaction_commit(transaction, msg, err))
+ die(%s, err.buf);
+ ref_transaction_free(transaction);
+ strbuf_release(err);
+ }
+
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.1.0.rc2.206.gedb03e5
--
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 09/20] fast-import.c: change update_branch to use ref transactions
From: Ronnie Sahlberg sahlb...@google.com
Date: Wed, 16 Apr 2014 16:21:13 -0700
Change update_branch() to use ref transactions for updates.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
fast-import.c | 25 +++--
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 6707a66..79160d5 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1679,8 +1679,9 @@ 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);
@@ -1689,29 +1690,33 @@ static int update_branch(struct branch *b)
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(err);
+ if (!transaction ||
+ ref_transaction_update(transaction, b-name, b-sha1, old_sha1,
+ 0, 1, err) ||
+ ref_transaction_commit(transaction, msg, err)) {
+ ref_transaction_free(transaction);
+ error(%s, err.buf);
+ strbuf_release(err);
+ return -1;
+ }
+ ref_transaction_free(transaction);
+ strbuf_release(err);
return 0;
}
--
2.1.0.rc2.206.gedb03e5
--
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 11/20] refs.c: change update_ref to use a transaction
From: Ronnie Sahlberg sahlb...@google.com
Date: Thu, 24 Apr 2014 16:36:55 -0700
Change the update_ref helper function to use a ref transaction internally.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
refs.c | 29 +
1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index cc63056..dcc877b 100644
--- a/refs.c
+++ b/refs.c
@@ -3519,11 +3519,32 @@ 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(err);
+ if (!t ||
+ ref_transaction_update(t, refname, sha1, oldval, flags,
+ !!oldval, err) ||
+ ref_transaction_commit(t, action, err)) {
+ const char *str = update_ref failed for ref '%s': %s;
+
+ ref_transaction_free(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);
+ }
+ strbuf_release(err);
+ return 0;
}
static int ref_update_compare(const void *r1, const void *r2)
--
2.1.0.rc2.206.gedb03e5
--
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 12/20] receive-pack.c: use a reference transaction for updating the refs
From: Ronnie Sahlberg sahlb...@google.com
Date: Mon, 28 Apr 2014 14:36:15 -0700
Wrap all the ref updates inside a transaction.
In the new API there is no distinction between failure to lock and
failure to write a ref. Both can be permanent (e.g., a ref
refs/heads/topic is blocking creation of the lock file
refs/heads/topic/1.lock) or transient (e.g., file system full) and
there's no clear difference in how the client should respond, so
replace the two statuses failed to lock and failed to write with
a single status failed to update ref. In both cases a more
detailed message is sent by sideband to diagnose the problem.
Example, before:
error: there are still refs under 'refs/heads/topic'
remote: error: failed to lock refs/heads/topic
To foo
! [remote rejected] HEAD - topic (failed to lock)
After:
error: there are still refs under 'refs/heads/topic'
remote: error: Cannot lock the ref 'refs/heads/topic'.
To foo
! [remote rejected] HEAD - topic (failed to update ref)
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
builtin/receive-pack.c | 25 -
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c323081..224fadc 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -475,7 +475,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)) {
@@ -576,19 +575,27 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
return NULL; /* good */
}
else {
+ struct strbuf err = STRBUF_INIT;
+ struct ref_transaction *transaction;
+
if (shallow_update si-shallow_ref[cmd-index]
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 */
+ transaction = ref_transaction_begin(err);
+ if (!transaction ||
+ ref_transaction_update(transaction, namespaced_name,
+ new_sha1, old_sha1, 0, 1, err) ||
+ ref_transaction_commit(transaction, push, err)) {
+ ref_transaction_free(transaction);
+
+ rp_error(%s, err.buf);
+ strbuf_release(err);
+ return failed to update ref;
}
+
+ ref_transaction_free(transaction);
+ strbuf_release(err);
return NULL; /* good */
}
}
--
2.1.0.rc2.206.gedb03e5
--
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 13/20] fast-import.c: use a ref transaction when dumping tags
From: Ronnie Sahlberg sahlb...@google.com
Date: Mon, 28 Apr 2014 15:23:58 -0700
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Reviewed-by: Michael Haggerty mhag...@alum.mit.edu
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
fast-import.c | 29 +++--
1 file changed, 23 insertions(+), 6 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 79160d5..e7f6e37 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1735,15 +1735,32 @@ 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 ref_name = STRBUF_INIT;
+ struct strbuf err = STRBUF_INIT;
+ struct ref_transaction *transaction;
+ transaction = ref_transaction_begin(err);
+ if (!transaction) {
+ failure |= error(%s, err.buf);
+ goto cleanup;
+ }
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);
+ strbuf_reset(ref_name);
+ strbuf_addf(ref_name, refs/tags/%s, t-name);
+
+ if (ref_transaction_update(transaction, ref_name.buf, t-sha1,
+ NULL, 0, 0, err)) {
+ failure |= error(%s, err.buf);
+ goto cleanup;
+ }
}
+ if (ref_transaction_commit(transaction, msg, err))
+ failure |= error(%s, err.buf);
+
+ cleanup:
+ ref_transaction_free(transaction);
+ strbuf_release(ref_name);
+ strbuf_release(err);
}
static void dump_marks_helper(FILE *f,
--
2.1.0.rc2.206.gedb03e5
--
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 15/20] refs.c: make lock_ref_sha1 static
From: Ronnie Sahlberg sahlb...@google.com
Date: Mon, 28 Apr 2014 15:38:47 -0700
No external callers reference lock_ref_sha1 any more so let's declare it
static.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Reviewed-by: Michael Haggerty mhag...@alum.mit.edu
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
refs.c | 7 +--
refs.h | 6 --
2 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/refs.c b/refs.c
index dcc877b..0dc093c 100644
--- a/refs.c
+++ b/refs.c
@@ -2069,7 +2069,10 @@ int dwim_log(const char *str, int len, unsigned char
*sha1, char **log)
return logs_found;
}
-/* This function should make sure errno is meaningful on error */
+/*
+ * Locks a refs/ ref returning the lock on success and NULL on failure.
+ * On failure errno is set to something meaningful.
+ */
static struct ref_lock *lock_ref_sha1_basic(const char *refname,
const unsigned char *old_sha1,
int flags, int *type_p)
@@ -2170,7 +2173,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 3f37c65..65dd593 100644
--- a/refs.h
+++ b/refs.h
@@ -170,12 +170,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.
- * On failure errno is set to something meaningful.
- */
-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
/* errno is set to something meaningful on failure */
--
2.1.0.rc2.206.gedb03e5
--
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 14/20] walker.c: use ref transaction for ref updates
From: Ronnie Sahlberg sahlb...@google.com
Date: Thu, 17 Apr 2014 11:31:06 -0700
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.
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 collisions that the previous locking would
protect against and cause the fetch to fail for are even more rare.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
walker.c | 72
1 file changed, 41 insertions(+), 31 deletions(-)
diff --git a/walker.c b/walker.c
index 1dd86b8..b8a5441 100644
--- a/walker.c
+++ b/walker.c
@@ -251,40 +251,40 @@ 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 *));
+ struct strbuf refname = STRBUF_INIT;
+ struct strbuf err = STRBUF_INIT;
+ struct ref_transaction *transaction = NULL;
unsigned char *sha1 = xmalloc(targets * 20);
- char *msg;
- int ret;
- int i;
+ char *msg = NULL;
+ int i, ret = -1;
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;
+ if (write_ref) {
+ transaction = ref_transaction_begin(err);
+ if (!transaction) {
+ error(%s, err.buf);
+ goto done;
}
}
-
if (!walker-get_recover)
for_each_ref(mark_complete, NULL);
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 done;
}
if (process(walker, lookup_unknown_object(sha1[20 * i])))
- goto unlock_and_fail;
+ goto done;
}
if (loop(walker))
- goto unlock_and_fail;
-
+ goto done;
+ if (!write_ref) {
+ ret = 0;
+ goto done;
+ }
if (write_ref_log_details) {
msg = xmalloc(strlen(write_ref_log_details) + 12);
sprintf(msg, fetch from %s, write_ref_log_details);
@@ -292,23 +292,33 @@ int walker_fetch(struct walker *walker, int targets, char
**target,
msg = NULL;
}
for (i = 0; i targets; i++) {
- if (!write_ref || !write_ref[i])
+ if (!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;
+ strbuf_reset(refname);
+ strbuf_addf(refname, refs/%s, write_ref[i]);
+ if (ref_transaction_update(transaction, refname.buf,
+ sha1[20 * i], NULL, 0, 0,
+ err)) {
+ error(%s, err.buf);
+ goto done;
+ }
}
+ if (ref_transaction_commit(transaction,
+ msg ? msg : fetch (unknown),
+ err)) {
+ error(%s, err.buf);
+ goto done;
+ }
+
+ ret = 0;
+
+done:
+ ref_transaction_free(transaction);
free(msg);
-
- return 0;
-
-unlock_and_fail:
- for (i = 0; i targets; i++)
- if (lock[i])
- unlock_ref(lock[i]);
-
- return -1;
+ free(sha1);
+ strbuf_release(err);
+ strbuf_release(refname);
+ return ret;
}
void walker_free(struct walker *walker)
--
2.1.0.rc2.206.gedb03e5
--
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 17/20] refs.c: remove the update_ref_write function
From: Ronnie Sahlberg sahlb...@google.com
Date: Tue, 29 Apr 2014 13:42:07 -0700
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. This changes the return status for _commit from
1 to -1 on failures when writing to the ref. Eventually we will want
_commit to start returning more detailed error conditions than the current
simple success/failure.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
refs.c | 34 --
1 file changed, 8 insertions(+), 26 deletions(-)
diff --git a/refs.c b/refs.c
index de07791..ef7660a 100644
--- a/refs.c
+++ b/refs.c
@@ -3336,25 +3336,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
@@ -3604,14 +3585,15 @@ 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) {
+ if (err)
+ strbuf_addf(err, Cannot update the ref
'%s'.,
+ update-refname);
goto cleanup;
+ }
}
}
--
2.1.0.rc2.206.gedb03e5
--
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 16/20] refs.c: remove the update_ref_lock function
From: Ronnie Sahlberg sahlb...@google.com
Date: Tue, 29 Apr 2014 12:14:47 -0700
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 sahlb...@google.com
Reviewed-by: Michael Haggerty mhag...@alum.mit.edu
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
refs.c | 30 ++
1 file changed, 6 insertions(+), 24 deletions(-)
diff --git a/refs.c b/refs.c
index 0dc093c..de07791 100644
--- a/refs.c
+++ b/refs.c
@@ -3336,24 +3336,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)
@@ -3602,12 +3584,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.1.0.rc2.206.gedb03e5
--
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 18/20] refs.c: remove lock_ref_sha1
From: Ronnie Sahlberg sahlb...@google.com
Date: Tue, 29 Apr 2014 15:45:52 -0700
lock_ref_sha1 was only called from one place in refs.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 sahlb...@google.com
Reviewed-by: Michael Haggerty mhag...@alum.mit.edu
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
refs.c | 15 +--
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/refs.c b/refs.c
index ef7660a..f0883d0 100644
--- a/refs.c
+++ b/refs.c
@@ -2173,15 +2173,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)
@@ -2391,8 +2382,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.1.0.rc2.206.gedb03e5
--
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 19/20] refs.c: make prune_ref use a transaction to delete the ref
From: Ronnie Sahlberg sahlb...@google.com
Date: Wed, 30 Apr 2014 09:03:36 -0700
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 sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
refs.c | 28 +---
refs.h | 13 +++--
2 files changed, 32 insertions(+), 9 deletions(-)
diff --git a/refs.c b/refs.c
index f0883d0..5b2d335 100644
--- a/refs.c
+++ b/refs.c
@@ -25,6 +25,11 @@ static unsigned char refname_disposition[256] = {
};
/*
+ * Used as a flag to ref_transaction_delete when a loose ref is being
+ * 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 legal. It is legal if it is something reasonable to have under
@@ -2382,17 +2387,25 @@ 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(err);
+ if (!transaction ||
+ ref_transaction_delete(transaction, r-name, r-sha1,
+ REF_ISPRUNING, 1, err) ||
+ ref_transaction_commit(transaction, NULL, err)) {
+ ref_transaction_free(transaction);
+ error(%s, err.buf);
+ strbuf_release(err);
+ return;
}
+ ref_transaction_free(transaction);
+ strbuf_release(err);
+ try_remove_empty_parents(r-name);
}
static void prune_refs(struct ref_to_prune *r)
@@ -3597,8 +3610,9 @@ 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);
+ if (!(update-flags REF_ISPRUNING))
+ delnames[delnum++] = update-lock-ref_name;
}
}
diff --git a/refs.h b/refs.h
index 65dd593..69ef28c 100644
--- a/refs.h
+++ b/refs.h
@@ -170,9 +170,18 @@ extern int ref_exists(const char *);
*/
extern int peel_ref(const char *refname, unsigned char *sha1);
-/** Locks any ref (for 'HEAD' type refs). */
+/*
+ * Flags controlling lock_any_ref_for_update(), ref_transaction_update(),
+ * ref_transaction_create(), etc.
+ * REF_NODEREF: act on the ref directly, instead of dereferencing
+ * symbolic references.
+ *
+ * Flags = 0x100 are reserved for internal use.
+ */
#define REF_NODEREF0x01
-/* errno is set to something meaningful on failure */
+/*
+ * This function sets errno to something meaningful on failure.
+ */
extern struct ref_lock *lock_any_ref_for_update(const char *refname,
const unsigned char *old_sha1,
int flags, int *type_p);
--
2.1.0.rc2.206.gedb03e5
--
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 20/20] refs.c: make delete_ref use a transaction
From: Ronnie Sahlberg sahlb...@google.com
Date: Wed, 30 Apr 2014 09:22:45 -0700
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.
Change delete_ref to return 0 on success and 1 on failure instead of the
previous 0 on success either 1 or -1 on failure.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Reviewed-by: Michael Haggerty mhag...@alum.mit.edu
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
Thanks for reading.
refs.c | 35 ++-
1 file changed, 14 insertions(+), 21 deletions(-)
diff --git a/refs.c b/refs.c
index 5b2d335..7996be9 100644
--- a/refs.c
+++ b/refs.c
@@ -2548,11 +2548,6 @@ 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 delete_ref_loose(struct ref_lock *lock, int flag)
{
if (!(flag REF_ISPACKED) || flag REF_ISSYMREF) {
@@ -2570,24 +2565,22 @@ static int delete_ref_loose(struct ref_lock *lock, int
flag)
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;
+ struct strbuf err = STRBUF_INIT;
- lock = lock_ref_sha1_basic(refname, sha1, delopt, flag);
- if (!lock)
+ transaction = ref_transaction_begin(err);
+ if (!transaction ||
+ ref_transaction_delete(transaction, refname, sha1, delopt,
+ sha1 !is_null_sha1(sha1), err) ||
+ ref_transaction_commit(transaction, NULL, err)) {
+ error(%s, err.buf);
+ ref_transaction_free(transaction);
+ strbuf_release(err);
return 1;
- ret |= delete_ref_loose(lock, flag);
-
- /* 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);
+ strbuf_release(err);
+ return 0;
}
/*
--
2.1.0.rc2.206.gedb03e5
--
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: problem with def of inet_ntop() in git-compat-util.h as well as other places
Hi, dev wrote: CC credential-store.o git-compat-util.h, line 516: error: identifier redeclared: inet_ntop current : function(int, pointer to const void, pointer to char, unsigned long) returning pointer to const char previous: function(int, pointer to const void, pointer to char, unsigned int) returning pointer to const char : /usr/include/arpa/inet.h, line 68 Why is NO_INET_NTOP set? What commands are you using to build? If you are using autoconf, what does your config.mak.autogen say? Puzzled, 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] Document LF appearing in shallow command during send-pack/receive-pack
Shawn Pearce wrote: The implementation sends an LF, but the protocol documentation was missing this detail. Signed-off-by: Shawn Pearce spea...@spearce.org --- Documentation/technical/pack-protocol.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt index 18dea8d..569c48a 100644 --- a/Documentation/technical/pack-protocol.txt +++ b/Documentation/technical/pack-protocol.txt @@ -467,7 +467,7 @@ references. update-request= *shallow command-list [pack-file] - shallow = PKT-LINE(shallow SP obj-id) + shallow = PKT-LINE(shallow SP obj-id LF) In git, the client sends LF and the server is happy with or without LF. JGit and libgit2 don't send 'shallow' lines. Reviewed-by: Jonathan Nieder jrnie...@gmail.com command-list = PKT-LINE(command NUL capability-list LF) *PKT-LINE(command LF) -- -- 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: problem with def of inet_ntop() in git-compat-util.h as well as other places
Hi again, dev wrote: So I guess I have to create a config.mak file from somewhere. Sorry, let's take a step back. What exact commands do you use to build, starting from a pristine extracted tarball? What output do you get back? [...] Undefined first referenced symbol in file libiconv_close libgit.a(utf8.o) (symbol belongs to implicit dependency /usr/local/lib/libiconv.so.2) Sounds like NEEDS_LIBICONV should be set on Solaris. You can test this by passing NEEDS_LIBICONV=YesPlease on the gmake command line and seeing what happens. But it seems odd --- was iconv once part of libc on Solaris and then moved out or something? There have been lots of people building git on Solaris over the years (and writing patches to fix other build problems) without needing to set that flag. [...] You would potentially also need to turn off a few feature flags manually The options for tweaking the build are described at the top of the Makefile. Thanks again, 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: revert top most commit
Keller, Jacob E wrote: On Wed, 2014-08-27 at 21:14 +, Keller, Jacob E wrote: I am having trouble using revert. If I attempt to $ git revert sha1id where sha1id is the same as the HEAD commit, I instead get the output of what looks like git status. [...] It's actually not my repository, I was helping a co-worker, and thought I'd ask if anyone here knew if it was expected behavior or not. More details about the output would help --- otherwise people have to guess[*]. I'm guessing your co-worker's working tree is not clean. Commiting or stashing their changes first might get things working. Hope that helps, Jonathan [*] Another nice thing about quoting output is that it becomes more obvious what about it wasn't helpful, which sometimes leads to patches from kind people on the list to fix it. -- 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] merge, pull: stop advising 'commit -a' in case of conflict
Matthieu Moy wrote: Signed-off-by: Matthieu Moy matthieu@imag.fr [...] --- advice.c| 3 +-- git-pull.sh | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) Thanks for taking it on. Reviewed-by: Jonathan Nieder jrnie...@gmail.com [...] It was already on my todo-list, as a friend of mine semi-beginner with Git complained about the mis-advice the other day, and I had to agree. That's a useful sort of thing to put in a commit message. :) Eventually, git could detect that conflicts have been resolved, but then that would be a different message, as not only use git commit -a could be resurected, but Fix them up in the work tree should be dropped when it is the case. As is this --- when I wonder why code isn't a certain way, ideas for future work found in the description for the blamed commit are often helpful in explaining the current state and saving me from blind alleys in changing it. Anyway, this is already a very good change as-is. Actually, I'd be nervous about suggesting use git commit -a without at least also saying inspect the result or run tests in the no-conflict-markers-found case. Rerere sometimes makes mistakes, and the result of picking one side when merging binary files can be even worse. 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 01/22] refs.c: change ref_transaction_create to do error checking and return status
From: Ronnie Sahlberg sahlb...@google.com
Date: Wed, 16 Apr 2014 15:26:44 -0700
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 sahlb...@google.com
Reviewed-by: Michael Haggerty mhag...@alum.mit.edu
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
builtin/update-ref.c | 4 +++-
refs.c | 18 --
refs.h | 48 +---
3 files changed, 56 insertions(+), 14 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 3067b11..41121fa 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, err))
+ die(%s, err.buf);
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index 3f05e88..c49f1c6 100644
--- a/refs.c
+++ b/refs.c
@@ -3449,18 +3449,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 c5376ce..b648819 100644
--- a/refs.h
+++ b/refs.h
@@ -10,6 +10,38 @@ struct ref_lock {
int force_write;
};
+/*
+ * A ref_transaction represents a collection of ref updates
+ * that should succeed or fail together.
+ *
+ * Calling sequence
+ *
+ * - Allocate and initialize a `struct ref_transaction` by calling
+ * `ref_transaction_begin()`.
+ *
+ * - List intended ref updates by calling functions like
+ * `ref_transaction_update()` and `ref_transaction_create()`.
+ *
+ * - Call `ref_transaction_commit()` to execute the transaction.
+ * If this succeeds, the ref updates will have taken place and
+ * the transaction cannot be rolled back.
+ *
+ * - At any time call `ref_transaction_free()` to discard the
+ * transaction and free associated resources. In particular,
+ * this rolls back the transaction if it has not been
+ * successfully committed.
+ *
+ * Error handling
+ * --
+ *
+ * On error, transaction functions append a message about what
+ * went wrong to the 'err' argument. The message mentions what
+ * ref was being updated (if any) when the error occurred so it
+ * can be passed to 'die' or 'error' as-is.
+ *
+ * The message is appended to err without first clearing err.
+ * err will not be '\n' terminated.
+ */
struct ref_transaction;
/*
@@ -248,7 +280,7 @@ struct ref_transaction *ref_transaction_begin(void);
* 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.
+ * rolled back.
*/
int ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
@@ -262,11 +294,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.
*/
-void ref_transaction_create(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *new_sha1
[PATCH 22/22] update-ref --stdin: pass transaction around explicitly
This makes it more obvious at a glance where the output of functions
parsing the --stdin stream goes.
No functional change intended.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Reviewed-by: Michael Haggerty mhag...@alum.mit.edu
---
Thanks for reading.
builtin/update-ref.c | 27 +++
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 866bbee..54a48c0 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -12,8 +12,6 @@ static const char * const git_update_ref_usage[] = {
NULL
};
-static struct ref_transaction *transaction;
-
static char line_termination = '\n';
static int update_flags;
@@ -176,7 +174,8 @@ static int parse_next_sha1(struct strbuf *input, const char
**next,
* depending on how line_termination is set.
*/
-static const char *parse_cmd_update(struct strbuf *input, const char *next)
+static const char *parse_cmd_update(struct ref_transaction *transaction,
+ struct strbuf *input, const char *next)
{
struct strbuf err = STRBUF_INIT;
char *refname;
@@ -209,7 +208,8 @@ static const char *parse_cmd_update(struct strbuf *input,
const char *next)
return next;
}
-static const char *parse_cmd_create(struct strbuf *input, const char *next)
+static const char *parse_cmd_create(struct ref_transaction *transaction,
+ struct strbuf *input, const char *next)
{
struct strbuf err = STRBUF_INIT;
char *refname;
@@ -239,7 +239,8 @@ static const char *parse_cmd_create(struct strbuf *input,
const char *next)
return next;
}
-static const char *parse_cmd_delete(struct strbuf *input, const char *next)
+static const char *parse_cmd_delete(struct ref_transaction *transaction,
+ struct strbuf *input, const char *next)
{
struct strbuf err = STRBUF_INIT;
char *refname;
@@ -273,7 +274,8 @@ static const char *parse_cmd_delete(struct strbuf *input,
const char *next)
return next;
}
-static const char *parse_cmd_verify(struct strbuf *input, const char *next)
+static const char *parse_cmd_verify(struct ref_transaction *transaction,
+ struct strbuf *input, const char *next)
{
struct strbuf err = STRBUF_INIT;
char *refname;
@@ -317,7 +319,7 @@ static const char *parse_cmd_option(struct strbuf *input,
const char *next)
return next + 8;
}
-static void update_refs_stdin(void)
+static void update_refs_stdin(struct ref_transaction *transaction)
{
struct strbuf input = STRBUF_INIT;
const char *next;
@@ -332,13 +334,13 @@ static void update_refs_stdin(void)
else if (isspace(*next))
die(whitespace before command: %s, next);
else if (starts_with(next, update ))
- next = parse_cmd_update(input, next + 7);
+ next = parse_cmd_update(transaction, input, next + 7);
else if (starts_with(next, create ))
- next = parse_cmd_create(input, next + 7);
+ next = parse_cmd_create(transaction, input, next + 7);
else if (starts_with(next, delete ))
- next = parse_cmd_delete(input, next + 7);
+ next = parse_cmd_delete(transaction, input, next + 7);
else if (starts_with(next, verify ))
- next = parse_cmd_verify(input, next + 7);
+ next = parse_cmd_verify(transaction, input, next + 7);
else if (starts_with(next, option ))
next = parse_cmd_option(input, next + 7);
else
@@ -373,6 +375,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
if (read_stdin) {
struct strbuf err = STRBUF_INIT;
+ struct ref_transaction *transaction;
transaction = ref_transaction_begin(err);
if (!transaction)
@@ -381,7 +384,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
usage_with_options(git_update_ref_usage, options);
if (end_null)
line_termination = '\0';
- update_refs_stdin();
+ update_refs_stdin(transaction);
if (ref_transaction_commit(transaction, msg, err))
die(%s, err.buf);
ref_transaction_free(transaction);
--
2.1.0.rc2.206.gedb03e5
--
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: [RFC PATCH v2 1/2] Makefile: add check-headers target
Hi, David Aguilar wrote: --- /dev/null +++ b/check-headers.sh @@ -0,0 +1,29 @@ [...] + $@ -Wno-unused -I$subdir -c -o $header.check -x c - $header All .c files in git are supposed to start by #include-ing git-compat-util.h, cache.h, or builtin.h to set the appropriate feature test macros and include system headers. Headers rely on that for basic types like int32_t. They don't need to include git-compat-util.h because the .c file that included them would have already, and .h files #include-ed by git-compat-util.h especially *shouldn't* #include the compat header, so how about something like the following for squashing in? A side-thought: as long as we're building pre-compiled headers, could we use them in the build? Thanks, Jonathan diff --git a/check-headers.sh b/check-headers.sh index bf85c41..08ca136 100755 --- a/check-headers.sh +++ b/check-headers.sh @@ -18,7 +18,7 @@ git ls-files *.h | while read header do echo HEADER $header - $@ -Wno-unused -x c -c -o $header.bin - $header + $@ -Wno-unused -x c -include git-compat-util.h -c -o $header.bin - $header rm $header.bin || maybe_exit $? 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: [RFC PATCH v2 2/2] headers: include dependent headers
David Aguilar wrote:
Add dependent headers so that including a header does not
require including additional headers.
I agree with this goal, modulo the compat-util.h caveat. Thanks
for working on it.
[...]
--- a/archive.h
+++ b/archive.h
@@ -1,6 +1,7 @@
#ifndef ARCHIVE_H
#define ARCHIVE_H
+#include cache.h
#include pathspec.h
struct archiver_args {
I'm less happy about the way of achieving that goal. Here's an
alternative. Advantages:
* (fully expanded) headers stay small
* fewer other headers included as a side-effect of including one
header, so callers are more likely to remember to #include the
headers defining things they need (which makes later refactoring
easier)
* circular header dependencies are harder to produce
If this seems like a good direction to go in, I can finish the patch
later today (or I don't mind if someone else takes care of it). This
is just to give the idea --- I stopped at dir.h.
Sensible?
Jonathan
diff --git i/archive.h w/archive.h
index 4a791e1..11f4d42 100644
--- i/archive.h
+++ w/archive.h
@@ -3,6 +3,9 @@
#include pathspec.h
+enum object_type;
+
+
struct archiver_args {
const char *base;
size_t baselen;
diff --git i/attr.h w/attr.h
index 8b08d33..c971ef2 100644
--- i/attr.h
+++ w/attr.h
@@ -1,6 +1,8 @@
#ifndef ATTR_H
#define ATTR_H
+struct index_state;
+
/* An attribute is a pointer to this opaque structure */
struct git_attr;
diff --git i/branch.h w/branch.h
index 64173ab..ed63209 100644
--- i/branch.h
+++ w/branch.h
@@ -1,6 +1,9 @@
#ifndef BRANCH_H
#define BRANCH_H
+enum branch_track;
+struct strbuf;
+
/* Functions for acting on the information about branches. */
/*
diff --git i/cache-tree.h w/cache-tree.h
index b47ccec..c22e2ec 100644
--- i/cache-tree.h
+++ w/cache-tree.h
@@ -1,8 +1,12 @@
#ifndef CACHE_TREE_H
#define CACHE_TREE_H
-#include tree.h
-#include tree-walk.h
+struct traverse_info;
+struct index_state;
+struct name_entry;
+struct tree;
+struct string_list;
+struct strbuf;
struct cache_tree;
struct cache_tree_sub {
diff --git i/column.h w/column.h
index 0a61917..8211386 100644
--- i/column.h
+++ w/column.h
@@ -1,6 +1,9 @@
#ifndef COLUMN_H
#define COLUMN_H
+struct option;
+struct string_list;
+
#define COL_LAYOUT_MASK 0x000F
#define COL_ENABLE_MASK 0x0030 /* always, never or auto */
#define COL_PARSEOPT 0x0040 /* --column is given from cmdline */
@@ -26,7 +29,6 @@ struct column_options {
const char *nl;
};
-struct option;
extern int parseopt_column_callback(const struct option *, const char *, int);
extern int git_column_config(const char *var, const char *value,
const char *command, unsigned int *colopts);
diff --git i/commit.h w/commit.h
index aa8c3ca..d2fd182 100644
--- i/commit.h
+++ w/commit.h
@@ -1,13 +1,18 @@
#ifndef COMMIT_H
#define COMMIT_H
+#include cache.h
#include object.h
-#include tree.h
-#include strbuf.h
-#include decorate.h
-#include gpg-interface.h
+#include trace.h
#include string-list.h
+struct reflog_walk_info;
+struct rev_info;
+struct ref;
+struct signature_check;
+struct sha1_array;
+struct strbuf;
+
struct commit_list {
struct commit *item;
struct commit_list *next;
@@ -151,7 +156,6 @@ struct userformat_want {
};
extern int has_non_ascii(const char *text);
-struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */
extern const char *logmsg_reencode(const struct commit *commit,
char **commit_encoding,
const char *output_encoding);
@@ -231,8 +235,6 @@ extern struct commit_list *get_octopus_merge_bases(struct
commit_list *in);
/* largest positive number a signed 32-bit integer can contain */
#define INFINITE_DEPTH 0x7fff
-struct sha1_array;
-struct ref;
extern int register_shallow(const unsigned char *sha1);
extern int unregister_shallow(const unsigned char *sha1);
extern int for_each_commit_graft(each_commit_graft_fn, void *);
diff --git i/convert.h w/convert.h
index 0c2143c..e623527 100644
--- i/convert.h
+++ w/convert.h
@@ -4,6 +4,8 @@
#ifndef CONVERT_H
#define CONVERT_H
+struct strbuf;
+
enum safe_crlf {
SAFE_CRLF_FALSE = 0,
SAFE_CRLF_FAIL = 1,
diff --git i/csum-file.h w/csum-file.h
index bb543d5..9e29e35 100644
--- i/csum-file.h
+++ w/csum-file.h
@@ -1,6 +1,8 @@
#ifndef CSUM_FILE_H
#define CSUM_FILE_H
+#include cache.h
+
struct progress;
/* A SHA1-protected file */
diff --git i/diffcore.h w/diffcore.h
index c876dac..96fc827 100644
--- i/diffcore.h
+++ w/diffcore.h
@@ -4,6 +4,9 @@
#ifndef DIFFCORE_H
#define DIFFCORE_H
+struct userdiff_driver;
+struct diff_options;
+
/* This header file is internal between diff.c and its diff transformers
* (e.g. diffcore-rename, diffcore-pickaxe). Never include this header
* in anything else.
@@ -22,8 +25,6 @@
#define MINIMUM_BREAK_SIZE 400 /* do not break a file smaller than this */
Re: [RFC PATCH v2 2/2] headers: include dependent headers
Johannes Sixt wrote:
Am 07.09.2014 21:49, schrieb Jonathan Nieder:
+enum object_type;
Enum forward declarations are a relatively new C feature. They certainly
don't exist pre-C99.
Good catch. That makes
diff --git i/archive.h w/archive.h
index 4a791e1..b2ca5bf 100644
--- i/archive.h
+++ w/archive.h
@@ -1,6 +1,7 @@
#ifndef ARCHIVE_H
#define ARCHIVE_H
+#include cache.h
#include pathspec.h
struct archiver_args {
diff --git i/attr.h w/attr.h
index 8b08d33..c971ef2 100644
--- i/attr.h
+++ w/attr.h
@@ -1,6 +1,8 @@
#ifndef ATTR_H
#define ATTR_H
+struct index_state;
+
/* An attribute is a pointer to this opaque structure */
struct git_attr;
diff --git i/branch.h w/branch.h
index 64173ab..5ce6e21 100644
--- i/branch.h
+++ w/branch.h
@@ -1,6 +1,8 @@
#ifndef BRANCH_H
#define BRANCH_H
+#include cache.h
+
/* Functions for acting on the information about branches. */
/*
diff --git i/cache-tree.h w/cache-tree.h
index b47ccec..c22e2ec 100644
--- i/cache-tree.h
+++ w/cache-tree.h
@@ -1,8 +1,12 @@
#ifndef CACHE_TREE_H
#define CACHE_TREE_H
-#include tree.h
-#include tree-walk.h
+struct traverse_info;
+struct index_state;
+struct name_entry;
+struct tree;
+struct string_list;
+struct strbuf;
struct cache_tree;
struct cache_tree_sub {
diff --git i/column.h w/column.h
index 0a61917..8211386 100644
--- i/column.h
+++ w/column.h
@@ -1,6 +1,9 @@
#ifndef COLUMN_H
#define COLUMN_H
+struct option;
+struct string_list;
+
#define COL_LAYOUT_MASK 0x000F
#define COL_ENABLE_MASK 0x0030 /* always, never or auto */
#define COL_PARSEOPT 0x0040 /* --column is given from cmdline */
@@ -26,7 +29,6 @@ struct column_options {
const char *nl;
};
-struct option;
extern int parseopt_column_callback(const struct option *, const char *, int);
extern int git_column_config(const char *var, const char *value,
const char *command, unsigned int *colopts);
diff --git i/commit.h w/commit.h
index aa8c3ca..097fc9e 100644
--- i/commit.h
+++ w/commit.h
@@ -1,13 +1,17 @@
#ifndef COMMIT_H
#define COMMIT_H
+#include cache.h
#include object.h
-#include tree.h
-#include strbuf.h
-#include decorate.h
-#include gpg-interface.h
+#include trace.h
#include string-list.h
+struct reflog_walk_info;
+struct rev_info;
+struct ref;
+struct signature_check;
+struct sha1_array;
+
struct commit_list {
struct commit *item;
struct commit_list *next;
@@ -151,7 +155,6 @@ struct userformat_want {
};
extern int has_non_ascii(const char *text);
-struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */
extern const char *logmsg_reencode(const struct commit *commit,
char **commit_encoding,
const char *output_encoding);
@@ -231,8 +234,6 @@ extern struct commit_list *get_octopus_merge_bases(struct
commit_list *in);
/* largest positive number a signed 32-bit integer can contain */
#define INFINITE_DEPTH 0x7fff
-struct sha1_array;
-struct ref;
extern int register_shallow(const unsigned char *sha1);
extern int unregister_shallow(const unsigned char *sha1);
extern int for_each_commit_graft(each_commit_graft_fn, void *);
diff --git i/convert.h w/convert.h
index 0c2143c..e623527 100644
--- i/convert.h
+++ w/convert.h
@@ -4,6 +4,8 @@
#ifndef CONVERT_H
#define CONVERT_H
+struct strbuf;
+
enum safe_crlf {
SAFE_CRLF_FALSE = 0,
SAFE_CRLF_FAIL = 1,
diff --git i/csum-file.h w/csum-file.h
index bb543d5..9e29e35 100644
--- i/csum-file.h
+++ w/csum-file.h
@@ -1,6 +1,8 @@
#ifndef CSUM_FILE_H
#define CSUM_FILE_H
+#include cache.h
+
struct progress;
/* A SHA1-protected file */
diff --git i/diffcore.h w/diffcore.h
index c876dac..96fc827 100644
--- i/diffcore.h
+++ w/diffcore.h
@@ -4,6 +4,9 @@
#ifndef DIFFCORE_H
#define DIFFCORE_H
+struct userdiff_driver;
+struct diff_options;
+
/* This header file is internal between diff.c and its diff transformers
* (e.g. diffcore-rename, diffcore-pickaxe). Never include this header
* in anything else.
@@ -22,8 +25,6 @@
#define MINIMUM_BREAK_SIZE 400 /* do not break a file smaller than this */
-struct userdiff_driver;
-
struct diff_filespec {
unsigned char sha1[20];
char *path;
diff --git i/object.h w/object.h
index 5e8d8ee..e61b290 100644
--- i/object.h
+++ w/object.h
@@ -1,6 +1,8 @@
#ifndef OBJECT_H
#define OBJECT_H
+#include cache.h
+
struct object_list {
struct object *item;
struct object_list *next;
diff --git i/tree-walk.h w/tree-walk.h
index ae7fb3a..d7612cf 100644
--- i/tree-walk.h
+++ w/tree-walk.h
@@ -1,6 +1,8 @@
#ifndef TREE_WALK_H
#define TREE_WALK_H
+struct strbuf;
+
struct name_entry {
const unsigned char *sha1;
const char *path;
diff --git i/tree.h w/tree.h
index d84ac63..ef84153 100644
--- i/tree.h
+++ w/tree.h
@@ -3,6 +3,9 @@
#include object.h
+struct pathspec;
+
+
extern const char *tree_type;
struct tree
Re: [PATCH] fsck: exit with non-zero status upon error from fsck_obj()
Junio C Hamano wrote: By the way, Jonathan, with dbedf8bf (t1450 (fsck): remove dangling objects, 2010-09-06) you added a 'test_might_fail git fsck' to the 1450 test that catches an object corruption. Do you remember if there was some flakiness in this test that necessitated it, or is it merely I think this should fail, but it does not, and we may fix it some day but I am not doing that in this patch? Thomas is the person to ask. :) See v1.6.3-rc0~176^2~3 (Test fsck a bit harder, 2009-02-19): + (git fsck 2out; true) which that cleanup tightened to test_might_fail. But yes, I'm pretty sure it was for futureproofing, not for hiding flakiness. I think your patch does the right thing in changing it to test_must_fail now that fsck exits nonzero. 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 v21 0/19] rs/ref-transaction (Re: Transaction patch series overview)
Jonathan Nieder wrote: The next series from Ronnie's collection is available at https://code-review.googlesource.com/#/q/topic:ref-transaction in case someone wants a fresh series to look at. Here is the outcome of that review. It could use another set of eyes (hint, hint) but should be mostly ready. Interdiff below. This is meant to replace rs/ref-transaction in 'pu' and I'd prefer to wait a little for more comments before merging to 'next'. These patches are also available from the git repository at git://repo.or.cz/git/jrn.git tags/rs/ref-transaction Use ref transactions, part 3 Ronnie explains: This is the third and final part of the original 48 patch series for basic transaction support. This version implements some changes suggested by mhagger for the warn_if_unremovable changes. It also adds a new patch fix handling of badly named refs that repairs the handling of badly named refs. This includes some improvements to the transaction API (in particular allowing different reflog messages per ref update in a transaction), some cleanups and consistency improvements, and preparation for an implementation of ref renaming in terms of the transaction API. It also improves handling of refs with invalid names. Today git branch --list and git for-each-ref do not provide a way to discover refs with invalid names and git branch -d and git update-ref -d cannot delete them. After this series, such bad refs will be discoverable and deletable again as in olden times. Jonathan Nieder (5): mv test: recreate mod/ directory instead of relying on stale copy branch -d: avoid repeated symref resolution refs.c: do not permit err == NULL lockfile: remove unable_to_lock_error ref_transaction_commit: bail out on failure to remove a ref Ronnie Sahlberg (14): wrapper.c: remove/unlink_or_warn: simplify, treat ENOENT as success wrapper.c: add a new function unlink_or_msg refs.c: add an err argument to delete_ref_loose refs.c: pass the ref log message to _create/delete/update instead of _commit rename_ref: don't ask read_ref_full where the ref came from refs.c: move the check for valid refname to lock_ref_sha1_basic refs.c: call lock_ref_sha1_basic directly from commit refs.c: pass a skip list to name_conflict_fn refs.c: ref_transaction_commit: distinguish name conflicts from other errors fetch.c: change s_update_ref to use a ref transaction refs.c: make write_ref_sha1 static refs.c: change resolve_ref_unsafe reading argument to be a flags field refs.c: fix handling of badly named refs for-each-ref.c: improve message before aborting on broken ref branch.c| 6 +- builtin/blame.c | 2 +- builtin/branch.c| 19 ++- builtin/checkout.c | 6 +- builtin/clone.c | 2 +- builtin/commit.c| 6 +- builtin/fetch.c | 34 +++-- builtin/fmt-merge-msg.c | 2 +- builtin/for-each-ref.c | 12 +- builtin/fsck.c | 2 +- builtin/log.c | 3 +- builtin/merge.c | 2 +- builtin/notes.c | 2 +- builtin/receive-pack.c | 9 +- builtin/remote.c| 5 +- builtin/replace.c | 5 +- builtin/show-branch.c | 6 +- builtin/symbolic-ref.c | 2 +- builtin/tag.c | 4 +- builtin/update-ref.c| 16 ++- bundle.c| 2 +- cache.h | 31 +++-- fast-import.c | 8 +- git-compat-util.h | 16 ++- http-backend.c | 3 +- lockfile.c | 10 -- notes-merge.c | 2 +- reflog-walk.c | 5 +- refs.c | 321 ++-- refs.h | 18 ++- remote.c| 10 +- sequencer.c | 8 +- t/t1400-update-ref.sh | 16 +++ t/t1402-check-ref-format.sh | 48 +++ t/t3200-branch.sh | 9 ++ t/t7001-mv.sh | 15 ++- transport-helper.c | 4 +- transport.c | 5 +- upload-pack.c | 2 +- walker.c| 5 +- wrapper.c | 28 ++-- wt-status.c | 2 +- 42 files changed, 487 insertions(+), 226 deletions(-) --- Changes since last appearance: diff --git c/branch.c w/branch.c index 76a8ec9..ba3e1c1 100644 --- c/branch.c +++ w/branch.c @@ -186,7 +186,7 @@ int validate_new_branchname(const char *name, struct strbuf *ref, const char *head; unsigned char sha1[20]; - head = resolve_ref_unsafe(HEAD, sha1, 0, NULL); + head = resolve_ref_unsafe(HEAD, sha1, NULL, 0); if (!is_bare_repository() head !strcmp(head, ref-buf)) die(_(Cannot force update the current branch.)); } diff --git c/builtin/blame.c w
[PATCH 01/19] mv test: recreate mod/ directory instead of relying on stale copy
The tests for 'git mv moves a submodule' functionality often run commands like git mv sub mod/sub to move a submodule into a subdirectory. Just like plain /bin/mv, this is supposed to succeed if the mod/ parent directory exists and fail if it doesn't exist. Usually these tests mkdir the parent directory beforehand, but some instead rely on it being left behind by previous tests. More precisely, when 'git reset --hard' tries to move to a state where mod/sub is not present any more, it would perform the following operations: rmdir(mod/sub) rmdir(mod) The first fails with ENOENT because the test script removed mod/sub with rm -rf already, so 'reset --hard' doesn't bother to move on to the second, and the mod/ directory is kept around. Better to explicitly remove and re-create the mod/ directory so later tests don't have to depend on the directory left behind by the earlier ones at all (making it easier to rearrange or skip some tests in the file or to tweak 'reset --hard' behavior without breaking unrelated tests). Noticed while testing a patch that fixes the reset --hard behavior described above. Signed-off-by: Jonathan Nieder jrnie...@gmail.com Reviewed-by: Ronnie Sahlberg sahlb...@google.com --- t/t7001-mv.sh | 15 ++- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh index 54d7807..69f11bd 100755 --- a/t/t7001-mv.sh +++ b/t/t7001-mv.sh @@ -350,10 +350,11 @@ test_expect_success 'git mv moves a submodule with a .git directory and .gitmodu ' test_expect_success 'git mv moves a submodule with gitfile' ' - rm -rf mod/sub + rm -rf mod git reset --hard git submodule update entry=$(git ls-files --stage sub | cut -f 1) + mkdir mod ( cd mod git mv ../sub/ . @@ -372,11 +373,12 @@ test_expect_success 'git mv moves a submodule with gitfile' ' ' test_expect_success 'mv does not complain when no .gitmodules file is found' ' - rm -rf mod/sub + rm -rf mod git reset --hard git submodule update git rm .gitmodules entry=$(git ls-files --stage sub | cut -f 1) + mkdir mod git mv sub mod/sub 2actual.err ! test -s actual.err ! test -e sub @@ -390,11 +392,12 @@ test_expect_success 'mv does not complain when no .gitmodules file is found' ' ' test_expect_success 'mv will error out on a modified .gitmodules file unless staged' ' - rm -rf mod/sub + rm -rf mod git reset --hard git submodule update git config -f .gitmodules foo.bar true entry=$(git ls-files --stage sub | cut -f 1) + mkdir mod test_must_fail git mv sub mod/sub 2actual.err test -s actual.err test -e sub @@ -413,13 +416,14 @@ test_expect_success 'mv will error out on a modified .gitmodules file unless sta ' test_expect_success 'mv issues a warning when section is not found in .gitmodules' ' - rm -rf mod/sub + rm -rf mod git reset --hard git submodule update git config -f .gitmodules --remove-section submodule.sub git add .gitmodules entry=$(git ls-files --stage sub | cut -f 1) echo warning: Could not find section in .gitmodules where path=sub expect.err + mkdir mod git mv sub mod/sub 2actual.err test_i18ncmp expect.err actual.err ! test -e sub @@ -433,9 +437,10 @@ test_expect_success 'mv issues a warning when section is not found in .gitmodule ' test_expect_success 'mv --dry-run does not touch the submodule or .gitmodules' ' - rm -rf mod/sub + rm -rf mod git reset --hard git submodule update + mkdir mod git mv -n sub mod/sub 2actual.err test -f sub/.git git diff-index --exit-code HEAD -- 2.1.0.rc2.206.gedb03e5 -- 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 02/19] wrapper.c: remove/unlink_or_warn: simplify, treat ENOENT as success
From: Ronnie Sahlberg sahlb...@google.com
Simplify the function warn_if_unremovable slightly. Additionally, change
behaviour slightly. If we failed to remove the object because the object
does not exist, we can still return success back to the caller since none of
the callers depend on fail if the file did not exist.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
git-compat-util.h | 7 +--
wrapper.c | 14 ++
2 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index b6f03b3..611e77b 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -706,11 +706,14 @@ void git_qsort(void *base, size_t nmemb, size_t size,
/*
* Preserves errno, prints a message, but gives no warning for ENOENT.
- * Always returns the return value of unlink(2).
+ * Returns 0 on success which includes trying to unlink an object that does
+ * not exist.
*/
int unlink_or_warn(const char *path);
/*
- * Likewise for rmdir(2).
+ * Preserves errno, prints a message, but gives no warning for ENOENT.
+ * Returns 0 on success which includes trying to remove a directory that does
+ * not exist.
*/
int rmdir_or_warn(const char *path);
/*
diff --git a/wrapper.c b/wrapper.c
index bc1bfb8..c9605cd 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -429,14 +429,12 @@ int xmkstemp_mode(char *template, int mode)
static int warn_if_unremovable(const char *op, const char *file, int rc)
{
- if (rc 0) {
- int err = errno;
- if (ENOENT != err) {
- warning(unable to %s %s: %s,
- op, file, strerror(errno));
- errno = err;
- }
- }
+ int err;
+ if (!rc || errno == ENOENT)
+ return 0;
+ err = errno;
+ warning(unable to %s %s: %s, op, file, strerror(errno));
+ errno = err;
return rc;
}
--
2.1.0.rc2.206.gedb03e5
--
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 03/19] wrapper.c: add a new function unlink_or_msg
From: Ronnie Sahlberg sahlb...@google.com
Date: Wed, 16 Jul 2014 11:20:36 -0700
This behaves like unlink_or_warn except that on failure it writes the message
to its 'err' argument, which the caller can display in an appropriate way or
ignore.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
git-compat-util.h | 9 +
wrapper.c | 14 ++
2 files changed, 23 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index 611e77b..5ee140c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -307,6 +307,8 @@ extern char *gitbasename(char *);
#include wildmatch.h
+struct strbuf;
+
/* General helper functions */
extern void vreportf(const char *prefix, const char *err, va_list params);
extern void vwritef(int fd, const char *prefix, const char *err, va_list
params);
@@ -710,6 +712,13 @@ void git_qsort(void *base, size_t nmemb, size_t size,
* not exist.
*/
int unlink_or_warn(const char *path);
+ /*
+ * Tries to unlink file. Returns 0 if unlink succeeded
+ * or the file already didn't exist. Returns -1 and
+ * appends a message to err suitable for
+ * 'error(%s, err-buf)' on error.
+ */
+int unlink_or_msg(const char *file, struct strbuf *err);
/*
* Preserves errno, prints a message, but gives no warning for ENOENT.
* Returns 0 on success which includes trying to remove a directory that does
diff --git a/wrapper.c b/wrapper.c
index c9605cd..ec7a08b 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -438,6 +438,20 @@ static int warn_if_unremovable(const char *op, const char
*file, int rc)
return rc;
}
+int unlink_or_msg(const char *file, struct strbuf *err)
+{
+ int rc = unlink(file);
+
+ assert(err);
+
+ if (!rc || errno == ENOENT)
+ return 0;
+
+ strbuf_addf(err, unable to unlink %s: %s,
+ file, strerror(errno));
+ return -1;
+}
+
int unlink_or_warn(const char *file)
{
return warn_if_unremovable(unlink, file, unlink(file));
--
2.1.0.rc2.206.gedb03e5
--
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 04/19] refs.c: add an err argument to delete_ref_loose
From: Ronnie Sahlberg sahlb...@google.com
Date: Thu, 15 May 2014 08:25:23 -0700
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.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
refs.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/refs.c b/refs.c
index 7235574..5609622 100644
--- a/refs.c
+++ b/refs.c
@@ -2548,16 +2548,16 @@ int repack_without_refs(const char **refnames, int n,
struct strbuf *err)
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);
+ res = unlink_or_msg(lock-lk-filename, err);
lock-lk-filename[i] = '.';
- if (err errno != ENOENT)
+ if (res)
return 1;
}
return 0;
@@ -3604,7 +3604,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (update-lock) {
- ret |= delete_ref_loose(update-lock, update-type);
+ ret |= delete_ref_loose(update-lock, update-type,
+ err);
if (!(update-flags REF_ISPRUNING))
delnames[delnum++] = update-lock-ref_name;
}
--
2.1.0.rc2.206.gedb03e5
--
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 07/19] refs.c: move the check for valid refname to lock_ref_sha1_basic
From: Ronnie Sahlberg sahlb...@google.com
Date: Thu, 1 May 2014 10:40:10 -0700
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 has no visible impact to callers,
except for inability to lock badly named refs which is not possible today
anyway but for other reasons.
Keep lock_any_ref_for_update as a no-op wrapper --- it is the public facing
version of this interface and keeping it as a separate function will make
it easier to experiment with the internal lock_ref_sha1_basic signature.
Note that if lock_ref_sha1_basic now checks the refname format and fails to
lock the ref it will not be possible to delete such a ref since deletion
implies we first lock the ref. In fact, we currently fail even earlier than
that since these refs are not even recognized to exist.
Example:
$ cp .git/refs/heads/master .git/refs/heads/echo...\*\*
$ ./git branch -D .git/refs/heads/echo...\*\*
error: branch '.git/refs/heads/echo...**' not found.
This is not a new regression and this has been broken for a while.
Later patches in the series will start repairing the handling of badly
named refs, at which time we will need to modify lock_ref_sha1_basic once more
in order to allow locking these refs for certain use cases such as rename
and delete.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
refs.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index 39571f5..3c2ce57 100644
--- a/refs.c
+++ b/refs.c
@@ -2091,6 +2091,11 @@ 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)) {
+ errno = EINVAL;
+ return NULL;
+ }
+
lock = xcalloc(1, sizeof(struct ref_lock));
lock-lock_fd = -1;
@@ -2182,8 +2187,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.1.0.rc2.206.gedb03e5
--
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 06/19] rename_ref: don't ask read_ref_full where the ref came from
From: Ronnie Sahlberg sahlb...@google.com
Date: Wed, 30 Apr 2014 12:41:04 -0700
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 sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
refs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 99a9b86..39571f5 100644
--- a/refs.c
+++ b/refs.c
@@ -2671,7 +2671,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.1.0.rc2.206.gedb03e5
--
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 05/19] refs.c: pass the ref log message to _create/delete/update instead of _commit
From: Ronnie Sahlberg sahlb...@google.com
Date: Wed, 30 Apr 2014 12:22:42 -0700
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 sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
branch.c | 4 ++--
builtin/commit.c | 4 ++--
builtin/receive-pack.c | 5 +++--
builtin/replace.c | 5 +++--
builtin/tag.c | 4 ++--
builtin/update-ref.c | 13 +++--
fast-import.c | 8
refs.c | 34 +-
refs.h | 8
sequencer.c| 4 ++--
walker.c | 5 ++---
11 files changed, 52 insertions(+), 42 deletions(-)
diff --git a/branch.c b/branch.c
index 37ac555..76a8ec9 100644
--- a/branch.c
+++ b/branch.c
@@ -301,8 +301,8 @@ void create_branch(const char *head,
transaction = ref_transaction_begin(err);
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);
strbuf_release(err);
diff --git a/builtin/commit.c b/builtin/commit.c
index 9bf1003..d23e876 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1767,8 +1767,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/receive-pack.c b/builtin/receive-pack.c
index 224fadc..d1f4cf7 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -585,8 +585,9 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
transaction = ref_transaction_begin(err);
if (!transaction ||
ref_transaction_update(transaction, namespaced_name,
- new_sha1, old_sha1, 0, 1, err) ||
- ref_transaction_commit(transaction, push, err)) {
+ new_sha1, old_sha1, 0, 1, push,
+ err) ||
+ ref_transaction_commit(transaction, err)) {
ref_transaction_free(transaction);
rp_error(%s, err.buf);
diff --git a/builtin/replace.c b/builtin/replace.c
index 1fcd06d..9d03b84 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -169,8 +169,9 @@ static int replace_object_sha1(const char *object_ref,
transaction = ref_transaction_begin(err);
if (!transaction ||
- ref_transaction_update(transaction, ref, repl, prev, 0, 1, err) ||
- ref_transaction_commit(transaction, NULL, err))
+ ref_transaction_update(transaction, ref, repl, prev,
+ 0, 1, NULL, err) ||
+ ref_transaction_commit(transaction, err))
die(%s, err.buf);
ref_transaction_free(transaction);
diff --git a/builtin/tag.c b/builtin/tag.c
index f3f172f..70d28c5 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -705,8 +705,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
transaction = ref_transaction_begin(err);
if (!transaction ||
ref_transaction_update(transaction, ref.buf, object, prev,
- 0, 1, err) ||
- ref_transaction_commit(transaction, NULL, err))
+ 0, 1, NULL, err) ||
+ ref_transaction_commit(transaction, err))
die(%s, err.buf);
ref_transaction_free(transaction);
if (force !is_null_sha1(prev) hashcmp(prev, object))
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 54a48c0..6c9be05 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -14,6 +14,7 @@ static const char * const git_update_ref_usage[] = {
static char line_termination = '\n';
static int update_flags;
+static const char *msg;
/*
* Parse one whitespace- or NUL-terminated, possibly C-quoted argument
@@ -198,7 +199,7 @@ static const char
[PATCH 08/19] refs.c: call lock_ref_sha1_basic directly from commit
From: Ronnie Sahlberg sahlb...@google.com
Date: Thu, 1 May 2014 10:43:39 -0700
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 sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
refs.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index 3c2ce57..f124c2b 100644
--- a/refs.c
+++ b/refs.c
@@ -3578,12 +3578,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.1.0.rc2.206.gedb03e5
--
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 10/19] refs.c: ref_transaction_commit: distinguish name conflicts from other errors
From: Ronnie Sahlberg sahlb...@google.com
Date: Fri, 16 May 2014 14:14:38 -0700
In _commit, ENOTDIR can happen in the call to lock_ref_sha1_basic, either when
we lstat the new refname and it returns ENOTDIR or if the name checking
function reports that the same type of conflict happened. In both cases it
means that we can not create the new ref due to a name conflict.
Start defining specific return codes for _commit: assign -1 as a generic
error and UPDATE_REFS_NAME_CONFLICT (-2) as the error that refers to a name
conflict.
When git fetch is creating refs, name conflicts differ from other errors in
that they are likely to be resolved by running git remote prune remote.
git fetch currently inspects errno to decide whether to give that advice.
Once it switches to the transaction API, it can check for
UPDATE_REFS_NAME_CONFLICT instead.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
refs.c | 18 --
refs.h | 5 +
2 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index b63ab2f..86c708a 100644
--- a/refs.c
+++ b/refs.c
@@ -3584,9 +3584,10 @@ 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);
- if (ret)
+ if (ref_update_reject_duplicates(updates, n, err)) {
+ ret = -1;
goto cleanup;
+ }
/* Acquire all locks while verifying old values */
for (i = 0; i n; i++) {
@@ -3600,10 +3601,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
update-flags,
update-type);
if (!update-lock) {
+ int df_conflict = (errno == ENOTDIR);
+
if (err)
strbuf_addf(err, Cannot lock the ref '%s'.,
update-refname);
- ret = 1;
+ ret = df_conflict ? UPDATE_REFS_NAME_CONFLICT : -1;
goto cleanup;
}
}
@@ -3620,6 +3623,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
if (err)
strbuf_addf(err, Cannot update the ref
'%s'.,
update-refname);
+ ret = -1;
goto cleanup;
}
}
@@ -3630,14 +3634,16 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (update-lock) {
- ret |= delete_ref_loose(update-lock, update-type,
- err);
+ if (delete_ref_loose(update-lock, update-type, err))
+ ret = -1;
+
if (!(update-flags REF_ISPRUNING))
delnames[delnum++] = update-lock-ref_name;
}
}
- ret |= repack_without_refs(delnames, delnum, err);
+ if (repack_without_refs(delnames, delnum, err))
+ ret = -1;
for (i = 0; i delnum; i++)
unlink_or_warn(git_path(logs/%s, delnames[i]));
clear_loose_ref_cache(ref_cache);
diff --git a/refs.h b/refs.h
index 7c1bf95..e14aa31 100644
--- a/refs.h
+++ b/refs.h
@@ -325,7 +325,12 @@ int 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.
+ *
+ * Function returns 0 on success, -1 for generic failures and
+ * UPDATE_REFS_NAME_CONFLICT (-2) if the failure was due to a naming conflict.
+ * For example, the ref names A and A/B conflict.
*/
+#define UPDATE_REFS_NAME_CONFLICT -2
int ref_transaction_commit(struct ref_transaction *transaction,
struct strbuf *err);
--
2.1.0.rc2.206.gedb03e5
--
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 09/19] refs.c: pass a skip list to name_conflict_fn
From: Ronnie Sahlberg sahlb...@google.com
Date: Thu, 1 May 2014 11:16:07 -0700
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 sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
refs.c | 41 -
1 file changed, 28 insertions(+), 13 deletions(-)
diff --git a/refs.c b/refs.c
index f124c2b..b63ab2f 100644
--- a/refs.c
+++ b/refs.c
@@ -801,14 +801,16 @@ static int names_conflict(const char *refname1, const
char *refname2)
struct name_conflict_cb {
const char *refname;
- const char *oldrefname;
const char *conflicting_refname;
+ struct string_list *skiplist;
};
static int name_conflict_fn(struct ref_entry *entry, void *cb_data)
{
struct name_conflict_cb *data = (struct name_conflict_cb *)cb_data;
- if (data-oldrefname !strcmp(data-oldrefname, entry-name))
+
+ if (data-skiplist
+ string_list_has_string(data-skiplist, entry-name))
return 0;
if (names_conflict(data-refname, entry-name)) {
data-conflicting_refname = entry-name;
@@ -822,15 +824,17 @@ 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). skiplist contains a list of refs we want to skip checking for
+ * conflicts with. skiplist must be sorted.
*/
-static int is_refname_available(const char *refname, const char *oldrefname,
- struct ref_dir *dir)
+static int is_refname_available(const char *refname,
+ struct ref_dir *dir,
+ struct string_list *skiplist)
{
struct name_conflict_cb data;
data.refname = refname;
- data.oldrefname = oldrefname;
data.conflicting_refname = NULL;
+ data.skiplist = skiplist;
sort_ref_dir(dir);
if (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, data)) {
@@ -2080,6 +2084,7 @@ 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,
+ struct string_list *skiplist,
int flags, int *type_p)
{
char *ref_file;
@@ -2129,7 +2134,8 @@ 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, get_packed_refs(ref_cache),
+ skiplist)) {
last_errno = ENOTDIR;
goto error_return;
}
@@ -2187,7 +2193,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, NULL, flags, type_p);
}
/*
@@ -2648,6 +2654,7 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
struct stat loginfo;
int log = !lstat(git_path(logs/%s, oldrefname), loginfo);
const char *symref = NULL;
+ struct string_list skiplist = STRING_LIST_INIT_NODUP;
if (log S_ISLNK(loginfo.st_mode))
return error(reflog for %s is a symlink, oldrefname);
@@ -2659,11 +2666,18 @@ 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)))
+ string_list_insert(skiplist, oldrefname);
+ if (!is_refname_available(newrefname, get_packed_refs(ref_cache),
+ skiplist)) {
+ string_list_clear(skiplist, 0);
return 1;
-
- if (!is_refname_available(newrefname, oldrefname,
get_loose_refs(ref_cache
[PATCH 11/19] fetch.c: change s_update_ref to use a ref transaction
From: Ronnie Sahlberg sahlb...@google.com
Date: Mon, 28 Apr 2014 13:49:07 -0700
Change s_update_ref to use a ref transaction for the ref update.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
builtin/fetch.c | 34 --
1 file changed, 24 insertions(+), 10 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 55f457c..2e3bc73 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -375,23 +375,37 @@ 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;
+ struct strbuf err = STRBUF_INIT;
+ int ret, df_conflict = 0;
if (dry_run)
return 0;
if (!rla)
rla = default_rla.buf;
snprintf(msg, sizeof(msg), %s: %s, rla, action);
- 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)
- return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
- STORE_REF_ERROR_OTHER;
+
+ transaction = ref_transaction_begin(err);
+ if (!transaction ||
+ ref_transaction_update(transaction, ref-name, ref-new_sha1,
+ ref-old_sha1, 0, check_old, msg, err))
+ goto fail;
+
+ ret = ref_transaction_commit(transaction, err);
+ if (ret == UPDATE_REFS_NAME_CONFLICT)
+ df_conflict = 1;
+ if (ret)
+ goto fail;
+
+ ref_transaction_free(transaction);
+ strbuf_release(err);
return 0;
+fail:
+ ref_transaction_free(transaction);
+ error(%s, err.buf);
+ strbuf_release(err);
+ return df_conflict ? STORE_REF_ERROR_DF_CONFLICT
+ : STORE_REF_ERROR_OTHER;
}
#define REFCOL_WIDTH 10
--
2.1.0.rc2.206.gedb03e5
--
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 12/19] refs.c: make write_ref_sha1 static
From: Ronnie Sahlberg sahlb...@google.com
Date: Mon, 28 Apr 2014 15:36:58 -0700
No external users call write_ref_sha1 any more so lets declare it static.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
refs.c | 10 --
refs.h | 3 ---
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/refs.c b/refs.c
index 86c708a..c2dab4a 100644
--- a/refs.c
+++ b/refs.c
@@ -2646,6 +2646,9 @@ static int rename_tmp_log(const char *newrefname)
return 0;
}
+static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
+ const char *logmsg);
+
int rename_ref(const char *oldrefname, const char *newrefname, const char
*logmsg)
{
unsigned char sha1[20], orig_sha1[20];
@@ -2901,8 +2904,11 @@ static int is_branch(const char *refname)
return !strcmp(refname, HEAD) || starts_with(refname, refs/heads/);
}
-/* This function must return a meaningful errno */
-int write_ref_sha1(struct ref_lock *lock,
+/*
+ * Writes sha1 into the ref specified by the lock. Makes sure that errno
+ * is sane on error.
+ */
+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 e14aa31..fafa493 100644
--- a/refs.h
+++ b/refs.h
@@ -195,9 +195,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. Set errno to something meaningful on failure.
*/
--
2.1.0.rc2.206.gedb03e5
--
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 13/19] refs.c: change resolve_ref_unsafe reading argument to be a flags field
From: Ronnie Sahlberg sahlb...@google.com
Date: Tue, 15 Jul 2014 12:59:36 -0700
resolve_ref_unsafe takes a boolean argument for reading.
Change this to be a flags field instead and pass the new constant
RESOLVE_REF_READING when we want this behaviour.
Swap two of the arguments in the function to make sure that we capture any
instances where callers have not been updated/aware of the new api so that
they can be audited.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
branch.c| 2 +-
builtin/blame.c | 2 +-
builtin/branch.c| 9 +++---
builtin/checkout.c | 6 ++--
builtin/clone.c | 2 +-
builtin/commit.c| 2 +-
builtin/fmt-merge-msg.c | 2 +-
builtin/for-each-ref.c | 6 ++--
builtin/fsck.c | 2 +-
builtin/log.c | 3 +-
builtin/merge.c | 2 +-
builtin/notes.c | 2 +-
builtin/receive-pack.c | 4 +--
builtin/remote.c| 5 ++--
builtin/show-branch.c | 6 ++--
builtin/symbolic-ref.c | 2 +-
bundle.c| 2 +-
cache.h | 21 ++---
http-backend.c | 3 +-
notes-merge.c | 2 +-
reflog-walk.c | 5 ++--
refs.c | 79 -
remote.c| 10 ---
sequencer.c | 4 +--
transport-helper.c | 4 ++-
transport.c | 5 ++--
upload-pack.c | 2 +-
wt-status.c | 2 +-
28 files changed, 111 insertions(+), 85 deletions(-)
diff --git a/branch.c b/branch.c
index 76a8ec9..ba3e1c1 100644
--- a/branch.c
+++ b/branch.c
@@ -186,7 +186,7 @@ int validate_new_branchname(const char *name, struct strbuf
*ref,
const char *head;
unsigned char sha1[20];
- head = resolve_ref_unsafe(HEAD, sha1, 0, NULL);
+ head = resolve_ref_unsafe(HEAD, sha1, NULL, 0);
if (!is_bare_repository() head !strcmp(head, ref-buf))
die(_(Cannot force update the current branch.));
}
diff --git a/builtin/blame.c b/builtin/blame.c
index a52a279..b8bec0a 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2292,7 +2292,7 @@ static struct commit *fake_working_tree_commit(struct
diff_options *opt,
commit-object.type = OBJ_COMMIT;
parent_tail = commit-parents;
- if (!resolve_ref_unsafe(HEAD, head_sha1, 1, NULL))
+ if (!resolve_ref_unsafe(HEAD, head_sha1, NULL, RESOLVE_REF_READING))
die(no such ref: HEAD);
parent_tail = append_parent(parent_tail, head_sha1);
diff --git a/builtin/branch.c b/builtin/branch.c
index 652b1d2..f144808 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -129,7 +129,8 @@ static int branch_merged(int kind, const char *name,
branch-merge[0]
branch-merge[0]-dst
(reference_name = reference_name_to_free =
-resolve_refdup(branch-merge[0]-dst, sha1, 1, NULL)) !=
NULL)
+resolve_refdup(branch-merge[0]-dst, sha1,
+ NULL, RESOLVE_REF_READING)) != NULL)
reference_rev = lookup_commit_reference(sha1);
}
if (!reference_rev)
@@ -233,7 +234,7 @@ static int delete_branches(int argc, const char **argv, int
force, int kinds,
free(name);
name = mkpathdup(fmt, bname.buf);
- target = resolve_ref_unsafe(name, sha1, 0, flags);
+ target = resolve_ref_unsafe(name, sha1, flags, 0);
if (!target ||
(!(flags REF_ISSYMREF) is_null_sha1(sha1))) {
error(remote_branch
@@ -296,7 +297,7 @@ static char *resolve_symref(const char *src, const char
*prefix)
int flag;
const char *dst, *cp;
- dst = resolve_ref_unsafe(src, sha1, 0, flag);
+ dst = resolve_ref_unsafe(src, sha1, flag, 0);
if (!(dst (flag REF_ISSYMREF)))
return NULL;
if (prefix (cp = skip_prefix(dst, prefix)))
@@ -862,7 +863,7 @@ int cmd_branch(int argc, const char **argv, const char
*prefix)
track = git_branch_track;
- head = resolve_refdup(HEAD, head_sha1, 0, NULL);
+ head = resolve_refdup(HEAD, head_sha1, NULL, 0);
if (!head)
die(_(Failed to resolve HEAD as a valid ref.));
if (!strcmp(head, HEAD)) {
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f1dc56e..64af1f7 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -356,7 +356,7 @@ static int checkout_paths(const struct checkout_opts *opts,
commit_locked_index(lock_file))
die(_(unable to write new index file));
- read_ref_full(HEAD, rev, 0, flag);
+ read_ref_full(HEAD, rev, flag, 0);
head = lookup_commit_reference_gently(rev, 1);
errs
[PATCH 17/19] refs.c: do not permit err == NULL
Some functions that take a strbuf argument to append an error to treat
!err as an indication that the message should be suppressed (e.g.,
ref_update_reject_duplicates). Others write the message to stderr on
!err (e.g., repack_without_refs). Others crash (e.g.,
ref_transaction_update).
Some of these behaviors are for historical reasons and others were
accidents. Luckily no callers pass err == NULL any more. Simplify
by consistently requiring the strbuf argument.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
refs.c | 46 +++---
1 file changed, 27 insertions(+), 19 deletions(-)
diff --git a/refs.c b/refs.c
index 6faf0c8..550223c 100644
--- a/refs.c
+++ b/refs.c
@@ -2560,6 +2560,8 @@ int repack_without_refs(const char **refnames, int n,
struct strbuf *err)
struct string_list_item *ref_to_delete;
int i, ret, removed = 0;
+ assert(err);
+
/* Look for a packed ref */
for (i = 0; i n; i++)
if (get_packed_ref(refnames[i]))
@@ -2570,13 +2572,8 @@ int repack_without_refs(const char **refnames, int n,
struct strbuf *err)
return 0; /* no refname exists in packed refs */
if (lock_packed_refs(0)) {
- if (err) {
- unable_to_lock_message(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]);
+ unable_to_lock_message(git_path(packed-refs), errno, err);
+ return -1;
}
packed = get_packed_refs(ref_cache);
@@ -2602,7 +2599,7 @@ int repack_without_refs(const char **refnames, int n,
struct strbuf *err)
/* Write what remains */
ret = commit_packed_refs();
- if (ret err)
+ if (ret)
strbuf_addf(err, unable to overwrite old ref-pack file: %s,
strerror(errno));
return ret;
@@ -2610,6 +2607,8 @@ int repack_without_refs(const char **refnames, int n,
struct strbuf *err)
static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf
*err)
{
+ assert(err);
+
if (!(flag REF_ISPACKED) || flag REF_ISSYMREF) {
/* loose */
int res, i = strlen(lock-lk-filename) - 5; /* .lock */
@@ -3459,6 +3458,8 @@ struct ref_transaction {
struct ref_transaction *ref_transaction_begin(struct strbuf *err)
{
+ assert(err);
+
return xcalloc(1, sizeof(struct ref_transaction));
}
@@ -3498,6 +3499,8 @@ int ref_transaction_update(struct ref_transaction
*transaction,
{
struct ref_update *update;
+ assert(err);
+
if (transaction-state != REF_TRANSACTION_OPEN)
die(BUG: update called for transaction that is not open);
@@ -3530,6 +3533,8 @@ int ref_transaction_create(struct ref_transaction
*transaction,
{
struct ref_update *update;
+ assert(err);
+
if (transaction-state != REF_TRANSACTION_OPEN)
die(BUG: create called for transaction that is not open);
@@ -3561,6 +3566,8 @@ int ref_transaction_delete(struct ref_transaction
*transaction,
{
struct ref_update *update;
+ assert(err);
+
if (transaction-state != REF_TRANSACTION_OPEN)
die(BUG: delete called for transaction that is not open);
@@ -3623,13 +3630,14 @@ static int ref_update_reject_duplicates(struct
ref_update **updates, int n,
struct strbuf *err)
{
int i;
+
+ assert(err);
+
for (i = 1; i n; i++)
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);
-
+ strbuf_addf(err,
+ Multiple updates for ref '%s' not
allowed.,
+ updates[i]-refname);
return 1;
}
return 0;
@@ -3643,6 +3651,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
int n = transaction-nr;
struct ref_update **updates = transaction-updates;
+ assert(err);
+
if (transaction-state != REF_TRANSACTION_OPEN)
die(BUG: commit called for transaction that is not open);
@@ -3675,9 +3685,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
if (!update-lock) {
int df_conflict = (errno == ENOTDIR);
- if (err)
- strbuf_addf(err, Cannot lock the ref '%s'.,
- update
[PATCH 16/19] for-each-ref.c: improve message before aborting on broken ref
From: Ronnie Sahlberg sahlb...@google.com
Date: Fri, 5 Sep 2014 14:35:17 -0700
Print a warning message for any badly named refs we find in the repo.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
builtin/for-each-ref.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index e193073..090390c 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -853,6 +853,12 @@ static int grab_single_ref(const char *refname, const
unsigned char *sha1, int f
struct refinfo *ref;
int cnt;
+ if ((flag REF_ISBROKEN)
+ check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+ warning(ignoring ref with broken name %s, refname);
+ return 0;
+ }
+
if (*cb-grab_pattern) {
const char **pattern;
int namelen = strlen(refname);
--
2.1.0.rc2.206.gedb03e5
--
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 15/19] refs.c: fix handling of badly named refs
From: Ronnie Sahlberg sahlb...@google.com
Date: Wed, 3 Sep 2014 11:45:43 -0700
We currently do not handle badly named refs well:
$ cp .git/refs/heads/master .git/refs/heads/master.@\*@\\.
$ git branch
fatal: Reference has invalid format: 'refs/heads/master.@*@\.'
$ git branch -D master.@\*@\\.
error: branch 'master.@*@\.' not found.
Currently we can not really recover from a badly named ref with less than
manually deleting the .git/refs/heads/refname file. This will also help
if change the naming rules in the future. For example if we decide to remove
`, and such from the set of valid characters.
The purpose of this change is to allow git branch --list to show these refs
and to allow git branch -d/-D and update-ref -d to delete them.
Other functions will continue to not handle these refs but can be changed in
later patches.
Introduce two new internal flags: RESOLVE_REF_ALLOW_BAD_NAME and REF_BADNAMEOK.
In resolving functions, refuse to resolve badly named refs unless the new
RESOLVE_REF_ALLOW_BAD_NAME flag is passed. For these cases, if the badly named
ref exists then flag it as REF_ISBROKEN and resolve it to nullsha1.
In locking functions, refuse to act on badly named refs unless the new
REF_BADNAMEOK flag is passed. This is currently used only from branch.c and
update-ref.c when deleting a ref.
Change the internal functions that read loose and packed refs to allow
badly named refs but flag them as REF_ISBROKEN just like unresolvable refs
are.
During ref iteration only include these refs during for_each_rawref but
none of the other iterators. I.e. just like unresolvable refs, flag
inclusion of these refs based on DO_FOR_EACH_INCLUDE_BROKEN during
iteration.
In the transaction API, always refuse to create or update badly named refs
and refuse to delete them unless REF_BADNAMEOK flag is passed.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
builtin/branch.c| 13 ++
builtin/update-ref.c| 3 ++-
cache.h | 8 +-
refs.c | 63 ++---
refs.h | 2 ++
t/t1402-check-ref-format.sh | 48 ++
6 files changed, 121 insertions(+), 16 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 7925660..5d5bc56 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -234,10 +234,13 @@ static int delete_branches(int argc, const char **argv,
int force, int kinds,
free(name);
name = mkpathdup(fmt, bname.buf);
- target = resolve_ref_unsafe(name, sha1,
- flags, RESOLVE_REF_NODEREF);
+ target = resolve_ref_unsafe(name, sha1, flags,
+ RESOLVE_REF_READING
+ | RESOLVE_REF_NODEREF
+ | RESOLVE_REF_ALLOW_BAD_NAME);
if (!target ||
- (!(flags REF_ISSYMREF) is_null_sha1(sha1))) {
+ (!(flags (REF_ISSYMREF|REF_ISBROKEN))
+is_null_sha1(sha1))) {
error(remote_branch
? _(remote branch '%s' not found.)
: _(branch '%s' not found.), bname.buf);
@@ -245,14 +248,14 @@ static int delete_branches(int argc, const char **argv,
int force, int kinds,
continue;
}
- if (!(flags REF_ISSYMREF)
+ if (!(flags (REF_ISSYMREF|REF_ISBROKEN))
check_branch_commit(bname.buf, name, sha1, head_rev, kinds,
force)) {
ret = 1;
continue;
}
- if (delete_ref(name, sha1, REF_NODEREF)) {
+ if (delete_ref(name, sha1, REF_NODEREF|REF_BADNAMEOK)) {
error(remote_branch
? _(Error deleting remote branch '%s')
: _(Error deleting branch '%s'),
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 6c9be05..e379fdd 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -419,7 +419,8 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
if (no_deref)
flags = REF_NODEREF;
if (delete)
- return delete_ref(refname, oldval ? oldsha1 : NULL, flags);
+ return delete_ref(refname, oldval ? oldsha1 : NULL,
+ flags|REF_BADNAMEOK);
else
return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
flags, UPDATE_REFS_DIE_ON_ERR);
diff --git a/cache.h b/cache.h
index 03ade12..03a6144 100644
--- a/cache.h
+++ b/cache.h
@@ -979,10 +979,16 @@ extern int
[PATCH 14/19] branch -d: avoid repeated symref resolution
If a repository gets in a broken state with too much symref nesting,
it cannot be repaired with git branch -d:
$ git symbolic-ref refs/heads/nonsense refs/heads/nonsense
$ git branch -d nonsense
error: branch 'nonsense' not found.
Worse, git update-ref --no-deref -d doesn't work for such repairs
either:
$ git update-ref -d refs/heads/nonsense
error: unable to resolve reference refs/heads/nonsense: Too many levels of
symbolic links
Fix both by teaching resolve_ref_unsafe a new RESOLVE_REF_NODEREF flag
and passing it when appropriate.
Callers can still read the value of a symref (for example to print a
message about it) with that flag set --- resolve_ref_unsafe will
resolve one level of symrefs and stop there.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
builtin/branch.c | 3 ++-
cache.h | 1 +
refs.c| 10 ++
t/t1400-update-ref.sh | 16
t/t3200-branch.sh | 9 +
5 files changed, 38 insertions(+), 1 deletion(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index f144808..7925660 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -234,7 +234,8 @@ static int delete_branches(int argc, const char **argv, int
force, int kinds,
free(name);
name = mkpathdup(fmt, bname.buf);
- target = resolve_ref_unsafe(name, sha1, flags, 0);
+ target = resolve_ref_unsafe(name, sha1,
+ flags, RESOLVE_REF_NODEREF);
if (!target ||
(!(flags REF_ISSYMREF) is_null_sha1(sha1))) {
error(remote_branch
diff --git a/cache.h b/cache.h
index 2d3c5ed..03ade12 100644
--- a/cache.h
+++ b/cache.h
@@ -982,6 +982,7 @@ extern int read_ref(const char *refname, unsigned char
*sha1);
* errno is set to something meaningful on error.
*/
#define RESOLVE_REF_READING 0x01
+#define RESOLVE_REF_NODEREF 0x02
extern const char *resolve_ref_unsafe(const char *ref, unsigned char *sha1,
int *flags, int resolve_flags);
extern char *resolve_refdup(const char *ref, unsigned char *sha1, int *flags,
int resolve_flags);
diff --git a/refs.c b/refs.c
index a5f9734..f11df33 100644
--- a/refs.c
+++ b/refs.c
@@ -1408,6 +1408,10 @@ const char *resolve_ref_unsafe(const char *refname,
unsigned char *sha1, int *fl
refname = refname_buffer;
if (flags)
*flags |= REF_ISSYMREF;
+ if (resolve_flags RESOLVE_REF_NODEREF) {
+ hashclr(sha1);
+ return refname;
+ }
continue;
}
}
@@ -1471,6 +1475,10 @@ const char *resolve_ref_unsafe(const char *refname,
unsigned char *sha1, int *fl
return NULL;
}
refname = strcpy(refname_buffer, buf);
+ if (resolve_flags RESOLVE_REF_NODEREF) {
+ hashclr(sha1);
+ return refname;
+ }
}
}
@@ -2110,6 +2118,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
if (mustexist)
resolve_flags |= RESOLVE_REF_READING;
+ if (flags REF_NODEREF)
+ resolve_flags |= RESOLVE_REF_NODEREF;
refname = resolve_ref_unsafe(refname, lock-old_sha1, type,
resolve_flags);
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 0218e96..ff4607b 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -110,6 +110,22 @@ test_expect_success delete symref without dereference
when the referred ref is
cp -f .git/HEAD.orig .git/HEAD
git update-ref -d $m
+test_expect_success 'update-ref -d is not confused by self-reference' '
+ git symbolic-ref refs/heads/self refs/heads/self
+ test_when_finished rm -f .git/refs/heads/self
+ test_path_is_file .git/refs/heads/self
+ test_must_fail git update-ref -d refs/heads/self
+ test_path_is_file .git/refs/heads/self
+'
+
+test_expect_success 'update-ref --no-deref -d can delete self-reference' '
+ git symbolic-ref refs/heads/self refs/heads/self
+ test_when_finished rm -f .git/refs/heads/self
+ test_path_is_file .git/refs/heads/self
+ git update-ref --no-deref -d refs/heads/self
+ test_path_is_missing .git/refs/heads/self
+'
+
test_expect_success '(not) create HEAD with old sha1'
test_must_fail git update-ref HEAD $A $B
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index ac31b71..432921b 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -285,6 +285,15 @@ test_expect_success 'deleting a dangling symref' '
test_i18ncmp expect actual
'
+test_expect_success 'deleting a self-referential symref
[PATCH 18/19] lockfile: remove unable_to_lock_error
The former caller uses unable_to_lock_message now.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Reviewed-by: Ronnie Sahlberg sahlb...@google.com
---
cache.h| 1 -
lockfile.c | 10 --
2 files changed, 11 deletions(-)
diff --git a/cache.h b/cache.h
index 03a6144..995729f 100644
--- a/cache.h
+++ b/cache.h
@@ -558,7 +558,6 @@ 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_message(const char *path, int err,
struct strbuf *buf);
extern NORETURN void unable_to_lock_index_die(const char *path, int err);
diff --git a/lockfile.c b/lockfile.c
index a921d77..dbd4101 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -176,16 +176,6 @@ void unable_to_lock_message(const char *path, int err,
struct strbuf *buf)
absolute_path(path), strerror(err));
}
-int unable_to_lock_error(const char *path, int err)
-{
- struct strbuf buf = STRBUF_INIT;
-
- unable_to_lock_message(path, err, buf);
- error(%s, buf.buf);
- strbuf_release(buf);
- return -1;
-}
-
NORETURN void unable_to_lock_index_die(const char *path, int err)
{
struct strbuf buf = STRBUF_INIT;
--
2.1.0.rc2.206.gedb03e5
--
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
