* Re: [PATCH] git: --no-lazy-fetch option
From: Linus Arver @ 2024-02-13 20:37 UTC (permalink / raw)
To: Junio C Hamano, git; +Cc: Christian Couder
In-Reply-To: <owlyr0hg9kr3.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sometimes, especially during tests of low level machinery, it is
>> handy to have a way to disable lazy fetching of objects. This
>> allows us to say, for example, "git cat-file -e <object-name>", to
>> see if the object is locally available.
>
> Nit: perhaps s/locally/already locally/ is better?
I forgot to mention that the new flag is missing a documentation entry
in Documentation/git.txt. Perhaps something like
--no-lazy-fetch::
Do not fetch missing objects. Useful together with `git cat-file -e
<object-name>`, for example, to see if the object is already
locally available.
?
^ permalink raw reply
* Re: [PATCH v4 15/28] format_trailer_info(): avoid double-printing the separator
From: Kristoffer Haugsbakk @ 2024-02-13 20:41 UTC (permalink / raw)
To: Josh Soref
Cc: Christian Couder, Junio C Hamano, Emily Shaffer, Josh Steadmon,
rsbecker, Linus Arver, git
In-Reply-To: <ba1f387747b08a7270f7387beddd75dc4a8eddfe.1707196348.git.gitgitgadget@gmail.com>
On Tue, Feb 6, 2024, at 06:12, Linus Arver via GitGitGadget wrote:
> From: Linus Arver <linusa@google.com>
>
> Do not hardcode the printing of ": " as the separator and space (which
> can result in double-printing these characters); instead only
> print the separator and space if we cannot find any recognized separator
> somewhere in the key (yes, keys may have a trailing separator in it ---
> we will eventually fix this design but not now). Do so by copying the
> code out of print_tok_val(), and deleting the same function.
>
> The test suite passes again with this change.
>
> Signed-off-by: Linus Arver <linusa@google.com>
Nice find and a great commit message!
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH v2] unit-tests: do show relative file paths on non-Windows, too
From: Junio C Hamano @ 2024-02-13 20:48 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Phillip Wood, git, Randall S. Becker
In-Reply-To: <0fb4e912-b25e-0240-c49f-aac5cbc3ef9e@gmx.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> + /*
>> + * The path could be relative (t/unit-tests/test-lib.c)
>> + * or full (/home/user/git/t/unit-tests/test-lib.c).
>> + * Check the slash between "t" and "unit-tests".
>> + */
>> + prefix_len = len - needle_len;
>> + if (prefix[prefix_len + 1] == '/') {
>> + /* Oh, we're not Windows */
>> + for (size_t i = 0; i < needle_len; i++)
>> + if (needle[i] == '\\')
>> + needle[i] = '/';
>
> This looks very similar to the `convert_slashes()` function that is
> defined in `compat/mingw.h`.
I lifted it from your later loop in the function, but given that
many of the things that needed on Windows came from you, it is not
surprising if you have another copy there ;-)
> It might be a good opportunity to rename that
> function and move it to `git-compat-util.h`, then use it here and once
> more below at the end of `make_relative()`.
Right. But not as part of the -rc fix. Let's leave it as
#leftoverbits.
Thanks.
^ permalink raw reply
* Re: [PATCH] git: --no-lazy-fetch option
From: Junio C Hamano @ 2024-02-13 20:49 UTC (permalink / raw)
To: Linus Arver; +Cc: git, Christian Couder
In-Reply-To: <owlyo7ck9k3y.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
> Linus Arver <linusa@google.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Sometimes, especially during tests of low level machinery, it is
>>> handy to have a way to disable lazy fetching of objects. This
>>> allows us to say, for example, "git cat-file -e <object-name>", to
>>> see if the object is locally available.
>>
>> Nit: perhaps s/locally/already locally/ is better?
>
> I forgot to mention that the new flag is missing a documentation entry
> in Documentation/git.txt. Perhaps something like
>
> --no-lazy-fetch::
> Do not fetch missing objects. Useful together with `git cat-file -e
> <object-name>`, for example, to see if the object is already
> locally available.
>
> ?
Wonderful. Thanks for an extra set of eyes.
^ permalink raw reply
* [PATCH 00/12] FSMonitor edge cases on case-insensitive file systems
From: Jeff Hostetler via GitGitGadget @ 2024-02-13 20:52 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler
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 (12):
sparse-index: pass string length to index_file_exists()
name-hash: add index_dir_exists2()
t7527: add case-insensitve test for FSMonitor
fsmonitor: refactor refresh callback on directory events
fsmonitor: refactor refresh callback for non-directory events
fsmonitor: clarify handling of directory events in callback
fsmonitor: refactor untracked-cache invalidation
fsmonitor: support case-insensitive directory events
fsmonitor: refactor non-directory callback
fsmonitor: support case-insensitive non-directory events
fsmonitor: refactor bit invalidation in refresh callback
t7527: update case-insenstive fsmonitor test
fsmonitor.c | 338 +++++++++++++++++++++++++++++------
name-hash.c | 16 ++
name-hash.h | 2 +
sparse-index.c | 4 +-
t/t7527-builtin-fsmonitor.sh | 220 +++++++++++++++++++++++
5 files changed, 522 insertions(+), 58 deletions(-)
base-commit: 3526e67d917bcd03f317a058208fa02737654637
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1662%2Fjeffhostetler%2Ffsmonitor-ignore-case-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1662/jeffhostetler/fsmonitor-ignore-case-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1662
--
gitgitgadget
^ permalink raw reply
* [PATCH 01/12] sparse-index: pass string length to index_file_exists()
From: Jeff Hostetler via GitGitGadget @ 2024-02-13 20:52 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.git.1707857541.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
The call to index_file_exists() in the loop in expand_to_path() passes
the wrong string length. Let's fix that.
The loop in expand_to_path() searches the name-hash for each
sub-directory prefix in the provided pathname. That is, by searching
for "dir1/" then "dir1/dir2/" then "dir1/dir2/dir3/" and so on until
it finds a cache-entry representing a sparse directory.
The code creates "strbuf path_mutable" to contain the working pathname
and modifies the buffer in-place by temporarily replacing the character
following each successive "/" with NUL for the duration of the call to
index_file_exists().
It does not update the strbuf.len during this substitution.
Pass the patched length of the prefix path instead.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
sparse-index.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sparse-index.c b/sparse-index.c
index 3578feb2837..e48e40cae71 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -579,8 +579,9 @@ void expand_to_path(struct index_state *istate,
replace++;
temp = *replace;
*replace = '\0';
+ substr_len = replace - path_mutable.buf;
if (index_file_exists(istate, path_mutable.buf,
- path_mutable.len, icase)) {
+ substr_len, icase)) {
/*
* We found a parent directory in the name-hash
* hashtable, because only sparse directory entries
@@ -593,7 +594,6 @@ void expand_to_path(struct index_state *istate,
}
*replace = temp;
- substr_len = replace - path_mutable.buf;
}
cleanup:
--
gitgitgadget
^ permalink raw reply related
* [PATCH 02/12] name-hash: add index_dir_exists2()
From: Jeff Hostetler via GitGitGadget @ 2024-02-13 20:52 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.git.1707857541.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Create a new version of index_dir_exists() to return the canonical
spelling of the matched directory prefix.
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 | 16 ++++++++++++++++
name-hash.h | 2 ++
2 files changed, 18 insertions(+)
diff --git a/name-hash.c b/name-hash.c
index 251f036eef6..d735c81acb3 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -694,6 +694,22 @@ int index_dir_exists(struct index_state *istate, const char *name, int namelen)
dir = find_dir_entry(istate, name, namelen);
return dir && dir->nr;
}
+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);
+
+ if (dir && dir->nr)
+ strbuf_add(canonical_path, dir->name, dir->namelen);
+
+ return dir && dir->nr;
+}
void adjust_dirname_case(struct index_state *istate, char *name)
{
diff --git a/name-hash.h b/name-hash.h
index b1b4b0fb337..2fcac5c4870 100644
--- a/name-hash.h
+++ b/name-hash.h
@@ -5,6 +5,8 @@ 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);
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 03/12] t7527: add case-insensitve test for FSMonitor
From: Jeff Hostetler via GitGitGadget @ 2024-02-13 20:52 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.git.1707857541.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 78503158fd6..5cd68b2ea82 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 04/12] fsmonitor: refactor refresh callback on directory events
From: Jeff Hostetler via GitGitGadget @ 2024-02-13 20:52 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.git.1707857541.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
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..b1ef01bf3cd 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -183,6 +183,35 @@ 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)
+{
+ 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;
- }
+ fsmonitor_refresh_callback_slash(istate, name, len, pos);
/*
* We need to remove the traling "/" from the path
--
gitgitgadget
^ permalink raw reply related
* [PATCH 05/12] fsmonitor: refactor refresh callback for non-directory events
From: Jeff Hostetler via GitGitGadget @ 2024-02-13 20:52 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.git.1707857541.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
fsmonitor.c | 66 ++++++++++++++++++++++++++++++-----------------------
1 file changed, 38 insertions(+), 28 deletions(-)
diff --git a/fsmonitor.c b/fsmonitor.c
index b1ef01bf3cd..614270fa5e8 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -183,6 +183,42 @@ 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)
+{
+ 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.".
+ */
+ 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;
+ }
+ }
+}
+
static void fsmonitor_refresh_callback_slash(
struct index_state *istate, const char *name, int len, int pos)
{
@@ -214,7 +250,7 @@ static void fsmonitor_refresh_callback_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,
@@ -229,34 +265,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;
- }
+ fsmonitor_refresh_callback_unqualified(istate, name, len, pos);
}
/*
--
gitgitgadget
^ permalink raw reply related
* [PATCH 06/12] fsmonitor: clarify handling of directory events in callback
From: Jeff Hostetler via GitGitGadget @ 2024-02-13 20:52 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.git.1707857541.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
fsmonitor.c | 47 +++++++++++++++++++++++++++++++++--------------
1 file changed, 33 insertions(+), 14 deletions(-)
diff --git a/fsmonitor.c b/fsmonitor.c
index 614270fa5e8..754fe20cfd0 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -219,24 +219,40 @@ static void fsmonitor_refresh_callback_unqualified(
}
}
-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
+ * 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).
+ *
+ * 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)
{
int i;
+ int nr_in_cone = 0;
- /*
- * 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;
@@ -245,7 +261,10 @@ 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)
--
gitgitgadget
^ permalink raw reply related
* [PATCH 07/12] fsmonitor: refactor untracked-cache invalidation
From: Jeff Hostetler via GitGitGadget @ 2024-02-13 20:52 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.git.1707857541.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
fsmonitor.c | 38 ++++++++++++++++++++++++++------------
1 file changed, 26 insertions(+), 12 deletions(-)
diff --git a/fsmonitor.c b/fsmonitor.c
index 754fe20cfd0..14585b6c516 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -183,11 +183,35 @@ 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)
{
int i;
+ my_invalidate_untracked_cache(istate, name, len);
+
if (pos >= 0) {
/*
* We have an exact match for this path and can just
@@ -253,6 +277,8 @@ static int fsmonitor_refresh_callback_slash(
int i;
int nr_in_cone = 0;
+ my_invalidate_untracked_cache(istate, name, len);
+
if (pos < 0)
pos = -pos - 1;
@@ -278,21 +304,9 @@ 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);
}
-
- /*
- * Mark the untracked cache dirty even if it wasn't found in the index
- * as it could be a new untracked file.
- */
- untracked_cache_invalidate_path(istate, name, 0);
}
/*
--
gitgitgadget
^ permalink raw reply related
* [PATCH 08/12] fsmonitor: support case-insensitive directory events
From: Jeff Hostetler via GitGitGadget @ 2024-02-13 20:52 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.git.1707857541.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Teach fsmonitor_refresh_callback() to handle case-insensitive
lookups if case-sensitive lookups fail on case-insensitive systems.
This can cause 'git status' to report stale status for files if there
are case issues/errors in the worktree.
The FSMonitor daemon sends FSEvents using the observed spelling
of each pathname. On case-insensitive file systems this may be
different than the expected case spelling.
The existing code uses index_name_pos() to find the cache-entry for
the pathname in the FSEvent and clear the CE_FSMONITOR_VALID bit so
that the worktree scan/index refresh will revisit and revalidate the
path.
On a case-insensitive file system, the exact match lookup may fail
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.)
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
fsmonitor.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 120 insertions(+), 2 deletions(-)
diff --git a/fsmonitor.c b/fsmonitor.c
index 14585b6c516..73e6ac82af7 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -5,6 +5,7 @@
#include "ewah/ewok.h"
#include "fsmonitor.h"
#include "fsmonitor-ipc.h"
+#include "name-hash.h"
#include "run-command.h"
#include "strbuf.h"
#include "trace2.h"
@@ -183,6 +184,9 @@ 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
@@ -205,6 +209,84 @@ static void my_invalidate_untracked_cache(
strbuf_release(&work_path);
}
+/*
+ * Use the name-hash to lookup the pathname.
+ *
+ * Returns the number of cache-entries that we invalidated.
+ */
+static int my_callback_name_hash(
+ struct index_state *istate, const char *name, int len)
+{
+ struct cache_entry *ce = NULL;
+
+ ce = index_file_exists(istate, name, len, 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.
+ *
+ * We should not have seen FSEvents for a sparse-index directory,
+ * but we handle it just in case.
+ *
+ * Either way, we know that there are not any cache-entries for
+ * children inside the cone of the directory, so we don't need to
+ * do the usual scan.
+ */
+ trace_printf_key(&trace_fsmonitor,
+ "fsmonitor_refresh_callback map '%s' '%s'",
+ name, ce->name);
+
+ my_invalidate_untracked_cache(istate, ce->name, ce->ce_namelen);
+
+ 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.
+ *
+ * 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)
+{
+ struct strbuf canonical_path = STRBUF_INIT;
+ int pos;
+ int nr_in_cone;
+
+ if (!index_dir_exists2(istate, name, len, &canonical_path))
+ return 0; /* name is untracked */
+ if (!memcmp(name, canonical_path.buf, len)) {
+ strbuf_release(&canonical_path);
+ return 0; /* should not happen */
+ }
+
+ trace_printf_key(&trace_fsmonitor,
+ "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.
+ */
+ 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);
+ 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)
{
@@ -269,7 +351,10 @@ 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)
@@ -293,17 +378,50 @@ static int fsmonitor_refresh_callback_slash(
return nr_in_cone;
}
+/*
+ * 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.
+ */
+ 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;
+
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);
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH 09/12] fsmonitor: refactor non-directory callback
From: Jeff Hostetler via GitGitGadget @ 2024-02-13 20:52 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.git.1707857541.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
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.
On platforms that DO NOT annotate FS events with a trailing
slash, if we fail to find an exact match for the pathname
in the index, we do not know if the pathname represents a
directory or simply an untracked file. Pretend that the pathname
is a directory and try again before assuming it is an untracked
file.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
fsmonitor.c | 59 +++++++++++++++++++++++++++++++----------------------
1 file changed, 35 insertions(+), 24 deletions(-)
diff --git a/fsmonitor.c b/fsmonitor.c
index 73e6ac82af7..cb27bae8aa8 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -287,41 +287,52 @@ static int my_callback_dir_name_hash(
return nr_in_cone;
}
-static void fsmonitor_refresh_callback_unqualified(
+/*
+ * 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
+ * untracked file, a sparse-directory, or a populated directory (on a
+ * platform such as Windows where FSEvents are not qualified).
+ *
+ * The pathname contains the observed case reported by the FS. We
+ * 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)
{
- int i;
-
my_invalidate_untracked_cache(istate, name, len);
if (pos >= 0) {
/*
- * We have an exact match for this path and can just
- * invalidate it.
+ * An exact match on a tracked file. We assume that we
+ * do not need to scan forward for a sparse-directory
+ * 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;
+ return 1;
} else {
+ int nr_in_cone;
+ struct strbuf work_path = STRBUF_INIT;
+
/*
- * 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.".
+ * The negative "pos" gives us the suggested insertion
+ * point for the pathname (without the trailing slash).
+ * We need to see if there is a directory with that
+ * prefix, but there can be lots of pathnames between
+ * "foo" and "foo/" like "foo-" or "foo-bar", so we
+ * don't want to do our own scan.
*/
- 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;
- }
+ strbuf_add(&work_path, name, len);
+ 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);
+ strbuf_release(&work_path);
+ return nr_in_cone;
}
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH 10/12] fsmonitor: support case-insensitive non-directory events
From: Jeff Hostetler via GitGitGadget @ 2024-02-13 20:52 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.git.1707857541.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
fsmonitor.c | 34 +++++++++++++++++++++++++++++++++-
1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/fsmonitor.c b/fsmonitor.c
index cb27bae8aa8..a7847f07a40 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -336,6 +336,36 @@ 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.
+ *
+ * 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
@@ -434,7 +464,9 @@ 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);
} 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);
}
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH 11/12] fsmonitor: refactor bit invalidation in refresh callback
From: Jeff Hostetler via GitGitGadget @ 2024-02-13 20:52 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.git.1707857541.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Refactor code in the fsmonitor_refresh_callback() call chain dealing
with invalidating the CE_FSMONITOR_VALID bit and add a trace message.
During the refresh, we clear the CE_FSMONITOR_VALID bit in response to
data from the FSMonitor daemon (so that a later phase will lstat() and
verify the true state of the file).
Create a new function to clear the bit and add some unique tracing for
it to help debug edge cases.
This is similar to the existing `mark_fsmonitor_invalid()` function,
but we don't need the extra stuff that it does.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
fsmonitor.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/fsmonitor.c b/fsmonitor.c
index a7847f07a40..75c7f73f68d 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -209,6 +209,20 @@ static void my_invalidate_untracked_cache(
strbuf_release(&work_path);
}
+/*
+ * 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)
+{
+ if (ce->ce_flags & CE_FSMONITOR_VALID)
+ trace_printf_key(&trace_fsmonitor,
+ "fsmonitor_refresh_cb_invalidate '%s'",
+ ce->name);
+ ce->ce_flags &= ~CE_FSMONITOR_VALID;
+}
+
/*
* Use the name-hash to lookup the pathname.
*
@@ -240,7 +254,7 @@ static int my_callback_name_hash(
my_invalidate_untracked_cache(istate, ce->name, ce->ce_namelen);
- ce->ce_flags &= ~CE_FSMONITOR_VALID;
+ my_invalidate_ce_fsm(ce);
return 1;
}
@@ -312,7 +326,7 @@ static int fsmonitor_refresh_callback_unqualified(
* 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]);
return 1;
} else {
int nr_in_cone;
@@ -412,7 +426,7 @@ static int fsmonitor_refresh_callback_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]);
nr_in_cone++;
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH 12/12] t7527: update case-insenstive fsmonitor test
From: Jeff Hostetler via GitGitGadget @ 2024-02-13 20:52 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1662.git.1707857541.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
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.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
t/t7527-builtin-fsmonitor.sh | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
index 5cd68b2ea82..03af8539ca8 100755
--- a/t/t7527-builtin-fsmonitor.sh
+++ b/t/t7527-builtin-fsmonitor.sh
@@ -1116,16 +1116,17 @@ test_expect_success CASE_INSENSITIVE_FS 'fsmonitor subdir case wrong on disk' '
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" \
+ "$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"
+ grep -q " M dir1/dir2/dir3/file3" "$PWD/subdir_case_wrong.out"
'
test_expect_success CASE_INSENSITIVE_FS 'fsmonitor file case wrong on disk' '
@@ -1246,12 +1247,14 @@ test_expect_success CASE_INSENSITIVE_FS 'fsmonitor file case wrong on disk' '
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"
+ # 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" \
+ "$PWD/file_case_wrong-try3.log" &&
+ 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" &&
+ grep -q " M dir1/dir2/dir4/FILE-4-A" "$PWD/file_case_wrong-try3.out"
'
test_done
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v4 00/28] Enrich Trailer API
From: Christian Couder @ 2024-02-13 20:55 UTC (permalink / raw)
To: Linus Arver
Cc: Linus Arver via GitGitGadget, git, Christian Couder,
Junio C Hamano, Emily Shaffer, Josh Steadmon, Randall S. Becker
In-Reply-To: <owlywmr89ms9.fsf@fine.c.googlers.com>
On Tue, Feb 13, 2024 at 8:39 PM Linus Arver <linusa@google.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
> > I took a look at all of them, and I think that this series should be
> > split into 4 or more series.
>
> This sounds fine to me. IIRC this means for the 2nd, 3rd+ series I have
> to remember to say "this series builds on top of the other topic
> branches '...'" in the cover letter. Now that I've written this out I
> will hopefully not forget to do this...
>
> Or, I suppose I could just introduce the 1st sub-series, wait for that
> to get queued to next, then (re)introduce the 2nd sub-series, etc, in
> order. Hmm. I think this will be simpler.
Yeah, sure.
> > Also it seems to me that many patches towards the end of this series
> > should be split.
>
> In hindsight, I fully agree.
>
> Aside: I am delighted with the quality of reviews on this project. It's
> not something I am used to, so please bear with me while I try to break
> old habits.
Sure no worries.
> Thanks.
[...]
> > I am a bit sad that this series doesn't introduce unit tests using the
> > new test framework in C yet. I understand that this series is mostly a
> > big refactoring and maybe it's better to introduce unit tests only
> > when the refactoring is finished though.
>
> This was my original goal as well, and still is.
>
> > Anyway I hope the next series will introduce such tests.
>
> I will see which API functions are stable enough, and add tests
> accordingly (in a patch series sooner than later).
>
> Probably the "biggest" (?) thing that is coming from the larger series
> is the introduction of a complete separation between parsing (without
> any modification of the input) and formatting. The parser/formatter is
> a large chunk of the trailer implementation, so I would expect unit
> tests for those bits to have to wait until those improvements are merged
> into "next".
Ok.
> >> Thanks to the aggressive refactoring in this series, I've been able to
> >> identify and fix several bugs in our existing implementation. Those fixes
> >> build on top of this series but were not included here, in order to keep
> >> this series small. Below is a "shortlog" of those fixes I have locally:
> >>
> >> * "trailer: trailer replacement should not change its position" (If we
> >> found a trailer we'd like to replace, preserve its position relative to
> >> the other trailers found in the trailer block, instead of always moving
> >> it to the beginning or end of the entire trailer block.)
> >
> > I believe there was a reason why it was done this way. I don't
> > remember what it was though.
>
> Noted. I'll see what I can find in our commit history.
>
> >> * "interpret-trailers: preserve trailers coming from the input" (Sometimes,
> >> the parsed trailers from the input will be formatted differently
> >> depending on whether we provide --only-trailers or not. Make the trailers
> >> that were not modified and which are coming directly from the input get
> >> formatted the same way, regardless of this flag.)
> >
> > It could be a feature to be able to normalize trailers in a certain way.
>
> True. But doing such normalization silently is undocumented behavior,
> and we should provide explicit flags for this sort of thing. Adding such
> flags might be the right thing to do (let's discuss more when this patch
> gets proposed). FWIW the behavior I observed is that this normalization
> only happens for *some* trailers that have configuration options, not
> all trailers in the input. So it's a special kind of (limited)
> normalization.
Perhaps because we consider that having some configuration means that
the user consistently expects things in a certain way.
> >> * "interpret-trailers: fail if given unrecognized arguments" (E.g., for
> >> "--where", only accept recognized WHERE_* enum values. If we get
> >> something unrecognized, fail with an error instead of silently doing
> >> nothing. Ditto for "--if-exists" and "--if-missing".)
> >
> > It's possible that there was a reason why it was done this way.
> >
> > I think you might want to take a look at the discussions on the
> > mailing list when "interpret-trailers" was developed. There were a lot
> > of discussions over a long time, and there were a lot of requests and
> > suggestions about what it should do.
>
> I confess I haven't looked too deeply into the original threads
> surrounding the introduction of "interpret-trailers". But all of the
> changes which I categorize as bugfixes above have a theme of
> undocumented modifications.
>
> While working on this (and the larger) series around trailers, I only
> looked into some (not all) of the discussions on the mailing list in
> this area. Instead, I deferred to
> Documentation/git-interpret-trailers.txt as the official (authoritative)
> source of truth. This is partly why I first started out on this project
> last year by making improvements to that doc. And, this is why seeing so
> many edge cases where we perform such undocumented modifications smelled
> more like bugs to me than features.
>
> That being said, I cannot disagree with your position that the so-called
> bugfixes I've reported above could be misguided (and should rather be
> just updates to missing documentation). Let's not try to decide that
> here, but instead later when I get to introduce those changes on the
> list, one at a time. Thanks.
Yeah, it might seem like undocumented features are bad, and I agree
that reading original discussions can be tiring. But if the latter
makes it possible to fix undocumented features by just properly
documenting them, then I think it might just be the best thing to do.
Ok not to decide about it now though.
^ permalink raw reply
* Re: [PATCH v3 0/2] column: disallow negative padding
From: Junio C Hamano @ 2024-02-13 20:58 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Rubén Justo, Tiago Pascoal, Chris Torek, git
In-Reply-To: <dc1be818-e793-44f6-98dd-b159e043da28@app.fastmail.com>
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
>> There are not many users of OPT_INTEGER, and a quick check gives me
>> the impression (maybe wrong...) that many of them do not expect
>> negative values.
>>
>> Maybe we should consider having an OPT_INTEGER that fails if the
>> value supplied is negative. Ideally, some kind of opt-in machinery
>> could be desirable, I think, for example to include/exclude:
>>
>> - negative values
>> - "0" ( may not be a desired value )
>> - "-1" ( may have some special meaning )
>> - ...
>>
>> I'll leave the idea here, just in case it inspires someone. Thank
>> you.
Interesting.
I wonder if there is a correlation between "never negative" and
"handy if it took scale unit (like 2k to mean 2048)"? If so,
perhaps we can replace those that use OPT_INTEGER to use
OPT_MAGNITUDE instead.
Thanks.
^ permalink raw reply
* Re: [PATCH v3 2/2] column: guard against negative padding
From: Junio C Hamano @ 2024-02-13 20:59 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Rubén Justo, git
In-Reply-To: <6446a48c-4b9a-4095-9083-071f95ce3b84@app.fastmail.com>
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
> (All I know is that it can be tricky balancing such defensive checks
> with readability and maintanability.)
Personally I think v3 is a good enough place to stop and we are now
entering into the realm of diminishing returns.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 2/4] oidset: refactor oidset_insert_from_set()
From: Linus Arver @ 2024-02-13 21:02 UTC (permalink / raw)
To: Christian Couder, git
Cc: Junio C Hamano, Patrick Steinhardt, John Cai, Christian Couder,
Christian Couder
In-Reply-To: <20240208135055.2705260-3-christian.couder@gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> In a following commit, we will need to add all the oids from a set into
> another set. In "list-objects-filter.c", there is already a static
> function called add_all() to do that.
Nice find.
> Let's rename this function oidset_insert_from_set() and move it into
> oidset.{c,h} to make it generally available.
At some point (I don't ask for it in this series) we should add unit
tests for this newly-exposed function. Presumably the stuff around
object/oid handling is stable enough to receive unit tests.
> While at it, let's remove a useless `!= NULL`.
Nice cleanup. It would have been fine to also put this in a separate
patch, but as it is so simple I think it's also fine to keep it mixed in
with the move as you did here.
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> list-objects-filter.c | 11 +----------
> oidset.c | 10 ++++++++++
> oidset.h | 6 ++++++
> 3 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/list-objects-filter.c b/list-objects-filter.c
> index da287cc8e0..4346f8da45 100644
> --- a/list-objects-filter.c
> +++ b/list-objects-filter.c
> @@ -711,15 +711,6 @@ static void filter_combine__free(void *filter_data)
> free(d);
> }
>
> -static void add_all(struct oidset *dest, struct oidset *src) {
> - struct oidset_iter iter;
> - struct object_id *src_oid;
> -
> - oidset_iter_init(src, &iter);
> - while ((src_oid = oidset_iter_next(&iter)) != NULL)
> - oidset_insert(dest, src_oid);
> -}
> -
> static void filter_combine__finalize_omits(
> struct oidset *omits,
> void *filter_data)
> @@ -728,7 +719,7 @@ static void filter_combine__finalize_omits(
> size_t sub;
>
> for (sub = 0; sub < d->nr; sub++) {
> - add_all(omits, &d->sub[sub].omits);
> + oidset_insert_from_set(omits, &d->sub[sub].omits);
> oidset_clear(&d->sub[sub].omits);
> }
> }
> diff --git a/oidset.c b/oidset.c
> index d1e5376316..91d1385910 100644
> --- a/oidset.c
> +++ b/oidset.c
> @@ -23,6 +23,16 @@ int oidset_insert(struct oidset *set, const struct object_id *oid)
> return !added;
> }
>
> +void oidset_insert_from_set(struct oidset *dest, struct oidset *src)
> +{
> + struct oidset_iter iter;
> + struct object_id *src_oid;
> +
> + oidset_iter_init(src, &iter);
> + while ((src_oid = oidset_iter_next(&iter)))
Are the extra parentheses necessary?
> + oidset_insert(dest, src_oid);
> +}
> +
> int oidset_remove(struct oidset *set, const struct object_id *oid)
> {
> khiter_t pos = kh_get_oid_set(&set->set, *oid);
> diff --git a/oidset.h b/oidset.h
> index ba4a5a2cd3..262f4256d6 100644
> --- a/oidset.h
> +++ b/oidset.h
> @@ -47,6 +47,12 @@ int oidset_contains(const struct oidset *set, const struct object_id *oid);
> */
> int oidset_insert(struct oidset *set, const struct object_id *oid);
>
> +/**
> + * Insert all the oids that are in set 'src' into set 'dest'; a copy
> + * is made of each oid inserted into set 'dest'.
> + */
Just above in oid_insert() there is already a comment about needing to
copy each oid.
/**
* Insert the oid into the set; a copy is made, so "oid" does not need
* to persist after this function is called.
*
* Returns 1 if the oid was already in the set, 0 otherwise. This can be used
* to perform an efficient check-and-add.
*/
so perhaps the following wording is simpler?
Like oid_insert(), but insert all oids found in 'src'. Calls
oid_insert() internally.
> +void oidset_insert_from_set(struct oidset *dest, struct oidset *src);
Perhaps "oidset_insert_all" would be a simpler name? I generally prefer
to reuse any descriptors in comments to guide the names. Plus this
function used to be called "add_all()" so keeping the "all" naming style
feels right.
> +
> /**
> * Remove the oid from the set.
> *
> --
> 2.43.0.565.g97b5fd12a3.dirty
^ permalink raw reply
* Re: [PATCH 02/12] name-hash: add index_dir_exists2()
From: Junio C Hamano @ 2024-02-13 21:43 UTC (permalink / raw)
To: Jeff Hostetler via GitGitGadget; +Cc: git, Jeff Hostetler
In-Reply-To: <3464545fe3feceb08408618c77a70cc95745bfe9.1707857541.git.gitgitgadget@gmail.com>
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Jeff Hostetler <jeffhostetler@github.com>
>
> Create a new version of index_dir_exists() to return the canonical
> spelling of the matched directory prefix.
>
> 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 | 16 ++++++++++++++++
> name-hash.h | 2 ++
> 2 files changed, 18 insertions(+)
>
> diff --git a/name-hash.c b/name-hash.c
> index 251f036eef6..d735c81acb3 100644
> --- a/name-hash.c
> +++ b/name-hash.c
> @@ -694,6 +694,22 @@ int index_dir_exists(struct index_state *istate, const char *name, int namelen)
> dir = find_dir_entry(istate, name, namelen);
> return dir && dir->nr;
> }
> +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);
> +
> + if (dir && dir->nr)
> + strbuf_add(canonical_path, dir->name, dir->namelen);
> +
> + return dir && dir->nr;
> +}
>
> void adjust_dirname_case(struct index_state *istate, char *name)
Missing inter-function blank line, before the new function.
I wonder if we can avoid such repetition---the body of
index_dir_exists() is 100% shared with this new function.
Isn't it extremely unusual to receive "struct strbuf *" and call
strbuf_init() on it? It means that the caller is expected to have a
strbuf and pass a pointer to it, but also it is expected to leave
the strbuf uninitialized.
I'd understand if it calls strbuf_reset(), but it may not even be
necessary, if we make it responsibility of the caller to pass a
valid strbuf to be appended into.
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;
}
Then we can do
#define index_dir_exists(i, n, l) index_dir_find((i), (n), (l), NULL)
in the header for existing callers.
> {
> diff --git a/name-hash.h b/name-hash.h
> index b1b4b0fb337..2fcac5c4870 100644
> --- a/name-hash.h
> +++ b/name-hash.h
> @@ -5,6 +5,8 @@ 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);
> 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);
^ permalink raw reply
* [PATCH] credential/osxkeychain: store new attributes
From: M Hickford via GitGitGadget @ 2024-02-13 21:43 UTC (permalink / raw)
To: git; +Cc: M Hickford, M Hickford
From: M Hickford <mirth.hickford@gmail.com>
d208bfd (credential: new attribute password_expiry_utc, 2023-02-18)
and a5c76569e7 (credential: new attribute oauth_refresh_token)
introduced new credential attributes.
Similar to 7144dee3 (credential/libsecret: erase matching creds only,
2023-07-26), we encode the new attributes in the secret, separated by
newline:
hunter2
password_expiry_utc=1684189401
oauth_refresh_token=xyzzy
This is extensible and backwards compatible. The credential protocol
already assumes that attribute values do not contain newlines.
Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
[RFC] contrib/credential/osxkeychain: store new attributes
Is any keen MacOS user interested in building and testing this RFC
patch? I personally don't have a MacOS machine, so haven't tried
building it. Fixes are surely necessary. Once it builds, you can test
the feature with:
GIT_TEST_CREDENTIAL_HELPER=osxkeychain ./t0303-credential-external.sh
The feature would help git-credential-oauth users on MacOS
https://github.com/hickford/git-credential-oauth/issues/42
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1663%2Fhickford%2Fosxkeychain-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1663/hickford/osxkeychain-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1663
.../osxkeychain/git-credential-osxkeychain.c | 56 ++++++++++++++++++-
1 file changed, 54 insertions(+), 2 deletions(-)
diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
index 5f2e5f16c88..25ffa84f4ba 100644
--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
@@ -8,6 +8,8 @@ static char *host;
static char *path;
static char *username;
static char *password;
+static char *password_expiry_utc;
+static char *oauth_refresh_token;
static UInt16 port;
__attribute__((format (printf, 1, 2)))
@@ -22,6 +24,17 @@ static void die(const char *err, ...)
exit(1);
}
+
+static void *xmalloc(size_t size)
+{
+ void *ret = malloc(size);
+ if (!ret && !size)
+ ret = malloc(1);
+ if (!ret)
+ die("Out of memory");
+ return ret;
+}
+
static void *xstrdup(const char *s1)
{
void *ret = strdup(s1);
@@ -69,11 +82,27 @@ static void find_internet_password(void)
void *buf;
UInt32 len;
SecKeychainItemRef item;
+ char *line;
+ char *remaining_lines;
+ char *part;
+ char *remaining_parts;
if (SecKeychainFindInternetPassword(KEYCHAIN_ARGS, &len, &buf, &item))
return;
- write_item("password", buf, len);
+ line = strtok_r(buf, "\n", &remaining_lines);
+ write_item("password", line, strlen(line));
+ while(line != NULL) {
+ part = strtok_r(line, "=", &remaining_parts);
+ if (!strcmp(part, "oauth_refresh_token")) {
+ write_item("oauth_refresh_token", remaining_parts, strlen(remaining_parts));
+ }
+ if (!strcmp(part, "password_expiry_utc")) {
+ write_item("password_expiry_utc", remaining_parts, strlen(remaining_parts));
+ }
+ line = strtok_r(NULL, "\n", &remaining_lines);
+ }
+
if (!username)
find_username_in_item(item);
@@ -100,13 +129,32 @@ static void delete_internet_password(void)
static void add_internet_password(void)
{
+ int len;
+
/* Only store complete credentials */
if (!protocol || !host || !username || !password)
return;
+ char *secret;
+ if (password_expiry_utc && oauth_refresh_token) {
+ len = strlen(password) + strlen(password_expiry_utc) + strlen(oauth_refresh_token) + strlen("\npassword_expiry_utc=\noauth_refresh_token=");
+ secret = xmalloc(len);
+ snprintf(secret, len, len, "%s\npassword_expiry_utc=%s\noauth_refresh_token=%s", password, oauth_refresh_token);
+ } else if (oauth_refresh_token) {
+ len = strlen(password) + strlen(oauth_refresh_token) + strlen("\noauth_refresh_token=");
+ secret = xmalloc(len);
+ snprintf(secret, len, len, "%s\noauth_refresh_token=%s", password, oauth_refresh_token);
+ } else if (password_expiry_utc) {
+ len = strlen(password) + strlen(password_expiry_utc) + strlen("\npassword_expiry_utc=");
+ secret = xmalloc(len);
+ snprintf(secret, len, len, "%s\npassword_expiry_utc=%s", password, password_expiry_utc);
+ } else {
+ secret = xstrdup(password);
+ }
+
if (SecKeychainAddInternetPassword(
KEYCHAIN_ARGS,
- KEYCHAIN_ITEM(password),
+ KEYCHAIN_ITEM(secret),
NULL))
return;
}
@@ -161,6 +209,10 @@ static void read_credential(void)
username = xstrdup(v);
else if (!strcmp(buf, "password"))
password = xstrdup(v);
+ else if (!strcmp(buf, "password_expiry_utc"))
+ password_expiry_utc = xstrdup(v);
+ else if (!strcmp(buf, "oauth_refresh_token"))
+ oauth_refresh_token = xstrdup(v);
/*
* Ignore other lines; we don't know what they mean, but
* this future-proofs us when later versions of git do
base-commit: c875e0b8e036c12cfbf6531962108a063c7a821c
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 01/12] sparse-index: pass string length to index_file_exists()
From: Junio C Hamano @ 2024-02-13 22:07 UTC (permalink / raw)
To: Jeff Hostetler via GitGitGadget; +Cc: git, Jeff Hostetler
In-Reply-To: <6f81e2e30609c70d4dcdbe9d4f11c4f6b5173c77.1707857541.git.gitgitgadget@gmail.com>
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Jeff Hostetler <jeffhostetler@github.com>
>
> The call to index_file_exists() in the loop in expand_to_path() passes
> the wrong string length. Let's fix that.
>
> The loop in expand_to_path() searches the name-hash for each
> sub-directory prefix in the provided pathname. That is, by searching
> for "dir1/" then "dir1/dir2/" then "dir1/dir2/dir3/" and so on until
> it finds a cache-entry representing a sparse directory.
>
> The code creates "strbuf path_mutable" to contain the working pathname
> and modifies the buffer in-place by temporarily replacing the character
> following each successive "/" with NUL for the duration of the call to
> index_file_exists().
>
> It does not update the strbuf.len during this substitution.
>
> Pass the patched length of the prefix path instead.
>
> Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
> ---
This looked familiar, and it turns out that
https://lore.kernel.org/git/pull.1649.git.1706897095273.gitgitgadget@gmail.com/
has already been merged to 'master'.
> sparse-index.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sparse-index.c b/sparse-index.c
> index 3578feb2837..e48e40cae71 100644
> --- a/sparse-index.c
> +++ b/sparse-index.c
> @@ -579,8 +579,9 @@ void expand_to_path(struct index_state *istate,
> replace++;
> temp = *replace;
> *replace = '\0';
> + substr_len = replace - path_mutable.buf;
> if (index_file_exists(istate, path_mutable.buf,
> - path_mutable.len, icase)) {
> + substr_len, icase)) {
> /*
> * We found a parent directory in the name-hash
> * hashtable, because only sparse directory entries
> @@ -593,7 +594,6 @@ void expand_to_path(struct index_state *istate,
> }
>
> *replace = temp;
> - substr_len = replace - path_mutable.buf;
> }
>
> cleanup:
^ permalink raw reply
* Re: [PATCH v2 4/4] rev-list: allow missing tips with --missing=[print|allow*]
From: Linus Arver @ 2024-02-13 22:33 UTC (permalink / raw)
To: Christian Couder, git
Cc: Junio C Hamano, Patrick Steinhardt, John Cai, Christian Couder,
Christian Couder
In-Reply-To: <20240208135055.2705260-5-christian.couder@gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> In 9830926c7d (rev-list: add commit object support in `--missing`
> option, 2023-10-27) we fixed the `--missing` option in `git rev-list`
> so that it works with with missing commits, not just blobs/trees.
>
> Unfortunately, such a command would still fail with a "fatal: bad
> object <oid>" if it is passed a missing commit, blob or tree as an
> argument (before the rev walking even begins).
>
> When such a command is used to find the dependencies of some objects,
> for example the dependencies of quarantined objects (see the
> "QUARANTINE ENVIRONMENT" section in the git-receive-pack(1)
> documentation), it would be better if the command would instead
> consider such missing objects, especially commits, in the same way as
> other missing objects.
Looks good.
> If, for example `--missing=print` is used, it would be nice for some
> use cases if the missing tips passed as arguments were reported in
> the same way as other missing objects instead of the command just
> failing.
Nit: this paragraph sounds a bit redundant but it is giving an explicit
example of `--missing=print` which was not discussed so far, so I think
it's fine as is.
> We could introduce a new option to make it work like this, but most
> users are likely to prefer the command to have this behavior as the
> default one. Introducing a new option would require another dumb loop
> to look for that options early, which isn't nice.
s/options/option
> Also we made `git rev-list` work with missing commits very recently
> and the command is most often passed commits as arguments. So let's
> consider this as a bug fix related to these previous change.
s/previous change/recent changes
> While at it let's add a NEEDSWORK comment to say that we should get
> rid of the existing ugly dumb loops that parse the
> `--exclude-promisor-objects` and `--missing=...` options early.
>
> Helped-by: Linus Arver <linusa@google.com>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> Documentation/rev-list-options.txt | 4 +++
> builtin/rev-list.c | 15 +++++++-
> revision.c | 14 ++++++--
> t/t6022-rev-list-missing.sh | 56 ++++++++++++++++++++++++++++++
> 4 files changed, 85 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index a583b52c61..bb753b6292 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -1019,6 +1019,10 @@ Unexpected missing objects will raise an error.
> +
> The form '--missing=print' is like 'allow-any', but will also print a
> list of the missing objects. Object IDs are prefixed with a ``?'' character.
> ++
> +If some tips passed to the traversal are missing, they will be
> +considered as missing too, and the traversal will ignore them. In case
> +we cannot get their Object ID though, an error will be raised.
The only other mention of the term "tips" is for the "--alternate-refs"
flag on line 215 which uses "ref tips". Perhaps this is obvious to
veteran Git developers but I do wonder if we need to somehow define it
(however briefly) the first time we mention it (either in the document
as a whole, or just within this newly added paragraph).
Here's an alternate wording
Ref tips given on the command line are a special case. They are
first dereferenced to Object IDs (if this is not possible, an error
will be raised). Then these IDs are checked to see if the objects
they refer to exist. If so, the traversal happens starting with
these tips. Otherwise, then such tips will not be used for
traversal.
Even though such missing tips won't be included for traversal, for
purposes of the `--missing` flag they will be treated the same way
as those objects that did get traversed (and were determined to be
missing). For example, if `--missing=print` is given then the Object
IDs of these tips will be printed just like all other missing
objects encountered during traversal.
But also, I realize that these documentation touch-ups might be better
served by an overall pass over the whole document, so it's fine if we
decide not to take this suggestion at this time.
Aside: unfortunately we don't seem to define the relationship between
ref tips (e.g., "HEAD"), object IDs (40-char hex string), and the actual
objects (the real data that we traverse over). It's probably another
thing that could be fixed up in the docs in the future.
> --exclude-promisor-objects::
> (For internal use only.) Prefilter object traversal at
> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index b3f4783858..ec9556f135 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -545,6 +545,15 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
> *
> * Let "--missing" to conditionally set fetch_if_missing.
> */
> + /*
> + * NEEDSWORK: These dump loops to look for some options early
> + * are ugly.
I agree with Junio's suggestion to use more objective language.
> We really need setup_revisions() to have a
> + * mechanism to allow and disallow some sets of options for
> + * different commands (like rev-list, replay, etc). Such
s/Such/Such a
> + * mechanism should do an early parsing of option and be able
s/option/options
> + * to manage the `--exclude-promisor-objects` and `--missing=...`
> + * options below.
> + */
> for (i = 1; i < argc; i++) {
> const char *arg = argv[i];
> if (!strcmp(arg, "--exclude-promisor-objects")) {
>
> [...]
>
> @@ -2178,13 +2183,18 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
> if (revarg_opt & REVARG_COMMITTISH)
> get_sha1_flags |= GET_OID_COMMITTISH;
>
> + /*
> + * Even if revs->do_not_die_on_missing_objects is set, we
> + * should error out if we can't even get an oid, ...
> + */
Perhaps this wording is more precise?
If we can't even get an oid, we are forced to error out (regardless
of revs->do_not_die_on_missing_objects) because a valid traversal
must start from *some* valid oid. OTOH we ignore the ref tip
altogether with revs->ignore_missing.
> + * ... as
> + * `--missing=print` should be able to report missing oids.
I think this comment would be better placed ...
> if (get_oid_with_context(revs->repo, arg, get_sha1_flags, &oid, &oc))
> return revs->ignore_missing ? 0 : -1;
> if (!cant_be_filename)
> verify_non_filename(revs->prefix, arg);
> object = get_reference(revs, arg, &oid, flags ^ local_flags);
... around here.
> if (!object)
> - return revs->ignore_missing ? 0 : -1;
> + return (revs->ignore_missing || revs->do_not_die_on_missing_objects) ? 0 : -1;
> add_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);
> add_pending_object_with_path(revs, object, arg, oc.mode, oc.path);
> free(oc.path);
> @@ -3830,8 +3840,6 @@ int prepare_revision_walk(struct rev_info *revs)
> FOR_EACH_OBJECT_PROMISOR_ONLY);
> }
>
> - oidset_init(&revs->missing_commits, 0);
> -
> if (!revs->reflog_info)
> prepare_to_use_bloom_filter(revs);
> if (!revs->unsorted_input)
> diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
> index 5f1be7abb5..78387eebb3 100755
> --- a/t/t6022-rev-list-missing.sh
> +++ b/t/t6022-rev-list-missing.sh
> @@ -78,4 +78,60 @@ do
> done
> done
>
> +for missing_tip in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
> +do
> + # We want to check that things work when both
> + # - all the tips passed are missing (case existing_tip = ""), and
> + # - there is one missing tip and one existing tip (case existing_tip = "HEAD")
> + for existing_tip in "" "HEAD"
> + do
Though I am biased, these new variable names do make this test that much
easier to read. Thanks.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox