Derrick Stolee writes:
First, this commit seems to try to do two things at once, and in its
current form it should be split into adding destroy_commit_graph() and
into grafts / replacements check. Those are separate and unconnected
features, and should be in separate patches, in my opinion.
> Create destroy_commit_graph() method to delete the commit-graph file
> when history is altered by a replace-object call. If the commit-graph
> is rebuilt after that, we will load the correct object while reading
> the commit-graph.
I'm not sure if this is the best solution. It would work both for
invalidation, and for turning off the feature, but in my opinion there
are better solutions for both:
- in case of invalidation, wouldn't it be better to re-create the file?
- in case of turning off, wouldn't it be better to simply set
core_commit_graph to false?
Nevertheless I think destroy_commit_graph(), however it would be named
at the end, would be a good to have.
> When parsing a commit, first check if the commit was grafted. If so,
> then ignore the commit-graph for that commit and insted use the parents
> loaded by parsing the commit buffer and comparing against the graft
> file.
Unless uyou turn off using generation numbers and other such indices,
this is not enough to prevent from generating incorrect results if
current commit-graph file doesn't agree with current altered view of the
project history.
Take a simple example of joining two histories using replacements
(git-replace) or grafts. Numbers denote generation numbers:
Original history:
A<---B<---C <=== master
123
a<---b<---c<---d<---e <=== historical
12345
Altered view of history:
a<---b<---c<---d<---e<---[A]<---B<---C <=== master
In the new view of history, 'e' is reachable from 'C'. But going by
pre-altering generation numbers, gen_{pre}(e) = 5 > gen_{pre}(C) = 3.
This means that we don't even arrive at '[A]', where replacement object
or graft changed parents compared to what's loaded from commit-graph
file, isn't it?
>
> Signed-off-by: Derrick Stolee
> ---
> builtin/replace.c | 3 +++
> commit-graph.c| 20 +++-
> commit-graph.h| 9 +
> commit.c | 5 +
> 4 files changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/replace.c b/builtin/replace.c
> index 9f01f3fc7f..d553aadcdc 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -15,6 +15,7 @@
> #include "parse-options.h"
> #include "run-command.h"
> #include "tag.h"
> +#include "commit-graph.h"
>
> static const char * const git_replace_usage[] = {
> N_("git replace [-f] "),
> @@ -468,6 +469,8 @@ int cmd_replace(int argc, const char **argv, const char
> *prefix)
> usage_msg_opt("--raw only makes sense with --edit",
> git_replace_usage, options);
>
> + destroy_commit_graph(get_object_directory());
> +
> switch (cmdmode) {
> case MODE_DELETE:
> if (argc < 1)
> diff --git a/commit-graph.c b/commit-graph.c
> index e9195dfb17..95af4ed519 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -6,6 +6,7 @@
> #include "pack.h"
> #include "packfile.h"
> #include "commit.h"
> +#include "dir.h"
> #include "object.h"
> #include "refs.h"
> #include "revision.h"
> @@ -240,15 +241,22 @@ static struct commit_list **insert_parent_or_die(struct
> commit_graph *g,
> {
> struct commit *c;
> struct object_id oid;
> + const unsigned char *real;
>
> if (pos >= g->num_commits)
> die("invalid parent position %"PRIu64, pos);
>
> hashcpy(oid.hash, g->chunk_oid_lookup + g->hash_len * pos);
> +
> + real = lookup_replace_object(oid.hash);
So this function returns either original SHA-1 / OID of object, or if
object should be replaced then it returns the replacement object's name
(replaced recursively, if necessary).
This deals with replacements, but not with grafts (from graft file)!
> +
> c = lookup_commit(&oid);
> if (!c)
> die("could not find commit %s", oid_to_hex(&oid));
> - c->graph_pos = pos;
> +
> + if (!hashcmp(real, oid.hash))
> + c->graph_pos = pos;
I don't quite understand this reasoning here. The serialized
commit-graph file can either follow the current state of altered
history, or not. In the later case the commit-graph can either follow
original history, or altered but not current view of history.
What does it matter of the object was replaced or not when marking
object as coming from graph at position 'pos'? If commit is replaced,
it still comes from commit-graph at position 'pos', isn't it?
Also, what if the starting commit was replaced? Then this logic
wouldn't file, is it?
I don't see how it relates to what's written in the commit message:
DS> When parsing a commit, first check if the commit was grafted. If so,
DS> then ignore the commit-graph for that comm
