Re: [PATCH] apply: mark some file-local symbols static
Hi Dscho, On Wed, Aug 3, 2016 at 4:37 PM, Johannes Schindelinwrote: > Hi Christian, > > On Wed, 3 Aug 2016, Christian Couder wrote: > >> Now there are different options to fix this: >> >> 1) remove the symbols in 9f87c22 ("apply: refactor `git apply` option >> parsing") at the end of the series, or >> 2) move 4820e13 (apply: make some parsing functions static again) at >> the end of the series and make it also remove them, or: >> 3) add another patch to remove them after 9f87c22 ("apply: refactor >> `git apply` option parsing") > > You forgot 4) provide fixup patches that fix the patch series. > > And 5) fix the patch series, push the branch to GitHub and provide a > pointer, but not sending a new iteration unless needed otherwise. Unfortunately there are other changes, especially those suggested by Peff, I have to make in the patch series, so I will resend everything. Thanks, Christian. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] apply: mark some file-local symbols static
Hi Christian, On Wed, 3 Aug 2016, Christian Couder wrote: > Now there are different options to fix this: > > 1) remove the symbols in 9f87c22 ("apply: refactor `git apply` option > parsing") at the end of the series, or > 2) move 4820e13 (apply: make some parsing functions static again) at > the end of the series and make it also remove them, or: > 3) add another patch to remove them after 9f87c22 ("apply: refactor > `git apply` option parsing") You forgot 4) provide fixup patches that fix the patch series. And 5) fix the patch series, push the branch to GitHub and provide a pointer, but not sending a new iteration unless needed otherwise. Ciao, Johannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] apply: mark some file-local symbols static
On 03/08/16 10:47, Christian Couder wrote: > Hi Ramsay, > > On Wed, Aug 3, 2016 at 12:44 AM, Junio C Hamanowrote: >> On Tue, Aug 2, 2016 at 3:33 PM, Ramsay Jones >> wrote: >>> >>> Signed-off-by: Ramsay Jones >>> --- >>> >>> Hi Christian, >>> [snip] >>> What am I missing? > > These symbols are still used in builtin/apply.c until 9f87c22 ("apply: > refactor `git apply` option parsing") at the end of the series, for > example: > > $ git checkout 4d18b33a > $ git grep -n apply_option_parse_directory builtin/apply.c > builtin/apply.c:86: 0, apply_option_parse_directory }, > Heh, thanks. I thought I had done exactly this, but I obviously messed up! [snip] > Yeah, I did not notice that they no longer need to be extern. > Now there are different options to fix this: > > 1) remove the symbols in 9f87c22 ("apply: refactor `git apply` option > parsing") at the end of the series, or > 2) move 4820e13 (apply: make some parsing functions static again) at > the end of the series and make it also remove them, or: > 3) add another patch to remove them after 9f87c22 ("apply: refactor > `git apply` option parsing") > > My preference is to do 1). This way, or if I do 3), I would not need > to resend the first 31 patches in the series. FWIW, I would go with option #1. ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] apply: mark some file-local symbols static
Hi Ramsay, On Wed, Aug 3, 2016 at 12:44 AM, Junio C Hamanowrote: > On Tue, Aug 2, 2016 at 3:33 PM, Ramsay Jones > wrote: >> >> Signed-off-by: Ramsay Jones >> --- >> >> Hi Christian, >> >> I had intended to ask you to squash this into your 'cc/apply-am' >> branch, specifically commit 4d18b33a (apply: move libified code >> from builtin/apply.c to apply.{c,h}, 30-07-2016). >> >> However, having read that commit a little closer, it seems that >> you deliberately made these symbols public. The commit message >> does not mention this issue at all, and it is not clear to me >> why these symbols should be public. >> >> What am I missing? These symbols are still used in builtin/apply.c until 9f87c22 ("apply: refactor `git apply` option parsing") at the end of the series, for example: $ git checkout 4d18b33a $ git grep -n apply_option_parse_directory builtin/apply.c builtin/apply.c:86: 0, apply_option_parse_directory }, > Their exports have been made obsolete by the reroll we have > in 'pu' when "builtin/am: use apply api in run_apply()" was > redone in a way not to duplicate the argument parsing. Yeah. > They should have been cleaned with 4820e13, 4820e13 (apply: make some parsing functions static again) is too early in the series for cleaning this. At that point the symbols are still used in builtin/apply.c. > but I think > Christian did not carefully review the whole series before > sending it out and did not notice that they no longer need > to be extern. Yeah, I did not notice that they no longer need to be extern. Now there are different options to fix this: 1) remove the symbols in 9f87c22 ("apply: refactor `git apply` option parsing") at the end of the series, or 2) move 4820e13 (apply: make some parsing functions static again) at the end of the series and make it also remove them, or: 3) add another patch to remove them after 9f87c22 ("apply: refactor `git apply` option parsing") My preference is to do 1). This way, or if I do 3), I would not need to resend the first 31 patches in the series. Thanks, Christian. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] apply: mark some file-local symbols static
On Tue, Aug 2, 2016 at 3:33 PM, Ramsay Joneswrote: > > Signed-off-by: Ramsay Jones > --- > > Hi Christian, > > I had intended to ask you to squash this into your 'cc/apply-am' > branch, specifically commit 4d18b33a (apply: move libified code > from builtin/apply.c to apply.{c,h}, 30-07-2016). > > However, having read that commit a little closer, it seems that > you deliberately made these symbols public. The commit message > does not mention this issue at all, and it is not clear to me > why these symbols should be public. > > What am I missing? Their exports have been made obsolete by the reroll we have in 'pu' when "builtin/am: use apply api in run_apply()" was redone in a way not to duplicate the argument parsing. They should have been cleaned with 4820e13, but I think Christian did not carefully review the whole series before sending it out and did not notice that they no longer need to be extern. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html