Re: [PATCH] clone: do not segfault when specifying a nonexistent branch

[Ralf Thielow]

On Sat, Oct 5, 2013 at 1:55 AM, Duy Nguyen pclo...@gmail.com wrote:
 On Fri, Oct 4, 2013 at 9:20 PM, Stefan Beller
 stefanbel...@googlemail.com wrote:
 I think we should emit a warning additionally?

 Signed-off-by: Stefan Beller stefanbel...@googlemail.com

 I think it's nice to credit Robert for reporting the fault in the
 commit message (something like reported-by: or noticed-by:...)

 ---
  builtin/clone.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/builtin/clone.c b/builtin/clone.c
 index 0aff974..b764ad0 100644
 --- a/builtin/clone.c
 +++ b/builtin/clone.c
 @@ -688,7 +688,7 @@ static void write_refspec_config(const char* 
 src_ref_prefix,

 if (option_mirror || !option_bare) {
 if (option_single_branch  !option_mirror) {
 -   if (option_branch) {
 +   if (option_branch  our_head_points_at) {
 if (strstr(our_head_points_at-name, 
 refs/tags/))
 strbuf_addf(value, +%s:%s, 
 our_head_points_at-name,
 our_head_points_at-name);

 This prevents the segfault, but what about remote.*.fetch? Should we
 setup standard refspec for fetch or..?
 --
 Duy

This segfault only happens when cloning an empty repository and only with option
--single-branch. Or do I miss something?

If we call git clone for a non-empty repository with a non-existing branch
using [--single-branch] --branch foo then Git will abort with a message that
the branch doesn't exist in upstream.

In an empty upstream repo the branch doesn't exist, either. So why not
abort with
the same message? That would be consistent. Otherwise I'd just
override the options
--single-branch and --branch to not set.

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

[PATCH] clone: do not segfault when specifying a nonexistent branch

[Stefan Beller]

Actually I only wanted to change one line to prevent a crash, when you
specify a non existing branch when cloning:
-   if (option_branch) {
+   if (option_branch  our_head_points_at) {

However it turns out this is not a good idea as we still want to setup
'remote.*.fetch', which previously depended the string buffer 'value'
being non empty.
Therefore I added a local variable 'set_remote', which determines whether
we want to setup 'remote.*.fetch'.


While staring at the code, I also think it is a good idea to restructure
the if clauses a little as previously we had
if (option_mirror || !option_bare) {
if (option_single_branch  !option_mirror) {
The 'option_mirror' is part of both ifs, but opposing each other.
This is not yet done in this patch, as it still needs some thinking how to
remove the nesting of the if clauses in a nice way.

Reported-by: Robert Mitwicki robert.mitwi...@opensoftware.pl
Signed-off-by: Stefan Beller stefanbel...@googlemail.com
---
 builtin/clone.c | 50 --
 1 file changed, 28 insertions(+), 22 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 0aff974..8b9a78a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -686,40 +686,46 @@ static void write_refspec_config(const char* 
src_ref_prefix,
struct strbuf key = STRBUF_INIT;
struct strbuf value = STRBUF_INIT;
 
+   int set_remote = 0;
if (option_mirror || !option_bare) {
+   set_remote = 1;
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);
+   if (our_head_points_at) {
+   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/));
+   } else {
+   /*
+* otherwise, the next git fetch will
+* simply fetch from HEAD without updating
+* any remote-tracking branch, which is what
+* we want.
+*/
+   set_remote = 0;
}
-   /*
-* 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_addf(key, remote.%s.fetch, option_origin);
-   git_config_set_multivar(key.buf, value.buf, ^$, 0);
-   strbuf_reset(key);
+   }
+   /* Configure the remote */
+   if (set_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);
-   }
+   if (option_mirror) {
+   strbuf_addf(key, remote.%s.mirror, option_origin);
+   git_config_set(key.buf, true);
+   strbuf_reset(key);
}
}
 
-- 
1.8.4.1.469.gb38b9db

--
To unsubscribe from this list: send the line 

Re: [PATCH] clone: do not segfault when specifying a nonexistent branch

[Duy Nguyen]

On Sun, Oct 6, 2013 at 4:27 PM, Stefan Beller
stefanbel...@googlemail.com wrote:
 @@ -688,7 +688,7 @@ static void write_refspec_config(const char* 
 src_ref_prefix,

 if (option_mirror || !option_bare) {
 if (option_single_branch  !option_mirror) {
 -   if (option_branch) {
 +   if (option_branch  our_head_points_at) {
 if (strstr(our_head_points_at-name, 
 refs/tags/))
 strbuf_addf(value, +%s:%s, 
 our_head_points_at-name,
 our_head_points_at-name);

 This prevents the segfault, but what about remote.*.fetch? Should we
 setup standard refspec for fetch or..?


 Looking at the code a few lines below, this comment comes up:

 /*
  * otherwise, the next git fetch will
  * simply fetch from HEAD without updating
  * any remote-tracking branch, which is what
  * we want.
  */

 This behavior was good for the case (!option_branch  !remote_head_points_at)
 Now we extend that behavior doing nothing to
  ((!option_branch || !our_head_points_at)   !remote_head_points_at)

 I am not sure how to handle that case best. The user has given a non existing 
 branch,
 so it doesn't make sense to track that branch, but only have that
 registered as a remote*.fetch?

 Reading the documentation enhancements of 31b808a
 (2012-09-20, clone --single: limit the fetch refspec to fetched branch), 
 doesn't
 talk about this corner case. So maybe the remote.*.fetch shall be set, but no 
 branch
 should be checked out, when running
 git clone --depth 1 -b test https://github.com/mitfik/coredump.git 
 /tmp/coredump.git

 Does that make sense?

Looking further back to 86ac751 (Allow cloning an empty repository -
2009-01-23), the reason to allow cloning an empty repository is so
that the user does not have do manual configuration, so I agree with
your maybe. git-clone.txt should have a short description about this
too in case somebody runs into this and cares enough to check the
document before heading to Stack Overflow.
-- 
Duy
--
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: [PATCH] clone: do not segfault when specifying a nonexistent branch

[Stefan Beller]

On 10/05/2013 01:55 AM, Duy Nguyen wrote:
 On Fri, Oct 4, 2013 at 9:20 PM, Stefan Beller
 stefanbel...@googlemail.com wrote:
 I think we should emit a warning additionally?

 Signed-off-by: Stefan Beller stefanbel...@googlemail.com
 
 I think it's nice to credit Robert for reporting the fault in the
 commit message (something like reported-by: or noticed-by:...)

I'll do so in a resend.

 
 ---
  builtin/clone.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/builtin/clone.c b/builtin/clone.c
 index 0aff974..b764ad0 100644
 --- a/builtin/clone.c
 +++ b/builtin/clone.c
 @@ -688,7 +688,7 @@ static void write_refspec_config(const char* 
 src_ref_prefix,

 if (option_mirror || !option_bare) {
 if (option_single_branch  !option_mirror) {
 -   if (option_branch) {
 +   if (option_branch  our_head_points_at) {
 if (strstr(our_head_points_at-name, 
 refs/tags/))
 strbuf_addf(value, +%s:%s, 
 our_head_points_at-name,
 our_head_points_at-name);
 
 This prevents the segfault, but what about remote.*.fetch? Should we
 setup standard refspec for fetch or..?
 

Looking at the code a few lines below, this comment comes up:

/*
 * otherwise, the next git fetch will
 * simply fetch from HEAD without updating
 * any remote-tracking branch, which is what
 * we want.
 */

This behavior was good for the case (!option_branch  !remote_head_points_at)
Now we extend that behavior doing nothing to
 ((!option_branch || !our_head_points_at)   !remote_head_points_at)

I am not sure how to handle that case best. The user has given a non existing 
branch,
so it doesn't make sense to track that branch, but only have that 
registered as a remote*.fetch?

Reading the documentation enhancements of 31b808a 
(2012-09-20, clone --single: limit the fetch refspec to fetched branch), 
doesn't 
talk about this corner case. So maybe the remote.*.fetch shall be set, but no 
branch
should be checked out, when running 
git clone --depth 1 -b test https://github.com/mitfik/coredump.git 
/tmp/coredump.git

Does that make sense?

Stefan




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

[PATCH] clone: do not segfault when specifying a nonexistent branch

[Stefan Beller]

I think we should emit a warning additionally?

Signed-off-by: Stefan Beller stefanbel...@googlemail.com
---
 builtin/clone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 0aff974..b764ad0 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -688,7 +688,7 @@ static void write_refspec_config(const char* src_ref_prefix,
 
if (option_mirror || !option_bare) {
if (option_single_branch  !option_mirror) {
-   if (option_branch) {
+   if (option_branch  our_head_points_at) {
if (strstr(our_head_points_at-name, 
refs/tags/))
strbuf_addf(value, +%s:%s, 
our_head_points_at-name,
our_head_points_at-name);
-- 
1.8.4.1.469.gb38b9db

--
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: [PATCH] clone: do not segfault when specifying a nonexistent branch

[Duy Nguyen]

On Fri, Oct 4, 2013 at 9:20 PM, Stefan Beller
stefanbel...@googlemail.com wrote:
 I think we should emit a warning additionally?

 Signed-off-by: Stefan Beller stefanbel...@googlemail.com

I think it's nice to credit Robert for reporting the fault in the
commit message (something like reported-by: or noticed-by:...)

 ---
  builtin/clone.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/builtin/clone.c b/builtin/clone.c
 index 0aff974..b764ad0 100644
 --- a/builtin/clone.c
 +++ b/builtin/clone.c
 @@ -688,7 +688,7 @@ static void write_refspec_config(const char* 
 src_ref_prefix,

 if (option_mirror || !option_bare) {
 if (option_single_branch  !option_mirror) {
 -   if (option_branch) {
 +   if (option_branch  our_head_points_at) {
 if (strstr(our_head_points_at-name, 
 refs/tags/))
 strbuf_addf(value, +%s:%s, 
 our_head_points_at-name,
 our_head_points_at-name);

This prevents the segfault, but what about remote.*.fetch? Should we
setup standard refspec for fetch or..?
-- 
Duy
--
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