git-bz should ideally have support for requesting credentials from the user and (securely) storing them itself, for handling such situations or unsupported browsers.

Created attachment 306782 [details] [review]
Choose between multiple Firefox profiles.

Current script was choosing only automatically default profile for the cookiefile to be used. Unfortunately, when the user doesn't explicitly set default profile, Firefox sets as default the last profile used, which can make git-bz choosing the wrong profile.

This patch makes git-bz to make the choice explicit and ask in the dialog for the corrent profile.

Created attachment 306784 [details] [review]
Choose between multiple Firefox profiles.

Current script was choosing only automatically default profile for the
cookiefile to be used. Unfortunately, when the user doesn't explicitly
set default profile, Firefox sets as default the last profile used,
which can make git-bz choosing the wrong profile.

This patch makes git-bz to make the choice explicit and ask in the
dialog for the corrent profile.

In case there is only one profile, user is not bothered with anything
and git-bz is as automatic as before.

Review of attachment 306784 [details] [review]:

::: git-bz
@@ +675,3 @@
+        default = 0
+        for p in profiles:
+            is_default = '*' if cp.has_option(p, 'Default') else ' '

Should you not also check that ‘cp.get(p, 'Default').strip() == '1'’?

If the default profile is set, why prompt the user? If the user has set their default profile incorrectly, I think it’s reasonable to require them to use `git config --global bz.ff3-cookie-file blah` manually.

@@ +677,3 @@
+            is_default = '*' if cp.has_option(p, 'Default') else ' '
+            if is_default == '*':
+                default = int(profiles.index(p))

You could use “for idx, p in enumerate(profiles):” instead of re-indexing into profiles.

@@ +698,3 @@
+def get_bugzilla_cookies_ff3(host):
+    if 'cookiefile' in git_config:
+        profile_path = git_config['cookiefile']

Perhaps this should be ff3-cookie-file instead of cookiefile?

@@ +707,3 @@
+        git_config['cookiefile'] = profile_path
+        Popen('git config bz.cookiefile %s' % profile_path,
+              shell=True).wait()

Why persist the default? This means that if the user changes their default Firefox profile from within Firefox, git-bz will not pick up the change.

I think it would be better to get the user to set their default profile in Firefox, or explicitly give them the git-config command which sets ff3-cookie-file after they have chosen a profile in determine_ff3_cookie_file().

@@ +773,3 @@
+            'galeon': get_bugzilla_cookies_galeon,
+            'chromium': get_bugzilla_cookies_chromium,
+            'google-chrome': get_bugzilla_cookies_google_chrome}

Irrelevant whitespace changes.

@@ +784,3 @@
     else:
+        die('Unsupported browser %s (we only support %s)' %
+            (browser, browser_list()))

Please remove all the irrelevant whitespace changes.

(In reply to Philip Withnall from comment #4)
> Review of attachment 306784 [details] [review] [review]:
> 
> ::: git-bz
> @@ +675,3 @@
> +        default = 0
> +        for p in profiles:
> +            is_default = '*' if cp.has_option(p, 'Default') else ' '
> 
> Should you not also check that ‘cp.get(p, 'Default').strip() == '1'’?

Probably, but I don't think Default if exists can have any other value.

> If the default profile is set, why prompt the user? If the user has set
> their default profile incorrectly, I think it’s reasonable to require them
> to use `git config --global bz.ff3-cookie-file blah` manually.

I explained it in the commit message. Default=1 doesn't mean only that the user explicitly marked the profile as default, but when the user (e.g., me ;)) doesn't declare any profile default, Firefox marks as default always the last profile used (so when restarted it is used again). Which means that Default profile can point somewhere else than to the most used profile.

Actually, it is even more complicated ... according to http://kb.mozillazine.org/Profiles.ini_file Default doesn't mean "default" in our sense of the word, but only the last profile used. Also we should acknowledge IsRelative option. And yes, Default doesn't have any other value than 1, it's just a flag.

> @@ +677,3 @@
> +            is_default = '*' if cp.has_option(p, 'Default') else ' '
> +            if is_default == '*':
> +                default = int(profiles.index(p))
> 
> You could use “for idx, p in enumerate(profiles):” instead of re-indexing
> into profiles.

Good point.

> @@ +698,3 @@
> +def get_bugzilla_cookies_ff3(host):
> +    if 'cookiefile' in git_config:
> +        profile_path = git_config['cookiefile']
> 
> Perhaps this should be ff3-cookie-file instead of cookiefile?

Perhaps. I haven't expected people using more than one browser, but probvably a good idea from the defensive programming point of view.

> @@ +707,3 @@
> +        git_config['cookiefile'] = profile_path
> +        Popen('git config bz.cookiefile %s' % profile_path,
> +              shell=True).wait()
> 
> Why persist the default? This means that if the user changes their default
> Firefox profile from within Firefox, git-bz will not pick up the change.

I tried to avoid bothering user too often especially with dialogs.

Perhaps I could print some notice that in such case user could just use 'git config --unset bz.cookiefile'.

> I think it would be better to get the user to set their default profile in
> Firefox, or explicitly give them the git-config command which sets
> ff3-cookie-file after they have chosen a profile in
> determine_ff3_cookie_file().

Of course, if bz.cookiefile (or bz.ff3-cookie-file) is set, then all this mechanism is ignored. However, it is completely undiscoverable.

> @@ +773,3 @@
> +            'galeon': get_bugzilla_cookies_galeon,
> +            'chromium': get_bugzilla_cookies_chromium,
> +            'google-chrome': get_bugzilla_cookies_google_chrome}
> 
> Irrelevant whitespace changes.

Sorry, flake8 complained. I'll stick just to my changes.

Created attachment 306814 [details] [review]
Choose between multiple Firefox profiles.

Current script was choosing only automatically default profile for the
cookiefile to be used. Unfortunately, when the user doesn't explicitly
set default profile, Firefox sets as default the last profile used,
which can make git-bz choosing the wrong profile.

This patch makes git-bz to make the choice explicit and ask in the
dialog for the corrent profile.

I tried the patch but got this error:

Traceback (most recent call last):

+ Trace 235314

Created attachment 308629 [details] [review]
Choose between multiple Firefox profiles.

Current script was choosing only automatically default profile for the
cookiefile to be used. Unfortunately, when the user doesn't explicitly
set default profile, Firefox sets as default the last profile used,
which can make git-bz choosing the wrong profile.

This patch makes git-bz to make the choice explicit and ask in the
dialog for the corrent profile.

Thanks, that worked!

git-bz saw its last code changes three years ago: http://git.fishsoup.net/cgit/git-bz/
GNOME is moving from Bugzilla (the "bz" in "git-bz") to Gitlab.

Closing this report as WONTFIX as part of Bugzilla Housekeeping.

Please feel free to reopen this ticket (or rather transfer the project to GNOME Gitlab, as Bugzilla is deprecated) if active development is about to take place again.