Re: [PATCHv5] clone --single: limit the fetch refspec to fetched branch
Ralf Thielow writes: > IMO if a user uses "clone --single-branch --branch ", then he/she > wants to have this tag only. Why should the next "git fetch" fetching > something different? OK, I can buy that. -- 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: [PATCHv5] clone --single: limit the fetch refspec to fetched branch
On Mon, Sep 17, 2012 at 11:39 PM, Junio C Hamano wrote: > Ralf Thielow writes: > - 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. > > Even though I did say I thought allowing tags was a huge mistake, I > was not suggesting to break existing users by making such a clone > into an error. > > But the main point of the discussion is what should happen upon the > next "git fetch" in the repository, no? Shouldn't the refspec be > the same as the case where you "clone --single" a repository whose > HEAD is detached at v1.7.10 in that example, instead of saying > "fetch the same tag over and over"? After all that is the way I > expect that most people would read "treat them line detached HEAD" > in the documentation. Subsequent "git fetch" would fetch from the > HEAD of the upstream just like a clone from a repository with a > detached HEAD. > IMO if a user uses "clone --single-branch --branch ", then he/she wants to have this tag only. Why should the next "git fetch" fetching something different? I read "treat them like detached HEAD" in the way that the resulting repo is in a "detached HEAD" state. > The above is *not* a rhetorical question; I just do not think of a > sensible reason why we want a refspec that says "keep updating the > v1.7.10 tag, as it might change on the other end, and do not bother > what other things that happen in that upstream repository". What > sensible workflow would benefit from such a refspec? > > When using a tag in a different meaning than "tag this version and do never change it", for example if someone uses a tag to describe which revision of the project was lastly delivered to the customer, then they could use a tag "delivered". After a new version was delivered, someone changes this tag from "revx" to "revy". Having such a refspec, you can fetch this tag and always get the delivered version. This example is very theoretically. -- 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: [PATCHv5] clone --single: limit the fetch refspec to fetched branch
Ralf Thielow writes: >>> - 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. Even though I did say I thought allowing tags was a huge mistake, I was not suggesting to break existing users by making such a clone into an error. But the main point of the discussion is what should happen upon the next "git fetch" in the repository, no? Shouldn't the refspec be the same as the case where you "clone --single" a repository whose HEAD is detached at v1.7.10 in that example, instead of saying "fetch the same tag over and over"? After all that is the way I expect that most people would read "treat them line detached HEAD" in the documentation. Subsequent "git fetch" would fetch from the HEAD of the upstream just like a clone from a repository with a detached HEAD. The above is *not* a rhetorical question; I just do not think of a sensible reason why we want a refspec that says "keep updating the v1.7.10 tag, as it might change on the other end, and do not bother what other things that happen in that upstream repository". What sensible workflow would benefit from such a refspec? -- 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: [PATCHv5] clone --single: limit the fetch refspec to fetched branch
On Mon, Sep 17, 2012 at 10:18 PM, Junio C Hamano wrote: > Ralf Thielow writes: > >> - 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. >> - 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. >> +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. -- 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: [PATCHv5] clone --single: limit the fetch refspec to fetched branch
Ralf Thielow writes: > - 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". > - 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". 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. > +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. -- 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
[PATCHv5] clone --single: limit the fetch refspec to fetched branch
After running "git clone --single", the resulting repository has the
usual default "+refs/heads/*:refs/remotes/origin/*" wildcard fetch
refspec installed, which means that a subsequent "git fetch" will
end up grabbing all the other branches.
Update the fetch refspec to cover only the singly cloned ref instead
to correct this.
Signed-off-by: Ralf Thielow
---
changes in v5:
- extract a function to write refspec config
- handle --mirror option (test added)
- install correct refspec if the value of --branch is a tag (test added)
Thanks to Junio for:
- refactor tests
- add tests for created refs
I'm not happy about using "our_head_points_to" for the remote
part of the refspec. Junio already complaint about that. As far
as I can see it's the only way of getting the information that
it's a tag. Also the condition "is this a tag" might not be the
best way.
builtin/clone.c | 66 +++-
t/t5709-clone-refspec.sh | 156 +++
2 files changed, 208 insertions(+), 14 deletions(-)
create mode 100755 t/t5709-clone-refspec.sh
diff --git a/builtin/clone.c b/builtin/clone.c
index 5e8f3ba..431635c 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -610,6 +610,55 @@ static void write_config(struct string_list *config)
}
}
+static void write_refspec_config(const char* src_ref_prefix,
+ const struct ref* our_head_points_at,
+ const struct ref* remote_head_points_at, struct strbuf*
branch_top)
+{
+ struct strbuf key = STRBUF_INIT;
+ struct strbuf value = STRBUF_INIT;
+
+ if (option_mirror || !option_bare) {
+ if (option_single_branch && !option_mirror) {
+ if (option_branch) {
+ if (strstr(our_head_points_at->name,
"refs/tags/"))
+ strbuf_addf(&value, "+%s:%s",
our_head_points_at->name,
+ our_head_points_at->name);
+ else
+ strbuf_addf(&value, "+%s:%s%s",
our_head_points_at->name,
+ branch_top->buf, option_branch);
+ } else if (remote_head_points_at) {
+ strbuf_addf(&value, "+%s:%s%s",
remote_head_points_at->name,
+ branch_top->buf,
+
skip_prefix(remote_head_points_at->name, "refs/heads/"));
+ }
+ /*
+* otherwise, the next "git fetch" will
+* simply fetch from HEAD without updating
+* any remote tracking branch, which is what
+* we want.
+*/
+ } else {
+ strbuf_addf(&value, "+%s*:%s*", src_ref_prefix,
branch_top->buf);
+ }
+ /* Configure the remote */
+ if (value.len) {
+ strbuf_reset(&key);
+ strbuf_addf(&key, "remote.%s.fetch", option_origin);
+ git_config_set_multivar(key.buf, value.buf, "^$", 0);
+ strbuf_reset(&key);
+
+ if (option_mirror) {
+ strbuf_addf(&key, "remote.%s.mirror",
option_origin);
+ git_config_set(key.buf, "true");
+ strbuf_reset(&key);
+ }
+ }
+ }
+
+ strbuf_release(&key);
+ strbuf_release(&value);
+}
+
int cmd_clone(int argc, const char **argv, const char *prefix)
{
int is_bundle = 0, is_local;
@@ -755,20 +804,6 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
}
strbuf_addf(&value, "+%s*:%s*", src_ref_prefix, branch_top.buf);
-
- if (option_mirror || !option_bare) {
- /* Configure the remote */
- strbuf_addf(&key, "remote.%s.fetch", option_origin);
- git_config_set_multivar(key.buf, value.buf, "^$", 0);
- strbuf_reset(&key);
-
- if (option_mirror) {
- strbuf_addf(&key, "remote.%s.mirror", option_origin);
- git_config_set(key.buf, "true");
- strbuf_reset(&key);
- }
- }
-
strbuf_addf(&key, "remote.%s.url", option_origin);
git_config_set(key.buf, repo);
strbuf_reset(&key);
@@ -853,6 +888,9 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
"refs/heads/master");
}
+ write_refspec_config(src_ref_prefix, our_head_points_at,
+ remote_head_points_at, &branch_top);
+
if (is_local)
clone_local(path, git_dir);
else if (refs && complet
