Re: [PATCH/RFC/GSoC 16/17] editor: implement git_sequence_editor() and launch_sequence_editor()
Hi Paul, On Wed, 16 Mar 2016, Paul Tan wrote: > Hi Dscho, > > On Tue, Mar 15, 2016 at 3:00 PM, Johannes Schindelin >wrote: > > On Sat, 12 Mar 2016, Paul Tan wrote: > >> --- > >> cache.h | 1 + > > > > No need to clutter cache.h with a function that is only to be used by the > > sequencer. IOW let's make this static in sequencer.c. > > The function needs to be implemented in editor.c No, *another* function needs to be implemented in editor.c: one that accepts the editor itself as parameter. You did that, but then you wrapped it as git_sequencer_editor() and left the *really* useful function *still* static to editor.c. Or maybe the best solution would be to simply extend git_editor() to accept the editor as an additional, first parameter, falling back to the current behavior if NULL is passed (and then change all callers to pass NULL). I guess my preference would be with the latter, that would make for the most elegant, minimally invasive and most reusable solution. Ciao, Dscho -- 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/RFC/GSoC 16/17] editor: implement git_sequence_editor() and launch_sequence_editor()
Hi Dscho, On Tue, Mar 15, 2016 at 3:00 PM, Johannes Schindelinwrote: > On Sat, 12 Mar 2016, Paul Tan wrote: >> --- >> cache.h | 1 + > > No need to clutter cache.h with a function that is only to be used by the > sequencer. IOW let's make this static in sequencer.c. The function needs to be implemented in editor.c because it would be better for it to share the same code that launch_editor() uses (implemented in this patch by splitting the logic into a static function launch_specific_editor()) We could move the declaration to sequencer.h though. > I would also prefer pairing this short function with the change that > actually uses it (in my topic branches, I like to compile with -Werror, > which would result in a failure due to an unused function), in the same > patch. Sure. Regards, Paul -- 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/RFC/GSoC 16/17] editor: implement git_sequence_editor() and launch_sequence_editor()
Hi Paul, On Sat, 12 Mar 2016, Paul Tan wrote: > Signed-off-by: Paul TanThis commit message is very short. > --- > cache.h | 1 + No need to clutter cache.h with a function that is only to be used by the sequencer. IOW let's make this static in sequencer.c. I would also prefer pairing this short function with the change that actually uses it (in my topic branches, I like to compile with -Werror, which would result in a failure due to an unused function), in the same patch. Ciao, Dscho -- 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