* 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: [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
* [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
* [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 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
* 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
* 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
* [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
* [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 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 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
* 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
* 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
* [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
* [PATCH 5/6] t5526: break test submodule differently
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: 1873 bytes --]
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.
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.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t5526-fetch-submodules.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 7ab220fa31..5e566205ba 100755
--- 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' '
git -C dst fetch --recurse-submodules &&
# 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 &&
# NOTE: without the fix the following tests will recurse forever!
# They should terminate with an error.
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 4/6] t1419: mark test suite as files-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: 1617 bytes --]
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.
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.
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.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1419-exclude-refs.sh | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/t/t1419-exclude-refs.sh b/t/t1419-exclude-refs.sh
index 5d8c86b657..1359574419 100755
--- a/t/t1419-exclude-refs.sh
+++ b/t/t1419-exclude-refs.sh
@@ -8,6 +8,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 `git for-each-ref --exclude` tests; need files backend'
+ test_done
+fi
+
for_each_ref__exclude () {
GIT_TRACE2_PERF=1 test-tool ref-store main \
for-each-ref--exclude "$@" >actual.raw
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 3/6] t1302: make tests more robust with new extensions
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: 2151 bytes --]
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 &&
+ git config extensions.preciousObjects 1 &&
check_allow
'
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 2/6] t1301: mark test for `core.sharedRepository` as reffiles 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: 1006 bytes --]
In t1301 we verify whether reflog files written by the "files" ref
backend correctly honor permissions when "core.sharedRepository" is set.
The test logic is thus specific to the reffiles backend and will not
work with any other backends.
Mark the test accordingly with the REFFILES prereq.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1301-shared-repo.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index e5a0d65caa..8e2c01e760 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -137,7 +137,7 @@ test_expect_success POSIXPERM 'info/refs respects umask in unshared repo' '
test_cmp expect actual
'
-test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '
+test_expect_success REFFILES,POSIXPERM 'git reflog expire honors core.sharedRepository' '
umask 077 &&
git config core.sharedRepository group &&
git reflog expire --all &&
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 1/6] t1300: mark tests to require default repo format
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: 2319 bytes --]
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.
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
'
-test_expect_success 'aliases can be CamelCased' '
+test_expect_success DEFAULT_REPO_FORMAT 'aliases can be CamelCased' '
test_config alias.CamelCased "rev-parse HEAD" &&
git CamelCased >out &&
git rev-parse HEAD >expect &&
@@ -2051,7 +2051,7 @@ test_expect_success '--show-origin stdin with file include' '
test_cmp expect output
'
-test_expect_success '--show-origin blob' '
+test_expect_success DEFAULT_REPO_FORMAT '--show-origin blob' '
blob=$(git hash-object -w "$CUSTOM_CONFIG_FILE") &&
cat >expect <<-EOF &&
blob:$blob user.custom=true
@@ -2060,7 +2060,7 @@ test_expect_success '--show-origin blob' '
test_cmp expect output
'
-test_expect_success '--show-origin blob ref' '
+test_expect_success DEFAULT_REPO_FORMAT '--show-origin blob ref' '
cat >expect <<-\EOF &&
blob:main:custom.conf user.custom=true
EOF
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 0/6] t: mark "files"-backend specific tests
From: Patrick Steinhardt @ 2024-01-09 12:17 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 1141 bytes --]
Hi,
this patch series amends some of our tests that exercise "files"-backend
specific logic to have the respective prereqs. The patch series is quite
unexciting and part of our final preparations to get the new "reftable"
backend upstream.
The series depends on ps/refstorage-extension as it makes use of the new
DEFAULT_REPO_FORMAT prereq introduced by that branch.
Patrick
Patrick Steinhardt (6):
t1300: mark tests to require default repo format
t1301: mark test for `core.sharedRepository` as reffiles specific
t1302: make tests more robust with new extensions
t1419: mark test suite as files-backend specific
t5526: break test submodule differently
t: mark tests regarding git-pack-refs(1) to be backend specific
t/t1300-config.sh | 8 ++++----
t/t1301-shared-repo.sh | 2 +-
t/t1302-repo-version.sh | 19 +++++++++++++++----
t/t1409-avoid-packing-refs.sh | 6 ++++++
t/t1419-exclude-refs.sh | 6 ++++++
t/t3210-pack-refs.sh | 6 ++++++
t/t5526-fetch-submodules.sh | 2 +-
7 files changed, 39 insertions(+), 10 deletions(-)
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] t7501: Add test for amending commit to add signoff.
From: Phillip Wood @ 2024-01-09 10:44 UTC (permalink / raw)
To: Ghanshyam Thakkar, git; +Cc: Christian Couder
In-Reply-To: <20240109060417.1144647-4-shyamthakkar001@gmail.com>
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.
> +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.
> + 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
> + 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
Best Wishes
Phillip
^ permalink raw reply
* Re: [Outreachy][PATCH v4] Port helper/test-ctype.c to unit-tests/t-ctype.c
From: Phillip Wood @ 2024-01-09 10:35 UTC (permalink / raw)
To: Achu Luma, git
Cc: gitster, chriscool, christian.couder, l.s.r, phillip.wood,
steadmon, me
In-Reply-To: <20240105161413.10422-1-ach.lumap@gmail.com>
Hi Achu
On 05/01/2024 16:14, Achu Luma wrote:
> In the recent codebase update (8bf6fbd00d (Merge branch
> 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was
> merged, providing a standardized approach for testing C code. Prior to
> this update, some unit tests relied on the test helper mechanism,
> lacking a dedicated unit testing framework. It's more natural to perform
> these unit tests using the new unit test framework.
>
> This commit migrates the unit tests for C character classification
> functions (isdigit(), isspace(), etc) from the legacy approach
> using the test-tool command `test-tool ctype` in t/helper/test-ctype.c
> to the new unit testing framework (t/unit-tests/test-lib.h).
>
> The migration involves refactoring the tests to utilize the testing
> macros provided by the framework (TEST() and check_*()).
>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Helped-by: René Scharfe <l.s.r@web.de>
> Helped-by: Phillip Wood <phillip.wood123@gmail.com>
> Helped-by: Taylor Blau <me@ttaylorr.com>
> Signed-off-by: Achu Luma <ach.lumap@gmail.com>
> ---
> The changes between version 3 and version 4 are the following:
>
> - Some duplication has been reduced using a new TEST_CHAR_CLASS() macro.
> - A "0x"prefix has been added to avoid confusion between decimal and
> hexadecimal codes printed by test_msg().
> - The "Mentored-by:..." trailer has been restored.
> - "works as expected" has been reduced to just "works" as suggested by Taylor.
> - Some "Helped-by: ..." trailers have been added.
> - Some whitespace fixes have been made.
Thanks for the re-roll, the changes look good, there is one issue that I
spotted below
> -#define TEST_CLASS(t,s) { \
> - int i; \
> - for (i = 0; i < 256; i++) { \
> - if (is_in(s, i) != t(i)) \
> - report_error(#t, i); \
> - } \
> - if (t(EOF)) \
> - report_error(#t, EOF); \
> -}
In the old version we test EOF in addition to each character between 0-255
> +/* Macro to test a character type */
> +#define TEST_CTYPE_FUNC(func, string) \
> +static void test_ctype_##func(void) { \
> + for (int i = 0; i < 256; i++) { \
> + if (!check_int(func(i), ==, is_in(string, i))) \
> + test_msg(" i: 0x%02x", i); \
> + } \
> +}
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.
Best Wishes
Phillip
^ permalink raw reply
* Re: [GSoC][RFC] Replace use of atoi() with strtol_i(), as a microproject
From: Christian Couder @ 2024-01-09 9:56 UTC (permalink / raw)
To: mohitmarathe
Cc: git@vger.kernel.org, gitster@pobox.com, britton.kerin@gmail.com,
peff@peff.net
In-Reply-To: <OqD4xQ_p-jcftCbAw0ovvrBztfiuoMGcTonCc0i6x7Ziy-hx3uA-Hoz4-3tfRI39KMj-V5OZIGgOe66b1eyX3YrKZNThMYjjMkn6g4-Ww8c=@proton.me>
Hi,
On Mon, Jan 8, 2024 at 6:38 PM <mohitmarathe@proton.me> wrote:
>
> Hello,
>
> I'm Mohit, an undergrad from India, and I want to start contributing to the Git project. I have already built Git from source and finished `git psuh` tutorial. And I must say the "Hacking Git" documentation is great (very detailed and maybe exhaustive) and easy to follow. So I also read the topic on "microprojects", and while searching for one, I came across this message: https://public-inbox.org/git/xmqqjzpjsbjl.fsf@gitster.g/.
> I want to work on this task (if it is not taken up already) as a microproject for GSoC.
Thanks for your interest in Git and the GSoC!
> Approach:
> From what I understood, the idea is to *not* allow non-integer characters in the arguments by printing an error message. So we have to replace `atoi` with `strtol_i`, like it is done in this patch: https://public-inbox.org/git/xmqq5y181fx0.fsf_-_@gitster.g/.
> There are some places where we want to continue to parse after the end of the integer (where `strspn` is used as mentioned by Junio), and based on Junio's suggestion, a new helper can be created like this:
>
> > static inline int strtol_i2(char const *s, int base, int *result, char **endp)
> > {
> > long ul;
> > char *dummy = NULL;
> >
> > if (!endp)
> > endp = &dummy;
> > errno = 0;
> > ul = strtol(s, &endp, base);
> > if (errno ||
> > /*
> > * if we are told to parse to the end of the string by
> > * passing NULL to endp, it is an error to have any
> > * remaining character after the digits.
> > */
> > (dummy && *dummy) ||
> > endp == s || (int) ul != ul)
> > return -1;
> > *result = ul;
> > return 0;
> > }
>
>
> So I searched for all the occurrences of `atoi(` (as suggested by Junio), and I found only two instances (both in `builtin/patch-id.c`) where it is followed by `strspn`. Is it safe to assume that this is the only place where we cannot directly replace `atoi` with `strtol_i`, or should I keep digging?
In https://public-inbox.org/git/xmqqjzpjsbjl.fsf@gitster.g/ Junio says:
"Some places use atoi() immediately followed by strspn() to skip over
digits, which means they are parsing an integer and want to continue
reading after the integer, which is incompatible with what
strtol_i() wants to do. They need either a separate helper or an
updated strtol_i() that optionally allows you to parse the prefix
and report where the integer ended, e.g., something like:"
and then he suggests the above helper.
So it seems that the two instances you found look like good places
where Junio says the new helper could be useful.
Now if you want to continue further on this, I think you would need to
take a closer look at those two instances to see if replacing atoi()
there with the new helper would improve something there or not. If you
find it would improve something, be sure to explain what would be
improved in the commit message.
> Also, this seems like a large change due to the number of files involved, while the documentation about the microproject emphasizes keeping the change small. Does it mean a small change per commit or a small change per Pull Request?
I think that adding a new helper in one .c file and its declaration in
the corresponding .h file, and then using it in another .c file would
change around 3 files and would be Ok size wise for a microproject.
Thanks.
^ permalink raw reply
* Re: [PATCH 0/2][GSOC] t7501: Add tests for various index usages, -i and -o, of commit command and amending commit to add signoff.
From: Christian Couder @ 2024-01-09 9:32 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: git
In-Reply-To: <20240109060417.1144647-2-shyamthakkar001@gmail.com>
On Tue, Jan 9, 2024 at 7:10 AM Ghanshyam Thakkar
<shyamthakkar001@gmail.com> wrote:
>
> This patch series adds tests for various index usages, -i and -o, of commit
> command and amending commit to add Signed-of-by trailer. This is in
> reference to the comment added in commit 12ace0b2 which reads:
>
> # FIXME: Test the various index usages, -i and -o, test reflog,
> # signoff, hooks
It seems to me that the patch series should remove or change that
FIXME if it resolves it, or some parts of it.
For example I would expect the patch that adds -i and -o related tests
to remove at least "-i and -o, " from that FIXME comment.
> Ghanshyam Thakkar (2):
> t7501: Add tests for various index usages, -i and -o, of commit
> command.
> t7501: Add test for amending commit to add signoff.
I commented on the first patch, and I took a look at the second one.
It seems to me that a number of comments I made on the first one are
valid for the second one as well. For a micro-project I would suggest
focussing on only one patch though. As we say in
https://git.github.io/General-Microproject-Information/, we want
quality, not quantity! You can always work on other possible bigger
things after your micro-project is merged.
Thanks!
^ permalink raw reply
* Re: [PATCH 1/2] t7501: Add tests for various index usages, -i and -o, of commit command.
From: Christian Couder @ 2024-01-09 9:20 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: git
In-Reply-To: <20240109060417.1144647-3-shyamthakkar001@gmail.com>
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".
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
> 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.
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.
> +test_expect_success 'commit with -i' '
> + echo content >bar &&
> + git add bar &&
> + git commit -m "bar" &&
Why is the above needed for this test?
> + 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.
> + 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/
> + 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.
^ 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