Skip to content
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

Test stripsigns #1

Conversation

paulirish
Copy link

this PR (to scott's bash->perl branch) also contains the so-fancy#125 PR.

but, in 06b9421 i've added an extra test that currently fails.

you can see the failure in action here. it's using the same git show command that @stevemao introduced before.

image

hopefully the new symbol stripping fixes this bug.

@paulirish
Copy link
Author

this might be a bug in git???

here's git --no-pager show --color=never 74804e377d4a54d1173d4393827d4e4b27e4d5d0

check out the indentation before some of the +/-

image

@scottchiefbaker
Copy link
Owner

I assume you're referring to the remaining + and - as the problem?

I've seen git use the second column (in addition to the first) to indicate line additions/deletions but never knew why. I suspect looking at this, it's a three-way merge. It's most likely indicating that the line addition came from file 1, or file 2.

@paulirish
Copy link
Author

ahhhh yup. three way.. that's why the hunk header is also crazy

@scottchiefbaker
Copy link
Owner

This gets a little hairy... we have to basically remove the first two columns of indicators... or maybe three if it's a four way merge?

@paulirish
Copy link
Author

yup. that's going to be hard for us to reformat successfully, i think.. :/

we could ask git whether that commit is a merge commit and then use some more invasive parsing.

seems like we'll have to put this on the roadmap and admit we have poor support for the moment.

@scottchiefbaker
Copy link
Owner

We could look at the diff header, determine how many merges (X) there are and remove the first X-1 columns? That will require some more intelligence in the strip_leading_char() function.

@paulirish
Copy link
Author

True true.

i'm good with deferring our support for merge commits for now. Would rather get the fundamentals feeling good before we add this kinda complexity.

wdyt

@scottchiefbaker
Copy link
Owner

I agree... this is a good roadmap thing. Not a show stopper.

@scottchiefbaker
Copy link
Owner

Do you want me to merge this before you can merge the "move functions to perl" in the main repo? Will this break tests?

@paulirish
Copy link
Author

no, we wont merge this PR. we'll keep the test around for when we want to fully support merge commits.

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