Re: Our merge bases sometimes suck
Michael Haggerty mhag...@alum.mit.edu writes: It just looks asymmetric, but actually it is symmetric, which was kindof surprising when I realized it Since |branch ∧ master| is the same for all candidates, minimizing N is the same as maximizing |candidate|, which is the same as git rev-list --count --no-merges $candidate This is clearly symmetric in master vs. base. Hmph, but that obviously will become very expensive to compute as project grows. When we (potentially) have multiple merge-bases, after finding all the candidates by traversing from the two commits to be merged, we already make another set of traversals, starting from the candidates and painting the ancestors down to their common ancestors. This is done to discover if each candidate is reachable from any other candidate (in which case the reachable one is not a merge-base). The resulting graph of this traversal is currently used only to cull non-merge-bases out of the candidates, but I wonder if you can *count* the nodes in it in each color and use that number (which is essentially the number of commits that can be reached only from one candidate and not from other candidates) to derive a score for each candidate, and use it to assess the goodness of merge-bases, just like the number you are counting in the above full traversal. -- 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: The different EOL behavior between libgit2-based software and official Git
Hm, I feeled puzzled here. Even if I wouldn't recommend to use core.autocrlf, and prefer to use .gitattributes, the CRLF conversion should work under Git, but it doensn't seem to do so. Clone this repo: origin https://github.com/YueLinHo/TestAutoCrlf.git Try to see if LF or CRLF can be converted into CRLF, when core.autocrlf is true. Neither msysgit nor Git under Linux produces CRLF (?) Git under Mac OS produces the CRLF: both Git 2.0.0 and the latest msygit code base (7e872d24a9bd03), compiled under Mac OS What do I miss ? git --version git version 2.0.0 tb@Linux:~/EOL_Test/TestAutoCrlf$ t=MIX-more_LF.txtrm -f $t git -c core.eol=CRLF checkout $t od -c $t 000 L i n e 1 \n l i n e ( 2 ) \r 020 \n l i n e 3 . \n t h i s i s 040 l i n e 4 \n l i n e 060 N o . 5 \n L i n e N u m b e 100 r 6 \n = $ git --version git version 1.9.2.msysgit.0.1206.g7e872d2 tb@msgit ~/EOL_test/TestAutoCrlf (master) $ t=MIX-more_LF.txtrm -f $t git -c core.eol=CRLF checkout $t od -c $t 000 L i n e 1 \n l i n e ( 2 ) \r 020 \n l i n e 3 . \n t h i s i s 040 l i n e 4 \n l i n e 060 N o . 5 \n L i n e N u m b e 100 r 6 \n = tb@mac:~/EOL_Test/TestAutoCrlf git --version git version 2.0.0.622.g9478935 tb@mac:~/EOL_Test/TestAutoCrlf t=MIX-more_LF.txtrm -f $t git -c core.eol=CRLF checkout $t od -c $t 000L i n e 1 \r \n l i n e ( 2 ) 020 \r \n l i n e 3 . \r \n t h i s 040i s l i n e 4 \r \n l i n 060e N o . 5 \r \n L i n e N 100u m b e r 6 \r \n == tb@mac:~/EOL_Test/TestAutoCrlf t=MIX-more_LF.txtrm -f $t ~/projects/git/tb.msygit/git -c core.eol=CRLF checkout $t od -c $t 000L i n e 1 \r \n l i n e ( 2 ) 020 \r \n l i n e 3 . \r \n t h i s 040i s l i n e 4 \r \n l i n 060e N o . 5 \r \n L i n e N 100u m b e r 6 \r \n -- 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: search for equivalent commits by patch-id
On Thu, Jun 19, 2014 at 03:26:17PM +0300, vicentiu.nea...@ni.com wrote: Is there a way to find all equivalent commits by patch-id? Something similar to: git branch -a --commit commit but instead of commit to search by patch-id. There isn't a ready-made command to do so, but you can easily script it: git for-each-ref --format='%(refname)' refs/heads | while read ref; do echo $(git diff-tree -p $ref | git patch-id) $ref done | grep $PATCH_ID_YOU_ARE_LOOKING_FOR If you want to look further back than the tips, you can feed the branches to rev-list, and then patch-id each commit you find. The more common way to use patch-ids is to look for commits that are in one branch but not another. For that, try the --cherry-pick and --cherry-mark options to git log. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: The different EOL behavior between libgit2-based software and official Git
Wow! P.S. A note: libgit2 just has a PR that try to be identical with official git 2.0.0. See https://github.com/libgit2/libgit2/pull/2432 -- View this message in context: http://git.661346.n2.nabble.com/The-different-EOL-behavior-between-libgit2-based-software-and-official-Git-tp7613670p7613801.html Sent from the git mailing list archive at Nabble.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: Our merge bases sometimes suck
On 06/20/2014 08:53 AM, Junio C Hamano wrote: Michael Haggerty mhag...@alum.mit.edu writes: It just looks asymmetric, but actually it is symmetric, which was kindof surprising when I realized it Since |branch ∧ master| is the same for all candidates, minimizing N is the same as maximizing |candidate|, which is the same as git rev-list --count --no-merges $candidate This is clearly symmetric in master vs. base. Hmph, but that obviously will become very expensive to compute as project grows. This formulation is theoretically interesting because it shows the symmetry of the criterion, but that doesn't mean it is the most practical to use. Given that multiple formulations are equivalent, any of them can be used. When we (potentially) have multiple merge-bases, after finding all the candidates by traversing from the two commits to be merged, we already make another set of traversals, starting from the candidates and painting the ancestors down to their common ancestors. This is done to discover if each candidate is reachable from any other candidate (in which case the reachable one is not a merge-base). The resulting graph of this traversal is currently used only to cull non-merge-bases out of the candidates, but I wonder if you can *count* the nodes in it in each color and use that number (which is essentially the number of commits that can be reached only from one candidate and not from other candidates) to derive a score for each candidate, and use it to assess the goodness of merge-bases, just like the number you are counting in the above full traversal. It sounds promising. Let's see if I correctly understand the algorithm that you described: common_ancestors = intersection of all candidates' histories painting_from($candidate) = $candidate - common_ancestors discard $candidate if $candidate is in painting_from($other_candidate) (i.e., it is not a merge base) If that is correct, then the candidate with the most commits in painting_from($candidate) should be the best merge base, because common_ancestors is a subset of the candidate's history, so |painting_from($candidate)| = |$candidate| - |common_ancestors| Since |common_ancestors| is the same for all candidates, minimizing |painting_from($candidate)| is the same as minimizing |$candidate|. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.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 10/16] fast-import: use skip_prefix for parsing input
On Fri, Jun 20, 2014 at 1:45 AM, Jeff King p...@peff.net wrote: On Thu, Jun 19, 2014 at 11:19:09PM -0400, Eric Sunshine wrote: - if (starts_with(command_buf.buf, M )) - file_change_m(b); - else if (starts_with(command_buf.buf, D )) - file_change_d(b); - else if (starts_with(command_buf.buf, R )) - file_change_cr(b, 1); - else if (starts_with(command_buf.buf, C )) - file_change_cr(b, 0); - else if (starts_with(command_buf.buf, N )) - note_change_n(b, prev_fanout); + const char *v; This declaration of 'v' shadows the 'v' added by patch 8/16 earlier in the function. Thanks. I reordered the patches before sending, so when this one was originally written, there was no v at the top-level of the function. I think we can just drop this interior one. The point of the short v is that it can be used as a temporary value for prefix matches, so I think we can just reuse the same one. Agreed. The intended usage of 'v' is clear enough and the code simple enough that confusion is unlikely. I tried compiling with -Wshadow (which I don't usually do), but we're not even close to compiling clean there. Some of them are legitimately confusing (e.g., try figuring out end in parse_rev_note). But others look just annoying (e.g., complaining that a local usage conflicts with the global function). I don't know if we want to put effort into being -Wshadow clean or not. I just happened to notice the shadowing declaration while reading the patch, but don't feel strongly about existing cases. It makes sense to clean up confusing cases, such 'end' in parse_rev_note(), when working on that code (just as with style cleanups), but thus far nobody has been complaining about existing shadowed variables, so global cleanup would likely be considered churn. -- 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
git gui - Linux
Hello, I am trying to initialize remote git and push complete folder files using git-gui. I'm Doing exactly the same sequence in windows with Git Gui works perfectly. But when doing remote-add in ubuntu, name: test Location: /home/ubuntu/test.git I get the following error: fatal: GIT_WORK_TREE (or --work-tree=) not allowed without specifying GIT_DIR (or --git-dir=) Should I set only GIT_DIR=/home/ubuntu/test.git? And after doing that, repeat the remote-add step giving the name and the same folder /home/ubuntu/test.git again ? Thanks, Ran -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: The different EOL behavior between libgit2-based software and official Git
Wow! P.S. libgit2 just has a PR that try to be identical with official git. See https://github.com/libgit2/libgit2/pull/2432 Yue Lin Ho I am not sure how much problems Git/libgit2 have with files contains mixed LF-CRLF, as I have the same problem with the LF.txt The handling, according to my understandig, is: When core.eol is CRLF (or native under Windows) and core.autocrlf is true, and a file is checked out: If a file has CRLF in one line in the repo, nothing is changed. If a file has LF in one line in the repo, LF is converted into CRLF in the workspace. But here at my systems this doesn't seem to work as expected either for LF.txt: tb@mac:~/EOL_Test/TestAutoCrlf t=LF.txtrm -f $t git -c core.eol=CRLF checkout $t od -c $t 000L i n e 1 \r \n l i n e ( 2 ) 020 \r \n l i n e 3 . \r \n t h i s 040i s l i n e 4 \r \n l i n 060e N o . 5 \r \n L i n e N 100u m b e r 6 \r \n \r \n == tb@Linux:~/EOL_Test/TestAutoCrlf$ t=LF.txtrm -f $t git -c core.eol=CRLF checkout $t od -c $t 000 L i n e 1 \n l i n e ( 2 ) \n 020 l i n e 3 . \n t h i s i s 040 l i n e 4 \n l i n e N 060 o . 5 \n L i n e N u m b e r 100 6 \n \n -- 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 1/7] rebase -i: Make option handling in pick_one more flexible
On 06/19/2014 05:28 AM, Fabian Ruch wrote:
`pick_one` and `pick_one_preserving_merges` are wrappers around
`cherry-pick` in `rebase --interactive`. They take the hash of a commit
and build a `cherry-pick` command line that
- respects the user-supplied merge options
- disables complaints about empty commits
- tries to fast-forward the rebase head unless rebase is forced
- suppresses output unless the user requested higher verbosity
- rewrites merge commits to point to their rebased parents.
`pick_one` is used to implement not only `pick` but also `squash`, which
amends the previous commit rather than creating a new one. When
`pick_one` is called from `squash`, it receives a second argument `-n`.
This tells `pick_one` to apply the changes to the index without
committing them. Since the argument is almost directly passed to
`cherry-pick`, we might want to do the same with other `cherry-pick`
options. Currently, `pick_one` expects `-n` to be the first and only
argument except for the commit hash.
Prepare `pick_one` for additional `cherry-pick` options by allowing `-n`
to appear anywhere before the commit hash in the argument list. Loop
over the argument list and pop each handled item until the commit hash
is the only parameter left on the list. If an option is not supported,
ignore it and issue a warning on the console. Construct a new arguments
list `extra_args` of recognized options that shall be passed to
`cherry-pick` on the command line.
Signed-off-by: Fabian Ruch baf...@gmail.com
---
git-rebase--interactive.sh | 61
++
1 file changed, 45 insertions(+), 16 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f267d8b..ea5514e 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -237,8 +237,26 @@ git_sequence_editor () {
pick_one () {
ff=--ff
+ extra_args=
+ while test $# -gt 0
+ do
+ case $1 in
+ -n)
+ ff=
+ extra_args=$extra_args -n
+ ;;
+ -*)
+ warn pick_one: ignored option -- $1
+ ;;
This is an internal interface, right? I.e., user input isn't being
processed here? If so, then the presence of an unrecognized option is a
bug and it is preferable to die here rather than warn.
The same below and in at least one later commit.
+ *)
+ break
+ ;;
+ esac
+ shift
+ done
+ test $# -ne 1 die pick_one: wrong number of arguments
+ sha1=$1
- case $1 in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac
case $force_rebase in '') ;; ?*) ff= ;; esac
output git rev-parse --verify $sha1 || die Invalid commit name: $sha1
@@ -248,24 +266,35 @@ pick_one () {
fi
test -d $rewritten
- pick_one_preserving_merges $@ return
+ pick_one_preserving_merges $extra_args $sha1 return
output eval git cherry-pick \
${gpg_sign_opt:+$(git rev-parse --sq-quote
$gpg_sign_opt)} \
- $strategy_args $empty_args $ff $@
+ $strategy_args $empty_args $ff $extra_args $sha1
}
It might be confusing that extra_args is used both in pick_one and in
pick_one_preserving_merges. Since these are not local variables, the
call to the latter changes the value of the variable in the former. I
don't know if that could be a problem now (can
pick_one_preserving_merges return with a nonzero exit code?) but even if
not, it is a trap for future developers. I recommend giving the two
variables different names.
pick_one_preserving_merges () {
fast_forward=t
- case $1 in
- -n)
- fast_forward=f
- sha1=$2
- ;;
- *)
- sha1=$1
- ;;
- esac
- sha1=$(git rev-parse $sha1)
+ no_commit=
+ extra_args=
+ while test $# -gt 0
+ do
+ case $1 in
+ -n)
+ fast_forward=f
+ extra_args=$extra_args -n
+ no_commit=y
+ ;;
+ -*)
+ warn pick_one_preserving_merges: ignored option -- $1
+ ;;
+ *)
+ break
+ ;;
+ esac
+ shift
+ done
+ test $# -ne 1 die pick_one_preserving_merges: wrong number of
arguments
+ sha1=$(git rev-parse $1)
if test -f $state_dir/current-commit
then
@@ -335,7 +364,7 @@ pick_one_preserving_merges () {
f)
first_parent=$(expr $new_parents : ' \([^ ]*\)')
- if [ $1 != -n ]
+ if test -z $no_commit
then
# detach HEAD to current parent
Re: [RFC PATCH 2/7] rebase -i: Teach do_pick the option --edit
On 06/19/2014 05:28 AM, Fabian Ruch wrote:
The to-do list command `reword` replays a commit like `pick` but lets
the user also edit the commit's log message. If one thinks of `pick`
entries as scheduled `cherry-pick` command lines, then `reword` becomes
an alias for the command line `cherry-pick --edit`. The porcelain
`rebase--interactive` defines a function `do_pick` for processing the
`pick` entries on to-do lists. Teach `do_pick` to handle the option
`--edit` and reimplement `reword` in terms of `do_pick --edit`. Refer to
`pick_one` for the way options are parsed.
`do_pick` ought to act as a wrapper around `cherry-pick`. Unfortunately,
it cannot just forward `--edit` to the `cherry-pick` command line. The
assembled command line is executed within a command substitution for
controlling the verbosity of `rebase--interactive`. Passing `--edit`
would either hang the terminal or clutter the substituted command output
with control sequences. Execute the `reword` code from `do_next` instead
if the option `--edit` is specified. Adjust the fragment in two regards.
Firstly, discard the additional message which is printed if an error
occurs because
Aborting commit due to empty commit message. (Duplicate Signed-off-by
lines.)
Could not amend commit after successfully picking 1234567... Some change
is more readable than and contains (almost) the same information as
Aborting commit due to empty commit message. (Duplicate Signed-off-by
lines.)
Could not amend commit after successfully picking 1234567... Some change
This is most likely due to an empty commit message, or the pre-commit hook
failed. If the pre-commit hook failed, you may need to resolve the issue
before
you are able to reword the commit.
(It is true that a hook might not output any diagnosis but the same
problem arises when using git-commit directly. git-rebase at least
prints a generic message saying that it failed to commit.) Secondly,
sneak in additional git-commit arguments:
- `--allow-empty` is missing: `rebase--interactive` suddenly fails if
an empty commit is picked using `reword` instead of `pick`. The
whether a commit is empty or not is not changed by an altered log
message, so do as `pick` does. Add test.
- `-n`: Hide the fact that we are committing several times by not
executing the pre-commit hook. Caveat: The altered log message is not
verified because `-n` also skips the commit-msg hook. Either the log
message verification must be included in the post-rewrite hook or
git-commit must support more fine-grained control over which hooks
are executed.
- `-q`: Hide the fact that we are committing several times by not
printing the commit summary.
It is preferable that each commit makes one logical change (though it
must always be a self-contained change; i.e., the code should never be
broken at the end of a commit). It would be clearer if you would split
this commit into one refactoring commit (moving the handling of --edit
to do_pick) plus one commit for each git commit option change and
error message change. That way,
* Each commit (and log message) becomes simpler, making it easier
to review.
* The changes can be discussed separately.
* If there is an error, git bisect can help determine which of
the changes is at fault.
Signed-off-by: Fabian Ruch baf...@gmail.com
---
git-rebase--interactive.sh| 52
---
t/t3404-rebase-interactive.sh | 8 +++
2 files changed, 52 insertions(+), 8 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index ea5514e..fffdfa5 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -490,7 +490,42 @@ record_in_rewritten() {
esac
}
+# Apply the changes introduced by the given commit to the current head.
+#
+# do_pick [--edit] commit title
+#
+# Wrapper around git-cherry-pick.
+#
+# title
+# The commit message title of commit. Used for information
+# purposes only.
+#
+# commit
+# The commit to cherry-pick.
Unless there is a reason to do otherwise, please order the documentation
to match the order that the do_pick arguments appear.
+#
+# -e, --edit
+# After picking commit, open an editor and let the user edit the
+# commit message. The editor contents becomes the commit message of
+# the new head.
do_pick () {
+ edit=
+ while test $# -gt 0
+ do
+ case $1 in
+ -e|--edit)
+ edit=y
+ ;;
+ -*)
+ warn do_pick: ignored option -- $1
+ ;;
+ *)
+ break
+ ;;
+ esac
+ shift
+ done
+ test $# -ne 2 die do_pick: wrong number of arguments
+
if test $(git rev-parse HEAD) = $squash_onto
then
[PATCH v20 04/48] refs.c: allow passing NULL to ref_transaction_free
Allow ref_transaction_free(NULL) as a no-op. This makes ref_transaction_free
easier to use and more similar to plain 'free'.
In particular, it lets us rollback unconditionally as part of cleanup code
after setting 'transaction = NULL' if a transaction has been committed or
rolled back already.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/refs.c b/refs.c
index 21ed465..1d6dece 100644
--- a/refs.c
+++ b/refs.c
@@ -3338,6 +3338,9 @@ void ref_transaction_free(struct ref_transaction
*transaction)
{
int i;
+ if (!transaction)
+ return;
+
for (i = 0; i transaction-nr; i++)
free(transaction-updates[i]);
--
2.0.0.420.g181e020.dirty
--
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 v20 00/48] Use ref transactions
This patch series can also be found at https://github.com/rsahlberg/git/tree/ref-transactions This patch series is based on current master and expands on the transaction API. It converts all ref updates, inside refs.c as well as external, to use the transaction API for updates. This makes most of the ref updates to become atomic when there are failures locking or writing to a ref. This version completes the work to convert all ref updates to use transactions. Now that all updates are through transactions I will start working on cleaning up the reading of refs and to create an api for managing reflogs but all that will go in a different patch series. Version 20: - Whitespace and style changes suggested by Jun. Ronnie Sahlberg (48): refs.c: remove ref_transaction_rollback refs.c: ref_transaction_commit should not free the transaction refs.c: constify the sha arguments for ref_transaction_create|delete|update refs.c: allow passing NULL to ref_transaction_free refs.c: add a strbuf argument to ref_transaction_commit for error logging lockfile.c: add a new public function unable_to_lock_message lockfile.c: make lock_file return a meaningful errno on failurei refs.c: add an err argument to repack_without_refs refs.c: make sure log_ref_setup returns a meaningful errno refs.c: verify_lock should set errno to something meaningful refs.c: make remove_empty_directories always set errno to something sane refs.c: commit_packed_refs to return a meaningful errno on failure refs.c: make resolve_ref_unsafe set errno to something meaningful on error refs.c: log_ref_write should try to return meaningful errno refs.c: make ref_update_reject_duplicates take a strbuf argument for errors refs.c: make update_ref_write update a strbuf on failure update-ref: use err argument to get error from ref_transaction_commit refs.c: remove the onerr argument to ref_transaction_commit refs.c: change ref_transaction_update() to do error checking and return status 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/ERROR 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 refs.c: add an err argument to delete_ref_loose refs.c: pass the ref log message to _create/delete/update instead of _commit refs.c: pass NULL as *flags to read_ref_full 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: propagate any errno==ENOTDIR from _commit back to the callers fetch.c: change s_update_ref to use a ref transaction refs.c: make write_ref_sha1 static branch.c | 30 +-- builtin/commit.c | 24 ++- builtin/fetch.c| 36 ++-- builtin/receive-pack.c | 97 ++ builtin/remote.c | 5 +- builtin/replace.c | 15 +- builtin/tag.c | 15 +- builtin/update-ref.c | 34 ++-- cache.h| 4 +- fast-import.c | 53 -- lockfile.c | 39 ++-- refs.c | 506 - refs.h | 131 + sequencer.c| 24 ++- walker.c | 58 +++--- wrapper.c | 14 +- 16 files changed, 710 insertions(+), 375 deletions(-) -- 2.0.0.420.g181e020.dirty -- 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 v20 05/48] refs.c: add a strbuf argument to ref_transaction_commit for error logging
Add a strbuf argument to _commit so that we can pass an error string back to
the caller. So that we can do error logging from the caller instead of from
_commit.
Longer term plan is to first convert all callers to use onerr==QUIET_ON_ERR
and craft any log messages from the callers themselves and finally remove the
onerr argument completely.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 2 +-
refs.c | 6 +-
refs.h | 5 -
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 1fd7a89..22617af 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -367,7 +367,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
if (end_null)
line_termination = '\0';
update_refs_stdin();
- ret = ref_transaction_commit(transaction, msg,
+ ret = ref_transaction_commit(transaction, msg, NULL,
UPDATE_REFS_DIE_ON_ERR);
ref_transaction_free(transaction);
return ret;
diff --git a/refs.c b/refs.c
index 1d6dece..db05602 100644
--- a/refs.c
+++ b/refs.c
@@ -3444,7 +3444,8 @@ static int ref_update_reject_duplicates(struct ref_update
**updates, int n,
}
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, enum action_on_err onerr)
+ const char *msg, struct strbuf *err,
+ enum action_on_err onerr)
{
int ret = 0, delnum = 0, i;
const char **delnames;
@@ -3473,6 +3474,9 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
update-flags,
update-type, onerr);
if (!update-lock) {
+ if (err)
+ strbuf_addf(err, Cannot lock the ref '%s'.,
+ update-refname);
ret = 1;
goto cleanup;
}
diff --git a/refs.h b/refs.h
index 8c7f9c4..09d3564 100644
--- a/refs.h
+++ b/refs.h
@@ -269,9 +269,12 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
* Commit all of the changes that have been queued in transaction, as
* atomically as possible. Return a nonzero value if there is a
* problem.
+ * If err is non-NULL we will add an error string to it to explain why
+ * the transaction failed. The string does not end in newline.
*/
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, enum action_on_err onerr);
+ const char *msg, struct strbuf *err,
+ enum action_on_err onerr);
/*
* Free an existing transaction and all associated data.
--
2.0.0.420.g181e020.dirty
--
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 v20 07/48] lockfile.c: make lock_file return a meaningful errno on failurei
Making errno when returning from lock_file() meaningful, which should
fix
* an existing almost-bug in lock_ref_sha1_basic where it assumes
errno==ENOENT is meaningful and could waste some work on retries
* an existing bug in repack_without_refs where it prints
strerror(errno) and picks advice based on errno, despite errno
potentially being zero and potentially having been clobbered by
that point
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
lockfile.c | 17 -
refs.c | 1 +
refs.h | 1 +
3 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index 464031b..a921d77 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -121,7 +121,7 @@ static char *resolve_symlink(char *p, size_t s)
return p;
}
-
+/* Make sure errno contains a meaningful value on error */
static int lock_file(struct lock_file *lk, const char *path, int flags)
{
/*
@@ -130,8 +130,10 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
*/
static const size_t max_path_len = sizeof(lk-filename) - 5;
- if (strlen(path) = max_path_len)
+ if (strlen(path) = max_path_len) {
+ errno = ENAMETOOLONG;
return -1;
+ }
strcpy(lk-filename, path);
if (!(flags LOCK_NODEREF))
resolve_symlink(lk-filename, max_path_len);
@@ -148,9 +150,13 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
lock_file_list = lk;
lk-on_list = 1;
}
- if (adjust_shared_perm(lk-filename))
- return error(cannot fix permission bits on %s,
-lk-filename);
+ if (adjust_shared_perm(lk-filename)) {
+ int save_errno = errno;
+ error(cannot fix permission bits on %s,
+ lk-filename);
+ errno = save_errno;
+ return -1;
+ }
}
else
lk-filename[0] = 0;
@@ -188,6 +194,7 @@ NORETURN void unable_to_lock_index_die(const char *path,
int err)
die(%s, buf.buf);
}
+/* This should return a meaningful errno on failure */
int hold_lock_file_for_update(struct lock_file *lk, const char *path, int
flags)
{
int fd = lock_file(lk, path, flags);
diff --git a/refs.c b/refs.c
index db05602..e9d53e4 100644
--- a/refs.c
+++ b/refs.c
@@ -2212,6 +2212,7 @@ static int write_packed_entry_fn(struct ref_entry *entry,
void *cb_data)
return 0;
}
+/* This should return a meaningful errno on failure */
int lock_packed_refs(int flags)
{
struct packed_ref_cache *packed_ref_cache;
diff --git a/refs.h b/refs.h
index 09d3564..64f25d9 100644
--- a/refs.h
+++ b/refs.h
@@ -82,6 +82,7 @@ extern void warn_dangling_symrefs(FILE *fp, const char
*msg_fmt, const struct st
/*
* Lock the packed-refs file for writing. Flags is passed to
* hold_lock_file_for_update(). Return 0 on success.
+ * Errno is set to something meaningful on error.
*/
extern int lock_packed_refs(int flags);
--
2.0.0.420.g181e020.dirty
--
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 v20 12/48] refs.c: commit_packed_refs to return a meaningful errno on failure
Making errno when returning from commit_packed_refs() meaningful,
which should fix
* a bug in git clone where it prints strerror(errno) based on
errno, despite errno possibly being zero and potentially having
been clobbered by that point
* the same kind of bug in git pack-refs
and prepares for repack_without_refs() to get a meaningful
error message when commit_packed_refs() fails without falling into
the same bug.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 10 +-
refs.h | 1 +
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index cc69581..7a815be 100644
--- a/refs.c
+++ b/refs.c
@@ -2239,11 +2239,16 @@ int lock_packed_refs(int flags)
return 0;
}
+/*
+ * Commit the packed refs changes.
+ * On error we must make sure that errno contains a meaningful value.
+ */
int commit_packed_refs(void)
{
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(ref_cache);
int error = 0;
+ int save_errno = 0;
if (!packed_ref_cache-lock)
die(internal error: packed-refs not locked);
@@ -2253,10 +2258,13 @@ int commit_packed_refs(void)
do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),
0, write_packed_entry_fn,
packed_ref_cache-lock-fd);
- if (commit_lock_file(packed_ref_cache-lock))
+ if (commit_lock_file(packed_ref_cache-lock)) {
+ save_errno = errno;
error = -1;
+ }
packed_ref_cache-lock = NULL;
release_packed_ref_cache(packed_ref_cache);
+ errno = save_errno;
return error;
}
diff --git a/refs.h b/refs.h
index 8d6cac7..e588ff8 100644
--- a/refs.h
+++ b/refs.h
@@ -98,6 +98,7 @@ extern void add_packed_ref(const char *refname, const
unsigned char *sha1);
* Write the current version of the packed refs cache from memory to
* disk. The packed-refs file must already be locked for writing (see
* lock_packed_refs()). Return zero on success.
+ * Sets errno to something meaningful on error.
*/
extern int commit_packed_refs(void);
--
2.0.0.420.g181e020.dirty
--
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 v20 02/48] refs.c: ref_transaction_commit should not free the transaction
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 1 +
refs.c | 1 -
refs.h | 5 ++---
3 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 405267f..1fd7a89 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -369,6 +369,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
update_refs_stdin();
ret = ref_transaction_commit(transaction, msg,
UPDATE_REFS_DIE_ON_ERR);
+ ref_transaction_free(transaction);
return ret;
}
diff --git a/refs.c b/refs.c
index 6d841a0..d9cac6d 100644
--- a/refs.c
+++ b/refs.c
@@ -3509,7 +3509,6 @@ cleanup:
if (updates[i]-lock)
unlock_ref(updates[i]-lock);
free(delnames);
- ref_transaction_free(transaction);
return ret;
}
diff --git a/refs.h b/refs.h
index cfd1832..7d946f6 100644
--- a/refs.h
+++ b/refs.h
@@ -219,8 +219,7 @@ enum action_on_err {
/*
* Begin a reference transaction. The reference transaction must
- * eventually be commited using ref_transaction_commit() or freed by
- * calling ref_transaction_free().
+ * be freed by calling ref_transaction_free().
*/
struct ref_transaction *ref_transaction_begin(void);
@@ -268,7 +267,7 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
/*
* Commit all of the changes that have been queued in transaction, as
* atomically as possible. Return a nonzero value if there is a
- * problem. The ref_transaction is freed by this function.
+ * problem.
*/
int ref_transaction_commit(struct ref_transaction *transaction,
const char *msg, enum action_on_err onerr);
--
2.0.0.420.g181e020.dirty
--
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 v20 44/48] refs.c: call lock_ref_sha1_basic directly from commit
Skip using the lock_any_ref_for_update wrapper and call lock_ref_sha1_basic
directly from the commit function.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index bccf8c3..f3fab37 100644
--- a/refs.c
+++ b/refs.c
@@ -3598,12 +3598,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i n; i++) {
struct ref_update *update = updates[i];
- update-lock = lock_any_ref_for_update(update-refname,
- (update-have_old ?
- update-old_sha1 :
- NULL),
- update-flags,
- update-type);
+ update-lock = lock_ref_sha1_basic(update-refname,
+ (update-have_old ?
+ update-old_sha1 :
+ NULL),
+ update-flags,
+ update-type);
if (!update-lock) {
if (err)
strbuf_addf(err, Cannot lock the ref '%s'.,
--
2.0.0.420.g181e020.dirty
--
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 v20 39/48] refs.c: make delete_ref use a transaction
Change delete_ref to use a ref transaction for the deletion. At the same time
since we no longer have any callers of repack_without_ref we can now delete
this function.
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.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 34 +-
1 file changed, 13 insertions(+), 21 deletions(-)
diff --git a/refs.c b/refs.c
index 3d070d5..92a06d4 100644
--- a/refs.c
+++ b/refs.c
@@ -2544,11 +2544,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) {
@@ -2566,24 +2561,21 @@ 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);
+ return 0;
}
/*
--
2.0.0.420.g181e020.dirty
--
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 v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic
Move the check for check_refname_format from lock_any_ref_for_update
to lock_ref_sha1_basic. At some later stage we will get rid of
lock_any_ref_for_update completely.
If lock_ref_sha1_basic fails the check_refname_format test, set errno to
EINVAL before returning NULL. This to guarantee that we will not return an
error without updating errno.
This leaves lock_any_ref_for_updates as a no-op wrapper which could be removed.
But this wrapper is also called from an external caller and we will soon
make changes to the signature to lock_ref_sha1_basic that we do not want to
expose to that caller.
This changes semantics for lock_ref_sha1_basic slightly. With this change
it is no longer possible to open a ref that has a badly name which breaks
any codepaths that tries to open and repair badly named refs. The normal refs
API should not allow neither creating nor accessing refs with invalid names.
If we need such recovery code we could add it as an option to git fsck and have
git fsck be the only sanctioned way of bypassing the normal API and checks.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index 389a55f..bccf8c3 100644
--- a/refs.c
+++ b/refs.c
@@ -2088,6 +2088,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;
@@ -2179,8 +2184,6 @@ struct ref_lock *lock_any_ref_for_update(const char
*refname,
const unsigned char *old_sha1,
int flags, int *type_p)
{
- if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
- return NULL;
return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
}
--
2.0.0.420.g181e020.dirty
--
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 v20 15/48] refs.c: make ref_update_reject_duplicates take a strbuf argument for errors
Make ref_update_reject_duplicates return any error that occurs through a
new strbuf argument. This means that when a transaction commit fails in
this function we will now be able to pass a helpful error message back to the
caller.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 61570c9..115f143 100644
--- a/refs.c
+++ b/refs.c
@@ -3488,6 +3488,7 @@ static int ref_update_compare(const void *r1, const void
*r2)
}
static int ref_update_reject_duplicates(struct ref_update **updates, int n,
+ struct strbuf *err,
enum action_on_err onerr)
{
int i;
@@ -3495,6 +3496,9 @@ static int ref_update_reject_duplicates(struct ref_update
**updates, int n,
if (!strcmp(updates[i - 1]-refname, updates[i]-refname)) {
const char *str =
Multiple updates for ref '%s' not allowed.;
+ if (err)
+ strbuf_addf(err, str, updates[i]-refname);
+
switch (onerr) {
case UPDATE_REFS_MSG_ON_ERR:
error(str, updates[i]-refname); break;
@@ -3525,7 +3529,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Copy, sort, and reject duplicate refs */
qsort(updates, n, sizeof(*updates), ref_update_compare);
- ret = ref_update_reject_duplicates(updates, n, onerr);
+ ret = ref_update_reject_duplicates(updates, n, err, onerr);
if (ret)
goto cleanup;
--
2.0.0.420.g181e020.dirty
--
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 v20 42/48] refs.c: pass NULL as *flags to read_ref_full
We call read_ref_full with a pointer to flags from rename_ref but since
we never actually use the returned flags we can just pass NULL here instead.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 1f17a13..389a55f 100644
--- a/refs.c
+++ b/refs.c
@@ -2688,7 +2688,7 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
goto rollback;
}
- if (!read_ref_full(newrefname, sha1, 1, flag)
+ if (!read_ref_full(newrefname, sha1, 1, NULL)
delete_ref(newrefname, sha1, REF_NODEREF)) {
if (errno==EISDIR) {
if (remove_empty_directories(git_path(%s,
newrefname))) {
--
2.0.0.420.g181e020.dirty
--
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 v20 38/48] refs.c: make prune_ref use a transaction to delete the ref
Change prune_ref to delete the ref using a ref transaction. To do this we also
need to add a new flag REF_ISPRUNING that will tell the transaction that we
do not want to delete this ref from the packed refs. This flag is private to
refs.c and not exposed to external callers.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 27 ---
refs.h | 14 --
2 files changed, 32 insertions(+), 9 deletions(-)
diff --git a/refs.c b/refs.c
index 699f1f6..3d070d5 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
@@ -2379,17 +2384,24 @@ static void try_remove_empty_parents(char *name)
/* make sure nobody touched the ref, and unlink */
static void prune_ref(struct ref_to_prune *r)
{
- struct ref_lock *lock;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
if (check_refname_format(r-name + 5, 0))
return;
- lock = lock_ref_sha1_basic(r-name, r-sha1, 0, NULL);
- if (lock) {
- unlink_or_warn(git_path(%s, r-name));
- unlock_ref(lock);
- try_remove_empty_parents(r-name);
+ transaction = ref_transaction_begin(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);
+ try_remove_empty_parents(r-name);
}
static void prune_refs(struct ref_to_prune *r)
@@ -3599,8 +3611,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 4ac4a7d..3b321c2 100644
--- a/refs.h
+++ b/refs.h
@@ -177,9 +177,19 @@ 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 */
+/*
+ * Locks any ref (for 'HEAD' type refs) and 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.0.0.420.g181e020.dirty
--
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 v20 01/48] refs.c: remove ref_transaction_rollback
We do not yet need both a rollback and a free function for transactions.
Remove ref_transaction_rollback and use ref_transaction_free instead.
At a later stage we may reintroduce a rollback function if we want to start
adding reusable transactions and similar.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 7 +--
refs.h | 16 +++-
2 files changed, 8 insertions(+), 15 deletions(-)
diff --git a/refs.c b/refs.c
index dc45774..6d841a0 100644
--- a/refs.c
+++ b/refs.c
@@ -3334,7 +3334,7 @@ struct ref_transaction *ref_transaction_begin(void)
return xcalloc(1, sizeof(struct ref_transaction));
}
-static void ref_transaction_free(struct ref_transaction *transaction)
+void ref_transaction_free(struct ref_transaction *transaction)
{
int i;
@@ -3345,11 +3345,6 @@ static void ref_transaction_free(struct ref_transaction
*transaction)
free(transaction);
}
-void ref_transaction_rollback(struct ref_transaction *transaction)
-{
- ref_transaction_free(transaction);
-}
-
static struct ref_update *add_update(struct ref_transaction *transaction,
const char *refname)
{
diff --git a/refs.h b/refs.h
index 4e3050d..cfd1832 100644
--- a/refs.h
+++ b/refs.h
@@ -219,18 +219,12 @@ enum action_on_err {
/*
* Begin a reference transaction. The reference transaction must
- * eventually be commited using ref_transaction_commit() or rolled
- * back using ref_transaction_rollback().
+ * eventually be commited using ref_transaction_commit() or freed by
+ * calling ref_transaction_free().
*/
struct ref_transaction *ref_transaction_begin(void);
/*
- * Roll back a ref_transaction and free all associated data.
- */
-void ref_transaction_rollback(struct ref_transaction *transaction);
-
-
-/*
* The following functions add a reference check or update to a
* ref_transaction. In all of them, refname is the name of the
* reference to be affected. The functions make internal copies of
@@ -238,7 +232,6 @@ void ref_transaction_rollback(struct ref_transaction
*transaction);
* can be REF_NODEREF; it is passed to update_ref_lock().
*/
-
/*
* Add a reference update to transaction. new_sha1 is the value that
* the reference should have after the update, or zeros if it should
@@ -280,6 +273,11 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
int ref_transaction_commit(struct ref_transaction *transaction,
const char *msg, enum action_on_err onerr);
+/*
+ * Free an existing transaction and all associated data.
+ */
+void ref_transaction_free(struct ref_transaction *transaction);
+
/** Lock a ref and then write its file */
int update_ref(const char *action, const char *refname,
const unsigned char *sha1, const unsigned char *oldval,
--
2.0.0.420.g181e020.dirty
--
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 v20 08/48] refs.c: add an err argument to repack_without_refs
Update repack_without_refs to take an err argument and update it if there
is a failure. Pass the err variable from ref_transaction_commit to this
function so that callers can print a meaningful error message if _commit
fails due to this function.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/remote.c | 5 +++--
refs.c | 19 ++-
refs.h | 3 ++-
3 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index c9102e8..401feb3 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -755,7 +755,7 @@ static int remove_branches(struct string_list *branches)
branch_names = xmalloc(branches-nr * sizeof(*branch_names));
for (i = 0; i branches-nr; i++)
branch_names[i] = branches-items[i].string;
- result |= repack_without_refs(branch_names, branches-nr);
+ result |= repack_without_refs(branch_names, branches-nr, NULL);
free(branch_names);
for (i = 0; i branches-nr; i++) {
@@ -1333,7 +1333,8 @@ static int prune_remote(const char *remote, int dry_run)
for (i = 0; i states.stale.nr; i++)
delete_refs[i] = states.stale.items[i].util;
if (!dry_run)
- result |= repack_without_refs(delete_refs,
states.stale.nr);
+ result |= repack_without_refs(delete_refs,
+ states.stale.nr, NULL);
free(delete_refs);
}
diff --git a/refs.c b/refs.c
index e9d53e4..67a0217 100644
--- a/refs.c
+++ b/refs.c
@@ -2456,12 +2456,12 @@ static int curate_packed_ref_fn(struct ref_entry
*entry, void *cb_data)
return 0;
}
-int repack_without_refs(const char **refnames, int n)
+int repack_without_refs(const char **refnames, int n, struct strbuf *err)
{
struct ref_dir *packed;
struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
struct string_list_item *ref_to_delete;
- int i, removed = 0;
+ int i, ret, removed = 0;
/* Look for a packed ref */
for (i = 0; i n; i++)
@@ -2473,6 +2473,11 @@ int repack_without_refs(const char **refnames, int n)
return 0; /* no refname exists in packed refs */
if (lock_packed_refs(0)) {
+ if (err) {
+ unable_to_lock_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]);
}
@@ -2499,12 +2504,16 @@ int repack_without_refs(const char **refnames, int n)
}
/* Write what remains */
- return commit_packed_refs();
+ ret = commit_packed_refs();
+ if (ret err)
+ strbuf_addf(err, unable to overwrite old ref-pack file: %s,
+ strerror(errno));
+ return ret;
}
static int repack_without_ref(const char *refname)
{
- return repack_without_refs(refname, 1);
+ return repack_without_refs(refname, 1, NULL);
}
static int delete_ref_loose(struct ref_lock *lock, int flag)
@@ -3508,7 +3517,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
}
}
- ret |= repack_without_refs(delnames, delnum);
+ ret |= repack_without_refs(delnames, delnum, err);
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 64f25d9..65f7637 100644
--- a/refs.h
+++ b/refs.h
@@ -122,7 +122,8 @@ extern void rollback_packed_refs(void);
*/
int pack_refs(unsigned int flags);
-extern int repack_without_refs(const char **refnames, int n);
+extern int repack_without_refs(const char **refnames, int n,
+ struct strbuf *err);
extern int ref_exists(const char *);
--
2.0.0.420.g181e020.dirty
--
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 v20 47/48] fetch.c: change s_update_ref to use a ref transaction
Change s_update_ref to use a ref transaction for the ref update.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/fetch.c | 33 +++--
1 file changed, 23 insertions(+), 10 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index faa1233..52f1ebc 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -375,23 +375,36 @@ 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);
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.0.0.420.g181e020.dirty
--
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 v20 48/48] refs.c: make write_ref_sha1 static
No external users call write_ref_sha1 any more so lets declare it static.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 10 --
refs.h | 3 ---
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/refs.c b/refs.c
index 74a55b5..5a0b138 100644
--- a/refs.c
+++ b/refs.c
@@ -2665,6 +2665,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];
@@ -2914,8 +2917,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 5c0543d..db463d0 100644
--- a/refs.h
+++ b/refs.h
@@ -203,9 +203,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.0.0.420.g181e020.dirty
--
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 v20 03/48] refs.c: constify the sha arguments for ref_transaction_create|delete|update
ref_transaction_create|delete|update has no need to modify the sha1
arguments passed to it so it should use const unsigned char* instead
of unsigned char*.
Some functions, such as fast_forward_to(), already have its old/new
sha1 arguments as consts. This function will at some point need to
use ref_transaction_update() in which case this change is required.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 7 ---
refs.h | 7 ---
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index d9cac6d..21ed465 100644
--- a/refs.c
+++ b/refs.c
@@ -3359,7 +3359,8 @@ static struct ref_update *add_update(struct
ref_transaction *transaction,
void ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1, unsigned char *old_sha1,
+ const unsigned char *new_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old)
{
struct ref_update *update = add_update(transaction, refname);
@@ -3373,7 +3374,7 @@ void ref_transaction_update(struct ref_transaction
*transaction,
void ref_transaction_create(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1,
+ const unsigned char *new_sha1,
int flags)
{
struct ref_update *update = add_update(transaction, refname);
@@ -3387,7 +3388,7 @@ void ref_transaction_create(struct ref_transaction
*transaction,
void ref_transaction_delete(struct ref_transaction *transaction,
const char *refname,
- unsigned char *old_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old)
{
struct ref_update *update = add_update(transaction, refname);
diff --git a/refs.h b/refs.h
index 7d946f6..8c7f9c4 100644
--- a/refs.h
+++ b/refs.h
@@ -240,7 +240,8 @@ struct ref_transaction *ref_transaction_begin(void);
*/
void ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1, unsigned char *old_sha1,
+ const unsigned char *new_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old);
/*
@@ -251,7 +252,7 @@ void ref_transaction_update(struct ref_transaction
*transaction,
*/
void ref_transaction_create(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1,
+ const unsigned char *new_sha1,
int flags);
/*
@@ -261,7 +262,7 @@ void ref_transaction_create(struct ref_transaction
*transaction,
*/
void ref_transaction_delete(struct ref_transaction *transaction,
const char *refname,
- unsigned char *old_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old);
/*
--
2.0.0.420.g181e020.dirty
--
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 v20 40/48] refs.c: add an err argument to delete_ref_loose
Add an err argument to delete_loose_ref so that we can pass a descriptive
error string back to the caller. Pass the err argument from transaction
commit to this function so that transaction users will have a nice error
string if the transaction failed due to delete_loose_ref.
Add a new function unlink_or_err that we can call from delete_ref_loose. This
function is similar to unlink_or_warn except that we can pass it an err
argument. If err is non-NULL the function will populate err instead of
printing a warning().
Simplify warn_if_unremovable.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c| 33 -
wrapper.c | 14 ++
2 files changed, 34 insertions(+), 13 deletions(-)
diff --git a/refs.c b/refs.c
index 92a06d4..c7d1f3e 100644
--- a/refs.c
+++ b/refs.c
@@ -2544,16 +2544,38 @@ 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 add_err_if_unremovable(const char *op, const char *file,
+ struct strbuf *e, int rc)
+{
+ int err = errno;
+ if (rc 0 errno != ENOENT) {
+ strbuf_addf(e, unable to %s %s: %s,
+ op, file, strerror(errno));
+ errno = err;
+ return -1;
+ }
+ return 0;
+}
+
+static int unlink_or_err(const char *file, struct strbuf *err)
+{
+ if (err)
+ return add_err_if_unremovable(unlink, file, err,
+ unlink(file));
+ else
+ return unlink_or_warn(file);
+}
+
+static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf
*err)
{
if (!(flag REF_ISPACKED) || flag REF_ISSYMREF) {
/* loose */
- int err, i = strlen(lock-lk-filename) - 5; /* .lock */
+ int res, i = strlen(lock-lk-filename) - 5; /* .lock */
lock-lk-filename[i] = 0;
- err = unlink_or_warn(lock-lk-filename);
+ res = unlink_or_err(lock-lk-filename, err);
lock-lk-filename[i] = '.';
- if (err errno != ENOENT)
+ if (res)
return 1;
}
return 0;
@@ -3603,7 +3625,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;
}
diff --git a/wrapper.c b/wrapper.c
index bc1bfb8..740e193 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 = 0 || errno == ENOENT)
+ return rc;
+ err = errno;
+ warning(unable to %s %s: %s, op, file, strerror(errno));
+ errno = err;
return rc;
}
--
2.0.0.420.g181e020.dirty
--
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 v20 06/48] lockfile.c: add a new public function unable_to_lock_message
Introducing a new unable_to_lock_message helper, which has nicer
semantics than unable_to_lock_error and cleans up lockfile.c a little.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
cache.h| 2 ++
lockfile.c | 22 --
2 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/cache.h b/cache.h
index cbe1935..8b12aa8 100644
--- a/cache.h
+++ b/cache.h
@@ -559,6 +559,8 @@ struct lock_file {
#define LOCK_DIE_ON_ERROR 1
#define LOCK_NODEREF 2
extern int unable_to_lock_error(const char *path, int err);
+extern void unable_to_lock_message(const char *path, int err,
+ struct strbuf *buf);
extern NORETURN void unable_to_lock_index_die(const char *path, int err);
extern int hold_lock_file_for_update(struct lock_file *, const char *path,
int);
extern int hold_lock_file_for_append(struct lock_file *, const char *path,
int);
diff --git a/lockfile.c b/lockfile.c
index 8fbcb6a..464031b 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -157,33 +157,35 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
return lk-fd;
}
-static char *unable_to_lock_message(const char *path, int err)
+void unable_to_lock_message(const char *path, int err, struct strbuf *buf)
{
- struct strbuf buf = STRBUF_INIT;
-
if (err == EEXIST) {
- strbuf_addf(buf, Unable to create '%s.lock': %s.\n\n
+ strbuf_addf(buf, Unable to create '%s.lock': %s.\n\n
If no other git process is currently running, this
probably means a\n
git process crashed in this repository earlier. Make sure
no other git\n
process is running and remove the file manually to
continue.,
absolute_path(path), strerror(err));
} else
- strbuf_addf(buf, Unable to create '%s.lock': %s,
+ strbuf_addf(buf, Unable to create '%s.lock': %s,
absolute_path(path), strerror(err));
- return strbuf_detach(buf, NULL);
}
int unable_to_lock_error(const char *path, int err)
{
- char *msg = unable_to_lock_message(path, err);
- error(%s, msg);
- free(msg);
+ struct strbuf buf = STRBUF_INIT;
+
+ unable_to_lock_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)
{
- die(%s, unable_to_lock_message(path, err));
+ struct strbuf buf = STRBUF_INIT;
+
+ unable_to_lock_message(path, err, buf);
+ die(%s, buf.buf);
}
int hold_lock_file_for_update(struct lock_file *lk, const char *path, int
flags)
--
2.0.0.420.g181e020.dirty
--
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 v20 37/48] refs.c: remove lock_ref_sha1
lock_ref_sha1 was only called from one place in refc.c and only provided
a check that the refname was sane before adding back the initial refs/
part of the ref path name, the initial refs/ that this caller had already
stripped off before calling lock_ref_sha1.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 15 +--
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/refs.c b/refs.c
index 7113843..699f1f6 100644
--- a/refs.c
+++ b/refs.c
@@ -2170,15 +2170,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)
@@ -2388,8 +2379,12 @@ static void try_remove_empty_parents(char *name)
/* make sure nobody touched the ref, and unlink */
static void prune_ref(struct ref_to_prune *r)
{
- struct ref_lock *lock = lock_ref_sha1(r-name + 5, r-sha1);
+ struct ref_lock *lock;
+
+ if (check_refname_format(r-name + 5, 0))
+ return;
+ lock = lock_ref_sha1_basic(r-name, r-sha1, 0, NULL);
if (lock) {
unlink_or_warn(git_path(%s, r-name));
unlock_ref(lock);
--
2.0.0.420.g181e020.dirty
--
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 v20 46/48] refs.c: propagate any errno==ENOTDIR from _commit back to the callers
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.
For these cases, save the errno value and abort and make sure that the caller
can see errno==ENOTDIR.
Also start defining specific return codes for _commit, assign -1 as a generic
error and -2 as the error that refers to a name conflict. Callers can (and
should) use that return value inspecting errno directly.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 22 +++---
refs.h | 6 ++
2 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/refs.c b/refs.c
index c6990d0..74a55b5 100644
--- a/refs.c
+++ b/refs.c
@@ -3582,7 +3582,7 @@ static int ref_update_reject_duplicates(struct ref_update
**updates, int n,
int ref_transaction_commit(struct ref_transaction *transaction,
struct strbuf *err)
{
- int ret = 0, delnum = 0, i;
+ int ret = 0, delnum = 0, i, df_conflict = 0;
const char **delnames;
int n = transaction-nr;
struct ref_update **updates = transaction-updates;
@@ -3600,9 +3600,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++) {
@@ -3616,10 +3617,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
update-type,
delnames, delnum);
if (!update-lock) {
+ if (errno == ENOTDIR)
+ df_conflict = 1;
if (err)
strbuf_addf(err, Cannot lock the ref '%s'.,
update-refname);
- ret = 1;
+ ret = -1;
goto cleanup;
}
}
@@ -3637,6 +3640,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
if (err)
strbuf_addf(err, str, update-refname);
+ ret = -1;
goto cleanup;
}
}
@@ -3647,14 +3651,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);
@@ -3667,6 +3673,8 @@ cleanup:
if (updates[i]-lock)
unlock_ref(updates[i]-lock);
free(delnames);
+ if (df_conflict)
+ ret = -2;
return ret;
}
diff --git a/refs.h b/refs.h
index f24b2c1..5c0543d 100644
--- a/refs.h
+++ b/refs.h
@@ -333,7 +333,13 @@ 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.
+ * If the transaction is already in failed state this function will return
+ * an error.
+ * Function returns 0 on success, -1 for generic failures and
+ * UPDATE_REFS_NAME_CONFLICT (-2) if the failure was due to a name
+ * collision (ENOTDIR).
*/
+#define UPDATE_REFS_NAME_CONFLICT -2
int ref_transaction_commit(struct ref_transaction *transaction,
struct strbuf *err);
--
2.0.0.420.g181e020.dirty
--
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 v20 45/48] refs.c: pass a skip list to name_conflict_fn
Allow passing a list of refs to skip checking to name_conflict_fn.
There are some conditions where we want to allow a temporary conflict and skip
checking those refs. For example if we have a transaction that
1, guarantees that m is a packed refs and there is no loose ref for m
2, the transaction will delete m from the packed ref
3, the transaction will create conflicting m/m
For this case we want to be able to lock and create m/m since we know that the
conflict is only transient. I.e. the conflict will be automatically resolved
by the transaction when it deletes m.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 41 ++---
1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/refs.c b/refs.c
index f3fab37..c6990d0 100644
--- a/refs.c
+++ b/refs.c
@@ -801,15 +801,18 @@ static int names_conflict(const char *refname1, const
char *refname2)
struct name_conflict_cb {
const char *refname;
- const char *oldrefname;
const char *conflicting_refname;
+ const char **skip;
+ int skipnum;
};
static int name_conflict_fn(struct ref_entry *entry, void *cb_data)
{
struct name_conflict_cb *data = (struct name_conflict_cb *)cb_data;
- if (data-oldrefname !strcmp(data-oldrefname, entry-name))
- return 0;
+ int i;
+ for (i = 0; i data-skipnum; i++)
+ if (!strcmp(entry-name, data-skip[i]))
+ return 0;
if (names_conflict(data-refname, entry-name)) {
data-conflicting_refname = entry-name;
return 1;
@@ -822,15 +825,18 @@ static int name_conflict_fn(struct ref_entry *entry, void
*cb_data)
* conflicting with the name of an existing reference in dir. If
* oldrefname is non-NULL, ignore potential conflicts with oldrefname
* (e.g., because oldrefname is scheduled for deletion in the same
- * operation).
+ * operation). skip contains a list of refs we want to skip checking for
+ * conflicts with.
*/
-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,
+ const char **skip, int skipnum)
{
struct name_conflict_cb data;
data.refname = refname;
- data.oldrefname = oldrefname;
data.conflicting_refname = NULL;
+ data.skip = skip;
+ data.skipnum = skipnum;
sort_ref_dir(dir);
if (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, data)) {
@@ -2077,7 +2083,8 @@ int dwim_log(const char *str, int len, unsigned char
*sha1, char **log)
/* This function should make sure errno is meaningful on error */
static struct ref_lock *lock_ref_sha1_basic(const char *refname,
const unsigned char *old_sha1,
- int flags, int *type_p)
+ int flags, int *type_p,
+ const char **skip, int skipnum)
{
char *ref_file;
const char *orig_refname = refname;
@@ -2126,7 +2133,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),
+ skip, skipnum)) {
last_errno = ENOTDIR;
goto error_return;
}
@@ -2184,7 +2192,7 @@ struct ref_lock *lock_any_ref_for_update(const char
*refname,
const unsigned char *old_sha1,
int flags, int *type_p)
{
- return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
+ return lock_ref_sha1_basic(refname, old_sha1, flags, type_p, NULL, 0);
}
/*
@@ -2676,10 +2684,12 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
if (!symref)
return error(refname %s not found, oldrefname);
- if (!is_refname_available(newrefname, oldrefname,
get_packed_refs(ref_cache)))
+ if (!is_refname_available(newrefname, get_packed_refs(ref_cache),
+ oldrefname, 1))
return 1;
- if (!is_refname_available(newrefname, oldrefname,
get_loose_refs(ref_cache)))
+ if (!is_refname_available(newrefname, get_loose_refs(ref_cache),
+ oldrefname, 1))
return 1;
if (log rename(git_path(logs/%s, oldrefname),
git_path(TMP_RENAMED_LOG)))
@@ -2709,7 +2719,7 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
logmoved = log;
- lock =
[PATCH v20 31/48] receive-pack.c: use a reference transaction for updating the refs
Wrap all the ref updates inside a transaction.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/receive-pack.c | 96 +-
1 file changed, 63 insertions(+), 33 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c323081..b51f8ae 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;
- const char *error_string;
+ char *error_string;
unsigned int skip_update:1,
did_not_exist:1;
int index;
@@ -468,19 +468,18 @@ static int update_shallow_ref(struct command *cmd, struct
shallow_info *si)
return 0;
}
-static const char *update(struct command *cmd, struct shallow_info *si)
+static char *update(struct command *cmd, struct shallow_info *si)
{
const char *name = cmd-ref_name;
struct strbuf namespaced_name_buf = STRBUF_INIT;
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)) {
rp_error(refusing to create funny ref '%s' remotely, name);
- return funny refname;
+ return xstrdup(funny refname);
}
strbuf_addf(namespaced_name_buf, %s%s, get_git_namespace(), name);
@@ -498,20 +497,20 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
rp_error(refusing to update checked out branch: %s,
name);
if (deny_current_branch == DENY_UNCONFIGURED)
refuse_unconfigured_deny();
- return branch is currently checked out;
+ return xstrdup(branch is currently checked out);
}
}
if (!is_null_sha1(new_sha1) !has_sha1_file(new_sha1)) {
error(unpack should have generated %s,
but I can't find it!, sha1_to_hex(new_sha1));
- return bad pack;
+ return xstrdup(bad pack);
}
if (!is_null_sha1(old_sha1) is_null_sha1(new_sha1)) {
if (deny_deletes starts_with(name, refs/heads/)) {
rp_error(denying ref deletion for %s, name);
- return deletion prohibited;
+ return xstrdup(deletion prohibited);
}
if (!strcmp(namespaced_name, head_name)) {
@@ -526,7 +525,7 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
if (deny_delete_current == DENY_UNCONFIGURED)
refuse_unconfigured_deny_delete_current();
rp_error(refusing to delete the current
branch: %s, name);
- return deletion of the current branch
prohibited;
+ return xstrdup(deletion of the current branch
prohibited);
}
}
}
@@ -544,19 +543,19 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
old_object-type != OBJ_COMMIT ||
new_object-type != OBJ_COMMIT) {
error(bad sha1 objects for %s, name);
- return bad ref;
+ return xstrdup(bad ref);
}
old_commit = (struct commit *)old_object;
new_commit = (struct commit *)new_object;
if (!in_merge_bases(old_commit, new_commit)) {
rp_error(denying non-fast-forward %s
(you should pull first), name);
- return non-fast-forward;
+ return xstrdup(non-fast-forward);
}
}
if (run_update_hook(cmd)) {
rp_error(hook declined to update %s, name);
- return hook declined;
+ return xstrdup(hook declined);
}
if (is_null_sha1(new_sha1)) {
@@ -571,24 +570,32 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
}
if (delete_ref(namespaced_name, old_sha1, 0)) {
rp_error(failed to delete %s, name);
- return failed to delete;
+ return xstrdup(failed to delete);
}
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;
-
-
[PATCH v20 28/48] fast-import.c: change update_branch to use ref transactions
Change update_branch() to use ref transactions for updates.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
fast-import.c | 24 ++--
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 6707a66..d5206ee 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,32 @@ 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);
return 0;
}
--
2.0.0.420.g181e020.dirty
--
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 v20 32/48] fast-import.c: use a ref transaction when dumping tags
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
fast-import.c | 29 +++--
1 file changed, 23 insertions(+), 6 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index d5206ee..a95e1be 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1734,15 +1734,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.0.0.420.g181e020.dirty
--
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 v20 34/48] refs.c: make lock_ref_sha1 static
No external callers reference lock_ref_sha1 any more so lets declare it static.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 2 +-
refs.h | 6 --
2 files changed, 1 insertion(+), 7 deletions(-)
diff --git a/refs.c b/refs.c
index 4bdccc5..456acdd 100644
--- a/refs.c
+++ b/refs.c
@@ -2170,7 +2170,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 e729ea9..4ac4a7d 100644
--- a/refs.h
+++ b/refs.h
@@ -177,12 +177,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.0.0.420.g181e020.dirty
--
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 v20 35/48] refs.c: remove the update_ref_lock function
Since we now only call update_ref_lock with onerr==QUIET_ON_ERR we no longer
need this function and can replace it with just calling lock_any_ref_for_update
directly.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 30 ++
1 file changed, 6 insertions(+), 24 deletions(-)
diff --git a/refs.c b/refs.c
index 456acdd..8fb0a9e 100644
--- a/refs.c
+++ b/refs.c
@@ -,24 +,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)
@@ -3603,12 +3585,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i n; i++) {
struct ref_update *update = updates[i];
- update-lock = update_ref_lock(update-refname,
- (update-have_old ?
- update-old_sha1 : NULL),
- update-flags,
- update-type,
- UPDATE_REFS_QUIET_ON_ERR);
+ update-lock = lock_any_ref_for_update(update-refname,
+ (update-have_old ?
+ update-old_sha1 :
+ NULL),
+ update-flags,
+ update-type);
if (!update-lock) {
if (err)
strbuf_addf(err, Cannot lock the ref '%s'.,
--
2.0.0.420.g181e020.dirty
--
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 v20 26/48] commit.c: use ref transactions for updates
Change commit.c to use ref transactions for all ref updates.
Make sure we pass a NULL pointer to ref_transaction_update if have_old
is false.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/commit.c | 24 +++-
1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 5e2221c..668fa6a 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));
--
2.0.0.420.g181e020.dirty
--
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 v20 33/48] walker.c: use ref transaction for ref updates
Switch to using ref transactions in walker_fetch(). As part of the refactoring
to use ref transactions we also fix a potential memory leak where in the
original code if write_ref_sha1() would fail we would end up returning from
the function without free()ing the msg string.
Note that this function is only called when fetching from a remote HTTP
repository onto the local (most of the time single-user) repository which
likely means that the type of collissions that the previous locking would
protect against and cause the fetch to fail for to be even more rare.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
walker.c | 59 +++
1 file changed, 35 insertions(+), 24 deletions(-)
diff --git a/walker.c b/walker.c
index 1dd86b8..60d9f9e 100644
--- a/walker.c
+++ b/walker.c
@@ -251,39 +251,36 @@ 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 ref_name = STRBUF_INIT;
+ struct strbuf err = STRBUF_INIT;
+ struct ref_transaction *transaction = NULL;
unsigned char *sha1 = xmalloc(targets * 20);
- char *msg;
- int ret;
+ char *msg = NULL;
int i;
save_commit_buffer = 0;
- for (i = 0; i targets; i++) {
- if (!write_ref || !write_ref[i])
- continue;
-
- lock[i] = lock_ref_sha1(write_ref[i], NULL);
- if (!lock[i]) {
- error(Can't lock ref %s, write_ref[i]);
- goto unlock_and_fail;
+ if (write_ref) {
+ transaction = ref_transaction_begin(err);
+ if (!transaction) {
+ error(%s, err.buf);
+ goto rollback_and_fail;
}
}
-
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 rollback_and_fail;
}
if (process(walker, lookup_unknown_object(sha1[20 * i])))
- goto unlock_and_fail;
+ goto rollback_and_fail;
}
if (loop(walker))
- goto unlock_and_fail;
+ goto rollback_and_fail;
if (write_ref_log_details) {
msg = xmalloc(strlen(write_ref_log_details) + 12);
@@ -294,19 +291,33 @@ int walker_fetch(struct walker *walker, int targets, char
**target,
for (i = 0; i targets; i++) {
if (!write_ref || !write_ref[i])
continue;
- ret = write_ref_sha1(lock[i], sha1[20 * i], msg ? msg : fetch
(unknown));
- lock[i] = NULL;
- if (ret)
- goto unlock_and_fail;
+ strbuf_reset(ref_name);
+ strbuf_addf(ref_name, refs/%s, write_ref[i]);
+ if (ref_transaction_update(transaction, ref_name.buf,
+ sha1[20 * i], NULL, 0, 0,
+ err)) {
+ error(%s, err.buf);
+ goto rollback_and_fail;
+ }
+ }
+ if (write_ref) {
+ if (ref_transaction_commit(transaction,
+ msg ? msg : fetch (unknown),
+ err)) {
+ error(%s, err.buf);
+ goto rollback_and_fail;
+ }
+ ref_transaction_free(transaction);
}
- free(msg);
+ free(msg);
return 0;
-unlock_and_fail:
- for (i = 0; i targets; i++)
- if (lock[i])
- unlock_ref(lock[i]);
+rollback_and_fail:
+ ref_transaction_free(transaction);
+ free(msg);
+ strbuf_release(err);
+ strbuf_release(ref_name);
return -1;
}
--
2.0.0.420.g181e020.dirty
--
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 v20 24/48] tag.c: use ref transactions when doing updates
Change tag.c to use ref transactions for all ref updates.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/tag.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index c6e8a71..c9bfc9a 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -548,7 +548,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
struct strbuf ref = STRBUF_INIT;
unsigned char object[20], prev[20];
const char *object_ref, *tag;
- struct ref_lock *lock;
struct create_tag_options opt;
char *cleanup_arg = NULL;
int annotate = 0, force = 0, lines = -1;
@@ -556,6 +555,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
const char *msgfile = NULL, *keyid = NULL;
struct msg_arg msg = { 0, STRBUF_INIT };
struct commit_list *with_commit = NULL;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
struct option options[] = {
OPT_CMDMODE('l', list, cmdmode, N_(list tag names), 'l'),
{ OPTION_INTEGER, 'n', NULL, lines, N_(n),
@@ -701,11 +702,13 @@ 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, !is_null_sha1(prev), err) ||
+ ref_transaction_commit(transaction, NULL, err))
+ die(%s, err.buf);
+ ref_transaction_free(transaction);
if (force !is_null_sha1(prev) hashcmp(prev, object))
printf(_(Updated tag '%s' (was %s)\n), tag,
find_unique_abbrev(prev, DEFAULT_ABBREV));
--
2.0.0.420.g181e020.dirty
--
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 v20 36/48] refs.c: remove the update_ref_write function
Since we only call update_ref_write from a single place and we only call it
with onerr==QUIET_ON_ERR we can just as well get rid of it and just call
write_ref_sha1 directly. 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. For example if the commit failed due to name
conflicts etc.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 35 +--
1 file changed, 9 insertions(+), 26 deletions(-)
diff --git a/refs.c b/refs.c
index 8fb0a9e..7113843 100644
--- a/refs.c
+++ b/refs.c
@@ -,25 +,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
@@ -3605,14 +3586,16 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (!is_null_sha1(update-new_sha1)) {
- ret = update_ref_write(msg,
- update-refname,
- update-new_sha1,
- update-lock, err,
- UPDATE_REFS_QUIET_ON_ERR);
- update-lock = NULL; /* freed by update_ref_write */
- if (ret)
+ ret = write_ref_sha1(update-lock, update-new_sha1,
+msg);
+ update-lock = NULL; /* freed by write_ref_sha1 */
+ if (ret) {
+ const char *str = Cannot update the ref '%s'.;
+
+ if (err)
+ strbuf_addf(err, str, update-refname);
goto cleanup;
+ }
}
}
--
2.0.0.420.g181e020.dirty
--
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 v20 25/48] replace.c: use the ref transaction functions for updates
Update replace.c to use ref transactions for updates. Reviewed-by: Jonathan Nieder jrnie...@gmail.com Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- builtin/replace.c | 15 +-- 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/builtin/replace.c b/builtin/replace.c index 1bb491d..7528f3d 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,14 @@ 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, !is_null_sha1(prev), err) || + ref_transaction_commit(transaction, NULL, err)) + die(%s, err.buf); + ref_transaction_free(transaction); return 0; } -- 2.0.0.420.g181e020.dirty -- 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 v20 29/48] branch.c: use ref transaction for all ref updates
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.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
branch.c | 30 --
1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/branch.c b/branch.c
index 660097b..c1eae00 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,25 @@ 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);
+ }
+
if (real_ref track)
setup_tracking(ref.buf + 11, real_ref, track, quiet);
- if (!dont_change_ref)
- if (write_ref_sha1(lock, sha1, msg) 0)
- die_errno(_(Failed to write ref));
-
strbuf_release(ref);
free(real_ref);
}
--
2.0.0.420.g181e020.dirty
--
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 v20 27/48] sequencer.c: use ref transactions for all ref updates
Change to use ref transactions for all updates to refs.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
sequencer.c | 24
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 0a80c58..fd8acaf 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -272,23 +272,31 @@ static int error_dirty_index(struct replay_opts *opts)
static int fast_forward_to(const unsigned char *to, const unsigned char *from,
int unborn, struct replay_opts *opts)
{
- struct ref_lock *ref_lock;
+ struct ref_transaction *transaction;
struct strbuf sb = STRBUF_INIT;
- int ret;
+ struct strbuf err = STRBUF_INIT;
read_cache();
if (checkout_fast_forward(from, to, 1))
exit(1); /* the callee should have complained already */
- ref_lock = lock_any_ref_for_update(HEAD, unborn ? null_sha1 : from,
- 0, NULL);
- if (!ref_lock)
- return error(_(Failed to lock HEAD during fast_forward_to));
strbuf_addf(sb, %s: fast-forward, action_name(opts));
- ret = write_ref_sha1(ref_lock, to, sb.buf);
+
+ transaction = ref_transaction_begin(err);
+ if (!transaction ||
+ ref_transaction_update(transaction, HEAD, to, from,
+ 0, !unborn, 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;
+ ref_transaction_free(transaction);
+ return 0;
}
static int do_recursive_merge(struct commit *base, struct commit *next,
--
2.0.0.420.g181e020.dirty
--
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 v20 20/48] refs.c: change ref_transaction_create to do error checking and return status
Do basic error checking in ref_transaction_create() and make it return
non-zero on error. Update all callers to check the result of
ref_transaction_create(). There are currently no conditions in _create that
will return error but there will be in the future. Add an err argument that
will be updated on failure.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 4 +++-
refs.c | 18 +++--
refs.h | 55 +---
3 files changed, 63 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..33b4383 100644
--- a/refs.h
+++ b/refs.h
@@ -10,6 +10,45 @@ 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.
+ * This allows the caller to prepare preamble text to the generated
+ * error message:
+ *
+ * strbuf_addf(err, Error while doing foo-bar: );
+ * if (ref_transaction_update(..., err)) {
+ * ret = error(%s, err.buf);
+ * goto cleanup;
+ * }
+ */
struct ref_transaction;
/*
@@ -248,7 +287,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 +301,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,
-
[PATCH v20 19/48] refs.c: change ref_transaction_update() to do error checking and return status
Update ref_transaction_update() do some basic error checking and return
non-zero on error. Update all callers to check ref_transaction_update() for
error. There are currently no conditions in _update that will return error but
there will be in the future. Add an err argument that will be updated on
failure. In future patches we will start doing both locking and checking
for name conflicts in _update instead of _commit at which time this function
will start returning errors for these conditions.
Also check for BUGs during update and die(BUG:...) if we are calling
_update with have_old but the old_sha1 pointer is NULL.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 12 +++-
refs.c | 18 --
refs.h | 14 +-
3 files changed, 28 insertions(+), 16 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 88ab785..3067b11 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -16,6 +16,7 @@ static struct ref_transaction *transaction;
static char line_termination = '\n';
static int update_flags;
+static struct strbuf err = STRBUF_INIT;
/*
* Parse one whitespace- or NUL-terminated, possibly C-quoted argument
@@ -197,8 +198,9 @@ static const char *parse_cmd_update(struct strbuf *input,
const char *next)
if (*next != line_termination)
die(update %s: extra input: %s, refname, next);
- ref_transaction_update(transaction, refname, new_sha1, old_sha1,
- update_flags, have_old);
+ if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+ update_flags, have_old, err))
+ die(%s, err.buf);
update_flags = 0;
free(refname);
@@ -286,8 +288,9 @@ static const char *parse_cmd_verify(struct strbuf *input,
const char *next)
if (*next != line_termination)
die(verify %s: extra input: %s, refname, next);
- ref_transaction_update(transaction, refname, new_sha1, old_sha1,
- update_flags, have_old);
+ if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+ update_flags, have_old, err))
+ die(%s, err.buf);
update_flags = 0;
free(refname);
@@ -342,7 +345,6 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
const char *refname, *oldval, *msg = NULL;
unsigned char sha1[20], oldsha1[20];
int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;
- struct strbuf err = STRBUF_INIT;
struct option options[] = {
OPT_STRING( 'm', NULL, msg, N_(reason), N_(reason of the
update)),
OPT_BOOL('d', NULL, delete, N_(delete the reference)),
diff --git a/refs.c b/refs.c
index 4f78bd9..3f05e88 100644
--- a/refs.c
+++ b/refs.c
@@ -3428,19 +3428,25 @@ static struct ref_update *add_update(struct
ref_transaction *transaction,
return update;
}
-void ref_transaction_update(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *new_sha1,
- const unsigned char *old_sha1,
- int flags, int have_old)
+int ref_transaction_update(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *new_sha1,
+ const unsigned char *old_sha1,
+ int flags, int have_old,
+ struct strbuf *err)
{
- struct ref_update *update = add_update(transaction, refname);
+ struct ref_update *update;
+
+ if (have_old !old_sha1)
+ die(BUG: have_old is true but old_sha1 is NULL);
+ update = add_update(transaction, refname);
hashcpy(update-new_sha1, new_sha1);
update-flags = flags;
update-have_old = have_old;
if (have_old)
hashcpy(update-old_sha1, old_sha1);
+ return 0;
}
void ref_transaction_create(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index 163b45c..c5376ce 100644
--- a/refs.h
+++ b/refs.h
@@ -246,12 +246,16 @@ struct ref_transaction *ref_transaction_begin(void);
* be deleted. If have_old is true, then old_sha1 holds the value
* that the reference should have had before the update, or zeros if
* it must not have existed beforehand.
+ * Function returns 0 on success and non-zero on failure. A failure to update
+ * means that the transaction as a whole has failed and will need to be
+ * rolled back. On failure the err buffer will be updated.
*/
-void ref_transaction_update(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char
[PATCH v20 22/48] refs.c: make ref_transaction_begin take an err argument
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 _being with
Can not connect to MySQL server. No route to host.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 2 +-
refs.c | 2 +-
refs.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 7c9c248..c6ad0be 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -365,7 +365,7 @@ 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 (delete || no_deref || argc 0)
usage_with_options(git_update_ref_usage, options);
if (end_null)
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 eeababd..e729ea9 100644
--- a/refs.h
+++ b/refs.h
@@ -269,7 +269,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.0.0.420.g181e020.dirty
--
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 v20 30/48] refs.c: change update_ref to use a transaction
Change the update_ref helper function to use a ref transaction internally.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 28
1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index 8c695ba..4bdccc5 100644
--- a/refs.c
+++ b/refs.c
@@ -3524,11 +3524,31 @@ 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);
+ }
+ return 0;
}
static int ref_update_compare(const void *r1, const void *r2)
--
2.0.0.420.g181e020.dirty
--
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 v20 21/48] refs.c: update ref_transaction_delete to check for error and return status
Change ref_transaction_delete() to do basic error checking and return
non-zero of error. Update all callers to check the return for
ref_transaction_delete(). There are currently no conditions in _delete that
will return error but there will be in the future. Add an err argument that
will be updated on failure.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.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 33b4383..eeababd 100644
--- a/refs.h
+++ b/refs.h
@@ -315,11 +315,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.0.0.420.g181e020.dirty
--
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 v20 23/48] refs.c: add transaction.status and track OPEN/CLOSED/ERROR
Track the status of a transaction in a new status field. Check the field for
sanity, i.e. that status must be OPEN when _commit/_create/_delete or
_update is called or else die(BUG:...)
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 40 +++-
1 file changed, 39 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 9cb7908..8c695ba 100644
--- a/refs.c
+++ b/refs.c
@@ -3387,6 +3387,25 @@ 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: If an open transaction is successfully committed the state will
+ * change to CLOSED. No further changes can be made to a CLOSED
+ * transaction.
+ * CLOSED means that all updates have been successfully committed and
+ * the only thing that remains is to free the completed transaction.
+ * ERROR: The transaction has failed and is no longer committable.
+ * No further changes can be made to a CLOSED transaction and it must
+ * be rolled back using transaction_free.
+ */
+enum ref_transaction_state {
+ REF_TRANSACTION_OPEN = 0,
+ REF_TRANSACTION_CLOSED = 1,
+ REF_TRANSACTION_ERROR = 2,
+};
+
+/*
* Data structure for holding a reference transaction, which can
* consist of checks and updates to multiple references, carried out
* as atomically as possible. This structure is opaque to callers.
@@ -3395,6 +3414,8 @@ struct ref_transaction {
struct ref_update **updates;
size_t alloc;
size_t nr;
+ enum ref_transaction_state state;
+ int status;
};
struct ref_transaction *ref_transaction_begin(struct strbuf *err)
@@ -3437,6 +3458,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 +3481,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 +3504,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 +3562,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 +3630,9 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
clear_loose_ref_cache(ref_cache);
cleanup:
+ transaction-state = ret ? REF_TRANSACTION_ERROR
+ : REF_TRANSACTION_CLOSED;
+
for (i = 0; i n; i++)
if (updates[i]-lock)
unlock_ref(updates[i]-lock);
--
2.0.0.420.g181e020.dirty
--
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 v20 18/48] refs.c: remove the onerr argument to ref_transaction_commit
Since all callers now use QUIET_ON_ERR we no longer need to provide an onerr
argument any more. Remove the onerr argument from the ref_transaction_commit
signature.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 3 +--
refs.c | 22 +++---
refs.h | 3 +--
3 files changed, 9 insertions(+), 19 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index aec9004..88ab785 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -366,8 +366,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
if (end_null)
line_termination = '\0';
update_refs_stdin();
- if (ref_transaction_commit(transaction, msg, err,
- UPDATE_REFS_QUIET_ON_ERR))
+ if (ref_transaction_commit(transaction, msg, err))
die(%s, err.buf);
ref_transaction_free(transaction);
return 0;
diff --git a/refs.c b/refs.c
index 003b313..4f78bd9 100644
--- a/refs.c
+++ b/refs.c
@@ -3491,8 +3491,7 @@ static int ref_update_compare(const void *r1, const void
*r2)
}
static int ref_update_reject_duplicates(struct ref_update **updates, int n,
- struct strbuf *err,
- enum action_on_err onerr)
+ struct strbuf *err)
{
int i;
for (i = 1; i n; i++)
@@ -3502,22 +3501,13 @@ static int ref_update_reject_duplicates(struct
ref_update **updates, int n,
if (err)
strbuf_addf(err, str, updates[i]-refname);
- switch (onerr) {
- case UPDATE_REFS_MSG_ON_ERR:
- error(str, updates[i]-refname); break;
- case UPDATE_REFS_DIE_ON_ERR:
- die(str, updates[i]-refname); break;
- case UPDATE_REFS_QUIET_ON_ERR:
- break;
- }
return 1;
}
return 0;
}
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, struct strbuf *err,
- enum action_on_err onerr)
+ const char *msg, struct strbuf *err)
{
int ret = 0, delnum = 0, i;
const char **delnames;
@@ -3532,7 +3522,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Copy, sort, and reject duplicate refs */
qsort(updates, n, sizeof(*updates), ref_update_compare);
- ret = ref_update_reject_duplicates(updates, n, err, onerr);
+ ret = ref_update_reject_duplicates(updates, n, err);
if (ret)
goto cleanup;
@@ -3544,7 +3534,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
(update-have_old ?
update-old_sha1 : NULL),
update-flags,
- update-type, onerr);
+ update-type,
+ UPDATE_REFS_QUIET_ON_ERR);
if (!update-lock) {
if (err)
strbuf_addf(err, Cannot lock the ref '%s'.,
@@ -3562,7 +3553,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
ret = update_ref_write(msg,
update-refname,
update-new_sha1,
- update-lock, err, onerr);
+ update-lock, err,
+ UPDATE_REFS_QUIET_ON_ERR);
update-lock = NULL; /* freed by update_ref_write */
if (ret)
goto cleanup;
diff --git a/refs.h b/refs.h
index e588ff8..163b45c 100644
--- a/refs.h
+++ b/refs.h
@@ -282,8 +282,7 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
* the transaction failed. The string does not end in newline.
*/
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, struct strbuf *err,
- enum action_on_err onerr);
+ const char *msg, struct strbuf *err);
/*
* Free an existing transaction and all associated data.
--
2.0.0.420.g181e020.dirty
--
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
[PATCH v20 10/48] refs.c: verify_lock should set errno to something meaningful
Making errno when returning from verify_lock() meaningful, which
should almost but not completely fix
* a bug in git fetch's s_update_ref, which trusts the result of an
errno == ENOTDIR check to detect D/F conflicts
ENOTDIR makes sense as a sign that a file was in the way of a
directory we wanted to create. Should git fetch also look for
ENOTEMPTY or EEXIST to catch cases where a directory was in the way
of a file to be created?
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 4
refs.h | 6 +-
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 9ea519c..a48f805 100644
--- a/refs.c
+++ b/refs.c
@@ -1932,18 +1932,22 @@ int refname_match(const char *abbrev_name, const char
*full_name)
return 0;
}
+/* This function should make sure errno is meaningful on error */
static struct ref_lock *verify_lock(struct ref_lock *lock,
const unsigned char *old_sha1, int mustexist)
{
if (read_ref_full(lock-ref_name, lock-old_sha1, mustexist, NULL)) {
+ int save_errno = errno;
error(Can't verify ref %s, lock-ref_name);
unlock_ref(lock);
+ errno = save_errno;
return NULL;
}
if (hashcmp(lock-old_sha1, old_sha1)) {
error(Ref %s is at %s but expected %s, lock-ref_name,
sha1_to_hex(lock-old_sha1), sha1_to_hex(old_sha1));
unlock_ref(lock);
+ errno = EBUSY;
return NULL;
}
return lock;
diff --git a/refs.h b/refs.h
index 82cc5cb..8d6cac7 100644
--- a/refs.h
+++ b/refs.h
@@ -137,11 +137,15 @@ 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. **/
+/*
+ * 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 */
extern struct ref_lock *lock_any_ref_for_update(const char *refname,
const unsigned char *old_sha1,
int flags, int *type_p);
--
2.0.0.420.g181e020.dirty
--
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 v20 13/48] refs.c: make resolve_ref_unsafe set errno to something meaningful on error
Making errno when returning from resolve_ref_unsafe() meaningful,
which should fix
* a bug in lock_ref_sha1_basic, where it assumes EISDIR
means it failed due to a directory being in the way
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
cache.h | 2 +-
refs.c | 19 +++
2 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/cache.h b/cache.h
index 8b12aa8..e7ec626 100644
--- a/cache.h
+++ b/cache.h
@@ -979,7 +979,7 @@ extern int read_ref(const char *refname, unsigned char
*sha1);
* NULL. If more than MAXDEPTH recursive symbolic lookups are needed,
* give up and return NULL.
*
- * errno is sometimes set on errors, but not always.
+ * errno is set to something meaningful on error.
*/
extern const char *resolve_ref_unsafe(const char *ref, unsigned char *sha1,
int reading, int *flag);
extern char *resolve_refdup(const char *ref, unsigned char *sha1, int reading,
int *flag);
diff --git a/refs.c b/refs.c
index 7a815be..211429d 100644
--- a/refs.c
+++ b/refs.c
@@ -1334,6 +1334,7 @@ static const char *handle_missing_loose_ref(const char
*refname,
}
}
+/* This function needs to return a meaningful errno on failure */
const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int
reading, int *flag)
{
int depth = MAXDEPTH;
@@ -1344,8 +1345,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];
@@ -1353,8 +1356,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);
@@ -1416,9 +1421,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';
@@ -1435,6 +1444,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;
@@ -1447,6 +1457,7 @@ const char *resolve_ref_unsafe(const char *refname,
unsigned char *sha1, int rea
if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {
if (flag)
*flag |= REF_ISBROKEN;
+ errno = EINVAL;
return NULL;
}
refname = strcpy(refname_buffer, buf);
--
2.0.0.420.g181e020.dirty
--
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 v20 11/48] refs.c: make remove_empty_directories always set errno to something sane
Making errno when returning from remove_empty_directories() more obviously meaningful, which should provide some peace of mind for people auditing lock_ref_sha1_basic. Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- refs.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/refs.c b/refs.c index a48f805..cc69581 100644 --- a/refs.c +++ b/refs.c @@ -1960,14 +1960,16 @@ static int remove_empty_directories(const char *file) * only empty directories), remove them. */ struct strbuf path; - int result; + int result, save_errno; strbuf_init(path, 20); strbuf_addstr(path, file); result = remove_dir_recursively(path, REMOVE_DIR_EMPTY_ONLY); + save_errno = errno; strbuf_release(path); + errno = save_errno; return result; } @@ -2056,6 +2058,7 @@ 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 */ static struct ref_lock *lock_ref_sha1_basic(const char *refname, const unsigned char *old_sha1, int flags, int *type_p) -- 2.0.0.420.g181e020.dirty -- 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 v20 14/48] refs.c: log_ref_write should try to return meaningful errno
Making errno from write_ref_sha1() meaningful, which should fix
* a bug in git checkout -b where it prints strerror(errno)
despite errno possibly being zero or clobbered
* a bug in git fetch's s_update_ref, which trusts the result of an
errno == ENOTDIR check to detect D/F conflicts
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 28 +++-
1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/refs.c b/refs.c
index 211429d..61570c9 100644
--- a/refs.c
+++ b/refs.c
@@ -2859,8 +2859,19 @@ static int log_ref_write(const char *refname, const
unsigned char *old_sha1,
len += copy_msg(logrec + len - 1, msg) - 1;
written = len = maxlen ? write_in_full(logfd, logrec, len) : -1;
free(logrec);
- if (close(logfd) != 0 || written != len)
- return error(Unable to append to %s, log_file);
+ if (written != len) {
+ int save_errno = errno;
+ close(logfd);
+ error(Unable to append to %s, log_file);
+ errno = save_errno;
+ return -1;
+ }
+ if (close(logfd)) {
+ int save_errno = errno;
+ error(Unable to append to %s, log_file);
+ errno = save_errno;
+ return -1;
+ }
return 0;
}
@@ -2869,14 +2880,17 @@ 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,
const unsigned char *sha1, const char *logmsg)
{
static char term = '\n';
struct object *o;
- if (!lock)
+ if (!lock) {
+ errno = EINVAL;
return -1;
+ }
if (!lock-force_write !hashcmp(lock-old_sha1, sha1)) {
unlock_ref(lock);
return 0;
@@ -2886,19 +2900,23 @@ int write_ref_sha1(struct ref_lock *lock,
error(Trying to write ref %s with nonexistent object %s,
lock-ref_name, sha1_to_hex(sha1));
unlock_ref(lock);
+ errno = EINVAL;
return -1;
}
if (o-type != OBJ_COMMIT is_branch(lock-ref_name)) {
error(Trying to write non-commit object %s to branch %s,
sha1_to_hex(sha1), lock-ref_name);
unlock_ref(lock);
+ errno = EINVAL;
return -1;
}
if (write_in_full(lock-lock_fd, sha1_to_hex(sha1), 40) != 40 ||
- write_in_full(lock-lock_fd, term, 1) != 1
- || close_ref(lock) 0) {
+ write_in_full(lock-lock_fd, term, 1) != 1 ||
+ close_ref(lock) 0) {
+ int save_errno = errno;
error(Couldn't write %s, lock-lk-filename);
unlock_ref(lock);
+ errno = save_errno;
return -1;
}
clear_loose_ref_cache(ref_cache);
--
2.0.0.420.g181e020.dirty
--
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 v20 16/48] refs.c: make update_ref_write update a strbuf on failure
Change update_ref_write to also update an error strbuf on failure.
This makes the error available to ref_transaction_commit callers if the
transaction failed due to update_ref_sha1/write_ref_sha1 failures.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/refs.c b/refs.c
index 115f143..003b313 100644
--- a/refs.c
+++ b/refs.c
@@ -3353,10 +3353,13 @@ static struct ref_lock *update_ref_lock(const char
*refname,
static int update_ref_write(const char *action, const char *refname,
const unsigned char *sha1, struct ref_lock *lock,
- enum action_on_err onerr)
+ struct strbuf *err, enum action_on_err onerr)
{
if (write_ref_sha1(lock, sha1, action) 0) {
const char *str = Cannot update the ref '%s'.;
+ if (err)
+ strbuf_addf(err, str, refname);
+
switch (onerr) {
case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
@@ -3477,7 +3480,7 @@ int update_ref(const char *action, const char *refname,
lock = update_ref_lock(refname, oldval, flags, NULL, onerr);
if (!lock)
return 1;
- return update_ref_write(action, refname, sha1, lock, onerr);
+ return update_ref_write(action, refname, sha1, lock, NULL, onerr);
}
static int ref_update_compare(const void *r1, const void *r2)
@@ -3559,7 +3562,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
ret = update_ref_write(msg,
update-refname,
update-new_sha1,
- update-lock, onerr);
+ update-lock, err, onerr);
update-lock = NULL; /* freed by update_ref_write */
if (ret)
goto cleanup;
--
2.0.0.420.g181e020.dirty
--
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 v20 09/48] refs.c: make sure log_ref_setup returns a meaningful errno
Making errno when returning from log_ref_setup() meaningful,
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 27 +++
refs.h | 4 +++-
2 files changed, 22 insertions(+), 9 deletions(-)
diff --git a/refs.c b/refs.c
index 67a0217..9ea519c 100644
--- a/refs.c
+++ b/refs.c
@@ -2751,6 +2751,7 @@ static int copy_msg(char *buf, const char *msg)
return cp - buf;
}
+/* This function must set a meaningful errno on failure */
int log_ref_setup(const char *refname, char *logfile, int bufsize)
{
int logfd, oflags = O_APPEND | O_WRONLY;
@@ -2761,9 +2762,12 @@ int log_ref_setup(const char *refname, char *logfile,
int bufsize)
starts_with(refname, refs/remotes/) ||
starts_with(refname, refs/notes/) ||
!strcmp(refname, HEAD))) {
- if (safe_create_leading_directories(logfile) 0)
- return error(unable to create directory for %s,
-logfile);
+ if (safe_create_leading_directories(logfile) 0) {
+ int save_errno = errno;
+ error(unable to create directory for %s, logfile);
+ errno = save_errno;
+ return -1;
+ }
oflags |= O_CREAT;
}
@@ -2774,15 +2778,22 @@ int log_ref_setup(const char *refname, char *logfile,
int bufsize)
if ((oflags O_CREAT) errno == EISDIR) {
if (remove_empty_directories(logfile)) {
- return error(There are still logs under '%s',
-logfile);
+ int save_errno = errno;
+ error(There are still logs under '%s',
+ logfile);
+ errno = save_errno;
+ return -1;
}
logfd = open(logfile, oflags, 0666);
}
- if (logfd 0)
- return error(Unable to append to %s: %s,
-logfile, strerror(errno));
+ if (logfd 0) {
+ int save_errno = errno;
+ error(Unable to append to %s: %s, logfile,
+ strerror(errno));
+ errno = save_errno;
+ return -1;
+ }
}
adjust_shared_perm(logfile);
diff --git a/refs.h b/refs.h
index 65f7637..82cc5cb 100644
--- a/refs.h
+++ b/refs.h
@@ -158,7 +158,9 @@ 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. **/
+/*
+ * Setup reflog before using. Set errno to something meaningful on failure.
+ */
int log_ref_setup(const char *refname, char *logfile, int bufsize);
/** Reads log for the value of ref during at_time. **/
--
2.0.0.420.g181e020.dirty
--
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 v20 17/48] update-ref: use err argument to get error from ref_transaction_commit
Call ref_transaction_commit with QUIET_ON_ERR and use the strbuf that is
returned to print a log message if/after the transaction fails.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 22617af..aec9004 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -342,6 +342,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
const char *refname, *oldval, *msg = NULL;
unsigned char sha1[20], oldsha1[20];
int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;
+ struct strbuf err = STRBUF_INIT;
struct option options[] = {
OPT_STRING( 'm', NULL, msg, N_(reason), N_(reason of the
update)),
OPT_BOOL('d', NULL, delete, N_(delete the reference)),
@@ -359,18 +360,17 @@ int cmd_update_ref(int argc, const char **argv, const
char *prefix)
die(Refusing to perform update with empty message.);
if (read_stdin) {
- int ret;
transaction = ref_transaction_begin();
-
if (delete || no_deref || argc 0)
usage_with_options(git_update_ref_usage, options);
if (end_null)
line_termination = '\0';
update_refs_stdin();
- ret = ref_transaction_commit(transaction, msg, NULL,
-UPDATE_REFS_DIE_ON_ERR);
+ if (ref_transaction_commit(transaction, msg, err,
+ UPDATE_REFS_QUIET_ON_ERR))
+ die(%s, err.buf);
ref_transaction_free(transaction);
- return ret;
+ return 0;
}
if (end_null)
--
2.0.0.420.g181e020.dirty
--
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 v20 41/48] refs.c: pass the ref log message to _create/delete/update instead of _commit
Change the reference transactions so that we pass the reflog message
through to the create/delete/update function instead of the commit message.
This allows for individual messages for each change in a multi ref
transaction.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
branch.c | 4 ++--
builtin/commit.c | 4 ++--
builtin/fetch.c| 3 +--
builtin/receive-pack.c | 5 +++--
builtin/replace.c | 4 ++--
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 ++---
12 files changed, 52 insertions(+), 44 deletions(-)
diff --git a/branch.c b/branch.c
index c1eae00..e0439af 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);
}
diff --git a/builtin/commit.c b/builtin/commit.c
index 668fa6a..c499826 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/fetch.c b/builtin/fetch.c
index 55f457c..faa1233 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -673,10 +673,9 @@ static int store_updated_refs(const char *raw_url, const
char *remote_name,
}
}
}
-
if (rc STORE_REF_ERROR_DF_CONFLICT)
error(_(some local refs could not be updated; try running\n
- 'git remote prune %s' to remove any old, conflicting
+ 'git remote prune %s' to remove any old, conflicting
branches), remote_name);
abort:
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index b51f8ae..0ed1ddd 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -585,8 +585,9 @@ static 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)) {
char *str = strbuf_detach(err, NULL);
ref_transaction_free(transaction);
diff --git a/builtin/replace.c b/builtin/replace.c
index 7528f3d..df060f8 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -170,8 +170,8 @@ static int replace_object_sha1(const char *object_ref,
transaction = ref_transaction_begin(err);
if (!transaction ||
ref_transaction_update(transaction, ref, repl, prev,
- 0, !is_null_sha1(prev), err) ||
- ref_transaction_commit(transaction, NULL, err))
+ 0, !is_null_sha1(prev), 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 c9bfc9a..74af63e 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, !is_null_sha1(prev), err) ||
- ref_transaction_commit(transaction, NULL, err))
+ 0, !is_null_sha1(prev), NULL, err) ||
+
Re: [msysGit] Re: The different EOL behavior between libgit2-based software and official Git
Torsten Bögershausen tbo...@web.de writes: tb@Linux:~/EOL_Test/TestAutoCrlf$ t=LF.txtrm -f $t git -c core.eol=CRLF checkout $t od -c $t 000 L i n e 1 \n l i n e ( 2 ) \n 020 l i n e 3 . \n t h i s i s 040 l i n e 4 \n l i n e N 060 o . 5 \n L i n e N u m b e r 100 6 \n \n In Documentation/config.txt, we find: core.eol:: Sets the line ending type to use in the working directory for files that have the `text` property set. Alternatives are ... Does that file $t in your practice have the `text` property set? -- 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 1/7] rebase -i: Make option handling in pick_one more flexible
Michael Haggerty mhag...@alum.mit.edu writes:
pick_one () {
ff=--ff
+extra_args=
+while test $# -gt 0
+do
+case $1 in
+-n)
+ff=
+extra_args=$extra_args -n
+;;
+-*)
+warn pick_one: ignored option -- $1
+;;
This is an internal interface, right? I.e., user input isn't being
processed here? If so, then the presence of an unrecognized option is a
bug and it is preferable to die here rather than warn.
The same below and in at least one later commit.
And if this is purely an internal interface, then I really do not
see the point of allowing -n to be anywhere other than the front.
If we are planning to accept other random options to cherry-pick in
later steps, but we are not yet doing so at this step, then I do not
thin we want to have any loop like this before we actually start
accepting and passing them to the underlying cherry-pick.
Furthermore, if the -n is currently used as an internal signal
from the caller to pick_one() that it is executing the end-user
supplied squash in the insn sheet, it may be a good idea to change
that -n to something that is *NOT* a valid option to cherry-pick
at this step, before we start accepting user-supplied options and
relaying them to underlying cherry-pick.
One way to do so cleanly may be to _always_ add the type of pick as
the first parameter to pick_one, i.e. either pick or squash, and
do:
pick_one () {
...
n_arg=
case $1 in
pick) ;;
squash) n_arg=-n ;;
*) die BUG: pick_one $1??? ;;
esac
shift
sha1=$1
...
output eval git cherry-pick $n_arg \
...
}
Also I suspect that you would need to be careful *not* to allow -n
to be given as part of the random user-specified options and pass
that to cherry-pick in the later steps of your series [*1*], and for
that you may need a loop that inspects the arguments like you had in
this patch.
[Footnote]
*1* The existing callers of pick_one -n very well know and expect
that the step will only update the working tree and the index
and it is the callers' responsibility to create a commit out of
that state (either by amending or committing); similarly the
existing callers of pick_one without -n very well know and
expect that the step will make a commit unless there is a
problem. I do not think you would consider it such a problem
to replay the change in the named commit for the end user's
insn sheet to pass a -n.
--
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 v6 01/11] trace: move trace declarations from cache.h to new trace.h
Also include direct dependencies (strbuf.h and git-compat-util.h for __attribute__) so that trace.h can be used independently of cache.h, e.g. in test programs. Signed-off-by: Karsten Blees bl...@dcon.de Signed-off-by: Junio C Hamano gits...@pobox.com --- cache.h | 13 ++--- trace.h | 17 + 2 files changed, 19 insertions(+), 11 deletions(-) create mode 100644 trace.h diff --git a/cache.h b/cache.h index cbe1935..466f6b3 100644 --- a/cache.h +++ b/cache.h @@ -7,6 +7,7 @@ #include advice.h #include gettext.h #include convert.h +#include trace.h #include SHA1_HEADER #ifndef git_SHA_CTX @@ -1377,17 +1378,7 @@ extern void *alloc_tag_node(void); extern void *alloc_object_node(void); extern void alloc_report(void); -/* trace.c */ -__attribute__((format (printf, 1, 2))) -extern void trace_printf(const char *format, ...); -__attribute__((format (printf, 2, 3))) -extern void trace_argv_printf(const char **argv, const char *format, ...); -extern void trace_repo_setup(const char *prefix); -extern int trace_want(const char *key); -__attribute__((format (printf, 2, 3))) -extern void trace_printf_key(const char *key, const char *fmt, ...); -extern void trace_strbuf(const char *key, const struct strbuf *buf); - +/* pkt-line.c */ void packet_trace_identity(const char *prog); /* add */ diff --git a/trace.h b/trace.h new file mode 100644 index 000..6cc4541 --- /dev/null +++ b/trace.h @@ -0,0 +1,17 @@ +#ifndef TRACE_H +#define TRACE_H + +#include git-compat-util.h +#include strbuf.h + +__attribute__((format (printf, 1, 2))) +extern void trace_printf(const char *format, ...); +__attribute__((format (printf, 2, 3))) +extern void trace_argv_printf(const char **argv, const char *format, ...); +extern void trace_repo_setup(const char *prefix); +extern int trace_want(const char *key); +__attribute__((format (printf, 2, 3))) +extern void trace_printf_key(const char *key, const char *fmt, ...); +extern void trace_strbuf(const char *key, const struct strbuf *buf); + +#endif /* TRACE_H */ -- 2.0.0.402.g13b8b25 -- 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 v6 00/11] add performance tracing facility
Changes since v5: [05/11]: GIT_TRACE_BARE=1 disables 'timestamp file:line' output for unit tests that rely on trace output (t1510 and t5503) [08/11]: Align original trace output at col 40 [09/11]: Dropped '(div 10e9)' from the commit message. [10/11]: Dropped trace_performance[_since]() return value and the respective usage example. Renamed trace_performance_vfl to trace_performance_vprintf_fl. The other patches are the versions from pu. Karsten Blees (11): trace: move trace declarations from cache.h to new trace.h trace: consistently name the format parameter trace: remove redundant printf format attribute trace: factor out printing to the trace file trace: add infrastructure to augment trace output with additional info trace: add current timestamp to all trace output trace: move code around, in preparation to file:line output trace: add 'file:line' to all trace output trace: add high resolution timer function to debug performance issues trace: add trace_performance facility to debug performance issues git: add performance tracing for git's main() function to debug scripts Makefile | 7 ++ cache.h | 13 +-- config.mak.uname | 1 + git-compat-util.h | 4 + git.c | 2 + t/t1510-repo-setup.sh | 2 +- t/t5503-tagfollow.sh | 8 +- trace.c | 313 -- trace.h | 90 +++ 9 files changed, 387 insertions(+), 53 deletions(-) create mode 100644 trace.h -- 2.0.0.402.g13b8b25 -- 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 v6 03/11] trace: remove redundant printf format attribute
trace_printf_key() is the only non-static function that duplicates the
printf format attribute in the .c file, remove it for consistency.
Signed-off-by: Karsten Blees bl...@dcon.de
Signed-off-by: Junio C Hamano gits...@pobox.com
---
trace.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/trace.c b/trace.c
index 37a7fa9..3e31558 100644
--- a/trace.c
+++ b/trace.c
@@ -75,7 +75,6 @@ static void trace_vprintf(const char *key, const char
*format, va_list ap)
strbuf_release(buf);
}
-__attribute__((format (printf, 2, 3)))
void trace_printf_key(const char *key, const char *format, ...)
{
va_list ap;
--
2.0.0.402.g13b8b25
--
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 v6 02/11] trace: consistently name the format parameter
The format parameter to trace_printf functions is sometimes abbreviated
'fmt'. Rename to 'format' everywhere (consistent with POSIX' printf
specification).
Signed-off-by: Karsten Blees bl...@dcon.de
Signed-off-by: Junio C Hamano gits...@pobox.com
---
trace.c | 22 +++---
trace.h | 2 +-
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/trace.c b/trace.c
index 08180a9..37a7fa9 100644
--- a/trace.c
+++ b/trace.c
@@ -62,7 +62,7 @@ static int get_trace_fd(const char *key, int *need_close)
static const char err_msg[] = Could not trace into fd given by
GIT_TRACE environment variable;
-static void trace_vprintf(const char *key, const char *fmt, va_list ap)
+static void trace_vprintf(const char *key, const char *format, va_list ap)
{
struct strbuf buf = STRBUF_INIT;
@@ -70,25 +70,25 @@ static void trace_vprintf(const char *key, const char *fmt,
va_list ap)
return;
set_try_to_free_routine(NULL); /* is never reset */
- strbuf_vaddf(buf, fmt, ap);
+ strbuf_vaddf(buf, format, ap);
trace_strbuf(key, buf);
strbuf_release(buf);
}
__attribute__((format (printf, 2, 3)))
-void trace_printf_key(const char *key, const char *fmt, ...)
+void trace_printf_key(const char *key, const char *format, ...)
{
va_list ap;
- va_start(ap, fmt);
- trace_vprintf(key, fmt, ap);
+ va_start(ap, format);
+ trace_vprintf(key, format, ap);
va_end(ap);
}
-void trace_printf(const char *fmt, ...)
+void trace_printf(const char *format, ...)
{
va_list ap;
- va_start(ap, fmt);
- trace_vprintf(GIT_TRACE, fmt, ap);
+ va_start(ap, format);
+ trace_vprintf(GIT_TRACE, format, ap);
va_end(ap);
}
@@ -106,7 +106,7 @@ void trace_strbuf(const char *key, const struct strbuf *buf)
close(fd);
}
-void trace_argv_printf(const char **argv, const char *fmt, ...)
+void trace_argv_printf(const char **argv, const char *format, ...)
{
struct strbuf buf = STRBUF_INIT;
va_list ap;
@@ -117,8 +117,8 @@ void trace_argv_printf(const char **argv, const char *fmt,
...)
return;
set_try_to_free_routine(NULL); /* is never reset */
- va_start(ap, fmt);
- strbuf_vaddf(buf, fmt, ap);
+ va_start(ap, format);
+ strbuf_vaddf(buf, format, ap);
va_end(ap);
sq_quote_argv(buf, argv, 0);
diff --git a/trace.h b/trace.h
index 6cc4541..8fea50b 100644
--- a/trace.h
+++ b/trace.h
@@ -11,7 +11,7 @@ extern void trace_argv_printf(const char **argv, const char
*format, ...);
extern void trace_repo_setup(const char *prefix);
extern int trace_want(const char *key);
__attribute__((format (printf, 2, 3)))
-extern void trace_printf_key(const char *key, const char *fmt, ...);
+extern void trace_printf_key(const char *key, const char *format, ...);
extern void trace_strbuf(const char *key, const struct strbuf *buf);
#endif /* TRACE_H */
--
2.0.0.402.g13b8b25
--
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 v6 04/11] trace: factor out printing to the trace file
Opening and writing to the trace file is currently duplicated in
trace_strbuf() and trace_argv_printf(). Factor out this logic to prepare
for adding timestamp and file:line to trace output.
In case of trace_argv_printf(), this adds an additional trace_want() check
to prevent unnecessary string formatting.
Signed-off-by: Karsten Blees bl...@dcon.de
Signed-off-by: Junio C Hamano gits...@pobox.com
---
trace.c | 37 +++--
1 file changed, 19 insertions(+), 18 deletions(-)
diff --git a/trace.c b/trace.c
index 3e31558..b7ca51b 100644
--- a/trace.c
+++ b/trace.c
@@ -62,6 +62,21 @@ static int get_trace_fd(const char *key, int *need_close)
static const char err_msg[] = Could not trace into fd given by
GIT_TRACE environment variable;
+/* Print 'buf' verbatim to trace file designated by env var 'key' */
+static void do_trace_print(const char *key, const struct strbuf *buf)
+{
+ int fd, need_close = 0;
+
+ fd = get_trace_fd(key, need_close);
+ if (!fd)
+ return;
+
+ write_or_whine_pipe(fd, buf-buf, buf-len, err_msg);
+
+ if (need_close)
+ close(fd);
+}
+
static void trace_vprintf(const char *key, const char *format, va_list ap)
{
struct strbuf buf = STRBUF_INIT;
@@ -71,7 +86,7 @@ static void trace_vprintf(const char *key, const char
*format, va_list ap)
set_try_to_free_routine(NULL); /* is never reset */
strbuf_vaddf(buf, format, ap);
- trace_strbuf(key, buf);
+ do_trace_print(key, buf);
strbuf_release(buf);
}
@@ -93,26 +108,15 @@ void trace_printf(const char *format, ...)
void trace_strbuf(const char *key, const struct strbuf *buf)
{
- int fd, need_close = 0;
-
- fd = get_trace_fd(key, need_close);
- if (!fd)
- return;
-
- write_or_whine_pipe(fd, buf-buf, buf-len, err_msg);
-
- if (need_close)
- close(fd);
+ do_trace_print(key, buf);
}
void trace_argv_printf(const char **argv, const char *format, ...)
{
struct strbuf buf = STRBUF_INIT;
va_list ap;
- int fd, need_close = 0;
- fd = get_trace_fd(GIT_TRACE, need_close);
- if (!fd)
+ if (!trace_want(GIT_TRACE))
return;
set_try_to_free_routine(NULL); /* is never reset */
@@ -122,11 +126,8 @@ void trace_argv_printf(const char **argv, const char
*format, ...)
sq_quote_argv(buf, argv, 0);
strbuf_addch(buf, '\n');
- write_or_whine_pipe(fd, buf.buf, buf.len, err_msg);
+ do_trace_print(GIT_TRACE, buf);
strbuf_release(buf);
-
- if (need_close)
- close(fd);
}
static const char *quote_crnl(const char *path)
--
2.0.0.402.g13b8b25
--
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 v6 06/11] trace: add current timestamp to all trace output
This is useful to tell apart trace output of separate test runs.
It can also be used for basic, coarse-grained performance analysis. Note
that the accuracy is tainted by writing to the trace file, and you have to
calculate the deltas yourself (which is next to impossible if multiple
threads or processes are involved).
Signed-off-by: Karsten Blees bl...@dcon.de
Signed-off-by: Junio C Hamano gits...@pobox.com
---
trace.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/trace.c b/trace.c
index 9fa406e..9fa6cc7 100644
--- a/trace.c
+++ b/trace.c
@@ -81,6 +81,10 @@ static int trace_bare = -1;
static int prepare_trace_line(const char *key, struct strbuf *buf)
{
+ struct timeval tv;
+ struct tm tm;
+ time_t secs;
+
if (!trace_want(key))
return 0;
@@ -92,7 +96,12 @@ static int prepare_trace_line(const char *key, struct strbuf
*buf)
if (trace_bare)
return 1;
- /* add line prefix here */
+ /* print current timestamp */
+ gettimeofday(tv, NULL);
+ secs = tv.tv_sec;
+ localtime_r(secs, tm);
+ strbuf_addf(buf, %02d:%02d:%02d.%06ld , tm.tm_hour, tm.tm_min,
+ tm.tm_sec, tv.tv_usec);
return 1;
}
--
2.0.0.402.g13b8b25
--
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 v6 05/11] trace: add infrastructure to augment trace output with additional info
To be able to add a common prefix or suffix to all trace output (e.g.
a timestamp or file:line of the caller), factor out common setup and
cleanup tasks of the trace* functions.
Some unit-tests use trace output to verify internal state, and variable
output such as timestamps and line numbers are not useful there. Disable
additional trace output if GIT_TRACE_BARE is set.
When adding a common prefix, it makes sense that the output of each trace
call starts on a new line. Add '\n' in case the caller forgot.
Note that this explicitly limits trace output to line-by-line, it is no
longer possible to trace-print just part of a line. Until now, this was
just an implicit assumption (trace-printing part of a line worked, but
messed up the trace file if multiple threads or processes were involved).
Thread-safety / inter-process-safety is also the reason why we need to do
the prefixing and suffixing in memory rather than issuing multiple write()
calls. Write_or_whine_pipe() / xwrite() is atomic unless the size exceeds
MAX_IO_SIZE (8MB, see wrapper.c). In case of trace_strbuf, this costs an
additional string copy (which should be irrelevant for performance in light
of actual file IO).
While we're at it, rename trace_strbuf's 'buf' argument, which suggests
that the function is modifying the buffer. Trace_strbuf() currently is the
only trace API that can print arbitrary binary data (without barfing on
'%' or stopping at '\0'), so 'data' seems more appropriate.
Signed-off-by: Karsten Blees bl...@dcon.de
---
t/t1510-repo-setup.sh | 2 +-
t/t5503-tagfollow.sh | 8
trace.c | 53 ---
trace.h | 2 +-
4 files changed, 48 insertions(+), 17 deletions(-)
diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh
index e1b2a99..8db8d68 100755
--- a/t/t1510-repo-setup.sh
+++ b/t/t1510-repo-setup.sh
@@ -57,7 +57,7 @@ test_repo () {
export GIT_WORK_TREE
fi
rm -f trace
- GIT_TRACE_SETUP=$(pwd)/trace git symbolic-ref HEAD /dev/null
+ GIT_TRACE_BARE=1 GIT_TRACE_SETUP=$(pwd)/trace git
symbolic-ref HEAD /dev/null
grep '^setup: ' trace result
test_cmp expected result
)
diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
index f30c038..dc10143 100755
--- a/t/t5503-tagfollow.sh
+++ b/t/t5503-tagfollow.sh
@@ -56,7 +56,7 @@ test_expect_success 'fetch A (new commit : 1 connection)' '
rm -f $U
(
cd cloned
- GIT_TRACE_PACKET=$UPATH git fetch
+ GIT_TRACE_BARE=1 GIT_TRACE_PACKET=$UPATH git fetch
test $A = $(git rev-parse --verify origin/master)
)
get_needs $U actual
@@ -86,7 +86,7 @@ test_expect_success 'fetch C, T (new branch, tag : 1
connection)' '
rm -f $U
(
cd cloned
- GIT_TRACE_PACKET=$UPATH git fetch
+ GIT_TRACE_BARE=1 GIT_TRACE_PACKET=$UPATH git fetch
test $C = $(git rev-parse --verify origin/cat)
test $T = $(git rev-parse --verify tag1)
test $A = $(git rev-parse --verify tag1^0)
@@ -122,7 +122,7 @@ test_expect_success 'fetch B, S (commit and tag : 1
connection)' '
rm -f $U
(
cd cloned
- GIT_TRACE_PACKET=$UPATH git fetch
+ GIT_TRACE_BARE=1 GIT_TRACE_PACKET=$UPATH git fetch
test $B = $(git rev-parse --verify origin/master)
test $B = $(git rev-parse --verify tag2^0)
test $S = $(git rev-parse --verify tag2)
@@ -146,7 +146,7 @@ test_expect_success 'new clone fetch master and tags' '
cd clone2
git init
git remote add origin ..
- GIT_TRACE_PACKET=$UPATH git fetch
+ GIT_TRACE_BARE=1 GIT_TRACE_PACKET=$UPATH git fetch
test $B = $(git rev-parse --verify origin/master)
test $S = $(git rev-parse --verify tag2)
test $B = $(git rev-parse --verify tag2^0)
diff --git a/trace.c b/trace.c
index b7ca51b..9fa406e 100644
--- a/trace.c
+++ b/trace.c
@@ -77,17 +77,45 @@ static void do_trace_print(const char *key, const struct
strbuf *buf)
close(fd);
}
+static int trace_bare = -1;
+
+static int prepare_trace_line(const char *key, struct strbuf *buf)
+{
+ if (!trace_want(key))
+ return 0;
+
+ set_try_to_free_routine(NULL); /* is never reset */
+
+ /* unit tests may want to disable additional trace output */
+ if (trace_bare 0)
+ trace_bare = trace_want(GIT_TRACE_BARE);
+ if (trace_bare)
+ return 1;
+
+ /* add line prefix here */
+
+ return 1;
+}
+
+static void print_trace_line(const char *key, struct strbuf *buf)
+{
+ /* append newline if missing */
+ if (buf-len
[PATCH v6 07/11] trace: move code around, in preparation to file:line output
No functional changes, just move stuff around so that the next patch isn't
that ugly...
Signed-off-by: Karsten Blees bl...@dcon.de
Signed-off-by: Junio C Hamano gits...@pobox.com
---
trace.c | 36 ++--
trace.h | 12
2 files changed, 26 insertions(+), 22 deletions(-)
diff --git a/trace.c b/trace.c
index 9fa6cc7..869fb7b 100644
--- a/trace.c
+++ b/trace.c
@@ -127,20 +127,20 @@ static void trace_vprintf(const char *key, const char
*format, va_list ap)
print_trace_line(key, buf);
}
-void trace_printf_key(const char *key, const char *format, ...)
+void trace_argv_printf(const char **argv, const char *format, ...)
{
+ struct strbuf buf = STRBUF_INIT;
va_list ap;
- va_start(ap, format);
- trace_vprintf(key, format, ap);
- va_end(ap);
-}
-void trace_printf(const char *format, ...)
-{
- va_list ap;
+ if (!prepare_trace_line(GIT_TRACE, buf))
+ return;
+
va_start(ap, format);
- trace_vprintf(GIT_TRACE, format, ap);
+ strbuf_vaddf(buf, format, ap);
va_end(ap);
+
+ sq_quote_argv(buf, argv, 0);
+ print_trace_line(GIT_TRACE, buf);
}
void trace_strbuf(const char *key, const struct strbuf *data)
@@ -154,20 +154,20 @@ void trace_strbuf(const char *key, const struct strbuf
*data)
print_trace_line(key, buf);
}
-void trace_argv_printf(const char **argv, const char *format, ...)
+void trace_printf(const char *format, ...)
{
- struct strbuf buf = STRBUF_INIT;
va_list ap;
-
- if (!prepare_trace_line(GIT_TRACE, buf))
- return;
-
va_start(ap, format);
- strbuf_vaddf(buf, format, ap);
+ trace_vprintf(GIT_TRACE, format, ap);
va_end(ap);
+}
- sq_quote_argv(buf, argv, 0);
- print_trace_line(GIT_TRACE, buf);
+void trace_printf_key(const char *key, const char *format, ...)
+{
+ va_list ap;
+ va_start(ap, format);
+ trace_vprintf(key, format, ap);
+ va_end(ap);
}
static const char *quote_crnl(const char *path)
diff --git a/trace.h b/trace.h
index e03db2f..5c7f2dc 100644
--- a/trace.h
+++ b/trace.h
@@ -4,14 +4,18 @@
#include git-compat-util.h
#include strbuf.h
-__attribute__((format (printf, 1, 2)))
-extern void trace_printf(const char *format, ...);
-__attribute__((format (printf, 2, 3)))
-extern void trace_argv_printf(const char **argv, const char *format, ...);
extern void trace_repo_setup(const char *prefix);
extern int trace_want(const char *key);
+
+__attribute__((format (printf, 1, 2)))
+extern void trace_printf(const char *format, ...);
+
__attribute__((format (printf, 2, 3)))
extern void trace_printf_key(const char *key, const char *format, ...);
+
+__attribute__((format (printf, 2, 3)))
+extern void trace_argv_printf(const char **argv, const char *format, ...);
+
extern void trace_strbuf(const char *key, const struct strbuf *data);
#endif /* TRACE_H */
--
2.0.0.402.g13b8b25
--
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 v6 08/11] trace: add 'file:line' to all trace output
This is useful to see where trace output came from.
Add 'const char *file, int line' parameters to the printing functions and
rename them to *_fl.
Add trace_printf* and trace_strbuf macros resolving to the *_fl functions
and let the preprocessor fill in __FILE__ and __LINE__.
As the trace_printf* functions take a variable number of arguments, this
requires variadic macros (i.e. '#define foo(...) foo_impl(__VA_ARGS__)'.
Though part of C99, it is unclear whether older compilers support this.
Thus keep the old functions and only enable variadic macros for GNUC and
MSVC 2005+ (_MSC_VER 1400). This has the nice side effect that the old
C-style declarations serve as documentation how the macros are to be used.
Print 'file:line ' as prefix to each trace line. Align the remaining trace
output at column 40 to accommodate 18 char file names + 4 digit line
number (currently there are 30 *.c files of length 18 and just 11 of 19).
Trace output from longer source files (e.g. builtin/receive-pack.c) will
not be aligned.
Signed-off-by: Karsten Blees bl...@dcon.de
---
git-compat-util.h | 4
trace.c | 72 +--
trace.h | 49 +
3 files changed, 113 insertions(+), 12 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index b6f03b3..4dd065d 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -704,6 +704,10 @@ void git_qsort(void *base, size_t nmemb, size_t size,
#endif
#endif
+#if defined(__GNUC__) || (_MSC_VER = 1400)
+#define HAVE_VARIADIC_MACROS 1
+#endif
+
/*
* Preserves errno, prints a message, but gives no warning for ENOENT.
* Always returns the return value of unlink(2).
diff --git a/trace.c b/trace.c
index 869fb7b..5844f31 100644
--- a/trace.c
+++ b/trace.c
@@ -79,7 +79,8 @@ static void do_trace_print(const char *key, const struct
strbuf *buf)
static int trace_bare = -1;
-static int prepare_trace_line(const char *key, struct strbuf *buf)
+static int prepare_trace_line(const char *file, int line, const char *key,
+ struct strbuf *buf)
{
struct timeval tv;
struct tm tm;
@@ -103,6 +104,14 @@ static int prepare_trace_line(const char *key, struct
strbuf *buf)
strbuf_addf(buf, %02d:%02d:%02d.%06ld , tm.tm_hour, tm.tm_min,
tm.tm_sec, tv.tv_usec);
+#ifdef HAVE_VARIADIC_MACROS
+ /* print file:line */
+ strbuf_addf(buf, %s:%d , file, line);
+ /* align trace output (column 40 catches most files names in git) */
+ while (buf-len 40)
+ strbuf_addch(buf, ' ');
+#endif
+
return 1;
}
@@ -116,49 +125,52 @@ static void print_trace_line(const char *key, struct
strbuf *buf)
strbuf_release(buf);
}
-static void trace_vprintf(const char *key, const char *format, va_list ap)
+static void trace_vprintf_fl(const char *file, int line, const char *key,
+const char *format, va_list ap)
{
struct strbuf buf = STRBUF_INIT;
- if (!prepare_trace_line(key, buf))
+ if (!prepare_trace_line(file, line, key, buf))
return;
strbuf_vaddf(buf, format, ap);
print_trace_line(key, buf);
}
-void trace_argv_printf(const char **argv, const char *format, ...)
+static void trace_argv_vprintf_fl(const char *file, int line,
+ const char **argv, const char *format,
+ va_list ap)
{
struct strbuf buf = STRBUF_INIT;
- va_list ap;
- if (!prepare_trace_line(GIT_TRACE, buf))
+ if (!prepare_trace_line(file, line, GIT_TRACE, buf))
return;
- va_start(ap, format);
strbuf_vaddf(buf, format, ap);
- va_end(ap);
sq_quote_argv(buf, argv, 0);
print_trace_line(GIT_TRACE, buf);
}
-void trace_strbuf(const char *key, const struct strbuf *data)
+void trace_strbuf_fl(const char *file, int line, const char *key,
+const struct strbuf *data)
{
struct strbuf buf = STRBUF_INIT;
- if (!prepare_trace_line(key, buf))
+ if (!prepare_trace_line(file, line, key, buf))
return;
strbuf_addbuf(buf, data);
print_trace_line(key, buf);
}
+#ifndef HAVE_VARIADIC_MACROS
+
void trace_printf(const char *format, ...)
{
va_list ap;
va_start(ap, format);
- trace_vprintf(GIT_TRACE, format, ap);
+ trace_vprintf_fl(NULL, 0, GIT_TRACE, format, ap);
va_end(ap);
}
@@ -166,10 +178,46 @@ void trace_printf_key(const char *key, const char
*format, ...)
{
va_list ap;
va_start(ap, format);
- trace_vprintf(key, format, ap);
+ trace_vprintf_fl(NULL, 0, key, format, ap);
va_end(ap);
}
+void trace_argv_printf(const char **argv, const char *format, ...)
+{
+ va_list ap;
+ va_start(ap, format);
+ trace_argv_vprintf_fl(NULL, 0, argv,
[PATCH v6 09/11] trace: add high resolution timer function to debug performance issues
Add a getnanotime() function that returns nanoseconds since 01/01/1970 as
unsigned 64-bit integer (i.e. overflows in july 2554). This is easier to
work with than e.g. struct timeval or struct timespec. Basing the timer on
the epoch allows using the results with other time-related APIs.
To simplify adaption to different platforms, split the implementation into
a common getnanotime() and a platform-specific highres_nanos() function.
The common getnanotime() function handles errors, falling back to
gettimeofday() if highres_nanos() isn't implemented or doesn't work.
getnanotime() is also responsible for normalizing to the epoch. The offset
to the system clock is calculated only once on initialization, i.e.
manually setting the system clock has no impact on the timer (except if
the fallback gettimeofday() is in use). Git processes are typically short
lived, so we don't need to handle clock drift.
The highres_nanos() function returns monotonically increasing nanoseconds
relative to some arbitrary point in time (e.g. system boot), or 0 on
failure. Providing platform-specific implementations should be relatively
easy, e.g. adapting to clock_gettime() as defined by the POSIX realtime
extensions is seven lines of code.
This version includes highres_nanos() implementations for:
* Linux: using clock_gettime(CLOCK_MONOTONIC)
* Windows: using QueryPerformanceCounter()
Todo:
* enable clock_gettime() on more platforms
* add Mac OSX version, e.g. using mach_absolute_time + mach_timebase_info
Signed-off-by: Karsten Blees bl...@dcon.de
Signed-off-by: Junio C Hamano gits...@pobox.com
---
Makefile | 7 +
config.mak.uname | 1 +
trace.c | 82
trace.h | 1 +
4 files changed, 91 insertions(+)
diff --git a/Makefile b/Makefile
index 07ea105..80f4390 100644
--- a/Makefile
+++ b/Makefile
@@ -340,6 +340,8 @@ all::
#
# Define GMTIME_UNRELIABLE_ERRORS if your gmtime() function does not
# return NULL when it receives a bogus time_t.
+#
+# Define HAVE_CLOCK_GETTIME if your platform has clock_gettime in librt.
GIT-VERSION-FILE: FORCE
@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1497,6 +1499,11 @@ ifdef GMTIME_UNRELIABLE_ERRORS
BASIC_CFLAGS += -DGMTIME_UNRELIABLE_ERRORS
endif
+ifdef HAVE_CLOCK_GETTIME
+ BASIC_CFLAGS += -DHAVE_CLOCK_GETTIME
+ EXTLIBS += -lrt
+endif
+
ifeq ($(TCLTK_PATH),)
NO_TCLTK = NoThanks
endif
diff --git a/config.mak.uname b/config.mak.uname
index 1ae675b..dad2618 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -34,6 +34,7 @@ ifeq ($(uname_S),Linux)
HAVE_PATHS_H = YesPlease
LIBC_CONTAINS_LIBINTL = YesPlease
HAVE_DEV_TTY = YesPlease
+ HAVE_CLOCK_GETTIME = YesPlease
endif
ifeq ($(uname_S),GNU/kFreeBSD)
HAVE_ALLOCA_H = YesPlease
diff --git a/trace.c b/trace.c
index 5844f31..88e05b9 100644
--- a/trace.c
+++ b/trace.c
@@ -275,3 +275,85 @@ int trace_want(const char *key)
return 0;
return 1;
}
+
+#ifdef HAVE_CLOCK_GETTIME
+
+static inline uint64_t highres_nanos(void)
+{
+ struct timespec ts;
+ if (clock_gettime(CLOCK_MONOTONIC, ts))
+ return 0;
+ return (uint64_t) ts.tv_sec * 10 + ts.tv_nsec;
+}
+
+#elif defined (GIT_WINDOWS_NATIVE)
+
+static inline uint64_t highres_nanos(void)
+{
+ static uint64_t high_ns, scaled_low_ns;
+ static int scale;
+ LARGE_INTEGER cnt;
+
+ if (!scale) {
+ if (!QueryPerformanceFrequency(cnt))
+ return 0;
+
+ /* high_ns = number of ns per cnt.HighPart */
+ high_ns = (10LL 32) / (uint64_t) cnt.QuadPart;
+
+ /*
+* Number of ns per cnt.LowPart is 10^9 / frequency (or
+* high_ns 32). For maximum precision, we scale this factor
+* so that it just fits within 32 bit (i.e. won't overflow if
+* multiplied with cnt.LowPart).
+*/
+ scaled_low_ns = high_ns;
+ scale = 32;
+ while (scaled_low_ns = 0x1LL) {
+ scaled_low_ns = 1;
+ scale--;
+ }
+ }
+
+ /* if QPF worked on initialization, we expect QPC to work as well */
+ QueryPerformanceCounter(cnt);
+
+ return (high_ns * cnt.HighPart) +
+ ((scaled_low_ns * cnt.LowPart) scale);
+}
+
+#else
+# define highres_nanos() 0
+#endif
+
+static inline uint64_t gettimeofday_nanos(void)
+{
+ struct timeval tv;
+ gettimeofday(tv, NULL);
+ return (uint64_t) tv.tv_sec * 10 + tv.tv_usec * 1000;
+}
+
+/*
+ * Returns nanoseconds since the epoch (01/01/1970), for performance tracing
+ * (i.e. favoring high precision over wall clock time accuracy).
+ */
+inline uint64_t getnanotime(void)
+{
+ static uint64_t offset;
+ if (offset 1) {
+ /* initialization
[PATCH v6 10/11] trace: add trace_performance facility to debug performance issues
Add trace_performance and trace_performance_since macros that print a
duration and an optional printf-formatted text to the file specified in
environment variable GIT_TRACE_PERFORMANCE.
These macros, in conjunction with getnanotime(), are intended to simplify
performance measurements from within the application (i.e. profiling via
manual instrumentation, rather than using an external profiling tool).
Unless enabled via GIT_TRACE_PERFORMANCE, these macros have no noticeable
impact on performance, so that test code for well known time killers may
be shipped in release builds. Alternatively, a developer could provide an
additional performance patch (not meant for master) that allows reviewers
to reproduce performance tests more easily, e.g. on other platforms or
using their own repositories.
Usage examples:
Simple use case (measure one code section):
uint64_t start = getnanotime();
/* code section to measure */
trace_performance_since(start, foobar);
Complex use case (measure repetitive code sections):
uint64_t t = 0;
for (;;) {
/* ignore */
t -= getnanotime();
/* code section to measure */
t += getnanotime();
/* ignore */
}
trace_performance(t, frotz);
Signed-off-by: Karsten Blees bl...@dcon.de
---
trace.c | 47 +++
trace.h | 18 ++
2 files changed, 65 insertions(+)
diff --git a/trace.c b/trace.c
index 88e05b9..65cd887 100644
--- a/trace.c
+++ b/trace.c
@@ -164,6 +164,27 @@ void trace_strbuf_fl(const char *file, int line, const
char *key,
print_trace_line(key, buf);
}
+static const char *GIT_TRACE_PERFORMANCE = GIT_TRACE_PERFORMANCE;
+
+static void trace_performance_vprintf_fl(const char *file, int line,
+uint64_t nanos, const char *format,
+va_list ap)
+{
+ struct strbuf buf = STRBUF_INIT;
+
+ if (!prepare_trace_line(file, line, GIT_TRACE_PERFORMANCE, buf))
+ return;
+
+ strbuf_addf(buf, performance: %.9f s, (double) nanos / 10);
+
+ if (format *format) {
+ strbuf_addstr(buf, : );
+ strbuf_vaddf(buf, format, ap);
+ }
+
+ print_trace_line(GIT_TRACE_PERFORMANCE, buf);
+}
+
#ifndef HAVE_VARIADIC_MACROS
void trace_printf(const char *format, ...)
@@ -195,6 +216,23 @@ void trace_strbuf(const char *key, const struct strbuf
*data)
trace_strbuf_fl(NULL, 0, key, data);
}
+void trace_performance(uint64_t nanos, const char *format, ...)
+{
+ va_list ap;
+ va_start(ap, format);
+ trace_performance_vprintf_fl(NULL, 0, nanos, format, ap);
+ va_end(ap);
+}
+
+void trace_performance_since(uint64_t start, const char *format, ...)
+{
+ va_list ap;
+ va_start(ap, format);
+ trace_performance_vprintf_fl(NULL, 0, getnanotime() - start,
+format, ap);
+ va_end(ap);
+}
+
#else
void trace_printf_key_fl(const char *file, int line, const char *key,
@@ -215,6 +253,15 @@ void trace_argv_printf_fl(const char *file, int line,
const char **argv,
va_end(ap);
}
+void trace_performance_fl(const char *file, int line, uint64_t nanos,
+ const char *format, ...)
+{
+ va_list ap;
+ va_start(ap, format);
+ trace_performance_vprintf_fl(file, line, nanos, format, ap);
+ va_end(ap);
+}
+
#endif /* HAVE_VARIADIC_MACROS */
diff --git a/trace.h b/trace.h
index c4964aa..f491471 100644
--- a/trace.h
+++ b/trace.h
@@ -21,6 +21,14 @@ extern void trace_argv_printf(const char **argv, const char
*format, ...);
extern void trace_strbuf(const char *key, const struct strbuf *data);
+/* Prints elapsed time (in nanoseconds) if GIT_TRACE_PERFORMANCE is enabled. */
+__attribute__((format (printf, 2, 3)))
+extern void trace_performance(uint64_t nanos, const char *format, ...);
+
+/* Prints elapsed time since 'start' if GIT_TRACE_PERFORMANCE is enabled. */
+__attribute__((format (printf, 2, 3)))
+extern void trace_performance_since(uint64_t start, const char *format, ...);
+
#else
/*
@@ -56,6 +64,13 @@ extern void trace_strbuf(const char *key, const struct
strbuf *data);
#define trace_strbuf(key, data) \
trace_strbuf_fl(__FILE__, __LINE__, key, data)
+#define trace_performance(nanos, ...) \
+ trace_performance_fl(__FILE__, __LINE__, nanos, __VA_ARGS__)
+
+#define trace_performance_since(start, ...) \
+ trace_performance_fl(__FILE__, __LINE__, getnanotime() - (start), \
+__VA_ARGS__)
+
/* backend functions, use non-*fl macros instead */
__attribute__((format (printf, 4, 5)))
extern void trace_printf_key_fl(const char *file, int line, const char *key,
@@ -65,6 +80,9 @@ extern void trace_argv_printf_fl(const char *file, int line,
const char **argv,
const char *format, ...);
extern void trace_strbuf_fl(const char *file, int
[PATCH v6 11/11] git: add performance tracing for git's main() function to debug scripts
Use trace_performance to measure and print execution time and command line
arguments of the entire main() function. In constrast to the shell's 'time'
utility, which measures total time of the parent process, this logs all
involved git commands recursively. This is particularly useful to debug
performance issues of scripted commands (i.e. which git commands were
called with which parameters, and how long did they execute).
Due to git's deliberate use of exit(), the implementation uses an atexit
routine rather than just adding trace_performance_since() at the end of
main().
Usage example: GIT_TRACE_PERFORMANCE=~/git-trace.log git stash list
Creates a log file like this:
23:57:38.638765 trace.c:405 performance: 0.000310107 s: git command: 'git'
'rev-parse' '--git-dir'
23:57:38.644387 trace.c:405 performance: 0.000261759 s: git command: 'git'
'rev-parse' '--show-toplevel'
23:57:38.646207 trace.c:405 performance: 0.000304468 s: git command: 'git'
'config' '--get-colorbool' 'color.interactive'
23:57:38.648491 trace.c:405 performance: 0.000241667 s: git command: 'git'
'config' '--get-color' 'color.interactive.help' 'red bold'
23:57:38.650465 trace.c:405 performance: 0.000243063 s: git command: 'git'
'config' '--get-color' '' 'reset'
23:57:38.654850 trace.c:405 performance: 0.025126313 s: git command: 'git'
'stash' 'list'
Signed-off-by: Karsten Blees bl...@dcon.de
Signed-off-by: Junio C Hamano gits...@pobox.com
---
git.c | 2 ++
trace.c | 22 ++
trace.h | 1 +
3 files changed, 25 insertions(+)
diff --git a/git.c b/git.c
index 7780572..d4daeb8 100644
--- a/git.c
+++ b/git.c
@@ -568,6 +568,8 @@ int main(int argc, char **av)
git_setup_gettext();
+ trace_command_performance(argv);
+
/*
* git- is the same as git , but we obviously:
*
diff --git a/trace.c b/trace.c
index 65cd887..dcd2685 100644
--- a/trace.c
+++ b/trace.c
@@ -404,3 +404,25 @@ inline uint64_t getnanotime(void)
return now;
}
}
+
+static uint64_t command_start_time;
+static struct strbuf command_line = STRBUF_INIT;
+
+static void print_command_performance_atexit(void)
+{
+ trace_performance_since(command_start_time, git command:%s,
+ command_line.buf);
+}
+
+void trace_command_performance(const char **argv)
+{
+ if (!trace_want(GIT_TRACE_PERFORMANCE))
+ return;
+
+ if (!command_start_time)
+ atexit(print_command_performance_atexit);
+
+ strbuf_reset(command_line);
+ sq_quote_argv(command_line, argv, 0);
+ command_start_time = getnanotime();
+}
diff --git a/trace.h b/trace.h
index f491471..5d1f174 100644
--- a/trace.h
+++ b/trace.h
@@ -7,6 +7,7 @@
extern void trace_repo_setup(const char *prefix);
extern int trace_want(const char *key);
extern uint64_t getnanotime(void);
+extern void trace_command_performance(const char **argv);
#ifndef HAVE_VARIADIC_MACROS
--
2.0.0.402.g13b8b25
--
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: Our merge bases sometimes suck
On Fri, Jun 20, 2014 at 1:53 AM, Junio C Hamano gits...@pobox.com wrote: Michael Haggerty mhag...@alum.mit.edu writes: [...] Hmph, but that obviously will become very expensive to compute as project grows. That's the main reason to like Fossil's approach (namely, the use of SQL, specifically SQLite3): you can write declarative queries and let the query planner optimize. (That's also about the only thing I like about Fossil: the use of SQL/SQLite3, and all that that implies. Fossil is otherwise an anti-git, and that makes it useless for me.) Pardon the intrusion, Nico -- -- 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
What's cooking in git.git (Jun 2014, #05; Fri, 20)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. Many topics that have been cooking in 'next' during the previous cycle and also some new topics since this cycle opened, totalling slightly more than 300 individual patches are in 'master' now. We have also accumulated some fixes we need to merge down to 'maint' and cut a v2.0.1 sometime next week. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to master] * jc/test-lazy-prereq (2014-06-13) 6 commits (merged to 'next' on 2014-06-10 at 4f774f7) + t3419: drop unnecessary NOT_EXPENSIVE pseudo-prerequisite + t3302: drop unnecessary NOT_EXPENSIVE pseudo-prerequisite + t3302: do not chdir around in the primary test process + t3302: coding style updates + test: turn USR_BIN_TIME into a lazy prerequisite + test: turn EXPENSIVE into a lazy prerequisite Test-script clean-up. * jc/revision-dash-count-parsing (2014-06-09) 1 commit (merged to 'next' on 2014-06-10 at 1aeca19) + revision: parse git log -count more carefully git log -2master is a common typo that shows two commits starting from whichever random branch that is not 'master' that happens to be checked out currently. * jm/api-strbuf-doc (2014-06-09) 1 commit (merged to 'next' on 2014-06-10 at 831aa30) + api-strbuf.txt minor typos * mt/send-email-cover-to-cc (2014-06-10) 3 commits (merged to 'next' on 2014-06-10 at 6bb1465) + t9001: avoid non-portable '\n' with sed + test/send-email: to-cover, cc-cover tests + git-send-email: two new options: to-cover, cc-cover Originally merged to 'next' on 2014-06-10 git send-email learns two new options. * rs/more-starts-with (2014-06-09) 1 commit (merged to 'next' on 2014-06-10 at efcd02e) + Use starts_with() for C strings instead of memcmp() * tb/t5551-clone-notice-to-stderr (2014-06-09) 1 commit (merged to 'next' on 2014-06-10 at 374082c) + t5551: fix the 50,000 tag test * jc/fetch-pull-refmap (2014-06-12) 10 commits (merged to 'next' on 2014-06-12 at 5428530) + docs: Explain the purpose of fetch's and pull's refspec parameter. (merged to 'next' on 2014-06-10 at 13c13ae) + fetch: allow explicit --refmap to override configuration + fetch doc: add a section on configured remote-tracking branches + fetch doc: remove short-cut section + fetch doc: update refspec format description + fetch doc: on pulling multiple refspecs + fetch doc: remove notes on outdated mixed layout + fetch doc: update note on '+' in front of the refspec + fetch doc: move FETCH_HEAD material lower and add an example + fetch doc: update introductory part for clarity -- [New Topics] * dt/refs-check-refname-component-sse (2014-06-18) 1 commit (merged to 'next' on 2014-06-20 at d286027) + refs.c: SSE2 optimizations for check_refname_component Further micro-optimization of a leaf-function. * tb/unicode-7.0-display-width (2014-06-18) 1 commit (merged to 'next' on 2014-06-20 at 111b246) + Update of unicode_width.h to Unicode Version 7.0 Will merge to 'master'. * ye/http-extract-charset (2014-06-17) 1 commit (merged to 'next' on 2014-06-20 at 9492bae) + http: fix charset detection of extract_content_type() Will merge to 'master'. * jk/skip-prefix (2014-06-20) 18 commits - http-push: refactor parsing of remote object names - imap-send: use skip_prefix instead of using magic numbers - use skip_prefix to avoid repeated calculations - git: avoid magic number with skip_prefix - fetch-pack: refactor parsing in get_ack - fast-import: refactor parsing of spaces - stat_opt: check extra strlen call - daemon: use skip_prefix to avoid magic numbers - fast-import: use skip_prefix for parsing input - use skip_prefix to avoid repeating strings - use skip_prefix to avoid magic numbers - transport-helper: avoid reading past end-of-string - fast-import: fix read of uninitialized argv memory - apply: use skip_prefix instead of raw addition - refactor skip_prefix to return a boolean - avoid using skip_prefix as a boolean - daemon: mark some strings as const - parse_diff_color_slot: drop ofs parameter Will merge to 'next'. * jk/xstrfmt (2014-06-19) 10 commits - unique_path: fix unlikely heap overflow - walker_fetch: fix minor memory leak - merge: use argv_array when spawning merge strategy - sequencer: use argv_array_pushf - setup_git_env: use git_pathdup instead of xmalloc + sprintf - use xstrfmt to replace xmalloc + strcpy/strcat - use xstrfmt to replace xmalloc + sprintf - use xstrdup instead of xmalloc + strcpy - use xstrfmt in favor of manual size calculations - strbuf: add xstrfmt helper Will merge to 'next'. * jm/dedup-name-compare (2014-06-20) 2 commits - cleanup duplicate
Re: Tool/Scripts - For maintaining different branches on a repo
On Thu, Jun 19, 2014 at 04:18:22PM +0530, Jagan Teki wrote: Hi, I have a single repo with different kinds of branches say 4 branches. Developers will send a patches wrt to specific branch. I presume here that you're referring to emailed patches, or patches in independent files, as opposed to just having branches with commits. Is there any opensource tool/script that does applying patches/maintaining the branches in repo w/o manual intervention? If you want something that works with patches specifically, TopGit might do what you want. If what you're looking for is a tool that accepts patches and automatically applies them, I'm not aware of one. It shouldn't be terribly difficult to script, though. If you don't need to deal with patches and can instead deal with git repositories, GitLab and Gitorious offer merge requests, which might make life easier. I have heard that GitLab is less painful to set up. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: Digital signature
Re: [PATCH v6 05/11] trace: add infrastructure to augment trace output with additional info
Karsten Blees karsten.bl...@gmail.com writes: To be able to add a common prefix or suffix to all trace output (e.g. a timestamp or file:line of the caller), factor out common setup and cleanup tasks of the trace* functions. Some unit-tests use trace output to verify internal state, and variable output such as timestamps and line numbers are not useful there. Disable additional trace output if GIT_TRACE_BARE is set. Hmph, this makes me wonder if we are better off making these additional trace output optional, i.e. not disabling with GIT_TRACE_BARE like this, but show the new output only when explicitly asked for by setting GIT_TRACE_PERF or something. -- 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 v6 00/11] add performance tracing facility
From: Karsten Blees karsten.bl...@gmail.com Changes since v5: [05/11]: GIT_TRACE_BARE=1 disables 'timestamp file:line' output for unit tests that rely on trace output (t1510 and t5503) [08/11]: Align original trace output at col 40 [09/11]: Dropped '(div 10e9)' from the commit message. [10/11]: Dropped trace_performance[_since]() return value and the respective usage example. Renamed trace_performance_vfl to trace_performance_vprintf_fl. The other patches are the versions from pu. Karsten Blees (11): trace: move trace declarations from cache.h to new trace.h trace: consistently name the format parameter trace: remove redundant printf format attribute trace: factor out printing to the trace file trace: add infrastructure to augment trace output with additional info trace: add current timestamp to all trace output trace: move code around, in preparation to file:line output trace: add 'file:line' to all trace output trace: add high resolution timer function to debug performance issues trace: add trace_performance facility to debug performance issues git: add performance tracing for git's main() function to debug scripts Makefile | 7 ++ cache.h | 13 +-- config.mak.uname | 1 + git-compat-util.h | 4 + git.c | 2 + t/t1510-repo-setup.sh | 2 +- t/t5503-tagfollow.sh | 8 +- trace.c | 313 -- trace.h | 90 +++ 9 files changed, 387 insertions(+), 53 deletions(-) Should there be some documentation as well? Perhaps in t/README, or in Documentation/howto. create mode 100644 trace.h -- Philip -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 05/11] trace: add infrastructure to augment trace output with additional info
Am 21.06.2014 00:33, schrieb Junio C Hamano: Karsten Blees karsten.bl...@gmail.com writes: To be able to add a common prefix or suffix to all trace output (e.g. a timestamp or file:line of the caller), factor out common setup and cleanup tasks of the trace* functions. Some unit-tests use trace output to verify internal state, and variable output such as timestamps and line numbers are not useful there. Disable additional trace output if GIT_TRACE_BARE is set. Hmph, this makes me wonder if we are better off making these additional trace output optional, i.e. not disabling with GIT_TRACE_BARE like this, but show the new output only when explicitly asked for by setting GIT_TRACE_PERF or something. GIT_TRACE_VERBOSE perhaps? It affects all trace output, not just GIT_TRACE_PERFORMANCE. The tests would still have to disable it explicitly, though, in case someone sets it in their profile. However, IIRC conclusion of v4 discussion was that this would be useful for all trace output [1], so I think it should be the default. [1] https://groups.google.com/forum/#!topic/msysgit/UMKTvmZX5aI -- 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 v6 00/11] add performance tracing facility
Am 21.06.2014 00:49, schrieb Philip Oakley: Should there be some documentation as well? Perhaps in t/README, or in Documentation/howto. I'll add Documentation/technical/api-trace.txt when I find the time. But lets settle on the final API first. -- 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 3/7] rebase -i: Stop on root commits with empty log messages
On Wed, Jun 18, 2014 at 11:28 PM, Fabian Ruch baf...@gmail.com wrote:
When `rebase` is executed with `--root` but no `--onto` is specified,
`rebase` creates a sentinel commit which is replaced with the root
commit in three steps. This combination of options results never in a
fast-forward.
1. The sentinel commit is forced into the authorship of the root
commit.
2. The changes introduced by the root commit are applied to the index
but not committed. If this step fails for whatever reason, all
commit information will be there and the user can safely run
`git-commit --amend` after resolving the problems.
3. The new root commit is created by squashing the changes into the
sentinel commit which already carries the authorship of the
cherry-picked root commit.
The command line used to create the commit in the third step specifies
effectless and erroneous options. Remove those.
- `--allow-empty-message` is erroneous: If the root's commit message is
empty, the rebase shall fail like for any other commit that is on the
to-do list and has an empty commit message.
Fix the bug that git-rebase does not fail when the initial commit has
an empty log message but is replayed using `--root` is specified.
Add test.
- `-C` is effectless: The commit being amended, which is the sentinel
commit, already carries the authorship and log message of the
cherry-picked root commit. The committer email and commit date fields
are reset either way.
After all, if step two fails, `rebase --continue` won't include these
flags in the git-commit command line either.
Signed-off-by: Fabian Ruch baf...@gmail.com
---
git-rebase--interactive.sh | 4 ++--
t/t3412-rebase-root.sh | 39 +++
2 files changed, 41 insertions(+), 2 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index fffdfa5..f09eeae 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -539,8 +539,8 @@ do_pick () {
git commit --allow-empty --allow-empty-message --amend \
--no-post-rewrite -n -q -C $1
pick_one -n $1
- git commit --allow-empty --allow-empty-message \
- --amend --no-post-rewrite -n -q -C $1 \
+ git commit --allow-empty --amend \
+ --no-post-rewrite -n -q \
${gpg_sign_opt:+$gpg_sign_opt} ||
die_with_patch $1 Could not apply $1... $2
else
diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
index 0b52105..3608db4 100755
--- a/t/t3412-rebase-root.sh
+++ b/t/t3412-rebase-root.sh
@@ -278,4 +278,43 @@ test_expect_success 'rebase -i -p --root with conflict
(second part)' '
test_cmp expect-conflict-p out
'
+test_expect_success 'stop rebase --root on empty root log message' '
+ # create a root commit with a non-empty tree so that rebase does
+ # not fail because of an empty commit, and an empty log message
+ echo root-commit file
+ git add file
+ tree=$(git write-tree)
+ root=$(git commit-tree $tree /dev/null)
+ git checkout -b no-message-root-commit $root
+ # do not ff because otherwise neither the patch nor the message
+ # are looked at and checked for emptiness
+ test_must_fail env EDITOR=true git rebase -i --force-rebase --root
+ echo root-commit file.expected
+ test_cmp file file.expected
It is customary, and provides nicer diagnostic output upon failure, to
have the expected file mentioned first:
test_cmp file.expected file
+ git rebase --abort
Do you want to place this under control of test_when_finished
somewhere above the git rebase invocation to ensure cleanup if
something fails before this point?
+'
+
+test_expect_success 'stop rebase --root on empty child log message' '
+ # create a root commit with a non-empty tree and provide a log
+ # message so that rebase does not fail until the root commit is
+ # successfully replayed
+ echo root-commit file
+ git add file
+ tree=$(git write-tree)
+ root=$(git commit-tree $tree -m root-commit)
+ git checkout -b no-message-child-commit $root
+ # create a child commit with a non-empty patch so that rebase
+ # does not fail because of an empty commit, but an empty log
+ # message
+ echo child-commit file
+ git add file
+ git commit --allow-empty-message --no-edit
+ # do not ff because otherwise neither the patch nor the message
+ # are looked at and checked for emptiness
+ test_must_fail env EDITOR=true git rebase -i --force-rebase --root
+ echo child-commit file.expected
+ test_cmp file file.expected
+
Re: [PATCH v6 05/11] trace: add infrastructure to augment trace output with additional info
On Fri, Jun 20, 2014 at 4:32 PM, Karsten Blees karsten.bl...@gmail.com wrote: Am 21.06.2014 00:33, schrieb Junio C Hamano: Karsten Blees karsten.bl...@gmail.com writes: GIT_TRACE_VERBOSE perhaps? It affects all trace output, not just GIT_TRACE_PERFORMANCE. The tests would still have to disable it explicitly, though, in case someone sets it in their profile. However, IIRC conclusion of v4 discussion was that this would be useful for all trace output [1], so I think it should be the default. [1] https://groups.google.com/forum/#!topic/msysgit/UMKTvmZX5aI Hmph, after re-reading of that thread, I recall that I thought Peff's Would it make sense to just tie this to regular trace_* calls, and if GIT_TRACE_PERFORMANCE is set, add a timestamp to each line? was the conclusion (i.e. by default no change, ask with GIT_TRACE_PERFORMANCE), and that I found that reasonable back then. As to the need to disable in the tests, yes, you are right. Just like we give predictiable environment to tests by setting EDITOR, HOME, etc. in test-lib.sh, we should do the same for these new settings there. 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
