Stefan Saasen <ssaasen@xxxxxxxxxxxxx> wrote: >Thanks for the review David, much appreciated. > >> I think this line was already too long in its current form. Would >you mind >> splitting up this long line? > >I've updated the patch and had a look at how to avoid repeating the >list of >available merge/difftools. > >> ... follow it up with a change that generalizes the "list >> of tools" thing so that it can be reused here, possibly. The >> show_tool_help() function, as used by "git difftool --tool-help" and >> "git mergetool --tool-help", might be a good place to look for >> inspiration. > >> We were able to eliminate duplication in the docs (see the handling >> for $(mergetools_txt) in Documentation/Makefile) so it'd be nice if >we >> could do the same for git-completion.bash, somehow. > >I can think of a number of approaches and I would like to get some >feedback. > >Firstly I think a similar solution to how the duplication is avoided in >the >documentation can't be easily applied to the completion script. Looking >at the >script itself (and/or usage docs like >http://git-scm.com/book/en/Git-Basics-Tips-and-Tricks) the recommended >way of >using it is by copying the script as-is. That means there won't be a >build step >we could rely on unless I've overlooked something? > >That leaves a different approach (run- vs. build time) where I can >think of two >possible solutions. >The first would be similar to what is being done at the moment by >looking at >the MERGE_TOOLS_DIR and in addition considering any custom merge tools >configured. I'm working with the premise that it is a reasonable >assumption >that users of the git completion script have a git installation >available even >though they may have gotten the script by other means. >For users to still be able to install the script by simply copying it >to any location >on the filesystem the list generation function(s) would either have to >be sourced >from the git installation or duplicated. I suppose the former would >need to >take into account that the completion script doesn't necessarily >matches the >installed version of git with some potential brittleness around >relying on external >files and directories. The latter doesn't buy us anything as it >duplicates even >more code than the current list of available mergetools. > >The second approach would be to do something similar to resolving the >merge >strategies (in __git_list_merge_strategies) by parsing the output of >the `git >merge tool --tools-help` option with a very similar disadvantage that >it relies >on the textual output of the help command and doesn't work outside of a >git >repository. > > >I'm currently leaning towards the last approach as it seems less >reliant on >implementation details but it doesn't look ideal either and I may be >missing >another approach that would be better suited. I agree that this seems like the way to go. Perhaps we can add git mergetool/difftool --list-tools which can print the available tools so that the completion can use it. > >> It might be worth leaving the git-completion.bash bits alone in this >> first patch and follow it up with a change that generalizes the "list >> of tools" thing so that it can be reused here, possibly. > >To decouple this and adding the diffmerge merge tool option, I'd rather >keep the >git-completion change part of the patch. That way the patch is self >contained >and covers the change including the completion using the current >approach and >doesn't rely on the duplication change. Any concerns around that, >otherwise I'll >resend the patch with only the long line fixed? That sounds good, we can keep these as separate patches. Thanks, -- David -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html