Re: [PATCH 1/3] builtin/fetch.c: Add pretty_url() and print_url()

[Tom Miller <jackerran@gmail.com>]

On Wed, Dec 18, 2013 at 3:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Tom Miller <jackerran@gmail.com> writes:
>
>> In order to fix branchname DF conflicts during `fetch --prune`, the way
>> the header is output to the screen needs to be refactored. Here is an
>> exmaple of the output with the line in question denoted by '>':
>>
>>       $ git fetch --prune --dry-run upstream
>>>      From https://github.com/git/git
>>          a155a5f..5512ac5  maint      -> upstream/maint
>>          d7aced9..7794a68  master     -> upstream/master
>>          523f7c4..3e57c29  next       -> upstream/next
>>        + 462f102...0937cdf pu         -> upstream/pu  (forced update)
>>          e24105a..5d352bc  todo       -> upstream/todo
>>        * [new tag]         v1.8.5.2   -> v1.8.5.2
>>        * [new tag]         v1.8.5.2   -> v1.8.5.2
>>
>> pretty_url():
>> This function when passed a transport url will anonymize the transport
>> of the url. It will strip a trailing '/'. It will also strip a trailing
>> '.git'. It will return the newly formated url for use. I do not believe
>> there is a need for stripping the trailing '/' and '.git' from a url,
>> but it was already there and I wanted to make as little changes as
>> possible.
>
> OK.  I tend to agree that stripping the trailing part is probably
> not a good idea and we would want to remove that but that definitely
> should be done as a separate step, or even as a separate series on
> top of this one.

I think that removing the trailing part will greatly reduce the complexity
to the point were it is unnecessary to have pretty_url().  My goal with
extracting this function is to isolate the complexity of formatting the
url to a single spot. I am thinking along the lines of the following
commit order:

1. Remove the "remove trailing part"
2. Add print_url()
3. Always print url when pruning
4. Reverse order of prune and fetch

>> print_url():
>> This function will convert a transport url to a pretty url using
>> pretty_url(). Then it will print out the pretty url to stderr as
>> indicated above in the example output. It uses a global variable
>> named "gshown_url' to prevent this header for being printed twice.
>
> Gaah.  What is that 'g' doing there?  Please don't do that
> meaningless naming.

I am not familiar with C conventions and I was trying to stay consistent.
I saw other global variables starting with 'g' and made an assumption.
It will use the original name in the upcoming patches.

> I do not think the change to introduce such a global variable
> belongs to this refactoring step.  The current caller can decide
> itself if it called that function, and if you are going to introduce
> new callers in later steps, they can coordinate among themselves,
> no?

I agree, there is no reason for introducing it in this step. Thanks for
pointing that out.