Skip to content

ParsePatchHeader: Copy functionality of git mailinfo's cleanup_subject #17

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 16, 2020

Conversation

gwd
Copy link
Contributor

@gwd gwd commented Sep 11, 2020

Primarily to get rid of [PATCH] at the front, but while we're here
just be generally compatible with git am:

  • Remove re and variations
  • Remove whitespace
  • Remove anything in brackets

But only at the very beginning of the subject.

Inspired by
https://github.com/git/git/blob/master/mailinfo.c:cleanup_subject()

Signed-off-by: George Dunlap [email protected]

This fixes issue #16

Copy link
Owner

@bluekeyes bluekeyes left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this and including tests! I had a few style comments and a thought about the API:

One of the goals of this library is to be relatively un-opinionated about how parsed information is presented to clients. To that end, I'd prefer if this cleaning wasn't mandatory (and in fact git am offers flags to disable it.) My first thought is to define a standalone CleanPatchTitle function that cleans the title string. Clients could assign the result to the Title field if they want to persist it.

Another option might be add a RawTitle field in addition to the Title field, which now contains the cleaned version. But I think I like the function better: parsed dates used to have a "raw" field and I removed it because it was confusing to use.

I'm also open to other suggestions if you have ideas.

@gwd
Copy link
Contributor Author

gwd commented Sep 13, 2020

Re the API: I was originally thinking, nobody would ever want to make a commit with [PATCH] or Re: in the title. But at very least, someone might want to be able to parse version / series information out of something like [PATCH v3 5/17].

Probably the most useful thing to do for the latter case would be to keep whatever we end up deleting from the beginning of the subject line and putting it in another field inside PatchHeader. Maybe "SubjectPrefix"? I think the most common case would be for people to parse out the version / series information, and then possibly apply the patch to a tree. If people really want to apply the prefixes, they can just always apply SubjectPrefix + Title + \n + Body.

Primarily to get rid of [PATCH] at the front, but while we're here
just be generally compatible with `git am`:

 * Remove `re` and variations
 * Remove whitespace
 * Remove anything in brackets

But only at the very beginning of the subject.

Store anything removed in this way in PatchHeader.SubjectPrefix.

Inspired by
https://github.com/git/git/blob/master/mailinfo.c:cleanup_subject()

Signed-off-by: George Dunlap <[email protected]>
@gwd gwd force-pushed the working/copy-git-cleanup_subject branch from 74682f1 to bf196ee Compare September 14, 2020 12:22
Copy link
Owner

@bluekeyes bluekeyes left a comment

Choose a reason for hiding this comment

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

After thinking about this a bit more, SubjectPrefix feels like a reasonable approach to solve this. It matches the default behavior of both format-patch and am, replicating the function of the -k flag is easy for clients who want that, and it fits with the contract of PatchHeader that some fields may be empty after parsing. Thanks again for submitting and updating this.

@bluekeyes bluekeyes merged commit 379b893 into bluekeyes:master Sep 16, 2020
@gwd gwd deleted the working/copy-git-cleanup_subject branch September 16, 2020 09:11
@gwd
Copy link
Contributor Author

gwd commented Sep 16, 2020

No prob, thanks for writing the package! Saved me a lot of work. :-)

Re API -- I was thinking, since you already have Title and Body broken down, and you have the Message "convenience method", to put them together, what you might do is add options to Message to emulate the -k flag. I've been playing around with https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis and found it pretty convenient. So then if you wanted to duplicate git am behavior, you'd just call ph.Message(); but if you wanted git am -k, you'd call ph.Message(gitdiff.OptKeepPrefix()) or something like that.

That's not something I have time to do at the moment, but it's worth thinking about.

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