* 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
* Re: [PATCH] fix: platform accordance while calculating murmur3
From: Junio C Hamano @ 2024-01-04 18:12 UTC (permalink / raw)
To: Jonathan Tan, Taylor Blau; +Cc: Chen Xuewei via GitGitGadget, git, Chen Xuewei
In-Reply-To: <pull.1636.git.git.1704376606625.gitgitgadget@gmail.com>
"Chen Xuewei via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Chen Xuewei <316403398@qq.com>
>
> It is known that whether the highest bit is extended when char cast to
> uint32, depends on CPU architecture, which will lead different hash
> value. This is a fix to accord all architecture behaviour.
>
> Signed-off-by: Chen Xuewei <316403398@qq.com>
> ---
Jonathan and Taylor, isn't this what you two were working together
on? How would we want to proceed?
Chen, using the right implementation of the hash function to be used
after the next rebuild of the Bloom data has so far been treated as
only one part of the solution (the others include "how to deal with
the existing data? do we have a way to tell our binary to safely
ignore the Bloom data using a wrong hash?" and "how to make sure old
binaries do not get confused by the Bloom data using the right/new
hash?").
Jonathan and Taylor's (stalled) effort is here
https://lore.kernel.org/git/cover.1697653929.git.me@ttaylorr.com
Thanks.
^ permalink raw reply
* Re: [PATCH] fetch: add new config option fetch.all
From: Eric Sunshine @ 2024-01-04 18:04 UTC (permalink / raw)
To: Taylor Blau; +Cc: Tamino Bauknecht, git
In-Reply-To: <ZZbr4yAJe0dnHRcO@nand.local>
On Thu, Jan 4, 2024 at 12:33 PM Taylor Blau <me@ttaylorr.com> wrote:
> On Thu, Jan 04, 2024 at 03:33:55PM +0100, Tamino Bauknecht wrote:
> > +cat > expect << EOF
> > + one/main
> > + ...
> > +EOF
>
> 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
> '
If you (Taylor) were writing these tests, you would also create the
"expect" file inside the test body:
test_expect_success 'git fetch --all (works with fetch.all = true)' '
cat >expect <<-\EOF &&
one/main
...
EOF
git clone one test10 &&
...
The <<- operator which Taylor used in his example allows you to indent
the body of the heredoc, so it can be indented the same amount as the
test body itself.
^ permalink raw reply
* Re: [PATCH v3] write-or-die: make GIT_FLUSH a Boolean environment variable
From: Taylor Blau @ 2024-01-04 17:36 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>
Hi Chandra,
On Thu, Jan 04, 2024 at 10:20:17AM +0000, Chandra Pratap via GitGitGadget wrote:
> ---
> Documentation/git.txt | 5 ++---
> write-or-die.c | 19 ++++++++-----------
> 2 files changed, 10 insertions(+), 14 deletions(-)
Thanks very much for working on this, and taking some of my suggestions
into account.
This version looks great to me.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v2] write-or-die: make GIT_FLUSH a Boolean environment variable
From: Taylor Blau @ 2024-01-04 17:35 UTC (permalink / raw)
To: Junio C Hamano
Cc: Torsten Bögershausen, Patrick Steinhardt,
Chandra Pratap via GitGitGadget, git, Chandra Pratap,
Chandra Pratap
In-Reply-To: <xmqq5y0a9qed.fsf@gitster.g>
On Wed, Jan 03, 2024 at 11:18:50AM -0800, Junio C Hamano wrote:
> > Thanks for a nice reading - I can not imagine a better version.
>
> Yup, the flow of the logic feels very natural with this version by
> making it clear that the case that the default "-1" is returned to
> us is where we need to figure out an appropriate value ourselves.
> An added bonus is that the scope "struct stat" has is limited to the
> absolute minimum. I like it, too.
Thanks, both.
Thanks,
Taylor
^ permalink raw reply
* Re: Concurrent fetch commands
From: Taylor Blau @ 2024-01-04 17:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Patrick Steinhardt, Oswald Buddenhagen, Stefan Haller, git
In-Reply-To: <xmqqwmsq83v3.fsf@gitster.g>
On Wed, Jan 03, 2024 at 02:10:56PM -0800, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > ... I suppose the answer is that they expect
> > concurrent fetches to be tolerated, but that the contents of FETCH_HEAD
> > (and of course the remote references) are consistent at the end of all
> > of the fetches.
>
> What does it mean to be "consistent" in this case, though? For the
> controlled form of multiple fetches performed by "git fetch --all",
> the answer is probably "as if we fetched sequentially from these
> remotes, one by one, and concatenated what these individual fetch
> invocations left in FETCH_HEAD". But for an uncontrolled background
> fetch IDE and others perform behind user's back, it is unclear what
> it means, or for that matter, it is dubious if there is a reasonable
> definition for the word.
Yeah, on thinking on it more I tend to agree here.
> Nobody brought up the latter so far on this discussion thread, but
> mucking with the remote-tracking branches behind user's back means
> completely breaking the end-user expectation that --force-with-lease
> would do something useful even when it is not given the commit the
> user expects to see at the remote. Perhaps those third-party tools
> that want to run "git fetch" in the background can learn from how
> "prefetch" task works to avoid the breakage they are inflicting on
> their users?
Probably so.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] fetch: add new config option fetch.all
From: Taylor Blau @ 2024-01-04 17:33 UTC (permalink / raw)
To: Tamino Bauknecht; +Cc: git
In-Reply-To: <20240104143656.615117-1-dev@tb6.eu>
On Thu, Jan 04, 2024 at 03:33:55PM +0100, Tamino Bauknecht wrote:
> ---
> Documentation/config/fetch.txt | 4 ++
> builtin/fetch.c | 18 +++++--
> t/t5514-fetch-multiple.sh | 88 ++++++++++++++++++++++++++++++++++
> 3 files changed, 105 insertions(+), 5 deletions(-)
Nice. This looks like a useful feature that folks working with more than
one remote may want to take advantage of. Not that typing "git fetch
--all" is all that hard, but I can see the utility of something like
this for a repository with >1 remotes where the individual wants to keep
all remotes up-to-date.
> 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.
> +
Instead of "can be overridden ...", how about:
This behavior can be overridden by explicitly specifying one or more
remote(s) to fetch from.
> @@ -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 */
To minimize the diff, let's leave the existing conditional as is, so the
end state would look like:
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 || (config.all > 0 && !argc))
(void) for_each_remote(get_one_remote_for_fetch, &list);
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.
> (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..cd0aee97f9 100755
> --- a/t/t5514-fetch-multiple.sh
> +++ b/t/t5514-fetch-multiple.sh
> @@ -209,4 +209,92 @@ test_expect_success 'git fetch --multiple --jobs=0 picks a default' '
> git fetch --multiple --jobs=0)
> '
>
> +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).
> + 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_expect_success 'git fetch (fetch all remotes with fetch.all = true)' '
> + (git clone one test9 &&
> + cd test9 &&
> + git config fetch.all true &&
> + git remote add one ../one &&
> + git remote add two ../two &&
> + git remote add three ../three &&
> + git fetch &&
> + git branch -r > output &&
Same note here about the space between the redirect.
> + test_cmp ../expect output)
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
'
While reviewing, I thought that the tests using the test9 and test10
clones were duplicates, but they are not: the earlier one uses a "git
fetch", and the latter uses a "git fetch --all".
If we take just the test10 and test11 tests, I think there is some
opportunity to consolidate these two, like so:
for v in true false
do
test_expect_success "git fetch --all (works with fetch.all=$v)" '
git clone one test10 &&
test_config -C test10 fetch.all $v &&
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
'
done
> +cat > expect << EOF
> + one/main
> + one/side
> + origin/HEAD -> origin/main
> + origin/main
> + origin/side
> +EOF
Same note(s) about cleaning up this heredoc.
> +test_expect_success 'git fetch one (explicit remote overrides fetch.all)' '
> + (git clone one test12 &&
> + cd test12 &&
> + git config fetch.all true &&
> + git remote add one ../one &&
> + git remote add two ../two &&
> + git remote add three ../three &&
> + git fetch one &&
> + git branch -r > output &&
> + test_cmp ../expect output)
> +'
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
;-).
Thanks,
Taylor
^ permalink raw reply
* [PATCH] fetch: add new config option fetch.all
From: Tamino Bauknecht @ 2024-01-04 14:33 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.
---
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.
+
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..367f8d3c74 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,
@@ -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);
/* do not do fetch_multiple() of one */
diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh
index a95841dc36..cd0aee97f9 100755
--- a/t/t5514-fetch-multiple.sh
+++ b/t/t5514-fetch-multiple.sh
@@ -209,4 +209,92 @@ test_expect_success 'git fetch --multiple --jobs=0 picks a default' '
git fetch --multiple --jobs=0)
'
+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_expect_success 'git fetch (fetch all remotes with fetch.all = true)' '
+ (git clone one test9 &&
+ cd test9 &&
+ git config fetch.all true &&
+ git remote add one ../one &&
+ git remote add two ../two &&
+ git remote add three ../three &&
+ git fetch &&
+ git branch -r > output &&
+ test_cmp ../expect output)
+'
+
+test_expect_success 'git fetch --all (works with fetch.all = true)' '
+ (git clone one test10 &&
+ cd test10 &&
+ git config fetch.all true &&
+ git remote add one ../one &&
+ git remote add two ../two &&
+ git remote add three ../three &&
+ git fetch --all &&
+ git branch -r > output &&
+ test_cmp ../expect output)
+'
+
+test_expect_success 'git fetch --all (works with fetch.all = false)' '
+ (git clone one test11 &&
+ cd test11 &&
+ git config fetch.all false &&
+ git remote add one ../one &&
+ git remote add two ../two &&
+ git remote add three ../three &&
+ git fetch --all &&
+ git branch -r > output &&
+ test_cmp ../expect output)
+'
+
+cat > expect << EOF
+ one/main
+ one/side
+ origin/HEAD -> origin/main
+ origin/main
+ origin/side
+EOF
+
+test_expect_success 'git fetch one (explicit remote overrides fetch.all)' '
+ (git clone one test12 &&
+ cd test12 &&
+ git config fetch.all true &&
+ git remote add one ../one &&
+ git remote add two ../two &&
+ git remote add three ../three &&
+ git fetch one &&
+ git branch -r > output &&
+ test_cmp ../expect output)
+'
+
+cat > expect << EOF
+ origin/HEAD -> origin/main
+ origin/main
+ origin/side
+EOF
+
+test_expect_success 'git config fetch.all false (fetch only default remote)' '
+ (git clone one test13 &&
+ cd test13 &&
+ git config fetch.all false &&
+ git remote add one ../one &&
+ git remote add two ../two &&
+ git remote add three ../three &&
+ git fetch &&
+ git branch -r > output &&
+ test_cmp ../expect output)
+'
+
test_done
--
2.43.0
^ permalink raw reply related
* Re: [PATCH] fix: platform accordance while calculating murmur3
From: Taylor Blau @ 2024-01-04 14:53 UTC (permalink / raw)
To: Chen Xuewei via GitGitGadget; +Cc: git, Chen Xuewei
In-Reply-To: <pull.1636.git.git.1704376606625.gitgitgadget@gmail.com>
Hi Chen,
On Thu, Jan 04, 2024 at 01:56:46PM +0000, Chen Xuewei via GitGitGadget wrote:
> From: Chen Xuewei <316403398@qq.com>
>
> It is known that whether the highest bit is extended when char cast to
> uint32, depends on CPU architecture, which will lead different hash
> value. This is a fix to accord all architecture behaviour.
Thanks for your patch. A similar fix is being pursued in [1], part of
which includes [2], which I believe is functionally equivalent to your
patch here.
> Others
> ======
>
> after fixed the bug, the historical bloom_filter data stored in
> commit-graph need to be updated. because the path's hash value is
> already calculated through a bad way. so we need to update it. this need
> to be done in repository
We would not want to impose that burden on all users upon upgrading to
the latest Git version. In [1] we are perusing an approach where:
- The Bloom data is stored with a version identifier, meaning that we
can still use the existing/non-murmur3 Bloom filters after
upgrading.
- When the user decides to upgrade from v1 -> v2 Bloom filters, we
reuse the existing Bloom filter data when possible, namely when all
paths within a tree have no non-ASCII characters.
If you have thoughts on the approach in [1], they would be most welcome.
Thanks,
Taylor
[1]: https://lore.kernel.org/git/cover.1697653929.git.me@ttaylorr.com/
[2]: https://lore.kernel.org/git/f6ab427ead86bc82284b2c721f3c177947ece3c9.1697653929.git.me@ttaylorr.com/
^ permalink raw reply
* Re: [PATCH v2] git-prompt: stop manually parsing HEAD with unknown ref formats
From: Patrick Steinhardt @ 2024-01-04 14:47 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: git, Eric Sunshine, SZEDER Gábor, Karthik Nayak
In-Reply-To: <ZZabqfPDcA1jtmZS@ugly>
[-- Attachment #1: Type: text/plain, Size: 1887 bytes --]
On Thu, Jan 04, 2024 at 12:51:05PM +0100, Oswald Buddenhagen wrote:
> On Thu, Jan 04, 2024 at 09:21:53AM +0100, Patrick Steinhardt wrote:
> > --- a/contrib/completion/git-prompt.sh
> > +++ b/contrib/completion/git-prompt.sh
> > @@ -408,7 +408,7 @@ __git_ps1 ()
> >
> > local repo_info rev_parse_exit_code
> > repo_info="$(git rev-parse --git-dir --is-inside-git-dir \
> > - --is-bare-repository --is-inside-work-tree \
> > + --is-bare-repository --is-inside-work-tree --show-ref-format \
> > --short HEAD 2>/dev/null)"
> >
> that makes me wonder whether adding support for `--symbolic-ref HEAD` here
> would not be the cleaner solution? and why stop there, and not add a few
> more ps1 would need, like --upstream and --sequencer-state? (though
> arguably, this overloading of `rev-parse` should be deprecated in favor of a
> new generalized `query` command, maybe even unified with `var`.)
I'm on board with extending git-rev-parse(1) to support direct output of
symbolic refs without resolving them to an object ID. Indeed, we plan to
tackle this lack of support soonish at GitLab. But given that such a
feature currently doesn't exist, and that I expect there to be some
discussion around it, I'd rather want to postpone this to a later point
so that we can meanwhile unblock the reftable backend.
Regarding the other options like `--upstream` and `--sequencer-state`
I'm less sure. As you say, git-rev-parse(1) is already quite loaded with
semi-related tools, and extending it even further like this is only
going to make this state worse. I also wish for an "informative" tool
that queries repository-level information and state like you propose,
but would argue that this is also a bigger topic.
So... for now I'd like to keep the current version, but I certainly
agree that the state can and should eventually be improved.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH] fix: platform accordance while calculating murmur3
From: Chen Xuewei via GitGitGadget @ 2024-01-04 13:56 UTC (permalink / raw)
To: git; +Cc: Chen Xuewei, Chen Xuewei
From: Chen Xuewei <316403398@qq.com>
It is known that whether the highest bit is extended when char cast to
uint32, depends on CPU architecture, which will lead different hash
value. This is a fix to accord all architecture behaviour.
Signed-off-by: Chen Xuewei <316403398@qq.com>
---
fix: platform accordance while calculating murmur3
Short Description
=================
fix: platform accordance while calculating murmur3
It is known that whether the highest bit is extended when char cast to
uint32, depends on CPU architecture, which will lead different hash
value. This is a fix to accord all architecture behaviour.
Problem backgroud:
==================
when using git log --max-count=1 <commit> -- <path> in an mixed cpu
cluster environment both arm and x86 in a cluster as a service, where
the <path> character is chinese or some other character that the highest
bit of char is 1. all machines share the same repo disk. It happened
that sometimes you can get the searched file among commit, sometimes you
cannot.
Conditions
==========
1. file path include chinese characters or other characters that the
highest bit is 1.
2. mixed cpu architecture as a git cluster service
Reason
======
when you have over 2 machines (both arm and x86 are included at least
one) as a git server cluster. once you open the commit-graph's
bloom_filter feature. The bloom filter stores the file path as hash
values using the murmur3 function. suppose the arm take it this time,
then the char's highest bit is not extended. for example, on arm,
char(11100110) to uint32(00000000 00000000 00000000 11100110) on x86,
char(11100110) to uint32(11111111 11111111 11111111 11100110) then
according to the murmur3 function that git currently use, the calculated
hash value will be different. If the value was calculated through the
same cpu architure machine, then it is ok. however, sometimes the hash
value is calculated through a different cpu architure machine, then you
cannot get the searched file. for example, bloom_filter's hash set is
calculated through arm, and query through x86. So the hash value is
incorrect, then missed the searched file.
Solution
========
No matter what the highest 24 bits will be when char cast to uint32, the
murmur3 function only cares about the char part , which is only the
lowest 8 bits, so we can use & 0xFF(11111111) to the casted uint32 value
to choose only the lowest 8 bits.
Others
======
after fixed the bug, the historical bloom_filter data stored in
commit-graph need to be updated. because the path's hash value is
already calculated through a bad way. so we need to update it. this need
to be done in repository
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1636%2Fcdegree%2Fmaster-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1636/cdegree/master-v1
Pull-Request: https://github.com/git/git/pull/1636
bloom.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/bloom.c b/bloom.c
index 1474aa19fa5..bc40edac795 100644
--- a/bloom.c
+++ b/bloom.c
@@ -116,11 +116,11 @@ uint32_t murmur3_seeded(uint32_t seed, const char *data, size_t len)
uint32_t k;
for (i = 0; i < len4; i++) {
- uint32_t byte1 = (uint32_t)data[4*i];
- uint32_t byte2 = ((uint32_t)data[4*i + 1]) << 8;
- uint32_t byte3 = ((uint32_t)data[4*i + 2]) << 16;
- uint32_t byte4 = ((uint32_t)data[4*i + 3]) << 24;
- k = byte1 | byte2 | byte3 | byte4;
+ uint32_t byte1 = ((uint32_t)data[4*i]) & 0xFF;
+ uint32_t byte2 = ((uint32_t)data[4*i + 1]) & 0xFF;
+ uint32_t byte3 = ((uint32_t)data[4*i + 2]) & 0xFF;
+ uint32_t byte4 = ((uint32_t)data[4*i + 3]) & 0xFF;
+ k = byte1 | (byte2 << 8) | (byte3 << 16) | (byte4 << 24);
k *= c1;
k = rotate_left(k, r1);
k *= c2;
base-commit: a26002b62827b89a19b1084bd75d9371d565d03c
--
gitgitgadget
^ permalink raw reply related
* Re: Concurrent fetch commands
From: Stefan Haller @ 2024-01-04 12:01 UTC (permalink / raw)
To: Junio C Hamano, Taylor Blau; +Cc: Patrick Steinhardt, Oswald Buddenhagen, git
In-Reply-To: <xmqqwmsq83v3.fsf@gitster.g>
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.
> Nobody brought up the latter so far on this discussion thread, but
> mucking with the remote-tracking branches behind user's back means
> completely breaking the end-user expectation that --force-with-lease
> would do something useful even when it is not given the commit the
> user expects to see at the remote.
That's an interesting point that indeed hasn't been brought up yet.
However, don't we all agree that --force-with-lease without specifying a
commit is not a good idea anyway, in general? That's why
--force-if-includes was invented, isn't it?
> Perhaps those third-party tools
> that want to run "git fetch" in the background can learn from how
> "prefetch" task works to avoid the breakage they are inflicting on
> their users?
Again, what you call "breakage" here is the very point of a background
fetch for me, so I don't want it to be avoided. (For FETCH_HEAD, yes,
and I learned that we have --no-write-fetch-head for that, but for
remote-tracking branches, no.)
^ permalink raw reply
* Re: [PATCH v2] git-prompt: stop manually parsing HEAD with unknown ref formats
From: Oswald Buddenhagen @ 2024-01-04 11:51 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Eric Sunshine, SZEDER Gábor, Karthik Nayak
In-Reply-To: <ef4e36a5a40c369da138242a8fdc9e12a846613b.1704356313.git.ps@pks.im>
On Thu, Jan 04, 2024 at 09:21:53AM +0100, Patrick Steinhardt wrote:
>--- a/contrib/completion/git-prompt.sh
>+++ b/contrib/completion/git-prompt.sh
>@@ -408,7 +408,7 @@ __git_ps1 ()
>
> local repo_info rev_parse_exit_code
> repo_info="$(git rev-parse --git-dir --is-inside-git-dir \
>- --is-bare-repository --is-inside-work-tree \
>+ --is-bare-repository --is-inside-work-tree --show-ref-format \
> --short HEAD 2>/dev/null)"
>
that makes me wonder whether adding support for `--symbolic-ref HEAD`
here would not be the cleaner solution? and why stop there, and not add
a few more ps1 would need, like --upstream and --sequencer-state?
(though arguably, this overloading of `rev-parse` should be deprecated
in favor of a new generalized `query` command, maybe even unified with
`var`.)
^ permalink raw reply
* Re: [PATCH 2/2] ref-filter: support filtering of operational refs
From: Karthik Nayak @ 2024-01-04 11:31 UTC (permalink / raw)
To: Patrick Steinhardt, Junio C Hamano; +Cc: Taylor Blau, git, christian.couder
In-Reply-To: <ZZWg5JvjQymy2wcn@tanuki>
[-- Attachment #1: Type: text/plain, Size: 2578 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> On Wed, Jan 03, 2024 at 09:59:13AM -0800, Junio C Hamano wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>>
>> >> I think we should tighten things up over time. First by teaching
>> >> the ref backend that anything that is not a pseudoref, HEAD or a
>> >> proper ref (one item of whose definition is "lives under refs/
>> >> hierarchy) should not resolve_ref() successfully. That should
>> >> correctly fail things like
>> >>
>> >> $ git rev-parse worktrees/$name/bisect/bad
>> >> $ git update-ref foo/bar HEAD
>> > ...
>> > Yeah, agreed, that's something we should do. I do wonder whether this
>> > will break existing usecases, but in any case I'd rather consider it an
>> > accident that it is possible to write (and read) such refs in the first
>> > place.
>>
>> Unfortunately, the worktrees/$name/refs/bisect/bad and its friends
>> are documented in "git worktree" and the refs.c layer is aware of
>> the "main-worktree/" and "worktrees/" hierarchy, so while I still
>> think it is a good long-term direction to make it impossible to
>> create random refs like "foo/bar" and "resf/heads/master" via the
>> commands like "git update-ref", we cannot limit ourselves only to
>> "refs/" hierarchy.
>
> Ah, I first wanted to point this out, but then noticed that you didn't
> include the "refs/" prefix in "worktrees/$name/bisect/bad" and thought
> this was intentional. But yes, per-worktree refs need to stay supported,
> weird as they may be.
>
> Patrick
Thanks all for the discussion, I'll try to summarize the path forward
as per my understanding.
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?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [GSOC][RFC] Heed core.bare from template config file when no command line override given, as a microproject.
From: Ghanshyam Thakkar @ 2024-01-04 10:39 UTC (permalink / raw)
To: Christian Couder; +Cc: git, newren, gitster, johannes.schindelin
In-Reply-To: <CAP8UFD1wMJMY6G4SaPTPwq6b9HbeXG1kB97-RRrL-KGN1wE0rg@mail.gmail.com>
On Thu Jan 4, 2024 at 3:54 PM IST, Christian Couder 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.
>
Thank you for your feedback. I will wait for Elijah's response on this and
perhaps look at other microprojects in the meantime. Although, I think I will
be able to do this with others help.
> > 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.
Appreciate your suggestions above. Will keep them in mind while sending the
patch.
> Best,
> Christian.
Thanks,
Ghanshyam
^ permalink raw reply
* Re: [GSOC][RFC] Heed core.bare from template config file when no command line override given, as a microproject.
From: Christian Couder @ 2024-01-04 10:24 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: git, newren, gitster, johannes.schindelin
In-Reply-To: <85d4e83c-b6c4-4308-ac8c-a65c911c8a95@gmail.com>
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.
> 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.
Best,
Christian.
^ permalink raw reply
* [PATCH v3] write-or-die: make GIT_FLUSH a Boolean environment variable
From: Chandra Pratap via GitGitGadget @ 2024-01-04 10:20 UTC (permalink / raw)
To: git; +Cc: Chandra Pratap, Chandra Pratap
In-Reply-To: <pull.1628.v2.git.1704268708720.gitgitgadget@gmail.com>
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,
i.e. values like 'yes', 'on', 'true' and positive numbers are
taken as "on" and values like 'no', 'off', 'false' are taken as
"off".
GIT_FLUSH can be used to force Git to use non-buffered I/O when
writing to stdout. It can only accept two values, '1' which causes
Git to flush more often and '0' which makes all output buffered.
Make GIT_FLUSH accept more values besides '0' and '1' by turning it
into a Boolean environment variable, modifying the required logic.
Update the related documentation.
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
write-or-die: make GIT_FLUSH a Boolean environment variable
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1628%2FChand-ra%2Fgit_flush-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1628/Chand-ra/git_flush-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1628
Range-diff vs v2:
1: 585c76fff68 ! 1: c98885c0ede write-or-die: make GIT_FLUSH a Boolean environment variable
@@ Documentation/git.txt: for further details.
- If this environment variable is set to "1", then commands such
+ If this Boolean environment variable is set to true, then commands such
as 'git blame' (in incremental mode), 'git rev-list', 'git log',
-- 'git check-attr' and 'git check-ignore' will
-- force a flush of the output stream after each record have been
-- flushed. If this
+ 'git check-attr' and 'git check-ignore' will
+ force a flush of the output stream after each record have been
+ flushed. If this
- variable is set to "0", the output of these commands will be done
-- using completely buffered I/O. If this environment variable is
-- not set, Git will choose buffered or record-oriented flushing
-- based on whether stdout appears to be redirected to a file or not.
-+ 'git check-attr' and 'git check-ignore' will force a flush of the output
-+ stream after each record have been flushed. If this variable is set to
-+ false, the output of these commands will be done using completely buffered
-+ I/O. If this environment variable is not set, Git will choose buffered or
-+ record-oriented flushing based on whether stdout appears to be redirected
-+ to a file or not.
-
- `GIT_TRACE`::
- Enables general trace messages, e.g. alias expansion, built-in
++ variable is set to false, the output of these commands will be done
+ using completely buffered I/O. If this environment variable is
+ not set, Git will choose buffered or record-oriented flushing
+ based on whether stdout appears to be redirected to a file or not.
## write-or-die.c ##
-@@ write-or-die.c: void maybe_flush_or_die(FILE *f, const char *desc)
+@@
+ void maybe_flush_or_die(FILE *f, const char *desc)
{
static int skip_stdout_flush = -1;
- struct stat st;
+- struct stat st;
- char *cp;
if (f == stdout) {
@@ write-or-die.c: void maybe_flush_or_die(FILE *f, const char *desc)
- if (cp)
- skip_stdout_flush = (atoi(cp) == 0);
- else if ((fstat(fileno(stdout), &st) == 0) &&
-+ if (!git_env_bool("GIT_FLUSH", -1))
-+ skip_stdout_flush = 1;
-+ else if (!fstat(fileno(stdout), &st) &&
- S_ISREG(st.st_mode))
- skip_stdout_flush = 1;
- else
+- S_ISREG(st.st_mode))
+- skip_stdout_flush = 1;
+- else
+- skip_stdout_flush = 0;
++ skip_stdout_flush = git_env_bool("GIT_FLUSH", -1);
++ if (skip_stdout_flush < 0) {
++ struct stat st;
++ if (fstat(fileno(stdout), &st))
++ skip_stdout_flush = 0;
++ else
++ skip_stdout_flush = S_ISREG(st.st_mode);
++ }
+ }
+ if (skip_stdout_flush && !ferror(f))
+ return;
Documentation/git.txt | 5 ++---
write-or-die.c | 19 ++++++++-----------
2 files changed, 10 insertions(+), 14 deletions(-)
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 2535a30194f..d06eea024f7 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -724,13 +724,12 @@ for further details.
waiting for someone with sufficient permissions to fix it.
`GIT_FLUSH`::
-// NEEDSWORK: make it into a usual Boolean environment variable
- If this environment variable is set to "1", then commands such
+ If this Boolean environment variable is set to true, then commands such
as 'git blame' (in incremental mode), 'git rev-list', 'git log',
'git check-attr' and 'git check-ignore' will
force a flush of the output stream after each record have been
flushed. If this
- variable is set to "0", the output of these commands will be done
+ variable is set to false, the output of these commands will be done
using completely buffered I/O. If this environment variable is
not set, Git will choose buffered or record-oriented flushing
based on whether stdout appears to be redirected to a file or not.
diff --git a/write-or-die.c b/write-or-die.c
index 42a2dc73cd3..39421528653 100644
--- a/write-or-die.c
+++ b/write-or-die.c
@@ -19,20 +19,17 @@
void maybe_flush_or_die(FILE *f, const char *desc)
{
static int skip_stdout_flush = -1;
- struct stat st;
- char *cp;
if (f == stdout) {
if (skip_stdout_flush < 0) {
- /* NEEDSWORK: make this a normal Boolean */
- cp = getenv("GIT_FLUSH");
- if (cp)
- skip_stdout_flush = (atoi(cp) == 0);
- else if ((fstat(fileno(stdout), &st) == 0) &&
- S_ISREG(st.st_mode))
- skip_stdout_flush = 1;
- else
- skip_stdout_flush = 0;
+ skip_stdout_flush = git_env_bool("GIT_FLUSH", -1);
+ if (skip_stdout_flush < 0) {
+ struct stat st;
+ if (fstat(fileno(stdout), &st))
+ skip_stdout_flush = 0;
+ else
+ skip_stdout_flush = S_ISREG(st.st_mode);
+ }
}
if (skip_stdout_flush && !ferror(f))
return;
base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] ci: add job performing static analysis on GitLab CI
From: Toon Claes @ 2024-01-04 9:19 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
In-Reply-To: <1536a5ef07ad24dafb5d685b40099882f89e6cc5.1703761005.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> --- a/ci/install-docker-dependencies.sh
> +++ b/ci/install-docker-dependencies.sh
> @@ -21,7 +21,7 @@ linux-musl)
> apache2 apache2-http2 apache2-proxy apache2-ssl apache2-webdav apr-util-dbd_sqlite3 \
> bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
> ;;
> -linux-*)
> +linux-*|StaticAnalysis)
> # Required so that apt doesn't wait for user input on certain packages.
> export DEBIAN_FRONTEND=noninteractive
>
> @@ -31,6 +31,11 @@ linux-*)
> perl-modules liberror-perl libauthen-sasl-perl libemail-valid-perl \
> libdbd-sqlite3-perl libio-socket-ssl-perl libnet-smtp-ssl-perl ${CC_PACKAGE:-${CC:-gcc}} \
> apache2 cvs cvsps gnupg libcgi-pm-perl subversion
> +
> + if test "$jobname" = StaticAnalysis
> + then
> + apt install -q -y coccinelle
> + fi
I was wondering why this was added, because I would assume the GitHub
Workflow needed this too. Well, it seems the "StaticAnalysis" job for
the Workflow runs ci/install-dependencies.sh instead of
ci/install-docker-dependencies.sh. The ci/install-docker-dependencies.sh
script is only used in the GitHub Workflow for the "dockerized" jobs.
They set $jobname to "linux-musl", "linux32", and "pedantic", so this
change is not affected by that.
Bottom line, changes all look good to me.
--
Toon
^ permalink raw reply
* [PATCH v2] git-prompt: stop manually parsing HEAD with unknown ref formats
From: Patrick Steinhardt @ 2024-01-04 8:21 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, SZEDER Gábor, Karthik Nayak
In-Reply-To: <cc902954f30c2faa92d1c5a4469f0dcc23e4acfe.1700825779.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3043 bytes --]
We're manually parsing the HEAD reference in git-prompt to figure out
whether it is a symbolic or direct reference. This makes it intimately
tied to the on-disk format we use to store references and will stop
working once we gain additional reference backends in the Git project.
Ideally, we would refactor the code to exclusively use plumbing tools to
read refs such that we do not have to care about the on-disk format at
all. Unfortunately though, spawning processes can be quite expensive on
some systems like Windows. As the Git prompt logic may be executed quite
frequently we try very hard to spawn as few processes as possible. This
refactoring is thus out of question for now.
Instead, condition the logic on the repository's ref format: if the repo
uses the the "files" backend we can continue to use the old logic and
read the respective files from disk directly. If it's anything else,
then we use git-symbolic-ref(1) to read the value of HEAD.
This change makes the Git prompt compatible with the upcoming "reftable"
format.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
This patch depends on "ps/refstorage-extension", which is currently at
1b2234079b (t9500: write "extensions.refstorage" into config,
2023-12-29).
contrib/completion/git-prompt.sh | 31 ++++++++++++++++++++++++-------
1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 2c030050ae..71f179cba3 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -408,7 +408,7 @@ __git_ps1 ()
local repo_info rev_parse_exit_code
repo_info="$(git rev-parse --git-dir --is-inside-git-dir \
- --is-bare-repository --is-inside-work-tree \
+ --is-bare-repository --is-inside-work-tree --show-ref-format \
--short HEAD 2>/dev/null)"
rev_parse_exit_code="$?"
@@ -421,6 +421,8 @@ __git_ps1 ()
short_sha="${repo_info##*$'\n'}"
repo_info="${repo_info%$'\n'*}"
fi
+ local ref_format="${repo_info##*$'\n'}"
+ repo_info="${repo_info%$'\n'*}"
local inside_worktree="${repo_info##*$'\n'}"
repo_info="${repo_info%$'\n'*}"
local bare_repo="${repo_info##*$'\n'}"
@@ -479,12 +481,25 @@ __git_ps1 ()
b="$(git symbolic-ref HEAD 2>/dev/null)"
else
local head=""
- if ! __git_eread "$g/HEAD" head; then
- return $exit
- fi
- # is it a symbolic ref?
- b="${head#ref: }"
- if [ "$head" = "$b" ]; then
+
+ case "$ref_format" in
+ files)
+ if ! __git_eread "$g/HEAD" head; then
+ return $exit
+ fi
+
+ if [[ $head == "ref: "* ]]; then
+ head="${head#ref: }"
+ else
+ head=""
+ fi
+ ;;
+ *)
+ head="$(git symbolic-ref HEAD 2>/dev/null)"
+ ;;
+ esac
+
+ if test -z "$head"; then
detached=yes
b="$(
case "${GIT_PS1_DESCRIBE_STYLE-}" in
@@ -502,6 +517,8 @@ __git_ps1 ()
b="$short_sha..."
b="($b)"
+ else
+ b="$head"
fi
fi
fi
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH] rebase: clarify --reschedule-failed-exec default
From: Illia Bobyr @ 2024-01-04 8:06 UTC (permalink / raw)
To: git; +Cc: Ævar Arnfjörð Bjarmason, Illia Bobyr
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
* Does extending `--empty` to git-cherry-pick make sense?
From: Brian Lyles @ 2024-01-04 6:57 UTC (permalink / raw)
To: git; +Cc: Brian Lyles
The `--empty=(keep|drop|ask)` option added to git-rebase in
e98c4269c86019bfe057a91b4305f784365b6f0b seems like it would be
applicable to git-cherry-pick (and maybe git-revert for completeness?).
While the shared sequencer code likely would already handle this fairly
well (at a cursory glance from someone with very little knowledge of C
or git's codebase, admittedly), it's only exposed via git-rebase.
The use case in which this came up involves a semi-automated workflow
for moving commits between branches. At a high level, a tool is:
- Identifying commits to be picked based on a specific trailer value
- Using git-cherry-pick with `-x` to pick those commits
- Relying on the presence of the `(cherry picked from commit ...)`
trailer to avoid re-picking commits that have already been picked
If the picked commits are then rewritten in the upstream such that there
are squashes or fixups, that trailer can end up missing in the upstream.
The next time the tool runs, it will end up trying to re-pick commits
that are already represented there. As a result, those commits become
empty upon being picked a second time and the cherry-pick ends up
breaking for the user to resolve:
$ git cherry-pick main
On branch feature
You are currently cherry-picking commit cfd7cd9.
(all conflicts fixed: run "git cherry-pick --continue")
(use "git cherry-pick --skip" to skip this patch)
(use "git cherry-pick --abort" to cancel the cherry-pick operation)
nothing to commit, working tree clean
The previous cherry-pick is now empty, possibly due to conflict resolution.
If you wish to commit it anyway, use:
git commit --allow-empty
Otherwise, please use 'git cherry-pick --skip'
I'll spare the details, but we do expect this to happen with enough
frequency that we'd really like to be able to prevent it. The
`--empty=drop` option sounds like exactly what we want here:
--empty=(drop|keep|ask)
How to handle commits that are not empty to start and are not
clean cherry-picks of any upstream commit, but which become
empty after rebasing (because they contain a subset of already
upstream changes). With drop (the default), commits that become
empty are dropped.
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.
Thanks,
-Brian Lyles
^ permalink raw reply
* Re: [PATCH] push: region_leave trace for negotiate_using_fetch
From: Junio C Hamano @ 2024-01-03 23:37 UTC (permalink / raw)
To: Sam Delmerico; +Cc: git, steadmon
In-Reply-To: <20240103224054.1940209-1-delmerico@google.com>
Sam Delmerico <delmerico@google.com> writes:
> There were two region_enter events for negotiate_using_fetch instead of
> one enter and one leave. This commit replaces the second region_enter
> event with a region_leave.
>
> Signed-off-by: Sam Delmerico <delmerico@google.com>
> ---
> fetch-pack.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Looks right, after skimming a29263cf (fetch-pack: add tracing for
negotiation rounds, 2022-08-02). Two questions and a half.
* How did you find it? Code inspection? While writing a script to
parse the output from around this area, your script noticed the
ever-increasing nesting level? Something else?
* Would it be feasible to write some tests or tools that find
similar problems (semi-)automatically?
* Is the breakage (before this patch) something easily demonstrated
in a new test in t/ somewhere? And if so, would it be worth
doing?
Thanks. Will queue.
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 31a72d43de..dba6d79944 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -2218,7 +2218,7 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips,
> the_repository, "%d",
> negotiation_round);
> }
> - trace2_region_enter("fetch-pack", "negotiate_using_fetch", the_repository);
> + trace2_region_leave("fetch-pack", "negotiate_using_fetch", the_repository);
> trace2_data_intmax("negotiate_using_fetch", the_repository,
> "total_rounds", negotiation_round);
> clear_common_flag(acked_commits);
>
> base-commit: a26002b62827b89a19b1084bd75d9371d565d03c
^ permalink raw reply
* [PATCH] push: region_leave trace for negotiate_using_fetch
From: Sam Delmerico @ 2024-01-03 22:40 UTC (permalink / raw)
To: git; +Cc: steadmon, Sam Delmerico
There were two region_enter events for negotiate_using_fetch instead of
one enter and one leave. This commit replaces the second region_enter
event with a region_leave.
Signed-off-by: Sam Delmerico <delmerico@google.com>
---
fetch-pack.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 31a72d43de..dba6d79944 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -2218,7 +2218,7 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips,
the_repository, "%d",
negotiation_round);
}
- trace2_region_enter("fetch-pack", "negotiate_using_fetch", the_repository);
+ trace2_region_leave("fetch-pack", "negotiate_using_fetch", the_repository);
trace2_data_intmax("negotiate_using_fetch", the_repository,
"total_rounds", negotiation_round);
clear_common_flag(acked_commits);
base-commit: a26002b62827b89a19b1084bd75d9371d565d03c
--
2.43.0.472.g3155946c3a-goog
^ permalink raw reply related
* Re: [PATCH V4 1/1] Replace SID with domain/username
From: Junio C Hamano @ 2024-01-03 22:22 UTC (permalink / raw)
To: Matthias Aßhauer
Cc: Sören Krecker, Johannes Schindelin, git, sunshine
In-Reply-To: <DB9P250MB0692C8B4D93ED92FEE680AA9A560A@DB9P250MB0692.EURP250.PROD.OUTLOOK.COM>
Matthias Aßhauer <mha1993@live.de> writes:
> This patch only changes the output of our error message, though.
> It does not change what ownership information we actually compare.
> So if we had a hypothetical user Bob that was part of the domain
> example.com (SID S-1-5-21-100000001-1000000001-10000001-1001) and
> had been moved over from the example.org domain (old SID S-1-5-21-
> 2000000002-2000000002-20000002-2002) and we would detect a repository
> owned by bobs old SID, we would now lookup the old SID, find it
> attached to a user named example.com\Bob, look up Bobs current SID,
> find it belongs to a user named example.com\Bob and print a confusing
> error message.
Yup, that is exactly the kind of breakage I was worried about.
Perhaps we should do something along the lines of ...
- The erroring out should be done purely by SID comparison, as that
is what we have been doing to protect the users.
- When creating a message, use LookupAccountSidA() to come up with
a pair of domain\user strings for the directory and the process
to be used in the error message:
- If they are different (which is expected to be the normal
case), we just use the pair of strings.
- If they are the same, show old and new SID in stringified form
(hopefully different SIDs would strigify to different
strings?), and optionally we give the domain\user string next
to it.
... then? Then we would emit an error message (in the best case)
'directory' is owned by:
'bob@example.org'
but the current user is:
'charlie@example.com'
and in a bad case we would instead see something like:
'directory' is owned by:
SID S-1-5-21-100000001-1000000001-10000001-1001 ('bob@example.org')
but the current user is:
SID S-1-5-21-200000002-2000000002-20000002-2002 ('bob@example.org')
which may still be serviceable. I dunno.
^ permalink raw reply
* Re: Concurrent fetch commands
From: Junio C Hamano @ 2024-01-03 22:10 UTC (permalink / raw)
To: Taylor Blau; +Cc: Patrick Steinhardt, Oswald Buddenhagen, Stefan Haller, git
In-Reply-To: <ZZWOBObBmLW9Nid6@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
> ... I suppose the answer is that they expect
> concurrent fetches to be tolerated, but that the contents of FETCH_HEAD
> (and of course the remote references) are consistent at the end of all
> of the fetches.
What does it mean to be "consistent" in this case, though? For the
controlled form of multiple fetches performed by "git fetch --all",
the answer is probably "as if we fetched sequentially from these
remotes, one by one, and concatenated what these individual fetch
invocations left in FETCH_HEAD". But for an uncontrolled background
fetch IDE and others perform behind user's back, it is unclear what
it means, or for that matter, it is dubious if there is a reasonable
definition for the word.
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.
Nobody brought up the latter so far on this discussion thread, but
mucking with the remote-tracking branches behind user's back means
completely breaking the end-user expectation that --force-with-lease
would do something useful even when it is not given the commit the
user expects to see at the remote. Perhaps those third-party tools
that want to run "git fetch" in the background can learn from how
"prefetch" task works to avoid the breakage they are inflicting on
their users?
^ 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