git archive: tar.umask ignored

git archive: tar.umask ignored

[Jacek Masiulaniec]

Hello git@,

I believe git has an issue with command line override of configuration options.
Notice how in both cases below the file mode is 775 despite tar.umask
difference:

[jacekm@localhost git]$ ./git --version
git version 1.7.4.4
[jacekm@localhost git]$ ./git archive --remote=/repo/website.git HEAD
| tar -tvf - | head -1
drwxrwxr-x root/root         0 2011-04-11 13:47:09 bin/
[jacekm@localhost git]$ ./git -c tar.umask=0022 archive
--remote=/repo/website.git HEAD | tar -tvf - | head -1
drwxrwxr-x root/root         0 2011-04-11 13:47:09 bin/
[jacekm@localhost git]$

In contrast, tweaking tar.umask in ~/.gitconfig does have the desired
impact on the file mode.

Jacek

Re: git archive: tar.umask ignored

[René Scharfe]

Am 12.04.2011 11:39, schrieb Jacek Masiulaniec:
> Hello git@,
>
> I believe git has an issue with command line override of configuration options.
> Notice how in both cases below the file mode is 775 despite tar.umask
> difference:
>
> [jacekm@localhost git]$ ./git --version
> git version 1.7.4.4
> [jacekm@localhost git]$ ./git archive --remote=/repo/website.git HEAD
> | tar -tvf - | head -1
> drwxrwxr-x root/root         0 2011-04-11 13:47:09 bin/
> [jacekm@localhost git]$ ./git -c tar.umask=0022 archive
> --remote=/repo/website.git HEAD | tar -tvf - | head -1
> drwxrwxr-x root/root         0 2011-04-11 13:47:09 bin/
> [jacekm@localhost git]$
>
> In contrast, tweaking tar.umask in ~/.gitconfig does have the desired
> impact on the file mode.

The local setting of tar.umask does not affect the archive created at 
the remote end.  If your "remote" repository is in fact located on the 
same machine and accessed with the same user then of course settings in 
~/.gitconfig will take effect.

If --remote is specified then the local archive process just passes all 
command line options (except --remote itself, --output and --exec) to 
the other side which then does all the actual work.  The config option 
tar.umask is not even read in that case.

We'd need to read all relevant config settings (from the command line, 
and perhaps the user's config file and the system-wide one, but not the 
ones from any local repository) and then pass them over somehow.  We 
could piggy-back on the existing command line option passing, but for 
that we'd need to add new command line options.  OK, just a single one, 
because archive only recognizes tar.umask currently.

Would a new --umask command line option alone (that overrides any config 
setting) be sufficient for your use case?

Thanks,
René

Re: git archive: tar.umask ignored

[René Scharfe]

Am 12.04.2011 20:19, schrieb René Scharfe:
> Would a new --umask command line option alone (that overrides any config 
> setting) be sufficient for your use case?

Something like this (docs and tests missing)?

I'm not so sure about PARSE_OPT_OPTARG.  E.g. this is confusing and even
becomes slightly dangerous if you have a branch named 022 with a file
named HEAD (the = is required):

	$ git archive --umask 022 HEAD
	fatal: Not a valid object name

Also I'm and not sure what --no-umask should do.  In this patch it is
equivalent to --umask=0; perhaps it should rather make archive use the
configured tar.umask value, i.e. neutralize any previous --umask
option.
---
 archive-tar.c |    5 ++++-
 archive.c     |   34 +++++++++++++++++++++++++++++++++-
 archive.h     |    1 +
 3 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/archive-tar.c b/archive-tar.c
index cee06ce..1a29f3c 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -238,7 +238,10 @@ int write_tar_archive(struct archiver_args *args)
 {
 	int err = 0;
 
-	git_config(git_tar_config, NULL);
+	if (args->umask == -1)
+		git_config(git_tar_config, NULL);
+	else
+		tar_umask = args->umask;
 
 	if (args->commit_sha1)
 		err = write_global_extended_header(args);
diff --git a/archive.c b/archive.c
index 1944ed4..2ec5391 100644
--- a/archive.c
+++ b/archive.c
@@ -15,13 +15,14 @@ static char const * const archive_usage[] = {
 };
 
 #define USES_ZLIB_COMPRESSION 1
+#define USES_UMASK 2
 
 static const struct archiver {
 	const char *name;
 	write_archive_fn_t write_archive;
 	unsigned int flags;
 } archivers[] = {
-	{ "tar", write_tar_archive },
+	{ "tar", write_tar_archive, USES_UMASK },
 	{ "zip", write_zip_archive, USES_ZLIB_COMPRESSION },
 };
 
@@ -285,6 +286,30 @@ static void parse_treeish_arg(const char **argv,
 	ar_args->time = archive_time;
 }
 
+static int umask_callback(const struct option *opt, const char *arg, int unset)
+{
+	int *target = opt->value;
+	int value;
+	const char *endp;
+
+	if (unset) {
+		*target = 0;
+		return 0;
+	}
+	if (arg) {
+		value = strtol(arg, (char **)&endp, 0);
+		if (*endp || value < 0) {
+			return error("option `%s' %s", "umask",
+				     "expects a non-negative numerical value");
+		}
+	} else {
+		value = umask(0);
+		umask(value);
+	}
+	*target = value;
+	return 0;
+}
+
 #define OPT__COMPR(s, v, h, p) \
 	{ OPTION_SET_INT, (s), NULL, (v), NULL, (h), \
 	  PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, (p) }
@@ -305,6 +330,7 @@ static int parse_archive_args(int argc, const char **argv,
 	int i;
 	int list = 0;
 	int worktree_attributes = 0;
+	int umask = -1;
 	struct option opts[] = {
 		OPT_GROUP(""),
 		OPT_STRING(0, "format", &format, "fmt", "archive format"),
@@ -325,6 +351,9 @@ static int parse_archive_args(int argc, const char **argv,
 		OPT__COMPR_HIDDEN('7', &compression_level, 7),
 		OPT__COMPR_HIDDEN('8', &compression_level, 8),
 		OPT__COMPR('9', &compression_level, "compress better", 9),
+		{ OPTION_CALLBACK, 0, "umask", &umask, "umask",
+			"apply user's umask or <umask> to archived files",
+			PARSE_OPT_OPTARG, umask_callback },
 		OPT_GROUP(""),
 		OPT_BOOLEAN('l', "list", &list,
 			"list supported archive formats"),
@@ -370,10 +399,13 @@ static int parse_archive_args(int argc, const char **argv,
 					format, compression_level);
 		}
 	}
+	if (umask != -1 && !((*ar)->flags & USES_UMASK))
+		die("Argument not supported for format '%s': --umask", format);
 	args->verbose = verbose;
 	args->base = base;
 	args->baselen = strlen(base);
 	args->worktree_attributes = worktree_attributes;
+	args->umask = umask;
 
 	return argc;
 }
diff --git a/archive.h b/archive.h
index 038ac35..083675c 100644
--- a/archive.h
+++ b/archive.h
@@ -12,6 +12,7 @@ struct archiver_args {
 	unsigned int verbose : 1;
 	unsigned int worktree_attributes : 1;
 	int compression_level;
+	int umask;
 };
 
 typedef int (*write_archive_fn_t)(struct archiver_args *);
-- 
1.7.4

Re: git archive: tar.umask ignored

[Jacek Masiulaniec]

On 12 April 2011 19:19, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:
> The local setting of tar.umask does not affect the archive created at the
> remote end.  If your "remote" repository is in fact located on the same
> machine and accessed with the same user then of course settings in
> ~/.gitconfig will take effect.

I've eventually realised this by reading the source code.  The command
I actually needed was:

  cd /repo/website.git && git -c tar.umask=0022 archive HEAD

Can't comment on the proposed diff without getting better handle on
the whole system, but my knee-jerk reaction would be to resist the
urge to add more switches.  Maybe the existing -c switch could be made
to cooperate with --remote, or maybe the documentation could be edited
to make it clearer that -c and --remote are not related in the way I
had originally assumed.

Jacek

Re: git archive: tar.umask ignored

[René Scharfe]

Am 12.04.2011 22:39, schrieb Jacek Masiulaniec:
> On 12 April 2011 19:19, René Scharfe<rene.scharfe@lsrfire.ath.cx>  wrote:
>> The local setting of tar.umask does not affect the archive created at the
>> remote end.  If your "remote" repository is in fact located on the same
>> machine and accessed with the same user then of course settings in
>> ~/.gitconfig will take effect.
> 
> I've eventually realised this by reading the source code.  The command
> I actually needed was:
> 
>    cd /repo/website.git&&  git -c tar.umask=0022 archive HEAD

Yes, there's no point in using --remote if you can read the files directly.
I assumed you did that to make the command easier to reproduce by avoiding
to set up a git daemon etc..

> Can't comment on the proposed diff without getting better handle on
> the whole system, but my knee-jerk reaction would be to resist the
> urge to add more switches.  Maybe the existing -c switch could be made
> to cooperate with --remote, or maybe the documentation could be edited
> to make it clearer that -c and --remote are not related in the way I
> had originally assumed.

Yes, the documentation should be updated first.

-- >8 --
Subject: archive: document limitation of tar.umask config setting

The local value of the config variable tar.umask is not passed to the
other side with --remote.  We may want to change that, but for now just
document this fact.

Reported-by: Jacek Masiulaniec <jacek.masiulaniec@gmail.com>
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 Documentation/git-archive.txt |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-archive.txt b/Documentation/git-archive.txt
index f2b8684..9c750e2 100644
--- a/Documentation/git-archive.txt
+++ b/Documentation/git-archive.txt
@@ -98,7 +98,8 @@ tar.umask::
 	tar archive entries.  The default is 0002, which turns off the
 	world write bit.  The special value "user" indicates that the
 	archiving user's umask will be used instead.  See umask(2) for
-	details.
+	details.  If `--remote` is used then only the configuration of
+	the remote repository takes effect.
 
 ATTRIBUTES
 ----------

Re: git archive: tar.umask ignored

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Yes, the documentation should be updated first.

Thanks; applied.