| Message ID | 20250204-toon-clone-refs-v5-0-37e34af283c8@iotcl.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Enable doing a shallow clone of a specific git revision | expand |
On Tue, Feb 04, 2025 at 10:33:59PM +0100, Toon Claes wrote: > Changes in v5: > - Add separate commit to introduce die_for_incompatible_opt2() > - Small tweaks in documentation about `--[no-]tags` and `--revision`. > - Better explain the refactoring of wanted_peer_refs() in the commit > message. > - Change type from `int` to `size_t` in wanted_peer_refs(). > - Use lookup_commit_or_die() instead lookup_commit_reference() to avoid > checking the result ourself. > - Add a few code comments to explain some things. > - Stylish cleanups like removal of unneeded empty lines, commented out > test-code and remarks. > - Link to v4: https://lore.kernel.org/r/20250131-toon-clone-refs-v4-0-2a4ff851498f@iotcl.com I've got a couple more nits, but this version looks mostly good to me. Thanks! Patrick
Toon Claes <toon@iotcl.com> writes: > The goal of this series is to add an option `--revision` to > git-clone(1). > > This series starts with a handful of preparatory refactoring commits > that make it more straight-forward to add this new option. In the last > commit we're actually adding the feature. > > This series sets an example on how I think we can further refactor > builtin/clone.c to increase the maintainability of the code. > > --- > Changes in v5: > - Add separate commit to introduce die_for_incompatible_opt2() > - Small tweaks in documentation about `--[no-]tags` and `--revision`. > - Better explain the refactoring of wanted_peer_refs() in the commit > message. > - Change type from `int` to `size_t` in wanted_peer_refs(). > - Use lookup_commit_or_die() instead lookup_commit_reference() to avoid > checking the result ourself. > - Add a few code comments to explain some things. > - Stylish cleanups like removal of unneeded empty lines, commented out > test-code and remarks. > - Link to v4: https://lore.kernel.org/r/20250131-toon-clone-refs-v4-0-2a4ff851498f@iotcl.com Looking good. Queued.
The goal of this series is to add an option `--revision` to git-clone(1). This series starts with a handful of preparatory refactoring commits that make it more straight-forward to add this new option. In the last commit we're actually adding the feature. This series sets an example on how I think we can further refactor builtin/clone.c to increase the maintainability of the code. --- Changes in v5: - Add separate commit to introduce die_for_incompatible_opt2() - Small tweaks in documentation about `--[no-]tags` and `--revision`. - Better explain the refactoring of wanted_peer_refs() in the commit message. - Change type from `int` to `size_t` in wanted_peer_refs(). - Use lookup_commit_or_die() instead lookup_commit_reference() to avoid checking the result ourself. - Add a few code comments to explain some things. - Stylish cleanups like removal of unneeded empty lines, commented out test-code and remarks. - Link to v4: https://lore.kernel.org/r/20250131-toon-clone-refs-v4-0-2a4ff851498f@iotcl.com Changes in v4: - Introduce a new commit to reduce the use of global variables. - Introduce a new commit to invert the flag --no-tags to --tags. - Introduce a new commit to refactor wanted_peer_refs() in builtin/clone.c. - Introduce a new commit to shuffle the handling of tags refspec. - Introduce a new commit to introduce a `struct clone_opts`. - Link to v3: https://lore.kernel.org/r/20241219-toon-clone-refs-v3-1-1484faea3008@iotcl.com Changes in v3: - Fail early when the revision was not found on the remote, instead of creating a clone that's in an invalid state. - State more clearly in the commit message adding this option is useful for a not uncommon use-case. - Be explicit in the documentation the ref needs to peel down to a commit. - Die in case we try to update_head() to an object that's not a commit. - Allow combining `--revision` with `--bare`. - Add die_for_incompatible_opt2() to parse-options.h and use it for the options that are not compatible with the new `--revision` option. - Small tweaks to the added tests. - Small touchups on commit messages. - Link to v2: https://lore.kernel.org/r/20241129-toon-clone-refs-v2-1-dca4c19a3510@iotcl.com --- Toon Claes (7): clone: cut down on global variables in clone.c clone: make it possible to specify --tags clone: refactor wanted_peer_refs() clone: add tags refspec earlier to fetch refspec clone: introduce struct clone_opts in builtin/clone.c parse-options: introduce die_for_incompatible_opt2() builtin/clone: teach git-clone(1) the --revision= option Documentation/git-clone.txt | 17 ++- builtin/clone.c | 350 +++++++++++++++++++++++++------------------- builtin/replay.c | 3 +- parse-options.h | 9 ++ remote.c | 2 +- remote.h | 5 + t/meson.build | 1 + t/t5621-clone-revision.sh | 123 ++++++++++++++++ 8 files changed, 351 insertions(+), 159 deletions(-) --- Range-diff versus v4: 1: a563ae1023 = 1: 43fbfe2a1c clone: cut down on global variables in clone.c 2: 031fec1961 ! 2: 68bbf04606 clone: make it possible to specify --tags @@ Documentation/git-clone.txt: git clone [--template=<template-directory>] [-o <name>] [-b <name>] [-u <upload-pack>] [--reference <repository>] [--dissociate] [--separate-git-dir <git-dir>] - [--depth <depth>] [--[no-]single-branch] [--no-tags] -+ [--depth <depth>] [--[no-]single-branch] [--[no-]-tags] ++ [--depth <depth>] [--[no-]single-branch] [--[no-]tags] [--recurse-submodules[=<pathspec>]] [--[no-]shallow-submodules] [--[no-]remote-submodules] [--jobs <n>] [--sparse] [--[no-]reject-shallow] [--filter=<filter-spec>] [--also-filter-submodules]] [--] <repository> @@ Documentation/git-clone.txt: corresponding `--mirror` and `--no-tags` options in -`--no-tags`:: - Don't clone any tags, and set +`--[no-]tags`:: -+ By default tags are cloned, and passing `--tags` doesn't change that. + With `--no-tags`, no tags are cloned, and set `remote.<remote>.tagOpt=--no-tags` in the config, ensuring that future `git pull` and `git fetch` operations won't follow any tags. Subsequent explicit tag fetches will still work, + (see linkgit:git-fetch[1]). ++ By default tags are cloned, and passing `--tags` doesn't change that. + + + Can be used in conjunction with `--single-branch` to clone and + maintain a branch with no references other than a single cloned ## builtin/clone.c ## @@ 3: b926bcc1df ! 3: ac0babfc2a clone: refactor wanted_peer_refs() @@ Commit message Over time this function grown to be very complex. Refactor it. + Previously, there was a separate code path for when + `option_single_branch` was set. It resulted in duplicated code and + deeper nested conditions. After this refactor the code path for when + `option_single_branch` is truthy modifies `refs` and then falls through + to the common code path. This approach relies on the `refspec` being set + correctly and thus only mapping refs that are relevant. + Signed-off-by: Toon Claes <toon@iotcl.com> ## builtin/clone.c ## @@ builtin/clone.c: static struct ref *wanted_peer_refs(const struct ref *refs, } - if (!option_mirror && !option_single_branch && option_tags) -+ for (int i = 0; i < refspec->nr; i++) ++ for (size_t i = 0; i < refspec->nr; i++) + get_fetch_map(refs, &refspec->items[i], &tail, 0); + + /* 4: 2201b996b3 ! 4: 8a98961fd5 clone: add tags refspec earlier to fetch refspec @@ Commit message In clone.c we call refspec_ref_prefixes() to copy the fetch refspecs from the `remote->fetch` refspec into `ref_prefixes` of - `transport_ls_refs_options`. Afterward we add the tags prefix + `transport_ls_refs_options`. Afterwards we add the tags prefix `refs/tags/` prefix as well. At a later point, in wanted_peer_refs() we process refs using both `remote->fetch` and `TAG_REFSPEC`. @@ builtin/clone.c: static struct ref *wanted_peer_refs(const struct ref *refs, if (!option_branch) refs = to_free = guess_remote_head(head, refs, 0); @@ builtin/clone.c: static struct ref *wanted_peer_refs(const struct ref *refs, - for (int i = 0; i < refspec->nr; i++) + for (size_t i = 0; i < refspec->nr; i++) get_fetch_map(refs, &refspec->items[i], &tail, 0); - /* @@ builtin/clone.c: int cmd_clone(int argc, strvec_push(&transport_ls_refs_options.ref_prefixes, "HEAD"); + + if (option_tags || option_branch) ++ /* ++ * Add tags refspec when user asked for tags (implicitly) or ++ * specified --branch, which argument might be a tag. ++ */ + refspec_append(&remote->fetch, TAG_REFSPEC); + refspec_ref_prefixes(&remote->fetch, 5: 14fb827c41 ! 5: b338eec186 clone: introduce struct clone_opts in builtin/clone.c @@ builtin/clone.c: static struct ref *find_remote_branch(const struct ref *refs, c + struct ref *head = copy_ref(find_ref_by_name(refs, "HEAD")); + if (head) + tail_link_ref(head, &tail); -+ + if (option_single_branch) refs = to_free = guess_remote_head(head, refs, 0); - else { @@ builtin/clone.c: static struct ref *find_remote_branch(const struct ref *refs, c - tail = &local_refs; - refs = to_free = copy_ref(find_remote_branch(refs, option_branch)); - } -+ } -+ -+ else if (option_single_branch) { ++ } else if (option_single_branch) { + local_refs = NULL; + tail = &local_refs; + refs = to_free = copy_ref(find_remote_branch(refs, option_branch)); } - for (int i = 0; i < refspec->nr; i++) + for (size_t i = 0; i < refspec->nr; i++) @@ builtin/clone.c: int cmd_clone(int argc, struct string_list server_options = STRING_LIST_INIT_NODUP; const char *bundle_uri = NULL; @@ remote.h: struct ref *alloc_ref(const char *name); struct ref *copy_ref(const struct ref *ref); struct ref *copy_ref_list(const struct ref *ref); int count_refspec_match(const char *, struct ref *refs, struct ref **matched_ref); ++/* ++ * Put a ref in the tail and prepare tail for adding another one. ++ * *tail is the pointer to the tail of the list of refs. ++ */ +void tail_link_ref(struct ref *ref, struct ref ***tail); int check_ref_type(const struct ref *ref, int flags); -: ---------- > 6: d312865d63 parse-options: introduce die_for_incompatible_opt2() 6: d87d155dfc ! 7: 8ddbc6eb41 builtin/clone: teach git-clone(1) the --revision= option @@ Documentation/git-clone.txt: objects from the source repository into a pack in t `--branch` can also take tags and detaches the `HEAD` at that commit in the resulting repository. -+`--revision` _<rev>_:: ++`--revision=<rev>`:: + Create a new repository, and fetch the history leading to the given + revision _<rev>_ (and nothing else), without making any remote-tracking + branch, and without making any local branch, and point `HEAD` to @@ builtin/clone.c: static void update_remote_refs(const struct ref *refs, if (refs_update_symref(get_main_ref_store(the_repository), "HEAD", our->name, NULL) < 0) die(_("unable to update HEAD")); @@ builtin/clone.c: static void update_head(const struct ref *our, const struct ref *remote, + install_branch_config(0, head, remote_name, our->name); + } } else if (our) { - struct commit *c = lookup_commit_reference(the_repository, - &our->old_oid); -+ -+ if (!c) -+ die(_("couldn't look up commit object for '%s'"), our->name); +- struct commit *c = lookup_commit_reference(the_repository, +- &our->old_oid); ++ struct commit *c = lookup_commit_or_die(&our->old_oid, ++ our->name); + /* --branch specifies a non-branch (i.e. tags), detach HEAD */ refs_update_ref(get_main_ref_store(the_repository), msg, @@ builtin/clone.c: int cmd_clone(int argc, + !!option_branch, "--branch"); + die_for_incompatible_opt2(!!option_rev, "--revision", + option_mirror, "--mirror"); -+ // TODO --no-single-branch + if (reject_shallow) transport_set_option(transport, TRANS_OPT_REJECT_SHALLOW, "1"); @@ builtin/clone.c: int cmd_clone(int argc, - strvec_push(&transport_ls_refs_options.ref_prefixes, "HEAD"); + if (option_rev) { + option_tags = 0; -+ option_branch = 0; + option_single_branch = 0; + opts.wants_head = 0; + opts.detach = 1; @@ builtin/clone.c: int cmd_clone(int argc, + } if (option_tags || option_branch) - refspec_append(&remote->fetch, TAG_REFSPEC); + /* @@ builtin/clone.c: int cmd_clone(int argc, expand_ref_prefix(&transport_ls_refs_options.ref_prefixes, option_branch); @@ builtin/clone.c: int cmd_clone(int argc, + if (!our_head_points_at) + die(_("Remote revision %s not found in upstream %s"), + option_rev, remote_name); -+ //mapped_refs->name[0] = 0; } else if (remote_head_points_at) { our_head_points_at = remote_head_points_at; } else if (remote_head) { @@ builtin/clone.c: int cmd_clone(int argc, /* * We want to show progress for recursive submodule clones iff - ## parse-options.h ## -@@ parse-options.h: static inline void die_for_incompatible_opt3(int opt1, const char *opt1_name, - 0, ""); - } - -+static inline void die_for_incompatible_opt2(int opt1, const char *opt1_name, -+ int opt2, const char *opt2_name) -+{ -+ die_for_incompatible_opt4(opt1, opt1_name, -+ opt2, opt2_name, -+ 0, "", -+ 0, ""); -+} -+ - /* - * Use these assertions for callbacks that expect to be called with NONEG and - * NOARG respectively, and do not otherwise handle the "unset" and "arg" - ## t/meson.build ## @@ t/meson.build: integration_tests = [ 't5617-clone-submodules-remote.sh', --- base-commit: bc204b742735ae06f65bb20291c95985c9633b7f change-id: 20241129-toon-clone-refs-ad3623772f92 Thanks -- Toon