Bug: Segmentation fault (core dumped)

Hi,

When I am trying to clone an empty repository and I will use together
--depth 1 and -b branch_name (branch does not exist) then I get
Segmentation fault (repo seems to be cloned correctly).

Please see attachment for more details.
Best regards
Robert Mitwicki

I think we should emit a warning additionally?

Signed-off-by: Stefan Beller <redacted>
---
 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

On Fri, Oct 4, 2013 at 9:20 PM, Stefan Beller
[off-list ref] wrote:

I think we should emit a warning additionally?

Signed-off-by: Stefan Beller <redacted>

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

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

On 10/05/2013 01:55 AM, Duy Nguyen wrote:

On Fri, Oct 4, 2013 at 9:20 PM, Stefan Beller
[off-list ref] wrote:

quoted

I think we should emit a warning additionally?

Signed-off-by: Stefan Beller <redacted>

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.

quoted

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

On Sun, Oct 6, 2013 at 4:27 PM, Stefan Beller
[off-list ref] wrote:

quoted

quoted

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

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 <redacted>
Signed-off-by: Stefan Beller <redacted>
---
 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

On Sat, Oct 5, 2013 at 1:55 AM, Duy Nguyen [off-list ref] wrote:

On Fri, Oct 4, 2013 at 9:20 PM, Stefan Beller
[off-list ref] wrote:

quoted

I think we should emit a warning additionally?

Signed-off-by: Stefan Beller <redacted>

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

quoted

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

Since 920b691 (clone: refuse to clone if --branch
points to bogus ref) we refuse to clone with option
"-b" if the specified branch does not exist in the
(non-empty) upstream. If the upstream repository is empty,
the branch doesn't exist, either. So refuse the clone too.

Signed-off-by: Ralf Thielow <redacted>
---
 builtin/clone.c         | 4 ++++
 t/t5706-clone-branch.sh | 8 +++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index ca3eb68..5af386e 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c

@@ -945,6 +945,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			our_head_points_at = remote_head_points_at;
 	}
 	else {
+		if (option_branch)
+			die(_("Remote branch %s not found in upstream %s"),
+					option_branch, option_origin);
+
 		warning(_("You appear to have cloned an empty repository."));
 		mapped_refs = NULL;
 		our_head_points_at = NULL;

diff --git a/t/t5706-clone-branch.sh b/t/t5706-clone-branch.sh
index 56be67e..6e7a7be 100755
--- a/t/t5706-clone-branch.sh
+++ b/t/t5706-clone-branch.sh

@@ -20,7 +20,9 @@ test_expect_success 'setup' '
 	 echo one >file && git add file && git commit -m one &&
 	 git checkout -b two &&
 	 echo two >file && git add file && git commit -m two &&
-	 git checkout master)
+	 git checkout master) &&
+	mkdir empty &&
+	(cd empty && git init)
 '
 
 test_expect_success 'vanilla clone chooses HEAD' '

@@ -61,4 +63,8 @@ test_expect_success 'clone -b with bogus branch' '
 	test_must_fail git clone -b bogus parent clone-bogus
 '
 
+test_expect_success 'clone -b not allowed with empty repos' '
+	test_must_fail git clone -b branch empty clone-branch-empty
+'
+
 test_done

-- 
1.8.4.652.g0d6e0ce

On Fri, Oct 11, 2013 at 11:49 PM, Ralf Thielow [off-list ref] wrote:

Since 920b691 (clone: refuse to clone if --branch
points to bogus ref) we refuse to clone with option
"-b" if the specified branch does not exist in the
(non-empty) upstream. If the upstream repository is empty,
the branch doesn't exist, either. So refuse the clone too.

Yeah, much simpler approach :)

-- 
Duy