* [PATCH v2 07/16] dir: create untracked_cache_invalidate_trimmed_path()
From: Jeff Hostetler via GitGitGadget @ 2024-02-23 3:18 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Jeff Hostetler, Jeff Hostetler,
Jeff Hostetler
In-Reply-To: <pull.1662.v2.git.1708658300.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Create a wrapper function for untracked_cache_invalidate_path()
that silently trims a trailing slash, if present, before calling
the wrapped function.
The untracked cache expects to be called with a pathname that
does not contain a trailing slash. This can make it inconvenient
for callers that have a directory path. Lets hide this complexity.
This will be used by a later commit in the FSMonitor code which
may receive directory pathnames from an FSEvent.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
dir.c | 20 ++++++++++++++++++++
dir.h | 7 +++++++
2 files changed, 27 insertions(+)
diff --git a/dir.c b/dir.c
index ac699542302..1157f3e43fa 100644
--- a/dir.c
+++ b/dir.c
@@ -3918,6 +3918,26 @@ void untracked_cache_invalidate_path(struct index_state *istate,
path, strlen(path));
}
+void untracked_cache_invalidate_trimmed_path(struct index_state *istate,
+ const char *path,
+ int safe_path)
+{
+ size_t len = strlen(path);
+
+ if (!len)
+ return; /* should not happen */
+
+ if (path[len - 1] != '/') {
+ untracked_cache_invalidate_path(istate, path, safe_path);
+ } else {
+ struct strbuf tmp = STRBUF_INIT;
+
+ strbuf_add(&tmp, path, len - 1);
+ untracked_cache_invalidate_path(istate, tmp.buf, safe_path);
+ strbuf_release(&tmp);
+ }
+}
+
void untracked_cache_remove_from_index(struct index_state *istate,
const char *path)
{
diff --git a/dir.h b/dir.h
index 98aa85fcc0e..45a7b9ec5f2 100644
--- a/dir.h
+++ b/dir.h
@@ -576,6 +576,13 @@ int cmp_dir_entry(const void *p1, const void *p2);
int check_dir_entry_contains(const struct dir_entry *out, const struct dir_entry *in);
void untracked_cache_invalidate_path(struct index_state *, const char *, int safe_path);
+/*
+ * Invalidate the untracked-cache for this path, but first strip
+ * off a trailing slash, if present.
+ */
+void untracked_cache_invalidate_trimmed_path(struct index_state *,
+ const char *path,
+ int safe_path);
void untracked_cache_remove_from_index(struct index_state *, const char *);
void untracked_cache_add_to_index(struct index_state *, const char *);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 06/16] fsmonitor: refactor refresh callback for non-directory events
From: Jeff Hostetler via GitGitGadget @ 2024-02-23 3:18 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Jeff Hostetler, Jeff Hostetler,
Jeff Hostetler
In-Reply-To: <pull.1662.v2.git.1708658300.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Move the code handle unqualified FSEvents (without a trailing slash)
into a helper function.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
fsmonitor.c | 67 +++++++++++++++++++++++++++++++----------------------
1 file changed, 39 insertions(+), 28 deletions(-)
diff --git a/fsmonitor.c b/fsmonitor.c
index 29cce32d81c..364198d258f 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -183,6 +183,43 @@ static int query_fsmonitor_hook(struct repository *r,
return result;
}
+static void handle_path_without_trailing_slash(
+ struct index_state *istate, const char *name, int pos)
+{
+ int i;
+
+ if (pos >= 0) {
+ /*
+ * We have an exact match for this path and can just
+ * invalidate it.
+ */
+ istate->cache[pos]->ce_flags &= ~CE_FSMONITOR_VALID;
+ } else {
+ /*
+ * The path is not a tracked file -or- it is a
+ * directory event on a platform that cannot
+ * distinguish between file and directory events in
+ * the event handler, such as Windows.
+ *
+ * Scan as if it is a directory and invalidate the
+ * cone under it. (But remember to ignore items
+ * between "name" and "name/", such as "name-" and
+ * "name.".
+ */
+ int len = strlen(name);
+ pos = -pos - 1;
+
+ for (i = pos; i < istate->cache_nr; i++) {
+ if (!starts_with(istate->cache[i]->name, name))
+ break;
+ if ((unsigned char)istate->cache[i]->name[len] > '/')
+ break;
+ if (istate->cache[i]->name[len] == '/')
+ istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
+ }
+ }
+}
+
/*
* The daemon can decorate directory events, such as a move or rename,
* by adding a trailing slash to the observed name. Use this to
@@ -225,7 +262,7 @@ static void handle_path_with_trailing_slash(
static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
{
- int i, len = strlen(name);
+ int len = strlen(name);
int pos = index_name_pos(istate, name, len);
trace_printf_key(&trace_fsmonitor,
@@ -240,34 +277,8 @@ static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
* for the untracked cache.
*/
name[len - 1] = '\0';
- } else if (pos >= 0) {
- /*
- * We have an exact match for this path and can just
- * invalidate it.
- */
- istate->cache[pos]->ce_flags &= ~CE_FSMONITOR_VALID;
} else {
- /*
- * The path is not a tracked file -or- it is a
- * directory event on a platform that cannot
- * distinguish between file and directory events in
- * the event handler, such as Windows.
- *
- * Scan as if it is a directory and invalidate the
- * cone under it. (But remember to ignore items
- * between "name" and "name/", such as "name-" and
- * "name.".
- */
- pos = -pos - 1;
-
- for (i = pos; i < istate->cache_nr; i++) {
- if (!starts_with(istate->cache[i]->name, name))
- break;
- if ((unsigned char)istate->cache[i]->name[len] > '/')
- break;
- if (istate->cache[i]->name[len] == '/')
- istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
- }
+ handle_path_without_trailing_slash(istate, name, pos);
}
/*
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 04/16] fsmonitor: refactor refresh callback on directory events
From: Jeff Hostetler via GitGitGadget @ 2024-02-23 3:18 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Jeff Hostetler, Jeff Hostetler,
Jeff Hostetler
In-Reply-To: <pull.1662.v2.git.1708658300.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Move the code to handle directory FSEvents (containing pathnames with
a trailing slash) into a helper function.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
fsmonitor.c | 52 ++++++++++++++++++++++++++++++----------------------
1 file changed, 30 insertions(+), 22 deletions(-)
diff --git a/fsmonitor.c b/fsmonitor.c
index f670c509378..6fecae9aeb2 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -183,6 +183,35 @@ static int query_fsmonitor_hook(struct repository *r,
return result;
}
+static void handle_path_with_trailing_slash(
+ struct index_state *istate, const char *name, int pos)
+{
+ int i;
+
+ /*
+ * The daemon can decorate directory events, such as
+ * moves or renames, with a trailing slash if the OS
+ * FS Event contains sufficient information, such as
+ * MacOS.
+ *
+ * Use this to invalidate the entire cone under that
+ * directory.
+ *
+ * We do not expect an exact match because the index
+ * does not normally contain directory entries, so we
+ * start at the insertion point and scan.
+ */
+ if (pos < 0)
+ pos = -pos - 1;
+
+ /* Mark all entries for the folder invalid */
+ 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;
+ }
+}
+
static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
{
int i, len = strlen(name);
@@ -193,28 +222,7 @@ static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
name, pos);
if (name[len - 1] == '/') {
- /*
- * The daemon can decorate directory events, such as
- * moves or renames, with a trailing slash if the OS
- * FS Event contains sufficient information, such as
- * MacOS.
- *
- * Use this to invalidate the entire cone under that
- * directory.
- *
- * We do not expect an exact match because the index
- * does not normally contain directory entries, so we
- * start at the insertion point and scan.
- */
- if (pos < 0)
- pos = -pos - 1;
-
- /* Mark all entries for the folder invalid */
- 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;
- }
+ handle_path_with_trailing_slash(istate, name, pos);
/*
* We need to remove the traling "/" from the path
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 05/16] fsmonitor: clarify handling of directory events in callback helper
From: Jeff Hostetler via GitGitGadget @ 2024-02-23 3:18 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Jeff Hostetler, Jeff Hostetler,
Jeff Hostetler
In-Reply-To: <pull.1662.v2.git.1708658300.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Improve documentation of the refresh callback helper function
used for directory FSEvents.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
fsmonitor.c | 37 ++++++++++++++++++++++++-------------
1 file changed, 24 insertions(+), 13 deletions(-)
diff --git a/fsmonitor.c b/fsmonitor.c
index 6fecae9aeb2..29cce32d81c 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -183,24 +183,35 @@ static int query_fsmonitor_hook(struct repository *r,
return result;
}
+/*
+ * The daemon can decorate directory events, such as a move or rename,
+ * by adding a trailing slash to the observed name. Use this to
+ * explicitly invalidate the entire cone under that directory.
+ *
+ * The daemon can only reliably do that if the OS FSEvent contains
+ * sufficient information in the event.
+ *
+ * macOS FSEvents have enough information.
+ *
+ * Other platforms may or may not be able to do it (and it might
+ * depend on the type of event (for example, a daemon could lstat() an
+ * observed pathname after a rename, but not after a delete)).
+ *
+ * If we find an exact match in the index for a path with a trailing
+ * slash, it means that we matched a sparse-index directory in a
+ * cone-mode sparse-checkout (since that's the only time we have
+ * directories in the index). We should never see this in practice
+ * (because sparse directories should not be present and therefore
+ * not generating FS events). Either way, we can treat them in the
+ * same way and just invalidate the cache-entry and the untracked
+ * cache (and in this case, the forward cache-entry scan won't find
+ * anything and it doesn't hurt to let it run).
+ */
static void handle_path_with_trailing_slash(
struct index_state *istate, const char *name, int pos)
{
int i;
- /*
- * The daemon can decorate directory events, such as
- * moves or renames, with a trailing slash if the OS
- * FS Event contains sufficient information, such as
- * MacOS.
- *
- * Use this to invalidate the entire cone under that
- * directory.
- *
- * We do not expect an exact match because the index
- * does not normally contain directory entries, so we
- * start at the insertion point and scan.
- */
if (pos < 0)
pos = -pos - 1;
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 02/16] t7527: add case-insensitve test for FSMonitor
From: Jeff Hostetler via GitGitGadget @ 2024-02-23 3:18 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Jeff Hostetler, Jeff Hostetler,
Jeff Hostetler
In-Reply-To: <pull.1662.v2.git.1708658300.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
The FSMonitor client code trusts the spelling of the pathnames in the
FSEvents received from the FSMonitor daemon. On case-insensitive file
systems, these OBSERVED pathnames may be spelled differently than the
EXPECTED pathnames listed in the .git/index. This causes a miss when
using `index_name_pos()` which expects the given case to be correct.
When this happens, the FSMonitor client code does not update the state
of the CE_FSMONITOR_VALID bit when refreshing the index (and before
starting to scan the worktree).
This results in modified files NOT being reported by `git status` when
there is a discrepancy in the case-spelling of a tracked file's
pathname.
This commit contains a (rather contrived) test case to demonstrate
this. A later commit in this series will update the FSMonitor client
code to recognize these discrepancies and update the CE_ bit accordingly.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
t/t7527-builtin-fsmonitor.sh | 217 +++++++++++++++++++++++++++++++++++
1 file changed, 217 insertions(+)
diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
index 363f9dc0e41..3d21295f789 100755
--- a/t/t7527-builtin-fsmonitor.sh
+++ b/t/t7527-builtin-fsmonitor.sh
@@ -1037,4 +1037,221 @@ test_expect_success 'split-index and FSMonitor work well together' '
)
'
+# The FSMonitor daemon reports the OBSERVED pathname of modified files
+# and thus contains the OBSERVED spelling on case-insensitive file
+# systems. The daemon does not (and should not) load the .git/index
+# file and therefore does not know the expected case-spelling. Since
+# it is possible for the user to create files/subdirectories with the
+# incorrect case, a modified file event for a tracked will not have
+# the EXPECTED case. This can cause `index_name_pos()` to incorrectly
+# report that the file is untracked. This causes the client to fail to
+# mark the file as possibly dirty (keeping the CE_FSMONITOR_VALID bit
+# set) so that `git status` will avoid inspecting it and thus not
+# present in the status output.
+#
+# The setup is a little contrived.
+#
+test_expect_success CASE_INSENSITIVE_FS 'fsmonitor subdir case wrong on disk' '
+ test_when_finished "stop_daemon_delete_repo subdir_case_wrong" &&
+
+ git init subdir_case_wrong &&
+ (
+ cd subdir_case_wrong &&
+ echo x >AAA &&
+ echo x >BBB &&
+
+ mkdir dir1 &&
+ echo x >dir1/file1 &&
+ mkdir dir1/dir2 &&
+ echo x >dir1/dir2/file2 &&
+ mkdir dir1/dir2/dir3 &&
+ echo x >dir1/dir2/dir3/file3 &&
+
+ echo x >yyy &&
+ echo x >zzz &&
+ git add . &&
+ git commit -m "data" &&
+
+ # This will cause "dir1/" and everything under it
+ # to be deleted.
+ git sparse-checkout set --cone --sparse-index &&
+
+ # Create dir2 with the wrong case and then let Git
+ # repopulate dir3 -- it will not correct the spelling
+ # of dir2.
+ mkdir dir1 &&
+ mkdir dir1/DIR2 &&
+ git sparse-checkout add dir1/dir2/dir3
+ ) &&
+
+ start_daemon -C subdir_case_wrong --tf "$PWD/subdir_case_wrong.trace" &&
+
+ # Enable FSMonitor in the client. Run enough commands for
+ # the .git/index to sync up with the daemon with everything
+ # marked clean.
+ git -C subdir_case_wrong config core.fsmonitor true &&
+ git -C subdir_case_wrong update-index --fsmonitor &&
+ git -C subdir_case_wrong status &&
+
+ # Make some files dirty so that FSMonitor gets FSEvents for
+ # each of them.
+ echo xx >>subdir_case_wrong/AAA &&
+ echo xx >>subdir_case_wrong/dir1/DIR2/dir3/file3 &&
+ echo xx >>subdir_case_wrong/zzz &&
+
+ GIT_TRACE_FSMONITOR="$PWD/subdir_case_wrong.log" \
+ git -C subdir_case_wrong --no-optional-locks status --short \
+ >"$PWD/subdir_case_wrong.out" &&
+
+ # "git status" should have gotten file events for each of
+ # the 3 files.
+ #
+ # "dir2" should be in the observed case on disk.
+ grep "fsmonitor_refresh_callback" \
+ <"$PWD/subdir_case_wrong.log" \
+ >"$PWD/subdir_case_wrong.log1" &&
+
+ grep -q "AAA.*pos 0" "$PWD/subdir_case_wrong.log1" &&
+ grep -q "zzz.*pos 6" "$PWD/subdir_case_wrong.log1" &&
+
+ grep -q "dir1/DIR2/dir3/file3.*pos -3" "$PWD/subdir_case_wrong.log1" &&
+
+ # The refresh-callbacks should have caused "git status" to clear
+ # the CE_FSMONITOR_VALID bit on each of those files and caused
+ # the worktree scan to visit them and mark them as modified.
+ grep -q " M AAA" "$PWD/subdir_case_wrong.out" &&
+ grep -q " M zzz" "$PWD/subdir_case_wrong.out" &&
+
+ # However, with the fsmonitor client bug, the "(pos -3)" causes
+ # the client to not update the bit and never rescan the file
+ # and therefore not report it as dirty.
+ ! grep -q " M dir1/dir2/dir3/file3" "$PWD/subdir_case_wrong.out"
+'
+
+test_expect_success CASE_INSENSITIVE_FS 'fsmonitor file case wrong on disk' '
+ test_when_finished "stop_daemon_delete_repo file_case_wrong" &&
+
+ git init file_case_wrong &&
+ (
+ cd file_case_wrong &&
+ echo x >AAA &&
+ echo x >BBB &&
+
+ mkdir dir1 &&
+ mkdir dir1/dir2 &&
+ mkdir dir1/dir2/dir3 &&
+ echo x >dir1/dir2/dir3/FILE-3-B &&
+ echo x >dir1/dir2/dir3/XXXX-3-X &&
+ echo x >dir1/dir2/dir3/file-3-a &&
+ echo x >dir1/dir2/dir3/yyyy-3-y &&
+ mkdir dir1/dir2/dir4 &&
+ echo x >dir1/dir2/dir4/FILE-4-A &&
+ echo x >dir1/dir2/dir4/XXXX-4-X &&
+ echo x >dir1/dir2/dir4/file-4-b &&
+ echo x >dir1/dir2/dir4/yyyy-4-y &&
+
+ echo x >yyy &&
+ echo x >zzz &&
+ git add . &&
+ git commit -m "data"
+ ) &&
+
+ start_daemon -C file_case_wrong --tf "$PWD/file_case_wrong.trace" &&
+
+ # Enable FSMonitor in the client. Run enough commands for
+ # the .git/index to sync up with the daemon with everything
+ # marked clean.
+ git -C file_case_wrong config core.fsmonitor true &&
+ git -C file_case_wrong update-index --fsmonitor &&
+ git -C file_case_wrong status &&
+
+ # Make some files dirty so that FSMonitor gets FSEvents for
+ # each of them.
+ echo xx >>file_case_wrong/AAA &&
+ echo xx >>file_case_wrong/zzz &&
+
+ # Rename some files so that FSMonitor sees a create and delete
+ # FSEvent for each. (A simple "mv foo FOO" is not portable
+ # between macOS and Windows. It works on both platforms, but makes
+ # the test messy, since (1) one platform updates "ctime" on the
+ # moved file and one does not and (2) it causes a directory event
+ # on one platform and not on the other which causes additional
+ # scanning during "git status" which causes a "H" vs "h" discrepancy
+ # in "git ls-files -f".) So old-school it and move it out of the
+ # way and copy it to the case-incorrect name so that we get fresh
+ # "ctime" and "mtime" values.
+
+ mv file_case_wrong/dir1/dir2/dir3/file-3-a file_case_wrong/dir1/dir2/dir3/ORIG &&
+ cp file_case_wrong/dir1/dir2/dir3/ORIG file_case_wrong/dir1/dir2/dir3/FILE-3-A &&
+ rm file_case_wrong/dir1/dir2/dir3/ORIG &&
+ mv file_case_wrong/dir1/dir2/dir4/FILE-4-A file_case_wrong/dir1/dir2/dir4/ORIG &&
+ cp file_case_wrong/dir1/dir2/dir4/ORIG file_case_wrong/dir1/dir2/dir4/file-4-a &&
+ rm file_case_wrong/dir1/dir2/dir4/ORIG &&
+
+ # Run status enough times to fully sync.
+ #
+ # The first instance should get the create and delete FSEvents
+ # for each pair. Status should update the index with a new FSM
+ # token (so the next invocation will not see data for these
+ # events).
+
+ GIT_TRACE_FSMONITOR="$PWD/file_case_wrong-try1.log" \
+ git -C file_case_wrong status --short \
+ >"$PWD/file_case_wrong-try1.out" &&
+ grep -q "fsmonitor_refresh_callback.*FILE-3-A.*pos -3" "$PWD/file_case_wrong-try1.log" &&
+ grep -q "fsmonitor_refresh_callback.*file-3-a.*pos 4" "$PWD/file_case_wrong-try1.log" &&
+ grep -q "fsmonitor_refresh_callback.*FILE-4-A.*pos 6" "$PWD/file_case_wrong-try1.log" &&
+ grep -q "fsmonitor_refresh_callback.*file-4-a.*pos -9" "$PWD/file_case_wrong-try1.log" &&
+
+ # FSM refresh will have invalidated the FSM bit and cause a regular
+ # (real) scan of these tracked files, so they should have "H" status.
+ # (We will not see a "h" status until the next refresh (on the next
+ # command).)
+
+ git -C file_case_wrong ls-files -f >"$PWD/file_case_wrong-lsf1.out" &&
+ grep -q "H dir1/dir2/dir3/file-3-a" "$PWD/file_case_wrong-lsf1.out" &&
+ grep -q "H dir1/dir2/dir4/FILE-4-A" "$PWD/file_case_wrong-lsf1.out" &&
+
+
+ # Try the status again. We assume that the above status command
+ # advanced the token so that the next one will not see those events.
+
+ GIT_TRACE_FSMONITOR="$PWD/file_case_wrong-try2.log" \
+ git -C file_case_wrong status --short \
+ >"$PWD/file_case_wrong-try2.out" &&
+ ! grep -q "fsmonitor_refresh_callback.*FILE-3-A.*pos" "$PWD/file_case_wrong-try2.log" &&
+ ! grep -q "fsmonitor_refresh_callback.*file-3-a.*pos" "$PWD/file_case_wrong-try2.log" &&
+ ! grep -q "fsmonitor_refresh_callback.*FILE-4-A.*pos" "$PWD/file_case_wrong-try2.log" &&
+ ! grep -q "fsmonitor_refresh_callback.*file-4-a.*pos" "$PWD/file_case_wrong-try2.log" &&
+
+ # FSM refresh saw nothing, so it will mark all files as valid,
+ # so they should now have "h" status.
+
+ git -C file_case_wrong ls-files -f >"$PWD/file_case_wrong-lsf2.out" &&
+ grep -q "h dir1/dir2/dir3/file-3-a" "$PWD/file_case_wrong-lsf2.out" &&
+ grep -q "h dir1/dir2/dir4/FILE-4-A" "$PWD/file_case_wrong-lsf2.out" &&
+
+
+ # We now have files with clean content, but with case-incorrect
+ # file names. Modify them to see if status properly reports
+ # them.
+
+ echo xx >>file_case_wrong/dir1/dir2/dir3/FILE-3-A &&
+ echo xx >>file_case_wrong/dir1/dir2/dir4/file-4-a &&
+
+ GIT_TRACE_FSMONITOR="$PWD/file_case_wrong-try3.log" \
+ git -C file_case_wrong --no-optional-locks status --short \
+ >"$PWD/file_case_wrong-try3.out" &&
+ # FSEvents are in observed case.
+ grep -q "fsmonitor_refresh_callback.*FILE-3-A.*pos -3" "$PWD/file_case_wrong-try3.log" &&
+ grep -q "fsmonitor_refresh_callback.*file-4-a.*pos -9" "$PWD/file_case_wrong-try3.log" &&
+
+ # Status should say these files are modified, but with the case
+ # bug, the "pos -3" cause the client to not update the FSM bit
+ # and never cause the file to be rescanned and therefore to not
+ # report it dirty.
+ ! grep -q " M dir1/dir2/dir3/file-3-a" "$PWD/file_case_wrong-try3.out" &&
+ ! grep -q " M dir1/dir2/dir4/FILE-4-A" "$PWD/file_case_wrong-try3.out"
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 03/16] t7527: temporarily disable case-insensitive tests
From: Jeff Hostetler via GitGitGadget @ 2024-02-23 3:18 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Jeff Hostetler, Jeff Hostetler,
Jeff Hostetler
In-Reply-To: <pull.1662.v2.git.1708658300.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Add non-existent "SKIPME" prereq to the case-insensitive tests.
The previous commit added test cases to demonstrate an error where
FSMonitor can get confused on a case-insensitive file system when the
on-disk spelling of a file or directory is wrong. Let's disable those
tests before we incrementally teach Git to properly recognize and
handle those types of problems (so that a bisect between here and the
final commit in this patch series won't throw a false alarm).
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
t/t7527-builtin-fsmonitor.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
index 3d21295f789..4acb547819c 100755
--- a/t/t7527-builtin-fsmonitor.sh
+++ b/t/t7527-builtin-fsmonitor.sh
@@ -1051,7 +1051,7 @@ test_expect_success 'split-index and FSMonitor work well together' '
#
# The setup is a little contrived.
#
-test_expect_success CASE_INSENSITIVE_FS 'fsmonitor subdir case wrong on disk' '
+test_expect_success SKIPME,CASE_INSENSITIVE_FS 'fsmonitor subdir case wrong on disk' '
test_when_finished "stop_daemon_delete_repo subdir_case_wrong" &&
git init subdir_case_wrong &&
@@ -1128,7 +1128,7 @@ test_expect_success CASE_INSENSITIVE_FS 'fsmonitor subdir case wrong on disk' '
! grep -q " M dir1/dir2/dir3/file3" "$PWD/subdir_case_wrong.out"
'
-test_expect_success CASE_INSENSITIVE_FS 'fsmonitor file case wrong on disk' '
+test_expect_success SKIPME,CASE_INSENSITIVE_FS 'fsmonitor file case wrong on disk' '
test_when_finished "stop_daemon_delete_repo file_case_wrong" &&
git init file_case_wrong &&
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 01/16] name-hash: add index_dir_find()
From: Jeff Hostetler via GitGitGadget @ 2024-02-23 3:18 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Jeff Hostetler, Jeff Hostetler,
Jeff Hostetler
In-Reply-To: <pull.1662.v2.git.1708658300.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Replace the index_dir_exists() function with index_dir_find() and
change the API to take an optional strbuf to return the canonical
spelling of the matched directory prefix.
Create an index_dir_exists() wrapper macro for existing callers.
The existing index_dir_exists() returns a boolean to indicate if
there is a case-insensitive match in the directory name-hash, but
it doesn't tell the caller the exact spelling of that match.
The new version also copies the matched spelling to a provided strbuf.
This lets the caller, for example, then call index_name_pos() with the
correct case to search the cache-entry array for the real insertion
position.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
name-hash.c | 9 ++++++++-
name-hash.h | 7 ++++++-
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/name-hash.c b/name-hash.c
index 251f036eef6..3a58ce03d9c 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -685,13 +685,20 @@ static int same_name(const struct cache_entry *ce, const char *name, int namelen
return slow_same_name(name, namelen, ce->name, len);
}
-int index_dir_exists(struct index_state *istate, const char *name, int namelen)
+int index_dir_find(struct index_state *istate, const char *name, int namelen,
+ struct strbuf *canonical_path)
{
struct dir_entry *dir;
lazy_init_name_hash(istate);
expand_to_path(istate, name, namelen, 0);
dir = find_dir_entry(istate, name, namelen);
+
+ if (canonical_path && dir && dir->nr) {
+ strbuf_reset(canonical_path);
+ strbuf_add(canonical_path, dir->name, dir->namelen);
+ }
+
return dir && dir->nr;
}
diff --git a/name-hash.h b/name-hash.h
index b1b4b0fb337..0cbfc428631 100644
--- a/name-hash.h
+++ b/name-hash.h
@@ -4,7 +4,12 @@
struct cache_entry;
struct index_state;
-int index_dir_exists(struct index_state *istate, const char *name, int namelen);
+
+int index_dir_find(struct index_state *istate, const char *name, int namelen,
+ struct strbuf *canonical_path);
+
+#define index_dir_exists(i, n, l) index_dir_find((i), (n), (l), NULL)
+
void adjust_dirname_case(struct index_state *istate, char *name);
struct cache_entry *index_file_exists(struct index_state *istate, const char *name, int namelen, int igncase);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 00/16] FSMonitor edge cases on case-insensitive file systems
From: Jeff Hostetler via GitGitGadget @ 2024-02-23 3:18 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.git.1707857541.gitgitgadget@gmail.com>
Here is version 2. I think I have addressed all of the comments on the first
version and have greatly consolidated/streamlined the "icase" code.
============== Fix FSMonitor client code to detect case-incorrect FSEvents
and map them to the canonical case expected by the index.
FSEvents are delivered to the FSMonitor daemon using the observed case which
may or may not match the expected case stored in the index for tracked files
and/or directories. This caused index_name_pos() to report a negative index
position (defined as the suggested insertion point). Since the value was
negative, the FSMonitor refresh lookup would not invalidate the
CE_FSMONITOR_VALID bit on the "expected" (case-insensitive-equivalent)
cache-entries. Therefore, git status would not report them as modified.
This was a fairly obscure problem and only happened when the case of a
sub-directory or a file was artificially changed.
This first runs the original lookup using the observed case. If that fails,
it assumes that the observed pathname refers to a file and uses the
case-insensitive name-hash hashmap to find an equivalent path (cache-entry)
in the index. If that fails, it assumes the pathname refers to a directory
and uses the case-insensitive dir-name-hash to find the equivalent directory
and then repeats the index_name_pos() lookup to find a directory or
suggested insertion point with the expected case.
Two new test cases were added to t7527 to demonstrate this.
Since this was rather obscure, I also added some additional tracing under
the GIT_TRACE_FSMONITOR key.
I also did considerable refactoring of the original code before adding the
new lookups.
Finally, I made more explicit the relationship between the FSEvents and the
(new) sparse-index directory cache-entries, since sparse-index was added
slightly after the FSMonitor feature.
Jeff Hostetler (16):
name-hash: add index_dir_find()
t7527: add case-insensitve test for FSMonitor
t7527: temporarily disable case-insensitive tests
fsmonitor: refactor refresh callback on directory events
fsmonitor: clarify handling of directory events in callback helper
fsmonitor: refactor refresh callback for non-directory events
dir: create untracked_cache_invalidate_trimmed_path()
fsmonitor: refactor untracked-cache invalidation
fsmonitor: move untracked invalidation into helper functions
fsmonitor: return invalidated cache-entry count on directory event
fsmonitor: remove custom loop from non-directory path handler
fsmonitor: return invalided cache-entry count on non-directory event
fsmonitor: trace the new invalidated cache-entry count
fsmonitor: support case-insensitive events
fsmonitor: refactor bit invalidation in refresh callback
t7527: update case-insenstive fsmonitor test
dir.c | 20 +++
dir.h | 7 +
fsmonitor.c | 299 ++++++++++++++++++++++++++++-------
name-hash.c | 9 +-
name-hash.h | 7 +-
t/t7527-builtin-fsmonitor.sh | 220 ++++++++++++++++++++++++++
6 files changed, 506 insertions(+), 56 deletions(-)
base-commit: f41f85c9ec8d4d46de0fd5fded88db94d3ec8c11
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1662%2Fjeffhostetler%2Ffsmonitor-ignore-case-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1662/jeffhostetler/fsmonitor-ignore-case-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1662
Range-diff vs v1:
1: 6f81e2e3060 < -: ----------- sparse-index: pass string length to index_file_exists()
2: 3464545fe3f ! 1: 03b07d9c25e name-hash: add index_dir_exists2()
@@ Metadata
Author: Jeff Hostetler <jeffhostetler@github.com>
## Commit message ##
- name-hash: add index_dir_exists2()
+ name-hash: add index_dir_find()
- Create a new version of index_dir_exists() to return the canonical
+ Replace the index_dir_exists() function with index_dir_find() and
+ change the API to take an optional strbuf to return the canonical
spelling of the matched directory prefix.
+ Create an index_dir_exists() wrapper macro for existing callers.
+
The existing index_dir_exists() returns a boolean to indicate if
there is a case-insensitive match in the directory name-hash, but
it doesn't tell the caller the exact spelling of that match.
@@ Commit message
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
## name-hash.c ##
-@@ name-hash.c: int index_dir_exists(struct index_state *istate, const char *name, int namelen)
- dir = find_dir_entry(istate, name, namelen);
- return dir && dir->nr;
+@@ name-hash.c: static int same_name(const struct cache_entry *ce, const char *name, int namelen
+ return slow_same_name(name, namelen, ce->name, len);
}
-+int index_dir_exists2(struct index_state *istate, const char *name, int namelen,
-+ struct strbuf *canonical_path)
-+{
-+ struct dir_entry *dir;
-+
-+ strbuf_init(canonical_path, namelen+1);
-+
-+ lazy_init_name_hash(istate);
-+ expand_to_path(istate, name, namelen, 0);
-+ dir = find_dir_entry(istate, name, namelen);
+
+-int index_dir_exists(struct index_state *istate, const char *name, int namelen)
++int index_dir_find(struct index_state *istate, const char *name, int namelen,
++ struct strbuf *canonical_path)
+ {
+ struct dir_entry *dir;
+
+ lazy_init_name_hash(istate);
+ expand_to_path(istate, name, namelen, 0);
+ dir = find_dir_entry(istate, name, namelen);
+
-+ if (dir && dir->nr)
++ if (canonical_path && dir && dir->nr) {
++ strbuf_reset(canonical_path);
+ strbuf_add(canonical_path, dir->name, dir->namelen);
++ }
+
-+ return dir && dir->nr;
-+}
+ return dir && dir->nr;
+ }
- void adjust_dirname_case(struct index_state *istate, char *name)
- {
## name-hash.h ##
-@@ name-hash.h: struct cache_entry;
+@@
+ struct cache_entry;
struct index_state;
- int index_dir_exists(struct index_state *istate, const char *name, int namelen);
-+int index_dir_exists2(struct index_state *istate, const char *name, int namelen,
-+ struct strbuf *canonical_path);
+-int index_dir_exists(struct index_state *istate, const char *name, int namelen);
++
++int index_dir_find(struct index_state *istate, const char *name, int namelen,
++ struct strbuf *canonical_path);
++
++#define index_dir_exists(i, n, l) index_dir_find((i), (n), (l), NULL)
++
void adjust_dirname_case(struct index_state *istate, char *name);
struct cache_entry *index_file_exists(struct index_state *istate, const char *name, int namelen, int igncase);
3: 272d7805f47 = 2: 7778cee1c10 t7527: add case-insensitve test for FSMonitor
-: ----------- > 3: dad079ade7f t7527: temporarily disable case-insensitive tests
4: 3fb8e0d0a7c ! 4: 5516670e30e fsmonitor: refactor refresh callback on directory events
@@ Metadata
## Commit message ##
fsmonitor: refactor refresh callback on directory events
+ Move the code to handle directory FSEvents (containing pathnames with
+ a trailing slash) into a helper function.
+
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
## fsmonitor.c ##
@@ fsmonitor.c: static int query_fsmonitor_hook(struct repository *r,
return result;
}
-+static void fsmonitor_refresh_callback_slash(
-+ struct index_state *istate, const char *name, int len, int pos)
++static void handle_path_with_trailing_slash(
++ struct index_state *istate, const char *name, int pos)
+{
+ int i;
+
@@ fsmonitor.c: static void fsmonitor_refresh_callback(struct index_state *istate,
- break;
- istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
- }
-+ fsmonitor_refresh_callback_slash(istate, name, len, pos);
++ handle_path_with_trailing_slash(istate, name, pos);
/*
* We need to remove the traling "/" from the path
6: 5b6f8bd1fe7 ! 5: c04fd4eae94 fsmonitor: clarify handling of directory events in callback
@@ Metadata
Author: Jeff Hostetler <jeffhostetler@github.com>
## Commit message ##
- fsmonitor: clarify handling of directory events in callback
+ fsmonitor: clarify handling of directory events in callback helper
+
+ Improve documentation of the refresh callback helper function
+ used for directory FSEvents.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
## fsmonitor.c ##
-@@ fsmonitor.c: static void fsmonitor_refresh_callback_unqualified(
- }
+@@ fsmonitor.c: static int query_fsmonitor_hook(struct repository *r,
+ return result;
}
--static void fsmonitor_refresh_callback_slash(
+/*
+ * The daemon can decorate directory events, such as a move or rename,
+ * by adding a trailing slash to the observed name. Use this to
@@ fsmonitor.c: static void fsmonitor_refresh_callback_unqualified(
+ * same way and just invalidate the cache-entry and the untracked
+ * cache (and in this case, the forward cache-entry scan won't find
+ * anything and it doesn't hurt to let it run).
-+ *
-+ * Return the number of cache-entries that we invalidated. We will
-+ * use this later to determine if we need to attempt a second
-+ * case-insensitive search.
+ */
-+static int fsmonitor_refresh_callback_slash(
- struct index_state *istate, const char *name, int len, int pos)
+ static void handle_path_with_trailing_slash(
+ struct index_state *istate, const char *name, int pos)
{
int i;
-+ int nr_in_cone = 0;
- /*
- * The daemon can decorate directory events, such as
@@ fsmonitor.c: static void fsmonitor_refresh_callback_unqualified(
if (pos < 0)
pos = -pos - 1;
-@@ fsmonitor.c: static void fsmonitor_refresh_callback_slash(
- if (!starts_with(istate->cache[i]->name, name))
- break;
- istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
-+ nr_in_cone++;
- }
-+
-+ return nr_in_cone;
- }
-
- static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
5: 0896d4af907 ! 6: 7ee6ca1aefd fsmonitor: refactor refresh callback for non-directory events
@@ Metadata
## Commit message ##
fsmonitor: refactor refresh callback for non-directory events
+ Move the code handle unqualified FSEvents (without a trailing slash)
+ into a helper function.
+
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
## fsmonitor.c ##
@@ fsmonitor.c: static int query_fsmonitor_hook(struct repository *r,
return result;
}
-+static void fsmonitor_refresh_callback_unqualified(
-+ struct index_state *istate, const char *name, int len, int pos)
++static void handle_path_without_trailing_slash(
++ struct index_state *istate, const char *name, int pos)
+{
+ int i;
+
@@ fsmonitor.c: static int query_fsmonitor_hook(struct repository *r,
+ * between "name" and "name/", such as "name-" and
+ * "name.".
+ */
++ int len = strlen(name);
+ pos = -pos - 1;
+
+ for (i = pos; i < istate->cache_nr; i++) {
@@ fsmonitor.c: static int query_fsmonitor_hook(struct repository *r,
+ }
+}
+
- static void fsmonitor_refresh_callback_slash(
- struct index_state *istate, const char *name, int len, int pos)
- {
-@@ fsmonitor.c: static void fsmonitor_refresh_callback_slash(
+ /*
+ * The daemon can decorate directory events, such as a move or rename,
+ * by adding a trailing slash to the observed name. Use this to
+@@ fsmonitor.c: static void handle_path_with_trailing_slash(
static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
{
@@ fsmonitor.c: static void fsmonitor_refresh_callback(struct index_state *istate,
- if (istate->cache[i]->name[len] == '/')
- istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
- }
-+ fsmonitor_refresh_callback_unqualified(istate, name, len, pos);
++ handle_path_without_trailing_slash(istate, name, pos);
}
/*
-: ----------- > 7: 99c0d3e0742 dir: create untracked_cache_invalidate_trimmed_path()
-: ----------- > 8: f2d6765d84f fsmonitor: refactor untracked-cache invalidation
7: 1df4019931c ! 9: af6f57ab3e6 fsmonitor: refactor untracked-cache invalidation
@@ Metadata
Author: Jeff Hostetler <jeffhostetler@github.com>
## Commit message ##
- fsmonitor: refactor untracked-cache invalidation
+ fsmonitor: move untracked invalidation into helper functions
+
+ Move the call to invalidate the untracked cache for the FSEvent
+ pathname into the two helper functions.
+
+ In a later commit in this series, we will call these helpers
+ from other contexts and it safer to include the UC invalidation
+ in the helper than to remember to also add it to each helper
+ call-site.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
## fsmonitor.c ##
-@@ fsmonitor.c: static int query_fsmonitor_hook(struct repository *r,
- return result;
- }
-
-+/*
-+ * Invalidate the untracked cache for the given pathname. Copy the
-+ * buffer to a proper null-terminated string (since the untracked
-+ * cache code does not use (buf, len) style argument). Also strip any
-+ * trailing slash.
-+ */
-+static void my_invalidate_untracked_cache(
-+ struct index_state *istate, const char *name, int len)
-+{
-+ struct strbuf work_path = STRBUF_INIT;
-+
-+ if (!len)
-+ return;
-+
-+ if (name[len-1] == '/')
-+ len--;
-+
-+ strbuf_add(&work_path, name, len);
-+ untracked_cache_invalidate_path(istate, work_path.buf, 0);
-+ strbuf_release(&work_path);
-+}
-+
- static void fsmonitor_refresh_callback_unqualified(
- struct index_state *istate, const char *name, int len, int pos)
+@@ fsmonitor.c: static void handle_path_without_trailing_slash(
{
int i;
-+ my_invalidate_untracked_cache(istate, name, len);
++ /*
++ * Mark the untracked cache dirty for this path (regardless of
++ * whether or not we find an exact match for it in the index).
++ * Since the path is unqualified (no trailing slash hint in the
++ * FSEvent), it may refer to a file or directory. So we should
++ * not assume one or the other and should always let the untracked
++ * cache decide what needs to invalidated.
++ */
++ untracked_cache_invalidate_trimmed_path(istate, name, 0);
+
if (pos >= 0) {
/*
* We have an exact match for this path and can just
-@@ fsmonitor.c: static int fsmonitor_refresh_callback_slash(
+@@ fsmonitor.c: static void handle_path_with_trailing_slash(
+ {
int i;
- int nr_in_cone = 0;
-+ my_invalidate_untracked_cache(istate, name, len);
++ /*
++ * Mark the untracked cache dirty for this directory path
++ * (regardless of whether or not we find an exact match for it
++ * in the index or find it to be proper prefix of one or more
++ * files in the index), since the FSEvent is hinting that
++ * there may be changes on or within the directory.
++ */
++ untracked_cache_invalidate_trimmed_path(istate, name, 0);
+
if (pos < 0)
pos = -pos - 1;
@@ fsmonitor.c: static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
-
- if (name[len - 1] == '/') {
- fsmonitor_refresh_callback_slash(istate, name, len, pos);
--
-- /*
-- * We need to remove the traling "/" from the path
-- * for the untracked cache.
-- */
-- name[len - 1] = '\0';
} else {
- fsmonitor_refresh_callback_unqualified(istate, name, len, pos);
+ handle_path_without_trailing_slash(istate, name, pos);
}
-
- /*
- * Mark the untracked cache dirty even if it wasn't found in the index
-- * as it could be a new untracked file.
+- * as it could be a new untracked file. (Let the untracked cache
+- * layer silently deal with any trailing slash.)
- */
-- untracked_cache_invalidate_path(istate, name, 0);
+- untracked_cache_invalidate_trimmed_path(istate, name, 0);
}
/*
-: ----------- > 10: 623c6f06e21 fsmonitor: return invalidated cache-entry count on directory event
9: a0cc4c8274c ! 11: 1853f77d333 fsmonitor: refactor non-directory callback
@@ Metadata
Author: Jeff Hostetler <jeffhostetler@github.com>
## Commit message ##
- fsmonitor: refactor non-directory callback
+ fsmonitor: remove custom loop from non-directory path handler
- Refactor the fsmonitor_refresh_callback_unqualified() code
- to try to use the _callback_slash() code and avoid having
- a custom filter in the child cache-entry scanner.
+ Refactor the code that handles refresh events for pathnames that do
+ not contain a trailing slash. Instead of using a custom loop to try
+ to scan the index and detect if the FSEvent named a file or might be a
+ directory prefix, use the recently created helper function to do that.
+
+ Also update the comments to describe what and why we are doing this.
On platforms that DO NOT annotate FS events with a trailing
slash, if we fail to find an exact match for the pathname
@@ Commit message
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
## fsmonitor.c ##
-@@ fsmonitor.c: static int my_callback_dir_name_hash(
- return nr_in_cone;
+@@ fsmonitor.c: static int query_fsmonitor_hook(struct repository *r,
+ return result;
}
--static void fsmonitor_refresh_callback_unqualified(
++static size_t handle_path_with_trailing_slash(
++ struct index_state *istate, const char *name, int pos);
++
+/*
+ * The daemon sent an observed pathname without a trailing slash.
+ * (This is the normal case.) We do not know if it is a tracked or
@@ fsmonitor.c: static int my_callback_dir_name_hash(
+ * do not know it is case-correct or -incorrect.
+ *
+ * Assume it is case-correct and try an exact match.
-+ *
-+ * Return the number of cache-entries that we invalidated.
+ */
-+static int fsmonitor_refresh_callback_unqualified(
- struct index_state *istate, const char *name, int len, int pos)
+ static void handle_path_without_trailing_slash(
+ struct index_state *istate, const char *name, int pos)
{
- int i;
-
- my_invalidate_untracked_cache(istate, name, len);
+ /*
+ * Mark the untracked cache dirty for this path (regardless of
+ * whether or not we find an exact match for it in the index).
+@@ fsmonitor.c: static void handle_path_without_trailing_slash(
if (pos >= 0) {
/*
@@ fsmonitor.c: static int my_callback_dir_name_hash(
+ * at that directory. (That is, assume no D/F conflicts.)
*/
istate->cache[pos]->ce_flags &= ~CE_FSMONITOR_VALID;
-+ return 1;
} else {
-+ int nr_in_cone;
+ struct strbuf work_path = STRBUF_INIT;
+
/*
@@ fsmonitor.c: static int my_callback_dir_name_hash(
+ * "foo" and "foo/" like "foo-" or "foo-bar", so we
+ * don't want to do our own scan.
*/
+- int len = strlen(name);
- pos = -pos - 1;
-
- for (i = pos; i < istate->cache_nr; i++) {
@@ fsmonitor.c: static int my_callback_dir_name_hash(
- if (istate->cache[i]->name[len] == '/')
- istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
- }
-+ strbuf_add(&work_path, name, len);
++ strbuf_add(&work_path, name, strlen(name));
+ strbuf_addch(&work_path, '/');
+ pos = index_name_pos(istate, work_path.buf, work_path.len);
-+ nr_in_cone = fsmonitor_refresh_callback_slash(
-+ istate, work_path.buf, work_path.len, pos);
++ handle_path_with_trailing_slash(istate, work_path.buf, pos);
+ strbuf_release(&work_path);
-+ return nr_in_cone;
}
}
10: bf18401f56c ! 12: f77d68c78ad fsmonitor: support case-insensitive non-directory events
@@ Metadata
Author: Jeff Hostetler <jeffhostetler@github.com>
## Commit message ##
- fsmonitor: support case-insensitive non-directory events
+ fsmonitor: return invalided cache-entry count on non-directory event
+
+ Teah the refresh callback helper function for unqualified FSEvents
+ (pathnames without a trailing slash) to return the number of
+ cache-entries that were invalided in response to the event.
+
+ This will be used in a later commit to help determine if the observed
+ pathname was (possibly) case-incorrect when (on a case-insensitive
+ file system).
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
## fsmonitor.c ##
-@@ fsmonitor.c: static int fsmonitor_refresh_callback_unqualified(
- }
- }
-
-+/*
-+ * On a case-insensitive FS, use the name-hash to map the case of
-+ * the observed path to the canonical case expected by the index.
-+ *
-+ * The given pathname DOES NOT include the trailing slash.
+@@ fsmonitor.c: static size_t handle_path_with_trailing_slash(
+ * do not know it is case-correct or -incorrect.
+ *
+ * Assume it is case-correct and try an exact match.
+ *
+ * Return the number of cache-entries that we invalidated.
-+ */
-+static int fsmonitor_refresh_callback_unqualified_icase(
-+ struct index_state *istate, const char *name, int len)
-+{
-+ int nr_in_cone;
-+
-+ /*
-+ * Look for a case-incorrect match for this non-directory
-+ * pathname.
-+ */
-+ nr_in_cone = my_callback_name_hash(istate, name, len);
-+ if (nr_in_cone)
-+ return nr_in_cone;
-+
-+ /*
-+ * Try the directory name-hash and see if there is a
-+ * case-incorrect directory with this pathanme.
-+ * (len) because we don't have a trailing slash.
-+ */
-+ nr_in_cone = my_callback_dir_name_hash(istate, name, len);
-+ return nr_in_cone;
-+}
-+
- /*
- * The daemon can decorate directory events, such as a move or rename,
- * by adding a trailing slash to the observed name. Use this to
-@@ fsmonitor.c: static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
- if (ignore_case && !nr_in_cone)
- fsmonitor_refresh_callback_slash_icase(istate, name, len);
+ */
+-static void handle_path_without_trailing_slash(
++static size_t handle_path_without_trailing_slash(
+ struct index_state *istate, const char *name, int pos)
+ {
+ /*
+@@ fsmonitor.c: static void handle_path_without_trailing_slash(
+ * at that directory. (That is, assume no D/F conflicts.)
+ */
+ istate->cache[pos]->ce_flags &= ~CE_FSMONITOR_VALID;
++ return 1;
} else {
-- fsmonitor_refresh_callback_unqualified(istate, name, len, pos);
-+ nr_in_cone = fsmonitor_refresh_callback_unqualified(istate, name, len, pos);
-+ if (ignore_case && !nr_in_cone)
-+ fsmonitor_refresh_callback_unqualified_icase(istate, name, len);
++ size_t nr_in_cone;
+ struct strbuf work_path = STRBUF_INIT;
+
+ /*
+@@ fsmonitor.c: static void handle_path_without_trailing_slash(
+ strbuf_add(&work_path, name, strlen(name));
+ strbuf_addch(&work_path, '/');
+ pos = index_name_pos(istate, work_path.buf, work_path.len);
+- handle_path_with_trailing_slash(istate, work_path.buf, pos);
++ nr_in_cone = handle_path_with_trailing_slash(
++ istate, work_path.buf, pos);
+ strbuf_release(&work_path);
++ return nr_in_cone;
}
}
-: ----------- > 13: 58b36673e15 fsmonitor: trace the new invalidated cache-entry count
8: e0029a2aad6 ! 14: 288f3f4e54e fsmonitor: support case-insensitive directory events
@@ Metadata
Author: Jeff Hostetler <jeffhostetler@github.com>
## Commit message ##
- fsmonitor: support case-insensitive directory events
+ fsmonitor: support case-insensitive events
Teach fsmonitor_refresh_callback() to handle case-insensitive
lookups if case-sensitive lookups fail on case-insensitive systems.
@@ Commit message
to find the associated cache-entry. This causes status to think that
the cached CE flags are correct and skip over the file.
- Update the handling of directory-style FSEvents (ones containing a
- path with a trailing slash) to optionally use the name-hash if the
- case-correct search does not find a match.
-
- (The FSMonitor daemon can send directory FSEvents if the OS provides
- that information.)
+ Update event handling to optionally use the name-hash and dir-name-hash
+ if necessary.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
@@ fsmonitor.c
#include "strbuf.h"
#include "trace2.h"
@@ fsmonitor.c: static int query_fsmonitor_hook(struct repository *r,
- return result;
- }
-
-+static int fsmonitor_refresh_callback_slash(
-+ struct index_state *istate, const char *name, int len, int pos);
-+
- /*
- * Invalidate the untracked cache for the given pathname. Copy the
- * buffer to a proper null-terminated string (since the untracked
-@@ fsmonitor.c: static void my_invalidate_untracked_cache(
- strbuf_release(&work_path);
- }
+ static size_t handle_path_with_trailing_slash(
+ struct index_state *istate, const char *name, int pos);
+/*
-+ * Use the name-hash to lookup the pathname.
++ * 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 int my_callback_name_hash(
-+ struct index_state *istate, const char *name, int len)
++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, len, 1);
++ ce = index_file_exists(istate, name, strlen(name), 1);
+ if (!ce)
+ return 0;
+
+ /*
-+ * The index contains a case-insensitive match for the pathname.
-+ * This could either be a regular file or a sparse-index directory.
++ * 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.
+ *
-+ * We should not have seen FSEvents for a sparse-index directory,
-+ * but we handle it just in case.
++ * 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'",
++ "fsmonitor_refresh_callback MAP: '%s' '%s'",
+ name, ce->name);
+
-+ my_invalidate_untracked_cache(istate, ce->name, ce->ce_namelen);
++ untracked_cache_invalidate_trimmed_path(istate, ce->name, 0);
+
+ ce->ce_flags &= ~CE_FSMONITOR_VALID;
+ return 1;
+}
+
+/*
-+ * Use the directory 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.
-+ *
-+ * The pathname MUST NOT have a trailing slash.
++ * 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 int my_callback_dir_name_hash(
-+ struct index_state *istate, const char *name, int len)
++static size_t handle_using_dir_name_hash_icase(
++ struct index_state *istate, const char *name)
+{
+ struct strbuf canonical_path = STRBUF_INIT;
+ int pos;
-+ int nr_in_cone;
++ size_t len = strlen(name);
++ size_t nr_in_cone;
++
++ if (name[len - 1] == '/')
++ len--;
+
-+ if (!index_dir_exists2(istate, name, len, &canonical_path))
++ if (!index_dir_find(istate, name, len, &canonical_path))
+ return 0; /* name is untracked */
-+ if (!memcmp(name, canonical_path.buf, len)) {
++
++ 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 */
+ }
+
+ trace_printf_key(&trace_fsmonitor,
-+ "fsmonitor_refresh_callback map '%s' '%s'",
++ "fsmonitor_refresh_callback MAP: '%s' '%s'",
+ name, canonical_path.buf);
+
+ /*
-+ * The directory 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
-+ * we repeat the original search using the case-corrected
-+ * spelling.
++ * 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 = fsmonitor_refresh_callback_slash(
-+ istate, canonical_path.buf, canonical_path.len, pos);
++ nr_in_cone = handle_path_with_trailing_slash(
++ istate, canonical_path.buf, pos);
+ strbuf_release(&canonical_path);
+ return nr_in_cone;
+}
+
- static void fsmonitor_refresh_callback_unqualified(
- struct index_state *istate, const char *name, int len, int pos)
- {
-@@ fsmonitor.c: static void fsmonitor_refresh_callback_unqualified(
- *
- * Return the number of cache-entries that we invalidated. We will
- * use this later to determine if we need to attempt a second
-- * case-insensitive search.
-+ * case-insensitive search. That is, if a observed-case search yields
-+ * any results, we assume the prefix is case-correct. If there are
-+ * no matches, we still don't know if the observed path is simply
-+ * untracked or case-incorrect.
- */
- static int fsmonitor_refresh_callback_slash(
- struct index_state *istate, const char *name, int len, int pos)
-@@ fsmonitor.c: static int fsmonitor_refresh_callback_slash(
- return nr_in_cone;
- }
+ /*
+ * The daemon sent an observed pathname without a trailing slash.
+ * (This is the normal case.) We do not know if it is a tracked or
+@@ fsmonitor.c: static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
+ else
+ nr_in_cone = handle_path_without_trailing_slash(istate, name, pos);
-+/*
-+ * On a case-insensitive FS, use the name-hash and directory name-hash
-+ * to map the case of the observed path to the canonical case expected
-+ * by the index.
-+ *
-+ * The given pathname includes the trailing slash.
-+ *
-+ * Return the number of cache-entries that we invalidated.
-+ */
-+static int fsmonitor_refresh_callback_slash_icase(
-+ struct index_state *istate, const char *name, int len)
-+{
-+ int nr_in_cone;
-+
-+ /*
-+ * Look for a case-incorrect sparse-index directory.
-+ */
-+ nr_in_cone = my_callback_name_hash(istate, name, len);
-+ if (nr_in_cone)
-+ return nr_in_cone;
-+
+ /*
-+ * (len-1) because we do not include the trailing slash in the
-+ * pathname.
++ * 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.
+ */
-+ nr_in_cone = my_callback_dir_name_hash(istate, name, len-1);
-+ return nr_in_cone;
-+}
-+
- static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
- {
- int len = strlen(name);
- int pos = index_name_pos(istate, name, len);
-+ int nr_in_cone;
++ 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);
++ }
+
-
- trace_printf_key(&trace_fsmonitor,
- "fsmonitor_refresh_callback '%s' (pos %d)",
- name, pos);
-
- if (name[len - 1] == '/') {
-- fsmonitor_refresh_callback_slash(istate, name, len, pos);
-+ nr_in_cone = fsmonitor_refresh_callback_slash(istate, name, len, pos);
-+ if (ignore_case && !nr_in_cone)
-+ fsmonitor_refresh_callback_slash_icase(istate, name, len);
- } else {
- fsmonitor_refresh_callback_unqualified(istate, name, len, pos);
- }
+ if (nr_in_cone)
+ trace_printf_key(&trace_fsmonitor,
+ "fsmonitor_refresh_callback CNT: %d",
11: 7775de735f4 ! 15: 3a20065dbf8 fsmonitor: refactor bit invalidation in refresh callback
@@ Commit message
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
## fsmonitor.c ##
-@@ fsmonitor.c: static void my_invalidate_untracked_cache(
- strbuf_release(&work_path);
- }
+@@ fsmonitor.c: 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.
+ */
-+static void my_invalidate_ce_fsm(struct cache_entry *ce)
++static void invalidate_ce_fsm(struct cache_entry *ce)
+{
+ if (ce->ce_flags & CE_FSMONITOR_VALID)
+ trace_printf_key(&trace_fsmonitor,
-+ "fsmonitor_refresh_cb_invalidate '%s'",
++ "fsmonitor_refresh_callback INV: '%s'",
+ ce->name);
+ ce->ce_flags &= ~CE_FSMONITOR_VALID;
+}
+
/*
- * Use the name-hash to lookup the pathname.
- *
-@@ fsmonitor.c: static int my_callback_name_hash(
+ * Use the name-hash to do a case-insensitive cache-entry lookup with
+ * the pathname and invalidate the cache-entry.
+@@ fsmonitor.c: static size_t handle_using_name_hash_icase(
- my_invalidate_untracked_cache(istate, ce->name, ce->ce_namelen);
+ untracked_cache_invalidate_trimmed_path(istate, ce->name, 0);
- ce->ce_flags &= ~CE_FSMONITOR_VALID;
-+ my_invalidate_ce_fsm(ce);
++ invalidate_ce_fsm(ce);
return 1;
}
-@@ fsmonitor.c: static int fsmonitor_refresh_callback_unqualified(
+@@ fsmonitor.c: 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;
-+ my_invalidate_ce_fsm(istate->cache[pos]);
++ invalidate_ce_fsm(istate->cache[pos]);
return 1;
} else {
- int nr_in_cone;
-@@ fsmonitor.c: static int fsmonitor_refresh_callback_slash(
+ size_t nr_in_cone;
+@@ fsmonitor.c: 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;
-+ my_invalidate_ce_fsm(istate->cache[i]);
++ invalidate_ce_fsm(istate->cache[i]);
nr_in_cone++;
}
12: 63edb68303f ! 16: 467d3c1fe2c t7527: update case-insenstive fsmonitor test
@@ Commit message
Now that the FSMonitor client has been updated to better
handle events on case-insenstive file systems, update the
- two tests that demonstrated the bug.
+ two tests that demonstrated the bug and remove the temporary
+ SKIPME prereq.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
## t/t7527-builtin-fsmonitor.sh ##
-@@ t/t7527-builtin-fsmonitor.sh: test_expect_success CASE_INSENSITIVE_FS 'fsmonitor subdir case wrong on disk' '
+@@ t/t7527-builtin-fsmonitor.sh: test_expect_success 'split-index and FSMonitor work well together' '
+ #
+ # The setup is a little contrived.
+ #
+-test_expect_success SKIPME,CASE_INSENSITIVE_FS 'fsmonitor subdir case wrong on disk' '
++test_expect_success CASE_INSENSITIVE_FS 'fsmonitor subdir case wrong on disk' '
+ test_when_finished "stop_daemon_delete_repo subdir_case_wrong" &&
+
+ git init subdir_case_wrong &&
+@@ t/t7527-builtin-fsmonitor.sh: test_expect_success SKIPME,CASE_INSENSITIVE_FS 'fsmonitor subdir case wrong on d
grep -q "dir1/DIR2/dir3/file3.*pos -3" "$PWD/subdir_case_wrong.log1" &&
+ # Also verify that we get a mapping event to correct the case.
-+ grep -q "map.*dir1/DIR2/dir3/file3.*dir1/dir2/dir3/file3" \
++ grep -q "MAP:.*dir1/DIR2/dir3/file3.*dir1/dir2/dir3/file3" \
+ "$PWD/subdir_case_wrong.log1" &&
+
# The refresh-callbacks should have caused "git status" to clear
@@ t/t7527-builtin-fsmonitor.sh: test_expect_success CASE_INSENSITIVE_FS 'fsmonitor
+ grep -q " M dir1/dir2/dir3/file3" "$PWD/subdir_case_wrong.out"
'
- test_expect_success CASE_INSENSITIVE_FS 'fsmonitor file case wrong on disk' '
-@@ t/t7527-builtin-fsmonitor.sh: test_expect_success CASE_INSENSITIVE_FS 'fsmonitor file case wrong on disk' '
+-test_expect_success SKIPME,CASE_INSENSITIVE_FS 'fsmonitor file case wrong on disk' '
++test_expect_success CASE_INSENSITIVE_FS 'fsmonitor file case wrong on disk' '
+ test_when_finished "stop_daemon_delete_repo file_case_wrong" &&
+
+ git init file_case_wrong &&
+@@ t/t7527-builtin-fsmonitor.sh: test_expect_success SKIPME,CASE_INSENSITIVE_FS 'fsmonitor file case wrong on dis
grep -q "fsmonitor_refresh_callback.*FILE-3-A.*pos -3" "$PWD/file_case_wrong-try3.log" &&
grep -q "fsmonitor_refresh_callback.*file-4-a.*pos -9" "$PWD/file_case_wrong-try3.log" &&
@@ t/t7527-builtin-fsmonitor.sh: test_expect_success CASE_INSENSITIVE_FS 'fsmonitor
- ! grep -q " M dir1/dir2/dir3/file-3-a" "$PWD/file_case_wrong-try3.out" &&
- ! grep -q " M dir1/dir2/dir4/FILE-4-A" "$PWD/file_case_wrong-try3.out"
+ # Also verify that we get a mapping event to correct the case.
-+ grep -q "fsmonitor_refresh_callback map.*dir1/dir2/dir3/FILE-3-A.*dir1/dir2/dir3/file-3-a" \
++ grep -q "fsmonitor_refresh_callback MAP:.*dir1/dir2/dir3/FILE-3-A.*dir1/dir2/dir3/file-3-a" \
+ "$PWD/file_case_wrong-try3.log" &&
-+ grep -q "fsmonitor_refresh_callback map.*dir1/dir2/dir4/file-4-a.*dir1/dir2/dir4/FILE-4-A" \
++ grep -q "fsmonitor_refresh_callback MAP:.*dir1/dir2/dir4/file-4-a.*dir1/dir2/dir4/FILE-4-A" \
+ "$PWD/file_case_wrong-try3.log" &&
+
+ grep -q " M dir1/dir2/dir3/file-3-a" "$PWD/file_case_wrong-try3.out" &&
--
gitgitgadget
^ permalink raw reply
* [PATCH v2] Add unix domain socket support to HTTP transport
From: Leslie Cheng via GitGitGadget @ 2024-02-23 1:58 UTC (permalink / raw)
To: git; +Cc: Eric Wong, Leslie Cheng, Leslie Cheng, Leslie Cheng
In-Reply-To: <pull.1681.git.git.1708506863243.gitgitgadget@gmail.com>
From: Leslie Cheng <leslie.cheng5@gmail.com>
This changeset introduces an `http.unixSocket` option so that users can
proxy their git over HTTP remotes to a unix domain socket. In terms of
why, since UDS are local and git already has a local protocol: some
corporate environments use a UDS to proxy requests to internal resources
(ie. source control), so this change would support those use-cases. This
proxy can occasionally be necessary to attach MFA tokens or client
certificates for CLI tools.
The implementation leverages `--unix-socket` option [0] via the
`CURLOPT_UNIX_SOCKET_PATH` flag available with libcurl [1].
`GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH` and `NO_UNIX_SOCKETS` were kept
separate so that we can spit out better error messages for users if git
was compiled with `NO_UNIX_SOCKETS`.
[0] https://curl.se/docs/manpage.html#--unix-socket
[1] https://curl.se/libcurl/c/CURLOPT_UNIX_SOCKET_PATH.html
Signed-off-by: Leslie Cheng <leslie@lc.fyi>
---
Add unix domain socket support to HTTP transport
Changes since v1:
* Updated test to use Perl instead of nc to proxy between UDS and TCP
socket; I chose not to split this out into a library since its use is
hyper-specific and has a dependency on lib-httpd.sh
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1681%2Flcfyi%2Flcfyi%2Fadd-unix-socket-support-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1681/lcfyi/lcfyi/add-unix-socket-support-v2
Pull-Request: https://github.com/git/git/pull/1681
Range-diff vs v1:
1: 3e531632329 ! 1: 2af5cc8089b Add unix domain socket support to HTTP transport.
@@ Metadata
Author: Leslie Cheng <leslie.cheng5@gmail.com>
## Commit message ##
- Add unix domain socket support to HTTP transport.
+ Add unix domain socket support to HTTP transport
This changeset introduces an `http.unixSocket` option so that users can
proxy their git over HTTP remotes to a unix domain socket. In terms of
@@ t/t5565-http-unix-domain-socket.sh (new)
+ test_done
+}
+
-+UDS_TO_TCP_FIFO=uds_to_tcp
-+TCP_TO_UDS_FIFO=tcp_to_uds
-+UDS_PID=
-+TCP_PID=
++if ! test_have_prereq PERL
++then
++ skip_all='skipping http-unix-socket tests; perl not available'
++ test_done
++fi
++
++SOCKET_PROXY_PIDFILE="$(pwd)/proxy.pid"
+UDS_SOCKET="$(pwd)/uds.sock"
-+UNRESOLVABLE_ENDPOINT=http://localhost:4242
++UNRESOLVABLE_ENDPOINT=http://unresolved
+
+start_proxy_unix_to_tcp() {
-+ local socket_path="$UDS_SOCKET"
-+ local host=127.0.0.1
-+ local port=$LIB_HTTPD_PORT
-+
-+ rm -f "$UDS_TO_TCP_FIFO"
-+ rm -f "$TCP_TO_UDS_FIFO"
-+ rm -f "$socket_path"
-+ mkfifo "$UDS_TO_TCP_FIFO"
-+ mkfifo "$TCP_TO_UDS_FIFO"
-+ nc -klU "$socket_path" <tcp_to_uds >uds_to_tcp &
-+ UDS_PID=$!
-+
-+ nc "$host" "$port" >tcp_to_uds <uds_to_tcp &
-+ TCP_PID=$!
-+
-+ test_atexit 'stop_proxy_unix_to_tcp'
++ test_atexit 'stop_proxy_unix_to_tcp'
++
++ perl -Mstrict -MIO::Select -MIO::Socket::INET -MIO::Socket::UNIX -e '
++ my $uds_path = $ARGV[0];
++ my $host = $ARGV[1];
++ my $port = $ARGV[2];
++ my $pidfile = $ARGV[3];
++
++ open(my $fh, ">", $pidfile) or die "failed to create pidfile";
++ print $fh "$$";
++ close($fh);
++
++ my $uds = IO::Socket::UNIX->new(
++ Local => $uds_path,
++ Type => SOCK_STREAM,
++ Listen => 5,
++ ) or die "failed to create unix domain socket";
++
++ while (my $conn = $uds->accept()) {
++ my $tcp_client = IO::Socket::INET->new(
++ PeerAddr => $host,
++ PeerPort => $port,
++ Proto => "tcp",
++ ) or die "failed to create TCP socket";
++
++ my $sel = IO::Select->new($conn, $tcp_client);
++
++ while (my @ready = $sel->can_read(10)) {
++ foreach my $socket (@ready) {
++ my $other = ($socket == $conn) ? $tcp_client : $conn;
++ my $data;
++ my $bytes = $socket->sysread($data, 4096);
++
++ if ($bytes) {
++ $other->syswrite($data, $bytes);
++ } else {
++ $socket->close();
++ }
++ }
++ }
++ }
++ ' "$UDS_SOCKET" "127.0.0.1" "$LIB_HTTPD_PORT" "$SOCKET_PROXY_PIDFILE" &
++ SOCKET_PROXY_PID=$!
+}
+
+stop_proxy_unix_to_tcp() {
-+ kill "$UDS_PID"
-+ kill "$TCP_PID"
-+ rm -f "$UDS_TO_TCP_FIFO"
-+ rm -f "$TCP_TO_UDS_FIFO"
++ kill -9 "$(cat "$SOCKET_PROXY_PIDFILE")"
++ rm -f "$SOCKET_PROXY_PIDFILE"
++ rm -f "$UDS_SOCKET"
+}
+
+start_httpd
@@ t/t5565-http-unix-domain-socket.sh (new)
+
+# sanity check that we can't clone normally
+test_expect_success 'cloning without UDS fails' '
-+ test_must_fail git clone "$UNRESOLVABLE_ENDPOINT/smart/repo.git" clone
++ test_must_fail git clone "$UNRESOLVABLE_ENDPOINT/smart/repo.git" clone
+'
+
+test_expect_success 'cloning with UDS succeeds' '
-+ test_when_finished "rm -rf clone" &&
++ test_when_finished "rm -rf clone" &&
+ test_config_global http.unixsocket "$UDS_SOCKET" &&
+ git clone "$UNRESOLVABLE_ENDPOINT/smart/repo.git" clone
+'
+
+test_expect_success 'cloning with a non-existent http proxy fails' '
-+ git clone $HTTPD_URL/smart/repo.git clone &&
-+ rm -rf clone &&
-+ test_config_global http.proxy 127.0.0.1:0 &&
-+ test_must_fail git clone $HTTPD_URL/smart/repo.git clone
++ git clone $HTTPD_URL/smart/repo.git clone &&
++ rm -rf clone &&
++ test_config_global http.proxy 127.0.0.1:0 &&
++ test_must_fail git clone $HTTPD_URL/smart/repo.git clone
+'
+
+test_expect_success 'UDS socket takes precedence over http proxy' '
-+ test_when_finished "rm -rf clone" &&
-+ test_config_global http.proxy 127.0.0.1:0 &&
-+ test_config_global http.unixsocket "$UDS_SOCKET" &&
-+ git clone $HTTPD_URL/smart/repo.git clone
++ test_when_finished "rm -rf clone" &&
++ test_config_global http.proxy 127.0.0.1:0 &&
++ test_config_global http.unixsocket "$UDS_SOCKET" &&
++ git clone $HTTPD_URL/smart/repo.git clone
+'
+
+test_done
Documentation/config/http.txt | 5 ++
git-curl-compat.h | 7 ++
http.c | 23 ++++++
t/t5565-http-unix-domain-socket.sh | 109 +++++++++++++++++++++++++++++
4 files changed, 144 insertions(+)
create mode 100755 t/t5565-http-unix-domain-socket.sh
diff --git a/Documentation/config/http.txt b/Documentation/config/http.txt
index 2d4e0c9b869..bf48cbd599a 100644
--- a/Documentation/config/http.txt
+++ b/Documentation/config/http.txt
@@ -277,6 +277,11 @@ http.followRedirects::
the base for the follow-up requests, this is generally
sufficient. The default is `initial`.
+http.unixSocket::
+ Connect through this Unix domain socket via HTTP, instead of using the
+ network. If set, this config takes precendence over `http.proxy` and
+ is incompatible with the proxy options (see `curl(1)`).
+
http.<url>.*::
Any of the http.* options above can be applied selectively to some URLs.
For a config key to match a URL, each element of the config key is
diff --git a/git-curl-compat.h b/git-curl-compat.h
index fd96b3cdffd..f0f3bec0e17 100644
--- a/git-curl-compat.h
+++ b/git-curl-compat.h
@@ -74,6 +74,13 @@
#define GIT_CURL_HAVE_CURLE_SSL_PINNEDPUBKEYNOTMATCH 1
#endif
+/**
+ * CURLOPT_UNIX_SOCKET_PATH was added in 7.40.0, released in January 2015.
+ */
+#if LIBCURL_VERSION_NUM >= 0x074000
+#define GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH 1
+#endif
+
/**
* CURL_HTTP_VERSION_2 was added in 7.43.0, released in June 2015.
*
diff --git a/http.c b/http.c
index e73b136e589..8cfdcaeac82 100644
--- a/http.c
+++ b/http.c
@@ -79,6 +79,9 @@ static const char *http_proxy_ssl_ca_info;
static struct credential proxy_cert_auth = CREDENTIAL_INIT;
static int proxy_ssl_cert_password_required;
+#if defined(GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH) && !defined(NO_UNIX_SOCKETS)
+static const char *curl_unix_socket_path;
+#endif
static struct {
const char *name;
long curlauth_param;
@@ -455,6 +458,20 @@ static int http_options(const char *var, const char *value,
return 0;
}
+ if (!strcmp("http.unixsocket", var)) {
+#ifdef GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH
+#ifndef NO_UNIX_SOCKETS
+ return git_config_string(&curl_unix_socket_path, var, value);
+#else
+ warning(_("Unix socket support unavailable in this build of Git"));
+ return 0;
+#endif
+#else
+ warning(_("Unix socket support is not supported with cURL < 7.40.0"));
+ return 0;
+#endif
+ }
+
if (!strcmp("http.cookiefile", var))
return git_config_pathname(&curl_cookie_file, var, value);
if (!strcmp("http.savecookies", var)) {
@@ -1203,6 +1220,12 @@ static CURL *get_curl_handle(void)
}
init_curl_proxy_auth(result);
+#if defined(GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH) && !defined(NO_UNIX_SOCKETS)
+ if (curl_unix_socket_path) {
+ curl_easy_setopt(result, CURLOPT_UNIX_SOCKET_PATH, curl_unix_socket_path);
+ }
+#endif
+
set_curl_keepalive(result);
return result;
diff --git a/t/t5565-http-unix-domain-socket.sh b/t/t5565-http-unix-domain-socket.sh
new file mode 100755
index 00000000000..2f9c53ab14f
--- /dev/null
+++ b/t/t5565-http-unix-domain-socket.sh
@@ -0,0 +1,109 @@
+#!/bin/sh
+
+test_description="test fetching through http via unix domain socket"
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-httpd.sh
+
+test -z "$NO_UNIX_SOCKETS" || {
+ skip_all='skipping http-unix-socket tests, unix sockets not available'
+ test_done
+}
+
+if ! test_have_prereq PERL
+then
+ skip_all='skipping http-unix-socket tests; perl not available'
+ test_done
+fi
+
+SOCKET_PROXY_PIDFILE="$(pwd)/proxy.pid"
+UDS_SOCKET="$(pwd)/uds.sock"
+UNRESOLVABLE_ENDPOINT=http://unresolved
+
+start_proxy_unix_to_tcp() {
+ test_atexit 'stop_proxy_unix_to_tcp'
+
+ perl -Mstrict -MIO::Select -MIO::Socket::INET -MIO::Socket::UNIX -e '
+ my $uds_path = $ARGV[0];
+ my $host = $ARGV[1];
+ my $port = $ARGV[2];
+ my $pidfile = $ARGV[3];
+
+ open(my $fh, ">", $pidfile) or die "failed to create pidfile";
+ print $fh "$$";
+ close($fh);
+
+ my $uds = IO::Socket::UNIX->new(
+ Local => $uds_path,
+ Type => SOCK_STREAM,
+ Listen => 5,
+ ) or die "failed to create unix domain socket";
+
+ while (my $conn = $uds->accept()) {
+ my $tcp_client = IO::Socket::INET->new(
+ PeerAddr => $host,
+ PeerPort => $port,
+ Proto => "tcp",
+ ) or die "failed to create TCP socket";
+
+ my $sel = IO::Select->new($conn, $tcp_client);
+
+ while (my @ready = $sel->can_read(10)) {
+ foreach my $socket (@ready) {
+ my $other = ($socket == $conn) ? $tcp_client : $conn;
+ my $data;
+ my $bytes = $socket->sysread($data, 4096);
+
+ if ($bytes) {
+ $other->syswrite($data, $bytes);
+ } else {
+ $socket->close();
+ }
+ }
+ }
+ }
+ ' "$UDS_SOCKET" "127.0.0.1" "$LIB_HTTPD_PORT" "$SOCKET_PROXY_PIDFILE" &
+ SOCKET_PROXY_PID=$!
+}
+
+stop_proxy_unix_to_tcp() {
+ kill -9 "$(cat "$SOCKET_PROXY_PIDFILE")"
+ rm -f "$SOCKET_PROXY_PIDFILE"
+ rm -f "$UDS_SOCKET"
+}
+
+start_httpd
+start_proxy_unix_to_tcp
+
+test_expect_success 'setup repository' '
+ test_commit foo &&
+ git init --bare "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ git push --mirror "$HTTPD_DOCUMENT_ROOT_PATH/repo.git"
+'
+
+# sanity check that we can't clone normally
+test_expect_success 'cloning without UDS fails' '
+ test_must_fail git clone "$UNRESOLVABLE_ENDPOINT/smart/repo.git" clone
+'
+
+test_expect_success 'cloning with UDS succeeds' '
+ test_when_finished "rm -rf clone" &&
+ test_config_global http.unixsocket "$UDS_SOCKET" &&
+ git clone "$UNRESOLVABLE_ENDPOINT/smart/repo.git" clone
+'
+
+test_expect_success 'cloning with a non-existent http proxy fails' '
+ git clone $HTTPD_URL/smart/repo.git clone &&
+ rm -rf clone &&
+ test_config_global http.proxy 127.0.0.1:0 &&
+ test_must_fail git clone $HTTPD_URL/smart/repo.git clone
+'
+
+test_expect_success 'UDS socket takes precedence over http proxy' '
+ test_when_finished "rm -rf clone" &&
+ test_config_global http.proxy 127.0.0.1:0 &&
+ test_config_global http.unixsocket "$UDS_SOCKET" &&
+ git clone $HTTPD_URL/smart/repo.git clone
+'
+
+test_done
base-commit: 3e0d3cd5c7def4808247caf168e17f2bbf47892b
--
gitgitgadget
^ permalink raw reply related
* RE: [RFC PATCH v2 1/6] t0080: turn t-basic unit test into a helper
From: rsbecker @ 2024-02-23 0:06 UTC (permalink / raw)
To: 'Josh Steadmon'
Cc: 'Jeff King', 'Junio C Hamano', git,
johannes.schindelin, phillip.wood
In-Reply-To: <Zdffgzf74eNeNBnF@google.com>
On Thursday, February 22, 2024 6:58 PM, Josh Steadmon wrote:
>On 2024.02.13 09:36, rsbecker@nexbridge.com wrote:
>> On Tuesday, February 13, 2024 2:41 AM, Peff wrote:
>> >On Mon, Feb 12, 2024 at 01:27:11PM -0800, Junio C Hamano wrote:
>> >
>> >> Josh Steadmon <steadmon@google.com> writes:
>> >>
>> >> > I see this line in the docs [1]: "As with wildcard expansion in
>> >> > rules, the results of the wildcard function are sorted". GNU Make
>> >> > has restored the sorted behavior of $(wildcard) since 2018 [2].
>> >> > I'll leave the sort off for now, but if folks feel like we need
>> >> > to support older versions of `make`, I'll add it back.
>> >> >
>> >> > [1]
>> >> > https://www.gnu.org/software/make/manual/html_node/Wildcard-Funct
>> >> > ion .html [2] https://savannah.gnu.org/bugs/index.php?52076
>> >>
>> >> Thanks for digging. I thought I was certain that woldcard is
>> >> sorted and stable and was quite perplexed when I could not find the
>> >> mention in a version of doc I had handy ("""This is Edition 0.75,
>> >> last updated
>> >> 19 January 2020, of 'The GNU Make Manual', for GNU 'make'
>> >> version 4.3.""").
>> >
>> >Likewise (mine is the latest version in Debian unstable). The change
>> >to sort comes from their[1] eedea52a, which was in GNU make 4.2.90.
>> >But the matching documentation change didn't happen until 5b993ae,
>> >which was
>> >4.3.90 in late 2021. So that explains the mystery.
>> >
>> >Those dates imply to me that we should keep the $(sort), though. Six
>> >years is not so long in distro timescales, especially given that
>> >Debian unstable is on a 4-year-old version. (And if we did want to
>> >get rid of it, certainly we should do so consistently across the
Makefile in a
>separate patch).
>>
>> I am stuck on 4.2.1 and cannot get to 4.3.90 any time soon. Can you
>> want on this? It will take us out unless we can suppress the $(sort)
>
>Hi Randall,
>
>I'm not sure I follow here. The change in 4.2.90 is that wildcard expansion
becomes
>sorted by default again. So adding the $(sort) back shouldn't cause any
problems in
>4.2.1. Or did I misunderstand your point?
I thought you were referring to a new ${sort} behaviour in 4.2.90. My bad.
^ permalink raw reply
* Re: [PATCH 0/2] commit-graph: suggest deleting corrupt graphs
From: Junio C Hamano @ 2024-02-23 0:05 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git
In-Reply-To: <cover.1708643825.git.steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes:
> At $WORK, we've had a few occasions where someone's commit-graph becomes
> corrupt, and hits various BUG()s that block their day-to-day work. When
> this happens, we advise the user to either disable the commit graph, or
> to delete it and let it be regenerated.
>
> It would be a nicer user experience if we can make this a self-serve
> procedure. To do this, let's add a new `git commit-graph clear`
> subcommand so that users don't need to manually delete files under their
> .git directories. And to make it self-documenting, update various BUG(),
> die(), and error() messages to suggest removing the commit graph to
> recover from the corruption.
I am of two minds.
For one, if we know there is a corruption and if we know that we
will certainly recover cleanly if we removed these files, it would
be fair for an end-user to respond with: instead of telling me to
run "commit-graph clear", you can run it for me, can't you?
The other one is if it hinders debugging the root cause to run
"clear", whether it is done by the end-user or by the mechanism that
detects and dies upon discovery of a corruption. Do we know how
these commit-graph files become corrupt? How valuable would these
corrupt files be to help us track down where the corruption comes
from? If they are not all that useful in debugging, then removing
them ourselves or telling users to remove them may be OK, of course.
Do these BUG()s come from corruption that can be diagnosed upfront
when we "open" the commit-graph files? I am wondering if it would
be the matter of teaching prepare_commit_graph() to check for
corruption and return without enabling the support.
Thanks.
^ permalink raw reply
* Re: [RFC PATCH v2 1/6] t0080: turn t-basic unit test into a helper
From: Josh Steadmon @ 2024-02-22 23:57 UTC (permalink / raw)
To: rsbecker
Cc: 'Jeff King', 'Junio C Hamano', git,
johannes.schindelin, phillip.wood
In-Reply-To: <016c01da5e8a$1ada8fc0$508faf40$@nexbridge.com>
On 2024.02.13 09:36, rsbecker@nexbridge.com wrote:
> On Tuesday, February 13, 2024 2:41 AM, Peff wrote:
> >On Mon, Feb 12, 2024 at 01:27:11PM -0800, Junio C Hamano wrote:
> >
> >> Josh Steadmon <steadmon@google.com> writes:
> >>
> >> > I see this line in the docs [1]: "As with wildcard expansion in
> >> > rules, the results of the wildcard function are sorted". GNU Make
> >> > has restored the sorted behavior of $(wildcard) since 2018 [2]. I'll
> >> > leave the sort off for now, but if folks feel like we need to
> >> > support older versions of `make`, I'll add it back.
> >> >
> >> > [1]
> >> > https://www.gnu.org/software/make/manual/html_node/Wildcard-Function
> >> > .html [2] https://savannah.gnu.org/bugs/index.php?52076
> >>
> >> Thanks for digging. I thought I was certain that woldcard is sorted
> >> and stable and was quite perplexed when I could not find the mention
> >> in a version of doc I had handy ("""This is Edition 0.75, last updated
> >> 19 January 2020, of 'The GNU Make Manual', for GNU 'make'
> >> version 4.3.""").
> >
> >Likewise (mine is the latest version in Debian unstable). The change to sort comes
> >from their[1] eedea52a, which was in GNU make 4.2.90. But the matching
> >documentation change didn't happen until 5b993ae, which was
> >4.3.90 in late 2021. So that explains the mystery.
> >
> >Those dates imply to me that we should keep the $(sort), though. Six years is not so
> >long in distro timescales, especially given that Debian unstable is on a 4-year-old
> >version. (And if we did want to get rid of it, certainly we should do so consistently
> >across the Makefile in a separate patch).
>
> I am stuck on 4.2.1 and cannot get to 4.3.90 any time soon. Can you
> want on this? It will take us out unless we can suppress the $(sort)
Hi Randall,
I'm not sure I follow here. The change in 4.2.90 is that wildcard
expansion becomes sorted by default again. So adding the $(sort) back
shouldn't cause any problems in 4.2.1. Or did I misunderstand your
point?
^ permalink raw reply
* Re: [RFC PATCH v2 1/6] t0080: turn t-basic unit test into a helper
From: Josh Steadmon @ 2024-02-22 23:55 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, johannes.schindelin, phillip.wood
In-Reply-To: <20240213074118.GA2225494@coredump.intra.peff.net>
On 2024.02.13 02:41, Jeff King wrote:
> On Mon, Feb 12, 2024 at 01:27:11PM -0800, Junio C Hamano wrote:
>
> > Josh Steadmon <steadmon@google.com> writes:
> >
> > > I see this line in the docs [1]: "As with wildcard expansion in rules,
> > > the results of the wildcard function are sorted". GNU Make has restored
> > > the sorted behavior of $(wildcard) since 2018 [2]. I'll leave the sort
> > > off for now, but if folks feel like we need to support older versions of
> > > `make`, I'll add it back.
> > >
> > > [1] https://www.gnu.org/software/make/manual/html_node/Wildcard-Function.html
> > > [2] https://savannah.gnu.org/bugs/index.php?52076
> >
> > Thanks for digging. I thought I was certain that woldcard is sorted
> > and stable and was quite perplexed when I could not find the mention
> > in a version of doc I had handy ("""This is Edition 0.75, last
> > updated 19 January 2020, of 'The GNU Make Manual', for GNU 'make'
> > version 4.3.""").
>
> Likewise (mine is the latest version in Debian unstable). The change to
> sort comes from their[1] eedea52a, which was in GNU make 4.2.90. But the
> matching documentation change didn't happen until 5b993ae, which was
> 4.3.90 in late 2021. So that explains the mystery.
>
> Those dates imply to me that we should keep the $(sort), though. Six
> years is not so long in distro timescales, especially given that Debian
> unstable is on a 4-year-old version. (And if we did want to get rid of
> it, certainly we should do so consistently across the Makefile in a
> separate patch).
Makes sense, thanks for investigating. I've restored the $(sort) in V3.
^ permalink raw reply
* [PATCH 2/2] commit-graph: suggest removing corrupt graphs
From: Josh Steadmon @ 2024-02-22 23:19 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1708643825.git.steadmon@google.com>
There are various ways the commit graph can be corrupted. When we detect
these, we issue an error(), BUG(), or die(). However, this doesn't help
the user correct the problem.
Since the commit graph can be regenerated from scratch, it may make
sense to just delete corrupt graphs. Suggest running the new
`git commit-graph clear` command in relevant error/BUG/die messages.
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
commit-graph.c | 16 +++++++++++++---
commit-reach.c | 4 +++-
t/t5318-commit-graph.sh | 4 ++++
3 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index ca84423042..0d5474852c 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -418,6 +418,7 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
if (graph_signature != GRAPH_SIGNATURE) {
error(_("commit-graph signature %X does not match signature %X"),
graph_signature, GRAPH_SIGNATURE);
+ error(_("try running: git commit-graph clear"));
return NULL;
}
@@ -425,6 +426,7 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
if (graph_version != GRAPH_VERSION) {
error(_("commit-graph version %X does not match version %X"),
graph_version, GRAPH_VERSION);
+ error(_("try running: git commit-graph clear"));
return NULL;
}
@@ -432,6 +434,7 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
if (hash_version != oid_version(the_hash_algo)) {
error(_("commit-graph hash version %X does not match version %X"),
hash_version, oid_version(the_hash_algo));
+ error(_("try running: git commit-graph clear"));
return NULL;
}
@@ -447,6 +450,7 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
GRAPH_FANOUT_SIZE + the_hash_algo->rawsz) {
error(_("commit-graph file is too small to hold %u chunks"),
graph->num_chunks);
+ error(_("try running: git commit-graph clear"));
free(graph);
return NULL;
}
@@ -459,14 +463,17 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
if (read_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, graph_read_oid_fanout, graph)) {
error(_("commit-graph required OID fanout chunk missing or corrupted"));
+ error(_("try running: git commit-graph clear"));
goto free_and_return;
}
if (read_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph)) {
error(_("commit-graph required OID lookup chunk missing or corrupted"));
+ error(_("try running: git commit-graph clear"));
goto free_and_return;
}
if (read_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph)) {
error(_("commit-graph required commit data chunk missing or corrupted"));
+ error(_("try running: git commit-graph clear"));
goto free_and_return;
}
@@ -860,7 +867,8 @@ static void load_oid_from_graph(struct commit_graph *g,
BUG("NULL commit-graph");
if (pos >= g->num_commits + g->num_commits_in_base)
- die(_("invalid commit position. commit-graph is likely corrupt"));
+ die(_("invalid commit position. The commit-graph is likely corrupt,\n"
+ "try running:\n\tgit commit-graph clear"));
lex_index = pos - g->num_commits_in_base;
@@ -876,7 +884,8 @@ static struct commit_list **insert_parent_or_die(struct repository *r,
struct object_id oid;
if (pos >= g->num_commits + g->num_commits_in_base)
- die("invalid parent position %"PRIu32, pos);
+ die("invalid parent position %"PRIu32". The commit-graph is likely corrupt,\n"
+ "try running:\n\tgit commit-graph clear", pos);
load_oid_from_graph(g, pos, &oid);
c = lookup_commit(r, &oid);
@@ -897,7 +906,8 @@ static void fill_commit_graph_info(struct commit *item, struct commit_graph *g,
g = g->base_graph;
if (pos >= g->num_commits + g->num_commits_in_base)
- die(_("invalid commit position. commit-graph is likely corrupt"));
+ die(_("invalid commit position. commit-graph is likely corrupt,\n"
+ "try running:\n\tgit commit-graph clear"));
lex_index = pos - g->num_commits_in_base;
commit_data = g->chunk_commit_data + st_mult(GRAPH_DATA_WIDTH, lex_index);
diff --git a/commit-reach.c b/commit-reach.c
index ecc913fc99..16765ce39b 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -81,7 +81,9 @@ static struct commit_list *paint_down_to_common(struct repository *r,
timestamp_t generation = commit_graph_generation(commit);
if (min_generation && generation > last_gen)
- BUG("bad generation skip %"PRItime" > %"PRItime" at %s",
+ BUG("bad generation skip %"PRItime" > %"PRItime" at %s\n"
+ "The commit graph is likely corrupt, try running:\n"
+ "\tgit commit-graph clear",
generation, last_gen,
oid_to_hex(&commit->object.oid));
last_gen = generation;
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 35354bddcb..f4553b1916 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -843,6 +843,7 @@ test_expect_success 'reader notices too-small oid fanout chunk' '
cat >expect.err <<-\EOF &&
error: commit-graph oid fanout chunk is wrong size
error: commit-graph required OID fanout chunk missing or corrupted
+ error: try running: git commit-graph clear
EOF
test_cmp expect.err err
'
@@ -852,6 +853,7 @@ test_expect_success 'reader notices fanout/lookup table mismatch' '
cat >expect.err <<-\EOF &&
error: commit-graph OID lookup chunk is the wrong size
error: commit-graph required OID lookup chunk missing or corrupted
+ error: try running: git commit-graph clear
EOF
test_cmp expect.err err
'
@@ -868,6 +870,7 @@ test_expect_success 'reader notices out-of-bounds fanout' '
cat >expect.err <<-\EOF &&
error: commit-graph fanout values out of order
error: commit-graph required OID fanout chunk missing or corrupted
+ error: try running: git commit-graph clear
EOF
test_cmp expect.err err
'
@@ -877,6 +880,7 @@ test_expect_success 'reader notices too-small commit data chunk' '
cat >expect.err <<-\EOF &&
error: commit-graph commit data chunk is wrong size
error: commit-graph required commit data chunk missing or corrupted
+ error: try running: git commit-graph clear
EOF
test_cmp expect.err err
'
--
2.44.0.rc0.258.g7320e95886-goog
^ permalink raw reply related
* [PATCH 1/2] commit-graph: add `git commit-graph clear` subcommand
From: Josh Steadmon @ 2024-02-22 23:19 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1708643825.git.steadmon@google.com>
In the event the commit graph becomes corrupted, one option for recovery
is to simply delete it and then rewrite it from scratch. However, this
requires users to manually delete files and directories under .git/,
which is generally discouraged.
Add a new subcommand `git commit-graph clear` to provide a convenient
option for removing the commit graph. Include tests for both single-file
and split-file commit graphs. While we're at it, replace various cleanup
steps in the commit graph tests with `git commit-graph clear`.
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
Documentation/git-commit-graph.txt | 5 ++++
builtin/commit-graph.c | 40 ++++++++++++++++++++++++++++++
commit-graph.c | 27 ++++++++++++++++++++
commit-graph.h | 1 +
t/t5318-commit-graph.sh | 13 ++++++++--
t/t5324-split-commit-graph.sh | 26 +++++++++++++------
6 files changed, 102 insertions(+), 10 deletions(-)
diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt
index 903b16830e..0c96c428e6 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -14,6 +14,7 @@ SYNOPSIS
[--split[=<strategy>]] [--reachable | --stdin-packs | --stdin-commits]
[--changed-paths] [--[no-]max-new-filters <n>] [--[no-]progress]
<split-options>
+'git commit-graph clear' [--object-dir <dir>]
DESCRIPTION
@@ -114,6 +115,10 @@ database. Used to check for corrupted data.
With the `--shallow` option, only check the tip commit-graph file in
a chain of split commit-graphs.
+'clear'::
+
+Delete the commit graph file(s) and directory, if any exist.
+
EXAMPLES
--------
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 7102ee90a0..0e2fecae50 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -23,6 +23,9 @@
" [--changed-paths] [--[no-]max-new-filters <n>] [--[no-]progress]\n" \
" <split-options>")
+#define BUILTIN_COMMIT_GRAPH_CLEAR_USAGE \
+ N_("git commit-graph clear [--object-dir <dir>]")
+
static const char * builtin_commit_graph_verify_usage[] = {
BUILTIN_COMMIT_GRAPH_VERIFY_USAGE,
NULL
@@ -33,9 +36,15 @@ static const char * builtin_commit_graph_write_usage[] = {
NULL
};
+static const char * builtin_commit_graph_clear_usage[] = {
+ BUILTIN_COMMIT_GRAPH_CLEAR_USAGE,
+ NULL
+};
+
static char const * const builtin_commit_graph_usage[] = {
BUILTIN_COMMIT_GRAPH_VERIFY_USAGE,
BUILTIN_COMMIT_GRAPH_WRITE_USAGE,
+ BUILTIN_COMMIT_GRAPH_CLEAR_USAGE,
NULL,
};
@@ -331,12 +340,43 @@ static int graph_write(int argc, const char **argv, const char *prefix)
return result;
}
+static int graph_clear(int argc, const char **argv, const char *prefix) {
+ int ret = 0;
+ struct object_directory *odb = NULL;
+ char *path;
+ static struct option builtin_commit_graph_clear_options[] = {
+ OPT_END(),
+ };
+ struct option *options = add_common_options(builtin_commit_graph_clear_options);
+
+ trace2_cmd_mode("clear");
+
+ argc = parse_options(argc, argv, NULL,
+ builtin_commit_graph_clear_options,
+ builtin_commit_graph_clear_usage, 0);
+
+ if (!opts.obj_dir)
+ opts.obj_dir = get_object_directory();
+
+ odb = find_odb(the_repository, opts.obj_dir);
+
+ path = get_commit_graph_filename(odb);
+ ret |= unlink_or_warn(path);
+ ret |= rm_commit_graph_chain(odb);
+
+ FREE_AND_NULL(options);
+ free(path);
+
+ return ret;
+}
+
int cmd_commit_graph(int argc, const char **argv, const char *prefix)
{
parse_opt_subcommand_fn *fn = NULL;
struct option builtin_commit_graph_options[] = {
OPT_SUBCOMMAND("verify", &fn, graph_verify),
OPT_SUBCOMMAND("write", &fn, graph_write),
+ OPT_SUBCOMMAND("clear", &fn, graph_clear),
OPT_END(),
};
struct option *options = parse_options_concat(builtin_commit_graph_options, common_opts);
diff --git a/commit-graph.c b/commit-graph.c
index 45417d7412..ca84423042 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -206,6 +206,33 @@ char *get_commit_graph_chain_filename(struct object_directory *odb)
return xstrfmt("%s/info/commit-graphs/commit-graph-chain", odb->path);
}
+int rm_commit_graph_chain(struct object_directory *odb)
+{
+ int ret = 0;
+ struct strbuf chain_dir = STRBUF_INIT, file_path = STRBUF_INIT;
+ struct dirent *d;
+ DIR *dir;
+
+ strbuf_addf(&chain_dir, "%s/info/commit-graphs/", odb->path);
+ strbuf_addbuf(&file_path, &chain_dir);
+ dir = opendir(chain_dir.buf);
+ if (!dir)
+ goto cleanup;
+ while ((d = readdir(dir))) {
+ if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
+ continue;
+ strbuf_setlen(&file_path, chain_dir.len);
+ strbuf_addstr(&file_path, d->d_name);
+ ret |= unlink_or_warn(file_path.buf);
+ }
+ closedir(dir);
+ rmdir_or_warn(chain_dir.buf);
+cleanup:
+ strbuf_release(&chain_dir);
+ strbuf_release(&file_path);
+ return ret;
+}
+
static struct commit_graph *alloc_commit_graph(void)
{
struct commit_graph *g = xcalloc(1, sizeof(*g));
diff --git a/commit-graph.h b/commit-graph.h
index e519cb81cb..1a6002767c 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -31,6 +31,7 @@ struct string_list;
char *get_commit_graph_filename(struct object_directory *odb);
char *get_commit_graph_chain_filename(struct object_directory *odb);
+int rm_commit_graph_chain(struct object_directory *odb);
int open_commit_graph(const char *graph_file, int *fd, struct stat *st);
int open_commit_graph_chain(const char *chain_file, int *fd, struct stat *st);
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index a2b4442660..35354bddcb 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -397,7 +397,7 @@ test_expect_success 'warn on improper hash version' '
test_expect_success TIME_IS_64BIT,TIME_T_IS_64BIT 'lower layers have overflow chunk' '
UNIX_EPOCH_ZERO="@0 +0000" &&
FUTURE_DATE="@4147483646 +0000" &&
- rm -f full/.git/objects/info/commit-graph &&
+ git -C full commit-graph clear &&
test_commit -C full --date "$FUTURE_DATE" future-1 &&
test_commit -C full --date "$UNIX_EPOCH_ZERO" old-1 &&
git -C full commit-graph write --reachable &&
@@ -824,7 +824,7 @@ test_expect_success 'overflow during generation version upgrade' '
corrupt_chunk () {
graph=full/.git/objects/info/commit-graph &&
- test_when_finished "rm -rf $graph" &&
+ test_when_finished "git -C full commit-graph clear" &&
git -C full commit-graph write --reachable &&
corrupt_chunk_file $graph "$@"
}
@@ -945,4 +945,13 @@ test_expect_success 'stale commit cannot be parsed when traversing graph' '
)
'
+test_expect_success 'commit-graph clear removes files' '
+ git -C full commit-graph write &&
+ git -C full commit-graph verify &&
+ test_path_is_file full/.git/objects/info/commit-graph &&
+ git -C full commit-graph clear &&
+ ! test_path_exists full/.git/objects/info/commit-graph &&
+ ! test_path_exists full/.git/objects/info/commit-graphs
+'
+
test_done
diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index 281266f788..ab5bc67fb6 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -120,7 +120,7 @@ test_expect_success 'fork and fail to base a chain on a commit-graph file' '
git clone . fork &&
(
cd fork &&
- rm .git/objects/info/commit-graph &&
+ git commit-graph clear &&
echo "$(pwd)/../.git/objects" >.git/objects/info/alternates &&
test_commit new-commit &&
git commit-graph write --reachable --split &&
@@ -177,7 +177,7 @@ test_expect_success 'create fork and chain across alternate' '
(
cd fork &&
git config core.commitGraph true &&
- rm -rf $graphdir &&
+ git commit-graph clear &&
echo "$(pwd)/../.git/objects" >.git/objects/info/alternates &&
test_commit 13 &&
git branch commits/13 &&
@@ -387,7 +387,7 @@ test_expect_success 'verify across alternates' '
git clone --no-hardlinks . verify-alt &&
(
cd verify-alt &&
- rm -rf $graphdir &&
+ git commit-graph clear &&
altdir="$(pwd)/../.git/objects" &&
echo "$altdir" >.git/objects/info/alternates &&
git commit-graph verify --object-dir="$altdir/" &&
@@ -435,7 +435,7 @@ test_expect_success 'split across alternate where alternate is not split' '
git clone --no-hardlinks . alt-split &&
(
cd alt-split &&
- rm -f .git/objects/info/commit-graph &&
+ git commit-graph clear &&
echo "$(pwd)"/../.git/objects >.git/objects/info/alternates &&
test_commit 18 &&
git commit-graph write --reachable --split &&
@@ -446,7 +446,7 @@ test_expect_success 'split across alternate where alternate is not split' '
test_expect_success '--split=no-merge always writes an incremental' '
test_when_finished rm -rf a b &&
- rm -rf $graphdir $infodir/commit-graph &&
+ git commit-graph clear &&
git reset --hard commits/2 &&
git rev-list HEAD~1 >a &&
git rev-list HEAD >b &&
@@ -456,7 +456,7 @@ test_expect_success '--split=no-merge always writes an incremental' '
'
test_expect_success '--split=replace replaces the chain' '
- rm -rf $graphdir $infodir/commit-graph &&
+ git commit-graph clear &&
git reset --hard commits/3 &&
git rev-list -1 HEAD~2 >a &&
git rev-list -1 HEAD~1 >b &&
@@ -490,7 +490,7 @@ test_expect_success ULIMIT_FILE_DESCRIPTORS 'handles file descriptor exhaustion'
while read mode modebits
do
test_expect_success POSIXPERM "split commit-graph respects core.sharedrepository $mode" '
- rm -rf $graphdir $infodir/commit-graph &&
+ git commit-graph clear &&
git reset --hard commits/1 &&
test_config core.sharedrepository "$mode" &&
git commit-graph write --split --reachable &&
@@ -508,7 +508,7 @@ done <<\EOF
EOF
test_expect_success '--split=replace with partial Bloom data' '
- rm -rf $graphdir $infodir/commit-graph &&
+ git commit-graph clear &&
git reset --hard commits/3 &&
git rev-list -1 HEAD~2 >a &&
git rev-list -1 HEAD~1 >b &&
@@ -718,4 +718,14 @@ test_expect_success 'write generation data chunk when commit-graph chain is repl
)
'
+
+test_expect_success 'commit-graph clear removes files' '
+ git commit-graph write &&
+ git commit-graph verify &&
+ ! test_dir_is_empty .git/objects/info/commit-graphs &&
+ git commit-graph clear &&
+ ! test_path_exists .git/objects/info/commit-graph &&
+ ! test_path_exists .git/objects/info/commit-graphs
+'
+
test_done
--
2.44.0.rc0.258.g7320e95886-goog
^ permalink raw reply related
* [PATCH 0/2] commit-graph: suggest deleting corrupt graphs
From: Josh Steadmon @ 2024-02-22 23:19 UTC (permalink / raw)
To: git
At $WORK, we've had a few occasions where someone's commit-graph becomes
corrupt, and hits various BUG()s that block their day-to-day work. When
this happens, we advise the user to either disable the commit graph, or
to delete it and let it be regenerated.
It would be a nicer user experience if we can make this a self-serve
procedure. To do this, let's add a new `git commit-graph clear`
subcommand so that users don't need to manually delete files under their
.git directories. And to make it self-documenting, update various BUG(),
die(), and error() messages to suggest removing the commit graph to
recover from the corruption.
This approach was suggested in [1] and generally positively received
[2], but was never implemented.
[1] https://lore.kernel.org/git/YBoBBie2t1EhcLAN@google.com/
[2] https://lore.kernel.org/git/xmqqk0rpc7uj.fsf@gitster.c.googlers.com/
Open questions for reviewers:
* Should we turn this into an advice setting instead?
* Should we also suggest running `commit-graph write` after clearing
the graph? I lean towards no; everything will still function as normal
without a commit graph.
* Does it make sense to add the suggestion in all of these corruption
error messages? There are many other error()s in commit-graph.c,
should we add this for all of them, or just the ones that specifically
mention corruption? Or maybe just the fatal BUG()s and die()s?
* Any other places this suggestion should be added that I've missed?
Josh Steadmon (2):
commit-graph: add `git commit-graph clear` subcommand
commit-graph: suggest removing corrupt graphs
Documentation/git-commit-graph.txt | 5 ++++
builtin/commit-graph.c | 40 +++++++++++++++++++++++++++
commit-graph.c | 43 +++++++++++++++++++++++++++---
commit-graph.h | 1 +
commit-reach.c | 4 ++-
t/t5318-commit-graph.sh | 17 ++++++++++--
t/t5324-split-commit-graph.sh | 26 ++++++++++++------
7 files changed, 122 insertions(+), 14 deletions(-)
base-commit: 3e0d3cd5c7def4808247caf168e17f2bbf47892b
--
2.44.0.rc0.258.g7320e95886-goog
^ permalink raw reply
* Re: [PATCH v5 3/3] test-stdlib: show that git-std-lib is independent
From: Junio C Hamano @ 2024-02-22 22:24 UTC (permalink / raw)
To: Calvin Wan; +Cc: git, Jonathan Tan, phillip.wood123
In-Reply-To: <20240222175033.1489723-4-calvinwan@google.com>
Calvin Wan <calvinwan@google.com> writes:
> diff --git a/Makefile b/Makefile
> index d37ea9d34b..1d762ce13a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -870,9 +870,7 @@ TEST_BUILTINS_OBJS += test-xml-encode.o
> # Do not add more tests here unless they have extra dependencies. Add
> # them in TEST_BUILTINS_OBJS above.
> TEST_PROGRAMS_NEED_X += test-fake-ssh
> -TEST_PROGRAMS_NEED_X += test-tool
> -
> -TEST_PROGRAMS = $(patsubst %,t/helper/%$X,$(TEST_PROGRAMS_NEED_X))
> +TEST_PROGRAMS_NEED_X += $(info tpnxnpg=$(NO_POSIX_GOODIES))test-tool
Is this version meant to be ready for reviewing? $(info) used like
this does not look like a good fit for production code.
> diff --git a/strbuf.h b/strbuf.h
> index e959caca87..f775416307 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -1,6 +1,8 @@
> #ifndef STRBUF_H
> #define STRBUF_H
>
> +#include "git-compat-util.h"
> +
> /*
> * NOTE FOR STRBUF DEVELOPERS
> *
The same comment about header inclusion I made on [1/3] applies
here, too. I am open to hearing better ideas to handle system
headers, but my preference is to allow any and all headers assume
<git-compat-util.h> (or its moral equivalent that may be stripped
down by moving non-essential things out) is already included, which
in turn means those *.c files (like t/helper/test-stdlib.c we see
below) would include <git-compat-util.h> (or its trimmed down
version) as the first header, before including <strbuf.h>.
In any case, this change, if we were to make it (and I do not think
we should), should be treated like the change to pager.h in [1/3],
i.e. part of making the existing headers ready to be shared with the
"stdlib" effort. It does not belong to this [3/3] step, where we
are supposed to be demonstrating the use of "stdlib", which has
become (minimally) usable with the steps before this one.
> diff --git a/stubs/misc.c b/stubs/misc.c
> index 92da76fd46..8d80581e39 100644
> --- a/stubs/misc.c
> +++ b/stubs/misc.c
> @@ -9,6 +9,7 @@
> * yet. To do that we need to start using dgettext() rather than
> * gettext() in our code.
> */
> +#include "gettext.h"
> int git_gettext_enabled = 0;
> #endif
This change should have happened before this [3/3] step, whose point
is to demonstrate "stdlib" that has already been made (minimally)
usable with steps before this one.
> diff --git a/t/helper/.gitignore b/t/helper/.gitignore
> index 8c2ddcce95..5cec3b357f 100644
> --- a/t/helper/.gitignore
> +++ b/t/helper/.gitignore
> @@ -1,2 +1,3 @@
> /test-tool
> /test-fake-ssh
> +/test-stdlib
> diff --git a/t/helper/test-stdlib.c b/t/helper/test-stdlib.c
> new file mode 100644
> index 0000000000..460b472fb4
> --- /dev/null
> +++ b/t/helper/test-stdlib.c
> @@ -0,0 +1,266 @@
> +#include "git-compat-util.h"
> +#include "abspath.h"
> +#include "hex-ll.h"
> +#include "parse.h"
> +#include "strbuf.h"
> +#include "string-list.h"
> +
> +/*
> + * Calls all functions from git-std-lib
> + * Some inline/trivial functions are skipped
> + *
> + * NEEDSWORK: The purpose of this file is to show that an executable can be
> + * built with git-std-lib.a and git-stub-lib.a, and then executed. If there
> + * is another executable that demonstrates this (for example, a unit test that
> + * takes the form of an executable compiled with git-std-lib.a and git-stub-
> + * lib.a), this file can be removed.
> + */
Or alternatively, these "random list of function calls" can be
turned into a more realistic test helpers in place. "stdlib"
will hopefully gain more coverage of the features of low level
helpers "git" binary proper uses, and I do not think it is
far-fetched to migrate the "test-tool date" subcommands all to not
link directly with "libgit.a" but with gitstdlib instead and the
things should work, right? Right now, the "random list of function
calls" do not do anything useful, but that does not have to be the
case. It should offer us more value to us than "It links!" ;-).
Having said that, the most valuable part in this [3/3] step is how
this t/helper/test-stdlib is linked, i.e. this part from the
Makefile:
> +t/helper/test-stdlib$X: t/helper/test-stdlib.o GIT-LDFLAGS $(STD_LIB_FILE) $(STUB_LIB_FILE) $(GITLIBS)
> + $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
> + $< $(STD_LIB_FILE) $(STUB_LIB_FILE) $(EXTLIBS)
where we have no $(LIB_FILE) (aka libgit.a). Especially if we can
grow the capability in $(STD_LIB_FILE) without adding too much stuff
to $(STUB_LIB_FILE), this is a major achievement. Very nice.
^ permalink raw reply
* Re: [PATCH v5 1/3] pager: include stdint.h because uintmax_t is used
From: Junio C Hamano @ 2024-02-22 21:43 UTC (permalink / raw)
To: Calvin Wan; +Cc: git, Jonathan Tan, phillip.wood123
In-Reply-To: <20240222175033.1489723-2-calvinwan@google.com>
Calvin Wan <calvinwan@google.com> writes:
> 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);
This is not going in a sensible direction from our portability
standard's point of view.
The reason why we do not include these system headers directly to
our source files, and instead make it a rule to include
<git-compat-util.h> as the first header instead, is exactly because
there are curiosities in various platforms that Git wants to run on
which system include headers give us the declarations for types and
functions we rely on, in what order they must be included, and after
what feature macros (the ones that give adjustment to what the
system headers do, like _POSIX_C_SOURCE) are defined, etc.
Given that in <git-compat-util.h>, inclusion of <stdint.h> is
conditional behind some #ifdef's, it does not look like a sensible
change. It is not very likely for <inttypes.h> and <stdint.h> to
declare uintmax_t in an incompatible way, but on a platform where
<git-compat-util.h> decides to include <inttypes.h> and use its
definition of what uintmax_t is, we should follow the same choice
and be consistent.
If there is a feature macro that affects sizes of the integer on a
platform, this patch will break it even more badly. Perhaps there
is a platform whose C-library header requires you to define a
feature macro to use 64-bit, and we may define that feature macro
in <git-compat-util.h> before including either <inttypes.h> or
<stdint.h>, but by including <stdint.h> directly like the above
patch does, only this file and the sources that include only this
file, refusing to include <git-compat-util.h> as everybody in the
Git source tree should, will end up using different notion of what
the integral type with maximum width is from everybody else.
What this patch _wants_ to do is of course sympathizable, and we
have "make hdr-check" rule to enforce "a header must include the
headers that declare what it uses", except that it lets the header
files being tested assume that the things made available by
including <git-compat-util.h> are always available.
I think a sensible direction to go for libification purposes is to
also make sure that sources that are compiled into gitstdlib.a, and
the headers that makes what is in gitstdlib.a available, include the
<git-compat-util.h> header file. There may be things declared in
the <git-compat-util.h> header that are _too_ specific to what ought
to be linked into the final "git" binary and unwanted by library
clients that are not "git" binary, and the right way to deal with it
is to split <git-compat-util.h> into two parts, i.e. what makes
system services available like its conditional inclusion of
<stdint.h> vs <inttypes.h>, definition of feature macros, order in
which the current <git-compat-util.h> includes system headers, etc.,
excluding those that made you write this patch to avoid assuming
that the client code would have included <git-compat-util.h> before
<pager.h>, would be the new <git-compat-core.h>. And everything
else will remain in <git-compat-util.h>, which will include the
<git-compat-core.h>. The <pager.h> header for library clients would
include <git-compat-core.h> instead, to still allow them to use the
same types as "git" binary itself that way.
^ permalink raw reply
* Re: Git commit causes data download in partial clone
From: charmocc @ 2024-02-22 19:34 UTC (permalink / raw)
To: Jeff King; +Cc: git@vger.kernel.org
In-Reply-To: <20240220024310.GD2713741@coredump.intra.peff.net>
Jeff thank you for taking a look at this.
On Tuesday, February 20th, 2024 at 3:43 AM, Jeff King <peff@peff.net>
wrote:
> a. Your --no-checkout skips the checkout, but it does not tell Git
> that you are fundamentally uninterested in those other paths. To do
> that, you can try the sparse-checkout mechanism. I'm not super
> familiar with the feature myself, but doing:
>
> git clone --sparse --filter=blob:none $url nes
That's it! Now git knows exactly what I want from it. I am now able
to add and commit new files and git status no longer lists any deleted
paths. Also I can search for files I actually need with "git ls-files"
and add them to the sparse checkout incrementally with "git
sparse-checkout add". I haven't explored this feature before because
because I assumed it is limited to selecting entire directory
subtrees not individual files which would not help with the
mentioned repo.
> Do note that --sparse checks out the contents of the top-level tree
> by default. That's OK for your repo (all of the files are in the
> Named_Titles directory), but it might not be true for some other
> repos (it may also not work if your intent is to put another entry
> into Named_Titles, though it looks like you might just need to say
> "git add --sparse").
I think this could be avoided by cloning with --no-checkout,
creating index with "git reset" and setting up empty sparse
checkout afterwards with "git sparse-checkout set /".
Anyway it seems like with these tools I'm finally ready to tackle any
huge git repo they throw at me in the future :)
^ permalink raw reply
* Re: [PATCH v2 6/8] cherry-pick: decouple `--allow-empty` and `--keep-redundant-commits`
From: Junio C Hamano @ 2024-02-22 18:41 UTC (permalink / raw)
To: Phillip Wood; +Cc: Brian Lyles, git, newren, me
In-Reply-To: <3f276e10-7b03-4480-a157-47a7648e7f19@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> ... I still tend to think the practical effect of implying
> --allow-empty with --keep-redundant-commits is largely beneficial as
> I'm skeptical that users want to keep commits that become empty but
> not the ones that started empty.
I share that feeling exactly.
There are good reasons to keep a commit that starts as empty (as
much as creating an empty commit in the first place), so if
anything, a more common workflow element would be to drop the ones
that have become unnecessary (e.g. because the upstream already
added a change that is equivalent to what is being picked) while
keeping the ones that are empty from the start (e.g. in some
workflows an empty commit can be used as a container of
metainfo---you can imagine that in an N-commit chain leading to the
tip of a topic branch forked from the master branch, the topmost
commit is an empty one with the cover letter being prepared, so that
the resulting topic branch can be either (1) made into a patch
series with an advanced version of "git format-patch" that knows how
to use such an empty top commit in the cover letter message, or (2)
merged to the mainline via a pull request, with an advanced version
of "git merge" that notices the empty commit at the tip, and makes a
merge with the commit topic~1 while using the empty top commit to
write the message for the merge commit.
I do not quite see a good reason to do the opposite, dropping
commits that started out as empty but keeping the ones that have
become empty. Such a behaviour has additional downside that after
such a cherry-pick, when you cherry-pick the resulting history onto
yet another base, your precious "were not empty but have become so
during the initial cherry-pick" commits will appear as commits that
were empty from the start. So I do not see much reason to allow the
decoupling, even with the new "empty=keep" thing that does not imply
"allow-empty".
Thanks.
^ permalink raw reply
* Re: [PATCH v2 3/8] rebase: update `--empty=ask` to `--empty=drop`
From: Junio C Hamano @ 2024-02-22 18:27 UTC (permalink / raw)
To: phillip.wood123; +Cc: Brian Lyles, git, newren, me
In-Reply-To: <9f16544e-b6cc-414f-81e5-aac9e076f8df@gmail.com>
phillip.wood123@gmail.com writes:
>> Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
>> Reported-by: Elijah Newren <newren@gmail.com>
>
> I think we normally put Reported-by: and Helped-by: etc above the
> patch authors Signed-off-by: trailer.
True.
Teaching how to fish, the rule is to try emulating chronological
order of events. Elijah reported and then the patch was written,
and the "seal on the envelope" is what the author's sign-off
attached at the end of the chain of events.
^ permalink raw reply
* Re: [PATCH v3 3/5] t4301: verify that merge-tree fails on missing blob objects
From: Junio C Hamano @ 2024-02-22 18:23 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Patrick Steinhardt, Eric Sunshine, Johannes Schindelin
In-Reply-To: <xmqqttm0fly9.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
>> +test_expect_success 'error out on missing blob objects' '
>> + echo 1 | git hash-object -w --stdin >blob1 &&
>> + echo 2 | git hash-object -w --stdin >blob2 &&
>> + echo 3 | git hash-object -w --stdin >blob3 &&
>> + printf "100644 blob $(cat blob1)\tblob\n" | git mktree >tree1 &&
>> + printf "100644 blob $(cat blob2)\tblob\n" | git mktree >tree2 &&
>> + printf "100644 blob $(cat blob3)\tblob\n" | git mktree >tree3 &&
>> + git init --bare missing-blob.git &&
>> + cat blob1 blob3 tree1 tree2 tree3 |
>> + git pack-objects missing-blob.git/objects/pack/side1-whatever-is-missing &&
>> + test_must_fail git --git-dir=missing-blob.git >actual 2>err \
>> + merge-tree --merge-base=$(cat tree1) $(cat tree2) $(cat tree3) &&
>> + test_grep "unable to read blob object $(cat blob2)" err &&
>> + test_must_be_empty actual
>> +'
>
> It would have been even easier to see that blob2 is what we expect
> to be complained about, if you listed all objects and filtered blob2
> out, but the number of objects involved here is so small, a "cat" of
> all objects we want to keep is OK here.
Just FYI for anybody reading from the sideline.
git cat-file --unordered \
--batch-check="%(objectname)" --batch-all-objects |
grep -v $(cat blob2) |
git pack-objects ...
would be a compact way to say "Give me a pack that lets me easily
simulate what happens in a repository identical to the current one,
if it lacked object "blob2", without having to enumerate what object
we want to include in the test repository.
Again, I think in this particular case, it is easy enough to see in
the "blob1 blob3 tree1 tree2 tree3" enumeration what is missing, so
the way the patch is written is fine.
Thanks.
^ permalink raw reply
* Re: [PATCH v3 5/5] cache-tree: avoid an unnecessary check
From: Junio C Hamano @ 2024-02-22 18:00 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Patrick Steinhardt, Eric Sunshine, Johannes Schindelin
In-Reply-To: <91dc4ccd04e3a6cc50ed389edb6814e1e7a0c4dc.1708612605.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The first thing the `parse_tree()` function does is to return early if
> the tree has already been parsed. Therefore we do not need to guard the
> `parse_tree()` call behind a check of that flag.
>
> As of time of writing, there are no other instances of this in Git's
> code bases: whenever the `parsed` flag guards a `parse_tree()` call, it
> guards more than just that call.
>
> Suggested-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> cache-tree.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/cache-tree.c b/cache-tree.c
> index c6508b64a5c..78d6ba92853 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -779,7 +779,7 @@ static void prime_cache_tree_rec(struct repository *r,
> struct cache_tree_sub *sub;
> struct tree *subtree = lookup_tree(r, &entry.oid);
>
> - if (!subtree->object.parsed && parse_tree(subtree) < 0)
> + if (parse_tree(subtree) < 0)
> exit(128);
> sub = cache_tree_sub(it, entry.path);
> sub->cache_tree = cache_tree();
Obviously makes sense.
I see no need for further comments. Will queue.
Thanks.
^ permalink raw reply
* Re: [PATCH v3 4/5] Always check `parse_tree*()`'s return value
From: Junio C Hamano @ 2024-02-22 17:58 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Patrick Steinhardt, Eric Sunshine, Johannes Schindelin
In-Reply-To: <9e4dc94ef036882c3ce27208ca9fa545d018f199.1708612605.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> @@ -707,7 +707,8 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
> init_checkout_metadata(&opts.meta, info->refname,
> info->commit ? &info->commit->object.oid : null_oid(),
> NULL);
> - parse_tree(tree);
> + if (parse_tree(tree) < 0)
> + return 128;
The other error returned from this function is when unpack_trees()
fails well before the writeout phase and the value we return is also
128, so the caller is prepared to act on it. OK.
> @@ -786,9 +787,15 @@ static int merge_working_tree(const struct checkout_opts *opts,
> if (new_branch_info->commit)
> BUG("'switch --orphan' should never accept a commit as starting point");
> new_tree = parse_tree_indirect(the_hash_algo->empty_tree);
> - } else
> + if (!new_tree)
> + BUG("unable to read empty tree");
parse_tree() of the_hash_algo->empty_tree should result in a tree
object without having to even consult the object store, so BUG(),
not die(), is very much appropriate here. OK.
> + } else {
> new_tree = repo_get_commit_tree(the_repository,
> new_branch_info->commit);
> + if (!new_tree)
> + return error(_("unable to read tree %s"),
> + oid_to_hex(&new_branch_info->commit->object.oid));
We can help translators by enclosing %s inside a pair of parentheses.
$ git grep -h 'msgid .*unable to read tree' po | sort | uniq -c
18 msgid "unable to read tree (%s)"
FYI, the message with "(%s)" is shared by four places; there is one
instance of the message without parentheses added very recently that
forced .po files to have both entries. We probably should unify them
to use the one with more existing users.
> @@ -823,7 +830,8 @@ static int merge_working_tree(const struct checkout_opts *opts,
> oid_to_hex(old_commit_oid));
>
> init_tree_desc(&trees[0], tree->buffer, tree->size);
> - parse_tree(new_tree);
> + if (parse_tree(new_tree) < 0)
> + exit(128);
There is another exit() in the same else clause this code is in, and
upon failing to unpack_trees(), that call exits with 128. This
parse_tree() is about preparing the input for that call, so it makes
sense to exit with the same code. Excellent.
> diff --git a/builtin/clone.c b/builtin/clone.c
> index c6357af9498..4410b55be98 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -736,7 +736,8 @@ static int checkout(int submodule_progress, int filter_submodules)
> tree = parse_tree_indirect(&oid);
> if (!tree)
> die(_("unable to parse commit %s"), oid_to_hex(&oid));
> - parse_tree(tree);
> + if (parse_tree(tree) < 0)
> + exit(128);
> init_tree_desc(&t, tree->buffer, tree->size);
> if (unpack_trees(1, &t, &opts) < 0)
> die(_("unable to checkout working tree"));
Exactly the same reasoning applies, as die() exits with 128.
We may want to "#define EXIT_DIE 128" and use it in appropriate
places to make such a reasoning/review easier (possibly an entry for
#leftoverbits)?
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 781af2e206c..0723f06de7a 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -339,7 +339,8 @@ static void create_base_index(const struct commit *current_head)
> tree = parse_tree_indirect(¤t_head->object.oid);
> if (!tree)
> die(_("failed to unpack HEAD tree object"));
> - parse_tree(tree);
> + if (parse_tree(tree) < 0)
> + exit(128);
> init_tree_desc(&t, tree->buffer, tree->size);
> if (unpack_trees(1, &t, &opts))
> exit(128); /* We've already reported the error, finish dying */
Ditto.
> diff --git a/builtin/read-tree.c b/builtin/read-tree.c
> index 8196ca9dd85..5923ed36893 100644
> --- a/builtin/read-tree.c
> +++ b/builtin/read-tree.c
> @@ -263,7 +263,8 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
> cache_tree_free(&the_index.cache_tree);
> for (i = 0; i < nr_trees; i++) {
> struct tree *tree = trees[i];
> - parse_tree(tree);
> + if (parse_tree(tree) < 0)
> + return 128;
> init_tree_desc(t+i, tree->buffer, tree->size);
> }
> if (unpack_trees(nr_trees, t, &opts))
Ditto. After the post-context we also return 128.
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 4b018d20e3b..f030f57f4e9 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -119,6 +119,10 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
>
> if (reset_type == MIXED || reset_type == HARD) {
> tree = parse_tree_indirect(oid);
> + if (!tree) {
> + error(_("unable to read tree %s"), oid_to_hex(oid));
> + goto out;
> + }
> prime_cache_tree(the_repository, the_repository->index, tree);
> }
Good.
> diff --git a/cache-tree.c b/cache-tree.c
> index 641427ed410..c6508b64a5c 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -779,8 +779,8 @@ static void prime_cache_tree_rec(struct repository *r,
> struct cache_tree_sub *sub;
> struct tree *subtree = lookup_tree(r, &entry.oid);
>
> - if (!subtree->object.parsed)
> - parse_tree(subtree);
> + if (!subtree->object.parsed && parse_tree(subtree) < 0)
> + exit(128);
> sub = cache_tree_sub(it, entry.path);
> sub->cache_tree = cache_tree();
The cache_tree() used to be just an optimization mechanism, but
there is no other way than fully populating it to write a tree
object out of the index, so dying here is the only sensible thing to
do upon unparseable subtree. Otherwise we would end up silently
writing a bogus result. Good.
> diff --git a/merge-recursive.c b/merge-recursive.c
> index e3beb0801b1..10d41bfd487 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -410,7 +410,8 @@ static inline int merge_detect_rename(struct merge_options *opt)
>
> static void init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree)
> {
> - parse_tree(tree);
> + if (parse_tree(tree) < 0)
> + exit(128);
> init_tree_desc(desc, tree->buffer, tree->size);
> }
OK.
> diff --git a/merge.c b/merge.c
> index b60925459c2..14a7325859d 100644
> --- a/merge.c
> +++ b/merge.c
> @@ -80,7 +80,10 @@ int checkout_fast_forward(struct repository *r,
> return -1;
> }
> for (i = 0; i < nr_trees; i++) {
> - parse_tree(trees[i]);
> + if (parse_tree(trees[i]) < 0) {
> + rollback_lock_file(&lock_file);
> + return -1;
> + }
> init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
> }
This handles the error in the same way as the other case earlier
where any of the tree-ish failed to load. OK.
> diff --git a/reset.c b/reset.c
> index 48da0adf851..a93fdbc12e3 100644
> --- a/reset.c
> +++ b/reset.c
> @@ -158,6 +158,11 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)
> }
>
> tree = parse_tree_indirect(oid);
> + if (!tree) {
> + ret = error(_("unable to read tree %s"), oid_to_hex(oid));
> + goto leave_reset_head;
> + }
OK, but the _("unable to read tree (%s)") comment applies here, too.
> diff --git a/sequencer.c b/sequencer.c
> index d584cac8ed9..407473bab28 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -715,6 +715,8 @@ static int do_recursive_merge(struct repository *r,
> o.show_rename_progress = 1;
>
> head_tree = parse_tree_indirect(head);
> + if (!head_tree)
> + return error(_("unable to read tree %s"), oid_to_hex(head));
> next_tree = next ? repo_get_commit_tree(r, next) : empty_tree(r);
> base_tree = base ? repo_get_commit_tree(r, base) : empty_tree(r);
Ditto.
> @@ -3887,6 +3889,8 @@ static int do_reset(struct repository *r,
> }
>
> tree = parse_tree_indirect(&oid);
> + if (!tree)
> + return error(_("unable to read tree %s"), oid_to_hex(&oid));
Ditto.
> prime_cache_tree(r, r->index, tree);
>
> if (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0)
Looking good, modulo the additional translator burden.
Thanks.
^ permalink raw reply
* [PATCH v5 3/3] test-stdlib: show that git-std-lib is independent
From: Calvin Wan @ 2024-02-22 17:50 UTC (permalink / raw)
To: git; +Cc: Calvin Wan, Jonathan Tan, phillip.wood123, Junio C Hamano
In-Reply-To: <cover.1696021277.git.jonathantanmy@google.com>
Add a test file that calls some functions defined in git-std-lib.a
object files to showcase that they do not reference missing objects and
that, together with git-stub-lib.a, git-std-lib.a can stand on its own.
As described in test-stdlib.c, this can probably be removed once we have
unit tests.
The variable TEST_PROGRAMS is moved lower in the Makefile after
NO_POSIX_GOODIES is defined in config.make.uname. TEST_PROGRAMS isn't
used earlier than that so this change should be safe.
Signed-off-by: Calvin Wan <calvinwan@google.com>
Helped-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Makefile | 23 +++-
strbuf.h | 2 +
stubs/misc.c | 1 +
t/helper/.gitignore | 1 +
t/helper/test-stdlib.c | 266 +++++++++++++++++++++++++++++++++++++++++
t/t0082-std-lib.sh | 11 ++
6 files changed, 299 insertions(+), 5 deletions(-)
create mode 100644 t/helper/test-stdlib.c
create mode 100755 t/t0082-std-lib.sh
diff --git a/Makefile b/Makefile
index d37ea9d34b..1d762ce13a 100644
--- a/Makefile
+++ b/Makefile
@@ -870,9 +870,7 @@ TEST_BUILTINS_OBJS += test-xml-encode.o
# Do not add more tests here unless they have extra dependencies. Add
# them in TEST_BUILTINS_OBJS above.
TEST_PROGRAMS_NEED_X += test-fake-ssh
-TEST_PROGRAMS_NEED_X += test-tool
-
-TEST_PROGRAMS = $(patsubst %,t/helper/%$X,$(TEST_PROGRAMS_NEED_X))
+TEST_PROGRAMS_NEED_X += $(info tpnxnpg=$(NO_POSIX_GOODIES))test-tool
# List built-in command $C whose implementation cmd_$C() is not in
# builtin/$C.o but is linked in as part of some other command.
@@ -2678,6 +2676,16 @@ REFTABLE_TEST_OBJS += reftable/stack_test.o
REFTABLE_TEST_OBJS += reftable/test_framework.o
REFTABLE_TEST_OBJS += reftable/tree_test.o
+ifndef NO_POSIX_GOODIES
+TEST_PROGRAMS_NEED_X += test-stdlib
+MY_VAR = not_else
+$(info insideifndefnpg=$(NO_POSIX_GOODIES))
+else
+MY_VAR = else
+endif
+
+TEST_PROGRAMS = $(info tptpnx=$(TEST_PROGRAMS_NEED_X) myvar=$(MY_VAR))$(patsubst %,t/helper/%$X,$(TEST_PROGRAMS_NEED_X))
+
TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
.PHONY: test-objs
@@ -3204,7 +3212,11 @@ GIT-PYTHON-VARS: FORCE
fi
endif
-test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
+test_bindir_programs := $(info tbptpnx=$(TEST_PROGRAMS_NEED_X))$(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
+
+t/helper/test-stdlib$X: t/helper/test-stdlib.o GIT-LDFLAGS $(STD_LIB_FILE) $(STUB_LIB_FILE) $(GITLIBS)
+ $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
+ $< $(STD_LIB_FILE) $(STUB_LIB_FILE) $(EXTLIBS)
all:: $(TEST_PROGRAMS) $(test_bindir_programs) $(UNIT_TEST_PROGS)
@@ -3635,7 +3647,8 @@ ifneq ($(INCLUDE_DLLS_IN_ARTIFACTS),)
OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll t/unit-tests/bin/*.dll)
endif
-artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
+# Added an info for debugging
+artifacts-tar:: $(info npg=$(NO_POSIX_GOODIES) cc=$(COMPAT_CFLAGS) tp=$(TEST_PROGRAMS))$(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
$(UNIT_TEST_PROGS) $(MOFILES)
$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \
diff --git a/strbuf.h b/strbuf.h
index e959caca87..f775416307 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -1,6 +1,8 @@
#ifndef STRBUF_H
#define STRBUF_H
+#include "git-compat-util.h"
+
/*
* NOTE FOR STRBUF DEVELOPERS
*
diff --git a/stubs/misc.c b/stubs/misc.c
index 92da76fd46..8d80581e39 100644
--- a/stubs/misc.c
+++ b/stubs/misc.c
@@ -9,6 +9,7 @@
* yet. To do that we need to start using dgettext() rather than
* gettext() in our code.
*/
+#include "gettext.h"
int git_gettext_enabled = 0;
#endif
diff --git a/t/helper/.gitignore b/t/helper/.gitignore
index 8c2ddcce95..5cec3b357f 100644
--- a/t/helper/.gitignore
+++ b/t/helper/.gitignore
@@ -1,2 +1,3 @@
/test-tool
/test-fake-ssh
+/test-stdlib
diff --git a/t/helper/test-stdlib.c b/t/helper/test-stdlib.c
new file mode 100644
index 0000000000..460b472fb4
--- /dev/null
+++ b/t/helper/test-stdlib.c
@@ -0,0 +1,266 @@
+#include "git-compat-util.h"
+#include "abspath.h"
+#include "hex-ll.h"
+#include "parse.h"
+#include "strbuf.h"
+#include "string-list.h"
+
+/*
+ * Calls all functions from git-std-lib
+ * Some inline/trivial functions are skipped
+ *
+ * NEEDSWORK: The purpose of this file is to show that an executable can be
+ * built with git-std-lib.a and git-stub-lib.a, and then executed. If there
+ * is another executable that demonstrates this (for example, a unit test that
+ * takes the form of an executable compiled with git-std-lib.a and git-stub-
+ * lib.a), this file can be removed.
+ */
+
+static void abspath_funcs(void) {
+ struct strbuf sb = STRBUF_INIT;
+
+ fprintf(stderr, "calling abspath functions\n");
+ is_directory("foo");
+ strbuf_realpath(&sb, "foo", 0);
+ strbuf_realpath_forgiving(&sb, "foo", 0);
+ real_pathdup("foo", 0);
+ absolute_path("foo");
+ absolute_pathdup("foo");
+ prefix_filename("foo/", "bar");
+ prefix_filename_except_for_dash("foo/", "bar");
+ is_absolute_path("foo");
+ strbuf_add_absolute_path(&sb, "foo");
+ strbuf_add_real_path(&sb, "foo");
+}
+
+static void hex_ll_funcs(void) {
+ unsigned char c;
+
+ fprintf(stderr, "calling hex-ll functions\n");
+
+ hexval('c');
+ hex2chr("A1");
+ hex_to_bytes(&c, "A1", 1);
+}
+
+static void parse_funcs(void) {
+ intmax_t foo;
+ ssize_t foo1 = -1;
+ unsigned long foo2;
+ int foo3;
+ int64_t foo4;
+
+ fprintf(stderr, "calling parse functions\n");
+
+ git_parse_signed("42", &foo, maximum_signed_value_of_type(int));
+ git_parse_ssize_t("42", &foo1);
+ git_parse_ulong("42", &foo2);
+ git_parse_int("42", &foo3);
+ git_parse_int64("42", &foo4);
+ git_parse_maybe_bool("foo");
+ git_parse_maybe_bool_text("foo");
+ git_env_bool("foo", 1);
+ git_env_ulong("foo", 1);
+}
+
+static int allow_unencoded_fn(char ch) {
+ return 0;
+}
+
+static void strbuf_funcs(void) {
+ struct strbuf *sb = xmalloc(sizeof(*sb));
+ struct strbuf *sb2 = xmalloc(sizeof(*sb2));
+ struct strbuf sb3 = STRBUF_INIT;
+ struct string_list list = STRING_LIST_INIT_NODUP;
+ int fd = open("/dev/null", O_RDONLY);
+
+ fprintf(stderr, "calling strbuf functions\n");
+
+ fprintf(stderr, "at line %d\n", __LINE__);
+ starts_with("foo", "bar");
+ fprintf(stderr, "at line %d\n", __LINE__);
+ istarts_with("foo", "bar");
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_init(sb, 0);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_init(sb2, 0);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_release(sb);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_attach(sb, strbuf_detach(sb, NULL), 0, 0);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_swap(sb, sb2);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_setlen(sb, 0);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_trim(sb);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_trim_trailing_dir_sep(sb);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_trim_trailing_newline(sb);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_reencode(sb, "foo", "bar");
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_tolower(sb);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_add_separated_string_list(sb, " ", &list);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_list_free(strbuf_split_buf("foo bar", 8, ' ', -1));
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_cmp(sb, sb2);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_addch(sb, 1);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_splice(sb, 0, 1, "foo", 3);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_insert(sb, 0, "foo", 3);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_insertf(sb, 0, "%s", "foo");
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_remove(sb, 0, 1);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_add(sb, "foo", 3);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_addbuf(sb, sb2);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_join_argv(sb, 0, NULL, ' ');
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_addchars(sb, 1, 1);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_addstr(sb, "foo");
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_add_commented_lines(sb, "foo", 3, '#');
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_commented_addf(sb, '#', "%s", "foo");
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_addbuf_percentquote(sb, &sb3);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_add_percentencode(sb, "foo", STRBUF_ENCODE_SLASH);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_fread(sb, 0, stdin);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_read(sb, fd, 0);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_read_once(sb, fd, 0);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_write(sb, stderr);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_readlink(sb, "/dev/null", 0);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_getcwd(sb);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_getwholeline(sb, stderr, '\n');
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_appendwholeline(sb, stderr, '\n');
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_getline(sb, stderr);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_getline_lf(sb, stderr);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_getline_nul(sb, stderr);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_getwholeline_fd(sb, fd, '\n');
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_read_file(sb, "/dev/null", 0);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_add_lines(sb, "foo", "bar", 0);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_addstr_xml_quoted(sb, "foo");
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_addstr_urlencode(sb, "foo", allow_unencoded_fn);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_humanise_bytes(sb, 42);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_humanise_rate(sb, 42);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ printf_ln("%s", sb->buf);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ fprintf_ln(stderr, "%s", sb->buf);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ xstrdup_tolower("foo");
+ fprintf(stderr, "at line %d\n", __LINE__);
+ xstrdup_toupper("foo");
+ fprintf(stderr, "at line %d\n", __LINE__);
+ xstrfmt("%s", "foo");
+ fprintf(stderr, "at line %d\n", __LINE__);
+}
+
+static void error_builtin(const char *err, va_list params) {}
+static void warn_builtin(const char *err, va_list params) {}
+
+static void usage_funcs(void) {
+ fprintf(stderr, "calling usage functions\n");
+ error("foo");
+ error_errno("foo");
+ die_message("foo");
+ die_message_errno("foo");
+ warning("foo");
+ warning_errno("foo");
+
+ get_die_message_routine();
+ set_error_routine(error_builtin);
+ get_error_routine();
+ set_warn_routine(warn_builtin);
+ get_warn_routine();
+}
+
+static void wrapper_funcs(void) {
+ int tmp;
+ void *ptr = xmalloc(1);
+ int fd = open("/dev/null", O_RDONLY);
+ struct strbuf sb = STRBUF_INIT;
+ int mode = 0444;
+ char host[PATH_MAX], path[PATH_MAX], path1[PATH_MAX];
+ xsnprintf(path, sizeof(path), "out-XXXXXX");
+ xsnprintf(path1, sizeof(path1), "out-XXXXXX");
+
+ fprintf(stderr, "calling wrapper functions\n");
+
+ xstrdup("foo");
+ xmalloc(1);
+ xmallocz(1);
+ xmallocz_gently(1);
+ xmemdupz("foo", 3);
+ xstrndup("foo", 3);
+ xrealloc(ptr, 2);
+ xcalloc(1, 1);
+ xsetenv("foo", "bar", 0);
+ xopen("/dev/null", O_RDONLY);
+ xread(fd, &sb, 1);
+ xwrite(fd, &sb, 1);
+ xpread(fd, &sb, 1, 0);
+ xdup(fd);
+ xfopen("/dev/null", "r");
+ xfdopen(fd, "r");
+ tmp = xmkstemp(path);
+ close(tmp);
+ unlink(path);
+ tmp = xmkstemp_mode(path1, mode);
+ close(tmp);
+ unlink(path1);
+ xgetcwd();
+ fopen_for_writing(path);
+ fopen_or_warn(path, "r");
+ xstrncmpz("foo", "bar", 3);
+ xgethostname(host, 3);
+ tmp = git_mkstemps_mode(path, 1, mode);
+ close(tmp);
+ unlink(path);
+ tmp = git_mkstemp_mode(path, mode);
+ close(tmp);
+ unlink(path);
+ read_in_full(fd, &sb, 1);
+ write_in_full(fd, &sb, 1);
+ pread_in_full(fd, &sb, 1, 0);
+}
+
+int main(int argc, const char **argv) {
+ abspath_funcs();
+ hex_ll_funcs();
+ parse_funcs();
+ strbuf_funcs();
+ usage_funcs();
+ wrapper_funcs();
+ fprintf(stderr, "all git-std-lib functions finished calling\n");
+ return 0;
+}
diff --git a/t/t0082-std-lib.sh b/t/t0082-std-lib.sh
new file mode 100755
index 0000000000..0d5a024deb
--- /dev/null
+++ b/t/t0082-std-lib.sh
@@ -0,0 +1,11 @@
+#!/bin/sh
+
+test_description='Test git-std-lib compilation'
+
+. ./test-lib.sh
+
+test_expect_success !WINDOWS 'stdlib-test compiles and runs' '
+ test-stdlib
+'
+
+test_done
--
2.44.0.rc0.258.g7320e95886-goog
^ permalink raw reply related
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