Re: [PATCH 3/3] transport: allow summary-width to be computed dynamically
Jeff King writes: > On Fri, Oct 21, 2016 at 09:39:45PM -0700, Junio C Hamano wrote: > >> And this is the final one. >> >> -- >8 -- >> From: Junio C Hamano >> Date: Fri, 21 Oct 2016 21:33:06 -0700 >> Subject: [PATCH] transport: compute summary-width dynamically >> >> Now all that is left to do is to actually iterate over the refs >> and measure the display width needed to show their abbreviation. > > I think we crossed emails. :) This is obviously correct, if we don't > mind paying the find_unique_abbrev cost twice for each sha1. Indeed we did. I do not think the cost matters that much in the codepath to produce the final summary output. > This is a minor style nit, but I think it's better to avoid mixing > unrelated bits between the initialization, condition, and iteration bits > of a for loop. Yeah, you're right.
Re: [PATCH 3/3] transport: allow summary-width to be computed dynamically
On Fri, Oct 21, 2016 at 09:39:45PM -0700, Junio C Hamano wrote:
> And this is the final one.
>
> -- >8 --
> From: Junio C Hamano
> Date: Fri, 21 Oct 2016 21:33:06 -0700
> Subject: [PATCH] transport: compute summary-width dynamically
>
> Now all that is left to do is to actually iterate over the refs
> and measure the display width needed to show their abbreviation.
I think we crossed emails. :) This is obviously correct, if we don't
mind paying the find_unique_abbrev cost twice for each sha1.
> int transport_summary_width(const struct ref *refs)
> {
> - return (2 * FALLBACK_DEFAULT_ABBREV + 3);
> + int maxw;
> +
> + for (maxw = -1; refs; refs = refs->next) {
> + maxw = measure_abbrev(&refs->old_oid, maxw);
> + maxw = measure_abbrev(&refs->new_oid, maxw);
> + }
This is a minor style nit, but I think it's better to avoid mixing
unrelated bits between the initialization, condition, and iteration bits
of a for loop. IOW:
int maxw = -1;
for (; refs; refs = refs->next) {
...
}
makes the for-loop _just_ about iteration, and it is more clear that the
manipulation of "maxw" is a side effect of the loop that is valid after
it finishes.
Though in this particular case, the loop and the whole function are
short enough that I don't care that much. Just raising a philosophical
point. :)
-Peff
Re: [PATCH 3/3] transport: allow summary-width to be computed dynamically
Junio C Hamano writes:
> Now we have identified three callchains that have a set of refs that
> they want to show their object names in an aligned output,
> we can replace their reference to the constant TRANSPORT_SUMMARY_WIDTH
> with a helper function call to transport_summary_width() that takes
> the set of ref as a parameter. This step does not yet iterate over
> the refs and compute, which is left as an exercise to the readers.
And this is the final one.
-- >8 --
From: Junio C Hamano
Date: Fri, 21 Oct 2016 21:33:06 -0700
Subject: [PATCH] transport: compute summary-width dynamically
Now all that is left to do is to actually iterate over the refs
and measure the display width needed to show their abbreviation.
Signed-off-by: Junio C Hamano
---
transport.c | 18 +-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/transport.c b/transport.c
index d4b8bf5f25..f1f95cf7c7 100644
--- a/transport.c
+++ b/transport.c
@@ -429,9 +429,25 @@ static int print_one_push_status(struct ref *ref, const
char *dest, int count,
return 1;
}
+static int measure_abbrev(const struct object_id *oid, int sofar)
+{
+ char hex[GIT_SHA1_HEXSZ + 1];
+ int w = find_unique_abbrev_r(hex, oid->hash, DEFAULT_ABBREV);
+
+ return (w < sofar) ? sofar : w;
+}
+
int transport_summary_width(const struct ref *refs)
{
- return (2 * FALLBACK_DEFAULT_ABBREV + 3);
+ int maxw;
+
+ for (maxw = -1; refs; refs = refs->next) {
+ maxw = measure_abbrev(&refs->old_oid, maxw);
+ maxw = measure_abbrev(&refs->new_oid, maxw);
+ }
+ if (maxw < 0)
+ maxw = FALLBACK_DEFAULT_ABBREV;
+ return (2 * maxw + 3);
}
void transport_print_push_status(const char *dest, struct ref *refs,
--
2.10.1-723-g2384e83bc3
Re: [PATCH 3/3] transport: allow summary-width to be computed dynamically
On Fri, Oct 21, 2016 at 03:39:27PM -0700, Junio C Hamano wrote:
> Now we have identified three callchains that have a set of refs that
> they want to show their object names in an aligned output,
> we can replace their reference to the constant TRANSPORT_SUMMARY_WIDTH
> with a helper function call to transport_summary_width() that takes
> the set of ref as a parameter. This step does not yet iterate over
> the refs and compute, which is left as an exercise to the readers.
The final step could be something like this:
diff --git a/transport.c b/transport.c
index 4dac713063..c1eaa4a 100644
--- a/transport.c
+++ b/transport.c
@@ -443,7 +443,21 @@ static int print_one_push_status(struct ref *ref, const
char *dest, int count,
int transport_summary_width(const struct ref *refs)
{
- return (2 * FALLBACK_DEFAULT_ABBREV + 3);
+ int max_abbrev;
+
+ /*
+* Computing the complete set of abbreviated sha1s is expensive just to
+* find their lengths, but we can at least find our real dynamic
+* minimum by picking an arbitrary sha1.
+*/
+ if (refs)
+ max_abbrev = strlen(find_unique_abbrev(refs->old_oid.hash,
+ DEFAULT_ABBREV));
+ else
+ max_abbrev = FALLBACK_DEFAULT_ABBREV;
+
+ /* 2 abbreviated sha1s, plus "..." in between */
+ return (2 * max_abbrev + 3);
}
void transport_print_push_status(const char *dest, struct ref *refs,
which produces reasonable results for me. But if we really wanted the
true value, I think we'd want to compute and store the abbreviated
sha1s, and then the refactoring done by your series probably isn't the
right direction.
I think we'd instead want to replace "struct strbuf *display" passed
down to update_local_ref() with something more like:
struct ref_status_table {
struct ref_status_item {
char old_hash[GIT_SHA1_HEX + 1];
char new_hash[GIT_SHA1_HEX + 1];
char *remote_ref;
char *local_ref;
char *summary;
char code;
} *items;
size_t alloc, nr;
};
and then format_display() would just add to the list (including calling
find_unique_abbrev()), and then at the end we'd call a function to show
them all.
That would also get rid of prepare_format_display(), as we could easily
walk over the prepared list before printing any of them (as opposed to
what that function does now, which is to walk over the ref map; that
requires that it know which refs are going to be printed).
-Peff
[PATCH 3/3] transport: allow summary-width to be computed dynamically
Now we have identified three callchains that have a set of refs that
they want to show their object names in an aligned output,
we can replace their reference to the constant TRANSPORT_SUMMARY_WIDTH
with a helper function call to transport_summary_width() that takes
the set of ref as a parameter. This step does not yet iterate over
the refs and compute, which is left as an exercise to the readers.
Signed-off-by: Junio C Hamano
---
builtin/fetch.c | 4 ++--
transport.c | 7 ++-
transport.h | 2 +-
3 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 40696e5338..09813cd826 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -722,7 +722,7 @@ static int store_updated_refs(const char *raw_url, const
char *remote_name,
char *url;
const char *filename = dry_run ? "/dev/null" : git_path_fetch_head();
int want_status;
- int summary_width = TRANSPORT_SUMMARY_WIDTH;
+ int summary_width = transport_summary_width(ref_map);
fp = fopen(filename, "a");
if (!fp)
@@ -906,7 +906,7 @@ static int prune_refs(struct refspec *refs, int ref_count,
struct ref *ref_map,
int url_len, i, result = 0;
struct ref *ref, *stale_refs = get_stale_heads(refs, ref_count,
ref_map);
char *url;
- int summary_width = TRANSPORT_SUMMARY_WIDTH;
+ int summary_width = transport_summary_width(stale_refs);
const char *dangling_msg = dry_run
? _(" (%s will become dangling)")
: _(" (%s has become dangling)");
diff --git a/transport.c b/transport.c
index ec02b78924..d4b8bf5f25 100644
--- a/transport.c
+++ b/transport.c
@@ -429,6 +429,11 @@ static int print_one_push_status(struct ref *ref, const
char *dest, int count,
return 1;
}
+int transport_summary_width(const struct ref *refs)
+{
+ return (2 * FALLBACK_DEFAULT_ABBREV + 3);
+}
+
void transport_print_push_status(const char *dest, struct ref *refs,
int verbose, int porcelain, unsigned int
*reject_reasons)
{
@@ -436,7 +441,7 @@ void transport_print_push_status(const char *dest, struct
ref *refs,
int n = 0;
unsigned char head_sha1[20];
char *head;
- int summary_width = TRANSPORT_SUMMARY_WIDTH;
+ int summary_width = transport_summary_width(refs);
head = resolve_refdup("HEAD", RESOLVE_REF_READING, head_sha1, NULL);
diff --git a/transport.h b/transport.h
index e783377e40..706d99e818 100644
--- a/transport.h
+++ b/transport.h
@@ -142,7 +142,7 @@ struct transport {
#define TRANSPORT_PUSH_ATOMIC 8192
#define TRANSPORT_PUSH_OPTIONS 16384
-#define TRANSPORT_SUMMARY_WIDTH (2 * FALLBACK_DEFAULT_ABBREV + 3)
+extern int transport_summary_width(const struct ref *refs);
/* Returns a transport suitable for the url */
struct transport *transport_get(struct remote *, const char *);
--
2.10.1-723-g2384e83bc3
