[FIX] git-archive error, gzip -cn : command not found #2077

r1walz · 2019-02-15T21:54:47Z

git-archive uses gzip -cn to compress tar files but for this to work, gzip needs to be present on the host system, which sometimes is not the case.

In our implementation, we are trying to add minimal code that will mimic gzip compression, so as to get rid of this gzip dependency

Closes: #1970

dscho

This is a great first version of that PR. I offered a couple of suggestions how to improve it, hopefully you agree with all of them...

dscho · 2019-02-18T13:27:16Z

archive-tar.c

 	if (gzip) {
-		if (gzwrite(gzip, block, BLOCKSIZE) != BLOCKSIZE) error("gzwrite failed");
+		if (gzwrite(gzip, block, (unsigned) BLOCKSIZE) != BLOCKSIZE) error("gzwrite failed");
 	} else write_or_die(1, block, BLOCKSIZE);


Please squash these two fixups into the previous commit, which introduces the bugs fixed here.

dscho · 2019-02-18T13:33:04Z

archive-tar.c

@@ -454,26 +455,36 @@ static int write_tar_filter_archive(const struct archiver *ar,
 	if (args->compression_level >= 0)
 		strbuf_addf(&cmd, " -%d", args->compression_level);

+	snprintf(outmode, sizeof(outmode), "%s%d", "wb", args->compression_level);


snprintf() will no longer be allowed in Git for Windows' code base, as soon as v2.21.0 comes out. It is not even necessary here: you can write

char outmode[4] = "wb\0"; if (args->compression_level >= 0 && args->compression_level <= 9) outmode[2] = '0' + args->compression_level;

BTW please move the declaration of outmode to the if (!strcmp("gzip -cn", ar->data)) block, as that is the right scope for that variable.

dscho · 2019-02-18T13:35:39Z

archive-tar.c

+			die_errno(_("unable to redirect descriptor"));
+		close(filter.in);
+		if (finish_command(&filter) != 0)
+			die(_("'%s' filter reported error"), argv[0]);


I do not think that this is correct. At this point, the filter cannot have received any input, so if we think it should exit here, then it definitely won't have written the data we want.

Instead, we need to leave the finish_command() statement where it is (if indented one level because it will be in the conditional else block of the if (gzip)).

dscho · 2019-02-18T13:36:06Z

archive-tar.c

-		die(_("'%s' filter reported error"), argv[0]);
+	if (gzip) {
+		if (gzclose(gzip) != Z_OK) error("failed gzclose");
+	} else close(1);


The finish_command() still needs to be here, albeit in the else block now.

dscho · 2019-02-18T13:37:11Z

archive-tar.c


-	strbuf_release(&cmd);


Why? I do not quite understand why we no longer release cmd here. Could you explain? (Or in the alternative, revert this change?)

dscho · 2019-02-18T13:38:45Z

archive-tar.c

@@ -38,11 +37,18 @@ static int write_tar_filter_archive(const struct archiver *ar,
 #define USTAR_MAX_MTIME 077777777777ULL
 #endif

+/* writes out the whole block */
+static void write_block(const char * block) {


It might make sense to call it write_block_or_die() and use die() instead of error() in case gzwrite() fails. That would make the gzip code more consistent with the !gzip one.

dscho · 2019-02-18T13:39:59Z

archive-tar.c

+/* writes out the whole block */
+static void write_block(const char * block) {
+	if (gzip) {
+		if (gzwrite(gzip, block, BLOCKSIZE) != BLOCKSIZE) error("gzwrite failed");


In Git's source code, we never have conditional statements on the same line as the if keyword. Please always put the conditional statements into their own line, whether with or without curlies is up to you, but it needs to be an own line.

dscho · 2019-02-18T13:40:57Z

archive-tar.c

@@ -14,9 +14,8 @@



The second paragraph of the commit message talks about "our implementation", but it actually wants to talk about the next commit (because there is no "our implementation" before the next commit yet...).

Except for the line "replace write_or_die() calls with write_block_or_die()", this commit message would now fit better onto the second commit than onto this one...

A better commit message for this here commit might be:

archive: replace write_or_die() calls with write_block_or_die() In the next commit, we will change the gzip compression so that we no longer spawn `gzip` but let zlib perform the compression in the same process. In preparation for this, we consolidate all the block writes into a single function.

archive-tar.c

dscho · 2019-02-19T19:08:19Z

archive-tar.c

 static int tar_umask = 002;
-


Could you leave the whitespace of unrelated code untouched, please? 😃

archive-tar.c

dscho · 2019-02-19T19:13:14Z

archive-tar.c

@@ -14,9 +14,8 @@



A better commit message for this here commit might be:

archive: replace write_or_die() calls with write_block_or_die() In the next commit, we will change the gzip compression so that we no longer spawn `gzip` but let zlib perform the compression in the same process. In preparation for this, we consolidate all the block writes into a single function.

dscho · 2019-02-19T19:14:53Z

archive-tar.c

-		die_errno(_("unable to redirect descriptor"));
-	close(filter.in);
+	if (!strcmp("gzip -cn", ar->data)) {
+		char outmode[4] = "wb6\0";


Why not "wb\0\0"? The previous code did not default to compression level 6, after all, but let zlib decided...

Better "wb\0", actually, as that automatically NUL terminates, i.e. it is already 4 bytes long.

dscho · 2019-02-19T19:16:14Z

archive-tar.c

-		die(_("'%s' filter reported error"), argv[0]);
+	if (gzip) {
+		if (gzclose(gzip) != Z_OK)
+			die(_("failed gzclose"));


Or maybe "gzclose failed"?

dscho · 2019-02-19T19:20:10Z

This is now very close! Just a little cleanup of the commit messages (they seem to have been mixed up, the current first commit message would look better on the second commit, and I offered a suggestion how the first commit message might look like), and a few touchups of the patches, and we're done!

MinGit for Windows comes without `gzip` bundled inside, git-archive uses `gzip -cn` to compress tar files but for this to work, gzip needs to be present on the host system, which sometimes is not the case In the next commit, we will change the gzip compression so that we no longer spawn `gzip` but let zlib perform the compression in the same process In preparation for this, we consolidate all the block writes into a single function Closes: #1970 Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>

As we already link to `zlib` library, we can perform the compression without even requiring gzip on the host machine We modify write_tar_filter_archive() function in archive-tar.c to handle the compression when `gzip -cn` is requested Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>

[FIX] git-archive error, gzip -cn : command not found

`git archive` [no longer requires `gzip` to generate `.tgz` archives](git-for-windows/git#2077) (this means in particular that it works in MinGit). Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

We accepted a Pull Request into `master`, but the main development happens on the topic branch `rebase-to-v2.21.0` right now, in preparation for the final v2.21.0 in a couple of days. So after we forward-ported the changes in a6567c7 (Merge pull request git-for-windows#2077 from r1walz/gzip-cmd-404, 2019-02-20), it is now time to pull in the commit history of origin/master in a fake merge (i.e. with `-s ours`), to make `master` fast-forward to `rebase-to-v2.21.0` again. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

[FIX] git-archive error, gzip -cn : command not found

see: git-for-windows/git#2077

weekly-digest bot mentioned this pull request Feb 17, 2019

Weekly Digest (10 February, 2019 - 17 February, 2019) #2078

Closed

dscho requested changes Feb 18, 2019

View reviewed changes

dscho reviewed Feb 19, 2019

View reviewed changes

archive-tar.c Show resolved Hide resolved

dscho reviewed Feb 19, 2019

View reviewed changes

r1walz added 2 commits February 20, 2019 02:01

dscho approved these changes Feb 20, 2019

View reviewed changes

dscho merged commit ed2b22a into git-for-windows:master Feb 20, 2019

dscho added this to the v2.20.1(2) milestone Feb 20, 2019

dscho added a commit to dscho/git that referenced this pull request Feb 20, 2019

Merge pull request git-for-windows#2077 from r1walz/gzip-cmd-404

a6567c7

[FIX] git-archive error, gzip -cn : command not found

r1walz deleted the gzip-cmd-404 branch February 20, 2019 21:23

git-for-windows-ci pushed a commit that referenced this pull request Feb 22, 2019

Merge pull request #2077 from r1walz/gzip-cmd-404

949e07c

[FIX] git-archive error, gzip -cn : command not found

git-for-windows-ci pushed a commit that referenced this pull request Feb 22, 2019

Merge pull request #2077 from r1walz/gzip-cmd-404

0579ba6

[FIX] git-archive error, gzip -cn : command not found

git-for-windows-ci pushed a commit that referenced this pull request Feb 23, 2019

Merge pull request #2077 from r1walz/gzip-cmd-404

768e4c2

[FIX] git-archive error, gzip -cn : command not found

git-for-windows-ci pushed a commit that referenced this pull request Feb 23, 2019

Merge pull request #2077 from r1walz/gzip-cmd-404

c3f27e0

[FIX] git-archive error, gzip -cn : command not found

weekly-digest bot mentioned this pull request Feb 24, 2019

Weekly Digest (17 February, 2019 - 24 February, 2019) #2087

Closed

dscho mentioned this pull request Apr 26, 2019

Avoid spawning gzip in git archive gitgitgadget/git#145

Closed

r1walz added a commit to r1walz/git-patches that referenced this pull request Jul 2, 2019

add gzip bug patch from git-for-windows

301111d

see: git-for-windows/git#2077

r1walz added a commit to r1walz/git-patches that referenced this pull request Aug 20, 2019

add gzip bug patch from git-for-windows

57cf8de

see: git-for-windows/git#2077

r1walz added a commit to r1walz/git-patches that referenced this pull request Aug 20, 2019

add gzip bug patch from git-for-windows

4e52220

see: git-for-windows/git#2077

r1walz added a commit to r1walz/git-patches that referenced this pull request Aug 22, 2019

add gzip bug patch from git-for-windows

808c33c

see: git-for-windows/git#2077

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] git-archive error, gzip -cn : command not found #2077

[FIX] git-archive error, gzip -cn : command not found #2077

r1walz commented Feb 15, 2019

dscho left a comment

dscho Feb 18, 2019

dscho Feb 18, 2019

dscho Feb 18, 2019

dscho Feb 18, 2019

dscho Feb 18, 2019

dscho Feb 18, 2019

dscho Feb 18, 2019

dscho Feb 18, 2019

dscho Feb 19, 2019

dscho Feb 19, 2019

dscho Feb 19, 2019

dscho Feb 19, 2019

dscho Feb 19, 2019

dscho Feb 19, 2019

dscho Feb 19, 2019

dscho commented Feb 19, 2019

[FIX] git-archive error, gzip -cn : command not found #2077

[FIX] git-archive error, gzip -cn : command not found #2077

Conversation

r1walz commented Feb 15, 2019

dscho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dscho commented Feb 19, 2019