* Re: [PATCH 2/7] repack: use die_for_incompatible_opt3() for -A/-k/--cruft
From: Taylor Blau @ 2023-12-06 19:18 UTC (permalink / raw)
To: René Scharfe; +Cc: git
In-Reply-To: <20231206115215.94467-3-l.s.r@web.de>
On Wed, Dec 06, 2023 at 12:51:56PM +0100, René Scharfe wrote:
> diff --git a/builtin/repack.c b/builtin/repack.c
> index edaee4dbec..c54777bbe5 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -1203,19 +1203,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> if (delete_redundant && repository_format_precious_objects)
> die(_("cannot delete packs in a precious-objects repo"));
>
> - if (keep_unreachable &&
> - (unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE)))
> - die(_("options '%s' and '%s' cannot be used together"), "--keep-unreachable", "-A");
> + die_for_incompatible_opt3(unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE), "-A",
> + keep_unreachable, "-k/--keep-unreachable",
> + pack_everything & PACK_CRUFT, "--cruft");
Oof, thanks for cleaning this one up.
I had to read this hunk a couple of times to convince myself that it was
doing the right thing, but I believe it is.
> - if (pack_everything & PACK_CRUFT) {
> + if (pack_everything & PACK_CRUFT)
> pack_everything |= ALL_INTO_ONE;
And adding the ALL_INTO_ONE bit here does not effect either of the below
two checks, so moving them up is fine, too.
> - if (unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE))
> - die(_("options '%s' and '%s' cannot be used together"), "--cruft", "-A");
> - if (keep_unreachable)
> - die(_("options '%s' and '%s' cannot be used together"), "--cruft", "-k");
> - }
Thanks,
Taylor
^ permalink raw reply
* Re: git switch has fatal dependency on default fetch config
From: Jeff King @ 2023-12-06 19:11 UTC (permalink / raw)
To: Jeremiah Steele (Jerry); +Cc: git
In-Reply-To: <634F40AF-25F8-4FE6-BDE3-08798E699A9E@dubsado.com>
On Tue, Dec 05, 2023 at 07:41:28PM -0800, Jeremiah Steele (Jerry) wrote:
> Changing the default fetch refspec for a remote breaks git switch:
>
> % git branch -r
> origin/HEAD -> origin/master
> origin/feature
> origin/master
> % git remote set-branches origin master
> % git switch -c feature --track origin/feature
> fatal: cannot set up tracking information; starting point 'origin/feature' is not a branch
> % git remote set-branches --add origin feature
> % git switch -c feature --track origin/feature
> branch 'feature' set up to track 'origin/feature'.
> Switched to a new branch 'feature'
>
> It seems like I should be able to fetch a remote branch and track it
> without having to monkey around with my default fetch config. Is there
> a reason git switch has a hard dependency on the default remote fetch
> refspec configuration?
I think it's required by the form of the tracking config, which is
defined by a named remote and a remote branch, like:
[branch "foo"]
remote = origin
merge = refs/heads/foo
You explicitly asked to track "origin/feature", which means that Git has
to be able to turn that into config as above. It can handle two cases:
1. It's a local branch in refs/heads/, in which case the remote is "."
and the "merge" field is the name of the branch.
2. It's a ref that can be found by reversing a fetch refspec. With the
default remote.origin.fetch refspec of refs/heads/*:refs/remotes/origin/*,
we know that "refs/remotes/origin/feature" comes from "refs/heads/feature"
on the "origin" remote.
But since you used "remote set-branches" to limit that refspec, we
can't do that same reversal. And "origin/feature", while we do
still have it as a ref, does not correspond to any remote ref we'd
fetch. So we don't know how to set up the tracking config.
The notion of "tracking" really came about as defining what "git pull"
does. And without a remote that fetches, "git pull" does not really make
much sense.
I'd guess that you are more interested in being able to just use
@{upstream}, especially as the default for things like rebase, etc. And
that could work without being able to actually fetch the ref. But those
two things are intertwined in Git. You obviously can still base a branch
off of "origin/feature", but you'll have to specify it manually when
doing rebase, etc.
-Peff
^ permalink raw reply
* Re: [PATCH 3/7] revision: use die_for_incompatible_opt3() for --graph/--reverse/--walk-reflogs
From: René Scharfe @ 2023-12-06 17:29 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git
In-Reply-To: <CAPig+cQBKk4PpCjT1RN+puS5vSo8_jZ3wONAgGhRvduw3jZT8A@mail.gmail.com>
Am 06.12.23 um 18:21 schrieb Eric Sunshine:
> On Wed, Dec 6, 2023 at 6:53 AM René Scharfe <l.s.r@web.de> wrote:
>> The revision options --reverse is incompatibel with --walk-reflogs and
>
> s/incompatibel/incompatible/
And s/options/option/, *sigh*.
>
>> --graph is incompatible with both --reverse and --walk-reflogs. So they
>> are all incompatible with each other.
>>
>> Use the function for checking three mutually incompatible options,
>> die_for_incompatible_opt3(), to perform this check in one place and
>> without repetition. This is shorter and clearer.
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
^ permalink raw reply
* Re: [PATCH 3/7] revision: use die_for_incompatible_opt3() for --graph/--reverse/--walk-reflogs
From: Eric Sunshine @ 2023-12-06 17:21 UTC (permalink / raw)
To: René Scharfe; +Cc: git
In-Reply-To: <20231206115215.94467-4-l.s.r@web.de>
On Wed, Dec 6, 2023 at 6:53 AM René Scharfe <l.s.r@web.de> wrote:
> The revision options --reverse is incompatibel with --walk-reflogs and
s/incompatibel/incompatible/
> --graph is incompatible with both --reverse and --walk-reflogs. So they
> are all incompatible with each other.
>
> Use the function for checking three mutually incompatible options,
> die_for_incompatible_opt3(), to perform this check in one place and
> without repetition. This is shorter and clearer.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
^ permalink raw reply
* Re: [PATCH 4/7] revision, rev-parse: factorize incompatibility messages about --exclude-hidden
From: René Scharfe @ 2023-12-06 17:07 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <ZXCHj3hIpQb900WX@tanuki>
Am 06.12.23 um 15:39 schrieb Patrick Steinhardt:
> On Wed, Dec 06, 2023 at 03:21:15PM +0100, René Scharfe wrote:
>> Am 06.12.23 um 14:08 schrieb Patrick Steinhardt:
>>> On Wed, Dec 06, 2023 at 12:51:58PM +0100, René Scharfe wrote:
>>>> Use the standard parameterized message for reporting incompatible
>>>> options to report options that are not accepted in combination with
>>>> --exclude-hidden. This reduces the number of strings to translate and
>>>> makes the UI a bit more consistent.
>>>>
>>>> Signed-off-by: René Scharfe <l.s.r@web.de>
>>>> ---
>>>> builtin/rev-parse.c | 9 ++++++---
>>>> revision.c | 18 ++++++++++++------
>>>> t/t6018-rev-list-glob.sh | 6 ++----
>>>> t/t6021-rev-list-exclude-hidden.sh | 4 ++--
>>>> 4 files changed, 22 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
>>>> index fde8861ca4..917f122440 100644
>>>> --- a/builtin/rev-parse.c
>>>> +++ b/builtin/rev-parse.c
>>>> @@ -893,13 +893,15 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>>>> }
>>>> if (opt_with_value(arg, "--branches", &arg)) {
>>>> if (ref_excludes.hidden_refs_configured)
>>>> - return error(_("--exclude-hidden cannot be used together with --branches"));
>>>> + return error(_("options '%s' and '%s' cannot be used together"),
>>>> + "--exclude-hidden", "--branches");
>>>
>>> The repetitive nature of this patch and subsequent ones made me wonder
>>> whether it would be useful to have a function similar to the
>>> `die_for_incompatible_*()` helper that knows to format this error
>>> correctly.
>>
>> I wondered the same and experimented with a die_for_incompatible_opt2().
>> It would allow the compiler to detect typos.
>>
>> Passing in the conditions as parameters is a bit tedious and unlike its
>> for its higher-numbered siblings there is not much to win by doing that
>> instead of using an if statement or two nested ones. We could pass in
>> 1 if we want to integrate that function into an if cascade like above,
>> but it would look a bit silly. And here we'd need a non-fatal version
>> anyway.
>
> Maybe the easiest solution would be to have `error_incompatible_usage()`
> that simply wraps `error()`.
Yes, but having two variants (die_ and error_) is unfortunate.
> You'd pass in the incompatible params and
> it makes sure to format them accordingly. It could either accept two
> args or even be a vararg function with sentinel `NULL`.
Tempting, but passing the conditions separately is actually useful to
improve the shown message when there are more than two options.
> It's not perfect
> of course, but would at least ensure that we can easily convert things
> over time without having to duplicate the exact message everywhere.
Maybe the simplest option would be to use a macro, e.g.
#define INCOMPATIBLE_OPTIONS_MESSAGE \
_("options '%s' and '%s' cannot be used together")
It could be used with both error() and die(), and the compiler would
still ensure that two strings are passed along with it, but I don't know
how to encode that requirement in the macro name somehow to make it
self-documenting. Perhaps by getting the number two in there?
> I don't think it's a problem to not convert everything in one go. The
> current series is a good step in the right direction, and any additional
> instances that were missed can be fixed in follow-ups.
Right; whatever we do, we can (and should) do it step by step.
René
^ permalink raw reply
* CSI Divisions
From: ronnypartin953 @ 2023-12-06 17:01 UTC (permalink / raw)
To: git
Hi,
We are an estimating organization and we provide Civil, Electrical, Plumbing, HVAC, Instrumentation & Controls, Fire Alarm & Detection, CCTV, Communication Systems, and Health Safety & Environment estimates and Take-Off.
We claim 99% accuracy on our service. With a refund policy. Let me know if you want to see our sample work or if you have any query. Just reply to this email Thanks.
Regards,
Ronny Partin
Marketing Manager
Estimating Solutions, LLC
^ permalink raw reply
* Re: [PATCH 4/7] revision, rev-parse: factorize incompatibility messages about --exclude-hidden
From: Patrick Steinhardt @ 2023-12-06 14:39 UTC (permalink / raw)
To: René Scharfe; +Cc: git
In-Reply-To: <389cd7b3-2350-4dc6-b282-e9d6e25fa68c@web.de>
[-- Attachment #1: Type: text/plain, Size: 3187 bytes --]
On Wed, Dec 06, 2023 at 03:21:15PM +0100, René Scharfe wrote:
> Am 06.12.23 um 14:08 schrieb Patrick Steinhardt:
> > On Wed, Dec 06, 2023 at 12:51:58PM +0100, René Scharfe wrote:
> >> Use the standard parameterized message for reporting incompatible
> >> options to report options that are not accepted in combination with
> >> --exclude-hidden. This reduces the number of strings to translate and
> >> makes the UI a bit more consistent.
> >>
> >> Signed-off-by: René Scharfe <l.s.r@web.de>
> >> ---
> >> builtin/rev-parse.c | 9 ++++++---
> >> revision.c | 18 ++++++++++++------
> >> t/t6018-rev-list-glob.sh | 6 ++----
> >> t/t6021-rev-list-exclude-hidden.sh | 4 ++--
> >> 4 files changed, 22 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> >> index fde8861ca4..917f122440 100644
> >> --- a/builtin/rev-parse.c
> >> +++ b/builtin/rev-parse.c
> >> @@ -893,13 +893,15 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> >> }
> >> if (opt_with_value(arg, "--branches", &arg)) {
> >> if (ref_excludes.hidden_refs_configured)
> >> - return error(_("--exclude-hidden cannot be used together with --branches"));
> >> + return error(_("options '%s' and '%s' cannot be used together"),
> >> + "--exclude-hidden", "--branches");
> >
> > The repetitive nature of this patch and subsequent ones made me wonder
> > whether it would be useful to have a function similar to the
> > `die_for_incompatible_*()` helper that knows to format this error
> > correctly.
>
> I wondered the same and experimented with a die_for_incompatible_opt2().
> It would allow the compiler to detect typos.
>
> Passing in the conditions as parameters is a bit tedious and unlike its
> for its higher-numbered siblings there is not much to win by doing that
> instead of using an if statement or two nested ones. We could pass in
> 1 if we want to integrate that function into an if cascade like above,
> but it would look a bit silly. And here we'd need a non-fatal version
> anyway.
Maybe the easiest solution would be to have `error_incompatible_usage()`
that simply wraps `error()`. You'd pass in the incompatible params and
it makes sure to format them accordingly. It could either accept two
args or even be a vararg function with sentinel `NULL`. It's not perfect
of course, but would at least ensure that we can easily convert things
over time without having to duplicate the exact message everywhere.
But ultimately I do not mind the current duplication all that much, the
patch series looks good to me even without such a new helper.
> Perhaps a build step that lists all new translatable strings would help?
> Nah, that would require building each commit.
>
> A LLM-based tool to find translatable strings with the same meaning?
> Don't know how difficult that would be.
I don't think it's a problem to not convert everything in one go. The
current series is a good step in the right direction, and any additional
instances that were missed can be fixed in follow-ups.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 4/7] revision, rev-parse: factorize incompatibility messages about --exclude-hidden
From: René Scharfe @ 2023-12-06 14:21 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <ZXByOXWo6cIy71s2@tanuki>
Am 06.12.23 um 14:08 schrieb Patrick Steinhardt:
> On Wed, Dec 06, 2023 at 12:51:58PM +0100, René Scharfe wrote:
>> Use the standard parameterized message for reporting incompatible
>> options to report options that are not accepted in combination with
>> --exclude-hidden. This reduces the number of strings to translate and
>> makes the UI a bit more consistent.
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>> builtin/rev-parse.c | 9 ++++++---
>> revision.c | 18 ++++++++++++------
>> t/t6018-rev-list-glob.sh | 6 ++----
>> t/t6021-rev-list-exclude-hidden.sh | 4 ++--
>> 4 files changed, 22 insertions(+), 15 deletions(-)
>>
>> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
>> index fde8861ca4..917f122440 100644
>> --- a/builtin/rev-parse.c
>> +++ b/builtin/rev-parse.c
>> @@ -893,13 +893,15 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>> }
>> if (opt_with_value(arg, "--branches", &arg)) {
>> if (ref_excludes.hidden_refs_configured)
>> - return error(_("--exclude-hidden cannot be used together with --branches"));
>> + return error(_("options '%s' and '%s' cannot be used together"),
>> + "--exclude-hidden", "--branches");
>
> The repetitive nature of this patch and subsequent ones made me wonder
> whether it would be useful to have a function similar to the
> `die_for_incompatible_*()` helper that knows to format this error
> correctly.
I wondered the same and experimented with a die_for_incompatible_opt2().
It would allow the compiler to detect typos.
Passing in the conditions as parameters is a bit tedious and unlike its
for its higher-numbered siblings there is not much to win by doing that
instead of using an if statement or two nested ones. We could pass in
1 if we want to integrate that function into an if cascade like above,
but it would look a bit silly. And here we'd need a non-fatal version
anyway.
Perhaps a build step that lists all new translatable strings would help?
Nah, that would require building each commit.
A LLM-based tool to find translatable strings with the same meaning?
Don't know how difficult that would be.
So I feel the same, but don't have a solution that would justify the
churn of replacing the duplicate strings with function calls. :-/
René
^ permalink raw reply
* Re: [PATCH 3/7] revision: use die_for_incompatible_opt3() for --graph/--reverse/--walk-reflogs
From: René Scharfe @ 2023-12-06 13:47 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <ZXByNGuQTaOQ3sKW@tanuki>
Am 06.12.23 um 14:08 schrieb Patrick Steinhardt:
> On Wed, Dec 06, 2023 at 12:51:57PM +0100, René Scharfe wrote:
>> The revision options --reverse is incompatibel with --walk-reflogs and
>> --graph is incompatible with both --reverse and --walk-reflogs. So they
>> are all incompatible with each other.
>>
>> Use the function for checking three mutually incompatible options,
>> die_for_incompatible_opt3(), to perform this check in one place and
>> without repetition. This is shorter and clearer.
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>> revision.c | 9 +++------
>> 1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/revision.c b/revision.c
>> index b2861474b1..1b7e1af6c6 100644
>> --- a/revision.c
>> +++ b/revision.c
>> @@ -3036,8 +3036,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>> revs->grep_filter.ignore_locale = 1;
>> compile_grep_patterns(&revs->grep_filter);
>>
>> - if (revs->reverse && revs->reflog_info)
>> - die(_("options '%s' and '%s' cannot be used together"), "--reverse", "--walk-reflogs");
>> if (revs->reflog_info && revs->limited)
>> die("cannot combine --walk-reflogs with history-limiting options");
>> if (revs->rewrite_parents && revs->children.name)
>> @@ -3048,11 +3046,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>> /*
>> * Limitations on the graph functionality
>> */
>> - if (revs->reverse && revs->graph)
>> - die(_("options '%s' and '%s' cannot be used together"), "--reverse", "--graph");
>> + die_for_incompatible_opt3(!!revs->graph, "--graph",
>> + !!revs->reverse, "--reverse",
>> + !!revs->reflog_info, "--walk-reflogs");
>
> I've been wondering why we use `!!` here, as `die_for_incompatible_*()`
> doesn't care for the actual value but only checks that it is non-zero.
> Is it because of the type mismatch, where these flags here use unsigned
> ints whereas `die_for_incompatible_*()` expect ints?
->graph and ->reflog_info are pointers and clang reports an int-conversion
warning without the double negation. ->reverse is an unsigned:1 and so
doesn't need it, but I gave it one anyway for aesthetic reasons.
René
^ permalink raw reply
* Re: [PATCH 0/7] clone: fix init of refdb with wrong object format
From: Patrick Steinhardt @ 2023-12-06 13:09 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1701863960.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3208 bytes --]
On Wed, Dec 06, 2023 at 01:39:44PM +0100, Patrick Steinhardt wrote:
> Hi,
>
> when using git-clone(1), we initialize the complete repository before we
> know about the object format used by the remote repository. This means
> that we'll potentially create the refdb with the wrong object format in
> case the local default object format and remote object format are not
> the same.
>
> This isn't much of a problem in the context of the files backend, which
> never records the object format anyway. But it is a problem for the
> reftable backend, which indeed records the object format in the on-disk
> data structures. The result is thus a reftable with wrong object format.
>
> This patch series aims to solve this issue by initializing the refdb at
> a later point after we have learned about the remote object format. This
> requires some careful reordering of code. Unfortunately, the end result
> is not easily verifiable and thus I didn't add tests here. But it does
> fix cloning of SHA256 repositories with the in-progress state of the
> reftable backend.
>
> While at it I noticed that this actually fixes a bug with bundle URIs
> when the object formats diverge in this way.
>
> The series is layed out as follows:
>
> - Patch 1 + 2: split out a function to create the refdb and make it
> possible to skip its initialization in `init_db()`.
>
> - Patch 3: allows git-remote-curl(1) to work with repos that get
> initialized during its lifetime.
>
> - Patch 4 - 6: address various corner cases where we access the refdb
> before we learned about the object format.
>
> - Patch 7: move initialization of the refdb to happen after we have
> learned about the object format.
>
> This patch series is actually the last incompatibility for the reftable
> backend that I have found. All tests except for the files-backend
> specific ones pass now with the current state I have at [1], which is
> currently at e6f2f592b7 (t: skip tests which are incompatible with
> reftable, 2023-11-24)
I forgot to add the link to the merge request that contains the current
reftable backend's implementation in case anybody is interested:
https://gitlab.com/gitlab-org/git/-/merge_requests/58.
Patrick
> Thanks in advance for your reviews!
>
> Patrick
>
> Patrick Steinhardt (7):
> setup: extract function to create the refdb
> setup: allow skipping creation of the refdb
> remote-curl: rediscover repository when fetching refs
> builtin/clone: fix bundle URIs with mismatching object formats
> builtin/clone: set up sparse checkout later
> builtin/clone: skip reading HEAD when retrieving remote
> builtin/clone: create the refdb with the correct object format
>
> builtin/clone.c | 65 ++++++++++++----------
> remote-curl.c | 7 ++-
> remote.c | 26 +++++----
> remote.h | 1 +
> setup.c | 106 +++++++++++++++++++++---------------
> setup.h | 6 +-
> t/t5558-clone-bundle-uri.sh | 18 ++++++
> 7 files changed, 140 insertions(+), 89 deletions(-)
>
> --
> 2.43.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 4/7] revision, rev-parse: factorize incompatibility messages about --exclude-hidden
From: Patrick Steinhardt @ 2023-12-06 13:08 UTC (permalink / raw)
To: René Scharfe; +Cc: git
In-Reply-To: <20231206115215.94467-5-l.s.r@web.de>
[-- Attachment #1: Type: text/plain, Size: 1444 bytes --]
On Wed, Dec 06, 2023 at 12:51:58PM +0100, René Scharfe wrote:
> Use the standard parameterized message for reporting incompatible
> options to report options that are not accepted in combination with
> --exclude-hidden. This reduces the number of strings to translate and
> makes the UI a bit more consistent.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> builtin/rev-parse.c | 9 ++++++---
> revision.c | 18 ++++++++++++------
> t/t6018-rev-list-glob.sh | 6 ++----
> t/t6021-rev-list-exclude-hidden.sh | 4 ++--
> 4 files changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index fde8861ca4..917f122440 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -893,13 +893,15 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> }
> if (opt_with_value(arg, "--branches", &arg)) {
> if (ref_excludes.hidden_refs_configured)
> - return error(_("--exclude-hidden cannot be used together with --branches"));
> + return error(_("options '%s' and '%s' cannot be used together"),
> + "--exclude-hidden", "--branches");
The repetitive nature of this patch and subsequent ones made me wonder
whether it would be useful to have a function similar to the
`die_for_incompatible_*()` helper that knows to format this error
correctly.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 3/7] revision: use die_for_incompatible_opt3() for --graph/--reverse/--walk-reflogs
From: Patrick Steinhardt @ 2023-12-06 13:08 UTC (permalink / raw)
To: René Scharfe; +Cc: git
In-Reply-To: <20231206115215.94467-4-l.s.r@web.de>
[-- Attachment #1: Type: text/plain, Size: 1957 bytes --]
On Wed, Dec 06, 2023 at 12:51:57PM +0100, René Scharfe wrote:
> The revision options --reverse is incompatibel with --walk-reflogs and
> --graph is incompatible with both --reverse and --walk-reflogs. So they
> are all incompatible with each other.
>
> Use the function for checking three mutually incompatible options,
> die_for_incompatible_opt3(), to perform this check in one place and
> without repetition. This is shorter and clearer.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> revision.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/revision.c b/revision.c
> index b2861474b1..1b7e1af6c6 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -3036,8 +3036,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
> revs->grep_filter.ignore_locale = 1;
> compile_grep_patterns(&revs->grep_filter);
>
> - if (revs->reverse && revs->reflog_info)
> - die(_("options '%s' and '%s' cannot be used together"), "--reverse", "--walk-reflogs");
> if (revs->reflog_info && revs->limited)
> die("cannot combine --walk-reflogs with history-limiting options");
> if (revs->rewrite_parents && revs->children.name)
> @@ -3048,11 +3046,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
> /*
> * Limitations on the graph functionality
> */
> - if (revs->reverse && revs->graph)
> - die(_("options '%s' and '%s' cannot be used together"), "--reverse", "--graph");
> + die_for_incompatible_opt3(!!revs->graph, "--graph",
> + !!revs->reverse, "--reverse",
> + !!revs->reflog_info, "--walk-reflogs");
I've been wondering why we use `!!` here, as `die_for_incompatible_*()`
doesn't care for the actual value but only checks that it is non-zero.
Is it because of the type mismatch, where these flags here use unsigned
ints whereas `die_for_incompatible_*()` expect ints?
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 0/7] standardize incompatibility messages
From: Patrick Steinhardt @ 2023-12-06 13:07 UTC (permalink / raw)
To: René Scharfe; +Cc: git
In-Reply-To: <20231206115215.94467-1-l.s.r@web.de>
[-- Attachment #1: Type: text/plain, Size: 1489 bytes --]
On Wed, Dec 06, 2023 at 12:51:54PM +0100, René Scharfe wrote:
> More of what a699367bb8 (i18n: factorize more 'incompatible options'
> messages, 2022-01-31) did: Simplify checks for multiple mutually
> exclusive options, reduce the number of strings to translate, improve UI
> consistency a bit.
Thanks for working on this! The patch series looks mostly good to me,
I've only got two questions.
Patrick
> push: use die_for_incompatible_opt4() for --delete/--tags/--all/--mirror
> repack: use die_for_incompatible_opt3() for -A/-k/--cruft
> revision: use die_for_incompatible_opt3() for --graph/--reverse/--walk-reflogs
> revision, rev-parse: factorize incompatibility messages about --exclude-hidden
> clean: factorize incompatibility message
> worktree: standardize incompatibility messages
> worktree: simplify incompatibility message for --orphan and commit-ish
>
> builtin/clean.c | 2 +-
> builtin/push.c | 12 ++++--------
> builtin/repack.c | 14 ++++----------
> builtin/rev-parse.c | 9 ++++++---
> builtin/worktree.c | 21 +++++++++++----------
> revision.c | 27 +++++++++++++++------------
> t/t2400-worktree-add.sh | 2 +-
> t/t6018-rev-list-glob.sh | 6 ++----
> t/t6021-rev-list-exclude-hidden.sh | 4 ++--
> 9 files changed, 46 insertions(+), 51 deletions(-)
>
> --
> 2.43.0
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 7/7] builtin/clone: create the refdb with the correct object format
From: Patrick Steinhardt @ 2023-12-06 12:40 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1701863960.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3121 bytes --]
We're currently creating the reference database with a potentially
incorrect object format when the remote repository's object format is
different from the local default object format. This works just fine for
now because the files backend never records the object format anywhere.
But this logic will fail with any new reference backend that encodes
this information in some form either on-disk or in-memory.
The preceding commits have reshuffled code in git-clone(1) so that there
is no code path that will access the reference database before we have
detected the remote's object format. With these refactorings we can now
defer initialization of the reference database until after we have
learned the remote's object format and thus initialize it with the
correct format from the get-go.
These refactorings are required to make git-clone(1) work with the
upcoming reftable backend when cloning repositories with the SHA256
object format.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/clone.c | 9 ++++++++-
setup.c | 2 +-
setup.h | 1 +
3 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 06966c5d4c..fd052b8b54 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1097,8 +1097,14 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
}
}
+ /*
+ * Initialize the repository, but skip initializing the reference
+ * database. We do not yet know about the object format of the
+ * repository, and reference backends may persist that information into
+ * their on-disk data structures.
+ */
init_db(git_dir, real_git_dir, option_template, GIT_HASH_UNKNOWN, NULL,
- do_not_override_repo_unix_permissions, INIT_DB_QUIET);
+ do_not_override_repo_unix_permissions, INIT_DB_QUIET | INIT_DB_SKIP_REFDB);
if (real_git_dir) {
free((char *)git_dir);
@@ -1282,6 +1288,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));
initialize_repository_version(hash_algo, 1);
repo_set_hash_algo(the_repository, hash_algo);
+ create_reference_database(NULL, 1);
/*
* Before fetching from the remote, download and install bundle
diff --git a/setup.c b/setup.c
index a80fc09b9c..e1d0ce29c6 100644
--- a/setup.c
+++ b/setup.c
@@ -1897,7 +1897,7 @@ static int is_reinit(void)
return ret;
}
-static void create_reference_database(const char *initial_branch, int quiet)
+void create_reference_database(const char *initial_branch, int quiet)
{
struct strbuf err = STRBUF_INIT;
int reinit = is_reinit();
diff --git a/setup.h b/setup.h
index cbf538286b..3f0f17c351 100644
--- a/setup.h
+++ b/setup.h
@@ -178,6 +178,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
const char *initial_branch, int init_shared_repository,
unsigned int flags);
void initialize_repository_version(int hash_algo, int reinit);
+void create_reference_database(const char *initial_branch, int quiet);
/*
* NOTE NOTE NOTE!!
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 6/7] builtin/clone: skip reading HEAD when retrieving remote
From: Patrick Steinhardt @ 2023-12-06 12:40 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1701863960.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 5197 bytes --]
After we have set up the remote configuration in git-clone(1) we'll call
`remote_get()` to read the remote from the on-disk configuration. But
next to reading the on-disk configuration, `remote_get()` will also
cause us to try and read the repository's HEAD reference so that we can
figure out the current branch. Besides being pointless in git-clone(1)
because we're operating in an empty repository anyway, this will also
break once we move creation of the reference database to a later point
in time.
Refactor the code to introduce a new `remote_get_early()` function that
will skip reading the HEAD reference to address this issue.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/clone.c | 2 +-
remote.c | 26 ++++++++++++++++----------
remote.h | 1 +
3 files changed, 18 insertions(+), 11 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 9c60923f31..06966c5d4c 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1185,7 +1185,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
if (option_required_reference.nr || option_optional_reference.nr)
setup_reference();
- remote = remote_get(remote_name);
+ remote = remote_get_early(remote_name);
refspec_appendf(&remote->fetch, "+%s*:%s*", src_ref_prefix,
branch_top.buf);
diff --git a/remote.c b/remote.c
index abb24822be..051d0a64a0 100644
--- a/remote.c
+++ b/remote.c
@@ -509,7 +509,7 @@ static void alias_all_urls(struct remote_state *remote_state)
}
}
-static void read_config(struct repository *repo)
+static void read_config(struct repository *repo, int early)
{
int flag;
@@ -518,7 +518,7 @@ static void read_config(struct repository *repo)
repo->remote_state->initialized = 1;
repo->remote_state->current_branch = NULL;
- if (startup_info->have_repository) {
+ if (startup_info->have_repository && !early) {
const char *head_ref = refs_resolve_ref_unsafe(
get_main_ref_store(repo), "HEAD", 0, NULL, &flag);
if (head_ref && (flag & REF_ISSYMREF) &&
@@ -561,7 +561,7 @@ static const char *remotes_remote_for_branch(struct remote_state *remote_state,
const char *remote_for_branch(struct branch *branch, int *explicit)
{
- read_config(the_repository);
+ read_config(the_repository, 0);
die_on_missing_branch(the_repository, branch);
return remotes_remote_for_branch(the_repository->remote_state, branch,
@@ -587,7 +587,7 @@ remotes_pushremote_for_branch(struct remote_state *remote_state,
const char *pushremote_for_branch(struct branch *branch, int *explicit)
{
- read_config(the_repository);
+ read_config(the_repository, 0);
die_on_missing_branch(the_repository, branch);
return remotes_pushremote_for_branch(the_repository->remote_state,
@@ -599,7 +599,7 @@ static struct remote *remotes_remote_get(struct remote_state *remote_state,
const char *remote_ref_for_branch(struct branch *branch, int for_push)
{
- read_config(the_repository);
+ read_config(the_repository, 0);
die_on_missing_branch(the_repository, branch);
if (branch) {
@@ -709,7 +709,13 @@ remotes_remote_get(struct remote_state *remote_state, const char *name)
struct remote *remote_get(const char *name)
{
- read_config(the_repository);
+ read_config(the_repository, 0);
+ return remotes_remote_get(the_repository->remote_state, name);
+}
+
+struct remote *remote_get_early(const char *name)
+{
+ read_config(the_repository, 1);
return remotes_remote_get(the_repository->remote_state, name);
}
@@ -722,7 +728,7 @@ remotes_pushremote_get(struct remote_state *remote_state, const char *name)
struct remote *pushremote_get(const char *name)
{
- read_config(the_repository);
+ read_config(the_repository, 0);
return remotes_pushremote_get(the_repository->remote_state, name);
}
@@ -738,7 +744,7 @@ int remote_is_configured(struct remote *remote, int in_repo)
int for_each_remote(each_remote_fn fn, void *priv)
{
int i, result = 0;
- read_config(the_repository);
+ read_config(the_repository, 0);
for (i = 0; i < the_repository->remote_state->remotes_nr && !result;
i++) {
struct remote *remote =
@@ -1831,7 +1837,7 @@ struct branch *branch_get(const char *name)
{
struct branch *ret;
- read_config(the_repository);
+ read_config(the_repository, 0);
if (!name || !*name || !strcmp(name, "HEAD"))
ret = the_repository->remote_state->current_branch;
else
@@ -1973,7 +1979,7 @@ static const char *branch_get_push_1(struct remote_state *remote_state,
const char *branch_get_push(struct branch *branch, struct strbuf *err)
{
- read_config(the_repository);
+ read_config(the_repository, 0);
die_on_missing_branch(the_repository, branch);
if (!branch)
diff --git a/remote.h b/remote.h
index cdc8b1db42..79353ba226 100644
--- a/remote.h
+++ b/remote.h
@@ -118,6 +118,7 @@ struct remote {
* and configuration.
*/
struct remote *remote_get(const char *name);
+struct remote *remote_get_early(const char *name);
struct remote *pushremote_get(const char *name);
int remote_is_configured(struct remote *remote, int in_repo);
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 5/7] builtin/clone: set up sparse checkout later
From: Patrick Steinhardt @ 2023-12-06 12:40 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1701863960.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1387 bytes --]
When asked to do a sparse checkout, then git-clone(1) will spawn
`git sparse-checkout set` to set up the configuration accordingly. This
requires a proper Git repository or otherwise the command will fail. But
as we are about to move creation of the reference database to a later
point, this prerequisite will not hold anymore.
Move the logic to a later point in time where we know to have created
the reference database already.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/clone.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index d188650881..9c60923f31 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1185,9 +1185,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
if (option_required_reference.nr || option_optional_reference.nr)
setup_reference();
- if (option_sparse_checkout && git_sparse_checkout_init(dir))
- return 1;
-
remote = remote_get(remote_name);
refspec_appendf(&remote->fetch, "+%s*:%s*", src_ref_prefix,
@@ -1426,6 +1423,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
dissociate_from_references();
}
+ if (option_sparse_checkout && git_sparse_checkout_init(dir))
+ return 1;
+
junk_mode = JUNK_LEAVE_REPO;
err = checkout(submodule_progress, filter_submodules);
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 4/7] builtin/clone: fix bundle URIs with mismatching object formats
From: Patrick Steinhardt @ 2023-12-06 12:40 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1701863960.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 6666 bytes --]
We create the reference database in git-clone(1) quite early before
connecting to the remote repository. Given that we do not yet know about
the object format that the remote repository uses at that point in time
the consequence is that the refdb may be initialized with the wrong
object format.
This is not a problem in the context of the files backend as we do not
encode the object format anywhere, and furthermore the only reference
that we write between initializing the refdb and learning about the
object format is the "HEAD" symref. It will become a problem though once
we land the reftable backend, which indeed does require to know about
the proper object format at the time of creation. We thus need to
rearrange the logic in git-clone(1) so that we only initialize the refdb
once we have learned about the actual object format.
As a first step, move listing of remote references to happen earlier,
which also allow us to set up the hash algorithm of the repository
earlier now. While we aim to execute this logic as late as possible
until after most of the setup has happened already, detection of the
object format and thus later the setup of the reference database must
happen before any other logic that may spawn Git commands or otherwise
these Git commands may not recognize the repository as such.
The first Git step where we expect the repository to be fully initalized
is when we fetch bundles via bundle URIs. Funny enough, the comments
there also state that "the_repository must match the cloned repo", which
is indeed not necessarily the case for the hash algorithm right now. So
in practice it is the right thing to detect the remote's object format
before downloading bundle URIs anyway, and not doing so causes clones
with bundle URIS to fail when the local default object format does not
match the remote repository's format.
Unfortunately though, this creates a new issue: downloading bundles may
take a long time, so if we list refs beforehand they might've grown
stale meanwhile. It is not clear how to solve this issue except for a
second reference listing though after we have downloaded the bundles,
which may be an expensive thing to do.
Arguably though, it's preferable to have a staleness issue compared to
being unable to clone a repository altogether.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/clone.c | 48 ++++++++++++++++++-------------------
t/t5558-clone-bundle-uri.sh | 18 ++++++++++++++
2 files changed, 41 insertions(+), 25 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index c6357af949..d188650881 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1266,6 +1266,26 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
if (transport->smart_options && !deepen && !filter_options.choice)
transport->smart_options->check_self_contained_and_connected = 1;
+ strvec_push(&transport_ls_refs_options.ref_prefixes, "HEAD");
+ refspec_ref_prefixes(&remote->fetch,
+ &transport_ls_refs_options.ref_prefixes);
+ if (option_branch)
+ expand_ref_prefix(&transport_ls_refs_options.ref_prefixes,
+ option_branch);
+ if (!option_no_tags)
+ strvec_push(&transport_ls_refs_options.ref_prefixes,
+ "refs/tags/");
+
+ refs = transport_get_remote_refs(transport, &transport_ls_refs_options);
+
+ /*
+ * Now that we know what algorithm the remote side is using, let's set
+ * ours to the same thing.
+ */
+ hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));
+ initialize_repository_version(hash_algo, 1);
+ repo_set_hash_algo(the_repository, hash_algo);
+
/*
* Before fetching from the remote, download and install bundle
* data from the --bundle-uri option.
@@ -1281,24 +1301,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
bundle_uri);
else if (has_heuristic)
git_config_set_gently("fetch.bundleuri", bundle_uri);
- }
-
- strvec_push(&transport_ls_refs_options.ref_prefixes, "HEAD");
- refspec_ref_prefixes(&remote->fetch,
- &transport_ls_refs_options.ref_prefixes);
- if (option_branch)
- expand_ref_prefix(&transport_ls_refs_options.ref_prefixes,
- option_branch);
- if (!option_no_tags)
- strvec_push(&transport_ls_refs_options.ref_prefixes,
- "refs/tags/");
-
- refs = transport_get_remote_refs(transport, &transport_ls_refs_options);
-
- if (refs)
- mapped_refs = wanted_peer_refs(refs, &remote->fetch);
-
- if (!bundle_uri) {
+ } else {
/*
* Populate transport->got_remote_bundle_uri and
* transport->bundle_uri. We might get nothing.
@@ -1319,13 +1322,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
}
}
- /*
- * Now that we know what algorithm the remote side is using,
- * let's set ours to the same thing.
- */
- hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));
- initialize_repository_version(hash_algo, 1);
- repo_set_hash_algo(the_repository, hash_algo);
+ if (refs)
+ mapped_refs = wanted_peer_refs(refs, &remote->fetch);
if (mapped_refs) {
/*
diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh
index 996a08e90c..1ca5f745e7 100755
--- a/t/t5558-clone-bundle-uri.sh
+++ b/t/t5558-clone-bundle-uri.sh
@@ -33,6 +33,15 @@ test_expect_success 'clone with path bundle' '
test_cmp expect actual
'
+test_expect_success 'clone with path bundle and non-default hash' '
+ test_when_finished "rm -rf clone-path-non-default-hash" &&
+ GIT_DEFAULT_HASH=sha256 git clone --bundle-uri="clone-from/B.bundle" \
+ clone-from clone-path-non-default-hash &&
+ git -C clone-path-non-default-hash rev-parse refs/bundles/topic >actual &&
+ git -C clone-from rev-parse topic >expect &&
+ test_cmp expect actual
+'
+
test_expect_success 'clone with file:// bundle' '
git clone --bundle-uri="file://$(pwd)/clone-from/B.bundle" \
clone-from clone-file &&
@@ -284,6 +293,15 @@ test_expect_success 'clone HTTP bundle' '
test_config -C clone-http log.excludedecoration refs/bundle/
'
+test_expect_success 'clone HTTP bundle with non-default hash' '
+ test_when_finished "rm -rf clone-http-non-default-hash" &&
+ GIT_DEFAULT_HASH=sha256 git clone --bundle-uri="$HTTPD_URL/B.bundle" \
+ "$HTTPD_URL/smart/fetch.git" clone-http-non-default-hash &&
+ git -C clone-http-non-default-hash rev-parse refs/bundles/topic >actual &&
+ git -C clone-from rev-parse topic >expect &&
+ test_cmp expect actual
+'
+
test_expect_success 'clone bundle list (HTTP, no heuristic)' '
test_when_finished rm -f trace*.txt &&
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 3/7] remote-curl: rediscover repository when fetching refs
From: Patrick Steinhardt @ 2023-12-06 12:39 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1701863960.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1997 bytes --]
We're about to change git-clone(1) so that we set up the reference
database at a later point. This change will cause git-remote-curl(1) to
not detect the repository anymore due to "HEAD" not having been created
yet at the time it spawns, and thus causes it to error out once it is
asked to fetch the references.
We can address this issue by trying to re-discover the Git repository in
case none was detected at startup time. With this change, the clone will
look as following:
1. git-clone(1) sets up the initial repository, excluding the
reference database.
2. git-clone(1) spawns git-remote-curl(1), which will be unable to
detect the repository due to a missing "HEAD".
3. git-clone(1) asks git-remote-curl(1) to list remote references.
This works just fine as this step does not require a local
repository
4. git-clone(1) creates the reference database as it has now learned
about the object format.
5. git-clone(1) asks git-remote-curl(1) to fetch the remote packfile.
The latter notices that it doesn't have a repository available, but
it now knows to try and re-discover it.
If the re-discovery succeeds in the last step we can continue with the
clone.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
remote-curl.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index ef05752ca5..fc29757b65 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1564,8 +1564,11 @@ int cmd_main(int argc, const char **argv)
if (buf.len == 0)
break;
if (starts_with(buf.buf, "fetch ")) {
- if (nongit)
- die(_("remote-curl: fetch attempted without a local repo"));
+ if (nongit) {
+ setup_git_directory_gently(&nongit);
+ if (nongit)
+ die(_("remote-curl: fetch attempted without a local repo"));
+ }
parse_fetch(&buf);
} else if (!strcmp(buf.buf, "list") || starts_with(buf.buf, "list ")) {
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 2/7] setup: allow skipping creation of the refdb
From: Patrick Steinhardt @ 2023-12-06 12:39 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1701863960.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2753 bytes --]
Allow callers to skip creation of the reference database via a new flag
`INIT_DB_SKIP_REFDB`, which is required for git-clone(1) so that we can
create it at a later point once the object format has been discovered
from the remote repository.
Note that we also uplift the call to `create_reference_database()` into
`init_db()`, which makes it easier to handle the new flag for us. This
changes the order in which we do initialization so that we now set up
the Git configuration before we create the reference database. In
practice this move should not result in any change in behaviour.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
setup.c | 13 +++++--------
setup.h | 5 +++--
2 files changed, 8 insertions(+), 10 deletions(-)
diff --git a/setup.c b/setup.c
index 9fcb64159f..a80fc09b9c 100644
--- a/setup.c
+++ b/setup.c
@@ -1941,11 +1941,9 @@ static void create_reference_database(const char *initial_branch, int quiet)
static int create_default_files(const char *template_path,
const char *original_git_dir,
- const char *initial_branch,
const struct repository_format *fmt,
int prev_bare_repository,
- int init_shared_repository,
- int quiet)
+ int init_shared_repository)
{
struct stat st1;
struct strbuf buf = STRBUF_INIT;
@@ -2016,7 +2014,6 @@ static int create_default_files(const char *template_path,
adjust_shared_perm(get_git_dir());
}
- create_reference_database(initial_branch, quiet);
initialize_repository_version(fmt->hash_algo, 0);
/* Check filemode trustability */
@@ -2176,11 +2173,11 @@ int init_db(const char *git_dir, const char *real_git_dir,
validate_hash_algorithm(&repo_fmt, hash);
reinit = create_default_files(template_dir, original_git_dir,
- initial_branch, &repo_fmt,
- prev_bare_repository,
- init_shared_repository,
- flags & INIT_DB_QUIET);
+ &repo_fmt, prev_bare_repository,
+ init_shared_repository);
+ if (!(flags & INIT_DB_SKIP_REFDB))
+ create_reference_database(initial_branch, flags & INIT_DB_QUIET);
create_object_directory();
if (get_shared_repository()) {
diff --git a/setup.h b/setup.h
index b48cf1c43b..cbf538286b 100644
--- a/setup.h
+++ b/setup.h
@@ -169,8 +169,9 @@ int verify_repository_format(const struct repository_format *format,
*/
void check_repository_format(struct repository_format *fmt);
-#define INIT_DB_QUIET 0x0001
-#define INIT_DB_EXIST_OK 0x0002
+#define INIT_DB_QUIET (1 << 0)
+#define INIT_DB_EXIST_OK (1 << 1)
+#define INIT_DB_SKIP_REFDB (1 << 2)
int init_db(const char *git_dir, const char *real_git_dir,
const char *template_dir, int hash_algo,
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 1/7] setup: extract function to create the refdb
From: Patrick Steinhardt @ 2023-12-06 12:39 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1701863960.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4260 bytes --]
We're about to let callers skip creation of the reference database when
calling `init_db()`. Extract the logic into a standalone function so
that it becomes easier to do this refactoring.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
setup.c | 95 ++++++++++++++++++++++++++++++++++-----------------------
1 file changed, 57 insertions(+), 38 deletions(-)
diff --git a/setup.c b/setup.c
index fc592dc6dd..9fcb64159f 100644
--- a/setup.c
+++ b/setup.c
@@ -1885,6 +1885,60 @@ void initialize_repository_version(int hash_algo, int reinit)
git_config_set_gently("extensions.objectformat", NULL);
}
+static int is_reinit(void)
+{
+ struct strbuf buf = STRBUF_INIT;
+ char junk[2];
+ int ret;
+
+ git_path_buf(&buf, "HEAD");
+ ret = !access(buf.buf, R_OK) || readlink(buf.buf, junk, sizeof(junk) - 1) != -1;
+ strbuf_release(&buf);
+ return ret;
+}
+
+static void create_reference_database(const char *initial_branch, int quiet)
+{
+ struct strbuf err = STRBUF_INIT;
+ int reinit = is_reinit();
+
+ /*
+ * We need to create a "refs" dir in any case so that older
+ * versions of git can tell that this is a repository.
+ */
+ safe_create_dir(git_path("refs"), 1);
+ adjust_shared_perm(git_path("refs"));
+
+ if (refs_init_db(&err))
+ die("failed to set up refs db: %s", err.buf);
+
+ /*
+ * Point the HEAD symref to the initial branch with if HEAD does
+ * not yet exist.
+ */
+ if (!reinit) {
+ char *ref;
+
+ if (!initial_branch)
+ initial_branch = git_default_branch_name(quiet);
+
+ ref = xstrfmt("refs/heads/%s", initial_branch);
+ if (check_refname_format(ref, 0) < 0)
+ die(_("invalid initial branch name: '%s'"),
+ initial_branch);
+
+ if (create_symref("HEAD", ref, NULL) < 0)
+ exit(1);
+ free(ref);
+ }
+
+ if (reinit && initial_branch)
+ warning(_("re-init: ignored --initial-branch=%s"),
+ initial_branch);
+
+ strbuf_release(&err);
+}
+
static int create_default_files(const char *template_path,
const char *original_git_dir,
const char *initial_branch,
@@ -1896,10 +1950,8 @@ static int create_default_files(const char *template_path,
struct stat st1;
struct strbuf buf = STRBUF_INIT;
char *path;
- char junk[2];
int reinit;
int filemode;
- struct strbuf err = STRBUF_INIT;
const char *init_template_dir = NULL;
const char *work_tree = get_git_work_tree();
@@ -1919,6 +1971,8 @@ static int create_default_files(const char *template_path,
reset_shared_repository();
git_config(git_default_config, NULL);
+ reinit = is_reinit();
+
/*
* We must make sure command-line options continue to override any
* values we might have just re-read from the config.
@@ -1962,39 +2016,7 @@ static int create_default_files(const char *template_path,
adjust_shared_perm(get_git_dir());
}
- /*
- * We need to create a "refs" dir in any case so that older
- * versions of git can tell that this is a repository.
- */
- safe_create_dir(git_path("refs"), 1);
- adjust_shared_perm(git_path("refs"));
-
- if (refs_init_db(&err))
- die("failed to set up refs db: %s", err.buf);
-
- /*
- * Point the HEAD symref to the initial branch with if HEAD does
- * not yet exist.
- */
- path = git_path_buf(&buf, "HEAD");
- reinit = (!access(path, R_OK)
- || readlink(path, junk, sizeof(junk)-1) != -1);
- if (!reinit) {
- char *ref;
-
- if (!initial_branch)
- initial_branch = git_default_branch_name(quiet);
-
- ref = xstrfmt("refs/heads/%s", initial_branch);
- if (check_refname_format(ref, 0) < 0)
- die(_("invalid initial branch name: '%s'"),
- initial_branch);
-
- if (create_symref("HEAD", ref, NULL) < 0)
- exit(1);
- free(ref);
- }
-
+ create_reference_database(initial_branch, quiet);
initialize_repository_version(fmt->hash_algo, 0);
/* Check filemode trustability */
@@ -2158,9 +2180,6 @@ int init_db(const char *git_dir, const char *real_git_dir,
prev_bare_repository,
init_shared_repository,
flags & INIT_DB_QUIET);
- if (reinit && initial_branch)
- warning(_("re-init: ignored --initial-branch=%s"),
- initial_branch);
create_object_directory();
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 0/7] clone: fix init of refdb with wrong object format
From: Patrick Steinhardt @ 2023-12-06 12:39 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 2793 bytes --]
Hi,
when using git-clone(1), we initialize the complete repository before we
know about the object format used by the remote repository. This means
that we'll potentially create the refdb with the wrong object format in
case the local default object format and remote object format are not
the same.
This isn't much of a problem in the context of the files backend, which
never records the object format anyway. But it is a problem for the
reftable backend, which indeed records the object format in the on-disk
data structures. The result is thus a reftable with wrong object format.
This patch series aims to solve this issue by initializing the refdb at
a later point after we have learned about the remote object format. This
requires some careful reordering of code. Unfortunately, the end result
is not easily verifiable and thus I didn't add tests here. But it does
fix cloning of SHA256 repositories with the in-progress state of the
reftable backend.
While at it I noticed that this actually fixes a bug with bundle URIs
when the object formats diverge in this way.
The series is layed out as follows:
- Patch 1 + 2: split out a function to create the refdb and make it
possible to skip its initialization in `init_db()`.
- Patch 3: allows git-remote-curl(1) to work with repos that get
initialized during its lifetime.
- Patch 4 - 6: address various corner cases where we access the refdb
before we learned about the object format.
- Patch 7: move initialization of the refdb to happen after we have
learned about the object format.
This patch series is actually the last incompatibility for the reftable
backend that I have found. All tests except for the files-backend
specific ones pass now with the current state I have at [1], which is
currently at e6f2f592b7 (t: skip tests which are incompatible with
reftable, 2023-11-24)
Thanks in advance for your reviews!
Patrick
Patrick Steinhardt (7):
setup: extract function to create the refdb
setup: allow skipping creation of the refdb
remote-curl: rediscover repository when fetching refs
builtin/clone: fix bundle URIs with mismatching object formats
builtin/clone: set up sparse checkout later
builtin/clone: skip reading HEAD when retrieving remote
builtin/clone: create the refdb with the correct object format
builtin/clone.c | 65 ++++++++++++----------
remote-curl.c | 7 ++-
remote.c | 26 +++++----
remote.h | 1 +
setup.c | 106 +++++++++++++++++++++---------------
setup.h | 6 +-
t/t5558-clone-bundle-uri.sh | 18 ++++++
7 files changed, 140 insertions(+), 89 deletions(-)
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 6/7] worktree: standardize incompatibility messages
From: René Scharfe @ 2023-12-06 11:52 UTC (permalink / raw)
To: git
In-Reply-To: <20231206115215.94467-1-l.s.r@web.de>
Use the standard parameterized message for reporting incompatible
options for worktree add. This reduces the number of strings to
translate and makes the UI slightly more consistent.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
builtin/worktree.c | 17 +++++++++--------
t/t2400-worktree-add.sh | 2 +-
2 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 62b7e26f4b..036ceaa981 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -730,11 +730,11 @@ static int dwim_orphan(const struct add_opts *opts, int opt_track, int remote)
}
if (opt_track) {
- die(_("'%s' and '%s' cannot be used together"), "--orphan",
- "--track");
+ die(_("options '%s' and '%s' cannot be used together"),
+ "--orphan", "--track");
} else if (!opts->checkout) {
- die(_("'%s' and '%s' cannot be used together"), "--orphan",
- "--no-checkout");
+ die(_("options '%s' and '%s' cannot be used together"),
+ "--orphan", "--no-checkout");
}
return 1;
}
@@ -806,13 +806,14 @@ static int add(int ac, const char **av, const char *prefix)
if (!!opts.detach + !!new_branch + !!new_branch_force > 1)
die(_("options '%s', '%s', and '%s' cannot be used together"), "-b", "-B", "--detach");
if (opts.detach && opts.orphan)
- die(_("options '%s', and '%s' cannot be used together"),
+ die(_("options '%s' and '%s' cannot be used together"),
"--orphan", "--detach");
if (opts.orphan && opt_track)
- die(_("'%s' and '%s' cannot be used together"), "--orphan", "--track");
+ die(_("options '%s' and '%s' cannot be used together"),
+ "--orphan", "--track");
if (opts.orphan && !opts.checkout)
- die(_("'%s' and '%s' cannot be used together"), "--orphan",
- "--no-checkout");
+ die(_("options '%s' and '%s' cannot be used together"),
+ "--orphan", "--no-checkout");
if (opts.orphan && ac == 2)
die(_("'%s' and '%s' cannot be used together"), "--orphan",
_("<commit-ish>"));
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index df4aff7825..245656b53a 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -711,7 +711,7 @@ test_dwim_orphan () {
local fetch_error_text="fatal: No local or remote refs exist despite at least one remote" &&
local orphan_hint="hint: If you meant to create a worktree containing a new orphan branch" &&
local invalid_ref_regex="^fatal: invalid reference: " &&
- local bad_combo_regex="^fatal: '[-a-z]*' and '[-a-z]*' cannot be used together" &&
+ local bad_combo_regex="^fatal: options '[-a-z]*' and '[-a-z]*' cannot be used together" &&
local git_ns="repo" &&
local dashc_args="-C $git_ns" &&
--
2.43.0
^ permalink raw reply related
* [PATCH 7/7] worktree: simplify incompatibility message for --orphan and commit-ish
From: René Scharfe @ 2023-12-06 11:52 UTC (permalink / raw)
To: git
In-Reply-To: <20231206115215.94467-1-l.s.r@web.de>
Use a single translatable string to report that the worktree add option
--orphan is incompatible with a commit-ish instead of having the
commit-ish in a separate translatable string. This reduces the number
of strings to translate and gives translators the full context.
A similar message is used in builtin/describe.c, but with the plural of
commit-ish, and here we need the singular form.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
builtin/worktree.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 036ceaa981..4ac1621541 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -815,8 +815,8 @@ static int add(int ac, const char **av, const char *prefix)
die(_("options '%s' and '%s' cannot be used together"),
"--orphan", "--no-checkout");
if (opts.orphan && ac == 2)
- die(_("'%s' and '%s' cannot be used together"), "--orphan",
- _("<commit-ish>"));
+ die(_("option '%s' and commit-ish cannot be used together"),
+ "--orphan");
if (lock_reason && !keep_locked)
die(_("the option '%s' requires '%s'"), "--reason", "--lock");
if (lock_reason)
--
2.43.0
^ permalink raw reply related
* [PATCH 5/7] clean: factorize incompatibility message
From: René Scharfe @ 2023-12-06 11:51 UTC (permalink / raw)
To: git
In-Reply-To: <20231206115215.94467-1-l.s.r@web.de>
Use the standard parameterized message for reporting incompatible
options to inform users that they can't use -x and -X together. This
reduces the number of strings to translate and makes the UI slightly
more consistent.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
builtin/clean.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/clean.c b/builtin/clean.c
index 49c224e626..d90766cad3 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -971,7 +971,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
dir.flags |= DIR_SHOW_OTHER_DIRECTORIES;
if (ignored && ignored_only)
- die(_("-x and -X cannot be used together"));
+ die(_("options '%s' and '%s' cannot be used together"), "-x", "-X");
if (!ignored)
setup_standard_excludes(&dir);
if (ignored_only)
--
2.43.0
^ permalink raw reply related
* [PATCH 2/7] repack: use die_for_incompatible_opt3() for -A/-k/--cruft
From: René Scharfe @ 2023-12-06 11:51 UTC (permalink / raw)
To: git
In-Reply-To: <20231206115215.94467-1-l.s.r@web.de>
The repack option --keep-unreachable is incompatible with -A, --cruft is
incompatible with -A and -k, and -k is short for --keep-unreachable. So
they are all incompatible with each other.
Use the function for checking three mutually incompatible options,
die_for_incompatible_opt3(), to perform this check in one place and
without repetition. This is shorter and clearer.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
builtin/repack.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index edaee4dbec..c54777bbe5 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -1203,19 +1203,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
if (delete_redundant && repository_format_precious_objects)
die(_("cannot delete packs in a precious-objects repo"));
- if (keep_unreachable &&
- (unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE)))
- die(_("options '%s' and '%s' cannot be used together"), "--keep-unreachable", "-A");
+ die_for_incompatible_opt3(unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE), "-A",
+ keep_unreachable, "-k/--keep-unreachable",
+ pack_everything & PACK_CRUFT, "--cruft");
- if (pack_everything & PACK_CRUFT) {
+ if (pack_everything & PACK_CRUFT)
pack_everything |= ALL_INTO_ONE;
- if (unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE))
- die(_("options '%s' and '%s' cannot be used together"), "--cruft", "-A");
- if (keep_unreachable)
- die(_("options '%s' and '%s' cannot be used together"), "--cruft", "-k");
- }
-
if (write_bitmaps < 0) {
if (!write_midx &&
(!(pack_everything & ALL_INTO_ONE) || !is_bare_repository()))
--
2.43.0
^ permalink raw reply related
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