On Wed, Apr 22, 2020 at 12:55:36PM +0200, SZEDER Gábor wrote:
> On Tue, Apr 14, 2020 at 10:31:37PM -0600, Taylor Blau wrote:
> > On Tue, Apr 14, 2020 at 10:29:30PM -0600, Taylor Blau wrote:
> > > Whoops. I sent the wrong version of this patch. It should be the below:
> >
> > Double whoops. I was on the wrong branch, and hit send too early. *This*
> > is the version of the patch that I meant to send ;).
> >
> > --- >8 ---
> >
> > Subject: [PATCH] commit-graph.c: introduce '--[no-]check-oids'
> >
> > When operating on a stream of commit OIDs on stdin, 'git commit-graph
> > write' checks that each OID refers to an object that is indeed a commit.
> > This is convenient to make sure that the given input is well-formed, but
> > can sometimes be undesirable.
> >
> > For example, server operators may wish to feed the refnames that were
>
> s/the refnames/full commit object IDs pointed to by refs/
>
> or something similar.
>
> > updated during a push to 'git commit-graph write --input=stdin-commits',
> > and silently discard refs that don't point at commits.
>
> s/refs/<something along the lines of the above>/
>
> > This can be done
> > by combing the output of 'git for-each-ref' with '--format
> > %(*objecttype)', but this requires opening up a potentially large number
> > of objects. Instead, it is more convenient to feed the updated refs to
>
> s/refs/.../
>
> > the commit-graph machinery, and let it throw out refs that don't point
>
> s/refs/.../
>
> > to commits.
> >
> > Introduce '--[no-]check-oids' to make such a behavior possible. With
> > '--check-oids' (the default behavior to retain backwards compatibility),
> > 'git commit-graph write' will barf on a non-commit line in its input.
> > With 'no-check-oids', such lines will be silently ignored, making the
>
> s/no-check-oids/--no-check-oids/
>
> > above possible by specifying this option.
> >
> > No matter which is supplied, 'git commit-graph write' retains the
> > behavior from the previous commit of rejecting non-OID inputs like
> > "HEAD" and "refs/heads/foo" as before.
>
> See? :) This is why all those s/// are necessary.
Ah :). All good points; thanks for your suggestions.
> > Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx>
> > ---
> > Documentation/git-commit-graph.txt | 5 +++++
> > builtin/commit-graph.c | 11 ++++++++---
> > commit-graph.c | 2 +-
> > t/t5318-commit-graph.sh | 28 ++++++++++++++++++++++++++++
> > 4 files changed, 42 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt
> > index 46f7f7c573..91e8027b86 100644
> > --- a/Documentation/git-commit-graph.txt
> > +++ b/Documentation/git-commit-graph.txt
> > @@ -82,6 +82,11 @@ tip with the previous tip.
> > Finally, if `--expire-time=<datetime>` is not specified, let `datetime`
> > be the current time. After writing the split commit-graph, delete all
> > unused commit-graph whose modified times are older than `datetime`.
> > ++
> > +The `--[no-]check-oids` option decides whether or not OIDs are required
> > +to be commits. By default, `--check-oids` is implied, generating an
> > +error on non-commit objects. If `--no-check-oids` is given, non-commits
> > +are silently discarded.
>
> What happens with OIDs of tags, in particular with OIDs of tags that
> can be peeled down to commit objects? According to (my (too
> pedantic?) interpretation of) the above description they will trigger
> an error with '--check-oids' or will be ignored with
> '--no-check-oids'. The implementation, however, accepts those oids
> and peels them down to commit objects; I think this is the right
> behaviour.
That's right, and certainly merits a mention in the documentation. I've
added that...
> What happens with OIDs that name non-existing objects?
...these are silently discarded. I think that you could make a
compelling argument in either direction on this one, but I'm slightly
swayed towards "discard these, too", since '--no-check-oids' is
literally saying "don't check these".
I guess that pushes us into the territory of whether or not "check" is
the right verb. "verify"? "scrutinize" :)? Do you have any thoughts
here?
If you're otherwise satisfied with this series, here's the updated
patch.
-- >8 --
Subject: [PATCH] commit-graph.c: introduce '--[no-]check-oids'
When operating on a stream of commit OIDs on stdin, 'git commit-graph
write' checks that each OID refers to an object that is indeed a commit.
This is convenient to make sure that the given input is well-formed, but
can sometimes be undesirable.
For example, server operators may wish to feed the full commit object
IDs pointed to by refs that were updated during a push to 'git
commit-graph write --input=stdin-commits', and silently discard any
input that doesn't point at a commit. This can be done by combing the
output of 'git for-each-ref' with '--format %(*objecttype)', but this
requires opening up a potentially large number of objects. Instead, it
is more convenient to feed the updated object IDs to the commit-graph
machinery, and let it throw out whatever remains. to commits.
Introduce '--[no-]check-oids' to make such a behavior possible. With
'--check-oids' (the default behavior to retain backwards compatibility),
'git commit-graph write' will barf on a non-commit line in its input.
With '--no-check-oids', such lines will be silently ignored, making the
above possible by specifying this option.
No matter which is supplied, 'git commit-graph write' retains the
behavior from the previous commit of rejecting non-OID inputs like
"HEAD" and "refs/heads/foo" as before.
Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx>
---
Documentation/git-commit-graph.txt | 6 ++++++
builtin/commit-graph.c | 11 ++++++++---
t/t5318-commit-graph.sh | 28 ++++++++++++++++++++++++++++
3 files changed, 42 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt
index 46f7f7c573..6bdbe42766 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -82,6 +82,12 @@ tip with the previous tip.
Finally, if `--expire-time=<datetime>` is not specified, let `datetime`
be the current time. After writing the split commit-graph, delete all
unused commit-graph whose modified times are older than `datetime`.
++
+The `--[no-]check-oids` option decides whether or not OIDs are required
+to be commits. By default, `--check-oids` is implied, generating an
+error on non-commit objects. If `--no-check-oids` is given, non-commits
+and non-existent objects are silently discarded. In either case, tags
+are peeled down to the object they reference.
'verify'::
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index c69716aa7e..2d0a8e822a 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -11,7 +11,7 @@ static char const * const builtin_commit_graph_usage[] = {
N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]"),
N_("git commit-graph write [--object-dir <objdir>] [--append] "
"[--split[=<strategy>]] [--reachable|--stdin-packs|--stdin-commits] "
- "[--[no-]progress] <split options>"),
+ "[--[no-]progress] [--[no-]check-oids] <split options>"),
NULL
};
@@ -23,7 +23,7 @@ static const char * const builtin_commit_graph_verify_usage[] = {
static const char * const builtin_commit_graph_write_usage[] = {
N_("git commit-graph write [--object-dir <objdir>] [--append] "
"[--split[=<strategy>]] [--reachable|--stdin-packs|--stdin-commits] "
- "[--[no-]progress] <split options>"),
+ "[--[no-]progress] [--[no-]check-oids] <split options>"),
NULL
};
@@ -36,6 +36,7 @@ static struct opts_commit_graph {
int split;
int shallow;
int progress;
+ int check_oids;
} opts;
static struct object_directory *find_odb(struct repository *r,
@@ -163,6 +164,8 @@ static int graph_write(int argc, const char **argv)
N_("allow writing an incremental commit-graph file"),
PARSE_OPT_OPTARG | PARSE_OPT_NONEG,
write_option_parse_split),
+ OPT_BOOL(0, "check-oids", &opts.check_oids,
+ N_("require OIDs to be commits")),
OPT_INTEGER(0, "max-commits", &split_opts.max_commits,
N_("maximum number of commits in a non-base split commit-graph")),
OPT_INTEGER(0, "size-multiple", &split_opts.size_multiple,
@@ -173,6 +176,7 @@ static int graph_write(int argc, const char **argv)
};
opts.progress = isatty(2);
+ opts.check_oids = 1;
split_opts.size_multiple = 2;
split_opts.max_commits = 0;
split_opts.expire_time = 0;
@@ -227,7 +231,8 @@ static int graph_write(int argc, const char **argv)
oidset_insert(&commits, &oid);
}
- flags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;
+ if (opts.check_oids)
+ flags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;
}
UNLEAK(buf);
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index e874a12696..7960cefa1b 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -49,6 +49,34 @@ test_expect_success 'exit with correct error on bad input to --stdin-commits' '
test_i18ngrep "invalid commit object id" stderr
'
+graph_expect_commits() {
+ test-tool read-graph >got
+ if ! grep "num_commits: $1" got
+ then
+ echo "graph_expect_commits: expected $1 commit(s), got:"
+ cat got
+ false
+ fi
+}
+
+test_expect_success 'ignores non-commit OIDs to --input=stdin-commits with --no-check-oids' '
+ test_when_finished rm -rf "$objdir/info/commit-graph" &&
+ cd "$TRASH_DIRECTORY/full" &&
+ # write a graph to ensure layers are/are not added appropriately
+ git rev-parse HEAD~1 >base &&
+ git commit-graph write --stdin-commits <base &&
+ graph_expect_commits 2 &&
+ # bad input is rejected
+ echo HEAD >bad &&
+ test_expect_code 1 git commit-graph write --stdin-commits <bad 2>err &&
+ test_i18ngrep "unexpected non-hex object ID: HEAD" err &&
+ graph_expect_commits 2 &&
+ # update with valid commit OID, ignore tree OID
+ git rev-parse HEAD HEAD^{tree} >in &&
+ git commit-graph write --stdin-commits --no-check-oids <in &&
+ graph_expect_commits 3
+'
+
graph_git_two_modes() {
git -c core.commitGraph=true $1 >output
git -c core.commitGraph=false $1 >expect
--
2.26.0.113.ge9739cdccc
