{"thread":{"id":"59464","subject":"[PATCH v2] blame: allow --contents to work with non-HEAD commit","startedAt":"2023-03-24T05:54:51Z","lastAt":"2023-03-24T15:30:36Z","messageCount":2,"participants":["Jacob Keller","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"474075","messageId":"20230324055437.297401-1-jacob.e.keller@intel.com","threadId":"59464","inReplyTo":null,"subject":"[PATCH v2] blame: allow --contents to work with non-HEAD commit","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2023-03-24T05:54:37Z","receivedAt":"2023-03-24T05:54:51Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nThe --contents option can be used with git blame to blame the file as if\nit had the contents from the specified file. This is akin to copying the\ncontents into the working tree and then running git blame. This option\nhas been supported since 1cfe77333f27 (\"git-blame: no rev means start\nfrom the working tree file.\")\n\nThe --contents option always blames the file as if it was based on the\ncurrent HEAD commit. If you try to pass a revision while using\n--contents, you get the following error:\n\n  fatal: cannot use --contents with final commit object name\n\nThis is because the blame process generates a fake working tree commit\nwhich always uses the HEAD object as its sole parent.\n\nEnhance fake_working_tree_commit to take the object ID to use for the\nparent instead of always using the HEAD object. Then, always generate a\nfake commit when we have contents provided, even if we have a final\nobject. Remove the check to disallow --contents and a final revision.\n\nThis enables use of --contents with an arbitrary revision, rather than\nforcing the use of the local HEAD commit. This makes the --contents\noption significantly more flexible, as it is no longer required to check\nout the working tree to the desired commit before using --contents.\n\nReword the documentation so that its clear that --contents can be used\nwith <rev>.\n\nAdd tests for the --contents option to the annotate-tests.sh test\nscript.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\nChanges since v1:\n* Reword commit message slightly, based on Junio's review comments.\n\nI ran into this because I use --contents in a process I'm working on for\ncomparing differences between two forks of a project. I use blame --contents\nwhere I point blame to the contents from the other repository. It is useful\nto be able to do with at arbitrary commits. Currently I have to switch the\nworking tree to the commit rather than being able to target a commit by its\noid.\n\nWith this change I can more easily run this process without the need to\nactually check the contents out in the working tree. Its relatively simple\nto make --contents work with a revision, since I just need to generate the\nfake contents starting from that revision instead of starting from HEAD.\n\nI suspect this may make it easier to get --reverse to work with --contents,\nbut I haven't investigated doing so.\n\n Documentation/blame-options.txt |  9 ++++-----\n Documentation/git-blame.txt     |  2 +-\n blame.c                         | 27 ++++++++++++++++-----------\n t/annotate-tests.sh             | 14 ++++++++++++++\n 4 files changed, 35 insertions(+), 17 deletions(-)\n\ndiff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\nindex 9a663535f443..6476dd327377 100644\n--- a/Documentation/blame-options.txt\n+++ b/Documentation/blame-options.txt\n@@ -64,11 +64,10 @@ include::line-range-format.txt[]\n \tmanual page.\n \n --contents <file>::\n-\tWhen <rev> is not specified, the command annotates the\n-\tchanges starting backwards from the working tree copy.\n-\tThis flag makes the command pretend as if the working\n-\ttree copy has the contents of the named file (specify\n-\t`-` to make the command read from the standard input).\n+\tPretend the file being annotated has the contents from the named\n+\tfile instead of using the contents of <rev> or the working tree\n+\tcopy. You may specify '-' to make the command read from standard\n+\tinput for the file contents.\n \n --date <format>::\n \tSpecifies the format used to output dates. If --date is not\ndiff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt\nindex 4400a17330b4..f69a871a96f7 100644\n--- a/Documentation/git-blame.txt\n+++ b/Documentation/git-blame.txt\n@@ -12,7 +12,7 @@ SYNOPSIS\n \t    [-L <range>] [-S <revs-file>] [-M] [-C] [-C] [-C] [--since=<date>]\n \t    [--ignore-rev <rev>] [--ignore-revs-file <file>]\n \t    [--color-lines] [--color-by-age] [--progress] [--abbrev=<n>]\n-\t    [<rev> | --contents <file> | --reverse <rev>..<rev>] [--] <file>\n+\t    [ --contents <file> ] [<rev> | --reverse <rev>..<rev>] [--] <file>\n \n DESCRIPTION\n -----------\ndiff --git a/blame.c b/blame.c\nindex e45d8a3bf92a..52fca5a7f5b7 100644\n--- a/blame.c\n+++ b/blame.c\n@@ -177,12 +177,12 @@ static void set_commit_buffer_from_strbuf(struct repository *r,\n static struct commit *fake_working_tree_commit(struct repository *r,\n \t\t\t\t\t       struct diff_options *opt,\n \t\t\t\t\t       const char *path,\n-\t\t\t\t\t       const char *contents_from)\n+\t\t\t\t\t       const char *contents_from,\n+\t\t\t\t\t       struct object_id *oid)\n {\n \tstruct commit *commit;\n \tstruct blame_origin *origin;\n \tstruct commit_list **parent_tail, *parent;\n-\tstruct object_id head_oid;\n \tstruct strbuf buf = STRBUF_INIT;\n \tconst char *ident;\n \ttime_t now;\n@@ -198,10 +198,7 @@ static struct commit *fake_working_tree_commit(struct repository *r,\n \tcommit->date = now;\n \tparent_tail = &commit->parents;\n \n-\tif (!resolve_ref_unsafe(\"HEAD\", RESOLVE_REF_READING, &head_oid, NULL))\n-\t\tdie(\"no such ref: HEAD\");\n-\n-\tparent_tail = append_parent(r, parent_tail, &head_oid);\n+\tparent_tail = append_parent(r, parent_tail, oid);\n \tappend_merge_parents(r, parent_tail);\n \tverify_working_tree_path(r, commit, path);\n \n@@ -2772,22 +2769,30 @@ void setup_scoreboard(struct blame_scoreboard *sb,\n \t\tsb->commits.compare = compare_commits_by_reverse_commit_date;\n \t}\n \n-\tif (sb->final && sb->contents_from)\n-\t\tdie(_(\"cannot use --contents with final commit object name\"));\n-\n \tif (sb->reverse && sb->revs->first_parent_only)\n \t\tsb->revs->children.name = NULL;\n \n-\tif (!sb->final) {\n+\tif (sb->contents_from || !sb->final) {\n+\t\tstruct object_id head_oid, *parent_oid;\n+\n \t\t/*\n \t\t * \"--not A B -- path\" without anything positive;\n \t\t * do not default to HEAD, but use the working tree\n \t\t * or \"--contents\".\n \t\t */\n+\t\tif (sb->final) {\n+\t\t\tparent_oid = &sb->final->object.oid;\n+\t\t} else {\n+\t\t\tif (!resolve_ref_unsafe(\"HEAD\", RESOLVE_REF_READING, &head_oid, NULL))\n+\t\t\t\tdie(\"no such ref: HEAD\");\n+\t\t\tparent_oid = &head_oid;\n+\t\t}\n+\n \t\tsetup_work_tree();\n \t\tsb->final = fake_working_tree_commit(sb->repo,\n \t\t\t\t\t\t     &sb->revs->diffopt,\n-\t\t\t\t\t\t     sb->path, sb->contents_from);\n+\t\t\t\t\t\t     sb->path, sb->contents_from,\n+\t\t\t\t\t\t     parent_oid);\n \t\tadd_pending_object(sb->revs, &(sb->final->object), \":\");\n \t}\n \ndiff --git a/t/annotate-tests.sh b/t/annotate-tests.sh\nindex f1b9a6ce4dae..b35be20cf327 100644\n--- a/t/annotate-tests.sh\n+++ b/t/annotate-tests.sh\n@@ -72,6 +72,16 @@ test_expect_success 'blame 1 author' '\n \tcheck_count A 2\n '\n \n+test_expect_success 'blame with --contents' '\n+\tcheck_count --contents=file A 2\n+'\n+\n+test_expect_success 'blame with --contents changed' '\n+\techo \"1A quick brown fox jumps over the\" >contents &&\n+\techo \"another lazy dog\" >>contents &&\n+\tcheck_count --contents=contents A 1 \"Not Committed Yet\" 1\n+'\n+\n test_expect_success 'blame in a bare repo without starting commit' '\n \tgit clone --bare . bare.git &&\n \t(\n@@ -98,6 +108,10 @@ test_expect_success 'blame 2 authors' '\n \tcheck_count A 2 B 2\n '\n \n+test_expect_success 'blame with --contents and revision' '\n+\tcheck_count -h testTag --contents=file A 2 \"Not Committed Yet\" 2\n+'\n+\n test_expect_success 'setup B1 lines (branch1)' '\n \tgit checkout -b branch1 main &&\n \techo \"3A slow green fox jumps into the\" >>file &&\n-- \n2.40.0.131.g970374df36d8\n\n"},{"id":"474095","messageId":"xmqqpm8y4161.fsf@gitster.g","threadId":"59464","inReplyTo":"20230324055437.297401-1-jacob.e.keller@intel.com","subject":"Re: [PATCH v2] blame: allow --contents to work with non-HEAD commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-24T15:30:30Z","receivedAt":"2023-03-24T15:30:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller <jacob.e.keller@intel.com> writes:\n\n> This is because the blame process generates a fake working tree commit\n> which always uses the HEAD object as its sole parent.\n>\n> Enhance fake_working_tree_commit to take the object ID to use for the\n> parent instead of always using the HEAD object. Then, always generate a\n> fake commit when we have contents provided, even if we have a final\n> object. Remove the check to disallow --contents and a final revision.\n\nAround here, probably in between the two paragraphs, it makes sense\nto explain why we do this enhancement only for --contents but not\nfor the case where we take contents from the working tree file (that\nwould ensure what I wrote will not go waste in my review ;-).\n\n> diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\n> index 9a663535f443..6476dd327377 100644\n> --- a/Documentation/blame-options.txt\n> +++ b/Documentation/blame-options.txt\n> @@ -64,11 +64,10 @@ include::line-range-format.txt[]\n>  \tmanual page.\n>  \n>  --contents <file>::\n> +\tPretend the file being annotated has the contents from the named\n> +\tfile instead of using the contents of <rev> or the working tree\n> +\tcopy. You may specify '-' to make the command read from standard\n> +\tinput for the file contents.\n\nHmph, I can sort of see that \"or the working tree copy\" refers to\nthe behaviour when <rev> is not given, but I wonder if it makes it\neasier to understand to explicitly say that missing <rev> (or having\nno positive end) defaults to \"HEAD\" when choosing the parent of the\nfake commit with the given contents in it.\n\n> @@ -198,10 +198,7 @@ static struct commit *fake_working_tree_commit(struct repository *r,\n>  \tcommit->date = now;\n>  \tparent_tail = &commit->parents;\n>  \n> -\tif (!resolve_ref_unsafe(\"HEAD\", RESOLVE_REF_READING, &head_oid, NULL))\n> -\t\tdie(\"no such ref: HEAD\");\n> -\n> -\tparent_tail = append_parent(r, parent_tail, &head_oid);\n> +\tparent_tail = append_parent(r, parent_tail, oid);\n\nGood.  As fake_working_tree_commit() is no longer about creating a\nfake commit based on \"HEAD\", we do not interpret \"HEAD\" here, and\ninstead the caller is now responsible for feeding the fake parent\nobject name to us.\n\n> @@ -2772,22 +2769,30 @@ void setup_scoreboard(struct blame_scoreboard *sb,\n>  \t\tsb->commits.compare = compare_commits_by_reverse_commit_date;\n>  \t}\n>  \n> -\tif (sb->final && sb->contents_from)\n> -\t\tdie(_(\"cannot use --contents with final commit object name\"));\n> -\n\nOK.\n\n> +\tif (sb->contents_from || !sb->final) {\n> +\t\tstruct object_id head_oid, *parent_oid;\n> +\n>  \t\t/*\n>  \t\t * \"--not A B -- path\" without anything positive;\n>  \t\t * do not default to HEAD, but use the working tree\n>  \t\t * or \"--contents\".\n>  \t\t */\n\nThis comment is no longer valid, as this block handles more cases now:\n\n\t/*\n         * Build a fake commit at the tip of the history, when\n         * (1) \"git blame ^A ^B -- path\", i.e. without any positive\n\t *     end of the history range, in which case we build such\n\t *     a fake commit on top of the HEAD to blame in-tree\n\t *     modifications.\n\t * (2) \"git blame --contents=file [A] -- path\", with or without\n\t *     positive end of the history range but with --contents,\n\t *     in which case we pretend that there is a fake commit\n         *     on top of the positive end (defaults to HEAD) that\n         *     has the given contents in the path.\n\t */\n\n> +\t\tif (sb->final) {\n> +\t\t\tparent_oid = &sb->final->object.oid;\n> +\t\t} else {\n> +\t\t\tif (!resolve_ref_unsafe(\"HEAD\", RESOLVE_REF_READING, &head_oid, NULL))\n> +\t\t\t\tdie(\"no such ref: HEAD\");\n> +\t\t\tparent_oid = &head_oid;\n> +\t\t}\n> +\n>  \t\tsetup_work_tree();\n>  \t\tsb->final = fake_working_tree_commit(sb->repo,\n>  \t\t\t\t\t\t     &sb->revs->diffopt,\n> -\t\t\t\t\t\t     sb->path, sb->contents_from);\n> +\t\t\t\t\t\t     sb->path, sb->contents_from,\n> +\t\t\t\t\t\t     parent_oid);\n>  \t\tadd_pending_object(sb->revs, &(sb->final->object), \":\");\n>  \t}\n\nOther than that, looking good.\n\nThanks.\n"}]}