* Re: [PATCH v4 0/5] road to reentrant real_path
From: Torsten Bögershausen @ 2017-01-04 6:56 UTC (permalink / raw)
To: Jeff King, Brandon Williams
Cc: git, sbeller, jacob.keller, gitster, ramsay, j6t, pclouds,
larsxschneider
In-Reply-To: <20170104004825.3s27dsircdp5lqte@sigill.intra.peff.net>
On 04.01.17 01:48, Jeff King wrote:
> On Tue, Jan 03, 2017 at 11:09:18AM -0800, Brandon Williams wrote:
>
>> Only change with v4 is in [1/5] renaming the #define MAXSYMLINKS back to
>> MAXDEPTH due to a naming conflict brought up by Lars Schneider.
>
> Hmm. Isn't MAXSYMLINKS basically what you want here, though? It what's
> what all other similar functions will be using.
>
> The only problem was that we were redefining the macro. So maybe:
>
> #ifndef MAXSYMLINKS
> #define MAXSYMLINKS 5
> #endif
>
> would be a good solution?
Why 5 ? (looking at the 20..30 below)
And why 5 on one system and e.g. on my Mac OS
#define MAXSYMLINKS 32
Would the same value value for all Git installations on all platforms make sense?
#define GITMAXSYMLINKS 20
>
> It looks like the "usual" value is more like 20 or 30 on most systems,
> though. We should probably also set errno to ELOOP when we hit the
> limit, which is what other symlink-resolving functions would do.
>
> I'm surprised we didn't hit this on Linux, which has MAXSYMLINKS, too.
> We should be picking it up from <sys/param.h>.
>
> -Peff
>
^ permalink raw reply
* [PATCH 2/2] pathspec: give better message for submodule related pathspec error
From: Stefan Beller @ 2017-01-04 1:48 UTC (permalink / raw)
To: gitster; +Cc: git, peff, bmwill, Stefan Beller
In-Reply-To: <20170104014835.22377-1-sbeller@google.com>
Every once in a while someone complains to the mailing list to have
run into this weird assertion[1]. The usual response from the mailing
list is link to old discussions[2], and acknowledging the problem
stating it is known.
This patch accomplishes two things:
1. Switch assert() to die("BUG") to give a more readable message.
2. Take one of the cases where we hit a BUG and turn it into a normal
"there was something wrong with the input" message.
[1] https://www.google.com/search?q=item-%3Enowildcard_len
[2] http://git.661346.n2.nabble.com/assert-failed-in-submodule-edge-case-td7628687.html
https://www.spinics.net/lists/git/msg249473.html
Helped-by: Jeff King <peff@peff.net>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
pathspec.c | 24 ++++++++++++++++++++++--
t/t6134-pathspec-in-submodule.sh | 33 +++++++++++++++++++++++++++++++++
2 files changed, 55 insertions(+), 2 deletions(-)
create mode 100755 t/t6134-pathspec-in-submodule.sh
diff --git a/pathspec.c b/pathspec.c
index 22ca74a126..574a0bb158 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -313,8 +313,28 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
}
/* sanity checks, pathspec matchers assume these are sane */
- assert(item->nowildcard_len <= item->len &&
- item->prefix <= item->len);
+ if (item->nowildcard_len > item->len ||
+ item->prefix > item->len) {
+ /* Historically this always was a submodule issue */
+ for (i = 0; i < active_nr; i++) {
+ struct cache_entry *ce = active_cache[i];
+ int len;
+
+ if (!S_ISGITLINK(ce->ce_mode))
+ continue;
+
+ len = ce_namelen(ce);
+ if (item->len < len)
+ len = item->len;
+
+ if (!memcmp(ce->name, item->match, len))
+ die (_("Pathspec '%s' is in submodule '%.*s'"),
+ item->original, ce_namelen(ce), ce->name);
+ }
+ /* The error is a new unknown bug */
+ die ("BUG: item->nowildcard_len > item->len || item->prefix > item->len)");
+ }
+
return magic;
}
diff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh
new file mode 100755
index 0000000000..2900d8d06e
--- /dev/null
+++ b/t/t6134-pathspec-in-submodule.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+
+test_description='test case exclude pathspec'
+
+TEST_CREATE_SUBMODULE=yes
+. ./test-lib.sh
+
+test_expect_success 'setup a submodule' '
+ git submodule add ./pretzel.bare sub &&
+ git commit -a -m "add submodule" &&
+ git submodule deinit --all
+'
+
+cat <<EOF >expect
+fatal: Pathspec 'sub/a' is in submodule 'sub'
+EOF
+
+test_expect_success 'error message for path inside submodule' '
+ echo a >sub/a &&
+ test_must_fail git add sub/a 2>actual &&
+ test_cmp expect actual
+'
+
+cat <<EOF >expect
+fatal: Pathspec '.' is in submodule 'sub'
+EOF
+
+test_expect_success 'error message for path inside submodule from within submodule' '
+ test_must_fail git -C sub add . 2>actual &&
+ test_cmp expect actual
+'
+
+test_done
--
2.11.0.rc2.31.g2cc886f
^ permalink raw reply related
* [PATCH 1/2] submodule tests: don't use itself as a submodule
From: Stefan Beller @ 2017-01-04 1:48 UTC (permalink / raw)
To: gitster; +Cc: git, peff, bmwill, Stefan Beller
In-Reply-To: <20170104014835.22377-1-sbeller@google.com>
In reality nobody would run "git submodule add ./. <submodule-path>"
to add the repository to itself as a submodule as this comes with some
nasty surprises, such as infinite recursion when cloning that repository.
However we do this all the time in the test suite, because most of the
time this was the most convenient way to test a very specific thing
for submodule behavior.
This provides an easier way to have submodules in tests, by just setting
TEST_CREATE_SUBMODULE to a non empty string, similar to
TEST_NO_CREATE_REPO.
Make use of it in those tests that add a submodule from ./. except for
the occurrence in create_lib_submodule_repo as there it seems we craft
a repository deliberately for both inside as well as outside use.
The name "pretzel.[non]bare" was chosen deliberate to not introduce
more strings to the test suite containing "sub[module]" as searching for
"sub" already yields a lot of hits from different contexts. "pretzel"
doesn't occur in the test suite yet, so it is a good candidate for
a potential remote for a submodule.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
t/lib-submodule-update.sh | 2 ++
t/t7001-mv.sh | 5 +++--
t/t7507-commit-verbose.sh | 4 +++-
t/t7800-difftool.sh | 4 +++-
t/test-lib-functions.sh | 16 ++++++++++++++++
5 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 79cdd34a54..58d76d9df8 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -44,6 +44,8 @@ create_lib_submodule_repo () {
git branch "no_submodule" &&
git checkout -b "add_sub1" &&
+ # Adding the repo itself as a submodule is a hack.
+ # Do not imitate this.
git submodule add ./. sub1 &&
git config -f .gitmodules submodule.sub1.ignore all &&
git config submodule.sub1.ignore all &&
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index e365d1ff77..6cb32f3a3a 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -1,6 +1,7 @@
#!/bin/sh
test_description='git mv in subdirs'
+TEST_CREATE_SUBMODULE=yes
. ./test-lib.sh
test_expect_success \
@@ -288,12 +289,12 @@ rm -f moved symlink
test_expect_success 'setup submodule' '
git commit -m initial &&
git reset --hard &&
- git submodule add ./. sub &&
+ git submodule add ./pretzel.bare sub &&
echo content >file &&
git add file &&
git commit -m "added sub and file" &&
mkdir -p deep/directory/hierarchy &&
- git submodule add ./. deep/directory/hierarchy/sub &&
+ git submodule add ./pretzel.bare deep/directory/hierarchy/sub &&
git commit -m "added another submodule" &&
git branch submodule
'
diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index ed2653d46f..d269900afa 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -1,6 +1,7 @@
#!/bin/sh
test_description='verbose commit template'
+TEST_CREATE_SUBMODULE=yes
. ./test-lib.sh
write_script "check-for-diff" <<\EOF &&
@@ -74,11 +75,12 @@ test_expect_success 'diff in message is retained with -v' '
test_expect_success 'submodule log is stripped out too with -v' '
git config diff.submodule log &&
- git submodule add ./. sub &&
+ git submodule add ./pretzel.bare sub &&
git commit -m "sub added" &&
(
cd sub &&
echo "more" >>file &&
+ git add file &&
git commit -a -m "submodule commit"
) &&
(
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 70a2de461a..d13a5d0453 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -7,6 +7,7 @@ test_description='git-difftool
Testing basic diff tool invocation
'
+TEST_CREATE_SUBMODULE=Yes
. ./test-lib.sh
@@ -534,7 +535,8 @@ test_expect_success PERL 'difftool --no-symlinks detects conflict ' '
'
test_expect_success PERL 'difftool properly honors gitlink and core.worktree' '
- git submodule add ./. submod/ule &&
+ git submodule add ./pretzel.bare submod/ule &&
+ test_commit -C submod/ule second_commit &&
test_config -C submod/ule diff.tool checktrees &&
test_config -C submod/ule difftool.checktrees.cmd '\''
test -d "$LOCAL" && test -d "$REMOTE" && echo good
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 579e812506..aa327a7dff 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -800,6 +800,22 @@ test_create_repo () {
error "cannot run git init -- have you built things yet?"
mv .git/hooks .git/hooks-disabled
) || exit
+ if test -n "$TEST_CREATE_SUBMODULE"
+ then
+ (
+ cd "$repo"
+ TEST_CREATE_SUBMODULE=
+ export TEST_CREATE_SUBMODULE
+ test_create_repo "pretzel.nonbare"
+ test_commit -C "pretzel.nonbare" \
+ "create submodule" "submodule-file" \
+ "submodule-content" "submodule-tag" >&3 2>&4 ||
+ error "cannot run test_commit"
+ git clone --bare "pretzel.nonbare" \
+ "pretzel.bare" >&3 2>&4 ||
+ error "cannot clone into bare"
+ )
+ fi
}
# This function helps on symlink challenged file systems when it is not
--
2.11.0.rc2.31.g2cc886f
^ permalink raw reply related
* [PATCHv4 0/2] pathspec: give better message for submodule related pathspec error
From: Stefan Beller @ 2017-01-04 1:48 UTC (permalink / raw)
To: gitster; +Cc: git, peff, bmwill, Stefan Beller
> It MIGHT be a handy hack when writing a test, but let's stop doing
> that insanity. No sane project does that in real life, doesn't it?
> Create a subdirectory, make it a repository, have a commit there and
> bind that as our own submodule. That would be a more normal way to
> start your own superproject and its submodule pair if they originate
> together at the same place.
This comes as an extra patch before the actual fix.
The actual fixing patch was reworded borrowing some words from Jeff.
As this makes use of "test_commit -C", it goes on top of sb/submodule-embed-gitdir
Thanks,
Stefan
Stefan Beller (2):
submodule tests: don't use itself as a submodule
pathspec: give better message for submodule related pathspec error
pathspec.c | 24 ++++++++++++++++++++++--
t/lib-submodule-update.sh | 2 ++
t/t6134-pathspec-in-submodule.sh | 33 +++++++++++++++++++++++++++++++++
t/t7001-mv.sh | 5 +++--
t/t7507-commit-verbose.sh | 4 +++-
t/t7800-difftool.sh | 4 +++-
t/test-lib-functions.sh | 16 ++++++++++++++++
7 files changed, 82 insertions(+), 6 deletions(-)
create mode 100755 t/t6134-pathspec-in-submodule.sh
--
2.11.0.rc2.31.g2cc886f
^ permalink raw reply
* Re: [PATCH] pathspec: give better message for submodule related pathspec error
From: Jeff King @ 2017-01-04 1:23 UTC (permalink / raw)
To: Stefan Beller; +Cc: Junio C Hamano, git@vger.kernel.org, Brandon Williams
In-Reply-To: <CAGZ79kZ5F46UzeNeVYQV1EKEEa6+az-Pe_jrT+wZF9X6b34CGA@mail.gmail.com>
On Tue, Jan 03, 2017 at 10:15:38AM -0800, Stefan Beller wrote:
> > I take that the new "BUG" thing tells the Git developers that no
> > sane codepath should throw an pathspec_item that satisfies the
> > condition of the if() statement for non-submodules?
>
> If we want to keep the semantics of the assert around, then we
> have to have a blank statement if the submodule error message
> is not triggered.
>
> I assume if we print this BUG, then there is an actual bug.
Right. I think this isn't a new "BUG", but rather a loosening of an
existing one. IOW, two things are going on here:
1. Switch assert() to die("BUG") to give a more readable message.
2. Take one of the cases where we hit a BUG and turn it into a normal
"there was something wrong with the input" message.
If I understand the submodule case correctly, then (2) is reasonable.
The user gave a bogus pathspec that crosses the submodule boundary.
I've no idea if there are other cases that could ever hit the remaining
BUG, but it seems like a good defensive measure to leave it in. If
somebody hits it, then we can investigate their case.
-Peff
^ permalink raw reply
* Re: [PATCHv2] submodule.c: use GIT_DIR_ENVIRONMENT consistently
From: Jeff King @ 2017-01-04 1:18 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, l.s.r, git
In-Reply-To: <20170103183047.17968-1-sbeller@google.com>
On Tue, Jan 03, 2017 at 10:30:47AM -0800, Stefan Beller wrote:
> In C code we have the luxury of having constants for all the important
> things that are hard coded. This is the only place in C, that hard codes
> the git directory environment variable, so fix it.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
This looks like a good change to me.
Minor nit: the comma after "C" in your commit message is extraneous. ;)
-Peff
^ permalink raw reply
* Re: builtin difftool parsing issue
From: Jacob Keller @ 2017-01-04 1:05 UTC (permalink / raw)
To: Stefan Beller
Cc: Paul Sbarra, Johannes Schindelin, David Aguilar,
Dennis Kaarsemaker, git@vger.kernel.org, Junio C Hamano
In-Reply-To: <CAGZ79kaMnFUefiMagU82euxjeQdnc9KdgSwyDvkDp--QT-MbCA@mail.gmail.com>
On Tue, Jan 3, 2017 at 10:47 AM, Stefan Beller <sbeller@google.com> wrote:
> On Mon, Jan 2, 2017 at 11:05 AM, Paul Sbarra <sbarra.paul@gmail.com> wrote:
>>> I would have expected `git difftool --submodule=diff ...` to work... What
>>> are the problems?
>>
>> The docs for difftool state...
>> "git difftool is a frontend to git diff and accepts the same options
>> and arguments."
>
> I think such a sentence in the man page is dangerous, as nobody
> was caught this issue until now. There have been numerous authors
> and reviewers that touched "diff --submodule=<format>, but as there
> is no back-reference, hinting that the patch is only half done, and the
> difftool also needs to implement such an option.
>
> We should reword the man page either as
>
> "git difftool is a frontend to git diff and accepts the most(?) options
> and arguments."
>
> or even be explicit and list the arguments instead. There we could also
> describe differences if any (e.g. the formats available might be different
> for --submodule=<format>)
I agree with updating the documentation. Difftool would require
extensive work to support the --submodule format, and I'm not sure
it's worth it.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH v4 1/4] Avoid Coverity warning about unfree()d git_exec_path()
From: Jeff King @ 2017-01-04 1:13 UTC (permalink / raw)
To: Stefan Beller
Cc: Johannes Schindelin, git@vger.kernel.org, Junio C Hamano,
David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <CAGZ79kZ--jp08pK+xwn1N2VQQr8bA5+DveE2HsoY90R1gR6c_A@mail.gmail.com>
On Tue, Jan 03, 2017 at 12:11:25PM -0800, Stefan Beller wrote:
> On Mon, Jan 2, 2017 at 8:22 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > Technically, it is correct that git_exec_path() returns a possibly
> > malloc()ed string. Practically, it is *sometimes* not malloc()ed. So
> > let's just use a static variable to make it a singleton. That'll shut
> > Coverity up, hopefully.
>
> I picked up this patch and applied it to the coverity branch
> that I maintain at github/stefanbeller/git.
>
> I'd love to see this patch upstream as it reduces my maintenance
> burden of the coverity branch by a patch.
There is another lurking issue in that function, which is that the
return value of getenv() is not guaranteed to last beyond more calls to
getenv() or setenv(). It should probably xstrdup() that result, too.
At that point 2 out of 3 of the return cases would be malloc'd strings,
so we _could_ switch the third and say "caller must free the result".
But I think I prefer something like Dscho's solution (more on that
below).
> Early on when Git was new to coverity, some arguments were made
> that patches like these only clutter the main code base which is read
> by a lot of people, hence we want these quirks for coverity not upstream.
> And I think that still holds.
>
> If this patch is only to appease coverity (as the commit message eludes
> to) I think this may be a bad idea for upstream. If this patch fixes an
> actual problem, then the commit message needs to spell that out.
This is a real leak, though in all cases the program typically exits
soon afterwards. But we leak from list_commands(), for example, and it
is not immediately obvious that this is only called right before
exiting.
But I think more important is that caching the result in a static
variable communicates something (both to Coverity and to a reader of the
code). This is a value we expect to live through the life of the
program, and it is OK for it to "leak" when it goes out of scope by the
program exiting.
So even though the behavior does not really change, the annotation has
value.
-Peff
^ permalink raw reply
* Re: [PATCH v4 4/5] real_path: have callers use real_pathdup and strbuf_realpath
From: Jacob Keller @ 2017-01-04 1:07 UTC (permalink / raw)
To: Brandon Williams
Cc: Git mailing list, Stefan Beller, Jeff King, Junio C Hamano,
Ramsay Jones, Torsten Bögershausen, Johannes Sixt,
Duy Nguyen, Lars Schneider
In-Reply-To: <20170103190923.11882-5-bmwill@google.com>
On Tue, Jan 3, 2017 at 11:09 AM, Brandon Williams <bmwill@google.com> wrote:
> Migrate callers of real_path() who duplicate the retern value to use
> real_pathdup or strbuf_realpath.
Nit: s/retern/return
^ permalink raw reply
* [PATCH 0/4] fix mergetool+rerere+subdir regression
From: Richard Hansen @ 2017-01-04 0:50 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr
[-- Attachment #1: Type: text/plain, Size: 667 bytes --]
If rerere is enabled, no pathnames are given, and mergetool is run
from a subdirectory, mergetool always prints "No files need merging".
Fix the bug.
This regression was introduced in
57937f70a09c12ef484c290865dac4066d207c9c (v2.11.0).
Richard Hansen (4):
t7610: update branch names to match test number
t7610: make tests more independent and debuggable
t7610: add test case for rerere+mergetool+subdir bug
mergetool: fix running in subdir when rerere enabled
git-mergetool.sh | 1 +
t/t7610-mergetool.sh | 132 ++++++++++++++++++++++++++++++++++-----------------
2 files changed, 90 insertions(+), 43 deletions(-)
--
2.11.0.390.gc69c2f50cf-goog
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]
^ permalink raw reply
* [PATCH 4/4] mergetool: fix running in subdir when rerere enabled
From: Richard Hansen @ 2017-01-04 0:50 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr
In-Reply-To: <20170104005042.51530-1-hansenr@google.com>
[-- Attachment #1: Type: text/plain, Size: 1473 bytes --]
If rerere is enabled and no pathnames are given, run cd_to_toplevel
before running 'git diff --name-only' so that 'git diff --name-only'
sees the pathnames emitted by 'git rerere remaining'.
Also run cd_to_toplevel before running 'git rerere remaining' in case
'git rerere remaining' is ever changed to print pathnames relative to
the current directory rather than to $GIT_WORK_TREE.
This fixes a regression introduced in
57937f70a09c12ef484c290865dac4066d207c9c (v2.11.0).
Signed-off-by: Richard Hansen <hansenr@google.com>
---
git-mergetool.sh | 1 +
t/t7610-mergetool.sh | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/git-mergetool.sh b/git-mergetool.sh
index e52b4e4f2..67ea0d6db 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -456,6 +456,7 @@ main () {
if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
then
+ cd_to_toplevel
set -- $(git rerere remaining)
if test $# -eq 0
then
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index e7b3e1866..1f7d63acb 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -200,7 +200,7 @@ test_expect_success 'mergetool merges all from subdir (rerere disabled)' '
)
'
-test_expect_failure 'mergetool merges all from subdir (rerere enabled)' '
+test_expect_success 'mergetool merges all from subdir (rerere enabled)' '
git reset --hard &&
git checkout -b test$test_count branch1 &&
test_config rerere.enabled true &&
--
2.11.0.390.gc69c2f50cf-goog
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]
^ permalink raw reply related
* [PATCH 1/4] t7610: update branch names to match test number
From: Richard Hansen @ 2017-01-04 0:50 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr
In-Reply-To: <20170104005042.51530-1-hansenr@google.com>
[-- Attachment #1: Type: text/plain, Size: 11801 bytes --]
Rename the testNN branches so that NN matches the test number. This
should make it easier to troubleshoot test issues. Use $test_count to
keep this future-proof.
Signed-off-by: Richard Hansen <hansenr@google.com>
---
t/t7610-mergetool.sh | 82 ++++++++++++++++++++++++++--------------------------
1 file changed, 41 insertions(+), 41 deletions(-)
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 6d9f21511..14090739f 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -94,7 +94,7 @@ test_expect_success 'setup' '
'
test_expect_success 'custom mergetool' '
- git checkout -b test1 branch1 &&
+ git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master >/dev/null 2>&1 &&
( yes "" | git mergetool both >/dev/null 2>&1 ) &&
@@ -113,7 +113,7 @@ test_expect_success 'custom mergetool' '
test_expect_success 'mergetool crlf' '
test_config core.autocrlf true &&
- git checkout -b test2 branch1 &&
+ git checkout -b test$test_count branch1 &&
test_must_fail git merge master >/dev/null 2>&1 &&
( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
@@ -134,7 +134,7 @@ test_expect_success 'mergetool crlf' '
'
test_expect_success 'mergetool in subdir' '
- git checkout -b test3 branch1 &&
+ git checkout -b test$test_count branch1 &&
git submodule update -N &&
(
cd subdir &&
@@ -161,7 +161,7 @@ test_expect_success 'mergetool on file in parent dir' '
'
test_expect_success 'mergetool skips autoresolved' '
- git checkout -b test4 branch1 &&
+ git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master &&
test -n "$(git ls-files -u)" &&
@@ -192,7 +192,7 @@ test_expect_success 'mergetool merges all from subdir' '
test_expect_success 'mergetool skips resolved paths when rerere is active' '
test_config rerere.enabled true &&
rm -rf .git/rr-cache &&
- git checkout -b test5 branch1 &&
+ git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master >/dev/null 2>&1 &&
( yes "l" | git mergetool --no-prompt submod >/dev/null 2>&1 ) &&
@@ -233,7 +233,7 @@ test_expect_success 'conflicted stash sets up rerere' '
test_expect_success 'mergetool takes partial path' '
git reset --hard &&
test_config rerere.enabled false &&
- git checkout -b test12 branch1 &&
+ git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master &&
@@ -308,12 +308,12 @@ test_expect_success 'mergetool keeps tempfiles when aborting delete/delete' '
'
test_expect_success 'deleted vs modified submodule' '
- git checkout -b test6 branch1 &&
+ git checkout -b test$test_count branch1 &&
git submodule update -N &&
mv submod submod-movedaside &&
git rm --cached submod &&
git commit -m "Submodule deleted from branch" &&
- git checkout -b test6.a test6 &&
+ git checkout -b test$test_count.a test$test_count &&
test_must_fail git merge master &&
test -n "$(git ls-files -u)" &&
( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
@@ -329,7 +329,7 @@ test_expect_success 'deleted vs modified submodule' '
git commit -m "Merge resolved by keeping module" &&
mv submod submod-movedaside &&
- git checkout -b test6.b test6 &&
+ git checkout -b test$test_count.b test$test_count &&
git submodule update -N &&
test_must_fail git merge master &&
test -n "$(git ls-files -u)" &&
@@ -343,9 +343,9 @@ test_expect_success 'deleted vs modified submodule' '
git commit -m "Merge resolved by deleting module" &&
mv submod-movedaside submod &&
- git checkout -b test6.c master &&
+ git checkout -b test$test_count.c master &&
git submodule update -N &&
- test_must_fail git merge test6 &&
+ test_must_fail git merge test$test_count &&
test -n "$(git ls-files -u)" &&
( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
( yes "" | git mergetool both >/dev/null 2>&1 ) &&
@@ -359,9 +359,9 @@ test_expect_success 'deleted vs modified submodule' '
git commit -m "Merge resolved by deleting module" &&
mv submod.orig submod &&
- git checkout -b test6.d master &&
+ git checkout -b test$test_count.d master &&
git submodule update -N &&
- test_must_fail git merge test6 &&
+ test_must_fail git merge test$test_count &&
test -n "$(git ls-files -u)" &&
( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
( yes "" | git mergetool both >/dev/null 2>&1 ) &&
@@ -377,14 +377,14 @@ test_expect_success 'deleted vs modified submodule' '
'
test_expect_success 'file vs modified submodule' '
- git checkout -b test7 branch1 &&
+ git checkout -b test$test_count branch1 &&
git submodule update -N &&
mv submod submod-movedaside &&
git rm --cached submod &&
echo not a submodule >submod &&
git add submod &&
git commit -m "Submodule path becomes file" &&
- git checkout -b test7.a branch1 &&
+ git checkout -b test$test_count.a branch1 &&
test_must_fail git merge master &&
test -n "$(git ls-files -u)" &&
( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
@@ -400,7 +400,7 @@ test_expect_success 'file vs modified submodule' '
git commit -m "Merge resolved by keeping module" &&
mv submod submod-movedaside &&
- git checkout -b test7.b test7 &&
+ git checkout -b test$test_count.b test$test_count &&
test_must_fail git merge master &&
test -n "$(git ls-files -u)" &&
( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
@@ -413,11 +413,11 @@ test_expect_success 'file vs modified submodule' '
test "$output" = "No files need merging" &&
git commit -m "Merge resolved by keeping file" &&
- git checkout -b test7.c master &&
+ git checkout -b test$test_count.c master &&
rmdir submod && mv submod-movedaside submod &&
test ! -e submod.orig &&
git submodule update -N &&
- test_must_fail git merge test7 &&
+ test_must_fail git merge test$test_count &&
test -n "$(git ls-files -u)" &&
( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
( yes "" | git mergetool both >/dev/null 2>&1 ) &&
@@ -430,10 +430,10 @@ test_expect_success 'file vs modified submodule' '
test "$output" = "No files need merging" &&
git commit -m "Merge resolved by keeping file" &&
- git checkout -b test7.d master &&
+ git checkout -b test$test_count.d master &&
rmdir submod && mv submod.orig submod &&
git submodule update -N &&
- test_must_fail git merge test7 &&
+ test_must_fail git merge test$test_count &&
test -n "$(git ls-files -u)" &&
( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
( yes "" | git mergetool both>/dev/null 2>&1 ) &&
@@ -448,7 +448,7 @@ test_expect_success 'file vs modified submodule' '
'
test_expect_success 'submodule in subdirectory' '
- git checkout -b test10 branch1 &&
+ git checkout -b test$test_count branch1 &&
git submodule update -N &&
(
cd subdir &&
@@ -464,52 +464,52 @@ test_expect_success 'submodule in subdirectory' '
git add subdir/subdir_module &&
git commit -m "add submodule in subdirectory" &&
- git checkout -b test10.a test10 &&
+ git checkout -b test$test_count.a test$test_count &&
git submodule update -N &&
(
cd subdir/subdir_module &&
git checkout -b super10.a &&
- echo test10.a >file15 &&
+ echo test$test_count.a >file15 &&
git add file15 &&
git commit -m "on branch 10.a"
) &&
git add subdir/subdir_module &&
- git commit -m "change submodule in subdirectory on test10.a" &&
+ git commit -m "change submodule in subdirectory on test$test_count.a" &&
- git checkout -b test10.b test10 &&
+ git checkout -b test$test_count.b test$test_count &&
git submodule update -N &&
(
cd subdir/subdir_module &&
git checkout -b super10.b &&
- echo test10.b >file15 &&
+ echo test$test_count.b >file15 &&
git add file15 &&
git commit -m "on branch 10.b"
) &&
git add subdir/subdir_module &&
- git commit -m "change submodule in subdirectory on test10.b" &&
+ git commit -m "change submodule in subdirectory on test$test_count.b" &&
- test_must_fail git merge test10.a >/dev/null 2>&1 &&
+ test_must_fail git merge test$test_count.a >/dev/null 2>&1 &&
(
cd subdir &&
( yes "l" | git mergetool subdir_module )
) &&
- test "$(cat subdir/subdir_module/file15)" = "test10.b" &&
+ test "$(cat subdir/subdir_module/file15)" = "test$test_count.b" &&
git submodule update -N &&
- test "$(cat subdir/subdir_module/file15)" = "test10.b" &&
+ test "$(cat subdir/subdir_module/file15)" = "test$test_count.b" &&
git reset --hard &&
git submodule update -N &&
- test_must_fail git merge test10.a >/dev/null 2>&1 &&
+ test_must_fail git merge test$test_count.a >/dev/null 2>&1 &&
( yes "r" | git mergetool subdir/subdir_module ) &&
- test "$(cat subdir/subdir_module/file15)" = "test10.b" &&
+ test "$(cat subdir/subdir_module/file15)" = "test$test_count.b" &&
git submodule update -N &&
- test "$(cat subdir/subdir_module/file15)" = "test10.a" &&
+ test "$(cat subdir/subdir_module/file15)" = "test$test_count.a" &&
git commit -m "branch1 resolved with mergetool" &&
rm -rf subdir/subdir_module
'
test_expect_success 'directory vs modified submodule' '
- git checkout -b test11 branch1 &&
+ git checkout -b test$test_count branch1 &&
mv submod submod-movedaside &&
git rm --cached submod &&
mkdir submod &&
@@ -537,9 +537,9 @@ test_expect_success 'directory vs modified submodule' '
test "$(cat submod/bar)" = "master submodule" &&
git reset --hard >/dev/null 2>&1 && rm -rf submod-movedaside &&
- git checkout -b test11.c master &&
+ git checkout -b test$test_count.c master &&
git submodule update -N &&
- test_must_fail git merge test11 &&
+ test_must_fail git merge test$test_count &&
test -n "$(git ls-files -u)" &&
( yes "l" | git mergetool submod ) &&
git submodule update -N &&
@@ -547,7 +547,7 @@ test_expect_success 'directory vs modified submodule' '
git reset --hard >/dev/null 2>&1 &&
git submodule update -N &&
- test_must_fail git merge test11 &&
+ test_must_fail git merge test$test_count &&
test -n "$(git ls-files -u)" &&
test ! -e submod.orig &&
( yes "r" | git mergetool submod ) &&
@@ -559,7 +559,7 @@ test_expect_success 'directory vs modified submodule' '
'
test_expect_success 'file with no base' '
- git checkout -b test13 branch1 &&
+ git checkout -b test$test_count branch1 &&
test_must_fail git merge master &&
git mergetool --no-prompt --tool mybase -- both &&
>expected &&
@@ -568,7 +568,7 @@ test_expect_success 'file with no base' '
'
test_expect_success 'custom commands override built-ins' '
- git checkout -b test14 branch1 &&
+ git checkout -b test$test_count branch1 &&
test_config mergetool.defaults.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" &&
test_config mergetool.defaults.trustExitCode true &&
test_must_fail git merge master &&
@@ -579,7 +579,7 @@ test_expect_success 'custom commands override built-ins' '
'
test_expect_success 'filenames seen by tools start with ./' '
- git checkout -b test15 branch1 &&
+ git checkout -b test$test_count branch1 &&
test_config mergetool.writeToTemp false &&
test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
test_config mergetool.myecho.trustExitCode true &&
@@ -595,7 +595,7 @@ test_lazy_prereq MKTEMP '
'
test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToTemp' '
- git checkout -b test16 branch1 &&
+ git checkout -b test$test_count branch1 &&
test_config mergetool.writeToTemp true &&
test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
test_config mergetool.myecho.trustExitCode true &&
--
2.11.0.390.gc69c2f50cf-goog
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]
^ permalink raw reply related
* [PATCH 3/4] t7610: add test case for rerere+mergetool+subdir bug
From: Richard Hansen @ 2017-01-04 0:50 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr
In-Reply-To: <20170104005042.51530-1-hansenr@google.com>
[-- Attachment #1: Type: text/plain, Size: 1722 bytes --]
If rerere is enabled and mergetool is run from a subdirectory,
mergetool always prints "No files need merging". Add an expected
failure test case for this situation.
Signed-off-by: Richard Hansen <hansenr@google.com>
---
t/t7610-mergetool.sh | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 8e2b4e147..e7b3e1866 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -182,7 +182,7 @@ test_expect_success 'mergetool skips autoresolved' '
git reset --hard
'
-test_expect_success 'mergetool merges all from subdir' '
+test_expect_success 'mergetool merges all from subdir (rerere disabled)' '
git reset --hard &&
git checkout -b test$test_count branch1 &&
test_config rerere.enabled false &&
@@ -200,6 +200,25 @@ test_expect_success 'mergetool merges all from subdir' '
)
'
+test_expect_failure 'mergetool merges all from subdir (rerere enabled)' '
+ git reset --hard &&
+ git checkout -b test$test_count branch1 &&
+ test_config rerere.enabled true &&
+ rm -rf .git/rr-cache &&
+ (
+ cd subdir &&
+ test_must_fail git merge master &&
+ ( yes "r" | git mergetool ../submod ) &&
+ ( yes "d" "d" | git mergetool --no-prompt ) &&
+ test "$(cat ../file1)" = "master updated" &&
+ test "$(cat ../file2)" = "master new" &&
+ test "$(cat file3)" = "master new sub" &&
+ ( cd .. && git submodule update -N ) &&
+ test "$(cat ../submod/bar)" = "master submodule" &&
+ git commit -m "branch2 resolved by mergetool from subdir"
+ )
+'
+
test_expect_success 'mergetool skips resolved paths when rerere is active' '
git reset --hard &&
test_config rerere.enabled true &&
--
2.11.0.390.gc69c2f50cf-goog
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]
^ permalink raw reply related
* [PATCH 2/4] t7610: make tests more independent and debuggable
From: Richard Hansen @ 2017-01-04 0:50 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr
In-Reply-To: <20170104005042.51530-1-hansenr@google.com>
[-- Attachment #1: Type: text/plain, Size: 7161 bytes --]
If a test fails it might leave the repository in a strange state. Add
'git reset --hard' at the beginning of each test to increase the odds
of passing when an earlier test fails.
Also use test-specific branches to avoid interfering with later tests
and to make the tests easier to debug.
Signed-off-by: Richard Hansen <hansenr@google.com>
---
t/t7610-mergetool.sh | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 14090739f..8e2b4e147 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -94,6 +94,7 @@ test_expect_success 'setup' '
'
test_expect_success 'custom mergetool' '
+ git reset --hard &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master >/dev/null 2>&1 &&
@@ -112,6 +113,7 @@ test_expect_success 'custom mergetool' '
'
test_expect_success 'mergetool crlf' '
+ git reset --hard &&
test_config core.autocrlf true &&
git checkout -b test$test_count branch1 &&
test_must_fail git merge master >/dev/null 2>&1 &&
@@ -134,6 +136,7 @@ test_expect_success 'mergetool crlf' '
'
test_expect_success 'mergetool in subdir' '
+ git reset --hard &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
(
@@ -145,8 +148,13 @@ test_expect_success 'mergetool in subdir' '
'
test_expect_success 'mergetool on file in parent dir' '
+ git reset --hard &&
+ git checkout -b test$test_count branch1 &&
+ git submodule update -N &&
(
cd subdir &&
+ test_must_fail git merge master >/dev/null 2>&1 &&
+ ( yes "" | git mergetool file3 >/dev/null 2>&1 ) &&
( yes "" | git mergetool ../file1 >/dev/null 2>&1 ) &&
( yes "" | git mergetool ../file2 ../spaced\ name >/dev/null 2>&1 ) &&
( yes "" | git mergetool ../both >/dev/null 2>&1 ) &&
@@ -161,6 +169,7 @@ test_expect_success 'mergetool on file in parent dir' '
'
test_expect_success 'mergetool skips autoresolved' '
+ git reset --hard &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master &&
@@ -174,6 +183,8 @@ test_expect_success 'mergetool skips autoresolved' '
'
test_expect_success 'mergetool merges all from subdir' '
+ git reset --hard &&
+ git checkout -b test$test_count branch1 &&
test_config rerere.enabled false &&
(
cd subdir &&
@@ -190,6 +201,7 @@ test_expect_success 'mergetool merges all from subdir' '
'
test_expect_success 'mergetool skips resolved paths when rerere is active' '
+ git reset --hard &&
test_config rerere.enabled true &&
rm -rf .git/rr-cache &&
git checkout -b test$test_count branch1 &&
@@ -204,6 +216,7 @@ test_expect_success 'mergetool skips resolved paths when rerere is active' '
'
test_expect_success 'conflicted stash sets up rerere' '
+ git reset --hard &&
test_config rerere.enabled true &&
git checkout stash1 &&
echo "Conflicting stash content" >file11 &&
@@ -244,6 +257,7 @@ test_expect_success 'mergetool takes partial path' '
'
test_expect_success 'mergetool delete/delete conflict' '
+ git reset --hard &&
git checkout -b delete-base branch1 &&
mkdir -p a/a &&
(echo one; echo two; echo 3; echo 4) >a/a/file.txt &&
@@ -274,6 +288,7 @@ test_expect_success 'mergetool delete/delete conflict' '
'
test_expect_success 'mergetool produces no errors when keepBackup is used' '
+ git reset --hard &&
test_config mergetool.keepBackup true &&
test_must_fail git merge move-to-b &&
: >expect &&
@@ -284,6 +299,7 @@ test_expect_success 'mergetool produces no errors when keepBackup is used' '
'
test_expect_success 'mergetool honors tempfile config for deleted files' '
+ git reset --hard &&
test_config mergetool.keepTemporaries false &&
test_must_fail git merge move-to-b &&
echo d | git mergetool a/a/file.txt &&
@@ -292,6 +308,7 @@ test_expect_success 'mergetool honors tempfile config for deleted files' '
'
test_expect_success 'mergetool keeps tempfiles when aborting delete/delete' '
+ git reset --hard &&
test_config mergetool.keepTemporaries true &&
test_must_fail git merge move-to-b &&
! (echo a; echo n) | git mergetool a/a/file.txt &&
@@ -308,6 +325,7 @@ test_expect_success 'mergetool keeps tempfiles when aborting delete/delete' '
'
test_expect_success 'deleted vs modified submodule' '
+ git reset --hard &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
mv submod submod-movedaside &&
@@ -377,6 +395,7 @@ test_expect_success 'deleted vs modified submodule' '
'
test_expect_success 'file vs modified submodule' '
+ git reset --hard &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
mv submod submod-movedaside &&
@@ -448,6 +467,7 @@ test_expect_success 'file vs modified submodule' '
'
test_expect_success 'submodule in subdirectory' '
+ git reset --hard &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
(
@@ -509,6 +529,7 @@ test_expect_success 'submodule in subdirectory' '
'
test_expect_success 'directory vs modified submodule' '
+ git reset --hard &&
git checkout -b test$test_count branch1 &&
mv submod submod-movedaside &&
git rm --cached submod &&
@@ -559,6 +580,7 @@ test_expect_success 'directory vs modified submodule' '
'
test_expect_success 'file with no base' '
+ git reset --hard &&
git checkout -b test$test_count branch1 &&
test_must_fail git merge master &&
git mergetool --no-prompt --tool mybase -- both &&
@@ -568,6 +590,7 @@ test_expect_success 'file with no base' '
'
test_expect_success 'custom commands override built-ins' '
+ git reset --hard &&
git checkout -b test$test_count branch1 &&
test_config mergetool.defaults.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" &&
test_config mergetool.defaults.trustExitCode true &&
@@ -579,6 +602,7 @@ test_expect_success 'custom commands override built-ins' '
'
test_expect_success 'filenames seen by tools start with ./' '
+ git reset --hard &&
git checkout -b test$test_count branch1 &&
test_config mergetool.writeToTemp false &&
test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
@@ -595,6 +619,7 @@ test_lazy_prereq MKTEMP '
'
test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToTemp' '
+ git reset --hard &&
git checkout -b test$test_count branch1 &&
test_config mergetool.writeToTemp true &&
test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
@@ -607,6 +632,7 @@ test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToT
'
test_expect_success 'diff.orderFile configuration is honored' '
+ git reset --hard &&
test_config diff.orderFile order-file &&
test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
test_config mergetool.myecho.trustExitCode true &&
@@ -639,6 +665,7 @@ test_expect_success 'diff.orderFile configuration is honored' '
git reset --hard >/dev/null
'
test_expect_success 'mergetool -Oorder-file is honored' '
+ git reset --hard &&
test_config diff.orderFile order-file &&
test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
test_config mergetool.myecho.trustExitCode true &&
--
2.11.0.390.gc69c2f50cf-goog
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]
^ permalink raw reply related
* Re: [PATCH v4 0/5] road to reentrant real_path
From: Jeff King @ 2017-01-04 0:48 UTC (permalink / raw)
To: Brandon Williams
Cc: git, sbeller, jacob.keller, gitster, ramsay, tboegi, j6t, pclouds,
larsxschneider
In-Reply-To: <20170103190923.11882-1-bmwill@google.com>
On Tue, Jan 03, 2017 at 11:09:18AM -0800, Brandon Williams wrote:
> Only change with v4 is in [1/5] renaming the #define MAXSYMLINKS back to
> MAXDEPTH due to a naming conflict brought up by Lars Schneider.
Hmm. Isn't MAXSYMLINKS basically what you want here, though? It what's
what all other similar functions will be using.
The only problem was that we were redefining the macro. So maybe:
#ifndef MAXSYMLINKS
#define MAXSYMLINKS 5
#endif
would be a good solution?
It looks like the "usual" value is more like 20 or 30 on most systems,
though. We should probably also set errno to ELOOP when we hit the
limit, which is what other symlink-resolving functions would do.
I'm surprised we didn't hit this on Linux, which has MAXSYMLINKS, too.
We should be picking it up from <sys/param.h>.
-Peff
^ permalink raw reply
* Re: [PATCH] archive-zip: load userdiff config
From: Jeff King @ 2017-01-04 0:40 UTC (permalink / raw)
To: René Scharfe; +Cc: git
In-Reply-To: <7710c564-6b53-1908-7205-210d80eda59b@web.de>
On Tue, Jan 03, 2017 at 06:24:39PM +0100, René Scharfe wrote:
> Am 02.01.2017 um 23:25 schrieb Jeff King:
> > Since 4aff646d17 (archive-zip: mark text files in archives,
> > 2015-03-05), the zip archiver will look at the userdiff
> > driver to decide whether a file is text or binary. This
> > usually doesn't need to look any further than the attributes
> > themselves (e.g., "-diff", etc). But if the user defines a
> > custom driver like "diff=foo", we need to look at
> > "diff.foo.binary" in the config. Prior to this patch, we
> > didn't actually load it.
>
> Ah, didn't think of that, obviously.
>
> Would it make sense for userdiff_find_by_path() to die if userdiff_config()
> hasn't been called, yet?
Yeah, perhaps. That makes it impossible for a program to intentionally
ignore the config. But it looks like even plumbing diff commands load
userdiff (which makes sense; they control its behavior through things
like ALLOW_TEXTCONV). So it's probably fine to have it everywhere.
Other options include:
1. Just loading it always as part of git_default_config.
2. Lazy-loading it on the first call. This seems elegant, though it
does open up hidden cache-invalidation issues. E.g., somebody asks
for userdiff_find_by_path(), we load the values, then they
setup_git_repository(), and we would need to reload. That's
far-fetched for userdiff, but it makes lazy-loading as a general
pattern a bit of a potential maintenance trap.
We could also introduce some infrastructure to deal with that
(e.g., if callers could ask the config machinery "have you been
invalidated"). That would help here and in other places (e.g., I
considered this when dealing with get_shared_repository()).
> > I also happened to notice that zipfiles are created using the local
> > timezone (because they have no notion of the timezone, so we have to
> > pick _something_). That's probably the least-terrible option, but it was
> > certainly surprising to me when I tried to bit-for-bit reproduce a
> > zipfile from GitHub on my local machine.
>
> That reminds me of an old request to allow users better control over the
> meta-data written into archives. Being able to specify a time zone offset
> could be a start.
I did it with:
TZ=PST8PDT git archive ...
which let me get a bit-for-bit match with what GitHub generates. The
real problem was just knowing that I needed to do that. OTOH, we're
considering having GitHub generate all archives in UTC for sanity's
sake, and it would be nice to do that by setting zip.timezone instead of
hacking $TZ for each invocation.
> > +static int archive_zip_config(const char *var, const char *value, void *data)
> > +{
> > + return userdiff_config(var, value);
> > +}
> > +
> > static int write_zip_archive(const struct archiver *ar,
> > struct archiver_args *args)
> > {
> > int err;
> >
> > + git_config(archive_zip_config, NULL);
> > +
>
> I briefly thought about moving this call to archive.c with the rest of the
> config-related stuff, but I agree it's better kept here.
That was my first thought, but there are already two config calls:
write_archive() loads default config, but then archive-tar loads
tar-specific config. Since only zip cares about userdiff, I patterned it
after the latter. But arguably everybody _could_ end up calling into
userdiff. If we take that philosophy, though, I'd be more inclined to
push it into git_default_config(). That covers archive writers _and_ any
other programs which might happen to call into the diff code.
> Looks good, thanks!
Thanks for reviewing.
-Peff
^ permalink raw reply
* Re: [PATCH v4 1/4] Avoid Coverity warning about unfree()d git_exec_path()
From: Stefan Beller @ 2017-01-03 20:11 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git@vger.kernel.org, Junio C Hamano, David Aguilar,
Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <18e9a1009aac2329cb9bf9d12fbac4e8ac19a5bb.1483373635.git.johannes.schindelin@gmx.de>
On Mon, Jan 2, 2017 at 8:22 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> Technically, it is correct that git_exec_path() returns a possibly
> malloc()ed string. Practically, it is *sometimes* not malloc()ed. So
> let's just use a static variable to make it a singleton. That'll shut
> Coverity up, hopefully.
I picked up this patch and applied it to the coverity branch
that I maintain at github/stefanbeller/git.
I'd love to see this patch upstream as it reduces my maintenance
burden of the coverity branch by a patch.
Early on when Git was new to coverity, some arguments were made
that patches like these only clutter the main code base which is read
by a lot of people, hence we want these quirks for coverity not upstream.
And I think that still holds.
If this patch is only to appease coverity (as the commit message eludes
to) I think this may be a bad idea for upstream. If this patch fixes an
actual problem, then the commit message needs to spell that out.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v2] git-p4: do not pass '-r 0' to p4 commands
From: Ori Rawlings @ 2017-01-03 20:02 UTC (permalink / raw)
To: Igor Kushnir; +Cc: Git Users, Lars Schneider, Luke Diamand
In-Reply-To: <20161229102223.6028-1-igorkuo@gmail.com>
Looks good to me.
Ori Rawlings
^ permalink raw reply
* [PATCH v3 2/2] t9813: avoid using pipes
From: Pranit Bauva @ 2017-01-03 19:57 UTC (permalink / raw)
To: git; +Cc: luke, sbeller, j6t, Pranit Bauva
In-Reply-To: <20170102184536.10488-1-pranit.bauva@gmail.com>
The exit code of the upstream in a pipe is ignored thus we should avoid
using it. By writing out the output of the git command to a file, we can
test the exit codes of both the commands.
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
t/t9813-git-p4-preserve-users.sh | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/t/t9813-git-p4-preserve-users.sh b/t/t9813-git-p4-preserve-users.sh
index 798bf2b67..2133b21ae 100755
--- a/t/t9813-git-p4-preserve-users.sh
+++ b/t/t9813-git-p4-preserve-users.sh
@@ -118,12 +118,12 @@ test_expect_success 'not preserving user with mixed authorship' '
make_change_by_user usernamefile3 Derek derek@example.com &&
P4EDITOR=cat P4USER=alice P4PASSWD=secret &&
export P4EDITOR P4USER P4PASSWD &&
- git p4 commit |\
- grep "git author derek@example.com does not match" &&
+ git p4 commit >actual &&
+ grep "git author derek@example.com does not match" actual &&
make_change_by_user usernamefile3 Charlie charlie@example.com &&
- git p4 commit |\
- grep "git author charlie@example.com does not match" &&
+ git p4 commit >actual &&
+ grep "git author charlie@example.com does not match" actual &&
make_change_by_user usernamefile3 alice alice@example.com &&
git p4 commit >actual 2>&1 &&
--
2.11.0
^ permalink raw reply related
* [PATCH v3 1/2] don't use test_must_fail with grep
From: Pranit Bauva @ 2017-01-03 19:57 UTC (permalink / raw)
To: git; +Cc: luke, sbeller, j6t, Pranit Bauva
In-Reply-To: <20170102184536.10488-1-pranit.bauva@gmail.com>
test_must_fail should only be used for testing git commands. To test the
failure of other commands use `!`.
Reported-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
t/t3510-cherry-pick-sequence.sh | 6 +++---
t/t5504-fetch-receive-strict.sh | 2 +-
t/t5516-fetch-push.sh | 2 +-
t/t5601-clone.sh | 2 +-
t/t6030-bisect-porcelain.sh | 2 +-
t/t7610-mergetool.sh | 2 +-
t/t9001-send-email.sh | 2 +-
t/t9117-git-svn-init-clone.sh | 12 ++++++------
t/t9813-git-p4-preserve-users.sh | 8 ++++----
t/t9814-git-p4-rename.sh | 6 +++---
10 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 372307c21..0acf4b146 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -385,7 +385,7 @@ test_expect_success '--continue respects opts' '
git cat-file commit HEAD~1 >picked_msg &&
git cat-file commit HEAD~2 >unrelatedpick_msg &&
git cat-file commit HEAD~3 >initial_msg &&
- test_must_fail grep "cherry picked from" initial_msg &&
+ ! grep "cherry picked from" initial_msg &&
grep "cherry picked from" unrelatedpick_msg &&
grep "cherry picked from" picked_msg &&
grep "cherry picked from" anotherpick_msg
@@ -426,9 +426,9 @@ test_expect_failure '--signoff is automatically propagated to resolved conflict'
git cat-file commit HEAD~1 >picked_msg &&
git cat-file commit HEAD~2 >unrelatedpick_msg &&
git cat-file commit HEAD~3 >initial_msg &&
- test_must_fail grep "Signed-off-by:" initial_msg &&
+ ! grep "Signed-off-by:" initial_msg &&
grep "Signed-off-by:" unrelatedpick_msg &&
- test_must_fail grep "Signed-off-by:" picked_msg &&
+ ! grep "Signed-off-by:" picked_msg &&
grep "Signed-off-by:" anotherpick_msg
'
diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 9b19cff72..49d3621a9 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -152,7 +152,7 @@ test_expect_success 'push with receive.fsck.missingEmail=warn' '
git --git-dir=dst/.git config --add \
receive.fsck.badDate warn &&
git push --porcelain dst bogus >act 2>&1 &&
- test_must_fail grep "missingEmail" act
+ ! grep "missingEmail" act
'
test_expect_success \
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 26b2cafc4..0fc5a7c59 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1004,7 +1004,7 @@ test_expect_success 'push --porcelain' '
test_expect_success 'push --porcelain bad url' '
mk_empty testrepo &&
test_must_fail git push >.git/bar --porcelain asdfasdfasd refs/heads/master:refs/remotes/origin/master &&
- test_must_fail grep -q Done .git/bar
+ ! grep -q Done .git/bar
'
test_expect_success 'push --porcelain rejected' '
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index a43339420..4241ea5b3 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -151,7 +151,7 @@ test_expect_success 'clone --mirror does not repeat tags' '
git clone --mirror src mirror2 &&
(cd mirror2 &&
git show-ref 2> clone.err > clone.out) &&
- test_must_fail grep Duplicate mirror2/clone.err &&
+ ! grep Duplicate mirror2/clone.err &&
grep some-tag mirror2/clone.out
'
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 5e5370feb..8c2c6eaef 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -407,7 +407,7 @@ test_expect_success 'good merge base when good and bad are siblings' '
test_i18ngrep "merge base must be tested" my_bisect_log.txt &&
grep $HASH4 my_bisect_log.txt &&
git bisect good > my_bisect_log.txt &&
- test_must_fail grep "merge base must be tested" my_bisect_log.txt &&
+ ! grep "merge base must be tested" my_bisect_log.txt &&
grep $HASH6 my_bisect_log.txt &&
git bisect reset
'
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 63d36fb28..0fe7e58cf 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -602,7 +602,7 @@ test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToT
test_config mergetool.myecho.trustExitCode true &&
test_must_fail git merge master &&
git mergetool --no-prompt --tool myecho -- both >actual &&
- test_must_fail grep ^\./both_LOCAL_ actual >/dev/null &&
+ ! grep ^\./both_LOCAL_ actual >/dev/null &&
grep /both_LOCAL_ actual >/dev/null &&
git reset --hard master >/dev/null 2>&1
'
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 3dc4a3454..0f398dd16 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -50,7 +50,7 @@ test_no_confirm () {
--smtp-server="$(pwd)/fake.sendmail" \
$@ \
$patches >stdout &&
- test_must_fail grep "Send this email" stdout &&
+ ! grep "Send this email" stdout &&
>no_confirm_okay
}
diff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh
index 69a675052..044f65e91 100755
--- a/t/t9117-git-svn-init-clone.sh
+++ b/t/t9117-git-svn-init-clone.sh
@@ -55,7 +55,7 @@ test_expect_success 'clone to target directory with --stdlayout' '
test_expect_success 'init without -s/-T/-b/-t does not warn' '
test ! -d trunk &&
git svn init "$svnrepo"/project/trunk trunk 2>warning &&
- test_must_fail grep -q prefix warning &&
+ ! grep -q prefix warning &&
rm -rf trunk &&
rm -f warning
'
@@ -63,7 +63,7 @@ test_expect_success 'init without -s/-T/-b/-t does not warn' '
test_expect_success 'clone without -s/-T/-b/-t does not warn' '
test ! -d trunk &&
git svn clone "$svnrepo"/project/trunk 2>warning &&
- test_must_fail grep -q prefix warning &&
+ ! grep -q prefix warning &&
rm -rf trunk &&
rm -f warning
'
@@ -86,7 +86,7 @@ EOF
test_expect_success 'init with -s/-T/-b/-t assumes --prefix=origin/' '
test ! -d project &&
git svn init -s "$svnrepo"/project project 2>warning &&
- test_must_fail grep -q prefix warning &&
+ ! grep -q prefix warning &&
test_svn_configured_prefix "origin/" &&
rm -rf project &&
rm -f warning
@@ -95,7 +95,7 @@ test_expect_success 'init with -s/-T/-b/-t assumes --prefix=origin/' '
test_expect_success 'clone with -s/-T/-b/-t assumes --prefix=origin/' '
test ! -d project &&
git svn clone -s "$svnrepo"/project 2>warning &&
- test_must_fail grep -q prefix warning &&
+ ! grep -q prefix warning &&
test_svn_configured_prefix "origin/" &&
rm -rf project &&
rm -f warning
@@ -104,7 +104,7 @@ test_expect_success 'clone with -s/-T/-b/-t assumes --prefix=origin/' '
test_expect_success 'init with -s/-T/-b/-t and --prefix "" still works' '
test ! -d project &&
git svn init -s "$svnrepo"/project project --prefix "" 2>warning &&
- test_must_fail grep -q prefix warning &&
+ ! grep -q prefix warning &&
test_svn_configured_prefix "" &&
rm -rf project &&
rm -f warning
@@ -113,7 +113,7 @@ test_expect_success 'init with -s/-T/-b/-t and --prefix "" still works' '
test_expect_success 'clone with -s/-T/-b/-t and --prefix "" still works' '
test ! -d project &&
git svn clone -s "$svnrepo"/project --prefix "" 2>warning &&
- test_must_fail grep -q prefix warning &&
+ ! grep -q prefix warning &&
test_svn_configured_prefix "" &&
rm -rf project &&
rm -f warning
diff --git a/t/t9813-git-p4-preserve-users.sh b/t/t9813-git-p4-preserve-users.sh
index 0fe231280..798bf2b67 100755
--- a/t/t9813-git-p4-preserve-users.sh
+++ b/t/t9813-git-p4-preserve-users.sh
@@ -126,13 +126,13 @@ test_expect_success 'not preserving user with mixed authorship' '
grep "git author charlie@example.com does not match" &&
make_change_by_user usernamefile3 alice alice@example.com &&
- git p4 commit |\
- test_must_fail grep "git author.*does not match" &&
+ git p4 commit >actual 2>&1 &&
+ ! grep "git author.*does not match" actual &&
git config git-p4.skipUserNameCheck true &&
make_change_by_user usernamefile3 Charlie charlie@example.com &&
- git p4 commit |\
- test_must_fail grep "git author.*does not match" &&
+ git p4 commit >actual 2>&1 &&
+ ! grep "git author.*does not match" actual &&
p4_check_commit_author usernamefile3 alice
)
diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
index c89992cf9..e7e0268e9 100755
--- a/t/t9814-git-p4-rename.sh
+++ b/t/t9814-git-p4-rename.sh
@@ -141,7 +141,7 @@ test_expect_success 'detect copies' '
git diff-tree -r -C HEAD &&
git p4 submit &&
p4 filelog //depot/file8 &&
- p4 filelog //depot/file8 | test_must_fail grep -q "branch from" &&
+ ! p4 filelog //depot/file8 | grep -q "branch from" &&
echo "file9" >>file2 &&
git commit -a -m "Differentiate file2" &&
@@ -154,7 +154,7 @@ test_expect_success 'detect copies' '
git config git-p4.detectCopies true &&
git p4 submit &&
p4 filelog //depot/file9 &&
- p4 filelog //depot/file9 | test_must_fail grep -q "branch from" &&
+ ! p4 filelog //depot/file9 | grep -q "branch from" &&
echo "file10" >>file2 &&
git commit -a -m "Differentiate file2" &&
@@ -202,7 +202,7 @@ test_expect_success 'detect copies' '
git config git-p4.detectCopies $(($level + 2)) &&
git p4 submit &&
p4 filelog //depot/file12 &&
- p4 filelog //depot/file12 | test_must_fail grep -q "branch from" &&
+ ! p4 filelog //depot/file12 | grep -q "branch from" &&
echo "file13" >>file2 &&
git commit -a -m "Differentiate file2" &&
--
2.11.0
^ permalink raw reply related
* Re: [PATCH v2 2/2] t9813: avoid using pipes
From: Stefan Beller @ 2017-01-03 19:48 UTC (permalink / raw)
To: Pranit Bauva; +Cc: git@vger.kernel.org, luke, Johannes Sixt
In-Reply-To: <CAFZEwPPfE_WSn2QbmER+5mkaC8RnVDs5gsSJE+Y0v-CfYaZB2w@mail.gmail.com>
>
> git p4 commit >actual &&
> grep "git author derek@example.com does not match" actual &&
>
> What do you think?
From the travis logs:
'actual.err' is not empty, it contains:
... - file(s) up-to-date.
I think(/hope) such a progress is tested for at another test,
and not relevant here so I'd think the proposed
git p4 commit >actual &&
grep "git author derek@example.com does not match" actual &&
is fine here.
Thanks,
Stefan
^ permalink raw reply
* Re: [RFC PATCH 0/5] Localise error headers
From: Stefan Beller @ 2017-01-03 19:45 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git@vger.kernel.org
In-Reply-To: <cover.1483354746.git.git@drmicha.warpmail.net>
On Mon, Jan 2, 2017 at 3:14 AM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> Currently, the headers "error: ", "warning: " etc. - generated by die(),
> warning() etc. - are not localized, but we feed many localized error messages
> into these functions so that we produce error messages with mixed localisation.
>
> This series introduces variants of die() etc. that use localised variants of
> the headers, i.e. _("error: ") etc., and are to be fed localized messages. So,
> instead of die(_("not workee")), which would produce a mixed localisation (such
> as "error: geht ned"), one should use die_(_("not workee")) (resulting in
> "Fehler: geht ned").
Have you considered
die_("non localized error here")
to produce:
"Fehler: trotzdem uebersetzt hier"
("error: here it is translated")
>
> In this implementation, the gettext call for the header and the body are done
> in different places (error function vs. caller) but this call pattern seems to
> be the easiest variant for the caller, because the message body has to be marked
> for localisation in any case, and N_() requires more letters than _(), an extra
> argument to die() etc. even more than the extra "_" in the function name.
I see. We have to markup the strings to be translatable such that the .po files
are complete. It would be really handy if there was a way to say "anything that
is fed to this function (die_) needs to be marked for translation.
Looking through
https://www.gnu.org/software/gettext/manual/html_node/xgettext-Invocation.html
such a thing doesn't seem to exist.
So in that case die_(_(...)) seems to be the easiest way forward.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v2 2/2] t9813: avoid using pipes
From: Pranit Bauva @ 2017-01-03 19:44 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, luke, Johannes Sixt
In-Reply-To: <CAGZ79kZRFLzD7wcAnFvke9vBxxTAgE7=Ud7F_O95EfkWqz=LJw@mail.gmail.com>
Hey Stefan,
On Tue, Jan 3, 2017 at 11:28 PM, Stefan Beller <sbeller@google.com> wrote:
> On Mon, Jan 2, 2017 at 10:45 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> The exit code of the upstream in a pipe is ignored thus we should avoid
>> using it.
>
> for commands under test, i.e. git things. Other parts can be piped if that makes
> the test easier. Though I guess that can be guessed by the reader as well,
> as you only convert git commands on upstream pipes.
>
>> By writing out the output of the git command to a file, we can
>> test the exit codes of both the commands.
>>
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>
> Thanks for taking ownership of this issue as well. :)
Welcome! ;)
>> ---
>> t/t9813-git-p4-preserve-users.sh | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/t/t9813-git-p4-preserve-users.sh b/t/t9813-git-p4-preserve-users.sh
>> index 798bf2b67..9d7550ff3 100755
>> --- a/t/t9813-git-p4-preserve-users.sh
>> +++ b/t/t9813-git-p4-preserve-users.sh
>> @@ -118,12 +118,12 @@ test_expect_success 'not preserving user with mixed authorship' '
>> make_change_by_user usernamefile3 Derek derek@example.com &&
>> P4EDITOR=cat P4USER=alice P4PASSWD=secret &&
>> export P4EDITOR P4USER P4PASSWD &&
>> - git p4 commit |\
>> - grep "git author derek@example.com does not match" &&
>> + git p4 commit >actual 2>&1 &&
>
> Why do we need to pipe 2>&1 here?
> Originally the piping only fed the stdout to grep, so this patch changes the
> test? Maybe
>
> 2>actual.err &&
> test_must_be_empty actual.err
>
> instead?
I tried this out but it seems that travis-ci build fails[1]. And I
don't have p4 on my machine to test what's happening actually. But I
just pushed out a few thing modifications to travis and it seems that
actual.err isn't really empty for some reason. So I think, I just
leave it as,
git p4 commit >actual &&
grep "git author derek@example.com does not match" actual &&
What do you think?
[1]: https://travis-ci.org/pranitbauva1997/git/jobs/188633734
Regards,
Pranit Bauva
^ permalink raw reply
* Re: Wanted: shallow submodule clones with --no-single-branch.
From: Stefan Beller @ 2017-01-03 19:21 UTC (permalink / raw)
To: Tor Andersson; +Cc: git@vger.kernel.org
In-Reply-To: <CAAmwXB=M8yZY2sFLwavrrQSEW9bipFhNZyLduwYXtZNK6-Ppxg@mail.gmail.com>
On Fri, Dec 30, 2016 at 2:50 AM, Tor Andersson <tor@ccxvii.net> wrote:
> Hi,
>
> When adding submodules with --depth=1 only the master branch is
> cloned. This often leaves the submodule pointing to a non-existing
> commit.
You can also use the "--branch=not_master" flag to track another branch.
This however doesn't clone the correct branch. I would have expected that
it cloned the correct branch instead.
>
> It would be useful if I could pass the --no-single-branch argument to
> the submodule clone process, since then a submodule can point to any
> tag or branch without ending up in this situation.
Adding --no-single-branch sounds like a good idea for general use,
but it seems like a clunky workaround when looking for a specific branch,
i.e. fixing the --branch flag sounds like the right approach for this
issue here?
> I've got a local
> patch to hardwire the --no-single-branch argument in the
> builtin/submodule--helper.c clone_submodule function, but I'm not sure
> if this will have any other adverse effects?
Well, when asking for depth=1, the user usually actually wants to have the
least amount of data, which is why --depth implies --single-branch
in clone.
So adding it unconditionally is a bad idea IMHO, we'd need to have a flag
for that propagated from git-submodule.sh (function cmd_add) to the
submodule--helpers module_clone.
>
> Better yet would be for the shallow submodule clone to automatically
> retrieve and graft the actual commit the submodule points to, but
> that's probably wishing for too much.
I think fixing the branch option comes a bit closer, but still doesn't
fix this root problem. "submodule update" tries to fetch by sha1
directly in the hope that the server has uploadpack.\
allowReachableSHA1InWant configured.
In "submodule add" I think it is sufficient to take the current remotes
$branch and record that, so I do not see the need in the add code
to support direct sha1s unlike the update command?
Thanks,
Stefan
^ permalink raw reply
* [PATCH v4 16/16] pathspec: rename prefix_pathspec to init_pathspec_item
From: Brandon Williams @ 2017-01-03 18:42 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <20170103184241.128409-1-bmwill@google.com>
Give a more relevant name to the prefix_pathspec function as it does
more than just prefix a pathspec element.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
pathspec.c | 24 +++++++-----------------
1 file changed, 7 insertions(+), 17 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index ae9e1401f..bcf3ba039 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -297,21 +297,11 @@ static void strip_submodule_slash_expensive(struct pathspec_item *item)
}
/*
- * Take an element of a pathspec and check for magic signatures.
- * Append the result to the prefix. Return the magic bitmap.
- *
- * For now, we only parse the syntax and throw out anything other than
- * "top" magic.
- *
- * NEEDSWORK: This needs to be rewritten when we start migrating
- * get_pathspec() users to use the "struct pathspec" interface. For
- * example, a pathspec element may be marked as case-insensitive, but
- * the prefix part must always match literally, and a single stupid
- * string cannot express such a case.
+ * Perform the initialization of a pathspec_item based on a pathspec element.
*/
-static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
- const char *prefix, int prefixlen,
- const char *elt)
+static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
+ const char *prefix, int prefixlen,
+ const char *elt)
{
unsigned magic = 0, element_magic = 0;
const char *copyfrom = elt;
@@ -329,6 +319,8 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
magic |= get_global_magic(element_magic);
}
+ item->magic = magic;
+
if (pathspec_prefix >= 0 &&
(prefixlen || (prefix && *prefix)))
die("BUG: 'prefix' magic is supposed to be used at worktree's root");
@@ -401,7 +393,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
/* sanity checks, pathspec matchers assume these are sane */
assert(item->nowildcard_len <= item->len &&
item->prefix <= item->len);
- return magic;
}
static int pathspec_item_cmp(const void *a_, const void *b_)
@@ -501,8 +492,7 @@ void parse_pathspec(struct pathspec *pathspec,
for (i = 0; i < n; i++) {
entry = argv[i];
- item[i].magic = prefix_pathspec(item + i, flags,
- prefix, prefixlen, entry);
+ init_pathspec_item(item + i, flags, prefix, prefixlen, entry);
if (item[i].magic & PATHSPEC_EXCLUDE)
nr_exclude++;
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox