{"thread":{"id":"59468","subject":"[PATCH v3] blame: allow --contents to work with non-HEAD commit","startedAt":"2023-03-24T17:08:17Z","lastAt":"2023-03-31T00:57:17Z","messageCount":4,"participants":["Jacob Keller","Junio C Hamano","Glen Choo"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"474100","messageId":"20230324170800.331022-1-jacob.e.keller@intel.com","threadId":"59468","inReplyTo":null,"subject":"[PATCH v3] blame: allow --contents to work with non-HEAD commit","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2023-03-24T17:08:00Z","receivedAt":"2023-03-24T17:08:17Z","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\nNote that the behavior of generating a fake working commit is still\nskipped when a revision is provided but --contents is not provided.\nGenerating such a commit in that case would combine the currently\nchecked out file contents with the provided revision, which breaks\nnormal blame behavior and produces unexpected results.\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\nChanges since v2:\n* Improved commit message further\n* Re-wrote documentation for --contents. I really had trouble figuring out a\n  good succinct way to explain the behavior, so hopefully this is good.\n* Updated the comment in setup_scoreboard.\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\n\n Documentation/blame-options.txt | 10 ++++-----\n Documentation/git-blame.txt     |  2 +-\n blame.c                         | 40 +++++++++++++++++++++------------\n t/annotate-tests.sh             | 14 ++++++++++++\n 4 files changed, 46 insertions(+), 20 deletions(-)\n\ndiff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\nindex 9a663535f443..95599bd6e5f4 100644\n--- a/Documentation/blame-options.txt\n+++ b/Documentation/blame-options.txt\n@@ -64,11 +64,11 @@ 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 a commit with the\n+\tcontents from the named file and a parent of <rev>,\n+\tdefaulting to HEAD when no <rev> is specified. You may\n+\tspecify '-' to make the command read from the 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..2d02cf0636ca 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,37 @@ 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 * Build a fake commit at the top of the history, when\n+\t\t * (1) \"git blame ^A ^B --path\", i.e. without any positive end\n+\t\t *     end of the history range, in which case we build such\n+\t\t *     a fake commit on top of the HEAD to blame in-tree\n+\t\t *     modifications.\n+\t\t * (2) \"git blame --contents=file [A] -- path\", with or\n+\t\t *     without positive end of the history range but with\n+\t\t *     --contents, in which case we pretend that there is\n+\t\t *     a fake commit on top of the positive end (defaulting to\n+\t\t *     HEAD) that has the given contents in the path.\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":"474113","messageId":"xmqq5yaqezlh.fsf@gitster.g","threadId":"59468","inReplyTo":"20230324170800.331022-1-jacob.e.keller@intel.com","subject":"Re: [PATCH v3] blame: allow --contents to work with non-HEAD commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-24T19:08:58Z","receivedAt":"2023-03-24T19:09:03Z","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> Changes since v2:\n> * Improved commit message further\n> * Re-wrote documentation for --contents. I really had trouble figuring out a\n>   good succinct way to explain the behavior, so hopefully this is good.\n> * Updated the comment in setup_scoreboard.\n\nLooking good.  Will queue.  Thanks for a quick update.\n\n"},{"id":"474488","messageId":"kl6l4jq2c6h9.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59468","inReplyTo":"20230324170800.331022-1-jacob.e.keller@intel.com","subject":"Re: [PATCH v3] blame: allow --contents to work with non-HEAD commit","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-03-30T20:46:42Z","receivedAt":"2023-03-30T20:46:57Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Hi Jacob! Great to see you at Review Club :)\n\nIf you'd like, yo ucan review the notes here:\n\n  https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w/edit\n\nbut as always, all important discussion happens on the ML.\n\nI see that this patch is already queued for 'master' (which is fine),\nthough I think it might even better with another patch on top.\n\nJacob Keller <jacob.e.keller@intel.com> writes:\n\n> From: Jacob Keller <jacob.keller@gmail.com>\n>\n> The --contents option can be used with git blame to blame the file as if\n> it had the contents from the specified file. This is akin to copying the\n> contents into the working tree and then running git blame. This option\n> has been supported since 1cfe77333f27 (\"git-blame: no rev means start\n> from the working tree file.\")\n>\n> The --contents option always blames the file as if it was based on the\n> current 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>\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\nI thought that the commit message was very clear and provided enough\ncontext even for reviewers who weren't familiar with \"git blame\n--contents\". Thanks!\n\nI'll reorder the patch hunks to make discussion easier:\n\n> diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh\n> index f1b9a6ce4dae..b35be20cf327 100644\n> --- a/t/annotate-tests.sh\n> +++ b/t/annotate-tests.sh\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\nAs the test notes, the author of the changes is \"Not Committed Yet\"...\n\n> diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\n> index 9a663535f443..95599bd6e5f4 100644\n> --- a/Documentation/blame-options.txt\n> +++ b/Documentation/blame-options.txt\n> @@ -64,11 +64,11 @@ 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 a commit with the\n> +\tcontents from the named file and a parent of <rev>,\n> +\tdefaulting to HEAD when no <rev> is specified. You may\n> +\tspecify '-' to make the command read from the standard\n> +\tinput for the file contents.\n\nwhich I found quite difficult to reconcile with the description here,\nin particular:\n\n  Pretend the file being annotated has a commit...\n\nWe could try to make the two more coherent by rewording the docs, maybe:\n\n  Pretend that the working copy has the contents of the named file. If\n  <rev> is also given, also pretend that HEAD is at <rev>.\n\nBut (as Junio suggested in Review Club), maybe it would be better to\njust change \"Not Committed Yet\" to something more accurate, like\n\"External file (--contents)\", and then we can drop the language around\n\"pretend\".\n\nIt would be pretty simple, too. Here's a rough patch (that I don't mind\nyou sending as your own):\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----\ndiff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\nindex 95599bd6e5..4a861ff31c 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 a commit with the\n-\tcontents from the named file and a parent of <rev>,\n-\tdefaulting to HEAD when no <rev> is specified. You may\n-\tspecify '-' to make the command read from the standard\n-\tinput for the file contents.\n+\tAnnotate using the contents from the named file instead of the\n+\tworking tree copy, starting with <rev> if it is specified, and\n+\tHEAD otherwise. You may specify '-' to make the command read\n+\tfrom the standard input for the file contents.\n \n --date <format>::\n \tSpecifies the format used to output dates. If --date is not\ndiff --git a/blame.c b/blame.c\nindex 2d02cf0636..129dae7641 100644\n--- a/blame.c\n+++ b/blame.c\n@@ -204,8 +204,12 @@ static struct commit *fake_working_tree_commit(struct repository *r,\n \n \torigin = make_origin(commit, path);\n \n-\tident = fmt_ident(\"Not Committed Yet\", \"not.committed.yet\",\n-\t\t\tWANT_BLANK_IDENT, NULL, 0);\n+\tif (contents_from)\n+\t\tident = fmt_ident(\"External file (--contents)\", \"external.file\",\n+\t\t\t\tWANT_BLANK_IDENT, NULL, 0);\n+\telse\n+\t\tident = fmt_ident(\"Not Committed Yet\", \"not.committed.yet\",\n+\t\t\t\tWANT_BLANK_IDENT, NULL, 0);\n \tstrbuf_addstr(&msg, \"tree 0000000000000000000000000000000000000000\\n\");\n \tfor (parent = commit->parents; parent; parent = parent->next)\n \t\tstrbuf_addf(&msg, \"parent %s\\n\",\ndiff --git a/t/annotate-tests.sh b/t/annotate-tests.sh\nindex b35be20cf3..2ef70235b1 100644\n--- a/t/annotate-tests.sh\n+++ b/t/annotate-tests.sh\n@@ -72,6 +72,13 @@ test_expect_success 'blame 1 author' '\n \tcheck_count A 2\n '\n \n+test_expect_success 'blame working copy' '\n+\ttest_when_finished \"git restore file\" &&\n+\techo \"1A quick brown fox jumps over the\" >file &&\n+\techo \"another lazy dog\" >>file &&\n+\tcheck_count A 1 \"Not Committed Yet\" 1\n+'\n+\n test_expect_success 'blame with --contents' '\n \tcheck_count --contents=file A 2\n '\n@@ -79,7 +86,7 @@ test_expect_success 'blame with --contents' '\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+\tcheck_count --contents=contents A 1 \"External file (--contents)\" 1\n '\n \n test_expect_success 'blame in a bare repo without starting commit' '\n@@ -109,7 +116,7 @@ test_expect_success 'blame 2 authors' '\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+\tcheck_count -h testTag --contents=file A 2 \"External file (--contents)\" 2\n '\n \n test_expect_success 'setup B1 lines (branch1)' '\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----\n\nThe patch doesn't do anything special, except that we'd need to make\nsure to test that the working copy is annotated correctly (we didn't\nhave tests for this before??).\n\nThe doc and the fake commit author could use some tweaking, but IMO the\nend result is a lot easier to explain to end users, even in the case\nwhere \"--contents\" is given without <rev>.\n\nThe _one_ reason not to merge this is that this changes the annotated\nauthor with \"--contents\", which could cause trouble for end users who\nare parsing the output and expecting literal \"Not Committed Yet\". Given\nour history with Hyrum's law, surely _someone_ is doing this in the\nnon-\"--contents\" case, but I suspect that \"--contents\" is uncommon\nenough that we could take the chance.\n"},{"id":"474494","messageId":"CA+P7+xqC71LrDNao2oeaMdHh=dC+HMwr8G1_NNmsvTxOVzevZw@mail.gmail.com","threadId":"59468","inReplyTo":"kl6l4jq2c6h9.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v3] blame: allow --contents to work with non-HEAD commit","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2023-03-31T00:56:12Z","receivedAt":"2023-03-31T00:57:17Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Mar 30, 2023 at 1:46 PM Glen Choo <chooglen@google.com> wrote:\n>\n> Hi Jacob! Great to see you at Review Club :)\n>\n> If you'd like, yo ucan review the notes here:\n>\n>   https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w/edit\n>\n> but as always, all important discussion happens on the ML.\n>\n> I see that this patch is already queued for 'master' (which is fine),\n> though I think it might even better with another patch on top.\n>\n\nYep. I'm happy to send some follow up.\n\n> Jacob Keller <jacob.e.keller@intel.com> writes:\n>\n> > From: Jacob Keller <jacob.keller@gmail.com>\n> >\n> > The --contents option can be used with git blame to blame the file as if\n> > it had the contents from the specified file. This is akin to copying the\n> > contents into the working tree and then running git blame. This option\n> > has been supported since 1cfe77333f27 (\"git-blame: no rev means start\n> > from the working tree file.\")\n> >\n> > The --contents option always blames the file as if it was based on the\n> > current 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> >\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>\n> I thought that the commit message was very clear and provided enough\n> context even for reviewers who weren't familiar with \"git blame\n> --contents\". Thanks!\n>\n> I'll reorder the patch hunks to make discussion easier:\n>\n\nThanks!\n\n> > diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh\n> > index f1b9a6ce4dae..b35be20cf327 100644\n> > --- a/t/annotate-tests.sh\n> > +++ b/t/annotate-tests.sh\n> > @@ -98,6 +108,10 @@ test_expect_success 'blame 2 authors' '\n> >       check_count A 2 B 2\n> >  '\n> >\n> > +test_expect_success 'blame with --contents and revision' '\n> > +     check_count -h testTag --contents=file A 2 \"Not Committed Yet\" 2\n> > +'\n> > +\n>\n> As the test notes, the author of the changes is \"Not Committed Yet\"...\n>\n> > diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\n> > index 9a663535f443..95599bd6e5f4 100644\n> > --- a/Documentation/blame-options.txt\n> > +++ b/Documentation/blame-options.txt\n> > @@ -64,11 +64,11 @@ include::line-range-format.txt[]\n> >       manual page.\n> >\n> >  --contents <file>::\n> > -     When <rev> is not specified, the command annotates the\n> > -     changes starting backwards from the working tree copy.\n> > -     This flag makes the command pretend as if the working\n> > -     tree copy has the contents of the named file (specify\n> > -     `-` to make the command read from the standard input).\n> > +     Pretend the file being annotated has a commit with the\n> > +     contents from the named file and a parent of <rev>,\n> > +     defaulting to HEAD when no <rev> is specified. You may\n> > +     specify '-' to make the command read from the standard\n> > +     input for the file contents.\n>\n> which I found quite difficult to reconcile with the description here,\n> in particular:\n>\n>   Pretend the file being annotated has a commit...\n>\n> We could try to make the two more coherent by rewording the docs, maybe:\n>\n>   Pretend that the working copy has the contents of the named file. If\n>   <rev> is also given, also pretend that HEAD is at <rev>.\n>\n\nIt is quite tricky to get a concise description here.\n\n> But (as Junio suggested in Review Club), maybe it would be better to\n> just change \"Not Committed Yet\" to something more accurate, like\n> \"External file (--contents)\", and then we can drop the language around\n> \"pretend\".\n>\n\nI think this is a good direction, It makes it distinguished from the\nworking tree output.\n\n> It would be pretty simple, too. Here's a rough patch (that I don't mind\n> you sending as your own):\n>\n> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----\n> diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\n> index 95599bd6e5..4a861ff31c 100644\n> --- a/Documentation/blame-options.txt\n> +++ b/Documentation/blame-options.txt\n> @@ -64,11 +64,10 @@ include::line-range-format.txt[]\n>         manual page.\n>\n>  --contents <file>::\n> -       Pretend the file being annotated has a commit with the\n> -       contents from the named file and a parent of <rev>,\n> -       defaulting to HEAD when no <rev> is specified. You may\n> -       specify '-' to make the command read from the standard\n> -       input for the file contents.\n> +       Annotate using the contents from the named file instead of the\n> +       working tree copy, starting with <rev> if it is specified, and\n> +       HEAD otherwise. You may specify '-' to make the command read\n> +       from the standard input for the file contents.\n>\n\nI think I would reword this slightly:\n\nAnnotate using the contents from the name file, starting from <rev> if\nit is specified, and HEAD otherwise.\n\nI do not think we need \"instead of the working tree copy\" as this  is\nconfusing since if <rev> is specified we do not use the working tree\ncopy today. This is where the difficulty in being concise here is. I\nthink just not mentioning working copy is correct here. Perhaps we\nneed to mention that the working copy is used if no <rev> is specified\nelsewhere in the documentation?\n\n>  --date <format>::\n>         Specifies the format used to output dates. If --date is not\n> diff --git a/blame.c b/blame.c\n> index 2d02cf0636..129dae7641 100644\n> --- a/blame.c\n> +++ b/blame.c\n> @@ -204,8 +204,12 @@ static struct commit *fake_working_tree_commit(struct repository *r,\n>\n>         origin = make_origin(commit, path);\n>\n> -       ident = fmt_ident(\"Not Committed Yet\", \"not.committed.yet\",\n> -                       WANT_BLANK_IDENT, NULL, 0);\n> +       if (contents_from)\n> +               ident = fmt_ident(\"External file (--contents)\", \"external.file\",\n> +                               WANT_BLANK_IDENT, NULL, 0);\n> +       else\n> +               ident = fmt_ident(\"Not Committed Yet\", \"not.committed.yet\",\n> +                               WANT_BLANK_IDENT, NULL, 0);\n>         strbuf_addstr(&msg, \"tree 0000000000000000000000000000000000000000\\n\");\n>         for (parent = commit->parents; parent; parent = parent->next)\n>                 strbuf_addf(&msg, \"parent %s\\n\",\n> diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh\n> index b35be20cf3..2ef70235b1 100644\n> --- a/t/annotate-tests.sh\n> +++ b/t/annotate-tests.sh\n> @@ -72,6 +72,13 @@ test_expect_success 'blame 1 author' '\n>         check_count A 2\n>  '\n>\n> +test_expect_success 'blame working copy' '\n> +       test_when_finished \"git restore file\" &&\n> +       echo \"1A quick brown fox jumps over the\" >file &&\n> +       echo \"another lazy dog\" >>file &&\n> +       check_count A 1 \"Not Committed Yet\" 1\n> +'\n> +\n>  test_expect_success 'blame with --contents' '\n>         check_count --contents=file A 2\n>  '\n> @@ -79,7 +86,7 @@ test_expect_success 'blame with --contents' '\n>  test_expect_success 'blame with --contents changed' '\n>         echo \"1A quick brown fox jumps over the\" >contents &&\n>         echo \"another lazy dog\" >>contents &&\n> -       check_count --contents=contents A 1 \"Not Committed Yet\" 1\n> +       check_count --contents=contents A 1 \"External file (--contents)\" 1\n>  '\n>\n>  test_expect_success 'blame in a bare repo without starting commit' '\n> @@ -109,7 +116,7 @@ test_expect_success 'blame 2 authors' '\n>  '\n>\n>  test_expect_success 'blame with --contents and revision' '\n> -       check_count -h testTag --contents=file A 2 \"Not Committed Yet\" 2\n> +       check_count -h testTag --contents=file A 2 \"External file (--contents)\" 2\n>  '\n>\n>  test_expect_success 'setup B1 lines (branch1)' '\n>\n> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----\n>\n> The patch doesn't do anything special, except that we'd need to make\n> sure to test that the working copy is annotated correctly (we didn't\n> have tests for this before??).\n\nWe check the authorship with the tests I added now.\n\n>\n> The doc and the fake commit author could use some tweaking, but IMO the\n> end result is a lot easier to explain to end users, even in the case\n> where \"--contents\" is given without <rev>.\n>\n\nYea I think changing the author name is good.\n\n> The _one_ reason not to merge this is that this changes the annotated\n> author with \"--contents\", which could cause trouble for end users who\n> are parsing the output and expecting literal \"Not Committed Yet\". Given\n> our history with Hyrum's law, surely _someone_ is doing this in the\n> non-\"--contents\" case, but I suspect that \"--contents\" is uncommon\n> enough that we could take the chance.\n\nI am one of those people using --contents and analyzing the output...\nbut at least in my case I'm looking for the 00000000 commit ID instead\nof looking for the author name.\n\nI'm in favor of changing the output, but I'd like to hear other opinions here.\n"}]}