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

From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Apr 30, 2014, 11:56 UTC
Message-ID: <CABPQNSafKXDLyBj5OYW-PPWdxfxQtC23vKQsQ-_Pa1empU=n8g@mail.gmail.com>
In-Reply-To: <20140430112724.GA22929@camelia.ucw.cz>

On Wed, Apr 30, 2014 at 1:27 PM, Stepan Kasal <kasal@ucw.cz> wrote:

Show 23 quoted lines

> Hello,
>
>> 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:
>> 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.

I doubt that a patch that doesn't describe exactly what kind of issues will get merged. And it certainly won't get my ack unless I understand why.

> It is probably much sane on Windows to use the compiler that the user
> set up for the build.

But you can already do that, the same way as for the rest of git, by overloading these in config.mak in the root of the git repo.

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

Even if I bought that it's needed (which I'm currently skeptical to), I think the "dunno about OSX" is a bit of a cop-out.

Show 33 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.

Following that logic, you should be submitting similar patches to the main git Makefile as well. Somehow I doubt that'll happen.

> Moreover, if the cardinality of the set ever increases, the
> indirection may get helpful.

I don't think there's any reason to expect the number of binaries to increase, so that's moot. And if I'm wrong, let's deal with that when the time comes. It's not like this version is prepared for the variable being a list either - neither should there IMO.

Show 9 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.

We don't usually do "this is subjectively better"-patches in Git. Instead we try to follow the current style.