Re: [PATCH] git-archive: accepts --owner --group aslike GNU tar.

[Junio C Hamano <gitster@pobox.com>]

suzuki toshiya <mpsuzuki@hiroshima-u.ac.jp> writes:

> Current tar output by git-archive has always root:root.

Thanks for a patch.  On top of Ævar's comments...

> ...
> * t/t5005-archive-uid-gid.sh: a test script comparing
>   uid, gid, uname, gname between the options and
>   generated tar file.
> ---

Before the "---" line, we need to get the patch signed off by the
author (see Documentation/SubmittingPatches).

> diff --git a/archive-tar.c b/archive-tar.c
> index c6ed96ee7..8546a6229 100644
> --- a/archive-tar.c
> +++ b/archive-tar.c
> @@ -204,10 +204,10 @@ static void prepare_header(struct archiver_args *args,
>  	xsnprintf(header->size, sizeof(header->size), "%011lo", S_ISREG(mode) ? size : 0);
>  	xsnprintf(header->mtime, sizeof(header->mtime), "%011lo", (unsigned long) args->time);
>  
> -	xsnprintf(header->uid, sizeof(header->uid), "%07o", 0);
> -	xsnprintf(header->gid, sizeof(header->gid), "%07o", 0);
> -	strlcpy(header->uname, "root", sizeof(header->uname));
> -	strlcpy(header->gname, "root", sizeof(header->gname));
> +	xsnprintf(header->uid, sizeof(header->uid), "%07o", args->uid);
> +	xsnprintf(header->gid, sizeof(header->gid), "%07o", args->gid);
> +	strlcpy(header->uname, args->uname ? args->uname : "root", sizeof(header->uname));
> +	strlcpy(header->gname, args->gname ? args->gname : "root", sizeof(header->gname));

Would it be cleaner to make sure aregs->[gu]name is always set
(i.e. stuff "root" when it is not given)?

>  	xsnprintf(header->devmajor, sizeof(header->devmajor), "%07o", 0);
>  	xsnprintf(header->devminor, sizeof(header->devminor), "%07o", 0);
>  
> diff --git a/archive.c b/archive.c
> index 0b7b62af0..db69041f1 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -8,6 +8,7 @@
>  #include "parse-options.h"
>  #include "unpack-trees.h"
>  #include "dir.h"
> +#include "git-compat-util.h"

The coding guideline says that "git-compat-util.h" (or one of the
well-known header that includes it) should be the first file to be
included, and we already include "cache.h" as the first thing, so
I do not think you want this addition here.

> @@ -417,6 +418,57 @@ static void parse_treeish_arg(const char **argv,
>  	{ OPTION_SET_INT, (s), NULL, (v), NULL, "", \
>  	  PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_HIDDEN, NULL, (p) }
>  
> +static void set_args_uname_uid(struct archiver_args *args,
> +		const char* tar_owner, int set_gid_too)

The asterisk sticks to the variable name, not the type name, i.e.

	const char *tar_owner

> +{
> +	if (!args || !tar_owner)
> +		return;
> +
> +	const char* col_pos = strchr(tar_owner, ':');
> +	struct passwd* pw = NULL;

Decl after statement.

> +	if (col_pos) {
> +		args->uname = xstrndup(tar_owner, col_pos - tar_owner);
> +		args->uid = atoi(col_pos + 1);
> +		return;
> +	}
> +
> +	args->uname = xstrndup(tar_owner, strlen(tar_owner));
> +	pw = getpwnam(tar_owner);
> +	if (!pw)
> +		return;

This means that upon error, the caller gets a half-filled args
structure and without any indication.

> diff --git a/t/parse-tar-file.py b/t/parse-tar-file.py

Hmph.  Do we still use Python around here?