[PATCH] fetch: fix --depth= and --unshallow for multi-remote fetches

[PATCH] fetch: fix --depth= and --unshallow for multi-remote fetches

[Rong Bao]

When running `git fetch --all --depth=<SOME-DEPTH>` or `git fetch --all
--unshallow` with Git 2.56.0, the fetch isn't really deepened:

        $ git init remote
        Initialized empty Git repository in /tmp/test/remote/.git/
        $ for i in 1 2 3; do echo "$i" >remote/"$i" && git -C remote add -A && git -C remote commit -m "$i"; done
        [... snip ...]
        $ git clone --depth 1 --no-local remote local
        Cloning into 'local'...
        remote: Enumerating objects: 5, done.
        remote: Counting objects: 100% (5/5), done.
        remote: Compressing objects: 100% (2/2), done.
        remote: Total 5 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
        Receiving objects: 100% (5/5), done.
        $ git -C local log --format='%s'
        3
        $ git -C local remote add origin2 ../remote
        $ git -C local fetch --all --unshallow
        Fetching origin
        Fetching origin2
        From ../remote
         * [new branch]      main       -> origin2/main
        $ git -C local log --format='%s'
        3
        $ git -C local fetch --all --depth=3
        Fetching origin
        Fetching origin2
        $ git -C local log --format='%s'
        3

This is because the parent process doesn't pass the --depth= flag to the
fetcher subprocess.

        [pid 1622822] execve("/usr/lib/git-core/git", ["/usr/lib/git-core/git",
        "-c", "fetch.bundleURI=", "fetch", "--append", "--no-auto-gc",
        "--no-write-commit-graph", "origin"], 0x5555c4b65ae0 /* 54 vars */) = 0

Fix this by passing it when specified or implied by --unshallow. After
fixing, both flags would work on shallow trees in conjunction with
--all, as demonstrated below:

        > ~/workspace/git/build/git -C local fetch --all --unshallow
        Fetching origin
        remote: Enumerating objects: 5, done.
        remote: Counting objects: 100% (5/5), done.
        remote: Compressing objects: 100% (3/3), done.
        remote: Total 4 (delta 1), reused 0 (delta 0), pack-reused 0 (from 0)
        Unpacking objects: 100% (4/4), 345 bytes | 345.00 KiB/s, done.
        Fetching origin2
        remote: Total 0 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
        > ~/workspace/git/build/git -C local log --format='%s'
        3
        2
        1

Signed-off-by: Rong Bao <rong.bao@csmantle.top>
---
 builtin/fetch.c           |  2 ++
 t/t5514-fetch-multiple.sh | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index b2decc6cfdb8ae86b8b1e92caa85f4961ffc47ff..a110a2dcf09c7e6764eb232bff73286753b27009 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -2247,6 +2247,8 @@ static void add_options_to_argv(struct strvec *argv,
 		strvec_push(argv, "--tags");
 	else if (tags == TAGS_UNSET)
 		strvec_push(argv, "--no-tags");
+	if (deepen)
+		strvec_pushf(argv, "--depth=%s", depth);
 	if (verbosity >= 2)
 		strvec_push(argv, "-v");
 	if (verbosity >= 1)
diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh
index 5d79472c39f67126079da5432938c6d97f08008a..b6568d33d7a6dd725a72584fbad942d6b3f7c39b 100755
--- a/t/t5514-fetch-multiple.sh
+++ b/t/t5514-fetch-multiple.sh
@@ -383,4 +383,38 @@ test_expect_success 'git fetch --no-all --all (fetch all remotes)' '
 	)
 '
 
+test_expect_success 'git fetch --all --depth=2 (fetch all remotes with depth 2)' '
+	setup_repository test16 &&
+	git clone --no-local --no-single-branch --depth=1 test16 test17 &&
+	(
+		cd test17 &&
+		git remote add remote2 ../test16 &&
+		test_path_is_file .git/shallow &&
+		git fetch --all --depth=2 &&
+		test_path_is_missing .git/shallow &&
+		git log --format=%s origin/side >actual1 &&
+		git log --format=%s remote2/side >actual2 &&
+		test_write_lines Second Initial >expect &&
+		test_cmp expect actual1 &&
+		test_cmp expect actual2
+	)
+'
+
+test_expect_success 'git fetch --all --unshallow (fetch all remotes with full history)' '
+	setup_repository test18 &&
+	git clone --no-local --no-single-branch --depth=1 test18 test19 &&
+	(
+		cd test19 &&
+		git remote add remote2 ../test18 &&
+		test_path_is_file .git/shallow &&
+		git fetch --all --unshallow &&
+		test_path_is_missing .git/shallow &&
+		git log --format=%s origin/side >actual1 &&
+		git log --format=%s remote2/side >actual2 &&
+		test_write_lines Second Initial >expect &&
+		test_cmp expect actual1 &&
+		test_cmp expect actual2
+	)
+'
+
 test_done

base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd
-- 
2.56.0

Re: [PATCH] fetch: fix --depth= and --unshallow for multi-remote fetches

[Junio C Hamano]

Rong Bao <rong.bao@csmantle.top> writes:

> When running `git fetch --all --depth=<SOME-DEPTH>` or `git fetch --all
> --unshallow` with Git 2.56.0, the fetch isn't really deepened:
> ...
>
> This is because the parent process doesn't pass the --depth= flag to the
> fetcher subprocess.
> ...
>
> Fix this by passing it when specified or implied by --unshallow. After
> fixing, both flags would work on shallow trees in conjunction with
> --all, as demonstrated below:

The above observes that "--depth=" given to the outer "fetch --all"
is not propagated down to subfetches.

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index b2decc6cfdb8ae86b8b1e92caa85f4961ffc47ff..a110a2dcf09c7e6764eb232bff73286753b27009 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -2247,6 +2247,8 @@ static void add_options_to_argv(struct strvec *argv,

This function is used to prepare a command line to run a single
fetch in a subprocess for 'fetch --all', and also to fetch
submodules when '--recurse-submodules' is in effect.  Does the
latter case share the same problem that '--depth=<N>' is not
propagated down?  Should the depth of submodules be controlled
independently by not passing '--depth=<N>' down in the first place,
in which case, this patch may be a regression for users who depend
on the behavior?

>  		strvec_push(argv, "--tags");
>  	else if (tags == TAGS_UNSET)
>  		strvec_push(argv, "--no-tags");
> +	if (deepen)
> +		strvec_pushf(argv, "--depth=%s", depth);

The 'deepen' variable checked here is set to true when any of
the '--depth=<N>', '--deepen-since=<TIME>', or
'--shallow-exclude=<REF>' were used.  If the outer
'fetch --all' was told to deepen with '--deepen-since=<TIME>',
without using '--depth=<N>', what do we have in the variable
'depth' at this point?  NULL?

In other words, shouldn't this be written in a more obvious,
simple, and straightforward way as three separate additions,
e.g.,

	if (depth)
		strvec_pushf(argv, "--depth=%s", depth);
	if (deepen_since)
		strvec_pushf(argv, "--shallow-since=%s", deepen_since);
	if (deepen_not.nr) {
		struct string_list_item *it;
		for_each_string_list_item(it, &deepen_not) {
                	strvec_pushf(argv, "--shallow-since=%s", it->string);
		}
	}

Propagating 'shallow-exclude' would probably not be a good
idea if this helper function is called within the context of
recursing into submodules, as their history and refs have no
connection to the history of the top-level project.  Please do
not blindly copy and paste the above to an updated patch.