[rust-dev] idiomatic conversion from Optionstr to *libc::c_char
When calling a native function that takes a char* string or NULL for
the default behavior I treat the option like this
pub fn read_file(self, path: Optionstr) {
let c_path = match path {
Some(ref path) = path.to_c_str().with_ref(|x| x),
None = ptr::null()
};
unsafe {
native_read_file(self.ch, c_path) as int
};
}
Is this the best way to handle the situation?
`path.to_cstr().with_ref(|x| x)` seems a bit verbose. On the other
hand, `unwrap` says it forgets the ownership which I'm assuming means
that the buffer won't be freed.
-Noah
___
Rust-dev mailing list
Rust-dev@mozilla.org
https://mail.mozilla.org/listinfo/rust-dev
Re: [rust-dev] Need help implementing some complex parent-child task behavior.
So I managed to implement a solution without needing a child task, using Kevin's initial idea of a struct with private fields and public getters. The idea had occurred to me but I wasn't sure exactly how to implement it; needless to say, I think I was overcomplicating things. Thanks for the help, Kevin. On Fri, Feb 14, 2014 at 4:18 PM, Damien Radtke damienrad...@gmail.comwrote: Ah, I think the problem is that I'm trying to create a new task within a loop over an iterator, so each value is an -ptr and is therefore causing it to fail... On Fri, Feb 14, 2014 at 4:12 PM, Damien Radtke damienrad...@gmail.comwrote: The function pointer is indeed a function pointer and all of the strings and vectors are ~, but the vector type is 'static. They're meant to hold references to card definitions, which is more efficient than passing around the cards themselves. I tried modifying the vectors to hold ~-strings instead, but it still didn't work. Looks like I'll need to do more research on Send. On Fri, Feb 14, 2014 at 3:19 PM, Kevin Ballard ke...@sb.org wrote: Depends. If the string or the vectors are instead of ~, that would do it. Also, if the element type of the vector does not fulfill Send. Oh, and the function pointer is a function pointer, not a closure, right? -Kevin On Feb 14, 2014, at 12:59 PM, Damien Radtke damienrad...@gmail.com wrote: Unfortunately, the type that maintains the state apparently doesn't fulfill Send, which confuses me because it's a struct that consists of a string, function pointer, and a few dynamically-sized vectors. Which of these types makes the struct as a whole violate Send? On Fri, Feb 14, 2014 at 2:47 PM, Kevin Ballard ke...@sb.org wrote: What if the state's fields are private, and in a different module than the players, but exposes getters to query the state? Then the players can't modify it, but if the component that processes the actions has visibility into the state's fields, it can modify them just fine. -Kevin On Feb 14, 2014, at 12:22 PM, Damien Radtke damienrad...@gmail.com wrote: I'm trying to write what is essentially a card game simulator in Rust, but I'm running into a bit of a roadblock with Rust's memory management. The gist of what I want to accomplish is: 1. In the program's main loop, iterate over several players and call their play method in turn. 2. Each play method should be able to send requests back to the parent in order to take certain actions, who will validate that the action is possible and update the player's state accordingly. The problem I'm running into is that, in order to let a player play and have the game validate actions for them, I would need to run each player in their own task, (I considered implementing it as each function call indicating a request for action [e.g. by returning Some(action), or None when finished] and calling it repeatedly until none are taken, but this makes the implementation for each player needlessly complex) but this makes for some tricky situations. My current implementation uses a DuplexStream to communicate back and forth, the child sending requests to the parent and the parent sending responses, but then I run into the issue of how to inform the child of their current state, but don't let them modify it outside of sending action requests. Ideally I'd like to be able to create an (unsafe) immutable pointer to the state held by the parent as mutable, but that gives me a values differ in mutability error. Other approaches so far have failed as well; Arcs don't work because I need to have one-sided mutability; standard borrowed pointers don't work because the child and parent need to access it at the same time (though only the parent should be able to modify it, ensuring its safety); even copying the state doesn't work because the child then needs to update its local state with a new copy sent by the parent, which is also prone to mutability-related errors. Any tips on how to accomplish something like this? ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev
Re: [rust-dev] idiomatic conversion from Optionstr to *libc::c_char
No, this is likely to crash. `.to_c_str()` constructs a CString, which you are
then promptly throwing away. So your subsequent access to `c_path` is actually
accessing freed memory.
Try something like this:
pub fn read_file(self, path: Optionstr) {
let path = path.map(|s| s.to_c_str());
let c_path = path.map_or(ptr::null(), |p| p.with_ref(|x| x));
unsafe {
native_read_file(self.ch, c_path) as int
}
}
This variant will keep the CString alive inside the `path` variable, which will
mean that your `c_path` pointer is still valid until you return from the
function.
-Kevin
On Feb 17, 2014, at 1:06 PM, Noah Watkins jayh...@cs.ucsc.edu wrote:
When calling a native function that takes a char* string or NULL for
the default behavior I treat the option like this
pub fn read_file(self, path: Optionstr) {
let c_path = match path {
Some(ref path) = path.to_c_str().with_ref(|x| x),
None = ptr::null()
};
unsafe {
native_read_file(self.ch, c_path) as int
};
}
Is this the best way to handle the situation?
`path.to_cstr().with_ref(|x| x)` seems a bit verbose. On the other
hand, `unwrap` says it forgets the ownership which I'm assuming means
that the buffer won't be freed.
smime.p7s
Description: S/MIME cryptographic signature
___
Rust-dev mailing list
Rust-dev@mozilla.org
https://mail.mozilla.org/listinfo/rust-dev
[rust-dev] issue numbers in commit messages
How would people feel about a requirement for all commit messages to have an issue number in them? And could we make bors enforce that? The reason is that GitHub is very bad at being able to trace back a commit to the issue it fixes (sometimes it manages, but not always). Not being able to find the discussion around a commit is extremely annoying. Cheers, Nick ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev
Re: [rust-dev] issue numbers in commit messages
Why not make bors simply add the issue number in when it makes the actual merge commit? ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev
Re: [rust-dev] issue numbers in commit messages
I believe that bors never does a fast forward merge and that the merge commits always contain the pull number. So, if you have a particular commit and you want to find the issue that it was part of, I believe you can always look look through its children until you find a commit by bors which should have a commit message like: auto merge of #12313 : bjz/rust/tuple, r=huonw which contains the issue number. Let says that the commit you are interested in is 6f39eb1. I think if you run the command: git log --author bors --ancestry-path 6f39eb1..origin/master And look at the commit at the very bottom of the list, that will be the merge commit that you are interested in. I'm not a git expert - there may be a better way to do that. -Palmer Cox On Mon, Feb 17, 2014 at 4:50 PM, Nick Cameron n...@ncameron.org wrote: How would people feel about a requirement for all commit messages to have an issue number in them? And could we make bors enforce that? The reason is that GitHub is very bad at being able to trace back a commit to the issue it fixes (sometimes it manages, but not always). Not being able to find the discussion around a commit is extremely annoying. Cheers, Nick ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev
Re: [rust-dev] issue numbers in commit messages
It is generally a good practice to embed the information of the tracking tool inside the git commit message so finding this information is straight forward, and you can jump from any commit to the GitHub issue also instaneously. We use Gerrit with a tracked-on field on the last paragraph of the commit message (which is supported by gerrit) in order to link this commit to our tracking tool. Github seems to support only #123 substring in the commit message. Beware because it will close automatically the issue if the string Fix #123 is found in the first line, and a single commit can refer to more than only one github issue. My 2 cents, - Gaetan 2014-02-17 23:43 GMT+01:00 Palmer Cox palmer...@gmail.com: I believe that bors never does a fast forward merge and that the merge commits always contain the pull number. So, if you have a particular commit and you want to find the issue that it was part of, I believe you can always look look through its children until you find a commit by bors which should have a commit message like: auto merge of #12313 : bjz/rust/tuple, r=huonw which contains the issue number. Let says that the commit you are interested in is 6f39eb1. I think if you run the command: git log --author bors --ancestry-path 6f39eb1..origin/master And look at the commit at the very bottom of the list, that will be the merge commit that you are interested in. I'm not a git expert - there may be a better way to do that. -Palmer Cox On Mon, Feb 17, 2014 at 4:50 PM, Nick Cameron n...@ncameron.org wrote: How would people feel about a requirement for all commit messages to have an issue number in them? And could we make bors enforce that? The reason is that GitHub is very bad at being able to trace back a commit to the issue it fixes (sometimes it manages, but not always). Not being able to find the discussion around a commit is extremely annoying. Cheers, Nick ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev
Re: [rust-dev] issue numbers in commit messages
Maybe I'm misunderstanding? This would require that all commits be specifically associated with an issue. I don't have actual stats, but briefly skimming recent commits and looking at the issue tracker, a lot of commits can't be reasonably associated with an issue. This requirement would either force people to create fake issues for each commit, or to reference tangentially-related or overly-broad issues in commit messages, neither of which is very useful. Referencing any conversation that leads to or influences a commit is a good idea, but something this inflexible doesn't seem right. My 1.5¢. On Tue, 18 Feb 2014, Nick Cameron wrote: How would people feel about a requirement for all commit messages to have an issue number in them? And could we make bors enforce that? The reason is that GitHub is very bad at being able to trace back a commit to the issue it fixes (sometimes it manages, but not always). Not being able to find the discussion around a commit is extremely annoying. Cheers, Nick -- Scott Lawrence___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev
Re: [rust-dev] issue numbers in commit messages
I really just want to avoid any kind of Git-fu. I would like to see the commit on GitHub and click a link. Or see a commit in git log, and copy paste something to my browser. Anything more complex than that is distracting. (thanks for the solution to my specific problem though - that is helpful!) On Tue, Feb 18, 2014 at 11:43 AM, Palmer Cox palmer...@gmail.com wrote: I believe that bors never does a fast forward merge and that the merge commits always contain the pull number. So, if you have a particular commit and you want to find the issue that it was part of, I believe you can always look look through its children until you find a commit by bors which should have a commit message like: auto merge of #12313 : bjz/rust/tuple, r=huonw which contains the issue number. Let says that the commit you are interested in is 6f39eb1. I think if you run the command: git log --author bors --ancestry-path 6f39eb1..origin/master And look at the commit at the very bottom of the list, that will be the merge commit that you are interested in. I'm not a git expert - there may be a better way to do that. -Palmer Cox On Mon, Feb 17, 2014 at 4:50 PM, Nick Cameron n...@ncameron.org wrote: How would people feel about a requirement for all commit messages to have an issue number in them? And could we make bors enforce that? The reason is that GitHub is very bad at being able to trace back a commit to the issue it fixes (sometimes it manages, but not always). Not being able to find the discussion around a commit is extremely annoying. Cheers, Nick ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev
Re: [rust-dev] issue numbers in commit messages
At worst you could just use the issue number for the PR. But I think all non-trivial commits _should_ have an issue associated. For really tiny commits we could allow no issue or '#0' in the message. Just so long as the author is being explicit, I think that is OK. On Tue, Feb 18, 2014 at 12:16 PM, Scott Lawrence byt...@gmail.com wrote: Maybe I'm misunderstanding? This would require that all commits be specifically associated with an issue. I don't have actual stats, but briefly skimming recent commits and looking at the issue tracker, a lot of commits can't be reasonably associated with an issue. This requirement would either force people to create fake issues for each commit, or to reference tangentially-related or overly-broad issues in commit messages, neither of which is very useful. Referencing any conversation that leads to or influences a commit is a good idea, but something this inflexible doesn't seem right. My 1.5¢. On Tue, 18 Feb 2014, Nick Cameron wrote: How would people feel about a requirement for all commit messages to have an issue number in them? And could we make bors enforce that? The reason is that GitHub is very bad at being able to trace back a commit to the issue it fixes (sometimes it manages, but not always). Not being able to find the discussion around a commit is extremely annoying. Cheers, Nick -- Scott Lawrence ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev
Re: [rust-dev] issue numbers in commit messages
This is not going to work in the slightest. Most PRs don't have an associated issue. The pull request is the issue. And that's perfectly fine. There's no need to file an issue separate from the PR itself. Requiring a referenced issue for every single commit would be extremely cumbersome, serve no real purpose aside from aiding an unwillingness to learn how source control works, and would probably slow down the rate of development of Rust. -Kevin On Feb 17, 2014, at 3:50 PM, Nick Cameron li...@ncameron.org wrote: At worst you could just use the issue number for the PR. But I think all non-trivial commits _should_ have an issue associated. For really tiny commits we could allow no issue or '#0' in the message. Just so long as the author is being explicit, I think that is OK. On Tue, Feb 18, 2014 at 12:16 PM, Scott Lawrence byt...@gmail.com wrote: Maybe I'm misunderstanding? This would require that all commits be specifically associated with an issue. I don't have actual stats, but briefly skimming recent commits and looking at the issue tracker, a lot of commits can't be reasonably associated with an issue. This requirement would either force people to create fake issues for each commit, or to reference tangentially-related or overly-broad issues in commit messages, neither of which is very useful. Referencing any conversation that leads to or influences a commit is a good idea, but something this inflexible doesn't seem right. My 1.5¢. On Tue, 18 Feb 2014, Nick Cameron wrote: How would people feel about a requirement for all commit messages to have an issue number in them? And could we make bors enforce that? The reason is that GitHub is very bad at being able to trace back a commit to the issue it fixes (sometimes it manages, but not always). Not being able to find the discussion around a commit is extremely annoying. Cheers, Nick -- Scott Lawrence ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev smime.p7s Description: S/MIME cryptographic signature ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev
Re: [rust-dev] issue numbers in commit messages
This is a nice solution, I like it. People who know about bors - is it possible to make it work? On Tue, Feb 18, 2014 at 11:39 AM, Steve Klabnik st...@steveklabnik.comwrote: Why not make bors simply add the issue number in when it makes the actual merge commit? ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev
Re: [rust-dev] issue numbers in commit messages
Bors already mentions the pull request that he merged, and any commits that close or work on issues usually mention that explicitly in their commit message. What more do you want? On Mon, Feb 17, 2014 at 6:40 PM, Nick Cameron n...@ncameron.org wrote: This is a nice solution, I like it. People who know about bors - is it possible to make it work? On Tue, Feb 18, 2014 at 11:39 AM, Steve Klabnik st...@steveklabnik.com wrote: Why not make bors simply add the issue number in when it makes the actual merge commit? ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev
Re: [rust-dev] issue numbers in commit messages
For _all_ commits to mention the issue explicitly, not just usually On Tue, Feb 18, 2014 at 1:06 PM, Corey Richardson co...@octayn.net wrote: Bors already mentions the pull request that he merged, and any commits that close or work on issues usually mention that explicitly in their commit message. What more do you want? On Mon, Feb 17, 2014 at 6:40 PM, Nick Cameron n...@ncameron.org wrote: This is a nice solution, I like it. People who know about bors - is it possible to make it work? On Tue, Feb 18, 2014 at 11:39 AM, Steve Klabnik st...@steveklabnik.com wrote: Why not make bors simply add the issue number in when it makes the actual merge commit? ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev
Re: [rust-dev] issue numbers in commit messages
Whether we need issues for PRs is a separate discussion. There has to be _something_ for every commit - either a PR or an issue, at the least there needs to be an r+ somewhere. I would like to see who reviewed something so I can ping someone with questions other than the author (if they are offline). Any discussion is likely to be useful. So the question is how to find that, when necessary. GitHub sometimes fails to point to the info. And when it does, you do not know if you are missing more info. For the price of 6 characters in the commit message (or no issue), we know with certainty where to find that info and that we are not missing other potentially useful info. This would not slow down development in any way. Note that this is orthogonal to use of version control - you still need to know Git in order to get the commit message - it is about how one can go easily from a commit message to meta-data about a commit. On Tue, Feb 18, 2014 at 12:53 PM, Kevin Ballard ke...@sb.org wrote: This is not going to work in the slightest. Most PRs don't have an associated issue. The pull request *is* the issue. And that's perfectly fine. There's no need to file an issue separate from the PR itself. Requiring a referenced issue for every single commit would be extremely cumbersome, serve no real purpose aside from aiding an unwillingness to learn how source control works, and would probably slow down the rate of development of Rust. -Kevin On Feb 17, 2014, at 3:50 PM, Nick Cameron li...@ncameron.org wrote: At worst you could just use the issue number for the PR. But I think all non-trivial commits _should_ have an issue associated. For really tiny commits we could allow no issue or '#0' in the message. Just so long as the author is being explicit, I think that is OK. On Tue, Feb 18, 2014 at 12:16 PM, Scott Lawrence byt...@gmail.com wrote: Maybe I'm misunderstanding? This would require that all commits be specifically associated with an issue. I don't have actual stats, but briefly skimming recent commits and looking at the issue tracker, a lot of commits can't be reasonably associated with an issue. This requirement would either force people to create fake issues for each commit, or to reference tangentially-related or overly-broad issues in commit messages, neither of which is very useful. Referencing any conversation that leads to or influences a commit is a good idea, but something this inflexible doesn't seem right. My 1.5¢. On Tue, 18 Feb 2014, Nick Cameron wrote: How would people feel about a requirement for all commit messages to have an issue number in them? And could we make bors enforce that? The reason is that GitHub is very bad at being able to trace back a commit to the issue it fixes (sometimes it manages, but not always). Not being able to find the discussion around a commit is extremely annoying. Cheers, Nick -- Scott Lawrence ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev
Re: [rust-dev] issue numbers in commit messages
Adding a few chars to a commit is not onerous and it is useful. You may not want it now, but perhaps you would if you had it to use. _I_ certainly want it, and I think others would find it useful if it was there to use. On Tue, Feb 18, 2014 at 1:37 PM, Steve Klabnik st...@steveklabnik.comwrote: Yeah, I'm not into modifying every single commit, I basically only want what bors already (apparently) already does. ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev
Re: [rust-dev] issue numbers in commit messages
What about having bors place the hash of each commit merged into the auto-merge message? Then finding the PR, and any closed issues, consists of backwards-searching in git-log. (Having bors modify commit messages would probably cause major problems with hashes changing.) On Tue, 18 Feb 2014, Nick Cameron wrote: Adding a few chars to a commit is not onerous and it is useful. You may not want it now, but perhaps you would if you had it to use. _I_ certainly want it, and I think others would find it useful if it was there to use. On Tue, Feb 18, 2014 at 1:37 PM, Steve Klabnik st...@steveklabnik.comwrote: Yeah, I'm not into modifying every single commit, I basically only want what bors already (apparently) already does. -- Scott Lawrence ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev
Re: [rust-dev] issue numbers in commit messages
Right, that is exactly what I want to see, just on every commit. For example, https://github.com/mozilla/rust/commit/a02b10a0621adfe36eb3cc2e46f45fc7ccdb7ea2. has none of that info and I can't see any way to get it (without the kind of Git-fu suggested earlier). (Well, I can actually see that r=nikomatsakis from the comments at the bottom, but I can't see how that r+ came about, whether there was any discussion, whether there was an issue where this was discussed or not, etc.). On Tue, Feb 18, 2014 at 3:02 PM, Corey Richardson co...@octayn.net wrote: https://github.com/mozilla/rust/commit/25147b2644ed569f16f22dc02d10a0a9b7b97c7e seems to provide all of the information you are asking for? It includes the text of the PR description, the PR number, the name of the branch, and who reviewed it. I agree with your premise but I'm not sure I agree that the current situation isn't adequate. But I wouldn't be opposed to such a change. On Mon, Feb 17, 2014 at 8:54 PM, Nick Cameron li...@ncameron.org wrote: Whether we need issues for PRs is a separate discussion. There has to be _something_ for every commit - either a PR or an issue, at the least there needs to be an r+ somewhere. I would like to see who reviewed something so I can ping someone with questions other than the author (if they are offline). Any discussion is likely to be useful. So the question is how to find that, when necessary. GitHub sometimes fails to point to the info. And when it does, you do not know if you are missing more info. For the price of 6 characters in the commit message (or no issue), we know with certainty where to find that info and that we are not missing other potentially useful info. This would not slow down development in any way. Note that this is orthogonal to use of version control - you still need to know Git in order to get the commit message - it is about how one can go easily from a commit message to meta-data about a commit. On Tue, Feb 18, 2014 at 12:53 PM, Kevin Ballard ke...@sb.org wrote: This is not going to work in the slightest. Most PRs don't have an associated issue. The pull request is the issue. And that's perfectly fine. There's no need to file an issue separate from the PR itself. Requiring a referenced issue for every single commit would be extremely cumbersome, serve no real purpose aside from aiding an unwillingness to learn how source control works, and would probably slow down the rate of development of Rust. -Kevin On Feb 17, 2014, at 3:50 PM, Nick Cameron li...@ncameron.org wrote: At worst you could just use the issue number for the PR. But I think all non-trivial commits _should_ have an issue associated. For really tiny commits we could allow no issue or '#0' in the message. Just so long as the author is being explicit, I think that is OK. On Tue, Feb 18, 2014 at 12:16 PM, Scott Lawrence byt...@gmail.com wrote: Maybe I'm misunderstanding? This would require that all commits be specifically associated with an issue. I don't have actual stats, but briefly skimming recent commits and looking at the issue tracker, a lot of commits can't be reasonably associated with an issue. This requirement would either force people to create fake issues for each commit, or to reference tangentially-related or overly-broad issues in commit messages, neither of which is very useful. Referencing any conversation that leads to or influences a commit is a good idea, but something this inflexible doesn't seem right. My 1.5¢. On Tue, 18 Feb 2014, Nick Cameron wrote: How would people feel about a requirement for all commit messages to have an issue number in them? And could we make bors enforce that? The reason is that GitHub is very bad at being able to trace back a commit to the issue it fixes (sometimes it manages, but not always). Not being able to find the discussion around a commit is extremely annoying. Cheers, Nick -- Scott Lawrence ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev
Re: [rust-dev] issue numbers in commit messages
The PR# and who reviewed it is already available in the merge commit and its already possible to take any arbitrary commit and to see which merge commit merged it into master. So, I don't see any benefit in changing anything about the merge commit. Unless I'm missing something, this isn't a question of information not being available; its a question of that information being inconvenient to get to. I think having bors rewrite the commit messages would be somewhat problematic since it would change all the hashes. So, I think the only solution would be to manually put the issue number into the messages. However, many PRs aren't related to issues. So, if some large percentages of commits are just annotated with no issue or the like, it seems to really impact the utility of this change. Thus, I think it would really have to be the PR# instead of an issue # since every commit is related to a PR. However, I think it isn't a zero impact procedure. I always right the changes I want to merge before opening the PR. So, when I'm making my changes, I don't know what the eventual PR# is going to be. Only after I open the PR with the commits already created, I find out the PR#. So, then I'd have to rewrite all of the commit messages and force push back into the branch to get the numbers right. Its not the worst thing in the world, but it is an extra few steps. So, I strongly agree that the current procedure for finding the github discussion is fairly unpleasant and I very much wish that Github had a button that would take me to the PR that merged it. However, I don't think there is a 100% consistent, zero impact workaround for that missing feature in Github. My vote would be to leave things as they are. A little scripting could improve the situation quite a bit, although it still won't be as nice as being able to click on a link in Github. -Palmer Cox On Mon, Feb 17, 2014 at 9:02 PM, Scott Lawrence byt...@gmail.com wrote: What about having bors place the hash of each commit merged into the auto-merge message? Then finding the PR, and any closed issues, consists of backwards-searching in git-log. (Having bors modify commit messages would probably cause major problems with hashes changing.) On Tue, 18 Feb 2014, Nick Cameron wrote: Adding a few chars to a commit is not onerous and it is useful. You may not want it now, but perhaps you would if you had it to use. _I_ certainly want it, and I think others would find it useful if it was there to use. On Tue, Feb 18, 2014 at 1:37 PM, Steve Klabnik st...@steveklabnik.com wrote: Yeah, I'm not into modifying every single commit, I basically only want what bors already (apparently) already does. -- Scott Lawrence ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev
Re: [rust-dev] issue numbers in commit messages
You are right, it is about convenient access to the info, not the lack of info. What is problematic about bors rewriting commit messages and changing hashes? My workflow is to always work on throw away branches and merge back into master. Is it common to work on master and merge back on top of your PR? Or are there other problems with changing the hash? On Tue, Feb 18, 2014 at 3:22 PM, Palmer Cox palmer...@gmail.com wrote: The PR# and who reviewed it is already available in the merge commit and its already possible to take any arbitrary commit and to see which merge commit merged it into master. So, I don't see any benefit in changing anything about the merge commit. Unless I'm missing something, this isn't a question of information not being available; its a question of that information being inconvenient to get to. I think having bors rewrite the commit messages would be somewhat problematic since it would change all the hashes. So, I think the only solution would be to manually put the issue number into the messages. However, many PRs aren't related to issues. So, if some large percentages of commits are just annotated with no issue or the like, it seems to really impact the utility of this change. Thus, I think it would really have to be the PR# instead of an issue # since every commit is related to a PR. However, I think it isn't a zero impact procedure. I always right the changes I want to merge before opening the PR. So, when I'm making my changes, I don't know what the eventual PR# is going to be. Only after I open the PR with the commits already created, I find out the PR#. So, then I'd have to rewrite all of the commit messages and force push back into the branch to get the numbers right. Its not the worst thing in the world, but it is an extra few steps. So, I strongly agree that the current procedure for finding the github discussion is fairly unpleasant and I very much wish that Github had a button that would take me to the PR that merged it. However, I don't think there is a 100% consistent, zero impact workaround for that missing feature in Github. My vote would be to leave things as they are. A little scripting could improve the situation quite a bit, although it still won't be as nice as being able to click on a link in Github. -Palmer Cox On Mon, Feb 17, 2014 at 9:02 PM, Scott Lawrence byt...@gmail.com wrote: What about having bors place the hash of each commit merged into the auto-merge message? Then finding the PR, and any closed issues, consists of backwards-searching in git-log. (Having bors modify commit messages would probably cause major problems with hashes changing.) On Tue, 18 Feb 2014, Nick Cameron wrote: Adding a few chars to a commit is not onerous and it is useful. You may not want it now, but perhaps you would if you had it to use. _I_ certainly want it, and I think others would find it useful if it was there to use. On Tue, Feb 18, 2014 at 1:37 PM, Steve Klabnik st...@steveklabnik.com wrote: Yeah, I'm not into modifying every single commit, I basically only want what bors already (apparently) already does. -- Scott Lawrence ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev
[rust-dev] [RFC] Proposal for associated/'static' method syntax
I'm looking at addressing #. Here is a possible solution ( https://github.com/mozilla/rust/issues/12358) - a variation on past proposals. I'd appreciate any comments. Thanks, Nick ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev
Re: [rust-dev] issue numbers in commit messages
If bors rewrites the commit messages, it means that if someone approves commit ABC, what actually gets merged will be commit XYZ. This seems potentially confusing to me and might also make it more difficult to start with a reviewed commit on Github, such as https://github.com/gentlefolk/rust/commit/37bf97a0f9cc764a19dfcff21d62384b2445dcbc, and then track back to the actually merged commit in the history. I'm also not 100% sure, but I think git might have some issues with it as well. If I do my work on a throwaway branch, after merging, will git know that the changes in that branch were merged? Or, will git require me to do a git branch -D to delete that branch? Are there other projects that rewrite commit messages before merging? It seems to me that the ideal case would be for Github to add a link on the commit view page back to the PR that merged that commit. I'd be concerned that if Github adds support for such a feature in the future that it might not work if we've re-written all of the commit messages in the meantime. -Palmer Cox On Mon, Feb 17, 2014 at 9:28 PM, Nick Cameron li...@ncameron.org wrote: You are right, it is about convenient access to the info, not the lack of info. What is problematic about bors rewriting commit messages and changing hashes? My workflow is to always work on throw away branches and merge back into master. Is it common to work on master and merge back on top of your PR? Or are there other problems with changing the hash? On Tue, Feb 18, 2014 at 3:22 PM, Palmer Cox palmer...@gmail.com wrote: The PR# and who reviewed it is already available in the merge commit and its already possible to take any arbitrary commit and to see which merge commit merged it into master. So, I don't see any benefit in changing anything about the merge commit. Unless I'm missing something, this isn't a question of information not being available; its a question of that information being inconvenient to get to. I think having bors rewrite the commit messages would be somewhat problematic since it would change all the hashes. So, I think the only solution would be to manually put the issue number into the messages. However, many PRs aren't related to issues. So, if some large percentages of commits are just annotated with no issue or the like, it seems to really impact the utility of this change. Thus, I think it would really have to be the PR# instead of an issue # since every commit is related to a PR. However, I think it isn't a zero impact procedure. I always right the changes I want to merge before opening the PR. So, when I'm making my changes, I don't know what the eventual PR# is going to be. Only after I open the PR with the commits already created, I find out the PR#. So, then I'd have to rewrite all of the commit messages and force push back into the branch to get the numbers right. Its not the worst thing in the world, but it is an extra few steps. So, I strongly agree that the current procedure for finding the github discussion is fairly unpleasant and I very much wish that Github had a button that would take me to the PR that merged it. However, I don't think there is a 100% consistent, zero impact workaround for that missing feature in Github. My vote would be to leave things as they are. A little scripting could improve the situation quite a bit, although it still won't be as nice as being able to click on a link in Github. -Palmer Cox On Mon, Feb 17, 2014 at 9:02 PM, Scott Lawrence byt...@gmail.com wrote: What about having bors place the hash of each commit merged into the auto-merge message? Then finding the PR, and any closed issues, consists of backwards-searching in git-log. (Having bors modify commit messages would probably cause major problems with hashes changing.) On Tue, 18 Feb 2014, Nick Cameron wrote: Adding a few chars to a commit is not onerous and it is useful. You may not want it now, but perhaps you would if you had it to use. _I_ certainly want it, and I think others would find it useful if it was there to use. On Tue, Feb 18, 2014 at 1:37 PM, Steve Klabnik st...@steveklabnik.com wrote: Yeah, I'm not into modifying every single commit, I basically only want what bors already (apparently) already does. -- Scott Lawrence ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev
Re: [rust-dev] issue numbers in commit messages
Use a better graphical Git client ? (instead of Github itself) I personally just do my reviews through Gitk from my Git Bash install on Windows. Screenshot: Gitk.PNGhttps://docs.google.com/file/d/0B533WzlrxWraX1U5eDJ0S245ZVE/edit?usp=drive_web On Linux, you might be better suited with other graphical Git clients, just for your own browsing of auto-merges, etc. Here's a dated article (2012) that covers some of them: http://www.maketecheasier.com/6-useful-graphical-git-client-for-linux/ On Mon, Feb 17, 2014 at 9:34 PM, Palmer Cox palmer...@gmail.com wrote: If bors rewrites the commit messages, it means that if someone approves commit ABC, what actually gets merged will be commit XYZ. This seems potentially confusing to me and might also make it more difficult to start with a reviewed commit on Github, such as https://github.com/gentlefolk/rust/commit/37bf97a0f9cc764a19dfcff21d62384b2445dcbc, and then track back to the actually merged commit in the history. I'm also not 100% sure, but I think git might have some issues with it as well. If I do my work on a throwaway branch, after merging, will git know that the changes in that branch were merged? Or, will git require me to do a git branch -D to delete that branch? Are there other projects that rewrite commit messages before merging? It seems to me that the ideal case would be for Github to add a link on the commit view page back to the PR that merged that commit. I'd be concerned that if Github adds support for such a feature in the future that it might not work if we've re-written all of the commit messages in the meantime. -Palmer Cox On Mon, Feb 17, 2014 at 9:28 PM, Nick Cameron li...@ncameron.org wrote: You are right, it is about convenient access to the info, not the lack of info. What is problematic about bors rewriting commit messages and changing hashes? My workflow is to always work on throw away branches and merge back into master. Is it common to work on master and merge back on top of your PR? Or are there other problems with changing the hash? On Tue, Feb 18, 2014 at 3:22 PM, Palmer Cox palmer...@gmail.com wrote: The PR# and who reviewed it is already available in the merge commit and its already possible to take any arbitrary commit and to see which merge commit merged it into master. So, I don't see any benefit in changing anything about the merge commit. Unless I'm missing something, this isn't a question of information not being available; its a question of that information being inconvenient to get to. I think having bors rewrite the commit messages would be somewhat problematic since it would change all the hashes. So, I think the only solution would be to manually put the issue number into the messages. However, many PRs aren't related to issues. So, if some large percentages of commits are just annotated with no issue or the like, it seems to really impact the utility of this change. Thus, I think it would really have to be the PR# instead of an issue # since every commit is related to a PR. However, I think it isn't a zero impact procedure. I always right the changes I want to merge before opening the PR. So, when I'm making my changes, I don't know what the eventual PR# is going to be. Only after I open the PR with the commits already created, I find out the PR#. So, then I'd have to rewrite all of the commit messages and force push back into the branch to get the numbers right. Its not the worst thing in the world, but it is an extra few steps. So, I strongly agree that the current procedure for finding the github discussion is fairly unpleasant and I very much wish that Github had a button that would take me to the PR that merged it. However, I don't think there is a 100% consistent, zero impact workaround for that missing feature in Github. My vote would be to leave things as they are. A little scripting could improve the situation quite a bit, although it still won't be as nice as being able to click on a link in Github. -Palmer Cox On Mon, Feb 17, 2014 at 9:02 PM, Scott Lawrence byt...@gmail.comwrote: What about having bors place the hash of each commit merged into the auto-merge message? Then finding the PR, and any closed issues, consists of backwards-searching in git-log. (Having bors modify commit messages would probably cause major problems with hashes changing.) On Tue, 18 Feb 2014, Nick Cameron wrote: Adding a few chars to a commit is not onerous and it is useful. You may not want it now, but perhaps you would if you had it to use. _I_ certainly want it, and I think others would find it useful if it was there to use. On Tue, Feb 18, 2014 at 1:37 PM, Steve Klabnik st...@steveklabnik.com wrote: Yeah, I'm not into modifying every single commit, I basically only want what bors already (apparently) already does. -- Scott Lawrence ___ Rust-dev mailing list
Re: [rust-dev] issue numbers in commit messages
You are right, it is about convenient access to the info, not the lack of info. I often wish I could conveniently find this information--far too often it's hard to identify the PR when a breaking feature came in. Often I end up waiting for Corey to publish TWiR, with all the relevant issue numbers in it... but this makes it hard for me to include references in commit messages in my own repositories. More often than not, it's an issue in the clarity and distinctness of the pull requests rather than the code, but often finding the changeset is easy and the PR hard. What is problematic about bors rewriting commit messages and changing hashes? My workflow is to always work on throw away branches and merge back into master. Is it common to work on master and merge back on top of your PR? Or are there other problems with changing the hash? It is not uncommon to base one's work upon another branch (one's own or another's), depending on that landing before the aforementioned work can land, or alternatively landing the depended-upon feature as part of said work. What is at present approximately automatic there would become a manual rebasing-- certainly not insurmountable, but an added inconvenience. ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev
Re: [rust-dev] issue numbers in commit messages
On 17/02/2014 23:50, Nick Cameron wrote: At worst you could just use the issue number for the PR. In order to get a PR number you need to have commits to submit, with already-composed messages. But I think all non-trivial commits _should_ have an issue associated. GitHub PRs *are* issues. Requiring *another* issue when someone has code to submit already is not useful IMO. -- Simon Sapin ___ Rust-dev mailing list Rust-dev@mozilla.org https://mail.mozilla.org/listinfo/rust-dev
