Re: The different EOL behavior between libgit2-based software and official Git
On 06/19/2014 04:59 AM, Yue Lin Ho wrote: Hi: ^_^ I did some test on the EOL behavior between official git and libgit2-based software(TortoiseGit). Then, I got that they have different EOL behavior. The blob stored in repository is a text file with mixed EOLs. Even set core.autocrlf = true, official git checkout the file as it is(means still *mixed EOLs* there). But, libgit2 checkout it with *All CRLF EOLs*. * The steps: * set core.autocrlf = false * add file with mixed EOLs * set core.autocrlf = true * delete that file in the working tree * checkout that file * examine the EOL If you are interested in this, you might take a look at my testing repository on GitHub. (https://github.com/YueLinHo/TestAutoCrlf) Thank you. Yue Lin Ho (I send a similar mail to msysgit, I'm not sure if this came trough) Sorry being late, I don't think there is something wrong with Git. The core.autocrlf is the "old" crlf handling, which has been in Git for a long time. If you exactly know what you are doing, know exactly which tools are doing what, convince everybody who pulls or pushes to that repo to use the same local config then it may be useful. In short: I would strongly recommend to use gitattributes, please see below. tb@msygit ~/temp $ git clone https://github.com/YueLinHo/TestAutoCrlf.git Cloning into 'TestAutoCrlf'... [snip] $ cd TestAutoCrlf/ tb@msygit ~/temp/TestAutoCrlf (master) $ ls CRLF.txt LF.txt MIX-more_CRLF.txt MIX-more_LF.txt Readme.md ## Check how the file looks like: $ od -c MIX-more_LF.txt 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 104 ### The file has one CRLF, the rest is LF, exactly how it had been ### commited. So this is what we expect. Or do I miss something ? ### Tell Git that MIX-more_LF.txt is a text file: $ echo MIX-more_LF.txt text >.gitattributes ### Verify that MIX-more_LF.txt is text, all other files ### are "binary" (or "-text" in Git language) $ git check-attr text * CRLF.txt: text: unspecified LF.txt: text: unspecified MIX-more_CRLF.txt: text: unspecified MIX-more_LF.txt: text: set Readme.md: text: unspecified ### Now ask Git to normalize the line endings in the working tree $ rm MIX-more_LF.txt tb@msygit ~/temp/TestAutoCrlf (master) $ git checkout MIX-more_LF.txt ## Check what we got: tb@msygit ~/temp/TestAutoCrlf (master) $ od -c MIX-more_LF.txt 000 L 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 040 i s l i n e 4 \r \n l i n 060 e N o . 5 \r \n L i n e N 100 u m b e r 6 \r \n 111 # (This is under Windows, under Linux I would expect only LF) # See core.eol for for information ## ## Now we need to normalize the file in the repo, ## All line endings should be LF, otherwise Git ## things the file is modified: $ git status On branch master Your branch is up-to-date with 'origin/master'. Changes not staged for commit: (use "git add ..." to update what will be committed) (use "git checkout -- ..." to discard changes in working directory) modified: MIX-more_LF.txt Untracked files: (use "git add ..." to include in what will be committed) .gitattributes no changes added to commit (use "git add" and/or "git commit -a") ### ### Do the normalization: tb@msygit ~/temp/TestAutoCrlf (master) $ git add MIX-more_LF.txt .gitattributes tb@msygit ~/temp/TestAutoCrlf (master) $ git commit -m "MIX-more_LF.txt is text" [master 200d874] MIX-more_LF.txt is text Committer: unknown Your name and email address were configured automatically based on your username and hostname. Please check that they are accurate. You can suppress this message by setting them explicitly: git config --global user.name "Your Name" git config --global user.email y...@example.com After doing this, you may fix the identity used for this commit with: git commit --amend --reset-author 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 .gitattributes tb@msygit ~/temp/TestAutoCrlf (master) $ From now on, MIX-more_LF.txt is treated as text by Git, and get CRLF under Windows. I think there is nothing wrong with Git here. If libgit2 does something different, we need to ask the libgit2 project, which is independent from Git -- 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 0/16] skip_prefix refactoring and cleanups
Hi,
I was about to send a patch like this series today, I am attaching it below.
-- >8 --
Subject: [PATCH] imap-send: use skip_prefix instead of using magic numbers
Signed-off-by: Tanay Abhra
---
imap-send.c | 6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index 83a6ed2..524fbab 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1328,13 +1328,9 @@ static char *imap_folder;
static int git_imap_config(const char *key, const char *val, void *cb)
{
- char imap_key[] = "imap.";
-
- if (strncmp(key, imap_key, sizeof imap_key - 1))
+ if (!skip_prefix(key, "imap.", &key))
return 0;
- key += sizeof imap_key - 1;
-
/* check booleans first, and barf on others */
if (!strcmp("sslverify", key))
server.ssl_verify = git_config_bool(key, val);
--
1.9.0.GIT
--
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
[RFC PATCH 7/7] rebase -i: Teach do_pick the options --amend and --file
The to-do list command `squash` and its close relative `fixup` replay
the changes of a commit like `pick` but do not recreate the commit.
Instead they replace the previous commit with a new commit that also
introduces the changes of the squashed commit. This is roughly like
cherry-picking without committing and using git-commit to amend the
previous commit.
The to-do list
pick a Some changes
squash b Some more changes
gets translated into the sequence of git commands
git cherry-pick a
git cherry-pick -n b
git commit --amend
and if `cherry-pick` supported `--amend` this would look even more like
the to-do list it is based on
git cherry-pick a
git cherry-pick --amend b.
Since `do_pick` takes care of `pick` entries and the above suggests
`squash` as an alias for `pick --amend`, teach `do_pick` to handle the
option `--amend` and reimplement `squash` in terms of `do_pick --amend`.
Also teach it the option `--file` which is used to specify `$squash_msg`
as commit message.
Both `--amend` and `--file` are commit rewriting options. If they are
encountered during options parsing, assign `rewrite` and pass `--amend`
(`--file` respectively) to the rewrite command. Be careful when
`--amend` is used to pick a root commit because HEAD might point to the
sentinel commit but there is still nothing to amend. Be sure to
initialize `$amend` so that commits are squashed even when `rebase` is
interrupted for resolving conflicts. It is not a mistake to do the
initialization regardless of any conflicts because `$amend` is always
cleared before the next to-do item is processed.
Signed-off-by: Fabian Ruch
---
Notes:
A question about when to enable the post-rewrite hook.
`rebase` collects the hashes of all processed commits using
`record_in_rewritten` and runs the post-rewrite script after the rebase
is complete. Two points seem to confuse me.
1) For a `pick` the hash is `record_in_rewritten` regardless of whether
the hash changed or not (the commit was recreated or the head was
fast-forwarded). Ok, the hook can figure that out. Is this behaviour
intended?
2) For a `reword` the amend disables the post-rewrite hook but for a
`squash` (or `fixup`) the hook is executed each time the squash
commit is amended. Does not this result in the hook being executed
twice for each scheduled `squash` command? Once for the amend and
once for the rebase. The hook most likely does not figure that out.
This patch never executes the post-rewrite hook when processing the
to-do list. The execution after the rebase is finished is still
conducted. I am uncertain whether this is correct. The tests seem to
succeed with both implementations.
git-rebase--interactive.sh | 65 --
1 file changed, 40 insertions(+), 25 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index ada520d..5ddc59d 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -493,7 +493,7 @@ record_in_rewritten() {
# Apply the changes introduced by the given commit to the current head.
#
-# do_pick [--edit]
+# do_pick [--file ] [--amend] [--edit]
#
# Wrapper around git-cherry-pick.
#
@@ -505,6 +505,17 @@ record_in_rewritten() {
# commit message. The editor contents becomes the commit message of
# the new head.
#
+# --amend
+# After picking , replace the current head commit with a new
+# commit that also introduces the changes of .
+#
+# _This is not a git-cherry-pick option._
+#
+# -F , --file
+# Take the commit message from the given file.
+#
+# _This is not a git-cherry-pick option._
+#
# The return value is 1 if applying the changes resulted in a conflict
# and 2 if the specified arguments were incorrect. If the changes could
# be applied successfully but creating the commit failed, a value
@@ -514,9 +525,30 @@ do_pick () {
rewrite=
rewrite_amend=
rewrite_edit=
+ rewrite_message=
while test $# -gt 0
do
case "$1" in
+ -F|--file)
+ if test $# -eq 0
+ then
+ warn "do_pick: option --file specified but no
given"
+ return 2
+ fi
+ rewrite=y
+ rewrite_message=$2
+ shift
+ ;;
+ --amend)
+ if test "$(git rev-parse HEAD)" = "$squash_onto" || !
git rev-parse --verify HEAD
+ then
+ warn "do_pick: nothing to amend"
+ return 2
+ fi
+ rewrite=y
+ rewrite_amend=y
+ git rev-parse --verify HEAD >"$amend"
+
[RFC PATCH 5/7] rebase -i: Do not die in do_pick
Since `do_pick` might be executed in a sub-shell (a modified author
environment for instance), calling `die` in `do_pick` has no effect but
exiting the sub-shell with a failure exit status. Moreover, if `do_pick`
is called while a squash or fixup is happening, `die_with_patch` will
discard `$squash_msg` as commit message. Indicate an error in `do_pick`
using return statements and properly kill the script at the call sites.
Remove unused commit message title argument from `do_pick`'s signature.
Signed-off-by: Fabian Ruch
---
git-rebase--interactive.sh | 33 ++---
1 file changed, 18 insertions(+), 15 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f903599..e4992dc 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -493,14 +493,10 @@ record_in_rewritten() {
# Apply the changes introduced by the given commit to the current head.
#
-# do_pick [--edit]
+# do_pick [--edit]
#
# Wrapper around git-cherry-pick.
#
-#
-# The commit message title of . Used for information
-# purposes only.
-#
#
# The commit to cherry-pick.
#
@@ -508,6 +504,12 @@ record_in_rewritten() {
# After picking , open an editor and let the user edit the
# commit message. The editor contents becomes the commit message of
# the new head.
+#
+# The return value is 1 if applying the changes resulted in a conflict
+# and 2 if the specified arguments were incorrect. If the changes could
+# be applied successfully but creating the commit failed, a value
+# greater than 2 is returned. No commit is created in either case and
+# the index is left with the (conflicting) changes in place.
do_pick () {
rewrite=
rewrite_amend=
@@ -528,7 +530,11 @@ do_pick () {
esac
shift
done
- test $# -ne 2 && die "do_pick: wrong number of arguments"
+ if test $# -ne 1
+ then
+ warn "do_pick: wrong number of arguments"
+ return 2
+ fi
if test "$(git rev-parse HEAD)" = "$squash_onto"
then
@@ -545,11 +551,9 @@ do_pick () {
# rebase --continue.
git commit --allow-empty --allow-empty-message --amend \
--no-post-rewrite -n -q -C $1 &&
- pick_one -n $1 ||
- die_with_patch $1 "Could not apply $1... $2"
+ pick_one -n $1 || return 1
else
- pick_one ${rewrite:+-n} $1 ||
- die_with_patch $1 "Could not apply $1... $2"
+ pick_one ${rewrite:+-n} $1 || return 1
fi
if test -n "$rewrite"
@@ -557,8 +561,7 @@ do_pick () {
git commit --allow-empty --no-post-rewrite -n -q \
${rewrite_amend:+--amend} \
${rewrite_edit:+--edit} \
- ${gpg_sign_opt:+"$gpg_sign_opt"} ||
- die_with_patch $1 "Could not rewrite commit after
successfully picking $1... $2"
+ ${gpg_sign_opt:+"$gpg_sign_opt"} || return 3
fi
}
@@ -573,21 +576,21 @@ do_next () {
comment_for_reflog pick
mark_action_done
- do_pick $sha1 "$rest"
+ do_pick $sha1 || die_with_patch $sha1 "Could not apply $sha1...
$rest"
record_in_rewritten $sha1
;;
reword|r)
comment_for_reflog reword
mark_action_done
- do_pick --edit $sha1 "$rest"
+ do_pick --edit $sha1 || die_with_patch $sha1 "Could not apply
$sha1... $rest"
record_in_rewritten $sha1
;;
edit|e)
comment_for_reflog edit
mark_action_done
- do_pick $sha1 "$rest"
+ do_pick $sha1 || die_with_patch $sha1 "Could not apply $sha1...
$rest"
warn "Stopped at $sha1... $rest"
exit_with_patch $sha1 0
;;
--
2.0.0
--
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
[RFC PATCH 6/7] rebase -i: Prepare for squash in terms of do_pick --amend
Rewrite `squash` and `fixup` handling in `do_next` using the atomic
sequence
pick_one
commit
in order to test the correctness of a single `do_squash` or parametrized
`do_pick` and make the subsequent patch reimplementing `squash` in terms
of such a single function more readable.
Might be squashed into the subsequent commit.
Signed-off-by: Fabian Ruch
---
git-rebase--interactive.sh | 20 +++-
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index e4992dc..ada520d 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -613,15 +613,15 @@ do_next () {
author_script_content=$(get_author_ident_from_commit HEAD)
echo "$author_script_content" > "$author_script"
eval "$author_script_content"
- if ! pick_one -n $sha1
- then
- git rev-parse --verify HEAD >"$amend"
- die_failed_squash $sha1 "$rest"
- fi
case "$(peek_next_command)" in
squash|s|fixup|f)
# This is an intermediate commit; its message will only
be
# used in case of trouble. So use the long version:
+ if ! pick_one -n $sha1
+ then
+ git rev-parse --verify HEAD >"$amend"
+ die_failed_squash $sha1 "Could not apply
$sha1... $rest"
+ fi
do_with_author output git commit --amend --no-verify -F
"$squash_msg" \
${gpg_sign_opt:+"$gpg_sign_opt"} ||
die_failed_squash $sha1 "$rest"
@@ -630,12 +630,22 @@ do_next () {
# This is the final command of this squash/fixup group
if test -f "$fixup_msg"
then
+ if ! pick_one -n $sha1
+ then
+ git rev-parse --verify HEAD >"$amend"
+ die_failed_squash $sha1 "Could not
apply $sha1... $rest"
+ fi
do_with_author git commit --amend --no-verify
-F "$fixup_msg" \
${gpg_sign_opt:+"$gpg_sign_opt"} ||
die_failed_squash $sha1 "$rest"
else
cp "$squash_msg" "$GIT_DIR"/SQUASH_MSG || exit
rm -f "$GIT_DIR"/MERGE_MSG
+ if ! pick_one -n $sha1
+ then
+ git rev-parse --verify HEAD >"$amend"
+ die_failed_squash $sha1 "Could not
apply $sha1... $rest"
+ fi
do_with_author git commit --amend --no-verify
-F "$GIT_DIR"/SQUASH_MSG -e \
${gpg_sign_opt:+"$gpg_sign_opt"} ||
die_failed_squash $sha1 "$rest"
--
2.0.0
--
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
[RFC PATCH 4/7] rebase -i: Commit only once when rewriting picks
The options passed to `do_pick` determine whether the picked commit will
be rewritten or not. If the commit gets rewritten, because the user
requested to edit the commit message for instance, let `pick_one` merely
apply the changes introduced by the commit and do not commit the
resulting tree yet. If the commit is replayed as is, leave it to
`pick_one` to recreate the commit (possibly by fast-forwarding the
head). This makes it easier to combine git-commit options like `--edit`
and `--amend` in `do_pick` because git-cherry-pick does not support
`--amend`.
In the case of `--edit`, do not `exit_with_patch` but assign `rewrite`
to pick the changes with `-n`. If the pick conflicts, no commit is
created which we would have to amend when continuing the rebase. To
complete the pick after the conflicts are resolved the user just resumes
with `git rebase --continue`.
If `rebase--interactive` is used to rebase a complete branch onto some
head, `rebase` creates a sentinel commit that requires special treatment
by `do_pick`. Do not finalize the pick here either because its commit
message can be altered as for any other pick. Since the orphaned root
commit gets a temporary parent, it is always rewritten. Safely use the
rewrite infrastructure of `do_pick` to create the final commit.
Signed-off-by: Fabian Ruch
---
git-rebase--interactive.sh | 31 ++-
1 file changed, 18 insertions(+), 13 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f09eeae..f903599 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -63,7 +63,8 @@ msgnum="$state_dir"/msgnum
author_script="$state_dir"/author-script
# When an "edit" rebase command is being processed, the SHA1 of the
-# commit to be edited is recorded in this file. When "git rebase
+# commit to be edited is recorded in this file. The same happens when
+# rewriting a commit fails, for instance "reword". When "git rebase
# --continue" is executed, if there are any staged changes then they
# will be amended to the HEAD commit, but only provided the HEAD
# commit is still the commit to be edited. When any other rebase
@@ -508,12 +509,15 @@ record_in_rewritten() {
# commit message. The editor contents becomes the commit message of
# the new head.
do_pick () {
- edit=
+ rewrite=
+ rewrite_amend=
+ rewrite_edit=
while test $# -gt 0
do
case "$1" in
-e|--edit)
- edit=y
+ rewrite=y
+ rewrite_edit=y
;;
-*)
warn "do_pick: ignored option -- $1"
@@ -528,6 +532,9 @@ do_pick () {
if test "$(git rev-parse HEAD)" = "$squash_onto"
then
+ rewrite=y
+ rewrite_amend=y
+ git rev-parse --verify HEAD >"$amend"
# Set the correct commit message and author info on the
# sentinel root before cherry-picking the original changes
# without committing (-n). Finally, update the sentinel again
@@ -538,22 +545,20 @@ do_pick () {
# rebase --continue.
git commit --allow-empty --allow-empty-message --amend \
--no-post-rewrite -n -q -C $1 &&
- pick_one -n $1 &&
- git commit --allow-empty --amend \
- --no-post-rewrite -n -q \
- ${gpg_sign_opt:+"$gpg_sign_opt"} ||
+ pick_one -n $1 ||
die_with_patch $1 "Could not apply $1... $2"
else
- pick_one $1 ||
+ pick_one ${rewrite:+-n} $1 ||
die_with_patch $1 "Could not apply $1... $2"
fi
- if test -n "$edit"
+ if test -n "$rewrite"
then
- git commit --allow-empty --amend --no-post-rewrite -n -q
${gpg_sign_opt:+"$gpg_sign_opt"} || {
- warn "Could not amend commit after successfully picking
$1... $2"
- exit_with_patch $1 1
- }
+ git commit --allow-empty --no-post-rewrite -n -q \
+ ${rewrite_amend:+--amend} \
+ ${rewrite_edit:+--edit} \
+ ${gpg_sign_opt:+"$gpg_sign_opt"} ||
+ die_with_patch $1 "Could not rewrite commit after
successfully picking $1... $2"
fi
}
--
2.0.0
--
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
[RFC PATCH 1/7] rebase -i: Make option handling in pick_one more flexible
`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
---
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"
+ ;;
+ *)
+ 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
}
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
output git checkout $first_parent 2> /dev/null ||
@@ -344,7 +373,7 @@ pick_one_preserving_merges () {
case "$new_parents" in
' '*' '*)
- test "a$1" = a-n && die "Refusing to squash a merge:
$sha1"
+ test -n "$no_commit" && die "Refusing to squash a
merge: $sha1"
# redo merge
author_script_content=$(get_author_ident_from_commit
$sha1)
@@ -365,7 +394,7 @@ pick_one_preserving_merges () {
*)
output eval git cherry-pick \
${gpg_sign_opt:+$(git rev-
[RFC PATCH 3/7] rebase -i: Stop on root commits with empty log messages
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
---
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 file.expected &&
+ test_cmp file file.expected &&
+ git rebase --abort
+'
+
+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 &&
+ git rebase --abort
+'
+
test_done
--
2.0.0
--
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
[RFC PATCH 2/7] rebase -i: Teach do_pick the option --edit
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.
Signed-off-by: Fabian Ruch
---
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]
+#
+# Wrapper around git-cherry-pick.
+#
+#
+# The commit message title of . Used for information
+# purposes only.
+#
+#
+# The commit to cherry-pick.
+#
+# -e, --edit
+# After picking , 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
# Set the correct commit message and author info on the
@@ -512,6 +547,14 @@ do_pick () {
pick_one $1 ||
die_with_patch $1 "Could not apply $1... $2"
fi
+
+ if test -n "$edit"
+ then
+ git commit --allow-empty --amend --no-post-rewrite -n -q
${gpg_sign_opt:+"$gpg_sign_opt"} || {
+ warn "Could not amend commit after successfully picking
$1... $2"
+ exit_with_patch $1 1
+ }
+ fi
}
do_next () {
@@ -532,14 +575,7 @@ do_next () {
comment_for_reflog reword
mark_action_done
- do_pick $sha1 "$rest"
- git commit --amend --no-post-rewrite
${gpg_sign_opt:+"$gpg_sign_opt"} || {
- warn "Could not amend commit after successfully picking
$sha1...
[RFC PATCH 0/7] rebase -i: Implement `reword` and `squash` in terms of `do_pick`
Hi, This patch series is part of the undertaking to add command line options to to-do list commands. The goal is to be able to write something similar to pick --reset-author --signoff a Some changes pick b Some more changes squash --no-edit c Ack Some more changes. The first submission to the mailing list adds options parsing to `do_pick` and reimplements the current set of to-do list commands in terms[1] of a parametrized `do_pick`. It neither adds new commands to to-do lists nor exposes the command line options feature to the user. Still it makes the implementation of `do_next` slightly more straight-forward and the function `do_pick` behave more like an internal git-cherry-pick. The patches try to accomplish one thing at a time but are not standalone in the sense that each of them constitutes a frame only in which a subsequent patch can express its one thing. For instance, only committing once and not dying in `do_pick` are things specific to implementing `squash` in terms of `do_pick` but presented as separate patches so that their correctness can be discussed independently. The last patch contains some notes about when to run the post-rewrite hook, something I could not figure out myself. Thanks for your time, Fabian [1] pick, reword, squash, fixup Fabian Ruch (7): rebase -i: Make option handling in pick_one more flexible rebase -i: Teach do_pick the option --edit rebase -i: Stop on root commits with empty log messages rebase -i: Commit only once when rewriting picks rebase -i: Do not die in do_pick rebase -i: Prepare for squash in terms of do_pick --amend rebase -i: Teach do_pick the options --amend and --file git-rebase--interactive.sh| 194 +++--- t/t3404-rebase-interactive.sh | 8 ++ t/t3412-rebase-root.sh| 39 + 3 files changed, 193 insertions(+), 48 deletions(-) -- 2.0.0 -- 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
The different EOL behavior between libgit2-based software and official Git
Hi: ^_^ I did some test on the EOL behavior between official git and libgit2-based software(TortoiseGit). Then, I got that they have different EOL behavior. The blob stored in repository is a text file with mixed EOLs. Even set core.autocrlf = true, official git checkout the file as it is(means still *mixed EOLs* there). But, libgit2 checkout it with *All CRLF EOLs*. * The steps: * set core.autocrlf = false * add file with mixed EOLs * set core.autocrlf = true * delete that file in the working tree * checkout that file * examine the EOL If you are interested in this, you might take a look at my testing repository on GitHub. (https://github.com/YueLinHo/TestAutoCrlf) Thank you. Yue Lin Ho -- View this message in context: http://git.661346.n2.nabble.com/The-different-EOL-behavior-between-libgit2-based-software-and-official-Git-tp7613670.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: [PATCH] gitk: use mktemp -d to avoid predictable temporary directories
On Mon, Jun 16, 2014 at 11:17:46AM -0700, Junio C Hamano wrote: > David Aguilar writes: > > > Hmm.. I guess what I could do is keep the old behavior (having gitk ignore > > TMPDIR) > > on Windows and only use the new code path on non-Windows. > > Or perhaps attempt to create, catch error and then retry the old way? > > Hopefully Windows folks do not have to worry about forgetting to > update the codepath when they update their tcl/wish if you did it > that way, no? True, that would be the safest. I just submitted a new replacement patch for these two patches. Thanks, -- David -- 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] gitk: catch mkdtemp errors
105b5d3fbb1c00bb0aeaf9d3e0fbe26a7b1993fc introduced a dependency
on mkdtemp, which is not available on Windows.
Use the original temporary directory behavior when mkdtemp fails.
This makes the code use mkdtemp when available and gracefully
fallback to the existing behavior when it is not available.
Helped-by: Junio C Hamano
Helped-by: brian m. carlson
Signed-off-by: David Aguilar
---
gitk | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/gitk b/gitk
index 41e5071..9237830 100755
--- a/gitk
+++ b/gitk
@@ -3504,7 +3504,9 @@ proc gitknewtmpdir {} {
set tmpdir $gitdir
}
set gitktmpformat [file join $tmpdir ".gitk-tmp.XX"]
- set gitktmpdir [exec mktemp -d $gitktmpformat]
+ if {[catch {set gitktmpdir [exec mktemp -d $gitktmpformat]}]} {
+ set gitktmpdir [file join $gitdir [format ".gitk-tmp.%s" [pid]]]
+ }
if {[catch {file mkdir $gitktmpdir} err]} {
error_popup "[mc "Error creating temporary directory %s:"
$gitktmpdir] $err"
unset gitktmpdir
--
2.0.0.257.g75cc6c6
--
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: Error 128 Clone succeeded, but checkout failed
Here is the fsck which is ok. Remember that if I do the clone to another file server it works fine. "c:\Program Files (x86)\Git\bin\git.exe" fsck --full Checking object directories: 100% (256/256), done. Checking objects: 100% (774/774), done. Checking connectivity: 5128, done. One thing I notice now, is that the object key is different on each run of the clone. 81f1bb353f860726835c6d265b3433a72c3fc082 vs a7b8f40dcafba3ec534db6d11e4b928775f26bcd and others we did yesterday. Also if we do the suggested command 'git checkout -f HEAD' it works fine. -Original Message- From: brian m. carlson [mailto:sand...@crustytoothpaste.net] Sent: Wednesday, June 18, 2014 5:25 PM To: Dodge, Warren L Cc: git@vger.kernel.org Subject: Re: Error 128 Clone succeeded, but checkout failed On Thu, Jun 19, 2014 at 12:11:43AM +, Dodge, Warren L wrote: > Hi Brian Hi. Please do keep the list in CC. Someone else may be able to help you better than I. > c:\Program Files (x86)\Git\bin\git.exe" --version git version > 1.9.4.msysgit.0 > > I also had 1.8.?? and it did the same thing so I updated to try that. > > This is exact. > "c:\Program Files (x86)\Git\bin\git.exe" clone --progress -v > VHDR_GIT_REPOSITORY fail Cloning into 'fail'... > done. > fatal: unable to read tree a7b8f40dcafba3ec534db6d11e4b928775f26bcd This means that git is unable to read this object. It could be missing or corrupt. What do you get if you try to run git fsck --full on this newly cloned repository? I realize it won't have a working directory, but git fsck should still run. > warning: Clone succeeded, but checkout failed. > You can inspect what was checked out with 'git status' > and retry the checkout with 'git checkout -f HEAD' > > I didn't get the status code but is was 128 when I did this in bash > > Our file servers are many terabytes and use DFS to break them up into > the file systems that are used. >From what I can tell of DFS from Wikipedia, it's using SMB under the hood, and >I'm not sure how well git works over SMB. Maybe some of the msysgit folks can >chime in here? -- 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 -- 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: Error 128 Clone succeeded, but checkout failed
On Thu, Jun 19, 2014 at 12:11:43AM +, Dodge, Warren L wrote: > Hi Brian Hi. Please do keep the list in CC. Someone else may be able to help you better than I. > c:\Program Files (x86)\Git\bin\git.exe" --version > git version 1.9.4.msysgit.0 > > I also had 1.8.?? and it did the same thing so I updated to try that. > > This is exact. > "c:\Program Files (x86)\Git\bin\git.exe" clone --progress -v > VHDR_GIT_REPOSITORY fail > Cloning into 'fail'... > done. > fatal: unable to read tree a7b8f40dcafba3ec534db6d11e4b928775f26bcd This means that git is unable to read this object. It could be missing or corrupt. What do you get if you try to run git fsck --full on this newly cloned repository? I realize it won't have a working directory, but git fsck should still run. > warning: Clone succeeded, but checkout failed. > You can inspect what was checked out with 'git status' > and retry the checkout with 'git checkout -f HEAD' > > I didn't get the status code but is was 128 when I did this in bash > > Our file servers are many terabytes and use DFS to break them up into > the file systems that are used. From what I can tell of DFS from Wikipedia, it's using SMB under the hood, and I'm not sure how well git works over SMB. Maybe some of the msysgit folks can chime in here? -- 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: Error 128 Clone succeeded, but checkout failed
On Wed, Jun 18, 2014 at 04:03:40PM -0700, warren.l.do...@tektronix.com wrote: > git.exe clone --progress -v L:\GIT_REPOSITORY L:\warrend\fail > > Cloning into L:\warrend\fail > Done. > Fatal: unable to read "Long hash key" > Warning: clone succeeded, but checkout failed. > You can inspect what was checked out with git status > And retry the checkout with git checkout -f HEAD What git version are you using? Also, is this the exact (copy-and-pasted) message you're getting? There are several similar messages starting with "unable to read", and knowing the exact error message (including casing) is important in order to be able to help you. -- 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
Conventions on struct copying?
I'm still working on the struct object_id patches, and I had a style question. In several places throughout the code, we do something like this: unsigned char a[20], b[20]; /* do some stuff with b */ hashcpy(a, b); I could implement an oidcpy that does the same thing. struct object_id a, b; /* do some stuff with b */ oidcpy(&a, &b); Or I could just write that as: a = b; and let the compiler do the heavy lifting. Is there any reason that we'd want the function for that purpose, or is it okay to just use the assignment? I don't know of any place we explicitly copy structs like this, but I don't know of any prohibition against it, either. -- 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
Error 128 Clone succeeded, but checkout failed
I have looked for an answer to the Error 128 Clone succeeded, but checkout failed message we are getting on a clone command. And there does not seem to be any that relates to our situation. The repository is on a local file server system that is mounted to the pc as L: If we clone the repository on to the L: directory structure we get the following git.exe clone --progress -v L:\GIT_REPOSITORY L:\warrend\fail Cloning into L:\warrend\fail Done. Fatal: unable to read "Long hash key" Warning: clone succeeded, but checkout failed. You can inspect what was checked out with git status And retry the checkout with git checkout -f HEAD Git did not exit cleanly (exit code 128) ( time and date etc) At this point there is only a .git directory at the destination We have another drive mounted as X: which utilizes a different file server. If we do this git.exe clone --progress -v L:\GIT_REPOSITORY X:\warrend\works It will clone and do the checkout properly. These does not seem to be any permission or disk space problems on the L: drive. We are unable to figure out why this is happening. I copied the two .git directories to a linux file system and did a diff -r of them and found this Bad one doesn't have the putty line for some reason diff -r fail/.git/config works/.git/config 12d11 < puttykeyfile = There was no index file in the bad tree. The config file which is in both trees was made after the index file. Only in .git: index I was hoping there was a debug method that would allow us to see what the actual check was that is failing. Any help on this would be greatly appreciated. -- 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 v18 16/48] refs.c: add an err argument to delete_ref_loose
On 06/19/2014 12:27 AM, Ronnie Sahlberg wrote: > It looks like we need to reorder two of the patches. > This patch needs to be moved to later in the series and happen after > the delete_ref conversion : > > refs.c: make delete_ref use a transaction > refs.c: add an err argument to delete_ref_loose That agrees with what I have found out since my first email. The failures go away starting with the later commit that you mentioned. > I will respin a v19 with these patches reordered. 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 1/2] strbuf: add xstrdup_fmt helper
Jeff King writes:
> You can use a strbuf to build up a string from parts, and
> then detach it. In the general case, you might use multiple
> strbuf_add* functions to do the building. However, in many
> cases, a single strbuf_addf is sufficient, and we end up
> with:
>
> struct strbuf buf = STRBUF_INIT;
> ...
> strbuf_addf(&buf, fmt, some, args);
> str = strbuf_detach(&buf, NULL);
>
> We can make this much more readable (and avoid introducing
> an extra variable, which can clutter the code) by
> introducing a convenience function:
>
> str = xstrdup_fmt(fmt, some, args);
>
> Signed-off-by: Jeff King
> ---
> I'm open to suggestions on the name. This really is the same thing
> conceptually as the GNU asprintf(), but the interface is different (that
> function takes a pointer-to-pointer as an out-parameter, and returns the
> number of characters return).
Naming it with anything "dup" certainly feels wrong. The returned
string is not a duplicate of anything.
To me, the function feels like an "sprintf done right"; as you said,
the best name for "printf-like format into an allocated piece of
memory" is unfortunately taken as asprintf(3).
I wonder if our callers can instead use asprintf(3) with its
slightly more quirky API (and then we supply compat/asprintf.c for
non-GNU platforms). Right now we only have three call sites, but if
we anticipate that "printf-like format into an allocated piece of
memory" will prove be generally useful in our code base, following
an API that other people already have established may give our
developers one less thing that they have to learn.
As usual, I would expect we would have xasprintf wrapper around it
to die instead of returning -1 upon allocation failure.
The call sites do not look too bad (see below) if we were to go that
route instead.
remote.c | 2 +-
unpack-trees.c | 10 ++
2 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/remote.c b/remote.c
index b46f467..87fa7ec 100644
--- a/remote.c
+++ b/remote.c
@@ -185,7 +185,7 @@ static struct branch *make_branch(const char *name, int len)
ret->name = xstrndup(name, len);
else
ret->name = xstrdup(name);
- ret->refname = xstrdup_fmt("refs/heads/%s", ret->name);
+ asprintf(&ret->refname, "refs/heads/%s", ret->name);
return ret;
}
diff --git a/unpack-trees.c b/unpack-trees.c
index dd1e06e..d6a07b8 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -63,8 +63,8 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options
*opts,
"Please, commit your changes or stash them before you
can %s.";
else
msg = "Your local changes to the following files would be
overwritten by %s:\n%%s";
- msgs[ERROR_WOULD_OVERWRITE] = msgs[ERROR_NOT_UPTODATE_FILE] =
- xstrdup_fmt(msg, cmd, cmd2);
+ xasprintf(&msgs[ERROR_WOULD_OVERWRITE], msg, cmd, cmd2);
+ msgs[ERROR_NOT_UPTODATE_FILE] = msgs[ERROR_WOULD_OVERWRITE];
msgs[ERROR_NOT_UPTODATE_DIR] =
"Updating the following directories would lose untracked files
in it:\n%s";
@@ -75,8 +75,10 @@ void setup_unpack_trees_porcelain(struct
unpack_trees_options *opts,
else
msg = "The following untracked working tree files would be %s
by %s:\n%%s";
- msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] = xstrdup_fmt(msg, "removed",
cmd, cmd2);
- msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = xstrdup_fmt(msg,
"overwritten", cmd, cmd2);
+ xasprintf(&msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED],
+msg, "removed", cmd, cmd2);
+ xasprintf(&msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN],
+msg, "overwritten", cmd, cmd2);
/*
* Special case: ERROR_BIND_OVERLAP refers to a pair of paths, we
--
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] alloc.c: remove alloc_raw_commit_node() function
On 18/06/14 21:08, Jeff King wrote:
> On Wed, Jun 18, 2014 at 08:52:46PM +0100, Ramsay Jones wrote:
>
[snip]
> Yeah, I noticed it while writing the patch but decided it wasn't worth
> the trouble to deal with (since after all, it's not advertised to any
> callers, the very thing that sparse is complaining about. :) ).
>
> I don't mind fixing it, though I really don't like repeating the
> contents of DEFINE_ALLOCATOR. I know it hasn't changed in a while, but
> it just feels wrong.
So, the patch below is a slight variation on the original patch.
I'm still slightly concerned about portability, but given that it
has been at least a decade since I last used a (pre-ANSI) compiler
which had a problem with this ...
[I have several versions of the C standard that I can use to check
up on the legalise, but I'm not sure I can be bothered! ;-) ]
ATB,
Ramsay Jones
-- >8 --
Subject: [PATCH] alloc.c: make alloc_raw_commit_node() a static function
In order to encapsulate the setting of the unique commit index, commit
969eba63 ("commit: push commit_index update into alloc_commit_node",
10-06-2014) introduced a (logically private) intermediary allocator
function. However, this function (alloc_raw_commit_node()) was declared
as a public function, which undermines its entire purpose.
Add a scope parameter to the DEFINE_ALLOCATOR macro to allow the
raw commit allocator definition to include the 'static' qualifier.
Noticed by sparse ("symbol 'alloc_raw_commit_node' was not declared.
Should it be static?").
Signed-off-by: Ramsay Jones
---
alloc.c | 17 ++---
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/alloc.c b/alloc.c
index eb22a45..5392d13 100644
--- a/alloc.c
+++ b/alloc.c
@@ -18,9 +18,12 @@
#define BLOCKING 1024
-#define DEFINE_ALLOCATOR(name, type) \
+#define PUBLIC
+#define PRIVATE static
+
+#define DEFINE_ALLOCATOR(scope, name, type)\
static unsigned int name##_allocs; \
-void *alloc_##name##_node(void)\
+scope void *alloc_##name##_node(void) \
{ \
static int nr; \
static type *block; \
@@ -45,11 +48,11 @@ union any_object {
struct tag tag;
};
-DEFINE_ALLOCATOR(blob, struct blob)
-DEFINE_ALLOCATOR(tree, struct tree)
-DEFINE_ALLOCATOR(raw_commit, struct commit)
-DEFINE_ALLOCATOR(tag, struct tag)
-DEFINE_ALLOCATOR(object, union any_object)
+DEFINE_ALLOCATOR(PUBLIC, blob, struct blob)
+DEFINE_ALLOCATOR(PUBLIC, tree, struct tree)
+DEFINE_ALLOCATOR(PRIVATE, raw_commit, struct commit)
+DEFINE_ALLOCATOR(PUBLIC, tag, struct tag)
+DEFINE_ALLOCATOR(PUBLIC, object, union any_object)
void *alloc_commit_node(void)
{
--
2.0.0
--
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 v18 16/48] refs.c: add an err argument to delete_ref_loose
It looks like we need to reorder two of the patches. This patch needs to be moved to later in the series and happen after the delete_ref conversion : refs.c: make delete_ref use a transaction refs.c: add an err argument to delete_ref_loose I will respin a v19 with these patches reordered. Thanks, ronine sahlberg On Wed, Jun 18, 2014 at 1:47 PM, Michael Haggerty wrote: > On 06/17/2014 05:53 PM, Ronnie Sahlberg wrote: >> 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. >> [...] > > I'm getting test failures starting with this commit: > >> Test Summary Report >> --- >> t5514-fetch-multiple.sh (Wstat: 256 Tests: 11 >> Failed: 3) >> Failed tests: 6, 8-9 >> Non-zero exit status: 1 >> t6050-replace.sh (Wstat: 256 Tests: 28 >> Failed: 1) >> Failed test: 15 >> Non-zero exit status: 1 >> t1400-update-ref.sh (Wstat: 256 Tests: 133 >> Failed: 4) >> Failed tests: 86-87, 130-131 >> Non-zero exit status: 1 >> t5540-http-push-webdav.sh(Wstat: 256 Tests: 19 >> Failed: 2) >> Failed tests: 8-9 >> Non-zero exit status: 1 >> t5505-remote.sh (Wstat: 256 Tests: 76 >> Failed: 5) >> Failed tests: 11, 45-48 >> Non-zero exit status: 1 >> t9903-bash-prompt.sh (Wstat: 256 Tests: 51 >> Failed: 1) >> Failed test: 19 >> Non-zero exit status: 1 >> t9300-fast-import.sh (Wstat: 256 Tests: 170 >> Failed: 1) >> Failed test: 71 >> Non-zero exit status: 1 >> t6030-bisect-porcelain.sh(Wstat: 256 Tests: 55 >> Failed: 47) >> Failed tests: 2-5, 7-11, 13-14, 16-30, 32-34, 36-37, 39-44 >> 46-55 >> Non-zero exit status: 1 >> t7512-status-help.sh (Wstat: 256 Tests: 35 >> Failed: 1) >> Failed test: 27 >> Non-zero exit status: 1 >> t5516-fetch-push.sh (Wstat: 256 Tests: 80 >> Failed: 3) >> Failed tests: 47-49 >> Non-zero exit status: 1 > > Let me know if you need more information. > > 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 v18 11/48] refs.c: make remove_empty_directories alwasy set errno to something sane
fixed Thanks On Wed, Jun 18, 2014 at 2:00 PM, Michael Haggerty wrote: > There is a typo in the commit log subject line: > > s/alwasy/always/ > > Michael > > On 06/17/2014 05:53 PM, Ronnie Sahlberg wrote: >> 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 >> --- >> 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) >> > > > -- > 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 v18 10/48] refs.c: verify_lock should set errno to something meaningful
fixed in 19
On Wed, Jun 18, 2014 at 1:38 PM, Michael Haggerty wrote:
> On 06/17/2014 05:53 PM, Ronnie Sahlberg wrote:
>> 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
>> ---
>> 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..af4fcdc 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 meaningfull.
>
> s/meaningfull/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_NODEREF 0x01
>> +/* 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);
>>
>
>
> --
> 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 v18 14/48] refs.c: log_ref_write should try to return meaningful errno
On Wed, Jun 18, 2014 at 2:08 PM, Michael Haggerty wrote: > On 06/17/2014 05:53 PM, Ronnie Sahlberg wrote: >> 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 >> --- >> refs.c | 29 - >> 1 file changed, 24 insertions(+), 5 deletions(-) >> >> diff --git a/refs.c b/refs.c >> index 211429d..1f2eb24 100644 >> --- a/refs.c >> +++ b/refs.c >> @@ -1979,6 +1979,7 @@ static int remove_empty_directories(const char *file) >> result = remove_dir_recursively(&path, REMOVE_DIR_EMPTY_ONLY); >> save_errno = errno; >> >> + errno = save_errno; >> strbuf_release(&path); >> errno = save_errno; > > This new line looks like an accident. Yepp. Too many rebases. Thanks. > >> [...] > > 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: [msysGit] Re: [PATCH 0/7] Second part of msysgit/unicode
Am 18.06.2014 19:33, schrieb Junio C Hamano: > In the meantime, are Windows folks happy with the four topics queued > on 'pu' so far? I would like to start moving them down to 'next' > and to 'master' soonish. > > They consist of these individual patches: > > $ git shortlog ^master \ > sk/mingw-dirent \ > sk/mingw-main \ > sk/mingw-uni-console \ > sk/mingw-unicode-spawn-args Topic sk/test-cmp-bin revealed a new breakage in t5000-tar-tree, specifically, the penultimate test "remote tar.gz is allowed by default". I have yet to find out what it is (I suspect a LF-CRLF conversion issue) and whether it is in connection with one of these topics. I haven't had a chance to test the topics in the field. In particular, I have a few files with Shift-JIS content (but ASCII file names), and I would like to see how well I fare with the unicode topics in this situation. -- Hannes -- 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 v3 04/14] refs.c: add a new update_type field to ref_update
Junio C Hamano writes:
> Ronnie Sahlberg writes:
>
>> Add a field that describes what type of update this refers to. For now
>> the only type is UPDATE_SHA1 but we will soon add more types.
>>
>> Signed-off-by: Ronnie Sahlberg
>> ---
>> refs.c | 25 +
>> 1 file changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/refs.c b/refs.c
>> index 4e3d4c3..4129de6 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -3374,6 +3374,10 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
>> return retval;
>> }
>>
>> +enum transaction_update_type {
>> + UPDATE_SHA1 = 0,
>
> indent with SP?
>
> Unlike an array initialisation, e.g.
>
> int foo[] = { 1,2,3,4,5, };
>
> some compilers we support complain if enum definition ends with a
> trailing comma.
I do recall we had fixes to drop the comma after the last element in
enum definition in the past, in response real compilation breakages
on some platforms. But there is a curious thing:
git grep -A 'enum ' master -- \*.c
tells me that builtin/clean.c would fail to compile for those folks.
Here is an off-topic "fix" that may no longer be needed.
builtin/clean.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/clean.c b/builtin/clean.c
index 9a91515..27701d2 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -48,7 +48,7 @@ enum color_clean {
CLEAN_COLOR_PROMPT = 2,
CLEAN_COLOR_HEADER = 3,
CLEAN_COLOR_HELP = 4,
- CLEAN_COLOR_ERROR = 5,
+ CLEAN_COLOR_ERROR = 5
};
#define MENU_OPTS_SINGLETON01
--
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 v18 14/48] refs.c: log_ref_write should try to return meaningful errno
On 06/17/2014 05:53 PM, Ronnie Sahlberg wrote: > 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 > --- > refs.c | 29 - > 1 file changed, 24 insertions(+), 5 deletions(-) > > diff --git a/refs.c b/refs.c > index 211429d..1f2eb24 100644 > --- a/refs.c > +++ b/refs.c > @@ -1979,6 +1979,7 @@ static int remove_empty_directories(const char *file) > result = remove_dir_recursively(&path, REMOVE_DIR_EMPTY_ONLY); > save_errno = errno; > > + errno = save_errno; > strbuf_release(&path); > errno = save_errno; This new line looks like an accident. > [...] 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 v18 11/48] refs.c: make remove_empty_directories alwasy set errno to something sane
There is a typo in the commit log subject line: s/alwasy/always/ Michael On 06/17/2014 05:53 PM, Ronnie Sahlberg wrote: > 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 > --- > 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) > -- 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 v18 16/48] refs.c: add an err argument to delete_ref_loose
On 06/17/2014 05:53 PM, Ronnie Sahlberg wrote: > 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. > [...] I'm getting test failures starting with this commit: > Test Summary Report > --- > t5514-fetch-multiple.sh (Wstat: 256 Tests: 11 > Failed: 3) > Failed tests: 6, 8-9 > Non-zero exit status: 1 > t6050-replace.sh (Wstat: 256 Tests: 28 > Failed: 1) > Failed test: 15 > Non-zero exit status: 1 > t1400-update-ref.sh (Wstat: 256 Tests: 133 > Failed: 4) > Failed tests: 86-87, 130-131 > Non-zero exit status: 1 > t5540-http-push-webdav.sh(Wstat: 256 Tests: 19 > Failed: 2) > Failed tests: 8-9 > Non-zero exit status: 1 > t5505-remote.sh (Wstat: 256 Tests: 76 > Failed: 5) > Failed tests: 11, 45-48 > Non-zero exit status: 1 > t9903-bash-prompt.sh (Wstat: 256 Tests: 51 > Failed: 1) > Failed test: 19 > Non-zero exit status: 1 > t9300-fast-import.sh (Wstat: 256 Tests: 170 > Failed: 1) > Failed test: 71 > Non-zero exit status: 1 > t6030-bisect-porcelain.sh(Wstat: 256 Tests: 55 > Failed: 47) > Failed tests: 2-5, 7-11, 13-14, 16-30, 32-34, 36-37, 39-44 > 46-55 > Non-zero exit status: 1 > t7512-status-help.sh (Wstat: 256 Tests: 35 > Failed: 1) > Failed test: 27 > Non-zero exit status: 1 > t5516-fetch-push.sh (Wstat: 256 Tests: 80 > Failed: 3) > Failed tests: 47-49 > Non-zero exit status: 1 Let me know if you need more information. 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 v18 10/48] refs.c: verify_lock should set errno to something meaningful
On 06/17/2014 05:53 PM, Ronnie Sahlberg wrote:
> 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
> ---
> 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..af4fcdc 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 meaningfull.
s/meaningfull/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_NODEREF 0x01
> +/* 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);
>
--
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 v3 04/14] refs.c: add a new update_type field to ref_update
Ronnie Sahlberg writes:
> Add a field that describes what type of update this refers to. For now
> the only type is UPDATE_SHA1 but we will soon add more types.
>
> Signed-off-by: Ronnie Sahlberg
> ---
> refs.c | 25 +
> 1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 4e3d4c3..4129de6 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3374,6 +3374,10 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
> return retval;
> }
>
> +enum transaction_update_type {
> + UPDATE_SHA1 = 0,
indent with SP?
Unlike an array initialisation, e.g.
int foo[] = { 1,2,3,4,5, };
some compilers we support complain if enum definition ends with a
trailing comma.
--
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 7/7] determine_author_info: stop leaking name/email
When we get the author name and email either from an
existing commit or from the "--author" option, we create a
copy of the strings. We cannot just free() these copies,
since the same pointers may also be pointing to getenv()
storage which we do not own.
Instead, let's treat these the same way as we do the date
buffer: keep a strbuf to be released, and point the bare
pointers at the strbuf.
Signed-off-by: Jeff King
---
builtin/commit.c | 20 +---
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 62abee0..72beb7f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -546,16 +546,20 @@ static void strbuf_add_pair(struct strbuf *buf, const
struct pointer_pair *p)
strbuf_add(buf, p->begin, p->end - p->begin);
}
-static char *xmemdupz_pair(const struct pointer_pair *p)
+static char *set_pair(struct strbuf *buf, struct pointer_pair *p)
{
- return xmemdupz(p->begin, p->end - p->begin);
+ strbuf_reset(buf);
+ strbuf_add_pair(buf, p);
+ return buf->buf;
}
static void determine_author_info(struct strbuf *author_ident)
{
char *name, *email, *date;
struct ident_split author;
- struct strbuf date_buf = STRBUF_INIT;
+ struct strbuf name_buf = STRBUF_INIT,
+ mail_buf = STRBUF_INIT,
+ date_buf = STRBUF_INIT;
name = getenv("GIT_AUTHOR_NAME");
email = getenv("GIT_AUTHOR_EMAIL");
@@ -572,8 +576,8 @@ static void determine_author_info(struct strbuf
*author_ident)
if (split_ident_line(&ident, a, len) < 0)
die(_("commit '%s' has malformed author line"),
author_message);
- name = xmemdupz_pair(&ident.name);
- email = xmemdupz_pair(&ident.mail);
+ name = set_pair(&name_buf, &ident.name);
+ email = set_pair(&mail_buf, &ident.mail);
if (ident.date.begin) {
strbuf_reset(&date_buf);
strbuf_addch(&date_buf, '@');
@@ -589,8 +593,8 @@ static void determine_author_info(struct strbuf
*author_ident)
if (split_ident_line(&ident, force_author,
strlen(force_author)) < 0)
die(_("malformed --author parameter"));
- name = xmemdupz_pair(&ident.name);
- email = xmemdupz_pair(&ident.mail);
+ name = set_pair(&name_buf, &ident.name);
+ email = set_pair(&mail_buf, &ident.mail);
}
if (force_date) {
@@ -608,6 +612,8 @@ static void determine_author_info(struct strbuf
*author_ident)
export_one("GIT_AUTHOR_DATE", author.date.begin, author.tz.end,
'@');
}
+ strbuf_release(&name_buf);
+ strbuf_release(&mail_buf);
strbuf_release(&date_buf);
}
--
2.0.0.566.gfe3e6b2
--
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 v18 07/48] lockfile.c: make lock_file return a meaningful errno on failurei
On 06/17/2014 05:53 PM, Ronnie Sahlberg wrote:
> 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
> ---
> 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;
> + }
Wouldn't it make sense for error() to save and restore errno instead of
scattering the save/restore code around everywhere? I saw the same type
of code about three commits later, too.
> [...]
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
[PATCH 6/7] determine_author_info: reuse parsing functions
Rather than parsing the header manually to find the "author"
field, and then parsing its sub-parts, let's use
find_commit_header and split_ident_line. This is shorter and
easier to read, and should do a more careful parsing job.
For example, the current parser could find the end-of-email
right-bracket across a newline (for a malformed commit), and
calculate a bogus gigantic length for the date (by using
"eol - rb").
As a bonus, this also plugs a memory leak when we pull the
date field from an existing commit (we still leak the name
and email buffers, which will be fixed in a later commit).
Signed-off-by: Jeff King
---
The large buffer comes from wrapping around the negative side of the
size_t space. In theory you could wrap far enough to get a buffer that
we can actually allocate (probably only on a 32-bit system), and then
we followup by copying "len" random bytes into it. I doubt an attacker
could get that data out of the program, though, as we then run it
through fmt_ident, which should complain if it's full of garbage.
builtin/commit.c | 61 +---
1 file changed, 32 insertions(+), 29 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index bf770cf..62abee0 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -541,6 +541,16 @@ static int parse_force_date(const char *in, struct strbuf
*out)
return 0;
}
+static void strbuf_add_pair(struct strbuf *buf, const struct pointer_pair *p)
+{
+ strbuf_add(buf, p->begin, p->end - p->begin);
+}
+
+static char *xmemdupz_pair(const struct pointer_pair *p)
+{
+ return xmemdupz(p->begin, p->end - p->begin);
+}
+
static void determine_author_info(struct strbuf *author_ident)
{
char *name, *email, *date;
@@ -552,42 +562,35 @@ static void determine_author_info(struct strbuf
*author_ident)
date = getenv("GIT_AUTHOR_DATE");
if (author_message) {
- const char *a, *lb, *rb, *eol;
- size_t len;
+ struct ident_split ident;
+ unsigned long len;
+ const char *a;
- a = strstr(author_message_buffer, "\nauthor ");
+ a = find_commit_header(author_message_buffer, "author", &len);
if (!a)
- die(_("invalid commit: %s"), author_message);
-
- lb = strchrnul(a + strlen("\nauthor "), '<');
- rb = strchrnul(lb, '>');
- eol = strchrnul(rb, '\n');
- if (!*lb || !*rb || !*eol)
- die(_("invalid commit: %s"), author_message);
-
- if (lb == a + strlen("\nauthor "))
- /* \nauthor */
- name = xcalloc(1, 1);
- else
- name = xmemdupz(a + strlen("\nauthor "),
- (lb - strlen(" ") -
-(a + strlen("\nauthor ";
- email = xmemdupz(lb + strlen("<"), rb - (lb + strlen("<")));
- len = eol - (rb + strlen("> "));
- date = xmalloc(len + 2);
- *date = '@';
- memcpy(date + 1, rb + strlen("> "), len);
- date[len + 1] = '\0';
+ die(_("commit '%s' lacks author header"),
author_message);
+ if (split_ident_line(&ident, a, len) < 0)
+ die(_("commit '%s' has malformed author line"),
author_message);
+
+ name = xmemdupz_pair(&ident.name);
+ email = xmemdupz_pair(&ident.mail);
+ if (ident.date.begin) {
+ strbuf_reset(&date_buf);
+ strbuf_addch(&date_buf, '@');
+ strbuf_add_pair(&date_buf, &ident.date);
+ strbuf_addch(&date_buf, ' ');
+ strbuf_add_pair(&date_buf, &ident.tz);
+ date = date_buf.buf;
+ }
}
if (force_author) {
- const char *lb = strstr(force_author, " <");
- const char *rb = strchr(force_author, '>');
+ struct ident_split ident;
- if (!lb || !rb)
+ if (split_ident_line(&ident, force_author,
strlen(force_author)) < 0)
die(_("malformed --author parameter"));
- name = xstrndup(force_author, lb - force_author);
- email = xstrndup(lb + 2, rb - (lb + 2));
+ name = xmemdupz_pair(&ident.name);
+ email = xmemdupz_pair(&ident.mail);
}
if (force_date) {
--
2.0.0.566.gfe3e6b2
--
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 v3 01/14] refs.c make ref_transaction_create a wrapper to ref_transaction_update
Ronnie Sahlberg writes:
> Signed-off-by: Ronnie Sahlberg
> ---
> refs.c | 13 ++---
> refs.h | 7 ---
> 2 files changed, 6 insertions(+), 14 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index dfbf003..a9f91ab 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3487,23 +3487,14 @@ int ref_transaction_create(struct ref_transaction
> *transaction,
> int flags, const char *msg,
> struct strbuf *err)
> {
> - 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");
>
> - update = add_update(transaction, refname);
> -
> - hashcpy(update->new_sha1, new_sha1);
> - hashclr(update->old_sha1);
> - update->flags = flags;
> - update->have_old = 1;
> - if (msg)
> - update->msg = xstrdup(msg);
> - return 0;
> + return ref_transaction_update(transaction, refname, new_sha1,
> + null_sha1, flags, 1, msg, err);
> }
Hmmm.
This probably logically belongs to the end of the previous series
and not necessarily tied to reflog transaction, no?
At the beginning of ref_transaction_update() there also is the same
guard on transaction->state, and having both feels somewhat iffy.
Of course it will give a wrong BUG message if we removed the check
from this function, so perhaps the code is OK as-is.
> int ref_transaction_delete(struct ref_transaction *transaction,
> diff --git a/refs.h b/refs.h
> index db463d0..495740d 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -283,9 +283,10 @@ struct ref_transaction *ref_transaction_begin(struct
> strbuf *err);
> /*
> * Add a reference update to transaction. new_sha1 is the value that
> * the reference should have after the update, or zeros if it should
> - * 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.
> + * be deleted. If have_old is true and old_sha is not the null_sha1
> + * then the previous value of the ref must match or the update will fail.
> + * If have_old is true and old_sha1 is the null_sha1 then the ref must not
> + * already exist and a new ref will be created with new_sha1.
> * 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.
--
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 5/7] use strbufs in date functions
Many of the date functions write into fixed-size buffers.
This is a minor pain, as we have to take special
precautions, and frequently end up copying the result into a
strbuf or heap-allocated buffer anyway (for which we
sometimes use strcpy!).
Let's instead teach parse_date, datestamp, etc to write to a
strbuf. The obvious downside is that we might need to
perform a heap allocation where we otherwise would not need
to. However, it turns out that the only two new allocations
required are:
1. In test-date.c, where we don't care about efficiency.
2. In determine_author_info, which is not performance
critical (and where the use of a strbuf will help later
refactoring).
Signed-off-by: Jeff King
---
I don't think the strcpys are a problem in practice, because we're
typically writing fixed-size output generated by parse_date. But I
didn't analyze it too deeply, so you might be able to cause shenanigans
if you can impact GIT_AUTHOR_DATE or something.
builtin/commit.c | 20 ++--
cache.h | 4 ++--
date.c | 13 +++--
fast-import.c| 20 +---
ident.c | 26 +++---
test-date.c | 10 ++
6 files changed, 45 insertions(+), 48 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 047cc76..bf770cf 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -526,19 +526,16 @@ static int sane_ident_split(struct ident_split *person)
return 1;
}
-static int parse_force_date(const char *in, char *out, int len)
+static int parse_force_date(const char *in, struct strbuf *out)
{
- if (len < 1)
- return -1;
- *out++ = '@';
- len--;
+ strbuf_addch(out, '@');
- if (parse_date(in, out, len) < 0) {
+ if (parse_date(in, out) < 0) {
int errors = 0;
unsigned long t = approxidate_careful(in, &errors);
if (errors)
return -1;
- snprintf(out, len, "%lu", t);
+ strbuf_addf(out, "%lu", t);
}
return 0;
@@ -548,7 +545,7 @@ static void determine_author_info(struct strbuf
*author_ident)
{
char *name, *email, *date;
struct ident_split author;
- char date_buf[64];
+ struct strbuf date_buf = STRBUF_INIT;
name = getenv("GIT_AUTHOR_NAME");
email = getenv("GIT_AUTHOR_EMAIL");
@@ -594,9 +591,10 @@ static void determine_author_info(struct strbuf
*author_ident)
}
if (force_date) {
- if (parse_force_date(force_date, date_buf, sizeof(date_buf)))
+ strbuf_reset(&date_buf);
+ if (parse_force_date(force_date, &date_buf))
die(_("invalid date format: %s"), force_date);
- date = date_buf;
+ date = date_buf.buf;
}
strbuf_addstr(author_ident, fmt_ident(name, email, date, IDENT_STRICT));
@@ -606,6 +604,8 @@ static void determine_author_info(struct strbuf
*author_ident)
export_one("GIT_AUTHOR_EMAIL", author.mail.begin,
author.mail.end, 0);
export_one("GIT_AUTHOR_DATE", author.date.begin, author.tz.end,
'@');
}
+
+ strbuf_release(&date_buf);
}
static void split_ident_or_die(struct ident_split *id, const struct strbuf
*buf)
diff --git a/cache.h b/cache.h
index 5255661..c7740a8 100644
--- a/cache.h
+++ b/cache.h
@@ -1023,10 +1023,10 @@ enum date_mode {
const char *show_date(unsigned long time, int timezone, enum date_mode mode);
void show_date_relative(unsigned long time, int tz, const struct timeval *now,
struct strbuf *timebuf);
-int parse_date(const char *date, char *buf, int bufsize);
+int parse_date(const char *date, struct strbuf *out);
int parse_date_basic(const char *date, unsigned long *timestamp, int *offset);
int parse_expiry_date(const char *date, unsigned long *timestamp);
-void datestamp(char *buf, int bufsize);
+void datestamp(struct strbuf *out);
#define approxidate(s) approxidate_careful((s), NULL)
unsigned long approxidate_careful(const char *, int *);
unsigned long approxidate_relative(const char *date, const struct timeval
*now);
diff --git a/date.c b/date.c
index 782de95..2c33468 100644
--- a/date.c
+++ b/date.c
@@ -605,7 +605,7 @@ static int match_tz(const char *date, int *offp)
return end - date;
}
-static int date_string(unsigned long date, int offset, char *buf, int len)
+static void date_string(unsigned long date, int offset, struct strbuf *buf)
{
int sign = '+';
@@ -613,7 +613,7 @@ static int date_string(unsigned long date, int offset, char
*buf, int len)
offset = -offset;
sign = '-';
}
- return snprintf(buf, len, "%lu %c%02d%02d", date, sign, offset / 60,
offset % 60);
+ strbuf_addf(buf, "%lu %c%02d%02d", date, sign, offset / 60, offset %
60);
}
/*
@@ -735,13 +735,14 @@ int parse_expiry_date(c
[PATCH 4/7] ident_split: store begin/end pairs on their own struct
When we parse an ident line, we end up with several fields,
each with a begin/end pointer into the buffer, like:
const char *name_begin;
const char *name_end;
There is nothing except the field names to indicate that
they are paired. This makes it annoying to write helper
functions for dealing with the sub-fields, as you have to
pass both sides. Instead, let's move them into a single
struct "name", with fields "begin" and "end". This will be
stored identically, but can be passed as a unit.
We have to do a mechanical update of "s/_/./" at each point
of use, but other than that, the fields should behave
identically.
Signed-off-by: Jeff King
---
Suggestions welcome on the name "pointer_pair".
While writing this series, I also noticed that it would be more
convenient to have a pointer/len combination rather than two pointers.
You can convert between them, of course, but I found I was always
converting the other way.
I left it this way because it makes the mass-update mechanical (and
because now that I can pass the pair as a unit, I don't have to write
the same "ident->name_begin, ident->name_end - ident->name_begin" pair
over and over).
builtin/blame.c | 16 +++---
builtin/check-mailmap.c | 8 +++
builtin/commit.c| 26 +++---
builtin/shortlog.c | 8 +++
cache.h | 17 ---
commit.c| 6 +++---
ident.c | 57 +++--
log-tree.c | 2 +-
pretty.c| 36 +++
revision.c | 12 +--
10 files changed, 93 insertions(+), 95 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 53f43ab..6e6dddb 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1599,19 +1599,19 @@ static void get_ac_line(const char *inbuf, const char
*what,
return;
}
- namelen = ident.name_end - ident.name_begin;
- namebuf = ident.name_begin;
+ namelen = ident.name.end - ident.name.begin;
+ namebuf = ident.name.begin;
- maillen = ident.mail_end - ident.mail_begin;
- mailbuf = ident.mail_begin;
+ maillen = ident.mail.end - ident.mail.begin;
+ mailbuf = ident.mail.begin;
- if (ident.date_begin && ident.date_end)
- *time = strtoul(ident.date_begin, NULL, 10);
+ if (ident.date.begin && ident.date.end)
+ *time = strtoul(ident.date.begin, NULL, 10);
else
*time = 0;
- if (ident.tz_begin && ident.tz_end)
- strbuf_add(tz, ident.tz_begin, ident.tz_end - ident.tz_begin);
+ if (ident.tz.begin && ident.tz.end)
+ strbuf_add(tz, ident.tz.begin, ident.tz.end - ident.tz.begin);
else
strbuf_addstr(tz, "(unknown)");
diff --git a/builtin/check-mailmap.c b/builtin/check-mailmap.c
index 8f4d809..65dcbc6 100644
--- a/builtin/check-mailmap.c
+++ b/builtin/check-mailmap.c
@@ -23,10 +23,10 @@ static void check_mailmap(struct string_list *mailmap,
const char *contact)
if (split_ident_line(&ident, contact, strlen(contact)))
die(_("unable to parse contact: %s"), contact);
- name = ident.name_begin;
- namelen = ident.name_end - ident.name_begin;
- mail = ident.mail_begin;
- maillen = ident.mail_end - ident.mail_begin;
+ name = ident.name.begin;
+ namelen = ident.name.end - ident.name.begin;
+ mail = ident.mail.begin;
+ maillen = ident.mail.end - ident.mail.begin;
map_user(mailmap, &mail, &maillen, &name, &namelen);
diff --git a/builtin/commit.c b/builtin/commit.c
index 84cec9a..047cc76 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -514,14 +514,14 @@ static void export_one(const char *var, const char *s,
const char *e, int hack)
static int sane_ident_split(struct ident_split *person)
{
- if (!person->name_begin || !person->name_end ||
- person->name_begin == person->name_end)
+ if (!person->name.begin || !person->name.end ||
+ person->name.begin == person->name.end)
return 0; /* no human readable name */
- if (!person->mail_begin || !person->mail_end ||
- person->mail_begin == person->mail_end)
+ if (!person->mail.begin || !person->mail.end ||
+ person->mail.begin == person->mail.end)
return 0; /* no usable mail */
- if (!person->date_begin || !person->date_end ||
- !person->tz_begin || !person->tz_end)
+ if (!person->date.begin || !person->date.end ||
+ !person->tz.begin || !person->tz.end)
return 0;
return 1;
}
@@ -602,9 +602,9 @@ static void determine_author_info(struct strbuf
*author_ident)
strbuf_addstr(author_ident, fmt_ident(name, email, date, IDENT_STRICT));
if (!split_ident_line(&author, author_ident->buf, author_ident->len) &&
sane_ide
Re: [PATCH v18 07/48] lockfile.c: make lock_file return a meaningful errno on failurei
There's a typo in the subject line of this commit.
Michael
On 06/17/2014 05:53 PM, Ronnie Sahlberg wrote:
> 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
> ---
> 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);
>
>
--
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
[PATCH 3/7] record_author_info: use find_commit_header
This saves us some manual parsing and makes the code more
readable.
Signed-off-by: Jeff King
---
I suspect there are other sites which could use this helper, too; I
didn't do an exhaustive search.
commit.c | 23 ---
1 file changed, 8 insertions(+), 15 deletions(-)
diff --git a/commit.c b/commit.c
index 0c40cfa..c33431c 100644
--- a/commit.c
+++ b/commit.c
@@ -606,26 +606,19 @@ define_commit_slab(author_date_slab, unsigned long);
static void record_author_date(struct author_date_slab *author_date,
struct commit *commit)
{
- const char *buf, *line_end, *ident_line;
const char *buffer = get_commit_buffer(commit, NULL);
struct ident_split ident;
+ const char *ident_line;
+ size_t ident_len;
char *date_end;
unsigned long date;
- for (buf = buffer; buf; buf = line_end + 1) {
- line_end = strchrnul(buf, '\n');
- ident_line = skip_prefix(buf, "author ");
- if (!ident_line) {
- if (!line_end[0] || line_end[1] == '\n')
- goto fail_exit; /* end of header */
- continue;
- }
- if (split_ident_line(&ident,
-ident_line, line_end - ident_line) ||
- !ident.date_begin || !ident.date_end)
- goto fail_exit; /* malformed "author" line */
- break;
- }
+ ident_line = find_commit_header(buffer, "author", &ident_len);
+ if (!ident_line)
+ goto fail_exit; /* no author line */
+ if (split_ident_line(&ident, ident_line, ident_len) ||
+ !ident.date_begin || !ident.date_end)
+ goto fail_exit; /* malformed "author" line */
date = strtoul(ident.date_begin, &date_end, 10);
if (date_end != ident.date_end)
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/7] record_author_info: fix memory leak on malformed commit
If we hit the end-of-header without finding an "author"
line, we just return from the function. We should jump to
the fail_exit path to clean up the buffer that we may have
allocated.
Signed-off-by: Jeff King
---
commit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/commit.c b/commit.c
index d04b525..0c40cfa 100644
--- a/commit.c
+++ b/commit.c
@@ -617,7 +617,7 @@ static void record_author_date(struct author_date_slab
*author_date,
ident_line = skip_prefix(buf, "author ");
if (!ident_line) {
if (!line_end[0] || line_end[1] == '\n')
- return; /* end of header */
+ goto fail_exit; /* end of header */
continue;
}
if (split_ident_line(&ident,
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/7] commit: provide a function to find a header in a buffer
Usually when we parse a commit, we read it line by line and
handle each header in a single pass (e.g., in parse_commit
and parse_commit_header). Sometimes, however, we only care
about extracting a single header. Code in this situation is
stuck doing an ad-hoc parse of the commit buffer.
Let's provide a reusable function to locate a header within
the commit. The code is modeled after pretty.c's
get_header, which is used to extract the encoding.
Since some callers may not have the "struct commit" to go
along with the buffer, we drop that parameter. The only
thing lost is a warning for truncated commits, but that's
OK. This shouldn't happen in practice, and even if it does,
there's no particular reason that this function needs to
complain about it. It either finds the header it was asked
for, or it doesn't (and in the latter case, the caller can
complain).
Signed-off-by: Jeff King
---
As noted in the comments below, I punted on extracting
multi-line headers like mergetag, because this function only
returns a pointer. It might make sense to wrap it with a
function to pull out a copy of the header, with continuation
lines connected to each other (that's almost what the static
get_header does, but I didn't make it public exactly because
it doesn't handle continuation lines).
That might be a more natural interface than
read_commit_extra_header_lines, which pulls out all headers
except ones that are specifically excluded. I haven't looked
closely enough. But in either case, that could easily come
on top of this.
commit.c | 23 +++
commit.h | 11 +++
pretty.c | 33 ++---
3 files changed, 40 insertions(+), 27 deletions(-)
diff --git a/commit.c b/commit.c
index 11106fb..d04b525 100644
--- a/commit.c
+++ b/commit.c
@@ -1652,3 +1652,26 @@ void print_commit_list(struct commit_list *list,
printf(format, sha1_to_hex(list->item->object.sha1));
}
}
+
+const char *find_commit_header(const char *msg, const char *key, size_t
*out_len)
+{
+ int key_len = strlen(key);
+ const char *line = msg;
+
+ while (line) {
+ const char *eol = strchrnul(line, '\n'), *next;
+
+ if (line == eol)
+ return NULL;
+ next = *eol ? eol + 1 : NULL;
+
+ if (eol - line > key_len &&
+ !strncmp(line, key, key_len) &&
+ line[key_len] == ' ') {
+ *out_len = eol - line - key_len - 1;
+ return line + key_len + 1;
+ }
+ line = next;
+ }
+ return NULL;
+}
diff --git a/commit.h b/commit.h
index 61559a9..7c766e9 100644
--- a/commit.h
+++ b/commit.h
@@ -312,6 +312,17 @@ extern struct commit_extra_header
*read_commit_extra_headers(struct commit *, co
extern void free_commit_extra_headers(struct commit_extra_header *extra);
+/*
+ * Search the commit object contents given by "msg" for the header "key".
+ * Returns a pointer to the start of the header contents, or NULL. The length
+ * of the header, up to the first newline, is returned via out_len.
+ *
+ * Note that some headers (like mergetag) may be multi-line. It is the caller's
+ * responsibility to parse further in this case!
+ */
+extern const char *find_commit_header(const char *msg, const char *key,
+ size_t *out_len);
+
struct merge_remote_desc {
struct object *obj; /* the named object, could be a tag */
const char *name;
diff --git a/pretty.c b/pretty.c
index cc5b45d..6081750 100644
--- a/pretty.c
+++ b/pretty.c
@@ -548,31 +548,11 @@ static void add_merge_info(const struct
pretty_print_context *pp,
strbuf_addch(sb, '\n');
}
-static char *get_header(const struct commit *commit, const char *msg,
- const char *key)
+static char *get_header(const char *msg, const char *key)
{
- int key_len = strlen(key);
- const char *line = msg;
-
- while (line) {
- const char *eol = strchrnul(line, '\n'), *next;
-
- if (line == eol)
- return NULL;
- if (!*eol) {
- warning("malformed commit (header is missing newline):
%s",
- sha1_to_hex(commit->object.sha1));
- next = NULL;
- } else
- next = eol + 1;
- if (eol - line > key_len &&
- !strncmp(line, key, key_len) &&
- line[key_len] == ' ') {
- return xmemdupz(line + key_len + 1, eol - line -
key_len - 1);
- }
- line = next;
- }
- return NULL;
+ size_t len;
+ const char *v = find_commit_header(msg, key, &len);
+ return v ? xmemdupz(v, len) : NULL;
}
static char *replace_encoding_header(char *buf, const char *encoding)
@@ -618,11 +598,10 @@ const char *logmsg_re
[PATCH 0/7] cleaning up determine_author_info
This is another function I ran across in today's cleanups. The memory leak in it has bugged me for a while (even though it's really not a big deal in practice). So this is mostly minor cleanups, but I did find a bug in the commit parser. [1/7]: commit: provide a function to find a header in a buffer [2/7]: record_author_info: fix memory leak on malformed commit [3/7]: record_author_info: use find_commit_header [4/7]: ident_split: store begin/end pairs on their own struct [5/7]: use strbufs in date functions [6/7]: determine_author_info: reuse parsing functions [7/7]: determine_author_info: stop leaking name/email I built it on top of my commit-slab topic, as otherwise you get some textual conflicts in determine_author_info. But I notice that Junio's jk/commit-buffer-length is based on an older master; applying there produces some other minor textual conflicts. I can build it on whatever is convenient and handle the conflicts myself. But if jk/commit-buffer-length is set to graduate soon (as it is marked in WC), I can just hold onto this until then. -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
Importing history to show up in a blame
The only way I have found so far to do this is to merge the branch on to a tmp branch and then back! The only other way I found seems real bad: http://stackoverflow.com/questions/16473363/tell-git-blame-to-use-imported-histo ry And the way below does not work if there are edits already on master (that is non-identical files). Is there a better way? -Jason jpyeron@black /tmp/foo $ git --version git version 1.8.4.21.g992c386 jpyeron@black /tmp/foo $ git init history Initialized empty Git repository in /tmp/foo/history/.git/ jpyeron@black /tmp/foo $ cd history jpyeron@black /tmp/foo/history $ #make source.txt and commit ... jpyeron@black /tmp/foo/history $ git checkout --orphan HISTORICAL Switched to a new branch 'HISTORICAL' jpyeron@black /tmp/foo/history $ # import each historical version and commit ... jpyeron@black /tmp/foo/history $ git log --oneline --graph && git blame source.txt && sha1sum.exe source.txt * 2889460 v6 - latest * 62e4a90 v5 * bfdf128 v4 * 416d32a v3 * 241a7dc v2 * 7ef41ad v1 ^7ef41ad (Jason Pyeron 2014-06-18 13:47:57 -0400 1) 1 the 241a7dc5 (Jason Pyeron 2014-06-18 13:48:53 -0400 2) 2 quick 62e4a90e (Jason Pyeron 2014-06-18 13:50:44 -0400 3) 3 brown bfdf1285 (Jason Pyeron 2014-06-18 13:49:55 -0400 4) 4 fox 416d32a4 (Jason Pyeron 2014-06-18 13:49:34 -0400 5) 5 jumped 416d32a4 (Jason Pyeron 2014-06-18 13:49:34 -0400 6) 6 over 416d32a4 (Jason Pyeron 2014-06-18 13:49:34 -0400 7) 7 the 416d32a4 (Jason Pyeron 2014-06-18 13:49:34 -0400 8) 8 lazy 28894602 (Jason Pyeron 2014-06-18 13:51:03 -0400 9) 9 dogs cd49f005ab64dac61b2d420a7903fcd02b5f373f *source.txt jpyeron@black /tmp/foo/history $ git checkout master Switched to branch 'master' jpyeron@black /tmp/foo/history $ git log --oneline --graph && git blame source.txt && sha1sum.exe source.txt * f25b132 import of latest source ^f25b132 (Jason Pyeron 2014-06-18 13:45:56 -0400 1) 1 the ^f25b132 (Jason Pyeron 2014-06-18 13:45:56 -0400 2) 2 quick ^f25b132 (Jason Pyeron 2014-06-18 13:45:56 -0400 3) 3 brown ^f25b132 (Jason Pyeron 2014-06-18 13:45:56 -0400 4) 4 fox ^f25b132 (Jason Pyeron 2014-06-18 13:45:56 -0400 5) 5 jumped ^f25b132 (Jason Pyeron 2014-06-18 13:45:56 -0400 6) 6 over ^f25b132 (Jason Pyeron 2014-06-18 13:45:56 -0400 7) 7 the ^f25b132 (Jason Pyeron 2014-06-18 13:45:56 -0400 8) 8 lazy ^f25b132 (Jason Pyeron 2014-06-18 13:45:56 -0400 9) 9 dogs cd49f005ab64dac61b2d420a7903fcd02b5f373f *source.txt jpyeron@black /tmp/foo/history $ git checkout HISTORICAL Switched to branch 'HISTORICAL' jpyeron@black /tmp/foo/history $ git branch historymerge jpyeron@black /tmp/foo/history $ git checkout historymerge Switched to branch 'historymerge' jpyeron@black /tmp/foo/history $ git merge master Merge made by the 'recursive' strategy. jpyeron@black /tmp/foo/history $ git checkout master Switched to branch 'master' jpyeron@black /tmp/foo/history $ git merge historymerge Updating f25b132..5a9408a Fast-forward jpyeron@black /tmp/foo/history $ git branch -d historymerge Deleted branch historymerge (was 5a9408a). jpyeron@black /tmp/foo/history $ git log --oneline --graph && git blame source.txt && sha1sum.exe source.txt * 5a9408a Merge branch 'master' into historymerge |\ | * f25b132 import of latest source * 2889460 v6 - latest * 62e4a90 v5 * bfdf128 v4 * 416d32a v3 * 241a7dc v2 * 7ef41ad v1 ^7ef41ad (Jason Pyeron 2014-06-18 13:47:57 -0400 1) 1 the 241a7dc5 (Jason Pyeron 2014-06-18 13:48:53 -0400 2) 2 quick 62e4a90e (Jason Pyeron 2014-06-18 13:50:44 -0400 3) 3 brown bfdf1285 (Jason Pyeron 2014-06-18 13:49:55 -0400 4) 4 fox 416d32a4 (Jason Pyeron 2014-06-18 13:49:34 -0400 5) 5 jumped 416d32a4 (Jason Pyeron 2014-06-18 13:49:34 -0400 6) 6 over 416d32a4 (Jason Pyeron 2014-06-18 13:49:34 -0400 7) 7 the 416d32a4 (Jason Pyeron 2014-06-18 13:49:34 -0400 8) 8 lazy 28894602 (Jason Pyeron 2014-06-18 13:51:03 -0400 9) 9 dogs cd49f005ab64dac61b2d420a7903fcd02b5f373f *source.txt -- -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- - - - Jason Pyeron PD Inc. http://www.pdinc.us - - Principal Consultant 10 West 24th Street #100- - +1 (443) 269-1555 x333Baltimore, Maryland 21218 - - - -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- This message is copyright PD Inc, subject to license 20080407P00. history.bundle Description: Binary data
Re: [PATCH] alloc.c: remove alloc_raw_commit_node() function
On Wed, Jun 18, 2014 at 08:52:46PM +0100, Ramsay Jones wrote:
> In order to encapsulate the setting of the unique commit index, commit
> 969eba63 ("commit: push commit_index update into alloc_commit_node",
> 10-06-2014) introduced a (logically private) intermediary allocator
> function. However, this function (alloc_raw_commit_node()) was declared
> as a public function, which undermines its entire purpose.
>
> Remove the alloc_raw_commit_node() function and inline its code into
> the (public) alloc_commit_node() function.
>
> Noticed by sparse ("symbol 'alloc_raw_commit_node' was not declared.
> Should it be static?").
>
> Signed-off-by: Ramsay Jones
> ---
>
> Hi Jeff,
>
> I noticed this while it was still in 'pu', but got distracted and
> didn't send this in time ... sorry about that! :(
Yeah, I noticed it while writing the patch but decided it wasn't worth
the trouble to deal with (since after all, it's not advertised to any
callers, the very thing that sparse is complaining about. :) ).
I don't mind fixing it, though I really don't like repeating the
contents of DEFINE_ALLOCATOR. I know it hasn't changed in a while, but
it just feels wrong.
> My first attempt at fixing this involved changing the DEFINE_ALLOCATOR
> macro to include a 'scope' parameter so that I could declare the
> raw_commit allocator 'static'. Unfortunately, I could not pass the
> extern keyword as the scope parameter to all the other allocators,
> because that made sparse even more upset - you can't use extern on
> a function _definition_. That meant passing an empty argument (or a
> comment token) to the scope parameter. This worked for gcc 4.8.2 and
> clang 3.4, but I was a little concerned about portability.
Yeah, passing an empty argument was my first thought, but I don't know
offhand if there are portability concerns.
You could also have DEFINE_ALLOCATOR just fill in the body, and do:
struct blob *alloc_blob_node(void)
{
DEFINE_ALLOCATOR(struct blob);
}
or similar.
-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
[PATCH 1/2] strbuf: add xstrdup_fmt helper
You can use a strbuf to build up a string from parts, and
then detach it. In the general case, you might use multiple
strbuf_add* functions to do the building. However, in many
cases, a single strbuf_addf is sufficient, and we end up
with:
struct strbuf buf = STRBUF_INIT;
...
strbuf_addf(&buf, fmt, some, args);
str = strbuf_detach(&buf, NULL);
We can make this much more readable (and avoid introducing
an extra variable, which can clutter the code) by
introducing a convenience function:
str = xstrdup_fmt(fmt, some, args);
Signed-off-by: Jeff King
---
I'm open to suggestions on the name. This really is the same thing
conceptually as the GNU asprintf(), but the interface is different (that
function takes a pointer-to-pointer as an out-parameter, and returns the
number of characters return).
strbuf.c | 19 +++
strbuf.h | 9 +
2 files changed, 28 insertions(+)
diff --git a/strbuf.c b/strbuf.c
index ac62982..6674d74 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -600,3 +600,22 @@ char *xstrdup_tolower(const char *string)
result[i] = '\0';
return result;
}
+
+char *xstrdup_vfmt(const char *fmt, va_list ap)
+{
+ struct strbuf buf = STRBUF_INIT;
+ strbuf_vaddf(&buf, fmt, ap);
+ return strbuf_detach(&buf, NULL);
+}
+
+char *xstrdup_fmt(const char *fmt, ...)
+{
+ va_list ap;
+ char *ret;
+
+ va_start(ap, fmt);
+ ret = xstrdup_vfmt(fmt, ap);
+ va_end(ap);
+
+ return ret;
+}
diff --git a/strbuf.h b/strbuf.h
index e9ad03e..61818f9 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -187,4 +187,13 @@ extern int fprintf_ln(FILE *fp, const char *fmt, ...);
char *xstrdup_tolower(const char *);
+/*
+ * Create a newly allocated string using printf format. You can do this easily
+ * with a strbuf, but this provides a shortcut to save a few lines.
+ */
+__attribute__((format (printf, 1, 0)))
+char *xstrdup_vfmt(const char *fmt, va_list ap);
+__attribute__((format (printf, 1, 2)))
+char *xstrdup_fmt(const char *fmt, ...);
+
#endif /* STRBUF_H */
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] use xstrdup_fmt in favor of manual size calculations
In many parts of the code, we do an ugly and error-prone
malloc like:
const char *fmt = "something %s";
buf = xmalloc(strlen(foo) + 10 + 1);
sprintf(buf, fmt, foo);
This makes the code brittle, and if we ever get the
allocation wrong, is a potential heap overflow. Let's
instead favor xstrdup_fmt, which handles the allocation
automatically, and makes the code shorter and more readable.
Signed-off-by: Jeff King
---
remote.c | 6 +-
unpack-trees.c | 17 ++---
2 files changed, 7 insertions(+), 16 deletions(-)
diff --git a/remote.c b/remote.c
index 0e9459c..792dcee 100644
--- a/remote.c
+++ b/remote.c
@@ -170,7 +170,6 @@ static struct branch *make_branch(const char *name, int len)
{
struct branch *ret;
int i;
- char *refname;
for (i = 0; i < branches_nr; i++) {
if (len ? (!strncmp(name, branches[i]->name, len) &&
@@ -186,10 +185,7 @@ static struct branch *make_branch(const char *name, int
len)
ret->name = xstrndup(name, len);
else
ret->name = xstrdup(name);
- refname = xmalloc(strlen(name) + strlen("refs/heads/") + 1);
- strcpy(refname, "refs/heads/");
- strcpy(refname + strlen("refs/heads/"), ret->name);
- ret->refname = refname;
+ ret->refname = xstrdup_fmt("refs/heads/%s", ret->name);
return ret;
}
diff --git a/unpack-trees.c b/unpack-trees.c
index 97fc995..dd1e06e 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -56,17 +56,15 @@ void setup_unpack_trees_porcelain(struct
unpack_trees_options *opts,
int i;
const char **msgs = opts->msgs;
const char *msg;
- char *tmp;
const char *cmd2 = strcmp(cmd, "checkout") ? cmd : "switch branches";
+
if (advice_commit_before_merge)
msg = "Your local changes to the following files would be
overwritten by %s:\n%%s"
"Please, commit your changes or stash them before you
can %s.";
else
msg = "Your local changes to the following files would be
overwritten by %s:\n%%s";
- tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen(cmd2) - 2);
- sprintf(tmp, msg, cmd, cmd2);
- msgs[ERROR_WOULD_OVERWRITE] = tmp;
- msgs[ERROR_NOT_UPTODATE_FILE] = tmp;
+ msgs[ERROR_WOULD_OVERWRITE] = msgs[ERROR_NOT_UPTODATE_FILE] =
+ xstrdup_fmt(msg, cmd, cmd2);
msgs[ERROR_NOT_UPTODATE_DIR] =
"Updating the following directories would lose untracked files
in it:\n%s";
@@ -76,12 +74,9 @@ void setup_unpack_trees_porcelain(struct
unpack_trees_options *opts,
"Please move or remove them before you can %s.";
else
msg = "The following untracked working tree files would be %s
by %s:\n%%s";
- tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen("removed") +
strlen(cmd2) - 4);
- sprintf(tmp, msg, "removed", cmd, cmd2);
- msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] = tmp;
- tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen("overwritten") +
strlen(cmd2) - 4);
- sprintf(tmp, msg, "overwritten", cmd, cmd2);
- msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = tmp;
+
+ msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] = xstrdup_fmt(msg, "removed",
cmd, cmd2);
+ msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = xstrdup_fmt(msg,
"overwritten", cmd, cmd2);
/*
* Special case: ERROR_BIND_OVERLAP refers to a pair of paths, we
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] dropping manual malloc calculations
While working on the skip_prefix series, I ended up grepping for:
+ strlen("
to find spots in need of skip_prefix. Of course, it turns up many other
nasty ad-hoc calculations. Here's a short series that addresses a few.
There are many more, but hopefully the first patch provides a tool that
can help us in the future.
[1/2]: strbuf: add xstrdup_fmt helper
[2/2]: use xstrdup_fmt in favor of manual size calculations
-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
[PATCH 16/16] use skip_prefix to avoid repeated calculations
In some cases, we use starts_with to check for a prefix, and
then use an already-calculated prefix length to advance a
pointer past the prefix. There are no magic numbers or
duplicated strings here, but we can still make the code
simpler and more obvious by using skip_prefix.
Signed-off-by: Jeff King
---
help.c | 11 +--
http.c | 3 +--
2 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/help.c b/help.c
index 79e8007..f31f29a 100644
--- a/help.c
+++ b/help.c
@@ -129,7 +129,6 @@ static void list_commands_in_dir(struct cmdnames *cmds,
const char *path,
const char *prefix)
{
- int prefix_len;
DIR *dir = opendir(path);
struct dirent *de;
struct strbuf buf = STRBUF_INIT;
@@ -139,15 +138,15 @@ static void list_commands_in_dir(struct cmdnames *cmds,
return;
if (!prefix)
prefix = "git-";
- prefix_len = strlen(prefix);
strbuf_addf(&buf, "%s/", path);
len = buf.len;
while ((de = readdir(dir)) != NULL) {
+ const char *ent;
int entlen;
- if (!starts_with(de->d_name, prefix))
+ if (!skip_prefix(de->d_name, prefix, &ent))
continue;
strbuf_setlen(&buf, len);
@@ -155,11 +154,11 @@ static void list_commands_in_dir(struct cmdnames *cmds,
if (!is_executable(buf.buf))
continue;
- entlen = strlen(de->d_name) - prefix_len;
- if (has_extension(de->d_name, ".exe"))
+ entlen = strlen(ent);
+ if (has_extension(ent, ".exe"))
entlen -= 4;
- add_cmdname(cmds, de->d_name + prefix_len, entlen);
+ add_cmdname(cmds, ent, entlen);
}
closedir(dir);
strbuf_release(&buf);
diff --git a/http.c b/http.c
index 2b4f6a3..f04621e 100644
--- a/http.c
+++ b/http.c
@@ -1087,11 +1087,10 @@ static int update_url_from_redirect(struct strbuf *base,
if (!strcmp(asked, got->buf))
return 0;
- if (!starts_with(asked, base->buf))
+ if (!skip_prefix(asked, base->buf, &tail))
die("BUG: update_url_from_redirect: %s is not a superset of %s",
asked, base->buf);
- tail = asked + base->len;
tail_len = strlen(tail);
if (got->len < tail_len ||
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 15/16] git: avoid magic number with skip_prefix
After handling options, any leftover arguments should be
commands. However, we pass through "--help" and "--version",
so that we convert them into "git help" and "git version"
respectively.
This is a straightforward use of skip_prefix to avoid a
magic number, but while we are there, it is worth adding a
comment to explain this otherwise confusing behavior.
Signed-off-by: Jeff King
---
Another option would be to teach handle_options to convert "--version"
into "version" itself. That's more disruptive, but I think would be
less confusing.
git.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/git.c b/git.c
index b2bb09e..1537f00 100644
--- a/git.c
+++ b/git.c
@@ -588,8 +588,8 @@ int main(int argc, char **av)
argc--;
handle_options(&argv, &argc, NULL);
if (argc > 0) {
- if (starts_with(argv[0], "--"))
- argv[0] += 2;
+ /* translate --help and --version into commands */
+ skip_prefix(argv[0], "--", &argv[0]);
} else {
/* The user didn't specify a command; give them help */
commit_pager_choice();
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 14/16] fetch-pack: refactor parsing in get_ack
There are several uses of the magic number "line+45" when
parsing ACK lines from the server, and it's rather unclear
why 45 is the correct number. We can make this more clear by
keeping a running pointer as we parse, using skip_prefix to
jump past the first "ACK ", then adding 40 to jump past
get_sha1_hex (which is still magical, but hopefully 40 is
less magical to readers of git code).
Note that this actually puts us at line+44. The original
required some character between the sha1 and further ACK
flags (it is supposed to be a space, but we never enforced
that). We start our search for flags at line+44, which
meanas we are slightly more liberal than the old code.
Signed-off-by: Jeff King
---
I actually think we could tighten this even more and drop the strstrs,
too, like:
arg += 40;
if (*arg++ != ' ')
return ACK;
if (!strcmp(arg, "continue"))
return ACK_continue;
and so on. But I wasn't sure if there was a reason for the use of
strstr. According to pack-protocol.txt, we would only get one at a time,
and always with a single space between them.
fetch-pack.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 3de3bd5..72ec520 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -189,20 +189,23 @@ static enum ack_type get_ack(int fd, unsigned char
*result_sha1)
{
int len;
char *line = packet_read_line(fd, &len);
+ const char *arg;
if (!len)
die("git fetch-pack: expected ACK/NAK, got EOF");
if (!strcmp(line, "NAK"))
return NAK;
- if (starts_with(line, "ACK ")) {
- if (!get_sha1_hex(line+4, result_sha1)) {
- if (len < 45)
+ if (skip_prefix(line, "ACK ", &arg)) {
+ if (!get_sha1_hex(arg, result_sha1)) {
+ arg += 40;
+ len -= arg - line;
+ if (len < 1)
return ACK;
- if (strstr(line+45, "continue"))
+ if (strstr(arg, "continue"))
return ACK_continue;
- if (strstr(line+45, "common"))
+ if (strstr(arg, "common"))
return ACK_common;
- if (strstr(line+45, "ready"))
+ if (strstr(arg, "ready"))
return ACK_ready;
return ACK;
}
--
2.0.0.566.gfe3e6b2
--
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] alloc.c: remove alloc_raw_commit_node() function
In order to encapsulate the setting of the unique commit index, commit
969eba63 ("commit: push commit_index update into alloc_commit_node",
10-06-2014) introduced a (logically private) intermediary allocator
function. However, this function (alloc_raw_commit_node()) was declared
as a public function, which undermines its entire purpose.
Remove the alloc_raw_commit_node() function and inline its code into
the (public) alloc_commit_node() function.
Noticed by sparse ("symbol 'alloc_raw_commit_node' was not declared.
Should it be static?").
Signed-off-by: Ramsay Jones
---
Hi Jeff,
I noticed this while it was still in 'pu', but got distracted and
didn't send this in time ... sorry about that! :(
My first attempt at fixing this involved changing the DEFINE_ALLOCATOR
macro to include a 'scope' parameter so that I could declare the
raw_commit allocator 'static'. Unfortunately, I could not pass the
extern keyword as the scope parameter to all the other allocators,
because that made sparse even more upset - you can't use extern on
a function _definition_. That meant passing an empty argument (or a
comment token) to the scope parameter. This worked for gcc 4.8.2 and
clang 3.4, but I was a little concerned about portability.
This seems a better solution to me. Having said that ... as I'm typing
this I realized that I could have removed the 'commit_count' variable
and used 'commit_allocs' to set c->index instead! :-P Oh well ...
ATB,
Ramsay Jones
alloc.c | 22 ++
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/alloc.c b/alloc.c
index eb22a45..124d710 100644
--- a/alloc.c
+++ b/alloc.c
@@ -47,16 +47,30 @@ union any_object {
DEFINE_ALLOCATOR(blob, struct blob)
DEFINE_ALLOCATOR(tree, struct tree)
-DEFINE_ALLOCATOR(raw_commit, struct commit)
DEFINE_ALLOCATOR(tag, struct tag)
DEFINE_ALLOCATOR(object, union any_object)
+static unsigned int commit_allocs;
+
void *alloc_commit_node(void)
{
static int commit_count;
- struct commit *c = alloc_raw_commit_node();
+ static int nr;
+ static struct commit *block;
+ struct commit *c;
+ void *ret;
+
+ if (!nr) {
+ nr = BLOCKING;
+ block = xmalloc(BLOCKING * sizeof(struct commit));
+ }
+ nr--;
+ commit_allocs++;
+ ret = block++;
+ memset(ret, 0, sizeof(struct commit));
+ c = (struct commit *) ret;
c->index = commit_count++;
- return c;
+ return ret;
}
static void report(const char *name, unsigned int count, size_t size)
@@ -72,7 +86,7 @@ void alloc_report(void)
{
REPORT(blob, struct blob);
REPORT(tree, struct tree);
- REPORT(raw_commit, struct commit);
+ REPORT(commit, struct commit);
REPORT(tag, struct tag);
REPORT(object, union any_object);
}
--
2.0.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 13/16] fast-import: refactor parsing of spaces
When we see a file change in a commit, we expect one of:
1. A mark.
2. An "inline" keyword.
3. An object sha1.
The handling of spaces is inconsistent between the three
options. Option 1 calls a sub-function which checks for the
space, but doesn't parse past it. Option 2 parses the space,
then deliberately avoids moving the pointer past it. Option
3 detects the space locally but doesn't move past it.
This is confusing, because it looks like option 1 forgets to
check for the space (it's just buried). And option 2 checks
for "inline ", but only moves strlen("inline") characters
forward, which looks like a bug but isn't.
We can make this more clear by just having each branch move
past the space as it is checked (and we can replace the
doubled use of "inline" with a call to skip_prefix).
Signed-off-by: Jeff King
---
fast-import.c | 20 +++-
1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 5f17adb..55ca7d8 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2269,7 +2269,7 @@ static uintmax_t parse_mark_ref_space(const char **p)
char *end;
mark = parse_mark_ref(*p, &end);
- if (*end != ' ')
+ if (*end++ != ' ')
die("Missing space after mark: %s", command_buf.buf);
*p = end;
return mark;
@@ -2304,20 +2304,17 @@ static void file_change_m(const char *p, struct branch
*b)
if (*p == ':') {
oe = find_mark(parse_mark_ref_space(&p));
hashcpy(sha1, oe->idx.sha1);
- } else if (starts_with(p, "inline ")) {
+ } else if (skip_prefix(p, "inline ", &p)) {
inline_data = 1;
oe = NULL; /* not used with inline_data, but makes gcc happy */
- p += strlen("inline"); /* advance to space */
} else {
if (get_sha1_hex(p, sha1))
die("Invalid dataref: %s", command_buf.buf);
oe = find_object(sha1);
p += 40;
- if (*p != ' ')
+ if (*p++ != ' ')
die("Missing space after SHA1: %s", command_buf.buf);
}
- assert(*p == ' ');
- p++; /* skip space */
strbuf_reset(&uq);
if (!unquote_c_style(&uq, p, &endp)) {
@@ -2474,20 +2471,17 @@ static void note_change_n(const char *p, struct branch
*b, unsigned char *old_fa
if (*p == ':') {
oe = find_mark(parse_mark_ref_space(&p));
hashcpy(sha1, oe->idx.sha1);
- } else if (starts_with(p, "inline ")) {
+ } else if (skip_prefix(p, "inline ", &p)) {
inline_data = 1;
oe = NULL; /* not used with inline_data, but makes gcc happy */
- p += strlen("inline"); /* advance to space */
} else {
if (get_sha1_hex(p, sha1))
die("Invalid dataref: %s", command_buf.buf);
oe = find_object(sha1);
p += 40;
- if (*p != ' ')
+ if (*p++ != ' ')
die("Missing space after SHA1: %s", command_buf.buf);
}
- assert(*p == ' ');
- p++; /* skip space */
/* */
s = lookup_branch(p);
@@ -3005,6 +2999,8 @@ static struct object_entry *parse_treeish_dataref(const
char **p)
die("Invalid dataref: %s", command_buf.buf);
e = find_object(sha1);
*p += 40;
+ if (*(*p)++ != ' ')
+ die("Missing space after tree-ish: %s",
command_buf.buf);
}
while (!e || e->type != OBJ_TREE)
@@ -3056,8 +3052,6 @@ static void parse_ls(const char *p, struct branch *b)
if (!is_null_sha1(root->versions[1].sha1))
root->versions[1].mode = S_IFDIR;
load_tree(root);
- if (*p++ != ' ')
- die("Missing space after tree-ish: %s",
command_buf.buf);
}
if (*p == '"') {
static struct strbuf uq = STRBUF_INIT;
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 12/16] stat_opt: check extra strlen call
As in earlier commits, the diff option parser uses
starts_with to find that an argument starts with "--stat-",
and then adds strlen("stat-") to find the rest of the
option.
However, in this case the starts_with and the strlen are
separated across functions, making it easy to call the
latter without the former. Let's use skip_prefix instead of
raw pointer arithmetic to catch such a case.
Signed-off-by: Jeff King
---
Another possibility would be for stat_opt to take only the
prefix-skipped part of the string. But that would involve refactoring
its use of "av" (it needs the whole array because it may need to grab a
follow-on argument).
diff.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/diff.c b/diff.c
index 2378ae4..06bdfb8 100644
--- a/diff.c
+++ b/diff.c
@@ -3422,7 +3422,8 @@ static int stat_opt(struct diff_options *options, const
char **av)
int count = options->stat_count;
int argcount = 1;
- arg += strlen("--stat");
+ if (!skip_prefix(arg, "--stat", &arg))
+ die("BUG: stat option does not begin with --stat: %s", arg);
end = (char *)arg;
switch (*arg) {
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 11/16] daemon: use skip_prefix to avoid magic numbers
Like earlier cases, we can use skip_prefix to avoid magic
numbers that must match the length of starts_with prefixes.
However, the numbers are a little more complicated here, as
we keep parsing past the prefix. We can solve it by keeping
a running pointer as we parse; its final value is the
location we want.
Signed-off-by: Jeff King
---
daemon.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/daemon.c b/daemon.c
index 6d25828..1eb6631 100644
--- a/daemon.c
+++ b/daemon.c
@@ -626,15 +626,16 @@ static int execute(void)
for (i = 0; i < ARRAY_SIZE(daemon_service); i++) {
struct daemon_service *s = &(daemon_service[i]);
- int namelen = strlen(s->name);
- if (starts_with(line, "git-") &&
- !strncmp(s->name, line + 4, namelen) &&
- line[namelen + 4] == ' ') {
+ const char *arg;
+
+ if (skip_prefix(line, "git-", &arg) &&
+ skip_prefix(arg, s->name, &arg) &&
+ *arg++ == ' ') {
/*
* Note: The directory here is probably context
sensitive,
* and might depend on the actual service being
performed.
*/
- return run_service(line + namelen + 5, s);
+ return run_service(arg, s);
}
}
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 10/16] fast-import: use skip_prefix for parsing input
Fast-import does a lot of parsing of commands and
dispatching to sub-functions. For example, given "option
foo", we might recognize "option " using starts_with, and
then hand it off to parse_option() to do the rest.
However, we do not let parse_option know that we have parsed
the first part already. It gets the full buffer, and has to
skip past the uninteresting bits. Some functions simply add
a magic constant:
char *option = command_buf.buf + 7;
Others use strlen:
char *option = command_buf.buf + strlen("option ");
And others use strchr:
char *option = strchr(command_buf.buf, ' ') + 1;
All of these are brittle and easy to get wrong (especially
given that the starts_with call and the code that assumes
the presence of the prefix are far apart). Instead, we can
use skip_prefix, and just pass each handler a pointer to its
arguments.
Signed-off-by: Jeff King
---
fast-import.c | 125 +-
1 file changed, 53 insertions(+), 72 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index a3ffe30..5f17adb 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -371,8 +371,8 @@ static volatile sig_atomic_t checkpoint_requested;
static int cat_blob_fd = STDOUT_FILENO;
static void parse_argv(void);
-static void parse_cat_blob(void);
-static void parse_ls(struct branch *b);
+static void parse_cat_blob(const char *p);
+static void parse_ls(const char *p, struct branch *b);
static void write_branch_report(FILE *rpt, struct branch *b)
{
@@ -1861,6 +1861,8 @@ static int read_next_command(void)
}
for (;;) {
+ const char *p;
+
if (unread_command_buf) {
unread_command_buf = 0;
} else {
@@ -1893,8 +1895,8 @@ static int read_next_command(void)
rc->prev->next = rc;
cmd_tail = rc;
}
- if (starts_with(command_buf.buf, "cat-blob ")) {
- parse_cat_blob();
+ if (skip_prefix(command_buf.buf, "cat-blob ", &p)) {
+ parse_cat_blob(p);
continue;
}
if (command_buf.buf[0] == '#')
@@ -2273,9 +2275,8 @@ static uintmax_t parse_mark_ref_space(const char **p)
return mark;
}
-static void file_change_m(struct branch *b)
+static void file_change_m(const char *p, struct branch *b)
{
- const char *p = command_buf.buf + 2;
static struct strbuf uq = STRBUF_INIT;
const char *endp;
struct object_entry *oe;
@@ -2376,9 +2377,8 @@ static void file_change_m(struct branch *b)
tree_content_set(&b->branch_tree, p, sha1, mode, NULL);
}
-static void file_change_d(struct branch *b)
+static void file_change_d(const char *p, struct branch *b)
{
- const char *p = command_buf.buf + 2;
static struct strbuf uq = STRBUF_INIT;
const char *endp;
@@ -2391,15 +2391,14 @@ static void file_change_d(struct branch *b)
tree_content_remove(&b->branch_tree, p, NULL, 1);
}
-static void file_change_cr(struct branch *b, int rename)
+static void file_change_cr(const char *s, struct branch *b, int rename)
{
- const char *s, *d;
+ const char *d;
static struct strbuf s_uq = STRBUF_INIT;
static struct strbuf d_uq = STRBUF_INIT;
const char *endp;
struct tree_entry leaf;
- s = command_buf.buf + 2;
strbuf_reset(&s_uq);
if (!unquote_c_style(&s_uq, s, &endp)) {
if (*endp != ' ')
@@ -2444,9 +2443,8 @@ static void file_change_cr(struct branch *b, int rename)
leaf.tree);
}
-static void note_change_n(struct branch *b, unsigned char *old_fanout)
+static void note_change_n(const char *p, struct branch *b, unsigned char
*old_fanout)
{
- const char *p = command_buf.buf + 2;
static struct strbuf uq = STRBUF_INIT;
struct object_entry *oe;
struct branch *s;
@@ -2587,7 +2585,7 @@ static int parse_from(struct branch *b)
const char *from;
struct branch *s;
- if (!starts_with(command_buf.buf, "from "))
+ if (!skip_prefix(command_buf.buf, "from ", &from))
return 0;
if (b->branch_tree.tree) {
@@ -2595,7 +2593,6 @@ static int parse_from(struct branch *b)
b->branch_tree.tree = NULL;
}
- from = strchr(command_buf.buf, ' ') + 1;
s = lookup_branch(from);
if (b == s)
die("Can't create a branch from itself: %s", b->name);
@@ -2636,8 +2633,7 @@ static struct hash_list *parse_merge(unsigned int *count)
struct branch *s;
*count = 0;
- while (starts_with(command_buf.buf, "merge ")) {
- from = strchr(command_buf.buf, ' ') + 1;
+ while (skip_prefix(command_buf.buf, "merge ", &from)) {
n = xmalloc(sizeof(*n));
s = lookup_branch(from);
if (s)
@@ -266
[PATCH 09/16] use skip_prefix to avoid repeating strings
It's a common idiom to match a prefix and then skip past it
with strlen, like:
if (starts_with(foo, "bar"))
foo += strlen("bar");
This avoids magic numbers, but means we have to repeat the
string (and there is no compiler check that we didn't make a
typo in one of the strings).
We can use skip_prefix to handle this case without repeating
ourselves.
Signed-off-by: Jeff King
---
builtin/checkout.c | 4 ++--
builtin/fmt-merge-msg.c | 6 +++---
builtin/log.c | 12 ++--
diff.c | 27 +--
help.c | 7 ---
merge-recursive.c | 15 ---
pretty.c| 3 +--
remote-curl.c | 15 ---
remote.c| 5 ++---
sha1_name.c | 4 +---
10 files changed, 44 insertions(+), 54 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f1dc56e..463cfee 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -776,8 +776,8 @@ static int switch_branches(const struct checkout_opts *opts,
if (!(flag & REF_ISSYMREF))
old.path = NULL;
- if (old.path && starts_with(old.path, "refs/heads/"))
- old.name = old.path + strlen("refs/heads/");
+ if (old.path)
+ skip_prefix(old.path, "refs/heads/", &old.name);
if (!new->name) {
new->name = "HEAD";
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 3c19241..ad3bc58 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -100,7 +100,8 @@ static int handle_line(char *line, struct merge_parents
*merge_parents)
{
int i, len = strlen(line);
struct origin_data *origin_data;
- char *src, *origin;
+ char *src;
+ const char *origin;
struct src_data *src_data;
struct string_list_item *item;
int pulling_head = 0;
@@ -164,8 +165,7 @@ static int handle_line(char *line, struct merge_parents
*merge_parents)
origin = line;
string_list_append(&src_data->tag, origin + 4);
src_data->head_status |= 2;
- } else if (starts_with(line, "remote-tracking branch ")) {
- origin = line + strlen("remote-tracking branch ");
+ } else if (skip_prefix(line, "remote-tracking branch ", &origin)) {
string_list_append(&src_data->r_branch, origin);
src_data->head_status |= 2;
} else {
diff --git a/builtin/log.c b/builtin/log.c
index a7ba211..0f59c25 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -872,7 +872,7 @@ static char *find_branch_name(struct rev_info *rev)
int i, positive = -1;
unsigned char branch_sha1[20];
const unsigned char *tip_sha1;
- const char *ref;
+ const char *ref, *v;
char *full_ref, *branch = NULL;
for (i = 0; i < rev->cmdline.nr; i++) {
@@ -888,9 +888,9 @@ static char *find_branch_name(struct rev_info *rev)
ref = rev->cmdline.rev[positive].name;
tip_sha1 = rev->cmdline.rev[positive].item->sha1;
if (dwim_ref(ref, strlen(ref), branch_sha1, &full_ref) &&
- starts_with(full_ref, "refs/heads/") &&
+ skip_prefix(full_ref, "refs/heads/", &v) &&
!hashcmp(tip_sha1, branch_sha1))
- branch = xstrdup(full_ref + strlen("refs/heads/"));
+ branch = xstrdup(v);
free(full_ref);
return branch;
}
@@ -1394,10 +1394,10 @@ int cmd_format_patch(int argc, const char **argv, const
char *prefix)
if (check_head) {
unsigned char sha1[20];
- const char *ref;
+ const char *ref, *v;
ref = resolve_ref_unsafe("HEAD", sha1, 1, NULL);
- if (ref && starts_with(ref, "refs/heads/"))
- branch_name = xstrdup(ref +
strlen("refs/heads/"));
+ if (ref && skip_prefix(ref, "refs/heads/", &v))
+ branch_name = xstrdup(v);
else
branch_name = xstrdup(""); /* no branch */
}
diff --git a/diff.c b/diff.c
index 97db932..2378ae4 100644
--- a/diff.c
+++ b/diff.c
@@ -3395,12 +3395,10 @@ int parse_long_opt(const char *opt, const char **argv,
const char **optarg)
{
const char *arg = argv[0];
- if (arg[0] != '-' || arg[1] != '-')
+ if (!skip_prefix(arg, "--", &arg))
return 0;
- arg += strlen("--");
- if (!starts_with(arg, opt))
+ if (!skip_prefix(arg, opt, &arg))
return 0;
- arg += strlen(opt);
if (*arg == '=') { /* stuck form: --option=value */
*optarg = arg + 1;
return 1;
@@ -3429,8 +3427,7 @@ static int stat_opt(struct diff_options *options, const
char **av)
switch (*arg) {
case '-':
-
[PATCH 08/16] use skip_prefix to avoid magic numbers
It's a common idiom to match a prefix and then skip past it
with a magic number, like:
if (starts_with(foo, "bar"))
foo += 3;
This is easy to get wrong, since you have to count the
prefix string yourself, and there's no compiler check if the
string changes. We can use skip_prefix to avoid the magic
numbers here.
Note that some of these conversions could be much shorter.
For example:
if (starts_with(arg, "--foo=")) {
bar = arg + 6;
continue;
}
could become:
if (skip_prefix(arg, "--foo=", &bar))
continue;
However, I have left it as:
if (skip_prefix(arg, "--foo=", &v)) {
bar = v;
continue;
}
to visually match nearby cases which need to actually
process the string. Like:
if (skip_prefix(arg, "--foo=", &v)) {
bar = atoi(v);
continue;
}
Signed-off-by: Jeff King
---
alias.c| 3 ++-
connect.c | 11 +
convert.c | 4 ++--
daemon.c | 73 ++
diff.c | 65 ++-
fast-import.c | 69 +-
fetch-pack.c | 9
git.c | 18 +++
help.c | 6 +++--
http-backend.c | 11 +
http-push.c| 11 +
11 files changed, 149 insertions(+), 131 deletions(-)
diff --git a/alias.c b/alias.c
index 5efc3d6..758c867 100644
--- a/alias.c
+++ b/alias.c
@@ -5,7 +5,8 @@ static char *alias_val;
static int alias_lookup_cb(const char *k, const char *v, void *cb)
{
- if (starts_with(k, "alias.") && !strcmp(k + 6, alias_key)) {
+ const char *name;
+ if (skip_prefix(k, "alias.", &name) && !strcmp(name, alias_key)) {
if (!v)
return config_error_nonbool(k);
alias_val = xstrdup(v);
diff --git a/connect.c b/connect.c
index 94a6650..37ff018 100644
--- a/connect.c
+++ b/connect.c
@@ -129,6 +129,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t
src_len,
char *name;
int len, name_len;
char *buffer = packet_buffer;
+ const char *arg;
len = packet_read(in, &src_buf, &src_len,
packet_buffer, sizeof(packet_buffer),
@@ -140,12 +141,12 @@ struct ref **get_remote_heads(int in, char *src_buf,
size_t src_len,
if (!len)
break;
- if (len > 4 && starts_with(buffer, "ERR "))
- die("remote error: %s", buffer + 4);
+ if (len > 4 && skip_prefix(buffer, "ERR ", &arg))
+ die("remote error: %s", arg);
- if (len == 48 && starts_with(buffer, "shallow ")) {
- if (get_sha1_hex(buffer + 8, old_sha1))
- die("protocol error: expected shallow sha-1,
got '%s'", buffer + 8);
+ if (len == 48 && skip_prefix(buffer, "shallow ", &arg)) {
+ if (get_sha1_hex(arg, old_sha1))
+ die("protocol error: expected shallow sha-1,
got '%s'", arg);
if (!shallow_points)
die("repository on the other end cannot be
shallow");
sha1_array_append(shallow_points, old_sha1);
diff --git a/convert.c b/convert.c
index ab80b72..cb5fbb4 100644
--- a/convert.c
+++ b/convert.c
@@ -1121,9 +1121,9 @@ static int is_foreign_ident(const char *str)
{
int i;
- if (!starts_with(str, "$Id: "))
+ if (!skip_prefix(str, "$Id: ", &str))
return 0;
- for (i = 5; str[i]; i++) {
+ for (i = 0; str[i]; i++) {
if (isspace(str[i]) && str[i+1] != '$')
return 1;
}
diff --git a/daemon.c b/daemon.c
index 18818c3..6d25828 100644
--- a/daemon.c
+++ b/daemon.c
@@ -235,8 +235,10 @@ static int service_enabled;
static int git_daemon_config(const char *var, const char *value, void *cb)
{
- if (starts_with(var, "daemon.") &&
- !strcmp(var + 7, service_looking_at->config_name)) {
+ const char *service;
+
+ if (skip_prefix(var, "daemon.", &service) &&
+ !strcmp(service, service_looking_at->config_name)) {
service_enabled = git_config_bool(var, value);
return 0;
}
@@ -1133,16 +1135,17 @@ int main(int argc, char **argv)
for (i = 1; i < argc; i++) {
char *arg = argv[i];
+ const char *v;
- if (starts_with(arg, "--listen=")) {
- string_list_append(&listen_addr, xstrdup_tolower(arg +
9));
+ if (skip_prefix(arg, "--listen=", &v)) {
+ string_list_append(&listen_addr, xstrdup_tolower(v));
continue;
}
- if (starts_with(arg, "--port=")) {
+
[PATCH 07/16] transport-helper: avoid reading past end-of-string
We detect the "import-marks" capability by looking for that
string, but _without_ a trailing space. Then we skip past it
using strlen("import-marks "), with a space. So if a remote
helper gives us exactly "import-marks", we will read past
the end-of-string by one character.
This is unlikely to be a problem in practice, because such
input is malformed in the first place, and because there is
a good chance that the string has an extra NUL terminator
one character after the original (because it formerly had a
newline in it that we parsed off).
We can fix it by using skip_prefix with "import-marks ",
with the space. The other form appears to be a typo from
a515ebe (transport-helper: implement marks location as
capability, 2011-07-16); "import-marks" has never existed
without an argument, and it should match the "export-marks"
definition above.
Speaking of which, we can also use skip_prefix in a few
other places while we are in the function.
Signed-off-by: Jeff King
---
transport-helper.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/transport-helper.c b/transport-helper.c
index 84c616f..3d8fe7d 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -153,7 +153,7 @@ static struct child_process *get_helper(struct transport
*transport)
write_constant(helper->in, "capabilities\n");
while (1) {
- const char *capname;
+ const char *capname, *arg;
int mandatory = 0;
if (recvline(data, &buf))
exit(128);
@@ -183,19 +183,19 @@ static struct child_process *get_helper(struct transport
*transport)
data->export = 1;
else if (!strcmp(capname, "check-connectivity"))
data->check_connectivity = 1;
- else if (!data->refspecs && starts_with(capname, "refspec ")) {
+ else if (!data->refspecs && skip_prefix(capname, "refspec ",
&arg)) {
ALLOC_GROW(refspecs,
refspec_nr + 1,
refspec_alloc);
- refspecs[refspec_nr++] = xstrdup(capname +
strlen("refspec "));
+ refspecs[refspec_nr++] = xstrdup(arg);
} else if (!strcmp(capname, "connect")) {
data->connect = 1;
} else if (!strcmp(capname, "signed-tags")) {
data->signed_tags = 1;
- } else if (starts_with(capname, "export-marks ")) {
- data->export_marks = xstrdup(capname +
strlen("export-marks "));
- } else if (starts_with(capname, "import-marks")) {
- data->import_marks = xstrdup(capname +
strlen("import-marks "));
+ } else if (skip_prefix(capname, "export-marks ", &arg)) {
+ data->export_marks = xstrdup(arg);
+ } else if (skip_prefix(capname, "import-marks ", &arg)) {
+ data->import_marks = xstrdup(arg);
} else if (starts_with(capname, "no-private-update")) {
data->no_private_update = 1;
} else if (mandatory) {
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 06/16] fast-import: fix read of uninitialized argv memory
Fast-import shares code between its command-line parser and
the "option" command. To do so, it strips the "--" from any
command-line options and passes them to the option parser.
However, it does not confirm that the option even begins
with "--" before blindly passing "arg + 2".
It does confirm that the option starts with "-", so the only
affected case was:
git fast-import -
which would read uninitialized memory after the argument. We
can fix it by using skip_prefix and checking the result. As
a bonus, this gets rid of some magic numbers.
Signed-off-by: Jeff King
---
fast-import.c | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 6707a66..b2030cc 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3342,18 +3342,21 @@ static void parse_argv(void)
if (*a != '-' || !strcmp(a, "--"))
break;
- if (parse_one_option(a + 2))
+ if (!skip_prefix(a, "--", &a))
+ die("unknown option %s", a);
+
+ if (parse_one_option(a))
continue;
- if (parse_one_feature(a + 2, 0))
+ if (parse_one_feature(a, 0))
continue;
- if (starts_with(a + 2, "cat-blob-fd=")) {
- option_cat_blob_fd(a + 2 + strlen("cat-blob-fd="));
+ if (skip_prefix(a, "cat-blob-fd=", &a)) {
+ option_cat_blob_fd(a);
continue;
}
- die("unknown option %s", a);
+ die("unknown option --%s", a);
}
if (i != global_argc)
usage(fast_import_usage);
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 05/16] apply: use skip_prefix instead of raw addition
A submodule diff generally has content like:
-Subproject commit [0-9a-f]{40}
+Subproject commit [0-9a-f]{40}
When we are using "git apply --index" with a submodule, we
first apply the textual diff, and then parse that result to
figure out the new sha1.
If the diff has bogus input like:
-Subproject commit 1234567890123456789012345678901234567890
+bogus
we will parse the "bogus" portion. Our parser assumes that
the buffer starts with "Subproject commit", and blindly
skips past it using strlen(). This can cause us to read
random memory after the buffer.
This problem was unlikely to have come up in practice (since
it requires a malformed diff), and even when it did, we
likely noticed the problem anyway as the next operation was
to call get_sha1_hex on the random memory.
However, we can easily fix it by using skip_prefix to notice
the parsing error.
Signed-off-by: Jeff King
---
builtin/apply.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index 9c5724e..bc924ab 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3847,9 +3847,10 @@ static void add_index_file(const char *path, unsigned
mode, void *buf, unsigned
ce->ce_flags = create_ce_flags(0);
ce->ce_namelen = namelen;
if (S_ISGITLINK(mode)) {
- const char *s = buf;
+ const char *s;
- if (get_sha1_hex(s + strlen("Subproject commit "), ce->sha1))
+ if (!skip_prefix(buf, "Subproject commit ", &s) ||
+ get_sha1_hex(s, ce->sha1))
die(_("corrupt patch for submodule %s"), path);
} else {
if (!cached) {
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 04/16] refactor skip_prefix to return a boolean
The skip_prefix function returns a pointer to the content
past the prefix, or NULL if the prefix was not found. While
this is nice and simple, in practice it makes it hard to use
for two reasons:
1. When you want to conditionally skip or keep the string
as-is, you have to introduce a second temporary
variable. For example:
tmp = skip_prefix(buf, "foo");
if (tmp)
buf = tmp;
2. It is verbose to check the outcome in a conditional, as
you need extra parentheses to silence compiler
warnings. For example:
if ((cp = skip_prefix(buf, "foo"))
/* do something with cp */
Both of these make it harder to use for long if-chains, and
we tend to use starts_with instead. However, the first line
of "do something" is often to then skip forward in buf past
the prefix, either using a magic constant or with an extra
strlen (which is generally computed at compile time, but
means we are repeating ourselves).
This patch refactors skip_prefix to return a simple boolean,
and to provide the pointer value as an out-parameter. If the
prefix is not found, the out-parameter is untouched. This
lets you write:
if (skip_prefix(arg, "foo ", &arg))
do_foo(arg);
else if (skip_prefix(arg, "bar ", &arg))
do_bar(arg);
Signed-off-by: Jeff King
---
The diffstat is misleading. We actually save lines, except that I added
documentation for the function. :)
advice.c | 5 -
branch.c | 4 ++--
builtin/branch.c | 6 +++---
builtin/clone.c| 11 +++
builtin/commit.c | 5 ++---
builtin/fmt-merge-msg.c| 2 +-
builtin/push.c | 7 +++
builtin/remote.c | 4 +---
column.c | 5 +++--
commit.c | 6 ++
config.c | 3 +--
credential-cache--daemon.c | 6 ++
credential.c | 3 +--
fsck.c | 14 +-
git-compat-util.h | 26 ++
parse-options.c| 16 +---
transport.c| 4 +++-
urlmatch.c | 3 +--
18 files changed, 72 insertions(+), 58 deletions(-)
diff --git a/advice.c b/advice.c
index c50ebdf..9b42033 100644
--- a/advice.c
+++ b/advice.c
@@ -61,9 +61,12 @@ void advise(const char *advice, ...)
int git_default_advice_config(const char *var, const char *value)
{
- const char *k = skip_prefix(var, "advice.");
+ const char *k;
int i;
+ if (!skip_prefix(var, "advice.", &k))
+ return 0;
+
for (i = 0; i < ARRAY_SIZE(advice_config); i++) {
if (strcmp(k, advice_config[i].name))
continue;
diff --git a/branch.c b/branch.c
index 660097b..46e8aa8 100644
--- a/branch.c
+++ b/branch.c
@@ -50,11 +50,11 @@ static int should_setup_rebase(const char *origin)
void install_branch_config(int flag, const char *local, const char *origin,
const char *remote)
{
- const char *shortname = skip_prefix(remote, "refs/heads/");
+ const char *shortname = NULL;
struct strbuf key = STRBUF_INIT;
int rebasing = should_setup_rebase(origin);
- if (shortname
+ if (skip_prefix(remote, "refs/heads/", &shortname)
&& !strcmp(local, shortname)
&& !origin) {
warning(_("Not setting branch %s as its own upstream."),
diff --git a/builtin/branch.c b/builtin/branch.c
index 652b1d2..0591b22 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -294,13 +294,13 @@ static char *resolve_symref(const char *src, const char
*prefix)
{
unsigned char sha1[20];
int flag;
- const char *dst, *cp;
+ const char *dst;
dst = resolve_ref_unsafe(src, sha1, 0, &flag);
if (!(dst && (flag & REF_ISSYMREF)))
return NULL;
- if (prefix && (cp = skip_prefix(dst, prefix)))
- dst = cp;
+ if (prefix)
+ skip_prefix(dst, prefix, &dst);
return xstrdup(dst);
}
diff --git a/builtin/clone.c b/builtin/clone.c
index b12989d..a5b2d9d 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -584,11 +584,11 @@ static void update_remote_refs(const struct ref *refs,
static void update_head(const struct ref *our, const struct ref *remote,
const char *msg)
{
- if (our && starts_with(our->name, "refs/heads/")) {
+ const char *head;
+ if (our && skip_prefix(our->name, "refs/heads/", &head)) {
/* Local default branch link */
create_symref("HEAD", our->name, NULL);
if (!option_bare) {
- const char *head = skip_prefix(our->name,
"refs/heads/");
update_ref(msg, "HEAD", our->old_sha1, NULL, 0,
UPDATE_REFS_DIE_ON_ERR);
install_branch_config(0, head, option_origin,
our->nam
[PATCH 03/16] avoid using skip_prefix as a boolean
There's no point in using: if (skip_prefix(buf, "foo")) over if (starts_with(buf, "foo")) as the point of skip_prefix is to return a pointer to the data after the prefix. Using starts_with is more readable, and will make refactoring skip_prefix easier. Signed-off-by: Jeff King --- builtin/fmt-merge-msg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c index 3906eda..72378e6 100644 --- a/builtin/fmt-merge-msg.c +++ b/builtin/fmt-merge-msg.c @@ -298,7 +298,7 @@ static void credit_people(struct strbuf *out, (them->nr == 1 && me && (me = skip_prefix(me, them->items->string)) != NULL && -skip_prefix(me, " <"))) +starts_with(me, " <"))) return; strbuf_addf(out, "\n%c %s ", comment_line_char, label); add_people_count(out, them); -- 2.0.0.566.gfe3e6b2 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 01/16] parse_diff_color_slot: drop ofs parameter
This function originally took a whole config variable name
("var") and an offset ("ofs"). It checked "var+ofs" against
each color slot, but reported errors using the whole "var".
However, since 8b8e862 (ignore unknown color configuration,
2009-12-12), it returns -1 rather than printing its own
error, and therefore only cares about var+ofs. We can drop
the ofs parameter and teach its sole caller to derive the
pointer itself.
Signed-off-by: Jeff King
---
diff.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/diff.c b/diff.c
index bba9a55..77c5eb4 100644
--- a/diff.c
+++ b/diff.c
@@ -52,23 +52,23 @@ static char diff_colors[][COLOR_MAXLEN] = {
GIT_COLOR_NORMAL, /* FUNCINFO */
};
-static int parse_diff_color_slot(const char *var, int ofs)
+static int parse_diff_color_slot(const char *var)
{
- if (!strcasecmp(var+ofs, "plain"))
+ if (!strcasecmp(var, "plain"))
return DIFF_PLAIN;
- if (!strcasecmp(var+ofs, "meta"))
+ if (!strcasecmp(var, "meta"))
return DIFF_METAINFO;
- if (!strcasecmp(var+ofs, "frag"))
+ if (!strcasecmp(var, "frag"))
return DIFF_FRAGINFO;
- if (!strcasecmp(var+ofs, "old"))
+ if (!strcasecmp(var, "old"))
return DIFF_FILE_OLD;
- if (!strcasecmp(var+ofs, "new"))
+ if (!strcasecmp(var, "new"))
return DIFF_FILE_NEW;
- if (!strcasecmp(var+ofs, "commit"))
+ if (!strcasecmp(var, "commit"))
return DIFF_COMMIT;
- if (!strcasecmp(var+ofs, "whitespace"))
+ if (!strcasecmp(var, "whitespace"))
return DIFF_WHITESPACE;
- if (!strcasecmp(var+ofs, "func"))
+ if (!strcasecmp(var, "func"))
return DIFF_FUNCINFO;
return -1;
}
@@ -240,7 +240,7 @@ int git_diff_basic_config(const char *var, const char
*value, void *cb)
return -1;
if (starts_with(var, "diff.color.") || starts_with(var, "color.diff."))
{
- int slot = parse_diff_color_slot(var, 11);
+ int slot = parse_diff_color_slot(var + 11);
if (slot < 0)
return 0;
if (!value)
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 02/16] daemon: mark some strings as const
None of these strings is modified; marking them as const
will help later refactoring.
Signed-off-by: Jeff King
---
daemon.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/daemon.c b/daemon.c
index f9c63e9..18818c3 100644
--- a/daemon.c
+++ b/daemon.c
@@ -39,8 +39,8 @@ static int strict_paths;
static int export_all_trees;
/* Take all paths relative to this one if non-NULL */
-static char *base_path;
-static char *interpolated_path;
+static const char *base_path;
+static const char *interpolated_path;
static int base_path_relaxed;
/* Flag indicating client sent extra args. */
@@ -106,12 +106,12 @@ static void NORETURN daemon_die(const char *err, va_list
params)
exit(1);
}
-static const char *path_ok(char *directory)
+static const char *path_ok(const char *directory)
{
static char rpath[PATH_MAX];
static char interp_path[PATH_MAX];
const char *path;
- char *dir;
+ const char *dir;
dir = directory;
@@ -131,7 +131,7 @@ static const char *path_ok(char *directory)
* "~alice/%s/foo".
*/
int namlen, restlen = strlen(dir);
- char *slash = strchr(dir, '/');
+ const char *slash = strchr(dir, '/');
if (!slash)
slash = dir + restlen;
namlen = slash - dir;
@@ -253,7 +253,7 @@ static int daemon_error(const char *dir, const char *msg)
return -1;
}
-static char *access_hook;
+static const char *access_hook;
static int run_access_hook(struct daemon_service *service, const char *dir,
const char *path)
{
@@ -318,7 +318,7 @@ error_return:
return -1;
}
-static int run_service(char *dir, struct daemon_service *service)
+static int run_service(const char *dir, struct daemon_service *service)
{
const char *path;
int enabled = service->enabled;
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/16] skip_prefix refactoring and cleanups
A while ago[1] we discussed refactoring skip_prefix (or adding something like it) to make it more natural to call. This morning I decided to take a look at doing this, and went down a rabbit hole of cleanups. This is part one of the result. The short of it is that skip_prefix can now be used like this: if (skip_prefix(arg, "--foo=", &value) handle_foo(value); or even: /* arg remains valid if we didn't match! */ if (skip_prefix(arg, "--foo=", &arg)) handle_foo(arg); else if (skip_prefix(arg, "--bar", &arg)) handle_bar(arg); though the latter form is not always useful if the conditional code wants to see all of "arg". [01/16]: parse_diff_color_slot: drop ofs parameter [02/16]: daemon: mark some strings as const [03/16]: avoid using skip_prefix as a boolean These ones are preparatory cleanup. [04/16]: refactor skip_prefix to return a boolean The actual refactoring; it changes the existing callers[2] at the same time. [05/16]: apply: use skip_prefix instead of raw addition [06/16]: fast-import: fix read of uninitialized argv memory [07/16]: transport-helper: avoid reading past end-of-string These three are conversions that actually fix bugs. [08/16]: use skip_prefix to avoid magic numbers [09/16]: use skip_prefix to avoid repeating strings These are the straightforward conversions lumped together by the problem they are solving. [10/16]: fast-import: use skip_prefix for parsing input [11/16]: daemon: use skip_prefix to avoid magic numbers [12/16]: stat_opt: check extra strlen call [13/16]: fast-import: refactor parsing of spaces [14/16]: fetch-pack: refactor parsing in get_ack [15/16]: git: avoid magic number with skip_prefix These ones are variants of the above two that needed extra discussion or attention for various reasons. [16/16]: use skip_prefix to avoid repeated calculations These ones don't solve a specific problem as the patches above do, but I think the code ends up more readable. My conversions are by now means exhaustive. After grepping through all of the starts_with up to about http.c, I decided to call it a day. But we can easily convert more as time goes on. [1] http://thread.gmane.org/gmane.comp.version-control.git/239438/focus=239564 I started from scratch this morning, oblivious to the fact that René posted something very similar to patch 4 in that thread. [2] I test-merged with 'pu'. There is a minor textual conflict with jk/commit-buffer-length that should be easy to resolve. There's also one new caller of skip_prefix added in cc/interpret-trailers. It needs this fix when merged: diff --git a/trailer.c b/trailer.c index eaf692b..987fa29 100644 --- a/trailer.c +++ b/trailer.c @@ -377,8 +377,7 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb) enum trailer_info_type type; int i; - trailer_item = skip_prefix(conf_key, "trailer."); - if (!trailer_item) + if (!skip_prefix(conf_key, "trailer.", &trailer_item)) return 0; variable_name = strrchr(trailer_item, '.'); -- 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 v3 0/5] cleanup duplicate name_compare() functions
Jeremiah Mahler wrote: > Jeremiah Mahler (5): > cache: rename cache_name_compare() to name_compare() > tree-walk.c: remove name_compare() function > unpack-trees.c: remove name_compare() function > dir.c: rename to name_compare() > name-hash.c: rename to name_compare() > > cache.h| 2 +- > dir.c | 3 +-- > name-hash.c| 2 +- > read-cache.c | 23 +-- > tree-walk.c| 10 -- > unpack-trees.c | 11 --- > 6 files changed, 16 insertions(+), 35 deletions(-) After looking at the patches I suspect this should be a single patch. That way it's bisectable, and the changes outside of read-cache.c are small enough that it's not too much of a burden to review as a single patch. The code change looked good. Thanks and hope that helps, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/5] cache: rename cache_name_compare() to name_compare()
Jeremiah Mahler wrote: > The cache_name_compare() function is not specific to a cache. > Make its name more general by renaming it to name_compare(). Sounds reasonable. -- 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 v3 5/5] name-hash.c: rename to name_compare()
Jeremiah Mahler wrote: > name-hash.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Same thoughts as patch 4/5. -- 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 v3 4/5] dir.c: rename to name_compare()
Jeremiah Mahler wrote: > This is a case where cache_name_compare() was used even though it had > nothing to do with a cache. The new name makes it clear that no cache > is involved. That's a perfect sort of thing to put in the commit message. ;-) Unlike patches 2 and 3, this could make sense to me as a separate patch from 1/5. Except... how does git work at all with patch 1 and without this patch? I thought that patch removed the public cache_name_compare function. Would it make sense to delay the removal of cache_name_compare until a patch at the end of the series? The patch is small enough that squashing into patch 1 seems fine, too. [...] > Rename the call to cache_name_compare() to name_compare(). It's not actually renaming but calling a different function, right? So I'd say something like read_directory: use name_compare instead of cache_name_compare This is a case where cache_name_compare() was used even though it had nothing to do with a cache. The new name makes it clear that no cache is involved. No functional change intended. Thanks, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/5] unpack-trees.c: remove name_compare() function
Jeremiah Mahler wrote: > unpack-trees.c | 11 --- > 1 file changed, 11 deletions(-) Same thoughts as patch 2/5. :) Thanks, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/5] tree-walk.c: remove name_compare() function
Jeremiah Mahler wrote: > Remove the duplicate name_compare() function and use the one provided by > read-cache.c. I'd squash this into patch 1/5. > --- > Notes: > There is one small difference between the old function and the new one. > The old one returned -N and +N whereas the new one returns -1 and +1. > However, there is no place where the magnitude was needed, so this > change will not alter its behavior. This is useful information for anyone looking back at the patch in the future, so it belongs above the three-dash divider. Thanks, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 4/5] dir.c: rename to name_compare()
Rename the call to cache_name_compare() to name_compare(). Signed-off-by: Jeremiah Mahler --- Notes: This is a case where cache_name_compare() was used even though it had nothing to do with a cache. The new name makes it clear that no cache is involved. dir.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dir.c b/dir.c index 797805d..e65888d 100644 --- a/dir.c +++ b/dir.c @@ -1354,8 +1354,7 @@ static int cmp_name(const void *p1, const void *p2) const struct dir_entry *e1 = *(const struct dir_entry **)p1; const struct dir_entry *e2 = *(const struct dir_entry **)p2; - return cache_name_compare(e1->name, e1->len, - e2->name, e2->len); + return name_compare(e1->name, e1->len, e2->name, e2->len); } static struct path_simplify *create_simplify(const char **pathspec) -- 2.0.0 -- 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 v3 3/5] unpack-trees.c: remove name_compare() function
Remove the duplicate name_compare() function and use the one provided by
read-cache.c.
Signed-off-by: Jeremiah Mahler
---
Notes:
There is one small difference between the old function and the new one.
The old one returned -N and +N whereas the new one returns -1 and +1.
However, there is no place where the magnitude was needed, so this
change will not alter its behavior.
unpack-trees.c | 11 ---
1 file changed, 11 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index 4a9cdf2..c4a97ca 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -629,17 +629,6 @@ static int unpack_failed(struct unpack_trees_options *o,
const char *message)
return -1;
}
-/* NEEDSWORK: give this a better name and share with tree-walk.c */
-static int name_compare(const char *a, int a_len,
- const char *b, int b_len)
-{
- int len = (a_len < b_len) ? a_len : b_len;
- int cmp = memcmp(a, b, len);
- if (cmp)
- return cmp;
- return (a_len - b_len);
-}
-
/*
* The tree traversal is looking at name p. If we have a matching entry,
* return it. If name p is a directory in the index, do not return
--
2.0.0
--
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 v3 2/5] tree-walk.c: remove name_compare() function
Remove the duplicate name_compare() function and use the one provided by
read-cache.c.
Signed-off-by: Jeremiah Mahler
---
Notes:
There is one small difference between the old function and the new one.
The old one returned -N and +N whereas the new one returns -1 and +1.
However, there is no place where the magnitude was needed, so this
change will not alter its behavior.
tree-walk.c | 10 --
1 file changed, 10 deletions(-)
diff --git a/tree-walk.c b/tree-walk.c
index 4dc86c7..5dd9a71 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -144,16 +144,6 @@ struct tree_desc_x {
struct tree_desc_skip *skip;
};
-static int name_compare(const char *a, int a_len,
- const char *b, int b_len)
-{
- int len = (a_len < b_len) ? a_len : b_len;
- int cmp = memcmp(a, b, len);
- if (cmp)
- return cmp;
- return (a_len - b_len);
-}
-
static int check_entry_match(const char *a, int a_len, const char *b, int
b_len)
{
/*
--
2.0.0
--
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 v3 5/5] name-hash.c: rename to name_compare()
Rename the call to cache_name_compare() to name_compare(). Signed-off-by: Jeremiah Mahler --- name-hash.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/name-hash.c b/name-hash.c index be7c4ae..e2bea88 100644 --- a/name-hash.c +++ b/name-hash.c @@ -179,7 +179,7 @@ static int same_name(const struct cache_entry *ce, const char *name, int namelen * Always do exact compare, even if we want a case-ignoring comparison; * we do the quick exact one first, because it will be the common case. */ - if (len == namelen && !cache_name_compare(name, namelen, ce->name, len)) + if (len == namelen && !name_compare(name, namelen, ce->name, len)) return 1; if (!icase) -- 2.0.0 -- 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 v3 1/5] cache: rename cache_name_compare() to name_compare()
The cache_name_compare() function is not specific to a cache.
Make its name more general by renaming it to name_compare().
Simplify cache_name_stage_compare() via name_compare().
Where lengths are involved, change int to size_t.
Signed-off-by: Jeremiah Mahler
---
cache.h | 2 +-
read-cache.c | 23 +--
2 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/cache.h b/cache.h
index c498a30..e3205fe 100644
--- a/cache.h
+++ b/cache.h
@@ -1027,7 +1027,7 @@ extern int validate_headref(const char *ref);
extern int base_name_compare(const char *name1, int len1, int mode1, const
char *name2, int len2, int mode2);
extern int df_name_compare(const char *name1, int len1, int mode1, const char
*name2, int len2, int mode2);
-extern int cache_name_compare(const char *name1, int len1, const char *name2,
int len2);
+extern int name_compare(const char *name1, size_t len1, const char *name2,
size_t len2);
extern int cache_name_stage_compare(const char *name1, int len1, int stage1,
const char *name2, int len2, int stage2);
extern void *read_object_with_reference(const unsigned char *sha1,
diff --git a/read-cache.c b/read-cache.c
index 9f56d76..158241d 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -434,18 +434,26 @@ int df_name_compare(const char *name1, int len1, int
mode1,
return c1 - c2;
}
-int cache_name_stage_compare(const char *name1, int len1, int stage1, const
char *name2, int len2, int stage2)
+int name_compare(const char *name1, size_t len1, const char *name2, size_t
len2)
{
- int len = len1 < len2 ? len1 : len2;
- int cmp;
-
- cmp = memcmp(name1, name2, len);
+ size_t min_len = (len1 < len2) ? len1 : len2;
+ int cmp = memcmp(name1, name2, min_len);
if (cmp)
return cmp;
if (len1 < len2)
return -1;
if (len1 > len2)
return 1;
+ return 0;
+}
+
+int cache_name_stage_compare(const char *name1, int len1, int stage1, const
char *name2, int len2, int stage2)
+{
+ int cmp;
+
+ cmp = name_compare(name1, len1, name2, len2);
+ if (cmp)
+ return cmp;
if (stage1 < stage2)
return -1;
@@ -454,11 +462,6 @@ int cache_name_stage_compare(const char *name1, int len1,
int stage1, const char
return 0;
}
-int cache_name_compare(const char *name1, int len1, const char *name2, int
len2)
-{
- return cache_name_stage_compare(name1, len1, 0, name2, len2, 0);
-}
-
static int index_name_stage_pos(const struct index_state *istate, const char
*name, int namelen, int stage)
{
int first, last;
--
2.0.0
--
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 v3 0/5] cleanup duplicate name_compare() functions
Version 3 of the patch series to cleanup duplicate name_compare() functions (previously was 'add strnncmp() function' [1]). This version goes in a slightly different direction than the previous version. Before I was trying to add a strnncmp() function so I could remove duplicate copies of the name_compare() function in tree-walk.c and unpack-trees.c. But then Torsten Bögershausen pointed out that there is a cache_name_compare() function which is nearly identical to name_compare() [2]*. * cache_name_compare() is not identical to name_compare(). The former returns +1, -1, whereas the latter returns +N, -N. But there is no place where name_compare() was used that needed the magnitude so this change would not alter its behavior. So I decided why not generalize the name of cache_name_compare() by renaming it to name_compare(), since it doesn't do anything with caches, other than being part of cache.h and read-cache.c. Then the duplicate name_compare() functions can be removed and the few places that used cache_name_compare() can be renamed to name_compare(). It cleans up the code with a minimal number of changes. It keeps existing functions instead of creating new ones. And there are several other functions in cache.h that are similarly named '*name_compare' so it follows the already established style. Also, the name_compare() now uses memcmp() as it did originally instead of using strncmp() as it did in the last version. [1]: http://marc.info/?l=git&m=140299051431479&w=2 [2]: http://marc.info/?l=git&m=140300329403706&w=2 Jeremiah Mahler (5): cache: rename cache_name_compare() to name_compare() tree-walk.c: remove name_compare() function unpack-trees.c: remove name_compare() function dir.c: rename to name_compare() name-hash.c: rename to name_compare() cache.h| 2 +- dir.c | 3 +-- name-hash.c| 2 +- read-cache.c | 23 +-- tree-walk.c| 10 -- unpack-trees.c | 11 --- 6 files changed, 16 insertions(+), 35 deletions(-) -- 2.0.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC] git-am: support any number of signatures
On Wed, Jun 18, 2014 at 10:51:04AM -0700, Junio C Hamano wrote: > Junio C Hamano writes: > > > On Tue, Jun 17, 2014 at 8:09 PM, Michael S. Tsirkin wrote: > >> > >> OK, after looking into this for a while, I realize > >> this is a special property of the Signed-off-by footer. > >> For now I think it's reasonable to just avoid de-duplicating > >> other footers if any. Agree? > > > > Not really. I'd rather see "git am" hardcode as little such policy as > > possible. > > We do need to support S-o-b footer and the policy we defined for it long > > time > > ago, if only for backward compatiblity, but for any other footers, > > policy decision > > such as "dedup by default" isn't something "am" should know about. > > By the way, "append without looking for dups" is a policy decision > that is as bad to have as "append with dedup". > > I'd rather not to see "am.signoff", or any name that implies what > the "-s" option to the command is about for that matter, to be used > in futzing with the trailers other than S-o-b in any way. As I > understand it, our longer term goal is to defer that task, including > the user-programmable policy decisions, to something like the > 'trailer' Christian started. > > I suspect that it may add unnecessary work later if we overloaded > "signoff" with a similar feature with the change under discussion. > I would feel safer to see it outlined how we envision to transition > to a more generic 'trailer' solution later if we were to enhance > "am" with "am.signoff" now. > > Thanks. I'll need to look at trailers, if indeed they are a superset of this functionality, I will transition to trailers when they land on master. In this case seems easier for me to keep this out tree patch for now, Good thing I didn't spend time writing docs and tests :) -- MST -- 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: cherry-pick lost
guangai@travelzen.com writes: > I delete a file and push to master branch, after code reviewing > in gerrit, then click 'Cherry Pick To' button to cherry-pick to > release/1.1 branch, and then code review and merge... Gerrit folks, for which this list may not be the best place to get in touch with, may know a lot better than anybody on this list what "Cherry Pick To" button does, how it does so, what the typical user errors are, etc. https://code.google.com/p/gerrit/ tells me that perhaps you would have better luck with these places: https://groups.google.com/forum/#!forum/repo-discuss IRC freenode #gerrit -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC] git-am: support any number of signatures
Junio C Hamano writes: > On Tue, Jun 17, 2014 at 8:09 PM, Michael S. Tsirkin wrote: >> >> OK, after looking into this for a while, I realize >> this is a special property of the Signed-off-by footer. >> For now I think it's reasonable to just avoid de-duplicating >> other footers if any. Agree? > > Not really. I'd rather see "git am" hardcode as little such policy as > possible. > We do need to support S-o-b footer and the policy we defined for it long time > ago, if only for backward compatiblity, but for any other footers, > policy decision > such as "dedup by default" isn't something "am" should know about. By the way, "append without looking for dups" is a policy decision that is as bad to have as "append with dedup". I'd rather not to see "am.signoff", or any name that implies what the "-s" option to the command is about for that matter, to be used in futzing with the trailers other than S-o-b in any way. As I understand it, our longer term goal is to defer that task, including the user-programmable policy decisions, to something like the 'trailer' Christian started. I suspect that it may add unnecessary work later if we overloaded "signoff" with a similar feature with the change under discussion. I would feel safer to see it outlined how we envision to transition to a more generic 'trailer' solution later if we were to enhance "am" with "am.signoff" now. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 5/5] refs.c: rollback the lockfile before we die() in repack_without_refs
Signed-off-by: Ronnie Sahlberg
---
refs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 89e5bc0..582591b 100644
--- a/refs.c
+++ b/refs.c
@@ -2548,8 +2548,10 @@ int repack_without_refs(const char **refnames, int n,
struct strbuf *err)
/* Remove any other accumulated cruft */
do_for_each_entry_in_dir(packed, 0, curate_packed_ref_fn,
&refs_to_delete);
for_each_string_list_item(ref_to_delete, &refs_to_delete) {
- if (remove_entry(packed, ref_to_delete->string) == -1)
+ if (remove_entry(packed, ref_to_delete->string) == -1) {
+ rollback_packed_refs();
die("internal error");
+ }
}
/* Write what remains */
--
2.0.0.467.g08c0633
--
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 v5 10/11] trace: add trace_performance facility to debug performance issues
Karsten Blees writes:
> Right, it makes no sense for trace_performance(), and for
> trace_performance_since() only if followed by another 'measured' code
> section. In that special case, I think it wouldn't hurt if you had to
> write:
>
> uint64_t start = getnanotime();
> /* first code section to measure */
> trace_performance_since(start, "first foobar");
>
> start = getnanotime();
> /* second code section to measure */
> trace_performance_since(start, "second foobar");
>
> So I guess I'll drop the return value (and the second example, which
> is then redundant to the first).
That also sounds OK to me.
>>> +static void trace_performance_vfl(const char *file, int line,
>>> + uint64_t nanos, const char *format,
>>> + va_list ap)
>>> +{
>>
>> Just being curious, but what does "v" stand for?
>>
>
> trace_performance_vfl(, va_list)
> vs.
> trace_performance_fl(, ...)
>
> Will change to trace_performance_vprintf_fl()
Ah, OK. The name with 'vprintf' in it does sound better.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] Second part of msysgit/unicode
Stepan Kasal writes: > Hello Karsten, > > On Tue, Jun 17, 2014 at 11:06:52AM +0200, Karsten Blees wrote: >> Am 11.06.2014 11:37, schrieb Stepan Kasal: >> > This is the second part of the time-proven unicode suport branch from >> > msysgit. >> > This batch is a collection of small independent changes, limited to >> > mingw.c. >> > The only exception is the last patch: it changes gitk and git-gui. >> >> I'm missing the other two "Unicode file name" patches (and "Win32: fix >> detection > > indeed. I noticed that after sending the plan quoted above. > Luckily, the gitk/git-gui patch was not accepted and has to be > resubmitted. > > So the plan for future submissions is: > > 1) two "Unicode file name" patches (with "fix... is_dir_empty" > squashed) > 2) the environament patches from your unicode branch (several > patches) > 3) "color term" (and env. var. TERM); updated according to your > instructions, thus sent separately after the series > 4) resubmit gitk / git-gui (have separate maintainers) > > This is work in progress, I suppose to mail 1) and 2) in a few days. > > Stepan In the meantime, are Windows folks happy with the four topics queued on 'pu' so far? I would like to start moving them down to 'next' and to 'master' soonish. They consist of these individual patches: $ git shortlog ^master \ sk/mingw-dirent \ sk/mingw-main \ sk/mingw-uni-console \ sk/mingw-unicode-spawn-args Johannes Schindelin (1): Win32: let mingw_execve() return an int Karsten Blees (18): Win32 dirent: remove unused dirent.d_ino member Win32 dirent: remove unused dirent.d_reclen member Win32 dirent: change FILENAME_MAX to MAX_PATH Win32 dirent: clarify #include directives Win32 dirent: improve dirent implementation Win32: move main macro to a function Win32: support Unicode console output Win32: detect console streams more reliably Win32: warn if the console font doesn't support Unicode Win32: add Unicode conversion functions Win32: Thread-safe windows console output Win32: fix broken pipe detection Win32: reliably detect console pipe handles Win32: simplify internal mingw_spawn* APIs Win32: fix potential multi-threading issue MinGW: disable CRT command line globbing Win32: Unicode arguments (outgoing) Win32: Unicode arguments (incoming) Stepan Kasal (1): mingw: avoid const warning Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Possible bug in `gitreset` in 1.9
Followup on this, it looks like the local repository actually didn't contain branch-2. So this doesn't appear to be an issue. -- 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, #04; Tue, 17)
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, totalling close to 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"] * mt/patch-id-stable (2014-06-10) 1 commit - patch-id: change default to stable Teaches "git patch-id" to compute the patch ID that does not change when the files in a single patch is reordered. As this new algorithm is backward incompatible, the last bit to flip it to be the default is left out of 'master' for now. * as/pretty-truncate (2014-05-21) 5 commits (merged to 'next' on 2014-06-10 at d8147a2) + pretty.c: format string with truncate respects logOutputEncoding + t4205, t6006: add tests that fail with i18n.logOutputEncoding set + t4205 (log-pretty-format): use `tformat` rather than `format` + t4041, t4205, t6006, t7102: don't hardcode tested encoding value + t4205 (log-pretty-formats): don't hardcode SHA-1 in expected outputs Originally merged to 'next' on 2014-05-23 * bg/xcalloc-nmemb-then-size (2014-05-27) 12 commits (merged to 'next' on 2014-06-10 at eddb5bc) + transport-helper.c: rearrange xcalloc arguments + remote.c: rearrange xcalloc arguments + reflog-walk.c: rearrange xcalloc arguments + pack-revindex.c: rearrange xcalloc arguments + notes.c: rearrange xcalloc arguments + imap-send.c: rearrange xcalloc arguments + http-push.c: rearrange xcalloc arguments + diff.c: rearrange xcalloc arguments + config.c: rearrange xcalloc arguments + commit.c: rearrange xcalloc arguments + builtin/remote.c: rearrange xcalloc arguments + builtin/ls-remote.c: rearrange xcalloc arguments Originally merged to 'next' on 2014-06-06 Like calloc(3), xcalloc() takes nmemb and then size. * cb/byte-order (2014-05-30) 3 commits (merged to 'next' on 2014-06-10 at 63db8ee) + compat/bswap.h: fix endianness detection + compat/bswap.h: restore preference __BIG_ENDIAN over BIG_ENDIAN + compat/bswap.h: detect endianness on more platforms that don't use BYTE_ORDER Originally merged to 'next' on 2014-05-30 Compatibility enhancement for Solaris. * cc/replace-edit (2014-05-19) 10 commits (merged to 'next' on 2014-06-10 at ff69722) + Documentation: replace: describe new --edit option + replace: add --edit to usage string + replace: add tests for --edit + replace: die early if replace ref already exists + replace: refactor checking ref validity + replace: make sure --edit results in a different object + replace: add --edit option + replace: factor object resolution out of replace_object + replace: use OPT_CMDMODE to handle modes + replace: refactor command-mode determination (this branch is used by cc/replace-graft.) Originally merged to 'next' on 2014-05-19 "git replace" learns a new "--edit" option. * dt/refs-check-refname-component-optim (2014-06-05) 1 commit (merged to 'next' on 2014-06-10 at 4560669) + refs.c: optimize check_refname_component() (this branch is used by dt/refs-check-refname-component-sse42.) Originally merged to 'next' on 2014-06-06 * fc/remote-helper-refmap (2014-04-21) 8 commits (merged to 'next' on 2014-06-10 at 8cd8cf8) + transport-helper: remove unnecessary strbuf resets + transport-helper: add support to delete branches + fast-export: add support to delete refs + fast-import: add support to delete refs + transport-helper: add support to push symbolic refs + transport-helper: add support for old:new refspec + fast-export: add new --refspec option + fast-export: improve argument parsing Originally merged to 'next' on 2014-04-22 Allow remote-helper/fast-import based transport to rename the refs while transferring the history. * ib/test-selectively-run (2014-06-06) 4 commits (merged to 'next' on 2014-06-10 at 1235570) + t-*.sh: fix the GIT_SKIP_TESTS sub-tests + test-lib: '--run' to run only specific tests + test-lib: tests skipped by GIT_SKIP_TESTS say so + test-lib: document short options in t/README Originally merged to 'next' on 2014-06-06 Allow specifying only certain individual test pieces to be run using a range notation (e.g. "t1234-test.sh --run='1-4 6 8 9-'"). * jk/argv-array-for-child-process (2014-05-15) 7 commits (merged to 'next' on 2014-06-10 at 07a167b) + argv-array: drop "detach" code + get_importer: use run-command's internal argv_array + get_exporter: use argv_array + get_helper: use run-command's internal argv_array + git_connect: use argv_array + run_column_filter: use argv_array + run-command: store an optional argv_array Originally merged t
[PATCH v3 1/5] refs.c: allow passing raw git_committer_info as email to _update_reflog
In many places in the code we do not have access to the individual fields
in the committer data. Instead we might only have access to prebaked data
such as what is returned by git_committer_info() containing a string
that consists of email, timestamp, zone etc.
This makes it inconvenient to use transaction_update_reflog since it means
you would have to first parse git_committer_info before you can call
update_reflog.
Add a new flag REFLOG_EMAIL_IS_COMMITTER to _update_reflog to tell it
that what we pass in as email is already the fully baked committer string
we can use as-is.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 20
refs.h | 1 +
2 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/refs.c b/refs.c
index 95c3eb8..11dcb07 100644
--- a/refs.c
+++ b/refs.c
@@ -3521,14 +3521,18 @@ int transaction_update_reflog(struct ref_transaction
*transaction,
hashcpy(update->old_sha1, old_sha1);
update->reflog_fd = -1;
if (email) {
- struct strbuf buf = STRBUF_INIT;
- char sign = (tz < 0) ? '-' : '+';
- int zone = (tz < 0) ? (-tz) : tz;
-
- strbuf_addf(&buf, "%s %lu %c%04d", email, timestamp, sign,
- zone);
- update->committer = xstrdup(buf.buf);
- strbuf_release(&buf);
+ if (flags & REFLOG_EMAIL_IS_COMMITTER)
+ update->committer = xstrdup(email);
+ else {
+ struct strbuf buf = STRBUF_INIT;
+ char sign = (tz < 0) ? '-' : '+';
+ int zone = (tz < 0) ? (-tz) : tz;
+
+ strbuf_addf(&buf, "%s %lu %c%04d", email, timestamp,
+ sign, zone);
+ update->committer = xstrdup(buf.buf);
+ strbuf_release(&buf);
+ }
}
if (msg)
update->msg = xstrdup(msg);
diff --git a/refs.h b/refs.h
index b674c20..469d27f 100644
--- a/refs.h
+++ b/refs.h
@@ -315,6 +315,7 @@ int transaction_delete_sha1(struct ref_transaction
*transaction,
* Flags >= 0x100 are reserved for internal use.
*/
#define REFLOG_TRUNCATE 0x01
+#define REFLOG_EMAIL_IS_COMMITTER 0x02
/*
* Append a reflog entry for refname. If the REFLOG_TRUNCATE flag is set
* this update will first truncate the reflog before writing the entry.
--
2.0.0.467.g08c0633
--
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 v3 3/5] refs.c: use packed refs when deleting refs during a transaction
Make the deletion of refs during a transaction more atomic.
Start by first copying all loose refs we will be deleting to the packed
refs file and then commit the packed refs file. Then re-lock the packed refs
file to avoid anyone else from modifying the refs we are to delete during
this transaction and keep it locked until we are finished.
Since all refs we are about to delete are now safely held in the packed refs
file we can proceed to immediately unlink any loose refs for those refs
and still be fully rollback-able.
The exception is for refs that can not be resolved. Those refs are never
added to the packed refs and will just be un-rollback-ably deleted during
commit.
By deleting all the loose refs at the start of the transaction we make make
it possible to both delete one ref and then re-create a different ref in
the same transaction, even if both the old-to-be-deleted and the
new-to-be-created refs would otherwise conflict in the file-system.
Example: rename m->m/m
In that example we want to delete the file 'm' so that we make room so
that we can create a directory with the same name in order to lock and
write to the ref m/m and its lock-file m/m.lock.
If there is a failure during the commit phase we can rollback without losing
any refs since we have so far only deleted the loose refs but we still have
the refs stored in the packed refs file.
Once we have finished all updates for refs and their reflogs we can repack
the packed refs file and remove the to-be-deleted refs from the packed refs,
at which point all the deleted refs will disappear in one atomic rename
operation.
Signed-off-by: Ronnie Sahlberg
---
builtin/remote.c | 13 +++--
refs.c | 150 +++
2 files changed, 127 insertions(+), 36 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index 401feb3..beb6e34 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -755,6 +755,8 @@ 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;
+ if (lock_packed_refs(0))
+ result |= unable_to_lock_error(git_path("packed-refs"), errno);
result |= repack_without_refs(branch_names, branches->nr, NULL);
free(branch_names);
@@ -1332,9 +1334,14 @@ static int prune_remote(const char *remote, int dry_run)
delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
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, NULL);
+ if (!dry_run) {
+ if (lock_packed_refs(0))
+ result |= unable_to_lock_error(
+ git_path("packed-refs"), errno);
+ else
+ result |= repack_without_refs(delete_refs,
+ states.stale.nr, NULL);
+ }
free(delete_refs);
}
diff --git a/refs.c b/refs.c
index a644e7c..63c2122 100644
--- a/refs.c
+++ b/refs.c
@@ -1322,7 +1322,7 @@ static struct ref_entry *get_packed_ref(const char
*refname)
/*
* A loose ref file doesn't exist; check for a packed ref. The
- * options are forwarded from resolve_safe_unsafe().
+ * options are forwarded from resolve_ref_unsafe().
*/
static const char *handle_missing_loose_ref(const char *refname,
unsigned char *sha1,
@@ -1379,7 +1379,6 @@ const char *resolve_ref_unsafe(const char *refname,
unsigned char *sha1, int rea
}
git_snpath(path, sizeof(path), "%s", refname);
-
/*
* We might have to loop back here to avoid a race
* condition: first we lstat() the file, then we try
@@ -2510,6 +2509,9 @@ static int curate_packed_ref_fn(struct ref_entry *entry,
void *cb_data)
return 0;
}
+/*
+ * Must be called with packed refs already locked (and sorted)
+ */
int repack_without_refs(const char **refnames, int n, struct strbuf *err)
{
struct ref_dir *packed;
@@ -2522,19 +2524,12 @@ int repack_without_refs(const char **refnames, int n,
struct strbuf *err)
if (get_packed_ref(refnames[i]))
break;
- /* Avoid locking if we have nothing to do */
- if (i == n)
+ /* Avoid processing if we have nothing to do */
+ if (i == n) {
+ rollback_packed_refs();
return 0; /* no refname exists in packed refs */
-
- if (lock_packed_refs(0)) {
- if (err) {
- unable_to_lock_mess
[PATCH v3 0/5] ref-transactions-rename
These patches can also be found at: https://github.com/rsahlberg/git/tree/ref-transactions-rename This series is based on, and applies ontop of, the previous ref-transactions-reflog series, also found at my githup repo. This series updates the reflog handling and converts rename_ref to use a single transaction to delete the old ref, create the new ref and to move the reflog. In order to make the transaction fully atomic we also introduce support for using the packed refs file for performing the delete-and-create that is done during the rename. This means that the rename operation is fully atomic for all external observers and there is no longer a point in time where the sha1 is unreferenced, after the old ref has been deleted but before the new ref has been created. Additionally, since we now use the transaction API for the reflog changes we no longer need to disallow renames for refs whose reflogs are symbolic links. Version 3: - Updated to build ontop of current ref-transactions-reflog Ronnie Sahlberg (5): refs.c: allow passing raw git_committer_info as email to _update_reflog refs.c: return error instead of dying when locking fails during transaction refs.c: use packed refs when deleting refs during a transaction refs.c: update rename_ref to use a transaction refs.c: rollback the lockfile before we die() in repack_without_refs builtin/remote.c | 13 +- refs.c| 368 +- refs.h| 1 + t/t3200-branch.sh | 7 -- 4 files changed, 209 insertions(+), 180 deletions(-) -- 2.0.0.467.g08c0633 -- 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 v3 2/5] refs.c: return error instead of dying when locking fails during transaction
Change lock_ref_sha1_basic to return an error instead of dying when we fail to lock a file during a transaction. Signed-off-by: Ronnie Sahlberg --- refs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/refs.c b/refs.c index 11dcb07..a644e7c 100644 --- a/refs.c +++ b/refs.c @@ -2195,7 +2195,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, */ goto retry; else - unable_to_lock_index_die(ref_file, errno); + goto error_return; } return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock; -- 2.0.0.467.g08c0633 -- 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 v3 4/5] refs.c: update rename_ref to use a transaction
Change refs.c to use a single transaction to copy/rename both the refs and
its reflog. Since we are no longer using rename() to move the reflog file
we no longer need to disallow rename_ref for refs with a symlink for its
reflog so we can remove that test from the testsuite.
Change the function to return 1 on failure instead of either -1 or 1.
Signed-off-by: Ronnie Sahlberg
---
refs.c| 192 ++
t/t3200-branch.sh | 7 --
2 files changed, 65 insertions(+), 134 deletions(-)
diff --git a/refs.c b/refs.c
index 63c2122..89e5bc0 100644
--- a/refs.c
+++ b/refs.c
@@ -2616,81 +2616,42 @@ int delete_ref(const char *refname, const unsigned char
*sha1, int delopt)
return 0;
}
-/*
- * People using contrib's git-new-workdir have .git/logs/refs ->
- * /some/other/path/.git/logs/refs, and that may live on another device.
- *
- * IOW, to avoid cross device rename errors, the temporary renamed log must
- * live into logs/refs.
- */
-#define TMP_RENAMED_LOG "logs/refs/.tmp-renamed-log"
+struct rename_reflog_cb {
+ struct ref_transaction *transaction;
+ const char *refname;
+ struct strbuf *err;
+};
-static int rename_tmp_log(const char *newrefname)
+static int rename_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
+const char *email, unsigned long timestamp, int tz,
+const char *message, void *cb_data)
{
- int attempts_remaining = 4;
-
- retry:
- switch (safe_create_leading_directories(git_path("logs/%s",
newrefname))) {
- case SCLD_OK:
- break; /* success */
- case SCLD_VANISHED:
- if (--attempts_remaining > 0)
- goto retry;
- /* fall through */
- default:
- error("unable to create directory for %s", newrefname);
- return -1;
- }
+ struct rename_reflog_cb *cb = cb_data;
- if (rename(git_path(TMP_RENAMED_LOG), git_path("logs/%s", newrefname)))
{
- if ((errno==EISDIR || errno==ENOTDIR) && --attempts_remaining >
0) {
- /*
-* rename(a, b) when b is an existing
-* directory ought to result in ISDIR, but
-* Solaris 5.8 gives ENOTDIR. Sheesh.
-*/
- if (remove_empty_directories(git_path("logs/%s",
newrefname))) {
- error("Directory not empty: logs/%s",
newrefname);
- return -1;
- }
- goto retry;
- } else if (errno == ENOENT && --attempts_remaining > 0) {
- /*
-* Maybe another process just deleted one of
-* the directories in the path to newrefname.
-* Try again from the beginning.
-*/
- goto retry;
- } else {
- error("unable to move logfile "TMP_RENAMED_LOG" to
logs/%s: %s",
- newrefname, strerror(errno));
- return -1;
- }
- }
- return 0;
+ return transaction_update_reflog(cb->transaction, cb->refname,
+nsha1, osha1, email, timestamp, tz,
+message, 0, cb->err);
}
-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];
- int flag = 0, logmoved = 0;
- struct ref_lock *lock;
- struct stat loginfo;
- int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
+ unsigned char sha1[20];
+ int flag = 0, log;
+ struct ref_transaction *transaction = NULL;
+ struct strbuf err = STRBUF_INIT;
const char *symref = NULL;
+ struct rename_reflog_cb cb;
- if (log && S_ISLNK(loginfo.st_mode))
- return error("reflog for %s is a symlink", oldrefname);
-
- symref = resolve_ref_unsafe(oldrefname, orig_sha1, 1, &flag);
- if (flag & REF_ISSYMREF)
- return error("refname %s is a symbolic ref, renaming it is not
supported",
+ symref = resolve_ref_unsafe(oldrefname, sha1, 1, &flag);
+ if (flag & REF_ISSYMREF) {
+ error("refname %s is a symbolic ref, renaming it is not
supported",
oldrefname);
- if (!symref)
- return error("refname %s not found", oldrefname);
+ return 1;
+ }
+ if (!symref) {
+ error("refname %s not found", oldrefname);
+ return 1;
+ }
if (!is_refname_available(newrefname, get_packe
RE: Why does gpg.program work for commit but not log?
-- -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- - - - Jason Pyeron PD Inc. http://www.pdinc.us - - Principal Consultant 10 West 24th Street #100- - +1 (443) 269-1555 x333Baltimore, Maryland 21218 - - - -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- This message is copyright PD Inc, subject to license 20080407P00. > -Original Message- > From: git-ow...@vger.kernel.org > [mailto:git-ow...@vger.kernel.org] On Behalf Of Jeff King > Sent: Wednesday, June 18, 2014 3:37 > To: Jason Pyeron > Cc: git@vger.kernel.org > Subject: Re: Why does gpg.program work for commit but not log? > > On Wed, Jun 18, 2014 at 12:18:35AM -0400, Jason Pyeron wrote: > > > jpyeron@black /projects/microsoft-smartcard-sign/tmp > > $ git --version > > git version 1.7.9 > > That's rather old. In the meantime we have: > > commit 6005dbb9bf21d10b209f7924e305bd04b9ab56d2 > Author: Jacob Sarvis > Date: Wed Mar 27 10:13:39 2013 -0500 > > log: read gpg settings for signed commit verification > > "show --show-signature" and "log --show-signature" > do not read the > gpg.program setting from git config, even though, > commit signing, > tag signing, and tag verification honor it. > > which is in v1.8.3. In general, please double-check your problem > against a recent version of "master" when making a bug report. Ok, so now I will be using locally compiled git. jpyeron@black /projects/microsoft-smartcard-sign/tmp $ /projects/git/local-build/bin/git --version git version 1.8.4.21.g992c386 jpyeron@black /projects/microsoft-smartcard-sign/tmp $ GIT_TRACE=1 /projects/git/local-build/bin/git log --show-signature trace: built-in: git 'log' '--show-signature' trace: run_command: 'less' trace: exec: 'less' trace: run_command: '/projects/microsoft-smartcard-sign/tmp/bin/logginggpg.sh' '--status-fd=1' '--verify' '/tmp/.git_vtag_tmp66WxpM' '-' commit 38afa1f4d0c73fd47d5788310a1a2080aa0abbba -- 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 v3 06/14] lockfile.c: make hold_lock_file_for_append preserve meaningful errno
Update hold_lock_file_for_append and copy_fd to return a meaningful errno
on failure.
Signed-off-by: Ronnie Sahlberg
---
copy.c | 20 +---
lockfile.c | 7 ++-
2 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/copy.c b/copy.c
index a7f58fd..5cb8679 100644
--- a/copy.c
+++ b/copy.c
@@ -9,10 +9,12 @@ int copy_fd(int ifd, int ofd)
if (!len)
break;
if (len < 0) {
- int read_error = errno;
+ int save_errno = errno;
close(ifd);
- return error("copy-fd: read returned %s",
-strerror(read_error));
+ error("copy-fd: read returned %s",
+ strerror(save_errno));
+ errno = save_errno;
+ return -1;
}
while (len) {
int written = xwrite(ofd, buf, len);
@@ -22,12 +24,16 @@ int copy_fd(int ifd, int ofd)
}
else if (!written) {
close(ifd);
- return error("copy-fd: write returned 0");
+ error("copy-fd: write returned 0");
+ errno = EAGAIN;
+ return -1;
} else {
- int write_error = errno;
+ int save_errno = errno;
close(ifd);
- return error("copy-fd: write returned %s",
-strerror(write_error));
+ error("copy-fd: write returned %s",
+ strerror(save_errno));
+ errno = save_errno;
+ return -1;
}
}
}
diff --git a/lockfile.c b/lockfile.c
index a921d77..32f4681 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -217,15 +217,20 @@ int hold_lock_file_for_append(struct lock_file *lk, const
char *path, int flags)
orig_fd = open(path, O_RDONLY);
if (orig_fd < 0) {
if (errno != ENOENT) {
+ int save_errno = errno;
if (flags & LOCK_DIE_ON_ERROR)
die("cannot open '%s' for copying", path);
close(fd);
- return error("cannot open '%s' for copying", path);
+ error("cannot open '%s' for copying", path);
+ errno = save_errno;
+ return -1;
}
} else if (copy_fd(orig_fd, fd)) {
+ int save_errno = errno;
if (flags & LOCK_DIE_ON_ERROR)
exit(128);
close(fd);
+ errno = save_errno;
return -1;
}
return fd;
--
2.0.0.467.g08c0633
--
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 v3 07/14] refs.c: add a transaction function to append a reflog entry
Define a new transaction update type, UPDATE_LOG, and a new function
transaction_update_reflog. This function will lock the reflog and append
an entry to it during transaction commit.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 101 -
refs.h | 12
2 files changed, 112 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index f203285..d673a0f 100644
--- a/refs.c
+++ b/refs.c
@@ -3388,6 +3388,7 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
enum transaction_update_type {
UPDATE_SHA1 = 0,
+ UPDATE_LOG = 1,
};
/**
@@ -3405,6 +3406,12 @@ struct ref_update {
struct ref_lock *lock;
int type;
char *msg;
+
+ /* used by reflog updates */
+ int reflog_fd;
+ struct lock_file reflog_lock;
+ char *committer;
+
const char refname[FLEX_ARRAY];
};
@@ -3454,6 +3461,7 @@ void transaction_free(struct ref_transaction *transaction)
for (i = 0; i < transaction->nr; i++) {
free(transaction->updates[i]->msg);
+ free(transaction->updates[i]->committer);
free(transaction->updates[i]);
}
free(transaction->updates);
@@ -3474,6 +3482,42 @@ static struct ref_update *add_update(struct
ref_transaction *transaction,
return update;
}
+int transaction_update_reflog(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *new_sha1,
+ const unsigned char *old_sha1,
+ const unsigned char *email,
+ unsigned long timestamp, int tz,
+ const char *msg, int flags,
+ struct strbuf *err)
+{
+ struct ref_update *update;
+
+ if (transaction->state != REF_TRANSACTION_OPEN)
+ die("BUG: update_reflog called for transaction that is not "
+ "open");
+
+ update = add_update(transaction, refname, UPDATE_LOG);
+ hashcpy(update->new_sha1, new_sha1);
+ hashcpy(update->old_sha1, old_sha1);
+ update->reflog_fd = -1;
+ if (email) {
+ struct strbuf buf = STRBUF_INIT;
+ char sign = (tz < 0) ? '-' : '+';
+ int zone = (tz < 0) ? (-tz) : tz;
+
+ strbuf_addf(&buf, "%s %lu %c%04d", email, timestamp, sign,
+ zone);
+ update->committer = xstrdup(buf.buf);
+ strbuf_release(&buf);
+ }
+ if (msg)
+ update->msg = xstrdup(msg);
+ update->flags = flags;
+
+ return 0;
+}
+
int transaction_update_sha1(struct ref_transaction *transaction,
const char *refname,
const unsigned char *new_sha1,
@@ -3640,7 +3684,28 @@ int transaction_commit(struct ref_transaction
*transaction,
}
}
- /* Perform updates first so live commits remain referenced */
+ /* Lock all reflog files */
+ for (i = 0; i < n; i++) {
+ struct ref_update *update = updates[i];
+
+ if (update->update_type != UPDATE_LOG)
+ continue;
+ update->reflog_fd = hold_lock_file_for_append(
+ &update->reflog_lock,
+ git_path("logs/%s", update->refname),
+ 0);
+ if (update->reflog_fd < 0) {
+ const char *str = "Cannot lock reflog for '%s'. %s";
+
+ ret = -1;
+ if (err)
+ strbuf_addf(err, str, update->refname,
+ strerror(errno));
+ goto cleanup;
+ }
+ }
+
+ /* Perform ref updates first so live commits remain referenced */
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
@@ -3676,6 +3741,40 @@ int transaction_commit(struct ref_transaction
*transaction,
}
}
+ /* Update all reflog files */
+ for (i = 0; i < n; i++) {
+ struct ref_update *update = updates[i];
+
+ if (update->update_type != UPDATE_LOG)
+ continue;
+ if (update->reflog_fd == -1)
+ continue;
+
+ if (log_ref_write_fd(update->reflog_fd, update->old_sha1,
+update->new_sha1,
+update->committer, update->msg)) {
+ error("Could write to reflog: %s. %s",
+ update->refname, strerror(errno));
+ rollback_lock_file(&update->reflog_lock);
+ update->reflog_fd = -1;
+ }
+
[PATCH v3 12/14] refs.c: rename log_ref_setup to create_reflog
log_ref_setup is used to do several semi-related things :
* sometimes it will create a new reflog including missing parent directories
and cleaning up any conflicting stale directories in the path.
* fill in a filename buffer for the full path to the reflog.
* unconditionally re-adjust the permissions for the file.
This function is only called from two places: checkout.c where it is always
used to create a reflog and refs.c:log_ref_write where it sometimes are
used to create a reflog and sometimes just used to fill in the filename.
Rename log_ref_setup to create_reflog and change it to only take the
refname as argument to make its signature similar to delete_reflog and
reflog_exists. Change create_reflog to ignore log_all_ref_updates and
"unconditionally" create the reflog when called. Since checkout.c always
wants to create a reflog we can call create_reflog directly and avoid the
temp-and-log_all_ref_update dance.
In log_ref_write, only call create_reflog iff we want to create a reflog
and if the reflog does not yet exist. This means that for the common case
where the log already exists we now only need to perform a single lstat()
instead of a open(O_CREAT)+lstat()+close().
Signed-off-by: Ronnie Sahlberg
---
builtin/checkout.c | 8 +---
refs.c | 22 +-
refs.h | 8 +++-
3 files changed, 17 insertions(+), 21 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f1dc56e..808c58f 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -583,19 +583,13 @@ static void update_refs_for_switch(const struct
checkout_opts *opts,
if (opts->new_branch) {
if (opts->new_orphan_branch) {
if (opts->new_branch_log && !log_all_ref_updates) {
- int temp;
- char log_file[PATH_MAX];
char *ref_name = mkpath("refs/heads/%s",
opts->new_orphan_branch);
- temp = log_all_ref_updates;
- log_all_ref_updates = 1;
- if (log_ref_setup(ref_name, log_file,
sizeof(log_file))) {
+ if (create_reflog(ref_name)) {
fprintf(stderr, _("Can not do reflog
for '%s'\n"),
opts->new_orphan_branch);
- log_all_ref_updates = temp;
return;
}
- log_all_ref_updates = temp;
}
}
else
diff --git a/refs.c b/refs.c
index 1288c49..9653a01 100644
--- a/refs.c
+++ b/refs.c
@@ -2822,16 +2822,16 @@ static int copy_msg(char *buf, const char *msg)
}
/* This function must set a meaningful errno on failure */
-int log_ref_setup(const char *refname, char *logfile, int bufsize)
+int create_reflog(const char *refname)
{
int logfd, oflags = O_APPEND | O_WRONLY;
+ char logfile[PATH_MAX];
- git_snpath(logfile, bufsize, "logs/%s", refname);
- if (log_all_ref_updates &&
- (starts_with(refname, "refs/heads/") ||
-starts_with(refname, "refs/remotes/") ||
-starts_with(refname, "refs/notes/") ||
-!strcmp(refname, "HEAD"))) {
+ git_snpath(logfile, sizeof(logfile), "logs/%s", refname);
+ if (starts_with(refname, "refs/heads/") ||
+ starts_with(refname, "refs/remotes/") ||
+ starts_with(refname, "refs/notes/") ||
+ !strcmp(refname, "HEAD")) {
if (safe_create_leading_directories(logfile) < 0) {
int save_errno = errno;
error("unable to create directory for %s", logfile);
@@ -2900,16 +2900,20 @@ static int log_ref_write_fd(int fd, const unsigned char
*old_sha1,
static int log_ref_write(const char *refname, const unsigned char *old_sha1,
const unsigned char *new_sha1, const char *msg)
{
- int logfd, result, oflags = O_APPEND | O_WRONLY;
+ int logfd, result = 0, oflags = O_APPEND | O_WRONLY;
char log_file[PATH_MAX];
if (log_all_ref_updates < 0)
log_all_ref_updates = !is_bare_repository();
- result = log_ref_setup(refname, log_file, sizeof(log_file));
+ if (log_all_ref_updates && !reflog_exists(refname))
+ result = create_reflog(refname);
+
if (result)
return result;
+ git_snpath(log_file, sizeof(log_file), "logs/%s", refname);
+
logfd = open(log_file, oflags);
if (logfd < 0)
return 0;
diff --git a/refs.h b/refs.h
index 0564955..e7892fc 100644
--- a/refs.h
+++ b/refs.h
@@ -203,11 +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);
[PATCH v3 10/14] refs.c: allow multiple reflog updates during a single transaction
Allow to make multiple reflog updates to the same ref during a transaction.
This means we only need to lock the reflog once, during the first update
that touches the reflog, and that all further updates can just write the
reflog entry since the reflog is already locked.
This allows us to write code such as:
t = transaction_begin()
transaction_reflog_update(t, "foo", REFLOG_TRUNCATE, NULL);
loop-over-somehting...
transaction_reflog_update(t, "foo", 0, );
transaction_commit(t)
where we first truncate the reflog and then build the new content one line at a
time.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 46 +-
1 file changed, 37 insertions(+), 9 deletions(-)
diff --git a/refs.c b/refs.c
index d6df28d..ad60231 100644
--- a/refs.c
+++ b/refs.c
@@ -30,6 +30,12 @@ static unsigned char refname_disposition[256] = {
*/
#define REF_ISPRUNING 0x0100
/*
+ * Only the first reflog update needs to lock the reflog file. Further updates
+ * just use the lock taken by the first update.
+ */
+#define UPDATE_REFLOG_NOLOCK 0x0200
+
+/*
* Try to read one refname component from the front of refname.
* Return the length of the component found, or -1 if the component is
* not legal. It is legal if it is something reasonable to have under
@@ -3391,7 +3397,7 @@ enum transaction_update_type {
UPDATE_LOG = 1,
};
-/**
+/*
* 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
* while locking the ref, set have_old to 1 and set old_sha1 to the
@@ -3401,7 +3407,7 @@ struct ref_update {
enum transaction_update_type update_type;
unsigned char new_sha1[20];
unsigned char old_sha1[20];
- int flags; /* REF_NODEREF? */
+ int flags; /* REF_NODEREF? or private flags */
int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
struct ref_lock *lock;
int type;
@@ -3409,8 +3415,9 @@ struct ref_update {
/* used by reflog updates */
int reflog_fd;
- struct lock_file reflog_lock;
+ struct lock_file *reflog_lock;
char *committer;
+ struct ref_update *orig_update; /* For UPDATE_REFLOG_NOLOCK */
const char refname[FLEX_ARRAY];
};
@@ -3492,12 +3499,27 @@ int transaction_update_reflog(struct ref_transaction
*transaction,
struct strbuf *err)
{
struct ref_update *update;
+ int i;
if (transaction->state != REF_TRANSACTION_OPEN)
die("BUG: update_reflog called for transaction that is not "
"open");
update = add_update(transaction, refname, UPDATE_LOG);
+ update->flags = flags;
+ for (i = 0; i < transaction->nr - 1; i++) {
+ if (transaction->updates[i]->update_type != UPDATE_LOG)
+ continue;
+ if (!strcmp(transaction->updates[i]->refname,
+ update->refname)) {
+ update->flags |= UPDATE_REFLOG_NOLOCK;
+ update->orig_update = transaction->updates[i];
+ break;
+ }
+ }
+ if (!(update->flags & UPDATE_REFLOG_NOLOCK))
+ update->reflog_lock = xcalloc(1, sizeof(struct lock_file));
+
hashcpy(update->new_sha1, new_sha1);
hashcpy(update->old_sha1, old_sha1);
update->reflog_fd = -1;
@@ -3513,7 +3535,6 @@ int transaction_update_reflog(struct ref_transaction
*transaction,
}
if (msg)
update->msg = xstrdup(msg);
- update->flags = flags;
return 0;
}
@@ -3690,10 +3711,15 @@ int transaction_commit(struct ref_transaction
*transaction,
if (update->update_type != UPDATE_LOG)
continue;
+ if (update->flags & UPDATE_REFLOG_NOLOCK) {
+ update->reflog_fd = update->orig_update->reflog_fd;
+ update->reflog_lock = update->orig_update->reflog_lock;
+ continue;
+ }
update->reflog_fd = hold_lock_file_for_append(
- &update->reflog_lock,
+ update->reflog_lock,
git_path("logs/%s", update->refname),
- 0);
+ LOCK_NODEREF);
if (update->reflog_fd < 0) {
const char *str = "Cannot lock reflog for '%s'. %s";
@@ -3759,7 +3785,7 @@ int transaction_commit(struct ref_transaction
*transaction,
ftruncate(update->reflog_fd, 0)) {
error("Could not truncate reflog: %s. %s",
update->refname, strerror(errno));
- rollback_lock_file(&update->reflog_lock);
+
[PATCH v3 09/14] refs.c: only write reflog update if msg is non-NULL
When performing a reflog transaction update, only write to the reflog iff
msg is non-NULL. This can then be combined with REFLOG_TRUNCATE to perform
an update that only truncates but does not write.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 5 +++--
refs.h | 1 +
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index c33d19e..d6df28d 100644
--- a/refs.c
+++ b/refs.c
@@ -3763,8 +3763,9 @@ int transaction_commit(struct ref_transaction
*transaction,
update->reflog_fd = -1;
continue;
}
- if (log_ref_write_fd(update->reflog_fd, update->old_sha1,
-update->new_sha1,
+ if (update->msg &&
+ log_ref_write_fd(update->reflog_fd,
+update->old_sha1, update->new_sha1,
update->committer, update->msg)) {
error("Could write to reflog: %s. %s",
update->refname, strerror(errno));
diff --git a/refs.h b/refs.h
index f14c8db..1d7906c 100644
--- a/refs.h
+++ b/refs.h
@@ -337,6 +337,7 @@ int transaction_delete_sha1(struct ref_transaction
*transaction,
/*
* Append a reflog entry for refname. If the REFLOG_TRUNCATE flag is set
* this update will first truncate the reflog before writing the entry.
+ * If msg is NULL no update will be written to the log.
*/
int transaction_update_reflog(struct ref_transaction *transaction,
const char *refname,
--
2.0.0.467.g08c0633
--
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 v3 03/14] refs.c: rename the transaction functions
Rename the transaction functions. Remove the leading ref_ from the
names and append _sha1 to the names for functions that create/delete/
update sha1 refs.
Signed-off-by: Ronnie Sahlberg
---
branch.c | 11 ---
builtin/commit.c | 14 -
builtin/fetch.c| 12
builtin/receive-pack.c | 14 -
builtin/replace.c | 10 +++---
builtin/tag.c | 10 +++---
builtin/update-ref.c | 22 +++---
fast-import.c | 22 +++---
refs.c | 82 +-
refs.h | 56 +-
sequencer.c| 12
walker.c | 16 +-
12 files changed, 141 insertions(+), 140 deletions(-)
diff --git a/branch.c b/branch.c
index e0439af..6fa6d78 100644
--- a/branch.c
+++ b/branch.c
@@ -298,13 +298,14 @@ void create_branch(const char *head,
struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
- transaction = ref_transaction_begin(&err);
+ transaction = transaction_begin(&err);
if (!transaction ||
- ref_transaction_update(transaction, ref.buf, sha1,
- null_sha1, 0, !forcing, msg, &err) ||
- ref_transaction_commit(transaction, &err))
+ transaction_update_sha1(transaction, ref.buf, sha1,
+ null_sha1, 0, !forcing, msg,
+ &err) ||
+ transaction_commit(transaction, &err))
die("%s", err.buf);
- ref_transaction_free(transaction);
+ transaction_free(transaction);
}
if (real_ref && track)
diff --git a/builtin/commit.c b/builtin/commit.c
index c499826..bf8d85a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1762,17 +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);
- transaction = ref_transaction_begin(&err);
+ transaction = transaction_begin(&err);
if (!transaction ||
- ref_transaction_update(transaction, "HEAD", sha1,
- current_head ?
- current_head->object.sha1 : NULL,
- 0, !!current_head, sb.buf, &err) ||
- ref_transaction_commit(transaction, &err)) {
+ transaction_update_sha1(transaction, "HEAD", sha1,
+ current_head ?
+ current_head->object.sha1 : NULL,
+ 0, !!current_head, sb.buf, &err) ||
+ transaction_commit(transaction, &err)) {
rollback_index_files();
die("%s", err.buf);
}
- ref_transaction_free(transaction);
+ transaction_free(transaction);
unlink(git_path("CHERRY_PICK_HEAD"));
unlink(git_path("REVERT_HEAD"));
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 52f1ebc..baf7929 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -385,22 +385,22 @@ static int s_update_ref(const char *action,
rla = default_rla.buf;
snprintf(msg, sizeof(msg), "%s: %s", rla, action);
- transaction = ref_transaction_begin(&err);
+ transaction = transaction_begin(&err);
if (!transaction ||
- ref_transaction_update(transaction, ref->name, ref->new_sha1,
- ref->old_sha1, 0, check_old, msg, &err))
+ transaction_update_sha1(transaction, ref->name, ref->new_sha1,
+ ref->old_sha1, 0, check_old, msg, &err))
goto fail;
- ret = ref_transaction_commit(transaction, &err);
+ ret = transaction_commit(transaction, &err);
if (ret == UPDATE_REFS_NAME_CONFLICT)
df_conflict = 1;
if (ret)
goto fail;
- ref_transaction_free(transaction);
+ transaction_free(transaction);
return 0;
fail:
- ref_transaction_free(transaction);
+ transaction_free(transaction);
error("%s", err.buf);
strbuf_release(&err);
return df_conflict ? STORE_REF_ERROR_DF_CONFLICT
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 0ed1ddd..dcd1862 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -582,20 +582,20 @@ static char *update(struct command *cmd, struct
shallow_info *si)
update_shallow_ref(cmd, si))
return xstrdup("shallow error");
- transaction = ref_transaction_begin(&err);
+ transaction = transaction_begin(&err);
if (!transaction ||
- ref_transaction
[PATCH v3 13/14] refs.c: make unlock_ref/close_ref/commit_ref static
unlock|close|commit_ref can be made static since there are no more external
callers.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 24
refs.h | 9 -
2 files changed, 12 insertions(+), 21 deletions(-)
diff --git a/refs.c b/refs.c
index 9653a01..ff98682 100644
--- a/refs.c
+++ b/refs.c
@@ -1960,6 +1960,16 @@ int refname_match(const char *abbrev_name, const char
*full_name)
return 0;
}
+static void unlock_ref(struct ref_lock *lock)
+{
+ /* Do not free lock->lk -- atexit() still looks at them */
+ if (lock->lk)
+ rollback_lock_file(lock->lk);
+ free(lock->ref_name);
+ free(lock->orig_ref_name);
+ free(lock);
+}
+
/* 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)
@@ -2769,7 +2779,7 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
return 1;
}
-int close_ref(struct ref_lock *lock)
+static int close_ref(struct ref_lock *lock)
{
if (close_lock_file(lock->lk))
return -1;
@@ -2777,7 +2787,7 @@ int close_ref(struct ref_lock *lock)
return 0;
}
-int commit_ref(struct ref_lock *lock)
+static int commit_ref(struct ref_lock *lock)
{
if (commit_lock_file(lock->lk))
return -1;
@@ -2785,16 +2795,6 @@ int commit_ref(struct ref_lock *lock)
return 0;
}
-void unlock_ref(struct ref_lock *lock)
-{
- /* Do not free lock->lk -- atexit() still looks at them */
- if (lock->lk)
- rollback_lock_file(lock->lk);
- free(lock->ref_name);
- free(lock->orig_ref_name);
- free(lock);
-}
-
/*
* copy the reflog message msg to buf, which has been allocated sufficiently
* large, while cleaning up the whitespaces. Especially, convert LF to space,
diff --git a/refs.h b/refs.h
index e7892fc..5054388 100644
--- a/refs.h
+++ b/refs.h
@@ -194,15 +194,6 @@ extern struct ref_lock *lock_any_ref_for_update(const char
*refname,
const unsigned char *old_sha1,
int flags, int *type_p);
-/** Close the file descriptor owned by a lock and return the status */
-extern int close_ref(struct ref_lock *lock);
-
-/** Close and commit the ref locked by the lock */
-extern int commit_ref(struct ref_lock *lock);
-
-/** Release any lock taken but not written. **/
-extern void unlock_ref(struct ref_lock *lock);
-
/** Reads log for the value of ref during at_time. **/
extern int read_ref_at(const char *refname, unsigned long at_time, int cnt,
unsigned char *sha1, char **msg,
--
2.0.0.467.g08c0633
--
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 v3 04/14] refs.c: add a new update_type field to ref_update
Add a field that describes what type of update this refers to. For now
the only type is UPDATE_SHA1 but we will soon add more types.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 25 +
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index 4e3d4c3..4129de6 100644
--- a/refs.c
+++ b/refs.c
@@ -3374,6 +3374,10 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
return retval;
}
+enum transaction_update_type {
+ UPDATE_SHA1 = 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
@@ -3381,6 +3385,7 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
* value or to zero to ensure the ref does not exist before update.
*/
struct ref_update {
+ enum transaction_update_type update_type;
unsigned char new_sha1[20];
unsigned char old_sha1[20];
int flags; /* REF_NODEREF? */
@@ -3444,12 +3449,14 @@ void transaction_free(struct ref_transaction
*transaction)
}
static struct ref_update *add_update(struct ref_transaction *transaction,
-const char *refname)
+const char *refname,
+enum transaction_update_type update_type)
{
size_t len = strlen(refname);
struct ref_update *update = xcalloc(1, sizeof(*update) + len + 1);
strcpy((char *)update->refname, refname);
+ update->update_type = update_type;
ALLOC_GROW(transaction->updates, transaction->nr + 1,
transaction->alloc);
transaction->updates[transaction->nr++] = update;
return update;
@@ -3470,7 +3477,7 @@ int transaction_update_sha1(struct ref_transaction
*transaction,
if (have_old && !old_sha1)
die("BUG: have_old is true but old_sha1 is NULL");
- update = add_update(transaction, refname);
+ update = add_update(transaction, refname, UPDATE_SHA1);
hashcpy(update->new_sha1, new_sha1);
update->flags = flags;
update->have_old = have_old;
@@ -3555,7 +3562,10 @@ static int ref_update_reject_duplicates(struct
ref_update **updates, int n,
struct strbuf *err)
{
int i;
- for (i = 1; i < n; i++)
+ for (i = 1; i < n; i++) {
+ if (updates[i - 1]->update_type != UPDATE_SHA1 ||
+ updates[i]->update_type != UPDATE_SHA1)
+ continue;
if (!strcmp(updates[i - 1]->refname, updates[i]->refname)) {
const char *str =
"Multiple updates for ref '%s' not allowed.";
@@ -3564,6 +3574,7 @@ static int ref_update_reject_duplicates(struct ref_update
**updates, int n,
return 1;
}
+ }
return 0;
}
@@ -3593,10 +3604,12 @@ int transaction_commit(struct ref_transaction
*transaction,
goto cleanup;
}
- /* Acquire all locks while verifying old values */
+ /* Acquire all ref locks while verifying old values */
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
+ if (update->update_type != UPDATE_SHA1)
+ continue;
update->lock = lock_ref_sha1_basic(update->refname,
(update->have_old ?
update->old_sha1 :
@@ -3619,6 +3632,8 @@ int transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
+ if (update->update_type != UPDATE_SHA1)
+ continue;
if (!is_null_sha1(update->new_sha1)) {
ret = write_ref_sha1(update->lock, update->new_sha1,
update->msg);
@@ -3638,6 +3653,8 @@ int transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
+ if (update->update_type != UPDATE_SHA1)
+ continue;
if (update->lock) {
if (delete_ref_loose(update->lock, update->type, err))
ret = -1;
--
2.0.0.467.g08c0633
--
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
