* 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: [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: 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
* [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: [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
* 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
* [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] 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
* 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 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: [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] 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] 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: 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: Taylor Blau @ 2024-01-04 18:27 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jonathan Tan, Chen Xuewei via GitGitGadget, git, Chen Xuewei
In-Reply-To: <xmqq7ckp7ysl.fsf@gitster.g>
On Thu, Jan 04, 2024 at 10:12:42AM -0800, Junio C Hamano wrote:
> Jonathan and Taylor, isn't this what you two were working together
> on? How would we want to proceed?
They are indeed similar. I think that Jonathan and my series would
supersede this effort.
But I would appreciate if Chen took a look at the approach in that
series to make sure that we're all on the same page and that Jonathan
and I aren't missing anything.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] fetch: add new config option fetch.all
From: Junio C Hamano @ 2024-01-04 18:29 UTC (permalink / raw)
To: Taylor Blau; +Cc: Tamino Bauknecht, git
In-Reply-To: <ZZbr4yAJe0dnHRcO@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
>> +cat > expect << EOF
>
> This should be `cat >expect <<-\EOF` (without the space between the
> redirect and heredoc, as well as indicating that the heredoc does not
> need any shell expansions).
I noticed the same but I refrained from commenting on them ;-)
The original already is littered with style violations of this kind
(aka "old style"). If we were writing the tests in this file today,
we would also move the preparation of "expect" inside the
test_expect_success block that uses the expected output file.
If we do a style fix of the existing tests in this file as a
preliminary clean-up before the main patch that adds fetch.all and
its tests, that would be great. But for an initial step, I think it
is OK to have a single step patch that imitates the existing ones.
Perhaps after the initial review, it can become a two-patch series
to do so.
Thanks.
^ permalink raw reply
* Re: [PATCH] fetch: add new config option fetch.all
From: Tamino Bauknecht @ 2024-01-04 18:32 UTC (permalink / raw)
To: Taylor Blau; +Cc: git
In-Reply-To: <ZZbr4yAJe0dnHRcO@nand.local>
Forgot to put the mailing list into the CC - sorry for the duplicate
mail, Taylor.
On 1/4/24 18:33, Taylor Blau wrote:
> Instead of "can be overridden ...", how about:
>
> This behavior can be overridden by explicitly specifying one or more
> remote(s) to fetch from.
Sure, although I feel a bit conflicted since "git fetch one two" still
doesn't work and would require "--multiple". But probably still better
than my wording.
> I don't feel particularly strongly about whether or not you reorganize
> these if-statements, but we should change "argc == 0" to "!argc", which
> matches the conventions of the rest of the project.
Are you sure that I shouldn't use "argc == 0" here instead since it's
also used in the next "else if" condition? Or is the goal to gradually
transition to "!argc" in the entire code base?
I agree with keeping the diff minimal, will change that to your suggestion.
> This should be `cat >expect <<-\EOF` (without the space between the
> redirect and heredoc, as well as indicating that the heredoc does not
> need any shell expansions).
Will do so, thanks.
I tried to match the existing test cases as closely as possible, but if
they are outdated, it might be better to use the more recent structure.
> It looks like these tests match the existing style of the test suite,
> but they are somewhat out of date with respect to our more modern
> standards. I would probably write this like:
>
> test_expect_success 'git fetch --all (works with fetch.all = true)' '
> git clone one test10 &&
> test_config -C test10 fetch.all true &&
> for r in one two three
> do
> git -C test10 remote add $r ../$r || return 1
> done &&
> git -C test10 fetch --all &&
> git -C test10 branch -r >actual &&
> test_cmp expect actual
> '
I think I'd prefer having the "actual" (and maybe also "expected")
output in the repository so that it won't be overwritten by the next
test case.
> I suspect that you could go further with a "setup" function that gives
> you a fresh clone (with fetch.all set to a specified value), and then
> test test would continue on from the line "git fetch one". But I think
> it's worth not getting too carried away with refactoring these tests
> ;-).
I think a setup function makes sense to at least remove the clutter from
adding the remotes. Although I think that setting the value of fetch.all
in the test case will make it easier to parse the test code - that way
you don't have to look up different functions to understand the test.
Thanks for the great review, will send an updated patch later.
^ permalink raw reply
* Re: [PATCH v3] write-or-die: make GIT_FLUSH a Boolean environment variable
From: Junio C Hamano @ 2024-01-04 18:35 UTC (permalink / raw)
To: Chandra Pratap via GitGitGadget; +Cc: git, Chandra Pratap, Chandra Pratap
In-Reply-To: <pull.1628.v3.git.1704363617842.gitgitgadget@gmail.com>
"Chandra Pratap via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Chandra Pratap <chandrapratap3519@gmail.com>
>
> Among Git's environment variable, the ones marked as "Boolean"
> accept values in a way similar to Boolean configuration variables,
With "Among" you probably mean that there are many and some of them
are "marked as 'Boolean'", so I'd do "variable" -> "variables" while
queuing.
Other than that, looks great. Will queue. Thanks.
^ permalink raw reply
* Re: [PATCH] fetch: add new config option fetch.all
From: Taylor Blau @ 2024-01-04 19:13 UTC (permalink / raw)
To: Tamino Bauknecht; +Cc: git
In-Reply-To: <88407aeb-bff7-4899-af7b-e7cb671d991a@tb6.eu>
On Thu, Jan 04, 2024 at 07:32:03PM +0100, Tamino Bauknecht wrote:
> > I don't feel particularly strongly about whether or not you reorganize
> > these if-statements, but we should change "argc == 0" to "!argc", which
> > matches the conventions of the rest of the project.
>
> Are you sure that I shouldn't use "argc == 0" here instead since it's also
> used in the next "else if" condition? Or is the goal to gradually transition
> to "!argc" in the entire code base?
> I agree with keeping the diff minimal, will change that to your suggestion.
See Documentation/CodingGuidelines.txt for more information about the
Git project's style guidelines, but in short: yes, any x == 0 should be
replaced with !x instead within if-statements.
> > This should be `cat >expect <<-\EOF` (without the space between the
> > redirect and heredoc, as well as indicating that the heredoc does not
> > need any shell expansions).
>
> Will do so, thanks.
> I tried to match the existing test cases as closely as possible, but if they
> are outdated, it might be better to use the more recent structure.
Junio notes in the thread further up that it is OK to imitate the
existing style of tests. I don't disagree, but I personally think it's
OK to introduce new tests in a better style without touching the
existing ones in the same patch (or requiring a preparatory patch to the
same effect).
> > It looks like these tests match the existing style of the test suite,
> > but they are somewhat out of date with respect to our more modern
> > standards. I would probably write this like:
> >
> > test_expect_success 'git fetch --all (works with fetch.all = true)' '
> > git clone one test10 &&
> > test_config -C test10 fetch.all true &&
> > for r in one two three
> > do
> > git -C test10 remote add $r ../$r || return 1
> > done &&
> > git -C test10 fetch --all &&
> > git -C test10 branch -r >actual &&
> > test_cmp expect actual
> > '
>
> I think I'd prefer having the "actual" (and maybe also "expected") output in
> the repository so that it won't be overwritten by the next test case.
Very reasonable.
> Thanks for the great review, will send an updated patch later.
Thanks for working on this!
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] rebase: clarify --reschedule-failed-exec default
From: Taylor Blau @ 2024-01-04 19:20 UTC (permalink / raw)
To: Illia Bobyr; +Cc: git, Ævar Arnfjörð Bjarmason
In-Reply-To: <20240104080631.3666413-1-illia.bobyr@gmail.com>
On Thu, Jan 04, 2024 at 12:06:31AM -0800, Illia Bobyr wrote:
> Documentation should mention the default behavior.
>
> It is better to explain the persistent nature of the
> --reschedule-failed-exec flag from the user standpoint, rather than from
> the implementation standpoint.
The first paragraph looks good, and I think your wording is an
improvement over what's already there (though of course this is
subjective, and YMMV).
> +Recording this option for the whole rebase is a convenience feature. Otherwise
> +an explicit `--no-reschedule-failed-exec` at the start would be overridden by
> +the presence of a `rebase.rescheduleFailedExec=true` configuration when `git
> +rebase --continue` is invoked. Currently, you can not, pass
> +`--[no-]reschedule-failed-exec` to `git rebase --continue`.
The last sentence was a bit confusing to me. I assume you meant
Currently, you cannot pass `--[no-]reschedule-failed-exec` [...]
without the comma between "pass" and "`--[no]reschedule-failed-exect`",
and replacing "can not" with "cannot".
Thanks,
Taylor
^ permalink raw reply
* [PATCH v5 0/1] Replace SID with domain/username on Windows
From: Sören Krecker @ 2024-01-04 19:22 UTC (permalink / raw)
To: git; +Cc: sunshine, Sören Krecker
In-Reply-To: <DB9P250MB0692C8B4D93ED92FEE680AA9A560A@DB9P250MB0692.EURP250.PROD.OUTLOOK.COM>
Hi everyone,
I change the error message. I Hope that it is now better for every one.
Thanks
Sören Krecker (1):
Adds domain/username to error message
compat/mingw.c | 64 ++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 51 insertions(+), 13 deletions(-)
base-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa
--
2.39.2
^ permalink raw reply
* [PATCH v5 1/1] Adds domain/username to error message
From: Sören Krecker @ 2024-01-04 19:22 UTC (permalink / raw)
To: git; +Cc: sunshine, Sören Krecker
In-Reply-To: <20240104192202.2124-1-soekkle@freenet.de>
Adds domain/username in error message, if owner sid of repository and
user sid are not equal on windows systems.
Old Prompted error message:
'''
fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
'C:/Users/test/source/repos/git' is owned by:
'S-1-5-21-571067702-4104414259-3379520149-500'
but the current user is:
'S-1-5-21-571067702-4104414259-3379520149-1001'
To add an exception for this directory, call:
git config --global --add safe.directory C:/Users/test/source/repos/git
'''
New prompted error massage:
'''
fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
'C:/Users/test/source/repos/git' is owned by:
'DESKTOP-L78JVA6/Administrator' (S-1-5-21-571067702-4104414259-3379520149-500)
but the current user is:
'DESKTOP-L78JVA6/test' (S-1-5-21-571067702-4104414259-3379520149-1001)
To add an exception for this directory, call:
git config --global --add safe.directory C:/Users/test/source/repos/git
'''
Signed-off-by: Sören Krecker <soekkle@freenet.de>
---
compat/mingw.c | 64 ++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 51 insertions(+), 13 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index 42053c1f65..6240387205 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2684,6 +2684,26 @@ static PSID get_current_user_sid(void)
return result;
}
+static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
+{
+ SID_NAME_USE pe_use;
+ DWORD len_user = 0, len_domain = 0;
+ BOOL translate_sid_to_user;
+
+ /* returns only FALSE, because the string pointers are NULL*/
+ LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
+ &pe_use);
+ /*Alloc needed space of the strings*/
+ ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user);
+ translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,
+ *str, &len_domain, &pe_use);
+ if (translate_sid_to_user == FALSE)
+ FREE_AND_NULL(*str);
+ else
+ (*str)[len_domain] = '/';
+ return translate_sid_to_user;
+}
+
static int acls_supported(const char *path)
{
size_t offset = offset_1st_component(path);
@@ -2765,27 +2785,45 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
strbuf_addf(report, "'%s' is on a file system that does "
"not record ownership\n", path);
} else if (report) {
- LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
+ LPSTR str1, str2, str3, str4, to_free1 = NULL, to_free3 = NULL, to_local_free2=NULL, to_local_free4=NULL;
- if (ConvertSidToStringSidA(sid, &str1))
+ if (user_sid_to_user_name(sid, &str1))
to_free1 = str1;
else
str1 = "(inconvertible)";
-
- if (!current_user_sid)
- str2 = "(none)";
- else if (!IsValidSid(current_user_sid))
- str2 = "(invalid)";
- else if (ConvertSidToStringSidA(current_user_sid, &str2))
- to_free2 = str2;
+ if (ConvertSidToStringSidA(sid, &str2))
+ to_local_free2 = str2;
else
str2 = "(inconvertible)";
+
+ if (!current_user_sid) {
+ str3 = "(none)";
+ str4 = "(none)";
+ }
+ else if (!IsValidSid(current_user_sid)) {
+ str3 = "(invalid)";
+ str4 = "(invalid)";
+ } else {
+ if (user_sid_to_user_name(current_user_sid,
+ &str3))
+ to_free3 = str3;
+ else
+ str3 = "(inconvertible)";
+ if (ConvertSidToStringSidA(current_user_sid,
+ &str4))
+ to_local_free4 = str4;
+ else
+ str4 = "(inconvertible)";
+ }
strbuf_addf(report,
"'%s' is owned by:\n"
- "\t'%s'\nbut the current user is:\n"
- "\t'%s'\n", path, str1, str2);
- LocalFree(to_free1);
- LocalFree(to_free2);
+ "\t'%s' (%s)\nbut the current user is:\n"
+ "\t'%s' (%s)\n",
+ path, str1, str2, str3, str4);
+ free(to_free1);
+ LocalFree(to_local_free2);
+ free(to_free3);
+ LocalFree(to_local_free4);
}
}
--
2.39.2
^ permalink raw reply related
* Re: Does extending `--empty` to git-cherry-pick make sense?
From: Taylor Blau @ 2024-01-04 19:33 UTC (permalink / raw)
To: Brian Lyles; +Cc: Elijah Newren, git
In-Reply-To: <CAHPHrSevBdQF0BisR8VK=jM=wj1dTUYEVrv31gLerAzL9=Cd8Q@mail.gmail.com>
[+cc Elijah]
On Thu, Jan 04, 2024 at 12:57:18AM -0600, Brian Lyles wrote:
> Is there any real barrier to exposing that option to git-cherry-pick as
> well? Was this an oversight, or intentionally left out? The
> corresponding commit message doesn't seem to indicate any specific
> reason for limiting it to git-rebase.
I am not nearly as familiar with this code as Elijah is, but this
certainly appears possible by setting the `drop_redundant_commits` and
`keep_redundant_commits` flags in the replay_opts struct.
I don't see any fundamental reason why cherry-pick shouldn't have the
same functionality.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v5 1/1] Adds domain/username to error message
From: Junio C Hamano @ 2024-01-04 20:09 UTC (permalink / raw)
To: Sören Krecker, Johannes Schindelin; +Cc: git, sunshine
In-Reply-To: <20240104192202.2124-2-soekkle@freenet.de>
Sören Krecker <soekkle@freenet.de> writes:
> Subject: Re: [PATCH v5 1/1] Adds domain/username to error message
Looking at past commits that worked on the area this patch touches,
namely, 7c83470e (mingw: be more informative when ownership check
fails on FAT32, 2022-08-08) and e883e04b (mingw: provide details
about unsafe directories' ownership, 2022-08-08), I would retitle
the commit perhaps like so:
Subject: [PATCH v5] mingw: give more details about unsafe directory's ownership
if I were doing this patch.
> Adds domain/username in error message, if owner sid of repository and
"Adds" -> "Add".
> user sid are not equal on windows systems.
>
> Old Prompted error message:
> '''
> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
> 'C:/Users/test/source/repos/git' is owned by:
> 'S-1-5-21-571067702-4104414259-3379520149-500'
> but the current user is:
> 'S-1-5-21-571067702-4104414259-3379520149-1001'
> To add an exception for this directory, call:
>
> git config --global --add safe.directory C:/Users/test/source/repos/git
> '''
>
> New prompted error massage:
"massage" -> "message".
I probably would drop two "prompted" from the above, too, if I were
doing this patch.
Thanks for working on making this error message more readable. I'll
queue it when I see an Ack from Dscho.
> '''
> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
> 'C:/Users/test/source/repos/git' is owned by:
> 'DESKTOP-L78JVA6/Administrator' (S-1-5-21-571067702-4104414259-3379520149-500)
> but the current user is:
> 'DESKTOP-L78JVA6/test' (S-1-5-21-571067702-4104414259-3379520149-1001)
> To add an exception for this directory, call:
>
> git config --global --add safe.directory C:/Users/test/source/repos/git
> '''
>
> Signed-off-by: Sören Krecker <soekkle@freenet.de>
> ---
> compat/mingw.c | 64 ++++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 51 insertions(+), 13 deletions(-)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 42053c1f65..6240387205 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -2684,6 +2684,26 @@ static PSID get_current_user_sid(void)
> return result;
> }
>
> +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
> +{
> + SID_NAME_USE pe_use;
> + DWORD len_user = 0, len_domain = 0;
> + BOOL translate_sid_to_user;
> +
> + /* returns only FALSE, because the string pointers are NULL*/
> + LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
> + &pe_use);
> + /*Alloc needed space of the strings*/
> + ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user);
> + translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,
> + *str, &len_domain, &pe_use);
> + if (translate_sid_to_user == FALSE)
> + FREE_AND_NULL(*str);
> + else
> + (*str)[len_domain] = '/';
> + return translate_sid_to_user;
> +}
> +
> static int acls_supported(const char *path)
> {
> size_t offset = offset_1st_component(path);
> @@ -2765,27 +2785,45 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
> strbuf_addf(report, "'%s' is on a file system that does "
> "not record ownership\n", path);
> } else if (report) {
> - LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
> + LPSTR str1, str2, str3, str4, to_free1 = NULL, to_free3 = NULL, to_local_free2=NULL, to_local_free4=NULL;
>
> - if (ConvertSidToStringSidA(sid, &str1))
> + if (user_sid_to_user_name(sid, &str1))
> to_free1 = str1;
> else
> str1 = "(inconvertible)";
> -
> - if (!current_user_sid)
> - str2 = "(none)";
> - else if (!IsValidSid(current_user_sid))
> - str2 = "(invalid)";
> - else if (ConvertSidToStringSidA(current_user_sid, &str2))
> - to_free2 = str2;
> + if (ConvertSidToStringSidA(sid, &str2))
> + to_local_free2 = str2;
> else
> str2 = "(inconvertible)";
> +
> + if (!current_user_sid) {
> + str3 = "(none)";
> + str4 = "(none)";
> + }
> + else if (!IsValidSid(current_user_sid)) {
> + str3 = "(invalid)";
> + str4 = "(invalid)";
> + } else {
> + if (user_sid_to_user_name(current_user_sid,
> + &str3))
> + to_free3 = str3;
> + else
> + str3 = "(inconvertible)";
> + if (ConvertSidToStringSidA(current_user_sid,
> + &str4))
> + to_local_free4 = str4;
> + else
> + str4 = "(inconvertible)";
> + }
> strbuf_addf(report,
> "'%s' is owned by:\n"
> - "\t'%s'\nbut the current user is:\n"
> - "\t'%s'\n", path, str1, str2);
> - LocalFree(to_free1);
> - LocalFree(to_free2);
> + "\t'%s' (%s)\nbut the current user is:\n"
> + "\t'%s' (%s)\n",
> + path, str1, str2, str3, str4);
> + free(to_free1);
> + LocalFree(to_local_free2);
> + free(to_free3);
> + LocalFree(to_local_free4);
> }
> }
^ permalink raw reply
* Re: [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
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