Re: [PATCHv5] clone --single: limit the fetch refspec to fetched branch

On Mon, Sep 17, 2012 at 10:18 PM, Junio C Hamano [off-list ref] wrote:

Ralf Thielow [off-list ref] writes:

quoted

- handle --mirror option (test added)

Handle how?  I personally think erroring out is the right way to
handle it, but if we care about people who have been misusing the
combination of single and mirror, the second best way would be to
imply "mirror" and "single" combination as "bare" and "single"
without "mirror".

With this patch it would be "bare" and "single", and with the next
fetch a "mirror". However, the combination of "single" and "mirror"
doesn't really make sense so I also think we should error it out.

quoted

- install correct refspec if the value of --branch is a tag (test added)

What is the definition of "correct"?  I see the documentation says
"--branch can also take tags and treat them like detached HEAD", and
even though I _think_ allowing tags was a huge mistake, if we claim
we treat it like detached HEAD, we should do the same when coming up
with the refspec used for the follow-up "fetch".

This patch would install the refspec "+refs/tags/v1.7.10:refs/tags/v1.7.10",
so we would do the same with the follow-up "fetch", not?
This can be seen as "it's not a bug, it's a feature". Why not cloning a
tag of a repo if someone just want to build a tag without having anything else.

Whatever we decide to do, the semantics we decided to use at least
need to be described in the commit log message, not just in the
"changes from the previous iteration".  Updating the
"Documentation/git-clone.txt" would also be necessary.

Ok.

quoted

+test_expect_success 'refspec contains all branches by default' '
+     echo "+refs/heads/*:refs/remotes/origin/*" > expect &&
+     git --git-dir=dir_all/.git config --get remote.origin.fetch > actual &&
+     test_cmp expect actual
+'

I still think these checks that know the current implementation
details (i.e. what exact configuration variables get what exact
values) are wrong thing to have in the longer term.  If the desired
behaviour (i.e. "later fetch do not screw up") can be tested
directly like the later parts of the test in this patch does, how
that desired behaviour is implemented should not have to be cast in
stone with these tests.

You wrote

I'll let it pass for now, though.

so I haven't removed them yet. I'll delete them.