{"thread":{"id":"41448","subject":"[PATCH] submodule: Fetch the direct sha1 first","startedAt":"2016-02-19T18:57:33Z","lastAt":"2016-02-22T19:22:04Z","messageCount":8,"participants":["Stefan Beller","Junio C Hamano","Jacob Keller","Jens Lehmann"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"278691","messageId":"1455908253-1136-1-git-send-email-sbeller@google.com","threadId":"41448","inReplyTo":null,"subject":"[PATCH] submodule: Fetch the direct sha1 first","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-19T18:57:33Z","receivedAt":"2016-02-19T18:57:33Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When reviewing a change in Gerrit, which also updates a submodule,\na common review practice is to download and cherry-pick the patch locally\nto test it. However when testing it locally, the 'git submodule update'\nmay fail fetching the correct submodule sha1 as the corresponding commit\nin the submodule is not yet part of the project history, but also just a\nproposed change.\n\nTo ease this, try fetching by sha1 first and when that fails (in case of\nservers which do not allow fetching by sha1), fall back to the default\nbehavior we already have.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nI think it's best to apply this on origin/master, there is no collision\nwith sb/submodule-parallel-update.\n\nAlso I do not see a good way to test this both in correctness as well\nas performance degeneration. If the first git fetch fails, the second\nfetch is executed, so it should behave as before this patch w.r.t. correctness.\n\nRegarding performance, the first fetch should fail quite fast iff the fetch\nfails and then continue with the normal fetch. In case the first fetch works\nfine getting the exact sha1, the fetch should be faster than a default fetch\nas potentially less data needs to be fetched.\n\nThanks,\nStefan\n\n git-submodule.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 9bc5c5f..ee0b985 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -746,8 +746,9 @@ Maybe you want to use 'update --init'?\")\"\n \t\t\t\t# Run fetch only if $sha1 isn't present or it\n \t\t\t\t# is not reachable from a ref.\n \t\t\t\t(clear_local_git_env; cd \"$sm_path\" &&\n+\t\t\t\t\tremote_name=$(get_default_remote)\n \t\t\t\t\t( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) &&\n-\t\t\t\t\t test -z \"$rev\") || git-fetch)) ||\n+\t\t\t\t\t test -z \"$rev\") || git-fetch $remote_name $rev || git-fetch)) ||\n \t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n \t\t\tfi\n \n-- \n2.7.0.rc0.34.ga06e0b3.dirty\n"},{"id":"278703","messageId":"xmqqpovsbdyu.fsf@gitster.mtv.corp.google.com","threadId":"41448","inReplyTo":"1455908253-1136-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] submodule: Fetch the direct sha1 first","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-19T21:13:45Z","receivedAt":"2016-02-19T21:13:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> When reviewing a change in Gerrit, which also updates a submodule,\n> a common review practice is to download and cherry-pick the patch locally\n> to test it. However when testing it locally, the 'git submodule update'\n> may fail fetching the correct submodule sha1 as the corresponding commit\n> in the submodule is not yet part of the project history, but also just a\n> proposed change.\n>\n> To ease this, try fetching by sha1 first and when that fails (in case of\n> servers which do not allow fetching by sha1), fall back to the default\n> behavior we already have.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>\n> I think it's best to apply this on origin/master, there is no collision\n> with sb/submodule-parallel-update.\n>\n> Also I do not see a good way to test this both in correctness as well\n> as performance degeneration. If the first git fetch fails, the second\n> fetch is executed, so it should behave as before this patch w.r.t. correctness.\n>\n> Regarding performance, the first fetch should fail quite fast iff the fetch\n> fails and then continue with the normal fetch. In case the first fetch works\n> fine getting the exact sha1, the fetch should be faster than a default fetch\n> as potentially less data needs to be fetched.\n\n\"The fetch should be faster\" may not be making a good trade-off\noverall--people may have depended on the branches configured to be\nfetched to be fetched after this codepath is exercised, but now if\nthe commit bound to the superproject tree happens to be complete,\neven though it is not anchored by any remote tracking ref (hence the\nnext GC may clobber it), the fetch of other branches will not\nhappen.\n\nMy knee-jerk reaction is that the order of fallback is probably the\nother way around.  That is, try \"git fetch\" as before, check again\nif the commit bound to the superproject tree is now complete, and\nfallback to fetch that commit with an extra \"git fetch\".\n\nJens, what do you think?\n\n>  git-submodule.sh | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 9bc5c5f..ee0b985 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -746,8 +746,9 @@ Maybe you want to use 'update --init'?\")\"\n>  \t\t\t\t# Run fetch only if $sha1 isn't present or it\n>  \t\t\t\t# is not reachable from a ref.\n>  \t\t\t\t(clear_local_git_env; cd \"$sm_path\" &&\n> +\t\t\t\t\tremote_name=$(get_default_remote)\n>  \t\t\t\t\t( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) &&\n> -\t\t\t\t\t test -z \"$rev\") || git-fetch)) ||\n> +\t\t\t\t\t test -z \"$rev\") || git-fetch $remote_name $rev\n\nRegardless of the \"fallback order\" issue, I do not think $rev is a\ncorrect thing to fetch here.  The superproject binds $sha1 to its\ntree, and you would be checking that out, so shouldn't you be\nfetching that commit?\n"},{"id":"278706","messageId":"CAGZ79kaOQTGEY6akKgz695nPdG4cG4SsYKLcJkKr1im+RQjK5A@mail.gmail.com","threadId":"41448","inReplyTo":"xmqqpovsbdyu.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule: Fetch the direct sha1 first","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-19T22:10:34Z","receivedAt":"2016-02-19T22:10:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Feb 19, 2016 at 1:13 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> When reviewing a change in Gerrit, which also updates a submodule,\n>> a common review practice is to download and cherry-pick the patch locally\n>> to test it. However when testing it locally, the 'git submodule update'\n>> may fail fetching the correct submodule sha1 as the corresponding commit\n>> in the submodule is not yet part of the project history, but also just a\n>> proposed change.\n>>\n>> To ease this, try fetching by sha1 first and when that fails (in case of\n>> servers which do not allow fetching by sha1), fall back to the default\n>> behavior we already have.\n>>\n>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>> ---\n>>\n>> I think it's best to apply this on origin/master, there is no collision\n>> with sb/submodule-parallel-update.\n>>\n>> Also I do not see a good way to test this both in correctness as well\n>> as performance degeneration. If the first git fetch fails, the second\n>> fetch is executed, so it should behave as before this patch w.r.t. correctness.\n>>\n>> Regarding performance, the first fetch should fail quite fast iff the fetch\n>> fails and then continue with the normal fetch. In case the first fetch works\n>> fine getting the exact sha1, the fetch should be faster than a default fetch\n>> as potentially less data needs to be fetched.\n>\n> \"The fetch should be faster\" may not be making a good trade-off\n> overall--people may have depended on the branches configured to be\n> fetched to be fetched after this codepath is exercised, but now if\n> the commit bound to the superproject tree happens to be complete,\n> even though it is not anchored by any remote tracking ref (hence the\n> next GC may clobber it), the fetch of other branches will not\n> happen.\n>\n> My knee-jerk reaction is that the order of fallback is probably the\n> other way around.  That is, try \"git fetch\" as before, check again\n> if the commit bound to the superproject tree is now complete, and\n> fallback to fetch that commit with an extra \"git fetch\".\n\nI thought about that and assumed we'd need to have an option for\nfetch like \"--try-to-get-sha1\", which depending on the servers capabilities\nwould just add that sha1 to the \"wants\" during fetching negotiation if the\nserver supports it, otherwise just fetch normally.\n\nDoing a 'git fetch' only and not the fetch for the specific sha1 would be\nincorrect? ('git fetch' with no args finishes successfully, so no fallback is\ntriggered. But we are not sure if we obtained the sha1, so we need to\ncheck if we have the sha1 by doing a local check and then try to get the sha1\nagain if we don't have it locally. So doing the reverse order would be\nmore code here for correctness.\n\n>\n> Jens, what do you think?\n>\n>>  git-submodule.sh | 3 ++-\n>>  1 file changed, 2 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/git-submodule.sh b/git-submodule.sh\n>> index 9bc5c5f..ee0b985 100755\n>> --- a/git-submodule.sh\n>> +++ b/git-submodule.sh\n>> @@ -746,8 +746,9 @@ Maybe you want to use 'update --init'?\")\"\n>>                               # Run fetch only if $sha1 isn't present or it\n>>                               # is not reachable from a ref.\n>>                               (clear_local_git_env; cd \"$sm_path\" &&\n>> +                                     remote_name=$(get_default_remote)\n>>                                       ( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) &&\n>> -                                      test -z \"$rev\") || git-fetch)) ||\n>> +                                      test -z \"$rev\") || git-fetch $remote_name $rev\n>\n> Regardless of the \"fallback order\" issue, I do not think $rev is a\n> correct thing to fetch here.  The superproject binds $sha1 to its\n> tree, and you would be checking that out, so shouldn't you be\n> fetching that commit?\n\nBoth $sha1 and $rev are in the submodule (because\n'git submodule--helper list' puts out the sha1 as the\nsubmodule sha1). $rev is either empty or equal to $sha1\nin my understanding of \"rev-list $sha1 --not --all\". However for\nreadability maybe we want to write:\n\n    (clear_local_git_env; cd \"$sm_path\" &&\n        test -z $(git rev-list -n 1 $sha1 --not --all 2>/dev/null) ||\n        git fetch $(get_default_remote) $sha1 ||\n        git fetch ||\n        die ...\n    )\n\nSo in case you want to the other order, I'd propose\n\n    (clear_local_git_env; cd \"$sm_path\" &&\n        test -z $(git rev-list -n 1 $sha1 --not --all 2>/dev/null) ||\n        git fetch ||\n        (git cat-file -e $sha1 && git fetch $(get_default_remote) $sha1) ||\n        die ...\n    )\n\nOh! Looking at that I suspect the\n\"test -z $(git rev-list -n 1 $sha1 --not --all 2>/dev/null)\"\nand \"git cat-file -e\" are serving the same purpose here and should just\nindicate if the given sha1 is present or not.\n\nSo we could reduce it further to\n\n    (clear_local_git_env; cd \"$sm_path\" &&\n        git cat-file -e $sha1 || git fetch ||\n        (git cat-file -e $sha1 && git fetch $(get_default_remote) $sha1) ||\n        die ...\n    )\n\nI may have messed up the logic operators along the way, maybe it is\neven better if\nwe rewrite it with non shorted conditions.\n\nThanks,\nStefan\n"},{"id":"278707","messageId":"xmqqbn7cbahb.fsf@gitster.mtv.corp.google.com","threadId":"41448","inReplyTo":"CAGZ79kaOQTGEY6akKgz695nPdG4cG4SsYKLcJkKr1im+RQjK5A@mail.gmail.com","subject":"Re: [PATCH] submodule: Fetch the direct sha1 first","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-19T22:29:04Z","receivedAt":"2016-02-19T22:29:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Doing a 'git fetch' only and not the fetch for the specific sha1 would be\n> incorrect?\n\nI thought that was what you are attempting to address.\n\n> ('git fetch' with no args finishes successfully, so no fallback is\n> triggered. But we are not sure if we obtained the sha1, so we need to\n> check if we have the sha1 by doing a local check and then try to get the sha1\n> again if we don't have it locally.\n\nYes, that is what I meant in the \"In the opposite fallback order\"\nsuggestion.\n>>>                               (clear_local_git_env; cd \"$sm_path\" &&\n>>> +                                     remote_name=$(get_default_remote)\n>>>                                       ( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) &&\n>>> -                                      test -z \"$rev\") || git-fetch)) ||\n>>> +                                      test -z \"$rev\") || git-fetch $remote_name $rev\n>>\n>> Regardless of the \"fallback order\" issue, I do not think $rev is a\n>> correct thing to fetch here.  The superproject binds $sha1 to its\n>> tree, and you would be checking that out, so shouldn't you be\n>> fetching that commit?\n>\n> Both $sha1 and $rev are in the submodule (because\n> 'git submodule--helper list' puts out the sha1 as the\n> submodule sha1). $rev is either empty or equal to $sha1\n> in my understanding of \"rev-list $sha1 --not --all\".\n\nNot quite.  The rev-list command expects [*1*] one of three outcomes\nin the original construct:\n\n * The repository does not know anything about $sha1; the command\n   fails, rev is left empty, but thanks to &&, git-fetch runs.\n\n * The repository has $sha1 but the history behind it is not\n   complete.  While digging from $sha1 following the parent chain,\n   it would hit a missing object and fails, rev may or may not be\n   empty, but thanks to &&, git-fetch runs.\n\n * The repository has $sha1 and its history is all connected.  The\n   command succeeds.  If $sha1 is not connected to any of the refs,\n   however, that commit may be shown and stored in $rev.  In this\n   case, \"$rev\" happens to be the same as \"$sha1\".\n\nAs this \"fetch\" is run in order to make sure that the history behind\n$sha1 is complete in the submodule repository, so that detaching the\nHEAD at that commit will give the user a useful repository and its\nworking tree, the check the code is doing in the original is already\nflawed.  If $sha1 and its ancestry is complete in the repository,\nrev-list would succeed, and if $sha1 is ahead of any of the refs,\nthe original code still runs \"git fetch\", which is not necessary for\nthe purpose of detaching the head at $sha1.  On the other hand, by\nusing \"-n 1\", it can cause rev-list stop before discovering a gap in\nhistory behind $sha1, allowing \"git fetch\" to be skipped when it\nshould be run to fill the gap in the history.\n\nTo be complete, the rev-list command line should also run with\n\"--objects\"; after all, a commit walker fetch may have downloaded\ncommit chain completely but haven't fetched necessary trees and\nblobs when it was killed, and \"rev-list $sha1 --not --all\" would not\ncatch such a breakage without \"--objects\".\n\n> Oh! Looking at that I suspect the\n> \"test -z $(git rev-list -n 1 $sha1 --not --all 2>/dev/null)\"\n> and \"git cat-file -e\" are serving the same purpose here and should just\n> indicate if the given sha1 is present or not.\n\nThat is the simplest explanation why the original \"rev-list\"\ninvocation is already wrong.  It should do an equivalent of\nbuiltin/fetch.c::quickfetch() to ensure that $sha1 is something that\nis complete, i.e. could be anchored with a ref if we wanted to,\nbefore deciding to avoid running \"git fetch\".\n"},{"id":"278712","messageId":"CAGZ79kaL8T72Fcy1kzuRrYagX9biRTscA4q=xBc7JaUXv5msVg@mail.gmail.com","threadId":"41448","inReplyTo":"xmqqbn7cbahb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule: Fetch the direct sha1 first","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-19T23:40:07Z","receivedAt":"2016-02-19T23:40:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Feb 19, 2016 at 2:29 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> Doing a 'git fetch' only and not the fetch for the specific sha1 would be\n>> incorrect?\n>\n> I thought that was what you are attempting to address.\n\nYep. In an ideal world I would imagine it would look like\n\n    if $sha1 doesn't exist:\n        fetch $sha1\n        if server did not support fetching direct sha1:\n            fallback to fetch <no args>\n\nand I thought this would be small enough to be expressed\nusing the || and &&.\n\nIt would be slightly more complicated for first doing the fetch\nand then refetching if the sha1 is still missing as the condition\nis more complicated to express.\n\n>\n>> ('git fetch' with no args finishes successfully, so no fallback is\n>> triggered. But we are not sure if we obtained the sha1, so we need to\n>> check if we have the sha1 by doing a local check and then try to get the sha1\n>> again if we don't have it locally.\n>\n> Yes, that is what I meant in the \"In the opposite fallback order\"\n> suggestion.\n>>>>                               (clear_local_git_env; cd \"$sm_path\" &&\n>>>> +                                     remote_name=$(get_default_remote)\n>>>>                                       ( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) &&\n>>>> -                                      test -z \"$rev\") || git-fetch)) ||\n>>>> +                                      test -z \"$rev\") || git-fetch $remote_name $rev\n>>>\n>>> Regardless of the \"fallback order\" issue, I do not think $rev is a\n>>> correct thing to fetch here.  The superproject binds $sha1 to its\n>>> tree, and you would be checking that out, so shouldn't you be\n>>> fetching that commit?\n>>\n>> Both $sha1 and $rev are in the submodule (because\n>> 'git submodule--helper list' puts out the sha1 as the\n>> submodule sha1). $rev is either empty or equal to $sha1\n>> in my understanding of \"rev-list $sha1 --not --all\".\n>\n> Not quite.  The rev-list command expects [*1*] one of three outcomes\n> in the original construct:\n>\n>  * The repository does not know anything about $sha1; the command\n>    fails, rev is left empty, but thanks to &&, git-fetch runs.\n\nok. \"git cat-file -e\" would be able to replace this case.\n\n>\n>  * The repository has $sha1 but the history behind it is not\n>    complete.  While digging from $sha1 following the parent chain,\n>    it would hit a missing object and fails, rev may or may not be\n>    empty, but thanks to &&, git-fetch runs.\n\nwhich I read as a broken shallow clone or a half-way gc'ed repository.\n(git fetch repairs that? ok.)\n\nAn intact shallow clone which has enough history to contain sha1 should\nnot be deepened in my understanding of \"submodule update\".\n\nRereading the man page for  \"git cat-file -e\", the output of\n\"cat-file -e\" is the same as in the first case, and we want to also fetch.\n\n>\n>  * The repository has $sha1 and its history is all connected.  The\n>    command succeeds.  If $sha1 is not connected to any of the refs,\n>    however, that commit may be shown and stored in $rev.  In this\n>    case, \"$rev\" happens to be the same as \"$sha1\".\n\nSo it would be possible to checkout $sha1 as a detached HEAD,\nbut it's not strongly protected against gc. (I assume gc will not\ntouch objects reachable from HEAD, but not referenced by any ref,\nbut HEAD can change in a heart beat, so it is not as strong of a protection\nas having a branch include that $sha1).\n\n>\n> As this \"fetch\" is run in order to make sure that the history behind\n> $sha1 is complete in the submodule repository, so that detaching the\n> HEAD at that commit will give the user a useful repository and its\n> working tree, the check the code is doing in the original is already\n> flawed.  If $sha1 and its ancestry is complete in the repository,\n> rev-list would succeed, and if $sha1 is ahead of any of the refs,\n> the original code still runs \"git fetch\", which is not necessary for\n> the purpose of detaching the head at $sha1.  On the other hand, by\n> using \"-n 1\", it can cause rev-list stop before discovering a gap in\n> history behind $sha1, allowing \"git fetch\" to be skipped when it\n> should be run to fill the gap in the history.\n>\n> To be complete, the rev-list command line should also run with\n> \"--objects\"; after all, a commit walker fetch may have downloaded\n> commit chain completely but haven't fetched necessary trees and\n> blobs when it was killed, and \"rev-list $sha1 --not --all\" would not\n> catch such a breakage without \"--objects\".\n\nSo 'cat-file -e' doesn't sound like it would back-test the history at all,\nso it doesn't sound like a sufficient replacement.\n\n>\n>> Oh! Looking at that I suspect the\n>> \"test -z $(git rev-list -n 1 $sha1 --not --all 2>/dev/null)\"\n>> and \"git cat-file -e\" are serving the same purpose here and should just\n>> indicate if the given sha1 is present or not.\n>\n> That is the simplest explanation why the original \"rev-list\"\n> invocation is already wrong.  It should do an equivalent of\n> builtin/fetch.c::quickfetch() to ensure that $sha1 is something that\n> is complete, i.e. could be anchored with a ref if we wanted to,\n> before deciding to avoid running \"git fetch\".\n\nWould it make sense in case of broken histories to not fetch\n(specially if the user asked to not fetch) and rather repair by\nmaking it a shallow repository?\n\n>\n"},{"id":"278714","messageId":"xmqqvb5k9r5g.fsf@gitster.mtv.corp.google.com","threadId":"41448","inReplyTo":"CAGZ79kaL8T72Fcy1kzuRrYagX9biRTscA4q=xBc7JaUXv5msVg@mail.gmail.com","subject":"Re: [PATCH] submodule: Fetch the direct sha1 first","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-20T00:11:55Z","receivedAt":"2016-02-20T00:11:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On Fri, Feb 19, 2016 at 2:29 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>>> Doing a 'git fetch' only and not the fetch for the specific sha1 would be\n>>> incorrect?\n>>\n>> I thought that was what you are attempting to address.\n>\n> Yep. In an ideal world I would imagine it would look like\n>\n>     if $sha1 doesn't exist:\n>         fetch $sha1\n>         if server did not support fetching direct sha1:\n>             fallback to fetch <no args>\n\nIt should look more like this:\n\n\tif $sha1's history and objects are incomplete:\n\t\tfetch ;# normally just like we have done before\n                if $sha1's history and objects are still incomplete:\n\t\t\tfetch $sha1\n\nas existing users already expect that commits and objects that are\nreachable from tips of refs configured to be fetched in the\nsubmodule via its configured refspecs are available after this part\nof the code runs, regardless of this \"Gerrit reviews may not have\narrived to branches yet\" issue.  The first \"normal\" fetch ensures\nthat the expectation is met.\n\n> Would it make sense in case of broken histories to not fetch\n> (specially if the user asked to not fetch) and rather repair by\n> making it a shallow repository?\n\nCommits whose ancestors, trees and/or blobs are incomplete can and\ndo exist in a perfectly healthy repository and there is no breakage\nin the history as long as such commits are not reachable from any of\nthe refs.\n\nYou can for example make a small fetch from 'pu' today, that results\nin unpack-objects to be run instead of index-pack, and then make\nanother fetch from 'pu', making these loose objects unreachable from\nanywhere.  Maybe there were 5 commits worth of objects in the\noriginal transfer, and the objects necessary for the bottom 2 were\npruned away while the tip one still in the repository [*1*].\n\n\"cat-file -e\" may find that the tip commit is there, but \"rev-list\n--objects $oldtip --not --all\" will find that the old tip of pu that\nis left behind is incomplete and cannot be safely used (e.g. \"git\nlog -p\" would fail).  The \"$sha1's history and objects are\nincomplete\" check aka \"quickfetch()\" test is a way to avoid getting\nfooled by an object that passes \"cat-file -e\" test.\n\nI am not sure if it is feasible, given such an island of commits and\nassociated objects, to craft a proper \"shallow\" boundary after the\nfact.  It should be doable with extra code, but I do not think there\nis a canned support at the plumbing level (you can obviously\nconstruct it with \"cat-file -e\" and following the inter object links\nyourself).\n\nThis \"fetch\" is in a cmd_update() codepath, whose purpose is \"Update\neach submodule path to correct revision, using clone and checkout as\nneeded\", so I am not sure \"to not fetch, specically if the user\nasked to not fetch\" makes much sense in the first place.\n\n\n[Footnote]\n\n*1* A canonical example used to be a commit walker that fetches from\n    the tip of a branch that is ahead of us by 5 commits, gets\n    killed after fetching and storing the tip commit object and some\n    of its trees and blobs, and before successfully fetching and\n    storing all the necessary objects, e.g. the parent commits and\n    its trees and blobs.  That would leave a disconnected island of\n    objects that are not anchored by any ref.\n"},{"id":"278729","messageId":"CA+P7+xrjE5fF9QKe5AvAcuwNtx4O5yq8FfkXtyrR8r7+E=d8Bw@mail.gmail.com","threadId":"41448","inReplyTo":"xmqqpovsbdyu.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule: Fetch the direct sha1 first","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-02-20T10:52:03Z","receivedAt":"2016-02-20T10:52:03Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Feb 19, 2016 at 1:13 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Regarding performance, the first fetch should fail quite fast iff the fetch\n>> fails and then continue with the normal fetch. In case the first fetch works\n>> fine getting the exact sha1, the fetch should be faster than a default fetch\n>> as potentially less data needs to be fetched.\n>\n> \"The fetch should be faster\" may not be making a good trade-off\n> overall--people may have depended on the branches configured to be\n> fetched to be fetched after this codepath is exercised, but now if\n> the commit bound to the superproject tree happens to be complete,\n> even though it is not anchored by any remote tracking ref (hence the\n> next GC may clobber it), the fetch of other branches will not\n> happen.\n>\n> My knee-jerk reaction is that the order of fallback is probably the\n> other way around.  That is, try \"git fetch\" as before, check again\n> if the commit bound to the superproject tree is now complete, and\n> fallback to fetch that commit with an extra \"git fetch\".\n>\n\nFWIW, I think the order you suggest here is probably better. It would\nbe lower risk of breaking something since we'd only do something more\nin this case if the current fetch fails.\n\nI've definitely been bit by this before thinking that the sub module\nwould be able to be fetched just fine only to discover that it wasn't\nable to locate the change.\n\nRegards,\nJake\n"},{"id":"278896","messageId":"56CB5FDC.5050409@web.de","threadId":"41448","inReplyTo":"xmqqvb5k9r5g.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule: Fetch the direct sha1 first","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2016-02-22T19:22:04Z","receivedAt":"2016-02-22T19:22:04Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 20.02.2016 um 01:11 schrieb Junio C Hamano:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> On Fri, Feb 19, 2016 at 2:29 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> Stefan Beller <sbeller@google.com> writes:\n>>>\n>>>> Doing a 'git fetch' only and not the fetch for the specific sha1 would be\n>>>> incorrect?\n>>>\n>>> I thought that was what you are attempting to address.\n>>\n>> Yep. In an ideal world I would imagine it would look like\n>>\n>>      if $sha1 doesn't exist:\n>>          fetch $sha1\n>>          if server did not support fetching direct sha1:\n>>              fallback to fetch <no args>\n>\n> It should look more like this:\n>\n> \tif $sha1's history and objects are incomplete:\n> \t\tfetch ;# normally just like we have done before\n>                  if $sha1's history and objects are still incomplete:\n> \t\t\tfetch $sha1\n\nThat makes lots of sense, doesn't break existing workflows and\nenables the use case Stefan described. And if people want to skip\nthe first fetch later we could still add a config option to do so.\n\n> as existing users already expect that commits and objects that are\n> reachable from tips of refs configured to be fetched in the\n> submodule via its configured refspecs are available after this part\n> of the code runs, regardless of this \"Gerrit reviews may not have\n> arrived to branches yet\" issue.  The first \"normal\" fetch ensures\n> that the expectation is met.\n\nNot sure if that has come up so far, but I believe we should not\nonly do that for the submodule command but also for a regular\nfetch when it is configured to fetch submodule commits too (which\nit is by default unless configured otherwise). Otherwise we'll\nlose the plane-safety fetch normally provides in case of these\nunconnected submodule sha1s, which would then again break users\nexpectations.\n\nAnd if we see demand for only fetching the sha1s without any\nextra history in the future (e.g. to minimize the amount of data\nto be fetched by a CI server), we could add a new value (\"by-sha1\"\nor such) for both the --recurse-submodules option of fetch and\npull and the submodule.<name>.fetchRecurseSubmodules config\nsetting. Then both a git submodule update and fetch would attempt\nto just fetch the sha1(s) needed without any fetching any extra\nhistory.\n"}]}