[PATCH] t/Makefile: make sure that file names are truly platform-independent

[PATCH] t/Makefile: make sure that file names are truly platform-independent

[Johannes Schindelin]

Some file names that are okay on ext4 and on HFS+ are illegal in
Windows. In order to stay truly platform-independent, Git's source code
must not contain such illegal file names, even if things just happen to
work on Linux.

One such file name was recently introduced into Git's `pu` branch:

t4013/diff.diff_--diff-line-prefix=-->_master_master^_sidt4013/diff.diff_--diff-line-prefix=-->_master_master^_sid

It is illegal because it contains the '>' character that is not part of
a valid filename on Windows.

To catch these issues early, let's introduce a new test-lint-* goal that
fails if such file names were detected and run it as part of a `make
test`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/test-lint-filenames-v1
Fetch-It-Via: git fetch https://github.com/dscho/git test-lint-filenames-v1

	This is the more complete fix I talked about.

 t/Makefile | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/t/Makefile b/t/Makefile
index 18e2b28..3c0eb48 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -52,7 +52,8 @@ clean-except-prove-cache:
 clean: clean-except-prove-cache
 	$(RM) .prove
 
-test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax
+test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \
+	test-lint-filenames
 
 test-lint-duplicates:
 	@dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \
@@ -67,6 +68,11 @@ test-lint-executable:
 test-lint-shell-syntax:
 	@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS)
 
+test-lint-filenames:
+	@illegal="$$(git ls-files | grep '["*:<>?\\|]')"; \
+		test -z "$$illegal" || { \
+		echo >&2 "illegal file name(s): " $$illegal; exit 1; }
+
 aggregate-results-and-cleanup: $(T)
 	$(MAKE) aggregate-results
 	$(MAKE) clean
-- 
2.9.2.691.g78954f3

base-commit: 5e18482599a6cfeb8d4e0ee5a98d30220cbdff72

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> Some file names that are okay on ext4 and on HFS+ are illegal in
> Windows. In order to stay truly platform-independent, Git's source code
> must not contain such illegal file names, even if things just happen to
> work on Linux.

Good thinking.

Some tests may have to be skipped on platforms that cannot express
certain paths, but even then they shouldn't ship a file with
pathname that cannot even be checked out (they should instead create
and use such a path, protected behind filesystem specific test
prerequisite).

> +test-lint-filenames:
> +	@illegal="$$(git ls-files | grep '["*:<>?\\|]')"; \

This pattern must exclude questionables on either NTFS or HFS+; it
is ironic that it is not even sufficient to limit ourselves to the
Portable Character Set [*1*], but such is life.

By the way, doesn't ls-files take pathspec glob, saving one extra
process to run grep?

    master$ git ls-files '*["*:<>?\\|]*'
    pu$ git ls-files '*["*:<>?\\|]*'
    t/t4013/diff.diff_--diff-line-prefix=-->_master_master^_side

Thanks.

> +		test -z "$$illegal" || { \
> +		echo >&2 "illegal file name(s): " $$illegal; exit 1; }

[Reference]

*1* http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap06.html#tag_06_01

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
>> Some file names that are okay on ext4 and on HFS+ are illegal in
>> Windows. In order to stay truly platform-independent, Git's source code
>> must not contain such illegal file names, even if things just happen to
>> work on Linux.
>
> Good thinking.
>
> Some tests may have to be skipped on platforms that cannot express
> certain paths, but even then they shouldn't ship a file with
> pathname that cannot even be checked out (they should instead create
> and use such a path, protected behind filesystem specific test
> prerequisite).

This is a tangent.

Adding a check target in t/Makefile is a good first step, but any
tree of a project that gets participation from those on different
platforms must allow any supported platforms to check it out.  The
issue is not limited to the t/ hierarchy, but the whole thing.  I
do not mean to say this patch needs to do more than checking t/ at
all, by the way.  After all, people send patches without running
test-lint so this only means that we as the project must be careful.

I wonder if we already have a good mechanism to allow a project and
its participants (say, "me") to declare "in this project, pathnames
must conform to this rule" and help them avoid creating a tree that
violates the rule customized to their project.

I guess "write_index_as_tree()" would be one of the central places
to hook into and that covers an individual contributor or a patch
applier who ends up adding offending paths to the project, as well
as a merge made in response to a pull request (unless it is a
fast-forward) [*1*].  The pre-receive hook can also be used to
inspect and reject an attempt to push an offending tree into the
history.

Such a mechanism would allow a project that wants participation by
folks with case insensitive filesystems to ensure that they do not
create a directory that has both xt_TCPMSS.h and xt_tcpmss.h at the
same time, for example, but the mechanism needs to allow visibility
into more than just a single path when the custom check is made
(e.g. a hook run in "write_index_as_tree()" can see all entries in
the index to make the decision; if we were to also hook into
"add_to_index()", the hook must be able to see other entries in the
index to which the new entry is being added).


[Footnote]

*1* "add_to_index()" could be another place to hook into, and doing
so has the merit of catching a breakage sooner, but I suspect that
alone may not be sufficient if there are other ways for new entries
to appear in the index.

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Jeff King]

On Mon, Aug 15, 2016 at 09:57:52AM -0700, Junio C Hamano wrote:

> I wonder if we already have a good mechanism to allow a project and
> its participants (say, "me") to declare "in this project, pathnames
> must conform to this rule" and help them avoid creating a tree that
> violates the rule customized to their project.
> 
> I guess "write_index_as_tree()" would be one of the central places
> to hook into and that covers an individual contributor or a patch
> applier who ends up adding offending paths to the project, as well
> as a merge made in response to a pull request (unless it is a
> fast-forward) [*1*].  The pre-receive hook can also be used to
> inspect and reject an attempt to push an offending tree into the
> history.
> 
> Such a mechanism would allow a project that wants participation by
> folks with case insensitive filesystems to ensure that they do not
> create a directory that has both xt_TCPMSS.h and xt_tcpmss.h at the
> same time, for example, but the mechanism needs to allow visibility
> into more than just a single path when the custom check is made
> (e.g. a hook run in "write_index_as_tree()" can see all entries in
> the index to make the decision; if we were to also hook into
> "add_to_index()", the hook must be able to see other entries in the
> index to which the new entry is being added).

I am not convinced this mechanism needs to be built into git. Because it
happens to be about filenames, git at least has a hope of making sense
of the various project rules.

But conceptually, I don't think this is really any different than "don't
check in code which does not build on platform X", or "do not check in
code that does not meet our style guide". We already have general hooks
where such checks can be made[1], and this could be checked in the same
place.

I actually think the whitespace-checking done by diff and apply is in a
similar boat, though it is useful in practice. OTOH, I think primarily
it is used as a tool to feed information to policy hooks, rather than as
an enforcement mechanism itself (maybe --whitespace=fix on apply is an
exception there, though).

-Peff

[1] Obviously we have pre-commit for local enforcement and pre-receive
    for central enforcement. But people with workflows around CI-style
    tests would want to be able to fetch, check "does this meet our
    policy", and return a yes/no answer on whether the result is OK to
    merge.

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Good thinking.
>
> Some tests may have to be skipped on platforms that cannot express
> certain paths, but even then they shouldn't ship a file with
> pathname that cannot even be checked out (they should instead create
> and use such a path, protected behind filesystem specific test
> prerequisite).
>
>> +test-lint-filenames:
>> +	@illegal="$$(git ls-files | grep '["*:<>?\\|]')"; \
>
> This pattern must exclude questionables on either NTFS or HFS+; it
> is ironic that it is not even sufficient to limit ourselves to the
> Portable Character Set [*1*], but such is life.
>
> By the way, doesn't ls-files take pathspec glob, saving one extra
> process to run grep?
>
>     master$ git ls-files '*["*:<>?\\|]*'
>     pu$ git ls-files '*["*:<>?\\|]*'
>     t/t4013/diff.diff_--diff-line-prefix=-->_master_master^_side

One more thing you may want to exclude is HT.  Here is a suggested
reroll.  I reworded to avoid a subjective "truly platform-independent",
which is not what we intend to aim for anyway (we only try to support
the platforms we care about).

-- >8 --
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mon, 15 Aug 2016 16:08:41 +0200
Subject: [PATCH] t/Makefile: make sure that paths can be checked out on
 platforms we care

Some pathnames that are okay on ext4 and on HFS+ cannot be checked
out on Windows.  Tests that want to see operations on such paths on
filesystems that support them must do so behind appropriate test
prerequisites, and must not include them in the source tree (instead
they should create them when they run).  Otherwise, the source tree
cannot be even checked out.

Make sure that double-quotes, asterisk, colon, greater/less-than,
question-mark, backslash, tab and vertical-bar never appears in the
pathnames with a new test-lint-* target as part of a `make test`.

Noticed when a topic wanted to add a pathname with '>' in it.  A
check like this will prevent a similar problems from happening.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/Makefile | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/t/Makefile b/t/Makefile
index 18e2b28..d4b2a50 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -52,7 +52,8 @@ clean-except-prove-cache:
 clean: clean-except-prove-cache
 	$(RM) .prove
 
-test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax
+test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \
+	test-lint-filenames
 
 test-lint-duplicates:
 	@dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \
@@ -67,6 +68,11 @@ test-lint-executable:
 test-lint-shell-syntax:
 	@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS)
 
+test-lint-filenames:
+	@bad=$$(git ls-files '*[	"*:<>?\\|]*'); \
+		test -z "$$bad" || { \
+		echo >&2 "do not use non-portable file name(s): $$bad"; exit 1; }
+
 aggregate-results-and-cleanup: $(T)
 	$(MAKE) aggregate-results
 	$(MAKE) clean
-- 
2.10.0-rc0-132-gce76bc9

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 23141 bytes --]

Hi Junio,

On Mon, 15 Aug 2016, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> >> +test-lint-filenames:
> >> +	@illegal="$$(git ls-files | grep '["*:<>?\\|]')"; \
> >
> > This pattern must exclude questionables on either NTFS or HFS+; it
> > is ironic that it is not even sufficient to limit ourselves to the
> > Portable Character Set [*1*], but such is life.
> >
> > By the way, doesn't ls-files take pathspec glob, saving one extra
> > process to run grep?

I specifically did not do that, sorry for omitting the rationale from the
commit message. The reason why I have that grep is so that the backslash
can also catch non-ASCII characters, such as "Hellö-Jüniö".

> One more thing you may want to exclude is HT.  Here is a suggested
> reroll.  I reworded to avoid a subjective "truly platform-independent",
> which is not what we intend to aim for anyway (we only try to support
> the platforms we care about).

Unfortunately, this version (actually, the version of `pu`, which I
assume is identical) does not work correctly:

$ make test-lint
do not use non-portable file name(s): Git-SVN/00compile.t
Git-SVN/Utils/add_path_to_url.t
Git-SVN/Utils/can_compress.t
Git-SVN/Utils/canonicalize_url.t
Git-SVN/Utils/collapse_dotdot.t
Git-SVN/Utils/fatal.t
Git-SVN/Utils/join_paths.t
diff-lib/COPYING
diff-lib/README
helper/.gitignore
helper/test-chmtime.c
helper/test-config.c
helper/test-ctype.c
helper/test-date.c
helper/test-delta.c
helper/test-dump-cache-tree.c
helper/test-dump-split-index.c
helper/test-dump-untracked-cache.c
helper/test-dump-watchman.c
helper/test-fake-ssh.c
helper/test-genrandom.c
helper/test-hashmap.c
helper/test-index-version.c
helper/test-line-buffer.c
helper/test-match-trees.c
helper/test-mergesort.c
helper/test-mktemp.c
helper/test-parse-options.c
helper/test-path-utils.c
helper/test-prio-queue.c
helper/test-read-cache.c
helper/test-regex.c
helper/test-revision-walking.c
helper/test-run-command.c
helper/test-scrap-cache-tree.c
helper/test-sha1-array.c
helper/test-sha1.c
helper/test-sha1.sh
helper/test-sigchain.c
helper/test-string-list.c
helper/test-submodule-config.c
helper/test-subprocess.c
helper/test-svn-fe.c
helper/test-urlmatch-normalization.c
helper/test-wildmatch.c
lib-gpg/keyring.gpg
lib-gpg/ownertrust
lib-httpd/apache.conf
lib-httpd/broken-smart-http.sh
lib-httpd/error.sh
lib-httpd/passwd
lib-httpd/ssl.cnf
perf/.gitignore
perf/Makefile
perf/README
perf/aggregate.perl
perf/min_time.perl
perf/p0000-perf-lib-sanity.sh
perf/p0001-rev-list.sh
perf/p0002-read-cache.sh
perf/p3400-rebase.sh
perf/p3404-rebase-interactive.sh
perf/p4000-diff-algorithms.sh
perf/p4001-diff-no-index.sh
perf/p4211-line-log.sh
perf/p5302-pack-index.sh
perf/p5303-many-packs.sh
perf/p5310-pack-bitmaps.sh
perf/p7000-filter-branch.sh
perf/p7300-clean.sh
perf/p7810-grep.sh
perf/perf-lib.sh
perf/run
t0110/README
t0110/url-1
t0110/url-10
t0110/url-11
t0110/url-2
t0110/url-3
t0110/url-4
t0110/url-5
t0110/url-6
t0110/url-7
t0110/url-8
t0110/url-9
t0200/test.c
t0200/test.perl
t0200/test.sh
t0202/test.pl
t1509/excludes
t1509/prepare-chroot.sh
t3900/1-UTF-8.txt
t3900/2-UTF-8.txt
t3900/ISO-2022-JP.txt
t3900/ISO8859-1.txt
t3900/UTF-16.txt
t3900/eucJP.txt
t4013/diff.config_format.subjectprefix_DIFFERENT_PREFIX
t4013/diff.diff-tree_--cc_--patch-with-stat_--summary_master
t4013/diff.diff-tree_--cc_--patch-with-stat_--summary_side
t4013/diff.diff-tree_--cc_--patch-with-stat_master
t4013/diff.diff-tree_--cc_--stat_--summary_master
t4013/diff.diff-tree_--cc_--stat_--summary_side
t4013/diff.diff-tree_--cc_--stat_master
t4013/diff.diff-tree_--cc_master
t4013/diff.diff-tree_--patch-with-raw_initial
t4013/diff.diff-tree_--patch-with-stat_initial
t4013/diff.diff-tree_--pretty=oneline_--patch-with-raw_initial
t4013/diff.diff-tree_--pretty=oneline_--patch-with-stat_initial
t4013/diff.diff-tree_--pretty=oneline_--root_--patch-with-raw_initial
t4013/diff.diff-tree_--pretty=oneline_--root_--patch-with-stat_initial
t4013/diff.diff-tree_--pretty=oneline_--root_-p_initial
t4013/diff.diff-tree_--pretty=oneline_--root_initial
t4013/diff.diff-tree_--pretty=oneline_-p_initial
t4013/diff.diff-tree_--pretty=oneline_initial
t4013/diff.diff-tree_--pretty_--patch-with-raw_initial
t4013/diff.diff-tree_--pretty_--patch-with-stat_initial
t4013/diff.diff-tree_--pretty_--patch-with-stat_side
t4013/diff.diff-tree_--pretty_--root_--patch-with-raw_initial
t4013/diff.diff-tree_--pretty_--root_--patch-with-stat_initial
t4013/diff.diff-tree_--pretty_--root_--stat_--summary_initial
t4013/diff.diff-tree_--pretty_--root_--stat_initial
t4013/diff.diff-tree_--pretty_--root_--summary_-r_initial
t4013/diff.diff-tree_--pretty_--root_--summary_initial
t4013/diff.diff-tree_--pretty_--root_-p_initial
t4013/diff.diff-tree_--pretty_--root_initial
t4013/diff.diff-tree_--pretty_--stat_--summary_initial
t4013/diff.diff-tree_--pretty_--stat_initial
t4013/diff.diff-tree_--pretty_--summary_initial
t4013/diff.diff-tree_--pretty_-p_initial
t4013/diff.diff-tree_--pretty_-p_side
t4013/diff.diff-tree_--pretty_initial
t4013/diff.diff-tree_--pretty_side
t4013/diff.diff-tree_--root_--abbrev_initial
t4013/diff.diff-tree_--root_--patch-with-raw_initial
t4013/diff.diff-tree_--root_--patch-with-stat_initial
t4013/diff.diff-tree_--root_-p_initial
t4013/diff.diff-tree_--root_-r_--abbrev=4_initial
t4013/diff.diff-tree_--root_-r_--abbrev_initial
t4013/diff.diff-tree_--root_-r_initial
t4013/diff.diff-tree_--root_initial
t4013/diff.diff-tree_-c_--abbrev_master
t4013/diff.diff-tree_-c_--stat_--summary_master
t4013/diff.diff-tree_-c_--stat_--summary_side
t4013/diff.diff-tree_-c_--stat_master
t4013/diff.diff-tree_-c_master
t4013/diff.diff-tree_-p_-m_master
t4013/diff.diff-tree_-p_initial
t4013/diff.diff-tree_-p_master
t4013/diff.diff-tree_-r_--abbrev=4_initial
t4013/diff.diff-tree_-r_--abbrev_initial
t4013/diff.diff-tree_-r_initial
t4013/diff.diff-tree_initial
t4013/diff.diff-tree_master
t4013/diff.diff_--abbrev_initial..side
t4013/diff.diff_--cached
t4013/diff.diff_--cached_--_file0
t4013/diff.diff_--dirstat-by-file_initial_rearrange
t4013/diff.diff_--dirstat_initial_rearrange
t4013/diff.diff_--dirstat_master~1_master~2
t4013/diff.diff_--name-status_dir2_dir
t4013/diff.diff_--no-index_--name-status_--_dir2_dir
t4013/diff.diff_--no-index_--name-status_dir2_dir
t4013/diff.diff_--no-index_dir_dir3
t4013/diff.diff_--patch-with-raw_-r_initial..side
t4013/diff.diff_--patch-with-raw_initial..side
t4013/diff.diff_--patch-with-stat_-r_initial..side
t4013/diff.diff_--patch-with-stat_initial..side
t4013/diff.diff_--stat_initial..side
t4013/diff.diff_-r_--stat_initial..side
t4013/diff.diff_-r_initial..side
t4013/diff.diff_initial..side
t4013/diff.diff_master_master^_side
t4013/diff.format-patch_--attach_--stdout_--suffix=.diff_initial..side
t4013/diff.format-patch_--attach_--stdout_initial..master
t4013/diff.format-patch_--attach_--stdout_initial..master^
t4013/diff.format-patch_--attach_--stdout_initial..side
t4013/diff.format-patch_--inline_--stdout_--numbered-files_initial..master
t4013/diff.format-patch_--inline_--stdout_--subject-prefix=TESTCASE_initial..master
t4013/diff.format-patch_--inline_--stdout_initial..master
t4013/diff.format-patch_--inline_--stdout_initial..master^
t4013/diff.format-patch_--inline_--stdout_initial..master^^
t4013/diff.format-patch_--inline_--stdout_initial..side
t4013/diff.format-patch_--stdout_--cover-letter_-n_initial..master^
t4013/diff.format-patch_--stdout_--no-numbered_initial..master
t4013/diff.format-patch_--stdout_--numbered_initial..master
t4013/diff.format-patch_--stdout_initial..master
t4013/diff.format-patch_--stdout_initial..master^
t4013/diff.format-patch_--stdout_initial..side
t4013/diff.log_--decorate=full_--all
t4013/diff.log_--decorate_--all
t4013/diff.log_--patch-with-stat_--summary_master_--_dir_
t4013/diff.log_--patch-with-stat_master
t4013/diff.log_--patch-with-stat_master_--_dir_
t4013/diff.log_--root_--cc_--patch-with-stat_--summary_master
t4013/diff.log_--root_--patch-with-stat_--summary_master
t4013/diff.log_--root_--patch-with-stat_master
t4013/diff.log_--root_-c_--patch-with-stat_--summary_master
t4013/diff.log_--root_-p_master
t4013/diff.log_--root_master
t4013/diff.log_-GF_-p_--pickaxe-all_master
t4013/diff.log_-GF_-p_master
t4013/diff.log_-GF_master
t4013/diff.log_-SF_-p_master
t4013/diff.log_-SF_master
t4013/diff.log_-SF_master_--max-count=0
t4013/diff.log_-SF_master_--max-count=1
t4013/diff.log_-SF_master_--max-count=2
t4013/diff.log_-S_F_master
t4013/diff.log_-m_-p_--first-parent_master
t4013/diff.log_-m_-p_master
t4013/diff.log_-p_--first-parent_master
t4013/diff.log_-p_master
t4013/diff.log_master
t4013/diff.rev-list_--children_HEAD
t4013/diff.rev-list_--parents_HEAD
t4013/diff.show_--first-parent_master
t4013/diff.show_--patch-with-raw_side
t4013/diff.show_--patch-with-stat_--summary_side
t4013/diff.show_--patch-with-stat_side
t4013/diff.show_--root_initial
t4013/diff.show_--stat_--summary_side
t4013/diff.show_--stat_side
t4013/diff.show_-c_master
t4013/diff.show_-m_master
t4013/diff.show_initial
t4013/diff.show_master
t4013/diff.show_side
t4013/diff.whatchanged_--patch-with-stat_--summary_master_--_dir_
t4013/diff.whatchanged_--patch-with-stat_master
t4013/diff.whatchanged_--patch-with-stat_master_--_dir_
t4013/diff.whatchanged_--root_--cc_--patch-with-stat_--summary_master
t4013/diff.whatchanged_--root_--patch-with-stat_--summary_master
t4013/diff.whatchanged_--root_--patch-with-stat_master
t4013/diff.whatchanged_--root_-c_--patch-with-stat_--summary_master
t4013/diff.whatchanged_--root_-p_master
t4013/diff.whatchanged_--root_master
t4013/diff.whatchanged_-SF_-p_master
t4013/diff.whatchanged_-SF_master
t4013/diff.whatchanged_-p_master
t4013/diff.whatchanged_master
t4018/README
t4018/cpp-c++-function
t4018/cpp-class-constructor
t4018/cpp-class-constructor-mem-init
t4018/cpp-class-definition
t4018/cpp-class-definition-derived
t4018/cpp-class-destructor
t4018/cpp-function-returning-global-type
t4018/cpp-function-returning-nested
t4018/cpp-function-returning-pointer
t4018/cpp-function-returning-reference
t4018/cpp-gnu-style-function
t4018/cpp-namespace-definition
t4018/cpp-operator-definition
t4018/cpp-skip-access-specifiers
t4018/cpp-skip-comment-block
t4018/cpp-skip-labels
t4018/cpp-struct-definition
t4018/cpp-struct-single-line
t4018/cpp-template-function-definition
t4018/cpp-union-definition
t4018/cpp-void-c-function
t4018/css-brace-in-col-1
t4018/css-colon-eol
t4018/css-colon-selector
t4018/css-common
t4018/css-long-selector-list
t4018/css-prop-sans-indent
t4018/css-short-selector-list
t4018/css-trailing-space
t4018/custom1-pattern
t4018/custom2-match-to-end-of-line
t4018/custom3-alternation-in-pattern
t4018/fountain-scene
t4018/java-class-member-function
t4018/perl-skip-end-of-heredoc
t4018/perl-skip-forward-decl
t4018/perl-skip-sub-in-pod
t4018/perl-sub-definition
t4018/perl-sub-definition-kr-brace
t4020/diff.NUL
t4034/ada/expect
t4034/ada/post
t4034/ada/pre
t4034/bibtex/expect
t4034/bibtex/post
t4034/bibtex/pre
t4034/cpp/expect
t4034/cpp/post
t4034/cpp/pre
t4034/csharp/expect
t4034/csharp/post
t4034/csharp/pre
t4034/css/expect
t4034/css/post
t4034/css/pre
t4034/fortran/expect
t4034/fortran/post
t4034/fortran/pre
t4034/html/expect
t4034/html/post
t4034/html/pre
t4034/java/expect
t4034/java/post
t4034/java/pre
t4034/matlab/expect
t4034/matlab/post
t4034/matlab/pre
t4034/objc/expect
t4034/objc/post
t4034/objc/pre
t4034/pascal/expect
t4034/pascal/post
t4034/pascal/pre
t4034/perl/expect
t4034/perl/post
t4034/perl/pre
t4034/php/expect
t4034/php/post
t4034/php/pre
t4034/python/expect
t4034/python/post
t4034/python/pre
t4034/ruby/expect
t4034/ruby/post
t4034/ruby/pre
t4034/tex/expect
t4034/tex/post
t4034/tex/pre
t4051/appended1.c
t4051/appended2.c
t4051/dummy.c
t4051/hello.c
t4051/includes.c
t4100/t-apply-1.expect
t4100/t-apply-1.patch
t4100/t-apply-2.expect
t4100/t-apply-2.patch
t4100/t-apply-3.expect
t4100/t-apply-3.patch
t4100/t-apply-4.expect
t4100/t-apply-4.patch
t4100/t-apply-5.expect
t4100/t-apply-5.patch
t4100/t-apply-6.expect
t4100/t-apply-6.patch
t4100/t-apply-7.expect
t4100/t-apply-7.patch
t4100/t-apply-8.expect
t4100/t-apply-8.patch
t4100/t-apply-9.expect
t4100/t-apply-9.patch
t4101/diff.0-1
t4101/diff.0-2
t4101/diff.0-3
t4101/diff.1-0
t4101/diff.1-2
t4101/diff.1-3
t4101/diff.2-0
t4101/diff.2-1
t4101/diff.2-3
t4101/diff.3-0
t4101/diff.3-1
t4101/diff.3-2
t4109/expect-1
t4109/expect-2
t4109/expect-3
t4109/patch1.patch
t4109/patch2.patch
t4109/patch3.patch
t4109/patch4.patch
t4110/expect
t4110/patch1.patch
t4110/patch2.patch
t4110/patch3.patch
t4110/patch4.patch
t4110/patch5.patch
t4135/.gitignore
t4135/add-plain.diff
t4135/add-with backslash.diff
t4135/add-with quote.diff
t4135/add-with spaces.diff
t4135/add-with tab.diff
t4135/damaged-tz.diff
t4135/damaged.diff
t4135/diff-plain.diff
t4135/diff-with backslash.diff
t4135/diff-with quote.diff
t4135/diff-with spaces.diff
t4135/diff-with tab.diff
t4135/funny-tz.diff
t4135/git-plain.diff
t4135/git-with backslash.diff
t4135/git-with quote.diff
t4135/git-with spaces.diff
t4135/git-with tab.diff
t4135/make-patches
t4211/expect.beginning-of-file
t4211/expect.end-of-file
t4211/expect.move-support-f
t4211/expect.multiple
t4211/expect.multiple-overlapping
t4211/expect.multiple-superset
t4211/expect.parallel-change-f-to-main
t4211/expect.simple-f
t4211/expect.simple-f-to-main
t4211/expect.simple-main
t4211/expect.simple-main-to-end
t4211/expect.two-ranges
t4211/expect.vanishes-early
t4211/history.export
t4252/am-test-1-1
t4252/am-test-1-2
t4252/am-test-2-1
t4252/am-test-2-2
t4252/am-test-3-1
t4252/am-test-3-2
t4252/am-test-4-1
t4252/am-test-4-2
t4252/am-test-5-1
t4252/am-test-5-2
t4252/am-test-6-1
t4252/file-1-0
t4252/file-2-0
t5000/19f9c8273ec45a8938e6999cb59b3ff66739902a
t5000/huge-and-future.tar
t5000/pax.tar
t5003/infozip-symlinks.zip
t5004/empty-with-pax-header.tar
t5004/empty.zip
t5100/.gitattributes
t5100/0001mboxrd
t5100/0002mboxrd
t5100/embed-from.expect
t5100/embed-from.in
t5100/empty
t5100/info-from.expect
t5100/info-from.in
t5100/info0001
t5100/info0002
t5100/info0003
t5100/info0004
t5100/info0005
t5100/info0006
t5100/info0007
t5100/info0008
t5100/info0009
t5100/info0010
t5100/info0011
t5100/info0012
t5100/info0012--message-id
t5100/info0013
t5100/info0014
t5100/info0014--scissors
t5100/info0015
t5100/info0015--no-inbody-headers
t5100/info0016
t5100/info0016--no-inbody-headers
t5100/info0017
t5100/msg0001
t5100/msg0002
t5100/msg0003
t5100/msg0004
t5100/msg0005
t5100/msg0006
t5100/msg0007
t5100/msg0008
t5100/msg0009
t5100/msg0010
t5100/msg0011
t5100/msg0012
t5100/msg0012--message-id
t5100/msg0013
t5100/msg0014
t5100/msg0014--scissors
t5100/msg0015
t5100/msg0015--no-inbody-headers
t5100/msg0016
t5100/msg0016--no-inbody-headers
t5100/msg0017
t5100/nul-b64.expect
t5100/nul-b64.in
t5100/nul-plain
t5100/patch0001
t5100/patch0002
t5100/patch0003
t5100/patch0004
t5100/patch0005
t5100/patch0006
t5100/patch0007
t5100/patch0008
t5100/patch0009
t5100/patch0010
t5100/patch0011
t5100/patch0012
t5100/patch0012--message-id
t5100/patch0013
t5100/patch0014
t5100/patch0014--scissors
t5100/patch0015
t5100/patch0015--no-inbody-headers
t5100/patch0016
t5100/patch0016--no-inbody-headers
t5100/patch0017
t5100/quoted-from.expect
t5100/quoted-from.in
t5100/rfc2047-info-0001
t5100/rfc2047-info-0002
t5100/rfc2047-info-0003
t5100/rfc2047-info-0004
t5100/rfc2047-info-0005
t5100/rfc2047-info-0006
t5100/rfc2047-info-0007
t5100/rfc2047-info-0008
t5100/rfc2047-info-0009
t5100/rfc2047-info-0010
t5100/rfc2047-info-0011
t5100/rfc2047-samples.mbox
t5100/sample.mbox
t5100/sample.mboxrd
t5515/fetch.br-branches-default
t5515/fetch.br-branches-default-merge
t5515/fetch.br-branches-default-merge_branches-default
t5515/fetch.br-branches-default-octopus
t5515/fetch.br-branches-default-octopus_branches-default
t5515/fetch.br-branches-default_branches-default
t5515/fetch.br-branches-one
t5515/fetch.br-branches-one-merge
t5515/fetch.br-branches-one-merge_branches-one
t5515/fetch.br-branches-one-octopus
t5515/fetch.br-branches-one-octopus_branches-one
t5515/fetch.br-branches-one_branches-one
t5515/fetch.br-config-explicit
t5515/fetch.br-config-explicit-merge
t5515/fetch.br-config-explicit-merge_config-explicit
t5515/fetch.br-config-explicit-octopus
t5515/fetch.br-config-explicit-octopus_config-explicit
t5515/fetch.br-config-explicit_config-explicit
t5515/fetch.br-config-glob
t5515/fetch.br-config-glob-merge
t5515/fetch.br-config-glob-merge_config-glob
t5515/fetch.br-config-glob-octopus
t5515/fetch.br-config-glob-octopus_config-glob
t5515/fetch.br-config-glob_config-glob
t5515/fetch.br-remote-explicit
t5515/fetch.br-remote-explicit-merge
t5515/fetch.br-remote-explicit-merge_remote-explicit
t5515/fetch.br-remote-explicit-octopus
t5515/fetch.br-remote-explicit-octopus_remote-explicit
t5515/fetch.br-remote-explicit_remote-explicit
t5515/fetch.br-remote-glob
t5515/fetch.br-remote-glob-merge
t5515/fetch.br-remote-glob-merge_remote-glob
t5515/fetch.br-remote-glob-octopus
t5515/fetch.br-remote-glob-octopus_remote-glob
t5515/fetch.br-remote-glob_remote-glob
t5515/fetch.br-unconfig
t5515/fetch.br-unconfig_--tags_.._.git
t5515/fetch.br-unconfig_.._.git
t5515/fetch.br-unconfig_.._.git_one
t5515/fetch.br-unconfig_.._.git_one_tag_tag-one_tag_tag-three-file
t5515/fetch.br-unconfig_.._.git_one_two
t5515/fetch.br-unconfig_.._.git_tag_tag-one-tree_tag_tag-three-file
t5515/fetch.br-unconfig_.._.git_tag_tag-one_tag_tag-three
t5515/fetch.br-unconfig_branches-default
t5515/fetch.br-unconfig_branches-one
t5515/fetch.br-unconfig_config-explicit
t5515/fetch.br-unconfig_config-glob
t5515/fetch.br-unconfig_remote-explicit
t5515/fetch.br-unconfig_remote-glob
t5515/fetch.master
t5515/fetch.master_--tags_.._.git
t5515/fetch.master_.._.git
t5515/fetch.master_.._.git_one
t5515/fetch.master_.._.git_one_tag_tag-one_tag_tag-three-file
t5515/fetch.master_.._.git_one_two
t5515/fetch.master_.._.git_tag_tag-one-tree_tag_tag-three-file
t5515/fetch.master_.._.git_tag_tag-one_tag_tag-three
t5515/fetch.master_branches-default
t5515/fetch.master_branches-one
t5515/fetch.master_config-explicit
t5515/fetch.master_config-glob
t5515/fetch.master_remote-explicit
t5515/fetch.master_remote-glob
t5515/refs.br-branches-default
t5515/refs.br-branches-default-merge
t5515/refs.br-branches-default-merge_branches-default
t5515/refs.br-branches-default-octopus
t5515/refs.br-branches-default-octopus_branches-default
t5515/refs.br-branches-default_branches-default
t5515/refs.br-branches-one
t5515/refs.br-branches-one-merge
t5515/refs.br-branches-one-merge_branches-one
t5515/refs.br-branches-one-octopus
t5515/refs.br-branches-one-octopus_branches-one
t5515/refs.br-branches-one_branches-one
t5515/refs.br-config-explicit
t5515/refs.br-config-explicit-merge
t5515/refs.br-config-explicit-merge_config-explicit
t5515/refs.br-config-explicit-octopus
t5515/refs.br-config-explicit-octopus_config-explicit
t5515/refs.br-config-explicit_config-explicit
t5515/refs.br-config-glob
t5515/refs.br-config-glob-merge
t5515/refs.br-config-glob-merge_config-glob
t5515/refs.br-config-glob-octopus
t5515/refs.br-config-glob-octopus_config-glob
t5515/refs.br-config-glob_config-glob
t5515/refs.br-remote-explicit
t5515/refs.br-remote-explicit-merge
t5515/refs.br-remote-explicit-merge_remote-explicit
t5515/refs.br-remote-explicit-octopus
t5515/refs.br-remote-explicit-octopus_remote-explicit
t5515/refs.br-remote-explicit_remote-explicit
t5515/refs.br-remote-glob
t5515/refs.br-remote-glob-merge
t5515/refs.br-remote-glob-merge_remote-glob
t5515/refs.br-remote-glob-octopus
t5515/refs.br-remote-glob-octopus_remote-glob
t5515/refs.br-remote-glob_remote-glob
t5515/refs.br-unconfig
t5515/refs.br-unconfig_--tags_.._.git
t5515/refs.br-unconfig_.._.git
t5515/refs.br-unconfig_.._.git_one
t5515/refs.br-unconfig_.._.git_one_tag_tag-one_tag_tag-three-file
t5515/refs.br-unconfig_.._.git_one_two
t5515/refs.br-unconfig_.._.git_tag_tag-one-tree_tag_tag-three-file
t5515/refs.br-unconfig_.._.git_tag_tag-one_tag_tag-three
t5515/refs.br-unconfig_branches-default
t5515/refs.br-unconfig_branches-one
t5515/refs.br-unconfig_config-explicit
t5515/refs.br-unconfig_config-glob
t5515/refs.br-unconfig_remote-explicit
t5515/refs.br-unconfig_remote-glob
t5515/refs.master
t5515/refs.master_--tags_.._.git
t5515/refs.master_.._.git
t5515/refs.master_.._.git_one
t5515/refs.master_.._.git_one_tag_tag-one_tag_tag-three-file
t5515/refs.master_.._.git_one_two
t5515/refs.master_.._.git_tag_tag-one-tree_tag_tag-three-file
t5515/refs.master_.._.git_tag_tag-one_tag_tag-three
t5515/refs.master_branches-default
t5515/refs.master_branches-one
t5515/refs.master_config-explicit
t5515/refs.master_config-glob
t5515/refs.master_remote-explicit
t5515/refs.master_remote-glob
t7500/add-comments
t7500/add-content
t7500/add-content-and-comment
t7500/add-signed-off
t7500/add-whitespaced-content
t7500/edit-content
t8005/euc-japan.txt
t8005/sjis.txt
t8005/utf8.txt
t9000/test.pl
t9110/svm.dump
t9111/svnsync.dump
t9115/funky-names.dump
t9121/renamed-dir.dump
t9126/follow-deleted-readded.dump
t9135/svn.dump
t9136/svn.dump
t9150/make-svk-dump
t9150/svk-merge.dump
t9151/.gitignore
t9151/make-svnmerge-dump
t9151/svn-mergeinfo.dump
t9153/svn.dump
t9154/svn.dump
t9161/branches.dump
t9601/cvsroot/.gitattributes
t9601/cvsroot/CVSROOT/.gitignore
t9601/cvsroot/module/added-imported.txt,v
t9601/cvsroot/module/imported-anonymously.txt,v
t9601/cvsroot/module/imported-modified-imported.txt,v
t9601/cvsroot/module/imported-modified.txt,v
t9601/cvsroot/module/imported-once.txt,v
t9601/cvsroot/module/imported-twice.txt,v
t9602/README
t9602/cvsroot/.gitattributes
t9602/cvsroot/CVSROOT/.gitignore
t9602/cvsroot/module/default,v
t9602/cvsroot/module/sub1/default,v
t9602/cvsroot/module/sub1/subsubA/default,v
t9602/cvsroot/module/sub1/subsubB/default,v
t9602/cvsroot/module/sub2/Attic/branch_B_MIXED_only,v
t9602/cvsroot/module/sub2/default,v
t9602/cvsroot/module/sub2/subsubA/default,v
t9602/cvsroot/module/sub3/default,v
t9603/cvsroot/.gitattributes
t9603/cvsroot/CVSROOT/.gitignore
t9603/cvsroot/module/a,v
t9603/cvsroot/module/b,v
t9604/cvsroot/.gitattributes
t9604/cvsroot/CVSROOT/.gitignore
t9604/cvsroot/module/a,v
t9700/test.pl
valgrind/.gitignore
valgrind/analyze.sh
valgrind/default.supp
valgrind/valgrind.sh
make: *** [Makefile:72: test-lint-filenames] Error 1

As a consequence, my two `pu`-based CI jobs failed to run even one
regression test.

Ciao,
Dscho

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1565 bytes --]

Hi Junio,

On Tue, 16 Aug 2016, Johannes Schindelin wrote:

> On Mon, 15 Aug 2016, Junio C Hamano wrote:
> 
> > Junio C Hamano <gitster@pobox.com> writes:
> > 
> > >> +test-lint-filenames:
> > >> +	@illegal="$$(git ls-files | grep '["*:<>?\\|]')"; \
> > >
> > > This pattern must exclude questionables on either NTFS or HFS+; it
> > > is ironic that it is not even sufficient to limit ourselves to the
> > > Portable Character Set [*1*], but such is life.
> > >
> > > By the way, doesn't ls-files take pathspec glob, saving one extra
> > > process to run grep?
> 
> I specifically did not do that, sorry for omitting the rationale from the
> commit message. The reason why I have that grep is so that the backslash
> can also catch non-ASCII characters, such as "Hellö-Jüniö".

And there is another very good reason to keep the grep: the problem I just
reported is *caused* by your avoiding the grep call.

In my tests, at least, `git ls-files '*\*' lists *all* files in
subdirectories. In other words, it matches the *forward* slash. This also
happens when I run the command in cmd.exe instead of Git Bash, meaning
that it is *not* caused by some MSYS2 path conversion from pseudo-Unix to
Windows paths. That only leaves the conclusion that some of our pathspec
code tries to be helpful and takes a backslash for a directory separator.

In light of these problems, and also in light of the fact that the
test-lint-filenames code is hardly performance critical, I am strongly in
favor of reverting to using grep.

Ciao,
Dscho

[PATCH v2] t/Makefile: make sure that paths can be checked out on platforms we care

[Johannes Schindelin]

Some pathnames that are okay on ext4 and on HFS+ cannot be checked
out on Windows. Tests that want to see operations on such paths on
filesystems that support them must do so behind appropriate test
prerequisites, and must not include them in the source tree (instead
they should create them when they run). Otherwise, the source tree
cannot even be checked out.

Make sure that double-quotes, asterisk, colon, greater/less-than,
question-mark, backslash, tab, vertical-bar, as well as any non-ASCII
characters above 0x7f never appear in the pathnames with a new
test-lint-* target as part of a `make test`.

Noticed when a topic wanted to add a pathname with '>' in it.  A
check like this will prevent a similar problems from happening in the
future.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	I re-munged the commit message (I saw that you munged it in `pu`
	and did not quite like the wording, so I permitted myself the
	indulgence).

	And of course I tested this, and it does *not* list all files in
	subdirectories of t/ as problematic.

Published-As: https://github.com/dscho/git/releases/tag/test-lint-filenames-v2
Fetch-It-Via: git fetch https://github.com/dscho/git test-lint-filenames-v2
Interdiff vs v1:

 diff --git a/t/Makefile b/t/Makefile
 index 3c0eb48..bf9cad9 100644
 --- a/t/Makefile
 +++ b/t/Makefile
 @@ -69,7 +69,7 @@ test-lint-shell-syntax:
  	@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS)
  
  test-lint-filenames:
 -	@illegal="$$(git ls-files | grep '["*:<>?\\|]')"; \
 +	@illegal="$$(git ls-files | grep '[	"*:<>?\\|]')"; \
  		test -z "$$illegal" || { \
  		echo >&2 "illegal file name(s): " $$illegal; exit 1; }
  


 t/Makefile | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/t/Makefile b/t/Makefile
index 18e2b28..bf9cad9 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -52,7 +52,8 @@ clean-except-prove-cache:
 clean: clean-except-prove-cache
 	$(RM) .prove
 
-test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax
+test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \
+	test-lint-filenames
 
 test-lint-duplicates:
 	@dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \
@@ -67,6 +68,11 @@ test-lint-executable:
 test-lint-shell-syntax:
 	@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS)
 
+test-lint-filenames:
+	@illegal="$$(git ls-files | grep '[	"*:<>?\\|]')"; \
+		test -z "$$illegal" || { \
+		echo >&2 "illegal file name(s): " $$illegal; exit 1; }
+
 aggregate-results-and-cleanup: $(T)
 	$(MAKE) aggregate-results
 	$(MAKE) clean
-- 
2.9.2.691.g78954f3

base-commit: 70dce4aee55ad8a39a53f86f37a4bd400e0cac7d

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> > > By the way, doesn't ls-files take pathspec glob, saving one extra
>> > > process to run grep?
>> 
>> I specifically did not do that, sorry for omitting the rationale from the
>> commit message. The reason why I have that grep is so that the backslash
>> can also catch non-ASCII characters, such as "Hellö-Jüniö".

That's clever and I did miss that.  It deserves an in-code comment
next to the use of separate grep to prevent others from making the
same mistake in the future.  Having an explanation in the log message
would help but not sufficient for a subtle construct like this.

The trick to catch non-ascii depends on core.quotepath set to true
(which is the default).  You would need to run ls-files with an
explicit "-c core.quotepath=false" to be safe.

It also means that the addition of HT to the grep pattern I
suggested is not necessary, because ls-files will always use "a\tb"
form for a pathname with HT, and you will catch it by looking for
the backslash.  You can lose '"' from the grep pattern for the same
reason.

Two other tricky things are

 (1) the test may be run inside a tarball extract and "git ls-files"
     may not report the pathnames in t/.

 (2) the user may not have a working Git yet in her path.

The invocation of "git -c core.quotepath=false ls-files" needs to at
least have 2>/dev/null to squelch an unnecessary error message.  In
such a scenario, we may miss a new offending pathname, but we will
not have false positive and I think that is the best we can do.

Thanks.

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> The trick to catch non-ascii depends on core.quotepath set to true
> (which is the default).  You would need to run ls-files with an
> explicit "-c core.quotepath=false" to be safe.
> ...
> The invocation of "git -c core.quotepath=false ls-files" needs to at
> least have 2>/dev/null to squelch an unnecessary error message.  In
> such a scenario, we may miss a new offending pathname, but we will
> not have false positive and I think that is the best we can do.
>
> Thanks.

Needless to say (1) both "false" should have been "true"; (2) I
shouldn't be crawling out of bed and typing with hazy brain early in
the morning.

Sorry for typoes and thinkos.

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Johannes Schindelin]

Hi Peff & Junio,

On Mon, 15 Aug 2016, Jeff King wrote:

> On Mon, Aug 15, 2016 at 09:57:52AM -0700, Junio C Hamano wrote:
> 
> > I wonder if we already have a good mechanism to allow a project and
> > its participants (say, "me") to declare "in this project, pathnames
> > must conform to this rule" and help them avoid creating a tree that
> > violates the rule customized to their project.
> > 
> > I guess "write_index_as_tree()" would be one of the central places to
> > hook into and that covers an individual contributor or a patch applier
> > who ends up adding offending paths to the project, as well as a merge
> > made in response to a pull request (unless it is a fast-forward)
> > [*1*].  The pre-receive hook can also be used to inspect and reject an
> > attempt to push an offending tree into the history.

FWIW I think it should be at a different level. See below for more
details.

> > Such a mechanism would allow a project that wants participation by
> > folks with case insensitive filesystems to ensure that they do not
> > create a directory that has both xt_TCPMSS.h and xt_tcpmss.h at the
> > same time, for example, but the mechanism needs to allow visibility
> > into more than just a single path when the custom check is made (e.g.
> > a hook run in "write_index_as_tree()" can see all entries in the index
> > to make the decision; if we were to also hook into "add_to_index()",
> > the hook must be able to see other entries in the index to which the
> > new entry is being added).
> 
> I am not convinced this mechanism needs to be built into git. Because it
> happens to be about filenames, git at least has a hope of making sense
> of the various project rules.

Both of you gentle people may recall a conversation in December 2014 when
we scrambled to plug a hole where maliciously-chosen file names would have
allowed to wreak havoc with a local Git repository's config (among other
things).

We did plug it, but not before I proposed to exclude many more file names
than just maliciously-chosen ones. For example, I wanted to exclude all
file names that are illegal on Windows when core.protectNTFS was set to
true.

If we were to implement this "let's help cross-platform projects"
functionality, it would be at that same level.

However, I have to agree with Junio that Git is *not* targeting *all*
platforms. Conversely, any solution we implement to try to be helpful by
pointing out unportable file names will certainly fall short of *some*
project's requirement.

Given that we have no shortage of problems to solve, I would vote for
addressing portability only as far as Git and its intended target
platforms are concerned.

Ciao,
Dscho

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Jeff King]

On Tue, Aug 16, 2016 at 03:10:46PM +0200, Johannes Schindelin wrote:

> > I am not convinced this mechanism needs to be built into git. Because it
> > happens to be about filenames, git at least has a hope of making sense
> > of the various project rules.
> 
> Both of you gentle people may recall a conversation in December 2014 when
> we scrambled to plug a hole where maliciously-chosen file names would have
> allowed to wreak havoc with a local Git repository's config (among other
> things).
> 
> We did plug it, but not before I proposed to exclude many more file names
> than just maliciously-chosen ones. For example, I wanted to exclude all
> file names that are illegal on Windows when core.protectNTFS was set to
> true.
> 
> If we were to implement this "let's help cross-platform projects"
> functionality, it would be at that same level.

Hrm. I am not sure I agree. At GitHub, for instance, we turn on
core.protectNTFS for all repositories because we do want to be a vector
for attacks. So the tradeoff is a good one: the restrictions on
filenames are not that big, and we gain a lot of safety (i.e., a known
remote code execution bug).

Whereas if core.protectNTFS started disallowing trees with both "foo"
and "FOO", that is a much different tradeoff. It is much more likely to
come up, and it is protecting a much less valuable thing (it's an
annoyance, not a security hole). Projects which do not care about people
on case-insensitive filesystems will be annoyed to have their commits
rejected (whether they are right to be so uncaring or not can be
debated, but I am not sure that GitHub wants to enforce a hard policy at
the fsck layer).

So even if we wanted a similar mechanism, I think it has to be triggered
by a separate config option. And I do not think general hosting sites
would turn it on. It's really a project decision, not a hosting-site
one.

There may be some rules that are in between. I.e., names that are
illegal on some common platform but are extremely unlikely to be chosen
in general. I'd have to see the rules to give an opinion.

-Peff

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Johannes Schindelin]

Hi Junio,

On Tue, 16 Aug 2016, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > The trick to catch non-ascii depends on core.quotepath set to true
> > (which is the default).  You would need to run ls-files with an
> > explicit "-c core.quotepath=false" to be safe.
> > ...
> > The invocation of "git -c core.quotepath=false ls-files" needs to at
> > least have 2>/dev/null to squelch an unnecessary error message.  In
> > such a scenario, we may miss a new offending pathname, but we will
> > not have false positive and I think that is the best we can do.
> >
> > Thanks.
> 
> Needless to say (1) both "false" should have been "true"; (2) I
> shouldn't be crawling out of bed and typing with hazy brain early in
> the morning.
> 
> Sorry for typoes and thinkos.

Sorry for not even expecting an answer from you that early. I will send
out a v3 in a moment that should address your concerns.

Ciao,
Dscho

[PATCH v3] t/Makefile: ensure that paths are valid on platforms we care

[Johannes Schindelin]

Some pathnames that are okay on ext4 and on HFS+ cannot be checked
out on Windows. Tests that want to see operations on such paths on
filesystems that support them must do so behind appropriate test
prerequisites, and must not include them in the source tree (instead
they should create them when they run). Otherwise, the source tree
cannot even be checked out.

Make sure that double-quotes, asterisk, colon, greater/less-than,
question-mark, backslash, tab, vertical-bar, as well as any non-ASCII
characters never appear in the pathnames with a new test-lint-* target
as part of a `make test`. To that end, we call `git ls-files` (ensuring
that the paths are quoted properly), relying on the fact that paths
containing non-ASCII characters are quoted within double-quotes.

In case that the source code does not actually live in a Git
repository (e.g. when extracted from a .zip file), or that the `git`
executable cannot be executed, we simply ignore the error for now; In
that case, our trusty Continuous Integration will be the last line of
defense and catch any problematic file name.

Noticed when a topic wanted to add a pathname with '>' in it.  A
check like this will prevent a similar problems from happening in the
future.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	Again some touch-ups to the commit message.

	Please note that the paths are now quoted within double-quotes,
	thanks to the core.quotepaths setting. As such, non-ASCII
	characters are no longer caught through the backslash, but through
	the double-quote character (i.e. '"').

	As previously, tested on Linux and Windows and verified that it
	does the job.

Published-As: https://github.com/dscho/git/releases/tag/test-lint-filenames-v3
Fetch-It-Via: git fetch https://github.com/dscho/git test-lint-filenames-v3
Interdiff vs v2:

 diff --git a/t/Makefile b/t/Makefile
 index bf9cad9..d613935 100644
 --- a/t/Makefile
 +++ b/t/Makefile
 @@ -69,9 +69,12 @@ test-lint-shell-syntax:
  	@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS)
  
  test-lint-filenames:
 -	@illegal="$$(git ls-files | grep '[	"*:<>?\\|]')"; \
 -		test -z "$$illegal" || { \
 -		echo >&2 "illegal file name(s): " $$illegal; exit 1; }
 +	@# We do *not* pass a glob to ls-files but use grep instead, to catch
 +	@# non-ASCII characters (which are quoted within double-quotes)
 +	@bad="$$(git -c core.quotepath=true ls-files 2>/dev/null | \
 +			grep '["*:<>?\\|]')"; \
 +		test -z "$$bad" || { \
 +		echo >&2 "non-portable file name(s): $$bad"; exit 1; }
  
  aggregate-results-and-cleanup: $(T)
  	$(MAKE) aggregate-results


 t/Makefile | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/t/Makefile b/t/Makefile
index 18e2b28..d613935 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -52,7 +52,8 @@ clean-except-prove-cache:
 clean: clean-except-prove-cache
 	$(RM) .prove
 
-test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax
+test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \
+	test-lint-filenames
 
 test-lint-duplicates:
 	@dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \
@@ -67,6 +68,14 @@ test-lint-executable:
 test-lint-shell-syntax:
 	@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS)
 
+test-lint-filenames:
+	@# We do *not* pass a glob to ls-files but use grep instead, to catch
+	@# non-ASCII characters (which are quoted within double-quotes)
+	@bad="$$(git -c core.quotepath=true ls-files 2>/dev/null | \
+			grep '["*:<>?\\|]')"; \
+		test -z "$$bad" || { \
+		echo >&2 "non-portable file name(s): $$bad"; exit 1; }
+
 aggregate-results-and-cleanup: $(T)
 	$(MAKE) aggregate-results
 	$(MAKE) clean
-- 
2.9.2.691.g78954f3

base-commit: 70dce4aee55ad8a39a53f86f37a4bd400e0cac7d

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Johannes Schindelin]

Hi Peff,

On Tue, 16 Aug 2016, Jeff King wrote:

> On Tue, Aug 16, 2016 at 03:10:46PM +0200, Johannes Schindelin wrote:
> 
> > > I am not convinced this mechanism needs to be built into git.
> > > Because it happens to be about filenames, git at least has a hope of
> > > making sense of the various project rules.
> > 
> > Both of you gentle people may recall a conversation in December 2014
> > when we scrambled to plug a hole where maliciously-chosen file names
> > would have allowed to wreak havoc with a local Git repository's config
> > (among other things).
> > 
> > We did plug it, but not before I proposed to exclude many more file
> > names than just maliciously-chosen ones. For example, I wanted to
> > exclude all file names that are illegal on Windows when
> > core.protectNTFS was set to true.
> > 
> > If we were to implement this "let's help cross-platform projects"
> > functionality, it would be at that same level.
> 
> Hrm. I am not sure I agree. At GitHub, for instance, we turn on
> core.protectNTFS for all repositories because we do want to be a vector
> for attacks.

I trust you meant "do *not* want to be a vector for attacks"...

> So the tradeoff is a good one: the restrictions on filenames are not
> that big, and we gain a lot of safety (i.e., a known remote code
> execution bug).
> 
> Whereas if core.protectNTFS started disallowing trees with both "foo"
> and "FOO", that is a much different tradeoff. It is much more likely to
> come up, and it is protecting a much less valuable thing (it's an
> annoyance, not a security hole). Projects which do not care about people
> on case-insensitive filesystems will be annoyed to have their commits
> rejected (whether they are right to be so uncaring or not can be
> debated, but I am not sure that GitHub wants to enforce a hard policy at
> the fsck layer).
> 
> So even if we wanted a similar mechanism, I think it has to be triggered
> by a separate config option. And I do not think general hosting sites
> would turn it on. It's really a project decision, not a hosting-site
> one.
> 
> There may be some rules that are in between. I.e., names that are
> illegal on some common platform but are extremely unlikely to be chosen
> in general. I'd have to see the rules to give an opinion.

Good point.

What I meant in my curt language was actually not to use core.protectNTFS
per se, but the same code path. That is, I would rather have any such
"cross-platform helping" code in verify_path() rather than
write_index_as_tree().

But you are correct, this hypothetical feature (pretty hypothetical,
indeed, at this point) would have to be configured differently than
via core.protectNTFS=true.

Ciao,
Dscho

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Jeff King]

On Tue, Aug 16, 2016 at 05:37:35PM +0200, Johannes Schindelin wrote:

> > Hrm. I am not sure I agree. At GitHub, for instance, we turn on
> > core.protectNTFS for all repositories because we do want to be a vector
> > for attacks.
> 
> I trust you meant "do *not* want to be a vector for attacks"...

Heh, yes.

> Good point.
> 
> What I meant in my curt language was actually not to use core.protectNTFS
> per se, but the same code path. That is, I would rather have any such
> "cross-platform helping" code in verify_path() rather than
> write_index_as_tree().
> 
> But you are correct, this hypothetical feature (pretty hypothetical,
> indeed, at this point) would have to be configured differently than
> via core.protectNTFS=true.

Ah, OK. Yeah, I don't have any real objection to that. I'm not sure how
easy it would be to check verify_path() for an existing commit (say, as
part of a pre-commit hook, or a pre-receive, or in a CI test). But if it
is not easy, that seems like a problem that can (and should) be fixed
separately.

-Peff

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Johannes Sixt]

Am 16.08.2016 um 10:42 schrieb Johannes Schindelin:
> That only leaves the conclusion that some of our pathspec
> code tries to be helpful and takes a backslash for a directory separator.

Very true. The code that does this is in prefix_filename():

https://github.com/git/git/blob/master/abspath.c#L170

-- Hannes