Re: [PATCH v3 2/5] commit test: Change $PWD to $(pwd)
On Tue, May 27, 2014 at 1:46 AM, Johannes Sixt j.s...@viscovery.net wrote: Am 5/26/2014 20:56, schrieb Caleb Thompson: Signed-off-by: Caleb Thompson ca...@calebthompson.io --- t/t7507-commit-verbose.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh index 6d778ed..3b06d73 100755 --- a/t/t7507-commit-verbose.sh +++ b/t/t7507-commit-verbose.sh @@ -8,7 +8,7 @@ cat check-for-diff EOF exec grep '^diff --git' \$1 EOF chmod +x check-for-diff -test_set_editor $PWD/check-for-diff +test_set_editor $(pwd)/check-for-diff cat message 'EOF' subject Why? I see no benefit. Both $PWD and $(pwd) work fine everywhere, including Windows, and the former is faster, particularly on Windows. Poor advice on my part when reviewing the previous round. When I had read in git/t/README (in the distant past): When a test checks for an absolute path that a git command generated, construct the expected value using $(pwd) rather than $PWD, $TEST_DIRECTORY, or $TRASH_DIRECTORY. It makes a difference on Windows, where the shell (MSYS bash) mangles absolute path names. For details, see the commit message of 4114156ae9. I must have missed the word check in the first sentence. -- To unsubscribe from this list: send the line 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] commit test: Change $PWD to $(pwd)
On Tue, May 27, 2014 at 07:46:59AM +0200, Johannes Sixt wrote: Am 5/26/2014 20:56, schrieb Caleb Thompson: Signed-off-by: Caleb Thompson ca...@calebthompson.io --- t/t7507-commit-verbose.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh index 6d778ed..3b06d73 100755 --- a/t/t7507-commit-verbose.sh +++ b/t/t7507-commit-verbose.sh @@ -8,7 +8,7 @@ cat check-for-diff EOF exec grep '^diff --git' \$1 EOF chmod +x check-for-diff -test_set_editor $PWD/check-for-diff +test_set_editor $(pwd)/check-for-diff cat message 'EOF' subject Why? I see no benefit. Both $PWD and $(pwd) work fine everywhere, including Windows, and the former is faster, particularly on Windows. -- Hannes I don't know the technical details of why this change is needed. But someone felt it was important enough to put in t/README. - When a test checks for an absolute path that a git command generated, construct the expected value using $(pwd) rather than $PWD, $TEST_DIRECTORY, or $TRASH_DIRECTORY. It makes a difference on Windows, where the shell (MSYS bash) mangles absolute path names. For details, see the commit message of 4114156ae9. -- Jeremiah Mahler jmmah...@gmail.com http://github.com/jmahler -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/5] commit test: Change $PWD to $(pwd)
Please do not cull the Cc list. Am 5/27/2014 8:14, schrieb Jeremiah Mahler: On Tue, May 27, 2014 at 07:46:59AM +0200, Johannes Sixt wrote: Am 5/26/2014 20:56, schrieb Caleb Thompson: Signed-off-by: Caleb Thompson ca...@calebthompson.io --- t/t7507-commit-verbose.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh index 6d778ed..3b06d73 100755 --- a/t/t7507-commit-verbose.sh +++ b/t/t7507-commit-verbose.sh @@ -8,7 +8,7 @@ cat check-for-diff EOF exec grep '^diff --git' \$1 EOF chmod +x check-for-diff -test_set_editor $PWD/check-for-diff +test_set_editor $(pwd)/check-for-diff cat message 'EOF' subject Why? I see no benefit. Both $PWD and $(pwd) work fine everywhere, including Windows, and the former is faster, particularly on Windows. I don't know the technical details of why this change is needed. But someone felt it was important enough to put in t/README. - When a test checks for an absolute path that a git command generated, construct the expected value using $(pwd) rather than $PWD, $TEST_DIRECTORY, or $TRASH_DIRECTORY. It makes a difference on Windows, where the shell (MSYS bash) mangles absolute path names. For details, see the commit message of 4114156ae9. That someone was I. I appreciate that people study t/README and do not ignore the sentence. However, it does not apply to the situation because the path to the editor is not generated by a git command and checked for by a test. That said, it is not wrong to use $(pwd) with test_set_editor, it's just unnecessarily slow. -- 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 2/5] commit test: Change $PWD to $(pwd)
Johannes Sixt j.s...@viscovery.net writes: That said, it is not wrong to use $(pwd) with test_set_editor, it's just unnecessarily slow. Any shell that knows $(...) is pretty sure to have pwd as a built-in. I don't think Git will run on those kind of ancient shells reverting to /bin/pwd here. The autoconf manual (info (autoconf) Limitations of Builtins) states 'pwd' With modern shells, plain 'pwd' outputs a logical directory name, some of whose components may be symbolic links. These directory names are in contrast to physical directory names, whose components are all directories. Posix 1003.1-2001 requires that 'pwd' must support the '-L' (logical) and '-P' (physical) options, with '-L' being the default. However, traditional shells do not support these options, and their 'pwd' command has the '-P' behavior. Portable scripts should assume neither option is supported, and should assume neither behavior is the default. Also, on many hosts '/bin/pwd' is equivalent to 'pwd -P', but Posix does not require this behavior and portable scripts should not rely on it. Typically it's best to use plain 'pwd'. On modern hosts this outputs logical directory names, which have the following advantages: * Logical names are what the user specified. * Physical names may not be portable from one installation host to another due to network file system gymnastics. * On modern hosts 'pwd -P' may fail due to lack of permissions to some parent directory, but plain 'pwd' cannot fail for this reason. Also please see the discussion of the 'cd' command. So $PWD is pretty much guaranteed to be the same as $(pwd) and pretty much guaranteed to _not_ be unnecessarily slow when not run in an inner loop. However, looking at (info (autoconf) Special Shell Variables) I see 'PWD' Posix 1003.1-2001 requires that 'cd' and 'pwd' must update the 'PWD' environment variable to point to the logical name of the current directory, but traditional shells do not support this. This can cause confusion if one shell instance maintains 'PWD' but a subsidiary and different shell does not know about 'PWD' and executes 'cd'; in this case 'PWD' points to the wrong directory. Use '`pwd`' rather than '$PWD'. Ok, probably Git relies on Posix 1003.1-2001 in other respects so it's likely not much of an actual issue. -- David Kastrup -- To unsubscribe from this list: send the line 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 00/10] Zsh prompt tests
This series adds test cases for running __git_ps1 (see
contrib/completion/git-prompt.sh) from Zsh.
This series also adds more Bash test cases to test how __git_ps1
reacts to disabling Bash's PS1 parameter expansion. (This is related
to adding Zsh test cases: Zsh doesn't perform parameter expansion on
PS1 by default but many users turn it on, so the Zsh test script must
test __git_ps1 in both states. Bash expands PS1 by default and users
rarely turn it off, but testing both states in Bash improves the
symmetry with the Zsh test cases.)
This is the approach I took:
1. delete the last test case in t9903 (prompt - zsh color pc mode)
2. add two new functions to t/lib-bash.sh:
ps1_expansion_enable () { shopt -s promptvars; }
ps1_expansion_disable () { shopt -u promptvars; }
3. loop over the relevant test cases twice: once after calling
ps1_expansion_enable and once after calling ps1_expansion_disable
(with appropriate adjustments to the expected output)
4. move the test cases in t9903 to a separate library file and
source it from t9903-bash-prompt.sh
5. create two new files:
* t/lib-zsh.sh (same as t/lib-bash.sh but tweaked for zsh)
* t/t9904-zsh-prompt.sh (same as t/t9903-bash-prompt.sh but
tweaked for zsh)
There are a lot of indendation changes, so I recommend examining the
changes via diff -w.
Richard Hansen (10):
t9903: remove Zsh test from the suite of Bash prompt tests
t9903: put the Bash pc mode prompt test cases in a function
t9903: move test name prefix to a separate variable
t9903: run pc mode tests again with PS1 expansion disabled
t9903: include Bash in test names via new $shellname var
t9903: move PS1 color code variable definitions to lib-bash.sh
t9903: move prompt tests to a new lib-prompt-tests.sh file
lib-prompt-tests.sh: put all tests inside a function
lib-prompt-tests.sh: add variable for string that encodes percent in
PS1
t9904: new __git_ps1 tests for Zsh
t/lib-bash.sh | 12 +
t/lib-prompt-tests.sh | 633 +
t/lib-zsh.sh | 30 +++
t/t9903-bash-prompt.sh | 582 +
t/t9904-zsh-prompt.sh | 10 +
5 files changed, 687 insertions(+), 580 deletions(-)
create mode 100644 t/lib-prompt-tests.sh
create mode 100644 t/lib-zsh.sh
create mode 100755 t/t9904-zsh-prompt.sh
--
1.9.3
--
To unsubscribe from this list: send the line 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/10] t9903: remove Zsh test from the suite of Bash prompt tests
This test is about to become redundant: All of the Bash prompt tests
will be moved into a separate library file that will also be used by a
new Zsh-specific test script.
Signed-off-by: Richard Hansen rhan...@bbn.com
---
t/t9903-bash-prompt.sh | 11 ---
1 file changed, 11 deletions(-)
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index 9150984..335383d 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -577,15 +577,4 @@ test_expect_success 'prompt - bash color pc mode -
untracked files status indica
test_cmp expected $actual
'
-test_expect_success 'prompt - zsh color pc mode' '
- printf BEFORE: (%%F{green}master%%f):AFTER expected
- (
- ZSH_VERSION=5.0.0
- GIT_PS1_SHOWCOLORHINTS=y
- __git_ps1 BEFORE: :AFTER
- printf %s $PS1 $actual
- )
- test_cmp expected $actual
-'
-
test_done
--
1.9.3
--
To unsubscribe from this list: send the line 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/10] t9903: run pc mode tests again with PS1 expansion disabled
Bash has a shell option that makes it possible to disable parameter
expansion in PS1. Test __git_ps1's ability to detect and react to
disabled PS1 expansion by running the pc mode tests twice: once
with PS1 parameter expansion enabled and once with it disabled.
Signed-off-by: Richard Hansen rhan...@bbn.com
---
t/lib-bash.sh | 3 ++
t/t9903-bash-prompt.sh | 75 ++
2 files changed, 55 insertions(+), 23 deletions(-)
diff --git a/t/lib-bash.sh b/t/lib-bash.sh
index 2be955f..37a48fd 100644
--- a/t/lib-bash.sh
+++ b/t/lib-bash.sh
@@ -14,4 +14,7 @@ else
exit 0
fi
+ps1_expansion_enable () { shopt -s promptvars; }
+ps1_expansion_disable () { shopt -u promptvars; }
+
. ./test-lib.sh
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index d29dd2b..fbd77e6 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -451,57 +451,81 @@ test_expect_success 'prompt - format string starting with
dash' '
test_cmp expected $actual
'
-run_pcmode_tests () {
- test_expect_success 'prompt - pc mode' '
- printf BEFORE: (\${__git_ps1_branch_name}):AFTER\\nmaster
expected
+pcmode_expected () {
+ case $ps1expansion in
+ on) printf $1 '${__git_ps1_branch_name}' $2;;
+ off) printf $1 $2 ;;
+ esac expected
+}
+
+pcmode_actual () {
+ case $ps1expansion in
+ on) printf %s\\n%s $PS1 ${__git_ps1_branch_name};;
+ off) printf %s\\n $PS1;;
+ esac $actual
+}
+
+_run_pcmode_tests () {
+ ps1expansion=$1; shift
+
+ case $ps1expansion in
+ # if the shell doesn't allow ps1 expansion to be enabled,
+ # quietly skip the tests (same goes for disabling)
+ on) ps1_expansion_enable || return 0;;
+ off) ps1_expansion_disable || return 0;;
+ *) error invalid argument to _run_pcmode_tests: $ps1expansion;;
+ esac
+
+ test_expect_success prompt - pc mode (PS1 expansion $ps1expansion) '
+ pcmode_expected BEFORE: (%s):AFTER\\n%s master
printf expected_output
(
__git_ps1 BEFORE: :AFTER $actual
test_cmp expected_output $actual
- printf %s\\n%s $PS1 ${__git_ps1_branch_name}
$actual
+ pcmode_actual
)
test_cmp expected $actual
'
- pfx=prompt - bash color pc mode
+ pfx=prompt - bash color pc mode (PS1 expansion $ps1expansion)
test_expect_success $pfx - branch name '
- printf BEFORE:
(${c_green}\${__git_ps1_branch_name}${c_clear}):AFTER\\nmaster expected
+ pcmode_expected BEFORE: (${c_green}%s${c_clear}):AFTER\\n%s
master
(
GIT_PS1_SHOWCOLORHINTS=y
__git_ps1 BEFORE: :AFTER $actual
- printf %s\\n%s $PS1 ${__git_ps1_branch_name}
$actual
+ pcmode_actual
)
test_cmp expected $actual
'
test_expect_success $pfx - detached head '
- printf BEFORE:
(${c_red}\${__git_ps1_branch_name}${c_clear}):AFTER\\n(%s...) $(git log -1
--format=%h b1^) expected
+ pcmode_expected BEFORE: (${c_red}%s${c_clear}):AFTER\\n%s
($(git log -1 --format=%h b1^)...)
git checkout b1^
test_when_finished git checkout master
(
GIT_PS1_SHOWCOLORHINTS=y
__git_ps1 BEFORE: :AFTER
- printf %s\\n%s $PS1 ${__git_ps1_branch_name}
$actual
+ pcmode_actual
)
test_cmp expected $actual
'
test_expect_success $pfx - dirty status indicator - dirty worktree '
- printf BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear}
${c_red}*${c_clear}):AFTER\\nmaster expected
+ pcmode_expected BEFORE: (${c_green}%s${c_clear}
${c_red}*${c_clear}):AFTER\\n%s master
echo dirty file
test_when_finished git reset --hard
(
GIT_PS1_SHOWDIRTYSTATE=y
GIT_PS1_SHOWCOLORHINTS=y
__git_ps1 BEFORE: :AFTER
- printf %s\\n%s $PS1 ${__git_ps1_branch_name}
$actual
+ pcmode_actual
)
test_cmp expected $actual
'
test_expect_success $pfx - dirty status indicator - dirty index '
- printf BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear}
${c_green}+${c_clear}):AFTER\\nmaster expected
+ pcmode_expected BEFORE: (${c_green}%s${c_clear}
${c_green}+${c_clear}):AFTER\\n%s master
echo dirty file
test_when_finished git reset --hard
git add -u
@@ -509,13 +533,13 @@ run_pcmode_tests () {
[PATCH 06/10] t9903: move PS1 color code variable definitions to lib-bash.sh
Define a new 'set_ps1_format_vars' function in lib-bash.sh that sets
the c_red, c_green, c_lblue, and c_clear variables. Call this
function from run_pcmode_tests(). This is a step toward moving the
shell prompt tests to a separate library file so that they can be
reused to test prompting in Zsh.
Signed-off-by: Richard Hansen rhan...@bbn.com
---
t/lib-bash.sh | 6 ++
t/t9903-bash-prompt.sh | 5 +
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/t/lib-bash.sh b/t/lib-bash.sh
index a0f4e16..9d428bd 100644
--- a/t/lib-bash.sh
+++ b/t/lib-bash.sh
@@ -18,5 +18,11 @@ shellname=Bash
ps1_expansion_enable () { shopt -s promptvars; }
ps1_expansion_disable () { shopt -u promptvars; }
+set_ps1_format_vars () {
+ c_red='\\[\\e[31m\\]'
+ c_green='\\[\\e[32m\\]'
+ c_lblue='\\[\\e[1;34m\\]'
+ c_clear='\\[\\e[0m\\]'
+}
. ./test-lib.sh
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index 05ff246..ef10e34 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -10,10 +10,7 @@ test_description='test git-specific bash prompt functions'
. $GIT_BUILD_DIR/contrib/completion/git-prompt.sh
actual=$TRASH_DIRECTORY/actual
-c_red='\\[\\e[31m\\]'
-c_green='\\[\\e[32m\\]'
-c_lblue='\\[\\e[1;34m\\]'
-c_clear='\\[\\e[0m\\]'
+set_ps1_format_vars
test_expect_success setup for $shellname prompt tests '
git init otherrepo
--
1.9.3
--
To unsubscribe from this list: send the line 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/10] t9904: new __git_ps1 tests for Zsh
These are the same tests as in t9903, but run in zsh instead of bash.
Signed-off-by: Richard Hansen rhan...@bbn.com
---
t/lib-zsh.sh | 30 ++
t/t9904-zsh-prompt.sh | 10 ++
2 files changed, 40 insertions(+)
create mode 100644 t/lib-zsh.sh
create mode 100755 t/t9904-zsh-prompt.sh
diff --git a/t/lib-zsh.sh b/t/lib-zsh.sh
new file mode 100644
index 000..fa6fcd9
--- /dev/null
+++ b/t/lib-zsh.sh
@@ -0,0 +1,30 @@
+# Shell library sourced instead of ./test-lib.sh by tests that need to
+# run under Zsh; primarily intended for tests of the git-prompt.sh
+# script.
+
+if test -n $ZSH_VERSION test -z $POSIXLY_CORRECT; then
+ true
+elif command -v zsh /dev/null 21; then
+ unset POSIXLY_CORRECT
+ exec zsh $0 $@
+else
+ echo '1..0 #SKIP skipping Zsh-specific tests; zsh not available'
+ exit 0
+fi
+
+# ensure that we are in full-on Zsh mode
+emulate -R zsh || exit 1
+
+shellname=Zsh
+
+ps1_expansion_enable () { setopt PROMPT_SUBST; }
+ps1_expansion_disable () { unsetopt PROMPT_SUBST; }
+set_ps1_format_vars () {
+ percent=''
+ c_red='%%F{red}'
+ c_green='%%F{green}'
+ c_lblue='%%F{blue}'
+ c_clear='%%f'
+}
+
+emulate sh -c '. ./test-lib.sh'
diff --git a/t/t9904-zsh-prompt.sh b/t/t9904-zsh-prompt.sh
new file mode 100755
index 000..a38a3fd
--- /dev/null
+++ b/t/t9904-zsh-prompt.sh
@@ -0,0 +1,10 @@
+#!/bin/sh
+
+test_description='test git-specific Zsh prompt functions'
+
+. ./lib-zsh.sh
+. $TEST_DIRECTORY/lib-prompt-tests.sh
+
+run_prompt_tests
+
+test_done
--
1.9.3
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 09/10] lib-prompt-tests.sh: add variable for string that encodes percent in PS1
To add a literal percent character to a Zsh prompt, the string %% is
used in PS1. Bash and POSIX shells simply use %. To accommodate
this difference, use ${percent} where a percent character is expected
and define the percent variable in the set_ps1_format_vars function.
Signed-off-by: Richard Hansen rhan...@bbn.com
---
t/lib-bash.sh | 1 +
t/lib-prompt-tests.sh | 15 ---
2 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/t/lib-bash.sh b/t/lib-bash.sh
index 9d428bd..8a030ac 100644
--- a/t/lib-bash.sh
+++ b/t/lib-bash.sh
@@ -19,6 +19,7 @@ shellname=Bash
ps1_expansion_enable () { shopt -s promptvars; }
ps1_expansion_disable () { shopt -u promptvars; }
set_ps1_format_vars () {
+ percent='%%'
c_red='\\[\\e[31m\\]'
c_green='\\[\\e[32m\\]'
c_lblue='\\[\\e[1;34m\\]'
diff --git a/t/lib-prompt-tests.sh b/t/lib-prompt-tests.sh
index baa6762..244e765 100644
--- a/t/lib-prompt-tests.sh
+++ b/t/lib-prompt-tests.sh
@@ -12,10 +12,11 @@
# (non-zero) and ps1_expansion_disable should simply return
# non-zero (0)
# 3. define a function named set_ps1_format_vars that sets the
-# variables c_red, c_green, c_lblue, and c_clear to the strings
-# that __git_ps1 uses to add color to the prompt. The values of
-# these variables are used in the first argument to the printf
-# command, so they must be escaped appropriately.
+# variables percent, c_red, c_green, c_lblue, and c_clear to the
+# strings that __git_ps1 uses to add percent characters and color
+# to the prompt. The values of these variables are used in the
+# first argument to the printf command, so they must be escaped
+# appropriately.
# 4. source this library
# 5. invoke the run_prompt_tests function
@@ -403,7 +404,7 @@ newline'
'
test_expect_success $pfx - untracked files status indicator -
untracked files '
- printf (master %%) expected
+ printf (master ${percent}) expected
(
GIT_PS1_SHOWUNTRACKEDFILES=y
__git_ps1 $actual
@@ -442,7 +443,7 @@ newline'
'
test_expect_success $pfx - untracked files status indicator - shell
variable set with config enabled '
- printf (master %%) expected
+ printf (master ${percent}) expected
test_config bash.showUntrackedFiles true
(
GIT_PS1_SHOWUNTRACKEDFILES=y
@@ -611,7 +612,7 @@ _run_pcmode_tests () {
'
test_expect_success $pfx - untracked files status indicator '
- pcmode_expected BEFORE: (${c_green}%s${c_clear}
${c_red}%%${c_clear}):AFTER\\n%s master
+ pcmode_expected BEFORE: (${c_green}%s${c_clear}
${c_red}${percent}${c_clear}):AFTER\\n%s master
(
GIT_PS1_SHOWUNTRACKEDFILES=y
GIT_PS1_SHOWCOLORHINTS=y
--
1.9.3
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 07/10] t9903: move prompt tests to a new lib-prompt-tests.sh file
This is a step toward creating a new test script that runs the same prompt tests as t9903 but with Zsh instead of Bash. Signed-off-by: Richard Hansen rhan...@bbn.com --- t/lib-prompt-tests.sh | 632 + t/t9903-bash-prompt.sh | 605 +- 2 files changed, 633 insertions(+), 604 deletions(-) create mode 100644 t/lib-prompt-tests.sh diff --git a/t/lib-prompt-tests.sh b/t/lib-prompt-tests.sh new file mode 100644 index 000..32c3c08 --- /dev/null +++ b/t/lib-prompt-tests.sh @@ -0,0 +1,632 @@ +# Copyright (c) 2012 SZEDER Gábor + +# To use this library: +# 1. set the variable shellname to the name of the shell (e.g., +# Bash) +# 2. define functions named ps1_expansion_enable and +# ps1_expansion_disable that, upon return, guarantee that the +# shell will and will not (respectively) perform parameter +# expansion on PS1, if supported by the shell. If it is not +# possible to configure the shell to disable (enable) PS1 +# expansion, ps1_expansion_enable should simply return 0 +# (non-zero) and ps1_expansion_disable should simply return +# non-zero (0) +# 3. define a function named set_ps1_format_vars that sets the +# variables c_red, c_green, c_lblue, and c_clear to the strings +# that __git_ps1 uses to add color to the prompt. The values of +# these variables are used in the first argument to the printf +# command, so they must be escaped appropriately. +# 4. source this library + +# sanity checks +[ -n $shellname ] || error shellname must be set to the name of the shell +for i in ps1_expansion_enable ps1_expansion_disable set_ps1_format_vars +do + command -v $i /dev/null 21 || error function $i not defined +done +(ps1_expansion_enable || ps1_expansion_disable) \ + || error either ps1_expansion_enable or ps1_expansion_disable must return true + +. $GIT_BUILD_DIR/contrib/completion/git-prompt.sh + +actual=$TRASH_DIRECTORY/actual +set_ps1_format_vars + +test_expect_success setup for $shellname prompt tests ' + git init otherrepo + echo 1 file + git add file + test_tick + git commit -m initial + git tag -a -m msg1 t1 + git checkout -b b1 + echo 2 file + git commit -m second b1 file + echo 3 file + git commit -m third b1 file + git tag -a -m msg2 t2 + git checkout -b b2 master + echo 0 file + git commit -m second b2 file + echo 00 file + git commit -m another b2 file + echo 000 file + git commit -m yet another b2 file + git checkout master +' + +pfx=$shellname prompt + +test_expect_success $pfx - branch name ' + printf (master) expected + __git_ps1 $actual + test_cmp expected $actual +' + +test_expect_success SYMLINKS $pfx - branch name - symlink symref ' + printf (master) expected + test_when_finished git checkout master + test_config core.preferSymlinkRefs true + git checkout master + __git_ps1 $actual + test_cmp expected $actual +' + +test_expect_success $pfx - unborn branch ' + printf (unborn) expected + git checkout --orphan unborn + test_when_finished git checkout master + __git_ps1 $actual + test_cmp expected $actual +' + +repo_with_newline='repo +with +newline' + +if mkdir $repo_with_newline 2/dev/null +then + test_set_prereq FUNNYNAMES +else + say 'Your filesystem does not allow newlines in filenames.' +fi + +test_expect_success FUNNYNAMES $pfx - with newline in path ' + printf (master) expected + git init $repo_with_newline + test_when_finished rm -rf \$repo_with_newline\ + mkdir $repo_with_newline/subdir + ( + cd $repo_with_newline/subdir + __git_ps1 $actual + ) + test_cmp expected $actual +' + +test_expect_success $pfx - detached head ' + printf ((%s...)) $(git log -1 --format=%h --abbrev=13 b1^) expected + test_config core.abbrev 13 + git checkout b1^ + test_when_finished git checkout master + __git_ps1 $actual + test_cmp expected $actual +' + +test_expect_success $pfx - describe detached head - contains ' + printf ((t2~1)) expected + git checkout b1^ + test_when_finished git checkout master + ( + GIT_PS1_DESCRIBE_STYLE=contains + __git_ps1 $actual + ) + test_cmp expected $actual +' + +test_expect_success $pfx - describe detached head - branch ' + printf ((b1~1)) expected + git checkout b1^ + test_when_finished git checkout master + ( + GIT_PS1_DESCRIBE_STYLE=branch + __git_ps1 $actual + ) + test_cmp expected $actual +' + +test_expect_success $pfx - describe detached head - describe ' + printf ((t1-1-g%s)) $(git log -1
[PATCH 08/10] lib-prompt-tests.sh: put all tests inside a function
Modify lib-prompt-tests.sh so that it does nothing when sourced except
define a function for running the prompt tests (plus some private
helper functions).
Signed-off-by: Richard Hansen rhan...@bbn.com
---
t/lib-prompt-tests.sh | 802 -
t/t9903-bash-prompt.sh | 2 +
2 files changed, 403 insertions(+), 401 deletions(-)
diff --git a/t/lib-prompt-tests.sh b/t/lib-prompt-tests.sh
index 32c3c08..baa6762 100644
--- a/t/lib-prompt-tests.sh
+++ b/t/lib-prompt-tests.sh
@@ -17,6 +17,7 @@
# these variables are used in the first argument to the printf
# command, so they must be escaped appropriately.
# 4. source this library
+# 5. invoke the run_prompt_tests function
# sanity checks
[ -n $shellname ] || error shellname must be set to the name of the shell
@@ -27,448 +28,445 @@ done
(ps1_expansion_enable || ps1_expansion_disable) \
|| error either ps1_expansion_enable or ps1_expansion_disable must
return true
-. $GIT_BUILD_DIR/contrib/completion/git-prompt.sh
+_run_non_pcmode_tests () {
+ test_expect_success setup for $shellname prompt tests '
+ git init otherrepo
+ echo 1 file
+ git add file
+ test_tick
+ git commit -m initial
+ git tag -a -m msg1 t1
+ git checkout -b b1
+ echo 2 file
+ git commit -m second b1 file
+ echo 3 file
+ git commit -m third b1 file
+ git tag -a -m msg2 t2
+ git checkout -b b2 master
+ echo 0 file
+ git commit -m second b2 file
+ echo 00 file
+ git commit -m another b2 file
+ echo 000 file
+ git commit -m yet another b2 file
+ git checkout master
+ '
-actual=$TRASH_DIRECTORY/actual
-set_ps1_format_vars
+ pfx=$shellname prompt
-test_expect_success setup for $shellname prompt tests '
- git init otherrepo
- echo 1 file
- git add file
- test_tick
- git commit -m initial
- git tag -a -m msg1 t1
- git checkout -b b1
- echo 2 file
- git commit -m second b1 file
- echo 3 file
- git commit -m third b1 file
- git tag -a -m msg2 t2
- git checkout -b b2 master
- echo 0 file
- git commit -m second b2 file
- echo 00 file
- git commit -m another b2 file
- echo 000 file
- git commit -m yet another b2 file
- git checkout master
-'
+ test_expect_success $pfx - branch name '
+ printf (master) expected
+ __git_ps1 $actual
+ test_cmp expected $actual
+ '
-pfx=$shellname prompt
+ test_expect_success SYMLINKS $pfx - branch name - symlink symref '
+ printf (master) expected
+ test_when_finished git checkout master
+ test_config core.preferSymlinkRefs true
+ git checkout master
+ __git_ps1 $actual
+ test_cmp expected $actual
+ '
-test_expect_success $pfx - branch name '
- printf (master) expected
- __git_ps1 $actual
- test_cmp expected $actual
-'
+ test_expect_success $pfx - unborn branch '
+ printf (unborn) expected
+ git checkout --orphan unborn
+ test_when_finished git checkout master
+ __git_ps1 $actual
+ test_cmp expected $actual
+ '
-test_expect_success SYMLINKS $pfx - branch name - symlink symref '
- printf (master) expected
- test_when_finished git checkout master
- test_config core.preferSymlinkRefs true
- git checkout master
- __git_ps1 $actual
- test_cmp expected $actual
-'
-
-test_expect_success $pfx - unborn branch '
- printf (unborn) expected
- git checkout --orphan unborn
- test_when_finished git checkout master
- __git_ps1 $actual
- test_cmp expected $actual
-'
-
-repo_with_newline='repo
+ repo_with_newline='repo
with
newline'
-if mkdir $repo_with_newline 2/dev/null
-then
- test_set_prereq FUNNYNAMES
-else
- say 'Your filesystem does not allow newlines in filenames.'
-fi
+ if mkdir $repo_with_newline 2/dev/null
+ then
+ test_set_prereq FUNNYNAMES
+ else
+ say 'Your filesystem does not allow newlines in filenames.'
+ fi
-test_expect_success FUNNYNAMES $pfx - with newline in path '
- printf (master) expected
- git init $repo_with_newline
- test_when_finished rm -rf \$repo_with_newline\
- mkdir $repo_with_newline/subdir
- (
- cd $repo_with_newline/subdir
- __git_ps1 $actual
- )
- test_cmp expected $actual
-'
+ test_expect_success FUNNYNAMES $pfx - with newline in path
[PATCH 05/10] t9903: include Bash in test names via new $shellname var
Define a new 'shellname' variable in lib-bash.sh and use it in the
prompt test names. This is a step toward moving the shell prompt
tests to a separate library file so that they can be reused to test
prompting in Zsh.
Signed-off-by: Richard Hansen rhan...@bbn.com
---
t/lib-bash.sh | 2 ++
t/t9903-bash-prompt.sh | 86 ++
2 files changed, 46 insertions(+), 42 deletions(-)
diff --git a/t/lib-bash.sh b/t/lib-bash.sh
index 37a48fd..a0f4e16 100644
--- a/t/lib-bash.sh
+++ b/t/lib-bash.sh
@@ -14,6 +14,8 @@ else
exit 0
fi
+shellname=Bash
+
ps1_expansion_enable () { shopt -s promptvars; }
ps1_expansion_disable () { shopt -u promptvars; }
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index fbd77e6..05ff246 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -15,7 +15,7 @@ c_green='\\[\\e[32m\\]'
c_lblue='\\[\\e[1;34m\\]'
c_clear='\\[\\e[0m\\]'
-test_expect_success 'setup for prompt tests' '
+test_expect_success setup for $shellname prompt tests '
git init otherrepo
echo 1 file
git add file
@@ -38,13 +38,15 @@ test_expect_success 'setup for prompt tests' '
git checkout master
'
-test_expect_success 'prompt - branch name' '
+pfx=$shellname prompt
+
+test_expect_success $pfx - branch name '
printf (master) expected
__git_ps1 $actual
test_cmp expected $actual
'
-test_expect_success SYMLINKS 'prompt - branch name - symlink symref' '
+test_expect_success SYMLINKS $pfx - branch name - symlink symref '
printf (master) expected
test_when_finished git checkout master
test_config core.preferSymlinkRefs true
@@ -53,7 +55,7 @@ test_expect_success SYMLINKS 'prompt - branch name - symlink
symref' '
test_cmp expected $actual
'
-test_expect_success 'prompt - unborn branch' '
+test_expect_success $pfx - unborn branch '
printf (unborn) expected
git checkout --orphan unborn
test_when_finished git checkout master
@@ -72,7 +74,7 @@ else
say 'Your filesystem does not allow newlines in filenames.'
fi
-test_expect_success FUNNYNAMES 'prompt - with newline in path' '
+test_expect_success FUNNYNAMES $pfx - with newline in path '
printf (master) expected
git init $repo_with_newline
test_when_finished rm -rf \$repo_with_newline\
@@ -84,7 +86,7 @@ test_expect_success FUNNYNAMES 'prompt - with newline in
path' '
test_cmp expected $actual
'
-test_expect_success 'prompt - detached head' '
+test_expect_success $pfx - detached head '
printf ((%s...)) $(git log -1 --format=%h --abbrev=13 b1^)
expected
test_config core.abbrev 13
git checkout b1^
@@ -93,7 +95,7 @@ test_expect_success 'prompt - detached head' '
test_cmp expected $actual
'
-test_expect_success 'prompt - describe detached head - contains' '
+test_expect_success $pfx - describe detached head - contains '
printf ((t2~1)) expected
git checkout b1^
test_when_finished git checkout master
@@ -104,7 +106,7 @@ test_expect_success 'prompt - describe detached head -
contains' '
test_cmp expected $actual
'
-test_expect_success 'prompt - describe detached head - branch' '
+test_expect_success $pfx - describe detached head - branch '
printf ((b1~1)) expected
git checkout b1^
test_when_finished git checkout master
@@ -115,7 +117,7 @@ test_expect_success 'prompt - describe detached head -
branch' '
test_cmp expected $actual
'
-test_expect_success 'prompt - describe detached head - describe' '
+test_expect_success $pfx - describe detached head - describe '
printf ((t1-1-g%s)) $(git log -1 --format=%h b1^) expected
git checkout b1^
test_when_finished git checkout master
@@ -126,7 +128,7 @@ test_expect_success 'prompt - describe detached head -
describe' '
test_cmp expected $actual
'
-test_expect_success 'prompt - describe detached head - default' '
+test_expect_success $pfx - describe detached head - default '
printf ((t2)) expected
git checkout --detach b1
test_when_finished git checkout master
@@ -134,7 +136,7 @@ test_expect_success 'prompt - describe detached head -
default' '
test_cmp expected $actual
'
-test_expect_success 'prompt - inside .git directory' '
+test_expect_success $pfx - inside .git directory '
printf (GIT_DIR!) expected
(
cd .git
@@ -143,7 +145,7 @@ test_expect_success 'prompt - inside .git directory' '
test_cmp expected $actual
'
-test_expect_success 'prompt - deep inside .git directory' '
+test_expect_success $pfx - deep inside .git directory '
printf (GIT_DIR!) expected
(
cd .git/refs/heads
@@ -152,7 +154,7 @@ test_expect_success 'prompt - deep inside .git directory' '
test_cmp expected $actual
'
[PATCH 02/10] t9903: put the Bash pc mode prompt test cases in a function
This is a step toward invoking the same pc mode test cases twice:
once with PS1 parameter expansion enabled and once with it disabled.
Signed-off-by: Richard Hansen rhan...@bbn.com
---
t/t9903-bash-prompt.sh | 236 +
1 file changed, 120 insertions(+), 116 deletions(-)
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index 335383d..c691869 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -451,130 +451,134 @@ test_expect_success 'prompt - format string starting
with dash' '
test_cmp expected $actual
'
-test_expect_success 'prompt - pc mode' '
- printf BEFORE: (\${__git_ps1_branch_name}):AFTER\\nmaster expected
- printf expected_output
- (
- __git_ps1 BEFORE: :AFTER $actual
- test_cmp expected_output $actual
- printf %s\\n%s $PS1 ${__git_ps1_branch_name} $actual
- )
- test_cmp expected $actual
-'
+run_pcmode_tests () {
+ test_expect_success 'prompt - pc mode' '
+ printf BEFORE: (\${__git_ps1_branch_name}):AFTER\\nmaster
expected
+ printf expected_output
+ (
+ __git_ps1 BEFORE: :AFTER $actual
+ test_cmp expected_output $actual
+ printf %s\\n%s $PS1 ${__git_ps1_branch_name}
$actual
+ )
+ test_cmp expected $actual
+ '
-test_expect_success 'prompt - bash color pc mode - branch name' '
- printf BEFORE:
(${c_green}\${__git_ps1_branch_name}${c_clear}):AFTER\\nmaster expected
- (
- GIT_PS1_SHOWCOLORHINTS=y
- __git_ps1 BEFORE: :AFTER $actual
- printf %s\\n%s $PS1 ${__git_ps1_branch_name} $actual
- )
- test_cmp expected $actual
-'
+ test_expect_success 'prompt - bash color pc mode - branch name' '
+ printf BEFORE:
(${c_green}\${__git_ps1_branch_name}${c_clear}):AFTER\\nmaster expected
+ (
+ GIT_PS1_SHOWCOLORHINTS=y
+ __git_ps1 BEFORE: :AFTER $actual
+ printf %s\\n%s $PS1 ${__git_ps1_branch_name}
$actual
+ )
+ test_cmp expected $actual
+ '
-test_expect_success 'prompt - bash color pc mode - detached head' '
- printf BEFORE:
(${c_red}\${__git_ps1_branch_name}${c_clear}):AFTER\\n(%s...) $(git log -1
--format=%h b1^) expected
- git checkout b1^
- test_when_finished git checkout master
- (
- GIT_PS1_SHOWCOLORHINTS=y
- __git_ps1 BEFORE: :AFTER
- printf %s\\n%s $PS1 ${__git_ps1_branch_name} $actual
- )
- test_cmp expected $actual
-'
+ test_expect_success 'prompt - bash color pc mode - detached head' '
+ printf BEFORE:
(${c_red}\${__git_ps1_branch_name}${c_clear}):AFTER\\n(%s...) $(git log -1
--format=%h b1^) expected
+ git checkout b1^
+ test_when_finished git checkout master
+ (
+ GIT_PS1_SHOWCOLORHINTS=y
+ __git_ps1 BEFORE: :AFTER
+ printf %s\\n%s $PS1 ${__git_ps1_branch_name}
$actual
+ )
+ test_cmp expected $actual
+ '
-test_expect_success 'prompt - bash color pc mode - dirty status indicator -
dirty worktree' '
- printf BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear}
${c_red}*${c_clear}):AFTER\\nmaster expected
- echo dirty file
- test_when_finished git reset --hard
- (
- GIT_PS1_SHOWDIRTYSTATE=y
- GIT_PS1_SHOWCOLORHINTS=y
- __git_ps1 BEFORE: :AFTER
- printf %s\\n%s $PS1 ${__git_ps1_branch_name} $actual
- )
- test_cmp expected $actual
-'
+ test_expect_success 'prompt - bash color pc mode - dirty status
indicator - dirty worktree' '
+ printf BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear}
${c_red}*${c_clear}):AFTER\\nmaster expected
+ echo dirty file
+ test_when_finished git reset --hard
+ (
+ GIT_PS1_SHOWDIRTYSTATE=y
+ GIT_PS1_SHOWCOLORHINTS=y
+ __git_ps1 BEFORE: :AFTER
+ printf %s\\n%s $PS1 ${__git_ps1_branch_name}
$actual
+ )
+ test_cmp expected $actual
+ '
-test_expect_success 'prompt - bash color pc mode - dirty status indicator -
dirty index' '
- printf BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear}
${c_green}+${c_clear}):AFTER\\nmaster expected
- echo dirty file
- test_when_finished git reset --hard
- git add -u
- (
- GIT_PS1_SHOWDIRTYSTATE=y
- GIT_PS1_SHOWCOLORHINTS=y
- __git_ps1 BEFORE: :AFTER
- printf %s\\n%s $PS1
[PATCH 03/10] t9903: move test name prefix to a separate variable
This is a step toward reusing the same test cases after disabling PS1
parameter expansion.
Signed-off-by: Richard Hansen rhan...@bbn.com
---
t/t9903-bash-prompt.sh | 20 +++-
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index c691869..d29dd2b 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -463,7 +463,9 @@ run_pcmode_tests () {
test_cmp expected $actual
'
- test_expect_success 'prompt - bash color pc mode - branch name' '
+ pfx=prompt - bash color pc mode
+
+ test_expect_success $pfx - branch name '
printf BEFORE:
(${c_green}\${__git_ps1_branch_name}${c_clear}):AFTER\\nmaster expected
(
GIT_PS1_SHOWCOLORHINTS=y
@@ -473,7 +475,7 @@ run_pcmode_tests () {
test_cmp expected $actual
'
- test_expect_success 'prompt - bash color pc mode - detached head' '
+ test_expect_success $pfx - detached head '
printf BEFORE:
(${c_red}\${__git_ps1_branch_name}${c_clear}):AFTER\\n(%s...) $(git log -1
--format=%h b1^) expected
git checkout b1^
test_when_finished git checkout master
@@ -485,7 +487,7 @@ run_pcmode_tests () {
test_cmp expected $actual
'
- test_expect_success 'prompt - bash color pc mode - dirty status
indicator - dirty worktree' '
+ test_expect_success $pfx - dirty status indicator - dirty worktree '
printf BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear}
${c_red}*${c_clear}):AFTER\\nmaster expected
echo dirty file
test_when_finished git reset --hard
@@ -498,7 +500,7 @@ run_pcmode_tests () {
test_cmp expected $actual
'
- test_expect_success 'prompt - bash color pc mode - dirty status
indicator - dirty index' '
+ test_expect_success $pfx - dirty status indicator - dirty index '
printf BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear}
${c_green}+${c_clear}):AFTER\\nmaster expected
echo dirty file
test_when_finished git reset --hard
@@ -512,7 +514,7 @@ run_pcmode_tests () {
test_cmp expected $actual
'
- test_expect_success 'prompt - bash color pc mode - dirty status
indicator - dirty index and worktree' '
+ test_expect_success $pfx - dirty status indicator - dirty index and
worktree '
printf BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear}
${c_red}*${c_green}+${c_clear}):AFTER\\nmaster expected
echo dirty index file
test_when_finished git reset --hard
@@ -527,7 +529,7 @@ run_pcmode_tests () {
test_cmp expected $actual
'
- test_expect_success 'prompt - bash color pc mode - dirty status
indicator - before root commit' '
+ test_expect_success $pfx - dirty status indicator - before root
commit '
printf BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear}
${c_green}#${c_clear}):AFTER\\nmaster expected
(
GIT_PS1_SHOWDIRTYSTATE=y
@@ -539,7 +541,7 @@ run_pcmode_tests () {
test_cmp expected $actual
'
- test_expect_success 'prompt - bash color pc mode - inside .git
directory' '
+ test_expect_success $pfx - inside .git directory '
printf BEFORE:
(${c_green}\${__git_ps1_branch_name}${c_clear}):AFTER\\nGIT_DIR! expected
echo dirty file
test_when_finished git reset --hard
@@ -553,7 +555,7 @@ run_pcmode_tests () {
test_cmp expected $actual
'
- test_expect_success 'prompt - bash color pc mode - stash status
indicator' '
+ test_expect_success $pfx - stash status indicator '
printf BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear}
${c_lblue}\$${c_clear}):AFTER\\nmaster expected
echo 2 file
git stash
@@ -567,7 +569,7 @@ run_pcmode_tests () {
test_cmp expected $actual
'
- test_expect_success 'prompt - bash color pc mode - untracked files
status indicator' '
+ test_expect_success $pfx - untracked files status indicator '
printf BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear}
${c_red}%%${c_clear}):AFTER\\nmaster expected
(
GIT_PS1_SHOWUNTRACKEDFILES=y
--
1.9.3
--
To unsubscribe from this list: send the line 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 v10 11/12] Documentation: add documentation for 'git interpret-trailers'
tl;dr: This patch series wants to introduce a permanent new Git data format. The current version can write trailers in formats that it is incapable of reading, which I consider broken. I advocate a stricter specification of the format of trailers, at least until we get feedback from users that they need more flexibility. On 05/25/2014 10:37 AM, Christian Couder wrote: From: Michael Haggerty mhag...@alum.mit.edu [...] * A way for users to add information to commits for whatever purpose they want, without having to convince upstream to built support in. Yeah, I agree this is the main purpose of trailers. * A standard format for that information, so that all tools can agree how to read/write trailers without being confused by or breaking trailers that they didn't know about in advance. Yeah, but don't you think this goal can sometimes go against the previous goal? I mean, if some users for their project think that it's better, for example, if they use trailers like Fix #42 instead of Fix: 42, because their bug tracking system supports Fix #42 better, we should let them do what suits them better, even if Git supports them not as well as if they used Fix: 42. The flexibility that comes from offering our users a more-or-less general key/value store already accomplishes the first goal. With that the users *could* store their data as Fix: 42 or Fixes: #42 and satisfy their functional requirements. Giving them the option to use Fix #42 doesn't make *any* new functionality possible. It is pure eye-candy. And it would come at the IMO high cost of making it harder for *everybody* to work with the metadata. It makes the specification more complicated. It makes the code more complicated. It makes the configuration more complicated. It makes it more likely that there will be false positives (text in a commit message that our code recognizes as key/value data even though it was not meant to be). And in my opinion it makes the k/v data itself harder for a human to read because it is not in a uniform format. The only justification I can think of for allowing more flexible formats would be to retroactively support metadata that people already have in their history. Are there famous or important existing metadata formats that are incompatible with Key: Value? More to the point: do you have a concrete reason for wanting to support alternative formats like Fix #42, or is it based more on the feeling that users will want it? Remember, it would be really easy to release v1 of this feature with a strict format, then wait and see if users clamor for more flexibility. We can always add more flexibility later. Whereas if v1 already supports more flexible formats, we pretty much have to support them forever. * A format that is straightforward enough that it can be machine- readable with minimum ambiguity. Yeah, but again this could go against the main purpose of trailers above. No, the users have all the flexibility they need if they can choose their own key/value schema. Allowing alternative formats adds very little. I feel strongly that it would be a bad mistake to leave the specification of trailers so loose that they cannot be machine-readable with a good degree of confidence. Tools that add trailers should be composable. The current scheme is not. For example, suppose one tool wants to add a Fix #42 line and another one wants to add Signed-off-by: git config trailer.fix.key Fix # git config trailer.sign.key Signed-off-by: git config trailer.sign.ifexists doNothing echo subject Signed-off-by: Alice al...@example.com | git interpret-trailers --trailer fix=42 | git interpret-trailers --trailer sign=Bob b...@example.com - output -- subject Signed-off-by: Alice al...@example.com Fix #42 Signed-off-by: Bob b...@example.com --- The result is that the trailers end up not in one block but in two (meaning that the first block is no longer recognized as a trailer block), and the second Signed-off-by line, which should have been omitted because of ifexists=doNothing, was incorrectly added. Or let's do something like the commit template example from the documentation, but using Fix # instead of Fixes: : echo ***subject*** ***message*** Fix # Cc: Reviewed-by: Signed-off-by: | sed -Ee 's/(Reviewed-by.*)/\1me/' | git interpret-trailers --trim-empty --trailer git-version: foo -- output -- ***subject*** ***message*** Fix # Cc: Reviewed-by: me Signed-off-by: git-version: foo Not only haven't the empty lines been stripped off, but a new trailer block has been created for git-version. I consider this broken. [...] For
[PATCH] config: respect '~' and '~user' in mailmap.file
git_config_string() does not handle '~' and '~user' as part of the
value. Using git_config_pathname() fixes this.
Signed-off-by: Øystein Walle oys...@gmail.com
---
config.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/config.c b/config.c
index 314d8ee..ec7af5f 100644
--- a/config.c
+++ b/config.c
@@ -963,7 +963,7 @@ static int git_default_push_config(const char *var, const
char *value)
static int git_default_mailmap_config(const char *var, const char *value)
{
if (!strcmp(var, mailmap.file))
- return git_config_string(git_mailmap_file, var, value);
+ return git_config_pathname(git_mailmap_file, var, value);
if (!strcmp(var, mailmap.blob))
return git_config_string(git_mailmap_blob, var, value);
--
2.0.0.rc4
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Kedves felhasználók e-mailben;
-- Kedves felhasználók e-mailben; Túllépte 23432 box set Web Service / Admin, és akkor nem lesz probléma a küldő és fogadhat e-maileket, amíg újra ellenőrizni. Kérjük, frissítse kattintva linkre, és töltse ki az adatokat, hogy ellenőrizze a számla Kérjük, kövesse az alábbi linkre, és majd másolja és illessze be a böngésző jelölőnégyzetet. http://updattwsd451.jigsy.com/ Figyelem! Ha nem, csak korlátozott hozzáférést az e-mail postafiókját. ha frissíteni? számla frissül három napon belül Értesítés a számla véglegesen be kell zárni. Tisztelettel, rendszergazda -- To unsubscribe from this list: send the line 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 v10 11/12] Documentation: add documentation for 'git interpret-trailers'
On Tue, May 27, 2014 at 10:21 AM, Michael Haggerty mhag...@alum.mit.edu wrote: tl;dr: This patch series wants to introduce a permanent new Git data format. The current version can write trailers in formats that it is incapable of reading, which I consider broken. I advocate a stricter specification of the format of trailers, at least until we get feedback from users that they need more flexibility. On 05/25/2014 10:37 AM, Christian Couder wrote: [...] My opinion is that many Git developers have been engaged and you can see that in the Cc. I cannot tell if they are all very happy or not but I suppose that if they were very unhappy they would tell it. [...] It was unfair of me to try to characterize the opinions of other developers. On the other hand, even though many people have commented on this proposal over its long lifetime, I didn't get the feeling that it has won a consensus of +1s in its current form. I'd love to hear the opinion of others because maybe I'm totally out in left field. FWIW, after a quick read, I find myself agreeing very much with Michael's arguments for a stricter format (at least in its initial version). We are formalizing and applying tools/automation to a part of the commit message that has so far been ad hoc and very informal. There is no expectation that _every_ _single_ existing use of (informal) trailers (except the somewhat-formalized support for --signoff) must be supported by git-interpret-trailers. However, there _is_ an expectation that git-interpret-trailers is self-consistent and does not stumble over its own trailers. Therefore, it makes perfect sense to make v1 very strict in what formats it produce (i.e. a strict key: value format is enough for now). And I want to reiterate that the reason I'm so emphatic about this proposal is because I think it will be such a great new feature. I just think that some tweaks would make it a more solid foundation for building even more functionality onto. Fully agreed. git-interpret-trailers have come up in several other discussion, both on the git mailing list and elsewhere, and I have no doubt that this will be a very useful feature that will be put to good use in many projects. ...Johan -- Johan Herland, jo...@herland.net www.herland.net -- To unsubscribe from this list: send the line 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 01/15] builtin/add.c: rearrange xcalloc arguments
Oomph, how embarrassing. Thanks for pointing that out! Would it be better if I rerolled the patches? - Brian Gesiak On Tue, May 27, 2014 at 12:25 PM, Eric Sunshine sunsh...@sunshineco.com wrote: On Mon, May 26, 2014 at 11:33 AM, Brian Gesiak modoca...@gmail.com wrote: xcalloc takes two arguments: the number of elements and their size. run_add_interactive passes the arguments in reverse order, passing the size of a char*, followed by the number of char* to be allocated. Rearrgange them so they are in the correct order. s/Rearrgange/Rearrange/ Same misspelling afflicts the entire patch series. Signed-off-by: Brian Gesiak modoca...@gmail.com --- builtin/add.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/add.c b/builtin/add.c index 672adc0..488acf4 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -248,7 +248,7 @@ int run_add_interactive(const char *revision, const char *patch_mode, int status, ac, i; const char **args; - args = xcalloc(sizeof(const char *), (pathspec-nr + 6)); + args = xcalloc((pathspec-nr + 6), sizeof(const char *)); ac = 0; args[ac++] = add--interactive; if (patch_mode) -- 2.0.0.rc1.543.gc8042da -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Best practices/conventions for tags and references in commit message
Hi, different projects or tools have conventions to include machine parsable information in commit messages, e.g.: Closes: #42 Thanks: my mother, my wife Git-Dch: Ignore Commit-Id: 50M3R34LLYR4ND0MB1TSANDNUMB3R5 (see thread: RFE: support change-id generation natively for Commit-Id) (Git-Dch: Ignore ecludes the commit from the changelog) Are you aware of any convention or best practices for such tags that are used in more than one project? Are there more tags like these? Maybe it would also be helpful to have some more plumbing support from Git for such tags. But I've not yet thought enough about this. Best regards, Thomas Koch -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 2/3] rebase -i: Reschedule tasks that failed before the index was touched
Hi,
Overall, this approach seems reasonable.
Please see the inline comments below.
On 05/27/2014 12:19 AM, Fabian Ruch wrote:
When `rebase--interactive` processes a task, it removes the item from
the todo list and appends it to another list of executed tasks. If a
`pick` (this includes `squash` and `fixup`) fails before the index has
recorded the changes, take the corresponding item and put it on the todo
list again. Otherwise, the changes introduced by the scheduled commit
would be lost.
That kind of decision is possible since the `cherry-pick` command
signals why it failed to apply the changes of the given commit. Either
the changes are recorded in the index using a conflict (return value 1)
and `rebase` does not continue until they are resolved or the changes
are not recorded in the index (return value neither 0 nor 1) and
`rebase` has to try again with the same task.
Reported-by: Phil Hord ho...@cisco.com
Signed-off-by: Fabian Ruch baf...@gmail.com
---
git-rebase--interactive.sh | 27 ++-
1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 9e1dd1e..bba4f3a 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -132,6 +132,16 @@ mark_action_done () {
fi
}
+# Put the last action marked done at the beginning of the todo list
+# again. If there has not been an action marked done yet, the list of
+# items on the todo list is left unchanged.
The comment would read better if the second sentence were also in active
voice, like the first sentence:
If there has not been an action marked done yet, leave the list of
items on the todo list unchanged.
+reschedule_last_action () {
+ tail -n 1 $done | cat - $todo $todo.new
+ sed -e \$d $done $done.new
+ mv -f $todo.new $todo
+ mv -f $done.new $done
+}
+
append_todo_help () {
git stripspace --comment-lines $todo \EOF
@@ -470,11 +480,15 @@ do_pick () {
--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 ||
- die_with_patch $1 Could not apply $1... $2
+--amend --no-post-rewrite -n -q -C $1
git cherry-pick indicates its error status specifically as 1 or some
other value. But here it could be that pick_one succeeds but git
commit fails; in that case ret is set to the return code of git
commit. So, if git commit fails with a retcode different than 1,
then reschedule_last_action will be called a few lines later. This
seems incorrect to me.
else
- pick_one $1 ||
- die_with_patch $1 Could not apply $1... $2
+ pick_one $1
+ fi
+ ret=$?
+ if test $ret -ne 0
+ then
+ test $ret -ne 1 reschedule_last_action
+ die_with_patch $1 Could not apply $1... $2
fi
}
@@ -533,8 +547,11 @@ 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
+ pick_one -n $sha1
+ ret=$?
+ if test $ret -ne 0
then
+ test $ret -ne 1 reschedule_last_action
git rev-parse --verify HEAD $amend
die_failed_squash $sha1 $rest
fi
I suggest that you add a comment for pick_one explaining that if it
fails, its failure code is like that of cherry-pick, namely ...etc...
This will warn future developers to preserve the error code semantics.
It is preferable to squash the next commit, containing the tests,
together with this commit.
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
gitk's truncated tags
Hi, is there any way to display full tag names in gitk, for tag names longer than 16 characters? The way 1.8.x did things? Thanks, Ondrej -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 3/3] tests: Add 'rebase -i commits that overwrite untracked files'
On 05/27/2014 12:19 AM, Fabian Ruch wrote: If a todo list will cherry-pick a commit that adds some file and the working tree already contains a file with the same name, the rebase sequence for that todo list will be interrupted and the cherry-picked commit will be lost after the rebasing process is resumed. This is fixed. Add as a test case for regression testing to the rebase-interactive test suite. The tests look fine. (I assume you tested the tests against the pre-fixed version of the software to make sure that they caught the problem that you fixed.) As I mentioned in patch 2/3, I think it would be better to add the tests in the same commit where you fix the problem. The commit message is, I think, confusing because the first paragraph is in future tense even though it is describing a problem that was just fixed. It will probably change completely when you squash this with the previous commit, but for future reference, I would have suggested something more like t3404: rebase -i retries pick when blocked by untracked file If a commit that is being picked adds a file that already exists as an untracked file in the working tree, cherry-pick fails before even trying to apply the change. rebase --interactive didn't distinguish this error from a conflict, and when rebase --continue was run it thought that the user had just resolved and committed the conflict. The result was that the commit was omitted entirely from the rebased series. This problem was fixed in the previous commit. Add tests. Reported-by: Phil Hord ho...@cisco.com Signed-off-by: Fabian Ruch baf...@gmail.com --- t/t3404-rebase-interactive.sh | 44 +++ 1 file changed, 44 insertions(+) diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 50e22b1..7f5ac18 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1074,4 +1074,48 @@ test_expect_success 'short SHA-1 collide' ' ) ' +test_expect_success 'rebase -i commits that overwrite untracked files (pick)' ' + git checkout branch2 + set_fake_editor + FAKE_LINES=edit 1 2 git rebase -i A + test_cmp_rev HEAD F + test_path_is_missing file6 + touch file6 + test_must_fail git rebase --continue + test_cmp_rev HEAD F + rm file6 + git rebase --continue + test_cmp_rev HEAD I +' + +test_expect_success 'rebase -i commits that overwrite untracked files (squash)' ' + git checkout branch2 + git tag original-branch2 It might be easier (and better decoupled from other tests) to git checkout -b squash-overwrite branch2 rather than moving branch2 then resetting it. That way you can also leave the branch in the repo when you are done rather than having to clean it up. + set_fake_editor + FAKE_LINES=edit 1 squash 2 git rebase -i A + test_cmp_rev HEAD F + test_path_is_missing file6 + touch file6 + test_must_fail git rebase --continue + test_cmp_rev HEAD F + rm file6 + git rebase --continue + test $(git cat-file commit HEAD | sed -ne \$p) = I + git reset --hard original-branch2 +' + +test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' ' + git checkout branch2 + set_fake_editor + FAKE_LINES=edit 1 2 git rebase -i --no-ff A + test $(git cat-file commit HEAD | sed -ne \$p) = F + test_path_is_missing file6 + touch file6 + test_must_fail git rebase --continue + test $(git cat-file commit HEAD | sed -ne \$p) = F + rm file6 + git rebase --continue + test $(git cat-file commit HEAD | sed -ne \$p) = I +' + test_done 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: Best practices/conventions for tags and references in commit message
Search the mailing list archives for git-interpret-trailers. It's coming. :) ...Johan On Tue, May 27, 2014 at 1:26 PM, Thomas Koch tho...@koch.ro wrote: Hi, different projects or tools have conventions to include machine parsable information in commit messages, e.g.: Closes: #42 Thanks: my mother, my wife Git-Dch: Ignore Commit-Id: 50M3R34LLYR4ND0MB1TSANDNUMB3R5 (see thread: RFE: support change-id generation natively for Commit-Id) (Git-Dch: Ignore ecludes the commit from the changelog) Are you aware of any convention or best practices for such tags that are used in more than one project? Are there more tags like these? Maybe it would also be helpful to have some more plumbing support from Git for such tags. But I've not yet thought enough about this. Best regards, Thomas Koch -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- Johan Herland, jo...@herland.net www.herland.net -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 1/2] config: Add cache for config value querying
Hi,
On 05/26/2014 01:02 PM, Torsten Bögershausen wrote:
Add an internal cache with the all variable value pairs read from the usual
cache: The word cache is in Git often used for index
Okay, point noted. I thought about choosing between hashmap and cache and
chose
the later.
variable value can be written as key value
I had used the term variable to be consistent with the documentation
(api-config.txt). But I think key is much clearer.
usual: I don't think we handle unusual config files,
(so can we drop the word usual ?)
Okay, noted.
I think the (important) first line can be written like this:
Add a hash table with the all key-value pairs read from the
or
Add a hash table to cache all key-value pairs read from the
config files(repo specific .git/config, user wide ~/.gitconfig and the global
/etc/gitconfig). Also, add two external functions `git_config_get_string` and
Can we drop Also ?
@@ -37,6 +39,102 @@ static struct config_source *cf;
static int zlib_compression_seen;
+struct hashmap config_cache;
+
+struct config_cache_node {
+struct hashmap_entry ent;
+struct strbuf key;
Do we need a whole strbuf for the key?
Or could a char *key work as well?
(and/or could it be const char *key ?
To maintain consistency with config.c. config.c uses strbuf for both key and
value
throughout. I found the reason by git-blaming config.c. Key length is flexible
so it
would be better to use a api construct such as strbuf for it.
+struct string_list *value_list ;
+static struct string_list *config_cache_get_value(const char *key)
+{
+struct config_cache_node *e = config_cache_find_entry(key);
why e ? Will node be easier to read ? Or entry ?
Noted. Entry is much better.
+static void config_cache_set_value(const char *key, const char *value)
+{
+struct config_cache_node *e;
+
+if (!value)
+return;
Hm, either NULL could mean unset==remove the value, (but we don't do that,
do we?
Or it could mean a programming or runtime error?, Should there be a warning ?
Nope. It is just a check to not save blank values for a key in the hashmap.
Removal
functionality will come later. NULL==remove is implemented in
git_config_set_multivar_in_file(). We are not reading key value pairs from
that, just
from git_config().
+
+e = config_cache_find_entry(key);
+if (!e) {
+e = malloc(sizeof(*e));
+hashmap_entry_init(e, strihash(key));
+strbuf_init((e-key), 1024);
+strbuf_addstr((e-key),key);
+e-value_list = malloc(sizeof(struct string_list));
+e-value_list-strdup_strings = 1;
+e-value_list-nr = 0;
+e-value_list-alloc = 0;
+e-value_list-items = NULL;
+e-value_list-cmp = NULL;
When malloc() is replaced by xcalloc() the x = NULL and y = 0 can go away,
and the code is shorter and easier to read.
Much better, thanks.
+extern const char *git_config_get_string(const char *name)
+{
+struct string_list *values;
+int num_values;
+char *result;
+values = config_cache_get_value(name);
+if (!values)
+return NULL;
+num_values = values-nr;
+result = values-items[num_values-1].string ;
We could get rid of the variable int num_values by simply writing
result = values-items[values-nr-1].string;
Noted.
Cheers,
Tanay Abhra.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Help with subtree pull / push
Hi I am new to git, trying to see if I can use it at my work place to replace Starteam. There is only one Starteam feature that I miss, comparing it to git: shared files. Most projects, here, use lot of shared files, and Starteam handle these pretty easily. You just check in (commit + push in git terms) these files in Starteam central repo and every project that share them see these as out-of-date (so you know you have to check out (pull)). Very handy. For every other aspect (not least ... ehm .. the price) Git seems better to me: it handles much better moving and creating files, and I like the distributed and tree-oriented approach better. But shared files are a problem. After lot of google search I came to the idea of using git subtree, to achieve the same results of Starteam shared files. So I have created 4 remote bare repositories, 1 for the main app and 3 for the shared files parts. After making some changes in shared files though I got in trouble: subtree push keeps give me this error: error: failed to push some refs to '//vm2003test/OuvertureWebShared/Ouverture' hint: Updates were rejected because the tip of your current branch is behind hint: its remote counterpart. Merge the remote changes (e.g. 'git pull') hint: before pushing again. hint: See the 'Note about fast-forwards' in 'git push --help' for details. Obviously I have already made the pull command (as suggested in the message) and it says to me that everything is up to date. On SO there is the very same question (http://stackoverflow.com/questions/13756055/git-subtree-subtree-up-to-date-but-cant-push) but I was unable to try the proposed solution since my git client (GitExtensions on windows) does not support apparently the grave accent. Moreover I don't actually understand that workaround. Thanks for any help Bye Nicola -- View this message in context: http://git.661346.n2.nabble.com/Help-with-subtree-pull-push-tp7611751.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
[PATCH/RFC] clean: add a flag to back up cleaned files
The combination of git clean and fat fingers can some times cause
data-loss, which can be frustrating.
So let's add a flag that imports the files to be deleted into the
object-database, in a way similar to what git-stash does. Maintain
a reflog of the previously backed up clean-runs.
Signed-off-by: Erik Faye-Lund kusmab...@gmail.com
---
I've had a similar patch laying around for quite a while, but since
f538a91 (git-clean: Display more accurate delete messages), this
patch is a lot less nasty than before. So here you go, perhaps
someone else has fat fingers and hate to lose work?
Documentation/config.txt| 4 ++
Documentation/git-clean.txt | 4 ++
builtin/clean.c | 111 +++-
t/t7300-clean.sh| 12 +
4 files changed, 130 insertions(+), 1 deletion(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1932e9b..d58fe31 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -797,6 +797,10 @@ clean.requireForce::
A boolean to make git-clean do nothing unless given -f,
-i or -n. Defaults to true.
+clean.backup::
+ A boolean to make git-clean back up files before they are
+ deleted. Defaults to false.
+
color.branch::
A boolean to enable/disable color in the output of
linkgit:git-branch[1]. May be set to `always`,
diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
index 8997922..bc9d703 100644
--- a/Documentation/git-clean.txt
+++ b/Documentation/git-clean.txt
@@ -41,6 +41,10 @@ OPTIONS
--interactive::
Show what would be done and clean files interactively. See
``Interactive mode'' for details.
+-b::
+--backup::
+ Back up files to a reflog before deleting them. The tree of
+ backed up files are stored in the reflog for refs/clean-backup.
-n::
--dry-run::
diff --git a/builtin/clean.c b/builtin/clean.c
index 9a91515..96fb4b2 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -16,9 +16,12 @@
#include column.h
#include color.h
#include pathspec.h
+#include tree-walk.h
+#include unpack-trees.h
+#include cache-tree.h
static int force = -1; /* unset */
-static int interactive;
+static int interactive, backup;
static struct string_list del_list = STRING_LIST_INIT_DUP;
static unsigned int colopts;
@@ -120,6 +123,11 @@ static int git_clean_config(const char *var, const char
*value, void *cb)
return 0;
}
+ if (!strcmp(var, clean.backup)) {
+ backup = git_config_bool(var, value);
+ return 0;
+ }
+
if (!strcmp(var, clean.requireforce)) {
force = !git_config_bool(var, value);
return 0;
@@ -148,6 +156,93 @@ static int exclude_cb(const struct option *opt, const char
*arg, int unset)
return 0;
}
+static int backed_up_anything;
+
+static void backup_file(const char *path, struct stat *st)
+{
+ if (add_to_cache(path, st, 0))
+ die(_(backing up '%s' failed), path);
+ backed_up_anything = 1;
+}
+
+static struct commit_list *parents;
+
+static void prepare_backup(void)
+{
+ struct unpack_trees_options opts;
+ unsigned char sha1[20];
+ struct tree *tree;
+ struct commit *parent;
+ struct tree_desc t;
+
+ if (get_sha1(HEAD, sha1))
+ die(_(You do not have the initial commit yet));
+
+ /* prepare parent-list */
+ parent = lookup_commit_or_die(sha1, HEAD);
+ commit_list_insert(parent, parents);
+
+ /* load HEAD into the index */
+
+ tree = parse_tree_indirect(sha1);
+ if (!tree)
+ die(_(Failed to unpack tree object %s), sha1);
+
+ parse_tree(tree);
+ init_tree_desc(t, tree-buffer, tree-size);
+
+ memset(opts, 0, sizeof(opts));
+ opts.head_idx = -1;
+ opts.src_index = the_index;
+ opts.dst_index = the_index;
+ opts.index_only = 1;
+
+ if (unpack_trees(1, t, opts)) {
+ /* We've already reported the error, finish dying */
+ exit(128);
+ }
+}
+
+static void finish_backup(void)
+{
+ const char *ref = refs/clean-backup;
+ unsigned char commit_sha1[20];
+ struct strbuf msg = STRBUF_INIT;
+ char logfile[PATH_MAX];
+ struct stat st;
+
+ if (!backed_up_anything)
+ return;
+
+ if (!active_cache_tree)
+ active_cache_tree = cache_tree();
+
+ if (!cache_tree_fully_valid(active_cache_tree)) {
+ if (cache_tree_update(active_cache_tree,
+ (const struct cache_entry * const *)active_cache,
+ active_nr, 0) 0)
+ die(failed to update cache);
+ }
+
+ strbuf_addstr(msg, Automatically committed by git-clean);
+
+ /* create a reflog, if there isn't one */
+ git_snpath(logfile, sizeof(logfile), logs/%s, ref);
+ if (stat(logfile, st)) {
+
Re: [PATCH/RFC] Gitweb: Convert UTF-8 encoded file names
W dniu 2014-05-16 19:05, Junio C Hamano pisze:
Jakub Narębski jna...@gmail.com writes:
Correct, but is where does it appear the question we are
primarily interested in, wrt this breakage and its fix?
That of course depends on how we want to test gitweb output.
The simplest solution, comparing with known output with perhaps
fragile / variable elements masked out could be done quickly...
but changes in output (even if they don't change functionality,
or don't change visible output) require regenerating test cases
(expected output) to test against - which might be source of
errors in test suite.
I agree with your to test it fully, we need extra dependencies,
but my point is that it does not have to be a full HTML-validating,
picking the expected attribute via XPATH matching kind of test if
what we want is only to add a new test to protect this particular
fix from future breakages.
For example, I think it is sufficient to grep for 'href=...%xx%xx'
in the output after preparing a sample tree with one entry to show.
The expected substring either exists (in which case we got it
right), or it doesn't (in which case we are showing garbage). Of
course that depends on the assumption that its output is not too
heavily contaminated with volatile parts outside our control, as I
already mentioned in the message you are responding to.
But it all depends on if we wanted to add a new test ;-)
I tried to add such simple test to t9502, but instead of tests
failing with current version, the test setup fails but succeeds
(i.e. test library says that it failed, but manual examination
shows that everything is O.K.).
-- 8 --
From: Jakub Narebski jna...@gmail.com
Subject: [PATCH/RFC] gitweb test: Test proper encoding of non US-ASCII
filenames in output (WIP)
This t9502 test is intended to test for proper encoding of non
US-ASCII filenames (i.e. UTF-8 filenames) in generated links (which
need some form of URI encoding) and in generated HTML (which needs
HTML encoding / escaping).
For now it tests only 'tree' view (though incidentally it also tests
UTF-8 in commit subject), as this was the action where reportedly
there was bug in link encoding: $t{'name'} coming from the
git ls-tree -z ... command via @ntries array was not marked as
UTF-8, making Perl assume that it is in internal Perl format
i.e. iso-8859-1 encoding and URI-escaping it as if it was in
iso-8859-1 encoding (e.g. Gütekriterien.txt in UTF-8 is
Gütekriterien.txt if treated as iso-8859-1, and it then
encodes to G%C3%83%C2%BCtekriterien.txt instead of correct
G%C3%BCtekriterien.txt).
UNFORTUNATELY test does not fail as it should, even though the issue
was not fixed... OTOH it fails in setup though it is successful.
Reported-by: Michael Wagner accou...@mwagner.org
Signed-off-by: Jakub Narębski jna...@gmail.com
---
t/t9502-gitweb-standalone-parse-output.sh | 34 +
1 files changed, 34 insertions(+), 0 deletions(-)
diff --git a/t/t9502-gitweb-standalone-parse-output.sh
b/t/t9502-gitweb-standalone-parse-output.sh
index 86dfee2..37246a3 100755
--- a/t/t9502-gitweb-standalone-parse-output.sh
+++ b/t/t9502-gitweb-standalone-parse-output.sh
@@ -201,4 +201,38 @@ test_expect_success 'xss checks' '
xss a=rssp=foo.gitf=$TAG
'
+link_check () {
+ grep -F %3C__%C2%A3%C3%A5%C3%AB%C3%AE%C3%B1%C3%B2%C3%BB%C3%BD%C2%B6
\
+ gitweb.body
+ ! grep -F %3C__%A3%E5%EB%EE%F1%F2%FB%FD%B6 \
+ gitweb.body
+}
+
+test_expect_success 'prepare UTF-8 output tests' '
+ FILENAME=__£åëîñòûý¶ +;?__
+ test_commit Adding $FILENAME $FILENAME $FILENAME contents
+'
+
+test_expect_success 'check URI-escaped UTF-8 filename in query-params link' '
+ cat gitweb_config.perl -\EOF
+ $feature{pathinfo}{default} = [0];
+ EOF
+ gitweb_run p=.git;a=tree
+ link_check
+'
+
+test_expect_success 'check URI-escaped UTF-8 filename in path_info link' '
+ cat gitweb_config.perl -\EOF
+ $feature{pathinfo}{default} = [1];
+ EOF
+ gitweb_run /.git/tree
+ link_check
+'
+
+test_expect_success 'check HTML-escaped UTF-8 filename in body' '
+ gitweb_run p=.git;a=tree
+ grep -F lt;__£åëîñòûý¶ +;?amp;__gt; gitweb.body
+ ! grep -F __£åëîñòûý¶ +;?__ gitweb.body
+'
+
test_done
--
1.7.1
--
To unsubscribe from this list: send the line 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] gitweb: Harden UTF-8 handling in generated links
W dniu 2014-05-15 21:28, Jakub Narębski pisze:
On Thu, May 15, 2014 at 8:48 PM, Michael Wagner accou...@mwagner.org wrote:
On Thu, May 15, 2014 at 10:04:24AM +0100, Peter Krefting wrote:
Michael Wagner:
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a9f57d6..f1414e1 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -7138,7 +7138,7 @@ sub git_tree {
my @entries = ();
{
local $/ = \0;
- open my $fd, -|, git_cmd(), ls-tree, '-z',
+ open my $fd, -|encoding(UTF-8), git_cmd(), ls-tree, '-z',
($show_sizes ? '-l' : ()), @extra_options, $hash
or die_error(500, Open git-ls-tree failed);
Or put
binmode $fd, ':utf8';
like in the rest of the code.
@entries = map { chomp; $_ } $fd;
Even better solution would be to use
use open IN = ':encoding(utf-8)';
at the beginning of gitweb.perl, once and for all.
Or harden esc_param / esc_path_info the same way esc_html
is hardened against missing ':utf8' flag.
-- 8 --
Subject: [PATCH] gitweb: Harden UTF-8 handling in generated links
esc_html() ensures that its input is properly UTF-8 encoded and marked
as UTF-8 with to_utf8(). Make esc_param() (used for query parameters
in generated URLs), esc_path_info() (for escaping path_info
components) and esc_url() use it too.
This hardens gitweb against errors in UTF-8 handling; because
to_utf8() is idempotent it won't change correct output.
Reported-by: Michael Wagner accou...@mwagner.org
Signed-off-by: Jakub Narębski jna...@gmail.com
---
gitweb/gitweb.perl |7 +++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a9f57d6..77e1312 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1548,8 +1548,11 @@ sub to_utf8 {
sub esc_param {
my $str = shift;
return undef unless defined $str;
+
+ $str = to_utf8($str);
$str =~ s/([^A-Za-z0-9\-_.~()\/:@ ]+)/CGI::escape($1)/eg;
$str =~ s/ /\+/g;
+
return $str;
}
@@ -1558,6 +1561,7 @@ sub esc_path_info {
my $str = shift;
return undef unless defined $str;
+ $str = to_utf8($str);
# path_info doesn't treat '+' as space (specially), but '?' must be
escaped
$str =~ s/([^A-Za-z0-9\-_.~();\/;:@= +]+)/CGI::escape($1)/eg;
@@ -1568,8 +1572,11 @@ sub esc_path_info {
sub esc_url {
my $str = shift;
return undef unless defined $str;
+
+ $str = to_utf8($str);
$str =~ s/([^A-Za-z0-9\-_.~();\/;?:@= ]+)/CGI::escape($1)/eg;
$str =~ s/ /\+/g;
+
return $str;
}
--
1.7.1
--
To unsubscribe from this list: send the line 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] config: respect '~' and '~user' in mailmap.file
On Tue, May 27, 2014 at 10:45:58AM +0200, Øystein Walle wrote: git_config_string() does not handle '~' and '~user' as part of the value. Using git_config_pathname() fixes this. Makes sense. Curious if there was a reason we did not use it in the first place, I looked at the history. The reason is that mailmap.file was added in d551a48 (2009-02-08) and git_config_pathname came months later in 395de25 (2009-11-17). Retro-fitting it now should not cause a problem for any sane paths. So: Reviewed-by: Jeff King p...@peff.net -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] clean: add a flag to back up cleaned files
On Tue, May 27, 2014 at 04:17:34PM +0200, Erik Faye-Lund wrote:
The combination of git clean and fat fingers can some times cause
data-loss, which can be frustrating.
So let's add a flag that imports the files to be deleted into the
object-database, in a way similar to what git-stash does. Maintain
a reflog of the previously backed up clean-runs.
Signed-off-by: Erik Faye-Lund kusmab...@gmail.com
---
I've had a similar patch laying around for quite a while, but since
f538a91 (git-clean: Display more accurate delete messages), this
patch is a lot less nasty than before. So here you go, perhaps
someone else has fat fingers and hate to lose work?
I've definitely considered doing something like this before (and for
git reset --hard). My biggest concern would be poor performance in
some cases. But since it's optional, and one can presumably override it
with --no-backup for a specific large cleanup, it would not hurt anybody
who does not want to play with it.
+ /* load HEAD into the index */
+
+ tree = parse_tree_indirect(sha1);
+ if (!tree)
+ die(_(Failed to unpack tree object %s), sha1);
+
+ parse_tree(tree);
+ init_tree_desc(t, tree-buffer, tree-size);
+
+ memset(opts, 0, sizeof(opts));
+ opts.head_idx = -1;
+ opts.src_index = the_index;
+ opts.dst_index = the_index;
+ opts.index_only = 1;
+
+ if (unpack_trees(1, t, opts)) {
+ /* We've already reported the error, finish dying */
+ exit(128);
+ }
This bit of the code surprised me. I guess you are trying to re-create
the index state of the HEAD so that the commit you build on top of it
contains _only_ the untracked files as changes, and not whatever
intermediate index state you had. That makes some sense to me, as clean
is never touching the index state.
Though taking a step back for a moment, I'd like to think about doing
something similar for reset --hard, which is the other destructive
operation in git[1]. In that case, I think saving the index state is also
useful, because it is being reset, too (and while those blobs are
recoverable, the exact state is sometimes useful).
If we were to do that, should it be a separate ref? Or should there be a
single backup ref for such oops, undo that operations? If the latter,
what should that ref look like? I think it would look something like
refs/stash, with the index and the working tree state stored as separate
commits (even though in the clean case, the index state is not likely
to be that interesting, it is cheap to store and makes the recovery
tools uniform to use).
And if we are going to store it like that, should we just be using git
stash save --keep-index --include-untracked? I think we would just need
to teach it a --no-reset option to leave the tracked files as-is.
I realize that I went a few steps down the if... chain there to get to
should we just be using stash?. But it would also make the code here
dirt-simple.
[1] Actually, reset --hard may be more of an education issue, as
simply running git stash has the same effect, but keeps a backup.
It's just that reset --hard is advertised as the solution to many
problems.
+ if (!active_cache_tree)
+ active_cache_tree = cache_tree();
+
+ if (!cache_tree_fully_valid(active_cache_tree)) {
+ if (cache_tree_update(active_cache_tree,
+ (const struct cache_entry * const *)active_cache,
+ active_nr, 0) 0)
+ die(failed to update cache);
+ }
I'd have thought you could use write_cache_as_tree, which backs git
write-tree, but there is currently no way to convince it not to write
out the new cache. This is little enough code that it may not be worth
refactoring write_cache_as_tree to handle it.
+ /* create a reflog, if there isn't one */
+ git_snpath(logfile, sizeof(logfile), logs/%s, ref);
+ if (stat(logfile, st)) {
+ FILE *fp = fopen(logfile, w);
+ if (!fp)
+ warning(_(Can not do reflog for '%s'\n), ref);
+ else
+ fclose(fp);
+ }
Kind of gross that we have to do this ourselves (and somewhat contrary
to efforts elsewhere to make the ref code more abstract), but I see that
git stash does the same thing.
Should you fopen with a here, to avoid a race condition where another
process creates it between the stat() and the fopen(), in which case we
would truncate what they wrote? You could even just get rid of the
stat(), then, like:
if ((fp = fopen(logfile, a)))
fclose(fp);
else
warning(_(Can not do reflog for '%s'), ref);
Also note that your warning has an extra \n in it.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sideband.c: Get rid of ANSI sequence for non-terminal shell
On Tue, May 27, 2014 at 01:27:12PM +1000, Michael Naumov wrote: From ae8d04fdbd71cf322e67903826544d5431f2866d Mon Sep 17 00:00:00 2001 From: Michael Naumov mnaou...@gmail.com Date: Tue, 27 May 2014 12:45:06 +1000 Subject: [PATCH] sideband.c: Get rid of ANSI sequence for non-terminal shell You can drop these lines; they are redundant with your email's headers (but see below for more email tips). Some git tools such as GitExtensions for Windows use environment variable TERM=msys which causes the weird ANSI sequence shown for the messages returned from server-side hooks See https://github.com/gitextensions/gitextensions/issues/1313 for more details It's nice to give a pointer to more discussion, but it's also a good idea to give a one- or two-sentence summary. Something like (after your first paragraph): We add those ANSI sequences to help format sideband data on the user's terminal. However, these extensions are not using a terminal, and the sequences just confuse them. We can recognize this use by checking isatty(). NOTE: I considered to cover the case that a pager has already been started. But decided that is probably not worth worrying about here, though, as we shouldn't be using a pager for commands that do network communications (and if we do, omitting the magic line-clearing signal is probably a sane thing to do). I was nodding my head in agreement at this, and then went back and realized that it is paraphrasing me. So I definitely agree with it. :) Signed-off-by: Michael Naumov mnaou...@gmail.com Thanks-to: Erik Faye-Lund kusmab...@gmail.com Thanks-to: Jeff King p...@peff.net A minor nit, but please keep these trailers together in a single paragraph. Elsewhere on the list people are developing tools to write and parse them, and I believe that they only look backwards up to the last empty line. diff --git a/sideband.c b/sideband.c index d1125f5..7f9dc22 100644 --- a/sideband.c +++ b/sideband.c @@ -30,7 +30,7 @@ int recv_sideband(const char *me, int in_stream, int out) memcpy(buf, PREFIX, pf); term = getenv(TERM); - if (term strcmp(term, dumb)) + if (isatty(2) term strcmp(term, dumb)) suffix = ANSI_SUFFIX; else suffix = DUMB_SUFFIX; Your patch looks whitespace damaged. It was also sent as a multipart/alternative with html, which I suspect means it did not make it to the list. It looks like you're using gmail. The simplest thing is to just use git-send-email to send it; there are tips for configuring send-email with gmail in git help send-email. -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
Git chokes on large file
I've discovered a problem using Git. It's not clear to me what the correct behavior should be, but it seems to me that Git is failing in an undesirable way. The problem arises when trying to handle a very large file. For example: $ git --version git version 1.8.3.1 $ mkdir $$ $ cd $$ $ git init Initialized empty Git repository in /common/not-replicated/worley/temp/5627/.git/ $ truncate --size=20G big_file $ ls -l total 0 -rw-rw-r--. 1 worley worley 21474836480 May 27 11:59 big_file $ time git add big_file real4m48.752s user4m31.295s sys 0m16.747s $ At this point, either 'git fsck' or 'git commit' fails: $ git fsck --full --strict notice: HEAD points to an unborn branch (master) Checking object directories: 100% (256/256), done. fatal: Out of memory, malloc failed (tried to allocate 21474836481 bytes) $ git commit -m Test. [master (root-commit) 3df3655] Test. fatal: Out of memory, malloc failed (tried to allocate 21474836481 bytes) The central problem is that one can accidentally add a file that leaves the repository in a broken state, where various normal commands simply don't work. The most worrying aspect is that git fsck fails -- of all the commands, the one that verifies the validity of the repository (and diagnoses errors) should be the most robust! Even doing a 'git reset' does not put the repository in a state where 'git fsck' will complete: $ git reset $ git fsck --full --strict notice: HEAD points to an unborn branch (master) Checking object directories: 100% (256/256), done. fatal: Out of memory, malloc failed (tried to allocate 21474836481 bytes) Dale -- To unsubscribe from this list: send the line 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] remote: defer repacking packed-refs when deleting refs
Jens Lindström j...@opera.com writes: Puzzled There is one reason why one would want to call delete_ref() even if the ref itself was already fully deleted by repack_without_refs() (because it was only packed) and that is that delete_ref() also removes the ref log, if there is one. Ahh, ok, no longer puzzled---I completely forgot about that part. We could refactor the deletion to 1) repack_without_refs() on all refs 2) delete_ref() on still existing (loose) refs 3) delete_ref_log() on all refs to let us only call delete_ref() on existing refs, and then keep the current value check. I tend to agree that it is sufficient for the purpose of this topic to be loose about the check; the refactoring can come later, as part of the ref-transaction refactoring that is going on in a separate thread. -- To unsubscribe from this list: send the line 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] Add an explicit GIT_DIR to the list of excludes
Add GIT_DIR to the list of excludes in setup_standard_excludes(), while checking that GIT_DIR is not just '.git', in which case it would be ignored by default, and that GIT_DIR is inside GIT_WORK_TREE gives git-grep.txt and git-ls-files.txt. I don't know if we need to add something about this extra exclude rule to those .txt. If it's so obvious that this should be the expected behavior, then probably not. I suggest this. There appears to be a notion of standard excludes both in the code (dir.c) and in the man pages (git-grep, git-ls-files). However, it doesn't actually appear to be defined strictly speaking. So my suggestion is to define those standard excludes in one place (say gitignore(5)), and make other man pages (git-config, git-grip, git-ls-files) have references to that place instead of explaining every time in detail what is being excluded. Now, gitignore(5) actually does have this list of ignored items, we only need to call it standard excludes. If done this way, then all that needs to be done regarding GIT_DIR is to insert it into that list in gitignore(5). Please let me know if that'd work Pasha -- To unsubscribe from this list: send the line 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] Add an explicit GIT_DIR to the list of excludes
Duy Nguyen pclo...@gmail.com writes: On Sat, May 24, 2014 at 12:33 AM, Pasha Bolokhov pasha.bolok...@gmail.com wrote: When an explicit '--git-dir' option points to a directory inside the work tree, git treats it as if it were any other directory. In particular, 'git status' lists it as untracked, while 'git add -A' stages the metadata directory entirely Add GIT_DIR to the list of excludes in setup_standard_excludes(), while checking that GIT_DIR is not just '.git', in which case it would be ignored by default, and that GIT_DIR is inside GIT_WORK_TREE Although an analogous comparison of any given path against '.git' is done in treat_path(), this does not seem to be the right place to compare against GIT_DIR. Instead, the excludes provide an effective mechanism of ignoring a file/directory, and adding GIT_DIR as an exclude is equivalent of putting it into '.gitignore'. Function setup_standard_excludes() was chosen because that is the place where the excludes are initialized by the commands that are concerned about excludes I like this approach. A search of exclude-standard in Documentation/ gives git-grep.txt and git-ls-files.txt. I don't know if we need to add something about this extra exclude rule to those .txt. If it's so obvious that this should be the expected behavior, then probably not. OK, so is that an Acked/Reviewed-by? The case of git grep --exclude-standard is interesting because it's intended to work without a repository. First reaction was would get_git_dir() return NULL in that case. But it should return .git so we're good. -- To unsubscribe from this list: send the line 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] gc --auto: do not lock refs in the background
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
9f673f9 (gc: config option for running --auto in background -
2014-02-08) puts gc --auto in background to reduce user's wait
time. Part of the garbage collecting is pack-refs and pruning
reflogs. These require locking some refs and may abort other processes
trying to lock the same ref. If gc --auto is fired in the middle of a
script, gc's holding locks in the background could fail the script,
which could never happen before 9f673f9.
Keep running pack-refs and reflog --prune in foreground to stop
parallel ref updates. The remaining background operations (repack,
prune and rerere) should impact running git processes.
Reported-by: Adam Borowski kilob...@angband.pl
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
builtin/gc.c | 26 --
1 file changed, 20 insertions(+), 6 deletions(-)
OK, as it happens the order of various gc phases we have is already
to run pack-refs and reflog expire before everything else, so this
change does not affect semantics, which is good ;-)
diff --git a/builtin/gc.c b/builtin/gc.c
index 85f5c2b..8d219d8 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -26,6 +26,7 @@ static const char * const builtin_gc_usage[] = {
};
static int pack_refs = 1;
+static int prune_reflogs = 1;
static int aggressive_depth = 250;
static int aggressive_window = 250;
static int gc_auto_threshold = 6700;
@@ -258,6 +259,19 @@ static const char *lock_repo_for_gc(int force, pid_t*
ret_pid)
return NULL;
}
+static int gc_before_repack(void)
+{
+ if (pack_refs run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))
+ return error(FAILED_RUN, pack_refs_cmd.argv[0]);
+
+ if (prune_reflogs run_command_v_opt(reflog.argv, RUN_GIT_CMD))
+ return error(FAILED_RUN, reflog.argv[0]);
+
+ pack_refs = 0;
+ prune_reflogs = 0;
+ return 0;
+}
+
int cmd_gc(int argc, const char **argv, const char *prefix)
{
int aggressive = 0;
@@ -320,12 +334,15 @@ int cmd_gc(int argc, const char **argv, const char
*prefix)
fprintf(stderr, _(Auto packing the repository
for optimum performance.\n));
fprintf(stderr, _(See \git help gc\ for manual
housekeeping.\n));
}
- if (detach_auto)
+ if (detach_auto) {
+ if (gc_before_repack())
+ return -1;
/*
* failure to daemonize is ok, we'll continue
* in foreground
*/
daemonize();
+ }
} else
add_repack_all_option();
@@ -337,11 +354,8 @@ int cmd_gc(int argc, const char **argv, const char
*prefix)
name, (uintmax_t)pid);
}
- if (pack_refs run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))
- return error(FAILED_RUN, pack_refs_cmd.argv[0]);
-
- if (run_command_v_opt(reflog.argv, RUN_GIT_CMD))
- return error(FAILED_RUN, reflog.argv[0]);
+ if (gc_before_repack())
+ return -1;
if (run_command_v_opt(repack.argv, RUN_GIT_CMD))
return error(FAILED_RUN, repack.argv[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: commit: support commit.verbose and --no-verbose
Jeremiah Mahler jmmah...@gmail.com writes: On Fri, May 23, 2014 at 04:22:22PM -0500, Caleb Thompson wrote: This patch allows people to set `commit.verbose` to implicitly send `--verbose` ... +cat check-for-no-diff EOF +#!$SHELL_PATH +exec grep -v '^diff --git' \$1 +EOF +chmod +x check-for-no-diff + For new tests, commands like this should be placed inside a test_expect_success structure. However, I can see why you did it this way since the code just above it does it this way. Perhaps others will have some recommendations. Also, \-EOF is used instead of EOF to remove the tabs. test_expect_success 'commit verbose setup' ' cat check-for-no-diff \-EOF #!SHELL_PATH exec grep -v '^diff --git' \$1 EOF chmod +x check-for-no-diff ' Also tests use write_script these days to do this kind of thing. -- To unsubscribe from this list: send the line 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] clean: add a flag to back up cleaned files
On Tue, May 27, 2014 at 6:37 PM, Jeff King p...@peff.net wrote:
On Tue, May 27, 2014 at 04:17:34PM +0200, Erik Faye-Lund wrote:
The combination of git clean and fat fingers can some times cause
data-loss, which can be frustrating.
So let's add a flag that imports the files to be deleted into the
object-database, in a way similar to what git-stash does. Maintain
a reflog of the previously backed up clean-runs.
Signed-off-by: Erik Faye-Lund kusmab...@gmail.com
---
I've had a similar patch laying around for quite a while, but since
f538a91 (git-clean: Display more accurate delete messages), this
patch is a lot less nasty than before. So here you go, perhaps
someone else has fat fingers and hate to lose work?
I've definitely considered doing something like this before (and for
git reset --hard). My biggest concern would be poor performance in
some cases. But since it's optional, and one can presumably override it
with --no-backup for a specific large cleanup, it would not hurt anybody
who does not want to play with it.
I also made it opt-in rather than opt-out by default. Perhaps it
shouldn't be, though - dunno.
+ /* load HEAD into the index */
+
+ tree = parse_tree_indirect(sha1);
+ if (!tree)
+ die(_(Failed to unpack tree object %s), sha1);
+
+ parse_tree(tree);
+ init_tree_desc(t, tree-buffer, tree-size);
+
+ memset(opts, 0, sizeof(opts));
+ opts.head_idx = -1;
+ opts.src_index = the_index;
+ opts.dst_index = the_index;
+ opts.index_only = 1;
+
+ if (unpack_trees(1, t, opts)) {
+ /* We've already reported the error, finish dying */
+ exit(128);
+ }
This bit of the code surprised me. I guess you are trying to re-create
the index state of the HEAD so that the commit you build on top of it
contains _only_ the untracked files as changes, and not whatever
intermediate index state you had. That makes some sense to me, as clean
is never touching the index state.
TBH, I didn't really think this stuff through, I basically just hacked
on this until I got it to save away superfluous files when the index
matched HEAD. So this part is more accidental than designed. I'm not
very familiar with the index-maniuplation code in Git either.
I *think* the right thing to do would be to save the tree of HEAD
*plus* those deleted files in this case. That way it only records the
destruction itself. This does *not* seem to be what currently happens.
If I have a change staged, that staged change also gets included in
the commit. That's not ideal, I think.
Though taking a step back for a moment, I'd like to think about doing
something similar for reset --hard, which is the other destructive
operation in git[1]. In that case, I think saving the index state is also
useful, because it is being reset, too (and while those blobs are
recoverable, the exact state is sometimes useful).
I agree. I guess that should essentially do a full git-stash.
If we were to do that, should it be a separate ref? Or should there be a
single backup ref for such oops, undo that operations? If the latter,
what should that ref look like? I think it would look something like
refs/stash, with the index and the working tree state stored as separate
commits (even though in the clean case, the index state is not likely
to be that interesting, it is cheap to store and makes the recovery
tools uniform to use).
Hmm, perhaps. I do like the concept of a git undo of sorts, but
perhaps that'll raise the expectations even further? Or maybe raising
them a good thing, so people add missing features? :)
And if we are going to store it like that, should we just be using git
stash save --keep-index --include-untracked? I think we would just need
to teach it a --no-reset option to leave the tracked files as-is.
Hm, interesting. But it does leave me with a bit of a bad feeling; git
stash is currently a shell-script, and I want us to move *away* from
depending on those rather than towards... Or perhaps I just convinced
myself to port git-stash to C? I guess the full script won't be
needed, only the heavy lifting...
I realize that I went a few steps down the if... chain there to get to
should we just be using stash?. But it would also make the code here
dirt-simple.
Simplicity is good, and it's always good to hear some different ideas
on how to reach a goal.
[1] Actually, reset --hard may be more of an education issue, as
simply running git stash has the same effect, but keeps a backup.
It's just that reset --hard is advertised as the solution to many
problems.
+ if (!active_cache_tree)
+ active_cache_tree = cache_tree();
+
+ if (!cache_tree_fully_valid(active_cache_tree)) {
+ if (cache_tree_update(active_cache_tree,
+ (const struct cache_entry * const *)active_cache,
+ active_nr, 0) 0)
+ die(failed to update
Re: [PATCH v8 41/44] refs.c: add a new flag for transaction delete for refs we know are packed only
Michael Haggerty mhag...@alum.mit.edu writes: This suggests to me that our current structure is best modeled as two independent reference back ends, with a third implementation of the same reference API whose job it is to compose the first two. In pseudocode, ... That is an interesting view. How does reflog fit into the picture? Is it a completely independent thing that is called from any implementation of ReferenceBackend interface? From this point of view it is clear that packing refs is not an operation that belongs in the ReferenceBackend API, but rather in the StackedReferenceBackend interface. When an implementation of ReferenceBackend has skewed performance characteristics (e.g. PackedReferenceBackend really prefers to be modified in bulk), how would that interact with the abstraction? For example, when the application does: begin_transaction() for ref in many_refs(): delete_reference(ref) commit_transaction() StackedReferenceBackend() that consists of Loose on top of Packed may want to implement the commit phase like so: - tell Packed backend to repack without the deleted refs - tell Loose backend to delete the deleted refs But the above would not quite work, as somebody needs to remove logs for refs that were only in the Packed backend, and repack without these refs API supported by the Packed backend cannot be that somebody---repack packed-refs without A B C cannot unconditionally remove logs for A B C without checking if A B C exists as Loose. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 2/3] rebase -i: Reschedule tasks that failed before the index was touched
Hi Fabian,
Thanks for looking into this.
On 05/27/2014 07:56 AM, Michael Haggerty wrote:
+reschedule_last_action () {
+tail -n 1 $done | cat - $todo $todo.new
+sed -e \$d $done $done.new
+mv -f $todo.new $todo
+mv -f $done.new $done
+}
+
append_todo_help () {
git stripspace --comment-lines $todo \EOF
@@ -470,11 +480,15 @@ do_pick () {
--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 ||
-die_with_patch $1 Could not apply $1... $2
+ --amend --no-post-rewrite -n -q -C $1
git cherry-pick indicates its error status specifically as 1 or some
other value. But here it could be that pick_one succeeds but git
commit fails; in that case ret is set to the return code of git
commit. So, if git commit fails with a retcode different than 1,
then reschedule_last_action will be called a few lines later. This
seems incorrect to me.
I agree. I was thinking that pick_one should get this new behavior
instead of do_pick, but some callers may not appreciate the new behavior
(i.e. squash/fixup), though I expect those callers have similar problems
as this commit is trying to fix.
I think adding a pick_one_with_reschedule function (to be called in both
places from do_pick) could solve the issue Michael mentioned without
breaking other pick_one callers.
I have not tested doing so, but I think it would look like this:
===
diff --git i/git-rebase--interactive.sh w/git-rebase--interactive.sh
index fe813ba..ae5603a 100644
--- i/git-rebase--interactive.sh
+++ w/git-rebase--interactive.sh
@@ -235,6 +235,17 @@ git_sequence_editor () {
eval $GIT_SEQUENCE_EDITOR '$@'
}
+pick_one_with_reschedule () {
+pick_one $1
+ret=$?
+case $ret in
+0) ;;
+1) ;;
+*) reschedule_last_action ;;
+esac
+return $ret
+}
+
pick_one () {
ff=--ff
@@ -474,13 +485,13 @@ do_pick () {
# rebase --continue.
git commit --allow-empty --allow-empty-message --amend \
--no-post-rewrite -n -q -C $1
-pick_one -n $1
+pick_one_with_reschedule -n $1
git commit --allow-empty --allow-empty-message \
--amend --no-post-rewrite -n -q -C $1 \
${gpg_sign_opt:+$gpg_sign_opt} ||
die_with_patch $1 Could not apply $1... $2
else
-pick_one $1 ||
+pick_one_with_reschedule $1 ||
die_with_patch $1 Could not apply $1... $2
fi
}
===
I don't much like the name 'pick_one_with_reschedule', but I didn't like
my other choices, either.
I'll try to look at this and your patches more closely when I have a bit
more time.
Phil
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 42/44] refs.c: pass a skip list to name_conflict_fn
On Thu, May 22, 2014 at 12:27 PM, Jonathan Nieder jrnie...@gmail.com wrote:
Ronnie Sahlberg wrote:
--- a/refs.c
+++ b/refs.c
@@ -798,11 +798,19 @@ struct name_conflict_cb {
const char *refname;
const char *oldrefname;
const char *conflicting_refname;
+ const char **skip;
+ int skipnum;
Would a struct string_list make sense here? (See
Documentation/technical/api-string-list.txt.)
It would. But it is better to do that change later.
Currently we have both repack_without_ref and repack_without_refs that
take the same char ** argument.
After next series we will have removed one of these functions and have
an easier API to modify (once we start tracking the skiplist as part
of the transaction instead).
Instead of doing this change and then redoing it once we move a lot of
the actual work from _commit to _update
I will do this change towards the end of the next series.
[...]
};
static int name_conflict_fn(struct ref_entry *entry, void *cb_data)
{
struct name_conflict_cb *data = (struct name_conflict_cb *)cb_data;
+ int i;
+ for(i = 0; i data-skipnum; i++) {
Fixed.
(style nit) missing space after 'for'.
+ if (!strcmp(entry-name, data-skip[i])) {
+ return 0;
+ }
Style: git tends to avoid braces around a single-line if/for/etc body.
Fixed.
[...]
@@ -817,15 +825,21 @@ static int name_conflict_fn(struct ref_entry *entry,
void *cb_data)
* conflicting with the name of an existing reference in dir. If
* oldrefname is non-NULL, ignore potential conflicts with oldrefname
* (e.g., because oldrefname is scheduled for deletion in the same
- * operation).
+ * operation). skip contains a list of refs we want to skip checking for
+ * conflicts with. Refs may be skipped due to us knowing that it will
+ * be deleted later during a transaction that deletes one reference and then
+ * creates a new conflicting reference. For example a rename from m to m/m.
This example of Refs may be skipped due to seems overly complicated.
Isn't the idea just that skip contains a list of refs scheduled for
deletion in this transaction, since they shouldn't be treated as
conflicts at all (for example when renamining m to m/m)?
I wonder if there's some way to make use of the result of the naive
refname_available check to decide what to do when creating a ref.
E.g.: if a refname would be available except there's a ref being
deleted in the way, we could do one of the following:
a. delete all relevant loose refs and perform the transaction in
packed-refs, or
b. order operations to avoid the D/F conflict, even with loose refs
(the hardest case is if the ref being deleted uses a directory
and we want to create a file with the same name. But that's
still doable if we're willing to rmdir when needed as part of
the loop to commit changes)
The packed-refs trick (a) seems much simpler, but either should work.
This could be done e.g. by checking is_refname_available with an empty
list first before doing the real thing with a list of exclusions.
[...]
@@ -2592,6 +2609,9 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
int log = !lstat(git_path(logs/%s, oldrefname), loginfo);
const char *symref = NULL;
+ if (!strcmp(oldrefname, newrefname))
+ return 0;
What is the intended result if I try to rename a nonexistent ref or an
existent symref to its own name?
Yeah, I should get rid of that.
I have renoved the rename_ref patch and moved it to the next series.
I think we can solve this easier/better once we have the other patches in first.
Sorry to be so fussy about this part. It's not that I think that this
change is trying to do something bad --- in fact, it's more the
opposite, that I'm excited to see git learning to have a better
understanding and handling of refname D/F conflicts.
That would allow:
* git fetch --prune working as a single transaction even if
the repository being fetched from removed a refs/heads/topic
branch and created refs/heads/topic/1 and refs/heads/topic/2
* git fast-import and git fetch --mirror learning the same trick
* fewer code paths having to be touched to be able to (optionally)
let git actually tolerate D/F conflicts, for people who want to
have 'topic', 'topic/1', and 'topic/2' branches at the same time.
This could be turned on by default for remote-tracking refs. It
would be especially nice for people on Windows and Mac OS where
there can be D/F conflicts that people on Linux didn't notice due
to case-sensitivity.
Longer term, through a configuration that starts turned off by
default and has the default flipped as more people have upgraded
git, this could make D/F conflicts in refnames stop being an error
altogether.
So it's kind of exciting to see, even though it's fussy to get it
right.
Re: [RFC 1/3] sequencer: Signal failed ff as an aborted, not a conflicted merge
Fabian Ruch baf...@gmail.com writes: `do_pick_commit` handles three situations if it is not fast-forwarding. In order for `do_pick_commit` to identify the situation, it examines the return value of the selected merge command. 1. return value 0 stands for a clean merge 2. 1 is passed in case of a failed merge due to conflict 3. any other return value means that the merge did not even start So far, the sequencer returns 1 in case of a failed fast-forward, which would mean failed merge due to conflict. However, a fast-forward either starts and succeeds or does not start at all. In particular, it cannot fail in between like a merge with a dirty index due to conflicts. In order to signal the three possible situations (not only success and failure to complete) after a pick through porcelain commands such as `cherry-pick`, exit with a return value that is neither 0 nor 1. -1 was chosen in line with the other situations in which the sequencer encounters an error. Hmph... do we still pass negative to exit() anywhere in our codebase? Signed-off-by: Fabian Ruch baf...@gmail.com --- sequencer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 90cac7b..97cecca 100644 --- a/sequencer.c +++ b/sequencer.c @@ -278,7 +278,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from, read_cache(); if (checkout_fast_forward(from, to, 1)) - exit(1); /* the callee should have complained already */ + exit(-1); /* the callee should have complained already */ ref_lock = lock_any_ref_for_update(HEAD, unborn ? null_sha1 : from, 0, NULL); strbuf_addf(sb, %s: fast-forward, action_name(opts)); -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 3/3] tests: Add 'rebase -i commits that overwrite untracked files'
Fabian Ruch baf...@gmail.com writes: If a todo list will cherry-pick a commit that adds some file and the working tree already contains a file with the same name, the rebase sequence for that todo list will be interrupted and the cherry-picked commit will be lost after the rebasing process is resumed. This is fixed. Add as a test case for regression testing to the rebase-interactive test suite. Reported-by: Phil Hord ho...@cisco.com Signed-off-by: Fabian Ruch baf...@gmail.com --- t/t3404-rebase-interactive.sh | 44 +++ 1 file changed, 44 insertions(+) diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 50e22b1..7f5ac18 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1074,4 +1074,48 @@ test_expect_success 'short SHA-1 collide' ' ) ' +test_expect_success 'rebase -i commits that overwrite untracked files (pick)' ' + git checkout branch2 + set_fake_editor + FAKE_LINES=edit 1 2 git rebase -i A + test_cmp_rev HEAD F + test_path_is_missing file6 + touch file6 Unless you care deeply about updating the timestamp file6 has, use of touch is misleading. Use something like this instead: file6 when it is the existence of file6 that you care about. 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/RFC] clean: add a flag to back up cleaned files
On Tue, May 27, 2014 at 08:12:52PM +0200, Erik Faye-Lund wrote:
I've definitely considered doing something like this before (and for
git reset --hard). My biggest concern would be poor performance in
some cases. But since it's optional, and one can presumably override it
with --no-backup for a specific large cleanup, it would not hurt anybody
who does not want to play with it.
I also made it opt[...]in rather than opt[...]out by default. Perhaps it
shouldn't be, though - dunno.
I like starting with it optional, and then people who are interested in
the feature can experiment with it, giving it a chance to prove itself
(and for us to work out any downsides) before turning it on by default.
BTW, I think the opt phrases above might be caught by vger's taboo
filter. If your mail doesn't appear on the list, that is likely the
reason.
This bit of the code surprised me. I guess you are trying to re-create
the index state of the HEAD so that the commit you build on top of it
contains _only_ the untracked files as changes, and not whatever
intermediate index state you had. That makes some sense to me, as clean
is never touching the index state.
TBH, I didn't really think this stuff through, I basically just hacked
on this until I got it to save away superfluous files when the index
matched HEAD. So this part is more accidental than designed. I'm not
very familiar with the index-maniuplation code in Git either.
I *think* the right thing to do would be to save the tree of HEAD
*plus* those deleted files in this case. That way it only records the
destruction itself. This does *not* seem to be what currently happens.
If I have a change staged, that staged change also gets included in
the commit. That's not ideal, I think.
Ah. Yeah, if you are going to just record the current index state, I do
not see a reason to call unpack_trees at all. You can just add new
entries to the index, and then throw the resulting index away without
writing it back to disk.
But I do think it would make sense to reset it back to HEAD and build
straight on there, in which case you basically want to do read-tree
HEAD. Or in C code, unpack-trees as a oneway merge. I thought that's
what was going on here. ;)
All of this is moot if you go in the stash direction, as then you would
be storing something a bit more complicated (and delegating the
complexity to stash anyway).
If we were to do that, should it be a separate ref? Or should there be a
single backup ref for such oops, undo that operations? If the latter,
what should that ref look like? I think it would look something like
refs/stash, with the index and the working tree state stored as separate
commits (even though in the clean case, the index state is not likely
to be that interesting, it is cheap to store and makes the recovery
tools uniform to use).
Hmm, perhaps. I do like the concept of a git undo of sorts, but
perhaps that'll raise the expectations even further? Or maybe raising
them a good thing, so people add missing features? :)
Yeah, I think this would just be a first step on the way to git undo.
It's the safety net for a few commands, but a true undo would need
somebody to build logic to see what the last command is, and then
reverse it (presumably using these safety nets). I don't think there's
any problem with building this first step and leaving the rest for
somebody to do later; it's a strict improvement.
And if we are going to store it like that, should we just be using git
stash save --keep-index --include-untracked? I think we would just need
to teach it a --no-reset option to leave the tracked files as-is.
Hm, interesting. But it does leave me with a bit of a bad feeling; git
stash is currently a shell-script, and I want us to move *away* from
depending on those rather than towards... Or perhaps I just convinced
myself to port git-stash to C? I guess the full script won't be
needed, only the heavy lifting...
Yeah, I wondered if you might say that, knowing how you Windows guys are
hesitant about shell scripts. :)
My feeling is that you should think about the best design, and implement
it that way. If stash as a shell script is a problem, then fix that on
the way. Of course it is very easy for me to tell you that, as I am not
the one volunteering to do the extra work. ;)
+ if (!active_cache_tree)
+ active_cache_tree = cache_tree();
+
+ if (!cache_tree_fully_valid(active_cache_tree)) {
+ if (cache_tree_update(active_cache_tree,
+ (const struct cache_entry * const *)active_cache,
+ active_nr, 0) 0)
+ die(failed to update cache);
+ }
I'd have thought you could use write_cache_as_tree, which backs git
write-tree, but there is currently no way to convince it not to write
out the new cache. This is little enough code that it may not be worth
refactoring write_cache_as_tree to handle
Re: [PATCH v1 0/3] Add --graft option to git replace
From: Junio C Hamano gits...@pobox.com Subject: Re: [PATCH v1 0/3] Add --graft option to git replace Date: Fri, 23 May 2014 09:59:05 -0700 Christian Couder chrisc...@tuxfamily.org writes: Here is a small patch series to implement: git replace [-f] --graft commit [parent...] The changes since the RFC/PATCH are the following: - in patch 1/3, parse_commit_buffer() is now used to make sure commit is not corrupt - patch 2/3 add some tests - patch 3/3 add some documentation About the documentation, maybe we should add that --graft can now be used instead of grafts in .git/info/grafts, and maybe we could add an example that shows how it can be done. Or a procedure that reads .git/info/grafts, converts it to a set of replacements and drops .git/info/grafts. A sample script could be thrown in to contrib/ somewhere as convert-graft-to-replace.sh. Ok, I just sent a patch that adds such a sample script. Thanks, Christian. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 00/44] Use ref transactions for all ref updates
On Thu, May 22, 2014 at 4:08 PM, Jonathan Nieder jrnie...@gmail.com wrote: Ronnie Sahlberg wrote: This patch series can also be found at https://github.com/rsahlberg/git/tree/ref-transactions Continuing with the review of 65a1cb7b (2014-05-22 12:08): 11/40 change ref_transaction_update() to do error checking and return status The there will be in the future sounds ominous. Do you have an example in mind? E.g., I suppose it would be nice if _update could notice D/F conflicts or connection to a database server closing early, but it's not clear to me whether the kind of errors you're talking about are that or something else. Updated the message. Next series moves both locking as well as checking for name conflicts to _update. With or without such a clarification, Reviewed-by: Jonathan Nieder jrnie...@gmail.com 12/40 change ref_transaction_create to do error checking and return status What does On failure the err buffer will be updated mean? Will it clear err and replace it with a message, append to err, or something else? Does the message explain the context or is the caller responsible for adding to it? Does the message end with a newline or is the caller responsible for adding one when printing it out? I have updated the documentation. Message is appended to the string buffer. Caller is required to strbuf_reset before calling the transaction if caller wants only most recent error instead of all errors appended one by one. For cases like this where lots of functions have a similar API, API comments start to become potentially repetitive. It might be better to explain conventions at the top of the file or in Documentation/technical/api-refs.txt and say See the top of the file for error handling conventions or Returns non-zero and appends a message to err on error. See Documentation/technical/api-refs.txt for more details on error handling. Done. 13/40 ref_transaction_delete to check for error and return status Each successive commit dropped something from its subject. :) (First the (), then the verb.) Done. Same comments as before about an example being useful for the log message and the API documentation on error handling being a bit vague. 14/40 make ref_transaction_begin take an err argument I found the failed to connect to mysql example instructive while doing reviews. Perhaps it would be worth mentioning in the commit message. Reviewed-by: Jonathan Nieder jrnie...@gmail.com 15/40 add transaction.status and track OPEN/CLOSED/ERROR It says an ERRORed transaction cannot be committed and can be rolled back by calling _free. Can a CLOSED transaction be committed or _freed? What does faild mean in the documentation comments? (Maybe non-OPEN?) In the previous version of this patch passing a non-OPEN transaction would die(BUG: ...) to diagnose the caller's mistake. Now I'm confused about the API: it seems you're allowed to pass a non-OPEN transaction but it doesn't append a message to 'err' in that case. Is this meant as a way to save the caller some typing, like fwrite/fclose do? (I've found people often make mistakes with the fwrite API fwiw but can understand the appeal of it.) Maybe with more context I'd like this. As is, it feels like a step in the wrong direction. 16/40 tag: use ref transactions when doing updates Reviewed-by: Jonathan Nieder jrnie...@gmail.com 17/40 replace: use ref transactions when doing updates Reviewed-by: Jonathan Nieder jrnie...@gmail.com 18/40 commit: use ref transactions for updates Reviewed-by: Jonathan Nieder jrnie...@gmail.com 19/40 sequencer: use ref transactions for all ref updates This would be a lot simpler if the ref_transaction_commit should not free the transaction patch came before it (yes, sorry, killing the fun). I can push the result of a rebase doing that somewhere if you like. Beeing done. 20/40 fast-import: change update_branch to use ref transactions Likewise. -- To unsubscribe from this list: send the line 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 v10 22/44] fetch.c: clear errno before calling functions that might set it
On Sat, May 17, 2014 at 7:56 AM, Michael Haggerty mhag...@alum.mit.edu wrote: On 05/16/2014 07:37 PM, Ronnie Sahlberg wrote: In s_update_ref there are two calls that when they fail we return an error based on the errno value. In particular we want to return a specific error if ENOTDIR happened. Both these functions do have failure modes where they may return an error without updating errno, in which case a previous and unrelated ENOTDIR may cause us to return the wrong error. Clear errno before calling any functions if we check errno afterwards. If I understand correctly, this is a workaround for some other broken functions that don't handle errno correctly. Long-term, wouldn't it be better to fix the other functions? In other words, they should change errno if an only if they return an error status, no? Yeah, but this patch is gone now. Longer term I think we should get rid of passing errno around. errno is best to only be checked immediately after a failed system call and never else. (otherwise you end up with a hairy forest of save_errno variables.) Of course you are under no obligation to fix the universe, so this change may be an expedient workaround anyway. But if you go this route, then I think a comment would be helpful to explain the unusual clearing of errno. Michael Also skip initializing a static variable to 0. Statics live in .bss and are all automatically initialized to 0. Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- builtin/fetch.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 55f457c..a93c893 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -44,7 +44,7 @@ static struct transport *gtransport; static struct transport *gsecondary; static const char *submodule_prefix = ; static const char *recurse_submodules_default; -static int shown_url = 0; +static int shown_url; static int option_parse_recurse_submodules(const struct option *opt, const char *arg, int unset) @@ -382,6 +382,8 @@ static int s_update_ref(const char *action, if (!rla) rla = default_rla.buf; snprintf(msg, sizeof(msg), %s: %s, rla, action); + + errno = 0; lock = lock_any_ref_for_update(ref-name, check_old ? ref-old_sha1 : NULL, 0, NULL); -- 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 v10 11/12] Documentation: add documentation for 'git interpret-trailers'
Christian Couder chrisc...@tuxfamily.org writes: From: Michael Haggerty mhag...@alum.mit.edu ... An option like --input-separator might be enough to support this. For me this means: * Enumerating a list of allowed separators (e.g., [:=#]) Junio suggested in a message that users might use different separators like '%'. I actually think we shouldn't go any fancier than : and nothing else, not even #. I was hoping that you would eventually realize that there are only two viable extremes when I suggested the users may want to use other random characters like '%' and also the users can specify the 'key' with colon and trailing SP (in $gmane/245960). - If you want to give the projects greater control of the format, then you cannot rely on separators anyway. Your users can list all possible footer keys the particular project would use, so that they are recognized by Git, be that Fixes: 4a28f16, Bug #12354, without hard-coding what separator Git must pay attention to. You can easily find a run of lines that begin with any of the key (e.g. Fixes: , Signed-off-by: , Bug #, ...) starting from the tail-end of the log message and that is your footer block. No need for separators at all. - If you want to give the projects freedom to come up with random new kinds of footers without pre-arrangement, then you need to have a reliable way to say if any line you have never seen could be a footer material. A colon has been used everywhere, and used even in the Fixes: 4a28f16 example you took from the kernel circle. I think you presented it with '#' but I do not think they even want that, looking at: http://lists.linuxfoundation.org/pipermail/ksummit-discuss/2014-May/000618.html I also think that bug tracking system using Bug #12345 is an unrelated issue, as log viewers would want to highlight and make links out of them anywhere in the log message text, not limited to the log footer part. As to which one of these two we should take, I tend to think that we should start small and limited; loosening the syntax later is much easier than going the other way, i.e. : and nothing else. -- To unsubscribe from this list: send the line 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 v10 04/44] refs.c: add an err argument to repack_without_refs
On Sat, May 17, 2014 at 5:40 AM, Michael Haggerty mhag...@alum.mit.edu wrote:
On 05/16/2014 07:36 PM, Ronnie Sahlberg wrote:
Update repack_without_refs to take an err argument and update it if there
is a failure. Pass the err variable from ref_transaction_commit to this
function so that callers can print a meaningful error message if _commit
fails due to a problem in repack_without_refs.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
cache.h| 2 ++
lockfile.c | 21 -
refs.c | 25 +++--
3 files changed, 33 insertions(+), 15 deletions(-)
diff --git a/cache.h b/cache.h
index 8c6cdc2..48548d6 100644
--- a/cache.h
+++ b/cache.h
@@ -559,6 +559,8 @@ struct lock_file {
#define LOCK_DIE_ON_ERROR 1
#define LOCK_NODEREF 2
extern int unable_to_lock_error(const char *path, int err);
+extern void unable_to_lock_strbuf(const char *path, int err,
+ struct strbuf *buf);
extern NORETURN void unable_to_lock_index_die(const char *path, int err);
extern int hold_lock_file_for_update(struct lock_file *, const char *path,
int);
extern int hold_lock_file_for_append(struct lock_file *, const char *path,
int);
diff --git a/lockfile.c b/lockfile.c
index 8fbcb6a..9e04b43 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -157,33 +157,36 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
return lk-fd;
}
-static char *unable_to_lock_message(const char *path, int err)
+void unable_to_lock_strbuf(const char *path, int err, struct strbuf *buf)
{
- struct strbuf buf = STRBUF_INIT;
if (err == EEXIST) {
- strbuf_addf(buf, Unable to create '%s.lock': %s.\n\n
+ strbuf_addf(buf, Unable to create '%s.lock': %s.\n\n
If no other git process is currently running, this
probably means a\n
git process crashed in this repository earlier. Make sure
no other git\n
process is running and remove the file manually to
continue.,
absolute_path(path), strerror(err));
} else
- strbuf_addf(buf, Unable to create '%s.lock': %s,
+ strbuf_addf(buf, Unable to create '%s.lock': %s,
absolute_path(path), strerror(err));
- return strbuf_detach(buf, NULL);
}
int unable_to_lock_error(const char *path, int err)
{
- char *msg = unable_to_lock_message(path, err);
- error(%s, msg);
- free(msg);
+ struct strbuf buf = STRBUF_INIT;
+
+ unable_to_lock_strbuf(path, err, buf);
+ error(%s, buf.buf);
+ strbuf_release(buf);
return -1;
}
NORETURN void unable_to_lock_index_die(const char *path, int err)
{
- die(%s, unable_to_lock_message(path, err));
+ struct strbuf buf = STRBUF_INIT;
+
+ unable_to_lock_strbuf(path, err, buf);
+ die(%s, buf.buf);
}
int hold_lock_file_for_update(struct lock_file *lk, const char *path, int
flags)
diff --git a/refs.c b/refs.c
index 686b802..a470e51 100644
--- a/refs.c
+++ b/refs.c
@@ -2208,6 +2208,7 @@ int commit_packed_refs(void)
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(ref_cache);
int error = 0;
+ int save_errno;
if (!packed_ref_cache-lock)
die(internal error: packed-refs not locked);
@@ -2217,10 +2218,13 @@ int commit_packed_refs(void)
do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),
0, write_packed_entry_fn,
packed_ref_cache-lock-fd);
- if (commit_lock_file(packed_ref_cache-lock))
+ if (commit_lock_file(packed_ref_cache-lock)) {
+ save_errno = errno;
error = -1;
+ }
packed_ref_cache-lock = NULL;
release_packed_ref_cache(packed_ref_cache);
+ errno = save_errno;
This code involving save_errno looks like a bug fix orthogonal to the
rest of the patch. It should at least be mentioned in the commit
message if not broken into a separate patch.
Text updated.
return error;
}
@@ -2427,12 +2431,12 @@ static int curate_packed_ref_fn(struct ref_entry
*entry, void *cb_data)
return 0;
}
-static int repack_without_refs(const char **refnames, int n)
+static int repack_without_refs(const char **refnames, int n, struct strbuf
*err)
{
struct ref_dir *packed;
struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
struct string_list_item *ref_to_delete;
- int i, removed = 0;
+ int i, ret, removed = 0;
/* Look for a packed ref */
for (i = 0; i n; i++)
@@ -2444,6 +2448,11 @@ static int repack_without_refs(const char **refnames,
int n)
return 0; /* no refname exists in packed refs */
if (lock_packed_refs(0)) {
+ if (err) {
+ unable_to_lock_strbuf(git_path(packed-refs),
Re: [PATCH v10 25/44] receive-pack.c: use a reference transaction for updating the refs
On Fri, May 23, 2014 at 2:02 PM, Michael Haggerty mhag...@alum.mit.edu wrote: On 05/23/2014 06:14 PM, Ronnie Sahlberg wrote: On Fri, May 23, 2014 at 6:49 AM, Michael Haggerty mhag...@alum.mit.edu wrote: [...] When I combine these two lines of thought, it suggests to me that we could do a better job of supporting *both* use cases. What if the transaction object contained not an err strbuf but a string_list? If an error occurs while building up the transaction, a message would be added to the string list and the function would return an error status. The caller can monitor errors while it is building up the transaction and abort immediately if it wants, or it can ignore the return values and let the error messages accumulate in the string list. When the caller attempts the commit, it would notice that the transaction failed, and at that time the caller could emit *all* of the accumulated error messages by reading them out of the string list; e.g., Error fetching from $REMOTE: - this is generated by caller $ERR[0]- these come from the error string list, $ERR[1] printed with indentation by caller $ERR[2] $ERR[3] This style would have another advantage: we might have some back ends for which transactions have a high overhead. Such a back end would probably choose not to do any checks while the transaction is being built up, e.g., to avoid a round-trip to a database. When commit() is called, it would learn about all of the errors at once. (1) It would need a way to return all of the errors to the caller. (2) It would be nice for the caller to be able to treat such a back end the same as it treats a back end that is able to report errors immediately. It seems to me that having a way to report multiple errors at the same time would solve both problems nicely. Inretesting. That would mean changing all functions to take a string_list provided by the caller instead of a strbuf. And then have _update/_create/_delete do actual work instead of bailing out after the first error. Users that want to check for error and log after each call to _update/_create/_delete could do so and just use the last entry added to the string list or otherwise they could just wait until _commit time and if it fails log all the strings. I still think we should consider storing the err string_list within the transaction object; otherwise it's annoying to have to pass that parameter around everywhere. And if there were also a policy that any caller that checks and reports any error should report *all* of the accumulated errors and abort the transaction, then we don't have to worry about error messages being output multiple times or zero times. It might be nice to have a printf-style helper function like ref_transaction_perror(transaction, fmt, ...) (or maybe ref_transaction_die()) that outputs the accumulated errors with msg as a header (like my example output above), to make error handling easy and uniform. We can add this later once we get the basic transaction stuff in. It will make it easier to experiment with different types of error reporting then. 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] t9138-git-svn-authors-prog.sh fixups
Jeremiah Mahler jmmah...@gmail.com writes:
Several fixups of the t9138-git-svn-authors-prog.sh test script to
follow current recommendations in t/README.
- Fixed a Perl script with a full #!/usr/bin/perl shebang
to use write_script() and $PERL_PATH as per t/README.
- Placed svn-authors data setup inside a test_expect_success.
- Fixed trailing quotes to use the same indentation throughout.
Signed-off-by: Jeremiah Mahler jmmah...@gmail.com
---
t/t9138-git-svn-authors-prog.sh | 35 +--
1 file changed, 17 insertions(+), 18 deletions(-)
diff --git a/t/t9138-git-svn-authors-prog.sh b/t/t9138-git-svn-authors-prog.sh
index 83cc5fc..d54c37a 100755
--- a/t/t9138-git-svn-authors-prog.sh
+++ b/t/t9138-git-svn-authors-prog.sh
@@ -7,40 +7,39 @@ test_description='git svn authors prog tests'
. ./lib-git-svn.sh
-cat svn-authors-prog 'EOF'
-#!/usr/bin/perl
-$_ = shift;
-if (s/-sub$//) {
- print $_ $_\@sub.example.com\n;
-}
-else {
- print $_ $_\@example.com\n;
-}
+write_script svn-authors-prog $PERL_PATH -\EOF
I think you meant to dq $PERL_PATH here. Other than that, looks
OK to me.
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 v2] wording fixes in the user manual and glossary
Jeremiah Mahler jmmah...@gmail.com writes: Various minor wording fixes throughout the user manual and glossary. The section on Updating a repository with git fetch was substantially re-worded to try and better explain `git fetch`. Signed-off-by: Jeremiah Mahler jmmah...@gmail.com --- Notes: From the feedback I received by Chris Packham [1] it was clear that my re-wording of the section Updating a repository with git fetch still wasn't quite right [1]. [1]: http://marc.info/?l=gitm=140100460903936w=2 I re-worded it some more to try and emphasize the remote (upstream) and local aspects of `git fetch`. Chris liked those changes better [2]. [2]: http://marc.info/?l=gitm=140109062903038w=2 I expanded upon this even further. The section on git-pull is similar so I tried to use that as a basis. I also thought the relationship between git fetch and git pull was worthy of a short note along with a link to the section on git-pull. Documentation/glossary-content.txt | 2 +- Documentation/user-manual.txt | 28 ++-- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt index be0858c..4e0b971 100644 --- a/Documentation/glossary-content.txt +++ b/Documentation/glossary-content.txt @@ -1,7 +1,7 @@ [[def_alternate_object_database]]alternate object database:: Via the alternates mechanism, a def_repository,repository can inherit part of its def_object_database,object database - from another object database, which is called alternate. + from another object database, which is called an alternate. [[def_bare_repository]]bare repository:: A bare repository is normally an appropriately diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt index d33f884..f5fd61b 100644 --- a/Documentation/user-manual.txt +++ b/Documentation/user-manual.txt @@ -416,14 +416,22 @@ REVISIONS section of linkgit:gitrevisions[7]. Updating a repository with git fetch -Eventually the developer cloned from will do additional work in her -repository, creating new commits and advancing the branches to point -at the new commits. +After you clone a repository and commit a few changes of your own, you +may wish to check the original repository for updates. The above is very good. +The linkgit:git-fetch[1] command is used to update all the remote-tracking +branches to the latest version found in those repositories. +It will not touch any of your own branches--not even the master +branch that was created during clone. It is harder to review with unnecessary rewrapping of the text X-. I somehow feel that the original was clear around here, by being explicit that git fetch $there $that is not it is talking about, which seems to have been lost in this update. +The linkgit:git-merge[1] command can then be used to merge the changes. + +- +$ git fetch +$ git merge origin/master +- That is not wrong per-se, but it is not a very good example. If you immediately merge, there is no reason not to say git pull in the first place ;-) For this to be a good example, there needs git log -p ..origin/master before that merge happens, I would think. Not that I read the text around here and confirmed that this is a good place in the overall flow of the learning to teach about log -p and merge, though. @@ -1811,8 +1819,8 @@ manner. You can then import these into your mail client and send them by hand. However, if you have a lot to send at once, you may prefer to use the linkgit:git-send-email[1] script to automate the process. -Consult the mailing list for your project first to determine how they -prefer such patches be handled. +Consult the mailing list for your project first to determine +their requirements for submitting patches. OK. [[importing-patches]] Importing patches to a project @@ -2255,7 +2263,7 @@ $ git checkout test git merge speed-up-spinlocks It is unlikely that you would have any conflicts here ... but you might if you spent a while on this step and had also pulled new versions from upstream. -Some time later when enough time has passed and testing done, you can pull the +Sometime later when enough time has passed and testing done, you can pull the OK. -- To unsubscribe from this list: send the line 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] t9138-git-svn-authors-prog.sh fixups
Junio,
On Tue, May 27, 2014 at 12:43:06PM -0700, Junio C Hamano wrote:
Jeremiah Mahler jmmah...@gmail.com writes:
...
diff --git a/t/t9138-git-svn-authors-prog.sh
b/t/t9138-git-svn-authors-prog.sh
index 83cc5fc..d54c37a 100755
--- a/t/t9138-git-svn-authors-prog.sh
+++ b/t/t9138-git-svn-authors-prog.sh
@@ -7,40 +7,39 @@ test_description='git svn authors prog tests'
. ./lib-git-svn.sh
-cat svn-authors-prog 'EOF'
-#!/usr/bin/perl
-$_ = shift;
-if (s/-sub$//) {
- print $_ $_\@sub.example.com\n;
-}
-else {
- print $_ $_\@example.com\n;
-}
+write_script svn-authors-prog $PERL_PATH -\EOF
I think you meant to dq $PERL_PATH here. Other than that, looks
OK to me.
Thanks.
Ah, you're right, it needs the quotes. Can this minor changed be fixed
by editing the patch or should I re-send it?
--
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v11 00/41] Use ref transactions
This patch series can also be found at https://github.com/rsahlberg/git/tree/ref-transactions This patch series is based on next and expands on the transaction API. It converts all ref updates, inside refs.c as well as external, to use the transaction API for updates. This makes most of the ref updates to become atomic when there are failures locking or writing to a ref. This version completes the work to convert all ref updates to use transactions. Now that all updates are through transactions I will start working on cleaning up the reading of refs and to create an api for managing reflogs but all that will go in a different patch series. Version 11: - Updates after JNs review of the series. Ronnie Sahlberg (41): refs.c: remove ref_transaction_rollback refs.c: ref_transaction_commit should not free the transaction refs.c: constify the sha arguments for ref_transaction_create|delete|update refs.c: allow passing NULL to ref_transaction_free refs.c: add a strbuf argument to ref_transaction_commit for error logging refs.c: add an err argument to repack_without_refs refs.c: make ref_update_reject_duplicates take a strbuf argument for errors refs.c: add an err argument to delete_ref_loose refs.c: make update_ref_write update a strbuf on failure update-ref.c: log transaction error from the update_ref refs.c: remove the onerr argument to ref_transaction_commit refs.c: change ref_transaction_update() to do error checking and return status refs.c: change ref_transaction_create to do error checking and return status refs.c: update ref_transaction_delete to check for error and return status refs.c: make ref_transaction_begin take an err argument refs.c: add transaction.status and track OPEN/CLOSED/ERROR tag.c: use ref transactions when doing updates replace.c: use the ref transaction functions for updates commit.c: use ref transactions for updates sequencer.c: use ref transactions for all ref updates fast-import.c: change update_branch to use ref transactions branch.c: use ref transaction for all ref updates refs.c: change update_ref to use a transaction receive-pack.c: use a reference transaction for updating the refs fast-import.c: use a ref transaction when dumping tags walker.c: use ref transaction for ref updates refs.c: make lock_ref_sha1 static refs.c: remove the update_ref_lock function refs.c: remove the update_ref_write function refs.c: remove lock_ref_sha1 refs.c: make prune_ref use a transaction to delete the ref refs.c: make delete_ref use a transaction refs.c: pass the ref log message to _create/delete/update instead of _commit refs.c: pass NULL as *flags to read_ref_full refs.c: pack all refs before we start to rename a ref refs.c: move the check for valid refname to lock_ref_sha1_basic refs.c: call lock_ref_sha1_basic directly from commit refs.c: pass a skip list to name_conflict_fn refs.c: propagate any errno==ENOTDIR from _commit back to the callers fetch.c: change s_update_ref to use a ref transaction refs.c: make write_ref_sha1 static branch.c | 30 ++-- builtin/commit.c | 24 ++- builtin/fetch.c| 25 +-- builtin/receive-pack.c | 44 -- builtin/replace.c | 15 +- builtin/tag.c | 15 +- builtin/update-ref.c | 34 ++-- cache.h| 2 + fast-import.c | 42 +++-- lockfile.c | 21 +-- refs.c | 420 - refs.h | 80 ++ sequencer.c| 24 ++- t/t3200-branch.sh | 6 +- walker.c | 55 --- 15 files changed, 531 insertions(+), 306 deletions(-) -- 2.0.0.rc3.474.g0203784 -- To unsubscribe from this list: send the line 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 v11 34/41] refs.c: pass NULL as *flags to read_ref_full
We call read_ref_full with a pointer to flags from rename_ref but since
we never actually use the returned flags we can just pass NULL here instead.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index e485d2f..3cdfc72 100644
--- a/refs.c
+++ b/refs.c
@@ -2642,7 +2642,7 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
goto rollback;
}
- if (!read_ref_full(newrefname, sha1, 1, flag)
+ if (!read_ref_full(newrefname, sha1, 1, NULL)
delete_ref(newrefname, sha1, REF_NODEREF)) {
if (errno==EISDIR) {
if (remove_empty_directories(git_path(%s,
newrefname))) {
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 02/41] refs.c: ref_transaction_commit should not free the transaction
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 1 +
refs.c | 1 -
refs.h | 5 ++---
3 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 405267f..1fd7a89 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -369,6 +369,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
update_refs_stdin();
ret = ref_transaction_commit(transaction, msg,
UPDATE_REFS_DIE_ON_ERR);
+ ref_transaction_free(transaction);
return ret;
}
diff --git a/refs.c b/refs.c
index 48573e3..33541f4 100644
--- a/refs.c
+++ b/refs.c
@@ -3483,7 +3483,6 @@ cleanup:
if (updates[i]-lock)
unlock_ref(updates[i]-lock);
free(delnames);
- ref_transaction_free(transaction);
return ret;
}
diff --git a/refs.h b/refs.h
index a07a5d0..306d833 100644
--- a/refs.h
+++ b/refs.h
@@ -216,8 +216,7 @@ enum action_on_err {
/*
* Begin a reference transaction. The reference transaction must
- * eventually be commited using ref_transaction_commit() or freed by
- * calling ref_transaction_free().
+ * be freed by calling ref_transaction_free().
*/
struct ref_transaction *ref_transaction_begin(void);
@@ -265,7 +264,7 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
/*
* Commit all of the changes that have been queued in transaction, as
* atomically as possible. Return a nonzero value if there is a
- * problem. The ref_transaction is freed by this function.
+ * problem.
*/
int ref_transaction_commit(struct ref_transaction *transaction,
const char *msg, enum action_on_err onerr);
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 29/41] refs.c: remove the update_ref_write function
Since we only call update_ref_write from a single place and we only call it
with onerr==QUIET_ON_ERR we can just as well get rid of it and just call
write_ref_sha1 directly. This changes the return status for _commit from
1 to -1 on failures when writing to the ref. Eventually we will want
_commit to start returning more detailed error conditions than the current
simple success/failure. For example if the commit failed due to name
conflicts etc.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 35 +--
1 file changed, 9 insertions(+), 26 deletions(-)
diff --git a/refs.c b/refs.c
index 0f82cdb..979b79a 100644
--- a/refs.c
+++ b/refs.c
@@ -3281,25 +3281,6 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
return retval;
}
-static int update_ref_write(const char *action, const char *refname,
- const unsigned char *sha1, struct ref_lock *lock,
- struct strbuf *err, enum action_on_err onerr)
-{
- if (write_ref_sha1(lock, sha1, action) 0) {
- const char *str = Cannot update the ref '%s'.;
- if (err)
- strbuf_addf(err, str, refname);
-
- switch (onerr) {
- case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
- case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
- case UPDATE_REFS_QUIET_ON_ERR: break;
- }
- return 1;
- }
- return 0;
-}
-
/**
* Information needed for a single ref update. Set new_sha1 to the
* new value or to zero to delete the ref. To check the old value
@@ -3552,14 +3533,16 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (!is_null_sha1(update-new_sha1)) {
- ret = update_ref_write(msg,
- update-refname,
- update-new_sha1,
- update-lock, err,
- UPDATE_REFS_QUIET_ON_ERR);
- update-lock = NULL; /* freed by update_ref_write */
- if (ret)
+ ret = write_ref_sha1(update-lock, update-new_sha1,
+msg);
+ update-lock = NULL; /* freed by write_ref_sha1 */
+ if (ret) {
+ const char *str = Cannot update the ref '%s'.;
+
+ if (err)
+ strbuf_addf(err, str, update-refname);
goto cleanup;
+ }
}
}
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 04/41] refs.c: allow passing NULL to ref_transaction_free
Allow ref_transaction_free(NULL) as a no-op. This makes ref_transaction_free
easier to use and more similar to plain 'free'.
In particular, it lets us rollback unconditionally as part of cleanup code
after setting 'transaction = NULL' if a transaction has been committed or
rolled back already.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/refs.c b/refs.c
index a767ef6..0faed29 100644
--- a/refs.c
+++ b/refs.c
@@ -3312,6 +3312,9 @@ void ref_transaction_free(struct ref_transaction
*transaction)
{
int i;
+ if (!transaction)
+ return;
+
for (i = 0; i transaction-nr; i++)
free(transaction-updates[i]);
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 08/41] refs.c: add an err argument to delete_ref_loose
Add an err argument to delete_loose_ref so that we can pass a descriptive
error string back to the caller. Pass the err argument from transaction
commit to this function so that transaction users will have a nice error
string if the transaction failed due to delete_loose_ref.
Add a new function unlink_or_err that we can call from delete_ref_loose. This
function is similar to unlink_or_warn except that we can pass it an err
argument. If err is non-NULL the function will populate err instead of
printing a warning().
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 39 +--
1 file changed, 33 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index 4c7f4f7..891b80c 100644
--- a/refs.c
+++ b/refs.c
@@ -2491,17 +2491,43 @@ static int repack_without_ref(const char *refname)
return repack_without_refs(refname, 1, NULL);
}
-static int delete_ref_loose(struct ref_lock *lock, int flag)
+static int add_err_if_unremovable(const char *op, const char *file,
+ struct strbuf *e, int rc)
+{
+ int err = errno;
+ if (rc 0 err != ENOENT) {
+ strbuf_addf(e, unable to %s %s: %s,
+ op, file, strerror(errno));
+ errno = err;
+ }
+ return rc;
+}
+
+static int unlink_or_err(const char *file, struct strbuf *err)
+{
+ if (err)
+ return add_err_if_unremovable(unlink, file, err,
+ unlink(file));
+ else
+ return unlink_or_warn(file);
+}
+
+static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf
*err)
{
if (!(flag REF_ISPACKED) || flag REF_ISSYMREF) {
/* loose */
- int err, i = strlen(lock-lk-filename) - 5; /* .lock */
+ int res, i = strlen(lock-lk-filename) - 5; /* .lock */
lock-lk-filename[i] = 0;
- err = unlink_or_warn(lock-lk-filename);
+ res = unlink_or_err(lock-lk-filename, err);
lock-lk-filename[i] = '.';
- if (err errno != ENOENT)
+ if (res errno != ENOENT) {
+ if (err)
+ strbuf_addf(err,
+ failed to delete loose ref '%s',
+ lock-lk-filename);
return 1;
+ }
}
return 0;
}
@@ -2514,7 +2540,7 @@ int delete_ref(const char *refname, const unsigned char
*sha1, int delopt)
lock = lock_ref_sha1_basic(refname, sha1, delopt, flag);
if (!lock)
return 1;
- ret |= delete_ref_loose(lock, flag);
+ ret |= delete_ref_loose(lock, flag, NULL);
/* removing the loose one could have resurrected an earlier
* packed one. Also, if it was not loose we need to repack
@@ -3494,7 +3520,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
if (update-lock) {
delnames[delnum++] = update-lock-ref_name;
- ret |= delete_ref_loose(update-lock, update-type);
+ ret |= delete_ref_loose(update-lock, update-type,
+ err);
}
}
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 37/41] refs.c: call lock_ref_sha1_basic directly from commit
Skip using the lock_any_ref_for_update wrapper and call lock_ref_sha1_basic
directly from the commit function.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index 2952871..28138ea 100644
--- a/refs.c
+++ b/refs.c
@@ -3525,12 +3525,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i n; i++) {
struct ref_update *update = updates[i];
- update-lock = lock_any_ref_for_update(update-refname,
- (update-have_old ?
- update-old_sha1 :
- NULL),
- update-flags,
- update-type);
+ update-lock = lock_ref_sha1_basic(update-refname,
+ (update-have_old ?
+ update-old_sha1 :
+ NULL),
+ update-flags,
+ update-type);
if (!update-lock) {
if (err)
strbuf_addf(err, Cannot lock the ref '%s'.,
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 23/41] refs.c: change update_ref to use a transaction
Change the update_ref helper function to use a ref transaction internally.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 25 +
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index 9992da4..fd8f3cf 100644
--- a/refs.c
+++ b/refs.c
@@ -3474,11 +3474,28 @@ int update_ref(const char *action, const char *refname,
const unsigned char *sha1, const unsigned char *oldval,
int flags, enum action_on_err onerr)
{
- struct ref_lock *lock;
- lock = update_ref_lock(refname, oldval, flags, NULL, onerr);
- if (!lock)
+ struct ref_transaction *t;
+ struct strbuf err = STRBUF_INIT;
+
+ t = ref_transaction_begin(err);
+ if ((!t ||
+ ref_transaction_update(t, refname, sha1, oldval, flags,
+ !!oldval, err)) ||
+ (ref_transaction_commit(t, action, err) !(t = NULL))) {
+ const char *str = update_ref failed for ref '%s': %s;
+
+ ref_transaction_free(t);
+ switch (onerr) {
+ case UPDATE_REFS_MSG_ON_ERR:
+ error(str, refname, err.buf); break;
+ case UPDATE_REFS_DIE_ON_ERR:
+ die(str, refname, err.buf); break;
+ case UPDATE_REFS_QUIET_ON_ERR: break;
+ }
+ strbuf_release(err);
return 1;
- return update_ref_write(action, refname, sha1, lock, NULL, onerr);
+ }
+ return 0;
}
static int ref_update_compare(const void *r1, const void *r2)
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 20/41] sequencer.c: use ref transactions for all ref updates
Change to use ref transactions for all updates to refs.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
sequencer.c | 24
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 0a80c58..fd8acaf 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -272,23 +272,31 @@ static int error_dirty_index(struct replay_opts *opts)
static int fast_forward_to(const unsigned char *to, const unsigned char *from,
int unborn, struct replay_opts *opts)
{
- struct ref_lock *ref_lock;
+ struct ref_transaction *transaction;
struct strbuf sb = STRBUF_INIT;
- int ret;
+ struct strbuf err = STRBUF_INIT;
read_cache();
if (checkout_fast_forward(from, to, 1))
exit(1); /* the callee should have complained already */
- ref_lock = lock_any_ref_for_update(HEAD, unborn ? null_sha1 : from,
- 0, NULL);
- if (!ref_lock)
- return error(_(Failed to lock HEAD during fast_forward_to));
strbuf_addf(sb, %s: fast-forward, action_name(opts));
- ret = write_ref_sha1(ref_lock, to, sb.buf);
+
+ transaction = ref_transaction_begin(err);
+ if (!transaction ||
+ ref_transaction_update(transaction, HEAD, to, from,
+ 0, !unborn, err) ||
+ ref_transaction_commit(transaction, sb.buf, err)) {
+ ref_transaction_free(transaction);
+ error(%s, err.buf);
+ strbuf_release(sb);
+ strbuf_release(err);
+ return -1;
+ }
strbuf_release(sb);
- return ret;
+ ref_transaction_free(transaction);
+ return 0;
}
static int do_recursive_merge(struct commit *base, struct commit *next,
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 16/41] refs.c: add transaction.status and track OPEN/CLOSED/ERROR
Track the status of a transaction in a new status field. Check the field for
sanity, i.e. that status must be OPEN when _commit/_create/_delete or
_update is called or else die(BUG:...)
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 42 +-
1 file changed, 41 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 1c660e0..9992da4 100644
--- a/refs.c
+++ b/refs.c
@@ -3335,6 +3335,27 @@ struct ref_update {
};
/*
+ * Transaction states.
+ * OPEN: The transaction is in a valid state and can accept new updates.
+ * An OPEN transaction can be committed.
+ * CLOSED: If an open transaction is successfully committed the state will
+ * change to CLOSED. No further changes can be made to a CLOSED
+ * transaction.
+ * CLOSED means that all updates have been successfully committed and
+ * the only thing that remains is to free the completed transaction.
+ * ERROR: The transaction has failed. No further updates are possible
+ * and any calls except free will return an error.
+ * An ERRORed transaction can not be committed.
+ * An ERRORed transaction can be rolled back and discarded by calling
+ * by calling transaction_free.
+ */
+enum ref_transaction_state {
+ REF_TRANSACTION_OPEN = 0,
+ REF_TRANSACTION_CLOSED = 1,
+ REF_TRANSACTION_ERROR = 2,
+};
+
+/*
* Data structure for holding a reference transaction, which can
* consist of checks and updates to multiple references, carried out
* as atomically as possible. This structure is opaque to callers.
@@ -3343,6 +3364,8 @@ struct ref_transaction {
struct ref_update **updates;
size_t alloc;
size_t nr;
+ enum ref_transaction_state state;
+ int status;
};
struct ref_transaction *ref_transaction_begin(struct strbuf *err)
@@ -3385,6 +3408,9 @@ int ref_transaction_update(struct ref_transaction
*transaction,
{
struct ref_update *update;
+ if (transaction-state != REF_TRANSACTION_OPEN)
+ return -1;
+
if (have_old !old_sha1)
die(BUG: have_old is true but old_sha1 is NULL);
@@ -3405,6 +3431,9 @@ int ref_transaction_create(struct ref_transaction
*transaction,
{
struct ref_update *update;
+ if (transaction-state != REF_TRANSACTION_OPEN)
+ return -1;
+
if (!new_sha1 || is_null_sha1(new_sha1))
die(BUG: create ref with null new_sha1);
@@ -3425,6 +3454,9 @@ int ref_transaction_delete(struct ref_transaction
*transaction,
{
struct ref_update *update;
+ if (transaction-state != REF_TRANSACTION_OPEN)
+ return -1;
+
if (have_old !old_sha1)
die(BUG: have_old is true but old_sha1 is NULL);
@@ -3480,8 +3512,13 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
int n = transaction-nr;
struct ref_update **updates = transaction-updates;
- if (!n)
+ if (transaction-state != REF_TRANSACTION_OPEN)
+ return transaction-status;
+
+ if (!n) {
+ transaction-state = REF_TRANSACTION_CLOSED;
return 0;
+ }
/* Allocate work space */
delnames = xmalloc(sizeof(*delnames) * n);
@@ -3544,6 +3581,9 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
clear_loose_ref_cache(ref_cache);
cleanup:
+ transaction-state = ret ? REF_TRANSACTION_ERROR
+ : REF_TRANSACTION_CLOSED;
+
for (i = 0; i n; i++)
if (updates[i]-lock)
unlock_ref(updates[i]-lock);
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 25/41] fast-import.c: use a ref transaction when dumping tags
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
fast-import.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 4a7b196..3db5b3d 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1735,15 +1735,22 @@ static void dump_tags(void)
{
static const char *msg = fast-import;
struct tag *t;
- struct ref_lock *lock;
char ref_name[PATH_MAX];
+ struct strbuf err = STRBUF_INIT;
+ struct ref_transaction *transaction;
+ transaction = ref_transaction_begin(err);
for (t = first_tag; t; t = t-next_tag) {
- sprintf(ref_name, tags/%s, t-name);
- lock = lock_ref_sha1(ref_name, NULL);
- if (!lock || write_ref_sha1(lock, t-sha1, msg) 0)
- failure |= error(Unable to update %s, ref_name);
+ snprintf(ref_name, PATH_MAX, refs/tags/%s, t-name);
+
+ if (ref_transaction_update(transaction, ref_name, t-sha1,
+ NULL, 0, 0, err))
+ break;
}
+ if (ref_transaction_commit(transaction, msg, err))
+ failure |= error(%s, err.buf);
+ ref_transaction_free(transaction);
+ strbuf_release(err);
}
static void dump_marks_helper(FILE *f,
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 40/41] fetch.c: change s_update_ref to use a ref transaction
Change s_update_ref to use a ref transaction for the ref update.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/fetch.c | 22 ++
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index faa1233..c46ccd9 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -45,6 +45,7 @@ static struct transport *gsecondary;
static const char *submodule_prefix = ;
static const char *recurse_submodules_default;
static int shown_url = 0;
+static struct strbuf err = STRBUF_INIT;
static int option_parse_recurse_submodules(const struct option *opt,
const char *arg, int unset)
@@ -375,22 +376,27 @@ static int s_update_ref(const char *action,
{
char msg[1024];
char *rla = getenv(GIT_REFLOG_ACTION);
- static struct ref_lock *lock;
+ struct ref_transaction *transaction;
if (dry_run)
return 0;
if (!rla)
rla = default_rla.buf;
snprintf(msg, sizeof(msg), %s: %s, rla, action);
- lock = lock_any_ref_for_update(ref-name,
- check_old ? ref-old_sha1 : NULL,
- 0, NULL);
- if (!lock)
- return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
- STORE_REF_ERROR_OTHER;
- if (write_ref_sha1(lock, ref-new_sha1, msg) 0)
+
+ transaction = ref_transaction_begin(err);
+ if (!transaction ||
+ ref_transaction_update(transaction, ref-name, ref-new_sha1,
+ ref-old_sha1, 0, check_old, msg, err) ||
+ ref_transaction_commit(transaction, err)) {
+ ref_transaction_free(transaction);
+ error(%s, err.buf);
+ strbuf_release(err);
return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
STORE_REF_ERROR_OTHER;
+ }
+ strbuf_release(err);
+ ref_transaction_free(transaction);
return 0;
}
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 14/41] refs.c: update ref_transaction_delete to check for error and return status
Change ref_transaction_delete() to do basic error checking and return
non-zero of error. Update all callers to check the return for
ref_transaction_delete(). There are currently no conditions in _delete that
will return error but there will be in the future. Add an err argument that
will be updated on failure.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 5 +++--
refs.c | 16 +++-
refs.h | 12
3 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 41121fa..7c9c248 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -258,8 +258,9 @@ static const char *parse_cmd_delete(struct strbuf *input,
const char *next)
if (*next != line_termination)
die(delete %s: extra input: %s, refname, next);
- ref_transaction_delete(transaction, refname, old_sha1,
- update_flags, have_old);
+ if (ref_transaction_delete(transaction, refname, old_sha1,
+ update_flags, have_old, err))
+ die(%s, err.buf);
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index 709758d..beddce0 100644
--- a/refs.c
+++ b/refs.c
@@ -3417,19 +3417,25 @@ int ref_transaction_create(struct ref_transaction
*transaction,
return 0;
}
-void ref_transaction_delete(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *old_sha1,
- int flags, int have_old)
+int ref_transaction_delete(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *old_sha1,
+ int flags, int have_old,
+ struct strbuf *err)
{
- struct ref_update *update = add_update(transaction, refname);
+ struct ref_update *update;
+ if (have_old !old_sha1)
+ die(BUG: have_old is true but old_sha1 is NULL);
+
+ update = add_update(transaction, refname);
update-flags = flags;
update-have_old = have_old;
if (have_old) {
assert(!is_null_sha1(old_sha1));
hashcpy(update-old_sha1, old_sha1);
}
+ return 0;
}
int update_ref(const char *action, const char *refname,
diff --git a/refs.h b/refs.h
index 22b8cc3..e0ab989 100644
--- a/refs.h
+++ b/refs.h
@@ -273,11 +273,15 @@ int ref_transaction_create(struct ref_transaction
*transaction,
* Add a reference deletion to transaction. If have_old is true, then
* old_sha1 holds the value that the reference should have had before
* the update (which must not be the null SHA-1).
+ * Function returns 0 on success and non-zero on failure. A failure to delete
+ * means that the transaction as a whole has failed and will need to be
+ * rolled back.
*/
-void ref_transaction_delete(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *old_sha1,
- int flags, int have_old);
+int ref_transaction_delete(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *old_sha1,
+ int flags, int have_old,
+ struct strbuf *err);
/*
* Commit all of the changes that have been queued in transaction, as
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 09/41] refs.c: make update_ref_write update a strbuf on failure
Change update_ref_write to also update an error strbuf on failure.
This makes the error available to ref_transaction_commit callers if the
transaction failed due to update_ref_sha1/write_ref_sha1 failures.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/refs.c b/refs.c
index 891b80c..b28f7d4 100644
--- a/refs.c
+++ b/refs.c
@@ -3301,10 +3301,13 @@ static struct ref_lock *update_ref_lock(const char
*refname,
static int update_ref_write(const char *action, const char *refname,
const unsigned char *sha1, struct ref_lock *lock,
- enum action_on_err onerr)
+ struct strbuf *err, enum action_on_err onerr)
{
if (write_ref_sha1(lock, sha1, action) 0) {
const char *str = Cannot update the ref '%s'.;
+ if (err)
+ strbuf_addf(err, str, refname);
+
switch (onerr) {
case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
@@ -3425,7 +3428,7 @@ int update_ref(const char *action, const char *refname,
lock = update_ref_lock(refname, oldval, flags, NULL, onerr);
if (!lock)
return 1;
- return update_ref_write(action, refname, sha1, lock, onerr);
+ return update_ref_write(action, refname, sha1, lock, NULL, onerr);
}
static int ref_update_compare(const void *r1, const void *r2)
@@ -3507,7 +3510,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
ret = update_ref_write(msg,
update-refname,
update-new_sha1,
- update-lock, onerr);
+ update-lock, err, onerr);
update-lock = NULL; /* freed by update_ref_write */
if (ret)
goto cleanup;
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 01/41] refs.c: remove ref_transaction_rollback
We do not yet need both a rollback and a free function for transactions.
Remove ref_transaction_rollback and use ref_transaction_free instead.
At a later stage we may reintroduce a rollback function if we want to start
adding reusable transactions and similar.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 7 +--
refs.h | 16 +++-
2 files changed, 8 insertions(+), 15 deletions(-)
diff --git a/refs.c b/refs.c
index 6898263..48573e3 100644
--- a/refs.c
+++ b/refs.c
@@ -3308,7 +3308,7 @@ struct ref_transaction *ref_transaction_begin(void)
return xcalloc(1, sizeof(struct ref_transaction));
}
-static void ref_transaction_free(struct ref_transaction *transaction)
+void ref_transaction_free(struct ref_transaction *transaction)
{
int i;
@@ -3319,11 +3319,6 @@ static void ref_transaction_free(struct ref_transaction
*transaction)
free(transaction);
}
-void ref_transaction_rollback(struct ref_transaction *transaction)
-{
- ref_transaction_free(transaction);
-}
-
static struct ref_update *add_update(struct ref_transaction *transaction,
const char *refname)
{
diff --git a/refs.h b/refs.h
index 09ff483..a07a5d0 100644
--- a/refs.h
+++ b/refs.h
@@ -216,18 +216,12 @@ enum action_on_err {
/*
* Begin a reference transaction. The reference transaction must
- * eventually be commited using ref_transaction_commit() or rolled
- * back using ref_transaction_rollback().
+ * eventually be commited using ref_transaction_commit() or freed by
+ * calling ref_transaction_free().
*/
struct ref_transaction *ref_transaction_begin(void);
/*
- * Roll back a ref_transaction and free all associated data.
- */
-void ref_transaction_rollback(struct ref_transaction *transaction);
-
-
-/*
* The following functions add a reference check or update to a
* ref_transaction. In all of them, refname is the name of the
* reference to be affected. The functions make internal copies of
@@ -235,7 +229,6 @@ void ref_transaction_rollback(struct ref_transaction
*transaction);
* can be REF_NODEREF; it is passed to update_ref_lock().
*/
-
/*
* Add a reference update to transaction. new_sha1 is the value that
* the reference should have after the update, or zeros if it should
@@ -277,6 +270,11 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
int ref_transaction_commit(struct ref_transaction *transaction,
const char *msg, enum action_on_err onerr);
+/*
+ * Free an existing transaction and all associated data.
+ */
+void ref_transaction_free(struct ref_transaction *transaction);
+
/** Lock a ref and then write its file */
int update_ref(const char *action, const char *refname,
const unsigned char *sha1, const unsigned char *oldval,
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 11/41] refs.c: remove the onerr argument to ref_transaction_commit
Since all callers now use QUIET_ON_ERR we no longer need to provide an onerr
argument any more. Remove the onerr argument from the ref_transaction_commit
signature.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 3 +--
refs.c | 22 +++---
refs.h | 3 +--
3 files changed, 9 insertions(+), 19 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index aec9004..88ab785 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -366,8 +366,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
if (end_null)
line_termination = '\0';
update_refs_stdin();
- if (ref_transaction_commit(transaction, msg, err,
- UPDATE_REFS_QUIET_ON_ERR))
+ if (ref_transaction_commit(transaction, msg, err))
die(%s, err.buf);
ref_transaction_free(transaction);
return 0;
diff --git a/refs.c b/refs.c
index b28f7d4..00b6e9a 100644
--- a/refs.c
+++ b/refs.c
@@ -3439,8 +3439,7 @@ static int ref_update_compare(const void *r1, const void
*r2)
}
static int ref_update_reject_duplicates(struct ref_update **updates, int n,
- struct strbuf *err,
- enum action_on_err onerr)
+ struct strbuf *err)
{
int i;
for (i = 1; i n; i++)
@@ -3450,22 +3449,13 @@ static int ref_update_reject_duplicates(struct
ref_update **updates, int n,
if (err)
strbuf_addf(err, str, updates[i]-refname);
- switch (onerr) {
- case UPDATE_REFS_MSG_ON_ERR:
- error(str, updates[i]-refname); break;
- case UPDATE_REFS_DIE_ON_ERR:
- die(str, updates[i]-refname); break;
- case UPDATE_REFS_QUIET_ON_ERR:
- break;
- }
return 1;
}
return 0;
}
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, struct strbuf *err,
- enum action_on_err onerr)
+ const char *msg, struct strbuf *err)
{
int ret = 0, delnum = 0, i;
const char **delnames;
@@ -3480,7 +3470,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Copy, sort, and reject duplicate refs */
qsort(updates, n, sizeof(*updates), ref_update_compare);
- ret = ref_update_reject_duplicates(updates, n, err, onerr);
+ ret = ref_update_reject_duplicates(updates, n, err);
if (ret)
goto cleanup;
@@ -3492,7 +3482,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
(update-have_old ?
update-old_sha1 : NULL),
update-flags,
- update-type, onerr);
+ update-type,
+ UPDATE_REFS_QUIET_ON_ERR);
if (!update-lock) {
if (err)
strbuf_addf(err, Cannot lock the ref '%s'.,
@@ -3510,7 +3501,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
ret = update_ref_write(msg,
update-refname,
update-new_sha1,
- update-lock, err, onerr);
+ update-lock, err,
+ UPDATE_REFS_QUIET_ON_ERR);
update-lock = NULL; /* freed by update_ref_write */
if (ret)
goto cleanup;
diff --git a/refs.h b/refs.h
index 94d4cd4..6ee9c9e 100644
--- a/refs.h
+++ b/refs.h
@@ -270,8 +270,7 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
* the transaction failed. The string does not end in newline.
*/
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, struct strbuf *err,
- enum action_on_err onerr);
+ const char *msg, struct strbuf *err);
/*
* Free an existing transaction and all associated data.
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at
[PATCH v11 24/41] receive-pack.c: use a reference transaction for updating the refs
Wrap all the ref updates inside a transaction.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/receive-pack.c | 43 ++-
1 file changed, 34 insertions(+), 9 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c323081..c88dc03 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -46,6 +46,8 @@ static void *head_name_to_free;
static int sent_capabilities;
static int shallow_update;
static const char *alt_shallow_file;
+static int num_strings;
+static const char **error_strings;
static enum deny_action parse_deny_action(const char *var, const char *value)
{
@@ -468,6 +470,13 @@ static int update_shallow_ref(struct command *cmd, struct
shallow_info *si)
return 0;
}
+static const char *add_error_string(const char *str)
+{
+ error_strings = xrealloc(error_strings,
+sizeof(*error_strings) * ++num_strings);
+ return error_strings[num_strings - 1] = xstrdup(str);
+}
+
static const char *update(struct command *cmd, struct shallow_info *si)
{
const char *name = cmd-ref_name;
@@ -475,7 +484,6 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
const char *namespaced_name;
unsigned char *old_sha1 = cmd-old_sha1;
unsigned char *new_sha1 = cmd-new_sha1;
- struct ref_lock *lock;
/* only refs/... are allowed */
if (!starts_with(name, refs/) || check_refname_format(name + 5, 0)) {
@@ -576,19 +584,27 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
return NULL; /* good */
}
else {
+ struct strbuf err = STRBUF_INIT;
+ struct ref_transaction *transaction;
+
if (shallow_update si-shallow_ref[cmd-index]
update_shallow_ref(cmd, si))
return shallow error;
- lock = lock_any_ref_for_update(namespaced_name, old_sha1,
- 0, NULL);
- if (!lock) {
- rp_error(failed to lock %s, name);
- return failed to lock;
- }
- if (write_ref_sha1(lock, new_sha1, push)) {
- return failed to write; /* error() already called */
+ transaction = ref_transaction_begin(err);
+ if (!transaction ||
+ ref_transaction_update(transaction, namespaced_name,
+ new_sha1, old_sha1, 0, 1, err) ||
+ ref_transaction_commit(transaction, push, err)) {
+ const char *str = add_error_string(err.buf);
+ ref_transaction_free(transaction);
+ strbuf_release(err);
+ rp_error(%s, str);
+ return str;
}
+
+ ref_transaction_free(transaction);
+ strbuf_release(err);
return NULL; /* good */
}
}
@@ -1120,6 +1136,14 @@ static int delete_only(struct command *commands)
return 1;
}
+static void free_all_strings(void)
+{
+ int i;
+ for (i = 0; i num_strings; i++)
+ free((void *)error_strings[i]);
+ free(error_strings);
+}
+
int cmd_receive_pack(int argc, const char **argv, const char *prefix)
{
int advertise_refs = 0;
@@ -1166,6 +1190,7 @@ int cmd_receive_pack(int argc, const char **argv, const
char *prefix)
usage(receive_pack_usage);
setup_path();
+ atexit(free_all_strings);
if (!enter_repo(dir, 0))
die('%s' does not appear to be a git repository, dir);
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 28/41] refs.c: remove the update_ref_lock function
Since we now only call update_ref_lock with onerr==QUIET_ON_ERR we no longer
need this function and can replace it with just calling lock_any_ref_for_update
directly.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 30 ++
1 file changed, 6 insertions(+), 24 deletions(-)
diff --git a/refs.c b/refs.c
index 01d8ac4..0f82cdb 100644
--- a/refs.c
+++ b/refs.c
@@ -3281,24 +3281,6 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
return retval;
}
-static struct ref_lock *update_ref_lock(const char *refname,
- const unsigned char *oldval,
- int flags, int *type_p,
- enum action_on_err onerr)
-{
- struct ref_lock *lock;
- lock = lock_any_ref_for_update(refname, oldval, flags, type_p);
- if (!lock) {
- const char *str = Cannot lock the ref '%s'.;
- switch (onerr) {
- case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
- case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
- case UPDATE_REFS_QUIET_ON_ERR: break;
- }
- }
- return lock;
-}
-
static int update_ref_write(const char *action, const char *refname,
const unsigned char *sha1, struct ref_lock *lock,
struct strbuf *err, enum action_on_err onerr)
@@ -3550,12 +3532,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i n; i++) {
struct ref_update *update = updates[i];
- update-lock = update_ref_lock(update-refname,
- (update-have_old ?
- update-old_sha1 : NULL),
- update-flags,
- update-type,
- UPDATE_REFS_QUIET_ON_ERR);
+ update-lock = lock_any_ref_for_update(update-refname,
+ (update-have_old ?
+ update-old_sha1 :
+ NULL),
+ update-flags,
+ update-type);
if (!update-lock) {
if (err)
strbuf_addf(err, Cannot lock the ref '%s'.,
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 27/41] refs.c: make lock_ref_sha1 static
No external callers reference lock_ref_sha1 any more so lets declare it static.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 2 +-
refs.h | 3 ---
2 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index fd8f3cf..01d8ac4 100644
--- a/refs.c
+++ b/refs.c
@@ -2124,7 +2124,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
return NULL;
}
-struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char
*old_sha1)
+static struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char
*old_sha1)
{
char refpath[PATH_MAX];
if (check_refname_format(refname, 0))
diff --git a/refs.h b/refs.h
index 6c830f2..1e25e1c 100644
--- a/refs.h
+++ b/refs.h
@@ -132,9 +132,6 @@ extern int ref_exists(const char *);
*/
extern int peel_ref(const char *refname, unsigned char *sha1);
-/** Locks a refs/ ref returning the lock on success and NULL on failure. **/
-extern struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char
*old_sha1);
-
/** Locks any ref (for 'HEAD' type refs). */
#define REF_NODEREF0x01
extern struct ref_lock *lock_any_ref_for_update(const char *refname,
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 36/41] refs.c: move the check for valid refname to lock_ref_sha1_basic
Move the check for check_refname_format from lock_any_ref_for_update
to lock_ref_sha1_basic. At some later stage we will get rid of
lock_any_ref_for_update completely.
This leaves lock_any_ref_for_updates as a no-op wrapper which could be removed.
But this wrapper is also called from an external caller and we will soon
make changes to the signature to lock_ref_sha1_basic that we do not want to
expose to that caller.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index 08dde5b..2952871 100644
--- a/refs.c
+++ b/refs.c
@@ -2043,6 +2043,9 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
int missing = 0;
int attempts_remaining = 3;
+ if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
+ return NULL;
+
lock = xcalloc(1, sizeof(struct ref_lock));
lock-lock_fd = -1;
@@ -2134,8 +2137,6 @@ struct ref_lock *lock_any_ref_for_update(const char
*refname,
const unsigned char *old_sha1,
int flags, int *type_p)
{
- if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
- return NULL;
return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
}
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 39/41] refs.c: propagate any errno==ENOTDIR from _commit back to the callers
In _commit, ENOTDIR can happen in the call to lock_ref_sha1_basic, either when
we lstat the new refname and it returns ENOTDIR or if the name checking
function reports that the same type of conflict happened. In both cases it
means that we can not create the new ref due to a name conflict.
For these cases, save the errno value and abort and make sure that the caller
can see errno==ENOTDIR.
Also start defining specific return codes for _commit, assign -1 as a generic
error and -2 as the error that refers to a name conflict. Callers can (and
should) use that return value inspecting errno directly.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 23 ---
refs.h | 4
2 files changed, 20 insertions(+), 7 deletions(-)
diff --git a/refs.c b/refs.c
index d3812b7..c108007 100644
--- a/refs.c
+++ b/refs.c
@@ -3517,7 +3517,7 @@ static int ref_update_reject_duplicates(struct ref_update
**updates, int n,
int ref_transaction_commit(struct ref_transaction *transaction,
struct strbuf *err)
{
- int ret = 0, delnum = 0, i;
+ int ret = 0, delnum = 0, i, save_errno = 0;
const char **delnames;
int n = transaction-nr;
struct ref_update **updates = transaction-updates;
@@ -3535,9 +3535,10 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Copy, sort, and reject duplicate refs */
qsort(updates, n, sizeof(*updates), ref_update_compare);
- ret = ref_update_reject_duplicates(updates, n, err);
- if (ret)
+ if (ref_update_reject_duplicates(updates, n, err)) {
+ ret = -1;
goto cleanup;
+ }
/* Acquire all locks while verifying old values */
for (i = 0; i n; i++) {
@@ -3551,10 +3552,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
update-type,
delnames, delnum);
if (!update-lock) {
+ if (errno == ENOTDIR)
+ save_errno = errno;
if (err)
strbuf_addf(err, Cannot lock the ref '%s'.,
update-refname);
- ret = 1;
+ ret = -1;
goto cleanup;
}
}
@@ -3572,6 +3575,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
if (err)
strbuf_addf(err, str, update-refname);
+ ret = -1;
goto cleanup;
}
}
@@ -3582,14 +3586,16 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (update-lock) {
- ret |= delete_ref_loose(update-lock, update-type,
- err);
+ if (delete_ref_loose(update-lock, update-type, err))
+ ret = -1;
+
if (!(update-flags REF_ISPRUNING))
delnames[delnum++] = update-lock-ref_name;
}
}
- ret |= repack_without_refs(delnames, delnum, err);
+ if (repack_without_refs(delnames, delnum, err))
+ ret = -1;
for (i = 0; i delnum; i++)
unlink_or_warn(git_path(logs/%s, delnames[i]));
clear_loose_ref_cache(ref_cache);
@@ -3602,6 +3608,9 @@ cleanup:
if (updates[i]-lock)
unlock_ref(updates[i]-lock);
free(delnames);
+ errno = save_errno;
+ if (save_errno == ENOTDIR)
+ ret = -2;
return ret;
}
diff --git a/refs.h b/refs.h
index 84e12b5..2823b72 100644
--- a/refs.h
+++ b/refs.h
@@ -289,6 +289,10 @@ int ref_transaction_delete(struct ref_transaction
*transaction,
* Commit all of the changes that have been queued in transaction, as
* atomically as possible. Return a nonzero value if there is a
* problem.
+ * If the transaction is already in failed state this function will return
+ * an error.
+ * Function returns 0 on success, -1 for generic failures and -2 if the
+ * failure was due to a name collision (ENOTDIR).
*/
int ref_transaction_commit(struct ref_transaction *transaction,
struct strbuf *err);
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 17/41] tag.c: use ref transactions when doing updates
Change tag.c to use ref transactions for all ref updates.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/tag.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index c6e8a71..c9bfc9a 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -548,7 +548,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
struct strbuf ref = STRBUF_INIT;
unsigned char object[20], prev[20];
const char *object_ref, *tag;
- struct ref_lock *lock;
struct create_tag_options opt;
char *cleanup_arg = NULL;
int annotate = 0, force = 0, lines = -1;
@@ -556,6 +555,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
const char *msgfile = NULL, *keyid = NULL;
struct msg_arg msg = { 0, STRBUF_INIT };
struct commit_list *with_commit = NULL;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
struct option options[] = {
OPT_CMDMODE('l', list, cmdmode, N_(list tag names), 'l'),
{ OPTION_INTEGER, 'n', NULL, lines, N_(n),
@@ -701,11 +702,13 @@ int cmd_tag(int argc, const char **argv, const char
*prefix)
if (annotate)
create_tag(object, tag, buf, opt, prev, object);
- lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL);
- if (!lock)
- die(_(%s: cannot lock the ref), ref.buf);
- if (write_ref_sha1(lock, object, NULL) 0)
- die(_(%s: cannot update the ref), ref.buf);
+ transaction = ref_transaction_begin(err);
+ if (!transaction ||
+ ref_transaction_update(transaction, ref.buf, object, prev,
+ 0, !is_null_sha1(prev), err) ||
+ ref_transaction_commit(transaction, NULL, err))
+ die(%s, err.buf);
+ ref_transaction_free(transaction);
if (force !is_null_sha1(prev) hashcmp(prev, object))
printf(_(Updated tag '%s' (was %s)\n), tag,
find_unique_abbrev(prev, DEFAULT_ABBREV));
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 18/41] replace.c: use the ref transaction functions for updates
Update replace.c to use ref transactions for updates. Reviewed-by: Jonathan Nieder jrnie...@gmail.com Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- builtin/replace.c | 15 +-- 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/builtin/replace.c b/builtin/replace.c index 3da1bae..f24d814 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -133,7 +133,8 @@ static int replace_object_sha1(const char *object_ref, unsigned char prev[20]; enum object_type obj_type, repl_type; char ref[PATH_MAX]; - struct ref_lock *lock; + struct ref_transaction *transaction; + struct strbuf err = STRBUF_INIT; if (snprintf(ref, sizeof(ref), refs/replace/%s, @@ -156,12 +157,14 @@ static int replace_object_sha1(const char *object_ref, else if (!force) die(replace ref '%s' already exists, ref); - lock = lock_any_ref_for_update(ref, prev, 0, NULL); - if (!lock) - die(%s: cannot lock the ref, ref); - if (write_ref_sha1(lock, repl, NULL) 0) - die(%s: cannot update the ref, ref); + transaction = ref_transaction_begin(err); + if (!transaction || + ref_transaction_update(transaction, ref, repl, prev, + 0, !is_null_sha1(prev), err) || + ref_transaction_commit(transaction, NULL, err)) + die(%s, err.buf); + ref_transaction_free(transaction); return 0; } -- 2.0.0.rc3.474.g0203784 -- To unsubscribe from this list: send the line 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 v11 32/41] refs.c: make delete_ref use a transaction
Change delete_ref to use a ref transaction for the deletion. At the same time
since we no longer have any callers of repack_without_ref we can now delete
this function.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 34 +-
1 file changed, 13 insertions(+), 21 deletions(-)
diff --git a/refs.c b/refs.c
index 1819434..88e7bb6 100644
--- a/refs.c
+++ b/refs.c
@@ -2494,11 +2494,6 @@ static int repack_without_refs(const char **refnames,
int n, struct strbuf *err)
return ret;
}
-static int repack_without_ref(const char *refname)
-{
- return repack_without_refs(refname, 1, NULL);
-}
-
static int add_err_if_unremovable(const char *op, const char *file,
struct strbuf *e, int rc)
{
@@ -2542,24 +2537,21 @@ static int delete_ref_loose(struct ref_lock *lock, int
flag, struct strbuf *err)
int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
{
- struct ref_lock *lock;
- int ret = 0, flag = 0;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
- lock = lock_ref_sha1_basic(refname, sha1, delopt, flag);
- if (!lock)
+ transaction = ref_transaction_begin(err);
+ if (!transaction ||
+ ref_transaction_delete(transaction, refname, sha1, delopt,
+ sha1 !is_null_sha1(sha1), err) ||
+ ref_transaction_commit(transaction, NULL, err)) {
+ error(%s, err.buf);
+ ref_transaction_free(transaction);
+ strbuf_release(err);
return 1;
- ret |= delete_ref_loose(lock, flag, NULL);
-
- /* removing the loose one could have resurrected an earlier
-* packed one. Also, if it was not loose we need to repack
-* without it.
-*/
- ret |= repack_without_ref(lock-ref_name);
-
- unlink_or_warn(git_path(logs/%s, lock-ref_name));
- clear_loose_ref_cache(ref_cache);
- unlock_ref(lock);
- return ret;
+ }
+ ref_transaction_free(transaction);
+ return 0;
}
/*
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 12/41] refs.c: change ref_transaction_update() to do error checking and return status
Update ref_transaction_update() do some basic error checking and return
non-zero on error. Update all callers to check ref_transaction_update() for
error. There are currently no conditions in _update that will return error but
there will be in the future. Add an err argument that will be updated on
failure. In future patches we will start doing both locking and checking
for name conflicts in _update instead of _commit at which time this function
will start returning errors for these conditions.
Also check for BUGs during update and die(BUG:...) if we are calling
_update with have_old but the old_sha1 pointer is NULL.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 12 +++-
refs.c | 18 --
refs.h | 14 +-
3 files changed, 28 insertions(+), 16 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 88ab785..3067b11 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -16,6 +16,7 @@ static struct ref_transaction *transaction;
static char line_termination = '\n';
static int update_flags;
+static struct strbuf err = STRBUF_INIT;
/*
* Parse one whitespace- or NUL-terminated, possibly C-quoted argument
@@ -197,8 +198,9 @@ static const char *parse_cmd_update(struct strbuf *input,
const char *next)
if (*next != line_termination)
die(update %s: extra input: %s, refname, next);
- ref_transaction_update(transaction, refname, new_sha1, old_sha1,
- update_flags, have_old);
+ if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+ update_flags, have_old, err))
+ die(%s, err.buf);
update_flags = 0;
free(refname);
@@ -286,8 +288,9 @@ static const char *parse_cmd_verify(struct strbuf *input,
const char *next)
if (*next != line_termination)
die(verify %s: extra input: %s, refname, next);
- ref_transaction_update(transaction, refname, new_sha1, old_sha1,
- update_flags, have_old);
+ if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+ update_flags, have_old, err))
+ die(%s, err.buf);
update_flags = 0;
free(refname);
@@ -342,7 +345,6 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
const char *refname, *oldval, *msg = NULL;
unsigned char sha1[20], oldsha1[20];
int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;
- struct strbuf err = STRBUF_INIT;
struct option options[] = {
OPT_STRING( 'm', NULL, msg, N_(reason), N_(reason of the
update)),
OPT_BOOL('d', NULL, delete, N_(delete the reference)),
diff --git a/refs.c b/refs.c
index 00b6e9a..79a4477 100644
--- a/refs.c
+++ b/refs.c
@@ -3376,19 +3376,25 @@ static struct ref_update *add_update(struct
ref_transaction *transaction,
return update;
}
-void ref_transaction_update(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *new_sha1,
- const unsigned char *old_sha1,
- int flags, int have_old)
+int ref_transaction_update(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *new_sha1,
+ const unsigned char *old_sha1,
+ int flags, int have_old,
+ struct strbuf *err)
{
- struct ref_update *update = add_update(transaction, refname);
+ struct ref_update *update;
+
+ if (have_old !old_sha1)
+ die(BUG: have_old is true but old_sha1 is NULL);
+ update = add_update(transaction, refname);
hashcpy(update-new_sha1, new_sha1);
update-flags = flags;
update-have_old = have_old;
if (have_old)
hashcpy(update-old_sha1, old_sha1);
+ return 0;
}
void ref_transaction_create(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index 6ee9c9e..32edf3f 100644
--- a/refs.h
+++ b/refs.h
@@ -234,12 +234,16 @@ struct ref_transaction *ref_transaction_begin(void);
* be deleted. If have_old is true, then old_sha1 holds the value
* that the reference should have had before the update, or zeros if
* it must not have existed beforehand.
+ * Function returns 0 on success and non-zero on failure. A failure to update
+ * means that the transaction as a whole has failed and will need to be
+ * rolled back. On failure the err buffer will be updated.
*/
-void ref_transaction_update(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char
[PATCH v11 38/41] refs.c: pass a skip list to name_conflict_fn
Allow passing a list of refs to skip checking to name_conflict_fn.
There are some conditions where we want to allow a temporary conflict and skip
checking those refs. For example if we have a transaction that
1, guarantees that m is a packed refs and there is no loose ref for m
2, the transaction will delete m from the packed ref
3, the transaction will create conflicting m/m
For this case we want to be able to lock and create m/m since we know that the
conflict is only transient. I.e. the conflict will be automatically resolved
by the transaction when it deletes m.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 39 +--
1 file changed, 29 insertions(+), 10 deletions(-)
diff --git a/refs.c b/refs.c
index 28138ea..d3812b7 100644
--- a/refs.c
+++ b/refs.c
@@ -793,11 +793,17 @@ struct name_conflict_cb {
const char *refname;
const char *oldrefname;
const char *conflicting_refname;
+ const char **skip;
+ int skipnum;
};
static int name_conflict_fn(struct ref_entry *entry, void *cb_data)
{
struct name_conflict_cb *data = (struct name_conflict_cb *)cb_data;
+ int i;
+ for (i = 0; i data-skipnum; i++)
+ if (!strcmp(entry-name, data-skip[i]))
+ return 0;
if (data-oldrefname !strcmp(data-oldrefname, entry-name))
return 0;
if (names_conflict(data-refname, entry-name)) {
@@ -812,15 +818,19 @@ static int name_conflict_fn(struct ref_entry *entry, void
*cb_data)
* conflicting with the name of an existing reference in dir. If
* oldrefname is non-NULL, ignore potential conflicts with oldrefname
* (e.g., because oldrefname is scheduled for deletion in the same
- * operation).
+ * operation). skip contains a list of refs we want to skip checking for
+ * conflicts with.
*/
static int is_refname_available(const char *refname, const char *oldrefname,
- struct ref_dir *dir)
+ struct ref_dir *dir,
+ const char **skip, int skipnum)
{
struct name_conflict_cb data;
data.refname = refname;
data.oldrefname = oldrefname;
data.conflicting_refname = NULL;
+ data.skip = skip;
+ data.skipnum = skipnum;
sort_ref_dir(dir);
if (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, data)) {
@@ -2032,7 +2042,8 @@ int dwim_log(const char *str, int len, unsigned char
*sha1, char **log)
static struct ref_lock *lock_ref_sha1_basic(const char *refname,
const unsigned char *old_sha1,
- int flags, int *type_p)
+ int flags, int *type_p,
+ const char **skip, int skipnum)
{
char *ref_file;
const char *orig_refname = refname;
@@ -2079,7 +2090,9 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
* name is a proper prefix of our refname.
*/
if (missing
-!is_refname_available(refname, NULL, get_packed_refs(ref_cache)))
{
+!is_refname_available(refname, NULL,
+ get_packed_refs(ref_cache),
+ skip, skipnum)) {
last_errno = ENOTDIR;
goto error_return;
}
@@ -2137,7 +2150,7 @@ struct ref_lock *lock_any_ref_for_update(const char
*refname,
const unsigned char *old_sha1,
int flags, int *type_p)
{
- return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
+ return lock_ref_sha1_basic(refname, old_sha1, flags, type_p, NULL, 0);
}
/*
@@ -2618,6 +2631,9 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
int log = !lstat(git_path(logs/%s, oldrefname), loginfo);
const char *symref = NULL;
+ if (!strcmp(oldrefname, newrefname))
+ return 0;
+
if (log S_ISLNK(loginfo.st_mode))
return error(reflog for %s is a symlink, oldrefname);
@@ -2628,10 +2644,12 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
if (!symref)
return error(refname %s not found, oldrefname);
- if (!is_refname_available(newrefname, oldrefname,
get_packed_refs(ref_cache)))
+ if (!is_refname_available(newrefname, oldrefname,
+ get_packed_refs(ref_cache), NULL, 0))
return 1;
- if (!is_refname_available(newrefname, oldrefname,
get_loose_refs(ref_cache)))
+ if (!is_refname_available(newrefname, oldrefname,
+ get_loose_refs(ref_cache), NULL, 0))
return 1;
if (log rename(git_path(logs/%s, oldrefname),
[PATCH v11 35/41] refs.c: pack all refs before we start to rename a ref
This means that most loose refs will no longer be present after the rename
which triggered a test failure since it assumes the file for an unrelated
ref would still be present after the rename.
We want to do this to make it easier to handle atomic renames in rename_ref for
the case 'git branch -m foo/bar foo'. If we can guarantee that foo/bar does not
exist as a loose ref we can avoid locking foo/bar.lock during transaction
commit and thus make it possible to delete the foo directory and re-create
it as a file(branch) in a single transaction.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c| 3 +++
t/t3200-branch.sh | 6 +++---
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/refs.c b/refs.c
index 3cdfc72..08dde5b 100644
--- a/refs.c
+++ b/refs.c
@@ -2637,6 +2637,9 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
return error(unable to move logfile logs/%s to
TMP_RENAMED_LOG: %s,
oldrefname, strerror(errno));
+ if (pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE))
+ return error(unable to pack refs);
+
if (delete_ref(oldrefname, orig_sha1, REF_NODEREF)) {
error(unable to delete old %s, oldrefname);
goto rollback;
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index ac31b71..de0c2b9 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -288,9 +288,9 @@ test_expect_success 'deleting a dangling symref' '
test_expect_success 'renaming a symref is not allowed' '
git symbolic-ref refs/heads/master2 refs/heads/master
test_must_fail git branch -m master2 master3
- git symbolic-ref refs/heads/master2
- test_path_is_file .git/refs/heads/master
- test_path_is_missing .git/refs/heads/master3
+ git rev-parse --verify refs/heads/master
+ test_must_fail git symbolic-ref refs/heads/master3
+ test_must_fail git rev-parse refs/heads/master3
'
test_expect_success SYMLINKS 'git branch -m u v should fail when the reflog
for u is a symlink' '
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 19/41] commit.c: use ref transactions for updates
Change commit.c to use ref transactions for all ref updates.
Make sure we pass a NULL pointer to ref_transaction_update if have_old
is false.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/commit.c | 24 +++-
1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index d28505a..7db194f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1581,11 +1581,12 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
const char *index_file, *reflog_msg;
char *nl;
unsigned char sha1[20];
- struct ref_lock *ref_lock;
struct commit_list *parents = NULL, **pptr = parents;
struct stat statbuf;
struct commit *current_head = NULL;
struct commit_extra_header *extra = NULL;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
if (argc == 2 !strcmp(argv[1], -h))
usage_with_options(builtin_commit_usage,
builtin_commit_options);
@@ -1707,16 +1708,6 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
strbuf_release(author_ident);
free_commit_extra_headers(extra);
- ref_lock = lock_any_ref_for_update(HEAD,
- !current_head
- ? NULL
- : current_head-object.sha1,
- 0, NULL);
- if (!ref_lock) {
- rollback_index_files();
- die(_(cannot lock HEAD ref));
- }
-
nl = strchr(sb.buf, '\n');
if (nl)
strbuf_setlen(sb, nl + 1 - sb.buf);
@@ -1725,10 +1716,17 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
strbuf_insert(sb, 0, reflog_msg, strlen(reflog_msg));
strbuf_insert(sb, strlen(reflog_msg), : , 2);
- if (write_ref_sha1(ref_lock, sha1, sb.buf) 0) {
+ transaction = ref_transaction_begin(err);
+ if (!transaction ||
+ ref_transaction_update(transaction, HEAD, sha1,
+ current_head ?
+ current_head-object.sha1 : NULL,
+ 0, !!current_head, err) ||
+ ref_transaction_commit(transaction, sb.buf, err)) {
rollback_index_files();
- die(_(cannot update HEAD ref));
+ die(%s, err.buf);
}
+ ref_transaction_free(transaction);
unlink(git_path(CHERRY_PICK_HEAD));
unlink(git_path(REVERT_HEAD));
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 30/41] refs.c: remove lock_ref_sha1
lock_ref_sha1 was only called from one place in refc.c and only provided
a check that the refname was sane before adding back the initial refs/
part of the ref path name, the initial refs/ that this caller had already
stripped off before calling lock_ref_sha1.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 15 +--
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/refs.c b/refs.c
index 979b79a..4ca84f7 100644
--- a/refs.c
+++ b/refs.c
@@ -2124,15 +2124,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
return NULL;
}
-static struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char
*old_sha1)
-{
- char refpath[PATH_MAX];
- if (check_refname_format(refname, 0))
- return NULL;
- strcpy(refpath, mkpath(refs/%s, refname));
- return lock_ref_sha1_basic(refpath, old_sha1, 0, NULL);
-}
-
struct ref_lock *lock_any_ref_for_update(const char *refname,
const unsigned char *old_sha1,
int flags, int *type_p)
@@ -2337,8 +2328,12 @@ static void try_remove_empty_parents(char *name)
/* make sure nobody touched the ref, and unlink */
static void prune_ref(struct ref_to_prune *r)
{
- struct ref_lock *lock = lock_ref_sha1(r-name + 5, r-sha1);
+ struct ref_lock *lock;
+
+ if (check_refname_format(r-name + 5, 0))
+ return;
+ lock = lock_ref_sha1_basic(r-name, r-sha1, 0, NULL);
if (lock) {
unlink_or_warn(git_path(%s, r-name));
unlock_ref(lock);
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 26/41] walker.c: use ref transaction for ref updates
Switch to using ref transactions in walker_fetch(). As part of the refactoring
to use ref transactions we also fix a potential memory leak where in the
original code if write_ref_sha1() would fail we would end up returning from
the function without free()ing the msg string.
This changes the locking slightly for walker_fetch. Previously the code would
lock all refs before writing them but now we do not lock the refs until the
commit stage. There is thus a very short window where changes could be done
locally during the fetch which would be overwritten when the fetch completes
and commits its transaction. But this window should be reasonably short.
Even if this race does trigger, since both the old code and the new code
just overwrites the refs to the new values without checking or comparing
them with the previous value, this is not too dissimilar to a similar scenario
where you first do a ref change locally and then later do a fetch that
overwrites the local change. With this in mind I do not see the change in
locking semantics to be critical.
Note that this function is only called when fetching from a remote HTTP
repository onto the local (most of the time single-user) repository which
likely means that the type of collissions that the previous locking would
protect against and cause the fetch to fail for to be even more rare.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
walker.c | 56 +---
1 file changed, 33 insertions(+), 23 deletions(-)
diff --git a/walker.c b/walker.c
index 1dd86b8..51ce1c6 100644
--- a/walker.c
+++ b/walker.c
@@ -251,39 +251,37 @@ void walker_targets_free(int targets, char **target,
const char **write_ref)
int walker_fetch(struct walker *walker, int targets, char **target,
const char **write_ref, const char *write_ref_log_details)
{
- struct ref_lock **lock = xcalloc(targets, sizeof(struct ref_lock *));
+ struct strbuf ref_name = STRBUF_INIT;
+ struct strbuf err = STRBUF_INIT;
+ struct ref_transaction *transaction;
unsigned char *sha1 = xmalloc(targets * 20);
char *msg;
- int ret;
int i;
save_commit_buffer = 0;
- for (i = 0; i targets; i++) {
- if (!write_ref || !write_ref[i])
- continue;
-
- lock[i] = lock_ref_sha1(write_ref[i], NULL);
- if (!lock[i]) {
- error(Can't lock ref %s, write_ref[i]);
- goto unlock_and_fail;
+ if (write_ref) {
+ transaction = ref_transaction_begin(err);
+ if (!transaction) {
+ error(%s, err.buf);
+ strbuf_release(err);
+ return -1;
}
}
-
if (!walker-get_recover)
for_each_ref(mark_complete, NULL);
for (i = 0; i targets; i++) {
if (interpret_target(walker, target[i], sha1[20 * i])) {
error(Could not interpret response from server '%s' as
something to pull, target[i]);
- goto unlock_and_fail;
+ goto rollback_and_fail;
}
if (process(walker, lookup_unknown_object(sha1[20 * i])))
- goto unlock_and_fail;
+ goto rollback_and_fail;
}
if (loop(walker))
- goto unlock_and_fail;
+ goto rollback_and_fail;
if (write_ref_log_details) {
msg = xmalloc(strlen(write_ref_log_details) + 12);
@@ -294,19 +292,31 @@ int walker_fetch(struct walker *walker, int targets, char
**target,
for (i = 0; i targets; i++) {
if (!write_ref || !write_ref[i])
continue;
- ret = write_ref_sha1(lock[i], sha1[20 * i], msg ? msg : fetch
(unknown));
- lock[i] = NULL;
- if (ret)
- goto unlock_and_fail;
+ strbuf_reset(ref_name);
+ strbuf_addf(ref_name, refs/%s, write_ref[i]);
+ if (ref_transaction_update(transaction, ref_name.buf,
+ sha1[20 * i], NULL, 0, 0,
+ err))
+ break;
+ }
+ if (write_ref) {
+ if (ref_transaction_commit(transaction,
+ msg ? msg : fetch (unknown),
+ err)) {
+ error(%s, err.buf);
+ ref_transaction_free(transaction);
+ goto rollback_and_fail;
+ }
+ ref_transaction_free(transaction);
}
- free(msg);
+ free(msg);
return 0;
-unlock_and_fail:
- for (i = 0; i targets; i++)
- if (lock[i])
- unlock_ref(lock[i]);
[PATCH v11 31/41] refs.c: make prune_ref use a transaction to delete the ref
Change prune_ref to delete the ref using a ref transaction. To do this we also
need to add a new flag REF_ISPRUNING that will tell the transaction that we
do not want to delete this ref from the packed refs. This flag is private to
refs.c and not exposed to external callers.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 28 +---
refs.h | 5 +
2 files changed, 26 insertions(+), 7 deletions(-)
diff --git a/refs.c b/refs.c
index 4ca84f7..1819434 100644
--- a/refs.c
+++ b/refs.c
@@ -30,6 +30,12 @@ static inline int bad_ref_char(int ch)
}
/*
+ * Used as a flag to ref_transaction_delete when a loose ref is being
+ * pruned.
+ */
+#define REF_ISPRUNING 0x0100
+
+/*
* Try to read one refname component from the front of refname. Return
* the length of the component found, or -1 if the component is not
* legal.
@@ -2328,17 +2334,24 @@ static void try_remove_empty_parents(char *name)
/* make sure nobody touched the ref, and unlink */
static void prune_ref(struct ref_to_prune *r)
{
- struct ref_lock *lock;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
if (check_refname_format(r-name + 5, 0))
return;
- lock = lock_ref_sha1_basic(r-name, r-sha1, 0, NULL);
- if (lock) {
- unlink_or_warn(git_path(%s, r-name));
- unlock_ref(lock);
- try_remove_empty_parents(r-name);
+ transaction = ref_transaction_begin(err);
+ if (!transaction ||
+ ref_transaction_delete(transaction, r-name, r-sha1,
+ REF_ISPRUNING, 1, err) ||
+ ref_transaction_commit(transaction, NULL, err)) {
+ ref_transaction_free(transaction);
+ error(%s, err.buf);
+ strbuf_release(err);
+ return;
}
+ ref_transaction_free(transaction);
+ try_remove_empty_parents(r-name);
}
static void prune_refs(struct ref_to_prune *r)
@@ -3546,9 +3559,10 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (update-lock) {
- delnames[delnum++] = update-lock-ref_name;
ret |= delete_ref_loose(update-lock, update-type,
err);
+ if (!(update-flags REF_ISPRUNING))
+ delnames[delnum++] = update-lock-ref_name;
}
}
diff --git a/refs.h b/refs.h
index 1e25e1c..39c97f8 100644
--- a/refs.h
+++ b/refs.h
@@ -235,6 +235,11 @@ struct ref_transaction *ref_transaction_begin(struct
strbuf *err);
*/
/*
+ * ref_transaction_update ref_transaction_create and ref_transaction_delete
+ * all take a flag argument. Currently the only public flag is REF_NODEREF.
+ * Flag values = 0x100 are reserved for internal use.
+ */
+/*
* 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
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 33/41] refs.c: pass the ref log message to _create/delete/update instead of _commit
Change the reference transactions so that we pass the reflog message
through to the create/delete/update function instead of the commit message.
This allows for individual messages for each change in a multi ref
transaction.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
branch.c | 4 ++--
builtin/commit.c | 4 ++--
builtin/fetch.c| 3 +--
builtin/receive-pack.c | 5 +++--
builtin/replace.c | 4 ++--
builtin/tag.c | 4 ++--
builtin/update-ref.c | 13 +++--
fast-import.c | 8
refs.c | 36 ++--
refs.h | 8
sequencer.c| 4 ++--
walker.c | 5 ++---
12 files changed, 53 insertions(+), 45 deletions(-)
diff --git a/branch.c b/branch.c
index c1eae00..e0439af 100644
--- a/branch.c
+++ b/branch.c
@@ -301,8 +301,8 @@ void create_branch(const char *head,
transaction = ref_transaction_begin(err);
if (!transaction ||
ref_transaction_update(transaction, ref.buf, sha1,
- null_sha1, 0, !forcing, err) ||
- ref_transaction_commit(transaction, msg, err))
+ null_sha1, 0, !forcing, msg, err) ||
+ ref_transaction_commit(transaction, err))
die(%s, err.buf);
ref_transaction_free(transaction);
}
diff --git a/builtin/commit.c b/builtin/commit.c
index 7db194f..608296c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1721,8 +1721,8 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
ref_transaction_update(transaction, HEAD, sha1,
current_head ?
current_head-object.sha1 : NULL,
- 0, !!current_head, err) ||
- ref_transaction_commit(transaction, sb.buf, err)) {
+ 0, !!current_head, sb.buf, err) ||
+ ref_transaction_commit(transaction, err)) {
rollback_index_files();
die(%s, err.buf);
}
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 55f457c..faa1233 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -673,10 +673,9 @@ static int store_updated_refs(const char *raw_url, const
char *remote_name,
}
}
}
-
if (rc STORE_REF_ERROR_DF_CONFLICT)
error(_(some local refs could not be updated; try running\n
- 'git remote prune %s' to remove any old, conflicting
+ 'git remote prune %s' to remove any old, conflicting
branches), remote_name);
abort:
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c88dc03..c91d3d6 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -594,8 +594,9 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
transaction = ref_transaction_begin(err);
if (!transaction ||
ref_transaction_update(transaction, namespaced_name,
- new_sha1, old_sha1, 0, 1, err) ||
- ref_transaction_commit(transaction, push, err)) {
+ new_sha1, old_sha1, 0, 1, push,
+ err) ||
+ ref_transaction_commit(transaction, err)) {
const char *str = add_error_string(err.buf);
ref_transaction_free(transaction);
strbuf_release(err);
diff --git a/builtin/replace.c b/builtin/replace.c
index f24d814..09bfd40 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -160,8 +160,8 @@ static int replace_object_sha1(const char *object_ref,
transaction = ref_transaction_begin(err);
if (!transaction ||
ref_transaction_update(transaction, ref, repl, prev,
- 0, !is_null_sha1(prev), err) ||
- ref_transaction_commit(transaction, NULL, err))
+ 0, !is_null_sha1(prev), NULL, err) ||
+ ref_transaction_commit(transaction, err))
die(%s, err.buf);
ref_transaction_free(transaction);
diff --git a/builtin/tag.c b/builtin/tag.c
index c9bfc9a..74af63e 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -705,8 +705,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
transaction = ref_transaction_begin(err);
if (!transaction ||
ref_transaction_update(transaction, ref.buf, object, prev,
- 0, !is_null_sha1(prev), err) ||
- ref_transaction_commit(transaction, NULL, err))
+ 0, !is_null_sha1(prev), NULL, err) ||
+
[PATCH v11 15/41] refs.c: make ref_transaction_begin take an err argument
Add an err argument to _begin so that on non-fatal failures in future ref
backends we can report a nice error back to the caller.
While _begin can currently never fail for other reasons than OOM, in which
case we die() anyway, we may add other types of backends in the future.
For example, a hypothetical MySQL backend could fail in _being with
Can not connect to MySQL server. No route to host.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 2 +-
refs.c | 2 +-
refs.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 7c9c248..c6ad0be 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -365,7 +365,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
die(Refusing to perform update with empty message.);
if (read_stdin) {
- transaction = ref_transaction_begin();
+ transaction = ref_transaction_begin(err);
if (delete || no_deref || argc 0)
usage_with_options(git_update_ref_usage, options);
if (end_null)
diff --git a/refs.c b/refs.c
index beddce0..1c660e0 100644
--- a/refs.c
+++ b/refs.c
@@ -3345,7 +3345,7 @@ struct ref_transaction {
size_t nr;
};
-struct ref_transaction *ref_transaction_begin(void)
+struct ref_transaction *ref_transaction_begin(struct strbuf *err)
{
return xcalloc(1, sizeof(struct ref_transaction));
}
diff --git a/refs.h b/refs.h
index e0ab989..6c830f2 100644
--- a/refs.h
+++ b/refs.h
@@ -227,7 +227,7 @@ enum action_on_err {
* Begin a reference transaction. The reference transaction must
* be freed by calling ref_transaction_free().
*/
-struct ref_transaction *ref_transaction_begin(void);
+struct ref_transaction *ref_transaction_begin(struct strbuf *err);
/*
* The following functions add a reference check or update to a
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 13/41] refs.c: change ref_transaction_create to do error checking and return status
Do basic error checking in ref_transaction_create() and make it return
non-zero on error. Update all callers to check the result of
ref_transaction_create(). There are currently no conditions in _create that
will return error but there will be in the future. Add an err argument that
will be updated on failure.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 4 +++-
refs.c | 18 --
refs.h | 25 ++---
3 files changed, 33 insertions(+), 14 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 3067b11..41121fa 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -226,7 +226,9 @@ static const char *parse_cmd_create(struct strbuf *input,
const char *next)
if (*next != line_termination)
die(create %s: extra input: %s, refname, next);
- ref_transaction_create(transaction, refname, new_sha1, update_flags);
+ if (ref_transaction_create(transaction, refname, new_sha1,
+ update_flags, err))
+ die(%s, err.buf);
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index 79a4477..709758d 100644
--- a/refs.c
+++ b/refs.c
@@ -3397,18 +3397,24 @@ int ref_transaction_update(struct ref_transaction
*transaction,
return 0;
}
-void ref_transaction_create(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *new_sha1,
- int flags)
+int ref_transaction_create(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *new_sha1,
+ int flags,
+ struct strbuf *err)
{
- struct ref_update *update = add_update(transaction, refname);
+ struct ref_update *update;
+
+ if (!new_sha1 || is_null_sha1(new_sha1))
+ die(BUG: create ref with null new_sha1);
+
+ update = add_update(transaction, refname);
- assert(!is_null_sha1(new_sha1));
hashcpy(update-new_sha1, new_sha1);
hashclr(update-old_sha1);
update-flags = flags;
update-have_old = 1;
+ return 0;
}
void ref_transaction_delete(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index 32edf3f..22b8cc3 100644
--- a/refs.h
+++ b/refs.h
@@ -215,6 +215,15 @@ enum action_on_err {
};
/*
+ * Transaction functions that take an err argument will append an error
+ * string to this buffer if there was a failure.
+ * This string is not cleared on each call and may contain an aggregate of
+ * errors from several previous calls.
+ * If the caller needs a guarantee that the buffer will only contain the
+ * current or most recent error it must call strbuf_reset before calling
+ * the transaction function.
+ */
+/*
* Begin a reference transaction. The reference transaction must
* be freed by calling ref_transaction_free().
*/
@@ -236,7 +245,7 @@ struct ref_transaction *ref_transaction_begin(void);
* it must not have existed beforehand.
* Function returns 0 on success and non-zero on failure. A failure to update
* means that the transaction as a whole has failed and will need to be
- * rolled back. On failure the err buffer will be updated.
+ * rolled back.
*/
int ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
@@ -250,11 +259,15 @@ int ref_transaction_update(struct ref_transaction
*transaction,
* that the reference should have after the update; it must not be the
* null SHA-1. It is verified that the reference does not exist
* already.
+ * Function returns 0 on success and non-zero on failure. A failure to create
+ * means that the transaction as a whole has failed and will need to be
+ * rolled back.
*/
-void ref_transaction_create(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *new_sha1,
- int flags);
+int ref_transaction_create(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *new_sha1,
+ int flags,
+ struct strbuf *err);
/*
* Add a reference deletion to transaction. If have_old is true, then
@@ -270,8 +283,6 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
* Commit all of the changes that have been queued in transaction, as
* atomically as possible. Return a nonzero value if there is a
* problem.
- * If err is non-NULL we will add an error string to it to explain why
- * the transaction failed. The string does not end in newline.
*/
int ref_transaction_commit(struct ref_transaction *transaction,
const char *msg, struct strbuf *err);
--
[PATCH v11 41/41] refs.c: make write_ref_sha1 static
No external users call write_ref_sha1 any more so lets declare it static.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 6 +-
refs.h | 3 ---
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index c108007..2b51547 100644
--- a/refs.c
+++ b/refs.c
@@ -2622,6 +2622,9 @@ static int rename_tmp_log(const char *newrefname)
return 0;
}
+static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
+ const char *logmsg);
+
int rename_ref(const char *oldrefname, const char *newrefname, const char
*logmsg)
{
unsigned char sha1[20], orig_sha1[20];
@@ -2855,7 +2858,8 @@ static int is_branch(const char *refname)
return !strcmp(refname, HEAD) || starts_with(refname, refs/heads/);
}
-int write_ref_sha1(struct ref_lock *lock,
+/** Writes sha1 into the ref specified by the lock. **/
+static int write_ref_sha1(struct ref_lock *lock,
const unsigned char *sha1, const char *logmsg)
{
static char term = '\n';
diff --git a/refs.h b/refs.h
index 2823b72..47775e8 100644
--- a/refs.h
+++ b/refs.h
@@ -147,9 +147,6 @@ extern int commit_ref(struct ref_lock *lock);
/** Release any lock taken but not written. **/
extern void unlock_ref(struct ref_lock *lock);
-/** Writes sha1 into the ref specified by the lock. **/
-extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
const char *msg);
-
/** Setup reflog before using. **/
int log_ref_setup(const char *refname, char *logfile, int bufsize);
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 22/41] branch.c: use ref transaction for all ref updates
Change create_branch to use a ref transaction when creating the new branch.
This also fixes a race condition in the old code where two concurrent
create_branch could race since the lock_any_ref_for_update/write_ref_sha1
did not protect against the ref already existing. I.e. one thread could end up
overwriting a branch even if the forcing flag is false.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
branch.c | 30 --
1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/branch.c b/branch.c
index 660097b..c1eae00 100644
--- a/branch.c
+++ b/branch.c
@@ -226,7 +226,6 @@ void create_branch(const char *head,
int force, int reflog, int clobber_head,
int quiet, enum branch_track track)
{
- struct ref_lock *lock = NULL;
struct commit *commit;
unsigned char sha1[20];
char *real_ref, msg[PATH_MAX + 20];
@@ -285,15 +284,6 @@ void create_branch(const char *head,
die(_(Not a valid branch point: '%s'.), start_name);
hashcpy(sha1, commit-object.sha1);
- if (!dont_change_ref) {
- lock = lock_any_ref_for_update(ref.buf, NULL, 0, NULL);
- if (!lock)
- die_errno(_(Failed to lock ref for update));
- }
-
- if (reflog)
- log_all_ref_updates = 1;
-
if (forcing)
snprintf(msg, sizeof msg, branch: Reset to %s,
start_name);
@@ -301,13 +291,25 @@ void create_branch(const char *head,
snprintf(msg, sizeof msg, branch: Created from %s,
start_name);
+ if (reflog)
+ log_all_ref_updates = 1;
+
+ if (!dont_change_ref) {
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
+
+ transaction = ref_transaction_begin(err);
+ if (!transaction ||
+ ref_transaction_update(transaction, ref.buf, sha1,
+ null_sha1, 0, !forcing, err) ||
+ ref_transaction_commit(transaction, msg, err))
+ die(%s, err.buf);
+ ref_transaction_free(transaction);
+ }
+
if (real_ref track)
setup_tracking(ref.buf + 11, real_ref, track, quiet);
- if (!dont_change_ref)
- if (write_ref_sha1(lock, sha1, msg) 0)
- die_errno(_(Failed to write ref));
-
strbuf_release(ref);
free(real_ref);
}
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 21/41] fast-import.c: change update_branch to use ref transactions
Change update_branch() to use ref transactions for updates.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
fast-import.c | 25 +++--
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 6707a66..4a7b196 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1679,39 +1679,44 @@ found_entry:
static int update_branch(struct branch *b)
{
static const char *msg = fast-import;
- struct ref_lock *lock;
+ struct ref_transaction *transaction;
unsigned char old_sha1[20];
+ struct strbuf err = STRBUF_INIT;
if (read_ref(b-name, old_sha1))
hashclr(old_sha1);
+
if (is_null_sha1(b-sha1)) {
if (b-delete)
delete_ref(b-name, old_sha1, 0);
return 0;
}
- lock = lock_any_ref_for_update(b-name, old_sha1, 0, NULL);
- if (!lock)
- return error(Unable to lock %s, b-name);
if (!force_update !is_null_sha1(old_sha1)) {
struct commit *old_cmit, *new_cmit;
old_cmit = lookup_commit_reference_gently(old_sha1, 0);
new_cmit = lookup_commit_reference_gently(b-sha1, 0);
- if (!old_cmit || !new_cmit) {
- unlock_ref(lock);
+ if (!old_cmit || !new_cmit)
return error(Branch %s is missing commits., b-name);
- }
if (!in_merge_bases(old_cmit, new_cmit)) {
- unlock_ref(lock);
warning(Not updating %s
(new tip %s does not contain %s),
b-name, sha1_to_hex(b-sha1),
sha1_to_hex(old_sha1));
return -1;
}
}
- if (write_ref_sha1(lock, b-sha1, msg) 0)
- return error(Unable to update %s, b-name);
+ transaction = ref_transaction_begin(err);
+ if (!transaction ||
+ ref_transaction_update(transaction, b-name, b-sha1, old_sha1,
+ 0, 1, err) ||
+ ref_transaction_commit(transaction, msg, err)) {
+ ref_transaction_free(transaction);
+ error(%s, err.buf);
+ strbuf_release(err);
+ return -1;
+ }
+ ref_transaction_free(transaction);
return 0;
}
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 03/41] refs.c: constify the sha arguments for ref_transaction_create|delete|update
ref_transaction_create|delete|update has no need to modify the sha1
arguments passed to it so it should use const unsigned char* instead
of unsigned char*.
Some functions, such as fast_forward_to(), already have its old/new
sha1 arguments as consts. This function will at some point need to
use ref_transaction_update() in which case this change is required.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 7 ---
refs.h | 7 ---
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index 33541f4..a767ef6 100644
--- a/refs.c
+++ b/refs.c
@@ -,7 +,8 @@ static struct ref_update *add_update(struct
ref_transaction *transaction,
void ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1, unsigned char *old_sha1,
+ const unsigned char *new_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old)
{
struct ref_update *update = add_update(transaction, refname);
@@ -3347,7 +3348,7 @@ void ref_transaction_update(struct ref_transaction
*transaction,
void ref_transaction_create(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1,
+ const unsigned char *new_sha1,
int flags)
{
struct ref_update *update = add_update(transaction, refname);
@@ -3361,7 +3362,7 @@ void ref_transaction_create(struct ref_transaction
*transaction,
void ref_transaction_delete(struct ref_transaction *transaction,
const char *refname,
- unsigned char *old_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old)
{
struct ref_update *update = add_update(transaction, refname);
diff --git a/refs.h b/refs.h
index 306d833..b893838 100644
--- a/refs.h
+++ b/refs.h
@@ -237,7 +237,8 @@ struct ref_transaction *ref_transaction_begin(void);
*/
void ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1, unsigned char *old_sha1,
+ const unsigned char *new_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old);
/*
@@ -248,7 +249,7 @@ void ref_transaction_update(struct ref_transaction
*transaction,
*/
void ref_transaction_create(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1,
+ const unsigned char *new_sha1,
int flags);
/*
@@ -258,7 +259,7 @@ void ref_transaction_create(struct ref_transaction
*transaction,
*/
void ref_transaction_delete(struct ref_transaction *transaction,
const char *refname,
- unsigned char *old_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old);
/*
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 07/41] refs.c: make ref_update_reject_duplicates take a strbuf argument for errors
Make ref_update_reject_duplicates return any error that occurs through a
new strbuf argument. This means that when a transaction commit fails in
this function we will now be able to pass a helpful error message back to the
caller.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 2da62ac..4c7f4f7 100644
--- a/refs.c
+++ b/refs.c
@@ -3410,6 +3410,7 @@ static int ref_update_compare(const void *r1, const void
*r2)
}
static int ref_update_reject_duplicates(struct ref_update **updates, int n,
+ struct strbuf *err,
enum action_on_err onerr)
{
int i;
@@ -3417,6 +3418,9 @@ static int ref_update_reject_duplicates(struct ref_update
**updates, int n,
if (!strcmp(updates[i - 1]-refname, updates[i]-refname)) {
const char *str =
Multiple updates for ref '%s' not allowed.;
+ if (err)
+ strbuf_addf(err, str, updates[i]-refname);
+
switch (onerr) {
case UPDATE_REFS_MSG_ON_ERR:
error(str, updates[i]-refname); break;
@@ -3447,7 +3451,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Copy, sort, and reject duplicate refs */
qsort(updates, n, sizeof(*updates), ref_update_compare);
- ret = ref_update_reject_duplicates(updates, n, onerr);
+ ret = ref_update_reject_duplicates(updates, n, err, onerr);
if (ret)
goto cleanup;
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 10/41] update-ref.c: log transaction error from the update_ref
Call ref_transaction_commit with QUIET_ON_ERR and use the strbuf that is
returned to print a log message if/after the transaction fails.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 22617af..aec9004 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -342,6 +342,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
const char *refname, *oldval, *msg = NULL;
unsigned char sha1[20], oldsha1[20];
int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;
+ struct strbuf err = STRBUF_INIT;
struct option options[] = {
OPT_STRING( 'm', NULL, msg, N_(reason), N_(reason of the
update)),
OPT_BOOL('d', NULL, delete, N_(delete the reference)),
@@ -359,18 +360,17 @@ int cmd_update_ref(int argc, const char **argv, const
char *prefix)
die(Refusing to perform update with empty message.);
if (read_stdin) {
- int ret;
transaction = ref_transaction_begin();
-
if (delete || no_deref || argc 0)
usage_with_options(git_update_ref_usage, options);
if (end_null)
line_termination = '\0';
update_refs_stdin();
- ret = ref_transaction_commit(transaction, msg, NULL,
-UPDATE_REFS_DIE_ON_ERR);
+ if (ref_transaction_commit(transaction, msg, err,
+ UPDATE_REFS_QUIET_ON_ERR))
+ die(%s, err.buf);
ref_transaction_free(transaction);
- return ret;
+ return 0;
}
if (end_null)
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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 v11 06/41] refs.c: add an err argument to repack_without_refs
Update repack_without_refs to take an err argument and update it if there
is a failure. Pass the err variable from ref_transaction_commit to this
function so that callers can print a meaningful error message if _commit
fails due to a problem in repack_without_refs.
Add a new function unable_to_lock_strbuf that takes a strbuf argument and
fills in the reason for the failure.
In commit_packed_refs, make sure that we propagate any errno that
commit_lock_file might have set back to our caller.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
cache.h| 2 ++
lockfile.c | 21 -
refs.c | 25 +++--
3 files changed, 33 insertions(+), 15 deletions(-)
diff --git a/cache.h b/cache.h
index 8c6cdc2..48548d6 100644
--- a/cache.h
+++ b/cache.h
@@ -559,6 +559,8 @@ struct lock_file {
#define LOCK_DIE_ON_ERROR 1
#define LOCK_NODEREF 2
extern int unable_to_lock_error(const char *path, int err);
+extern void unable_to_lock_strbuf(const char *path, int err,
+ struct strbuf *buf);
extern NORETURN void unable_to_lock_index_die(const char *path, int err);
extern int hold_lock_file_for_update(struct lock_file *, const char *path,
int);
extern int hold_lock_file_for_append(struct lock_file *, const char *path,
int);
diff --git a/lockfile.c b/lockfile.c
index 8fbcb6a..9e04b43 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -157,33 +157,36 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
return lk-fd;
}
-static char *unable_to_lock_message(const char *path, int err)
+void unable_to_lock_strbuf(const char *path, int err, struct strbuf *buf)
{
- struct strbuf buf = STRBUF_INIT;
if (err == EEXIST) {
- strbuf_addf(buf, Unable to create '%s.lock': %s.\n\n
+ strbuf_addf(buf, Unable to create '%s.lock': %s.\n\n
If no other git process is currently running, this
probably means a\n
git process crashed in this repository earlier. Make sure
no other git\n
process is running and remove the file manually to
continue.,
absolute_path(path), strerror(err));
} else
- strbuf_addf(buf, Unable to create '%s.lock': %s,
+ strbuf_addf(buf, Unable to create '%s.lock': %s,
absolute_path(path), strerror(err));
- return strbuf_detach(buf, NULL);
}
int unable_to_lock_error(const char *path, int err)
{
- char *msg = unable_to_lock_message(path, err);
- error(%s, msg);
- free(msg);
+ struct strbuf buf = STRBUF_INIT;
+
+ unable_to_lock_strbuf(path, err, buf);
+ error(%s, buf.buf);
+ strbuf_release(buf);
return -1;
}
NORETURN void unable_to_lock_index_die(const char *path, int err)
{
- die(%s, unable_to_lock_message(path, err));
+ struct strbuf buf = STRBUF_INIT;
+
+ unable_to_lock_strbuf(path, err, buf);
+ die(%s, buf.buf);
}
int hold_lock_file_for_update(struct lock_file *lk, const char *path, int
flags)
diff --git a/refs.c b/refs.c
index 25c3a93..2da62ac 100644
--- a/refs.c
+++ b/refs.c
@@ -2208,6 +2208,7 @@ int commit_packed_refs(void)
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(ref_cache);
int error = 0;
+ int save_errno = 0;
if (!packed_ref_cache-lock)
die(internal error: packed-refs not locked);
@@ -2217,10 +2218,13 @@ int commit_packed_refs(void)
do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),
0, write_packed_entry_fn,
packed_ref_cache-lock-fd);
- if (commit_lock_file(packed_ref_cache-lock))
+ if (commit_lock_file(packed_ref_cache-lock)) {
+ save_errno = errno;
error = -1;
+ }
packed_ref_cache-lock = NULL;
release_packed_ref_cache(packed_ref_cache);
+ errno = save_errno;
return error;
}
@@ -2427,12 +2431,12 @@ static int curate_packed_ref_fn(struct ref_entry
*entry, void *cb_data)
return 0;
}
-static int repack_without_refs(const char **refnames, int n)
+static int repack_without_refs(const char **refnames, int n, struct strbuf
*err)
{
struct ref_dir *packed;
struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
struct string_list_item *ref_to_delete;
- int i, removed = 0;
+ int i, ret, removed = 0;
/* Look for a packed ref */
for (i = 0; i n; i++)
@@ -2444,6 +2448,11 @@ static int repack_without_refs(const char **refnames,
int n)
return 0; /* no refname exists in packed refs */
if (lock_packed_refs(0)) {
+ if (err) {
+ unable_to_lock_strbuf(git_path(packed-refs), errno,
+ err);
+ return
[PATCH v11 05/41] refs.c: add a strbuf argument to ref_transaction_commit for error logging
Add a strbuf argument to _commit so that we can pass an error string back to
the caller. So that we can do error logging from the caller instead of from
_commit.
Longer term plan is to first convert all callers to use onerr==QUIET_ON_ERR
and craft any log messages from the callers themselves and finally remove the
onerr argument completely.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 2 +-
refs.c | 6 +-
refs.h | 5 -
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 1fd7a89..22617af 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -367,7 +367,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
if (end_null)
line_termination = '\0';
update_refs_stdin();
- ret = ref_transaction_commit(transaction, msg,
+ ret = ref_transaction_commit(transaction, msg, NULL,
UPDATE_REFS_DIE_ON_ERR);
ref_transaction_free(transaction);
return ret;
diff --git a/refs.c b/refs.c
index 0faed29..25c3a93 100644
--- a/refs.c
+++ b/refs.c
@@ -3418,7 +3418,8 @@ static int ref_update_reject_duplicates(struct ref_update
**updates, int n,
}
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, enum action_on_err onerr)
+ const char *msg, struct strbuf *err,
+ enum action_on_err onerr)
{
int ret = 0, delnum = 0, i;
const char **delnames;
@@ -3447,6 +3448,9 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
update-flags,
update-type, onerr);
if (!update-lock) {
+ if (err)
+ strbuf_addf(err, Cannot lock the ref '%s'.,
+ update-refname);
ret = 1;
goto cleanup;
}
diff --git a/refs.h b/refs.h
index b893838..94d4cd4 100644
--- a/refs.h
+++ b/refs.h
@@ -266,9 +266,12 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
* Commit all of the changes that have been queued in transaction, as
* atomically as possible. Return a nonzero value if there is a
* problem.
+ * If err is non-NULL we will add an error string to it to explain why
+ * the transaction failed. The string does not end in newline.
*/
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, enum action_on_err onerr);
+ const char *msg, struct strbuf *err,
+ enum action_on_err onerr);
/*
* Free an existing transaction and all associated data.
--
2.0.0.rc3.474.g0203784
--
To unsubscribe from this list: send the line 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] contrib: add convert-grafts-to-replace-refs.sh
On Tuesday, May 27, 2014, Christian Couder chrisc...@tuxfamily.org wrote:
This script adds into contrib/ an example script to convert
grafts from an existing grafts file into replace refs using
the new --graft option of git replace.
While at it let's mention this new script in the
git replace documentation for the --graft option.
Signed-off-by: Christian Couder chrisc...@tuxfamily.org
---
Documentation/git-replace.txt | 4 +++-
contrib/convert-grafts-to-replace-refs.sh | 24
2 files changed, 27 insertions(+), 1 deletion(-)
create mode 100755 contrib/convert-grafts-to-replace-refs.sh
diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt
index 491875e..e1be2e6 100644
--- a/Documentation/git-replace.txt
+++ b/Documentation/git-replace.txt
@@ -79,7 +79,9 @@ OPTIONS
content as commit except that its parents will be
[parent...] instead of commit's parents. A replacement ref
is then created to replace commit with the newly created
- commit.
+ commit. See contrib/convert-grafts-to-replace-refs.sh for an
+ example script based on this option that can convert grafts to
+ replace refs.
-l pattern::
--list pattern::
diff --git a/contrib/convert-grafts-to-replace-refs.sh
b/contrib/convert-grafts-to-replace-refs.sh
new file mode 100755
index 000..2404cc6
--- /dev/null
+++ b/contrib/convert-grafts-to-replace-refs.sh
@@ -0,0 +1,24 @@
+#!/bin/sh
+
+# You should execute this script in the repository where you
+# want to convert grafts to replace refs.
+
+die () {
+ echo 2 $@
+ exit 1
+}
+
+GRAFTS_FILE=${GIT_DIR:-.git}/info/grafts
+
+test -f $GRAFTS_FILE || die Could not find graft file: '$GRAFTS_FILE'
+
+grep '^[^# ]' $GRAFTS_FILE | while read definition
+do
+ test -n $definition {
+ echo Converting: $definition
+ git replace --graft $definition || die Convertion failed
for: $definition
s/Convertion/Conversion/
+ }
+done
+
+echo Success! All the grafts in '$GRAFTS_FILE' have been converted to
replace refs!
+echo You should now move away or delete the grafts file: '$GRAFTS_FILE'
Rather than merely giving advice, would it make sees for the script
rename the file (adding .bak or some such) and report that it did so?
--
To unsubscribe from this list: send the line 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 01/15] builtin/add.c: rearrange xcalloc arguments
On Tue, May 27, 2014 at 7:32 AM, Brian Gesiak modoca...@gmail.com wrote: Oomph, how embarrassing. Thanks for pointing that out! Etiquette on this list is to avoid top-posting [1]. [1]: https://lkml.org/lkml/2005/1/11/111 Would it be better if I rerolled the patches? Junio may or may not make small fixes himself when he picks up a patch series. If you don't hear from him and your patches don't appear in his 'pu' branch with that fix, re-rolling might be advisable. - Brian Gesiak On Tue, May 27, 2014 at 12:25 PM, Eric Sunshine sunsh...@sunshineco.com wrote: On Mon, May 26, 2014 at 11:33 AM, Brian Gesiak modoca...@gmail.com wrote: xcalloc takes two arguments: the number of elements and their size. run_add_interactive passes the arguments in reverse order, passing the size of a char*, followed by the number of char* to be allocated. Rearrgange them so they are in the correct order. If you do re-roll, perhaps consider simplifying the commit messages. The patch itself states concisely and precisely what is being changed; the lengthy prose description doesn't really add anything (and makes more work for you and the reader of the message). It might be sufficient to use a single-line (Subject:) commit message, like this: builtin/add.c: fix order of xcalloc arguments s/Rearrgange/Rearrange/ Same misspelling afflicts the entire patch series. Signed-off-by: Brian Gesiak modoca...@gmail.com --- builtin/add.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/add.c b/builtin/add.c index 672adc0..488acf4 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -248,7 +248,7 @@ int run_add_interactive(const char *revision, const char *patch_mode, int status, ac, i; const char **args; - args = xcalloc(sizeof(const char *), (pathspec-nr + 6)); + args = xcalloc((pathspec-nr + 6), sizeof(const char *)); ac = 0; args[ac++] = add--interactive; if (patch_mode) -- 2.0.0.rc1.543.gc8042da -- To unsubscribe from this list: send the line 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] Add an explicit GIT_DIR to the list of excludes
I've sent out version [PATCH v4] with most of the things addressed. That one hasn't been reviewed by anyone yet On Tue, May 27, 2014 at 11:04 AM, Junio C Hamano gits...@pobox.com wrote: Duy Nguyen pclo...@gmail.com writes: On Sat, May 24, 2014 at 12:33 AM, Pasha Bolokhov pasha.bolok...@gmail.com wrote: When an explicit '--git-dir' option points to a directory inside the work tree, git treats it as if it were any other directory. In particular, 'git status' lists it as untracked, while 'git add -A' stages the metadata directory entirely Add GIT_DIR to the list of excludes in setup_standard_excludes(), while checking that GIT_DIR is not just '.git', in which case it would be ignored by default, and that GIT_DIR is inside GIT_WORK_TREE Although an analogous comparison of any given path against '.git' is done in treat_path(), this does not seem to be the right place to compare against GIT_DIR. Instead, the excludes provide an effective mechanism of ignoring a file/directory, and adding GIT_DIR as an exclude is equivalent of putting it into '.gitignore'. Function setup_standard_excludes() was chosen because that is the place where the excludes are initialized by the commands that are concerned about excludes I like this approach. A search of exclude-standard in Documentation/ gives git-grep.txt and git-ls-files.txt. I don't know if we need to add something about this extra exclude rule to those .txt. If it's so obvious that this should be the expected behavior, then probably not. OK, so is that an Acked/Reviewed-by? The case of git grep --exclude-standard is interesting because it's intended to work without a repository. First reaction was would get_git_dir() return NULL in that case. But it should return .git so we're good. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
