Re: [PATCH 4/6] config: return configset value for current_config_ functions

[Junio C Hamano]

Jeff King  writes:

> I think the issue is that bin-wrappers serves two purposes. One is for
> testing, but the other is for people who run git directly without
> installing.

Hmph.  It may have been a useful way to "run without installing"
once in the past, but with the "check and run it under GDB" etc.,
I am not sure if it still is.  I certainly did not think about that
as a valid use case when I wrote the message you are responding to.

> I think you can make it even simpler by not really doing a "make
> install", but just linking or bin-wrappering a fake exec-path. It would
> be great if we could truly just "make install" into a fake area and test
> that (dropping bin-wrappers entirely), but git cares too much about some
> hard-coded paths, I think. We'd have to first have a truly relocatable
> binary.

Looking at what bin-wrappers do, I do not think "hard coded paths"
is something we need them for.  If we wanted to make the "test what
we just built" and the "test what is already installed" closely
mimic each other, I have a feeling that setting of the environment
variable done by the bin-wrappers can and should be moved to the
test framework.  When testing what we just built, set these
environment variables you see in any of the bin-wrappers/ script to
point at various places in the "make DESTDIR=there install" tree,
and when testing what is installed, set them to different values
(possibly "nothing", e.g. GIT_EXEC_PATH would not be needed if we
are testing an installed and already working version).

So I suspect "truly relocatable binary" is necessary for testing.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] config: return configset value for current_config_ functions

[Jeff King]

On Thu, May 26, 2016 at 10:36:44AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > The problem is running test-config inside of a git alias. The
> > bin-wrappers will set the exec-path to the root-level of git's build
> > directory, which the git binary will then stick at the front of the
> > $PATH.
> 
> I was wondering why exec-path does not point at bin-wrappers in the
> first place.
> 
> A wrapper script needs to know where the real thing lives in order
> to "exec" (or "exec gdb") anyway, and it hardcodes the path to it.
> It happens to use GIT_EXEC_PATH to shorten the hardcoded path it
> uses when it does "exec", but it does not have to.
> 
> Wouldn't we want to see "git" use any of these wrapped ones when it
> invokes a non-builtin subcommand when it does so normally, honoring
> GIT_EXEC_PATH?  Pointing GIT_EXEC_PATH at the top-level means that
> wrappers are bypassed for such an invocation (if what is run happens
> to have executable at the top-level), and possibly a totally wrong
> thing is run (when we start generating the binaries in different
> directories, which is what we are seeing here).

I think the issue is that bin-wrappers serves two purposes. One is for
testing, but the other is for people who run git directly without
installing. For us to set GIT_EXEC_PATH to bin-wrappers, it would have
to have all of the git-* external programs, which would then put them
all in the $PATH of people doing the no-install thing.

That's certainly not insurmountable. Either we can tell them to live
with it, or we can break out a separate wrapper directory that serves as
a pseudo-exec-path.

> > So the simplest solution really is: don't do that. The only debate
> > in my mind is whether this is rare enough that it won't bite
> > somebody again in the future, or if we should look into a solution
> > that makes this Just Work.
> 
> I think it was you who alluded to revamping the test framework along
> the lines of preparing a "test installation" (aka "make install"
> with DESTDIR set to somewhere) and making bin-wrappers to point into
> that installation (or if we are testing an installed Git that may be
> different from what we have source for, that final installed
> location).  An installed version of Git will not have test-* helpers
> so they need to come from a freshly built source tree, not from
> "test installation" or "installed Git".  There may be other details
> that need to be worked out, but as a longer term direction that may
> not be a bad idea.

I think you can make it even simpler by not really doing a "make
install", but just linking or bin-wrappering a fake exec-path. It would
be great if we could truly just "make install" into a fake area and test
that (dropping bin-wrappers entirely), but git cares too much about some
hard-coded paths, I think. We'd have to first have a truly relocatable
binary.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] config: return configset value for current_config_ functions

[Jeff King]

On Thu, May 26, 2016 at 12:50:33PM -0400, Jeff King wrote:

> > Remind me why we end up running ./test-config instead of
> > ./bin-wrappers/test-config?  Should our tests be running 
> > bin-wrappers early in their $PATH, perhaps?
> 
> The problem is running test-config inside of a git alias. The
> bin-wrappers will set the exec-path to the root-level of git's build
> directory, which the git binary will then stick at the front of the
> $PATH.
> 
> So the simplest solution really is: don't do that. The only debate in my
> mind is whether this is rare enough that it won't bite somebody again in
> the future, or if we should look into a solution that makes this Just
> Work.

That being said, it's easy enough to work around this one case, so we
don't have to stall the topic thinking about this (and maybe if it never
comes up again, we can just not think about it further :) ).

So here's a replacement patch for patch 4 of jk/upload-pack-hook (the
others are untouched, and don't even have conflicts rebasing on top).

I went with just setting $GIT_CONFIG_PARAMETERS. The full-path thing Duy
suggested would work, and you can avoid "which" by just pointing to
$TEST_DIRECTORY/helper/test-config. But besides being slightly brittle,
you also have to jump through some hoops to handle a $TEST_DIRECTORY
with spaces in it. The solution here is pretty straightforward, I think.

-- >8 --
Subject: config: return configset value for current_config_ functions

When 473166b (config: add 'origin_type' to config_source
struct, 2016-02-19) added accessor functions for the origin
type and name, it taught them only to look at the "cf"
struct that is filled in while we are parsing the config.
This is sufficient to make it work with git-config, which
uses git_config_with_options() under the hood. That function
freshly parses the config files and triggers the callback
when it parses each key.

Most git programs, however, use git_config(). This interface
will populate a cache during the actual parse, and then
serve values from the cache. Calling current_config_filename()
in a callback here will find a NULL cf and produce an error.
There are no such callers right now, but let's prepare for
adding some by making this work.

We already record source information in a struct attached to
each value. We just need to make it globally available and
then consult it from the accessor functions.

Signed-off-by: Jeff King 
---
 cache.h|  1 +
 config.c   | 51 +-
 t/helper/test-config.c | 20 
 t/t1308-config-set.sh  | 24 
 4 files changed, 87 insertions(+), 9 deletions(-)

diff --git a/cache.h b/cache.h
index 6049f86..1bce212 100644
--- a/cache.h
+++ b/cache.h
@@ -1696,6 +1696,7 @@ extern int ignore_untracked_cache_config;
 struct key_value_info {
const char *filename;
int linenr;
+   const char *origin_type;
 };
 
 extern NORETURN void git_die_config(const char *key, const char *err, ...) 
__attribute__((format(printf, 2, 3)));
diff --git a/config.c b/config.c
index 571151f..d555dee 100644
--- a/config.c
+++ b/config.c
@@ -38,7 +38,24 @@ struct config_source {
long (*do_ftell)(struct config_source *c);
 };
 
+/*
+ * These variables record the "current" config source, which
+ * can be accessed by parsing callbacks.
+ *
+ * The "cf" variable will be non-NULL only when we are actually parsing a real
+ * config source (file, blob, cmdline, etc).
+ *
+ * The "current_config_kvi" variable will be non-NULL only when we are feeding
+ * cached config from a configset into a callback.
+ *
+ * They should generally never be non-NULL at the same time. If they are both
+ * NULL, then we aren't parsing anything (and depending on the function looking
+ * at the variables, it's either a bug for it to be called in the first place,
+ * or it's a function which can be reused for non-config purposes, and should
+ * fall back to some sane behavior).
+ */
 static struct config_source *cf;
+static struct key_value_info *current_config_kvi;
 
 static int zlib_compression_seen;
 
@@ -1284,16 +1301,20 @@ static void configset_iter(struct config_set *cs, 
config_fn_t fn, void *data)
struct string_list *values;
struct config_set_element *entry;
struct configset_list *list = >list;
-   struct key_value_info *kv_info;
 
for (i = 0; i < list->nr; i++) {
entry = list->items[i].e;
value_index = list->items[i].value_index;
values = >value_list;
-   if (fn(entry->key, values->items[value_index].string, data) < 
0) {
-   kv_info = values->items[value_index].util;
-   git_die_config_linenr(entry->key, kv_info->filename, 
kv_info->linenr);
-   }
+
+   current_config_kvi = values->items[value_index].util;
+
+   if (fn(entry->key, 

Re: [PATCH 4/6] config: return configset value for current_config_ functions

[Junio C Hamano]

Jeff King  writes:

> The problem is running test-config inside of a git alias. The
> bin-wrappers will set the exec-path to the root-level of git's build
> directory, which the git binary will then stick at the front of the
> $PATH.

I was wondering why exec-path does not point at bin-wrappers in the
first place.

A wrapper script needs to know where the real thing lives in order
to "exec" (or "exec gdb") anyway, and it hardcodes the path to it.
It happens to use GIT_EXEC_PATH to shorten the hardcoded path it
uses when it does "exec", but it does not have to.

Wouldn't we want to see "git" use any of these wrapped ones when it
invokes a non-builtin subcommand when it does so normally, honoring
GIT_EXEC_PATH?  Pointing GIT_EXEC_PATH at the top-level means that
wrappers are bypassed for such an invocation (if what is run happens
to have executable at the top-level), and possibly a totally wrong
thing is run (when we start generating the binaries in different
directories, which is what we are seeing here).

> So the simplest solution really is: don't do that. The only debate
> in my mind is whether this is rare enough that it won't bite
> somebody again in the future, or if we should look into a solution
> that makes this Just Work.

I think it was you who alluded to revamping the test framework along
the lines of preparing a "test installation" (aka "make install"
with DESTDIR set to somewhere) and making bin-wrappers to point into
that installation (or if we are testing an installed Git that may be
different from what we have source for, that final installed
location).  An installed version of Git will not have test-* helpers
so they need to come from a freshly built source tree, not from
"test installation" or "installed Git".  There may be other details
that need to be worked out, but as a longer term direction that may
not be a bad idea.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] config: return configset value for current_config_ functions

[Jeff King]

On Thu, May 26, 2016 at 09:42:48AM -0700, Junio C Hamano wrote:

> Duy Nguyen  writes:
> 
> > On Thu, May 19, 2016 at 7:08 AM, Jeff King  wrote:
> >> On Wed, May 18, 2016 at 06:43:23PM -0400, Jeff King wrote:
> >>
> >>>  cache.h|  1 +
> >>>  config.c   | 51 
> >>> +-
> >>>  t/helper/test-config.c | 20 
> >>>  t/t1308-config-set.sh  | 23 +++
> >>> [...]
> >>> +test_expect_success 'iteration shows correct origins' '
> >>> + echo "[alias]test-config = !test-config" >.gitconfig &&
> >
> > How about using 'which' to get absolute path for test-config and put
> > it here? Then we don't rely on $PATH anymore.
> 
> Don't use which, which is not portable.
> 
> Remind me why we end up running ./test-config instead of
> ./bin-wrappers/test-config?  Should our tests be running 
> bin-wrappers early in their $PATH, perhaps?

The problem is running test-config inside of a git alias. The
bin-wrappers will set the exec-path to the root-level of git's build
directory, which the git binary will then stick at the front of the
$PATH.

So the simplest solution really is: don't do that. The only debate in my
mind is whether this is rare enough that it won't bite somebody again in
the future, or if we should look into a solution that makes this Just
Work.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] config: return configset value for current_config_ functions

[Junio C Hamano]

Duy Nguyen  writes:

> On Thu, May 19, 2016 at 7:08 AM, Jeff King  wrote:
>> On Wed, May 18, 2016 at 06:43:23PM -0400, Jeff King wrote:
>>
>>>  cache.h|  1 +
>>>  config.c   | 51 
>>> +-
>>>  t/helper/test-config.c | 20 
>>>  t/t1308-config-set.sh  | 23 +++
>>> [...]
>>> +test_expect_success 'iteration shows correct origins' '
>>> + echo "[alias]test-config = !test-config" >.gitconfig &&
>
> How about using 'which' to get absolute path for test-config and put
> it here? Then we don't rely on $PATH anymore.

Don't use which, which is not portable.

Remind me why we end up running ./test-config instead of
./bin-wrappers/test-config?  Should our tests be running 
bin-wrappers early in their $PATH, perhaps?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] config: return configset value for current_config_ functions

[Duy Nguyen]

On Thu, May 19, 2016 at 7:08 AM, Jeff King  wrote:
> On Wed, May 18, 2016 at 06:43:23PM -0400, Jeff King wrote:
>
>>  cache.h|  1 +
>>  config.c   | 51 
>> +-
>>  t/helper/test-config.c | 20 
>>  t/t1308-config-set.sh  | 23 +++
>> [...]
>> +test_expect_success 'iteration shows correct origins' '
>> + echo "[alias]test-config = !test-config" >.gitconfig &&

How about using 'which' to get absolute path for test-config and put
it here? Then we don't rely on $PATH anymore.

>> [...]
>> + git -c foo.bar=from-cmdline test-config iterate >actual &&
>
> While writing and testing this, I got bit by e6e7530 (test helpers: move
> test-* to t/helper/ subdirectory, 2016-04-13). I had an old test-config
> binary leftover in the root of my repository, and the new one was
> correctly built in t/helper/. Running "test-config" is fine, but inside
> the git alias, it sticks the repository root at the front of $PATH
> (because it's the exec-path). And so it ran the old version of
> test-config, which did not understand my new "iterate" option.
>
> Now I'll admit what I'm doing here is pretty funny (running test-* from
> an alias). I'm doing it because I want to see how the program operates
> with the "-c" config, and it's nicer to spell it as a user would,
> instead of munging $GIT_CONFIG_PARAMETERS directly.
>
> So I'm not sure if it's worth working around or not. The single tree
> state produced by this commit is fine, but it does behave badly if
> there's leftover cruft from a pre-e6e7530 build. A more robust version
> would look more like:
>
>   sq=\' ;# to ease quoting later
>   ...
>   GIT_CONFIG_PARAMETERS=${sq}foo.bar=from-cmdline${sq} test-config ...
>
> Which is ugly, but it's probably worth it to avoid the flakiness.
>
> The other option is to somehow make bin-wrappers more robust. E.g., it
> would be nice if we didn't actually point into the repository root
> directly, but rather somehow linked all of the git-* entries that
> _would_ be installed into the exec-path into a fake exec-path (or
> alternatively, actually build them directly into that fake exec-path).
>
> That's a much bigger change, though. Given how unlikely the sequence of
> steps in my test is, maybe it's better to just work around it in this
> one case.
>
> -Peff



-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] config: return configset value for current_config_ functions

[Jeff King]

On Wed, May 18, 2016 at 06:43:23PM -0400, Jeff King wrote:

>  cache.h|  1 +
>  config.c   | 51 
> +-
>  t/helper/test-config.c | 20 
>  t/t1308-config-set.sh  | 23 +++
> [...]
> +test_expect_success 'iteration shows correct origins' '
> + echo "[alias]test-config = !test-config" >.gitconfig &&
> [...]
> + git -c foo.bar=from-cmdline test-config iterate >actual &&

While writing and testing this, I got bit by e6e7530 (test helpers: move
test-* to t/helper/ subdirectory, 2016-04-13). I had an old test-config
binary leftover in the root of my repository, and the new one was
correctly built in t/helper/. Running "test-config" is fine, but inside
the git alias, it sticks the repository root at the front of $PATH
(because it's the exec-path). And so it ran the old version of
test-config, which did not understand my new "iterate" option.

Now I'll admit what I'm doing here is pretty funny (running test-* from
an alias). I'm doing it because I want to see how the program operates
with the "-c" config, and it's nicer to spell it as a user would,
instead of munging $GIT_CONFIG_PARAMETERS directly.

So I'm not sure if it's worth working around or not. The single tree
state produced by this commit is fine, but it does behave badly if
there's leftover cruft from a pre-e6e7530 build. A more robust version
would look more like:

  sq=\' ;# to ease quoting later
  ...
  GIT_CONFIG_PARAMETERS=${sq}foo.bar=from-cmdline${sq} test-config ...

Which is ugly, but it's probably worth it to avoid the flakiness.

The other option is to somehow make bin-wrappers more robust. E.g., it
would be nice if we didn't actually point into the repository root
directly, but rather somehow linked all of the git-* entries that
_would_ be installed into the exec-path into a fake exec-path (or
alternatively, actually build them directly into that fake exec-path).

That's a much bigger change, though. Given how unlikely the sequence of
steps in my test is, maybe it's better to just work around it in this
one case.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html