{"thread":{"id":"47503","subject":"[PATCH] git-archive: accepts --owner --group aslike GNU tar.","startedAt":"2017-12-28T09:37:04Z","lastAt":"2017-12-29T14:04:27Z","messageCount":4,"participants":["suzuki toshiya","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"335458","messageId":"20171228090527.25056-1-mpsuzuki@hiroshima-u.ac.jp","threadId":"47503","inReplyTo":null,"subject":"[PATCH] git-archive: accepts --owner --group aslike GNU tar.","fromName":"suzuki toshiya","fromEmail":"mpsuzuki@hiroshima-u.ac.jp","sentAt":"2017-12-28T09:05:27Z","receivedAt":"2017-12-28T09:37:04Z","isPatch":true,"sender":{"key":"mpsuzuki@hiroshima-u.ac.jp","avatar":null},"body":"Current tar output by git-archive has always root:root.\nTo generate tar output with non-root owner/group,\nthe options like GNU tar are added.\n\n* archive.h: add members 'uid', 'gid', 'uname', 'gname'\n  to struct archiver_args.\n\n* archive.c: add functions to reflect the operands of\n  '--owner' and '--group' to archiver_args.\n\n* archive-tar.c: copy 'uid', 'gid', 'uname', 'gname'\n  from archiver_args to the entry headers in tar archive.\n\n* t/parse-tar-file.py: a script to dump uid, gid, uname,\n  gname fields from a tar archive.\n\n* t/t5005-archive-uid-gid.sh: a test script comparing\n  uid, gid, uname, gname between the options and\n  generated tar file.\n---\n archive-tar.c              |  8 +++---\n archive.c                  | 66 ++++++++++++++++++++++++++++++++++++++++++\n archive.h                  |  4 +++\n t/parse-tar-file.py        | 56 ++++++++++++++++++++++++++++++++++++\n t/t5005-archive-uid-gid.sh | 71 ++++++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 201 insertions(+), 4 deletions(-)\n create mode 100755 t/parse-tar-file.py\n create mode 100755 t/t5005-archive-uid-gid.sh\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex c6ed96ee7..8546a6229 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -204,10 +204,10 @@ static void prepare_header(struct archiver_args *args,\n \txsnprintf(header->size, sizeof(header->size), \"%011lo\", S_ISREG(mode) ? size : 0);\n \txsnprintf(header->mtime, sizeof(header->mtime), \"%011lo\", (unsigned long) args->time);\n \n-\txsnprintf(header->uid, sizeof(header->uid), \"%07o\", 0);\n-\txsnprintf(header->gid, sizeof(header->gid), \"%07o\", 0);\n-\tstrlcpy(header->uname, \"root\", sizeof(header->uname));\n-\tstrlcpy(header->gname, \"root\", sizeof(header->gname));\n+\txsnprintf(header->uid, sizeof(header->uid), \"%07o\", args->uid);\n+\txsnprintf(header->gid, sizeof(header->gid), \"%07o\", args->gid);\n+\tstrlcpy(header->uname, args->uname ? args->uname : \"root\", sizeof(header->uname));\n+\tstrlcpy(header->gname, args->gname ? args->gname : \"root\", sizeof(header->gname));\n \txsnprintf(header->devmajor, sizeof(header->devmajor), \"%07o\", 0);\n \txsnprintf(header->devminor, sizeof(header->devminor), \"%07o\", 0);\n \ndiff --git a/archive.c b/archive.c\nindex 0b7b62af0..db69041f1 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -8,6 +8,7 @@\n #include \"parse-options.h\"\n #include \"unpack-trees.h\"\n #include \"dir.h\"\n+#include \"git-compat-util.h\"\n \n static char const * const archive_usage[] = {\n \tN_(\"git archive [<options>] <tree-ish> [<path>...]\"),\n@@ -417,6 +418,57 @@ static void parse_treeish_arg(const char **argv,\n \t{ OPTION_SET_INT, (s), NULL, (v), NULL, \"\", \\\n \t  PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_HIDDEN, NULL, (p) }\n \n+static void set_args_uname_uid(struct archiver_args *args,\n+\t\tconst char* tar_owner, int set_gid_too)\n+{\n+\tif (!args || !tar_owner)\n+\t\treturn;\n+\n+\tconst char* col_pos = strchr(tar_owner, ':');\n+\tstruct passwd* pw = NULL;\n+\n+\tif (col_pos) {\n+\t\targs->uname = xstrndup(tar_owner, col_pos - tar_owner);\n+\t\targs->uid = atoi(col_pos + 1);\n+\t\treturn;\n+\t}\n+\n+\targs->uname = xstrndup(tar_owner, strlen(tar_owner));\n+\tpw = getpwnam(tar_owner);\n+\tif (!pw)\n+\t\treturn;\n+\n+\targs->uid = pw->pw_uid;\n+\tif (set_gid_too)\n+\t\targs->gid = pw->pw_gid;\n+\n+\treturn;\n+}\n+\n+static void set_args_gname_gid(struct archiver_args *args,\n+\t\tconst char* tar_group)\n+{\n+\tif (!args || !tar_group)\n+\t\treturn;\n+\n+\tconst char* col_pos = strchr(tar_group, ':');\n+\tstruct group* gr = NULL;\n+\n+\tif (col_pos) {\n+\t\targs->gname = xstrndup(tar_group, col_pos - tar_group);\n+\t\targs->gid = atoi(col_pos + 1);\n+\t\treturn;\n+\t}\n+\n+\targs->gname = xstrndup(tar_group, strlen(tar_group));\n+\tgr = getgrnam(tar_group);\n+\tif (!gr)\n+\t\treturn;\n+\n+\targs->gid = gr->gr_gid;\n+\treturn;\n+}\n+\n static int parse_archive_args(int argc, const char **argv,\n \t\tconst struct archiver **ar, struct archiver_args *args,\n \t\tconst char *name_hint, int is_remote)\n@@ -431,6 +483,8 @@ static int parse_archive_args(int argc, const char **argv,\n \tint i;\n \tint list = 0;\n \tint worktree_attributes = 0;\n+\tchar *tar_owner = NULL;\n+\tchar *tar_group = NULL;\n \tstruct option opts[] = {\n \t\tOPT_GROUP(\"\"),\n \t\tOPT_STRING(0, \"format\", &format, N_(\"fmt\"), N_(\"archive format\")),\n@@ -459,6 +513,8 @@ static int parse_archive_args(int argc, const char **argv,\n \t\t\tN_(\"retrieve the archive from remote repository <repo>\")),\n \t\tOPT_STRING(0, \"exec\", &exec, N_(\"command\"),\n \t\t\tN_(\"path to the remote git-upload-archive command\")),\n+\t\tOPT_STRING(0, \"owner\", &tar_owner, N_(\"owner\"), N_(\"<name[:uid]> in tar\")),\n+\t\tOPT_STRING(0, \"group\", &tar_group, N_(\"group\"), N_(\"<name[:gid]> in tar\")),\n \t\tOPT_END()\n \t};\n \n@@ -507,6 +563,16 @@ static int parse_archive_args(int argc, const char **argv,\n \targs->baselen = strlen(base);\n \targs->worktree_attributes = worktree_attributes;\n \n+\targs->uname = NULL;\n+\targs->gname = NULL;\n+\targs->uid = 0;\n+\targs->gid = 0;\n+\tset_args_uname_uid(args,\n+\t\ttar_owner,\n+\t\t1 /* init args->gid by pw, if resolved */);\n+\tset_args_gname_gid(args,\n+\t\ttar_group);\n+\n \treturn argc;\n }\n \ndiff --git a/archive.h b/archive.h\nindex 62d1d82c1..fad4a5d3e 100644\n--- a/archive.h\n+++ b/archive.h\n@@ -15,6 +15,10 @@ struct archiver_args {\n \tunsigned int worktree_attributes : 1;\n \tunsigned int convert : 1;\n \tint compression_level;\n+\tint uid;\n+\tint gid;\n+\tconst char *uname;\n+\tconst char *gname;\n };\n \n #define ARCHIVER_WANT_COMPRESSION_LEVELS 1\ndiff --git a/t/parse-tar-file.py b/t/parse-tar-file.py\nnew file mode 100755\nindex 000000000..c240a8676\n--- /dev/null\n+++ b/t/parse-tar-file.py\n@@ -0,0 +1,56 @@\n+#!/usr/bin/env python\n+\n+import sys\n+import getopt\n+import tarfile\n+\n+optlist, args = getopt.getopt( sys.argv[1:], \"\", [\n+\t\t\t\t\"print=\", \"show=\",\n+\t\t\t\t\"uniq\",\n+\t\t\t\t\"fail-if-multi\",\n+\t\t] )\n+\n+infos_to_print = []\n+uniq = False\n+fail_if_multi = False\n+\n+for opt in optlist:\n+  if opt[0] == \"--print\":\n+    infos_to_print.append(opt[1])\n+  elif opt[0] == \"--show\":\n+    infos_to_print.append(opt[1])\n+  elif opt[0] == \"--uniq\":\n+    uniq = True\n+  elif opt[0] == \"--fail-if-multi\":\n+    uniq = True\n+    fail_if_multi = True\n+\n+if len(infos_to_print) == 0:\n+  infos_to_print = [\"uid\", \"gid\", \"uname\", \"gname\", \"name\"]\n+\n+tar = tarfile.open( args[0] )\n+out_lines = []\n+for tarinfo in tar:\n+  infos = []\n+  for info_tag in infos_to_print:\n+    if info_tag == \"uid\":\n+      infos.append( str(tarinfo.uid) )\n+    elif info_tag == \"gid\":\n+      infos.append( str(tarinfo.gid) )\n+    elif info_tag == \"uname\" or info_tag == \"owner\":\n+      infos.append( tarinfo.uname )\n+    elif info_tag == \"gname\" or info_tag == \"group\":\n+      infos.append( tarinfo.gname )\n+    elif info_tag == \"name\" or info_tag == \"pathname\":\n+      infos.append( tarinfo.name )\n+  out_lines.append( \"\\t\".join(infos) )\n+tar.close()\n+\n+if uniq:\n+  out_lines = list(set(out_lines))\n+  if fail_if_multi and (len(out_lines) > 1):\n+    sys.stderr.write(\"*** not unique value, \" + str(len(out_lines)) + \" values found\\n\")\n+    sys.exit(len(out_lines))\n+\n+for line in out_lines:\n+  print line\ndiff --git a/t/t5005-archive-uid-gid.sh b/t/t5005-archive-uid-gid.sh\nnew file mode 100755\nindex 000000000..2b2a694d8\n--- /dev/null\n+++ b/t/t5005-archive-uid-gid.sh\n@@ -0,0 +1,71 @@\n+#!/bin/sh\n+\n+test_description='test --owner --group options for git-archive'\n+. ./test-lib.sh\n+\n+check_uid_gid_uname_gname_in_tar() {\n+\t# $1 tar pathname\n+\t# $2 uid (digit in string)\n+\t# $3 gid (digit in string)\n+\t# $4 uname (string)\n+\t# $5 gname (string)\n+\tuid=`./parse-tar-file.py --print=uid --fail-if-multi $1`\n+\tif test $? != 0 -o x\"${uid}\" != \"x\"$2\n+\tthen\n+\t  echo \"(some) uid differs from the specified value\"\n+\t  return $?\n+        fi\n+\n+\tgid=`./parse-tar-file.py --print=gid --fail-if-multi $1`\n+\tif test $? != 0 -o x\"${gid}\" != \"x\"$3\n+\tthen\n+\t  echo \"(some) gid differs from the specified value\"\n+\t  return $?\n+\t  exit $?\n+        fi\n+\n+\tuname=`./parse-tar-file.py --print=uname --fail-if-multi $1`\n+\tif test $? != 0 -o x\"${uname}\" != \"x\"$4\n+\tthen\n+\t  echo \"(some) uname differs from the specified value\"\n+\t  return $?\n+        fi\n+\n+\tgname=`./parse-tar-file.py --print=gname --fail-if-multi $1`\n+\tif test $? != 0 -o x\"${gname}\" != \"x\"$5\n+\tthen\n+\t  echo \"(some) gname differs from the specified value\"\n+\t  return $?\n+        fi\n+\n+\treturn 0\n+}\n+\n+git init . 1>/dev/null 2>/dev/null\n+touch uid-gid-test.001\n+mkdir uid-gid-test.002\n+mkdir uid-gid-test.002/uid-gid-test.003\n+git add uid-gid-test.001\n+git add uid-gid-test.002\n+git add uid-gid-test.002/uid-gid-test.003\n+git commit -m \"uid-gid-test\" 2>/dev/null 1>/dev/null\n+\n+test_expect_success 'test a case with explicitly specified name/id, owner=nobody:1234 group=nogroup:5678' '\n+\tgit archive --format=tar --owner nobody:1234 --group nogroup:5678 HEAD > uid-gid-test1.tar &&\n+\tcheck_uid_gid_uname_gname_in_tar uid-gid-test1.tar 1234 5678 nobody nogroup &&\n+\ttest_result=$? &&\n+\treturn ${test_result}\n+'\n+\n+test_expect_success 'test a case with only name is specified, owner=(current my name) group=(current my group)' '\n+\tmy_uid=`id -u` &&\n+\tmy_gid=`id -g` &&\n+\tmy_uname=`id -u -n` &&\n+\tmy_gname=`id -g -n` &&\n+\tgit archive --format=tar --owner ${my_uname} --group ${my_gname} HEAD > uid-gid-test2.tar &&\n+\tcheck_uid_gid_uname_gname_in_tar uid-gid-test2.tar ${my_uid} ${my_gid} ${my_uname} ${my_gname} &&\n+\ttest_result=$? &&\n+\treturn ${test_result}\n+'\n+\n+test_done\n-- \n2.11.0\n\n\n"},{"id":"335468","messageId":"87incqrk1u.fsf@evledraar.gmail.com","threadId":"47503","inReplyTo":"20171228090527.25056-1-mpsuzuki@hiroshima-u.ac.jp","subject":"Re: [PATCH] git-archive: accepts --owner --group aslike GNU tar.","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-28T14:42:05Z","receivedAt":"2017-12-28T14:42:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Dec 28 2017, suzuki toshiya jotted:\n\nThanks for working on this Just a quick review from skimming this,\nhaven't actually applied + run this:\n\n> git-archive: accepts --owner --group aslike GNU tar.\n\nMake this:\n\n    git-archive: accept --owner and --group like GNU tar\n\nI.e. imperative voice, s/aslike/like/, no full stop at the end. See\nDocumentation/SubmittingPatches.\n\n> Current tar output by git-archive has always root:root.\n> To generate tar output with non-root owner/group,\n> the options like GNU tar are added.\n\nGrammar/wording fixes (IMHO):\n\n    The ownership of files craeted by git-archive is always\n    root:root. Add --owner and --group options which work like the GNU\n    tar equivalent to allow overriding these defaults.\n\n> * archive.h: add members 'uid', 'gid', 'uname', 'gname'\n>   to struct archiver_args.\n>\n> * archive.c: add functions to reflect the operands of\n>   '--owner' and '--group' to archiver_args.\n>\n> * archive-tar.c: copy 'uid', 'gid', 'uname', 'gname'\n>   from archiver_args to the entry headers in tar archive.\n>\n> * t/parse-tar-file.py: a script to dump uid, gid, uname,\n>   gname fields from a tar archive.\n>\n> * t/t5005-archive-uid-gid.sh: a test script comparing\n>   uid, gid, uname, gname between the options and\n>   generated tar file.\n\nI'd skip this whole bit, no need to itemize changes per-file, we see\nthat from the patch, especially stuff like \"add xyz struct field\".\n\n> ---\n>  archive-tar.c              |  8 +++---\n>  archive.c                  | 66 ++++++++++++++++++++++++++++++++++++++++++\n>  archive.h                  |  4 +++\n>  t/parse-tar-file.py        | 56 ++++++++++++++++++++++++++++++++++++\n>  t/t5005-archive-uid-gid.sh | 71 ++++++++++++++++++++++++++++++++++++++++++++++\n>  5 files changed, 201 insertions(+), 4 deletions(-)\n>  create mode 100755 t/parse-tar-file.py\n>  create mode 100755 t/t5005-archive-uid-gid.sh\n>\n> diff --git a/archive-tar.c b/archive-tar.c\n> index c6ed96ee7..8546a6229 100644\n> --- a/archive-tar.c\n> +++ b/archive-tar.c\n> @@ -204,10 +204,10 @@ static void prepare_header(struct archiver_args *args,\n>  \txsnprintf(header->size, sizeof(header->size), \"%011lo\", S_ISREG(mode) ? size : 0);\n>  \txsnprintf(header->mtime, sizeof(header->mtime), \"%011lo\", (unsigned long) args->time);\n>\n> -\txsnprintf(header->uid, sizeof(header->uid), \"%07o\", 0);\n> -\txsnprintf(header->gid, sizeof(header->gid), \"%07o\", 0);\n> -\tstrlcpy(header->uname, \"root\", sizeof(header->uname));\n> -\tstrlcpy(header->gname, \"root\", sizeof(header->gname));\n> +\txsnprintf(header->uid, sizeof(header->uid), \"%07o\", args->uid);\n> +\txsnprintf(header->gid, sizeof(header->gid), \"%07o\", args->gid);\n> +\tstrlcpy(header->uname, args->uname ? args->uname : \"root\", sizeof(header->uname));\n> +\tstrlcpy(header->gname, args->gname ? args->gname : \"root\", sizeof(header->gname));\n>  \txsnprintf(header->devmajor, sizeof(header->devmajor), \"%07o\", 0);\n>  \txsnprintf(header->devminor, sizeof(header->devminor), \"%07o\", 0);\n>\n> diff --git a/archive.c b/archive.c\n> index 0b7b62af0..db69041f1 100644\n> --- a/archive.c\n> +++ b/archive.c\n> @@ -8,6 +8,7 @@\n>  #include \"parse-options.h\"\n>  #include \"unpack-trees.h\"\n>  #include \"dir.h\"\n> +#include \"git-compat-util.h\"\n>\n>  static char const * const archive_usage[] = {\n>  \tN_(\"git archive [<options>] <tree-ish> [<path>...]\"),\n> @@ -417,6 +418,57 @@ static void parse_treeish_arg(const char **argv,\n>  \t{ OPTION_SET_INT, (s), NULL, (v), NULL, \"\", \\\n>  \t  PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_HIDDEN, NULL, (p) }\n>\n> +static void set_args_uname_uid(struct archiver_args *args,\n> +\t\tconst char* tar_owner, int set_gid_too)\n> +{\n> +\tif (!args || !tar_owner)\n> +\t\treturn;\n> +\n> +\tconst char* col_pos = strchr(tar_owner, ':');\n> +\tstruct passwd* pw = NULL;\n> +\n> +\tif (col_pos) {\n> +\t\targs->uname = xstrndup(tar_owner, col_pos - tar_owner);\n> +\t\targs->uid = atoi(col_pos + 1);\n> +\t\treturn;\n> +\t}\n> +\n> +\targs->uname = xstrndup(tar_owner, strlen(tar_owner));\n> +\tpw = getpwnam(tar_owner);\n> +\tif (!pw)\n> +\t\treturn;\n\nBoth here and in set_args_gname_gid you just silently ignore failures\nwhen you can't find the group, is this really the desired behavior?\n\nAlso, it seems there's no way to specify a numeric id / group id, e.g. a\nreasonable use-case is hardcoding a group that you don't have on the\ncomputer you make the tarball on. Doesn't GNU tar support that as well?\n\n> +\targs->uid = pw->pw_uid;\n> +\tif (set_gid_too)\n> +\t\targs->gid = pw->pw_gid;\n> +\n> +\treturn;\n> +}\n> +\n> +static void set_args_gname_gid(struct archiver_args *args,\n> +\t\tconst char* tar_group)\n> +{\n> +\tif (!args || !tar_group)\n> +\t\treturn;\n> +\n> +\tconst char* col_pos = strchr(tar_group, ':');\n> +\tstruct group* gr = NULL;\n> +\n> +\tif (col_pos) {\n> +\t\targs->gname = xstrndup(tar_group, col_pos - tar_group);\n> +\t\targs->gid = atoi(col_pos + 1);\n> +\t\treturn;\n> +\t}\n> +\n> +\targs->gname = xstrndup(tar_group, strlen(tar_group));\n> +\tgr = getgrnam(tar_group);\n> +\tif (!gr)\n> +\t\treturn;\n> +\n> +\targs->gid = gr->gr_gid;\n> +\treturn;\n> +}\n> +\n>  static int parse_archive_args(int argc, const char **argv,\n>  \t\tconst struct archiver **ar, struct archiver_args *args,\n>  \t\tconst char *name_hint, int is_remote)\n> @@ -431,6 +483,8 @@ static int parse_archive_args(int argc, const char **argv,\n>  \tint i;\n>  \tint list = 0;\n>  \tint worktree_attributes = 0;\n> +\tchar *tar_owner = NULL;\n> +\tchar *tar_group = NULL;\n>  \tstruct option opts[] = {\n>  \t\tOPT_GROUP(\"\"),\n>  \t\tOPT_STRING(0, \"format\", &format, N_(\"fmt\"), N_(\"archive format\")),\n> @@ -459,6 +513,8 @@ static int parse_archive_args(int argc, const char **argv,\n>  \t\t\tN_(\"retrieve the archive from remote repository <repo>\")),\n>  \t\tOPT_STRING(0, \"exec\", &exec, N_(\"command\"),\n>  \t\t\tN_(\"path to the remote git-upload-archive command\")),\n> +\t\tOPT_STRING(0, \"owner\", &tar_owner, N_(\"owner\"), N_(\"<name[:uid]> in tar\")),\n> +\t\tOPT_STRING(0, \"group\", &tar_group, N_(\"group\"), N_(\"<name[:gid]> in tar\")),\n>  \t\tOPT_END()\n>  \t};\n>\n> @@ -507,6 +563,16 @@ static int parse_archive_args(int argc, const char **argv,\n>  \targs->baselen = strlen(base);\n>  \targs->worktree_attributes = worktree_attributes;\n>\n> +\targs->uname = NULL;\n> +\targs->gname = NULL;\n> +\targs->uid = 0;\n> +\targs->gid = 0;\n> +\tset_args_uname_uid(args,\n> +\t\ttar_owner,\n> +\t\t1 /* init args->gid by pw, if resolved */);\n> +\tset_args_gname_gid(args,\n> +\t\ttar_group);\n> +\n\nSome overzelous line wrapping here. This could all just be on one line.\n\n>  \treturn argc;\n>  }\n>\n> diff --git a/archive.h b/archive.h\n> index 62d1d82c1..fad4a5d3e 100644\n> --- a/archive.h\n> +++ b/archive.h\n> @@ -15,6 +15,10 @@ struct archiver_args {\n>  \tunsigned int worktree_attributes : 1;\n>  \tunsigned int convert : 1;\n>  \tint compression_level;\n> +\tint uid;\n> +\tint gid;\n> +\tconst char *uname;\n> +\tconst char *gname;\n>  };\n>\n>  #define ARCHIVER_WANT_COMPRESSION_LEVELS 1\n> diff --git a/t/parse-tar-file.py b/t/parse-tar-file.py\n\nIf you add a helper script please put it in a folder in t/, see \"ls\nt/*/*.pl\" for examples.\n\n> new file mode 100755\n> index 000000000..c240a8676\n> --- /dev/null\n> +++ b/t/parse-tar-file.py\n> @@ -0,0 +1,56 @@\n> +#!/usr/bin/env python\n> +\n> +import sys\n> +import getopt\n> +import tarfile\n> +\n> +optlist, args = getopt.getopt( sys.argv[1:], \"\", [\n> +\t\t\t\t\"print=\", \"show=\",\n> +\t\t\t\t\"uniq\",\n> +\t\t\t\t\"fail-if-multi\",\n> +\t\t] )\n> +\n> +infos_to_print = []\n> +uniq = False\n> +fail_if_multi = False\n> +\n> +for opt in optlist:\n> +  if opt[0] == \"--print\":\n> +    infos_to_print.append(opt[1])\n> +  elif opt[0] == \"--show\":\n> +    infos_to_print.append(opt[1])\n> +  elif opt[0] == \"--uniq\":\n> +    uniq = True\n> +  elif opt[0] == \"--fail-if-multi\":\n> +    uniq = True\n> +    fail_if_multi = True\n> +\n\nAlso the Git project uses 4-space indent for Python elsewhere AFACT.\n\n> +if len(infos_to_print) == 0:\n> +  infos_to_print = [\"uid\", \"gid\", \"uname\", \"gname\", \"name\"]\n> +\n> +tar = tarfile.open( args[0] )\n> +out_lines = []\n> +for tarinfo in tar:\n> +  infos = []\n> +  for info_tag in infos_to_print:\n> +    if info_tag == \"uid\":\n> +      infos.append( str(tarinfo.uid) )\n> +    elif info_tag == \"gid\":\n> +      infos.append( str(tarinfo.gid) )\n> +    elif info_tag == \"uname\" or info_tag == \"owner\":\n> +      infos.append( tarinfo.uname )\n> +    elif info_tag == \"gname\" or info_tag == \"group\":\n> +      infos.append( tarinfo.gname )\n> +    elif info_tag == \"name\" or info_tag == \"pathname\":\n> +      infos.append( tarinfo.name )\n> +  out_lines.append( \"\\t\".join(infos) )\n> +tar.close()\n> +\n> +if uniq:\n> +  out_lines = list(set(out_lines))\n> +  if fail_if_multi and (len(out_lines) > 1):\n> +    sys.stderr.write(\"*** not unique value, \" + str(len(out_lines)) + \" values found\\n\")\n> +    sys.exit(len(out_lines))\n> +\n> +for line in out_lines:\n> +  print line\n> diff --git a/t/t5005-archive-uid-gid.sh b/t/t5005-archive-uid-gid.sh\n> new file mode 100755\n> index 000000000..2b2a694d8\n> --- /dev/null\n> +++ b/t/t5005-archive-uid-gid.sh\n> @@ -0,0 +1,71 @@\n> +#!/bin/sh\n> +\n> +test_description='test --owner --group options for git-archive'\n> +. ./test-lib.sh\n> +\n> +check_uid_gid_uname_gname_in_tar() {\n> +\t# $1 tar pathname\n> +\t# $2 uid (digit in string)\n> +\t# $3 gid (digit in string)\n> +\t# $4 uname (string)\n> +\t# $5 gname (string)\n> +\tuid=`./parse-tar-file.py --print=uid --fail-if-multi $1`\n> +\tif test $? != 0 -o x\"${uid}\" != \"x\"$2\n> +\tthen\n> +\t  echo \"(some) uid differs from the specified value\"\n> +\t  return $?\n> +        fi\n\nVery odd indenting going on here. Should be tabs.\n\n> +\n> +\tgid=`./parse-tar-file.py --print=gid --fail-if-multi $1`\n> +\tif test $? != 0 -o x\"${gid}\" != \"x\"$3\n> +\tthen\n> +\t  echo \"(some) gid differs from the specified value\"\n> +\t  return $?\n> +\t  exit $?\n> +        fi\n> +\n> +\tuname=`./parse-tar-file.py --print=uname --fail-if-multi $1`\n> +\tif test $? != 0 -o x\"${uname}\" != \"x\"$4\n> +\tthen\n> +\t  echo \"(some) uname differs from the specified value\"\n> +\t  return $?\n> +        fi\n> +\n> +\tgname=`./parse-tar-file.py --print=gname --fail-if-multi $1`\n> +\tif test $? != 0 -o x\"${gname}\" != \"x\"$5\n> +\tthen\n> +\t  echo \"(some) gname differs from the specified value\"\n> +\t  return $?\n> +        fi\n> +\n> +\treturn 0\n> +}\n> +\n> +git init . 1>/dev/null 2>/dev/null\n> +touch uid-gid-test.001\n> +mkdir uid-gid-test.002\n> +mkdir uid-gid-test.002/uid-gid-test.003\n> +git add uid-gid-test.001\n> +git add uid-gid-test.002\n> +git add uid-gid-test.002/uid-gid-test.003\n> +git commit -m \"uid-gid-test\" 2>/dev/null 1>/dev/null\n\nAll of this setup stuff should be in a test, see \"git grep\ntest_expect.*setup\" for examples. Also see the helpers we have in\nt/README, i.e. \"test_commit\" etc.\n\n> +test_expect_success 'test a case with explicitly specified name/id, owner=nobody:1234 group=nogroup:5678' '\n> +\tgit archive --format=tar --owner nobody:1234 --group nogroup:5678 HEAD > uid-gid-test1.tar &&\n> +\tcheck_uid_gid_uname_gname_in_tar uid-gid-test1.tar 1234 5678 nobody nogroup &&\n> +\ttest_result=$? &&\n> +\treturn ${test_result}\n> +'\n\nThere's no need to assign $? to a variable and then return it, just make\ncheck_uid_gid_uname_gname_in_tar be the last thing you do in the test.\n\n> +\n> +test_expect_success 'test a case with only name is specified, owner=(current my name) group=(current my group)' '\n> +\tmy_uid=`id -u` &&\n> +\tmy_gid=`id -g` &&\n> +\tmy_uname=`id -u -n` &&\n> +\tmy_gname=`id -g -n` &&\n> +\tgit archive --format=tar --owner ${my_uname} --group ${my_gname} HEAD > uid-gid-test2.tar &&\n> +\tcheck_uid_gid_uname_gname_in_tar uid-gid-test2.tar ${my_uid} ${my_gid} ${my_uname} ${my_gname} &&\n> +\ttest_result=$? &&\n> +\treturn ${test_result}\n> +'\n> +\n> +test_done\n\nI'd probably notice other stuff if I did more than skim this, but I'll\nstop for now...\n"},{"id":"335481","messageId":"xmqqy3lmk66n.fsf@gitster.mtv.corp.google.com","threadId":"47503","inReplyTo":"20171228090527.25056-1-mpsuzuki@hiroshima-u.ac.jp","subject":"Re: [PATCH] git-archive: accepts --owner --group aslike GNU tar.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-28T19:23:28Z","receivedAt":"2017-12-28T19:23:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"suzuki toshiya <mpsuzuki@hiroshima-u.ac.jp> writes:\n\n> Current tar output by git-archive has always root:root.\n\nThanks for a patch.  On top of Ævar's comments...\n\n> ...\n> * t/t5005-archive-uid-gid.sh: a test script comparing\n>   uid, gid, uname, gname between the options and\n>   generated tar file.\n> ---\n\nBefore the \"---\" line, we need to get the patch signed off by the\nauthor (see Documentation/SubmittingPatches).\n\n> diff --git a/archive-tar.c b/archive-tar.c\n> index c6ed96ee7..8546a6229 100644\n> --- a/archive-tar.c\n> +++ b/archive-tar.c\n> @@ -204,10 +204,10 @@ static void prepare_header(struct archiver_args *args,\n>  \txsnprintf(header->size, sizeof(header->size), \"%011lo\", S_ISREG(mode) ? size : 0);\n>  \txsnprintf(header->mtime, sizeof(header->mtime), \"%011lo\", (unsigned long) args->time);\n>  \n> -\txsnprintf(header->uid, sizeof(header->uid), \"%07o\", 0);\n> -\txsnprintf(header->gid, sizeof(header->gid), \"%07o\", 0);\n> -\tstrlcpy(header->uname, \"root\", sizeof(header->uname));\n> -\tstrlcpy(header->gname, \"root\", sizeof(header->gname));\n> +\txsnprintf(header->uid, sizeof(header->uid), \"%07o\", args->uid);\n> +\txsnprintf(header->gid, sizeof(header->gid), \"%07o\", args->gid);\n> +\tstrlcpy(header->uname, args->uname ? args->uname : \"root\", sizeof(header->uname));\n> +\tstrlcpy(header->gname, args->gname ? args->gname : \"root\", sizeof(header->gname));\n\nWould it be cleaner to make sure aregs->[gu]name is always set\n(i.e. stuff \"root\" when it is not given)?\n\n>  \txsnprintf(header->devmajor, sizeof(header->devmajor), \"%07o\", 0);\n>  \txsnprintf(header->devminor, sizeof(header->devminor), \"%07o\", 0);\n>  \n> diff --git a/archive.c b/archive.c\n> index 0b7b62af0..db69041f1 100644\n> --- a/archive.c\n> +++ b/archive.c\n> @@ -8,6 +8,7 @@\n>  #include \"parse-options.h\"\n>  #include \"unpack-trees.h\"\n>  #include \"dir.h\"\n> +#include \"git-compat-util.h\"\n\nThe coding guideline says that \"git-compat-util.h\" (or one of the\nwell-known header that includes it) should be the first file to be\nincluded, and we already include \"cache.h\" as the first thing, so\nI do not think you want this addition here.\n\n> @@ -417,6 +418,57 @@ static void parse_treeish_arg(const char **argv,\n>  \t{ OPTION_SET_INT, (s), NULL, (v), NULL, \"\", \\\n>  \t  PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_HIDDEN, NULL, (p) }\n>  \n> +static void set_args_uname_uid(struct archiver_args *args,\n> +\t\tconst char* tar_owner, int set_gid_too)\n\nThe asterisk sticks to the variable name, not the type name, i.e.\n\n\tconst char *tar_owner\n\n> +{\n> +\tif (!args || !tar_owner)\n> +\t\treturn;\n> +\n> +\tconst char* col_pos = strchr(tar_owner, ':');\n> +\tstruct passwd* pw = NULL;\n\nDecl after statement.\n\n> +\tif (col_pos) {\n> +\t\targs->uname = xstrndup(tar_owner, col_pos - tar_owner);\n> +\t\targs->uid = atoi(col_pos + 1);\n> +\t\treturn;\n> +\t}\n> +\n> +\targs->uname = xstrndup(tar_owner, strlen(tar_owner));\n> +\tpw = getpwnam(tar_owner);\n> +\tif (!pw)\n> +\t\treturn;\n\nThis means that upon error, the caller gets a half-filled args\nstructure and without any indication.\n\n> diff --git a/t/parse-tar-file.py b/t/parse-tar-file.py\n\nHmph.  Do we still use Python around here?\n"},{"id":"335530","messageId":"5A464B6D.1010102@hiroshima-u.ac.jp","threadId":"47503","inReplyTo":"548d442f05e24e22a1cf4e0074f23f16@OS2PR01MB1147.jpnprd01.prod.outlook.com","subject":"Re: [PATCH] git-archive: accepts --owner --group aslike GNU tar.","fromName":"suzuki toshiya","fromEmail":"mpsuzuki@hiroshima-u.ac.jp","sentAt":"2017-12-29T14:04:29Z","receivedAt":"2017-12-29T14:04:27Z","isPatch":true,"sender":{"key":"mpsuzuki@hiroshima-u.ac.jp","avatar":null},"body":"Dear Junio, Ævar\n\nThank you very much for your reviews, in spite of my many\noverlooking of the requirements written in the documents.\n\nTo classify various cases, I modified my patch heavily.\nIt would be posted soon.\n\nI found that some Python scripts are included in the git\nrepository, but nothing in t/ directories. A helper written\nin Python is not welcomed? If so, I would rewrite the\nhelper in C (perl's standard library does not have a parse\nof tarfile). So, please do not review parse-tar-file.py.\n\nRegards,\nmpsuzuki\n\nJunio C Hamano wrote:\n> suzuki toshiya <mpsuzuki@hiroshima-u.ac.jp> writes:\n> \n>> Current tar output by git-archive has always root:root.\n> \n> Thanks for a patch.  On top of Ævar's comments...\n> \n>> ...\n>> * t/t5005-archive-uid-gid.sh: a test script comparing\n>>   uid, gid, uname, gname between the options and\n>>   generated tar file.\n>> ---\n> \n> Before the \"---\" line, we need to get the patch signed off by the\n> author (see Documentation/SubmittingPatches).\n> \n>> diff --git a/archive-tar.c b/archive-tar.c\n>> index c6ed96ee7..8546a6229 100644\n>> --- a/archive-tar.c\n>> +++ b/archive-tar.c\n>> @@ -204,10 +204,10 @@ static void prepare_header(struct archiver_args *args,\n>>  \txsnprintf(header->size, sizeof(header->size), \"%011lo\", S_ISREG(mode) ? size : 0);\n>>  \txsnprintf(header->mtime, sizeof(header->mtime), \"%011lo\", (unsigned long) args->time);\n>>  \n>> -\txsnprintf(header->uid, sizeof(header->uid), \"%07o\", 0);\n>> -\txsnprintf(header->gid, sizeof(header->gid), \"%07o\", 0);\n>> -\tstrlcpy(header->uname, \"root\", sizeof(header->uname));\n>> -\tstrlcpy(header->gname, \"root\", sizeof(header->gname));\n>> +\txsnprintf(header->uid, sizeof(header->uid), \"%07o\", args->uid);\n>> +\txsnprintf(header->gid, sizeof(header->gid), \"%07o\", args->gid);\n>> +\tstrlcpy(header->uname, args->uname ? args->uname : \"root\", sizeof(header->uname));\n>> +\tstrlcpy(header->gname, args->gname ? args->gname : \"root\", sizeof(header->gname));\n> \n> Would it be cleaner to make sure aregs->[gu]name is always set\n> (i.e. stuff \"root\" when it is not given)?\n> \n>>  \txsnprintf(header->devmajor, sizeof(header->devmajor), \"%07o\", 0);\n>>  \txsnprintf(header->devminor, sizeof(header->devminor), \"%07o\", 0);\n>>  \n>> diff --git a/archive.c b/archive.c\n>> index 0b7b62af0..db69041f1 100644\n>> --- a/archive.c\n>> +++ b/archive.c\n>> @@ -8,6 +8,7 @@\n>>  #include \"parse-options.h\"\n>>  #include \"unpack-trees.h\"\n>>  #include \"dir.h\"\n>> +#include \"git-compat-util.h\"\n> \n> The coding guideline says that \"git-compat-util.h\" (or one of the\n> well-known header that includes it) should be the first file to be\n> included, and we already include \"cache.h\" as the first thing, so\n> I do not think you want this addition here.\n> \n>> @@ -417,6 +418,57 @@ static void parse_treeish_arg(const char **argv,\n>>  \t{ OPTION_SET_INT, (s), NULL, (v), NULL, \"\", \\\n>>  \t  PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_HIDDEN, NULL, (p) }\n>>  \n>> +static void set_args_uname_uid(struct archiver_args *args,\n>> +\t\tconst char* tar_owner, int set_gid_too)\n> \n> The asterisk sticks to the variable name, not the type name, i.e.\n> \n> \tconst char *tar_owner\n> \n>> +{\n>> +\tif (!args || !tar_owner)\n>> +\t\treturn;\n>> +\n>> +\tconst char* col_pos = strchr(tar_owner, ':');\n>> +\tstruct passwd* pw = NULL;\n> \n> Decl after statement.\n> \n>> +\tif (col_pos) {\n>> +\t\targs->uname = xstrndup(tar_owner, col_pos - tar_owner);\n>> +\t\targs->uid = atoi(col_pos + 1);\n>> +\t\treturn;\n>> +\t}\n>> +\n>> +\targs->uname = xstrndup(tar_owner, strlen(tar_owner));\n>> +\tpw = getpwnam(tar_owner);\n>> +\tif (!pw)\n>> +\t\treturn;\n> \n> This means that upon error, the caller gets a half-filled args\n> structure and without any indication.\n> \n>> diff --git a/t/parse-tar-file.py b/t/parse-tar-file.py\n> \n> Hmph.  Do we still use Python around here?\n> \n\n"}]}