Re: [PATCH 1/3] builtin/fetch.c: Add pretty_url() and print_url()
On Wed, Dec 18, 2013 at 7:18 PM, Tom Miller wrote: > On Wed, Dec 18, 2013 at 3:47 PM, Junio C Hamano wrote: >> Tom Miller 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()
On Wed, Dec 18, 2013 at 3:47 PM, Junio C Hamano wrote: > Tom Miller 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
Re: [PATCH 1/3] builtin/fetch.c: Add pretty_url() and print_url()
Tom Miller 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
[PATCH 1/3] builtin/fetch.c: Add pretty_url() and print_url()
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.
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.
Signed-off-by: Tom Miller
---
builtin/fetch.c | 60 -
1 file changed, 42 insertions(+), 18 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 3d978eb..b3145f6 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -44,6 +44,42 @@ static struct transport *gtransport;
static struct transport *gsecondary;
static const char *submodule_prefix = "";
static const char *recurse_submodules_default;
+static int gshown_url = 0;
+
+static char *pretty_url(const char *raw_url) {
+ if (raw_url) {
+ int url_len, i;
+ char *pretty_url, *url;
+
+ url = transport_anonymize_url(raw_url);
+
+ url_len = strlen(url);
+ for (i = url_len - 1; url[i] == '/' && 0 <= i; i--)
+ ;
+ url_len = i + 1;
+ if (4 < i && !strncmp(".git", url + i - 3, 4))
+ url_len = i - 3;
+
+ pretty_url = xcalloc(1, 1 + url_len);
+ memcpy(pretty_url, url, url_len);
+
+ free(url);
+ return pretty_url;
+ }
+ return xstrdup("foreign");
+}
+
+static void print_url(const char *raw_url) {
+ if (!gshown_url) {
+ char *url = pretty_url(raw_url);
+
+ fprintf(stderr, _("From %s\n"), url);
+
+ gshown_url = 1;
+ free(url);
+ }
+}
+
static int option_parse_recurse_submodules(const struct option *opt,
const char *arg, int unset)
@@ -535,7 +571,7 @@ static int store_updated_refs(const char *raw_url, const
char *remote_name,
{
FILE *fp;
struct commit *commit;
- int url_len, i, shown_url = 0, rc = 0;
+ int url_len, i, rc = 0;
struct strbuf note = STRBUF_INIT;
const char *what, *kind;
struct ref *rm;
@@ -546,10 +582,8 @@ static int store_updated_refs(const char *raw_url, const
char *remote_name,
if (!fp)
return error(_("cannot open %s: %s\n"), filename,
strerror(errno));
- if (raw_url)
- url = transport_anonymize_url(raw_url);
- else
- url = xstrdup("foreign");
+ url = pretty_url(raw_url);
+ url_len = strlen(url);
rm = ref_map;
if (check_everything_connected(iterate_ref_map, 0, &rm)) {
@@ -606,13 +640,6 @@ static int store_updated_refs(const char *raw_url, const
char *remote_name,
what = rm->name;
}
- url_len = strlen(url);
- for (i = url_len - 1; url[i] == '/' && 0 <= i; i--)
- ;
- url_len = i + 1;
- if (4 < i && !strncmp(".git", url + i - 3, 4))
- url_len = i - 3;
-
strbuf_reset(¬e);
if (*what) {
if (*kind)
@@ -651,13 +678,10 @@ static int store_updated_refs(const char *raw_url, const
char *remote_name,
REFCOL_WIDTH,
*what ? what : "HEAD");
if (note.len) {
- if (verbosity >= 0 && !shown_url) {
- fprintf(stderr, _("From %.*s\n"),
- url_len, url);
- shown_url = 1;
- }
- if (verbosity >= 0)
+
