{"thread":{"id":"33523","subject":"[PATCH] help.c: add a compatibility comment to cmd_version()","startedAt":"2013-04-16T20:33:25Z","lastAt":"2013-04-18T23:00:07Z","messageCount":7,"participants":["David Aguilar","Junio C Hamano","Philip Oakley"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"214533","messageId":"1366144405-61438-1-git-send-email-davvid@gmail.com","threadId":"33523","inReplyTo":null,"subject":"[PATCH] help.c: add a compatibility comment to cmd_version()","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-04-16T20:33:25Z","receivedAt":"2013-04-16T20:33:25Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"External projects have been known to parse the output of\n\"git version\". Help prevent future authors from changing\nits format by adding a comment to its implementation.\n\nSigned-off-by: David Aguilar \n---\n help.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/help.c b/help.c\nindex 1dfa0b0..02ba043 100644\n--- a/help.c\n+++ b/help.c\n@@ -397,6 +397,10 @@ const char *help_unknown_cmd(const char *cmd)\n \n int cmd_version(int argc, const char **argv, const char *prefix)\n {\n+\t/*\n+\t * The format of this string should be kept stable for compatibility\n+\t * with external projects that rely on the output of \"git version\".\n+\t */\n \tprintf(\"git version %s\\n\", git_version_string);\n \treturn 0;\n }\n-- \n1.8.2.1.652.ge104b5e\n"},{"id":"214537","messageId":"7vsj2qjjal.fsf@alter.siamese.dyndns.org","threadId":"33523","inReplyTo":"1366144405-61438-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH] help.c: add a compatibility comment to cmd_version()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-16T20:46:10Z","receivedAt":"2013-04-16T20:46:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> External projects have been known to parse the output of\n> \"git version\". Help prevent future authors from changing\n> its format by adding a comment to its implementation.\n>\n> Signed-off-by: David Aguilar \n> ---\n> help.c | 4 ++++\n> 1 file changed, 4 insertions(+)\n>\n> diff --git a/help.c b/help.c\n> index 1dfa0b0..02ba043 100644\n> --- a/help.c\n> +++ b/help.c\n> @@ -397,6 +397,10 @@ const char *help_unknown_cmd(const char *cmd)\n> \n> int cmd_version(int argc, const char **argv, const char *prefix)\n> {\n> +\t/*\n> +\t * The format of this string should be kept stable for compatibility\n> +\t * with external projects that rely on the output of \"git version\".\n> +\t */\n> \tprintf(\"git version %s\\n\", git_version_string);\n> \treturn 0;\n> }\n\nWill queue; thanks.\n"},{"id":"214548","messageId":"3D6BDB8830FC44A38B1AC508488049C0@PhilipOakley","threadId":"33523","inReplyTo":"1366144405-61438-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH] help.c: add a compatibility comment to cmd_version()","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2013-04-16T21:59:56Z","receivedAt":"2013-04-16T21:59:56Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"David Aguilar\" \nSent: Tuesday, April 16, 2013 9:33 PM\n> External projects have been known to parse the output of\n> \"git version\". Help prevent future authors from changing\n> its format by adding a comment to its implementation.\n>\n> Signed-off-by: David Aguilar \n> ---\n> help.c | 4 ++++\n> 1 file changed, 4 insertions(+)\n>\n> diff --git a/help.c b/help.c\n> index 1dfa0b0..02ba043 100644\n> --- a/help.c\n> +++ b/help.c\n> @@ -397,6 +397,10 @@ const char *help_unknown_cmd(const char *cmd)\n>\n> int cmd_version(int argc, const char **argv, const char *prefix)\n> {\n> + /*\n> + * The format of this string should be kept stable for compatibility\n> + * with external projects that rely on the output of \"git version\".\n\nShouldn't the expected format of our known external project also be \nindicated?\nOr mention \"such as git gui\"?\n\n> + */\n> printf(\"git version %s\\n\", git_version_string);\n> return 0;\n> }\n> -- \n> 1.8.2.1.652.ge104b5e\n>\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>\n>\n> -----\n> No virus found in this message.\n> Checked by AVG - www.avg.com\n> Version: 2013.0.3272 / Virus Database: 3162/6248 - Release Date: \n> 04/16/13\n> \n"},{"id":"214553","messageId":"7vvc7mhzok.fsf@alter.siamese.dyndns.org","threadId":"33523","inReplyTo":"3D6BDB8830FC44A38B1AC508488049C0@PhilipOakley","subject":"Re: [PATCH] help.c: add a compatibility comment to cmd_version()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-16T22:35:07Z","receivedAt":"2013-04-16T22:35:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philip Oakley\" writes:\n\n>> int cmd_version(int argc, const char **argv, const char *prefix)\n>> {\n>> + /*\n>> + * The format of this string should be kept stable for compatibility\n>> + * with external projects that rely on the output of \"git version\".\n>\n> Shouldn't the expected format of our known external project also be\n> indicated?\n> ...\n>> printf(\"git version %s\\n\", git_version_string);\n\nIt is fairly clear from the commented code that the only guarantee\nthey will be getting is that it begins with a string \"git version \".\ngit_version_string[] has anything of the builder's choice. I am not\nsure if there anything more to \"indicate\".\n\nReally, if you run\n\n\t$ git version\n\nand you get \"Git Source Code Management Version 3.56\" from its\noutput, it is likely that the version is very different from what\nyou know, and you should not assume any your assumption would hold.\n\n> Or mention \"such as git gui\"?\n\nI do not see what it would buy us. It is not like it is OK as long\nas we upadte Git gui at the same time.\n"},{"id":"214652","messageId":"EAD161814DB34F5AAC61A9EF06873314@PhilipOakley","threadId":"33523","inReplyTo":"7vvc7mhzok.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] help.c: add a compatibility comment to cmd_version()","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2013-04-17T23:03:40Z","receivedAt":"2013-04-17T23:03:40Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Junio C Hamano\" \nSent: Tuesday, April 16, 2013 11:35 PM\n> \"Philip Oakley\" writes:\n>\n>>> int cmd_version(int argc, const char **argv, const char *prefix)\n>>> {\n>>> + /*\n>>> + * The format of this string should be kept stable for \n>>> compatibility\n>>> + * with external projects that rely on the output of \"git version\".\n\nThis 'tantalizes without telling', the same complaint that is given \noften for over-succinct commit messages.\nHow about\n * E.g. git gui uses the extended regular expression \"^git version \n[1-9]+(\\.[0-9]+)+.*\"\n * to check for an acceptable version string.\n\nThe ERE is from git-gui.sh:L958.\n\n>>\n>> Shouldn't the expected format of our known external project also be\n>> indicated?\n>> ...\n>>> printf(\"git version %s\\n\", git_version_string);\n>\n> It is fairly clear from the commented code that the only guarantee\n> they will be getting is that it begins with a string \"git version \".\n\nI read the code the opposite way. It says \"This is the code to be \nchanged\" if you (anyone doing tweaks) want a special version string.\n\n> git_version_string[] has anything of the builder's choice. I am not\n> sure if there anything more to \"indicate\".\n>\n> Really, if you run\n>\n> $ git version\n>\n> and you get \"Git Source Code Management Version 3.56\" from its\n> output, it is likely that the version is very different from what\n> you know, and you should not assume any your assumption would hold.\n\nAgain I am reading this from the opposite side. There would be no \nassumption of difference if it _passed_ the test scripts. Unfortunately \nit wouldn't be friendly to other tools (like git gui). Hence my \nsuggestion of the basic test that a \"passing\" git would produce a \nconsistent version string. It still allows the supplier's suffixes to be \nadded, but not the prefixes. The test suite tests that git is 'good \nenough for most usages and picks up regressions. No?\n\nObvious inconsistent special versions would fail in many other places.\n>\n>> Or mention \"such as git gui\"?\n>\n> I do not see what it would buy us. It is not like it is OK as long\n> as we upadte Git gui at the same time.\n\nPhilip \n"},{"id":"214662","messageId":"7vehe8d7bq.fsf@alter.siamese.dyndns.org","threadId":"33523","inReplyTo":"EAD161814DB34F5AAC61A9EF06873314@PhilipOakley","subject":"Re: [PATCH] help.c: add a compatibility comment to cmd_version()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-18T00:13:29Z","receivedAt":"2013-04-18T00:13:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philip Oakley\" writes:\n\n> How about\n> * E.g. git gui uses the extended regular expression \"^git version\n> [1-9]+(\\.[0-9]+)+.*\"\n> * to check for an acceptable version string.\n>\n> The ERE is from git-gui.sh:L958.\n\nThat is exactly the kind of guarantee we do _not_ want to give.\n\n> ... Hence my suggestion of the basic test that a \"passing\" git\n> would produce a consistent version string.\n\nI have been assuming that you are trying to avoid an exchange like\nthis one we had in the past:\n\n http://thread.gmane.org/gmane.comp.version-control.git/216923/focus=217007\n\nI also have been assuming that you are pushing to limit the possible\nversioning scheme, but I do not know what that extra limitation\nwould achieve in the real world.\n\nBy sticking to the pattern \"git gui\" happens to use, \"git gui\" may\nbe able to guess that the next version of Git says it is verison\n\"1.8.3\". That is the _only_ thing your \"test\" buys.\n\nBut having parsed the \"1.8.3\" substring out of it, \"git gui\" does\nnot know anything about what 1.8.3 gives it. As far as it is\nconcerned, it is a version whose \"git version\" output it has never\nseen (unless it has been kept up to date with the development of\nGit).\n\nBy matching against \"git version [1-9]+(\\.[0-9]+)+\", it is accepting\nthat future versions may break assumptions it makes for some known\nversions (which is warranted) and all future versions (which is\nunwarranted) of Git. Maybe the version 2.0 of Git adds all changes\nin the directory \"d\", including removals, when you say \"git add d\",\nbut it may have assumed that with Git version 1.5.0 or newer, saying\n\"git add d\" would result in added and modified inside that directory\ngetting updated in the index, but paths removed from the working\ntree will stay in the index.\n\nThe only thing the scripts that read from the output of \"git\nversion\" can reliably tell is if it is interacting with a version of\nGit it knows about. If it made any unwarranted assumption on the\nversions it hasn't seen, it has to be fixed in \"git gui\" _anyway_.\n\nOf course, we would not change the output of \"git version\"\nwilly-nilly without good reason, but that is a different topic.\n\nSo I do not know what you want to achieve in the real world with \nthat extra limitation on the \"git version\" output format.\n\nMaybe you are proposing something else. I dunno.\n"},{"id":"214769","messageId":"046DF03A85924057935F3EB1064DC27B@PhilipOakley","threadId":"33523","inReplyTo":"7vehe8d7bq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] help.c: add a compatibility comment to cmd_version()","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2013-04-18T23:00:07Z","receivedAt":"2013-04-18T23:00:07Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Junio C Hamano\" \nSent: Thursday, April 18, 2013 1:13 AM\n> \"Philip Oakley\" writes:\n>\n>> How about\n>> * E.g. git gui uses the extended regular expression \"^git version\n>> [1-9]+(\\.[0-9]+)+.*\"\n>> * to check for an acceptable version string.\n>>\n>> The ERE is from git-gui.sh:L958.\n>\n> That is exactly the kind of guarantee we do _not_ want to give.\n>\n\nSo you are trying to avoid giving _any_ \"guarantee\" about what visible \nmanifestation a user may obtain about a system that passes the test \nsuite we have set out. I was hoping we could give a basic minimum \nindication of the expected style of the version string for a git which \n*passes* our tests. But like you say, it doesn't form a real guarantee - \ncaveat emptor still applies.\n\n>> ... Hence my suggestion of the basic test that a \"passing\" git\n>> would produce a consistent version string.\n>\n> I have been assuming that you are trying to avoid an exchange like\n> this one we had in the past:\n>\n> \n> http://thread.gmane.org/gmane.comp.version-control.git/216923/focus=217007\n>\nIn a sense yes. As David noted, others do attempt to trust us via our \ncurrent version string format. I thought it worthwhile (given my earlier \n'mistake' in 216923/focus=216925) to suggest such a basic indication of \nour current string style.\n\n> I also have been assuming that you are pushing to limit the possible\n> versioning scheme, but I do not know what that extra limitation\n> would achieve in the real world.\n>\n> By sticking to the pattern \"git gui\" happens to use, \"git gui\" may\n> be able to guess that the next version of Git says it is verison\n> \"1.8.3\". That is the _only_ thing your \"test\" buys.\n>\n> But having parsed the \"1.8.3\" substring out of it, \"git gui\" does\n> not know anything about what 1.8.3 gives it. As far as it is\n> concerned, it is a version whose \"git version\" output it has never\n> seen (unless it has been kept up to date with the development of\n> Git).\n\nYou are focusssing on the wrong side of issue (from my viewpoint).\nIf my earlier patch had gone in, it would have passsed our tests but not \nplayed nicely with the community tools. That would have been IMHO a \nregression, so from my viewpoint I believed it was worth having a test \nto avoid such a 'regression'.\n\n>\n> By matching against \"git version [1-9]+(\\.[0-9]+)+\", it is accepting\n> that future versions may break assumptions it makes for some known\n> versions (which is warranted) and all future versions (which is\n> unwarranted) of Git. Maybe the version 2.0 of Git adds all changes\n> in the directory \"d\", including removals, when you say \"git add d\",\n> but it may have assumed that with Git version 1.5.0 or newer, saying\n> \"git add d\" would result in added and modified inside that directory\n> getting updated in the index, but paths removed from the working\n> tree will stay in the index.\n\nIf it was a gross incompatibility then (a) we are likley to be \nsignalling it for a while, and (b) other tools would need updating as \nwell, and they would hope that they could 'read' the version in a \nconsistent style. We would also still have the choice of changing our \nexisting string style, which would explicitly signal a change via the \ntest fail.\n\n>\n> The only thing the scripts that read from the output of \"git\n> version\" can reliably tell is if it is interacting with a version of\n> Git it knows about. If it made any unwarranted assumption on the\n> versions it hasn't seen, it has to be fixed in \"git gui\" _anyway_.\n>\n> Of course, we would not change the output of \"git version\"\n> willy-nilly without good reason, but that is a different topic.\nAh. I thought it was the [original] topic. I was calibrating the \nwilly-nillyness ;-)\n\n>\n> So I do not know what you want to achieve in the real world with\n> that extra limitation on the \"git version\" output format.\n>\n> Maybe you are proposing something else. I dunno.\nI was just using a slightly different philosophical approach.\n\n> --\nPhilip\n"}]}