Skip to content

Add rfc to remove an TODO: that seems to be referencing the wrong struct field commit.c:360 #1867

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

Conversation

lowspeedburt
Copy link

Thanks for taking the time to contribute to Git!

Those seeking to contribute to the Git for Windows fork should see
http://gitforwindows.org/#contribute on how to contribute Windows specific enhancements.

If your contribution is for the core Git functions and documentation
please be aware that the Git community does not use the github.com issues
or pull request mechanism for their contributions.

Instead, we use the Git mailing list ([email protected]) for code and
documenatation submissions, code reviews, and bug reports. The
mailing list is plain text only (anything with HTML is sent directly
to the spam folder).

Nevertheless, you can use submitGit to conveniently send your Pull
Requests commits to our mailing list.

Please read the "guidelines for contributing" linked above!

@lowspeedburt lowspeedburt force-pushed the skinnyal/microprojects branch from 4b3cd7a to 29c1c46 Compare October 7, 2018 04:40
@lowspeedburt
Copy link
Author

Was doing some digging within the code, and came across this. @dscho

Copy link
Member

@dscho dscho left a comment

Choose a reason for hiding this comment

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

A good initial version!

I left a couple of comments trying to help improve it to the point where we can offer it up on the Git mailing list.

commit.c Outdated

*/

This comment was marked as off-topic.

This comment was marked as off-topic.

@lowspeedburt
Copy link
Author

hey @dscho went ahead, and made the small change. Have been behind on some project work, but wanted to make sure that I had this checked in.

Would I just leave this here, or send this to the mailing list?

-Al

@dscho
Copy link
Member

dscho commented Oct 15, 2018

Would I just leave this here, or send this to the mailing list?

After renaming the commit message prefix from "TODO doc:" to "doc:" please do send this to the mailing list, it is ready!

@dscho
Copy link
Member

dscho commented Dec 31, 2018

@skinnyal how about it?

@lowspeedburt
Copy link
Author

Hey @dscho I'll work on this right now. Sorry about the delay school, and things got hectic.

@dscho
Copy link
Member

dscho commented Dec 31, 2018

@skinnyal could you do a git rebase -i on top of upstream's master, squashing all the iterative changes into a single commit/patch?

@lowspeedburt
Copy link
Author

@dscho I went ahead, and rebased. I think I did it in the correct order. I'm unsure if the merging to my local remote should be the latest entry in git log , or no.

@dscho
Copy link
Member

dscho commented Dec 31, 2018

I'm unsure if the merging to my local remote should be the latest entry in git log , or no.

Ideally, there should not even be a merge commit in your branch. There is no way to "export" a merge commit via format-patch. It cannot be represented as a patch.

@dscho
Copy link
Member

dscho commented Dec 31, 2018

Ideally, there should not even be a merge commit in your branch.

To clarify what I mean: this is what I am seeing in the Commits tab of this PR:

What I expected to see would look more like

  • doc: remove unneeded TODO for release_commit_memory

This could be achieved by running these commands:

git fetch https://github.com/git/git master
git rebase -i FETCH_HEAD

and then changing each pick except the very first one to squash. Then save the file and close the editor (or editor tab), and when the editor comes back up, edit the commit message appropriately (most likely just keeping the last commit message).

The code that was merged with commit 1102405 left in an TODO that
is no longer relevant. It seems as if we can remove this todo.
util seems to not be a field of the struct commit,
definition of struct commit --> commit.c:27
The commit list also, does not contain a field for util.

Signed-off-by: Albert Burt <[email protected]>
@lowspeedburt lowspeedburt force-pushed the skinnyal/microprojects branch from d449be9 to 642b097 Compare December 31, 2018 22:08
@lowspeedburt
Copy link
Author

Ah, Okay. I think it should be fixed now @dscho .

@dscho
Copy link
Member

dscho commented Dec 31, 2018

I think it should be fixed now

Yep! Thanks!

@lowspeedburt
Copy link
Author

No problemo! Thanks for following up with this. Sent it off to the mailing list!

@dscho
Copy link
Member

dscho commented Jan 1, 2019

@dscho dscho closed this Jan 1, 2019
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