patch, 2 partssubmodule: fix confusing variable name

There are configuration options for each submodule that specify under what circumstances git status should display output for that submodule. Unfortunately, these settings were not being respected, and as such the tests were marked TODO.

This patch series consists of two patches: the first is a fix for a confusing variable name, and the latter actually makes git status not output the submodule information. The tests have been updated accordingly.

 git-submodule.sh  | 10 ++++++----
 t/t7508-status.sh |  4 ++--
 2 files changed, 8 insertions(+), 6 deletions(-)

cmd_summary reads the output of git diff, but reads in the submodule path into a variable called name. Since this variable does not contain the name of the submodule, but the path, rename it to be clearer what data it actually holds.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 git-submodule.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

brian m. carlson wrote:

Nice.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Am 03.08.2013 20:14, schrieb Jonathan Nieder:

I totally agree. Paths and names of submodules are often confused (especially as they are identical for a submodule that was never moved), so cleaning up our sources so they use the correct term is The Right Thing.

But we'll have to use sm_path here (like everywhere else in the submodule script), because we'll run into problems under Windows otherwise (see 64394e3ae9 for details). Apart from that the patch is fine.

On Sun, Aug 04, 2013 at 07:34:48PM +0200, Jens Lehmann wrote:

We're still using path= in the foreach-script. Or rather, we're setting it. From what I can see and from the commit message 64394e3ae9 it could possible be a problem.

Not sure how to solve it though... Just a simple correction would break all script depending on that.

Am 04.08.2013 23:29, schrieb Fredrik Gustafsson:

The commit message of 64394e3ae9 explicitly talks about that one:

  We note that the foreach subcommand provides $path to user scripts
  (ie it is part of the API), so we can't simply rename it to $sm_path.

That's exactly why that one is still there. We could deprecate it in favor of $sm_path and remove it some time later, but I'm not convinced it is an urgent issue. But me thinks the documentation of foreach should be updated, as it still talks about $path.

Fredrik Gustafsson wrote:

Please do not use a $path variable in any script intended to be run on windows; those poor souls who would otherwise have to fix the bugs will thank you! :-D

Actually, it's not so much the use of a $path variable, rather the act of _exporting_ such a variable that causes the problem. (Which is why using $path with eval_gettext[ln] is such a problem, of course.)

As noted in the above commit, $path is unfortunately a documented part of the public API for the foreach subcommand. However, the foreach subcommand is (mostly) fine; given the fact that the user script is eval-ed in a context in which $path is not exported. The reason for the 'mostly' is simply that the user could shoot himself in the foot by export-ing $path in their script, so that something like:

    $ git submodule foreach 'export path; echo $path `git rev-parse HEAD`'

will indeed fail (ie git rev-parse will not execute).

$path is part of the public API, so we can't just remove it. It would require a deprecation period, etc,. (Adding/documenting $sm_path as an alternative *may* be worth doing. dunno.)

HTH

ATB, Ramsay Jones

On Thu, Aug 08, 2013 at 06:44:22PM +0100, Ramsay Jones wrote:

Maybe something for git 2.0? Well, Jens and Junio is the ones who can make sane decissions about this. I trust they make a good decision. The state as now is that this is a bug for case insesitive systems.

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:

I think exporting sm_path (if not done already) and documenting the transition may be a good starting point, but deprecation of $path may be tricky. There is no good way for us to warn people who continue using $path and ask them to fix their fingers and scripts to use $sm_path instead.

On 08/08/2013 01:44 PM, Ramsay Jones wrote:

Please note that especially in this case, Cygwin != Windows. Cygwin allows $path, $Path, $PATH, etc., to all coexist and be accessed case sensitively. Exporting $path causes no problem, either. Should the eval invoke a Windows program, $PATH is converted to Windows format and exported, the other case-sensitive variants of path remain in the environment and can be accessed by any program implementing case-sensitive lookup as well. Not sure what will happen with case-insensitive lookups, but a quick test showed that cmd.exe is not bothered by $path given $PATH exists.

Mark

git status prints information for submodules, but it should ignore the status of those which have submodule.<name>.ignore set to all. Fix it so that it does properly ignore those which have that setting either in .git/config or in .gitmodules.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 git-submodule.sh  | 2 ++
 t/t7508-status.sh | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

brian m. carlson wrote:

Thanks. Cc-ing Jens, who wrote that test and knows this code much better than I do. :)

[...]

I'm not sure what the exact semantics should be here, though that's mostly because of my unfamiliarity with submodules in general.

If I have '[submodule "favorite"] ignore = all' and I then replace that submodule with a blob, should "git submodule status" not mention that path?

If I just renamed a submodule, will 'module_name "$path"' do the right thing with the old path?

(rest of the patch kept unsnipped for reference)

Am 03.08.2013 20:24, schrieb Jonathan Nieder:

I'm a bit confused. The commit message talks about "git status", but the code you changed handles "git submodule summary". Looks like you are trying to fix the output of status when the status.submodulesummary option is set, right? That's a good thing to do.

But your patch also changes the default behavior of "git submodule summary", which is a change in behavior as that is currently not configurable via the ignore option (and I believe it should stay that way for backward compatibility reasons unless actual users provide sound reasons to change that). So a NACK on this patch from me (and a note to self that tests are missing that should have failed due to this change).

As a short term solution you could honor the submodule.<name>.ignore setting only if --for-status is used, as that is explicitly given by "git status" when it forks the "git submodule summary" script (to make it prepend "# " to each line, which it could do easily itself nowadays using recently added code ;-).

If you want to fix that issue and make git status perform a lot better, you should make the status.submodulesummary use what we already have in the diff machinery instead of forking the submodule script (which it does for hysterical raisins). Basically you'd need to run just what "git diff-files" and "git diff-index HEAD" run when they are given the --submodule option, prepend "# " to the output and limit it to the amount of lines configured via the status.submodulesummary option. Then we could get rid of the --for-status option of submodule summary and move some more functionality from that script into core git.

I'll be glad to help you fixing this problem either way.

On Sun, Aug 04, 2013 at 08:24:09PM +0200, Jens Lehmann wrote:

Right, that wasn't the intent.

I think I'm going to go this route. My goal is to fix up the TODO tests and make them work so I can get more familiar with the code.

On Sat, Aug 03, 2013 at 11:24:20AM -0700, Jonathan Nieder wrote:

Yes, I think it should.  I'll fix this in the reroll.

module_name uses whatever's in .gitmodules. I'm not sure what you mean by "renamed a submodule", since "git mv foo bar" fails with:

  vauxhall ok % git mv .vim/bundle/ctrlp .vim/bundle/ctrlq
  fatal: source directory is empty, source=.vim/bundle/ctrlp, destination=.vim/bundle/ctrlq

Can you provide me a set of steps to reproduce that operation so I can test it effectively?

brian m. carlson wrote:

Ah, I forgot Jens's "submodule-mv" patch series has not hit master yet. You can get it by running

	git merge origin/pu^{/jl/submodule-mv}^2

before building git.

Thanks, Jonathan

On Sun, Aug 11, 2013 at 04:03:17PM +0000, brian m. carlson wrote:

Okay, I've tested this against next and it seems that the code handles it properly (at least I think it does). I left the following code

  # Always show modules deleted or type-changed (blob<->module)
  test $status = D -o $status = T && echo "$sm_path" && continue

before the ignore code, so I adjusted the new ignore code so that it prints both the new and the old path. I don't know that it's possible to print neither in that case, since we no longer can look up the old path in .gitmodules. I'll be sending my reroll shortly.