* Re: [ANNOUNCE] Git v2.44.0
From: Patrick Steinhardt @ 2024-02-24 6:36 UTC (permalink / raw)
To: Mike Hommey; +Cc: Junio C Hamano, git, git-packagers
In-Reply-To: <20240224051040.ftuo24smozqugbde@glandium.org>
[-- Attachment #1: Type: text/plain, Size: 3097 bytes --]
On Sat, Feb 24, 2024 at 02:10:40PM +0900, Mike Hommey wrote:
> Hi,
>
> On Fri, Feb 23, 2024 at 09:17:07AM -0800, Junio C Hamano wrote:
> > Patrick Steinhardt (139):
> > builtin/clone: create the refdb with the correct object format
>
> I haven't analyzed how/why exactly yet, but I've bisected a regression
> in the behavior of is_git_directory() during a clone to originate from
> this change.
>
> Here's a way to reproduce the problem:
>
> ```
> $ cat > git-remote-foo <<EOF
> #!/bin/sh
> git config --local -l >&2
> exit 1
> EOF
> $ chmod +x git-remote-foo
> $ PATH=$PWD:$PATH git clone foo::bar
> ```
>
> With versions < 2.44.0, it displays the local configuration, e.g.:
> ```
> core.repositoryformatversion=0
> core.filemode=true
> core.bare=false
> core.logallrefupdates=true
> remote.origin.url=foo::bar
> ```
>
> but with 2.44.0, it fails with:
> ```
> fatal: --local can only be used inside a git repository
> ```
Thanks for your report!
This has to be because we now initialize the refdb at a later point. The
problem here was that before my change, we initialized the refdb at a
point when it wasn't clear what the remote actually used as the object
format. The consequence was twofold:
- Cloning a repository with bundles was broken in case the remote uses
the SHA256 object format.
- Cloning into a repository that uses reftables when the remote uses
the SHA256 object format was broken, too.
Both of these have the same root cause: because we didn't connect to the
remote yet we had no idea what object format the remote uses. And as we
initialized the refdb early, it was then initialized with the default
object format, which is SHA1.
The change was to move initialization of the refdb to a later point in
time where we know what object format the remote uses. By necessity,
this has to be _after_ we have connected to the remote, because there is
no way to learn about it without connecting to it.
One consequence of initializing the refdb at a later point in time is
that we have no "HEAD" yet, and a repo without the "HEAD" file is not
considered to be a repo. Thus, git-config(1) would now rightfully fail.
I assume that you discovered it via a remote helper that does something
more interesting than git-config(1). I have to wonder whether we ever
really specified what the environment of a remote helper should look
like when used during cloning. Conceptually it doesn't feel _wrong_ to
have a not-yet-initialized repo during clone.
But on the other hand, regressing functionality like this is of course
bad. I was wondering whether we can get around this issue by setting
e.g. GIT_DIR explicitly when spawning the remote helper, but I don't
think it's as easy as that.
Another idea would be to simply pre-create HEAD regardless of the ref
format, pointing to an invalid ref "refs/heads/.invalid". This is the
same trick we use for the reftable backend, and should likely address
your issue.
I will have a deeper look on Tuesday and send a patch.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [ANNOUNCE] Git v2.44.0
From: Mike Hommey @ 2024-02-24 5:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, git-packagers, Patrick Steinhardt
In-Reply-To: <xmqqbk87w164.fsf@gitster.g>
Hi,
On Fri, Feb 23, 2024 at 09:17:07AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt (139):
> builtin/clone: create the refdb with the correct object format
I haven't analyzed how/why exactly yet, but I've bisected a regression
in the behavior of is_git_directory() during a clone to originate from
this change.
Here's a way to reproduce the problem:
```
$ cat > git-remote-foo <<EOF
#!/bin/sh
git config --local -l >&2
exit 1
EOF
$ chmod +x git-remote-foo
$ PATH=$PWD:$PATH git clone foo::bar
```
With versions < 2.44.0, it displays the local configuration, e.g.:
```
core.repositoryformatversion=0
core.filemode=true
core.bare=false
core.logallrefupdates=true
remote.origin.url=foo::bar
```
but with 2.44.0, it fails with:
```
fatal: --local can only be used inside a git repository
```
Mike
^ permalink raw reply
* Re: [PATCH v5 1/3] pager: include stdint.h because uintmax_t is used
From: Kyle Lippincott @ 2024-02-24 1:33 UTC (permalink / raw)
To: Calvin Wan; +Cc: git, Jonathan Tan, phillip.wood123, Junio C Hamano
In-Reply-To: <20240222175033.1489723-2-calvinwan@google.com>
On Thu, Feb 22, 2024 at 9:51 AM Calvin Wan <calvinwan@google.com> wrote:
>
> From: Jonathan Tan <jonathantanmy@google.com>
>
> pager.h uses uintmax_t but does not include stdint.h. Therefore, add
> this include statement.
>
> This was discovered when writing a stub pager.c file.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> ---
> pager.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/pager.h b/pager.h
> index b77433026d..015bca95e3 100644
> --- a/pager.h
> +++ b/pager.h
> @@ -1,6 +1,8 @@
> #ifndef PAGER_H
> #define PAGER_H
>
> +#include <stdint.h>
> +
> struct child_process;
>
> const char *git_pager(int stdout_is_tty);
> --
> 2.44.0.rc0.258.g7320e95886-goog
>
>
As far as I can tell, we need pager.h because of the `pager_in_use`
symbol. We need that symbol because of its use in date.c's
`parse_date_format`. I wonder if we can side step the `#include
<stdint.h>` concerns by splitting pager.h into pager.h and
pager_in_use.h, and have pager.h include pager_in_use.h instead. This
way pager.h (and its [unused] forward declarations) aren't part of
git-std-lib at all. I believe this was done for things like hex-ll.h,
so maybe we call it pager-ll.h. The goal being to (a) not need the
`#include <stdint.h>` because that's currently contentious, but also
(b) to identify the minimum set of symbols needed for the stubs
library, and not declare things that we don't have any intention of
actually providing / stubbing out.
I have some more thoughts on this, but they're much more appropriate
for the next patch in the series, so I'll leave them there.
^ permalink raw reply
* Re: [PATCH v2 03/11] Start reporting missing commits in `repo_in_merge_bases_many()`
From: Junio C Hamano @ 2024-02-24 0:33 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Patrick Steinhardt, Johannes Schindelin
In-Reply-To: <4dd214f91d4783f29b03908cc0034156253889a7.1708608110.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Some functions in Git's source code follow the convention that returning
> a negative value indicates a fatal error, e.g. repository corruption.
>
> Let's use this convention in `repo_in_merge_bases()` to report when one
> of the specified commits is missing (i.e. when `repo_parse_commit()`
> reports an error).
>
> Also adjust the callers of `repo_in_merge_bases()` to handle such
> negative return values.
All of the above makes sense, but I have to wonder if this hunk
should rather want to be part of the previous step:
> @@ -486,10 +488,10 @@ int repo_in_merge_bases_many(struct repository *r, struct commit *commit,
> timestamp_t generation, max_generation = GENERATION_NUMBER_ZERO;
>
> if (repo_parse_commit(r, commit))
> - return ret;
> + return ignore_missing_commits ? 0 : -1;
> for (i = 0; i < nr_reference; i++) {
> if (repo_parse_commit(r, reference[i]))
> - return ret;
> + return ignore_missing_commits ? 0 : -1;
>
> generation = commit_graph_generation(reference[i]);
> if (generation > max_generation)
as this hunk is not about many callers of repo_in_merge_bases() that
ignored the return values, which are all fixed by this patch, but
about returning that error signal back to the caller.
Yes, I know you wrote in [02/11] that it does not change the
behaviour, and if you move this hunk to [02/11], it might change the
behaviour, but that is changing for the better. Besides, adding a
parameter "ignore_missing" to the function only to be ignored until
the next patch feels rather incomplete.
The other changes in this patch about its primary theme, fixing the
callers that used to ignore return values of repo_in_merge_bases(),
all looked sensible. This hunk somehow stood out like a sore thumb
to me.
^ permalink raw reply
* [PATCH v3 7/7] ci: use test-tool as unit test runner on Windows
From: Josh Steadmon @ 2024-02-23 23:33 UTC (permalink / raw)
To: git; +Cc: johannes.schindelin, peff, phillip.wood, gitster
In-Reply-To: <cover.1708728717.git.steadmon@google.com>
Although the previous commit changed t/Makefile to run unit tests
alongside shell tests, the Windows CI still needs a separate unit-tests
step due to how the test sharding works.
We want to avoid using `prove` as a test running on Windows due to
performance issues [1], so use the new test-tool runner instead.
[1] https://lore.kernel.org/git/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
ci/run-test-slice.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ci/run-test-slice.sh b/ci/run-test-slice.sh
index ae8094382f..e167e646f7 100755
--- a/ci/run-test-slice.sh
+++ b/ci/run-test-slice.sh
@@ -17,7 +17,7 @@ handle_failed_tests
# We only have one unit test at the moment, so run it in the first slice
if [ "$1" == "0" ] ; then
- group "Run unit tests" make --quiet -C t unit-tests-prove
+ group "Run unit tests" make --quiet -C t unit-tests-test-tool
fi
check_unignored_build_artifacts
--
2.44.0.rc0.258.g7320e95886-goog
^ permalink raw reply related
* [PATCH v3 6/7] t/Makefile: run unit tests alongside shell tests
From: Josh Steadmon @ 2024-02-23 23:33 UTC (permalink / raw)
To: git; +Cc: johannes.schindelin, peff, phillip.wood, gitster
In-Reply-To: <cover.1708728717.git.steadmon@google.com>
From: Jeff King <peff@peff.net>
Add a wrapper script to allow `prove` to run both shell tests and unit
tests from a single invocation. This avoids issues around running prove
twice in CI, as discussed in [1].
Additionally, this moves the unit tests into the main dev workflow, so
that errors can be spotted more quickly. Accordingly, we remove the
separate unit tests step for Linux CI. (We leave the Windows CI
unit-test step as-is, because the sharding scheme there involves
selecting specific test files rather than running `make test`.)
[1] https://lore.kernel.org/git/pull.1613.git.1699894837844.gitgitgadget@gmail.com/
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
ci/run-build-and-tests.sh | 2 --
t/Makefile | 2 +-
t/run-test.sh | 13 +++++++++++++
3 files changed, 14 insertions(+), 3 deletions(-)
create mode 100755 t/run-test.sh
diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 7a1466b868..2528f25e31 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -50,8 +50,6 @@ if test -n "$run_tests"
then
group "Run tests" make test ||
handle_failed_tests
- group "Run unit tests" \
- make DEFAULT_UNIT_TEST_TARGET=unit-tests-prove unit-tests
fi
check_unignored_build_artifacts
diff --git a/t/Makefile b/t/Makefile
index 0ae04f1e42..0c739754d8 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -68,7 +68,7 @@ failed:
test -z "$$failed" || $(MAKE) $$failed
prove: pre-clean check-chainlint $(TEST_LINT)
- @echo "*** prove ***"; $(CHAINLINTSUPPRESS) $(PROVE) --exec '$(TEST_SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
+ @echo "*** prove (shell & unit tests) ***"; $(CHAINLINTSUPPRESS) $(PROVE) --exec ./run-test.sh $(GIT_PROVE_OPTS) $(T) $(UNIT_TESTS) :: $(GIT_TEST_OPTS)
$(MAKE) clean-except-prove-cache
$(T):
diff --git a/t/run-test.sh b/t/run-test.sh
new file mode 100755
index 0000000000..c29fef48dc
--- /dev/null
+++ b/t/run-test.sh
@@ -0,0 +1,13 @@
+#!/bin/sh
+
+# A simple wrapper to run shell tests via TEST_SHELL_PATH,
+# or exec unit tests directly.
+
+case "$1" in
+*.sh)
+ exec ${TEST_SHELL_PATH:-/bin/sh} "$@"
+ ;;
+*)
+ exec "$@"
+ ;;
+esac
--
2.44.0.rc0.258.g7320e95886-goog
^ permalink raw reply related
* [PATCH v3 5/7] unit tests: add rule for running with test-tool
From: Josh Steadmon @ 2024-02-23 23:33 UTC (permalink / raw)
To: git; +Cc: johannes.schindelin, peff, phillip.wood, gitster
In-Reply-To: <cover.1708728717.git.steadmon@google.com>
In the previous commit, we added support in test-tool for running
collections of unit tests. Now, add rules in t/Makefile for running in
this way.
This new rule can be executed from the top-level Makefile via
`make DEFAULT_UNIT_TEST_TARGET=unit-tests-test-tool unit-tests`, or by
setting DEFAULT_UNIT_TEST_TARGET in config.mak.
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
Makefile | 2 +-
t/Makefile | 10 +++++++++-
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/Makefile b/Makefile
index ba55d817ee..b0d1f04b4d 100644
--- a/Makefile
+++ b/Makefile
@@ -3870,5 +3870,5 @@ $(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/
.PHONY: build-unit-tests unit-tests
build-unit-tests: $(UNIT_TEST_PROGS)
-unit-tests: $(UNIT_TEST_PROGS)
+unit-tests: $(UNIT_TEST_PROGS) t/helper/test-tool$X
$(MAKE) -C t/ unit-tests
diff --git a/t/Makefile b/t/Makefile
index 4861edafe6..0ae04f1e42 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -49,6 +49,7 @@ CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
UNIT_TESTS = $(sort $(UNIT_TEST_PROGRAMS))
+UNIT_TESTS_NO_DIR = $(notdir $(UNIT_TESTS))
# `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)
# checks all tests in all scripts via a single invocation, so tell individual
@@ -76,7 +77,7 @@ $(T):
$(UNIT_TESTS):
@echo "*** $@ ***"; $@
-.PHONY: unit-tests unit-tests-raw unit-tests-prove
+.PHONY: unit-tests unit-tests-raw unit-tests-prove unit-tests-test-tool
unit-tests: $(DEFAULT_UNIT_TEST_TARGET)
unit-tests-raw: $(UNIT_TESTS)
@@ -84,6 +85,13 @@ unit-tests-raw: $(UNIT_TESTS)
unit-tests-prove:
@echo "*** prove - unit tests ***"; $(PROVE) $(GIT_PROVE_OPTS) $(UNIT_TESTS)
+unit-tests-test-tool:
+ @echo "*** test-tool - unit tests **"
+ ( \
+ cd unit-tests/bin && \
+ ../../helper/test-tool$X run-command testsuite $(UNIT_TESTS_NO_DIR)\
+ )
+
pre-clean:
$(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)'
--
2.44.0.rc0.258.g7320e95886-goog
^ permalink raw reply related
* [PATCH v3 4/7] test-tool run-command testsuite: support unit tests
From: Josh Steadmon @ 2024-02-23 23:33 UTC (permalink / raw)
To: git; +Cc: johannes.schindelin, peff, phillip.wood, gitster
In-Reply-To: <cover.1708728717.git.steadmon@google.com>
Teach the testsuite runner in `test-tool run-command testsuite` how to
run unit tests: if TEST_SHELL_PATH is not set, run the programs directly
from CWD, rather than defaulting to "sh" as an interpreter.
With this change, you can now use test-tool to run the unit tests:
$ make
$ cd t/unit-tests/bin
$ ../../helper/test-tool run-command testsuite
This should be helpful on Windows to allow running tests without
requiring Perl (for `prove`), as discussed in [1] and [2].
This again breaks backwards compatibility, as it is now required to set
TEST_SHELL_PATH properly for executing shell scripts, but again, as
noted in [2], there are no longer any such invocations in our codebase.
[1] https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/
[2] https://lore.kernel.org/git/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
t/helper/test-run-command.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index e6bd792274..61eb1175fe 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -158,6 +158,8 @@ static int testsuite(int argc, const char **argv)
.task_finished = test_finished,
.data = &suite,
};
+ struct strbuf progpath = STRBUF_INIT;
+ size_t path_prefix_len;
argc = parse_options(argc, argv, NULL, options,
testsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION);
@@ -165,9 +167,13 @@ static int testsuite(int argc, const char **argv)
if (max_jobs <= 0)
max_jobs = online_cpus();
+ /*
+ * If we run without a shell, execute the programs directly from CWD.
+ */
suite.shell_path = getenv("TEST_SHELL_PATH");
if (!suite.shell_path)
- suite.shell_path = "sh";
+ strbuf_addstr(&progpath, "./");
+ path_prefix_len = progpath.len;
dir = opendir(".");
if (!dir)
@@ -180,13 +186,17 @@ static int testsuite(int argc, const char **argv)
/* No pattern: match all */
if (!argc) {
- string_list_append(&suite.tests, p);
+ strbuf_setlen(&progpath, path_prefix_len);
+ strbuf_addstr(&progpath, p);
+ string_list_append(&suite.tests, progpath.buf);
continue;
}
for (i = 0; i < argc; i++)
if (!wildmatch(argv[i], p, 0)) {
- string_list_append(&suite.tests, p);
+ strbuf_setlen(&progpath, path_prefix_len);
+ strbuf_addstr(&progpath, p);
+ string_list_append(&suite.tests, progpath.buf);
break;
}
}
@@ -213,6 +223,7 @@ static int testsuite(int argc, const char **argv)
string_list_clear(&suite.tests, 0);
string_list_clear(&suite.failed, 0);
+ strbuf_release(&progpath);
return ret;
}
--
2.44.0.rc0.258.g7320e95886-goog
^ permalink raw reply related
* [PATCH v3 3/7] test-tool run-command testsuite: remove hardcoded filter
From: Josh Steadmon @ 2024-02-23 23:33 UTC (permalink / raw)
To: git; +Cc: johannes.schindelin, peff, phillip.wood, gitster
In-Reply-To: <cover.1708728717.git.steadmon@google.com>
`test-tool run-command testsuite` currently assumes that it will only be
running the shell test suite, and therefore filters out anything that
does not match a hardcoded pattern of "t[0-9][0-9][0-9][0-9]-*.sh".
Later in this series, we'll adapt `test-tool run-command testsuite` to
also support unit tests, which do not follow the same naming conventions
as the shell tests, so this hardcoded pattern is inconvenient.
Since `testsuite` also allows specifying patterns on the command-line,
let's just remove this pattern. As noted in [1], there are no longer any
uses of `testsuite` in our codebase, it should be OK to break backwards
compatibility in this case. We also add a new filter to avoid trying to
execute "." and "..", so that users who wish to execute every test in a
directory can do so without specifying a pattern.
[1] https://lore.kernel.org/git/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
t/helper/test-run-command.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index a41a54d9cb..e6bd792274 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -175,9 +175,7 @@ static int testsuite(int argc, const char **argv)
while ((d = readdir(dir))) {
const char *p = d->d_name;
- if (*p != 't' || !isdigit(p[1]) || !isdigit(p[2]) ||
- !isdigit(p[3]) || !isdigit(p[4]) || p[5] != '-' ||
- !ends_with(p, ".sh"))
+ if (!strcmp(p, ".") || !strcmp(p, ".."))
continue;
/* No pattern: match all */
--
2.44.0.rc0.258.g7320e95886-goog
^ permalink raw reply related
* [PATCH v3 2/7] test-tool run-command testsuite: get shell from env
From: Josh Steadmon @ 2024-02-23 23:33 UTC (permalink / raw)
To: git; +Cc: johannes.schindelin, peff, phillip.wood, gitster
In-Reply-To: <cover.1708728717.git.steadmon@google.com>
When running tests through `test-tool run-command testsuite`, we
currently hardcode `sh` as the command interpreter. As discussed in [1],
this is incorrect, and we should be using the shell set in
TEST_SHELL_PATH instead.
Add a shell_path field in struct testsuite so that we can pass this to
the task runner callback. If this is non-null, we'll use it as the
argv[0] of the subprocess. Otherwise, we'll just execute the test
program directly. We will use this feature in a later commit to enable
running binary executable unit tests.
However, for now when setting up the struct testsuite in testsuite(),
use the value of TEST_SHELL_PATH if it's set, otherwise keep the
original behavior by defaulting to `sh`.
[1] https://lore.kernel.org/git/20240123005913.GB835964@coredump.intra.peff.net/
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
t/helper/test-run-command.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index c0ed8722c8..a41a54d9cb 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -65,6 +65,7 @@ struct testsuite {
struct string_list tests, failed;
int next;
int quiet, immediate, verbose, verbose_log, trace, write_junit_xml;
+ const char *shell_path;
};
#define TESTSUITE_INIT { \
.tests = STRING_LIST_INIT_DUP, \
@@ -80,7 +81,9 @@ static int next_test(struct child_process *cp, struct strbuf *err, void *cb,
return 0;
test = suite->tests.items[suite->next++].string;
- strvec_pushl(&cp->args, "sh", test, NULL);
+ if (suite->shell_path)
+ strvec_push(&cp->args, suite->shell_path);
+ strvec_push(&cp->args, test);
if (suite->quiet)
strvec_push(&cp->args, "--quiet");
if (suite->immediate)
@@ -162,6 +165,10 @@ static int testsuite(int argc, const char **argv)
if (max_jobs <= 0)
max_jobs = online_cpus();
+ suite.shell_path = getenv("TEST_SHELL_PATH");
+ if (!suite.shell_path)
+ suite.shell_path = "sh";
+
dir = opendir(".");
if (!dir)
die("Could not open the current directory");
--
2.44.0.rc0.258.g7320e95886-goog
^ permalink raw reply related
* [PATCH v3 1/7] t0080: turn t-basic unit test into a helper
From: Josh Steadmon @ 2024-02-23 23:33 UTC (permalink / raw)
To: git; +Cc: johannes.schindelin, peff, phillip.wood, gitster
In-Reply-To: <cover.1708728717.git.steadmon@google.com>
While t/unit-tests/t-basic.c uses the unit-test framework added in
e137fe3b29 (unit tests: add TAP unit test framework, 2023-11-09), it is
not a true unit test in that it intentionally fails in order to exercise
various codepaths in the unit-test framework. Thus, we intentionally
exclude it when running unit tests through the various t/Makefile
targets. Instead, it is executed by t0080-unit-test-output.sh, which
verifies its output follows the TAP format expected for the various
pass, skip, or fail cases.
As such, it makes more sense for t-basic to be a helper item for
t0080-unit-test-output.sh, so let's move it to
t/helper/test-example-tap.c and adjust Makefiles as necessary.
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
Makefile | 4 ++--
t/Makefile | 2 +-
.../t-basic.c => helper/test-example-tap.c} | 5 ++--
t/helper/test-tool.c | 1 +
t/helper/test-tool.h | 1 +
t/t0080-unit-test-output.sh | 24 +++++++++----------
6 files changed, 20 insertions(+), 17 deletions(-)
rename t/{unit-tests/t-basic.c => helper/test-example-tap.c} (95%)
diff --git a/Makefile b/Makefile
index 23723367b8..ba55d817ee 100644
--- a/Makefile
+++ b/Makefile
@@ -802,6 +802,7 @@ TEST_BUILTINS_OBJS += test-dump-split-index.o
TEST_BUILTINS_OBJS += test-dump-untracked-cache.o
TEST_BUILTINS_OBJS += test-env-helper.o
TEST_BUILTINS_OBJS += test-example-decorate.o
+TEST_BUILTINS_OBJS += test-example-tap.o
TEST_BUILTINS_OBJS += test-find-pack.o
TEST_BUILTINS_OBJS += test-fsmonitor-client.o
TEST_BUILTINS_OBJS += test-genrandom.o
@@ -1338,7 +1339,6 @@ THIRD_PARTY_SOURCES += compat/regex/%
THIRD_PARTY_SOURCES += sha1collisiondetection/%
THIRD_PARTY_SOURCES += sha1dc/%
-UNIT_TEST_PROGRAMS += t-basic
UNIT_TEST_PROGRAMS += t-mem-pool
UNIT_TEST_PROGRAMS += t-strbuf
UNIT_TEST_PROGRAMS += t-ctype
@@ -3218,7 +3218,7 @@ perf: all
.PRECIOUS: $(TEST_OBJS)
-t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
+t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS)) $(UNIT_TEST_DIR)/test-lib.o
t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS) $(REFTABLE_TEST_LIB)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
diff --git a/t/Makefile b/t/Makefile
index 2d95046f26..4861edafe6 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -48,7 +48,7 @@ CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.tes
CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
-UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))
+UNIT_TESTS = $(sort $(UNIT_TEST_PROGRAMS))
# `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)
# checks all tests in all scripts via a single invocation, so tell individual
diff --git a/t/unit-tests/t-basic.c b/t/helper/test-example-tap.c
similarity index 95%
rename from t/unit-tests/t-basic.c
rename to t/helper/test-example-tap.c
index fda1ae59a6..d072ad559f 100644
--- a/t/unit-tests/t-basic.c
+++ b/t/helper/test-example-tap.c
@@ -1,4 +1,5 @@
-#include "test-lib.h"
+#include "test-tool.h"
+#include "t/unit-tests/test-lib.h"
/*
* The purpose of this "unit test" is to verify a few invariants of the unit
@@ -69,7 +70,7 @@ static void t_empty(void)
; /* empty */
}
-int cmd_main(int argc, const char **argv)
+int cmd__example_tap(int argc, const char **argv)
{
test_res = TEST(check_res = check_int(1, ==, 1), "passing test");
TEST(t_res(1), "passing test and assertion return 1");
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 33b9501c21..bb5c04c9c0 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -29,6 +29,7 @@ static struct test_cmd cmds[] = {
{ "dump-untracked-cache", cmd__dump_untracked_cache },
{ "env-helper", cmd__env_helper },
{ "example-decorate", cmd__example_decorate },
+ { "example-tap", cmd__example_tap },
{ "find-pack", cmd__find_pack },
{ "fsmonitor-client", cmd__fsmonitor_client },
{ "genrandom", cmd__genrandom },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index b72f07ded9..38001bd1c6 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -23,6 +23,7 @@ int cmd__dump_untracked_cache(int argc, const char **argv);
int cmd__dump_reftable(int argc, const char **argv);
int cmd__env_helper(int argc, const char **argv);
int cmd__example_decorate(int argc, const char **argv);
+int cmd__example_tap(int argc, const char **argv);
int cmd__find_pack(int argc, const char **argv);
int cmd__fsmonitor_client(int argc, const char **argv);
int cmd__genrandom(int argc, const char **argv);
diff --git a/t/t0080-unit-test-output.sh b/t/t0080-unit-test-output.sh
index 961b54b06c..83b1e3b7f5 100755
--- a/t/t0080-unit-test-output.sh
+++ b/t/t0080-unit-test-output.sh
@@ -8,50 +8,50 @@ test_expect_success 'TAP output from unit tests' '
cat >expect <<-EOF &&
ok 1 - passing test
ok 2 - passing test and assertion return 1
- # check "1 == 2" failed at t/unit-tests/t-basic.c:76
+ # check "1 == 2" failed at t/helper/test-example-tap.c:77
# left: 1
# right: 2
not ok 3 - failing test
ok 4 - failing test and assertion return 0
not ok 5 - passing TEST_TODO() # TODO
ok 6 - passing TEST_TODO() returns 1
- # todo check ${SQ}check(x)${SQ} succeeded at t/unit-tests/t-basic.c:25
+ # todo check ${SQ}check(x)${SQ} succeeded at t/helper/test-example-tap.c:26
not ok 7 - failing TEST_TODO()
ok 8 - failing TEST_TODO() returns 0
- # check "0" failed at t/unit-tests/t-basic.c:30
+ # check "0" failed at t/helper/test-example-tap.c:31
# skipping test - missing prerequisite
- # skipping check ${SQ}1${SQ} at t/unit-tests/t-basic.c:32
+ # skipping check ${SQ}1${SQ} at t/helper/test-example-tap.c:33
ok 9 - test_skip() # SKIP
ok 10 - skipped test returns 1
# skipping test - missing prerequisite
ok 11 - test_skip() inside TEST_TODO() # SKIP
ok 12 - test_skip() inside TEST_TODO() returns 1
- # check "0" failed at t/unit-tests/t-basic.c:48
+ # check "0" failed at t/helper/test-example-tap.c:49
not ok 13 - TEST_TODO() after failing check
ok 14 - TEST_TODO() after failing check returns 0
- # check "0" failed at t/unit-tests/t-basic.c:56
+ # check "0" failed at t/helper/test-example-tap.c:57
not ok 15 - failing check after TEST_TODO()
ok 16 - failing check after TEST_TODO() returns 0
- # check "!strcmp("\thello\\\\", "there\"\n")" failed at t/unit-tests/t-basic.c:61
+ # check "!strcmp("\thello\\\\", "there\"\n")" failed at t/helper/test-example-tap.c:62
# left: "\011hello\\\\"
# right: "there\"\012"
- # check "!strcmp("NULL", NULL)" failed at t/unit-tests/t-basic.c:62
+ # check "!strcmp("NULL", NULL)" failed at t/helper/test-example-tap.c:63
# left: "NULL"
# right: NULL
- # check "${SQ}a${SQ} == ${SQ}\n${SQ}" failed at t/unit-tests/t-basic.c:63
+ # check "${SQ}a${SQ} == ${SQ}\n${SQ}" failed at t/helper/test-example-tap.c:64
# left: ${SQ}a${SQ}
# right: ${SQ}\012${SQ}
- # check "${SQ}\\\\${SQ} == ${SQ}\\${SQ}${SQ}" failed at t/unit-tests/t-basic.c:64
+ # check "${SQ}\\\\${SQ} == ${SQ}\\${SQ}${SQ}" failed at t/helper/test-example-tap.c:65
# left: ${SQ}\\\\${SQ}
# right: ${SQ}\\${SQ}${SQ}
not ok 17 - messages from failing string and char comparison
- # BUG: test has no checks at t/unit-tests/t-basic.c:91
+ # BUG: test has no checks at t/helper/test-example-tap.c:92
not ok 18 - test with no checks
ok 19 - test with no checks returns 0
1..19
EOF
- ! "$GIT_BUILD_DIR"/t/unit-tests/bin/t-basic >actual &&
+ ! test-tool example-tap >actual &&
test_cmp expect actual
'
--
2.44.0.rc0.258.g7320e95886-goog
^ permalink raw reply related
* [PATCH v3 0/7] test-tool: add unit test suite runner
From: Josh Steadmon @ 2024-02-23 23:33 UTC (permalink / raw)
To: git; +Cc: johannes.schindelin, peff, phillip.wood, gitster
In-Reply-To: <cover.1705443632.git.steadmon@google.com>
Please note: this series has once again been rebased onto the latest
jk/unit-tests-buildfix.
For various reasons (see discussion at [1]) we would like an alternative
to `prove` for running test suites (including the unit tests) on
Windows.
[1] https://lore.kernel.org/git/pull.1613.git.1699894837844.gitgitgadget@gmail.com/
This series extends the existing `test-tool run-command testsuite` to
support running unit tests. In addition, it includes some small
cleanups:
* move t-basic out of the unit-tests directory
* don't hardcode the shell for running tests in `test-tool ... testsuite`
* don't hardcode a test name filter in `test-tool ... testsuite`
* add a test wrapper script to allow unit tests and the shell test suite
to run in a single `prove` process
Changes in V3:
* Added new patch (#7) to use the new test-tool runner for unit tests in
Windows CI.
* Restored the explicit sort call in t/Makefile, for backwards
compatibility with older GNU Make versions.
* Rebased the series on the latest jk/unit-tests-buildfix branch.
* Fixed header include order in patch #1.
* Removed a paragraph in patch #1's commit message that is obsolete now
that we're building the list of test files from the sources rather
than by globbing.
* Added a note in patch #2 that setting a NULL suite.shell_path will be
used in a later commit.
* Clarified up some sloppy wording in commit messages and comments in
t/helper/test-run-command.c.
Changes in V2:
* Rebased the series on the latest jk/unit-tests-buildfix branch.
* Patch 1: move t-basic to a test-tool subcommand rather than a new
executable under t/t0080/
* New patch 2: get the shell path from TEST_SHELL_PATH in
`test-tool run-command testsuite`
* New patch 3: remove the hardcoded filename filter in
`test-tool run-command testsuite`
* Patch 4 (previously 2): simplified now that we no longer need to add
any command-line flags to support unit tests
* Patch 5 (previously 3): avoid trying to run cmake *.pdb files by using
the unit test list built in the makefile in jk/unit-tests-buildfix.
Jeff King (1):
t/Makefile: run unit tests alongside shell tests
Josh Steadmon (6):
t0080: turn t-basic unit test into a helper
test-tool run-command testsuite: get shell from env
test-tool run-command testsuite: remove hardcoded filter
test-tool run-command testsuite: support unit tests
unit tests: add rule for running with test-tool
ci: use test-tool as unit test runner on Windows
Makefile | 6 ++--
ci/run-build-and-tests.sh | 2 --
ci/run-test-slice.sh | 2 +-
t/Makefile | 14 ++++++++--
.../t-basic.c => helper/test-example-tap.c} | 5 ++--
t/helper/test-run-command.c | 28 +++++++++++++++----
t/helper/test-tool.c | 1 +
t/helper/test-tool.h | 1 +
t/run-test.sh | 13 +++++++++
t/t0080-unit-test-output.sh | 24 ++++++++--------
10 files changed, 67 insertions(+), 29 deletions(-)
rename t/{unit-tests/t-basic.c => helper/test-example-tap.c} (95%)
create mode 100755 t/run-test.sh
Range-diff against v2:
1: da756b4bfb ! 1: 6777451100 t0080: turn t-basic unit test into a helper
@@ Commit message
t0080-unit-test-output.sh, so let's move it to
t/helper/test-example-tap.c and adjust Makefiles as necessary.
- This has the additional benefit that test harnesses seeking to run all
- unit tests can find them with a simple glob of "t/unit-tests/bin/t-*",
- with no exceptions needed. This will be important in a later patch where
- we add support for running the unit tests via a test-tool subcommand.
-
## Makefile ##
@@ Makefile: perf: all
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
## t/Makefile ##
-@@ t/Makefile: TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))
- CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
+@@ t/Makefile: CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.tes
CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
--UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
+ UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
-UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))
-+UNIT_TESTS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
++UNIT_TESTS = $(sort $(UNIT_TEST_PROGRAMS))
# `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)
# checks all tests in all scripts via a single invocation, so tell individual
@@ t/Makefile: TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))
## t/unit-tests/t-basic.c => t/helper/test-example-tap.c ##
@@
-#include "test-lib.h"
-+#include "t/unit-tests/test-lib.h"
+#include "test-tool.h"
++#include "t/unit-tests/test-lib.h"
/*
* The purpose of this "unit test" is to verify a few invariants of the unit
2: c8448406d7 ! 2: 24f47f8fc7 test-tool run-command testsuite: get shell from env
@@ Commit message
Add a shell_path field in struct testsuite so that we can pass this to
the task runner callback. If this is non-null, we'll use it as the
argv[0] of the subprocess. Otherwise, we'll just execute the test
- program directly.
+ program directly. We will use this feature in a later commit to enable
+ running binary executable unit tests.
- When setting up the struct testsuite in testsuite(), use the value
- of TEST_SHELL_PATH if it's set, otherwise default to `sh`.
+ However, for now when setting up the struct testsuite in testsuite(),
+ use the value of TEST_SHELL_PATH if it's set, otherwise keep the
+ original behavior by defaulting to `sh`.
[1] https://lore.kernel.org/git/20240123005913.GB835964@coredump.intra.peff.net/
3: e1b89ae93e = 3: 4a16a3ec24 test-tool run-command testsuite: remove hardcoded filter
4: b5665386b5 ! 4: abc9a7afe8 test-tool run-command testsuite: support unit tests
@@ Commit message
test-tool run-command testsuite: support unit tests
Teach the testsuite runner in `test-tool run-command testsuite` how to
- run unit tests: if TEST_SHELL_PATH is not set, assume that we're running
- the programs directly from CWD, rather than defaulting to "sh" as an
- interpreter.
+ run unit tests: if TEST_SHELL_PATH is not set, run the programs directly
+ from CWD, rather than defaulting to "sh" as an interpreter.
With this change, you can now use test-tool to run the unit tests:
$ make
@@ t/helper/test-run-command.c: static int testsuite(int argc, const char **argv)
max_jobs = online_cpus();
+ /*
-+ * If we run without a shell, we have to provide the relative path to
-+ * the executables.
++ * If we run without a shell, execute the programs directly from CWD.
+ */
suite.shell_path = getenv("TEST_SHELL_PATH");
if (!suite.shell_path)
5: f2746703d5 ! 5: a8bbff2c6b unit tests: add rule for running with test-tool
@@ Makefile: $(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_
$(MAKE) -C t/ unit-tests
## t/Makefile ##
-@@ t/Makefile: CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.tes
- CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
+@@ t/Makefile: CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
- UNIT_TESTS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
+ UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
+ UNIT_TESTS = $(sort $(UNIT_TEST_PROGRAMS))
+UNIT_TESTS_NO_DIR = $(notdir $(UNIT_TESTS))
# `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)
6: cd7467a7bd ! 6: cfcc4bd427 t/Makefile: run unit tests alongside shell tests
@@ Commit message
twice in CI, as discussed in [1].
Additionally, this moves the unit tests into the main dev workflow, so
- that errors can be spotted more quickly.
-
- NEEDS WORK: as discussed in previous commits in this series, there's a
- desire to avoid `prove` specifically and (IIUC) unnecessary
- fork()/exec()ing in general on Windows. This change adds an extra exec()
- for each shell and unit test execution, will that be a problem for
- Windows?
+ that errors can be spotted more quickly. Accordingly, we remove the
+ separate unit tests step for Linux CI. (We leave the Windows CI
+ unit-test step as-is, because the sharding scheme there involves
+ selecting specific test files rather than running `make test`.)
[1] https://lore.kernel.org/git/pull.1613.git.1699894837844.gitgitgadget@gmail.com/
Signed-off-by: Jeff King <peff@peff.net>
+ ## ci/run-build-and-tests.sh ##
+@@ ci/run-build-and-tests.sh: if test -n "$run_tests"
+ then
+ group "Run tests" make test ||
+ handle_failed_tests
+- group "Run unit tests" \
+- make DEFAULT_UNIT_TEST_TARGET=unit-tests-prove unit-tests
+ fi
+ check_unignored_build_artifacts
+
+
## t/Makefile ##
@@ t/Makefile: failed:
test -z "$$failed" || $(MAKE) $$failed
-: ---------- > 7: cbf37e0ddc ci: use test-tool as unit test runner on Windows
base-commit: 4904a4d08cc085716df12ce713ae7ee3d5ecb75a
--
2.44.0.rc0.258.g7320e95886-goog
^ permalink raw reply
* Re: [RFC PATCH v2 4/6] test-tool run-command testsuite: support unit tests
From: Josh Steadmon @ 2024-02-23 22:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, johannes.schindelin, peff, phillip.wood
In-Reply-To: <xmqqbk8s80hh.fsf@gitster.g>
On 2024.02.07 12:48, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
>
> > Teach the testsuite runner in `test-tool run-command testsuite` how to
> > run unit tests: if TEST_SHELL_PATH is not set, assume that we're running
> > the programs directly from CWD, rather than defaulting to "sh" as an
> > interpreter.
>
> Hmph. It sounds more like "the run-command testsuite subcommand
> only runs programs in the current directory", not "assume" (which
> implies there is a way to override the assumption). Not that the
> limitation would hurt us in any way, though.
>
> > + /*
> > + * If we run without a shell, we have to provide the relative path to
> > + * the executables.
> > + */
>
> It sounds more like "If TEST_SHELL_PATH is not given, then we run
> them in the current directory.".
Reworded both of these in V3.
^ permalink raw reply
* Re: [PATCH v2] Add unix domain socket support to HTTP transport
From: Leslie Cheng @ 2024-02-23 22:24 UTC (permalink / raw)
To: Junio C Hamano, Leslie Cheng via GitGitGadget
Cc: git, Eric Wong, Leslie Cheng
In-Reply-To: <xmqq5xyfyyn0.fsf@gitster.g>
On 2024-02-23 12:37 a.m., Junio C Hamano wrote:
> How about following that convention, perhaps like:
>
> In some corporate environments, the proxy server listens to a
> local unix domain socket for requests, instead of listening to a
> network port. Even though we have http.proxy (and more
> destination specific http.<url>.proxy) configuration variables
> to specify the network address/port of a proxy, that would not
> help if your proxy does not listen to the network.
>
> Introduce an `http.unixSocket` (and `http.<url>.unixSocket`)
> configuration variables that specify the path to a unix domain
> socket for such a proxy. Recent versions of libcURL library
> added CURLOPT_UNIX_SOCKET_PATH to support "curl --unix-socket
> <path>"---use the same mechanism to implement it.
This is excellent, thanks for the guidance (and all the other
suggestions prior)! I'll update in the next patch.
> Unlike NO_UNIX_SOCKETS, GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH is
> entirely internal to your implementation and not surfaced to neither
> the end-users or the binary packagers. Because of that, I suspect
> that any description that has to use that name probably falls on the
> other side of "too much implementation details" to be useful to help
> future developers..
That's reasonable, I figured it would fit as a cover letter detail but I
agree it's not relevant as a commit message that lives in the history of
the project. I'll also update this in the next patch.
> Talking about precedence between this and http.proxy is good thing,
> but one very important piece of information is missing. What value
> does it take?
>
> The absolute path of a unix-domain socket to pass the HTTP
> traffic over, instead of using the network.
>
> or something, perhaps?
I like that wording, I'll update in the next patch.
> It might make the code easier to follow if you did:
>
> #if !defined(NO_CURLOPT_UNIX_SOCKET_PATH) && !defined(NO_UNIX_SOCKETS)
> #if defined(GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH)
> #define USE_CURLOPT_UNIX_SOCKET_PATH
> #endif
> #endif
>
> The points are
>
> (1) the users can decline to use CURLOPT_UNIX_SOCKET_PATH while
> still using unix domain socket for other purposes, and
>
> (2) you do not have to care if you HAVE it or not most of time;
> what matters more often is if the user told you to USE it.
>
> Hmm?
Do you think this functionality is worth adding another macro to
conditionally include it in the build? It felt out-of-the-way enough
that we could just use the same `NO_UNIX_SOCKETS` macro to control for
environments that don't support unix domain sockets.
>> +#if defined(GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH) &&
!defined(NO_UNIX_SOCKETS)
>> +static const char *curl_unix_socket_path;
>> +#endif
>
> The guard here would become "#ifdef USE_CURLOPT_UNIX_SOCKET_PATH" if
> we wanted this to be conditional, but I think it is easier to make
> the variable unconditionally available; see below.
Agreed in general, I was looking to other patterns for conditional
variables in file, e.g.
https://github.com/git/git/blob/3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0/http.c#L66-L68
But, revisiting, this looks like an exception rather than the norm.
> In general, it is inadvisable to issue a warning in the codepath
> that parses configuration variables, as the values we read may not
> be necessarily used. We could instead accept the given path into a
> variable unconditionally, and complain only before it gets used,
> near the call to curl_easy_setopt().
Similar to above, I followed what was already done for certain
configuration variables (e.g.
https://github.com/git/git/blob/3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0/http.c#L485-L501),
but I agree with your feedback since this would result in constant warnings.
To summarize, I'll do the following in the next patch:
- change the wording of the commit message
- move the conditional variables and parsing to a check at
`curl_easy_setopt()` time
I'm still undecided on whether I should introduce another macro
specifically for this functionality, and I'd like to hear your thoughts
on why it might make sense.
^ permalink raw reply
* Re: [PATCH] Documentation/config/pack.txt: fix broken AsciiDoc
From: Junio C Hamano @ 2024-02-23 22:15 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Jeff King
In-Reply-To: <72bb58e5f3b8a5a622394c5ff40426156e122580.1708720255.git.me@ttaylorr.com>
Taylor Blau <me@ttaylorr.com> writes:
> diff --git a/Documentation/config/pack.txt b/Documentation/config/pack.txt
> index 9c630863e6..da527377fa 100644
> --- a/Documentation/config/pack.txt
> +++ b/Documentation/config/pack.txt
> @@ -34,11 +34,10 @@ pack.allowPackReuse::
> reachability bitmap is available, pack-objects will try to send
> parts of all packs in the MIDX.
> +
> - If only a single pack bitmap is available, and
> - `pack.allowPackReuse` is set to "multi", reuse parts of just the
> - bitmapped packfile. This can reduce memory and CPU usage to
> - serve fetches, but might result in sending a slightly larger
> - pack. Defaults to true.
> +If only a single pack bitmap is available, and `pack.allowPackReuse`
> +is set to "multi", reuse parts of just the bitmapped packfile. This
> +can reduce memory and CPU usage to serve fetches, but might result in
> +sending a slightly larger pack. Defaults to true.
Thanks.
^ permalink raw reply
* Re: Interactive rebase: using "pick" for merge commits
From: Stefan Haller @ 2024-02-23 20:59 UTC (permalink / raw)
To: phillip.wood, Patrick Steinhardt; +Cc: git
In-Reply-To: <040f142c-7ee2-429e-88eb-d328b01a4b8c@gmail.com>
On 12.02.24 15:38, Phillip Wood wrote:
> Hi Patrick and Stefan
>
> On 12/02/2024 07:15, Patrick Steinhardt wrote:
>> On Sat, Feb 10, 2024 at 10:23:16AM +0100, Stefan Haller wrote:
>>> On 09.02.24 17:24, Phillip Wood wrote:
>>> Yes, I'm familiar with all this, but that's not what I mean. I don't
>>> want to maintain the topology here, and I'm also not suggesting that git
>>> itself generates such "pick" entries with -mX arguments (maybe I wasn't
>>> clear on that). What I want to do is to add such entries myself, as a
>>> user, resulting in the equivalent of doing a "break" at that point in
>>> the rebase and doing a "git cherry-pick -mX <hash-of-merge-commit>"
>>> manually.
>>
>> It would be neat indeed if this could be specified in the instruction
>> sheet. We already support options for the "merge" instruction, so
>> extending "pick" to support options isn't that far-fetched. Then it
>> would become possible to say "pick -m1 fa1afe1".
>
> It would certainly be possible to extend the sequencer to do that but
> I'm not familiar with why people use "git cherry-pick -m" [1] so I'm
> wondering what this would be used for. It would involve a bit of extra
> complexity so I think we'd want a compelling reason as to why
> cherry-picking merges without maintaining the topology is useful
> especially as one can currently do that via "exec git cherry-pick -m ..."
Ok, I suppose the answer will probably not count as a compelling reason.
My reason for wanting this is that lazygit currently implements
cherry-picking in terms of an interactive rebase, rather then calling
git-cherry-pick. And the reason why it does this is that when you
cherry-pick multiple commits, and one of them conflicts, then you get
lazygit's nice visualization of the rebase todo list to show you where
in the sequence you are, what the conflicting commit is, how many are
left etc. It just happens to support this well for
.git/rebase-merge/git-rebase-todo, but not for .git/sequencer/todo.
It probably makes more sense to teach lazygit to visualize the
.git/sequencer/todo file, and then use git cherry-pick.
See https://github.com/jesseduffield/lazygit/issues/3317
-Stefan
^ permalink raw reply
* [ANNOUNCE] Git for Windows 2.44.0
From: Johannes Schindelin @ 2024-02-23 20:57 UTC (permalink / raw)
To: git-for-windows, git, git-packagers; +Cc: Johannes Schindelin
Dear Git users,
I hereby announce that Git for Windows 2.44.0 is available from:
https://gitforwindows.org/
Changes since Git for Windows v2.43.0 (November 20th 2023)
Git for Windows for Windows v2.44 is the last version to support for
Windows 7 and for Windows 8, see MSYS2's corresponding deprecation
announcement (Git for Windows relies on MSYS2 for components such as
Bash and Perl).
Please also note that the 32-bit variant of Git for Windows is
deprecated; Its last official release is planned for 2025.
New Features
* Comes with Git v2.44.0.
* Comes with libfido2 v1.14.0.
* Comes with the MSYS2 runtime (Git for Windows flavor) based on
Cygwin v3.4.10.
* Comes with Perl v5.38.2.
* Git for Windows learned to detect and use native Windows support
for ANSI sequences, which allows using 24-bit colors in terminal
windows.
* Comes with Git LFS v3.4.1.
* The repository viewer Tig that is included in Git for Windows can
now be called also directly from PowerShell/CMD.
* Comes with OpenSSH v9.6.P1.
* Comes with Bash v5.2.26.
* Comes with GNU TLS v3.8.3.
* Comes with OpenSSL v3.2.1.
* Comes with cURL v8.6.0.
* Comes with GNU Privacy Guard v2.4.4.
Bug Fixes
* The 32-bit variant of Git for Windows was missing some MSYS2
runtime updates, which was addressed; Do note 32-bit support is
phased out.
* The Git for Windows installer showed cut-off text in some setups.
This has been fixed.
* The git credential-manager --help command previously would not find
a page to display in the web browser, which has been fixed.
* A couple of bugs that could cause Git Bash to hang in certain
scenarios were fixed.
Git-2.44.0-64-bit.exe | 914ffc96cee0631d09049b9d87d4cd8ac9c98ead9a9f9a094d3341348324a9ec
Git-2.44.0-32-bit.exe | 5ba23d73e861d872416175ac6a05304875d6ec420c08d0217329580ca1ea0fff
PortableGit-2.44.0-64-bit.7z.exe | 1fc64ca91b9b475ab0ada72c9f7b3addbe69a6c8f520be31425cf21841cca369
PortableGit-2.44.0-32-bit.7z.exe | e70c80672069907961f6db68b0db5e14ea0447f39c74cfd3c385882f3b934c6f
MinGit-2.44.0-64-bit.zip | ed4e74e171c59c9c9d418743c7109aa595e0cc0d1c80cac574d69ed5e571ae59
MinGit-2.44.0-32-bit.zip | 3c946898cd78c5106b1672dd80051953bdb245fb46352a70606f271d8b0233c7
MinGit-2.44.0-busybox-64-bit.zip | a2377f6e4214f16afa1a5a23d9a291d09a2234bcac67c5aeb36d9cce4b7b4d5b
MinGit-2.44.0-busybox-32-bit.zip | 83dd7903f8a4b2a035eda510d6d1394acc9ff36ce45b9e55efd7dd48c83471a4
Git-2.44.0-64-bit.tar.bz2 | d78c40d768eb7af7e14d5cd47dac89a2e50786c89a67be6249e1a041ae5eb20d
Git-2.44.0-32-bit.tar.bz2 | 14541119fe97b4d34126ee136cbdba8da171b8cbd42543185a259128a3eed6b3
Ciao,
Johannes
^ permalink raw reply
* [PATCH] Documentation/config/pack.txt: fix broken AsciiDoc
From: Taylor Blau @ 2024-02-23 20:30 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In af626ac0e0 (pack-bitmap: enable reuse from all bitmapped packs,
2023-12-14), the documentation for `pack.allowPackReuse` was amended to
include its effect when set to "multi".
This split the documentation into two paragraphs, but did not de-dent
the second paragraph on the right-hand side of a line-continuation
marker. This causes the rendered documentation to appear oddly, where
the second paragraph is treated as a <pre> block when rendered as HTML.
Fix this by correctly removing the indentation on the second paragraph.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
Documentation/config/pack.txt | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/Documentation/config/pack.txt b/Documentation/config/pack.txt
index 9c630863e6..da527377fa 100644
--- a/Documentation/config/pack.txt
+++ b/Documentation/config/pack.txt
@@ -34,11 +34,10 @@ pack.allowPackReuse::
reachability bitmap is available, pack-objects will try to send
parts of all packs in the MIDX.
+
- If only a single pack bitmap is available, and
- `pack.allowPackReuse` is set to "multi", reuse parts of just the
- bitmapped packfile. This can reduce memory and CPU usage to
- serve fetches, but might result in sending a slightly larger
- pack. Defaults to true.
+If only a single pack bitmap is available, and `pack.allowPackReuse`
+is set to "multi", reuse parts of just the bitmapped packfile. This
+can reduce memory and CPU usage to serve fetches, but might result in
+sending a slightly larger pack. Defaults to true.
pack.island::
An extended regular expression configuring a set of delta
--
2.44.0.rc2.25.g54c9b1361db
^ permalink raw reply related
* Re: [PATCH v5 0/5] for-each-ref: add '--include-root-refs' option
From: Junio C Hamano @ 2024-02-23 20:13 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, ps, phillip.wood123
In-Reply-To: <xmqqzfvrrpju.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Changes from v4:
>> 1. Fixed erratic whitespace
>> 2. Remove braces from single line block
>> 3. Starting the comments with a capital and also adding more context.
>> 4. Removed a duplicate check.
>
> Does #4 refer to this removal?
>
> if (filter->kind == FILTER_REFS_KIND_MASK && kind == FILTER_REFS_DETACHED_HEAD)
> kind = FILTER_REFS_PSEUDOREFS;
> else if (!(kind & filter->kind))
> return NULL;
>
> - if (!(kind & filter->kind))
> - return NULL;
> -
> if (!filter_pattern_match(filter, refname))
> return NULL;
>
>
> If filter->kind is MASK and kind is set to filter detached HEAD, we
> assign to and change the value of kind to FILTER_REFS_PSEUDOREFS,
> so it is unclear if the freestanding "if kind and filter->kind does
> not overlap, return NULL" was redundant or outright buggy.
Ah, of course this is simply redundant. The assignment to limit the
kind to PSEUDOREFS happens only when filter->kind has all bits, and
after assigning a different non-zero value to kind, (kind & filter->kind)
will still overlap, so a freestanding "if no overlap between kind and
filter-kind then return NULL" will not trigger for such a case.
Thanks.
^ permalink raw reply
* Re: [PATCH] unit-tests: convert t/helper/test-oid-array.c to unit-tests
From: Ghanshyam Thakkar @ 2024-02-23 19:37 UTC (permalink / raw)
To: Ghanshyam Thakkar, git
In-Reply-To: <20240223193257.9222-1-shyamthakkar001@gmail.com>
On Sat Feb 24, 2024 at 1:02 AM IST, Ghanshyam Thakkar wrote:
> [RFC]: I recently saw a series by Eric W. Biederman [1] which enables
> the use of oid's with different hash algorithms into the same
> oid_array safely. However, there were no tests added for this. So, I
> am wondering if we should have a input format which allows us to
> specify hash algo for each oid with its hex value. i.e. "sha1:55" or
> "sha256:55", instead of just "55" and relying on GIT_TEST_DEFAULT_HASH
> for algo. So far, I tried to imitate the existing tests but I suppose
> this may be useful in the future if that series gets merged.
[1]:
https://lore.kernel.org/git/20231002024034.2611-2-ebiederm@gmail.com/
apologies for the noise.
^ permalink raw reply
* [PATCH] unit-tests: convert t/helper/test-oid-array.c to unit-tests
From: Ghanshyam Thakkar @ 2024-02-23 19:32 UTC (permalink / raw)
To: git; +Cc: Ghanshyam Thakkar
Migrate t/helper/test-oid-array.c and t/t0064-oid-array.sh to the
recently added unit testing framework. This would improve runtime
performance and provide better debugging via displaying array content,
index at which the test failed etc. directly to stdout.
There is only one change in the new testing approach. In the previous
testing method, a new repo gets initialized for the test according to
GIT_TEST_DEFAULT_HASH algorithm. In unit testing however, we do not
need to initialize the repo. We can set the length of the hexadecimal
strbuf according to the algorithm used directly.
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
[RFC]: I recently saw a series by Eric W. Biederman [1] which enables
the use of oid's with different hash algorithms into the same
oid_array safely. However, there were no tests added for this. So, I
am wondering if we should have a input format which allows us to
specify hash algo for each oid with its hex value. i.e. "sha1:55" or
"sha256:55", instead of just "55" and relying on GIT_TEST_DEFAULT_HASH
for algo. So far, I tried to imitate the existing tests but I suppose
this may be useful in the future if that series gets merged.
Makefile | 2 +-
t/helper/test-oid-array.c | 45 --------
t/helper/test-tool.c | 1 -
t/helper/test-tool.h | 1 -
t/t0064-oid-array.sh | 104 -----------------
t/unit-tests/t-oid-array.c | 222 +++++++++++++++++++++++++++++++++++++
6 files changed, 223 insertions(+), 152 deletions(-)
delete mode 100644 t/helper/test-oid-array.c
delete mode 100755 t/t0064-oid-array.sh
create mode 100644 t/unit-tests/t-oid-array.c
diff --git a/Makefile b/Makefile
index 78e874099d..5060d7eff3 100644
--- a/Makefile
+++ b/Makefile
@@ -820,7 +820,6 @@ TEST_BUILTINS_OBJS += test-lazy-init-name-hash.o
TEST_BUILTINS_OBJS += test-match-trees.o
TEST_BUILTINS_OBJS += test-mergesort.o
TEST_BUILTINS_OBJS += test-mktemp.o
-TEST_BUILTINS_OBJS += test-oid-array.o
TEST_BUILTINS_OBJS += test-oidmap.o
TEST_BUILTINS_OBJS += test-oidtree.o
TEST_BUILTINS_OBJS += test-online-cpus.o
@@ -1346,6 +1345,7 @@ UNIT_TEST_PROGRAMS += t-mem-pool
UNIT_TEST_PROGRAMS += t-strbuf
UNIT_TEST_PROGRAMS += t-ctype
UNIT_TEST_PROGRAMS += t-prio-queue
+UNIT_TEST_PROGRAMS += t-oid-array
UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
diff --git a/t/helper/test-oid-array.c b/t/helper/test-oid-array.c
deleted file mode 100644
index aafe398ef0..0000000000
--- a/t/helper/test-oid-array.c
+++ /dev/null
@@ -1,45 +0,0 @@
-#include "test-tool.h"
-#include "hex.h"
-#include "oid-array.h"
-#include "setup.h"
-#include "strbuf.h"
-
-static int print_oid(const struct object_id *oid, void *data UNUSED)
-{
- puts(oid_to_hex(oid));
- return 0;
-}
-
-int cmd__oid_array(int argc UNUSED, const char **argv UNUSED)
-{
- struct oid_array array = OID_ARRAY_INIT;
- struct strbuf line = STRBUF_INIT;
- int nongit_ok;
-
- setup_git_directory_gently(&nongit_ok);
-
- while (strbuf_getline(&line, stdin) != EOF) {
- const char *arg;
- struct object_id oid;
-
- if (skip_prefix(line.buf, "append ", &arg)) {
- if (get_oid_hex(arg, &oid))
- die("not a hexadecimal oid: %s", arg);
- oid_array_append(&array, &oid);
- } else if (skip_prefix(line.buf, "lookup ", &arg)) {
- if (get_oid_hex(arg, &oid))
- die("not a hexadecimal oid: %s", arg);
- printf("%d\n", oid_array_lookup(&array, &oid));
- } else if (!strcmp(line.buf, "clear"))
- oid_array_clear(&array);
- else if (!strcmp(line.buf, "for_each_unique"))
- oid_array_for_each_unique(&array, print_oid, NULL);
- else
- die("unknown command: %s", line.buf);
- }
-
- strbuf_release(&line);
- oid_array_clear(&array);
-
- return 0;
-}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 482a1e58a4..ad74bfffbe 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -42,7 +42,6 @@ static struct test_cmd cmds[] = {
{ "match-trees", cmd__match_trees },
{ "mergesort", cmd__mergesort },
{ "mktemp", cmd__mktemp },
- { "oid-array", cmd__oid_array },
{ "oidmap", cmd__oidmap },
{ "oidtree", cmd__oidtree },
{ "online-cpus", cmd__online_cpus },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index b1be7cfcf5..4f961a38c0 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -65,7 +65,6 @@ int cmd__scrap_cache_tree(int argc, const char **argv);
int cmd__serve_v2(int argc, const char **argv);
int cmd__sha1(int argc, const char **argv);
int cmd__sha1_is_sha1dc(int argc, const char **argv);
-int cmd__oid_array(int argc, const char **argv);
int cmd__sha256(int argc, const char **argv);
int cmd__sigchain(int argc, const char **argv);
int cmd__simple_ipc(int argc, const char **argv);
diff --git a/t/t0064-oid-array.sh b/t/t0064-oid-array.sh
deleted file mode 100755
index 88c89e8f48..0000000000
--- a/t/t0064-oid-array.sh
+++ /dev/null
@@ -1,104 +0,0 @@
-#!/bin/sh
-
-test_description='basic tests for the oid array implementation'
-
-TEST_PASSES_SANITIZE_LEAK=true
-. ./test-lib.sh
-
-echoid () {
- prefix="${1:+$1 }"
- shift
- while test $# -gt 0
- do
- echo "$prefix$ZERO_OID" | sed -e "s/00/$1/g"
- shift
- done
-}
-
-test_expect_success 'ordered enumeration' '
- echoid "" 44 55 88 aa >expect &&
- {
- echoid append 88 44 aa 55 &&
- echo for_each_unique
- } | test-tool oid-array >actual &&
- test_cmp expect actual
-'
-
-test_expect_success 'ordered enumeration with duplicate suppression' '
- echoid "" 44 55 88 aa >expect &&
- {
- echoid append 88 44 aa 55 &&
- echoid append 88 44 aa 55 &&
- echoid append 88 44 aa 55 &&
- echo for_each_unique
- } | test-tool oid-array >actual &&
- test_cmp expect actual
-'
-
-test_expect_success 'lookup' '
- {
- echoid append 88 44 aa 55 &&
- echoid lookup 55
- } | test-tool oid-array >actual &&
- n=$(cat actual) &&
- test "$n" -eq 1
-'
-
-test_expect_success 'lookup non-existing entry' '
- {
- echoid append 88 44 aa 55 &&
- echoid lookup 33
- } | test-tool oid-array >actual &&
- n=$(cat actual) &&
- test "$n" -lt 0
-'
-
-test_expect_success 'lookup with duplicates' '
- {
- echoid append 88 44 aa 55 &&
- echoid append 88 44 aa 55 &&
- echoid append 88 44 aa 55 &&
- echoid lookup 55
- } | test-tool oid-array >actual &&
- n=$(cat actual) &&
- test "$n" -ge 3 &&
- test "$n" -le 5
-'
-
-test_expect_success 'lookup non-existing entry with duplicates' '
- {
- echoid append 88 44 aa 55 &&
- echoid append 88 44 aa 55 &&
- echoid append 88 44 aa 55 &&
- echoid lookup 66
- } | test-tool oid-array >actual &&
- n=$(cat actual) &&
- test "$n" -lt 0
-'
-
-test_expect_success 'lookup with almost duplicate values' '
- # n-1 5s
- root=$(echoid "" 55) &&
- root=${root%5} &&
- {
- id1="${root}5" &&
- id2="${root}f" &&
- echo "append $id1" &&
- echo "append $id2" &&
- echoid lookup 55
- } | test-tool oid-array >actual &&
- n=$(cat actual) &&
- test "$n" -eq 0
-'
-
-test_expect_success 'lookup with single duplicate value' '
- {
- echoid append 55 55 &&
- echoid lookup 55
- } | test-tool oid-array >actual &&
- n=$(cat actual) &&
- test "$n" -ge 0 &&
- test "$n" -le 1
-'
-
-test_done
diff --git a/t/unit-tests/t-oid-array.c b/t/unit-tests/t-oid-array.c
new file mode 100644
index 0000000000..b4f43c025d
--- /dev/null
+++ b/t/unit-tests/t-oid-array.c
@@ -0,0 +1,222 @@
+#include "test-lib.h"
+#include "hex.h"
+#include "oid-array.h"
+#include "strbuf.h"
+
+#define INPUT "88", "44", "aa", "55"
+#define INPUT_DUP \
+ "88", "44", "aa", "55", "88", "44", "aa", "55", "88", "44", "aa", "55"
+#define INPUT_ONLY_DUP "55", "55"
+#define ENUMERATION_RESULT_SORTED "44", "55", "88", "aa"
+
+/*
+ * allocates the memory based on the hash algorithm used and sets the length to
+ * it.
+ */
+static void hex_strbuf_init(struct strbuf *hex)
+{
+ static int sz = -1;
+
+ if (sz == -1) {
+ char *algo_env = getenv("GIT_TEST_DEFAULT_HASH");
+ if (algo_env && !strcmp(algo_env, "sha256"))
+ sz = GIT_SHA256_HEXSZ;
+ else
+ sz = GIT_SHA1_HEXSZ;
+ }
+
+ strbuf_init(hex, sz);
+ strbuf_setlen(hex, sz);
+}
+
+/* callback function for for_each used for printing */
+static int print_cb(const struct object_id *oid, void *data)
+{
+ int *i = data;
+ test_msg("%d. %s", *i, oid_to_hex(oid));
+ *i += 1;
+ return 0;
+}
+
+/* prints the oid_array with a message title */
+static void print_oid_array(struct oid_array *array, char *msg)
+{
+ int i = 0;
+ test_msg("%s", msg);
+ oid_array_for_each(array, print_cb, &i);
+}
+
+/* fills the hex strbuf with alternating characters from 'c' */
+static void fill_hex_strbuf(struct strbuf *hex, char *c)
+{
+ size_t i;
+ for (i = 0; i < hex->len; i++)
+ hex->buf[i] = (i & 1) ? c[1] : c[0];
+}
+
+/* populates object_id with hexadecimal representation generated from 'c' */
+static int get_oid_hex_input(struct object_id *oid, char *c)
+{
+ int ret;
+ struct strbuf hex;
+
+ hex_strbuf_init(&hex);
+ fill_hex_strbuf(&hex, c);
+ ret = get_oid_hex_any(hex.buf, oid);
+ if (ret == GIT_HASH_UNKNOWN)
+ test_msg("not a valid hexadecimal oid: %s", hex.buf);
+ strbuf_release(&hex);
+ return ret;
+}
+
+/* populates the oid_array with input from entries array */
+static int populate_oid_array(struct oid_array *oidarray, char **entries,
+ size_t len)
+{
+ size_t i;
+ struct object_id oid;
+
+ for (i = 0; i < len; i++) {
+ if (!check_int(get_oid_hex_input(&oid, entries[i]), !=,
+ GIT_HASH_UNKNOWN))
+ return -1;
+ oid_array_append(oidarray, &oid);
+ }
+ return 0;
+}
+
+/* callback function for enumeration test */
+static int add_to_oid_array(const struct object_id *oid, void *data)
+{
+ struct oid_array *array = data;
+ oid_array_append(array, oid);
+ return 0;
+}
+
+static void test_enumeration(char *input_entries[], size_t input_size,
+ char *expected_entries[], size_t expected_size)
+{
+ int i;
+ struct oid_array input = OID_ARRAY_INIT;
+ struct oid_array actual = OID_ARRAY_INIT;
+ struct oid_array expect = OID_ARRAY_INIT;
+
+ if (populate_oid_array(&input, input_entries, input_size) == -1)
+ goto cleanup;
+
+ if (populate_oid_array(&expect, expected_entries, expected_size) == -1)
+ goto cleanup;
+
+ oid_array_for_each_unique(&input, add_to_oid_array, &actual);
+ if (!check_int(expect.nr, ==, expected_size) ||
+ !check_int(actual.nr, ==, expected_size))
+ goto cleanup;
+
+ for (i = 0; i < expected_size; i++) {
+ if (!check_int(oideq(&actual.oid[i], &expect.oid[i]), ==, 1)) {
+ test_msg("failed at index %d", i);
+ print_oid_array(&expect, "expected array content:");
+ print_oid_array(&actual, "actual array content:");
+ goto cleanup;
+ }
+ }
+
+cleanup:
+ oid_array_clear(&input);
+ oid_array_clear(&actual);
+ oid_array_clear(&expect);
+}
+
+#define ENUMERATION_INPUT(INPUT, RESULT, NAME) \
+ static void t_ordered_enumeration_##NAME(void) \
+ { \
+ char *input_entries[] = { INPUT }; \
+ char *expected_entries[] = { RESULT }; \
+ size_t input_size = ARRAY_SIZE(input_entries); \
+ size_t expected_size = ARRAY_SIZE(expected_entries); \
+ test_enumeration(input_entries, input_size, expected_entries, \
+ expected_size); \
+ }
+
+ENUMERATION_INPUT(INPUT, ENUMERATION_RESULT_SORTED, non_duplicate)
+ENUMERATION_INPUT(INPUT_DUP, ENUMERATION_RESULT_SORTED, duplicate)
+
+static void lookup_setup(void (*f)(struct strbuf *buf, struct oid_array *array))
+{
+ struct oid_array array = OID_ARRAY_INIT;
+ struct strbuf buf;
+
+ hex_strbuf_init(&buf);
+ f(&buf, &array);
+ oid_array_clear(&array);
+ strbuf_release(&buf);
+}
+
+#define LOOKUP_INPUT(INPUT, QUERY, NAME, CONDITION) \
+ static void t_##NAME(struct strbuf *buf UNUSED, \
+ struct oid_array *array) \
+ { \
+ struct object_id oid_query; \
+ char *input_entries[] = { INPUT }; \
+ size_t input_size = ARRAY_SIZE(input_entries); \
+ int ret; \
+ if (!check_int(get_oid_hex_input(&oid_query, QUERY), !=, \
+ GIT_HASH_UNKNOWN)) \
+ return; \
+ if (!check_int(populate_oid_array(array, input_entries, \
+ input_size), \
+ !=, -1)) \
+ return; \
+ ret = oid_array_lookup(array, &oid_query); \
+ if (!check(CONDITION)) { \
+ print_oid_array(array, "array content:"); \
+ test_msg("oid query for lookup: %s", oid_query.hash); \
+ } \
+ }
+
+/* ret is return value of oid_array_lookup() */
+LOOKUP_INPUT(INPUT, "55", lookup, ret == 1)
+LOOKUP_INPUT(INPUT, "33", lookup_nonexist, ret < 1)
+LOOKUP_INPUT(INPUT_DUP, "66", lookup_nonexist_dup, ret < 0)
+LOOKUP_INPUT(INPUT_DUP, "55", lookup_dup, ret >= 3 && ret <= 5)
+LOOKUP_INPUT(INPUT_ONLY_DUP, "55", lookup_only_dup, ret >= 0 && ret <= 1)
+
+static void t_lookup_almost_dup(struct strbuf *hex, struct oid_array *array)
+{
+ struct object_id oid;
+
+ fill_hex_strbuf(hex, "55");
+ if (!check_int(get_oid_hex_any(hex->buf, &oid), !=, GIT_HASH_UNKNOWN))
+ return;
+
+ oid_array_append(array, &oid);
+ /* last character different */
+ hex->buf[hex->len - 1] = 'f';
+ if (!check_int(get_oid_hex_any(hex->buf, &oid), !=, GIT_HASH_UNKNOWN))
+ return;
+
+ oid_array_append(array, &oid);
+ if (!check_int(oid_array_lookup(array, &oid), ==, 1)) {
+ print_oid_array(array, "array content:");
+ test_msg("oid query for lookup: %s", hex->buf);
+ }
+}
+
+int cmd_main(int argc, const char **argv)
+{
+ TEST(t_ordered_enumeration_non_duplicate(),
+ "ordered enumeration works");
+ TEST(t_ordered_enumeration_duplicate(),
+ "ordered enumeration with duplicate suppresion works");
+ TEST(lookup_setup(t_lookup), "lookup works");
+ TEST(lookup_setup(t_lookup_nonexist), "lookup non-existant entry");
+ TEST(lookup_setup(t_lookup_dup), "lookup with duplicates works");
+ TEST(lookup_setup(t_lookup_nonexist_dup),
+ "lookup non-existant entry with duplicates");
+ TEST(lookup_setup(t_lookup_almost_dup),
+ "lookup with almost duplicate values works");
+ TEST(lookup_setup(t_lookup_only_dup),
+ "lookup with single duplicate value works");
+
+ return test_done();
+}
--
2.43.2
^ permalink raw reply related
* Re: [PATCH v5 0/5] for-each-ref: add '--include-root-refs' option
From: Junio C Hamano @ 2024-02-23 18:41 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, ps, phillip.wood123
In-Reply-To: <20240223100112.44127-1-karthik.188@gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> Changes from v4:
> 1. Fixed erratic whitespace
> 2. Remove braces from single line block
> 3. Starting the comments with a capital and also adding more context.
> 4. Removed a duplicate check.
Does #4 refer to this removal?
if (filter->kind == FILTER_REFS_KIND_MASK && kind == FILTER_REFS_DETACHED_HEAD)
kind = FILTER_REFS_PSEUDOREFS;
else if (!(kind & filter->kind))
return NULL;
- if (!(kind & filter->kind))
- return NULL;
-
if (!filter_pattern_match(filter, refname))
return NULL;
If filter->kind is MASK and kind is set to filter detached HEAD, we
assign to and change the value of kind to FILTER_REFS_PSEUDOREFS,
so it is unclear if the freestanding "if kind and filter->kind does
not overlap, return NULL" was redundant or outright buggy.
The hunk just stood out to me, but I haven't read other parts of the
series yet.
Thanks.
^ permalink raw reply
* Re: [PATCH v4 0/6] merge-tree: handle missing objects correctly
From: Junio C Hamano @ 2024-02-23 18:23 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Patrick Steinhardt, Eric Sunshine, Johannes Schindelin
In-Reply-To: <pull.1651.v4.git.1708677266.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> I recently looked into issues where git merge-tree calls returned bogus data
> (in one instance returning an empty tree for non-empty merge parents). By
> the time I had a look at the corresponding repository, the issue was no
> longer reproducible, but a closer look at the code combined with some manual
> experimenting turned up the fact that missing tree objects aren't handled as
> errors by git merge-tree.
Looking good. Will replace. Thanks.
^ permalink raw reply
* Re: [PATCH v2 15/16] fsmonitor: refactor bit invalidation in refresh callback
From: Junio C Hamano @ 2024-02-23 18:18 UTC (permalink / raw)
To: Jeff Hostetler via GitGitGadget
Cc: git, Patrick Steinhardt, Jeff Hostetler, Jeff Hostetler
In-Reply-To: <3a20065dbf80eabfc62c0bdebc16df0b5a4c7b02.1708658300.git.gitgitgadget@gmail.com>
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Jeff Hostetler <jeffhostetler@github.com>
>
> Refactor code in the fsmonitor_refresh_callback() call chain dealing
> with invalidating the CE_FSMONITOR_VALID bit and add a trace message.
>
> During the refresh, we clear the CE_FSMONITOR_VALID bit in response to
> data from the FSMonitor daemon (so that a later phase will lstat() and
> verify the true state of the file).
>
> Create a new function to clear the bit and add some unique tracing for
> it to help debug edge cases.
>
> This is similar to the existing `mark_fsmonitor_invalid()` function,
> but we don't need the extra stuff that it does.
>
> Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
> ---
> fsmonitor.c | 20 +++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/fsmonitor.c b/fsmonitor.c
> index ac638a61c00..0667a8c297c 100644
> --- a/fsmonitor.c
> +++ b/fsmonitor.c
> @@ -187,6 +187,20 @@ static int query_fsmonitor_hook(struct repository *r,
> static size_t handle_path_with_trailing_slash(
> struct index_state *istate, const char *name, int pos);
>
> +/*
> + * Invalidate the FSM bit on this CE. This is like mark_fsmonitor_invalid()
> + * but we've already handled the untracked-cache and I want a different
> + * trace message.
> + */
"I want" -> "want" perhaps.
More importantly, when new developers come and want to touch this
file in the future, how would they choose which one to call? Would
it make a better comment if we rewrote the above for such future
developers as intended audiences?
> +static void invalidate_ce_fsm(struct cache_entry *ce)
> +{
> + if (ce->ce_flags & CE_FSMONITOR_VALID)
> + trace_printf_key(&trace_fsmonitor,
> + "fsmonitor_refresh_callback INV: '%s'",
> + ce->name);
> + ce->ce_flags &= ~CE_FSMONITOR_VALID;
> +}
> +
> /*
> * Use the name-hash to do a case-insensitive cache-entry lookup with
> * the pathname and invalidate the cache-entry.
> @@ -224,7 +238,7 @@ static size_t handle_using_name_hash_icase(
>
> untracked_cache_invalidate_trimmed_path(istate, ce->name, 0);
>
> - ce->ce_flags &= ~CE_FSMONITOR_VALID;
> + invalidate_ce_fsm(ce);
> return 1;
> }
>
> @@ -316,7 +330,7 @@ static size_t handle_path_without_trailing_slash(
> * cache-entry with the same pathname, nor for a cone
> * at that directory. (That is, assume no D/F conflicts.)
> */
> - istate->cache[pos]->ce_flags &= ~CE_FSMONITOR_VALID;
> + invalidate_ce_fsm(istate->cache[pos]);
> return 1;
> } else {
> size_t nr_in_cone;
> @@ -394,7 +408,7 @@ static size_t handle_path_with_trailing_slash(
> for (i = pos; i < istate->cache_nr; i++) {
> if (!starts_with(istate->cache[i]->name, name))
> break;
> - istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
> + invalidate_ce_fsm(istate->cache[i]);
> nr_in_cone++;
> }
Nice.
^ permalink raw reply
* Re: [PATCH v2 14/16] fsmonitor: support case-insensitive events
From: Junio C Hamano @ 2024-02-23 18:14 UTC (permalink / raw)
To: Jeff Hostetler via GitGitGadget
Cc: git, Patrick Steinhardt, Jeff Hostetler, Jeff Hostetler
In-Reply-To: <288f3f4e54e98a68d72e97125b1520605c138c3c.1708658300.git.gitgitgadget@gmail.com>
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> +/*
> + * Use the name-hash to do a case-insensitive cache-entry lookup with
> + * the pathname and invalidate the cache-entry.
> + *
> + * Returns the number of cache-entries that we invalidated.
> + */
> +static size_t handle_using_name_hash_icase(
> + struct index_state *istate, const char *name)
> +{
> + struct cache_entry *ce = NULL;
> +
> + ce = index_file_exists(istate, name, strlen(name), 1);
> + if (!ce)
> + return 0;
> +
> + /*
> + * A case-insensitive search in the name-hash using the
> + * observed pathname found a cache-entry, so the observed path
> + * is case-incorrect. Invalidate the cache-entry and use the
> + * correct spelling from the cache-entry to invalidate the
> + * untracked-cache. Since we now have sparse-directories in
> + * the index, the observed pathname may represent a regular
> + * file or a sparse-index directory.
> + *
> + * Note that we should not have seen FSEvents for a
> + * sparse-index directory, but we handle it just in case.
> + *
> + * Either way, we know that there are not any cache-entries for
> + * children inside the cone of the directory, so we don't need to
> + * do the usual scan.
> + */
> + trace_printf_key(&trace_fsmonitor,
> + "fsmonitor_refresh_callback MAP: '%s' '%s'",
> + name, ce->name);
> +
> + untracked_cache_invalidate_trimmed_path(istate, ce->name, 0);
> + ce->ce_flags &= ~CE_FSMONITOR_VALID;
> + return 1;
> +}
You first ask the name-hash to turn the incoming "name" into the
case variant that we know about, i.e. ce->name, and use that to
access the untracked cache. Clever and makes sense. But if we have
ce->name, doesn't it mean the name is tracked? Do we find anything
useful to do in the untracked cache invalidation codepath in that
case?
An FSmonitor event with case-incorrect pathname for a directory may
not be this trivial, I presume, and I expect that is what the
remainder of this patch is about.
> +
> +/*
> + * Use the dir-name-hash to find the correct-case spelling of the
> + * directory. Use the canonical spelling to invalidate all of the
> + * cache-entries within the matching cone.
> + *
> + * Returns the number of cache-entries that we invalidated.
> + */
> +static size_t handle_using_dir_name_hash_icase(
> + struct index_state *istate, const char *name)
It is a bit unfortunate that here on the name-hash side we contrast
the two helper function variants as "dir-name" vs "name", while the
original handle_path side use "without_slash" vs "with_slash".
If I understand correctly, it is not like there are two distinct
hashes, "name-hash" vs "dir-name-hash". Both of these helpers use
the same "name-hash" mechanism, and this function differs from the
previous one in that it is about a directory, which is why it has
"dir" in its name. I wonder if we renamed the other one with
"nondir" in its name, and the other without_slash and with_slash
pair to match, e.g., handle_nondir_path() vs handle_dir_path(), or
something like that, the resulting names for these four functions
become easier to contrast and understand?
> +{
> + struct strbuf canonical_path = STRBUF_INIT;
> + int pos;
> + size_t len = strlen(name);
> + size_t nr_in_cone;
> +
> + if (name[len - 1] == '/')
> + len--;
> +
> + if (!index_dir_find(istate, name, len, &canonical_path))
> + return 0; /* name is untracked */
> +
> + if (!memcmp(name, canonical_path.buf, canonical_path.len)) {
> + strbuf_release(&canonical_path);
> + /*
> + * NEEDSWORK: Our caller already tried an exact match
> + * and failed to find one. They called us to do an
> + * ICASE match, so we should never get an exact match,
> + * so we could promote this to a BUG() here if we
> + * wanted to. It doesn't hurt anything to just return
> + * 0 and go on becaus we should never get here. Or we
> + * could just get rid of the memcmp() and this "if"
> + * clause completely.
> + */
> + return 0; /* should not happen */
> + }
"becaus" -> "because".
If we should never get here, having BUG("we should never get here")
would not hurt anything, either. On the other hand, silently
returning 0 will hide the bug under the carpet, and I am not sure it
is fair to call it "doesn't hurt anything".
> +
> + trace_printf_key(&trace_fsmonitor,
> + "fsmonitor_refresh_callback MAP: '%s' '%s'",
> + name, canonical_path.buf);
> +
> + /*
> + * The dir-name-hash only tells us the corrected spelling of
> + * the prefix. We have to use this canonical path to do a
> + * lookup in the cache-entry array so that we repeat the
> + * original search using the case-corrected spelling.
> + */
> + strbuf_addch(&canonical_path, '/');
> + pos = index_name_pos(istate, canonical_path.buf,
> + canonical_path.len);
> + nr_in_cone = handle_path_with_trailing_slash(
> + istate, canonical_path.buf, pos);
> + strbuf_release(&canonical_path);
> + return nr_in_cone;
> +}
Nice. Do we need to give this corrected name to help untracked
cache invalidation from the caller that called us?
> @@ -319,6 +416,19 @@ static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
> else
> nr_in_cone = handle_path_without_trailing_slash(istate, name, pos);
>
> + /*
> + * If we did not find an exact match for this pathname or any
> + * cache-entries with this directory prefix and we're on a
> + * case-insensitive file system, try again using the name-hash
> + * and dir-name-hash.
> + */
> + if (!nr_in_cone && ignore_case) {
> + nr_in_cone = handle_using_name_hash_icase(istate, name);
> + if (!nr_in_cone)
> + nr_in_cone = handle_using_dir_name_hash_icase(
> + istate, name);
> + }
It might be interesting to learn how often we go through these
"fallback" code paths by tracing. Maybe it will become too noisy?
I dunno.
> if (nr_in_cone)
> trace_printf_key(&trace_fsmonitor,
> "fsmonitor_refresh_callback CNT: %d",
^ 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