| Message ID | b1f8e288dde9a9dd46386524189da66c7ad2c333.1539990488.git.steadmon@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix format detection when archiving remotely | expand |
On Fri, Oct 19, 2018 at 04:19:28PM -0700, steadmon@google.com wrote: > diff --git a/builtin/archive.c b/builtin/archive.c > index e74f675390..dd3283a247 100644 > --- a/builtin/archive.c > +++ b/builtin/archive.c > @@ -45,7 +45,10 @@ static int run_remote_archiver(int argc, const char **argv, > * it. > */ > if (name_hint) { > - const char *format = archive_format_from_filename(name_hint); > + const char *format; > + init_tar_archiver(); > + init_zip_archiver(); > + format = archive_format_from_filename(name_hint); > if (format) > packet_write_fmt(fd[1], "argument --format=%s\n", format); Hrm. This code was added back in 56baa61d01 (archive: move file extension format-guessing lower, 2011-06-21), and your example invocation worked back then! Unfortunately it was broken by the very next patch in the series, 08716b3c11 (archive: refactor file extension format-guessing, 2011-06-21). I guess that's what I get for not adding regression tests. It's probably worth mentioning those points in the commit message. Does this work with configured archiver extensions, too? I think so, because we load them via init_tar_archiver(). Can we avoid repeating the list of archivers here? This needs to stay in sync with the list in write_archive(). I know there are only two, but can we factor out an init_archivers() call or something? We also should probably just call it unconditionally when we start the archiver command (I don't think there are any other bugs like this lurking, but it doesn't cost very much to initialize these; it makes sense to just do it early). Other than those minor points (and the lack of test), your fix looks good to me. -Peff
Jeff King <peff@peff.net> writes: > On Fri, Oct 19, 2018 at 04:19:28PM -0700, steadmon@google.com wrote: > >> diff --git a/builtin/archive.c b/builtin/archive.c >> index e74f675390..dd3283a247 100644 >> --- a/builtin/archive.c >> +++ b/builtin/archive.c >> @@ -45,7 +45,10 @@ static int run_remote_archiver(int argc, const char **argv, >> * it. >> */ >> if (name_hint) { >> - const char *format = archive_format_from_filename(name_hint); >> + const char *format; >> + init_tar_archiver(); >> + init_zip_archiver(); >> + format = archive_format_from_filename(name_hint); >> if (format) >> packet_write_fmt(fd[1], "argument --format=%s\n", format); > > Hrm. This code was added back in 56baa61d01 (archive: move file > extension format-guessing lower, 2011-06-21), and your example > invocation worked back then! > > Unfortunately it was broken by the very next patch in the series, > 08716b3c11 (archive: refactor file extension format-guessing, > 2011-06-21). I guess that's what I get for not adding regression tests. > > It's probably worth mentioning those points in the commit message. > > Does this work with configured archiver extensions, too? I think so, > because we load them via init_tar_archiver(). > > Can we avoid repeating the list of archivers here? This needs to stay in > sync with the list in write_archive(). I know there are only two, but > can we factor out an init_archivers() call or something? > > We also should probably just call it unconditionally when we start the > archiver command (I don't think there are any other bugs like this > lurking, but it doesn't cost very much to initialize these; it makes > sense to just do it early). > > Other than those minor points (and the lack of test), your fix looks > good to me. Thanks for a patch and an excellent review. Looking forward to the finalized version. Thanks, both.
On 2018.10.19 19:59, Jeff King wrote: > On Fri, Oct 19, 2018 at 04:19:28PM -0700, steadmon@google.com wrote: > > > diff --git a/builtin/archive.c b/builtin/archive.c > > index e74f675390..dd3283a247 100644 > > --- a/builtin/archive.c > > +++ b/builtin/archive.c > > @@ -45,7 +45,10 @@ static int run_remote_archiver(int argc, const char **argv, > > * it. > > */ > > if (name_hint) { > > - const char *format = archive_format_from_filename(name_hint); > > + const char *format; > > + init_tar_archiver(); > > + init_zip_archiver(); > > + format = archive_format_from_filename(name_hint); > > if (format) > > packet_write_fmt(fd[1], "argument --format=%s\n", format); > > Hrm. This code was added back in 56baa61d01 (archive: move file > extension format-guessing lower, 2011-06-21), and your example > invocation worked back then! > > Unfortunately it was broken by the very next patch in the series, > 08716b3c11 (archive: refactor file extension format-guessing, > 2011-06-21). I guess that's what I get for not adding regression tests. > > It's probably worth mentioning those points in the commit message. Done. > Does this work with configured archiver extensions, too? I think so, > because we load them via init_tar_archiver(). If you mean things like .tgz and .tar.gz, then yes, they are affected by the bug as well, and this patch fixes them. The test included in v2 uses a .tgz file. > Can we avoid repeating the list of archivers here? This needs to stay in > sync with the list in write_archive(). I know there are only two, but > can we factor out an init_archivers() call or something? Done. > We also should probably just call it unconditionally when we start the > archiver command (I don't think there are any other bugs like this > lurking, but it doesn't cost very much to initialize these; it makes > sense to just do it early). Done. > Other than those minor points (and the lack of test), your fix looks > good to me. Thanks for the review!
On Mon, Oct 22, 2018 at 02:47:56PM -0700, Josh Steadmon wrote: > > Does this work with configured archiver extensions, too? I think so, > > because we load them via init_tar_archiver(). > > If you mean things like .tgz and .tar.gz, then yes, they are affected by > the bug as well, and this patch fixes them. The test included in v2 uses > a .tgz file. Yes, but also ones that are provided by the user. E.g., does: git config tar.foo.command "foo" git archive -o out.tar.foo HEAD work? (I think the answer is yes, because we read git_tar_config in the same place). -Peff
diff --git a/builtin/archive.c b/builtin/archive.c index e74f675390..dd3283a247 100644 --- a/builtin/archive.c +++ b/builtin/archive.c @@ -45,7 +45,10 @@ static int run_remote_archiver(int argc, const char **argv, * it. */ if (name_hint) { - const char *format = archive_format_from_filename(name_hint); + const char *format; + init_tar_archiver(); + init_zip_archiver(); + format = archive_format_from_filename(name_hint); if (format) packet_write_fmt(fd[1], "argument --format=%s\n", format); }
When passing both --remote and --output to git-archive, initialize the archivers before attempting to determine the format from the output filename. Without initialization, the format cannot be determined. Signed-off-by: Josh Steadmon <steadmon@google.com> --- builtin/archive.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)