Re: Bug report: git branch behaves as if --no-replace-objects is passed

From

Jeff King <peff@peff.net>

Date

Mar 30, 2021, 07:02 UTC

Message-ID

<YGLNBFJv8NKmrbvz@coredump.intra.peff.net>

In-Reply-To

<CABPp-BEAbN05+hCtK=xhGg5uZFqbUvH9hMcCNMcBWp5JWLqzPw@mail.gmail.com>

On Mon, Mar 29, 2021 at 11:05:05PM -0700, Elijah Newren wrote:

> it all works well.  BUT, if I try to use it with branch it doesn't work:
> 
>     $ git branch --contains deadbeefdeadbeefdeadbeefdeadbeefdeadbeef
>     $

diff --git a/parse-options-cb.c b/parse-options-cb.c
index 4542d4d3f9..200be4e3d2 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -6,6 +6,7 @@
 #include "string-list.h"
 #include "strvec.h"
 #include "oid-array.h"
+#include "replace-object.h"
 
 /*----- some often used options -----*/
 
@@ -84,6 +85,7 @@ int parse_opt_verbosity_cb(const struct option *opt, const char *arg,
 int parse_opt_commits(const struct option *opt, const char *arg, int unset)
 {
 	struct object_id oid;
+	const struct object_id *replace;
 	struct commit *commit;
 
 	BUG_ON_OPT_NEG(unset);
@@ -92,7 +94,9 @@ int parse_opt_commits(const struct option *opt, const char *arg, int unset)
 		return -1;
 	if (get_oid(arg, &oid))
 		return error("malformed object name %s", arg);
-	commit = lookup_commit_reference(the_repository, &oid);
+
+	replace = lookup_replace_object(the_repository, &oid);
+	commit = lookup_commit_reference(the_repository, replace);
 	if (!commit)
 		return error("no such commit %s", arg);
 	commit_list_insert(commit, opt->value);

though if we go that route, I suspect we ought to be adding both the
original _and_ the replacement.

I'm not entirely sure this is a good direction, though.

> and possibly worse, if I create a new branch based on it and use it:
> 
>     $ git branch foobar deadbeefdeadbeefdeadbeefdeadbeefdeadbeef
>     $ git checkout foobar
>     $ echo stuff >empty
>     $ git add empty
>     $ git commit -m more
> 
> then it's clear that branch created foobar pointing to the replaced
> object rather than the replacement object -- despite the fact that the
> replaced object doesn't even exist within this repo:
> 
>     $ git cat-file -p HEAD
>     tree 18108bae26dc91af2055bc66cc9fea278012dbd3
>     parent deadbeefdeadbeefdeadbeefdeadbeefdeadbeef
>     author Elijah Newren <newren@gmail.com> 1617083739 -0700
>     committer Elijah Newren <newren@gmail.com> 1617083739 -0700
> 
>     more

Yeah, that's pretty horrible. I do think you're using replace objects in
a way they weren't really intended for, in two ways:

  - usually you'd actually have deadbeef, and you just want to rewrite
    it to something else

  - you wouldn't usually work directly with the replace object on the
    command line like this. Usually the intent is to patch some old
    section of history to get a different view.

None of which is an excuse exactly. But just to say that I'm not too
surprised that the "replace" mechanism is a bit half-baked, and there
are probably a lot of weird implications nobody has thought through.

Patches welcome, of course, though I suspect it may be a rabbit hole
that isn't worth your time. :)

> I poked around in the code a little but it is not at all clear to me
> why some parts of the code (log, diff) translate replace refs
> correctly, while others (branch) don't.  It is clear from the output
> that log is aware that the refs are replaced, which makes me wonder if
> every caller needs to be aware of replace refs for them to work
> correctly everywhere, because I couldn't find a missing environment
> setup for "branch".

Right. To work as you expect above, basically everything that is going
to look at an oid would need to consider whether to use a replace ref.
Shoving the lookup into get_oid() would "fix" that, but I suspect would
confuse some other callers. Right now the rule is "when you access the
object contents, pretend as if it had X instead of Y", so we can enforce
that at the layer of accessing the object database.

-Peff