* [PATCH 1/1] branch.c: ammend error messages for die()
From: Isoken June Ibizugbe @ 2023-10-11 15:24 UTC (permalink / raw)
To: git
In-Reply-To: <20231011152424.6957-1-isokenjune@gmail.com>
Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
---
builtin/branch.c | 38 +++++++++++++++++++-------------------
1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 2ec190b14a..a756543d64 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -518,11 +518,11 @@ static void reject_rebase_or_bisect_branch(struct worktree **worktrees,
continue;
if (is_worktree_being_rebased(wt, target))
- die(_("Branch %s is being rebased at %s"),
+ die(_("branch %s is being rebased at %s"),
target, wt->path);
if (is_worktree_being_bisected(wt, target))
- die(_("Branch %s is being bisected at %s"),
+ die(_("branch %s is being bisected at %s"),
target, wt->path);
}
}
@@ -578,7 +578,7 @@ 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);
+ die(_("invalid branch name: '%s'"), oldname);
}
for (int i = 0; worktrees[i]; i++) {
@@ -594,9 +594,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if ((copy || !(oldref_usage & IS_HEAD)) && !ref_exists(oldref.buf)) {
if (oldref_usage & IS_HEAD)
- die(_("No commit on branch '%s' yet."), oldname);
+ die(_("no commit on branch '%s' yet"), oldname);
else
- die(_("No branch named '%s'."), oldname);
+ die(_("no branch named '%s'"), oldname);
}
/*
@@ -624,9 +624,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if (!copy && !(oldref_usage & IS_ORPHAN) &&
rename_ref(oldref.buf, newref.buf, logmsg.buf))
- die(_("Branch rename failed"));
+ die(_("branch rename failed"));
if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
- die(_("Branch copy failed"));
+ die(_("branch copy failed"));
if (recovery) {
if (copy)
@@ -640,16 +640,16 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if (!copy && (oldref_usage & IS_HEAD) &&
replace_each_worktree_head_symref(worktrees, oldref.buf, newref.buf,
logmsg.buf))
- die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
+ die(_("branch renamed to %s, but HEAD is not updated!"), newname);
strbuf_release(&logmsg);
strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
strbuf_addf(&newsection, "branch.%s", interpreted_newname);
if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
- die(_("Branch is renamed, but update of config-file failed"));
+ die(_("branch is renamed, but update of config-file failed"));
if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
- die(_("Branch is copied, but update of config-file failed"));
+ die(_("branch is copied, but update of config-file failed"));
strbuf_release(&oldref);
strbuf_release(&newref);
strbuf_release(&oldsection);
@@ -773,7 +773,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
head = resolve_refdup("HEAD", 0, &head_oid, NULL);
if (!head)
- die(_("Failed to resolve HEAD as a valid ref."));
+ die(_("failed to resolve HEAD as a valid ref"));
if (!strcmp(head, "HEAD"))
filter.detached = 1;
else if (!skip_prefix(head, "refs/heads/", &head))
@@ -866,7 +866,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!argc) {
if (filter.detached)
- die(_("Cannot give description to detached HEAD"));
+ die(_("cannot give description to detached HEAD"));
branch_name = head;
} else if (argc == 1) {
strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
@@ -892,7 +892,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!argc)
die(_("branch name required"));
else if ((argc == 1) && filter.detached)
- die(copy? _("cannot copy the current branch while not on any.")
+ die(copy? _("cannot copy the current branch while not on any")
: _("cannot rename the current branch while not on any."));
else if (argc == 1)
copy_or_rename_branch(head, argv[0], copy, copy + rename > 1);
@@ -916,14 +916,14 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!branch) {
if (!argc || !strcmp(argv[0], "HEAD"))
die(_("could not set upstream of HEAD to %s when "
- "it does not point to any branch."),
+ "it does not point to any branch"),
new_upstream);
die(_("no such branch '%s'"), argv[0]);
}
if (!ref_exists(branch->refname)) {
if (!argc || branch_checked_out(branch->refname))
- die(_("No commit on branch '%s' yet."), branch->name);
+ die(_("no commit on branch '%s' yet"), branch->name);
die(_("branch '%s' does not exist"), branch->name);
}
@@ -946,12 +946,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!branch) {
if (!argc || !strcmp(argv[0], "HEAD"))
die(_("could not unset upstream of HEAD when "
- "it does not point to any branch."));
+ "it does not point to any branch"));
die(_("no such branch '%s'"), argv[0]);
}
if (!branch_has_merge_config(branch))
- die(_("Branch '%s' has no upstream information"), branch->name);
+ die(_("branch '%s' has no upstream information"), branch->name);
strbuf_reset(&buf);
strbuf_addf(&buf, "branch.%s.remote", branch->name);
@@ -965,11 +965,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
const char *start_name = argc == 2 ? argv[1] : head;
if (filter.kind != FILTER_REFS_BRANCHES)
- die(_("The -a, and -r, options to 'git branch' do not take a branch name.\n"
+ die(_("the -a, and -r, options to 'git branch' do not take a branch name\n"
"Did you mean to use: -a|-r --list <pattern>?"));
if (track == BRANCH_TRACK_OVERRIDE)
- die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead."));
+ die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead"));
if (recurse_submodules) {
create_branches_recursively(the_repository, branch_name,
--
2.42.0.325.g3a06386e31
^ permalink raw reply related
* Re: [Outreachy][PATCH 0/1] builtin/branch.c: ammend die() error message
From: Dragan Simic @ 2023-10-11 15:34 UTC (permalink / raw)
To: Isoken June Ibizugbe; +Cc: git
In-Reply-To: <20231011152424.6957-1-isokenjune@gmail.com>
On 2023-10-11 17:24, Isoken June Ibizugbe wrote:
> This patch improves the consistency and clarity of error messages
> across Git commands. It adheres to Git's Coding Guidelines for error
> messages:
>
> - Error messages no longer end with a full stop.
> - Capitalization is avoided in error messages.
> - Error messages lead with a description of the issue, enhancing
> readability.
>
> Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
When there's only one patch and not a series, there's no need for a
cover letter, and the description of the patch actually needs to go into
the patch itself, so it can be latter pulled into the repository as part
of the patch submission.
> Isoken June Ibizugbe (1):
> branch.c: ammend error messages for die()
>
> builtin/branch.c | 38 +++++++++++++++++++-------------------
> 1 file changed, 19 insertions(+), 19 deletions(-)
^ permalink raw reply
* Re: [Outreachy][PATCH 0/1] builtin/branch.c: ammend die() error message
From: Dragan Simic @ 2023-10-11 15:46 UTC (permalink / raw)
To: Isoken Ibizugbe; +Cc: git
In-Reply-To: <CAJHH8bGOSXNNYfOCkS=ck8r-=Mbq62Rs1c4NsgFWYD999kNfRw@mail.gmail.com>
On 2023-10-11 17:42, Isoken Ibizugbe wrote:
> Thanks for the feedback. How do I add the description of the patch.
Could you, please, use inline replying? We've asked that at least four
times already.
Regarding the description of the patch, just write it as the commit
comment in your local git repository. Using git-format-patch will pick
the commit comment up and put it into the email message file you'll then
send to the mailing list using git-send-email. It's all rather simple
and straightforward.
> On Wed, 11 Oct 2023 at 4:34 PM, Dragan Simic <dsimic@manjaro.org>
> wrote:
>
>> On 2023-10-11 17:24, Isoken June Ibizugbe wrote:
>>> This patch improves the consistency and clarity of error messages
>>> across Git commands. It adheres to Git's Coding Guidelines for
>> error
>>> messages:
>>>
>>> - Error messages no longer end with a full stop.
>>> - Capitalization is avoided in error messages.
>>> - Error messages lead with a description of the issue, enhancing
>>> readability.
>>>
>>> Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
>>
>> When there's only one patch and not a series, there's no need for a
>> cover letter, and the description of the patch actually needs to go
>> into
>> the patch itself, so it can be latter pulled into the repository as
>> part
>> of the patch submission.
>>
>>> Isoken June Ibizugbe (1):
>>> branch.c: ammend error messages for die()
>>>
>>> builtin/branch.c | 38 +++++++++++++++++++-------------------
>>> 1 file changed, 19 insertions(+), 19 deletions(-)
^ permalink raw reply
* [PATCH] upload-pack: add tracing for fetches
From: Robert Coup via GitGitGadget @ 2023-10-11 16:04 UTC (permalink / raw)
To: git; +Cc: Robert Coup, Robert Coup
From: Robert Coup <robert@coup.net.nz>
Information on how users are accessing hosted repositories can be
helpful to server operators. For example, being able to broadly
differentiate between fetches and initial clones; the use of shallow
repository features; or partial clone filters.
a29263c (fetch-pack: add tracing for negotiation rounds, 2022-08-02)
added some information on have counts to fetch-pack itself to help
diagnose negotiation; but from a git-upload-pack (server) perspective,
there's no means of accessing such information without using
GIT_TRACE_PACKET to examine the protocol packets.
Improve this by emitting a Trace2 JSON event from upload-pack with
summary information on the contents of a fetch request.
* haves, wants, and want-ref counts can help determine (broadly) between
fetches and clones, and the use of single-branch, etc.
* shallow clone depth, tip counts, and deepening options.
* any partial clone filter type.
Signed-off-by: Robert Coup <robert@coup.net.nz>
---
upload-pack: add tracing for fetches
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1598%2Frcoup%2Frc-upload-pack-trace-info-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1598/rcoup/rc-upload-pack-trace-info-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1598
t/t5500-fetch-pack.sh | 38 +++++++++++++++++++++++++++-----------
upload-pack.c | 29 +++++++++++++++++++++++++++++
2 files changed, 56 insertions(+), 11 deletions(-)
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index d18f2823d86..bb15ac34f77 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -132,13 +132,18 @@ test_expect_success 'single branch object count' '
'
test_expect_success 'single given branch clone' '
- git clone --single-branch --branch A "file://$(pwd)/." branch-a &&
- test_must_fail git --git-dir=branch-a/.git rev-parse origin/B
+ GIT_TRACE2_EVENT="$(pwd)/branch-a/trace2_event" \
+ git clone --single-branch --branch A "file://$(pwd)/." branch-a &&
+ test_must_fail git --git-dir=branch-a/.git rev-parse origin/B &&
+ grep \"fetch-info\".*\"haves\":0 branch-a/trace2_event &&
+ grep \"fetch-info\".*\"wants\":1 branch-a/trace2_event
'
test_expect_success 'clone shallow depth 1' '
- git clone --no-single-branch --depth 1 "file://$(pwd)/." shallow0 &&
- test "$(git --git-dir=shallow0/.git rev-list --count HEAD)" = 1
+ GIT_TRACE2_EVENT="$(pwd)/shallow0/trace2_event" \
+ git clone --no-single-branch --depth 1 "file://$(pwd)/." shallow0 &&
+ test "$(git --git-dir=shallow0/.git rev-list --count HEAD)" = 1 &&
+ grep \"fetch-info\".*\"depth\":1 shallow0/trace2_event
'
test_expect_success 'clone shallow depth 1 with fsck' '
@@ -235,7 +240,10 @@ test_expect_success 'add two more (part 2)' '
test_expect_success 'deepening pull in shallow repo' '
(
cd shallow &&
- git pull --depth 4 .. B
+ GIT_TRACE2_EVENT="$(pwd)/trace2_event" \
+ git pull --depth 4 .. B &&
+ grep \"fetch-info\".*\"depth\":4 trace2_event &&
+ grep \"fetch-info\".*\"shallows\":2 trace2_event
)
'
@@ -306,9 +314,12 @@ test_expect_success 'fetch --depth --no-shallow' '
test_expect_success 'turn shallow to complete repository' '
(
cd shallow &&
- git fetch --unshallow &&
+ GIT_TRACE2_EVENT="$(pwd)/trace2_event" \
+ git fetch --unshallow &&
! test -f .git/shallow &&
- git fsck --full
+ git fsck --full &&
+ grep \"fetch-info\".*\"shallows\":2 trace2_event &&
+ grep \"fetch-info\".*\"depth\":2147483647 trace2_event
)
'
@@ -826,13 +837,15 @@ test_expect_success 'clone shallow since ...' '
'
test_expect_success 'fetch shallow since ...' '
- git -C shallow11 fetch --shallow-since "200000000 +0700" origin &&
+ GIT_TRACE2_EVENT=$(pwd)/shallow11/trace2_event \
+ git -C shallow11 fetch --shallow-since "200000000 +0700" origin &&
git -C shallow11 log --pretty=tformat:%s origin/main >actual &&
cat >expected <<-\EOF &&
three
two
EOF
- test_cmp expected actual
+ test_cmp expected actual &&
+ grep \"fetch-info\".*\"deepen-since\":true shallow11/trace2_event
'
test_expect_success 'clone shallow since selects no commits' '
@@ -987,13 +1000,16 @@ test_expect_success 'filtering by size' '
test_config -C server uploadpack.allowfilter 1 &&
test_create_repo client &&
- git -C client fetch-pack --filter=blob:limit=0 ../server HEAD &&
+ GIT_TRACE2_EVENT=$(pwd)/client/trace2_event \
+ git -C client fetch-pack --filter=blob:limit=0 ../server HEAD &&
# Ensure that object is not inadvertently fetched
commit=$(git -C server rev-parse HEAD) &&
blob=$(git hash-object server/one.t) &&
git -C client rev-list --objects --missing=allow-any "$commit" >oids &&
- ! grep "$blob" oids
+ ! grep "$blob" oids &&
+
+ grep \"fetch-info\".*\"filter\":\"blob:limit\" client/trace2_event
'
test_expect_success 'filtering by size has no effect if support for it is not advertised' '
diff --git a/upload-pack.c b/upload-pack.c
index 83f3d2651ab..165c1c6350a 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -33,6 +33,7 @@
#include "commit-reach.h"
#include "shallow.h"
#include "write-or-die.h"
+#include "json-writer.h"
/* Remember to update object flag allocation in object.h */
#define THEY_HAVE (1u << 11)
@@ -1552,6 +1553,32 @@ static int parse_have(const char *line, struct oid_array *haves)
return 0;
}
+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, "wants", data->want_obj.nr);
+ jw_object_intmax(&jw, "want-refs", data->wanted_refs.nr);
+ 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_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));
+ else
+ jw_object_null(&jw, "filter");
+ }
+ jw_end(&jw);
+
+ trace2_data_json("upload-pack", the_repository, "fetch-info", &jw);
+
+ jw_release(&jw);
+}
+
static void process_args(struct packet_reader *request,
struct upload_pack_data *data)
{
@@ -1640,6 +1667,8 @@ static void process_args(struct packet_reader *request,
if (request->status != PACKET_READ_FLUSH)
die(_("expected flush after fetch arguments"));
+
+ trace2_fetch_info(data);
}
static int process_haves(struct upload_pack_data *data, struct oid_array *common)
base-commit: aab89be2eb6ca51eefeb8c8066f673f447058856
--
gitgitgadget
^ permalink raw reply related
* Re: Bug: git stash store can create stash entries that can't be dropped
From: Junio C Hamano @ 2023-10-11 16:29 UTC (permalink / raw)
To: Erik Cervin Edin; +Cc: Git Mailing List
In-Reply-To: <CA+JQ7M_effxh9BSOhF67N+rsvBVTULe0dWZzp-kq1yOiDq3+hQ@mail.gmail.com>
Erik Cervin Edin <erik@cervined.in> writes:
> ... even if it wasn't created using git stash create
I am of two minds. As "stash store" and "stash create" were
invented as, and have always been ever since, pretty much
implementation details of scripted "stash save", the user deserves
what they get when they abuse them: garbage-in, garbage-out.
> A stash entry is created that cannot be dropped, because it's not
> stash-like commit.
>
> git stash drop
> fatal: 'refs/stash@{0}' is not a stash-like commit
Yes, this is exactly what the user deserves.
Having said that, I agree that this shows an uneven UI. The "drop"
command cares about what it is dropping and refuses if it is not a
stash-like thing, so it is understandable to wish "store" to also
care to the same degree.
It may be just the matter of doing something silly like this. Not
even compile tested, but hopefully it is sufficient to convey the
idea.
builtin/stash.c | 6 ++++++
t/t3903-stash.sh | 4 ++++
2 files changed, 10 insertions(+)
diff --git c/builtin/stash.c w/builtin/stash.c
index 1ad496985a..4a6771c9f4 100644
--- c/builtin/stash.c
+++ w/builtin/stash.c
@@ -989,6 +989,12 @@ static int show_stash(int argc, const char **argv, const char *prefix)
static int do_store_stash(const struct object_id *w_commit, const char *stash_msg,
int quiet)
{
+ struct stash_info info;
+ char revision[GIT_MAX_HEXSZ];
+
+ oid_to_hex_r(revision, w_commit);
+ assert_stash_like(&info, revision);
+
if (!stash_msg)
stash_msg = "Created via \"git stash store\".";
diff --git c/t/t3903-stash.sh w/t/t3903-stash.sh
index 0b3dfeaea2..30b64260a8 100755
--- c/t/t3903-stash.sh
+++ w/t/t3903-stash.sh
@@ -931,6 +931,10 @@ test_expect_success 'store called with invalid commit' '
test_must_fail git stash store foo
'
+test_expect_success 'store called with non-stash commit' '
+ test_must_fail git stash store HEAD
+'
+
test_expect_success 'store updates stash ref and reflog' '
git stash clear &&
git reset --hard &&
^ permalink raw reply related
* Re: [PATCH] revision: Don't queue uninteresting commits
From: Junio C Hamano @ 2023-10-11 16:40 UTC (permalink / raw)
To: Øystein Walle; +Cc: git, derrickstolee
In-Reply-To: <20231011123534.119994-1-oystwa@gmail.com>
Øystein Walle <oystwa@gmail.com> writes:
> Currently all given commits are added to the topo_queue during
> init_topo_walk(). Later on in get_revision_1() the uninteresting ones
> are skipped because simplify_commit() tells it to.
>
> Let's not add them to the topo_queue in the first place.
What is not described here is what benefit we are expecting to gain
by making this change. Is anything leaking? Are we showing wrong
output? Is the effect something we can demonstrate, and more
importantly we can protect from future breakages, with a test or
two?
^ permalink raw reply
* Re: [PATCH 0/3] rev-list: add support for commits in `--missing`
From: Junio C Hamano @ 2023-10-11 16:54 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Patrick Steinhardt, git
In-Reply-To: <CAOLa=ZSbd_E+DAkhuGrUpfHkxaje3jrH9-fEDyctAPFExKnj9A@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> Seems like this is because of commit-graph being enabled, I think
> the best thing to do here would be to disable the commit graph of
> these tests.
If the CI uncovered that the new code is broken and does not work
with commit-graph, wouldn't the above a totally wrong approach to
correct it? If the updated logic cannot work correctly when
commit-graph covers the history you intentionally break, shouldn't
the code, when the feature that is incompatible with commit-graph is
triggered, disable the commit-graph? I am assuming that the new
feature is meant to be used to recover from a corrupt repository,
and if it does not work well when commit-graph knows (now stale
after repository corruption) more about the objects that are corrupt
in the object store, we do want to disable commit-graph. After all,
commit-graph is a secondary information that is supposed to be
recoverable from the primary data that is what is in the object
store. Disabling commit-graph in the test means you are telling the
end-users "do not use commit-graph if you want to use this feature",
which sounds like a wrong thing to do.
^ permalink raw reply
* [PATCH v4 0/2] attr: add attr.tree config
From: John Cai via GitGitGadget @ 2023-10-11 17:13 UTC (permalink / raw)
To: git; +Cc: Jeff King, Jonathan Tan, Eric Sunshine, John Cai
In-Reply-To: <pull.1577.v3.git.git.1696967380.gitgitgadget@gmail.com>
44451a2e5e (attr: teach "--attr-source=" global option to "git", 2023-05-06)
provided the ability to pass in a treeish as the attr source. When a
revision does not resolve to a valid tree is passed, Git will die. At
GitLab, we server repositories as bare repos and would like to always read
attributes from the default branch, so we'd like to pass in HEAD as the
treeish to read gitattributes from on every command. In this context we
would not want Git to die if HEAD is unborn, like in the case of empty
repositories.
Instead of modifying the default behavior of --attr-source, create a new
config attr.tree with which an admin can configure a ref for all commands to
read gitattributes from. Also make the default tree to read from HEAD on
bare repositories.
Changes since v2:
* relax the restrictions around attr.tree so that if it does not resolve to
a valid treeish, ignore it.
* add a commit to default to HEAD in bare repositories
Changes since v1:
* Added a commit to add attr.tree config
John Cai (2):
attr: read attributes from HEAD when bare repo
attr: add attr.tree for setting the treeish to read attributes from
Documentation/config.txt | 2 +
Documentation/config/attr.txt | 7 +++
attr.c | 19 +++++++-
attr.h | 2 +
config.c | 16 +++++++
t/t0003-attributes.sh | 84 +++++++++++++++++++++++++++++++++++
t/t5001-archive-attr.sh | 2 +-
7 files changed, 130 insertions(+), 2 deletions(-)
create mode 100644 Documentation/config/attr.txt
base-commit: 1fc548b2d6a3596f3e1c1f8b1930d8dbd1e30bf3
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1577%2Fjohn-cai%2Fjc%2Fconfig-attr-invalid-source-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1577/john-cai/jc/config-attr-invalid-source-v4
Pull-Request: https://github.com/git/git/pull/1577
Range-diff vs v3:
1: cef206d47c7 ! 1: eaa27c47810 attr: read attributes from HEAD when bare repo
@@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
+test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '
+ test_when_finished rm -rf test bare_with_gitattribute &&
+ git init test &&
-+ (
-+ cd test &&
-+ test_commit gitattributes .gitattributes "f/path test=val"
-+ ) &&
++ test_commit -C test gitattributes .gitattributes "f/path test=val" &&
+ git clone --bare test bare_with_gitattribute &&
+ echo "f/path: test: val" >expect &&
+ git -C bare_with_gitattribute check-attr test -- f/path >actual &&
2: dadb822da99 ! 2: 749d8a8082e attr: add attr.tree for setting the treeish to read attributes from
@@ Documentation/config.txt: other popular tools, and describe them in your documen
## Documentation/config/attr.txt (new) ##
@@
-+attr.tree:
-+ A <tree-ish> to read gitattributes from instead of the worktree. See
-+ linkgit:gitattributes[5]. If `attr.tree` does not resolve to a valid tree,
-+ treat it as an empty tree. --attr-source and GIT_ATTR_SOURCE take
-+ precedence over attr.tree.
++attr.tree::
++ A reference to a tree in the repository from which to read attributes,
++ instead of the `.gitattributes` file in the working tree. In a bare
++ repository, this defaults to `HEAD:.gitattributes`. If the value does
++ not resolve to a valid tree object, an empty tree is used instead.
++ When the `GIT_ATTR_SOURCE` environment variable or `--attr-source`
++ command line option are used, this configuration variable has no effect.
## attr.c ##
@@
@@ attr.c: static void compute_default_attr_source(struct object_id *attr_source)
if (!default_attr_source_tree_object_name)
default_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);
-+ if (!default_attr_source_tree_object_name) {
++ if (!default_attr_source_tree_object_name && git_attr_tree) {
+ default_attr_source_tree_object_name = git_attr_tree;
+ ignore_bad_attr_tree = 1;
+ }
@@ config.c: static int git_default_mailmap_config(const char *var, const char *val
+ if (!strcmp(var, "attr.tree"))
+ return git_config_string(&git_attr_tree, var, value);
+
-+ /* Add other attribute related config variables here and to
-+ Documentation/config/attr.txt. */
++ /*
++ * Add other attribute related config variables here and to
++ * Documentation/config/attr.txt.
++ */
+ return 0;
+}
+
@@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
+bad_attr_source_err="fatal: bad --attr-source or GIT_ATTR_SOURCE"
+
++test_expect_success '--attr-source is bad' '
++ test_when_finished rm -rf empty &&
++ git init empty &&
++ (
++ cd empty &&
++ echo "$bad_attr_source_err" >expect_err &&
++ test_must_fail git --attr-source=HEAD check-attr test -- f/path 2>err &&
++ test_cmp expect_err err
++ )
++'
++
+test_expect_success 'attr.tree when HEAD is unborn' '
+ test_when_finished rm -rf empty &&
+ git init empty &&
+ (
+ cd empty &&
-+ echo $bad_attr_source_err >expect_err &&
+ echo "f/path: test: unspecified" >expect &&
+ git -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&
+ test_must_be_empty err &&
@@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
+ git init empty &&
+ (
+ cd empty &&
-+ echo $bad_attr_source_err >expect_err &&
+ echo "f/path: test: unspecified" >expect &&
+ git -c attr.tree=refs/does/not/exist check-attr test -- f/path >actual 2>err &&
+ test_must_be_empty err &&
@@ t/t0003-attributes.sh: test_expect_success 'bare repo defaults to reading .gitat
test_cmp expect actual
'
-+test_expect_success '--attr-source and GIT_ATTR_SOURCE take precedence over attr.tree' '
++test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tree' '
+ test_when_finished rm -rf empty &&
+ git init empty &&
+ (
@@ t/t0003-attributes.sh: test_expect_success 'bare repo defaults to reading .gitat
+ test_commit "val2" .gitattributes "f/path test=val2" &&
+ git checkout attr-source &&
+ echo "f/path: test: val1" >expect &&
-+ git -c attr.tree=attr-tree --attr-source=attr-source check-attr test -- f/path >actual &&
++ GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree --attr-source=attr-source \
++ check-attr test -- f/path >actual &&
+ test_cmp expect actual &&
-+ GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree check-attr test -- f/path >actual &&
++ GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree \
++ check-attr test -- f/path >actual &&
+ test_cmp expect actual
+ )
+'
--
gitgitgadget
^ permalink raw reply
* [PATCH v4 1/2] attr: read attributes from HEAD when bare repo
From: John Cai via GitGitGadget @ 2023-10-11 17:13 UTC (permalink / raw)
To: git; +Cc: Jeff King, Jonathan Tan, Eric Sunshine, John Cai, John Cai
In-Reply-To: <pull.1577.v4.git.git.1697044422.gitgitgadget@gmail.com>
From: John Cai <johncai86@gmail.com>
The motivation for 44451a2e5e (attr: teach "--attr-source=<tree>" global
option to "git" , 2023-05-06), was to make it possible to use
gitattributes with bare repositories.
To make it easier to read gitattributes in bare repositories however,
let's just make HEAD:.gitattributes the default. This is in line with
how mailmap works, 8c473cecfd (mailmap: default mailmap.blob in bare
repositories, 2012-12-13).
Signed-off-by: John Cai <johncai86@gmail.com>
---
attr.c | 12 +++++++++++-
t/t0003-attributes.sh | 11 +++++++++++
t/t5001-archive-attr.sh | 2 +-
3 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/attr.c b/attr.c
index 71c84fbcf86..bf2ea1626a6 100644
--- a/attr.c
+++ b/attr.c
@@ -1194,6 +1194,7 @@ static void collect_some_attrs(struct index_state *istate,
}
static const char *default_attr_source_tree_object_name;
+static int ignore_bad_attr_tree;
void set_git_attr_source(const char *tree_object_name)
{
@@ -1205,10 +1206,19 @@ static void compute_default_attr_source(struct object_id *attr_source)
if (!default_attr_source_tree_object_name)
default_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);
+ if (!default_attr_source_tree_object_name &&
+ startup_info->have_repository &&
+ is_bare_repository()) {
+ default_attr_source_tree_object_name = "HEAD";
+ ignore_bad_attr_tree = 1;
+ }
+
if (!default_attr_source_tree_object_name || !is_null_oid(attr_source))
return;
- if (repo_get_oid_treeish(the_repository, default_attr_source_tree_object_name, attr_source))
+ if (repo_get_oid_treeish(the_repository,
+ default_attr_source_tree_object_name,
+ attr_source) && !ignore_bad_attr_tree)
die(_("bad --attr-source or GIT_ATTR_SOURCE"));
}
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 26e082f05b4..5665cdc079f 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -342,6 +342,17 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' '
)
'
+
+test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '
+ test_when_finished rm -rf test bare_with_gitattribute &&
+ git init test &&
+ test_commit -C test gitattributes .gitattributes "f/path test=val" &&
+ git clone --bare test bare_with_gitattribute &&
+ echo "f/path: test: val" >expect &&
+ git -C bare_with_gitattribute check-attr test -- f/path >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'bare repository: with --source' '
(
cd bare.git &&
diff --git a/t/t5001-archive-attr.sh b/t/t5001-archive-attr.sh
index 0ff47a239db..eaf959d8f63 100755
--- a/t/t5001-archive-attr.sh
+++ b/t/t5001-archive-attr.sh
@@ -138,7 +138,7 @@ test_expect_success 'git archive with worktree attributes, bare' '
'
test_expect_missing bare-worktree/ignored
-test_expect_exists bare-worktree/ignored-by-tree
+test_expect_missing bare-worktree/ignored-by-tree
test_expect_exists bare-worktree/ignored-by-worktree
test_expect_success 'export-subst' '
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 2/2] attr: add attr.tree for setting the treeish to read attributes from
From: John Cai via GitGitGadget @ 2023-10-11 17:13 UTC (permalink / raw)
To: git; +Cc: Jeff King, Jonathan Tan, Eric Sunshine, John Cai, John Cai
In-Reply-To: <pull.1577.v4.git.git.1697044422.gitgitgadget@gmail.com>
From: John Cai <johncai86@gmail.com>
44451a2 (attr: teach "--attr-source=<tree>" global option to "git",
2023-05-06) provided the ability to pass in a treeish as the attr
source. In the context of serving Git repositories as bare repos like we
do at GitLab however, it would be easier to point --attr-source to HEAD
for all commands by setting it once.
Add a new config attr.tree that allows this.
Signed-off-by: John Cai <johncai86@gmail.com>
---
Documentation/config.txt | 2 +
Documentation/config/attr.txt | 7 ++++
attr.c | 7 ++++
attr.h | 2 +
config.c | 16 ++++++++
t/t0003-attributes.sh | 73 +++++++++++++++++++++++++++++++++++
6 files changed, 107 insertions(+)
create mode 100644 Documentation/config/attr.txt
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 229b63a454c..b1891c2b5af 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -371,6 +371,8 @@ other popular tools, and describe them in your documentation.
include::config/advice.txt[]
+include::config/attr.txt[]
+
include::config/core.txt[]
include::config/add.txt[]
diff --git a/Documentation/config/attr.txt b/Documentation/config/attr.txt
new file mode 100644
index 00000000000..1a482d6af2b
--- /dev/null
+++ b/Documentation/config/attr.txt
@@ -0,0 +1,7 @@
+attr.tree::
+ A reference to a tree in the repository from which to read attributes,
+ instead of the `.gitattributes` file in the working tree. In a bare
+ repository, this defaults to `HEAD:.gitattributes`. If the value does
+ not resolve to a valid tree object, an empty tree is used instead.
+ When the `GIT_ATTR_SOURCE` environment variable or `--attr-source`
+ command line option are used, this configuration variable has no effect.
diff --git a/attr.c b/attr.c
index bf2ea1626a6..e62876dfd3e 100644
--- a/attr.c
+++ b/attr.c
@@ -24,6 +24,8 @@
#include "tree-walk.h"
#include "object-name.h"
+const char *git_attr_tree;
+
const char git_attr__true[] = "(builtin)true";
const char git_attr__false[] = "\0(builtin)false";
static const char git_attr__unknown[] = "(builtin)unknown";
@@ -1206,6 +1208,11 @@ static void compute_default_attr_source(struct object_id *attr_source)
if (!default_attr_source_tree_object_name)
default_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);
+ if (!default_attr_source_tree_object_name && git_attr_tree) {
+ default_attr_source_tree_object_name = git_attr_tree;
+ ignore_bad_attr_tree = 1;
+ }
+
if (!default_attr_source_tree_object_name &&
startup_info->have_repository &&
is_bare_repository()) {
diff --git a/attr.h b/attr.h
index 2b745df4054..127998ae013 100644
--- a/attr.h
+++ b/attr.h
@@ -236,4 +236,6 @@ const char *git_attr_global_file(void);
/* Return whether the system gitattributes file is enabled and should be used. */
int git_attr_system_is_enabled(void);
+extern const char *git_attr_tree;
+
#endif /* ATTR_H */
diff --git a/config.c b/config.c
index 3846a37be97..fb6a2db1d9b 100644
--- a/config.c
+++ b/config.c
@@ -18,6 +18,7 @@
#include "repository.h"
#include "lockfile.h"
#include "mailmap.h"
+#include "attr.h"
#include "exec-cmd.h"
#include "strbuf.h"
#include "quote.h"
@@ -1904,6 +1905,18 @@ static int git_default_mailmap_config(const char *var, const char *value)
return 0;
}
+static int git_default_attr_config(const char *var, const char *value)
+{
+ if (!strcmp(var, "attr.tree"))
+ return git_config_string(&git_attr_tree, var, value);
+
+ /*
+ * Add other attribute related config variables here and to
+ * Documentation/config/attr.txt.
+ */
+ return 0;
+}
+
int git_default_config(const char *var, const char *value,
const struct config_context *ctx, void *cb)
{
@@ -1927,6 +1940,9 @@ int git_default_config(const char *var, const char *value,
if (starts_with(var, "mailmap."))
return git_default_mailmap_config(var, value);
+ if (starts_with(var, "attr."))
+ return git_default_attr_config(var, value);
+
if (starts_with(var, "advice.") || starts_with(var, "color.advice"))
return git_default_advice_config(var, value);
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 5665cdc079f..e9953ce19c5 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -40,6 +40,10 @@ attr_check_source () {
test_cmp expect actual &&
test_must_be_empty err
+ git $git_opts -c "attr.tree=$source" check-attr test -- "$path" >actual 2>err &&
+ test_cmp expect actual &&
+ test_must_be_empty err
+
GIT_ATTR_SOURCE="$source" git $git_opts check-attr test -- "$path" >actual 2>err &&
test_cmp expect actual &&
test_must_be_empty err
@@ -342,6 +346,55 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' '
)
'
+bad_attr_source_err="fatal: bad --attr-source or GIT_ATTR_SOURCE"
+
+test_expect_success '--attr-source is bad' '
+ test_when_finished rm -rf empty &&
+ git init empty &&
+ (
+ cd empty &&
+ echo "$bad_attr_source_err" >expect_err &&
+ test_must_fail git --attr-source=HEAD check-attr test -- f/path 2>err &&
+ test_cmp expect_err err
+ )
+'
+
+test_expect_success 'attr.tree when HEAD is unborn' '
+ test_when_finished rm -rf empty &&
+ git init empty &&
+ (
+ cd empty &&
+ echo "f/path: test: unspecified" >expect &&
+ git -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&
+ test_must_be_empty err &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'attr.tree points to non-existing ref' '
+ test_when_finished rm -rf empty &&
+ git init empty &&
+ (
+ cd empty &&
+ echo "f/path: test: unspecified" >expect &&
+ git -c attr.tree=refs/does/not/exist check-attr test -- f/path >actual 2>err &&
+ test_must_be_empty err &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'bad attr source defaults to reading .gitattributes file' '
+ test_when_finished rm -rf empty &&
+ git init empty &&
+ (
+ cd empty &&
+ echo "f/path test=val" >.gitattributes &&
+ echo "f/path: test: val" >expect &&
+ git -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&
+ test_must_be_empty err &&
+ test_cmp expect actual
+ )
+'
test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '
test_when_finished rm -rf test bare_with_gitattribute &&
@@ -353,6 +406,26 @@ test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '
test_cmp expect actual
'
+test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tree' '
+ test_when_finished rm -rf empty &&
+ git init empty &&
+ (
+ cd empty &&
+ git checkout -b attr-source &&
+ test_commit "val1" .gitattributes "f/path test=val1" &&
+ git checkout -b attr-tree &&
+ test_commit "val2" .gitattributes "f/path test=val2" &&
+ git checkout attr-source &&
+ echo "f/path: test: val1" >expect &&
+ GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree --attr-source=attr-source \
+ check-attr test -- f/path >actual &&
+ test_cmp expect actual &&
+ GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree \
+ check-attr test -- f/path >actual &&
+ test_cmp expect actual
+ )
+'
+
test_expect_success 'bare repository: with --source' '
(
cd bare.git &&
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 1/1] branch.c: ammend error messages for die()
From: Junio C Hamano @ 2023-10-11 17:29 UTC (permalink / raw)
To: Isoken June Ibizugbe; +Cc: git
In-Reply-To: <20231011152424.6957-2-isokenjune@gmail.com>
Isoken June Ibizugbe <isokenjune@gmail.com> writes:
> Subject: Re: [PATCH 1/1] branch.c: ammend error messages for die()
"ammend" is misspelt, but more importantly, it has less information
contents than other possible phrases, e.g.,
Subject: [PATCH 1/1] branch.c: adjust die() messages to coding guidelines
In any case, the title of a commit has insufficient space to
describe what the amendment is about, or which exact guideline
these messages violate and needs adjustment. This space before your
sign-off is where you write it.
Other outreachy candidates have already been given pretty much the
same pieces of advice. It may help candidates to learn from the
responses given to other candidates. For example, I said the same
thing in https://lore.kernel.org/git/xmqqlecbzl5e.fsf@gitster.g/
> Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
> ---
> builtin/branch.c | 38 +++++++++++++++++++-------------------
> 1 file changed, 19 insertions(+), 19 deletions(-)
Not a fault of this patch at all, but it is somewhat surprising that
we do not break any existing test with this many messages changed.
Did you run the test suite before making this commit?
Make it a habit to always do "make test" before committing your
work. I am not saying "do not commit what does not pass the tests".
What I mean is "be aware of what is still broken (when fixing a bug)
or what you broke (when adding a new feature, perhaps as an
unintended side effect), before you commit, so that you can describe
them in your commit log message".
Thanks.
^ permalink raw reply
* Re: [PATCH] t-progress.c : unit tests for progress.c
From: Junio C Hamano @ 2023-10-11 17:29 UTC (permalink / raw)
To: Siddharth Singh; +Cc: git, nasamuffin
In-Reply-To: <20231011082716.901048-1-siddhartth@google.com>
Siddharth Singh <siddhartth@google.com> writes:
> These tests are directly inspired from the tests inside
> t/t0500-progress-display.sh
>
> The existing shell tests for the Git progress library only test the output of the library, not the correctness of the progress struct fields. Unit tests can fill this gap and improve confidence that the library works as expected. For example, unit tests can verify that the progress struct fields are updated correctly when the library is used.
>
> Change-Id: I190522f29fdab9291af71b7788eeee2c0f26282d
> Signed-off-by: Siddharth Singh <siddhartth@google.com>
> ---
The contents is of course important, but please also pay attention
to the formatting to make what you write readable. Writing good
things does not help if it is not read. Wrap lines to appropriate
lengths.
Documentation/SubmittingPatches is your friend.
>
> Dear Git Community,
> As you may be aware, my colleague Josh is proposing a unit testing
> framework[1] on the mailing list. I attempted to use the latest
> version of that series to convert t/helper/test-progress.c to unit
> tests. However, while writing the tests, I realized that the way
> progress.c is implemented makes it very difficult to test it in
> units.
>
> Firstly, most unit tests are typically written as a method that
> takes the expected output and the actual output of the unit being
> tested, and compares them for equality. However, because it's
> intended to print user-facing output on an interactive terminal,
> progress.c prints everything out to stderr, which makes it
> difficult to unit test.
It sounds like you found where the test framework is lacking. If
making sure what we spew out to the standard error stream is worth
covering in the unit tests, the test framework needs to support it,
right?
> There are a few ways to work around this issue in my opinion. One
> way is to modify the library that does not print to output stream
> and returns the data to the caller:
>
> static void display(struct progress *progress, uint64_t n, char *done){
> …
> }
>
> becomes
>
> struct strbuf display(struct progress *progress,uint64_t n,char *done){
> …
> }
The approach adds a feature for outside callers to access this bit
of internal implementation detail of the progress code. If no real
callers want to exercise that feature and it is only useful for
testing, it smells like the tail wagging the dog, though.
It certainly is not worth butchering the real code for the sake of
working around the lack of current unit test framework to
contaminate the global namespace with way too generically named
function "display()".
Assuming that you *must* work around the lack of stderr support, it
probably would make much more sense to add a new member that points
at an output stream to "struct progress". Make it a thin wrapper
around "FILE *" that supports whatever display() and the helper
functions it calls needs, like write(2) or fprintf(3). And you can
mock that "output stream" in your unit tests to append to an in-core
buffer if you wanted to. The production code does not have to care
about your mock that way.
^ permalink raw reply
* Re: [PATCH 1/1] branch.c: ammend error messages for die()
From: Rubén Justo @ 2023-10-11 18:04 UTC (permalink / raw)
To: Isoken June Ibizugbe; +Cc: git, Junio C Hamano
In-Reply-To: <20231011152424.6957-2-isokenjune@gmail.com>
On 11-oct-2023 16:24:20, Isoken June Ibizugbe wrote:
Hi Isoken,
> Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
> ---
> builtin/branch.c | 38 +++++++++++++++++++-------------------
> 1 file changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 2ec190b14a..a756543d64 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -518,11 +518,11 @@ static void reject_rebase_or_bisect_branch(struct worktree **worktrees,
> continue;
>
> if (is_worktree_being_rebased(wt, target))
> - die(_("Branch %s is being rebased at %s"),
> + die(_("branch %s is being rebased at %s"),
OK.
> target, wt->path);
>
> if (is_worktree_being_bisected(wt, target))
> - die(_("Branch %s is being bisected at %s"),
> + die(_("branch %s is being bisected at %s"),
OK.
> target, wt->path);
> }
> }
> @@ -578,7 +578,7 @@ 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);
> + die(_("invalid branch name: '%s'"), oldname);
OK.
> }
>
> for (int i = 0; worktrees[i]; i++) {
> @@ -594,9 +594,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>
> if ((copy || !(oldref_usage & IS_HEAD)) && !ref_exists(oldref.buf)) {
> if (oldref_usage & IS_HEAD)
> - die(_("No commit on branch '%s' yet."), oldname);
> + die(_("no commit on branch '%s' yet"), oldname);
> else
> - die(_("No branch named '%s'."), oldname);
> + die(_("no branch named '%s'"), oldname);
OK. Both.
> }
>
> /*
> @@ -624,9 +624,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>
> if (!copy && !(oldref_usage & IS_ORPHAN) &&
> rename_ref(oldref.buf, newref.buf, logmsg.buf))
> - die(_("Branch rename failed"));
> + die(_("branch rename failed"));
> if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
> - die(_("Branch copy failed"));
> + die(_("branch copy failed"));
Ditto
>
> if (recovery) {
> if (copy)
> @@ -640,16 +640,16 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
> if (!copy && (oldref_usage & IS_HEAD) &&
> replace_each_worktree_head_symref(worktrees, oldref.buf, newref.buf,
> logmsg.buf))
> - die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
> + die(_("branch renamed to %s, but HEAD is not updated!"), newname);
IMO we can go further here and also remove that final "!". But it's OK
the way you have done it.
>
> strbuf_release(&logmsg);
>
> strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
> strbuf_addf(&newsection, "branch.%s", interpreted_newname);
> if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
> - die(_("Branch is renamed, but update of config-file failed"));
> + die(_("branch is renamed, but update of config-file failed"));
> if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
> - die(_("Branch is copied, but update of config-file failed"));
> + die(_("branch is copied, but update of config-file failed"));
OK, both.
> strbuf_release(&oldref);
> strbuf_release(&newref);
> strbuf_release(&oldsection);
> @@ -773,7 +773,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>
> head = resolve_refdup("HEAD", 0, &head_oid, NULL);
> if (!head)
> - die(_("Failed to resolve HEAD as a valid ref."));
> + die(_("failed to resolve HEAD as a valid ref"));
OK.
> if (!strcmp(head, "HEAD"))
> filter.detached = 1;
> else if (!skip_prefix(head, "refs/heads/", &head))
> @@ -866,7 +866,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>
> if (!argc) {
> if (filter.detached)
> - die(_("Cannot give description to detached HEAD"));
> + die(_("cannot give description to detached HEAD"));
OK.
> branch_name = head;
> } else if (argc == 1) {
> strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
> @@ -892,7 +892,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> if (!argc)
> die(_("branch name required"));
> else if ((argc == 1) && filter.detached)
> - die(copy? _("cannot copy the current branch while not on any.")
> + die(copy? _("cannot copy the current branch while not on any")
> : _("cannot rename the current branch while not on any."));
OK. But I think you want to modify also the second message, to remove
its full stop as well.
> else if (argc == 1)
> copy_or_rename_branch(head, argv[0], copy, copy + rename > 1);
> @@ -916,14 +916,14 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> if (!branch) {
> if (!argc || !strcmp(argv[0], "HEAD"))
> die(_("could not set upstream of HEAD to %s when "
> - "it does not point to any branch."),
> + "it does not point to any branch"),
OK.
> new_upstream);
> die(_("no such branch '%s'"), argv[0]);
> }
>
> if (!ref_exists(branch->refname)) {
> if (!argc || branch_checked_out(branch->refname))
> - die(_("No commit on branch '%s' yet."), branch->name);
> + die(_("no commit on branch '%s' yet"), branch->name);
OK.
> die(_("branch '%s' does not exist"), branch->name);
> }
>
> @@ -946,12 +946,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> if (!branch) {
> if (!argc || !strcmp(argv[0], "HEAD"))
> die(_("could not unset upstream of HEAD when "
> - "it does not point to any branch."));
> + "it does not point to any branch"));
> die(_("no such branch '%s'"), argv[0]);
> }
>
> if (!branch_has_merge_config(branch))
> - die(_("Branch '%s' has no upstream information"), branch->name);
> + die(_("branch '%s' has no upstream information"), branch->name);
OK, both.
>
> strbuf_reset(&buf);
> strbuf_addf(&buf, "branch.%s.remote", branch->name);
> @@ -965,11 +965,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> const char *start_name = argc == 2 ? argv[1] : head;
>
> if (filter.kind != FILTER_REFS_BRANCHES)
> - die(_("The -a, and -r, options to 'git branch' do not take a branch name.\n"
> + die(_("the -a, and -r, options to 'git branch' do not take a branch name\n"
> "Did you mean to use: -a|-r --list <pattern>?"));
Good; the full stop removed and here that question mark is valuable. And ...
>
> if (track == BRANCH_TRACK_OVERRIDE)
> - die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead."));
> + die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead"));
also good. But since we're here, maybe we can break this long message, remove
the first full stop and leave the second part of the message as is, as a
suggestion. Like we do in the previous message, above.
For your consideration, I mean something like:
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -969,7 +969,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
"Did you mean to use: -a|-r --list <pattern>?"));
if (track == BRANCH_TRACK_OVERRIDE)
- die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead."));
+ die(_("the '--set-upstream' option is no longer supported\n"
+ "Please use '--track' or '--set-upstream-to' instead."));
>
> if (recurse_submodules) {
> create_branches_recursively(the_repository, branch_name,
> --
> 2.42.0.325.g3a06386e31
>
One final comment, leaving aside my suggestions; this changes break
some tests that you need to adjust. Try:
$ make && (cd t; ./t3200-branch.sh -vi)
Or, as Junio has already suggested in another message:
$ make test
I have some unfinished work that you might find useful:
https://lore.kernel.org/git/eb3c689e-efeb-4468-a10f-dd32bc0ee37b@gmail.com/
Thank you for working on this.
^ permalink raw reply
* Re: gpg.ssh.defaultKeyCommand docs bug?
From: matthew sporleder @ 2023-10-11 18:16 UTC (permalink / raw)
To: Jeff King; +Cc: Fabian Stelzer, Git Mailing List
In-Reply-To: <20231009204341.GB3281325@coredump.intra.peff.net>
On Mon, Oct 9, 2023 at 4:43 PM Jeff King <peff@peff.net> wrote:
>
> [+cc Fabian, who wrote this code]
>
> On Fri, Oct 06, 2023 at 01:14:49PM -0400, matthew sporleder wrote:
>
> > https://git-scm.com/docs/git-config#Documentation/git-config.txt-gpgsshdefaultKeyCommand
> >
> > This command that will be run when user.signingkey is not set and a
> > ssh signature is requested. On successful exit a valid ssh public key
> > prefixed with key:: is expected in the first line of its output. This
> > allows for a script doing a dynamic lookup of the correct public key
> > when it is impractical to statically configure user.signingKey. For
> > example when keys or SSH Certificates are rotated frequently or
> > selection of the right key depends on external factors unknown to git.
> >
> > ---
> >
> > The command does not actually work (for me, git version 2.42.0) with
> > key:: prefixed.
> >
> > It only works if I cat the public key as-is.
> >
> > I only figured this out because the docs previously said it took the
> > format of ssh-add -L, which also doesn't not contain key::.
> >
> > I am using this script for my "dynamic" key discovery:
> > #!/bin/sh
> > f=$(ssh -G $(git remote get-url $(git remote|head -1)|awk -F':' '{
> > print $1 }') |grep -E '^identityfile'|sed 's#^identityfile ##g')
> > cat $(eval realpath ${f}.pub)
>
> I'm not very familiar with this part of Git, but looking at the code
> which parses the output of gpg.ssh.defaultKeyCommand, it splits it by
> line and then calls is_literal_ssh_key() on it, which is:
>
> static int is_literal_ssh_key(const char *string, const char **key)
> {
> if (skip_prefix(string, "key::", key))
> return 1;
> if (starts_with(string, "ssh-")) {
> *key = string;
> return 1;
> }
> return 0;
> }
>
> So your script works because the pub file starts with "ssh-rsa" or
> similar (and so would "ssh-add -L" output).
>
> The user.signingKey docs say:
>
> For backward compatibility, a raw key which begins with "ssh-", such
> as "ssh-rsa XXXXXX identifier", is treated as "key::ssh-rsa XXXXXX
> identifier", but this form is deprecated; use the key:: form instead.
>
> From reading the commit messages here, I guess this is about supporting
> non-ssh key types (e.g., my TPM-based key is ecdsa-sha2-nistp256 in the
> "ssh-add -L" output). But I'm not sure who is supposed to be put "key::"
> there.
>
> You said it "does not actually work" with "key::" prefixed. What
> happens? In the signing code we make a similar call to
> is_literal_ssh_key() that wills trip off the "key::" prefix, so I'd
> expect it work. But I could also believe there is a bug. :)
>
> -Peff
It gave very confusing errors!
key::ssh-rsa ABC123 me@localhost (no new line)
error: Load key "....: invalid format?
key::ABC123 (yes new line)
error: Couldn't load public key ...: No such file or directory?
key::ssh-rsa ABC123 me@localhost (yes new line)
works, I think
ssh-rsa ABC123 me@localhost (yes new line)
works (the script I provided)
^ permalink raw reply
* Re: Bug: 'git stash --staged' crashes with binary files
From: Jeff King @ 2023-10-11 18:17 UTC (permalink / raw)
To: Adam Johnson; +Cc: git
In-Reply-To: <CAMyDDM3DFyru6zph4qqf_QoaOeezvYkT7SmwinCfJNnFsnuRjQ@mail.gmail.com>
On Thu, Jun 01, 2023 at 10:26:13PM +0100, Adam Johnson wrote:
> Stage a binary file and run 'git stash --staged'. The stash is created but
> the command fails to remove changes from the index:
>
> $ echo -n "\0" >file
>
> $ git add file
>
> $ git stash --staged
> Saved working directory and index state WIP on main: e7911b6 Whatever
> error: cannot apply binary patch to 'file' without full index line
> error: file: patch does not apply
> Cannot remove worktree changes
>
> $ git status
> ...
> Changes to be committed:
> new file: file
>
> The "remove from the index" step seems not to be binary-compatible.
This seems like a bug. It looks like stash does pass "--binary" in some
cases, but not all. So it's probably a matter of finding the right
invocation and adding it.
> The below patch adds a reproducing test.
I think finding the bug and writing the test is probably 75% of the
work. :)
> diff --git t/t3903-stash.sh t/t3903-stash.sh
> index 376cc8f4ab..5e3ea64f38 100755
> --- t/t3903-stash.sh
> +++ t/t3903-stash.sh
> @@ -392,6 +392,13 @@ setup_stash()
> git stash pop &&
> test bar,bar4 = $(cat file),$(cat file2)
> '
> +test_expect_success 'stash --staged with binary file' '
> + echo -n "\0" >file &&
Unfortunately "echo -n" isn't portable. But you can use:
printf "\0" >file
instead.
> + git add file &&
> + git stash --staged &&
> + git stash pop &&
> + test "\0" = $(cat file)
> +'
I doubt this "test" will work, as the shell won't interpolate that into
a NUL (and anyway, I think having NULs in shell variables isn't
portable). You could perhaps do:
printf "\0" >expect &&
test_cmp expect file
-Peff
^ permalink raw reply
* RE: Bug: 'git stash --staged' crashes with binary files
From: rsbecker @ 2023-10-11 18:23 UTC (permalink / raw)
To: 'Jeff King', 'Adam Johnson'; +Cc: git
In-Reply-To: <20231011181733.GA514244@coredump.intra.peff.net>
On Wednesday, October 11, 2023 2:18 PM, Peff wrote:
>On Thu, Jun 01, 2023 at 10:26:13PM +0100, Adam Johnson wrote:
>
>> Stage a binary file and run 'git stash --staged'. The stash is created
>> but the command fails to remove changes from the index:
>>
>> $ echo -n "\0" >file
>>
>> $ git add file
>>
>> $ git stash --staged
>> Saved working directory and index state WIP on main: e7911b6 Whatever
>> error: cannot apply binary patch to 'file' without full index line
>> error: file: patch does not apply
>> Cannot remove worktree changes
>>
>> $ git status
>> ...
>> Changes to be committed:
>> new file: file
>>
>> The "remove from the index" step seems not to be binary-compatible.
>
>This seems like a bug. It looks like stash does pass "--binary" in some cases, but not
>all. So it's probably a matter of finding the right invocation and adding it.
>
>> The below patch adds a reproducing test.
>
>I think finding the bug and writing the test is probably 75% of the work. :)
>
>> diff --git t/t3903-stash.sh t/t3903-stash.sh index
>> 376cc8f4ab..5e3ea64f38 100755
>> --- t/t3903-stash.sh
>> +++ t/t3903-stash.sh
>> @@ -392,6 +392,13 @@ setup_stash()
>> git stash pop &&
>> test bar,bar4 = $(cat file),$(cat file2) '
>> +test_expect_success 'stash --staged with binary file' '
>> + echo -n "\0" >file &&
>
>Unfortunately "echo -n" isn't portable. But you can use:
>
> printf "\0" >file
>
>instead.
>
>> + git add file &&
>> + git stash --staged &&
>> + git stash pop &&
>> + test "\0" = $(cat file)
>> +'
>
>I doubt this "test" will work, as the shell won't interpolate that into a NUL (and
>anyway, I think having NULs in shell variables isn't portable). You could perhaps do:
>
> printf "\0" >expect &&
> test_cmp expect file
Agreed. The specific test construct does not appear portable, but piping printf "\0" to expect works.
/home/randall: test "\0" != `printf "\0"`
bash: warning: command substitution: ignored null byte in input
bash: test: \0: unary operator expected
--Randall
^ permalink raw reply
* Re: [PATCH 09/20] midx: check size of object offset chunk
From: Taylor Blau @ 2023-10-11 18:31 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231009210527.GI3282181@coredump.intra.peff.net>
On Mon, Oct 09, 2023 at 05:05:27PM -0400, Jeff King wrote:
> @@ -88,6 +88,19 @@ static int midx_read_oid_lookup(const unsigned char *chunk_start,
> return 0;
> }
>
> +static int midx_read_object_offsets(const unsigned char *chunk_start,
> + size_t chunk_size, void *data)
> +{
> + struct multi_pack_index *m = data;
> + m->chunk_object_offsets = chunk_start;
> +
> + if (chunk_size != st_mult(m->num_objects, MIDX_CHUNK_OFFSET_WIDTH)) {
> + error(_("multi-pack-index object offset chunk is the wrong size"));
> + return 1;
> + }
> + return 0;
> +}
Makes sense, and the (elided) test below looks good, too. I think that
this is another case that would benefit from having the chunk-format API
take in an "expected size" and validate that the requested chunk is
actually that size before assigning its address to some pointer.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 10/20] midx: bounds-check large offset chunk
From: Taylor Blau @ 2023-10-11 18:38 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231009210530.GJ3282181@coredump.intra.peff.net>
On Mon, Oct 09, 2023 at 05:05:30PM -0400, Jeff King wrote:
> When we see a large offset bit in the regular midx offset table, we
> use the entry as an index into a separate large offset table (just like
> a pack idx does). But we don't bounds-check the access to that large
> offset table (nor even record its size when we parse the chunk!).
>
> The equivalent code for a regular pack idx is in check_pack_index_ptr().
> But things are a bit simpler here because of the chunked format: we can
> just check our array index directly.
>
> As a bonus, we can get rid of the st_mult() here. If our array
> bounds-check is successful, then we know that the result will fit in a
> size_t (and the bounds check uses a division to avoid overflow
> entirely).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> midx.c | 8 +++++---
> midx.h | 1 +
> t/t5319-multi-pack-index.sh | 20 ++++++++++++++++++++
> 3 files changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/midx.c b/midx.c
> index 7b1b45f381..3e768d0df0 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -180,7 +180,8 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local
> if (read_chunk(cf, MIDX_CHUNKID_OBJECTOFFSETS, midx_read_object_offsets, m))
> die(_("multi-pack-index required object offsets chunk missing or corrupted"));
>
> - pair_chunk_unsafe(cf, MIDX_CHUNKID_LARGEOFFSETS, &m->chunk_large_offsets);
> + pair_chunk(cf, MIDX_CHUNKID_LARGEOFFSETS, &m->chunk_large_offsets,
> + &m->chunk_large_offsets_len);
>
> if (git_env_bool("GIT_TEST_MIDX_READ_RIDX", 1))
> pair_chunk_unsafe(cf, MIDX_CHUNKID_REVINDEX, &m->chunk_revindex);
> @@ -303,8 +304,9 @@ off_t nth_midxed_offset(struct multi_pack_index *m, uint32_t pos)
> die(_("multi-pack-index stores a 64-bit offset, but off_t is too small"));
>
> offset32 ^= MIDX_LARGE_OFFSET_NEEDED;
> - return get_be64(m->chunk_large_offsets +
> - st_mult(sizeof(uint64_t), offset32));
> + if (offset32 >= m->chunk_large_offsets_len / sizeof(uint64_t))
> + die(_("multi-pack-index large offset out of bounds"));
> + return get_be64(m->chunk_large_offsets + sizeof(uint64_t) * offset32);
Makes sense, and this seems very reasonable. I had a couple of thoughts
on this hunk, one nitpick, and one non-nitpick ;-).
The nitpick is the easy one, which is that I typically think of scaling
some index to produce an offset into some chunk, instead of dividing and
going the other way.
So I probably would have written something like:
if (st_mult(offset32, sizeof(uint64_t)) >= m->chunk_large_offsets_len)
die(_("multi-pack-index large offset out of bounds"));
But that is definitely a subjective/minor point, and I would not at all
be sad if you felt differently about it. That said, I do wish for a
little more information in the die() message, perhaps:
if (st_mult(offset32, sizeof(uint64_t)) >= m->chunk_large_offsets_len)
die(_("multi-pack-index large offset for %s out of bounds "
"(%"PRIuMAX" is beyond %"PRIuMAX")"),
(uintmax_t)(offset32 * sizeof(uint64_t)), /* checked earlier */
(uintmax_t)m->chunk_large_offsets_len);
I can imagine that for debugging corrupt MIDXs in the future, having
some extra information like the above would give us a better starting
point than popping into a debugger to get the same information.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 11/20] midx: check size of revindex chunk
From: Taylor Blau @ 2023-10-11 18:41 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231009210533.GK3282181@coredump.intra.peff.net>
On Mon, Oct 09, 2023 at 05:05:33PM -0400, Jeff King wrote:
> +static int can_use_midx_ridx_chunk(struct multi_pack_index *m)
> +{
> + if (!m->chunk_revindex)
> + return 0;
> + if (m->chunk_revindex_len != st_mult(sizeof(uint32_t), m->num_objects)) {
> + error(_("multi-pack-index reverse-index chunk is the wrong size"));
> + return 0;
> + }
> + return 1;
> +}
This all looks good to me. I was going to suggest that it might be worth
just NULL-ing out the chunk_revindex field here altogether. I think that
we have some prior art for doing that in the commit-graph code (though
I'd have to look to be sure...), but that's all within the same
compilation unit.
Having the pack-revindex machinery NULL out a field of the MIDX
structure feels icky to me, so I think that having a "can we use this
function?" instead makes much more sense.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 12/20] commit-graph: check size of commit data chunk
From: Taylor Blau @ 2023-10-11 18:46 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231009210536.GL3282181@coredump.intra.peff.net>
On Mon, Oct 09, 2023 at 05:05:36PM -0400, Jeff King wrote:
> We expect a commit-graph file to have a fixed-size data record for each
> commit in the file (and we know the number of commits to expct from the
> size of the lookup table). If we encounter a file where this is too
> small, we'll look past the end of the chunk (and possibly even off the
> mapped memory).
>
> We can fix this by checking the size up front when we record the
> pointer.
>
> The included test doesn't segfault, since it ends up reading bytes
> from another chunk. But it produces nonsense results, since the values
> it reads are garbage. Our test notices this by comparing the output to a
> non-corrupted run of the same command (and of course we also check that
> the expected error is printed to stderr).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> commit-graph.c | 12 +++++++++++-
> t/t5318-commit-graph.sh | 9 +++++++++
> 2 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 472332f603..9b80bbd75b 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -340,6 +340,16 @@ static int graph_read_oid_lookup(const unsigned char *chunk_start,
> return 0;
> }
>
> +static int graph_read_commit_data(const unsigned char *chunk_start,
> + size_t chunk_size, void *data)
> +{
> + struct commit_graph *g = data;
> + if (chunk_size != g->num_commits * GRAPH_DATA_WIDTH)
Should this be guarded with an st_mult? I think that GRAPH_DATA_WIDTH is
defined as (the_hash_algo->rawsz + 16), so I *think* that the expression
in the parenthesis would get done as a size_t, and then g->num_commits
would be widened to a size_t for the purposes of evaluating this
expression.
So I think that this is all OK in the sense that we'd never underflow
the 64-bit space, and having more than 2^64-1/36 (some eighteen
quintillion) commits is... unlikely ;-).
But it may be worth wrapping this computation in an st_mult() anyway to
avoid future readers having to think about this.
> + return error("commit-graph commit data chunk is wrong size");
> + g->chunk_commit_data = chunk_start;
> + return 0;
> +}
> +
> static int graph_read_bloom_data(const unsigned char *chunk_start,
> size_t chunk_size, void *data)
> {
> @@ -422,7 +432,7 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
>
> read_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, graph_read_oid_fanout, graph);
> read_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph);
> - pair_chunk_unsafe(cf, GRAPH_CHUNKID_DATA, &graph->chunk_commit_data);
> + read_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph);
Here again would be a good use-case for a `pair_chunk_expect()`
function, but I don't want to beat a dead horse ;-).
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 13/20] commit-graph: detect out-of-bounds extra-edges pointers
From: Taylor Blau @ 2023-10-11 19:02 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231009210538.GM3282181@coredump.intra.peff.net>
On Mon, Oct 09, 2023 at 05:05:38PM -0400, Jeff King wrote:
> ---
> commit-graph.c | 20 ++++++++++++++------
> commit-graph.h | 1 +
> t/t5318-commit-graph.sh | 8 ++++++++
> 3 files changed, 23 insertions(+), 6 deletions(-)
All looks good here, and the proposed log message is very clear and
well-written. One minor note below...
> @@ -931,14 +932,21 @@ static int fill_commit_in_graph(struct repository *r,
> return 1;
> }
>
> - parent_data_ptr = (uint32_t*)(g->chunk_extra_edges +
> - st_mult(4, edge_value & GRAPH_EDGE_LAST_MASK));
> + parent_data_pos = edge_value & GRAPH_EDGE_LAST_MASK;
> do {
> - edge_value = get_be32(parent_data_ptr);
> + if (g->chunk_extra_edges_size / sizeof(uint32_t) <= parent_data_pos) {
> + error("commit-graph extra-edges pointer out of bounds");
It might be nice to add some extra data here, too, like which commit OID
we were trying to load, the offset we were supposed to read at, and the
size of the extra edges chunk itself.
We should probably also mark this string for translation, but both are
minor points.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 14/20] commit-graph: bounds-check base graphs chunk
From: Taylor Blau @ 2023-10-11 19:05 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231009210541.GN3282181@coredump.intra.peff.net>
On Mon, Oct 09, 2023 at 05:05:41PM -0400, Jeff King wrote:
> When we are loading a commit-graph chain, we check that each slice of the
> chain points to the appropriate set of base graphs via its BASE chunk.
> But since we don't record the size of the chunk, we may access
> out-of-bounds memory if the file is corrupted.
>
> Since we know the number of entries we expect to find (based on the
> position within the commit-graph-chain file), we can just check the size
> up front.
>
> In theory this would also let us drop the st_mult() call a few lines
> later when we actually access the memory, since we know that the
> computed offset will fit in a size_t. But because the operands
> "g->hash_len" and "n" have types "unsigned char" and "int", we'd have to
> cast to size_t first. Leaving the st_mult() does that cast, and makes it
> more obvious that we don't have an overflow problem.
>
> Note that the test does not actually segfault before this patch, since
> it just reads garbage from the chunk after BASE (and indeed, it even
> rejects the file because that garbage does not have the expected hash
> value). You could construct a file with BASE at the end that did
> segfault, but corrupting the existing one is easy, and we can check
> stderr for the expected message.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> commit-graph.c | 8 +++++++-
> commit-graph.h | 1 +
> t/t5324-split-commit-graph.sh | 14 ++++++++++++++
> 3 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index e4860841fc..4377b547c8 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -435,7 +435,8 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
> read_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph);
> pair_chunk(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges,
> &graph->chunk_extra_edges_size);
> - pair_chunk_unsafe(cf, GRAPH_CHUNKID_BASE, &graph->chunk_base_graphs);
> + pair_chunk(cf, GRAPH_CHUNKID_BASE, &graph->chunk_base_graphs,
> + &graph->chunk_base_graphs_size);
>
> if (s->commit_graph_generation_version >= 2) {
> pair_chunk_unsafe(cf, GRAPH_CHUNKID_GENERATION_DATA,
> @@ -546,6 +547,11 @@ static int add_graph_to_chain(struct commit_graph *g,
> return 0;
> }
>
> + if (g->chunk_base_graphs_size / g->hash_len < n) {
> + warning(_("commit-graph base graphs chunk is too small"));
> + return 0;
> + }
> +
Nice. Here's a spot where we would not benefit from a function like
`pair_chunk_expect()`, since we don't know about the chain when we are
parsing an individual layer of it. So storing the length off to the side
and checking it within `add_graph_to_chain()` makes sense.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 1/1] branch.c: ammend error messages for die()
From: Junio C Hamano @ 2023-10-11 19:10 UTC (permalink / raw)
To: Rubén Justo; +Cc: Isoken June Ibizugbe, git
In-Reply-To: <06bc7b39-ed70-460f-b6f1-47a0c94c0538@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
>> @@ -640,16 +640,16 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>> if (!copy && (oldref_usage & IS_HEAD) &&
>> replace_each_worktree_head_symref(worktrees, oldref.buf, newref.buf,
>> logmsg.buf))
>> - die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
>> + die(_("branch renamed to %s, but HEAD is not updated!"), newname);
>
> IMO we can go further here and also remove that final "!". But it's OK
> the way you have done it.
Thanks for good eyes. I do not think '!' is adding any value in
this case, so removing it is probably a good idea, even if it is
done outside the scope of this patch.
>> else if ((argc == 1) && filter.detached)
>> - die(copy? _("cannot copy the current branch while not on any.")
>> + die(copy? _("cannot copy the current branch while not on any")
>> : _("cannot rename the current branch while not on any."));
>
> OK. But I think you want to modify also the second message, to remove
> its full stop as well.
Nice spotting.
>> @@ -965,11 +965,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>> const char *start_name = argc == 2 ? argv[1] : head;
>>
>> if (filter.kind != FILTER_REFS_BRANCHES)
>> - die(_("The -a, and -r, options to 'git branch' do not take a branch name.\n"
>> + die(_("the -a, and -r, options to 'git branch' do not take a branch name\n"
>> "Did you mean to use: -a|-r --list <pattern>?"));
>
> Good; the full stop removed and here that question mark is valuable. And ...
This one is not a single sentence, so retaining the full stop at the
end of the first sentence would actually be good, in addition to the
capitalization of the second sentence.
In a modern world, I would suspect that we would use die_message()
interface to emit the first sentence, show the "Did you mean..."
with advice_if_enabled(), and then exit with the status returned by
die_message(), similar to how branch.c::setup_tracking() does. When
it happens, the die() message will be only the first sentence, and
what this patch did can be retained. The second message will have
to be reworked to make it more appropriate as an advice message.
Thanks for a prompt review.
^ permalink raw reply
* Re: [PATCH 17/20] commit-graph: check bounds when accessing BDAT chunk
From: Taylor Blau @ 2023-10-11 19:11 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231009210550.GQ3282181@coredump.intra.peff.net>
On Mon, Oct 09, 2023 at 05:05:50PM -0400, Jeff King wrote:
> ---
> bloom.c | 24 ++++++++++++++++++++++++
> commit-graph.c | 10 ++++++++++
> commit-graph.h | 1 +
> t/t4216-log-bloom.sh | 28 ++++++++++++++++++++++++++++
> 4 files changed, 63 insertions(+)
>
> diff --git a/bloom.c b/bloom.c
> index aef6b5fea2..61abad7f8c 100644
> --- a/bloom.c
> +++ b/bloom.c
> @@ -29,6 +29,26 @@ static inline unsigned char get_bitmask(uint32_t pos)
> return ((unsigned char)1) << (pos & (BITS_PER_WORD - 1));
> }
>
> +static int check_bloom_offset(struct commit_graph *g, uint32_t pos,
> + uint32_t offset)
> +{
> + /*
> + * Note that we allow offsets equal to the data size, which would set
> + * our pointers at one past the end of the chunk memory. This is
> + * necessary because the on-disk index points to the end of the
> + * entries (so we can compute size by comparing adjacent ones). And
> + * naturally the final entry's end is one-past-the-end of the chunk.
> + */
> + if (offset <= g->chunk_bloom_data_size - BLOOMDATA_CHUNK_HEADER_SIZE)
> + return 0;
> +
> + warning("ignoring out-of-range offset (%"PRIuMAX") for changed-path"
> + " filter at pos %"PRIuMAX" of %s (chunk size: %"PRIuMAX")",
> + (uintmax_t)offset, (uintmax_t)pos,
> + g->filename, (uintmax_t)g->chunk_bloom_data_size);
Should this be marked for translation?
> + return -1;
> +}
> +
> static int load_bloom_filter_from_graph(struct commit_graph *g,
> struct bloom_filter *filter,
> uint32_t graph_pos)
> @@ -51,6 +71,10 @@ static int load_bloom_filter_from_graph(struct commit_graph *g,
> else
> start_index = 0;
>
> + if (check_bloom_offset(g, lex_pos, end_index) < 0 ||
> + check_bloom_offset(g, lex_pos - 1, start_index) < 0)
Can lex_pos ever be zero? I can't think of any reason that it couldn't,
and indeed the (elided) diff context shows that immediately preceding
this if-statement is another that checks "if (lex_pos > 0)".
So I think we'd want to avoid checking lex_pos - 1 if we know that
lex_pos is zero. Not that any of this really matters, since the only
thing we use the computed value for is showing the graph position in the
warning() message. So seeing a bogus value there isn't the end of the
world.
But avoiding this check when lex_pos == 0 is straightforward, so is
probably worth doing.
(As an aside, we should be able to simplify that if statement to just
"(if lex_pos)", since lex_pos will never be negative).
> + return 0;
> +
> filter->len = end_index - start_index;
> filter->data = (unsigned char *)(g->chunk_bloom_data +
> sizeof(unsigned char) * start_index +
> diff --git a/commit-graph.c b/commit-graph.c
> index f446e76c28..f7a42be6d0 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -365,7 +365,17 @@ static int graph_read_bloom_data(const unsigned char *chunk_start,
> {
> struct commit_graph *g = data;
> uint32_t hash_version;
> +
> + if (chunk_size < BLOOMDATA_CHUNK_HEADER_SIZE) {
> + warning("ignoring too-small changed-path chunk"
> + " (%"PRIuMAX" < %"PRIuMAX") in commit-graph file",
> + (uintmax_t)chunk_size,
> + (uintmax_t)BLOOMDATA_CHUNK_HEADER_SIZE);
Ditto on the translation suggestion from above.
Thanks,
Taylor
^ permalink raw reply
* Bug: `git stash push ':(attr:<somevalue>)' attr magic does not get parsed correctly
From: Joanna Wang @ 2023-10-11 19:14 UTC (permalink / raw)
To: git
What did you do before the bug happened? (Steps to reproduce your issue)
run the following with a debugger: `git stash push ':(attr:<somevalue>)'`
check the resulting pathspec after parse_pathspec()
What did you expect to happen? (Expected behavior)
the resulting pathspec should contain ':(attr:<somevalue>)'.
What happened instead? (Actual behavior)
The resulting pathspec is ':(attr,prefix:0)'
This is due to parse_pathspec in git-stash being called with
PATHSPEC_PREFIX_ORIGIN as a flag.
':(attr:<somevalue>)' gets parsed into ':(attr,prefix:0)' because prefix_magic()
https://github.com/git/git/blob/master/pathspec.c#L112 does not take
into account
that some magic can have values in addition to their magic names
specified in the pathspec.
What's different between what you expected and what actually happened?
Resulting pathspec is ':(attr,prefix:0)' when it should be
':(attr:<somevalue>,prefix:0)'
Anything else you want to add:
attr pathspec magic is currently blocked for git stash by GUARD_PATHSPEC
https://github.com/git/git/blob/master/dir.c#L2176
But, it looks like it can be unblocked, once prefix_magic() is fixed.
Also add-interactive also uses PATHSPEC_PREFIX_ORIGIN as a flag and
the resulting pathspec from parse_pathspec
shows the same bug. But `git add -i ':(attr:<somevalue>'` behaves as
expected and I'm not sure why. It may be using different parts of the
pathspec struct down the line to filter out files.
[Enabled Hooks]
commit-msg
pre-commit
prepare-commit-msg
^ 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