On 3/4/2022 6:35 PM, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > >> From: Derrick Stolee <derrickstolee@xxxxxxxxxx> >> >> A previous change allowed Git to parse bundles with the 'filter' >> capability. Now, teach Git to create bundles with this option. >> >> Some rearranging of code is required to get the option parsing in the >> correct spot. There are now two reasons why we might need capabilities >> (a new hash algorithm or an object filter) so that is pulled out into a >> place where we can check both at the same time. >> >> The --filter option is parsed as part of setup_revisions(), but it >> expected the --objects flag, too. That flag is somewhat implied by 'git >> bundle' because it creates a pack-file walking objects, but there is >> also a walk that walks the revision range expecting only commits. Make >> this parsing work by setting 'revs.tree_objects' and 'revs.blob_objects' >> before the call to setup_revisions(). >> >> Signed-off-by: Derrick Stolee <derrickstolee@xxxxxxxxxx> >> --- > > Now, the gem of the series ;-) :D >> + if (the_hash_algo != &hash_algos[GIT_HASH_SHA1] || >> + revs.filter) > > Did we need to wrap? With these on a single line, the line is way > shorter than the line with "because our hash algorithm is not" on > it. Perhaps I was thinking about having one line per "reason", which might be extended in the future. But there's no reason to waste space right now. >> + /* >> + * Nullify the filter here, and any object walking. We only care >> + * about commits and tags here. The revs_copy has the right >> + * instances of these values. >> + */ >> + revs.filter = NULL; >> revs.blob_objects = revs.tree_objects = 0; >> traverse_commit_list(&revs, write_bundle_prerequisites, ignore_object, &bpi); >> object_array_remove_duplicates(&revs_copy.pending); > > OK. We prepare revs, and we save it to revs_copy, because we > perform two traversals, one to determine which bottom commits are > required to unbundle the bundle (which is done with the instance > "revs"), and then later to actually enumerate the objects to place > in the bundle (using "revs_copy"). Is there a reason why we need to > remove .filter in order to perform the first traversal? > > This is a tangent, but I wish we could reliably determine when we > can optimize the first traversal away, by inspecting revs. If there > are any pending objects with UNINTERESTING bit, or members like > max_count, max_age, min_age are set, we'd end up traversing down to > all roots and the prerequisites list would be empty. Noted for potential follow-up. >> + test_expect_success 'filtered bundle: $filter' ' ... > > OK. > > It is somewhat curious why our bundle tests do not unbundle and > check the resulting contents of the repository we unbundle it in. I haven't checked your response yet, but hopefully this is answered in the next patch which teaches Git how to unbundle bundles with this new capability. Thanks, -Stolee
