Re: [PATCHv2 2/2] submodule update: allow '.' for branch value

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 28, 2016, 20:02 UTC
Message-ID: <xmqqy44lr0o4.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAGZ79kactn4VZ5i+fCrBGa77dzFiTngAgVU3R1orz2=EMAZ1Jg@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

Show 9 quoted lines

> 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.

Show 10 quoted lines

>> 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?

Show 26 quoted lines

>>> +       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.

Show 9 quoted lines

>>> +      # 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.

Show 6 quoted lines

>> 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.