{"thread":{"id":"52169","subject":"[PATCH] Segmentation Fault on non-commit --branch clone","startedAt":"2019-11-01T00:24:39Z","lastAt":"2019-11-06T09:53:07Z","messageCount":14,"participants":["Davide Berardi","Johannes Schindelin","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"385249","messageId":"20191101002432.GA49846@carpenter.lan","threadId":"52169","inReplyTo":null,"subject":"[PATCH] Segmentation Fault on non-commit --branch clone","fromName":"Davide Berardi","fromEmail":"berardi.dav@gmail.com","sentAt":"2019-11-01T00:24:32Z","receivedAt":"2019-11-01T00:24:39Z","isPatch":true,"sender":{"key":"berardi.dav@gmail.com","avatar":"https://gravatar.com/avatar/5d15ae87f80da0830f2d7e86ba3a6553047a1447ca625f1fc29082632e9a7b13?d=mp&s=160"},"body":"Fixed segmentation fault that can be triggered using\n$ git clone --branch $object $repository\nwith object pointing to a non-commit ref (e.g. a blob).\n\nSigned-off-by: Davide Berardi <berardi.dav@gmail.com>\n\n---\n builtin/clone.c         | 26 ++++++++++++++++++++++++++\n refs.h                  |  7 +++++++\n t/t5609-clone-branch.sh | 22 +++++++++++++++++++++-\n 3 files changed, 54 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex f665b28ccc..0f4a18302c 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -704,11 +704,32 @@ static void update_remote_refs(const struct ref *refs,\n \t}\n }\n \n+static int fallback_on_noncommit(const struct ref *check,\n+\t\t\t\t const struct ref *remote,\n+\t\t\t\t const char *msg)\n+{\n+\tif (check == NULL)\n+\t\treturn 1;\n+\tstruct commit *c = lookup_commit_reference_gently(the_repository,\n+\t\t\t\t\t\t   &check->old_oid, 1);\n+\tif (c == NULL) {\n+\t\t/* Fallback HEAD to fallback refs */\n+\t\twarning(_(\"%s is not a valid commit object, HEAD will fallback to %s\"),\n+\t\t\tcheck->name, FALLBACK_REF);\n+\t\tupdate_ref(msg, FALLBACK_REF, &remote->old_oid, NULL,\n+\t\t\t   REF_NO_DEREF, UPDATE_REFS_DIE_ON_ERR);\n+\t}\n+\n+\treturn c == NULL;\n+}\n+\n static void update_head(const struct ref *our, const struct ref *remote,\n \t\t\tconst char *msg)\n {\n \tconst char *head;\n \tif (our && skip_prefix(our->name, \"refs/heads/\", &head)) {\n+\t\tif (fallback_on_noncommit(our, remote, msg) != 0)\n+\t\t\treturn;\n \t\t/* Local default branch link */\n \t\tif (create_symref(\"HEAD\", our->name, NULL) < 0)\n \t\t\tdie(_(\"unable to update HEAD\"));\n@@ -718,12 +739,17 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \t\t\tinstall_branch_config(0, head, option_origin, our->name);\n \t\t}\n \t} else if (our) {\n+\t\tif (fallback_on_noncommit(our, remote, msg) != 0)\n+\t\t\treturn;\n+\t\t/* here commit is valid */\n \t\tstruct commit *c = lookup_commit_reference(the_repository,\n \t\t\t\t\t\t\t   &our->old_oid);\n \t\t/* --branch specifies a non-branch (i.e. tags), detach HEAD */\n \t\tupdate_ref(msg, \"HEAD\", &c->object.oid, NULL, REF_NO_DEREF,\n \t\t\t   UPDATE_REFS_DIE_ON_ERR);\n \t} else if (remote) {\n+\t\tif (fallback_on_noncommit(remote, remote, msg) != 0)\n+\t\t\treturn;\n \t\t/*\n \t\t * We know remote HEAD points to a non-branch, or\n \t\t * HEAD points to a branch but we don't know which one.\ndiff --git a/refs.h b/refs.h\nindex 730d05ad91..35ee6eb058 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -497,6 +497,13 @@ enum action_on_err {\n \tUPDATE_REFS_QUIET_ON_ERR\n };\n \n+/*\n+ * In case of a remote HEAD pointing to a non-commit update_head\n+ * will make HEAD reference fallback to this value, master ref\n+ * should be safe.\n+ */\n+#define FALLBACK_REF \"refs/heads/master\"\n+\n /*\n  * Begin a reference transaction.  The reference transaction must\n  * be freed by calling ref_transaction_free().\ndiff --git a/t/t5609-clone-branch.sh b/t/t5609-clone-branch.sh\nindex 6e7a7be052..0680cf5a89 100755\n--- a/t/t5609-clone-branch.sh\n+++ b/t/t5609-clone-branch.sh\n@@ -20,7 +20,13 @@ test_expect_success 'setup' '\n \t echo one >file && git add file && git commit -m one &&\n \t git checkout -b two &&\n \t echo two >file && git add file && git commit -m two &&\n-\t git checkout master) &&\n+\t git checkout master &&\n+\t # Create dummy objects\n+\t _B=$(git rev-list --objects --all | grep -m 1 \"^[^ ]\\+ [^ ]\\+\" | \\\n+\t      awk \"{print \\$1}\") &&\n+\t echo \"${_B}\" >> .git/refs/tags/broken-tag &&\n+\t echo \"${_B}\" >> .git/refs/heads/broken-head\n+\t) &&\n \tmkdir empty &&\n \t(cd empty && git init)\n '\n@@ -67,4 +73,18 @@ test_expect_success 'clone -b not allowed with empty repos' '\n \ttest_must_fail git clone -b branch empty clone-branch-empty\n '\n \n+test_expect_success 'clone -b with broken tag will fallback to master' '\n+\tgit clone -b broken-tag parent clone-broken-tag &&\n+\t(cd clone-broken-tag &&\n+\t check_HEAD master\n+\t)\n+'\n+\n+test_expect_success 'clone -b with broken head will fallback to master' '\n+\tgit clone -b broken-head parent clone-broken-head &&\n+\t(cd clone-broken-head &&\n+\t check_HEAD master\n+\t)\n+'\n+\n test_done\n-- \n2.22.0\n\n"},{"id":"385274","messageId":"nycvar.QRO.7.76.6.1911012000160.46@tvgsbejvaqbjf.bet","threadId":"52169","inReplyTo":"20191101002432.GA49846@carpenter.lan","subject":"Re: [PATCH] Segmentation Fault on non-commit --branch clone","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-11-01T19:08:10Z","receivedAt":"2019-11-01T19:08:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Davide,\n\nI wonder whether you might want to reword the Subject: such that it\nreflects what the patch does instead of what the problem is that\nmotivated you to work on the patch (the patch does not cause the\nsegmentation fault, after all, but it fixes it).\n\nOn Fri, 1 Nov 2019, Davide Berardi wrote:\n\n> Fixed segmentation fault that can be triggered using\n> $ git clone --branch $object $repository\n> with object pointing to a non-commit ref (e.g. a blob).\n>\n> Signed-off-by: Davide Berardi <berardi.dav@gmail.com>\n>\n> ---\n> builtin/clone.c         | 26 ++++++++++++++++++++++++++\n> refs.h                  |  7 +++++++\n> t/t5609-clone-branch.sh | 22 +++++++++++++++++++++-\n> 3 files changed, 54 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index f665b28ccc..0f4a18302c 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -704,11 +704,32 @@ static void update_remote_refs(const struct ref *refs,\n> \t}\n> }\n>\n> +static int fallback_on_noncommit(const struct ref *check,\n> +\t\t\t\t const struct ref *remote,\n> +\t\t\t\t const char *msg)\n> +{\n> +\tif (check == NULL)\n> +\t\treturn 1;\n> +\tstruct commit *c = lookup_commit_reference_gently(the_repository,\n> +\t\t\t\t\t\t   &check->old_oid, 1);\n> +\tif (c == NULL) {\n> +\t\t/* Fallback HEAD to fallback refs */\n> +\t\twarning(_(\"%s is not a valid commit object, HEAD will fallback\n> to %s\"),\n> +\t\t\tcheck->name, FALLBACK_REF);\n\nQuite honestly, I do not think that it is a good idea to fall back in\nthis case. The user asked for something that cannot be accomplished, and\nthe best way to handle this is to exit with an error, i.e. `die()`.\n\n> +\t\tupdate_ref(msg, FALLBACK_REF, &remote->old_oid, NULL,\n> +\t\t\t   REF_NO_DEREF, UPDATE_REFS_DIE_ON_ERR);\n> +\t}\n> +\n> +\treturn c == NULL;\n> +}\n> +\n> static void update_head(const struct ref *our, const struct ref *remote,\n> \t\t\tconst char *msg)\n> {\n> \tconst char *head;\n> \tif (our && skip_prefix(our->name, \"refs/heads/\", &head)) {\n> +\t\tif (fallback_on_noncommit(our, remote, msg) != 0)\n> +\t\t\treturn;\n> \t\t/* Local default branch link */\n> \t\tif (create_symref(\"HEAD\", our->name, NULL) < 0)\n> \t\t\tdie(_(\"unable to update HEAD\"));\n> @@ -718,12 +739,17 @@ static void update_head(const struct ref *our, const\n> struct ref *remote,\n> \t\t\tinstall_branch_config(0, head, option_origin,\n> \t\tour->name);\n> \t\t}\n> \t} else if (our) {\n> +\t\tif (fallback_on_noncommit(our, remote, msg) != 0)\n> +\t\t\treturn;\n> +\t\t/* here commit is valid */\n> \t\tstruct commit *c = lookup_commit_reference(the_repository,\n> \t\t\t\t\t\t\t   &our->old_oid);\n> \t\t/* --branch specifies a non-branch (i.e. tags), detach HEAD */\n> \t\tupdate_ref(msg, \"HEAD\", &c->object.oid, NULL, REF_NO_DEREF,\n> \t\t\t   UPDATE_REFS_DIE_ON_ERR);\n> \t} else if (remote) {\n> +\t\tif (fallback_on_noncommit(remote, remote, msg) != 0)\n> +\t\t\treturn;\n> \t\t/*\n> \t\t * We know remote HEAD points to a non-branch, or\n> \t\t * HEAD points to a branch but we don't know which one.\n> diff --git a/refs.h b/refs.h\n> index 730d05ad91..35ee6eb058 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -497,6 +497,13 @@ enum action_on_err {\n> \tUPDATE_REFS_QUIET_ON_ERR\n> };\n>\n> +/*\n> + * In case of a remote HEAD pointing to a non-commit update_head\n> + * will make HEAD reference fallback to this value, master ref\n> + * should be safe.\n> + */\n> +#define FALLBACK_REF \"refs/heads/master\"\n> +\n> /*\n>  * Begin a reference transaction.  The reference transaction must\n>  * be freed by calling ref_transaction_free().\n> diff --git a/t/t5609-clone-branch.sh b/t/t5609-clone-branch.sh\n> index 6e7a7be052..0680cf5a89 100755\n> --- a/t/t5609-clone-branch.sh\n> +++ b/t/t5609-clone-branch.sh\n> @@ -20,7 +20,13 @@ test_expect_success 'setup' '\n> \t echo one >file && git add file && git commit -m one &&\n> \t git checkout -b two &&\n> \t echo two >file && git add file && git commit -m two &&\n> -\t git checkout master) &&\n> +\t git checkout master &&\n> +\t # Create dummy objects\n> +\t _B=$(git rev-list --objects --all | grep -m 1 \"^[^ ]\\+ [^ ]\\+\" | \\\n\nHmm. That naming convention (`_B`) is a new one, and does not really\nalign with the existing code in the test suite, does it?\n\nFurther, it seems that you pick some semi-random object from the object\ndatabase. I think you can do better: if you want to get the hash of a\nblob, use `git rev-parse HEAD:file`, if you want the hash of a tree, use\nthe same command with `HEAD:`, if you want a commit, with `HEAD`.\n\n> +\t      awk \"{print \\$1}\") &&\n> +\t echo \"${_B}\" >> .git/refs/tags/broken-tag &&\n\nIn the Git test suite, we only use curlies to expand shell variables\nwhen necessary, i.e. when followed by a valid variable name character.\n\n> +\t echo \"${_B}\" >> .git/refs/heads/broken-head\n> +\t) &&\n> \tmkdir empty &&\n> \t(cd empty && git init)\n> '\n> @@ -67,4 +73,18 @@ test_expect_success 'clone -b not allowed with empty repos'\n> '\n> \ttest_must_fail git clone -b branch empty clone-branch-empty\n> '\n>\n> +test_expect_success 'clone -b with broken tag will fallback to master' '\n> +\tgit clone -b broken-tag parent clone-broken-tag &&\n\nAs I said earlier, I think this should fail, i.e. something like\n\n\ttest_must_fail git clone ... 2>err &&\n\ttest_i18ngrep [something-from-that-error-message] err\n\nThanks,\nJohannes\n\n> +\t(cd clone-broken-tag &&\n> +\t check_HEAD master\n> +\t)\n> +'\n> +\n> +test_expect_success 'clone -b with broken head will fallback to master' '\n> +\tgit clone -b broken-head parent clone-broken-head &&\n> +\t(cd clone-broken-head &&\n> +\t check_HEAD master\n> +\t)\n> +'\n> +\n> test_done\n> --\n> 2.22.0\n>\n>\n>\n"},{"id":"385276","messageId":"20191101193558.GA1169@sigill.intra.peff.net","threadId":"52169","inReplyTo":"nycvar.QRO.7.76.6.1911012000160.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] Segmentation Fault on non-commit --branch clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-01T19:35:58Z","receivedAt":"2019-11-01T19:36:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 01, 2019 at 08:08:10PM +0100, Johannes Schindelin wrote:\n\n> > +static int fallback_on_noncommit(const struct ref *check,\n> > +\t\t\t\t const struct ref *remote,\n> > +\t\t\t\t const char *msg)\n> > +{\n> > +\tif (check == NULL)\n> > +\t\treturn 1;\n> > +\tstruct commit *c = lookup_commit_reference_gently(the_repository,\n> > +\t\t\t\t\t\t   &check->old_oid, 1);\n> > +\tif (c == NULL) {\n> > +\t\t/* Fallback HEAD to fallback refs */\n> > +\t\twarning(_(\"%s is not a valid commit object, HEAD will fallback\n> > to %s\"),\n> > +\t\t\tcheck->name, FALLBACK_REF);\n> \n> Quite honestly, I do not think that it is a good idea to fall back in\n> this case. The user asked for something that cannot be accomplished, and\n> the best way to handle this is to exit with an error, i.e. `die()`.\n\nThe main reason I proposed falling back here is that the user can\ncorrect the situation without having to redo the clone from scratch\n(which might have been very expensive). And we cannot just leave HEAD\nempty there; we have to put _something_ in it.\n\nI do think it's important, though, that we don't just fall back; we\nshould still report an error exit from the program (just as we do for\nthe similar case when clone's checkout step fails). Otherwise something\nas simple as:\n\n  git clone -b $url repo &&\n  cd repo &&\n  do_something\n\ncould have quite unexpected results.\n\nI don't know how often this would actually help users, though. It _is_ a\npretty rare situation to ask for a non-commit. So maybe it's all\nover-engineering, and we should start with just die(). If somebody comes\nalong later and wants to enhance it, it should be pretty\nstraightforward.\n\n-Peff\n"},{"id":"385277","messageId":"20191101194339.GB1169@sigill.intra.peff.net","threadId":"52169","inReplyTo":"20191101002432.GA49846@carpenter.lan","subject":"Re: [PATCH] Segmentation Fault on non-commit --branch clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-01T19:43:39Z","receivedAt":"2019-11-01T19:43:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 01, 2019 at 01:24:32AM +0100, Davide Berardi wrote:\n\n> Fixed segmentation fault that can be triggered using\n> $ git clone --branch $object $repository\n> with object pointing to a non-commit ref (e.g. a blob).\n\nThanks for working on this. I left some thoughts on the overall fallback\nscheme in the other part of the thread, and other than I agree with all\nof Dscho's review.\n\nA few other comments:\n\n> +static int fallback_on_noncommit(const struct ref *check,\n> +\t\t\t\t const struct ref *remote,\n> +\t\t\t\t const char *msg)\n> +{\n> +\tif (check == NULL)\n> +\t\treturn 1;\n\nI wondered in what circumstances \"check\" would be NULL. In the first\nconditional we pass \"our\" after checking it's non-NULL:\n\n> \tif (our && skip_prefix(our->name, \"refs/heads/\", &head)) {\n> +\t\tif (fallback_on_noncommit(our, remote, msg) != 0)\n\nand then again in the second case-arm:\n\n> \t} else if (our) {\n> +\t\tif (fallback_on_noncommit(our, remote, msg) != 0)\n\nand then in the third we pass remote after checking that it's not NULL:\n\n> \t} else if (remote) {\n> +\t\tif (fallback_on_noncommit(remote, remote, msg) != 0)\n> +\t\t\treturn;\n\nSo I think this NULL check can go away. In general I don't mind\nfunctions being defensive, but it's hard to decide whether it's doing\nthe right thing since it's not a case we think can come up (it could be\nmarked with a BUG() assertion, but IMHO it's not worth it; most\nfunctions require their arguments to be non-NULL, so checking it would\nbe unusual in our code base).\n\n> +static int fallback_on_noncommit(const struct ref *check,\n> +\t\t\t\t const struct ref *remote,\n> +\t\t\t\t const char *msg)\n> [...]\n> +\treturn c == NULL;\n\nThe return value for this function is unusual for our code base. If it's\njust an error return, we'd usually use \"0\" for success and a negative\nvalue for errors (i.e., mimicking system calls).\n\n> diff --git a/refs.h b/refs.h\n> index 730d05ad91..35ee6eb058 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -497,6 +497,13 @@ enum action_on_err {\n> \tUPDATE_REFS_QUIET_ON_ERR\n> };\n> \n> +/*\n> + * In case of a remote HEAD pointing to a non-commit update_head\n> + * will make HEAD reference fallback to this value, master ref\n> + * should be safe.\n> + */\n> +#define FALLBACK_REF \"refs/heads/master\"\n> +\n> /*\n\nSince this is only used in one spot, I think it's better to keep it\nlocalized to that function. E.g., with:\n\n  static const char fallback_ref[] = \"refs/heads/master\";\n\nThat way it's clear that no other code depends on it.\n\n-Peff\n"},{"id":"385311","messageId":"xmqqtv7m1vrf.fsf@gitster-ct.c.googlers.com","threadId":"52169","inReplyTo":"nycvar.QRO.7.76.6.1911012000160.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] Segmentation Fault on non-commit --branch clone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-02T09:18:44Z","receivedAt":"2019-11-02T09:24:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Hi Davide,\n>\n> I wonder whether you might want to reword the Subject: such that it\n> reflects what the patch does instead of what the problem is that\n> motivated you to work on the patch (the patch does not cause the\n> segmentation fault, after all, but it fixes it).\n\nGood point.\n\n    Subject: clone: do not segfault on \"clone -b B\" where B is a non-commit\n\n    The code in \"git clone -b B\" to decide what branch to check out\n    assumed that B points at a commit object without checking,\n    leading to dereferencing a NULL pointer and causing a segfault.\n\n    Just aborting the operation when it happens is not a very\n    attractive option because we would be left with a directory\n    without .git/HEAD that cannot be used as a valid repository the\n    user can attempt to recover from the situation by checking out\n    something.\n\n    Fall back to use the 'master' branch, which is what we use when\n    the command without the \"-b\" option cannot figure out what\n    branch the remote side points with its HEAD.\n\nor something like that, perhaps?\n\nI am not sure if the existing code is careful enough setting up the\nresulting local 'master' branch, or needs more changes associated\nwith this patch, though.  For example, does it want to be set to\nintegrate with the 'master' branch on the remote side by setting\n\"branch.master.remote\" and \"branch.master.merge\" configuration, or\ndo we want to turn them off?  I _think_ the answer is that we want\nto behave as if the user said \"-b master\" instead of \"-b B\" (with B\nthat does not point at a commit), but I am not sure.  Also, don't we\ntry to be a bit noisier when the fallback fails?  For example, if\nthe user said \"clone -b master\" and the 'master' points at an object\nthat is not a commit, falling back and writing refs/heads/master in\nthe HEAD would leave us in the same position as we did not have any\nfallback.\n\nI skimmed your review and I think I agree with most (if not all) of\nthem.  Thanks.\n"},{"id":"385312","messageId":"xmqqlfsy1tii.fsf@gitster-ct.c.googlers.com","threadId":"52169","inReplyTo":"20191101002432.GA49846@carpenter.lan","subject":"Re: [PATCH] Segmentation Fault on non-commit --branch clone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-02T10:07:17Z","receivedAt":"2019-11-02T10:07:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Davide Berardi <berardi.dav@gmail.com> writes:\n\n> +static int fallback_on_noncommit(const struct ref *check,\n> +\t\t\t\t const struct ref *remote,\n> +\t\t\t\t const char *msg)\n> +{\n> +\tif (check == NULL)\n> +\t\treturn 1;\n> +\tstruct commit *c = lookup_commit_reference_gently(the_repository,\n> +\t\t\t\t\t\t   &check->old_oid, 1);\n\nThis is decl-after-stmt.  Can check be NULL, though?  IOW, the first\ntwo lines in this function should go.\n\n> +\tif (c == NULL) {\n\nWe tend to say \"if (!c) {\" instead.\n\n> +\t\t/* Fallback HEAD to fallback refs */\n\nYou are falling back to just a single ref (i.e. s/refs/ref/) but\nmore importantly, what the code is doing is obvious enough without\nthis comment.  What we want commenting on is _why_ we do this.\n\n\t/*\n\t * The ref specified to be checked out does not point at a\n\t * commit so pointing HEAD at it will leave us a broken\n         * repository.  But we need to have _something_ plausible\n\t * in HEAD---otherwise the result would not be a repository.\n\t */\n\nwould explain why we point HEAD to the default 'master' branch.\n\n> +\t\twarning(_(\"%s is not a valid commit object, HEAD will fallback to %s\"),\n> +\t\t\tcheck->name, FALLBACK_REF);\n> +\t\tupdate_ref(msg, FALLBACK_REF, &remote->old_oid, NULL,\n> +\t\t\t   REF_NO_DEREF, UPDATE_REFS_DIE_ON_ERR);\n\nHaving said that, I was hoping that this would use the help from the\nguess_remote_head() like the case without \"-b\" option does, so that\nwe do not have to use a hardcoded default.\n\n> +\t}\n> +\n> +\treturn c == NULL;\n\nOur API convention is 0 for success and negavive for failure.\n\n> +}\n> +\n> static void update_head(const struct ref *our, const struct ref *remote,\n> \t\t\tconst char *msg)\n> {\n> \tconst char *head;\n> \tif (our && skip_prefix(our->name, \"refs/heads/\", &head)) {\n> +\t\tif (fallback_on_noncommit(our, remote, msg) != 0)\n> +\t\t\treturn;\n> \t\t/* Local default branch link */\n> \t\tif (create_symref(\"HEAD\", our->name, NULL) < 0)\n> \t\t\tdie(_(\"unable to update HEAD\"));\n> @@ -718,12 +739,17 @@ static void update_head(const str\n\nHere in the patch context is a \"do this in a non-bare repository\"\nguard, and crucially, we do this:\n\n> \t\t\tinstall_branch_config(0, head, option_origin, our->name);\n\nThat is, we add configuration for our->name (which is now\n\"refs/heads/master\"), but I do not think we updated any of the other\nfield in *our to make the world a consistent place.  Is the object\npointed at by our local refs/heads/master in a reasonable\nrelationship with the object at the tip of the 'master' branch at\nthe remote site, or is can totally be unrelated because we made no\nattempt to make our->old_oid or whatever field consistent with the\n\"corrected\" reality?\n\nGiven the potential complication, and given that we are doing a\ncorretive action only so that we leave some repository the user can\nfix manually, I am inclined to say that we should avoid falling into\nthis codepath.  I'll wonder about a totally different approach later\nin this message that makes the fallback_on_noncommit() helper and\nchange to these existing parts of the update_head() function\nunnecessary.\n\n> \t\t}\n> \t} else if (our) {\n> +\t\tif (fallback_on_noncommit(our, remote, msg) != 0)\n> +\t\t\treturn;\n> +\t\t/* here commit is valid */\n> \t\tstruct commit *c = lookup_commit_reference(the_repository,\n> \t\t\t\t\t\t\t   &our->old_oid);\n\nWhat makes us certain that commit is valid?  our->old_oid is not\nadjusted by the fallback_on_noncommit() function, but we did check\nif it is a commit by doing the exact same lookup_commit_reference()\nin there already, and we know it found a commit (otherwise the\nfunction would have returned a non-zero to signal an error).\n\nBut it also means that we are making a redundant and unnecessary\ncall if the code is structured better.\n\nThis makes me wonder why we are not adding a single call to a helper\nfunction at the beginning of the function, something like\n\n\tconst struct oid *tip = NULL;\n\tstruct commit *tip_commit = NULL;\n\n\tif (our)\n\t\ttip = &our->old_oid;\n\telse if (remote)\n\t\ttip = &remote->old_oid;\n\n\tif (!tip)\n\t\treturn;\n\n\ttip_commit = lookup_commit_reference_gently(the_repository, tip);\n\nThen, if !tip_commit, we know we need to fall back to something.  I\nactually think it would probably be cleanest if we added\n\n\tif (!tip_commit) {\n\t\t/*\n\t\t * The given non-commit cannot be checked out,\n                 * so have a 'master' branch and leave it unborn.\n                 */\n\t\twarning(_\"non-commit cannot be checked out\");\n\t\tcreate_symref(\"HEAD\", \"refs/heads/master\", msg);\n\t\treturn;\n\t}\n\nThat is, we always check out an unborn 'master' branch (which would\nyield another warning at the beginning of checkout() function, which\nis what we want) doing minimum damage, without even configuring any\nremote tracking information.\n\nThe rest of the update_head() function could be left as-is, but with\nthe \"see what we would be checking out first\" approach, we probably\ncan lose some code (like the call to lookup_commit_reference() in\nthe \"detached HEAD\" case), without adding any extra logic.\n\n> \t} else if (remote) {\n> +\t\tif (fallback_on_noncommit(remote, remote, msg) != 0)\n> +\t\t\treturn;\n> \t\t/*\n> \t\t * We know remote HEAD points to a non-branch, or\n> \t\t * HEAD points to a branch but we don't know which one.\n\nThanks.\n"},{"id":"385313","messageId":"xmqqh83m1t3c.fsf@gitster-ct.c.googlers.com","threadId":"52169","inReplyTo":"20191101193558.GA1169@sigill.intra.peff.net","subject":"Re: [PATCH] Segmentation Fault on non-commit --branch clone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-02T10:16:23Z","receivedAt":"2019-11-02T10:16:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I don't know how often this would actually help users, though. It _is_ a\n> pretty rare situation to ask for a non-commit. So maybe it's all\n> over-engineering, and we should start with just die(). If somebody comes\n> along later and wants to enhance it, it should be pretty\n> straightforward.\n\nI like that; after update_head() finishes, there are a few clean-up\nthings that the caller wants to do besides a checkout() call, but if\nwe make update_head() return a failure, perhaps the caller side\nchange would be as small as the attached patch.  That would go nicely\nwith the \"make the result just barely usable\" approach of leaving an\nunborn master branch I suggested in a separate message, I would think.\n\n builtin/clone.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex c46ee29f0a..fa0558fa3e 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1246,7 +1246,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\t   branch_top.buf, reflog_msg.buf, transport,\n \t\t\t   !is_local, filter_options.choice);\n \n-\tupdate_head(our_head_points_at, remote_head, reflog_msg.buf);\n+\terr = update_head(our_head_points_at, remote_head, reflog_msg.buf) < 0;\n \n \t/*\n \t * We want to show progress for recursive submodule clones iff\n@@ -1265,8 +1265,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t}\n \n \tjunk_mode = JUNK_LEAVE_REPO;\n-\tfetch_if_missing = 1;\n-\terr = checkout(submodule_progress);\n+\tif (!err) {\n+\t\tfetch_if_missing = 1;\n+\t\terr = checkout(submodule_progress);\n+\t}\n \n \tstrbuf_release(&reflog_msg);\n \tstrbuf_release(&branch_top);\n\n\n\n"},{"id":"385346","messageId":"20191103180716.GA72007@carpenter.lan","threadId":"52169","inReplyTo":"20191101002432.GA49846@carpenter.lan","subject":"[PATCH v2] clone: Don't segfault on -b specifing a non-commit","fromName":"Davide Berardi","fromEmail":"davide.berardi6@unibo.it","sentAt":"2019-11-03T18:07:18Z","receivedAt":"2019-11-03T18:07:27Z","isPatch":true,"sender":{"key":"davide.berardi6@unibo.it","avatar":null},"body":"The code in \"git clone -b B\" to decide what branch to check out\nassumed that B points at a commit object without checking,\nleading to dereferencing a NULL pointer and causing a segmentation\nfault.\n\nJust aborting the operation when it happens is not a very\nattractive option because we would be left with a directory\nwithout .git/HEAD that cannot be used as a valid repository the\nuser can attempt to recover from the situation by checking out\nsomething.\n\nFall back to use the 'master' branch, which is what we use when\nthe command without the \"-b\" option cannot figure out what\nbranch the remote side points with its HEAD.\n\nSigned-off-by: Davide Berardi <berardi.dav@gmail.com>\n---\n builtin/clone.c         | 49 ++++++++++++++++++++++++++++++++++++-----\n commit.c                | 16 +++++++++++---\n commit.h                |  4 ++++\n t/t5609-clone-branch.sh | 16 +++++++++++++-\n 4 files changed, 75 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex f665b28ccc..f185b7f193 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -704,10 +704,46 @@ static void update_remote_refs(const struct ref *refs,\n \t}\n }\n \n-static void update_head(const struct ref *our, const struct ref *remote,\n+static struct commit *lookup_commit_helper(const struct ref *our,\n+\t\t\t\t\t   const struct ref *remote,\n+\t\t\t\t\t   const char *msg, int *err)\n+{\n+\tconst struct object_id *tip = NULL;\n+\tstruct commit *tip_commit = NULL;\n+\n+\tif (our)\n+\t\ttip = &our->old_oid;\n+\telse if (remote)\n+\t\ttip = &remote->old_oid;\n+\n+\tif (!tip)\n+\t\treturn NULL;\n+\n+\ttip_commit = lookup_commit_reference_gently_err(the_repository, tip, 1, err);\n+\tif (!tip_commit) {\n+\t\t/*\n+\t\t * The given non-commit cannot be checked out,\n+\t\t * so have a 'master' branch and leave it unborn.\n+\t\t */\n+\t\twarning(_(\"non-commit cannot be checked out\"));\n+\t\tcreate_symref(\"HEAD\", \"refs/heads/master\", msg);\n+\t\treturn NULL;\n+\t}\n+\n+\treturn tip_commit;\n+}\n+\n+static int update_head(const struct ref *our, const struct ref *remote,\n \t\t\tconst char *msg)\n {\n+\tint err = 0;\n \tconst char *head;\n+\tstruct commit *c = NULL;\n+\n+\tc = lookup_commit_helper(our, remote, msg, &err);\n+\tif (err)\n+\t\treturn -1;\n+\n \tif (our && skip_prefix(our->name, \"refs/heads/\", &head)) {\n \t\t/* Local default branch link */\n \t\tif (create_symref(\"HEAD\", our->name, NULL) < 0)\n@@ -718,8 +754,6 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \t\t\tinstall_branch_config(0, head, option_origin, our->name);\n \t\t}\n \t} else if (our) {\n-\t\tstruct commit *c = lookup_commit_reference(the_repository,\n-\t\t\t\t\t\t\t   &our->old_oid);\n \t\t/* --branch specifies a non-branch (i.e. tags), detach HEAD */\n \t\tupdate_ref(msg, \"HEAD\", &c->object.oid, NULL, REF_NO_DEREF,\n \t\t\t   UPDATE_REFS_DIE_ON_ERR);\n@@ -732,6 +766,7 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \t\tupdate_ref(msg, \"HEAD\", &remote->old_oid, NULL, REF_NO_DEREF,\n \t\t\t   UPDATE_REFS_DIE_ON_ERR);\n \t}\n+\treturn err;\n }\n \n static int checkout(int submodule_progress)\n@@ -1249,7 +1284,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\t   branch_top.buf, reflog_msg.buf, transport,\n \t\t\t   !is_local, filter_options.choice);\n \n-\tupdate_head(our_head_points_at, remote_head, reflog_msg.buf);\n+\terr = update_head(our_head_points_at, remote_head, reflog_msg.buf) < 0;\n \n \t/*\n \t * We want to show progress for recursive submodule clones iff\n@@ -1268,8 +1303,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t}\n \n \tjunk_mode = JUNK_LEAVE_REPO;\n-\tfetch_if_missing = 1;\n-\terr = checkout(submodule_progress);\n+\tif (!err) {\n+\t\tfetch_if_missing = 1;\n+\t\terr = checkout(submodule_progress);\n+\t}\n \n \tstrbuf_release(&reflog_msg);\n \tstrbuf_release(&branch_top);\ndiff --git a/commit.c b/commit.c\nindex a98de16e3d..ffb5230f0f 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -26,16 +26,26 @@ int save_commit_buffer = 1;\n \n const char *commit_type = \"commit\";\n \n-struct commit *lookup_commit_reference_gently(struct repository *r,\n-\t\tconst struct object_id *oid, int quiet)\n+struct commit *lookup_commit_reference_gently_err(struct repository *r,\n+\t\tconst struct object_id *oid, int quiet, int *err)\n {\n+\tstruct commit *retval;\n \tstruct object *obj = deref_tag(r,\n \t\t\t\t       parse_object(r, oid),\n \t\t\t\t       NULL, 0);\n \n \tif (!obj)\n \t\treturn NULL;\n-\treturn object_as_type(r, obj, OBJ_COMMIT, quiet);\n+\tretval = object_as_type(r, obj, OBJ_COMMIT, quiet);\n+\tif (!retval && err)\n+\t\t*err = 1;\n+\treturn retval;\n+}\n+\n+struct commit *lookup_commit_reference_gently(struct repository *r,\n+\t\tconst struct object_id *oid, int quiet)\n+{\n+\treturn lookup_commit_reference_gently_err(r, oid, quiet, NULL);\n }\n \n struct commit *lookup_commit_reference(struct repository *r, const struct object_id *oid)\ndiff --git a/commit.h b/commit.h\nindex f5295ca7f3..157c5dc526 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -70,6 +70,10 @@ struct commit *lookup_commit_reference(struct repository *r,\n struct commit *lookup_commit_reference_gently(struct repository *r,\n \t\t\t\t\t      const struct object_id *oid,\n \t\t\t\t\t      int quiet);\n+struct commit *lookup_commit_reference_gently_err(struct repository *r,\n+\t\t\t\t\t\t  const struct object_id *oid,\n+\t\t\t\t\t\t  int quiet,\n+\t\t\t\t\t\t  int *err);\n struct commit *lookup_commit_reference_by_name(const char *name);\n \n /*\ndiff --git a/t/t5609-clone-branch.sh b/t/t5609-clone-branch.sh\nindex 6e7a7be052..d57f750eeb 100755\n--- a/t/t5609-clone-branch.sh\n+++ b/t/t5609-clone-branch.sh\n@@ -20,7 +20,10 @@ test_expect_success 'setup' '\n \t echo one >file && git add file && git commit -m one &&\n \t git checkout -b two &&\n \t echo two >file && git add file && git commit -m two &&\n-\t git checkout master) &&\n+\t git checkout master &&\n+\t blob=$(git rev-parse HEAD:file)  &&\n+\t echo $blob > .git/refs/heads/broken-tag &&\n+\t echo $blob > .git/refs/heads/broken-head) &&\n \tmkdir empty &&\n \t(cd empty && git init)\n '\n@@ -67,4 +70,15 @@ test_expect_success 'clone -b not allowed with empty repos' '\n \ttest_must_fail git clone -b branch empty clone-branch-empty\n '\n \n+test_expect_success 'clone -b with a non-commit tag must fallback' '\n+\ttest_must_fail git clone -b broken-tag parent clone-broken-tag &&\n+\t(cd clone-broken-tag &&\n+\t check_HEAD master)\n+'\n+test_expect_success 'clone -b with a non-commit head must fallback' '\n+\ttest_must_fail git clone -b broken-head parent clone-broken-head &&\n+\t(cd clone-broken-head &&\n+\t check_HEAD master)\n+'\n+\n test_done\n-- \n2.22.0\n\n"},{"id":"385347","messageId":"20191103181657.GA89185@carpenter.lan","threadId":"52169","inReplyTo":"xmqqh83m1t3c.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] Segmentation Fault on non-commit --branch clone","fromName":"Davide Berardi","fromEmail":"berardi.dav@gmail.com","sentAt":"2019-11-03T18:16:57Z","receivedAt":"2019-11-03T18:17:03Z","isPatch":true,"sender":{"key":"berardi.dav@gmail.com","avatar":"https://gravatar.com/avatar/5d15ae87f80da0830f2d7e86ba3a6553047a1447ca625f1fc29082632e9a7b13?d=mp&s=160"},"body":"On Sat, Nov 02, 2019 at 07:16:23PM +0900, Junio C Hamano wrote:\n>Jeff King <peff@peff.net> writes:\n>\n>> I don't know how often this would actually help users, though. It _is_ a\n>> pretty rare situation to ask for a non-commit. So maybe it's all\n>> over-engineering, and we should start with just die(). If somebody comes\n>> along later and wants to enhance it, it should be pretty\n>> straightforward.\n>\n>I like that; after update_head() finishes, there are a few clean-up\n>things that the caller wants to do besides a checkout() call, but if\n>we make update_head() return a failure, perhaps the caller side\n>change would be as small as the attached patch.  That would go nicely\n>with the \"make the result just barely usable\" approach of leaving an\n>unborn master branch I suggested in a separate message, I would think.\n>\nThank you all for your precious comments, I've tried to implement\nyour suggestions and I've sent the patch here.\n\nThe problem with the proposed approach was that the code was\nincompatible with some tests (specifically the tests which specifies an\nempty .git directory would fail and fallback to the unborn master\nbranch).\n\nThe lookup commit have two error-paths:\n\n1. the commit cannot be found;\n2. the commit is found and cannot be casted to a commit (whoops!).\n\nso, I've returned the second condition using an auxiliary variable and\ndeclaring a new lookup_commit function keeping compatibility with the\nold one.\n\nI'm sorry for my errors but I'm far for an expert of git internals,\nthank you (all) for your time and kindness.\n\nciao,\nD.\n"},{"id":"385359","messageId":"xmqq36f41ejm.fsf@gitster-ct.c.googlers.com","threadId":"52169","inReplyTo":"20191103181657.GA89185@carpenter.lan","subject":"Re: [PATCH] Segmentation Fault on non-commit --branch clone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-04T03:55:09Z","receivedAt":"2019-11-04T03:55:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Davide Berardi <berardi.dav@gmail.com> writes:\n\n> The problem with the proposed approach was that the code was\n> incompatible with some tests (specifically the tests which specifies an\n> empty .git directory would fail and fallback to the unborn master\n> branch).\n>\n> The lookup commit have two error-paths:\n>\n> 1. the commit cannot be found;\n> 2. the commit is found and cannot be casted to a commit (whoops!).\n>\n> so, I've returned the second condition using an auxiliary variable and\n> declaring a new lookup_commit function keeping compatibility with the\n> old one.\n\nIt's more like three, I think.\n\n  1a. The given object name is a sentinel, \"no such object\", value.\n\n  1b. The given object name is meant to name a real object, but\n      there is no object with such a name in the repository.\n\n  2.  The given object name names an existing object, but it does\n      not peel to a commit.\n\nTraditionally, we had only lookup_commit() and died on any of the\nabove.  Later, we added the _gently() variant for callers that want\nto use a commit and also want to handle an error case where the\nobject name they are handed by their callers does not peel to a\ncommit.\n\nIn the \"unborn repository, empty .git\" case, are you getting a\nrandom object name, or null_oid (aka case 1a. above)?  If that is\nthe case, then your solution to introduce another variant of\nlookup_commit() that takes *err parameter is a wrong approach would\nnot differenciate between 1a. and 1b., which would not help, as I\nsuspect that we still do want to treat 1b. as an error.\n\nWouldn't it be cleaner to catch 1a. upfront, e.g.\n\n\tif (our)\n\t\ttip = &our->old_oid;\n\telse if (remote)\n\t\ttip = &remote->old_oid;\n\telse\n\t\treturn NULL;\n\n\tif (is_null_oid(tip))\n        \treturn NULL;\n\n\ttip_commit = lookup_commit_reference_gently(the_repository, tip, 1);\n\tif (!tip_commit) {\n\t\twarning(...);\n\t\tcreate_symref(...);\n\t\treturn NULL;\n\t}\n\nI am not offhand sure if the places we return NULL upfront want to\nalso create HEAD symref, or that is something automatically happens\nfor us in the downstream of this function, though.\n\nThanks.\n"},{"id":"385443","messageId":"20191105043749.GA27601@sigill.intra.peff.net","threadId":"52169","inReplyTo":"20191103180716.GA72007@carpenter.lan","subject":"Re: [PATCH v2] clone: Don't segfault on -b specifing a non-commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-05T04:37:49Z","receivedAt":"2019-11-05T04:37:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 03, 2019 at 06:07:18PM +0000, Davide Berardi wrote:\n\n> -static void update_head(const struct ref *our, const struct ref *remote,\n> +static struct commit *lookup_commit_helper(const struct ref *our,\n> +\t\t\t\t\t   const struct ref *remote,\n> +\t\t\t\t\t   const char *msg, int *err)\n> +{\n> +\tconst struct object_id *tip = NULL;\n> +\tstruct commit *tip_commit = NULL;\n> +\n> +\tif (our)\n> +\t\ttip = &our->old_oid;\n> +\telse if (remote)\n> +\t\ttip = &remote->old_oid;\n> +\n> +\tif (!tip)\n> +\t\treturn NULL;\n> +\n> +\ttip_commit = lookup_commit_reference_gently_err(the_repository, tip, 1, err);\n> +\tif (!tip_commit) {\n> +\t\t/*\n> +\t\t * The given non-commit cannot be checked out,\n> +\t\t * so have a 'master' branch and leave it unborn.\n> +\t\t */\n> +\t\twarning(_(\"non-commit cannot be checked out\"));\n> +\t\tcreate_symref(\"HEAD\", \"refs/heads/master\", msg);\n> +\t\treturn NULL;\n> +\t}\n> +\n> +\treturn tip_commit;\n> +}\n\nI like the logic flow in this function, which is IMHO clearer than the\nexisting code. But the \"err\" thing puzzled me for a moment. I think you\nare trying to tell the difference between the case that both \"our\" and\n\"remote\" are NULL, and the case that we saw a non-commit. In either case\nwe return NULL, but only one is an error.\n\nBut:\n\n  - I don't think that logic needs to extend down into\n    lookup_commit_reference_gently(); a NULL return from it would always\n    be an error, wouldn't it?\n\n  - we could probably simplify this by just inlining it into\n    update_head(). Something like:\n\n      if (our)\n              tip = &our->old_oid;\n      else if (remote)\n              tip = &remote->old_oid;\n\n      if (!tip) {\n\t      /*\n\t       * We have no local branch requested with \"-b\", and the\n\t       * remote HEAD is unborn. There's nothing to update HEAD\n\t       * to, but this state is not an error.\n\t       */\n              return 0;\n      }\n\n      tip_commit = lookup_commit_reference_gently(...);\n      if (!tip_commit) {\n              /*\n\t       * The given non-commit cannot be checked out, etc...\n\t       */\n\t      warning(...);\n\t      create_symref(...);\n\t      return -1;\n      }\n\n      ...and then existing code to use tip_commit...\n\n      /*\n       * we'd always return 0 here, because our update_ref calls die on\n       * error\n       */\n      return 0;\n\n> @@ -1268,8 +1303,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \t}\n>  \n>  \tjunk_mode = JUNK_LEAVE_REPO;\n> -\tfetch_if_missing = 1;\n> -\terr = checkout(submodule_progress);\n> +\tif (!err) {\n> +\t\tfetch_if_missing = 1;\n> +\t\terr = checkout(submodule_progress);\n> +\t}\n\nThis part makes sense. We might want an explanatory comment along the\nlines of:\n\n  /*\n   * Only try to checkout if we successfully updated HEAD; otherwise\n   * HEAD isn't pointing to the thing the user wanted.\n   /\n   if (!err) {\n       ...\n   \n\n> -struct commit *lookup_commit_reference_gently(struct repository *r,\n> -\t\tconst struct object_id *oid, int quiet)\n> +struct commit *lookup_commit_reference_gently_err(struct repository *r,\n> +\t\tconst struct object_id *oid, int quiet, int *err)\n\nAnd this part I think could just go away, if you take my suggestion\nabove.\n\n> diff --git a/t/t5609-clone-branch.sh b/t/t5609-clone-branch.sh\n> index 6e7a7be052..d57f750eeb 100755\n> --- a/t/t5609-clone-branch.sh\n> +++ b/t/t5609-clone-branch.sh\n> @@ -20,7 +20,10 @@ test_expect_success 'setup' '\n>  \t echo one >file && git add file && git commit -m one &&\n>  \t git checkout -b two &&\n>  \t echo two >file && git add file && git commit -m two &&\n> -\t git checkout master) &&\n> +\t git checkout master &&\n> +\t blob=$(git rev-parse HEAD:file)  &&\n> +\t echo $blob > .git/refs/heads/broken-tag &&\n> +\t echo $blob > .git/refs/heads/broken-head) &&\n\nMinor style nit, but we usually avoid the space after \">\".\n\n> +test_expect_success 'clone -b with a non-commit tag must fallback' '\n> +\ttest_must_fail git clone -b broken-tag parent clone-broken-tag &&\n> +\t(cd clone-broken-tag &&\n> +\t check_HEAD master)\n> +'\n> +test_expect_success 'clone -b with a non-commit head must fallback' '\n> +\ttest_must_fail git clone -b broken-head parent clone-broken-head &&\n> +\t(cd clone-broken-head &&\n> +\t check_HEAD master)\n> +'\n\nOK, this second one covers the first conditional from update_head():\n\n  if (our && skip_prefix(our->name, \"refs/heads/\", &head)) {\n\nand the first one covers the second conditional:\n\n  } else if (our) {\n\nShould we cover the third conditional, too?\n\nI think it would be the case that the remote HEAD is pointing to a\nnon-commit (since that's a corrupt repository, it might make sense\ncreate a separate sub-repository).\n\n-Peff\n"},{"id":"385506","messageId":"xmqqbltpyeel.fsf@gitster-ct.c.googlers.com","threadId":"52169","inReplyTo":"20191105043749.GA27601@sigill.intra.peff.net","subject":"Re: [PATCH v2] clone: Don't segfault on -b specifing a non-commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-06T01:36:18Z","receivedAt":"2019-11-06T01:36:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I like the logic flow in this function, which is IMHO clearer than the\n> existing code. But the \"err\" thing puzzled me for a moment. I think you\n> are trying to tell the difference between the case that both \"our\" and\n> \"remote\" are NULL, and the case that we saw a non-commit. In either case\n> we return NULL, but only one is an error.\n\nYup.\n\n> But:\n>\n>   - I don't think that logic needs to extend down into\n>     lookup_commit_reference_gently(); a NULL return from it would always\n>     be an error, wouldn't it?\n\nYes.\n\n>   - we could probably simplify this by just inlining it into\n>     update_head(). Something like:\n>\n>       if (our)\n>               tip = &our->old_oid;\n>       else if (remote)\n>               tip = &remote->old_oid;\n>\n>       if (!tip) {\n> \t      /*\n> \t       * We have no local branch requested with \"-b\", and the\n> \t       * remote HEAD is unborn. There's nothing to update HEAD\n> \t       * to, but this state is not an error.\n> \t       */\n>               return 0;\n>       }\n\nI somehow had an impression that Davide was protecting against the\ncase where tip->old_oid is null_oid (cloning from an empty repo?);\nNULL return from lookup_commit_reference_gently(null_oid) would not\ndeserve a warning from this codepath, and should work just like the\nway it has worked before these changes.\n\n>       tip_commit = lookup_commit_reference_gently(...);\n>       if (!tip_commit) {\n>               /*\n>> -\tfetch_if_missing = 1;\n>> -\terr = checkout(submodule_progress);\n>> +\tif (!err) {\n>> +\t\tfetch_if_missing = 1;\n>> +\t\terr = checkout(submodule_progress);\n>> +\t}\n>\n> This part makes sense. We might want an explanatory comment along the\n> lines of:\n>\n>   /*\n>    * Only try to checkout if we successfully updated HEAD; otherwise\n>    * HEAD isn't pointing to the thing the user wanted.\n>    /\n>    if (!err) {\n>        ...\n\nYup.\n\n>> diff --git a/t/t5609-clone-branch.sh b/t/t5609-clone-branch.sh\n>\n> I think it would be the case that the remote HEAD is pointing to a\n> non-commit (since that's a corrupt repository, it might make sense\n> create a separate sub-repository).\n\nAll good comments. \n\nThanks\n"},{"id":"385524","messageId":"20191106040523.GB4307@sigill.intra.peff.net","threadId":"52169","inReplyTo":"xmqqbltpyeel.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] clone: Don't segfault on -b specifing a non-commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-06T04:05:23Z","receivedAt":"2019-11-06T04:05:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 06, 2019 at 10:36:18AM +0900, Junio C Hamano wrote:\n\n> >   - we could probably simplify this by just inlining it into\n> >     update_head(). Something like:\n> >\n> >       if (our)\n> >               tip = &our->old_oid;\n> >       else if (remote)\n> >               tip = &remote->old_oid;\n> >\n> >       if (!tip) {\n> > \t      /*\n> > \t       * We have no local branch requested with \"-b\", and the\n> > \t       * remote HEAD is unborn. There's nothing to update HEAD\n> > \t       * to, but this state is not an error.\n> > \t       */\n> >               return 0;\n> >       }\n> \n> I somehow had an impression that Davide was protecting against the\n> case where tip->old_oid is null_oid (cloning from an empty repo?);\n> NULL return from lookup_commit_reference_gently(null_oid) would not\n> deserve a warning from this codepath, and should work just like the\n> way it has worked before these changes.\n\nI'm not sure if we need to worry about that case. Right now it would\njust segfault, I think, which means it's not something that comes up\nregularly (though whether the _best_ thing might be to consider a\nnon-error, I don't know without understanding why we'd see a null_oid in\nthe first place).\n\nIf we did want to handle it, though, I think it would be easy with the\nsetup I described; update_head() could check is_null_oid() itself.\n\n-Peff\n"},{"id":"385553","messageId":"xmqqo8xptjpd.fsf@gitster-ct.c.googlers.com","threadId":"52169","inReplyTo":"20191106040523.GB4307@sigill.intra.peff.net","subject":"Re: [PATCH v2] clone: Don't segfault on -b specifing a non-commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-06T09:53:02Z","receivedAt":"2019-11-06T09:53:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> If we did want to handle it, though, I think it would be easy with the\n> setup I described; update_head() could check is_null_oid() itself.\n\nYup, that was what I had in my review on the previous round ;-)\n"}]}