{"thread":{"id":"37758","subject":"[PATCH] object: make add_object_array_with_mode a static function","startedAt":"2014-10-18T23:19:07Z","lastAt":"2014-10-20T17:03:15Z","messageCount":5,"participants":["Ramsay Jones","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"250812","messageId":"5442F56B.8020205@ramsay1.demon.co.uk","threadId":"37758","inReplyTo":null,"subject":"[PATCH] object: make add_object_array_with_mode a static function","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-10-18T23:19:07Z","receivedAt":"2014-10-18T23:19:07Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nSigned-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n---\n\nHi Jeff,\n\nI noticed that your 'jk/prune-mtime' branch also removes the only\ncall to the add_object_array_with_mode() function outside of the\nobject.c file; specifically commit 75ac69fa (\"traverse_commit_list:\nsupport pending blobs/trees with paths\", 15-10-2014).\n\nThis patch (which was generated using the '--histogram' option to\nformat-patch), moves the function to before the definition of the\nadd_object_array() function (to avoid a forward declaration), and\nmakes it static.\n\nIf you need to re-roll this branch, could you please squash this\npatch into the above commit. (again, assuming you have no plans\nto add new external callers.)\n\n[If new external callers are very likely in the future (i.e. this\nfunction is an essential part of the object-array API), then it may\nwell not be worth doing this. (with perhaps a note in the commit\nmessage? - dunno). Similar comments apply to the previous 'add_object'\npatch as well!]\n\nThanks!\n\nATB,\nRamsay Jones\n\n\n object.c | 10 +++++-----\n object.h |  1 -\n 2 files changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex df86bdd..e1ef3f9 100644\n--- a/object.c\n+++ b/object.c\n@@ -339,16 +339,16 @@ void add_object_array_with_path(struct object *obj, const char *name,\n \tarray->nr = ++nr;\n }\n \n+static void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode)\n+{\n+\tadd_object_array_with_path(obj, name, array, mode, NULL);\n+}\n+\n void add_object_array(struct object *obj, const char *name, struct object_array *array)\n {\n \tadd_object_array_with_mode(obj, name, array, S_IFINVALID);\n }\n \n-void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode)\n-{\n-\tadd_object_array_with_path(obj, name, array, mode, NULL);\n-}\n-\n /*\n  * Free all memory associated with an entry; the result is\n  * in an unspecified state and should not be examined.\ndiff --git a/object.h b/object.h\nindex e5178a5..6416247 100644\n--- a/object.h\n+++ b/object.h\n@@ -114,7 +114,6 @@ int object_list_contains(struct object_list *list, struct object *obj);\n \n /* Object array handling .. */\n void add_object_array(struct object *obj, const char *name, struct object_array *array);\n-void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode);\n void add_object_array_with_path(struct object *obj, const char *name, struct object_array *array, unsigned mode, const char *path);\n \n typedef int (*object_array_each_func_t)(struct object_array_entry *, void *);\n-- \n2.1.0\n"},{"id":"250816","messageId":"20141019020319.GB17908@peff.net","threadId":"37758","inReplyTo":"5442F56B.8020205@ramsay1.demon.co.uk","subject":"Re: [PATCH] object: make add_object_array_with_mode a static function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-19T02:03:19Z","receivedAt":"2014-10-19T02:03:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 19, 2014 at 12:19:07AM +0100, Ramsay Jones wrote:\n\n> I noticed that your 'jk/prune-mtime' branch also removes the only\n> call to the add_object_array_with_mode() function outside of the\n> object.c file; specifically commit 75ac69fa (\"traverse_commit_list:\n> support pending blobs/trees with paths\", 15-10-2014).\n> \n> This patch (which was generated using the '--histogram' option to\n> format-patch), moves the function to before the definition of the\n> add_object_array() function (to avoid a forward declaration), and\n> makes it static.\n> \n> If you need to re-roll this branch, could you please squash this\n> patch into the above commit. (again, assuming you have no plans\n> to add new external callers.)\n\nThat seems reasonable. Because it's a code movement, I'd actually be\njust as happy with it as a separate patch, where it's more obvious what\nis going on.\n\n> [If new external callers are very likely in the future (i.e. this\n> function is an essential part of the object-array API), then it may\n> well not be worth doing this. (with perhaps a note in the commit\n> message? - dunno). Similar comments apply to the previous 'add_object'\n> patch as well!]\n\nI actually wondered while writing this series whether anyone actually\n_uses_ the mode in object_array (the new code I added sets it to the\nappropriate value to be on the safe side, but traverse_commit_list does\nnot actually care about it).\n\nDigging in the history, it looks like it is used for blob-to-blob diffs\n(see 01618a3, \"use mode of the tree in git-diff, if <tree>:<file> syntax\nis used\", 2007-04-22). And that still seems to be the case today. It's a\nshame we have to keep such complication around for one single case\n(especially because getting it wrong in other cases is likely to go\nunnoticed for years), but I think it would probably require major\nsurgery to extract it.\n\nI think we can take your patch a step further, though, like:\n\n-- >8 --\nSubject: [PATCH] drop add_object_array_with_mode\n\nThis is a thin compatibility wrapper around\nadd_pending_object_with_path. But the only caller is\nadd_object_array, which is itself just a thin compatibility\nwrapper. There are no external callers, so we can just\nremove this middle wrapper.\n\nNoticed-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI also wondered if add_pending_object_with_mode could get the same\ntreatment. But we _do_ use it in setup_revisions and friends when we\nparse something like \"HEAD:foo\". It would be trivial here to call\nadd_pending_object_with_path instead, and actually feed \"foo\". That\nwould mean that \"git rev-list --objects HEAD:foo\" reported the pathname\n\"foo\" alongside the object. Or if \"foo\" is a tree, all of its sub-parts\nwould be \"foo/whatever\" instead of just \"whatever\". That seems somewhat\nsensible to me, but I would be unsurprised if it broke some weird corner\ncase that is expecting paths to be relative to the given tree.\n\n object.c | 7 +------\n object.h | 1 -\n 2 files changed, 1 insertion(+), 7 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex df86bdd..23d6c96 100644\n--- a/object.c\n+++ b/object.c\n@@ -341,12 +341,7 @@ void add_object_array_with_path(struct object *obj, const char *name,\n \n void add_object_array(struct object *obj, const char *name, struct object_array *array)\n {\n-\tadd_object_array_with_mode(obj, name, array, S_IFINVALID);\n-}\n-\n-void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode)\n-{\n-\tadd_object_array_with_path(obj, name, array, mode, NULL);\n+\tadd_object_array_with_path(obj, name, array, S_IFINVALID, NULL);\n }\n \n /*\ndiff --git a/object.h b/object.h\nindex e5178a5..6416247 100644\n--- a/object.h\n+++ b/object.h\n@@ -114,7 +114,6 @@ int object_list_contains(struct object_list *list, struct object *obj);\n \n /* Object array handling .. */\n void add_object_array(struct object *obj, const char *name, struct object_array *array);\n-void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode);\n void add_object_array_with_path(struct object *obj, const char *name, struct object_array *array, unsigned mode, const char *path);\n \n typedef int (*object_array_each_func_t)(struct object_array_entry *, void *);\n-- \n2.1.2.596.g7379948\n"},{"id":"250822","messageId":"544390AA.5000406@ramsay1.demon.co.uk","threadId":"37758","inReplyTo":"20141019020319.GB17908@peff.net","subject":"Re: [PATCH] object: make add_object_array_with_mode a static function","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-10-19T10:21:30Z","receivedAt":"2014-10-19T10:21:30Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 19/10/14 03:03, Jeff King wrote:\n> On Sun, Oct 19, 2014 at 12:19:07AM +0100, Ramsay Jones wrote:\n> \n\n[snip]\n\n> I actually wondered while writing this series whether anyone actually\n> _uses_ the mode in object_array (the new code I added sets it to the\n> appropriate value to be on the safe side, but traverse_commit_list does\n> not actually care about it).\n> \n> Digging in the history, it looks like it is used for blob-to-blob diffs\n> (see 01618a3, \"use mode of the tree in git-diff, if <tree>:<file> syntax\n> is used\", 2007-04-22). And that still seems to be the case today. It's a\n> shame we have to keep such complication around for one single case\n> (especially because getting it wrong in other cases is likely to go\n> unnoticed for years), but I think it would probably require major\n> surgery to extract it.\n> \n> I think we can take your patch a step further, though, like:\n\nYes, I'm always in favour of removing unused code!  :-D\n\n> \n> -- >8 --\n> Subject: [PATCH] drop add_object_array_with_mode\n> \n> This is a thin compatibility wrapper around\n> add_pending_object_with_path. But the only caller is\n> add_object_array, which is itself just a thin compatibility\n> wrapper. There are no external callers, so we can just\n> remove this middle wrapper.\n> \n> Noticed-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I also wondered if add_pending_object_with_mode could get the same\n> treatment. But we _do_ use it in setup_revisions and friends when we\n> parse something like \"HEAD:foo\". It would be trivial here to call\n> add_pending_object_with_path instead, and actually feed \"foo\". That\n> would mean that \"git rev-list --objects HEAD:foo\" reported the pathname\n> \"foo\" alongside the object. Or if \"foo\" is a tree, all of its sub-parts\n> would be \"foo/whatever\" instead of just \"whatever\". That seems somewhat\n> sensible to me, but I would be unsurprised if it broke some weird corner\n> case that is expecting paths to be relative to the given tree.\n> \n>  object.c | 7 +------\n>  object.h | 1 -\n>  2 files changed, 1 insertion(+), 7 deletions(-)\n> \n> diff --git a/object.c b/object.c\n> index df86bdd..23d6c96 100644\n> --- a/object.c\n> +++ b/object.c\n> @@ -341,12 +341,7 @@ void add_object_array_with_path(struct object *obj, const char *name,\n>  \n>  void add_object_array(struct object *obj, const char *name, struct object_array *array)\n>  {\n> -\tadd_object_array_with_mode(obj, name, array, S_IFINVALID);\n> -}\n> -\n> -void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode)\n> -{\n> -\tadd_object_array_with_path(obj, name, array, mode, NULL);\n> +\tadd_object_array_with_path(obj, name, array, S_IFINVALID, NULL);\n>  }\n>  \n>  /*\n> diff --git a/object.h b/object.h\n> index e5178a5..6416247 100644\n> --- a/object.h\n> +++ b/object.h\n> @@ -114,7 +114,6 @@ int object_list_contains(struct object_list *list, struct object *obj);\n>  \n>  /* Object array handling .. */\n>  void add_object_array(struct object *obj, const char *name, struct object_array *array);\n> -void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode);\n>  void add_object_array_with_path(struct object *obj, const char *name, struct object_array *array, unsigned mode, const char *path);\n>  \n>  typedef int (*object_array_each_func_t)(struct object_array_entry *, void *);\n> \n\nYep, this is a better patch.\n\nThanks!\n\nATB,\nRamsay Jones\n"},{"id":"250858","messageId":"xmqqa94qr9tn.fsf@gitster.dls.corp.google.com","threadId":"37758","inReplyTo":"20141019020319.GB17908@peff.net","subject":"Re: [PATCH] object: make add_object_array_with_mode a static function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-20T16:21:24Z","receivedAt":"2014-10-20T16:21:24Z","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> I think we can take your patch a step further, though, like:\n>\n> -- >8 --\n> Subject: [PATCH] drop add_object_array_with_mode\n> ...\n\nThanks.  I think I picked up all incrementals in this thread, but\nplease holler if I missed anything.\n"},{"id":"250861","messageId":"20141020170314.GA20331@peff.net","threadId":"37758","inReplyTo":"xmqqa94qr9tn.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] object: make add_object_array_with_mode a static function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-20T17:03:15Z","receivedAt":"2014-10-20T17:03:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 20, 2014 at 09:21:24AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I think we can take your patch a step further, though, like:\n> >\n> > -- >8 --\n> > Subject: [PATCH] drop add_object_array_with_mode\n> > ...\n> \n> Thanks.  I think I picked up all incrementals in this thread, but\n> please holler if I missed anything.\n\nI just checked it (using Thomas's excellent \"tbdiff\" tool) and you have\neverything. Thanks, and sorry for feeding it so piecemeal. :)\n\n-Peff\n"}]}