* 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
* [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: 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
* 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: 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: [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: 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: 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: [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
* [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
* [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
* 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
* 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: 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: [PATCH 2/2] ref-filter: support filtering of operational refs
From: Junio C Hamano @ 2024-01-04 23:59 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Patrick Steinhardt, Taylor Blau, git, christian.couder
In-Reply-To: <CAOLa=ZStP6F1njTeoQZwN58k+_0r9LT7z-wg2819FWZPq90wQg@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> Thanks all for the discussion, I'll try to summarize the path forward
> as per my understanding.
It has already been clear for the past 5 years or so since 3a3b9d8c
(refs: new ref types to make per-worktree refs visible to all
worktrees, 2018-10-21) that we need to treat "worktrees/foo/HEAD",
"worktrees/bar/refs/bisect/bad", etc. as something end-users can
access via the normal ref mechansim (by a resolve_ref() call that is
eventually made from get_sha1() when these are passed to say "git
log"); I just did not remember that one but that does not mean we
can suddenly change the rules.
So you'd probably need to tweak the end of your bullet point #3, but
other than that it is a great summary.
Thanks.
> 1. We want to introduce a way to output all refs. This includes refs
> under "refs/", pseudo refs, HEAD, and any other ref like objects under
> $GIT_DIR. The reasoning being that users are allowed currently to create
> refs without any directory restrictions. So with the upcoming reftable
> backend, it becomes important to provide a utility to print all the refs
> held in the reftable. Ideally we want to restrict such ref's from being
> created but for the time being, since thats allowed, we should also
> provide the utility to print such refs.
>
> 2. In the files backend, this would involve iterating through the
> $GIT_DIR and finding all ref-like objects and printing them.
>
> 3. To expose this to the user, we could do something like
>
> $ git for-each-ref ""
>
> Which is a natural extension of the current syntax, where the empty
> string would imply that we do not filter to the "refs/" directory.
> It is still not clear whether we should support "worktrees".
>
> Any corrections here?
^ permalink raw reply
* [PATCH v2] rebase: clarify --reschedule-failed-exec default
From: Illia Bobyr @ 2024-01-05 0:42 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
In-Reply-To: <ZZcE/Kw24YKlqSOT@nand.local>
Applied.
Thank you for reviewing!
^ permalink raw reply
* [PATCH v2] rebase: clarify --reschedule-failed-exec default
From: Illia Bobyr @ 2024-01-05 0:42 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Illia Bobyr
In-Reply-To: <20240105004246.1317445-1-illia.bobyr@gmail.com>
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.
Signed-off-by: Illia Bobyr <illia.bobyr@gmail.com>
---
Documentation/git-rebase.txt | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git Documentation/git-rebase.txt Documentation/git-rebase.txt
index 1dd65..45d3c 100644
--- Documentation/git-rebase.txt
+++ Documentation/git-rebase.txt
@@ -626,13 +626,16 @@ See also INCOMPATIBLE OPTIONS below.
Automatically reschedule `exec` commands that failed. This only makes
sense in interactive mode (or when an `--exec` option was provided).
+
-Even though this option applies once a rebase is started, it's set for
-the whole rebase at the start based on either the
-`rebase.rescheduleFailedExec` configuration (see linkgit:git-config[1]
-or "CONFIGURATION" below) or whether this option is
-provided. Otherwise an explicit `--no-reschedule-failed-exec` at the
-start would be overridden by the presence of
-`rebase.rescheduleFailedExec=true` configuration.
+This option applies once a rebase is started. It is preserved for the whole
+rebase based on, in order, the command line option provided to the initial `git
+rebase`, the `rebase.rescheduleFailedExec` configuration (see
+linkgit:git-config[1] or "CONFIGURATION" below), or it defaults to false.
++
+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`.
--update-refs::
--no-update-refs::
--
2.40.1
^ permalink raw reply
* Re: [PATCH v2 1/2] fetch: add new config option fetch.all
From: Eric Sunshine @ 2024-01-05 1:02 UTC (permalink / raw)
To: Tamino Bauknecht; +Cc: git
In-Reply-To: <20240104222259.15659-1-dev@tb6.eu>
On Thu, Jan 4, 2024 at 5:23 PM Tamino Bauknecht <dev@tb6.eu> wrote:
> 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".
Thanks, looks better. More below...
> diff --git 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
> +}
I wasn't paying attention to this function in the previous round, but
it ought to be made more robust. If the `clone` operation fails, we
want to know about it right away rather than finding out about it "by
accident" when the subsequent `remote add` operation fails. In other
words, you should maintain an unbroken &&-chain, not just in test
bodies, but also in functions which are called from within test
bodies. So, this should really be:
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
}
> @@ -209,4 +218,90 @@ 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 &&
> + ...
> + 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 &&
> + ...
> + EOF
> + test_cmp expect actual
> + )
> +'
I'm probably overlooking something, but isn't this testing the exact
same thing as was tested in the "true" case of the loop just above?
Or maybe there is a bug in this test and you meant `git fetch` rather
than `git fetch --all`?
^ permalink raw reply
* Re: [PATCH] push: region_leave trace for negotiate_using_fetch
From: Sam Delmerico @ 2024-01-05 1:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, steadmon
In-Reply-To: <xmqqbka27zu9.fsf@gitster.g>
* I don't exactly remember how I noticed it. I was doing some
debugging around the push negotiation code and either 1) saw this
region get entered twice in the trace output, or 2) I was just reading
the code around here and saw two enters.
* Perhaps there could be a check before the last git process ends that
checks that all opened trace regions have been closed? I'm not sure
how much work this would involve. It's probably also not a very
proactive way to catch these bugs since it would only get triggered
when a *user* hits a code path with a trace region that never exits.
* There could also be a test that checks that every region_enter trace
log has a corresponding region_leave. But I'm not sure how to ensure
that every code path is checked.
Overall, I'm not sure how much benefit there is from checking for
this. I'm not sure that it would have a large impact if it were to
happen again. For example, I think that it could be noticed relatively
quickly by a person/system looking at metrics like I was (e.g. if the
time spent in a region is infinite or zero).
FWIW I didn't see any other examples of this when going through logs.
Sam
<br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed,
Jan 3, 2024 at 3:37 PM Junio C Hamano <gitster@pobox.com>
wrote:<br></div><blockquote class="gmail_quote" style="margin: 0px 0px
0px 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left:
1ex;">Sam Delmerico <<a href="mailto:delmerico@google.com"
target="_blank">delmerico@google.com</a>> writes:<br>
<br>
> There were two region_enter events for negotiate_using_fetch instead of<br>
> one enter and one leave. This commit replaces the second region_enter<br>
> event with a region_leave.<br>
><br>
> Signed-off-by: Sam Delmerico <<a
href="mailto:delmerico@google.com"
target="_blank">delmerico@google.com</a>><br>
> ---<br>
> fetch-pack.c | 2 +-<br>
> 1 file changed, 1 insertion(+), 1 deletion(-)<br>
<br>
Looks right, after skimming a29263cf (fetch-pack: add tracing for<br>
negotiation rounds, 2022-08-02). Two questions and a half.<br>
<br>
* How did you find it? Code inspection? While
writing a script to<br>
parse the output from around this area, your script noticed the<br>
ever-increasing nesting level? Something else?<br>
<br>
* Would it be feasible to write some tests or tools that find<br>
similar problems (semi-)automatically?<br>
<br>
* Is the breakage (before this patch) something easily demonstrated<br>
in a new test in t/ somewhere? And if so, would it
be worth<br>
doing?<br>
<br>
Thanks. Will queue.<br>
<br>
<br>
><br>
> diff --git a/fetch-pack.c b/fetch-pack.c<br>
> index 31a72d43de..dba6d79944 100644<br>
> --- a/fetch-pack.c<br>
> +++ b/fetch-pack.c<br>
> @@ -2218,7 +2218,7 @@ void negotiate_using_fetch(const struct
oid_array *negotiation_tips,<br>
>
the_repository, "%d",<br>
>
negotiation_round);<br>
> }<br>
> - trace2_region_enter("fetch-pa<wbr>ck",
"negotiate_using_fetch", the_repository);<br>
> + trace2_region_leave("fetch-pa<wbr>ck",
"negotiate_using_fetch", the_repository);<br>
>
trace2_data_intmax("<wbr>negotiate_using_fetch",
the_repository,<br>
>
"total_rounds", negotiation_round);<br>
> clear_common_flag(acked_commi<wbr>ts);<br>
><br>
> base-commit: a26002b62827b89a19b1084bd75d93<wbr>71d565d03c<br>
</blockquote></div>
^ permalink raw reply
* [PATCH v3] rebase: clarify --reschedule-failed-exec default
From: Illia Bobyr @ 2024-01-05 1:14 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
In-Reply-To: <ZZcE/Kw24YKlqSOT@nand.local>
Sorry, I did not actually include the change in v2.
Still learning how to use git send-email.
On Thu, Jan 04, 2024 at 11:20:28AM -0800, Taylor Blau wrote:
> [...]
>
> > +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".
Applied.
Thank you for reviewing!
^ permalink raw reply
* [PATCH v3] rebase: clarify --reschedule-failed-exec default
From: Illia Bobyr @ 2024-01-05 1:14 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Illia Bobyr
In-Reply-To: <ZZcE/Kw24YKlqSOT@nand.local>
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.
Signed-off-by: Illia Bobyr <illia.bobyr@gmail.com>
---
Documentation/git-rebase.txt | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git Documentation/git-rebase.txt Documentation/git-rebase.txt
index 1dd65..b6282 100644
--- Documentation/git-rebase.txt
+++ Documentation/git-rebase.txt
@@ -626,13 +626,16 @@ See also INCOMPATIBLE OPTIONS below.
Automatically reschedule `exec` commands that failed. This only makes
sense in interactive mode (or when an `--exec` option was provided).
+
-Even though this option applies once a rebase is started, it's set for
-the whole rebase at the start based on either the
-`rebase.rescheduleFailedExec` configuration (see linkgit:git-config[1]
-or "CONFIGURATION" below) or whether this option is
-provided. Otherwise an explicit `--no-reschedule-failed-exec` at the
-start would be overridden by the presence of
-`rebase.rescheduleFailedExec=true` configuration.
+This option applies once a rebase is started. It is preserved for the whole
+rebase based on, in order, the command line option provided to the initial `git
+rebase`, the `rebase.rescheduleFailedExec` configuration (see
+linkgit:git-config[1] or "CONFIGURATION" below), or it defaults to false.
++
+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 cannot pass
+`--[no-]reschedule-failed-exec` to `git rebase --continue`.
--update-refs::
--no-update-refs::
--
2.40.1
^ permalink raw reply
* Re: [GSOC][RFC] Heed core.bare from template config file when no command line override given, as a microproject.
From: Elijah Newren @ 2024-01-05 2:11 UTC (permalink / raw)
To: Christian Couder; +Cc: Ghanshyam Thakkar, git, gitster, johannes.schindelin
In-Reply-To: <CAP8UFD1wMJMY6G4SaPTPwq6b9HbeXG1kB97-RRrL-KGN1wE0rg@mail.gmail.com>
On Thu, Jan 4, 2024 at 2:24 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Tue, Jan 2, 2024 at 11:17 PM Ghanshyam Thakkar
> <shyamthakkar001@gmail.com> wrote:
> >
> > Hello,
> >
> > I'm currently an undergrad beginning my journey of contributing to the
> > Git project. I am seeking feedback on doing "Heed core.bare from
> > template config file when no command line override given" described
> > here
> > https://lore.kernel.org/git/5b39c530f2a0edf3b1492fa13a1132d622a0678e.1684218850.git.gitgitgadget@gmail.com/
> > by Elijah Newren, as a microproject. I would like to know from the
> > community, if the complexity and scope of the project is appropriate
> > for a microproject.
>
> Thanks for your interest in the next GSoC!
>
> My opinion is that it's too complex for a micro-project. Now maybe if
> Elijah or others are willing to help you on it, perhaps it will work
> out. I think it's safer to look at simpler micro-projects though.
An important part of solving this problem, if you were to do so, would
be adding several testcases (including some showing how it currently
fails). Simply adding some or all of those testcases would be a good
micro-project. (If you take this on, it'd probably be worthwhile to
include a reference to any such added tests in the TODO comment here
so that future folks noticing the TODO are aware of them). Then, if
adding those testcases goes well and you feel ambitious, you can try
to tackle the underlying problem too.
> > e.g. in builtin/init-db.c :
> >
> > static int template_bare_config(const char *var, const char *value,
> > const struct config_context *ctx, void *cb)
> > {
> > if(!strcmp(var,"core.bare")) {
>
> We like to have a space character between "if" and "(" as well as after a ","
>
> > is_bare_repository_cfg = git_config_bool(var, value);
> > }
> > return 0;
> > }
> >
> > int cmd_init_db(int argc, const char **argv, const char *prefix)
> > {
> > ...
> > ...
> > if(is_bare_repository_cfg==-1) {
>
> We like to have a space character both before and after "==" as well
> as between "if" and "(".
>
> > if(!template_dir)
> > git_config_get_pathname("init.templateDir",
> > &template_dir);
> >
> > if(template_dir) {
> > const char* template_config_path
> > = xstrfmt("%s/config",
> > struct stat st;
> >
> > if(!stat(template_config_path, &st) &&
> > !S_ISDIR(st.st_mode)) {
> > git_config_from_file(template_bare_cfg,
> > template_config_path, NULL);
> > }
> > }
> > ...
> > ...
> > return init_db(git_dir, real_git_dir, template_dir, hash_algo,
> > initial_branch, init_shared_repository, flags);
> > }
> >
> > I also wanted to know if the global config files should have an effect
> > in deciding if the repo is bare or not.
> >
> > Curious to know your thoughts on, if this is the right approach or
> > does it require doing refactoring to bring all the logic in setup.c.
> > Based on your feedback, I can quickly send a patch.
>
> I don't know this area of the code well, so I don't think I can help
> you much on this.
If you look back at the mailing list discussion on the series that
introduced this TODO comment you are trying to address, you'll note
that both Glen and I dug into the code and attempted to explain it to
each other, and we both got it wrong on our first try. If I knew the
correct solution without digging, I probably would have just
implemented it and sent it in as another patch series. My TODO was
not meant to be a definitive guide about where to make the fixes
(because I don't know yet), but just a helpful guide that the first
spot you'd expect cannot be the correct location (I already tried a
few things there) and that you need to dig further. Anyway, I'm sure
the correct place to fix can be figured out with a bit of work, but in
this case, figuring out where the changes need to be made is probably
the majority of the effort.
You may well need to just pick an area, start modifying, go through it
in a debugger and with your testcases, etc., and learn whether
modifications in that area even can solve the problem. You may have
to discard your first or even second attempts, but take what you
learned to guide you on future attempts.
And if you do get it all working, this is a case where it'd likely be
important to comment in the commit message not just why you are making
changes, but why you believe the area you modified is the correct one
(i.e. to mention why the more obvious places to modify don't actually
solve the problem). And then be prepared for folks on the mailing
list to use the information you provided in the commit message to dive
in and point out some other way to fix the problem that is even better
but requires you to redo it again. (And I'm not just saying that
because you're new; I would not be surprised if the same happened to
me. Others are more familiar with various parts of the general setup
and config code than me, and sometimes additional expertise coupled
with a solution of some sort can make it easier to identify an even
better solution.)
^ permalink raw reply
* Re: Does extending `--empty` to git-cherry-pick make sense?
From: Elijah Newren @ 2024-01-05 2:28 UTC (permalink / raw)
To: Taylor Blau; +Cc: Brian Lyles, git
In-Reply-To: <ZZcIG+mNXhZ0rHw3@nand.local>
On Thu, Jan 4, 2024 at 11:33 AM Taylor Blau <me@ttaylorr.com> wrote:
>
> [+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.
I was indeed focused on simply getting the multiple rebase backends to
have consistent behavior (we had like 4-5 backends at the time,
right?) and just didn't consider cherry-pick at the time. Now that
those are more consistent (though there's still work to be done on
that end too), cherry-pick and rebase really ought to share a lot more
between each other, from command line flags, to temporary control
files written, to more shared code paths. Adding an --empty flag to
cherry-pick as a step along the way makes sense.
Note that git-am also gained a similar flag in the meantime, but
changed the names slightly: --empty=(stop,drop,keep). I think 'stop'
is a better name than 'ask', and we really should make rebase suggest
'stop' instead (but keep 'ask' as a synonym for 'stop', for backwards
compatibility). Also, I kind of want to replace 'keep' with 'roll' in
the --empty option for both git-am and git-rebase, while keeping
'keep' as a synonym for 'roll'. But I'm not sure if others on the
list would go along with it...
^ permalink raw reply
* Re: [PATCH v2 2/2] fetch: add cli option --default-only
From: Eric Sunshine @ 2024-01-05 2:43 UTC (permalink / raw)
To: Tamino Bauknecht; +Cc: git
In-Reply-To: <20240104222259.15659-2-dev@tb6.eu>
On Thu, Jan 4, 2024 at 5:23 PM Tamino Bauknecht <dev@tb6.eu> wrote:
> 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>
> ---
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> @@ -2344,15 +2347,23 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> + if (all && default_only) {
> + die(_("fetch --all does not work with fetch --default-only"));
To simplify the life of people who translate Git messages into other
languages, these days we have standard wording for this type of
message, and we extract the literal option from the message itself.
So, this should be:
die(_("options '%s' and '%s' cannot be used together"),
"--all", "--default-only");
> diff --git 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
Do you also want to test the case when "fetch.all" isn't set?
> +test_expect_success 'git fetch --all does not work with --default-only' '
> + (
> + cd test &&
> + test_must_fail git fetch --all --default-only
> + )
> +'
Minor point: This sort of test can be written more succinctly without
the subshell:
test_expect_success 'git fetch --all does not work with --default-only' '
test_must_fail git -C test fetch --all --default-only
'
^ permalink raw reply
* Re: Does extending `--empty` to git-cherry-pick make sense?
From: Brian Lyles @ 2024-01-05 3:20 UTC (permalink / raw)
To: Elijah Newren; +Cc: Taylor Blau, git
In-Reply-To: <CABPp-BGJfvBhO_zEX8nLoa8WNsjmwvtZ2qOjmYm9iPoZg4SwPw@mail.gmail.com>
On Thu, Jan 4, 2024 at 8:28 PM Elijah Newren <newren@gmail.com> wrote:
>
> I was indeed focused on simply getting the multiple rebase backends to
> have consistent behavior (we had like 4-5 backends at the time,
> right?) and just didn't consider cherry-pick at the time. Now that
> those are more consistent (though there's still work to be done on
> that end too), cherry-pick and rebase really ought to share a lot more
> between each other, from command line flags, to temporary control
> files written, to more shared code paths. Adding an --empty flag to
> cherry-pick as a step along the way makes sense.
I appreciate the insight from both of you.
It sounds like I'll be diving into some C code for the first time in
over a decade, then! I'll plan to take a crack at this soon.
-Brian Lyles
^ 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