[PATCH 0/2] Better ref summary alignment in "git fetch"

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/2] Better ref summary alignment in "git fetch"

Duy Nguyen
I recently got a taste of a busy github repository where many branches
are created (for Pull Requests) every day. Because branch names should
be descriptive, branch names could range from 10 to 60 chars long.
This makes "git fetch" output really messy.

So I resurrect a patch (1/2) I sent two years ago to keep alignment
somewhat better. 2/2 is some more fancy adjustment to make sure one
really long branch name will not make the rest of ref list suffer. But
I think 1/2 is good enough most of the time.

Nguyễn Thái Ngọc Duy (2):
  fetch: better alignment in ref summary
  fetch: reduce ref column size when there are enough short ref names

 builtin/fetch.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

--
2.8.2.524.g6ff3d78

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/2] fetch: better alignment in ref summary

Duy Nguyen
Currently fetch hard-codes the "remote" column to be 10. For repos
with long branch names, the output could look ugly like this

From github.com:pclouds/git
 * [new branch]      2nd-index  -> pclouds/2nd-index
 * [new branch]      3nd-index  -> pclouds/3nd-index
 * [new branch]      file-watcher -> pclouds/file-watcher
 * [new branch]      inst       -> pclouds/inst
 * [new branch]      large-file-fixes -> pclouds/large-file-fixes
 * [new branch]      ls         -> pclouds/ls
 * [new branch]      master     -> pclouds/master
 * [new branch]      multiple-work-trees -> pclouds/multiple-work-trees
 * [new branch]      mv         -> pclouds/mv
 * [new branch]      read-cache-daemon -> pclouds/read-cache-daemon
 * [new branch]      split-blob -> pclouds/split-blob
 * [new branch]      split-index -> pclouds/split-index
 * [new branch]      status-fast-fast -> pclouds/status-fast-fast
 * [new branch]      untracked-cache -> pclouds/untracked-cache

This patch makes the output a bit better with minimum code change

From github.com:pclouds/git
 * [new branch]      2nd-index  -> pclouds/2nd-index
 * [new branch]      3nd-index  -> pclouds/3nd-index
 * [new branch]      file-watcher -> pclouds/file-watcher
 * [new branch]      inst         -> pclouds/inst
 * [new branch]      large-file-fixes -> pclouds/large-file-fixes
 * [new branch]      ls               -> pclouds/ls
 * [new branch]      master           -> pclouds/master
 * [new branch]      multiple-work-trees -> pclouds/multiple-work-trees
 * [new branch]      mv                  -> pclouds/mv
 * [new branch]      read-cache-daemon   -> pclouds/read-cache-daemon
 * [new branch]      split-blob          -> pclouds/split-blob
 * [new branch]      split-index         -> pclouds/split-index
 * [new branch]      status-fast-fast    -> pclouds/status-fast-fast
 * [new branch]      untracked-cache     -> pclouds/untracked-cache

To make all "->" aligned, we may need to go through the ref list
twice, or buffer the output and let column.c align it. Either way
needs a lot more work than this.

Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
---
 builtin/fetch.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 1cf15d0..223e09b 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -465,7 +465,14 @@ fail:
    : STORE_REF_ERROR_OTHER;
 }
 
-#define REFCOL_WIDTH  10
+static int REFCOL_WIDTH = 10;
+
+static void adjust_refcol_width(int len)
+{
+ if (REFCOL_WIDTH < len) {
+ REFCOL_WIDTH = len;
+ }
+}
 
 static int update_local_ref(struct ref *ref,
     const char *remote,
@@ -477,6 +484,8 @@ static int update_local_ref(struct ref *ref,
  struct branch *current_branch = branch_get(NULL);
  const char *pretty_ref = prettify_refname(ref->name);
 
+ adjust_refcol_width(gettext_width(remote));
+
  type = sha1_object_info(ref->new_oid.hash, NULL);
  if (type < 0)
  die(_("object %s not found"), oid_to_hex(&ref->new_oid));
--
2.8.2.524.g6ff3d78

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/2] fetch: reduce ref column size when there are enough short ref names

Duy Nguyen
In reply to this post by Duy Nguyen
A flaw with the previous patch is, if there is a single long ref name,
the rest of the ref list will be aligned with the big column, wasting
lots of space (and potentially be wrapped around by the terminal, making
it even harder to read).

This patch tries to mitigate that. If there are five consecutive refs
whose name is at least ten chars shorter than column width, the width is
reduced by ten. Five and ten are picked out of thin air. But the short
width reduction (instead of "REFCOL_WIDTH = len") is to avoid the column
width being reduced then grown back too much and too fast (imagine the
next ref is really short then the ref after that is super long).

Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
---
 builtin/fetch.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 223e09b..ae2ff0c 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -469,8 +469,20 @@ static int REFCOL_WIDTH = 10;
 
 static void adjust_refcol_width(int len)
 {
+ static int reduce_width_count;
+
+ if (REFCOL_WIDTH > 10 && len < REFCOL_WIDTH - 10) {
+ reduce_width_count++;
+ if (reduce_width_count == 5) {
+ REFCOL_WIDTH -= 10;
+ reduce_width_count = 0;
+ }
+ } else
+ reduce_width_count = 0;
+
  if (REFCOL_WIDTH < len) {
  REFCOL_WIDTH = len;
+ reduce_width_count = 0;
  }
 }
 
--
2.8.2.524.g6ff3d78

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] fetch: better alignment in ref summary

Junio C Hamano
In reply to this post by Duy Nguyen
Nguyễn Thái Ngọc Duy  <[hidden email]> writes:

> Currently fetch hard-codes the "remote" column to be 10. For repos
> with long branch names, the output could look ugly like this
>
> From github.com:pclouds/git
>  * [new branch]      2nd-index  -> pclouds/2nd-index
>  * [new branch]      3nd-index  -> pclouds/3nd-index
>  * [new branch]      file-watcher -> pclouds/file-watcher
>  ...

So, we have to show "an object taken from their name is copied to
our name", and somebody designed the format to report it to use
"their-name -> our-name", and decided that a certain number of
columns is sufficient for "their-name" part and that attempting to
align "->" sign is a good idea..

That was long before a few best practices were established.  We
encourage people to use longer and more descriptive branch names, so
"their-name" is a lot longer than 10 columns, which contradicts the
first one of two old assumptions.  And we want to keep the second
old assumption alive.

Progressively pushing "->" to the right like you did might be a
cheap way to do so, but shouldn't we be also taking advantage of
another best practice that has been established since we started
reporting this "their-name came to our-name", namely, very often,
our-name is a short and fixed prefix plus their-name?

That is, I wonder if the above can become something like:

> From github.com:pclouds/git
>  * [new branch]      { -> pclouds/}2nd-index
>  * [new branch]      { -> pclouds/}3nd-index
>  * [new branch]      { -> pclouds/}file-watcher
>  ...

The above example borrows the idea used in diffstat label for
renaming patch and I think you can design a better notataion, but a
big point is that you can shorten the whole thing by not repeating
the common part twice.  The arrow aligns merely as a side effect of
the shortening, taking advantage of the fact that most people fetch
with "$their_prefix/*:$our_prefix/*" renaming refspec.


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] fetch: better alignment in ref summary

Duy Nguyen
On Mon, May 23, 2016 at 7:58 AM, Junio C Hamano <[hidden email]> wrote:

> That is, I wonder if the above can become something like:
>
>> From github.com:pclouds/git
>>  * [new branch]      { -> pclouds/}2nd-index
>>  * [new branch]      { -> pclouds/}3nd-index
>>  * [new branch]      { -> pclouds/}file-watcher
>>  ...
>
> The above example borrows the idea used in diffstat label for
> renaming patch and I think you can design a better notataion, but a
> big point is that you can shorten the whole thing by not repeating
> the common part twice.  The arrow aligns merely as a side effect of
> the shortening, taking advantage of the fact that most people fetch
> with "$their_prefix/*:$our_prefix/*" renaming refspec.

I did think of that. My only concern is, with the new output I can't
simply copy the ref name (e.g. pclouds/2nd-index) and use it to, say,
examine with git-log. But I'm more of a "tab tab tab" person than
"copy and paste", so I don't know how often people need to do that.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] fetch: better alignment in ref summary

Jeff King
In reply to this post by Duy Nguyen
On Sun, May 22, 2016 at 06:20:18PM +0700, Nguyễn Thái Ngọc Duy wrote:

> To make all "->" aligned, we may need to go through the ref list
> twice, or buffer the output and let column.c align it. Either way
> needs a lot more work than this.

I don't think a two-pass approach is _too_ bad. The trickiest thing is
that we handle the "prune" refs separately, even though they go in the
same status table.

However, I tried it, and the results looked much worse for my example
repo than yours. The problem is that I had one gigantic refname, and
that shoved the "->" and everything after far to the right, where they
wrapped to the next line.

Though the stair-stepping in your patch is funny, the output is easier
to read.

I do agree with Junio that we could probably improve the output quite a
bit by not showing each refname twice in the common case. I don't quite
find the "{ -> origin/}whatever" particularly pretty, but something like
that which keeps the variable bits at the end would make this problem
just go away.

> -#define REFCOL_WIDTH  10
> +static int REFCOL_WIDTH = 10;

This should probably go lower-case if it's not a preprocessor macro
anymore. I know it makes the diff a lot noisier, but I think it's worth
it.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] fetch: better alignment in ref summary

Marc Branchaud
In reply to this post by Duy Nguyen
On 2016-05-22 09:59 PM, Duy Nguyen wrote:

> On Mon, May 23, 2016 at 7:58 AM, Junio C Hamano <[hidden email]> wrote:
>> That is, I wonder if the above can become something like:
>>
>>>  From github.com:pclouds/git
>>>   * [new branch]      { -> pclouds/}2nd-index
>>>   * [new branch]      { -> pclouds/}3nd-index
>>>   * [new branch]      { -> pclouds/}file-watcher
>>>   ...
>>
>> The above example borrows the idea used in diffstat label for
>> renaming patch and I think you can design a better notataion, but a
>> big point is that you can shorten the whole thing by not repeating
>> the common part twice.  The arrow aligns merely as a side effect of
>> the shortening, taking advantage of the fact that most people fetch
>> with "$their_prefix/*:$our_prefix/*" renaming refspec.
>
> I did think of that. My only concern is, with the new output I can't
> simply copy the ref name (e.g. pclouds/2nd-index) and use it to, say,
> examine with git-log. But I'm more of a "tab tab tab" person than
> "copy and paste", so I don't know how often people need to do that.

Why do we need any kind of "->" at all?  How about simply (with an
update to "old-branch" for comparison to probably-more-common output):

 From github.com:pclouds/git
    cafed0c..badfeed  pclouds/old-branch
  * [new branch]      pclouds/2nd-index
  * [new branch]      pclouds/3nd-index
  * [new branch]      pclouds/file-watcher

                M.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] fetch: better alignment in ref summary

Jeff King
On Thu, May 26, 2016 at 10:22:25AM -0400, Marc Branchaud wrote:

> Why do we need any kind of "->" at all?  How about simply (with an update to
> "old-branch" for comparison to probably-more-common output):
>
> From github.com:pclouds/git
>    cafed0c..badfeed  pclouds/old-branch
>  * [new branch]      pclouds/2nd-index
>  * [new branch]      pclouds/3nd-index
>  * [new branch]      pclouds/file-watcher

That covers the common case of:

  refs/heads/*:refs/remotes/pclouds/*

but sometimes the remote and local names are not the same, and the
mapping is interesting. Like:

  $ git fetch origin refs/pull/*/head:refs/remotes/pr/*
  ...
   * [new ref]         refs/pull/77/head -> pr/77

Or even:

  $ git fetch origin refs/pull/77/head refs/pull/78/head
  From ...
   * branch            refs/pull/77/head -> FETCH_HEAD
   * branch            refs/pull/78/head -> FETCH_HEAD

So I think we need a scheme that can show the interesting mappings, but
collapses to something simple for the common case.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] fetch: better alignment in ref summary

Junio C Hamano
Jeff King <[hidden email]> writes:

> On Thu, May 26, 2016 at 10:22:25AM -0400, Marc Branchaud wrote:
>
>> Why do we need any kind of "->" at all?  How about simply (with an update to
>> "old-branch" for comparison to probably-more-common output):
>>
>> From github.com:pclouds/git
>>    cafed0c..badfeed  pclouds/old-branch
>>  * [new branch]      pclouds/2nd-index
>>  * [new branch]      pclouds/3nd-index
>>  * [new branch]      pclouds/file-watcher
>
> That covers the common case of:
>
>   refs/heads/*:refs/remotes/pclouds/*
>
> but sometimes the remote and local names are not the same, and the
> mapping is interesting. Like:
>
>   $ git fetch origin refs/pull/*/head:refs/remotes/pr/*
>   ...
>    * [new ref]         refs/pull/77/head -> pr/77
>
> Or even:
>
>   $ git fetch origin refs/pull/77/head refs/pull/78/head
>   From ...
>    * branch            refs/pull/77/head -> FETCH_HEAD
>    * branch            refs/pull/78/head -> FETCH_HEAD
>
> So I think we need a scheme that can show the interesting mappings, but
> collapses to something simple for the common case.

True.  One of the entries in Marc's example is easily misread as
"pclouds/2nd-index branch at its refs/heads/pclouds/2nd-index was
fetched to its usual place", when Marc wanted to say "they had
2nd-index branch at refs/heads/2nd-index, and it was copied to our
refs/remotes/pclouds/2nd-index".

So even though we might be able to make sure it is unambiguous
without "this -> that" ("->" is pronounced as 'became'), it is
easily misread.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] fetch: better alignment in ref summary

Marc Branchaud
On 2016-05-26 01:42 PM, Junio C Hamano wrote:

>
> True.  One of the entries in Marc's example is easily misread as
> "pclouds/2nd-index branch at its refs/heads/pclouds/2nd-index was
> fetched to its usual place", when Marc wanted to say "they had
> 2nd-index branch at refs/heads/2nd-index, and it was copied to our
> refs/remotes/pclouds/2nd-index".
>
> So even though we might be able to make sure it is unambiguous
> without "this -> that" ("->" is pronounced as 'became'), it is
> easily misread.

Actually, I tend to just think of it as "this is a local name you can
use to refer to the new thing that was just fetched."  The left-hand
side describes the thing being fetched (new or updated branch/tag/ref),
and the right hand side shows how to locally refer to that thing.

The fact that something is buried in some odd part of the ref tree is
less relevant, IMO.  If I'm using custom fetch refspecs or other
oddities, I'll have that in the back of my head.  But what I really care
about is what ref I can use with commands like log and checkout.

So, regarding Peff's examples:

 > $ git fetch origin refs/pull/*/head:refs/remotes/pr/*

Just show me the "pr/foo" refs.  I know where things are coming from.
Even if I configured that fetch refspec a long time ago, I don't need to
see the exact mapping every time I fetch.

 > $ git fetch origin refs/pull/77/head refs/pull/78/head

Ah, now this is an odd case.  Maybe there should be a different output
format altogether for this one.  The problem is that the remote refs
being fetched are stored without any kind of local ref.  (Commands like
"git log FETCH_HEAD" only work with the last ref fetched, even though
all the SHAs get added to the .git/FETCH_HEAD file.  Maybe if git
supported a "FETCH_HEAD@{x}" syntax...)

I think the output should show the plain SHA values, since those are the
only things the user can use to work with those refs.  Maybe something like:

        From ...
         * origin:refs/pull/77/head  abcd0123
         * origin:refs/pull/78/head  453def00

(Not 100% sure about the "origin:" prefix...)

                M.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] fetch: better alignment in ref summary

Junio C Hamano
Marc Branchaud <[hidden email]> writes:

> The fact that something is buried in some odd part of the ref tree is
> less relevant, IMO.  If I'm using custom fetch refspecs or other
> oddities, I'll have that in the back of my head.  But what I really
> care about is what ref I can use with commands like log and checkout.
>
> So, regarding Peff's examples:
>
>> $ git fetch origin refs/pull/*/head:refs/remotes/pr/*
>
> Just show me the "pr/foo" refs.  I know where things are coming
> from. Even if I configured that fetch refspec a long time ago, I don't
> need to see the exact mapping every time I fetch.

That is only because you are used to seeing them.  You cannot claim
"I'll remember forever without seeing them" without actually
experiencing not seeing them for a long time.

> I think the output should show the plain SHA values, since those are
> the only things the user can use to work with those refs.

When they tell others how to reproduce what they did (or record what
they did so that they can reproduce it later), they need what it is
called at the remote end.

I would hesitate to go in the approach based on discarding
information, as if it is the only way to shorten and clarify the
output.  Let's not do so before thinking things through to achieve
the same while keeping the information we give to the users.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html