Re: git format-patch can clobber existing patch

Re: git format-patch can clobber existing patch

[Σταύρος Ντέντος]

Hello there,

During my work flow, I tend to use `git format-patch` to move patches
around (nasty, I know, but bear with me)

It has occured to me that, it is possible that `git format-patch` can
clobber existing files, if they match in the name and in the "patch
order".
However, if the patch is one, then, it will normally start with `0001-x.patch`

It was fine for me when I pushed updates of the same "patch series".
Now that I wanted to diff a "previous patch file" with the patch
upstream, however, it was almost a "disaster". :-)

Would it make sense / be easy enough to have some clobbering check / flag?

Ντέντος Σταύρος

Re: git format-patch can clobber existing patch

[Junio C Hamano]

Σταύρος Ντέντος  <stdedos@gmail.com> writes:

> During my work flow, I tend to use `git format-patch` to move patches
> around (nasty, I know, but bear with me)
>
> It has occured to me that, it is possible that `git format-patch` can
> clobber existing files, if they match in the name and in the "patch
> order".
> However, if the patch is one, then, it will normally start with `0001-x.patch`
>
> It was fine for me when I pushed updates of the same "patch series".
> Now that I wanted to diff a "previous patch file" with the patch
> upstream, however, it was almost a "disaster". :-)
>
> Would it make sense / be easy enough to have some clobbering check / flag?

Given that use of '-o' to redirect to a fresh/new directory would
reduce the risk of such clobbering, and use of '-v' to force
different filenames would reduce the risk of such clobbering,
it seems to me that aborting the operation when we fail to open
the output, without any option to override and allow clobbering,
would make sense.  If existing files record 4 patch series
0001-x.patch, 0002-y.patch, 0003-z.patch, and 0004-w.patch, and you
generate with "format-patch --allow-clobbering" a three-patch series,
it would overwrite 0001 thru 0003 but will not remove 0004, so the
end result will still be confusing.

This is not even compile-tested, but something along these lines may
work.  I am reasonably sure that existing tests won't let this patch
alone pass, as some may depend on being able to overwrite output
files left behind by previous tests.

 builtin/log.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index 3e145fe502..cb7a9eb7f9 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -864,6 +864,16 @@ static int git_format_config(const char *var, const char *value, void *cb)
 static const char *output_directory = NULL;
 static int outdir_offset;
 
+static FILE *fopen_excl(const char *filename)
+{
+	int fd = open(filename, O_CREAT | O_EXCL | O_WRONLY, 0666);
+	if (fd < 0) {
+		error_errno("%s", filename);
+		return NULL;
+	}
+	return fdopen(fd, "w");
+}
+
 static int open_next_file(struct commit *commit, const char *subject,
 			 struct rev_info *rev, int quiet)
 {
@@ -890,7 +900,7 @@ static int open_next_file(struct commit *commit, const char *subject,
 	if (!quiet)
 		printf("%s\n", filename.buf + outdir_offset);
 
-	if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL) {
+	if ((rev->diffopt.file = fopen_excl(filename.buf)) == NULL) {
 		error_errno(_("Cannot open patch file %s"), filename.buf);
 		strbuf_release(&filename);
 		return -1;

Re: git format-patch can clobber existing patch

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1239 bytes --]

On Thu, Feb 21, 2019 at 03:40:09PM -0800, Junio C Hamano wrote:
> Σταύρος Ντέντος  <stdedos@gmail.com> writes:
> > Would it make sense / be easy enough to have some clobbering check / flag?
> 
> Given that use of '-o' to redirect to a fresh/new directory would
> reduce the risk of such clobbering, and use of '-v' to force
> different filenames would reduce the risk of such clobbering,
> it seems to me that aborting the operation when we fail to open
> the output, without any option to override and allow clobbering,
> would make sense.  If existing files record 4 patch series
> 0001-x.patch, 0002-y.patch, 0003-z.patch, and 0004-w.patch, and you
> generate with "format-patch --allow-clobbering" a three-patch series,
> it would overwrite 0001 thru 0003 but will not remove 0004, so the
> end result will still be confusing.

I think a flag for this would be useful. For people that store tarballs
(or pristine-tar files) and patches in their repository, overwriting the
existing files is definitely desired.

My personal preference is that the flag be --no-clobber, but I can see
arguments for the other side as well.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: git format-patch can clobber existing patch

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> On Thu, Feb 21, 2019 at 03:40:09PM -0800, Junio C Hamano wrote:
>> Σταύρος Ντέντος  <stdedos@gmail.com> writes:
>> > Would it make sense / be easy enough to have some clobbering check / flag?
>> 
>> Given that use of '-o' to redirect to a fresh/new directory would
>> reduce the risk of such clobbering, and use of '-v' to force
>> different filenames would reduce the risk of such clobbering,
>> it seems to me that aborting the operation when we fail to open
>> the output, without any option to override and allow clobbering,
>> would make sense.  If existing files record 4 patch series
>> 0001-x.patch, 0002-y.patch, 0003-z.patch, and 0004-w.patch, and you
>> generate with "format-patch --allow-clobbering" a three-patch series,
>> it would overwrite 0001 thru 0003 but will not remove 0004, so the
>> end result will still be confusing.
>
> I think a flag for this would be useful. For people that store tarballs
> (or pristine-tar files) and patches in their repository, overwriting the
> existing files is definitely desired.
>
> My personal preference is that the flag be --no-clobber, but I can see
> arguments for the other side as well.

That's actually simpler, which is good, as we do not have to worry
about adjusting the existing tests that rely on the clobbering
behaviour ;-).

I'll find time before I leave for my offline week, but this
obviously will not be merged before the upcoming release.

Thanks.  I think I've mostly outlined a three-patch series to do
this ready.

Re: git format-patch can clobber existing patch

[Σταύρος Ντέντος]

Ντέντος Σταύρος
On Fri, Feb 22, 2019 at 9:38 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
> > On Thu, Feb 21, 2019 at 03:40:09PM -0800, Junio C Hamano wrote:
> >> Σταύρος Ντέντος  <stdedos@gmail.com> writes:
> >> > Would it make sense / be easy enough to have some clobbering check / flag?
> >>
> >> Given that use of '-o' to redirect to a fresh/new directory would
> >> reduce the risk of such clobbering, and use of '-v' to force
> >> different filenames would reduce the risk of such clobbering,
> >> it seems to me that aborting the operation when we fail to open
> >> the output, without any option to override and allow clobbering,
> >> would make sense.  If existing files record 4 patch series
> >> 0001-x.patch, 0002-y.patch, 0003-z.patch, and 0004-w.patch, and you
> >> generate with "format-patch --allow-clobbering" a three-patch series,
> >> it would overwrite 0001 thru 0003 but will not remove 0004, so the
> >> end result will still be confusing.

There might be a handful of complexities / undefined behaviors coming
out of this; however, I think "not" overwriting a file to be a higher
directive (given that then, it is unrecoverable).
Using any of the '-o', '-v' might be helpful - if you are re-running
commands from history, however, it wouldn't necessarily provide any
protection ;-)

> > I think a flag for this would be useful. For people that store tarballs
> > (or pristine-tar files) and patches in their repository, overwriting the
> > existing files is definitely desired.
> >
> > My personal preference is that the flag be --no-clobber, but I can see
> > arguments for the other side as well.

On my head, I was thinging that preferably `git config
format-patch.no-clobber true` could be set, instead of (continuously)
carrying around a `--no-clobber` flag.
I should've probably be more clear about that. However, any way is "as
good as any".

> That's actually simpler, which is good, as we do not have to worry
> about adjusting the existing tests that rely on the clobbering
> behaviour ;-).
>
> I'll find time before I leave for my offline week, but this
> obviously will not be merged before the upcoming release.
>
> Thanks.  I think I've mostly outlined a three-patch series to do
> this ready.
>
>
>
>
>
>
>