{"thread":{"id":"44192","subject":"[PATCH] diff_unique_abbrev(): document its assumtion and limitation","startedAt":"2016-09-30T17:55:01Z","lastAt":"2016-10-03T17:08:10Z","messageCount":5,"participants":["Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"302994","messageId":"xmqqvaxd9ssy.fsf@gitster.mtv.corp.google.com","threadId":"44192","inReplyTo":null,"subject":"[PATCH] diff_unique_abbrev(): document its assumtion and limitation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-30T17:54:53Z","receivedAt":"2016-09-30T17:55:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This function is used to add \"...\" to displayed object names in\n\"diff --raw --abbrev[=]\" output. It bases its behaviour on an\nuntold assumption that the abbreviation length requested by the\ncaller is \"reasonble\", i.e. most of the objects will abbreviate\nwithin the requested length and the resulting length would never\nexceed it by more than a few hexdigits (otherwise the resulting\ncolumns would not align). Explain that in a comment.\n\nSigned-off-by: Junio C Hamano \n---\n\n * I had to scratch my head wondering what impact Linus's\n auto-abbrev change will have on this code, which I wrote many\n years ago in 47dd0d59 (\"diff: --abbrev option\", 2005-12-13).\n\n diff.c | 23 ++++++++++++++++++++++-\n 1 file changed, 22 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex cefc13eb8e..428ed4f4c9 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4108,7 +4108,8 @@ void diff_free_filepair(struct diff_filepair *p)\n \tfree(p);\n }\n \n-/* This is different from find_unique_abbrev() in that\n+/*\n+ * This is different from find_unique_abbrev() in that\n * it stuffs the result with dots for alignment.\n */\n const char *diff_unique_abbrev(const unsigned char *sha1, int len)\n@@ -4120,6 +4121,26 @@ const char *diff_unique_abbrev(const unsigned char *sha1, int len)\n \n \tabbrev = find_unique_abbrev(sha1, len);\n \tabblen = strlen(abbrev);\n+\n+\t/*\n+\t * In well-behaved cases, where the abbbreviated result is the\n+\t * same as the requested length, append three dots after the\n+\t * abbreviation (hence the whole logic is limited to the case\n+\t * where abblen < 37); when the actual abbreviated result is a\n+\t * bit longer than the requested length, we reduce the number\n+\t * of dots so that they match the well-behaved ones. However,\n+\t * if the actual abbreviation is longer than the requested\n+\t * length by more than three, we give up on aligning, and add\n+\t * three dots anyway, to indicate that the output is not the\n+\t * full object name. Yes, this may be suboptimal, but this\n+\t * appears only in \"diff --raw --abbrev\" output and it is not\n+\t * worth the effort to change it now. Note that this would\n+\t * likely to work fine when the automatic sizing of default\n+\t * abbreviation length is used--we would be fed -1 in \"len\" in\n+\t * that case, and will end up always appending three-dots, but\n+\t * the automatic sizing is supposed to give abblen that ensures\n+\t * uniqueness across all objects (statistically speaking).\n+\t */\n \tif (abblen < 37) {\n \t\tstatic char hex[41];\n \t\tif (len < abblen && abblen <= len + 2)\n-- \n2.10.0-612-g22341905f2\n\n"},{"id":"302997","messageId":"20160930180957.xj4jqoslbtevhqpb@sigill.intra.peff.net","threadId":"44192","inReplyTo":"xmqqvaxd9ssy.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] diff_unique_abbrev(): document its assumtion and limitation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-30T18:09:57Z","receivedAt":"2016-09-30T18:10:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 30, 2016 at 10:54:53AM -0700, Junio C Hamano wrote:\n\n> This function is used to add \"...\" to displayed object names in\n> \"diff --raw --abbrev[=]\" output. It bases its behaviour on an\n> untold assumption that the abbreviation length requested by the\n> caller is \"reasonble\", i.e. most of the objects will abbreviate\n> within the requested length and the resulting length would never\n> exceed it by more than a few hexdigits (otherwise the resulting\n> columns would not align). Explain that in a comment.\n\nHeh, I have actually have a similar patch that renames it to\ndiff_aligned_abbrev(). Because I wanted to add another function:\n\n static const char *diff_abbrev_oid(const struct object_id *oid,\n int abbrev)\n {\n if (startup_info->have-repository)\n return find_unique_abbrev(oid->hash, abbrev);\n else {\n char *hex = oid_to_hex(oid);\n if (abbrev < 0) || abbrev > GIT_SHA1_HEXSZ)\n die(\"BUG: oid abbreviation out of range: %d\", abbrev);\n hex[abbrev] = '\\0';\n return hex;\n }\n }\n\nand I didn't want people to confuse the two. Now that function _would_\nwant to be updated as a result of the other conversation (it would need\nto do something sensible with \"-1\", like turning it into \"7\", or\nwhatever else is deemed reasonable outside of a repository).\n\nAnyway. I just wonder if you want to give it a better name while you are\nat it.\n\n-Peff\n"},{"id":"303004","messageId":"xmqqd1jl9ovc.fsf@gitster.mtv.corp.google.com","threadId":"44192","inReplyTo":"20160930180957.xj4jqoslbtevhqpb@sigill.intra.peff.net","subject":"Re: [PATCH] diff_unique_abbrev(): document its assumtion and limitation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-30T19:19:51Z","receivedAt":"2016-09-30T19:20:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... Now that function _would_\n> want to be updated as a result of the other conversation (it would need\n> to do something sensible with \"-1\", like turning it into \"7\", or\n> whatever else is deemed reasonable outside of a repository).\n>\n> Anyway. I just wonder if you want to give it a better name while you are\n> at it.\n\nI'd say the patch to introduce the new function that makes the old\nname potentially confusing is a good one to do the rename. Until\nthen I do not think there is no need to rename the existing one ;-)\n\nRelated tangent about \"like turning it into\", I am thinking adding\nsomething like this as a preparatory step to Linus's auto-sizing\nserires. That way, we do not have to spell \"7\"\n\nHaving to spell FALLBACK_DEFAULT_ABBREV over and over again would be\nmore irritating than having to spell \"7\" often, but I think it would\nbe a sign of a deeper problem if it turns out we have to repeat this\nconstant in many places, so an irritatingly long name may serve as a\ncanary in the coalmine ;-)\n\n-- >8 --\nSubject: abbrev: add FALLBACK_DEFAULT_ABBREV to prepare for auto sizing\n\nWe'll be introducing a new way to decide the default abbreviation\nlength by initialising DEFAULT_ABBREV to -1 to signal the first call\nto \"find unique abbreviation\" codepath to compute a reasonable value\nbased on the number of objects we have to avoid collisions.\n\nWe have long relied on DEFAULT_ABBREV being a positive concrete\nvalue that is used as the abbreviation length when no extra\nconfiguration or command line option has overridden it. Some\ncodepaths wants to use such a positive concrete default value\neven before making their first request to actually trigger the\ncomputation for the auto sized default.\n\nIntroduce FALLBACK_DEFAULT_ABBREV and use it to the code that\nattempts to align the report from \"git fetch\". For now, this\nmacro is also used to initialize the default_abbrev variable,\nbut the auto-sizing code will use -1 and then use the value of\nFALLBACK_DEFAULT_ABBREV as the starting point of auto-sizing.\n\nSigned-off-by: Junio C Hamano \n---\n\n builtin/fetch.c | 3 +++\n cache.h | 3 +++\n environment.c | 2 +-\n transport.h | 3 +--\n 4 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex e4639d8eb1..5d6994d8e7 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -16,6 +16,9 @@\n #include \"connected.h\"\n #include \"argv-array.h\"\n \n+#define TRANSPORT_SUMMARY(x) \\\n+\t(int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)\n+\n static const char * const builtin_fetch_usage[] = {\n \tN_(\"git fetch [] [ [...]]\"),\n \tN_(\"git fetch [] \"),\ndiff --git a/cache.h b/cache.h\nindex 4ff196c259..677554c59f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1133,6 +1133,9 @@ static inline unsigned int hexval(unsigned char c)\n #define MINIMUM_ABBREV minimum_abbrev\n #define DEFAULT_ABBREV default_abbrev\n \n+/* used when the code does not know or care what the default abbrev is */\n+#define FALLBACK_DEFAULT_ABBREV 7\n+\n struct object_context {\n \tunsigned char tree[20];\n \tchar path[PATH_MAX];\ndiff --git a/environment.c b/environment.c\nindex 96160a75a5..c8860f722d 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -16,7 +16,7 @@ int trust_executable_bit = 1;\n int trust_ctime = 1;\n int check_stat = 1;\n int has_symlinks = 1;\n-int minimum_abbrev = 4, default_abbrev = 7;\n+int minimum_abbrev = 4, default_abbrev = FALLBACK_DEFAULT_ABBREV;\n int ignore_case;\n int assume_unchanged;\n int prefer_symlink_refs;\ndiff --git a/transport.h b/transport.h\nindex c68140892c..ea25e42317 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -135,8 +135,7 @@ struct transport {\n #define TRANSPORT_PUSH_CERT_IF_ASKED 4096\n #define TRANSPORT_PUSH_ATOMIC 8192\n \n-#define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)\n-#define TRANSPORT_SUMMARY(x) (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)\n+#define TRANSPORT_SUMMARY_WIDTH (2 * FALLBACK_DEFAULT_ABBREV + 3)\n \n /* Returns a transport suitable for the url */\n struct transport *transport_get(struct remote *, const char *);\n"},{"id":"303041","messageId":"20161001091558.guduirzlkog5fkzd@sigill.intra.peff.net","threadId":"44192","inReplyTo":"xmqqd1jl9ovc.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] diff_unique_abbrev(): document its assumtion and limitation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-01T09:15:58Z","receivedAt":"2016-10-01T09:16:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 30, 2016 at 12:19:51PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > ... Now that function _would_\n> > want to be updated as a result of the other conversation (it would need\n> > to do something sensible with \"-1\", like turning it into \"7\", or\n> > whatever else is deemed reasonable outside of a repository).\n> >\n> > Anyway. I just wonder if you want to give it a better name while you are\n> > at it.\n> \n> I'd say the patch to introduce the new function that makes the old\n> name potentially confusing is a good one to do the rename. Until\n> then I do not think there is no need to rename the existing one ;-)\n\nI guess my point was that the poor name may have contributed to the need\nto explain it. But I'm happy to deal with it in my series (I also\nupdated it to use \"struct oid\"; I'll probably rebase mine on top of your\ncomment to save you dealing with the nasty merge).\n\n> Related tangent about \"like turning it into\", I am thinking adding\n> something like this as a preparatory step to Linus's auto-sizing\n> serires. That way, we do not have to spell \"7\"\n> [...]\n> -- >8 --\n> Subject: abbrev: add FALLBACK_DEFAULT_ABBREV to prepare for auto sizing\n\nYep, looks like a good idea.\n\n-Peff\n"},{"id":"303096","messageId":"xmqqr37x743x.fsf@gitster.mtv.corp.google.com","threadId":"44192","inReplyTo":"20161001091558.guduirzlkog5fkzd@sigill.intra.peff.net","subject":"Re: [PATCH] diff_unique_abbrev(): document its assumtion and limitation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-03T17:08:02Z","receivedAt":"2016-10-03T17:08:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I guess my point was that the poor name may have contributed to the need\n> to explain it.\n\nThe comment was not about \"it may not be obvious but this tries to\npad and align\", but to say \"the way this tries to pad and align is\nbased on an unsaid assumption that leads to this limitation\". I do\nagree it is a good idea to rename it to a name that has 'pad' or\n'align' in addition to 'unique', but I doubt renaming alone would\nreduce the need for the new comment.\n"}]}