[PATCH 0/1] windows: avoid illegal filenames during a build

[PATCH 0/1] windows: avoid illegal filenames during a build

[Johannes Schindelin via GitGitGadget]

The + is not allowed on NTFS filesystems. Even if the MSYS2 make/bash we use
to build Git for Windows can work around it, it is not necessary.

Johannes Schindelin (1):
  Avoid illegal filenames when building Documentation on NTFS

 Documentation/Makefile | 88 +++++++++++++++++++++---------------------
 1 file changed, 44 insertions(+), 44 deletions(-)


base-commit: aa25c82427ae70aebf3b8f970f2afd54e9a2a8c6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-216%2Fdscho%2Fmingw-avoid-illegal-filenames-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-216/dscho/mingw-avoid-illegal-filenames-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/216
-- 
gitgitgadget

[PATCH 1/1] Avoid illegal filenames when building Documentation on NTFS

[Johannes Schindelin via GitGitGadget]

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

A `+` is not a valid part of a filename with Windows file systems (it is
reserved because the `+` operator meant file concatenation back in the
DOS days).

When building in Git for Windows' SDK, the `make.exe` that interprets
our `Makefile` is an MSYS2 program, which uses the MSYS2/Cygwin trick of
mapping such illegal characters into the private Unicode page, so as
long as you use programs that are aware of this trick (all MSYS2/Cygwin
programs are, however e.g. `git.exe` is not) things are all dandy.

Let's just not use the `+` character.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/Makefile | 88 +++++++++++++++++++++---------------------
 1 file changed, 44 insertions(+), 44 deletions(-)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index dbf5a0f276..792ddf872e 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -297,9 +297,9 @@ docdep_prereqs = \
 	cmd-list.made $(cmds_txt)
 
 doc.dep : $(docdep_prereqs) $(wildcard *.txt) $(wildcard config/*.txt) build-docdep.perl
-	$(QUIET_GEN)$(RM) $@+ $@ && \
-	$(PERL_PATH) ./build-docdep.perl >$@+ $(QUIET_STDERR) && \
-	mv $@+ $@
+	$(QUIET_GEN)$(RM) $@.new $@ && \
+	$(PERL_PATH) ./build-docdep.perl >$@.new $(QUIET_STDERR) && \
+	mv $@.new $@
 
 -include doc.dep
 
@@ -344,8 +344,8 @@ GIT-ASCIIDOCFLAGS: FORCE
             fi
 
 clean:
-	$(RM) *.xml *.xml+ *.html *.html+ *.1 *.5 *.7
-	$(RM) *.texi *.texi+ *.texi++ git.info gitman.info
+	$(RM) *.xml *.xml.new *.html *.html.new *.1 *.5 *.7
+	$(RM) *.texi *.texi.new *.texi.new.new git.info gitman.info
 	$(RM) *.pdf
 	$(RM) howto-index.txt howto/*.html doc.dep
 	$(RM) technical/*.html technical/api-index.txt
@@ -355,14 +355,14 @@ clean:
 	$(RM) GIT-ASCIIDOCFLAGS
 
 $(MAN_HTML): %.html : %.txt asciidoc.conf asciidoctor-extensions.rb GIT-ASCIIDOCFLAGS
-	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
-	$(TXT_TO_HTML) -d manpage -o $@+ $< && \
-	mv $@+ $@
+	$(QUIET_ASCIIDOC)$(RM) $@.new $@ && \
+	$(TXT_TO_HTML) -d manpage -o $@.new $< && \
+	mv $@.new $@
 
 $(OBSOLETE_HTML): %.html : %.txto asciidoc.conf asciidoctor-extensions.rb GIT-ASCIIDOCFLAGS
-	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
-	$(TXT_TO_HTML) -o $@+ $< && \
-	mv $@+ $@
+	$(QUIET_ASCIIDOC)$(RM) $@.new $@ && \
+	$(TXT_TO_HTML) -o $@.new $< && \
+	mv $@.new $@
 
 manpage-base-url.xsl: manpage-base-url.xsl.in
 	$(QUIET_GEN)sed "s|@@MAN_BASE_URL@@|$(MAN_BASE_URL)|" $< > $@
@@ -372,14 +372,14 @@ manpage-base-url.xsl: manpage-base-url.xsl.in
 	$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
 
 %.xml : %.txt asciidoc.conf asciidoctor-extensions.rb GIT-ASCIIDOCFLAGS
-	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
-	$(TXT_TO_XML) -d manpage -o $@+ $< && \
-	mv $@+ $@
+	$(QUIET_ASCIIDOC)$(RM) $@.new $@ && \
+	$(TXT_TO_XML) -d manpage -o $@.new $< && \
+	mv $@.new $@
 
 user-manual.xml: user-manual.txt user-manual.conf asciidoctor-extensions.rb GIT-ASCIIDOCFLAGS
-	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
-	$(TXT_TO_XML) -d book -o $@+ $< && \
-	mv $@+ $@
+	$(QUIET_ASCIIDOC)$(RM) $@.new $@ && \
+	$(TXT_TO_XML) -d book -o $@.new $< && \
+	mv $@.new $@
 
 technical/api-index.txt: technical/api-index-skel.txt \
 	technical/api-index.sh $(patsubst %,%.txt,$(API_DOCS))
@@ -397,46 +397,46 @@ XSLT = docbook.xsl
 XSLTOPTS = --xinclude --stringparam html.stylesheet docbook-xsl.css
 
 user-manual.html: user-manual.xml $(XSLT)
-	$(QUIET_XSLTPROC)$(RM) $@+ $@ && \
-	xsltproc $(XSLTOPTS) -o $@+ $(XSLT) $< && \
-	mv $@+ $@
+	$(QUIET_XSLTPROC)$(RM) $@.new $@ && \
+	xsltproc $(XSLTOPTS) -o $@.new $(XSLT) $< && \
+	mv $@.new $@
 
 git.info: user-manual.texi
 	$(QUIET_MAKEINFO)$(MAKEINFO) --no-split -o $@ user-manual.texi
 
 user-manual.texi: user-manual.xml
-	$(QUIET_DB2TEXI)$(RM) $@+ $@ && \
-	$(DOCBOOK2X_TEXI) user-manual.xml --encoding=UTF-8 --to-stdout >$@++ && \
-	$(PERL_PATH) fix-texi.perl <$@++ >$@+ && \
-	rm $@++ && \
-	mv $@+ $@
+	$(QUIET_DB2TEXI)$(RM) $@.new $@ && \
+	$(DOCBOOK2X_TEXI) user-manual.xml --encoding=UTF-8 --to-stdout >$@.new.new && \
+	$(PERL_PATH) fix-texi.perl <$@.new.new >$@.new && \
+	rm $@.new.new && \
+	mv $@.new $@
 
 user-manual.pdf: user-manual.xml
-	$(QUIET_DBLATEX)$(RM) $@+ $@ && \
-	$(DBLATEX) -o $@+ $(DBLATEX_COMMON) $< && \
-	mv $@+ $@
+	$(QUIET_DBLATEX)$(RM) $@.new $@ && \
+	$(DBLATEX) -o $@.new $(DBLATEX_COMMON) $< && \
+	mv $@.new $@
 
 gitman.texi: $(MAN_XML) cat-texi.perl texi.xsl
-	$(QUIET_DB2TEXI)$(RM) $@+ $@ && \
-	($(foreach xml,$(sort $(MAN_XML)),xsltproc -o $(xml)+ texi.xsl $(xml) && \
-		$(DOCBOOK2X_TEXI) --encoding=UTF-8 --to-stdout $(xml)+ && \
-		rm $(xml)+ &&) true) > $@++ && \
-	$(PERL_PATH) cat-texi.perl $@ <$@++ >$@+ && \
-	rm $@++ && \
-	mv $@+ $@
+	$(QUIET_DB2TEXI)$(RM) $@.new $@ && \
+	($(foreach xml,$(sort $(MAN_XML)),xsltproc -o $(xml).new texi.xsl $(xml) && \
+		$(DOCBOOK2X_TEXI) --encoding=UTF-8 --to-stdout $(xml).new && \
+		rm $(xml).new &&) true) > $@.new.new && \
+	$(PERL_PATH) cat-texi.perl $@ <$@.new.new >$@.new && \
+	rm $@.new.new && \
+	mv $@.new $@
 
 gitman.info: gitman.texi
 	$(QUIET_MAKEINFO)$(MAKEINFO) --no-split --no-validate $*.texi
 
 $(patsubst %.txt,%.texi,$(MAN_TXT)): %.texi : %.xml
-	$(QUIET_DB2TEXI)$(RM) $@+ $@ && \
-	$(DOCBOOK2X_TEXI) --to-stdout $*.xml >$@+ && \
-	mv $@+ $@
+	$(QUIET_DB2TEXI)$(RM) $@.new $@ && \
+	$(DOCBOOK2X_TEXI) --to-stdout $*.xml >$@.new && \
+	mv $@.new $@
 
 howto-index.txt: howto-index.sh $(wildcard howto/*.txt)
-	$(QUIET_GEN)$(RM) $@+ $@ && \
-	'$(SHELL_PATH_SQ)' ./howto-index.sh $(sort $(wildcard howto/*.txt)) >$@+ && \
-	mv $@+ $@
+	$(QUIET_GEN)$(RM) $@.new $@ && \
+	'$(SHELL_PATH_SQ)' ./howto-index.sh $(sort $(wildcard howto/*.txt)) >$@.new && \
+	mv $@.new $@
 
 $(patsubst %,%.html,$(ARTICLES)) : %.html : %.txt
 	$(QUIET_ASCIIDOC)$(TXT_TO_HTML) $*.txt
@@ -445,10 +445,10 @@ WEBDOC_DEST = /pub/software/scm/git/docs
 
 howto/%.html: ASCIIDOC_EXTRA += -a git-relative-html-prefix=../
 $(patsubst %.txt,%.html,$(wildcard howto/*.txt)): %.html : %.txt GIT-ASCIIDOCFLAGS
-	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
+	$(QUIET_ASCIIDOC)$(RM) $@.new $@ && \
 	sed -e '1,/^$$/d' $< | \
-	$(TXT_TO_HTML) - >$@+ && \
-	mv $@+ $@
+	$(TXT_TO_HTML) - >$@.new && \
+	mv $@.new $@
 
 install-webdoc : html
 	'$(SHELL_PATH_SQ)' ./install-webdoc.sh $(WEBDOC_DEST)
-- 
gitgitgadget

Re: [PATCH 1/1] Avoid illegal filenames when building Documentation on NTFS

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> A `+` is not a valid part of a filename with Windows file systems (it is
> reserved because the `+` operator meant file concatenation back in the
> DOS days).

The title of the cover letter had "windows" in it, so calling '+'
illegal was OK, but this patch does not.

I'd rather not to take this patch, because "generate in the target
with plus appended, make sure it succeeds, and then rename it to the
real target" is quite an established pattern not limited to the
Documentation/ directory of this project, if your tooling has been
supporting it and can continue to do so (which was the impression I
got from the cover letter).  Even the top-level .gitignore file
knows about it, so does the top-level Makefile and it uses the same
pattern.

Re: [PATCH 1/1] Avoid illegal filenames when building Documentation on NTFS

[Johannes Schindelin]

Hi Junio,

On Mon, 8 Jul 2019, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > A `+` is not a valid part of a filename with Windows file systems (it is
> > reserved because the `+` operator meant file concatenation back in the
> > DOS days).
>
> I'd rather not to take this patch, because "generate in the target
> with plus appended, make sure it succeeds, and then rename it to the
> real target" is quite an established pattern not limited to the
> Documentation/ directory of this project, if your tooling has been
> supporting it and can continue to do so (which was the impression I
> got from the cover letter).  Even the top-level .gitignore file
> knows about it, so does the top-level Makefile and it uses the same
> pattern.

Using `.lock` is actually an even more established pattern. (I used `.new`
because the intention is not to lock files, but I would be prepared to
change the patch to that end.)

In addition, your `+` scheme will break on Windows once it uses `git.exe`
or any other non-MSYS2 helper, so it is not future-proof, no matter how
established you claim it to be.

Ciao,
Dscho

Re: [PATCH 1/1] Avoid illegal filenames when building Documentation on NTFS

[Junio C Hamano]

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

> Using `.lock` is actually an even more established pattern. (I used `.new`
> because the intention is not to lock files, but I would be prepared to
> change the patch to that end.)

When I said that the plus convention is established in this project
and not limited to the Documentation subdirectory, what I meant was
that your patch is insufficient if your goal is to depart from the
convention.

The pattern "*+" is known bythe top-level .gitignore file (not just
in Documentation/.gitignore) and that is because the build procedure
outside Documentation/ also follow the convention, e.g. the toplevel
Makefile.  Futzing with only one Makefile is not enough.

I think I saw "generate in .tmp, then move to final" pattern used in
projects by others.  The plus-sign is a lot shorter than anything
else and it is cute, but if some filesystems cannot deal with it,
changing it to something else may be a plausible workaround, as long
as it is done consistently and throughout the codebase.

> In addition, your `+` scheme will break on Windows once it uses `git.exe`
> or any other non-MSYS2 helper...

I am not sure what you mean here.  Is your git.exe disabled not to
be able to do this: "git.exe add hello+kitty.txt"?  I think that is
a more grave problem, and not limited to the Makefile in the
Documentation/ directory.

Re: [PATCH 1/1] Avoid illegal filenames when building Documentation on NTFS

[Johannes Schindelin]

Hi Junio,

On Tue, 9 Jul 2019, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > In addition, your `+` scheme will break on Windows once it uses
> > `git.exe` or any other non-MSYS2 helper...
>
> I am not sure what you mean here.  Is your git.exe disabled not to
> be able to do this: "git.exe add hello+kitty.txt"?  I think that is
> a more grave problem, and not limited to the Makefile in the
> Documentation/ directory.

A `+` character is not allowed as part of a file name on Windows.
Therefore, it is not so much `git.exe`'s problem, as the Git build's
problem that it uses such an illegal character.

It _just happens_ to work at the moment, by virtue of Git for Windows'
SDK _by happenstance_ using only MSYS2-programs to manipulate those
files in question. And MSYS2 programs use a derivative of the Cygwin
runtime which uses a dirty trick to pretend _to MSYS2 programs only_
that `+` (and other characters that are equally illegal in file names on
Windows) can be used for file names: those characters are mapped into
a private page of the Unicode range.

Needless to say, only MSYS2 programs know how to deal with such file
names. If you ever use any program that is not based on the MSYS2/Cygwin
runtime (such as `git.exe`), things will break.

This strikes me as a fragile setup, hence my attempt to fix it.

Ciao,
Dscho

Re: [PATCH 1/1] Avoid illegal filenames when building Documentation on NTFS

[Junio C Hamano]

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

>> In addition, your `+` scheme will break on Windows once it uses `git.exe`
>> or any other non-MSYS2 helper...
>
> I am not sure what you mean here.  Is your git.exe disabled not to
> be able to do this: "git.exe add hello+kitty.txt"?  I think that is
> a more grave problem, and not limited to the Makefile in the
> Documentation/ directory.

The reason why I thought it was a more grave problem is because (1)
gets 7648757 out of (2), which gets 2297783817 (that is, among the
2.2 billion paths that appear in various GitHub public repositories,
a tad short of 8 million paths have '+' somewhere):

    (1) SELECT count(*) FROM `bigquery-public-data.github_repos.files`
        where path like '%+%';

    (2) SELECT count(*) FROM `bigquery-public-data.github_repos.files`;

If you drop the technique MSYS2 borrowed from Cygwin to support
these characters NTFS does not like by mapping them to a private
page, wouldn't it mean that these projects cannot be used with Git
for Windows?  Are we going to bug each of these projects to change
their pathnames?

If that is not the case, then that's fine.

FWIW, I am OK with moving away from '+' in *our* project; that is
easy enough to do, and such a change does not have to make the end
result visually less pleasing, as long as we pick a good substitute
suffix.  Is '~' (or ',') allowed on NTFS, for example?  The point of
'+' being short, sweet, and easy to spot, I'd rather not to use
'.<anyword>' if we can avoid it.

Whatever suffix we choose, we need to be aware that the pattern is
not limited to Documentation/ directory, so at least the top level
.gitignore and Makefile in various subdirectories need vetting.

Re: [PATCH 1/1] Avoid illegal filenames when building Documentation on NTFS

[Johannes Schindelin]

Hi Junio,

On Wed, 10 Jul 2019, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
> >> In addition, your `+` scheme will break on Windows once it uses `git.exe`
> >> or any other non-MSYS2 helper...
> >
> > I am not sure what you mean here.  Is your git.exe disabled not to
> > be able to do this: "git.exe add hello+kitty.txt"?

It's not that. The `+` is not allowed on Windows in general.

Mind you, I just read up on the details (see e.g.
https://en.wikipedia.org/wiki/Filename#Comparison_of_filename_limitations)
and it looks as if NTFS actually _allows_ this. Even VFAT allows it, but
FAT32 does not.

So I guess I retract my objections, as there are probably smarter ideas
than building Git for Windows on a FAT32 drive.

For aesthetical reasons, I still do not like the convention to append a
`+` when appending `.new` would make things clearer, but this is not a
hill I am prepared to die on.

Ciao,
Dscho