Re: [PATCH v5 3/3] t0302: test credential-store support for XDG_CONFIG_HOME
On Tue, Mar 24, 2015 at 5:52 AM, Matthieu Moy wrote: > Paul Tan writes: > >> Matthieu and Eric: I know I said I will try to re-order the patches to >> put the tests before the implementation, but after thinking and trying >> to rewrite the commit messages I realised it seems really weird to me. >> In this patch series, the implementation is split across the first two >> patches. The first patch should use the old tests, and ideally, the new >> tests should be squashed with the second patch because it seems more >> logical to me to implement the tests at the same time as the new >> feature. However, since the tests patch is very long, to make it easier >> to review it is split into a separate patch which is applied after the >> implementation patches. > > No problem, your version is very good. I was pointing out alternatives, > but not requesting a change, and your reasoning makes perfect sense. > > I had reviewed v4 in details, and checked the diff between v4 and v5. > The whole series is now > > Reviewed-by: Matthieu Moy With the POSIXPERM issue[1] addressed (if necessary), patch 3/3 is also: Reviewed-by: Eric Sunshine [1]: http://article.gmane.org/gmane.comp.version-control.git/266265 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: Sparse checkout not working as expected (colons in filenames on Windows)
On Wed, Mar 25, 2015 at 1:39 PM, Johannes Schindelin wrote: > Hi Duy, > > On 2015-03-25 01:46, Duy Nguyen wrote: >> On Wed, Mar 25, 2015 at 6:50 AM, Philip Oakley wrote: >> >>> That said, the final error (which I'd missed in the earlier post) is: >>> fatal: make_cache_entry failed for path 'ifcfg-eth0:0' >>> >>> This is on the Windows (pre-compiled msysgit at v1.9.5) Git bash, so could >>> be a catch path in that code for make_cache_entry (I've not checked the code >>> yet). So at the moment it doesn't look like sparse checkout can be used to >>> avoid colons in windows on-disk files based on the current code. >> >> Both of your commands below fail by the same function, verify_path() >> because of this msysgit commit 2e2a2d1 (NTFS: Prevent problematic >> paths from being checked out - 2014-12-10). I guess that check is a >> bit too strong, it should apply when new index entries are created >> from worktree (not from a tree).. > > Oh, right, that check needs some relaxing. But certainly in a different way: > you *definitely* want to prevent attacks from the outside, and those > originate from the *tree*. > > What we need to do instead is to relax the check to apply only if we are > really going to write out that file, not when it is skipped. Yeah. In fact if you do this, not checking out files that can't be or not safe to be checked out, then the ifcfg-eth0:0 problem that Phillip wanted to avoid using sparse checkout is also gone. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/3] t0302: test credential-store support for XDG_CONFIG_HOME
On Tue, Mar 24, 2015 at 1:20 AM, Paul Tan wrote: > t0302 now tests git-credential-store's support for the XDG user-specific > configuration file $XDG_CONFIG_HOME/git/credentials. Specifically: > > * Ensure that the XDG file is strictly opt-in. It should not be created > by git at all times if it does not exist. > > * Conversely, if the XDG file exists, ~/.git-credentials should > not be created at all times. > > * If both the XDG file and ~/.git-credentials exists, then both files > should be used for credential lookups. However, credentials should > only be written to ~/.git-credentials. > > * Credentials must be erased from both files. > > * $XDG_CONFIG_HOME can be a custom directory set by the user as per the > XDG base directory specification. Test that git-credential-store > respects that, but defaults to "~/.config/git/credentials" if it does > not exist or is empty. > > Helped-by: Matthieu Moy > Helped-by: Junio C Hamano > Helped-by: Eric Sunshine > Signed-off-by: Paul Tan > --- > diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh > index f61b40c..4e1f8ec 100755 > --- a/t/t0302-credential-store.sh > +++ b/t/t0302-credential-store.sh > @@ -6,4 +6,118 @@ test_description='credential-store tests' > > helper_test store > > +test_expect_success 'get: use xdg file if home file is unreadable' ' I meant to mention this earlier. Does this test need to be protected by the POSIXPERM prerequisite since it's using chmod? test_expect_success POSIXPERM 'get: ... unreadable' ' Otherwise, the test will likely fail on Windows. > + echo "https://home-user:home-p...@example.com"; > >"$HOME/.git-credentials" && > + chmod -r "$HOME/.git-credentials" && > + mkdir -p "$HOME/.config/git" && > + echo "https://xdg-user:xdg-p...@example.com"; > >"$HOME/.config/git/credentials" && > + check fill store <<-\EOF > + protocol=https > + host=example.com > + -- > + protocol=https > + host=example.com > + username=xdg-user > + password=xdg-pass > + -- > + EOF > +' -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: Sparse checkout not working as expected (colons in filenames on Windows)
Hi Duy, On 2015-03-25 01:46, Duy Nguyen wrote: > On Wed, Mar 25, 2015 at 6:50 AM, Philip Oakley wrote: > >> That said, the final error (which I'd missed in the earlier post) is: >> fatal: make_cache_entry failed for path 'ifcfg-eth0:0' >> >> This is on the Windows (pre-compiled msysgit at v1.9.5) Git bash, so could >> be a catch path in that code for make_cache_entry (I've not checked the code >> yet). So at the moment it doesn't look like sparse checkout can be used to >> avoid colons in windows on-disk files based on the current code. > > Both of your commands below fail by the same function, verify_path() > because of this msysgit commit 2e2a2d1 (NTFS: Prevent problematic > paths from being checked out - 2014-12-10). I guess that check is a > bit too strong, it should apply when new index entries are created > from worktree (not from a tree).. Oh, right, that check needs some relaxing. But certainly in a different way: you *definitely* want to prevent attacks from the outside, and those originate from the *tree*. What we need to do instead is to relax the check to apply only if we are really going to write out that file, not when it is skipped. Thanks, Johannes -- To unsubscribe from this list: send the line "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: macblame - al alterntive to 'git blame'
sample output.. for file Gemfile.. Contributor: Prasanna with 93.75 % contribution with 30 lines of code Contributor: h4r1sh with 6.25 % contribution with 2 lines of code * * * * * * * * * * * * * * * * * * * * * * * * * and I built this tool by pipelining the output produced by 'git blame' and we use this tool for our internal purposes in our organisation where some files are edited by some 15 people and this will tell us whom to catch if that files run into any problems (assuming largest contributor knows what this file is upto). Thanks, Prasanna -- To unsubscribe from this list: send the line "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/GSoC] Proposal: Make git-pull and git-am builtins
Hi, On Wed, Mar 25, 2015 at 2:37 AM, Junio C Hamano wrote: > Paul Tan writes: > >> ..., I propose the following requirements for the rewritten code: >> >> 1. No spawning of external git processes. This is to support systems with >> high >>``fork()`` or process creation overhead, and to reduce redundant IO by >>taking advantage of the internal object, index and configuration cache. > > I suspect this may probably be too strict in practice. > > True, we should never say "run_command_capture()" just to to read > from "git rev-parse"---we should just call get_sha1() instead. > > But for a complex command whose execution itself far outweighs the > cost of forking, I do not think it is fair to say your project > failed if you chose to run_command() it. For example, it may be > perfectly OK to invoke "git merge" via run_command(). Yes, which is why I proposed writing a baseline using only the run-command APIs first. Any other optimizations can then be done selectively after that. I think it's still good to have the ideal in mind though (and whoops I forgot to put in the word "ideal" in the text). > >> 3. The resulting builtin should not have wildly different behavior or bugs >>compared to the shell script. > > This on the other hand is way too loose. > > The original and the port must behave identically, unless the > difference is fixing bugs in the original. > I was considering that there may be slight behavioral changes when the rewritten code is modified to take greater advantage of the internal API, especially since some builtins due to historical issues, may have duplicated code from the internal API[1]. [1] I'm suspecting that the implementation of --merge-base in show-branch.c re-implements get_octopus_merge_bases(). >> Potential difficulties >> === >> >> Rewriting code may introduce bugs >> ... > > Yes, but that is a reasonable risk you need to manage to gain the > benefit from this project. > >> Of course, the downside of following this too strictly is that if there were >> any logical bugs in the original code, or if the original code is unclear, >> the >> rewritten code would inherit these problems too. > > I'd repeat my comment on the 3. above. Identifying and fixing bugs > is great, but otherwise don't worry about this too much. > > Being bug-to-bug compatible with the original is way better than > introducing new bugs of an unknown nature. Well yes, but I was thinking that if there are any glaring errors in the original source then it would be better to fix these errors during the rewrite than wasting time writing code that replicates these errors. >> Rewritten code may become harder to understand >> ... > > And also it may become harder to modify. > > That is the largest problem with any rewrite, and we should spend > the most effort to avoid it. > > A new bugs introduced we can later fix as long as the result is > understandable and maintainable. > >> For the purpose of reducing git's dependencies, the rewritten C code should >> not >> depend on other libraries or executables other than what is already available >> to git builtins. > > Perhaps misphrased; see below. In this case I was thinking of "making git depend on another project". (e.g, using an external regular expression library). Of course a balance has to be made in this aspect (thus the use of "should not"), but git-pull and git-am are relatively simple so there should be no need for that, > >> We can see that the C version requires much more lines compared to the shell >> pipeline,... > > That is something you would solve by introducing reusable code in > run_command API, isn't it? That is how various rewrites in the past > did, and this project should do so too. You should aim to do this > project by not just using "what is already available", but adding > what you discover is a useful reusable pattern into a set of new > functions in the "already available" API set. Whoops, forgot to mention that here. (A brief mention was made on this kind of refactoring in the Development Approach). Thank you for your review. -- To unsubscribe from this list: send the line "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 8/8] t9001: drop save_confirm helper
The idea of this helper is that we want to save the current
value of a config variable and then restore it again after
the test completes. However, there's no point in actually
saving the value; it should always be restored to the string
"never" (which you can confirm by instrumenting
save_confirm to print the value it finds).
Let's just replace it with a single test_when_finished call.
Suggested-by: SZEDER Gábor
Signed-off-by: Jeff King
---
t/t9001-send-email.sh | 15 +--
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index c9f54d5..7be14a4 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -817,25 +817,20 @@ test_expect_success $PREREQ '--confirm=compose' '
test_confirm --confirm=compose --compose
'
-save_confirm () {
- CONFIRM=$(git config --get sendemail.confirm) &&
- test_when_finished "git config sendemail.confirm ${CONFIRM:-never}"
-}
-
test_expect_success $PREREQ 'confirm by default (due to cc)' '
- save_confirm &&
+ test_when_finished git config sendemail.confirm never &&
git config --unset sendemail.confirm &&
test_confirm
'
test_expect_success $PREREQ 'confirm by default (due to --compose)' '
- save_confirm &&
+ test_when_finished git config sendemail.confirm never &&
git config --unset sendemail.confirm &&
test_confirm --suppress-cc=all --compose
'
test_expect_success $PREREQ 'confirm detects EOF (inform assumes y)' '
- save_confirm &&
+ test_when_finished git config sendemail.confirm never &&
git config --unset sendemail.confirm &&
rm -fr outdir &&
git format-patch -2 -o outdir &&
@@ -848,7 +843,7 @@ test_expect_success $PREREQ 'confirm detects EOF (inform
assumes y)' '
'
test_expect_success $PREREQ 'confirm detects EOF (auto causes failure)' '
- save_confirm &&
+ test_when_finished git config sendemail.confirm never &&
git config sendemail.confirm auto &&
GIT_SEND_EMAIL_NOTTY=1 &&
export GIT_SEND_EMAIL_NOTTY &&
@@ -860,7 +855,7 @@ test_expect_success $PREREQ 'confirm detects EOF (auto
causes failure)' '
'
test_expect_success $PREREQ 'confirm does not loop forever' '
- save_confirm &&
+ test_when_finished git config sendemail.confirm never &&
git config sendemail.confirm auto &&
GIT_SEND_EMAIL_NOTTY=1 &&
export GIT_SEND_EMAIL_NOTTY &&
--
2.3.4.635.gd6ffcfe
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 7/8] t0020: use test_* helpers instead of hand-rolled messages
These tests are not wrong, but it is much shorter and more idiomatic to say "verbose" or "test_must_fail" rather than printing our own messages on failure. Likewise, there is no need to say "happy" at the end of a test; the test suite takes care of that. Signed-off-by: Jeff King --- I somehow missed these when doing 9157c5c in the earlier series. t/t0020-crlf.sh | 38 +- 1 file changed, 5 insertions(+), 33 deletions(-) diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh index 144fdcd..f94120a 100755 --- a/t/t0020-crlf.sh +++ b/t/t0020-crlf.sh @@ -35,9 +35,7 @@ test_expect_success setup ' for w in Some extra lines here; do echo $w; done >>one && git diff >patch.file && patched=$(git hash-object --stdin .gitattributes && git read-tree --reset -u HEAD && - if has_cr dir/two - then - echo "Huh?" - false - else - : happy - fi + test_must_fail has_cr dir/two ' test_expect_success '.gitattributes says two and three are text' ' -- 2.3.4.635.gd6ffcfe -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/8] t: simplify loop exit-code status variables
Since shell loops may drop the exit code of failed commands
inside the loop, some tests try to keep track of the status
by setting a variable. This can end up cumbersome and hard
to read; it is much simpler to just exit directly from the
loop using "return 1" (since each case is either in a helper
function or inside a test snippet).
Signed-off-by: Jeff King
---
t/t3060-ls-files-with-tree.sh | 13 -
t/t3901-i18n-patch.sh | 8 ++--
2 files changed, 6 insertions(+), 15 deletions(-)
diff --git a/t/t3060-ls-files-with-tree.sh b/t/t3060-ls-files-with-tree.sh
index 61c1f53..36b10f7 100755
--- a/t/t3060-ls-files-with-tree.sh
+++ b/t/t3060-ls-files-with-tree.sh
@@ -25,15 +25,10 @@ test_expect_success setup '
do
num=00$n$m &&
>sub/file-$num &&
- echo file-$num >>expected || {
- bad=t
- break
- }
- done && test -z "$bad" || {
- bad=t
- break
- }
- done && test -z "$bad" &&
+ echo file-$num >>expected ||
+ return 1
+ done
+ done &&
git add . &&
git commit -m "add a bunch of files" &&
diff --git a/t/t3901-i18n-patch.sh b/t/t3901-i18n-patch.sh
index a392f3d..75cf3ff 100755
--- a/t/t3901-i18n-patch.sh
+++ b/t/t3901-i18n-patch.sh
@@ -9,7 +9,7 @@ test_description='i18n settings and format-patch | am pipe'
check_encoding () {
# Make sure characters are not corrupted
- cnt="$1" header="$2" i=1 j=0 bad=0
+ cnt="$1" header="$2" i=1 j=0
while test "$i" -le $cnt
do
git format-patch --encoding=UTF-8 --stdout HEAD~$i..HEAD~$j |
@@ -20,14 +20,10 @@ check_encoding () {
grep "^encoding ISO8859-1" ;;
*)
grep "^encoding ISO8859-1"; test "$?" != 0 ;;
- esac || {
- bad=1
- break
- }
+ esac || return 1
j=$i
i=$(($i+1))
done
- (exit $bad)
}
test_expect_success setup '
--
2.3.4.635.gd6ffcfe
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/8] t: fix some trivial cases of ignored exit codes in loops
These are all cases where we do a setup step of the form: for i in $foo; do set_up $i || break done && more_setup would not notice a failure in set_up (because break always returns a 0 exit code). These are just setup steps that we do not expect to fail, but it does not hurt to be defensive. Most can be fixed by converting the "break" to a "return 1" (since we eval our tests inside a function for just this purpose). A few of the loops are inside subshells, so we can use just "exit 1" to break out of the subshell. And a few can actually be made shorter by just unrolling the loop. Signed-off-by: Jeff King --- t/t3010-ls-files-killed-modified.sh | 11 --- t/t3031-merge-criscross.sh | 2 +- t/t3202-show-branch-octopus.sh | 2 +- t/t4024-diff-optimize-common.sh | 2 +- t/t4046-diff-unmerged.sh| 8 t/t4151-am-abort.sh | 2 +- t/t5505-remote.sh | 8 t/t5514-fetch-multiple.sh | 4 ++-- t/t6026-merge-attr.sh | 6 +++--- t/t6040-tracking-info.sh| 7 +++ 10 files changed, 24 insertions(+), 28 deletions(-) diff --git a/t/t3010-ls-files-killed-modified.sh b/t/t3010-ls-files-killed-modified.sh index 62fce10..580e158 100755 --- a/t/t3010-ls-files-killed-modified.sh +++ b/t/t3010-ls-files-killed-modified.sh @@ -55,13 +55,10 @@ test_expect_success 'git update-index --add to add various paths.' ' : >path9 && date >path10 && git update-index --add -- path0 path?/file? pathx/ju path7 path8 path9 path10 && - for i in 1 2 - do - git init submod$i && - ( - cd submod$i && git commit --allow-empty -m "empty $i" - ) || break - done && + git init submod1 && + git -C submod1 commit --allow-empty -m "empty 1" && + git init submod2 && + git -C submod2 commit --allow-empty -m "empty 2" && git update-index --add submod[12] && ( cd submod1 && diff --git a/t/t3031-merge-criscross.sh b/t/t3031-merge-criscross.sh index 7f41607..e59b0a3 100755 --- a/t/t3031-merge-criscross.sh +++ b/t/t3031-merge-criscross.sh @@ -32,7 +32,7 @@ test_expect_success 'setup repo with criss-cross history' ' do echo $n > data/$n && n=$(($n+1)) || - break + return 1 done && # check them in diff --git a/t/t3202-show-branch-octopus.sh b/t/t3202-show-branch-octopus.sh index 0a5d5e6..6adf478 100755 --- a/t/t3202-show-branch-octopus.sh +++ b/t/t3202-show-branch-octopus.sh @@ -19,7 +19,7 @@ test_expect_success 'setup' ' > file$i && git add file$i && test_tick && - git commit -m branch$i || break + git commit -m branch$i || return 1 done ' diff --git a/t/t4024-diff-optimize-common.sh b/t/t4024-diff-optimize-common.sh index c4d733f..7e76018 100755 --- a/t/t4024-diff-optimize-common.sh +++ b/t/t4024-diff-optimize-common.sh @@ -139,7 +139,7 @@ test_expect_success setup ' ( printf C; zs $n ) >file-c$n && ( echo D; zs $n ) >file-d$n && - expect_pattern $n || break + expect_pattern $n || return 1 done >expect ' diff --git a/t/t4046-diff-unmerged.sh b/t/t4046-diff-unmerged.sh index 25d50a6..d0f1447 100755 --- a/t/t4046-diff-unmerged.sh +++ b/t/t4046-diff-unmerged.sh @@ -8,7 +8,7 @@ test_expect_success setup ' do blob=$(echo $i | git hash-object --stdin) && eval "blob$i=$blob" && - eval "m$i=\"100644 \$blob$i $i\"" || break + eval "m$i=\"100644 \$blob$i $i\"" || return 1 done && paths= && for b in o x @@ -24,9 +24,9 @@ test_expect_success setup ' case "$b" in x) echo "$m1$p" ;; esac && case "$o" in x) echo "$m2$p" ;; esac && case "$t" in x) echo "$m3$p" ;; esac || - break - done || break - done || break + return 1 + done + done done >ls-files-s.expect && git update-index --index-info ls-files-s.actual && diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh index 1176bcc..8d90634 100755 --- a/t/t4151-am-abort.sh +++ b/t/t4151-am-abort.sh @@ -20,7 +20,7 @@ test_expect_success setup ' echo $i >otherfile-$i && git add otherfile-$i && test_tick && - git commit -a -m $i || break + git commit -a -m $i || return 1 done && git format-patch --no-numbered initial && git checkout -b side initial && diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index 17c6330..7a8499c 100755 --- a/t/t5505-remote.sh
[PATCH 4/8] t7701: fix ignored exit code inside loop
When checking a list of file mtimes, we use a loop and break
out early from the loop if any entry does not match.
However, the exit code of a loop exited via break is always
0, meaning that the test will fail to notice we had a
mismatch. Since the loop is inside a function, we can fix
this by doing an early "return 1".
Signed-off-by: Jeff King
---
t/t7701-repack-unpack-unreachable.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t7701-repack-unpack-unreachable.sh
b/t/t7701-repack-unpack-unreachable.sh
index aad8a9c..b66e383 100755
--- a/t/t7701-repack-unpack-unreachable.sh
+++ b/t/t7701-repack-unpack-unreachable.sh
@@ -57,7 +57,7 @@ compare_mtimes ()
{
read tref rest &&
while read t rest; do
- test "$tref" = "$t" || break
+ test "$tref" = "$t" || return 1
done
}
--
2.3.4.635.gd6ffcfe
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/8] t0020: fix ignored exit code inside loops
A loop like:
for f in one two; do
something $f ||
break
done
will correctly break out of the loop when we see a failure
of one item, but the resulting exit code will always be
zero. We can fix that by putting the loop into a function or
subshell, but in this case it is simpler still to just
unroll the loop. We do add a helper function, which
hopefully makes the end result even more readable (in
addition to being shorter).
Reported-by: SZEDER Gábor
Signed-off-by: Jeff King
---
You can make each call site a one-liner that does the loop inside the
function (including the update-index call). But I tried to shoot for
readability rather than absolute minimum characters.
t/t0020-crlf.sh | 54 +++---
1 file changed, 19 insertions(+), 35 deletions(-)
diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
index 9fa26df..144fdcd 100755
--- a/t/t0020-crlf.sh
+++ b/t/t0020-crlf.sh
@@ -8,6 +8,13 @@ has_cr() {
tr '\015' Q <"$1" | grep Q >/dev/null
}
+# add or remove CRs to disk file in-place
+# usage: munge_cr
+munge_cr () {
+ "${1}_cr" <"$2" >tmp &&
+ mv tmp "$2"
+}
+
test_expect_success setup '
git config core.autocrlf false &&
@@ -100,14 +107,9 @@ test_expect_success 'update with autocrlf=input' '
rm -f tmp one dir/two three &&
git read-tree --reset -u HEAD &&
git config core.autocrlf input &&
-
- for f in one dir/two
- do
- append_cr <$f >tmp && mv -f tmp $f &&
- git update-index -- $f ||
- break
- done &&
-
+ munge_cr append one &&
+ munge_cr append dir/two &&
+ git update-index -- one dir/two &&
differs=$(git diff-index --cached HEAD) &&
verbose test -z "$differs"
@@ -118,14 +120,9 @@ test_expect_success 'update with autocrlf=true' '
rm -f tmp one dir/two three &&
git read-tree --reset -u HEAD &&
git config core.autocrlf true &&
-
- for f in one dir/two
- do
- append_cr <$f >tmp && mv -f tmp $f &&
- git update-index -- $f ||
- break
- done &&
-
+ munge_cr append one &&
+ munge_cr append dir/two &&
+ git update-index -- one dir/two &&
differs=$(git diff-index --cached HEAD) &&
verbose test -z "$differs"
@@ -136,13 +133,9 @@ test_expect_success 'checkout with autocrlf=true' '
rm -f tmp one dir/two three &&
git config core.autocrlf true &&
git read-tree --reset -u HEAD &&
-
- for f in one dir/two
- do
- remove_cr <"$f" >tmp && mv -f tmp $f &&
- verbose git update-index -- $f ||
- break
- done &&
+ munge_cr remove one &&
+ munge_cr remove dir/two &&
+ git update-index -- one dir/two &&
test "$one" = $(git hash-object --stdin http://vger.kernel.org/majordomo-info.html
[PATCH 3/8] t3305: fix ignored exit code inside loop
When we test deleting notes, we run "git notes remove" in a loop. However, the exit value of the loop will only reflect the final note we process. We should break out of the loop with a failing exit code as soon as we see a problem. Note that we can call "exit 1" here without explicitly creating a subshell, because the while loop on the right-hand side of a pipe executes in its own implicit subshell. Note also that the "break" above does not suffer the same problem; it is meant to exit the loop early at a certain number of iterations. We can bump it into the conditional of the loop to make this more obvious. Signed-off-by: Jeff King --- t/t3305-notes-fanout.sh | 9 +++-- 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/t/t3305-notes-fanout.sh b/t/t3305-notes-fanout.sh index b1ea64b..54460be 100755 --- a/t/t3305-notes-fanout.sh +++ b/t/t3305-notes-fanout.sh @@ -51,15 +51,12 @@ test_expect_success 'deleting most notes with git-notes' ' num_notes=250 && i=0 && git rev-list HEAD | - while read sha1 + while test $i -lt $num_notes && read sha1 do i=$(($i + 1)) && - if test $i -gt $num_notes - then - break - fi && test_tick && - git notes remove "$sha1" + git notes remove "$sha1" || + exit 1 done ' -- 2.3.4.635.gd6ffcfe -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/8] perf-lib: fix ignored exit code inside loop
When copying the test repository, we try to detect whether
the copy succeeded. However, most of the heavy lifting is
done inside a for loop, where our "break" will lose the exit
code of the failing "cp". We can take advantage of the fact
that we are in a subshell, and just "exit 1" to break out
with a code.
Signed-off-by: Jeff King
---
t/perf/perf-lib.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index a8c9574..5cf74ed 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -91,7 +91,7 @@ test_perf_create_repo_from () {
*/objects|*/hooks|*/config)
;;
*)
- cp -R "$stuff" . || break
+ cp -R "$stuff" . || exit 1
;;
esac
done &&
--
2.3.4.635.gd6ffcfe
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/8] more &&-chaining test fixups
Here's what I found looking for loops like: for i in a b c; do something_important $i || break done && something_else which presumably expect the chain to stop when something_important fails for any loop element. The solutions are one of (depending on the surrounding code): 1. Switch the break to "return 1". The tests are all &&-chained, so the effect of a failed command is to exit the test immediately anyway. And we wrap our eval'd test snippets inside an extra layer of function call to explicitly allow early returns like this. 2. Switch the break to "exit 1". Calling "return" from a subshell inside a function is a bit weird. It doesn't exit the function at all, but rather just the subshell (in both bash and dash). But if you are not in a function, calling "return" at all is an error in bash (subshell or no), and OK in dash (where it acts like "exit"). POSIX explicitly marks the "outside of a function" behavior as unspecified, but I couldn't find anything about the subshell behavior. So I'm loathe to depend on it, even though it does seem to do what we want, as I do not want to even think what some more obscure shells might do with it. And especially because we know that "exit 1" portably does what we want (the only downside is that you have to recognize which situation you are in and use "exit" versus "return"). 3. Unroll the loops. In some cases the result is actually shorter, and (IMHO) more readable. These sites were all found with: git grep -E '(^|\|\|)[]*break' t/t*.sh (that's a space and a tab in the brackets). There are some matches there that I did not touch, because they were already fine. E.g., t5302 and t7700 use loops to assign a value to a variable and break out early, and then check the value of the variable. That's just the tip of the iceberg, though. Searching for git grep 'for .* in ' yields hundreds of hits. Most of which are probably fine (quite a few are outside &&-chains entirely). I focused on the ones that called break, because that indicated to me that the author was trying to address the &&-chain. Certainly anybody else is welcome to take a stab at the rest, but I'm also happy to fix them up as we touch nearby code and notice them. Most of the loops are in setup code that we do not expect to fail anyway, so examining them is a lot of work for a little gain. There were a few legitimate problems, though. I've ordered the patches below by descending severity. These apply on top of jk/test-chain-lint. [1/8]: perf-lib: fix ignored exit code inside loop [2/8]: t0020: fix ignored exit code inside loops [3/8]: t3305: fix ignored exit code inside loop [4/8]: t7701: fix ignored exit code inside loop These four are actual bugs. [5/8]: t: fix some trivial cases of ignored exit codes in loops These ones are in setup code, and so would almost certainly never fail. [6/8]: t: simplify loop exit-code status variables [7/8]: t0020: use test_* helpers instead of hand-rolled messages [8/8]: t9001: drop save_confirm helper These last three are pure cleanup, no behavior changes. The last two are not even strictly related to the same topic, but I noticed them while in the area. -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 01/25] t/test-lib: introduce --chain-lint option
On Wed, Mar 25, 2015 at 03:53:52AM +0100, SZEDER Gábor wrote: > > cmd1 && > > for i in a b c; do > > cmd2 $i > > done && > > cmd3 > > > > which will not notice failures of "cmd2 a" or "cmd b" > > s/cmd b/cmd2 b/ ? Yes, but the patches are already in next, so it is sadly too late for commit message fixups. > > - it cannot find a missing &&-chain inside a block or > > subfunction, like: > [...] > And inside subshells. [...] Yeah, I had mentally filed them with "block", but true subshells are probably the most common place. However, I'd suspect a good portion of them are going to be the "trivial" type, especially if they involve setting up the sub-environment at the top of the subshell. E.g., something like this: cmd1 && ( FOO=bar; export FOO cmd2 ) && cmd3 does not break the outer chain (which is what --chain-lint checks). It does break the chain inside the subshell, but we never expect variable assignment or export to fail (it is nice to fix it, of course, but the main purpose in fixing the ones in my "trivial" patch was more about shutting up --chain-lint to make the real breakages more obvious). -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 17/25] t0020: use modern test_* helpers
On Wed, Mar 25, 2015 at 01:23:23AM +0100, SZEDER Gábor wrote:
> > for f in one dir/two
> > do
> > append_cr <$f >tmp && mv -f tmp $f &&
> >-git update-index -- $f || {
> >-echo Oops
> >-false
> >-break
> >-}
> >+git update-index -- $f ||
> >+break
> > done &&
>
> Ah, these tests are evil, I remember them from the time when I was fiddling
> with Jonathan's patch. They can fail silently without testing what they
> were supposed to test.
>
> If something in the loop fails, the break will leave the loop but it will do
> so with zero return value and, consequently, the test will continue as if
> everything were OK.
> And unless it was 'git update-index' that failed in a way that left a borked
> index behind, the 'git diff-index --cached' below will not error out or
> produce some output that would cause the test to fail. i.e. I tried e.g.
>
> append_cr <$f >tmp && mv -f tmp $f && false &&
>
> in the loop and the test succeeded.
Ugh, you're right. I remembered that for loops were tricky in &&-chains,
but for some reason was thinking that "break" would give you the last
exit code, But I just re-tested, and of course it does not work.
> I think the best fix would be to unroll the loop: after this patch the loop
> body consists of only two significant lines and we iterate through the loop
> only twice, so the test would be even shorter.
Yeah, unrolling may be the best thing here, given the size of the loops.
As a general rule, I think it has to be a subshell with an exit, like:
(
for i in one two three; do
echo $i &&
test $i = one ||
exit 1
done
)
echo exit=$?
which should yield one, two, and exit=1. 7b1732c (t7510: use consistent
&&-chains in loop, 2014-06-16) deals with this in another test.
-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 01/25] t/test-lib: introduce --chain-lint option
Quoting Jeff King :
However, there are a number of places it cannot reach:
- it cannot find a failure to break out of loops on error,
like:
cmd1 &&
for i in a b c; do
cmd2 $i
done &&
cmd3
which will not notice failures of "cmd2 a" or "cmd b"
s/cmd b/cmd2 b/ ?
- it cannot find a missing &&-chain inside a block or
subfunction, like:
foo () {
cmd1
cmd2
}
foo &&
bar
which will not notice a failure of cmd1.
And inside subshells. I think it's worth mentioning, too, because
subshells are used frequently when setting environment variables
( GIT_FOO=bar && export GIT_FOO && cmd1 && ... ) && test_cmp
or changing directory
( cd subdir && cmd1 && ... ) && test_cmp
I was wondering whether we could do better here with helper functions,
something along the lines of:
# Set and export environment variable, automatically unsetting it after
# the test is over
test_setenv () {
eval "$1=\$2" &&
export "$1" &&
# sane_unset, to allow unsetting during the test
test_when_finished "sane_unset '$1'"
}
# Change to given directory, automatically return to current working
# directory after the test is over
test_cd () {
test_when_finished "cd '$PWD'" &&
cd "$1"
}
With these the above examples would become
test_setenv GIT_FOO bar && cmd1 && ... && test_cmp
and
test_cd subdir && cmd1 && ... && test_cmp
which means increased coverage for --chain-lint.
Thanks for working on this. I looked into this after seeing Jonathan's
patch back then, got quite far but never reached a chain-lint-clean
state, and only sent patches for the two most amusing breakages
(ddeaf7ef0d, 056f34bbcd).
I'm glad it's off my TODO list and I don't have to rebase a 1.5 year old
branch to current master :)
Gábor
- it only checks tests that you run; every platform will
have some tests skipped due to missing prequisites,
so it's impossible to say from one run that the test
suite is free of broken &&-chains. However, all tests get
run by _somebody_, so eventually we will notice problems.
- it does not operate on test_when_finished or prerequisite
blocks. It could, but these tends to be much shorter and
less of a problem, so I punted on them in this patch.
This patch was inspired by an earlier patch by Jonathan
Nieder:
http://article.gmane.org/gmane.comp.version-control.git/235913
This implementation and all bugs are mine.
Signed-off-by: Jeff King
---
t/README | 10 ++
t/test-lib.sh | 16
2 files changed, 26 insertions(+)
diff --git a/t/README b/t/README
index d5bb0c9..35438bc 100644
--- a/t/README
+++ b/t/README
@@ -168,6 +168,16 @@ appropriately before running "make".
Using this option with a RAM-based filesystem (such as tmpfs)
can massively speed up the test suite.
+--chain-lint::
+--no-chain-lint::
+ If --chain-lint is enabled, the test harness will check each
+ test to make sure that it properly "&&-chains" all commands (so
+ that a failure in the middle does not go unnoticed by the final
+ exit code of the test). This check is performed in addition to
+ running the tests themselves. You may also enable or disable
+ this feature by setting the GIT_TEST_CHAIN_LINT environment
+ variable to "1" or "0", respectively.
+
You can also set the GIT_TEST_INSTALLED environment variable to
the bindir of an existing git installation to test that installation.
You still need to have built this git sandbox, from which various
diff --git a/t/test-lib.sh b/t/test-lib.sh
index c096778..50b3d3f 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -232,6 +232,12 @@ do
--root=*)
root=$(expr "z$1" : 'z[^=]*=\(.*\)')
shift ;;
+ --chain-lint)
+ GIT_TEST_CHAIN_LINT=1
+ shift ;;
+ --no-chain-lint)
+ GIT_TEST_CHAIN_LINT=0
+ shift ;;
-x)
trace=t
verbose=t
@@ -524,6 +530,16 @@ test_eval_ () {
test_run_ () {
test_cleanup=:
expecting_failure=$2
+
+ if test "${GIT_TEST_CHAIN_LINT:-0}" != 0; then
+ # 117 is magic because it is unlikely to match the exit
+ # code of other programs
+ test_eval_ "(exit 117) && $1"
+ if test "$?" != 117; then
+ error "bug in the test script: broken &&-chain: $1"
+ fi
+ fi
+
setup_malloc_check
test_eval_ "$1"
eval_ret=$?
--
2.3.3.520.g3cfbb5d
--
To unsubscribe from this list: send the line "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 21/25] t9001: use test_when_finished
On Wed, Mar 25, 2015 at 03:00:22AM +0100, SZEDER Gábor wrote: > >Instead, they can all use test_when_finished, and we can > >even make the code simpler by factoring out the shared > >lines. > > I think that saving the value of 'sendemail.confirm' is not necessary. > > There are two blocks of confirmation tests, this patch concerns only tests > of the second block. The first block of confirmation tests is nearly at > the beginning of the file in order to check the "no confirm" cases early. > If any of those fails the remainig tests in the file are skipped because > they might hang. The last of those tests sets 'sendemail.confirm' to > 'never' and leaves it so to avoid unintentional prompting in the remaining > tests and then its value is not modified until that second block of > confirm tests are reached. This means that when those tests save the > value of 'sendemail.confirm' they always save 'never'. Then why save it, > just use test_when_finished to restore it to 'never' and all is well. Yeah, I suspected this while writing it the patch, but I preferred to keep it more obvious that there would be no accidental regression, since the series was already so long (and also because calling save_confirm is not any worse than test_when_finished). I don't mind a patch on top simplifying out save_confirm, if you're confident that's what we're always saving. -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 18/25] t1301: use modern test_* helpers
On Wed, Mar 25, 2015 at 12:51:20AM +0100, SZEDER Gábor wrote: > >@@ -33,7 +32,7 @@ do > > git init --shared=1 && > > test 1 = "$(git config core.sharedrepository)" > > ) && > >-actual=$(ls -l sub/.git/HEAD) > >+actual=$(ls -l sub/.git/HEAD) && > > case "$actual" in > > -rw-rw-r--*) > > : happy > > This hunk could go into the "moderate &&-chain breakage" patch. > Doesn't really matter, what matters most is that it's fixed, but I really > liked your classification of missing &&s in the early patches. Yeah, these later ones are a mish-mash of real fixes and just quieting --chain-lint. I pulled out ones that I felt needed a little more explanation, and generally kept changes for a file together (though I am not sure not always). I'm not sure it's worth the effort to go through another round of classifying. -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 21/25] t9001: use test_when_finished
Quoting Jeff King :
The confirmation tests in t9001 all save the value of
sendemail.confirm, do something to it, then restore it at
the end, in a way that breaks the &&-chain (they are not
wrong, because they save the $? value, but it fools
--chain-lint).
Instead, they can all use test_when_finished, and we can
even make the code simpler by factoring out the shared
lines.
I think that saving the value of 'sendemail.confirm' is not necessary.
There are two blocks of confirmation tests, this patch concerns only tests
of the second block. The first block of confirmation tests is nearly at
the beginning of the file in order to check the "no confirm" cases early.
If any of those fails the remainig tests in the file are skipped because
they might hang. The last of those tests sets 'sendemail.confirm' to
'never' and leaves it so to avoid unintentional prompting in the remaining
tests and then its value is not modified until that second block of
confirm tests are reached. This means that when those tests save the
value of 'sendemail.confirm' they always save 'never'. Then why save it,
just use test_when_finished to restore it to 'never' and all is well.
Note that we can _almost_ use test_config here, except that:
1. We do not restore the config with test_unconfig, but by
setting it back to some prior value.
2. We are not always setting a config variable. Sometimes
the change to be undone is unsetting it entirely.
We could teach test_config to handle these cases, but it's
not worth the complexity for a single call-site.
Signed-off-by: Jeff King
---
t/t9001-send-email.sh | 30 ++
1 file changed, 10 insertions(+), 20 deletions(-)
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 37caa18..c9f54d5 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -817,26 +817,25 @@ test_expect_success $PREREQ '--confirm=compose' '
test_confirm --confirm=compose --compose
'
-test_expect_success $PREREQ 'confirm by default (due to cc)' '
+save_confirm () {
CONFIRM=$(git config --get sendemail.confirm) &&
+ test_when_finished "git config sendemail.confirm ${CONFIRM:-never}"
+}
+
+test_expect_success $PREREQ 'confirm by default (due to cc)' '
+ save_confirm &&
git config --unset sendemail.confirm &&
test_confirm
- ret="$?"
- git config sendemail.confirm ${CONFIRM:-never}
- test $ret = "0"
'
test_expect_success $PREREQ 'confirm by default (due to --compose)' '
- CONFIRM=$(git config --get sendemail.confirm) &&
+ save_confirm &&
git config --unset sendemail.confirm &&
test_confirm --suppress-cc=all --compose
- ret="$?"
- git config sendemail.confirm ${CONFIRM:-never}
- test $ret = "0"
'
test_expect_success $PREREQ 'confirm detects EOF (inform assumes y)' '
- CONFIRM=$(git config --get sendemail.confirm) &&
+ save_confirm &&
git config --unset sendemail.confirm &&
rm -fr outdir &&
git format-patch -2 -o outdir &&
@@ -846,13 +845,10 @@ test_expect_success $PREREQ 'confirm detects EOF
(inform assumes y)' '
--to=nob...@example.com \
--smtp-server="$(pwd)/fake.sendmail" \
outdir/*.patch
--
To unsubscribe from this list: send the line "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: What's cooking in git.git (Mar 2015, #08; Mon, 23)
Jakub Narębski writes: > On Tue, Mar 24, 2015 at 11:26 PM, Junio C Hamano wrote: >> >> Junio C Hamano writes: >> >> > * jn/gitweb-utf8-in-links (2014-05-27) 1 commit >> > - gitweb: Harden UTF-8 handling in generated links >> >> This has been lingering in my 'pu' branch without seeing any updates >> since $gmane/250758; is anybody still interested in resurrecting it >> and moving it forward? > > I can try to pick it up, but I am no longer sure that it is a good idea > to solve the problem. After re-reading the discussion thread, I had the same impression. Let's drop the patch for now. It can be re-raised as/if needed. 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] gc: save log from daemonized gc --auto and print it next time
On Wed, Mar 25, 2015 at 5:07 AM, Junio C Hamano wrote: >> + LANG=C git gc --auto && >> + sleep 1 && # give it time to daemonize >> + while test -f .git/gc.pid; do sleep 1; done && > > Yuck... Yeah.. it's hard to test daemon things. I'm not even sure if we should add a test, but I tried anyway. >> + grep "too many unreachable loose objects" .git/gc.log && >> + LANG=C git gc --auto 2>error && >> + grep skipped error && >> + grep "too many unreachable loose objects" error && >> + ! test -f .git/gc.log >> + ) >> +' > > For that "17/ has very many loose objects that are still young and > unreachable" issue, I wonder if the right solution is somehow to > flag the repository and prevent "gc --auto" from running until the > situation improves. "I checked at this time and found too many in > 17/"; upon finding that flag file (with a timestamp), if there are > new files in 17/ or if there are other reasons to do a gc (perhaps > there are too many packfiles to be consolidated?), then do the gc > but otherwise quit silently before spending too many cycles on it, > or something along that line? That's a separate problem that's being discussed in another thread. I think Jeff's idea of storing the number of estimated loose objects may be more reliable than timestamps.. -- Duy -- To unsubscribe from this list: send the line "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: Sparse checkout not working as expected (colons in filenames on Windows)
On Wed, Mar 25, 2015 at 6:50 AM, Philip Oakley wrote: > I've corrected the sparse-checkout, but won't the command line 'git > update-index --skip-worktree' will still need it? (demo commands below) A "git checkout" (without arguments) or "read-trree -mu" should attach skip-worktree properly. You don't need to do it yourself. > That said, the final error (which I'd missed in the earlier post) is: > fatal: make_cache_entry failed for path 'ifcfg-eth0:0' > > This is on the Windows (pre-compiled msysgit at v1.9.5) Git bash, so could > be a catch path in that code for make_cache_entry (I've not checked the code > yet). So at the moment it doesn't look like sparse checkout can be used to > avoid colons in windows on-disk files based on the current code. Both of your commands below fail by the same function, verify_path() because of this msysgit commit 2e2a2d1 (NTFS: Prevent problematic paths from being checked out - 2014-12-10). I guess that check is a bit too strong, it should apply when new index entries are created from worktree (not from a tree).. > -- > Philip > > Philip@PHILIPOAKLEY /d/Git_repos/colons > $ cd tortoisegit-colons/ > > Philip@PHILIPOAKLEY /d/Git_repos/colons/tortoisegit-colons (test) > $ git update-index --skip-worktree -- ifcfg-eth0\:0 > Ignoring path ifcfg-eth0:0 > > Philip@PHILIPOAKLEY /d/Git_repos/colons/tortoisegit-colons (test) > $ git reset > error: Invalid path 'ifcfg-eth0:0' > fatal: make_cache_entry failed for path 'ifcfg-eth0:0' -- Duy -- To unsubscribe from this list: send the line "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: What's cooking in git.git (Mar 2015, #08; Mon, 23)
On Tue, Mar 24, 2015 at 11:26 PM, Junio C Hamano wrote: > > Junio C Hamano writes: > > > * jn/gitweb-utf8-in-links (2014-05-27) 1 commit > > - gitweb: Harden UTF-8 handling in generated links > > This has been lingering in my 'pu' branch without seeing any updates > since $gmane/250758; is anybody still interested in resurrecting it > and moving it forward? I can try to pick it up, but I am no longer sure that it is a good idea to solve the problem. In particular git does not require that paths (i.e. filesystem) use utf-8 encoding; it could use latin1 / iso-8859-1 and non-utf8 octet. -- Jakub Narębski -- To unsubscribe from this list: send the line "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 17/25] t0020: use modern test_* helpers
Quoting Jeff King :
This test contains a lot of hand-rolled messages to show
when the test fails. We can omit most of these by using
"verbose" and "test_must_fail". A few of them are for
update-index, but we can assume it produces reasonable error
messages when it fails.
Signed-off-by: Jeff King
---
t/t0020-crlf.sh | 144
+++-
1 file changed, 28 insertions(+), 116 deletions(-)
diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
index d2e51a8..9fa26df 100755
--- a/t/t0020-crlf.sh
+++ b/t/t0020-crlf.sh
@@ -104,18 +104,12 @@ test_expect_success 'update with autocrlf=input' '
for f in one dir/two
do
append_cr <$f >tmp && mv -f tmp $f &&
- git update-index -- $f || {
- echo Oops
- false
- break
- }
+ git update-index -- $f ||
+ break
done &&
Ah, these tests are evil, I remember them from the time when I was
fiddling with Jonathan's patch. They can fail silently without
testing what they were supposed to test.
If something in the loop fails, the break will leave the loop but it
will do so with zero return value and, consequently, the test will
continue as if everything were OK.
And unless it was 'git update-index' that failed in a way that left a
borked index behind, the 'git diff-index --cached' below will not
error out or produce some output that would cause the test to fail.
i.e. I tried e.g.
append_cr <$f >tmp && mv -f tmp $f && false &&
in the loop and the test succeeded.
I think the best fix would be to unroll the loop: after this patch the
loop body consists of only two significant lines and we iterate
through the loop only twice, so the test would be even shorter.
differs=$(git diff-index --cached HEAD) &&
- test -z "$differs" || {
- echo Oops "$differs"
- false
- }
+ verbose test -z "$differs"
'
@@ -128,18 +122,12 @@ test_expect_success 'update with autocrlf=true' '
for f in one dir/two
do
append_cr <$f >tmp && mv -f tmp $f &&
- git update-index -- $f || {
- echo "Oops $f"
- false
- break
- }
+ git update-index -- $f ||
+ break
done &&
differs=$(git diff-index --cached HEAD) &&
- test -z "$differs" || {
- echo Oops "$differs"
- false
- }
+ verbose test -z "$differs"
'
@@ -152,19 +140,13 @@ test_expect_success 'checkout with autocrlf=true' '
for f in one dir/two
do
remove_cr <"$f" >tmp && mv -f tmp $f &&
- git update-index -- $f || {
- echo "Eh? $f"
- false
- break
- }
+ verbose git update-index -- $f ||
+ break
done &&
test "$one" = $(git hash-object --stdin .gitattributes &&
git read-tree --reset -u HEAD &&
- if has_cr dir/two
- then
- : happy
- else
- echo "Huh?"
- false
- fi &&
-
- if has_cr three
- then
- : happy
- else
- echo "Huh?"
- false
- fi
+ verbose has_cr dir/two &&
+ verbose has_cr three
'
test_expect_success 'in-tree .gitattributes (1)' '
@@ -352,17 +300,8 @@ test_expect_success 'in-tree .gitattributes (1)' '
rm -rf tmp one dir .gitattributes patch.file three &&
git read-tree --reset -u HEAD &&
- if has_cr one
- then
- echo "Eh? one should not have CRLF"
- false
- else
- : happy
- fi &&
- has_cr three || {
- echo "Eh? three should still have CRLF"
- false
- }
+ test_must_fail has_cr one &&
+ verbose has_cr three
'
test_expect_success 'in-tree .gitattributes (2)' '
@@ -371,17 +310,8 @@ test_expect_success 'in-tree .gitattributes (2)' '
git read-tree --reset HEAD &&
git checkout-index -f -q -u -a &&
- if has_cr one
- then
- echo "Eh? one should not have CRLF"
- false
- else
- : happy
- fi &&
- has_cr three || {
- echo "Eh? three should still have CRLF"
- false
- }
+ test_must_fail has_cr one &&
+ verbose has_cr three
'
test_expect_success 'in-tree .gitattributes (3)' '
@@ -391,17 +321,8 @@ test_expect_success 'in-tree .gitattributes (3)' '
git checkout-index -u .gitattributes &&
git checkout-index -u one dir/two three &&
- if has_cr one
- then
- echo "Eh? one should not have CRLF"
- false
- else
-
Re: [PATCH 18/25] t1301: use modern test_* helpers
Quoting Jeff King :
This shortens the code and fixes some &&-chaining.
Signed-off-by: Jeff King
---
t/t1301-shared-repo.sh | 20 +++-
1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 7eecfb8..ac10875 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -12,12 +12,11 @@ setfacl -k . 2>/dev/null
# User must have read permissions to the repo -> failure on --shared=0400
test_expect_success 'shared = 0400 (faulty permission u-w)' '
+ test_when_finished "rm -rf sub" &&
mkdir sub && (
- cd sub && git init --shared=0400
+ cd sub &&
+ test_must_fail git init --shared=0400
)
- ret="$?"
- rm -rf sub
- test $ret != "0"
'
modebits () {
@@ -33,7 +32,7 @@ do
git init --shared=1 &&
test 1 = "$(git config core.sharedrepository)"
) &&
- actual=$(ls -l sub/.git/HEAD)
+ actual=$(ls -l sub/.git/HEAD) &&
case "$actual" in
-rw-rw-r--*)
: happy
This hunk could go into the "moderate &&-chain breakage" patch.
Doesn't really matter, what matters most is that it's fixed, but I
really liked your classification of missing &&s in the early patches.
@@ -90,10 +89,8 @@ do
rm -f .git/info/refs &&
git update-server-info &&
actual="$(modebits .git/info/refs)" &&
- test "x$actual" = "x-$y" || {
- ls -lt .git/info
- false
- }
+ verbose test "x$actual" = "x-$y"
+
'
umask 077 &&
@@ -102,10 +99,7 @@ do
rm -f .git/info/refs &&
git update-server-info &&
actual="$(modebits .git/info/refs)" &&
- test "x$actual" = "x-$x" || {
- ls -lt .git/info
- false
- }
+ verbose test "x$actual" = "x-$x"
'
--
2.3.3.520.g3cfbb5d
--
To unsubscribe from this list: send the line "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: Sparse checkout not working as expected (colons in filenames on Windows)
From: "Duy Nguyen" On Fri, Mar 20, 2015 at 6:07 AM, Philip Oakley wrote: Hi, I was expecting that sparse checkout could be used to avoid the checking out, by git, of files which have colons in their name into the worktree when on Windows. Yue Lin Ho reported on the Msygit list [1] that he had a repo where there was already committed a file with a colon in it's name, which was a needed file and had been committed by a Linux user. The problem was how to work with that repo on a Windows box where such a file is prohibited to exist on the FS (hence the expectation that a sparse checkout would suffice). Yue has created a short test repo [2] Even after getting the pathspec escaping right, I still haven't been able to make this expected behaviour work [3]. Am I wrong to expect that sparse checkout (and the skip-worktree bit) can be used to avoid files with undesirable filenames hitting the OS's file system? If it should be OK, what's the correct recipe? -- Philip [1] https://groups.google.com/forum/?hl=en_US?hl%3Den#!topic/msysgit/D4HcHRpxPgU "How to play around with the filename with colon on Windows?" [2] Test repo https://github.com/t-pascal/tortoisegit-colons [3] test sequence:: $ mkdir colons && cd colons $ git clone -n https://github.com/t-pascal/tortoisegit-colons $ cd tortoisegit-colons/ $ git config core.sparseCheckout true $ cat .git/info/sparse-checkout # external editor /* !ifcfg-eth0\:0 Colons have no special meaning in gitignore rules and therefore need not be escaped. The backslash is considered a literal character in this case, probably not what you want. $ git update-index --skip-worktree -- ifcfg-eth0\:0 Ignoring path ifcfg-eth0:0 $ git checkout -b test 7f35d34bc6160cc # tip commit, we are still unborn! error: Invalid path 'ifcfg-eth0:0 D ifcfg-eth0:0 Switched to a new branch 'test' -- I've corrected the sparse-checkout, but won't the command line 'git update-index --skip-worktree' will still need it? (demo commands below) That said, the final error (which I'd missed in the earlier post) is: fatal: make_cache_entry failed for path 'ifcfg-eth0:0' This is on the Windows (pre-compiled msysgit at v1.9.5) Git bash, so could be a catch path in that code for make_cache_entry (I've not checked the code yet). So at the moment it doesn't look like sparse checkout can be used to avoid colons in windows on-disk files based on the current code. -- Philip Philip@PHILIPOAKLEY /d/Git_repos/colons $ cd tortoisegit-colons/ Philip@PHILIPOAKLEY /d/Git_repos/colons/tortoisegit-colons (test) $ git update-index --skip-worktree -- ifcfg-eth0\:0 Ignoring path ifcfg-eth0:0 Philip@PHILIPOAKLEY /d/Git_repos/colons/tortoisegit-colons (test) $ git reset error: Invalid path 'ifcfg-eth0:0' fatal: make_cache_entry failed for path 'ifcfg-eth0:0' -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 19/25] t6034: use modern test_* helpers
Quoting Jeff King : These say roughly the same thing as the hand-rolled messages. We do lose the "merge did not complete" debug message, but merge and write-tree are prefectly capable of s/prefectly/perfectly/ writing useful error messages when they fail. Signed-off-by: Jeff King --- -- To unsubscribe from this list: send the line "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: What's cooking in git.git (Mar 2015, #08; Mon, 23)
Junio C Hamano writes: > Here are the topics that have been cooking. Commits prefixed with > '-' are only in 'pu' (proposed updates) while commits prefixed with > '+' are in 'next'. > > This cycle is turning out to be a "shoot for product excellence" > release. About half of the commits that have been merged to the > 'master' so far have also been merged to 'maint' and v2.3.4 has been > tagged. I've merged a few more topics to 'next' and hopefully will push the result out in a few hours. Some topics that are in 'next' but not in 'master' are high-impact ones that I'd feel more comfortable if we cooked them there for a bit more, and am planning to merge them (if there are no issues discovered in the meantime, of course) after this cycle finishes, but for others, I'm hoping to merge them to 'master' before we tag the -rc0 preview release. Please give them a good beating and report any regressions you find. 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] Documentation: Add target to build PDF manpages
From: "Michael J Gruber" Junio C Hamano venit, vidit, dixit 20.03.2015 23:38: Stefan Beller writes: Thomas referencing reading the man page offline, made me wonder why you wouldn't read the man pages itself as they can also be carried around offline. But the striking point is "on an iPad", which doesn't offer you the convenience of a shell etc, but pdf is fine to read there. Also you can add comments to pdfs more easily that html pages I'd guess. So the patch makes sense to me now. It's just a use case I'm personally not interested in for now, but I don't oppose it as is. Well, my comment was not about opposing to it, but was about questioning the usefulness of it, iow, who would benefit from having this patch in my tree? I didn't see (and I still do not quite see) why people would want to have separate pdf files for all the subcommands (instead of say an .epub or .pdf that binds all the man pages and perhaps user-manual, just like we do for .texi/.info). Exactly. For PDF, a combined document is more natural and will hopefully make crosslinks work as crossrefs within one document, rather than links to external documents. I'd say that would make a valuable target. As per the original request, it is useful to some, and the usefulness of a very large pdf containing all the documentation shouldn't be a reason to not have such a 'one at a time' target available (though personally I would suggest that it is the users responsibility to 'make' such a target, not the maintainers!). The single large pdf has also been discussed (http://thread.gmane.org/gmane.comp.version-control.git/207151/focus=207165) but didn't get into the code base either. The user-manual is available as a pdf target. Philip -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: A good time to pull from your gitk tree?
On Mon, Mar 23, 2015 at 12:03:37PM -0700, Junio C Hamano wrote: > > Is it a good time for me to pull from you, or do you recommend me to > wait for a bit, expecting more? We'll go in the pre-release freeze > soon-ish, so I thought I should ping. Now is a good time to pull from the usual place, thanks. Regards, Paul. -- To unsubscribe from this list: send the line "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: What's cooking in git.git (Mar 2015, #08; Mon, 23)
Junio C Hamano writes: > * jn/gitweb-utf8-in-links (2014-05-27) 1 commit > - gitweb: Harden UTF-8 handling in generated links This has been lingering in my 'pu' branch without seeing any updates since $gmane/250758; is anybody still interested in resurrecting it and moving it forward? 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: What's cooking in git.git (Mar 2015, #08; Mon, 23)
> * pw/remote-set-url-fetch (2014-11-26) 1 commit > - remote: add --fetch and --both options to set-url This has not seen any activity for a few months since $gmane/261483; is anybody still interested in resurrecting it? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gc: save log from daemonized gc --auto and print it next time
On Tue, Mar 24, 2015 at 8:17 AM, Nguyễn Thái Ngọc Duy wrote:
> While commit 9f673f9 (gc: config option for running --auto in
> background - 2014-02-08) helps reduce some complaints about 'gc
> --auto' hogging the terminal, it creates another set of problems.
>
> The latest in this set is, as the result of daemonizing, stderr is
> closed and all warnings are lost. This warning at the end of cmd_gc()
> is particularly important because it tells the user how to avoid "gc
> --auto" running repeatedly. Because stderr is closed, the user does
> not know, naturally they complain about 'gc --auto' wasting CPU.
>
> Besides reverting 9f673f9 and looking at the problem from another
> angle, we could save the stderr in $GIT_DIR/gc.log. Next time, 'gc
> --auto' will print the saved warnings, delete gc.log and exit.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> builtin/gc.c | 37 -
> t/t6500-gc.sh | 20
> 2 files changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 5c634af..07769a9 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -32,6 +32,8 @@ static int aggressive_window = 250;
> static int gc_auto_threshold = 6700;
> static int gc_auto_pack_limit = 50;
> static int detach_auto = 1;
> +static struct strbuf log_filename = STRBUF_INIT;
> +static int daemonized;
> static const char *prune_expire = "2.weeks.ago";
>
> static struct argv_array pack_refs_cmd = ARGV_ARRAY_INIT;
> @@ -44,6 +46,15 @@ static char *pidfile;
>
> static void remove_pidfile(void)
> {
> + if (daemonized && log_filename.len) {
> + struct stat st;
> +
> + close(2);
> + if (stat(log_filename.buf, &st) ||
> + !st.st_size ||
> + rename(log_filename.buf, git_path("gc.log")))
> + unlink(log_filename.buf);
> + }
> if (pidfile)
> unlink(pidfile);
> }
> @@ -324,13 +335,25 @@ int cmd_gc(int argc, const char **argv, const char
> *prefix)
> fprintf(stderr, _("See \"git help gc\" for manual
> housekeeping.\n"));
> }
> if (detach_auto) {
> + struct strbuf sb = STRBUF_INIT;
> +
> + if (strbuf_read_file(&sb, git_path("gc.log"), 0) > 0)
> {
> + warning(_("Last gc run reported the
> following, gc skipped"));
When I read this message, it makes me think that the previous gc was
skipped, even though it's actually skipping the current one. Perhaps
rephrase as "skipping gc; last gc reported:"?
> + fputs(sb.buf, stderr);
> + strbuf_release(&sb);
> + /* let the next gc --auto run as usual */
> + unlink(git_path("gc.log"));
> + return 0;
> + }
> +
> if (gc_before_repack())
> return -1;
> /*
> * failure to daemonize is ok, we'll continue
> * in foreground
> */
> - daemonize();
> + if (!daemonize())
> + daemonized = 1;
> }
> } else
> add_repack_all_option();
> @@ -343,6 +366,18 @@ int cmd_gc(int argc, const char **argv, const char
> *prefix)
> name, (uintmax_t)pid);
> }
>
> + if (daemonized) {
> + int fd;
> +
> + strbuf_addstr(&log_filename, git_path("gc.log_XX"));
> + fd = xmkstemp(log_filename.buf);
> + if (fd >= 0) {
> + dup2(fd, 2);
> + close(fd);
> + } else
> + strbuf_release(&log_filename);
> + }
> +
> if (gc_before_repack())
> return -1;
>
> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
> index 63194d8..54bc9c4 100755
> --- a/t/t6500-gc.sh
> +++ b/t/t6500-gc.sh
> @@ -30,4 +30,24 @@ test_expect_success 'gc -h with invalid configuration' '
> test_i18ngrep "[Uu]sage" broken/usage
> '
>
> +test_expect_success !MINGW 'gc --auto and logging' '
> + git init abc &&
> + (
> + cd abc &&
> + # These create blobs starting with the magic number "17"
> + for i in 901 944; do
> + echo $i >test && git hash-object -w test >/dev/null
> + done &&
> + git config gc.auto 1 &&
> + LANG=C git gc --auto &&
> + sleep 1 && # give it time to daemonize
> + while test -f .git/gc.pid; do sleep 1; done &&
> + grep "too many unreachable loose objects" .git/gc.log &&
> + LANG=C git gc --auto 2>error &&
> +
Re: [PATCH] gc: save log from daemonized gc --auto and print it next time
Nguyễn Thái Ngọc Duy writes: > While commit 9f673f9 (gc: config option for running --auto in > background - 2014-02-08) helps reduce some complaints about 'gc > --auto' hogging the terminal, it creates another set of problems. > > The latest in this set is, as the result of daemonizing, stderr is > closed and all warnings are lost. This warning at the end of cmd_gc() > is particularly important because it tells the user how to avoid "gc > --auto" running repeatedly. Because stderr is closed, the user does > not know, naturally they complain about 'gc --auto' wasting CPU. > > Besides reverting 9f673f9 and looking at the problem from another > angle, we could save the stderr in $GIT_DIR/gc.log. Next time, 'gc > --auto' will print the saved warnings, delete gc.log and exit. I wonder what this buys us if multiple auto-gc's are triggered because the user is running a long svn import or something similar. > diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh > index 63194d8..54bc9c4 100755 > --- a/t/t6500-gc.sh > +++ b/t/t6500-gc.sh > @@ -30,4 +30,24 @@ test_expect_success 'gc -h with invalid configuration' ' > test_i18ngrep "[Uu]sage" broken/usage > ' > > +test_expect_success !MINGW 'gc --auto and logging' ' > + git init abc && > + ( > + cd abc && > + # These create blobs starting with the magic number "17" > + for i in 901 944; do There are numbers smaller than these, like 263 and 410 ;-) > + echo $i >test && git hash-object -w test >/dev/null "hash-object --stdin"? > + done && > + git config gc.auto 1 && test_config? > + LANG=C git gc --auto && > + sleep 1 && # give it time to daemonize > + while test -f .git/gc.pid; do sleep 1; done && Yuck... > + grep "too many unreachable loose objects" .git/gc.log && > + LANG=C git gc --auto 2>error && > + grep skipped error && > + grep "too many unreachable loose objects" error && > + ! test -f .git/gc.log > + ) > +' For that "17/ has very many loose objects that are still young and unreachable" issue, I wonder if the right solution is somehow to flag the repository and prevent "gc --auto" from running until the situation improves. "I checked at this time and found too many in 17/"; upon finding that flag file (with a timestamp), if there are new files in 17/ or if there are other reasons to do a gc (perhaps there are too many packfiles to be consolidated?), then do the gc but otherwise quite silently before spending too many cycles on it, or something along that line? -- To unsubscribe from this list: send the line "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] read-cache: tighten checks for do_read_index
On 03/24, Junio C Hamano wrote: > Thomas Gummerer writes: > > > 03f15a7 read-cache: fix reading of split index moved the checks for the > > correct order of index entries out of do_read_index. This loosens the > > checks more than necessary. Re-introduce the checks for the order, but > > don't error out when we have multiple stage-0 entries in the index. > > Return a flag for the caller instead, if we have multiple stage-0 > > entries and let the caller decide if we need to error out. > > > > Signed-off-by: Thomas Gummerer > > --- > > > > This is a patch on top of my previous patch, as that one has already > > been merged to next. > > I am not convinced that this is a good change in the first place. > > The original before your fix was wrong exactly because it was too > tightly tied to the implementation of the index file format where > there was only one file whose contents must be sorted, and that is > why it was a broken check in a new world with split-index. And your > fix in 'next' is the right fix---it makes the verification happen > only on the result is given to the caller for its consumption. > > It may be true that entries may have to be sorted in a certain order > when reading the original index file format and also reading some > steps in reading the split-index, but that merely happens to be an > imprementation detail of the two format currently supported, and as > we improve these formats (or even introduce yet another one) in the > longer term, this patch would re-introduce the same issue your > earlier fix corrected, wouldn't it? Yes, after looking at it again I completely agree. Sorry for the noise. -- To unsubscribe from this list: send the line "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] read-cache: tighten checks for do_read_index
Thomas Gummerer writes: > 03f15a7 read-cache: fix reading of split index moved the checks for the > correct order of index entries out of do_read_index. This loosens the > checks more than necessary. Re-introduce the checks for the order, but > don't error out when we have multiple stage-0 entries in the index. > Return a flag for the caller instead, if we have multiple stage-0 > entries and let the caller decide if we need to error out. > > Signed-off-by: Thomas Gummerer > --- > > This is a patch on top of my previous patch, as that one has already > been merged to next. I am not convinced that this is a good change in the first place. The original before your fix was wrong exactly because it was too tightly tied to the implementation of the index file format where there was only one file whose contents must be sorted, and that is why it was a broken check in a new world with split-index. And your fix in 'next' is the right fix---it makes the verification happen only on the result is given to the caller for its consumption. It may be true that entries may have to be sorted in a certain order when reading the original index file format and also reading some steps in reading the split-index, but that merely happens to be an imprementation detail of the two format currently supported, and as we improve these formats (or even introduce yet another one) in the longer term, this patch would re-introduce the same issue your earlier fix corrected, wouldn't it? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re* [PATCH 10/15] commit.c: fix a memory leak
On Tue, Mar 24, 2015 at 2:17 PM, Junio C Hamano wrote: > > Move it to dir.c where match_pathspec() is defined. > > Signed-off-by: Junio C Hamano Reviewed-by: Stefan Beller -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re* [PATCH 10/15] commit.c: fix a memory leak
Duy Nguyen writes:
> On Sat, Mar 21, 2015 at 10:59 AM, Junio C Hamano wrote:
>> A further tangent (Duy Cc'ed for this point). We might want to
>> rethink the interface to ce_path_match() and report_path_error()
>> so that we do not have to do a separate allocation of "has this
>> pathspec been used?" array. This was a remnant from the olden days
>> back when pathspec were mere "const char **" where we did not have
>> any room to add per-item bit---these days pathspec is repreasented
>> as an array of "struct pathspec" and we can afford to add a bit
>> to the structure---which will make this kind of leak much less
>> likely to happen.
>
> I just want to say "noted" (and therefore in my backlog). But no
> promise that it will happen any time soon. Low hanging fruit, perhaps
> some people may be interested..
OK, the other one I just did so that we won't forget. Otherwise we
will leave too many loose ends untied.
-- >8 --
From: Junio C Hamano
Date: Tue, 24 Mar 2015 14:12:10 -0700
Subject: [PATCH] report_path_error(): move to dir.c
The expected call sequence is for the caller to use match_pathspec()
repeatedly on a set of pathspecs, accumulating the "hits" in a
separate array, and then call this function to diagnose a pathspec
that never matched anything, as that can indicate a typo from the
command line, e.g. "git commit Maekfile".
Many builtin commands use this function from builtin/ls-files.c,
which is not a very healthy arrangement. ls-files might have been
the first command to feel the need for such a helper, but the need
is shared by everybody who uses the "match and then report" pattern.
Move it to dir.c where match_pathspec() is defined.
Signed-off-by: Junio C Hamano
---
builtin/ls-files.c | 43 ---
cache.h| 1 -
dir.c | 43 +++
dir.h | 1 +
4 files changed, 44 insertions(+), 44 deletions(-)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 47c3880..47d70b2 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -354,49 +354,6 @@ void overlay_tree_on_cache(const char *tree_name, const
char *prefix)
}
}
-int report_path_error(const char *ps_matched,
- const struct pathspec *pathspec,
- const char *prefix)
-{
- /*
-* Make sure all pathspec matched; otherwise it is an error.
-*/
- struct strbuf sb = STRBUF_INIT;
- int num, errors = 0;
- for (num = 0; num < pathspec->nr; num++) {
- int other, found_dup;
-
- if (ps_matched[num])
- continue;
- /*
-* The caller might have fed identical pathspec
-* twice. Do not barf on such a mistake.
-* FIXME: parse_pathspec should have eliminated
-* duplicate pathspec.
-*/
- for (found_dup = other = 0;
-!found_dup && other < pathspec->nr;
-other++) {
- if (other == num || !ps_matched[other])
- continue;
- if (!strcmp(pathspec->items[other].original,
- pathspec->items[num].original))
- /*
-* Ok, we have a match already.
-*/
- found_dup = 1;
- }
- if (found_dup)
- continue;
-
- error("pathspec '%s' did not match any file(s) known to git.",
- pathspec->items[num].original);
- errors++;
- }
- strbuf_release(&sb);
- return errors;
-}
-
static const char * const ls_files_usage[] = {
N_("git ls-files [options] [...]"),
NULL
diff --git a/cache.h b/cache.h
index f23fdbe..8ec0b65 100644
--- a/cache.h
+++ b/cache.h
@@ -1411,7 +1411,6 @@ extern int ws_blank_line(const char *line, int len,
unsigned ws_rule);
#define ws_tab_width(rule) ((rule) & WS_TAB_WIDTH_MASK)
/* ls-files */
-int report_path_error(const char *ps_matched, const struct pathspec *pathspec,
const char *prefix);
void overlay_tree_on_cache(const char *tree_name, const char *prefix);
char *alias_lookup(const char *alias);
diff --git a/dir.c b/dir.c
index 797805d..5d6e102 100644
--- a/dir.c
+++ b/dir.c
@@ -377,6 +377,49 @@ int match_pathspec(const struct pathspec *ps,
return negative ? 0 : positive;
}
+int report_path_error(const char *ps_matched,
+ const struct pathspec *pathspec,
+ const char *prefix)
+{
+ /*
+* Make sure all pathspec matched; otherwise it is an error.
+*/
+ struct strbuf sb = STRBUF_INIT;
+ int num, errors = 0;
+ for (num = 0; num < pathspec->nr; num++) {
+ int other, found_dup;
+
+ if
Re: Git ignore help
Eric Sunshine writes: > On Tue, Mar 24, 2015 at 5:39 AM, Duy Nguyen wrote: >> On Tue, Mar 24, 2015 at 8:55 AM, Eric Sunshine >> wrote: e.g. "db", "reports" or "scripts", we could keep going for a while. I think I attempted to do this in the past and failed (don't remember exactly why). Maybe I'll try again some time in future. >>> >>> I also was pretty sure that you had attempted this, but couldn't find >>> a reference to it, so I didn't mention it in my response. However, >>> with some more digging, I finally located it[1]. >>> >>> [1]: http://article.gmane.org/gmane.comp.version-control.git/259870 >> >> Thank you. I only looked at my repo and no branch name suggested it >> (if only there is google search for a git repository..). So I gave up >> because of performance reasons again but that was for enabling it >> unconditionaly. If we enable it via a config variable and the user is >> made aware of the performance implications, I guess it would be ok. So >> it's back in my back log. > > How much does a config variable actually help? In a sense, one could > argue that this is already an opt-in feature since it requires > crafting gitignore in a particular fashion. Existing projects which > have (properly) functioning gitignore rules won't trigger this > behavior. In many cases, Git already allows people to shoot themselves > in the foot if they desire, thus, as long as the potential performance > impact is properly documented, this could be considered another such > instance. Yeah, as I re-read that old thread, I really do not think anything wrong with the reasoning expressed in the proposed log message. It is pointless to hunt for "!do-not-exclude-me-please" in D/.gitignore when "D/" appears in .gitignore in the higher level, but if these two i.e. D/ !D/do-not-exclude-me-please appear together in .gitignore in the higher level, we can pay attention to that and pick up that single path. And doing so would be a lot more intuitive to the end user. My comment in the thread was only about the documentation being unclear and not about the feature, but somehow we failed to follow-up the topic to completion X-<. -- To unsubscribe from this list: send the line "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/1] l10n: de.po: use "bla …" instead of "bla..."
Thanks a lot for fixing! Phillip > >Let's apply this instead. > >-- >8 -- >From: Phillip Sz >Date: Sat, 21 Mar 2015 13:52:37 +0100 >Subject: [PATCH] l10n: de.po: add space before ellipsis > >Signed-off-by: Phillip Sz >Signed-off-by: Ralf Thielow >--- > po/de.po | 32 > 1 file changed, 16 insertions(+), 16 deletions(-) > >diff --git a/po/de.po b/po/de.po >index 11fbd0f..9fa3f4c 100644 >--- a/po/de.po >+++ b/po/de.po >@@ -616,7 +616,7 @@ msgstr "" > #: help.c:373 > #, c-format > msgid "in %0.1f seconds automatically..." >-msgstr "Automatische Ausführung in %0.1f Sekunden..." >+msgstr "Automatische Ausführung in %0.1f Sekunden ..." > > #: help.c:380 > #, c-format >@@ -1271,12 +1271,12 @@ msgstr "Kann keine Commit-Beschreibung für %s >bekommen" > #: sequencer.c:611 > #, c-format > msgid "could not revert %s... %s" >-msgstr "Konnte \"revert\" nicht auf %s ausführen... %s" >+msgstr "Konnte \"revert\" nicht auf %s ausführen ... %s" > > #: sequencer.c:612 > #, c-format > msgid "could not apply %s... %s" >-msgstr "Konnte %s nicht anwenden... %s" >+msgstr "Konnte %s nicht anwenden ... %s" > > #: sequencer.c:648 > msgid "empty commit set passed" >@@ -2436,7 +2436,7 @@ msgstr "%s: Patch konnte nicht angewendet werden" > #: builtin/apply.c:3653 > #, c-format > msgid "Checking patch %s..." >-msgstr "Prüfe Patch %s..." >+msgstr "Prüfe Patch %s ..." > > #: builtin/apply.c:3746 builtin/checkout.c:231 builtin/reset.c:135 > #, c-format >@@ -4091,7 +4091,7 @@ msgstr "Konnte zu klonenden Remote-Branch %s >nicht finden." > #: builtin/clone.c:561 > #, c-format > msgid "Checking connectivity... " >-msgstr "Prüfe Konnektivität... " >+msgstr "Prüfe Konnektivität ... " > > #: builtin/clone.c:564 > msgid "remote did not send all necessary objects" >@@ -4165,12 +4165,12 @@ msgstr "Konnte Arbeitsverzeichnis '%s' nicht >erstellen." > #: builtin/clone.c:870 > #, c-format > msgid "Cloning into bare repository '%s'...\n" >-msgstr "Klone in Bare-Repository '%s'...\n" >+msgstr "Klone in Bare-Repository '%s' ...\n" > > #: builtin/clone.c:872 > #, c-format > msgid "Cloning into '%s'...\n" >-msgstr "Klone nach '%s'...\n" >+msgstr "Klone nach '%s' ...\n" > > #: builtin/clone.c:897 > msgid "--dissociate given, but there is no --reference" >@@ -4600,7 +4600,7 @@ msgstr "" > #: builtin/commit.c:1199 > msgid "Clever... amending the last one with dirty index." > msgstr "" >-"Klug... den letzten Commit mit einer geänderten Staging-Area >nachbessern." >+"Klug ... den letzten Commit mit einer geänderten Staging-Area >nachbessern." > > #: builtin/commit.c:1201 >msgid "Explicit paths specified without -i or -o; assuming --only >paths..." >@@ -7335,7 +7335,7 @@ msgstr "Aktualisiere %s..%s\n" > #: builtin/merge.c:1388 > #, c-format > msgid "Trying really trivial in-index merge...\n" >-msgstr "Probiere wirklich trivialen \"in-index\"-Merge...\n" >+msgstr "Probiere wirklich trivialen \"in-index\"-Merge ...\n" > > #: builtin/merge.c:1395 > #, c-format >@@ -7349,12 +7349,12 @@ msgstr "Vorspulen nicht möglich, breche ab." > #: builtin/merge.c:1450 builtin/merge.c:1529 > #, c-format > msgid "Rewinding the tree to pristine...\n" >-msgstr "Rücklauf des Verzeichnisses bis zum Ursprung...\n" >+msgstr "Rücklauf des Verzeichnisses bis zum Ursprung ...\n" > > #: builtin/merge.c:1454 > #, c-format > msgid "Trying merge strategy %s...\n" >-msgstr "Probiere Merge-Strategie %s...\n" >+msgstr "Probiere Merge-Strategie %s ...\n" > > #: builtin/merge.c:1520 > #, c-format >@@ -7682,7 +7682,7 @@ msgstr "git notes copy [] >" > > #: builtin/notes.c:51 > msgid "git notes copy --stdin [ ]..." >-msgstr "git notes copy --stdin [ ]..." >+msgstr "git notes copy --stdin [ ] ..." > > #: builtin/notes.c:56 > msgid "git notes append [] []" >@@ -8689,7 +8689,7 @@ msgstr "git remote prune [] " > > #: builtin/remote.c:64 > msgid "git remote update [] [ | ]..." >-msgstr "git remote update [] [ | >]..." >+msgstr "git remote update [] [ | >] ..." > > #: builtin/remote.c:88 > #, c-format >@@ -9865,7 +9865,7 @@ msgstr "fehlerhaftes Objekt bei '%s'" > #: builtin/tag.c:301 > #, c-format > msgid "tag name too long: %.*s..." >-msgstr "Tagname zu lang: %.*s..." >+msgstr "Tagname zu lang: %.*s ..." > > #: builtin/tag.c:306 > #, c-format >@@ -10450,7 +10450,7 @@ msgstr "" > > #: git-am.sh:166 > msgid "Falling back to patching base and 3-way merge..." >-msgstr "Falle zurück zum Patchen der Basis und des 3-Wege-Merges..." >+msgstr "Falle zurück zum Patchen der Basis und des 3-Wege-Merges ..." > > #: git-am.sh:182 > msgid "Failed to merge in the changes." >@@ -10943,7 +10943,7 @@ msgstr "Änderungen von $mb zu $onto:" > msgid "First, rewinding head to replay your work on top of it..." > msgstr "" > "Zunächst wird der Branch zurückgespult, um Ihre Änderungen\n" >-"darauf neu anzuwenden..." >+"darauf neu anzuwenden ..." > > #: git-rebase.sh:620 > #, sh-format -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to
Re: What's cooking in git.git (Mar 2015, #08; Mon, 23)
Junio C Hamano writes: > [Stalled] > > * mh/fdopen-with-retry (2015-03-06) 6 commits > - buffer_fdinit(): use fdopen_with_retry() > - update_info_file(): use fdopen_with_retry() > - copy_to_log(): use fdopen_with_retry() > - fdopen_lock_file(): use fdopen_with_retry() > - SQUASH??? $gmane/264889 > - xfdopen(): if first attempt fails, free memory and try again > > Various parts of the code where they call fdopen() can fail when > they run out of memory; attempt to proceed by retrying the > operation after freeing some resource. > > Waiting for further comments. Sorry, but I lost track. Is this one still viable, or have we decided that it is not worth doing it? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Mar 2015, #08; Mon, 23)
On Tue, Mar 24, 2015 at 01:02:35PM -0700, Junio C Hamano wrote:
> > * jk/test-chain-lint (2015-03-22) 28 commits
> [...]
> > What I queued here has fix to the issue J6t found in 15/25 squashed
> > in, and also has 26/25 and 27/25 follow-up fixes from Michael, plus
> > 28/25 follow-up from Torsten. If everybody involved is happy with
> > it, we can just proceed with this copy, otherwise I'll let Peff
> > reroll. I am happy either way.
>
> I'll merge this to 'next' soonish, unless I hear otherwise. I
> double checked 15/25 (i.e. $feature{"forks"}{"default"} = [1];)
> so I think we are in good shape.
Thanks, yeah, I think what you have queued is good.
-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: What's cooking in git.git (Mar 2015, #08; Mon, 23)
Junio C Hamano writes:
> * jk/test-chain-lint (2015-03-22) 28 commits
> - t6039: fix broken && chain
> - t9158, t9161: fix broken &&-chain in git-svn tests
> - t9104: fix test for following larger parents
> - t4104: drop hand-rolled error reporting
> - t0005: fix broken &&-chains
> - t7004: fix embedded single-quotes
> - t0050: appease --chain-lint
> - t9001: use test_when_finished
> - t4117: use modern test_* helpers
> - t6034: use modern test_* helpers
> - t1301: use modern test_* helpers
> - t0020: use modern test_* helpers
> - t6030: use modern test_* helpers
> - t9502: fix &&-chain breakage
> - t7201: fix &&-chain breakage
> - t3600: fix &&-chain breakage for setup commands
> - t: avoid using ":" for comments
> - t: wrap complicated expect_code users in a block
> - t: use test_expect_code instead of hand-rolled comparison
> - t: use test_might_fail for diff and grep
> - t: fix &&-chaining issues around setup which might fail
> - t: use test_must_fail instead of hand-rolled blocks
> - t: use verbose instead of hand-rolled errors
> - t: assume test_cmp produces verbose output
> - t: fix trivial &&-chain breakage
> - t: fix moderate &&-chain breakage
> - t: fix severe &&-chain breakage
> - t/test-lib: introduce --chain-lint option
>
> People often forget to chain the commands in their test together
> with &&, leaving a failure from an earlier command in the test go
> unnoticed. The new GIT_TEST_CHAIN_LINT mechanism allows you to
> catch such a mistake more easily.
>
> What I queued here has fix to the issue J6t found in 15/25 squashed
> in, and also has 26/25 and 27/25 follow-up fixes from Michael, plus
> 28/25 follow-up from Torsten. If everybody involved is happy with
> it, we can just proceed with this copy, otherwise I'll let Peff
> reroll. I am happy either way.
I'll merge this to 'next' soonish, unless I hear otherwise. I
double checked 15/25 (i.e. $feature{"forks"}{"default"} = [1];)
so I think we are in good shape.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] t1501: fix test with split index
t1501-worktree.sh does not copy the shared index in the "relative $GIT_WORK_TREE and git subprocesses" test, which makes the test fail when GIT_TEST_SPLIT_INDEX is set. Copy the shared index as well in order to fix this. Helped-by: Junio C Hamano Signed-off-by: Thomas Gummerer --- > Is this a good place to use "test-might-fail", e.g. > >test_might_fail cp repo.git/sharedindex.* repo.git/repos/foo && > > or something? Yeah that makes sense, thanks. t/t1501-worktree.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh index 4df7a2f..cc5b870 100755 --- a/t/t1501-worktree.sh +++ b/t/t1501-worktree.sh @@ -350,6 +350,7 @@ test_expect_success 'Multi-worktree setup' ' mkdir work && mkdir -p repo.git/repos/foo && cp repo.git/HEAD repo.git/index repo.git/repos/foo && + test_might_fail cp repo.git/sharedindex.* repo.git/repos/foo && sane_unset GIT_DIR GIT_CONFIG GIT_WORK_TREE ' -- 2.1.0.264.g0463184.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] l10n: de.po: add space before ellipsis
From: Phillip Sz Signed-off-by: Phillip Sz Signed-off-by: Ralf Thielow --- po/de.po | 22 +++--- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/po/de.po b/po/de.po index 11fbd0f..7b30f62 100644 --- a/po/de.po +++ b/po/de.po @@ -616,7 +616,7 @@ msgstr "" #: help.c:373 #, c-format msgid "in %0.1f seconds automatically..." -msgstr "Automatische Ausführung in %0.1f Sekunden..." +msgstr "Automatische Ausführung in %0.1f Sekunden ..." #: help.c:380 #, c-format @@ -2436,7 +2436,7 @@ msgstr "%s: Patch konnte nicht angewendet werden" #: builtin/apply.c:3653 #, c-format msgid "Checking patch %s..." -msgstr "Prüfe Patch %s..." +msgstr "Prüfe Patch %s ..." #: builtin/apply.c:3746 builtin/checkout.c:231 builtin/reset.c:135 #, c-format @@ -4091,7 +4091,7 @@ msgstr "Konnte zu klonenden Remote-Branch %s nicht finden." #: builtin/clone.c:561 #, c-format msgid "Checking connectivity... " -msgstr "Prüfe Konnektivität... " +msgstr "Prüfe Konnektivität ... " #: builtin/clone.c:564 msgid "remote did not send all necessary objects" @@ -4165,12 +4165,12 @@ msgstr "Konnte Arbeitsverzeichnis '%s' nicht erstellen." #: builtin/clone.c:870 #, c-format msgid "Cloning into bare repository '%s'...\n" -msgstr "Klone in Bare-Repository '%s'...\n" +msgstr "Klone in Bare-Repository '%s' ...\n" #: builtin/clone.c:872 #, c-format msgid "Cloning into '%s'...\n" -msgstr "Klone nach '%s'...\n" +msgstr "Klone nach '%s' ...\n" #: builtin/clone.c:897 msgid "--dissociate given, but there is no --reference" @@ -4600,7 +4600,7 @@ msgstr "" #: builtin/commit.c:1199 msgid "Clever... amending the last one with dirty index." msgstr "" -"Klug... den letzten Commit mit einer geänderten Staging-Area nachbessern." +"Klug ... den letzten Commit mit einer geänderten Staging-Area nachbessern." #: builtin/commit.c:1201 msgid "Explicit paths specified without -i or -o; assuming --only paths..." @@ -7335,7 +7335,7 @@ msgstr "Aktualisiere %s..%s\n" #: builtin/merge.c:1388 #, c-format msgid "Trying really trivial in-index merge...\n" -msgstr "Probiere wirklich trivialen \"in-index\"-Merge...\n" +msgstr "Probiere wirklich trivialen \"in-index\"-Merge ...\n" #: builtin/merge.c:1395 #, c-format @@ -7349,12 +7349,12 @@ msgstr "Vorspulen nicht möglich, breche ab." #: builtin/merge.c:1450 builtin/merge.c:1529 #, c-format msgid "Rewinding the tree to pristine...\n" -msgstr "Rücklauf des Verzeichnisses bis zum Ursprung...\n" +msgstr "Rücklauf des Verzeichnisses bis zum Ursprung ...\n" #: builtin/merge.c:1454 #, c-format msgid "Trying merge strategy %s...\n" -msgstr "Probiere Merge-Strategie %s...\n" +msgstr "Probiere Merge-Strategie %s ...\n" #: builtin/merge.c:1520 #, c-format @@ -10450,7 +10450,7 @@ msgstr "" #: git-am.sh:166 msgid "Falling back to patching base and 3-way merge..." -msgstr "Falle zurück zum Patchen der Basis und des 3-Wege-Merges..." +msgstr "Falle zurück zum Patchen der Basis und des 3-Wege-Merges ..." #: git-am.sh:182 msgid "Failed to merge in the changes." @@ -10943,7 +10943,7 @@ msgstr "Änderungen von $mb zu $onto:" msgid "First, rewinding head to replay your work on top of it..." msgstr "" "Zunächst wird der Branch zurückgespult, um Ihre Änderungen\n" -"darauf neu anzuwenden..." +"darauf neu anzuwenden ..." #: git-rebase.sh:620 #, sh-format -- 2.3.3.434.g642b19b -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] l10n: de.po: fix messages with abbreviated hashs
The three dots in messages where the hash is abbreviated were misinterpreted and are fixed with this commit. Noticed-by: Junio C Hamano Signed-off-by: Ralf Thielow --- po/de.po | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/po/de.po b/po/de.po index 7b30f62..f818350 100644 --- a/po/de.po +++ b/po/de.po @@ -1271,12 +1271,12 @@ msgstr "Kann keine Commit-Beschreibung für %s bekommen" #: sequencer.c:611 #, c-format msgid "could not revert %s... %s" -msgstr "Konnte \"revert\" nicht auf %s ausführen... %s" +msgstr "Konnte \"revert\" nicht auf %s...(%s) ausführen" #: sequencer.c:612 #, c-format msgid "could not apply %s... %s" -msgstr "Konnte %s nicht anwenden... %s" +msgstr "Konnte %s...(%s) nicht anwenden" #: sequencer.c:648 msgid "empty commit set passed" -- 2.3.3.434.g642b19b -- To unsubscribe from this list: send the line "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/GSoC] Proposal: Make git-pull and git-am builtins
Paul Tan writes: > ..., I propose the following requirements for the rewritten code: > > 1. No spawning of external git processes. This is to support systems with high >``fork()`` or process creation overhead, and to reduce redundant IO by >taking advantage of the internal object, index and configuration cache. I suspect this may probably be too strict in practice. True, we should never say "run_command_capture()" just to to read from "git rev-parse"---we should just call get_sha1() instead. But for a complex command whose execution itself far outweighs the cost of forking, I do not think it is fair to say your project failed if you chose to run_command() it. For example, it may be perfectly OK to invoke "git merge" via run_command(). > 3. The resulting builtin should not have wildly different behavior or bugs >compared to the shell script. This on the other hand is way too loose. The original and the port must behave identically, unless the difference is fixing bugs in the original. > Potential difficulties > === > > Rewriting code may introduce bugs > ... Yes, but that is a reasonable risk you need to manage to gain the benefit from this project. > Of course, the downside of following this too strictly is that if there were > any logical bugs in the original code, or if the original code is unclear, the > rewritten code would inherit these problems too. I'd repeat my comment on the 3. above. Identifying and fixing bugs is great, but otherwise don't worry about this too much. Being bug-to-bug compatible with the original is way better than introducing new bugs of an unknown nature. > Rewritten code may become harder to understand > ... And also it may become harder to modify. That is the largest problem with any rewrite, and we should spend the most effort to avoid it. A new bugs introduced we can later fix as long as the result is understandable and maintainable. > For the purpose of reducing git's dependencies, the rewritten C code should > not > depend on other libraries or executables other than what is already available > to git builtins. Perhaps misphrased; see below. > We can see that the C version requires much more lines compared to the shell > pipeline,... That is something you would solve by introducing reusable code in run_command API, isn't it? That is how various rewrites in the past did, and this project should do so too. You should aim to do this project by not just using "what is already available", but adding what you discover is a useful reusable pattern into a set of new functions in the "already available" API set. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH, RFC] checkout: Attempt to checkout submodules
On Mon, Mar 23, 2015 at 09:01:48PM +0100, Jens Lehmann wrote:
> Am 20.03.2015 um 01:13 schrieb Trevor Saunders:
> >On Thu, Mar 19, 2015 at 02:15:19PM -0700, Junio C Hamano wrote:
> >>Trevor Saunders writes:
> >>I have a feeling that an optional feature that allows "git submodule
> >>update" to happen automatically from this codepath might be
> >>acceptable by the submodule folks, and they might even say it does
> >>not even have to be optional but should be enabled by default.
> >
> >ok, that seems fairly reasonable. I do kind of wonder though if it
> >shouldn't be 'git submodule update --checkout' but that would get us
> >kind of back to where we started. I guess since the default is checkout
> >if you set the pref then you can be assumed to have some amount of idea
> >what your doing.
>
> Me thinks it should be "git checkout" for those submodules that have
> their update setting set to 'checkout' (or not set at all). I'm not
> sure yet if it makes sense to attempt a rebase or merge here, but that
> can be added later if necessary.
sgtm
> >>But I do not think it would fly well to unconditionally run
> >>"checkout -f" here.
> >
> >agreed
>
> Using -f here is ok when you extend the appropriate verify functions
> in unpack-trees.c to check that no modifications will be lost (unless
> the original checkout is used with -f). See the commit 76dbdd62
> ("submodule: teach unpack_trees() to update submodules") in my github
> repo at https://github.com/jlehmann/git-submod-enhancements for
> the basic concept (There is already a fixup! for that a bit further
> down the branch which handles submodule to file conversion, maybe one
> or two other changes will be needed when the test suite covers all
> relevant cases).
ah, I see your already working a more complete solution to this sort of
issue. I'll get out of your way then unless you want help.
Trev
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing
Michael Haggerty writes: > Regarding specifically allowing/disallowing a leading '+': I saw a > couple of callsites that explicitly check that the first character is a > digit before calling strtol(). I assumed that is to disallow sign > characters [1]. For example, > > diff.c: optarg() This one I know is from "if not a digit we know it is not a number"; it is not an attempt to say "we must forbid numbers to be spelled with '+'", but more about "we do not need it and this is easier to code without a full fledged str-to-num helper API" sloppiness. > builtin/apply.c: parse_num() This parses "@@ -l,k +m,n @@@" after stripping the punctuation around the numbers, so this is a valid reason why you would want an optional feature "CANNOT_HAVE_SIGN" in the str-to-num parser and use it. > maybe date.c: match_multi_number() The approxidate callers parse random garbage input and attempt to make best sense out of it, so they tend to try limitting the damage caused by incorrect guesses by insisting "we do not consider +0 to be zero" and such. So this is a valid reason why you would want an optional feature "CANNOT_HAVE_SIGN" in the str-to-num parser and use it. > I'm coming around to an alternate plan: > > Step 1: write a NUM_DEFAULT combination-of-options that gives the new > functions behavior very like strtol()/strtoul() but without their insane > features. > > Step 2: rewrite all callers to use that option (and usually endptr=NULL, > meaning no trailing characters) unless it is manifestly clear that they > are already trying to forbid some other features. This will already > produce the largest benefit: avoiding overflows, missing error checking, > etc. > > Steps 3 through ∞: as time allows, rewrite individual callsites to be > stricter where appropriate. > > Hopefully steps 1 and 2 will not be too controversial. All sounds sensible to me. -- To unsubscribe from this list: send the line "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 0/3] protocol v2
Stefan Beller writes: > So I started looking into extending the buffer size as another 'first step' > towards the protocol version 2 again. But now I think the packed length > limit of 64k is actually a good and useful thing to have and should be > extended/fixed if and only if we run into serious trouble with too small > packets later. I tend to agree. Too large a packet size would mean your latency would also suck, as pkt-line interface will not give you anything until you read the entire packet. The new protocol should be designed around a reasonably sized packets, using multiple packets to carry larger payload as necessary. -- To unsubscribe from this list: send the line "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] t1501: fix test with split index
Thomas Gummerer writes: > t1501-worktree.sh does not copy the shared index in the "relative > $GIT_WORK_TREE and git subprocesses" test, which makes the test fail > when GIT_TEST_SPLIT_INDEX is set. Copy the shared index as well in > order to fix this. > > Signed-off-by: Thomas Gummerer > --- > > This applies on top of nd/multiple-work-trees. Sorry for not catching it > earlier, but I haven't tried to run the test-suite for the next branch > then, where this appears. > > t/t1501-worktree.sh | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh > index 4df7a2f..ce5c654 100755 > --- a/t/t1501-worktree.sh > +++ b/t/t1501-worktree.sh > @@ -350,6 +350,7 @@ test_expect_success 'Multi-worktree setup' ' > mkdir work && > mkdir -p repo.git/repos/foo && > cp repo.git/HEAD repo.git/index repo.git/repos/foo && > + ( cp repo.git/sharedindex.* repo.git/repos/foo 2>/dev/null || : ) && Is this a good place to use "test-might-fail", e.g. test_might_fail cp repo.git/sharedindex.* repo.git/repos/foo && or something? > sane_unset GIT_DIR GIT_CONFIG GIT_WORK_TREE > ' -- To unsubscribe from this list: send the line "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] read-cache: tighten checks for do_read_index
03f15a7 read-cache: fix reading of split index moved the checks for the
correct order of index entries out of do_read_index. This loosens the
checks more than necessary. Re-introduce the checks for the order, but
don't error out when we have multiple stage-0 entries in the index.
Return a flag for the caller instead, if we have multiple stage-0
entries and let the caller decide if we need to error out.
Signed-off-by: Thomas Gummerer
---
This is a patch on top of my previous patch, as that one has already
been merged to next.
cache.h | 2 +-
read-cache.c| 54 -
test-dump-split-index.c | 2 +-
3 files changed, 37 insertions(+), 21 deletions(-)
diff --git a/cache.h b/cache.h
index e7b24a2..3eaa258 100644
--- a/cache.h
+++ b/cache.h
@@ -487,7 +487,7 @@ struct lock_file;
extern int read_index(struct index_state *);
extern int read_index_preload(struct index_state *, const struct pathspec
*pathspec);
extern int do_read_index(struct index_state *istate, const char *path,
-int must_exist); /* for testting only! */
+int must_exist, int *multiple_stage_entries); /* for
testting only! */
extern int read_index_from(struct index_state *, const char *path);
extern int is_index_unborn(struct index_state *);
extern int read_index_unmerged(struct index_state *);
diff --git a/read-cache.c b/read-cache.c
index 36ff89f..2ba67ce 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1488,30 +1488,39 @@ static struct cache_entry *create_from_disk(struct
ondisk_cache_entry *ondisk,
return ce;
}
-static void check_ce_order(struct index_state *istate)
+static int check_ce_order(struct cache_entry *ce, struct cache_entry *next_ce,
+ int gentle_multiple_stage)
{
- unsigned int i;
-
- for (i = 1; i < istate->cache_nr; i++) {
- struct cache_entry *ce = istate->cache[i - 1];
- struct cache_entry *next_ce = istate->cache[i];
- int name_compare = strcmp(ce->name, next_ce->name);
+ int name_compare = strcmp(ce->name, next_ce->name);
- if (0 < name_compare)
- die("unordered stage entries in index");
- if (!name_compare) {
- if (!ce_stage(ce))
+ if (0 < name_compare)
+ die("unordered stage entries in index");
+ if (!name_compare) {
+ if (!ce_stage(ce)) {
+ if (gentle_multiple_stage)
+ return 1;
+ else
die("multiple stage entries for merged file
'%s'",
ce->name);
- if (ce_stage(ce) > ce_stage(next_ce))
- die("unordered stage entries for '%s'",
- ce->name);
}
+ if (ce_stage(ce) > ce_stage(next_ce))
+ die("unordered stage entries for '%s'",
+ ce->name);
}
+ return 0;
+}
+
+static void check_istate_order(struct index_state *istate)
+{
+ unsigned int i;
+
+ for (i = 1; i < istate->cache_nr; i++)
+ check_ce_order(istate->cache[i - 1], istate->cache[i], 0);
}
/* remember to discard_cache() before reading a different cache! */
-int do_read_index(struct index_state *istate, const char *path, int must_exist)
+int do_read_index(struct index_state *istate, const char *path, int must_exist,
+ int *multiple_stage_entries)
{
int fd, i;
struct stat st;
@@ -1571,6 +1580,11 @@ int do_read_index(struct index_state *istate, const char
*path, int must_exist)
ce = create_from_disk(disk_ce, &consumed, previous_name);
set_index_entry(istate, i, ce);
+ if (i > 0)
+ if (check_ce_order(istate->cache[i - 1], ce, 1) > 0 &&
+ multiple_stage_entries)
+ *multiple_stage_entries |= 1;
+
src_offset += consumed;
}
strbuf_release(&previous_name_buf);
@@ -1607,15 +1621,17 @@ int read_index_from(struct index_state *istate, const
char *path)
{
struct split_index *split_index;
int ret;
+ int multiple_stage_entries = 0;
/* istate->initialized covers both .git/index and .git/sharedindex.xxx
*/
if (istate->initialized)
return istate->cache_nr;
- ret = do_read_index(istate, path, 0);
+ ret = do_read_index(istate, path, 0, &multiple_stage_entries);
split_index = istate->split_index;
if (!split_index || is_null_sha1(split_index->base_sha1)) {
- check_ce_order(istate);
+ if (multiple_stage_entries)
+ check_istate_order(istate);
return ret;
}
@@ -1625,7
Re: [PATCH 1/1] l10n: de.po: use "bla …" instead of "bla..."
2015-03-24 18:32 GMT+01:00 Junio C Hamano : > Ralf Thielow writes: > >> diff --git a/po/de.po b/po/de.po >> index 11fbd0f..9fa3f4c 100644 >> --- a/po/de.po >> +++ b/po/de.po >> @@ -616,7 +616,7 @@ msgstr "" >> #: help.c:373 >> #, c-format >> msgid "in %0.1f seconds automatically..." >> -msgstr "Automatische Ausführung in %0.1f Sekunden..." >> +msgstr "Automatische Ausführung in %0.1f Sekunden ..." >> >> #: help.c:380 >> #, c-format >> @@ -1271,12 +1271,12 @@ msgstr "Kann keine Commit-Beschreibung für %s >> bekommen" >> #: sequencer.c:611 >> #, c-format >> msgid "could not revert %s... %s" >> -msgstr "Konnte \"revert\" nicht auf %s ausführen... %s" >> +msgstr "Konnte \"revert\" nicht auf %s ausführen ... %s" > > I do not read German, but aren't these two completely in different > classes? The first one is not abbreviating any part of a word, but > the second one's first "..." is showing that %s is not giving the > full word (it is fed the find-unique-abbrev result and adding ... > to say it is not a full 40-hex). > > I do not read German, but I would not be surprised if the original > were a mistranslation. Is it saying > > Could not revert THAT ONE ... THE SUBJECT OF THE COMMIT > > as if "..." is some punctuation in the sentence (i.e. it could have > been a ';' or ':' or '.' that ends the first part of the sentence, > but "..." is used to tell the reader to "wait a bit before > continuing with the rest of the sentence"), not as part of "THAT > ONE"? The "..." in the original is a three-dot that means "we say > THAT ONE but it is not fully spelled out, there are more letters > here". > > Please ignore the above if the convention in German is to have SP > before three-dots for both cases. I do not read German. > > Still the placement of a word "perform" (ausführen) between "%s" and > "..." in the translation of the second one looks suspicious to me, > though. > >> @@ -9865,7 +9865,7 @@ msgstr "fehlerhaftes Objekt bei '%s'" >> #: builtin/tag.c:301 >> #, c-format >> msgid "tag name too long: %.*s..." >> -msgstr "Tagname zu lang: %.*s..." >> +msgstr "Tagname zu lang: %.*s ..." > > This is also "We are not spelling it fully and there are actually > some more letters in the original" three-dots. You're right. I just misinterpreted the dots. It seems I'm chatting too much where "..." means nothing but space. ;-) I'll fix it in this patch and will check other messages later. 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 09/15] http: release the memory of a http pack request as well
Stefan Beller writes:
> Well there is hope, as `release_request` only touches
> free(request->url);
> free(request);
>
> and not the userData pointer.
OK.
> I am a bit puzzled what you're trying to hint at.
The caller does this:
static void start_fetch_packed(struct transfer_request *request)
{
...
preq = new_http_pack_request(target, repo->url);
...
preq->slot->callback_func = process_response;
preq->slot->callback_data = request;
request->slot = preq->slot;
request->userData = preq;
/* Try to get the request started, abort the request on error */
request->state = RUN_FETCH_PACKED;
if (!start_active_slot(preq->slot)) {
fprintf(stderr, "Unable to start GET request\n");
release_http_pack_request(preq);
repo->can_update_info_refs = 0;
release_request(request);
}
}
and start_active_slot() actually not just "starts" but calls
curl_multi_perform() to do things, like calling process_response(),
which has calls to release_{,http_pack_}request(). I didn't see
those releases and the releases we see in the above (i.e. when !start)
will not run at the same time (but I see it now ;-))
In short, not hinting at anything. I was genuinely having a hard time
following the codeflow.
--
To unsubscribe from this list: send the line "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] diff-lib.c: adjust position of i-t-a entries in diff
Duy Nguyen writes: > "read-tree -m" does not invoke diff, does it? If I went with my > previous approach (modifying unpack-trees to ignore i-t-a entries) > then this could be a problem, but because unpack-trees is untouched, > merge operations should not be impacted by this patch. Theoretically yes, but not quite. I wouldn't be surprised if an enterprising soul saw an optimization opportunity in the "read-tree -m A B" codepath. When it finds that a tree in A and a valid cache-tree entry that corresponds to the tree matches, it could blow away all index entries covered by the cache-tree entry and replace them with B, either (1) unconditionally when "-u" is not given; or (2) as long as the working tree matches the index inside that directory when running with "-u". And such an optimization used to be a valid thing to do in the old world; but (1) will break in the new world, if we drop that invalidation---the i-t-a entries will be discarded from the index. As i-t-a is not a norm but an abberration, I'd rather keep the pessimizing invalidation to keep the door open for such an optimization for a more common case, and there may be other cases in which our correctness around i-t-a depends on. -- To unsubscribe from this list: send the line "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 0/3] protocol v2
On Tue, Mar 3, 2015 at 9:13 AM, Junio C Hamano wrote: > Duy Nguyen writes: > >> Junio pointed out in private that I didn't address the packet length >> limit (64k). I thought I could get away with a new capability >> (i.e. not worry about it now) but I finally admit that was a bad >> hack. So perhaps this on top. > > No, I didn't ;-) but I tend to agree that "perhaps 4GB huge packet?" > is a bad idea. > > The problem I had with the version in your write-up was that it > still assumed that all capabilities must come on one packet-line. > So I started looking into extending the buffer size as another 'first step' towards the protocol version 2 again. But now I think the packed length limit of 64k is actually a good and useful thing to have and should be extended/fixed if and only if we run into serious trouble with too small packets later. I mean we can add the possibility now by introducing these special length 0x or 0xFFFE to mean we'd want to extend it in the future. But when doing this we need to be extra careful with buffer allocation. As it is easy to produce a denial of service attack if the receiving side blindly trusts the length and allocates as much memory. So having a 64k limit actually helps preventing this attack a bit as it is a very small number. -- To unsubscribe from this list: send the line "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: Git ignore help
On Tue, Mar 24, 2015 at 5:39 AM, Duy Nguyen wrote: > On Tue, Mar 24, 2015 at 8:55 AM, Eric Sunshine > wrote: >>> e.g. "db", "reports" or "scripts", we could keep going for a while. I >>> think I attempted to do this in the past and failed (don't remember >>> exactly why). Maybe I'll try again some time in future. >> >> I also was pretty sure that you had attempted this, but couldn't find >> a reference to it, so I didn't mention it in my response. However, >> with some more digging, I finally located it[1]. >> >> [1]: http://article.gmane.org/gmane.comp.version-control.git/259870 > > Thank you. I only looked at my repo and no branch name suggested it > (if only there is google search for a git repository..). So I gave up > because of performance reasons again but that was for enabling it > unconditionaly. If we enable it via a config variable and the user is > made aware of the performance implications, I guess it would be ok. So > it's back in my back log. How much does a config variable actually help? In a sense, one could argue that this is already an opt-in feature since it requires crafting gitignore in a particular fashion. Existing projects which have (properly) functioning gitignore rules won't trigger this behavior. In many cases, Git already allows people to shoot themselves in the foot if they desire, thus, as long as the potential performance impact is properly documented, this could be considered another such instance. -- To unsubscribe from this list: send the line "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] t1501: fix test with split index
t1501-worktree.sh does not copy the shared index in the "relative $GIT_WORK_TREE and git subprocesses" test, which makes the test fail when GIT_TEST_SPLIT_INDEX is set. Copy the shared index as well in order to fix this. Signed-off-by: Thomas Gummerer --- This applies on top of nd/multiple-work-trees. Sorry for not catching it earlier, but I haven't tried to run the test-suite for the next branch then, where this appears. t/t1501-worktree.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh index 4df7a2f..ce5c654 100755 --- a/t/t1501-worktree.sh +++ b/t/t1501-worktree.sh @@ -350,6 +350,7 @@ test_expect_success 'Multi-worktree setup' ' mkdir work && mkdir -p repo.git/repos/foo && cp repo.git/HEAD repo.git/index repo.git/repos/foo && + ( cp repo.git/sharedindex.* repo.git/repos/foo 2>/dev/null || : ) && sane_unset GIT_DIR GIT_CONFIG GIT_WORK_TREE ' -- 2.1.0.264.g0463184.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing
On 03/24/2015 04:58 PM, Junio C Hamano wrote: > Michael Haggerty writes: > >> It is easy to allow "--abbrev=+7"; I would just need to add NUM_PLUS to >> those call sites. Should I do so? > > The more relevant question to ask from my point of view is why you > need to "add" NUM_PLUS to "enable" it. What valid reason do you > have to forbid it anywhere? Only because you do not accept it by > default, you need to "add" to "enable". I want to be able to plunge into this project without first auditing all call sites to see which features will turn out to be needed. So I'm erring on the side of flexibility. For now, I want to be able to prohibit '+' signs. Currently all of the flags cause additional features to be enabled. My guess was that most callers *won't* need most features, so it seemed easiest and most consistent to have all features be turned off by default and let the caller add the features that it wants to allow. Regarding specifically allowing/disallowing a leading '+': I saw a couple of callsites that explicitly check that the first character is a digit before calling strtol(). I assumed that is to disallow sign characters [1]. For example, diff.c: optarg() builtin/apply.c: parse_num() maybe date.c: match_multi_number() There are other callsites that call strtoul(), but store the result in a signed variable. Those would presumably not want to allow leading '-', but I'm not sure. I also imagine that there are places in protocols or file formats where signs should not be allowed (e.g., timestamps in commits?). >>> Why is it a problem to allow "git cmd --hexval=0x1234", even if "git >>> cmd --hexval=1234" would suffice? >> >> In some cases we would like to allow that flexibility; in some cases >> not. But the strtol()/strtoul() functions *always* allow it. > > The same issue. Whare are these "some cases"? I admit I'm not sure there are such places for hexadecimal numbers. I'm coming around to an alternate plan: Step 1: write a NUM_DEFAULT combination-of-options that gives the new functions behavior very like strtol()/strtoul() but without their insane features. Step 2: rewrite all callers to use that option (and usually endptr=NULL, meaning no trailing characters) unless it is manifestly clear that they are already trying to forbid some other features. This will already produce the largest benefit: avoiding overflows, missing error checking, etc. Steps 3 through ∞: as time allows, rewrite individual callsites to be stricter where appropriate. Hopefully steps 1 and 2 will not be too controversial. Michael [1] That assumption is based on a rather quick look over the code, because with well over 100 callsites, it is not practical to study each callsite carefully. -- Michael Haggerty mhag...@alum.mit.edu -- To unsubscribe from this list: send the line "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/1] l10n: de.po: use "bla …" instead of "bla..."
2015-03-24 18:10 GMT+01:00 Ralf Thielow : > Let's apply this instead. > > -- >8 -- > #: builtin/notes.c:51 > msgid "git notes copy --stdin [ ]..." > -msgstr "git notes copy --stdin [ ]..." > +msgstr "git notes copy --stdin [ ] ..." > > #: builtin/remote.c:64 > msgid "git remote update [] [ | ]..." > -msgstr "git remote update [] [ | ]..." > +msgstr "git remote update [] [ | ] ..." > Oops. I'll remove the space in these two messages as the dots aren't ellipsis obviously. -- To unsubscribe from this list: send the line "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/GSoC] Proposal: Make git-pull and git-am builtins
Paul Tan writes: > On Tue, Mar 24, 2015 at 6:19 PM, Matthieu Moy > wrote: > >> About the timeline: I'd avoid too much parallelism. Usually, it's best >> to try to send a first patch to the mailing list as soon as possible, >> hence focus on one point first (I'd do that with pull, since that's the >> one which is already started). Then, you can parallelize coding on git >> am and the discussion on the pull patches. Whatever you plan, review and >> polishing takes more than that ;-). The risk is to end up with an almost >> good but not good enough to be mergeable code. That said, your timeline >> does plan patches and review early, so I'm not too worried. >> > > Well, I was thinking that after the full rewrite (2nd stage, halfway > through the project), any optimizations made to the code will be done > iteratively (and in separate small patches) Yes, that's why I'm not too worried. But being able to say "this part is done, it won't disturb me anymore" ASAP is still good IMHO, even if "this part" is not so big. But again, I'm thinking out loudly, feel free to ignore. >> A general advice: if time allows, try to contribute to discussions and >> review other than your own patches. It's nice to feel integrated in the >> community and not "the GSoC student working alone at home" ;-). > > Yeah I apologize for not participating in the list so actively because > writing the git-pull prototype and the proposal took a fair chunk of > my time. Don't apologize, you're doing great. I'm only pointing out things that could be "even better", but certainly not blaming you! -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "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/GSoC] Proposal: Make git-pull and git-am builtins
On Tue, Mar 24, 2015 at 6:19 PM, Matthieu Moy wrote: > A few minor details: > > "on operating systems with poor file system performance (i.e. Windows)" > => that's not only windows, I also commonly use a slow filesystem on > Linux, just because it's NFS. Mentionning other cases of poor filesystem > performance would show that the benefit is not limited to windows users, > and would give less of a taste of "windows-bashing". Ah right, I didn't think of network file systems. Thanks for the suggestion. > About the timeline: I'd avoid too much parallelism. Usually, it's best > to try to send a first patch to the mailing list as soon as possible, > hence focus on one point first (I'd do that with pull, since that's the > one which is already started). Then, you can parallelize coding on git > am and the discussion on the pull patches. Whatever you plan, review and > polishing takes more than that ;-). The risk is to end up with an almost > good but not good enough to be mergeable code. That said, your timeline > does plan patches and review early, so I'm not too worried. > Well, I was thinking that after the full rewrite (2nd stage, halfway through the project), any optimizations made to the code will be done iteratively (and in separate small patches) so as to keep the patch series in an "always almost mergeable" state. This will hopefully make it much easier and shorter to do any final polishing and review for merging. > A general advice: if time allows, try to contribute to discussions and > review other than your own patches. It's nice to feel integrated in the > community and not "the GSoC student working alone at home" ;-). Yeah I apologize for not participating in the list so actively because writing the git-pull prototype and the proposal took a fair chunk of my time. Also, my expertise with the code base is not that great yet so it takes quite a bit more effort for me to contribute constructively, but I expect that will improve in the future. Now that the proposal is more or less complete I can spend more time on discussions. Thanks, Paul -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] read-cache: fix reading of split index
Duy Nguyen writes: > Thank you for catching this. I was about to write "would be nice to > point out what tests fail so the reviewer has easier time trying > themselves", but whoa.. quite a few of them! > > May I suggest a slight modification. Even though stage info is messed > up before the index is merged, I think we should still check that both > front and base indexes have all the names sorted correctly (and even > stronger, the base index should never contain staged entries). I smell a slight layering violation here, though. As far as the code to check the validity of the index is concerned, it is only about the in-core index other code uses at runtime, and how that in-core index is prepared, and most importantly, what is recorded in the istate before it gets ready to be used by other code, is not its concern. The state immediately after the base index is read but before it gets fixed up by the split-index code can have quirks specific to how split-index code does things and it is perfectly OK if it does not pass the check for the final shape. The above does not change if the current split-index code happens to promise certain properties in that intermediate state. It is fine if the split-index codepath wants to add its own validator to the intermediate state for added robustness, but the rules for the intermediate state and the rules for the final state can be different, and from the maintainability's point of view, it is better if we keep the validator for the final-shape oblivious to what split-index does. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] read-cache: fix reading of split index
On 03/24, Duy Nguyen wrote:
> On Sat, Mar 21, 2015 at 4:43 AM, Thomas Gummerer wrote:
> > The split index extension uses ewah bitmaps to mark index entries as
> > deleted, instead of removing them from the index directly. This can
> > result in an on-disk index, in which entries of stage #0 and higher
> > stages appear, which are removed later when the index bases are merged.
> >
> > 15999d0 read_index_from(): catch out of order entries when reading an
> > index file introduces a check which checks if the entries are in order
> > after each index entry is read in do_read_index. This check may however
> > fail when a split index is read.
> >
> > Fix this by moving checking the index after we know there is no split
> > index or after the split index bases are successfully merged instead.
>
> Thank you for catching this. I was about to write "would be nice to
> point out what tests fail so the reviewer has easier time trying
> themselves", but whoa.. quite a few of them!
>
> May I suggest a slight modification. Even though stage info is messed
> up before the index is merged, I think we should still check that both
> front and base indexes have all the names sorted correctly (and even
> stronger, the base index should never contain staged entries). If
Hmm I just tried adding another check for that, but the base index
does seem to include staged entries sometimes.
I've tried with this, but there are quite a few test failures. For
example in t3600-rm.sh test #52 fails, and test-dump-split-index shows
the submodule with stages 1, 2 and 3 in the index.
own 74cd8e14a8fcc5df52e5c47a3ba0c30b29e5075a
base 0ff6ae43b1caa039c2a6262f07678b88314a5b4f
16 6daff6d0fc4a9299deb0a51881e14cdbda16b88d 1 submod
16 ee8321115a919c0607236124af886df2c9f16e2f 2 submod
16 f3abce3ddcc2d68a8c113bd16767deeb376276f9 3 submod
replacements:
deletions: 3
diff --git a/read-cache.c b/read-cache.c
index 2ba67ce..b502290 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1528,6 +1528,7 @@ int do_read_index(struct index_state *istate, const char
*path, int must_exist,
struct cache_header *hdr;
void *mmap;
size_t mmap_size;
+ int fully_merged = 1;
struct strbuf previous_name_buf = STRBUF_INIT, *previous_name;
if (istate->initialized)
@@ -1580,6 +1581,10 @@ int do_read_index(struct index_state *istate, const char
*path, int must_exist,
ce = create_from_disk(disk_ce, &consumed, previous_name);
set_index_entry(istate, i, ce);
+ if (ce_stage(ce)) {
+ fully_merged = 0;
+ }
+
if (i > 0)
if (check_ce_order(istate->cache[i - 1], ce, 1) > 0 &&
multiple_stage_entries)
@@ -1610,6 +1615,8 @@ int do_read_index(struct index_state *istate, const char
*path, int must_exist,
src_offset += extsize;
}
munmap(mmap, mmap_size);
+ if (!fully_merged && istate->split_index)
+ die("base index cannot contain staged entries");
return istate->cache_nr;
unmap:
> sorting order is messed up, it could lead to other problems. So
> instead of removing the test from do_read_index(), perhaps add a flag
> in check_ce_order() to optionally detect the stage problem, but
> print/do nothing, only set a flag so the caller know there may be a
> problem. In the two new call sites you added, we still call the new
> check_ce_order() again to make sure everything is in order. In the
> call site where split index is not active, if the previous
> check_ce_order() call from inside do_read_index() says "everything is
> ok", we could even skip the check.
> --
> Duy
--
Thomas Gummerer
--
To unsubscribe from this list: send the line "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/1] l10n: de.po: use "bla …" instead of "bla..."
Michael J Gruber wrote: > Ralf Thielow venit, vidit, dixit 21.03.2015 22:21: > > Am 21. März 2015 um 13:52 schrieb Phillip Sz : > >> > >> I think we should use it like this, as most open-source projects do. > >> Also we should use a space before the three dots as per > >> http://www.duden.de/sprachwissen/rechtschreibregeln/auslassungspunkte > >> > > > > I don't think this rule of ellipsis applies here as the dots are meant > > to be a pattern to tell users that an argument can be passed multiple > > times. > > > > "..." is used in (at least) 2 cases: > > * ellipsis > * continuation arguments > > The patch changes both, but the Duden rule applies to the first case > only (and is different from "legacy rules", I think). > > Also, you might want to check the format of other commit messages and > reword yours accordingly. > Let's apply this instead. -- >8 -- From: Phillip Sz Date: Sat, 21 Mar 2015 13:52:37 +0100 Subject: [PATCH] l10n: de.po: add space before ellipsis Signed-off-by: Phillip Sz Signed-off-by: Ralf Thielow --- po/de.po | 32 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/po/de.po b/po/de.po index 11fbd0f..9fa3f4c 100644 --- a/po/de.po +++ b/po/de.po @@ -616,7 +616,7 @@ msgstr "" #: help.c:373 #, c-format msgid "in %0.1f seconds automatically..." -msgstr "Automatische Ausführung in %0.1f Sekunden..." +msgstr "Automatische Ausführung in %0.1f Sekunden ..." #: help.c:380 #, c-format @@ -1271,12 +1271,12 @@ msgstr "Kann keine Commit-Beschreibung für %s bekommen" #: sequencer.c:611 #, c-format msgid "could not revert %s... %s" -msgstr "Konnte \"revert\" nicht auf %s ausführen... %s" +msgstr "Konnte \"revert\" nicht auf %s ausführen ... %s" #: sequencer.c:612 #, c-format msgid "could not apply %s... %s" -msgstr "Konnte %s nicht anwenden... %s" +msgstr "Konnte %s nicht anwenden ... %s" #: sequencer.c:648 msgid "empty commit set passed" @@ -2436,7 +2436,7 @@ msgstr "%s: Patch konnte nicht angewendet werden" #: builtin/apply.c:3653 #, c-format msgid "Checking patch %s..." -msgstr "Prüfe Patch %s..." +msgstr "Prüfe Patch %s ..." #: builtin/apply.c:3746 builtin/checkout.c:231 builtin/reset.c:135 #, c-format @@ -4091,7 +4091,7 @@ msgstr "Konnte zu klonenden Remote-Branch %s nicht finden." #: builtin/clone.c:561 #, c-format msgid "Checking connectivity... " -msgstr "Prüfe Konnektivität... " +msgstr "Prüfe Konnektivität ... " #: builtin/clone.c:564 msgid "remote did not send all necessary objects" @@ -4165,12 +4165,12 @@ msgstr "Konnte Arbeitsverzeichnis '%s' nicht erstellen." #: builtin/clone.c:870 #, c-format msgid "Cloning into bare repository '%s'...\n" -msgstr "Klone in Bare-Repository '%s'...\n" +msgstr "Klone in Bare-Repository '%s' ...\n" #: builtin/clone.c:872 #, c-format msgid "Cloning into '%s'...\n" -msgstr "Klone nach '%s'...\n" +msgstr "Klone nach '%s' ...\n" #: builtin/clone.c:897 msgid "--dissociate given, but there is no --reference" @@ -4600,7 +4600,7 @@ msgstr "" #: builtin/commit.c:1199 msgid "Clever... amending the last one with dirty index." msgstr "" -"Klug... den letzten Commit mit einer geänderten Staging-Area nachbessern." +"Klug ... den letzten Commit mit einer geänderten Staging-Area nachbessern." #: builtin/commit.c:1201 msgid "Explicit paths specified without -i or -o; assuming --only paths..." @@ -7335,7 +7335,7 @@ msgstr "Aktualisiere %s..%s\n" #: builtin/merge.c:1388 #, c-format msgid "Trying really trivial in-index merge...\n" -msgstr "Probiere wirklich trivialen \"in-index\"-Merge...\n" +msgstr "Probiere wirklich trivialen \"in-index\"-Merge ...\n" #: builtin/merge.c:1395 #, c-format @@ -7349,12 +7349,12 @@ msgstr "Vorspulen nicht möglich, breche ab." #: builtin/merge.c:1450 builtin/merge.c:1529 #, c-format msgid "Rewinding the tree to pristine...\n" -msgstr "Rücklauf des Verzeichnisses bis zum Ursprung...\n" +msgstr "Rücklauf des Verzeichnisses bis zum Ursprung ...\n" #: builtin/merge.c:1454 #, c-format msgid "Trying merge strategy %s...\n" -msgstr "Probiere Merge-Strategie %s...\n" +msgstr "Probiere Merge-Strategie %s ...\n" #: builtin/merge.c:1520 #, c-format @@ -7682,7 +7682,7 @@ msgstr "git notes copy [] " #: builtin/notes.c:51 msgid "git notes copy --stdin [ ]..." -msgstr "git notes copy --stdin [ ]..." +msgstr "git notes copy --stdin [ ] ..." #: builtin/notes.c:56 msgid "git notes append [] []" @@ -8689,7 +8689,7 @@ msgstr "git remote prune [] " #: builtin/remote.c:64 msgid "git remote update [] [ | ]..." -msgstr "git remote update [] [ | ]..." +msgstr "git remote update [] [ | ] ..." #: builtin/remote.c:88 #, c-format @@ -9865,7 +9865,7 @@ msgstr "fehlerhaftes Objekt bei '%s'" #: builtin/tag.c:301 #, c-format msgid "tag name too long: %.*s..." -msgstr "Tagname zu lang: %.*s..." +msgstr "Tagname zu lang: %.*s ..." #: builtin/tag.c:306 #, c-format @@ -10450,7 +10450,7 @@ msgstr "" #: git-am.sh:166 msgid "F
Re: [PATCH 09/15] http: release the memory of a http pack request as well
On Sun, Mar 22, 2015 at 12:36 PM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> The cleanup function is used in 4 places now and it's always safe to
>> free up the memory as well.
>>
>> Signed-off-by: Stefan Beller
>> ---
>> http.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/http.c b/http.c
>> index 9c825af..4b179f6 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -1462,6 +1462,7 @@ void release_http_pack_request(struct
>> http_pack_request *preq)
>> }
>> preq->slot = NULL;
>> free(preq->url);
>> + free(preq);
>> }
>>
>> int finish_http_pack_request(struct http_pack_request *preq)
>
> Freeing of preq in all the callers of this one looks sensible,
> except for the one in finish_request() of http-push.c that pulls an
> preq instance out of request->userData.
>
> Can somebody help me follow the dataflow to convince me that this is
> not leading to double-free in start_fetch_packed()?
I am not sure where you suspect the double free problem.
In start_fetch_packed we have a call to release_http_pack_request
2 times but just in an error-out-and-cleanup case, so either of one cases
is called.
In the latter place (http-push.c lines 335-347), we have code like
request->userData = preq;
if (!start_active_slot(preq->slot)) {
release_http_pack_request(preq);
repo->can_update_info_refs = 0;
release_request(request);
}
Do you mean that the release_http_pack_request and release_request may collide
as the `release_request(request);` has a pointer to preq via its userData field?
Well there is hope, as `release_request` only touches
free(request->url);
free(request);
and not the userData pointer.
I am a bit puzzled what you're trying to hint at.
>
> 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 00/14] numparse module: systematically tighten up integer parsing
Am 24.03.2015 um 17:06 schrieb Michael Haggerty: Parsing numbers is not rocket science, but there are a lot of pitfalls, especially around overflow. It's even harder to write such code via macros and the result is less readable. This patch series is mostly about finding a reasonable API and whipping the callers into shape. That seems ambitious enough for me. I'd rather stick with boring wrappers for now and lean on strtol()/strtoul() to do the dirty work. It will be easy for a motivated person to change the implementation later. The OpenBSD developers invented strtonum for that. Are you aware of it? Would it fit? This discussion may be useful: http://www.tedunangst.com/flak/post/the-design-of-strtonum René -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/GSoC] Proposal: Make git-pull and git-am builtins
Hi all, I'm applying for git in the Google Summer of Code this year. For my project, I propose to rewrite git-pull.sh and git-am.sh into fast optimized C builtins. I've already hacked up a prototype of a builtin git-pull in [1], and it showed a promising 8x improvement in execution time on Windows. Below is the full text of the proposal as submitted to google-melange for your review and feedback. It is marked up in reStructuredText. The latest (and rendered) version can be found at [2]. Regards, Paul. [1] http://thread.gmane.org/gmane.comp.version-control.git/265628 [2] https://gist.github.com/pyokagan/1b7b0d1f4dab6ba3cef1 (Thanks Matthieu for suggesting to post this on the mailing list. Will reply to your comments in a separate email). -->8-- Make `git-pull` and `git-am` builtins :Abstract: `git-pull` and `git-am` are frequently used git subcommands. However, they are porcelain commands and implemented as shell scripts, which has some limitations which can cause poor performance, especially in non-POSIX environments like Windows. I propose to rewrite these scripts into low level C code and make them builtins. This will increase git's portability, and may improve the efficiency and performance of these commands. .. section-numbering:: Limitations of shell scripts = `git-pull` is a commonly executed command to check for new changes in the upstream repository and, if there are, fetch and integrate them into the current branch. `git-am` is another commonly executed command for applying a series of patches from a mailbox to the current branch. They are both git porcelain commands -- with no access to git's low level internal API. Currently, they are implemented by the shell scripts ``git-pull.sh`` and ``git-am.sh`` respectively. These shell scripts require a fully-functioning POSIX shell and utilities. As a result, these commands are difficult to port to non-POSIX environments like Windows. Since porcelain commands do not have access to git's internal API, performing any git-related function, no matter how trivial, requires git to be spawned in a separate process. This limitation leads to these git commands being relatively inefficient, and can cause long run times on certain platforms that do not have copy-on-write ``fork()`` semantics. Spawning processes can be slow --- Shell scripting, by itself, is severely limited in what it can do. Performing most operations in shell scripts require external executables to be called. For example, ``git-pull.sh`` spawns the git executable not only to perform git operations like `git-fetch` and `git-merge`, but it also spawns the git executable for trivial tasks such as retrieving configuration values with `git-config` and even quoting of command-line arguments with ``git rev-parse --sq-quote``. As a result, these shell scripts usually end up spawning a lot of processes. Process spawning is usually implemented as a ``fork()`` followed by an ``exec()`` by shells. This can be slow on systems that do not support copy-on-write semantics for ``fork()``, and thus needs to duplicate the memory of the parent process for every ``fork()`` call -- an expensive process. Furthermore, starting up processes on Windows is generally expensive as it performs `several extra steps`_ such as such as using an inter-process call to notify the Windows Client/Server Runtime Subsystem(CSRSS) about the process creation and checking for App Compatibility requirements. .. _`several extra steps`: http://www.microsoft.com/mspress/books/sampchap/4354a.aspx The official Windows port of git, Git for Windows, uses MSYS2 [#]_ to emulate ``fork()``. Since Windows does not support forking semantics natively, MSYS2 can only emulate ``fork()`` `without copy-on-write semantics`_. Coupled with Windows heavy process creation, this causes huge slowdowns of git on Windows. .. _`without copy-on-write semantics`: https://www.cygwin.com/faq.html#faq.api.fork A "no-updates" `git-pull`, for example, takes an average of 5.1s [#]_, as compared to Linux which only takes an average of 0.08s. 5 seconds, while seemingly short, would seem like an eternity to a user who just wants to quickly fetch and merge changes from upstream. `git-am`'s implementation reads each patch from the mailbox in a while loop, spawning many processes for each patch. Considering the cost of spawning each process, as well as the fact that runtime grows linearly with the number of patches, git-am takes a long time to process a seemingly small number of patches on Windows as compared to Linux. A quick benchmarks shows that `git-am` takes 7m 20.39s to apply 100 patches on Windows, compared to Linux, which took only 0.08s. Commands which call `git-am` are also affected as well. ``git-rebase--am.sh``, which implements the defau
Re: [PATCH 1/1] l10n: de.po: use "bla …" instead of "bla..."
Ralf Thielow venit, vidit, dixit 21.03.2015 22:21: > Am 21. März 2015 um 13:52 schrieb Phillip Sz : >> >> I think we should use it like this, as most open-source projects do. >> Also we should use a space before the three dots as per >> http://www.duden.de/sprachwissen/rechtschreibregeln/auslassungspunkte >> > > I don't think this rule of ellipsis applies here as the dots are meant > to be a pattern to tell users that an argument can be passed multiple > times. > "..." is used in (at least) 2 cases: * ellipsis * continuation arguments The patch changes both, but the Duden rule applies to the first case only (and is different from "legacy rules", I think). Also, you might want to check the format of other commit messages and reword yours accordingly. Michael -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing
On 03/19/2015 07:22 AM, Junio C Hamano wrote:
> Michael Haggerty writes:
>
>> * It allows leading whitespace.
>
> This might be blessing in disguise. Our friends on MacOS may be
> relying on that
>
> git cmd --abbrev="$(wc -c
> to work "as expected", even though their "wc" gives leading spaces,
> for example.
Yuck. If you'd like, I can make sure to continue allowing leading
whitespace everywhere that it is allowed now. Alternatively, we could
make the parsing tighter and see if anybody squeals. What do you think?
>> * It allows arbitrary trailing characters.
>
> Which is why we have introduced strtoul_ui() and such.
>
>> * It allows a leading sign character ('+' or '-'), even though the
>> result is unsigned.
>
> I do not particularly see it a bad thing to accept "--abbrev=+7".
> Using unsigned type to accept a width and parsing "--abbrev=-7" into
> a large positive integer _is_ a problem, and we may want to fix it,
> but I think that is still within the scope of the original "better
> strtol/strtoul", and I do not see it as a justification to introduce
> a set of functions with completely unfamiliar name.
It is easy to allow "--abbrev=+7"; I would just need to add NUM_PLUS to
those call sites. Should I do so?
It would even be possible to allow "--abbrev=-0", which is also a
non-negative integer. But it's more work and it seems pretty silly to me.
>> * If the string doesn't contain a number at all, it sets its "endptr"
>> argument to point at the start of the string but doesn't set errno.
>
> Why is that a problem? A caller that cares is supposed to check
> endptr and the string it gave, no? Now, if strtoul_ui() and friends
> do not do so, I again think that is still within the scope of the
> original "better strtol/strtoul".
The text you are quoting was my argument for why we need wrappers around
strtol() and strtoul(), not for how the wrappers should be named.
The "endptr" convention for detecting errors is fine in theory, but in
practice a large majority of callers didn't check for errors correctly.
This is partly because the "endptr" convention is so awkward.
The existing strtoul_ui() and strtol_i() did check endptr correctly, but
there were only int-sized variants of the functions, and they didn't
give the caller the chance to capture endptr if the caller wanted to
allow trailing characters.
Why did I rename the wrapper functions?
* The old names, I think, emphasize that they take the long-sized
results of strtou?l() and convert them to integer size, which I think is
an implementation detail.
* The new functions will also have long versions. The obvious way to
generalize the existing function names for long variants (srtoul_ul()
and strtol_l()) seem kindof redundant.
* I wanted to change the signature and behavior of the functions, so
knowledge of the existing functions wouldn't be super helpful in
understanding the new ones.
>> * If the value is larger than fits in an unsigned long, it returns the
>> value clamped to the range 0..ULONG_MAX (setting errno to ERANGE).
>
> Ditto.
>
>> * If the value is between -ULONG_MAX and 0, it returns the positive
>> integer with the same bit pattern, without setting errno(!) (I can
>> hardly think of a scenario where this would be useful.)
>
> Ditto.
>
>> * For base=0 (autodetect base), it allows a base specifier prefix "0x"
>> or "0" and parses the number accordingly. For base=16 it also allows
>> a "0x" prefix.
>
> Why is it a problem to allow "git cmd --hexval=0x1234", even if "git
> cmd --hexval=1234" would suffice?
In some cases we would like to allow that flexibility; in some cases
not. But the strtol()/strtoul() functions *always* allow it.
>> When I looked around, I found scores of sites that call atoi(),
>> strtoul(), and strtol() carelessly. And it's understandable. Calling
>> any of these functions safely is so much work as to be completely
>> impractical in day-to-day code.
>
> Yes, these burdens on the caller were exactly why strtoul_ui()
> etc. wanted to reduce---and it will be a welcome change to do
> necessary checks that are currently not done.
>
>> Please see the docstrings in numparse.h in the first commit for
>> detailed API docs. Briefly, there are eight new functions:
>>
>> parse_{l,ul,i,ui}(const char *s, unsigned int flags,
>> T *result, char **endptr);
>> convert_{l,ul,i,ui}(const char *s, unsigned int flags, T *result);
>>
>> The parse_*() functions are for parsing a number from the start of a
>> string; the convert_*() functions are for parsing a string that
>> consists of a single number.
>
> I am not sure if I follow. Why not unify them into one 4-function
> set, with optional endptr that could be NULL?
In the next version of this patch series I plan to include only four
functions, str_to_{i,ui,l,ul}(), named that way to make them more
reminiscent of the functions that they replace. They will take an entptr
argument, but if that argument is set to NULL,
Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing
Junio C Hamano writes: > Michael Haggerty writes: > >> It is easy to allow "--abbrev=+7"; I would just need to add NUM_PLUS to >> those call sites. Should I do so? > > The more relevant question to ask from my point of view is why you > need to "add" NUM_PLUS to "enable" it. What valid reason do you > have to forbid it anywhere? Only because you do not accept it by > default, you need to "add" to "enable". > >>> Why is it a problem to allow "git cmd --hexval=0x1234", even if "git >>> cmd --hexval=1234" would suffice? >> >> In some cases we would like to allow that flexibility; in some cases >> not. But the strtol()/strtoul() functions *always* allow it. > > The same issue. Whare are these "some cases"? And the same issue appears in the "leading whitespace" thing I did not mention in the earlier part of your message I responded to. I also notice you answered yourself that there may not be a valid reason to forbid end-user supplied "0x" prefix to arguments we expect an integer for in your other message. In short, if it is not a clearly bogus input that indicates a typo or something (e.g. "--size=48l? did the user meant 48, 48k, or 48m?"), and if it is clear we can tell the user meant what the code would naturally interpret as (e.g. "--hexval=0x1234"), why forbid it? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing
On 03/19/2015 08:32 AM, Junio C Hamano wrote:
> Jeff King writes:
>
>> I wonder how much of the boilerplate in the parse_* functions could be
>> factored out to use a uintmax_t, with the caller just providing the
>> range. That would make it easier to add new types like off_t, and
>> possibly even constrained types (e.g., an integer from 0 to 100). On the
>> other hand, you mentioned to me elsewhere that there may be some bugs in
>> the range-checks of config.c's integer parsing. I suspect they are
>> related to exactly this kind of refactoring, so perhaps writing things
>> out is best.
>
> I like this idea very well. I wonder if we can implement the family
> of
>
> parse_{type}(const char *, unsigned int flags,
>const char **endptr, {type} *result)
>
> functions by calls a helper that internally deals with the numbers
> in uintmax_t, and then checks if the value fits within the possible
> range of the *result before returning.
>
> int parse_i(const char *str, unsigned flags,
> const char **endptr, int *result) {
> uintmax_t val;
> int sign = 1, status;
> if (*str == '-') {
> sign = -1;
> str++;
> }
> status = parse_num(str, flags, endptr, &val, INT_MAX);
> if (status < 0)
> return status;
> *result = sign < 0 ? -val : val;
> return 0;
> }
>
> (assuming the last parameter to parse_num is used to check if the
> result fits within that range). Or that may be easier and cleaner
> to be done in the caller with or something like that:
>
> status = parse_num(str, flags, endptr, &val);
> if (status < 0)
> return status;
> if (INT_MAX <= val * sign || val * sign <= INT_MIN) {
> errno = ERANGE;
> return -1;
> }
>
> If we wanted to, we may even be able to avoid duplication of
> boilerplate by wrapping the above pattern within a macro,
> parameterizing the TYPE_{MIN,MAX} and using token pasting, to
> expand to four necessary result types.
>
> There is no reason for the implementation of the parse_num() helper
> to be using strtoul(3) or strtoull(3); its behaviour will be under
> our total control. It can become fancier by enriching the flags
> bits (e.g. allow scaling factor, etc.) only once and all variants
> for various result types will get the same feature.
Parsing numbers is not rocket science, but there are a lot of pitfalls,
especially around overflow. It's even harder to write such code via
macros and the result is less readable.
This patch series is mostly about finding a reasonable API and whipping
the callers into shape. That seems ambitious enough for me. I'd rather
stick with boring wrappers for now and lean on strtol()/strtoul() to do
the dirty work. It will be easy for a motivated person to change the
implementation later.
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
--
To unsubscribe from this list: send the line "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/GSOC] make git-pull a builtin
On Mon, Mar 23, 2015 at 6:18 PM, Duy Nguyen wrote: > Could you share these changes? I'm just wondering if we can add kcov > support to the test suite. In this case it's more of an embarrassing hack, as I just needed a way to make git run "kcov outdir git-pull.sh" whenever git pull is called since kcov will not instrument any spawned subprocesses. I modified execv_dashed_external() in git.c to prepend kcov to argv (diff probably munged by gmail): diff --git a/git.c b/git.c index 8c7ee9c..0f8e7d4 100644 --- a/git.c +++ b/git.c @@ -536,6 +536,8 @@ static void execv_dashed_external(const char **argv) struct strbuf cmd = STRBUF_INIT; const char *tmp; int status; + struct argv_array args = ARGV_ARRAY_INIT; + int i; if (use_pager == -1) use_pager = check_pager_config(argv[0]); @@ -551,6 +553,11 @@ static void execv_dashed_external(const char **argv) */ tmp = argv[0]; argv[0] = cmd.buf; + argv_array_push(&args, "kcov"); + argv_array_push(&args, "/home/pyokagan/pyk/git/kcov"); + argv_array_push(&args, cmd.buf); + for (i = 1; argv[i]; i++) + argv_array_push(&args, argv[i]); trace_argv_printf(argv, "trace: exec:"); @@ -558,7 +565,7 @@ static void execv_dashed_external(const char **argv) * if we fail because the command is not found, it is * OK to return. Otherwise, we just pass along the status code. */ - status = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE | RUN_CLEAN_ON_EXIT); + status = run_command_v_opt(args.argv, RUN_SILENT_EXEC_FAILURE | RUN_CLEAN_ON_EXIT); if (status >= 0 || errno != ENOENT) exit(status); I'm guessing a real solution is to follow what the test suite does for the --valgrind option, though I haven't looked into it in detail. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing
Michael Haggerty writes: > It is easy to allow "--abbrev=+7"; I would just need to add NUM_PLUS to > those call sites. Should I do so? The more relevant question to ask from my point of view is why you need to "add" NUM_PLUS to "enable" it. What valid reason do you have to forbid it anywhere? Only because you do not accept it by default, you need to "add" to "enable". >> Why is it a problem to allow "git cmd --hexval=0x1234", even if "git >> cmd --hexval=1234" would suffice? > > In some cases we would like to allow that flexibility; in some cases > not. But the strtol()/strtoul() functions *always* allow it. The same issue. Whare are these "some cases"? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFH/PATCH] git-svn: adjust info to svn 1.7 and 1.8
t9119 refuses to run with svn versions greater than 1.6 since "git svn
info" does not even try to match the output of "svn info" for later
versions.
Adjust "git svn info" to match these versions and make t9119 run with
them. This requires the following changes:
* compute checksums with SHA1 instead of MD5 with svn >= 1.7.0
* omit the line "Revision: 0" for added content with svn >= 1.8.0 (TBC)
* print the "Repository UUID" line even for added content with svn >=
1.8.0 (TBC)
* add a "Relative URL" line for svn >= 1.8.0
* add a "Working Copy Root Path" line for svn >= 1.8.0 (TBC, RFH)
Signed-off-by: Michael J Gruber
---
Notes:
While trying to increase my test run coverage I noticed that most of us
won't
run t9119 at all. Bad bad.
My svn is 1.8.11 (r1643975) on Fedora 21.
I would appreciate help with the following items:
TBC = to be confirmed: confirm the svn version where this change kicked it,
or run this patch and t9119 with an svn version other than mine. Please
run with "-v" to make sure only the RFH item fails, see below.
RFH = request for help: I couldn't figure out how to get the working
copy root path in cmd_info.
18 subtests will fail because of the RFH item.
git-svn.perl| 38 +-
t/t9119-git-svn-info.sh | 2 +-
2 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 36f7240..00c9cc1 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1580,6 +1580,7 @@ sub cmd_info {
}
# canonicalize_path() will return "" to make libsvn 1.5.x happy,
+ my $rpath = $path;
$path = "." if $path eq "";
my $full_url = canonicalize_url( add_path_to_url( $url, $path ) );
@@ -1591,7 +1592,9 @@ sub cmd_info {
my $result = "Path: $path_arg\n";
$result .= "Name: " . basename($path) . "\n" if $file_type ne "dir";
+ $result .= "Working Copy Root Path: " . $gs->path . "\n" if
::compare_svn_version('1.7.0') >= 0; #TODO
$result .= "URL: $full_url\n";
+ $result .= "Relative URL: ^/" . $rpath . "\n" if
::compare_svn_version('1.8.0')>=0;
eval {
my $repos_root = $gs->repos_root;
@@ -1603,8 +1606,10 @@ sub cmd_info {
}
::_req_svn();
$result .= "Repository UUID: $uuid\n" unless $diff_status eq "A" &&
+ ::compare_svn_version('1.8.0') < 0 &&
(::compare_svn_version('1.5.4') <= 0 || $file_type ne "dir");
- $result .= "Revision: " . ($diff_status eq "A" ? 0 : $rev) . "\n";
+ $result .= "Revision: " . ($diff_status eq "A" ? 0 : $rev) . "\n" unless
+ $diff_status eq "A" && ::compare_svn_version('1.8.0') >= 0;
$result .= "Node Kind: " .
($file_type eq "dir" ? "directory" : "file") . "\n";
@@ -1653,19 +1658,19 @@ sub cmd_info {
command_output_pipe(qw(cat-file blob),
"HEAD:$path");
if ($file_type eq "link") {
my $file_name = <$fh>;
- $checksum = md5sum("link $file_name");
+ $checksum = md5sha1sum("link $file_name");
} else {
- $checksum = md5sum($fh);
+ $checksum = md5sha1sum($fh);
}
command_close_pipe($fh, $ctx);
} elsif ($file_type eq "link") {
my $file_name =
command(qw(cat-file blob), "HEAD:$path");
$checksum =
- md5sum("link " . $file_name);
+ md5sha1sum("link " . $file_name);
} else {
open FILE, "<", $path or die $!;
- $checksum = md5sum(\*FILE);
+ $checksum = md5sha1sum(\*FILE);
close FILE or die $!;
}
$result .= "Checksum: " . $checksum . "\n";
@@ -2135,6 +2140,29 @@ sub md5sum {
return $md5->hexdigest();
}
+sub md5sha1sum {
+ my $arg = shift;
+ my $ref = ref $arg;
+ my $md5;
+ if (::compare_svn_version('1.7.0') < 0) {
+ require Digest::MD5;
+ $md5 = Digest::MD5->new();
+ } else {
+ require Digest::SHA1;
+ $md5 = Digest::SHA1->new();
+ }
+if ($ref eq 'GLOB' || $ref eq 'IO::File' || $ref eq 'File::Temp') {
+ $md5->addfile($arg) or croak $!;
+ } elsif ($ref eq 'SCALAR') {
+ $md5->add($$arg) or croak $!;
+ } elsif (!$ref) {
+ $md5->add($arg) or croak $!;
+ } else {
+ fatal "Can't provide MD5 hash for unknown ref type: '", $ref,
"'";
+ }
+ return $md5->hexdigest();
+}
+
sub gc_directory {
if (can_compress() && -f $_ &&
Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing
On 03/19/2015 06:26 AM, Jeff King wrote: > On Tue, Mar 17, 2015 at 05:00:02PM +0100, Michael Haggerty wrote: > >> My main questions: >> >> * Do people like the API? My main goal was to make these functions as >> painless as possible to use correctly, because there are so many >> call sites. >> >> * Is it too gimmicky to encode the base together with other options in >> `flags`? (I think it is worth it to avoid the need for another >> parameter, which callers could easily put in the wrong order.) > > I definitely like the overall direction of this. My first thought was > that it does seem like there are a lot of possible options to the > functions (and OR-ing the flags with the base does seem weird, though I > can't think of a plausible case where it would actually cause errors). > Many of those options don't seem used in the example conversions (I'm > not clear who would want NUM_SATURATE, for example). There are a lot of options, but so far only few of them have been used. As the call sites are rewritten we will see which of these features are useful, and which can be dispensed with. If groups of options tend to be used together, we can define constants for them like I've done with NUM_SLOPPY and NUM_SIGN. Regarding NUM_SATURATE: I'm not sure who would want it either, but I thought there might be places where the user wants to specify (effectively) "infinity", and it might be convenient to let him specify something easy to type like "--max=" rather than "--max=2147483647". > I wondered if we could do away with the radix entirely. Wouldn't we be > asking for base 10 most of the time? Of course, your first few patches > show octal parsing, but I wonder if we should actually have a separate > parse_mode() for that, since that seems to be the general reason for > doing octal parsing. 10644 does not overflow an int, but it is > hardly a reasonable mode. Again, as a first pass I wanted to just have a really flexible API so that call sites can be rewritten without a lot of extra thought. If somebody wants to add a parse_mode() function, it will be easy to build on top of convert_ui(). But that change can be done after this one. > I also wondered if we could get rid of NUM_SIGN in favor of just having > the type imply it (e.g., convert_l would always allow negative numbers, > whereas convert_ul would not). But I suppose there are times when we end > up using an "int" to store an unsigned value for a good reason (e.g., > "-1" is a sentinel value, but we expect only positive values from the > user). So that might be a bad idea. Yes, as I was rewriting call sites, I found many that used the unsigned variants of the parsing functions but stored the result in an int. Probably some of these use -1 to denote "unset"; it might be that there are other cases where the variable could actually be declared to be "unsigned int". Prohibiting signs when parsing signed quantities isn't really elegant from an API purity point of view, but it sure is handy! > I notice that you go up to "unsigned long" here for sizes. If we want to > use this everywhere, we'll need something larger for parsing off_t > values on 32-bit machines (though I would not at all be surprised with > the current code if 32-bit machines have a hard time configuring a > pack.packSizeLimit above 4G). Yes, probably. I haven't run into such call sites yet. > I wonder how much of the boilerplate in the parse_* functions could be > factored out to use a uintmax_t, with the caller just providing the > range. That would make it easier to add new types like off_t, and > possibly even constrained types (e.g., an integer from 0 to 100). On the > other hand, you mentioned to me elsewhere that there may be some bugs in > the range-checks of config.c's integer parsing. I suspect they are > related to exactly this kind of refactoring, so perhaps writing things > out is best. It's not a lot of code yet. If we find out we need variants for size_t and off_t and uintmax_t and intmax_t then such a refactoring would definitely be worth considering. >> * Am I making callers too strict? In many cases where a positive >> integer is expected (e.g., "--abbrev="), I have been replacing >> code like >> [...] > > IMHO most of the tightening happening here is a good thing, and means we > are more likely to notice mistakes rather than silently doing something > stupid. > > For sites that currently allow it, I could imagine people using hex > notation for some values, though, depending on the context. It looks > there aren't many of them ((it is just when the radix is "0", right?). > Some of them look to be accidental (does anybody really ask for > --threads=0x10?), but others might not be (the pack index-version > contains an offset field that might be quite big). Yes, we can even debate whether we want to implement a general policy that user-entered integers can be specified in any radix. Probably nobody will ever specify "--threads=0x10", but is
Re: [PATCH] git.txt: list index versions in plain English
Nguyễn Thái Ngọc Duy writes: > At the first look, a user may think the default version is "23". Even > with UNIX background, there's no reference anywhere close that may > indicate this is glob or regex. > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- Thanks. > Documentation/git.txt | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/Documentation/git.txt b/Documentation/git.txt > index b37f1ab..29d9257 100644 > --- a/Documentation/git.txt > +++ b/Documentation/git.txt > @@ -766,7 +766,8 @@ Git so take care if using Cogito etc. > 'GIT_INDEX_VERSION':: > This environment variable allows the specification of an index > version for new repositories. It won't affect existing index > - files. By default index file version [23] is used. > + files. By default index file version 2 or 3 is used. See > + linkgit:git-update-index[1] for more information. > > 'GIT_OBJECT_DIRECTORY':: > If the object storage directory is specified via this -- To unsubscribe from this list: send the line "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: per-repository and per-worktree config variables
On Thu, Mar 19, 2015 at 4:33 AM, Max Kirillov wrote: > On Sun, Feb 08, 2015 at 09:36:43AM -0800, Jens Lehmann wrote: >> I wonder if it's worth all the hassle to invent new names. Wouldn't >> it be much better to just keep a list of per-worktree configuration >> value names and use that inside the config code to decide where to >> find them for multiple work trees. That would also work easily for >> stuff like EOL-config and would push the complexity in the config >> machinery and not onto the user. > > I actually thought about the same, and now tend to think > that most of config variables make sense to be per-worktree > in some cases. Only few variable must always be per > repository. I tried to summarize the variables which now > (in current pu) should be common, also listed all the rest > so somebody could scan through the list and spot anything I > could miss. Thanks for compiling the list. At this point I think it may not be sensible to hard code some config vars (e.g. core.worktree) to be local or shared. So I'm thinking (out loud) that we may have a file $GIT_DIR/worktrees//local-config-patterns (or some other name) that would define what vars are local. gitignore syntax will be reused for this. The file would provide more flexibility.. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/15] commit.c: fix a memory leak
On Sat, Mar 21, 2015 at 10:59 AM, Junio C Hamano wrote: > A further tangent (Duy Cc'ed for this point). We might want to > rethink the interface to ce_path_match() and report_path_error() > so that we do not have to do a separate allocation of "has this > pathspec been used?" array. This was a remnant from the olden days > back when pathspec were mere "const char **" where we did not have > any room to add per-item bit---these days pathspec is repreasented > as an array of "struct pathspec" and we can afford to add a bit > to the structure---which will make this kind of leak much less > likely to happen. I just want to say "noted" (and therefore in my backlog). But no promise that it will happen any time soon. Low hanging fruit, perhaps some people may be interested.. -- Duy -- To unsubscribe from this list: send the line "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: Git ignore help
On Tue, Mar 24, 2015 at 7:46 PM, wrote: > Duy, you wrote: > > "This is true. To elaborate, if we have to recurse in excluded directories so > that we can include some back, then the reason for excluding is already > defeated as we may need to traverse the entire directory structure. However > in this particular case where we do know in advance that only certain > directories may have "re-include" rules, e.g. "db", "reports" or "scripts", > we could keep going for a while." > > ... so according to that it sounds like including /db, /reports, /scripts > should actually also NOT work. But it does work - i.e. when I add the > following: > > # exclude > /* > > # except > !/db > !/reports > !/scripts > > then any content within those 3 directories (and their sub directories) is > included and not ignored... > > It ONLY does not work when I add more levels - e.g.: > > !/reports/something > > In this case neither /reports nor /reports/something or any sub directory is > included. Yes. It's the subtlety of optimizing ;-) If you read the man page really carefully (*), "if the _parent_ directory of that file(**) is excluded" and the parent of these three directories is _not_ excluded. (*) I'm not saying this is a good thing. Only docs such as language spec or RFCs need that level of attention. But I'm not a good document writer myself, can't blame others. Improvements are welcome though. (**) "that file" should be "that file or directory" but I guess simplification here is ok -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] read-cache: fix reading of split index
On Sat, Mar 21, 2015 at 4:43 AM, Thomas Gummerer wrote: > The split index extension uses ewah bitmaps to mark index entries as > deleted, instead of removing them from the index directly. This can > result in an on-disk index, in which entries of stage #0 and higher > stages appear, which are removed later when the index bases are merged. > > 15999d0 read_index_from(): catch out of order entries when reading an > index file introduces a check which checks if the entries are in order > after each index entry is read in do_read_index. This check may however > fail when a split index is read. > > Fix this by moving checking the index after we know there is no split > index or after the split index bases are successfully merged instead. Thank you for catching this. I was about to write "would be nice to point out what tests fail so the reviewer has easier time trying themselves", but whoa.. quite a few of them! May I suggest a slight modification. Even though stage info is messed up before the index is merged, I think we should still check that both front and base indexes have all the names sorted correctly (and even stronger, the base index should never contain staged entries). If sorting order is messed up, it could lead to other problems. So instead of removing the test from do_read_index(), perhaps add a flag in check_ce_order() to optionally detect the stage problem, but print/do nothing, only set a flag so the caller know there may be a problem. In the two new call sites you added, we still call the new check_ce_order() again to make sure everything is in order. In the call site where split index is not active, if the previous check_ce_order() call from inside do_read_index() says "everything is ok", we could even skip the check. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Bug? git push --recurse-submodules=on-demand is not truly recursive
I have the following project structure: root-project | |-- A | | | |-- C | |-- B A and B are submodules of the root-project. C is in turn a submodule of project A. Suppose I have made changes to projects A,B and C and commited these changes to the respective indices. After that I update the references to A and B in the root-project and commit that change as well. When I push the commit of the root-project with the option --recurse-submodules=on-demand, git pushes the commits of projects A, B and the root-project but silently ignores all unpublished commits of project C. I end up publishing a project that no one can successfully clone because of the dangling link to C. Is this the expected behaviour or is this a bug? I have written a small shell script that sets up the project structure and executes the described scenario: https://gist.github.com/usommerl/6e8defcba94bd4ba1438 git version 2.3.3 Uwe Sommerlatt -- To unsubscribe from this list: send the line "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: Git ignore help
Hi Duy, Eric, thx a lot. So net net - I can't really achieve this. It feels like something very basic and simple so pretty surprising but guess that's what it is :) Normally, I'd expect that the functionality will behave like with similar other blacklist/whitelist functionalities in Linux - that the more specific definition overrides the less specific one. And when there are 2 on the same level then one (include or exclude) has the priority by default... Anyways, what I am still not clear on is how come that including just the 'single level folder' work? Duy, you wrote: "This is true. To elaborate, if we have to recurse in excluded directories so that we can include some back, then the reason for excluding is already defeated as we may need to traverse the entire directory structure. However in this particular case where we do know in advance that only certain directories may have "re-include" rules, e.g. "db", "reports" or "scripts", we could keep going for a while." ... so according to that it sounds like including /db, /reports, /scripts should actually also NOT work. But it does work - i.e. when I add the following: # exclude /* # except !/db !/reports !/scripts then any content within those 3 directories (and their sub directories) is included and not ignored... It ONLY does not work when I add more levels - e.g.: !/reports/something In this case neither /reports nor /reports/something or any sub directory is included. Martin -- Původní zpráva -- Od: Duy Nguyen Komu: Eric Sunshine Datum: 24. 3. 2015 10:40:33 Předmět: Re: Git ignore help On Tue, Mar 24, 2015 at 8:55 AM, Eric Sunshine wrote: >> e.g. "db", "reports" or "scripts", we could keep going for a while. I >> think I attempted to do this in the past and failed (don't remember >> exactly why). Maybe I'll try again some time in future. > > I also was pretty sure that you had attempted this, but couldn't find > a reference to it, so I didn't mention it in my response. However, > with some more digging, I finally located it[1]. > > [1]: http://article.gmane.org/gmane.comp.version-control.git/259870 Thank you. I only looked at my repo and no branch name suggested it (if only there is google search for a git repository..). So I gave up because of performance reasons again but that was for enabling it unconditionaly. If we enable it via a config variable and the user is made aware of the performance implications, I guess it would be ok. So it's back in my back log. -- Duy-- To unsubscribe from this list: send the line "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] gc: save log from daemonized gc --auto and print it next time
While commit 9f673f9 (gc: config option for running --auto in
background - 2014-02-08) helps reduce some complaints about 'gc
--auto' hogging the terminal, it creates another set of problems.
The latest in this set is, as the result of daemonizing, stderr is
closed and all warnings are lost. This warning at the end of cmd_gc()
is particularly important because it tells the user how to avoid "gc
--auto" running repeatedly. Because stderr is closed, the user does
not know, naturally they complain about 'gc --auto' wasting CPU.
Besides reverting 9f673f9 and looking at the problem from another
angle, we could save the stderr in $GIT_DIR/gc.log. Next time, 'gc
--auto' will print the saved warnings, delete gc.log and exit.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/gc.c | 37 -
t/t6500-gc.sh | 20
2 files changed, 56 insertions(+), 1 deletion(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index 5c634af..07769a9 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -32,6 +32,8 @@ static int aggressive_window = 250;
static int gc_auto_threshold = 6700;
static int gc_auto_pack_limit = 50;
static int detach_auto = 1;
+static struct strbuf log_filename = STRBUF_INIT;
+static int daemonized;
static const char *prune_expire = "2.weeks.ago";
static struct argv_array pack_refs_cmd = ARGV_ARRAY_INIT;
@@ -44,6 +46,15 @@ static char *pidfile;
static void remove_pidfile(void)
{
+ if (daemonized && log_filename.len) {
+ struct stat st;
+
+ close(2);
+ if (stat(log_filename.buf, &st) ||
+ !st.st_size ||
+ rename(log_filename.buf, git_path("gc.log")))
+ unlink(log_filename.buf);
+ }
if (pidfile)
unlink(pidfile);
}
@@ -324,13 +335,25 @@ int cmd_gc(int argc, const char **argv, const char
*prefix)
fprintf(stderr, _("See \"git help gc\" for manual
housekeeping.\n"));
}
if (detach_auto) {
+ struct strbuf sb = STRBUF_INIT;
+
+ if (strbuf_read_file(&sb, git_path("gc.log"), 0) > 0) {
+ warning(_("Last gc run reported the following,
gc skipped"));
+ fputs(sb.buf, stderr);
+ strbuf_release(&sb);
+ /* let the next gc --auto run as usual */
+ unlink(git_path("gc.log"));
+ return 0;
+ }
+
if (gc_before_repack())
return -1;
/*
* failure to daemonize is ok, we'll continue
* in foreground
*/
- daemonize();
+ if (!daemonize())
+ daemonized = 1;
}
} else
add_repack_all_option();
@@ -343,6 +366,18 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
name, (uintmax_t)pid);
}
+ if (daemonized) {
+ int fd;
+
+ strbuf_addstr(&log_filename, git_path("gc.log_XX"));
+ fd = xmkstemp(log_filename.buf);
+ if (fd >= 0) {
+ dup2(fd, 2);
+ close(fd);
+ } else
+ strbuf_release(&log_filename);
+ }
+
if (gc_before_repack())
return -1;
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index 63194d8..54bc9c4 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -30,4 +30,24 @@ test_expect_success 'gc -h with invalid configuration' '
test_i18ngrep "[Uu]sage" broken/usage
'
+test_expect_success !MINGW 'gc --auto and logging' '
+ git init abc &&
+ (
+ cd abc &&
+ # These create blobs starting with the magic number "17"
+ for i in 901 944; do
+ echo $i >test && git hash-object -w test >/dev/null
+ done &&
+ git config gc.auto 1 &&
+ LANG=C git gc --auto &&
+ sleep 1 && # give it time to daemonize
+ while test -f .git/gc.pid; do sleep 1; done &&
+ grep "too many unreachable loose objects" .git/gc.log &&
+ LANG=C git gc --auto 2>error &&
+ grep skipped error &&
+ grep "too many unreachable loose objects" error &&
+ ! test -f .git/gc.log
+ )
+'
+
test_done
--
2.3.0.rc1.137.g477eb31
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/3] t0302: test credential-store support for XDG_CONFIG_HOME
Paul Tan writes: > Matthieu and Eric: I know I said I will try to re-order the patches to > put the tests before the implementation, but after thinking and trying > to rewrite the commit messages I realised it seems really weird to me. > In this patch series, the implementation is split across the first two > patches. The first patch should use the old tests, and ideally, the new > tests should be squashed with the second patch because it seems more > logical to me to implement the tests at the same time as the new > feature. However, since the tests patch is very long, to make it easier > to review it is split into a separate patch which is applied after the > implementation patches. No problem, your version is very good. I was pointing out alternatives, but not requesting a change, and your reasoning makes perfect sense. I had reviewed v4 in details, and checked the diff between v4 and v5. The whole series is now Reviewed-by: Matthieu Moy Thanks, -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "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: Git ignore help
On Tue, Mar 24, 2015 at 8:55 AM, Eric Sunshine wrote: >> e.g. "db", "reports" or "scripts", we could keep going for a while. I >> think I attempted to do this in the past and failed (don't remember >> exactly why). Maybe I'll try again some time in future. > > I also was pretty sure that you had attempted this, but couldn't find > a reference to it, so I didn't mention it in my response. However, > with some more digging, I finally located it[1]. > > [1]: http://article.gmane.org/gmane.comp.version-control.git/259870 Thank you. I only looked at my repo and no branch name suggested it (if only there is google search for a git repository..). So I gave up because of performance reasons again but that was for enabling it unconditionaly. If we enable it via a config variable and the user is made aware of the performance implications, I guess it would be ok. So it's back in my back log. -- Duy -- To unsubscribe from this list: send the line "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: macblame - al alterntive to 'git blame'
On Tue, Mar 24, 2015 at 2:07 PM, Shenbaga Prasanna S wrote: > https://rubygems.org/gems/macblame/ > > check this out.. and you can also contribute to the developement at, > > https://github.com/praserocking/macblame-gem > or > https://github.com/praserocking/macblame > .. > hope this tool will be helpful to you all! It would help if you pasted a sample output to see what it looks like. Although macblame script says "macblame shows stats about the files tracked by git. It uses the output of 'git blame' and summarize it in a cleaner and intuitive format" so it's not really an alternative to git-blame (you can't "blame" anybody by line with this). This is more like "git blame --stat" (if that option existed) -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
macblame - al alterntive to 'git blame'
https://rubygems.org/gems/macblame/ check this out.. and you can also contribute to the developement at, https://github.com/praserocking/macblame-gem or https://github.com/praserocking/macblame .. hope this tool will be helpful to you all! Thanks, Prasanna -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
