Re: [PATCH] format-patch: remove existing output-directory
On Fri, Jun 14, 2013 at 06:13:33PM +0530, Ramkumar Ramachandra wrote: The following command $ git format-patch -o outgoing master does not ensure that the output-directory outgoing doesn't already exist. As a result, it's possible for patches from two different series to get mixed up if the user is not careful. Fix the problem by unconditionally removing the output-directory before writing to it. I don't think this is the correct behaviour. I can think of cases where I would want to output multiple things into the same directory. It may be better to issue a warning when this happens, or die and provide a flag to let the user bypass that. -- 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] format-patch: remove existing output-directory
Hi, just some questions about your patch. On Fri, Jun 14, 2013 at 06:13:33PM +0530, Ramkumar Ramachandra wrote: The following command $ git format-patch -o outgoing master does not ensure that the output-directory outgoing doesn't already exist. As a result, it's possible for patches from two different series to get mixed up if the user is not careful. Fix the problem by unconditionally removing the output-directory before writing to it. I'm not entirely happy about removing untracked stuff without asking the user. What if the output directory isn't empty? What if a user just want them in ~? However I think this patch can improve the workflow for experienced developers. Can we tweak this in some way to get the best out of both worlds? + struct strbuf buf = STRBUF_INIT; + if (use_stdout) die(_(standard output, or directory, which one?)); + strbuf_addstr(buf, output_directory); + remove_dir_recursively(buf, 0); Should we have a strbuf_release here? -- Med vänliga hälsningar Fredrik Gustafsson tel: 0733-608274 e-post: iv...@iveqy.com -- 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] format-patch: remove existing output-directory
John Keeping wrote: I don't think this is the correct behaviour. I can think of cases where I would want to output multiple things into the same directory. format.cleanOutputDirectory = true|false? -- 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] format-patch: remove existing output-directory
On Fri, Jun 14, 2013 at 06:45:19PM +0530, Ramkumar Ramachandra wrote: John Keeping wrote: I don't think this is the correct behaviour. I can think of cases where I would want to output multiple things into the same directory. format.cleanOutputDirectory = true|false? Maybe, but I was thinking of something more like: Output directory is not empty, use --allow-non-empty-dir if you really want to proceed. Using that configuration variable lets someone shoot themselves in the foot quite badly if they forget that they have set it and set the output directory to somewhere containing important data. -- 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] format-patch: remove existing output-directory
Fredrik Gustafsson wrote: However I think this patch can improve the workflow for experienced developers. Can we tweak this in some way to get the best out of both worlds? The main problem is that output-directory can be an absolute path (like ~, in the extreme case). I'm not sure how to trade-off safety for speed. My main itch is that completion doesn't work with my fp: alias.fp = !rm -rf outgoing git format-patch -M -C -o outgoing + struct strbuf buf = STRBUF_INIT; + if (use_stdout) die(_(standard output, or directory, which one?)); + strbuf_addstr(buf, output_directory); + remove_dir_recursively(buf, 0); Should we have a strbuf_release here? Yeah, my stupidity. -- 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] format-patch: remove existing output-directory
On Fri, Jun 14, 2013 at 07:04:18PM +0530, Ramkumar Ramachandra wrote: Fredrik Gustafsson wrote: However I think this patch can improve the workflow for experienced developers. Can we tweak this in some way to get the best out of both worlds? The main problem is that output-directory can be an absolute path (like ~, in the extreme case). I'm not sure how to trade-off safety for speed. My main itch is that completion doesn't work with my fp: alias.fp = !rm -rf outgoing git format-patch -M -C -o outgoing If I let myself to drift off a bit... git format-patch always creates a new directory like: .git/outgoing/[patchname]FROM commit short sha1...TO commit short sha1 and possible runs a custom command afterwards. Like cd to the patch directory, open the cover-letter in your editor etc. git send-email without patches specified gives you a list of patch-series from .git/outgoing to choose from with the latest generated patchserie already selected. -- Med vänliga hälsningar Fredrik Gustafsson tel: 0733-608274 e-post: iv...@iveqy.com -- 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] format-patch: remove existing output-directory
On Fri, Jun 14, 2013 at 07:04:18PM +0530, Ramkumar Ramachandra wrote:
My main itch is that completion doesn't work with my fp:
alias.fp = !rm -rf outgoing git format-patch -M -C -o outgoing
Why not define your custom completion function for this alias in your
.bashrc?
_git_fp () { _git_format_patch ; }
Best,
Gábor
--
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] format-patch: remove existing output-directory
SZEDER Gábor wrote:
_git_fp () { _git_format_patch ; }
Good stopgap, thanks.
--
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] format-patch: remove existing output-directory
Fredrik Gustafsson wrote: git format-patch always creates a new directory like: .git/outgoing/[patchname]FROM commit short sha1...TO commit short sha1 and possible runs a custom command afterwards. Like cd to the patch directory, open the cover-letter in your editor etc. git send-email without patches specified gives you a list of patch-series from .git/outgoing to choose from with the latest generated patchserie already selected. Excellent. I like the idea. Let's see what the others have to say. -- 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] format-patch: remove existing output-directory
John Keeping j...@keeping.me.uk writes: On Fri, Jun 14, 2013 at 06:13:33PM +0530, Ramkumar Ramachandra wrote: The following command $ git format-patch -o outgoing master does not ensure that the output-directory outgoing doesn't already exist. As a result, it's possible for patches from two different series to get mixed up if the user is not careful. Fix the problem by unconditionally removing the output-directory before writing to it. I don't think this is the correct behaviour. I can think of cases where I would want to output multiple things into the same directory. It may be better to issue a warning when this happens, or die and provide a flag to let the user bypass that. Absolutely. I think this too dangeous, and will kill the possibility to further improve format-patch -v$n. An option (and configuration that expresses the user's wish) to error out when -o diretory exists is probably as far as we would want to go. Removal is way too much to accept without risk of hurting users. -- 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] format-patch: remove existing output-directory
John Keeping j...@keeping.me.uk writes: On Fri, Jun 14, 2013 at 06:45:19PM +0530, Ramkumar Ramachandra wrote: John Keeping wrote: I don't think this is the correct behaviour. I can think of cases where I would want to output multiple things into the same directory. format.cleanOutputDirectory = true|false? Maybe, but I was thinking of something more like: Output directory is not empty, use --allow-non-empty-dir if you really want to proceed. Using that configuration variable lets someone shoot themselves in the foot quite badly if they forget that they have set it and set the output directory to somewhere containing important data. Yes. format.refuseToOutputToNonEmpty that defaults to false and gives your error message is a sensible way to go. -- 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
