From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 08 Jan 2008 22:54:54 GMT
Subject: Re: [PATCH] gitk: Update and fix Makefile
Message-ID: <7vk5mkq669.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <200801082154.21282.stimming@tuhh.de>

Christian Stimming <stimming@tuhh.de> writes:

> -# 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.