* Re: [RFC] Define "precious" attribute and support it in `git clean`
From: Kristoffer Haugsbakk @ 2023-10-11 21:41 UTC (permalink / raw)
To: Sebastian Thiel; +Cc: Josh Triplett, git
In-Reply-To: <79901E6C-9839-4AB2-9360-9EBCA1AAE549@icloud.com>
On Tue, Oct 10, 2023, at 14:37, Sebastian Thiel wrote:
> [Note: I'm collaborating with Josh Triplett (CCed) on the design.]
>
> I'd like to propose adding a new standard gitattribute "precious". I've
> included proposed documentation at the end of this mail, and I'm happy to write
> the code. I wanted to get feedback on the concept first.
>
> What's a 'precious' file?
>
> "Precious" files are files that are specific to a user or local configuration
> and thus not tracked by Git. As such, a user spent time to create or generate
> them, and to tune them to fit their needs. They are typically also ignored by
> `git` due to `.gitignore` configuration, preventing them to be tracked by
> accident.
How do people deal with these precious files today?
You could track these precious files somewhere else. Maybe a Git directory
which is a sibling of `.git`.
.git-local
Files that are useless to the project but important to the individual.
It would get its own “excludes” file.
.gitignore-local
Now the normal repository (`.git`) needs to ignore these things.
# Cast a wide net: could want other siblings
# Use another pattern if you have something like `.git-blame-ignore-revs`
printf '.git-*\n' >> .git/info/exclude
printf '.gitignore-*\n' >> .git/info/exclude
You need to pass in two arguments to `git` every time you want to use
`.git-local`.
alias gitl='git --git-dir=.git-local -c core.excludesFile=.gitignore-local'
Git Local should ignore everything by default. You should check with
`.gitignore` to make sure that it ignores the files that Git Local does
_not_ ignore.
printf '*\n' >> .gitignore-local
printf '!.gitignore-local\n' >> .gitignore-local
printf '!.idea/**\n' >> .gitignore-local
Now you can backup your local files.
gitl add .gitignore-local
gitl add .idea
gitl commit -m'Update local'
But you can also version control them by providing real (intentional)
messages.
(Maybe `.idea/` is just an XML soup and thus hard to make a VCS narrative
around; I don't know yet.)
`git clean` won't help you. But an alias can.
Or if writing a shell-oneliner alias is too hard for you, I mean me.
#!/bin/sh
# git-klean
git --git-dir=.git-local -c core.excludesFile=.gitignore-local add --all
git --git-dir=.git-local -c core.excludesFile=.gitignore-local commit -mUpdate
git clean -e .gitignore-local -e .git-local "$@"
The two `-e` switches protect the Git Local things from being wiped by
`-xd` (tested with `--dry-run`).
§ Sibling repositories
At first I thought that `git clean` could use a `pre-clean` hook. But
that's not very satisfying.
Maybe Git could be told about its siblings via a multi-valued
configuration variable.
sibling=local
Then it expects there to exist `.git-<sibling name>` repository next to
it (`.git/../.git-<sibling name>`).
Then the rule becomes:
Only do destructive operations if all of the working trees[1] of the
sibling repositories are clean (cannot override with `--force`).
Additionally one could say that directories that are Git repositories
should be ignored by `git clean`, always.[2] Or siblings which
match the glob:
.git-*
... Or if you want something longer:
.git-sibling-*
(And also ignore `.gitignore-*` and maybe more things)
Then the regular Git repository might still blow away your precious
files. But they will be backed up by the siblings.
Or you put these other Git repositories outside of `.git/..`. Sidestepping
the issue at the cost of some path confusion (for yourself). Maybe at:
/home/user/git-siblings/repository1/local
† 1: Worktrees not considered.
† 2: And what would that break? People who make Git repositories in their
working trees and then delete them? (Well they can still use `rm -r`.)
⧫ ⧫
§ Worktrees
But seriously: worktrees probably makes this not work.
--
Kristoffer
^ permalink raw reply
* Re: [PATCH] upload-pack: add tracing for fetches
From: Taylor Blau @ 2023-10-11 21:32 UTC (permalink / raw)
To: Robert Coup via GitGitGadget; +Cc: git, Robert Coup
In-Reply-To: <pull.1598.git.1697040242703.gitgitgadget@gmail.com>
On Wed, Oct 11, 2023 at 04:04:02PM +0000, Robert Coup via GitGitGadget wrote:
> 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.
Indeed. One of the custom patches that GitHub is carrying in its private
fork is something similar to this that dumps information from
upload-pack into a custom logging system specific to GitHub (called
"sockstat", in case anybody was curious).
I suspect that we would still live with that patch because we depend on
some of the custom logging infrastructure provided by sockstat, but this
is definitely a good direction to be pursuing for git.git nonetheless.
> @@ -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);
> + {
Is there a reason that we have a separate scope here? I think we may
want to drop this as unnecessary, but it's entirely possible that I'm
missing something here...
> + 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));
I'm pretty sure that list_object_filter_config_name() returns characters
that are safe for JSON-encoding, and/or that jw_object_string() does any
quoting beforehand, but worth checking nonetheless.
> + else
> + jw_object_null(&jw, "filter");
These all seem like useful things to have.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error
From: Junio C Hamano @ 2023-10-11 21:27 UTC (permalink / raw)
To: Michael Strawbridge
Cc: Bagas Sanjaya, Jeff King, Todd Zullinger, Luben Tuikov,
Ævar Arnfjörð Bjarmason, Taylor Blau,
Git Mailing List
In-Reply-To: <7e2c92ff-b42c-4b3f-a509-9d0785448262@amd.com>
Michael Strawbridge <michael.strawbridge@amd.com> writes:
> @@ -799,6 +799,10 @@ sub is_format_patch_arg {
>
> $time = time - scalar $#files;
>
> +@initial_to = process_address_list(@initial_to);
> +@initial_cc = process_address_list(@initial_cc);
> +@initial_bcc = process_address_list(@initial_bcc);
> +
This does not look OK. If we trace how @initial_to gets its value,
- it first gets its value from @getopt_to and @config_to
- if that is empty, and there is no $to_cmd, the end-user is
interactively asked.
- then process_address_list() is applied.
But this patch just swapped the second one and the third one, so
process_address_list() does not process what the end-user gave
interactively, no?
> if ($validate) {
> # FIFOs can only be read once, exclude them from validation.
> my @real_files = ();
It almost feels like what need to move is not the setting of these
address lists, but the code that calls int validation callchain that
needs access to these address lists---the block that begins with the
above "if ($validate) {" needs to move below ...
> @@ -1099,10 +1103,6 @@ sub expand_one_alias {
> return $aliases{$alias} ? expand_aliases(@{$aliases{$alias}}) : $alias;
> }
>
> -@initial_to = process_address_list(@initial_to);
> -@initial_cc = process_address_list(@initial_cc);
> -@initial_bcc = process_address_list(@initial_bcc);
> -
... this point, or something, perhaps?
> if ($thread && !defined $initial_in_reply_to && $prompting) {
> $initial_in_reply_to = ask(
> __("Message-ID to be used as In-Reply-To for the first email (if any)? "),
^ permalink raw reply
* Re: [PATCH] upload-pack: add tracing for fetches
From: Taylor Blau @ 2023-10-11 21:26 UTC (permalink / raw)
To: Robert Coup via GitGitGadget; +Cc: git, Robert Coup
In-Reply-To: <pull.1598.git.1697040242703.gitgitgadget@gmail.com>
On Wed, Oct 11, 2023 at 04:04:02PM +0000, Robert Coup via GitGitGadget wrote:
> 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
Not at all related to your patch here, but I feel like we have a bunch
of these greps sprinkled throughout the test suite which serve to
inspect some key(s) of a JSON object printed to the trace2 event stream.
It might be nice to have a more robust test-tool that could do this
heavy lifting for us instead of having to write these grep expressions
ourselves.
But definitely outside the scope of this patch ;-).
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v8 1/3] unit tests: Add a project plan document
From: Josh Steadmon @ 2023-10-11 21:14 UTC (permalink / raw)
To: Oswald Buddenhagen
Cc: git, phillip.wood123, linusa, calvinwan, gitster, rsbecker
In-Reply-To: <ZSUR+YdzqNTSB0XC@ugly>
On 2023.10.10 10:57, Oswald Buddenhagen wrote:
> On Mon, Oct 09, 2023 at 03:21:20PM -0700, Josh Steadmon wrote:
> > +=== Comparison
> > +
> > +[format="csv",options="header",width="33%"]
> > +|=====
> > +Framework,"<<license,License>>","<<vendorable-or-ubiquitous,Vendorable or ubiquitous>>","<<maintainable-extensible,Maintainable / extensible>>","<<major-platform-support,Major platform support>>","<<tap-support,TAP support>>","<<diagnostic-output,Diagnostic output>>","<<runtime--skippable-tests,Runtime- skippable tests>>","<<parallel-execution,Parallel execution>>","<<mock-support,Mock support>>","<<signal-error-handling,Signal & error handling>>","<<project-kloc,Project KLOC>>","<<adoption,Adoption>>"
> >
> the redundancy seems unnecessary; asciidoc should automatically use each
> target's section title as the xreflabel.
Hmm, this doesn't seem to work for me. It only renders as
"[anchor-label]".
> > +https://lore.kernel.org/git/c902a166-98ce-afba-93f2-ea6027557176@gmail.com/[Custom Git impl.],[lime-background]#GPL v2#,[lime-background]#True#,[lime-background]#True#,[lime-background]#True#,[lime-background]#True#,[lime-background]#True#,[lime-background]#True#,[red-background]#False#,[red-background]#False#,[red-background]#False#,1,0
> > +https://github.com/silentbicycle/greatest[Greatest],[lime-background]#ISC#,[lime-background]#True#,[yellow-background]#Partial#,[lime-background]#True#,[yellow-background]#Partial#,[lime-background]#True#,[lime-background]#True#,[red-background]#False#,[red-background]#False#,[red-background]#False#,3,1400
> > +https://github.com/Snaipe/Criterion[Criterion],[lime-background]#MIT#,[red-background]#False#,[yellow-background]#Partial#,[lime-background]#True#,[lime-background]#True#,[lime-background]#True#,[lime-background]#True#,[lime-background]#True#,[red-background]#False#,[lime-background]#True#,19,1800
> > +https://github.com/rra/c-tap-harness/[C TAP],[lime-background]#Expat#,[lime-background]#True#,[yellow-background]#Partial#,[yellow-background]#Partial#,[lime-background]#True#,[red-background]#False#,[lime-background]#True#,[red-background]#False#,[red-background]#False#,[red-background]#False#,4,33
> > +https://libcheck.github.io/check/[Check],[lime-background]#LGPL v2.1#,[red-background]#False#,[yellow-background]#Partial#,[lime-background]#True#,[lime-background]#True#,[lime-background]#True#,[red-background]#False#,[red-background]#False#,[red-background]#False#,[lime-background]#True#,17,973
> > +|=====
> > +
> i find this totally unreadable in its raw form.
> consider user-defined document-attributes for specific cell contents.
> externalizing the urls would probably help as well (i'm not sure how to do
> that best).
Ah yeah, user-defined attributes definitely cleans this up quite a bit.
Thanks for the tip!
^ permalink raw reply
* [PATCH] stash: be careful what we store
From: Junio C Hamano @ 2023-10-11 20:44 UTC (permalink / raw)
To: Git Mailing List; +Cc: Erik Cervin Edin
In-Reply-To: <xmqqzg0pnmv5.fsf@gitster.g>
"git stash store" is meant to store what "git stash create"
produces, as these two are implementation details of the end-user
facing "git stash save" command. Even though it is clearly
documented as such, users would try silly things like "git stash
store HEAD" to render their stash unusable.
Worse yet, because "git stash drop" does not allow such a stash
entry to be removed, "git stash clear" would be the only way to
recover from such a mishap. Reuse the logic that allows "drop" to
refrain from working on such a stash entry to teach "store" to avoid
storing an object that is not a stash entry in the first place.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
> 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.
Now it is at least compile-tested.
builtin/stash.c | 6 ++++++
t/t3903-stash.sh | 4 ++++
2 files changed, 10 insertions(+)
diff --git a/builtin/stash.c b/builtin/stash.c
index 3a4f9fd566..8073ef4019 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -977,6 +977,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 a/t/t3903-stash.sh b/t/t3903-stash.sh
index 376cc8f4ab..35c8569aea 100755
--- a/t/t3903-stash.sh
+++ b/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 &&
--
2.42.0-345-gaab89be2eb
^ permalink raw reply related
* Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error
From: Michael Strawbridge @ 2023-10-11 20:25 UTC (permalink / raw)
To: Bagas Sanjaya, Jeff King, Todd Zullinger
Cc: Junio C Hamano, Luben Tuikov,
Ævar Arnfjörð Bjarmason, Taylor Blau,
Git Mailing List
In-Reply-To: <7e2c92ff-b42c-4b3f-a509-9d0785448262@amd.com>
Hi Bagas,
If possible, please try the patch I just sent out and let me know if it works for your situation.
Thanks,
Michael
On 10/11/23 16:22, Michael Strawbridge wrote:
> Move processing of email address lists before the sendemail-validate
> hook code. This fixes email address validation errors when the optional
> perl module Email::Valid is installed and multiple addresses are passed
> in on a single to/cc argument like --to=foo@example.com,bar@example.com.
> ---
> git-send-email.perl | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 288ea1ae80..cfd80c9d8b 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -799,6 +799,10 @@ sub is_format_patch_arg {
>
> $time = time - scalar $#files;
>
> +@initial_to = process_address_list(@initial_to);
> +@initial_cc = process_address_list(@initial_cc);
> +@initial_bcc = process_address_list(@initial_bcc);
> +
> if ($validate) {
> # FIFOs can only be read once, exclude them from validation.
> my @real_files = ();
> @@ -1099,10 +1103,6 @@ sub expand_one_alias {
> return $aliases{$alias} ? expand_aliases(@{$aliases{$alias}}) : $alias;
> }
>
> -@initial_to = process_address_list(@initial_to);
> -@initial_cc = process_address_list(@initial_cc);
> -@initial_bcc = process_address_list(@initial_bcc);
> -
> if ($thread && !defined $initial_in_reply_to && $prompting) {
> $initial_in_reply_to = ask(
> __("Message-ID to be used as In-Reply-To for the first email (if any)? "),
^ permalink raw reply
* [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error
From: Michael Strawbridge @ 2023-10-11 20:22 UTC (permalink / raw)
To: Bagas Sanjaya, Jeff King, Todd Zullinger
Cc: Junio C Hamano, Luben Tuikov,
Ævar Arnfjörð Bjarmason, Taylor Blau,
Git Mailing List
In-Reply-To: <95b9e5d5-ab07-48a6-b972-af5348f653be@amd.com>
Move processing of email address lists before the sendemail-validate
hook code. This fixes email address validation errors when the optional
perl module Email::Valid is installed and multiple addresses are passed
in on a single to/cc argument like --to=foo@example.com,bar@example.com.
---
git-send-email.perl | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 288ea1ae80..cfd80c9d8b 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -799,6 +799,10 @@ sub is_format_patch_arg {
$time = time - scalar $#files;
+@initial_to = process_address_list(@initial_to);
+@initial_cc = process_address_list(@initial_cc);
+@initial_bcc = process_address_list(@initial_bcc);
+
if ($validate) {
# FIFOs can only be read once, exclude them from validation.
my @real_files = ();
@@ -1099,10 +1103,6 @@ sub expand_one_alias {
return $aliases{$alias} ? expand_aliases(@{$aliases{$alias}}) : $alias;
}
-@initial_to = process_address_list(@initial_to);
-@initial_cc = process_address_list(@initial_cc);
-@initial_bcc = process_address_list(@initial_bcc);
-
if ($thread && !defined $initial_in_reply_to && $prompting) {
$initial_in_reply_to = ask(
__("Message-ID to be used as In-Reply-To for the first email (if any)? "),
--
2.34.1
^ permalink raw reply related
* [PATCH 1/1] add: enable attr pathspec magic
From: Joanna Wang @ 2023-10-11 20:20 UTC (permalink / raw)
To: gitster; +Cc: git, jojwang
In-Reply-To: <xmqqbkda4ubp.fsf@gitster.g>
This lets users limit added files or exclude files based on file
attributes. For example, the chromium project would like to use
this like "git add --all ':(exclude,attr:submodule)'", as submodules
are managed in a unique way and often results in submodule changes
that users do not want in their commits.
This does not change any attr magic implementation. It is only adding
attr as an allowed pathspec in git-add, which was previously
blocked by GUARD_PATHSPEC and a pathspec mask in parse_pathspec()).
With this patch, attr is supported. It is possible that when the attr
pathspec feature was first added in
b0db704652 (pathspec: allow querying for attributes, 2017-03-13),
"PATHSPEC_ATTR" was just unintentionally left out of a few
GUARD_PATHSPEC() invocations. Later, to get a more user-friendly error
message when attr was used with git-add, PATHSPEC_ATTR was added as a
mask to git-add's invocation of parse_pathspec()
84d938b732 (add: do not accept pathspec magic 'attr', 2018-09-18).
git-stash which goes through the same GUARD_PATHSPEC(), currently
does not work with attr.
https://lore.kernel.org/git/CAMmZTi-0QKtj7Q=sbC5qhipGsQxJFOY-Qkk1jfkRYwfF5FcUVg@mail.gmail.com/T/#u
So a PATHSPEC_ATTR mask has been added to its parse_pathspec and
parse_pathspec_file().
Signed-off-by: Joanna Wang <jojwang@google.com>
---
> Do you know what exactly is not ready, so that perhaps others can
> help figuring out how to make it ready for the attr magic?
I have filed a bug which describes what I have found:
https://lore.kernel.org/git/CAMmZTi-0QKtj7Q=sbC5qhipGsQxJFOY-Qkk1jfkRYwfF5FcUVg@mail.gmail.com/T/#u
> Why this reindent?
Fixed
> There is a typo here.
Fixed
builtin/add.c | 7 ++-
builtin/stash.c | 7 ++-
dir.c | 3 +-
t/t6135-pathspec-with-attrs.sh | 106 +++++++++++++++++++++++++++++++--
4 files changed, 112 insertions(+), 11 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index c27254a5cd..2de83964a3 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -424,7 +424,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
* Check the "pathspec '%s' did not match any files" block
* below before enabling new magic.
*/
- parse_pathspec(&pathspec, PATHSPEC_ATTR,
+ parse_pathspec(&pathspec, 0,
PATHSPEC_PREFER_FULL |
PATHSPEC_SYMLINK_LEADING_PATH,
prefix, argv);
@@ -433,7 +433,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
if (pathspec.nr)
die(_("'%s' and pathspec arguments cannot be used together"), "--pathspec-from-file");
- parse_pathspec_file(&pathspec, PATHSPEC_ATTR,
+ parse_pathspec_file(&pathspec, 0,
PATHSPEC_PREFER_FULL |
PATHSPEC_SYMLINK_LEADING_PATH,
prefix, pathspec_from_file, pathspec_file_nul);
@@ -504,7 +504,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
PATHSPEC_LITERAL |
PATHSPEC_GLOB |
PATHSPEC_ICASE |
- PATHSPEC_EXCLUDE);
+ PATHSPEC_EXCLUDE |
+ PATHSPEC_ATTR);
for (i = 0; i < pathspec.nr; i++) {
const char *path = pathspec.items[i].match;
diff --git a/builtin/stash.c b/builtin/stash.c
index 1ad496985a..af1b3a7146 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1760,7 +1760,10 @@ static int push_stash(int argc, const char **argv, const char *prefix,
}
}
- parse_pathspec(&ps, 0, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,
+ // For parse_pathspec() and parse_pathspec_file() below, PATHSPEC_ATTR is blocked for git stash
+ // because the magic attr does not get properly parsed when the PATHSPEC_PREFIX_ORIGIN flag is
+ // used, resulting in incorrect file filtering for attr.
+ parse_pathspec(&ps, PATHSPEC_ATTR, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,
prefix, argv);
if (pathspec_from_file) {
@@ -1773,7 +1776,7 @@ static int push_stash(int argc, const char **argv, const char *prefix,
if (ps.nr)
die(_("'%s' and pathspec arguments cannot be used together"), "--pathspec-from-file");
- parse_pathspec_file(&ps, 0,
+ parse_pathspec_file(&ps, PATHSPEC_ATTR,
PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,
prefix, pathspec_from_file, pathspec_file_nul);
} else if (pathspec_file_nul) {
diff --git a/dir.c b/dir.c
index 8486e4d56f..ee3f3777df 100644
--- a/dir.c
+++ b/dir.c
@@ -2179,7 +2179,8 @@ static int exclude_matches_pathspec(const char *path, int pathlen,
PATHSPEC_LITERAL |
PATHSPEC_GLOB |
PATHSPEC_ICASE |
- PATHSPEC_EXCLUDE);
+ PATHSPEC_EXCLUDE |
+ PATHSPEC_ATTR);
for (i = 0; i < pathspec->nr; i++) {
const struct pathspec_item *item = &pathspec->items[i];
diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
index f70c395e75..403ee5e6b6 100755
--- a/t/t6135-pathspec-with-attrs.sh
+++ b/t/t6135-pathspec-with-attrs.sh
@@ -64,12 +64,24 @@ test_expect_success 'setup .gitattributes' '
fileSetLabel label
fileValue label=foo
fileWrongLabel label☺
+ newFileA* labelA
+ newFileB* labelB
EOF
echo fileSetLabel label1 >sub/.gitattributes &&
git add .gitattributes sub/.gitattributes &&
git commit -m "add attributes"
'
+test_expect_success 'setup .gitignore' '
+ cat <<-\EOF >.gitignore &&
+ actual
+ expect
+ pathspec_file
+ EOF
+ git add .gitignore &&
+ git commit -m "add gitignore"
+'
+
test_expect_success 'check specific set attr' '
cat <<-\EOF >expect &&
fileSetLabel
@@ -150,6 +162,7 @@ test_expect_success 'check specific value attr (2)' '
test_expect_success 'check unspecified attr' '
cat <<-\EOF >expect &&
.gitattributes
+ .gitignore
fileA
fileAB
fileAC
@@ -175,6 +188,7 @@ test_expect_success 'check unspecified attr' '
test_expect_success 'check unspecified attr (2)' '
cat <<-\EOF >expect &&
HEAD:.gitattributes
+ HEAD:.gitignore
HEAD:fileA
HEAD:fileAB
HEAD:fileAC
@@ -200,6 +214,7 @@ test_expect_success 'check unspecified attr (2)' '
test_expect_success 'check multiple unspecified attr' '
cat <<-\EOF >expect &&
.gitattributes
+ .gitignore
fileC
fileNoLabel
fileWrongLabel
@@ -239,16 +254,97 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '
test_i18ngrep "Only one" actual
'
-test_expect_success 'fail if attr magic is used places not implemented' '
+test_expect_success 'fail if attr magic is used in places not implemented' '
# The main purpose of this test is to check that we actually fail
# when you attempt to use attr magic in commands that do not implement
- # attr magic. This test does not advocate git-add to stay that way,
- # though, but git-add is convenient as it has its own internal pathspec
- # parsing.
- test_must_fail git add ":(attr:labelB)" 2>actual &&
+ # attr magic. This test does not advocate stash push to stay that way.
+ # When you teach the command to grok the pathspec, you need to find
+ # another command to replace it for the test.
+ test_must_fail git stash push ":(attr:labelB)" 2>actual &&
+ test_i18ngrep "magic not supported" actual
+'
+
+test_expect_success 'fail if attr magic is used in --pathspec-from-file when not implemented' '
+ # This is like the test above but for attr magic passed in via --pathspec-from-file.
+ cat <<-\EOF >pathspec_file &&
+ :(attr:labelB)
+ EOF
+ test_must_fail git stash push --pathspec-from-file=pathspec_file 2>actual &&
test_i18ngrep "magic not supported" actual
'
+test_expect_success 'check that attr magic works for git add --all' '
+ cat <<-\EOF >expect &&
+ sub/newFileA-foo
+ EOF
+ >sub/newFileA-foo &&
+ >sub/newFileB-foo &&
+ git add --all ":(exclude,attr:labelB)" &&
+ git diff --name-only --cached >actual &&
+ git restore -W -S . &&
+ test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add -u' '
+ cat <<-\EOF >expect &&
+ sub/fileA
+ EOF
+ >sub/newFileA-foo &&
+ >sub/newFileB-foo &&
+ >sub/fileA &&
+ >sub/fileB &&
+ git add -u ":(exclude,attr:labelB)" &&
+ git diff --name-only --cached >actual &&
+ git restore -S -W . && rm sub/new* &&
+ test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add <path>' '
+ cat <<-\EOF >expect &&
+ fileA
+ fileB
+ sub/fileA
+ EOF
+ >fileA &&
+ >fileB &&
+ >sub/fileA &&
+ >sub/fileB &&
+ git add ":(exclude,attr:labelB)sub/*" &&
+ git diff --name-only --cached >actual &&
+ git restore -S -W . &&
+ test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git -add .' '
+ cat <<-\EOF >expect &&
+ sub/fileA
+ EOF
+ >fileA &&
+ >fileB &&
+ >sub/fileA &&
+ >sub/fileB &&
+ cd sub &&
+ git add . ":(exclude,attr:labelB)" &&
+ cd .. &&
+ git diff --name-only --cached >actual &&
+ git restore -S -W . &&
+ test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add --pathspec-from-file' '
+ cat <<-\EOF >pathspec_file &&
+ :(exclude,attr:labelB)
+ EOF
+ cat <<-\EOF >expect &&
+ sub/newFileA-foo
+ EOF
+ >sub/newFileA-foo &&
+ >sub/newFileB-foo &&
+ git add --all --pathspec-from-file=pathspec_file &&
+ git diff --name-only --cached >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'abort on giving invalid label on the command line' '
test_must_fail git ls-files . ":(attr:☺)"
'
--
2.42.0.609.gbb76f46606-goog
^ permalink raw reply related
* Re: Bug: git stash store can create stash entries that can't be dropped
From: Erik Cervin Edin @ 2023-10-11 19:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <xmqqzg0pnmv5.fsf@gitster.g>
On Wed, Oct 11, 2023 at 6:29 PM Junio C Hamano <gitster@pobox.com> wrote:
> 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.
Fair. I knew what I was doing was probably not kosher. Still, the
documentation says
git stash store [(-m | --message) <message>] [-q | --quiet] <commit>
and then later clarifying
Store a given stash created via git stash create (which is
a dangling merge commit) in the stash ref, updating the stash reflog.
This is intended to be useful for scripts.
It is probably not the command you want to use; see "push" above.
I knew git stash store is meant to be used with git stash create. But
I didn't expect to be able to create a stash entry that I wouldn't be
able to drop.
> 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.
I can see why I deserve the mess I made for myself. But I'm also
inclined to think that if I've been allowed to make said mess, I
should similarly be allowed to clean it up, without having to manually
delete the entry from the stash reflog. (In case someone else finds
themselves in a similar mess, I just went into the
.git/logs/refs/stash and deleted line of the offending stash entry)
I think a more user-friendly behavior is to either prevent the user
from making the mess, or not prevent the user from cleaning it up.
Doing both seems a tad overzealous. Hard for me to say which is
better. For example, it's not immediately clear, to me, why git stash
drop cares about the validity of stash entries to be dropped. But
maybe there are edge cases where it's good to be defensive here.
^ permalink raw reply
* Re: [PATCH v6] merge-tree: add -X strategy option
From: Junio C Hamano @ 2023-10-11 19:38 UTC (permalink / raw)
To: Jeff King; +Cc: phillip.wood, Izzy via GitGitGadget, git, Elijah Newren, Izzy
In-Reply-To: <20231009185234.GB3270793@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Oops, I totally missed the loop around the call to real_merge(). So
> yeah, I think this is rather tricky.
> ...
> I don't think there are any bugs with the state at the current tip of
> ty/merge-tree-strategy-options, but if we want to make it safer, I think
> we have two options:
>
> - delay the conversion of the "xopts" list into merge_options until we
> initialize it in real_merge(). This avoids breaking abstraction
> boundaries, but does mean that the sanity-checking of the options
> happens a little later (but not much in practice).
>
> - provide a copy_merge_options() function, which makes this kind of
> "set up a template and then copy it" pattern official. It can be a
> struct assignment for now, but it at least alerts anybody adding new
> options to the notion that a deep copy might be required.
>
> Option 1 looks something like this (a lot of the hunks are just
> reverting the tip of that branch, so squashed in it would be even
> smaller):
If we have no plan and intention to extend "merge-tree" even more in
the future, then option 1 would be the approach with least patch
noise, and as your "something like this" shows, it is a nice and
clean solution. I very much like it.
But as the renovated "merge-tree" is a relatively young thing in our
toolbox, I suspect that more and more work may want to go into it.
And the other "official copy_merge_options()" approach would be a
more healthy solution in the longer run, I would think. If we were
to go that route, we should also give an interface to free the
resources held by the copy.
It is not that much code on top of the commit that is already queued
in 'next', I suspect. Perhaps something like this?
builtin/merge-tree.c | 4 +++-
merge-recursive.c | 16 ++++++++++++++++
merge-recursive.h | 3 +++
3 files changed, 22 insertions(+), 1 deletion(-)
diff --git c/builtin/merge-tree.c w/builtin/merge-tree.c
index 7024b5ce2e..a35e0452d6 100644
--- c/builtin/merge-tree.c
+++ w/builtin/merge-tree.c
@@ -425,10 +425,11 @@ static int real_merge(struct merge_tree_options *o,
{
struct commit *parent1, *parent2;
struct commit_list *merge_bases = NULL;
- struct merge_options opt = o->merge_options;
struct merge_result result = { 0 };
int show_messages = o->show_messages;
+ struct merge_options opt;
+ copy_merge_options(&opt, &o->merge_options);
parent1 = get_merge_parent(branch1);
if (!parent1)
help_unknown_ref(branch1, "merge-tree",
@@ -507,6 +508,7 @@ static int real_merge(struct merge_tree_options *o,
if (o->use_stdin)
putchar(line_termination);
merge_finalize(&opt, &result);
+ clear_merge_options(&opt);
return !result.clean; /* result.clean < 0 handled above */
}
diff --git c/merge-recursive.c w/merge-recursive.c
index 0d7e57e2df..e3beb0801b 100644
--- c/merge-recursive.c
+++ w/merge-recursive.c
@@ -3912,6 +3912,22 @@ void init_merge_options(struct merge_options *opt,
opt->buffer_output = 0;
}
+/*
+ * For now, members of merge_options do not need deep copying, but
+ * it may change in the future, in which case we would need to update
+ * this, and also make a matching change to clear_merge_options() to
+ * release the resources held by a copied instance.
+ */
+void copy_merge_options(struct merge_options *dst, struct merge_options *src)
+{
+ *dst = *src;
+}
+
+void clear_merge_options(struct merge_options *opt UNUSED)
+{
+ ; /* no-op as our copy is shallow right now */
+}
+
int parse_merge_opt(struct merge_options *opt, const char *s)
{
const char *arg;
diff --git c/merge-recursive.h w/merge-recursive.h
index b88000e3c2..3d3b3e3c29 100644
--- c/merge-recursive.h
+++ w/merge-recursive.h
@@ -55,6 +55,9 @@ struct merge_options {
void init_merge_options(struct merge_options *opt, struct repository *repo);
+void copy_merge_options(struct merge_options *dst, struct merge_options *src);
+void clear_merge_options(struct merge_options *opt);
+
/* parse the option in s and update the relevant field of opt */
int parse_merge_opt(struct merge_options *opt, const char *s);
^ permalink raw reply related
* Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas
From: Michael Strawbridge @ 2023-10-11 19:27 UTC (permalink / raw)
To: Bagas Sanjaya, Jeff King, Todd Zullinger
Cc: Junio C Hamano, Luben Tuikov,
Ævar Arnfjörð Bjarmason, Taylor Blau,
Git Mailing List
In-Reply-To: <ZSal-mQIZAUBaq6g@debian.me>
On 10/11/23 09:41, Bagas Sanjaya wrote:
> On Mon, Sep 25, 2023 at 12:17:48PM -0400, Jeff King wrote:
>> On Mon, Sep 25, 2023 at 10:48:29AM -0400, Todd Zullinger wrote:
>>
>>> From the peanut gallery... could the presence or lack of the
>>> Email::Valid perl module be a factor?
>> Ah, thanks! The thought of differing modules even occurred to me, since
>> I know we have a few optimistic dependencies, but when I looked I didn't
>> manage to find that one (somehow I thought Mail::Address was the
>> interesting one here; I think I might be getting senile).
>>
>> With Email::Valid installed, I can reproduce with just (in git.git, but
>> I think it would work in any repo):
>>
>> $ echo "exit 0" >.git/hooks/sendemail-validate
>> $ chmod +x .git/hooks/sendemail-validate
>> $ git send-email --dry-run -1 --to=foo@example.com,bar@example.com
>> error: unable to extract a valid address from: foo@example.com,bar@example.com
>>
>> Disabling the hook with "chmod -x" makes the problem go away (and this
>> is with current "master", hence the more readable error message).
>>
>> I think the issue is that a8022c5f7b ends up in extract_valid_address()
>> via this call stack:
>>
>> $ = main::extract_valid_address('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1161
>> $ = main::extract_valid_address_or_die('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 2087
>> @ = main::unique_email_list('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1507
>> @ = main::gen_header() called from file '/home/peff/compile/git/git-send-email' line 2113
>> . = main::validate_patch('/tmp/WfoPQSKCUa/0001-The-twelfth-batch.patch', 'auto') called from file '/home/peff/compile/git/git-send-email' line 815
>>
>> whereas prior to that commit, we hit it later:
>>
>> $ = main::extract_valid_address('foo@example.com') called from file '/home/peff/compile/git/git-send-email' line 1166
>> @ = main::validate_address('foo@example.com') called from file '/home/peff/compile/git/git-send-email' line 1189
>> @ = main::validate_address_list('foo@example.com', 'bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1348
>> @ = main::process_address_list('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1091
>>
>> So the issue is the call to gen_header() added in validate_patch(). We
>> won't yet have processed the address lists by that point. We can move
>> those calls up, but it requires moving a bit of extra code, too (like
>> the parts prompting for the "to" list if it isn't filled in).
>>
>> Possibly the validation checks need to be moved down, if they want to
>> see a more complete view of the emails. But now we're doing more work
>> (like asking the user to write the cover letter!) before we do
>> validation, which is probably bad.
>>
>> So I dunno. Maybe gen_header() should be lazily doing this
>> process_address_list() stuff? I'm not very familiar with the send-email
>> code, so I'm not sure what secondary effects that could have.
>>
> Michael, did you look into this since you authored the culprit?
>
I tried to repro the issue previously but didn't have luck (even with Email::Valid installed). I decided to try again today and realised I forgot to make the test sendemail-validate hook executable before. Now that I can repro it, I can look further.
The fix may not be easy for the reasons that Jeff King states. There is a lot of implicit order in the code because there are several places where code exists outside of any function, which has function definitions scattered through it. My change attempted to clean a portion of that up and encapsulated it into a reusable function for generating header information so that sendemail-validate could see it. I'll keep digging into it.
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2023, #04; Tue, 10)
From: Taylor Blau @ 2023-10-11 19:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Tan, SZEDER Gábor, Elijah Newren, git
In-Reply-To: <xmqqwmvuosf3.fsf@gitster.g>
On Tue, Oct 10, 2023 at 06:32:16PM -0700, Junio C Hamano wrote:
> * tb/path-filter-fix (2023-08-30) 15 commits
> - bloom: introduce `deinit_bloom_filters()`
> - commit-graph: reuse existing Bloom filters where possible
> - object.h: fix mis-aligned flag bits table
> - commit-graph: drop unnecessary `graph_read_bloom_data_context`
> - commit-graph.c: unconditionally load Bloom filters
> - t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`
> - bloom: prepare to discard incompatible Bloom filters
> - bloom: annotate filters with hash version
> - commit-graph: new filter ver. that fixes murmur3
> - repo-settings: introduce commitgraph.changedPathsVersion
> - t4216: test changed path filters with high bit paths
> - t/helper/test-read-graph: implement `bloom-filters` mode
> - bloom.h: make `load_bloom_filter_from_graph()` public
> - t/helper/test-read-graph.c: extract `dump_graph_info()`
> - gitformat-commit-graph: describe version 2 of BDAT
>
> The Bloom filter used for path limited history traversal was broken
> on systems whose "char" is unsigned; update the implementation and
> bump the format version to 2.
>
> Reroll exists, not picked up yet.
> cf. <20230830200218.GA5147@szeder.dev>
> cf. <20230901205616.3572722-1-jonathantanmy@google.com>
> cf. <20230924195900.GA1156862@szeder.dev>
> cf. <20231008143523.GA18858@szeder.dev>
> source: <cover.1693413637.git.jonathantanmy@google.com>
Great, thanks for noting that you saw it ;-). I think that this one is
ready to go, but I'm obviously biased and I'd feel better if Jonathan or
Gábor (both CC'd) would take a look before you merge this down.
> * tb/repack-max-cruft-size (2023-10-09) 5 commits
> (merged to 'next' on 2023-10-09 at 38f039e880)
> + repack: free existing_cruft array after use
> (merged to 'next' on 2023-10-06 at b3ca6df3b9)
> + builtin/repack.c: avoid making cruft packs preferred
> + builtin/repack.c: implement support for `--max-cruft-size`
> + builtin/repack.c: parse `--max-pack-size` with OPT_MAGNITUDE
> + t7700: split cruft-related tests to t7704
>
> "git repack" learned "--max-cruft-size" to prevent cruft packs from
> growing without bounds.
>
> Will merge to 'master'.
> source: <cover.1696293862.git.me@ttaylorr.com>
> source: <20231007172031.GA1524950@coredump.intra.peff.net>
> source: <035393935108d02aaf8927189b05102f4f74f340.1696370003.git.me@ttaylorr.com>
Great, thanks.
The only series of mine I didn't see mentioned here was the merge ORT ->
bulk-checkin stuff that I posted in [1]. The discussion there has sort
of morphed beyond the patches themselves, so I'd love some review on
these patches (perhaps from Elijah?, also CC'd) if time permits.
Thanks,
Taylor
[1]: https://lore.kernel.org/git/cover.1696629697.git.me@ttaylorr.com/
^ permalink raw reply
* Re: [PATCH 0/20] bounds-checks for chunk-based files
From: Taylor Blau @ 2023-10-11 19:19 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231009205544.GA3281950@coredump.intra.peff.net>
On Mon, Oct 09, 2023 at 04:55:44PM -0400, Jeff King wrote:
> bloom.c | 34 +++++++++
> chunk-format.c | 24 ++++--
> chunk-format.h | 9 ++-
> commit-graph.c | 119 ++++++++++++++++++++++++-----
> commit-graph.h | 4 +
> midx.c | 68 +++++++++++++----
> midx.h | 3 +
> pack-revindex.c | 13 +++-
> t/lib-chunk.sh | 17 +++++
> t/lib-chunk/corrupt-chunk-file.pl | 66 ++++++++++++++++
> t/t4216-log-bloom.sh | 50 ++++++++++++
> t/t5318-commit-graph.sh | 76 +++++++++++++++++-
> t/t5319-multi-pack-index.sh | 102 ++++++++++++++++++++++++-
> t/t5324-split-commit-graph.sh | 20 ++++-
> t/t5328-commit-graph-64bit-time.sh | 10 +++
> 15 files changed, 568 insertions(+), 47 deletions(-)
> create mode 100644 t/lib-chunk.sh
> create mode 100644 t/lib-chunk/corrupt-chunk-file.pl
I reviewed this carefully (well, except for the new Perl script, for
obvious[^1] reasons ;-)).
Everything mostly looks good to me, though I
had a handful of review comments throughout. Many of them are trivial
(e.g. a number of warning() and error() strings should be marked for
translation, etc.), but a couple of them I think are worth looking at.
Most notably, I think that by the end of the series, I was convinced
that having some kind of 'pair_chunk_expectsz()' or similar would be
useful and eliminate a good chunk of the boilerplate you have to write
to check the chunk size against an expected value when using
read_chunk().
Otherwise, this looks great. I appreciate the care you took in finding
and fixing these issues, as well as thoroughly documenting the process
(and the security implications, or lack thereof). Thanks for working on
this!
Thanks,
Taylor
[^1]: That I may be the world's least competent Perl programmer.
^ permalink raw reply
* Re: [PATCH 19/20] commit-graph: detect out-of-order BIDX offsets
From: Taylor Blau @ 2023-10-11 19:16 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231009210556.GS3282181@coredump.intra.peff.net>
On Mon, Oct 09, 2023 at 05:05:56PM -0400, Jeff King wrote:
> diff --git a/bloom.c b/bloom.c
> index 61abad7f8c..1474aa19fa 100644
> --- a/bloom.c
> +++ b/bloom.c
> @@ -75,6 +75,16 @@ static int load_bloom_filter_from_graph(struct commit_graph *g,
> check_bloom_offset(g, lex_pos - 1, start_index) < 0)
> return 0;
>
> + if (end_index < start_index) {
> + warning("ignoring decreasing changed-path index offsets"
> + " (%"PRIuMAX" > %"PRIuMAX") for positions"
> + " %"PRIuMAX" and %"PRIuMAX" of %s",
Should this be marked for translation?
Otherwise this LGTM.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 18/20] commit-graph: check bounds when accessing BIDX chunk
From: Taylor Blau @ 2023-10-11 19:15 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231009210553.GR3282181@coredump.intra.peff.net>
On Mon, Oct 09, 2023 at 05:05:53PM -0400, Jeff King wrote:
> We load the bloom_filter_indexes chunk using pair_chunk(), so we have no
> idea how big it is. This can lead to out-of-bounds reads if it is
> smaller than expected, since we index it based on the number of commits
> found elsewhere in the graph file.
>
> We can check the chunk size up front, like we do for CDAT and other
> chunks with one fixed-size record per commit.
>
> The test case demonstrates the problem. It actually won't segfault,
> because we end up reading random data from the follow-on chunk (BDAT in
> this case), and the bounds checks added in the previous patch complain.
> But this is by no means assured, and you can craft a commit-graph file
> with BIDX at the end (or a smaller BDAT) that does segfault.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> commit-graph.c | 16 ++++++++++++++--
> t/t4216-log-bloom.sh | 9 +++++++++
> 2 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index f7a42be6d0..1f334987b5 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -360,6 +360,18 @@ static int graph_read_generation_data(const unsigned char *chunk_start,
> return 0;
> }
>
> +static int graph_read_bloom_index(const unsigned char *chunk_start,
> + size_t chunk_size, void *data)
> +{
> + struct commit_graph *g = data;
> + if (chunk_size != g->num_commits * 4) {
Here we probably should wrap `g->num_commits * 4` in an st_mult() call.
Having 4B commits is probably pretty unlikely in practice, but we have
definitely seen issues in the wild at GitHub where we have more than
2^32-1/20 commits in a single network.git.
> + warning("commit-graph changed-path index chunk is too small");
Should this be marked for translation?
> + return -1;
> + }
> + g->chunk_bloom_indexes = chunk_start;
> + return 0;
> +}
> +
> static int graph_read_bloom_data(const unsigned char *chunk_start,
> size_t chunk_size, void *data)
> {
> @@ -470,8 +482,8 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
> }
>
> if (s->commit_graph_read_changed_paths) {
> - pair_chunk_unsafe(cf, GRAPH_CHUNKID_BLOOMINDEXES,
> - &graph->chunk_bloom_indexes);
> + read_chunk(cf, GRAPH_CHUNKID_BLOOMINDEXES,
> + graph_read_bloom_index, graph);
Since we know g->num_commits by this point, this would be another spot
that would benefit from a `pair_chunk_expect()` function. But, again,
not trying to beat a dead horse here or anything ;-).
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
* 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
* 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 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 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 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 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 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 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
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