* Re: [PATCH 3/3] scalar: only warn when background maintenance fails
From: Victoria Dye @ 2023-01-27 22:06 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git; +Cc: gitster, Derrick Stolee
In-Reply-To: <d75780e0567b5f765816ab7522afe550ebaa3521.1674849963.git.gitgitgadget@gmail.com>
Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@github.com>
>
> A user reported issues with 'scalar clone' and 'scalar register' when
> working in an environment that had locked down the ability to run
> 'crontab' or 'systemctl' in that those commands registered as _failures_
> instead of opportunistically reporting a success with just a warning
> about background maintenance.
>
> As a workaround, they can use GIT_TEST_MAINT_SCHEDULER to fake a
> successful background maintenance, but this is not a viable strategy for
> long-term.
>
> Update 'scalar register' and 'scalar clone' to no longer fail by
> modifying register_dir() to only warn when toggle_maintenance(1) fails.
>
> Since background maintenance is a "nice to have" and not a requirement
> for a working repository, it is best to move this from hard error to
> gentle warning.
>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
> scalar.c | 2 +-
> t/t9210-scalar.sh | 4 ++--
> t/t9211-scalar-clone.sh | 4 ++--
> 3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/scalar.c b/scalar.c
> index f25d5f1d0ef..ca19b95ce46 100644
> --- a/scalar.c
> +++ b/scalar.c
> @@ -262,7 +262,7 @@ static int register_dir(void)
> return error(_("could not set recommended config"));
>
> if (toggle_maintenance(1))
> - return error(_("could not turn on maintenance"));
> + warning(_("could not turn on maintenance"));
Should we do the same thing for 'unregister_dir()'? Unlike 'register_dir()',
it doesn't break immediately (and finishes removing the enlistment), but it
still returns a nonzero error code from 'scalar unregister'.
>
> if (have_fsmonitor_support() && start_fsmonitor_daemon()) {
> return error(_("could not start the FSMonitor daemon"));
> diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh
> index 13a4f6dbcf4..4432a30d10b 100755
> --- a/t/t9210-scalar.sh
> +++ b/t/t9210-scalar.sh
> @@ -104,10 +104,10 @@ test_expect_success FSMONITOR_DAEMON 'scalar register starts fsmon daemon' '
> test_cmp_config -C test/src true core.fsmonitor
> '
>
> -test_expect_success 'scalar register fails when background maintenance fails' '
> +test_expect_success 'scalar register warns when background maintenance fails' '
> git init register-repo &&
> GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \
> - test_must_fail scalar register register-repo 2>err &&
> + scalar register register-repo 2>err &&
> grep "could not turn on maintenance" err
> '
>
> diff --git a/t/t9211-scalar-clone.sh b/t/t9211-scalar-clone.sh
> index a6156da29ac..872ad1c9c2b 100755
> --- a/t/t9211-scalar-clone.sh
> +++ b/t/t9211-scalar-clone.sh
> @@ -174,9 +174,9 @@ test_expect_success 'progress without tty' '
> cleanup_clone $enlistment
> '
>
> -test_expect_success 'scalar clone fails when background maintenance fails' '
> +test_expect_success 'scalar clone warns when background maintenance fails' '
> GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \
> - test_must_fail scalar clone "file://$(pwd)/to-clone" maint-fail 2>err &&
> + scalar clone "file://$(pwd)/to-clone" maint-fail 2>err &&
> grep "could not turn on maintenance" err
> '
Similarly, it might be nice to show how 'scalar unregister' behaves when
maintenance fails in the tests.
^ permalink raw reply
* Re: [PATCH 3/3] scalar: only warn when background maintenance fails
From: Junio C Hamano @ 2023-01-27 20:36 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget; +Cc: git, vdye, Derrick Stolee
In-Reply-To: <d75780e0567b5f765816ab7522afe550ebaa3521.1674849963.git.gitgitgadget@gmail.com>
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Derrick Stolee <derrickstolee@github.com>
>
> A user reported issues with 'scalar clone' and 'scalar register' when
> working in an environment that had locked down the ability to run
> 'crontab' or 'systemctl' in that those commands registered as _failures_
> instead of opportunistically reporting a success with just a warning
> about background maintenance.
>
> As a workaround, they can use GIT_TEST_MAINT_SCHEDULER to fake a
> successful background maintenance, but this is not a viable strategy for
> long-term.
>
> Update 'scalar register' and 'scalar clone' to no longer fail by
> modifying register_dir() to only warn when toggle_maintenance(1) fails.
>
> Since background maintenance is a "nice to have" and not a requirement
> for a working repository, it is best to move this from hard error to
> gentle warning.
Wasn't the main selling point of scalar that users do not have to
worry about various minute details of configuration settings to
maintain their clone of projects on the larger side? The "maintain
their clone" certainly should include running periodic maintenance
tasks without them having to worry about it. It feels like this is
calling for an explicit "disable periodic maintenance tasks in this
repository" option to help these esoteric environments that disable
cron-like system services, while keeping the default safer,
i.e. fail loudly when the periodic maintenance tasks that the users
expect to happen cannot be enabled, or something.
Perhaps I am not the primary audience, but hmph, I have a feeling
that this is not exactly going into a healthy direction.
Other two steps that led to this step looked quite sensible, though.
Thanks.
^ permalink raw reply
* [PATCH 1/3] t: allow 'scalar' in test_must_fail
From: Derrick Stolee via GitGitGadget @ 2023-01-27 20:06 UTC (permalink / raw)
To: git; +Cc: vdye, gitster, Derrick Stolee, Derrick Stolee
In-Reply-To: <pull.1473.git.1674849963.gitgitgadget@gmail.com>
From: Derrick Stolee <derrickstolee@github.com>
This will enable scalar tests to use the test_must_fail helper, when
necessary.
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
t/test-lib-functions.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 79922227030..75b8ee95e7f 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1016,7 +1016,7 @@ test_must_fail_acceptable () {
fi
case "$1" in
- git|__git*|test-tool|test_terminal)
+ git|__git*|scalar|test-tool|test_terminal)
return 0
;;
*)
--
gitgitgadget
^ permalink raw reply related
* [PATCH 2/3] t921*: test scalar behavior starting maintenance
From: Derrick Stolee via GitGitGadget @ 2023-01-27 20:06 UTC (permalink / raw)
To: git; +Cc: vdye, gitster, Derrick Stolee, Derrick Stolee
In-Reply-To: <pull.1473.git.1674849963.gitgitgadget@gmail.com>
From: Derrick Stolee <derrickstolee@github.com>
A user recently reported issues with 'scalar register' and 'scalar
clone' in that they failed when the system had permissions locked down
so both 'crontab' and 'systemctl' commands failed when trying to enable
background maintenance.
This hard error is undesirable, but let's create tests that demonstrate
this behavior before modiying the behavior. We can use
GIT_TEST_MAINT_SCHEDULER to guarantee failure and check the exit code
and error message.
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
t/t9210-scalar.sh | 7 +++++++
t/t9211-scalar-clone.sh | 6 ++++++
2 files changed, 13 insertions(+)
diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh
index 25f500cf682..13a4f6dbcf4 100755
--- a/t/t9210-scalar.sh
+++ b/t/t9210-scalar.sh
@@ -104,6 +104,13 @@ test_expect_success FSMONITOR_DAEMON 'scalar register starts fsmon daemon' '
test_cmp_config -C test/src true core.fsmonitor
'
+test_expect_success 'scalar register fails when background maintenance fails' '
+ git init register-repo &&
+ GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \
+ test_must_fail scalar register register-repo 2>err &&
+ grep "could not turn on maintenance" err
+'
+
test_expect_success 'scalar unregister' '
git init vanish/src &&
scalar register vanish/src &&
diff --git a/t/t9211-scalar-clone.sh b/t/t9211-scalar-clone.sh
index 02d32fb6ede..a6156da29ac 100755
--- a/t/t9211-scalar-clone.sh
+++ b/t/t9211-scalar-clone.sh
@@ -174,4 +174,10 @@ test_expect_success 'progress without tty' '
cleanup_clone $enlistment
'
+test_expect_success 'scalar clone fails when background maintenance fails' '
+ GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \
+ test_must_fail scalar clone "file://$(pwd)/to-clone" maint-fail 2>err &&
+ grep "could not turn on maintenance" err
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH 3/3] scalar: only warn when background maintenance fails
From: Derrick Stolee via GitGitGadget @ 2023-01-27 20:06 UTC (permalink / raw)
To: git; +Cc: vdye, gitster, Derrick Stolee, Derrick Stolee
In-Reply-To: <pull.1473.git.1674849963.gitgitgadget@gmail.com>
From: Derrick Stolee <derrickstolee@github.com>
A user reported issues with 'scalar clone' and 'scalar register' when
working in an environment that had locked down the ability to run
'crontab' or 'systemctl' in that those commands registered as _failures_
instead of opportunistically reporting a success with just a warning
about background maintenance.
As a workaround, they can use GIT_TEST_MAINT_SCHEDULER to fake a
successful background maintenance, but this is not a viable strategy for
long-term.
Update 'scalar register' and 'scalar clone' to no longer fail by
modifying register_dir() to only warn when toggle_maintenance(1) fails.
Since background maintenance is a "nice to have" and not a requirement
for a working repository, it is best to move this from hard error to
gentle warning.
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
scalar.c | 2 +-
t/t9210-scalar.sh | 4 ++--
t/t9211-scalar-clone.sh | 4 ++--
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/scalar.c b/scalar.c
index f25d5f1d0ef..ca19b95ce46 100644
--- a/scalar.c
+++ b/scalar.c
@@ -262,7 +262,7 @@ static int register_dir(void)
return error(_("could not set recommended config"));
if (toggle_maintenance(1))
- return error(_("could not turn on maintenance"));
+ warning(_("could not turn on maintenance"));
if (have_fsmonitor_support() && start_fsmonitor_daemon()) {
return error(_("could not start the FSMonitor daemon"));
diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh
index 13a4f6dbcf4..4432a30d10b 100755
--- a/t/t9210-scalar.sh
+++ b/t/t9210-scalar.sh
@@ -104,10 +104,10 @@ test_expect_success FSMONITOR_DAEMON 'scalar register starts fsmon daemon' '
test_cmp_config -C test/src true core.fsmonitor
'
-test_expect_success 'scalar register fails when background maintenance fails' '
+test_expect_success 'scalar register warns when background maintenance fails' '
git init register-repo &&
GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \
- test_must_fail scalar register register-repo 2>err &&
+ scalar register register-repo 2>err &&
grep "could not turn on maintenance" err
'
diff --git a/t/t9211-scalar-clone.sh b/t/t9211-scalar-clone.sh
index a6156da29ac..872ad1c9c2b 100755
--- a/t/t9211-scalar-clone.sh
+++ b/t/t9211-scalar-clone.sh
@@ -174,9 +174,9 @@ test_expect_success 'progress without tty' '
cleanup_clone $enlistment
'
-test_expect_success 'scalar clone fails when background maintenance fails' '
+test_expect_success 'scalar clone warns when background maintenance fails' '
GIT_TEST_MAINT_SCHEDULER="crontab:false,launchctl:false,schtasks:false" \
- test_must_fail scalar clone "file://$(pwd)/to-clone" maint-fail 2>err &&
+ scalar clone "file://$(pwd)/to-clone" maint-fail 2>err &&
grep "could not turn on maintenance" err
'
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/3] Allow scalar to succeed despite maintenance failures
From: Derrick Stolee via GitGitGadget @ 2023-01-27 20:06 UTC (permalink / raw)
To: git; +Cc: vdye, gitster, Derrick Stolee
At $DAYJOB we received a report of issues with scalar clone and scalar
register in a protected environment. Specifically, they couldn't run crontab
or systemctl, which causes git maintenance start to fail. That failure was
percolating through scalar clone and scalar register, even though the rest
of their repository was properly set up.
Convert these hard failures into soft warnings.
This change could probably be done in a single commit, but I wanted to
demonstrate the existing behavior before changing it. The first patch was
also a related necessary step that will be helpful for future scalar tests.
Thanks,
* Stolee
Derrick Stolee (3):
t: allow 'scalar' in test_must_fail
t921*: test scalar behavior starting maintenance
scalar: only warn when background maintenance fails
scalar.c | 2 +-
t/t9210-scalar.sh | 7 +++++++
t/t9211-scalar-clone.sh | 6 ++++++
t/test-lib-functions.sh | 2 +-
4 files changed, 15 insertions(+), 2 deletions(-)
base-commit: 5cc9858f1b470844dea5c5d3e936af183fdf2c68
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1473%2Fderrickstolee%2Fscalar-register-warn-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1473/derrickstolee/scalar-register-warn-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1473
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH v2 05/10] bundle-uri: download in creationToken order
From: Junio C Hamano @ 2023-01-27 19:32 UTC (permalink / raw)
To: Victoria Dye
Cc: Derrick Stolee via GitGitGadget, git, me, avarab, steadmon,
chooglen, Derrick Stolee
In-Reply-To: <a2896d5b-f395-68df-1f23-356d0128cb9c@github.com>
Victoria Dye <vdye@github.com> writes:
>> + /*
>> + * Not downloaded yet. Try downloading.
>> + *
>> + * Note that bundle->file is non-NULL if a download
>> + * was attempted, even if it failed to download.
>> + */
>> + if (fetch_bundle_uri_internal(ctx.r, bundle, ctx.depth + 1, ctx.list)) {
>> + /* Mark as unbundled so we do not retry. */
>> + bundle->unbundled = 1;
>
> This implicitly shows that, unlike a failed unbundling, a failed download is
> always erroneous behavior, with the added benefit of avoiding (potentially
> expensive) download re-attempts.
Hmph, I somehow was hoping that we'd allow an option to use range
requests to resume an interrupted download in the future, so
outright "always avoid attempts to download again" may not be what
we want in the longer run. But being able to tell if download
failed (and there will probably be more than "success/failure" bit,
but something like "we got an explicit 401 not found" vs "we were
disconnected after downloading a few megabytes"), and unbundling
failed (where there is no point attempting) is a good idea.
>> cat >expect <<-EOF &&
>> - $HTTPD_URL/bundle-1.bundle
>> - $HTTPD_URL/bundle-2.bundle
>> - $HTTPD_URL/bundle-3.bundle
>> + $HTTPD_URL/bundle-list
>> $HTTPD_URL/bundle-4.bundle
>> + $HTTPD_URL/bundle-3.bundle
>> + $HTTPD_URL/bundle-2.bundle
>> + $HTTPD_URL/bundle-1.bundle
>> + EOF
>
> Ooh, interesting - using the new "test_remote_https_urls", these tests now
> also verify that the bundles were downloaded in decreasing order when using
> the 'creationToken' heuristic. That's a nice extra confirmation that the
> heuristic is working as intended.
Yes, that indeed is very nice.
^ permalink raw reply
* Re: [PATCH v2 00/10] Bundle URIs V: creationToken heuristic for incremental fetches
From: Victoria Dye @ 2023-01-27 19:28 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git
Cc: gitster, me, avarab, steadmon, chooglen, Derrick Stolee
In-Reply-To: <pull.1454.v2.git.1674487310.gitgitgadget@gmail.com>
Derrick Stolee via GitGitGadget wrote:
> Updates in v2
> =============
In patches 2-9, I only had a few minor nits I commented on in the respective
patches; everything else either addressed something directly noted in the
last set of reviews or otherwise was sufficiently explained by
comments/commit messages.
As for patch 1, I think the approach to relaxing the unbundling checks you
settled on in [1] after discussing with Junio makes sense.
[1] https://lore.kernel.org/git/ecc6b167-f5c4-48ce-3973-461d1659ed40@github.com/
>
> Thanks,
>
> * Stolee
>
> Derrick Stolee (10):
> bundle: optionally skip reachability walk
> t5558: add tests for creationToken heuristic
> bundle-uri: parse bundle.heuristic=creationToken
> bundle-uri: parse bundle.<id>.creationToken values
> bundle-uri: download in creationToken order
> clone: set fetch.bundleURI if appropriate
> bundle-uri: drop bundle.flag from design doc
> fetch: fetch from an external bundle URI
> bundle-uri: store fetch.bundleCreationToken
> bundle-uri: test missing bundles with heuristic
>
> Documentation/config/bundle.txt | 7 +
> Documentation/config/fetch.txt | 24 +
> Documentation/technical/bundle-uri.txt | 8 +-
> builtin/clone.c | 6 +-
> builtin/fetch.c | 7 +
> bundle-uri.c | 257 +++++++++-
> bundle-uri.h | 28 +-
> bundle.c | 3 +-
> bundle.h | 1 +
> t/t5558-clone-bundle-uri.sh | 672 ++++++++++++++++++++++++-
> t/t5601-clone.sh | 46 ++
> t/t5750-bundle-uri-parse.sh | 37 ++
> t/test-lib-functions.sh | 8 +
> 13 files changed, 1091 insertions(+), 13 deletions(-)
>
>
> base-commit: 4dbebc36b0893f5094668ddea077d0e235560b16
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1454%2Fderrickstolee%2Fbundle-redo%2FcreationToken-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1454/derrickstolee/bundle-redo/creationToken-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1454
>
> Range-diff vs v1:
>
> -: ----------- > 1: b3828725bc8 bundle: optionally skip reachability walk
> 1: 39eed914878 ! 2: 427aff4d5e5 t5558: add tests for creationToken heuristic
> @@ Commit message
> meantime, create tests that add creation tokens to the bundle list. For
> now, the Git client correctly ignores these unknown keys.
>
> + Create a new test helper function, test_remote_https_urls, which filters
> + GIT_TRACE2_EVENT output to extract a list of URLs passed to
> + git-remote-https child processes. This can be used to verify the order
> + of these requests as we implement the creationToken heuristic. For now,
> + we need to sort the actual output since the current client does not have
> + a well-defined order that it applies to the bundles.
> +
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
>
> ## t/t5558-clone-bundle-uri.sh ##
> @@ t/t5558-clone-bundle-uri.sh: test_expect_success 'clone HTTP bundle' '
> - test_config -C clone-http log.excludedecoration refs/bundle/
> '
>
> -+# usage: test_bundle_downloaded <bundle-name> <trace-file>
> -+test_bundle_downloaded () {
> -+ cat >pattern <<-EOF &&
> -+ "event":"child_start".*"argv":\["git-remote-https","$HTTPD_URL/$1"\]
> -+ EOF
> -+ grep -f pattern "$2"
> -+}
> -+
> test_expect_success 'clone bundle list (HTTP, no heuristic)' '
> + test_when_finished rm -f trace*.txt &&
> +
> @@ t/t5558-clone-bundle-uri.sh: test_expect_success 'clone bundle list (HTTP, no he
> - git -C clone-list-http cat-file --batch-check <oids
> + git -C clone-list-http cat-file --batch-check <oids &&
> +
> -+ for b in 1 2 3 4
> -+ do
> -+ test_bundle_downloaded bundle-$b.bundle trace-clone.txt ||
> -+ return 1
> -+ done
> ++ cat >expect <<-EOF &&
> ++ $HTTPD_URL/bundle-1.bundle
> ++ $HTTPD_URL/bundle-2.bundle
> ++ $HTTPD_URL/bundle-3.bundle
> ++ $HTTPD_URL/bundle-4.bundle
> ++ $HTTPD_URL/bundle-list
> ++ EOF
> ++
> ++ # Sort the list, since the order is not well-defined
> ++ # without a heuristic.
> ++ test_remote_https_urls <trace-clone.txt | sort >actual &&
> ++ test_cmp expect actual
> '
>
> test_expect_success 'clone bundle list (HTTP, any mode)' '
> @@ t/t5558-clone-bundle-uri.sh: test_expect_success 'clone bundle list (HTTP, any m
> '
>
> +test_expect_success 'clone bundle list (http, creationToken)' '
> ++ test_when_finished rm -f trace*.txt &&
> ++
> + cp clone-from/bundle-*.bundle "$HTTPD_DOCUMENT_ROOT_PATH/" &&
> + cat >"$HTTPD_DOCUMENT_ROOT_PATH/bundle-list" <<-EOF &&
> + [bundle]
> @@ t/t5558-clone-bundle-uri.sh: test_expect_success 'clone bundle list (HTTP, any m
> + creationToken = 4
> + EOF
> +
> -+ git clone --bundle-uri="$HTTPD_URL/bundle-list" . clone-list-http-2 &&
> ++ GIT_TRACE2_EVENT="$(pwd)/trace-clone.txt" git \
> ++ clone --bundle-uri="$HTTPD_URL/bundle-list" \
> ++ "$HTTPD_URL/smart/fetch.git" clone-list-http-2 &&
> +
> + git -C clone-from for-each-ref --format="%(objectname)" >oids &&
> -+ git -C clone-list-http-2 cat-file --batch-check <oids
> ++ git -C clone-list-http-2 cat-file --batch-check <oids &&
> ++
> ++ cat >expect <<-EOF &&
> ++ $HTTPD_URL/bundle-1.bundle
> ++ $HTTPD_URL/bundle-2.bundle
> ++ $HTTPD_URL/bundle-3.bundle
> ++ $HTTPD_URL/bundle-4.bundle
> ++ $HTTPD_URL/bundle-list
> ++ EOF
> ++
> ++ # Since the creationToken heuristic is not yet understood by the
> ++ # client, the order cannot be verified at this moment. Sort the
> ++ # list for consistent results.
> ++ test_remote_https_urls <trace-clone.txt | sort >actual &&
> ++ test_cmp expect actual
> +'
> +
> # Do not add tests here unless they use the HTTP server, as they will
> # not run unless the HTTP dependencies exist.
>
> +
> + ## t/test-lib-functions.sh ##
> +@@ t/test-lib-functions.sh: test_region () {
> + return 0
> + }
> +
> ++# Given a GIT_TRACE2_EVENT log over stdin, writes to stdout a list of URLs
> ++# sent to git-remote-https child processes.
> ++test_remote_https_urls() {
> ++ grep -e '"event":"child_start".*"argv":\["git-remote-https",".*"\]' |
> ++ sed -e 's/{"event":"child_start".*"argv":\["git-remote-https","//g' \
> ++ -e 's/"\]}//g'
> ++}
> ++
> + # Print the destination of symlink(s) provided as arguments. Basically
> + # the same as the readlink command, but it's not available everywhere.
> + test_readlink () {
> 2: 9007249b948 ! 3: f6f8197c9cc bundle-uri: parse bundle.heuristic=creationToken
> @@ Commit message
> bundle-uri' to print the heuristic value and verify that the parsing
> works correctly.
>
> + As an extra precaution, create the internal 'heuristics' array to be a
> + list of (enum, string) pairs so we can iterate through the array entries
> + carefully, regardless of the enum values.
> +
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
>
> ## Documentation/config/bundle.txt ##
> @@ bundle-uri.c
> #include "config.h"
> #include "remote.h"
>
> -+static const char *heuristics[] = {
> -+ [BUNDLE_HEURISTIC_NONE] = "",
> -+ [BUNDLE_HEURISTIC_CREATIONTOKEN] = "creationToken",
> ++static struct {
> ++ enum bundle_list_heuristic heuristic;
> ++ const char *name;
> ++} heuristics[BUNDLE_HEURISTIC__COUNT] = {
> ++ { BUNDLE_HEURISTIC_NONE, ""},
> ++ { BUNDLE_HEURISTIC_CREATIONTOKEN, "creationToken" },
> +};
> +
> static int compare_bundles(const void *hashmap_cmp_fn_data,
> @@ bundle-uri.c: void print_bundle_list(FILE *fp, struct bundle_list *list)
> fprintf(fp, "\tversion = %d\n", list->version);
> fprintf(fp, "\tmode = %s\n", mode);
>
> -+ if (list->heuristic)
> -+ printf("\theuristic = %s\n", heuristics[list->heuristic]);
> ++ if (list->heuristic) {
> ++ int i;
> ++ for (i = 0; i < BUNDLE_HEURISTIC__COUNT; i++) {
> ++ if (heuristics[i].heuristic == list->heuristic) {
> ++ printf("\theuristic = %s\n",
> ++ heuristics[list->heuristic].name);
> ++ break;
> ++ }
> ++ }
> ++ }
> +
> for_all_bundles_in_list(list, summarize_bundle, fp);
> }
> @@ bundle-uri.c: static int bundle_list_update(const char *key, const char *value,
> + if (!strcmp(subkey, "heuristic")) {
> + int i;
> + for (i = 0; i < BUNDLE_HEURISTIC__COUNT; i++) {
> -+ if (!strcmp(value, heuristics[i])) {
> -+ list->heuristic = i;
> ++ if (heuristics[i].heuristic &&
> ++ heuristics[i].name &&
> ++ !strcmp(value, heuristics[i].name)) {
> ++ list->heuristic = heuristics[i].heuristic;
> + return 0;
> + }
> + }
> @@ bundle-uri.h: enum bundle_list_mode {
> + BUNDLE_HEURISTIC_CREATIONTOKEN,
> +
> + /* Must be last. */
> -+ BUNDLE_HEURISTIC__COUNT,
> ++ BUNDLE_HEURISTIC__COUNT
> +};
> +
> /**
> 3: a1808f0b10c = 4: 12efa228d04 bundle-uri: parse bundle.<id>.creationToken values
> 4: 57c0174d375 ! 5: 7cfaa3c518c bundle-uri: download in creationToken order
> @@ Commit message
> strategy implemented here provides that short-circuit where the client
> downloads a minimal set of bundles.
>
> + However, we are not satisfied by the naive approach of downloading
> + bundles until one successfully unbundles, expecting the earlier bundles
> + to successfully unbundle now. The example repository in t5558
> + demonstrates this well:
> +
> + ---------------- bundle-4
> +
> + 4
> + / \
> + ----|---|------- bundle-3
> + | |
> + | 3
> + | |
> + ----|---|------- bundle-2
> + | |
> + 2 |
> + | |
> + ----|---|------- bundle-1
> + \ /
> + 1
> + |
> + (previous commits)
> +
> + In this repository, if we already have the objects for bundle-1 and then
> + try to fetch from this list, the naive approach will fail. bundle-4
> + requires both bundle-3 and bundle-2, though bundle-3 will successfully
> + unbundle without bundle-2. Thus, the algorithm needs to keep this in
> + mind.
> +
> A later implementation detail will store the maximum creationToken seen
> during such a bundle download, and the client will avoid downloading a
> bundle unless its creationToken is strictly greater than that stored
> @@ bundle-uri.c: static int download_bundle_to_file(struct remote_bundle_info *bund
> return 0;
> }
>
> -+struct sorted_bundle_list {
> ++struct bundles_for_sorting {
> + struct remote_bundle_info **items;
> + size_t alloc;
> + size_t nr;
> +};
> +
> -+static int insert_bundle(struct remote_bundle_info *bundle, void *data)
> ++static int append_bundle(struct remote_bundle_info *bundle, void *data)
> +{
> -+ struct sorted_bundle_list *list = data;
> ++ struct bundles_for_sorting *list = data;
> + list->items[list->nr++] = bundle;
> + return 0;
> +}
> +
> -+static int compare_creation_token(const void *va, const void *vb)
> ++/**
> ++ * For use in QSORT() to get a list sorted by creationToken
> ++ * in decreasing order.
> ++ */
> ++static int compare_creation_token_decreasing(const void *va, const void *vb)
> +{
> + const struct remote_bundle_info * const *a = va;
> + const struct remote_bundle_info * const *b = vb;
> @@ bundle-uri.c: static int download_bundle_to_file(struct remote_bundle_info *bund
> + struct bundle_list *list)
> +{
> + int cur;
> -+ int pop_or_push = 0;
> ++ int move_direction = 0;
> + struct bundle_list_context ctx = {
> + .r = r,
> + .list = list,
> + .mode = list->mode,
> + };
> -+ struct sorted_bundle_list sorted = {
> ++ struct bundles_for_sorting bundles = {
> + .alloc = hashmap_get_size(&list->bundles),
> + };
> +
> -+ ALLOC_ARRAY(sorted.items, sorted.alloc);
> ++ ALLOC_ARRAY(bundles.items, bundles.alloc);
> +
> -+ for_all_bundles_in_list(list, insert_bundle, &sorted);
> ++ for_all_bundles_in_list(list, append_bundle, &bundles);
> +
> -+ QSORT(sorted.items, sorted.nr, compare_creation_token);
> ++ QSORT(bundles.items, bundles.nr, compare_creation_token_decreasing);
> +
> + /*
> -+ * Use a stack-based approach to download the bundles and attempt
> -+ * to unbundle them in decreasing order by creation token. If we
> -+ * fail to unbundle (after a successful download) then move to the
> -+ * next non-downloaded bundle (push to the stack) and attempt
> -+ * downloading. Once we succeed in applying a bundle, move to the
> -+ * previous unapplied bundle (pop the stack) and attempt to unbundle
> -+ * it again.
> ++ * Attempt to download and unbundle the minimum number of bundles by
> ++ * creationToken in decreasing order. If we fail to unbundle (after
> ++ * a successful download) then move to the next non-downloaded bundle
> ++ * and attempt downloading. Once we succeed in applying a bundle,
> ++ * move to the previous unapplied bundle and attempt to unbundle it
> ++ * again.
> + *
> + * In the case of a fresh clone, we will likely download all of the
> + * bundles before successfully unbundling the oldest one, then the
> @@ bundle-uri.c: static int download_bundle_to_file(struct remote_bundle_info *bund
> + * repo's object store.
> + */
> + cur = 0;
> -+ while (cur >= 0 && cur < sorted.nr) {
> -+ struct remote_bundle_info *bundle = sorted.items[cur];
> ++ while (cur >= 0 && cur < bundles.nr) {
> ++ struct remote_bundle_info *bundle = bundles.items[cur];
> + if (!bundle->file) {
> -+ /* Not downloaded yet. Try downloading. */
> -+ if (download_bundle_to_file(bundle, &ctx)) {
> -+ /* Failure. Push to the stack. */
> -+ pop_or_push = 1;
> ++ /*
> ++ * Not downloaded yet. Try downloading.
> ++ *
> ++ * Note that bundle->file is non-NULL if a download
> ++ * was attempted, even if it failed to download.
> ++ */
> ++ if (fetch_bundle_uri_internal(ctx.r, bundle, ctx.depth + 1, ctx.list)) {
> ++ /* Mark as unbundled so we do not retry. */
> ++ bundle->unbundled = 1;
> ++
> ++ /* Try looking deeper in the list. */
> ++ move_direction = 1;
> + goto stack_operation;
> + }
> +
> @@ bundle-uri.c: static int download_bundle_to_file(struct remote_bundle_info *bund
> + * unbundled. Try unbundling again.
> + */
> + if (unbundle_from_file(ctx.r, bundle->file)) {
> -+ /* Failed to unbundle. Push to stack. */
> -+ pop_or_push = 1;
> ++ /* Try looking deeper in the list. */
> ++ move_direction = 1;
> + } else {
> -+ /* Succeeded in unbundle. Pop stack. */
> -+ pop_or_push = -1;
> ++ /*
> ++ * Succeeded in unbundle. Retry bundles
> ++ * that previously failed to unbundle.
> ++ */
> ++ move_direction = -1;
> ++ bundle->unbundled = 1;
> + }
> + }
> +
> @@ bundle-uri.c: static int download_bundle_to_file(struct remote_bundle_info *bund
> +
> +stack_operation:
> + /* Move in the specified direction and repeat. */
> -+ cur += pop_or_push;
> ++ cur += move_direction;
> + }
> +
> -+ free(sorted.items);
> ++ free(bundles.items);
> +
> + /*
> + * We succeed if the loop terminates because 'cur' drops below
> @@ bundle-uri.c: static int fetch_bundle_list_in_config_format(struct repository *r
> + * it advertises are expected to be bundles, not nested lists.
> + * We can drop 'global_list' and 'depth'.
> + */
> -+ if (list_from_bundle.heuristic == BUNDLE_HEURISTIC_CREATIONTOKEN)
> ++ if (list_from_bundle.heuristic == BUNDLE_HEURISTIC_CREATIONTOKEN) {
> + result = fetch_bundles_by_token(r, &list_from_bundle);
> -+ else if ((result = download_bundle_list(r, &list_from_bundle,
> ++ global_list->heuristic = BUNDLE_HEURISTIC_CREATIONTOKEN;
> ++ } else if ((result = download_bundle_list(r, &list_from_bundle,
> global_list, depth)))
> goto cleanup;
>
> @@ bundle-uri.c: int fetch_bundle_list(struct repository *r, struct bundle_list *li
> for_all_bundles_in_list(&global_list, unlink_bundle, NULL);
>
> ## t/t5558-clone-bundle-uri.sh ##
> -@@ t/t5558-clone-bundle-uri.sh: test_expect_success 'clone bundle list (HTTP, any mode)' '
> - '
> -
> - test_expect_success 'clone bundle list (http, creationToken)' '
> -+ test_when_finished rm -f trace*.txt &&
> -+
> - cp clone-from/bundle-*.bundle "$HTTPD_DOCUMENT_ROOT_PATH/" &&
> - cat >"$HTTPD_DOCUMENT_ROOT_PATH/bundle-list" <<-EOF &&
> - [bundle]
> @@ t/t5558-clone-bundle-uri.sh: test_expect_success 'clone bundle list (http, creationToken)' '
> - creationToken = 4
> - EOF
> + git -C clone-list-http-2 cat-file --batch-check <oids &&
>
> -- git clone --bundle-uri="$HTTPD_URL/bundle-list" . clone-list-http-2 &&
> -+ GIT_TRACE2_EVENT=$(pwd)/trace-clone.txt \
> -+ git clone --bundle-uri="$HTTPD_URL/bundle-list" \
> -+ clone-from clone-list-http-2 &&
> -
> - git -C clone-from for-each-ref --format="%(objectname)" >oids &&
> -- git -C clone-list-http-2 cat-file --batch-check <oids
> -+ git -C clone-list-http-2 cat-file --batch-check <oids &&
> -+
> -+ for b in 1 2 3 4
> -+ do
> -+ test_bundle_downloaded bundle-$b.bundle trace-clone.txt ||
> -+ return 1
> -+ done
> + cat >expect <<-EOF &&
> +- $HTTPD_URL/bundle-1.bundle
> +- $HTTPD_URL/bundle-2.bundle
> +- $HTTPD_URL/bundle-3.bundle
> ++ $HTTPD_URL/bundle-list
> + $HTTPD_URL/bundle-4.bundle
> ++ $HTTPD_URL/bundle-3.bundle
> ++ $HTTPD_URL/bundle-2.bundle
> ++ $HTTPD_URL/bundle-1.bundle
> ++ EOF
> ++
> ++ test_remote_https_urls <trace-clone.txt >actual &&
> ++ test_cmp expect actual
> +'
> +
> -+test_expect_success 'clone bundle list (http, creationToken)' '
> ++test_expect_success 'clone incomplete bundle list (http, creationToken)' '
> + test_when_finished rm -f trace*.txt &&
> +
> + cp clone-from/bundle-*.bundle "$HTTPD_DOCUMENT_ROOT_PATH/" &&
> @@ t/t5558-clone-bundle-uri.sh: test_expect_success 'clone bundle list (http, creat
> + [bundle "bundle-1"]
> + uri = bundle-1.bundle
> + creationToken = 1
> -+
> -+ [bundle "bundle-2"]
> -+ uri = bundle-2.bundle
> -+ creationToken = 2
> + EOF
> +
> + GIT_TRACE2_EVENT=$(pwd)/trace-clone.txt \
> + git clone --bundle-uri="$HTTPD_URL/bundle-list" \
> -+ clone-from clone-token-http &&
> ++ --single-branch --branch=base --no-tags \
> ++ "$HTTPD_URL/smart/fetch.git" clone-token-http &&
> +
> -+ test_bundle_downloaded bundle-1.bundle trace-clone.txt &&
> -+ test_bundle_downloaded bundle-2.bundle trace-clone.txt
> ++ cat >expect <<-EOF &&
> + $HTTPD_URL/bundle-list
> ++ $HTTPD_URL/bundle-1.bundle
> + EOF
> +
> +- # Since the creationToken heuristic is not yet understood by the
> +- # client, the order cannot be verified at this moment. Sort the
> +- # list for consistent results.
> +- test_remote_https_urls <trace-clone.txt | sort >actual &&
> ++ test_remote_https_urls <trace-clone.txt >actual &&
> + test_cmp expect actual
> '
>
> - # Do not add tests here unless they use the HTTP server, as they will
>
> ## t/t5601-clone.sh ##
> @@ t/t5601-clone.sh: test_expect_success 'auto-discover multiple bundles from HTTP clone' '
> grep -f pattern trace.txt
> '
>
> -+# Usage: test_bundle_downloaded <bundle-id> <trace-filename>
> -+test_bundle_downloaded () {
> -+ cat >pattern <<-EOF &&
> -+ "event":"child_start".*"argv":\["git-remote-https","$HTTPD_URL/$1.bundle"\]
> -+ EOF
> -+ grep -f pattern "$2"
> -+}
> -+
> +test_expect_success 'auto-discover multiple bundles from HTTP clone: creationToken heuristic' '
> + test_when_finished rm -rf "$HTTPD_DOCUMENT_ROOT_PATH/repo4.git" &&
> + test_when_finished rm -rf clone-heuristic trace*.txt &&
> @@ t/t5601-clone.sh: test_expect_success 'auto-discover multiple bundles from HTTP
> + -c transfer.bundleURI=true clone \
> + "$HTTPD_URL/smart/repo4.git" clone-heuristic &&
> +
> -+ # We should fetch all bundles
> -+ for b in everything new newest
> -+ do
> -+ test_bundle_downloaded $b trace-clone.txt || return 1
> -+ done
> ++ cat >expect <<-EOF &&
> ++ $HTTPD_URL/newest.bundle
> ++ $HTTPD_URL/new.bundle
> ++ $HTTPD_URL/everything.bundle
> ++ EOF
> ++
> ++ # We should fetch all bundles in the expected order.
> ++ test_remote_https_urls <trace-clone.txt >actual &&
> ++ test_cmp expect actual
> +'
> +
> # DO NOT add non-httpd-specific tests here, because the last part of this
> 5: d9c6f50e4f2 ! 6: 17c404c1b83 clone: set fetch.bundleURI if appropriate
> @@ Documentation/config/fetch.txt: fetch.writeCommitGraph::
> `git push -f`, and `git log --graph`. Defaults to false.
> +
> +fetch.bundleURI::
> -+ This value stores a URI for fetching Git object data from a bundle URI
> -+ before performing an incremental fetch from the origin Git server. If
> -+ the value is `<uri>` then running `git fetch <args>` is equivalent to
> -+ first running `git fetch --bundle-uri=<uri>` immediately before
> -+ `git fetch <args>`. See details of the `--bundle-uri` option in
> -+ linkgit:git-fetch[1].
> ++ This value stores a URI for downloading Git object data from a bundle
> ++ URI before performing an incremental fetch from the origin Git server.
> ++ This is similar to how the `--bundle-uri` option behaves in
> ++ linkgit:git-clone[1]. `git clone --bundle-uri` will set the
> ++ `fetch.bundleURI` value if the supplied bundle URI contains a bundle
> ++ list that is organized for incremental fetches.
>
> ## builtin/clone.c ##
> @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
> @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
> strvec_push(&transport_ls_refs_options.ref_prefixes, "HEAD");
>
> ## bundle-uri.c ##
> -@@ bundle-uri.c: static int fetch_bundle_list_in_config_format(struct repository *r,
> - * it advertises are expected to be bundles, not nested lists.
> - * We can drop 'global_list' and 'depth'.
> - */
> -- if (list_from_bundle.heuristic == BUNDLE_HEURISTIC_CREATIONTOKEN)
> -+ if (list_from_bundle.heuristic == BUNDLE_HEURISTIC_CREATIONTOKEN) {
> - result = fetch_bundles_by_token(r, &list_from_bundle);
> -- else if ((result = download_bundle_list(r, &list_from_bundle,
> -+ global_list->heuristic = BUNDLE_HEURISTIC_CREATIONTOKEN;
> -+ } else if ((result = download_bundle_list(r, &list_from_bundle,
> - global_list, depth)))
> - goto cleanup;
> -
> @@ bundle-uri.c: static int unlink_bundle(struct remote_bundle_info *info, void *data)
> return 0;
> }
> @@ bundle-uri.h: int bundle_uri_parse_config_format(const char *uri,
> * Given a bundle list that was already advertised (likely by the
>
> ## t/t5558-clone-bundle-uri.sh ##
> -@@ t/t5558-clone-bundle-uri.sh: test_expect_success 'clone bundle list (http, creationToken)' '
> - test_bundle_downloaded bundle-2.bundle trace-clone.txt
> +@@ t/t5558-clone-bundle-uri.sh: test_expect_success 'clone incomplete bundle list (http, creationToken)' '
> + --single-branch --branch=base --no-tags \
> + "$HTTPD_URL/smart/fetch.git" clone-token-http &&
> +
> ++ test_cmp_config -C clone-token-http "$HTTPD_URL/bundle-list" fetch.bundleuri &&
> ++
> + cat >expect <<-EOF &&
> + $HTTPD_URL/bundle-list
> + $HTTPD_URL/bundle-1.bundle
> +@@ t/t5558-clone-bundle-uri.sh: test_expect_success 'clone incomplete bundle list (http, creationToken)' '
> + test_cmp expect actual
> '
>
> +test_expect_success 'http clone with bundle.heuristic creates fetch.bundleURI' '
> @@ t/t5558-clone-bundle-uri.sh: test_expect_success 'clone bundle list (http, creat
> +
> + test_cmp_config -C fetch-http-4 "$HTTPD_URL/bundle-list" fetch.bundleuri &&
> +
> -+ # The clone should copy two files: the list and bundle-1.
> -+ test_bundle_downloaded bundle-list trace-clone.txt &&
> -+ test_bundle_downloaded bundle-1.bundle trace-clone.txt &&
> ++ cat >expect <<-EOF &&
> ++ $HTTPD_URL/bundle-list
> ++ $HTTPD_URL/bundle-1.bundle
> ++ EOF
> ++
> ++ test_remote_https_urls <trace-clone.txt >actual &&
> ++ test_cmp expect actual &&
> +
> + # only received base ref from bundle-1
> + git -C fetch-http-4 for-each-ref --format="%(refname)" "refs/bundles/*" >refs &&
> 6: afcfd27a883 = 7: d491070efed bundle-uri: drop bundle.flag from design doc
> 7: 1627fc158b1 ! 8: 59e57e04968 fetch: fetch from an external bundle URI
> @@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)
> if (dry_run)
> write_fetch_head = 0;
>
> -+ if (!git_config_get_string_tmp("fetch.bundleuri", &bundle_uri) &&
> -+ !starts_with(bundle_uri, "remote:")) {
> ++ if (!git_config_get_string_tmp("fetch.bundleuri", &bundle_uri)) {
> + if (fetch_bundle_uri(the_repository, bundle_uri, NULL))
> + warning(_("failed to fetch bundles from '%s'"), bundle_uri);
> + }
> @@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)
> die(_("fetch --all does not take a repository argument"));
>
> ## t/t5558-clone-bundle-uri.sh ##
> +@@ t/t5558-clone-bundle-uri.sh: test_expect_success 'clone incomplete bundle list (http, creationToken)' '
> + EOF
> +
> + test_remote_https_urls <trace-clone.txt >actual &&
> +- test_cmp expect actual
> ++ test_cmp expect actual &&
> ++
> ++ # We now have only one bundle ref.
> ++ git -C clone-token-http for-each-ref --format="%(refname)" "refs/bundles/*" >refs &&
> ++ cat >expect <<-\EOF &&
> ++ refs/bundles/base
> ++ EOF
> ++ test_cmp expect refs &&
> ++
> ++ # Add remaining bundles, exercising the "deepening" strategy
> ++ # for downloading via the creationToken heurisitc.
> ++ cat >>"$HTTPD_DOCUMENT_ROOT_PATH/bundle-list" <<-EOF &&
> ++ [bundle "bundle-2"]
> ++ uri = bundle-2.bundle
> ++ creationToken = 2
> ++
> ++ [bundle "bundle-3"]
> ++ uri = bundle-3.bundle
> ++ creationToken = 3
> ++
> ++ [bundle "bundle-4"]
> ++ uri = bundle-4.bundle
> ++ creationToken = 4
> ++ EOF
> ++
> ++ GIT_TRACE2_EVENT="$(pwd)/trace1.txt" \
> ++ git -C clone-token-http fetch origin --no-tags \
> ++ refs/heads/merge:refs/heads/merge &&
> ++
> ++ cat >expect <<-EOF &&
> ++ $HTTPD_URL/bundle-list
> ++ $HTTPD_URL/bundle-4.bundle
> ++ $HTTPD_URL/bundle-3.bundle
> ++ $HTTPD_URL/bundle-2.bundle
> ++ EOF
> ++
> ++ test_remote_https_urls <trace1.txt >actual &&
> ++ test_cmp expect actual &&
> ++
> ++ # We now have all bundle refs.
> ++ git -C clone-token-http for-each-ref --format="%(refname)" "refs/bundles/*" >refs &&
> ++
> ++ cat >expect <<-\EOF &&
> ++ refs/bundles/base
> ++ refs/bundles/left
> ++ refs/bundles/merge
> ++ refs/bundles/right
> ++ EOF
> ++ test_cmp expect refs
> + '
> +
> + test_expect_success 'http clone with bundle.heuristic creates fetch.bundleURI' '
> @@ t/t5558-clone-bundle-uri.sh: test_expect_success 'http clone with bundle.heuristic creates fetch.bundleURI' '
> cat >expect <<-\EOF &&
> refs/bundles/base
> @@ t/t5558-clone-bundle-uri.sh: test_expect_success 'http clone with bundle.heurist
> + refs/heads/left:refs/heads/left \
> + refs/heads/right:refs/heads/right &&
> +
> -+ # This fetch should copy two files: the list and bundle-2.
> -+ test_bundle_downloaded bundle-list trace1.txt &&
> -+ test_bundle_downloaded bundle-2.bundle trace1.txt &&
> -+ ! test_bundle_downloaded bundle-1.bundle trace1.txt &&
> ++ cat >expect <<-EOF &&
> ++ $HTTPD_URL/bundle-list
> ++ $HTTPD_URL/bundle-2.bundle
> ++ EOF
> ++
> ++ test_remote_https_urls <trace1.txt >actual &&
> ++ test_cmp expect actual &&
> +
> + # received left from bundle-2
> + git -C fetch-http-4 for-each-ref --format="%(refname)" "refs/bundles/*" >refs &&
> @@ t/t5558-clone-bundle-uri.sh: test_expect_success 'http clone with bundle.heurist
> + creationToken = 4
> + EOF
> +
> -+ # This fetch should skip bundle-3.bundle, since its objets are
> ++ # This fetch should skip bundle-3.bundle, since its objects are
> + # already local (we have the requisite commits for bundle-4.bundle).
> + GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
> + git -C fetch-http-4 fetch origin --no-tags \
> + refs/heads/merge:refs/heads/merge &&
> +
> -+ # This fetch should copy three files: the list, bundle-3, and bundle-4.
> -+ test_bundle_downloaded bundle-list trace2.txt &&
> -+ test_bundle_downloaded bundle-4.bundle trace2.txt &&
> -+ ! test_bundle_downloaded bundle-1.bundle trace2.txt &&
> -+ ! test_bundle_downloaded bundle-2.bundle trace2.txt &&
> -+ ! test_bundle_downloaded bundle-3.bundle trace2.txt &&
> ++ cat >expect <<-EOF &&
> ++ $HTTPD_URL/bundle-list
> ++ $HTTPD_URL/bundle-4.bundle
> ++ EOF
> ++
> ++ test_remote_https_urls <trace2.txt >actual &&
> ++ test_cmp expect actual &&
> +
> + # received merge ref from bundle-4, but right is missing
> + # because we did not download bundle-3.
> 8: 51f210ddeb4 ! 9: 6a1504b1c3a bundle-uri: store fetch.bundleCreationToken
> @@ Commit message
> When checking the same bundle list twice, this strategy requires
> downloading the bundle with the maximum creationToken again, which is
> wasteful. The creationToken heuristic promises that the client will not
> - have a use for that bundle if its creationToken value is the at most the
> + have a use for that bundle if its creationToken value is at most the
> previous creationToken value.
>
> To prevent these wasteful downloads, create a fetch.bundleCreationToken
> @@ Commit message
>
> ## Documentation/config/fetch.txt ##
> @@ Documentation/config/fetch.txt: fetch.bundleURI::
> - first running `git fetch --bundle-uri=<uri>` immediately before
> - `git fetch <args>`. See details of the `--bundle-uri` option in
> - linkgit:git-fetch[1].
> + linkgit:git-clone[1]. `git clone --bundle-uri` will set the
> + `fetch.bundleURI` value if the supplied bundle URI contains a bundle
> + list that is organized for incremental fetches.
> +++
> ++If you modify this value and your repository has a `fetch.bundleCreationToken`
> ++value, then remove that `fetch.bundleCreationToken` value before fetching from
> ++the new bundle URI.
> +
> +fetch.bundleCreationToken::
> + When using `fetch.bundleURI` to fetch incrementally from a bundle
> @@ Documentation/config/fetch.txt: fetch.bundleURI::
> + This value is used to prevent downloading bundles in the future
> + if the advertised `creationToken` is not strictly larger than this
> + value.
> +++
> ++The creation token values are chosen by the provider serving the specific
> ++bundle URI. If you modify the URI at `fetch.bundleURI`, then be sure to
> ++remove the value for the `fetch.bundleCreationToken` value before fetching.
>
> ## bundle-uri.c ##
> @@ bundle-uri.c: static int fetch_bundles_by_token(struct repository *r,
> {
> int cur;
> - int pop_or_push = 0;
> + int move_direction = 0;
> + const char *creationTokenStr;
> -+ uint64_t maxCreationToken;
> ++ uint64_t maxCreationToken = 0, newMaxCreationToken = 0;
> struct bundle_list_context ctx = {
> .r = r,
> .list = list,
> @@ bundle-uri.c: static int fetch_bundles_by_token(struct repository *r,
>
> - for_all_bundles_in_list(list, insert_bundle, &sorted);
> + for_all_bundles_in_list(list, append_bundle, &bundles);
>
> -+ if (!sorted.nr) {
> -+ free(sorted.items);
> ++ if (!bundles.nr) {
> ++ free(bundles.items);
> + return 0;
> + }
> +
> - QSORT(sorted.items, sorted.nr, compare_creation_token);
> + QSORT(bundles.items, bundles.nr, compare_creation_token_decreasing);
>
> + /*
> + * If fetch.bundleCreationToken exists, parses to a uint64t, and
> @@ bundle-uri.c: static int fetch_bundles_by_token(struct repository *r,
> + "fetch.bundlecreationtoken",
> + &creationTokenStr) &&
> + sscanf(creationTokenStr, "%"PRIu64, &maxCreationToken) == 1 &&
> -+ sorted.items[0]->creationToken <= maxCreationToken) {
> -+ free(sorted.items);
> ++ bundles.items[0]->creationToken <= maxCreationToken) {
> ++ free(bundles.items);
> + return 0;
> + }
> +
> /*
> - * Use a stack-based approach to download the bundles and attempt
> - * to unbundle them in decreasing order by creation token. If we
> + * Attempt to download and unbundle the minimum number of bundles by
> + * creationToken in decreasing order. If we fail to unbundle (after
> +@@ bundle-uri.c: static int fetch_bundles_by_token(struct repository *r,
> + cur = 0;
> + while (cur >= 0 && cur < bundles.nr) {
> + struct remote_bundle_info *bundle = bundles.items[cur];
> ++
> ++ /*
> ++ * If we need to dig into bundles below the previous
> ++ * creation token value, then likely we are in an erroneous
> ++ * state due to missing or invalid bundles. Halt the process
> ++ * instead of continuing to download extra data.
> ++ */
> ++ if (bundle->creationToken <= maxCreationToken)
> ++ break;
> ++
> + if (!bundle->file) {
> + /*
> + * Not downloaded yet. Try downloading.
> +@@ bundle-uri.c: static int fetch_bundles_by_token(struct repository *r,
> + */
> + move_direction = -1;
> + bundle->unbundled = 1;
> ++
> ++ if (bundle->creationToken > newMaxCreationToken)
> ++ newMaxCreationToken = bundle->creationToken;
> + }
> + }
> +
> @@ bundle-uri.c: stack_operation:
> - cur += pop_or_push;
> + cur += move_direction;
> }
>
> -- free(sorted.items);
> +- free(bundles.items);
> -
> /*
> * We succeed if the loop terminates because 'cur' drops below
> @@ bundle-uri.c: stack_operation:
> */
> + if (cur < 0) {
> + struct strbuf value = STRBUF_INIT;
> -+ strbuf_addf(&value, "%"PRIu64"", sorted.items[0]->creationToken);
> ++ strbuf_addf(&value, "%"PRIu64"", newMaxCreationToken);
> + if (repo_config_set_multivar_gently(ctx.r,
> + "fetch.bundleCreationToken",
> + value.buf, NULL, 0))
> @@ bundle-uri.c: stack_operation:
> + strbuf_release(&value);
> + }
> +
> -+ free(sorted.items);
> ++ free(bundles.items);
> return cur >= 0;
> }
>
>
> ## t/t5558-clone-bundle-uri.sh ##
> +@@ t/t5558-clone-bundle-uri.sh: test_expect_success 'clone incomplete bundle list (http, creationToken)' '
> + "$HTTPD_URL/smart/fetch.git" clone-token-http &&
> +
> + test_cmp_config -C clone-token-http "$HTTPD_URL/bundle-list" fetch.bundleuri &&
> ++ test_cmp_config -C clone-token-http 1 fetch.bundlecreationtoken &&
> +
> + cat >expect <<-EOF &&
> + $HTTPD_URL/bundle-list
> +@@ t/t5558-clone-bundle-uri.sh: test_expect_success 'clone incomplete bundle list (http, creationToken)' '
> + GIT_TRACE2_EVENT="$(pwd)/trace1.txt" \
> + git -C clone-token-http fetch origin --no-tags \
> + refs/heads/merge:refs/heads/merge &&
> ++ test_cmp_config -C clone-token-http 4 fetch.bundlecreationtoken &&
> +
> + cat >expect <<-EOF &&
> + $HTTPD_URL/bundle-list
> @@ t/t5558-clone-bundle-uri.sh: test_expect_success 'http clone with bundle.heuristic creates fetch.bundleURI' '
> "$HTTPD_URL/smart/fetch.git" fetch-http-4 &&
>
> test_cmp_config -C fetch-http-4 "$HTTPD_URL/bundle-list" fetch.bundleuri &&
> + test_cmp_config -C fetch-http-4 1 fetch.bundlecreationtoken &&
>
> - # The clone should copy two files: the list and bundle-1.
> - test_bundle_downloaded bundle-list trace-clone.txt &&
> + cat >expect <<-EOF &&
> + $HTTPD_URL/bundle-list
> @@ t/t5558-clone-bundle-uri.sh: test_expect_success 'http clone with bundle.heuristic creates fetch.bundleURI' '
> + git -C fetch-http-4 fetch origin --no-tags \
> refs/heads/left:refs/heads/left \
> refs/heads/right:refs/heads/right &&
> -
> + test_cmp_config -C fetch-http-4 2 fetch.bundlecreationtoken &&
> -+
> - # This fetch should copy two files: the list and bundle-2.
> - test_bundle_downloaded bundle-list trace1.txt &&
> - test_bundle_downloaded bundle-2.bundle trace1.txt &&
> +
> + cat >expect <<-EOF &&
> + $HTTPD_URL/bundle-list
> @@ t/t5558-clone-bundle-uri.sh: test_expect_success 'http clone with bundle.heuristic creates fetch.bundleURI' '
> EOF
> test_cmp expect refs &&
> @@ t/t5558-clone-bundle-uri.sh: test_expect_success 'http clone with bundle.heurist
> + git -C fetch-http-4 fetch origin --no-tags \
> + refs/heads/left:refs/heads/left \
> + refs/heads/right:refs/heads/right &&
> -+ test_bundle_downloaded bundle-list trace1b.txt &&
> -+ ! test_bundle_downloaded bundle-1.bundle trace1b.txt &&
> -+ ! test_bundle_downloaded bundle-2.bundle trace1b.txt &&
> ++
> ++ cat >expect <<-EOF &&
> ++ $HTTPD_URL/bundle-list
> ++ EOF
> ++ test_remote_https_urls <trace1b.txt >actual &&
> ++ test_cmp expect actual &&
> +
> cat >>"$HTTPD_DOCUMENT_ROOT_PATH/bundle-list" <<-EOF &&
> [bundle "bundle-3"]
> uri = bundle-3.bundle
> @@ t/t5558-clone-bundle-uri.sh: test_expect_success 'http clone with bundle.heuristic creates fetch.bundleURI' '
> + GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
> git -C fetch-http-4 fetch origin --no-tags \
> refs/heads/merge:refs/heads/merge &&
> -
> + test_cmp_config -C fetch-http-4 4 fetch.bundlecreationtoken &&
> -+
> - # This fetch should copy three files: the list, bundle-3, and bundle-4.
> - test_bundle_downloaded bundle-list trace2.txt &&
> - test_bundle_downloaded bundle-4.bundle trace2.txt &&
> +
> + cat >expect <<-EOF &&
> + $HTTPD_URL/bundle-list
> @@ t/t5558-clone-bundle-uri.sh: test_expect_success 'http clone with bundle.heuristic creates fetch.bundleURI' '
> refs/bundles/left
> refs/bundles/merge
> @@ t/t5558-clone-bundle-uri.sh: test_expect_success 'http clone with bundle.heurist
> + # No-op fetch
> + GIT_TRACE2_EVENT="$(pwd)/trace2b.txt" \
> + git -C fetch-http-4 fetch origin &&
> -+ test_bundle_downloaded bundle-list trace2b.txt &&
> -+ ! test_bundle_downloaded bundle-1.bundle trace2b.txt &&
> -+ ! test_bundle_downloaded bundle-2.bundle trace2b.txt &&
> -+ ! test_bundle_downloaded bundle-3.bundle trace2b.txt &&
> -+ ! test_bundle_downloaded bundle-4.bundle trace2b.txt
> ++
> ++ cat >expect <<-EOF &&
> ++ $HTTPD_URL/bundle-list
> ++ EOF
> ++ test_remote_https_urls <trace2b.txt >actual &&
> ++ test_cmp expect actual
> '
>
> # Do not add tests here unless they use the HTTP server, as they will
> -: ----------- > 10: 676522615ad bundle-uri: test missing bundles with heuristic
>
^ permalink raw reply
* Re: [PATCH v2 10/10] bundle-uri: test missing bundles with heuristic
From: Victoria Dye @ 2023-01-27 19:21 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git
Cc: gitster, me, avarab, steadmon, chooglen, Derrick Stolee
In-Reply-To: <676522615ad0e8f24099ef35a0f39367e5f688ae.1674487310.git.gitgitgadget@gmail.com>
Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@github.com>
>
> The creationToken heuristic uses a different mechanism for downloading
> bundles from the "standard" approach. Specifically: it uses a concrete
> order based on the creationToken values and attempts to download as few
> bundles as possible. It also modifies local config to store a value for
> future fetches to avoid downloading bundles, if possible.
>
> However, if any of the individual bundles has a failed download, then
> the logic for the ordering comes into question. It is important to avoid
> infinite loops, assigning invalid creation token values in config, but
> also to be opportunistic as possible when downloading as many bundles as
> seem appropriate.
>
> These tests were used to inform the implementation of
> fetch_bundles_by_token() in bundle-uri.c, but are being added
> independently here to allow focusing on faulty downloads. There may be
> more cases that could be added that result in modifications to
> fetch_bundles_by_token() as interesting data shapes reveal themselves in
> real scenarios.
>
The expanded testing is great, thanks for adding it!
> + # Case 2: middle bundle does not exist, only two bundles can unbundle
> + cat >"$HTTPD_DOCUMENT_ROOT_PATH/bundle-list" <<-EOF &&
> + [bundle]
> + version = 1
> + mode = all
> + heuristic = creationToken
> +
> + [bundle "bundle-1"]
> + uri = bundle-1.bundle
> + creationToken = 1
> +
> + [bundle "bundle-2"]
> + uri = fake.bundle
> + creationToken = 2
> +
> + [bundle "bundle-3"]
> + uri = bundle-3.bundle
> + creationToken = 3
> +
> + [bundle "bundle-4"]
> + uri = bundle-4.bundle
> + creationToken = 4
> + EOF
> +
> + GIT_TRACE2_EVENT="$(pwd)/trace-clone-2.txt" \
> + git clone --single-branch --branch=base \
> + --bundle-uri="$HTTPD_URL/bundle-list" \
> + "$HTTPD_URL/smart/fetch.git" download-2 &&
> +
> + # Bundle failure does not set these configs.
> + test_must_fail git -C download-2 config fetch.bundleuri &&
> + test_must_fail git -C download-2 config fetch.bundlecreationtoken &&
> +
> + cat >expect <<-EOF &&
> + $HTTPD_URL/bundle-list
> + $HTTPD_URL/bundle-4.bundle
> + $HTTPD_URL/bundle-3.bundle
> + $HTTPD_URL/fake.bundle
> + $HTTPD_URL/bundle-1.bundle
> + EOF
> + test_remote_https_urls <trace-clone-2.txt >actual &&
> + test_cmp expect actual &&
> +
> + # Only base bundle unbundled.
> + git -C download-2 for-each-ref --format="%(refname)" "refs/bundles/*" >refs &&
> + cat >expect <<-EOF &&
> + refs/bundles/base
> + refs/bundles/right
> + EOF
> + test_cmp expect refs &&
Maybe I'm misreading, but I don't think the comment ("Only base bundle
unbundled") lines up with the expected bundle refs (both bundle-1
('refs/bundles/base') and bundle-3 ('refs/bundles/right') seem to be
unbundled).
> +
> + # Case 3: top bundle does not exist, rest unbundle fine.
> + cat >"$HTTPD_DOCUMENT_ROOT_PATH/bundle-list" <<-EOF &&
> + [bundle]
> + version = 1
> + mode = all
> + heuristic = creationToken
> +
> + [bundle "bundle-1"]
> + uri = bundle-1.bundle
> + creationToken = 1
> +
> + [bundle "bundle-2"]
> + uri = bundle-2.bundle
> + creationToken = 2
> +
> + [bundle "bundle-3"]
> + uri = bundle-3.bundle
> + creationToken = 3
> +
> + [bundle "bundle-4"]
> + uri = fake.bundle
> + creationToken = 4
> + EOF
> +
> + GIT_TRACE2_EVENT="$(pwd)/trace-clone-3.txt" \
> + git clone --single-branch --branch=base \
> + --bundle-uri="$HTTPD_URL/bundle-list" \
> + "$HTTPD_URL/smart/fetch.git" download-3 &&
> +
> + # As long as we have continguous successful downloads,
> + # we _do_ set these configs.
> + test_cmp_config -C download-3 "$HTTPD_URL/bundle-list" fetch.bundleuri &&
> + test_cmp_config -C download-3 3 fetch.bundlecreationtoken &&
> +
> + cat >expect <<-EOF &&
> + $HTTPD_URL/bundle-list
> + $HTTPD_URL/fake.bundle
> + $HTTPD_URL/bundle-3.bundle
> + $HTTPD_URL/bundle-2.bundle
> + $HTTPD_URL/bundle-1.bundle
> + EOF
> + test_remote_https_urls <trace-clone-3.txt >actual &&
> + test_cmp expect actual &&
> +
> + # All bundles failed to unbundle
> + git -C download-3 for-each-ref --format="%(refname)" "refs/bundles/*" >refs &&
> + cat >expect <<-EOF &&
> + refs/bundles/base
> + refs/bundles/left
> + refs/bundles/right
> + EOF
> + test_cmp expect refs
Similar issue with the comment here - it says that all bundles *failed* to
unbundle, but the test case description ("Case 3: top bundle does not exist,
rest unbundle fine.") and the result show bundle-1, bundle-2, and bundle-3
all unbundling successfully.
> +'
> +
> +# Expand the bundle list to include other interesting shapes, specifically
> +# interesting for use when fetching from a previous state.
> +#
> +# ---------------- bundle-7
> +# 7
> +# _/|\_
> +# ---/--|--\------ bundle-6
> +# 5 | 6
> +# --|---|---|----- bundle-4
> +# | 4 |
> +# | / \ /
> +# --|-|---|/------ bundle-3 (the client will be caught up to this point.)
> +# \ | 3
> +# ---\|---|------- bundle-2
> +# 2 |
> +# ----|---|------- bundle-1
> +# \ /
> +# 1
> +# |
> +# (previous commits)
...
> + # Case 1: all bundles exist: successful unbundling of all bundles
...
> + # Case 2: middle bundle does not exist, only bundle-4 can unbundle
...
> + # Case 3: top bundle does not exist, rest unbundle fine.
The rest of these cases look okay and, at a high-level, it's helpful to have
these additional tests covering a different topology.
^ permalink raw reply
* Re: [PATCH v2 08/10] fetch: fetch from an external bundle URI
From: Victoria Dye @ 2023-01-27 19:18 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git
Cc: gitster, me, avarab, steadmon, chooglen, Derrick Stolee
In-Reply-To: <59e57e049683e42248c270b3bfcad2d72769219d.1674487310.git.gitgitgadget@gmail.com>
Derrick Stolee via GitGitGadget wrote:
> @@ -2109,6 +2110,7 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,
> int cmd_fetch(int argc, const char **argv, const char *prefix)
> {
> int i;
> + const char *bundle_uri;
> struct string_list list = STRING_LIST_INIT_DUP;
> struct remote *remote = NULL;
> int result = 0;
> @@ -2194,6 +2196,11 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> if (dry_run)
> write_fetch_head = 0;
>
> + if (!git_config_get_string_tmp("fetch.bundleuri", &bundle_uri)) {
> + if (fetch_bundle_uri(the_repository, bundle_uri, NULL))
> + warning(_("failed to fetch bundles from '%s'"), bundle_uri);
nit: these conditions don't need to be nested and could instead be joined
with '&&' in the outer 'if ()' (unless you plan to add more to this block in
a future series - I didn't see anything in later patches here).
Everything else (tests, doc updates since the last version) looks good.
^ permalink raw reply
* Re: [PATCH v2 05/10] bundle-uri: download in creationToken order
From: Victoria Dye @ 2023-01-27 19:17 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git
Cc: gitster, me, avarab, steadmon, chooglen, Derrick Stolee
In-Reply-To: <7cfaa3c518cbedb65c585cc02015bb21ae24e9fa.1674487310.git.gitgitgadget@gmail.com>
Derrick Stolee via GitGitGadget wrote:
> +struct bundles_for_sorting {
...
> +static int append_bundle(struct remote_bundle_info *bundle, void *data)
...
> +/**
> + * For use in QSORT() to get a list sorted by creationToken
> + * in decreasing order.
> + */
> +static int compare_creation_token_decreasing(const void *va, const void *vb)
These new function/struct names are all unambiguous. Looks good!
> + cur = 0;
> + while (cur >= 0 && cur < bundles.nr) {
> + struct remote_bundle_info *bundle = bundles.items[cur];
> + if (!bundle->file) {
> + /*
> + * Not downloaded yet. Try downloading.
> + *
> + * Note that bundle->file is non-NULL if a download
> + * was attempted, even if it failed to download.
> + */
> + if (fetch_bundle_uri_internal(ctx.r, bundle, ctx.depth + 1, ctx.list)) {
> + /* Mark as unbundled so we do not retry. */
> + bundle->unbundled = 1;
This implicitly shows that, unlike a failed unbundling, a failed download is
always erroneous behavior, with the added benefit of avoiding (potentially
expensive) download re-attempts.
> +
> + /* Try looking deeper in the list. */
> + move_direction = 1;
> + goto stack_operation;
> + }
> +
> + /* We expect bundles when using creationTokens. */
> + if (!is_bundle(bundle->file, 1)) {
> + warning(_("file downloaded from '%s' is not a bundle"),
> + bundle->uri);
> + break;
> + }
> + }
> +
> + if (bundle->file && !bundle->unbundled) {
> + /*
> + * This was downloaded, but not successfully
> + * unbundled. Try unbundling again.
> + */
> + if (unbundle_from_file(ctx.r, bundle->file)) {
> + /* Try looking deeper in the list. */
> + move_direction = 1;
> + } else {
> + /*
> + * Succeeded in unbundle. Retry bundles
> + * that previously failed to unbundle.
> + */
> + move_direction = -1;
> + bundle->unbundled = 1;
> + }
> + }
> +
> + /*
> + * Else case: downloaded and unbundled successfully.
> + * Skip this by moving in the same direction as the
> + * previous step.
> + */
> +
> +stack_operation:
Other than this label, it looks like you've replaced all of the
"stack-based" language. Should this be replaced as well? No problem if not,
I just wasn't sure whether it was left that way intentionally.
> diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh
> index 474432c8ace..6f9417a0afb 100755
> --- a/t/t5558-clone-bundle-uri.sh
> +++ b/t/t5558-clone-bundle-uri.sh
> @@ -401,17 +401,43 @@ test_expect_success 'clone bundle list (http, creationToken)' '
> git -C clone-list-http-2 cat-file --batch-check <oids &&
>
> cat >expect <<-EOF &&
> - $HTTPD_URL/bundle-1.bundle
> - $HTTPD_URL/bundle-2.bundle
> - $HTTPD_URL/bundle-3.bundle
> + $HTTPD_URL/bundle-list
> $HTTPD_URL/bundle-4.bundle
> + $HTTPD_URL/bundle-3.bundle
> + $HTTPD_URL/bundle-2.bundle
> + $HTTPD_URL/bundle-1.bundle
> + EOF
Ooh, interesting - using the new "test_remote_https_urls", these tests now
also verify that the bundles were downloaded in decreasing order when using
the 'creationToken' heuristic. That's a nice extra confirmation that the
heuristic is working as intended.
> +test_expect_success 'clone incomplete bundle list (http, creationToken)' '
...
> +test_expect_success 'auto-discover multiple bundles from HTTP clone: creationToken heuristic' '
These tests look good as well, especially 'clone incomplete bundle list's
now-more descriptive name.
^ permalink raw reply
* Re: [PATCH v2 02/10] t5558: add tests for creationToken heuristic
From: Victoria Dye @ 2023-01-27 19:15 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git
Cc: gitster, me, avarab, steadmon, chooglen, Derrick Stolee
In-Reply-To: <427aff4d5e5c85b601f43af8b664515380e11453.1674487310.git.gitgitgadget@gmail.com>
Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@github.com>
>
> As documented in the bundle URI design doc in 2da14fad8fe (docs:
> document bundle URI standard, 2022-08-09), the 'creationToken' member of
> a bundle URI allows a bundle provider to specify a total order on the
> bundles.
>
> Future changes will allow the Git client to understand these members and
> modify its behavior around downloading the bundles in that order. In the
> meantime, create tests that add creation tokens to the bundle list. For
> now, the Git client correctly ignores these unknown keys.
>
> Create a new test helper function, test_remote_https_urls, which filters
> GIT_TRACE2_EVENT output to extract a list of URLs passed to
> git-remote-https child processes. This can be used to verify the order
> of these requests as we implement the creationToken heuristic. For now,
> we need to sort the actual output since the current client does not have
> a well-defined order that it applies to the bundles.
...
> +# Given a GIT_TRACE2_EVENT log over stdin, writes to stdout a list of URLs
> +# sent to git-remote-https child processes.
> +test_remote_https_urls() {
> + grep -e '"event":"child_start".*"argv":\["git-remote-https",".*"\]' |
> + sed -e 's/{"event":"child_start".*"argv":\["git-remote-https","//g' \
> + -e 's/"\]}//g'
> +}
> +
...
> + cat >expect <<-EOF &&
> + $HTTPD_URL/bundle-1.bundle
> + $HTTPD_URL/bundle-2.bundle
> + $HTTPD_URL/bundle-3.bundle
> + $HTTPD_URL/bundle-4.bundle
> + $HTTPD_URL/bundle-list
> + EOF
> +
> + # Sort the list, since the order is not well-defined
> + # without a heuristic.
> + test_remote_https_urls <trace-clone.txt | sort >actual &&
> + test_cmp expect actual
...
> + cat >expect <<-EOF &&
> + $HTTPD_URL/bundle-1.bundle
> + $HTTPD_URL/bundle-2.bundle
> + $HTTPD_URL/bundle-3.bundle
> + $HTTPD_URL/bundle-4.bundle
> + $HTTPD_URL/bundle-list
> + EOF
> +
> + # Since the creationToken heuristic is not yet understood by the
> + # client, the order cannot be verified at this moment. Sort the
> + # list for consistent results.
> + test_remote_https_urls <trace-clone.txt | sort >actual &&
> + test_cmp expect actual
These updates make the tests stronger (that is, less likely to let a
regression slip through), and the additional comments are helpful for
explaining what is and is not implemented at this point in the series.
^ permalink raw reply
* Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails
From: Junio C Hamano @ 2023-01-27 18:46 UTC (permalink / raw)
To: Mathias Krause
Cc: git, Ævar Arnfjörð Bjarmason,
Carlo Marcelo Arenas Belón
In-Reply-To: <xmqq1qnfancf.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
>> What am I missing?
>
> Note that I've seen and recently re-read the discussion that leads to
> https://lore.kernel.org/git/f680b274-fa85-6624-096a-7753a2671c15@grsecurity.net/
>
> I suspect that this auto-probe is related to solving "the user
> thinks JIT is in use but because of failing JIT the user's pattern
> is getting horrible performance" somehow. But I do not think a hard
> failure is a good approach to help users in such a situation.
I guess what I am saying is that the previous one that has been
queued on 'seen' may be better. It should cover your original
"SELinux and other mechanisms can render JIT unusable because they
do not allow dynamic generation of code" use case.
Thanks.
^ permalink raw reply
* Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails
From: Junio C Hamano @ 2023-01-27 17:39 UTC (permalink / raw)
To: Mathias Krause
Cc: git, Ævar Arnfjörð Bjarmason,
Carlo Marcelo Arenas Belón
In-Reply-To: <xmqqbkmk9bsn.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Yes, the "instead of failing hard, fall back" makes sense. Just
> that I do not see why the runtime test is a good thing to have. In
> short, we are not in the business of catching bugs in pcre2_jit
> implementations, so if they say they cannot compile the pattern (I
> would even say I doubt the point of checking the return code to
> ensure it is NOMEMORY), it would be fine to let the interpreter
> codepath to inspect the pattern and diagnose problems with it, or
> take the slow match without JIT.
>
> What am I missing?
Note that I've seen and recently re-read the discussion that leads to
https://lore.kernel.org/git/f680b274-fa85-6624-096a-7753a2671c15@grsecurity.net/
I suspect that this auto-probe is related to solving "the user
thinks JIT is in use but because of failing JIT the user's pattern
is getting horrible performance" somehow. But I do not think a hard
failure is a good approach to help users in such a situation.
After such a failure, the user can prefix "(*NO_JIT)" to the pattern
and retry, or give up the operation altogether and not get a useful
result, but wouldn't it be far more helpful to just fallback as if
(*NO_JIT) was on from the beginning?
Also I notice that p->pcre2_jit_on is per "struct grep_pat", so it
is not like "once we see a pathological pattern, we turn off JIT
completely for other patterns", right? That is, if you have
git grep -P -e "$A" -e "$B"
and we fail to compile "$A" (for whatever reason), we could still
(attempt to) compile "$B". Perhaps $A was too complex or was
incompatible with JIT combined with other options, but $B may be
easy enough to still be JITtable, in which case we would match with
the JITted version of $B with interpreted version of $A, instead of
failing, right?
THanks.
^ permalink raw reply
* How experimental are git switch/restore?
From: Ilya Kantor @ 2023-01-27 17:10 UTC (permalink / raw)
To: git
Hello,
How experimental are the "recent" git switch and restore commands?
Are there any plans to change the current behavior or remove the "experimental" warning from the manual?
Kind regards,
Ilya Kantor
^ permalink raw reply
* Re: Git over HTTP; have flexible SASL authentication
From: Junio C Hamano @ 2023-01-27 17:06 UTC (permalink / raw)
To: Rick van Rein; +Cc: git, Jeff King, Matthew John Cheetham
In-Reply-To: <20230127163434.GA784@openfortress.nl>
Rick van Rein <rick@openfortress.nl> writes:
> Git providers are inventing proprietary extensions to HTTP authentication
> for Git. It seems smarter to use SASL for this purpose, possibly allowing
> the client a choice and authentication ringback to the client's own domain.
To adopt things like this, the work to extend how to make extensible
what is on WWW-Authenticate in the thread that contains this recent
message https://lore.kernel.org/git/Y9LvFMzriAWUsS58@coredump.intra.peff.net/
may be relevant, perhaps?
^ permalink raw reply
* Git over HTTP; have flexible SASL authentication
From: Rick van Rein @ 2023-01-27 16:34 UTC (permalink / raw)
To: git
Hello,
Git providers are inventing proprietary extensions to HTTP authentication
for Git. It seems smarter to use SASL for this purpose, possibly allowing
the client a choice and authentication ringback to the client's own domain.
I wrote an Internet Draft to allow just that, and we implemented it for
Apache, Nginx and FireFox. I would love to learn if this list considers
it a sensible extensions to Git.
https://datatracker.ietf.org/doc/html/draft-vanrein-httpauth-sasl
If you think this is useful, it would be wonderful to mention that on the
HTTP WorkGroup list as well, because they are now considering HTTP-SASL
for adoption, a formal requisite to send "Authorization: SASL" headers.
https://datatracker.ietf.org/wg/httpbis/about/
Thanks!
Rick van Rein
InternetWide.org
Apache :- https://gitlab.com/arpa2/apachemod/-/tree/master/arpa2_sasl
https://gitlab.com/arpa2/apachemod/-/tree/master/arpa2_diasasl
Nginx :- https://github.com/stef/ngx_http_auth_sasl_module
FireFox :- https://gitlab.com/arpa2/http_sasl_client
^ permalink raw reply
* Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails
From: Junio C Hamano @ 2023-01-27 16:34 UTC (permalink / raw)
To: Mathias Krause
Cc: git, Ævar Arnfjörð Bjarmason,
Carlo Marcelo Arenas Belón
In-Reply-To: <20230127154952.485913-1-minipli@grsecurity.net>
Mathias Krause <minipli@grsecurity.net> writes:
> As having a functional PCRE2 JIT compiler is a legitimate use case for
> performance reasons, we'll only do the fallback if the supposedly
> available JIT is found to be non-functional by attempting to JIT compile
> a very simple pattern. If this fails, JIT is deemed to be non-functional
> and we do the interpreter fallback. For all other cases, i.e. the simple
> pattern can be compiled but the user provided cannot, we fail hard as we
> do now as the reason for the failure must be the pattern itself.
I do not know if it is a good idea to rely on the "very simple
pattern". The implementation of JIT could devise various ways to
succeed for such simple patterns without having writable-executable
piece of memory. What happened to the earlier idea of falling back
to the interpreted codepath, which will catch any bad pattern that
has "the reason for the failure" by failing anyway?
> +static int pcre2_jit_functional(void)
> +{
> + static int jit_working = -1;
> + pcre2_code *code;
> + size_t off;
> + int err;
> +
> + if (jit_working != -1)
> + return jit_working;
> +
> + /*
> + * Try to JIT compile a simple pattern to probe if the JIT is
> + * working in general. It might fail for systems where creating
> + * memory mappings for runtime code generation is restricted.
> + */
> + code = pcre2_compile((PCRE2_SPTR)".", 1, 0, &err, &off, NULL);
> + if (!code)
> + return 0;
> +
> + jit_working = pcre2_jit_compile(code, PCRE2_JIT_COMPLETE) == 0;
> + pcre2_code_free(code);
I'd prefer not having to worry about: Or it might not fail for such
systems, as the pattern is too simple and future versions of
pcre2_compile() could have special case code.
> @@ -317,8 +342,23 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
> pcre2_config(PCRE2_CONFIG_JIT, &p->pcre2_jit_on);
> if (p->pcre2_jit_on) {
> jitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);
> - if (jitret)
> + if (jitret == PCRE2_ERROR_NOMEMORY && !pcre2_jit_functional()) {
> + /*
> + * Even though pcre2_config(PCRE2_CONFIG_JIT, ...)
> + * indicated JIT support, the library might still
> + * fail to generate JIT code for various reasons,
> + * e.g. when SELinux's 'deny_execmem' or PaX's
> + * MPROTECT prevent creating W|X memory mappings.
> + *
> + * Instead of faling hard, fall back to interpreter
> + * mode, just as if the pattern was prefixed with
> + * '(*NO_JIT)'.
> + */
> + p->pcre2_jit_on = 0;
> + return;
Yes, the "instead of failing hard, fall back" makes sense. Just
that I do not see why the runtime test is a good thing to have. In
short, we are not in the business of catching bugs in pcre2_jit
implementations, so if they say they cannot compile the pattern (I
would even say I doubt the point of checking the return code to
ensure it is NOMEMORY), it would be fine to let the interpreter
codepath to inspect the pattern and diagnose problems with it, or
take the slow match without JIT.
What am I missing?
^ permalink raw reply
* [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails
From: Mathias Krause @ 2023-01-27 15:49 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Mathias Krause,
Carlo Marcelo Arenas Belón
In-Reply-To: <20221216121557.30714-1-minipli@grsecurity.net>
Under Linux systems with SELinux's 'deny_execmem' or PaX's MPROTECT
enabled, the allocation of PCRE2's JIT rwx memory may be prohibited,
making pcre2_jit_compile() fail with PCRE2_ERROR_NOMEMORY (-48):
[user@fedora git]$ git grep -c PCRE2_JIT
grep.c:1
[user@fedora git]$ # Enable SELinux's W^X policy
[user@fedora git]$ sudo semanage boolean -m -1 deny_execmem
[user@fedora git]$ # JIT memory allocation fails, breaking 'git grep'
[user@fedora git]$ git grep -c PCRE2_JIT
fatal: Couldn't JIT the PCRE2 pattern 'PCRE2_JIT', got '-48'
Instead of failing hard in this case and making 'git grep' unusable on
such systems, simply fall back to interpreter mode, leading to a much
better user experience.
As having a functional PCRE2 JIT compiler is a legitimate use case for
performance reasons, we'll only do the fallback if the supposedly
available JIT is found to be non-functional by attempting to JIT compile
a very simple pattern. If this fails, JIT is deemed to be non-functional
and we do the interpreter fallback. For all other cases, i.e. the simple
pattern can be compiled but the user provided cannot, we fail hard as we
do now as the reason for the failure must be the pattern itself.
Cc: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
This patch is based on a previous attempt proposed by Carlo already 4
years ago[1]. However, it wasn't applied as there were still ongoing
discussions about how to handle and possibly avoid the automatic
fallback.
A follow-up RFC had been posted half a year later[2], adding a config
option and, after some more discussion, even command line switches[3].
But, after all, it was agreed on that this is far too much and Junio
suggested to simply revert back to the initial RFC and implement the
automatic fallback[4], basically merging it with a proper changelog[5].
As that never happened, I took up the work and tried to do just that.
This, again, lead to some discussion but, fortunately, less about the
config knobs, but more about the implications of such a change. I tried
to address Ævar's concerns about always falling back to the interpreter
and limited it to the problematic case I want to get solved.
1. https://lore.kernel.org/r/20181209230024.43444-3-carenas@gmail.com
2. https://lore.kernel.org/git/20190728235427.41425-1-carenas@gmail.com/
3. https://lore.kernel.org/git/20190729105955.44390-1-carenas@gmail.com/
4. https://lore.kernel.org/git/xmqqh874vikk.fsf@gitster-ct.c.googlers.com/
5. https://lore.kernel.org/git/xmqqef1zmkp5.fsf@gitster-ct.c.googlers.com/
Changes in v2:
The current version takes a conservative approach by only implementing
the fallback to interpreter mode when the runtime test for basic JIT
support fails as well, indicating the inability of PCRE2's memory
allocator to acquire a W|X mappings for runtime code generation.
pcre2_jit_functional() very much intentional calls pcre2_compile()
without making use of the context set up in compile_pcre2_pattern() to
exclude any errors related to that -- a wrong combination of options
making pcre2_jit_compile() fail for these reasons instead of the memory
mapping error we try to detect. This ensures we're doing a dumb simple
JIT compile test to probe its general runtime availability and not
wrongly fall back to interpreter mode because the option combination
we're trying to make use of isn't supported by the JIT.
I also changed the author to myself as the current state has little in
common with what Carlo once proposed.
---
grep.c | 42 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 41 insertions(+), 1 deletion(-)
diff --git a/grep.c b/grep.c
index 06eed694936c..59afc3f07fc9 100644
--- a/grep.c
+++ b/grep.c
@@ -262,6 +262,31 @@ static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)
free(pointer);
}
+static int pcre2_jit_functional(void)
+{
+ static int jit_working = -1;
+ pcre2_code *code;
+ size_t off;
+ int err;
+
+ if (jit_working != -1)
+ return jit_working;
+
+ /*
+ * Try to JIT compile a simple pattern to probe if the JIT is
+ * working in general. It might fail for systems where creating
+ * memory mappings for runtime code generation is restricted.
+ */
+ code = pcre2_compile((PCRE2_SPTR)".", 1, 0, &err, &off, NULL);
+ if (!code)
+ return 0;
+
+ jit_working = pcre2_jit_compile(code, PCRE2_JIT_COMPLETE) == 0;
+ pcre2_code_free(code);
+
+ return jit_working;
+}
+
static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)
{
int error;
@@ -317,8 +342,23 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
pcre2_config(PCRE2_CONFIG_JIT, &p->pcre2_jit_on);
if (p->pcre2_jit_on) {
jitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);
- if (jitret)
+ if (jitret == PCRE2_ERROR_NOMEMORY && !pcre2_jit_functional()) {
+ /*
+ * Even though pcre2_config(PCRE2_CONFIG_JIT, ...)
+ * indicated JIT support, the library might still
+ * fail to generate JIT code for various reasons,
+ * e.g. when SELinux's 'deny_execmem' or PaX's
+ * MPROTECT prevent creating W|X memory mappings.
+ *
+ * Instead of faling hard, fall back to interpreter
+ * mode, just as if the pattern was prefixed with
+ * '(*NO_JIT)'.
+ */
+ p->pcre2_jit_on = 0;
+ return;
+ } else if (jitret) {
die("Couldn't JIT the PCRE2 pattern '%s', got '%d'\n", p->pattern, jitret);
+ }
/*
* The pcre2_config(PCRE2_CONFIG_JIT, ...) call just
--
2.39.0
^ permalink raw reply related
* Re: [PATCH 4/5] sequencer: use the new hook API for the simpler "post-rewrite" call
From: Phillip Wood @ 2023-01-27 15:08 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Emily Shaffer, Junio C Hamano, Eric Sunshine, Felipe Contreras,
Taylor Blau, Michael Strawbridge
In-Reply-To: <a2810f20-c093-ba73-0fed-5d179e3e954b@dunelm.org.uk>
Hi Ævar
On 24/01/2023 14:46, Phillip Wood wrote:
> Hi Ævar
>
> On 23/01/2023 17:15, Ævar Arnfjörð Bjarmason wrote:
>> From: Emily Shaffer <emilyshaffer@google.com>
>>
>> Change the invocation of the "post-rewrite" hook added in
>> 795160457db (sequencer (rebase -i): run the post-rewrite hook, if
>> needed, 2017-01-02) to use the new hook API.
>>
>> This leaves the more complex "post-rewrite" invocation added in
>> a87a6f3c98e (commit: move post-rewrite code to libgit, 2017-11-17)
>> here in sequencer.c unconverted. That'll be done in a subsequent
>> commit.
>
> As a reader I'd find it more helpful to explain why the conversion isn't
> done here rather than leaving be to run "git show" to figure it out. If
> you re-roll perhaps we could replace the commit citation with something
> like
>
> sequencer.c also contains an invocation of the "post-rewrite" hook in
> run_rewrite_hook() that is not converted as the hook API does not allow
> us to pass the hook input as a string yet.
Sorry, I forgot to say in my previous reply that I like the code change
here - it is a nice simplification for callers. builtin/am.c has a
similar function to the one that is converted here.
Best Wishes
Phillip
> Best Wishes
>
> Phillip
>
>> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>> sequencer.c | 18 ++++--------------
>> 1 file changed, 4 insertions(+), 14 deletions(-)
>>
>> diff --git a/sequencer.c b/sequencer.c
>> index 3e4a1972897..d8d59d05dd4 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -4834,8 +4834,7 @@ static int pick_commits(struct repository *r,
>> if (!stat(rebase_path_rewritten_list(), &st) &&
>> st.st_size > 0) {
>> struct child_process child = CHILD_PROCESS_INIT;
>> - const char *post_rewrite_hook =
>> - find_hook("post-rewrite");
>> + struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT;
>> child.in = open(rebase_path_rewritten_list(), O_RDONLY);
>> child.git_cmd = 1;
>> @@ -4845,18 +4844,9 @@ static int pick_commits(struct repository *r,
>> /* we don't care if this copying failed */
>> run_command(&child);
>> - if (post_rewrite_hook) {
>> - struct child_process hook = CHILD_PROCESS_INIT;
>> -
>> - hook.in = open(rebase_path_rewritten_list(),
>> - O_RDONLY);
>> - hook.stdout_to_stderr = 1;
>> - hook.trace2_hook_name = "post-rewrite";
>> - strvec_push(&hook.args, post_rewrite_hook);
>> - strvec_push(&hook.args, "rebase");
>> - /* we don't care if this hook failed */
>> - run_command(&hook);
>> - }
>> + hook_opt.path_to_stdin = rebase_path_rewritten_list();
>> + strvec_push(&hook_opt.args, "rebase");
>> + run_hooks_opt("post-rewrite", &hook_opt);
>> }
>> apply_autostash(rebase_path_autostash());
^ permalink raw reply
* Re: [PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees
From: Phillip Wood @ 2023-01-27 14:46 UTC (permalink / raw)
To: Carlo Arenas
Cc: git, pclouds, gitster, Jinwook Jeong, Eric Sunshine,
Rubén Justo, Phillip Wood
In-Reply-To: <CAPUEspjuXSncRxo5DMj1pA5zcYvn4Y6epdijYL6HJRGhk_6q7g@mail.gmail.com>
Hi Carlo
On 20/01/2023 22:12, Carlo Arenas wrote:
> On Fri, Jan 20, 2023 at 7:08 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> On 20/01/2023 11:35, Carlo Marcelo Arenas Belón wrote:
>>> Commands `git switch -C` and `git checkout -B` neglect to check whether
>>> the provided branch is already checked out in some other worktree, as
>>> shown by the following:
>>>
>>> $ git worktree list
>>> .../foo beefb00f [main]
>>> $ git worktree add ../other
>>> Preparing worktree (new branch 'other')
>>> HEAD is now at beefb00f first
>>> $ cd ../other
>>> $ git switch -C main
>>> Switched to and reset branch 'main'
>>> $ git worktree list
>>> .../foo beefb00f [main]
>>> .../other beefb00f [main]
>>>
>>> Fix this problem by teaching `git switch -C` and `git checkout -B` to
>>> check whether the branch in question is already checked out elsewhere.
>>>
>>> Unlike what it is done for `git switch` and `git checkout`, that have
>>> an historical exception to ignore other worktrees if the branch to
>>> check is the current one (as required when called as part of other
>>> tools), the logic implemented is more strict and will require the user
>>> to invoke the command with `--ignore-other-worktrees` to explicitly
>>> indicate they want the risky behaviour.
>>>
>>> This matches the current behaviour of `git branch -f` and is safer; for
>>> more details see the tests in t2400.
>>
>> As I said before, it would be much easier for everyone else to
>> understand the changes if you wrote out what they were rather than
>> saying "look at the tests". I do appreciate the improved test
>> descriptions though - thanks for that.
>
> Apologies on that, I tried to come up with something that would
> describe the change of behaviour other than the paragraph above and
> couldn't come out with a better explanation except reading the tests
> (which I know is complicated by the fact they are interlinked).
>
> The behaviour I am changing is also not documented (other than by the
> test) and might have been added as a quirk to keep the scripted rebase
> and bisect going when confronted with branches that were checked out
> in multiple worktrees, and as such might had not be intended for
> `switch`, and might not be needed anymore either.
>
> Using`checkout` for simplicity, but also applies to `switch`,
>
> % git worktree list
> .../base 6a45aba [main]
> % git worktree add -f ../other main
> Preparing worktree (checking out 'main')
> HEAD is now at 6a45aba init
> % cd ../other
> % git checkout main
> Already on 'main'
> % git checkout -B main
> fatal: 'main' is already checked out at '.../base'
Thanks for explaining that. If there is no <start-point> given we don't
reset the branch so it seems a bit harsh to error out here. For "git
checkout -B <branch> <start-point>" when <branch> is checked out in
another worktree requiring --ignore-other-worktrees makes sense.
> % git checkout --ignore-other-worktrees -B main
> Already on 'main'
>
> The change of behaviour only applies to -B and it actually matches the
> documentation better.
>
>>> @@ -1533,13 +1534,13 @@ static int checkout_branch(struct checkout_opts *opts,
>>> if (!opts->can_switch_when_in_progress)
>>> die_if_some_operation_in_progress();
>>>
>>> - if (new_branch_info->path && !opts->force_detach && !opts->new_branch &&
>>> - !opts->ignore_other_worktrees) {
>>> + if (!opts->ignore_other_worktrees && !opts->force_detach &&
>>> + check_branch_path && ref_exists(check_branch_path)) {
>>
>> I think check_branch_path is NULL if opts->ignore_other_worktrees is set
>> so we could maybe lose "!opts->ignore_other_worktrees" here (or possibly
>> below where you set check_branch_path).
>
> opts->ignore_other_worktrees was kept from the original expression;
> you are correct that is not needed anymore, but I thought it didn't
> hurt and made the code intention clearer (meaning it is obvious to
> anyone new to the code that this code will be skipped if that flag is
> set), would using an assert or a comment be a better option?
It's a good point that it makes the intention clearer, maybe we should
just leave it as it is.
>>> /*
>>> * Extract branch name from command line arguments, so
>>> * all that is left is pathspecs.
>>> @@ -1741,6 +1751,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
>>> new_branch_info, opts, &rev);
>>> argv += n;
>>> argc -= n;
>>> +
>>> + if (!opts->ignore_other_worktrees && !check_branch_path && new_branch_info->path)
>>> + check_branch_path = xstrdup(new_branch_info->path);
>>
>> I'm a bit confused what this is doing.
>
> The branch we are interested in might come from 2 places, either it is
> a parameter to -b, which was picked up before, or it is the argument
> to the command itself, which was detected above.
Oh, of course. I was so focused on the -b that I'd forgotten we need the
same check when we're checking out an existing branch - thanks for
putting me right.
> If both are provided, we want to make sure to use the one from -b, or
> will have the bug you sharply spotted before, which was frankly
> embarrassing.
>
>>> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
>>> index d587e0b20d..7ab7e87440 100755
>>> @@ -133,17 +136,34 @@ test_expect_success SYMLINKS 'die the same branch is already checked out (symlin
>>> test_must_fail git -C here checkout newmain
>>> '
>>>
>>> -test_expect_success 'not die the same branch is already checked out' '
>>> +test_expect_success 'allow creating multiple worktrees for same branch with force' '
>>> + git worktree add --force anothernewmain newmain
>>> +'
>>> +
>>> +test_expect_success 'allow checkout/reset from the conflicted branch' '
>>
>> I'm not sure what "the conflicted branch" means (it reminds we of merge
>> conflicts).
>
> by "conflicted" I meant one that is checked out in more than one worktree
I think it would be clearer so say that rather than "conflicted" which
has a strong association with merge conflicts.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH v2] ls-tree: add --sparse-filter-oid argument
From: William Sprent @ 2023-01-27 11:58 UTC (permalink / raw)
To: Elijah Newren
Cc: William Sprent via GitGitGadget, git, Eric Sunshine,
Ævar Arnfjörð Bjarmason, Derrick Stolee,
Victoria Dye
In-Reply-To: <CABPp-BFtLdRV2zWXn0On0b6mOJgMAatwvUumUxfXfNXo9gc=HA@mail.gmail.com>
On 26/01/2023 04.25, Elijah Newren wrote:
> On Wed, Jan 25, 2023 at 8:16 AM William Sprent <williams@unity3d.com> wrote:
>>
>> On 25/01/2023 06.11, Elijah Newren wrote:
>>> It looks like Ævar and Victoria have both given really good reviews
>>> already, but I think I spotted some additional things to comment on.
>>>
>>> On Mon, Jan 23, 2023 at 3:46 AM William Sprent via GitGitGadget
>>> <gitgitgadget@gmail.com> wrote:
>>>>
>>>> From: William Sprent <williams@unity3d.com>
>>>>
>>>> There is currently no way to ask git the question "which files would be
>>>> part of a sparse checkout of commit X with sparse checkout patterns Y".
>>>> One use-case would be that tooling may want know whether sparse checkouts
>>>> of two commits contain the same content even if the full trees differ.
>>>
>>> Could you say more about this usecase? Why does tooling need or want
>>> to know this; won't a checkout of the new commit end up being quick
>>> and simple? (I'm not saying your usecase is bad, just curious that it
>>> had never occurred to me, and I'm afraid I'm still not sure what your
>>> purpose might be.)
>>>
>>
>> I'm thinking mainly about a monorepo context where there are a number of
>> distinct 'units' that can be described with sparse checkout patterns.
>> And perhaps there's some tooling that only wants to perform an action if
>> the content of a 'unit' changes.
>
> So, you're basically wanting to do something like
> git ls-tree --paths-matching-sparsity-file=<pattern-file> $COMMIT1
>> sparse-files
> git ls-tree --paths-matching-sparsity-file=<pattern-file> $COMMIT2
>>> sparse-files
> sort sparse-files | uniq >relevant-files
> git diff --name-only $COMMIT1 $COMMIT2 >changed-files
> and then checking whether relevant-files and changed-files have a
> non-empty intersection?
Well, the concrete use-case I'm exploring is something along the lines
of using the content hashes of sparse checkouts as cache keys for resource
heavy jobs (builds/tests/etc).
So, that would be something along the lines of,
git ls-tree -r --paths-matching-sparsity-file=<pattern-file> \
| sha1sum > cache-key
and then performing a lookup before performing an action (which would
then only be done in the context of the sparse checkout). My thinking
is that this only would require git and no additional tooling, which in
turn makes it very easy to reproduce the state where the job took place.
> Would that potentially be better handled by
> git diff --name-only $COMMIT1 $COMMIT2 | git check-ignore
> --ignore-file=<pattern-file> --stdin
> and seeing whether the output is non-empty? We'd have to add an
> "--ignore-file" option to check-ignore to override reading of
> .gitignore files and such, and it'd be slightly confusing because the
> documentation talks about "ignored" files rather than "selected"
> files, but that's a confusion point that has been with us ever since
> the gitignore mechanism was repurposed for sparse checkouts. Or maybe
> we could just add a check-sparsity helper, and then allow it to take
> directories in-lieu of patterns.
I don't think it necessarily would be better handled by that. But it would
be workable. It would be a matter of collating the output of
git ls-tree -r <commit>
with
git ls-tree --name-only -r <commit> | git check-ignore ...
Which is less ergonomic. But it is also a less intrusive change.
Really, the main thing is to expose the sparse filtering logic somehow, and
allow for building tooling on top of it.
> This seems nicer than opening a can of worms about letting every git
> command specify a different set of sparsity rules.
I think you are the better judge of how much of a can of worms that would
be. I don't think it would be too out of line with how git acts in general
though, as we have things like the the 'GIT_INDEX_FILE' env-var.
>> Depending on the repo, it won't necessarily be quick to check out the
>> commit with the given patterns. However, it is more about it being
>> inconvenient to have to have a working directory, especially so if you
>> want use the tooling in some kind of service or query rapidly about
>> different revisions/patterns.
>>
>>>> Another interesting use-case would be for tooling to use in conjunction
>>>> with 'git update-index --index-info'.
>>>
>>> Sorry, I'm not following. Could you expound on this a bit?
>>>
>>
>> I was imagining something along the lines of being able to generate new
>> tree objects based on what matches the given sparse checkout patterns.
>> Not that I have a specific use case for it right now.
>>
>> I think what I'm trying to evoke with that paragraph is that this
>> enables integrations with git that seem interesting and weren't possible
>> before.
>
> I'm not sure if it's interesting, frightening, or something else.
> Hard to say without better descriptions of usecases, which we can't
> have if we don't even have a usecase. I think I'd just strike this
> paragraph.
>
> [...]
Fair. Will do.
>>>> + (*d)->pl.use_cone_patterns = core_sparse_checkout_cone;
>>>
>>> Hmm, so the behavior still depends upon the current sparse-checkout
>>> (or lack thereof), despite the documentation and rationale of your
>>> feature as being there to check how a different sparse checkout would
>>> behave?
>>>
>>> I would hate to unconditionally turn cone_patterns off, since that
>>> would come with a huge performance penalty for the biggest repos. But
>>> turning it unconditionally on wouldn't be good for the non-cone users.
>>> This probably suggests we need something like another flag, or perhaps
>>> separate flags for each mode. Separate flags might provide the
>>> benefit of allowing cone mode users to specify directories rather than
>>> patterns, which would make it much easier for them to use.
>>>
>> I used 'core_sparse_checkout_cone' because I wanted to allow for the
>> cone mode optimisations, but I also figured that I should respect the
>> configuration. It doesn't change how the patterns are parsed in this case.
>>
>> I agree that it is a bit awkward to have to "translate" the directories
>> into patterns when wanting to use cone mode. I can try adding
>> '--[no]-cone' flags and see how that feels. Together with Victoria's
>> suggestions that would result in having the following flags:
>>
>> * --scope=(sparse|all)
>> * --sparse-patterns-file=<path>
>> * --[no]-cone: used together with --sparse-patterns-file to tell git
>> whether to interpret the patterns given as directories (cone) or
>> patterns (no-cone).
>>
>> Which seems like a lot at first glance. But it allows for passing
>> directories instead of patterns for cone mode, and is similar to the
>> behaviour of 'sparse-checkout set'.
>>
>> Does that seem like something that would make sense?
>
> --sparse-patterns-file still implies patterns; I think that would need
> some rewording.
Yeah. After sleeping on it, I also think that it becomes a difficult
interface to work with, and you'll get different results with the same
patterns whether you pass --cone or --no-cone, which seems error prone
to me.
For better or for worse, both cone and non-cone uses of sparse-checkouts
end up producing pattern files. And those pattern files do unambiguously
describe a filtering of the worktree whether it is in cone-mode or not.
Given that 'ls-tree' is more of a plumbing command, I think it might still
make sense to use the patterns. That would also make the interaction
a bit more logical to me -- e.g. if you want to override the patterns
you have to pass them in the same format as the ones that would be read
by default.
Then maybe it could eventually make sense to expose the translation of
cone-mode patterns as well, e.g.
git sparse-checkout set --cone --std-out dir1 dir2 dir3
or similar.
> More importantly, though, based on your usecase description, I wonder
> if you might be better served by either extending the check-ignore
> subcommand or adding a similar helper ("check-sparsity"?), rather than
> tweaking ls-tree.
^ permalink raw reply
* Git submodule commits
From: Martin Terra @ 2023-01-27 9:32 UTC (permalink / raw)
To: git
In-Reply-To: <CAN_dfTJY6bo7dBSBJoX7nxQMKt4VwVe2gAW7SEYM4cHffySrAQ@mail.gmail.com>
Hi!
We have git repository A and B.
A depends on B.
The team working on B has no visibility to A.
However, team A has visibility to both A and B.
You can think of it as if repository B is outsourced.
Now when team A is working on features and make a change which affects
also B, ideally there would be a single (single index)
commit/pull/etc. across A and B. Ideally, there would be no duplicate
copies of artifacts, i.e., optimally something from B commits only to
B and something form A only to A (think Submodule but with ability to
commit also).
How can this be accomplished using Git or is it a missing/planned
feature in git, or is there a 3rd party tool that helps with this?
Submodule and subtree don't seem to have these capabilities, or do
they?
Currently we have these set up on cvs and committing across projects
works effortlessly, but cvs is in many other ways outdated.
Related topics:
- https://jira.atlassian.com/browse/BSERV-4577
Thanks.
**
Martin
^ permalink raw reply
* Re: When using several ssh key, Git match ssh key by host, instead of hostname in ssh config file.
From: Yangyang Hua @ 2023-01-27 1:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org
In-Reply-To: <xmqqk019cmss.fsf@gitster.g>
Thank you!
I understand this feature of ssh.
________________________________________
From: Junio C Hamano <jch2355@gmail.com> on behalf of Junio C Hamano <gitster@pobox.com>
Sent: Friday, 27 January 2023 12:56 am
To: Yangyang Hua
Cc: git@vger.kernel.org
Subject: Re: When using several ssh key, Git match ssh key by host, instead of hostname in ssh config file.
Yangyang Hua <hyy_41@live.com> writes:
> Hi, I find when I use several ssh keys with the right config file
> and clone my private repo, git can't match the key by hostname.
> ...
> I think when git read ssh config, it uses host to match the key
> instead of hostname. Is this bug?
This useful feature is given by ssh, and Git does not deserve credit
for it. The config file of SSH allows you to write multiple entries
that points at the same host, e.g.
Host host1
HostName host.example.com
IdentityFile ~/.ssh/id_rsa_111
Host host2
HostName host.example.com
IdentityFile ~/.ssh/id_rsa_222
so that you can specify which key to use for the same destination
when you have more than one user there. "ssh host1" uses id_rsa_111
while "ssh host2" uses the other one, both connections going to the
same destination host.
If host.example.com were a hosting site like github.com, you can use this
feature to say
$ git push git@host1:/me/lesson1
to connect using id_rsa_111. If you use
$ git clone git@github.com:/me/lesson1
there is no clue which of the two entries you want to use.
^ permalink raw reply
* What's cooking in git.git (Jan 2023, #07; Thu, 26)
From: Junio C Hamano @ 2023-01-26 22:54 UTC (permalink / raw)
To: git
Here are the topics that have been cooking in my tree. Commits
prefixed with '+' are in 'next' (being in 'next' is a sign that a
topic is stable enough to be used and are candidate to be in a future
release). Commits prefixed with '-' are only in 'seen', and aren't
considered "accepted" at all. A topic without enough support may be
discarded after a long period of no activity.
Copies of the source code to Git live in many repositories, and the
following is a list of the ones I push into or their mirrors. Some
repositories have only a subset of branches.
With maint, master, next, seen, todo:
git://git.kernel.org/pub/scm/git/git.git/
git://repo.or.cz/alt-git.git/
https://kernel.googlesource.com/pub/scm/git/git/
https://github.com/git/git/
https://gitlab.com/git-vcs/git/
With all the integration branches and topics broken out:
https://github.com/gitster/git/
Even though the preformatted documentation in HTML and man format
are not sources, they are published in these repositories for
convenience (replace "htmldocs" with "manpages" for the manual
pages):
git://git.kernel.org/pub/scm/git/git-htmldocs.git/
https://github.com/gitster/git-htmldocs.git/
Release tarballs are available at:
https://www.kernel.org/pub/software/scm/git/
--------------------------------------------------
[Graduated to 'master']
* ab/cache-api-cleanup (2023-01-16) 5 commits
(merged to 'next' on 2023-01-16 at a0f388b149)
+ cache API: add a "INDEX_STATE_INIT" macro/function, add release_index()
+ read-cache.c: refactor set_new_index_sparsity() for subsequent commit
+ sparse-index API: BUG() out on NULL ensure_full_index()
+ sparse-index.c: expand_to_path() can assume non-NULL "istate"
+ builtin/difftool.c: { 0 }-initialize rather than using memset()
(this branch is used by ab/cache-api-cleanup-users.)
Code clean-up to tighten the use of in-core index in the API.
source: <cover-v2-0.6-00000000000-20230112T124842Z-avarab@gmail.com>
* ab/test-env-helper (2023-01-14) 1 commit
(merged to 'next' on 2023-01-16 at 82c17f02e5)
+ env-helper: move this built-in to "test-tool env-helper"
Remove "git env--helper" and demote it to a test-tool subcommand.
source: <patch-1.1-e662c570f1d-20230112T155226Z-avarab@gmail.com>
* ds/omit-trailing-hash-in-index (2023-01-17) 1 commit
(merged to 'next' on 2023-01-17 at 8dde3cf2db)
+ t1600: fix racy index.skipHash test
(this branch is used by ab/cache-api-cleanup-users.)
Quickfix for a topic already in 'master'.
source: <76204710-356a-2a85-9057-302e6619b9df@github.com>
* en/t6426-todo-cleanup (2023-01-14) 1 commit
(merged to 'next' on 2023-01-16 at 7d13842eeb)
+ t6426: fix TODO about making test more comprehensive
Test clean-up.
source: <pull.1462.v2.git.1673722187025.gitgitgadget@gmail.com>
* jc/format-patch-v-unleak (2023-01-16) 1 commit
(merged to 'next' on 2023-01-16 at 2155d512bc)
+ format-patch: unleak "-v <num>"
Plug a small leak.
source: <xmqqv8l8gr6s.fsf@gitster.g>
* kn/attr-from-tree (2023-01-14) 2 commits
(merged to 'next' on 2023-01-16 at 426f357683)
+ attr: add flag `--source` to work with tree-ish
+ t0003: move setup for `--all` into new block
"git check-attr" learned to take an optional tree-ish to read the
.gitattributes file from.
source: <cover.1673684790.git.karthik.188@gmail.com>
* rs/ls-tree-path-expansion-fix (2023-01-14) 2 commits
(merged to 'next' on 2023-01-16 at 6359f28ba7)
+ ls-tree: remove dead store and strbuf for quote_c_style()
+ ls-tree: fix expansion of repeated %(path)
"git ls-tree --format='%(path) %(path)' $tree $path" showed the
path three times, which has been corrected.
source: <55ae7333-3a13-0575-93ed-f858a1c2877e@web.de>
* rs/use-enhanced-bre-on-macos (2023-01-08) 1 commit
(merged to 'next' on 2023-01-16 at 9b80d4253f)
+ use enhanced basic regular expressions on macOS
Newer regex library macOS stopped enabling GNU-like enhanced BRE,
where '\(A\|B\)' works as alternation, unless explicitly asked with
the REG_ENHANCED flag. "git grep" now can be compiled to do so, to
retain the old behaviour.
source: <26a0d4ca-3d97-ace4-1a1f-92b1ee6715a6@web.de>
* sk/win32-close-handle-upon-pthread-join (2023-01-04) 2 commits
(merged to 'next' on 2023-01-16 at faa279fd5b)
+ win32: close handles of threads that have been joined
+ win32: prepare pthread.c for change by formatting
Pthread emulation on Win32 leaked thread handle when a thread is
joined.
source: <pull.1406.v13.git.git.1672762819.gitgitgadget@gmail.com>
* zh/scalar-progress (2023-01-16) 1 commit
(merged to 'next' on 2023-01-17 at d4c25cc71f)
+ scalar: show progress if stderr refers to a terminal
"scalar" learned to give progress bar.
source: <pull.1441.v3.git.1673442860379.gitgitgadget@gmail.com>
--------------------------------------------------
[New Topics]
* ar/markup-em-dash (2023-01-23) 1 commit
(merged to 'next' on 2023-01-24 at 0367e3035f)
+ Documentation: render dash correctly
Doc mark-up updates.
Will merge to 'master'.
source: <20230123090114.429844-1-rybak.a.v@gmail.com>
* ab/hook-api-with-stdin (2023-01-23) 5 commits
- hook: support a --to-stdin=<path> option for testing
- sequencer: use the new hook API for the simpler "post-rewrite" call
- hook API: support passing stdin to hooks, convert am's 'post-rewrite'
- run-command: allow stdin for run_processes_parallel
- run-command.c: remove dead assignment in while-loop
Extend the run-hooks API to allow feeding data from the standard
input when running the hook script(s).
Expecting review responses.
source: <cover-0.5-00000000000-20230123T170550Z-avarab@gmail.com>
* as/ssh-signing-improve-key-missing-error (2023-01-25) 1 commit
(merged to 'next' on 2023-01-25 at 140f2c2c60)
+ ssh signing: better error message when key not in agent
Improve the error message given when private key is not loaded in
the ssh agent in the codepath to sign with an ssh key.
Will merge to 'master'.
source: <pull.1270.v3.git.git.1674650450662.gitgitgadget@gmail.com>
* en/rebase-incompatible-opts (2023-01-25) 10 commits
- rebase: provide better error message for apply options vs. merge config
- rebase: put rebase_options initialization in single place
- rebase: fix formatting of rebase --reapply-cherry-picks option in docs
- rebase: clarify the OPT_CMDMODE incompatibilities
- rebase: add coverage of other incompatible options
- rebase: fix incompatiblity checks for --[no-]reapply-cherry-picks
- rebase: fix docs about incompatibilities with --root
- rebase: remove --allow-empty-message from incompatible opts
- rebase: flag --apply and --merge as incompatible
- rebase: mark --update-refs as requiring the merge backend
"git rebase" often ignored incompatible options instead of
complaining, which has been corrected.
Will merge to 'next'.
Replaces en/rebase-update-refs-needs-merge-backend.
source: <pull.1466.v5.git.1674619434.gitgitgadget@gmail.com>
* gm/request-pull-with-non-pgp-signed-tags (2023-01-25) 1 commit
- request-pull: filter out SSH/X.509 tag signatures
source: <20230125234725.3918563-1-gwymor@tilde.club>
--------------------------------------------------
[Stalled]
* ja/worktree-orphan (2023-01-13) 4 commits
- worktree add: add hint to direct users towards --orphan
- worktree add: add --orphan flag
- worktree add: refactor opt exclusion tests
- worktree add: include -B in usage docs
'git worktree add' learned how to create a worktree based on an
orphaned branch with `--orphan`.
Expecting a reroll.
cf. <11be1b0e-ee38-119f-1d80-cb818946116b@dunelm.org.uk>
source: <20230109173227.29264-1-jacobabel@nullpo.dev>
* ab/avoid-losing-exit-codes-in-tests (2022-12-20) 6 commits
- tests: don't lose misc "git" exit codes
- tests: don't lose "git" exit codes in "! ( git ... | grep )"
- tests: don't lose exit status with "test <op> $(git ...)"
- tests: don't lose exit status with "(cd ...; test <op> $(git ...))"
- t/lib-patch-mode.sh: fix ignored exit codes
- auto-crlf tests: don't lose exit code in loops and outside tests
Test clean-up.
Expecting a hopefully minor and final reroll.
cf. <1182283a-4a78-3c99-e716-a8c3e58a5823@web.de>
cf. <xmqqsfhb0vum.fsf@gitster.g>
source: <cover-v4-0.6-00000000000-20221219T101240Z-avarab@gmail.com>
* tl/notes--blankline (2022-11-09) 5 commits
- notes.c: introduce "--no-blank-line" option
- notes.c: provide tips when target and append note are both empty
- notes.c: drop unreachable code in 'append_edit()'
- notes.c: cleanup for "designated init" and "char ptr init"
- notes.c: cleanup 'strbuf_grow' call in 'append_edit'
'git notes append' was taught '--[no-]blank-line' to conditionally
add a LF between a new and existing note.
Expecting a reroll.
cf. <CAPig+cRcezSp4Rqt1Y9bD-FT6+7b0g9qHfbGRx65AOnw2FQXKg@mail.gmail.com>
source: <cover.1667980450.git.dyroneteng@gmail.com>
* mc/switch-advice (2022-11-09) 1 commit
- po: use `switch` over `checkout` in error message
Use 'switch' instead of 'checkout' in an error message.
Waiting for review response.
source: <pull.1308.git.git.1668018620148.gitgitgadget@gmail.com>
* js/range-diff-mbox (2022-11-23) 1 commit
- range-diff: support reading mbox files
'git range-diff' gained support for reading either side from an .mbox
file instead of a revision range.
Waiting for review response.
cf. <xmqqr0xupmnf.fsf@gitster.g>
source: <pull.1420.v3.git.1669108102092.gitgitgadget@gmail.com>
* ab/tag-object-type-errors (2022-11-22) 5 commits
- tag: don't emit potentially incorrect "object is a X, not a Y"
- tag: don't misreport type of tagged objects in errors
- object tests: add test for unexpected objects in tags
- object-file.c: free the "t.tag" in check_tag()
- Merge branch 'jk/parse-object-type-mismatch' into ab/tag-object-type-errors
Hardening checks around mismatched object types when one of those
objects is a tag.
Expecting a reroll.
cf. <xmqqzgb5jz5c.fsf@gitster.g>
cf. <xmqqsfgxjugi.fsf@gitster.g>
source: <cover-0.4-00000000000-20221118T113442Z-avarab@gmail.com>
* ab/config-multi-and-nonbool (2022-11-27) 9 commits
- for-each-repo: with bad config, don't conflate <path> and <cmd>
- config API: add "string" version of *_value_multi(), fix segfaults
- config API users: test for *_get_value_multi() segfaults
- for-each-repo: error on bad --config
- config API: have *_multi() return an "int" and take a "dest"
- versioncmp.c: refactor config reading next commit
- config tests: add "NULL" tests for *_get_value_multi()
- config tests: cover blind spots in git_die_config() tests
- for-each-repo tests: test bad --config keys
Assorted config API updates.
Expecting a reroll.
source: <cover-v3-0.9-00000000000-20221125T093158Z-avarab@gmail.com>
* ed/fsmonitor-inotify (2022-12-13) 6 commits
- fsmonitor: update doc for Linux
- fsmonitor: test updates
- fsmonitor: enable fsmonitor for Linux
- fsmonitor: implement filesystem change listener for Linux
- fsmonitor: determine if filesystem is local or remote
- fsmonitor: prepare to share code between Mac OS and Linux
Bundled fsmonitor for Linux using inotify API.
Needs review on the updated round.
source: <pull.1352.v5.git.git.1670882286.gitgitgadget@gmail.com>
* jc/spell-id-in-both-caps-in-message-id (2022-12-17) 1 commit
- e-mail workflow: Message-ID is spelled with ID in both capital letters
Consistently spell "Message-ID" as such, not "Message-Id".
Needs review.
source: <xmqqsfhgnmqg.fsf@gitster.g>
* cb/grep-fallback-failing-jit (2022-12-17) 1 commit
- grep: fall back to interpreter mode if JIT fails
In an environment where dynamically generated code is prohibited to
run (e.g. SELinux), failure to JIT pcre patterns is expected. Fall
back to interpreted execution in such a case.
Expecting a reroll.
cf. <f680b274-fa85-6624-096a-7753a2671c15@grsecurity.net>
source: <20221216121557.30714-1-minipli@grsecurity.net>
* ad/test-record-count-when-harness-is-in-use (2022-12-25) 1 commit
- test-lib: allow storing counts with test harnesses
Allow summary results from tests to be written to t/test-results
directory even when a test harness like 'prove' is in use.
Needs review.
source: <20221224225200.1027806-1-adam@dinwoodie.org>
* so/diff-merges-more (2022-12-18) 5 commits
- diff-merges: improve --diff-merges documentation
- diff-merges: issue warning on lone '-m' option
- diff-merges: support list of values for --diff-merges
- diff-merges: implement log.diffMerges-m-imply-p config
- diff-merges: implement [no-]hide option and log.diffMergesHide config
Assorted updates to "--diff-merges=X" option.
May want to discard. Breaking compatibility does not seem worth it.
source: <20221217132955.108542-1-sorganov@gmail.com>
--------------------------------------------------
[Cooking]
* ab/cache-api-cleanup-users (2023-01-17) 3 commits
(merged to 'next' on 2023-01-18 at c5a4374652)
+ treewide: always have a valid "index_state.repo" member
+ Merge branch 'ds/omit-trailing-hash-in-index' into ab/cache-api-cleanup-users
+ Merge branch 'ab/cache-api-cleanup' into ab/cache-api-cleanup-users
Updates the users of the cache API.
Will merge to 'master'.
cf. <db312853-81a1-542b-db96-d816c463516c@github.com>
source: <patch-1.1-b4998652822-20230117T135234Z-avarab@gmail.com>
* cb/checkout-same-branch-twice (2023-01-20) 1 commit
- checkout/switch: disallow checking out same branch in multiple worktrees
"git checkout -B $branch" failed to protect against checking out
a branch that is checked out elsewhere, unlike "git branch -f" did.
Expecting a (hopefully final) reroll.
cf. <8f24fc3c-c30f-dc70-5a94-5ee4ed3de102@dunelm.org.uk>
source: <20230120113553.24655-1-carenas@gmail.com>
* sa/cat-file-mailmap--batch-check (2023-01-18) 1 commit
(merged to 'next' on 2023-01-18 at 25ecb1dd3a)
+ git-cat-file.txt: fix list continuations rendering literally
Docfix.
Will merge to 'master'.
source: <20230118082749.1252459-1-martin.agren@gmail.com>
* pb/branch-advice-recurse-submodules (2023-01-18) 1 commit
(merged to 'next' on 2023-01-19 at 13747fc72d)
+ branch: improve advice when --recurse-submodules fails
Improve advice message given when "git branch --resurse-submodules"
fails.
Will merge to 'master'.
source: <pull.1464.git.1673890908453.gitgitgadget@gmail.com>
* ab/sequencer-unleak (2023-01-18) 8 commits
- commit.c: free() revs.commit in get_fork_point()
- builtin/rebase.c: free() "options.strategy_opts"
- sequencer.c: always free() the "msgbuf" in do_pick_commit()
- builtin/rebase.c: fix "options.onto_name" leak
- builtin/revert.c: move free-ing of "revs" to replay_opts_release()
- rebase & sequencer API: fix get_replay_opts() leak in "rebase"
- sequencer.c: split up sequencer_remove_state()
- rebase: use "cleanup" pattern in do_interactive_rebase()
Plug leaks in sequencer subsystem and its users.
Expecting a hopefully minor and final reroll.
cf. <xmqqedry17r4.fsf@gitster.g>
source: <cover-v3-0.8-00000000000-20230118T160600Z-avarab@gmail.com>
* jk/hash-object-literally-fd-leak (2023-01-19) 1 commit
(merged to 'next' on 2023-01-19 at fff9b60a36)
+ hash-object: fix descriptor leak with --literally
Leakfix.
Will merge to 'master'.
source: <Y8ijpJqtkDTi792i@coredump.intra.peff.net>
* jc/doc-branch-update-checked-out-branch (2023-01-18) 1 commit
(merged to 'next' on 2023-01-19 at 970900a232)
+ branch: document `-f` and linked worktree behaviour
Document that "branch -f <branch>" disables only the safety to
avoid recreating an existing branch.
Will merge to 'master'.
source: <xmqqa62f2dj1.fsf_-_@gitster.g>
* jc/doc-checkout-b (2023-01-19) 1 commit
(merged to 'next' on 2023-01-23 at 95340e1941)
+ checkout: document -b/-B to highlight the differences from "git branch"
Clarify how "checkout -b/-B" and "git branch [-f]" are similar but
different in the documentation.
Will merge to 'master'.
source: <xmqqtu0m1m9i.fsf@gitster.g>
* cw/fetch-remote-group-with-duplication (2023-01-19) 1 commit
(merged to 'next' on 2023-01-20 at 7f00e43209)
+ fetch: fix duplicate remote parallel fetch bug
"git fetch <group>", when "<group>" of remotes lists the same
remote twice, unnecessarily failed when parallel fetching was
enabled, which has been corrected.
Will merge to 'master'.
source: <20230119220538.1522464-1-calvinwan@google.com>
* po/pretty-format-columns-doc (2023-01-19) 5 commits
(merged to 'next' on 2023-01-23 at d41cb5f527)
+ doc: pretty-formats note wide char limitations, and add tests
+ doc: pretty-formats describe use of ellipsis in truncation
+ doc: pretty-formats document negative column alignments
+ doc: pretty-formats: delineate `%<|(` parameter values
+ doc: pretty-formats: separate parameters from placeholders
Clarify column-padding operators in the pretty format string.
Will merge to 'master'.
source: <20230119181827.1319-1-philipoakley@iee.email>
* jk/hash-object-fsck (2023-01-19) 7 commits
(merged to 'next' on 2023-01-23 at 985e87fc34)
+ fsck: do not assume NUL-termination of buffers
+ hash-object: use fsck for object checks
+ fsck: provide a function to fsck buffer without object struct
+ t: use hash-object --literally when created malformed objects
+ t7030: stop using invalid tag name
+ t1006: stop using 0-padded timestamps
+ t1007: modernize malformed object tests
"git hash-object" now checks that the resulting object is well
formed with the same code as "git fsck".
Will merge to 'master'.
source: <Y8hX+pIZUKXsyYj5@coredump.intra.peff.net>
source: <Y8ifa7hyqxSbL92U@coredump.intra.peff.net>
* tb/t0003-invoke-dd-more-portably (2023-01-22) 1 commit
(merged to 'next' on 2023-01-23 at 917aa24a27)
+ t0003: call dd with portable blocksize
Test portability fix.
Will merge to 'master'.
source: <20230122062839.14542-1-tboegi@web.de>
* jc/attr-doc-fix (2023-01-22) 1 commit
(merged to 'next' on 2023-01-24 at a25d37cb0f)
+ attr: fix instructions on how to check attrs
Comment fix.
Will merge to 'master'.
source: <pull.1441.v2.git.git.1674447742078.gitgitgadget@gmail.com>
* rj/avoid-switching-to-already-used-branch (2023-01-22) 3 commits
- switch: reject if the branch is already checked out elsewhere (test)
- rebase: refuse to switch to a branch already checked out elsewhere (test)
- branch: fix die_if_checked_out() when ignore_current_worktree
A few subcommands have been taught to stop users from working on a
branch that is being used in another worktree linked to the same
repository.
Expecting a (hopefully final) reroll.
cf. <d61a2393-64c8-da49-fe13-00bc4a52d5e3@gmail.com>
source: <f7f45f54-9261-45ea-3399-8ba8dee6832b@gmail.com>
* rj/bisect-already-used-branch (2023-01-22) 1 commit
- bisect: fix "reset" when branch is checked out elsewhere
Allow "git bisect reset [name]" to check out the named branch (or
the original one) even when the branch is already checked out in a
different worktree linked to the same repository.
Leaning negative. Why is it a good thing?
cf. <xmqqo7qqovp1.fsf@gitster.g>
source: <1c36c334-9f10-3859-c92f-3d889e226769@gmail.com>
* en/ls-files-doc-update (2023-01-13) 4 commits
- ls-files: guide folks to --exclude-standard over other --exclude* options
- ls-files: clarify descriptions of status tags for -t
- ls-files: clarify descriptions of file selection options
- ls-files: add missing documentation for --resolve-undo option
Doc update to ls-files.
Will merge to 'next'.
source: <pull.1463.git.1673584914.gitgitgadget@gmail.com>
* ms/send-email-feed-header-to-validate-hook (2023-01-19) 2 commits
- send-email: expose header information to git-send-email's sendemail-validate hook
- send-email: refactor header generation functions
"git send-email" learned to give the e-mail headers to the validate
hook by passing an extra argument from the command line.
Expecting a (hopefully final) reroll.
cf. <c1ba0a28-3c39-b313-2757-dceb02930334@amd.com>
source: <20230120012459.920932-1-michael.strawbridge@amd.com>
* ds/bundle-uri-5 (2023-01-23) 10 commits
- bundle-uri: test missing bundles with heuristic
- bundle-uri: store fetch.bundleCreationToken
- fetch: fetch from an external bundle URI
- bundle-uri: drop bundle.flag from design doc
- clone: set fetch.bundleURI if appropriate
- bundle-uri: download in creationToken order
- bundle-uri: parse bundle.<id>.creationToken values
- bundle-uri: parse bundle.heuristic=creationToken
- t5558: add tests for creationToken heuristic
- bundle: optionally skip reachability walk
The bundle-URI subsystem adds support for creation-token heuristics
to help incremental fetches.
Expecting a reroll.
cf. <771a2993-85bd-0831-0977-24204f84e206@github.com>
cf. <01f97aff-58a1-ef2c-e668-d37ea513c64e@github.com>
cf. <ecc6b167-f5c4-48ce-3973-461d1659ed40@github.com>
source: <pull.1454.v2.git.1674487310.gitgitgadget@gmail.com>
* tc/cat-file-z-use-cquote (2023-01-16) 1 commit
- cat-file: quote-format name in error when using -z
"cat-file" in the batch mode that is fed NUL-terminated pathnames
learned to cquote them in its error output (otherwise, a funny
pathname with LF in it would break the lines in the output stream).
Expecting a reroll.
cf. <2a2a46f0-a9bc-06a6-72e1-28800518777c@dunelm.org.uk>
source: <20230116190749.4141516-1-toon@iotcl.com>
* cb/grep-pcre-ucp (2023-01-18) 1 commit
(merged to 'next' on 2023-01-19 at 2c7e531839)
+ grep: correctly identify utf-8 characters with \{b,w} in -P
"grep -P" learned to use Unicode Character Property to grok
character classes when processing \b and \w etc.
Will merge to 'master'.
cf. <xmqqzgaf2zpt.fsf@gitster.g>
source: <20230108155217.2817-1-carenas@gmail.com>
* cw/submodule-status-in-parallel (2023-01-17) 6 commits
- submodule: call parallel code from serial status
- diff-lib: parallelize run_diff_files for submodules
- diff-lib: refactor match_stat_with_submodule
- submodule: move status parsing into function
- submodule: strbuf variable rename
- run-command: add duplicate_output_fn to run_processes_parallel_opts
"git submodule status" learned to run the comparison in submodule
repositories in parallel.
Expecting a reroll.
cf. <CAFySSZBiW7=ZTmXRaLzCoKUi0Jd=fzvW5PJ6=Ka0jKHoP2ddSw@mail.gmail.com>
cf. <kl6lo7qlvg4h.fsf@chooglen-macbookpro.roam.corp.google.com>
source: <20230104215415.1083526-1-calvinwan@google.com>
* ab/various-leak-fixes (2023-01-18) 19 commits
- push: free_refs() the "local_refs" in set_refspecs()
- receive-pack: free() the "ref_name" in "struct command"
- grep API: plug memory leaks by freeing "header_list"
- grep.c: refactor free_grep_patterns()
- object-file.c: release the "tag" in check_tag()
- builtin/merge.c: free "&buf" on "Your local changes..." error
- builtin/merge.c: use fixed strings, not "strbuf", fix leak
- show-branch: free() allocated "head" before return
- commit-graph: fix a parse_options_concat() leak
- http-backend.c: fix cmd_main() memory leak, refactor reg{exec,free}()
- http-backend.c: fix "dir" and "cmd_arg" leaks in cmd_main()
- worktree: fix a trivial leak in prune_worktrees()
- repack: fix leaks on error with "goto cleanup"
- name-rev: don't xstrdup() an already dup'd string
- various: add missing clear_pathspec(), fix leaks
- clone: use free() instead of UNLEAK()
- commit-graph: use free_commit_graph() instead of UNLEAK()
- bundle.c: don't leak the "args" in the "struct child_process"
- tests: mark tests as passing with SANITIZE=leak
Leak fixes.
Needs review.
source: <cover-v5-00.19-00000000000-20230118T120334Z-avarab@gmail.com>
* rj/branch-unborn-in-other-worktrees (2023-01-19) 3 commits
- branch: rename orphan branches in any worktree
- branch: description for orphan branch errors
- avoid unnecessary worktrees traversing
Error messages given when working on an unborn branch that is
checked out in another worktree have been improvved.
Expecting a reroll.
cf. <527f7315-be7b-7ec0-04fc-d07da7d4fefa@gmail.com>
source: <34a58449-4f2e-66ef-ea01-119186aebd23@gmail.com>
* km/send-email-with-v-reroll-count (2022-11-27) 1 commit
(merged to 'next' on 2023-01-19 at 9b3543471c)
+ send-email: relay '-v N' to format-patch
"git send-email -v 3" used to be expanded to "git send-email
--validate 3" when the user meant to pass them down to
"format-patch", which has been corrected.
Will merge to 'master'.
source: <87edtp5uws.fsf@kyleam.com>
* mc/credential-helper-auth-headers (2023-01-20) 12 commits
(merged to 'next' on 2023-01-25 at cb95006bb2)
+ credential: add WWW-Authenticate header to cred requests
+ http: read HTTP WWW-Authenticate response headers
+ http: replace unsafe size_t multiplication with st_mult
+ test-http-server: add sending of arbitrary headers
+ test-http-server: add simple authentication
+ test-http-server: pass Git requests to http-backend
+ test-http-server: add HTTP request parsing
+ test-http-server: add HTTP error response function
+ test-http-server: add stub HTTP server test helper
+ daemon: rename some esoteric/laboured terminology
+ daemon: libify child process handling functions
+ daemon: libify socket setup and option functions
Extending credential helper protocol.
Will kick out of 'next'. The test-only server is an eyesore.
cf. <e57c1ca3-c21c-db41-a386-e5887f46055c@github.com>
cf. <Y9JkMLueCwjkLHOr@coredump.intra.peff.net>
source: <pull.1352.v7.git.1674252530.gitgitgadget@gmail.com>
--------------------------------------------------
[Discarded]
* jc/ci-deprecated-declarations-are-not-fatal (2023-01-14) 1 commit
(merged to 'next' on 2023-01-14 at 5efb778ab0)
+ ci: do not die on deprecated-declarations warning
CI build fix for overzealous -Werror.
Reverted out of 'next'
Preferring jk/curl-avoid-deprecated-api that fixes the code properly.
source: <xmqq7cxpkpjp.fsf@gitster.g>
* po/pretty-hard-trunc (2022-11-13) 1 commit
. pretty-formats: add hard truncation, without ellipsis, options
Add a new pretty format which truncates without ellipsis.
Superseded by the 'po/pretty-format-columns-doc' topic.
source: <20221112143616.1429-1-philipoakley@iee.email>
* en/rebase-update-refs-needs-merge-backend (2023-01-22) 9 commits
(merged to 'next' on 2023-01-23 at 1b65346647)
+ rebase: provide better error message for apply options vs. merge config
+ rebase: put rebase_options initialization in single place
+ rebase: fix formatting of rebase --reapply-cherry-picks option in docs
+ rebase: clarify the OPT_CMDMODE incompatibilities
+ rebase: add coverage of other incompatible options
+ rebase: fix docs about incompatibilities with --root
+ rebase: remove --allow-empty-message from incompatible opts
+ rebase: flag --apply and --merge as incompatible
+ rebase: mark --update-refs as requiring the merge backend
The "--update-refs" feature of "git rebase" requires the use of the
merge backend, while "--whitespace=fix" feature does not work with
the said backend. Notice the combination and error out, instead of
silently ignoring one of the features requested.
Reverted out of 'next' to be replaced with en/rebase-incompatible-opts
source: <pull.1466.v4.git.1674367961.gitgitgadget@gmail.com>
* rs/tree-parse-mode-overflow-check (2023-01-21) 1 commit
. tree-walk: disallow overflowing modes
Reject tree objects with entries whose mode bits are overly wide.
Retracted.
cf. <b4b48877-5b80-e96f-d09f-2fe275f42950@web.de>
source: <d673fde7-7eb2-6306-86b6-1c1a4c988ee8@web.de>
* cc/filtered-repack (2022-12-25) 3 commits
. gc: add gc.repackFilter config option
. repack: add --filter=<filter-spec> option
. pack-objects: allow --filter without --stdout
"git repack" learns to discard objects that ought to be retrievable
again from the promisor remote.
May want to discard. Its jaggy edges may be a bit too sharp.
cf. <Y7WTv19aqiFCU8au@ncase>
source: <20221221040446.2860985-1-christian.couder@gmail.com>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox