{"thread":{"id":"62278","subject":"[PATCH] blame: respect .git-blame-ignore-revs automatically","startedAt":"2024-10-08T07:01:36Z","lastAt":"2024-10-22T07:54:46Z","messageCount":24,"participants":["Abhijeetsingh Meena via GitGitGadget","Kristoffer Haugsbakk","Phillip Wood","Abhijeetsingh Meena","Eric Sunshine","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"504394","messageId":"pull.1809.git.1728370892696.gitgitgadget@gmail.com","threadId":"62278","inReplyTo":null,"subject":"[PATCH] blame: respect .git-blame-ignore-revs automatically","fromName":"Abhijeetsingh Meena via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-08T07:01:32Z","receivedAt":"2024-10-08T07:01:36Z","isPatch":true,"sender":{"key":"abhijeetsingh.github@gmail.com","avatar":null},"body":"From: Abhijeetsingh Meena \n\nModify `git blame` to automatically respect a `.git-blame-ignore-revs`\nfile if it exists in the repository. This file is used by many projects\nto ignore non-functional changes, such as reformatting or large-scale\nrefactoring, when generating blame information.\n\nBefore this change, users had to manually specify the file with the\n`--ignore-revs-file` option. This update streamlines the process by\nautomatically detecting the `.git-blame-ignore-revs` file, reducing\nmanual effort.\n\nThis change aligns with the standardized practice in many repositories\nand simplifies the workflow for users.\n\nSigned-off-by: Abhijeetsingh Meena \n---\n blame: respect .git-blame-ignore-revs automatically\n \n \n Introduction\n ============\n \n Hi, I'm Abhijeet (Ethan0456), and this is my first contribution to the\n Git project. I currently work as an ML Engineer at an early-stage\n startup, and I’m excited to contribute to this open-source project.\n \n \n Why the Change?\n ===============\n \n I came across this enhancement request on the bug tracker and found it\n beginner-friendly, making it a great opportunity for me to get familiar\n with the Git codebase. The ability for git blame to automatically\n respect the .git-blame-ignore-revs file is something that can streamline\n workflows for many users, and I felt it would be a valuable addition.\n \n \n Feedback\n ========\n \n While I’m confident in the changes made to builtin/blame.c and the new\n test case in t/t8015-blame-ignore-revs.sh, I welcome any feedback or\n suggestions to improve both my code and approach. I’m eager to learn\n from the community and improve where needed.\n \n \n Community Need\n ==============\n \n There is precedent for this functionality in other projects:\n \n * Chromium\n [https://chromium.googlesource.com/chromium/src.git/+/f0596779e57f46fccb115a0fd65f0305894e3031/.git-blame-ignore-revs],\n which powers many popular browsers, uses .git-blame-ignore-revs to\n simplify the blame process by ignoring non-functional changes.\n * Rob Allen's blog post\n [https://akrabat.com/ignoring-revisions-with-git-blame/] discusses\n the need for ignoring revisions with git blame, and a commenter\n specifically suggests that it would be helpful if Git automatically\n respected .git-blame-ignore-revs.\n \n I hope this change aligns with community needs and improves the git\n blame experience for users.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1809%2FEthan0456%2Fblame-auto-ignore-revs-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1809/Ethan0456/blame-auto-ignore-revs-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1809\n\n builtin/blame.c | 8 ++++++++\n t/t8015-blame-default-ignore-revs.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 34 insertions(+)\n create mode 100755 t/t8015-blame-default-ignore-revs.sh\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex e407a22da3b..1eddabaf60f 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1105,6 +1105,14 @@ parse_done:\n \t\tadd_pending_object(&revs, &head_commit->object, \"HEAD\");\n \t}\n \n+\t/*\n+\t* By default, add .git-blame-ignore-revs to the list of files\n+\t* containing revisions to ignore if it exists.\n+\t*/\n+\tif (access(\".git-blame-ignore-revs\", F_OK) == 0) {\n+\t\tstring_list_append(&ignore_revs_file_list, \".git-blame-ignore-revs\");\n+\t}\n+\n \tinit_scoreboard(&sb);\n \tsb.revs = &revs;\n \tsb.contents_from = contents_from;\ndiff --git a/t/t8015-blame-default-ignore-revs.sh b/t/t8015-blame-default-ignore-revs.sh\nnew file mode 100755\nindex 00000000000..84e1a9e87e6\n--- /dev/null\n+++ b/t/t8015-blame-default-ignore-revs.sh\n@@ -0,0 +1,26 @@\n+#!/bin/sh\n+\n+test_description='default revisions to ignore when blaming'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+test_expect_success 'blame: default-ignore-revs-file' '\n+ test_commit first-commit hello.txt hello &&\n+\n+ echo world >>hello.txt &&\n+ test_commit second-commit hello.txt &&\n+\n+ sed \"1s/hello/hi/\" hello.txt.tmp &&\n+ mv hello.txt.tmp hello.txt &&\n+ test_commit third-commit hello.txt &&\n+\n+ git rev-parse HEAD >ignored-file &&\n+ git blame --ignore-revs-file=ignored-file hello.txt >expect &&\n+ git rev-parse HEAD >.git-blame-ignore-revs &&\n+ git blame hello.txt >actual &&\n+\n+ test_cmp expect actual\n+'\n+\n+test_done\n\\ No newline at end of file\n\nbase-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2\n-- \ngitgitgadget\n"},{"id":"504408","messageId":"dc01d9b3-d5fd-451f-8ce4-411f3a9b4b22@app.fastmail.com","threadId":"62278","inReplyTo":"pull.1809.git.1728370892696.gitgitgadget@gmail.com","subject":"Re: [PATCH] blame: respect .git-blame-ignore-revs automatically","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-10-08T09:07:40Z","receivedAt":"2024-10-08T09:08:02Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Thank you for working on this. I think an ability to respect a\nconventional ignore list would be useful.\n\nI was a bit concerned that there doesn’t seem to be something to turn\noff ignore lists. The man page says that the discussed option exists\nbut not some `--no` variant. But then I tested this:\n\n```\ngit blame --ignore-revs-file=README.md \\\n --no-ignore-revs-file README.md\n```\n\nAnd that works without giving errors. So it’s there. Just apparently\nundocumented?\n\nMy assumption then is that, with this change, I could use\n`--no-ignore-revs-file` to turn off the default file.\n\n(One can also use the config `blame.ignoreRevsFile` set to the\nempty string)\n\nOn Tue, Oct 8, 2024, at 09:01, Abhijeetsingh Meena via GitGitGadget wrote:\n> From: Abhijeetsingh Meena \n>\n> Modify `git blame` to automatically respect a `.git-blame-ignore-revs`\n> file if it exists in the repository. This file is used by many projects\n> to ignore non-functional changes, such as reformatting or large-scale\n> refactoring, when generating blame information.\n\nNice.\n\n> Before this change, users had to manually specify the file with the\n> `--ignore-revs-file` option. This update streamlines the process by\n> automatically detecting the `.git-blame-ignore-revs` file, reducing\n> manual effort.\n\n(They can also use the `blame.ignoreRevsFile` config in order to not\nhave to give the option every time)\n\nThe project convention is to use the present tense for describing the\ncurrent behavior (without this patch). Like:\n\n Users that have a standard list of commits to ignore have to use\n `--ignore-revs-file`.\n\nThis style often lends itself to this structure:\n\n- (optionally introduce the wider topic)\n- Describe the current behavior and why that is a problem\n- Say how we’re changing the code (which you already did a good job of in\n your first paragraph).\n\nIt might end up something like this:\n\n git-blame(1) can ignore a list of commits with `--ignore-revs-file`.\n This is useful for marking uninteresting commits like formatting\n changes, refactors and whatever else should not be “blamed”. Some\n projects even version control this file so that all contributors can\n use it; the conventional name is `.git-blame-ignore-revs`.\n\n But each user still has to opt-in to the standard ignore list,\n either with this option or with the config `blame.ignoreRevsFile`.\n Let’s teach git-blame(1) to respect this conventional file in order\n to streamline the process.\n\n> This change aligns with the standardized practice in many repositories\n> and simplifies the workflow for users.\n>\n> Signed-off-by: Abhijeetsingh Meena \n> ---\n> blame: respect .git-blame-ignore-revs automatically\n>\n>\n> Introduction\n> ============\n>\n> Hi, I'm Abhijeet (Ethan0456), and this is my first contribution to the\n> Git project. I currently work as an ML Engineer at an early-stage\n> startup, and I’m excited to contribute to this open-source project.\n>\n>\n> Why the Change?\n> ===============\n>\n> I came across this enhancement request on the bug tracker and found it\n> beginner-friendly, making it a great opportunity for me to get familiar\n> with the Git codebase. The ability for git blame to automatically\n> respect the .git-blame-ignore-revs file is something that can streamline\n> workflows for many users, and I felt it would be a valuable addition.\n\nThe issue that is referred:\n\nhttps://github.com/gitgitgadget/git/issues/1494\n\nThat issue tracker is unofficial by the way.\n\n>\n>\n> Feedback\n> ========\n>\n> While I’m confident in the changes made to builtin/blame.c and the new\n> test case in t/t8015-blame-ignore-revs.sh, I welcome any feedback or\n> suggestions to improve both my code and approach. I’m eager to learn\n> from the community and improve where needed.\n>\n>\n> Community Need\n> ==============\n>\n> There is precedent for this functionality in other projects:\n>\n> * Chromium\n>\n> [https://chromium.googlesource.com/chromium/src.git/+/f0596779e57f46fccb115a0fd65f0305894e3031/.git-blame-ignore-revs],\n> which powers many popular browsers, uses .git-blame-ignore-revs\n> to\n> simplify the blame process by ignoring non-functional changes.\n> * Rob Allen's blog post\n> [https://akrabat.com/ignoring-revisions-with-git-blame/]\n> discusses\n> the need for ignoring revisions with git blame, and a commenter\n> specifically suggests that it would be helpful if Git\n> automatically\n> respected .git-blame-ignore-revs.\n>\n> I hope this change aligns with community needs and improves the git\n> blame experience for users.\n\nIt’s nice that you mention specific projects that use this convention.\nSometimes people just say “this is used by many people/projects”. But\nnot what projects. (Then perhaps it is eventually revealed that “we use\nit so therefore it is important” ;) )\n\n> Published-As:\n> https://github.com/gitgitgadget/git/releases/tag/pr-1809%2FEthan0456%2Fblame-auto-ignore-revs-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git\n> pr-1809/Ethan0456/blame-auto-ignore-revs-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1809\n>\n> builtin/blame.c | 8 ++++++++\n> t/t8015-blame-default-ignore-revs.sh | 26 ++++++++++++++++++++++++++\n> 2 files changed, 34 insertions(+)\n> create mode 100755 t/t8015-blame-default-ignore-revs.sh\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index e407a22da3b..1eddabaf60f 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -1105,6 +1105,14 @@ parse_done:\n> \t\tadd_pending_object(&revs, &head_commit->object, \"HEAD\");\n> \t}\n>\n> +\t/*\n> +\t* By default, add .git-blame-ignore-revs to the list of files\n> +\t* containing revisions to ignore if it exists.\n> +\t*/\n> +\tif (access(\".git-blame-ignore-revs\", F_OK) == 0) {\n> +\t\tstring_list_append(&ignore_revs_file_list, \".git-blame-ignore-revs\");\n> +\t}\n> +\n> \tinit_scoreboard(&sb);\n> \tsb.revs = &revs;\n> \tsb.contents_from = contents_from;\n> diff --git a/t/t8015-blame-default-ignore-revs.sh\n> b/t/t8015-blame-default-ignore-revs.sh\n> new file mode 100755\n> index 00000000000..84e1a9e87e6\n> --- /dev/null\n> +++ b/t/t8015-blame-default-ignore-revs.sh\n> @@ -0,0 +1,26 @@\n> +#!/bin/sh\n> +\n> +test_description='default revisions to ignore when blaming'\n> +\n> +TEST_PASSES_SANITIZE_LEAK=true\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'blame: default-ignore-revs-file' '\n> + test_commit first-commit hello.txt hello &&\n> +\n> + echo world >>hello.txt &&\n> + test_commit second-commit hello.txt &&\n> +\n> + sed \"1s/hello/hi/\" hello.txt.tmp &&\n> + mv hello.txt.tmp hello.txt &&\n> + test_commit third-commit hello.txt &&\n> +\n> + git rev-parse HEAD >ignored-file &&\n> + git blame --ignore-revs-file=ignored-file hello.txt >expect &&\n> + git rev-parse HEAD >.git-blame-ignore-revs &&\n> + git blame hello.txt >actual &&\n> +\n> + test_cmp expect actual\n> +'\n> +\n> +test_done\n> \\ No newline at end of file\n\nThere should be a final newline. ;)\n\n>\n> base-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2\n> --\n> gitgitgadget\n"},{"id":"504410","messageId":"8d27ea8e-4562-4fcc-a7b3-399abc42135b@gmail.com","threadId":"62278","inReplyTo":"pull.1809.git.1728370892696.gitgitgadget@gmail.com","subject":"Re: [PATCH] blame: respect .git-blame-ignore-revs automatically","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-10-08T09:51:01Z","receivedAt":"2024-10-08T09:51:04Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Abhijeetsingh\n\nOn 08/10/2024 08:01, Abhijeetsingh Meena via GitGitGadget wrote:\n> From: Abhijeetsingh Meena \n> \n> Modify `git blame` to automatically respect a `.git-blame-ignore-revs`\n> file if it exists in the repository. This file is used by many projects\n> to ignore non-functional changes, such as reformatting or large-scale\n> refactoring, when generating blame information.\n> \n> Before this change, users had to manually specify the file with the\n> `--ignore-revs-file` option. This update streamlines the process by\n> automatically detecting the `.git-blame-ignore-revs` file, reducing\n> manual effort.\n\nAs Kristoffer mentioned there is a config option so that the user does \nnot have to specify the file each time. The decision not to support a \ndefault file for this feature was deliberate [1,2]. It is possible that \nwe now have enough experience with the feature that we think it would be \na good idea but any such change would need to address the concerns about \nignoring \"cleanup\" commits that introduce bugs. If we do decide to \nsupport a default file we'd need to work out how it interacted with the \nconfig setting and make sure there was an easy way to turn the feature off.\n\nBest Wishes\n\nPhillip\n\n[1] \nhttps://lore.kernel.org/git/c34159af-f97e-82b3-e2a1-04adae5c10ac@google.com/\n[2] https://lore.kernel.org/git/YLfe+HXl4hkzs44b@nand.local/\n\n> This change aligns with the standardized practice in many repositories\n> and simplifies the workflow for users.\n> \n> Signed-off-by: Abhijeetsingh Meena \n> ---\n> blame: respect .git-blame-ignore-revs automatically\n> \n> \n> Introduction\n> ============\n> \n> Hi, I'm Abhijeet (Ethan0456), and this is my first contribution to the\n> Git project. I currently work as an ML Engineer at an early-stage\n> startup, and I’m excited to contribute to this open-source project.\n> \n> \n> Why the Change?\n> ===============\n> \n> I came across this enhancement request on the bug tracker and found it\n> beginner-friendly, making it a great opportunity for me to get familiar\n> with the Git codebase. The ability for git blame to automatically\n> respect the .git-blame-ignore-revs file is something that can streamline\n> workflows for many users, and I felt it would be a valuable addition.\n> \n> \n> Feedback\n> ========\n> \n> While I’m confident in the changes made to builtin/blame.c and the new\n> test case in t/t8015-blame-ignore-revs.sh, I welcome any feedback or\n> suggestions to improve both my code and approach. I’m eager to learn\n> from the community and improve where needed.\n> \n> \n> Community Need\n> ==============\n> \n> There is precedent for this functionality in other projects:\n> \n> * Chromium\n> [https://chromium.googlesource.com/chromium/src.git/+/f0596779e57f46fccb115a0fd65f0305894e3031/.git-blame-ignore-revs],\n> which powers many popular browsers, uses .git-blame-ignore-revs to\n> simplify the blame process by ignoring non-functional changes.\n> * Rob Allen's blog post\n> [https://akrabat.com/ignoring-revisions-with-git-blame/] discusses\n> the need for ignoring revisions with git blame, and a commenter\n> specifically suggests that it would be helpful if Git automatically\n> respected .git-blame-ignore-revs.\n> \n> I hope this change aligns with community needs and improves the git\n> blame experience for users.\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1809%2FEthan0456%2Fblame-auto-ignore-revs-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1809/Ethan0456/blame-auto-ignore-revs-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1809\n> \n> builtin/blame.c | 8 ++++++++\n> t/t8015-blame-default-ignore-revs.sh | 26 ++++++++++++++++++++++++++\n> 2 files changed, 34 insertions(+)\n> create mode 100755 t/t8015-blame-default-ignore-revs.sh\n> \n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index e407a22da3b..1eddabaf60f 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -1105,6 +1105,14 @@ parse_done:\n> \t\tadd_pending_object(&revs, &head_commit->object, \"HEAD\");\n> \t}\n> \n> +\t/*\n> +\t* By default, add .git-blame-ignore-revs to the list of files\n> +\t* containing revisions to ignore if it exists.\n> +\t*/\n> +\tif (access(\".git-blame-ignore-revs\", F_OK) == 0) {\n> +\t\tstring_list_append(&ignore_revs_file_list, \".git-blame-ignore-revs\");\n> +\t}\n> +\n> \tinit_scoreboard(&sb);\n> \tsb.revs = &revs;\n> \tsb.contents_from = contents_from;\n> diff --git a/t/t8015-blame-default-ignore-revs.sh b/t/t8015-blame-default-ignore-revs.sh\n> new file mode 100755\n> index 00000000000..84e1a9e87e6\n> --- /dev/null\n> +++ b/t/t8015-blame-default-ignore-revs.sh\n> @@ -0,0 +1,26 @@\n> +#!/bin/sh\n> +\n> +test_description='default revisions to ignore when blaming'\n> +\n> +TEST_PASSES_SANITIZE_LEAK=true\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'blame: default-ignore-revs-file' '\n> + test_commit first-commit hello.txt hello &&\n> +\n> + echo world >>hello.txt &&\n> + test_commit second-commit hello.txt &&\n> +\n> + sed \"1s/hello/hi/\" hello.txt.tmp &&\n> + mv hello.txt.tmp hello.txt &&\n> + test_commit third-commit hello.txt &&\n> +\n> + git rev-parse HEAD >ignored-file &&\n> + git blame --ignore-revs-file=ignored-file hello.txt >expect &&\n> + git rev-parse HEAD >.git-blame-ignore-revs &&\n> + git blame hello.txt >actual &&\n> +\n> + test_cmp expect actual\n> +'\n> +\n> +test_done\n> \\ No newline at end of file\n> \n> base-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2\n\n"},{"id":"504454","messageId":"CAAirc3jxEV9ZrLcZRMNahZTXWV-1H3KwGXT2iEU+nsZWX2bUnA@mail.gmail.com","threadId":"62278","inReplyTo":"CAAirc3gX=PBixcyR0NRyRDnrydkeeu8A9or90c8MSUdHQ6uo=w@mail.gmail.com","subject":"Re: [PREVIEW v2] blame: respect .git-blame-ignore-revs automatically","fromName":"Abhijeetsingh Meena","fromEmail":"abhijeetsingh.github@gmail.com","sentAt":"2024-10-08T16:12:42Z","receivedAt":"2024-10-08T16:12:55Z","isPatch":false,"sender":{"key":"abhijeetsingh.github@gmail.com","avatar":null},"body":"Hi Kristoffer,\n\nThank you so much for your feedback and for taking the time to review\nmy patch. I appreciate your suggestions, particularly regarding the\nproject convention of using the present tense to describe the current\nbehavior, as well as your mention of the --no-ignore-revs-file option\nand the importance of documenting it.\n\nRegarding your first point:\n\n> My assumption then is that, with this change, I could use --no-ignore-revs-file to turn off the default file.\n\nI’ll check how this works in the current setup and get back to you to confirm.\n\nAs for your second observation:\n\n> git blame --ignore-revs-file=README.md\n> --no-ignore-revs-file README.md\n> And that works without giving errors. So it’s there. Just apparently undocumented?\n\nI’ll look into opening an issue and submitting a PR to add\ndocumentation for this, as it seems important to address.\n\nI’m excited to refine the patch with these improvements in mind and\nwill continue working on the commit message and necessary\ndocumentation in line with your feedback.\n\nThanks again for your guidance and support!\n\nBest regards,\nAbhijeet (Ethan0456)\n\nOn Tue, Oct 8, 2024 at 8:53 PM Abhijeetsingh Meena\n wrote:\n>\n> Hi Phillip,\n>\n> Thank you for your feedback on my patch. I appreciate your insights regarding the decision not to support a default file for the --ignore-revs-file feature and the concerns related to ignoring \"cleanup\" commits that could potentially introduce bugs.\n>\n> I understand that addressing these concerns is crucial. Specifically, I will focus on:\n>\n> 1. Ensuring there’s a clear and easy way for users to opt out or override the default behavior of ignoring certain commits, especially if they suspect those commits might contain bugs.\n> 2. Investigating how this new feature could interact with existing configuration options that allow users to specify what should be ignored manually.\n>\n> I will take these points into consideration as I work on my changes and come up with a solution to address these concerns.\n>\n> Thank you again for your guidance, and I look forward to your continued feedback!\n>\n> Best wishes,\n> Abhijeet (Ethan0456)\n>\n> On Tue, Oct 8, 2024 at 8:53 PM Abhijeetsingh Meena wrote:\n>>\n>> Hi Kristoffer,\n>>\n>> Thank you so much for your feedback and for taking the time to review my patch. I appreciate your suggestions, particularly regarding the project convention of using the present tense to describe the current behavior, as well as your mention of the --no-ignore-revs-file option and the importance of documenting it.\n>>\n>> Regarding your first point:\n>>\n>> > My assumption then is that, with this change, I could use --no-ignore-revs-file to turn off the default file.\n>>\n>> I’ll check how this works in the current setup and get back to you to confirm.\n>>\n>> As for your second observation:\n>>\n>> > git blame --ignore-revs-file=README.md\n>> > --no-ignore-revs-file README.md\n>> > And that works without giving errors. So it’s there. Just apparently undocumented?\n>>\n>> I’ll look into opening an issue and submitting a PR to add documentation for this, as it seems important to address.\n>>\n>> I’m excited to refine the patch with these improvements in mind and will continue working on the commit message and necessary documentation in line with your feedback.\n>>\n>> Thanks again for your guidance and support!\n>>\n>> Best regards,\n>> Abhijeet (Ethan0456)\n>>\n>> On Tue, Oct 8, 2024 at 7:54 PM Abhijeetsingh Meena via GitGitGadget wrote:\n>>>\n>>> From: Abhijeetsingh Meena \n>>>\n>>> Modify `git blame` to automatically respect a `.git-blame-ignore-revs`\n>>> file if it exists in the repository. This file is used by many projects\n>>> to ignore non-functional changes, such as reformatting or large-scale\n>>> refactoring, when generating blame information.\n>>>\n>>> Before this change, users had to manually specify the file with the\n>>> `--ignore-revs-file` option. This update streamlines the process by\n>>> automatically detecting the `.git-blame-ignore-revs` file, reducing\n>>> manual effort.\n>>>\n>>> This change aligns with the standardized practice in many repositories\n>>> and simplifies the workflow for users.\n>>>\n>>> Signed-off-by: Abhijeetsingh Meena \n>>> ---\n>>> blame: respect .git-blame-ignore-revs automatically\n>>>\n>>>\n>>> Introduction\n>>> ============\n>>>\n>>> Hi, I'm Abhijeet (Ethan0456), and this is my first contribution to the\n>>> Git project. I currently work as an ML Engineer at an early-stage\n>>> startup, and I’m excited to contribute to this open-source project.\n>>>\n>>>\n>>> Why the Change?\n>>> ===============\n>>>\n>>> I came across this enhancement request on the bug tracker and found it\n>>> beginner-friendly, making it a great opportunity for me to get familiar\n>>> with the Git codebase. The ability for git blame to automatically\n>>> respect the .git-blame-ignore-revs file is something that can streamline\n>>> workflows for many users, and I felt it would be a valuable addition.\n>>>\n>>>\n>>> Feedback\n>>> ========\n>>>\n>>> While I’m confident in the changes made to builtin/blame.c and the new\n>>> test case in t/t8015-blame-ignore-revs.sh, I welcome any feedback or\n>>> suggestions to improve both my code and approach. I’m eager to learn\n>>> from the community and improve where needed.\n>>>\n>>>\n>>> Community Need\n>>> ==============\n>>>\n>>> There is precedent for this functionality in other projects:\n>>>\n>>> * Chromium\n>>> [https://chromium.googlesource.com/chromium/src.git/+/f0596779e57f46fccb115a0fd65f0305894e3031/.git-blame-ignore-revs],\n>>> which powers many popular browsers, uses .git-blame-ignore-revs to\n>>> simplify the blame process by ignoring non-functional changes.\n>>> * Rob Allen's blog post\n>>> [https://akrabat.com/ignoring-revisions-with-git-blame/] discusses\n>>> the need for ignoring revisions with git blame, and a commenter\n>>> specifically suggests that it would be helpful if Git automatically\n>>> respected .git-blame-ignore-revs.\n>>>\n>>> I hope this change aligns with community needs and improves the git\n>>> blame experience for users.\n>>>\n>>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1809%2FEthan0456%2Fblame-auto-ignore-revs-v2\n>>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1809/Ethan0456/blame-auto-ignore-revs-v2\n>>> Pull-Request: https://github.com/gitgitgadget/git/pull/1809\n>>>\n>>> Range-diff vs v1:\n>>>\n>>> 1: 666404681d9 = 1: 666404681d9 blame: respect .git-blame-ignore-revs automatically\n>>>\n>>>\n>>> builtin/blame.c | 8 ++++++++\n>>> t/t8015-blame-default-ignore-revs.sh | 26 ++++++++++++++++++++++++++\n>>> 2 files changed, 34 insertions(+)\n>>> create mode 100755 t/t8015-blame-default-ignore-revs.sh\n>>>\n>>> diff --git a/builtin/blame.c b/builtin/blame.c\n>>> index e407a22da3b..1eddabaf60f 100644\n>>> --- a/builtin/blame.c\n>>> +++ b/builtin/blame.c\n>>> @@ -1105,6 +1105,14 @@ parse_done:\n>>> add_pending_object(&revs, &head_commit->object, \"HEAD\");\n>>> }\n>>>\n>>> + /*\n>>> + * By default, add .git-blame-ignore-revs to the list of files\n>>> + * containing revisions to ignore if it exists.\n>>> + */\n>>> + if (access(\".git-blame-ignore-revs\", F_OK) == 0) {\n>>> + string_list_append(&ignore_revs_file_list, \".git-blame-ignore-revs\");\n>>> + }\n>>> +\n>>> init_scoreboard(&sb);\n>>> sb.revs = &revs;\n>>> sb.contents_from = contents_from;\n>>> diff --git a/t/t8015-blame-default-ignore-revs.sh b/t/t8015-blame-default-ignore-revs.sh\n>>> new file mode 100755\n>>> index 00000000000..84e1a9e87e6\n>>> --- /dev/null\n>>> +++ b/t/t8015-blame-default-ignore-revs.sh\n>>> @@ -0,0 +1,26 @@\n>>> +#!/bin/sh\n>>> +\n>>> +test_description='default revisions to ignore when blaming'\n>>> +\n>>> +TEST_PASSES_SANITIZE_LEAK=true\n>>> +. ./test-lib.sh\n>>> +\n>>> +test_expect_success 'blame: default-ignore-revs-file' '\n>>> + test_commit first-commit hello.txt hello &&\n>>> +\n>>> + echo world >>hello.txt &&\n>>> + test_commit second-commit hello.txt &&\n>>> +\n>>> + sed \"1s/hello/hi/\" hello.txt.tmp &&\n>>> + mv hello.txt.tmp hello.txt &&\n>>> + test_commit third-commit hello.txt &&\n>>> +\n>>> + git rev-parse HEAD >ignored-file &&\n>>> + git blame --ignore-revs-file=ignored-file hello.txt >expect &&\n>>> + git rev-parse HEAD >.git-blame-ignore-revs &&\n>>> + git blame hello.txt >actual &&\n>>> +\n>>> + test_cmp expect actual\n>>> +'\n>>> +\n>>> +test_done\n>>> \\ No newline at end of file\n>>>\n>>> base-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2\n>>> --\n>>> gitgitgadget\n"},{"id":"504455","messageId":"CAAirc3iMSo9euq+UPoxi+zXU-a6n_VJOfHog5SQKHzxUVrt4rw@mail.gmail.com","threadId":"62278","inReplyTo":"CAAirc3jxEV9ZrLcZRMNahZTXWV-1H3KwGXT2iEU+nsZWX2bUnA@mail.gmail.com","subject":"Re: [PREVIEW v2] blame: respect .git-blame-ignore-revs automatically","fromName":"Abhijeetsingh Meena","fromEmail":"abhijeetsingh.github@gmail.com","sentAt":"2024-10-08T16:12:55Z","receivedAt":"2024-10-08T16:13:08Z","isPatch":false,"sender":{"key":"abhijeetsingh.github@gmail.com","avatar":null},"body":"Hi Phillip,\n\nThank you for your feedback on my patch. I appreciate your insights\nregarding the decision not to support a default file for the\n--ignore-revs-file feature and the concerns related to ignoring\n\"cleanup\" commits that could potentially introduce bugs.\n\nI understand that addressing these concerns is crucial. Specifically,\nI will focus on:\n\n1. Ensuring there’s a clear and easy way for users to opt out or\noverride the default behavior of ignoring certain commits, especially\nif they suspect those commits might contain bugs.\n2. Investigating how this new feature could interact with existing\nconfiguration options that allow users to specify what should be\nignored manually.\n\nI will take these points into consideration as I work on my changes\nand come up with a solution to address these concerns.\n\nThank you again for your guidance, and I look forward to your\ncontinued feedback!\n\nBest wishes,\nAbhijeet (Ethan0456)\n\nOn Tue, Oct 8, 2024 at 9:42 PM Abhijeetsingh Meena\n wrote:\n>\n> Hi Kristoffer,\n>\n> Thank you so much for your feedback and for taking the time to review\n> my patch. I appreciate your suggestions, particularly regarding the\n> project convention of using the present tense to describe the current\n> behavior, as well as your mention of the --no-ignore-revs-file option\n> and the importance of documenting it.\n>\n> Regarding your first point:\n>\n> > My assumption then is that, with this change, I could use --no-ignore-revs-file to turn off the default file.\n>\n> I’ll check how this works in the current setup and get back to you to confirm.\n>\n> As for your second observation:\n>\n> > git blame --ignore-revs-file=README.md\n> > --no-ignore-revs-file README.md\n> > And that works without giving errors. So it’s there. Just apparently undocumented?\n>\n> I’ll look into opening an issue and submitting a PR to add\n> documentation for this, as it seems important to address.\n>\n> I’m excited to refine the patch with these improvements in mind and\n> will continue working on the commit message and necessary\n> documentation in line with your feedback.\n>\n> Thanks again for your guidance and support!\n>\n> Best regards,\n> Abhijeet (Ethan0456)\n>\n> On Tue, Oct 8, 2024 at 8:53 PM Abhijeetsingh Meena\n> wrote:\n> >\n> > Hi Phillip,\n> >\n> > Thank you for your feedback on my patch. I appreciate your insights regarding the decision not to support a default file for the --ignore-revs-file feature and the concerns related to ignoring \"cleanup\" commits that could potentially introduce bugs.\n> >\n> > I understand that addressing these concerns is crucial. Specifically, I will focus on:\n> >\n> > 1. Ensuring there’s a clear and easy way for users to opt out or override the default behavior of ignoring certain commits, especially if they suspect those commits might contain bugs.\n> > 2. Investigating how this new feature could interact with existing configuration options that allow users to specify what should be ignored manually.\n> >\n> > I will take these points into consideration as I work on my changes and come up with a solution to address these concerns.\n> >\n> > Thank you again for your guidance, and I look forward to your continued feedback!\n> >\n> > Best wishes,\n> > Abhijeet (Ethan0456)\n> >\n> > On Tue, Oct 8, 2024 at 8:53 PM Abhijeetsingh Meena wrote:\n> >>\n> >> Hi Kristoffer,\n> >>\n> >> Thank you so much for your feedback and for taking the time to review my patch. I appreciate your suggestions, particularly regarding the project convention of using the present tense to describe the current behavior, as well as your mention of the --no-ignore-revs-file option and the importance of documenting it.\n> >>\n> >> Regarding your first point:\n> >>\n> >> > My assumption then is that, with this change, I could use --no-ignore-revs-file to turn off the default file.\n> >>\n> >> I’ll check how this works in the current setup and get back to you to confirm.\n> >>\n> >> As for your second observation:\n> >>\n> >> > git blame --ignore-revs-file=README.md\n> >> > --no-ignore-revs-file README.md\n> >> > And that works without giving errors. So it’s there. Just apparently undocumented?\n> >>\n> >> I’ll look into opening an issue and submitting a PR to add documentation for this, as it seems important to address.\n> >>\n> >> I’m excited to refine the patch with these improvements in mind and will continue working on the commit message and necessary documentation in line with your feedback.\n> >>\n> >> Thanks again for your guidance and support!\n> >>\n> >> Best regards,\n> >> Abhijeet (Ethan0456)\n> >>\n> >> On Tue, Oct 8, 2024 at 7:54 PM Abhijeetsingh Meena via GitGitGadget wrote:\n> >>>\n> >>> From: Abhijeetsingh Meena \n> >>>\n> >>> Modify `git blame` to automatically respect a `.git-blame-ignore-revs`\n> >>> file if it exists in the repository. This file is used by many projects\n> >>> to ignore non-functional changes, such as reformatting or large-scale\n> >>> refactoring, when generating blame information.\n> >>>\n> >>> Before this change, users had to manually specify the file with the\n> >>> `--ignore-revs-file` option. This update streamlines the process by\n> >>> automatically detecting the `.git-blame-ignore-revs` file, reducing\n> >>> manual effort.\n> >>>\n> >>> This change aligns with the standardized practice in many repositories\n> >>> and simplifies the workflow for users.\n> >>>\n> >>> Signed-off-by: Abhijeetsingh Meena \n> >>> ---\n> >>> blame: respect .git-blame-ignore-revs automatically\n> >>>\n> >>>\n> >>> Introduction\n> >>> ============\n> >>>\n> >>> Hi, I'm Abhijeet (Ethan0456), and this is my first contribution to the\n> >>> Git project. I currently work as an ML Engineer at an early-stage\n> >>> startup, and I’m excited to contribute to this open-source project.\n> >>>\n> >>>\n> >>> Why the Change?\n> >>> ===============\n> >>>\n> >>> I came across this enhancement request on the bug tracker and found it\n> >>> beginner-friendly, making it a great opportunity for me to get familiar\n> >>> with the Git codebase. The ability for git blame to automatically\n> >>> respect the .git-blame-ignore-revs file is something that can streamline\n> >>> workflows for many users, and I felt it would be a valuable addition.\n> >>>\n> >>>\n> >>> Feedback\n> >>> ========\n> >>>\n> >>> While I’m confident in the changes made to builtin/blame.c and the new\n> >>> test case in t/t8015-blame-ignore-revs.sh, I welcome any feedback or\n> >>> suggestions to improve both my code and approach. I’m eager to learn\n> >>> from the community and improve where needed.\n> >>>\n> >>>\n> >>> Community Need\n> >>> ==============\n> >>>\n> >>> There is precedent for this functionality in other projects:\n> >>>\n> >>> * Chromium\n> >>> [https://chromium.googlesource.com/chromium/src.git/+/f0596779e57f46fccb115a0fd65f0305894e3031/.git-blame-ignore-revs],\n> >>> which powers many popular browsers, uses .git-blame-ignore-revs to\n> >>> simplify the blame process by ignoring non-functional changes.\n> >>> * Rob Allen's blog post\n> >>> [https://akrabat.com/ignoring-revisions-with-git-blame/] discusses\n> >>> the need for ignoring revisions with git blame, and a commenter\n> >>> specifically suggests that it would be helpful if Git automatically\n> >>> respected .git-blame-ignore-revs.\n> >>>\n> >>> I hope this change aligns with community needs and improves the git\n> >>> blame experience for users.\n> >>>\n> >>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1809%2FEthan0456%2Fblame-auto-ignore-revs-v2\n> >>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1809/Ethan0456/blame-auto-ignore-revs-v2\n> >>> Pull-Request: https://github.com/gitgitgadget/git/pull/1809\n> >>>\n> >>> Range-diff vs v1:\n> >>>\n> >>> 1: 666404681d9 = 1: 666404681d9 blame: respect .git-blame-ignore-revs automatically\n> >>>\n> >>>\n> >>> builtin/blame.c | 8 ++++++++\n> >>> t/t8015-blame-default-ignore-revs.sh | 26 ++++++++++++++++++++++++++\n> >>> 2 files changed, 34 insertions(+)\n> >>> create mode 100755 t/t8015-blame-default-ignore-revs.sh\n> >>>\n> >>> diff --git a/builtin/blame.c b/builtin/blame.c\n> >>> index e407a22da3b..1eddabaf60f 100644\n> >>> --- a/builtin/blame.c\n> >>> +++ b/builtin/blame.c\n> >>> @@ -1105,6 +1105,14 @@ parse_done:\n> >>> add_pending_object(&revs, &head_commit->object, \"HEAD\");\n> >>> }\n> >>>\n> >>> + /*\n> >>> + * By default, add .git-blame-ignore-revs to the list of files\n> >>> + * containing revisions to ignore if it exists.\n> >>> + */\n> >>> + if (access(\".git-blame-ignore-revs\", F_OK) == 0) {\n> >>> + string_list_append(&ignore_revs_file_list, \".git-blame-ignore-revs\");\n> >>> + }\n> >>> +\n> >>> init_scoreboard(&sb);\n> >>> sb.revs = &revs;\n> >>> sb.contents_from = contents_from;\n> >>> diff --git a/t/t8015-blame-default-ignore-revs.sh b/t/t8015-blame-default-ignore-revs.sh\n> >>> new file mode 100755\n> >>> index 00000000000..84e1a9e87e6\n> >>> --- /dev/null\n> >>> +++ b/t/t8015-blame-default-ignore-revs.sh\n> >>> @@ -0,0 +1,26 @@\n> >>> +#!/bin/sh\n> >>> +\n> >>> +test_description='default revisions to ignore when blaming'\n> >>> +\n> >>> +TEST_PASSES_SANITIZE_LEAK=true\n> >>> +. ./test-lib.sh\n> >>> +\n> >>> +test_expect_success 'blame: default-ignore-revs-file' '\n> >>> + test_commit first-commit hello.txt hello &&\n> >>> +\n> >>> + echo world >>hello.txt &&\n> >>> + test_commit second-commit hello.txt &&\n> >>> +\n> >>> + sed \"1s/hello/hi/\" hello.txt.tmp &&\n> >>> + mv hello.txt.tmp hello.txt &&\n> >>> + test_commit third-commit hello.txt &&\n> >>> +\n> >>> + git rev-parse HEAD >ignored-file &&\n> >>> + git blame --ignore-revs-file=ignored-file hello.txt >expect &&\n> >>> + git rev-parse HEAD >.git-blame-ignore-revs &&\n> >>> + git blame hello.txt >actual &&\n> >>> +\n> >>> + test_cmp expect actual\n> >>> +'\n> >>> +\n> >>> +test_done\n> >>> \\ No newline at end of file\n> >>>\n> >>> base-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2\n> >>> --\n> >>> gitgitgadget\n"},{"id":"504897","messageId":"pull.1809.v2.git.1728707867.gitgitgadget@gmail.com","threadId":"62278","inReplyTo":"pull.1809.git.1728370892696.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] blame: respect .git-blame-ignore-revs automatically","fromName":"Abhijeetsingh Meena via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-12T04:37:45Z","receivedAt":"2024-10-12T04:37:50Z","isPatch":true,"sender":{"key":"abhijeetsingh.github@gmail.com","avatar":null},"body":"\nSummary\n=======\n\nThis patch introduces the ability for git blame to automatically respect the\n.git-blame-ignore-revs file, which can help users streamline their workflows\nby ignoring revisions for non-functional changes.\n\nThe feature is inspired by similar practices in other projects like\nChromium, and it aligns with needs expressed by users in discussions around\nrevision-ignoring mechanisms for git blame.\n\nI have incorporated suggestions provided during the review of patch v1,\nincluding improvements to the commit message and addressing how to override\nignore revisions using a new option.\n\n\nPatch Updates and Improvements\n==============================\n\n\nChanges since v1\n================\n\n * Updated the commit message for the first patch. I incorporated\n Kristoffer’s suggestions by directly using the example he provided,\n ensuring the commit message aligns with the project's standards.\n * Attempted to use --no-ignore-revs-file to bypass the\n .git-blame-ignore-revs file, but observed that it did not ignore the\n specified revisions.\n * Introduced the --override-ignore-revs option (short form: -O) as a\n straightforward method to override any configured revision ignores,\n providing users with more control.\n * For the second commit, I applied Kristoffer’s feedback in writing the\n commit message to ensure consistency and clarity.\n\nAbhijeetsingh Meena (2):\n blame: respect .git-blame-ignore-revs automatically\n blame: introduce --override-ignore-revs to bypass ignore revisions\n list\n\n builtin/blame.c | 16 +++++++++++++++-\n t/t8015-blame-default-ignore-revs.sh | 26 ++++++++++++++++++++++++++\n t/t8016-blame-override-ignore-revs.sh | 25 +++++++++++++++++++++++++\n 3 files changed, 66 insertions(+), 1 deletion(-)\n create mode 100755 t/t8015-blame-default-ignore-revs.sh\n create mode 100755 t/t8016-blame-override-ignore-revs.sh\n\n\nbase-commit: ef8ce8f3d4344fd3af049c17eeba5cd20d98b69f\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1809%2FEthan0456%2Fblame-auto-ignore-revs-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1809/Ethan0456/blame-auto-ignore-revs-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1809\n\nRange-diff vs v1:\n\n 1: 666404681d9 ! 1: 4ed930cab1b blame: respect .git-blame-ignore-revs automatically\n @@ Metadata\n ## Commit message ##\n blame: respect .git-blame-ignore-revs automatically\n \n - Modify `git blame` to automatically respect a `.git-blame-ignore-revs`\n - file if it exists in the repository. This file is used by many projects\n - to ignore non-functional changes, such as reformatting or large-scale\n - refactoring, when generating blame information.\n + git-blame(1) can ignore a list of commits with `--ignore-revs-file`.\n + This is useful for marking uninteresting commits like formatting\n + changes, refactors and whatever else should not be “blamed”. Some\n + projects even version control this file so that all contributors can\n + use it; the conventional name is `.git-blame-ignore-revs`.\n \n - Before this change, users had to manually specify the file with the\n - `--ignore-revs-file` option. This update streamlines the process by\n - automatically detecting the `.git-blame-ignore-revs` file, reducing\n - manual effort.\n -\n - This change aligns with the standardized practice in many repositories\n - and simplifies the workflow for users.\n + But each user still has to opt-in to the standard ignore list,\n + either with this option or with the config `blame.ignoreRevsFile`.\n + Let’s teach git-blame(1) to respect this conventional file in order\n + to streamline the process.\n \n Signed-off-by: Abhijeetsingh Meena \n \n @@ t/t8015-blame-default-ignore-revs.sh (new)\n +'\n +\n +test_done\n - \\ No newline at end of file\n -: ----------- > 2: 8d2fa3af796 blame: introduce --override-ignore-revs to bypass ignore revisions list\n\n-- \ngitgitgadget\n"},{"id":"504898","messageId":"4ed930cab1b7f5e9738e73c7b9374d927a8acd94.1728707867.git.gitgitgadget@gmail.com","threadId":"62278","inReplyTo":"pull.1809.v2.git.1728707867.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] blame: respect .git-blame-ignore-revs automatically","fromName":"Abhijeetsingh Meena via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-12T04:37:46Z","receivedAt":"2024-10-12T04:37:51Z","isPatch":true,"sender":{"key":"abhijeetsingh.github@gmail.com","avatar":null},"body":"From: Abhijeetsingh Meena \n\ngit-blame(1) can ignore a list of commits with `--ignore-revs-file`.\nThis is useful for marking uninteresting commits like formatting\nchanges, refactors and whatever else should not be “blamed”. Some\nprojects even version control this file so that all contributors can\nuse it; the conventional name is `.git-blame-ignore-revs`.\n\nBut each user still has to opt-in to the standard ignore list,\neither with this option or with the config `blame.ignoreRevsFile`.\nLet’s teach git-blame(1) to respect this conventional file in order\nto streamline the process.\n\nSigned-off-by: Abhijeetsingh Meena \n---\n builtin/blame.c | 8 ++++++++\n t/t8015-blame-default-ignore-revs.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 34 insertions(+)\n create mode 100755 t/t8015-blame-default-ignore-revs.sh\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex e407a22da3b..1eddabaf60f 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1105,6 +1105,14 @@ parse_done:\n \t\tadd_pending_object(&revs, &head_commit->object, \"HEAD\");\n \t}\n \n+\t/*\n+\t* By default, add .git-blame-ignore-revs to the list of files\n+\t* containing revisions to ignore if it exists.\n+\t*/\n+\tif (access(\".git-blame-ignore-revs\", F_OK) == 0) {\n+\t\tstring_list_append(&ignore_revs_file_list, \".git-blame-ignore-revs\");\n+\t}\n+\n \tinit_scoreboard(&sb);\n \tsb.revs = &revs;\n \tsb.contents_from = contents_from;\ndiff --git a/t/t8015-blame-default-ignore-revs.sh b/t/t8015-blame-default-ignore-revs.sh\nnew file mode 100755\nindex 00000000000..d4ab686f14d\n--- /dev/null\n+++ b/t/t8015-blame-default-ignore-revs.sh\n@@ -0,0 +1,26 @@\n+#!/bin/sh\n+\n+test_description='default revisions to ignore when blaming'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+test_expect_success 'blame: default-ignore-revs-file' '\n+ test_commit first-commit hello.txt hello &&\n+\n+ echo world >>hello.txt &&\n+ test_commit second-commit hello.txt &&\n+\n+ sed \"1s/hello/hi/\" hello.txt.tmp &&\n+ mv hello.txt.tmp hello.txt &&\n+ test_commit third-commit hello.txt &&\n+\n+ git rev-parse HEAD >ignored-file &&\n+ git blame --ignore-revs-file=ignored-file hello.txt >expect &&\n+ git rev-parse HEAD >.git-blame-ignore-revs &&\n+ git blame hello.txt >actual &&\n+\n+ test_cmp expect actual\n+'\n+\n+test_done\n-- \ngitgitgadget\n\n"},{"id":"504899","messageId":"8d2fa3af7964dacd09d454de4325b1d5eb7a5c3d.1728707867.git.gitgitgadget@gmail.com","threadId":"62278","inReplyTo":"pull.1809.v2.git.1728707867.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] blame: introduce --override-ignore-revs to bypass ignore revisions list","fromName":"Abhijeetsingh Meena via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-12T04:37:47Z","receivedAt":"2024-10-12T04:37:52Z","isPatch":true,"sender":{"key":"abhijeetsingh.github@gmail.com","avatar":null},"body":"From: Abhijeetsingh Meena \n\nThe git blame command can ignore a list of revisions specified either\nthrough the --ignore-revs-file option or the blame.ignoreRevsFile\nconfiguration. This feature is useful for excluding irrelevant\ncommits, such as formatting changes or large refactors, from blame\nannotations.\n\nHowever, users may encounter cases where they need to\ntemporarily override these configurations to inspect all commits,\neven those excluded by the ignore list. Currently, there is no\nsimple way to bypass all ignore revisions settings in one go.\n\nThis patch introduces the --override-ignore-revs option (or -O),\nwhich allows users to easily bypass the --ignore-revs-file\noption, --ignore-rev option and the blame.ignoreRevsFile\nconfiguration. When this option is used, git blame will completely\ndisregard all configured ignore revisions lists.\n\nThe motivation behind this feature is to provide users with more\nflexibility when dealing with large codebases that rely on\n.git-blame-ignore-revs files for shared configurations, while\nstill allowing them to disable the ignore list when necessary\nfor troubleshooting or deeper inspections.\n\nSigned-off-by: Abhijeetsingh Meena \n---\n builtin/blame.c | 8 +++++++-\n t/t8016-blame-override-ignore-revs.sh | 25 +++++++++++++++++++++++++\n 2 files changed, 32 insertions(+), 1 deletion(-)\n create mode 100755 t/t8016-blame-override-ignore-revs.sh\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 1eddabaf60f..956520edcd9 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -69,6 +69,7 @@ static int coloring_mode;\n static struct string_list ignore_revs_file_list = STRING_LIST_INIT_DUP;\n static int mark_unblamable_lines;\n static int mark_ignored_lines;\n+static int override_ignore_revs = 0;\n \n static struct date_mode blame_date_mode = { DATE_ISO8601 };\n static size_t blame_date_width;\n@@ -901,6 +902,7 @@ int cmd_blame(int argc,\n \t\tOPT_BIT('w', NULL, &xdl_opts, N_(\"ignore whitespace differences\"), XDF_IGNORE_WHITESPACE),\n \t\tOPT_STRING_LIST(0, \"ignore-rev\", &ignore_rev_list, N_(\"rev\"), N_(\"ignore when blaming\")),\n \t\tOPT_STRING_LIST(0, \"ignore-revs-file\", &ignore_revs_file_list, N_(\"file\"), N_(\"ignore revisions from \")),\n+\t\tOPT_BOOL('O', \"override-ignore-revs\", &override_ignore_revs, N_(\"override all configurations that exclude revisions\")),\n \t\tOPT_BIT(0, \"color-lines\", &output_option, N_(\"color redundant metadata from previous line differently\"), OUTPUT_COLOR_LINE),\n \t\tOPT_BIT(0, \"color-by-age\", &output_option, N_(\"color lines by age\"), OUTPUT_SHOW_AGE_WITH_COLOR),\n \t\tOPT_BIT(0, \"minimal\", &xdl_opts, N_(\"spend extra cycles to find better match\"), XDF_NEED_MINIMAL),\n@@ -1119,7 +1121,11 @@ parse_done:\n \tsb.reverse = reverse;\n \tsb.repo = the_repository;\n \tsb.path = path;\n-\tbuild_ignorelist(&sb, &ignore_revs_file_list, &ignore_rev_list);\n+\n+\tif (!override_ignore_revs) {\n+\t\tbuild_ignorelist(&sb, &ignore_revs_file_list, &ignore_rev_list);\n+\t}\n+\n \tstring_list_clear(&ignore_revs_file_list, 0);\n \tstring_list_clear(&ignore_rev_list, 0);\n \tsetup_scoreboard(&sb, &o);\ndiff --git a/t/t8016-blame-override-ignore-revs.sh b/t/t8016-blame-override-ignore-revs.sh\nnew file mode 100755\nindex 00000000000..b5899729f8e\n--- /dev/null\n+++ b/t/t8016-blame-override-ignore-revs.sh\n@@ -0,0 +1,25 @@\n+#!/bin/sh\n+\n+test_description='default revisions to ignore when blaming'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+test_expect_success 'blame: override-ignore-revs' '\n+ test_commit first-commit hello.txt hello &&\n+\n+ echo world >>hello.txt &&\n+ test_commit second-commit hello.txt &&\n+\n+ sed \"1s/hello/hi/\" hello.txt.tmp &&\n+ mv hello.txt.tmp hello.txt &&\n+ test_commit third-commit hello.txt &&\n+\n+ git blame hello.txt >expect &&\n+ git rev-parse HEAD >.git-blame-ignore-revs &&\n+ git blame -O hello.txt >actual &&\n+\n+ test_cmp expect actual\n+'\n+\n+test_done\n-- \ngitgitgadget\n"},{"id":"504901","messageId":"CAPig+cR4WbcDDav0cdXxOMC-EDe2ipWxEzB+0C7zbFjvY_kXtg@mail.gmail.com","threadId":"62278","inReplyTo":"4ed930cab1b7f5e9738e73c7b9374d927a8acd94.1728707867.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] blame: respect .git-blame-ignore-revs automatically","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-10-12T06:07:36Z","receivedAt":"2024-10-12T06:07:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Oct 12, 2024 at 12:38 AM Abhijeetsingh Meena via GitGitGadget\n wrote:\n> git-blame(1) can ignore a list of commits with `--ignore-revs-file`.\n> This is useful for marking uninteresting commits like formatting\n> changes, refactors and whatever else should not be “blamed”. Some\n> projects even version control this file so that all contributors can\n> use it; the conventional name is `.git-blame-ignore-revs`.\n>\n> But each user still has to opt-in to the standard ignore list,\n> either with this option or with the config `blame.ignoreRevsFile`.\n> Let’s teach git-blame(1) to respect this conventional file in order\n> to streamline the process.\n>\n> Signed-off-by: Abhijeetsingh Meena \n> ---\n> builtin/blame.c | 8 ++++++++\n> t/t8015-blame-default-ignore-revs.sh | 26 ++++++++++++++++++++++++++\n> 2 files changed, 34 insertions(+)\n\nThis change should be accompanied by a documentation update, I would think.\n\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> @@ -1105,6 +1105,14 @@ parse_done:\n> + /*\n> + * By default, add .git-blame-ignore-revs to the list of files\n> + * containing revisions to ignore if it exists.\n> + */\n> + if (access(\".git-blame-ignore-revs\", F_OK) == 0) {\n> + string_list_append(&ignore_revs_file_list, \".git-blame-ignore-revs\");\n> + }\n\nA couple style nits and a couple questions...\n\nnit: drop the braces around the one-line `if` body\n\nnit: this project uses `!foo(...)` rather than `foo(...) == 0`\n\nPresumably this consults \".git-blame-ignore-revs\" in the top-level\ndirectory (as you intended) rather than \".git-blame-ignore-revs\" in\nwhatever subdirectory you happen to issue the command because the\ncurrent-working-directory has already been set to the top-level\ndirectory by the time cmd_blame() has been called, right?\n\nBut that leads to the next question. Should automatic consulting of\n\".git-blame-ignore-revs\" be restricted to just the top-level\ndirectory, or should it be modeled after, say, \".gitignore\" which may\nbe strewn around project directories and in which \".gitignore\" files\nare consulted rootward starting from the directory in which the\ncommand is invoked. My knee-jerk thought was that the \".gitignore\"\nmodel may not make sense for \".git-blame-ignore-revs\", but the fact\nthat `git blame` can accept and work with multiple ignore-revs files\nmakes me question that knee-jerk response.\n\n> diff --git a/t/t8015-blame-default-ignore-revs.sh b/t/t8015-blame-default-ignore-revs.sh\n> new file mode 100755\n\nLet's avoid allocating a new test number just for this single new\ntest. Instead, the existing t8013-blame-ignore-revs.sh would probably\nbe a good home for this new test.\n\n> +test_expect_success 'blame: default-ignore-revs-file' '\n> + test_commit first-commit hello.txt hello &&\n> +\n> + echo world >>hello.txt &&\n> + test_commit second-commit hello.txt &&\n> +\n> + sed \"1s/hello/hi/\" hello.txt.tmp &&\n\nstyle: drop space after redirection operator\n\n sed \"1s/hello/hi/\" hello.txt.tmp &&\n\n> + mv hello.txt.tmp hello.txt &&\n> + test_commit third-commit hello.txt &&\n> +\n> + git rev-parse HEAD >ignored-file &&\n> + git blame --ignore-revs-file=ignored-file hello.txt >expect &&\n> + git rev-parse HEAD >.git-blame-ignore-revs &&\n> + git blame hello.txt >actual &&\n\nI would suggest copying or renaming \"ignored-file\" to\n\".git-blame-ignore-revs\" rather than running `git rev-parse HEAD`\ntwice. This way readers won't have to waste mental effort verifying\nthat the result of `git rev-parse HEAD` isn't intended to change\nbetween invocations.\n"},{"id":"504904","messageId":"CAPig+cRkp3ZrykLvgDaGnKgZNV5GiFqc5zq6ZZ-fM7RJcqxM7A@mail.gmail.com","threadId":"62278","inReplyTo":"8d2fa3af7964dacd09d454de4325b1d5eb7a5c3d.1728707867.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] blame: introduce --override-ignore-revs to bypass ignore revisions list","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-10-12T06:24:37Z","receivedAt":"2024-10-12T06:24:49Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Oct 12, 2024 at 12:38 AM Abhijeetsingh Meena via GitGitGadget\n wrote:\n> The git blame command can ignore a list of revisions specified either\n> through the --ignore-revs-file option or the blame.ignoreRevsFile\n> configuration. This feature is useful for excluding irrelevant\n> commits, such as formatting changes or large refactors, from blame\n> annotations.\n>\n> However, users may encounter cases where they need to\n> temporarily override these configurations to inspect all commits,\n> even those excluded by the ignore list. Currently, there is no\n> simple way to bypass all ignore revisions settings in one go.\n>\n> This patch introduces the --override-ignore-revs option (or -O),\n> which allows users to easily bypass the --ignore-revs-file\n> option, --ignore-rev option and the blame.ignoreRevsFile\n> configuration. When this option is used, git blame will completely\n> disregard all configured ignore revisions lists.\n\nIt's not clear from this description whether \".git-blame-ignore-revs\"\nis also ignored by --override-ignore-revs. Looking at the\nimplementation, it appears that it is, but it would be good to state\nso here.\n\n> The motivation behind this feature is to provide users with more\n> flexibility when dealing with large codebases that rely on\n> .git-blame-ignore-revs files for shared configurations, while\n> still allowing them to disable the ignore list when necessary\n> for troubleshooting or deeper inspections.\n>\n> Signed-off-by: Abhijeetsingh Meena \n> ---\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> @@ -901,6 +902,7 @@ int cmd_blame(int argc,\n> OPT_STRING_LIST(0, \"ignore-rev\", &ignore_rev_list, N_(\"rev\"), N_(\"ignore when blaming\")),\n> OPT_STRING_LIST(0, \"ignore-revs-file\", &ignore_revs_file_list, N_(\"file\"), N_(\"ignore revisions from \")),\n> + OPT_BOOL('O', \"override-ignore-revs\", &override_ignore_revs, N_(\"override all configurations that exclude revisions\")),\n\nWe don't normally allocate a short option name (\"-O\" in this case)\nwhen introducing a new option since short option names are considered\na valuable and limited resource. A short option name may be added\n*later* if experience shows that the option is popular enough that the\nconvenience of the short option name is warranted.\n\n> @@ -1119,7 +1121,11 @@ parse_done:\n> - build_ignorelist(&sb, &ignore_revs_file_list, &ignore_rev_list);\n> +\n> + if (!override_ignore_revs) {\n> + build_ignorelist(&sb, &ignore_revs_file_list, &ignore_rev_list);\n> + }\n\nA style nit and questions and observations...\n\nnit: drop the braces around the one-line `if` body\n\nIs the all-or-nothing behavior implemented by this patch desirable? If\nso, should the command warn or error out if the user gives conflicting\noptions like --ignore-revs-file and --override-ignore-revs together?\n\nA common behavior of many Git commands when dealing with options is\n\"last wins\", and following that precedent could make this new option\neven much more useful by allowing the user to ignore project-supplied\nignore-revs but still take advantage of the feature with a different\nset of ignore-revs that make sense to the local user. For instance:\n\n git blame --override-ignore-revs --ignore-revs-file=my-ignore-revs ...\n\n> diff --git a/t/t8016-blame-override-ignore-revs.sh b/t/t8016-blame-override-ignore-revs.sh\n> new file mode 100755\n\nLet's avoid allocating a new test number just for this single new\ntest. Instead, the existing t8013-blame-ignore-revs.sh would probably\nbe a good home for this new test.\n\n> @@ -0,0 +1,25 @@\n> +test_expect_success 'blame: override-ignore-revs' '\n> + test_commit first-commit hello.txt hello &&\n> +\n> + echo world >>hello.txt &&\n> + test_commit second-commit hello.txt &&\n> +\n> + sed \"1s/hello/hi/\" hello.txt.tmp &&\n\nstyle: drop space after redirection operator\n\n sed \"1s/hello/hi/\" hello.txt.tmp &&\n\n> + mv hello.txt.tmp hello.txt &&\n> + test_commit third-commit hello.txt &&\n> +\n> + git blame hello.txt >expect &&\n> + git rev-parse HEAD >.git-blame-ignore-revs &&\n> + git blame -O hello.txt >actual &&\n> +\n> + test_cmp expect actual\n> +'\n\nWhat is this test actually checking? It doesn't seem to use\n--override-ignore-revs at all.\n"},{"id":"504906","messageId":"CAPig+cQ=WGz4vmkcBR0Rk8KtQiF2vaH1PLt0-rTDvYi=z=EEUg@mail.gmail.com","threadId":"62278","inReplyTo":"CAPig+cRkp3ZrykLvgDaGnKgZNV5GiFqc5zq6ZZ-fM7RJcqxM7A@mail.gmail.com","subject":"Re: [PATCH v2 2/2] blame: introduce --override-ignore-revs to bypass ignore revisions list","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-10-12T06:26:40Z","receivedAt":"2024-10-12T06:26:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Oct 12, 2024 at 2:24 AM Eric Sunshine wrote:\n> On Sat, Oct 12, 2024 at 12:38 AM Abhijeetsingh Meena via GitGitGadget\n> wrote:\n> > [...]\n> > This patch introduces the --override-ignore-revs option (or -O),\n> > which allows users to easily bypass the --ignore-revs-file\n> > option, --ignore-rev option and the blame.ignoreRevsFile\n> > configuration. When this option is used, git blame will completely\n> > disregard all configured ignore revisions lists.\n> > + OPT_BOOL('O', \"override-ignore-revs\", &override_ignore_revs, N_(\"override all configurations that exclude revisions\")),\n>\n> We don't normally allocate a short option name (\"-O\" in this case)\n> when introducing a new option since short option names are considered\n> a valuable and limited resource. A short option name may be added\n> *later* if experience shows that the option is popular enough that the\n> convenience of the short option name is warranted.\n\nI forgot to mention that this patch also deserves a documentation\nupdate; otherwise how are users going to know that the new option\nexists?\n"},{"id":"504908","messageId":"CAPig+cQZkBVEHmUqzmouM4A=pgpXhTPNc8cBg0rDdTN=v=hW=g@mail.gmail.com","threadId":"62278","inReplyTo":"CAPig+cR4WbcDDav0cdXxOMC-EDe2ipWxEzB+0C7zbFjvY_kXtg@mail.gmail.com","subject":"Re: [PATCH v2 1/2] blame: respect .git-blame-ignore-revs automatically","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-10-12T06:43:12Z","receivedAt":"2024-10-12T06:43:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Oct 12, 2024 at 2:07 AM Eric Sunshine wrote:\n> On Sat, Oct 12, 2024 at 12:38 AM Abhijeetsingh Meena via GitGitGadget\n> wrote:\n> > + /*\n> > + * By default, add .git-blame-ignore-revs to the list of files\n> > + * containing revisions to ignore if it exists.\n> > + */\n> > + if (access(\".git-blame-ignore-revs\", F_OK) == 0) {\n> > + string_list_append(&ignore_revs_file_list, \".git-blame-ignore-revs\");\n> > + }\n>\n> A couple style nits and a couple questions...\n\nOne other observation: The comment above this code block doesn't say\nanything that isn't already stated just as clearly by the code itself.\nHence, the comment adds no value, thus should be dropped.\n"},{"id":"504912","messageId":"4f835282-d921-4cd3-9680-d8522bef7b39@app.fastmail.com","threadId":"62278","inReplyTo":"4ed930cab1b7f5e9738e73c7b9374d927a8acd94.1728707867.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] blame: respect .git-blame-ignore-revs automatically","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-10-12T13:58:03Z","receivedAt":"2024-10-12T13:58:24Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Hi Abhijeetsingh\n\nFor what it’s worth here’s how I imagine this feature could work\nconceptually:\n\nBefore this feature/change, the effective config for Git use looks like this:\n\n```\n[blame]\n```\n\nNo `blame.ignoreRevsFile`.\n\nBut with/after it:\n\n```\n[blame]\n\tignoreRevsFile=.git-blame-ignore-revs\n```\n\nThis is the effective config. Not what the user has typed out.\n\nIf the user types out this:\n\n```\n[blame]\n\tignoreRevsFile=.git-blame-more-revs\n```\n\nThen this becomes their effective config:\n\n```\n[blame]\n\tignoreRevsFile=.git-blame-ignore-revs\n\tignoreRevsFile=.git-blame-more-revs\n```\n\nNow there are two files: the default one and the user-supplied one (this\nconfig variable is documented as being multi-valued: “This option may be\nrepeated multiple times.”).\n\n§ How to ignore this new default §§§\n\nConsidering users who do not want this new default:\n\n```\n[blame]\n\tignoreRevsFile=\n```\n\nThis is the change they would have to make. Because a blank/empty\nresets/empties the list of files.\n\nOn Sat, Oct 12, 2024, at 06:37, Abhijeetsingh Meena via GitGitGadget wrote:\n> From: Abhijeetsingh Meena \n>\n> git-blame(1) can ignore a list of commits with `--ignore-revs-file`.\n> This is useful for marking uninteresting commits like formatting\n> changes, refactors and whatever else should not be “blamed”. Some\n> projects even version control this file so that all contributors can\n> use it; the conventional name is `.git-blame-ignore-revs`.\n>\n> But each user still has to opt-in to the standard ignore list,\n> either with this option or with the config `blame.ignoreRevsFile`.\n> Let’s teach git-blame(1) to respect this conventional file in order\n> to streamline the process.\n>\n> Signed-off-by: Abhijeetsingh Meena \n> ---\n> builtin/blame.c | 8 ++++++++\n> t/t8015-blame-default-ignore-revs.sh | 26 ++++++++++++++++++++++++++\n> 2 files changed, 34 insertions(+)\n> create mode 100755 t/t8015-blame-default-ignore-revs.sh\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index e407a22da3b..1eddabaf60f 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -1105,6 +1105,14 @@ parse_done:\n> \t\tadd_pending_object(&revs, &head_commit->object, \"HEAD\");\n> \t}\n>\n> +\t/*\n> +\t* By default, add .git-blame-ignore-revs to the list of files\n> +\t* containing revisions to ignore if it exists.\n> +\t*/\n> +\tif (access(\".git-blame-ignore-revs\", F_OK) == 0) {\n> +\t\tstring_list_append(&ignore_revs_file_list, \".git-blame-ignore-revs\");\n> +\t}\n> +\n\nI have not tested these patches. But I see why you check for file access/existence. Because with this config:\n\n```\n[blame]\n\tignoreRevsFile=.git-blame-ignore-revs\n```\n\nI get this warning in repositories that don’t have the file:\n\n```\nfatal: could not open object name list: .git-blame-ignore-revs\n```\n\nWhich is just noise.\n\nI get the same thing with Git Notes namespace configurations. I need to\nconfigure them for certain repositories (like `amlog` in this project),\nbut then I get warnings about them when using the relevant commands in a\nproject that does not have them.\n\nMaybe this is totally off-topic but I think it would make more sense if\n`blame.ignoreRevsFile` just didn’t say anything if it didn’t find the\nfile. Because the point of the config might be to opt-in to this file\nfor those projects that does have it.\n\n> \tinit_scoreboard(&sb);\n> \tsb.revs = &revs;\n> \tsb.contents_from = contents_from;\n> diff --git a/t/t8015-blame-default-ignore-revs.sh\n> b/t/t8015-blame-default-ignore-revs.sh\n> new file mode 100755\n> index 00000000000..d4ab686f14d\n> --- /dev/null\n> +++ b/t/t8015-blame-default-ignore-revs.sh\n> @@ -0,0 +1,26 @@\n> +#!/bin/sh\n> +\n> +test_description='default revisions to ignore when blaming'\n> +\n> +TEST_PASSES_SANITIZE_LEAK=true\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'blame: default-ignore-revs-file' '\n> + test_commit first-commit hello.txt hello &&\n> +\n> + echo world >>hello.txt &&\n> + test_commit second-commit hello.txt &&\n> +\n> + sed \"1s/hello/hi/\" hello.txt.tmp &&\n> + mv hello.txt.tmp hello.txt &&\n> + test_commit third-commit hello.txt &&\n> +\n> + git rev-parse HEAD >ignored-file &&\n> + git blame --ignore-revs-file=ignored-file hello.txt >expect &&\n> + git rev-parse HEAD >.git-blame-ignore-revs &&\n> + git blame hello.txt >actual &&\n> +\n> + test_cmp expect actual\n> +'\n> +\n> +test_done\n> --\n> gitgitgadget\n\n-- \nKristoffer\n"},{"id":"504913","messageId":"09a4cec6-c655-44e9-9577-01e6a8cb1a3d@app.fastmail.com","threadId":"62278","inReplyTo":"8d2fa3af7964dacd09d454de4325b1d5eb7a5c3d.1728707867.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] blame: introduce --override-ignore-revs to bypass ignore revisions list","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-10-12T14:25:23Z","receivedAt":"2024-10-12T14:25:54Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Sat, Oct 12, 2024, at 06:37, Abhijeetsingh Meena via GitGitGadget wrote:\n> From: Abhijeetsingh Meena \n>\n> The git blame command can ignore a list of revisions specified either\n> through the --ignore-revs-file option or the blame.ignoreRevsFile\n> configuration. This feature is useful for excluding irrelevant\n> commits, such as formatting changes or large refactors, from blame\n> annotations.\n\nIn a later paragraph you mention `--ignore-rev` but not here.\n\n> However, users may encounter cases where they need to\n> temporarily override these configurations to inspect all commits,\n> even those excluded by the ignore list. Currently, there is no\n> simple way to bypass all ignore revisions settings in one go.\n\n“No simple way” gives me pause. But there are those options/methods\nthat we discussed before:\n\n• `--no-ignore-rev`\n• `--no-ignore-revs-file`\n\nThese are not documented but I can provide these options and get a\ndifferent output from git-blame(1).\n\n`builtin/blame.c` uses `parse-options.h` which provides automatic\nnegated options. I just looked at the code today (so it’s new to me)\nbut it seems like it will empty the lists that are associated with these\noptions. See `parse-options-cb.c:parse_opt_string_list`.\n\nSo I think this should be sufficient to reset all “ignore” options:\n\n```\ngit blame --no-ignore-rev --no-ignore-revs-file\n```\n\nHowever I tested with this:\n\n```\ngit blame --ignore-revs-file=.git-blame-ignore-revs --no-ignore-revs\n```\n\nAnd the output suggests to me that `--no-ignore-revs` affect the result\nof the before-mentioned list of files. Even though these are two\ndifferent lists. I can’t make sense of that from the code. But I’m not\na C programmer so this might just be a me-problem.\n\n>\n> This patch introduces the --override-ignore-revs option (or -O),\n\nPhrases like “this patch” is discouraged compared to the imperative mood\nstyle of commanding the code base to change (so to speak). See\n`imperative-mood` in Documentation/SubmittingPatches.\n\n> which allows users to easily bypass the --ignore-revs-file\n> option, --ignore-rev option and the blame.ignoreRevsFile\n\nI can see no precedence for the name “override” for an option in this\nproject. The convention is `--[no-]option`.\n\nLike Eric Sunshine discussed: a common convention is to let the user\nactivate and negate options according to the last-wins rule. This is\npretty useful in my opinion. Because I can then make an alias which\ndisplays some Git Note:\n\n```\ntimber = log [options] --notes=results\n```\n\nBut then what if I don’t want any notes for a specific invocation? I\ndon’t have to copy the whole alias and modify it. I can just:\n\n```\ngit timber --no-notes\n```\n\nAnd the same goes for an alias which disables notes:\n\n```\ntimber = log [options] --no-notes\n```\n\nBecause then I can use `git timber --notes=results`.\n\n> configuration. When this option is used, git blame will completely\n> disregard all configured ignore revisions lists.\n>\n> The motivation behind this feature is to provide users with more\n> flexibility when dealing with large codebases that rely on\n> .git-blame-ignore-revs files for shared configurations, while\n> still allowing them to disable the ignore list when necessary\n> for troubleshooting or deeper inspections.\n\nYou might be able to achieve the same thing with the existing negated\noptions.\n\nIf you *cannot* disable all “ignore” config and options in one negated\none then you might want an option like `--no-ignores` which acts like:\n\n```\ngit blame --no-ignore-rev --no-ignore-revs-file\n```\n\n>\n> Signed-off-by: Abhijeetsingh Meena \n> ---\n> builtin/blame.c | 8 +++++++-\n> t/t8016-blame-override-ignore-revs.sh | 25 +++++++++++++++++++++++++\n> 2 files changed, 32 insertions(+), 1 deletion(-)\n> create mode 100755 t/t8016-blame-override-ignore-revs.sh\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 1eddabaf60f..956520edcd9 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -69,6 +69,7 @@ static int coloring_mode;\n> static struct string_list ignore_revs_file_list = STRING_LIST_INIT_DUP;\n> static int mark_unblamable_lines;\n> static int mark_ignored_lines;\n> +static int override_ignore_revs = 0;\n>\n> static struct date_mode blame_date_mode = { DATE_ISO8601 };\n> static size_t blame_date_width;\n> @@ -901,6 +902,7 @@ int cmd_blame(int argc,\n> \t\tOPT_BIT('w', NULL, &xdl_opts, N_(\"ignore whitespace differences\"),\n> XDF_IGNORE_WHITESPACE),\n> \t\tOPT_STRING_LIST(0, \"ignore-rev\", &ignore_rev_list, N_(\"rev\"),\n> N_(\"ignore when blaming\")),\n> \t\tOPT_STRING_LIST(0, \"ignore-revs-file\", &ignore_revs_file_list,\n> N_(\"file\"), N_(\"ignore revisions from \")),\n> +\t\tOPT_BOOL('O', \"override-ignore-revs\", &override_ignore_revs,\n> N_(\"override all configurations that exclude revisions\")),\n> \t\tOPT_BIT(0, \"color-lines\", &output_option, N_(\"color redundant\n> metadata from previous line differently\"), OUTPUT_COLOR_LINE),\n> \t\tOPT_BIT(0, \"color-by-age\", &output_option, N_(\"color lines by age\"),\n> OUTPUT_SHOW_AGE_WITH_COLOR),\n> \t\tOPT_BIT(0, \"minimal\", &xdl_opts, N_(\"spend extra cycles to find\n> better match\"), XDF_NEED_MINIMAL),\n> @@ -1119,7 +1121,11 @@ parse_done:\n> \tsb.reverse = reverse;\n> \tsb.repo = the_repository;\n> \tsb.path = path;\n> -\tbuild_ignorelist(&sb, &ignore_revs_file_list, &ignore_rev_list);\n> +\n> +\tif (!override_ignore_revs) {\n> +\t\tbuild_ignorelist(&sb, &ignore_revs_file_list, &ignore_rev_list);\n> +\t}\n> +\n\nThis demonstrates the more limited behavior: you either override\n(discard) the ignores or you don’t. With the negated options you build\nup and reset/empty those lists before you get to this point. That ends\nup being more flexible for the user.\n\n> \tstring_list_clear(&ignore_revs_file_list, 0);\n> \tstring_list_clear(&ignore_rev_list, 0);\n> \tsetup_scoreboard(&sb, &o);\n> diff --git a/t/t8016-blame-override-ignore-revs.sh\n> b/t/t8016-blame-override-ignore-revs.sh\n> new file mode 100755\n> index 00000000000..b5899729f8e\n> --- /dev/null\n> +++ b/t/t8016-blame-override-ignore-revs.sh\n> @@ -0,0 +1,25 @@\n> +#!/bin/sh\n> +\n> +test_description='default revisions to ignore when blaming'\n> +\n> +TEST_PASSES_SANITIZE_LEAK=true\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'blame: override-ignore-revs' '\n> + test_commit first-commit hello.txt hello &&\n> +\n> + echo world >>hello.txt &&\n> + test_commit second-commit hello.txt &&\n> +\n> + sed \"1s/hello/hi/\" hello.txt.tmp &&\n> + mv hello.txt.tmp hello.txt &&\n> + test_commit third-commit hello.txt &&\n> +\n> + git blame hello.txt >expect &&\n> + git rev-parse HEAD >.git-blame-ignore-revs &&\n> + git blame -O hello.txt >actual &&\n> +\n> + test_cmp expect actual\n> +'\n> +\n> +test_done\n> --\n> gitgitgadget\n"},{"id":"504949","messageId":"0ac9d9a1-cb91-4b1c-bba1-022ecc485684@gmail.com","threadId":"62278","inReplyTo":"4ed930cab1b7f5e9738e73c7b9374d927a8acd94.1728707867.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] blame: respect .git-blame-ignore-revs automatically","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-10-13T15:18:29Z","receivedAt":"2024-10-13T15:18:35Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Abhijeetsingh\n\nOn 12/10/2024 05:37, Abhijeetsingh Meena via GitGitGadget wrote:\n> From: Abhijeetsingh Meena \n> \n> git-blame(1) can ignore a list of commits with `--ignore-revs-file`.\n> This is useful for marking uninteresting commits like formatting\n> changes, refactors and whatever else should not be “blamed”. Some\n> projects even version control this file so that all contributors can\n> use it; the conventional name is `.git-blame-ignore-revs`.\n> \n> But each user still has to opt-in to the standard ignore list,\n> either with this option or with the config `blame.ignoreRevsFile`.\n> Let’s teach git-blame(1) to respect this conventional file in order\n> to streamline the process.\n\nIt's good that the commit message now mentions the config setting. It \nwould be helpful to explain why the original implementation deliberately \ndecided not to implement a default file and explain why it is now a good \nidea to do so. Supporting a default file in addition to the files listed \nin blame.ignoreRevsFile config setting leaves us in an odd position \ncompared to other settings which use a fixed name like .gitignore or \nhave a default that can be overridden by a config setting like \ncore.excludesFile or require a config setting to enable the feature like \ndiff.orderFile.\n\nI've left a couple of code comments below but really the most important \nthings are to come up with a convincing reason for changing the behavior \nand figuring out how the default file should interact with the config \nsetting.\n\n> +\t/*\n> +\t* By default, add .git-blame-ignore-revs to the list of files\n> +\t* containing revisions to ignore if it exists.\n> +\t*/\n> +\tif (access(\".git-blame-ignore-revs\", F_OK) == 0) {\n\nThere are some uses of \"access(.., F_OK)\" in our code base but it is \nmore usual to call file_exists() these days.\n\n> +\t\tstring_list_append(&ignore_revs_file_list, \".git-blame-ignore-revs\");\n\nIf the user already has this path in their config we'll waste time \nparsing it twice. We could avoid that by using a \"struct strset\" rather \nthan a \"struct string_list\". I don't think we have OPT_STRSET but it \nshould be easy to add one by copying OPT_STRING_LIST.\n\n> + echo world >>hello.txt &&\n> + test_commit second-commit hello.txt &&\n\ntest_commit overwrites the file it is committing so you need to use the \n--printf option\n\n\ttest_commit --printf second-commit hello.txt \"hello\\nworld\\n\"\n\n> + git rev-parse HEAD >ignored-file &&\n> + git blame --ignore-revs-file=ignored-file hello.txt >expect &&\n> + git rev-parse HEAD >.git-blame-ignore-revs &&\n> + git blame hello.txt >actual &&\n> + test_cmp expect actual\n\nI have mixed feelings about this sort of differential testing, comparing \nthe actual output of git blame to what we expect makes it unambiguous \nthat the test is checking what we want it to.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"504950","messageId":"2a119bdd-93ba-468b-bf80-bea41a5e8e60@gmail.com","threadId":"62278","inReplyTo":"8d2fa3af7964dacd09d454de4325b1d5eb7a5c3d.1728707867.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] blame: introduce --override-ignore-revs to bypass ignore revisions list","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-10-13T15:20:51Z","receivedAt":"2024-10-13T15:20:55Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Abhijeetsingh\n\nOn 12/10/2024 05:37, Abhijeetsingh Meena via GitGitGadget wrote:\n> From: Abhijeetsingh Meena \n> \n> The git blame command can ignore a list of revisions specified either\n> through the --ignore-revs-file option or the blame.ignoreRevsFile\n> configuration. This feature is useful for excluding irrelevant\n> commits, such as formatting changes or large refactors, from blame\n> annotations.\n> \n> However, users may encounter cases where they need to\n> temporarily override these configurations to inspect all commits,\n> even those excluded by the ignore list. Currently, there is no\n> simple way to bypass all ignore revisions settings in one go.\n\nAs Kristoffer has pointed out --no-ignore-revs-file should be sufficient \nto disable the default file. If it isn't we should fix it so that it is, \nnot add a new option.\n\nBest Wishes\n\nPhillip\n\n> This patch introduces the --override-ignore-revs option (or -O),\n> which allows users to easily bypass the --ignore-revs-file\n> option, --ignore-rev option and the blame.ignoreRevsFile\n> configuration. When this option is used, git blame will completely\n> disregard all configured ignore revisions lists.\n> \n> The motivation behind this feature is to provide users with more\n> flexibility when dealing with large codebases that rely on\n> .git-blame-ignore-revs files for shared configurations, while\n> still allowing them to disable the ignore list when necessary\n> for troubleshooting or deeper inspections.\n> \n> Signed-off-by: Abhijeetsingh Meena \n> ---\n> builtin/blame.c | 8 +++++++-\n> t/t8016-blame-override-ignore-revs.sh | 25 +++++++++++++++++++++++++\n> 2 files changed, 32 insertions(+), 1 deletion(-)\n> create mode 100755 t/t8016-blame-override-ignore-revs.sh\n> \n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 1eddabaf60f..956520edcd9 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -69,6 +69,7 @@ static int coloring_mode;\n> static struct string_list ignore_revs_file_list = STRING_LIST_INIT_DUP;\n> static int mark_unblamable_lines;\n> static int mark_ignored_lines;\n> +static int override_ignore_revs = 0;\n> \n> static struct date_mode blame_date_mode = { DATE_ISO8601 };\n> static size_t blame_date_width;\n> @@ -901,6 +902,7 @@ int cmd_blame(int argc,\n> \t\tOPT_BIT('w', NULL, &xdl_opts, N_(\"ignore whitespace differences\"), XDF_IGNORE_WHITESPACE),\n> \t\tOPT_STRING_LIST(0, \"ignore-rev\", &ignore_rev_list, N_(\"rev\"), N_(\"ignore when blaming\")),\n> \t\tOPT_STRING_LIST(0, \"ignore-revs-file\", &ignore_revs_file_list, N_(\"file\"), N_(\"ignore revisions from \")),\n> +\t\tOPT_BOOL('O', \"override-ignore-revs\", &override_ignore_revs, N_(\"override all configurations that exclude revisions\")),\n> \t\tOPT_BIT(0, \"color-lines\", &output_option, N_(\"color redundant metadata from previous line differently\"), OUTPUT_COLOR_LINE),\n> \t\tOPT_BIT(0, \"color-by-age\", &output_option, N_(\"color lines by age\"), OUTPUT_SHOW_AGE_WITH_COLOR),\n> \t\tOPT_BIT(0, \"minimal\", &xdl_opts, N_(\"spend extra cycles to find better match\"), XDF_NEED_MINIMAL),\n> @@ -1119,7 +1121,11 @@ parse_done:\n> \tsb.reverse = reverse;\n> \tsb.repo = the_repository;\n> \tsb.path = path;\n> -\tbuild_ignorelist(&sb, &ignore_revs_file_list, &ignore_rev_list);\n> +\n> +\tif (!override_ignore_revs) {\n> +\t\tbuild_ignorelist(&sb, &ignore_revs_file_list, &ignore_rev_list);\n> +\t}\n> +\n> \tstring_list_clear(&ignore_revs_file_list, 0);\n> \tstring_list_clear(&ignore_rev_list, 0);\n> \tsetup_scoreboard(&sb, &o);\n> diff --git a/t/t8016-blame-override-ignore-revs.sh b/t/t8016-blame-override-ignore-revs.sh\n> new file mode 100755\n> index 00000000000..b5899729f8e\n> --- /dev/null\n> +++ b/t/t8016-blame-override-ignore-revs.sh\n> @@ -0,0 +1,25 @@\n> +#!/bin/sh\n> +\n> +test_description='default revisions to ignore when blaming'\n> +\n> +TEST_PASSES_SANITIZE_LEAK=true\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'blame: override-ignore-revs' '\n> + test_commit first-commit hello.txt hello &&\n> +\n> + echo world >>hello.txt &&\n> + test_commit second-commit hello.txt &&\n> +\n> + sed \"1s/hello/hi/\" hello.txt.tmp &&\n> + mv hello.txt.tmp hello.txt &&\n> + test_commit third-commit hello.txt &&\n> +\n> + git blame hello.txt >expect &&\n> + git rev-parse HEAD >.git-blame-ignore-revs &&\n> + git blame -O hello.txt >actual &&\n> +\n> + test_cmp expect actual\n> +'\n> +\n> +test_done\n\n"},{"id":"504951","messageId":"98261da2-c8a6-4ae2-b62d-323093f94bfb@gmail.com","threadId":"62278","inReplyTo":"4f835282-d921-4cd3-9680-d8522bef7b39@app.fastmail.com","subject":"Re: [PATCH v2 1/2] blame: respect .git-blame-ignore-revs automatically","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-10-13T15:25:51Z","receivedAt":"2024-10-13T15:25:56Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Kristoffer\n\nOn 12/10/2024 14:58, Kristoffer Haugsbakk wrote:\n> Hi Abhijeetsingh\n> \n> Maybe this is totally off-topic but I think it would make more sense if\n> `blame.ignoreRevsFile` just didn’t say anything if it didn’t find the\n> file. Because the point of the config might be to opt-in to this file\n> for those projects that does have it.\n\nSee https://lore.kernel.org/git/xmqqr1f5hszw.fsf@gitster.g/ for some \ndiscussion about this\n\nBest Wishes\n\nPhillip\n\n"},{"id":"505059","messageId":"a4261533-33b3-49f6-861b-3571c2666c81@app.fastmail.com","threadId":"62278","inReplyTo":"98261da2-c8a6-4ae2-b62d-323093f94bfb@gmail.com","subject":"Re: [PATCH v2 1/2] blame: respect .git-blame-ignore-revs automatically","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-10-14T21:00:19Z","receivedAt":"2024-10-14T21:00:41Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Sun, Oct 13, 2024, at 17:25, Phillip Wood wrote:\n> Hi Kristoffer\n>> […]\n>\n> See https://lore.kernel.org/git/xmqqr1f5hszw.fsf@gitster.g/ for some\n> discussion about this\n>\n> Best Wishes\n>\n> Phillip\n\nThanks! That was an interesting read. And an interesting idea.\n\nAnd then today we got this:\n\nhttps://lore.kernel.org/git/xmqq5ywehb69.fsf@gitster.g/T/#mce170a493a7b324c585124a9124356a0f87c77a6\n\n-- \nKristoffer\n\n"},{"id":"505064","messageId":"Zw2IYMPsywFCN5Yj@nand.local","threadId":"62278","inReplyTo":"CAPig+cR4WbcDDav0cdXxOMC-EDe2ipWxEzB+0C7zbFjvY_kXtg@mail.gmail.com","subject":"Re: [PATCH v2 1/2] blame: respect .git-blame-ignore-revs automatically","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-14T21:08:48Z","receivedAt":"2024-10-14T21:08:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Oct 12, 2024 at 02:07:36AM -0400, Eric Sunshine wrote:\n> > diff --git a/builtin/blame.c b/builtin/blame.c\n> > @@ -1105,6 +1105,14 @@ parse_done:\n> > + /*\n> > + * By default, add .git-blame-ignore-revs to the list of files\n> > + * containing revisions to ignore if it exists.\n> > + */\n> > + if (access(\".git-blame-ignore-revs\", F_OK) == 0) {\n> > + string_list_append(&ignore_revs_file_list, \".git-blame-ignore-revs\");\n> > + }\n>\n> A couple style nits and a couple questions...\n>\n> nit: drop the braces around the one-line `if` body\n>\n> nit: this project uses `!foo(...)` rather than `foo(...) == 0`\n>\n> Presumably this consults \".git-blame-ignore-revs\" in the top-level\n> directory (as you intended) rather than \".git-blame-ignore-revs\" in\n> whatever subdirectory you happen to issue the command because the\n> current-working-directory has already been set to the top-level\n> directory by the time cmd_blame() has been called, right?\n>\n> But that leads to the next question. Should automatic consulting of\n> \".git-blame-ignore-revs\" be restricted to just the top-level\n> directory, or should it be modeled after, say, \".gitignore\" which may\n> be strewn around project directories and in which \".gitignore\" files\n> are consulted rootward starting from the directory in which the\n> command is invoked. My knee-jerk thought was that the \".gitignore\"\n> model may not make sense for \".git-blame-ignore-revs\", but the fact\n> that `git blame` can accept and work with multiple ignore-revs files\n> makes me question that knee-jerk response.\n\nAll very good suggestions and questions for Abhijeetsingh to consider.\n\nAt a minimum, I think the style nits need to be addressed here. But I\nalso think it is worth considering seriously whether or not multiple\n`.git-blame-ignore-revs` files should be considered, and if so, in what\norder and how they override (or not) each other.\n\nI am generally OK with adding one of these special files and having 'git\nblame' respect it automatically. But once we do so, it is going to be\nconsidered part of our compatibility guarantee, so we should get it\nright the first time.\n\nThanks,\nTaylor\n"},{"id":"505231","messageId":"CAAirc3hF2L1U7ZoQM0pYYiqtnA28o=78MyyxR9eK7GXN9Jfr1A@mail.gmail.com","threadId":"62278","inReplyTo":"CAPig+cR4WbcDDav0cdXxOMC-EDe2ipWxEzB+0C7zbFjvY_kXtg@mail.gmail.com","subject":"Re: [PATCH v2 1/2] blame: respect .git-blame-ignore-revs automatically","fromName":"Abhijeetsingh Meena","fromEmail":"abhijeetsingh.github@gmail.com","sentAt":"2024-10-16T06:04:59Z","receivedAt":"2024-10-16T06:05:12Z","isPatch":true,"sender":{"key":"abhijeetsingh.github@gmail.com","avatar":null},"body":"Hi Eric,\n\nThank you for your thoughtful feedback on v2 of the patch.\nBefore I proceed with v3, I'd like to address some of the\nnon-code-related questions and seek your input.\n\n> Presumably this consults \".git-blame-ignore-revs\" in the top-level\n> directory (as you intended) rather than \".git-blame-ignore-revs\" in\n> whatever subdirectory you happen to issue the command because the\n> current-working-directory has already been set to the top-level\n> directory by the time cmd_blame() has been called, right?\n\nYes, it seems that the current-working-directory is set to the root of\nthe repository,\nas I tested this behaviour locally. The .git-blame-ignore-revs file in\nthe root worked\nas expected, while a similar file in a subdirectory did not.\n\n\n> But that leads to the next question. Should automatic consulting of\n> \".git-blame-ignore-revs\" be restricted to just the top-level\n> directory, or should it be modeled after, say, \".gitignore\" which may\n> be strewn around project directories and in which \".gitignore\" files\n> are consulted rootward starting from the directory in which the\n> command is invoked. My knee-jerk thought was that the \".gitignore\"\n> model may not make sense for \".git-blame-ignore-revs\", but the fact\n> that `git blame` can accept and work with multiple ignore-revs files\n> makes me question that knee-jerk response.\n\nI think both approaches have their merits:\n\n1. Single file\n*Purpose:* Having a single .git-blame-ignore-revs file aligns with the idea\nof globally ignoring revisions, making it easier for maintainers to\ncontrol irrelevant commits.\n*Simplicity:* Keeping the file in the root ensures centralized management,\nsimplifying configuration.\n\n2. Multiple files:\n*Large repositories:* In large monorepos, different teams working in separate\nsubdirectories may want to manage their own ignored revisions. Multiple files\nwould offer flexibility, particularly for modular projects or those\nwith distinct submodules.\n*Flexibility:* Subdirectory-level .git-blame-ignore-revs files could\nallow users to\n fine-tune blame results for their specific areas, especially when\nlocal refactors\nare limited to certain parts of the codebase.\n\nGiven this, I would like to know your suggestions, as I’m not too\nexperienced with\nthe user workflows and what would be more helpful to them. For now, I think we\nshould stick with the single .git-blame-ignore-revs file at the top level.\nHowever, we could keep the option open for future enhancements, allowing\nmultiple files to be consulted by setting a configuration flag if a\nspecific use case arises.\n\n\n> Is the all-or-nothing behavior implemented by this patch desirable? If\n> so, should the command warn or error out if the user gives conflicting\n> options like --ignore-revs-file and --override-ignore-revs together?\n>\n> A common behavior of many Git commands when dealing with options is\n> \"last wins\", and following that precedent could make this new option\n> even much more useful by allowing the user to ignore project-supplied\n> ignore-revs but still take advantage of the feature with a different\n> set of ignore-revs that make sense to the local user. For instance:\n>\n> git blame --override-ignore-revs --ignore-revs-file=my-ignore-revs ...\n\nI don’t think the all-or-nothing approach is ideal. Based on Phillip's\nsuggestions and\nKristoffer's conceptual workflow, I explored using `git_config_set` to\nset `blame.ignoreRevsFile`\nconfiguration. This would integrate well with existing configuration\nlogic and provide greater flexibility.\n\nWith `git_config_set`:\n\ngit blame hello.txt\nwould consult the default .git-blame-ignore-revs file.\n\ngit blame --no-ignore-revs-file hello.txt\nwould disable the default ignore file.\n\ngit blame --no-ignore-revs-file --ignore-revs-file=ignore-list hello.txt\nwould allow the user to specify a custom ignore list while bypassing\nthe global list,\noffering the flexibility you suggested.\n\n\n> What is this test actually checking? It doesn't seem to use\n> --override-ignore-revs at all.\n\nActually, I used the short form -O to represent --override-ignore-revs\nin this test.\n\nThank you again for your time and feedback. I look forward to your thoughts on\nthese points before finalising the next patch revision.\n\nBest regards,\nAbhijeet\n\nOn Sat, Oct 12, 2024 at 11:37 AM Eric Sunshine wrote:\n>\n> On Sat, Oct 12, 2024 at 12:38 AM Abhijeetsingh Meena via GitGitGadget\n> wrote:\n> > git-blame(1) can ignore a list of commits with `--ignore-revs-file`.\n> > This is useful for marking uninteresting commits like formatting\n> > changes, refactors and whatever else should not be “blamed”. Some\n> > projects even version control this file so that all contributors can\n> > use it; the conventional name is `.git-blame-ignore-revs`.\n> >\n> > But each user still has to opt-in to the standard ignore list,\n> > either with this option or with the config `blame.ignoreRevsFile`.\n> > Let’s teach git-blame(1) to respect this conventional file in order\n> > to streamline the process.\n> >\n> > Signed-off-by: Abhijeetsingh Meena \n> > ---\n> > builtin/blame.c | 8 ++++++++\n> > t/t8015-blame-default-ignore-revs.sh | 26 ++++++++++++++++++++++++++\n> > 2 files changed, 34 insertions(+)\n>\n> This change should be accompanied by a documentation update, I would think.\n>\n> > diff --git a/builtin/blame.c b/builtin/blame.c\n> > @@ -1105,6 +1105,14 @@ parse_done:\n> > + /*\n> > + * By default, add .git-blame-ignore-revs to the list of files\n> > + * containing revisions to ignore if it exists.\n> > + */\n> > + if (access(\".git-blame-ignore-revs\", F_OK) == 0) {\n> > + string_list_append(&ignore_revs_file_list, \".git-blame-ignore-revs\");\n> > + }\n>\n> A couple style nits and a couple questions...\n>\n> nit: drop the braces around the one-line `if` body\n>\n> nit: this project uses `!foo(...)` rather than `foo(...) == 0`\n>\n> Presumably this consults \".git-blame-ignore-revs\" in the top-level\n> directory (as you intended) rather than \".git-blame-ignore-revs\" in\n> whatever subdirectory you happen to issue the command because the\n> current-working-directory has already been set to the top-level\n> directory by the time cmd_blame() has been called, right?\n>\n> But that leads to the next question. Should automatic consulting of\n> \".git-blame-ignore-revs\" be restricted to just the top-level\n> directory, or should it be modeled after, say, \".gitignore\" which may\n> be strewn around project directories and in which \".gitignore\" files\n> are consulted rootward starting from the directory in which the\n> command is invoked. My knee-jerk thought was that the \".gitignore\"\n> model may not make sense for \".git-blame-ignore-revs\", but the fact\n> that `git blame` can accept and work with multiple ignore-revs files\n> makes me question that knee-jerk response.\n>\n> > diff --git a/t/t8015-blame-default-ignore-revs.sh b/t/t8015-blame-default-ignore-revs.sh\n> > new file mode 100755\n>\n> Let's avoid allocating a new test number just for this single new\n> test. Instead, the existing t8013-blame-ignore-revs.sh would probably\n> be a good home for this new test.\n>\n> > +test_expect_success 'blame: default-ignore-revs-file' '\n> > + test_commit first-commit hello.txt hello &&\n> > +\n> > + echo world >>hello.txt &&\n> > + test_commit second-commit hello.txt &&\n> > +\n> > + sed \"1s/hello/hi/\" hello.txt.tmp &&\n>\n> style: drop space after redirection operator\n>\n> sed \"1s/hello/hi/\" hello.txt.tmp &&\n>\n> > + mv hello.txt.tmp hello.txt &&\n> > + test_commit third-commit hello.txt &&\n> > +\n> > + git rev-parse HEAD >ignored-file &&\n> > + git blame --ignore-revs-file=ignored-file hello.txt >expect &&\n> > + git rev-parse HEAD >.git-blame-ignore-revs &&\n> > + git blame hello.txt >actual &&\n>\n> I would suggest copying or renaming \"ignored-file\" to\n> \".git-blame-ignore-revs\" rather than running `git rev-parse HEAD`\n> twice. This way readers won't have to waste mental effort verifying\n> that the result of `git rev-parse HEAD` isn't intended to change\n> between invocations.\n"},{"id":"505232","messageId":"CAAirc3jqDf_jHudiJZaD4+pMYYj01xhG1398+i4B=J22Dqm7GQ@mail.gmail.com","threadId":"62278","inReplyTo":"4f835282-d921-4cd3-9680-d8522bef7b39@app.fastmail.com","subject":"Re: [PATCH v2 1/2] blame: respect .git-blame-ignore-revs automatically","fromName":"Abhijeetsingh Meena","fromEmail":"abhijeetsingh.github@gmail.com","sentAt":"2024-10-16T06:06:12Z","receivedAt":"2024-10-16T06:06:25Z","isPatch":true,"sender":{"key":"abhijeetsingh.github@gmail.com","avatar":null},"body":"Hi Kristoffer,\n\nThank you for reviewing the v2 of my patch. I appreciate your\nthoughtful feedback.\nBefore proceeding with v3, I’d like to address some of your questions\nand suggestions.\n\n> Hi Abhijeetsingh\n>\n> For what it’s worth here’s how I imagine this feature could work\n> conceptually:\n>\n> Before this feature/change, the effective config for Git use looks like this:\n>\n> ```\n> [blame]\n> ```\n>\n> No `blame.ignoreRevsFile`.\n>\n> But with/after it:\n>\n> ```\n> [blame]\n> ignoreRevsFile=.git-blame-ignore-revs\n> ```\n>\n> This is the effective config. Not what the user has typed out.\n>\n> If the user types out this:\n>\n> ```\n> [blame]\n> ignoreRevsFile=.git-blame-more-revs\n> ```\n>\n> Then this becomes their effective config:\n>\n> ```\n> [blame]\n> ignoreRevsFile=.git-blame-ignore-revs\n> ignoreRevsFile=.git-blame-more-revs\n> ```\n>\n> Now there are two files: the default one and the user-supplied one (this\n> config variable is documented as being multi-valued: “This option may be\n> repeated multiple times.”).\n>\n> § How to ignore this new default §§§\n>\n> Considering users who do not want this new default:\n>\n> ```\n> [blame]\n> ignoreRevsFile=\n> ```\n>\n> This is the change they would have to make. Because a blank/empty\n> resets/empties the list of files.\n\nThanks, Kristoffer. Your conceptual explanation gave me a new\nperspective on how this\nfeature can be implemented using the existing configuration flow\nwithout disrupting\nother settings. It has helped shape the solution, as I described in my\nresponse to Eric earlier.\n\nBased on Phillip's clue of exploring how this feature would interact\nwith existing configuration\nsettings and your conceptual workflow, I explored git_config_set and\nused it to set the\nblame.ignoreRevsFile configuration. This approach fits well with the\nexisting configuration\nlogic and provides greater flexibility.\n\nWith git_config_set to set blame.ignoreRevsFile:\n\ngit blame hello.txt\nwould consult the default .git-blame-ignore-revs file.\n\ngit blame --no-ignore-revs-file hello.txt\nwould disable the default ignore file.\n\ngit blame --no-ignore-revs-file --ignore-revs-file=ignore-list hello.txt\nwould allow the user to specify a custom ignore list while bypassing\nthe global list,\noffering the flexibility you suggested.\n\nThis would maintain consistency with Git’s existing behavior, allowing\nusers to modify\nconfigurations with a “last-wins” approach and enabling both global\nand custom ignore\nlists as needed.\n\n\n> I have not tested these patches. But I see why you check for file access/existence. Because with this config:\n>\n> ```\n> [blame]\n> ignoreRevsFile=.git-blame-ignore-revs\n> ```\n>\n> I get this warning in repositories that don’t have the file:\n>\n> ```\n> fatal: could not open object name list: .git-blame-ignore-revs\n> ```\n>\n> Which is just noise.\n>\n> I get the same thing with Git Notes namespace configurations. I need to\n> configure them for certain repositories (like `amlog` in this project),\n> but then I get warnings about them when using the relevant commands in a\n> project that does not have them.\n>\n> Maybe this is totally off-topic but I think it would make more sense if\n> `blame.ignoreRevsFile` just didn’t say anything if it didn’t find the\n> file. Because the point of the config might be to opt-in to this file\n> for those projects that does have it.\n\nYes, I agree. For a default ignore file, we shouldn't raise a fatal\nerror if the file is missing, especially if it’s not present in every\nrepository.\nSuppressing the warning for the default file would improve user experience\nand prevent unnecessary noise.\n\n\n> > However, users may encounter cases where they need to\n> > temporarily override these configurations to inspect all commits,\n> > even those excluded by the ignore list. Currently, there is no\n> > simple way to bypass all ignore revisions settings in one go.\n>\n> “No simple way” gives me pause. But there are those options/methods\n> that we discussed before:\n>\n> • `--no-ignore-rev`\n> • `--no-ignore-revs-file`\n>\n> These are not documented but I can provide these options and get a\n> different output from git-blame(1).\n>\n> `builtin/blame.c` uses `parse-options.h` which provides automatic\n> negated options. I just looked at the code today (so it’s new to me)\n> but it seems like it will empty the lists that are associated with these\n> options. See `parse-options-cb.c:parse_opt_string_list`.\n>\n> So I think this should be sufficient to reset all “ignore” options:\n>\n> ```\n> git blame --no-ignore-rev --no-ignore-revs-file\n> ```\n>\n> However I tested with this:\n>\n> ```\n> git blame --ignore-revs-file=.git-blame-ignore-revs --no-ignore-revs\n> ```\n>\n> And the output suggests to me that `--no-ignore-revs` affect the result\n> of the before-mentioned list of files. Even though these are two\n> different lists. I can’t make sense of that from the code. But I’m not\n> a C programmer so this might just be a me-problem.\n\nYes, --no-ignore-revs-file and --no-ignore-rev flags work as intended\nto bypass the configuration that ignores revisions. They are separate lists, so\n--no-ignore-revs shouldn’t affect the --ignore-revs-file list. My previous\n testing post v1 had some issues in test setup, which led me to believe that\nthe --no-ignore flags don’t work and I worked on --override-ignore-revs.\n\n\n> > which allows users to easily bypass the --ignore-revs-file\n> > option, --ignore-rev option and the blame.ignoreRevsFile\n>\n> I can see no precedence for the name “override” for an option in this\n> project. The convention is `--[no-]option`.\n>\n> Like Eric Sunshine discussed: a common convention is to let the user\n> activate and negate options according to the last-wins rule. This is\n> pretty useful in my opinion. Because I can then make an alias which\n> displays some Git Note:\n>\n> ```\n> timber = log [options] --notes=results\n> ```\n>\n> But then what if I don’t want any notes for a specific invocation? I\n> don’t have to copy the whole alias and modify it. I can just:\n>\n> ```\n> git timber --no-notes\n> ```\n>\n> And the same goes for an alias which disables notes:\n>\n> ```\n> timber = log [options] --no-notes\n> ```\n>\n> Because then I can use `git timber --notes=results`.\n\nI agree that the override option is unnecessary, as both\n--no-ignore-rev and --no-ignore-revs-file\nalready allow users to bypass the ignore configurations. Also, the\n“last-wins” approach is more\nuseful and aligns with how Git typically handles configurations. It’s\nflexible and user-friendly,\nallowing for easy toggling within aliases or individual commands.\nImplementing this using the\nexisting configuration method, such as git_config_set, would be a\nclean and effective solution\nto ensure that users can quickly modify or negate options as needed.\n\n\n> > configuration. When this option is used, git blame will completely\n> > disregard all configured ignore revisions lists.\n> >> The motivation behind this feature is to provide users with more\n> > flexibility when dealing with large codebases that rely on\n> > .git-blame-ignore-revs files for shared configurations, while\n> > still allowing them to disable the ignore list when necessary\n> > for troubleshooting or deeper inspections.\n>\n> You might be able to achieve the same thing with the existing negated\n> options.\n>\n> If you *cannot* disable all “ignore” config and options in one negated\n> one then you might want an option like `--no-ignores` which acts like:\n>\n> ```\n> git blame --no-ignore-rev --no-ignore-revs-file\n> ```\n\nYes, the override option isn’t necessary since the existing flags work\nas intended.\nIf needed in the future, we can add a single flag to reset both lists, or as you\nmentioned it can be an alias too.\n\n\n> > + if (!override_ignore_revs) {\n> > + build_ignorelist(&sb, &ignore_revs_file_list, &ignore_rev_list);\n> > + }\n> > +\n>\n> This demonstrates the more limited behavior: you either override\n> (discard) the ignores or you don’t. With the negated options you build\n> up and reset/empty those lists before you get to this point. That ends\n> up being more flexible for the user.\n\nYes, this approach was more limited, we can follow the approach\ndescribed earlier that uses git_config_set to handle ignoring\nrevisions and revision lists more flexibly.\n\n\nThanks again for your detailed feedback. I hope this approach is\nbetter than my previous approach.\nI’ll incorporate these changes and move forward with v3. Looking\nforward to your further thoughts!\n\nBest regards,\nAbhijeetsingh\n\nOn Sat, Oct 12, 2024 at 7:28 PM Kristoffer Haugsbakk\n wrote:\n>\n> Hi Abhijeetsingh\n>\n> For what it’s worth here’s how I imagine this feature could work\n> conceptually:\n>\n> Before this feature/change, the effective config for Git use looks like this:\n>\n> ```\n> [blame]\n> ```\n>\n> No `blame.ignoreRevsFile`.\n>\n> But with/after it:\n>\n> ```\n> [blame]\n> ignoreRevsFile=.git-blame-ignore-revs\n> ```\n>\n> This is the effective config. Not what the user has typed out.\n>\n> If the user types out this:\n>\n> ```\n> [blame]\n> ignoreRevsFile=.git-blame-more-revs\n> ```\n>\n> Then this becomes their effective config:\n>\n> ```\n> [blame]\n> ignoreRevsFile=.git-blame-ignore-revs\n> ignoreRevsFile=.git-blame-more-revs\n> ```\n>\n> Now there are two files: the default one and the user-supplied one (this\n> config variable is documented as being multi-valued: “This option may be\n> repeated multiple times.”).\n>\n> § How to ignore this new default §§§\n>\n> Considering users who do not want this new default:\n>\n> ```\n> [blame]\n> ignoreRevsFile=\n> ```\n>\n> This is the change they would have to make. Because a blank/empty\n> resets/empties the list of files.\n>\n> On Sat, Oct 12, 2024, at 06:37, Abhijeetsingh Meena via GitGitGadget wrote:\n> > From: Abhijeetsingh Meena \n> >\n> > git-blame(1) can ignore a list of commits with `--ignore-revs-file`.\n> > This is useful for marking uninteresting commits like formatting\n> > changes, refactors and whatever else should not be “blamed”. Some\n> > projects even version control this file so that all contributors can\n> > use it; the conventional name is `.git-blame-ignore-revs`.\n> >\n> > But each user still has to opt-in to the standard ignore list,\n> > either with this option or with the config `blame.ignoreRevsFile`.\n> > Let’s teach git-blame(1) to respect this conventional file in order\n> > to streamline the process.\n> >\n> > Signed-off-by: Abhijeetsingh Meena \n> > ---\n> > builtin/blame.c | 8 ++++++++\n> > t/t8015-blame-default-ignore-revs.sh | 26 ++++++++++++++++++++++++++\n> > 2 files changed, 34 insertions(+)\n> > create mode 100755 t/t8015-blame-default-ignore-revs.sh\n> >\n> > diff --git a/builtin/blame.c b/builtin/blame.c\n> > index e407a22da3b..1eddabaf60f 100644\n> > --- a/builtin/blame.c\n> > +++ b/builtin/blame.c\n> > @@ -1105,6 +1105,14 @@ parse_done:\n> > add_pending_object(&revs, &head_commit->object, \"HEAD\");\n> > }\n> >\n> > + /*\n> > + * By default, add .git-blame-ignore-revs to the list of files\n> > + * containing revisions to ignore if it exists.\n> > + */\n> > + if (access(\".git-blame-ignore-revs\", F_OK) == 0) {\n> > + string_list_append(&ignore_revs_file_list, \".git-blame-ignore-revs\");\n> > + }\n> > +\n>\n> I have not tested these patches. But I see why you check for file access/existence. Because with this config:\n>\n> ```\n> [blame]\n> ignoreRevsFile=.git-blame-ignore-revs\n> ```\n>\n> I get this warning in repositories that don’t have the file:\n>\n> ```\n> fatal: could not open object name list: .git-blame-ignore-revs\n> ```\n>\n> Which is just noise.\n>\n> I get the same thing with Git Notes namespace configurations. I need to\n> configure them for certain repositories (like `amlog` in this project),\n> but then I get warnings about them when using the relevant commands in a\n> project that does not have them.\n>\n> Maybe this is totally off-topic but I think it would make more sense if\n> `blame.ignoreRevsFile` just didn’t say anything if it didn’t find the\n> file. Because the point of the config might be to opt-in to this file\n> for those projects that does have it.\n>\n> > init_scoreboard(&sb);\n> > sb.revs = &revs;\n> > sb.contents_from = contents_from;\n> > diff --git a/t/t8015-blame-default-ignore-revs.sh\n> > b/t/t8015-blame-default-ignore-revs.sh\n> > new file mode 100755\n> > index 00000000000..d4ab686f14d\n> > --- /dev/null\n> > +++ b/t/t8015-blame-default-ignore-revs.sh\n> > @@ -0,0 +1,26 @@\n> > +#!/bin/sh\n> > +\n> > +test_description='default revisions to ignore when blaming'\n> > +\n> > +TEST_PASSES_SANITIZE_LEAK=true\n> > +. ./test-lib.sh\n> > +\n> > +test_expect_success 'blame: default-ignore-revs-file' '\n> > + test_commit first-commit hello.txt hello &&\n> > +\n> > + echo world >>hello.txt &&\n> > + test_commit second-commit hello.txt &&\n> > +\n> > + sed \"1s/hello/hi/\" hello.txt.tmp &&\n> > + mv hello.txt.tmp hello.txt &&\n> > + test_commit third-commit hello.txt &&\n> > +\n> > + git rev-parse HEAD >ignored-file &&\n> > + git blame --ignore-revs-file=ignored-file hello.txt >expect &&\n> > + git rev-parse HEAD >.git-blame-ignore-revs &&\n> > + git blame hello.txt >actual &&\n> > +\n> > + test_cmp expect actual\n> > +'\n> > +\n> > +test_done\n> > --\n> > gitgitgadget\n>\n> --\n> Kristoffer\n"},{"id":"505233","messageId":"CAAirc3hkiQ95kzniP4Ws_op8nWuY4tdNrCjrPXMDShq3KX_h+g@mail.gmail.com","threadId":"62278","inReplyTo":"0ac9d9a1-cb91-4b1c-bba1-022ecc485684@gmail.com","subject":"Re: [PATCH v2 1/2] blame: respect .git-blame-ignore-revs automatically","fromName":"Abhijeetsingh Meena","fromEmail":"abhijeetsingh.github@gmail.com","sentAt":"2024-10-16T06:07:27Z","receivedAt":"2024-10-16T06:07:40Z","isPatch":true,"sender":{"key":"abhijeetsingh.github@gmail.com","avatar":null},"body":"Hi Phillip,\nThank you for reviewing the patch and providing valuable feedback.\nI’d like to address some of your points below:\n\n\n> Supporting a default file in addition to the files listed in\n> blame.ignoreRevsFile config setting leaves us in an odd position\n> compared to other settings which use a fixed name like .gitignore\n> or have a default that can be overridden by a config setting like\n> core.excludesFile or require a config setting to enable the feature\n> like diff.orderFile.\n\nYes, I now understand that we can solve this by using the existing method for\ninteracting with configurations, as suggested by you and Kristoffer. We can work\nwith the existing configuration method like git_config_set to set ignore\nrevisions file. This (I hope) will also keep it consistent with how\nother settings like .gitignore\nand core.excludesFile work, making the interaction more predictable for users.\n\n\n> I've left a couple of code comments below but really\n> the most important things are to come up with a convincing\n> reason for changing the behavior and figuring out how\n> the default file should interact with the config setting.\n\nI agree. After revisiting the use case and the flow, I see now that\nthe solution can be\nmore straightforward with git_config_set than my previous approach. This\nbehavior allows for interaction through the configuration system\nwithout the need to\nintroduce new options. Kristoffer’s suggestion clarified that handling\n.git-blame-ignore-revs\n a default file and allowing it to be overridden or disabled via\n--no-ignore-revs-file is sufficient.\n\n\n> As Kristoffer has pointed out --no-ignore-revs-file should\n> be sufficient to disable the default file. If it isn't we\n> should fix it so that it is, not add a new option.\n\nAbsolutely, you're right. After revisiting my earlier testing issues,\nI realized that the\n--no-ignore-revs-file and --no-ignore-rev flag works as intended. My\nprevious confusion was due to a mistake in my test setup. I agree with your\nsuggestion that we should not add a new option and instead focus on ensuring\n that the current flag behavior is clear and functions correctly.\n\n\nThanks again for your review. I hope this approach is better than my\nprevious approach.\nI’ll make sure the changes are implemented correctly in v3\nand test the interaction between the default file and config settings\nmore thoroughly.\nLooking forward to your further thoughts!\n\nBest regards,\nAbhijeetsingh\n\nOn Sun, Oct 13, 2024 at 8:48 PM Phillip Wood wrote:\n>\n> Hi Abhijeetsingh\n>\n> On 12/10/2024 05:37, Abhijeetsingh Meena via GitGitGadget wrote:\n> > From: Abhijeetsingh Meena \n> >\n> > git-blame(1) can ignore a list of commits with `--ignore-revs-file`.\n> > This is useful for marking uninteresting commits like formatting\n> > changes, refactors and whatever else should not be “blamed”. Some\n> > projects even version control this file so that all contributors can\n> > use it; the conventional name is `.git-blame-ignore-revs`.\n> >\n> > But each user still has to opt-in to the standard ignore list,\n> > either with this option or with the config `blame.ignoreRevsFile`.\n> > Let’s teach git-blame(1) to respect this conventional file in order\n> > to streamline the process.\n>\n> It's good that the commit message now mentions the config setting. It\n> would be helpful to explain why the original implementation deliberately\n> decided not to implement a default file and explain why it is now a good\n> idea to do so. Supporting a default file in addition to the files listed\n> in blame.ignoreRevsFile config setting leaves us in an odd position\n> compared to other settings which use a fixed name like .gitignore or\n> have a default that can be overridden by a config setting like\n> core.excludesFile or require a config setting to enable the feature like\n> diff.orderFile.\n>\n> I've left a couple of code comments below but really the most important\n> things are to come up with a convincing reason for changing the behavior\n> and figuring out how the default file should interact with the config\n> setting.\n>\n> > + /*\n> > + * By default, add .git-blame-ignore-revs to the list of files\n> > + * containing revisions to ignore if it exists.\n> > + */\n> > + if (access(\".git-blame-ignore-revs\", F_OK) == 0) {\n>\n> There are some uses of \"access(.., F_OK)\" in our code base but it is\n> more usual to call file_exists() these days.\n>\n> > + string_list_append(&ignore_revs_file_list, \".git-blame-ignore-revs\");\n>\n> If the user already has this path in their config we'll waste time\n> parsing it twice. We could avoid that by using a \"struct strset\" rather\n> than a \"struct string_list\". I don't think we have OPT_STRSET but it\n> should be easy to add one by copying OPT_STRING_LIST.\n>\n> > + echo world >>hello.txt &&\n> > + test_commit second-commit hello.txt &&\n>\n> test_commit overwrites the file it is committing so you need to use the\n> --printf option\n>\n> test_commit --printf second-commit hello.txt \"hello\\nworld\\n\"\n>\n> > + git rev-parse HEAD >ignored-file &&\n> > + git blame --ignore-revs-file=ignored-file hello.txt >expect &&\n> > + git rev-parse HEAD >.git-blame-ignore-revs &&\n> > + git blame hello.txt >actual &&\n> > + test_cmp expect actual\n>\n> I have mixed feelings about this sort of differential testing, comparing\n> the actual output of git blame to what we expect makes it unambiguous\n> that the test is checking what we want it to.\n>\n> Best Wishes\n>\n> Phillip\n>\n"},{"id":"505809","messageId":"CAAirc3gAFqa5Ox=Hp26zBXuKwE5OmHDE+j-4MnKysrgfD8u2pg@mail.gmail.com","threadId":"62278","inReplyTo":"CAAirc3hkiQ95kzniP4Ws_op8nWuY4tdNrCjrPXMDShq3KX_h+g@mail.gmail.com","subject":"Re: [PATCH v2 1/2] blame: respect .git-blame-ignore-revs automatically","fromName":"Abhijeetsingh Meena","fromEmail":"abhijeetsingh.github@gmail.com","sentAt":"2024-10-22T06:49:35Z","receivedAt":"2024-10-22T06:49:50Z","isPatch":true,"sender":{"key":"abhijeetsingh.github@gmail.com","avatar":null},"body":"Hi Eric, Kristoffer, and Phillip,\n\nThank you for reviewing the v2 of my patch. I appreciate your\nthoughtful feedback.\n\nA few days ago, I sent a detailed email addressing each of your\nquestions and suggestions individually. Before proceeding with v3,\nI’d like to consolidate my thoughts on the next possible approach\nto respect '.git-blame-ignore-revs' based on Kristoffer's conceptual\nexplanation.\n\n> Hi Abhijeetsingh\n>\n> For what it’s worth here’s how I imagine this feature could work\n> conceptually:\n>\n> Before this feature/change, the effective config for Git use looks like this:\n>\n> ```\n> [blame]\n> ```\n>\n> No `blame.ignoreRevsFile`.\n>\n> But with/after it:\n>\n> ```\n> [blame]\n> ignoreRevsFile=.git-blame-ignore-revs\n> ```\n>\n> This is the effective config. Not what the user has typed out.\n>\n> If the user types out this:\n>\n> ```\n> [blame]\n> ignoreRevsFile=.git-blame-more-revs\n> ```\n>\n> Then this becomes their effective config:\n>\n> ```\n> [blame]\n> ignoreRevsFile=.git-blame-ignore-revs\n> ignoreRevsFile=.git-blame-more-revs\n> ```\n>\n> Now there are two files: the default one and the user-supplied one (this\n> config variable is documented as being multi-valued: “This option may be\n> repeated multiple times.”).\n>\n> § How to ignore this new default §§§\n>\n> Considering users who do not want this new default:\n>\n> ```\n> [blame]\n> ignoreRevsFile=\n> ```\n>\n> This is the change they would have to make. Because a blank/empty\n> resets/empties the list of files.\n\nThanks, Kristoffer. Your conceptual explanation gave me a new\nperspective on how this feature can be implemented using the\nexisting configuration flow without disrupting other settings.\n\nBased on Phillip's clue of exploring how this feature would interact\nwith existing configuration settings and your conceptual workflow,\nI explored git_config_set and used it to set the\nblame.ignoreRevsFile configuration. This approach aligns well with\nthe existing configuration logic and provides greater flexibility.\n\nWith git_config_set to set blame.ignoreRevsFile:\n\ngit blame hello.txt\nwould consult the default .git-blame-ignore-revs file.\n\ngit blame --no-ignore-revs-file hello.txt\nwould disable the default ignore file.\n\ngit blame --no-ignore-revs-file --ignore-revs-file=ignore-list hello.txt\nwould allow the user to specify a custom ignore list while bypassing\nthe global list, offering the flexibility you suggested.\n\nThis would maintain consistency with Git’s existing behavior, allowing\nusers to modify configurations with a “last-wins” approach and enabling\nboth global and custom ignore lists as needed.\n\nI hope this approach is better than my previous one. I look forward to\nyour thoughts!\n\nBest Regards,\nAbhijeetsingh\n\nOn Wed, Oct 16, 2024 at 11:37 AM Abhijeetsingh Meena\n wrote:\n>\n> Hi Phillip,\n> Thank you for reviewing the patch and providing valuable feedback.\n> I’d like to address some of your points below:\n>\n>\n> > Supporting a default file in addition to the files listed in\n> > blame.ignoreRevsFile config setting leaves us in an odd position\n> > compared to other settings which use a fixed name like .gitignore\n> > or have a default that can be overridden by a config setting like\n> > core.excludesFile or require a config setting to enable the feature\n> > like diff.orderFile.\n>\n> Yes, I now understand that we can solve this by using the existing method for\n> interacting with configurations, as suggested by you and Kristoffer. We can work\n> with the existing configuration method like git_config_set to set ignore\n> revisions file. This (I hope) will also keep it consistent with how\n> other settings like .gitignore\n> and core.excludesFile work, making the interaction more predictable for users.\n>\n>\n> > I've left a couple of code comments below but really\n> > the most important things are to come up with a convincing\n> > reason for changing the behavior and figuring out how\n> > the default file should interact with the config setting.\n>\n> I agree. After revisiting the use case and the flow, I see now that\n> the solution can be\n> more straightforward with git_config_set than my previous approach. This\n> behavior allows for interaction through the configuration system\n> without the need to\n> introduce new options. Kristoffer’s suggestion clarified that handling\n> .git-blame-ignore-revs\n> a default file and allowing it to be overridden or disabled via\n> --no-ignore-revs-file is sufficient.\n>\n>\n> > As Kristoffer has pointed out --no-ignore-revs-file should\n> > be sufficient to disable the default file. If it isn't we\n> > should fix it so that it is, not add a new option.\n>\n> Absolutely, you're right. After revisiting my earlier testing issues,\n> I realized that the\n> --no-ignore-revs-file and --no-ignore-rev flag works as intended. My\n> previous confusion was due to a mistake in my test setup. I agree with your\n> suggestion that we should not add a new option and instead focus on ensuring\n> that the current flag behavior is clear and functions correctly.\n>\n>\n> Thanks again for your review. I hope this approach is better than my\n> previous approach.\n> I’ll make sure the changes are implemented correctly in v3\n> and test the interaction between the default file and config settings\n> more thoroughly.\n> Looking forward to your further thoughts!\n>\n> Best regards,\n> Abhijeetsingh\n>\n> On Sun, Oct 13, 2024 at 8:48 PM Phillip Wood wrote:\n> >\n> > Hi Abhijeetsingh\n> >\n> > On 12/10/2024 05:37, Abhijeetsingh Meena via GitGitGadget wrote:\n> > > From: Abhijeetsingh Meena \n> > >\n> > > git-blame(1) can ignore a list of commits with `--ignore-revs-file`.\n> > > This is useful for marking uninteresting commits like formatting\n> > > changes, refactors and whatever else should not be “blamed”. Some\n> > > projects even version control this file so that all contributors can\n> > > use it; the conventional name is `.git-blame-ignore-revs`.\n> > >\n> > > But each user still has to opt-in to the standard ignore list,\n> > > either with this option or with the config `blame.ignoreRevsFile`.\n> > > Let’s teach git-blame(1) to respect this conventional file in order\n> > > to streamline the process.\n> >\n> > It's good that the commit message now mentions the config setting. It\n> > would be helpful to explain why the original implementation deliberately\n> > decided not to implement a default file and explain why it is now a good\n> > idea to do so. Supporting a default file in addition to the files listed\n> > in blame.ignoreRevsFile config setting leaves us in an odd position\n> > compared to other settings which use a fixed name like .gitignore or\n> > have a default that can be overridden by a config setting like\n> > core.excludesFile or require a config setting to enable the feature like\n> > diff.orderFile.\n> >\n> > I've left a couple of code comments below but really the most important\n> > things are to come up with a convincing reason for changing the behavior\n> > and figuring out how the default file should interact with the config\n> > setting.\n> >\n> > > + /*\n> > > + * By default, add .git-blame-ignore-revs to the list of files\n> > > + * containing revisions to ignore if it exists.\n> > > + */\n> > > + if (access(\".git-blame-ignore-revs\", F_OK) == 0) {\n> >\n> > There are some uses of \"access(.., F_OK)\" in our code base but it is\n> > more usual to call file_exists() these days.\n> >\n> > > + string_list_append(&ignore_revs_file_list, \".git-blame-ignore-revs\");\n> >\n> > If the user already has this path in their config we'll waste time\n> > parsing it twice. We could avoid that by using a \"struct strset\" rather\n> > than a \"struct string_list\". I don't think we have OPT_STRSET but it\n> > should be easy to add one by copying OPT_STRING_LIST.\n> >\n> > > + echo world >>hello.txt &&\n> > > + test_commit second-commit hello.txt &&\n> >\n> > test_commit overwrites the file it is committing so you need to use the\n> > --printf option\n> >\n> > test_commit --printf second-commit hello.txt \"hello\\nworld\\n\"\n> >\n> > > + git rev-parse HEAD >ignored-file &&\n> > > + git blame --ignore-revs-file=ignored-file hello.txt >expect &&\n> > > + git rev-parse HEAD >.git-blame-ignore-revs &&\n> > > + git blame hello.txt >actual &&\n> > > + test_cmp expect actual\n> >\n> > I have mixed feelings about this sort of differential testing, comparing\n> > the actual output of git blame to what we expect makes it unambiguous\n> > that the test is checking what we want it to.\n> >\n> > Best Wishes\n> >\n> > Phillip\n> >\n"},{"id":"505810","messageId":"CAPig+cTVjUvE-P1RPHehV6uN3f+wMWmpUpiD8JNqzUQDoNuHhA@mail.gmail.com","threadId":"62278","inReplyTo":"CAAirc3gAFqa5Ox=Hp26zBXuKwE5OmHDE+j-4MnKysrgfD8u2pg@mail.gmail.com","subject":"Re: [PATCH v2 1/2] blame: respect .git-blame-ignore-revs automatically","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-10-22T07:54:34Z","receivedAt":"2024-10-22T07:54:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Oct 22, 2024 at 2:49 AM Abhijeetsingh Meena\n wrote:\n> Hi Eric, Kristoffer, and Phillip,\n>\n> A few days ago, I sent a detailed email addressing each of your\n> questions and suggestions individually. Before proceeding with v3,\n> I’d like to consolidate my thoughts on the next possible approach\n> to respect '.git-blame-ignore-revs' based on Kristoffer's conceptual\n> explanation.\n\nAlthough I asked several questions (which popped into my head) during\nmy review of the patch, I don't have much to add to the discussion\nsince I don't use this feature and either wasn't aware of it or forgot\nabout it until your patch arrived.\n\nPhillip is off-list through the end of the month, so it is unlikely\nthat he will respond before then.\n\nThe general idea of implementing this new behavior via config-list may\nindeed be a reasonable way to make it more well-integrated with the\nexisting methods of specifying ignore-revs. However, that's also an\nimplementation detail which seems less important presently than these\npossible open concerns (unless they've been answered elsewhere), all\nof which have to do with forward and backward compatibility...\n\n* Will automatic reading of top-level .git-blame-ignore-revs cause any\nunexpected behaviors for existing users who (presumably) are using it\nvia configuration? It seems unlikely, but the implementation ought to\nbe careful to process .git-blame-ignore-revs only once.\n\n* Will automatic reading of .git-blame-ignore-revs cause unexpected\nbehavior for people who have not set up the configuration? In other\nwords, maybe some projects have a .git-blame-ignore-revs but some\nusers don't want it consulted by default, thus avoid setting the\nconfiguration.\n\n* It sounds like you want to punt on the idea of cumulatively\nprocessing .git-blame-ignore-revs files from the current subdirectory\nup to the project root (as .gitignore works), but can this be done in\nsuch a way that we don't break user's expectations or automations if\nsuch behavior is eventually implemented?\n"}]}