Skip to content

Fit the building tools to Plan 9 environment #694

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Fit the building tools to Plan 9 environment #694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
534378c
Fit to Plan 9's ANSI/POSIX compatibility layer
lufia Sep 9, 2020
7d33e11
Use $(LD) instead of $(CC) for linking the object files
lufia Sep 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@ jobs:
pool: ubuntu-latest
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Kyohei Kadota via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/Makefile b/Makefile
> index 86e5411f39..c0c0c280f0 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -543,6 +543,7 @@ export prefix bindir sharedir sysconfdir gitwebdir perllibdir localedir
>  
>  # Set our default programs
>  CC = cc
> +LD = cc

This is good that any system that use 'cc' to link now will reach
the same 'cc' via $(LD) instead of $(CC).

But those who customize $(CC) to their taste now needs to override
not just CC but also LD.  Can we avoid regressing for them?  Perhaps
default the value of LD to $(CC) instead of hardcoded 'cc'?

> @@ -2121,7 +2122,7 @@ git.sp git.s git.o: EXTRA_CPPFLAGS = \
>  	'-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
>  
>  git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
> -	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
> +	$(QUIET_LINK)$(LD) -o $@ $(ALL_LDFLAGS) \
>  		$(filter %.o,$^) $(LIBS)

Are there those who depended on ALL_CFLAGS on the command line when
the final linking happens?

For example, people have '-g -O2' on CFLAGS, and that will be
included in ALL_CFLAGS and passed to the final linking phase.  GNU
ld has "-O" option to optimize; when existing systems link with "cc
-g -O2 -o $@ ...", it is likely that "-O2" given to "cc" will cause
the "-O" option to be passed to underlying "ld".

But with the updated procedure (where LD is 'cc' for them), the
linkage will be done without ALL_CFLAGS passed on the command line,
and results in "cc -o $@ ...", without "-g -O2".

That sounds like a regression.

> @@ -2445,17 +2446,17 @@ compat/nedmalloc/nedmalloc.sp: SP_EXTRA_FLAGS += -Wno-non-pointer-null
>  endif
>  
>  git-%$X: %.o GIT-LDFLAGS $(GITLIBS)
> -	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
> +	$(QUIET_LINK)$(LD) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
>  
>  git-imap-send$X: imap-send.o $(IMAP_SEND_BUILDDEPS) GIT-LDFLAGS $(GITLIBS)
> -	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
> +	$(QUIET_LINK)$(LD) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
>  		$(IMAP_SEND_LDFLAGS) $(LIBS)
>  
>  git-http-fetch$X: http.o http-walker.o http-fetch.o GIT-LDFLAGS $(GITLIBS)
> -	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
> +	$(QUIET_LINK)$(LD) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
>  		$(CURL_LIBCURL) $(LIBS)
>  git-http-push$X: http.o http-push.o GIT-LDFLAGS $(GITLIBS)
> -	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
> +	$(QUIET_LINK)$(LD) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
>  		$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)
>  
>  $(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY)
> @@ -2465,7 +2466,7 @@ $(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY)
>  	cp $< $@
>  
>  $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS)
> -	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
> +	$(QUIET_LINK)$(LD) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
>  		$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)
>
> @@ -2753,7 +2754,7 @@ perf: all
>  t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
>  
>  t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS)
> -	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
> +	$(QUIET_LINK)$(LD) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)

Likewise for all of the above.

> diff --git a/ci/lib.sh b/ci/lib.sh
> index 3eefec500d..19c5beb277 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -125,6 +125,7 @@ then

All changes to this file to pass LD=cc everywhere CC=cc is passed
look good to me.

> diff --git a/config.mak.in b/config.mak.in
> index e6a6d0f941..76ea7e781e 100644
> --- a/config.mak.in
> +++ b/config.mak.in
> @@ -2,6 +2,7 @@
>  # @configure_input@
>  
>  CC = @CC@
> +LD = @CC@
>  CFLAGS = @CFLAGS@
>  CPPFLAGS = @CPPFLAGS@
>  LDFLAGS = @LDFLAGS@

OK.

> diff --git a/config.mak.uname b/config.mak.uname
> index c7eba69e54..886ce068f8 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -69,6 +69,7 @@ ifeq ($(uname_S),GNU/kFreeBSD)
>  endif
>  ifeq ($(uname_S),UnixWare)
>  	CC = cc
> +	LD = $(CC)
>  	NEEDS_SOCKET = YesPlease
>  	NEEDS_NSL = YesPlease
>  	NEEDS_SSL_WITH_CRYPTO = YesPlease
> @@ -90,6 +91,7 @@ ifeq ($(uname_S),SCO_SV)
>  	endif
>  	ifeq ($(uname_R),5)
>  		CC = cc
> +		LD = $(CC)
>  		BASIC_CFLAGS += -Kthread
>  	endif
>  	NEEDS_SOCKET = YesPlease
> @@ -435,6 +437,7 @@ ifeq ($(uname_S),Windows)
>  	DEFAULT_HELP_FORMAT = html
>  
>  	CC = compat/vcbuild/scripts/clink.pl
> +	LD = $(CC)
>  	AR = compat/vcbuild/scripts/lib.pl
>  	CFLAGS =
>  	BASIC_CFLAGS = -nologo -I. -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE
> @@ -525,6 +528,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
>  	ifeq ($(uname_R).$(uname_V),L17.02)
>  		CC += -WRVU=L16.05
>  	endif
> +	LD = $(CC)
>  	# Disable all optimization, seems to result in bad code, with -O or -O2
>  	# or even -O1 (default), /usr/local/libexec/git-core/git-pack-objects
>  	# abends on "git push". Needs more investigation.

Ditto.

> @@ -665,8 +669,10 @@ else
>  			BASIC_LDFLAGS += -Wl,--large-address-aware
>  		endif
>  		CC = gcc
> +		LD = $(CC)
>  		COMPAT_CFLAGS += -D__USE_MINGW_ANSI_STDIO=0 -DDETECT_MSYS_TTY \
>  			-fstack-protector-strong
> +		BASIC_LDFLAGS += -fstack-protector-strong
>  		EXTLIBS += -lntdll
>  		INSTALL = /bin/install
>  		NO_R_TO_GCC_LINKER = YesPlease

This is an example of the problem we discussed earlier for not
passing ALL_CFLAGS to the linking phase.  I am not sure what the
best solution is, though.

Thanks.

env:
CC: ${{matrix.vector.cc}}
LD: ${{matrix.vector.cc}}
jobname: ${{matrix.vector.jobname}}
runs-on: ${{matrix.vector.pool}}
steps:
Expand Down
15 changes: 8 additions & 7 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,7 @@ export prefix bindir sharedir sysconfdir gitwebdir perllibdir localedir

# Set our default programs
CC = cc
LD = cc
AR = ar
RM = rm -f
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

> From: lufia <lufia@lufia.org>

See Documentation/SubmittingPatches[[real-name]].

> Plan 9's tar(1) don't support o option.
> So I changed Makefiles to replace tar commands if needed.

There is a big gap/leap between these two sentences.  The first
sentence might justify TAR_XF but it does not explain why TAR_CF
is needed at all.  Also there is "$(TAR) czf" that is left without
getting turned into $(TAR_CF)z.

Are there still remaining uses of $(TAR) after applying this patch?
If with a different justification, a patch that abstracts our use of
"tar" as an archiving tool into different use cases and give one
Makefile macro to each of them with more meaningful names might be
more palatable.  E.g. instead of TAR_CF that abstracts only the "tar
cf" part, make "take everything in the current directory and send an
archive of it to the standard output", i.e. "tar cf - .", into a
Makefile macro, with matching "take an archive from the standard
input and extract it to the current directory", i.e. "tar xf -", as
a matching Makefile macro, perhaps---that way people without a
working "tar" might be able to use zip, cpio or pax (or a script
written around them) as an alternative "archiving tool", for
example.

	Side note.  The above would probably be a huge undertaking
	and I am not suggesting it as a serious counterproposal.

A more plausible alternative is to just touch "$(TAR) xof -" and
replace it with $(TAR_XOF) or even $(EXTRACT_TARBALL_AS_MYSELF)
while leaving $(TAR_CF) alone.  That _might_ be palatable.

I dunno.

DIFF = diff
Expand Down Expand Up @@ -2121,7 +2122,7 @@ git.sp git.s git.o: EXTRA_CPPFLAGS = \
'-DGIT_INFO_PATH="$(infodir_relative_SQ)"'

git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
$(QUIET_LINK)$(LD) -o $@ $(ALL_LDFLAGS) \
$(filter %.o,$^) $(LIBS)

help.sp help.s help.o: command-list.h
Expand Down Expand Up @@ -2445,17 +2446,17 @@ compat/nedmalloc/nedmalloc.sp: SP_EXTRA_FLAGS += -Wno-non-pointer-null
endif

git-%$X: %.o GIT-LDFLAGS $(GITLIBS)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
$(QUIET_LINK)$(LD) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)

git-imap-send$X: imap-send.o $(IMAP_SEND_BUILDDEPS) GIT-LDFLAGS $(GITLIBS)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
$(QUIET_LINK)$(LD) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
$(IMAP_SEND_LDFLAGS) $(LIBS)

git-http-fetch$X: http.o http-walker.o http-fetch.o GIT-LDFLAGS $(GITLIBS)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
$(QUIET_LINK)$(LD) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
$(CURL_LIBCURL) $(LIBS)
git-http-push$X: http.o http-push.o GIT-LDFLAGS $(GITLIBS)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
$(QUIET_LINK)$(LD) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)

$(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY)
Expand All @@ -2465,7 +2466,7 @@ $(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY)
cp $< $@

$(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
$(QUIET_LINK)$(LD) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)

$(LIB_FILE): $(LIB_OBJS)
Expand Down Expand Up @@ -2753,7 +2754,7 @@ perf: all
t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))

t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
$(QUIET_LINK)$(LD) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)

check-sha1:: t/helper/test-tool$X
t/helper/test-sha1.sh
Expand Down
8 changes: 7 additions & 1 deletion ci/lib.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ then
test darwin != "$CI_OS_NAME" || CI_OS_NAME=osx
CI_REPO_SLUG="$(expr "$BUILD_REPOSITORY_URI" : '.*/\([^/]*/[^/]*\)$')"
CC="${CC:-gcc}"
LD="$CC"

# use a subdirectory of the cache dir (because the file share is shared
# among *all* phases)
Expand All @@ -149,6 +150,7 @@ then
CI_REPO_SLUG="$GITHUB_REPOSITORY"
CI_JOB_ID="$GITHUB_RUN_ID"
CC="${CC:-gcc}"
LD="$CC"

cache_dir="$HOME/none"

Expand Down Expand Up @@ -184,6 +186,7 @@ linux-clang|linux-gcc)
if [ "$jobname" = linux-gcc ]
then
export CC=gcc-8
export LD="$CC"
MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/python3"
else
MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/python2"
Expand All @@ -207,6 +210,7 @@ osx-clang|osx-gcc)
if [ "$jobname" = osx-gcc ]
then
export CC=gcc-9
export LD="$CC"
MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=$(which python3)"
else
MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=$(which python2)"
Expand All @@ -222,12 +226,14 @@ GETTEXT_POISON)
;;
Linux32)
CC=gcc
LD="$CC"
;;
linux-musl)
CC=gcc
LD="$CC"
MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/python3 USE_LIBPCRE2=Yes"
MAKEFLAGS="$MAKEFLAGS NO_REGEX=Yes ICONV_OMITS_BOM=Yes"
;;
esac

MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"
MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc} LD=${LD:-cc}"
1 change: 1 addition & 0 deletions config.mak.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
# @configure_input@

CC = @CC@
LD = @CC@
CFLAGS = @CFLAGS@
CPPFLAGS = @CPPFLAGS@
LDFLAGS = @LDFLAGS@
Expand Down
6 changes: 6 additions & 0 deletions config.mak.uname
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ ifeq ($(uname_S),GNU/kFreeBSD)
endif
ifeq ($(uname_S),UnixWare)
CC = cc
LD = $(CC)
NEEDS_SOCKET = YesPlease
NEEDS_NSL = YesPlease
NEEDS_SSL_WITH_CRYPTO = YesPlease
Expand All @@ -90,6 +91,7 @@ ifeq ($(uname_S),SCO_SV)
endif
ifeq ($(uname_R),5)
CC = cc
LD = $(CC)
BASIC_CFLAGS += -Kthread
endif
NEEDS_SOCKET = YesPlease
Expand Down Expand Up @@ -435,6 +437,7 @@ ifeq ($(uname_S),Windows)
DEFAULT_HELP_FORMAT = html

CC = compat/vcbuild/scripts/clink.pl
LD = $(CC)
AR = compat/vcbuild/scripts/lib.pl
CFLAGS =
BASIC_CFLAGS = -nologo -I. -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE
Expand Down Expand Up @@ -525,6 +528,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
ifeq ($(uname_R).$(uname_V),L17.02)
CC += -WRVU=L16.05
endif
LD = $(CC)
# Disable all optimization, seems to result in bad code, with -O or -O2
# or even -O1 (default), /usr/local/libexec/git-core/git-pack-objects
# abends on "git push". Needs more investigation.
Expand Down Expand Up @@ -665,8 +669,10 @@ else
BASIC_LDFLAGS += -Wl,--large-address-aware
endif
CC = gcc
LD = $(CC)
COMPAT_CFLAGS += -D__USE_MINGW_ANSI_STDIO=0 -DDETECT_MSYS_TTY \
-fstack-protector-strong
BASIC_LDFLAGS += -fstack-protector-strong
EXTLIBS += -lntdll
INSTALL = /bin/install
NO_R_TO_GCC_LINKER = YesPlease
Expand Down
2 changes: 1 addition & 1 deletion generate-cmdlist.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ command_list () {
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Wed, Sep 9, 2020 at 3:48 PM Kyohei Kadota via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> And its sed(1)'s label is limited to maximum seven characters.
> Therefore I replaced some labels to drop a character.
>
> * close -> cl
> * continue -> cont (cnt is used for count)
> * line -> ln
> * hered -> hdoc
> * shell -> sh
> * string -> str

These are reasonable. "cl" feels a little odd, but I can't think of
anything better.

> diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
> @@ -24,7 +24,7 @@ category_list () {
>  get_synopsis () {
>         sed -n '
> -               /^NAME/,/'"$1"'/H
> +               /^NAME/,/'"$1"'/h

This change is not mentioned in the commit message. "H" and "h" are
very different commands, so it's difficult, at a glance, to tell if
this change is even valid. Some explanation in the commit message
would help (if it is indeed valid).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Wed, Sep 9, 2020 at 3:48 PM Kyohei Kadota via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> And its sed(1)'s label is limited to maximum seven characters.
>> Therefore I replaced some labels to drop a character.
>>
>> * close -> cl
>> * continue -> cont (cnt is used for count)
>> * line -> ln
>> * hered -> hdoc
>> * shell -> sh
>> * string -> str
>
> These are reasonable. "cl" feels a little odd, but I can't think of
> anything better.
>
>> diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
>> @@ -24,7 +24,7 @@ category_list () {
>>  get_synopsis () {
>>         sed -n '
>> -               /^NAME/,/'"$1"'/H
>> +               /^NAME/,/'"$1"'/h
>
> This change is not mentioned in the commit message. "H" and "h" are
> very different commands, so it's difficult, at a glance, to tell if
> this change is even valid. Some explanation in the commit message
> would help (if it is indeed valid).

I am glad somebody else caught the same thing as I did.  copying and
appending will give very different results.

Thanks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Kyohei Kadota wrote (reply to this):

Thank you for your review!

2020年9月10日(木) 4:56 Eric Sunshine <sunshine@sunshineco.com>:
>
> On Wed, Sep 9, 2020 at 3:48 PM Kyohei Kadota via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > And its sed(1)'s label is limited to maximum seven characters.
> > Therefore I replaced some labels to drop a character.
> >
> > * close -> cl
> > * continue -> cont (cnt is used for count)
> > * line -> ln
> > * hered -> hdoc
> > * shell -> sh
> > * string -> str
>
> These are reasonable. "cl" feels a little odd, but I can't think of
> anything better.
>
> > diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
> > @@ -24,7 +24,7 @@ category_list () {
> >  get_synopsis () {
> >         sed -n '
> > -               /^NAME/,/'"$1"'/H
> > +               /^NAME/,/'"$1"'/h
>
> This change is not mentioned in the commit message. "H" and "h" are
> very different commands, so it's difficult, at a glance, to tell if
> this change is even valid. Some explanation in the commit message
> would help (if it is indeed valid).

I missed, this change was not needed. It is remnants of trial and error.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Kyohei Kadota via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Kyohei Kadota <lufia@lufia.org>
>
> tr(1) of ANSI/POSIX environment, aka APE, don't support \n literal.
> It's handles only octal(\ooo) or hexadecimal(\xhhhh) numbers.
>
> And its sed(1)'s label is limited to maximum seven characters.
> Therefore I replaced some labels to drop a character.
>
> * close -> cl
> * continue -> cont (cnt is used for count)
> * line -> ln
> * hered -> hdoc
> * shell -> sh
> * string -> str
>
> Signed-off-by: Kyohei Kadota <lufia@lufia.org>
> ---

This round, without the confusion between 'h' and 'H' commands, I
see nothing funny in it.

Will queue.  Thanks.


get_categories () {
tr ' ' '\n'|
tr ' ' '\012'|
grep -v '^$' |
sort |
uniq
Expand Down
66 changes: 33 additions & 33 deletions t/chainlint.sed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@
/^[ ]*!*[ ]*(..*)[ ]*[0-9]*[<>|&]/boneline

# multi-line "(...\n...)"
/^[ ]*(/bsubshell
/^[ ]*(/bsubsh

# innocuous line -- print it and advance to next line
b
Expand All @@ -130,11 +130,11 @@ b
}
b

:subshell
:subsh
# bare "(" line? -- stash for later printing
/^[ ]*([ ]*$/ {
h
bnextline
bnextln
}
# "(..." line -- split off and stash "(", then process "..." as its own line
x
Expand All @@ -143,18 +143,18 @@ x
s/(//
bslurp

:nextline
:nextln
N
s/.*\n//

:slurp
# incomplete line "...\"
/\\$/bicmplte
# multi-line quoted string "...\n..."?
/"/bdqstring
/"/bdqstr
# multi-line quoted string '...\n...'? (but not contraction in string "it's")
/'/{
/"[^'"]*'[^'"]*"/!bsqstring
/"[^'"]*'[^'"]*"/!bsqstr
}
:folded
# here-doc -- swallow it
Expand All @@ -163,8 +163,8 @@ s/.*\n//
# before closing ")", "done", "elsif", "else", or "fi" will need to be
# re-visited to drop "suspect" marking since final line of those constructs
# legitimately lacks "&&", so "suspect" mark must be removed
/^[ ]*#/bnextline
/^[ ]*$/bnextline
/^[ ]*#/bnextln
/^[ ]*$/bnextln
# in-line comment -- strip it (but not "#" in a string, Bash ${#...} array
# length, or Perforce "//depot/path#42" revision in filespec)
/[ ]#/{
Expand All @@ -175,22 +175,22 @@ s/.*\n//
# multi-line "case ... esac"
/^[ ]*case[ ]..*[ ]in/bcase
# multi-line "for ... done" or "while ... done"
/^[ ]*for[ ]..*[ ]in/bcontinue
/^[ ]*while[ ]/bcontinue
/^[ ]*do[ ]/bcontinue
/^[ ]*do[ ]*$/bcontinue
/;[ ]*do/bcontinue
/^[ ]*for[ ]..*[ ]in/bcont
/^[ ]*while[ ]/bcont
/^[ ]*do[ ]/bcont
/^[ ]*do[ ]*$/bcont
/;[ ]*do/bcont
/^[ ]*done[ ]*&&[ ]*$/bdone
/^[ ]*done[ ]*$/bdone
/^[ ]*done[ ]*[<>|]/bdone
/^[ ]*done[ ]*)/bdone
/||[ ]*exit[ ]/bcontinue
/||[ ]*exit[ ]*$/bcontinue
/||[ ]*exit[ ]/bcont
/||[ ]*exit[ ]*$/bcont
# multi-line "if...elsif...else...fi"
/^[ ]*if[ ]/bcontinue
/^[ ]*then[ ]/bcontinue
/^[ ]*then[ ]*$/bcontinue
/;[ ]*then/bcontinue
/^[ ]*if[ ]/bcont
/^[ ]*then[ ]/bcont
/^[ ]*then[ ]*$/bcont
/;[ ]*then/bcont
/^[ ]*elif[ ]/belse
/^[ ]*elif[ ]*$/belse
/^[ ]*else[ ]/belse
Expand Down Expand Up @@ -234,10 +234,10 @@ s/.*\n//
}
}
# line ends with pipe "...|" -- valid; not missing "&&"
/|[ ]*$/bcontinue
/|[ ]*$/bcont
# missing end-of-line "&&" -- mark suspect
/&&[ ]*$/!s/^/?!AMP?!/
:continue
:cont
# retrieve and print previous line
x
n
Expand All @@ -250,29 +250,29 @@ s/\\\n//
bslurp

# check for multi-line double-quoted string "...\n..." -- fold to one line
:dqstring
:dqstr
# remove all quote pairs
s/"\([^"]*\)"/@!\1@!/g
# done if no dangling quote
/"/!bdqdone
# otherwise, slurp next line and try again
N
s/\n//
bdqstring
bdqstr
:dqdone
s/@!/"/g
bfolded

# check for multi-line single-quoted string '...\n...' -- fold to one line
:sqstring
:sqstr
# remove all quote pairs
s/'\([^']*\)'/@!\1@!/g
# done if no dangling quote
/'/!bsqdone
# otherwise, slurp next line and try again
N
s/\n//
bsqstring
bsqstr
:sqdone
s/@!/'/g
bfolded
Expand All @@ -282,11 +282,11 @@ bfolded
:heredoc
s/^\(.*\)<<[ ]*[-\\'"]*\([A-Za-z0-9_][A-Za-z0-9_]*\)['"]*/<\2>\1<</
s/[ ]*<<//
:heredsub
:hdocsub
N
/^<\([^>]*\)>.*\n[ ]*\1[ ]*$/!{
s/\n.*$//
bheredsub
bhdocsub
}
s/^<[^>]*>//
s/\n.*$//
Expand All @@ -305,7 +305,7 @@ bcase
x
s/?!AMP?!//
x
bcontinue
bcont

# found "done" closing for-loop or while-loop, or "fi" closing if-then -- drop
# "suspect" from final contained line since that line legitimately lacks "&&"
Expand All @@ -321,22 +321,22 @@ bchkchn
# found nested multi-line "(...\n...)" -- pass through untouched
:nest
x
:nstslurp
:nstslrp
n
# closing ")" on own line -- stop nested slurp
/^[ ]*)/bnstclose
/^[ ]*)/bnstcl
# comment -- not closing ")" if in comment
/^[ ]*#/bnstcnt
# "$((...))" -- arithmetic expansion; not closing ")"
/\$(([^)][^)]*))[^)]*$/bnstcnt
# "$(...)" -- command substitution; not closing ")"
/\$([^)][^)]*)[^)]*$/bnstcnt
# closing "...)" -- stop nested slurp
/)/bnstclose
/)/bnstcl
:nstcnt
x
bnstslurp
:nstclose
bnstslrp
:nstcl
s/^/>>/
# is it "))" which closes nested and parent subshells?
/)[ ]*)/bslurp
Expand Down