> The patches don't apply to the devel branch. You've made patches on > top of the master branch, but the master branch of Bash is just a > release branch, where each commit corresponds to a release. You should > normally work based on the devel branch.
I see. I will rebase the patch on top of the devel branch. > This patch is independent of the present change for the source option. > This is unrelated. > > Also, you've factored out one function from `source_builtin(...)' and > moved another function and it to common.c. However, the new function > `execute_file_contents' is still called from only one place of the > original location. That's true. This change is from the original import builtin patch. The import builtin made use of it, but source does not need it. I kept it because the code was more organized this way, at least in my opinion. Perhaps other parts of the code would like to access the file evaluation functionality in the future. In that case, the needed refactoring would already be done. > By exposing this function in `common.h', > this effectively becomes a part of the public > interface for loadable builtins. I did not realize that. Definitely didn't mean to add some new API. I thought it was an internal file for defining shared functions. That's indeed a serious issue and a very good reason to move this function back into builtins/source.def. I can also simply drop this commit and rebase the rest of the patches if the refactoring is considered more trouble than its worth. Are all external functions defined in all files inside the builtins directory exposed as part of a public API/ABI? If that's the case, then it should also be made static as you suggested. Can you break down for me which parts of the code base are public and which are private? This will allow me to avoid making such mistakes in the future. I'd also like to suggest creating a private commons.c file where potentially reusable functionality can be moved to without exporting them. > There is a possibility that we want to later add other parameters > after seeing the actual uses. However, once it becomes a part of > the public interface, we wouldn't be able to modify it later easily. Absolutely agree. Once this becomes part of a public interface, it can't be changed without breaking everything. It's definitely something that will add to the maintenance efforts which is not what I intended to do. > At least the new function should still be defined in `builtins/source.def' > with the static keyword Agree. I will move it back there in the v2 patch set. > but even that way, the change is completely unrelated to the addition > of the new flag to the source builtin. That's true. I thought the refactoring added some value even though it's a remnant of a previous patch set. If it's more trouble than its worth, I can drop it. > To what you are signing off... By putting this line, you imply that > you agree on the rules that the project defines for copyright, etc., > but Bash doesn't specify it as far as I know. > Then, you've signed off what you haven't even checked the existence. I have read the "helping GNU" pages and know that the GNU/FSF projects require copyright reassignment to the FSF. I am willing to do that. I don't really know the process though. I'm also aware that the sign off is not sufficient in this case. I just figured adding that line couldn't possibly hurt. > Once the patches are accepted with a non-trivial amount of codes, > Chet will explain it to you. I see. Then I'll wait for instructions on that matter. Thanks for the feedback, Matheus
