* Bug: Segmentation fault (core dumped) @ 2013-10-04 13:54 Robert Mitwicki 2013-10-04 14:20 ` [PATCH] clone: do not segfault when specifying a nonexistent branch Stefan Beller 0 siblings, 1 reply; 9+ messages in thread From: Robert Mitwicki @ 2013-10-04 13:54 UTC (permalink / raw) To: git [-- Attachment #1: Type: text/plain, Size: 265 bytes --] 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 [-- Attachment #2: Log.txt --] [-- Type: text/plain, Size: 2727 bytes --] > git clone --depth 1 -b test https://github.com/mitfik/coredump.git /tmp/coredump.git Cloning into '/tmp/coredump.git'... warning: You appear to have cloned an empty repository. Segmentation fault (core dumped) Unexpected end of command stream > git --version git version 1.8.4 (gdb) info registers rax 0x0 0 rbx 0x0 0 rcx 0x0 0 rdx 0x72 114 rsi 0x519d04 5348612 rdi 0x58 88 rbp 0x7d9680 0x7d9680 rsp 0x7fffffffd8f8 0x7fffffffd8f8 r8 0x7fffffffe30b 140737488347915 r9 0x58 88 r10 0x7fffffffd6c0 140737488344768 r11 0x7ffff72c1d50 140737340251472 r12 0x0 0 r13 0x1 1 r14 0x58 88 r15 0x0 0 rip 0x7ffff72c1d68 0x7ffff72c1d68 eflags 0x10206 [ PF IF RF ] cs 0x33 51 ss 0x2b 43 ds 0x0 0 es 0x0 0 fs 0x0 0 gs 0x0 0 (gdb) bt #0 0x00007ffff72c1d68 in ?? () from /lib/x86_64-linux-gnu/libc.so.6 #1 0x00000000004204a0 in ?? () #2 0x0000000000405b84 in ?? () #3 0x0000000000404f6d in ?? () #4 0x00007ffff71af76d in __libc_start_main () from /lib/x86_64-linux-gnu/libc.so.6 #5 0x00000000004053b9 in ?? () #6 0x00007fffffffe078 in ?? () #7 0x000000000000001c in ?? () #8 0x0000000000000008 in ?? () #9 0x00007fffffffe2eb in ?? () #10 0x00007fffffffe310 in ?? () #11 0x00007fffffffe337 in ?? () #12 0x0000000000000000 in ?? () (gdb) x/60x $sp 0x7fffffffd8f8: 0x00000000 0x00000000 0x007d9680 0x00000000 0x7fffffffd908: 0x00000000 0x00000000 0x00000001 0x00000000 0x7fffffffd918: 0x00000058 0x00000000 0x00000000 0x00000000 0x7fffffffd928: 0x004204a0 0x00000000 0xffffe30b 0x00007fff 0x7fffffffd938: 0xf7fdcd20 0x00007fff 0x007d1fd0 0x00000000 0x7fffffffd948: 0x00000000 0x00000000 0xffffdd60 0x00007fff 0x7fffffffd958: 0xffffdad0 0x00000000 0x00000000 0x00000000 0x7fffffffd968: 0x00000000 0x00000000 0x00000000 0x00000000 0x7fffffffd978: 0xf7fdb630 0x00007fff 0x00000001 0x00007fff 0x7fffffffd988: 0x00000000 0x00000000 0x00000001 0xffffefbd 0x7fffffffd998: 0xf7fdc9c8 0x00007fff 0x003b4700 0x00000000 0x7fffffffd9a8: 0x003b4700 0x00000000 0x00003900 0x00000000 0x7fffffffd9b8: 0xffffddd0 0x00007fff 0xffffde50 0x00007fff 0x7fffffffd9c8: 0xf7ffa4c8 0x00007fff 0x00000000 0x00000000 0x7fffffffd9d8: 0xf7fdcd20 0x00007fff 0xffffdb30 0x00000001 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] clone: do not segfault when specifying a nonexistent branch 2013-10-04 13:54 Bug: Segmentation fault (core dumped) Robert Mitwicki @ 2013-10-04 14:20 ` Stefan Beller 2013-10-04 23:55 ` Duy Nguyen 0 siblings, 1 reply; 9+ messages in thread From: Stefan Beller @ 2013-10-04 14:20 UTC (permalink / raw) To: gitster, ralf.thielow, robert.mitwicki, git; +Cc: Stefan Beller I think we should emit a warning additionally? Signed-off-by: Stefan Beller <stefanbeller@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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] clone: do not segfault when specifying a nonexistent branch 2013-10-04 14:20 ` [PATCH] clone: do not segfault when specifying a nonexistent branch Stefan Beller @ 2013-10-04 23:55 ` Duy Nguyen 2013-10-06 9:27 ` Stefan Beller 2013-10-09 16:38 ` Ralf Thielow 0 siblings, 2 replies; 9+ messages in thread From: Duy Nguyen @ 2013-10-04 23:55 UTC (permalink / raw) To: Stefan Beller Cc: Junio C Hamano, Ralf Thielow, robert.mitwicki, Git Mailing List On Fri, Oct 4, 2013 at 9:20 PM, Stefan Beller <stefanbeller@googlemail.com> wrote: > I think we should emit a warning additionally? > > Signed-off-by: Stefan Beller <stefanbeller@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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] clone: do not segfault when specifying a nonexistent branch 2013-10-04 23:55 ` Duy Nguyen @ 2013-10-06 9:27 ` Stefan Beller 2013-10-07 10:46 ` Duy Nguyen 2013-10-09 16:38 ` Ralf Thielow 1 sibling, 1 reply; 9+ messages in thread From: Stefan Beller @ 2013-10-06 9:27 UTC (permalink / raw) To: Duy Nguyen Cc: Junio C Hamano, Ralf Thielow, robert.mitwicki, Git Mailing List On 10/05/2013 01:55 AM, Duy Nguyen wrote: > On Fri, Oct 4, 2013 at 9:20 PM, Stefan Beller > <stefanbeller@googlemail.com> wrote: >> I think we should emit a warning additionally? >> >> Signed-off-by: Stefan Beller <stefanbeller@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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] clone: do not segfault when specifying a nonexistent branch 2013-10-06 9:27 ` Stefan Beller @ 2013-10-07 10:46 ` Duy Nguyen 2013-10-08 10:06 ` Stefan Beller 0 siblings, 1 reply; 9+ messages in thread From: Duy Nguyen @ 2013-10-07 10:46 UTC (permalink / raw) To: Stefan Beller Cc: Junio C Hamano, Ralf Thielow, robert.mitwicki, Git Mailing List On Sun, Oct 6, 2013 at 4:27 PM, Stefan Beller <stefanbeller@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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] clone: do not segfault when specifying a nonexistent branch 2013-10-07 10:46 ` Duy Nguyen @ 2013-10-08 10:06 ` Stefan Beller 0 siblings, 0 replies; 9+ messages in thread From: Stefan Beller @ 2013-10-08 10:06 UTC (permalink / raw) To: gitster, ralf.thielow, robert.mitwicki, git, pclouds, jrnieder Cc: 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.mitwicki@opensoftware.pl> Signed-off-by: Stefan Beller <stefanbeller@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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] clone: do not segfault when specifying a nonexistent branch 2013-10-04 23:55 ` Duy Nguyen 2013-10-06 9:27 ` Stefan Beller @ 2013-10-09 16:38 ` Ralf Thielow 2013-10-11 16:49 ` [PATCH] clone --branch: refuse to clone if upstream repo is empty Ralf Thielow 1 sibling, 1 reply; 9+ messages in thread From: Ralf Thielow @ 2013-10-09 16:38 UTC (permalink / raw) To: Duy Nguyen Cc: Stefan Beller, Junio C Hamano, robert.mitwicki, Git Mailing List On Sat, Oct 5, 2013 at 1:55 AM, Duy Nguyen <pclouds@gmail.com> wrote: > On Fri, Oct 4, 2013 at 9:20 PM, Stefan Beller > <stefanbeller@googlemail.com> wrote: >> I think we should emit a warning additionally? >> >> Signed-off-by: Stefan Beller <stefanbeller@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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] clone --branch: refuse to clone if upstream repo is empty 2013-10-09 16:38 ` Ralf Thielow @ 2013-10-11 16:49 ` Ralf Thielow 2013-10-14 19:08 ` Duy Nguyen 0 siblings, 1 reply; 9+ messages in thread From: Ralf Thielow @ 2013-10-11 16:49 UTC (permalink / raw) To: git Cc: stefanbeller, gitster, jrnieder, robert.mitwicki, pclouds, Ralf Thielow 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 <ralf.thielow@gmail.com> --- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] clone --branch: refuse to clone if upstream repo is empty 2013-10-11 16:49 ` [PATCH] clone --branch: refuse to clone if upstream repo is empty Ralf Thielow @ 2013-10-14 19:08 ` Duy Nguyen 0 siblings, 0 replies; 9+ messages in thread From: Duy Nguyen @ 2013-10-14 19:08 UTC (permalink / raw) To: Ralf Thielow Cc: Git Mailing List, Stefan Beller, Junio C Hamano, Jonathan Nieder, robert.mitwicki On Fri, Oct 11, 2013 at 11:49 PM, Ralf Thielow <ralf.thielow@gmail.com> 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 :) > Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com> > --- > 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 > -- Duy ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-10-14 19:09 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-10-04 13:54 Bug: Segmentation fault (core dumped) Robert Mitwicki 2013-10-04 14:20 ` [PATCH] clone: do not segfault when specifying a nonexistent branch Stefan Beller 2013-10-04 23:55 ` Duy Nguyen 2013-10-06 9:27 ` Stefan Beller 2013-10-07 10:46 ` Duy Nguyen 2013-10-08 10:06 ` Stefan Beller 2013-10-09 16:38 ` Ralf Thielow 2013-10-11 16:49 ` [PATCH] clone --branch: refuse to clone if upstream repo is empty Ralf Thielow 2013-10-14 19:08 ` Duy Nguyen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.