* [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
* 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
* 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 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 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 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 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 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
* 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: 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
* 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 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: 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 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: [PATCH 4/7] revision, rev-parse: factorize incompatibility messages about --exclude-hidden
From: Taylor Blau @ 2023-12-06 19:25 UTC (permalink / raw)
To: René Scharfe; +Cc: Patrick Steinhardt, git
In-Reply-To: <4954cf77-63f6-4225-833f-3c28d642ed11@web.de>
On Wed, Dec 06, 2023 at 06:07:29PM +0100, René Scharfe wrote:
> > 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 think that this is a great idea. It nicely solves Patrick's concern
that we have to duplicate this message ID everywhere, and equally solves
yours by calling error() inline instead of having to pass down the
option values.
I think that including a number in the macro name would be helpful here.
> > 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.
I agree :-).
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] object-name: reject too-deep recursive ancestor queries
From: Jeff King @ 2023-12-06 19:40 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: Junio C Hamano, Taylor Blau, git,
Carlos Andrés Ramírez Cataño
In-Reply-To: <ZWB26TH0CFW1KC4L@tanuki>
On Fri, Nov 24, 2023 at 11:11:53AM +0100, Patrick Steinhardt wrote:
> > When we get "HEAD~~~~~~~~~^2~~~~~~" from the user, do we somehow try
> > to create a file or a directory with that name and fail due to
> > ENAMETOOLONG?
>
> Sorry, this was a typo on my part. I didn't mean "revision", I meant
> "reference" here. References are limited to at most 4kB on most
> platforms due to filesystem limitations, whereas revisions currently
> have no limits in place.
Even without filesystem limitations, references are effectively limited
to 64kb due to the pkt-line format.
Revisions can be much longer than a reference, though. We accept
"some_ref:some/path/in/tree", for instance[1]. I think you could argue
that paths are likewise limited by the filesystem, though. Even on
systems like Linux where paths can grow arbitrarily long (by descending
and adding to the current directory), you're still limited in specifying
a full pathname. And Git will always use the full path from the project
root when creating worktree entries. Plus my recent tree-depth patches
effectively limit us to 16MB in the default config.
So I think it might be reasonable to limit revision lengths just as a
belt-and-suspenders against overflow attacks, etc. But I suspect that
the limits we'd choose there might not match what we'd want for
protection against stack exhaustion via recursion. E.g., I think 8k is
probably the minimum I'd want for a revision ("my/4k/ref:my/4k/path").
If one "~" character can create an expensive recursion, that might be
too much.
So we probably need something like Taylor's patch anyway (or to switch
to an iterative algorithm, though that might be tricky because of the
way we parse). I agree it needs to handle "^", though.
-Peff
[1] There are other more exotic revisions, too. The most arbitrary-sized
that comes to mind is ":/some-string-to-match". I doubt anybody
would be too mad if that were limited to 8k or even 4k, though.
^ permalink raw reply
* Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default
From: Jeff King @ 2023-12-06 19:49 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Junio C Hamano, git, Karthik Nayak
In-Reply-To: <ZVTJFOSnVonoPgZk@tanuki>
On Wed, Nov 15, 2023 at 02:35:16PM +0100, Patrick Steinhardt wrote:
> > > Just to make sure we do not miscommunicate, I do not think we want
> > > to trigger the paranoia mode only in our tests. We want to be
> > > paranoid to help real users who used "--missing" for their real use,
> > > so enabling PARANOIA in the test script is a wrong approach. We
> > > should enable it inside "rev-list --missing" codepath.
> >
> > Yeah. Just like we auto-enabled GIT_REF_PARANOIA for git-gc, etc, I
> > think we should do the same here.
>
> I'm honestly still torn on this one. There are two cases that I can
> think of where missing objects would be benign and where one wants to
> use `git rev-list --missing`:
>
> - Repositories with promisor remotes, to find the boundary of where
> we need to fetch new objects.
>
> - Quarantine directories where you only intend to list new objects
> or find the boundary.
>
> And in neither of those cases I can see a path for how the commit-graph
> would contain such missing commits when using regular tooling to perform
> repository maintenance.
Sorry for being unclear (and for the very slow response!). What I meant
by "here" was not "rev-list --missing" in particular, but rather that
"here" is "GIT_COMMIT_GRAPH_PARANOIA". And like GIT_REF_PARANOIA, we
should make sure it is turned on when checking for repository
corruption.
So specifically I meant that turning it off should:
1. Not cause us to miss corruption with fsck.
2. Not cause us to make corruption worse during a destructive repack
(e.g., "repack -ad").
3. Not admit corruption into the repository by fooling the rev-list
invocation for check_connected().
I don't think the third one uses --missing at all, but even if it did,
the interesting thing to me is not "--missing", but rather that the
caller knows it is doing a corruption check. So it would set the
environment variable itself.
So in your loosening patch, I would have expected to see a couple of
those cases overriding the new "default to off" behavior. But it may be
that they are not necessary (e.g., I think fsck may turn off the commit
graph entirely already).
> So I'm still not sure why we think that this case is so much more
> special than others. If a user wants to check for repository corruption
> the tool shouldn't be `git rev-list --missing`, but git-fsck(1). To me,
> the former is only useful in very specific circumstances where the user
> knows what they are doing, and in none of the usecases I can think of
> should we have a stale commit-graph _unless_ we have actual repository
> corruption.
>
> In reverse, to me this means that `--missing` is no more special than
> any of the other low-level tooling, where our stance seems to be "We
> assume that the repository is not corrupt". In that spirit, I'd argue
> that the same default value should apply here as for all the other
> cases.
Yeah, I think we are on the same page here. The need for paranoia is
really in the eyes of the caller, because only they know how careful
they want the operation to be.
> Oh, well. I'll wait for answers to this reply until tomorrow, and if I
> still haven't been able to convince anybody then I'll assume it's just
> me and adapt accordingly :)
Sorry, better late than never. ;)
-Peff
^ permalink raw reply
* Re: Minor UX annoyance w/`git add --patch untracked/file`
From: Jeff King @ 2023-12-06 19:54 UTC (permalink / raw)
To: Vito Caputo; +Cc: git
In-Reply-To: <20231130192637.wqpmidfv2roqmxdx@shells.gnugeneration.com>
On Thu, Nov 30, 2023 at 11:26:37AM -0800, Vito Caputo wrote:
> Couldn't the following two steps be done automagically by --patch:
>
> ```
> git add -N path/to/untracked/file/wishing/to/partially/add
> git add --patch path/to/untracked/file/wishing/to/partially/add
> ```
>
> when one simply does:
>
> `git add --patch path/to/untracked/file/wishing/to/partially/add`
>
> ?
They _could_, but keep in mind that the argument is not strictly a path.
It is a pathspec that may match multiple paths. So:
git add -p path/to/
for example will pick up the tracked files in path/to/, but not your
untracked one.
It would be possible to distinguish the two cases, and only auto-add
files which are explicitly mentioned as full paths. But we usually shy
away from too many special cases like this, as the resulting behavior
can end up confusing and hard to explain.
-Peff
^ permalink raw reply
* Re: [PATCH] setup: recognize bare repositories with packed-refs
From: Jeff King @ 2023-12-06 20:10 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Adam Majer, git
In-Reply-To: <ZWcOvjGPVS_CMUAk@tanuki>
On Wed, Nov 29, 2023 at 11:13:18AM +0100, Patrick Steinhardt wrote:
> As I'm currently working on the reftable backend this thought has also
> crossed my mind. The reftable backend doesn't only create "refs/", but
> it also creates "HEAD" with contents "ref: refs/heads/.invalid" so that
> Git commands recognize the Git directory properly. Longer-term I would
> really love to see us doing a better job of detecting Git repositories
> so that we don't have to carry this legacy baggage around.
>
> I can see different ways for how to do this:
>
> - Either we iterate through all known reference backends, asking
> each of them whether they recognize the directory as something
> they understand.
>
> - Or we start parsing the gitconfig of the repository so that we can
> learn about which reference backend to expect, and then ask that
> specific backend whether it thinks that the directory indeed looks
> like something it can handle.
>
> I'd personally prefer the latter, but I'm not sure whether we really
> want to try and parse any file that happens to be called "config".
We do eventually parse the config file to pick up repositoryFormatVersion.
But there's sort of a chicken-and-egg here where we only do so after
gaining some confidence that it's a repo directory. :)
I actually think the "ask each backend if it looks plausible" is
reasonable, at least for an implementation that knows about all
backends. Though what gives me pause is how older versions of Git will
behave with a new-format repository that does not have a "refs"
directory.
There are really two compatibility checks. In is_git_directory(), we
want to say "is this a repo or not". And then later we parse the config,
make sure the repository format is OK, and that we support all
extensions. So right now, an older version of Git that encounters a
reftable-formatted repo (that has a vestigial "refs/" directory) says
"ah, that is a repo, but I don't understand it" (the latter because
presumably the repo version/extensions in .git/config are values it
doesn't know about). But if we get rid of "refs/", then older versions
of Git will stop even considering it as a repo at all, and will keep
searching up to the ceiling directory. So either:
1. They'll find nothing, and you'll get "you're not in a git repo",
rather than "you're in a git repo, but I don't understand it".
Which is slightly worse.
2. They'll find some _other_ containing repo. Which could be quite
confusing.
So forgetting at all about how we structure the code, it seems to me
that the problem is not new code, but all of the existing code which
looks for access("refs", X_OK).
I dunno. Maybe that is being too paranoid about backwards compatibility.
People will have to turn on reftable manually, at least for a while, and
would hopefully know what they are signing up for, and that old versions
might not work as well. And by the time a new format becomes the
default, it's possible that those older versions would have become quite
rare.
> Just throwing this out there, but we could use this as an excuse to
> introduce "extensions.refFormat". If it's explicitly configured to be
> "reffiles" then we accept repositories even if they don't have the
> "refs/" directory or a "packed-refs" file. This would still require work
> in alternative implementations of Git, but this work will need to happen
> anyway when the reftable backend lands.
>
> I'd personally love for this extension to be introduced before I'm
> sending the reftable backend upstream so that we can have discussions
> around it beforehand.
We already have an extension config option to specify that we're using
reftable, don't we? But anything in config has the same chicken-and-egg
problems as above, I think.
-Peff
^ permalink raw reply
* Re: [PATCH] setup: recognize bare repositories with packed-refs
From: Jeff King @ 2023-12-06 21:08 UTC (permalink / raw)
To: Taylor Blau; +Cc: Adam Majer, git
In-Reply-To: <ZWethlRRtuQLDRlJ@nand.local>
On Wed, Nov 29, 2023 at 04:30:46PM -0500, Taylor Blau wrote:
> On Tue, Nov 28, 2023 at 02:04:46PM -0500, Jeff King wrote:
> > - whatever is consuming the embedded repos could "mkdir -p refs
> > objects" as needed. This is a minor pain, but I think in the long
> > term we are moving to a world where you have to explicitly do
> > "GIT_DIR=$PWD/embedded.git" to access an embedded bare repo. So
> > they're already special and require some setup; adding an extra step
> > may not be so bad.
>
> I hope not. I suppose that using embedded bare repositories in a test
> requires additional setup at least to "cd" into the directory (if they
> are not using `$GIT_DIR` or `--git-dir` already). But I fear that
> imposing even a small change like this is too tall an order for how many
> millions of these exist in the wild across all sorts of projects.
I dunno. I am skeptical that there are millions of these. Who really
wants to embed bare git repos except for projects related to Git itself,
which want test vectors? Is there a use case I'm missing?
-Peff
^ permalink raw reply
* Re: [PATCH 1/7] setup: extract function to create the refdb
From: Karthik Nayak @ 2023-12-06 21:10 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <b69c57d27269c9b40c9e4394861dffd8a8b9860c.1701863960.git.ps@pks.im>
On Wed, Dec 6, 2023 at 1:40 PM Patrick Steinhardt <ps@pks.im> wrote:
> +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.
> + */
How does this work though, even if an earlier version of git can tell
that this is a repository,
it still won't be able to read the reftable backend. In that sense,
what do we achieve here?
> + safe_create_dir(git_path("refs"), 1);
> + adjust_shared_perm(git_path("refs"));
> +
Not related to your commit per se, but we ignore the return value
here, shouldn't we die in this case?
^ permalink raw reply
* Re: [PATCH 4/7] builtin/clone: fix bundle URIs with mismatching object formats
From: Karthik Nayak @ 2023-12-06 21:13 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <c7a9d6ef74ff39e660f80e2e104a96b7c875845d.1701863960.git.ps@pks.im>
On Wed, Dec 6, 2023 at 1:40 PM Patrick Steinhardt <ps@pks.im> wrote:
> 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.
>
Nit: s/URIS/URIs
^ permalink raw reply
* Re: --end-of-options inconsistently available?!
From: Jeff King @ 2023-12-06 21:16 UTC (permalink / raw)
To: Sven Strickroth; +Cc: git
In-Reply-To: <ab14260c-d515-425e-8ef6-5739d3d6ca4e@cs-ware.de>
On Tue, Nov 28, 2023 at 09:40:08AM +0100, Sven Strickroth wrote:
> > This one is intentional. rev-parse in its default mode is not just
> > spitting out revisions, but also options that are meant to be passed
> > along to the revision machinery via other commands (like rev-list). So
> > for example:
> >
> > $ git rev-parse --foo HEAD
> > --foo
> > 564d0252ca632e0264ed670534a51d18a689ef5d
> >
> > And it does understand end-of-options explicitly, so:
> >
> > $ git rev-parse --end-of-options --foo --
> > --end-of-options
> > fatal: bad revision '--foo'
> >
> > If you just want to parse a name robustly, use --verify.
>
> I would expect that -- and --end-of-options are handled in a special way
> here so that rev-parse can also be used in scripts. I need to check whether
> --verify works for me (from the manual I thought I need to specify full
> reference names).
They _are_ handled specially, and for the purpose of using rev-parse in
scripts. It's just that in its default mode it does not do what you
want, because it has another purpose.
> > > $ git checkout -f --end-of-options HEAD~1 -- afile.txt
> > > fatal: only one reference expected, 2 given.
> >
> > I think this is the same KEEP_DASHDASH problem as with git-reset.
>
> I also found another problem:
> $ git format-patch --end-of-options -1
> fatal: option '-1' must come before non-option arguments
>
> Where -1 is the number of commits here...
This is the same as the "log --end-of-options --foo" example I showed
earlier. That "-1" cannot mean "use 1 commit", since you used it after
--end-of-options. It will correctly resolve refs/heads/-1 if you have
such a ref. But if you don't, then the DWIM logic for distinguishing
revisions and pathspecs produces a confusing message.
It would be possible to fix that (by telling verify_filename() that we
saw --end-of-options, and not to treat dashes specially). But in
practice if you are bothering to use --end-of-options, you really ought
to be using "--" as well, like:
git format-patch --end-of-options $revs -- $paths
in which case it will know that "-1" _must_ be a revision, and complain:
$ git format-patch --end-of-options -1 --
fatal: bad revision '-1'
-Peff
^ permalink raw reply
* [PATCH] parse-options: decouple "--end-of-options" and "--"
From: Jeff King @ 2023-12-06 22:21 UTC (permalink / raw)
To: Sven Strickroth; +Cc: git
In-Reply-To: <20231127212254.GA87495@coredump.intra.peff.net>
On Mon, Nov 27, 2023 at 04:22:54PM -0500, Jeff King wrote:
> So something like this works:
>
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 4b018d20e3..a0d801179a 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -259,6 +259,9 @@ static void parse_args(struct pathspec *pathspec,
> * At this point, argv points immediately after [-opts].
> */
>
> + if (argv[0] && !strcmp(argv[0], "--end-of-options"))
> + argv++;
> +
> if (argv[0]) {
> if (!strcmp(argv[0], "--")) {
> argv++; /* reset to HEAD, possibly with paths */
>
> but it feels like a maintenance problem that we'd have to audit every
> caller that uses KEEP_DASHDASH.
So here's my attempt at a central fix. There is a downside (see the
discussion below), but I think in practice it should be a strict
improvement for most commands. I won't be too surprised if we find a
counter-example, but even if we do, my gut feeling is that we should fix
that command on top of this, rather than give up and require every
command to be aware of --end-of-options.
-- >8 --
Subject: [PATCH] parse-options: decouple "--end-of-options" and "--"
When we added generic end-of-options support in 51b4594b40
(parse-options: allow --end-of-options as a synonym for "--",
2019-08-06), we made them true synonyms. They both stop option parsing,
and they are both returned in the resulting argv if the KEEP_DASHDASH
flag is used.
The hope was that this would work for all callers:
- most generic callers would not pass KEEP_DASHDASH, and so would just
do the right thing (stop parsing there) without needing to know
anything more.
- callers with KEEP_DASHDASH were generally going to rely on
setup_revisions(), which knew to handle --end-of-options specially
But that turned out miss quite a few cases that pass KEEP_DASHDASH but
do their own manual parsing. For example, "git reset", "git checkout",
and so on want pass KEEP_DASHDASH so they can support:
git reset $revs -- $paths
but of course aren't going to actually do a traversal, so they don't
call setup_revisions(). And those cases currently get confused by
--end-of-options being left in place, like:
$ git reset --end-of-options HEAD
fatal: option '--end-of-options' must come before non-option arguments
We could teach each of these callers to handle the leftover option
explicitly. But let's try to be a bit more clever and see if we can
solve it centrally in parse-options.c.
The bogus assumption here is that KEEP_DASHDASH tells us the caller
wants to see --end-of-options in the result. But really, the callers
which need to know that --end-of-options was reached are those that may
potentially parse more options from argv. In other words, those that
pass the KEEP_UNKNOWN_OPT flag.
If such a caller is aware of --end-of-options (e.g., because they call
setup_revisions() with the result), then this will continue to do the
right thing, treating anything after --end-of-options as a non-option.
And if the caller is not aware of --end-of-options, they are better off
keeping it intact, because either:
1. They are just passing the options along to somebody else anyway, in
which case that somebody would need to know about the
--end-of-options marker.
2. They are going to parse the remainder themselves, at which point
choking on --end-of-options is much better than having it silently
removed. The point is to avoid option injection from untrusted
command line arguments, and bailing is better than quietly treating
the untrusted argument as an option.
This fixes bugs with --end-of-options across several commands, but I've
focused on two in particular here:
- t7102 confirms that "git reset --end-of-options --foo" now works.
This checks two things. One, that we no longer barf on
"--end-of-options" itself (which previously we did, even if the rev
was something vanilla like "HEAD" instead of "--foo"). And two, that
we correctly treat "--foo" as a revision rather than an option.
This fix applies to any other cases which pass KEEP_DASHDASH but not
KEEP_UNKNOWN_OPT, like "git checkout", "git check-attr", "git grep",
etc, which would previously choke on "--end-of-options".
- t9350 shows the opposite case: fast-export passed KEEP_UNKNOWN_OPT
but not KEEP_DASHDASH, but then passed the result on to
setup_revisions(). So it never saw --end-of-options, and would
erroneously parse "fast-export --end-of-options --foo" as having a
"--foo" option. This is now fixed.
Note that this does shut the door for callers which want to know if we
hit end-of-options, but don't otherwise need to keep unknown opts. The
obvious thing here is feeding it to the DWIM verify_filename()
machinery. And indeed, this is a problem even for commands which do
understand --end-of-options already. For example, without this patch,
you get:
$ git log --end-of-options --foo
fatal: option '--foo' must come before non-option arguments
because we refuse to accept "--foo" as a filename (because it starts
with a dash) even though we could know that we saw end-of-options. The
verify_filename() function simply doesn't accept this extra information.
So that is the status quo, and this patch doubles down further on that.
Commands like "git reset" have the same problem, but they won't even
know that parse-options saw --end-of-options! So even if we fixed
verify_filename(), they wouldn't have anything to pass to it.
But in practice I don't think this is a big deal. If you are being
careful enough to use --end-of-options, then you should also be using
"--" to disambiguate and avoid the DWIM behavior in the first place. In
other words, doing:
git log --end-of-options --this-is-a-rev -- --this-is-a-path
works correctly, and will continue to do so. And likewise, with this
patch now:
git reset --end-of-options --this-is-a-rev -- --this-is-a-path
will work, as well.
Signed-off-by: Jeff King <peff@peff.net>
---
parse-options.c | 9 +++++++--
t/t7102-reset.sh | 8 ++++++++
t/t9350-fast-export.sh | 10 ++++++++++
3 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index e0c94b0546..d50962062e 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -929,13 +929,18 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
continue;
}
- if (!arg[2] /* "--" */ ||
- !strcmp(arg + 2, "end-of-options")) {
+ if (!arg[2] /* "--" */) {
if (!(ctx->flags & PARSE_OPT_KEEP_DASHDASH)) {
ctx->argc--;
ctx->argv++;
}
break;
+ } else if (!strcmp(arg + 2, "end-of-options")) {
+ if (!(ctx->flags & PARSE_OPT_KEEP_UNKNOWN_OPT)) {
+ ctx->argc--;
+ ctx->argv++;
+ }
+ break;
}
if (internal_help && !strcmp(arg + 2, "help-all"))
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 4287863ae6..62d9f846ce 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -616,4 +616,12 @@ test_expect_success 'reset --mixed sets up work tree' '
test_must_be_empty actual
'
+test_expect_success 'reset handles --end-of-options' '
+ git update-ref refs/heads/--foo HEAD^ &&
+ git log -1 --format=%s refs/heads/--foo >expect &&
+ git reset --hard --end-of-options --foo &&
+ git log -1 --format=%s HEAD >actual &&
+ test_cmp expect actual
+'
+
test_done
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 26c25c0eb2..e9a12c18bb 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -791,4 +791,14 @@ test_expect_success 'fast-export --first-parent outputs all revisions output by
)
'
+test_expect_success 'fast-export handles --end-of-options' '
+ git update-ref refs/heads/nodash HEAD &&
+ git update-ref refs/heads/--dashes HEAD &&
+ git fast-export --end-of-options nodash >expect &&
+ git fast-export --end-of-options --dashes >actual.raw &&
+ # fix up lines which mention the ref for comparison
+ sed s/--dashes/nodash/ <actual.raw >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.43.0.664.ga12c899002
^ permalink raw reply related
* t7900 fails with recent debian systemd?
From: Jeff King @ 2023-12-06 22:31 UTC (permalink / raw)
To: git
I noticed t7900 failing today. The failure looks like this:
$ ./t7900-maintenance.sh -v -i -x
[...]
+ systemd-analyze verify systemd/user/git-maintenance@hourly.service
Unit git-maintenance@hourly.service not found.
error: last command exited with $?=1
not ok 36 - start and stop Linux/systemd maintenance
The problem started after upgrading my Debian unstable system to the
systemd 255~rc4-2 deb. Downgrading back to 254.5-1 makes the test pass
again.
I'm sure it's something silly with finding paths in XDG_CONFIG_HOME or
something like that. I haven't dug further, but I thought I'd post this
to save somebody else going through the same initial debugging. (And of
course any wisdom or further debugging is greatly appreciated).
-Peff
^ 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