* Re: [Outreachy][PATCH v4] Port helper/test-ctype.c to unit-tests/t-ctype.c
From: Junio C Hamano @ 2024-01-09 17:12 UTC (permalink / raw)
To: Phillip Wood
Cc: Achu Luma, git, chriscool, christian.couder, l.s.r, phillip.wood,
steadmon, me
In-Reply-To: <33c81610-0958-49da-b702-ba8d96ecf1d3@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> In the new version we only test the characters 0-255, not EOF. If
> there is a good reason for removing the EOF tests then it should be
> explained in the commit message. If not it would be good to add those
> tests back.
Thanks for sharp eyes. Didn't notice the handling of EOF myself.
^ permalink raw reply
* Re: Analyzing a corrupted index file
From: Junio C Hamano @ 2024-01-09 17:16 UTC (permalink / raw)
To: Nathan Manceaux-Panot; +Cc: git
In-Reply-To: <29D15110-E308-475F-A722-1CDD448CACDA@lemon.garden>
Nathan Manceaux-Panot <fresh.tree3651@lemon.garden> writes:
>> On Jan 8, 2024, at 20:51, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> "git ls-files" with its various options is probably the closest, but
>> even the command is not meant for "debugging the bits"
>
> Thanks for the pointer. In the meantime I've started to make sense
> of the binary representation, against all odds, so there's hope
> for me there!
Documentation/gitformat-index.txt may be a good place to start, of
course.
https://github.com/git/htmldocs/blob/gh-pages/gitformat-index.txt
^ permalink raw reply
* Re: [PATCH 2/2] t7501: Add test for amending commit to add signoff.
From: Ghanshyam Thakkar @ 2024-01-09 17:24 UTC (permalink / raw)
To: phillip.wood, git; +Cc: Christian Couder
In-Reply-To: <2113e178-149b-49aa-9d78-ff1c480f754c@gmail.com>
On Tue Jan 9, 2024 at 4:14 PM IST, Phillip Wood wrote:
> Hi Ghanshyam
>
> On 09/01/2024 06:04, Ghanshyam Thakkar wrote:
> > This commit adds test for amending the latest commit to add
> > Signed-off-by trailer at the end of commit message.
>
> If we're not already testing this then it seems like a useful addition,
> thanks for working on it. It would also be helpful to check that "git
> commit --amend --signoff" does not add a Signed-off-by: trailer if it
> already exists.
I hadn't thought of that. I have hastily sent the v2 without seeing this
comment. I will add this test in v3.
>
> > +test_expect_success 'amend commit to add signoff' '
> > +
> > + test_when_finished "rm -rf testdir" &&
> > + git init testdir &&
>
> As Christian said about the other patch in this series I don't think we
> need a new repository here. In our test files we use the same repository
> for the whole file unless there is a compelling reason not to.
Updated from v2 onwards.
>
> > + echo content >testdir/file &&
> > + git -C testdir add file &&
> > + git -C testdir commit -m "file" &&
>
> I think these three lines can be replaced by
>
> test_commit --no-tag file file content
Thank you for the suggestion. I have updated the test to use test_commit.
>
> > + git -C testdir commit --amend --signoff &&
>
> > + git -C testdir log -1 --pretty=format:%B >actual &&
> > + (
> > + echo file &&
> > + echo &&
> > + git -C testdir var GIT_COMMITTER_IDENT >ident &&
> > + sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /" ident
> > + ) >expected &&
> > + test_cmp expected actual
>
> This section of the test can be improved by using test_commit_message
>
> test_commit_message HEAD <<-EOF
> file
>
> Signed-off-by: $GIT_COMMITER_NAME <$GIT_COMMITTER_EMAIL>
> EOF
I have updated the test to use above approach.
Thank you for the review!
^ permalink raw reply
* Re: [PATCH 1/2] t7501: Add tests for various index usages, -i and -o, of commit command.
From: Junio C Hamano @ 2024-01-09 17:35 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: git
In-Reply-To: <20240109060417.1144647-3-shyamthakkar001@gmail.com>
Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
> Subject: Re: [PATCH 1/2] t7501: Add tests for various index usages, -i and -o, of commit command.
Overly long subject that has an unusual capitalization after
"t7501:" (see "git log --no-merges --format=%s -20 v2.43.0" for
example and try to write something that blends better).
> +test_expect_success 'commit with -i fails with untracked files' '
> + test_when_finished "rm -rf testdir" &&
> + git init testdir &&
> + echo content >testdir/file.txt &&
> + test_must_fail git -C testdir commit -i file.txt -m initial
> +'
In addition to "why a new repository???" comment raised already, I
do not want to see the last command spelled like so. Always write
dashed options (and their parameters) before non-option arguments,
i.e.
git commit -i -m initial file.txt
git -C testdir commit -i -m initial file.txt
test_must_fail git -C testdir commit -i -m initial file.txt
The command line parser does rotate the unrecognized arguments to
the end and keeps looking for recognisable option (possibly followed
by its parameter), but that is purely to help lazy writers (i.e.,
interactive command users). When writers know "-i" does not take
any parameter, it may be convenient if the writer who forgot to say
"-m" can just append "-m initial" to what has already be written.
When writing source (be it the production code or test), however, we
write for readers. What you wrote at a first glance, especially
given that "-i" (or "-o" for that matter) is a relatively less
commonly used option, would confuse less experienced readers by
making them wonder what "-i file.txt" means (e.g., "is that taking
input from the contents of file.txt?").
^ permalink raw reply
* Re: [PATCH 2/2] t7501: Add test for amending commit to add signoff.
From: Junio C Hamano @ 2024-01-09 17:45 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: git
In-Reply-To: <20240109060417.1144647-4-shyamthakkar001@gmail.com>
Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
> Subject: Re: [PATCH 2/2] t7501: Add test for amending commit to add signoff.
The title is with unusual capitalization and final full-stop (again,
check "git log --no-merges --format=%s -20 v2.43.0" and try to blend
in).
> This commit adds test for amending the latest commit to add
> Signed-off-by trailer at the end of commit message.
"This commit adds ..." -> "Add ..."
Also what the patch does can be read from the patch text below, but
it cannot be read _why_ the patch author thought it was a good idea
to make such a change. The proposed commit log message is a place
to describe the reason behind the patch. Why do we want a new test?
Why do we want that new test in this particular file? etc.
> +test_expect_success 'amend commit to add signoff' '
> +
> + test_when_finished "rm -rf testdir" &&
> + git init testdir &&
The same "why a new repository for just this test???" applies here.
> + echo content >testdir/file &&
> + git -C testdir add file &&
> + git -C testdir commit -m "file" &&
> + git -C testdir commit --amend --signoff &&
> + git -C testdir log -1 --pretty=format:%B >actual &&
If you are doing many things in a separate directory, the usual
pattern is
# create a directory DIR (usuall "mkdir", not "git init")
mkdir DIR &&
(
cd DIR &&
git do this &&
git do that &&
inspect the result of this >actual &&
prepare the expected outcome >expect &&
test_cmp expect actual
) &&
Thanks.
^ permalink raw reply
* Re: [PATCH v2 1/2] t7501: add tests for --include, --only of commit
From: Junio C Hamano @ 2024-01-09 17:50 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: git, christian.couder, phillip.wood123
In-Reply-To: <20240109165304.8027-4-shyamthakkar001@gmail.com>
Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
> This commit adds tests for testing --include (-o) and --only (-o). It
"-i" not "-o" stands for "--include".
Please write in imperative mood.
> +test_expect_success '--include fails with untracked files' '
> + echo content >baz &&
> + test_must_fail git commit --include baz -m initial
> +'
My comment on argument order applies to this iteration, too. Please
write your code to help readers.
> +test_expect_success 'commit --include' '
> + test_when_finished "rm -rf remaining" &&
Why recursive removal when you _know_ what you create is a plan
file?
^ permalink raw reply
* [PATCH 0/3] Strengthen fsck checks for submodule URLs
From: Victoria Dye via GitGitGadget @ 2024-01-09 17:53 UTC (permalink / raw)
To: git; +Cc: Victoria Dye
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 adds a 'check-url' mode to 'test-tool submodule', calling the
now-public 'check_submodule_url()' method on a given URL, and adds a new
test checking a list of valid and invalid submodule URLs.
* Patch 3 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)).
Thanks!
* Victoria
Victoria Dye (3):
submodule-config.h: move check_submodule_url
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 | 31 ++++++--
t/t7450-bad-git-dotfiles.sh | 26 +++++++
5 files changed, 196 insertions(+), 137 deletions(-)
base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1635%2Fvdye%2Fvdye%2Fstrengthen-fsck-url-checks-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1635/vdye/vdye/strengthen-fsck-url-checks-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1635
--
gitgitgadget
^ permalink raw reply
* [PATCH 1/3] submodule-config.h: move check_submodule_url
From: Victoria Dye via GitGitGadget @ 2024-01-09 17:53 UTC (permalink / raw)
To: git; +Cc: Victoria Dye, Victoria Dye
In-Reply-To: <pull.1635.git.1704822817.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 @@ done:
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 @@ in_component:
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 2/3] t7450: test submodule urls
From: Victoria Dye via GitGitGadget @ 2024-01-09 17:53 UTC (permalink / raw)
To: git; +Cc: Victoria Dye, Victoria Dye
In-Reply-To: <pull.1635.git.1704822817.gitgitgadget@gmail.com>
From: Victoria Dye <vdye@github.com>
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'.
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.
Signed-off-by: Victoria Dye <vdye@github.com>
---
t/helper/test-submodule.c | 31 +++++++++++++++++++++++++++----
t/t7450-bad-git-dotfiles.sh | 26 ++++++++++++++++++++++++++
2 files changed, 53 insertions(+), 4 deletions(-)
diff --git a/t/helper/test-submodule.c b/t/helper/test-submodule.c
index 50c154d0370..da89d265f0f 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 <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[] = {
@@ -36,22 +43,24 @@ static const char *submodule_usage[] = {
NULL
};
+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)
{
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);
@@ -69,7 +78,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(argc, argv);
+ return check_submodule(argc, argv, 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(argc, argv, check_submodule_url);
}
static int cmd__submodule_is_active(int argc, const char **argv)
@@ -195,6 +217,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..0dbf13724f4 100755
--- a/t/t7450-bad-git-dotfiles.sh
+++ b/t/t7450-bad-git-dotfiles.sh
@@ -45,6 +45,32 @@ test_expect_success 'check names' '
test_cmp expect actual
'
+test_expect_failure '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
+ http://example.com:test/foo.git
+ https::example.com/foo.git
+ http:::example.com/foo.git
+ EOF
+
+ test_cmp expect actual
+'
+
test_expect_success 'create innocent subrepo' '
git init innocent &&
git -C innocent commit --allow-empty -m foo
--
gitgitgadget
^ permalink raw reply related
* [PATCH 3/3] submodule-config.c: strengthen URL fsck check
From: Victoria Dye via GitGitGadget @ 2024-01-09 17:53 UTC (permalink / raw)
To: git; +Cc: Victoria Dye, Victoria Dye
In-Reply-To: <pull.1635.git.1704822817.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 | 2 +-
2 files changed, 12 insertions(+), 6 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 0dbf13724f4..46d4fb0354b 100755
--- a/t/t7450-bad-git-dotfiles.sh
+++ b/t/t7450-bad-git-dotfiles.sh
@@ -45,7 +45,7 @@ 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
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 1/3] t/test-tool: usage description
From: Junio C Hamano @ 2024-01-09 18:19 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List
In-Reply-To: <ec57072d-0069-4d07-a695-b89436350568@gmail.com>
> Subject: Re: [PATCH 1/3] t/test-tool: usage description
Good eyes to spot the missing close-angle-bracket. I'll add some
missing verb, e.g. "fix usage string", while queuing.
I would not bother replacing the fprintf() format string in the same
patch. Hits from
$ git grep '"usage:' t/helper
indicates that far less than half (3 among 12) reuses usage_str[]
for this purpose. Making these "usage:" strings come from a unified
API (perhaps parse_options() family of functions have something more
appropriate than ad-hoc use of fprintf()? I didn't check) might be
a welcome change but that is clearly outside the scope of the
mark-up fix, and I do not see touching only this one that still uses
fprintf() advances toward such a goal.
t/helper/test-chmtime.c: fprintf(stderr, "usage: %s %s\n", argv[0], usage_str);
t/helper/test-delta.c: fprintf(stderr, "usage: %s\n", usage_str);
t/helper/test-windows-named-pipe.c: fprintf(stderr, "usage: %s %s\n", argv[0], usage_string);
t/helper/test-advise.c: die("usage: %s <advice>", argv[0]);
t/helper/test-csprng.c: fprintf(stderr, "usage: %s [<size>]\n", argv[0]);
t/helper/test-genrandom.c: fprintf(stderr, "usage: %s <seed_string> [<size>]\n", argv[0]);
t/helper/test-genzeros.c: fprintf(stderr, "usage: %s [<count>]\n", argv[0]);
t/helper/test-hash-speed.c: die("usage: test-tool hash-speed algo_name");
t/helper/test-mergesort.c: fprintf(stderr, "usage: test-tool mergesort generate <distribution> <mode> <n> <m>\n");
t/helper/test-strcmp-offset.c: die("usage: %s <string1> <string2>", argv[0]);
t/helper/test-tool.c: fprintf(stderr, "usage: test-tool <toolname> [args]\n");
> Even though this is an internal tool, let's keep the usage description
> correct and well organized.
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
> t/helper/test-tool.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
> index 37ba996539..d9f57c20db 100644
> --- a/t/helper/test-tool.c
> +++ b/t/helper/test-tool.c
> @@ -5,7 +5,7 @@
> #include "parse-options.h"
>
> static const char * const test_tool_usage[] = {
> - "test-tool [-C <directory>] <command [<arguments>...]]",
> + "test-tool [-C <directory>] <command> [<arguments>...]]",
> NULL
> };
>
> @@ -100,7 +100,7 @@ static NORETURN void die_usage(void)
> {
> size_t i;
>
> - fprintf(stderr, "usage: test-tool <toolname> [args]\n");
> + fprintf(stderr, "usage: %s\n", test_tool_usage[0]);
> for (i = 0; i < ARRAY_SIZE(cmds); i++)
> fprintf(stderr, " %s\n", cmds[i].name);
> exit(128);
^ permalink raw reply
* Re: [PATCH 2/3] t/test-tool: handle -c <name>=<value> arguments
From: Junio C Hamano @ 2024-01-09 18:19 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List
In-Reply-To: <6a4d6a56-ab6f-4557-a5a3-1713f57cbfc9@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
> Soon we're going to need to pass configuration values to a command in
> test-tool.
>
> Let's teach test-tool to take config values via command line arguments.
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
> t/helper/test-tool.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
Nice.
>
> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
> index d9f57c20db..7eba4ec9ab 100644
> --- a/t/helper/test-tool.c
> +++ b/t/helper/test-tool.c
> @@ -3,9 +3,10 @@
> #include "test-tool-utils.h"
> #include "trace2.h"
> #include "parse-options.h"
> +#include "config.h"
>
> static const char * const test_tool_usage[] = {
> - "test-tool [-C <directory>] <command> [<arguments>...]]",
> + "test-tool [-C <directory>] [-c <name>=<value>] <command> [<arguments>...]",
> NULL
> };
>
> @@ -106,6 +107,13 @@ static NORETURN void die_usage(void)
> exit(128);
> }
>
> +static int parse_config_option(const struct option *opt, const char *arg,
> + int unset)
> +{
> + git_config_push_parameter(arg);
> + return 0;
> +}
> +
> int cmd_main(int argc, const char **argv)
> {
> int i;
> @@ -113,6 +121,9 @@ int cmd_main(int argc, const char **argv)
> struct option options[] = {
> OPT_STRING('C', NULL, &working_directory, "directory",
> "change the working directory"),
> + OPT_CALLBACK('c', NULL, NULL, "<name>=<value>",
> + "pass a configuration parameter to the command",
> + parse_config_option),
> OPT_END()
> };
^ permalink raw reply
* Re: [PATCH 2/3] t/test-tool: handle -c <name>=<value> arguments
From: Taylor Blau @ 2024-01-09 18:20 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List
In-Reply-To: <6a4d6a56-ab6f-4557-a5a3-1713f57cbfc9@gmail.com>
On Tue, Jan 09, 2024 at 04:29:57PM +0100, Rubén Justo wrote:
> Soon we're going to need to pass configuration values to a command in
> test-tool.
>
> Let's teach test-tool to take config values via command line arguments.
I wasn't expecting a step like this to appear in this series. I don't
have strong feelings about it, especially since test-tool helpers
already understand $GIT_DIR/config when they rely on library code which
implicitly reads configuration.
But it does seem odd to have test-tool invocations that intimately
depend on a particular set of configuration values. At the very least,
this step seems to encourage passing finely tuned configuration values
to test-tool helpers, which I am not sure is a good idea.
Your patch message suggests that this will be useful in the following
patch, which makes sense. But I wonder if it would be easier to avoid
the test-tool entirely and call some Git command in a state that we
expect to generate advice. Then we can test its output with various
values of advice.adviceOff.
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
> t/helper/test-tool.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
The patch itself looks reasonable, though.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()
From: Junio C Hamano @ 2024-01-09 18:23 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List
In-Reply-To: <d6099d78-43c6-4709-9121-11f84228cf91@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
> Using advise_if_enabled() to display an advice will automatically
> include instructions on how to disable the advice, along with the
> main advice:
>
> hint: use --reapply-cherry-picks to include skipped commits
> hint: Disable this message with "git config advice.skippedCherryPicks false"
>
> This can become distracting or noisy over time, while the user may
> still want to receive the main advice.
>
> Let's have a switch to allow disabling this automatic advice.
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
> advice.c | 3 ++-
> advice.h | 3 ++-
> t/t0018-advice.sh | 8 ++++++++
> 3 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/advice.c b/advice.c
> index 50c79443ba..fa203f8806 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -79,6 +79,7 @@ static struct {
> [ADVICE_UPDATE_SPARSE_PATH] = { "updateSparsePath", 1 },
> [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor", 1 },
> [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan", 1 },
> + [ADVICE_ADVICE_OFF] = { "adviceOff", 1 },
> };
>
> static const char turn_off_instructions[] =
> @@ -93,7 +94,7 @@ static void vadvise(const char *advice, int display_instructions,
>
> strbuf_vaddf(&buf, advice, params);
>
> - if (display_instructions)
> + if (display_instructions && advice_enabled(ADVICE_ADVICE_OFF))
> strbuf_addf(&buf, turn_off_instructions, key);
>
> for (cp = buf.buf; *cp; cp = np) {
> diff --git a/advice.h b/advice.h
> index 2affbe1426..1f2eef034e 100644
> --- a/advice.h
> +++ b/advice.h
> @@ -10,7 +10,7 @@ struct string_list;
> * Add the new config variable to Documentation/config/advice.txt.
> * Call advise_if_enabled to print your advice.
> */
> - enum advice_type {
> +enum advice_type {
> ADVICE_ADD_EMBEDDED_REPO,
> ADVICE_ADD_EMPTY_PATHSPEC,
> ADVICE_ADD_IGNORED_FILE,
> @@ -50,6 +50,7 @@ struct string_list;
> ADVICE_WAITING_FOR_EDITOR,
> ADVICE_SKIPPED_CHERRY_PICKS,
> ADVICE_WORKTREE_ADD_ORPHAN,
> + ADVICE_ADVICE_OFF,
> };
>
> int git_default_advice_config(const char *var, const char *value);
> diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
> index c13057a4ca..0b6a8b4a10 100755
> --- a/t/t0018-advice.sh
> +++ b/t/t0018-advice.sh
> @@ -30,4 +30,12 @@ test_expect_success 'advice should not be printed when config variable is set to
> test_must_be_empty actual
> '
>
> +test_expect_success 'advice without the instructions to disable it' '
> + cat >expect <<-\EOF &&
> + hint: This is a piece of advice
> + EOF
> + test-tool -c advice.adviceOff=0 advise "This is a piece of advice" 2>actual &&
> + test_cmp expect actual
> +'
This is testing the right thing but in a "showing off a shiny new
toy" way. We want to make sure we will catch regressions in the
future by testing with a bit more conditions perturbed. For
example, with the new "-c var=val" mechanism, we could
* set advice.nestedtag to off (which would disable the whole
advice)
* set advice.adviceoff to on (which should be the same as not
setting it explicitly at all).
to test different combinations that we were unable to test before
[2/3] invented the mechanism.
Thanks.
^ permalink raw reply
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()
From: Taylor Blau @ 2024-01-09 18:27 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List
In-Reply-To: <d6099d78-43c6-4709-9121-11f84228cf91@gmail.com>
On Tue, Jan 09, 2024 at 04:30:16PM +0100, Rubén Justo wrote:
> Using advise_if_enabled() to display an advice will automatically
> include instructions on how to disable the advice, along with the
> main advice:
>
> hint: use --reapply-cherry-picks to include skipped commits
> hint: Disable this message with "git config advice.skippedCherryPicks false"
>
> This can become distracting or noisy over time, while the user may
> still want to receive the main advice.
Presumably for more trivial pieces of advice, a user may want to
immediately disable those hints in the future more quickly after first
receiving the advice, in which case this feature may not be as useful
for them.
But for trickier pieces of advice, I agree completely with your
reasoning and think that something like this makes sense.
> Let's have a switch to allow disabling this automatic advice.
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
> advice.c | 3 ++-
> advice.h | 3 ++-
> t/t0018-advice.sh | 8 ++++++++
> 3 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/advice.c b/advice.c
> index 50c79443ba..fa203f8806 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -79,6 +79,7 @@ static struct {
> [ADVICE_UPDATE_SPARSE_PATH] = { "updateSparsePath", 1 },
> [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor", 1 },
> [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan", 1 },
> + [ADVICE_ADVICE_OFF] = { "adviceOff", 1 },
The name seems to imply that setting `advice.adviceOff=true` would cause
Git to suppress the turn-off instructions. But...
> };
>
> static const char turn_off_instructions[] =
> @@ -93,7 +94,7 @@ static void vadvise(const char *advice, int display_instructions,
>
> strbuf_vaddf(&buf, advice, params);
>
> - if (display_instructions)
> + if (display_instructions && advice_enabled(ADVICE_ADVICE_OFF))
> strbuf_addf(&buf, turn_off_instructions, key);
...it looks like the opposite is true. I guess the "adviceOff" part of
this new configuration option suggests "show me advice on how to turn
off advice of xyz kind in the future".
Perhaps a clearer alternative might be "advice.showDisableInstructions"
or something? I don't know, coming up with a direct/clear name of this
configuration is challenging for me.
>
> for (cp = buf.buf; *cp; cp = np) {
> diff --git a/advice.h b/advice.h
> index 2affbe1426..1f2eef034e 100644
> --- a/advice.h
> +++ b/advice.h
> @@ -10,7 +10,7 @@ struct string_list;
> * Add the new config variable to Documentation/config/advice.txt.
> * Call advise_if_enabled to print your advice.
> */
> - enum advice_type {
> +enum advice_type {
> ADVICE_ADD_EMBEDDED_REPO,
> ADVICE_ADD_EMPTY_PATHSPEC,
> ADVICE_ADD_IGNORED_FILE,
> @@ -50,6 +50,7 @@ struct string_list;
> ADVICE_WAITING_FOR_EDITOR,
> ADVICE_SKIPPED_CHERRY_PICKS,
> ADVICE_WORKTREE_ADD_ORPHAN,
> + ADVICE_ADVICE_OFF,
> };
>
> int git_default_advice_config(const char *var, const char *value);
> diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
> index c13057a4ca..0b6a8b4a10 100755
> --- a/t/t0018-advice.sh
> +++ b/t/t0018-advice.sh
> @@ -30,4 +30,12 @@ test_expect_success 'advice should not be printed when config variable is set to
> test_must_be_empty actual
> '
>
> +test_expect_success 'advice without the instructions to disable it' '
> + cat >expect <<-\EOF &&
> + hint: This is a piece of advice
> + EOF
> + test-tool -c advice.adviceOff=0 advise "This is a piece of advice" 2>actual &&
> + test_cmp expect actual
> +'
Looking at this test, I wonder why we don't imitate the existing style
of:
test_config advice.adviceOff false &&
test-tool advise "This is a piece of advice" 2>actual &&
test_cmp expect actual
instead of teaching the test-tool helpers how to interpret `-c`
arguments. Doing so would allow us to drop the first couple of patches
in this series and simplify things a bit.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 0/3] allow disabling the automatic hint in advise_if_enabled()
From: Taylor Blau @ 2024-01-09 18:28 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List
In-Reply-To: <7c68392c-af2f-4999-ae64-63221bf7833a@gmail.com>
On Tue, Jan 09, 2024 at 04:25:32PM +0100, Rubén Justo wrote:
> Using advise_if_enabled() to display an advice will automatically
> include instructions on how to disable the advice, along with the main
> advice:
>
> hint: use --reapply-cherry-picks to include skipped commits
> hint: Disable this message with "git config advice.skippedCherryPicks false"
>
> This may become distracting or "noisy" over time, while the user may
> still want to receive the main advice.
>
> Let's have a switch to allow disabling this automatic advice.
I reviewed this and had a couple of notes, mostly focused on what to
call the new configuration option, and if we should be modifying the
test-tool helpers to accept arbitrary configuration via command-line
options.
I think that we could reasonably drop the first two patches by
imitating the existing style of t0018 more closely, but I don't feel all
that strongly about it.
So I would probably expect a reroll of this series, but if you disagree
with my comments, I would not be sad to see this series merged as-is.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 1/6] t1300: mark tests to require default repo format
From: Taylor Blau @ 2024-01-09 18:41 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <ec1b5bdd176e6a3f093b76b732fd9e960a7880ca.1704802213.git.ps@pks.im>
On Tue, Jan 09, 2024 at 01:17:04PM +0100, Patrick Steinhardt wrote:
> The t1300 test suite exercises the git-config(1) tool. To do so we
> overwrite ".git/config" to contain custom contents. While this is easy
> enough to do, it may create problems when using a non-default repository
> format because we also overwrite the repository format version as well
> as any potential extensions.
>
> Mark these tests with the DEFAULT_REPO_FORMAT prerequisite to avoid the
> problem. An alternative would be to carry over mandatory config keys
> into the rewritten config file. But the effort does not seem worth it
> given that the system under test is git-config(1), which is at a lower
> level than the repository format.
I think I am missing something obvious here ;-).
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> t/t1300-config.sh | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index f4e2752134..1e953a0fc2 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -1098,7 +1098,7 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' '
> test_must_fail git config --file=linktolinktonada --list
> '
>
> -test_expect_success 'check split_cmdline return' "
> +test_expect_success DEFAULT_REPO_FORMAT 'check split_cmdline return' "
> git config alias.split-cmdline-fix 'echo \"' &&
> test_must_fail git split-cmdline-fix &&
> echo foo > foo &&
> @@ -1156,7 +1156,7 @@ test_expect_success 'git -c works with aliases of builtins' '
> test_cmp expect actual
> '
Looking at this first test, for example, I see two places where we
modify the configuration file:
- git config alias.split-cmdline-fix 'echo \"'
- git config branch.main.mergeoptions 'echo \"'
I think I am missing some detail about why we can't do this when we have
extensions enabled?
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 3/6] t1302: make tests more robust with new extensions
From: Taylor Blau @ 2024-01-09 18:43 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <9af1e418d47730f503dabb271d30c848bf74fa0b.1704802213.git.ps@pks.im>
On Tue, Jan 09, 2024 at 01:17:12PM +0100, Patrick Steinhardt wrote:
> In t1302 we exercise logic around "core.repositoryFormatVersion" and
> extensions. These tests are not particularly robust against extensions
> like the newly introduced "refStorage" extension.
>
> Refactor the tests to be more robust:
>
> - Check the DEFAULT_REPO_FORMAT prereq to determine the expected
> repository format version. This helps to ensure that we only need to
> update the prereq in a central place when new extensions are added.
>
> - Use a separate repository to rewrite ".git/config" to test
> combinations of the repository format version and extensions. This
> ensures that we don't break the main test repository.
>
> - Do not rewrite ".git/config" when exercising the "preciousObjects"
> extension.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> t/t1302-repo-version.sh | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
> index 179474fa65..fb30c87e1b 100755
> --- a/t/t1302-repo-version.sh
> +++ b/t/t1302-repo-version.sh
> @@ -28,7 +28,12 @@ test_expect_success 'setup' '
> '
>
> test_expect_success 'gitdir selection on normal repos' '
> - test_oid version >expect &&
> + if test_have_prereq DEFAULT_REPO_FORMAT
> + then
> + echo 0
> + else
> + echo 1
> + fi >expect &&
> git config core.repositoryformatversion >actual &&
> git -C test config core.repositoryformatversion >actual2 &&
> test_cmp expect actual &&
> @@ -79,8 +84,13 @@ mkconfig () {
>
> while read outcome version extensions; do
> test_expect_success "$outcome version=$version $extensions" "
> - mkconfig $version $extensions >.git/config &&
> - check_${outcome}
> + test_when_finished 'rm -rf extensions' &&
> + git init extensions &&
> + (
> + cd extensions &&
> + mkconfig $version $extensions >.git/config &&
> + check_${outcome}
> + )
> "
> done <<\EOF
> allow 0
> @@ -94,7 +104,8 @@ allow 1 noop-v1
> EOF
>
> test_expect_success 'precious-objects allowed' '
> - mkconfig 1 preciousObjects >.git/config &&
> + git config core.repositoryformatversion 1 &&
I'm nit-picking, but it looks like core.repositoryformatversion is all
lower-case, whereas extensions.preciousObjects is camel-case. I don't
think it's a big deal either way, but I couldn't *not* mention it while
reading ;-)
> + git config extensions.preciousObjects 1 &&
> check_allow
> '
>
> --
> 2.43.GIT
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 5/6] t5526: break test submodule differently
From: Eric Sunshine @ 2024-01-09 19:23 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <51e494a50e4416ed0cbfd3c474ffcaf8b72e6ef4.1704802213.git.ps@pks.im>
On Tue, Jan 9, 2024 at 7:17 AM Patrick Steinhardt <ps@pks.im> wrote:
> In 10f5c52656 (submodule: avoid auto-discovery in
> prepare_submodule_repo_env(), 2016-09-01) we fixed a bug when doing a
> recursive fetch with submodule in the case where the submodule is broken
> due to whatever reason. The test to exercise that the fix works breaks
> the submodule by deleting its `HEAD` reference, which will cause us to
> not detect the directory as a Git repository.
>
> While this is perfectly fine in theory, this way of breaking the repo
> becomes problematic with the current efforts to introduce another refdb
> backend into Git. The new reftable backend has a stub HEAD file that
> always contains "ref: refs/heads/.invalid" so that tools continue to be
> able to detect such a repository. But as the reftable backend will never
> delete this file even when asked to delete `HEAD` the current way to
> delete the `HEAD` reference will stop working.
This patch is not the appropriate place to bikeshed (but since this is
the first time I've read or paid attention to it), if I saw "ref:
refs/heads/.invalid", the word ".invalid" would make me think
something was broken in my repository. Would it make sense to use some
less alarming word, such as perhaps ".placeholder", ".stand-in",
".synthesized" or even the name of the non-file-based backend in use?
> Adapt the code to instead delete the objects database. Going back with
> this new way to cuase breakage confirms that it triggers the infinite
> recursion just the same, and there are no equivalent ongoing efforts to
> replace the object database with an alternate backend.
s/cuase/cause/
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> @@ -771,7 +771,7 @@ test_expect_success 'fetching submodule into a broken repository' '
> # Break the receiving submodule
> - test-tool -C dst/sub ref-store main delete-refs REF_NO_DEREF msg HEAD &&
> + rm -r dst/sub/.git/objects &&
If there is no guarantee that .git/objects will exist when any
particular backend is in use, would it be more robust to use -f here,
as well?
rm -rf dst/sub/.git/objects &&
^ permalink raw reply
* Re: [PATCH v7 1/1] mingw: give more details about unsafe directory's ownership
From: Johannes Sixt @ 2024-01-09 19:27 UTC (permalink / raw)
To: Sören Krecker; +Cc: sunshine, git
In-Reply-To: <20240108173837.20480-2-soekkle@freenet.de>
Am 08.01.24 um 18:38 schrieb Sören Krecker:
> +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
> +{
> + SID_NAME_USE pe_use;
> + DWORD len_user = 0, len_domain = 0;
> + BOOL translate_sid_to_user;
> +
> + /*
> + * returns only FALSE, because the string pointers are NULL
> + */
> + LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
> + &pe_use);
At this point, the function fails, so len_user and len_domain contain
the required buffer size (including the trailing NUL).
> + /*
> + * Alloc needed space of the strings
> + */
> + ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user);
> + translate_sid_to_user = LookupAccountSidA(NULL, sid,
> + (*str) + len_domain, &len_user, *str, &len_domain, &pe_use);
At this point, if the function is successful, len_user and len_domain
contain the lengths of the names (without the trailing NUL).
> + if (!translate_sid_to_user)
> + FREE_AND_NULL(*str);
> + else
> + (*str)[len_domain] = '/';
Therefore, this overwrites the NUL after the domain name and so
concatenates the two names. Good.
I found this by dumping the values of the variables, because the
documentation of LookupAccountSid is not clear about the values that the
variables receive in the success case.
> + return translate_sid_to_user;
> +}
> +
This patch looks good and works for me.
Acked-by: Johannes Sixt <j6t@kdbg.org>
Thank you!
-- Hannes
^ permalink raw reply
* Re: [PATCH 1/6] t1300: mark tests to require default repo format
From: Eric Sunshine @ 2024-01-09 19:35 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <ec1b5bdd176e6a3f093b76b732fd9e960a7880ca.1704802213.git.ps@pks.im>
On Tue, Jan 9, 2024 at 7:17 AM Patrick Steinhardt <ps@pks.im> wrote:
> The t1300 test suite exercises the git-config(1) tool. To do so we
> overwrite ".git/config" to contain custom contents. While this is easy
> enough to do, it may create problems when using a non-default repository
> format because we also overwrite the repository format version as well
> as any potential extensions.
>
> Mark these tests with the DEFAULT_REPO_FORMAT prerequisite to avoid the
> problem. An alternative would be to carry over mandatory config keys
> into the rewritten config file. But the effort does not seem worth it
> given that the system under test is git-config(1), which is at a lower
> level than the repository format.
If I'm understanding correctly, with the approach taken by this patch,
won't we undesirably lose some git-config test coverage if the
file-based backend is ever retired, or if tests specific to it are
ever disabled by default? As such, it seems like the alternative "fix"
you mention above would be preferable to ensure that coverage of
git-config doesn't get diluted.
Or am I misunderstanding something?
^ permalink raw reply
* Re: [PATCH 4/6] t1419: mark test suite as files-backend specific
From: Eric Sunshine @ 2024-01-09 19:40 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <d7c6b8b2a7b3b4d776f06ce577bdbdbaff66f225.1704802213.git.ps@pks.im>
On Tue, Jan 9, 2024 at 7:17 AM Patrick Steinhardt <ps@pks.im> wrote:
> With 59c35fac54 (refs/packed-backend.c: implement jump lists to avoid
> excluded pattern(s), 2023-07-10) we have implemented logic to handle
> excluded refs more efficiently in the "packed" ref backend. This logic
> allows us to skip emitting refs completely which we know to not be of
> any interest to the caller, which can avoid quite some allocaitons and
> object lookups.
s/allocaitons/allocations/
> This was wired up via a new `exclude_patterns` parameter passed to the
> backend's ref iterator. The backend only needs to handle them on a best
> effort basis though, and in fact we only handle it for the "packed-refs"
> file, but not for loose references. Consequentially, all callers must
> still filter emitted refs with those exclude patterns.
s/Consequentially/Consequently/
> The result is that handling exclude patterns is completely optional in
> the ref backend, and any future backends may or may not implement it.
> Let's thus mark the test for t1419 to depend on the REFFILES prereq.
This change seems to be abusing the meaning of the REFFILES
prerequisite. Instead the above description argues for introduction of
a new prerequisite which indicates whether or not the backend honors
the exclude patterns. Or, am I misunderstanding this?
^ permalink raw reply
* Add more builtin patterns for userdiff, as Microproject.
From: Sergius Nyah @ 2024-01-09 19:54 UTC (permalink / raw)
To: git, gitster@pobox.com; +Cc: newren, l.s.r
Hello everyone,
I'm Sergius, a Computer Science undergraduate student, and I want to
begin Contributing to the Git project. So far, I've gone through
Matheus' tutorial on First steps Contributing to Git, and I found it
very helpful. I've also read the Contribution guidelines keenly and
built Git from source.
In accordance with the contributor guidelines, I came across this
Mircoproject idea from: https://git.github.io/SoC-2022-Microprojects/
which I'm willing to work on. It talked about enhancing Git's
"userdiff" feature in "userdiff.c" which is crucial for identifying
function names in various programming languages, thereby improving the
readability of "git diff" outputs.
From my understanding, the project involves extending the `userdiff`
feature to support additional programming languages that are currently
not covered such as Shell, Swift, Go and the others.
Here is a sample of how a language is defined in `userdiff.c`:
> #define PATTERNS(lang, rx, wrx) { \
> .name = lang, \
> .binary = -1, \
> .funcname = { \
> .pattern = rx, \
> .cflags = REG_EXTENDED, \
> }, \
> .word_regex = wrx "|[^[:space:]]|[\xc0-\xff][\x80-\xbf]+", \
> .word_regex_multi_byte = wrx "|[^[:space:]]", \
> }
In this code, `lang` is the name of the language, `rx` is the regular
expression for identifying function names, and `wrx` is the word
regex.
Approach: I Identified the Programming Languages that are not
currently supported by the userdiff feature by reviewing the existing
patterns in userdiff.c and comparing them with some popular
programming languages.
For each supported language, I would define a regular expression that
could help identify function names in that language. This could
include researching each language's syntax and testing their
expressions to ensure that they work well.
Also, I'd add a new IPATTERN definition for each language to the
"userdiff.c" file, then rebuild Git and test the changes by creating a
repo with files in the newly supported languages then run "git diff"
to ensure the line @@ ... @@ produces their correct function names.
Then submit a patch.
Best Regards!
Sergius.
^ permalink raw reply
* [GSOC][RFC] Add more builtin patterns for userdiff, as Mircroproject.
From: Sergius Nyah @ 2024-01-09 19:55 UTC (permalink / raw)
To: git, gitster@pobox.com
Hello everyone,
I'm Sergius, a Computer Science undergraduate student, and I want to
begin Contributing to the Git project. So far, I've gone through
Matheus' tutorial on First steps Contributing to Git, and I found it
very helpful. I've also read the Contribution guidelines keenly and
built Git from source.
In accordance to the contributor guidelines, I came across this
Mircoproject idea from: https://git.github.io/SoC-2022-Microprojects/
which I'm willing to work on. It talked about enhancing Git's
"userdiff" feature in "userdiff.c" which is crucial for identifying
function names in various programming languages, thereby improving the
readability of "git diff" outputs.
From my understanding, the project involves extending the `userdiff`
feature to support additional programming languages that are currently
not covered such as Shell, Swift, Go and the others.
Here is a sample of how a language is defined in `userdiff.c`:
> #define PATTERNS(lang, rx, wrx) { \
> .name = lang, \
> .binary = -1, \
> .funcname = { \
> .pattern = rx, \
> .cflags = REG_EXTENDED, \
> }, \
> .word_regex = wrx "|[^[:space:]]|[\xc0-\xff][\x80-\xbf]+", \
> .word_regex_multi_byte = wrx "|[^[:space:]]", \
> }
In this code, `lang` is the name of the language, `rx` is the regular
expression for identifying function names, and `wrx` is the word
regex.
Approach: I Identified the Programming Languages that are not
currently supported by the userdiff feature by reviewing the existing
patterns in userdiff.c and comparing them with some popular
programming languages.
For each supported language, I would define a regular expression that
could help identify function names in that language. This could
include researching each language's syntax and testing their
expressions to ensure that they work well.
Also, I'd add a new IPATTERN definition for each language to the
"userdiff.c" file, then rebuild Git and test the changes by creating a
repo with files in the newly supported languages then run "git diff"
to ensure the line @@ ... @@ produces their correct function names.
Then submit a patch.
Best Regards!
Sergius.
^ permalink raw reply
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()
From: Junio C Hamano @ 2024-01-09 19:57 UTC (permalink / raw)
To: Taylor Blau; +Cc: Rubén Justo, Git List
In-Reply-To: <ZZ2QGYBBmK8cSYBD@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
> Looking at this test, I wonder why we don't imitate the existing style
> of:
>
> test_config advice.adviceOff false &&
> test-tool advise "This is a piece of advice" 2>actual &&
> test_cmp expect actual
>
> instead of teaching the test-tool helpers how to interpret `-c`
> arguments. Doing so would allow us to drop the first couple of patches
> in this series and simplify things a bit.
Thanks for a dose of sanity. I too got a bit too excited by a shiny
new toy ;-) Reusing the existing mechanism does make sense.
^ 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