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

mach update-css has confusing semantics #10884

Closed
perlun opened this issue Apr 27, 2016 · 2 comments · Fixed by #10892
Closed

mach update-css has confusing semantics #10884

perlun opened this issue Apr 27, 2016 · 2 comments · Fixed by #10892
Labels
A-mach C-assigned There is someone working on resolving the issue E-less-complex Straightforward. Recommended for a new contributor. I-papercut Small but painful. L-python Python is required

Comments

@perlun
Copy link
Contributor

perlun commented Apr 27, 2016

I ran the following, after changing some code that caused tests that previously passed to fail:

$ ./mach update-css /tmp/foo

Then I tried with git diff. It didn't show me anything; I assumed the command hadn't worked. It was only when @jdm told me to git show that I realized it had behind-the-scenes creates a new commit with the .ini file changes in.

This is a confusing behavior and IMHO a bad default. If we really want it to automagically commit code (which I think is questionable to begin with), it should at least not just keep silent about it. The git commit output should be shown if we retain the current behavior. (but I'd vote for just leaving the files dirty since that'd be how I'd guess it would work by just looking at it from the outside.)

Looked a bit but I couldn't immediately find where in this code this happens.

@jdm jdm added I-papercut Small but painful. A-mach labels Apr 27, 2016
@KiChjang
Copy link
Contributor

I recall seeing a similar issue before... @wafflespeanut might know more.

@wafflespeanut
Copy link
Contributor

Yeah, that was update-wpt. We should do something similar to what we did for #9666 (i.e., pass --no-patch argument by default)

Code: python/servo/testing_commands.py

@wafflespeanut wafflespeanut added E-less-complex Straightforward. Recommended for a new contributor. L-python Python is required labels Apr 28, 2016
@wafflespeanut wafflespeanut added the C-assigned There is someone working on resolving the issue label Apr 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mach C-assigned There is someone working on resolving the issue E-less-complex Straightforward. Recommended for a new contributor. I-papercut Small but painful. L-python Python is required
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants