{"thread":{"id":"54537","subject":"[PATCH] Fix potential segfault on cloning invalid tag","startedAt":"2020-10-30T11:46:46Z","lastAt":"2020-11-04T19:31:28Z","messageCount":4,"participants":["Sohom Datta via GitGitGadget","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"408734","messageId":"pull.906.git.git.1604058401991.gitgitgadget@gmail.com","threadId":"54537","inReplyTo":null,"subject":"[PATCH] Fix potential segfault on cloning invalid tag","fromName":"Sohom Datta via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-30T11:46:41Z","receivedAt":"2020-10-30T11:46:46Z","isPatch":true,"sender":{"key":"name:Sohom Datta","avatar":null},"body":"From: Sohom Datta <sohom.datta@learner.manipal.edu>\n\nGit allows users to create tags pointing to object hashes\nthat may or may not be commits. When a tag that doesn't\npoint to a commit is used with the -b (--branch) parameter\nof git clone, git segfaults as it assumes that the tag will\nalways reference a commit.\n\nAdd a check to make sure that lookup_commit_reference returns a commit\nbefore detaching HEAD.\n\nSigned-off-by: Sohom Datta <sohom.datta@learner.manipal.edu>\n---\n    Fix potential segfault on cloning invalid tag\n    \n    The bug can be reproduced by running git tag 1.4.0 $(git rev-parse\n    :filename) on the parent repository and then cloning the repo using git\n    clone --branch 1.4.0 file://path/to/repo. The output should be something\n    along the lines of:\n    \n    Cloning into '<path/to/repo>'...\n    remote: Enumerating objects: 8, done.\n    remote: Counting objects: 100% (8/8), done.\n    remote: Compressing objects: 100% (5/5), done.\n    remote: Total 8 (delta 1), reused 0 (delta 0), pack-reused 0\n    Receiving objects: 100% (8/8), done.\n    Resolving deltas: 100% (1/1), done.\n    error: object d670460b4b4aece5915caf5c68d12f560a9fe3e4 is a blob, not a commit\n    zsh: segmentation fault (core dumped)\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-906%2Fsohomdatta1%2Fsegfault-while-cloning-invalid-tag-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-906/sohomdatta1/segfault-while-cloning-invalid-tag-v1\nPull-Request: https://github.com/git/git/pull/906\n\n builtin/clone.c           | 2 ++\n t/t5610-clone-detached.sh | 5 +++++\n 2 files changed, 7 insertions(+)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex a0841923cf..b4760ac887 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -727,6 +727,8 @@ static void update_head(const struct ref *our, const struct ref *remote,\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\tif ( !c )\n+\t\t\tdie(_(\"%s does not point to a commit.\"), our->name);\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);\ndiff --git a/t/t5610-clone-detached.sh b/t/t5610-clone-detached.sh\nindex 8b0d607df1..c7fd2c5f5c 100755\n--- a/t/t5610-clone-detached.sh\n+++ b/t/t5610-clone-detached.sh\n@@ -15,6 +15,7 @@ test_expect_success 'setup' '\n \techo two >file &&\n \tgit commit -a -m two &&\n \tgit tag two &&\n+\tgit tag four $(git rev-parse :file) &&\n \techo three >file &&\n \tgit commit -a -m three\n '\n@@ -72,5 +73,9 @@ test_expect_success 'cloned HEAD matches' '\n test_expect_success 'cloned HEAD is detached' '\n \thead_is_detached detached-orphan\n '\n+test_expect_success 'cloning invalid tag' '\n+\ttest_must_fail git clone \"file://$PWD\" -b four 2>err &&\n+\ttest_i18ngrep \"does not point to a commit.\" err\n+'\n \n test_done\n\nbase-commit: ad27df6a5cff694add500ab8c7f97234feb4a91f\n-- \ngitgitgadget\n"},{"id":"408751","messageId":"20201030150942.GB3282152@coredump.intra.peff.net","threadId":"54537","inReplyTo":"pull.906.git.git.1604058401991.gitgitgadget@gmail.com","subject":"Re: [PATCH] Fix potential segfault on cloning invalid tag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-30T15:09:42Z","receivedAt":"2020-10-30T15:09:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 30, 2020 at 11:46:41AM +0000, Sohom Datta via GitGitGadget wrote:\n\n> From: Sohom Datta <sohom.datta@learner.manipal.edu>\n> \n> Git allows users to create tags pointing to object hashes\n> that may or may not be commits. When a tag that doesn't\n> point to a commit is used with the -b (--branch) parameter\n> of git clone, git segfaults as it assumes that the tag will\n> always reference a commit.\n> \n> Add a check to make sure that lookup_commit_reference returns a commit\n> before detaching HEAD.\n\nThis seemed eerily familiar, so I dug up these threads:\n\n  - https://lore.kernel.org/git/20191029092735.GA84120@carpenter.lan/\n\n  - https://lore.kernel.org/git/20191101002432.GA49846@carpenter.lan/\n\nThe interesting things are:\n\n  - there are some other cases where we might see a non-commit, which\n    are also worth covering\n\n  - it would be friendlier to the user to put in a fallback value for\n    HEAD. We already transferred the whole history, so deleting it is a\n    bit less friendly than leaving it in a state the user can recover\n    from locally\n\nI think the patches there were moving in a good direction, but it looks\nlike they just got stalled.\n\n-Peff\n"},{"id":"409103","messageId":"pull.906.v2.git.git.1604510485432.gitgitgadget@gmail.com","threadId":"54537","inReplyTo":"pull.906.git.git.1604058401991.gitgitgadget@gmail.com","subject":"[PATCH v2] Avoid segfault and provide fallback when cloning invalid branch/tag","fromName":"Sohom Datta via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-04T17:21:25Z","receivedAt":"2020-11-04T17:21:30Z","isPatch":true,"sender":{"key":"name:Sohom Datta","avatar":null},"body":"From: Sohom Datta <sohom.datta@learner.manipal.edu>\n\nThe Git command to clone a specific branch of a git repository always\nassumes that the commit pointed to by the branch are going to valid. It\nthus segfaults and crashes whenever a tag or a branch with a invalid\ncommit is specified.\n\nAborting the operation is not desirable since the user is left with no\nusable git folder to work with despite git having done most of the work\nin setting everything up.\n\nThis commit is based of David Berardi's commit at\nhttps://lore.kernel.org/git/20191103180716.GA72007@carpenter.lan/\n\nMake git fallback on creating a unborn master branch when encountering\na invalid commit.\n\nSigned-off-by: Sohom Datta <sohom.datta@learner.manipal.edu>\n---\n    Fix potential segfault on cloning invalid tag\n    \n    Changes since v1:\n    \n     * Reworked the patch to use the fallback approach based on feedback\n       from Jeff King.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-906%2Fsohomdatta1%2Fsegfault-while-cloning-invalid-tag-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-906/sohomdatta1/segfault-while-cloning-invalid-tag-v2\nPull-Request: https://github.com/git/git/pull/906\n\nRange-diff vs v1:\n\n 1:  576b6049b2 < -:  ---------- Fix potential segfault on cloning invalid tag\n -:  ---------- > 1:  02b50572ff Avoid segfault and provide fallback when cloning invalid branch/tag\n\n\n builtin/clone.c         | 69 ++++++++++++++++++++++++++++++++++++++---\n t/t5609-clone-branch.sh | 15 +++++++++\n 2 files changed, 79 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex a0841923cf..53930f7536 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -711,10 +711,63 @@ 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 * commit_lookup_helper (const struct ref *our,\n+\t\t\t\t\t      const struct ref *remote,\n+\t\t\t\t\t      const char *msg,\n+\t\t\t\t\t      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+\telse {\n+\t\t/*\n+\t\t * We have no local branch requested with \"-b\", and the\n+\t\t * remote HEAD is unborn. There's nothing to update HEAD\n+\t\t * to, but this state is not an error.\n+\t\t */\n+\t\treturn NULL;\n+\t}\n+\n+\tif ( !lookup_object(the_repository, tip)) {\n+\t\t/**\n+\t\t * We have a object id associated with the tip of the branch\n+\t\t * but the object id doesn't resolve to a object. This will be\n+\t\t * handled downstream in update_ref().\n+\t\t */\n+\t\treturn NULL;\n+\t}\n+\n+\ttip_commit = lookup_commit_reference_gently(the_repository, tip, 1);\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\terror(_(\"non-commit branch cannot be checked out.\"));\n+\t\tcreate_symref(\"HEAD\", \"refs/heads/master\" ,msg);\n+\t\t*err = -1;\n+\t\treturn NULL;\n+\t}\n+\n+\treturn tip_commit;\n+}\n+\n+static int update_head(const struct ref *our,\n+\t\t\tconst struct ref *remote,\n \t\t\tconst char *msg)\n {\n \tconst char *head;\n+\tint err = 0;\n+\tconst struct commit * c;\n+\tc = commit_lookup_helper(our, remote, msg, &err);\n+\tif (err < 0) {\n+\t\treturn -1;\n+\t}\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@@ -725,8 +778,6 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \t\t\tinstall_branch_config(0, head, remote_name, 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@@ -739,6 +790,8 @@ 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+\n+\treturn 0;\n }\n \n static int git_sparse_checkout_init(const char *repo)\n@@ -1346,7 +1399,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);\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);\n \n \t/*\n \t * We want to show progress for recursive submodule clones iff\n@@ -1365,7 +1418,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t}\n \n \tjunk_mode = JUNK_LEAVE_REPO;\n-\terr = checkout(submodule_progress);\n+\tif ( err == 0 ) {\n+\t\t/*\n+\t\t * Only try to checkout if we successfully updated HEAD; otherwise\n+\t\t * HEAD isn't pointing to the thing the user wanted.\n+\t\t */\n+\t\terr = checkout(submodule_progress);\n+\t}\n \n \tfree(remote_name);\n \tstrbuf_release(&reflog_msg);\ndiff --git a/t/t5609-clone-branch.sh b/t/t5609-clone-branch.sh\nindex 6e7a7be052..a589db9aa0 100755\n--- a/t/t5609-clone-branch.sh\n+++ b/t/t5609-clone-branch.sh\n@@ -20,6 +20,9 @@ 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 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 \t git checkout master) &&\n \tmkdir empty &&\n \t(cd empty && git init)\n@@ -67,4 +70,16 @@ 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 'cloning -b for invalid tag must fail and fallback on remote head' '\n+\ttest_must_fail git clone -b broken-tag parent broken-tag 2>error &&\n+\ttest_i18ngrep \"non-commit branch cannot be checked out.\" error &&\n+\t(cd broken-tag && check_HEAD master)\n+'\n+\n+test_expect_success 'cloning -b for broken head must fail and fallback on remote head' '\n+\ttest_must_fail git clone -b broken-head parent broken-head &&\n+\ttest_i18ngrep \"non-commit branch cannot be checked out.\" error &&\n+\t(cd broken-head && check_HEAD master)\n+'\n+\n test_done\n\nbase-commit: 7f7ebe054af6d831b999d6c2241b9227c4e4e08d\n-- \ngitgitgadget\n"},{"id":"409128","messageId":"xmqq7dr0ncli.fsf@gitster.c.googlers.com","threadId":"54537","inReplyTo":"pull.906.v2.git.git.1604510485432.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] Avoid segfault and provide fallback when cloning invalid branch/tag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-04T19:31:05Z","receivedAt":"2020-11-04T19:31:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Sohom Datta via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Sohom Datta <sohom.datta@learner.manipal.edu>\n\n> Subject: Re: [PATCH v2] Avoid segfault and provide fallback when cloning invalid branch/tag\n\ncf. https://git-scm.com/docs/SubmittingPatches#describe-changes\n\ne.g. Subject: [PATCH v2] clone: do not put a non-commit on branch head\n\nor something like that?\n\n>\n> The Git command to clone a specific branch of a git repository always\n> assumes that the commit pointed to by the branch are going to valid. It\n\ns/are going to valid/is going to be valid/, or just \"is valid\".\nProbably the latter is preferable.\n\n> thus segfaults and crashes whenever a tag or a branch with a invalid\n> commit is specified.\n>\n> Aborting the operation is not desirable since the user is left with no\n> usable git folder to work with despite git having done most of the work\n\ns/folder/repository/; not everything is Windows ;-)\n\nInsert a comma after \"work with\" before \"despite\".\n\n> in setting everything up.\n\nThe above description is not wrong per-se, but the most expensive\npart of discarding the result at this poihnt would be the copying of\ncontents from the other side, not just setting things up locally,\nno?\n\n> This commit is based of David Berardi's commit at\n> https://lore.kernel.org/git/20191103180716.GA72007@carpenter.lan/\n>\n> Make git fallback on creating a unborn master branch when encountering\n> a invalid commit.\n>\n> Signed-off-by: Sohom Datta <sohom.datta@learner.manipal.edu>\n> ---\n>     Fix potential segfault on cloning invalid tag\n>     \n>     Changes since v1:\n>     \n>      * Reworked the patch to use the fallback approach based on feedback\n>        from Jeff King.\n\nThanks.  Other than the nit pointed out above, the log message looks\nreadable.  Let's take a look at the code.\n\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index a0841923cf..53930f7536 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -711,10 +711,63 @@ 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 * commit_lookup_helper (const struct ref *our,\n> +\t\t\t\t\t      const struct ref *remote,\n> +\t\t\t\t\t      const char *msg,\n> +\t\t\t\t\t      int *err)\n\nStyle.  Remove SP around commit_lookup_helper on both sides.\n\nfoo_helper() is not particularly a meaningful name.  Let's read on\nand see if we can come up with a better name.\n\n> +{\n> +\tconst struct object_id *tip = NULL;\n\nThis initialization is not strictly needed, no?  I'd rather see us\nleave it uninitialized.\n\n> +\tstruct commit *tip_commit =  NULL;\n\nExcess SP before NULL.\n\n\n> +\tif (our)\n> +\t\ttip = &our->old_oid;\n> +\telse if (remote)\n> +\t\ttip = &remote->old_oid;\n> +\telse {\n> +\t\t/*\n> +\t\t * We have no local branch requested with \"-b\", and the\n> +\t\t * remote HEAD is unborn. There's nothing to update HEAD\n> +\t\t * to, but this state is not an error.\n> +\t\t */\n> +\t\treturn NULL;\n> +\t}\n\nOK, so tip points at the location that has the object name of the\nobject that is planned to be at the tip of the branch that is\nchecked out to the working tree.\n\n> +\tif ( !lookup_object(the_repository, tip)) {\n\nStyle. No SP after '(' before the condition.\n\n> +\t\t/**\n\nWhy double asterisk here, unlike the early return we saw earlier?\nLet's lose the extra one.\n\n> +\t\t * We have a object id associated with the tip of the branch\n\ns/a object id/an object name/;\n\n> +\t\t * but the object id doesn't resolve to a object. This will be\n> +\t\t * handled downstream in update_ref().\n> +\t\t */\n\nThe design to punt on a non-existing object at this point in the\ncallflow may be sound, but is !lookup_object() a reliable indication\nthat the object name refers to a non-existing object?\n\nIn other words, I do not see what guarantees that we have already\nsaw and parsed the named object and placed it in the hashtable\nthe_repository->parsed_objects->obj_hash[].\n\nIs there a reason we should avoid using parse_object() here instead?\n\n> +\t\treturn NULL;\n> +\t}\n> +\n> +\ttip_commit = lookup_commit_reference_gently(the_repository, tip, 1);\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\nOK, the reasoning here is that we checked if tip refers to\nnon-existing object before we tried lookup_commit_reference_gently(),\nso a failure to find a commit here must mean the object does exist\nbut is not a committish.  And in such a case, we decided to abort\nthe calling update_head() and create an unborn branch ourselves.\n\nPerhaps makes sense, I need to think about it a bit more to be\ncertain, though.\n\n> +\t\terror(_(\"non-commit branch cannot be checked out.\"));\n> +\t\tcreate_symref(\"HEAD\", \"refs/heads/master\" ,msg);\n\nStyle.  Wrong placement of a comma before 'msg'.\n\nPost v2.29 codebase should ask what our default first branch name\nshould be.\n\n> +\t\t*err = -1;\n> +\t\treturn NULL;\n\nIt is customary to use the returned value from error(), like so\n\n\t\t*status = error(_(\"...\"));\n\nthat would save you one line.  Taking all the nits above together,\nperhaps\n\n\tif (!tip_commit) {\n\t\tchar *ref = xstrfmt(\"refs/heads/%s\", git_default_branch_name());\n\t\tcreate_symref(\"HEAD\", ref, msg);\n\t\t*err = error(_(\"...\"));\n\t\tfree(ref);\n\t\treturn NULL;\n\t}\n\nHOWEVER, I think it is much cleaner to lose the \"we first check if\nthe object exists\" part and directly see if tip points at a\ncommittish first.  Only when the tip does not point at a committish,\nwe need to tell the two cases apart.  So after figuring out into\nwhich ref the variable \"tip\" should point, we'd do something like:\n\n\n\ttip_commit = lookup_commit_refernece_gently(... tip, 1);\n\tif (tip_commit)\n\t\treturn tip_commit;\n\n\t/*\n\t * Now, \"tip\" is not a committish.  Does it even exist?\n\t */\n\tif (!lookup_object(... tip))\n\t\t/* let update_ref() handle this error condition */\n\t\treturn NULL;\n\n\t/*\n\t * Give an error, but leave the default initial branch\n\t * unborn, instead of failing the whole operation to cause\n\t * the directory removed.\n\t */\n\t... do the create_symref() thing ...\n\treturn NULL;\n\nWouldn't it be cleaner?\n\n> +\t}\n> +\n> +\treturn tip_commit;\n> +}\n> +\n> +static int update_head(const struct ref *our,\n> +\t\t\tconst struct ref *remote,\n\nIf you did not do unnecessary rewrapping, the patch noise in the\nfunction definition here would have been avoided, no?\n\n>  \t\t\tconst char *msg)\n>  {\n>  \tconst char *head;\n> +\tint err = 0;\n> +\tconst struct commit * c;\n\nStyle.  Lose SP between asterisk and c; in our codebase the asterisk\nsticks to the identifier (not type).\n\nHave a blank line here, between the end of decl and the first stmt\nin the block.\n\n> +\tc = commit_lookup_helper(our, remote, msg, &err);\n> +\tif (err < 0) {\n> +\t\treturn -1;\n> +\t}\n\nStyle. No {} around a single stmt.\n\nMore importantly, without a name that is more meaningful than\n\"commit_lookup_helper()\" and without any comment near its definition\nto describe what the function is supposed to compute and what it\nconsiders an error, the reader of this callsite cannot tell what we\nare calling the \"helper\" for, and under what condition we are told\nby the helper to return an error to our callers.\n\nWith the analysis and rewrite of that _helper() function, I think a\nmore descriptive name for the helper should contain words like\n\"check/ensure\" and \"commit\" in it.  And the \"err\" variables, both in\nthe caller and in the helper function, are somewhat misnamed.  It is\na signal from the callee to the caller that updating the branch head\nhas already been done by the callee, so the caller does not have to \ndo anything further.  Perhaps\n\n\n\tint skip_update_ref = 0;\n\tconst struct commit *c;\n\n\tc = check_tip_commit(our, remote, msg, &skip_update_ref);\n\tif (skip_update_ref)\n\t\treturn -1; /* left a fallback unborn branch */\n\n\t... and then \"c\" is a commit (already peeled) usable for\n\t... pointing HEAD to at this point.\n\nI'd suggest giving the \"c\" variable, whose lifespan is now much\nlonger than in the original code, a more meaningful name, though.\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> @@ -725,8 +778,6 @@ static void update_head(const struct ref *our, const struct ref *remote,\n>  \t\t\tinstall_branch_config(0, head, remote_name, 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> @@ -739,6 +790,8 @@ 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\nCan't this cause discrepancy when the helper picked remote->old_oid\nas the \"tip\" there?  We called lookup_commit_reference_gently() on\nthat oid, and decided to check out the resulting commit and point\nHEAD at it, so we are certain that your \"c\" is a commit object, but\nremote->old_oid may name an annotated tag that points at the commit\nobject.  Don't we want to use &c->object.old here as well?\n\nIf that is the case (you need to think about it yourself---I didn't\nspend more than 5 minutes on this particular point myself and the\nabove might be total nonsense), this part would become\n\n\tif (our && our is in our local branch namespace) {\n\t\t... do the local thing ...\n\t} else if (our || remote) {\n\t\tupdate_ref(msg, \"HEAD\", &c->object.oid, NULL, ...);\n\t}\n\nas the body for 'our' and 'remote' are the same.\n\n>  \t}\n> +\n> +\treturn 0;\n>  }\n>  \n>  static int git_sparse_checkout_init(const char *repo)\n> @@ -1346,7 +1399,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);\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);\n>  \n>  \t/*\n>  \t * We want to show progress for recursive submodule clones iff\n> @@ -1365,7 +1418,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \t}\n>  \n>  \tjunk_mode = JUNK_LEAVE_REPO;\n> -\terr = checkout(submodule_progress);\n> +\tif ( err == 0 ) {\n> +\t\t/*\n> +\t\t * Only try to checkout if we successfully updated HEAD; otherwise\n> +\t\t * HEAD isn't pointing to the thing the user wanted.\n> +\t\t */\n> +\t\terr = checkout(submodule_progress);\n> +\t}\n\nThe logic looks OK.  We avoid checking out an unborn if we left HEAD\nunborn because there is no better choice.  And by not calling\ncheckout() and overwriting the err variable, we make sure that the\nentire command still reports a failure.\n\nI am not absolutely sure if reusing 'err' like the above two hunks\nis the best thing to do, or the code becomes easier to follow if we\nintroduced another variable to receive the returned value from\nupdate_head().  Perhaps what we have above is good enough.  I dunno.\n\n> diff --git a/t/t5609-clone-branch.sh b/t/t5609-clone-branch.sh\n> index 6e7a7be052..a589db9aa0 100755\n> --- a/t/t5609-clone-branch.sh\n> +++ b/t/t5609-clone-branch.sh\n> @@ -20,6 +20,9 @@ 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 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\nStyle.  Do not leave SP between redirection operator and its target,\ni.e.\n\n\techo $blob >.git/refs/heads/...\n\nDidn't you mean to use refs/tags/broken-tag for the first one, though?\nOtherwise, I do not see how anybody would expect the two tests check\ndifferent things.\n\nThanks.\n\n\n> +test_expect_success 'cloning -b for invalid tag must fail and fallback on remote head' '\n> +\ttest_must_fail git clone -b broken-tag parent broken-tag 2>error &&\n> +\ttest_i18ngrep \"non-commit branch cannot be checked out.\" error &&\n> +\t(cd broken-tag && check_HEAD master)\n> +'\n> +\n> +test_expect_success 'cloning -b for broken head must fail and fallback on remote head' '\n> +\ttest_must_fail git clone -b broken-head parent broken-head &&\n> +\ttest_i18ngrep \"non-commit branch cannot be checked out.\" error &&\n> +\t(cd broken-head && check_HEAD master)\n> +'\n> +\n>  test_done\n>\n> base-commit: 7f7ebe054af6d831b999d6c2241b9227c4e4e08d\n"}]}