* [PATCH 6/6] t: mark tests regarding git-pack-refs(1) to be backend specific
From: Patrick Steinhardt @ 2024-01-09 12:17 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1704802213.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1577 bytes --]
Both t1409 and t3210 exercise parts of git-pack-refs(1). Given that we
must check the on-disk files to verify whether the backend has indeed
packed refs as expected those test suites are deeply tied to the actual
backend that is in use.
Mark the test suites to depend on the REFFILES backend.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1409-avoid-packing-refs.sh | 6 ++++++
t/t3210-pack-refs.sh | 6 ++++++
2 files changed, 12 insertions(+)
diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh
index f23c0152a8..7748973733 100755
--- a/t/t1409-avoid-packing-refs.sh
+++ b/t/t1409-avoid-packing-refs.sh
@@ -5,6 +5,12 @@ test_description='avoid rewriting packed-refs unnecessarily'
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
+if test_have_prereq !REFFILES
+then
+ skip_all='skipping files-backend specific pack-refs tests'
+ test_done
+fi
+
# Add an identifying mark to the packed-refs file header line. This
# shouldn't upset readers, and it should be omitted if the file is
# ever rewritten.
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index 7f4e98db7d..c0f1f9cfb7 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -15,6 +15,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
+if test_have_prereq !REFFILES
+then
+ skip_all='skipping files-backend specific pack-refs tests'
+ test_done
+fi
+
test_expect_success 'enable reflogs' '
git config core.logallrefupdates true
'
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Credential error - Cache credential helper does not work properly
From: Lopez Sanchez, Jose Maria @ 2024-01-09 13:21 UTC (permalink / raw)
To: git@vger.kernel.org
Dear Mr. / Ms.
I am José María López Sánchez a senior system engineer working for the EDAG Group company. We are willing to use GIT to start controlling our project versions.
I have an issue when I want to stop inserting repeatedly my credentials. The cache function is not working somehow.
I have typed the following:
> C:\Users\jl83870\Desktop\00_temp\GIT EAGEL\eagel-project>git config credential.helper 'cache
Then I introduced once my credentials to store them in cache. If the command worked, the second time that I attempted to pull the remote server, I should not have typed the credentials again to be able to trigger the command.
> C:\Users\jl83870\Desktop\00_temp\GIT EAGEL\eagel-project>git pull origin
> git credential-'cache get: -c: line 0: unexpected EOF while looking for matching `''
> git credential-'cache get: -c: line 1: syntax error: unexpected end of file
> Username for 'https://csp.edag.de': jl83870
> Password for 'https://jl83870@csp.edag.de':
> git credential-'cache store: -c: line 0: unexpected EOF while looking for matching `''
> git credential-'cache store: -c: line 1: syntax error: unexpected end of file
> Already up to date.
As I mentioned above, when I want to pull again, appears the same previous code.
I have also tried with the below options:
- git config credential.helper cache (without ‘ )
- git config credential helper cache --timeout=1800
- git config credential.helper ‘cache --timeout=1800’
- git config credential.helper ‘cache [--timeout=1800]’
Am I programming something wrong? Could you please help me with it?
Mit freundlichen Grüßen / Best regards / Saludos cordiales
i.A. José María López Sánchez
Senior Systemingenieur Antriebsstrang / Senior Systems engineer Drivetrain / Ingeniero senior de sistemas de trenes de transmisión
Fahrzeug-Integration / Vehicle Integration / Integración de vehículos
EDAG Engineering Spain, S.L I Reesbergstraße 1 I 36039 Fulda I Germany
Phone: +49 661 6000-83870
E-Mail: jose.maria.lopez.sanchez@edag.com I www.edag.com
YOUR GLOBAL MOBILITY AND
INDUSTRY ENGINEERING EXPERTS
Find us online: LinkedIn I XING I Facebook I Instagram I Twitter I YouTube I TechInsights
EDAG Engineering Spain, S.L, Carretera del Prat, 65, E-08940 Cornellá de Llobregat (Barcelona), Spanien
El court register: Registro Mercantil de Barcelona, Tomo 41564, Folio 15, Hoja 92217
Please consider the environment before printing this e-mail.
This e-mail may contain confidential and / or privileged information. If you are not the intended recipient (or have received this e-mail in error) please notify the sender immediately and destroy this e-mail. Any unauthorised copying, disclosure or distribution of the material in this e-mail is strictly forbidden.
________________________________
Die Pflichtinformationen nach Art. 12 ff. DSGVO erhalten Sie hier<https://www.edag.com/de/rechtliches/datenschutz-teams-e-mail>. / Mandatory information pursuant to Article 12 et seq. GDPR can be found here<https://www.edag.com/en/legal/data-privacy-teams-e-mail>.
^ permalink raw reply
* Re: Analyzing a corrupted index file
From: Nathan Manceaux-Panot @ 2024-01-09 14:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqh6jnk3i8.fsf@gitster.g>
> 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!
^ permalink raw reply
* [PATCH 0/3] allow disabling the automatic hint in advise_if_enabled()
From: Rubén Justo @ 2024-01-09 15:25 UTC (permalink / raw)
To: Git List
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.
Rubén Justo (3):
t/test-tool: usage description
t/test-tool: handle -c <name>=<value> arguments
advice: allow disabling the automatic hint in advise_if_enabled()
advice.c | 3 ++-
advice.h | 3 ++-
t/helper/test-tool.c | 15 +++++++++++++--
t/t0018-advice.sh | 8 ++++++++
4 files changed, 25 insertions(+), 4 deletions(-)
--
2.42.0
^ permalink raw reply
* [PATCH 1/3] t/test-tool: usage description
From: Rubén Justo @ 2024-01-09 15:29 UTC (permalink / raw)
To: Git List
In-Reply-To: <7c68392c-af2f-4999-ae64-63221bf7833a@gmail.com>
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);
--
2.42.0
^ permalink raw reply related
* [PATCH 2/3] t/test-tool: handle -c <name>=<value> arguments
From: Rubén Justo @ 2024-01-09 15:29 UTC (permalink / raw)
To: Git List
In-Reply-To: <7c68392c-af2f-4999-ae64-63221bf7833a@gmail.com>
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(-)
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()
};
--
2.42.0
^ permalink raw reply related
* [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()
From: Rubén Justo @ 2024-01-09 15:30 UTC (permalink / raw)
To: Git List
In-Reply-To: <7c68392c-af2f-4999-ae64-63221bf7833a@gmail.com>
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
+'
+
test_done
--
2.42.0
^ permalink raw reply related
* Bug: Git spawns subprocesses on windows
From: Domen Golob @ 2024-01-09 15:33 UTC (permalink / raw)
To: git
Hello,
the problem is exactly the same as what was reported here on stackoverflow:
c# - Git for windows seems to create sub-processes that never die -
Stack Overflow
https://stackoverflow.com/questions/69579065/git-for-windows-seems-to-create-sub-processes-that-never-die
Additionally I have found out that:
- a Git for Windows subprocess is created for each repository and
every time a git command is issued this process grows in size and it
never dies.
- you cannot delete the .git folder from the terminal, but it works
via file explorer
- deleting the .git folder kills the Git for windows process
- creating changes in several repos creates multiple processes, and
sometimes the process is created as a subprocess
Here are some screenshots:
https://photos.app.goo.gl/oinHVY1NAzHChU6CA
Hope this helps,
Thanks for making git awesome :)
Best regards,
Domen
^ permalink raw reply
* Re: Credential error - Cache credential helper does not work properly
From: Matthias Aßhauer @ 2024-01-09 16:12 UTC (permalink / raw)
To: Lopez Sanchez, Jose Maria; +Cc: git@vger.kernel.org
In-Reply-To: <FRYP281MB329114171822EE43AF9C8C0DFD6A2@FRYP281MB3291.DEUP281.PROD.OUTLOOK.COM>
On Tue, 9 Jan 2024, Lopez Sanchez, Jose Maria wrote:
> Dear Mr. / Ms.
>
> I am José María López Sánchez a senior system engineer working for the EDAG Group company. We are willing to use GIT to start controlling our project versions.
> I have an issue when I want to stop inserting repeatedly my credentials. The cache function is not working somehow.
> I have typed the following:
>
>> C:\Users\jl83870\Desktop\00_temp\GIT EAGEL\eagel-project>git config credential.helper 'cache
Looks like you're running this on Windows, so you're probably using Git
for Windows, where this helper will error out with the message "fatal:
credential-cache unavailable; no unix socket support", even when
otherwise configured correctly [1]. If you compiled your own git (2.34.0
or newer) without NO_UNIX_SOCKETS this helper should work on Windows 10
build 17063 and newer [2][3]. You seem to be running this in CMD
or maybe powershell. Different quoting rules apply for the two, but the
leading "'" before the "cache" is definitely wrong without a trailing
counterpart.
[1] https://github.com/git-for-windows/git/issues/3892
[2] https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/
[3] https://github.com/git/git/commit/bb390b1f49406d967e8bf3401337cf1d9535f820
>
> Then I introduced once my credentials to store them in cache. If the command worked, the second time that I attempted to pull the remote server, I should not have typed the credentials again to be able to trigger the command.
>
>> C:\Users\jl83870\Desktop\00_temp\GIT EAGEL\eagel-project>git pull origin
>> git credential-'cache get: -c: line 0: unexpected EOF while looking for matching `''
>> git credential-'cache get: -c: line 1: syntax error: unexpected end of file
>> Username for 'https://csp.edag.de': jl83870
>> Password for 'https://jl83870@csp.edag.de':
>> git credential-'cache store: -c: line 0: unexpected EOF while looking for matching `''
>> git credential-'cache store: -c: line 1: syntax error: unexpected end of file
>> Already up to date.
The superfluous "'" made it into the config file and is causing these
error messages.
> As I mentioned above, when I want to pull again, appears the same previous code.
>
> I have also tried with the below options:
> - git config credential.helper cache (without ‘ )
> - git config credential helper cache --timeout=1800
> - git config credential.helper ‘cache --timeout=1800’
> - git config credential.helper ‘cache [--timeout=1800]’
I assume the "‘" and "‘" are supposed to be "'" and your MUA mangled them
a little. Otherwise that's definitely part of the problem.
If you're on CMD you want either
> git config credential.helper cache
or
> git config credential.helper "cache --timeout=1800"
If you're on powershell
> git config credential.helper 'cache --timeout=1800'
should also work.
Do note that the Git documentation assumes a POSIX shell when it gives
examples of commands.
Best regards
Matthias
> Am I programming something wrong? Could you please help me with it?
>
> Mit freundlichen Grüßen / Best regards / Saludos cordiales
> i.A. José María López Sánchez
> Senior Systemingenieur Antriebsstrang / Senior Systems engineer Drivetrain / Ingeniero senior de sistemas de trenes de transmisión
> Fahrzeug-Integration / Vehicle Integration / Integración de vehículos
>
> EDAG Engineering Spain, S.L I Reesbergstraße 1 I 36039 Fulda I Germany
> Phone: +49 661 6000-83870
> E-Mail: jose.maria.lopez.sanchez@edag.com I www.edag.com
>
> YOUR GLOBAL MOBILITY AND
> INDUSTRY ENGINEERING EXPERTS
> Find us online: LinkedIn I XING I Facebook I Instagram I Twitter I YouTube I TechInsights
>
> EDAG Engineering Spain, S.L, Carretera del Prat, 65, E-08940 Cornellá de Llobregat (Barcelona), Spanien
> El court register: Registro Mercantil de Barcelona, Tomo 41564, Folio 15, Hoja 92217
>
> Please consider the environment before printing this e-mail.
>
> This e-mail may contain confidential and / or privileged information. If you are not the intended recipient (or have received this e-mail in error) please notify the sender immediately and destroy this e-mail. Any unauthorised copying, disclosure or distribution of the material in this e-mail is strictly forbidden.
>
> ________________________________
> Die Pflichtinformationen nach Art. 12 ff. DSGVO erhalten Sie hier<https://www.edag.com/de/rechtliches/datenschutz-teams-e-mail>. / Mandatory information pursuant to Article 12 et seq. GDPR can be found here<https://www.edag.com/en/legal/data-privacy-teams-e-mail>.
>
^ permalink raw reply
* [PATCH v2 0/2] t7501: add tests for --include, --only,
From: Ghanshyam Thakkar @ 2024-01-09 16:51 UTC (permalink / raw)
To: git; +Cc: christian.couder, phillip.wood123, Ghanshyam Thakkar
In-Reply-To: <20240109060417.1144647-2-shyamthakkar001@gmail.com>
This patch series adds tests for --include, --only of commit and test
for amending the commit with --signoff. And it also addresses the
reviews of Christian Couder and Phillip Wood.
The v2 of this patch removes unnecessary initialization of git repos
and reuses existing files instead of creating new files. Also, it
removes some redundant code, namely committing everytime for tracking new
files, instead of just staging the files.
The second patch of this series refactors the test with a better
approach as suggested by Phillip Wood.
Ghanshyam Thakkar (2):
t7501: add tests for --include, --only of commit
t7501: add test for --amend with --signoff
t/t7501-commit-basic-functionality.sh | 68 ++++++++++++++++++++++++++-
1 file changed, 66 insertions(+), 2 deletions(-)
--
2.43.0
^ permalink raw reply
* [PATCH v2 1/2] t7501: add tests for --include, --only of commit
From: Ghanshyam Thakkar @ 2024-01-09 16:51 UTC (permalink / raw)
To: git; +Cc: christian.couder, phillip.wood123, Ghanshyam Thakkar
In-Reply-To: <20240109060417.1144647-2-shyamthakkar001@gmail.com>
This commit adds tests for testing --include (-o) and --only (-o). It
include testing --include with and without staged changes, testing
--only with and without staged changes and testing --only and --include
together.
Some tests already exist in t7501 for testing --only, however, they
are only tested in combination with --amend --allow-empty and to-be-born
branch, and not for testing --only separately.
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
A single test also exists in t1092-sparse-checkout-compatibility.sh
named "commit including unstaged changes" for --include, however, that
compares the results of sparse-checkout with full-checkout when using
--include and is not necessarily for testing --include itself. Otherthan
that, I could not find any other test for --include.
t/t7501-commit-basic-functionality.sh | 55 ++++++++++++++++++++++++++-
1 file changed, 54 insertions(+), 1 deletion(-)
diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
index 3d8500a52e..9325db1f66 100755
--- a/t/t7501-commit-basic-functionality.sh
+++ b/t/t7501-commit-basic-functionality.sh
@@ -3,7 +3,7 @@
# Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
#
-# FIXME: Test the various index usages, -i and -o, test reflog,
+# FIXME: Test the various index usages, test reflog,
# signoff
test_description='git commit'
@@ -150,6 +150,59 @@ test_expect_success 'setup: commit message from file' '
git commit -F msg -a
'
+test_expect_success '--include fails with untracked files' '
+ echo content >baz &&
+ test_must_fail git commit --include baz -m initial
+'
+
+test_expect_success 'commit --include' '
+ test_when_finished "rm -rf remaining" &&
+ echo content >file &&
+ git commit --include file -m "file" &&
+ git diff --name-only >remaining &&
+ test_must_be_empty remaining
+'
+
+test_expect_success '--include with staged changes' '
+ echo newcontent >baz &&
+ echo newcontent >file &&
+ git add file &&
+ git commit --include baz -m "file baz" &&
+
+ git diff --name-only >remaining &&
+ test_must_be_empty remaining &&
+ git diff --name-only --staged >remaining &&
+ test_must_be_empty remaining
+'
+
+test_expect_success 'commit --only' '
+ echo change >file &&
+ git commit --only file -m "file" &&
+ git diff --name-only >actual &&
+ test_must_be_empty actual
+'
+
+test_expect_success '--only fails with untracked files' '
+ echo content >newfile &&
+ test_must_fail git commit --only newfile -m "newfile"
+'
+
+test_expect_success '--only excludes staged changes' '
+ echo content >file &&
+ echo content >baz &&
+ git add baz &&
+ git commit --only file -m "file" &&
+ git diff --name-only --staged >actual &&
+ test_grep "baz" actual
+'
+
+test_expect_success '--include and --only together fails' '
+ test_when_finished "git reset --hard" &&
+ echo new >file &&
+ echo new >baz &&
+ test_must_fail git commit --include baz --only bar -m "bar"
+'
+
test_expect_success 'amend commit' '
cat >editor <<-\EOF &&
#!/bin/sh
--
2.43.0
^ permalink raw reply related
* [PATCH v2 2/2] t7501: add test for --amend with --signoff
From: Ghanshyam Thakkar @ 2024-01-09 16:51 UTC (permalink / raw)
To: git; +Cc: christian.couder, phillip.wood123, Ghanshyam Thakkar
In-Reply-To: <20240109060417.1144647-2-shyamthakkar001@gmail.com>
This commit adds test for amending commit to add Signed-off-by
trailer.
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
I believe there are many existing tests which already cover almost
all of the scenarios. So in addition to this test, I have also
updated the FIXME comment to remove signoff.
t/t7501-commit-basic-functionality.sh | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
index 9325db1f66..376a7d59cc 100755
--- a/t/t7501-commit-basic-functionality.sh
+++ b/t/t7501-commit-basic-functionality.sh
@@ -3,8 +3,7 @@
# Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
#
-# FIXME: Test the various index usages, test reflog,
-# signoff
+# FIXME: Test the various index usages, test reflog
test_description='git commit'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
@@ -442,6 +441,18 @@ test_expect_success 'amend commit to fix date' '
'
+test_expect_success 'amend commit to add signoff' '
+
+ test_commit file file content &&
+ git commit --amend --signoff &&
+ test_commit_message HEAD <<-EOF
+ file
+
+ Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+ EOF
+
+'
+
test_expect_success 'commit mentions forced date in output' '
git commit --amend --date=2010-01-02T03:04:05 >output &&
grep "Date: *Sat Jan 2 03:04:05 2010" output
--
2.43.0
^ permalink raw reply related
* Re: [PATCH 1/2] t7501: Add tests for various index usages, -i and -o, of commit command.
From: Ghanshyam Thakkar @ 2024-01-09 17:10 UTC (permalink / raw)
To: Christian Couder; +Cc: git
In-Reply-To: <CAP8UFD0GJf5+eOTxy6s+zCzpDmCU_FY4BjwtjTE7RvZ5mKettA@mail.gmail.com>
On Tue Jan 9, 2024 at 2:50 PM IST, Christian Couder wrote:
> First about the commit subject:
>
> "t7501: Add tests for various index usages, -i and -o, of commit command."
>
> it should be shorter, shouldn't end with a "." and "Add" should be "add".
Updated in v2.
> On Tue, Jan 9, 2024 at 7:10 AM Ghanshyam Thakkar
> <shyamthakkar001@gmail.com> wrote:
> >
> > This commit adds tests for -i and -o flags of the commit command. It
> > includes testing -i with and without staged changes, testing -o with and
> > without staged changes, and testing -i and -o together.
>
> A few suggestions to make things a bit more clear:
>
> - tell that -i is a synonymous for --include and -o for --only
> - tell if there are already tests for these options
> - tell why the tests you add are worth it if tests for an option already exist
I have updated the commit messages in v2 to address these points.
> > Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> > ---
> > t/t7501-commit-basic-functionality.sh | 90 +++++++++++++++++++++++++++
> > 1 file changed, 90 insertions(+)
> >
> > diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
> > index 3d8500a52e..71dc52ce3a 100755
> > --- a/t/t7501-commit-basic-functionality.sh
> > +++ b/t/t7501-commit-basic-functionality.sh
> > @@ -76,6 +76,96 @@ test_expect_success 'nothing to commit' '
> > test_must_fail git commit -m initial
> > '
> >
> > +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
> > +'
>
> Existing tests didn't need a repo, so I am not sure it's worth
> creating a testdir repo just for this test.
Yes, I just wanted to make sure the files were not tracked. However, I
have updated these instaces to use the existing repo and use unique
non-generic names for generating untracked files.
> Also I am not sure this is the best place in the test script to add -i
> and -o related tests. Here we are between a 'nothing to commit' test
> and a '--dry-run fails with nothing to commit' and then more 'nothing
> to commit' related tests. I think it would be better if all those
> 'nothing to commit' related tests could stay together.
I have moved these tests above the "--amend" related tests, which do not
break the flow of the tests.
> > +test_expect_success 'commit with -i' '
> > + echo content >bar &&
> > + git add bar &&
> > + git commit -m "bar" &&
>
> Why is the above needed for this test?
This was to make sure that the file is tracked, however, I realised that
committing is not necessary, so I have updated this test to use existing
files in repo and to not generate a new one.
>
> > + echo content2 >bar &&
> > + git commit -i bar -m "bar second"
> > +'
> > +
> > +test_expect_success 'commit with -i multiple files' '
> > + test_when_finished "git reset --hard" &&
> > + echo content >bar &&
> > + echo content >baz &&
> > + echo content >saz &&
> > + git add bar baz saz &&
> > + git commit -m "bar baz saz" &&
>
> Not sure why the above is needed here too.
Similar to above, I have updated this test to use existing files in repo
and to not generate a new one.
>
> > + echo content2 >bar &&
> > + echo content2 >baz &&
> > + echo content2 >saz &&
> > + git commit -i bar saz -m "bar" &&
> > + git diff --name-only >remaining &&
> > + test_grep "baz" remaining
> > +'
> > +
> > +test_expect_success 'commit with -i includes staged changes' '
> > + test_when_finished "git reset --hard" &&
> > + echo content >bar &&
> > + git add bar &&
> > + git commit -m "bar" &&
> > +
> > + echo content2 >bar &&
> > + echo content2 >baz &&
> > + git add baz &&
> > + git commit -i bar -m "bar baz" &&
> > + git diff --name-only >remaining &&
> > + test_must_be_empty remaining &&
> > + git diff --name-only --staged >remaining &&
> > + test_must_be_empty remaining
> > +'
> > +
> > +test_expect_success 'commit with -o' '
> > + echo content >bar &&
> > + git add bar &&
> > + git commit -m "bar" &&
> > + echo content2 >bar &&
> > + git commit -o bar -m "bar again"
> > +'
> > +
> > +test_expect_success 'commit with -o fails with untracked files' '
> > + test_when_finished "rm -rf testdir" &&
> > + git init testdir &&
> > + echo content >testdir/bar &&
> > + test_must_fail git -C testdir commit -o bar -m "bar"
> > +'
> > +
> > +test_expect_success 'commit with -o exludes staged changes' '
>
> s/exludes/excludes/
Done.
>
> > + test_when_finished "git reset --hard" &&
> > + echo content >bar &&
> > + echo content >baz &&
> > + git add . &&
> > + git commit -m "bar baz" &&
> > +
> > + echo content2 >bar &&
> > + echo content2 >baz &&
> > + git add baz &&
> > + git commit -o bar -m "bar" &&
> > + git diff --name-only --staged >actual &&
> > + test_grep "baz" actual
> > +'
>
> Thanks.
Thanks for the review!
^ permalink raw reply
* 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
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