Re: [PATCH] gitk: Update and fix Makefile

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 8, 2008, 22:54 UTC
Message-ID: <7vk5mkq669.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <200801082154.21282.stimming@tuhh.de>

Christian Stimming <stimming@tuhh.de> writes:

Show 7 quoted lines

> -# From here on, these are needed in git.git/gitk/Makefile.
> +prefix ?= $(HOME)
> +bindir ?= $(prefix)/bin
> +sharedir ?= $(prefix)/share
>  gitk_libdir   ?= $(sharedir)/gitk/lib
>  msgsdir    ?= $(gitk_libdir)/msgs
>  msgsdir_SQ  = $(subst ','\'',$(msgsdir))

I see somwhat funny spacing there. I'd suggest giving up aligning with spaces and consistently saying "var ?= val" instead.

	I am reading the diff between the gitk-git/Makefile
	before and after merging gitk with your patch and
	the last three lines above are not context but additions
	from my point of view.

> +install:: all
> +	$(INSTALL) gitk-wish '$(DESTDIR_SQ)$(bindir_SQ)'/gitk
> +	$(INSTALL) -d '$(DESTDIR_SQ)$(msgsdir_SQ)'
> +	$(foreach p,$(ALL_MSGFILES), $(INSTALL) $p '$(DESTDIR_SQ)$(msgsdir_SQ)' &&) true

This is cute and correct (except I would have spelled "true" as ":" myself). I think we need to fix a few such constructs that use ";" instead of "&&" in Makefile in git.git.

> +uninstall::
> +	$(foreach p,$(ALL_MSGFILES), $(RM) '$(DESTDIR_SQ)$(msgsdir_SQ)'/$(notdir $p) &&) true
> +	$(RM) '$(DESTDIR_SQ)$(bindir_SQ)'/gitk

I have a mild dislike against uninstall target, but that's Paul's call.

> +clean::
> +	$(RM) gitk-wish po/*.msg

And this makes me wonder if the last token should be $(ALL_MSGFILES).

Other than that, Ack from me.

Thanks.