{"thread":{"id":"36989","subject":"[PATCH 0/4] git replace --edit --raw","startedAt":"2014-06-24T09:42:25Z","lastAt":"2014-06-26T15:55:30Z","messageCount":9,"participants":["Jeff King","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"244914","messageId":"20140624094217.GA14216@sigill.intra.peff.net","threadId":"36989","inReplyTo":null,"subject":"[PATCH 0/4] git replace --edit --raw","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-24T09:42:25Z","receivedAt":"2014-06-24T09:42:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I tried to use \"git replace --edit\" today to repair a corrupted tree\nthat had an empty filename:\n\n  $ git replace --edit HEAD^{tree}\n  fatal: empty filename in tree entry\n  fatal: cat-file reported failure\n\nOops. One of the major purposes of the command is to repair broken\nobjects. This series introduces a \"--raw\" option to let you easily edit\nthe binary if need be.\n\nThe first two patches are unrelated cleanups I noticed in the area. The\nthird is refactoring, and the final one is the interesting bit.\n\nThere's a minor textual conflict in the documentation with\ncc/replace-graft, but it's pretty straightforward to resolve.\n\n  [1/4]: replace: replace spaces with tabs in indentation\n  [2/4]: avoid double close of descriptors handed to run_command\n  [3/4]: replace: use argv_array in export_object\n  [4/4]: replace: add a --raw mode for --edit\n\n-Peff\n"},{"id":"244915","messageId":"20140624094303.GA14514@sigill.intra.peff.net","threadId":"36989","inReplyTo":"20140624094217.GA14216@sigill.intra.peff.net","subject":"[PATCH 1/4] replace: replace spaces with tabs in indentation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-24T09:43:07Z","receivedAt":"2014-06-24T09:43:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This matches our usual style and the surrounding code.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/replace.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex 1bb491d..8507835 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -23,9 +23,9 @@ static const char * const git_replace_usage[] = {\n };\n \n enum replace_format {\n-      REPLACE_FORMAT_SHORT,\n-      REPLACE_FORMAT_MEDIUM,\n-      REPLACE_FORMAT_LONG\n+\tREPLACE_FORMAT_SHORT,\n+\tREPLACE_FORMAT_MEDIUM,\n+\tREPLACE_FORMAT_LONG\n };\n \n struct show_data {\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244916","messageId":"20140624094545.GB14514@sigill.intra.peff.net","threadId":"36989","inReplyTo":"20140624094217.GA14216@sigill.intra.peff.net","subject":"[PATCH 2/4] avoid double close of descriptors handed to run_command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-24T09:45:46Z","receivedAt":"2014-06-24T09:45:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When a file descriptor is given to run_command via the\n\"in\", \"out\", or \"err\" parameters, run_command takes\nownership. The descriptor will be closed in the parent\nprocess whether the process is spawned successfully or not,\nand closing it again is wrong.\n\nIn practice this has not caused problems, because we usually\nclose() right after start_command returns, meaning no other\ncode has opened a descriptor in the meantime. So we just get\nEBADF and ignore it (rather than accidentally closing\nsomebody else's descriptor!).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI noticed the one in replace, and grepped around for other instances. I\nthink this is all of them. The only possible cases I didn't investigate\ncarefully were in send-pack/receive-pack. The logic there is quite\ncomplicated, and I remember looking at and fixing close() issues there\nnot too long ago (e.g., 49ecfa1), so I assumed it was OK.\n\n builtin/replace.c | 2 --\n daemon.c          | 1 -\n 2 files changed, 3 deletions(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex 8507835..eb1d2ec 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -207,8 +207,6 @@ static void export_object(const unsigned char *sha1, const char *filename)\n \n \tif (run_command(&cmd))\n \t\tdie(\"cat-file reported failure\");\n-\n-\tclose(fd);\n }\n \n /*\ndiff --git a/daemon.c b/daemon.c\nindex f9c63e9..e1e424d 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -775,7 +775,6 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n \t\tlogerror(\"unable to fork\");\n \telse\n \t\tadd_child(&cld, addr, addrlen);\n-\tclose(incoming);\n }\n \n static void child_handler(int signo)\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244917","messageId":"20140624094605.GC14514@sigill.intra.peff.net","threadId":"36989","inReplyTo":"20140624094217.GA14216@sigill.intra.peff.net","subject":"[PATCH 3/4] replace: use argv_array in export_object","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-24T09:46:05Z","receivedAt":"2014-06-24T09:46:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is a little more verbose, but will make it easier to\nmake parts of our command-line conditional (without\nresorting to magic numbers or lots of NULLs to get an\nappropriately sized argv array).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/replace.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex eb1d2ec..2584170 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -193,15 +193,17 @@ static int replace_object(const char *object_ref, const char *replace_ref, int f\n  */\n static void export_object(const unsigned char *sha1, const char *filename)\n {\n-\tconst char *argv[] = { \"--no-replace-objects\", \"cat-file\", \"-p\", NULL, NULL };\n-\tstruct child_process cmd = { argv };\n+\tstruct child_process cmd = { NULL };\n \tint fd;\n \n \tfd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, 0666);\n \tif (fd < 0)\n \t\tdie_errno(\"unable to open %s for writing\", filename);\n \n-\targv[3] = sha1_to_hex(sha1);\n+\targv_array_push(&cmd.args, \"--no-replace-objects\");\n+\targv_array_push(&cmd.args, \"cat-file\");\n+\targv_array_push(&cmd.args, \"-p\");\n+\targv_array_push(&cmd.args, sha1_to_hex(sha1));\n \tcmd.git_cmd = 1;\n \tcmd.out = fd;\n \n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244918","messageId":"20140624094631.GD14514@sigill.intra.peff.net","threadId":"36989","inReplyTo":"20140624094217.GA14216@sigill.intra.peff.net","subject":"[PATCH 4/4] replace: add a --raw mode for --edit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-24T09:46:31Z","receivedAt":"2014-06-24T09:46:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"One of the purposes of \"git replace --edit\" is to help a\nuser repair objects which are malformed or corrupted.\nUsually we pretty-print trees with \"ls-tree\", which is much\neasier to work with than the raw binary data.  However, some\nforms of corruption break the tree-walker, in which case our\npretty-printing fails, rendering \"--edit\" useless for the\nuser.\n\nThis patch introduces a \"--raw\" option, which lets you edit\nthe binary data in these instances.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/git-replace.txt |  8 ++++++++\n builtin/replace.c             | 31 +++++++++++++++++++++----------\n 2 files changed, 29 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt\nindex 61461b9..089dcac 100644\n--- a/Documentation/git-replace.txt\n+++ b/Documentation/git-replace.txt\n@@ -73,6 +73,14 @@ OPTIONS\n \tnewly created object. See linkgit:git-var[1] for details about\n \thow the editor will be chosen.\n \n+--raw::\n+\tWhen editing, provide the raw object contents rather than\n+\tpretty-printed ones. Currently this only affects trees, which\n+\twill be shown in their binary form. This is harder to work with,\n+\tbut can help when repairing a tree that is so corrupted it\n+\tcannot be pretty-printed. Note that you may need to configure\n+\tyour editor to cleanly read and write binary data.\n+\n -l <pattern>::\n --list <pattern>::\n \tList replace refs for objects that match the given pattern (or\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex 2584170..d1ea2c2 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -188,10 +188,12 @@ static int replace_object(const char *object_ref, const char *replace_ref, int f\n }\n \n /*\n- * Write the contents of the object named by \"sha1\" to the file \"filename\",\n- * pretty-printed for human editing based on its type.\n+ * Write the contents of the object named by \"sha1\" to the file \"filename\".\n+ * If \"raw\" is true, then the object's raw contents are printed according to\n+ * \"type\". Otherwise, we pretty-print the contents for human editing.\n  */\n-static void export_object(const unsigned char *sha1, const char *filename)\n+static void export_object(const unsigned char *sha1, enum object_type type,\n+\t\t\t  int raw, const char *filename)\n {\n \tstruct child_process cmd = { NULL };\n \tint fd;\n@@ -202,7 +204,10 @@ static void export_object(const unsigned char *sha1, const char *filename)\n \n \targv_array_push(&cmd.args, \"--no-replace-objects\");\n \targv_array_push(&cmd.args, \"cat-file\");\n-\targv_array_push(&cmd.args, \"-p\");\n+\tif (raw)\n+\t\targv_array_push(&cmd.args, typename(type));\n+\telse\n+\t\targv_array_push(&cmd.args, \"-p\");\n \targv_array_push(&cmd.args, sha1_to_hex(sha1));\n \tcmd.git_cmd = 1;\n \tcmd.out = fd;\n@@ -217,7 +222,7 @@ static void export_object(const unsigned char *sha1, const char *filename)\n  * The sha1 of the written object is returned via sha1.\n  */\n static void import_object(unsigned char *sha1, enum object_type type,\n-\t\t\t  const char *filename)\n+\t\t\t  int raw, const char *filename)\n {\n \tint fd;\n \n@@ -225,7 +230,7 @@ static void import_object(unsigned char *sha1, enum object_type type,\n \tif (fd < 0)\n \t\tdie_errno(\"unable to open %s for reading\", filename);\n \n-\tif (type == OBJ_TREE) {\n+\tif (!raw && type == OBJ_TREE) {\n \t\tconst char *argv[] = { \"mktree\", NULL };\n \t\tstruct child_process cmd = { argv };\n \t\tstruct strbuf result = STRBUF_INIT;\n@@ -265,7 +270,7 @@ static void import_object(unsigned char *sha1, enum object_type type,\n \t */\n }\n \n-static int edit_and_replace(const char *object_ref, int force)\n+static int edit_and_replace(const char *object_ref, int force, int raw)\n {\n \tchar *tmpfile = git_pathdup(\"REPLACE_EDITOBJ\");\n \tenum object_type type;\n@@ -281,10 +286,10 @@ static int edit_and_replace(const char *object_ref, int force)\n \n \tcheck_ref_valid(old, prev, ref, sizeof(ref), force);\n \n-\texport_object(old, tmpfile);\n+\texport_object(old, type, raw, tmpfile);\n \tif (launch_editor(tmpfile, NULL, NULL) < 0)\n \t\tdie(\"editing object file failed\");\n-\timport_object(new, type, tmpfile);\n+\timport_object(new, type, raw, tmpfile);\n \n \tfree(tmpfile);\n \n@@ -297,6 +302,7 @@ static int edit_and_replace(const char *object_ref, int force)\n int cmd_replace(int argc, const char **argv, const char *prefix)\n {\n \tint force = 0;\n+\tint raw = 0;\n \tconst char *format = NULL;\n \tenum {\n \t\tMODE_UNSPECIFIED = 0,\n@@ -310,6 +316,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n \t\tOPT_CMDMODE('d', \"delete\", &cmdmode, N_(\"delete replace refs\"), MODE_DELETE),\n \t\tOPT_CMDMODE('e', \"edit\", &cmdmode, N_(\"edit existing object\"), MODE_EDIT),\n \t\tOPT_BOOL('f', \"force\", &force, N_(\"replace the ref if it exists\")),\n+\t\tOPT_BOOL(0, \"raw\", &raw, N_(\"do not pretty-print contents for --edit\")),\n \t\tOPT_STRING(0, \"format\", &format, N_(\"format\"), N_(\"use this format\")),\n \t\tOPT_END()\n \t};\n@@ -329,6 +336,10 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n \t\tusage_msg_opt(\"-f only makes sense when writing a replacement\",\n \t\t\t      git_replace_usage, options);\n \n+\tif (raw && cmdmode != MODE_EDIT)\n+\t\tusage_msg_opt(\"--raw only makes sense with --edit\",\n+\t\t\t      git_replace_usage, options);\n+\n \tswitch (cmdmode) {\n \tcase MODE_DELETE:\n \t\tif (argc < 1)\n@@ -346,7 +357,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n \t\tif (argc != 1)\n \t\t\tusage_msg_opt(\"-e needs exactly one argument\",\n \t\t\t\t      git_replace_usage, options);\n-\t\treturn edit_and_replace(argv[0], force);\n+\t\treturn edit_and_replace(argv[0], force, raw);\n \n \tcase MODE_LIST:\n \t\tif (argc > 1)\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244954","messageId":"CAPig+cRKxwJ64=qFAtefonutmZuURpTRZyjyjSaxcpuinRGL4g@mail.gmail.com","threadId":"36989","inReplyTo":"20140624094631.GD14514@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] replace: add a --raw mode for --edit","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-25T01:40:09Z","receivedAt":"2014-06-25T01:40:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jun 24, 2014 at 5:46 AM, Jeff King <peff@peff.net> wrote:\n> One of the purposes of \"git replace --edit\" is to help a\n> user repair objects which are malformed or corrupted.\n> Usually we pretty-print trees with \"ls-tree\", which is much\n> easier to work with than the raw binary data.  However, some\n> forms of corruption break the tree-walker, in which case our\n> pretty-printing fails, rendering \"--edit\" useless for the\n> user.\n>\n> This patch introduces a \"--raw\" option, which lets you edit\n> the binary data in these instances.\n\nIs there a possibility that any of the other git-replace modes will\ngrow a need for \"raw\"? If not, would it make sense to make this\nspecific to \"edit\" as --edit=raw?\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  Documentation/git-replace.txt |  8 ++++++++\n>  builtin/replace.c             | 31 +++++++++++++++++++++----------\n>  2 files changed, 29 insertions(+), 10 deletions(-)\n>\n> diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt\n> index 61461b9..089dcac 100644\n> --- a/Documentation/git-replace.txt\n> +++ b/Documentation/git-replace.txt\n> @@ -73,6 +73,14 @@ OPTIONS\n>         newly created object. See linkgit:git-var[1] for details about\n>         how the editor will be chosen.\n>\n> +--raw::\n> +       When editing, provide the raw object contents rather than\n> +       pretty-printed ones. Currently this only affects trees, which\n> +       will be shown in their binary form. This is harder to work with,\n> +       but can help when repairing a tree that is so corrupted it\n> +       cannot be pretty-printed. Note that you may need to configure\n> +       your editor to cleanly read and write binary data.\n> +\n>  -l <pattern>::\n>  --list <pattern>::\n>         List replace refs for objects that match the given pattern (or\n> diff --git a/builtin/replace.c b/builtin/replace.c\n> index 2584170..d1ea2c2 100644\n> --- a/builtin/replace.c\n> +++ b/builtin/replace.c\n> @@ -188,10 +188,12 @@ static int replace_object(const char *object_ref, const char *replace_ref, int f\n>  }\n>\n>  /*\n> - * Write the contents of the object named by \"sha1\" to the file \"filename\",\n> - * pretty-printed for human editing based on its type.\n> + * Write the contents of the object named by \"sha1\" to the file \"filename\".\n> + * If \"raw\" is true, then the object's raw contents are printed according to\n> + * \"type\". Otherwise, we pretty-print the contents for human editing.\n>   */\n> -static void export_object(const unsigned char *sha1, const char *filename)\n> +static void export_object(const unsigned char *sha1, enum object_type type,\n> +                         int raw, const char *filename)\n>  {\n>         struct child_process cmd = { NULL };\n>         int fd;\n> @@ -202,7 +204,10 @@ static void export_object(const unsigned char *sha1, const char *filename)\n>\n>         argv_array_push(&cmd.args, \"--no-replace-objects\");\n>         argv_array_push(&cmd.args, \"cat-file\");\n> -       argv_array_push(&cmd.args, \"-p\");\n> +       if (raw)\n> +               argv_array_push(&cmd.args, typename(type));\n> +       else\n> +               argv_array_push(&cmd.args, \"-p\");\n>         argv_array_push(&cmd.args, sha1_to_hex(sha1));\n>         cmd.git_cmd = 1;\n>         cmd.out = fd;\n> @@ -217,7 +222,7 @@ static void export_object(const unsigned char *sha1, const char *filename)\n>   * The sha1 of the written object is returned via sha1.\n>   */\n>  static void import_object(unsigned char *sha1, enum object_type type,\n> -                         const char *filename)\n> +                         int raw, const char *filename)\n>  {\n>         int fd;\n>\n> @@ -225,7 +230,7 @@ static void import_object(unsigned char *sha1, enum object_type type,\n>         if (fd < 0)\n>                 die_errno(\"unable to open %s for reading\", filename);\n>\n> -       if (type == OBJ_TREE) {\n> +       if (!raw && type == OBJ_TREE) {\n>                 const char *argv[] = { \"mktree\", NULL };\n>                 struct child_process cmd = { argv };\n>                 struct strbuf result = STRBUF_INIT;\n> @@ -265,7 +270,7 @@ static void import_object(unsigned char *sha1, enum object_type type,\n>          */\n>  }\n>\n> -static int edit_and_replace(const char *object_ref, int force)\n> +static int edit_and_replace(const char *object_ref, int force, int raw)\n>  {\n>         char *tmpfile = git_pathdup(\"REPLACE_EDITOBJ\");\n>         enum object_type type;\n> @@ -281,10 +286,10 @@ static int edit_and_replace(const char *object_ref, int force)\n>\n>         check_ref_valid(old, prev, ref, sizeof(ref), force);\n>\n> -       export_object(old, tmpfile);\n> +       export_object(old, type, raw, tmpfile);\n>         if (launch_editor(tmpfile, NULL, NULL) < 0)\n>                 die(\"editing object file failed\");\n> -       import_object(new, type, tmpfile);\n> +       import_object(new, type, raw, tmpfile);\n>\n>         free(tmpfile);\n>\n> @@ -297,6 +302,7 @@ static int edit_and_replace(const char *object_ref, int force)\n>  int cmd_replace(int argc, const char **argv, const char *prefix)\n>  {\n>         int force = 0;\n> +       int raw = 0;\n>         const char *format = NULL;\n>         enum {\n>                 MODE_UNSPECIFIED = 0,\n> @@ -310,6 +316,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n>                 OPT_CMDMODE('d', \"delete\", &cmdmode, N_(\"delete replace refs\"), MODE_DELETE),\n>                 OPT_CMDMODE('e', \"edit\", &cmdmode, N_(\"edit existing object\"), MODE_EDIT),\n>                 OPT_BOOL('f', \"force\", &force, N_(\"replace the ref if it exists\")),\n> +               OPT_BOOL(0, \"raw\", &raw, N_(\"do not pretty-print contents for --edit\")),\n>                 OPT_STRING(0, \"format\", &format, N_(\"format\"), N_(\"use this format\")),\n>                 OPT_END()\n>         };\n> @@ -329,6 +336,10 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n>                 usage_msg_opt(\"-f only makes sense when writing a replacement\",\n>                               git_replace_usage, options);\n>\n> +       if (raw && cmdmode != MODE_EDIT)\n> +               usage_msg_opt(\"--raw only makes sense with --edit\",\n> +                             git_replace_usage, options);\n> +\n>         switch (cmdmode) {\n>         case MODE_DELETE:\n>                 if (argc < 1)\n> @@ -346,7 +357,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n>                 if (argc != 1)\n>                         usage_msg_opt(\"-e needs exactly one argument\",\n>                                       git_replace_usage, options);\n> -               return edit_and_replace(argv[0], force);\n> +               return edit_and_replace(argv[0], force, raw);\n>\n>         case MODE_LIST:\n>                 if (argc > 1)\n> --\n> 2.0.0.566.gfe3e6b2\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"244963","messageId":"20140625102454.GA5130@sigill.intra.peff.net","threadId":"36989","inReplyTo":"CAPig+cRKxwJ64=qFAtefonutmZuURpTRZyjyjSaxcpuinRGL4g@mail.gmail.com","subject":"Re: [PATCH 4/4] replace: add a --raw mode for --edit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-25T10:24:54Z","receivedAt":"2014-06-25T10:24:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 24, 2014 at 09:40:09PM -0400, Eric Sunshine wrote:\n\n> On Tue, Jun 24, 2014 at 5:46 AM, Jeff King <peff@peff.net> wrote:\n> > One of the purposes of \"git replace --edit\" is to help a\n> > user repair objects which are malformed or corrupted.\n> > Usually we pretty-print trees with \"ls-tree\", which is much\n> > easier to work with than the raw binary data.  However, some\n> > forms of corruption break the tree-walker, in which case our\n> > pretty-printing fails, rendering \"--edit\" useless for the\n> > user.\n> >\n> > This patch introduces a \"--raw\" option, which lets you edit\n> > the binary data in these instances.\n> \n> Is there a possibility that any of the other git-replace modes will\n> grow a need for \"raw\"? If not, would it make sense to make this\n> specific to \"edit\" as --edit=raw?\n\nI doubt that any other modes will want it, as it is about the\npretty-printing step which is pretty specific to --edit. However, making\nit \"--edit=raw\" also precludes adding other \"modes\" to --edit. I do\nnot have any in mind, but I do not see it as impossible.\n\nPreclude is maybe a strong word. You could have \"--edit=raw,flag1,flag2\",\nbut then we are essentially reinventing an option parser inside --edit's\nvalue. Not to mention that you cannot do \"--no-raw\", even without other\nflags being added.\n\n-Peff\n"},{"id":"244991","messageId":"xmqqsims62m1.fsf@gitster.dls.corp.google.com","threadId":"36989","inReplyTo":"20140624094631.GD14514@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] replace: add a --raw mode for --edit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-25T22:33:42Z","receivedAt":"2014-06-25T22:33:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> One of the purposes of \"git replace --edit\" is to help a\n> user repair objects which are malformed or corrupted.\n> Usually we pretty-print trees with \"ls-tree\", which is much\n> easier to work with than the raw binary data.  However, some\n> forms of corruption break the tree-walker, in which case our\n> pretty-printing fails, rendering \"--edit\" useless for the\n> user.\n>\n> This patch introduces a \"--raw\" option, which lets you edit\n> the binary data in these instances.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n\nHmmmmm, this feels almost like inviting accidents to make it easy\nand limiting the attempt to only one shot at the \"transformation\"\nstep.\n\nWill queue, but we can do the same with \"cat-file $type >temp\", do\nsome transformation on \"temp\", doubly make sure what is in \"temp\" is\nsensible and then finally \"hash-object -w -t $type temp\".  And it\nmakes me feel uneasy that the new feature seems to make it harder to\ndo the \"doubly make sure\" part.\n\nI dunno.\n\n>  Documentation/git-replace.txt |  8 ++++++++\n>  builtin/replace.c             | 31 +++++++++++++++++++++----------\n>  2 files changed, 29 insertions(+), 10 deletions(-)\n>\n> diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt\n> index 61461b9..089dcac 100644\n> --- a/Documentation/git-replace.txt\n> +++ b/Documentation/git-replace.txt\n> @@ -73,6 +73,14 @@ OPTIONS\n>  \tnewly created object. See linkgit:git-var[1] for details about\n>  \thow the editor will be chosen.\n>  \n> +--raw::\n> +\tWhen editing, provide the raw object contents rather than\n> +\tpretty-printed ones. Currently this only affects trees, which\n> +\twill be shown in their binary form. This is harder to work with,\n> +\tbut can help when repairing a tree that is so corrupted it\n> +\tcannot be pretty-printed. Note that you may need to configure\n> +\tyour editor to cleanly read and write binary data.\n> +\n>  -l <pattern>::\n>  --list <pattern>::\n>  \tList replace refs for objects that match the given pattern (or\n> diff --git a/builtin/replace.c b/builtin/replace.c\n> index 2584170..d1ea2c2 100644\n> --- a/builtin/replace.c\n> +++ b/builtin/replace.c\n> @@ -188,10 +188,12 @@ static int replace_object(const char *object_ref, const char *replace_ref, int f\n>  }\n>  \n>  /*\n> - * Write the contents of the object named by \"sha1\" to the file \"filename\",\n> - * pretty-printed for human editing based on its type.\n> + * Write the contents of the object named by \"sha1\" to the file \"filename\".\n> + * If \"raw\" is true, then the object's raw contents are printed according to\n> + * \"type\". Otherwise, we pretty-print the contents for human editing.\n>   */\n> -static void export_object(const unsigned char *sha1, const char *filename)\n> +static void export_object(const unsigned char *sha1, enum object_type type,\n> +\t\t\t  int raw, const char *filename)\n>  {\n>  \tstruct child_process cmd = { NULL };\n>  \tint fd;\n> @@ -202,7 +204,10 @@ static void export_object(const unsigned char *sha1, const char *filename)\n>  \n>  \targv_array_push(&cmd.args, \"--no-replace-objects\");\n>  \targv_array_push(&cmd.args, \"cat-file\");\n> -\targv_array_push(&cmd.args, \"-p\");\n> +\tif (raw)\n> +\t\targv_array_push(&cmd.args, typename(type));\n> +\telse\n> +\t\targv_array_push(&cmd.args, \"-p\");\n>  \targv_array_push(&cmd.args, sha1_to_hex(sha1));\n>  \tcmd.git_cmd = 1;\n>  \tcmd.out = fd;\n> @@ -217,7 +222,7 @@ static void export_object(const unsigned char *sha1, const char *filename)\n>   * The sha1 of the written object is returned via sha1.\n>   */\n>  static void import_object(unsigned char *sha1, enum object_type type,\n> -\t\t\t  const char *filename)\n> +\t\t\t  int raw, const char *filename)\n>  {\n>  \tint fd;\n>  \n> @@ -225,7 +230,7 @@ static void import_object(unsigned char *sha1, enum object_type type,\n>  \tif (fd < 0)\n>  \t\tdie_errno(\"unable to open %s for reading\", filename);\n>  \n> -\tif (type == OBJ_TREE) {\n> +\tif (!raw && type == OBJ_TREE) {\n>  \t\tconst char *argv[] = { \"mktree\", NULL };\n>  \t\tstruct child_process cmd = { argv };\n>  \t\tstruct strbuf result = STRBUF_INIT;\n> @@ -265,7 +270,7 @@ static void import_object(unsigned char *sha1, enum object_type type,\n>  \t */\n>  }\n>  \n> -static int edit_and_replace(const char *object_ref, int force)\n> +static int edit_and_replace(const char *object_ref, int force, int raw)\n>  {\n>  \tchar *tmpfile = git_pathdup(\"REPLACE_EDITOBJ\");\n>  \tenum object_type type;\n> @@ -281,10 +286,10 @@ static int edit_and_replace(const char *object_ref, int force)\n>  \n>  \tcheck_ref_valid(old, prev, ref, sizeof(ref), force);\n>  \n> -\texport_object(old, tmpfile);\n> +\texport_object(old, type, raw, tmpfile);\n>  \tif (launch_editor(tmpfile, NULL, NULL) < 0)\n>  \t\tdie(\"editing object file failed\");\n> -\timport_object(new, type, tmpfile);\n> +\timport_object(new, type, raw, tmpfile);\n>  \n>  \tfree(tmpfile);\n>  \n> @@ -297,6 +302,7 @@ static int edit_and_replace(const char *object_ref, int force)\n>  int cmd_replace(int argc, const char **argv, const char *prefix)\n>  {\n>  \tint force = 0;\n> +\tint raw = 0;\n>  \tconst char *format = NULL;\n>  \tenum {\n>  \t\tMODE_UNSPECIFIED = 0,\n> @@ -310,6 +316,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n>  \t\tOPT_CMDMODE('d', \"delete\", &cmdmode, N_(\"delete replace refs\"), MODE_DELETE),\n>  \t\tOPT_CMDMODE('e', \"edit\", &cmdmode, N_(\"edit existing object\"), MODE_EDIT),\n>  \t\tOPT_BOOL('f', \"force\", &force, N_(\"replace the ref if it exists\")),\n> +\t\tOPT_BOOL(0, \"raw\", &raw, N_(\"do not pretty-print contents for --edit\")),\n>  \t\tOPT_STRING(0, \"format\", &format, N_(\"format\"), N_(\"use this format\")),\n>  \t\tOPT_END()\n>  \t};\n> @@ -329,6 +336,10 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n>  \t\tusage_msg_opt(\"-f only makes sense when writing a replacement\",\n>  \t\t\t      git_replace_usage, options);\n>  \n> +\tif (raw && cmdmode != MODE_EDIT)\n> +\t\tusage_msg_opt(\"--raw only makes sense with --edit\",\n> +\t\t\t      git_replace_usage, options);\n> +\n>  \tswitch (cmdmode) {\n>  \tcase MODE_DELETE:\n>  \t\tif (argc < 1)\n> @@ -346,7 +357,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n>  \t\tif (argc != 1)\n>  \t\t\tusage_msg_opt(\"-e needs exactly one argument\",\n>  \t\t\t\t      git_replace_usage, options);\n> -\t\treturn edit_and_replace(argv[0], force);\n> +\t\treturn edit_and_replace(argv[0], force, raw);\n>  \n>  \tcase MODE_LIST:\n>  \t\tif (argc > 1)\n"},{"id":"245020","messageId":"20140626155530.GB10402@sigill.intra.peff.net","threadId":"36989","inReplyTo":"xmqqsims62m1.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 4/4] replace: add a --raw mode for --edit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-26T15:55:30Z","receivedAt":"2014-06-26T15:55:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 25, 2014 at 03:33:42PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > One of the purposes of \"git replace --edit\" is to help a\n> > user repair objects which are malformed or corrupted.\n> > Usually we pretty-print trees with \"ls-tree\", which is much\n> > easier to work with than the raw binary data.  However, some\n> > forms of corruption break the tree-walker, in which case our\n> > pretty-printing fails, rendering \"--edit\" useless for the\n> > user.\n> >\n> > This patch introduces a \"--raw\" option, which lets you edit\n> > the binary data in these instances.\n> >\n> > Signed-off-by: Jeff King <peff@peff.net>\n> > ---\n> \n> Hmmmmm, this feels almost like inviting accidents to make it easy\n> and limiting the attempt to only one shot at the \"transformation\"\n> step.\n> \n> Will queue, but we can do the same with \"cat-file $type >temp\", do\n> some transformation on \"temp\", doubly make sure what is in \"temp\" is\n> sensible and then finally \"hash-object -w -t $type temp\".  And it\n> makes me feel uneasy that the new feature seems to make it harder to\n> do the \"doubly make sure\" part.\n\nI do not think it is any worse than \"--edit\" is by itself. True, editing\nthe binary contents is hard, but we do check that the format is sane\nwhen we read it back in (using the same checks that hash-object does). I\nthink it would be nice to take that a step further and actually let\nhash-object (and \"replace --edit\") do the more rigorous fsck checks on\ncreated objects.\n\nI do still think even with those automated sanity checks that it makes\nsense to double-check the replacement manually. But I think that is one\nof the features of replace objects: you are not doing anything\npermanent, and can view the object in place. So not only can you examine\nthe object by \"git show $new_sha1\", you can see it in place as \"git log\n-p\", etc. And easily back it out with \"git replace -d\" (or fix it up\nagain with \"git replace --edit\") if need be.\n\n-Peff\n"}]}