Re: [PATCH] mergetools: Simplify how we handle "vim" and "defaults"

From: John Keeping <john@keeping.me.uk>
Date: Jan 26, 2013, 20:54 UTC
Message-ID: <20130126205434.GL7498@serenity.lan>
In-Reply-To: <CAJDDKr5UMyJOthSOPuChx7BCpcGwtmYcnVMh83q9kgCWSxDp4w@mail.gmail.com>

On Sat, Jan 26, 2013 at 12:40:23PM -0800, David Aguilar wrote:

Show 27 quoted lines

> On Sat, Jan 26, 2013 at 4:12 AM, John Keeping <john@keeping.me.uk> wrote:
> > On Fri, Jan 25, 2013 at 10:50:58PM -0800, David Aguilar wrote:
> >> diff --git a/Makefile b/Makefile
> >> index f69979e..3bc6eb5 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -2724,7 +2724,7 @@ install: all
> >>       $(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'
> >>       $(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
> >>       $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
> >> -     $(INSTALL) -m 644 mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
> >> +     cp -R mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
> >
> > Shouldn't this be more like this?
> >
> >         $(INSTALL) -m 644 $(subst include,,$(wildcard mergetools/*)) \
> >                 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
> >         $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'
> >         $(INSTALL) -m 644 mergetools/include/* \
> >                 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'
> >
> > I'm not sure creating an 'include' directory actually buys us much over
> > declaring that 'vimdiff' is the real script and the others just source
> > it.  Either way there is a single special entry in the mergetools
> > directory.
>
> Thanks (and thanks for the Makefile hint.. I knew it was wrong! ;-).

I think that version's still not right actually, the first line should probably use filter-out not subst:

	$(INSTALL) -m 644 $(filter-out include,$(wildcard mergetools/*)) \
		'$(DESTDIR_SQ)$(mergetools_instdir_SQ)'

John