* Re: Feasibility of folding `unit-tests` into `make test`, was Re: [PATCH] ci: avoid running the test suite _twice_
From: Josh Steadmon @ 2024-01-04 23:54 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Jeff King, Johannes Schindelin via GitGitGadget, Phillip Wood,
git
In-Reply-To: <850ea42c-f103-68d5-896b-9120e2628686@gmx.de>
On 2023.11.16 09:42, Johannes Schindelin wrote:
> Hi Josh,
>
> On Wed, 15 Nov 2023, Josh Steadmon wrote:
[snip]
> > If I was forced to pick a way to get everything under one process, I'd
> > lean towards autogenerating individual shell script wrappers for each
> > unit test. But I'm open to discussion, especially if people have other
> > approaches I haven't thought of.
>
> One alternative would be to avoid running the unit tests via `prove` in
> the first place.
>
> For example, we could use the helper from be5d88e11280 (test-tool
> run-command: learn to run (parts of) the testsuite, 2019-10-04) [*1*]. It
> would probably need a few improvements, but certainly no wizardry nor
> witchcraft would be required. It would also help on Windows, where running
> a simple test helper written in C is vastly faster than running a complex
> Perl script (which `prove` is).
>
> Ciao,
> Johannes
>
> Footnote *1*: I had always wanted to improve that test helper to the point
> where it could replace our use of `prove`, at least on Windows. It seems,
> however, that as of 4c2c38e800f3 (ci: modification of main.yml to use
> cmake for vs-build job, 2020-06-26) we do not use the helper at all
> anymore. Hopefully it can still be useful. 🤞
Sorry for the silence on this topic; the holidays plus some family
illnesses kept me away from the list for a while. I have a working
implementation of this. I plan on cleaning it up a bit and sending it as
an RFC series either tomorrow or next week. Thank you for the
suggestion!
^ permalink raw reply
* Re: Concurrent fetch commands
From: Mike Hommey @ 2024-01-04 22:25 UTC (permalink / raw)
To: Junio C Hamano
Cc: Stefan Haller, Taylor Blau, Patrick Steinhardt,
Oswald Buddenhagen, git
In-Reply-To: <xmqq34vc7nl1.fsf@gitster.g>
On Thu, Jan 04, 2024 at 02:14:50PM -0800, Junio C Hamano wrote:
> Mike Hommey <mh@glandium.org> writes:
>
> > On Thu, Jan 04, 2024 at 01:01:26PM +0100, Stefan Haller wrote:
> >> On 03.01.24 23:10, Junio C Hamano wrote:
> >> > Folks who invented "git maintenance" designed their "prefetch" task
> >> > to perform the best practice, without interfering any foreground
> >> > fetches by not touching FETCH_HEAD and the remote-tracking branches.
> >>
> >> That's good, but it's for a very different purpose than an IDE's
> >> background fetch. git maintenance's prefetch is just to improve
> >> performance for the next pull; the point of an IDE's background fetch is
> >> to show me which of my remote branches have new stuff that I might be
> >> interested in pulling, without having to fetch myself. So I *want* this
> >> to be mucking with my remote-tracking branches.
> >
> > Use `git remote update`?
>
> Hmph, it seems that it does not pass "--no-write-fetch-head" so it
> would interfere with the foreground "fetch" or "pull" the end user
> consciously makes, exactly the same way Stefan's demonstration in
> the first message in the thread, no?
Interesting, I never realized it updated FETCH_HEAD, but it does. All
the entries are marked "not-for-merge", though, whatever that implies.
Mike
^ permalink raw reply
* Re: [PATCH v2 00/26] pack-objects: multi-pack verbatim reuse
From: Taylor Blau @ 2024-01-04 22:24 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Patrick Steinhardt
In-Reply-To: <20231221105124.GD570888@coredump.intra.peff.net>
On Thu, Dec 21, 2023 at 05:51:24AM -0500, Jeff King wrote:
> I suspect this is a race in LSan caused by a thread calling exit() while
> other threads are spawning. Here's my theory.
>
> When a thread is spawned, LSan needs to know where its stack is (so it
> can look for points to reachable memory). It calls pthread_getattr_np(),
> which gets an attributes object that must be cleaned up with
> pthread_attr_destroy(). Presumably it does this shortly after. But
> there's a race window where that attr object is allocated and we haven't
> yet set up the new thread's info. If another thread calls exit() then,
> LSan will run but its book-keeping will be in an inconsistent state.
Thanks for digging. I agree with your theory, and am annoyed with how
clever it is ;-).
> So it's a pretty easy fix, though I don't love it in general. Every
> place that spawns multiple threads that can die() would need the same
> treatment. And this isn't a "real" leak in any reasonable sense; it only
> happens because we're exiting the program directly, at which point all
> of the memory is returned to the OS anyway. So I hate changing
> production code to satisfy a leak-checking false positive.
>
> OTOH, dealing with false positives is annoying for humans, and the
> run-time cost should be negligible. We can work around this one, and
> avoid making the same change in other spots unless somebody sees a racy
> failure in practice.
Yeah... I share your thoughts here as well. It's kind of gross that we
have to touch production code purely to appease the leak checker, but I
think that the trade-off is worth it since:
- the false positives are annoying to diagnose (as you said, and as
evidenced by the time that you, Junio, and myself have sunk into
discussing this ;-)).
- the run-time cost is negligible.
So I think that this is a good change to make, and I'm happy to see it
go through. I don't think we should necessarily try too hard to find all
spots that might benefit from a similar change, and instead just apply
the same treatment if/when we notice false positives in CI.
Thanks,
Taylor
^ permalink raw reply
* [PATCH v2 2/2] fetch: add cli option --default-only
From: Tamino Bauknecht @ 2024-01-04 22:22 UTC (permalink / raw)
To: git; +Cc: Tamino Bauknecht
In-Reply-To: <20240104222259.15659-1-dev@tb6.eu>
This option can be used to restore the default behavior of "git fetch"
if the "fetch.all" config option is enabled.
The flag cannot be used in combination with "--all" or explicit
remote(s).
Signed-off-by: Tamino Bauknecht <dev@tb6.eu>
---
A first proposal for the command line option Junio mentioned.
It's called "--default-only" for now, but I don't have a strong opinion
on that matter and am open to suggestions. Alternatives I considered
were "--default-remote" and only "--default".
I'm also not sure about the positioning in code and documentation, is
there some kind of convention about the order? For now, I simply added
it behind "all" since it is related to (although incompatible with) it.
Documentation/config/fetch.txt | 5 ++--
Documentation/fetch-options.txt | 4 ++++
builtin/fetch.c | 21 +++++++++++++----
t/t5514-fetch-multiple.sh | 41 +++++++++++++++++++++++++++++++++
4 files changed, 64 insertions(+), 7 deletions(-)
diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
index 0638cf276e..6c3a9bc3f6 100644
--- a/Documentation/config/fetch.txt
+++ b/Documentation/config/fetch.txt
@@ -52,8 +52,9 @@ fetch.pruneTags::
fetch.all::
If true, fetch will attempt to update all available remotes.
- This behavior can be overridden by explicitly specifying one or
- more remote(s) to fetch from. Defaults to false.
+ This behavior can be overridden by passing `--default-only` or
+ by explicitly specifying one or more remote(s) to fetch from.
+ Defaults to false.
fetch.output::
Control how ref update status is printed. Valid values are
diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index a1d6633a4f..61da5915f1 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -1,6 +1,10 @@
--all::
Fetch all remotes.
+--default-only::
+ Fetch only default remote. This flag can be used to overrule the
+ `fetch.all` configuration option and restore the default behavior.
+
-a::
--append::
Append ref names and object names of fetched refs to the
diff --git a/builtin/fetch.c b/builtin/fetch.c
index f1ad3e608e..de1f659b96 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -2140,6 +2140,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
struct string_list list = STRING_LIST_INIT_DUP;
struct remote *remote = NULL;
int all = 0, multiple = 0;
+ int default_only = 0;
int result = 0;
int prune_tags_ok = 1;
int enable_auto_gc = 1;
@@ -2157,6 +2158,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
OPT__VERBOSITY(&verbosity),
OPT_BOOL(0, "all", &all,
N_("fetch from all remotes")),
+ OPT_BOOL(0, "default-only", &default_only,
+ N_("only fetch default remote")),
OPT_BOOL(0, "set-upstream", &set_upstream,
N_("set upstream for git pull/fetch")),
OPT_BOOL('a', "append", &append,
@@ -2344,15 +2347,23 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
fetch_bundle_uri(the_repository, bundle_uri, NULL))
warning(_("failed to fetch bundles from '%s'"), bundle_uri);
- if (all) {
+ if (all && default_only) {
+ die(_("fetch --all does not work with fetch --default-only"));
+ } else if (all || default_only) {
+ const char *fetch_argument = all ? "--all" : "--default-only";
if (argc == 1)
- die(_("fetch --all does not take a repository argument"));
+ die(_("fetch %s does not take a repository argument"),
+ fetch_argument);
else if (argc > 1)
- die(_("fetch --all does not make sense with refspecs"));
+ die(_("fetch %s does not make sense with refspecs"),
+ fetch_argument);
}
- if (all || (config.all > 0 && !argc)) {
- /* Only use fetch.all config option if no remotes were explicitly given */
+ if (all || (config.all > 0 && !argc && !default_only)) {
+ /*
+ * Only use fetch.all config option if no remotes were
+ * explicitly given and if --default-only was not passed
+ */
(void) for_each_remote(get_one_remote_for_fetch, &list);
/* do not do fetch_multiple() of one */
diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh
index 781c781808..1b23eef32c 100755
--- a/t/t5514-fetch-multiple.sh
+++ b/t/t5514-fetch-multiple.sh
@@ -304,4 +304,45 @@ test_expect_success 'git config fetch.all false (fetch only default remote)' '
)
'
+for fetch_all in true false
+do
+ test_expect_success "git fetch --default-only (fetch only default remote with fetch.all = $fetch_all)" '
+ test_dir="test_default_only_$fetch_all" &&
+ setup_test_clone "$test_dir" &&
+ (
+ cd "$test_dir" &&
+ git config fetch.all $fetch_all &&
+ git fetch --default-only &&
+ cat >expect <<-\EOF &&
+ origin/HEAD -> origin/main
+ origin/main
+ origin/side
+ EOF
+ git branch -r >actual &&
+ test_cmp expect actual
+ )
+ '
+done
+
+test_expect_success 'git fetch --all does not work with --default-only' '
+ (
+ cd test &&
+ test_must_fail git fetch --all --default-only
+ )
+'
+
+test_expect_success 'git fetch --default-only does not accept one explicit remote' '
+ (
+ cd test &&
+ test_must_fail git fetch --default-only one
+ )
+'
+
+test_expect_success 'git fetch --default-only does not accept multiple explicit remotes' '
+ (
+ cd test &&
+ test_must_fail git fetch --default-only one two three
+ )
+'
+
test_done
--
2.43.0
^ permalink raw reply related
* [PATCH v2 1/2] fetch: add new config option fetch.all
From: Tamino Bauknecht @ 2024-01-04 22:22 UTC (permalink / raw)
To: git; +Cc: Tamino Bauknecht
In-Reply-To: <cc74dc58-4fbe-470d-a212-4c2d2249918c@tb6.eu>
This commit introduces the new boolean configuration option fetch.all
which allows to fetch all available remotes by default. The config
option can be overridden by explicitly specifying a remote.
The behavior for --all is unchanged and calling git-fetch with --all and
a remote will still result in an error.
The option was also added to the config documentation and new tests
cover the expected behavior.
Signed-off-by: Tamino Bauknecht <dev@tb6.eu>
---
I fixed the formatting in the test cases and replaced the "cp" with an
explicit "expect".
Documentation/config/fetch.txt | 5 ++
builtin/fetch.c | 11 ++++
t/t5514-fetch-multiple.sh | 95 ++++++++++++++++++++++++++++++++++
3 files changed, 111 insertions(+)
diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
index aea5b97477..0638cf276e 100644
--- a/Documentation/config/fetch.txt
+++ b/Documentation/config/fetch.txt
@@ -50,6 +50,11 @@ fetch.pruneTags::
refs. See also `remote.<name>.pruneTags` and the PRUNING
section of linkgit:git-fetch[1].
+fetch.all::
+ If true, fetch will attempt to update all available remotes.
+ This behavior can be overridden by explicitly specifying one or
+ more remote(s) to fetch from. Defaults to false.
+
fetch.output::
Control how ref update status is printed. Valid values are
`full` and `compact`. Default value is `full`. See the
diff --git a/builtin/fetch.c b/builtin/fetch.c
index a284b970ef..f1ad3e608e 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -102,6 +102,7 @@ static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;
struct fetch_config {
enum display_format display_format;
+ int all;
int prune;
int prune_tags;
int show_forced_updates;
@@ -115,6 +116,11 @@ static int git_fetch_config(const char *k, const char *v,
{
struct fetch_config *fetch_config = cb;
+ if (!strcmp(k, "fetch.all")) {
+ fetch_config->all = git_config_bool(k, v);
+ return 0;
+ }
+
if (!strcmp(k, "fetch.prune")) {
fetch_config->prune = git_config_bool(k, v);
return 0;
@@ -2121,6 +2127,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
{
struct fetch_config config = {
.display_format = DISPLAY_FORMAT_FULL,
+ .all = -1,
.prune = -1,
.prune_tags = -1,
.show_forced_updates = 1,
@@ -2342,6 +2349,10 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
die(_("fetch --all does not take a repository argument"));
else if (argc > 1)
die(_("fetch --all does not make sense with refspecs"));
+ }
+
+ if (all || (config.all > 0 && !argc)) {
+ /* Only use fetch.all config option if no remotes were explicitly given */
(void) for_each_remote(get_one_remote_for_fetch, &list);
/* do not do fetch_multiple() of one */
diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh
index a95841dc36..781c781808 100755
--- a/t/t5514-fetch-multiple.sh
+++ b/t/t5514-fetch-multiple.sh
@@ -24,6 +24,15 @@ setup_repository () {
)
}
+setup_test_clone () {
+ test_dir="$1"
+ git clone one "$test_dir"
+ for r in one two three
+ do
+ git -C "$test_dir" remote add "$r" "../$r" || return 1
+ done
+}
+
test_expect_success setup '
setup_repository one &&
setup_repository two &&
@@ -209,4 +218,90 @@ test_expect_success 'git fetch --multiple --jobs=0 picks a default' '
git fetch --multiple --jobs=0)
'
+for fetch_all in true false
+do
+ test_expect_success "git fetch --all (works with fetch.all = $fetch_all)" '
+ test_dir="test_fetch_all_$fetch_all" &&
+ setup_test_clone "$test_dir" &&
+ (
+ cd "$test_dir" &&
+ git config fetch.all $fetch_all &&
+ git fetch --all &&
+ cat >expect <<-\EOF &&
+ one/main
+ one/side
+ origin/HEAD -> origin/main
+ origin/main
+ origin/side
+ three/another
+ three/main
+ three/side
+ two/another
+ two/main
+ two/side
+ EOF
+ git branch -r >actual &&
+ test_cmp expect actual
+ )
+ '
+done
+
+test_expect_success 'git fetch (fetch all remotes with fetch.all = true)' '
+ setup_test_clone test9 &&
+ (
+ cd test9 &&
+ git config fetch.all true &&
+ git fetch --all &&
+ git branch -r >actual &&
+ cat >expect <<-\EOF &&
+ one/main
+ one/side
+ origin/HEAD -> origin/main
+ origin/main
+ origin/side
+ three/another
+ three/main
+ three/side
+ two/another
+ two/main
+ two/side
+ EOF
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'git fetch one (explicit remote overrides fetch.all)' '
+ setup_test_clone test10 &&
+ (
+ cd test10 &&
+ git config fetch.all true &&
+ git fetch one &&
+ cat >expect <<-\EOF &&
+ one/main
+ one/side
+ origin/HEAD -> origin/main
+ origin/main
+ origin/side
+ EOF
+ git branch -r >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'git config fetch.all false (fetch only default remote)' '
+ setup_test_clone test11 &&
+ (
+ cd test11 &&
+ git config fetch.all false &&
+ git fetch &&
+ cat >expect <<-\EOF &&
+ origin/HEAD -> origin/main
+ origin/main
+ origin/side
+ EOF
+ git branch -r >actual &&
+ test_cmp expect actual
+ )
+'
+
test_done
--
2.43.0
^ permalink raw reply related
* Re: [PATCH 00/24] pack-objects: multi-pack verbatim reuse
From: Taylor Blau @ 2024-01-04 22:22 UTC (permalink / raw)
To: Jeff King; +Cc: git, Patrick Steinhardt, Junio C Hamano
In-Reply-To: <20231221111333.GE570888@coredump.intra.peff.net>
On Thu, Dec 21, 2023 at 06:13:33AM -0500, Jeff King wrote:
> But that's not quite the whole story. There is still a CPU improvement
> in your series (1.2s vs 1.0s, a 20% speedup). And as I'd expect, a
> memory improvement from avoiding the extra book-keeping (almost 10%):
>
> > Benchmark 1: single-pack reuse, pack.window=0
> > 354.224 MB (max RSS)
> > Benchmark 4: multi-pack reuse, pack.window=10
> > 328.786 MB (max RSS)
I agree. And I expect that we'd see larger savings on larger, real-world
repositories (the numbers here are generated from a semi out-of-date
copy of git.git).
> So while it's a lot less code to just set the window size, I do think
> those improvements are worth it. And really, it's the same tradeoff we
> make for the single-pack case (i.e., one could argue that we
> could/should rip out the verbatim-reuse code entirely in favor of just
> tweaking the window size).
Definitely an interesting direction. One question that comes to mind is
whether or not we'd want to keep the "verbatim" reuse code around to
avoid adding objects to the packing list directly. That's a
non-negligible memory savings when generating large packs where we can
reuse large swaths of existing packs.
That seems like a topic for another day, though ;-). Not quite
left-over-bits, so maybe... #leftoverboulders?
Thanks,
Taylor
^ permalink raw reply
* Re: Concurrent fetch commands
From: Junio C Hamano @ 2024-01-04 22:14 UTC (permalink / raw)
To: Mike Hommey
Cc: Stefan Haller, Taylor Blau, Patrick Steinhardt,
Oswald Buddenhagen, git
In-Reply-To: <20240104205408.h5wvcissfbat7acw@glandium.org>
Mike Hommey <mh@glandium.org> writes:
> On Thu, Jan 04, 2024 at 01:01:26PM +0100, Stefan Haller wrote:
>> On 03.01.24 23:10, Junio C Hamano wrote:
>> > Folks who invented "git maintenance" designed their "prefetch" task
>> > to perform the best practice, without interfering any foreground
>> > fetches by not touching FETCH_HEAD and the remote-tracking branches.
>>
>> That's good, but it's for a very different purpose than an IDE's
>> background fetch. git maintenance's prefetch is just to improve
>> performance for the next pull; the point of an IDE's background fetch is
>> to show me which of my remote branches have new stuff that I might be
>> interested in pulling, without having to fetch myself. So I *want* this
>> to be mucking with my remote-tracking branches.
>
> Use `git remote update`?
Hmph, it seems that it does not pass "--no-write-fetch-head" so it
would interfere with the foreground "fetch" or "pull" the end user
consciously makes, exactly the same way Stefan's demonstration in
the first message in the thread, no?
^ permalink raw reply
* Re: Concurrent fetch commands
From: Mike Hommey @ 2024-01-04 20:54 UTC (permalink / raw)
To: Stefan Haller
Cc: Junio C Hamano, Taylor Blau, Patrick Steinhardt,
Oswald Buddenhagen, git
In-Reply-To: <80efcb43-122c-421a-b763-6da6ff620538@haller-berlin.de>
On Thu, Jan 04, 2024 at 01:01:26PM +0100, Stefan Haller wrote:
> On 03.01.24 23:10, Junio C Hamano wrote:
> > Folks who invented "git maintenance" designed their "prefetch" task
> > to perform the best practice, without interfering any foreground
> > fetches by not touching FETCH_HEAD and the remote-tracking branches.
>
> That's good, but it's for a very different purpose than an IDE's
> background fetch. git maintenance's prefetch is just to improve
> performance for the next pull; the point of an IDE's background fetch is
> to show me which of my remote branches have new stuff that I might be
> interested in pulling, without having to fetch myself. So I *want* this
> to be mucking with my remote-tracking branches.
Use `git remote update`?
Mike
^ permalink raw reply
* Re: [RFC PATCH] grep: default to posix digits with -P
From: René Scharfe @ 2024-01-04 21:14 UTC (permalink / raw)
To: Carlo Arenas; +Cc: git
In-Reply-To: <CAPUEspgbVx6wbp4UNjjxFO8iNJw3i2FnJxNwn4pk5EbaAP-7gQ@mail.gmail.com>
Am 02.01.24 um 20:02 schrieb Carlo Arenas:
> On Mon, Jan 1, 2024 at 9:18 AM René Scharfe <l.s.r@web.de> wrote:
>>
>> Am 01.01.24 um 16:03 schrieb Carlo Marcelo Arenas Belón:
>>> @@ -321,17 +327,22 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
>>> }
>>> options |= PCRE2_CASELESS;
>>> }
>>> - if (!opt->ignore_locale && is_utf8_locale() && !literal)
>>> - options |= (PCRE2_UTF | PCRE2_UCP | PCRE2_MATCH_INVALID_UTF);
>>> + if (!opt->ignore_locale && is_utf8_locale() && !literal) {
>>> + options |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);
>>>
>>> -#ifndef GIT_PCRE2_VERSION_10_35_OR_HIGHER
>>> - /*
>>> - * Work around a JIT bug related to invalid Unicode character handling
>>> - * fixed in 10.35:
>>> - * https://github.com/PCRE2Project/pcre2/commit/c21bd977547d
>>> - */
>>> - options &= ~PCRE2_UCP;
>>> +#ifdef GIT_PCRE2_VERSION_10_35_OR_HIGHER
>>> + /*
>>> + * Work around a JIT bug related to invalid Unicode character handling
>>> + * fixed in 10.35:
>>> + * https://github.com/PCRE2Project/pcre2/commit/c21bd977547d
>>> + */
>>> + options |= PCRE2_UCP;
>>> +#ifdef GIT_PCRE2_VERSION_10_43_OR_HIGHER
>>> + if (!opt->perl_digit)
>>> + xoptions |= (PCRE2_EXTRA_ASCII_BSD | PCRE2_EXTRA_ASCII_DIGIT);
>>> #endif
>>> +#endif
>>
>> Why do we need that extra level of indentation?
>
> I was just trying to simplify the code by including all the logic in
> one single set.
>
> The original lack of indentation that was introduced by later fixes to
> the code was IMHO also misguided since the obvious "objective" as set
> in the original code that added PCRE2_UCP was that it should be used
> whenever UTF was also in use as shown by
> acabd2048ee0ee53728100408970ab45a6dab65e.
One level of indentation is correct because the code in question is part
of a function and it's not nested in a loop or conditional. And
preprocessor directives don't affect the indentation of C code. Am I
missing something?
> Of course, we soon found out that the original implementation of
> PCRE2_MATCH_INVALID_UTF that came with PCRE2 10.34 was buggy and so an
> exception was introduced in 14b9a044798ebb3858a1f1a1377309a3d6054ac8.
>
> Note that the problematic code is only relevant when JIT is also
> enabled, but JIT is almost always enabled.
>
>> The old code turned PCRE2_UCP on by default and turned it off for older
>> versions. The new code enables PCRE2_UCP only for newer versions. The
>> result should be the same, no? So why change that part at all?
>
> Because it gets us a little closer to the real reason why we need to
> disable UCP for anything older than 10.35, and that is that there is a
> bug there that is ONLY relevant if we are using JIT.
How so? If the same flags are set in the end then it seems like a
lateral movement to me.
Do you want to disable JIT compilation for older versions?
> My hope though is that with the release of 10.43 (currently in RC1),
> 10.34 will become irrelevant soon enough and this whole code could be
> cleaned up further.
Cleanup is good, but if we package the workarounds nicely we can keep
them around indefinitely without them bothering us. Debian buster
still has a few months of support left and comes with PCRE2 10.32..
>> But the comment is now off, isn't it? The workaround was turning
>> PCRE2_UCP off for older versions (because those were broken), not
>> turning it on for newer versions (because it would be required by some
>> unfixed regression).
>
> The comment was never correct, because it was turning it off, because
> the combination of JIT + MATCH_INVALID_UTF (which was released in
> 10.34) + UCP is broken.
The code disabled PCRE2_UCP for PCRE2 10.34 and older. The comment
claimed that this was done as a workaround for a bug fixed in 10.35.
You seem to say the same. What was incorrect about the comment?
>> Do we need to nest the checks for GIT_PCRE2_VERSION_10_35_OR_HIGHER and
>> GIT_PCRE2_VERSION_10_43_OR_HIGHER? Keeping them separate would help
>> keep the #ifdef parts as short as possible and maintainable
>> independently.
>
> I thought that nesting them would make it simpler to maintain, since
> there are somehow connected anyway.
>
> The additional flags that are added in PCRE 10.43 are ONLY needed and
> useful on top of PCRE2_UCP, which itself only makes sense on top of
> UTF.
This conditional stacking is complicated. I find the old model where
we say which features we want up front and then disable buggy ones
for specific versions easier to grasp.
>>> @@ -27,13 +34,13 @@ do
>>> if ! test_have_prereq PERF_GREP_ENGINES_THREADS
>>> then
>>> test_perf "grep -P '$pattern'" --prereq PCRE "
>>> - git -P grep -f pat || :
>>> + git grep -P -f pat || :
>>> "
>>> else
>>> for threads in $GIT_PERF_GREP_THREADS
>>> do
>>> test_perf "grep -P '$pattern' with $threads threads" --prereq PTHREADS,PCRE "
>>> - git -c grep.threads=$threads -P grep -f pat || :
>>> + git -c grep.threads=$threads grep -P -f pat || :
>>
>> What's going on here? You remove the -P option of git (--no-pager) and
>> add the -P option of git grep (--perl-regexp). So this perf test never
>> tested PCRE despite its name? Seems worth a separate patch.
>
> My original code was a dud. This would make that at least more obvious,
The change is good, but I don't see any connection to the
grep.perl.digit feature that this patch is primarily about,
so I (still) think it deserves its own patch.
>> Do the performance numbers in acabd2048e (grep: correctly identify utf-8
>> characters with \{b,w} in -P, 2023-01-08) still hold up in that light?
>
> FWIW the performance numbers still hold up, but just because I did the
> tests independently using hyperfine, and indeed when I did the first
> version of this patch, I had a nice reproducible description that
> showed how to get 20% better performance while searching the git
> repository itself for something like 4 digits (as used in Copyright
> dates).
Great!
> My idea was to reply to my own post with instructions on how to test
> this, which basically require to also compile the recently released
> 10.43RC1 and build against that.
OK, so this is about the grep.perl.digit feature, right?
What I meant above was: Is the statement in acabd2048e (grep: correctly
identify utf-8 characters with \{b,w} in -P, 2023-01-08) about 20-40%
performance impact (in which direction, by the way) still measurable
with the fixed perf test script?
With the fix and PCRE2 10.42 and PCRE2_UCP I get:
Test this tree
----------------------------------------------
7822.1: grep -P '\bhow' 0.05(0.02+0.30)
7822.2: grep -P '\bÆvar' 0.05(0.02+0.29)
7822.3: grep -P '\d+ \bÆvar' 0.05(0.02+0.27)
7822.4: grep -P '\bBelón\b' 0.04(0.02+0.23)
7822.5: grep -P '\w{12}\b' 0.03(0.02+0.13)
... and without PCRE2_UCP:
Test this tree
----------------------------------------------
7822.1: grep -P '\bhow' 0.05(0.02+0.26)
7822.2: grep -P '\bÆvar' 0.04(0.02+0.18)
7822.3: grep -P '\d+ \bÆvar' 0.05(0.02+0.26)
7822.4: grep -P '\bBelón\b' 0.05(0.02+0.27)
7822.5: grep -P '\w{12}\b' 0.04(0.02+0.18)
Hmm, inconclusive. Perhaps the test data is too small?
> Indeed, AFAIK the test would fail if built with an older version of
> PCRE, but wasn't sure if making a prerequisite that hardcoded the
> version in test-tool might be the best approach to prevent that,
> especially with all the libification work.
You could extend test-pcre2-config to report whether that feature is
active. Performance tests could set prerequisites based on its output.
> PS. your reply got lost somehow in my SPAM folder, so I apologize for
> the late reply
No worries, I need days to reply without any detour through the
spam folder..
René
^ permalink raw reply
* Re: [PATCH] fetch: add new config option fetch.all
From: Eric Sunshine @ 2024-01-04 20:55 UTC (permalink / raw)
To: Tamino Bauknecht; +Cc: git
In-Reply-To: <20240104202605.7382-1-dev@tb6.eu>
On Thu, Jan 4, 2024 at 3:26 PM Tamino Bauknecht <dev@tb6.eu> wrote:
> diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh
> @@ -24,6 +24,15 @@ setup_repository () {
> +test_expect_success 'git fetch (fetch all remotes with fetch.all = true)' '
> + setup_test_clone test9 && (
> + cd test9 &&
> + git config fetch.all true &&
> + git fetch --all &&
> + git branch -r >actual &&
> + cp ../test_fetch_all_true/expect . &&
> + test_cmp expect actual)
> +'
I forgot to mention that the formatting and subsequent indentation of
the subshell is a bit unusual and out of line with project style. We
normally format subshells like this:
setup_test_clone test9 &&
(
cd test9 &&
...
)
where the code outside the shell is indented by one TAB, and the code
inside the subshell indented by two TABs.
That differs from what is in your patch, in which the code inside the
subshell is indented by a TAB followed by a space.
^ permalink raw reply
* Re: [PATCH] fetch: add new config option fetch.all
From: Eric Sunshine @ 2024-01-04 20:50 UTC (permalink / raw)
To: Tamino Bauknecht; +Cc: git
In-Reply-To: <20240104202605.7382-1-dev@tb6.eu>
On Thu, Jan 4, 2024 at 3:26 PM Tamino Bauknecht <dev@tb6.eu> wrote:
> I'm still not entirely happy with the tests (especially the `cp` in
> there)
Easily enough solved; see below...
> and the heredoc doesn't seem to respect the one additional
> space of its indentation - I am admittedly not the best POSIX shell
> developer, if anyone has an idea on how to improve it, your suggestion
> is welcome.
Not sure what you mean by the heredoc not "respecting one additional
space of indentation".
The <<- operator strips leading TABs from the body of the heredoc but
leaves other leading whitespace alone. In your tests, you have one or
more TABs followed by two spaces, followed by the remaining (actual)
text. So, with the leading TABs stripped off by the <<- operator,
you're left with the two spaces and the remaining text, which is
exactly what you want.
> diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh
> @@ -209,4 +218,70 @@ test_expect_success 'git fetch --multiple --jobs=0 picks a default' '
> +for fetch_all in true false
> +do
> + test_expect_success "git fetch --all (works with fetch.all = $fetch_all)" '
> + test_dir="test_fetch_all_$fetch_all" &&
> + setup_test_clone "$test_dir" && (
> + cd "$test_dir" &&
> + git config fetch.all $fetch_all &&
> + git fetch --all &&
> + cat >expect <<-\ EOF &&
> + one/main
> + one/side
> + origin/HEAD -> origin/main
> + origin/main
> + origin/side
> + three/another
> + three/main
> + three/side
> + two/another
> + two/main
> + two/side
> + EOF
> + git branch -r >actual &&
> + test_cmp expect actual)
> + '
> +done
> +
> +test_expect_success 'git fetch (fetch all remotes with fetch.all = true)' '
> + setup_test_clone test9 && (
> + cd test9 &&
> + git config fetch.all true &&
> + git fetch --all &&
> + git branch -r >actual &&
> + cp ../test_fetch_all_true/expect . &&
> + test_cmp expect actual)
> +'
Ideally, this test should create the "expect" file itself, even if the
"expect" file happens to exactly match the "expect" file from the
preceding test. Doing so will make the test (more) self-contained,
which makes it possible to run the test in isolation without having to
run all tests preceding it (see the --run option in t/README).
^ permalink raw reply
* Re: [PATCH] fetch: add new config option fetch.all
From: Junio C Hamano @ 2024-01-04 20:49 UTC (permalink / raw)
To: Tamino Bauknecht; +Cc: git
In-Reply-To: <70f17f0c-2226-41eb-ae08-348c794a3411@tb6.eu>
Tamino Bauknecht <dev@tb6.eu> writes:
> Do you think it is worth adding a flag for it?
Otherwise I wouldn't have pointed it out ;-). It probably has about
the same value as the fetch.all configuration variable has, meaning
that we either have both or neither.
Thanks.
^ permalink raw reply
* [PATCH] fetch: add new config option fetch.all
From: Tamino Bauknecht @ 2024-01-04 20:25 UTC (permalink / raw)
To: git; +Cc: Tamino Bauknecht
In-Reply-To: <cc74dc58-4fbe-470d-a212-4c2d2249918c@tb6.eu>
This commit introduces the new boolean configuration option fetch.all
which allows to fetch all available remotes by default. The config
option can be overridden by explicitly specifying a remote.
The behavior for --all is unchanged and calling git-fetch with --all and
a remote will still result in an error.
The option was also added to the config documentation and new tests
cover the expected behavior.
Signed-off-by: Tamino Bauknecht <dev@tb6.eu>
---
I hopefully incorporated the feedback of all of you, thanks for the
valuable suggestions.
I'm still not entirely happy with the tests (especially the `cp` in
there) and the heredoc doesn't seem to respect the one additional
space of its indentation - I am admittedly not the best POSIX shell
developer, if anyone has an idea on how to improve it, your suggestion
is welcome.
Documentation/config/fetch.txt | 5 +++
builtin/fetch.c | 11 +++++
t/t5514-fetch-multiple.sh | 75 ++++++++++++++++++++++++++++++++++
3 files changed, 91 insertions(+)
diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
index aea5b97477..0638cf276e 100644
--- a/Documentation/config/fetch.txt
+++ b/Documentation/config/fetch.txt
@@ -50,6 +50,11 @@ fetch.pruneTags::
refs. See also `remote.<name>.pruneTags` and the PRUNING
section of linkgit:git-fetch[1].
+fetch.all::
+ If true, fetch will attempt to update all available remotes.
+ This behavior can be overridden by explicitly specifying one or
+ more remote(s) to fetch from. Defaults to false.
+
fetch.output::
Control how ref update status is printed. Valid values are
`full` and `compact`. Default value is `full`. See the
diff --git a/builtin/fetch.c b/builtin/fetch.c
index a284b970ef..f1ad3e608e 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -102,6 +102,7 @@ static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;
struct fetch_config {
enum display_format display_format;
+ int all;
int prune;
int prune_tags;
int show_forced_updates;
@@ -115,6 +116,11 @@ static int git_fetch_config(const char *k, const char *v,
{
struct fetch_config *fetch_config = cb;
+ if (!strcmp(k, "fetch.all")) {
+ fetch_config->all = git_config_bool(k, v);
+ return 0;
+ }
+
if (!strcmp(k, "fetch.prune")) {
fetch_config->prune = git_config_bool(k, v);
return 0;
@@ -2121,6 +2127,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
{
struct fetch_config config = {
.display_format = DISPLAY_FORMAT_FULL,
+ .all = -1,
.prune = -1,
.prune_tags = -1,
.show_forced_updates = 1,
@@ -2342,6 +2349,10 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
die(_("fetch --all does not take a repository argument"));
else if (argc > 1)
die(_("fetch --all does not make sense with refspecs"));
+ }
+
+ if (all || (config.all > 0 && !argc)) {
+ /* Only use fetch.all config option if no remotes were explicitly given */
(void) for_each_remote(get_one_remote_for_fetch, &list);
/* do not do fetch_multiple() of one */
diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh
index a95841dc36..806b87c727 100755
--- a/t/t5514-fetch-multiple.sh
+++ b/t/t5514-fetch-multiple.sh
@@ -24,6 +24,15 @@ setup_repository () {
)
}
+setup_test_clone () {
+ test_dir="$1"
+ git clone one "$test_dir"
+ for r in one two three
+ do
+ git -C "$test_dir" remote add "$r" "../$r" || return 1
+ done
+}
+
test_expect_success setup '
setup_repository one &&
setup_repository two &&
@@ -209,4 +218,70 @@ test_expect_success 'git fetch --multiple --jobs=0 picks a default' '
git fetch --multiple --jobs=0)
'
+for fetch_all in true false
+do
+ test_expect_success "git fetch --all (works with fetch.all = $fetch_all)" '
+ test_dir="test_fetch_all_$fetch_all" &&
+ setup_test_clone "$test_dir" && (
+ cd "$test_dir" &&
+ git config fetch.all $fetch_all &&
+ git fetch --all &&
+ cat >expect <<-\ EOF &&
+ one/main
+ one/side
+ origin/HEAD -> origin/main
+ origin/main
+ origin/side
+ three/another
+ three/main
+ three/side
+ two/another
+ two/main
+ two/side
+ EOF
+ git branch -r >actual &&
+ test_cmp expect actual)
+ '
+done
+
+test_expect_success 'git fetch (fetch all remotes with fetch.all = true)' '
+ setup_test_clone test9 && (
+ cd test9 &&
+ git config fetch.all true &&
+ git fetch --all &&
+ git branch -r >actual &&
+ cp ../test_fetch_all_true/expect . &&
+ test_cmp expect actual)
+'
+
+test_expect_success 'git fetch one (explicit remote overrides fetch.all)' '
+ setup_test_clone test10 && (
+ cd test10 &&
+ git config fetch.all true &&
+ git fetch one &&
+ cat >expect <<-\ EOF &&
+ one/main
+ one/side
+ origin/HEAD -> origin/main
+ origin/main
+ origin/side
+ EOF
+ git branch -r >actual &&
+ test_cmp expect actual)
+'
+
+test_expect_success 'git config fetch.all false (fetch only default remote)' '
+ setup_test_clone test11 && (
+ cd test11 &&
+ git config fetch.all false &&
+ git fetch &&
+ cat >expect <<-\ EOF &&
+ origin/HEAD -> origin/main
+ origin/main
+ origin/side
+ EOF
+ git branch -r >actual &&
+ test_cmp expect actual)
+'
+
test_done
--
2.43.0
^ permalink raw reply related
* Re: [PATCH] fetch: add new config option fetch.all
From: Tamino Bauknecht @ 2024-01-04 20:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqv8896jqo.fsf@gitster.g>
On 1/4/24 19:23, Junio C Hamano wrote:> This sounds like a usability
annoyance for forks that use --all some
> of the time and not always, as they now have to remember not just to
> pass something to countermand the configured fetch.all but what that
> something exactly is (i.e., the name of the remote they fetch from
> by default), which makes fetch.all less appealing. I wonder if we
> can instead take "--no-all" from the command line to make configured
> fetch.all ignored (hence, giving an explicit remote will fetch from
> there, and not giving any remote will fetch from whereever the
> command will fetch from with "git -c fetch.all=false fetch")?
I don't think I fully understand the scenario you describe here.
But I see that the change would disallow users to fetch only the default
remote without its name in a straight-forward way - either your proposed
solution to overrule the config value or using something like
`git config "branch.$(git branch --show-current).remote"` in combination
with `git fetch` would be workarounds.
Do you think it is worth adding a flag for it? I can't really think of a
real-world use case for it. E.g. the config option "fetch.prune" also
doesn't have anything to counteract it (as far as I see).
If a flag is necessary, I think something like "--default-only" (or
similar) might be more descriptive than "--no-all".
> Missing sign-off.
Sorry, thanks for pointing it out.
> And we should say that this configuration variable defaults to false.
Will do so.
> This conditional cascade will probably need to change when we allow
> "--no-all" to countermand the configured fetch.all anyway, so I
> won't worry about it now, but it looks somewhat convoluted that we
> have to re-check "all" many times, which was the point of the
> original that implemented this as a nested conditional.
It's probably because of the tab width of 8 that I feel like three
indentation levels are already too much. I'll use Taylor's suggestion to
keep the `argc` check as-is (although two checks will still be necessary).
As an alternative I thought about modifying the current behavior of
"--all" in combination with an explicit remote as well, but discarded
that idea because it might be less intuitive than the error.
^ permalink raw reply
* Re: [PATCH v5 1/1] Adds domain/username to error message
From: Junio C Hamano @ 2024-01-04 20:09 UTC (permalink / raw)
To: Sören Krecker, Johannes Schindelin; +Cc: git, sunshine
In-Reply-To: <20240104192202.2124-2-soekkle@freenet.de>
Sören Krecker <soekkle@freenet.de> writes:
> Subject: Re: [PATCH v5 1/1] Adds domain/username to error message
Looking at past commits that worked on the area this patch touches,
namely, 7c83470e (mingw: be more informative when ownership check
fails on FAT32, 2022-08-08) and e883e04b (mingw: provide details
about unsafe directories' ownership, 2022-08-08), I would retitle
the commit perhaps like so:
Subject: [PATCH v5] mingw: give more details about unsafe directory's ownership
if I were doing this patch.
> Adds domain/username in error message, if owner sid of repository and
"Adds" -> "Add".
> user sid are not equal on windows systems.
>
> Old Prompted error message:
> '''
> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
> 'C:/Users/test/source/repos/git' is owned by:
> 'S-1-5-21-571067702-4104414259-3379520149-500'
> but the current user is:
> 'S-1-5-21-571067702-4104414259-3379520149-1001'
> To add an exception for this directory, call:
>
> git config --global --add safe.directory C:/Users/test/source/repos/git
> '''
>
> New prompted error massage:
"massage" -> "message".
I probably would drop two "prompted" from the above, too, if I were
doing this patch.
Thanks for working on making this error message more readable. I'll
queue it when I see an Ack from Dscho.
> '''
> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
> 'C:/Users/test/source/repos/git' is owned by:
> 'DESKTOP-L78JVA6/Administrator' (S-1-5-21-571067702-4104414259-3379520149-500)
> but the current user is:
> 'DESKTOP-L78JVA6/test' (S-1-5-21-571067702-4104414259-3379520149-1001)
> To add an exception for this directory, call:
>
> git config --global --add safe.directory C:/Users/test/source/repos/git
> '''
>
> Signed-off-by: Sören Krecker <soekkle@freenet.de>
> ---
> compat/mingw.c | 64 ++++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 51 insertions(+), 13 deletions(-)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 42053c1f65..6240387205 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -2684,6 +2684,26 @@ static PSID get_current_user_sid(void)
> return result;
> }
>
> +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
> +{
> + SID_NAME_USE pe_use;
> + DWORD len_user = 0, len_domain = 0;
> + BOOL translate_sid_to_user;
> +
> + /* returns only FALSE, because the string pointers are NULL*/
> + LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
> + &pe_use);
> + /*Alloc needed space of the strings*/
> + ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user);
> + translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,
> + *str, &len_domain, &pe_use);
> + if (translate_sid_to_user == FALSE)
> + FREE_AND_NULL(*str);
> + else
> + (*str)[len_domain] = '/';
> + return translate_sid_to_user;
> +}
> +
> static int acls_supported(const char *path)
> {
> size_t offset = offset_1st_component(path);
> @@ -2765,27 +2785,45 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
> strbuf_addf(report, "'%s' is on a file system that does "
> "not record ownership\n", path);
> } else if (report) {
> - LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
> + LPSTR str1, str2, str3, str4, to_free1 = NULL, to_free3 = NULL, to_local_free2=NULL, to_local_free4=NULL;
>
> - if (ConvertSidToStringSidA(sid, &str1))
> + if (user_sid_to_user_name(sid, &str1))
> to_free1 = str1;
> else
> str1 = "(inconvertible)";
> -
> - if (!current_user_sid)
> - str2 = "(none)";
> - else if (!IsValidSid(current_user_sid))
> - str2 = "(invalid)";
> - else if (ConvertSidToStringSidA(current_user_sid, &str2))
> - to_free2 = str2;
> + if (ConvertSidToStringSidA(sid, &str2))
> + to_local_free2 = str2;
> else
> str2 = "(inconvertible)";
> +
> + if (!current_user_sid) {
> + str3 = "(none)";
> + str4 = "(none)";
> + }
> + else if (!IsValidSid(current_user_sid)) {
> + str3 = "(invalid)";
> + str4 = "(invalid)";
> + } else {
> + if (user_sid_to_user_name(current_user_sid,
> + &str3))
> + to_free3 = str3;
> + else
> + str3 = "(inconvertible)";
> + if (ConvertSidToStringSidA(current_user_sid,
> + &str4))
> + to_local_free4 = str4;
> + else
> + str4 = "(inconvertible)";
> + }
> strbuf_addf(report,
> "'%s' is owned by:\n"
> - "\t'%s'\nbut the current user is:\n"
> - "\t'%s'\n", path, str1, str2);
> - LocalFree(to_free1);
> - LocalFree(to_free2);
> + "\t'%s' (%s)\nbut the current user is:\n"
> + "\t'%s' (%s)\n",
> + path, str1, str2, str3, str4);
> + free(to_free1);
> + LocalFree(to_local_free2);
> + free(to_free3);
> + LocalFree(to_local_free4);
> }
> }
^ permalink raw reply
* Re: Does extending `--empty` to git-cherry-pick make sense?
From: Taylor Blau @ 2024-01-04 19:33 UTC (permalink / raw)
To: Brian Lyles; +Cc: Elijah Newren, git
In-Reply-To: <CAHPHrSevBdQF0BisR8VK=jM=wj1dTUYEVrv31gLerAzL9=Cd8Q@mail.gmail.com>
[+cc Elijah]
On Thu, Jan 04, 2024 at 12:57:18AM -0600, Brian Lyles wrote:
> Is there any real barrier to exposing that option to git-cherry-pick as
> well? Was this an oversight, or intentionally left out? The
> corresponding commit message doesn't seem to indicate any specific
> reason for limiting it to git-rebase.
I am not nearly as familiar with this code as Elijah is, but this
certainly appears possible by setting the `drop_redundant_commits` and
`keep_redundant_commits` flags in the replay_opts struct.
I don't see any fundamental reason why cherry-pick shouldn't have the
same functionality.
Thanks,
Taylor
^ permalink raw reply
* [PATCH v5 1/1] Adds domain/username to error message
From: Sören Krecker @ 2024-01-04 19:22 UTC (permalink / raw)
To: git; +Cc: sunshine, Sören Krecker
In-Reply-To: <20240104192202.2124-1-soekkle@freenet.de>
Adds domain/username in error message, if owner sid of repository and
user sid are not equal on windows systems.
Old Prompted error message:
'''
fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
'C:/Users/test/source/repos/git' is owned by:
'S-1-5-21-571067702-4104414259-3379520149-500'
but the current user is:
'S-1-5-21-571067702-4104414259-3379520149-1001'
To add an exception for this directory, call:
git config --global --add safe.directory C:/Users/test/source/repos/git
'''
New prompted error massage:
'''
fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
'C:/Users/test/source/repos/git' is owned by:
'DESKTOP-L78JVA6/Administrator' (S-1-5-21-571067702-4104414259-3379520149-500)
but the current user is:
'DESKTOP-L78JVA6/test' (S-1-5-21-571067702-4104414259-3379520149-1001)
To add an exception for this directory, call:
git config --global --add safe.directory C:/Users/test/source/repos/git
'''
Signed-off-by: Sören Krecker <soekkle@freenet.de>
---
compat/mingw.c | 64 ++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 51 insertions(+), 13 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index 42053c1f65..6240387205 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2684,6 +2684,26 @@ static PSID get_current_user_sid(void)
return result;
}
+static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
+{
+ SID_NAME_USE pe_use;
+ DWORD len_user = 0, len_domain = 0;
+ BOOL translate_sid_to_user;
+
+ /* returns only FALSE, because the string pointers are NULL*/
+ LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
+ &pe_use);
+ /*Alloc needed space of the strings*/
+ ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user);
+ translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,
+ *str, &len_domain, &pe_use);
+ if (translate_sid_to_user == FALSE)
+ FREE_AND_NULL(*str);
+ else
+ (*str)[len_domain] = '/';
+ return translate_sid_to_user;
+}
+
static int acls_supported(const char *path)
{
size_t offset = offset_1st_component(path);
@@ -2765,27 +2785,45 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
strbuf_addf(report, "'%s' is on a file system that does "
"not record ownership\n", path);
} else if (report) {
- LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
+ LPSTR str1, str2, str3, str4, to_free1 = NULL, to_free3 = NULL, to_local_free2=NULL, to_local_free4=NULL;
- if (ConvertSidToStringSidA(sid, &str1))
+ if (user_sid_to_user_name(sid, &str1))
to_free1 = str1;
else
str1 = "(inconvertible)";
-
- if (!current_user_sid)
- str2 = "(none)";
- else if (!IsValidSid(current_user_sid))
- str2 = "(invalid)";
- else if (ConvertSidToStringSidA(current_user_sid, &str2))
- to_free2 = str2;
+ if (ConvertSidToStringSidA(sid, &str2))
+ to_local_free2 = str2;
else
str2 = "(inconvertible)";
+
+ if (!current_user_sid) {
+ str3 = "(none)";
+ str4 = "(none)";
+ }
+ else if (!IsValidSid(current_user_sid)) {
+ str3 = "(invalid)";
+ str4 = "(invalid)";
+ } else {
+ if (user_sid_to_user_name(current_user_sid,
+ &str3))
+ to_free3 = str3;
+ else
+ str3 = "(inconvertible)";
+ if (ConvertSidToStringSidA(current_user_sid,
+ &str4))
+ to_local_free4 = str4;
+ else
+ str4 = "(inconvertible)";
+ }
strbuf_addf(report,
"'%s' is owned by:\n"
- "\t'%s'\nbut the current user is:\n"
- "\t'%s'\n", path, str1, str2);
- LocalFree(to_free1);
- LocalFree(to_free2);
+ "\t'%s' (%s)\nbut the current user is:\n"
+ "\t'%s' (%s)\n",
+ path, str1, str2, str3, str4);
+ free(to_free1);
+ LocalFree(to_local_free2);
+ free(to_free3);
+ LocalFree(to_local_free4);
}
}
--
2.39.2
^ permalink raw reply related
* [PATCH v5 0/1] Replace SID with domain/username on Windows
From: Sören Krecker @ 2024-01-04 19:22 UTC (permalink / raw)
To: git; +Cc: sunshine, Sören Krecker
In-Reply-To: <DB9P250MB0692C8B4D93ED92FEE680AA9A560A@DB9P250MB0692.EURP250.PROD.OUTLOOK.COM>
Hi everyone,
I change the error message. I Hope that it is now better for every one.
Thanks
Sören Krecker (1):
Adds domain/username to error message
compat/mingw.c | 64 ++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 51 insertions(+), 13 deletions(-)
base-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa
--
2.39.2
^ permalink raw reply
* Re: [PATCH] rebase: clarify --reschedule-failed-exec default
From: Taylor Blau @ 2024-01-04 19:20 UTC (permalink / raw)
To: Illia Bobyr; +Cc: git, Ævar Arnfjörð Bjarmason
In-Reply-To: <20240104080631.3666413-1-illia.bobyr@gmail.com>
On Thu, Jan 04, 2024 at 12:06:31AM -0800, Illia Bobyr wrote:
> Documentation should mention the default behavior.
>
> It is better to explain the persistent nature of the
> --reschedule-failed-exec flag from the user standpoint, rather than from
> the implementation standpoint.
The first paragraph looks good, and I think your wording is an
improvement over what's already there (though of course this is
subjective, and YMMV).
> +Recording this option for the whole rebase is a convenience feature. Otherwise
> +an explicit `--no-reschedule-failed-exec` at the start would be overridden by
> +the presence of a `rebase.rescheduleFailedExec=true` configuration when `git
> +rebase --continue` is invoked. Currently, you can not, pass
> +`--[no-]reschedule-failed-exec` to `git rebase --continue`.
The last sentence was a bit confusing to me. I assume you meant
Currently, you cannot pass `--[no-]reschedule-failed-exec` [...]
without the comma between "pass" and "`--[no]reschedule-failed-exect`",
and replacing "can not" with "cannot".
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] fetch: add new config option fetch.all
From: Taylor Blau @ 2024-01-04 19:13 UTC (permalink / raw)
To: Tamino Bauknecht; +Cc: git
In-Reply-To: <88407aeb-bff7-4899-af7b-e7cb671d991a@tb6.eu>
On Thu, Jan 04, 2024 at 07:32:03PM +0100, Tamino Bauknecht wrote:
> > I don't feel particularly strongly about whether or not you reorganize
> > these if-statements, but we should change "argc == 0" to "!argc", which
> > matches the conventions of the rest of the project.
>
> Are you sure that I shouldn't use "argc == 0" here instead since it's also
> used in the next "else if" condition? Or is the goal to gradually transition
> to "!argc" in the entire code base?
> I agree with keeping the diff minimal, will change that to your suggestion.
See Documentation/CodingGuidelines.txt for more information about the
Git project's style guidelines, but in short: yes, any x == 0 should be
replaced with !x instead within if-statements.
> > This should be `cat >expect <<-\EOF` (without the space between the
> > redirect and heredoc, as well as indicating that the heredoc does not
> > need any shell expansions).
>
> Will do so, thanks.
> I tried to match the existing test cases as closely as possible, but if they
> are outdated, it might be better to use the more recent structure.
Junio notes in the thread further up that it is OK to imitate the
existing style of tests. I don't disagree, but I personally think it's
OK to introduce new tests in a better style without touching the
existing ones in the same patch (or requiring a preparatory patch to the
same effect).
> > It looks like these tests match the existing style of the test suite,
> > but they are somewhat out of date with respect to our more modern
> > standards. I would probably write this like:
> >
> > test_expect_success 'git fetch --all (works with fetch.all = true)' '
> > git clone one test10 &&
> > test_config -C test10 fetch.all true &&
> > for r in one two three
> > do
> > git -C test10 remote add $r ../$r || return 1
> > done &&
> > git -C test10 fetch --all &&
> > git -C test10 branch -r >actual &&
> > test_cmp expect actual
> > '
>
> I think I'd prefer having the "actual" (and maybe also "expected") output in
> the repository so that it won't be overwritten by the next test case.
Very reasonable.
> Thanks for the great review, will send an updated patch later.
Thanks for working on this!
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v3] write-or-die: make GIT_FLUSH a Boolean environment variable
From: Junio C Hamano @ 2024-01-04 18:35 UTC (permalink / raw)
To: Chandra Pratap via GitGitGadget; +Cc: git, Chandra Pratap, Chandra Pratap
In-Reply-To: <pull.1628.v3.git.1704363617842.gitgitgadget@gmail.com>
"Chandra Pratap via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Chandra Pratap <chandrapratap3519@gmail.com>
>
> Among Git's environment variable, the ones marked as "Boolean"
> accept values in a way similar to Boolean configuration variables,
With "Among" you probably mean that there are many and some of them
are "marked as 'Boolean'", so I'd do "variable" -> "variables" while
queuing.
Other than that, looks great. Will queue. Thanks.
^ permalink raw reply
* Re: [PATCH] fetch: add new config option fetch.all
From: Tamino Bauknecht @ 2024-01-04 18:32 UTC (permalink / raw)
To: Taylor Blau; +Cc: git
In-Reply-To: <ZZbr4yAJe0dnHRcO@nand.local>
Forgot to put the mailing list into the CC - sorry for the duplicate
mail, Taylor.
On 1/4/24 18:33, Taylor Blau wrote:
> Instead of "can be overridden ...", how about:
>
> This behavior can be overridden by explicitly specifying one or more
> remote(s) to fetch from.
Sure, although I feel a bit conflicted since "git fetch one two" still
doesn't work and would require "--multiple". But probably still better
than my wording.
> I don't feel particularly strongly about whether or not you reorganize
> these if-statements, but we should change "argc == 0" to "!argc", which
> matches the conventions of the rest of the project.
Are you sure that I shouldn't use "argc == 0" here instead since it's
also used in the next "else if" condition? Or is the goal to gradually
transition to "!argc" in the entire code base?
I agree with keeping the diff minimal, will change that to your suggestion.
> This should be `cat >expect <<-\EOF` (without the space between the
> redirect and heredoc, as well as indicating that the heredoc does not
> need any shell expansions).
Will do so, thanks.
I tried to match the existing test cases as closely as possible, but if
they are outdated, it might be better to use the more recent structure.
> It looks like these tests match the existing style of the test suite,
> but they are somewhat out of date with respect to our more modern
> standards. I would probably write this like:
>
> test_expect_success 'git fetch --all (works with fetch.all = true)' '
> git clone one test10 &&
> test_config -C test10 fetch.all true &&
> for r in one two three
> do
> git -C test10 remote add $r ../$r || return 1
> done &&
> git -C test10 fetch --all &&
> git -C test10 branch -r >actual &&
> test_cmp expect actual
> '
I think I'd prefer having the "actual" (and maybe also "expected")
output in the repository so that it won't be overwritten by the next
test case.
> I suspect that you could go further with a "setup" function that gives
> you a fresh clone (with fetch.all set to a specified value), and then
> test test would continue on from the line "git fetch one". But I think
> it's worth not getting too carried away with refactoring these tests
> ;-).
I think a setup function makes sense to at least remove the clutter from
adding the remotes. Although I think that setting the value of fetch.all
in the test case will make it easier to parse the test code - that way
you don't have to look up different functions to understand the test.
Thanks for the great review, will send an updated patch later.
^ permalink raw reply
* Re: [PATCH] fetch: add new config option fetch.all
From: Junio C Hamano @ 2024-01-04 18:29 UTC (permalink / raw)
To: Taylor Blau; +Cc: Tamino Bauknecht, git
In-Reply-To: <ZZbr4yAJe0dnHRcO@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
>> +cat > expect << EOF
>
> This should be `cat >expect <<-\EOF` (without the space between the
> redirect and heredoc, as well as indicating that the heredoc does not
> need any shell expansions).
I noticed the same but I refrained from commenting on them ;-)
The original already is littered with style violations of this kind
(aka "old style"). If we were writing the tests in this file today,
we would also move the preparation of "expect" inside the
test_expect_success block that uses the expected output file.
If we do a style fix of the existing tests in this file as a
preliminary clean-up before the main patch that adds fetch.all and
its tests, that would be great. But for an initial step, I think it
is OK to have a single step patch that imitates the existing ones.
Perhaps after the initial review, it can become a two-patch series
to do so.
Thanks.
^ permalink raw reply
* Re: [PATCH] fix: platform accordance while calculating murmur3
From: Taylor Blau @ 2024-01-04 18:27 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jonathan Tan, Chen Xuewei via GitGitGadget, git, Chen Xuewei
In-Reply-To: <xmqq7ckp7ysl.fsf@gitster.g>
On Thu, Jan 04, 2024 at 10:12:42AM -0800, Junio C Hamano wrote:
> Jonathan and Taylor, isn't this what you two were working together
> on? How would we want to proceed?
They are indeed similar. I think that Jonathan and my series would
supersede this effort.
But I would appreciate if Chen took a look at the approach in that
series to make sure that we're all on the same page and that Jonathan
and I aren't missing anything.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] fetch: add new config option fetch.all
From: Junio C Hamano @ 2024-01-04 18:23 UTC (permalink / raw)
To: Tamino Bauknecht; +Cc: git
In-Reply-To: <20240104143656.615117-1-dev@tb6.eu>
Tamino Bauknecht <dev@tb6.eu> writes:
> This commit introduces the new boolean configuration option fetch.all
> which allows to fetch all available remotes by default. The config
> option can be overridden by explicitly specifying a remote.
> The behavior for --all is unchanged and calling git-fetch with --all and
> a remote will still result in an error.
This sounds like a usability annoyance for forks that use --all some
of the time and not always, as they now have to remember not just to
pass something to countermand the configured fetch.all but what that
something exactly is (i.e., the name of the remote they fetch from
by default), which makes fetch.all less appealing. I wonder if we
can instead take "--no-all" from the command line to make configured
fetch.all ignored (hence, giving an explicit remote will fetch from
there, and not giving any remote will fetch from whereever the
command will fetch from with "git -c fetch.all=false fetch")?
> The option was also added to the config documentation and new tests
> cover the expected behavior.
> ---
Missing sign-off.
> Documentation/config/fetch.txt | 4 ++
> builtin/fetch.c | 18 +++++--
> t/t5514-fetch-multiple.sh | 88 ++++++++++++++++++++++++++++++++++
> 3 files changed, 105 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
> index aea5b97477..4f12433874 100644
> --- a/Documentation/config/fetch.txt
> +++ b/Documentation/config/fetch.txt
> @@ -50,6 +50,10 @@ fetch.pruneTags::
> refs. See also `remote.<name>.pruneTags` and the PRUNING
> section of linkgit:git-fetch[1].
>
> +fetch.all::
> + If true, fetch will attempt to update all available remotes.
> + This behavior can be overridden by explicitly specifying a remote.
And we should say that this configuration variable defaults to false.
> @@ -2337,11 +2344,12 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> fetch_bundle_uri(the_repository, bundle_uri, NULL))
> warning(_("failed to fetch bundles from '%s'"), bundle_uri);
>
> - if (all) {
> - if (argc == 1)
> - die(_("fetch --all does not take a repository argument"));
> - else if (argc > 1)
> - die(_("fetch --all does not make sense with refspecs"));
> + if (all && argc == 1) {
> + die(_("fetch --all does not take a repository argument"));
> + } else if (all && argc > 1) {
> + die(_("fetch --all does not make sense with refspecs"));
> + } else if (all || (config.all > 0 && argc == 0)) {
> + /* Only use fetch.all config option if no remotes were explicitly given */
> (void) for_each_remote(get_one_remote_for_fetch, &list);
This conditional cascade will probably need to change when we allow
"--no-all" to countermand the configured fetch.all anyway, so I
won't worry about it now, but it looks somewhat convoluted that we
have to re-check "all" many times, which was the point of the
original that implemented this as a nested conditional.
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