Builtin FSMonitor Part 2.5 #1174
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
There are issues in commit 8780488: |
|
There are issues in commit 8d977e4: |
|
There are issues in commit 5657465: |
|
There are issues in commit acaed47: |
|
There are issues in commit d745c48: |
|
There are issues in commit a80a383: |
|
There are issues in commit 5547978: |
|
There are issues in commit 7fa6f69: |
|
There are issues in commit 7057d60: |
|
There are issues in commit 51ce7e8: |
|
There are issues in commit b0d6945: |
|
There are issues in commit b50677e: |
|
There are issues in commit e1c2ea9: |
|
There are issues in commit 933955a: |
|
There are issues in commit c725c27: |
|
There are issues in commit 80282f3: |
fixup! help: include fsmonitor--daemon feature flag in version info Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
fixup! update-index: convert fsmonitor warnings to advise Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
jeffhostetler
force-pushed
the
builtin-fsmonitor-part2.5
branch
from
80282f3
to
4a12e29
Compare
|
There are issues in commit 0850c4e: |
|
There are issues in commit 82c19ad: |
|
There are issues in commit ed52c98: |
|
There are issues in commit 669b068: |
fixup! compat/fsmonitor/fsm-listen-darwin: add MacOS header files \ for FSEvent Split out GCC-specific MacOS declarations into separate file from the clang-specific header file includes to reduce visual noise. Eliminate unnecessary includes when using clang. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
fixup! t/helper/fsmonitor-client: create IPC client to talk to \ FSMonitor Daemon Elminate unnecessary code in cmd__fsmonitor_client() WRT parsing of options. Fix name of test-tool in usage. Don't localize die() message. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
fixup! fsmonitor--daemon: use a cookie file to sync with file system Use implicit definitions for FCIR_ enum values. Remove const from cookie->name. Reverse if then and else branches around open() to ease readability. Document that we don't care about errors from close() and unlink(). Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
fixup! t/perf/p7519: speed up test on Windows Use grep rather than egrep Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
fixup! t/perf/p7519: add fsmonitor--daemon test cases Add "_hook" to name of shell function to set up for Watchman/hook test runs. Fix code style of added "if then". Add body of builtin test to a test_expect_success. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
fixup! t7527: create test for fsmonitor--daemon Add parameters to start_daemon() and handle subshell and logging args. Remove /dev/null from commands. Fix &&-chaining of functions. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
fixup! t7527: test status with untracked-cache and fsmonitor--daemon Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -1238,18 +1238,18 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) | |||
| if (fsmonitor > 0) { | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> fixup! update-index: convert fsmonitor warnings to advise
Same comment as 01/16 applies here. "convert ... back to ..." in
the title refers to the fact that builtin-fsmonitor-part2 topic
turned warning() into advise() without a good justification, I
think. Flipping and flopping between warning and advise, without
giving any justification going either direction, is not a good move.
I only have looked at one eighth of this part 2.5, but it starts to
look that ejecting part-2 and redoing it may result in a cleaner
code that is easier to understand, perhaps? For example, instead of
applying this patch, we can just get rid of 1a9241e1 (update-index:
convert fsmonitor warnings to advise, 2022-03-01). As I read more,
my impression will certainly change, I would expect. Let's see.
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
> builtin/update-index.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index d335f1ac72a..75d646377cc 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -1238,18 +1238,18 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
> if (fsmonitor > 0) {
> enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r);
> if (fsm_mode == FSMONITOR_MODE_DISABLED) {
> - advise(_("core.fsmonitor is unset; "
> - "set it if you really want to "
> - "enable fsmonitor"));
> + warning(_("core.fsmonitor is unset; "
> + "set it if you really want to "
> + "enable fsmonitor"));
> }
> add_fsmonitor(&the_index);
> report(_("fsmonitor enabled"));
> } else if (!fsmonitor) {
> enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r);
> if (fsm_mode > FSMONITOR_MODE_DISABLED)
> - advise(_("core.fsmonitor is set; "
> - "remove it if you really want to "
> - "disable fsmonitor"));
> + warning(_("core.fsmonitor is set; "
> + "remove it if you really want to "
> + "disable fsmonitor"));
> remove_fsmonitor(&the_index);
> report(_("fsmonitor disabled"));
> }
There was a problem hiding this comment.
On the Git mailing list, Jeff Hostetler wrote (reply to this):
On 3/14/22 2:08 AM, Junio C Hamano wrote:
> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> fixup! update-index: convert fsmonitor warnings to advise
> > Same comment as 01/16 applies here. "convert ... back to ..." in
> the title refers to the fact that builtin-fsmonitor-part2 topic
> turned warning() into advise() without a good justification, I
> think. Flipping and flopping between warning and advise, without
> giving any justification going either direction, is not a good move.
Sorry for not including the backstory.
Ben wrote the original warning message back in 2017.
In my original version of part 2 I added a more detailed warning message
because of the new `core.useBuiltinFSMonitor` config settings and
how it interacted with `core.fsmonitor`. And we talked about making
that longer message advise rather than a warning. So I changed them
to advise().
But then we decided to remove `core.useBuiltinFSMonitor` and overload
`core.fsmonitor` to take a boolean, so the original text of the message
was sufficient and correct. So to minimize the diff, I reverted the
text change and kept the change from warning() to advise().
But then there were comments from AEvar on either changing all of
the nearby warning() calls to advise() *and* to change them to use
the `advise_type` enum. That seemed like a large/disruptive change
at this point.
Also, I wasn't really sure of the need for Ben's original warning
message, so I'd rather leave it as is than expand the scope.
So I basically reverted the change for this series. If (in a later
series) we want to revisit all of the warnings in update-index.c,
then we can think about this.
Jeff
| @@ -11,30 +11,85 @@ then | |||
| fi | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> fixup! t7527: test status with untracked-cache and fsmonitor--daemon
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
> t/t7527-builtin-fsmonitor.sh | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
> index 026382a0d86..f60e211dbab 100755
> --- a/t/t7527-builtin-fsmonitor.sh
> +++ b/t/t7527-builtin-fsmonitor.sh
> @@ -536,9 +536,9 @@ matrix_clean_up_repo () {
> }
>
> matrix_try () {
> - uc=$1
> - fsm=$2
> - fn=$3
> + uc=$1 &&
> + fsm=$2 &&
> + fn=$3 &&
After seeing up to this step, I am reasonably well convinced that
what we want is to kick the jh/builtin-fsmonitor-part2 topic back to
'seen', and you send instead of part2.5 an updated part2, with
range-diff since the last round and this final iteration. A change
like the above will be seen in the range-diff in the cover letter
and most of them, like the above, will become trivial improvements,
and then the result can hopefully be placed back in 'next' reasonably
fast.
Opinions?
| @@ -49,7 +49,7 @@ static int do_send_query(const char *token) | |||
|
|
|||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Fri, Mar 11 2022, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> fixup! t/helper/fsmonitor-client: create IPC client to talk to \
> FSMonitor Daemon
>
> Elminate unnecessary code in cmd__fsmonitor_client() WRT
> parsing of options.
>
> Fix name of test-tool in usage.
>
> Don't localize die() message.
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
> t/helper/test-fsmonitor-client.c | 17 ++++++-----------
> 1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/t/helper/test-fsmonitor-client.c b/t/helper/test-fsmonitor-client.c
> index f7a5b3a32fa..d59a640f1f9 100644
> --- a/t/helper/test-fsmonitor-client.c
> +++ b/t/helper/test-fsmonitor-client.c
> @@ -49,7 +49,7 @@ static int do_send_query(const char *token)
>
> ret = fsmonitor_ipc__send_query(token, &answer);
> if (ret < 0)
> - die(_("could not query fsmonitor--daemon"));
> + die("could not query fsmonitor--daemon");
Good, since this is just a test helper.
> write_in_full(1, answer.buf, answer.len);
> strbuf_release(&answer);
> @@ -85,8 +85,8 @@ int cmd__fsmonitor_client(int argc, const char **argv)
> const char *token = NULL;
>
> const char * const fsmonitor_client_usage[] = {
> - N_("test-helper fsmonitor-client query [<token>]"),
> - N_("test-helper fsmonitor-client flush"),
> + N_("test-tool fsmonitor-client query [<token>]"),
> + N_("test-tool fsmonitor-client flush"),
These are still marked for N_() translation.
Even if tehse were built-ins only the one containing <token> should have
N_(), as the other one is a literal command whose translation won't
change.
> NULL,
> };
>
> @@ -96,17 +96,12 @@ int cmd__fsmonitor_client(int argc, const char **argv)
> OPT_END()
> };
>
> - if (argc < 2)
> - usage_with_options(fsmonitor_client_usage, options);
> + argc = parse_options(argc, argv, NULL, options, fsmonitor_client_usage, 0);
>
> - if (argc == 2 && !strcmp(argv[1], "-h"))
> + if (argc != 1)
> usage_with_options(fsmonitor_client_usage, options);
>
> - subcmd = argv[1];
> - argv--;
> - argc++;
> -
> - argc = parse_options(argc, argv, NULL, options, fsmonitor_client_usage, 0);
> + subcmd = argv[0];
>
> setup_git_directory();
Looks good.
|
User |
| @@ -141,7 +141,7 @@ test_expect_success "one time repo setup" ' | |||
| fi | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Fri, Mar 11 2022, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhost@microsoft.com>
> Fix code style of added "if then".
*nod*
> Add body of builtin test to a test_expect_success.
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
> t/perf/p7519-fsmonitor.sh | 21 +++++++++++----------
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh
> index 5f97edf6a11..7a7981b0e61 100755
> --- a/t/perf/p7519-fsmonitor.sh
> +++ b/t/perf/p7519-fsmonitor.sh
> @@ -141,7 +141,7 @@ test_expect_success "one time repo setup" '
> fi
> '
>
> -setup_for_fsmonitor () {
> +setup_for_fsmonitor_hook () {
> # set INTEGRATION_SCRIPT depending on the environment
> if test -n "$INTEGRATION_PATH"
> then
> @@ -182,8 +182,7 @@ test_perf_w_drop_caches () {
> }
>
> test_fsmonitor_suite () {
> - if test -n "$USE_FSMONITOR_DAEMON"
> - then
> + if test -n "$USE_FSMONITOR_DAEMON"; then
> DESC="builtin fsmonitor--daemon"
This "; then" etc. still uses the non-standard style (and this was added
in aa072da617e (t/perf/p7519: add fsmonitor--daemon test cases,
2022-03-01).
| @@ -109,14 +109,14 @@ static int do_as_client__status(void) | |||
|
|
|||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Fri, Mar 11 2022, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> fixup! fsmonitor--daemon: use a cookie file to sync with file system
>
> Use implicit definitions for FCIR_ enum values.
>
> Remove const from cookie->name.
>
> Reverse if then and else branches around open() to ease readability.
>
> Document that we don't care about errors from close() and unlink().
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
> builtin/fsmonitor--daemon.c | 53 +++++++++++++++++++++----------------
> 1 file changed, 30 insertions(+), 23 deletions(-)
>
> diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
> index 97ca2a356e5..02a99ce98a2 100644
> --- a/builtin/fsmonitor--daemon.c
> +++ b/builtin/fsmonitor--daemon.c
> @@ -109,14 +109,14 @@ static int do_as_client__status(void)
>
> enum fsmonitor_cookie_item_result {
> FCIR_ERROR = -1, /* could not create cookie file ? */
> - FCIR_INIT = 0,
> + FCIR_INIT,
> FCIR_SEEN,
> FCIR_ABORT,
> };
>
> struct fsmonitor_cookie_item {
> struct hashmap_entry entry;
> - const char *name;
> + char *name;
> enum fsmonitor_cookie_item_result result;
> };
>
> @@ -166,37 +166,44 @@ static enum fsmonitor_cookie_item_result with_lock__wait_for_cookie(
> * that the listener thread has seen it.
> */
> fd = open(cookie_pathname.buf, O_WRONLY | O_CREAT | O_EXCL, 0600);
> - if (fd >= 0) {
> - close(fd);
> - unlink(cookie_pathname.buf);
> -
> - /*
> - * Technically, this is an infinite wait (well, unless another
> - * thread sends us an abort). I'd like to change this to
> - * use `pthread_cond_timedwait()` and return an error/timeout
> - * and let the caller do the trivial response thing, but we
> - * don't have that routine in our thread-utils.
> - *
> - * After extensive beta testing I'm not really worried about
> - * this. Also note that the above open() and unlink() calls
> - * will cause at least two FS events on that path, so the odds
> - * of getting stuck are pretty slim.
> - */
> - while (cookie->result == FCIR_INIT)
> - pthread_cond_wait(&state->cookies_cond,
> - &state->main_lock);
> - } else {
> + if (fd < 0) {
> error_errno(_("could not create fsmonitor cookie '%s'"),
> cookie->name);
>
> cookie->result = FCIR_ERROR;
> + goto done;
> }
>
> + /*
> + * Technically, close() and unlink() can fail, but we don't
> + * care here. We only created the file to trigger a watch
> + * event from the FS to know that when we're up to date.
> + */
> + close(fd);
It still seems odd to explicitly want to ignore close() return values.
I realize that we do in (too many) existing places, but why wouldn't we
want to e.g. catch an I/O error here early?
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 3/14/2022 4:00 AM, Ævar Arnfjörð Bjarmason wrote:
>
> On Fri, Mar 11 2022, Jeff Hostetler via GitGitGadget wrote:
>> + /*
>> + * Technically, close() and unlink() can fail, but we don't
>> + * care here. We only created the file to trigger a watch
>> + * event from the FS to know that when we're up to date.
>> + */
>> + close(fd);
>
> It still seems odd to explicitly want to ignore close() return values.
>
> I realize that we do in (too many) existing places, but why wouldn't we
> want to e.g. catch an I/O error here early?
What exactly do you propose we do here if there is an I/O error
during close()?
Thanks,
-Stolee
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Derrick Stolee <derrickstolee@github.com> writes:
> On 3/14/2022 4:00 AM, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Fri, Mar 11 2022, Jeff Hostetler via GitGitGadget wrote:
>
>>> + /*
>>> + * Technically, close() and unlink() can fail, but we don't
>>> + * care here. We only created the file to trigger a watch
>>> + * event from the FS to know that when we're up to date.
>>> + */
>>> + close(fd);
>>
>> It still seems odd to explicitly want to ignore close() return values.
>>
>> I realize that we do in (too many) existing places, but why wouldn't we
>> want to e.g. catch an I/O error here early?
>
> What exactly do you propose we do here if there is an I/O error
> during close()?
We created the file to trigger a watch event, but now we have a
reason to suspect that the wished-for watch event may not come.
We only did so to know that when we're up to date. Now we may never
know? We may go without realizing we are already up to date a bit
longer than the reality?
How much damage would it cause us to miss a watch event in this
case? Very little? Is it a thing that sysadmins may care if we see
too many of, but there is nothing the end user can immediately do
about? If it is, perhaps a trace2 event to report it (and other "we
do not care here" syscalls that fail)?
There was a problem hiding this comment.
On the Git mailing list, Jeff Hostetler wrote (reply to this):
On 3/14/22 1:47 PM, Junio C Hamano wrote:
> Derrick Stolee <derrickstolee@github.com> writes:
> >> On 3/14/2022 4:00 AM, Ævar Arnfjörð Bjarmason wrote:
>>>
>>> On Fri, Mar 11 2022, Jeff Hostetler via GitGitGadget wrote:
>>
>>>> + /*
>>>> + * Technically, close() and unlink() can fail, but we don't
>>>> + * care here. We only created the file to trigger a watch
>>>> + * event from the FS to know that when we're up to date.
>>>> + */
>>>> + close(fd);
>>>
>>> It still seems odd to explicitly want to ignore close() return values.
>>>
>>> I realize that we do in (too many) existing places, but why wouldn't we
>>> want to e.g. catch an I/O error here early?
>>
>> What exactly do you propose we do here if there is an I/O error
>> during close()?
> > We created the file to trigger a watch event, but now we have a
> reason to suspect that the wished-for watch event may not come.
> > We only did so to know that when we're up to date. Now we may never
> know? We may go without realizing we are already up to date a bit
> longer than the reality?
> > How much damage would it cause us to miss a watch event in this
> case? Very little? Is it a thing that sysadmins may care if we see
> too many of, but there is nothing the end user can immediately do
> about? If it is, perhaps a trace2 event to report it (and other "we
> do not care here" syscalls that fail)?
> > > The open(... O_CREAT ...) succeeded, so we actually created a
new file and expect a FS event for it. That FS event (when seen
by the FS listener thread) will cause our condition to be
signaled and allow this thread to wake up and respond to the client.
The odds of the close() failing on a plain file (after a successful
open()) are very slim. And there's nothing that we can do about
the failure anyway. (And we're not relying on an FS event from the
close() succeeding, so it really doesn't matter.) Technically, it
is possible that the daemon could run out of fd's if this close()
fails often, so at some point the daemon might not be able to create
new cookie files. But the daemon currently defaults to sending a
trivial response to the client -- if this turns out to be a real
issue, we could have the daemon restart or something, but I'm not
going to worry about that right now.
The odds of a failure in unlink() is a little more interesting.
This would mean that a stale cookie file would be left in the
cookie directory (and waste a little disk space). But that is
not likely either (for a plain file that we just created).
Since we're not relying on the FS event for the unlink(), the
failure here won't block the current thread either. Deleting
stale cookie files is something that we could try to address
in the future if it turns out to be a problem.
Jeff
| @@ -11,30 +11,85 @@ then | |||
| fi | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Fri, Mar 11 2022, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> fixup! t7527: create test for fsmonitor--daemon
>
> Add parameters to start_daemon() and handle subshell and logging args.
>
> Remove /dev/null from commands.
>
> Fix &&-chaining of functions.
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
> t/t7527-builtin-fsmonitor.sh | 217 ++++++++++++++++++-----------------
> 1 file changed, 111 insertions(+), 106 deletions(-)
>
> diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
> index 0ccbfb9616f..026382a0d86 100755
> --- a/t/t7527-builtin-fsmonitor.sh
> +++ b/t/t7527-builtin-fsmonitor.sh
> @@ -11,30 +11,85 @@ then
> fi
>
> stop_daemon_delete_repo () {
> - r=$1
> - git -C $r fsmonitor--daemon stop >/dev/null 2>/dev/null
> + r=$1 &&
> + test_might_fail git -C $r fsmonitor--daemon stop &&
> rm -rf $1
> - return 0
> }
>
> start_daemon () {
> - case "$#" in
> - 1) r="-C $1";;
> - *) r="";
> - esac
> + r="" &&
> + tf="" &&
> + t2="" &&
> + tk="" &&
We usually just do :
x= &&
Not:
x="" &&
I.e. no need for the "".
>
> - git $r fsmonitor--daemon start || return $?
> - git $r fsmonitor--daemon status || return $?
> + while test "$#" -ne 0
> + do
> + case "$1" in
> + -C)
> + shift;
> + test "$#" -ne 0 ||
> + { echo >&2 "error: -C requires arg"; exit 1; }
IMO this exhaustive checking is better just skipped, we e.g.don't do
this in "test_commit". Can't you just...
> + r="-C $1"
> + shift
...properly &&-chain these whole things and have the "shift too many"
and exit code from "shift" catch this?
> + ;;
> + -tf)
> + shift;
> + test "$#" -ne 0 ||
> + { echo >&2 "error: -tf requires arg"; exit 1; }
> + tf="$1"
> + shift
> + ;;
> + -t2)
> + shift;
> + test "$#" -ne 0 ||
> + { echo >&2 "error: -t2 requires arg"; exit 1; }
> + t2="$1"
> + shift
> + ;;
> + -tk)
> + shift;
> + test "$#" -ne 0 ||
> + { echo >&2 "error: -tk requires arg"; exit 1; }
> + tk="$1"
> + shift
> + ;;
> + *)
ditto.
> + echo >&2 "error: unknown option: '$1'"
> + exit 1
But this sort of case is needed, but should use BUG "..."
> + ;;
> + esac
> + done &&
> +
> + (
> + if test ! -z "$tf"
Instead of "! -z x" use "-n x" ?
> + then
> + GIT_TRACE_FSMONITOR="$tf"
> + export GIT_TRACE_FSMONITOR
> + fi &&
> +
> + if test ! -z "$t2"
> + then
> + GIT_TRACE2_PERF="$t2"
> + export GIT_TRACE2_PERF
> + fi &&
> +
> + if test ! -z "$tk"
ditto.
> + then
> + GIT_TEST_FSMONITOR_TOKEN="$tk"
> + export GIT_TEST_FSMONITOR_TOKEN
> + fi &&
> - touch test_flush/file_1 &&
> - touch test_flush/file_2 &&
> + > test_flush/file_1 &&
> + > test_flush/file_2 &&
style: ">x" not "> x" (ditto below).
|
User |
| @@ -22,11 +22,10 @@ static void lookup_fsmonitor_settings(struct repository *r) | |||
| return; | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> fixup! fsmonitor: config settings are repository-specific
I cannot tell the validity of this change alone without the base
commit in part2 to choose between setting s->mode directly or
calling __set_disabled(r).
I've read all the 16 patches here, and I found that the majority of
them (except for "it is hard to tell why we are changing in 2.5; it
needs a better justification" ones like this) are "oops, the part2
patches had these obvious and trivial mistakes that we should have
fixed before we merged them to 'next', or even sending them to the
list in the first place" fixes.
Let's kick part2 out of 'next', and replace it with a corrected set
of patches and have people review them, this time for real, before
merging it back to 'next'. I just do not get the good feel that the
series was adequately reviewed---if we have this many "oops" fixups
needed after getting merged to 'next'.
Thanks.
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
> fsmonitor-settings.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fsmonitor-settings.c b/fsmonitor-settings.c
> index 3b54e7a51f6..757d230d538 100644
> --- a/fsmonitor-settings.c
> +++ b/fsmonitor-settings.c
> @@ -22,11 +22,10 @@ static void lookup_fsmonitor_settings(struct repository *r)
> return;
>
> CALLOC_ARRAY(s, 1);
> + s->mode = FSMONITOR_MODE_DISABLED;
>
> r->settings.fsmonitor = s;
>
> - fsm_settings__set_disabled(r);
> -
> /*
> * Overload the existing "core.fsmonitor" config setting (which
> * has historically been either unset or a hook pathname) to
There was a problem hiding this comment.
On the Git mailing list, Jeff Hostetler wrote (reply to this):
On 3/14/22 3:49 PM, Junio C Hamano wrote:
> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> fixup! fsmonitor: config settings are repository-specific
> > > Let's kick part2 out of 'next', and replace it with a corrected set
> of patches and have people review them, this time for real, before
> merging it back to 'next'. I just do not get the good feel that the
> series was adequately reviewed---if we have this many "oops" fixups
> needed after getting merged to 'next'.
Yeah if you haven't merged part2 to master yet, wait and i'll
send a V7 of part2 that squashes in these fixups.
and addresses any other comments on part2.5 itself.
Thanks and sorry.
Jeff
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Jeff Hostetler <git@jeffhostetler.com> writes:
> Yeah if you haven't merged part2 to master yet, wait and i'll
> send a V7 of part2 that squashes in these fixups.
>
> and addresses any other comments on part2.5 itself.
Thanks. I think that would be cleaner in the end.
|
This branch is now known as |
|
This patch series was integrated into seen via git@b6309a4. |
|
There was a status update in the "New Topics" section about the branch Various small fixes and cleanups on part-2 of the same topic. Needs review. source: <pull.1174.git.1647033303.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@80d7ae2. |
|
User |
|
This patch series was integrated into seen via git@19265b0. |
|
This patch series was integrated into seen via git@6974796. |
|
This patch series was integrated into seen via git@028f888. |
|
This patch series was integrated into seen via git@c61b0b6. |
|
There was a status update in the "Cooking" section about the branch Various small fixes and cleanups on part-2 of the same topic. Needs review. source: <pull.1174.git.1647033303.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@938a209. |
|
This patch series was integrated into seen via git@bea1cc1. |
|
This patch series was integrated into seen via git@46acb22. |
|
This patch series was integrated into seen via git@5b99e56. |
|
This patch series was integrated into seen via git@186e454. |
|
This patch series was integrated into seen via git@053b5a1. |
|
This patch series was integrated into seen via git@1479246. |
|
I've squashed the code into part 2, so this part 2.5 is obsolete. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm calling this series part 2.5. It should be inserted between parts 2 and 3.
I'll send a new version of part 3 after I send part 2.5.
This series is a bunch of "fixup!" commits for part 2. It addresses feedback
on part 2 that wasn't received until after part 2 had graduated to "next".
I was originally planning to include them at the beginning of part 3, but
since there are so many, I thought it would be easier to add a fixup series
in the middle. (And since GGG has a limit of 30 commits, I couldn't add
them to either existing series without going over the limit.)
Most of the commits deal with translation markings for die/error messages
and fixing && chains in the tests. And there were a few readability improvements.
cc: Ævar Arnfjörð Bjarmason avarab@gmail.com
cc: Derrick Stolee derrickstolee@github.com
cc: Jeff Hostetler git@jeffhostetler.com