www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.learn - Where should I dump workarounds?

reply Dukc <ajieskola gmail.com> writes:
Well, I was working on std.range.chain (I'm new to contributing), 
and when trying to test locally:

...\phobos\std\range>rdmd -unittest -main package
C:\D\dmd2\windows\bin\..\..\src\phobos\std\path.d(1319): Error: 
pure function 'std.path.buildPath!char.buildPath' cannot call 
impure function 'std.path.buildPath!(const(char)[][]).buildPath'
C:\D\dmd2\windows\bin\..\..\src\phobos\std\path.d(1319): Error: 
 safe function 'std.path.buildPath!char.buildPath' cannot call 
 system function 'std.path.buildPath!(const(char)[][]).buildPath'
C:\D\dmd2\windows\bin\..\..\src\phobos\std\path.d(1319): Error: 
function 'std.path.buildPath!(const(char)[][]).buildPath' is not 
nothrow
C:\D\dmd2\windows\bin\..\..\src\phobos\std\path.d(1315): Error: 
nothrow function 'std.path.buildPath!char.buildPath' may throw
package.d(8718): Error: template instance std.path.buildPath!char 
error instantiating

Error in a totally different place! If my code caused this, I 
have no idea how: I haven't touched unittests nor std.path.

Since I don't think you like if my pr (if I manage to even do it) 
contains fixes to other stuff than what I'm trying to do, I 
wonder where I should start patching this. Should I make a new 
branch off master and when done, merge it into the branch I have 
now? Or should I make an entirely new fork?
Nov 30 2016
parent reply Jonathan M Davis via Digitalmars-d-learn writes:
On Wednesday, November 30, 2016 17:14:37 Dukc via Digitalmars-d-learn wrote:

 Well, I was working on std.range.chain (I'm new to contributing),
 and when trying to test locally:

 ...\phobos\std\range>rdmd -unittest -main package
 C:\D\dmd2\windows\bin\..\..\src\phobos\std\path.d(1319): Error:
 pure function 'std.path.buildPath!char.buildPath' cannot call
 impure function 'std.path.buildPath!(const(char)[][]).buildPath'
 C:\D\dmd2\windows\bin\..\..\src\phobos\std\path.d(1319): Error:
  safe function 'std.path.buildPath!char.buildPath' cannot call
  system function 'std.path.buildPath!(const(char)[][]).buildPath'
 C:\D\dmd2\windows\bin\..\..\src\phobos\std\path.d(1319): Error:
 function 'std.path.buildPath!(const(char)[][]).buildPath' is not
 nothrow
 C:\D\dmd2\windows\bin\..\..\src\phobos\std\path.d(1315): Error:
 nothrow function 'std.path.buildPath!char.buildPath' may throw
 package.d(8718): Error: template instance std.path.buildPath!char
 error instantiating

 Error in a totally different place! If my code caused this, I
 have no idea how: I haven't touched unittests nor std.path.

 Since I don't think you like if my pr (if I manage to even do it)
 contains fixes to other stuff than what I'm trying to do, I
 wonder where I should start patching this. Should I make a new
 branch off master and when done, merge it into the branch I have
 now? Or should I make an entirely new fork?


Well, normally, you wouldn't be using master to do pull requests at all. You
create a branch for a particular fix, push that up to github, and then you
create a PR from that branch. You can then have multiple, independent
branches with fixes at once, and your master branch can then always match
whatever the official master branch is. I don't think that anyone who
contributes more than once or twice is creating pull requests from their
master branch. And personally, I think that it's far too valuable to have
your master branch match the official master branch to ever do anything on
my own master branch.

Whether separate PRs or one are better for what you're doing, I can't say,
because I don't know what you're doing. But if you have an independent fix -
particularly if the main thing that you're doing is an enhancement rather
than a fix - then I'd say that you should create a branch for that and
submit it. You can then create another branch from that one to work on
whatever it is you're doing that needed that fix. Then, if you finish that
before the first is merged, you'd either need to wait for the first one to
be merged, or you'd need to remove the first fix's commits from the second
branch (easy enough with git rebase) and create a PR from that with a
comment that it depended on the other PR. If you wait to create the second
PR (either because the first gets merged before the second is ready or
because you just choose to wait), then you can rebase the second branch so
that its changes go on top of the updated master (which should work without
removing the first branch's commits from the second branch before rebasing
if no other changes were made to the first branch before it was merged, but
I'd probably just remove those commits from the second branch to ensure that
there were no merge issues), and then you can create a PR from that.

But git makes branching very easy (even if the commands are more confusing
than they should be), so it makes a lot of sense to create separate branches
for each fix, regardless of what gets merged when.

In case you're not very familiar with git rebase, here's a relevant SO
question (where I got to feel like an idiot for mixing up the git commands):

http://stackoverflow.com/questions/7297379

- Jonathan M Davis
Nov 30 2016
parent reply Dukc <ajieskola gmail.com> writes:
On Wednesday, 30 November 2016 at 18:26:32 UTC, Jonathan M Davis 
wrote:

 [snip]

 - Jonathan M Davis


Luckily, I have made a branch for my stuff instead of using 
master. But thanks for the help, now I know that it does not 
matter where I create the other branch for that workaround 
because I can rebase it after I have used it to test my stuff.

I just hope I don't screw the whole thing up with git commands...
Nov 30 2016
next sibling parent Jonathan M Davis via Digitalmars-d-learn writes:
On Wednesday, November 30, 2016 18:50:42 Dukc via Digitalmars-d-learn wrote:

 On Wednesday, 30 November 2016 at 18:26:32 UTC, Jonathan M Davis

 wrote:

 [snip]

 - Jonathan M Davis


 Luckily, I have made a branch for my stuff instead of using
 master. But thanks for the help, now I know that it does not
 matter where I create the other branch for that workaround
 because I can rebase it after I have used it to test my stuff.

 I just hope I don't screw the whole thing up with git commands...


Worst case, you copy your changes elsewhere, blow away your branch, create a
new one, and then copy your changes back. So, I expect that you'll be fine.
git takes some getting used to, but after a bit of practice, it usually
isn't a problem unless you're trying to do something particularly
complicated or abnormal. It has the right model for how it works. It just
isn't as good with the CLI as it should be. But at least we get the
"porcelain" layer. If we were stuck with what Linus had done with the CLI
originally, git would never have gone anywhere.

- Jonathan M Davis
Nov 30 2016
prev sibling parent reply Johan Engelen <j j.nl> writes:
On Wednesday, 30 November 2016 at 18:50:42 UTC, Dukc wrote:

 On Wednesday, 30 November 2016 at 18:26:32 UTC, Jonathan M 
 Davis wrote:

 [snip]

 - Jonathan M Davis


 Luckily, I have made a branch for my stuff instead of using 
 master. But thanks for the help, now I know that it does not 
 matter where I create the other branch for that workaround 
 because I can rebase it after I have used it to test my stuff.


Tip: forget about the `master` branch in your own fork on GH. I 
never use it.
Instead, my advice is to locally checkout the master branch from 
the repo you want to contribute to (dlang/phobos in your case).


 I just hope I don't screw the whole thing up with git 
 commands...


Tip: use both a GUI and the commandline. Without SourceTree [1], 
I would be nowhere near as effective with git.

With rebasing, you'll end up having to force push. Force pushing 
is scary (because the repo state may have changed between you 
updating your local state and you pushing), so it's nicer to use 
--force-with-lease. [2]
Make an alias for that:
`git config --global alias.pushf "push --force-with-lease"`
then you can do "git pushf" to force push rebased branches 
"safely". I can't remember the last time I used `git push 
--force`.

-Johan

[1] https://www.sourcetreeapp.com/
[2] 
https://stackoverflow.com/questions/30542491/push-force-with-lease-by-default
Nov 30 2016
next sibling parent reply Jonathan M Davis via Digitalmars-d-learn writes:
On Wednesday, November 30, 2016 21:48:20 Johan Engelen via Digitalmars-d-
learn wrote:

 Tip: use both a GUI and the commandline. Without SourceTree [1],
 I would be nowhere near as effective with git.

 With rebasing, you'll end up having to force push. Force pushing
 is scary (because the repo state may have changed between you
 updating your local state and you pushing), so it's nicer to use
 --force-with-lease. [2]
 Make an alias for that:
 `git config --global alias.pushf "push --force-with-lease"`
 then you can do "git pushf" to force push rebased branches
 "safely". I can't remember the last time I used `git push
 --force`.


Really? I use git push -f all the time without problems. But I'm always
pushing to a branch that's for a PR on github. So, normally, no one would
have been doing anything with it but reviewing it or merging it, and after
it's merged, there isn't even a reason to keep the branch around. So,
forcing works great when dealing with PRs and github, but I wouldn't use it
for much else. Certainly, it isn't at all appropriate for anything that you
would normally expect folks to be branching from.

- Jonathan M Davis
Nov 30 2016
parent Dukc <ajieskola gmail.com> writes:
It is my code what caused it after all. How, I do not know yet. 
But a similar unittest compiled and passed when I tested with the 
master. Anyway, it seems I have no need to make other branches 
this time. Thanks for the advice trough. After all, I may need to 
rebase anyway when doing the pull request.
Nov 30 2016
prev sibling parent Dukc <ajieskola gmail.com> writes:
On Wednesday, 30 November 2016 at 21:48:20 UTC, Johan Engelen 
wrote:

 Tip: forget about the `master` branch in your own fork on GH. I 
 never use it.
 Instead, my advice is to locally checkout the master branch 
 from the repo you want to contribute to (dlang/phobos in your 
 case).


That works and is fine? Great, no forking should simplify things 
a bit!
Nov 30 2016