* [Bug?] "git diff --no-rename A B"
From: Junio C Hamano @ 2024-01-18 1:07 UTC (permalink / raw)
To: git
When the user misspells "--no-renames" as "--no-rename", it seems
that the rename detection (which is ont by default these days) still
kicks in, which means the misspelt option is silently ignored.
Should we show an error message instead?
^ permalink raw reply
* Re: [PATCH 00/12] Group reffiles tests
From: Junio C Hamano @ 2024-01-18 1:17 UTC (permalink / raw)
To: John Cai via GitGitGadget; +Cc: git, John Cai
In-Reply-To: <pull.1647.git.git.1705521155.gitgitgadget@gmail.com>
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> This series groups REFFILES specific tests together. These tests are
> currently grouped together across the test suite based on functionality.
> However, since they exercise low-level behavior specific to the refs backend
> being used (in these cases, the ref-files backend), group them together
> based on which refs backend they test. This way, in the near future when the
> reftables backend gets upstreamed we can add tests that exercise the
> reftables backend close by in the t06xx area.
>
> These patches also remove the REFFILES prerequisite, since all the tests in
> t06xx are reffiles specific.
As we already have REFFILES lazy prereq, even _before_ we enable the
reftable backend, I think that we should start t0600 and t0602 with
. ./test-lib.sh
if ! test_have_prereq REFFILES
then
skip_all='skipping reffiles specific tests'
test_done
fi
which is more in line with the existing convention. It is more
efficient than "forcing t0600 and t0602 to run always with reffiles"
when you have a CI job that uses reftable for all tests and another
CI job that uses reffiles for all tests.
> In the near future, once the reftable backend is upstreamed, all
> the tests in t06xx will be forced to run with the reffiles
> backend.
Presumably if there are reftable backend specific tests, they will
also be given names out of t06xx range, right? And then they will
be skipped when the test is not using reftable as the default ref
backend, using the REFTABLE prerequisite in a similar way as shown
above for REFFILES, right?
Thanks.
^ permalink raw reply
* [PATCH v2 0/4] Strengthen fsck checks for submodule URLs
From: Victoria Dye via GitGitGadget @ 2024-01-18 1:55 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Jeff King, Victoria Dye
In-Reply-To: <pull.1635.git.1704822817.gitgitgadget@gmail.com>
While testing 'git fsck' checks on .gitmodules URLs, I noticed that some
invalid URLs were passing the checks. Digging into it a bit more, the issue
turned out to be that 'credential_from_url_gently()' parses certain URLs
(like "http://example.com:something/deeper/path") incorrectly, in a way that
appeared to return a valid result.
Fortunately, these URLs are rejected in fetches/clones/pushes anyway because
'url_normalize()' (called in 'validate_remote_url()') correctly identifies
them as invalid. So, to bring 'git fsck' in line with other (stronger)
validation done on remote URLs, this series replaces the
'credential_from_url_gently()' check with one that uses 'url_normalize()'.
* Patch 1 moves 'check_submodule_url()' to a public location so that it can
be used outside of 'fsck.c'.
* Patch 2 removes the obsolete/never-used code in 'test-tool submodule
check-name' handling names provided on the command line.
* Patch 3 adds a 'check-url' mode to 'test-tool submodule', calling the
now-public 'check_submodule_url()' method on a given URL, and adds new
tests checking valid and invalid submodule URLs.
* Patch 4 replaces the 'credential_from_url_gently()' check with
'url_normalize()' followed by 'url_decode()' and an explicit check for
newlines (to preserve the newline handling added in 07259e74ec1 (fsck:
detect gitmodules URLs with embedded newlines, 2020-03-11)).
Changes since V1
================
* Added 'TEST_TOOL_CHECK_URL_USAGE' to 'submodule_usage'.
* Removed unused/unreachable code related to command line inputs in
'test-tool submodule check-name' and 'test-tool submodule check-url'.
* Split the new t7450 test case into two tests (the first contains URLs
that are validated successfully, the second demonstrates a URL
incorrectly marked valid) to clearly show which pattern is handled
improperly. The tests are merged in the final patch once the validation
is corrected.
Thanks!
* Victoria
Victoria Dye (4):
submodule-config.h: move check_submodule_url
test-submodule: remove command line handling for check-name
t7450: test submodule urls
submodule-config.c: strengthen URL fsck check
fsck.c | 133 ----------------------------------
submodule-config.c | 140 ++++++++++++++++++++++++++++++++++++
submodule-config.h | 3 +
t/helper/test-submodule.c | 52 +++++++++-----
t/t7450-bad-git-dotfiles.sh | 26 +++++++
5 files changed, 203 insertions(+), 151 deletions(-)
base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1635%2Fvdye%2Fvdye%2Fstrengthen-fsck-url-checks-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1635/vdye/vdye/strengthen-fsck-url-checks-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1635
Range-diff vs v1:
1: 588de3022d7 = 1: ce1de0406ef submodule-config.h: move check_submodule_url
-: ----------- > 2: 14e8834c38b test-submodule: remove command line handling for check-name
2: cf7848edffc ! 3: b6843a58389 t7450: test submodule urls
@@ Metadata
## Commit message ##
t7450: test submodule urls
- Add a test to 't7450-bad-git-dotfiles.sh' to check the validity of different
- submodule URLs. To test this directly (without setting up test repositories
- & submodules), add a 'check-url' subcommand to 'test-tool submodule' that
- calls 'check_submodule_url' in the same way that 'check-name' calls
- 'check_submodule_name'.
+ Add tests to 't7450-bad-git-dotfiles.sh' to check the validity of different
+ submodule URLs. To verify this directly (without setting up test
+ repositories & submodules), add a 'check-url' subcommand to 'test-tool
+ submodule' that calls 'check_submodule_url' in the same way that
+ 'check-name' calls 'check_submodule_name'.
- Mark the test with 'test_expect_failure' because, as it stands,
- 'check_submodule_url' marks certain invalid URLs valid. Specifically, the
- invalid URL "http://example.com:test/foo.git" is incorrectly marked valid in
- the test.
+ Add two tests to separately address cases where the URL check correctly
+ filters out invalid URLs and cases where the check misses invalid URLs. Mark
+ the latter ("url check misses invalid cases") with 'test_expect_failure' to
+ indicate that this not the undesired behavior.
Signed-off-by: Victoria Dye <vdye@github.com>
@@ t/helper/test-submodule.c: static const char *submodule_check_name_usage[] = {
};
+#define TEST_TOOL_CHECK_URL_USAGE \
-+ "test-tool submodule check-url <url>"
++ "test-tool submodule check-url"
+static const char *submodule_check_url_usage[] = {
+ TEST_TOOL_CHECK_URL_USAGE,
+ NULL
@@ t/helper/test-submodule.c: static const char *submodule_check_name_usage[] = {
#define TEST_TOOL_IS_ACTIVE_USAGE \
"test-tool submodule is-active <name>"
static const char *submodule_is_active_usage[] = {
-@@ t/helper/test-submodule.c: static const char *submodule_usage[] = {
+@@ t/helper/test-submodule.c: static const char *submodule_resolve_relative_url_usage[] = {
+
+ static const char *submodule_usage[] = {
+ TEST_TOOL_CHECK_NAME_USAGE,
++ TEST_TOOL_CHECK_URL_USAGE,
+ TEST_TOOL_IS_ACTIVE_USAGE,
+ TEST_TOOL_RESOLVE_RELATIVE_URL_USAGE,
NULL
};
+-/* Filter stdin to print only valid names. */
+-static int check_name(void)
+typedef int (*check_fn_t)(const char *);
+
- /*
- * Exit non-zero if any of the submodule names given on the command line is
- * invalid. If no names are given, filter stdin to print only valid names
- * (which is primarily intended for testing).
- */
--static int check_name(int argc, const char **argv)
-+static int check_submodule(int argc, const char **argv, check_fn_t check_fn)
++/*
++ * Apply 'check_fn' to each line of stdin, printing values that pass the check
++ * to stdout.
++ */
++static int check_submodule(check_fn_t check_fn)
{
- if (argc > 1) {
- while (*++argv) {
-- if (check_submodule_name(*argv) < 0)
-+ if (check_fn(*argv) < 0)
- return 1;
- }
- } else {
- struct strbuf buf = STRBUF_INIT;
- while (strbuf_getline(&buf, stdin) != EOF) {
-- if (!check_submodule_name(buf.buf))
-+ if (!check_fn(buf.buf))
- printf("%s\n", buf.buf);
- }
- strbuf_release(&buf);
+ struct strbuf buf = STRBUF_INIT;
+ while (strbuf_getline(&buf, stdin) != EOF) {
+- if (!check_submodule_name(buf.buf))
++ if (!check_fn(buf.buf))
+ printf("%s\n", buf.buf);
+ }
+ strbuf_release(&buf);
@@ t/helper/test-submodule.c: static int cmd__submodule_check_name(int argc, const char **argv)
if (argc)
usage_with_options(submodule_check_name_usage, options);
-- return check_name(argc, argv);
-+ return check_submodule(argc, argv, check_submodule_name);
+- return check_name();
++ return check_submodule(check_submodule_name);
+}
+
+static int cmd__submodule_check_url(int argc, const char **argv)
@@ t/helper/test-submodule.c: static int cmd__submodule_check_name(int argc, const
+ if (argc)
+ usage_with_options(submodule_check_url_usage, options);
+
-+ return check_submodule(argc, argv, check_submodule_url);
++ return check_submodule(check_submodule_url);
}
static int cmd__submodule_is_active(int argc, const char **argv)
@@ t/t7450-bad-git-dotfiles.sh: test_expect_success 'check names' '
test_cmp expect actual
'
-+test_expect_failure 'check urls' '
++test_expect_success 'check urls' '
+ cat >expect <<-\EOF &&
+ ./bar/baz/foo.git
+ https://example.com/foo.git
@@ t/t7450-bad-git-dotfiles.sh: test_expect_success 'check names' '
+ ./%0ahost=example.com/foo.git
+ https://one.example.com/evil?%0ahost=two.example.com
+ https:///example.com/foo.git
-+ http://example.com:test/foo.git
+ https::example.com/foo.git
+ http:::example.com/foo.git
+ EOF
+
+ test_cmp expect actual
+'
++
++# NEEDSWORK: the URL checked here is not valid (and will not work as a remote if
++# a user attempts to clone it), but the fsck check passes.
++test_expect_failure 'url check misses invalid cases' '
++ test-tool submodule check-url >actual <<-\EOF &&
++ http://example.com:test/foo.git
++ EOF
++
++ test_must_be_empty actual
++'
+
test_expect_success 'create innocent subrepo' '
git init innocent &&
3: 893071530d3 ! 4: b79b1a71780 submodule-config.c: strengthen URL fsck check
@@ submodule-config.c: int check_submodule_url(const char *url)
## t/t7450-bad-git-dotfiles.sh ##
-@@ t/t7450-bad-git-dotfiles.sh: test_expect_success 'check names' '
+@@ t/t7450-bad-git-dotfiles.sh: test_expect_success 'check urls' '
+ ./%0ahost=example.com/foo.git
+ https://one.example.com/evil?%0ahost=two.example.com
+ https:///example.com/foo.git
++ http://example.com:test/foo.git
+ https::example.com/foo.git
+ http:::example.com/foo.git
+ EOF
+@@ t/t7450-bad-git-dotfiles.sh: test_expect_success 'check urls' '
test_cmp expect actual
'
--test_expect_failure 'check urls' '
-+test_expect_success 'check urls' '
- cat >expect <<-\EOF &&
- ./bar/baz/foo.git
- https://example.com/foo.git
+-# NEEDSWORK: the URL checked here is not valid (and will not work as a remote if
+-# a user attempts to clone it), but the fsck check passes.
+-test_expect_failure 'url check misses invalid cases' '
+- test-tool submodule check-url >actual <<-\EOF &&
+- http://example.com:test/foo.git
+- EOF
+-
+- test_must_be_empty actual
+-'
+-
+ test_expect_success 'create innocent subrepo' '
+ git init innocent &&
+ git -C innocent commit --allow-empty -m foo
--
gitgitgadget
^ permalink raw reply
* [PATCH v2 1/4] submodule-config.h: move check_submodule_url
From: Victoria Dye via GitGitGadget @ 2024-01-18 1:55 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Jeff King, Victoria Dye, Victoria Dye
In-Reply-To: <pull.1635.v2.git.1705542918.gitgitgadget@gmail.com>
From: Victoria Dye <vdye@github.com>
Move 'check_submodule_url' out of 'fsck.c' and into 'submodule-config.h' as
a public method, similar to 'check_submodule_name'. With the function now
accessible outside of 'fsck', it can be used in a later commit to extend
'test-tool submodule' to check the validity of submodule URLs as it does
with names in the 'check-name' subcommand.
Other than its location, no changes are made to 'check_submodule_url' in
this patch.
Signed-off-by: Victoria Dye <vdye@github.com>
---
fsck.c | 133 --------------------------------------------
submodule-config.c | 134 +++++++++++++++++++++++++++++++++++++++++++++
submodule-config.h | 3 +
3 files changed, 137 insertions(+), 133 deletions(-)
diff --git a/fsck.c b/fsck.c
index 1ad02fcdfab..8ded0a473a4 100644
--- a/fsck.c
+++ b/fsck.c
@@ -20,7 +20,6 @@
#include "packfile.h"
#include "submodule-config.h"
#include "config.h"
-#include "credential.h"
#include "help.h"
static ssize_t max_tree_entry_len = 4096;
@@ -1047,138 +1046,6 @@ int fsck_tag_standalone(const struct object_id *oid, const char *buffer,
return ret;
}
-static int starts_with_dot_slash(const char *const path)
-{
- return path_match_flags(path, PATH_MATCH_STARTS_WITH_DOT_SLASH |
- PATH_MATCH_XPLATFORM);
-}
-
-static int starts_with_dot_dot_slash(const char *const path)
-{
- return path_match_flags(path, PATH_MATCH_STARTS_WITH_DOT_DOT_SLASH |
- PATH_MATCH_XPLATFORM);
-}
-
-static int submodule_url_is_relative(const char *url)
-{
- return starts_with_dot_slash(url) || starts_with_dot_dot_slash(url);
-}
-
-/*
- * Count directory components that a relative submodule URL should chop
- * from the remote_url it is to be resolved against.
- *
- * In other words, this counts "../" components at the start of a
- * submodule URL.
- *
- * Returns the number of directory components to chop and writes a
- * pointer to the next character of url after all leading "./" and
- * "../" components to out.
- */
-static int count_leading_dotdots(const char *url, const char **out)
-{
- int result = 0;
- while (1) {
- if (starts_with_dot_dot_slash(url)) {
- result++;
- url += strlen("../");
- continue;
- }
- if (starts_with_dot_slash(url)) {
- url += strlen("./");
- continue;
- }
- *out = url;
- return result;
- }
-}
-/*
- * Check whether a transport is implemented by git-remote-curl.
- *
- * If it is, returns 1 and writes the URL that would be passed to
- * git-remote-curl to the "out" parameter.
- *
- * Otherwise, returns 0 and leaves "out" untouched.
- *
- * Examples:
- * http::https://example.com/repo.git -> 1, https://example.com/repo.git
- * https://example.com/repo.git -> 1, https://example.com/repo.git
- * git://example.com/repo.git -> 0
- *
- * This is for use in checking for previously exploitable bugs that
- * required a submodule URL to be passed to git-remote-curl.
- */
-static int url_to_curl_url(const char *url, const char **out)
-{
- /*
- * We don't need to check for case-aliases, "http.exe", and so
- * on because in the default configuration, is_transport_allowed
- * prevents URLs with those schemes from being cloned
- * automatically.
- */
- if (skip_prefix(url, "http::", out) ||
- skip_prefix(url, "https::", out) ||
- skip_prefix(url, "ftp::", out) ||
- skip_prefix(url, "ftps::", out))
- return 1;
- if (starts_with(url, "http://") ||
- starts_with(url, "https://") ||
- starts_with(url, "ftp://") ||
- starts_with(url, "ftps://")) {
- *out = url;
- return 1;
- }
- return 0;
-}
-
-static int check_submodule_url(const char *url)
-{
- const char *curl_url;
-
- if (looks_like_command_line_option(url))
- return -1;
-
- if (submodule_url_is_relative(url) || starts_with(url, "git://")) {
- char *decoded;
- const char *next;
- int has_nl;
-
- /*
- * This could be appended to an http URL and url-decoded;
- * check for malicious characters.
- */
- decoded = url_decode(url);
- has_nl = !!strchr(decoded, '\n');
-
- free(decoded);
- if (has_nl)
- return -1;
-
- /*
- * URLs which escape their root via "../" can overwrite
- * the host field and previous components, resolving to
- * URLs like https::example.com/submodule.git and
- * https:///example.com/submodule.git that were
- * susceptible to CVE-2020-11008.
- */
- if (count_leading_dotdots(url, &next) > 0 &&
- (*next == ':' || *next == '/'))
- return -1;
- }
-
- else if (url_to_curl_url(url, &curl_url)) {
- struct credential c = CREDENTIAL_INIT;
- int ret = 0;
- if (credential_from_url_gently(&c, curl_url, 1) ||
- !*c.host)
- ret = -1;
- credential_clear(&c);
- return ret;
- }
-
- return 0;
-}
-
struct fsck_gitmodules_data {
const struct object_id *oid;
struct fsck_options *options;
diff --git a/submodule-config.c b/submodule-config.c
index f4dd482abc9..3b295e9f89c 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -14,6 +14,8 @@
#include "parse-options.h"
#include "thread-utils.h"
#include "tree-walk.h"
+#include "url.h"
+#include "credential.h"
/*
* submodule cache lookup structure
@@ -228,6 +230,138 @@ int check_submodule_name(const char *name)
return 0;
}
+static int starts_with_dot_slash(const char *const path)
+{
+ return path_match_flags(path, PATH_MATCH_STARTS_WITH_DOT_SLASH |
+ PATH_MATCH_XPLATFORM);
+}
+
+static int starts_with_dot_dot_slash(const char *const path)
+{
+ return path_match_flags(path, PATH_MATCH_STARTS_WITH_DOT_DOT_SLASH |
+ PATH_MATCH_XPLATFORM);
+}
+
+static int submodule_url_is_relative(const char *url)
+{
+ return starts_with_dot_slash(url) || starts_with_dot_dot_slash(url);
+}
+
+/*
+ * Count directory components that a relative submodule URL should chop
+ * from the remote_url it is to be resolved against.
+ *
+ * In other words, this counts "../" components at the start of a
+ * submodule URL.
+ *
+ * Returns the number of directory components to chop and writes a
+ * pointer to the next character of url after all leading "./" and
+ * "../" components to out.
+ */
+static int count_leading_dotdots(const char *url, const char **out)
+{
+ int result = 0;
+ while (1) {
+ if (starts_with_dot_dot_slash(url)) {
+ result++;
+ url += strlen("../");
+ continue;
+ }
+ if (starts_with_dot_slash(url)) {
+ url += strlen("./");
+ continue;
+ }
+ *out = url;
+ return result;
+ }
+}
+/*
+ * Check whether a transport is implemented by git-remote-curl.
+ *
+ * If it is, returns 1 and writes the URL that would be passed to
+ * git-remote-curl to the "out" parameter.
+ *
+ * Otherwise, returns 0 and leaves "out" untouched.
+ *
+ * Examples:
+ * http::https://example.com/repo.git -> 1, https://example.com/repo.git
+ * https://example.com/repo.git -> 1, https://example.com/repo.git
+ * git://example.com/repo.git -> 0
+ *
+ * This is for use in checking for previously exploitable bugs that
+ * required a submodule URL to be passed to git-remote-curl.
+ */
+static int url_to_curl_url(const char *url, const char **out)
+{
+ /*
+ * We don't need to check for case-aliases, "http.exe", and so
+ * on because in the default configuration, is_transport_allowed
+ * prevents URLs with those schemes from being cloned
+ * automatically.
+ */
+ if (skip_prefix(url, "http::", out) ||
+ skip_prefix(url, "https::", out) ||
+ skip_prefix(url, "ftp::", out) ||
+ skip_prefix(url, "ftps::", out))
+ return 1;
+ if (starts_with(url, "http://") ||
+ starts_with(url, "https://") ||
+ starts_with(url, "ftp://") ||
+ starts_with(url, "ftps://")) {
+ *out = url;
+ return 1;
+ }
+ return 0;
+}
+
+int check_submodule_url(const char *url)
+{
+ const char *curl_url;
+
+ if (looks_like_command_line_option(url))
+ return -1;
+
+ if (submodule_url_is_relative(url) || starts_with(url, "git://")) {
+ char *decoded;
+ const char *next;
+ int has_nl;
+
+ /*
+ * This could be appended to an http URL and url-decoded;
+ * check for malicious characters.
+ */
+ decoded = url_decode(url);
+ has_nl = !!strchr(decoded, '\n');
+
+ free(decoded);
+ if (has_nl)
+ return -1;
+
+ /*
+ * URLs which escape their root via "../" can overwrite
+ * the host field and previous components, resolving to
+ * URLs like https::example.com/submodule.git and
+ * https:///example.com/submodule.git that were
+ * susceptible to CVE-2020-11008.
+ */
+ if (count_leading_dotdots(url, &next) > 0 &&
+ (*next == ':' || *next == '/'))
+ return -1;
+ }
+
+ else if (url_to_curl_url(url, &curl_url)) {
+ struct credential c = CREDENTIAL_INIT;
+ int ret = 0;
+ if (credential_from_url_gently(&c, curl_url, 1) ||
+ !*c.host)
+ ret = -1;
+ credential_clear(&c);
+ return ret;
+ }
+
+ return 0;
+}
+
static int name_and_item_from_var(const char *var, struct strbuf *name,
struct strbuf *item)
{
diff --git a/submodule-config.h b/submodule-config.h
index 958f320ac6c..b6133af71b0 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -89,6 +89,9 @@ int config_set_in_gitmodules_file_gently(const char *key, const char *value);
*/
int check_submodule_name(const char *name);
+/* Returns 0 if the URL valid per RFC3986 and -1 otherwise. */
+int check_submodule_url(const char *url);
+
/*
* Note: these helper functions exist solely to maintain backward
* compatibility with 'fetch' and 'update_clone' storing configuration in
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 2/4] test-submodule: remove command line handling for check-name
From: Victoria Dye via GitGitGadget @ 2024-01-18 1:55 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Jeff King, Victoria Dye, Victoria Dye
In-Reply-To: <pull.1635.v2.git.1705542918.gitgitgadget@gmail.com>
From: Victoria Dye <vdye@github.com>
The 'check-name' subcommand to 'test-tool submodule' is documented as being
able to take a command line argument '<name>'. However, this does not work -
and has never worked - because 'argc > 0' triggers the usage message in
'cmd__submodule_check_name()'. To simplify the helper and avoid future
confusion around proper use of the subcommand, remove any references to
command line arguments for 'check-name' in usage strings and handling in
'check_name()'.
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Victoria Dye <vdye@github.com>
---
t/helper/test-submodule.c | 29 +++++++++--------------------
1 file changed, 9 insertions(+), 20 deletions(-)
diff --git a/t/helper/test-submodule.c b/t/helper/test-submodule.c
index 50c154d0370..9adbc8d1568 100644
--- a/t/helper/test-submodule.c
+++ b/t/helper/test-submodule.c
@@ -9,7 +9,7 @@
#include "submodule.h"
#define TEST_TOOL_CHECK_NAME_USAGE \
- "test-tool submodule check-name <name>"
+ "test-tool submodule check-name"
static const char *submodule_check_name_usage[] = {
TEST_TOOL_CHECK_NAME_USAGE,
NULL
@@ -36,26 +36,15 @@ static const char *submodule_usage[] = {
NULL
};
-/*
- * Exit non-zero if any of the submodule names given on the command line is
- * invalid. If no names are given, filter stdin to print only valid names
- * (which is primarily intended for testing).
- */
-static int check_name(int argc, const char **argv)
+/* Filter stdin to print only valid names. */
+static int check_name(void)
{
- if (argc > 1) {
- while (*++argv) {
- if (check_submodule_name(*argv) < 0)
- return 1;
- }
- } else {
- struct strbuf buf = STRBUF_INIT;
- while (strbuf_getline(&buf, stdin) != EOF) {
- if (!check_submodule_name(buf.buf))
- printf("%s\n", buf.buf);
- }
- strbuf_release(&buf);
+ struct strbuf buf = STRBUF_INIT;
+ while (strbuf_getline(&buf, stdin) != EOF) {
+ if (!check_submodule_name(buf.buf))
+ printf("%s\n", buf.buf);
}
+ strbuf_release(&buf);
return 0;
}
@@ -69,7 +58,7 @@ static int cmd__submodule_check_name(int argc, const char **argv)
if (argc)
usage_with_options(submodule_check_name_usage, options);
- return check_name(argc, argv);
+ return check_name();
}
static int cmd__submodule_is_active(int argc, const char **argv)
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 3/4] t7450: test submodule urls
From: Victoria Dye via GitGitGadget @ 2024-01-18 1:55 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Jeff King, Victoria Dye, Victoria Dye
In-Reply-To: <pull.1635.v2.git.1705542918.gitgitgadget@gmail.com>
From: Victoria Dye <vdye@github.com>
Add tests to 't7450-bad-git-dotfiles.sh' to check the validity of different
submodule URLs. To verify this directly (without setting up test
repositories & submodules), add a 'check-url' subcommand to 'test-tool
submodule' that calls 'check_submodule_url' in the same way that
'check-name' calls 'check_submodule_name'.
Add two tests to separately address cases where the URL check correctly
filters out invalid URLs and cases where the check misses invalid URLs. Mark
the latter ("url check misses invalid cases") with 'test_expect_failure' to
indicate that this not the undesired behavior.
Signed-off-by: Victoria Dye <vdye@github.com>
---
t/helper/test-submodule.c | 35 +++++++++++++++++++++++++++++++----
t/t7450-bad-git-dotfiles.sh | 35 +++++++++++++++++++++++++++++++++++
2 files changed, 66 insertions(+), 4 deletions(-)
diff --git a/t/helper/test-submodule.c b/t/helper/test-submodule.c
index 9adbc8d1568..7197969a081 100644
--- a/t/helper/test-submodule.c
+++ b/t/helper/test-submodule.c
@@ -15,6 +15,13 @@ static const char *submodule_check_name_usage[] = {
NULL
};
+#define TEST_TOOL_CHECK_URL_USAGE \
+ "test-tool submodule check-url"
+static const char *submodule_check_url_usage[] = {
+ TEST_TOOL_CHECK_URL_USAGE,
+ NULL
+};
+
#define TEST_TOOL_IS_ACTIVE_USAGE \
"test-tool submodule is-active <name>"
static const char *submodule_is_active_usage[] = {
@@ -31,17 +38,23 @@ static const char *submodule_resolve_relative_url_usage[] = {
static const char *submodule_usage[] = {
TEST_TOOL_CHECK_NAME_USAGE,
+ TEST_TOOL_CHECK_URL_USAGE,
TEST_TOOL_IS_ACTIVE_USAGE,
TEST_TOOL_RESOLVE_RELATIVE_URL_USAGE,
NULL
};
-/* Filter stdin to print only valid names. */
-static int check_name(void)
+typedef int (*check_fn_t)(const char *);
+
+/*
+ * Apply 'check_fn' to each line of stdin, printing values that pass the check
+ * to stdout.
+ */
+static int check_submodule(check_fn_t check_fn)
{
struct strbuf buf = STRBUF_INIT;
while (strbuf_getline(&buf, stdin) != EOF) {
- if (!check_submodule_name(buf.buf))
+ if (!check_fn(buf.buf))
printf("%s\n", buf.buf);
}
strbuf_release(&buf);
@@ -58,7 +71,20 @@ static int cmd__submodule_check_name(int argc, const char **argv)
if (argc)
usage_with_options(submodule_check_name_usage, options);
- return check_name();
+ return check_submodule(check_submodule_name);
+}
+
+static int cmd__submodule_check_url(int argc, const char **argv)
+{
+ struct option options[] = {
+ OPT_END()
+ };
+ argc = parse_options(argc, argv, "test-tools", options,
+ submodule_check_url_usage, 0);
+ if (argc)
+ usage_with_options(submodule_check_url_usage, options);
+
+ return check_submodule(check_submodule_url);
}
static int cmd__submodule_is_active(int argc, const char **argv)
@@ -184,6 +210,7 @@ static int cmd__submodule_config_writeable(int argc, const char **argv UNUSED)
static struct test_cmd cmds[] = {
{ "check-name", cmd__submodule_check_name },
+ { "check-url", cmd__submodule_check_url },
{ "is-active", cmd__submodule_is_active },
{ "resolve-relative-url", cmd__submodule_resolve_relative_url},
{ "config-list", cmd__submodule_config_list },
diff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh
index 35a31acd4d7..c73b1c92ecc 100755
--- a/t/t7450-bad-git-dotfiles.sh
+++ b/t/t7450-bad-git-dotfiles.sh
@@ -45,6 +45,41 @@ test_expect_success 'check names' '
test_cmp expect actual
'
+test_expect_success 'check urls' '
+ cat >expect <<-\EOF &&
+ ./bar/baz/foo.git
+ https://example.com/foo.git
+ http://example.com:80/deeper/foo.git
+ EOF
+
+ test-tool submodule check-url >actual <<-\EOF &&
+ ./bar/baz/foo.git
+ https://example.com/foo.git
+ http://example.com:80/deeper/foo.git
+ -a./foo
+ ../../..//test/foo.git
+ ../../../../../:localhost:8080/foo.git
+ ..\../.\../:example.com/foo.git
+ ./%0ahost=example.com/foo.git
+ https://one.example.com/evil?%0ahost=two.example.com
+ https:///example.com/foo.git
+ https::example.com/foo.git
+ http:::example.com/foo.git
+ EOF
+
+ test_cmp expect actual
+'
+
+# NEEDSWORK: the URL checked here is not valid (and will not work as a remote if
+# a user attempts to clone it), but the fsck check passes.
+test_expect_failure 'url check misses invalid cases' '
+ test-tool submodule check-url >actual <<-\EOF &&
+ http://example.com:test/foo.git
+ EOF
+
+ test_must_be_empty actual
+'
+
test_expect_success 'create innocent subrepo' '
git init innocent &&
git -C innocent commit --allow-empty -m foo
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 4/4] submodule-config.c: strengthen URL fsck check
From: Victoria Dye via GitGitGadget @ 2024-01-18 1:55 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Jeff King, Victoria Dye, Victoria Dye
In-Reply-To: <pull.1635.v2.git.1705542918.gitgitgadget@gmail.com>
From: Victoria Dye <vdye@github.com>
Update the validation of "curl URL" submodule URLs (i.e. those that specify
an "http[s]" or "ftp[s]" protocol) in 'check_submodule_url()' to catch more
invalid URLs. The existing validation using 'credential_from_url_gently()'
parses certain URLs incorrectly, leading to invalid submodule URLs passing
'git fsck' checks. Conversely, 'url_normalize()' - used to validate remote
URLs in 'remote_get()' - correctly identifies the invalid URLs missed by
'credential_from_url_gently()'.
To catch more invalid cases, replace 'credential_from_url_gently()' with
'url_normalize()' followed by a 'url_decode()' and a check for newlines
(mirroring 'check_url_component()' in the 'credential_from_url_gently()'
validation).
Signed-off-by: Victoria Dye <vdye@github.com>
---
submodule-config.c | 16 +++++++++++-----
t/t7450-bad-git-dotfiles.sh | 11 +----------
2 files changed, 12 insertions(+), 15 deletions(-)
diff --git a/submodule-config.c b/submodule-config.c
index 3b295e9f89c..54130f6a385 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -15,7 +15,7 @@
#include "thread-utils.h"
#include "tree-walk.h"
#include "url.h"
-#include "credential.h"
+#include "urlmatch.h"
/*
* submodule cache lookup structure
@@ -350,12 +350,18 @@ int check_submodule_url(const char *url)
}
else if (url_to_curl_url(url, &curl_url)) {
- struct credential c = CREDENTIAL_INIT;
int ret = 0;
- if (credential_from_url_gently(&c, curl_url, 1) ||
- !*c.host)
+ char *normalized = url_normalize(curl_url, NULL);
+ if (normalized) {
+ char *decoded = url_decode(normalized);
+ if (strchr(decoded, '\n'))
+ ret = -1;
+ free(normalized);
+ free(decoded);
+ } else {
ret = -1;
- credential_clear(&c);
+ }
+
return ret;
}
diff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh
index c73b1c92ecc..46d4fb0354b 100755
--- a/t/t7450-bad-git-dotfiles.sh
+++ b/t/t7450-bad-git-dotfiles.sh
@@ -63,6 +63,7 @@ test_expect_success 'check urls' '
./%0ahost=example.com/foo.git
https://one.example.com/evil?%0ahost=two.example.com
https:///example.com/foo.git
+ http://example.com:test/foo.git
https::example.com/foo.git
http:::example.com/foo.git
EOF
@@ -70,16 +71,6 @@ test_expect_success 'check urls' '
test_cmp expect actual
'
-# NEEDSWORK: the URL checked here is not valid (and will not work as a remote if
-# a user attempts to clone it), but the fsck check passes.
-test_expect_failure 'url check misses invalid cases' '
- test-tool submodule check-url >actual <<-\EOF &&
- http://example.com:test/foo.git
- EOF
-
- test_must_be_empty actual
-'
-
test_expect_success 'create innocent subrepo' '
git init innocent &&
git -C innocent commit --allow-empty -m foo
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()
From: Dragan Simic @ 2024-01-18 6:15 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Rubén Justo, Git List
In-Reply-To: <20240111080429.GG48154@coredump.intra.peff.net>
On 2024-01-11 09:04, Jeff King wrote:
> On Wed, Jan 10, 2024 at 06:45:49PM +0100, Dragan Simic wrote:
>
>> 4) As a careful git user that remembers important things, you go back
>> to your git configuration file and set core.verboseAdvice to true, and
>> the additional advices are back, telling you how to disable the
>> unwanted advice.
>>
>> 5) After you disable the unwanted advice, you set core.verboseAdvice
>> back to false and keep it that way until the next redundant advice
>> pops up.
>>
>> However, I do see that this approach has its downsides, for example
>> the need for the unwanted advice to be displayed again together with
>> the additional advice, by executing the appropriate git commands,
>> after the above-described point #4.
>
> Right, the need to re-trigger the advice is the problematic part here,
> I
> think. In some cases it is easy. But in others, especially commands
> which mutate the repo state (like the empty-commit rebase that started
> this thread), you may need to do a lot of work to recreate the
> situation.
I apologize for my delayed response.
Yes, recreating some situations may simply require an unacceptable
amount of work and time, making it pretty much infeasible in practice.
>> Let's see what it would look like with the granular, per-advice on/off
>> knobs:
>>
>> 1) You use git and find some advice useful, so you decide to keep it
>> displayed. However, the additional advice about turning the advice
>> off becomes annoying a bit, or better said, it becomes redundant
>> because the main advice stays.
>>
>> 2) As a result, you follow the additional advice and set the specific
>> knob to false, and voila, the redundant additional advice disappears.
>> Of course, it once again isn't perfect, as the next point will clearly
>> show.
>>
>> 3) You keep using git, and one of the advices that you previously used
>> to find useful becomes no longer needed. But, what do you do, where's
>> that helpful additional advice telling you how to turn the advice off?
>>
>> 4) Now you need to dig though the manual pages, or to re-enable the
>> additional advices in your git configuration file, perhaps all of them
>> at once, while keeping a backup of your original settings, to restore
>> it later. Then, you again need to wait until the original advice gets
>> displayed.
>
> These steps (3) and (4) seem unlikely to me. These are by definition
> messages you have seen before and decided to configure specifically (to
> "always", and not just "off"). So you probably only have a handful (if
> even more than one) of them in your config file.
Yes, the number of such messages shouldn't, in general, get out of hand
over time. Though, different users may do it differently.
> Whereas for the case I am worried about, you are exposed to a _new_
> message that you haven't seen before (and is maybe even new to Git!),
> from the much-larger pool of "all advice messages Git knows about".
>
> It's possible we could implement both mechanisms and let people choose
> which one they want, depending on their preferences. It's not very much
> code. I just hesitate to make things more complex than they need to be.
Perhaps having both options available could be a good thing. Though,
adding quite a few knobs may end up confusing the users, so we should
make sure to document it well.
^ permalink raw reply
* Re: [Bug?] "git diff --no-rename A B"
From: Dragan Simic @ 2024-01-18 6:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq34uvtpob.fsf@gitster.g>
Hello,
On 2024-01-18 02:07, Junio C Hamano wrote:
> When the user misspells "--no-renames" as "--no-rename", it seems
> that the rename detection (which is ont by default these days) still
> kicks in, which means the misspelt option is silently ignored.
> Should we show an error message instead?
Unless I'm missing something obvious, an error condition should be
generated in general when an unknown command-line option is specified.
^ permalink raw reply
* Re: [PATCH 1/3] ci: make p4 setup on macOS more robust
From: Matthias Aßhauer @ 2024-01-18 7:19 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <a5d725bea7b2cc2b8f66682920dbba016e89da9e.1705318985.git.ps@pks.im>
On Mon, 15 Jan 2024, Patrick Steinhardt wrote:
> When setting up Perforce on macOS we put both `p4` and `p4d` into
> "$HOME/bin". On GitHub CI this directory is indeed contained in the PATH
> environment variable and thus there is no need for additional setup than
> to put the binaries there. But GitLab CI does not do this, and thus our
> Perforce-based tests would be skipped there even though we download the
> binaries.
>
> Refactor the setup code to become more robust by downloading binaries
> into a separate directory which we then manually append to our PATH.
> This matches what we do on Linux-based jobs.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> ci/install-dependencies.sh | 10 ++++------
> ci/lib.sh | 3 +++
> 2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> index 4f407530d3..b4e22de3cb 100755
> --- a/ci/install-dependencies.sh
> +++ b/ci/install-dependencies.sh
> @@ -37,15 +37,13 @@ macos-*)
> test -z "$BREW_INSTALL_PACKAGES" ||
> brew install $BREW_INSTALL_PACKAGES
> brew link --force gettext
> - mkdir -p $HOME/bin
> - (
> - cd $HOME/bin
> +
> + mkdir -p "$P4_PATH"
> + pushd "$P4_PATH"
> wget -q "$P4WHENCE/bin.macosx1015x86_64/helix-core-server.tgz" &&
> tar -xf helix-core-server.tgz &&
> sudo xattr -d com.apple.quarantine p4 p4d 2>/dev/null || true
> - )
> - PATH="$PATH:${HOME}/bin"
> - export PATH
Didn't this put "$HOME/bin" on the PATH? And isn't the main premise of
this patch that "$HOME/bin" is not on the PATH?
or is the issue mainly about where we modify and export PATH and was
masked by GitHub Actions already having "$HOME/bin" on the PATH?
> + popd
>
> if test -n "$CC_PACKAGE"
> then
> diff --git a/ci/lib.sh b/ci/lib.sh
> index c749b21366..f631206a44 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -344,6 +344,9 @@ macos-*)
> then
> MAKEFLAGS="$MAKEFLAGS APPLE_COMMON_CRYPTO_SHA1=Yes"
> fi
> +
> + P4_PATH="$HOME/custom/p4"
> + export PATH="$P4_PATH:$PATH"
> ;;
> esac
>
> --
> 2.43.GIT
>
>
^ permalink raw reply
* Re: [PATCH] precious-files.txt: new document proposing new precious file type
From: Sebastian Thiel @ 2024-01-18 7:51 UTC (permalink / raw)
To: Junio C Hamano
Cc: Elijah Newren, Elijah Newren via GitGitGadget, git, Josh Triplett,
Phillip Wood
In-Reply-To: <xmqqsf3nl2b3.fsf@gitster.g>
I thought it would be helpful to see the syntax being referred to here,
as first brought up by Phillip Wood:
#(keep)
/my-precious-file
The main benefit I see for it is that it's extensible, despite having
trouble imagining what such extension would be 10 years from now.
On the flip side, since it's already using a comment, people will
be even more inclined to document the reason for the preciousness
of the file.
# The kernel configuration, typically created by running a TUI program
#(keep)
.config
As a side-effect of the syntax, it's obvious this is an 'upgrade', with
perfect backwards compatibility as old git does the same as always.
I'd love to take first steps into the implementation, and if the above
should be the syntax to use, I'd be happy to submit a patch for parsing
it, along with initial support for precious files in `git clean` and
`git status`.
Does that sound like a reasonable next step?
On 27 Dec 2023, at 23:15, Junio C Hamano wrote:
> Elijah Newren <newren@gmail.com> writes:
>
>> There are
>> precisely two choices in our design for how older Git versions can
>> treat precious files:
>> * ignored-and-expendable
>> * untracked-and-precious
>> If we pick syntax that causes older Git versions to treat precious
>> files as ignored-and-expendable, we risk deleting important files.
>
> Yes but not really. I'd expect the adoption of precious feature and
> the adoption of versions of Git that supports that feature will go
> more or less hand in hand. Projects that, for any reason, need to
> keep their participants at pre-precious versions of Git would
> naturally refrain from marking the "precious" paths in their "ignore"
> mechanism before their participants are ready, so even if we chose
> syntax that will make the precious ones mistaken as merely ignored,
> the damage would be fairly small.
^ permalink raw reply
* git fsck does not check the packed-refs file
From: R. Diez @ 2024-01-18 8:02 UTC (permalink / raw)
To: git
Hi all:
I have been hit by an unfortunate system problem, and as a result, a few files in my Git repository got corrupted on my last git push. Some random blocks of bytes were overwritten with binary zeros, so I started getting weird unpacking errors etc.
It took a while to realise what the problem was. During my investigation, I ran "git fsck", which reported no problems, and then "git push" failed.
One of the very few corrupted files was packed-refs. This is a text file, so it was easy to compare it and see the corrupting binary zeros. But that made me wonder what "git fsck" checks.
I am guessing that "git fsck" does not check file packed-refs at all. I mean, it does not even attempt to parse it, in order to check whether at least the format makes any sense. Only "git push" does it.
What other parts of the repository does "git fsck" not check then?
The repository check is suspiciously fast. Is there a slow way to check that a repository is fine? I mean, something along the lines of checking whether every commit can be checked out without problems.
Best regards,
rdiez
^ permalink raw reply
* Re: [PATCH 1/1] builtin/show-ref: treat directory directory as non-existing in --exists
From: Toon Claes @ 2024-01-18 8:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Patrick Steinhardt, Eric Sunshine, git
In-Reply-To: <xmqqmst3tv7j.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Yup. The patch looks good. Here is what I tentatively queued.
>
> ----- >8 -----
> From: Toon Claes <toon@iotcl.com>
> Date: Wed, 10 Jan 2024 15:15:59 +0100
> Subject: [PATCH] builtin/show-ref: treat directory directory as non-existing
> in --exists
You have a duplicate "directory" here.
> 9080a7f178 (builtin/show-ref: add new mode to check for reference
> existence, 2023-10-31) added the option --exists to git-show-ref(1).
>
> When you use this option against a ref that doesn't exist, but it is
> a parent directory of an existing ref, you get the following error:
>
> $ git show-ref --exists refs/heads
> error: failed to look up reference: Is a directory
>
> when the ref-files backend is in use. To be more clear to user,
> hide the error about having found a directory. What matters to the
> user is that the named ref does not exist. Instead, print the same
> error as when the ref was not found:
>
> error: reference does not exist
>
> Signed-off-by: Toon Claes <toon@iotcl.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> builtin/show-ref.c | 2 +-
> t/t1403-show-ref.sh | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/show-ref.c b/builtin/show-ref.c
> index 7aac525a87..6025ce223d 100644
> --- a/builtin/show-ref.c
> +++ b/builtin/show-ref.c
> @@ -239,7 +239,7 @@ static int cmd_show_ref__exists(const char **refs)
> if (refs_read_raw_ref(get_main_ref_store(the_repository), ref,
> &unused_oid, &unused_referent, &unused_type,
> &failure_errno)) {
> - if (failure_errno == ENOENT) {
> + if (failure_errno == ENOENT || failure_errno == EISDIR) {
> error(_("reference does not exist"));
> ret = 2;
> } else {
> diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
> index b50ae6fcf1..a8055f7fe1 100755
> --- a/t/t1403-show-ref.sh
> +++ b/t/t1403-show-ref.sh
> @@ -260,9 +260,9 @@ test_expect_success '--exists with non-commit object' '
>
> test_expect_success '--exists with directory fails with generic error' '
> cat >expect <<-EOF &&
> - error: failed to look up reference: Is a directory
> + error: reference does not exist
> EOF
> - test_expect_code 1 git show-ref --exists refs/heads 2>err &&
> + test_expect_code 2 git show-ref --exists refs/heads 2>err &&
> test_cmp expect err
> '
The rest looks all good to me.
--
Toon
^ permalink raw reply
* Re: [PATCH 1/3] ci: make p4 setup on macOS more robust
From: Patrick Steinhardt @ 2024-01-18 9:44 UTC (permalink / raw)
To: Matthias Aßhauer; +Cc: git
In-Reply-To: <DB9P250MB0692629640B05593B798E5A0A5712@DB9P250MB0692.EURP250.PROD.OUTLOOK.COM>
[-- Attachment #1: Type: text/plain, Size: 2286 bytes --]
On Thu, Jan 18, 2024 at 08:19:10AM +0100, Matthias Aßhauer wrote:
>
>
> On Mon, 15 Jan 2024, Patrick Steinhardt wrote:
>
> > When setting up Perforce on macOS we put both `p4` and `p4d` into
> > "$HOME/bin". On GitHub CI this directory is indeed contained in the PATH
> > environment variable and thus there is no need for additional setup than
> > to put the binaries there. But GitLab CI does not do this, and thus our
> > Perforce-based tests would be skipped there even though we download the
> > binaries.
> >
> > Refactor the setup code to become more robust by downloading binaries
> > into a separate directory which we then manually append to our PATH.
> > This matches what we do on Linux-based jobs.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > ci/install-dependencies.sh | 10 ++++------
> > ci/lib.sh | 3 +++
> > 2 files changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> > index 4f407530d3..b4e22de3cb 100755
> > --- a/ci/install-dependencies.sh
> > +++ b/ci/install-dependencies.sh
> > @@ -37,15 +37,13 @@ macos-*)
> > test -z "$BREW_INSTALL_PACKAGES" ||
> > brew install $BREW_INSTALL_PACKAGES
> > brew link --force gettext
> > - mkdir -p $HOME/bin
> > - (
> > - cd $HOME/bin
> > +
> > + mkdir -p "$P4_PATH"
> > + pushd "$P4_PATH"
> > wget -q "$P4WHENCE/bin.macosx1015x86_64/helix-core-server.tgz" &&
> > tar -xf helix-core-server.tgz &&
> > sudo xattr -d com.apple.quarantine p4 p4d 2>/dev/null || true
> > - )
> > - PATH="$PATH:${HOME}/bin"
> > - export PATH
>
> Didn't this put "$HOME/bin" on the PATH? And isn't the main premise of this
> patch that "$HOME/bin" is not on the PATH?
> or is the issue mainly about where we modify and export PATH and was masked
> by GitHub Actions already having "$HOME/bin" on the PATH?
Yes and no. While these lines put it in PATH, this only works inside of
"ci/install-dependencies.sh". When we call "ci/run-build-and-test.sh" we
do not source this script though, which means that "$HOME/bin" will not
be part of PATH during the actual test run unless it was already added
by the CI.
I'll update the commit message to explain this better.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v2 0/5] ci: add support for macOS to GitLab CI
From: Patrick Steinhardt @ 2024-01-18 10:22 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Matthias Aßhauer
In-Reply-To: <cover.1705318985.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 5172 bytes --]
Hi,
this is the second version of my patch series that adds a macOS job to
GitLab CI. Changes compared to v1:
- Added a fix for a flaky test in t7527 that caused the pipeline to
fail in ~50% of all runs.
- Improved some commit messages.
- Tests now write test data into a RAMDisk. This speeds up tests and
fixes some hung pipelines I was seeing.
Thanks for your reviews so far!
Patrick
Patrick Steinhardt (5):
t7527: decrease likelihood of racing with fsmonitor daemon
Makefile: detect new Homebrew location for ARM-based Macs
ci: handle TEST_OUTPUT_DIRECTORY when printing test failures
ci: make p4 setup on macOS more robust
ci: add macOS jobs to GitLab CI
.gitlab-ci.yml | 34 +++++++++++++++++++++++++++++++++-
ci/install-dependencies.sh | 10 ++++------
ci/lib.sh | 12 +++++++++++-
ci/print-test-failures.sh | 2 +-
config.mak.uname | 13 +++++++++++++
t/t7527-builtin-fsmonitor.sh | 2 +-
6 files changed, 63 insertions(+), 10 deletions(-)
Range-diff against v1:
-: ---------- > 1: 554b1c8546 t7527: decrease likelihood of racing with fsmonitor daemon
2: 3adb0b7ae8 = 2: 32d8bd1d78 Makefile: detect new Homebrew location for ARM-based Macs
-: ---------- > 3: d55da77747 ci: handle TEST_OUTPUT_DIRECTORY when printing test failures
1: a5d725bea7 ! 4: 1ed6e68650 ci: make p4 setup on macOS more robust
@@ Commit message
into a separate directory which we then manually append to our PATH.
This matches what we do on Linux-based jobs.
+ Note that it may seem like we already did append "$HOME/bin" to PATH
+ because we're actually removing the lines that adapt PATH. But we only
+ ever adapted the PATH variable in "ci/install-dependencies.sh", and
+ didn't adapt it when running "ci/run-build-and-test.sh". Consequently,
+ the required binaries wouldn't be found during the test run unless the
+ CI platform already had the "$HOME/bin" in PATH right from the start.
+
Signed-off-by: Patrick Steinhardt <ps@pks.im>
## ci/install-dependencies.sh ##
3: d196cfd9d0 ! 5: c5ed38f0a6 ci: add macOS jobs to GitLab CI
@@ Metadata
## Commit message ##
ci: add macOS jobs to GitLab CI
- Add two macOS-based jobs to GitLab CI, one for Clang and one for GCC.
- This matches equivalent jobs we have for GitHub Workflows, except that
- we use macOS 14 instead of macOS 13.
+ Add a job to GitLab CI which runs tests on macOS, which matches the
+ equivalent "osx-clang" job that we have for GitHub Workflows. One
+ significant difference though is that this new job runs on Apple M1
+ machines and thus uses the "arm64" architecture. As GCC does not yet
+ support this comparatively new architecture we cannot easily include an
+ equivalent for the "osx-gcc" job that exists in GitHub Workflows.
Note that one test marked as `test_must_fail` is surprisingly passing:
t7815-grep-binary.sh (Wstat: 0 Tests: 22 Failed: 0)
TODO passed: 12
- This seems to boil down to an unexpected difference in how regcomp(1)
+ This seems to boil down to an unexpected difference in how regcomp(3P)
works when matching NUL bytes. Cross-checking with the respective GitHub
- job shows though that this is not an issue unique to the GitLab CI job
- as it passes in the same way there.
-
- Further note that we do not include the equivalent for the "osx-gcc" job
- that we use with GitHub Workflows. This is because the runner for macOS
- on GitLab is running on Apple M1 machines and thus uses the "arm64"
- architecture. GCC does not support this platform yet.
+ job shows that this is not an issue unique to the GitLab CI job as it
+ passes in the same way there.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
@@ .gitlab-ci.yml: test:
+ image: $image
+ tags:
+ - saas-macos-medium-m1
++ variables:
++ TEST_OUTPUT_DIRECTORY: "/Volumes/RAMDisk"
+ before_script:
++ # Create a 4GB RAM disk that we use to store test output on. This small hack
++ # significantly speeds up tests by more than a factor of 2 because the
++ # macOS runners use network-attached storage as disks, which is _really_
++ # slow with the many small writes that our tests do.
++ - sudo diskutil apfs create $(hdiutil attach -nomount ram://8192000) RAMDisk
+ - ./ci/install-dependencies.sh
+ script:
+ - ./ci/run-build-and-tests.sh
@@ .gitlab-ci.yml: test:
+ if test "$CI_JOB_STATUS" != 'success'
+ then
+ ./ci/print-test-failures.sh
++ mv "$TEST_OUTPUT_DIRECTORY"/failed-test-artifacts t/
+ fi
+ parallel:
+ matrix:
base-commit: cd69c635a1a62b0c8bfdbf221778be8a512ad048
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v2 1/5] t7527: decrease likelihood of racing with fsmonitor daemon
From: Patrick Steinhardt @ 2024-01-18 10:22 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Matthias Aßhauer
In-Reply-To: <cover.1705573336.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1768 bytes --]
In t7527, we test that the builtin fsmonitor daemon works well in
various edge cases. One of these tests is frequently failing because
events reported by the fsmonitor--daemon are missing an expected event.
This failure is essentially a race condition: we do not wait for the
daemon to flush out all events before we ask it to quit. Consequently,
it can happen that we miss some expected events.
In other testcases we counteract this race by sending a simple query to
the daemon. Quoting a comment:
We run a simple query after modifying the filesystem just to introduce
a bit of a delay so that the trace logging from the daemon has time to
get flushed to disk.
Now this workaround is not a "proper" fix as we do not wait for all
events to have been synchronized in a deterministic way. But this fix
seems to be sufficient for all the other tests to pass, so it must not
be all that bad.
Convert the failing test to do the same. While the test was previously
failing in about 50% of the test runs, I couldn't reproduce the failure
after the change anymore.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t7527-builtin-fsmonitor.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
index 78503158fd..363f9dc0e4 100755
--- a/t/t7527-builtin-fsmonitor.sh
+++ b/t/t7527-builtin-fsmonitor.sh
@@ -978,7 +978,7 @@ test_expect_success !UNICODE_COMPOSITION_SENSITIVE 'Unicode nfc/nfd' '
mkdir test_unicode/nfd &&
mkdir test_unicode/nfd/d_${utf8_nfd} &&
- git -C test_unicode fsmonitor--daemon stop &&
+ test-tool -C test_unicode fsmonitor-client query --token 0 &&
if test_have_prereq UNICODE_NFC_PRESERVED
then
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 2/5] Makefile: detect new Homebrew location for ARM-based Macs
From: Patrick Steinhardt @ 2024-01-18 10:22 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Matthias Aßhauer
In-Reply-To: <cover.1705573336.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1625 bytes --]
With the introduction of the ARM-based Macs the default location for
Homebrew has changed from "/usr/local" to "/opt/homebrew". We only
handle the former location though, which means that unless the user has
manually configured required search paths we won't be able to locate it.
Improve upon this by adding relevant paths to our CFLAGS and LDFLAGS as
well as detecting the location of msgfmt(1).
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
config.mak.uname | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/config.mak.uname b/config.mak.uname
index 3bb03f423a..dacc95172d 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -158,6 +158,19 @@ ifeq ($(uname_S),Darwin)
ifeq ($(shell test -x /usr/local/opt/gettext/bin/msgfmt && echo y),y)
MSGFMT = /usr/local/opt/gettext/bin/msgfmt
endif
+ # On newer ARM-based machines the default installation path has changed to
+ # /opt/homebrew. Include it in our search paths so that the user does not
+ # have to configure this manually.
+ #
+ # Note that we do not employ the same workaround as above where we manually
+ # add gettext. The issue was fixed more than three years ago by now, and at
+ # that point there haven't been any ARM-based Macs yet.
+ else ifeq ($(shell test -d /opt/homebrew/ && echo y),y)
+ BASIC_CFLAGS += -I/opt/homebrew/include
+ BASIC_LDFLAGS += -L/opt/homebrew/lib
+ ifeq ($(shell test -x /opt/homebrew/bin/msgfmt && echo y),y)
+ MSGFMT = /opt/homebrew/bin/msgfmt
+ endif
endif
# The builtin FSMonitor on MacOS builds upon Simple-IPC. Both require
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 3/5] ci: handle TEST_OUTPUT_DIRECTORY when printing test failures
From: Patrick Steinhardt @ 2024-01-18 10:22 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Matthias Aßhauer
In-Reply-To: <cover.1705573336.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 989 bytes --]
The TEST_OUTPUT_DIRECTORY environment variable can be used to instruct
the test suite to write test data and test results into a different
location than into "t/". The "ci/print-test-failures.sh" script does not
know to handle this environment variable though, which means that it
will search for test results in the wrong location if it was set.
Update the script to handle TEST_OUTPUT_DIRECTORY so that we can start
to set it in our CI.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/print-test-failures.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ci/print-test-failures.sh b/ci/print-test-failures.sh
index c33ad4e3a2..b1f80aeac3 100755
--- a/ci/print-test-failures.sh
+++ b/ci/print-test-failures.sh
@@ -8,7 +8,7 @@
# Tracing executed commands would produce too much noise in the loop below.
set +x
-cd t/
+cd "${TEST_OUTPUT_DIRECTORY:-t/}"
if ! ls test-results/*.exit >/dev/null 2>/dev/null
then
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 4/5] ci: make p4 setup on macOS more robust
From: Patrick Steinhardt @ 2024-01-18 10:22 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Matthias Aßhauer
In-Reply-To: <cover.1705573336.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2153 bytes --]
When setting up Perforce on macOS we put both `p4` and `p4d` into
"$HOME/bin". On GitHub CI this directory is indeed contained in the PATH
environment variable and thus there is no need for additional setup than
to put the binaries there. But GitLab CI does not do this, and thus our
Perforce-based tests would be skipped there even though we download the
binaries.
Refactor the setup code to become more robust by downloading binaries
into a separate directory which we then manually append to our PATH.
This matches what we do on Linux-based jobs.
Note that it may seem like we already did append "$HOME/bin" to PATH
because we're actually removing the lines that adapt PATH. But we only
ever adapted the PATH variable in "ci/install-dependencies.sh", and
didn't adapt it when running "ci/run-build-and-test.sh". Consequently,
the required binaries wouldn't be found during the test run unless the
CI platform already had the "$HOME/bin" in PATH right from the start.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/install-dependencies.sh | 10 ++++------
ci/lib.sh | 3 +++
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 4f407530d3..b4e22de3cb 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -37,15 +37,13 @@ macos-*)
test -z "$BREW_INSTALL_PACKAGES" ||
brew install $BREW_INSTALL_PACKAGES
brew link --force gettext
- mkdir -p $HOME/bin
- (
- cd $HOME/bin
+
+ mkdir -p "$P4_PATH"
+ pushd "$P4_PATH"
wget -q "$P4WHENCE/bin.macosx1015x86_64/helix-core-server.tgz" &&
tar -xf helix-core-server.tgz &&
sudo xattr -d com.apple.quarantine p4 p4d 2>/dev/null || true
- )
- PATH="$PATH:${HOME}/bin"
- export PATH
+ popd
if test -n "$CC_PACKAGE"
then
diff --git a/ci/lib.sh b/ci/lib.sh
index c749b21366..f631206a44 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -344,6 +344,9 @@ macos-*)
then
MAKEFLAGS="$MAKEFLAGS APPLE_COMMON_CRYPTO_SHA1=Yes"
fi
+
+ P4_PATH="$HOME/custom/p4"
+ export PATH="$P4_PATH:$PATH"
;;
esac
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 5/5] ci: add macOS jobs to GitLab CI
From: Patrick Steinhardt @ 2024-01-18 10:23 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Matthias Aßhauer
In-Reply-To: <cover.1705573336.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3142 bytes --]
Add a job to GitLab CI which runs tests on macOS, which matches the
equivalent "osx-clang" job that we have for GitHub Workflows. One
significant difference though is that this new job runs on Apple M1
machines and thus uses the "arm64" architecture. As GCC does not yet
support this comparatively new architecture we cannot easily include an
equivalent for the "osx-gcc" job that exists in GitHub Workflows.
Note that one test marked as `test_must_fail` is surprisingly passing:
t7815-grep-binary.sh (Wstat: 0 Tests: 22 Failed: 0)
TODO passed: 12
This seems to boil down to an unexpected difference in how regcomp(3P)
works when matching NUL bytes. Cross-checking with the respective GitHub
job shows that this is not an issue unique to the GitLab CI job as it
passes in the same way there.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
.gitlab-ci.yml | 34 +++++++++++++++++++++++++++++++++-
ci/lib.sh | 9 ++++++++-
2 files changed, 41 insertions(+), 2 deletions(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 793243421c..43bfbd8834 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -7,7 +7,7 @@ workflow:
- if: $CI_COMMIT_TAG
- if: $CI_COMMIT_REF_PROTECTED == "true"
-test:
+test:linux:
image: $image
before_script:
- ./ci/install-docker-dependencies.sh
@@ -52,6 +52,38 @@ test:
- t/failed-test-artifacts
when: on_failure
+test:osx:
+ image: $image
+ tags:
+ - saas-macos-medium-m1
+ variables:
+ TEST_OUTPUT_DIRECTORY: "/Volumes/RAMDisk"
+ before_script:
+ # Create a 4GB RAM disk that we use to store test output on. This small hack
+ # significantly speeds up tests by more than a factor of 2 because the
+ # macOS runners use network-attached storage as disks, which is _really_
+ # slow with the many small writes that our tests do.
+ - sudo diskutil apfs create $(hdiutil attach -nomount ram://8192000) RAMDisk
+ - ./ci/install-dependencies.sh
+ script:
+ - ./ci/run-build-and-tests.sh
+ after_script:
+ - |
+ if test "$CI_JOB_STATUS" != 'success'
+ then
+ ./ci/print-test-failures.sh
+ mv "$TEST_OUTPUT_DIRECTORY"/failed-test-artifacts t/
+ fi
+ parallel:
+ matrix:
+ - jobname: osx-clang
+ image: macos-13-xcode-14
+ CC: clang
+ artifacts:
+ paths:
+ - t/failed-test-artifacts
+ when: on_failure
+
static-analysis:
image: ubuntu:22.04
variables:
diff --git a/ci/lib.sh b/ci/lib.sh
index f631206a44..d5dd2f2697 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -252,7 +252,14 @@ then
CI_COMMIT="$CI_COMMIT_SHA"
case "$CI_JOB_IMAGE" in
macos-*)
- CI_OS_NAME=osx;;
+ # GitLab CI has Python installed via multiple package managers,
+ # most notably via asdf and Homebrew. Ensure that our builds
+ # pick up the Homebrew one by prepending it to our PATH as the
+ # asdf one breaks tests.
+ export PATH="$(brew --prefix)/bin:$PATH"
+
+ CI_OS_NAME=osx
+ ;;
alpine:*|fedora:*|ubuntu:*)
CI_OS_NAME=linux;;
*)
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: git fsck does not check the packed-refs file
From: Patrick Steinhardt @ 2024-01-18 11:15 UTC (permalink / raw)
To: R. Diez; +Cc: git
In-Reply-To: <6cfee0e4-3285-4f18-91ff-d097da9de737@rd10.de>
[-- Attachment #1: Type: text/plain, Size: 3164 bytes --]
On Thu, Jan 18, 2024 at 09:02:30AM +0100, R. Diez wrote:
> Hi all:
>
> I have been hit by an unfortunate system problem, and as a result, a
> few files in my Git repository got corrupted on my last git push. Some
> random blocks of bytes were overwritten with binary zeros, so I
> started getting weird unpacking errors etc.
>
> It took a while to realise what the problem was. During my
> investigation, I ran "git fsck", which reported no problems, and then
> "git push" failed.
>
> One of the very few corrupted files was packed-refs. This is a text
> file, so it was easy to compare it and see the corrupting binary
> zeros. But that made me wonder what "git fsck" checks.
Can you maybe expand a bit on how you arrived at this bug? Was this a
hard crash of the system that corrupted the repository or rather
something like actual disk corruption?
I'm mostly asking because I have been fixing some sources of refdb
corruption:
- bc22d845c4 (core.fsync: new option to harden references, 2022-03-11)
started to fsync loose refs to disk before renaming them into place,
released with Git v2.36.
- ce54672f9b (refs: fix corruption by not correctly syncing
packed-refs to disk, 2022-12-20) started to sync packed-refs to disk
before renaming them into place, released with Git v2.40 and
backported to Git v2.39.3.
So if:
- you use a journaling filesystem,
- you didn't disable `core.fsync`,
- you use Git v2.40 or newer,
then you should in theory not run into any refdb corruption anymore. At
least we didn't experience corruption anymore at GitLab.com, whereas
before we encountered corruption every so often.
> I am guessing that "git fsck" does not check file packed-refs at all.
> I mean, it does not even attempt to parse it, in order to check
> whether at least the format makes any sense. Only "git push" does it.
Indeed it doesn't. While the issue is comparatively easy to spot by
manually inspecting the `packed-refs` file, I agree that it would be
great if git-fsck(1) knew how to check the refdb for consistency. This
problem is only going to get worse once the upcoming reftable backend
lands -- it is a binary format, and just opening it with a text editor
to check whether it looks sane-ish stops being a viable option here.
In fact, I already planned to introduce such consistency checks for the
refdb soonish. Once the reftable backend is upstream I will focus more
on additional tooling to support it, and extending our consistency
checks is one of the first items on my todo list here.
> What other parts of the repository does "git fsck" not check then?
There may be some metadata and cache-like data structures that we don't
check, but the object database is checked by default.
> The repository check is suspiciously fast. Is there a slow way to
> check that a repository is fine? I mean, something along the lines of
> checking whether every commit can be checked out without problems.
Other than running `git fsck --full --strict`: not that I'm aware of.
And `--full` isn't even needed because it's the default.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 02/12] remove REFFILES prerequisite
From: Patrick Steinhardt @ 2024-01-18 11:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Cai via GitGitGadget, git, John Cai
In-Reply-To: <xmqqedeftqn7.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 1524 bytes --]
On Wed, Jan 17, 2024 at 04:46:20PM -0800, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: John Cai <johncai86@gmail.com>
> >
> > These tests are compatible with the reftable backend and thus do not
> > need the REFFILES prerequisite.
>
> May want to give a bit more backstory here? After all, 53af25e4
> (t1405: mark test that checks existence as REFFILES, 2022-01-31) and
> 53af25e4 (t1405: mark test that checks existence as REFFILES,
> 2022-01-31) marked these tests to require REFFILES and they explain
> the reason for doing so was exactly because the reftable backend did
> not have the notion of "the reflog for this ref exists" that is
> independent from "the reflog for this ref exists and has one or more
> reflog records". If your work on the reftable backend during the
> past few years added support for "already exists, but there is no
> entry yet" state for reflogs, that would be great, but it would make
> sense to explain why they suddenly have become "compatible with the
> reftable backend".
I don't know a lot about the history any why we initially didn't think
it would be compatible, mostly because there is no history of how the
reftable backend itself evolved over time. I can only say that when I
took over the effort that this indeed worked as expected by writing
"existence" markers into the reflog, where this existence marker is a
simple entry where both old and new object ID are set to the null OID.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 01/12] t3210: move to t0602
From: Patrick Steinhardt @ 2024-01-18 11:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Cai via GitGitGadget, git, John Cai
In-Reply-To: <xmqqil3rtqxh.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 1970 bytes --]
On Wed, Jan 17, 2024 at 04:40:10PM -0800, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: John Cai <johncai86@gmail.com>
> >
> > Move t3210 to t0602, since these tests are reffiles specific in that
> > they modify loose refs manually. This is part of the effort to
> > categorize these tests together based on the ref backend they test. When
> > we upstream the reftable backend, we can add more tests to t06xx. This
> > way, all tests that test specific ref backend behavior will be grouped
> > together.
>
> So, ... is the idea to have (1) majority of ref tests, against which
> all backends ought to behave the same way, will be written in
> backend agnostic way (e.g., we have seen some patches to stop
> touching the filesystem .git/refs/ hierarchy manually), and (2) some
> backend specific tests will be grouped in a small number of test
> script files for each backend and they all will use t6xx numbrs?
>
> OK. Sounds like a good plan to me.
Yes, that's the plan. The backend specific tests will be free to also
exercise filesystem-level behaviour in order to pin down that things
work as expected. But once their behaviour is nailed down all other
generic tests should refrain from doing that to the best extent possible
and instead use Git commands to do their thing.
> > Signed-off-by: John Cai <johncai86@gmail.com>
> > ---
> > t/{t3210-pack-refs.sh => t0602-reffiles-pack-refs.sh} | 0
> > 1 file changed, 0 insertions(+), 0 deletions(-)
> > rename t/{t3210-pack-refs.sh => t0602-reffiles-pack-refs.sh} (100%)
Is there a reason why you picked t0602 instead of the not-yet-taken
t0601? If it's only because I use t0601 in my reftable integration
branch then I'd like us to pick t0601 here instead to avoid a weird gap.
I'll adapt accordingly and rename the reftable tests to have a t061x
prefix in that case so that they are nicely grouped together.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 00/12] Group reffiles tests
From: Patrick Steinhardt @ 2024-01-18 11:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Cai via GitGitGadget, git, John Cai
In-Reply-To: <xmqqv87rsan6.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 1938 bytes --]
On Wed, Jan 17, 2024 at 05:17:17PM -0800, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > This series groups REFFILES specific tests together. These tests are
> > currently grouped together across the test suite based on functionality.
> > However, since they exercise low-level behavior specific to the refs backend
> > being used (in these cases, the ref-files backend), group them together
> > based on which refs backend they test. This way, in the near future when the
> > reftables backend gets upstreamed we can add tests that exercise the
> > reftables backend close by in the t06xx area.
> >
> > These patches also remove the REFFILES prerequisite, since all the tests in
> > t06xx are reffiles specific.
>
> As we already have REFFILES lazy prereq, even _before_ we enable the
> reftable backend, I think that we should start t0600 and t0602 with
>
> . ./test-lib.sh
> if ! test_have_prereq REFFILES
> then
> skip_all='skipping reffiles specific tests'
> test_done
> fi
>
> which is more in line with the existing convention. It is more
> efficient than "forcing t0600 and t0602 to run always with reffiles"
> when you have a CI job that uses reftable for all tests and another
> CI job that uses reffiles for all tests.
I think it depends. If we use the REFFILES prereq for the files-specific
tests, then we should likely also use the REFTABLE prereq for the
reftable-specific tests.
But that raises the question of whether we want to add a CI job that
exercises code with the reftable backend for every major platform
(Linux, macOS, Windows). If so then your proposal would be fine with me
as we make sure that things work alright on all of them. But if we think
that this would be too expensive then I'd like to at least have very
basic test coverage on all platforms by always running these
backend-specific tests.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* How to execute a command on git am/rebase/cherry pick --abort ?
From: Konstantin Kharlamov @ 2024-01-18 12:53 UTC (permalink / raw)
To: git
(please keep me CC'ed, I'm not subscribed)
Hello!
There's a well-known problem of git not fully checking out changes
while doing e.g. `git checkout` and similar commands when you have
submodules. So e.g. if HEAD changes a submodule commit and you do an
interactive rebase to HEAD~2, you may be lucky to find a submodule
commit change in `git diff` (because if you don't get lucky, you won't
notice that and commit the change to the unrelated HEAD~2).
As a workaround I have a `git submodule update` inside `post-checkout`
hook.
Now, the problem is I still often finding myself having the wrong
submodule ID, and I tracked down that problem to commands such as
`am/rebase/cherry-pick --abort` also not updating the submodule, nor
executing `post-checkout`.
I looked through `man githooks` but couldn't find any way to execute a
`git submodule update` during these aborts.
Any ideas how to fix these?
^ 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