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

2013-12-19 Thread Thomas Miller
On Wed, Dec 18, 2013 at 7:18 PM, Tom Miller jacker...@gmail.com wrote:
 On Wed, Dec 18, 2013 at 3:47 PM, Junio C Hamano gits...@pobox.com wrote:
 Tom Miller jacker...@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.

After working on this some more and realizing there is more work to
be done on the fetch --prune should prune before fetching issue. Also,
seeing Jeff's response opened my eyes even more. I believe you are
correct. The trailing parts piece should be split off into another patch set.
I think it would make sense to add the fetch --prune should print the header
url to that patch set. Should I submit those patches as a separate thread
or reply to this thread with just those patches?

-- 
Tom Miller
jacker...@gmail.com
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2013-12-18 Thread Junio C Hamano
Tom Miller jacker...@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.

 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 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?

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2013-12-18 Thread Tom Miller
On Wed, Dec 18, 2013 at 3:47 PM, Junio C Hamano gits...@pobox.com wrote:
 Tom Miller jacker...@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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html