Skip to content

Conversation

@mohanmanikanta2299
Copy link
Contributor

@mohanmanikanta2299 mohanmanikanta2299 commented Jan 13, 2026

🛠️ Description

  • copywrite headers now not only adds headers to files without headers but also updates year-1 and year-2 information in files with existing headers.
  • Updates will only happen if the organisation name in the header matches with the copyright_holder information in the .copywrite.hcl file. copyright_holder value is 'IBM Corp.' by default.
  • License file headers are updated by copywrite headers command if any copyright changes are detected in the current year from git commit history of the repo.
  • Year-1 is updated only when the first commit year of the repo is not same as the copyright_year value stored in the .copywrite.hcl file.
  • Any file that is updated in the current year or needs an update in the current year gets its year-2 updated to current year.
  • Copyright updates happen in place. Any additional text beyond the copyright statement is preserved.
  • Order of copyright_holder and copywrite license command validation with respect to new changes made in year information.
  • All occurrences of copyright statements in a file will be updated.
  • Readme file has been updated with the information and changes made.

🔗 External Links

👍 Definition of Done

✅ New functionality works?
✅ Tests added?

🤔 Can be merged upon approval?

PCI review checklist

  • I have documented a clear reason for, and description of, the change I am making.

  • If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.

  • If applicable, I've documented the impact of any changes to security controls.

    Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.

@mohanmanikanta2299 mohanmanikanta2299 requested a review from a team as a code owner January 13, 2026 07:21
License, v. 2.0. If a copy of the MPL was not distributed with this
file, You can obtain one at https://mozilla.org/MPL/2.0/.`

const tmplSPDX = `Copyright (c){{ if .Holder }} {{.Holder}}{{ end }}{{ if .Year }} {{.Year}}{{ end }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we removing (c) was it discussed earlier? This can be configurable or keep (c) as-is unless there’s a strong policy reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a policy reason for the same. IBM header policy says that (c) symbol is not needed in the copyright statement which we are updating across the repos.
Moreover, refer the MEMO for the copyright statement format.

While running updates across repos in the org, we have removed the (c) symbol. None of the updated statements contain the (c) symbol.

The (c) symbol is not legally required. Since the Berne Convention (1989), copyright protection is automatic - the © or (c) symbol is optional.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

split into a separate, focused PR explicitly about header format standardization.

if lastCommitYear > info.EndYear || shouldUpdate {
// File was modified after copyright end year (or will be modified by us), update end year
// Use lastCommitYear if available, otherwise fall back to currentYear
targetEndYear := currentYear
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

End-year logic is not actually “git-aware”

So even if:

  • a file was last changed in 2023, and
  • there have been no commits touching that file since,

running the tool in 2026 will still change the header to end in 2026 ?

That means the header reflects when the tool was run, not when the file was last modified.

This contradicts:

  • the README explanation, and
  • the intent implied by function names like GetFileLastCommitYear().

If the goal is “git-aware” and historically accurate headers, the end year should come from:

  • the file’s last commit year (for source files), or
  • the repo’s last commit year (for LICENSE),

and only fall back to the current year if git data isn’t available.

Right now, the behavior is effectively “always bump to now”, which can unintentionally rewrite large parts of a repo without any real code changes.

PLEASE VERIFY THIS!!!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic is git aware currently.
There is no contradiction of what is mentioned in the readme vs what the code is actually doing.

The current behaviour for year-2 update is as follows:

  1. If copyright header's year-1 is updated due to change of copyright_year info in config file.
  2. If the file's year-2 value is < last commit year of file.

In both cases, the year-2 value will be updated to current year as the last commit year of that file will be current year post header statement fix.


To answer your query,

So even if:

  • a file was last changed in 2023, and
  • there have been no commits touching that file since,
    running the tool in 2026 will still change the header to end in 2026 ?

Ans -> No, the tool will not change the header to end in 2026. It will remain untouched.

Attached testing document which contains the test cases for the same.
Test_Copywrite_Repo.docx

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, what I meant is that you need to clarify the ReamMe in a more clear way, here's my view:

The README says the end year is derived from git history (e.g: “End year: file’s last commit year” / “repo’s last commit year”). However, in the code (calculateYearUpdates), git history is only used to decide whether an update is needed, and when an update happens the end year is set to currentYear, not the git commit year.

It would be good to clarify this nuance explicitly in the README

}

// GetFileLastCommitYear returns the year of the last commit that modified a file
func GetFileLastCommitYear(filePath string) (int, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential issue with GetFileLastCommitYear()
The function runs git log from the file’s directory and passes only the filename. This is unreliable for nested paths, since Git resolves file paths relative to the repo root. In such cases, Git may return no commits or incorrect results, silently causing the logic to fall back to the current year and bump copyright years incorrectly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current code walks through each file in each directory/subdirectory and covers all nested paths.

Code snippet here

However, for best practice, the fixed implementation is in place that runs from repo root with full relative paths.

cmd/headers.go Outdated
}

// updateExistingHeaders walks through files and updates copyright headers based on config and git history
func updateExistingHeaders(cmd *cobra.Command, ignoredPatterns []string, dryRun bool) int {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore pattern handling – please double-check consistency

In cmd/headers.go, the new updateExistingHeaders() path introduces a custom ignore matcher (shouldIgnoreFile / matchWildcard) while addlicense.Run() relies on its existing ignore handling.

The current implementation approximates ** and * matching (prefix/suffix + strings.Contains), which may not behave like standard glob semantics. For example:

  • patterns like **/*.go won’t match as expected,
  • .github/** or paths with leading ./ can behave inconsistently,
  • behavior may diverge between STEP 1 (updateExistingHeaders) and STEP 3 (addlicense.Run).

Could you please verify this against common ignore patterns and consider:

  • reusing the same matcher as addlicense.Run() for consistency, or
  • switching to a glob library with proper ** support (e.g. doublestar)?

This would help avoid surprising header updates in paths that are meant to be ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-used the same matched as addLicense.Run() for consistency.
Removed the matchWildcard function altogether.


// Try to update copyright in this file
if !dryRun {
updated, err := licensecheck.UpdateCopyrightHeader(path, targetHolder, configYear, false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only mark the repo as ‘changed’ when we update existing headers, but we don’t mark it as ‘changed’ when we add missing headers, so LICENSE may not be updated even though we modified files. Anyways, I am still re-collecting if we discussed anything regarding the behavior change for License.

Copy link
Contributor Author

@mohanmanikanta2299 mohanmanikanta2299 Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the fix to consider the repo as changed when headers are added. Added test cases in test file to show license update behaviour in the repo.

Note: License header is only updated when a valid header exists. For a license file with non-existent header, new header will not be added. (existing functionality)
copywrite license command adds headers to license files which do not have a header.(existing functionality)

@mohanmanikanta2299
Copy link
Contributor Author

Attached the testing document for the changes added : Test_Copywrite_Repo.docx

Copy link
Contributor

@CreatorHead CreatorHead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please split this PR into smaller, focused ones and test each feature in isolation? It’ll make the review and validation much easier. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants