Stefan Beller <sbeller@xxxxxxxxxx> writes: > Well I wanted to express: > > The .gitmodules used in these Gerrit projects do not conform > to Gits understanding of how .gitmodules should look like. > Let's make Git understand this Gerrit corner case (which you > would only need when using Gerrit). > > Adding special treatment of the "." value is safe as this is an > invalid branch name in Git. Yup, I got it after reading it twice. My point was that you shouldn't have to read it twice to get it. >> I wonder if the above 8-line block wants to be encapsulated to >> become a part of "git submodule--helper" interface, though. IOW, >> branch=$(git submodule--helper branch "$name") or something? > > There is already get_submodule_config that we implement in shell, > which is also used in cmd_summary for reading the .ignore > field. > > So having the "." check in that method (whether in shell or in C) > doesn't make sense to me. That's an excuse from the helper implementor's side, isn't it? I was coming from the opposite direction, i.e. potential caller of a helper. Whenever I want to know "is there a branch configured for this submodule, and if so what is it?", wouldn't I be entitled to a helper that consistently gets the real branch name with the magic "." resolved for me? >>> + test "$(git log -1 --oneline)" = "$(GIT_DIR=../../submodule/.git git log -1 --oneline master)" >> >> A few issues: >> >> * A crash in "git log" would not be noticed with this. Perhaps >> >> git log -1 --oneline $one_way_to_invoke >expect && >> git log -1 --oneline $the_other_way >actual && >> test_cmp expect actual >> >> would be better? > > Of course. I tried to blend in with the code after looking at the surrounding > code. Maybe I need to modernize that whole test file as a preparatory step. >> >> * What exactly is this testing? The current branch (in submodule) >> pointing at the same commit as the tip of 'master'? Or the >> current branch _is_ 'master'? >> >> * What exactly is the reason why one has GIT_DIR=... and the other >> does not? I do not think this a place to test that "gitdir: " >> in .git points at the right place, so it must be testing >> something else, but I cannot guess. > > It is testing that the local state is at the same commit as the > master branch on the remote side. Ahh, OK, I totally misread that. "git -C ../../submodule log" would have been the more modern way to say that, I would guess, but now it makes sense. >>> + # update is not confused by branch="." even if the the superproject >>> + # is not on any branch currently >>> + git submodule update && >>> + git revert HEAD && >> >> "revert" is rather unusual thing to see in the test. > > The tests are so long that I tried to get back in a state that is as least > different from before to not break the following tests. I guessed that much; I just expected to see "git reset --hard $some_old_state" if you want to rewind to the previous state the next test expects and "revert" looked unusual. >> Also I am not >> sure why cmd_update that now has an explicit check to die when >> branch is set to "." and the head is detached is expected "not" to >> be confused. Perhaps I misread the main part of the patch? > > Well you *only* explicitly die(..) when you ask for --remote. OK, I _did_ misread the patch, then. It would help to have "when giving no --remote, git submodule" before the comment that begins with "update is not confused" to avoid the same confusion. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html