* [PATCH 1/1] clean: further clean-up of implementation around "--force"
From: Junio C Hamano @ 2024-03-03 22:06 UTC (permalink / raw)
To: git
In-Reply-To: <20240303220600.2491792-1-gitster@pobox.com>
We clarified how clean.requireForce interacts with the --dry-run
option in the previous commit, both in the implementation and in the
documentation. Even when "git clean" (without other options) is
required to be used with "--force" (i.e. either clean.requireForce
is unset, or explicitly set to true) to protect end-users from
casual invocation of the command by mistake, "--dry-run" does not
require "--force" to be used, because it is already its own
protection mechanism by being a no-op to the working tree files.
The previous commit, however, missed another clean-up opportunity
around the same area. Just like in the "--dry-run" mode, the
command in the "--interactive" mode does not require "--force",
either. This is because by going interactive and giving the end
user one more step to confirm, the mode itself is serving as its own
protection mechanism.
Let's take things one step further, unify the code that defines
interaction between `--force` and these two other options. Just
like we added explanation for the reason why "--dry-run" does not
honor `clean.requireForce`, add the same explanation for
"--interactive". Finally, add some tests to show the interaction
between "--force" and "--interactive" (we already have tests that
show interaction between "--force" and "--dry-run").
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/config/clean.txt | 2 +-
Documentation/git-clean.txt | 4 +++-
builtin/clean.c | 9 ++-------
t/t7300-clean.sh | 6 ++++++
4 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/Documentation/config/clean.txt b/Documentation/config/clean.txt
index b19ca210f3..c0188ead4e 100644
--- a/Documentation/config/clean.txt
+++ b/Documentation/config/clean.txt
@@ -1,3 +1,3 @@
clean.requireForce::
A boolean to make git-clean refuse to delete files unless -f
- or -i is given. Defaults to true.
+ is given. Defaults to true.
diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
index 662eebb852..082d033438 100644
--- a/Documentation/git-clean.txt
+++ b/Documentation/git-clean.txt
@@ -37,7 +37,7 @@ OPTIONS
--force::
If the Git configuration variable clean.requireForce is not set
to false, 'git clean' will refuse to delete files or directories
- unless given -f or -i. Git will refuse to modify untracked
+ unless given -f. Git will refuse to modify untracked
nested git repositories (directories with a .git subdirectory)
unless a second -f is given.
@@ -45,6 +45,8 @@ OPTIONS
--interactive::
Show what would be done and clean files interactively. See
``Interactive mode'' for details.
+ Configuration variable clean.requireForce is ignored, as
+ this mode gives its own safety protection by going interactive.
-n::
--dry-run::
diff --git a/builtin/clean.c b/builtin/clean.c
index 41502dcb0d..29efe84153 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -950,13 +950,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,
0);
- /* Dry run won't remove anything, so requiring force makes no sense */
- if (dry_run)
- require_force = 0;
-
- if (require_force != 0 && !force && !interactive)
- die(_("clean.requireForce is true and neither -f nor -i given:"
- " refusing to clean"));
+ if (require_force != 0 && !force && !interactive && !dry_run)
+ die(_("clean.requireForce is true and -f not given: refusing to clean"));
if (force > 1)
rm_flags = 0;
diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 611b3dd3ae..1f7201eb60 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -407,6 +407,12 @@ test_expect_success 'clean.requireForce and -f' '
'
+test_expect_success 'clean.requireForce and --interactive' '
+ git clean --interactive </dev/null >output 2>error &&
+ test_grep ! "requireForce is true and" error &&
+ test_grep "\*\*\* Commands \*\*\*" output
+'
+
test_expect_success 'core.excludesfile' '
echo excludes >excludes &&
--
2.44.0-84-gb387623c12
^ permalink raw reply related
* Re: [PATCH 1/1] clean: further clean-up of implementation around "--force"
From: Junio C Hamano @ 2024-03-03 22:18 UTC (permalink / raw)
To: git
In-Reply-To: <20240303220600.2491792-2-gitster@pobox.com>
Junio C Hamano <gitster@pobox.com> writes:
> @@ -950,13 +950,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
> argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,
> 0);
>
> - /* Dry run won't remove anything, so requiring force makes no sense */
> - if (dry_run)
> - require_force = 0;
> -
> - if (require_force != 0 && !force && !interactive)
> - die(_("clean.requireForce is true and neither -f nor -i given:"
> - " refusing to clean"));
> + if (require_force != 0 && !force && !interactive && !dry_run)
> + die(_("clean.requireForce is true and -f not given: refusing to clean"));
>
> if (force > 1)
> rm_flags = 0;
An obvious alternative way to clean-up the logic is to do this
instead:
if (dry_run || interactive))
require_force = 0;
if (require_force != 0 && !force)
die(_("clean.requireForce is true and ..."));
But as I wrote, the most important improvement done by Sergey's
patch was to remove the dual meaning of the "force" variable so that
it indicates if the "--force" option was given and nothing else,
while the "require_force" variable indicates if clean.requireForce
was given and nothing else. From that point of view, the
conditional tweaking done to require_force in the above alternative
makes the code worse, relative to Sergey's patch, and certainly to
its follow up, my patch about "--interactive".
^ permalink raw reply
* Re: [PATCH v2 1/1] branch: advise about ref syntax rules
From: Junio C Hamano @ 2024-03-03 22:42 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <4ad5d4190649dcb5f26c73a6f15ab731891b9dfd.1709491818.git.code@khaugsbakk.name>
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
This has sufficiently been advanced since the previous one, that
range-diff would need to be prodded with a larger --creation-factor
value to avoid getting a rather useless output.
1: 5548e6fa34 < -: ---------- branch: advise about ref syntax rules
-: ---------- > 1: 202d4e29ef branch: advise about ref syntax rules
> git-branch(1) will error out if you give it a bad ref name. But the user
> might not understand why or what part of the name is illegal.
>
> The user might know that there are some limitations based on the *loose
> ref* format (filenames), but there are also further rules for
> easier integration with shell-based tools, pathname expansion, and
> playing well with reference name expressions.
>
> The man page for git-check-ref-format(1) contains these rules. Let’s
> advise about it since that is not a command that you just happen
> upon. Also make this advise configurable since you might not want to be
> reminded every time you make a little typo.
Nicely written and easily read. Well done.
> + refSyntax::
> + Point the user towards the ref syntax documentation if
> + they give an invalid ref name.
I noticed a minor phrasing issue, but many other entries talk about
"shown when ...", even though a handful of them use "if ...". Do we
want to make them consistent?
> resetNoRefresh::
> Advice to consider using the `--no-refresh` option to
> linkgit:git-reset[1] when the command takes more than 2 seconds
> diff --git a/advice.c b/advice.c
> index 6e9098ff089..550c2968908 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -68,6 +68,7 @@ static struct {
> [ADVICE_PUSH_UNQUALIFIED_REF_NAME] = { "pushUnqualifiedRefName" },
> [ADVICE_PUSH_UPDATE_REJECTED] = { "pushUpdateRejected" },
> [ADVICE_PUSH_UPDATE_REJECTED_ALIAS] = { "pushNonFastForward" }, /* backwards compatibility */
> + [ADVICE_REF_SYNTAX] = { "refSyntax" },
> [ADVICE_RESET_NO_REFRESH_WARNING] = { "resetNoRefresh" },
> [ADVICE_RESOLVE_CONFLICT] = { "resolveConflict" },
> [ADVICE_RM_HINTS] = { "rmHints" },
> diff --git a/advice.h b/advice.h
> index 9d4f49ae38b..d15fe2351ab 100644
> --- a/advice.h
> +++ b/advice.h
> @@ -36,6 +36,7 @@ enum advice_type {
> ADVICE_PUSH_UNQUALIFIED_REF_NAME,
> ADVICE_PUSH_UPDATE_REJECTED,
> ADVICE_PUSH_UPDATE_REJECTED_ALIAS,
> + ADVICE_REF_SYNTAX,
> ADVICE_RESET_NO_REFRESH_WARNING,
> ADVICE_RESOLVE_CONFLICT,
> ADVICE_RM_HINTS,
Both of these are in lexicographic order, which is good.
> diff --git a/branch.c b/branch.c
> index 6719a181bd1..621019fcf4b 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -370,8 +370,12 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)
> */
> int validate_branchname(const char *name, struct strbuf *ref)
> {
> - if (strbuf_check_branch_ref(ref, name))
> - die(_("'%s' is not a valid branch name"), name);
> + if (strbuf_check_branch_ref(ref, name)) {
> + int code = die_message(_("'%s' is not a valid branch name"), name);
> + advise_if_enabled(ADVICE_REF_SYNTAX,
> + _("See `man git check-ref-format`"));
> + exit(code);
> + }
Nice.
> diff --git a/builtin/branch.c b/builtin/branch.c
> index cfb63cce5fb..1c122ee8a7b 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -576,8 +576,12 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
> */
> if (ref_exists(oldref.buf))
> recovery = 1;
> - else
> - die(_("invalid branch name: '%s'"), oldname);
> + else {
> + int code = die_message(_("invalid branch name: '%s'"), oldname);
> + advise_if_enabled(ADVICE_REF_SYNTAX,
> + _("See `man git check-ref-format`"));
> + exit(code);
> + }
> }
Good, too.
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index de7d3014e4f..d21fdf09c90 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -1725,4 +1725,15 @@ test_expect_success '--track overrides branch.autoSetupMerge' '
> test_cmp_config "" --default "" branch.foo5.merge
> '
>
> +cat <<\EOF >expect
> +fatal: 'foo..bar' is not a valid branch name
> +hint: See `man git check-ref-format`
> +hint: Disable this message with "git config advice.refSyntax false"
> +EOF
> +
> +test_expect_success 'errors if given a bad branch name' '
> + test_must_fail git branch foo..bar >actual 2>&1 &&
> + test_cmp expect actual
> +'
Even though there are a few ancient style tests that have code to
set up expectations outside the test_expect_success, most of the
tests in t3200 do use a more modern style. Let's not make it worse,
by moving it inside, perhaps like:
test_expect_success 'errors if given a bad branch name' '
cat >expect <<-\EOF &&
fatal: '\''foo..bar'\'' is not a valid branch name
hint: See `man git check-ref-format`
hint: Disable this message with "git config advice.refSyntax false"
EOF
test_must_fail git branch foo..bar >actual 2>&1 &&
test_cmp expect actual
'
We could make a preliminary clean-up to the file in question before
adding the above test, if we wanted to. Or we can do so after the
dust settles. Such a fix may look like the attached.
Thanks.
t/t3200-branch.sh | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git c/t/t3200-branch.sh w/t/t3200-branch.sh
index 94b536ef51..ba1e0eace5 100755
--- c/t/t3200-branch.sh
+++ w/t/t3200-branch.sh
@@ -1112,14 +1112,14 @@ test_expect_success '--set-upstream-to notices an error to set branch as own ups
test_cmp expect actual
"
-# Keep this test last, as it changes the current branch
-cat >expect <<EOF
-$HEAD refs/heads/g/h/i@{0}: branch: Created from main
-EOF
test_expect_success 'git checkout -b g/h/i -l should create a branch and a log' '
GIT_COMMITTER_DATE="2005-05-26 23:30" \
git checkout -b g/h/i -l main &&
test_ref_exists refs/heads/g/h/i &&
+
+ cat >expect <<-EOF &&
+ $HEAD refs/heads/g/h/i@{0}: branch: Created from main
+ EOF
git reflog show --no-abbrev-commit refs/heads/g/h/i >actual &&
test_cmp expect actual
'
^ permalink raw reply related
* Re: [PATCH] sequencer: allow disabling conflict advice
From: Junio C Hamano @ 2024-03-03 22:57 UTC (permalink / raw)
To: Philippe Blain via GitGitGadget; +Cc: git, Philippe Blain
In-Reply-To: <pull.1682.git.1709396291693.gitgitgadget@gmail.com>
"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
> if (msg) {
> - advise("%s\n", msg);
> + advise_if_enabled(ADVICE_SEQUENCER_CONFLICT, "%s\n", msg);
> /*
> * A conflict has occurred but the porcelain
> * (typically rebase --interactive) wants to take care
This hunk is good. The block removes the CHERRY_PICK_HEAD after
giving this advice and then returns.
> @@ -480,22 +480,25 @@ static void print_advice(struct repository *r, int show_hint,
>
> if (show_hint) {
> if (opts->no_commit)
> - advise(_("after resolving the conflicts, mark the corrected paths\n"
> - "with 'git add <paths>' or 'git rm <paths>'"));
> + advise_if_enabled(ADVICE_SEQUENCER_CONFLICT,
> + _("after resolving the conflicts, mark the corrected paths\n"
> + "with 'git add <paths>' or 'git rm <paths>'"));
> else if (opts->action == REPLAY_PICK)
> - advise(_("After resolving the conflicts, mark them with\n"
> - "\"git add/rm <pathspec>\", then run\n"
> - "\"git cherry-pick --continue\".\n"
> - "You can instead skip this commit with \"git cherry-pick --skip\".\n"
> - "To abort and get back to the state before \"git cherry-pick\",\n"
> - "run \"git cherry-pick --abort\"."));
> + advise_if_enabled(ADVICE_SEQUENCER_CONFLICT,
> + _("After resolving the conflicts, mark them with\n"
> + "\"git add/rm <pathspec>\", then run\n"
> + "\"git cherry-pick --continue\".\n"
> + "You can instead skip this commit with \"git cherry-pick --skip\".\n"
> + "To abort and get back to the state before \"git cherry-pick\",\n"
> + "run \"git cherry-pick --abort\"."));
> else if (opts->action == REPLAY_REVERT)
> - advise(_("After resolving the conflicts, mark them with\n"
> - "\"git add/rm <pathspec>\", then run\n"
> - "\"git revert --continue\".\n"
> - "You can instead skip this commit with \"git revert --skip\".\n"
> - "To abort and get back to the state before \"git revert\",\n"
> - "run \"git revert --abort\"."));
> + advise_if_enabled(ADVICE_SEQUENCER_CONFLICT,
> + _("After resolving the conflicts, mark them with\n"
> + "\"git add/rm <pathspec>\", then run\n"
> + "\"git revert --continue\".\n"
> + "You can instead skip this commit with \"git revert --skip\".\n"
> + "To abort and get back to the state before \"git revert\",\n"
> + "run \"git revert --abort\"."));
> else
> BUG("unexpected pick action in print_advice()");
> }
This hunk can be improved. If I were doing this patch, I probably
would have just done
- if (show_hint) {
+ if (show_hint && advice_enabled(ADVICE_SEQUENCER_CONFLICT)) {
and nothing else, and doing so would keep the block easier to extend
and maintain in the future.
Because the block is all about "show_hint", we have code to print
advice messages and nothing else in it currently, and more
importantly, we will not add anything other than code to print
advice messages in it. Because of that, skipping everything when
ADVICE_SEQUENCER_CONFLICT is not enabled will not cause problems
(unlike the earlier hunk---which will break if we added "&&
advice_enabled()" to "if (msg)"). That way, when somebody teaches
this code a new kind of opts->action, they do not have to say
"advice_if_enabled(ADVICE_SEQUENCER_CONFLICT()"; they can just use
"advise()".
> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index aeab689a98d..bc7c878b236 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
All changes to the file look good.
^ permalink raw reply
* Re: [PATCH v2 1/1] branch: advise about ref syntax rules
From: Kristoffer Haugsbakk @ 2024-03-03 22:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqil23uebw.fsf@gitster.g>
On Sun, Mar 3, 2024, at 23:42, Junio C Hamano wrote:
>> + refSyntax::
>> + Point the user towards the ref syntax documentation if
>> + they give an invalid ref name.
>
> I noticed a minor phrasing issue, but many other entries talk about
> "shown when ...", even though a handful of them use "if ...". Do we
> want to make them consistent?
Sure thing. Do you prefer the “shown when” alternative?
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH 1/1] rebase: teach `--exec` about `GIT_REBASE_BRANCH`
From: Junio C Hamano @ 2024-03-03 23:24 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <4140fca4f454310d215df8bdac237caeb5c38521.1709495964.git.code@khaugsbakk.name>
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
> The command fed to `--exec` might need some contextual information from
> the branch name. But there is no convenient access to the branch name
> that we were on before starting the rebase; rebase operates in detached
> HEAD mode so we cannot ask for it directly. This means that we need to
> parse something like this from the first line of `git branch --list`:
>
> (no branch, rebasing <branch>)
>
> This is a moderate amount of effort for something that git-rebase(1) can
> store for us.
>
> To that end, teach `--exec` about an env. variable which stores the
> branch name for the rebase-in-progress, if applicable.
You seem to be saying that `git branch --list` output already
contains the necessary information but it is shown in a hard to use
format. Is the information given at least always accurate and
reliable?
Assuming it is, do you know where "git branch --list" gets that
information when it says "(no branch, rebasing <branch>)"?
git-rebase(1) is already storing information sufficient to let "git
branch --list" to produce that information, and there are other ways
to inspect that state ("git status" gives the same information but
it also is in a "meant for humans" format).
So, isn't it just the matter of surfacing the information that we
are already recording and is already available in a fashion that is
easier to use? For example, if "git status --porcelain=[version]"
does not give the information, perhaps you can add a line or two to
it, instead of duplicating the same information in two places?
It comes from wt-status.c:wt_status_check_rebase() where state->branch
is assigned to, by reading "$GIT_DIR/rebase-{apply,merge}/head-name".
^ permalink raw reply
* Re: [question] How to customize git revert template
From: Wu Zhenyu @ 2024-03-03 14:44 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <9389c159-ba55-4329-9d86-bc95b321737f@app.fastmail.com>
Can git support the feature to customize template? Is it possible?
TIA!
On Thu, Feb 29, 2024 at 04:18:20PM +0100, Kristoffer Haugsbakk wrote:
> Hi
>
> On Thu, Feb 29, 2024, at 13:52, Wu Zhenyu wrote:
> > In https://git-scm.com/docs/git-revert
> >
> >> While git creates a basic commit message automatically
> >
> > How can I customize git revert template? e.g.,
> >
> > ```sh
> > git config revert.template ~/.config/git/template
> > ```
> >
> > TIA!
> >
> > --
> > Best Regards
> > Wu Zhenyu
>
> I see no way to customize this message. Your only choices seem to be
>
> 1. This default
> 2. The “reference” style using `--reference`
>
> So there doesn’t seem to be a template alternative here.
>
> --
> Kristoffer Haugsbakk
>
>
--
Best Regards
Wu Zhenyu
^ permalink raw reply
* RE: [PATCH v3] build: support z/OS (OS/390).
From: Haritha D @ 2024-03-04 4:19 UTC (permalink / raw)
To: rsbecker@nexbridge.com, 'Junio C Hamano'
Cc: git@vger.kernel.org, 'Kristoffer Haugsbakk'
In-Reply-To: <04df01da6c05$caaa5dd0$5fff1970$@nexbridge.com>
Looks like , I do not have sufficient permissions to rerun the tests. How do I proceed?
Please suggest.
Thank you
Haritha
On 01/03/24, 11:55 PM, "rsbecker@nexbridge.com <mailto:rsbecker@nexbridge.com>" <rsbecker@nexbridge.com <mailto:rsbecker@nexbridge.com>> wrote:
On Friday, March 1, 2024 1:15 PM, Junio C Hamano wrote:
>Haritha D <Harithamma.D@ibm.com <mailto:Harithamma.D@ibm.com>> writes:
>
>> The win test(7) test case failed stating the reason as "The Operation
>> cancelled". I saw that it failed after 5 hours 59 minutes of running
>> the test cases(build). How do I handle this?
>
>Those "win test (n)", at least some of them, seem to have been somewhat
flaky [*1*]. If you are certain you did not break them with
>your change, you do not have to fix them yourself.
>
>I am a wrong person to ask how the test failure that may [*2*] block GGG
submission can be circumvented, as I am not involved in that
>machinery at all.
>
>Thanks.
>
>
>[Footnotes]
>
>*1* Also I've seen osx-clang job time-out from time to time, without
> failing any specific test. Re-running failed jobs from the menu
> often make them pass, which is why I said "somewhat flaky".
>
>*2* I do not even know if GGG refuses to submit a series with a test
> failure, let alone if it allows to override such a safety if
> exists.
Which tests have been hanging on S390? We have occasional hangs on NonStop
that end up being attributed to our CI build system not supplying pipes
properly to git. It would be interesting if the same tests are having issues
on different platforms.
--Randall
^ permalink raw reply
* [PATCH] Allow git-config to append a comment
From: Ralph Seichter via GitGitGadget @ 2024-03-04 6:00 UTC (permalink / raw)
To: git; +Cc: Ralph Seichter, Ralph Seichter
From: Ralph Seichter <github@seichter.de>
Introduce the ability to append comments to modifications
made using git-config. Example usage:
git config --comment "I changed this. --A. Script" \
--add safe.directory /home/alice/somerepo.git
The implementation ensures that a # character is always
prepended to the provided comment string.
Signed-off-by: Ralph Seichter <github@seichter.de>
---
Allow git-config to append a comment
Introduce the ability to append comments to modifications made using
git-config. Example usage:
git config --comment "I changed this. --A. Script" \
--add safe.directory /home/alice/somerepo.git
The implementation ensures that a # character is always prepended to the
provided comment string.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1681%2Frseichter%2Fissue-1680-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1681/rseichter/issue-1680-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1681
Documentation/git-config.txt | 10 +++++++---
builtin/config.c | 21 ++++++++++++++-------
builtin/gc.c | 4 ++--
builtin/submodule--helper.c | 2 +-
builtin/worktree.c | 4 ++--
config.c | 21 +++++++++++++--------
config.h | 4 ++--
sequencer.c | 28 ++++++++++++++--------------
submodule-config.c | 2 +-
submodule.c | 2 +-
t/t1300-config.sh | 9 +++++++--
worktree.c | 4 ++--
12 files changed, 66 insertions(+), 45 deletions(-)
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index dff39093b5e..ee8cd251b24 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,9 +9,9 @@ git-config - Get and set repository or global options
SYNOPSIS
--------
[verse]
-'git config' [<file-option>] [--type=<type>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] <name> [<value> [<value-pattern>]]
-'git config' [<file-option>] [--type=<type>] --add <name> <value>
-'git config' [<file-option>] [--type=<type>] [--fixed-value] --replace-all <name> <value> [<value-pattern>]
+'git config' [<file-option>] [--type=<type>] [--comment=<value>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] <name> [<value> [<value-pattern>]]
+'git config' [<file-option>] [--type=<type>] [--comment=<value>] --add <name> <value>
+'git config' [<file-option>] [--type=<type>] [--comment=<value>] [--fixed-value] --replace-all <name> <value> [<value-pattern>]
'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get <name> [<value-pattern>]
'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all <name> [<value-pattern>]
'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp <name-regex> [<value-pattern>]
@@ -87,6 +87,10 @@ OPTIONS
values. This is the same as providing '^$' as the `value-pattern`
in `--replace-all`.
+--comment::
+ Append a comment to new or modified lines. A '#' character
+ will be automatically prepended to the value.
+
--get::
Get the value for a given key (optionally filtered by a regex
matching the value). Returns error code 1 if the key was not
diff --git a/builtin/config.c b/builtin/config.c
index b55bfae7d66..2aab3c0baf3 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -44,6 +44,7 @@ static struct config_options config_options;
static int show_origin;
static int show_scope;
static int fixed_value;
+static const char *comment;
#define ACTION_GET (1<<0)
#define ACTION_GET_ALL (1<<1)
@@ -173,6 +174,7 @@ static struct option builtin_config_options[] = {
OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, standard input, blob, command line)")),
OPT_BOOL(0, "show-scope", &show_scope, N_("show scope of config (worktree, local, global, system, command)")),
OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")),
+ OPT_STRING(0, "comment", &comment, N_("value"), N_("human-readable comment string (# will be prepended automatically)")),
OPT_END(),
};
@@ -797,6 +799,11 @@ int cmd_config(int argc, const char **argv, const char *prefix)
usage_builtin_config();
}
+ if (comment && !(actions & (ACTION_ADD|ACTION_SET|ACTION_SET_ALL|ACTION_REPLACE_ALL))) {
+ error(_("--comment is only applicable to add/set/replace operations"));
+ usage_builtin_config();
+ }
+
/* check usage of --fixed-value */
if (fixed_value) {
int allowed_usage = 0;
@@ -880,7 +887,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
check_write();
check_argc(argc, 2, 2);
value = normalize_value(argv[0], argv[1], &default_kvi);
- ret = git_config_set_in_file_gently(given_config_source.file, argv[0], value);
+ ret = git_config_set_in_file_gently(given_config_source.file, argv[0], comment, value);
if (ret == CONFIG_NOTHING_SET)
error(_("cannot overwrite multiple values with a single value\n"
" Use a regexp, --add or --replace-all to change %s."), argv[0]);
@@ -891,7 +898,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
value = normalize_value(argv[0], argv[1], &default_kvi);
ret = git_config_set_multivar_in_file_gently(given_config_source.file,
argv[0], value, argv[2],
- flags);
+ comment, flags);
}
else if (actions == ACTION_ADD) {
check_write();
@@ -900,7 +907,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
ret = git_config_set_multivar_in_file_gently(given_config_source.file,
argv[0], value,
CONFIG_REGEX_NONE,
- flags);
+ comment, flags);
}
else if (actions == ACTION_REPLACE_ALL) {
check_write();
@@ -908,7 +915,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
value = normalize_value(argv[0], argv[1], &default_kvi);
ret = git_config_set_multivar_in_file_gently(given_config_source.file,
argv[0], value, argv[2],
- flags | CONFIG_FLAGS_MULTI_REPLACE);
+ comment, flags | CONFIG_FLAGS_MULTI_REPLACE);
}
else if (actions == ACTION_GET) {
check_argc(argc, 1, 2);
@@ -936,17 +943,17 @@ int cmd_config(int argc, const char **argv, const char *prefix)
if (argc == 2)
return git_config_set_multivar_in_file_gently(given_config_source.file,
argv[0], NULL, argv[1],
- flags);
+ NULL, flags);
else
return git_config_set_in_file_gently(given_config_source.file,
- argv[0], NULL);
+ argv[0], NULL, NULL);
}
else if (actions == ACTION_UNSET_ALL) {
check_write();
check_argc(argc, 1, 2);
return git_config_set_multivar_in_file_gently(given_config_source.file,
argv[0], NULL, argv[1],
- flags | CONFIG_FLAGS_MULTI_REPLACE);
+ NULL, flags | CONFIG_FLAGS_MULTI_REPLACE);
}
else if (actions == ACTION_RENAME_SECTION) {
check_write();
diff --git a/builtin/gc.c b/builtin/gc.c
index cb80ced6cb5..342907f7bdb 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1553,7 +1553,7 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)
die(_("$HOME not set"));
rc = git_config_set_multivar_in_file_gently(
config_file, "maintenance.repo", maintpath,
- CONFIG_REGEX_NONE, 0);
+ CONFIG_REGEX_NONE, NULL, 0);
free(global_config_file);
if (rc)
@@ -1620,7 +1620,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
if (!config_file)
die(_("$HOME not set"));
rc = git_config_set_multivar_in_file_gently(
- config_file, key, NULL, maintpath,
+ config_file, key, NULL, maintpath, NULL,
CONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);
free(global_config_file);
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index fda50f2af1e..e4e18adb575 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1283,7 +1283,7 @@ static void sync_submodule(const char *path, const char *prefix,
submodule_to_gitdir(&sb, path);
strbuf_addstr(&sb, "/config");
- if (git_config_set_in_file_gently(sb.buf, remote_key, sub_origin_url))
+ if (git_config_set_in_file_gently(sb.buf, remote_key, NULL, sub_origin_url))
die(_("failed to update remote for submodule '%s'"),
path);
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 9c76b62b02d..a20cc8820e5 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -365,12 +365,12 @@ static void copy_filtered_worktree_config(const char *worktree_git_dir)
if (!git_configset_get_bool(&cs, "core.bare", &bare) &&
bare &&
git_config_set_multivar_in_file_gently(
- to_file, "core.bare", NULL, "true", 0))
+ to_file, "core.bare", NULL, "true", NULL, 0))
error(_("failed to unset '%s' in '%s'"),
"core.bare", to_file);
if (!git_configset_get(&cs, "core.worktree") &&
git_config_set_in_file_gently(to_file,
- "core.worktree", NULL))
+ "core.worktree", NULL, NULL))
error(_("failed to unset '%s' in '%s'"),
"core.worktree", to_file);
diff --git a/config.c b/config.c
index 3cfeb3d8bd9..a22594eabd9 100644
--- a/config.c
+++ b/config.c
@@ -3001,6 +3001,7 @@ static ssize_t write_section(int fd, const char *key,
}
static ssize_t write_pair(int fd, const char *key, const char *value,
+ const char *comment,
const struct config_store_data *store)
{
int i;
@@ -3041,7 +3042,10 @@ static ssize_t write_pair(int fd, const char *key, const char *value,
strbuf_addch(&sb, value[i]);
break;
}
- strbuf_addf(&sb, "%s\n", quote);
+ if (comment)
+ strbuf_addf(&sb, "%s #%s\n", quote, comment);
+ else
+ strbuf_addf(&sb, "%s\n", quote);
ret = write_in_full(fd, sb.buf, sb.len);
strbuf_release(&sb);
@@ -3130,9 +3134,9 @@ static void maybe_remove_section(struct config_store_data *store,
}
int git_config_set_in_file_gently(const char *config_filename,
- const char *key, const char *value)
+ const char *key, const char *comment, const char *value)
{
- return git_config_set_multivar_in_file_gently(config_filename, key, value, NULL, 0);
+ return git_config_set_multivar_in_file_gently(config_filename, key, value, NULL, comment, 0);
}
void git_config_set_in_file(const char *config_filename,
@@ -3153,7 +3157,7 @@ int repo_config_set_worktree_gently(struct repository *r,
if (r->repository_format_worktree_config) {
char *file = repo_git_path(r, "config.worktree");
int ret = git_config_set_multivar_in_file_gently(
- file, key, value, NULL, 0);
+ file, key, value, NULL, NULL, 0);
free(file);
return ret;
}
@@ -3195,6 +3199,7 @@ void git_config_set(const char *key, const char *value)
int git_config_set_multivar_in_file_gently(const char *config_filename,
const char *key, const char *value,
const char *value_pattern,
+ const char *comment,
unsigned flags)
{
int fd = -1, in_fd = -1;
@@ -3245,7 +3250,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
free(store.key);
store.key = xstrdup(key);
if (write_section(fd, key, &store) < 0 ||
- write_pair(fd, key, value, &store) < 0)
+ write_pair(fd, key, value, comment, &store) < 0)
goto write_err_out;
} else {
struct stat st;
@@ -3399,7 +3404,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
if (write_section(fd, key, &store) < 0)
goto write_err_out;
}
- if (write_pair(fd, key, value, &store) < 0)
+ if (write_pair(fd, key, value, comment, &store) < 0)
goto write_err_out;
}
@@ -3444,7 +3449,7 @@ void git_config_set_multivar_in_file(const char *config_filename,
const char *value_pattern, unsigned flags)
{
if (!git_config_set_multivar_in_file_gently(config_filename, key, value,
- value_pattern, flags))
+ value_pattern, NULL, flags))
return;
if (value)
die(_("could not set '%s' to '%s'"), key, value);
@@ -3467,7 +3472,7 @@ int repo_config_set_multivar_gently(struct repository *r, const char *key,
int res = git_config_set_multivar_in_file_gently(file,
key, value,
value_pattern,
- flags);
+ NULL, flags);
free(file);
return res;
}
diff --git a/config.h b/config.h
index 5dba984f770..a85a8271696 100644
--- a/config.h
+++ b/config.h
@@ -290,7 +290,7 @@ int git_config_pathname(const char **, const char *, const char *);
int git_config_expiry_date(timestamp_t *, const char *, const char *);
int git_config_color(char *, const char *, const char *);
-int git_config_set_in_file_gently(const char *, const char *, const char *);
+int git_config_set_in_file_gently(const char *, const char *, const char *, const char *);
/**
* write config values to a specific config file, takes a key/value pair as
@@ -336,7 +336,7 @@ int git_config_parse_key(const char *, char **, size_t *);
int git_config_set_multivar_gently(const char *, const char *, const char *, unsigned);
void git_config_set_multivar(const char *, const char *, const char *, unsigned);
int repo_config_set_multivar_gently(struct repository *, const char *, const char *, const char *, unsigned);
-int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned);
+int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, const char *, unsigned);
/**
* takes four parameters:
diff --git a/sequencer.c b/sequencer.c
index f49a871ac06..4c91ca5a844 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3460,54 +3460,54 @@ static int save_opts(struct replay_opts *opts)
if (opts->no_commit)
res |= git_config_set_in_file_gently(opts_file,
- "options.no-commit", "true");
+ "options.no-commit", NULL, "true");
if (opts->edit >= 0)
- res |= git_config_set_in_file_gently(opts_file, "options.edit",
+ res |= git_config_set_in_file_gently(opts_file, "options.edit", NULL,
opts->edit ? "true" : "false");
if (opts->allow_empty)
res |= git_config_set_in_file_gently(opts_file,
- "options.allow-empty", "true");
+ "options.allow-empty", NULL, "true");
if (opts->allow_empty_message)
res |= git_config_set_in_file_gently(opts_file,
- "options.allow-empty-message", "true");
+ "options.allow-empty-message", NULL, "true");
if (opts->keep_redundant_commits)
res |= git_config_set_in_file_gently(opts_file,
- "options.keep-redundant-commits", "true");
+ "options.keep-redundant-commits", NULL, "true");
if (opts->signoff)
res |= git_config_set_in_file_gently(opts_file,
- "options.signoff", "true");
+ "options.signoff", NULL, "true");
if (opts->record_origin)
res |= git_config_set_in_file_gently(opts_file,
- "options.record-origin", "true");
+ "options.record-origin", NULL, "true");
if (opts->allow_ff)
res |= git_config_set_in_file_gently(opts_file,
- "options.allow-ff", "true");
+ "options.allow-ff", NULL, "true");
if (opts->mainline) {
struct strbuf buf = STRBUF_INIT;
strbuf_addf(&buf, "%d", opts->mainline);
res |= git_config_set_in_file_gently(opts_file,
- "options.mainline", buf.buf);
+ "options.mainline", NULL, buf.buf);
strbuf_release(&buf);
}
if (opts->strategy)
res |= git_config_set_in_file_gently(opts_file,
- "options.strategy", opts->strategy);
+ "options.strategy", NULL, opts->strategy);
if (opts->gpg_sign)
res |= git_config_set_in_file_gently(opts_file,
- "options.gpg-sign", opts->gpg_sign);
+ "options.gpg-sign", NULL, opts->gpg_sign);
for (size_t i = 0; i < opts->xopts.nr; i++)
res |= git_config_set_multivar_in_file_gently(opts_file,
"options.strategy-option",
- opts->xopts.v[i], "^$", 0);
+ opts->xopts.v[i], "^$", NULL, 0);
if (opts->allow_rerere_auto)
res |= git_config_set_in_file_gently(opts_file,
- "options.allow-rerere-auto",
+ "options.allow-rerere-auto", NULL,
opts->allow_rerere_auto == RERERE_AUTOUPDATE ?
"true" : "false");
if (opts->explicit_cleanup)
res |= git_config_set_in_file_gently(opts_file,
- "options.default-msg-cleanup",
+ "options.default-msg-cleanup", NULL,
describe_cleanup_mode(opts->default_msg_cleanup));
return res;
}
diff --git a/submodule-config.c b/submodule-config.c
index 54130f6a385..11428b4adad 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -978,7 +978,7 @@ int config_set_in_gitmodules_file_gently(const char *key, const char *value)
{
int ret;
- ret = git_config_set_in_file_gently(GITMODULES_FILE, key, value);
+ ret = git_config_set_in_file_gently(GITMODULES_FILE, key, NULL, value);
if (ret < 0)
/* Maybe the user already did that, don't error out here */
warning(_("Could not update .gitmodules entry %s"), key);
diff --git a/submodule.c b/submodule.c
index 213da79f661..86630932d09 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2052,7 +2052,7 @@ void submodule_unset_core_worktree(const struct submodule *sub)
submodule_name_to_gitdir(&config_path, the_repository, sub->name);
strbuf_addstr(&config_path, "/config");
- if (git_config_set_in_file_gently(config_path.buf, "core.worktree", NULL))
+ if (git_config_set_in_file_gently(config_path.buf, "core.worktree", NULL, NULL))
warning(_("Could not unset core.worktree setting in submodule '%s'"),
sub->path);
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 31c38786870..daddaedd23c 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -69,13 +69,18 @@ test_expect_success 'replace with non-match (actually matching)' '
cat > expect << EOF
[section]
- penguin = very blue
Movie = BadPhysics
UPPERCASE = true
- penguin = kingpin
+ penguin = gentoo #Pygoscelis papua
+ disposition = peckish #find fish
[Sections]
WhatEver = Second
EOF
+test_expect_success 'append comments' '
+ git config --replace-all --comment="Pygoscelis papua" section.penguin gentoo &&
+ git config --comment="find fish" section.disposition peckish &&
+ test_cmp expect .git/config
+'
test_expect_success 'non-match result' 'test_cmp expect .git/config'
diff --git a/worktree.c b/worktree.c
index b02a05a74a3..cf5eea8c931 100644
--- a/worktree.c
+++ b/worktree.c
@@ -807,9 +807,9 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath,
static int move_config_setting(const char *key, const char *value,
const char *from_file, const char *to_file)
{
- if (git_config_set_in_file_gently(to_file, key, value))
+ if (git_config_set_in_file_gently(to_file, key, NULL, value))
return error(_("unable to set %s in '%s'"), key, to_file);
- if (git_config_set_in_file_gently(from_file, key, NULL))
+ if (git_config_set_in_file_gently(from_file, key, NULL, NULL))
return error(_("unable to unset %s in '%s'"), key, from_file);
return 0;
}
base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
--
gitgitgadget
^ permalink raw reply related
* GSoC 2024
From: Aishwarya Narayanan @ 2024-03-04 6:01 UTC (permalink / raw)
To: git
Dear Git Organization,
I am writing to express my strong interest in participating in Google
Summer of Code (GSoC) 2024 with the Git Organization. As a highly
motivated Final-year Electrical and Electronics Engineer, I am eager
to contribute to the open-source community and learn from experienced
developers like yourselves.
While this is my first foray into open-source development and GSoC, I
am excited by the prospect of contributing to the ongoing success of
Git. I have been actively learning and gaining a strong foundation in
Git commands, programming languages, and version control concepts. I
am particularly interested in the following project ideas:
1) Implement consistency checks for refs
2) Move existing tests to a unit testing framework
However, I understand the importance of starting with smaller
contributions before taking on a larger project like GSoC. I am eager
to begin by contributing to micro-projects and familiarizing myself
with the Git codebase and development workflow.
In this regard, I would greatly appreciate any guidance you can offer
on where to find existing micro-projects and how to get involved in
the Git community. Additionally, I would be grateful for any resources
or suggestions that can help me prepare for GSoC and submit a strong
proposal.
Thank you for your time and consideration. I am enthusiastic about the
opportunity to contribute to the Git project and look forward to
hearing from you soon.
Sincerely,
Aishwarya N
aishnana.03@gmail.com
https://github.com/Aishwarya-Narayanan03
^ permalink raw reply
* Re: [PATCH 1/2] refs/reftable: don't fail empty transactions in repo without HEAD
From: Patrick Steinhardt @ 2024-03-04 6:44 UTC (permalink / raw)
To: git, Junio C Hamano, Mike Hommey
In-Reply-To: <xcbwno3x7tzcg77rtfkaqtqcgll32vhx5mlhcvlclew6lkkofu@h5jzvft55uvv>
[-- Attachment #1: Type: text/plain, Size: 1693 bytes --]
On Thu, Feb 29, 2024 at 07:20:42PM -0600, Justin Tobler wrote:
> On 24/02/27 03:27PM, Patrick Steinhardt wrote:
> > Under normal circumstances, it shouldn't ever happen that a repository
> > has no HEAD reference. In fact, git-update-ref(1) would fail any request
> > to delete the HEAD reference, and a newly initialized repository always
> > pre-creates it, too.
> >
> > But in the next commit, we are going to change git-clone(1) to partially
> > initialize the refdb just up to the point where remote helpers can find
> > the repository. With that change, we are going to run into a situation
> > where repositories have no refs at all.
> >
> > Now there is a very particular edge case in this situation: when
> > preparing an empty ref transacton, we end up returning whatever value
> > `read_ref_without_reload()` returned to the caller. Under normal
> > conditions this would be fine: "HEAD" should usually exist, and thus the
> > function would return `0`. But if "HEAD" doesn't exist, the function
> > returns a positive value which we end up returning to the caller.
>
> In what context are reference transactions being created before the
> refdb is fully initialized? Is this in regards to repositories
> initialized with the reftables backend and used during execution of a
> remote-helper? I was originally under the impression that a partially
> initalized refdb would appear as the reffile backend before being fully
> initialized.
The only case where this can happen is during git-clone(1).
Patrick
> > Fix this bug by resetting the return code to `0` and add a test.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
>
> -Justin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] refs/reftable: don't fail empty transactions in repo without HEAD
From: Patrick Steinhardt @ 2024-03-04 6:44 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, Junio C Hamano, Mike Hommey
In-Reply-To: <CAOLa=ZSycN0iYbBP-rXKW5=tNJLaSd0q8+Vm=CzNfsP2nR0sJg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2845 bytes --]
On Wed, Feb 28, 2024 at 03:32:35AM -0800, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > Under normal circumstances, it shouldn't ever happen that a repository
> > has no HEAD reference. In fact, git-update-ref(1) would fail any request
> > to delete the HEAD reference, and a newly initialized repository always
> > pre-creates it, too.
> >
> > But in the next commit, we are going to change git-clone(1) to partially
> > initialize the refdb just up to the point where remote helpers can find
> > the repository. With that change, we are going to run into a situation
> > where repositories have no refs at all.
> >
> > Now there is a very particular edge case in this situation: when
> > preparing an empty ref transacton, we end up returning whatever value
> > `read_ref_without_reload()` returned to the caller. Under normal
> > conditions this would be fine: "HEAD" should usually exist, and thus the
> > function would return `0`. But if "HEAD" doesn't exist, the function
> > returns a positive value which we end up returning to the caller.
> >
> > Fix this bug by resetting the return code to `0` and add a test.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > refs/reftable-backend.c | 1 +
> > t/t0610-reftable-basics.sh | 13 +++++++++++++
> > 2 files changed, 14 insertions(+)
> >
> > diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> > index a14f2ad7f4..45568818f0 100644
> > --- a/refs/reftable-backend.c
> > +++ b/refs/reftable-backend.c
> > @@ -821,6 +821,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
> > &head_referent, &head_type);
> > if (ret < 0)
> > goto done;
> > + ret = 0;
> >
>
> So this is not really a problem in this function, it's more of that
> `refs.c:ref_transaction_prepare` checks if `ret` is non-zero.
Well, yes. I'd claim that it is a problem in this function because it
returns positive even though the transaction was prepared successfully.
> Nit: would be nice to have a comment about why overriding this value is
> ok.
True.
> > for (i = 0; i < transaction->nr; i++) {
> > struct ref_update *u = transaction->updates[i];
> > diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
> > index 6a131e40b8..c5f4d23433 100755
> > --- a/t/t0610-reftable-basics.sh
> > +++ b/t/t0610-reftable-basics.sh
> > @@ -328,6 +328,19 @@ test_expect_success 'ref transaction: writes are synced' '
> > EOF
> > '
> >
> > +test_expect_success 'ref transaction: empty transaction in empty repo' '
> > + test_when_finished "rm -rf repo" &&
> > + git init repo &&
> > + test_commit -C repo --no-tag A &&
> > + COMMIT=$(git -C repo rev-parse HEAD) &&
>
> why do we do this?
Oh, true, this isn't actually needed.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 0/2] builtin/clone: allow remote helpers to detect repo
From: Patrick Steinhardt @ 2024-03-04 6:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Mike Hommey
In-Reply-To: <xmqqh6htpp6k.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 1067 bytes --]
On Tue, Feb 27, 2024 at 01:33:55PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Patrick Steinhardt <ps@pks.im> writes:
> >
> >> Hi,
> >>
> >> this patch series addresses a regression reported by Mike in Git v2.44
> >> where remote helpers cannot access the Git repository anymore when
> >> running git-clone(1).
> >> ...
> >> builtin/clone.c | 46 ++++++++++++++++++++++++++++++++++++++
> >> refs/reftable-backend.c | 1 +
> >
> > Sorry, but this confuses me. Was a regression really in v2.44.0,
> > where refs/reftable-backend.c did not even exist? If so why does a
> > fix for it need to touch that file?
> >
> > Thanks.
>
> I guess [2/2] alone is the fix to be applied directly on top of v2.44.0
> and eventually be merged to 'maint' to become v2.44.1 release, while
> [1/2] is necessary to adjust reftable backend when such a fix is
> merged to more recent codebase that already has the reftable
> backend?
Yes, exactly! Sorry, should've explained this more thoroughly.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] builtin/clone: allow remote helpers to detect repo
From: Patrick Steinhardt @ 2024-03-04 6:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Mike Hommey
In-Reply-To: <xmqqle75ppa3.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 6140 bytes --]
On Tue, Feb 27, 2024 at 01:31:48PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > Instead, fix this issue by partially initializing the refdb up to the
> > point where it becomes discoverable by Git commands.
>
> As you mentioned earlier, this indeed is ugly, but I agree that
> there is no other reasonable way out. I am also unsure if this is a
> viable workaround in the longer term, but it should do for now.
>
> > + /*
> > + * We have a chicken-and-egg situation between initializing the refdb
> > + * and spawning transport helpers:
> > + *
> > + * - Initializing the refdb requires us to know about the object
> > + * format. We thus have to spawn the transport helper to learn
> > + * about it.
> > + * - The transport helper may want to access the Git repository. But
> > + * because the refdb has not been initialized, we don't have "HEAD"
> > + * or "refs/". Thus, the helper cannot find the Git repository.
> > + *
> > + * Ideally, we would have structured the helper protocol such that it's
> > + * mandatory for the helper to first announce its capabilities without
> > + * yet assuming a fully initialized repository. Like that, we could
> > + * have added a "lazy-refdb-init" capability that announces whether the
> > + * helper is ready to handle not-yet-initialized refdbs. If any helper
> > + * didn't support them, we would have fully initialized the refdb with
> > + * the SHA1 object format, but later on bailed out if we found out that
> > + * the remote repository used a different object format.
> > + * But we didn't, and thus we use the following workaround to partially
> > + * initialize the repository's refdb such that it can be discovered by
> > + * Git commands. To do so, we:
> > + *
> > + * - Create an invalid HEAD ref pointing at "refs/heads/.invalid".
> > + *
> > + * - Create the "refs/" directory.
> > + *
> > + * - Set up the ref storage format and repository version as
> > + * required.
>
> "as required" by whom and in what way?
>
> "The code to recognize a repository requires them to be set already,
> but they do not have to be the real value---we just assign random
> valid values for now, let remote helper do its work and then set the
> real values after they are done" would be a plausible interpretation
> of the above. Is that what is going on?
Partially. While we cannot yet determine the object format, we do know
the ref storage format as it is specified by the caller when invoking
git-clone(1) with the `--ref-format=` switch, not by the remote repo.
In that case, we'd have to set up the ref format accordingly and also
set the repository version to "1".
Patrick
> > + * This is sufficient for Git commands to discover the Git directory.
> > + */
> > + initialize_repository_version(GIT_HASH_UNKNOWN,
> > + the_repository->ref_storage_format, 1);
>
> OK, so we start with "we do not know it yet" here.
>
> > + strbuf_addf(&buf, "%s/HEAD", git_dir);
> > + write_file(buf.buf, "ref: refs/heads/.invalid");
> > +
> > + strbuf_reset(&buf);
> > + strbuf_addf(&buf, "%s/refs", git_dir);
> > + safe_create_dir(buf.buf, 1);
> > +
> > /*
> > * additional config can be injected with -c, make sure it's included
> > * after init_db, which clears the entire config environment.
> > @@ -1453,6 +1498,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> > free(remote_name);
> > strbuf_release(&reflog_msg);
> > strbuf_release(&branch_top);
> > + strbuf_release(&buf);
> > strbuf_release(&key);
> > free_refs(mapped_refs);
> > free_refs(remote_head_points_at);
> > diff --git a/setup.c b/setup.c
> > index b69b1cbc2a..e3b76e84b5 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -1889,6 +1889,13 @@ void initialize_repository_version(int hash_algo,
> > char repo_version_string[10];
> > int repo_version = GIT_REPO_VERSION;
> >
> > + /*
> > + * Note that we initialize the repository version to 1 when the ref
> > + * storage format is unknown. This is on purpose so that we can add the
> > + * correct object format to the config during git-clone(1). The format
> > + * version will get adjusted by git-clone(1) once it has learned about
> > + * the remote repository's format.
> > + */
> > if (hash_algo != GIT_HASH_SHA1 ||
> > ref_storage_format != REF_STORAGE_FORMAT_FILES)
> > repo_version = GIT_REPO_VERSION_READ;
> > @@ -1898,7 +1905,7 @@ void initialize_repository_version(int hash_algo,
> > "%d", repo_version);
> > git_config_set("core.repositoryformatversion", repo_version_string);
> >
> > - if (hash_algo != GIT_HASH_SHA1)
> > + if (hash_algo != GIT_HASH_SHA1 && hash_algo != GIT_HASH_UNKNOWN)
> > git_config_set("extensions.objectformat",
> > hash_algos[hash_algo].name);
> > else if (reinit)
>
> Here we refrain from writing extensions.objectformat in the
> repository when we do not know what hash function is being used.
> Without this change, we would instead remove extensions.objectformat,
> which does not sound too bad, as "reinit" is passed by the new call
> we saw above and this "else if (reinit)" will do exactly that anyway.
>
> In any case, after we finish talking with the other side over the
> transport, we make another call to initialize_repository_version()
> with the real objectformat and everything will be fine.
>
> The ealier description of the implementation idea made it sound
> really yucky, but I do not think the solution presented here is too
> bad.
>
> > diff --git a/t/t5801/git-remote-testgit b/t/t5801/git-remote-testgit
> > index 1544d6dc6b..bcfb358c51 100755
> > --- a/t/t5801/git-remote-testgit
> > +++ b/t/t5801/git-remote-testgit
> > @@ -12,6 +12,11 @@ url=$2
> >
> > dir="$GIT_DIR/testgit/$alias"
> >
> > +if ! git rev-parse --is-inside-git-dir
> > +then
> > + exit 1
> > +fi
> > +
> > h_refspec="refs/heads/*:refs/testgit/$alias/heads/*"
> > t_refspec="refs/tags/*:refs/testgit/$alias/tags/*"
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/3] unpack: replace xwrite() loop with write_in_full()
From: Patrick Steinhardt @ 2024-03-04 6:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20240302190348.3946569-2-gitster@pobox.com>
[-- Attachment #1: Type: text/plain, Size: 3345 bytes --]
On Sat, Mar 02, 2024 at 11:03:46AM -0800, Junio C Hamano wrote:
> We have two packfile stream consumers, index-pack and
> unpack-objects, that allow excess payload after the packfile stream
> data. Their code to relay excess data hasn't changed significantly
> since their original implementation that appeared in 67e5a5ec
> (git-unpack-objects: re-write to read from stdin, 2005-06-28) and
> 9bee2478 (mimic unpack-objects when --stdin is used with index-pack,
> 2006-10-25).
>
> These code blocks contain hand-rolled loops using xwrite(), written
> before our write_in_full() helper existed. This helper now provides
> the same functionality.
>
> Replace these loops with write_in_full() for shorter, clearer
> code. Update related variables accordingly.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> builtin/index-pack.c | 17 +++--------------
> builtin/unpack-objects.c | 8 +-------
> 2 files changed, 4 insertions(+), 21 deletions(-)
>
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index a3a37bd215..856428fef9 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1524,14 +1524,12 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
> struct strbuf pack_name = STRBUF_INIT;
> struct strbuf index_name = STRBUF_INIT;
> struct strbuf rev_index_name = STRBUF_INIT;
> - int err;
>
> if (!from_stdin) {
> close(input_fd);
> } else {
> fsync_component_or_die(FSYNC_COMPONENT_PACK, output_fd, curr_pack_name);
> - err = close(output_fd);
> - if (err)
> + if (close(output_fd))
> die_errno(_("error while closing pack file"));
> }
>
> @@ -1566,17 +1564,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
> write_or_die(1, buf.buf, buf.len);
> strbuf_release(&buf);
>
> - /*
> - * Let's just mimic git-unpack-objects here and write
> - * the last part of the input buffer to stdout.
> - */
> - while (input_len) {
> - err = xwrite(1, input_buffer + input_offset, input_len);
> - if (err <= 0)
> - break;
> - input_len -= err;
> - input_offset += err;
> - }
> + /* Write the last part of the buffer to stdout */
> + write_in_full(1, input_buffer + input_offset, input_len);
With this change we stop updating `input_len` and `input_offset`, both
of which are global variables. Assuming that tests pass this must be
okay right now given that this is the final part of what we are writing.
But I wonder whether we shouldn't update those regardless just so that
these remain consistent?
> }
>
> strbuf_release(&rev_index_name);
> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
> index e0a701f2b3..f1c85a00ae 100644
> --- a/builtin/unpack-objects.c
> +++ b/builtin/unpack-objects.c
> @@ -679,13 +679,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)
> use(the_hash_algo->rawsz);
>
> /* Write the last part of the buffer to stdout */
> - while (len) {
> - int ret = xwrite(1, buffer + offset, len);
> - if (ret <= 0)
> - break;
> - len -= ret;
> - offset += ret;
> - }
> + write_in_full(1, buffer + offset, len);
Same here.
Patrick
> /* All done */
> return has_errors;
> --
> 2.44.0-84-gb387623c12
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/3] unpack: replace xwrite() loop with write_in_full()
From: Junio C Hamano @ 2024-03-04 7:29 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <ZeVxA1KvN18Y85J_@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
>> - while (input_len) {
>> - err = xwrite(1, input_buffer + input_offset, input_len);
>> - if (err <= 0)
>> - break;
>> - input_len -= err;
>> - input_offset += err;
>> - }
>> + /* Write the last part of the buffer to stdout */
>> + write_in_full(1, input_buffer + input_offset, input_len);
>
> With this change we stop updating `input_len` and `input_offset`, both
> of which are global variables. Assuming that tests pass this must be
> okay right now given that this is the final part of what we are writing.
> But I wonder whether we shouldn't update those regardless just so that
> these remain consistent?
It is probably a good hygiene, even though it may not matter at all
for the correctness in the current code.
Thanks for your sharp eyes.
>> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
>> index e0a701f2b3..f1c85a00ae 100644
>> --- a/builtin/unpack-objects.c
>> +++ b/builtin/unpack-objects.c
>> @@ -679,13 +679,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)
>> use(the_hash_algo->rawsz);
>>
>> /* Write the last part of the buffer to stdout */
>> - while (len) {
>> - int ret = xwrite(1, buffer + offset, len);
>> - if (ret <= 0)
>> - break;
>> - len -= ret;
>> - offset += ret;
>> - }
>> + write_in_full(1, buffer + offset, len);
>
> Same here.
>
> Patrick
>
>> /* All done */
>> return has_errors;
>> --
>> 2.44.0-84-gb387623c12
>>
>>
^ permalink raw reply
* Re: [PATCH 0/4] some v2 capability advertisement cleanups
From: Patrick Steinhardt @ 2024-03-04 7:44 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20240228224625.GA1158651@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1394 bytes --]
On Wed, Feb 28, 2024 at 05:46:25PM -0500, Jeff King wrote:
> While working on another series, I noticed that upload-pack will accept
> the "packfile-uris" directive even if we didn't advertise it. That's not
> a huge deal in practice, but the spec says we're not supposed to. And
> while cleaning that up, I noticed a bit of duplication in the existing
> advertisement/allow code.
>
> So patches 1-3 clean up the situation a bit, and then patch 4 tightens
> up the packfile-uris handling.
This patch series was really easy to follow and feels like the right
thing to do. I've got a couple of nits, but none of them are important
enough to warrant a reroll.
Thanks!
Patrick
> There are some small textual conflicts with the series I just posted in:
>
> https://lore.kernel.org/git/20240228223700.GA1157826@coredump.intra.peff.net/
>
> I'm happy to prepare this on top of that if it's easier.
>
> [1/4]: upload-pack: use repository struct to get config
> [2/4]: upload-pack: centralize setup of sideband-all config
> [3/4]: upload-pack: use existing config mechanism for advertisement
> [4/4]: upload-pack: only accept packfile-uris if we advertised it
>
> t/t5702-protocol-v2.sh | 18 +++++++++++++
> upload-pack.c | 58 +++++++++++++++++++-----------------------
> 2 files changed, 44 insertions(+), 32 deletions(-)
>
> -Peff
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/4] upload-pack: use repository struct to get config
From: Patrick Steinhardt @ 2024-03-04 7:45 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20240228224647.GA1158898@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 2587 bytes --]
On Wed, Feb 28, 2024 at 05:46:47PM -0500, Jeff King wrote:
> Our upload_pack_v2() function gets a repository struct, but we ignore it
> totally. In practice this doesn't cause any problems, as it will never
> differ from the_repository. But in the spirit of taking a small step
> towards getting rid of the_repository, let's at least starting using it
Nit: s/starting/start
Patrick
> to grab config. There are probably other spots that could benefit, but
> it's a start.
>
> Note that we don't need to pass the repo for protected_config(); the
> whole point there is that we are not looking at repo config, so there is
> no repo-specific version of the function.
>
> For the v0 version of the protocol, we're not passed a repository
> struct, so we'll continue to use the_repository there.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> upload-pack.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/upload-pack.c b/upload-pack.c
> index 2537affa90..e156c1e472 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -1385,9 +1385,10 @@ static int upload_pack_protected_config(const char *var, const char *value,
> return 0;
> }
>
> -static void get_upload_pack_config(struct upload_pack_data *data)
> +static void get_upload_pack_config(struct repository *r,
> + struct upload_pack_data *data)
> {
> - git_config(upload_pack_config, data);
> + repo_config(r, upload_pack_config, data);
> git_protected_config(upload_pack_protected_config, data);
> }
>
> @@ -1398,7 +1399,7 @@ void upload_pack(const int advertise_refs, const int stateless_rpc,
> struct upload_pack_data data;
>
> upload_pack_data_init(&data);
> - get_upload_pack_config(&data);
> + get_upload_pack_config(the_repository, &data);
>
> data.stateless_rpc = stateless_rpc;
> data.timeout = timeout;
> @@ -1771,7 +1772,7 @@ enum fetch_state {
> FETCH_DONE,
> };
>
> -int upload_pack_v2(struct repository *r UNUSED, struct packet_reader *request)
> +int upload_pack_v2(struct repository *r, struct packet_reader *request)
> {
> enum fetch_state state = FETCH_PROCESS_ARGS;
> struct upload_pack_data data;
> @@ -1780,7 +1781,7 @@ int upload_pack_v2(struct repository *r UNUSED, struct packet_reader *request)
>
> upload_pack_data_init(&data);
> data.use_sideband = LARGE_PACKET_MAX;
> - get_upload_pack_config(&data);
> + get_upload_pack_config(r, &data);
>
> while (state != FETCH_DONE) {
> switch (state) {
> --
> 2.44.0.rc2.424.gbdbf4d014b
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 4/4] upload-pack: only accept packfile-uris if we advertised it
From: Patrick Steinhardt @ 2024-03-04 7:45 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20240228225050.GA1159078@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 4624 bytes --]
On Wed, Feb 28, 2024 at 05:50:50PM -0500, Jeff King wrote:
> Clients are only supposed to request particular capabilities or features
> if the server advertised them. For the "packfile-uris" feature, we only
> advertise it if uploadpack.blobpacfileuri is set, but we always accept a
Nit: s/uploadpack.blobpackfileuri. I noticed that this isn't actually
documented in git-config(1), but that's not a problem of this commit.
> request from the client regardless.
>
> In practice this doesn't really hurt anything, as we'd pass the client's
> protocol list on to pack-objects, which ends up ignoring it. But we
> should try to follow the protocol spec, and tightening this up may catch
> buggy or misbehaving clients more easily.
>
> Thanks to recent refactoring, we can hoist the config check from
Nit: s/refactoring/&s.
Patrick
> upload_pack_advertise() into upload_pack_config(). Note the subtle
> handling of a value-less bool (which does not count for triggering an
> advertisement).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I suspect in the long term that we may have other ways to trigger this
> feature than the static blobpackfileuri config (e.g., a hook that knows
> about site-specific packfiles "somehow"). So we may need to update the
> test later for that, but presumably in the vanilla config we'll continue
> to skip advertising it.
>
> t/t5702-protocol-v2.sh | 18 ++++++++++++++++++
> upload-pack.c | 16 +++++++---------
> 2 files changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index 6ef4971845..902e42c1c0 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -778,6 +778,24 @@ test_expect_success 'archive with custom path does not request v2' '
> ! grep ^GIT_PROTOCOL env.trace
> '
>
> +test_expect_success 'reject client packfile-uris if not advertised' '
> + {
> + packetize command=fetch &&
> + printf 0001 &&
> + packetize packfile-uris https &&
> + packetize done &&
> + printf 0000
> + } >input &&
> + test_must_fail env GIT_PROTOCOL=version=2 \
> + git upload-pack client <input &&
> + test_must_fail env GIT_PROTOCOL=version=2 \
> + git -c uploadpack.blobpackfileuri \
> + upload-pack client <input &&
> + GIT_PROTOCOL=version=2 \
> + git -c uploadpack.blobpackfileuri=anything \
> + upload-pack client <input
> +'
> +
> # Test protocol v2 with 'http://' transport
> #
> . "$TEST_DIRECTORY"/lib-httpd.sh
> diff --git a/upload-pack.c b/upload-pack.c
> index 491ef51daa..66f4de9d87 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -113,6 +113,7 @@ struct upload_pack_data {
> unsigned done : 1; /* v2 only */
> unsigned allow_ref_in_want : 1; /* v2 only */
> unsigned allow_sideband_all : 1; /* v2 only */
> + unsigned allow_packfile_uris : 1; /* v2 only */
> unsigned advertise_sid : 1;
> unsigned sent_capabilities : 1;
> };
> @@ -1362,6 +1363,9 @@ static int upload_pack_config(const char *var, const char *value,
> data->allow_ref_in_want = git_config_bool(var, value);
> } else if (!strcmp("uploadpack.allowsidebandall", var)) {
> data->allow_sideband_all = git_config_bool(var, value);
> + } else if (!strcmp("uploadpack.blobpackfileuri", var)) {
> + if (value)
> + data->allow_packfile_uris = 1;
> } else if (!strcmp("core.precomposeunicode", var)) {
> precomposed_unicode = git_config_bool(var, value);
> } else if (!strcmp("transfer.advertisesid", var)) {
> @@ -1647,7 +1651,8 @@ static void process_args(struct packet_reader *request,
> continue;
> }
>
> - if (skip_prefix(arg, "packfile-uris ", &p)) {
> + if (data->allow_packfile_uris &&
> + skip_prefix(arg, "packfile-uris ", &p)) {
> string_list_split(&data->uri_protocols, p, ',', -1);
> continue;
> }
> @@ -1847,8 +1852,6 @@ int upload_pack_advertise(struct repository *r,
> get_upload_pack_config(r, &data);
>
> if (value) {
> - char *str = NULL;
> -
> strbuf_addstr(value, "shallow wait-for-done");
>
> if (data.allow_filter)
> @@ -1860,13 +1863,8 @@ int upload_pack_advertise(struct repository *r,
> if (data.allow_sideband_all)
> strbuf_addstr(value, " sideband-all");
>
> - if (!repo_config_get_string(r,
> - "uploadpack.blobpackfileuri",
> - &str) &&
> - str) {
> + if (data.allow_packfile_uris)
> strbuf_addstr(value, " packfile-uris");
> - free(str);
> - }
> }
>
> upload_pack_data_clear(&data);
> --
> 2.44.0.rc2.424.gbdbf4d014b
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 0/4] osxkeychain: bring in line with other credential helpers
From: M Hickford @ 2024-03-04 8:00 UTC (permalink / raw)
To: Bo Anderson; +Cc: M Hickford, Bo Anderson via GitGitGadget, git
In-Reply-To: <CFC1A507-A9EF-4330-8C98-34C2B73BC036@boanderson.me>
On Sun, 18 Feb 2024 at 23:24, Bo Anderson <mail@boanderson.me> wrote:
>
> > On 18 Feb 2024, at 20:40, M Hickford <mirth.hickford@gmail.com> wrote:
> >
> >
> > Could these tests run as part of macOS CI?
>
> Do we do so for any of the other external credential helpers?
>
We don't.
> It definitely makes sense in principle. Though the concern perhaps will be that any new features added to the credential helpers and thus its test suite would need adding to each credential helper simultaneously to avoid failing CI. Ideally we would do exactly that, though that requires knowledge on each of the keystore APIs used in each of the credential helpers.
Good point.
^ permalink raw reply
* Re: [PATCH 9/9] upload-pack: free tree buffers after parsing
From: Patrick Steinhardt @ 2024-03-04 8:33 UTC (permalink / raw)
To: Jeff King; +Cc: git, Benjamin Flesch
In-Reply-To: <20240228223907.GI1158131@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 8793 bytes --]
On Wed, Feb 28, 2024 at 05:39:07PM -0500, Jeff King wrote:
> When a client sends us a "want" or "have" line, we call parse_object()
> to get an object struct. If the object is a tree, then the parsed state
> means that tree->buffer points to the uncompressed contents of the tree.
> But we don't really care about it. We only really need to parse commits
> and tags; for trees and blobs, the important output is just a "struct
> object" with the correct type.
>
> But much worse, we do not ever free that tree buffer. It's not leaked in
> the traditional sense, in that we still have a pointer to it from the
> global object hash. But if the client requests many trees, we'll hold
> all of their contents in memory at the same time.
>
> Nobody really noticed because it's rare for clients to directly request
> a tree. It might happen for a lightweight tag pointing straight at a
> tree, or it might happen for a "tree:depth" partial clone filling in
> missing trees.
>
> But it's also possible for a malicious client to request a lot of trees,
> causing upload-pack's memory to balloon. For example, without this
> patch, requesting every tree in git.git like:
>
> pktline() {
> local msg="$*"
> printf "%04x%s\n" $((1+4+${#msg})) "$msg"
> }
>
> want_trees() {
> pktline command=fetch
> printf 0001
> git cat-file --batch-all-objects --batch-check='%(objectname) %(objecttype)' |
> while read oid type; do
> test "$type" = "tree" || continue
> pktline want $oid
> done
> pktline done
> printf 0000
> }
>
> want_trees | GIT_PROTOCOL=version=2 valgrind --tool=massif ./git upload-pack . >/dev/null
>
> shows a peak heap usage of ~3.7GB. Which is just about the sum of the
> sizes of all of the uncompressed trees. For linux.git, it's closer to
> 17GB.
>
> So the obvious thing to do is to call free_tree_buffer() after we
> realize that we've parsed a tree. We know that upload-pack won't need it
> later. But let's push the logic into parse_object_with_flags(), telling
> it to discard the tree buffer immediately. There are two reasons for
> this. One, all of the relevant call-sites already call the with_options
> variant to pass the SKIP_HASH flag. So it actually ends up as less code
> than manually free-ing in each spot. And two, it enables an extra
> optimization that I'll discuss below.
>
> I've touched all of the sites that currently use SKIP_HASH in
> upload-pack. That drops the peak heap of the upload-pack invocation
> above from 3.7GB to ~24MB.
>
> I've also modified the caller in get_reference(); a partial clone
> benefits from its use in pack-objects for the reasons given in
> 0bc2557951 (upload-pack: skip parse-object re-hashing of "want" objects,
> 2022-09-06), where we were measuring blob requests. But note that the
> results of get_reference() are used for traversing, as well; so we
> really would _eventually_ use the tree contents. That makes this at
> first glance a space/time tradeoff: we won't hold all of the trees in
> memory at once, but we'll have to reload them each when it comes time to
> traverse.
>
> And here's where our extra optimization comes in. If the caller is not
> going to immediately look at the tree contents, and it doesn't care
> about checking the hash, then parse_object() can simply skip loading the
> tree entirely, just like we do for blobs! And now it's not a space/time
> tradeoff in get_reference() anymore. It's just a lazy-load: we're
> delaying reading the tree contents until it's time to actually traverse
> them one by one.
>
> And of course for upload-pack, this optimization means we never load the
> trees at all, saving lots of CPU time. Timing the "every tree from
> git.git" request above shows upload-pack dropping from 32 seconds of CPU
> to 19 (the remainder is mostly due to pack-objects actually sending the
> pack; timing just the upload-pack portion shows we go from 13s to
> ~0.28s).
>
> These are all highly gamed numbers, of course. For real-world
> partial-clone requests we're saving only a small bit of time in
> practice. But it does help harden upload-pack against malicious
> denial-of-service attacks.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> object.c | 14 ++++++++++++++
> object.h | 1 +
> revision.c | 3 ++-
> upload-pack.c | 9 ++++++---
> 4 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/object.c b/object.c
> index e6a1c4d905..f11c59ac0c 100644
> --- a/object.c
> +++ b/object.c
> @@ -271,6 +271,7 @@ struct object *parse_object_with_flags(struct repository *r,
> enum parse_object_flags flags)
> {
> int skip_hash = !!(flags & PARSE_OBJECT_SKIP_HASH_CHECK);
> + int discard_tree = !!(flags & PARSE_OBJECT_DISCARD_TREE);
> unsigned long size;
> enum object_type type;
> int eaten;
> @@ -298,6 +299,17 @@ struct object *parse_object_with_flags(struct repository *r,
> return lookup_object(r, oid);
> }
>
> + /*
> + * If the caller does not care about the tree buffer and does not
> + * care about checking the hash, we can simply verify that we
> + * have the on-disk object with the correct type.
> + */
> + if (skip_hash && discard_tree &&
> + (!obj || obj->type == OBJ_TREE) &&
> + oid_object_info(r, oid, NULL) == OBJ_TREE) {
> + return &lookup_tree(r, oid)->object;
> + }
The other condition for blobs does the same, but the condition here
confuses me. Why do we call `oid_object_info()` if we have already
figured out that `obj->type == OBJ_TREE`? Feels like wasted effort if
the in-memory object has been determined to be a tree already anyway.
I'd rather have expected it to look like the following:
if (skip_hash && discard_tree &&
((obj && obj->type == OBJ_TREE) ||
(!obj && oid_object_info(r, oid, NULL)) == OBJ_TREE)) {
return &lookup_tree(r, oid)->object;
}
Am I missing some side effect that `oid_object_info()` provides?
Patrick
> buffer = repo_read_object_file(r, oid, &type, &size);
> if (buffer) {
> if (!skip_hash &&
> @@ -311,6 +323,8 @@ struct object *parse_object_with_flags(struct repository *r,
> buffer, &eaten);
> if (!eaten)
> free(buffer);
> + if (discard_tree && type == OBJ_TREE)
> + free_tree_buffer((struct tree *)obj);
> return obj;
> }
> return NULL;
> diff --git a/object.h b/object.h
> index 114d45954d..c7123cade6 100644
> --- a/object.h
> +++ b/object.h
> @@ -197,6 +197,7 @@ void *object_as_type(struct object *obj, enum object_type type, int quiet);
> */
> enum parse_object_flags {
> PARSE_OBJECT_SKIP_HASH_CHECK = 1 << 0,
> + PARSE_OBJECT_DISCARD_TREE = 1 << 1,
> };
> struct object *parse_object(struct repository *r, const struct object_id *oid);
> struct object *parse_object_with_flags(struct repository *r,
> diff --git a/revision.c b/revision.c
> index 2424c9bd67..b10f63a607 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -381,7 +381,8 @@ static struct object *get_reference(struct rev_info *revs, const char *name,
>
> object = parse_object_with_flags(revs->repo, oid,
> revs->verify_objects ? 0 :
> - PARSE_OBJECT_SKIP_HASH_CHECK);
> + PARSE_OBJECT_SKIP_HASH_CHECK |
> + PARSE_OBJECT_DISCARD_TREE);
>
> if (!object) {
> if (revs->ignore_missing)
> diff --git a/upload-pack.c b/upload-pack.c
> index b721155442..761af4a532 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -470,7 +470,8 @@ static int do_got_oid(struct upload_pack_data *data, const struct object_id *oid
> {
> int we_knew_they_have = 0;
> struct object *o = parse_object_with_flags(the_repository, oid,
> - PARSE_OBJECT_SKIP_HASH_CHECK);
> + PARSE_OBJECT_SKIP_HASH_CHECK |
> + PARSE_OBJECT_DISCARD_TREE);
>
> if (!o)
> die("oops (%s)", oid_to_hex(oid));
> @@ -1150,7 +1151,8 @@ static void receive_needs(struct upload_pack_data *data,
> }
>
> o = parse_object_with_flags(the_repository, &oid_buf,
> - PARSE_OBJECT_SKIP_HASH_CHECK);
> + PARSE_OBJECT_SKIP_HASH_CHECK |
> + PARSE_OBJECT_DISCARD_TREE);
> if (!o) {
> packet_writer_error(&data->writer,
> "upload-pack: not our ref %s",
> @@ -1467,7 +1469,8 @@ static int parse_want(struct packet_writer *writer, const char *line,
> "expected to get oid, not '%s'", line);
>
> o = parse_object_with_flags(the_repository, &oid,
> - PARSE_OBJECT_SKIP_HASH_CHECK);
> + PARSE_OBJECT_SKIP_HASH_CHECK |
> + PARSE_OBJECT_DISCARD_TREE);
>
> if (!o) {
> packet_writer_error(writer,
> --
> 2.44.0.rc2.424.gbdbf4d014b
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 6/9] upload-pack: disallow object-info capability by default
From: Patrick Steinhardt @ 2024-03-04 8:34 UTC (permalink / raw)
To: Jeff King; +Cc: git, Benjamin Flesch
In-Reply-To: <20240228223858.GF1158131@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 6335 bytes --]
On Wed, Feb 28, 2024 at 05:38:58PM -0500, Jeff King wrote:
> From: Taylor Blau <me@ttaylorr.com>
>
> We added an "object-info" capability to the v2 upload-pack protocol in
> a2ba162cda (object-info: support for retrieving object info,
> 2021-04-20). In the almost 3 years since, we have not added any
> client-side support, and it does not appear to exist in other
> implementations either (JGit understands the verb on the server side,
> but not on the client side).
>
> Since this largely unused code is accessible over the network by
> default, it increases the attack surface of upload-pack. I don't know of
> any particularly severe problem, but one issue is that because of the
> request/response nature of the v2 protocol, it will happily read an
> unbounded number of packets, adding each one to a string list (without
> regard to whether they are objects we know about, duplicates, etc).
>
> This may be something we want to improve in the long run, but in the
> short term it makes sense to disable the feature entirely. We'll add a
> config option as an escape hatch for anybody who wants to develop the
> feature further.
>
> A more gentle option would be to add the config option to let people
> disable it manually, but leave it enabled by default. But given that
> there's no client side support, that seems like the wrong balance with
> security.
>
> Disabling by default will slow adoption a bit once client-side support
> does become available (there were some patches[1] in 2022, but nothing
> got merged and there's been nothing since). But clients have to deal
> with older servers that do not understand the option anyway (and the
> capability system handles that), so it will just be a matter of servers
> flipping their config at that point (and hopefully once any unbounded
> allocations have been addressed).
>
> [jk: this is a patch that GitHub has been running for several years, but
> rebased forward and with a new commit message for upstream]
>
> [1] https://lore.kernel.org/git/20220208231911.725273-1-calvinwan@google.com/
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Documentation/config/transfer.txt | 4 ++++
> serve.c | 14 +++++++++++++-
> t/t5555-http-smart-common.sh | 1 -
> t/t5701-git-serve.sh | 24 +++++++++++++++++++++++-
> 4 files changed, 40 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/config/transfer.txt b/Documentation/config/transfer.txt
> index a9cbdb88a1..f1ce50f4a6 100644
> --- a/Documentation/config/transfer.txt
> +++ b/Documentation/config/transfer.txt
> @@ -121,3 +121,7 @@ transfer.bundleURI::
> information from the remote server (if advertised) and download
> bundles before continuing the clone through the Git protocol.
> Defaults to `false`.
> +
> +transfer.advertiseObjectInfo::
> + When `true`, the `object-info` capability is advertised by
> + servers. Defaults to false.
> diff --git a/serve.c b/serve.c
> index a1d71134d4..aa651b73e9 100644
> --- a/serve.c
> +++ b/serve.c
> @@ -12,6 +12,7 @@
> #include "trace2.h"
>
> static int advertise_sid = -1;
> +static int advertise_object_info = -1;
> static int client_hash_algo = GIT_HASH_SHA1;
>
> static int always_advertise(struct repository *r UNUSED,
> @@ -67,6 +68,17 @@ static void session_id_receive(struct repository *r UNUSED,
> trace2_data_string("transfer", NULL, "client-sid", client_sid);
> }
>
> +static int object_info_advertise(struct repository *r, struct strbuf *value UNUSED)
> +{
> + if (advertise_object_info == -1 &&
> + repo_config_get_bool(r, "transfer.advertiseobjectinfo",
> + &advertise_object_info)) {
> + /* disabled by default */
> + advertise_object_info = 0;
> + }
> + return advertise_object_info;
> +}
> +
> struct protocol_capability {
> /*
> * The name of the capability. The server uses this name when
> @@ -135,7 +147,7 @@ static struct protocol_capability capabilities[] = {
> },
> {
> .name = "object-info",
> - .advertise = always_advertise,
> + .advertise = object_info_advertise,
> .command = cap_object_info,
> },
> {
> diff --git a/t/t5555-http-smart-common.sh b/t/t5555-http-smart-common.sh
> index b1cfe8b7db..3dcb3340a3 100755
> --- a/t/t5555-http-smart-common.sh
> +++ b/t/t5555-http-smart-common.sh
> @@ -131,7 +131,6 @@ test_expect_success 'git upload-pack --advertise-refs: v2' '
> fetch=shallow wait-for-done
> server-option
> object-format=$(test_oid algo)
> - object-info
> 0000
> EOF
>
> diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
> index 3591bc2417..c48830de8f 100755
> --- a/t/t5701-git-serve.sh
> +++ b/t/t5701-git-serve.sh
> @@ -20,7 +20,6 @@ test_expect_success 'test capability advertisement' '
> fetch=shallow wait-for-done
> server-option
> object-format=$(test_oid algo)
> - object-info
> EOF
> cat >expect.trailer <<-EOF &&
> 0000
> @@ -323,6 +322,8 @@ test_expect_success 'unexpected lines are not allowed in fetch request' '
> # Test the basics of object-info
> #
> test_expect_success 'basics of object-info' '
> + test_config transfer.advertiseObjectInfo true &&
> +
> test-tool pkt-line pack >in <<-EOF &&
> command=object-info
> object-format=$(test_oid algo)
> @@ -380,4 +381,25 @@ test_expect_success 'basics of bundle-uri: dies if not enabled' '
> test_must_be_empty out
> '
>
> +test_expect_success 'object-info missing from capabilities when disabled' '
> + test_config transfer.advertiseObjectInfo false &&
> +
> + GIT_TEST_SIDEBAND_ALL=0 test-tool serve-v2 \
> + --advertise-capabilities >out &&
> + test-tool pkt-line unpack <out >actual &&
> +
> + ! grep object.info actual
> +'
Is it intentional that you grep for "object.info" instead of
"object-info"?
Patrick
> +test_expect_success 'object-info commands rejected when disabled' '
> + test_config transfer.advertiseObjectInfo false &&
> +
> + test-tool pkt-line pack >in <<-EOF &&
> + command=object-info
> + EOF
> +
> + test_must_fail test-tool serve-v2 --stateless-rpc <in 2>err &&
> + grep invalid.command err
> +'
> +
> test_done
> --
> 2.44.0.rc2.424.gbdbf4d014b
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: GSoC 2024
From: Christian Couder @ 2024-03-04 8:37 UTC (permalink / raw)
To: Aishwarya Narayanan; +Cc: git
In-Reply-To: <CAHCXyj3agZKxgseGwS-1WD3NR1nD7ni80Q5UEhf2EVpMiaSV4A@mail.gmail.com>
Hi,
On Mon, Mar 4, 2024 at 7:02 AM Aishwarya Narayanan
<aishnana.03@gmail.com> wrote:
>
> Dear Git Organization,
>
> I am writing to express my strong interest in participating in Google
> Summer of Code (GSoC) 2024 with the Git Organization. As a highly
> motivated Final-year Electrical and Electronics Engineer, I am eager
> to contribute to the open-source community and learn from experienced
> developers like yourselves.
Thanks for your interest in contributing to Git!
> While this is my first foray into open-source development and GSoC, I
> am excited by the prospect of contributing to the ongoing success of
> Git. I have been actively learning and gaining a strong foundation in
> Git commands, programming languages, and version control concepts. I
> am particularly interested in the following project ideas:
> 1) Implement consistency checks for refs
> 2) Move existing tests to a unit testing framework
>
> However, I understand the importance of starting with smaller
> contributions before taking on a larger project like GSoC. I am eager
> to begin by contributing to micro-projects and familiarizing myself
> with the Git codebase and development workflow.
>
> In this regard, I would greatly appreciate any guidance you can offer
> on where to find existing micro-projects and how to get involved in
> the Git community. Additionally, I would be grateful for any resources
> or suggestions that can help me prepare for GSoC and submit a strong
> proposal.
Please take a look at the following web pages:
- https://git.github.io/General-Microproject-Information/
- https://git.github.io/SoC-2024-Microprojects/
Thanks!
^ permalink raw reply
* [PATCH] Fix git-p4 decode() missing an assignment
From: W Sero via GitGitGadget @ 2024-03-04 8:54 UTC (permalink / raw)
To: git; +Cc: W Sero, SaNeOr
From: SaNeOr <sane0r@outlook.com>
bugfix: When using git-p4 in the python2 environment,
some places decode() missing an assignment.
Signed-off-by: W Sero <sane0r@outlook.com>
---
Fix git-p4 decode_path() missing an assignment
When using git-p4 in the python2 environment, some places decode( )
missing an assignment.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1683%2FSaNeOr%2Fmaster-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1683/SaNeOr/master-v1
Pull-Request: https://github.com/git/git/pull/1683
git-p4.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/git-p4.py b/git-p4.py
index 28ab12c72b6..9fa4b9b104e 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -307,7 +307,7 @@ def decode_path(path):
return path.decode(encoding, errors='replace') if isinstance(path, bytes) else path
else:
try:
- path.decode('ascii')
+ path = path.decode('ascii')
except:
path = path.decode(encoding, errors='replace')
if verbose:
@@ -3114,7 +3114,7 @@ def writeToGitStream(self, gitMode, relPath, contents):
def encodeWithUTF8(self, path):
try:
- path.decode('ascii')
+ path = path.decode('ascii')
except:
encoding = 'utf8'
if gitConfig('git-p4.pathEncoding'):
base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
--
gitgitgadget
^ permalink raw reply related
* Re: [GSOC][PATCH] userdiff: Add JavaScript function patterns
From: Patrick Steinhardt @ 2024-03-04 9:04 UTC (permalink / raw)
To: Sergius Nyah; +Cc: git, christian.couder, gitster, pk, shyamthakkar001
In-Reply-To: <20240301074048.188835-1-sergiusnyah@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2914 bytes --]
On Fri, Mar 01, 2024 at 08:40:48AM +0100, Sergius Nyah wrote:
> This commit adds a patterns used to match JavaScript functions.
> It now correctly identifies function declarations, function expressions,
> and functions defined inside blocks. Add test for corresponding change in userdiff.
>
> Signed-off-by: Sergius Nyah <sergiusnyah@gmail.com>
> ---
> t/t4018-diff-funcname.sh | 22 ++++++++++++++++++++++
> userdiff.c | 12 ++++++++++++
> 2 files changed, 34 insertions(+)
>
> diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
> index e026fac1f4..d35cce18a0 100755
> --- a/t/t4018-diff-funcname.sh
> +++ b/t/t4018-diff-funcname.sh
> @@ -120,3 +120,25 @@ do
> done
>
> test_done
> +
> +test_expect_success 'identify builtin patterns in JavaScript' '
> + # setup
> + echo "function myFunction() { return true; }" > test.js &&
> + echo "var myVar = function() { return false; }" >> test.js &&
> + git add test.js &&
> + git commit -m "add test.js" &&
> +
> + # modify the file
> + echo "function myFunction() { return false; }" > test.js &&
> + echo "var myVar = function() { return true; }" >> test.js &&
> +
> + # command under test
> + git diff >output &&
> +
> + # check results
> + test_i18ngrep "function myFunction() { return true; }" output &&
> + test_i18ngrep "function myFunction() { return false; }" output &&
> + test_i18ngrep "var myVar = function() { return false; }" output &&
> + test_i18ngrep "var myVar = function() { return true; }" output
> +'
> +test_done
> \ No newline at end of file
This `test_done` only needs to be added because you add the new test
before the preceding `test_done`. Instead, you should move up this test
so that it comes before it.
> diff --git a/userdiff.c b/userdiff.c
> index 2b1dab2649..bbe2bcb9a3 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> +PATTERNS("javascript",
> + /* Looks for lines that start with optional whitespace, followed
Multi-line comments should start with their delimiters on separate
lines. So the "/*" should be on its own line.
Also, the code should be indented with tabs and not spaces. It might
help to read through Documentation/CodingGuidelines to get more familiar
with Git's coding style.
Patrick
> + * by 'function'* and any characters (for function declarations),
> + * or valid JavaScript identifiers, equals sign '=', 'function' keyword
> + * and any characters (for function expressions).
> + * Also considers functions defined inside blocks with '{...}'.
> + */
> + "^[ \t]*(function[ \t]*.*|[a-zA-Z_$][0-9a-zA-Z_$]*[ \t]*=[ \t]*function[ \t]*.*|(\\{[ \t]*)?)\n",
> + /* This pattern matches JavaScript identifiers */
> + "[a-zA-Z_$][0-9a-zA-Z_$]*"
> + "|[-+0-9.eE]+|0[xX][0-9a-fA-F]+"
> + "|[-+*/<>%&^|=!:]=|--|\\+\\+|<<=?|>>=?|&&|\\|\\|"),
> --
> 2.43.2
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ 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