patch, 11 partsMove #include <sys/select.h> and <sys/ioctl.h> to git-compat-util.h.

> If ETC_GITCONFIG is not an absolute path, interpret it relative to
> --exec-dir. This makes the installed binaries relocatable because the
> prefix is not compiled-in.
>
> Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
> ---
>  config.c |   13 ++++++++++++-
>  1 files changed, 12 insertions(+), 1 deletions(-)
>
> diff --git a/config.c b/config.c
> index dd7e9ad..9f014bb 100644
> --- a/config.c
> +++ b/config.c
> @@ -6,6 +6,7 @@
>   *
>   */
>  #include "cache.h"
> +#include "exec_cmd.h"
>
>  #define MAXNAME (256)
>
> @@ -454,7 +455,17 @@ int git_config_from_file(config_fn_t fn, const  
> char *filename)
>
>  const char *git_etc_gitconfig(void)
>  {
> -	return ETC_GITCONFIG;
> +	static const char *system_wide;
> +	if (!system_wide) {
> +		system_wide = ETC_GITCONFIG;
> +		if (!is_absolute_path(system_wide)) {
> +			/* interpret path relative to exec-dir */
> +			const char *exec_path = git_exec_path();
> +			system_wide = prefix_path(exec_path, strlen(exec_path),
> +						system_wide);
> +		}
> +	}
> +	return system_wide;
>  }

> Now I'm wondering if we could make path relocation a bit more
> explicit.  How about doing something like.
>
> 	system_wide = relocate_path(ETC_GITCONFIG);
>
> and relocate_path(const char *) would expand a format
> string in path.  At this point I see only a single %s
> that would be expanded with the install prefix.  If
> ETC_GITCONFIG is "%s/etc/gitconfig" relocate path will return
> "C:/msysgit/bin/etc/gitconfig" for my above example.  It is
> basically a printf with the install prefix path.

> On Thursday 15 November 2007 07:53, Steffen Prohaska wrote:
>> Now I'm wondering if we could make path relocation a bit more
>> explicit.  How about doing something like.
>>
>> 	system_wide = relocate_path(ETC_GITCONFIG);
>>
>> and relocate_path(const char *) would expand a format
>> string in path.  At this point I see only a single %s
>> that would be expanded with the install prefix.  If
>> ETC_GITCONFIG is "%s/etc/gitconfig" relocate path will return
>> "C:/msysgit/bin/etc/gitconfig" for my above example.  It is
>> basically a printf with the install prefix path.
>
> I don't see the problem that you are trying to solve.

> On Nov 15, 2007, at 7:31 PM, Johannes Sixt wrote:
> > On Thursday 15 November 2007 07:53, Steffen Prohaska wrote:
> >> Now I'm wondering if we could make path relocation a bit more
> >> explicit.  How about doing something like.
> >>
> >> 	system_wide = relocate_path(ETC_GITCONFIG);
> >>
> >> and relocate_path(const char *) would expand a format
> >> string in path.  At this point I see only a single %s
> >> that would be expanded with the install prefix.  If
> >> ETC_GITCONFIG is "%s/etc/gitconfig" relocate path will return
> >> "C:/msysgit/bin/etc/gitconfig" for my above example.  It is
> >> basically a printf with the install prefix path.
> >
> > I don't see the problem that you are trying to solve.
>
> Path relocation would be more explicit:
> 1) Paths that need to be relocated are filtered through
>     relocate_path().  That's easy to understand.
> 2) All the code prefixing the path is in relocate_path().
>     This avoids code duplication.
> 3) Path that should be relocated contain "%s" in the Makefile.
>     This indicates that some string expansion may take place.
>
> (1) and (2) would be useful even if you do not agree with (3).
> The code in PATCH 9/11 and PATCH 11/11 looks very similar.
> If the prefixing code went into a separate function, we'd
> have less code.  Also, relocate_path() could be useful at
> other places.  For example, I'd use it to locate the HTML
> documentation relative to the installation directory.

> On Thursday 15 November 2007 21:10, Steffen Prohaska wrote:
>> On Nov 15, 2007, at 7:31 PM, Johannes Sixt wrote:
>>> On Thursday 15 November 2007 07:53, Steffen Prohaska wrote:
>>>> Now I'm wondering if we could make path relocation a bit more
>>>> explicit.  How about doing something like.
>>>>
>>>> 	system_wide = relocate_path(ETC_GITCONFIG);
>>>>
>>>> and relocate_path(const char *) would expand a format
>>>> string in path.  At this point I see only a single %s
>>>> that would be expanded with the install prefix.  If
>>>> ETC_GITCONFIG is "%s/etc/gitconfig" relocate path will return
>>>> "C:/msysgit/bin/etc/gitconfig" for my above example.  It is
>>>> basically a printf with the install prefix path.
>>>
>>> I don't see the problem that you are trying to solve.
>>
>> Path relocation would be more explicit:
>> 1) Paths that need to be relocated are filtered through
>>     relocate_path().  That's easy to understand.
>> 2) All the code prefixing the path is in relocate_path().
>>     This avoids code duplication.
>> 3) Path that should be relocated contain "%s" in the Makefile.
>>     This indicates that some string expansion may take place.
>>
>> (1) and (2) would be useful even if you do not agree with (3).
>> The code in PATCH 9/11 and PATCH 11/11 looks very similar.
>> If the prefixing code went into a separate function, we'd
>> have less code.  Also, relocate_path() could be useful at
>> other places.  For example, I'd use it to locate the HTML
>> documentation relative to the installation directory.
>
> How is %s/foo different from ../foo? There are only 2 paths that  
> need to be
> relocatable. Your proposal is over-engineering, IMHO.

> diff --git a/config.c b/config.c
> index dc3148d..dd7e9ad 100644
> --- a/config.c
> +++ b/config.c
> @@ -452,6 +452,11 @@ int git_config_from_file(config_fn_t fn, const char *filename)
>  	return ret;
>  }
>  
> +const char *git_etc_gitconfig(void)
> +{
> +	return ETC_GITCONFIG;
> +}
> +

> Hi,
>
> On Tue, 13 Nov 2007, Johannes Sixt wrote:
> > diff --git a/config.c b/config.c
> > index dc3148d..dd7e9ad 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -452,6 +452,11 @@ int git_config_from_file(config_fn_t fn, const char
> > *filename) return ret;
> >  }
> >
> > +const char *git_etc_gitconfig(void)
> > +{
> > +	return ETC_GITCONFIG;
> > +}
> > +
>
> Wouldn't this logically belong into environment.c?

> On Tuesday 13 November 2007 22:22, Johannes Schindelin wrote:
>
> > On Tue, 13 Nov 2007, Johannes Sixt wrote:
> > > diff --git a/config.c b/config.c
> > > index dc3148d..dd7e9ad 100644
> > > --- a/config.c
> > > +++ b/config.c
> > > @@ -452,6 +452,11 @@ int git_config_from_file(config_fn_t fn, const char
> > > *filename) return ret;
> > >  }
> > >
> > > +const char *git_etc_gitconfig(void)
> > > +{
> > > +	return ETC_GITCONFIG;
> > > +}
> > > +
> >
> > Wouldn't this logically belong into environment.c?
> 
> I don't think so. environment.c is about things that concern the 
> repository. This is really only about configuration.

> From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> 
> This is needed on Windows since open files cannot be unlinked.
> 
> Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
> ---
> 
> 	This was authored by Dscho, but carries only my sign-off.
> 	Is this ok?

> diff --git a/sha1_file.c b/sha1_file.c
> index f007874..560c0e0 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -86,7 +86,7 @@ int safe_create_leading_directories(char *path)
>  	char *pos = path;
>  	struct stat st;
>  
> -	if (*pos == '/')
> +	if (is_absolute_path(path))
>  		pos++;

> Hi,
>
> On Tue, 13 Nov 2007, Johannes Sixt wrote:
> > diff --git a/sha1_file.c b/sha1_file.c
> > index f007874..560c0e0 100644
> > --- a/sha1_file.c
> > +++ b/sha1_file.c
> > @@ -86,7 +86,7 @@ int safe_create_leading_directories(char *path)
> >  	char *pos = path;
> >  	struct stat st;
> >
> > -	if (*pos == '/')
> > +	if (is_absolute_path(path))
> >  		pos++;
>
> Is this enough?  On Windows, certain "absolute" paths would need "pos +=
> 3", no?

> On Tuesday 13 November 2007 22:39, Johannes Schindelin wrote:
>
> > On Tue, 13 Nov 2007, Johannes Sixt wrote:
> > > diff --git a/sha1_file.c b/sha1_file.c
> > > index f007874..560c0e0 100644
> > > --- a/sha1_file.c
> > > +++ b/sha1_file.c
> > > @@ -86,7 +86,7 @@ int safe_create_leading_directories(char *path)
> > >  	char *pos = path;
> > >  	struct stat st;
> > >
> > > -	if (*pos == '/')
> > > +	if (is_absolute_path(path))
> > >  		pos++;
> >
> > Is this enough?  On Windows, certain "absolute" paths would need "pos +=
> > 3", no?
> 
> True, but this series is not yet about the MinGW port itself. It will be 
> changed eventually, but not at this time.

> On Tuesday 13 November 2007 22:39, Johannes Schindelin wrote:
> > Hi,
> >
> > On Tue, 13 Nov 2007, Johannes Sixt wrote:
> > > diff --git a/sha1_file.c b/sha1_file.c
> > > index f007874..560c0e0 100644
> > > --- a/sha1_file.c
> > > +++ b/sha1_file.c
> > > @@ -86,7 +86,7 @@ int safe_create_leading_directories(char *path)
> > >  	char *pos = path;
> > >  	struct stat st;
> > >
> > > -	if (*pos == '/')
> > > +	if (is_absolute_path(path))
> > >  		pos++;
> >
> > Is this enough?  On Windows, certain "absolute" paths would need "pos +=
> > 3", no?
> 
> True, but this series is not yet about the MinGW port itself. It will be 
> changed eventually, but not at this time.

> [PATCH 09/11] Allow a relative builtin template directory.
> [PATCH 10/11] Introduce git_etc_gitconfig() that encapsulates access
> 	of ETC_GITCONFIG.
> [PATCH 11/11] Allow ETC_GITCONFIG to be a relative path.
>
> These need probably some discussion. They avoid that $(prefix) is
> hardcoded and so allows that an arbitrary installation directory.

> On Tuesday 13 November 2007 21:04, Johannes Sixt wrote:
>> [PATCH 09/11] Allow a relative builtin template directory.
>> [PATCH 10/11] Introduce git_etc_gitconfig() that encapsulates access
>> 	of ETC_GITCONFIG.
>> [PATCH 11/11] Allow ETC_GITCONFIG to be a relative path.
>>
>> These need probably some discussion. They avoid that $(prefix) is
>> hardcoded and so allows that an arbitrary installation directory.
>
> ... and so allow that the compiled binaries are installed in any directory 
> that the user chooses.

> Johannes Sixt <johannes.sixt@telecom.at> writes:
> > On Tuesday 13 November 2007 21:04, Johannes Sixt wrote:
> >> [PATCH 09/11] Allow a relative builtin template directory.
> >> [PATCH 10/11] Introduce git_etc_gitconfig() that encapsulates access
> >> 	of ETC_GITCONFIG.
> >> [PATCH 11/11] Allow ETC_GITCONFIG to be a relative path.
> >>
> >> These need probably some discussion. They avoid that $(prefix) is
> >> hardcoded and so allows that an arbitrary installation directory.
> >
> > ... and so allow that the compiled binaries are installed in any
> > directory that the user chooses.
>
> If you can do that without breaking the tests (specifically, the
> test script should pick up the version of git you just built,
> not from /usr/bin nor /usr/local/stow/git/bin) that would be
> great.

> On Tuesday 13 November 2007 22:10, Junio C Hamano wrote:
> > Johannes Sixt <johannes.sixt@telecom.at> writes:
> > > On Tuesday 13 November 2007 21:04, Johannes Sixt wrote:
> > >> [PATCH 09/11] Allow a relative builtin template directory.
> > >> [PATCH 10/11] Introduce git_etc_gitconfig() that encapsulates access
> > >> 	of ETC_GITCONFIG.
> > >> [PATCH 11/11] Allow ETC_GITCONFIG to be a relative path.
> > >>
> > >> These need probably some discussion. They avoid that $(prefix) is
> > >> hardcoded and so allows that an arbitrary installation directory.
> > >
> > > ... and so allow that the compiled binaries are installed in any
> > > directory that the user chooses.
> >
> > If you can do that without breaking the tests (specifically, the
> > test script should pick up the version of git you just built,
> > not from /usr/bin nor /usr/local/stow/git/bin) that would be
> > great.
> 
> Sorry, I don't understand your statement. Do you see any tests breaking?

> Hi,
>
> On Tue, 13 Nov 2007, Johannes Sixt wrote:
> > On Tuesday 13 November 2007 22:10, Junio C Hamano wrote:
> > > Johannes Sixt <johannes.sixt@telecom.at> writes:
> > > > On Tuesday 13 November 2007 21:04, Johannes Sixt wrote:
> > > >> [PATCH 09/11] Allow a relative builtin template directory.
> > > >> [PATCH 10/11] Introduce git_etc_gitconfig() that encapsulates access
> > > >> 	of ETC_GITCONFIG.
> > > >> [PATCH 11/11] Allow ETC_GITCONFIG to be a relative path.
> > > >>
> > > >> These need probably some discussion. They avoid that $(prefix) is
> > > >> hardcoded and so allows that an arbitrary installation directory.
> > > >
> > > > ... and so allow that the compiled binaries are installed in any
> > > > directory that the user chooses.
> > >
> > > If you can do that without breaking the tests (specifically, the
> > > test script should pick up the version of git you just built,
> > > not from /usr/bin nor /usr/local/stow/git/bin) that would be
> > > great.
> >
> > Sorry, I don't understand your statement. Do you see any tests breaking?
>
> I guess what Junio is getting at: if your changes could lead to our not
> needing to hard code defaults, that would be awesome.
>
> For example, a very unhappy camper reported recently that installing git
> with a different prefix triggers a complete rebuild.

> On Tue, 13 Nov 2007, Johannes Sixt wrote:
>> On Tuesday 13 November 2007 22:10, Junio C Hamano wrote:
>> > Johannes Sixt <johannes.sixt@telecom.at> writes:
>> > > On Tuesday 13 November 2007 21:04, Johannes Sixt wrote:
>> > > ... and so allow that the compiled binaries are installed in any
>> > > directory that the user chooses.
>> >
>> > If you can do that without breaking the tests (specifically, the
>> > test script should pick up the version of git you just built,
>> > not from /usr/bin nor /usr/local/stow/git/bin) that would be
>> > great.
>> 
>> Sorry, I don't understand your statement. Do you see any tests breaking?
>
> I guess what Junio is getting at: if your changes could lead to our not 
> needing to hard code defaults, that would be awesome.

> [PATCH 05/11] Use is_absolute_path() in sha1_file.c.
> [PATCH 06/11] Move #include <sys/select.h> and <sys/ioctl.h> to
> 	git-compat-util.h.
>
> These two are certainly undisputed.

> [PATCH 09/11] Allow a relative builtin template directory.
> [PATCH 10/11] Introduce git_etc_gitconfig() that encapsulates access
> 	of ETC_GITCONFIG.
> [PATCH 11/11] Allow ETC_GITCONFIG to be a relative path.
>
> These need probably some discussion. They avoid that $(prefix) is
> hardcoded and so allows that an arbitrary installation directory.

> Johannes Sixt <johannes.sixt@telecom.at> writes:
> 
> > This is a series of smallish, unrelated changes that were necessary
> > for the MinGW port.
> 
> I was _VERY_ afraid of reviewing this series.

> > [PATCH 05/11] Use is_absolute_path() in sha1_file.c.
> > [PATCH 06/11] Move #include <sys/select.h> and <sys/ioctl.h> to
> > 	git-compat-util.h.
> >
> > These two are certainly undisputed.
> 
> Except on esoteric/broken systems there might be some dependency
> on the order the system include files are included, so 06/11
> needs some testing.  But it is a change in the right direction.

> > [PATCH 08/11] Close files opened by lock_file() before unlinking.
> >
> > This one was authored by Dscho. It is a definite MUST on Windows.
> 
> This was something we've talked about doing a few times on the
> list but did not.  It is good that this saw some testing in the
> field, as it is easy to get wrong while moving the call site of
> close(2) around.

> > [PATCH 09/11] Allow a relative builtin template directory.
> > [PATCH 10/11] Introduce git_etc_gitconfig() that encapsulates access
> > 	of ETC_GITCONFIG.
> > [PATCH 11/11] Allow ETC_GITCONFIG to be a relative path.
> >
> > These need probably some discussion. They avoid that $(prefix) is
> > hardcoded and so allows that an arbitrary installation directory.
> 
> I had to worry a bit about bootstrapping issues in 11/11.  We
> need to ensure that anybody who wants to read the configuration
> data first does setup_path() because git_exec_path() reads from
> argv_exec_path and setup_path() is what assigns to that
> variable.

> On Wed, 14 Nov 2007, Junio C Hamano wrote:
>
>> Johannes Sixt <johannes.sixt@telecom.at> writes:
>> 
>> > This is a series of smallish, unrelated changes that were necessary
>> > for the MinGW port.
>> 
>> I was _VERY_ afraid of reviewing this series.
>
> Why?  Because we get closer to MinGW integration into git.git for real? 
> ;-)

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Wed, 14 Nov 2007, Junio C Hamano wrote:
> >
> >> Johannes Sixt <johannes.sixt@telecom.at> writes:
> >> 
> >> > This is a series of smallish, unrelated changes that were necessary
> >> > for the MinGW port.
> >> 
> >> I was _VERY_ afraid of reviewing this series.
> >
> > Why?  Because we get closer to MinGW integration into git.git for real? 
> > ;-)
> 
> I know you know me better than that.
> 
> Conversion from "Too UNIXy" stuff into another form that "claims to" run 
> on a different platform that I do not have a good way of testing myself?  
> I should feel scared, even if I can always punt and say "I'll only make 
> sure UNIX side does not regress, Windose, who cares" ;-).