* git-config doc: variables sorting bug
From: Bruno Haible @ 2024-02-28 10:12 UTC (permalink / raw)
To: git
Hi,
In https://git-scm.com/docs/git-config the variables (advice.* ...
worktree.guessRemote) are apparently meant to be in alphabetical order.
However,
filter.<driver>.clean
filter.<driver>.smudge
come after format.*. They should come before format.*, since 'i' < 'o'.
Bruno
^ permalink raw reply
* Re: [PATCH 1/2] refs/reftable: don't fail empty transactions in repo without HEAD
From: Karthik Nayak @ 2024-02-28 11:32 UTC (permalink / raw)
To: Patrick Steinhardt, git; +Cc: Junio C Hamano, Mike Hommey
In-Reply-To: <95be968e10bd02c64448786e690bbefe5c082577.1709041721.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2395 bytes --]
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.
Nit: would be nice to have a comment about why overriding this value is
ok.
> 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?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: What's cooking in git.git (Feb 2024, #09; Tue, 27)
From: Philippe Blain @ 2024-02-28 13:38 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <xmqqjzmpm9b8.fsf@gitster.g>
Hi Junio,
Le 2024-02-28 à 00:44, Junio C Hamano a écrit :
> * ml/log-merge-with-cherry-pick-and-other-pseudo-heads (2024-02-27) 2 commits
> - revision: implement `git log --merge` also for rebase/cherry-pick/revert
> - revision: ensure MERGE_HEAD is a ref in prepare_show_merge
>
> "git log --merge" learned to pay attention to CHERRY_PICK_HEAD and
> other kinds of *_HEAD pseudorefs.
>
> Will merge to 'next'?
> source: <20240225-ml-log-merge-with-cherry-pick-and-other-pseudo-heads-v5-0-af1ef2d9e44d@gmail.com>
I'll send a new version with a reworked error message, so best to wait.
Thanks,
Philippe.
^ permalink raw reply
* [PATCH v6 0/2] Implement `git log --merge` also for rebase/cherry-pick/revert
From: Philippe Blain @ 2024-02-28 13:54 UTC (permalink / raw)
To: git
Cc: Johannes Sixt, Elijah Newren, Michael Lohmann, Phillip Wood,
Patrick Steinhardt, Junio C Hamano, Michael Lohmann,
Philippe Blain
In-Reply-To: <20240225-ml-log-merge-with-cherry-pick-and-other-pseudo-heads-v5-0-af1ef2d9e44d@gmail.com>
Changes in v6:
- Changed the error message added in 1/2 and adjusted in 2/2 to avoid a rhetorical
question
Changes in v5:
- Marked error messages for translation and tweaked them as suggested by Phillip
- Reworded the message of 2/2 as suggested by Phillip
- Removed the change to gitk's doc in 2/2 as pointed out by Johannes
- Fixed the trailers in 2/2
- Improved the doc in 2/2 as suggested by Phillip and Jean-Noël
Changes in v4:
- Added a commit message for 2/2 detailing the use case and summarizing the discussion in the thread
- Adjusted the documentation of the option
---
Michael Lohmann (2):
revision: ensure MERGE_HEAD is a ref in prepare_show_merge
revision: implement `git log --merge` also for rebase/cherry-pick/revert
Documentation/rev-list-options.txt | 7 +++++--
revision.c | 27 +++++++++++++++++++++++----
2 files changed, 28 insertions(+), 6 deletions(-)
---
base-commit: 3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0
change-id: 20240210-ml-log-merge-with-cherry-pick-and-other-pseudo-heads-05bd8e8797db
Range-diff versus v5:
1: c9536431d1 ! 1: 363657561c revision: ensure MERGE_HEAD is a ref in prepare_show_merge
@@ Commit message
(2) obtain the oid without any prefixing by refs.c:repo_dwim_ref()
(3) error out when MERGE_HEAD is a symref.
+ Note that we avoid marking the new error message for translation as it
+ will be done in the next commit when the message is generalized to other
+ special refs.
+
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
@@ revision.c: static void prepare_show_merge(struct rev_info *revs)
+ &oid, NULL))
die("--merge without MERGE_HEAD?");
+ if (is_null_oid(&oid))
-+ die(_("MERGE_HEAD is a symbolic ref?"));
++ die(_("MERGE_HEAD exists but is a symbolic ref"));
other = lookup_commit_or_die(&oid, "MERGE_HEAD");
add_pending_object(revs, &head->object, "HEAD");
add_pending_object(revs, &other->object, "MERGE_HEAD");
2: 1641c4be81 ! 2: 749abadc04 revision: implement `git log --merge` also for rebase/cherry-pick/revert
@@ revision.c: static void add_pending_commit_list(struct rev_info *revs,
+ RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
+ oid, NULL)) {
+ if (is_null_oid(oid))
-+ die(_("%s is a symbolic ref?"), other_head[i]);
++ die(_("%s exists but is a symbolic ref"), other_head[i]);
+ return other_head[i];
+ }
+
@@ revision.c: static void prepare_show_merge(struct rev_info *revs)
- &oid, NULL))
- die("--merge without MERGE_HEAD?");
- if (is_null_oid(&oid))
-- die(_("MERGE_HEAD is a symbolic ref?"));
+- die(_("MERGE_HEAD exists but is a symbolic ref"));
- other = lookup_commit_or_die(&oid, "MERGE_HEAD");
+ other_name = lookup_other_head(&oid);
+ other = lookup_commit_or_die(&oid, other_name);
^ permalink raw reply
* [PATCH v6 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge
From: Philippe Blain @ 2024-02-28 13:54 UTC (permalink / raw)
To: git
Cc: Johannes Sixt, Elijah Newren, Michael Lohmann, Phillip Wood,
Patrick Steinhardt, Junio C Hamano, Michael Lohmann,
Philippe Blain
In-Reply-To: <20240228-ml-log-merge-with-cherry-pick-and-other-pseudo-heads-v6-0-8ec34c052b39@gmail.com>
From: Michael Lohmann <mi.al.lohmann@gmail.com>
This is done to
(1) ensure MERGE_HEAD is a ref,
(2) obtain the oid without any prefixing by refs.c:repo_dwim_ref()
(3) error out when MERGE_HEAD is a symref.
Note that we avoid marking the new error message for translation as it
will be done in the next commit when the message is generalized to other
special refs.
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
revision.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/revision.c b/revision.c
index 2424c9bd67..df775f74d0 100644
--- a/revision.c
+++ b/revision.c
@@ -1973,8 +1973,12 @@ static void prepare_show_merge(struct rev_info *revs)
if (repo_get_oid(the_repository, "HEAD", &oid))
die("--merge without HEAD?");
head = lookup_commit_or_die(&oid, "HEAD");
- if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
+ if (read_ref_full("MERGE_HEAD",
+ RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
+ &oid, NULL))
die("--merge without MERGE_HEAD?");
+ if (is_null_oid(&oid))
+ die(_("MERGE_HEAD exists but is a symbolic ref"));
other = lookup_commit_or_die(&oid, "MERGE_HEAD");
add_pending_object(revs, &head->object, "HEAD");
add_pending_object(revs, &other->object, "MERGE_HEAD");
--
2.39.1
^ permalink raw reply related
* [PATCH v6 2/2] revision: implement `git log --merge` also for rebase/cherry-pick/revert
From: Philippe Blain @ 2024-02-28 13:54 UTC (permalink / raw)
To: git
Cc: Johannes Sixt, Elijah Newren, Michael Lohmann, Phillip Wood,
Patrick Steinhardt, Junio C Hamano, Michael Lohmann,
Philippe Blain
In-Reply-To: <20240228-ml-log-merge-with-cherry-pick-and-other-pseudo-heads-v6-0-8ec34c052b39@gmail.com>
From: Michael Lohmann <mi.al.lohmann@gmail.com>
'git log' learned in ae3e5e1ef2 (git log -p --merge [[--] paths...],
2006-07-03) to show commits touching conflicted files in the range
HEAD...MERGE_HEAD, an addition documented in d249b45547 (Document
rev-list's option --merge, 2006-08-04).
It can be useful to look at the commit history to understand what lead
to merge conflicts also for other mergy operations besides merges, like
cherry-pick, revert and rebase.
For rebases and cherry-picks, an interesting range to look at is
HEAD...{REBASE_HEAD,CHERRY_PICK_HEAD}, since even if all the commits
included in that range are not directly part of the 3-way merge,
conflicts encountered during these operations can indeed be caused by
changes introduced in preceding commits on both sides of the history.
For revert, as we are (most likely) reversing changes from a previous
commit, an appropriate range is REVERT_HEAD..HEAD, which is equivalent
to REVERT_HEAD...HEAD and to HEAD...REVERT_HEAD, if we keep HEAD and its
parents on the left side of the range.
As such, adjust the code in prepare_show_merge so it constructs the
range HEAD...$OTHER for OTHER={MERGE_HEAD, CHERRY_PICK_HEAD, REVERT_HEAD
or REBASE_HEAD}. Note that we try these pseudorefs in order, so keep
REBASE_HEAD last since the three other operations can be performed
during a rebase. Note also that in the uncommon case where $OTHER and
HEAD do not share a common ancestor, this will show the complete
histories of both sides since their root commits, which is the same
behaviour as currently happens in that case for HEAD and MERGE_HEAD.
Adjust the documentation of this option accordingly.
Co-authored-by: Johannes Sixt <j6t@kdbg.org>
Co-authored-by: Philippe Blain <levraiphilippeblain@gmail.com>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
[jc: tweaked in j6t's precedence fix that tries REBASE_HEAD last]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
Documentation/rev-list-options.txt | 7 +++++--
revision.c | 31 +++++++++++++++++++++++--------
2 files changed, 28 insertions(+), 10 deletions(-)
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index a583b52c61..29e7b50bcf 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -341,8 +341,11 @@ See also linkgit:git-reflog[1].
Under `--pretty=reference`, this information will not be shown at all.
--merge::
- After a failed merge, show refs that touch files having a
- conflict and don't exist on all heads to merge.
+ Show commits touching conflicted paths in the range `HEAD...<other>`,
+ where `<other>` is the first existing pseudoref in `MERGE_HEAD`,
+ `CHERRY_PICK_HEAD`, `REVERT_HEAD` or `REBASE_HEAD`. Only works
+ when the index has unmerged entries. This option can be used to show
+ relevant commits when resolving conflicts from a 3-way merge.
--boundary::
Output excluded boundary commits. Boundary commits are
diff --git a/revision.c b/revision.c
index df775f74d0..d2ebdd045a 100644
--- a/revision.c
+++ b/revision.c
@@ -1961,11 +1961,31 @@ static void add_pending_commit_list(struct rev_info *revs,
}
}
+static const char *lookup_other_head(struct object_id *oid)
+{
+ int i;
+ static const char *const other_head[] = {
+ "MERGE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD", "REBASE_HEAD"
+ };
+
+ for (i = 0; i < ARRAY_SIZE(other_head); i++)
+ if (!read_ref_full(other_head[i],
+ RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
+ oid, NULL)) {
+ if (is_null_oid(oid))
+ die(_("%s exists but is a symbolic ref"), other_head[i]);
+ return other_head[i];
+ }
+
+ die(_("--merge requires one of the pseudorefs MERGE_HEAD, CHERRY_PICK_HEAD, REVERT_HEAD or REBASE_HEAD"));
+}
+
static void prepare_show_merge(struct rev_info *revs)
{
struct commit_list *bases;
struct commit *head, *other;
struct object_id oid;
+ const char *other_name;
const char **prune = NULL;
int i, prune_num = 1; /* counting terminating NULL */
struct index_state *istate = revs->repo->index;
@@ -1973,15 +1993,10 @@ static void prepare_show_merge(struct rev_info *revs)
if (repo_get_oid(the_repository, "HEAD", &oid))
die("--merge without HEAD?");
head = lookup_commit_or_die(&oid, "HEAD");
- if (read_ref_full("MERGE_HEAD",
- RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
- &oid, NULL))
- die("--merge without MERGE_HEAD?");
- if (is_null_oid(&oid))
- die(_("MERGE_HEAD exists but is a symbolic ref"));
- other = lookup_commit_or_die(&oid, "MERGE_HEAD");
+ other_name = lookup_other_head(&oid);
+ other = lookup_commit_or_die(&oid, other_name);
add_pending_object(revs, &head->object, "HEAD");
- add_pending_object(revs, &other->object, "MERGE_HEAD");
+ add_pending_object(revs, &other->object, other_name);
bases = repo_get_merge_bases(the_repository, head, other);
add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM);
add_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);
--
2.39.1
^ permalink raw reply related
* Re: [PATCH v6 0/2] Implement `git log --merge` also for rebase/cherry-pick/revert
From: phillip.wood123 @ 2024-02-28 14:40 UTC (permalink / raw)
To: Philippe Blain, git
Cc: Johannes Sixt, Elijah Newren, Michael Lohmann, Phillip Wood,
Patrick Steinhardt, Junio C Hamano, Michael Lohmann
In-Reply-To: <20240228-ml-log-merge-with-cherry-pick-and-other-pseudo-heads-v6-0-8ec34c052b39@gmail.com>
Hi Philippe
On 28/02/2024 13:54, Philippe Blain wrote:
> Range-diff versus v5:
>
> 1: c9536431d1 ! 1: 363657561c revision: ensure MERGE_HEAD is a ref in prepare_show_merge
> @@ Commit message
> (2) obtain the oid without any prefixing by refs.c:repo_dwim_ref()
> (3) error out when MERGE_HEAD is a symref.
>
> + Note that we avoid marking the new error message for translation as it
> + will be done in the next commit when the message is generalized to other
> + special refs.
Looking at the change below, the new message is in fact marked for
translation. I don't think this matters (other than the commit message
being confusing) as the translators will only see the final version of
the massage.
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> @@ revision.c: static void prepare_show_merge(struct rev_info *revs)
> + &oid, NULL))
> die("--merge without MERGE_HEAD?");
> + if (is_null_oid(&oid))
> -+ die(_("MERGE_HEAD is a symbolic ref?"));
> ++ die(_("MERGE_HEAD exists but is a symbolic ref"));
The new message is marked with _(...) so will be translated.
> other = lookup_commit_or_die(&oid, "MERGE_HEAD");
> add_pending_object(revs, &head->object, "HEAD");
> add_pending_object(revs, &other->object, "MERGE_HEAD");
> 2: 1641c4be81 ! 2: 749abadc04 revision: implement `git log --merge` also for rebase/cherry-pick/revert
> @@ revision.c: static void add_pending_commit_list(struct rev_info *revs,
> + RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> + oid, NULL)) {
> + if (is_null_oid(oid))
> -+ die(_("%s is a symbolic ref?"), other_head[i]);
> ++ die(_("%s exists but is a symbolic ref"), other_head[i]);
The second patch updates the message and this new version retains the _(...)
Best Wishes
Phillip
> + return other_head[i];
> + }
> +
> @@ revision.c: static void prepare_show_merge(struct rev_info *revs)
> - &oid, NULL))
> - die("--merge without MERGE_HEAD?");
> - if (is_null_oid(&oid))
> -- die(_("MERGE_HEAD is a symbolic ref?"));
> +- die(_("MERGE_HEAD exists but is a symbolic ref"));
> - other = lookup_commit_or_die(&oid, "MERGE_HEAD");
> + other_name = lookup_other_head(&oid);
> + other = lookup_commit_or_die(&oid, other_name);
>
^ permalink raw reply
* Re: [PATCH] rebase -i: stop setting GIT_CHERRY_PICK_HELP
From: phillip.wood123 @ 2024-02-28 14:45 UTC (permalink / raw)
To: Junio C Hamano, Phillip Wood via GitGitGadget
Cc: git, Johannes Schindelin, Phillip Wood
In-Reply-To: <xmqqil29sqfp.fsf@gitster.g>
On 27/02/2024 18:38, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> Note that we retain the changes in e4301f73fff (sequencer: unset
>> GIT_CHERRY_PICK_HELP for 'exec' commands, 2024-02-02) just in case
>> GIT_CHERRY_PICK_HELP is set in the environment when "git rebase" is
>> run.
>
> Is this comment about this part of the code?
No, it is about
strvec_push(&cmd.env, "GIT_CHERRY_PICK_HELP");
in do_exec() which clears GIT_CHERRY_PICK_HELP in the child environment
when running an exec command so that "exec git cherry-pick ..." retains
the correct author information.
>> + const char *msg;
>> +
>> + if (is_rebase_i(opts))
>> + msg = rebase_resolvemsg;
>> + else
>> + msg = getenv("GIT_CHERRY_PICK_HELP");
>
> Testing is_rebase_i() first means we ignore the environment
> unconditionally and use our own message always in "rebase -i", no?
Yes, this matches the existing behavior in builtin/rebase.c where we call
setenv("GIT_CHERRY_PICK_HELP", resolvemsg, 1);
to set GIT_CHERRY_PICK_HELP even if it is already set in the environment.
> Not that I think we should honor the environment variable and let it
> override our message. I just found the description a bit confusing.
I should have been clearer what that it was talking about - i.e. it is
still worth clearing GIT_CHERRY_PICK_HELP in the environment when
running exec commands.
Best Wishes
Phillip
>> diff --git a/sequencer.h b/sequencer.h
>> index dcef7bb99c0..437eabd38af 100644
>> --- a/sequencer.h
>> +++ b/sequencer.h
>> @@ -14,6 +14,8 @@ const char *rebase_path_todo(void);
>> const char *rebase_path_todo_backup(void);
>> const char *rebase_path_dropped(void);
>>
>> +extern const char *rebase_resolvemsg;
>
> This is more library-ish part of the system than a random file in
> the builtin/ directory. This place as the final location for the
> string makes sense to me.
>
> Thanks.
^ permalink raw reply
* [GSOC][PATCH] Userdiff: add builtin patterns for JavaScript.
From: Sergius Nyah @ 2024-02-28 15:00 UTC (permalink / raw)
To: git, christian.couder; +Cc: pk, shyamthakkar001, Sergius Nyah
In-Reply-To: <CANAnif-OganZLi0Cu_uq=nveC+u5n14c=o_DQHT-wFOqQ9Vs0Q@mail.gmail.com>
This commit introduces builtin patterns for JavaScript in userdiff and adds a new test
case in t4018-diff-funcname.sh to verify the enhanced JavaScript function detection in Git diffs.
Signed-off-by: Sergius Justus Chesami Nyah <sergiusnyah@gmail.com>
---
...-PATCH-1-2-Add-builtin-patterns-for-.patch | 53 ++++++-------------
t/t4018-diff-funcname.sh | 25 ++++++++-
2 files changed, 38 insertions(+), 40 deletions(-)
diff --git a/Pattern/0001-Subject-GSOC-RFC-PATCH-1-2-Add-builtin-patterns-for-.patch b/Pattern/0001-Subject-GSOC-RFC-PATCH-1-2-Add-builtin-patterns-for-.patch
index 43593866bc..9c3b80665e 100644
--- a/Pattern/0001-Subject-GSOC-RFC-PATCH-1-2-Add-builtin-patterns-for-.patch
+++ b/Pattern/0001-Subject-GSOC-RFC-PATCH-1-2-Add-builtin-patterns-for-.patch
-This patch adds the regular expression for detecting JavaScript functions and expressions in Git diffs. The pattern accurately identifies function declerations, expressions, and definitions inside blocks.
----
+This patch adds the regular expression for detecting JavaScript functions and expressions in Git diffs.
userdiff.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
@@ -18,40 +16,19 @@ index e399543823..12e31ff14d 100644
#include "git-compat-util.h"
#include "config.h"
#include "userdiff.h"
#include "strbuf.h"
+PATTERNS("javascript",
-+ /*
++ * Looks for lines that start with optional whitespace, followed
++ * 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]+"
-+ "|[-+*/<>%&^|=!:]=|--|\\+\\+|<<=?|>>=?|&&|\\|\\|"),
Here is the test file.
-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
--
2.43.2
^ permalink raw reply
* Re: [PATCH] branch: adjust documentation
From: Junio C Hamano @ 2024-02-28 17:20 UTC (permalink / raw)
To: Dragan Simic; +Cc: git, Rubén Justo
In-Reply-To: <d1f928b98238a60a96bee0d3f410deef@manjaro.org>
Dragan Simic <dsimic@manjaro.org> writes:
> Hello Junio,
>
> Just checking, do you see the changes that Ruben proposed in this patch
> fit for our starting point in the git-branch documentation rewrite?
The synopsis part may, but with reservations. I know you two are
aiming to make this in many small incremental steps, but even then
I'd have to say the way these placeholder words will be used in the
text part (both DESCRIPTION and OPTIONS) will be so different from
the final shape envisioned [*1*], the wording that may fit well within
the current text might not be the best fit in the final text.
The current description section talks about option and its arguments
without showing the syntax, making it unnecessary to be extra
verbose. For example, we see something like this:
With a `-m` or `-M` option, <oldbranch> will be renamed to
<newbranch>. If <oldbranch> had a corresponding reflog, it is
renamed to match ...
But in the final shape of the documentation, I would like to see the
description section not talk about these arguments, and would read
more like
When given `-m` or `-M` options, the command renames an existing
branch to a different name.
among short descriptions made at the conceptual level on other modes
like "listing" mode, "delete" mode, etc. [*3*]
And the option description would become something like [*]:
-m [<one>] <two>::
Renames the branch <one> (the current branch is used when
not given) to a new name <two>, together with its reflog and
configuration settings for the branch. ...
Side note: <one> and <two> are meta-placeholders for the purpose
of this document; Ruben's patch proposes to call them <branch>
and <new-branch>.
Now in such a context, <one> and <two> placeholders having actually
the word "branch" in it would sound redundant and awkward to read,
Even though the choice of words Ruben made in the patch under
discussion may work well in the current document structure. I
suspect these words will have to be chosen again when we start
making the real organizational changes to the description and
options sections.
In other words, I do not think that the patch makes an effective
"one good step in the right direction". At least, I couldn't see
how the wording for placeholders the patch proposes to use would be
helpful in deciding the placeholders used in the final version.
Thanks for pinging.
[Footnotes]
*1* Do we share the vision on how the final version should look
like? Here I am assuming "the final shape envisioned" is along
the lines of [*2*], i.e.
(1) trim descriptions to just enumerate different modes of
operations with explanation on what they do at the
conceptual level;
(2) make each entry in the options section self contained, by
showing the option with their <argument>s, referring to
them in the explanation text;
(3) remove non-option <argument> from the options list.
*2* https://lore.kernel.org/git/xmqqttmmlahf.fsf@gitster.g/
*3* Because "git branch" does so many things, the DESCRIPTION
section should first say the purpose of the section is to serve
as brief catalog of features to help readers to find the entry
in the option section to jump to quickly. Something like:
The "git branch" command works in many modes (see SYNOPSIS
above). By default the command works in the "list" option (the
`--list` option explicitly asks for this mode).
will be at the beginning of the section. The first four
paragraphs in the current DESCRIPTION section is about this
list mode. The first three of them should probably be moved to
the OPTIONS section under `--list`. The fourth paragraph
should be split and described in the entries of individual
options it talks about in the OPTIONS section.
.
^ permalink raw reply
* Re: [PATCH] branch: adjust documentation
From: Junio C Hamano @ 2024-02-28 17:24 UTC (permalink / raw)
To: Dragan Simic; +Cc: git, Rubén Justo
In-Reply-To: <xmqqttlsld4t.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> The current description section talks about option and its arguments
> without showing the syntax, making it unnecessary to be extra
> verbose. For example, we see something like this:
"unnecessarily extra verbose" is what I meant to say.
^ permalink raw reply
* Re: What's cooking in git.git (Feb 2024, #09; Tue, 27)
From: Junio C Hamano @ 2024-02-28 17:25 UTC (permalink / raw)
To: Philippe Blain; +Cc: git
In-Reply-To: <dbfc2788-1a02-2b5f-bee0-6fae747ac92e@gmail.com>
Philippe Blain <levraiphilippeblain@gmail.com> writes:
> Hi Junio,
>
> Le 2024-02-28 à 00:44, Junio C Hamano a écrit :
>> * ml/log-merge-with-cherry-pick-and-other-pseudo-heads (2024-02-27) 2 commits
> ...
> I'll send a new version with a reworked error message, so best to wait.
Thanks. Will do.
^ permalink raw reply
* Re: [PATCH] revision: fix --missing=[print|allow*] for annotated tags
From: Junio C Hamano @ 2024-02-28 17:46 UTC (permalink / raw)
To: Christian Couder
Cc: git, Patrick Steinhardt, John Cai, Linus Arver, Eric Sunshine,
Christian Couder
In-Reply-To: <20240228091011.3652532-1-christian.couder@gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> diff --git a/revision.c b/revision.c
> index 0c7266b1eb..8f0d638af1 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -419,15 +419,21 @@ static struct commit *handle_commit(struct rev_info *revs,
> */
> while (object->type == OBJ_TAG) {
> struct tag *tag = (struct tag *) object;
> + struct object_id *oid;
> if (revs->tag_objects && !(flags & UNINTERESTING))
> add_pending_object(revs, object, tag->tag);
> - object = parse_object(revs->repo, get_tagged_oid(tag));
> + oid = get_tagged_oid(tag);
> + object = parse_object(revs->repo, oid);
This is locally a no-op but we need it because we will use oid
later, OK.
> if (!object) {
> if (revs->ignore_missing_links || (flags & UNINTERESTING))
> return NULL;
> if (revs->exclude_promisor_objects &&
> is_promisor_object(&tag->tagged->oid))
> return NULL;
> + if (revs->do_not_die_on_missing_objects && oid) {
> + oidset_insert(&revs->missing_commits, oid);
> + return NULL;
> + }
And we recover from the "oh, that is not an object" by doing the
usual "add to missing-objects list". OK.
At this point we do not know the type of the tagged object (the tag
itself may hint what the tagged object is, though). We might want
to rename .missing_commits to .missing_objects later after the dust
settles. revision.c:get_reference() already adds anything that is
pointed at by a ref to this oidset already, so it is not a new
problem with this patch, though.
> diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
> index 78387eebb3..127180e1c9 100755
> --- a/t/t6022-rev-list-missing.sh
> +++ b/t/t6022-rev-list-missing.sh
> @@ -10,7 +10,10 @@ TEST_PASSES_SANITIZE_LEAK=true
> test_expect_success 'create repository and alternate directory' '
> test_commit 1 &&
> test_commit 2 &&
> - test_commit 3
> + test_commit 3 &&
> + git tag -m "tag message" annot_tag HEAD~1 &&
> + git tag regul_tag HEAD~1 &&
> + git branch a_branch HEAD~1
> '
>
> # We manually corrupt the repository, which means that the commit-graph may
> @@ -78,7 +81,7 @@ do
> done
> done
>
> -for missing_tip in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
> +for missing_tip in "annot_tag" "regul_tag" "a_branch" "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
> do
> # We want to check that things work when both
> # - all the tips passed are missing (case existing_tip = ""), and
> @@ -88,9 +91,6 @@ do
> for action in "allow-any" "print"
> do
> test_expect_success "--missing=$action with tip '$missing_tip' missing and tip '$existing_tip'" '
> - oid="$(git rev-parse $missing_tip)" &&
> - path=".git/objects/$(test_oid_to_path $oid)" &&
> -
> # Before the object is made missing, we use rev-list to
> # get the expected oids.
> if test "$existing_tip" = "HEAD"
> @@ -109,11 +109,23 @@ do
> echo $(git rev-parse HEAD:2.t) >>expect.raw
> fi &&
>
> + missing_oid="$(git rev-parse $missing_tip)" &&
> +
> + if test "$missing_tip" = "annot_tag"
> + then
> + oid="$(git rev-parse $missing_tip^{commit})" &&
> + echo "$missing_oid" >>expect.raw
> + else
> + oid="$missing_oid"
> + fi &&
> +
> + path=".git/objects/$(test_oid_to_path $oid)" &&
> +
> mv "$path" "$path.hidden" &&
> test_when_finished "mv $path.hidden $path" &&
Hmph, this might be OK for now, but recently I saw Dscho used a nice
trick to prepare a packfile that excludes certain objects in a
separate directory and use that directory as GIT_OBJECT_DIRECTORY to
simulate a situation where some objects are missing without touching
this level of implementation details. We may want to clean things
up.
Perhaps somebody will write a shell helper function that creates
such an object directory that contains all objects in the current
repository, except for ones that are specified. And then we add it
to t/test-lib-functions.sh, so that it can be used to update various
tests (#leftoverbits).
> git rev-list --missing=$action --objects --no-object-names \
> - $oid $existing_tip >actual.raw &&
> + $missing_oid $existing_tip >actual.raw &&
>
> # When the action is to print, we should also add the missing
> # oid to the expect list.
^ permalink raw reply
* Re: [PATCH] send-email: implement SMTP bearer authentication
From: M Hickford @ 2024-02-28 17:53 UTC (permalink / raw)
To: julian; +Cc: git
In-Reply-To: <20240225103413.9845-1-julian@swagemakers.org>
Neat idea. I recall it was awkward to configure git-send-email to send with
Gmail. I had to configure a security-compromising 'app password' [1][2].
OAuth is a great improvement.
[1] https://support.google.com/mail/answer/185833
[2] https://security.google.com/settings/security/apppasswords
^ permalink raw reply
* Re: [GSOC][PATCH] Userdiff: add builtin patterns for JavaScript.
From: Junio C Hamano @ 2024-02-28 18:19 UTC (permalink / raw)
To: Sergius Nyah; +Cc: git, christian.couder, pk, shyamthakkar001
In-Reply-To: <20240228150034.313196-1-sergiusnyah@gmail.com>
Sergius Nyah <sergiusnyah@gmail.com> writes:
> This commit introduces builtin patterns for JavaScript in userdiff and adds a new test
> case in t4018-diff-funcname.sh to verify the enhanced JavaScript function detection in Git diffs.
> Signed-off-by: Sergius Justus Chesami Nyah <sergiusnyah@gmail.com>
> ---
> ...-PATCH-1-2-Add-builtin-patterns-for-.patch | 53 ++++++-------------
> t/t4018-diff-funcname.sh | 25 ++++++++-
> 2 files changed, 38 insertions(+), 40 deletions(-)
You apparently have done "git add" of a file that is a format-patch
output, which led to ...
> diff --git a/Pattern/0001-Subject-GSOC-RFC-PATCH-1-2-Add-builtin-patterns-for-.patch b/Pattern/0001-Subject-GSOC-RFC-PATCH-1-2-Add-builtin-patterns-for-.patch
> index 43593866bc..9c3b80665e 100644
> --- a/Pattern/0001-Subject-GSOC-RFC-PATCH-1-2-Add-builtin-patterns-for-.patch
> +++ b/Pattern/0001-Subject-GSOC-RFC-PATCH-1-2-Add-builtin-patterns-for-.patch
... this thing.
Wrap the proposed commit log message to a reasonable length and have
a blank line before your sign-off. Other pieces of advice about
formatting and contents found in Documentation/SubmittingPatches are
also helpful.
Thanks.
^ permalink raw reply
* Re: [PATCH v4 04/11] commit-reach(paint_down_to_common): prepare for handling shallow commits
From: Junio C Hamano @ 2024-02-28 20:24 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Patrick Steinhardt, Dirk Gouders, Johannes Schindelin
In-Reply-To: <837aa5a89c640427a5de064da93f1de4934d8212.1709113458.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> When `git fetch --update-shallow` needs to test for commit ancestry, it
> can naturally run into a missing object (e.g. if it is a parent of a
> shallow commit). For the purpose of `--update-shallow`, this needs to be
> treated as if the child commit did not even have that parent, i.e. the
> commit history needs to be clamped.
>
> For all other scenarios, clamping the commit history is actually a bug,
> as it would hide repository corruption (for an analysis regarding
> shallow and partial clones, see the analysis further down).
>
> Add a flag to optionally ask the function to ignore missing commits, as
> `--update-shallow` needs it to, while detecting missing objects as a
> repository corruption error by default.
>
> This flag is needed, and cannot replaced by `is_repository_shallow()` to
> indicate that situation, because that function would return 0 in the
> `--update-shallow` scenario: There is not actually a `shallow` file in
> that scenario, as demonstrated e.g. by t5537.10 ("add new shallow root
> with receive.updateshallow on") and t5538.4 ("add new shallow root with
> receive.updateshallow on").
Nicely written.
The description above that has been totally revamped reads much much
clearer, at least to me, compared to the previous round.
Should we declare the topic done and mark it for 'next'?
Thanks.
^ permalink raw reply
* Re: [PATCH v4 04/11] commit-reach(paint_down_to_common): prepare for handling shallow commits
From: Dirk Gouders @ 2024-02-28 20:59 UTC (permalink / raw)
To: Junio C Hamano, Johannes Schindelin
Cc: Johannes Schindelin via GitGitGadget, git, Patrick Steinhardt
In-Reply-To: <xmqqedcwjq04.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> When `git fetch --update-shallow` needs to test for commit ancestry, it
>> can naturally run into a missing object (e.g. if it is a parent of a
>> shallow commit). For the purpose of `--update-shallow`, this needs to be
>> treated as if the child commit did not even have that parent, i.e. the
>> commit history needs to be clamped.
>>
>> For all other scenarios, clamping the commit history is actually a bug,
>> as it would hide repository corruption (for an analysis regarding
>> shallow and partial clones, see the analysis further down).
>>
>> Add a flag to optionally ask the function to ignore missing commits, as
>> `--update-shallow` needs it to, while detecting missing objects as a
>> repository corruption error by default.
>>
>> This flag is needed, and cannot replaced by `is_repository_shallow()` to
>> indicate that situation, because that function would return 0 in the
>> `--update-shallow` scenario: There is not actually a `shallow` file in
>> that scenario, as demonstrated e.g. by t5537.10 ("add new shallow root
>> with receive.updateshallow on") and t5538.4 ("add new shallow root with
>> receive.updateshallow on").
>
> Nicely written.
>
> The description above that has been totally revamped reads much much
> clearer, at least to me, compared to the previous round.
>
> Should we declare the topic done and mark it for 'next'?
>
> Thanks.
I agree that this text reads much clearer -- even to me with close to
zero experience, here.
Thank you for taking the time to rewrite the text, Johannes.
Dirk
^ permalink raw reply
* Draft of Git Rev News edition 108
From: Christian Couder @ 2024-02-28 21:24 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jakub Narebski, Markus Jansen, Kaartic Sivaraam,
Štěpán Němec, Taylor Blau,
Johannes Schindelin, Ævar Arnfjörð Bjarmason,
Bruno Brito, René Scharfe, Jeff King, Dragan Simic,
Brooke Kuhlmann
Hi everyone,
A draft of a new Git Rev News edition is available here:
https://github.com/git/git.github.io/blob/master/rev_news/drafts/edition-108.md
Everyone is welcome to contribute in any section either by editing the
above page on GitHub and sending a pull request, or by commenting on
this GitHub issue:
https://github.com/git/git.github.io/issues/686
You can also reply to this email.
In general all kinds of contributions, for example proofreading,
suggestions for articles or links, help on the issues in GitHub,
volunteering for being interviewed and so on, are very much
appreciated.
I tried to Cc everyone who appears in this edition, but maybe I missed
some people, sorry about that.
Jakub, Markus, Kaartic and I plan to publish this edition on
Friday March 1st, 2024.
Thanks,
Christian.
^ permalink raw reply
* [PATCH 0/9] bound upload-pack memory allocations
From: Jeff King @ 2024-02-28 22:37 UTC (permalink / raw)
To: git; +Cc: Benjamin Flesch
Benjamin Flesch reported to the security list that a client can convince
upload-pack to allocate arbitrary amounts of memory by sending a few
repeated nonsense directives. Patches 1-6 here eliminate those spots.
which roughly bounds the amount of memory that upload-pack will allocate
to scale with the number of refs and objects in the repository. I also
found a few spots where upload-pack is a little too eager to hold object
contents in memory (so still bounded, but something like "all the trees"
can get pretty big). That's patches 7-9.
We're not doing a coordinated disclosure or special release for this.
Even after these patches, it's possible to get upload-pack to allocate
quite a bit of memory, especially for a large repository. Not to mention
that pack-objects may also allocate quite a bit to serve the pack
itself. So while this is low-hanging fruit, a public-facing Git site
probably still needs to have some kind of external tooling to kill
hungry processes (even if it's just OOM-killing them so they don't hurt
other clients).
There are a few unbounded parts of receive-pack (e.g., you can send an
infinite number of refs to push). That may be something we want to put
some configurable boundaries on, but I punted on it for this series.
IMHO it is a lot less interesting since you'd usually have to
authenticate to invoke receive-pack in the first place.
[1/9]: upload-pack: drop separate v2 "haves" array
[2/9]: upload-pack: switch deepen-not list to an oid_array
[3/9]: upload-pack: use oidset for deepen_not list
[4/9]: upload-pack: use a strmap for want-ref lines
[5/9]: upload-pack: accept only a single packfile-uri line
[6/9]: upload-pack: disallow object-info capability by default
[7/9]: upload-pack: always turn off save_commit_buffer
[8/9]: upload-pack: use PARSE_OBJECT_SKIP_HASH_CHECK in more places
[9/9]: upload-pack: free tree buffers after parsing
Documentation/config/transfer.txt | 4 +
Documentation/gitprotocol-v2.txt | 6 +-
builtin/upload-pack.c | 2 +
object.c | 14 ++++
object.h | 1 +
revision.c | 3 +-
serve.c | 14 +++-
t/t5555-http-smart-common.sh | 1 -
t/t5701-git-serve.sh | 24 +++++-
upload-pack.c | 117 +++++++++++++-----------------
10 files changed, 113 insertions(+), 73 deletions(-)
-Peff
^ permalink raw reply
* [PATCH 1/9] upload-pack: drop separate v2 "haves" array
From: Jeff King @ 2024-02-28 22:37 UTC (permalink / raw)
To: git; +Cc: Benjamin Flesch
In-Reply-To: <20240228223700.GA1157826@coredump.intra.peff.net>
When upload-pack sees a "have" line in the v0 protocol, it immediately
calls got_oid() with its argument and potentially produces an ACK
response. In the v2 protocol, we simply record the argument in an
oid_array, and only later process all of the "have" objects by calling
the equivalent of got_oid() on the contents of the array.
This makes some sense, as v2 is a pure request/response protocol, as
opposed to v0's asynchronous negotiation phase. But there's a downside:
a client can send us an infinite number of garbage "have" lines, which
we'll happily slurp into the array, consuming memory. Whereas in v0,
they are limited by the number of objects in the repository (because
got_oid() only records objects we have ourselves, and we avoid
duplicates by setting a flag on the object struct).
We can make v2 behave more like v0 by also calling got_oid() directly
when v2 parses a "have" line. Calling it early like this is OK because
got_oid() itself does not interact with the client; it only confirms
that we have the object and sets a few flags. Note that unlike v0, v2
does not ever (before or after this patch) check the return code of
got_oid(), which lets the caller know whether we have the object. But
again, that makes sense; v0 is using it to asynchronously tell the
client to stop sending. In v2's synchronous protocol, we just discard
those entries (and decide how to ACK at the end of each round).
There is one slight tweak we need, though. In v2's state machine, we
reach the SEND_ACKS state if the other side sent us any "have" lines,
whether they were useful or not. Right now we do that by checking
whether the "have" array had any entries, but if we record only the
useful ones, that doesn't work. Instead, we can add a simple boolean
that tells us whether we saw any have line (even if it was useless).
This lets us drop the "haves" array entirely, as we're now placing
objects directly into the "have_obj" object array (which is where
got_oid() put them in the long run anyway). And as a bonus, we can drop
the secondary "common" array used in process_haves_and_send_acks(). It
was essentially a copy of "haves" minus the objects we do not have. But
now that we are using "have_obj" directly, we know everything in it is
useful. So in addition to protecting ourselves against malicious input,
we should slightly lower our memory usage for normal inputs.
Note that there is one user-visible effect. The trace2 output records
the number of "haves". Previously this was the total number of "have"
lines we saw, but now is the number of useful ones. We could retain the
original meaning by keeping a separate counter, but it doesn't seem
worth the effort; this trace info is for debugging and metrics, and
arguably the count of common oids is at least as useful as the total
count.
Reported-by: Benjamin Flesch <benjaminflesch@icloud.com>
Signed-off-by: Jeff King <peff@peff.net>
---
upload-pack.c | 48 ++++++++++--------------------------------------
1 file changed, 10 insertions(+), 38 deletions(-)
diff --git a/upload-pack.c b/upload-pack.c
index 2537affa90..7a83127121 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -61,7 +61,6 @@ struct upload_pack_data {
struct string_list symref; /* v0 only */
struct object_array want_obj;
struct object_array have_obj;
- struct oid_array haves; /* v2 only */
struct string_list wanted_refs; /* v2 only */
struct strvec hidden_refs;
@@ -113,6 +112,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 seen_haves : 1; /* v2 only */
unsigned advertise_sid : 1;
unsigned sent_capabilities : 1;
};
@@ -124,7 +124,6 @@ static void upload_pack_data_init(struct upload_pack_data *data)
struct strvec hidden_refs = STRVEC_INIT;
struct object_array want_obj = OBJECT_ARRAY_INIT;
struct object_array have_obj = OBJECT_ARRAY_INIT;
- struct oid_array haves = OID_ARRAY_INIT;
struct object_array shallows = OBJECT_ARRAY_INIT;
struct string_list deepen_not = STRING_LIST_INIT_DUP;
struct string_list uri_protocols = STRING_LIST_INIT_DUP;
@@ -137,7 +136,6 @@ static void upload_pack_data_init(struct upload_pack_data *data)
data->hidden_refs = hidden_refs;
data->want_obj = want_obj;
data->have_obj = have_obj;
- data->haves = haves;
data->shallows = shallows;
data->deepen_not = deepen_not;
data->uri_protocols = uri_protocols;
@@ -159,7 +157,6 @@ static void upload_pack_data_clear(struct upload_pack_data *data)
strvec_clear(&data->hidden_refs);
object_array_clear(&data->want_obj);
object_array_clear(&data->have_obj);
- oid_array_clear(&data->haves);
object_array_clear(&data->shallows);
string_list_clear(&data->deepen_not, 0);
object_array_clear(&data->extra_edge_obj);
@@ -1532,15 +1529,14 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,
return 0;
}
-static int parse_have(const char *line, struct oid_array *haves)
+static int parse_have(const char *line, struct upload_pack_data *data)
{
const char *arg;
if (skip_prefix(line, "have ", &arg)) {
struct object_id oid;
- if (get_oid_hex(arg, &oid))
- die("git upload-pack: expected SHA1 object, got '%s'", arg);
- oid_array_append(haves, &oid);
+ got_oid(data, arg, &oid);
+ data->seen_haves = 1;
return 1;
}
@@ -1552,7 +1548,7 @@ static void trace2_fetch_info(struct upload_pack_data *data)
struct json_writer jw = JSON_WRITER_INIT;
jw_object_begin(&jw, 0);
- jw_object_intmax(&jw, "haves", data->haves.nr);
+ jw_object_intmax(&jw, "haves", data->have_obj.nr);
jw_object_intmax(&jw, "wants", data->want_obj.nr);
jw_object_intmax(&jw, "want-refs", data->wanted_refs.nr);
jw_object_intmax(&jw, "depth", data->depth);
@@ -1586,7 +1582,7 @@ static void process_args(struct packet_reader *request,
&data->hidden_refs, &data->want_obj))
continue;
/* process have line */
- if (parse_have(arg, &data->haves))
+ if (parse_have(arg, data))
continue;
/* process args like thin-pack */
@@ -1664,27 +1660,7 @@ static void process_args(struct packet_reader *request,
trace2_fetch_info(data);
}
-static int process_haves(struct upload_pack_data *data, struct oid_array *common)
-{
- int i;
-
- /* Process haves */
- for (i = 0; i < data->haves.nr; i++) {
- const struct object_id *oid = &data->haves.oid[i];
-
- if (!repo_has_object_file_with_flags(the_repository, oid,
- OBJECT_INFO_QUICK | OBJECT_INFO_SKIP_FETCH_OBJECT))
- continue;
-
- oid_array_append(common, oid);
-
- do_got_oid(data, oid);
- }
-
- return 0;
-}
-
-static int send_acks(struct upload_pack_data *data, struct oid_array *acks)
+static int send_acks(struct upload_pack_data *data, struct object_array *acks)
{
int i;
@@ -1696,7 +1672,7 @@ static int send_acks(struct upload_pack_data *data, struct oid_array *acks)
for (i = 0; i < acks->nr; i++) {
packet_writer_write(&data->writer, "ACK %s\n",
- oid_to_hex(&acks->oid[i]));
+ oid_to_hex(&acks->objects[i].item->oid));
}
if (!data->wait_for_done && ok_to_give_up(data)) {
@@ -1710,13 +1686,11 @@ static int send_acks(struct upload_pack_data *data, struct oid_array *acks)
static int process_haves_and_send_acks(struct upload_pack_data *data)
{
- struct oid_array common = OID_ARRAY_INIT;
int ret = 0;
- process_haves(data, &common);
if (data->done) {
ret = 1;
- } else if (send_acks(data, &common)) {
+ } else if (send_acks(data, &data->have_obj)) {
packet_writer_delim(&data->writer);
ret = 1;
} else {
@@ -1725,8 +1699,6 @@ static int process_haves_and_send_acks(struct upload_pack_data *data)
ret = 0;
}
- oid_array_clear(&data->haves);
- oid_array_clear(&common);
return ret;
}
@@ -1796,7 +1768,7 @@ int upload_pack_v2(struct repository *r UNUSED, struct packet_reader *request)
* they didn't want anything.
*/
state = FETCH_DONE;
- } else if (data.haves.nr) {
+ } else if (data.seen_haves) {
/*
* Request had 'have' lines, so lets ACK them.
*/
--
2.44.0.rc2.424.gbdbf4d014b
^ permalink raw reply related
* [PATCH 2/9] upload-pack: switch deepen-not list to an oid_array
From: Jeff King @ 2024-02-28 22:37 UTC (permalink / raw)
To: git; +Cc: Benjamin Flesch
In-Reply-To: <20240228223700.GA1157826@coredump.intra.peff.net>
When we see a "deepen-not" line from the client, we verify that the
given name can be resolved as a ref, and then add it to a string list to
be passed later to an internal "rev-list --not" traversal. We record the
actual refname in the string list (so the traversal resolves it again
later), but we'd be better off recording the resolved oid:
1. There's a tiny bit of wasted work in resolving it twice.
2. There's a small race condition with simultaneous updates; the later
traversal may resolve to a different value (or not at all). This
shouldn't cause any bad behavior (we do not care about the value
in this first resolution, so whatever value rev-list gets is OK)
but it could mean a confusing error message (if upload-pack fails
to resolve the ref it produces a useful message, but a failing
traversal later results in just "revision walk setup failed").
3. It makes it simpler to de-duplicate the results. We don't de-dup at
all right now, but we will in the next patch.
From the client's perspective the behavior should be the same.
Signed-off-by: Jeff King <peff@peff.net>
---
upload-pack.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/upload-pack.c b/upload-pack.c
index 7a83127121..f6395b210e 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -65,7 +65,7 @@ struct upload_pack_data {
struct strvec hidden_refs;
struct object_array shallows;
- struct string_list deepen_not;
+ struct oid_array deepen_not;
struct object_array extra_edge_obj;
int depth;
timestamp_t deepen_since;
@@ -125,7 +125,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)
struct object_array want_obj = OBJECT_ARRAY_INIT;
struct object_array have_obj = OBJECT_ARRAY_INIT;
struct object_array shallows = OBJECT_ARRAY_INIT;
- struct string_list deepen_not = STRING_LIST_INIT_DUP;
+ struct oid_array deepen_not = OID_ARRAY_INIT;
struct string_list uri_protocols = STRING_LIST_INIT_DUP;
struct object_array extra_edge_obj = OBJECT_ARRAY_INIT;
struct string_list allowed_filters = STRING_LIST_INIT_DUP;
@@ -158,7 +158,7 @@ static void upload_pack_data_clear(struct upload_pack_data *data)
object_array_clear(&data->want_obj);
object_array_clear(&data->have_obj);
object_array_clear(&data->shallows);
- string_list_clear(&data->deepen_not, 0);
+ oid_array_clear(&data->deepen_not);
object_array_clear(&data->extra_edge_obj);
list_objects_filter_release(&data->filter_options);
string_list_clear(&data->allowed_filters, 0);
@@ -926,8 +926,8 @@ static int send_shallow_list(struct upload_pack_data *data)
if (data->deepen_not.nr) {
strvec_push(&av, "--not");
for (i = 0; i < data->deepen_not.nr; i++) {
- struct string_list_item *s = data->deepen_not.items + i;
- strvec_push(&av, s->string);
+ struct object_id *oid = data->deepen_not.oid + i;
+ strvec_push(&av, oid_to_hex(oid));
}
strvec_push(&av, "--not");
}
@@ -1004,15 +1004,15 @@ static int process_deepen_since(const char *line, timestamp_t *deepen_since, int
return 0;
}
-static int process_deepen_not(const char *line, struct string_list *deepen_not, int *deepen_rev_list)
+static int process_deepen_not(const char *line, struct oid_array *deepen_not, int *deepen_rev_list)
{
const char *arg;
if (skip_prefix(line, "deepen-not ", &arg)) {
char *ref = NULL;
struct object_id oid;
if (expand_ref(the_repository, arg, strlen(arg), &oid, &ref) != 1)
die("git upload-pack: ambiguous deepen-not: %s", line);
- string_list_append(deepen_not, ref);
+ oid_array_append(deepen_not, &oid);
free(ref);
*deepen_rev_list = 1;
return 1;
--
2.44.0.rc2.424.gbdbf4d014b
^ permalink raw reply related
* [PATCH 3/9] upload-pack: use oidset for deepen_not list
From: Jeff King @ 2024-02-28 22:37 UTC (permalink / raw)
To: git; +Cc: Benjamin Flesch
In-Reply-To: <20240228223700.GA1157826@coredump.intra.peff.net>
We record the oid of every deepen-not line the client sends to us. For a
well-behaved client, the resulting array should be bounded by the number
of unique refs we have. But because there's no de-duplication, a
malicious client can cause the array to grow unbounded by just sending
the same "refs/heads/foo" over and over (assuming such a ref exists).
Since the deepen-not list is just being fed to a "rev-list --not"
traversal, the order of items doesn't matter. So we can replace the
oid_array with an oidset which notices and skips duplicates.
That bounds the memory in malicious cases to be linear in the number of
unique refs. And even in non-malicious cases, there may be a slight
improvement in memory usage if multiple refs point to the same oid
(though in practice this list is probably pretty tiny anyway, as it
comes from the user specifying "--shallow-exclude" on the client fetch).
Note that in the trace2 output we'll now output the number of
de-duplicated objects, rather than the total number of "deepen-not"
lines we received. This is arguably a more useful value for tracing /
debugging anyway.
Reported-by: Benjamin Flesch <benjaminflesch@icloud.com>
Signed-off-by: Jeff King <peff@peff.net>
---
upload-pack.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/upload-pack.c b/upload-pack.c
index f6395b210e..ebad9026d7 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -65,7 +65,7 @@ struct upload_pack_data {
struct strvec hidden_refs;
struct object_array shallows;
- struct oid_array deepen_not;
+ struct oidset deepen_not;
struct object_array extra_edge_obj;
int depth;
timestamp_t deepen_since;
@@ -125,7 +125,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)
struct object_array want_obj = OBJECT_ARRAY_INIT;
struct object_array have_obj = OBJECT_ARRAY_INIT;
struct object_array shallows = OBJECT_ARRAY_INIT;
- struct oid_array deepen_not = OID_ARRAY_INIT;
+ struct oidset deepen_not = OID_ARRAY_INIT;
struct string_list uri_protocols = STRING_LIST_INIT_DUP;
struct object_array extra_edge_obj = OBJECT_ARRAY_INIT;
struct string_list allowed_filters = STRING_LIST_INIT_DUP;
@@ -158,7 +158,7 @@ static void upload_pack_data_clear(struct upload_pack_data *data)
object_array_clear(&data->want_obj);
object_array_clear(&data->have_obj);
object_array_clear(&data->shallows);
- oid_array_clear(&data->deepen_not);
+ oidset_clear(&data->deepen_not);
object_array_clear(&data->extra_edge_obj);
list_objects_filter_release(&data->filter_options);
string_list_clear(&data->allowed_filters, 0);
@@ -923,12 +923,13 @@ static int send_shallow_list(struct upload_pack_data *data)
strvec_push(&av, "rev-list");
if (data->deepen_since)
strvec_pushf(&av, "--max-age=%"PRItime, data->deepen_since);
- if (data->deepen_not.nr) {
+ if (oidset_size(&data->deepen_not)) {
+ const struct object_id *oid;
+ struct oidset_iter iter;
strvec_push(&av, "--not");
- for (i = 0; i < data->deepen_not.nr; i++) {
- struct object_id *oid = data->deepen_not.oid + i;
+ oidset_iter_init(&data->deepen_not, &iter);
+ while ((oid = oidset_iter_next(&iter)))
strvec_push(&av, oid_to_hex(oid));
- }
strvec_push(&av, "--not");
}
for (i = 0; i < data->want_obj.nr; i++) {
@@ -1004,15 +1005,15 @@ static int process_deepen_since(const char *line, timestamp_t *deepen_since, int
return 0;
}
-static int process_deepen_not(const char *line, struct oid_array *deepen_not, int *deepen_rev_list)
+static int process_deepen_not(const char *line, struct oidset *deepen_not, int *deepen_rev_list)
{
const char *arg;
if (skip_prefix(line, "deepen-not ", &arg)) {
char *ref = NULL;
struct object_id oid;
if (expand_ref(the_repository, arg, strlen(arg), &oid, &ref) != 1)
die("git upload-pack: ambiguous deepen-not: %s", line);
- oid_array_append(deepen_not, &oid);
+ oidset_insert(deepen_not, &oid);
free(ref);
*deepen_rev_list = 1;
return 1;
@@ -1554,7 +1555,7 @@ static void trace2_fetch_info(struct upload_pack_data *data)
jw_object_intmax(&jw, "depth", data->depth);
jw_object_intmax(&jw, "shallows", data->shallows.nr);
jw_object_bool(&jw, "deepen-since", data->deepen_since);
- jw_object_intmax(&jw, "deepen-not", data->deepen_not.nr);
+ jw_object_intmax(&jw, "deepen-not", oidset_size(&data->deepen_not));
jw_object_bool(&jw, "deepen-relative", data->deepen_relative);
if (data->filter_options.choice)
jw_object_string(&jw, "filter", list_object_filter_config_name(data->filter_options.choice));
--
2.44.0.rc2.424.gbdbf4d014b
^ permalink raw reply related
* [PATCH 4/9] upload-pack: use a strmap for want-ref lines
From: Jeff King @ 2024-02-28 22:38 UTC (permalink / raw)
To: git; +Cc: Benjamin Flesch
In-Reply-To: <20240228223700.GA1157826@coredump.intra.peff.net>
When the "ref-in-want" capability is advertised (which it is not by
default), then upload-pack processes a "want-ref" line from the client
by checking that the name is a valid ref and recording it in a
string-list.
In theory this list should grow no larger than the number of refs in the
server-side repository. But since we don't do any de-duplication, a
client which sends "want-ref refs/heads/foo" over and over will cause
the array to grow without bound.
We can fix this by switching to strmap, which efficiently detects
duplicates. There are two client-visible changes here:
1. The "wanted-refs" response will now be in an apparently-random
order (based on iterating the hashmap) rather than the order given
by the client. The protocol documentation is quiet on ordering
here. The current fetch-pack implementation is happy with any
order, as it looks up each returned ref using a binary search in
its local sorted list. JGit seems to implement want-ref on the
server side, but has no client-side support. libgit2 doesn't
support either side.
It would obviously be possible to record the original order or to
use the strmap as an auxiliary data structure. But if the client
doesn't care, we may as well do the simplest thing.
2. We'll now reject duplicates explicitly as a protocol error. The
client should never send them (and our current implementation, even
when asked to "git fetch master:one master:two" will de-dup on the
client side).
If we wanted to be more forgiving, we could perhaps just throw away
the duplicates. But then our "wanted-refs" response back to the
client would omit the duplicates, and it's hard to say what a
client that accidentally sent a duplicate would do with that. So I
think we're better off to complain loudly before anybody
accidentally writes such a client.
Let's also add a note to the protocol documentation clarifying that
duplicates are forbidden. As discussed above, this was already the
intent, but it's not very explicit.
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/gitprotocol-v2.txt | 3 ++-
upload-pack.c | 30 +++++++++++++++++-------------
2 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/Documentation/gitprotocol-v2.txt b/Documentation/gitprotocol-v2.txt
index 0b800abd56..837ea6171e 100644
--- a/Documentation/gitprotocol-v2.txt
+++ b/Documentation/gitprotocol-v2.txt
@@ -346,7 +346,8 @@ the 'wanted-refs' section in the server's response as explained below.
want-ref <ref>
Indicates to the server that the client wants to retrieve a
particular ref, where <ref> is the full name of a ref on the
- server.
+ server. It is a protocol error to send want-ref for the
+ same ref more than once.
If the 'sideband-all' feature is advertised, the following argument can be
included in the client's request:
diff --git a/upload-pack.c b/upload-pack.c
index ebad9026d7..8b47576ec7 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -28,6 +28,7 @@
#include "shallow.h"
#include "write-or-die.h"
#include "json-writer.h"
+#include "strmap.h"
/* Remember to update object flag allocation in object.h */
#define THEY_HAVE (1u << 11)
@@ -61,7 +62,7 @@ struct upload_pack_data {
struct string_list symref; /* v0 only */
struct object_array want_obj;
struct object_array have_obj;
- struct string_list wanted_refs; /* v2 only */
+ struct strmap wanted_refs; /* v2 only */
struct strvec hidden_refs;
struct object_array shallows;
@@ -120,7 +121,7 @@ struct upload_pack_data {
static void upload_pack_data_init(struct upload_pack_data *data)
{
struct string_list symref = STRING_LIST_INIT_DUP;
- struct string_list wanted_refs = STRING_LIST_INIT_DUP;
+ struct strmap wanted_refs = STRMAP_INIT;
struct strvec hidden_refs = STRVEC_INIT;
struct object_array want_obj = OBJECT_ARRAY_INIT;
struct object_array have_obj = OBJECT_ARRAY_INIT;
@@ -153,7 +154,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)
static void upload_pack_data_clear(struct upload_pack_data *data)
{
string_list_clear(&data->symref, 1);
- string_list_clear(&data->wanted_refs, 1);
+ strmap_clear(&data->wanted_refs, 1);
strvec_clear(&data->hidden_refs);
object_array_clear(&data->want_obj);
object_array_clear(&data->have_obj);
@@ -1488,14 +1489,13 @@ static int parse_want(struct packet_writer *writer, const char *line,
}
static int parse_want_ref(struct packet_writer *writer, const char *line,
- struct string_list *wanted_refs,
+ struct strmap *wanted_refs,
struct strvec *hidden_refs,
struct object_array *want_obj)
{
const char *refname_nons;
if (skip_prefix(line, "want-ref ", &refname_nons)) {
struct object_id oid;
- struct string_list_item *item;
struct object *o = NULL;
struct strbuf refname = STRBUF_INIT;
@@ -1507,8 +1507,11 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,
}
strbuf_release(&refname);
- item = string_list_append(wanted_refs, refname_nons);
- item->util = oiddup(&oid);
+ if (strmap_put(wanted_refs, refname_nons, oiddup(&oid))) {
+ packet_writer_error(writer, "duplicate want-ref %s",
+ refname_nons);
+ die("duplicate want-ref %s", refname_nons);
+ }
if (!starts_with(refname_nons, "refs/tags/")) {
struct commit *commit = lookup_commit_in_graph(the_repository, &oid);
@@ -1551,7 +1554,7 @@ static void trace2_fetch_info(struct upload_pack_data *data)
jw_object_begin(&jw, 0);
jw_object_intmax(&jw, "haves", data->have_obj.nr);
jw_object_intmax(&jw, "wants", data->want_obj.nr);
- jw_object_intmax(&jw, "want-refs", data->wanted_refs.nr);
+ jw_object_intmax(&jw, "want-refs", strmap_get_size(&data->wanted_refs));
jw_object_intmax(&jw, "depth", data->depth);
jw_object_intmax(&jw, "shallows", data->shallows.nr);
jw_object_bool(&jw, "deepen-since", data->deepen_since);
@@ -1705,17 +1708,18 @@ static int process_haves_and_send_acks(struct upload_pack_data *data)
static void send_wanted_ref_info(struct upload_pack_data *data)
{
- const struct string_list_item *item;
+ struct hashmap_iter iter;
+ const struct strmap_entry *e;
- if (!data->wanted_refs.nr)
+ if (strmap_empty(&data->wanted_refs))
return;
packet_writer_write(&data->writer, "wanted-refs\n");
- for_each_string_list_item(item, &data->wanted_refs) {
+ strmap_for_each_entry(&data->wanted_refs, &iter, e) {
packet_writer_write(&data->writer, "%s %s\n",
- oid_to_hex(item->util),
- item->string);
+ oid_to_hex(e->value),
+ e->key);
}
packet_writer_delim(&data->writer);
--
2.44.0.rc2.424.gbdbf4d014b
^ permalink raw reply related
* [PATCH 5/9] upload-pack: accept only a single packfile-uri line
From: Jeff King @ 2024-02-28 22:38 UTC (permalink / raw)
To: git; +Cc: Benjamin Flesch
In-Reply-To: <20240228223700.GA1157826@coredump.intra.peff.net>
When we see a packfile-uri line from the client, we use
string_list_split() to split it on commas and store the result in a
string_list. A single packfile-uri line is therefore limited to storing
~64kb, the size of a pkt-line.
But we'll happily accept multiple such lines, and each line appends to
the string list, growing without bound.
In theory this could be useful, making:
0017packfile-uris http
0018packfile-uris https
equivalent to:
001dpackfile-uris http,https
But the protocol documentation doesn't indicate that this should work
(and indeed, refers to this in the singular as "the following argument
can be included in the client's request"). And the client-side
implementation in fetch-pack has always sent a single line (JGit appears
to understand the line on the server side but has no client-side
implementation, and libgit2 understands neither).
If we were worried about compatibility, we could instead just put a
limit on the maximum number of values we'd accept. The current client
implementation limits itself to only two values: "http" and "https", so
something like "256" would be more than enough. But accepting only a
single line seems more in line with the protocol documentation, and
matches other parts of the protocol (e.g., we will not accept a second
"filter" line).
We'll also make this more explicit in the protocol documentation; as
above, I think this was always the intent, but there's no harm in making
it clear.
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/gitprotocol-v2.txt | 3 ++-
upload-pack.c | 3 +++
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/Documentation/gitprotocol-v2.txt b/Documentation/gitprotocol-v2.txt
index 837ea6171e..414bc625d5 100644
--- a/Documentation/gitprotocol-v2.txt
+++ b/Documentation/gitprotocol-v2.txt
@@ -362,7 +362,8 @@ included in the client's request:
If the 'packfile-uris' feature is advertised, the following argument
can be included in the client's request as well as the potential
addition of the 'packfile-uris' section in the server's response as
-explained below.
+explained below. Note that at most one `packfile-uris` line can be sent
+to the server.
packfile-uris <comma-separated-list-of-protocols>
Indicates to the server that the client is willing to receive
diff --git a/upload-pack.c b/upload-pack.c
index 8b47576ec7..2a5c52666e 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1646,6 +1646,9 @@ static void process_args(struct packet_reader *request,
}
if (skip_prefix(arg, "packfile-uris ", &p)) {
+ if (data->uri_protocols.nr)
+ send_err_and_die(data,
+ "multiple packfile-uris lines forbidden");
string_list_split(&data->uri_protocols, p, ',', -1);
continue;
}
--
2.44.0.rc2.424.gbdbf4d014b
^ permalink raw reply related
* [PATCH 6/9] upload-pack: disallow object-info capability by default
From: Jeff King @ 2024-02-28 22:38 UTC (permalink / raw)
To: git; +Cc: Benjamin Flesch
In-Reply-To: <20240228223700.GA1157826@coredump.intra.peff.net>
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
+'
+
+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
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox