Re: [PATCH v3 2/5] commit test: Change $PWD to $(pwd)
Johannes Sixt 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 --- 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 --- 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_cle
[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 --- 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 --- 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 2>&1; 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 --- 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 --- 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 2>&1 || 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_DESCR
[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 --- 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\"" &
[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 --- 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 && (
[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 --- 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 && - ec
[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 --- 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 > [...] >> * 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 " | git interpret-trailers --trailer fix=42 | git interpret-trailers --trailer sign="Bob " - output -- subject Signed-off-by: Alice Fix #42 Signed-off-by: Bob --- 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 examp
[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 --- 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 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, 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 wrote: > On Mon, May 26, 2014 at 11:33 AM, Brian Gesiak 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 >> --- >> 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 > Signed-off-by: Fabian Ruch > --- > 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 "pick"ed 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 > Signed-off-by: Fabian Ruch > --- > 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 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, 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 --- 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
Re: [PATCH/RFC] Gitweb: Convert UTF-8 encoded file names
W dniu 2014-05-16 19:05, Junio C Hamano pisze: > Jakub Narębski 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 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 Signed-off-by: Jakub Narębski --- 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=rss&p=foo.git&f=$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 "<__£åëîñòûý¶ +;?&__>" 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 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 Signed-off-by: Jakub Narębski --- 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 -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 > --- > 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
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 > 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 > > Thanks-to: Erik Faye-Lund > Thanks-to: Jeff King 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 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 writes: > On Sat, May 24, 2014 at 12:33 AM, Pasha Bolokhov > 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 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 > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > 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 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 <> +#!$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 < > 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 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 >> --- >> 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 c
Re: [PATCH v8 41/44] refs.c: add a new flag for transaction delete for refs we know are packed only
Michael Haggerty 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 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 conflic
Re: [RFC 1/3] sequencer: Signal failed ff as an aborted, not a conflicted merge
Fabian Ruch 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 > --- > 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 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 > Signed-off-by: Fabian Ruch > --- > 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
Re: [PATCH v1 0/3] Add --graft option to git replace
From: Junio C Hamano Subject: Re: [PATCH v1 0/3] Add --graft option to git replace Date: Fri, 23 May 2014 09:59:05 -0700 > Christian Couder writes: > >> Here is a small patch series to implement: >> >> git replace [-f] --graft [...] >> >> The changes since the RFC/PATCH are the following: >> >> - in patch 1/3, parse_commit_buffer() is now used to >> make sure 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 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 > > 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 > > 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 > > 17/40 replace: use ref transactions when doing updates > Reviewed-by: Jonathan Nieder > > 18/40 commit: use ref transactions for updates > Reviewed-by: Jonathan Nieder > > 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 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 >> --- >> 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 writes: > From: Michael Haggerty > ... > 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 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 >> --- >> 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
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 wrote: > On 05/23/2014 06:14 PM, Ronnie Sahlberg wrote: >> On Fri, May 23, 2014 at 6:49 AM, Michael Haggerty >> 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 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 > --- > 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 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 > --- > > 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=git&m=140100460903936&w=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=git&m=140109062903038&w=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 <> > can inherit part of its <> > - 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 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 Signed-off-by: Ronnie Sahlberg --- 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 --- 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 --- 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 Signed-off-by: Ronnie Sahlberg --- 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 --- 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 Signed-off-by: Ronnie Sahlberg --- 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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 Signed-off-by: Ronnie Sahlberg --- 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 Signed-off-by: Ronnie Sahlberg --- 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 Signed-off-by: Ronnie Sahlberg --- builtin/update-ref.c | 3 +-- refs.c | 22 +++--- refs.h | 3 +-- 3 files changed, 9 insertions(+), 19 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 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 http://vger.kernel.org/majordomo-info.htm
[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 --- 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 Signed-off-by: Ronnie Sahlberg --- 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 Signed-off-by: Ronnie Sahlberg --- refs.c | 2 +- refs.h | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/refs.c b/refs.c index 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 --- 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 --- 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 Signed-off-by: Ronnie Sahlberg --- 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 Signed-off-by: Ronnie Sahlberg --- 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 --- 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 Signed-off-by: Ronnie Sahlberg --- builtin/update-ref.c | 12 +++- refs.c | 18 -- refs.h | 14 +- 3 files changed, 28 insertions(+), 16 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 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 *new_sha1, -
[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 --- 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), git_pa
[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 --- 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 Signed-off-by: Ronnie Sahlberg --- 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 Signed-off-by: Ronnie Sahlberg --- 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 --- 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[
[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 --- 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 --- 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 Signed-off-by: Ronnie Sahlberg --- 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 --- 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); -- 2.0.0.r
[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 --- 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 Signed-off-by: Ronnie Sahlberg --- 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 --- 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 Signed-off-by: Ronnie Sahlberg --- refs.c | 7 --- refs.h | 7 --- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/refs.c b/refs.c index 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 Signed-off-by: Ronnie Sahlberg --- 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 Signed-off-by: Ronnie Sahlberg --- builtin/update-ref.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 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 --- 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); +
[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 Signed-off-by: Ronnie Sahlberg --- builtin/update-ref.c | 2 +- refs.c | 6 +- refs.h | 5 - 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 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 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 > --- > 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 except that its parents will be > [...] instead of 's parents. A replacement ref > is then created to replace 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 :: > --list :: > 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 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 > wrote: >> On Mon, May 26, 2014 at 11:33 AM, Brian Gesiak 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 >>> --- >>> 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 wrote: > Duy Nguyen writes: > >> On Sat, May 24, 2014 at 12:33 AM, Pasha Bolokhov >> 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] contrib: add convert-grafts-to-replace-refs.sh
Eric Sunshine writes: >> +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? Yeah, I would say 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 v3 3/5] commit test: Use write_script
On Mon, May 26, 2014 at 2:56 PM, Caleb Thompson wrote: > Use write_script from t/test-lib-functions instead of cat, shebang, and > chmod. > > Signed-off-by: Caleb Thompson > --- > t/t7507-commit-verbose.sh | 6 ++ > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh > index 3b06d73..e62d921 100755 > --- a/t/t7507-commit-verbose.sh > +++ b/t/t7507-commit-verbose.sh > @@ -3,11 +3,9 @@ > test_description='verbose commit template' > . ./test-lib.sh > > -cat >check-for-diff < -#!$SHELL_PATH > -exec grep '^diff --git' "\$1" > +write_script check-for-diff <<-EOF > + exec grep '^diff --git' "\$1" Food for thought: The original code used < EOF > -chmod +x check-for-diff > test_set_editor "$(pwd)/check-for-diff" > > cat >message <<'EOF' > -- > 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 01/15] builtin/add.c: rearrange xcalloc arguments
Eric Sunshine writes: > 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 Yeah, I like that. I do not think it is worth doing this change starting from maint, so I've dropped this one and a few others that did not apply to master and queued the remainder to 'pu'. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html