windows: avoid illegal filenames during a build

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

From: Johannes Schindelin <[email protected]>

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.

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

index dbf5a0f276..792ddf872e 100644
@@ -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) [email protected] $@ && \
+	$(PERL_PATH) ./build-docdep.perl >[email protected] $(QUIET_STDERR) && \
+	mv [email protected] $@
 
 -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) [email protected] $@ && \
+	$(TXT_TO_HTML) -d manpage -o [email protected] $< && \
+	mv [email protected] $@
 
 $(OBSOLETE_HTML): %.html : %.txto asciidoc.conf asciidoctor-extensions.rb GIT-ASCIIDOCFLAGS
-	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
-	$(TXT_TO_HTML) -o $@+ $< && \
-	mv $@+ $@
+	$(QUIET_ASCIIDOC)$(RM) [email protected] $@ && \
+	$(TXT_TO_HTML) -o [email protected] $< && \
+	mv [email protected] $@
 
 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) [email protected] $@ && \
+	$(TXT_TO_XML) -d manpage -o [email protected] $< && \
+	mv [email protected] $@
 
 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) [email protected] $@ && \
+	$(TXT_TO_XML) -d book -o [email protected] $< && \
+	mv [email protected] $@
 
 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) [email protected] $@ && \
+	xsltproc $(XSLTOPTS) -o [email protected] $(XSLT) $< && \
+	mv [email protected] $@
 
 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) [email protected] $@ && \
+	$(DOCBOOK2X_TEXI) user-manual.xml --encoding=UTF-8 --to-stdout >[email protected] && \
+	$(PERL_PATH) fix-texi.perl <[email protected] >[email protected] && \
+	rm [email protected] && \
+	mv [email protected] $@
 
 user-manual.pdf: user-manual.xml
-	$(QUIET_DBLATEX)$(RM) $@+ $@ && \
-	$(DBLATEX) -o $@+ $(DBLATEX_COMMON) $< && \
-	mv $@+ $@
+	$(QUIET_DBLATEX)$(RM) [email protected] $@ && \
+	$(DBLATEX) -o [email protected] $(DBLATEX_COMMON) $< && \
+	mv [email protected] $@
 
 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) [email protected] $@ && \
+	($(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) > [email protected] && \
+	$(PERL_PATH) cat-texi.perl $@ <[email protected] >[email protected] && \
+	rm [email protected] && \
+	mv [email protected] $@
 
 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) [email protected] $@ && \
+	$(DOCBOOK2X_TEXI) --to-stdout $*.xml >[email protected] && \
+	mv [email protected] $@
 
 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) [email protected] $@ && \
+	'$(SHELL_PATH_SQ)' ./howto-index.sh $(sort $(wildcard howto/*.txt)) >[email protected] && \
+	mv [email protected] $@
 
 $(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) [email protected] $@ && \
 	sed -e '1,/^$$/d' $< | \
-	$(TXT_TO_HTML) - >$@+ && \
-	mv $@+ $@
+	$(TXT_TO_HTML) - >[email protected] && \
+	mv [email protected] $@
 
 install-webdoc : html
 	'$(SHELL_PATH_SQ)' ./install-webdoc.sh $(WEBDOC_DEST)

gitgitgadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

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.

Hi Junio,

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

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.

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.

Hi Junio,

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

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.

Hi Junio,

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