{"thread":{"id":"31431","subject":"[PATCH 2/5] path.c: Don't discard the return value of vsnpath()","startedAt":"2012-09-04T17:27:54Z","lastAt":"2012-09-07T19:52:31Z","messageCount":3,"participants":["Ramsay Jones","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"198328","messageId":"50463A1A.9000801@ramsay1.demon.co.uk","threadId":"31431","inReplyTo":null,"subject":"[PATCH 2/5] path.c: Don't discard the return value of vsnpath()","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2012-09-04T17:27:54Z","receivedAt":"2012-09-04T17:27:54Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nThe git_snpath() and git_pathdup() functions both use the (static)\nfunction vsnpath() in their implementation. Also, they both discard\nthe return value of vsnpath(), which has the effect of ignoring the\nside effect of calling cleanup_path() in the non-error return path.\n\nIn order to ensure that the required cleanup happens, we use the\npointer returned by vsnpath(), rather than the buffer passed into\nvsnpath(), to derive the return value from git_snpath() and\ngit_pathdup().\n\nSigned-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n---\n path.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex 9eb5333..741ae77 100644\n--- a/path.c\n+++ b/path.c\n@@ -70,21 +70,22 @@ bad:\n \n char *git_snpath(char *buf, size_t n, const char *fmt, ...)\n {\n+\tchar *ret;\n \tva_list args;\n \tva_start(args, fmt);\n-\t(void)vsnpath(buf, n, fmt, args);\n+\tret = vsnpath(buf, n, fmt, args);\n \tva_end(args);\n-\treturn buf;\n+\treturn ret;\n }\n \n char *git_pathdup(const char *fmt, ...)\n {\n-\tchar path[PATH_MAX];\n+\tchar path[PATH_MAX], *ret;\n \tva_list args;\n \tva_start(args, fmt);\n-\t(void)vsnpath(path, sizeof(path), fmt, args);\n+\tret = vsnpath(path, sizeof(path), fmt, args);\n \tva_end(args);\n-\treturn xstrdup(path);\n+\treturn xstrdup(ret);\n }\n \n char *mkpathdup(const char *fmt, ...)\n-- \n1.7.12\n"},{"id":"198343","messageId":"7vipbt8q7r.fsf@alter.siamese.dyndns.org","threadId":"31431","inReplyTo":"50463A1A.9000801@ramsay1.demon.co.uk","subject":"Re: [PATCH 2/5] path.c: Don't discard the return value of vsnpath()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-04T20:53:28Z","receivedAt":"2012-09-04T20:53:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n\n> The git_snpath() and git_pathdup() functions both use the (static)\n> function vsnpath() in their implementation. Also, they both discard\n> the return value of vsnpath(), which has the effect of ignoring the\n> side effect of calling cleanup_path() in the non-error return path.\n>\n> In order to ensure that the required cleanup happens, we use the\n> pointer returned by vsnpath(), rather than the buffer passed into\n> vsnpath(), to derive the return value from git_snpath() and\n> git_pathdup().\n>\n> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n> ---\n>  path.c | 11 ++++++-----\n>  1 file changed, 6 insertions(+), 5 deletions(-)\n>\n> diff --git a/path.c b/path.c\n> index 9eb5333..741ae77 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -70,21 +70,22 @@ bad:\n>  \n>  char *git_snpath(char *buf, size_t n, const char *fmt, ...)\n>  {\n> +\tchar *ret;\n>  \tva_list args;\n>  \tva_start(args, fmt);\n> -\t(void)vsnpath(buf, n, fmt, args);\n> +\tret = vsnpath(buf, n, fmt, args);\n>  \tva_end(args);\n> -\treturn buf;\n> +\treturn ret;\n>  }\n>  \n>  char *git_pathdup(const char *fmt, ...)\n>  {\n> -\tchar path[PATH_MAX];\n> +\tchar path[PATH_MAX], *ret;\n>  \tva_list args;\n>  \tva_start(args, fmt);\n> -\t(void)vsnpath(path, sizeof(path), fmt, args);\n> +\tret = vsnpath(path, sizeof(path), fmt, args);\n>  \tva_end(args);\n> -\treturn xstrdup(path);\n> +\treturn xstrdup(ret);\n>  }\n>  \n>  char *mkpathdup(const char *fmt, ...)\n\ncleanup_path() is a quick-and-dirty hack that only deals with\nleading \".///\" (e.g. \"foo//bar\" is not reduced to \"foo/bar\"), and\nthe callers that allocate 4 bytes for buf to hold \"foo\" may not be\nable to fit it if for some reason there are some bytes that must be\ncleaned, e.g. \".///foo\".\n\nThe worst part of its use is that buf and ret could be different,\nwhich means you cannot safely do:\n\n\tchar *buf = xmalloc(...);\n        buf = git_snpath(buf, n, \"%s\", ...);\n        ... use buf ...\n        free(buf);\n\nbut instead have to do something like:\n\n\tchar *path, *buf = xmalloc(...);\n        path = git_snpath(buf, n, \"%s\", ...);\n        ... use path ...\n        free(buf);\n\nAnd this series goes in a direction of making more callers aware of\nthe twisted calling convention, which smells really bad.\n\nAs long as the callers are contained within this file, it is not\nmaking things _too_ bad, so...\n"},{"id":"198550","messageId":"504A507F.9030802@ramsay1.demon.co.uk","threadId":"31431","inReplyTo":"7vipbt8q7r.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/5] path.c: Don't discard the return value of vsnpath()","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2012-09-07T19:52:31Z","receivedAt":"2012-09-07T19:52:31Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Junio C Hamano wrote:\n> cleanup_path() is a quick-and-dirty hack that only deals with\n> leading \".///\" (e.g. \"foo//bar\" is not reduced to \"foo/bar\"), and\n> the callers that allocate 4 bytes for buf to hold \"foo\" may not be\n> able to fit it if for some reason there are some bytes that must be\n> cleaned, e.g. \".///foo\".\n> \n> The worst part of its use is that buf and ret could be different,\n> which means you cannot safely do:\n> \n> \tchar *buf = xmalloc(...);\n>         buf = git_snpath(buf, n, \"%s\", ...);\n>         ... use buf ...\n>         free(buf);\n\nHmm, this seems very unnatural to me; it would never have occurred to\nme to use anything other than a stack allocated buffer (or *maybe* a\nglobal buffer) when calling git_snpath().\n\nIn this situation (ie a dynamically allocated buffer used to hold the\nresult), why would you not use git_pathdup()? (which does not suffer\nthis problem.)\n\nI guess it does not matter what I find unnatural, ... :(\n\n> but instead have to do something like:\n> \n> \tchar *path, *buf = xmalloc(...);\n>         path = git_snpath(buf, n, \"%s\", ...);\n>         ... use path ...\n>         free(buf);\n> \n> And this series goes in a direction of making more callers aware of\n> the twisted calling convention, which smells really bad.\n\nNote that, prior to commit aba13e7c (\"git_pathdup: returns xstrdup-ed\ncopy of the formatted path\", 27-10-2008), git_snpath() was calling\ncleanup_path() in the non-error return path. I suspect that this\ncommit did not intend to \"fix the git_snpath() interface\" and only\ndid so by accident. (That's a guess, of course)\n\nHowever, I much prefer git_snpath(), git_pathdup() and git_path()\nreturn the same result string given the same inputs! :D\n\nATB,\nRamsay Jones\n"}]}