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

Fix #3627: use `git -c diff.noprefix=false diff` #3628

Closed

Conversation

Projects
None yet
2 participants
@Blaisorblade
Copy link

commented Oct 24, 2018

Fix #3627 by ensuring git produces a -p1 diff. This is a very limited
fix but appears to work in my usecase. I have reviewed the other uses of
git diff in the same file, but it appears their output is not passed
to patch hence this change is unnecessary.

Fix #3627: use git -c diff.noprefix=false diff
Fix #3627 by ensuring git produces a `-p1` diff. This is a very limited
fix but appears to work in my usecase. I have reviewed the other uses of
`git diff` in the same file, but it appears their output is not passed
to `patch` hence this change is unnecessary.
@rjbou

This comment has been minimized.

Copy link
Collaborator

commented Oct 25, 2018

Thanks for the fix!

Instead of putting the option in the diff command, it would be better to add it in the local config of the internal git repo here.

It's OpamVCS.fetchs_repo_upgrade which performs the first init and then uses the same directory to update it.

@Blaisorblade

This comment has been minimized.

Copy link
Author

commented Oct 25, 2018

Instead of putting the option in the diff command, it would be better to add it in the local config of the internal git repo here.

Makes sense, done and testing locally.

Still, I will note if I just change init, it seems existing repos won't be fixed; I can remove and readd the repo, but that's slightly not ideal. Not that I'd insist on this.

Overall, opam update is slow enough here that I guess resetting the config after init wouldn't hurt? Or one could pass the config to each command, but adding wrappers to avoid code duplication.

EDIT: worked exactly as predicted.

Alternative fix, as suggested during review
This will prevent the problem for repositories added in the future. Existing
repositories must be removed and readded (or fixed by hand).

I confirmed the above by local testing.
@rjbou

This comment has been minimized.

Copy link
Collaborator

commented Oct 26, 2018

It seems existing repos won't be fixed

Indeed. We can have both: the local config updated, and just adding it to that git command. Other git diffs don't generate patches (--quiet option), so there is no effect on them.
Like that new init won't be affected, existing repo are handled, and with the comments, new added diffs will have the good option to set or no (the wrapper can be set only if need be).

@rjbou

This comment has been minimized.

Copy link
Collaborator

commented Feb 28, 2019

@rjbou rjbou added this to the 2.1.0 milestone Feb 28, 2019

@Blaisorblade

This comment has been minimized.

Copy link
Author

commented Feb 28, 2019

@rjbou Sorry for the delay — been busy! I am not sure what you are asking for — in particular I don't get:

Like that new init won't be affected, existing repo are handled, and with the comments, new added diffs will have the good option to set or no (the wrapper can be set only if need be).

So, should we both set diff.noprefix=false both in the repo config and pass it to diff commands? Alternatively, maybe you can sketch which code you have in mind, and then I can revise/test it (or feel free to finish the PR yourself if that's easier).

@rjbou

This comment has been minimized.

Copy link
Collaborator

commented Mar 5, 2019

What I wanted to say is that, with the comment on git diffcommand, if a new git diff command will be added in this file, developer will see the comment and don't forget it (or set a wrapper on that time to avoid duplication, and don't forget about noprefix option).
What I proposed is to merge your two addition (at init & at diff). If you are busy ftm, i can update your PR with this modification.

@Blaisorblade

This comment has been minimized.

Copy link
Author

commented Mar 6, 2019

What I proposed is to merge your two addition (at init & at diff). If you are busy ftm, i can update your PR with this modification.

Please do, thanks!

@rjbou

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2019

Updated and rebased on master in the new pr #3788.
Thanks for the contrib!

@Blaisorblade

This comment has been minimized.

Copy link
Author

commented Mar 20, 2019

BTW, now I finally get what you meant — I thought you were talking about comments to the opam users (who are also developers)! Sorry for the confusion and thanks for taking it up :-)

@rjbou rjbou closed this in #3788 Mar 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.