Re: Bring together merge and rebase
On Fri, Dec 22, 2017 at 11:10:19PM -0700, Carl Baldwin wrote: > I've been calling this proposal `git replay` or `git replace` but I'd > like to hear other suggestions for what to name it. It works like > rebase except with one very important difference. Instead of orphaning > the original commit, it keeps a pointer to it in the commit just like > a `parent` entry but calls it `replaces` instead to distinguish it > from regular history. In the resulting commit history, following > `parent` pointers shows exactly the same history as if the commit had > been rebased. Meanwhile, the history of iterating on the change itself > is available by following `replaces` pointers. The new commit replaces > the old one but keeps it around to record how the change evolved. As a suggestion, before diving into the technical details of your proposal, it might be useful consider the usage scenario you are targetting. Things like "git rebase" and "git merge" and your proposed "git replace/replay" are *mechanisms*. But how they fit into a particular workflow is much more important from a design perspective, and given that there are many different git workflows which are used by different projects, and by different developers within a particular project. For example, rebase gets used in many different ways, and many of the debates when people talk about "git rebase" being evil generally presuppose a particular workflow that that the advocate has in mind. If someone is using git rebase or git commit --amend before git commits have ever been pushed out to a public repository, or to anyone else, that's a very different case where it has been visible elsewhere. Even the the most strident, "you must never rewrite a commit and all history must be preserved" generally don't insist that every single edit must be preserved on the theory that "all history is valuable". > The git history now has two dimensions. The first shows a cleaned up > history where fix ups and code review feedback have been rolled into > the original changes and changes can possibly be ordered in a nice > linear progression that is much easier to understand. The second > drills into the history of a change. There is no loss and you don't > change history in a way that will cause problems for others who have > the older commits. If your goal is to preserve the history of the change, one of the problems with any git-centric solution is that you generally lose the code review feedback and the discussions that are involved with a commit. Just simply preserving the different versions of the commits is going to lose a huge amount of the context that makes the history valuable. So for example, I would claim that if *that* is your goal, a better solution is to use Gerrit, so that all of the different versions of the commits are preserved along with the line-by-line comments and discussions that were part of the code review. In that model, each commit has something like this in the commit trailer: Change-Id: I8d89b33683274451bcd6bfbaf75bce98 You can then cut and paste the Change-Id into the Gerrit user interface, and see the different commits, more important, the discussion surrounding each change. If the complaint about Gerrit is that it's not a core part of Git, the challenge is (a) how to carry the code review comments in the git repository, and (b) do so in a while that it doesn't bloat the core repository, since most of the time, you *don't* want or need to keep a local copy of all of the code review comments going back since the beginning of the project. - Here's another potential use case. The stable kernels (e.g., 3.18.y, 4.4.y, 4.9.y, etc.) have cherry picks from the the upstream kernel, and this is handled by putting in the commit body something like this: [ Upstream commit 3a4b77cd47bb837b8557595ec7425f281f2ca1fe ] And here's yet another use case. For internal Google kernel development, we maintain a kernel that has a large number of patches on top of a kernel version. When we backport an upstream fix (say, one that first appeared in the 4.12 version of the upstream kernel), we include a line in the commit body that looks like this: Upstream-4.12-SHA1: 5649645d725c73df4302428ee4e02c869248b4c5 This is useful, because when we switch to use a newer upstream kernel, we need make sure we can account for all patches that were built on top of the 3xx kernel (which might have been using 4.10, for the sake of argument), to the 4xx kernel series (which might be using 4.15 --- the version numbers have been changed to protect the innocent). This means going through each and every patch that was on top of the 3xx kernel, and if it has a line such as "Upstream 4.12-SHA1", we know that it will already be included in a 4.15 based kernel, so we don't need to worry about carrying that patch forward. In other cases, we might decide that the patch is no longer needed. It could be because the patch has already be included upstream, in wh
PLEASE I NEED YOUR HELP FOR SCHOOL!
Dear Sir/Madam, Good day ! Please permit me to introduce myself, I am MIMI IBRAHIM COULIBALY 17 years old female from the Republic of Ivory Coast, in Abidjan; I'm the Daughter of Late Chief Sgt. Ibrahim Coulibaly (A.K.A General Warlord IB ). My late Father was a well known Ivory Coast military leader. He died on Thursday 28 April 2011 following a fight with the Republican Forces of Ivory Coast (FRCI). You can read more about my late father in the news graduating of ivory cost in bouake. www.guardian.co.uk/world/2011/apr/28/ivory-coast-renegade-warlord-ibrahim-coulibaly Please I want to leave my situation here to come over to your country to continue my education. I have made research before contacting you and please I want you to help me come to your country to start my new life. I will tell you more about my condition if you are willing to help me come over to your country to continue my education. I will truly appreciate your help to my life for my education. I will be happy to hear from you e-mail.mimiibrahimcouib...@gmail.com Miss. MIMI IBRAHIM COULIBALY
Re: [RFC/PATCH] perl: bump the required Perl version to 5.10.0 from 5.8.0
Ævar Arnfjörð Bjarmason wrote:
> I think for any given external dependency of git.git it makes sense to
> just pick a version, not say that this script requires perl so-and-so,
> this one python so-and-so, or curl/openssl so-and-so etc.
Agreed. Any version support changes should be tree-wide.
> On Sun, Dec 24 2017, Eric Wong jotted:
> > Maybe we change our docs to say we welcome 5.10 features for new
> > code, but I'm against changing things for the sake of change.
>
> I should have mentioned this in the commit message, but for me it's
> mainly that whenever I patch the Git perl code there's no easy way to
> test if it works on a currently supported release, 5.8.* doesn't even
> build anymore on a modern compiler without monkeypatching with
> Devel::PatchPerl (and then only some subreleases).
Fair enough, I haven't run 5.8 in a while, either. One concern
I have is it makes reviewing more difficult as the language gets
bigger and (even more) unfamiliar constructs pop up. This is
probably more important for git as most of us are not dedicated
Perl hackers.
What mostly bugs me about this is going from:
"we'll accept patches to keep your old system working"
to:
"your software is too old, upgrade or go away"
> I think it's reasonable for us, in general, to at some point pass the
> buck in maintaining dependencies to people who want to still build on
> ancient versions. And not just for perl, but e.g. curl too, which is
> something I commented on at some length in <874ltg2tvo@gmail.com>
> (https://public-inbox.org/git/874ltg2tvo@gmail.com/). I.e. if you
> need to really build the latest git on RHEL 5 with all bells & whistles
> you also build perl.
I don't disagree; but curl is different animal in that it was a
maintenance burden for us.
> It's not just change for the sake of change, there's a high cognitive
> overhead in trying to write code against the last 15 years of some
> software as opposed to "just" 10 years (which is already bad enough).
Heh, I was making the same point for staying with older versions
since it's less cognitive overhead for me (and presumably others)
to use a smaller featureset.
> Of course any one change isn't going to be what makes it or breaks it,
> so it's hard to make the argument in terms of "I must use this new
> feature here". But if that was the standard we were applying we'd still
> be supporting perl 5.6[1].
Right, if there's a compelling case to depend on 5.10 then I'm
all for it. I don't think we've hit a breaking point like
we did with 5.8, yet.
> 1. If it hadn't turned out that it was broken for years because of using
> a new feature, see d48b284183 ("perl: bump the required Perl version
> to 5.8 from 5.6.[21]", 2010-09-24)
[PATCH v2 5/7] wildmatch test: remove dead fnmatch() test code
Remove the unused fnmatch() test parameter from the wildmatch
test. The code that used to test this was removed in 70a8fc999d ("stop
using fnmatch (either native or compat)", 2014-02-15).
As a --word-diff shows the only change to the body of the tests is the
removal of the second out of four parameters passed to match().
Signed-off-by: Ævar Arnfjörð Bjarmason
---
t/t3070-wildmatch.sh | 356 +--
1 file changed, 178 insertions(+), 178 deletions(-)
diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
index 9691d8eda3..2f8a681c72 100755
--- a/t/t3070-wildmatch.sh
+++ b/t/t3070-wildmatch.sh
@@ -7,13 +7,13 @@ test_description='wildmatch tests'
match() {
if test "$1" = 1
then
- test_expect_success "wildmatch: match '$3' '$4'" "
- test-wildmatch wildmatch '$3' '$4'
+ test_expect_success "wildmatch: match '$2' '$3'" "
+ test-wildmatch wildmatch '$2' '$3'
"
elif test "$1" = 0
then
- test_expect_success "wildmatch: no match '$3' '$4'" "
- ! test-wildmatch wildmatch '$3' '$4'
+ test_expect_success "wildmatch: no match '$2' '$3'" "
+ ! test-wildmatch wildmatch '$2' '$3'
"
else
test_expect_success "PANIC: Test framework error. Unknown
matches value $1" 'false'
@@ -53,176 +53,176 @@ pathmatch() {
}
# Basic wildmat features
-match 1 1 foo foo
-match 0 0 foo bar
-match 1 1 '' ""
-match 1 1 foo '???'
-match 0 0 foo '??'
-match 1 1 foo '*'
-match 1 1 foo 'f*'
-match 0 0 foo '*f'
-match 1 1 foo '*foo*'
-match 1 1 foobar '*ob*a*r*'
-match 1 1 aaabababab '*ab'
-match 1 1 'foo*' 'foo\*'
-match 0 0 foobar 'foo\*bar'
-match 1 1 'f\oo' 'f\\oo'
-match 1 1 ball '*[al]?'
-match 0 0 ten '[ten]'
-match 0 1 ten '**[!te]'
-match 0 0 ten '**[!ten]'
-match 1 1 ten 't[a-g]n'
-match 0 0 ten 't[!a-g]n'
-match 1 1 ton 't[!a-g]n'
-match 1 1 ton 't[^a-g]n'
-match 1 x 'a]b' 'a[]]b'
-match 1 x a-b 'a[]-]b'
-match 1 x 'a]b' 'a[]-]b'
-match 0 x aab 'a[]-]b'
-match 1 x aab 'a[]a-]b'
-match 1 1 ']' ']'
+match 1 foo foo
+match 0 foo bar
+match 1 '' ""
+match 1 foo '???'
+match 0 foo '??'
+match 1 foo '*'
+match 1 foo 'f*'
+match 0 foo '*f'
+match 1 foo '*foo*'
+match 1 foobar '*ob*a*r*'
+match 1 aaabababab '*ab'
+match 1 'foo*' 'foo\*'
+match 0 foobar 'foo\*bar'
+match 1 'f\oo' 'f\\oo'
+match 1 ball '*[al]?'
+match 0 ten '[ten]'
+match 0 ten '**[!te]'
+match 0 ten '**[!ten]'
+match 1 ten 't[a-g]n'
+match 0 ten 't[!a-g]n'
+match 1 ton 't[!a-g]n'
+match 1 ton 't[^a-g]n'
+match 1 'a]b' 'a[]]b'
+match 1 a-b 'a[]-]b'
+match 1 'a]b' 'a[]-]b'
+match 0 aab 'a[]-]b'
+match 1 aab 'a[]a-]b'
+match 1 ']' ']'
# Extended slash-matching features
-match 0 0 'foo/baz/bar' 'foo*bar'
-match 0 0 'foo/baz/bar' 'foo**bar'
-match 0 1 'foobazbar' 'foo**bar'
-match 1 1 'foo/baz/bar' 'foo/**/bar'
-match 1 0 'foo/baz/bar' 'foo/**/**/bar'
-match 1 0 'foo/b/a/z/bar' 'foo/**/bar'
-match 1 0 'foo/b/a/z/bar' 'foo/**/**/bar'
-match 1 0 'foo/bar' 'foo/**/bar'
-match 1 0 'foo/bar' 'foo/**/**/bar'
-match 0 0 'foo/bar' 'foo?bar'
-match 0 0 'foo/bar' 'foo[/]bar'
-match 0 0 'foo/bar' 'foo[^a-z]bar'
-match 0 0 'foo/bar' 'f[^eiu][^eiu][^eiu][^eiu][^eiu]r'
-match 1 1 'foo-bar' 'f[^eiu][^eiu][^eiu][^eiu][^eiu]r'
-match 1 0 'foo' '**/foo'
-match 1 x 'XXX/foo' '**/foo'
-match 1 0 'bar/baz/foo' '**/foo'
-match 0 0 'bar/baz/foo' '*/foo'
-match 0 0 'foo/bar/baz' '**/bar*'
-match 1 0 'deep/foo/bar/baz' '**/bar/*'
-match 0 0 'deep/foo/bar/baz/' '**/bar/*'
-match 1 0 'deep/foo/bar/baz/' '**/bar/**'
-match 0 0 'deep/foo/bar' '**/bar/*'
-match 1 0 'deep/foo/bar/' '**/bar/**'
-match 0 0 'foo/bar/baz' '**/bar**'
-match 1 0 'foo/bar/baz/x' '*/bar/**'
-match 0 0 'deep/foo/bar/baz/x' '*/bar/**'
-match 1 0 'deep/foo/bar/baz/x' '**/bar/*/*'
+match 0 'foo/baz/bar' 'foo*bar'
+match 0 'foo/baz/bar' 'foo**bar'
+match 0 'foobazbar' 'foo**bar'
+match 1 'foo/baz/bar' 'foo/**/bar'
+match 1 'foo/baz/bar' 'foo/**/**/bar'
+match 1 'foo/b/a/z/bar' 'foo/**/bar'
+match 1 'foo/b/a/z/bar' 'foo/**/**/bar'
+match 1 'foo/bar' 'foo/**/bar'
+match 1 'foo/bar' 'foo/**/**/bar'
+match 0 'foo/bar' 'foo?bar'
+match 0 'foo/bar' 'foo[/]bar'
+match 0 'foo/bar' 'foo[^a-z]bar'
+match 0 'foo/bar' 'f[^eiu][^eiu][^eiu][^eiu][^eiu]r'
+match 1 'foo-bar' 'f[^eiu][^eiu][^eiu][^eiu][^eiu]r'
+match 1 'foo' '**/foo'
+match 1 'XXX/foo' '**/foo'
+match 1 'bar/baz/foo' '**/foo'
+match 0 'bar/baz/foo' '*/foo'
+match 0 'foo/bar/baz' '**/bar*'
+match 1 'deep/foo/bar/baz' '**/bar/*'
+match 0 'deep/foo/bar/baz/' '**/bar/*'
+match 1 'deep/foo/bar/baz/' '**/bar/**'
+match 0 'deep/foo/bar' '**/bar/*'
+match 1 'deep/foo/bar/' '**/bar/**'
+match 0 'foo/bar/baz' '**/bar**'
+match 1 'foo/bar/baz/x' '*/bar/**'
+match 0 'deep/foo/bar/baz/x' '*/bar/**'
+match 1 'deep/foo/bar/baz/x' '**/bar/*/*'
# Various additional tests
-match 0 0 'acrt' 'a[c-c]st'
-match 1 1 'acrt'
[PATCH v2 6/7] wildmatch test: perform all tests under all wildmatch() modes
Rewrite the wildmatch() test suite so that each test now tests all
combinations of the wildmatch() WM_CASEFOLD and WM_PATHNAME flags.
Before this change some test inputs were not tested on
e.g. WM_PATHNAME. Now the function is stress tested on all possible
inputs, and for each input we declare what the result should be if the
mode is case-insensitive, or pathname matching, or case-sensitive or
not matching pathnames.
Also before this change, nothing was testing case-insensitive
non-pathname matching, so I've added that to test-wildmatch.c and made
use of it.
This yields a rather scary patch, but there are no functional changes
here, just more test coverage. Some now-redundant tests were deleted
as a result of this change, since they were now duplicating an earlier
test.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
t/helper/test-wildmatch.c | 2 +
t/t3070-wildmatch.sh | 478 +++---
2 files changed, 239 insertions(+), 241 deletions(-)
diff --git a/t/helper/test-wildmatch.c b/t/helper/test-wildmatch.c
index 921d7b3e7e..66d33dfcfd 100644
--- a/t/helper/test-wildmatch.c
+++ b/t/helper/test-wildmatch.c
@@ -16,6 +16,8 @@ int cmd_main(int argc, const char **argv)
return !!wildmatch(argv[3], argv[2], WM_PATHNAME | WM_CASEFOLD);
else if (!strcmp(argv[1], "pathmatch"))
return !!wildmatch(argv[3], argv[2], 0);
+ else if (!strcmp(argv[1], "ipathmatch"))
+ return !!wildmatch(argv[3], argv[2], WM_CASEFOLD);
else
return 1;
}
diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
index 2f8a681c72..593b25b278 100755
--- a/t/t3070-wildmatch.sh
+++ b/t/t3070-wildmatch.sh
@@ -4,278 +4,274 @@ test_description='wildmatch tests'
. ./test-lib.sh
-match() {
- if test "$1" = 1
+wildtest() {
+ match_w_glob=$1
+ match_w_globi=$2
+ match_w_pathmatch=$3
+ match_w_pathmatchi=$4
+ text=$5
+ pattern=$6
+
+ if test "$match_w_glob" = 1
then
- test_expect_success "wildmatch: match '$2' '$3'" "
- test-wildmatch wildmatch '$2' '$3'
+ test_expect_success "wildmatch: match '$text' '$pattern'" "
+ test-wildmatch wildmatch '$text' '$pattern'
"
- elif test "$1" = 0
+ elif test "$match_w_glob" = 0
then
- test_expect_success "wildmatch: no match '$2' '$3'" "
- ! test-wildmatch wildmatch '$2' '$3'
+ test_expect_success "wildmatch: no match '$text' '$pattern'" "
+ ! test-wildmatch wildmatch '$text' '$pattern'
"
else
- test_expect_success "PANIC: Test framework error. Unknown
matches value $1" 'false'
+ test_expect_success "PANIC: Test framework error. Unknown
matches value $match_w_glob" 'false'
fi
-}
-imatch() {
- if test "$1" = 1
+ if test "$match_w_globi" = 1
then
- test_expect_success "iwildmatch: match '$2' '$3'" "
- test-wildmatch iwildmatch '$2' '$3'
+ test_expect_success "iwildmatch: match '$text' '$pattern'" "
+ test-wildmatch iwildmatch '$text' '$pattern'
"
- elif test "$1" = 0
+ elif test "$match_w_globi" = 0
then
- test_expect_success "iwildmatch: no match '$2' '$3'" "
- ! test-wildmatch iwildmatch '$2' '$3'
+ test_expect_success "iwildmatch: no match '$text' '$pattern'" "
+ ! test-wildmatch iwildmatch '$text' '$pattern'
"
else
- test_expect_success "PANIC: Test framework error. Unknown
matches value $1" 'false'
+ test_expect_success "PANIC: Test framework error. Unknown
matches value $match_w_globi" 'false'
fi
-}
-pathmatch() {
- if test "$1" = 1
+ if test "$match_w_pathmatch" = 1
then
- test_expect_success "pathmatch: match '$2' '$3'" "
- test-wildmatch pathmatch '$2' '$3'
+ test_expect_success "pathmatch: match '$text' '$pattern'" "
+ test-wildmatch pathmatch '$text' '$pattern'
"
- elif test "$1" = 0
+ elif test "$match_w_pathmatch" = 0
then
- test_expect_success "pathmatch: no match '$2' '$3'" "
- ! test-wildmatch pathmatch '$2' '$3'
+ test_expect_success "pathmatch: no match '$text' '$pattern'" "
+ ! test-wildmatch pathmatch '$text' '$pattern'
"
else
- test_expect_success "PANIC: Test framework error. Unknown
matches value $1" 'false'
+ test_expect_success "PANIC: Test framework error. Unknown
matches value $match_w_pathmatch" 'false'
+ fi
+
+ if test "$match_w_pathmatchi" = 1
+
[PATCH v2 3/7] wildmatch test: don't try to vertically align our output
Don't try to vertically align the test output, which is futile anyway
under the TAP output where we're going to be emitting a number for
each test without aligning the test count.
This makes subsequent changes of mine where I'm not going to be
aligning this output as I add new tests easier.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
t/t3070-wildmatch.sh | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
index 4d589d1f9a..19ea64bba9 100755
--- a/t/t3070-wildmatch.sh
+++ b/t/t3070-wildmatch.sh
@@ -7,11 +7,11 @@ test_description='wildmatch tests'
match() {
if test "$1" = 1
then
- test_expect_success "wildmatch: match '$3' '$4'" "
+ test_expect_success "wildmatch: match '$3' '$4'" "
test-wildmatch wildmatch '$3' '$4'
"
else
- test_expect_success "wildmatch: no match '$3' '$4'" "
+ test_expect_success "wildmatch: no match '$3' '$4'" "
! test-wildmatch wildmatch '$3' '$4'
"
fi
@@ -20,7 +20,7 @@ match() {
imatch() {
if test "$1" = 1
then
- test_expect_success "iwildmatch:match '$2' '$3'" "
+ test_expect_success "iwildmatch: match '$2' '$3'" "
test-wildmatch iwildmatch '$2' '$3'
"
else
@@ -33,11 +33,11 @@ imatch() {
pathmatch() {
if test "$1" = 1
then
- test_expect_success "pathmatch: match '$2' '$3'" "
+ test_expect_success "pathmatch: match '$2' '$3'" "
test-wildmatch pathmatch '$2' '$3'
"
else
- test_expect_success "pathmatch: no match '$2' '$3'" "
+ test_expect_success "pathmatch: no match '$2' '$3'" "
! test-wildmatch pathmatch '$2' '$3'
"
fi
--
2.15.1.424.g9478a66081
[PATCH v2 7/7] wildmatch test: create & test files on disk in addition to in-memory
There has never been any full roundtrip testing of what git-ls-files
and other functions that use wildmatch() actually do, rather we've
been satisfied with just testing the underlying C function.
Due to git-ls-files and friends having their own codepaths before they
call wildmatch() there's sometimes differences in the behavior between
the two, and even when we test for those (as with
9e4e8a64c2 ("pathspec: die on empty strings as pathspec", 2017-06-06))
there was no one place where you can review how these two modes
differ.
Now there is. We now attempt to create a file called $haystack and
match $needle against it for each pair of $needle and $haystack that
we were passing to test-wildmatch.
If we can't create the file we skip the test. This ensures that we can
run this on all platforms and not maintain some infinitely growing
whitelist of e.g. platforms that don't support certain characters in
filenames.
As a result of doing this we can now see the cases where these two
ways of testing wildmatch differ:
* Creating a file called 'a[]b' and running ls-files 'a[]b' will show
that file, but wildmatch("a[]b", "a[]b") will not match
* wildmatch() won't match a file called \ against \, but ls-files
will.
* `git --glob-pathspecs ls-files 'foo**'` will match a file
'foo/bba/arr', but wildmatch won't, however pathmatch will.
This seems like a bug to me, the two are otherwise equivalent as
these tests show.
This also reveals the case discussed in 9e4e8a64c2 above, where '' is
now an error as far as ls-files is concerned, but wildmatch() itself
happily accepts it.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
t/t3070-wildmatch.sh | 285 ---
1 file changed, 273 insertions(+), 12 deletions(-)
diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
index 593b25b278..80d13b5b60 100755
--- a/t/t3070-wildmatch.sh
+++ b/t/t3070-wildmatch.sh
@@ -4,14 +4,95 @@ test_description='wildmatch tests'
. ./test-lib.sh
+create_test_file() {
+ file=$1
+
+ case $file in
+ # `touch .` will succeed but obviously not do what we intend
+ # here.
+ ".")
+ return 1
+ ;;
+ # We cannot create a file with an empty filename.
+ "")
+ return 1
+ ;;
+ # The tests that are testing that e.g. foo//bar is matched by
+ # foo/*/bar can't be tested on filesystems since there's no
+ # way we're getting a double slash.
+ *//*)
+ return 1
+ ;;
+ # When testing the difference between foo/bar and foo/bar/ we
+ # can't test the latter.
+ */)
+ return 1
+ ;;
+ esac
+
+ # Turn foo/bar/baz into foo/bar to create foo/bar as a
+ # directory structure.
+ dirs=$(echo "$file" | sed -r 's!/[^/]+$!!')
+
+ # We touch "./$file" instead of "$file" because even an
+ # escaped "touch -- -" means get arguments from stdin.
+ if test "$file" != "$dirs"
+ then
+ mkdir -p -- "$dirs" &&
+ touch -- "./$file" &&
+ return 0
+ else
+ touch -- "./$file" &&
+ return 0
+ fi
+ return 1
+}
+
+wildtest_file_setup() {
+ test_when_finished "
+ rm -rf -- * &&
+ git reset
+ " &&
+ git add -A &&
+ >expect.err
+}
+
+wildtest_stdout_stderr_cmp() {
+ tr -d '\0' actual &&
+ test_cmp expect.err actual.err &&
+ test_cmp expect actual
+}
+
wildtest() {
- match_w_glob=$1
- match_w_globi=$2
- match_w_pathmatch=$3
- match_w_pathmatchi=$4
- text=$5
- pattern=$6
+ if test "$#" = 6
+ then
+ # When test-wildmatch and git ls-files produce the same
+ # result.
+ match_w_glob=$1
+ match_f_w_glob=$match_w_glob
+ match_w_globi=$2
+ match_f_w_globi=$match_w_globi
+ match_w_pathmatch=$3
+ match_f_w_pathmatch=$match_w_pathmatch
+ match_w_pathmatchi=$4
+ match_f_w_pathmatchi=$match_w_pathmatchi
+ text=$5
+ pattern=$6
+ elif test "$#" = 10
+ then
+ match_w_glob=$1
+ match_w_globi=$2
+ match_w_pathmatch=$3
+ match_w_pathmatchi=$4
+ match_f_w_glob=$5
+ match_f_w_globi=$6
+ match_f_w_pathmatch=$7
+ match_f_w_pathmatchi=$8
+ text=$9
+ pattern=$10
+ fi
+ # $1: Case sensitive glob match: test-wildmatch
if test "$match_w_glob" = 1
then
test_expect_success "wildmatch: match '$text' '$pattern'" "
@@ -26,6 +107,50 @@ wildtest() {
test_expect_success "PANIC: Test framework error. Unknown
matches value $match_w_glob" 'false'
[PATCH v2 0/7] increase wildmatch test coverage
This v2 addresses comments by Johannes Sixt in <8f4cdb23-8e2e-144a-1f70-99776b027...@kdbg.org> and there's osme other cleanups as noted below. Ævar Arnfjörð Bjarmason (7): wildmatch test: indent with tabs, not spaces wildmatch test: use more standard shell style No changes. wildmatch test: don't try to vertically align our output NEW: Don't try to do whitespace alignment in the tests. wildmatch test: use a paranoia pattern from nul_match() Explain in the commit message why we're not using say '...'; exit 1. I said I'd use this in <874logs7vi@evledraar.gmail.com>, but on further consideration it's a bad idea. wildmatch test: remove dead fnmatch() test code wildmatch test: perform all tests under all wildmatch() modes Just changes to rebase them on the changes above. wildmatch test: create & test files on disk in addition to in-memory Avoid some forking by using a case statement instead of if .. grep && return. Add comments to the code to clarify what it's doing. Factoro out the repetitive part of the tests into functions, making the patch shorter. I didn't change the "rm -rf -- *" pattern Johannes was concerned about, because after looking at it it would be a pain to create some-test-subdir/ and only if some-test-subdir/$our_file gets created cd to it and then remove it afterwards, it's much easier not to change the directory. The test test_when_finished always runs in the directory the tests executed in, so I don't see how this is dangerous in practice. I didn't move the "printf" pattern to here-docs as discussed in the thread. t/helper/test-wildmatch.c | 2 + t/t3070-wildmatch.sh | 759 +++--- 2 files changed, 516 insertions(+), 245 deletions(-) -- 2.15.1.424.g9478a66081
[PATCH v2 4/7] wildmatch test: use a paranoia pattern from nul_match()
Use a pattern from the nul_match() function in t7008-grep-binary.sh to
make sure that we don't just fall through to the "else" if there's an
unknown parameter.
This is something I added in commit 77f6f4406f ("grep: add a test
helper function for less verbose -f \0 tests", 2017-05-20) to grep
tests, which were modeled on these wildmatch tests, and I'm now
porting back to the original wildmatch tests.
I am not using the "say '...'; exit 1" pattern from t-basic.sh
because if I fail I want to run the rest of the tests (unless under
-i), and doing this makes sure we do that and don't exit right away
without fully reporting our errors.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
t/t3070-wildmatch.sh | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
index 19ea64bba9..9691d8eda3 100755
--- a/t/t3070-wildmatch.sh
+++ b/t/t3070-wildmatch.sh
@@ -10,10 +10,13 @@ match() {
test_expect_success "wildmatch: match '$3' '$4'" "
test-wildmatch wildmatch '$3' '$4'
"
- else
+ elif test "$1" = 0
+ then
test_expect_success "wildmatch: no match '$3' '$4'" "
! test-wildmatch wildmatch '$3' '$4'
"
+ else
+ test_expect_success "PANIC: Test framework error. Unknown
matches value $1" 'false'
fi
}
@@ -23,10 +26,13 @@ imatch() {
test_expect_success "iwildmatch: match '$2' '$3'" "
test-wildmatch iwildmatch '$2' '$3'
"
- else
+ elif test "$1" = 0
+ then
test_expect_success "iwildmatch: no match '$2' '$3'" "
! test-wildmatch iwildmatch '$2' '$3'
"
+ else
+ test_expect_success "PANIC: Test framework error. Unknown
matches value $1" 'false'
fi
}
@@ -36,10 +42,13 @@ pathmatch() {
test_expect_success "pathmatch: match '$2' '$3'" "
test-wildmatch pathmatch '$2' '$3'
"
- else
+ elif test "$1" = 0
+ then
test_expect_success "pathmatch: no match '$2' '$3'" "
! test-wildmatch pathmatch '$2' '$3'
"
+ else
+ test_expect_success "PANIC: Test framework error. Unknown
matches value $1" 'false'
fi
}
--
2.15.1.424.g9478a66081
[PATCH v2 2/7] wildmatch test: use more standard shell style
Change the wildmatch test to use more standard shell style, usually we
use "if test" not "if [".
Signed-off-by: Ævar Arnfjörð Bjarmason
---
t/t3070-wildmatch.sh | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
index 27fa878f6e..4d589d1f9a 100755
--- a/t/t3070-wildmatch.sh
+++ b/t/t3070-wildmatch.sh
@@ -5,7 +5,8 @@ test_description='wildmatch tests'
. ./test-lib.sh
match() {
- if [ $1 = 1 ]; then
+ if test "$1" = 1
+ then
test_expect_success "wildmatch: match '$3' '$4'" "
test-wildmatch wildmatch '$3' '$4'
"
@@ -17,7 +18,8 @@ match() {
}
imatch() {
- if [ $1 = 1 ]; then
+ if test "$1" = 1
+ then
test_expect_success "iwildmatch:match '$2' '$3'" "
test-wildmatch iwildmatch '$2' '$3'
"
@@ -29,7 +31,8 @@ imatch() {
}
pathmatch() {
- if [ $1 = 1 ]; then
+ if test "$1" = 1
+ then
test_expect_success "pathmatch: match '$2' '$3'" "
test-wildmatch pathmatch '$2' '$3'
"
--
2.15.1.424.g9478a66081
[PATCH v2 1/7] wildmatch test: indent with tabs, not spaces
Replace the 4-width mixed space & tab indentation in this file with
indentation with tabs as we do in most of the rest of our tests.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
t/t3070-wildmatch.sh | 54 ++--
1 file changed, 27 insertions(+), 27 deletions(-)
diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
index 163a14a1c2..27fa878f6e 100755
--- a/t/t3070-wildmatch.sh
+++ b/t/t3070-wildmatch.sh
@@ -5,39 +5,39 @@ test_description='wildmatch tests'
. ./test-lib.sh
match() {
-if [ $1 = 1 ]; then
- test_expect_success "wildmatch: match '$3' '$4'" "
- test-wildmatch wildmatch '$3' '$4'
- "
-else
- test_expect_success "wildmatch: no match '$3' '$4'" "
- ! test-wildmatch wildmatch '$3' '$4'
- "
-fi
+ if [ $1 = 1 ]; then
+ test_expect_success "wildmatch: match '$3' '$4'" "
+ test-wildmatch wildmatch '$3' '$4'
+ "
+ else
+ test_expect_success "wildmatch: no match '$3' '$4'" "
+ ! test-wildmatch wildmatch '$3' '$4'
+ "
+ fi
}
imatch() {
-if [ $1 = 1 ]; then
- test_expect_success "iwildmatch:match '$2' '$3'" "
- test-wildmatch iwildmatch '$2' '$3'
- "
-else
- test_expect_success "iwildmatch: no match '$2' '$3'" "
- ! test-wildmatch iwildmatch '$2' '$3'
- "
-fi
+ if [ $1 = 1 ]; then
+ test_expect_success "iwildmatch:match '$2' '$3'" "
+ test-wildmatch iwildmatch '$2' '$3'
+ "
+ else
+ test_expect_success "iwildmatch: no match '$2' '$3'" "
+ ! test-wildmatch iwildmatch '$2' '$3'
+ "
+ fi
}
pathmatch() {
-if [ $1 = 1 ]; then
- test_expect_success "pathmatch: match '$2' '$3'" "
- test-wildmatch pathmatch '$2' '$3'
- "
-else
- test_expect_success "pathmatch: no match '$2' '$3'" "
- ! test-wildmatch pathmatch '$2' '$3'
- "
-fi
+ if [ $1 = 1 ]; then
+ test_expect_success "pathmatch: match '$2' '$3'" "
+ test-wildmatch pathmatch '$2' '$3'
+ "
+ else
+ test_expect_success "pathmatch: no match '$2' '$3'" "
+ ! test-wildmatch pathmatch '$2' '$3'
+ "
+ fi
}
# Basic wildmat features
--
2.15.1.424.g9478a66081
Re: [RFC/PATCH] perl: bump the required Perl version to 5.10.0 from 5.8.0
On Sun, Dec 24 2017, Eric Wong jotted:
[Removed Petr Baudis from CC, his suse.cz E-Mail address
is bouncing]
> Ævar Arnfjörð Bjarmason wrote:
>> On Sun, Dec 24 2017, Jeff King jotted:
>> > As far as this actual perl change goes, I don't have a strong opinion. I
>> > agree it would be nice to eventually move forward, and your reasoning
>> > about what constitutes "old" seems sane. But we also don't write much
>> > perl in this project these days, and I don't see a lack of modern perl
>> > features causing a lot of headaches.
>
> Agreed.
>
>> Yes, unlike with the curl patches it's not a big PITA to maintain
>> compatibility with 5.8, it would just be easier to write new code &
>> maintain old code and not have to be on guard about not using features
>> one takes for grantend, and maintain compatibility with 5.8 versions of
>> core modules.
>
> As one of the more frequent Perl users here (even outside of
> git.git), I never considered using 5.10+ features at all until
> now. Maybe 5.8 compatibility is just too ingrained into me and
> much of the stuff I work on is old and ancient(*).
>
> That said, reading perl5100delta does reveal features such as
> defined-or and given/when that I might find useful; but I'm also
> not going to replace existing code to use new features unless
> there is a clear improvement.
>
> If there's new code people are developing using 5.10; I would
> not object at all. Otherwise, I don't see compatibility with
> 5.8 hurts more than it helps.
Aside from whatever we do here, I don't think this would be a good idea
& would introduce a lot of confusion for packagers, e.g. requiring one
version of OpenSSL for hashing, but another one for "imap-send", or one
version of curl for "push", and another for "imap-send".
I think for any given external dependency of git.git it makes sense to
just pick a version, not say that this script requires perl so-and-so,
this one python so-and-so, or curl/openssl so-and-so etc.
> Maybe we change our docs to say we welcome 5.10 features for new
> code, but I'm against changing things for the sake of change.
I should have mentioned this in the commit message, but for me it's
mainly that whenever I patch the Git perl code there's no easy way to
test if it works on a currently supported release, 5.8.* doesn't even
build anymore on a modern compiler without monkeypatching with
Devel::PatchPerl (and then only some subreleases).
I think it's reasonable for us, in general, to at some point pass the
buck in maintaining dependencies to people who want to still build on
ancient versions. And not just for perl, but e.g. curl too, which is
something I commented on at some length in <874ltg2tvo@gmail.com>
(https://public-inbox.org/git/874ltg2tvo@gmail.com/). I.e. if you
need to really build the latest git on RHEL 5 with all bells & whistles
you also build perl.
It's not just change for the sake of change, there's a high cognitive
overhead in trying to write code against the last 15 years of some
software as opposed to "just" 10 years (which is already bad enough).
Of course any one change isn't going to be what makes it or breaks it,
so it's hard to make the argument in terms of "I must use this new
feature here". But if that was the standard we were applying we'd still
be supporting perl 5.6[1].
1. If it hadn't turned out that it was broken for years because of using
a new feature, see d48b284183 ("perl: bump the required Perl version
to 5.8 from 5.6.[21]", 2010-09-24)
Re: [RFC/PATCH] perl: bump the required Perl version to 5.10.0 from 5.8.0
Ævar Arnfjörð Bjarmason wrote: > On Sun, Dec 24 2017, Jeff King jotted: > > As far as this actual perl change goes, I don't have a strong opinion. I > > agree it would be nice to eventually move forward, and your reasoning > > about what constitutes "old" seems sane. But we also don't write much > > perl in this project these days, and I don't see a lack of modern perl > > features causing a lot of headaches. Agreed. > Yes, unlike with the curl patches it's not a big PITA to maintain > compatibility with 5.8, it would just be easier to write new code & > maintain old code and not have to be on guard about not using features > one takes for grantend, and maintain compatibility with 5.8 versions of > core modules. As one of the more frequent Perl users here (even outside of git.git), I never considered using 5.10+ features at all until now. Maybe 5.8 compatibility is just too ingrained into me and much of the stuff I work on is old and ancient(*). That said, reading perl5100delta does reveal features such as defined-or and given/when that I might find useful; but I'm also not going to replace existing code to use new features unless there is a clear improvement. If there's new code people are developing using 5.10; I would not object at all. Otherwise, I don't see compatibility with 5.8 hurts more than it helps. Maybe we change our docs to say we welcome 5.10 features for new code, but I'm against changing things for the sake of change. (*) Like this 32-bit laptop from 2005. Only problem I have with it is the noisy fan.
Re: [PATCH] setup.c: move statement under condition
On Sun, Dec 24 2017, Kevin Daudt jotted:
> On Sun, Dec 24, 2017 at 12:15:35PM +0400, Vadim Petrov wrote:
>> Thank you for your replay.
>>
>> > I have to be honest: this commit message (including the subject) left me
>> > quite puzzled as to the intent of this patch.
>>
>> I still only learn English and correctly express my thoughts while somewhat
>> difficult.
>>
>> > If you also have a background story that motivated you to work on this
>> > patch (for example, if you hit a huge performance bottleneck with some
>> > tool that fed thousands of absolute paths to Git that needed to be turned
>> > into paths relative to the worktree's top-level directory), I would
>> > definitely put that into the commit message, too, if I were you.
>>
>> I have no such reason. I just saw it and wanted to change it.
>
> A commit message contains the reason why this is a good change to make.
> It lets others know what problems it's trying to solve or what usecase
> it tries to satisfy.
>
> The commit message basically needs to convince others why the change is
> necessary / desired, now, and in the future.
>
> This will help others to follow your thought process and it gives you
> the possiblity to communicate trade-offs you made, all which cannot
> inferred from the patch.
>
> For simple changes, the motivation can be simple too.
...and a good example would be 6127ff63cf which introduced this logic
Vadim is trying to change, i.e. does this still retain the fix for
whatever issue that was trying to address?
It's also good to CC the people who've directly worked on the code
you're changing in the past, I've CC'd Martin.
> To make it concrete: You are talking about a condition. What condition?
> And you say that the previously obtained value will not be necessary.
> What do you do with that value then? Why does this change improve the
> situation?
>
> These are things you can state in your commit message.
>
> Hope this helps, Kevin
>
>> > Up until recently, we encouraged dropping the curly brackets from
>> > single-line statements, but apparently that changed. It is now no longer
>> > clear, and often left to the taste of the contributor. But not always.
>> > Sometimes we start a beautiful thread discussion the pros and cons of
>> > curly brackets in the middle of patch review, and drop altogether
>> > reviewing the actual patch.
>>
>> I was guided by the rule from the Documentation/CodingGuidelines:
>> When there are multiple arms to a conditional and some of them
>> require braces, enclose even a single line block in braces for
>> consistency.
>> And other code from setup.c:
>> from function get_common_dir:
>> if (!has_common) {
>> /* several commands */
>> } else {
>> free(candidate->work_tree);
>> }
>> from function get_common_dir_noenv:
>> if (file_exists(path.buf)) {
>> /* several commands */
>> } else {
>> strbuf_addstr(sb, gitdir);
>> }
>>
>> > In short: I think your patch does the right thing, and I hope that you
>> > find my suggestions to improve the patch useful.
>>
>> I fixed the patch according to your suggestions.
>>
>>
>> Signed-off-by: Vadim Petrov
>> ---
>> setup.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/setup.c b/setup.c
>> index 8cc34186c..1a414c256 100644
>> --- a/setup.c
>> +++ b/setup.c
>> @@ -27,26 +27,26 @@ static int abspath_part_inside_repo(char *path)
>> {
>> size_t len;
>> size_t wtlen;
>> char *path0;
>> int off;
>> const char *work_tree = get_git_work_tree();
>>
>> if (!work_tree)
>> return -1;
>> wtlen = strlen(work_tree);
>> len = strlen(path);
>> -off = offset_1st_component(path);
>>
>> -/* check if work tree is already the prefix */
>> -if (wtlen <= len && !strncmp(path, work_tree, wtlen)) {
>> +if (wtlen > len || strncmp(path, work_tree, wtlen))
>> +off = offset_1st_component(path);
>> +else { /* check if work tree is already the prefix */
>> if (path[wtlen] == '/') {
>> memmove(path, path + wtlen + 1, len - wtlen);
>> return 0;
>> } else if (path[wtlen - 1] == '/' || path[wtlen] == '\0') {
>> /* work tree is the root, or the whole path */
>> memmove(path, path + wtlen, len - wtlen + 1);
>> return 0;
>> }
>> /* work tree might match beginning of a symlink to work tree */
>> off = wtlen;
>> }
>> --
>> 2.15.1.433.g936d1b989
Re: [PATCH] setup.c: move statement under condition
On Sun, Dec 24, 2017 at 12:15:35PM +0400, Vadim Petrov wrote:
> Thank you for your replay.
>
> > I have to be honest: this commit message (including the subject) left me
> > quite puzzled as to the intent of this patch.
>
> I still only learn English and correctly express my thoughts while somewhat
> difficult.
>
> > If you also have a background story that motivated you to work on this
> > patch (for example, if you hit a huge performance bottleneck with some
> > tool that fed thousands of absolute paths to Git that needed to be turned
> > into paths relative to the worktree's top-level directory), I would
> > definitely put that into the commit message, too, if I were you.
>
> I have no such reason. I just saw it and wanted to change it.
A commit message contains the reason why this is a good change to make.
It lets others know what problems it's trying to solve or what usecase
it tries to satisfy.
The commit message basically needs to convince others why the change is
necessary / desired, now, and in the future.
This will help others to follow your thought process and it gives you
the possiblity to communicate trade-offs you made, all which cannot
inferred from the patch.
For simple changes, the motivation can be simple too.
To make it concrete: You are talking about a condition. What condition?
And you say that the previously obtained value will not be necessary.
What do you do with that value then? Why does this change improve the
situation?
These are things you can state in your commit message.
Hope this helps, Kevin
> > Up until recently, we encouraged dropping the curly brackets from
> > single-line statements, but apparently that changed. It is now no longer
> > clear, and often left to the taste of the contributor. But not always.
> > Sometimes we start a beautiful thread discussion the pros and cons of
> > curly brackets in the middle of patch review, and drop altogether
> > reviewing the actual patch.
>
> I was guided by the rule from the Documentation/CodingGuidelines:
> When there are multiple arms to a conditional and some of them
> require braces, enclose even a single line block in braces for
> consistency.
> And other code from setup.c:
> from function get_common_dir:
> if (!has_common) {
> /* several commands */
> } else {
> free(candidate->work_tree);
> }
> from function get_common_dir_noenv:
> if (file_exists(path.buf)) {
> /* several commands */
> } else {
> strbuf_addstr(sb, gitdir);
> }
>
> > In short: I think your patch does the right thing, and I hope that you
> > find my suggestions to improve the patch useful.
>
> I fixed the patch according to your suggestions.
>
>
> Signed-off-by: Vadim Petrov
> ---
> setup.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/setup.c b/setup.c
> index 8cc34186c..1a414c256 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -27,26 +27,26 @@ static int abspath_part_inside_repo(char *path)
> {
> size_t len;
> size_t wtlen;
> char *path0;
> int off;
> const char *work_tree = get_git_work_tree();
>
> if (!work_tree)
> return -1;
> wtlen = strlen(work_tree);
> len = strlen(path);
> - off = offset_1st_component(path);
>
> - /* check if work tree is already the prefix */
> - if (wtlen <= len && !strncmp(path, work_tree, wtlen)) {
> + if (wtlen > len || strncmp(path, work_tree, wtlen))
> + off = offset_1st_component(path);
> + else { /* check if work tree is already the prefix */
> if (path[wtlen] == '/') {
> memmove(path, path + wtlen + 1, len - wtlen);
> return 0;
> } else if (path[wtlen - 1] == '/' || path[wtlen] == '\0') {
> /* work tree is the root, or the whole path */
> memmove(path, path + wtlen, len - wtlen + 1);
> return 0;
> }
> /* work tree might match beginning of a symlink to work tree */
> off = wtlen;
> }
> --
> 2.15.1.433.g936d1b989
Re: [RFC/PATCH] perl: bump the required Perl version to 5.10.0 from 5.8.0
On Sun, Dec 24 2017, Jeff King jotted: > On Sat, Dec 23, 2017 at 05:44:00PM +, Ævar Arnfjörð Bjarmason wrote: > >> This is similar to Jeff King's jk/drop-ancient-curl series in that >> we're dropping perl releases that are rarely tested anymore, however >> unlike those patches git still works on e.g. 5.8.8 (I couldn't build >> anything older). > > Heh, I'm not sure if those are the best prior art to justify this, since > I stopped posting them after getting complaints (though I'll admit I was > considering re-posting them since AFAICT nobody has stepped up to fix > the breakage after many months). Less of a justification, more of a "this is going to be a similar sort of dumpster fire" :) > This may be more like the recent C99 weather-balloon patches, in that > we're not using the new features yet, but want to see if anybody screams > at this first change. > >> The reason to do this is to be able to use features released with perl >> in the last decade, 5.10 was a major feature release including things >> like new regex features, state variables, the defined-or operator >> etc.[3] >> >> I expect this to be more controversial as since the 5.8 release stayed >> along for longer in various distributions, e.g. it's the version >> shipped with RHEL 5, replaced by 5.10 in RHEL 6 released in late 2010, >> similarly the first Debian release to include 5.10 was 5.0 (Lenny) >> released in early 2009. The release history for other distributions >> can be seen on CPAN's "Perl Binaries" page[3]. > > As far as this actual perl change goes, I don't have a strong opinion. I > agree it would be nice to eventually move forward, and your reasoning > about what constitutes "old" seems sane. But we also don't write much > perl in this project these days, and I don't see a lack of modern perl > features causing a lot of headaches. Yes, unlike with the curl patches it's not a big PITA to maintain compatibility with 5.8, it would just be easier to write new code & maintain old code and not have to be on guard about not using features one takes for grantend, and maintain compatibility with 5.8 versions of core modules.
RE: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())
On December 24, 2017 9:54 AM, Jeff King wrote: > Subject: Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor > out rewrite_file()) > > On Sat, Nov 18, 2017 at 10:01:45AM +0100, Johannes Sixt wrote: > > > > Yeah, I have mixed feelings on that. I think it does make the > > > control flow less clear. At the same time, what I found was that > > > handlers like die/ignore/warn were the thing that gave the most > > > reduction in complexity in the callers. > > > > Would you not consider switching over to C++? With exceptions, you get > > the error context without cluttering the API. (Did I mention that > > librarification would become a breeze? Do not die in library routines: > > not a problem anymore, just catch the exception. die_on_error > > parameters? Not needed anymore. Not to mention that resource leaks > > would be much, MUCH simpler to treat.) > > I threw this email on my todo pile since I was traveling when it came, but I > think it deserves a response (albeit quite late). > > It's been a long while since I've done any serious C++, but I did really like > the > RAII pattern coupled with exceptions. That said, I think it's dangerous to do > it > half-way, and especially to retrofit an existing code base. It introduces a > whole new control-flow pattern that is invisible to the existing code, so > you're going to get leaks and variables in unexpected states whenever you > see an exception. > > I also suspect there'd be a fair bit of in converting the existing code to > something that actually compiles as C++. > > So if we were starting the project from scratch and thinking about using > C++ with RAII and exceptions, sure, that's something I'd entertain[1] > (and maybe even Linus has softened on his opinion of C++ these days ;) ). > But at this point, it doesn't seem like the tradeoff for switching is there. While I'm a huge fan of OO, you really need a solid justification for going there, and a good study of your target audience for Open Source C++. My comments are based on porting experience outside of Linux/Windows: 1. Conversion to C++ just to pick up exceptions is a lot like "One does not simply walk to Mordor", as Peff hinted at above. 2. Moving to C++ generally involves a **complete** redesign. While Command Patterns (and and...) may be very helpful in one level, the current git code base is very procedural in nature. 3. From a porting perspective, applications written in with C++ generally (there are exceptions) are significantly harder than C. The POSIX APIs are older and more broadly supported/emulated than what is available in C++. Once you start getting into "my favourite C++ library is...", or "version2 or version3", or smart pointers vs. scope allocation, things get pretty argumentative. It is (arguably) much easier to disable a section of code that won't function on a platform in C without having to rework an OO model, making subsequent merges pretty much impossible and the port unsustainable. That is unless the overall design really factors in platform differences right into the OO model from the beginning of incubation. 4. I really hate making these points because I am an OO "fanspert", just not when doing portable code. Even in java, which is more port-stable than C++ (arguably, but in my experience), you tend to hit platform library differences than can invalidate ports. My take is "oh my please don't go there" for the git project, for a component that has become/is becoming required core infrastructure for so many platforms. With Respect, Randall -- Brief whoami: NonStop&UNIX developer since approximately UNIX(421664400)/NonStop(2112884442) -- In my real life, I talk too much.
Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())
On Sat, Nov 18, 2017 at 10:01:45AM +0100, Johannes Sixt wrote: > > Yeah, I have mixed feelings on that. I think it does make the control > > flow less clear. At the same time, what I found was that handlers like > > die/ignore/warn were the thing that gave the most reduction in > > complexity in the callers. > > Would you not consider switching over to C++? With exceptions, you get the > error context without cluttering the API. (Did I mention that > librarification would become a breeze? Do not die in library routines: not a > problem anymore, just catch the exception. die_on_error parameters? Not > needed anymore. Not to mention that resource leaks would be much, MUCH > simpler to treat.) I threw this email on my todo pile since I was traveling when it came, but I think it deserves a response (albeit quite late). It's been a long while since I've done any serious C++, but I did really like the RAII pattern coupled with exceptions. That said, I think it's dangerous to do it half-way, and especially to retrofit an existing code base. It introduces a whole new control-flow pattern that is invisible to the existing code, so you're going to get leaks and variables in unexpected states whenever you see an exception. I also suspect there'd be a fair bit of in converting the existing code to something that actually compiles as C++. So if we were starting the project from scratch and thinking about using C++ with RAII and exceptions, sure, that's something I'd entertain[1] (and maybe even Linus has softened on his opinion of C++ these days ;) ). But at this point, it doesn't seem like the tradeoff for switching is there. -Peff [1] I'd also consider Rust, though I'm not too experienced with it myself.
Re: [RFC/PATCH] perl: bump the required Perl version to 5.10.0 from 5.8.0
On Sat, Dec 23, 2017 at 05:44:00PM +, Ævar Arnfjörð Bjarmason wrote: > This is similar to Jeff King's jk/drop-ancient-curl series in that > we're dropping perl releases that are rarely tested anymore, however > unlike those patches git still works on e.g. 5.8.8 (I couldn't build > anything older). Heh, I'm not sure if those are the best prior art to justify this, since I stopped posting them after getting complaints (though I'll admit I was considering re-posting them since AFAICT nobody has stepped up to fix the breakage after many months). This may be more like the recent C99 weather-balloon patches, in that we're not using the new features yet, but want to see if anybody screams at this first change. > The reason to do this is to be able to use features released with perl > in the last decade, 5.10 was a major feature release including things > like new regex features, state variables, the defined-or operator > etc.[3] > > I expect this to be more controversial as since the 5.8 release stayed > along for longer in various distributions, e.g. it's the version > shipped with RHEL 5, replaced by 5.10 in RHEL 6 released in late 2010, > similarly the first Debian release to include 5.10 was 5.0 (Lenny) > released in early 2009. The release history for other distributions > can be seen on CPAN's "Perl Binaries" page[3]. As far as this actual perl change goes, I don't have a strong opinion. I agree it would be nice to eventually move forward, and your reasoning about what constitutes "old" seems sane. But we also don't write much perl in this project these days, and I don't see a lack of modern perl features causing a lot of headaches. -Peff
Re: [PATCH v2 1/5] core.aheadbehind: add new config setting
On Fri, Dec 22, 2017 at 10:21:23AM -0800, Junio C Hamano wrote: > >> +core.aheadbehind:: > >> + If true, tells commands like status and branch to print ahead and > >> + behind counts for the branch relative to its upstream branch. > >> + This computation may be very expensive when there is a great > >> + distance between the two branches. If false, these commands > >> + only print that the two branches refer to different commits. > >> + Defaults to true. > > > > This doesn't seem like a particularly core feature to me. Should it be > > e.g. status.aheadbehind (even though it also affects "git branch") or > > even something like diff.aheadbehind? I'm not sure. > > FWIW, I do not think it is core at all, either; sorry for not > anticipating that a wrong name will be picked without a proper > guidance when I saw the "not limited to status" mentioned in the > discussion, but I was sick and offline for a few days, so... I, too, had a funny feeling about calling this "core". But I didn't have a better name, as I'm not sure what other place we have for config options that cross many command boundaries. "diff" and "status" don't seem quite right to me. While you can argue they are subsystems, it seems too easy for users to confuse them with the commands of the same names. Maybe there should be a "ui.*" config hierarchy for these kinds of cross-command interface options? > > I also wonder if there's a way to achieve the same benefit without > > having it be configurable. E.g. if a branch is way behind, couldn't > > we terminate the walk early to get the same bounded cost per branch > > without requiring configuration? > > Hmm, that is an interesting thought. Yes, it is. Two thoughts: - It probably doesn't let us punt on the config naming, because we'd probably still want a knob for "how much work". - I wondered if we could give a better answer than "these two are different" based on a partial walk. But certainly not in the general case. E.g., imagine: ... -- master -- A -- B -- ... -- Y -- Z -- origin/master If we walk back from origin/master and give up somewhere in the middle, we can't say anything intelligent about the relationship. -Peff
Re: [PATCH] revision: introduce prepare_revision_walk_extended()
On Thu, Dec 21, 2017 at 07:41:44PM +0100, René Scharfe wrote: > Am 20.12.2017 um 14:08 schrieb Jeff King: > > On Tue, Dec 19, 2017 at 10:33:55AM -0800, Junio C Hamano wrote: > > > >> Should we take the patch posted as-is and move forward? > > > > I suppose so. I don't really have anything against the patch. My main > > complaint was just that I don't think it's actually cleaning up the > > gross parts of the interface. It's just substituting one gross thing (a > > funny struct flag) for another (a special version of the prepare > > function that takes a funny out-parameter). > > If this is a fair description (and I have to admit that it is) then I > don't understand why you aren't against the patch. Let's drop it. Heh, I almost wrote more on this. My thinking was two-fold: - while I think the fundamental gross-ness is still there after your patch, it does smooth some of the rough edges. So it's a slight improvement over the status quo. - I read an article a while back (which unfortunately I can't find now) about the idea of "default to yes" in open source. I.e., the idea that if somebody bothered to cook up a patch and there is no good reason to reject it, you should take it. Certainly there are cases where that can go too far: obvious ones like bad ideas where it is not really "default" anymore, but also subtle ones where the changes are code churn whose fallouts will be dealt with by others. But this patch is a good example. I'm not convinced it makes things better, but I don't think it makes things worse. If you, who looked a lot closer at the problem than I did, still think it's a good idea after our discussion, it's probably worth applying. So my comments weren't really "this is a bad idea" as much as "is there a better idea". We didn't come up with one after some discussion, and I'm willing to let it go there for now. But if you want to keep thinking on it, I'm game. ;) > Can we do better? How about something this? It reverts bundle to > duplicate the object_array, but creates just a commit_list in the other > two cases. As a side-effect this allows clearing flags for all entries > with a single traversal. I think this is basically the "copy it before-hand" thing from earlier in the thread. Switching to just keeping commits seems like a nice change. It's an easy (if minor) optimization, and it makes clear exactly which part of the data we need to keep around. The single-traversal thing I suspect doesn't matter much in practice. In both cases if we would visit commit X twice, we'd immediately see on the second visit that it has already been cleared and not do anymore work. Side note: Another question is whether it would simply be faster to clear the flags for _all_ objects that we've touched in the current process (we have clear_object_flags() for this already). Then we know that we touch each one once, and we as a bonus we don't even have to keep the previous tips. The downsides are: - if another traversal in the process looked at many objects, but our current traversal looked at few, then we would examine more objects than we need to (albeit with lower cost per object) - it's possible there's another traversal in the same process whose flags we would want to have saved. I suspect such a setup is broken already, though, unless there's a guarantee that the two traversals don't overlap. So anyway, I think this is a reasonable approach, unless we're really worried about the extra O(# of pending) allocation. -Peff
Re: Bring together merge and rebase
On Dec 24, 2017, at 01:01, Johannes Schindelin wrote: > > Hi Carl, > > On Sat, 23 Dec 2017, Carl Baldwin wrote: > >> I imagine that a "git commit --amend" would also insert a "replaces" >> reference to the original commit but I failed to mention that in my >> original post. > > And cherry-pick, too, of course. Why would it? In my mind, cherry-picking does not 'replace' or 'refine' commits, it copies them into other, unrelated branches (usually something like stable branches maintained separately from the mainline). If anything, cherry-pick could add a separate "cherry-picked from" reference which may be useful, I guess, for conflict resolution if two branches with the same commit are merged. > Of course, that is only my wish, other users in similar situations may > want that information. Demonstrating that you would be better served with > an opt-in feature that uses notes rather than a baked-in commit header. Using notes also allows to test and evaluate this new feature without any changes to core git, using it as an extension at first.
GREETINGS,
GREETINGS, I AM BORTE ,I WAS DIAGNOSE OF OVARIAN CANCER,WHICH DOCTORS HAVE CONFIRMED THAT I HAVE ONLY FEW WEEKS TO LIVE, SO I HAVE DECIDED TO DONATE EVERYTHING I HAVE TO THE ORPHANAGE AND THE POOR WIDOWS THROUGH YOU .PLEASE KINDLY REPLY ME ONLY ON MY EMAIL ADDRES HERE (missbrtoeogo...@gmail.com) AS SOON AS POSIBLE TO ENABLE ME GIVE YOU MORE INFORMATION ABOUT MYSELF AND HOW TO GO ABOUT IT . THANKS MISS BORTE
Re: Re: Unify annotated and non-annotated tags
From: "anatoly techtonik"
From: Philip Oakley
> So if I understand correctly, the hope is that `git show-ref --tags`
> could
> get an alternate option `--all-tags` [proper option name required...]
> such
> that the user would not have to develop the rather over the complicated
> expression that used a newish capability of a different command.
> Would that be right?
That's correct.
> Or at least update the man page docs to clarify the annotated vs
> non-annotated tags issue (many SO questions!).
Are there stats how many users read man pages and what is their
reading session length? I mean docs may not help much,
The "reading the manual" question is fairly well answered in the Human Error
literature in terms of clarity and effectiveness, and the normal human error
rates (for interest search for "Panko" "Spreadsheet errors" [1]). Typical
human error rate is 1%. Most pilot error ends up being, in part, caused by
confusing / incomplete manuals (i.e. we fail to support them).
If the manuals are the peak of perfection then they are well visited and the
supporting material is usually good. If manuals are a sprawling upland with
bogs, fissure, islands of inaccessability, then they are rarely used.
Git does suffer from having a lot of separate commands, which makes seeing
the woods for the trees difficult sometimes, especially as its core concepts
are not always well understood.
Improving the manuals (as reference material) will always help, even if the
trickle down effect is slow (made worse by alternate sources of error -
Stackoverflow and blogs... ;-)
> And indicate if the --dereference and/or --hash options would do the
> trick!
> - maybe the "^{}" appended would be part of the problem (and need that
> new
> option "--objectreference" ).
--dereference would work if it didn't require extra processing.
It is hard to think about other option name that would give
desired result.
---
anatoly t.
--
Philip
[1] https://arxiv.org/abs/1602.02601 https://arxiv.org/pdf/1602.02601
"This paper reviews human cognition processes and shows first that humans
cannot be error free no matter how hard they try, and second that our
intuition about errors and how we can reduce them is based on appallingly
bad knowledge."
Re: [RFC/PATCH] perl: bump the required Perl version to 5.10.0 from 5.8.0
On Sat, Dec 23 2017, brian m. carlson jotted: > On Sat, Dec 23, 2017 at 05:44:00PM +, Ævar Arnfjörð Bjarmason wrote: >> The reason to do this is to be able to use features released with perl >> in the last decade, 5.10 was a major feature release including things >> like new regex features, state variables, the defined-or operator >> etc.[3] >> >> I expect this to be more controversial as since the 5.8 release stayed >> along for longer in various distributions, e.g. it's the version >> shipped with RHEL 5, replaced by 5.10 in RHEL 6 released in late 2010, >> similarly the first Debian release to include 5.10 was 5.0 (Lenny) >> released in early 2009. The release history for other distributions >> can be seen on CPAN's "Perl Binaries" page[3]. > > This is fine by me. As far as I know, 5.10.1 is the oldest version of > Perl still security-supported by a major Linux vendor. > > Feature-wise, the release I'd much rather see is 5.14, since it provides > the r modifier to s/// and tr/// and undef-transparent length, but that > simply won't be possible until RHEL 6 and CentOS 6 go EOL. Upgrading to > 5.10 is better than nothing, and it does get us defined-or, which is one > of the only 5.10 features I ever see used. Indeed, but as you point out it's not going to happen for some time given the burden we can reasonably place on downstream packagers. > I'm curious, though, is there some reason you went with the "v5.10.0" > syntax other than "5.010"? I believe the latter provides a better error > message on older Perls, although I agree the former is more readable. It would only provide confusing errors on 5.6 and older, which as noted we haven't supported at all since before 2010, so people are unlikely to be running it. I'll note why I did that in a non-RFC commit message, FWIW this wording was also confusing in perl's own documentation, which I fixed the other day: https://github.com/Perl/perl5/commit/f1546a83e7
Re: [PATCH 6/6] wildmatch test: create & test files on disk in addition to in-memory
Am 24.12.2017 um 12:06 schrieb Ævar Arnfjörð Bjarmason: On Sun, Dec 24 2017, Johannes Sixt jotted: Am 23.12.2017 um 22:30 schrieb Ævar Arnfjörð Bjarmason: + printf '%s' '$text' >expect && There are no single-quotes in any $text instances, right? There's not, but maybe we should be more careful here and use here-docs. Unless it is essential to test the single-quote case, we need not complicate the code. I just wanted to rise awareness. If a problematic test case is introduced, it will be noticed soon enough. It's not that we deal with unknown input here. -- Hannes
Re: [PATCH 6/6] wildmatch test: create & test files on disk in addition to in-memory
On Sun, Dec 24 2017, Johannes Sixt jotted:
> Am 23.12.2017 um 22:30 schrieb Ævar Arnfjörð Bjarmason:
>> Signed-off-by: Ævar Arnfjörð Bjarmason
>> ---
>> a[]b | 0
>> t/t3070-wildmatch.sh | 336
>> ---
>> 2 files changed, 319 insertions(+), 17 deletions(-)
>> create mode 100644 a[]b
>>
>> diff --git a/a[]b b/a[]b
>> new file mode 100644
>> index 00..e69de29bb2
>
> A big no-no! This file can't be created on Windows!
Urgh, that was a mistake of mine. Will be gone in v2.
>> diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
>> index 47b479e423..d423bb01f3 100755
>> --- a/t/t3070-wildmatch.sh
>> +++ b/t/t3070-wildmatch.sh
>> @@ -4,31 +4,146 @@ test_description='wildmatch tests'
>>
>> . ./test-lib.sh
>>
>> +create_test_file() {
>> +file=$1
>> +
>> +# `touch .` will succeed but obviously not do what we intend
>> +# here.
>> +test "$file" = "." && return 1
>> +# We cannot create a file with an empty filename.
>> +test "$file" = "" && return 1
>> +# The tests that are testing that e.g. foo//bar is matched by
>> +# foo/*/bar can't be tested on filesystems since there's no
>> +# way we're getting a double slash.
>> +echo "$file" | grep -q -F '//' && return 1
>> +# When testing the difference between foo/bar and foo/bar/ we
>> +# can't test the latter.
>> +echo "$file" | grep -q -E '/$' && return 1
>> +
>> +dirs=$(echo "$file" | sed -r 's!/[^/]+$!!')
>
> Booh! Booh! So many fork()s! ;)
>
> case "$file" in
> *//*)
> # The tests that are testing that e.g. foo//bar is matched by
> # foo/*/bar can't be tested on filesystems since there's no
> # way we're getting a double slash.
> return 1;;
> */)
> # When testing the difference between foo/bar and foo/bar/ we
> # can't test the latter.
> return 1;;
> esac
>
> dirs=${file%/*}
Thanks. Will fix.
>> +
>> +# We touch "./$file" instead of "$file" because even an
>> +# escaped "touch -- -" means something different.
>> +if test "$file" != "$dirs"
>> +then
>> +mkdir -p -- "$dirs" 2>/dev/null &&
>> +touch -- "./$file" 2>/dev/null &&
>> +return 0
>> +else
>> +touch -- "./$file" 2>/dev/null &&
>> +return 0
>> +fi
>> +return 1
>> +}
>> +
>> wildtest() {
>> -match_w_glob=$1
>> -match_w_globi=$2
>> -match_w_pathmatch=$3
>> -match_w_pathmatchi=$4
>> -text=$5
>> -pattern=$6
>> +if test "$#" = 6
>> +then
>> +# When test-wildmatch and git ls-files produce the same
>> +# result.
>> +match_w_glob=$1
>> +match_f_w_glob=$match_w_glob
>> +match_w_globi=$2
>> +match_f_w_globi=$match_w_globi
>> +match_w_pathmatch=$3
>> +match_f_w_pathmatch=$match_w_pathmatch
>> +match_w_pathmatchi=$4
>> +match_f_w_pathmatchi=$match_w_pathmatchi
>> +text=$5
>> +pattern=$6
>> +elif test "$#" = 10
>> +then
>> +match_w_glob=$1
>> +match_w_globi=$2
>> +match_w_pathmatch=$3
>> +match_w_pathmatchi=$4
>> +match_f_w_glob=$5
>> +match_f_w_globi=$6
>> +match_f_w_pathmatch=$7
>> +match_f_w_pathmatchi=$8
>> +text=$9
>> +pattern=$10
>> +fi
>>
>> +# $1: Case sensitive glob match: test-wildmatch
>> if test "$match_w_glob" = 1
>> then
>> -test_expect_success "wildmatch: match '$text' '$pattern'" "
>> +test_expect_success "wildmatch: match '$text' '$pattern'" "
>> test-wildmatch wildmatch '$text' '$pattern'
>> "
>> elif test "$match_w_glob" = 0
>> then
>> -test_expect_success "wildmatch: no match '$text' '$pattern'" "
>> +test_expect_success "wildmatch: no match '$text' '$pattern'" "
>> ! test-wildmatch wildmatch '$text' '$pattern'
>> "
>> else
>> test_expect_success "PANIC: Test framework error. Unknown
>> matches value $match_w_glob" 'false'
>
> I think you can write this as 'say ...; exit 1'. See t*.
Thanks. Didn't see an existing idiom for this, will use that.
>> fi
>>
>> +# $1: Case sensitive glob match: ls-files
>> +if test "$match_f_w_glob" = 'E'
>> +then
>> +if create_test_file "$text"
>> +then
>> +test_expect_success "wildmatch(ls): match dies on
>> '$pattern' '$text'" "
>> +test_when_finished \"
>> +rm -rf -- * &&
>
> Can we be a bit more careful with this rm -rf, please?
> There is only one similarly loose case in t/t7003-filter-branch.sh,
> and it is outside test_
[PATCH 1/2] fast-export: ensure proper order of modify, copy and rename entries
From: Mark Nauwelaerts
As b3e8ca8 ("fast-export: do not copy from modified file", 2017-09-20)
indicates, if a commit both modifies a file and uses it as a source for a
copy, then the specifications of git-fast-import require that the copy
should be reported first followed by the modification to the source file.
The commit above addressed the problem by never reporting the copy. However,
the copy can still be reported if the entries are sorted properly. That
can be achieved by adjusting the order of the sort that is performed anyway
for other reasons of consistency. This is merely an extra order condition.
Furthermore, when using fast-export to export or bridge to another
version control system which explicitly tracks copies, then the 'C' commands
in the output are quite useful and necessary.
Signed-off-by: Mark Nauwelaerts
---
builtin/fast-export.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 2fb60d6..1b3e250 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -262,6 +262,14 @@ static void export_blob(const struct object_id *oid)
free(buf);
}
+static int order_status(const char status)
+{
+ if (status == 'R')
+ return 2;
+ else
+ return status == 'M' ? 1 : 0;
+}
+
static int depth_first(const void *a_, const void *b_)
{
const struct diff_filepair *a = *((const struct diff_filepair **)a_);
@@ -288,8 +296,12 @@ static int depth_first(const void *a_, const void *b_)
* Move 'R'ename entries last so that all references of the file
* appear in the output before it is renamed (e.g., when a file
* was copied and renamed in the same commit).
+* Moreover, 'C' needs to go before 'M' if the file was copied
+* and then modified a bit, as it should be done that way as well
+* at import time (also recall that 'C' is calculated on the
+* original content).
*/
- return (a->status == 'R') - (b->status == 'R');
+ return order_status(a->status) - order_status(b->status);
}
static void print_path_1(const char *path)
--
2.7.4
[PATCH 0/2] restore fast-export full filecopy detection
From: Mark Nauwelaerts When using fast-export/fast-import to interface/bridge with another VCS that explicitly tracks copy/rename as metadata, fast-export's ability to report filecopy/filerename is quite useful (if not essential). There has been a fix in this area recently with as side-effect that in some scenarios a filecopy is no longer reported as such. These few patches provide an alternative fix for the original problem while still retaining previous fast-export's filecopy reporting. See patches for additional details and commit references. Mark Nauwelaerts (2): fast-export: ensure proper order of modify, copy and rename entries fast-export: remove now obsolete filtering of modified files builtin/fast-export.c | 60 +++--- t/t9350-fast-export.sh | 2 +- 2 files changed, 28 insertions(+), 34 deletions(-) -- 2.7.4
[PATCH 2/2] fast-export: remove now obsolete filtering of modified files
From: Mark Nauwelaerts
Revert b3e8ca8 ("fast-export: do not copy from modified file", 2017-09-20)
partially, since it is not necessary to filter out 'C' commands if the 'M'
and 'C' commands are in correct order.
The unit test that was added is kept around though, and it still passes
with this new approach.
Signed-off-by: Mark Nauwelaerts
---
builtin/fast-export.c | 46 ++
t/t9350-fast-export.sh | 2 +-
2 files changed, 15 insertions(+), 33 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 1b3e250..6aa4073 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -356,7 +356,6 @@ static void show_filemodify(struct diff_queue_struct *q,
struct diff_options *options, void *data)
{
int i;
- struct string_list *changed = data;
/*
* Handle files below a directory first, in case they are all deleted
@@ -372,31 +371,20 @@ static void show_filemodify(struct diff_queue_struct *q,
case DIFF_STATUS_DELETED:
printf("D ");
print_path(spec->path);
- string_list_insert(changed, spec->path);
putchar('\n');
break;
case DIFF_STATUS_COPIED:
case DIFF_STATUS_RENAMED:
- /*
-* If a change in the file corresponding to ospec->path
-* has been observed, we cannot trust its contents
-* because the diff is calculated based on the prior
-* contents, not the current contents. So, declare a
-* copy or rename only if there was no change observed.
-*/
- if (!string_list_has_string(changed, ospec->path)) {
- printf("%c ", q->queue[i]->status);
- print_path(ospec->path);
- putchar(' ');
- print_path(spec->path);
- string_list_insert(changed, spec->path);
- putchar('\n');
-
- if (!oidcmp(&ospec->oid, &spec->oid) &&
- ospec->mode == spec->mode)
- break;
- }
+ printf("%c ", q->queue[i]->status);
+ print_path(ospec->path);
+ putchar(' ');
+ print_path(spec->path);
+ putchar('\n');
+
+ if (!oidcmp(&ospec->oid, &spec->oid) &&
+ ospec->mode == spec->mode)
+ break;
/* fallthrough */
case DIFF_STATUS_TYPE_CHANGED:
@@ -417,7 +405,6 @@ static void show_filemodify(struct diff_queue_struct *q,
get_object_mark(object));
}
print_path(spec->path);
- string_list_insert(changed, spec->path);
putchar('\n');
break;
@@ -553,8 +540,7 @@ static void anonymize_ident_line(const char **beg, const
char **end)
*end = out->buf + out->len;
}
-static void handle_commit(struct commit *commit, struct rev_info *rev,
- struct string_list *paths_of_changed_objects)
+static void handle_commit(struct commit *commit, struct rev_info *rev)
{
int saved_output_format = rev->diffopt.output_format;
const char *commit_buffer;
@@ -641,7 +627,6 @@ static void handle_commit(struct commit *commit, struct
rev_info *rev,
if (full_tree)
printf("deleteall\n");
log_tree_diff_flush(rev);
- string_list_clear(paths_of_changed_objects, 0);
rev->diffopt.output_format = saved_output_format;
printf("\n");
@@ -657,15 +642,14 @@ static void *anonymize_tag(const void *old, size_t *len)
return strbuf_detach(&out, len);
}
-static void handle_tail(struct object_array *commits, struct rev_info *revs,
- struct string_list *paths_of_changed_objects)
+static void handle_tail(struct object_array *commits, struct rev_info *revs)
{
struct commit *commit;
while (commits->nr) {
commit = (struct commit *)object_array_pop(commits);
if (has_unshown_parent(commit))
return;
- handle_commit(commit, revs, paths_of_changed_objects);
+ handle_commit(commit, revs);
}
}
@@ -1004,7 +988,6 @@ int cmd_fast_export(int argc, const char **argv, const
char *prefix)
char *export_filename = NULL, *import_filename = NULL;
uint32_t lastimportid;
struct string_list refspecs_list = STRI
Re: [PATCH 6/6] wildmatch test: create & test files on disk in addition to in-memory
Am 23.12.2017 um 22:30 schrieb Ævar Arnfjörð Bjarmason:
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
> a[]b | 0
> t/t3070-wildmatch.sh | 336
> ---
> 2 files changed, 319 insertions(+), 17 deletions(-)
> create mode 100644 a[]b
>
> diff --git a/a[]b b/a[]b
> new file mode 100644
> index 00..e69de29bb2
A big no-no! This file can't be created on Windows!
> diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
> index 47b479e423..d423bb01f3 100755
> --- a/t/t3070-wildmatch.sh
> +++ b/t/t3070-wildmatch.sh
> @@ -4,31 +4,146 @@ test_description='wildmatch tests'
>
> . ./test-lib.sh
>
> +create_test_file() {
> + file=$1
> +
> + # `touch .` will succeed but obviously not do what we intend
> + # here.
> + test "$file" = "." && return 1
> + # We cannot create a file with an empty filename.
> + test "$file" = "" && return 1
> + # The tests that are testing that e.g. foo//bar is matched by
> + # foo/*/bar can't be tested on filesystems since there's no
> + # way we're getting a double slash.
> + echo "$file" | grep -q -F '//' && return 1
> + # When testing the difference between foo/bar and foo/bar/ we
> + # can't test the latter.
> + echo "$file" | grep -q -E '/$' && return 1
> +
> + dirs=$(echo "$file" | sed -r 's!/[^/]+$!!')
Booh! Booh! So many fork()s! ;)
case "$file" in
*//*)
# The tests that are testing that e.g. foo//bar is matched by
# foo/*/bar can't be tested on filesystems since there's no
# way we're getting a double slash.
return 1;;
*/)
# When testing the difference between foo/bar and foo/bar/ we
# can't test the latter.
return 1;;
esac
dirs=${file%/*}
> +
> + # We touch "./$file" instead of "$file" because even an
> + # escaped "touch -- -" means something different.
> + if test "$file" != "$dirs"
> + then
> + mkdir -p -- "$dirs" 2>/dev/null &&
> + touch -- "./$file" 2>/dev/null &&
> + return 0
> + else
> + touch -- "./$file" 2>/dev/null &&
> + return 0
> + fi
> + return 1
> +}
> +
> wildtest() {
> - match_w_glob=$1
> - match_w_globi=$2
> - match_w_pathmatch=$3
> - match_w_pathmatchi=$4
> - text=$5
> - pattern=$6
> + if test "$#" = 6
> + then
> + # When test-wildmatch and git ls-files produce the same
> + # result.
> + match_w_glob=$1
> + match_f_w_glob=$match_w_glob
> + match_w_globi=$2
> + match_f_w_globi=$match_w_globi
> + match_w_pathmatch=$3
> + match_f_w_pathmatch=$match_w_pathmatch
> + match_w_pathmatchi=$4
> + match_f_w_pathmatchi=$match_w_pathmatchi
> + text=$5
> + pattern=$6
> + elif test "$#" = 10
> + then
> + match_w_glob=$1
> + match_w_globi=$2
> + match_w_pathmatch=$3
> + match_w_pathmatchi=$4
> + match_f_w_glob=$5
> + match_f_w_globi=$6
> + match_f_w_pathmatch=$7
> + match_f_w_pathmatchi=$8
> + text=$9
> + pattern=$10
> + fi
>
> + # $1: Case sensitive glob match: test-wildmatch
> if test "$match_w_glob" = 1
> then
> - test_expect_success "wildmatch: match '$text' '$pattern'" "
> + test_expect_success "wildmatch: match '$text' '$pattern'" "
> test-wildmatch wildmatch '$text' '$pattern'
> "
> elif test "$match_w_glob" = 0
> then
> - test_expect_success "wildmatch: no match '$text' '$pattern'" "
> + test_expect_success "wildmatch: no match '$text' '$pattern'" "
> ! test-wildmatch wildmatch '$text' '$pattern'
> "
> else
> test_expect_success "PANIC: Test framework error. Unknown
> matches value $match_w_glob" 'false'
I think you can write this as 'say ...; exit 1'. See t*.
> fi
>
> + # $1: Case sensitive glob match: ls-files
> + if test "$match_f_w_glob" = 'E'
> + then
> + if create_test_file "$text"
> + then
> + test_expect_success "wildmatch(ls): match dies on
> '$pattern' '$text'" "
> + test_when_finished \"
> + rm -rf -- * &&
Can we be a bit more careful with this rm -rf, please?
There is only one similarly loose case in t/t7003-filter-branch.sh,
and it is outside test_when_finished, i.e., it is well under control;
this instance here inside test_when_finished is not.
> + git reset
> + \" &&
> +
Re: [PATCH] setup.c: move statement under condition
Thank you for your replay.
> I have to be honest: this commit message (including the subject) left me
> quite puzzled as to the intent of this patch.
I still only learn English and correctly express my thoughts while somewhat
difficult.
> If you also have a background story that motivated you to work on this
> patch (for example, if you hit a huge performance bottleneck with some
> tool that fed thousands of absolute paths to Git that needed to be turned
> into paths relative to the worktree's top-level directory), I would
> definitely put that into the commit message, too, if I were you.
I have no such reason. I just saw it and wanted to change it.
> Up until recently, we encouraged dropping the curly brackets from
> single-line statements, but apparently that changed. It is now no longer
> clear, and often left to the taste of the contributor. But not always.
> Sometimes we start a beautiful thread discussion the pros and cons of
> curly brackets in the middle of patch review, and drop altogether
> reviewing the actual patch.
I was guided by the rule from the Documentation/CodingGuidelines:
When there are multiple arms to a conditional and some of them
require braces, enclose even a single line block in braces for
consistency.
And other code from setup.c:
from function get_common_dir:
if (!has_common) {
/* several commands */
} else {
free(candidate->work_tree);
}
from function get_common_dir_noenv:
if (file_exists(path.buf)) {
/* several commands */
} else {
strbuf_addstr(sb, gitdir);
}
> In short: I think your patch does the right thing, and I hope that you
> find my suggestions to improve the patch useful.
I fixed the patch according to your suggestions.
Signed-off-by: Vadim Petrov
---
setup.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/setup.c b/setup.c
index 8cc34186c..1a414c256 100644
--- a/setup.c
+++ b/setup.c
@@ -27,26 +27,26 @@ static int abspath_part_inside_repo(char *path)
{
size_t len;
size_t wtlen;
char *path0;
int off;
const char *work_tree = get_git_work_tree();
if (!work_tree)
return -1;
wtlen = strlen(work_tree);
len = strlen(path);
- off = offset_1st_component(path);
- /* check if work tree is already the prefix */
- if (wtlen <= len && !strncmp(path, work_tree, wtlen)) {
+ if (wtlen > len || strncmp(path, work_tree, wtlen))
+ off = offset_1st_component(path);
+ else { /* check if work tree is already the prefix */
if (path[wtlen] == '/') {
memmove(path, path + wtlen + 1, len - wtlen);
return 0;
} else if (path[wtlen - 1] == '/' || path[wtlen] == '\0') {
/* work tree is the root, or the whole path */
memmove(path, path + wtlen, len - wtlen + 1);
return 0;
}
/* work tree might match beginning of a symlink to work tree */
off = wtlen;
}
--
2.15.1.433.g936d1b989
