Git development
 help / color / mirror / Atom feed
* Re: Bugreport
From: brian m. carlson @ 2024-01-19 23:14 UTC (permalink / raw)
  To: Frank Schwidom; +Cc: git
In-Reply-To: <20240119132551.GA31532@debian64>

[-- Attachment #1: Type: text/plain, Size: 1195 bytes --]

On 2024-01-19 at 13:25:51, Frank Schwidom wrote:
> 
> This bug exists in possibly all git versions.
> 
> $ git init
> $ touch a.txt
> $ ln -s a.txt d
> $ git add .
> $ git commit -m + .
> [master (root-commit) f6b4468] +
>  2 files changed, 1 insertion(+)
>  create mode 100644 a.txt
>  create mode 120000 d
> $ ls -la
> total 12
> drwxr-xr-x 3 ox ox 4096 Jan 19 14:10 .
> drwxr-xr-x 4 ox ox 4096 Jan 19 14:04 ..
> drwxr-xr-x 8 ox ox 4096 Jan 19 14:10 .git
> -rw-r--r-- 1 ox ox    0 Jan 19 14:10 a.txt
> lrwxrwxrwx 1 ox ox    5 Jan 19 14:10 d -> a.txt
> $ rm d
> $ mkdir d
> $ touch d/b.txt
> $ git add .
> $ git commit . -m +
> error: 'd' does not have a commit checked out
> fatal: updating files failed

I can confirm this behaviour[0], and it's definitely wrong that it
thinks `d` is a submodule.  It does, however, work if you do `git commit
-m +` (that is, without the .), which makes sense since the relevant
change is already staged in the index.

I'm not going to get to a patch or more thorough investigation, but
perhaps someone else will.

[0] git version 2.43.0.381.gb435a96ce8
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

^ permalink raw reply

* Re: [PATCH v2 3/4] config: factor out global config file retrieval
From: Junio C Hamano @ 2024-01-19 23:04 UTC (permalink / raw)
  To: Patrick Steinhardt
  Cc: Kristoffer Haugsbakk, git, stolee, Eric Sunshine, Taylor Blau
In-Reply-To: <Zaor5zNLKE3UXhHM@tanuki>

Patrick Steinhardt <ps@pks.im> writes:

> Yeah, you're right that `git_system_config()` is bad in the same way. In
> fact I think it's worse here because we have both `git_config_system()`
> and `git_system_config()`, which has certainly confused me multiple
> times in the past. So I'd be happy to see it renamed, as well, either
> now or in a follow-up patch series.

OK, let's make a note #leftoverbits here and merge the topic down to
'next'.  By the time it graduates to 'master', we may have a clean-up
patch to rename them.

Thanks, all.

^ permalink raw reply

* Re: [PATCH] MyFirstContribution: update mailing list sub steps
From: Kyle Lippincott @ 2024-01-19 22:46 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Konstantin Ryabitsev, spectral via GitGitGadget, git
In-Reply-To: <xmqqmst1hsd6.fsf@gitster.g>

On Fri, Jan 19, 2024 at 2:26 PM Junio C Hamano <gitster@pobox.com> wrote:
> Subject: Docs: majordomo@vger.kernel.org has been decomissioned
>
> Update the instruction for subscribing to the Git mailing list
> we have on a few documentation pages.
>
> Noticed-by: Kyle Lippincott <spectral@google.com>
> Helped-by: Konstantin Ryabitsev <konstantin@linuxfoundation.org>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/MyFirstContribution.txt | 5 +++--
>  README.md                             | 4 ++--
>  2 files changed, 5 insertions(+), 4 deletions(-)

This version looks good, thanks for catching README.md's reference as well.

^ permalink raw reply

* Re: [PATCH 1/2] fuzz: fix fuzz test build rules
From: Junio C Hamano @ 2024-01-19 22:46 UTC (permalink / raw)
  To: Josh Steadmon; +Cc: git
In-Reply-To: <9332e225e44b29be25d10229b05f0b9775b85568.1705700054.git.steadmon@google.com>

Josh Steadmon <steadmon@google.com> writes:

> @@ -762,7 +763,7 @@ fuzz-objs: $(FUZZ_OBJS)
>  # Always build fuzz objects even if not testing, to prevent bit-rot.
>  all:: $(FUZZ_OBJS)

So, this is what you referred to in your proposed log message.  We
do build objects to prevent bit-rot, but we do not link, so it is
merely half a protection.

> ...
>  fuzz-all: $(FUZZ_PROGRAMS)

But there is this target.  I wonder if it makes it even better to
update the "always build fuzz objects" one?  Given that some folks
may not have the necessary clang toochain for linking, it may
probably be a bit too much, perhaps?

It definitely is an improvement to build them in the CI environment,
like you have in [2/2].

Thanks.  Will queue.



^ permalink raw reply

* Re: [PATCH 0/2] Run limited fuzz tests in GitHub CI
From: Junio C Hamano @ 2024-01-19 22:28 UTC (permalink / raw)
  To: Josh Steadmon; +Cc: git
In-Reply-To: <cover.1705700054.git.steadmon@google.com>

Josh Steadmon <steadmon@google.com> writes:

> Add a simple smoke test in CI to make sure that the fuzz tests can build
> and execute properly. While we already compile the fuzz-test objects in
> the default make target, we don't link the executables due to these
> requiring clang-specific support. However, this means that the fuzz
> tests have been vulnerable to unnoticed build breakages as the code that
> they link against has changed over time.
>
> Adding this CI test should make such build breakages more visible more
> quickly.

Nice.

> Josh Steadmon (2):
>   fuzz: fix fuzz test build rules
>   ci: build and run minimal fuzzers in GitHub CI
>
>  .github/workflows/main.yml          | 11 +++++++++++
>  Makefile                            | 17 +++++++++++------
>  ci/run-build-and-minimal-fuzzers.sh | 19 +++++++++++++++++++
>  oss-fuzz/dummy-cmd-main.c           | 14 ++++++++++++++
>  4 files changed, 55 insertions(+), 6 deletions(-)
>  create mode 100755 ci/run-build-and-minimal-fuzzers.sh
>  create mode 100644 oss-fuzz/dummy-cmd-main.c
>
>
> base-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5

^ permalink raw reply

* Re: [PATCH] MyFirstContribution: update mailing list sub steps
From: Junio C Hamano @ 2024-01-19 22:26 UTC (permalink / raw)
  To: Konstantin Ryabitsev; +Cc: spectral via GitGitGadget, git, spectral
In-Reply-To: <20240119-flat-jellyfish-of-drizzle-b31abe@lemur>

Konstantin Ryabitsev <konstantin@linuxfoundation.org> writes:

> On Fri, Jan 19, 2024 at 08:59:15PM +0000, spectral via GitGitGadget wrote:
>>  contributing are welcome to post questions here. The Git list requires
>>  plain-text-only emails and prefers inline and bottom-posting when replying to
>>  mail; you will be CC'd in all replies to you. Optionally, you can subscribe to
>> -the list by sending an email to majordomo@vger.kernel.org with "subscribe git"
>> -in the body. The https://lore.kernel.org/git[archive] of this mailing list is
>> -available to view in a browser.
>> +the list by visiting https://subspace.kernel.org/vger.kernel.org.html and using
>> +the `sub` link next to the `git` list (this is a mailto link; you can leave
>> +subject and body blank, but you still have to send the email). The
>
> I recommend just telling people to send a message to
> git+subscribe@vger.kernel.org and linking to
> https://subspace.kernel.org/subscribing.html for more details. While
> "sub/unsub" links will do the job for some people, webmail users may not
> have things properly configured to correctly process the mailto: links.

Yeah, good suggestion.

Kyle, thanks for noticing the majordomo deprecation.

Perhaps something like this is agreeable to everybody?

------------ >8 ----------------------- >8 ----------------------- >8 ------------
Subject: Docs: majordomo@vger.kernel.org has been decomissioned

Update the instruction for subscribing to the Git mailing list
we have on a few documentation pages.

Noticed-by: Kyle Lippincott <spectral@google.com>
Helped-by: Konstantin Ryabitsev <konstantin@linuxfoundation.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/MyFirstContribution.txt | 5 +++--
 README.md                             | 4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git c/Documentation/MyFirstContribution.txt w/Documentation/MyFirstContribution.txt
index 279f6a3e7c..f06563e981 100644
--- c/Documentation/MyFirstContribution.txt
+++ w/Documentation/MyFirstContribution.txt
@@ -35,8 +35,9 @@ announcements, design discussions, and more take place. Those interested in
 contributing are welcome to post questions here. The Git list requires
 plain-text-only emails and prefers inline and bottom-posting when replying to
 mail; you will be CC'd in all replies to you. Optionally, you can subscribe to
-the list by sending an email to majordomo@vger.kernel.org with "subscribe git"
-in the body. The https://lore.kernel.org/git[archive] of this mailing list is
+the list by sending an email to <git+subscribe@vger.kernel.org>
+(see https://subspace.kernel.org/subscribing.html for details).
+The https://lore.kernel.org/git[archive] of this mailing list is
 available to view in a browser.
 
 ==== https://groups.google.com/forum/#!forum/git-mentoring[git-mentoring@googlegroups.com]
diff --git c/README.md w/README.md
index 2c3de2f9c8..665ce5f5a8 100644
--- c/README.md
+++ w/README.md
@@ -39,8 +39,8 @@ Those wishing to help with error message, usage and informational message
 string translations (localization l10) should see [po/README.md][]
 (a `po` file is a Portable Object file that holds the translations).
 
-To subscribe to the list, send an email with just "subscribe git" in
-the body to majordomo@vger.kernel.org (not the Git list). The mailing
+To subscribe to the list, send an email to <git+subscribe@vger.kernel.org>
+(see https://subspace.kernel.org/subscribing.html for details). The mailing
 list archives are available at <https://lore.kernel.org/git/>,
 <https://marc.info/?l=git> and other archival sites.
 

^ permalink raw reply related

* [PATCH 2/2] ci: build and run minimal fuzzers in GitHub CI
From: Josh Steadmon @ 2024-01-19 21:38 UTC (permalink / raw)
  To: git
In-Reply-To: <cover.1705700054.git.steadmon@google.com>

To prevent bitrot, we would like to regularly exercise the fuzz tests in
order to make sure they still link & run properly. We already compile
the fuzz test objects as part of the default `make` target, but we do
not link the executables due to the fuzz tests needing specific
compilers and compiler features. This has lead to frequent build
breakages for the fuzz tests.

To remedy this, we can add a CI step to actually link the fuzz
executables, and run them (with finite input rather than the default
infinite random input mode) to verify that they execute properly.

Since the main use of the fuzz tests is via OSS-Fuzz [1], and OSS-Fuzz
only runs tests on Linux [2], we only set up a CI test for the fuzzers
on Linux.

[1] https://github.com/google/oss-fuzz
[2] https://google.github.io/oss-fuzz/further-reading/fuzzer-environment/

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 .github/workflows/main.yml          | 11 +++++++++++
 Makefile                            |  3 +++
 ci/run-build-and-minimal-fuzzers.sh | 19 +++++++++++++++++++
 3 files changed, 33 insertions(+)
 create mode 100755 ci/run-build-and-minimal-fuzzers.sh

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 9fdbd54028..4d97da57ec 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -309,6 +309,17 @@ jobs:
       with:
         name: failed-tests-${{matrix.vector.jobname}}
         path: ${{env.FAILED_TEST_ARTIFACTS}}
+  fuzz-smoke-test:
+    name: fuzz smoke test
+    needs: ci-config
+    if: needs.ci-config.outputs.enabled == 'yes'
+    env:
+      CC: clang
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v3
+    - run: ci/install-dependencies.sh
+    - run: ci/run-build-and-minimal-fuzzers.sh
   dockerized:
     name: ${{matrix.vector.jobname}} (${{matrix.vector.image}})
     needs: ci-config
diff --git a/Makefile b/Makefile
index 1e9bd6430f..2e94c566e0 100644
--- a/Makefile
+++ b/Makefile
@@ -752,6 +752,9 @@ SCRIPTS = $(SCRIPT_SH_GEN) \
 
 ETAGS_TARGET = TAGS
 
+# If you add a new fuzzer, please also make sure to run it in
+# ci/run-build-and-minimal-fuzzers.sh so that we make sure it still links and
+# runs in the future.
 FUZZ_OBJS += oss-fuzz/dummy-cmd-main.o
 FUZZ_OBJS += oss-fuzz/fuzz-commit-graph.o
 FUZZ_OBJS += oss-fuzz/fuzz-date.o
diff --git a/ci/run-build-and-minimal-fuzzers.sh b/ci/run-build-and-minimal-fuzzers.sh
new file mode 100755
index 0000000000..8ba486f659
--- /dev/null
+++ b/ci/run-build-and-minimal-fuzzers.sh
@@ -0,0 +1,19 @@
+#!/bin/sh
+#
+# Build and test Git's fuzzers
+#
+
+. ${0%/*}/lib.sh
+
+group "Build fuzzers" make \
+	CC=clang \
+	CXX=clang++ \
+	CFLAGS="-fsanitize=fuzzer-no-link,address" \
+	LIB_FUZZING_ENGINE="-fsanitize=fuzzer,address" \
+	fuzz-all
+
+for fuzzer in commit-graph date pack-headers pack-idx ; do
+	begin_group "fuzz-$fuzzer"
+	./oss-fuzz/fuzz-$fuzzer -verbosity=0 -runs=1 || exit 1
+	end_group "fuzz-$fuzzer"
+done
-- 
2.43.0.429.g432eaa2c6b-goog


^ permalink raw reply related

* [PATCH 1/2] fuzz: fix fuzz test build rules
From: Josh Steadmon @ 2024-01-19 21:38 UTC (permalink / raw)
  To: git
In-Reply-To: <cover.1705700054.git.steadmon@google.com>

When we originally added the fuzz tests in 5e47215080 (fuzz: add basic
fuzz testing target., 2018-10-12), we went to some trouble to create a
Makefile rule that allowed linking the fuzz executables without pulling
in common-main.o. This was necessary to prevent the
fuzzing-engine-provided main() from clashing with Git's main().

However, since 19d75948ef (common-main.c: move non-trace2 exit()
behavior out of trace2.c, 2022-06-02), it has been necessary to link
common-main.o due to moving the common_exit() function to that file.
Ævar suggested a set of compiler flags to allow this in [1], but this
was never reflected in the Makefile.

Since we now must include common-main.o, there's no reason to pick and
choose a subset of object files to link, so simplify the Makefile rule
for the fuzzer executables to just use libgit.a. While we're at it,
include the necessary linker flag to allow multiple definitions
directly in the Makefile rule, rather than requiring it to be passed on
the command-line each time. This means the Makefile rule as written is
now more compiler-specific, but this was already the case for the
fuzzers themselves anyway.

[1] https://lore.kernel.org/git/220607.8635ggupws.gmgdl@evledraar.gmail.com/

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Makefile                  | 14 ++++++++------
 oss-fuzz/dummy-cmd-main.c | 14 ++++++++++++++
 2 files changed, 22 insertions(+), 6 deletions(-)
 create mode 100644 oss-fuzz/dummy-cmd-main.c

diff --git a/Makefile b/Makefile
index 15990ff312..1e9bd6430f 100644
--- a/Makefile
+++ b/Makefile
@@ -752,6 +752,7 @@ SCRIPTS = $(SCRIPT_SH_GEN) \
 
 ETAGS_TARGET = TAGS
 
+FUZZ_OBJS += oss-fuzz/dummy-cmd-main.o
 FUZZ_OBJS += oss-fuzz/fuzz-commit-graph.o
 FUZZ_OBJS += oss-fuzz/fuzz-date.o
 FUZZ_OBJS += oss-fuzz/fuzz-pack-headers.o
@@ -762,7 +763,7 @@ fuzz-objs: $(FUZZ_OBJS)
 # Always build fuzz objects even if not testing, to prevent bit-rot.
 all:: $(FUZZ_OBJS)
 
-FUZZ_PROGRAMS += $(patsubst %.o,%,$(FUZZ_OBJS))
+FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS)))
 
 # Empty...
 EXTRA_PROGRAMS =
@@ -3850,16 +3851,17 @@ cover_db_html: cover_db
 #
 # make CC=clang CXX=clang++ \
 #      CFLAGS="-fsanitize=fuzzer-no-link,address" \
-#      LIB_FUZZING_ENGINE="-fsanitize=fuzzer" \
+#      LIB_FUZZING_ENGINE="-fsanitize=fuzzer,address" \
 #      fuzz-all
 #
-FUZZ_CXXFLAGS ?= $(CFLAGS)
+FUZZ_CXXFLAGS ?= $(ALL_CFLAGS)
 
 .PHONY: fuzz-all
 
-$(FUZZ_PROGRAMS): all
-	$(QUIET_LINK)$(CXX) $(FUZZ_CXXFLAGS) $(LIB_OBJS) $(BUILTIN_OBJS) \
-		$(XDIFF_OBJS) $(EXTLIBS) git.o $@.o $(LIB_FUZZING_ENGINE) -o $@
+$(FUZZ_PROGRAMS): %: %.o oss-fuzz/dummy-cmd-main.o $(GITLIBS) GIT-LDFLAGS
+	$(QUIET_LINK)$(CXX) $(FUZZ_CXXFLAGS) -o $@ $(ALL_LDFLAGS) \
+		-Wl,--allow-multiple-definition \
+		$(filter %.o,$^) $(filter %.a,$^) $(LIBS) $(LIB_FUZZING_ENGINE)
 
 fuzz-all: $(FUZZ_PROGRAMS)
 
diff --git a/oss-fuzz/dummy-cmd-main.c b/oss-fuzz/dummy-cmd-main.c
new file mode 100644
index 0000000000..071cb231ba
--- /dev/null
+++ b/oss-fuzz/dummy-cmd-main.c
@@ -0,0 +1,14 @@
+#include "git-compat-util.h"
+
+/*
+ * When linking the fuzzers, we link against common-main.o to pick up some
+ * symbols. However, even though we ignore common-main:main(), we still need to
+ * provide all the symbols it references. In the fuzzers' case, we need to
+ * provide a dummy cmd_main() for the linker to be happy. It will never be
+ * executed.
+ */
+
+int cmd_main(int argc, const char **argv) {
+	BUG("We should not execute cmd_main() from a fuzz target");
+	return 1;
+}
-- 
2.43.0.429.g432eaa2c6b-goog


^ permalink raw reply related

* [PATCH 0/2] Run limited fuzz tests in GitHub CI
From: Josh Steadmon @ 2024-01-19 21:38 UTC (permalink / raw)
  To: git

Add a simple smoke test in CI to make sure that the fuzz tests can build
and execute properly. While we already compile the fuzz-test objects in
the default make target, we don't link the executables due to these
requiring clang-specific support. However, this means that the fuzz
tests have been vulnerable to unnoticed build breakages as the code that
they link against has changed over time.

Adding this CI test should make such build breakages more visible more
quickly.


Josh Steadmon (2):
  fuzz: fix fuzz test build rules
  ci: build and run minimal fuzzers in GitHub CI

 .github/workflows/main.yml          | 11 +++++++++++
 Makefile                            | 17 +++++++++++------
 ci/run-build-and-minimal-fuzzers.sh | 19 +++++++++++++++++++
 oss-fuzz/dummy-cmd-main.c           | 14 ++++++++++++++
 4 files changed, 55 insertions(+), 6 deletions(-)
 create mode 100755 ci/run-build-and-minimal-fuzzers.sh
 create mode 100644 oss-fuzz/dummy-cmd-main.c


base-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5
-- 
2.43.0.429.g432eaa2c6b-goog


^ permalink raw reply

* Re: [PATCH] MyFirstContribution: update mailing list sub steps
From: Konstantin Ryabitsev @ 2024-01-19 21:06 UTC (permalink / raw)
  To: spectral via GitGitGadget; +Cc: git, spectral
In-Reply-To: <pull.1644.git.1705697955144.gitgitgadget@gmail.com>

On Fri, Jan 19, 2024 at 08:59:15PM +0000, spectral via GitGitGadget wrote:
>  contributing are welcome to post questions here. The Git list requires
>  plain-text-only emails and prefers inline and bottom-posting when replying to
>  mail; you will be CC'd in all replies to you. Optionally, you can subscribe to
> -the list by sending an email to majordomo@vger.kernel.org with "subscribe git"
> -in the body. The https://lore.kernel.org/git[archive] of this mailing list is
> -available to view in a browser.
> +the list by visiting https://subspace.kernel.org/vger.kernel.org.html and using
> +the `sub` link next to the `git` list (this is a mailto link; you can leave
> +subject and body blank, but you still have to send the email). The

I recommend just telling people to send a message to
git+subscribe@vger.kernel.org and linking to
https://subspace.kernel.org/subscribing.html for more details. While
"sub/unsub" links will do the job for some people, webmail users may not
have things properly configured to correctly process the mailto: links.

-K

^ permalink raw reply

* [PATCH] MyFirstContribution: update mailing list sub steps
From: spectral via GitGitGadget @ 2024-01-19 20:59 UTC (permalink / raw)
  To: git; +Cc: Konstantin Ryabitsev, spectral, Kyle Lippincott

From: Kyle Lippincott <spectral@google.com>

The documentation says to send an email to majordomo. Doing so gets you
an auto response saying majordomo has been turned down, and to instead
visit https://subspace.kernel.org/vger.kernel.org.html and use the
sub/unsub links.

Update the instructions to reference the supported mechanism for
subscribing to the git mailing list. Include details like these being
mailto links but the subject/body aren't necessary, since it's not
obvious what to put in those fields.

Signed-off-by: Kyle Lippincott <spectral@google.com>
---
    MyFirstContribution: update mailing list sub steps

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1644%2Fspectral54%2Fmy-first-contribution-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1644/spectral54/my-first-contribution-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1644

 Documentation/MyFirstContribution.txt | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/MyFirstContribution.txt b/Documentation/MyFirstContribution.txt
index 279f6a3e7ca..2361fe27e97 100644
--- a/Documentation/MyFirstContribution.txt
+++ b/Documentation/MyFirstContribution.txt
@@ -35,9 +35,11 @@ announcements, design discussions, and more take place. Those interested in
 contributing are welcome to post questions here. The Git list requires
 plain-text-only emails and prefers inline and bottom-posting when replying to
 mail; you will be CC'd in all replies to you. Optionally, you can subscribe to
-the list by sending an email to majordomo@vger.kernel.org with "subscribe git"
-in the body. The https://lore.kernel.org/git[archive] of this mailing list is
-available to view in a browser.
+the list by visiting https://subspace.kernel.org/vger.kernel.org.html and using
+the `sub` link next to the `git` list (this is a mailto link; you can leave
+subject and body blank, but you still have to send the email). The
+https://lore.kernel.org/git[archive] of this mailing list is available to view
+in a browser.
 
 ==== https://groups.google.com/forum/#!forum/git-mentoring[git-mentoring@googlegroups.com]
 

base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b
-- 
gitgitgadget

^ permalink raw reply related

* Re: [PATCH 4/5] refs: introduce `refs_for_each_all_refs()`
From: Junio C Hamano @ 2024-01-19 20:57 UTC (permalink / raw)
  To: Karthik Nayak; +Cc: git
In-Reply-To: <20240119142705.139374-5-karthik.188@gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

> +static void add_pseudoref_like_entries(struct ref_store *ref_store,
> +					 struct ref_dir *dir,
> +					 const char *dirname)
> +{
> +	struct files_ref_store *refs =
> +		files_downcast(ref_store, REF_STORE_READ, "fill_ref_dir");
> +	struct strbuf path = STRBUF_INIT, refname = STRBUF_INIT;
> +	struct dirent *de;
> +	size_t dirnamelen;
> +	DIR *d;
> +
> +	files_ref_path(refs, &path, dirname);
> +
> +	d = opendir(path.buf);
> +	if (!d) {
> +		strbuf_release(&path);
> +		return;
> +	}
> +
> +	strbuf_addstr(&refname, dirname);
> +	dirnamelen = refname.len;
> +
> +	while ((de = readdir(d)) != NULL) {
> +		unsigned char dtype;
> +
> +		if (de->d_name[0] == '.')
> +			continue;
> +		if (ends_with(de->d_name, ".lock"))
> +			continue;
> +		strbuf_addstr(&refname, de->d_name);
> +
> +		dtype = get_dtype(de, &path, 1);
> +		if (dtype == DT_REG && is_pseudoref_syntax(de->d_name))
> +			loose_fill_ref_dir_regular_file(refs, refname.buf, dir);

This looks more like add_pseudoref_entries() given that the general
direction is to have an "allow" list of pseudo refs (at this point
after the previous step of the series, is_pseudoref_syntax() is the
is_pseudoref() function, and uses ends_with("_HEAD") as a mere
optimization to avoid listing all the possible pseudo refs that
exists or will be added in the future whose name ends with "_HEAD").

Other than the naming, I think these two steps make sense.

Thanks.

^ permalink raw reply

* Re: [PATCH 2/5] refs: make `is_pseudoref_syntax()` stricter
From: Junio C Hamano @ 2024-01-19 20:44 UTC (permalink / raw)
  To: Karthik Nayak; +Cc: git
In-Reply-To: <20240119142705.139374-3-karthik.188@gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

> Using this information we make the `is_pseudoref_syntax()` function
> stricter, by adding the check for "HEAD" suffix and for refs which don't
> end with the HEAD suffix, matching them with a predetermined list.

OK, so this partly answers my question on the previous step.  Before
making it more strict, the function worked only on the "syntax", so
a random string that can be a pseudo ref passed the check.

But stepping back a bit, if we call this function is_pseudoref_syntax(),
wouldn't it be what we want to have anyway?  You seem to want a
separate function called is_pseudoref() that rejects bogus uppercase
string "FOO_BAR" while accepting the known-good pseudoref you add
tests for, plus the ${FOO}_HEAD for any value of ${FOO} we currently
have and we may want to add in the future.

>  int is_pseudoref_syntax(const char *refname)
>  {
> +	/* TODO: move these pseudorefs to have _HEAD suffix */
> +	static const char *const irregular_pseudorefs[] = {
> +		"BISECT_EXPECTED_REV",
> +		"NOTES_MERGE_PARTIAL",
> +		"NOTES_MERGE_REF",
> +		"AUTO_MERGE"
> +	};
> +	size_t i;
>  	const char *c;
>  
>  	for (c = refname; *c; c++) {
> @@ -837,10 +845,17 @@ int is_pseudoref_syntax(const char *refname)
>  	}
>  
>  	/*
> -	 * HEAD is not a pseudoref, but it certainly uses the
> -	 * pseudoref syntax.
> +	 * Most pseudorefs end with _HEAD. HEAD itself is not a
> +	 * pseudoref, but it certainly uses the pseudoref syntax.
>  	 */
> -	return 1;
> +	if (ends_with(refname, "HEAD"))
> +		return 1;

I would imagine that at the final stage in which something like this
will be named is_pseudoref(), asking is_pseudoref("HEAD") would
return "No" (even though "is_pseudoref_syntax()", if the helper
function remains, may say "Yes" to "HEAD").  And this ends_with()
will use "_HEAD", instead of "HEAD".  But I am reading ahead of
myself, so let's keep going.

> +	for (i = 0; i < ARRAY_SIZE(irregular_pseudorefs); i++)
> +		if (!strcmp(refname, irregular_pseudorefs[i]))
> +			return 1;
> +
> +	return 0;
>  }

^ permalink raw reply

* Re: [PATCH 1/5] refs: expose `is_pseudoref_syntax()`
From: Junio C Hamano @ 2024-01-19 20:37 UTC (permalink / raw)
  To: Karthik Nayak; +Cc: git
In-Reply-To: <20240119142705.139374-2-karthik.188@gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

> The `is_pseudoref_syntax()` function is static, since it is only used
> within `refs.c`. In the following commit, we will use this function to
> provide an utility to add pseudorefs to the loose refs cache. So let's
> expose this function via `refs.h`.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  refs.c | 2 +-
>  refs.h | 6 ++++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/refs.c b/refs.c
> index 2f58a3460a..5999605230 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -827,7 +827,7 @@ int is_per_worktree_ref(const char *refname)
>  	       starts_with(refname, "refs/rewritten/");
>  }
>  
> -static int is_pseudoref_syntax(const char *refname)
> +int is_pseudoref_syntax(const char *refname)
>  {
>  	const char *c;
>  
> diff --git a/refs.h b/refs.h
> index ff113bb12a..f1bbad83fb 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -846,6 +846,12 @@ const char **hidden_refs_to_excludes(const struct strvec *hide_refs);
>  /* Is this a per-worktree ref living in the refs/ namespace? */
>  int is_per_worktree_ref(const char *refname);
>  
> +/*
> + * Check whether a refname matches the pseudoref syntax. This is a surface
> + * level check and can present false positives.
> + */

What does "false positive" mean in this context?

is_pseudoref_syntax("FOO_HEAD") says "true", and then if it is
"false positive", that would mean "FOO_HEAD" is not a pseudo ref,
right?  What can a caller of this function do to deal with a false
positive?

Or do you mean "FOO_HEAD" is still a pseudo ref, but it may not
currently exist?  That is different from "false positive".

As the check is about "does it match the pseudoref syntax?", I would
understand if what you wanted to say was something like: This only
checks the syntax, and such a pseudoref may not currently exist in
the repository---for that you'd need to call read_ref_full() or
other ref API functions.

Puzzled...

Thanks.

^ permalink raw reply

* Re: [PATCH 6/7] refs: redefine special refs
From: Junio C Hamano @ 2024-01-19 20:28 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git
In-Reply-To: <a4b4dd51f81fdf65f79b9afc3bd85109817ea128.1705659748.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

> Now that our list of special refs really only contains refs which have
> actually-special semantics, let's redefine what makes a special ref.

Yay.

>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  refs.c | 33 +++++++--------------------------
>  1 file changed, 7 insertions(+), 26 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 047c81b1c1..08a900a047 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1839,13 +1839,10 @@ static int refs_read_special_head(struct ref_store *ref_store,
>  static int is_special_ref(const char *refname)
>  {
>  	/*
> -	 * Special references get written and read directly via the filesystem
> -	 * by the subsystems that create them. Thus, they must not go through
> -	 * the reference backend but must instead be read directly. It is
> -	 * arguable whether this behaviour is sensible, or whether it's simply
> -	 * a leaky abstraction enabled by us only having a single reference
> -	 * backend implementation. But at least for a subset of references it
> -	 * indeed does make sense to treat them specially:
> +	 * Special references are refs that have different semantics compared
> +	 * to "normal" refs. These refs can thus not be stored in the ref
> +	 * backend, but must always be accessed via the filesystem. The
> +	 * following refs are special:
>  	 *
>  	 * - FETCH_HEAD may contain multiple object IDs, and each one of them
>  	 *   carries additional metadata like where it came from.
> @@ -1853,25 +1850,9 @@ static int is_special_ref(const char *refname)
>  	 * - MERGE_HEAD may contain multiple object IDs when merging multiple
>  	 *   heads.
>  	 *
> -	 * There are some exceptions that you might expect to see on this list
> -	 * but which are handled exclusively via the reference backend:
> -	 *
> -	 * - BISECT_EXPECTED_REV
> -	 *
> -	 * - CHERRY_PICK_HEAD
> -	 *
> -	 * - HEAD
> -	 *
> -	 * - ORIG_HEAD
> -	 *
> -	 * - "rebase-apply/" and "rebase-merge/" contain all of the state for
> -	 *   rebases, including some reference-like files. These are
> -	 *   exclusively read and written via the filesystem and never go
> -	 *   through the refdb.
> -	 *
> -	 * Writing or deleting references must consistently go either through
> -	 * the filesystem (special refs) or through the reference backend
> -	 * (normal ones).
> +	 * Reading, writing or deleting references must consistently go either
> +	 * through the filesystem (special refs) or through the reference
> +	 * backend (normal ones).
>  	 */
>  	static const char * const special_refs[] = {
>  		"FETCH_HEAD",

^ permalink raw reply

* [PATCH v2 12/12] t5312: move reffiles specific tests to t0601
From: John Cai via GitGitGadget @ 2024-01-19 20:19 UTC (permalink / raw)
  To: git; +Cc: Patrick Steinhardt, John Cai, John Cai
In-Reply-To: <pull.1647.v2.git.git.1705695540.gitgitgadget@gmail.com>

From: John Cai <johncai86@gmail.com>

Move a few tests into t0601 since they specifically test the packed-refs
file and thus are specific to the reffiles backend.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 t/t0601-reffiles-pack-refs.sh | 30 ++++++++++++++++++++++++++++++
 t/t5312-prune-corruption.sh   | 26 --------------------------
 2 files changed, 30 insertions(+), 26 deletions(-)

diff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh
index c2c19befacc..263e99cd84b 100755
--- a/t/t0601-reffiles-pack-refs.sh
+++ b/t/t0601-reffiles-pack-refs.sh
@@ -328,4 +328,34 @@ test_expect_success 'refs/worktree must not be packed' '
 	test_path_is_file .git/worktrees/wt2/refs/worktree/foo
 '
 
+# we do not want to count on running pack-refs to
+# actually pack it, as it is perfectly reasonable to
+# skip processing a broken ref
+test_expect_success 'create packed-refs file with broken ref' '
+	test_tick && git commit --allow-empty -m one &&
+	recoverable=$(git rev-parse HEAD) &&
+	test_tick && git commit --allow-empty -m two &&
+	missing=$(git rev-parse HEAD) &&
+	rm -f .git/refs/heads/main &&
+	cat >.git/packed-refs <<-EOF &&
+	$missing refs/heads/main
+	$recoverable refs/heads/other
+	EOF
+	echo $missing >expect &&
+	git rev-parse refs/heads/main >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pack-refs does not silently delete broken packed ref' '
+	git pack-refs --all --prune &&
+	git rev-parse refs/heads/main >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pack-refs does not drop broken refs during deletion' '
+	git update-ref -d refs/heads/other &&
+	git rev-parse refs/heads/main >actual &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh
index 230cb387122..d8d2e304687 100755
--- a/t/t5312-prune-corruption.sh
+++ b/t/t5312-prune-corruption.sh
@@ -111,30 +111,4 @@ test_expect_success 'pack-refs does not silently delete broken loose ref' '
 	test_cmp expect actual
 '
 
-# we do not want to count on running pack-refs to
-# actually pack it, as it is perfectly reasonable to
-# skip processing a broken ref
-test_expect_success REFFILES 'create packed-refs file with broken ref' '
-	rm -f .git/refs/heads/main &&
-	cat >.git/packed-refs <<-EOF &&
-	$missing refs/heads/main
-	$recoverable refs/heads/other
-	EOF
-	echo $missing >expect &&
-	git rev-parse refs/heads/main >actual &&
-	test_cmp expect actual
-'
-
-test_expect_success REFFILES 'pack-refs does not silently delete broken packed ref' '
-	git pack-refs --all --prune &&
-	git rev-parse refs/heads/main >actual &&
-	test_cmp expect actual
-'
-
-test_expect_success REFFILES  'pack-refs does not drop broken refs during deletion' '
-	git update-ref -d refs/heads/other &&
-	git rev-parse refs/heads/main >actual &&
-	test_cmp expect actual
-'
-
 test_done
-- 
gitgitgadget

^ permalink raw reply related

* [PATCH v2 11/12] t4202: move reffiles specific tests to t0600
From: John Cai via GitGitGadget @ 2024-01-19 20:18 UTC (permalink / raw)
  To: git; +Cc: Patrick Steinhardt, John Cai, John Cai
In-Reply-To: <pull.1647.v2.git.git.1705695540.gitgitgadget@gmail.com>

From: John Cai <johncai86@gmail.com>

Move two tests into t0600 since they write loose reflog refs manually
and thus are specific to the reffiles backend.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 t/t0600-reffiles-backend.sh | 17 +++++++++++++++++
 t/t4202-log.sh              | 17 -----------------
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
index a2ef34eab28..17ff60dde77 100755
--- a/t/t0600-reffiles-backend.sh
+++ b/t/t0600-reffiles-backend.sh
@@ -364,4 +364,21 @@ test_expect_success SYMLINKS 'ref resolution not confused by broken symlinks' '
        test_must_fail git rev-parse --verify broken
 '
 
+test_expect_success 'log diagnoses bogus HEAD hash' '
+	git init empty &&
+	test_when_finished "rm -rf empty" &&
+	echo 1234abcd >empty/.git/refs/heads/main &&
+	test_must_fail git -C empty log 2>stderr &&
+	test_grep broken stderr
+'
+
+test_expect_success 'log diagnoses bogus HEAD symref' '
+	git init empty &&
+	test-tool -C empty ref-store main create-symref HEAD refs/heads/invalid.lock &&
+	test_must_fail git -C empty log 2>stderr &&
+	test_grep broken stderr &&
+	test_must_fail git -C empty log --default totally-bogus 2>stderr &&
+	test_grep broken stderr
+'
+
 test_done
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index ddd205f98ab..60fe60d7610 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -2255,23 +2255,6 @@ test_expect_success 'log on empty repo fails' '
 	test_grep does.not.have.any.commits stderr
 '
 
-test_expect_success REFFILES 'log diagnoses bogus HEAD hash' '
-	git init empty &&
-	test_when_finished "rm -rf empty" &&
-	echo 1234abcd >empty/.git/refs/heads/main &&
-	test_must_fail git -C empty log 2>stderr &&
-	test_grep broken stderr
-'
-
-test_expect_success REFFILES 'log diagnoses bogus HEAD symref' '
-	git init empty &&
-	test-tool -C empty ref-store main create-symref HEAD refs/heads/invalid.lock &&
-	test_must_fail git -C empty log 2>stderr &&
-	test_grep broken stderr &&
-	test_must_fail git -C empty log --default totally-bogus 2>stderr &&
-	test_grep broken stderr
-'
-
 test_expect_success 'log does not default to HEAD when rev input is given' '
 	git log --branches=does-not-exist >actual &&
 	test_must_be_empty actual
-- 
gitgitgadget


^ permalink raw reply related

* [PATCH v2 10/12] t3903: make drop stash test ref backend agnostic
From: John Cai via GitGitGadget @ 2024-01-19 20:18 UTC (permalink / raw)
  To: git; +Cc: Patrick Steinhardt, John Cai, John Cai
In-Reply-To: <pull.1647.v2.git.git.1705695540.gitgitgadget@gmail.com>

From: John Cai <johncai86@gmail.com>

In this test, the calls to cut(1) are only used to verify that the
contents of the reflog entry look as expected. By replacing these with
git-reflog(1) calls, we can make this test ref-backend agnostic.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 t/t3903-stash.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 34faeac3f1c..33192405155 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -200,7 +200,7 @@ test_expect_success 'drop stash reflog updates refs/stash' '
 	test_cmp expect actual
 '
 
-test_expect_success REFFILES 'drop stash reflog updates refs/stash with rewrite' '
+test_expect_success 'drop stash reflog updates refs/stash with rewrite' '
 	git init repo &&
 	(
 		cd repo &&
@@ -213,16 +213,16 @@ test_expect_success REFFILES 'drop stash reflog updates refs/stash with rewrite'
 	new_oid="$(git -C repo rev-parse stash@{0})" &&
 
 	cat >expect <<-EOF &&
-	$(test_oid zero) $old_oid
-	$old_oid $new_oid
+	$new_oid
+	$old_oid
 	EOF
-	cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
+	git -C repo reflog show refs/stash --format=%H >actual &&
 	test_cmp expect actual &&
 
 	git -C repo stash drop stash@{1} &&
-	cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
+	git -C repo reflog show refs/stash --format=%H >actual &&
 	cat >expect <<-EOF &&
-	$(test_oid zero) $new_oid
+	$new_oid
 	EOF
 	test_cmp expect actual
 '
-- 
gitgitgadget


^ permalink raw reply related

* [PATCH v2 09/12] t1503: move reffiles specific tests to t0600
From: John Cai via GitGitGadget @ 2024-01-19 20:18 UTC (permalink / raw)
  To: git; +Cc: Patrick Steinhardt, John Cai, John Cai
In-Reply-To: <pull.1647.v2.git.git.1705695540.gitgitgadget@gmail.com>

From: John Cai <johncai86@gmail.com>

Move this test to t0600 with other reffiles specific tests since it
checks for loose refs and is specific to the reffiles backend.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 t/t0600-reffiles-backend.sh | 5 +++++
 t/t1503-rev-parse-verify.sh | 5 -----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
index 44571033fac..a2ef34eab28 100755
--- a/t/t0600-reffiles-backend.sh
+++ b/t/t0600-reffiles-backend.sh
@@ -359,4 +359,9 @@ test_expect_success 'empty reflog' '
 	test_must_be_empty err
 '
 
+test_expect_success SYMLINKS 'ref resolution not confused by broken symlinks' '
+       ln -s does-not-exist .git/refs/heads/broken &&
+       test_must_fail git rev-parse --verify broken
+'
+
 test_done
diff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh
index bc136833c10..79df65ec7f6 100755
--- a/t/t1503-rev-parse-verify.sh
+++ b/t/t1503-rev-parse-verify.sh
@@ -144,11 +144,6 @@ test_expect_success 'main@{n} for various n' '
 	test_must_fail git rev-parse --verify main@{$Np1}
 '
 
-test_expect_success SYMLINKS,REFFILES 'ref resolution not confused by broken symlinks' '
-	ln -s does-not-exist .git/refs/heads/broken &&
-	test_must_fail git rev-parse --verify broken
-'
-
 test_expect_success 'options can appear after --verify' '
 	git rev-parse --verify HEAD >expect &&
 	git rev-parse --verify -q HEAD >actual &&
-- 
gitgitgadget


^ permalink raw reply related

* [PATCH v2 08/12] t1415: move reffiles specific tests to t0601
From: John Cai via GitGitGadget @ 2024-01-19 20:18 UTC (permalink / raw)
  To: git; +Cc: Patrick Steinhardt, John Cai, John Cai
In-Reply-To: <pull.1647.v2.git.git.1705695540.gitgitgadget@gmail.com>

From: John Cai <johncai86@gmail.com>

Move this test into t0601 with other reffiles pack-refs specific tests
since it checks for individua loose refs and thus is specific to the
reffiles backend.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 t/t0601-reffiles-pack-refs.sh | 20 ++++++++++++++++++++
 t/t1415-worktree-refs.sh      | 11 -----------
 2 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh
index 2e457c4f2df..c2c19befacc 100755
--- a/t/t0601-reffiles-pack-refs.sh
+++ b/t/t0601-reffiles-pack-refs.sh
@@ -308,4 +308,24 @@ test_expect_success SYMLINKS 'pack symlinked packed-refs' '
 	test "$(test_readlink .git/packed-refs)" = "my-deviant-packed-refs"
 '
 
+# The 'packed-refs' file is stored directly in .git/. This means it is global
+# to the repository, and can only contain refs that are shared across all
+# worktrees.
+test_expect_success 'refs/worktree must not be packed' '
+	test_commit initial &&
+	test_commit wt1 &&
+	test_commit wt2 &&
+	git worktree add wt1 wt1 &&
+	git worktree add wt2 wt2 &&
+	git checkout initial &&
+	git update-ref refs/worktree/foo HEAD &&
+	git -C wt1 update-ref refs/worktree/foo HEAD &&
+	git -C wt2 update-ref refs/worktree/foo HEAD &&
+	git pack-refs --all &&
+	test_path_is_missing .git/refs/tags/wt1 &&
+	test_path_is_file .git/refs/worktree/foo &&
+	test_path_is_file .git/worktrees/wt1/refs/worktree/foo &&
+	test_path_is_file .git/worktrees/wt2/refs/worktree/foo
+'
+
 test_done
diff --git a/t/t1415-worktree-refs.sh b/t/t1415-worktree-refs.sh
index 3b531842dd4..eb4eec8becb 100755
--- a/t/t1415-worktree-refs.sh
+++ b/t/t1415-worktree-refs.sh
@@ -17,17 +17,6 @@ test_expect_success 'setup' '
 	git -C wt2 update-ref refs/worktree/foo HEAD
 '
 
-# The 'packed-refs' file is stored directly in .git/. This means it is global
-# to the repository, and can only contain refs that are shared across all
-# worktrees.
-test_expect_success REFFILES 'refs/worktree must not be packed' '
-	git pack-refs --all &&
-	test_path_is_missing .git/refs/tags/wt1 &&
-	test_path_is_file .git/refs/worktree/foo &&
-	test_path_is_file .git/worktrees/wt1/refs/worktree/foo &&
-	test_path_is_file .git/worktrees/wt2/refs/worktree/foo
-'
-
 test_expect_success 'refs/worktree are per-worktree' '
 	test_cmp_rev worktree/foo initial &&
 	( cd wt1 && test_cmp_rev worktree/foo wt1 ) &&
-- 
gitgitgadget


^ permalink raw reply related

* [PATCH v2 07/12] t1410: move reffiles specific tests to t0600
From: John Cai via GitGitGadget @ 2024-01-19 20:18 UTC (permalink / raw)
  To: git; +Cc: Patrick Steinhardt, John Cai, John Cai
In-Reply-To: <pull.1647.v2.git.git.1705695540.gitgitgadget@gmail.com>

From: John Cai <johncai86@gmail.com>

Move these tests to t0600 with other reffiles specific tests since they
do things like take a lock on an individual ref, and write directly into
the reflog refs

Signed-off-by: John Cai <johncai86@gmail.com>
---
 t/t0600-reffiles-backend.sh | 51 +++++++++++++++++++++++++++++++++++++
 t/t1410-reflog.sh           | 42 ------------------------------
 2 files changed, 51 insertions(+), 42 deletions(-)

diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
index 3bd28699d53..44571033fac 100755
--- a/t/t0600-reffiles-backend.sh
+++ b/t/t0600-reffiles-backend.sh
@@ -308,4 +308,55 @@ test_expect_success 'for_each_reflog()' '
 	test_cmp expected actual
 '
 
+# Triggering the bug detected by this test requires a newline to fall
+# exactly BUFSIZ-1 bytes from the end of the file. We don't know
+# what that value is, since it's platform dependent. However, if
+# we choose some value N, we also catch any D which divides N evenly
+# (since we will read backwards in chunks of D). So we choose 8K,
+# which catches glibc (with an 8K BUFSIZ) and *BSD (1K).
+#
+# Each line is 114 characters, so we need 75 to still have a few before the
+# last 8K. The 89-character padding on the final entry lines up our
+# newline exactly.
+test_expect_success SHA1 'parsing reverse reflogs at BUFSIZ boundaries' '
+	git checkout -b reflogskip &&
+	zf=$(test_oid zero_2) &&
+	ident="abc <xyz> 0000000001 +0000" &&
+	for i in $(test_seq 1 75); do
+		printf "$zf%02d $zf%02d %s\t" $i $(($i+1)) "$ident" &&
+		if test $i = 75; then
+			for j in $(test_seq 1 89); do
+				printf X || return 1
+			done
+		else
+			printf X
+		fi &&
+		printf "\n" || return 1
+	done >.git/logs/refs/heads/reflogskip &&
+	git rev-parse reflogskip@{73} >actual &&
+	echo ${zf}03 >expect &&
+	test_cmp expect actual
+'
+
+# This test takes a lock on an individual ref; this is not supported in
+# reftable.
+test_expect_success 'reflog expire operates on symref not referrent' '
+	git branch --create-reflog the_symref &&
+	git branch --create-reflog referrent &&
+	git update-ref referrent HEAD &&
+	git symbolic-ref refs/heads/the_symref refs/heads/referrent &&
+	test_when_finished "rm -f .git/refs/heads/referrent.lock" &&
+	touch .git/refs/heads/referrent.lock &&
+	git reflog expire --expire=all the_symref
+'
+
+test_expect_success 'empty reflog' '
+	test_when_finished "rm -rf empty" &&
+	git init empty &&
+	test_commit -C empty A &&
+	>empty/.git/logs/refs/heads/foo &&
+	git -C empty reflog expire --all 2>err &&
+	test_must_be_empty err
+'
+
 test_done
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index a0ff8d51f04..d2f5f42e674 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -354,36 +354,6 @@ test_expect_success 'stale dirs do not cause d/f conflicts (reflogs off)' '
 	test_must_be_empty actual
 '
 
-# Triggering the bug detected by this test requires a newline to fall
-# exactly BUFSIZ-1 bytes from the end of the file. We don't know
-# what that value is, since it's platform dependent. However, if
-# we choose some value N, we also catch any D which divides N evenly
-# (since we will read backwards in chunks of D). So we choose 8K,
-# which catches glibc (with an 8K BUFSIZ) and *BSD (1K).
-#
-# Each line is 114 characters, so we need 75 to still have a few before the
-# last 8K. The 89-character padding on the final entry lines up our
-# newline exactly.
-test_expect_success REFFILES,SHA1 'parsing reverse reflogs at BUFSIZ boundaries' '
-	git checkout -b reflogskip &&
-	zf=$(test_oid zero_2) &&
-	ident="abc <xyz> 0000000001 +0000" &&
-	for i in $(test_seq 1 75); do
-		printf "$zf%02d $zf%02d %s\t" $i $(($i+1)) "$ident" &&
-		if test $i = 75; then
-			for j in $(test_seq 1 89); do
-				printf X || return 1
-			done
-		else
-			printf X
-		fi &&
-		printf "\n" || return 1
-	done >.git/logs/refs/heads/reflogskip &&
-	git rev-parse reflogskip@{73} >actual &&
-	echo ${zf}03 >expect &&
-	test_cmp expect actual
-'
-
 test_expect_success 'no segfaults for reflog containing non-commit sha1s' '
 	git update-ref --create-reflog -m "Creating ref" \
 		refs/tests/tree-in-reflog HEAD &&
@@ -397,18 +367,6 @@ test_expect_failure 'reflog with non-commit entries displays all entries' '
 	test_line_count = 3 actual
 '
 
-# This test takes a lock on an individual ref; this is not supported in
-# reftable.
-test_expect_success REFFILES 'reflog expire operates on symref not referrent' '
-	git branch --create-reflog the_symref &&
-	git branch --create-reflog referrent &&
-	git update-ref referrent HEAD &&
-	git symbolic-ref refs/heads/the_symref refs/heads/referrent &&
-	test_when_finished "rm -f .git/refs/heads/referrent.lock" &&
-	touch .git/refs/heads/referrent.lock &&
-	git reflog expire --expire=all the_symref
-'
-
 test_expect_success 'continue walking past root commits' '
 	git init orphanage &&
 	(
-- 
gitgitgadget


^ permalink raw reply related

* [PATCH v2 06/12] t1406: move reffiles specific tests to t0600
From: John Cai via GitGitGadget @ 2024-01-19 20:18 UTC (permalink / raw)
  To: git; +Cc: Patrick Steinhardt, John Cai, John Cai
In-Reply-To: <pull.1647.v2.git.git.1705695540.gitgitgadget@gmail.com>

From: John Cai <johncai86@gmail.com>

Move this test to t0600 with the rest of the tests that are specific to
reffiles. This test reaches into reflog directories manually, and so are
specific to reffiles.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 t/t0600-reffiles-backend.sh   | 48 +++++++++++++++++++++++++++++++++++
 t/t1407-worktree-ref-store.sh | 37 ---------------------------
 2 files changed, 48 insertions(+), 37 deletions(-)

diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
index 2f910bd76ad..3bd28699d53 100755
--- a/t/t0600-reffiles-backend.sh
+++ b/t/t0600-reffiles-backend.sh
@@ -260,4 +260,52 @@ test_expect_success 'delete fails cleanly if packed-refs.new write fails' '
 	test_cmp unchanged actual
 '
 
+RWT="test-tool ref-store worktree:wt"
+RMAIN="test-tool ref-store worktree:main"
+
+test_expect_success 'setup worktree' '
+	test_commit first &&
+	git worktree add -b wt-main wt &&
+	(
+		cd wt &&
+		test_commit second
+	)
+'
+
+# Some refs (refs/bisect/*, pseudorefs) are kept per worktree, so they should
+# only appear in the for-each-reflog output if it is called from the correct
+# worktree, which is exercised in this test. This test is poorly written for
+# mulitple reasons: 1) it creates invalidly formatted log entres. 2) it uses
+# direct FS access for creating the reflogs. 3) PSEUDO-WT and refs/bisect/random
+# do not create reflogs by default, so it is not testing a realistic scenario.
+test_expect_success 'for_each_reflog()' '
+	echo $ZERO_OID > .git/logs/PSEUDO-MAIN &&
+	mkdir -p     .git/logs/refs/bisect &&
+	echo $ZERO_OID > .git/logs/refs/bisect/random &&
+
+	echo $ZERO_OID > .git/worktrees/wt/logs/PSEUDO-WT &&
+	mkdir -p     .git/worktrees/wt/logs/refs/bisect &&
+	echo $ZERO_OID > .git/worktrees/wt/logs/refs/bisect/wt-random &&
+
+	$RWT for-each-reflog | cut -d" " -f 2- | sort >actual &&
+	cat >expected <<-\EOF &&
+	HEAD 0x1
+	PSEUDO-WT 0x0
+	refs/bisect/wt-random 0x0
+	refs/heads/main 0x0
+	refs/heads/wt-main 0x0
+	EOF
+	test_cmp expected actual &&
+
+	$RMAIN for-each-reflog | cut -d" " -f 2- | sort >actual &&
+	cat >expected <<-\EOF &&
+	HEAD 0x1
+	PSEUDO-MAIN 0x0
+	refs/bisect/random 0x0
+	refs/heads/main 0x0
+	refs/heads/wt-main 0x0
+	EOF
+	test_cmp expected actual
+'
+
 test_done
diff --git a/t/t1407-worktree-ref-store.sh b/t/t1407-worktree-ref-store.sh
index 05b1881c591..48b1c92a414 100755
--- a/t/t1407-worktree-ref-store.sh
+++ b/t/t1407-worktree-ref-store.sh
@@ -53,41 +53,4 @@ test_expect_success 'create_symref(FOO, refs/heads/main)' '
 	test_cmp expected actual
 '
 
-# Some refs (refs/bisect/*, pseudorefs) are kept per worktree, so they should
-# only appear in the for-each-reflog output if it is called from the correct
-# worktree, which is exercised in this test. This test is poorly written (and
-# therefore marked REFFILES) for mulitple reasons: 1) it creates invalidly
-# formatted log entres. 2) it uses direct FS access for creating the reflogs. 3)
-# PSEUDO-WT and refs/bisect/random do not create reflogs by default, so it is
-# not testing a realistic scenario.
-test_expect_success REFFILES 'for_each_reflog()' '
-	echo $ZERO_OID > .git/logs/PSEUDO-MAIN &&
-	mkdir -p     .git/logs/refs/bisect &&
-	echo $ZERO_OID > .git/logs/refs/bisect/random &&
-
-	echo $ZERO_OID > .git/worktrees/wt/logs/PSEUDO-WT &&
-	mkdir -p     .git/worktrees/wt/logs/refs/bisect &&
-	echo $ZERO_OID > .git/worktrees/wt/logs/refs/bisect/wt-random &&
-
-	$RWT for-each-reflog | cut -d" " -f 2- | sort >actual &&
-	cat >expected <<-\EOF &&
-	HEAD 0x1
-	PSEUDO-WT 0x0
-	refs/bisect/wt-random 0x0
-	refs/heads/main 0x0
-	refs/heads/wt-main 0x0
-	EOF
-	test_cmp expected actual &&
-
-	$RMAIN for-each-reflog | cut -d" " -f 2- | sort >actual &&
-	cat >expected <<-\EOF &&
-	HEAD 0x1
-	PSEUDO-MAIN 0x0
-	refs/bisect/random 0x0
-	refs/heads/main 0x0
-	refs/heads/wt-main 0x0
-	EOF
-	test_cmp expected actual
-'
-
 test_done
-- 
gitgitgadget


^ permalink raw reply related

* [PATCH v2 04/12] t1404: move reffiles specific tests to t0600
From: John Cai via GitGitGadget @ 2024-01-19 20:18 UTC (permalink / raw)
  To: git; +Cc: Patrick Steinhardt, John Cai, John Cai
In-Reply-To: <pull.1647.v2.git.git.1705695540.gitgitgadget@gmail.com>

From: John Cai <johncai86@gmail.com>

These tests modify loose refs manually and are specific to the reffiles
backend. Move these to t0600 to be part of a test suite of reffiles
specific tests.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 t/t0600-reffiles-backend.sh  | 263 +++++++++++++++++++++++++++++++++++
 t/t1404-update-ref-errors.sh | 237 -------------------------------
 2 files changed, 263 insertions(+), 237 deletions(-)
 create mode 100755 t/t0600-reffiles-backend.sh

diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
new file mode 100755
index 00000000000..2f910bd76ad
--- /dev/null
+++ b/t/t0600-reffiles-backend.sh
@@ -0,0 +1,263 @@
+#!/bin/sh
+
+test_description='Test reffiles backend'
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
+
+if ! test_have_prereq REFFILES
+	then
+		skip_all='skipping reffiles specific tests'
+		test_done
+fi
+
+test_expect_success 'setup' '
+	git commit --allow-empty -m Initial &&
+	C=$(git rev-parse HEAD) &&
+	git commit --allow-empty -m Second &&
+	D=$(git rev-parse HEAD) &&
+	git commit --allow-empty -m Third &&
+	E=$(git rev-parse HEAD)
+'
+
+test_expect_success 'empty directory should not fool rev-parse' '
+	prefix=refs/e-rev-parse &&
+	git update-ref $prefix/foo $C &&
+	git pack-refs --all &&
+	mkdir -p .git/$prefix/foo/bar/baz &&
+	echo "$C" >expected &&
+	git rev-parse $prefix/foo >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'empty directory should not fool for-each-ref' '
+	prefix=refs/e-for-each-ref &&
+	git update-ref $prefix/foo $C &&
+	git for-each-ref $prefix >expected &&
+	git pack-refs --all &&
+	mkdir -p .git/$prefix/foo/bar/baz &&
+	git for-each-ref $prefix >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'empty directory should not fool create' '
+	prefix=refs/e-create &&
+	mkdir -p .git/$prefix/foo/bar/baz &&
+	printf "create %s $C\n" $prefix/foo |
+	git update-ref --stdin
+'
+
+test_expect_success 'empty directory should not fool verify' '
+	prefix=refs/e-verify &&
+	git update-ref $prefix/foo $C &&
+	git pack-refs --all &&
+	mkdir -p .git/$prefix/foo/bar/baz &&
+	printf "verify %s $C\n" $prefix/foo |
+	git update-ref --stdin
+'
+
+test_expect_success 'empty directory should not fool 1-arg update' '
+	prefix=refs/e-update-1 &&
+	git update-ref $prefix/foo $C &&
+	git pack-refs --all &&
+	mkdir -p .git/$prefix/foo/bar/baz &&
+	printf "update %s $D\n" $prefix/foo |
+	git update-ref --stdin
+'
+
+test_expect_success 'empty directory should not fool 2-arg update' '
+	prefix=refs/e-update-2 &&
+	git update-ref $prefix/foo $C &&
+	git pack-refs --all &&
+	mkdir -p .git/$prefix/foo/bar/baz &&
+	printf "update %s $D $C\n" $prefix/foo |
+	git update-ref --stdin
+'
+
+test_expect_success 'empty directory should not fool 0-arg delete' '
+	prefix=refs/e-delete-0 &&
+	git update-ref $prefix/foo $C &&
+	git pack-refs --all &&
+	mkdir -p .git/$prefix/foo/bar/baz &&
+	printf "delete %s\n" $prefix/foo |
+	git update-ref --stdin
+'
+
+test_expect_success 'empty directory should not fool 1-arg delete' '
+	prefix=refs/e-delete-1 &&
+	git update-ref $prefix/foo $C &&
+	git pack-refs --all &&
+	mkdir -p .git/$prefix/foo/bar/baz &&
+	printf "delete %s $C\n" $prefix/foo |
+	git update-ref --stdin
+'
+
+test_expect_success 'non-empty directory blocks create' '
+	prefix=refs/ne-create &&
+	mkdir -p .git/$prefix/foo/bar &&
+	: >.git/$prefix/foo/bar/baz.lock &&
+	test_when_finished "rm -f .git/$prefix/foo/bar/baz.lock" &&
+	cat >expected <<-EOF &&
+	fatal: cannot lock ref $SQ$prefix/foo$SQ: there is a non-empty directory $SQ.git/$prefix/foo$SQ blocking reference $SQ$prefix/foo$SQ
+	EOF
+	printf "%s\n" "update $prefix/foo $C" |
+	test_must_fail git update-ref --stdin 2>output.err &&
+	test_cmp expected output.err &&
+	cat >expected <<-EOF &&
+	fatal: cannot lock ref $SQ$prefix/foo$SQ: unable to resolve reference $SQ$prefix/foo$SQ
+	EOF
+	printf "%s\n" "update $prefix/foo $D $C" |
+	test_must_fail git update-ref --stdin 2>output.err &&
+	test_cmp expected output.err
+'
+
+test_expect_success 'broken reference blocks create' '
+	prefix=refs/broken-create &&
+	mkdir -p .git/$prefix &&
+	echo "gobbledigook" >.git/$prefix/foo &&
+	test_when_finished "rm -f .git/$prefix/foo" &&
+	cat >expected <<-EOF &&
+	fatal: cannot lock ref $SQ$prefix/foo$SQ: unable to resolve reference $SQ$prefix/foo$SQ: reference broken
+	EOF
+	printf "%s\n" "update $prefix/foo $C" |
+	test_must_fail git update-ref --stdin 2>output.err &&
+	test_cmp expected output.err &&
+	cat >expected <<-EOF &&
+	fatal: cannot lock ref $SQ$prefix/foo$SQ: unable to resolve reference $SQ$prefix/foo$SQ: reference broken
+	EOF
+	printf "%s\n" "update $prefix/foo $D $C" |
+	test_must_fail git update-ref --stdin 2>output.err &&
+	test_cmp expected output.err
+'
+
+test_expect_success 'non-empty directory blocks indirect create' '
+	prefix=refs/ne-indirect-create &&
+	git symbolic-ref $prefix/symref $prefix/foo &&
+	mkdir -p .git/$prefix/foo/bar &&
+	: >.git/$prefix/foo/bar/baz.lock &&
+	test_when_finished "rm -f .git/$prefix/foo/bar/baz.lock" &&
+	cat >expected <<-EOF &&
+	fatal: cannot lock ref $SQ$prefix/symref$SQ: there is a non-empty directory $SQ.git/$prefix/foo$SQ blocking reference $SQ$prefix/foo$SQ
+	EOF
+	printf "%s\n" "update $prefix/symref $C" |
+	test_must_fail git update-ref --stdin 2>output.err &&
+	test_cmp expected output.err &&
+	cat >expected <<-EOF &&
+	fatal: cannot lock ref $SQ$prefix/symref$SQ: unable to resolve reference $SQ$prefix/foo$SQ
+	EOF
+	printf "%s\n" "update $prefix/symref $D $C" |
+	test_must_fail git update-ref --stdin 2>output.err &&
+	test_cmp expected output.err
+'
+
+test_expect_success 'broken reference blocks indirect create' '
+	prefix=refs/broken-indirect-create &&
+	git symbolic-ref $prefix/symref $prefix/foo &&
+	echo "gobbledigook" >.git/$prefix/foo &&
+	test_when_finished "rm -f .git/$prefix/foo" &&
+	cat >expected <<-EOF &&
+	fatal: cannot lock ref $SQ$prefix/symref$SQ: unable to resolve reference $SQ$prefix/foo$SQ: reference broken
+	EOF
+	printf "%s\n" "update $prefix/symref $C" |
+	test_must_fail git update-ref --stdin 2>output.err &&
+	test_cmp expected output.err &&
+	cat >expected <<-EOF &&
+	fatal: cannot lock ref $SQ$prefix/symref$SQ: unable to resolve reference $SQ$prefix/foo$SQ: reference broken
+	EOF
+	printf "%s\n" "update $prefix/symref $D $C" |
+	test_must_fail git update-ref --stdin 2>output.err &&
+	test_cmp expected output.err
+'
+
+test_expect_success 'no bogus intermediate values during delete' '
+	prefix=refs/slow-transaction &&
+	# Set up a reference with differing loose and packed versions:
+	git update-ref $prefix/foo $C &&
+	git pack-refs --all &&
+	git update-ref $prefix/foo $D &&
+	# Now try to update the reference, but hold the `packed-refs` lock
+	# for a while to see what happens while the process is blocked:
+	: >.git/packed-refs.lock &&
+	test_when_finished "rm -f .git/packed-refs.lock" &&
+	{
+		# Note: the following command is intentionally run in the
+		# background. We increase the timeout so that `update-ref`
+		# attempts to acquire the `packed-refs` lock for much longer
+		# than it takes for us to do the check then delete it:
+		git -c core.packedrefstimeout=30000 update-ref -d $prefix/foo &
+	} &&
+	pid2=$! &&
+	# Give update-ref plenty of time to get to the point where it tries
+	# to lock packed-refs:
+	sleep 1 &&
+	# Make sure that update-ref did not complete despite the lock:
+	kill -0 $pid2 &&
+	# Verify that the reference still has its old value:
+	sha1=$(git rev-parse --verify --quiet $prefix/foo || echo undefined) &&
+	case "$sha1" in
+	$D)
+		# This is what we hope for; it means that nothing
+		# user-visible has changed yet.
+		: ;;
+	undefined)
+		# This is not correct; it means the deletion has happened
+		# already even though update-ref should not have been
+		# able to acquire the lock yet.
+		echo "$prefix/foo deleted prematurely" &&
+		break
+		;;
+	$C)
+		# This value should never be seen. Probably the loose
+		# reference has been deleted but the packed reference
+		# is still there:
+		echo "$prefix/foo incorrectly observed to be C" &&
+		break
+		;;
+	*)
+		# WTF?
+		echo "unexpected value observed for $prefix/foo: $sha1" &&
+		break
+		;;
+	esac >out &&
+	rm -f .git/packed-refs.lock &&
+	wait $pid2 &&
+	test_must_be_empty out &&
+	test_must_fail git rev-parse --verify --quiet $prefix/foo
+'
+
+test_expect_success 'delete fails cleanly if packed-refs file is locked' '
+	prefix=refs/locked-packed-refs &&
+	# Set up a reference with differing loose and packed versions:
+	git update-ref $prefix/foo $C &&
+	git pack-refs --all &&
+	git update-ref $prefix/foo $D &&
+	git for-each-ref $prefix >unchanged &&
+	# Now try to delete it while the `packed-refs` lock is held:
+	: >.git/packed-refs.lock &&
+	test_when_finished "rm -f .git/packed-refs.lock" &&
+	test_must_fail git update-ref -d $prefix/foo >out 2>err &&
+	git for-each-ref $prefix >actual &&
+	test_grep "Unable to create $SQ.*packed-refs.lock$SQ: " err &&
+	test_cmp unchanged actual
+'
+
+test_expect_success 'delete fails cleanly if packed-refs.new write fails' '
+	# Setup and expectations are similar to the test above.
+	prefix=refs/failed-packed-refs &&
+	git update-ref $prefix/foo $C &&
+	git pack-refs --all &&
+	git update-ref $prefix/foo $D &&
+	git for-each-ref $prefix >unchanged &&
+	# This should not happen in practice, but it is an easy way to get a
+	# reliable error (we open with create_tempfile(), which uses O_EXCL).
+	: >.git/packed-refs.new &&
+	test_when_finished "rm -f .git/packed-refs.new" &&
+	test_must_fail git update-ref -d $prefix/foo &&
+	git for-each-ref $prefix >actual &&
+	test_cmp unchanged actual
+'
+
+test_done
diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh
index 0369beea33b..00b70137053 100755
--- a/t/t1404-update-ref-errors.sh
+++ b/t/t1404-update-ref-errors.sh
@@ -191,78 +191,6 @@ test_expect_success 'one new ref is a simple prefix of another' '
 
 '
 
-test_expect_success REFFILES 'empty directory should not fool rev-parse' '
-	prefix=refs/e-rev-parse &&
-	git update-ref $prefix/foo $C &&
-	git pack-refs --all &&
-	mkdir -p .git/$prefix/foo/bar/baz &&
-	echo "$C" >expected &&
-	git rev-parse $prefix/foo >actual &&
-	test_cmp expected actual
-'
-
-test_expect_success REFFILES 'empty directory should not fool for-each-ref' '
-	prefix=refs/e-for-each-ref &&
-	git update-ref $prefix/foo $C &&
-	git for-each-ref $prefix >expected &&
-	git pack-refs --all &&
-	mkdir -p .git/$prefix/foo/bar/baz &&
-	git for-each-ref $prefix >actual &&
-	test_cmp expected actual
-'
-
-test_expect_success REFFILES 'empty directory should not fool create' '
-	prefix=refs/e-create &&
-	mkdir -p .git/$prefix/foo/bar/baz &&
-	printf "create %s $C\n" $prefix/foo |
-	git update-ref --stdin
-'
-
-test_expect_success REFFILES 'empty directory should not fool verify' '
-	prefix=refs/e-verify &&
-	git update-ref $prefix/foo $C &&
-	git pack-refs --all &&
-	mkdir -p .git/$prefix/foo/bar/baz &&
-	printf "verify %s $C\n" $prefix/foo |
-	git update-ref --stdin
-'
-
-test_expect_success REFFILES 'empty directory should not fool 1-arg update' '
-	prefix=refs/e-update-1 &&
-	git update-ref $prefix/foo $C &&
-	git pack-refs --all &&
-	mkdir -p .git/$prefix/foo/bar/baz &&
-	printf "update %s $D\n" $prefix/foo |
-	git update-ref --stdin
-'
-
-test_expect_success REFFILES 'empty directory should not fool 2-arg update' '
-	prefix=refs/e-update-2 &&
-	git update-ref $prefix/foo $C &&
-	git pack-refs --all &&
-	mkdir -p .git/$prefix/foo/bar/baz &&
-	printf "update %s $D $C\n" $prefix/foo |
-	git update-ref --stdin
-'
-
-test_expect_success REFFILES 'empty directory should not fool 0-arg delete' '
-	prefix=refs/e-delete-0 &&
-	git update-ref $prefix/foo $C &&
-	git pack-refs --all &&
-	mkdir -p .git/$prefix/foo/bar/baz &&
-	printf "delete %s\n" $prefix/foo |
-	git update-ref --stdin
-'
-
-test_expect_success REFFILES 'empty directory should not fool 1-arg delete' '
-	prefix=refs/e-delete-1 &&
-	git update-ref $prefix/foo $C &&
-	git pack-refs --all &&
-	mkdir -p .git/$prefix/foo/bar/baz &&
-	printf "delete %s $C\n" $prefix/foo |
-	git update-ref --stdin
-'
-
 test_expect_success REFFILES 'D/F conflict prevents add long + delete short' '
 	df_test refs/df-al-ds --add-del foo/bar foo
 '
@@ -468,169 +396,4 @@ test_expect_success 'incorrect old value blocks indirect no-deref delete' '
 	test_cmp expected output.err
 '
 
-test_expect_success REFFILES 'non-empty directory blocks create' '
-	prefix=refs/ne-create &&
-	mkdir -p .git/$prefix/foo/bar &&
-	: >.git/$prefix/foo/bar/baz.lock &&
-	test_when_finished "rm -f .git/$prefix/foo/bar/baz.lock" &&
-	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/foo$SQ: there is a non-empty directory $SQ.git/$prefix/foo$SQ blocking reference $SQ$prefix/foo$SQ
-	EOF
-	printf "%s\n" "update $prefix/foo $C" |
-	test_must_fail git update-ref --stdin 2>output.err &&
-	test_cmp expected output.err &&
-	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/foo$SQ: unable to resolve reference $SQ$prefix/foo$SQ
-	EOF
-	printf "%s\n" "update $prefix/foo $D $C" |
-	test_must_fail git update-ref --stdin 2>output.err &&
-	test_cmp expected output.err
-'
-
-test_expect_success REFFILES 'broken reference blocks create' '
-	prefix=refs/broken-create &&
-	mkdir -p .git/$prefix &&
-	echo "gobbledigook" >.git/$prefix/foo &&
-	test_when_finished "rm -f .git/$prefix/foo" &&
-	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/foo$SQ: unable to resolve reference $SQ$prefix/foo$SQ: reference broken
-	EOF
-	printf "%s\n" "update $prefix/foo $C" |
-	test_must_fail git update-ref --stdin 2>output.err &&
-	test_cmp expected output.err &&
-	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/foo$SQ: unable to resolve reference $SQ$prefix/foo$SQ: reference broken
-	EOF
-	printf "%s\n" "update $prefix/foo $D $C" |
-	test_must_fail git update-ref --stdin 2>output.err &&
-	test_cmp expected output.err
-'
-
-test_expect_success REFFILES 'non-empty directory blocks indirect create' '
-	prefix=refs/ne-indirect-create &&
-	git symbolic-ref $prefix/symref $prefix/foo &&
-	mkdir -p .git/$prefix/foo/bar &&
-	: >.git/$prefix/foo/bar/baz.lock &&
-	test_when_finished "rm -f .git/$prefix/foo/bar/baz.lock" &&
-	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/symref$SQ: there is a non-empty directory $SQ.git/$prefix/foo$SQ blocking reference $SQ$prefix/foo$SQ
-	EOF
-	printf "%s\n" "update $prefix/symref $C" |
-	test_must_fail git update-ref --stdin 2>output.err &&
-	test_cmp expected output.err &&
-	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/symref$SQ: unable to resolve reference $SQ$prefix/foo$SQ
-	EOF
-	printf "%s\n" "update $prefix/symref $D $C" |
-	test_must_fail git update-ref --stdin 2>output.err &&
-	test_cmp expected output.err
-'
-
-test_expect_success REFFILES 'broken reference blocks indirect create' '
-	prefix=refs/broken-indirect-create &&
-	git symbolic-ref $prefix/symref $prefix/foo &&
-	echo "gobbledigook" >.git/$prefix/foo &&
-	test_when_finished "rm -f .git/$prefix/foo" &&
-	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/symref$SQ: unable to resolve reference $SQ$prefix/foo$SQ: reference broken
-	EOF
-	printf "%s\n" "update $prefix/symref $C" |
-	test_must_fail git update-ref --stdin 2>output.err &&
-	test_cmp expected output.err &&
-	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/symref$SQ: unable to resolve reference $SQ$prefix/foo$SQ: reference broken
-	EOF
-	printf "%s\n" "update $prefix/symref $D $C" |
-	test_must_fail git update-ref --stdin 2>output.err &&
-	test_cmp expected output.err
-'
-
-test_expect_success REFFILES 'no bogus intermediate values during delete' '
-	prefix=refs/slow-transaction &&
-	# Set up a reference with differing loose and packed versions:
-	git update-ref $prefix/foo $C &&
-	git pack-refs --all &&
-	git update-ref $prefix/foo $D &&
-	# Now try to update the reference, but hold the `packed-refs` lock
-	# for a while to see what happens while the process is blocked:
-	: >.git/packed-refs.lock &&
-	test_when_finished "rm -f .git/packed-refs.lock" &&
-	{
-		# Note: the following command is intentionally run in the
-		# background. We increase the timeout so that `update-ref`
-		# attempts to acquire the `packed-refs` lock for much longer
-		# than it takes for us to do the check then delete it:
-		git -c core.packedrefstimeout=30000 update-ref -d $prefix/foo &
-	} &&
-	pid2=$! &&
-	# Give update-ref plenty of time to get to the point where it tries
-	# to lock packed-refs:
-	sleep 1 &&
-	# Make sure that update-ref did not complete despite the lock:
-	kill -0 $pid2 &&
-	# Verify that the reference still has its old value:
-	sha1=$(git rev-parse --verify --quiet $prefix/foo || echo undefined) &&
-	case "$sha1" in
-	$D)
-		# This is what we hope for; it means that nothing
-		# user-visible has changed yet.
-		: ;;
-	undefined)
-		# This is not correct; it means the deletion has happened
-		# already even though update-ref should not have been
-		# able to acquire the lock yet.
-		echo "$prefix/foo deleted prematurely" &&
-		break
-		;;
-	$C)
-		# This value should never be seen. Probably the loose
-		# reference has been deleted but the packed reference
-		# is still there:
-		echo "$prefix/foo incorrectly observed to be C" &&
-		break
-		;;
-	*)
-		# WTF?
-		echo "unexpected value observed for $prefix/foo: $sha1" &&
-		break
-		;;
-	esac >out &&
-	rm -f .git/packed-refs.lock &&
-	wait $pid2 &&
-	test_must_be_empty out &&
-	test_must_fail git rev-parse --verify --quiet $prefix/foo
-'
-
-test_expect_success REFFILES 'delete fails cleanly if packed-refs file is locked' '
-	prefix=refs/locked-packed-refs &&
-	# Set up a reference with differing loose and packed versions:
-	git update-ref $prefix/foo $C &&
-	git pack-refs --all &&
-	git update-ref $prefix/foo $D &&
-	git for-each-ref $prefix >unchanged &&
-	# Now try to delete it while the `packed-refs` lock is held:
-	: >.git/packed-refs.lock &&
-	test_when_finished "rm -f .git/packed-refs.lock" &&
-	test_must_fail git update-ref -d $prefix/foo >out 2>err &&
-	git for-each-ref $prefix >actual &&
-	test_grep "Unable to create $SQ.*packed-refs.lock$SQ: " err &&
-	test_cmp unchanged actual
-'
-
-test_expect_success REFFILES 'delete fails cleanly if packed-refs.new write fails' '
-	# Setup and expectations are similar to the test above.
-	prefix=refs/failed-packed-refs &&
-	git update-ref $prefix/foo $C &&
-	git pack-refs --all &&
-	git update-ref $prefix/foo $D &&
-	git for-each-ref $prefix >unchanged &&
-	# This should not happen in practice, but it is an easy way to get a
-	# reliable error (we open with create_tempfile(), which uses O_EXCL).
-	: >.git/packed-refs.new &&
-	test_when_finished "rm -f .git/packed-refs.new" &&
-	test_must_fail git update-ref -d $prefix/foo &&
-	git for-each-ref $prefix >actual &&
-	test_cmp unchanged actual
-'
-
 test_done
-- 
gitgitgadget


^ permalink raw reply related

* [PATCH v2 05/12] t1405: move reffiles specific tests to t0601
From: John Cai via GitGitGadget @ 2024-01-19 20:18 UTC (permalink / raw)
  To: git; +Cc: Patrick Steinhardt, John Cai, John Cai
In-Reply-To: <pull.1647.v2.git.git.1705695540.gitgitgadget@gmail.com>

From: John Cai <johncai86@gmail.com>

Move this test to t0601 with other reffiles specific pack-refs tests
since it is reffiles specific in that it looks into the loose refs
directory for an assertion.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 t/t0601-reffiles-pack-refs.sh | 8 ++++++++
 t/t1405-main-ref-store.sh     | 8 --------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh
index f7a3f693901..2e457c4f2df 100755
--- a/t/t0601-reffiles-pack-refs.sh
+++ b/t/t0601-reffiles-pack-refs.sh
@@ -32,6 +32,14 @@ test_expect_success 'prepare a trivial repository' '
 	HEAD=$(git rev-parse --verify HEAD)
 '
 
+test_expect_success 'pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE)' '
+	N=`find .git/refs -type f | wc -l` &&
+	test "$N" != 0 &&
+	test-tool ref-store main pack-refs PACK_REFS_PRUNE,PACK_REFS_ALL &&
+	N=`find .git/refs -type f` &&
+	test -z "$N"
+'
+
 SHA1=
 
 test_expect_success 'see if git show-ref works as expected' '
diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh
index 62c1eadb190..976bd71efb5 100755
--- a/t/t1405-main-ref-store.sh
+++ b/t/t1405-main-ref-store.sh
@@ -15,14 +15,6 @@ test_expect_success 'setup' '
 	test_commit one
 '
 
-test_expect_success REFFILES 'pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE)' '
-	N=`find .git/refs -type f | wc -l` &&
-	test "$N" != 0 &&
-	$RUN pack-refs PACK_REFS_PRUNE,PACK_REFS_ALL &&
-	N=`find .git/refs -type f` &&
-	test -z "$N"
-'
-
 test_expect_success 'create_symref(FOO, refs/heads/main)' '
 	$RUN create-symref FOO refs/heads/main nothing &&
 	echo refs/heads/main >expected &&
-- 
gitgitgadget


^ permalink raw reply related

* [PATCH v2 03/12] t1414: convert test to use Git commands instead of writing refs manually
From: John Cai via GitGitGadget @ 2024-01-19 20:18 UTC (permalink / raw)
  To: git; +Cc: Patrick Steinhardt, John Cai, John Cai
In-Reply-To: <pull.1647.v2.git.git.1705695540.gitgitgadget@gmail.com>

From: John Cai <johncai86@gmail.com>

This test can be re-written to use Git commands rather than writing a
manual ref in the reflog. This way this test no longer needs the
REFFILES prerequisite.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 t/t1414-reflog-walk.sh | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/t/t1414-reflog-walk.sh b/t/t1414-reflog-walk.sh
index ea64cecf47b..be6c3f472c1 100755
--- a/t/t1414-reflog-walk.sh
+++ b/t/t1414-reflog-walk.sh
@@ -121,13 +121,12 @@ test_expect_success 'min/max age uses entry date to limit' '
 
 # Create a situation where the reflog and ref database disagree about the latest
 # state of HEAD.
-test_expect_success REFFILES 'walk prefers reflog to ref tip' '
+test_expect_success 'walk prefers reflog to ref tip' '
+	test_commit A &&
+	test_commit B &&
+	git reflog delete HEAD@{0} &&
 	head=$(git rev-parse HEAD) &&
-	one=$(git rev-parse one) &&
-	ident="$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE" &&
-	echo "$head $one $ident	broken reflog entry" >>.git/logs/HEAD &&
-
-	echo $one >expect &&
+	git rev-parse A >expect &&
 	git log -g --format=%H -1 >actual &&
 	test_cmp expect actual
 '
-- 
gitgitgadget


^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox