Re: [PATCH] wincred: add install target and avoid overwriting configured variables

From: Stepan Kasal <kasal@ucw.cz>
Date: Apr 30, 2014, 11:27 UTC
Message-ID: <20140430112724.GA22929@camelia.ucw.cz>
In-Reply-To: <CABPQNSZsviaGqFeKZE4ofF6HoUQrPvNPuowar4YDjk_Mbu5iCQ@mail.gmail.com>

Hello,

Show 9 quoted lines

> On Wed, Apr 30, 2014 at 8:46 AM, Stepan Kasal <kasal@ucw.cz> wrote:
> > Date: Wed, 24 Oct 2012 00:15:29 +0100
> >
> > Signed-off-by: Pat Thoyts <patthoyts@users.sourceforge.net>
> > Signed-off-by: Stepan Kasal <kasal@ucw.cz>
> > ---
> > Another one from msysGit project.
> > Original subject by Pat; I would suggest:
> >     wincred: improve Makefile

On Wed, Apr 30, 2014 at 11:21:17AM +0200, Erik Faye-Lund wrote:

Show 7 quoted lines

> I'm a little bit unsure about this, because the makefile was basically
> just copied from contrib/credential/osxkeychain/Makefile (which was
> the first credential helper) and tweaked slightly.
> 
> So, what makes wincred special compared to gnome-keyring, netrc and
> osxkeychain wrt installation? Shouldn't all helpers get the same
> treatment?

I can only guess that the hardwired CC and CFLAGS values can cause problems. It is probably much sane on Windows to use the compiler that the user set up for the build.

I'm not sure if users of, say, OS X, have the same problems, so I would hesitate to apply these changes to all helpers.

Show 29 quoted lines

> > From: Pat Thoyts <patthoyts@users.sourceforge.net>
> >  contrib/credential/wincred/Makefile | 22 ++++++++++++++--------
> >  1 file changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/contrib/credential/wincred/Makefile b/contrib/credential/wincred/Makefile
> > index bad45ca..3ce6aba 100644
> > --- a/contrib/credential/wincred/Makefile
> > +++ b/contrib/credential/wincred/Makefile
> > @@ -1,14 +1,20 @@
> > -all: git-credential-wincred.exe
> > -
> > -CC = gcc
> > -RM = rm -f
> > -CFLAGS = -O2 -Wall
> > -
> >  -include ../../../config.mak.autogen
> >  -include ../../../config.mak
> >
> > -git-credential-wincred.exe : git-credential-wincred.c
> > +prefix ?= /usr/local
> > +libexecdir ?= $(prefix)/libexec/git-core
> > +
> > +INSTALL ?= install
> > +
> > +GIT_CREDENTIAL_WINCRED := git-credential-wincred.exe
> 
> Why this variable? IMO, it's just as "GIT_CREDENTIAL_WINCRED" easy to
> miss-spell as "git-credential-wincred.exe", and it doesn't seem to be
> possible to overload.

If you mis-spell a variable name, nothing is build. If you misspell a binary name, that binary may get compiled using a default rule; that is why I would generally prefer variables. Moreover, if the cardinality of the set ever increases, the indirection may get helpful. No big deal.

Show 6 quoted lines

> > +
> > +all: $(GIT_CREDENTIAL_WINCRED)
> > +
> 
> Also, why move the all-target down from the top? Is it simply because
> of the definition above?

Again, I agree with Pat that it is nicer this way, but no big deal.

Stepan Kasal