{"thread":{"id":"43183","subject":"Re: [PATCH] Improve git-prune -n output","startedAt":"2006-10-26T10:38:24Z","lastAt":"2006-10-28T20:37:56Z","messageCount":10,"participants":["Junio C Hamano","Andy Parkins","Petr Baudis","Jakub Narebski","Andy Whitcroft"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"294461","messageId":"200610261138.24761.andyparkins@gmail.com","threadId":"43183","inReplyTo":null,"subject":"[PATCH] Improve git-prune -n output","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2006-10-26T10:38:24Z","receivedAt":"2006-10-26T10:38:24Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"prune_object() in show_only mode would previously just show the path to the\nobject that would be deleted. The path the object is stored in shouldn't be\nshown to users, they only know about sha1 identifiers so show that instead.\n\nFurther, the sha1 alone isn't that useful for examining what is going to be\ndeleted. This patch also adds the object type to the output, which makes it\neasy to pick out, say, the commits and use git-show to display them.\nSigned-off-by: Andy Parkins \n---\n builtin-prune.c | 7 ++++++-\n 1 files changed, 6 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-prune.c b/builtin-prune.c\nindex 7290e6d..e3bcf5f 100644\n--- a/builtin-prune.c\n+++ b/builtin-prune.c\n@@ -16,8 +16,13 @@ static struct rev_info revs;\n \n static int prune_object(char *path, const char *filename, const unsigned char *sha1)\n {\n+\tchar type[20];\n+\n \tif (show_only) {\n-\t\tprintf(\"would prune %s/%s\\n\", path, filename);\n+\t\tif (sha1_object_info(sha1, type, NULL)) {\n+\t\t\tstrcpy( type, \"unknown type\" );\n+\t\t}\n+\t\tprintf(\"would prune %s %s\\n\", sha1_to_hex( sha1 ), type );\n \t\treturn 0;\n \t}\n \tunlink(mkpath(\"%s/%s\", path, filename));\n-- \n1.4.2.3\n"},{"id":"295167","messageId":"7vlkn3kn65.fsf@assigned-by-dhcp.cox.net","threadId":"43183","inReplyTo":"200610261138.24761.andyparkins@gmail.com","subject":"Re: [PATCH] Improve git-prune -n output","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-10-26T17:21:54Z","receivedAt":"2006-10-26T17:21:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andy Parkins writes:\n\n> prune_object() in show_only mode would previously just show the path to the\n> object that would be deleted. The path the object is stored in shouldn't be\n> shown to users, they only know about sha1 identifiers so show that instead.\n\nI do not have an objection to what this does, except I wonder if\nsomebody's script relies on parsing this output already.\n\n> Further, the sha1 alone isn't that useful for examining what is going to be\n> deleted. This patch also adds the object type to the output, which makes it\n> easy to pick out, say, the commits and use git-show to display them.\n> Signed-off-by: Andy Parkins \n\nIt is customary to have one empty line before the S-o-b: line.\n\n> ---\n> builtin-prune.c | 7 ++++++-\n> 1 files changed, 6 insertions(+), 1 deletions(-)\n>\n> diff --git a/builtin-prune.c b/builtin-prune.c\n> index 7290e6d..e3bcf5f 100644\n> --- a/builtin-prune.c\n> +++ b/builtin-prune.c\n> @@ -16,8 +16,13 @@ static struct rev_info revs;\n> \n> static int prune_object(char *path, const char *filename, const unsigned char *sha1)\n> {\n> +\tchar type[20];\n> +\n> \tif (show_only) {\n> -\t\tprintf(\"would prune %s/%s\\n\", path, filename);\n> +\t\tif (sha1_object_info(sha1, type, NULL)) {\n> +\t\t\tstrcpy( type, \"unknown type\" );\n> +\t\t}\n> +\t\tprintf(\"would prune %s %s\\n\", sha1_to_hex( sha1 ), type );\n\nStyle.\n\n - We do not leave blank inside () pairs.\n - We do not enclose a single statement in {}.\n\nPerhaps an extra variable of type \"char *\" that initially point\nat type[] but later points at the \"unknown\" might be more\nefficient, but that being on the error path I do not think\nsaving one strcpy() that way is worth it.\n\nSo, just the style and a slight worry that this _might_ break\npeople's scripts.\n\nObjections?\n"},{"id":"297517","messageId":"4540F16F.4060604@shadowen.org","threadId":"43183","inReplyTo":"200610261138.24761.andyparkins@gmail.com","subject":"Re: [PATCH] Improve git-prune -n output","fromName":"Andy Whitcroft","fromEmail":"apw@shadowen.org","sentAt":"2006-10-26T17:33:35Z","receivedAt":"2006-10-26T17:33:35Z","isPatch":true,"sender":{"key":"apw@shadowen.org","avatar":"https://gravatar.com/avatar/d3088262854661a913ef35cc40fedcc270142d4461791142bc1ea0b2a4e2e147?d=mp&s=160"},"body":"Andy Parkins wrote:\n> prune_object() in show_only mode would previously just show the path to the\n> object that would be deleted. The path the object is stored in shouldn't be\n> shown to users, they only know about sha1 identifiers so show that instead.\n> \n> Further, the sha1 alone isn't that useful for examining what is going to be\n> deleted. This patch also adds the object type to the output, which makes it\n> easy to pick out, say, the commits and use git-show to display them.\n> Signed-off-by: Andy Parkins \n> ---\n> builtin-prune.c | 7 ++++++-\n> 1 files changed, 6 insertions(+), 1 deletions(-)\n> \n> diff --git a/builtin-prune.c b/builtin-prune.c\n> index 7290e6d..e3bcf5f 100644\n> --- a/builtin-prune.c\n> +++ b/builtin-prune.c\n> @@ -16,8 +16,13 @@ static struct rev_info revs;\n> \n> static int prune_object(char *path, const char *filename, const unsigned char *sha1)\n> {\n> +\tchar type[20];\n> +\n> \tif (show_only) {\n> -\t\tprintf(\"would prune %s/%s\\n\", path, filename);\n> +\t\tif (sha1_object_info(sha1, type, NULL)) {\n> +\t\t\tstrcpy( type, \"unknown type\" );\n> +\t\t}\n> +\t\tprintf(\"would prune %s %s\\n\", sha1_to_hex( sha1 ), type );\n> \t\treturn 0;\n> \t}\n> \tunlink(mkpath(\"%s/%s\", path, filename));\n\nIf we are changing the format would it not make more sense to be in the\nsame order as the tool that lets you use it?\n\nI thought that was git cat-file \n\n"},{"id":"294171","messageId":"200610261837.31356.andyparkins@gmail.com","threadId":"43183","inReplyTo":"7vlkn3kn65.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Improve git-prune -n output","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2006-10-26T17:37:23Z","receivedAt":"2006-10-26T17:37:23Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"On Thursday 2006, October 26 18:21, Junio C Hamano wrote:\n\n> I do not have an objection to what this does, except I wonder if\n> somebody's script relies on parsing this output already.\n\nI did think of that, but the existing output looked pretty non-useful for easy \nparsing because of the fact that filenames where being printed instead of \nhashes. If I were given free reign I think I'd want to drop the \"would \nprune\" message as well.\n\n> It is customary to have one empty line before the S-o-b: line.\n\nI didn't do it. Git did :-) I'll keep an eye out for it from now on. \nApologies (as usual)\n\n\nAndy\n-- \nDr Andrew Parkins, M Eng (Hons), AMIEE\n"},{"id":"295860","messageId":"20061027014501.GU20017@pasky.or.cz","threadId":"43183","inReplyTo":"4540F16F.4060604@shadowen.org","subject":"Re: [PATCH] Improve git-prune -n output","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2006-10-27T01:45:01Z","receivedAt":"2006-10-27T01:45:01Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"Junio wrote:\n> - We do not enclose a single statement in {}.\n\nI don't think we need to enforce this rule in particular too rigorously\nsince in comparison with other style slips, it doesn't put you off by\nunusual styling when you are reading the code, its only effect at all is\nslight vertical space waste (but it isn't a pure loss anyway, for that\nmatter).\n\n\nDear diary, on Thu, Oct 26, 2006 at 07:33:35PM CEST, I got a letter\nwhere Andy Whitcroft said that...\n> Andy Parkins wrote:\n> > prune_object() in show_only mode would previously just show the path to the\n> > object that would be deleted. The path the object is stored in shouldn't be\n> > shown to users, they only know about sha1 identifiers so show that instead.\n> > \n> > Further, the sha1 alone isn't that useful for examining what is going to be\n> > deleted. This patch also adds the object type to the output, which makes it\n> > easy to pick out, say, the commits and use git-show to display them.\n> > Signed-off-by: Andy Parkins \n> > ---\n> > builtin-prune.c | 7 ++++++-\n> > 1 files changed, 6 insertions(+), 1 deletions(-)\n> > \n> > diff --git a/builtin-prune.c b/builtin-prune.c\n> > index 7290e6d..e3bcf5f 100644\n> > --- a/builtin-prune.c\n> > +++ b/builtin-prune.c\n> > @@ -16,8 +16,13 @@ static struct rev_info revs;\n> > \n> > static int prune_object(char *path, const char *filename, const unsigned char *sha1)\n> > {\n> > +\tchar type[20];\n> > +\n> > \tif (show_only) {\n> > -\t\tprintf(\"would prune %s/%s\\n\", path, filename);\n> > +\t\tif (sha1_object_info(sha1, type, NULL)) {\n> > +\t\t\tstrcpy( type, \"unknown type\" );\n> > +\t\t}\n> > +\t\tprintf(\"would prune %s %s\\n\", sha1_to_hex( sha1 ), type );\n> > \t\treturn 0;\n> > \t}\n> > \tunlink(mkpath(\"%s/%s\", path, filename));\n> \n> If we are changing the format would it not make more sense to be in the\n> same order as the tool that lets you use it?\n> \n> I thought that was git cat-file \n\nSeconded and please use just 'unknown' for the type string, so that you\ncan split the lines on spaces in scripts.\n\nBesides, in what situation do we actually get the unknown output?\n\nThanks,\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nStuff: http://pasky.or.cz/\n#!/bin/perl -sp0777i If we are changing the format would it not make more sense to be in the\n> same order as the tool that lets you use it?\n>\n> I thought that was git cat-file \n\nThe only problem with that is that the type string is of variable width, while \nthe hash is fixed; hence is more visually appealing than \n.\n\nDoes it really matter anyway? Some sort of processing would have to happen \nbefore using git-prune output as git-cat-file parameters...\n\ngit-prune -n |\nwhile read hash type\ndo\n git-cat-file $type $hash\ndone\n\n\n\nAndy\n\n-- \nDr Andy Parkins, M Eng (hons), MIEE\n"},{"id":"298627","messageId":"200610270937.11368.andyparkins@gmail.com","threadId":"43183","inReplyTo":"7vlkn3kn65.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH] Improve git-prune -n output","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2006-10-27T08:37:11Z","receivedAt":"2006-10-27T08:37:11Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"prune_object() in show_only mode would previously just show the path to the\nobject that would be deleted. The path the object is stored in shouldn't be\nshown to users, they only know about sha1 identifiers so show that instead.\n\nFurther, the sha1 alone isn't that useful for examining what is going to be\ndeleted. This patch also adds the object type to the output, which makes it\neasy to pick out, say, the commits and use git-show to display them.\n\nSigned-off-by: Andy Parkins \n---\nFixes issues from...\n\nJunio C Hamano\n Do not leave blank inside () pairs.\n Do not enclose a single statement in {}.\n No longer uses strcpy()\n\nPetr Baudis\n \"unknown type\" is now simply \"unknown\"\n\nAndy Parkins\n No longer outputs \"would prune\"\n\n builtin-prune.c | 8 +++++++-\n 1 files changed, 7 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-prune.c b/builtin-prune.c\nindex 7290e6d..b8b2d05 100644\n--- a/builtin-prune.c\n+++ b/builtin-prune.c\n@@ -16,8 +16,14 @@ static struct rev_info revs;\n \n static int prune_object(char *path, const char *filename, const unsigned char \n*sha1)\n {\n+\tchar buf[20];\n+\tconst char *type;\n+\n \tif (show_only) {\n-\t\tprintf(\"would prune %s/%s\\n\", path, filename);\n+\t\ttype = buf;\n+\t\tif (sha1_object_info(sha1, type, NULL))\n+\t\t\ttype = \"unknown\";\n+\t\tprintf(\"%s %s\\n\", sha1_to_hex(sha1), type );\n \t\treturn 0;\n \t}\n \tunlink(mkpath(\"%s/%s\", path, filename));\n-- \n1.4.3.3.g5bca6\n"},{"id":"293988","messageId":"7vslhagm6n.fsf@assigned-by-dhcp.cox.net","threadId":"43183","inReplyTo":"200610270920.06064.andyparkins@gmail.com","subject":"Re: [PATCH] Improve git-prune -n output","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-10-27T09:09:04Z","receivedAt":"2006-10-27T09:09:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andy Parkins writes:\n\n> The only problem with that is that the type string is of\n> variable width, while the hash is fixed; hence \n> is more visually appealing than .\n>\n> Does it really matter anyway?\n\nI would rather question if the alignment really matters. You\ncould always printf(\"%10s %s\\n\", type, hex) if you really cared,\nthough.\n\n> ... Some sort of processing would have to happen \n> before using git-prune output as git-cat-file parameters...\n>\n> git-prune -n |\n> while read hash type\n> do\n> git-cat-file $type $hash\n> done\n\nwhich is probably not the way in which the command output is\ngoing to be used. I'd expect a sequence more like:\n\n\t$ git prune -n\n\tblob deadbeefdeadbeef...\n commit deadbeefdeadbeef...\n ...\n\nThe user would not just pipe all of the output to cat-file, but\nwould pick ones that are meaningful to be cat'ed (commit and\nperhaps blob but definitely not tree). And the cue to pick\nwhich ones would likely to be the object type not hash, so in\nthat sense type-then-hash might be easier to read.\n\nHaving said that, a potentially useful application would be \"a\nbackup before prune\" which can be done more easily if you use\nhash-then-type: \"git prune -n | git pack-objects backup\"\n\nSo while I do not think it matters that much, I slightly prefer\nhash-then-type better than type-then-hash.\n\n\n\n\n"},{"id":"295928","messageId":"ehvnob$tmj$1@sea.gmane.org","threadId":"43183","inReplyTo":"200610270937.11368.andyparkins@gmail.com","subject":"Re: [PATCH] Improve git-prune -n output","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-10-28T14:00:28Z","receivedAt":"2006-10-28T14:00:28Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Andy Parkins wrote:\n\n> prune_object() in show_only mode would previously just show the path to the\n> object that would be deleted. The path the object is stored in shouldn't be\n> shown to users, they only know about sha1 identifiers so show that instead.\n\nThis allowed to 'rm -f' [some] of what git-prune -n shows.\n\nDid anybody used that? I don't know...\n\n> Further, the sha1 alone isn't that useful for examining what is going to be\n> deleted. This patch also adds the object type to the output, which makes it\n> easy to pick out, say, the commits and use git-show to display them.\n\nWell, you can always use git-lost-found. Perhaps it would be better to add\n-n option to git-lost-found to not create refs in $GIT_DIR/lost-found/\ndirectory.\n\nSorry for being late with that suggestion.\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n\n"},{"id":"295351","messageId":"7vejss6usb.fsf@assigned-by-dhcp.cox.net","threadId":"43183","inReplyTo":"ehvnob$tmj$1@sea.gmane.org","subject":"Re: [PATCH] Improve git-prune -n output","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-10-28T20:37:56Z","receivedAt":"2006-10-28T20:37:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> Andy Parkins wrote:\n>\n>> prune_object() in show_only mode would previously just show the path to the\n>> object that would be deleted. The path the object is stored in shouldn't be\n>> shown to users, they only know about sha1 identifiers so show that instead.\n>\n> This allowed to 'rm -f' [some] of what git-prune -n shows.\n>\n> Did anybody used that? I don't know...\n\nYes and probably no because the output did not have enough clues\nto help picking which ones to remove. If the command said\nsomething like this:\n\n\t.git/objetcts/11/feed0... ;# commit 2006-08-28: WIP for\t\"foo\"\n\nit might have been a good clue and would have made sense, though.\n"}]}