Re: [PATCH] diff_unique_abbrev(): document its assumtion and limitation

Jeff King [off-list ref] writes:

... Now that function _would_
want to be updated as a result of the other conversation (it would need
to do something sensible with "-1", like turning it into "7", or
whatever else is deemed reasonable outside of a repository).

Anyway. I just wonder if you want to give it a better name while you are
at it.

I'd say the patch to introduce the new function that makes the old
name potentially confusing is a good one to do the rename.  Until
then I do not think there is no need to rename the existing one ;-)

Related tangent about "like turning it into", I am thinking adding
something like this as a preparatory step to Linus's auto-sizing
serires.  That way, we do not have to spell "7"

Having to spell FALLBACK_DEFAULT_ABBREV over and over again would be
more irritating than having to spell "7" often, but I think it would
be a sign of a deeper problem if it turns out we have to repeat this
constant in many places, so an irritatingly long name may serve as a
canary in the coalmine ;-)

-- >8 --
Subject: abbrev: add FALLBACK_DEFAULT_ABBREV to prepare for auto sizing

We'll be introducing a new way to decide the default abbreviation
length by initialising DEFAULT_ABBREV to -1 to signal the first call
to "find unique abbreviation" codepath to compute a reasonable value
based on the number of objects we have to avoid collisions.

We have long relied on DEFAULT_ABBREV being a positive concrete
value that is used as the abbreviation length when no extra
configuration or command line option has overridden it.  Some
codepaths wants to use such a positive concrete default value
even before making their first request to actually trigger the
computation for the auto sized default.

Introduce FALLBACK_DEFAULT_ABBREV and use it to the code that
attempts to align the report from "git fetch".  For now, this
macro is also used to initialize the default_abbrev variable,
but the auto-sizing code will use -1 and then use the value of
FALLBACK_DEFAULT_ABBREV as the starting point of auto-sizing.

Signed-off-by: Junio C Hamano <redacted>
---

 builtin/fetch.c | 3 +++
 cache.h         | 3 +++
 environment.c   | 2 +-
 transport.h     | 3 +--
 4 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index e4639d8eb1..5d6994d8e7 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c

@@ -16,6 +16,9 @@
 #include "connected.h"
 #include "argv-array.h"
 
+#define TRANSPORT_SUMMARY(x) \
+	(int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)
+
 static const char * const builtin_fetch_usage[] = {
 	N_("git fetch [<options>] [<repository> [<refspec>...]]"),
 	N_("git fetch [<options>] <group>"),

diff --git a/cache.h b/cache.h
index 4ff196c259..677554c59f 100644
--- a/cache.h
+++ b/cache.h

@@ -1133,6 +1133,9 @@ static inline unsigned int hexval(unsigned char c)
 #define MINIMUM_ABBREV minimum_abbrev
 #define DEFAULT_ABBREV default_abbrev
 
+/* used when the code does not know or care what the default abbrev is */
+#define FALLBACK_DEFAULT_ABBREV 7
+
 struct object_context {
 	unsigned char tree[20];
 	char path[PATH_MAX];

diff --git a/environment.c b/environment.c
index 96160a75a5..c8860f722d 100644
--- a/environment.c
+++ b/environment.c

@@ -16,7 +16,7 @@ int trust_executable_bit = 1;
 int trust_ctime = 1;
 int check_stat = 1;
 int has_symlinks = 1;
-int minimum_abbrev = 4, default_abbrev = 7;
+int minimum_abbrev = 4, default_abbrev = FALLBACK_DEFAULT_ABBREV;
 int ignore_case;
 int assume_unchanged;
 int prefer_symlink_refs;

diff --git a/transport.h b/transport.h
index c68140892c..ea25e42317 100644
--- a/transport.h
+++ b/transport.h

@@ -135,8 +135,7 @@ struct transport {
 #define TRANSPORT_PUSH_CERT_IF_ASKED 4096
 #define TRANSPORT_PUSH_ATOMIC 8192
 
-#define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
-#define TRANSPORT_SUMMARY(x) (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)
+#define TRANSPORT_SUMMARY_WIDTH (2 * FALLBACK_DEFAULT_ABBREV + 3)
 
 /* Returns a transport suitable for the url */
 struct transport *transport_get(struct remote *, const char *);

On Fri, Sep 30, 2016 at 12:19:51PM -0700, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

... Now that function _would_
want to be updated as a result of the other conversation (it would need
to do something sensible with "-1", like turning it into "7", or
whatever else is deemed reasonable outside of a repository).

Anyway. I just wonder if you want to give it a better name while you are
at it.

I'd say the patch to introduce the new function that makes the old
name potentially confusing is a good one to do the rename.  Until
then I do not think there is no need to rename the existing one ;-)

I guess my point was that the poor name may have contributed to the need
to explain it. But I'm happy to deal with it in my series (I also
updated it to use "struct oid"; I'll probably rebase mine on top of your
comment to save you dealing with the nasty merge).

Related tangent about "like turning it into", I am thinking adding
something like this as a preparatory step to Linus's auto-sizing
serires.  That way, we do not have to spell "7"
[...]
-- >8 --
Subject: abbrev: add FALLBACK_DEFAULT_ABBREV to prepare for auto sizing

Yep, looks like a good idea.

-Peff