Re: [PATCH] ppc: remove custom SHA-1 implementation

[Junio C Hamano <gitster@pobox.com>]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> The main reason for doing so at this point is to simplify follow-up
> Makefile change. Since PPC_SHA1 included the only in-tree *.S assembly
> file we needed to keep around special support for building objects
> from it. By getting rid of it we know we'll always build *.o from *.c
> files, which makes the build process simpler.

Yuck.

> diff --git a/Makefile b/Makefile
> index 70f0a004e75..965e51f773e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -155,9 +155,6 @@ include shared.mak
>  # Define BLK_SHA1 environment variable to make use of the bundled
>  # optimized C SHA1 routine.
>  #
> -# Define PPC_SHA1 environment variable when running make to make use of
> -# a bundled SHA1 routine optimized for PowerPC.
> -#
>  # Define DC_SHA1 to unconditionally enable the collision-detecting sha1
>  # algorithm. This is slower, but may detect attempted collision attacks.
>  # Takes priority over other *_SHA1 knobs.
> @@ -1770,14 +1767,14 @@ ifdef OPENSSL_SHA1
>  	EXTLIBS += $(LIB_4_CRYPTO)
>  	BASIC_CFLAGS += -DSHA1_OPENSSL
>  else
> +ifdef PPC_SHA1
> +$(error PPC_SHA1 has been removed! You should almost definitely remove that \
> +knob and use the DC_SHA1 default! See INSTALL for more information)
> +endif

"use the DC_SHA1 default"?  

	PPC_SHA1 support is being removed.  Use DC_SHA1 instead, 
	which is the default.


I am wondering if we can make only these four lines the first step
of remova, without doing anything else.  It would give us a good
feel on how many users we may be inconveniencing (not necessarily
hurting, as switching to DC_SHA1 would be a good move) by the
removal.

> @@ -2509,6 +2505,11 @@ OBJECTS += $(SCALAR_OBJECTS)
>  .PHONY: objects
>  objects: $(OBJECTS)
>  
> +# Derived from $(OBJECTS)
> +OBJECTS_C = $(OBJECTS:%.o=%.c)
> +OBJECTS_S = $(OBJECTS:%.o=%.s)
> +OBJECTS_SP = $(OBJECTS:%.o=%.sp)

Usually we build objects from sources, so "derived from" is a
puzzling way to call them.  I understand we are deriving a list from
another list, but it still feels confusing (see below for ASM_OBJ).

This seems to have nothing to do with "we no longer have *.S files"
and looks more like "now we have excuse to touch Makefile, make
random changes that look subjectively good to me, burying them in
the noise so that we can sneak them in without justifying them much
in the proposed log message".

>  dep_files := $(foreach f,$(OBJECTS),$(dir $f).depend/$(notdir $f).d)
>  dep_dirs := $(addsuffix .depend,$(sort $(dir $(OBJECTS))))
>  
> @@ -2540,13 +2541,7 @@ missing_compdb_dir =
>  compdb_args =
>  endif
>  
> -ASM_SRC := $(wildcard $(OBJECTS:o=S))
> -ASM_OBJ := $(ASM_SRC:S=o)
> -C_OBJ := $(filter-out $(ASM_OBJ),$(OBJECTS))

I tend to agree with this patch that these three lines are ugly in
multiple ways.

It's a confusing construct.  The list of OBJECTS is used as the
single source of truth and others are derived by filtering the list
and futzing the suffix of the resulting subset of elements; it makes
me wonder if it should be the other way around (i.e. we have a list
of source files in various languages, all get turned into objects,
rather than we have a list of object files, and we see if
corresponding source file in possible languages, if any, exists on
disk).  It is pleasing that we can see them go.

Unfortunately the new "Derived from $(OBJECTS) lists are in the same
spirit as used by ASM_SRC we are removing here, aren't they?

> -$(C_OBJ): %.o: %.c GIT-CFLAGS $(missing_dep_dirs) $(missing_compdb_dir)
> -	$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(compdb_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
> -$(ASM_OBJ): %.o: %.S GIT-CFLAGS $(missing_dep_dirs) $(missing_compdb_dir)
> +$(OBJECTS): %.o: %.c GIT-CFLAGS $(missing_dep_dirs) $(missing_compdb_dir)
>  	$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(compdb_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<

Now we deal only with *.c sources and all objects come from them,
and the action is the same as the old C_OBJ rule, naturally.

>  %.s: %.c GIT-CFLAGS FORCE

This is the only remaining rule regarding assembly and it is the
target to generate for debugging, not even used as a source to
create object files.  Do we need OBJECTS_S defined above?  I somehow
doubt it.  FWIW, I do not see the need for OBJECTS_SP or OBJECTS_C,
either.  E.g.

> @@ -2692,7 +2687,7 @@ XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
>  	--keyword=gettextln --keyword=eval_gettextln
>  XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --language=Perl \
>  	--keyword=__ --keyword=N__ --keyword="__n:1,2"
> -LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
> +LOCALIZED_C = $(OBJECTS_C) $(LIB_H) $(GENERATED_H)

Shouldn't it be sufficient to use %(OBJECTS:o=c) here, i.e. equating
the old C_OBJ with OBJECTS, now we know all objects come from C?
The same question for SP_OBJ below, but I won't repeat.