Re: [PATCH v2 4/6] config: don't implicitly use gitdir

[Brandon Williams]

On 06/14, Jeff King wrote:
> On Tue, Jun 13, 2017 at 02:38:15PM -0700, Brandon Williams wrote:
> 
> > > The same comments as before still apply:
> > > 
> > > - this changes API to make opts->git_dir mandatory, which is error prone
> > >   and easily avoidable, e.g. by making git_dir an argument to
> > >   git_config_with_options
> > 
> > I still don't agree with this.  I have looked at all callers and ensured
> > that 'git_dir' will be set when appropriate in the 'config_options'
> > struct.  I find the notion ridiculous that I would need to change a
> > function's name or arguments every time the internals of the function
> > are adjusted or when an options struct obtains a new field.  Plus, there
> > is already an aptly named parameter of type 'config_options' with which
> > to hold options for the config machinery.  This struct is also added to
> > in a later patch to include commondir so that the gitdir vs commondir
> > issue can be resolved.
> 
> I've already said "I'm OK either way for this case", but let me clarify
> a bit.
> 
> It's not about changing a function's internals, or even the struct
> obtaining a new field. The key change here is that the _interface_
> changed. Callers used to be able to pass NULL for the git_dir and have
> the function behave one way, and now if they do so it behaves
> differently. That leads to spooky action at a distance. Code which you
> didn't know about still compiles but does something subtly different.
> 
> We can catch that at the compile stage, or we can catch it at the test
> stage, or we can decide it's somebody else's problem to deal with if
> they wrote code that the rest of the project hasn't seen.
> 
> But it is a real thing that comes up in a big, open project. There is no
> "looked at all the callers", because you can't see the whole universe of
> code. I do think it's a much bigger deal in a project like the kernel,
> which has hundreds of long-lasting forks. Git has only a handful, and we
> don't necessarily need to bend over backwards for people whose code
> hasn't been shared.

At this point I'm done arguing, I'll rename the function and drop the
'git' prefix as that's the only sensible name I can think of.

> 
> > > - the commit message doesn't say anything about to git dir vs common dir
> > >   change.  It needs to, or even better, the switch to use common dir
> > >   instead of git dir can happen as a separate patch.
> > 
> > There really isn't any switching in this patch.  One of the following
> > patches in this series addresses this problem in more detail though.
> 
> I would have expected that patch to actually come earlier. That fixes
> the bug there, and then this conversion patch becomes that much more
> straightforward.

True, I can reorder the patches in v3 so that this change flows better.

-- 
Brandon Williams

Re: [PATCH v2 4/6] config: don't implicitly use gitdir

[Jeff King]

On Tue, Jun 13, 2017 at 02:38:15PM -0700, Brandon Williams wrote:

> > The same comments as before still apply:
> > 
> > - this changes API to make opts->git_dir mandatory, which is error prone
> >   and easily avoidable, e.g. by making git_dir an argument to
> >   git_config_with_options
> 
> I still don't agree with this.  I have looked at all callers and ensured
> that 'git_dir' will be set when appropriate in the 'config_options'
> struct.  I find the notion ridiculous that I would need to change a
> function's name or arguments every time the internals of the function
> are adjusted or when an options struct obtains a new field.  Plus, there
> is already an aptly named parameter of type 'config_options' with which
> to hold options for the config machinery.  This struct is also added to
> in a later patch to include commondir so that the gitdir vs commondir
> issue can be resolved.

I've already said "I'm OK either way for this case", but let me clarify
a bit.

It's not about changing a function's internals, or even the struct
obtaining a new field. The key change here is that the _interface_
changed. Callers used to be able to pass NULL for the git_dir and have
the function behave one way, and now if they do so it behaves
differently. That leads to spooky action at a distance. Code which you
didn't know about still compiles but does something subtly different.

We can catch that at the compile stage, or we can catch it at the test
stage, or we can decide it's somebody else's problem to deal with if
they wrote code that the rest of the project hasn't seen.

But it is a real thing that comes up in a big, open project. There is no
"looked at all the callers", because you can't see the whole universe of
code. I do think it's a much bigger deal in a project like the kernel,
which has hundreds of long-lasting forks. Git has only a handful, and we
don't necessarily need to bend over backwards for people whose code
hasn't been shared.

> > - the commit message doesn't say anything about to git dir vs common dir
> >   change.  It needs to, or even better, the switch to use common dir
> >   instead of git dir can happen as a separate patch.
> 
> There really isn't any switching in this patch.  One of the following
> patches in this series addresses this problem in more detail though.

I would have expected that patch to actually come earlier. That fixes
the bug there, and then this conversion patch becomes that much more
straightforward.

-Peff

Re: [PATCH v2 4/6] config: don't implicitly use gitdir

[Jacob Keller]

On Tue, Jun 13, 2017 at 3:05 PM, Jonathan Nieder  wrote:
> Junio C Hamano wrote:
>> On Tue, Jun 13, 2017 at 2:51 PM, Jonathan Nieder  wrote:
>
>>> What is the next step, then?  You can find the notion ridiculous but
>>> it's how this project has worked in my experience (and how other
>>> projects with similar patch-based workflows work).
>>
>> Does "patch-based" have much to do with this? I agree that distributed
>> nature of the development would bring this issue, but I tend to think that
>> using merge/pull based workflow would not alleviate it--am I mistaken?
>
> Thanks, you're right.  Distributed is the relevant feature.
>
> The same issue can even come up when using a centralized version
> control system like Subversion or Perforce --- without attention to
> API compatibility, someone's change that was thoroughly reviewed and
> well tested locally in a developer's working directory can introduce
> subtle breakage once they run "svn commit", causing it to merge with
> the latest upstream changes.  The problem becomes more likely the more
> distributed a project is since each developer becomes less aware of
> the other changes that their modifications need to be compatible with.
>
> Jonathan

Which would be why early integration (pu) and a good test suite are
for. However, it's much easier to catch if you change the name when
the function no longer behaves the same. In this case I guess you
could argue that the part that changed wasn't part of the API...
However, I think I'd lean towards "we had code depending on it, so yes
it was".

It's ok to add new functionality only obtained by new flags or
similar,  but not ok to break potentially existing callers.

Thanks,
Jake

Re: [PATCH v2 4/6] config: don't implicitly use gitdir

[Jonathan Nieder]

Junio C Hamano wrote:
> On Tue, Jun 13, 2017 at 2:51 PM, Jonathan Nieder  wrote:

>> What is the next step, then?  You can find the notion ridiculous but
>> it's how this project has worked in my experience (and how other
>> projects with similar patch-based workflows work).
>
> Does "patch-based" have much to do with this? I agree that distributed
> nature of the development would bring this issue, but I tend to think that
> using merge/pull based workflow would not alleviate it--am I mistaken?

Thanks, you're right.  Distributed is the relevant feature.

The same issue can even come up when using a centralized version
control system like Subversion or Perforce --- without attention to
API compatibility, someone's change that was thoroughly reviewed and
well tested locally in a developer's working directory can introduce
subtle breakage once they run "svn commit", causing it to merge with
the latest upstream changes.  The problem becomes more likely the more
distributed a project is since each developer becomes less aware of
the other changes that their modifications need to be compatible with.

Jonathan

Re: [PATCH v2 4/6] config: don't implicitly use gitdir

[Junio C Hamano]

On Tue, Jun 13, 2017 at 2:51 PM, Jonathan Nieder  wrote:
>
> What is the next step, then?  You can find the notion ridiculous but
> it's how this project has worked in my experience (and how other
> projects with similar patch-based workflows work).

Does "patch-based" have much to do with this? I agree that distributed
nature of the development would bring this issue, but I tend to think that
using merge/pull based workflow would not alleviate it--am I mistaken?

Re: [PATCH v2 4/6] config: don't implicitly use gitdir

[Jonathan Nieder]

Brandon Williams wrote:
> On 06/13, Jonathan Nieder wrote:
>> Brandon Williams wrote:

>>> Commit 2185fde56 (config: handle conditional include when $GIT_DIR is
>>> not set up) added a 'git_dir' field to the config_options struct.  Let's
>>> use this option field explicitly all the time instead of occasionally
>>> falling back to calling 'git_pathdup("config")' to get the path to the
>>> local repository configuration.  This allows 'do_git_config_sequence()'
>>> to not implicitly rely on global repository state.
>>>
>>> Signed-off-by: Brandon Williams 
>>> ---
>>>  builtin/config.c | 2 ++
>>>  config.c | 6 ++
>>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> The same comments as before still apply:
>>
>> - this changes API to make opts->git_dir mandatory, which is error prone
>>   and easily avoidable, e.g. by making git_dir an argument to
>>   git_config_with_options
>
> I still don't agree with this.  I have looked at all callers and ensured
> that 'git_dir' will be set when appropriate in the 'config_options'
> struct.  I find the notion ridiculous that I would need to change a
> function's name or arguments every time the internals of the function
> are adjusted or when an options struct obtains a new field.  Plus, there
> is already an aptly named parameter of type 'config_options' with which
> to hold options for the config machinery.  This struct is also added to
> in a later patch to include commondir so that the gitdir vs commondir
> issue can be resolved.

What is the next step, then?  You can find the notion ridiculous but
it's how this project has worked in my experience (and how other
projects with similar patch-based workflows work).

I also don't really understand why it is so bad to have to care about
API compatibility when it is so simple to do --- change the function
signature or change the function name.  That's all it takes.

>> - the commit message doesn't say anything about to git dir vs common dir
>>   change.  It needs to, or even better, the switch to use common dir
>>   instead of git dir can happen as a separate patch.
>
> There really isn't any switching in this patch.  One of the following
> patches in this series addresses this problem in more detail though.

There is, because of the gitdir: behavior change.

Jonathan

Re: [PATCH v2 4/6] config: don't implicitly use gitdir

[Brandon Williams]

On 06/13, Jonathan Nieder wrote:
> Brandon Williams wrote:
> 
> > Commit 2185fde56 (config: handle conditional include when $GIT_DIR is
> > not set up) added a 'git_dir' field to the config_options struct.  Let's
> > use this option field explicitly all the time instead of occasionally
> > falling back to calling 'git_pathdup("config")' to get the path to the
> > local repository configuration.  This allows 'do_git_config_sequence()'
> > to not implicitly rely on global repository state.
> >
> > Signed-off-by: Brandon Williams 
> > ---
> >  builtin/config.c | 2 ++
> >  config.c | 6 ++
> >  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> The same comments as before still apply:
> 
> - this changes API to make opts->git_dir mandatory, which is error prone
>   and easily avoidable, e.g. by making git_dir an argument to
>   git_config_with_options

I still don't agree with this.  I have looked at all callers and ensured
that 'git_dir' will be set when appropriate in the 'config_options'
struct.  I find the notion ridiculous that I would need to change a
function's name or arguments every time the internals of the function
are adjusted or when an options struct obtains a new field.  Plus, there
is already an aptly named parameter of type 'config_options' with which
to hold options for the config machinery.  This struct is also added to
in a later patch to include commondir so that the gitdir vs commondir
issue can be resolved.

> - the commit message doesn't say anything about to git dir vs common dir
>   change.  It needs to, or even better, the switch to use common dir
>   instead of git dir can happen as a separate patch.

There really isn't any switching in this patch.  One of the following
patches in this series addresses this problem in more detail though.

-- 
Brandon Williams

Re: [PATCH v2 4/6] config: don't implicitly use gitdir

[Jonathan Nieder]

Brandon Williams wrote:

> Commit 2185fde56 (config: handle conditional include when $GIT_DIR is
> not set up) added a 'git_dir' field to the config_options struct.  Let's
> use this option field explicitly all the time instead of occasionally
> falling back to calling 'git_pathdup("config")' to get the path to the
> local repository configuration.  This allows 'do_git_config_sequence()'
> to not implicitly rely on global repository state.
>
> Signed-off-by: Brandon Williams 
> ---
>  builtin/config.c | 2 ++
>  config.c | 6 ++
>  2 files changed, 4 insertions(+), 4 deletions(-)

The same comments as before still apply:

- this changes API to make opts->git_dir mandatory, which is error prone
  and easily avoidable, e.g. by making git_dir an argument to
  git_config_with_options

- the commit message doesn't say anything about to git dir vs common dir
  change.  It needs to, or even better, the switch to use common dir
  instead of git dir can happen as a separate patch.

Thanks,
Jonathan

[PATCH v2 4/6] config: don't implicitly use gitdir

[Brandon Williams]

Commit 2185fde56 (config: handle conditional include when $GIT_DIR is
not set up) added a 'git_dir' field to the config_options struct.  Let's
use this option field explicitly all the time instead of occasionally
falling back to calling 'git_pathdup("config")' to get the path to the
local repository configuration.  This allows 'do_git_config_sequence()'
to not implicitly rely on global repository state.

Signed-off-by: Brandon Williams 
---
 builtin/config.c | 2 ++
 config.c | 6 ++
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 753c40a5c..f06a8dff2 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -539,6 +539,8 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
config_options.respect_includes = !given_config_source.file;
else
config_options.respect_includes = respect_includes_opt;
+   if (!nongit)
+   config_options.git_dir = get_git_common_dir();
 
if (end_null) {
term = '\0';
diff --git a/config.c b/config.c
index 2390f98e3..4e2842689 100644
--- a/config.c
+++ b/config.c
@@ -219,8 +219,6 @@ static int include_by_gitdir(const struct config_options 
*opts,
 
if (opts->git_dir)
git_dir = opts->git_dir;
-   else if (have_git_dir())
-   git_dir = get_git_dir();
else
goto done;
 
@@ -1548,8 +1546,6 @@ static int do_git_config_sequence(const struct 
config_options *opts,
 
if (opts->git_dir)
repo_config = mkpathdup("%s/config", opts->git_dir);
-   else if (have_git_dir())
-   repo_config = git_pathdup("config");
else
repo_config = NULL;
 
@@ -1613,6 +1609,8 @@ static void git_config_raw(config_fn_t fn, void *data)
struct config_options opts = {0};
 
opts.respect_includes = 1;
+   if (have_git_dir())
+   opts.git_dir = get_git_common_dir();
if (git_config_with_options(fn, data, NULL, ) < 0)
/*
 * git_config_with_options() normally returns only
-- 
2.13.1.518.g3df882009-goog