From: Patrick Steinhardt <ps@pks•im>
To: "Samo Pogačnik via GitGitGadget" <gitgitgadget@gmail•com>
Cc: git@vger•kernel.org, "Samo Pogačnik" <samo_pogacnik@t-2•net>
Subject: Re: [PATCH 2/2] shallow: handling fetch relative-deepen
Date: Tue, 6 Jan 2026 08:44:43 +0100 [thread overview]
Message-ID: <aVy9a9f1AZzTbBQa@pks.im> (raw)
In-Reply-To: <b352a33c90ca67f4ad68df08c0fd155ceeeb167c.1765303880.git.gitgitgadget@gmail.com>
On Tue, Dec 09, 2025 at 06:11:20PM +0000, Samo Pogačnik via GitGitGadget wrote:
> From: =?UTF-8?q?Samo=20Poga=C4=8Dnik?= <samo_pogacnik@t-2•net>
>
> When a shallowed repository gets deepened beyond the beginning of a
> merged branch, we may endup with some shallows, that are behind the
s/endup/end up/
s/shallows, that/shallows that/
> reachable ones.
Hm, which reachable ones? Sorry, I can't quite follow, it would help the
reviewer to add a bit more context.
> Added test 'fetching deepen beyond merged branch' exposes that
> behaviour.
>
> On the other hand, it seems that equivalent absolute depth driven
> fetches result in all the correct shallows. That led to this proposal,
> which unifies absolute and relative deepening in a way that the same
> get_shallow_commits() call is used in both cases. The difference is
> only that depth is adapted for relative deepening by measuring
> equivalent depth of current local shallow commits in the current remote
> repo. Thus a new function get_shallows_depth() has been added and the
> function get_reachable_list() became redundant / removed.
>
> The get_shallows_depth() function also shares the logic of the
> get_shallow_commits() function, but it focuses on counting depth of
> each existing shallow commit. The minimum result is stored as
> 'data->deepen_relative', which is set not to be zero for relative
> deepening anyway. That way we can allways summ 'data->deepen_relative'
> and 'depth' values, because 'data->deepen_relative' is always 0 in
> absolute deepening.
I think the commit message needs some polishing. I myself am not that
familiar with our shallow logic, so I'm a bit lost here to be honest.
Typically, a commit message should be self-explanatory and guide the
reader through the problem space as well as the solution. It should, in
the following order:
- Explain what the actual issue is as observed by the user. I'm not
really sure about this part, only that it's something related to
shallow clones, deepening and merge commits.
- Explain what the root cause of the issue is.
- Explain how the root cause is being fixed. Ideally, it should also
explain why that is the correct fix, potentially referencing other
code like you do.
Your commit message on the other hand explains more of the "what" and
less of the "why", which makes it hard to follow. Also, an ASCII commit
graph would probably go a long way in explaining the issue :)
> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 2677cd5faa..d05c45e32b 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -955,6 +955,30 @@ test_expect_success 'fetching deepen' '
> )
> '
>
> +test_expect_success 'fetching deepen beyond merged branch' '
> + test_create_repo shallow-deepen-merged &&
> + (
> + cd shallow-deepen-merged &&
> + git commit --allow-empty -m one &&
> + git commit --allow-empty -m two &&
> + git commit --allow-empty -m three &&
> + git switch -c branch &&
> + git commit --allow-empty -m four &&
> + git commit --allow-empty -m five &&
> + git switch main &&
> + git merge --no-ff branch &&
> + cd - &&
> + git clone --bare --depth 3 "file://$(pwd)/shallow-deepen-merged" deepen.git &&
> + git -C deepen.git fetch origin --deepen=1 &&
> + echo "Shallow:" && cat deepen.git/shallow &&
> + git -C deepen.git rev-list --all >actual &&
> + echo "All rev-lis:" && cat actual &&
This statement and the one two lines further up look like debug code to
me.
> + for commit in $(sed "/^$/d" deepen.git/shallow); do
Nit: loops should be formatted like this:
for commit in ...
do
...
done
> diff --git a/upload-pack.c b/upload-pack.c
> index 2d2b70cbf2..ecd3e7f5ef 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -704,54 +705,82 @@ error:
> return -1;
> }
>
> -static int get_reachable_list(struct upload_pack_data *data,
> - struct object_array *reachable)
> +define_commit_slab(commit_depth, int *);
> +static void free_depth_in_slab(int **ptr)
> {
> - struct child_process cmd = CHILD_PROCESS_INIT;
> - int i;
> - struct object *o;
> - char namebuf[GIT_MAX_HEXSZ + 2]; /* ^ + hash + LF */
> - const unsigned hexsz = the_hash_algo->hexsz;
> - int ret;
> -
> - if (do_reachable_revlist(&cmd, &data->shallows, reachable,
> - data->allow_uor) < 0) {
> - ret = -1;
> - goto out;
> - }
> -
> - while ((i = read_in_full(cmd.out, namebuf, hexsz + 1)) == hexsz + 1) {
> - struct object_id oid;
> - const char *p;
> -
> - if (parse_oid_hex(namebuf, &oid, &p) || *p != '\n')
> - break;
> -
> - o = lookup_object(the_repository, &oid);
> - if (o && o->type == OBJ_COMMIT) {
> - o->flags &= ~TMP_MARK;
> + FREE_AND_NULL(*ptr);
> +}
> +static void get_shallows_depth(struct upload_pack_data *data)
This function looks very similar to `get_shallow_commits()`. Is it
possible to deduplicate the logic?
Thanks!
Patrick
next prev parent reply other threads:[~2026-01-06 7:44 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-12-09 18:11 [PATCH 0/2] shallow: handling fetch relative-deepen Samo Pogačnik via GitGitGadget
2025-12-09 18:11 ` [PATCH 1/2] shallow: free local object_array allocations Samo Pogačnik via GitGitGadget
2026-01-06 7:44 ` Patrick Steinhardt
2026-01-09 16:21 ` Samo Pogačnik
2026-01-09 16:33 ` Patrick Steinhardt
2025-12-09 18:11 ` [PATCH 2/2] shallow: handling fetch relative-deepen Samo Pogačnik via GitGitGadget
2026-01-06 7:44 ` Patrick Steinhardt [this message]
2026-01-09 16:48 ` Samo Pogačnik
2026-01-09 22:23 ` [PATCH v2 0/2] " Samo Pogačnik via GitGitGadget
2026-01-09 22:23 ` [PATCH v2 1/2] shallow: free local object_array allocations Samo Pogačnik via GitGitGadget
2026-01-09 22:23 ` [PATCH v2 2/2] shallow: handling fetch relative-deepen Samo Pogačnik via GitGitGadget
2026-01-10 4:17 ` Junio C Hamano
2026-01-10 5:13 ` [PATCH v3 0/2] " Samo Pogačnik via GitGitGadget
2026-01-10 5:13 ` [PATCH v3 1/2] shallow: free local object_array allocations Samo Pogačnik via GitGitGadget
2026-01-10 5:13 ` [PATCH v3 2/2] shallow: handling fetch relative-deepen Samo Pogačnik via GitGitGadget
2026-01-15 15:50 ` Kristoffer Haugsbakk
2026-01-16 22:30 ` [PATCH v4 0/2] " Samo Pogačnik via GitGitGadget
2026-01-16 22:31 ` [PATCH v4 1/2] shallow: free local object_array allocations Samo Pogačnik via GitGitGadget
2026-01-16 22:31 ` [PATCH v4 2/2] shallow: handling fetch relative-deepen Samo Pogačnik via GitGitGadget
2026-02-11 13:38 ` Patrick Steinhardt
2026-02-13 20:48 ` Samo Pogačnik
2026-02-14 9:40 ` Samo Pogačnik
2026-02-15 11:19 ` Samo Pogačnik
2026-02-15 20:11 ` [PATCH v5 0/2] " Samo Pogačnik via GitGitGadget
2026-02-15 20:11 ` [PATCH v5 1/2] shallow: free local object_array allocations Samo Pogačnik via GitGitGadget
2026-02-15 20:11 ` [PATCH v5 2/2] shallow: handling fetch relative-deepen Samo Pogačnik via GitGitGadget
2026-02-20 22:34 ` [PATCH v5 0/2] " Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aVy9a9f1AZzTbBQa@pks.im \
--to=ps@pks$(echo .)im \
--cc=git@vger$(echo .)kernel.org \
--cc=gitgitgadget@gmail$(echo .)com \
--cc=samo_pogacnik@t-2$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox