Re: [PATCH 5/5] builtin-remote: Make "remote -v" display push urls

[Michael J Gruber <git@drmicha.warpmail.net>]

> On Tue, Jun 9, 2009 at 18:01, Michael J Gruber<git@drmicha.warpmail.net> wrote:
>> Currently, "remote -v" simply lists all urls so that one has to remember
>> that only the first one is used for fetches, and all are used for
>> pushes.
>>
>> Change this so that the role of an url is displayed in parentheses, and
>> also display push urls.
>>
>> Example with "mjg" having 1 url and 1 pushurl, "origin" having 3 urls,
>> sb having 1 url:
>>
>> mjg     git://repo.or.cz/git/mjg.git (fetch)
>> mjg     repoor:/srv/git/git/mjg.git (push)
>> origin  git://repo.or.cz/git.git (fetch)
>> origin  git://repo.or.cz/git.git (push)
>> origin  git://git2.kernel.org/pub/scm/git/git.git (push)
>> origin  git://repo.or.cz/alt-git.git (push)
>> sb      git://repo.or.cz/git/sbeyer.git (fetch)
>> sb      git://repo.or.cz/git/sbeyer.git (push)

Junio wrote:
> I am debating myself if the last two should be just one line,
> without "(fetch)" nor "(push)" tacked at the end, like this:
> 
>         sb     git://repo.or.cz/git/sbeyer.git
> 
> If we change the rule in your patch to format a remote.*.url used for both
> push and fetch as a single line to achieve this, however, it would make
> your "origin" example come out like this instead:
> 
> 	origin git://repo.or.cz/git.git
>         origin git://git.kernel.org/pub/scm/git/git.git (push)
>         origin git://repo.or.cz/alt-git.git (push)
> 
> which is arguably better (one less line) and worse (it is unclear if the
> top one is only for fetching) at the same time.
> 
> Or perhaps we could go with something like this.
> 
> 	origin git://repo.or.cz/git.git (fetch/push)
>         origin git://git.kernel.org/pub/scm/git/git.git (push)
>         origin git://repo.or.cz/alt-git.git (push)
>         sb     git://repo.or.cz/git/sbeyer.git
> 
> i.e. make the rule such that a URL used for both are shown with (fetch/push)
> only if there are other lines for the same remote.

Bert wrote:
> Wouldn't it be more readable if push|fetch comes first?
> 
> mjg     (fetch) git://repo.or.cz/git/mjg.git
> mjg     (push)  repoor:/srv/git/git/mjg.git
> origin  (fetch) git://repo.or.cz/git.git
> origin  (push)  git://repo.or.cz/git.git
> origin  (push)  git://git2.kernel.org/pub/scm/git/git.git
> origin  (push)  git://repo.or.cz/alt-git.git
> sb      (fetch) git://repo.or.cz/git/sbeyer.git
> sb      (push)  git://repo.or.cz/git/sbeyer.git
> 
> And how about to print only one line for (url_nr == 1 && pushurl_nr == 0):
> 
> mjg     (fetch) git://repo.or.cz/git/mjg.git
> mjg     (push)  repoor:/srv/git/git/mjg.git
> origin  (fetch) git://repo.or.cz/git.git
> origin  (push)  git://repo.or.cz/git.git
> origin  (push)  git://git2.kernel.org/pub/scm/git/git.git
> origin  (push)  git://repo.or.cz/alt-git.git
> sb              git://repo.or.cz/git/sbeyer.git
> 

All this shows why this one is 5/5 and separate ;) I was thinking hard
about the best display also. From my point of view, there are two
arguments against the seemingly more user friendly variants (collapsing
fetch and push entries with the same url):

- It introduces 3 different types of lines, rather than 2.
- The code would have to go through all (push) urls and check whether it
matches url[0].

While the latter one is technical, the first makes it more difficult to
parse the output, both visually and programmatically (I know, porc...).

Note that at least implicitely we always had 2 different types of lines
in the output of "remote -v": the first one (fetch+push) and the others
(push). There is no way to keep only those 2 types (when there is a
"pushurl" setting), my suggestion was to use 2 types now for the 2 purposes.

>>
>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>> ---
>>  builtin-remote.c |   27 +++++++++++++++++++++++----
>>  1 files changed, 23 insertions(+), 4 deletions(-)
>>
>> diff --git a/builtin-remote.c b/builtin-remote.c
>> index b350b18..80b2536 100644
>> --- a/builtin-remote.c
>> +++ b/builtin-remote.c
>> @@ -1276,14 +1276,31 @@ static int update(int argc, const char **argv)
>>  static int get_one_entry(struct remote *remote, void *priv)
>>  {
>>        struct string_list *list = priv;
>> +       const char **url;
>> +       int i, url_nr;
>> +       void **utilp;
>>
>>        if (remote->url_nr > 0) {
>> -               int i;
>> -
>> -               for (i = 0; i < remote->url_nr; i++)
>> -                       string_list_append(remote->name, list)->util = (void *)remote->url[i];
>> +               utilp = &(string_list_append(remote->name, list)->util);
>> +               *utilp = malloc(strlen(remote->url[0])+strlen(" (fetch)")+1);
>> +               strcpy((char *) *utilp, remote->url[0]);
>> +               strcat((char *) *utilp, " (fetch)");
> How about using struct strbuf?

I thought it's complicated enough for the little change in output... But
what do strbufs bring us here?
Before the patch, builtin-remote would leak the string_list. They way I
use malloc enables me to free the string list (using string_list_clear)
later on, including the strings and utils. strcpy and strcat look a bit
low level but I see no point in using the sprintf shotgun here.

More or less I have zero clue about strbuf (OK, probably not less...)
but I think that if util is a strbuf then freeing everything would be
more complicated because a strbuf may or may not be allocated.

Michael