GNOME Bugzilla – Bug 606792
"push branch to" menu does not look at configured pushes
Last modified: 2010-05-29 19:29:02 UTC
Created attachment 151294 [details] [review] Scan through remotes.*.push and add them to the menu A remote can have a "push" configured.. like this. [remote "Intranet01"] url = app01:portals/intranet/modules/Tickets fetch = +refs/heads/master:refs/remotes/Intranet01/master push = refs/heads/2008-Testing:refs/heads/master [remote "Intranet02"] url = app02:portals/intranet/modules/Tickets fetch = +refs/heads/master:refs/remotes/Intranet02/master push = refs/heads/2008-Testing:refs/heads/master This allows for git push Intranet01 to push the 2008-Testing branch to the "master" branch on the remote end (in this case a deployed web application with a post-update hook to checkout the pushed branch point) currently gitg does NOT look at those fields and instead suggests I create "new" branches for those remotes. The attached patch scans through the remote pushes and adds them to the menu just after the tracking branch.
Review of attachment 151294 [details] [review]: I've been thinking about supporting this, but then forgot again. So I'm happy to see the patch, there are some comments inline though. ::: gitg/gitg-repository.c @@ +1651,3 @@ } +gitg_repository_get_remotes_pushed (GitgRepository *repository, GitgRef *ref) +gchar ** I don't like the name so much (it doesn't really describe it well). Maybe it should be gitg_repository_get_ref_pushes? @@ +1659,3 @@ +{ +gitg_repository_get_remotes_pushed (GitgRepository *repository, GitgRef *ref) +gchar ** Use GPtrArray instead @@ +1681,3 @@ + gchar *remote = g_match_info_fetch (info, 1); + gchar *localref = g_match_info_fetch (info, 2); + gchar *branch = g_match_info_fetch (info, 3); Here you are leaking remote, localref and branch (they should be freed with g_free) @@ +1683,3 @@ + gchar *branch = g_match_info_fetch (info, 3); + + if (g_str_equal( gitg_ref_get_name(ref), localref)) Small whitespace error @@ +1687,3 @@ + gchar *push = g_strconcat (remote, "/", branch, NULL); + remotes = g_realloc (remotes, sizeof(gchar *) * (++num + 1)); + remotes[num - 1] = push; Use GPtrArray instead @@ +1697,3 @@ + remotes[num] = NULL; + g_object_unref (config); + return remotes; You're leaking ret and lines here ::: gitg/gitg-window.c @@ +2084,3 @@ + { + if (remotes_pushed) + /* Get remote pushing branches */ It's a bit strange to first combine remote/branch in gitg_repository_get_remotes_pushed, and then break it apart here. I think it would be better to have gitg_repository_get_remotes_pushed return a list of GitgRef and use gitg_ref_get_prefix and gitg_ref_get_local_name instead.
I agree on the name it is silly and not very obvious, I just couldn't think of anything else. Yeah.. Lekas... This is my first time really playing with glib/gtk+ in recent years (I played SOME with it back in gimp 1.0 days.) That and I was spending more time writing using APR which I never have to worry much about freeing memory:-D I was thinking the same thing about the return value of having it return a GitgRef array instead, but was just getting it working.. then I'll clean up. I'll send a re-vamped patch here in a few days. (oh.. love the git. makes managing this SOO much easier)
Created attachment 151524 [details] [review] Cleaned up patch fix various memory leaks rename gitg_repository_get_remotes_pushed to gitg_repository_get_ref_pushes make the gitg_repository_get_ref_pushes function return an array of GitgRefs instead of a string array.
Created attachment 151525 [details] [review] suggested rewrite for gitg_ref_get_localname Now.. I do have one question.. the function gitg_ref_get_localname is different from the rest of the gitg_ref_get_* functions in that it requires you to free the result. I've attached a patch with a suggested rewrite that keeps the style consistent with the rest of the gig_ref_get_* functions.
oops.. RE attachment #151524 [details] I don't need to add the repository_has_branch function anymore..
Review of attachment 151525 [details] [review]: I think it's a good idea in general, but: 1) Please mark the return value of that function as const 2) Please include in the patch the removal of freeing the result of that function in the current code
Review of attachment 151524 [details] [review]: Some minor things in gitg-repository. The code in gitg-window doesn't seem very much correct at the moment. ::: gitg/gitg-repository.c @@ +1671,3 @@ +{ +gitg_repository_get_ref_pushes (GitgRepository *repository, GitgRef *ref) +char ** Still need to free ret here @@ +1689,3 @@ + + gchar *rr = g_strconcat ("refs/remotes/", remote, "/", branch, NULL); + GitgRef *remref = gitg_ref_new ("0000000000000000000000000000000000000000", rr); It's a bit weird to use the 0000 hash here. I know it's not really needed for the use case you're implemented, but it's better if the ref is valid. You can lookup the hash by iterating over the refs hashtable and comparing the name. ::: gitg/gitg-window.c @@ +2077,3 @@ + { + if (ref_pushes) + /* Get remote pushing branches */ Something not quite right here. I guess this is a left over from the previous patch?
Created attachment 151580 [details] [review] rewrite of gitg_ref_get_localname Here it is making it const and removing all the other "frees"
Created attachment 151581 [details] [review] re-re-cleaned up patch for scanning configured ref pushes re: comment #7 only reason I used "0000000".... is because it was used in gitg-branch-actions.c line 1447 for creating a remote Ref pointer. Also for the sake of building the push menu I don't need the hash and in some cases it may not exist at all. (ie. a new Ref) so it seems less efficient to scan through to find the ref that may not exist. Oh, I found more leaks :(
I found the time to cleanup the patch. I've pushed this to master now. There are some changes to your original patch (some trivial, like the return type of the added function, and some less trivial, like a cache). Anyway, thanks for the patch!