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

avoid being tripped up by git status weirdness #40

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@rjbs
Contributor

rjbs commented Aug 29, 2015

it might have color (if color.status is always/yes), or change format in
future git, etc.

stick to --porcelain, which is guaranteed not to change

this probably paves the way for fixing #38, but I didn't attempt it

avoid being tripped up by git status weirdness
it might have color, or change format, etc.

stick to --porcelain, which is guaranteed not to change
@2shortplanks

This comment has been minimized.

Show comment
Hide comment
@2shortplanks

2shortplanks Aug 29, 2015

Contributor

I'm not sure I understand this completely.

  • aren't we capturing the wrong file name in the case of a rename? (Do we
    want -z?)
  • doesn't this get deleted files and other unwanted statuses too?

On Friday, August 28, 2015, Ricardo Signes notifications@github.com wrote:

it might have color (if color.status is always/yes), or change format in
future git, etc.

stick to --porcelain, which is guaranteed not to change

this probably paves the way for fixing #38
#38, but I

didn't attempt it

You can view, comment on, or merge this pull request online at:

#40
Commit Summary

  • avoid being tripped up by git status weirdness

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#40.

Contributor

2shortplanks commented Aug 29, 2015

I'm not sure I understand this completely.

  • aren't we capturing the wrong file name in the case of a rename? (Do we
    want -z?)
  • doesn't this get deleted files and other unwanted statuses too?

On Friday, August 28, 2015, Ricardo Signes notifications@github.com wrote:

it might have color (if color.status is always/yes), or change format in
future git, etc.

stick to --porcelain, which is guaranteed not to change

this probably paves the way for fixing #38
#38, but I

didn't attempt it

You can view, comment on, or merge this pull request online at:

#40
Commit Summary

  • avoid being tripped up by git status weirdness

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#40.

@rjbs

This comment has been minimized.

Show comment
Hide comment
@rjbs

rjbs Aug 31, 2015

Contributor

You're right, I will do more work on this. My testing (which was non-zero) was naive.

Contributor

rjbs commented Aug 31, 2015

You're right, I will do more work on this. My testing (which was non-zero) was naive.

use git status --porcelain -z
This was surprisingly obnoxious.

It sounds great, at first, that git uses \0 terminators, but its
records are of variable length, and the record and field separators
are the same -- \0!

So, first you read in the change mode (in format "XY ") and then the
filename ("filename\0").  Then, if the mode was a rename, you read in
a second filename and throw it away.  The first filename is the
renamed-to filename.  (That's backward from the for-humans "x -> y"
format.)

Still, this is less ambiguous.  For example, if there are two ->
arrows in the human-readable "git status," the correct filename to
pick out is indeterminable.  Is it worth adding this complexity to
deal with human boneheadedness?  I leave that decision up to you.
@rjbs

This comment has been minimized.

Show comment
Hide comment
@rjbs

rjbs Oct 30, 2015

Contributor

I have updated the branch. You should read the commit (and message) decide whether it's worth merging. I think it probably is, but it's not an obvious thing.

Contributor

rjbs commented Oct 30, 2015

I have updated the branch. You should read the commit (and message) decide whether it's worth merging. I think it probably is, but it's not an obvious thing.

@autarch

This comment has been minimized.

Show comment
Hide comment
@autarch

autarch Nov 7, 2015

Contributor

There's a certain irony to the fact that Travis failing because you didn't run tidyall on your code ;)

Contributor

autarch commented Nov 7, 2015

There's a certain irony to the fact that Travis failing because you didn't run tidyall on your code ;)

@rjbs

This comment has been minimized.

Show comment
Hide comment
@rjbs

rjbs Nov 7, 2015

Contributor

Funny! :) :( I just ran prove -lr t

Contributor

rjbs commented Nov 7, 2015

Funny! :) :( I just ran prove -lr t

@autarch

This comment has been minimized.

Show comment
Hide comment
@autarch

autarch Nov 7, 2015

Contributor

I think I relegated all these tests to author or release testing so you'd need to run dzil test --release.

Contributor

autarch commented Nov 7, 2015

I think I relegated all these tests to author or release testing so you'd need to run dzil test --release.

@autarch

This comment has been minimized.

Show comment
Hide comment
@autarch

autarch Feb 13, 2016

Contributor

I tweaked this a bit to get tests passing and merged it from the CLI. Thanks!

Contributor

autarch commented Feb 13, 2016

I tweaked this a bit to get tests passing and merged it from the CLI. Thanks!

@autarch autarch closed this Feb 13, 2016

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