From: Jonathan Tan <hidden> Date: 2021-01-15 23:43:52
Someone at $DAYJOB noticed that if a .gitmodules-containing tree and the
.gitmodules blob itself are sent in 2 separate packfiles during a fetch
(which can happen when packfile URIs are used), transfer.fsckobjects
causes the fetch to fail. You can reproduce it as follows (as of the
time of writing):
$ git -c fetch.uriprotocols=https -c transfer.fsckobjects=true clone https://chromium.googlesource.com/chromiumos/codesearch
Cloning into 'codesearch'...
remote: Total 2242 (delta 0), reused 2242 (delta 0)
Receiving objects: 100% (2242/2242), 1.77 MiB | 4.62 MiB/s, done.
error: object 1f155c20935ee1154a813a814f03ef2b3976680f: gitmodulesMissing: unable to read .gitmodules blob
fatal: fsck error in pack objects
fatal: index-pack failed
This happens because the fsck part is currently being done in
index-pack, which operates on one pack at a time. When index-pack sees
the tree, it runs fsck on it (like any other object), and the fsck
subsystem remembers the .gitmodules target (specifically, in
gitmodules_found in fsck.c). Later, index-pack runs fsck_finish() which
checks if the target exists, but it doesn't, so it reports the failure.
One option is for fetch to do its own pass of checking all downloaded
objects once all packfiles have been downloaded, but that seems wasteful
as all trees would have to be re-inflated.
Another option is to do it within the connectivity check instead - so,
update rev-list and the object walking mechanism to be able to detect
.gitmodules in trees and fsck the target blob whenever such an entry
occurs. This has the advantage that there is no extra re-inflation,
although it might be strange to have object walking be able to fsck.
The simplest solution would be to just relax this - check the blob if it
exists, but if it doesn't, it's OK. Some things in favor of this
solution:
- This is something we already do in the partial clone case (although
it could be argued that in this case, we're already trusting the
server for far more than .gitmodules, so just because it's OK in the
partial clone case doesn't mean that it's OK in the regular case).
- Also, the commit message for this feature (from ed8b10f631 ("fsck: check
.gitmodules content", 2018-05-21)) gives a rationale of a newer
server being able to protect older clients.
- Servers using receive-pack (instead of fetch-pack) to obtain
objects would still be protected, since receive-pack still only
accepts one packfile at a time (and there are currently no plans
to expand this).
- Also, malicious .gitobjects files could still be crafted that pass
fsck checking - for example, by containing a URL (of another
server) that refers to a repo with a .gitobjects that would fail
fsck.
So I would rather go with just relaxing the check, but if consensus is
that we should still do it, I'll investigate doing it in the
connectivity check.
Someone at $DAYJOB noticed that if a .gitmodules-containing tree and the
.gitmodules blob itself are sent in 2 separate packfiles during a fetch
(which can happen when packfile URIs are used), transfer.fsckobjects
causes the fetch to fail. You can reproduce it as follows (as of the
time of writing):
$ git -c fetch.uriprotocols=https -c transfer.fsckobjects=true clone https://chromium.googlesource.com/chromiumos/codesearch
Cloning into 'codesearch'...
remote: Total 2242 (delta 0), reused 2242 (delta 0)
Receiving objects: 100% (2242/2242), 1.77 MiB | 4.62 MiB/s, done.
error: object 1f155c20935ee1154a813a814f03ef2b3976680f: gitmodulesMissing: unable to read .gitmodules blob
fatal: fsck error in pack objects
fatal: index-pack failed
This happens because the fsck part is currently being done in
index-pack, which operates on one pack at a time. When index-pack sees
the tree, it runs fsck on it (like any other object), and the fsck
subsystem remembers the .gitmodules target (specifically, in
gitmodules_found in fsck.c). Later, index-pack runs fsck_finish() which
checks if the target exists, but it doesn't, so it reports the failure.
One option is for fetch to do its own pass of checking all downloaded
objects once all packfiles have been downloaded, but that seems wasteful
as all trees would have to be re-inflated.
Another option is to do it within the connectivity check instead - so,
update rev-list and the object walking mechanism to be able to detect
.gitmodules in trees and fsck the target blob whenever such an entry
occurs. This has the advantage that there is no extra re-inflation,
although it might be strange to have object walking be able to fsck.
The simplest solution would be to just relax this - check the blob if it
exists, but if it doesn't, it's OK. Some things in favor of this
solution:
- This is something we already do in the partial clone case (although
it could be argued that in this case, we're already trusting the
server for far more than .gitmodules, so just because it's OK in the
partial clone case doesn't mean that it's OK in the regular case).
- Also, the commit message for this feature (from ed8b10f631 ("fsck: check
.gitmodules content", 2018-05-21)) gives a rationale of a newer
server being able to protect older clients.
- Servers using receive-pack (instead of fetch-pack) to obtain
objects would still be protected, since receive-pack still only
accepts one packfile at a time (and there are currently no plans
to expand this).
- Also, malicious .gitobjects files could still be crafted that pass
fsck checking - for example, by containing a URL (of another
server) that refers to a repo with a .gitobjects that would fail
fsck.
So I would rather go with just relaxing the check, but if consensus is
that we should still do it, I'll investigate doing it in the
connectivity check.
Would this still behave if the $DAYJOB's packfile-uri server support was
behaving as documented in packfile-uri.txt, or just because it has
outside-spec behavior?
I.e. the spec[1] says this:
This is the implementation: a feature, marked experimental, that
allows the server to be configured by one or more
`uploadpack.blobPackfileUri=<sha1> <uri>` entries. Whenever the list
of objects to be sent is assembled, all such blobs are excluded,
replaced with URIs. The client will download those URIs, expecting
them to each point to packfiles containing single blobs.
Which I can't see leaving an opening for more than packfile-uri being to
serve up packfiles which each contain a single blob.
In that case it seems to me we'd be OK (but I haven't tested), because
fsck_finish() will call read_object_file() which'll try to read that
"blob from the object store when it encounters the ".gitmodules" tree,
and because we'd have already downloaded the packfile with the blob
before moving onto the main dialog.
But as we discussed on-list before[2] this isn't the way packfile-uri
actually works in the wild. It's really just sending some arbitrary data
in a pack in that URI, with a server that knows what's in that pack and
will send the rest in such a way that everything ends up being
connected.
As far as I can tell the only reason this is called "packfile URI" and
behaves this way in git.git is because of the convenience of
intrumenting pack-objects.c with an "oidset excluded_by_config" to not
stream those blobs in a pack, but it isn't how the only (I'm pretty
sure) production server implementation in the wild behaves at all.
So *poke* about the reply I had in [3] late last year. I think the first
thing worth doing here is fixing the docs so they describe how this
works. You didn't get back on that (and I also forgot about it until
this thread), but it would be nice to know what you think about the
suggested prose there.
Re-reading it I'd add something like this to the spec:
A. That the config is called "uploadpack.blobPackfileUri" in git.git
has nothing to do with how this is expected to behave on the
wire. It's just to serve the narrow support pack-objects.c has for
crafting such a pack.
B. It's then called "packfile-uris" on the wire, nothing to do with
blobs. Just packs with a checksum that we'll validate. An older
versions of this spec said "[a] packfiles containing single blobs"
but it can be any combination of blob/tree/commit data.
C. A client is then expected to deal with any combination of data
ordered/sliced/split up etc. in any possible way from such a
combination of "packfile-uris" and PACK dialog, as long as the end
result is valid.
Except that the result of this discussion will perhaps be a more narrow
definition for "C".
1. https://github.com/git/git/blob/cd8402e0fd8cfc0ec9fb10e22ffb6aabd992eae1/Documentation/technical/packfile-uri.txt#L37-L41
2. https://lore.kernel.org/git/20201125190957.1113461-1-jonathantanmy@google.com/
3. https://lore.kernel.org/git/87tut5vghw.fsf@evledraar.gmail.com/
From: Jonathan Tan <hidden> Date: 2021-01-20 19:39:52
Clarify that, when the packfile-uri feature is used, the client should
not assume that the extra packfiles downloaded would only contain a
single blob, but support packfiles containing multiple objects of all
types.
Signed-off-by: Jonathan Tan <redacted>
---
Documentation/technical/packfile-uri.txt | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
@@ -37,8 +37,11 @@ at least so that we can test the client. This is the implementation: a feature, marked experimental, that allows the server to be configured by one or more `uploadpack.blobPackfileUri=<sha1> <uri>` entries. Whenever the list of objects to be sent is assembled, all such-blobs are excluded, replaced with URIs. The client will download those URIs,-expecting them to each point to packfiles containing single blobs.+blobs are excluded, replaced with URIs. As noted in "Future work" below, the+server can evolve in the future to support excluding other objects (or other+implementations of servers could be made that support excluding other objects)+without needing a protocol change, so clients should not expect that packfiles+downloaded in this way only contain single blobs. Client design -------------
From: Jonathan Tan <hidden> Date: 2021-01-20 19:42:07
Would this still behave if the $DAYJOB's packfile-uri server support was
behaving as documented in packfile-uri.txt, or just because it has
outside-spec behavior?
I.e. the spec[1] says this:
This is the implementation: a feature, marked experimental, that
allows the server to be configured by one or more
`uploadpack.blobPackfileUri=<sha1> <uri>` entries. Whenever the list
of objects to be sent is assembled, all such blobs are excluded,
replaced with URIs. The client will download those URIs, expecting
them to each point to packfiles containing single blobs.
Which I can't see leaving an opening for more than packfile-uri being to
serve up packfiles which each contain a single blob.
I meant to leave an opening by referring to this just as a Minimum
Viable Product and by explaining in Future Work that the protocol allows
evolution of (among other things) which objects the server sends through
a URI without any protocol changes.
But in any case, this will also happen even if we constrain ourselves to
excluding single blobs and sending them via other packfiles instead -
see below.
In that case it seems to me we'd be OK (but I haven't tested), because
fsck_finish() will call read_object_file() which'll try to read that
"blob from the object store when it encounters the ".gitmodules" tree,
and because we'd have already downloaded the packfile with the blob
before moving onto the main dialog.
We wouldn't be OK, actually. Suppose we have a separate packfile
containing only the ".gitmodules" blob - when we call fsck_finish(), we
would not have downloaded the other packfile yet. Git processes the
entire fetch response by piping the inline packfile (after demux) into
index-pack (which is the one that calls fsck_finish()) before it
downloads any of the other packfile(s).
But as we discussed on-list before[2] this isn't the way packfile-uri
actually works in the wild. It's really just sending some arbitrary data
in a pack in that URI, with a server that knows what's in that pack and
will send the rest in such a way that everything ends up being
connected.
As far as I can tell the only reason this is called "packfile URI" and
behaves this way in git.git is because of the convenience of
intrumenting pack-objects.c with an "oidset excluded_by_config" to not
stream those blobs in a pack, but it isn't how the only (I'm pretty
sure) production server implementation in the wild behaves at all.
I don't know if this is the only production server implementation, but
yes, this particular one (googlesource.com) can put objects of multiple
types in the other packfile, not only a single blob. There is some JGit
code here [1] that can send a URI corresponding to a "CachedPack" (which
may contain all objects, not only blobs) if that pack is also available
through a URI.
[1] https://gerrit.googlesource.com/jgit/+/a004820858b54d18c6f72fc94dc33bce8b606d66
So *poke* about the reply I had in [3] late last year. I think the first
thing worth doing here is fixing the docs so they describe how this
works. You didn't get back on that (and I also forgot about it until
this thread), but it would be nice to know what you think about the
suggested prose there.
Rereading that, the issue is that uploadpack.blobPackfileUri is indeed
how the current Git server handles it - it excludes a blob and sends a
URI instead. The client is not supposed to see how the server has
configured it, and should not be constrained by the fact that the server
that is being shipped with it only excludes single blobs.
Re-reading it I'd add something like this to the spec:
A. That the config is called "uploadpack.blobPackfileUri" in git.git
has nothing to do with how this is expected to behave on the
wire. It's just to serve the narrow support pack-objects.c has for
crafting such a pack.
Yes, that's true.
B. It's then called "packfile-uris" on the wire, nothing to do with
blobs. Just packs with a checksum that we'll validate. An older
versions of this spec said "[a] packfiles containing single blobs"
but it can be any combination of blob/tree/commit data.
Yes, we can delete that line.
C. A client is then expected to deal with any combination of data
ordered/sliced/split up etc. in any possible way from such a
combination of "packfile-uris" and PACK dialog, as long as the end
result is valid.
Except that the result of this discussion will perhaps be a more narrow
definition for "C".
Yes. I think all these can be done just by changing the last sentence in
"Server design" - I'll send a patch.
From: Jonathan Tan <hidden> Date: 2021-01-24 02:35:40
This patch set resolves the .gitmodules-and-tree-in-separate-packfiles
issue I mentioned in [1] by having index-pack print out all dangling
.gitmodules (instead of returning with an error code) and then teaching
fetch-pack to read those and run its own fsck checks after all
index-pack invocations are complete.
As part of this, index-pack has to output (1) the hash that goes into
the name of the .pack/.idx file and (2) the hashes of all dangling
.gitmodules. I just had (2) come after (1). If anyone has a better idea,
I'm interested.
I also discovered a bug in that different index-pack arguments were used
when processing the inline packfile and when processing the ones
referenced by URIs. Patch 1-3 fixes that bug by passing the arguments to
use as a space-separated URL-encoded list. (URL-encoded so that we can
have spaces in the arguments.) Again, if anyone has a better idea, I'm
interested. It is only in patch 4 that we have the dangling .gitmodules
fix.
[1] https://lore.kernel.org/git/20210115234300.350442-1-jonathantanmy@google.com/
Jonathan Tan (4):
http: allow custom index-pack args
http-fetch: allow custom index-pack args
fetch-pack: with packfile URIs, use index-pack arg
fetch-pack: print and use dangling .gitmodules
Documentation/git-http-fetch.txt | 9 ++-
Documentation/git-index-pack.txt | 7 +-
builtin/index-pack.c | 9 ++-
builtin/receive-pack.c | 2 +-
fetch-pack.c | 106 ++++++++++++++++++++++++++-----
fsck.c | 16 +++--
fsck.h | 8 +++
http-fetch.c | 35 +++++++++-
http.c | 15 +++--
http.h | 10 +--
pack-write.c | 8 ++-
pack.h | 2 +-
t/t5550-http-fetch-dumb.sh | 3 +-
t/t5702-protocol-v2.sh | 47 ++++++++++++++
14 files changed, 232 insertions(+), 45 deletions(-)
--
2.30.0.280.ga3ce27912f-goog
From: Jonathan Tan <hidden> Date: 2021-01-24 02:35:40
Currently, when fetching, packfiles referenced by URIs are run through
index-pack without any arguments other than --stdin and --keep, no
matter what arguments are used for the packfile that is inline in the
fetch response. As a preparation for ensuring that all packs (whether
inline or not) use the same index-pack arguments, teach the http
subsystem to allow custom index-pack arguments.
http-fetch has been updated to use the new API. For now, it passes
--keep alone instead of --keep with a process ID, but this is only
temporary because http-fetch itself will be taught to accept index-pack
parameters (instead of using a hardcoded constant) in a subsequent
commit.
Signed-off-by: Jonathan Tan <redacted>
---
http-fetch.c | 6 +++++-
http.c | 15 ++++++++-------
http.h | 10 +++++-----
3 files changed, 18 insertions(+), 13 deletions(-)
From: Jonathan Tan <hidden> Date: 2021-01-24 02:35:40
This is the next step in teaching fetch-pack to pass its index-pack
arguments when processing packfiles referenced by URIs.
The "--keep" in fetch-pack.c will be replaced with a full message in a
subsequent commit.
Signed-off-by: Jonathan Tan <redacted>
---
Documentation/git-http-fetch.txt | 9 ++++++--
fetch-pack.c | 1 +
http-fetch.c | 35 +++++++++++++++++++++++++++-----
t/t5550-http-fetch-dumb.sh | 3 ++-
4 files changed, 40 insertions(+), 8 deletions(-)
@@ -41,11 +41,16 @@ commit-id:: <commit-id>['\t'<filename-as-in--w>] --packfile=<hash>::- Instead of a commit id on the command line (which is not expected in+ For internal use only. Instead of a commit id on the command line (which is not expected in this case), 'git http-fetch' fetches the packfile directly at the given URL and uses index-pack to generate corresponding .idx and .keep files. The hash is used to determine the name of the temporary file and is- arbitrary. The output of index-pack is printed to stdout.+ arbitrary. The output of index-pack is printed to stdout. Requires+ --index-pack-args.++--index-pack-args=<args>::+ For internal use only. The command to run on the contents of the+ downloaded pack. Arguments are URL-encoded separated by spaces. --recover:: Verify that everything reachable from target is fetched. Used after
@@ -43,11 +44,9 @@ static int fetch_using_walker(const char *raw_url, int get_verbosely,returnrc;}-staticconstchar*index_pack_args[]=-{"index-pack","--stdin","--keep",NULL};-staticvoidfetch_single_packfile(structobject_id*packfile_hash,-constchar*url){+constchar*url,+constchar**index_pack_args){structhttp_pack_request*preq;structslot_resultsresults;intret;
@@ -90,6 +89,7 @@ int cmd_main(int argc, const char **argv)intpackfile=0;intnongit;structobject_idpackfile_hash;+constchar*index_pack_args=NULL;setup_git_directory_gently(&nongit);
@@ -116,6 +116,8 @@ int cmd_main(int argc, const char **argv)packfile=1;if(parse_oid_hex(p,&packfile_hash,&end)||*end)die(_("argument to --packfile must be a valid hash (got '%s')"),p);+}elseif(skip_prefix(argv[arg],"--index-pack-args=",&p)){+index_pack_args=p;}arg++;}
@@ -128,10 +130,33 @@ int cmd_main(int argc, const char **argv)git_config(git_default_config,NULL);if(packfile){-fetch_single_packfile(&packfile_hash,argv[arg]);+structstrvecencoded=STRVEC_INIT;+char**raw;+inti;++if(!index_pack_args)+die(_("--packfile requires --index-pack-args"));++strvec_split(&encoded,index_pack_args);++CALLOC_ARRAY(raw,encoded.nr+1);+for(i=0;i<encoded.nr;i++)+raw[i]=url_percent_decode(encoded.v[i]);++fetch_single_packfile(&packfile_hash,argv[arg],+(constchar**)raw);++for(i=0;i<encoded.nr;i++)+free(raw[i]);+free(raw);+strvec_clear(&encoded);+return0;}+if(index_pack_args)+die(_("--index-pack-args can only be used with --packfile"));+if(commits_on_stdin){commits=walker_targets_stdin(&commit_id,&write_ref);}else{
From: Jonathan Tan <hidden> Date: 2021-01-24 02:35:40
Unify the index-pack arguments used when processing the inline pack and
when downloading packfiles referenced by URIs. This is done by teaching
get_pack() to also store the index-pack arguments whenever at least one
packfile URI is given, and then when processing the packfile URI(s),
using the stored arguments.
Signed-off-by: Jonathan Tan <redacted>
---
fetch-pack.c | 35 ++++++++++++++++++++++++++---------
1 file changed, 26 insertions(+), 9 deletions(-)
From: Jonathan Tan <hidden> Date: 2021-01-24 02:35:40
Teach index-pack to print dangling .gitmodules links after its "keep" or
"pack" line instead of declaring an error, and teach fetch-pack to check
such lines printed.
This allows the tree side of the .gitmodules link to be in one packfile
and the blob side to be in another without failing the fsck check,
because it is now fetch-pack which checks such objects after all
packfiles have been downloaded and indexed (and not index-pack on an
individual packfile, as it is before this commit).
Signed-off-by: Jonathan Tan <redacted>
---
Documentation/git-index-pack.txt | 7 ++-
builtin/index-pack.c | 9 +++-
builtin/receive-pack.c | 2 +-
fetch-pack.c | 78 +++++++++++++++++++++++++++-----
fsck.c | 16 +++++--
fsck.h | 8 ++++
pack-write.c | 8 +++-
pack.h | 2 +-
t/t5702-protocol-v2.sh | 47 +++++++++++++++++++
9 files changed, 155 insertions(+), 22 deletions(-)
@@ -78,7 +78,12 @@ OPTIONS Die if the pack contains broken links. For internal use only. --fsck-objects::- Die if the pack contains broken objects. For internal use only.+ For internal use only.+++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"). --threads=<n>:: Specifies the number of threads to spawn when resolving
@@ -936,6 +936,53 @@ test_expect_success 'packfile-uri with transfer.fsckobjects fails on bad object'test_i18ngrep"invalid author/committer line - missing email"error'+test_expect_success'packfile-uri with transfer.fsckobjects succeeds when .gitmodules is separate from tree''+P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent"&&+rm-rf"$P"http_child&&++gitinit"$P"&&+git-C"$P"config"uploadpack.allowsidebandall""true"&&++echo"[submodule libfoo]">"$P/.gitmodules"&&+echo"path = include/foo">>"$P/.gitmodules"&&+echo"url = git://example.com/git/lib.git">>"$P/.gitmodules"&&+git-C"$P"add.gitmodules&&+git-C"$P"commit-mx&&++configure_exclusion"$P".gitmodules>h&&++sane_unsetGIT_TEST_SIDEBAND_ALL&&+git-cprotocol.version=2-ctransfer.fsckobjects=1\+-cfetch.uriprotocols=http,https\+clone"$HTTPD_URL/smart/http_parent"http_child&&++# Ensure that there are exactly 4 files (2 .pack and 2 .idx).+lshttp_child/.git/objects/pack/*>filelist&&+test_line_count=4filelist+'++test_expect_success'packfile-uri with transfer.fsckobjects fails when .gitmodules separate from tree is invalid''+P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent"&&+rm-rf"$P"http_childerr&&++gitinit"$P"&&+git-C"$P"config"uploadpack.allowsidebandall""true"&&++echo"[submodule \"..\"]">"$P/.gitmodules"&&+echo"path = include/foo">>"$P/.gitmodules"&&+echo"url = git://example.com/git/lib.git">>"$P/.gitmodules"&&+git-C"$P"add.gitmodules&&+git-C"$P"commit-mx&&++configure_exclusion"$P".gitmodules>h&&++sane_unsetGIT_TEST_SIDEBAND_ALL&&+test_must_failgit-cprotocol.version=2-ctransfer.fsckobjects=1\+-cfetch.uriprotocols=http,https\+clone"$HTTPD_URL/smart/http_parent"http_child2>err&&+test_i18ngrep"disallowed submodule name"err+'+# DO NOT add non-httpd-specific tests here, because the last part of this# test script is only executed when httpd is available and enabled.
--packfile=<hash>::
- Instead of a commit id on the command line (which is not expected in
+ For internal use only. Instead of a commit id on the command line (which is not expected in
Leaves the rest at ~79 and this long line at ~100. Perhaps a follow-up
change to re-word-wrap would be in order?
In fsck.c we only use this variable to insert into it, or in fsck_blob()
to do the actual check, but then we either abort early if we've found
it, or right after that:
if (object_on_skiplist(options, oid))
return 0;
So (along with comments I have below...) you could just use the existing
"skiplist" option instead, no?
quoted hunk
int fsck_finish(struct fsck_options *options)
{
int ret = 0;
@@ -1262,10 +1267,13 @@ int fsck_finish(struct fsck_options *options) if (!buf) { if (is_promisor_object(oid)) continue;- ret |= report(options,- oid, OBJ_BLOB,- FSCK_MSG_GITMODULES_MISSING,- "unable to read .gitmodules blob");+ if (options->print_dangling_gitmodules)+ printf("%s\n", oid_to_hex(oid));+ else+ ret |= report(options,+ oid, OBJ_BLOB,+ FSCK_MSG_GITMODULES_MISSING,+ "unable to read .gitmodules blob"); continue; }
This whole thing seems just like the bad path I took in earlier rounds
of my in-flight mktag series. You don't need this new custom API. You
just setup an error handler for your fsck which ignores / prints / logs
/ whatever the OIDs you want if you get a FSCK_MSG_GITMODULES_MISSING
error, which you then "return 0" on.
If you don't have FSCK_MSG_GITMODULES_MISSING punt and call
fsck_error_function().
--fsck-objects::
- Die if the pack contains broken objects. For internal use only.
+ For internal use only.
++
+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").
[I should have waited a bit and sent one E-Mail]
Is this really generally usable as an IPC mechanism, what if we need
another set of OIDs we care about? Shouldn't it at least be hidden
behind some option so you don't get a deluge of output from index-pack
if you're not in this packfile-uri mode?
But, along with my other E-Mail...
Doesn't this IPC mechanism already exist in the form of fsck.skipList?
See my 1f3299fda9 (fsck: make fsck_config() re-usable, 2021-01-05) on
"next". I.e. as noted in my just-sent-E-Mail you could probably just
re-use skiplist as-is.
Or if not it seems to me that this whole IPC mechanism would be better
done with a tempfile and passing it along like we already pass the
fsck.skipList between these processes.
I doubt it's going to be large enough to matter, we could just put it in
.git/ somewhere, like we put gc.log etc (but created with a mktemp()
name...).
Or if we want to keep the "print <list> | process" model we can refactor
the existing fsck IPC noted in 1f3299fda9 a bit, so e.g. you pass some
version of "lines prefixed with "fsck-skiplist: " go into list xyz via a
command-line option. And then existing option(s) and your potential new
list (which as noted, I think is probably redundant to the skiplist) can
use it.
From: Jonathan Tan <hidden> Date: 2021-01-28 00:33:58
On Sun, Jan 24 2021, Jonathan Tan wrote:
quoted
--packfile=<hash>::
- Instead of a commit id on the command line (which is not expected in
+ For internal use only. Instead of a commit id on the command line (which is not expected in
Leaves the rest at ~79 and this long line at ~100. Perhaps a follow-up
change to re-word-wrap would be in order?
Hmm...I'll split that onto two lines then. I don't think it's worth the
extra commit in history to have it exactly wrapped right, so I'll forgo
the follow-up change for now.
In fsck.c we only use this variable to insert into it, or in fsck_blob()
to do the actual check, but then we either abort early if we've found
it, or right after that:
By "this variable", do you mean gitmodules_found? fsck_finish() consumes
it.
if (object_on_skiplist(options, oid))
return 0;
So (along with comments I have below...) you could just use the existing
"skiplist" option instead, no?
This whole thing seems just like the bad path I took in earlier rounds
of my in-flight mktag series. You don't need this new custom API. You
just setup an error handler for your fsck which ignores / prints / logs
/ whatever the OIDs you want if you get a FSCK_MSG_GITMODULES_MISSING
error, which you then "return 0" on.
If you don't have FSCK_MSG_GITMODULES_MISSING punt and call
fsck_error_function().
I tried that first, and the issue is that IDs like
FSCK_MSG_GITMODULES_MISSING are internal to fsck.c. As for whether we
should start exposing the IDs publicly, I think we should wait until a
few new cases like this come up, so that we more fully understand the
requirements first.
From: Jonathan Tan <hidden> Date: 2021-01-28 01:18:18
On Sun, Jan 24 2021, Jonathan Tan wrote:
quoted
--fsck-objects::
- Die if the pack contains broken objects. For internal use only.
+ For internal use only.
++
+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").
[I should have waited a bit and sent one E-Mail]
Is this really generally usable as an IPC mechanism, what if we need
another set of OIDs we care about? Shouldn't it at least be hidden
behind some option so you don't get a deluge of output from index-pack
if you're not in this packfile-uri mode?
Doesn't this IPC mechanism already exist in the form of fsck.skipList?
See my 1f3299fda9 (fsck: make fsck_config() re-usable, 2021-01-05) on
"next". I.e. as noted in my just-sent-E-Mail you could probably just
re-use skiplist as-is.
I'm not sure how fsck.skipList could be used here. Before running
fsck_finish() for the first time, we don't know which .gitmodules are
missing and which are not. And when running fsck_finish() for the second
time, we definitely do not want to skip any blobs.
Or if not it seems to me that this whole IPC mechanism would be better
done with a tempfile and passing it along like we already pass the
fsck.skipList between these processes.
I doubt it's going to be large enough to matter, we could just put it in
.git/ somewhere, like we put gc.log etc (but created with a mktemp()
name...).
Or if we want to keep the "print <list> | process" model we can refactor
the existing fsck IPC noted in 1f3299fda9 a bit, so e.g. you pass some
version of "lines prefixed with "fsck-skiplist: " go into list xyz via a
command-line option. And then existing option(s) and your potential new
list (which as noted, I think is probably redundant to the skiplist) can
use it.
I think using stdout is superior to using a tempfile - we don't have to
worry about interrupted invocations, for example.
What do you mean by "the existing fsck IPC noted in 1f3299fda9"? If you
mean the ability to pass a list of OIDs, for example using "-c
fsck.skipList=filename.txt", I'm not sure that it solves anything.
Firstly, I don't think that the skipList is useful here (as I said
earlier). And secondly, I don't think that OID input is the issue -
right now, the design is a process (index-pack, calling fsck_finish())
writing to its output which is then picked up by the calling process
(fetch-pack). We are not sending the dangling .gitmodules through stdin
anywhere.
This is the next step in teaching fetch-pack to pass its index-pack
arguments when processing packfiles referenced by URIs.
The "--keep" in fetch-pack.c will be replaced with a full message in a
subsequent commit.
Signed-off-by: Jonathan Tan <redacted>
---
Documentation/git-http-fetch.txt | 9 ++++++--
fetch-pack.c | 1 +
http-fetch.c | 35 +++++++++++++++++++++++++++-----
t/t5550-http-fetch-dumb.sh | 3 ++-
4 files changed, 40 insertions(+), 8 deletions(-)
@@ -41,11 +41,16 @@ commit-id:: <commit-id>['\t'<filename-as-in--w>] --packfile=<hash>::- Instead of a commit id on the command line (which is not expected in+ For internal use only. Instead of a commit id on the command line (which is not expected in this case), 'git http-fetch' fetches the packfile directly at the given URL and uses index-pack to generate corresponding .idx and .keep files. The hash is used to determine the name of the temporary file and is- arbitrary. The output of index-pack is printed to stdout.+ arbitrary. The output of index-pack is printed to stdout. Requires+ --index-pack-args.++--index-pack-args=<args>::+ For internal use only. The command to run on the contents of the+ downloaded pack. Arguments are URL-encoded separated by spaces.
I'm a bit skeptical of using URL encoding to work around embedded
spaces. I believe in Emily's config-based hooks series, she wrote an
argument parser to pull repeated arguments into a strvec, could you do
something like that here?
I'm sympathetic to the idea that since this is an internal-only flag, we
can be a bit weird with the argument format, though.
quoted hunk
--recover::
Verify that everything reachable from target is fetched. Used after
@@ -43,11 +44,9 @@ static int fetch_using_walker(const char *raw_url, int get_verbosely,returnrc;}-staticconstchar*index_pack_args[]=-{"index-pack","--stdin","--keep",NULL};-staticvoidfetch_single_packfile(structobject_id*packfile_hash,-constchar*url){+constchar*url,+constchar**index_pack_args){structhttp_pack_request*preq;structslot_resultsresults;intret;
@@ -90,6 +89,7 @@ int cmd_main(int argc, const char **argv)intpackfile=0;intnongit;structobject_idpackfile_hash;+constchar*index_pack_args=NULL;setup_git_directory_gently(&nongit);
@@ -116,6 +116,8 @@ int cmd_main(int argc, const char **argv)packfile=1;if(parse_oid_hex(p,&packfile_hash,&end)||*end)die(_("argument to --packfile must be a valid hash (got '%s')"),p);+}elseif(skip_prefix(argv[arg],"--index-pack-args=",&p)){+index_pack_args=p;}arg++;}
@@ -128,10 +130,33 @@ int cmd_main(int argc, const char **argv)git_config(git_default_config,NULL);if(packfile){-fetch_single_packfile(&packfile_hash,argv[arg]);+structstrvecencoded=STRVEC_INIT;+char**raw;+inti;++if(!index_pack_args)+die(_("--packfile requires --index-pack-args"));++strvec_split(&encoded,index_pack_args);++CALLOC_ARRAY(raw,encoded.nr+1);+for(i=0;i<encoded.nr;i++)+raw[i]=url_percent_decode(encoded.v[i]);++fetch_single_packfile(&packfile_hash,argv[arg],+(constchar**)raw);++for(i=0;i<encoded.nr;i++)+free(raw[i]);+free(raw);+strvec_clear(&encoded);+return0;}+if(index_pack_args)+die(_("--index-pack-args can only be used with --packfile"));+if(commits_on_stdin){commits=walker_targets_stdin(&commit_id,&write_ref);}else{
In fsck.c we only use this variable to insert into it, or in fsck_blob()
to do the actual check, but then we either abort early if we've found
it, or right after that:
By "this variable", do you mean gitmodules_found? fsck_finish() consumes
it.
Yes, consumes it to emit errors with report(), no?
quoted
if (object_on_skiplist(options, oid))
return 0;
So (along with comments I have below...) you could just use the existing
"skiplist" option instead, no?
This whole thing seems just like the bad path I took in earlier rounds
of my in-flight mktag series. You don't need this new custom API. You
just setup an error handler for your fsck which ignores / prints / logs
/ whatever the OIDs you want if you get a FSCK_MSG_GITMODULES_MISSING
error, which you then "return 0" on.
If you don't have FSCK_MSG_GITMODULES_MISSING punt and call
fsck_error_function().
I tried that first, and the issue is that IDs like
FSCK_MSG_GITMODULES_MISSING are internal to fsck.c. As for whether we
should start exposing the IDs publicly, I think we should wait until a
few new cases like this come up, so that we more fully understand the
requirements first.
The requirement is that you want the objects ids we'd otherwise error
about in fsck_finish(). Yeah we don't pass the "fsck_msg_id" down in the
"report()" function, but you can reliably strstr() it out of the
message. We document & hard rely on that already, since it's also a
config key.
But yeah, we could just change the report function to pass down the id
and move the relevant macros from fsck.c to fsck.h. I think that would
be a smaller change conceptually than a special-case flag in
fsck_options for something we could otherwise do with the error
reporting.
--fsck-objects::
- Die if the pack contains broken objects. For internal use only.
+ For internal use only.
++
+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").
[I should have waited a bit and sent one E-Mail]
Is this really generally usable as an IPC mechanism, what if we need
another set of OIDs we care about? Shouldn't it at least be hidden
behind some option so you don't get a deluge of output from index-pack
if you're not in this packfile-uri mode?
Doesn't this IPC mechanism already exist in the form of fsck.skipList?
See my 1f3299fda9 (fsck: make fsck_config() re-usable, 2021-01-05) on
"next". I.e. as noted in my just-sent-E-Mail you could probably just
re-use skiplist as-is.
I'm not sure how fsck.skipList could be used here. Before running
fsck_finish() for the first time, we don't know which .gitmodules are
missing and which are not. And when running fsck_finish() for the second
time, we definitely do not want to skip any blobs.
quoted
Or if not it seems to me that this whole IPC mechanism would be better
done with a tempfile and passing it along like we already pass the
fsck.skipList between these processes.
I doubt it's going to be large enough to matter, we could just put it in
.git/ somewhere, like we put gc.log etc (but created with a mktemp()
name...).
Or if we want to keep the "print <list> | process" model we can refactor
the existing fsck IPC noted in 1f3299fda9 a bit, so e.g. you pass some
version of "lines prefixed with "fsck-skiplist: " go into list xyz via a
command-line option. And then existing option(s) and your potential new
list (which as noted, I think is probably redundant to the skiplist) can
use it.
I think using stdout is superior to using a tempfile - we don't have to
worry about interrupted invocations, for example.
What do you mean by "the existing fsck IPC noted in 1f3299fda9"? If you
mean the ability to pass a list of OIDs, for example using "-c
fsck.skipList=filename.txt", I'm not sure that it solves anything.
Firstly, I don't think that the skipList is useful here (as I said
earlier). And secondly, I don't think that OID input is the issue -
right now, the design is a process (index-pack, calling fsck_finish())
writing to its output which is then picked up by the calling process
(fetch-pack). We are not sending the dangling .gitmodules through stdin
anywhere.
Sorry for being unclear here. I don't think (honestly I don't remember,
it's been almost a month) that I meant to you should use the skipList.
Looking at that code again we use object_on_skiplist() to do an early
punt in report(), but also fsck_blob(), presumably you never want the
latter, and that early punting wouldn't be needed if your report()
function intercepted the modules blob id for stashing it away / later
reporting / whatever.
So yeah, I'm 99% sure now that's not what I meant :)
What I meant with:
Or if we want to keep the "print <list> | process"[...]
Is that we have an existing ad-hoc IPC model for these commands in
passing along the skipList, which is made more complex because sometimes
the initial process reads the file, sometimes it passes it along as-is
to the child.
And then there's this patch that passes OIDs too, but through a
different mechanism.
I was suggesting that perhaps it made more sense to refactor both so
they could use the same mechanism, because we're potentially passing two
lists of OIDs between the two. Just one goes via line-at-a-time in the
output, the other via a config option on the command-line.
Jonathan Tan pointed out that the fsck error_func doesn't pass you the
ID of the fsck failure in [1]. This series improves the API so it
does, and moves the gitmodules_{found,done} variables into the
fsck_options struct.
The result is that instead of the "print_dangling_gitmodules" member
in that series we can just implement that with the diff at the end of
this cover letter (goes on top of a merge of this series & "seen"),
and without any changes to fsck_finish().
This conflicts with other in-flight fsck changes but the conflict is
rather trivial. Jeff King has another concurrent series to add a
couple of new fsck checks, those need to be moved to fsck.h, and
there's another trivial conflict in 2 hunks due to the
gitmodules_{found,done} move.
1. https://lore.kernel.org/git/87blcja2ha.fsf@evledraar.gmail.com/
Ævar Arnfjörð Bjarmason (14):
fsck.h: indent arguments to of fsck_set_msg_type
fsck.h: use use "enum object_type" instead of "int"
fsck.c: rename variables in fsck_set_msg_type() for less confusion
fsck.c: move definition of msg_id into append_msg_id()
fsck.c: rename remaining fsck_msg_id "id" to "msg_id"
fsck.h: move FSCK_{FATAL,INFO,ERROR,WARN,IGNORE} into an enum
fsck.c: call parse_msg_type() early in fsck_set_msg_type()
fsck.c: undefine temporary STR macro after use
fsck.c: give "FOREACH_MSG_ID" a more specific name
fsck.[ch]: move FOREACH_FSCK_MSG_ID & fsck_msg_id from *.c to *.h
fsck.c: pass along the fsck_msg_id in the fsck_error callback
fsck.c: add an fsck_set_msg_type() API that takes enums
fsck.h: update FSCK_OPTIONS_* for object_name
fsck.c: move gitmodules_{found,done} into fsck_options
builtin/fsck.c | 7 +-
builtin/index-pack.c | 3 +-
builtin/mktag.c | 7 +-
builtin/unpack-objects.c | 3 +-
fsck.c | 160 ++++++++++++---------------------------
fsck.h | 98 +++++++++++++++++++++---
6 files changed, 152 insertions(+), 126 deletions(-)
--
Change the fsck_walk_func to use an "enum object_type" instead of an
"int" type. The types are compatible, and ever since this was added in
355885d5315 (add generic, type aware object chain walker, 2008-02-25)
we've used entries from object_type (OBJ_BLOB etc.).
So this doesn't really change anything as far as the generated code is
concerned, it just gives the compiler more information and makes this
easier to read.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/fsck.c | 3 ++-
builtin/index-pack.c | 3 ++-
builtin/unpack-objects.c | 3 ++-
fsck.h | 3 ++-
4 files changed, 8 insertions(+), 4 deletions(-)
@@ -23,7 +23,8 @@ int is_valid_msg_type(const char *msg_id, const char *msg_type);*<0errorsignaledandabort*>0errorsignaledanddonotabort*/-typedefint(*fsck_walk_func)(structobject*obj,inttype,void*data,structfsck_options*options);+typedefint(*fsck_walk_func)(structobject*obj,enumobject_typeobject_type,+void*data,structfsck_options*options);/* callback for fsck_object, type is FSCK_ERROR or FSCK_WARN */typedefint(*fsck_error)(structfsck_options*o,
Rename variables in a function added in 0282f4dced0 (fsck: offer a
function to demote fsck errors to warnings, 2015-06-22).
It was needlessly confusing that it took a "msg_type" argument, but
then later declared another "msg_type" of a different type.
Let's rename that to "tmp", and rename "id" to "msg_id" and "msg_id"
to "msg_id_str" etc. This will make a follow-up change smaller.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
fsck.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
Refactor code added in 71ab8fa840f (fsck: report the ID of the
error/warning, 2015-06-22) to resolve the msg_id to a string in the
function that wants it, instead of doing it in report().
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
fsck.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
There's no reason to defer the calling of parse_msg_type() until after
we've checked if the "id < 0". This is not a hot codepath, and
parse_msg_type() itself may die on invalid input.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
fsck.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
Rename the remaining variables of type fsck_msg_id from "id" to
"msg_id". This change is relatively small, and is worth the churn for
a later change where we have different id's in the "report" function.
---
fsck.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
In f417eed8cde (fsck: provide a function to parse fsck message IDs,
2015-06-22) the "STR" macro was introduced, but that short macro name
was not undefined after use as was done earlier in the same series for
the MSG_ID macro in c99ba492f1c (fsck: introduce identifiers for fsck
messages, 2015-06-22).
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
fsck.c | 1 +
1 file changed, 1 insertion(+)
Move the FOREACH_FSCK_MSG_ID macro and the fsck_msg_id enum it helps
define from fsck.c to fsck.h. This is in preparation for having
non-static functions take the fsck_msg_id as an argument.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
fsck.c | 66 ---------------------------------------------------------
fsck.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 67 insertions(+), 66 deletions(-)
Change code I added in acf9de4c94e (mktag: use fsck instead of custom
verify_tag(), 2021-01-05) to make use of a new API function that takes
the fsck_msg_{id,type} types, instead of arbitrary strings that
we'll (hopefully) parse into those types.
At the time that the fsck_set_msg_type() API was introduced in
0282f4dced0 (fsck: offer a function to demote fsck errors to warnings,
2015-06-22) it was only intended to be used to parse user-supplied
data.
For things that are purely internal to the C code it makes sense to
have the compiler check these arguments, and to skip the sanity
checking of the data in fsck_set_msg_type() which is redundant to
checks we get from the compiler.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/mktag.c | 3 ++-
fsck.c | 27 +++++++++++++++++----------
fsck.h | 3 +++
3 files changed, 22 insertions(+), 11 deletions(-)
@@ -93,7 +93,8 @@ int cmd_mktag(int argc, const char **argv, const char *prefix)die_errno(_("could not read from stdin"));fsck_options.error_func=mktag_fsck_error_func;-fsck_set_msg_type(&fsck_options,"extraheaderentry","warn");+fsck_set_msg_type_from_ids(&fsck_options,FSCK_MSG_EXTRA_HEADER_ENTRY,+FSCK_WARN);/* config might set fsck.extraHeaderEntry=* again */git_config(mktag_config,NULL);if(fsck_tag_standalone(NULL,buf.buf,buf.len,&fsck_options,
Change the fsck_error callback to also pass along the
fsck_msg_id. Before this change the only way to get the message id was
to parse it back out of the "message".
Let's pass it down explicitly for the benefit of callers that might
want to use it, as discussed in [1].
Passing the msg_type is now redundant, as you can always get it back
from the msg_id, but I'm not changing that convention. It's really
common to need the msg_type, and the report() function itself (which
calls "fsck_error") needs to call fsck_msg_type() to discover
it. Let's not needlessly re-do that work in the user callback.
1. https://lore.kernel.org/git/87blcja2ha.fsf@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/fsck.c | 4 +++-
builtin/mktag.c | 1 +
fsck.c | 6 ++++--
fsck.h | 6 ++++--
4 files changed, 12 insertions(+), 5 deletions(-)
@@ -99,11 +99,13 @@ typedef int (*fsck_walk_func)(struct object *obj, enum object_type object_type,/* callback for fsck_object, type is FSCK_ERROR or FSCK_WARN */typedefint(*fsck_error)(structfsck_options*o,conststructobject_id*oid,enumobject_typeobject_type,-enumfsck_msg_typemsg_type,constchar*message);+enumfsck_msg_typemsg_type,enumfsck_msg_idmsg_id,+constchar*message);intfsck_error_function(structfsck_options*o,conststructobject_id*oid,enumobject_typeobject_type,-enumfsck_msg_typemsg_type,constchar*message);+enumfsck_msg_typemsg_type,enumfsck_msg_idmsg_id,+constchar*message);structfsck_options{fsck_walk_funcwalk;
@@ -29,17 +32,17 @@ typedef int (*fsck_walk_func)(struct object *obj, enum object_type object_type,/* callback for fsck_object, type is FSCK_ERROR or FSCK_WARN */typedefint(*fsck_error)(structfsck_options*o,conststructobject_id*oid,enumobject_typeobject_type,-intmsg_type,constchar*message);+enumfsck_msg_typemsg_type,constchar*message);intfsck_error_function(structfsck_options*o,conststructobject_id*oid,enumobject_typeobject_type,-intmsg_type,constchar*message);+enumfsck_msg_typemsg_type,constchar*message);structfsck_options{fsck_walk_funcwalk;fsck_errorerror_func;unsignedstrict:1;-int*msg_type;+enumfsck_msg_type*msg_type;structoidsetskiplist;kh_oid_map_t*object_names;};
Add the object_name member to the initialization macro. This was
omitted in 7b35efd734e (fsck_walk(): optionally name objects on the
go, 2016-07-17) when the field was added.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
fsck.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Move the gitmodules_{found,done} static variables added in
159e7b080bf (fsck: detect gitmodules files, 2018-05-02) into the
fsck_options struct. It makes sense to keep all the context in the
same place.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
fsck.c | 19 ++++++++-----------
fsck.h | 6 ++++--
2 files changed, 12 insertions(+), 13 deletions(-)
Rename the FOREACH_MSG_ID macro to FOREACH_FSCK_MSG_ID in preparation
for moving it over to fsck.h. It's good convention to name macros
in *.h files in such a way as to clearly not clash with any other
names in other files.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
fsck.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: Jonathan Tan <hidden> Date: 2021-02-17 20:06:07
quoted
I tried that first, and the issue is that IDs like
FSCK_MSG_GITMODULES_MISSING are internal to fsck.c. As for whether we
should start exposing the IDs publicly, I think we should wait until a
few new cases like this come up, so that we more fully understand the
requirements first.
The requirement is that you want the objects ids we'd otherwise error
about in fsck_finish(). Yeah we don't pass the "fsck_msg_id" down in the
"report()" function, but you can reliably strstr() it out of the
message.
We can't strstr() because of false positives (if, e.g. there is a
submodule name that contains the string we're looking for), but looking
at report() in fsck.c, the message ID is the very first thing appended,
so I think we can use starts_with().
We document & hard rely on that already, since it's also a
config key.
Ah, good point.
But yeah, we could just change the report function to pass down the id
and move the relevant macros from fsck.c to fsck.h. I think that would
be a smaller change conceptually than a special-case flag in
fsck_options for something we could otherwise do with the error
reporting.
I agree - I thought this wouldn't be possible, but like you said, we can
reliably make use of the string in report() (or pass the ID, like your
patch set [1] does) so we should do this.
What would be the best way to proceed, now that we have at least 2 patch
sets (mine and yours) in play? I was thinking that I should update my
one to use the string reported in report() (with starts_with()), so that
both our patch sets can be reviewed and merged in parallel, and after
that, update the fsck code to use the ID instead of the string.
[1] https://lore.kernel.org/git/87blcja2ha.fsf@evledraar.gmail.com/
From: Jonathan Tan <hidden> Date: 2021-02-17 20:10:52
Sorry for being unclear here. I don't think (honestly I don't remember,
it's been almost a month) that I meant to you should use the skipList.
Looking at that code again we use object_on_skiplist() to do an early
punt in report(), but also fsck_blob(), presumably you never want the
latter, and that early punting wouldn't be needed if your report()
function intercepted the modules blob id for stashing it away / later
reporting / whatever.
So yeah, I'm 99% sure now that's not what I meant :)
What I meant with:
Or if we want to keep the "print <list> | process"[...]
Is that we have an existing ad-hoc IPC model for these commands in
passing along the skipList, which is made more complex because sometimes
the initial process reads the file, sometimes it passes it along as-is
to the child.
And then there's this patch that passes OIDs too, but through a
different mechanism.
I was suggesting that perhaps it made more sense to refactor both so
they could use the same mechanism, because we're potentially passing two
lists of OIDs between the two. Just one goes via line-at-a-time in the
output, the other via a config option on the command-line.
Thanks for your explanation. I still think that they are quite different
- skiplist is a user-written file containing a list of OIDs that will
likely never change, whereas my list of dangling .gitmodules is a list
of OIDs dynamically generated (and thus, always different) whenever a
fetch is done. So I think it's quite reasonable to pass skiplist as a
file name, and my list should be passed line-by-line.
Rename variables in a function added in 0282f4dced0 (fsck: offer a
function to demote fsck errors to warnings, 2015-06-22).
It was needlessly confusing that it took a "msg_type" argument, but
then later declared another "msg_type" of a different type.
Let's rename that to "tmp", and rename "id" to "msg_id" and "msg_id"
to "msg_id_str" etc. This will make a follow-up change smaller.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
fsck.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
Rename the remaining variables of type fsck_msg_id from "id" to
"msg_id". This change is relatively small, and is worth the churn for
a later change where we have different id's in the "report" function.
---
fsck.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
Rename the FOREACH_MSG_ID macro to FOREACH_FSCK_MSG_ID in preparation
for moving it over to fsck.h. It's good convention to name macros
in *.h files in such a way as to clearly not clash with any other
names in other files.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
fsck.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
@@ -29,17 +32,17 @@ typedef int (*fsck_walk_func)(struct object *obj, enum object_type object_type,/* callback for fsck_object, type is FSCK_ERROR or FSCK_WARN */typedefint(*fsck_error)(structfsck_options*o,conststructobject_id*oid,enumobject_typeobject_type,-intmsg_type,constchar*message);+enumfsck_msg_typemsg_type,constchar*message);intfsck_error_function(structfsck_options*o,conststructobject_id*oid,enumobject_typeobject_type,-intmsg_type,constchar*message);+enumfsck_msg_typemsg_type,constchar*message);structfsck_options{fsck_walk_funcwalk;fsck_errorerror_func;unsignedstrict:1;-int*msg_type;+enumfsck_msg_type*msg_type;structoidsetskiplist;kh_oid_map_t*object_names;};
Add the object_name member to the initialization macro. This was
omitted in 7b35efd734e (fsck_walk(): optionally name objects on the
go, 2016-07-17) when the field was added.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
fsck.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
In f417eed8cde (fsck: provide a function to parse fsck message IDs,
2015-06-22) the "STR" macro was introduced, but that short macro name
was not undefined after use as was done earlier in the same series for
the MSG_ID macro in c99ba492f1c (fsck: introduce identifiers for fsck
messages, 2015-06-22).
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
fsck.c | 1 +
1 file changed, 1 insertion(+)
There's no reason to defer the calling of parse_msg_type() until after
we've checked if the "id < 0". This is not a hot codepath, and
parse_msg_type() itself may die on invalid input.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
fsck.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
Refactor code added in 71ab8fa840f (fsck: report the ID of the
error/warning, 2015-06-22) to resolve the msg_id to a string in the
function that wants it, instead of doing it in report().
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
fsck.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Change the fsck_walk_func to use an "enum object_type" instead of an
"int" type. The types are compatible, and ever since this was added in
355885d5315 (add generic, type aware object chain walker, 2008-02-25)
we've used entries from object_type (OBJ_BLOB etc.).
So this doesn't really change anything as far as the generated code is
concerned, it just gives the compiler more information and makes this
easier to read.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/fsck.c | 3 ++-
builtin/index-pack.c | 3 ++-
builtin/unpack-objects.c | 3 ++-
fsck.h | 3 ++-
4 files changed, 8 insertions(+), 4 deletions(-)
@@ -23,7 +23,8 @@ int is_valid_msg_type(const char *msg_id, const char *msg_type);*<0errorsignaledandabort*>0errorsignaledanddonotabort*/-typedefint(*fsck_walk_func)(structobject*obj,inttype,void*data,structfsck_options*options);+typedefint(*fsck_walk_func)(structobject*obj,enumobject_typeobject_type,+void*data,structfsck_options*options);/* callback for fsck_object, type is FSCK_ERROR or FSCK_WARN */typedefint(*fsck_error)(structfsck_options*o,
As suggested in
https://lore.kernel.org/git/87zh028ctp.fsf@evledraar.gmail.com/ a
version of this that doesn't conflict with other in-flight topics. I
can submit the rest later.
Ævar Arnfjörð Bjarmason (10):
fsck.h: indent arguments to of fsck_set_msg_type
fsck.h: use "enum object_type" instead of "int"
fsck.c: rename variables in fsck_set_msg_type() for less confusion
fsck.c: move definition of msg_id into append_msg_id()
fsck.c: rename remaining fsck_msg_id "id" to "msg_id"
fsck.h: move FSCK_{FATAL,INFO,ERROR,WARN,IGNORE} into an enum
fsck.c: call parse_msg_type() early in fsck_set_msg_type()
fsck.c: undefine temporary STR macro after use
fsck.c: give "FOREACH_MSG_ID" a more specific name
fsck.h: update FSCK_OPTIONS_* for object_name
builtin/fsck.c | 5 ++--
builtin/index-pack.c | 3 +-
builtin/mktag.c | 3 +-
builtin/unpack-objects.c | 3 +-
fsck.c | 60 ++++++++++++++++++++--------------------
fsck.h | 26 +++++++++--------
6 files changed, 54 insertions(+), 46 deletions(-)
Range-diff:
-: ----------- > 1: 88b347b74ed fsck.h: indent arguments to of fsck_set_msg_type
1: 1a60d65d2ca ! 2: 868eac3d4d1 fsck.h: use use "enum object_type" instead of "int"
@@ Metadata
Author: Ævar Arnfjörð Bjarmason [off-list ref]
## Commit message ##
- fsck.h: use use "enum object_type" instead of "int"
+ fsck.h: use "enum object_type" instead of "int"
Change the fsck_walk_func to use an "enum object_type" instead of an
"int" type. The types are compatible, and ever since this was added in
2: 24761f269b7 = 3: f599dc6c8f3 fsck.c: rename variables in fsck_set_msg_type() for less confusion
3: fb4c66f9305 = 4: 33f3b1942c1 fsck.c: move definition of msg_id into append_msg_id()
4: a129dbd9964 = 5: 28c9245e418 fsck.c: rename remaining fsck_msg_id "id" to "msg_id"
5: d9bee41072e = 6: d25037c6f18 fsck.h: move FSCK_{FATAL,INFO,ERROR,WARN,IGNORE} into an enum
6: 423568026c3 = 7: 66d0f1047cc fsck.c: call parse_msg_type() early in fsck_set_msg_type()
7: cb43e832738 = 8: 7643a5bf211 fsck.c: undefine temporary STR macro after use
8: 2cd14cb4e2a = 9: 7c64e2267ce fsck.c: give "FOREACH_MSG_ID" a more specific name
9: 1ada154ef23 < -: ----------- fsck.[ch]: move FOREACH_FSCK_MSG_ID & fsck_msg_id from *.c to *.h
10: c4179445f22 < -: ----------- fsck.c: pass along the fsck_msg_id in the fsck_error callback
11: c1fc724f0e8 < -: ----------- fsck.c: add an fsck_set_msg_type() API that takes enums
12: 8de91fac068 = 10: a98a3512629 fsck.h: update FSCK_OPTIONS_* for object_name
13: 29ff97856ff < -: ----------- fsck.c: move gitmodules_{found,done} into fsck_options
--
2.30.0.284.gd98b1dd5eaa7
Sorry for being unclear here. I don't think (honestly I don't remember,
it's been almost a month) that I meant to you should use the skipList.
Looking at that code again we use object_on_skiplist() to do an early
punt in report(), but also fsck_blob(), presumably you never want the
latter, and that early punting wouldn't be needed if your report()
function intercepted the modules blob id for stashing it away / later
reporting / whatever.
So yeah, I'm 99% sure now that's not what I meant :)
What I meant with:
Or if we want to keep the "print <list> | process"[...]
Is that we have an existing ad-hoc IPC model for these commands in
passing along the skipList, which is made more complex because sometimes
the initial process reads the file, sometimes it passes it along as-is
to the child.
And then there's this patch that passes OIDs too, but through a
different mechanism.
I was suggesting that perhaps it made more sense to refactor both so
they could use the same mechanism, because we're potentially passing two
lists of OIDs between the two. Just one goes via line-at-a-time in the
output, the other via a config option on the command-line.
Thanks for your explanation. I still think that they are quite different
- skiplist is a user-written file containing a list of OIDs that will
likely never change, whereas my list of dangling .gitmodules is a list
of OIDs dynamically generated (and thus, always different) whenever a
fetch is done. So I think it's quite reasonable to pass skiplist as a
file name, and my list should be passed line-by-line.
Sure, but I'm not talking about passing it as a tempfile.
Yes, I suggested that in the third-to-last paragraph of [1] but then
went on to say that we could also move to some IPC mechanism where you
spew in the list of dangling .gitmodules, and we also spew in the
skipList and anything else we want to pass in.
I'm not saying this needs to be part of this series. But let me
rephrase:
We now have some combination of
{receive-pack,upload-pack,send-pack,fetch-pack,unpack-objects} that need
to communicate locally or pass data back & forth, passing data either
via a CLI option to read a file, packnames/refs on --stdin, or (now) a
single list of OIDs on stdout.
Let's say we don't just need to pass the .gitmodules OIDs, but also
e.g. .mailmap OIDs or whatever (due to some future vulnerability).
Would this IPC mechanism deal with that, or would we need to introduce a
breaking change (Re: my recently send mail about concurrent updates of
libexec programs)? Can we use soemething like pkt-line to talk back &
forth in an extensible way?
Not needed now, just food for thought...
1. https://lore.kernel.org/git/87czxu7c15.fsf@evledraar.gmail.com/
From: Jeff King <hidden> Date: 2021-02-18 19:50:28
On Thu, Feb 18, 2021 at 11:58:33AM +0100, Ævar Arnfjörð Bjarmason wrote:
Rename variables in a function added in 0282f4dced0 (fsck: offer a
function to demote fsck errors to warnings, 2015-06-22).
It was needlessly confusing that it took a "msg_type" argument, but
then later declared another "msg_type" of a different type.
Let's rename that to "tmp", and rename "id" to "msg_id" and "msg_id"
to "msg_id_str" etc. This will make a follow-up change smaller.
I think this is an improvement, though maybe "severity" would be a
less-generic term than "type".
I always get nervous when a refactoring renames something away from
"foo", and then renames another thing _to_ "foo". Any untouched bits of
code are vulnerable to confusing them.
But I think the types are sufficiently different that we can mostly rely
on the compiler (though things like numeric or bool comparisons can work
with either pointers or ints), and the fact that we can see the entire
function is small enough that we can see the entire thing in the context
here.
So I think it is OK.
-Peff
You kept the values the same as they were before, which is good in a
refactoring step, but...wow, the ordering is weird and confusing.
In FATAL/ERROR/WARN/IGNORE the number increases as severity decreases.
Maybe reversed from how I'd do it, but at least the order makes sense.
But somehow INFO is on the far side of FATAL?
Again, not something to address in this patch, but I hope something we
could maybe deal with in the longer term (perhaps along with fixing the
weird "INFO is a warning from the user's perspective, but WARNING is
generally an error" behavior).
I also know that this is assigning WARN and IGNORE based on
counting-by-one from ERROR, so it's correct. But I think it would be
more obvious if you simply filled in the values manually, so a reader
does not have to wonder why some are assigned and some are not.
-Peff
From: Jeff King <hidden> Date: 2021-02-18 19:59:46
On Thu, Feb 18, 2021 at 11:58:40AM +0100, Ævar Arnfjörð Bjarmason wrote:
Add the object_name member to the initialization macro. This was
omitted in 7b35efd734e (fsck_walk(): optionally name objects on the
go, 2016-07-17) when the field was added.
We're correct either way here, because trailing fields that are not
initialized will get the usual zero-initialization. But I don't mind
trying to be more complete.
That said, we have embraced designated initializers these days, in which
case we usually omit the NULL ones. So perhaps:
#define FSCK_OPTIONS_DEFAULT { \
.walk = fsck_error_function, \
.skiplist = OIDSET_INIT, \
}
#define FSCK_OPTIONS_STRICT { \
.walk = fsck_error_function, \
.skiplist = OIDSET_INIT, \
.strict = 1, \
}
would be more readable still?
-Peff
From: Jeff King <hidden> Date: 2021-02-18 20:00:37
On Thu, Feb 18, 2021 at 11:58:39AM +0100, Ævar Arnfjörð Bjarmason wrote:
Rename the FOREACH_MSG_ID macro to FOREACH_FSCK_MSG_ID in preparation
for moving it over to fsck.h. It's good convention to name macros
in *.h files in such a way as to clearly not clash with any other
names in other files.
The patch to move it is not in this v2 of the series, so arguably this
is less interesting. However, I think the resulting code is equally or
more readable, so I don't mind it standing on its own.
-Peff
From: Jonathan Tan <hidden> Date: 2021-02-22 19:26:31
Currently, when fetching, packfiles referenced by URIs are run through
index-pack without any arguments other than --stdin and --keep, no
matter what arguments are used for the packfile that is inline in the
fetch response. As a preparation for ensuring that all packs (whether
inline or not) use the same index-pack arguments, teach the http
subsystem to allow custom index-pack arguments.
http-fetch has been updated to use the new API. For now, it passes
--keep alone instead of --keep with a process ID, but this is only
temporary because http-fetch itself will be taught to accept index-pack
parameters (instead of using a hardcoded constant) in a subsequent
commit.
Signed-off-by: Jonathan Tan <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
http-fetch.c | 6 +++++-
http.c | 15 ++++++++-------
http.h | 10 +++++-----
3 files changed, 18 insertions(+), 13 deletions(-)
From: Jonathan Tan <hidden> Date: 2021-02-22 19:26:32
This is the next step in teaching fetch-pack to pass its index-pack
arguments when processing packfiles referenced by URIs.
The "--keep" in fetch-pack.c will be replaced with a full message in a
subsequent commit.
Signed-off-by: Jonathan Tan <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
Documentation/git-http-fetch.txt | 10 ++++++++--
fetch-pack.c | 3 +++
http-fetch.c | 20 +++++++++++++++-----
t/t5550-http-fetch-dumb.sh | 5 ++++-
4 files changed, 30 insertions(+), 8 deletions(-)
@@ -41,11 +41,17 @@ commit-id:: <commit-id>['\t'<filename-as-in--w>] --packfile=<hash>::- Instead of a commit id on the command line (which is not expected in+ For internal use only. Instead of a commit id on the command+ line (which is not expected in this case), 'git http-fetch' fetches the packfile directly at the given URL and uses index-pack to generate corresponding .idx and .keep files. The hash is used to determine the name of the temporary file and is- arbitrary. The output of index-pack is printed to stdout.+ arbitrary. The output of index-pack is printed to stdout. Requires+ --index-pack-args.++--index-pack-args=<args>::+ For internal use only. The command to run on the contents of the+ downloaded pack. Arguments are URL-encoded separated by spaces. --recover:: Verify that everything reachable from target is fetched. Used after
@@ -43,11 +44,9 @@ static int fetch_using_walker(const char *raw_url, int get_verbosely,returnrc;}-staticconstchar*index_pack_args[]=-{"index-pack","--stdin","--keep",NULL};-staticvoidfetch_single_packfile(structobject_id*packfile_hash,-constchar*url){+constchar*url,+constchar**index_pack_args){structhttp_pack_request*preq;structslot_resultsresults;intret;
@@ -90,6 +89,7 @@ int cmd_main(int argc, const char **argv)intpackfile=0;intnongit;structobject_idpackfile_hash;+structstrvecindex_pack_args=STRVEC_INIT;setup_git_directory_gently(&nongit);
@@ -116,6 +116,8 @@ int cmd_main(int argc, const char **argv)packfile=1;if(parse_oid_hex(p,&packfile_hash,&end)||*end)die(_("argument to --packfile must be a valid hash (got '%s')"),p);+}elseif(skip_prefix(argv[arg],"--index-pack-arg=",&p)){+strvec_push(&index_pack_args,p);}arg++;}
@@ -128,10 +130,18 @@ int cmd_main(int argc, const char **argv)git_config(git_default_config,NULL);if(packfile){-fetch_single_packfile(&packfile_hash,argv[arg]);+if(!index_pack_args.nr)+die(_("--packfile requires --index-pack-args"));++fetch_single_packfile(&packfile_hash,argv[arg],+index_pack_args.v);+return0;}+if(index_pack_args.nr)+die(_("--index-pack-args can only be used with --packfile"));+if(commits_on_stdin){commits=walker_targets_stdin(&commit_id,&write_ref);}else{
From: Jonathan Tan <hidden> Date: 2021-02-22 19:26:35
Unify the index-pack arguments used when processing the inline pack and
when downloading packfiles referenced by URIs. This is done by teaching
get_pack() to also store the index-pack arguments whenever at least one
packfile URI is given, and then when processing the packfile URI(s),
using the stored arguments.
Signed-off-by: Jonathan Tan <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
fetch-pack.c | 34 +++++++++++++++++++++++-----------
1 file changed, 23 insertions(+), 11 deletions(-)
From: Jonathan Tan <hidden> Date: 2021-02-22 19:26:47
Teach index-pack to print dangling .gitmodules links after its "keep" or
"pack" line instead of declaring an error, and teach fetch-pack to check
such lines printed.
This allows the tree side of the .gitmodules link to be in one packfile
and the blob side to be in another without failing the fsck check,
because it is now fetch-pack which checks such objects after all
packfiles have been downloaded and indexed (and not index-pack on an
individual packfile, as it is before this commit).
Signed-off-by: Jonathan Tan <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
Documentation/git-index-pack.txt | 7 ++-
builtin/index-pack.c | 25 +++++++++-
builtin/receive-pack.c | 2 +-
fetch-pack.c | 78 +++++++++++++++++++++++++++-----
fsck.c | 5 ++
fsck.h | 2 +
pack-write.c | 8 +++-
pack.h | 2 +-
t/t5702-protocol-v2.sh | 58 ++++++++++++++++++++++--
9 files changed, 165 insertions(+), 22 deletions(-)
@@ -78,7 +78,12 @@ OPTIONS Die if the pack contains broken links. For internal use only. --fsck-objects::- Die if the pack contains broken objects. For internal use only.+ For internal use only.+++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"). --threads=<n>:: Specifies the number of threads to spawn when resolving
@@ -847,8 +847,9 @@ test_expect_success 'part of packfile response provided as URI' 'test-fhfound&&test-fh2found&&-# Ensure that there are exactly 6 files (3 .pack and 3 .idx).-lshttp_child/.git/objects/pack/*>filelist&&+# Ensure that there are exactly 3 packfiles with associated .idx+lshttp_child/.git/objects/pack/*.pack\+http_child/.git/objects/pack/*.idx>filelist&&test_line_count=6filelist'
@@ -901,8 +902,9 @@ test_expect_success 'packfile-uri with transfer.fsckobjects' '-cfetch.uriprotocols=http,https\clone"$HTTPD_URL/smart/http_parent"http_child&&-# Ensure that there are exactly 4 files (2 .pack and 2 .idx).-lshttp_child/.git/objects/pack/*>filelist&&+# Ensure that there are exactly 2 packfiles with associated .idx+lshttp_child/.git/objects/pack/*.pack\+http_child/.git/objects/pack/*.idx>filelist&&test_line_count=4filelist'
@@ -936,6 +938,54 @@ test_expect_success 'packfile-uri with transfer.fsckobjects fails on bad object'test_i18ngrep"invalid author/committer line - missing email"error'+test_expect_success'packfile-uri with transfer.fsckobjects succeeds when .gitmodules is separate from tree''+P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent"&&+rm-rf"$P"http_child&&++gitinit"$P"&&+git-C"$P"config"uploadpack.allowsidebandall""true"&&++echo"[submodule libfoo]">"$P/.gitmodules"&&+echo"path = include/foo">>"$P/.gitmodules"&&+echo"url = git://example.com/git/lib.git">>"$P/.gitmodules"&&+git-C"$P"add.gitmodules&&+git-C"$P"commit-mx&&++configure_exclusion"$P".gitmodules>h&&++sane_unsetGIT_TEST_SIDEBAND_ALL&&+git-cprotocol.version=2-ctransfer.fsckobjects=1\+-cfetch.uriprotocols=http,https\+clone"$HTTPD_URL/smart/http_parent"http_child&&++# Ensure that there are exactly 2 packfiles with associated .idx+lshttp_child/.git/objects/pack/*.pack\+http_child/.git/objects/pack/*.idx>filelist&&+test_line_count=4filelist+'++test_expect_success'packfile-uri with transfer.fsckobjects fails when .gitmodules separate from tree is invalid''+P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent"&&+rm-rf"$P"http_childerr&&++gitinit"$P"&&+git-C"$P"config"uploadpack.allowsidebandall""true"&&++echo"[submodule \"..\"]">"$P/.gitmodules"&&+echo"path = include/foo">>"$P/.gitmodules"&&+echo"url = git://example.com/git/lib.git">>"$P/.gitmodules"&&+git-C"$P"add.gitmodules&&+git-C"$P"commit-mx&&++configure_exclusion"$P".gitmodules>h&&++sane_unsetGIT_TEST_SIDEBAND_ALL&&+test_must_failgit-cprotocol.version=2-ctransfer.fsckobjects=1\+-cfetch.uriprotocols=http,https\+clone"$HTTPD_URL/smart/http_parent"http_child2>err&&+test_i18ngrep"disallowed submodule name"err+'+# DO NOT add non-httpd-specific tests here, because the last part of this# test script is only executed when httpd is available and enabled.
@@ -41,11 +41,17 @@ commit-id:: <commit-id>['\t'<filename-as-in--w>] --packfile=<hash>::- Instead of a commit id on the command line (which is not expected in+ For internal use only. Instead of a commit id on the command+ line (which is not expected in this case), 'git http-fetch' fetches the packfile directly at the given URL and uses index-pack to generate corresponding .idx and .keep files. The hash is used to determine the name of the temporary file and is- arbitrary. The output of index-pack is printed to stdout.+ arbitrary. The output of index-pack is printed to stdout. Requires+ --index-pack-args.++--index-pack-args=<args>::+ For internal use only. The command to run on the contents of the+ downloaded pack. Arguments are URL-encoded separated by spaces. --recover:: Verify that everything reachable from target is fetched. Used after
@@ -41,11 +41,17 @@ commit-id:: <commit-id>['\t'<filename-as-in--w>] --packfile=<hash>::- Instead of a commit id on the command line (which is not expected in+ For internal use only. Instead of a commit id on the command+ line (which is not expected in this case), 'git http-fetch' fetches the packfile directly at the given URL and uses index-pack to generate corresponding .idx and .keep files. The hash is used to determine the name of the temporary file and is- arbitrary. The output of index-pack is printed to stdout.+ arbitrary. The output of index-pack is printed to stdout. Requires+ --index-pack-args.++--index-pack-args=<args>::+ For internal use only. The command to run on the contents of the+ downloaded pack. Arguments are URL-encoded separated by spaces. --recover:: Verify that everything reachable from target is fetched. Used after
The docs say --*-args, but the code checks --*arg, that seems like a
mistake that should be fixed to make the code/tests use the plural form,
no?
Thanks for catching that. Originally it was plural since this single
argument would give multiple arguments to index-pack, but now each
argument gives only a single argument, so "arg" is correct. I'll update
it in the next version.
From: Jonathan Nieder <hidden> Date: 2021-03-05 00:19:22
Hi Jonathan,
Jonathan Tan wrote:
This is the next step in teaching fetch-pack to pass its index-pack
arguments when processing packfiles referenced by URIs.
The "--keep" in fetch-pack.c will be replaced with a full message in a
subsequent commit.
Signed-off-by: Jonathan Tan <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
Documentation/git-http-fetch.txt | 10 ++++++++--
fetch-pack.c | 3 +++
http-fetch.c | 20 +++++++++++++++-----
t/t5550-http-fetch-dumb.sh | 5 ++++-
4 files changed, 30 insertions(+), 8 deletions(-)
This is producing an interesting symptom for me:
git init repro
cd repro
git config fetch.uriprotocols https
git config remote.origin.url https://fuchsia.googlesource.com/fuchsia
git config remote.origin.fetch +refs/heads/*:refs/remotes/origin/*
git fetch -p origin
Expected result: fetches
Actual result:
fatal: pack has bad object at offset 12: unknown object type 5
fatal: finish_http_pack_request gave result -1
fatal: fetch-pack: expected keep then TAB at start of http-fetch output
Thanks to Nathan Mulcahey (cc-ed) for a clear report.
Bisects to b664e9ffa153189dae9b88f32d1c5fedcf85056a, which is part of
"next" and 2.31.0-rc1. Another report of the same is at
https://crbug.com/1184814.
Known problem?
Thanks,
Jonathan
From: Jonathan Tan <hidden> Date: 2021-03-05 01:16:27
When fetching (as opposed to cloning) from a repository with packfile
URIs enabled, an error like this may occur:
fatal: pack has bad object at offset 12: unknown object type 5
fatal: finish_http_pack_request gave result -1
fatal: fetch-pack: expected keep then TAB at start of http-fetch output
This bug was introduced in b664e9ffa1 ("fetch-pack: with packfile URIs,
use index-pack arg", 2021-02-22), when the index-pack args used when
processing the inline packfile of a fetch response and when processing
packfile URIs were unified.
This bug happens because fetch, by default, partially reads (and
consumes) the header of the inline packfile to determine if it should
store the downloaded objects as a packfile or loose objects, and thus
passes --pack_header=<...> to index-pack to inform it that some bytes
are missing. However, when it subsequently fetches the additional
packfiles linked by URIs, it reuses the same index-pack arguments, thus
wrongly passing --index-pack-arg=--pack_header=<...> when no bytes are
missing.
This does not happen when cloning because "git clone" always passes
do_keep, which instructs the fetch mechanism to always retain the
packfile, eliminating the need to read the header.
There are a few ways to fix this, including filtering out pack_header
arguments when downloading the additional packfiles, but I decided to
stick to always using index-pack throughout when packfile URIs are
present - thus, Git no longer needs to read the bytes, and no longer
needs --pack_header here.
Signed-off-by: Jonathan Tan <redacted>
---
Here's a fix for this issue.
This is on jt/transfer-fsck-across-packs.
One simplification that we could do is to eliminate the unpack-objects
codepath. As far as I understand, the main advantage of writing loose
objects is that we have automatic SHA-1 collision detection, but we have
such mitigations when writing packs too, so that might not be as large a
benefit as we think. This simplification would have enabled us to avoid
this bug, I think.
---
fetch-pack.c | 4 ++--
t/t5702-protocol-v2.sh | 21 +++++++++++++++++++++
2 files changed, 23 insertions(+), 2 deletions(-)
@@ -853,6 +853,27 @@ test_expect_success 'part of packfile response provided as URI' 'test_line_count=6filelist'+test_expect_success'packfile URIs with fetch instead of clone''+P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent"&&+rm-rf"$P"http_childlog&&++gitinit"$P"&&+git-C"$P"config"uploadpack.allowsidebandall""true"&&++echomy-blob>"$P/my-blob"&&+git-C"$P"addmy-blob&&+git-C"$P"commit-mx&&++configure_exclusion"$P"my-blob>h&&++gitinithttp_child&&++GIT_TEST_SIDEBAND_ALL=1\+git-Chttp_child-cprotocol.version=2\+-cfetch.uriprotocols=http,https\+fetch"$HTTPD_URL/smart/http_parent"+'+ test_expect_success'fetching with valid packfile URI but invalid hash fails''P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent"&&rm-rf"$P"http_childlog&&
Add the object_name member to the initialization macro. This was
omitted in 7b35efd734e (fsck_walk(): optionally name objects on the
go, 2016-07-17) when the field was added.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
fsck.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Now that jt/transfer-fsck-across-packs has been merged to master
here's a re-roll of v1[1]+v2[2] of this series. v2 was slimmed-down +
had a trivial typo fix, so I've done the range-diff against v1.
This makes the recent fetch-pack work use the fsck_msg_id API to
distinguish messages, and has other various cleanups and improvements
to make the fsck API easier to use in the future.
There's a an easy merge conflict here with other in-flight changes to
fsck. I figured it was better to send this now than wait for those to
land.
1. https://lore.kernel.org/git/20210217194246.25342-1-avarab@gmail.com/
2. https://lore.kernel.org/git/20210218105840.11989-1-avarab@gmail.com/
Ævar Arnfjörð Bjarmason (22):
fsck.h: update FSCK_OPTIONS_* for object_name
fsck.h: use designed initializers for FSCK_OPTIONS_{DEFAULT,STRICT}
fsck.h: reduce duplication between FSCK_OPTIONS_{DEFAULT,STRICT}
fsck.h: add a FSCK_OPTIONS_COMMON_ERROR_FUNC macro
fsck.h: indent arguments to of fsck_set_msg_type
fsck.h: use "enum object_type" instead of "int"
fsck.c: rename variables in fsck_set_msg_type() for less confusion
fsck.c: move definition of msg_id into append_msg_id()
fsck.c: rename remaining fsck_msg_id "id" to "msg_id"
fsck.c: refactor fsck_msg_type() to limit scope of "int msg_type"
fsck.h: move FSCK_{FATAL,INFO,ERROR,WARN,IGNORE} into an enum
fsck.h: re-order and re-assign "enum fsck_msg_type"
fsck.c: call parse_msg_type() early in fsck_set_msg_type()
fsck.c: undefine temporary STR macro after use
fsck.c: give "FOREACH_MSG_ID" a more specific name
fsck.[ch]: move FOREACH_FSCK_MSG_ID & fsck_msg_id from *.c to *.h
fsck.c: pass along the fsck_msg_id in the fsck_error callback
fsck.c: add an fsck_set_msg_type() API that takes enums
fsck.c: move gitmodules_{found,done} into fsck_options
fetch-pack: don't needlessly copy fsck_options
fetch-pack: use file-scope static struct for fsck_options
fetch-pack: use new fsck API to printing dangling submodules
Makefile | 1 +
builtin/fsck.c | 7 +-
builtin/index-pack.c | 30 ++-----
builtin/mktag.c | 7 +-
builtin/unpack-objects.c | 3 +-
fetch-pack.c | 6 +-
fsck-cb.c | 16 ++++
fsck.c | 175 ++++++++++++---------------------------
fsck.h | 132 ++++++++++++++++++++++++++---
9 files changed, 211 insertions(+), 166 deletions(-)
create mode 100644 fsck-cb.c
Range-diff:
13: 8de91fac068 = 1: 9d809466bd1 fsck.h: update FSCK_OPTIONS_* for object_name
-: ----------- > 2: 33e8b6d6545 fsck.h: use designed initializers for FSCK_OPTIONS_{DEFAULT,STRICT}
-: ----------- > 3: c23f7ce9e4a fsck.h: reduce duplication between FSCK_OPTIONS_{DEFAULT,STRICT}
-: ----------- > 4: 5dde68df6c3 fsck.h: add a FSCK_OPTIONS_COMMON_ERROR_FUNC macro
1: 88b347b74ed = 5: 7ae35a6e9d2 fsck.h: indent arguments to of fsck_set_msg_type
2: 1a60d65d2ca ! 6: dfb5f754b37 fsck.h: use use "enum object_type" instead of "int"
@@ Metadata
Author: Ævar Arnfjörð Bjarmason [off-list ref]
## Commit message ##
- fsck.h: use use "enum object_type" instead of "int"
+ fsck.h: use "enum object_type" instead of "int"
Change the fsck_walk_func to use an "enum object_type" instead of an
"int" type. The types are compatible, and ever since this was added in
3: 24761f269b7 ! 7: fd58ec73c6b fsck.c: rename variables in fsck_set_msg_type() for less confusion
@@ Commit message
It was needlessly confusing that it took a "msg_type" argument, but
then later declared another "msg_type" of a different type.
- Let's rename that to "tmp", and rename "id" to "msg_id" and "msg_id"
- to "msg_id_str" etc. This will make a follow-up change smaller.
+ Let's rename that to "severity", and rename "id" to "msg_id" and
+ "msg_id" to "msg_id_str" etc. This will make a follow-up change
+ smaller.
Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
@@ fsck.c: int is_valid_msg_type(const char *msg_id, const char *msg_type)
int i;
- int *msg_type;
- ALLOC_ARRAY(msg_type, FSCK_MSG_MAX);
-+ int *tmp;
-+ ALLOC_ARRAY(tmp, FSCK_MSG_MAX);
++ int *severity;
++ ALLOC_ARRAY(severity, FSCK_MSG_MAX);
for (i = 0; i < FSCK_MSG_MAX; i++)
- msg_type[i] = fsck_msg_type(i, options);
- options->msg_type = msg_type;
-+ tmp[i] = fsck_msg_type(i, options);
-+ options->msg_type = tmp;
++ severity[i] = fsck_msg_type(i, options);
++ options->msg_type = severity;
}
- options->msg_type[id] = type;
4: fb4c66f9305 = 8: 48cb4d3bb70 fsck.c: move definition of msg_id into append_msg_id()
5: a129dbd9964 ! 9: 2c80ad32038 fsck.c: rename remaining fsck_msg_id "id" to "msg_id"
@@ Commit message
"msg_id". This change is relatively small, and is worth the churn for
a later change where we have different id's in the "report" function.
+ Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
+
## fsck.c ##
@@ fsck.c: void fsck_set_msg_types(struct fsck_options *options, const char *values)
free(to_free);
-: ----------- > 10: 92dfbdfb624 fsck.c: refactor fsck_msg_type() to limit scope of "int msg_type"
6: d9bee41072e ! 11: c1c476af69b fsck.h: move FSCK_{FATAL,INFO,ERROR,WARN,IGNORE} into an enum
@@ Commit message
- f27d05b1704 (fsck: allow upgrading fsck warnings to errors,
2015-06-22)
+ The reason these were defined in two different places is because we
+ use FSCK_{IGNORE,INFO,FATAL} only in fsck.c, but FSCK_{ERROR,WARN} are
+ used by external callbacks.
+
+ Untangling that would take some more work, since we expose the new
+ "enum fsck_msg_type" to both. Similar to "enum object_type" it's not
+ worth structuring the API in such a way that only those who need
+ FSCK_{ERROR,WARN} pass around a different type.
+
Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
## builtin/fsck.c ##
@@ builtin/fsck.c: static int objerror(struct object *obj, const char *err)
switch (msg_type) {
case FSCK_WARN:
+ ## builtin/index-pack.c ##
+@@ builtin/index-pack.c: static void show_pack_info(int stat_only)
+ static int print_dangling_gitmodules(struct fsck_options *o,
+ const struct object_id *oid,
+ enum object_type object_type,
+- int msg_type, const char *message)
++ enum fsck_msg_type msg_type,
++ const char *message)
+ {
+ /*
+ * NEEDSWORK: Plumb the MSG_ID (from fsck.c) here and use it
+
## builtin/mktag.c ##
@@ builtin/mktag.c: static int mktag_config(const char *var, const char *value, void *cb)
static int mktag_fsck_error_func(struct fsck_options *o,
@@ fsck.c: void list_config_fsck_msg_ids(struct string_list *list, const char *pref
+static enum fsck_msg_type fsck_msg_type(enum fsck_msg_id msg_id,
struct fsck_options *options)
{
-- int msg_type;
-+ enum fsck_msg_type msg_type;
-
assert(msg_id >= 0 && msg_id < FSCK_MSG_MAX);
+ if (!options->msg_type) {
+- int msg_type = msg_id_info[msg_id].msg_type;
++ enum fsck_msg_type msg_type = msg_id_info[msg_id].msg_type;
+
+ if (options->strict && msg_type == FSCK_WARN)
+ msg_type = FSCK_ERROR;
@@ fsck.c: static int fsck_msg_type(enum fsck_msg_id msg_id,
- return msg_type;
+ return options->msg_type[msg_id];
}
-static int parse_msg_type(const char *str)
@@ fsck.c: void fsck_set_msg_type(struct fsck_options *options,
if (!options->msg_type) {
int i;
-- int *tmp;
-+ enum fsck_msg_type *tmp;
- ALLOC_ARRAY(tmp, FSCK_MSG_MAX);
+- int *severity;
++ enum fsck_msg_type *severity;
+ ALLOC_ARRAY(severity, FSCK_MSG_MAX);
for (i = 0; i < FSCK_MSG_MAX; i++)
- tmp[i] = fsck_msg_type(i, options);
+ severity[i] = fsck_msg_type(i, options);
@@ fsck.c: static int report(struct fsck_options *options,
{
va_list ap;
@@ fsck.h
-#define FSCK_ERROR 1
-#define FSCK_WARN 2
-#define FSCK_IGNORE 3
--
+enum fsck_msg_type {
-+ FSCK_INFO = -2,
++ FSCK_INFO = -2,
+ FSCK_FATAL = -1,
+ FSCK_ERROR = 1,
+ FSCK_WARN,
+ FSCK_IGNORE
+};
+
struct fsck_options;
struct object;
-
@@ fsck.h: typedef int (*fsck_walk_func)(struct object *obj, enum object_type object_type,
/* callback for fsck_object, type is FSCK_ERROR or FSCK_WARN */
typedef int (*fsck_error)(struct fsck_options *o,
-: ----------- > 12: d55587719a5 fsck.h: re-order and re-assign "enum fsck_msg_type"
7: 423568026c3 = 13: 32828d1c78c fsck.c: call parse_msg_type() early in fsck_set_msg_type()
8: cb43e832738 = 14: 5c62066235c fsck.c: undefine temporary STR macro after use
9: 2cd14cb4e2a = 15: f8e50fbf7d3 fsck.c: give "FOREACH_MSG_ID" a more specific name
10: 1ada154ef23 ! 16: cd74dee8769 fsck.[ch]: move FOREACH_FSCK_MSG_ID & fsck_msg_id from *.c to *.h
@@ fsck.c
## fsck.h ##
@@ fsck.h: enum fsck_msg_type {
FSCK_WARN,
- FSCK_IGNORE
};
-+
+
+#define FOREACH_FSCK_MSG_ID(FUNC) \
+ /* fatal errors */ \
+ FUNC(NUL_IN_HEADER, FATAL) \
11: c4179445f22 ! 17: 234e287d081 fsck.c: pass along the fsck_msg_id in the fsck_error callback
@@ builtin/fsck.c: static int objerror(struct object *obj, const char *err)
switch (msg_type) {
case FSCK_WARN:
+ ## builtin/index-pack.c ##
+@@ builtin/index-pack.c: static int print_dangling_gitmodules(struct fsck_options *o,
+ const struct object_id *oid,
+ enum object_type object_type,
+ enum fsck_msg_type msg_type,
++ enum fsck_msg_id msg_id,
+ const char *message)
+ {
+ /*
+@@ builtin/index-pack.c: static int print_dangling_gitmodules(struct fsck_options *o,
+ printf("%s\n", oid_to_hex(oid));
+ return 0;
+ }
+- return fsck_error_function(o, oid, object_type, msg_type, message);
++ return fsck_error_function(o, oid, object_type, msg_type, msg_id, message);
+ }
+
+ int cmd_index_pack(int argc, const char **argv, const char *prefix)
+
## builtin/mktag.c ##
@@ builtin/mktag.c: static int mktag_fsck_error_func(struct fsck_options *o,
const struct object_id *oid,
12: c1fc724f0e8 ! 18: 8049dc07391 fsck.c: add an fsck_set_msg_type() API that takes enums
@@ fsck.c: int is_valid_msg_type(const char *msg_id, const char *msg_type)
+{
+ if (!options->msg_type) {
+ int i;
-+ enum fsck_msg_type *tmp;
-+ ALLOC_ARRAY(tmp, FSCK_MSG_MAX);
++ enum fsck_msg_type *severity;
++ ALLOC_ARRAY(severity, FSCK_MSG_MAX);
+ for (i = 0; i < FSCK_MSG_MAX; i++)
-+ tmp[i] = fsck_msg_type(i, options);
-+ options->msg_type = tmp;
++ severity[i] = fsck_msg_type(i, options);
++ options->msg_type = severity;
+ }
+
+ options->msg_type[msg_id] = msg_type;
@@ fsck.c: void fsck_set_msg_type(struct fsck_options *options,
- if (!options->msg_type) {
- int i;
-- enum fsck_msg_type *tmp;
-- ALLOC_ARRAY(tmp, FSCK_MSG_MAX);
+- enum fsck_msg_type *severity;
+- ALLOC_ARRAY(severity, FSCK_MSG_MAX);
- for (i = 0; i < FSCK_MSG_MAX; i++)
-- tmp[i] = fsck_msg_type(i, options);
-- options->msg_type = tmp;
+- severity[i] = fsck_msg_type(i, options);
+- options->msg_type = severity;
- }
-
- options->msg_type[msg_id] = msg_type;
14: 29ff97856ff ! 19: 4224a29d15c fsck.c: move gitmodules_{found,done} into fsck_options
@@ Commit message
fsck_options struct. It makes sense to keep all the context in the
same place.
+ This requires changing the recently added register_found_gitmodules()
+ function added in 5476e1efde (fetch-pack: print and use dangling
+ .gitmodules, 2021-02-22) to take fsck_options. That function will be
+ removed in a subsequent commit, but as it'll require the new
+ gitmodules_found attribute of "fsck_options" we need this intermediate
+ step first.
+
Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
+ ## fetch-pack.c ##
+@@ fetch-pack.c: static void fsck_gitmodules_oids(struct oidset *gitmodules_oids)
+
+ oidset_iter_init(gitmodules_oids, &iter);
+ while ((oid = oidset_iter_next(&iter)))
+- register_found_gitmodules(oid);
++ register_found_gitmodules(&fo, oid);
+ if (fsck_finish(&fo))
+ die("fsck failed");
+ }
+
## fsck.c ##
@@
#include "credential.h"
@@ fsck.c: static int fsck_blob(const struct object_id *oid, const char *buf,
if (object_on_skiplist(options, oid))
return 0;
+@@ fsck.c: int fsck_error_function(struct fsck_options *o,
+ return 1;
+ }
+
+-void register_found_gitmodules(const struct object_id *oid)
++void register_found_gitmodules(struct fsck_options *options, const struct object_id *oid)
+ {
+- oidset_insert(&gitmodules_found, oid);
++ oidset_insert(&options->gitmodules_found, oid);
+ }
+
+ int fsck_finish(struct fsck_options *options)
@@ fsck.c: int fsck_finish(struct fsck_options *options)
struct oidset_iter iter;
const struct object_id *oid;
@@ fsck.h: struct fsck_options {
kh_oid_map_t *object_names;
};
--#define FSCK_OPTIONS_DEFAULT { NULL, fsck_error_function, 0, NULL, OIDSET_INIT, NULL }
--#define FSCK_OPTIONS_STRICT { NULL, fsck_error_function, 1, NULL, OIDSET_INIT, NULL }
-+#define FSCK_OPTIONS_DEFAULT { NULL, fsck_error_function, 0, NULL, OIDSET_INIT, OIDSET_INIT, OIDSET_INIT, NULL }
-+#define FSCK_OPTIONS_STRICT { NULL, fsck_error_function, 1, NULL, OIDSET_INIT, OIDSET_INIT, OIDSET_INIT, NULL }
+@@ fsck.h: struct fsck_options {
+ .walk = NULL, \
+ .msg_type = NULL, \
+ .skiplist = OIDSET_INIT, \
++ .gitmodules_found = OIDSET_INIT, \
++ .gitmodules_done = OIDSET_INIT, \
+ .object_names = NULL,
+ #define FSCK_OPTIONS_COMMON_ERROR_FUNC \
+ FSCK_OPTIONS_COMMON \
+@@ fsck.h: int fsck_walk(struct object *obj, void *data, struct fsck_options *options);
+ int fsck_object(struct object *obj, void *data, unsigned long size,
+ struct fsck_options *options);
+
+-void register_found_gitmodules(const struct object_id *oid);
++void register_found_gitmodules(struct fsck_options *options,
++ const struct object_id *oid);
- /* descend in all linked child objects
- * the return value is:
+ /*
+ * fsck a tag, and pass info about it back to the caller. This is
-: ----------- > 20: 40b13468129 fetch-pack: don't needlessly copy fsck_options
-: ----------- > 21: 8e418abfbd7 fetch-pack: use file-scope static struct for fsck_options
-: ----------- > 22: 113de190f7d fetch-pack: use new fsck API to printing dangling submodules
--
2.31.0.rc0.126.g04f22c5b82
Add a FSCK_OPTIONS_COMMON_ERROR_FUNC macro for those that would like
to use FSCK_OPTIONS_COMMON in their own initialization, but supply
their own error functions.
Nothing is being changed to use this yet, but in some subsequent
commits we'll make use of this macro.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
fsck.h | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
Use a temporary macro to define what FSCK_OPTIONS_{DEFAULT,STRICT}
have in common, and define the two in terms of that macro.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
fsck.h | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)
Change the fsck_walk_func to use an "enum object_type" instead of an
"int" type. The types are compatible, and ever since this was added in
355885d5315 (add generic, type aware object chain walker, 2008-02-25)
we've used entries from object_type (OBJ_BLOB etc.).
So this doesn't really change anything as far as the generated code is
concerned, it just gives the compiler more information and makes this
easier to read.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/fsck.c | 3 ++-
builtin/index-pack.c | 3 ++-
builtin/unpack-objects.c | 3 ++-
fsck.h | 3 ++-
4 files changed, 8 insertions(+), 4 deletions(-)
@@ -23,7 +23,8 @@ int is_valid_msg_type(const char *msg_id, const char *msg_type);*<0errorsignaledandabort*>0errorsignaledanddonotabort*/-typedefint(*fsck_walk_func)(structobject*obj,inttype,void*data,structfsck_options*options);+typedefint(*fsck_walk_func)(structobject*obj,enumobject_typeobject_type,+void*data,structfsck_options*options);/* callback for fsck_object, type is FSCK_ERROR or FSCK_WARN */typedefint(*fsck_error)(structfsck_options*o,
Refactor code added in 71ab8fa840f (fsck: report the ID of the
error/warning, 2015-06-22) to resolve the msg_id to a string in the
function that wants it, instead of doing it in report().
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
fsck.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Refactor "if options->msg_type" and other code added in
0282f4dced0 (fsck: offer a function to demote fsck errors to warnings,
2015-06-22) to reduce the scope of the "int msg_type" variable.
This is in preparation for changing its type in a subsequent commit,
only using it in the "!options->msg_type" scope makes that change
This also brings the code in line with the fsck_set_msg_type()
function (also added in 0282f4dced0), which does a similar check for
"!options->msg_type". Another minor benefit is getting rid of the
style violation of not having braces for the body of the "if".
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
fsck.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
There's no reason to defer the calling of parse_msg_type() until after
we've checked if the "id < 0". This is not a hot codepath, and
parse_msg_type() itself may die on invalid input.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
fsck.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
Rename the FOREACH_MSG_ID macro to FOREACH_FSCK_MSG_ID in preparation
for moving it over to fsck.h. It's good convention to name macros
in *.h files in such a way as to clearly not clash with any other
names in other files.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
fsck.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
Rename the remaining variables of type fsck_msg_id from "id" to
"msg_id". This change is relatively small, and is worth the churn for
a later change where we have different id's in the "report" function.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
fsck.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
Move the FSCK_{FATAL,INFO,ERROR,WARN,IGNORE} defines into a new
fsck_msg_type enum.
These defines were originally introduced in:
- ba002f3b28a (builtin-fsck: move common object checking code to
fsck.c, 2008-02-25)
- f50c4407305 (fsck: disallow demoting grave fsck errors to warnings,
2015-06-22)
- efaba7cc77f (fsck: optionally ignore specific fsck issues
completely, 2015-06-22)
- f27d05b1704 (fsck: allow upgrading fsck warnings to errors,
2015-06-22)
The reason these were defined in two different places is because we
use FSCK_{IGNORE,INFO,FATAL} only in fsck.c, but FSCK_{ERROR,WARN} are
used by external callbacks.
Untangling that would take some more work, since we expose the new
"enum fsck_msg_type" to both. Similar to "enum object_type" it's not
worth structuring the API in such a way that only those who need
FSCK_{ERROR,WARN} pass around a different type.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/fsck.c | 2 +-
builtin/index-pack.c | 3 ++-
builtin/mktag.c | 3 ++-
fsck.c | 21 ++++++++++-----------
fsck.h | 16 ++++++++++------
5 files changed, 25 insertions(+), 20 deletions(-)
@@ -29,17 +33,17 @@ typedef int (*fsck_walk_func)(struct object *obj, enum object_type object_type,/* callback for fsck_object, type is FSCK_ERROR or FSCK_WARN */typedefint(*fsck_error)(structfsck_options*o,conststructobject_id*oid,enumobject_typeobject_type,-intmsg_type,constchar*message);+enumfsck_msg_typemsg_type,constchar*message);intfsck_error_function(structfsck_options*o,conststructobject_id*oid,enumobject_typeobject_type,-intmsg_type,constchar*message);+enumfsck_msg_typemsg_type,constchar*message);structfsck_options{fsck_walk_funcwalk;fsck_errorerror_func;unsignedstrict:1;-int*msg_type;+enumfsck_msg_type*msg_type;structoidsetskiplist;kh_oid_map_t*object_names;};