From: Junio C Hamano Date: Fri, 24 Mar 2023 15:30:30 GMT Subject: Re: [PATCH v2] blame: allow --contents to work with non-HEAD commit Message-ID: In-Reply-To: <20230324055437.297401-1-jacob.e.keller@intel.com> Jacob Keller writes: > This is because the blame process generates a fake working tree commit > which always uses the HEAD object as its sole parent. > > Enhance fake_working_tree_commit to take the object ID to use for the > parent instead of always using the HEAD object. Then, always generate a > fake commit when we have contents provided, even if we have a final > object. Remove the check to disallow --contents and a final revision. Around here, probably in between the two paragraphs, it makes sense to explain why we do this enhancement only for --contents but not for the case where we take contents from the working tree file (that would ensure what I wrote will not go waste in my review ;-). > diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt > index 9a663535f443..6476dd327377 100644 > --- a/Documentation/blame-options.txt > +++ b/Documentation/blame-options.txt > @@ -64,11 +64,10 @@ include::line-range-format.txt[] > manual page. > > --contents :: > + Pretend the file being annotated has the contents from the named > + file instead of using the contents of or the working tree > + copy. You may specify '-' to make the command read from standard > + input for the file contents. Hmph, I can sort of see that "or the working tree copy" refers to the behaviour when is not given, but I wonder if it makes it easier to understand to explicitly say that missing (or having no positive end) defaults to "HEAD" when choosing the parent of the fake commit with the given contents in it. > @@ -198,10 +198,7 @@ static struct commit *fake_working_tree_commit(struct repository *r, > commit->date = now; > parent_tail = &commit->parents; > > - if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &head_oid, NULL)) > - die("no such ref: HEAD"); > - > - parent_tail = append_parent(r, parent_tail, &head_oid); > + parent_tail = append_parent(r, parent_tail, oid); Good. As fake_working_tree_commit() is no longer about creating a fake commit based on "HEAD", we do not interpret "HEAD" here, and instead the caller is now responsible for feeding the fake parent object name to us. > @@ -2772,22 +2769,30 @@ void setup_scoreboard(struct blame_scoreboard *sb, > sb->commits.compare = compare_commits_by_reverse_commit_date; > } > > - if (sb->final && sb->contents_from) > - die(_("cannot use --contents with final commit object name")); > - OK. > + if (sb->contents_from || !sb->final) { > + struct object_id head_oid, *parent_oid; > + > /* > * "--not A B -- path" without anything positive; > * do not default to HEAD, but use the working tree > * or "--contents". > */ This comment is no longer valid, as this block handles more cases now: /* * Build a fake commit at the tip of the history, when * (1) "git blame ^A ^B -- path", i.e. without any positive * end of the history range, in which case we build such * a fake commit on top of the HEAD to blame in-tree * modifications. * (2) "git blame --contents=file [A] -- path", with or without * positive end of the history range but with --contents, * in which case we pretend that there is a fake commit * on top of the positive end (defaults to HEAD) that * has the given contents in the path. */ > + if (sb->final) { > + parent_oid = &sb->final->object.oid; > + } else { > + if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &head_oid, NULL)) > + die("no such ref: HEAD"); > + parent_oid = &head_oid; > + } > + > setup_work_tree(); > sb->final = fake_working_tree_commit(sb->repo, > &sb->revs->diffopt, > - sb->path, sb->contents_from); > + sb->path, sb->contents_from, > + parent_oid); > add_pending_object(sb->revs, &(sb->final->object), ":"); > } Other than that, looking good. Thanks.