summaryrefslogtreecommitdiff
path: root/t/t5616-partial-clone.sh
AgeCommit message (Collapse)AuthorFilesLines
2020-12-17Merge branch 'tb/partial-clone-filters-fix'Libravatar Junio C Hamano1-1/+9
Fix potential server side resource deallocation issues when responding to a partial clone request. * tb/partial-clone-filters-fix: upload-pack.c: don't free allowed_filters util pointers builtin/clone.c: don't ignore transport_fetch_refs() errors
2020-12-03upload-pack.c: don't free allowed_filters util pointersLibravatar Taylor Blau1-1/+9
To keep track of which object filters are allowed or not, 'git upload-pack' stores the name of each filter in a string_list, and sets it ->util pointer to be either 0 or 1, indicating whether it is banned or allowed. Later on, we attempt to clear that list, but we incorrectly ask for the util pointers to be free()'d, too. This behavior (introduced back in 6dd3456a8c (upload-pack.c: allow banning certain object filter(s), 2020-08-03)) leads to an invalid free, and causes us to crash. In order to trigger this, one needs to fetch from a server that (a) has at least one object filter allowed, and (b) issue a fetch that contains a subset of the allowed filters (i.e., we cannot ask for a banned filter, since this causes us to die() before we hit the bogus string_list_clear()). In that case, whatever banned filters exist will cause a noop free() (since those ->util pointers are set to 0), but the first allowed filter we try to free will crash us. We never noticed this in the tests because we didn't have an example of setting 'uploadPackFilter' configuration variables and then following up with a valid fetch. The first new 'git clone' prevents further regression here. For good measure on top, add a test which checks the same behavior at a tree depth greater than 0. Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-12-03upload-pack: propagate return value from object filter config callbackLibravatar Jeff King1-0/+8
If we encounter an error in parse_filter_object_config(), we'll complain to stderr but won't actually propagate the return value up the stack. This is unlike most of our config callbacks, which return the error to git_config() so it can die (this includes the call just below us to parse_hide_refs_config(), which can also produce errors). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-09-03Merge branch 'jt/lazy-fetch'Libravatar Junio C Hamano1-0/+20
Updates to on-demand fetching code in lazily cloned repositories. * jt/lazy-fetch: fetch: no FETCH_HEAD display if --no-write-fetch-head fetch-pack: remove no_dependents code promisor-remote: lazy-fetch objects in subprocess fetch-pack: do not lazy-fetch during ref iteration fetch: only populate existing_refs if needed fetch: avoid reading submodule config until needed fetch: allow refspecs specified through stdin negotiator/noop: add noop fetch negotiator
2020-08-20fetch-pack: in partial clone, pass --promisorLibravatar Jonathan Tan1-0/+16
When fetching a pack from a promisor remote, the corresponding .promisor file needs to be created. "fetch-pack" originally did this by passing "--promisor" to "index-pack", but in 5374a290aa ("fetch-pack: write fetched refs to .promisor", 2019-10-16), "fetch-pack" was taught to do this itself instead, because it needed to store ref information in the .promisor file. This causes a problem with superprojects when transfer.fsckobjects is set, because in the current implementation, it is "index-pack" that calls fsck_finish() to check the objects; before 5374a290aa, fsck_finish() would see that .gitmodules is a promisor object and tolerate it being missing, but after, there is no .promisor file (at the time of the invocation of fsck_finish() by "index-pack") to tell it that .gitmodules is a promisor object, so it returns an error. Therefore, teach "fetch-pack" to pass "--promisor" to index pack once again. "fetch-pack" will subsequently overwrite this file with the ref information. An alternative is to instead move object checking to "fetch-pack", and let "index-pack" only index the files. However, since "index-pack" has to inflate objects in order to index them, it seems reasonable to also let it check the objects (which also require inflated files). Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-08-18fetch-pack: do not lazy-fetch during ref iterationLibravatar Jonathan Tan1-0/+20
In order to determine negotiation tips, "fetch-pack" iterates over all refs and dereferences all annotated tags found. This causes the existence of targets of refs and annotated tags to be checked. Avoiding this is especially important when we use "git fetch" (which invokes "fetch-pack") to perform lazy fetches in a partial clone because a target of such a ref or annotated tag may need to be itself lazy-fetched (and otherwise causing an infinite loop). Therefore, teach "fetch-pack" not to lazy fetch whenever iterating over refs. This is done by using the raw form of ref iteration and by dereferencing tags ourselves. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-08-11Merge branch 'tb/upload-pack-filters'Libravatar Junio C Hamano1-0/+33
The component to respond to "git fetch" request is made more configurable to selectively allow or reject object filtering specification used for partial cloning. * tb/upload-pack-filters: t5616: use test_i18ngrep for upload-pack errors upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth' upload-pack.c: allow banning certain object filter(s) list_objects_filter_options: introduce 'list_object_filter_config_name'
2020-08-05t5616: use test_i18ngrep for upload-pack errorsLibravatar Jeff King1-4/+4
The tests added to t5616 in 6dd3456a8c (upload-pack.c: allow banning certain object filter(s), 2020-08-03) can fail racily, but only with GETTEXT_POISON enabled. The tests in question look something like this: test_must_fail ok=sigpipe git clone --filter=blob:none ... 2>err && grep "filter blob:none not supported' err The remote upload-pack process writes that error message both as an ERR packet, but also via a die() message. In theory we should see the message twice in the "err" file. The client relays the message from the packet to its stderr (with a "remote error:" prefix), and because this is a local-system clone, upload-pack's stderr goes to the same place. But because clone may be writing to the pipe when upload-pack calls die(), it may get SIGPIPE and fail to relay the message. That's why we need our "ok=sigpipe" trick. But our grep should still work reliably in that case. Either: - we got SIGPIPE on the client, which means upload-pack completed its die(), and we'll see that version of the message. - the client didn't get SIGPIPE, and so it successfully relays the message. In theory we'd see both copies of the message in the second case. But now always! As soon as the client sees ERR, it exits and we run grep. But we have no guarantee that the upload-pack process has exited at this point, or even written its die() message. We might only see the client version of the message. Normally that's OK. We only need to see one or the other to pass the test. But now consider GETTEXT_POISON. upload-pack doesn't translate the die() message nor the ERR packet. But once the client receives it, it calls: die(_("remote error: %s"), buffer + 4); That message _is_ marked for translation. Normally we'd just replace the "remote error:" portion of it, but in GETTEXT_POISON mode, we replace the whole thing with "# GETTEXT POISON #" and don't include the "%s" part at all. So the whole text from the ERR packet is dropped, and so we may racily see a test failure if upload-pack's die() call wasn't yet written. We can fix it by using test_i18ngrep, which just makes this grep a noop in the poison mode. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-08-03upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'Libravatar Taylor Blau1-0/+9
In b79cf959b2 (upload-pack.c: allow banning certain object filter(s), 2020-02-26), we introduced functionality to disallow certain object filters from being chosen from within 'git upload-pack'. Traditionally, administrators use this functionality to disallow filters that are known to perform slowly, for e.g., those that do not have bitmap-level filtering. In the past, the '--filter=tree:<n>' was one such filter that does not have bitmap-level filtering support, and so was likely to be banned by administrators. However, in the previous couple of commits, we introduced bitmap-level filtering for the case when 'n' is equal to '0', i.e., as if we had a '--filter=tree:none' choice. While it would be sufficient to simply write $ git config uploadpackfilter.tree.allow true (since it would allow all values of 'n'), we would like to be able to allow this filter for certain values of 'n', i.e., those no greater than some pre-specified maximum. In order to do this, introduce a new configuration key, as follows: $ git config uploadpackfilter.tree.maxDepth <m> where '<m>' specifies the maximum allowed value of 'n' in the filter 'tree:n'. Administrators who wish to allow for only the value '0' can write: $ git config uploadpackfilter.tree.allow true $ git config uploadpackfilter.tree.maxDepth 0 which allows '--filter=tree:0', but no other values. Signed-off-by: Taylor Blau <me@ttaylorr.com> Acked-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-08-03upload-pack.c: allow banning certain object filter(s)Libravatar Taylor Blau1-0/+24
Git clients may ask the server for a partial set of objects, where the set of objects being requested is refined by one or more object filters. Server administrators can configure 'git upload-pack' to allow or ban these filters by setting the 'uploadpack.allowFilter' variable to 'true' or 'false', respectively. However, administrators using bitmaps may wish to allow certain kinds of object filters, but ban others. Specifically, they may wish to allow object filters that can be optimized by the use of bitmaps, while rejecting other object filters which aren't and represent a perceived performance degradation (as well as an increased load factor on the server). Allow configuring 'git upload-pack' to support object filters on a case-by-case basis by introducing two new configuration variables: - 'uploadpackfilter.allow' - 'uploadpackfilter.<kind>.allow' where '<kind>' may be one of 'blobNone', 'blobLimit', 'tree', and so on. Setting the second configuration variable for any valid value of '<kind>' explicitly allows or disallows restricting that kind of object filter. If a client requests the object filter <kind> and the respective configuration value is not set, 'git upload-pack' will default to the value of 'uploadpackfilter.allow', which itself defaults to 'true' to maintain backwards compatibility. Note that this differs from 'uploadpack.allowfilter', which controls whether or not the 'filter' capability is advertised. Helped-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-07-16upload-pack: do not lazy-fetch "have" objectsLibravatar Jonathan Tan1-0/+38
When upload-pack receives a request containing "have" hashes, it (among other things) checks if the served repository has the corresponding objects. However, it does not do so with the OBJECT_INFO_SKIP_FETCH_OBJECT flag, so if serving a partial clone, a lazy fetch will be triggered first. This was discovered at $DAYJOB when a user fetched from a partial clone (into another partial clone - although this would also happen if the repo to be fetched into is not a partial clone). Therefore, whenever "have" hashes are checked for existence, pass the OBJECT_INFO_SKIP_FETCH_OBJECT flag. Also add the OBJECT_INFO_QUICK flag to improve performance, as it is typical that such objects do not exist in the serving repo, and the consequences of a false negative are minor (usually, a slightly larger pack sent). Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-05-13Merge branch 'cc/upload-pack-v2-fetch-fix'Libravatar Junio C Hamano1-4/+3
Serving a "git fetch" client over "git://" and "ssh://" protocols using the on-wire protocol version 2 was buggy on the server end when the client needs to make a follow-up request to e.g. auto-follow tags. * cc/upload-pack-v2-fetch-fix: upload-pack: clear filter_options for each v2 fetch command
2020-05-08upload-pack: clear filter_options for each v2 fetch commandLibravatar Christian Couder1-4/+3
Because of the request/response model of protocol v2, the upload_pack_v2() function is sometimes called twice in the same process, while 'struct list_objects_filter_options filter_options' was declared as static at the beginning of 'upload-pack.c'. This made the check in list_objects_filter_die_if_populated(), which is called by process_args(), fail the second time upload_pack_v2() is called, as filter_options had already been populated the first time. To fix that, filter_options is not static any more. It's now owned directly by upload_pack(). It's now also part of 'struct upload_pack_data', so that it's owned indirectly by upload_pack_v2(). In the long term, the goal is to also have upload_pack() use 'struct upload_pack_data', so adding filter_options to this struct makes more sense than to have it owned directly by upload_pack_v2(). This fixes the first of the 2 bugs documented by d0badf8797 (partial-clone: demonstrate bugs in partial fetch, 2020-02-21). Helped-by: Derrick Stolee <dstolee@microsoft.com> Helped-by: Jeff King <peff@peff.net> Helped-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-28Merge branch 'dd/test-with-busybox'Libravatar Junio C Hamano1-1/+1
Various tests have been updated to work around issues found with shell utilities that come with busybox etc. * dd/test-with-busybox: t5703: feed raw data into test-tool unpack-sideband t4124: tweak test so that non-compliant diff(1) can also be used t7063: drop non-POSIX argument "-ls" from find(1) t5616: use rev-parse instead to get HEAD's object_id t5003: skip conversion test if unzip -a is unavailable t5003: drop the subshell in test_lazy_prereq test-lib-functions: test_cmp: eval $GIT_TEST_CMP t4061: use POSIX compliant regex(7)
2020-04-22Merge branch 'jk/use-quick-lookup-in-clone-for-tag-following'Libravatar Junio C Hamano1-0/+8
The logic to auto-follow tags by "git clone --single-branch" was not careful to avoid lazy-fetching unnecessary tags, which has been corrected. * jk/use-quick-lookup-in-clone-for-tag-following: clone: use "quick" lookup while following tags
2020-04-01clone: use "quick" lookup while following tagsLibravatar Jeff King1-0/+8
When cloning with --single-branch, we implement git-fetch's usual tag-following behavior, grabbing any tag objects that point to objects we have locally. When we're a partial clone, though, our has_object_file() check will actually lazy-fetch each tag. That not only defeats the purpose of --single-branch, but it does it incredibly slowly, potentially kicking off a new fetch for each tag. This is even worse for a shallow clone, which implies --single-branch, because even tags which are supersets of each other will be fetched individually. We can fix this by passing OBJECT_INFO_SKIP_FETCH_OBJECT to the call, which is what git-fetch does in this case. Likewise, let's include OBJECT_INFO_QUICK, as that's what git-fetch does. The rationale is discussed in 5827a03545 (fetch: use "quick" has_sha1_file for tag following, 2016-10-13), but here the tradeoff would apply even more so because clone is very unlikely to be racing with another process repacking our newly-created repository. This may provide a very small speedup even in the non-partial case case, as we'd avoid calling reprepare_packed_git() for each tag (though in practice, we'd only have a single packfile, so that reprepare should be quite cheap). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-03-25t5616: use rev-parse instead to get HEAD's object_idLibravatar Đoàn Trần Công Danh1-1/+1
Only HEAD's object_id is necessary, rev-list is an overkill. Despite POSIX requires grep(1) treat single pattern with <newline> as multiple patterns. busybox's grep(1) (as of v1.31.1) haven't implemented it yet. Use rev-parse to simplify the test and avoid busybox unimplemented features. Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-03-05Merge branch 'js/ci-windows-update'Libravatar Junio C Hamano1-17/+21
Updates to the CI settings. * js/ci-windows-update: Azure Pipeline: switch to the latest agent pools ci: prevent `perforce` from being quarantined t/lib-httpd: avoid using macOS' sed
2020-03-02Merge branch 'ds/partial-clone-fixes'Libravatar Junio C Hamano1-0/+31
Fix for a bug revealed by a recent change to make the protocol v2 the default. * ds/partial-clone-fixes: partial-clone: avoid fetching when looking for objects partial-clone: demonstrate bugs in partial fetch
2020-02-27t/lib-httpd: avoid using macOS' sedLibravatar Johannes Schindelin1-17/+21
Among other differences relative to GNU sed, macOS' sed always ends its output with a trailing newline, even if the input did not have such a trailing newline. Surprisingly, this makes three httpd-based tests fail on macOS: t5616, t5702 and t5703. ("Surprisingly" because those tests have been around for some time, but apparently nobody runs them on macOS with a working Apache2 setup.) The reason is that we use `sed` in those tests to filter the response of the web server. Apart from the fact that we use GNU constructs (such as using a space after the `c` command instead of a backslash and a newline), we have another problem: macOS' sed LF-only newlines while webservers are supposed to use CR/LF ones. Even worse, t5616 uses `sed` to replace a binary part of the response with a new binary part (kind of hoping that the replaced binary part does not contain a 0x0a byte which would be interpreted as a newline). To that end, it calls on Perl to read the binary pack file and hex-encode it, then calls on `sed` to prefix every hex digit pair with a `\x` in order to construct the text that the `c` statement of the `sed` invocation is supposed to insert. So we call Perl and sed to construct a sed statement. The final nail in the coffin is that macOS' sed does not even interpret those `\x<hex>` constructs. Let's just replace all of that by Perl snippets. With Perl, at least, we do not have to deal with GNU vs macOS semantics, we do not have to worry about unwanted trailing newlines, and we do not have to spawn commands to construct arguments for other commands to be spawned (i.e. we can avoid a whole lot of shell scripting complexity). The upshot is that this fixes t5616, t5702 and t5703 on macOS with Apache2. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-22partial-clone: avoid fetching when looking for objectsLibravatar Derrick Stolee1-1/+1
When using partial clone, find_non_local_tags() in builtin/fetch.c checks each remote tag to see if its object also exists locally. There is no expectation that the object exist locally, but this function nevertheless triggers a lazy fetch if the object does not exist. This can be extremely expensive when asking for a commit, as we are completely removed from the context of the non-existent object and thus supply no "haves" in the request. 6462d5eb9a (fetch: remove fetch_if_missing=0, 2019-11-05) removed a global variable that prevented these fetches in favor of a bitflag. However, some object existence checks were not updated to use this flag. Update find_non_local_tags() to use OBJECT_INFO_SKIP_FETCH_OBJECT in addition to OBJECT_INFO_QUICK. The _QUICK option only prevents repreparing the pack-file structures. We need to be extremely careful about supplying _SKIP_FETCH_OBJECT when we expect an object to not exist due to updated refs. This resolves a broken test in t5616-partial-clone.sh. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-22partial-clone: demonstrate bugs in partial fetchLibravatar Derrick Stolee1-0/+31
While testing partial clone, I noticed some odd behavior. I was testing a way of running 'git init', followed by manually configuring the remote for partial clone, and then running 'git fetch'. Astonishingly, I saw the 'git fetch' process start asking the server for multiple rounds of pack-file downloads! When tweaking the situation a little more, I discovered that I could cause the remote to hang up with an error. Add two tests that demonstrate these two issues. In the first test, we find that when fetching with blob filters from a repository that previously did not have any tags, the 'git fetch --tags origin' command fails because the server sends "multiple filter-specs cannot be combined". This only happens when using protocol v2. In the second test, we see that a 'git fetch origin' request with several ref updates results in multiple pack-file downloads. This must be due to Git trying to fault-in the objects pointed by the refs. What makes this matter particularly nasty is that this goes through the do_oid_object_info_extended() method, so there are no "haves" in the negotiation. This leads the remote to send every reachable commit and tree from each new ref, providing a quadratic amount of data transfer! This test is fixed if we revert 6462d5eb9a (fetch: remove fetch_if_missing=0, 2019-11-05), but that revert causes other test failures. The real fix will need more care. The tests are ordered in this way because if I swap the test order the tag test will succeed instead of fail. I believe this is because somehow we need the srv.bare repo to not have any tags when we clone, but then have tags in our next fetch. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-01-27t5616: make robust to delta base changeLibravatar Jonathan Tan1-13/+23
Commit 6462d5eb9a ("fetch: remove fetch_if_missing=0", 2019-11-08) contains a test that relies on having to lazily fetch the delta base of a blob, but assumes that the tree being fetched (as part of the test) is sent as a non-delta object. This assumption may not hold in the future; for example, a change in the length of the object hash might result in the tree being sent as a delta instead. Make the test more robust by relying on having to lazily fetch the delta base of the tree instead, and by making no assumptions on whether the blobs are sent as delta or non-delta. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-12-01Merge branch 'jt/fetch-remove-lazy-fetch-plugging'Libravatar Junio C Hamano1-0/+70
"git fetch" codepath had a big "do not lazily fetch missing objects when I ask if something exists" switch. This has been corrected by marking the "does this thing exist?" calls with "if not please do not lazily fetch it" flag. * jt/fetch-remove-lazy-fetch-plugging: promisor-remote: remove fetch_if_missing=0 clone: remove fetch_if_missing=0 fetch: remove fetch_if_missing=0
2019-11-08fetch: remove fetch_if_missing=0Libravatar Jonathan Tan1-0/+70
In fetch_pack() (and all functions it calls), pass OBJECT_INFO_SKIP_FETCH_OBJECT whenever we query an object that could be a tree or blob that we do not want to be lazy-fetched even if it is absent. Thus, the only lazy-fetches occurring for trees and blobs are when resolving deltas. Thus, we can remove fetch_if_missing=0 from builtin/fetch.c. Remove this, and also add a test ensuring that such objects are not lazy-fetched. (We might be able to remove fetch_if_missing=0 from other places too, but I have limited myself to builtin/fetch.c in this commit because I have not written tests for the other commands yet.) Note that commits and tags may still be lazy-fetched. I limited myself to objects that could be trees or blobs here because Git does not support creating such commit- and tag-excluding clones yet, and even if such a clone were manually created, Git does not have good support for fetching a single commit (when fetching a commit, it and all its ancestors would be sent). Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-10-16fetch-pack: write fetched refs to .promisorLibravatar Jonathan Tan1-0/+8
The specification of promisor packfiles (in partial-clone.txt) states that the .promisor files that accompany packfiles do not matter (just like .keep files), so whenever a packfile is fetched from the promisor remote, Git has been writing empty .promisor files. But these files could contain more useful information. So instead of writing empty files, write the refs fetched to these files. This makes it easier to debug issues with partial clones, as we can identify what refs (and their associated hashes) were fetched at the time the packfile was downloaded, and if necessary, compare those hashes against what the promisor remote reports now. This is implemented by teaching fetch-pack to write its own non-empty .promisor file whenever it knows the name of the pack's lockfile. This covers the case wherein the user runs "git fetch" with an internal protocol or HTTP protocol v2 (fetch_refs_via_pack() in transport.c sets lock_pack) and with HTTP protocol v0/v1 (fetch_git() in remote-curl.c passes "--lock-pack" to "fetch-pack"). Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Acked-by: Josh Steadmon <steadmon@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-10-07Merge branch 'jk/partial-clone-sparse-blob'Libravatar Junio C Hamano1-0/+36
The name of the blob object that stores the filter specification for sparse cloning/fetching was interpreted in a wrong place in the code, causing Git to abort. * jk/partial-clone-sparse-blob: list-objects-filter: use empty string instead of NULL for sparse "base" list-objects-filter: give a more specific error sparse parsing error list-objects-filter: delay parsing of sparse oid t5616: test cloning/fetching with sparse:oid=<oid> filter
2019-09-18Merge branch 'md/list-objects-filter-combo'Libravatar Junio C Hamano1-0/+19
The list-objects-filter API (used to create a sparse/lazy clone) learned to take a combined filter specification. * md/list-objects-filter-combo: list-objects-filter-options: make parser void list-objects-filter-options: clean up use of ALLOC_GROW list-objects-filter-options: allow mult. --filter strbuf: give URL-encoding API a char predicate fn list-objects-filter-options: make filter_spec a string_list list-objects-filter-options: move error check up list-objects-filter: implement composite filters list-objects-filter-options: always supply *errbuf list-objects-filter: put omits set in filter struct list-objects-filter: encapsulate filter components
2019-09-18Merge branch 'cc/multi-promisor'Libravatar Junio C Hamano1-2/+2
Teach the lazy clone machinery that there can be more than one promisor remote and consult them in order when downloading missing objects on demand. * cc/multi-promisor: Move core_partial_clone_filter_default to promisor-remote.c Move repository_format_partial_clone to promisor-remote.c Remove fetch-object.{c,h} in favor of promisor-remote.{c,h} remote: add promisor and partial clone config to the doc partial-clone: add multiple remotes in the doc t0410: test fetching from many promisor remotes builtin/fetch: remove unique promisor remote limitation promisor-remote: parse remote.*.partialclonefilter Use promisor_remote_get_direct() and has_promisor_remote() promisor-remote: use repository_format_partial_clone promisor-remote: add promisor_remote_reinit() promisor-remote: implement promisor_remote_get_direct() Add initial support for many promisor remotes fetch-object: make functions return an error code t0410: remove pipes after git commands
2019-09-16list-objects-filter: give a more specific error sparse parsing errorLibravatar Jon Simons1-1/+1
The sparse:oid filter has two error modes: we might fail to resolve the name to an OID, or we might fail to parse the contents of that OID. In the latter case, let's give a less generic error message, and mention the OID we did find. While we're here, let's also mark both messages as translatable. Signed-off-by: Jeff King <peff@peff.net> Acked-by: Jeff Hostetler <jeffhost@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-09-16list-objects-filter: delay parsing of sparse oidLibravatar Jeff King1-2/+2
The list-objects-filter code has two steps to its initialization: 1. parse_list_objects_filter() makes sure the spec is a filter we know about and is syntactically correct. This step is done by "rev-list" or "upload-pack" that is going to apply a filter, but also by "git clone" or "git fetch" before they send the spec across the wire. 2. list_objects_filter__init() runs the type-specific initialization (using function pointers established in step 1). This happens at the start of traverse_commit_list_filtered(), when we're about to actually use the filter. It's a good idea to parse as much as we can in step 1, in order to catch problems early (e.g., a blob size limit that isn't a number). But one thing we _shouldn't_ do is resolve any oids at that step (e.g., for sparse-file contents specified by oid). In the case of a fetch, the oid has to be resolved on the remote side. The current code does resolve the oid during the parse phase, but ignores any error (which we must do, because we might just be sending the spec across the wire). This leads to two bugs: - if we're not in a repository (e.g., because it's git-clone parsing the spec), then we trigger a BUG() trying to resolve the name - if we did hit the error case, we still have to notice that later and bail. The code path in rev-list handles this, but the one in upload-pack does not, leading to a segfault. We can fix both by moving the oid resolution into the sparse-oid init function. At that point we know we have a repository (because we're about to traverse), and handling the error there fixes the segfault. As a bonus, we can drop the NULL sparse_oid_value check in rev-list, since this is now handled in the sparse-oid-filter init function. Signed-off-by: Jeff King <peff@peff.net> Acked-by: Jeff Hostetler <jeffhost@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-09-16t5616: test cloning/fetching with sparse:oid=<oid> filterLibravatar Jon Simons1-0/+36
We test in t5317 that "sparse:oid" filters work with rev-list, but there's no coverage at all confirming that they work with a fetch or clone (and in fact, there are several bugs). Let's do a basic test that a clone fetches the correct objects. [jk: extracted from Jon's earlier fix patches. I also simplified the setup down to a single sparse file, and I added checks that we got the right blobs] Signed-off-by: Jon Simons <jon@jonsimons.org> Signed-off-by: Jeff King <peff@peff.net> Acked-by: Jeff Hostetler <jeffhost@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-08-02t: warn against adding non-httpd-specific tests after sourcing 'lib-httpd'Libravatar SZEDER Gábor1-0/+3
We have a couple of test scripts that are not completely httpd-specific, but do run a few httpd-specific tests at the end. These test scripts source 'lib-httpd.sh' somewhere mid-script, which then skips all the rest of the test script if the dependencies for running httpd tests are not fulfilled. As the previous two patches in this series show, already on two occasions non-httpd-specific tests were appended at the end of such test scripts, and, consequently, they were skipped as well when httpd tests couldn't be run. Add a comment at the end of these test scripts to warn against adding non-httpd-specific tests at the end, in the hope that they will help prevent similar issues in the future. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-06-28list-objects-filter-options: allow mult. --filterLibravatar Matthew DeVore1-0/+19
Allow combining of multiple filters by simply repeating the --filter flag. Before this patch, the user had to combine them in a single flag somewhat awkwardly (e.g. --filter=combine:FOO+BAR), including URL-encoding the individual filters. To make this work, in the --filter flag parsing callback, rather than error out when we detect that the filter_options struct is already populated, we modify it in-place to contain the added sub-filter. The existing sub-filter becomes the lhs of the combined filter, and the next sub-filter becomes the rhs. We also have to URL-encode the LHS and RHS sub-filters. We can simplify the operation if the LHS is already a combine: filter. In that case, we just append the URL-encoded RHS sub-filter to the LHS spec to get the new spec. Helped-by: Emily Shaffer <emilyshaffer@google.com> Helped-by: Jeff Hostetler <git@jeffhostetler.com> Helped-by: Jeff King <peff@peff.net> Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Matthew DeVore <matvore@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-06-25promisor-remote: parse remote.*.partialclonefilterLibravatar Christian Couder1-1/+1
This makes it possible to specify a different partial clone filter for each promisor remote. Signed-off-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-06-25Use promisor_remote_get_direct() and has_promisor_remote()Libravatar Christian Couder1-1/+1
Instead of using the repository_format_partial_clone global and fetch_objects() directly, let's use has_promisor_remote() and promisor_remote_get_direct(). This way all the configured promisor remotes will be taken into account, not only the one specified by extensions.partialClone. Also when cloning or fetching using a partial clone filter, remote.origin.promisor will be set to "true" instead of setting extensions.partialClone to "origin". This makes it possible to use many promisor remote just by fetching from them. Signed-off-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-06-11t5616: cover case of client having delta baseLibravatar Jonathan Tan1-11/+28
When fetching into a partial clone, Git first prefetches missing REF_DELTA bases from the promisor remote. (This feature was introduced in [1].) But as can be seen in a recent test coverage report [2], the case in which a REF_DELTA base is already present is not covered by tests. Extend the tests slightly to cover this case. [1] 8a30a1efd1 ("index-pack: prefetch missing REF_DELTA bases", 2019-05-15). [2] https://public-inbox.org/git/396091fc-5572-19a5-4f18-61c258590dd5@gmail.com/ Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-06-11t5616: use correct flag to check object is missingLibravatar Jonathan Tan1-1/+1
If we want to check whether an object is missing, the correct flag to pass to rev-list is --ignore-missing; --exclude-promisor-objects will exclude any object that came from the promisor remote, whether it is present or missing. Use the correct flag. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-05-15index-pack: prefetch missing REF_DELTA basesLibravatar Jonathan Tan1-0/+61
When fetching, the client sends "have" commit IDs indicating that the server does not need to send any object referenced by those commits, reducing network I/O. When the client is a partial clone, the client still sends "have"s in this way, even if it does not have every object referenced by a commit it sent as "have". If a server omits such an object, it is fine: the client could lazily fetch that object before this fetch, and it can still do so after. The issue is when the server sends a thin pack containing an object that is a REF_DELTA against such a missing object: index-pack fails to fix the thin pack. When support for lazily fetching missing objects was added in 8b4c0103a9 ("sha1_file: support lazily fetching missing objects", 2017-12-08), support in index-pack was turned off in the belief that it accesses the repo only to do hash collision checks. However, this is not true: it also needs to access the repo to resolve REF_DELTA bases. Support for lazy fetching should still generally be turned off in index-pack because it is used as part of the lazy fetching process itself (if not, infinite loops may occur), but we do need to fetch the REF_DELTA bases. (When fetching REF_DELTA bases, it is unlikely that those are REF_DELTA themselves, because we do not send "have" when making such fetches.) To resolve this, prefetch all missing REF_DELTA bases before attempting to resolve them. This both ensures that all bases are attempted to be fetched, and ensures that we make only one request per index-pack invocation, and not one request per missing object. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-05-15t5616: refactor packfile replacementLibravatar Jonathan Tan1-13/+21
A subsequent patch will perform the same packfile replacement that is already done twice, so refactor it into its own function. Also, the same subsequent patch will use, in another way, part of the packfile replacement functionality, so extract those out too. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-03-14tests: use 'test_atexit' to stop httpdLibravatar SZEDER Gábor1-2/+0
Use 'test_atexit' to run cleanup commands to stop httpd at the end of the test script or upon interrupt or failure, as it is shorter, simpler, and more robust than registering such cleanup commands in the trap on EXIT in the test scripts. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-01-04Merge branch 'nd/i18n'Libravatar Junio C Hamano1-1/+1
More _("i18n") markings. * nd/i18n: fsck: mark strings for translation fsck: reduce word legos to help i18n parse-options.c: mark more strings for translation parse-options.c: turn some die() to BUG() parse-options: replace opterror() with optname() repack: mark more strings for translation remote.c: mark messages for translation remote.c: turn some error() or die() to BUG() reflog: mark strings for translation read-cache.c: add missing colon separators read-cache.c: mark more strings for translation read-cache.c: turn die("internal error") to BUG() attr.c: mark more string for translation archive.c: mark more strings for translation alias.c: mark split_cmdline_strerror() strings for translation git.c: mark more strings for translation
2018-10-30Merge branch 'md/filter-trees'Libravatar Junio C Hamano1-0/+42
The "rev-list --filter" feature learned to exclude all trees via "tree:0" filter. * md/filter-trees: list-objects: support for skipping tree traversal filter-trees: code clean-up of tests list-objects-filter: implement filter tree:0 list-objects-filter-options: do not over-strbuf_init list-objects-filter: use BUG rather than die revision: mark non-user-given objects instead rev-list: handle missing tree objects properly list-objects: always parse trees gently list-objects: refactor to process_tree_contents list-objects: store common func args in struct
2018-10-19Merge branch 'jt/fetch-tips-in-partial-clone'Libravatar Junio C Hamano1-0/+17
"git fetch $repo $object" in a partial clone did not correctly fetch the asked-for object that is referenced by an object in promisor packfile, which has been fixed. * jt/fetch-tips-in-partial-clone: fetch: in partial clone, check presence of targets connected: document connectivity in partial clones
2018-10-15filter-trees: code clean-up of testsLibravatar Matthew DeVore1-1/+1
A few trivial updates to test to match the current best practices. - avoid "grep -q" that strips potentially useful output from tests running under "-v". - use test_write_lines to prepare multi-line expected output file. - reserve use of test_must_fail to "git" commands. Signed-off-by: Matthew DeVore <matvore@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-10-07list-objects-filter: implement filter tree:0Libravatar Matthew DeVore1-0/+42
Teach list-objects the "tree:0" filter which allows for filtering out all tree and blob objects (unless other objects are explicitly specified by the user). The purpose of this patch is to allow smaller partial clones. The name of this filter - tree:0 - does not explicitly specify that it also filters out all blobs, but this should not cause much confusion because blobs are not at all useful without the trees that refer to them. I also considered only:commits as a name, but this is inaccurate because it suggests that annotated tags are omitted, but actually they are included. The name "tree:0" allows later filtering based on depth, i.e. "tree:1" would filter out all but the root tree and blobs. In order to avoid confusion between 0 and capital O, the documentation was worded in a somewhat round-about way that also hints at this future improvement to the feature. Signed-off-by: Matthew DeVore <matvore@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-10-07tests: order arguments to git-rev-list properlyLibravatar Matthew DeVore1-11/+15
It is a common mistake to put positional arguments before flags when invoking git-rev-list. Order the positional arguments last. This patch skips git-rev-list invocations which include the --not flag, since the ordering of flags and positional arguments affects the behavior. This patch also skips invocations of git-rev-list that occur in command substitution in which the exit code is discarded, since fixing those properly will require a more involved cleanup. Signed-off-by: Matthew DeVore <matvore@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-10-07tests: don't swallow Git errors upstream of pipesLibravatar Matthew DeVore1-6/+8
Some pipes in tests lose the exit code of git processes, which can mask unexpected behavior like crashes. Split these pipes up so that git commands are only at the end of pipes rather than the beginning or middle. The violations fixed in this patch were found in the process of fixing pipe placement in a prior patch. Signed-off-by: Matthew DeVore <matvore@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-10-07tests: standardize pipe placementLibravatar Matthew DeVore1-13/+19
Instead of using a line-continuation and pipe on the second line, take advantage of the shell's implicit line continuation after a pipe character. So for example, instead of some long line \ | next line use some long line | next line And add a blank line before and after the pipe where it aids readability (it usually does). This better matches the coding style documented in Documentation/CodingGuidelines and used in shell scripts elsewhere in the tree. Signed-off-by: Matthew DeVore <matvore@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-09-21fetch: in partial clone, check presence of targetsLibravatar Jonathan Tan1-0/+17
When fetching an object that is known as a promisor object to the local repository, the connectivity check in quickfetch() in builtin/fetch.c succeeds, causing object transfer to be bypassed. However, this should not happen if that object is merely promised and not actually present. Because this happens, when a user invokes "git fetch origin <sha-1>" on the command-line, the <sha-1> object may not actually be fetched even though the command returns an exit code of 0. This is a similar issue (but with a different cause) to the one fixed by a0c9016abd ("upload-pack: send refs' objects despite "filter"", 2018-07-09). Therefore, update quickfetch() to also directly check for the presence of all objects to be fetched. Its documentation and name are also updated to better reflect what it does. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>