* [PATCH v3 1/2] index-pack: test and document --strict=<msg-id>=<severity>...
2024-01-26 20:59 ` [PATCH v3 0/2] index-pack: fsck honor checks John Cai via GitGitGadget
@ 2024-01-26 20:59 ` John Cai via GitGitGadget
2024-01-26 20:59 ` [PATCH v3 2/2] index-pack: --fsck-objects to take an optional argument for fsck msgs John Cai via GitGitGadget
` (2 subsequent siblings)
3 siblings, 0 replies; 29+ messages in thread
From: John Cai via GitGitGadget @ 2024-01-26 20:59 UTC (permalink / raw)
To: git; +Cc: John Cai, John Cai
From: John Cai <johncai86@gmail.com>
5d477a334a (fsck (receive-pack): allow demoting errors to warnings,
2015-06-22) allowed a list of fsck msg to downgrade to be passed to
--strict. However this is a hidden argument that was not documented nor
tested. Though it is true that most users would not call this option
directly, (nor use index-pack for that matter) it is still useful to
document and test this feature.
Reviewed-by: Christian Couder <christian.couder@gmail.com>
Signed-off-by: John Cai <johncai86@gmail.com>
---
Documentation/git-index-pack.txt | 9 +++++++--
builtin/index-pack.c | 2 +-
t/t5300-pack-object.sh | 22 ++++++++++++++++++++++
3 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt
index 6486620c3d8..694bb9409bf 100644
--- a/Documentation/git-index-pack.txt
+++ b/Documentation/git-index-pack.txt
@@ -79,8 +79,13 @@ OPTIONS
to force the version for the generated pack index, and to force
64-bit index entries on objects located above the given offset.
---strict::
- Die, if the pack contains broken objects or links.
+--strict[=<msg-id>=<severity>...]::
+ Die, if the pack contains broken objects or links. An optional
+ comma-separated list of `<msg-id>=<severity>` can be passed to change
+ the severity of some possible issues, e.g.,
+ `--strict="missingEmail=ignore,badTagName=error"`. See the entry for the
+ `fsck.<msg-id>` configuration options in linkgit:git-fsck[1] for more
+ information on the possible values of `<msg-id>` and `<severity>`.
--progress-title::
For internal use only.
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 1ea87e01f29..240c7021168 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -24,7 +24,7 @@
#include "setup.h"
static const char index_pack_usage[] =
-"git index-pack [-v] [-o <index-file>] [--keep | --keep=<msg>] [--[no-]rev-index] [--verify] [--strict] (<pack-file> | --stdin [--fix-thin] [<pack-file>])";
+"git index-pack [-v] [-o <index-file>] [--keep | --keep=<msg>] [--[no-]rev-index] [--verify] [--strict[=<msg-id>=<severity>...]] (<pack-file> | --stdin [--fix-thin] [<pack-file>])";
struct object_entry {
struct pack_idx_entry idx;
diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index d402ec18b79..496fffa0f8a 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -441,6 +441,28 @@ test_expect_success 'index-pack with --strict' '
)
'
+test_expect_success 'index-pack with --strict downgrading fsck msgs' '
+ test_when_finished rm -rf strict &&
+ git init strict &&
+ (
+ cd strict &&
+ test_commit first hello &&
+ cat >commit <<-EOF &&
+ tree $(git rev-parse HEAD^{tree})
+ parent $(git rev-parse HEAD)
+ author A U Thor
+ committer A U Thor
+
+ commit: this is a commit with bad emails
+
+ EOF
+ git hash-object --literally -t commit -w --stdin <commit >commit_list &&
+ PACK=$(git pack-objects test <commit_list) &&
+ test_must_fail git index-pack --strict "test-$PACK.pack" &&
+ git index-pack --strict="missingEmail=ignore" "test-$PACK.pack"
+ )
+'
+
test_expect_success 'honor pack.packSizeLimit' '
git config pack.packSizeLimit 3m &&
packname_10=$(git pack-objects test-10 <obj-list) &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v3 2/2] index-pack: --fsck-objects to take an optional argument for fsck msgs
2024-01-26 20:59 ` [PATCH v3 0/2] index-pack: fsck honor checks John Cai via GitGitGadget
2024-01-26 20:59 ` [PATCH v3 1/2] index-pack: test and document --strict=<msg-id>=<severity> John Cai via GitGitGadget
@ 2024-01-26 20:59 ` John Cai via GitGitGadget
2024-01-26 21:18 ` [PATCH v3 0/2] index-pack: fsck honor checks Junio C Hamano
2024-02-01 1:38 ` [PATCH v4 " John Cai via GitGitGadget
3 siblings, 0 replies; 29+ messages in thread
From: John Cai via GitGitGadget @ 2024-01-26 20:59 UTC (permalink / raw)
To: git; +Cc: John Cai, John Cai
From: John Cai <johncai86@gmail.com>
git-index-pack has a --strict option that can take an optional argument
to provide a list of fsck issues to change their severity.
--fsck-objects does not have such a utility, which would be useful if
one would like to be more lenient or strict on data integrity in a
repository.
Like --strict, allow --fsck-objects to also take a list of fsck msgs to
change the severity.
Remove the "For internal use only" note for --fsck-objects, and document
the option. This won't often be used by the normal end user, but it
turns out it is useful for Git forges like GitLab.
Reviewed-by: Christian Couder <christian.couder@gmail.com>
Signed-off-by: John Cai <johncai86@gmail.com>
---
Documentation/git-index-pack.txt | 17 +++++++++++------
builtin/index-pack.c | 5 +++--
t/t5300-pack-object.sh | 29 ++++++++++++++++++++++++-----
3 files changed, 38 insertions(+), 13 deletions(-)
diff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt
index 694bb9409bf..3db1062d1e4 100644
--- a/Documentation/git-index-pack.txt
+++ b/Documentation/git-index-pack.txt
@@ -96,13 +96,18 @@ default and "Indexing objects" when `--stdin` is specified.
--check-self-contained-and-connected::
Die if the pack contains broken links. For internal use only.
---fsck-objects::
- For internal use only.
+--fsck-objects[=<msg-id>=<severity>...]::
+ Die if the pack contains broken objects. If the pack contains a tree
+ pointing to a .gitmodules blob that does not exist, prints the hash of
+ that blob (for the caller to check) after the hash that goes into the
+ name of the pack/idx file (see "Notes").
+
-Die if the pack contains broken objects. If the pack contains a tree
-pointing to a .gitmodules blob that does not exist, prints the hash of
-that blob (for the caller to check) after the hash that goes into the
-name of the pack/idx file (see "Notes").
+Unlike `--strict` however, don't choke on broken links. An optional
+comma-separated list of `<msg-id>=<severity>` can be passed to change the
+severity of some possible issues, e.g.,
+`--fsck-objects="missingEmail=ignore,badTagName=ignore"`. See the entry for the
+`fsck.<msg-id>` configuration options in linkgit:git-fsck[1] for more
+information on the possible values of `<msg-id>` and `<severity>`.
--threads=<n>::
Specifies the number of threads to spawn when resolving
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 240c7021168..a3a37bd215d 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -24,7 +24,7 @@
#include "setup.h"
static const char index_pack_usage[] =
-"git index-pack [-v] [-o <index-file>] [--keep | --keep=<msg>] [--[no-]rev-index] [--verify] [--strict[=<msg-id>=<severity>...]] (<pack-file> | --stdin [--fix-thin] [<pack-file>])";
+"git index-pack [-v] [-o <index-file>] [--keep | --keep=<msg>] [--[no-]rev-index] [--verify] [--strict[=<msg-id>=<severity>...]] [--fsck-objects[=<msg-id>=<severity>...]] (<pack-file> | --stdin [--fix-thin] [<pack-file>])";
struct object_entry {
struct pack_idx_entry idx;
@@ -1785,8 +1785,9 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
} else if (!strcmp(arg, "--check-self-contained-and-connected")) {
strict = 1;
check_self_contained_and_connected = 1;
- } else if (!strcmp(arg, "--fsck-objects")) {
+ } else if (skip_to_optional_arg(arg, "--fsck-objects", &arg)) {
do_fsck_object = 1;
+ fsck_set_msg_types(&fsck_options, arg);
} else if (!strcmp(arg, "--verify")) {
verify = 1;
} else if (!strcmp(arg, "--verify-stat")) {
diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 496fffa0f8a..a58f91035d1 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -441,8 +441,7 @@ test_expect_success 'index-pack with --strict' '
)
'
-test_expect_success 'index-pack with --strict downgrading fsck msgs' '
- test_when_finished rm -rf strict &&
+test_expect_success 'setup for --strict and --fsck-objects downgrading fsck msgs' '
git init strict &&
(
cd strict &&
@@ -457,12 +456,32 @@ test_expect_success 'index-pack with --strict downgrading fsck msgs' '
EOF
git hash-object --literally -t commit -w --stdin <commit >commit_list &&
- PACK=$(git pack-objects test <commit_list) &&
- test_must_fail git index-pack --strict "test-$PACK.pack" &&
- git index-pack --strict="missingEmail=ignore" "test-$PACK.pack"
+ git pack-objects test <commit_list >pack-name
)
'
+test_with_bad_commit () {
+ must_fail_arg="$1" &&
+ must_pass_arg="$2" &&
+ (
+ cd strict &&
+ test_expect_fail git index-pack "$must_fail_arg" "test-$(cat pack-name).pack"
+ git index-pack "$must_pass_arg" "test-$(cat pack-name).pack"
+ )
+}
+
+test_expect_success 'index-pack with --strict downgrading fsck msgs' '
+ test_with_bad_commit --strict --strict="missingEmail=ignore"
+'
+
+test_expect_success 'index-pack with --fsck-objects downgrading fsck msgs' '
+ test_with_bad_commit --fsck-objects --fsck-objects="missingEmail=ignore"
+'
+
+test_expect_success 'cleanup for --strict and --fsck-objects downgrading fsck msgs' '
+ rm -rf strict
+'
+
test_expect_success 'honor pack.packSizeLimit' '
git config pack.packSizeLimit 3m &&
packname_10=$(git pack-objects test-10 <obj-list) &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3 0/2] index-pack: fsck honor checks
2024-01-26 20:59 ` [PATCH v3 0/2] index-pack: fsck honor checks John Cai via GitGitGadget
2024-01-26 20:59 ` [PATCH v3 1/2] index-pack: test and document --strict=<msg-id>=<severity> John Cai via GitGitGadget
2024-01-26 20:59 ` [PATCH v3 2/2] index-pack: --fsck-objects to take an optional argument for fsck msgs John Cai via GitGitGadget
@ 2024-01-26 21:18 ` Junio C Hamano
2024-01-26 22:11 ` John Cai
2024-01-26 22:13 ` Jonathan Tan
2024-02-01 1:38 ` [PATCH v4 " John Cai via GitGitGadget
3 siblings, 2 replies; 29+ messages in thread
From: Junio C Hamano @ 2024-01-26 21:18 UTC (permalink / raw)
To: John Cai via GitGitGadget, Jonathan Tan; +Cc: git, John Cai
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 1: b3b3e8bd0bf ! 1: cdf7fc7fe8a index-pack: test and document --strict=<msg>
> @@ Metadata
> Author: John Cai <johncai86@gmail.com>
>
> ## Commit message ##
> - index-pack: test and document --strict=<msg>
> + index-pack: test and document --strict=<msg-id>=<severity>...
Ah, I missed this one. Nice spotting.
> 5d477a334a (fsck (receive-pack): allow demoting errors to warnings,
> 2015-06-22) allowed a list of fsck msg to downgrade to be passed to
> @@ Commit message
> directly, (nor use index-pack for that matter) it is still useful to
> document and test this feature.
>
> + Reviewed-by: Christian Couder <christian.couder@gmail.com>
> Signed-off-by: John Cai <johncai86@gmail.com>
I haven't seen Christian involved (by getting Cc'ed these patches,
sending out review comments, or giving his Reviewed-by:) during
these three rounds of this topic. I'll wait until I hear from him
before queuing this, just to be safe.
> ++ Die, if the pack contains broken objects or links. An optional
> ++ comma-separated list of `<msg-id>=<severity>` can be passed to change
> ++ the severity of some possible issues, e.g.,
> ++ `--strict="missingEmail=ignore,badTagName=error"`. See the entry for the
> ++ `fsck.<msg-id>` configuration options in linkgit:git-fsck[1] for more
> ++ information on the possible values of `<msg-id>` and `<severity>`.
This is much better than the tentative text I tweaked. Nice.
> ++--fsck-objects[=<msg-id>=<severity>...]::
> ++ Die if the pack contains broken objects. If the pack contains a tree
> ++ pointing to a .gitmodules blob that does not exist, prints the hash of
> ++ that blob (for the caller to check) after the hash that goes into the
> ++ name of the pack/idx file (see "Notes").
Not a new problem bit I have to wonder what happens if the pack
contains many trees that point at different blobs for ".gitmodules"
path and many of these blobs are not included in the packfile? Will
the caller receive all of these blob object names so that they can
be verified? The reference to the "Notes" only refer to the fact
that usually a single hash value that is used in constructing the
name of the packfile "pack-<Hashvalue>.pack" is emitted to the
standard output, which is not wrong per se, but does not help
readers very much wrt to understanding this.
[jc: dragging JTan into the thread, as this comes from his 5476e1ef
(fetch-pack: print and use dangling .gitmodules, 2021-02-22)].
> +
> ++Unlike `--strict` however, don't choke on broken links. An optional
You'd need a comma on both sides of "however" used like this, I
think.
In any case, I thought your original construction to have this
"unlike" immediately after "die on broken objects" was far easier to
follow.
Thanks.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 0/2] index-pack: fsck honor checks
2024-01-26 21:18 ` [PATCH v3 0/2] index-pack: fsck honor checks Junio C Hamano
@ 2024-01-26 22:11 ` John Cai
2024-01-29 11:15 ` Patrick Steinhardt
2024-01-26 22:13 ` Jonathan Tan
1 sibling, 1 reply; 29+ messages in thread
From: John Cai @ 2024-01-26 22:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Cai via GitGitGadget, Jonathan Tan, git
Hi Junio,
On 26 Jan 2024, at 16:18, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> 1: b3b3e8bd0bf ! 1: cdf7fc7fe8a index-pack: test and document --strict=<msg>
>> @@ Metadata
>> Author: John Cai <johncai86@gmail.com>
>>
>> ## Commit message ##
>> - index-pack: test and document --strict=<msg>
>> + index-pack: test and document --strict=<msg-id>=<severity>...
>
> Ah, I missed this one. Nice spotting.
>
>> 5d477a334a (fsck (receive-pack): allow demoting errors to warnings,
>> 2015-06-22) allowed a list of fsck msg to downgrade to be passed to
>> @@ Commit message
>> directly, (nor use index-pack for that matter) it is still useful to
>> document and test this feature.
>>
>> + Reviewed-by: Christian Couder <christian.couder@gmail.com>
>> Signed-off-by: John Cai <johncai86@gmail.com>
>
> I haven't seen Christian involved (by getting Cc'ed these patches,
> sending out review comments, or giving his Reviewed-by:) during
> these three rounds of this topic. I'll wait until I hear from him
> before queuing this, just to be safe.
Christian was involved on an off-list review of this patch series. You can see
it in [1].
1. https://gitlab.com/gitlab-org/git/-/merge_requests/88
>
>> ++ Die, if the pack contains broken objects or links. An optional
>> ++ comma-separated list of `<msg-id>=<severity>` can be passed to change
>> ++ the severity of some possible issues, e.g.,
>> ++ `--strict="missingEmail=ignore,badTagName=error"`. See the entry for the
>> ++ `fsck.<msg-id>` configuration options in linkgit:git-fsck[1] for more
>> ++ information on the possible values of `<msg-id>` and `<severity>`.
>
> This is much better than the tentative text I tweaked. Nice.
>
>> ++--fsck-objects[=<msg-id>=<severity>...]::
>> ++ Die if the pack contains broken objects. If the pack contains a tree
>> ++ pointing to a .gitmodules blob that does not exist, prints the hash of
>> ++ that blob (for the caller to check) after the hash that goes into the
>> ++ name of the pack/idx file (see "Notes").
>
> Not a new problem bit I have to wonder what happens if the pack
> contains many trees that point at different blobs for ".gitmodules"
> path and many of these blobs are not included in the packfile? Will
> the caller receive all of these blob object names so that they can
> be verified? The reference to the "Notes" only refer to the fact
> that usually a single hash value that is used in constructing the
> name of the packfile "pack-<Hashvalue>.pack" is emitted to the
> standard output, which is not wrong per se, but does not help
> readers very much wrt to understanding this.
>
> [jc: dragging JTan into the thread, as this comes from his 5476e1ef
> (fetch-pack: print and use dangling .gitmodules, 2021-02-22)].
sounds good, will wait for some clarification here
>
>> +
>> ++Unlike `--strict` however, don't choke on broken links. An optional
>
> You'd need a comma on both sides of "however" used like this, I
> think.
good catch
>
> In any case, I thought your original construction to have this
> "unlike" immediately after "die on broken objects" was far easier to
> follow.
I'll reformulate this to be clearer. From the previous version I realized I
didn't take into account the pre-existing "Die if the pack contains broken
objects" block so I put it at the beginning. But now I think you're right in
that the "Unlike..." comes too late.
>
> Thanks.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 0/2] index-pack: fsck honor checks
2024-01-26 22:11 ` John Cai
@ 2024-01-29 11:15 ` Patrick Steinhardt
2024-01-29 17:18 ` Junio C Hamano
0 siblings, 1 reply; 29+ messages in thread
From: Patrick Steinhardt @ 2024-01-29 11:15 UTC (permalink / raw)
To: John Cai; +Cc: Junio C Hamano, John Cai via GitGitGadget, Jonathan Tan, git
[-- Attachment #1: Type: text/plain, Size: 2334 bytes --]
On Fri, Jan 26, 2024 at 05:11:14PM -0500, John Cai wrote:
> Hi Junio,
>
> On 26 Jan 2024, at 16:18, Junio C Hamano wrote:
>
> > "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> >> 1: b3b3e8bd0bf ! 1: cdf7fc7fe8a index-pack: test and document --strict=<msg>
> >> @@ Metadata
> >> Author: John Cai <johncai86@gmail.com>
> >>
> >> ## Commit message ##
> >> - index-pack: test and document --strict=<msg>
> >> + index-pack: test and document --strict=<msg-id>=<severity>...
> >
> > Ah, I missed this one. Nice spotting.
> >
> >> 5d477a334a (fsck (receive-pack): allow demoting errors to warnings,
> >> 2015-06-22) allowed a list of fsck msg to downgrade to be passed to
> >> @@ Commit message
> >> directly, (nor use index-pack for that matter) it is still useful to
> >> document and test this feature.
> >>
> >> + Reviewed-by: Christian Couder <christian.couder@gmail.com>
> >> Signed-off-by: John Cai <johncai86@gmail.com>
> >
> > I haven't seen Christian involved (by getting Cc'ed these patches,
> > sending out review comments, or giving his Reviewed-by:) during
> > these three rounds of this topic. I'll wait until I hear from him
> > before queuing this, just to be safe.
>
> Christian was involved on an off-list review of this patch series. You can see
> it in [1].
>
> 1. https://gitlab.com/gitlab-org/git/-/merge_requests/88
I'm always a bit hesitant to add trailers referring to off-list reviews
to commits. It's impossible for a future reader to discover how that
trailer came to be by just using the mailing list archive, and expecting
them to use third-party services to verify them feels wrong to me.
It's part of the reason why I'm pushing more into the direction of
on-list reviews at GitLab. It makes it a lot more obvious how such a
Reviewed-by came to be and keeps things self-contained on the mailing
list. It also grows new contributors who are becoming more familiar with
how the Git mailing list works. If such a review already happened
internally due to whatever reason then I think it ought to be fine for
that reviewer to chime in saying that they have already reviewed the
patch series and that things look good to them.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 0/2] index-pack: fsck honor checks
2024-01-29 11:15 ` Patrick Steinhardt
@ 2024-01-29 17:18 ` Junio C Hamano
0 siblings, 0 replies; 29+ messages in thread
From: Junio C Hamano @ 2024-01-29 17:18 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: John Cai, John Cai via GitGitGadget, Jonathan Tan, git
Patrick Steinhardt <ps@pks.im> writes:
> I'm always a bit hesitant to add trailers referring to off-list reviews
> to commits. It's impossible for a future reader to discover how that
> trailer came to be by just using the mailing list archive, and expecting
> them to use third-party services to verify them feels wrong to me.
>
> It's part of the reason why I'm pushing more into the direction of
> on-list reviews at GitLab. It makes it a lot more obvious how such a
> Reviewed-by came to be and keeps things self-contained on the mailing
> list. It also grows new contributors who are becoming more familiar with
> how the Git mailing list works. If such a review already happened
> internally due to whatever reason then I think it ought to be fine for
> that reviewer to chime in saying that they have already reviewed the
> patch series and that things look good to them.
Thanks. That would improve clarifying a situation like this one
(eh, actually, once it is done this particular situation wouldn't
need any clarification).
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 0/2] index-pack: fsck honor checks
2024-01-26 21:18 ` [PATCH v3 0/2] index-pack: fsck honor checks Junio C Hamano
2024-01-26 22:11 ` John Cai
@ 2024-01-26 22:13 ` Jonathan Tan
2024-01-27 2:31 ` John Cai
1 sibling, 1 reply; 29+ messages in thread
From: Jonathan Tan @ 2024-01-26 22:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Tan, John Cai via GitGitGadget, git, John Cai
Junio C Hamano <gitster@pobox.com> writes:
> > ++--fsck-objects[=<msg-id>=<severity>...]::
> > ++ Die if the pack contains broken objects. If the pack contains a tree
> > ++ pointing to a .gitmodules blob that does not exist, prints the hash of
> > ++ that blob (for the caller to check) after the hash that goes into the
> > ++ name of the pack/idx file (see "Notes").
>
> Not a new problem bit I have to wonder what happens if the pack
> contains many trees that point at different blobs for ".gitmodules"
> path and many of these blobs are not included in the packfile? Will
> the caller receive all of these blob object names so that they can
> be verified? The reference to the "Notes" only refer to the fact
> that usually a single hash value that is used in constructing the
> name of the packfile "pack-<Hashvalue>.pack" is emitted to the
> standard output, which is not wrong per se, but does not help
> readers very much wrt to understanding this.
>
> [jc: dragging JTan into the thread, as this comes from his 5476e1ef
> (fetch-pack: print and use dangling .gitmodules, 2021-02-22)].
Ah...I can see how that documentation isn't clear. The intention of that
commit is to check every link to a .gitmodules blob. The tests perhaps
should have been written with 2 .gitmodules blobs (in separate commits),
but I think the production code works: I tried changing the test to have
2 commits each with their own .gitmodules blob, and error messages were
printed for both blobs.
(If someone changes that test, e.g. to have 2 blobs, the ">h" in the
"configure_exclusion" invocations look superfluous and is perhaps a
copy-and-paste error from other tests that needed the hash later.)
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 0/2] index-pack: fsck honor checks
2024-01-26 22:13 ` Jonathan Tan
@ 2024-01-27 2:31 ` John Cai
2024-01-31 22:30 ` Jonathan Tan
0 siblings, 1 reply; 29+ messages in thread
From: John Cai @ 2024-01-27 2:31 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Junio C Hamano, John Cai via GitGitGadget, git
Hi Jonathan,
On 26 Jan 2024, at 17:13, Jonathan Tan wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>>> ++--fsck-objects[=<msg-id>=<severity>...]::
>>> ++ Die if the pack contains broken objects. If the pack contains a tree
>>> ++ pointing to a .gitmodules blob that does not exist, prints the hash of
>>> ++ that blob (for the caller to check) after the hash that goes into the
>>> ++ name of the pack/idx file (see "Notes").
>>
>> Not a new problem bit I have to wonder what happens if the pack
>> contains many trees that point at different blobs for ".gitmodules"
>> path and many of these blobs are not included in the packfile? Will
>> the caller receive all of these blob object names so that they can
>> be verified? The reference to the "Notes" only refer to the fact
>> that usually a single hash value that is used in constructing the
>> name of the packfile "pack-<Hashvalue>.pack" is emitted to the
>> standard output, which is not wrong per se, but does not help
>> readers very much wrt to understanding this.
>>
>> [jc: dragging JTan into the thread, as this comes from his 5476e1ef
>> (fetch-pack: print and use dangling .gitmodules, 2021-02-22)].
>
> Ah...I can see how that documentation isn't clear. The intention of that
> commit is to check every link to a .gitmodules blob. The tests perhaps
> should have been written with 2 .gitmodules blobs (in separate commits),
> but I think the production code works: I tried changing the test to have
> 2 commits each with their own .gitmodules blob, and error messages were
> printed for both blobs.
Thanks for clarifying! Would you mind providing a patch to revise the wording
here to make it clearer? I would try but I feel like I might get the wording
wrong.
>
> (If someone changes that test, e.g. to have 2 blobs, the ">h" in the
> "configure_exclusion" invocations look superfluous and is perhaps a
> copy-and-paste error from other tests that needed the hash later.)
thanks
John
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 0/2] index-pack: fsck honor checks
2024-01-27 2:31 ` John Cai
@ 2024-01-31 22:30 ` Jonathan Tan
2024-02-01 1:34 ` John Cai
0 siblings, 1 reply; 29+ messages in thread
From: Jonathan Tan @ 2024-01-31 22:30 UTC (permalink / raw)
To: John Cai; +Cc: Jonathan Tan, Junio C Hamano, John Cai via GitGitGadget, git
John Cai <johncai86@gmail.com> writes:
> Hi Jonathan,
>
> On 26 Jan 2024, at 17:13, Jonathan Tan wrote:
>
> > Junio C Hamano <gitster@pobox.com> writes:
> >>> ++--fsck-objects[=<msg-id>=<severity>...]::
> >>> ++ Die if the pack contains broken objects. If the pack contains a tree
> >>> ++ pointing to a .gitmodules blob that does not exist, prints the hash of
> >>> ++ that blob (for the caller to check) after the hash that goes into the
> >>> ++ name of the pack/idx file (see "Notes").
> Thanks for clarifying! Would you mind providing a patch to revise the wording
> here to make it clearer? I would try but I feel like I might get the wording
> wrong.
I think the wording there is already mostly correct, except maybe make
everything plural (a tree -> trees, a .gitmodules blob -> .gitmodules
blobs, hash of that blob -> hashes of those blobs). We might also need
to modify a test to show that the current code indeed handles the plural
situation correctly. I don't have time right now to get to this, so
hopefully someone could pick this up.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 0/2] index-pack: fsck honor checks
2024-01-31 22:30 ` Jonathan Tan
@ 2024-02-01 1:34 ` John Cai
2024-02-01 16:44 ` Junio C Hamano
0 siblings, 1 reply; 29+ messages in thread
From: John Cai @ 2024-02-01 1:34 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Junio C Hamano, John Cai via GitGitGadget, git
Hi Jonathan,
On 31 Jan 2024, at 17:30, Jonathan Tan wrote:
> John Cai <johncai86@gmail.com> writes:
>> Hi Jonathan,
>>
>> On 26 Jan 2024, at 17:13, Jonathan Tan wrote:
>>
>>> Junio C Hamano <gitster@pobox.com> writes:
>>>>> ++--fsck-objects[=<msg-id>=<severity>...]::
>>>>> ++ Die if the pack contains broken objects. If the pack contains a tree
>>>>> ++ pointing to a .gitmodules blob that does not exist, prints the hash of
>>>>> ++ that blob (for the caller to check) after the hash that goes into the
>>>>> ++ name of the pack/idx file (see "Notes").
>
>> Thanks for clarifying! Would you mind providing a patch to revise the wording
>> here to make it clearer? I would try but I feel like I might get the wording
>> wrong.
>
> I think the wording there is already mostly correct, except maybe make
> everything plural (a tree -> trees, a .gitmodules blob -> .gitmodules
> blobs, hash of that blob -> hashes of those blobs). We might also need
> to modify a test to show that the current code indeed handles the plural
> situation correctly. I don't have time right now to get to this, so
> hopefully someone could pick this up.
Thanks! It sounds like we may want to tackle this as part of another patch.
John
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 0/2] index-pack: fsck honor checks
2024-02-01 1:34 ` John Cai
@ 2024-02-01 16:44 ` Junio C Hamano
0 siblings, 0 replies; 29+ messages in thread
From: Junio C Hamano @ 2024-02-01 16:44 UTC (permalink / raw)
To: John Cai; +Cc: Jonathan Tan, John Cai via GitGitGadget, git
John Cai <johncai86@gmail.com> writes:
>>> Thanks for clarifying! Would you mind providing a patch to revise the wording
>>> here to make it clearer? I would try but I feel like I might get the wording
>>> wrong.
>>
>> I think the wording there is already mostly correct, except maybe make
>> everything plural (a tree -> trees, a .gitmodules blob -> .gitmodules
>> blobs, hash of that blob -> hashes of those blobs). We might also need
>> to modify a test to show that the current code indeed handles the plural
>> situation correctly. I don't have time right now to get to this, so
>> hopefully someone could pick this up.
>
> Thanks! It sounds like we may want to tackle this as part of another patch.
Yeah, the existing documentation has been with our users for some
time, and it is not ultra urgent to fix it in that sense. I'd say
that it can even wait until JTan gets bored with what he's doing and
needs some distraction himself ;-)
As long as our collective mind remembers it as #leftoverbits it
would be sufficient.
Thanks, both.
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v4 0/2] index-pack: fsck honor checks
2024-01-26 20:59 ` [PATCH v3 0/2] index-pack: fsck honor checks John Cai via GitGitGadget
` (2 preceding siblings ...)
2024-01-26 21:18 ` [PATCH v3 0/2] index-pack: fsck honor checks Junio C Hamano
@ 2024-02-01 1:38 ` John Cai via GitGitGadget
2024-02-01 1:38 ` [PATCH v4 1/2] index-pack: test and document --strict=<msg-id>=<severity> John Cai via GitGitGadget
` (2 more replies)
3 siblings, 3 replies; 29+ messages in thread
From: John Cai via GitGitGadget @ 2024-02-01 1:38 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, Patrick Steinhardt, John Cai
git-index-pack has a --strict mode that can take an optional argument to
provide a list of fsck issues to change their severity. --fsck-objects does
not have such a utility, which would be useful if one would like to be more
lenient or strict on data integrity in a repository.
Like --strict, Allow --fsck-objects to also take a list of fsck msgs to
change the severity.
Changes since V3:
* clarification of --fsck-objects documentation wording
Changes since V2:
* fixed some typos in the documentation
* added commit trailers
Change since V1:
* edited commit messages
* clarified formatting in documentation for --strict= and --fsck-objects=
John Cai (2):
index-pack: test and document --strict=<msg-id>=<severity>...
index-pack: --fsck-objects to take an optional argument for fsck msgs
Documentation/git-index-pack.txt | 26 +++++++++++++-------
builtin/index-pack.c | 5 ++--
t/t5300-pack-object.sh | 41 ++++++++++++++++++++++++++++++++
3 files changed, 62 insertions(+), 10 deletions(-)
base-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1658%2Fjohn-cai%2Fjc%2Findex-pack-fsck-honor-checks-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1658/john-cai/jc/index-pack-fsck-honor-checks-v4
Pull-Request: https://github.com/git/git/pull/1658
Range-diff vs v3:
1: cdf7fc7fe8a = 1: cdf7fc7fe8a index-pack: test and document --strict=<msg-id>=<severity>...
2: a2b9adb93d8 ! 2: f29ab9136fb index-pack: --fsck-objects to take an optional argument for fsck msgs
@@ Documentation/git-index-pack.txt: default and "Indexing objects" when `--stdin`
---fsck-objects::
- For internal use only.
+--fsck-objects[=<msg-id>=<severity>...]::
-+ Die if the pack contains broken objects. If the pack contains a tree
-+ pointing to a .gitmodules blob that does not exist, prints the hash of
-+ that blob (for the caller to check) after the hash that goes into the
-+ name of the pack/idx file (see "Notes").
++ Die if the pack contains broken objects, but unlike `--strict`, don't
++ choke on broken links. If the pack contains a tree pointing to a
++ .gitmodules blob that does not exist, prints the hash of that blob
++ (for the caller to check) after the hash that goes into the name of the
++ pack/idx file (see "Notes").
+
-Die if the pack contains broken objects. If the pack contains a tree
-pointing to a .gitmodules blob that does not exist, prints the hash of
-that blob (for the caller to check) after the hash that goes into the
-name of the pack/idx file (see "Notes").
-+Unlike `--strict` however, don't choke on broken links. An optional
-+comma-separated list of `<msg-id>=<severity>` can be passed to change the
-+severity of some possible issues, e.g.,
++An optional comma-separated list of `<msg-id>=<severity>` can be passed to
++change the severity of some possible issues, e.g.,
+`--fsck-objects="missingEmail=ignore,badTagName=ignore"`. See the entry for the
+`fsck.<msg-id>` configuration options in linkgit:git-fsck[1] for more
+information on the possible values of `<msg-id>` and `<severity>`.
--
gitgitgadget
^ permalink raw reply [flat|nested] 29+ messages in thread* [PATCH v4 1/2] index-pack: test and document --strict=<msg-id>=<severity>...
2024-02-01 1:38 ` [PATCH v4 " John Cai via GitGitGadget
@ 2024-02-01 1:38 ` John Cai via GitGitGadget
2024-02-01 1:38 ` [PATCH v4 2/2] index-pack: --fsck-objects to take an optional argument for fsck msgs John Cai via GitGitGadget
2024-02-02 15:48 ` [PATCH v4 0/2] index-pack: fsck honor checks Christian Couder
2 siblings, 0 replies; 29+ messages in thread
From: John Cai via GitGitGadget @ 2024-02-01 1:38 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, Patrick Steinhardt, John Cai, John Cai
From: John Cai <johncai86@gmail.com>
5d477a334a (fsck (receive-pack): allow demoting errors to warnings,
2015-06-22) allowed a list of fsck msg to downgrade to be passed to
--strict. However this is a hidden argument that was not documented nor
tested. Though it is true that most users would not call this option
directly, (nor use index-pack for that matter) it is still useful to
document and test this feature.
Reviewed-by: Christian Couder <christian.couder@gmail.com>
Signed-off-by: John Cai <johncai86@gmail.com>
---
Documentation/git-index-pack.txt | 9 +++++++--
builtin/index-pack.c | 2 +-
t/t5300-pack-object.sh | 22 ++++++++++++++++++++++
3 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt
index 6486620c3d8..694bb9409bf 100644
--- a/Documentation/git-index-pack.txt
+++ b/Documentation/git-index-pack.txt
@@ -79,8 +79,13 @@ OPTIONS
to force the version for the generated pack index, and to force
64-bit index entries on objects located above the given offset.
---strict::
- Die, if the pack contains broken objects or links.
+--strict[=<msg-id>=<severity>...]::
+ Die, if the pack contains broken objects or links. An optional
+ comma-separated list of `<msg-id>=<severity>` can be passed to change
+ the severity of some possible issues, e.g.,
+ `--strict="missingEmail=ignore,badTagName=error"`. See the entry for the
+ `fsck.<msg-id>` configuration options in linkgit:git-fsck[1] for more
+ information on the possible values of `<msg-id>` and `<severity>`.
--progress-title::
For internal use only.
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 1ea87e01f29..240c7021168 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -24,7 +24,7 @@
#include "setup.h"
static const char index_pack_usage[] =
-"git index-pack [-v] [-o <index-file>] [--keep | --keep=<msg>] [--[no-]rev-index] [--verify] [--strict] (<pack-file> | --stdin [--fix-thin] [<pack-file>])";
+"git index-pack [-v] [-o <index-file>] [--keep | --keep=<msg>] [--[no-]rev-index] [--verify] [--strict[=<msg-id>=<severity>...]] (<pack-file> | --stdin [--fix-thin] [<pack-file>])";
struct object_entry {
struct pack_idx_entry idx;
diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index d402ec18b79..496fffa0f8a 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -441,6 +441,28 @@ test_expect_success 'index-pack with --strict' '
)
'
+test_expect_success 'index-pack with --strict downgrading fsck msgs' '
+ test_when_finished rm -rf strict &&
+ git init strict &&
+ (
+ cd strict &&
+ test_commit first hello &&
+ cat >commit <<-EOF &&
+ tree $(git rev-parse HEAD^{tree})
+ parent $(git rev-parse HEAD)
+ author A U Thor
+ committer A U Thor
+
+ commit: this is a commit with bad emails
+
+ EOF
+ git hash-object --literally -t commit -w --stdin <commit >commit_list &&
+ PACK=$(git pack-objects test <commit_list) &&
+ test_must_fail git index-pack --strict "test-$PACK.pack" &&
+ git index-pack --strict="missingEmail=ignore" "test-$PACK.pack"
+ )
+'
+
test_expect_success 'honor pack.packSizeLimit' '
git config pack.packSizeLimit 3m &&
packname_10=$(git pack-objects test-10 <obj-list) &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v4 2/2] index-pack: --fsck-objects to take an optional argument for fsck msgs
2024-02-01 1:38 ` [PATCH v4 " John Cai via GitGitGadget
2024-02-01 1:38 ` [PATCH v4 1/2] index-pack: test and document --strict=<msg-id>=<severity> John Cai via GitGitGadget
@ 2024-02-01 1:38 ` John Cai via GitGitGadget
2024-03-08 22:24 ` SZEDER Gábor
2024-02-02 15:48 ` [PATCH v4 0/2] index-pack: fsck honor checks Christian Couder
2 siblings, 1 reply; 29+ messages in thread
From: John Cai via GitGitGadget @ 2024-02-01 1:38 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, Patrick Steinhardt, John Cai, John Cai
From: John Cai <johncai86@gmail.com>
git-index-pack has a --strict option that can take an optional argument
to provide a list of fsck issues to change their severity.
--fsck-objects does not have such a utility, which would be useful if
one would like to be more lenient or strict on data integrity in a
repository.
Like --strict, allow --fsck-objects to also take a list of fsck msgs to
change the severity.
Remove the "For internal use only" note for --fsck-objects, and document
the option. This won't often be used by the normal end user, but it
turns out it is useful for Git forges like GitLab.
Reviewed-by: Christian Couder <christian.couder@gmail.com>
Signed-off-by: John Cai <johncai86@gmail.com>
---
Documentation/git-index-pack.txt | 17 +++++++++++------
builtin/index-pack.c | 5 +++--
t/t5300-pack-object.sh | 29 ++++++++++++++++++++++++-----
3 files changed, 38 insertions(+), 13 deletions(-)
diff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt
index 694bb9409bf..5a20deefd5f 100644
--- a/Documentation/git-index-pack.txt
+++ b/Documentation/git-index-pack.txt
@@ -96,13 +96,18 @@ default and "Indexing objects" when `--stdin` is specified.
--check-self-contained-and-connected::
Die if the pack contains broken links. For internal use only.
---fsck-objects::
- For internal use only.
+--fsck-objects[=<msg-id>=<severity>...]::
+ Die if the pack contains broken objects, but unlike `--strict`, don't
+ choke on broken links. If the pack contains a tree pointing to a
+ .gitmodules blob that does not exist, prints the hash of that blob
+ (for the caller to check) after the hash that goes into the name of the
+ pack/idx file (see "Notes").
+
-Die if the pack contains broken objects. If the pack contains a tree
-pointing to a .gitmodules blob that does not exist, prints the hash of
-that blob (for the caller to check) after the hash that goes into the
-name of the pack/idx file (see "Notes").
+An optional comma-separated list of `<msg-id>=<severity>` can be passed to
+change the severity of some possible issues, e.g.,
+`--fsck-objects="missingEmail=ignore,badTagName=ignore"`. See the entry for the
+`fsck.<msg-id>` configuration options in linkgit:git-fsck[1] for more
+information on the possible values of `<msg-id>` and `<severity>`.
--threads=<n>::
Specifies the number of threads to spawn when resolving
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 240c7021168..a3a37bd215d 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -24,7 +24,7 @@
#include "setup.h"
static const char index_pack_usage[] =
-"git index-pack [-v] [-o <index-file>] [--keep | --keep=<msg>] [--[no-]rev-index] [--verify] [--strict[=<msg-id>=<severity>...]] (<pack-file> | --stdin [--fix-thin] [<pack-file>])";
+"git index-pack [-v] [-o <index-file>] [--keep | --keep=<msg>] [--[no-]rev-index] [--verify] [--strict[=<msg-id>=<severity>...]] [--fsck-objects[=<msg-id>=<severity>...]] (<pack-file> | --stdin [--fix-thin] [<pack-file>])";
struct object_entry {
struct pack_idx_entry idx;
@@ -1785,8 +1785,9 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
} else if (!strcmp(arg, "--check-self-contained-and-connected")) {
strict = 1;
check_self_contained_and_connected = 1;
- } else if (!strcmp(arg, "--fsck-objects")) {
+ } else if (skip_to_optional_arg(arg, "--fsck-objects", &arg)) {
do_fsck_object = 1;
+ fsck_set_msg_types(&fsck_options, arg);
} else if (!strcmp(arg, "--verify")) {
verify = 1;
} else if (!strcmp(arg, "--verify-stat")) {
diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 496fffa0f8a..a58f91035d1 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -441,8 +441,7 @@ test_expect_success 'index-pack with --strict' '
)
'
-test_expect_success 'index-pack with --strict downgrading fsck msgs' '
- test_when_finished rm -rf strict &&
+test_expect_success 'setup for --strict and --fsck-objects downgrading fsck msgs' '
git init strict &&
(
cd strict &&
@@ -457,12 +456,32 @@ test_expect_success 'index-pack with --strict downgrading fsck msgs' '
EOF
git hash-object --literally -t commit -w --stdin <commit >commit_list &&
- PACK=$(git pack-objects test <commit_list) &&
- test_must_fail git index-pack --strict "test-$PACK.pack" &&
- git index-pack --strict="missingEmail=ignore" "test-$PACK.pack"
+ git pack-objects test <commit_list >pack-name
)
'
+test_with_bad_commit () {
+ must_fail_arg="$1" &&
+ must_pass_arg="$2" &&
+ (
+ cd strict &&
+ test_expect_fail git index-pack "$must_fail_arg" "test-$(cat pack-name).pack"
+ git index-pack "$must_pass_arg" "test-$(cat pack-name).pack"
+ )
+}
+
+test_expect_success 'index-pack with --strict downgrading fsck msgs' '
+ test_with_bad_commit --strict --strict="missingEmail=ignore"
+'
+
+test_expect_success 'index-pack with --fsck-objects downgrading fsck msgs' '
+ test_with_bad_commit --fsck-objects --fsck-objects="missingEmail=ignore"
+'
+
+test_expect_success 'cleanup for --strict and --fsck-objects downgrading fsck msgs' '
+ rm -rf strict
+'
+
test_expect_success 'honor pack.packSizeLimit' '
git config pack.packSizeLimit 3m &&
packname_10=$(git pack-objects test-10 <obj-list) &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v4 2/2] index-pack: --fsck-objects to take an optional argument for fsck msgs
2024-02-01 1:38 ` [PATCH v4 2/2] index-pack: --fsck-objects to take an optional argument for fsck msgs John Cai via GitGitGadget
@ 2024-03-08 22:24 ` SZEDER Gábor
2024-03-09 1:55 ` John Cai
0 siblings, 1 reply; 29+ messages in thread
From: SZEDER Gábor @ 2024-03-08 22:24 UTC (permalink / raw)
To: John Cai via GitGitGadget; +Cc: git, Jonathan Tan, Patrick Steinhardt, John Cai
On Thu, Feb 01, 2024 at 01:38:02AM +0000, John Cai via GitGitGadget wrote:
> diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
> index 496fffa0f8a..a58f91035d1 100755
> --- a/t/t5300-pack-object.sh
> +++ b/t/t5300-pack-object.sh
> @@ -441,8 +441,7 @@ test_expect_success 'index-pack with --strict' '
> )
> '
>
> -test_expect_success 'index-pack with --strict downgrading fsck msgs' '
> - test_when_finished rm -rf strict &&
> +test_expect_success 'setup for --strict and --fsck-objects downgrading fsck msgs' '
> git init strict &&
> (
> cd strict &&
> @@ -457,12 +456,32 @@ test_expect_success 'index-pack with --strict downgrading fsck msgs' '
>
> EOF
> git hash-object --literally -t commit -w --stdin <commit >commit_list &&
> - PACK=$(git pack-objects test <commit_list) &&
> - test_must_fail git index-pack --strict "test-$PACK.pack" &&
> - git index-pack --strict="missingEmail=ignore" "test-$PACK.pack"
> + git pack-objects test <commit_list >pack-name
> )
> '
>
> +test_with_bad_commit () {
> + must_fail_arg="$1" &&
> + must_pass_arg="$2" &&
> + (
> + cd strict &&
> + test_expect_fail git index-pack "$must_fail_arg" "test-$(cat pack-name).pack"
There is no such command as 'test_expect_fail', resulting in:
expecting success of 5300.34 'index-pack with --strict downgrading fsck msgs':
test_with_bad_commit --strict --strict="missingEmail=ignore"
+ test_with_bad_commit --strict --strict=missingEmail=ignore
+ must_fail_arg=--strict
+ must_pass_arg=--strict=missingEmail=ignore
+ cd strict
+ cat pack-name
+ test_expect_fail git index-pack --strict test-e4e1649155bf444fbd9cd85e376628d6eaf3d3bd.pack
./t5300-pack-object.sh: 468: eval: test_expect_fail: not found
+ cat pack-name
+ git index-pack --strict=missingEmail=ignore test-e4e1649155bf444fbd9cd85e376628d6eaf3d3bd.pack
e4e1649155bf444fbd9cd85e376628d6eaf3d3bd
ok 34 - index-pack with --strict downgrading fsck msgs
The missing command should fail the test, but it doesn't, because the
&&-chain is broken on this line as well.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v4 2/2] index-pack: --fsck-objects to take an optional argument for fsck msgs
2024-03-08 22:24 ` SZEDER Gábor
@ 2024-03-09 1:55 ` John Cai
0 siblings, 0 replies; 29+ messages in thread
From: John Cai @ 2024-03-09 1:55 UTC (permalink / raw)
To: SZEDER Gábor
Cc: John Cai via GitGitGadget, git, Jonathan Tan, Patrick Steinhardt
Hi Szeder,
On 8 Mar 2024, at 17:24, SZEDER Gábor wrote:
> On Thu, Feb 01, 2024 at 01:38:02AM +0000, John Cai via GitGitGadget wrote:
>> diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
>> index 496fffa0f8a..a58f91035d1 100755
>> --- a/t/t5300-pack-object.sh
>> +++ b/t/t5300-pack-object.sh
>> @@ -441,8 +441,7 @@ test_expect_success 'index-pack with --strict' '
>> )
>> '
>>
>> -test_expect_success 'index-pack with --strict downgrading fsck msgs' '
>> - test_when_finished rm -rf strict &&
>> +test_expect_success 'setup for --strict and --fsck-objects downgrading fsck msgs' '
>> git init strict &&
>> (
>> cd strict &&
>> @@ -457,12 +456,32 @@ test_expect_success 'index-pack with --strict downgrading fsck msgs' '
>>
>> EOF
>> git hash-object --literally -t commit -w --stdin <commit >commit_list &&
>> - PACK=$(git pack-objects test <commit_list) &&
>> - test_must_fail git index-pack --strict "test-$PACK.pack" &&
>> - git index-pack --strict="missingEmail=ignore" "test-$PACK.pack"
>> + git pack-objects test <commit_list >pack-name
>> )
>> '
>>
>> +test_with_bad_commit () {
>> + must_fail_arg="$1" &&
>> + must_pass_arg="$2" &&
>> + (
>> + cd strict &&
>> + test_expect_fail git index-pack "$must_fail_arg" "test-$(cat pack-name).pack"
>
> There is no such command as 'test_expect_fail', resulting in:
Indeed, thanks for catching this.
>
> expecting success of 5300.34 'index-pack with --strict downgrading fsck msgs':
> test_with_bad_commit --strict --strict="missingEmail=ignore"
>
> + test_with_bad_commit --strict --strict=missingEmail=ignore
> + must_fail_arg=--strict
> + must_pass_arg=--strict=missingEmail=ignore
> + cd strict
> + cat pack-name
> + test_expect_fail git index-pack --strict test-e4e1649155bf444fbd9cd85e376628d6eaf3d3bd.pack
> ./t5300-pack-object.sh: 468: eval: test_expect_fail: not found
> + cat pack-name
> + git index-pack --strict=missingEmail=ignore test-e4e1649155bf444fbd9cd85e376628d6eaf3d3bd.pack
> e4e1649155bf444fbd9cd85e376628d6eaf3d3bd
>
> ok 34 - index-pack with --strict downgrading fsck msgs
>
> The missing command should fail the test, but it doesn't, because the
> &&-chain is broken on this line as well.
yes will need to fix this as well
thanks
John
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 0/2] index-pack: fsck honor checks
2024-02-01 1:38 ` [PATCH v4 " John Cai via GitGitGadget
2024-02-01 1:38 ` [PATCH v4 1/2] index-pack: test and document --strict=<msg-id>=<severity> John Cai via GitGitGadget
2024-02-01 1:38 ` [PATCH v4 2/2] index-pack: --fsck-objects to take an optional argument for fsck msgs John Cai via GitGitGadget
@ 2024-02-02 15:48 ` Christian Couder
2 siblings, 0 replies; 29+ messages in thread
From: Christian Couder @ 2024-02-02 15:48 UTC (permalink / raw)
To: John Cai via GitGitGadget; +Cc: git, Jonathan Tan, Patrick Steinhardt, John Cai
(John, sorry for having already sent this only to you.)
On Thu, Feb 1, 2024 at 2:47 AM John Cai via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> git-index-pack has a --strict mode that can take an optional argument to
> provide a list of fsck issues to change their severity. --fsck-objects does
> not have such a utility, which would be useful if one would like to be more
> lenient or strict on data integrity in a repository.
>
> Like --strict, Allow --fsck-objects to also take a list of fsck msgs to
> change the severity.
>
> Changes since V3:
>
> * clarification of --fsck-objects documentation wording
>
> Changes since V2:
>
> * fixed some typos in the documentation
> * added commit trailers
>
> Change since V1:
>
> * edited commit messages
> * clarified formatting in documentation for --strict= and --fsck-objects=
>
> John Cai (2):
> index-pack: test and document --strict=<msg-id>=<severity>...
> index-pack: --fsck-objects to take an optional argument for fsck msgs
I reviewed internally on GitLab the initial versions of this series
and I reviewed this version 4. It looks great to me, so I am happy to
give my "Reviewed-by:".
Thanks!
^ permalink raw reply [flat|nested] 29+ messages in thread