Re: [PATCH] commit: configure submodules

2012-09-24 Thread Junio C Hamano
Junio C Hamano gits...@pobox.com writes:

 Jens Lehmann jens.lehm...@web.de writes:

 Jens, what do you think?  I see no reason for anybody other than
 submodule init to call gitmodules_config() that reads from the
 in-tree .gitmodules file.

 I think the copying on init is not what we should do here because
 it sets the user's customization to what ever happened to be in
 .gitmodules at the time he initialized the submodule.

 Hrm, why does the user have submodule.$name.$whatever customized
 before saying submodule init $whategver for that copying to be
 problematic?

 So I think Orgad's change is sane and should go in.

 Matching what cmd_commit() does to what cmd_status() does, i.e. grab
 submodule.$name.ignore from somewhere, is not something I questioned.
 The patch is a good change to make them consistent.

 What I was wondering was if that is a consistently wrong thing to do
 to read from .gitmodules not $GIT_DIR/config.

 In any case, the log message I suggested in the review needs to be
 updated in the reroll to make it clear that this is about reading
 from .gitmodules, not configuration.  AFAICS, gitmodule_config()
 does not even read from $GIT_DIR/config, right?

OK.  gitmodule_config() does not read $GIT_DIR/config, but
cmd_status() and cmd_commit() call git_diff_basic_config() that is
called from git_diff_ui_config() to read submodule.$name.ignore from
it.  So Orgad's patch is _only_ about submodule.$name.ignore that is
in in-tree .gitmodule; the log message shouldn't mention config,
as setting configuration variables work for both status and commit
just fine.

--
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] commit: configure submodules

2012-09-24 Thread Jens Lehmann
Am 24.09.2012 18:27, schrieb Junio C Hamano:
 Junio C Hamano gits...@pobox.com writes:
 In any case, the log message I suggested in the review needs to be
 updated in the reroll to make it clear that this is about reading
 from .gitmodules, not configuration.  AFAICS, gitmodule_config()
 does not even read from $GIT_DIR/config, right?
 
 OK.  gitmodule_config() does not read $GIT_DIR/config, but
 cmd_status() and cmd_commit() call git_diff_basic_config() that is
 called from git_diff_ui_config() to read submodule.$name.ignore from
 it.  So Orgad's patch is _only_ about submodule.$name.ignore that is
 in in-tree .gitmodule; the log message shouldn't mention config,
 as setting configuration variables work for both status and commit
 just fine.

Yes, I was just checking that call path too to make sure the user
settings from $GIT_DIR/config really override those found in
.gitmodules. And of course you are right, while the change to the
code is sane the commit message still needs some work.
--
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] commit: configure submodules

2012-09-24 Thread Orgad Shaneh
On Mon, Sep 24, 2012 at 8:34 PM, Junio C Hamano gits...@pobox.com wrote:

 Jens Lehmann jens.lehm...@web.de writes:

  Am 24.09.2012 18:27, schrieb Junio C Hamano:
  Junio C Hamano gits...@pobox.com writes:
  In any case, the log message I suggested in the review needs to be
  updated in the reroll to make it clear that this is about reading
  from .gitmodules, not configuration.  AFAICS, gitmodule_config()
  does not even read from $GIT_DIR/config, right?
 
  OK.  gitmodule_config() does not read $GIT_DIR/config, but
  cmd_status() and cmd_commit() call git_diff_basic_config() that is
  called from git_diff_ui_config() to read submodule.$name.ignore from
  it.  So Orgad's patch is _only_ about submodule.$name.ignore that is
  in in-tree .gitmodule; the log message shouldn't mention config,
  as setting configuration variables work for both status and commit
  just fine.
 
  Yes, I was just checking that call path too to make sure the user
  settings from $GIT_DIR/config really override those found in
  .gitmodules. And of course you are right, while the change to the
  code is sane the commit message still needs some work.

 Here is what I tentatively queued on 'pu' (not pushed out yet).

 -- 8 --
 From: Orgad Shaneh org...@gmail.com
 Date: Sun, 23 Sep 2012 09:37:47 +0200
 Subject: [PATCH] commit: pay attention to submodule.$name.ignore in
  .gitmodules

 git status does not list a submodule with uncommitted working tree
 files as modified when submodule.$name.ignore is set to dirty in
 in-tree .gitmodules file.  Both status and commit honor the setting
 in $GIT_DIR/config, but commit does not pick it up from .gitmodules,
 which is inconsistent.

 Teach git commit to pay attention to the setting in .gitmodules as
 well.

 Signed-off-by: Orgad Shaneh org...@gmail.com
 Signed-off-by: Junio C Hamano gits...@pobox.com
 ---
  builtin/commit.c | 1 +
  1 file changed, 1 insertion(+)

 diff --git a/builtin/commit.c b/builtin/commit.c
 index 66fdd22..3cb1ef7 100644
 --- a/builtin/commit.c
 +++ b/builtin/commit.c
 @@ -1256,6 +1256,7 @@ int cmd_commit(int argc, const char **argv, const char 
 *prefix)
 struct wt_status s;

 wt_status_prepare(s);
 +   gitmodules_config();
 git_config(git_commit_config, s);
 in_merge = file_exists(git_path(MERGE_HEAD));
 s.in_merge = in_merge;
 --
 1.7.12.1.441.g794a63b


That is not correct. git-config is ignored as well for commit. Maybe
another fix is needed?

- Orgad
--
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] commit: configure submodules

2012-09-24 Thread Junio C Hamano
Orgad Shaneh org...@gmail.com writes:

 That is not correct. git-config is ignored as well for commit.

What do you mean?  As far as I can tell, if you have

[submodule var]
path = var
ignore = dirty

in $GIT_DIR/config, a work-tree-dirty submodule var is not
reported by git status and git commit without your patch, and
your patch does not seem to break that.  The only difference your
patch makes is that if you had the above three-line block in
the .gitmodules file and not in $GIT_DIR/config, git status
ignored the dirtyness in the working tree, but git commit did
notice and report it.

What am I missing?


--
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] commit: configure submodules

2012-09-24 Thread Orgad Shaneh
On Mon, Sep 24, 2012 at 9:06 PM, Junio C Hamano gits...@pobox.com wrote:
 Orgad Shaneh org...@gmail.com writes:

 That is not correct. git-config is ignored as well for commit.

 What do you mean?  As far as I can tell, if you have

 [submodule var]
 path = var
 ignore = dirty

 in $GIT_DIR/config, a work-tree-dirty submodule var is not
 reported by git status and git commit without your patch, and
 your patch does not seem to break that.  The only difference your
 patch makes is that if you had the above three-line block in
 the .gitmodules file and not in $GIT_DIR/config, git status
 ignored the dirtyness in the working tree, but git commit did
 notice and report it.

 What am I missing?



I have:
[submodule mod]
url = [...]
ignore = dirty

in .git/config, and I removed the ignore part from .gitmodules to be even.

I made a change inside mod, git status doesn't report its dirtiness,
while git commit does.

git status:
# On branch master
# Changes to be committed:
#   (use git reset HEAD file... to unstage)
#
#   modified:   foo
#
# Changes not staged for commit:
#   (use git add file... to update what will be committed)
#   (use git checkout -- file... to discard changes in working directory)
#
#   modified:   .gitmodules
#

git commit:
# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
# On branch master
# Changes to be committed:
#   (use git reset HEAD file... to unstage)
#
#   modified:   foo
#
# Changes not staged for commit:
#   (use git add file... to update what will be committed)
#   (use git checkout -- file... to discard changes in working directory)
#   (commit or discard the untracked or modified content in submodules)
#
#   modified:   .gitmodules
#   modified:   mod (modified content)
#

Now I get it! That's because I don't have submodule.mod.path!
config_name_for_path only gets initialized if path exists. Apparently
git submodule init doesn't configure 'path', so it stays
uninitialized.

- Orgad
--
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] commit: configure submodules

2012-09-24 Thread Jens Lehmann
Am 24.09.2012 21:16, schrieb Orgad Shaneh:
 On Mon, Sep 24, 2012 at 9:06 PM, Junio C Hamano gits...@pobox.com wrote:
 Orgad Shaneh org...@gmail.com writes:

 That is not correct. git-config is ignored as well for commit.

 What do you mean?  As far as I can tell, if you have

 [submodule var]
 path = var
 ignore = dirty

 in $GIT_DIR/config, a work-tree-dirty submodule var is not
 reported by git status and git commit without your patch, and
 your patch does not seem to break that.  The only difference your
 patch makes is that if you had the above three-line block in
 the .gitmodules file and not in $GIT_DIR/config, git status
 ignored the dirtyness in the working tree, but git commit did
 notice and report it.

 What am I missing?


 
 I have:
 [submodule mod]
 url = [...]
 ignore = dirty
 
 in .git/config, and I removed the ignore part from .gitmodules to be even.
 
 I made a change inside mod, git status doesn't report its dirtiness,
 while git commit does.
 
 git status:
 # On branch master
 # Changes to be committed:
 #   (use git reset HEAD file... to unstage)
 #
 #   modified:   foo
 #
 # Changes not staged for commit:
 #   (use git add file... to update what will be committed)
 #   (use git checkout -- file... to discard changes in working directory)
 #
 #   modified:   .gitmodules
 #
 
 git commit:
 # Please enter the commit message for your changes. Lines starting
 # with '#' will be ignored, and an empty message aborts the commit.
 # On branch master
 # Changes to be committed:
 #   (use git reset HEAD file... to unstage)
 #
 #   modified:   foo
 #
 # Changes not staged for commit:
 #   (use git add file... to update what will be committed)
 #   (use git checkout -- file... to discard changes in working directory)
 #   (commit or discard the untracked or modified content in submodules)
 #
 #   modified:   .gitmodules
 #   modified:   mod (modified content)
 #
 
 Now I get it! That's because I don't have submodule.mod.path!
 config_name_for_path only gets initialized if path exists. Apparently
 git submodule init doesn't configure 'path', so it stays
 uninitialized.

But submodule.mod.path should only be set in .gitmodules, not in
$GIT_DIR/config. Did you just remove the ignore setting from
.gitmodules or the path too?
--
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] commit: configure submodules

2012-09-24 Thread Orgad Shaneh
On Mon, Sep 24, 2012 at 9:56 PM, Jens Lehmann jens.lehm...@web.de wrote:
 Am 24.09.2012 21:16, schrieb Orgad Shaneh:
 On Mon, Sep 24, 2012 at 9:06 PM, Junio C Hamano gits...@pobox.com wrote:
 Orgad Shaneh org...@gmail.com writes:

 That is not correct. git-config is ignored as well for commit.

 What do you mean?  As far as I can tell, if you have

 [submodule var]
 path = var
 ignore = dirty

 in $GIT_DIR/config, a work-tree-dirty submodule var is not
 reported by git status and git commit without your patch, and
 your patch does not seem to break that.  The only difference your
 patch makes is that if you had the above three-line block in
 the .gitmodules file and not in $GIT_DIR/config, git status
 ignored the dirtyness in the working tree, but git commit did
 notice and report it.

 What am I missing?



 I have:
 [submodule mod]
 url = [...]
 ignore = dirty

 in .git/config, and I removed the ignore part from .gitmodules to be even.

 I made a change inside mod, git status doesn't report its dirtiness,
 while git commit does.

 git status:
 # On branch master
 # Changes to be committed:
 #   (use git reset HEAD file... to unstage)
 #
 #   modified:   foo
 #
 # Changes not staged for commit:
 #   (use git add file... to update what will be committed)
 #   (use git checkout -- file... to discard changes in working directory)
 #
 #   modified:   .gitmodules
 #

 git commit:
 # Please enter the commit message for your changes. Lines starting
 # with '#' will be ignored, and an empty message aborts the commit.
 # On branch master
 # Changes to be committed:
 #   (use git reset HEAD file... to unstage)
 #
 #   modified:   foo
 #
 # Changes not staged for commit:
 #   (use git add file... to update what will be committed)
 #   (use git checkout -- file... to discard changes in working directory)
 #   (commit or discard the untracked or modified content in submodules)
 #
 #   modified:   .gitmodules
 #   modified:   mod (modified content)
 #

 Now I get it! That's because I don't have submodule.mod.path!
 config_name_for_path only gets initialized if path exists. Apparently
 git submodule init doesn't configure 'path', so it stays
 uninitialized.

 But submodule.mod.path should only be set in .gitmodules, not in
 $GIT_DIR/config. Did you just remove the ignore setting from
 .gitmodules or the path too?

Just the ignore, and my patch of course.

If it is not set in $GIT_DIR/config, then config_name_for_path is not
initialized, and if it is not initialized, then
set_diffopt_flags_from_submodule_config does nothing
(handle_ignore_submodules_arg is not called). That is the main
problem.

- Orgad
--
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] commit: configure submodules

2012-09-24 Thread Jens Lehmann
Am 24.09.2012 21:59, schrieb Orgad Shaneh:
 On Mon, Sep 24, 2012 at 9:56 PM, Jens Lehmann jens.lehm...@web.de wrote:
 Am 24.09.2012 21:16, schrieb Orgad Shaneh:
 On Mon, Sep 24, 2012 at 9:06 PM, Junio C Hamano gits...@pobox.com wrote:
 Orgad Shaneh org...@gmail.com writes:

 That is not correct. git-config is ignored as well for commit.

 What do you mean?  As far as I can tell, if you have

 [submodule var]
 path = var
 ignore = dirty

 in $GIT_DIR/config, a work-tree-dirty submodule var is not
 reported by git status and git commit without your patch, and
 your patch does not seem to break that.  The only difference your
 patch makes is that if you had the above three-line block in
 the .gitmodules file and not in $GIT_DIR/config, git status
 ignored the dirtyness in the working tree, but git commit did
 notice and report it.

 What am I missing?



 I have:
 [submodule mod]
 url = [...]
 ignore = dirty

 in .git/config, and I removed the ignore part from .gitmodules to be even.

 I made a change inside mod, git status doesn't report its dirtiness,
 while git commit does.

 git status:
 # On branch master
 # Changes to be committed:
 #   (use git reset HEAD file... to unstage)
 #
 #   modified:   foo
 #
 # Changes not staged for commit:
 #   (use git add file... to update what will be committed)
 #   (use git checkout -- file... to discard changes in working 
 directory)
 #
 #   modified:   .gitmodules
 #

 git commit:
 # Please enter the commit message for your changes. Lines starting
 # with '#' will be ignored, and an empty message aborts the commit.
 # On branch master
 # Changes to be committed:
 #   (use git reset HEAD file... to unstage)
 #
 #   modified:   foo
 #
 # Changes not staged for commit:
 #   (use git add file... to update what will be committed)
 #   (use git checkout -- file... to discard changes in working 
 directory)
 #   (commit or discard the untracked or modified content in submodules)
 #
 #   modified:   .gitmodules
 #   modified:   mod (modified content)
 #

 Now I get it! That's because I don't have submodule.mod.path!
 config_name_for_path only gets initialized if path exists. Apparently
 git submodule init doesn't configure 'path', so it stays
 uninitialized.

 But submodule.mod.path should only be set in .gitmodules, not in
 $GIT_DIR/config. Did you just remove the ignore setting from
 .gitmodules or the path too?
 
 Just the ignore, and my patch of course.
 
 If it is not set in $GIT_DIR/config, then config_name_for_path is not
 initialized, and if it is not initialized, then
 set_diffopt_flags_from_submodule_config does nothing
 (handle_ignore_submodules_arg is not called). That is the main
 problem.

But config_name_for_path can only be set via .gitmodules. It is set in
parse_submodule_config_option() called by submodule_config() which is
called from gitmodules_config() ... but only if .gitmodules doesn't
have a merge conflict.

So either your .gitmodules has a merge conflict or the logic setting
gitmodules_is_unmerged in gitmodules_config() is buggy.
--
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] commit: configure submodules

2012-09-24 Thread Junio C Hamano
Jens Lehmann jens.lehm...@web.de writes:

 But submodule.mod.path should only be set in .gitmodules, not in
 $GIT_DIR/config. Did you just remove the ignore setting from
 .gitmodules or the path too?

Without that in $GIT_DIR/config, how would path-name mapping
correctly work???

Confused...
--
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] commit: configure submodules

2012-09-23 Thread Junio C Hamano
Orgad Shaneh org...@gmail.com writes:

 As reported on the mailing list[1], ignore submodule config is not
 respected on commit.

 [1] 
 http://git.661346.n2.nabble.com/submodule-ignore-is-not-respected-on-commit-td7539238.html

 Signed-off-by: Orgad Shaneh org...@gmail.com
 ---

Thanks.

Please do not force people to go to external website like that while
reading the git log output.  You could have just said

git status does not list a submodule with uncommitted
working tree files as modified when submodule.$name.ignore
configuration is set to dirty, but git commit forgets
to take this configuration variable into account.

or something.  It would also be good to say I reported it earlier
in this message with the URL _after_ the three-dash line.

I see Jens added with 302ad7a (Submodules: Use ignore settings
from .gitmodules too for diff and status, 2010-08-06) the call to
gitmodules_config() to git status and git diff family, but I
suspect that was a huge mistake.  Once a submodule is initialized
with submodule init, the default set of configuration should be
copied to the user's $GIT_DIR/config and subsequent run-time
invocation should read $GIT_DIR/config and $GIT_DIR/config alone, to
honor user's customization.

Instead, I think git_commit_config() and git_status_config() should
call submodule_config() function to read submodule.$name.ignore not
from .gitmodules file but from $GIT_DIR/config.

Jens, what do you think?  I see no reason for anybody other than
submodule init to call gitmodules_config() that reads from the
in-tree .gitmodules file.

  builtin/commit.c |1 +
  1 file changed, 1 insertion(+)

 diff --git a/builtin/commit.c b/builtin/commit.c
 index 62028e7..7a83cae 100644
 --- a/builtin/commit.c
 +++ b/builtin/commit.c
 @@ -1452,6 +1452,7 @@ int cmd_commit(int argc, const char **argv, const char 
 *prefix)
   usage_with_options(builtin_commit_usage, 
 builtin_commit_options);
  
   wt_status_prepare(s);
 + gitmodules_config();
   git_config(git_commit_config, s);
   determine_whence(s);
   s.colopts = 0;
--
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] commit: configure submodules

2012-09-23 Thread Jens Lehmann
Am 23.09.2012 10:37, schrieb Junio C Hamano:
 I see Jens added with 302ad7a (Submodules: Use ignore settings
 from .gitmodules too for diff and status, 2010-08-06) the call to
 gitmodules_config() to git status and git diff family, but I
 suspect that was a huge mistake.  Once a submodule is initialized
 with submodule init, the default set of configuration should be
 copied to the user's $GIT_DIR/config and subsequent run-time
 invocation should read $GIT_DIR/config and $GIT_DIR/config alone, to
 honor user's customization.

Not honoring the user's customization would be a big mistake, but
this is not what happens here. A setting in $GIT_DIR/config always
overrides the one in .gitmodules (that's why gitmodules_config() is
called before git_config()).

 Instead, I think git_commit_config() and git_status_config() should
 call submodule_config() function to read submodule.$name.ignore not
 from .gitmodules file but from $GIT_DIR/config.
 
 Jens, what do you think?  I see no reason for anybody other than
 submodule init to call gitmodules_config() that reads from the
 in-tree .gitmodules file.

I think the copying on init is not what we should do here because
it sets the user's customization to what ever happened to be in
.gitmodules at the time he initialized the submodule. Later changes
from upstream to such a setting would not be honored unless the
user copies that new setting herself (which I think is The Right
Thing for the URL, but not for the other work tree related settings
like 'ignore').

Imagine you have a submodule containing a huge media file which is
set to be ignored for performance reasons. When upstream later
decides it should rather use .gitattributes to just disable diffing
that file and removes the submodule ignore so the users see changes
to other files of the submodule again, that will just work the way
it is done now, but won't when we copy that setting on init.

So it is either honor upstream unless the user decides otherwise
or take what upstream configured at init time as the users choice
(until he actively changes it). And I think the former is more
flexible as it allows upstream to change settings without user
intervention, which is why I did it that way.

And as I understand that .gitattributes follow the same principle:
Unless the user configured something different in his
$GIT_DIR/info/attributes file, git will use the settings from the
.gitattributes file of the currently checked out commit.

So I think Orgad's change is sane and should go in.
--
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] commit: configure submodules

2012-09-23 Thread Junio C Hamano
Jens Lehmann jens.lehm...@web.de writes:

 Jens, what do you think?  I see no reason for anybody other than
 submodule init to call gitmodules_config() that reads from the
 in-tree .gitmodules file.

 I think the copying on init is not what we should do here because
 it sets the user's customization to what ever happened to be in
 .gitmodules at the time he initialized the submodule.

Hrm, why does the user have submodule.$name.$whatever customized
before saying submodule init $whategver for that copying to be
problematic?

 So I think Orgad's change is sane and should go in.

Matching what cmd_commit() does to what cmd_status() does, i.e. grab
submodule.$name.ignore from somewhere, is not something I questioned.
The patch is a good change to make them consistent.

What I was wondering was if that is a consistently wrong thing to do
to read from .gitmodules not $GIT_DIR/config.

In any case, the log message I suggested in the review needs to be
updated in the reroll to make it clear that this is about reading
from .gitmodules, not configuration.  AFAICS, gitmodule_config()
does not even read from $GIT_DIR/config, right?
--
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