Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace remote change op #1166

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions bash/ostree
Original file line number Diff line number Diff line change
Expand Up @@ -985,6 +985,7 @@ _ostree_remote_add() {
local boolean_options="
$main_boolean_options
--if-not-exists
--force
--no-gpg-verify
"

Expand Down
16 changes: 16 additions & 0 deletions man/ostree-remote.xml
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,22 @@ Boston, MA 02111-1307, USA.
</para></listitem>
</varlistentry>

<varlistentry>
<term><option>--if-not-exists</option></term>

<listitem><para>
Do nothing if the provided remote exists.
</para></listitem>
</varlistentry>

<varlistentry>
<term><option>--force</option></term>

<listitem><para>
Replace the provided remote if it exists.
</para></listitem>
</varlistentry>

<varlistentry>
<term><option>--no-gpg-verify</option></term>

Expand Down
85 changes: 85 additions & 0 deletions src/libostree/ostree-repo.c
Original file line number Diff line number Diff line change
Expand Up @@ -1735,6 +1735,88 @@ ostree_repo_remote_delete (OstreeRepo *self,
return impl_repo_remote_delete (self, NULL, FALSE, name, cancellable, error);
}


static gboolean
impl_repo_remote_replace (OstreeRepo *self,
GFile *sysroot,
const char *name,
const char *url,
GVariant *options,
GCancellable *cancellable,
GError **error)
{
g_return_val_if_fail (name != NULL, FALSE);
g_return_val_if_fail (url != NULL, FALSE);
g_return_val_if_fail (options == NULL || g_variant_is_of_type (options, G_VARIANT_TYPE ("a{sv}")), FALSE);

if (!ostree_validate_remote_name (name, error))
return FALSE;

g_autoptr(GError) local_error = NULL;
g_autoptr(OstreeRemote) remote = _ostree_repo_get_remote (self, name, &local_error);
if (remote == NULL)
{
if (!g_error_matches (local_error, G_IO_ERROR, G_IO_ERROR_NOT_FOUND))
{
g_propagate_error (error, g_steal_pointer (&local_error));
return FALSE;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

g_clear_error (&local_error) here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally I had that, but is it necessary here? It's declared with g_autoptr and never reused in this branch. But I can add it to be sure it doesn't leak.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but at this point the error is set - so we must clear it for the invocation of impl_repo_remote_add() right below, otherwise get could get the overwriting-error error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The call to impl_repo_remote_add uses error, not local_error. local_error is only used to check if the repo exists. The caller's error is used when adding and local_error will be cleared regardless of what happens there. I don't mind clearing the error, but I don't think it's really necessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, you're totally right. That said, I still think it's best practice to clear checked errors right after checking them. Imagine if someone later comes and wants to add another checked call, sees there's already a local_error value and doesn't realize it may have been written earlier.

g_clear_error (&local_error);
if (!impl_repo_remote_add (self, sysroot, FALSE, name, url, options,
cancellable, error))
return FALSE;
}
else
{
/* Replace the entire option group */
if (!g_key_file_remove_group (remote->options, remote->group, error))
return FALSE;

if (g_str_has_prefix (url, "metalink="))
g_key_file_set_string (remote->options, remote->group, "metalink",
url + strlen ("metalink="));
else
g_key_file_set_string (remote->options, remote->group, "url", url);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, at this point we should extract this to a helper I'd say.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I didn't spend a lot of time thinking about refactoring things. I mostly wanted to get the feature and tests in place to see what you thought. But I'll look through again and see if there are clear winners.


if (options != NULL)
keyfile_set_from_vardict (remote->options, remote->group, options);

/* Write out updated settings */
if (remote->file != NULL)
{
gsize length;
g_autofree char *data = g_key_file_to_data (remote->options, &length,
NULL);

if (!g_file_replace_contents (remote->file, data, length,
NULL, FALSE, 0, NULL,
cancellable, error))
return FALSE;
}
else
{
g_autoptr(GKeyFile) config = ostree_repo_copy_config (self);

/* Remove the existing group if it exists */
if (!g_key_file_remove_group (config, remote->group, &local_error))
{
if (!g_error_matches (local_error, G_KEY_FILE_ERROR,
G_KEY_FILE_ERROR_GROUP_NOT_FOUND))
{
g_propagate_error (error, g_steal_pointer (&local_error));
return FALSE;
}
}

ot_keyfile_copy_group (remote->options, config, remote->group);
if (!ostree_repo_write_config (self, config, error))
return FALSE;
}
}

return TRUE;
}

/**
* ostree_repo_remote_change:
* @self: Repo
Expand Down Expand Up @@ -1776,6 +1858,9 @@ ostree_repo_remote_change (OstreeRepo *self,
case OSTREE_REPO_REMOTE_CHANGE_DELETE_IF_EXISTS:
return impl_repo_remote_delete (self, sysroot, TRUE, name,
cancellable, error);
case OSTREE_REPO_REMOTE_CHANGE_REPLACE:
return impl_repo_remote_replace (self, sysroot, name, url, options,
cancellable, error);
}
g_assert_not_reached ();
}
Expand Down
4 changes: 3 additions & 1 deletion src/libostree/ostree-repo.h
Original file line number Diff line number Diff line change
Expand Up @@ -166,12 +166,14 @@ gboolean ostree_repo_remote_delete (OstreeRepo *self,
* @OSTREE_REPO_REMOTE_CHANGE_ADD_IF_NOT_EXISTS: Like above, but do nothing if the remote exists
* @OSTREE_REPO_REMOTE_CHANGE_DELETE: Delete a remote
* @OSTREE_REPO_REMOTE_CHANGE_DELETE_IF_EXISTS: Delete a remote, do nothing if the remote does not exist
* @OSTREE_REPO_REMOTE_CHANGE_REPLACE: Add or replace a remote (Since: 2019.1)
*/
typedef enum {
OSTREE_REPO_REMOTE_CHANGE_ADD,
OSTREE_REPO_REMOTE_CHANGE_ADD_IF_NOT_EXISTS,
OSTREE_REPO_REMOTE_CHANGE_DELETE,
OSTREE_REPO_REMOTE_CHANGE_DELETE_IF_EXISTS
OSTREE_REPO_REMOTE_CHANGE_DELETE_IF_EXISTS,
OSTREE_REPO_REMOTE_CHANGE_REPLACE,
} OstreeRepoRemoteChange;

_OSTREE_PUBLIC
Expand Down
21 changes: 18 additions & 3 deletions src/ostree/ot-remote-builtin-add.c
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
static char **opt_set;
static gboolean opt_no_gpg_verify;
static gboolean opt_if_not_exists;
static gboolean opt_force;
static char *opt_gpg_import;
static char *opt_contenturl;
static char *opt_collection_id;
Expand All @@ -45,6 +46,7 @@ static GOptionEntry option_entries[] = {
{ "set", 0, 0, G_OPTION_ARG_STRING_ARRAY, &opt_set, "Set config option KEY=VALUE for remote", "KEY=VALUE" },
{ "no-gpg-verify", 0, 0, G_OPTION_ARG_NONE, &opt_no_gpg_verify, "Disable GPG verification", NULL },
{ "if-not-exists", 0, 0, G_OPTION_ARG_NONE, &opt_if_not_exists, "Do nothing if the provided remote exists", NULL },
{ "force", 0, 0, G_OPTION_ARG_NONE, &opt_force, "Replace the provided remote if it exists", NULL },
{ "gpg-import", 0, 0, G_OPTION_ARG_FILENAME, &opt_gpg_import, "Import GPG key from FILE", "FILE" },
{ "contenturl", 0, 0, G_OPTION_ARG_STRING, &opt_contenturl, "Use URL when fetching content", "URL" },
{ "collection-id", 0, 0, G_OPTION_ARG_STRING, &opt_collection_id,
Expand Down Expand Up @@ -84,6 +86,14 @@ ot_remote_builtin_add (int argc, char **argv, OstreeCommandInvocation *invocatio
goto out;
}

if (opt_if_not_exists && opt_force)
{
ot_util_usage_error (context,
"Can only specify one of --if-not-exists and --force",
error);
goto out;
}

remote_name = argv[1];
remote_url = argv[2];

Expand Down Expand Up @@ -135,9 +145,14 @@ ot_remote_builtin_add (int argc, char **argv, OstreeCommandInvocation *invocatio

options = g_variant_ref_sink (g_variant_builder_end (optbuilder));

if (!ostree_repo_remote_change (repo, NULL,
opt_if_not_exists ? OSTREE_REPO_REMOTE_CHANGE_ADD_IF_NOT_EXISTS :
OSTREE_REPO_REMOTE_CHANGE_ADD,
OstreeRepoRemoteChange changeop;
if (opt_if_not_exists)
changeop = OSTREE_REPO_REMOTE_CHANGE_ADD_IF_NOT_EXISTS;
else if (opt_force)
changeop = OSTREE_REPO_REMOTE_CHANGE_REPLACE;
else
changeop = OSTREE_REPO_REMOTE_CHANGE_ADD;
if (!ostree_repo_remote_change (repo, NULL, changeop,
remote_name, remote_url,
options,
cancellable, error))
Expand Down
14 changes: 13 additions & 1 deletion tests/test-remote-add.sh
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ set -euo pipefail

. $(dirname $0)/libtest.sh

echo '1..14'
echo '1..16'

setup_test_repository "bare"
$OSTREE remote add origin http://example.com/ostree/gnome
Expand Down Expand Up @@ -106,3 +106,15 @@ assert_not_file_has_content list.txt "origin"
# Can't grep for 'another' because of 'another-noexist'
assert_file_has_content list.txt "another-noexist"
echo "ok remote list remaining"

# Both --if-not-exists and --force cannot be used
if $OSTREE remote add --if-not-exists --force yetanother http://yetanother.com/repo 2>/dev/null; then
assert_not_reached "Adding remote with --if-not-exists and --force unexpectedly succeeded"
fi
echo "ok remote add fail --if-not-exists and --force"

# Overwrite with --force
$OSTREE remote add --force another http://another.example.com/anotherrepo
$OSTREE remote list --show-urls > list.txt
assert_file_has_content list.txt "^another \+http://another.example.com/anotherrepo$"
echo "ok remote add --force"
56 changes: 53 additions & 3 deletions tests/test-remotes-config-dir.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ function assertNotEquals(a, b) {
throw new Error("assertion failed " + JSON.stringify(a) + " != " + JSON.stringify(b));
}

print('1..6')
print('1..9')

let remotesDir = Gio.File.new_for_path('remotes.d');
remotesDir.make_directory(null);
Expand All @@ -45,6 +45,10 @@ remoteConfig.set_value('remote "foo"', 'url', 'http://foo')
let remoteConfigFile = remotesDir.get_child('foo.conf')
remoteConfig.save_to_file(remoteConfigFile.get_path())

let remoteOptions = GLib.Variant.new('a{sv}', {
'branches': GLib.Variant.new('as', ['test']),
});

// Use the full Repo constructor to set remotes-config-dir
let repoFile = Gio.File.new_for_path('repo');
let repo = new OSTree.Repo({path: repoFile,
Expand All @@ -60,7 +64,7 @@ print("ok read-remotes-config-dir");

// Adding a remote should not go in the remotes.d dir unless this is a
// system repo or add-remotes-config-dir is set to true
repo.remote_add('bar', 'http://bar', null, null);
repo.remote_add('bar', 'http://bar', remoteOptions, null);
remotes = repo.remote_list()
assertNotEquals(remotes.indexOf('bar'), -1);
assertEquals(remotesDir.get_child('bar.conf').query_exists(null), false);
Expand All @@ -81,7 +85,7 @@ let repoConfig = repo.copy_config();
repoConfig.set_boolean('core', 'add-remotes-config-dir', true);
repo.write_config(repoConfig);
repo.reload_config(null);
repo.remote_add('baz', 'http://baz', null, null);
repo.remote_add('baz', 'http://baz', remoteOptions, null);
remotes = repo.remote_list()
assertNotEquals(remotes.indexOf('baz'), -1);
assertEquals(remotesDir.get_child('baz.conf').query_exists(null), true);
Expand Down Expand Up @@ -114,3 +118,49 @@ try {
}

print("ok config-remote-in-config-dir-fails");

// Replacing a non-existent remote should succeed. This should go in the
// config dir since add-remote-config-dir is set to true above
repo.remote_change(null, OSTree.RepoRemoteChange.REPLACE,
'nonexistent', 'http://nonexistent',
null, null);
remotes = repo.remote_list();
assertNotEquals(remotes.indexOf('nonexistent'), -1);
assertEquals(remotesDir.get_child('nonexistent.conf').query_exists(null), true);

print("ok replace-missing-remote-succeeds");

// Test replacing remote options in config dir. This should remove the
// branches setting above
repo.remote_change(null, OSTree.RepoRemoteChange.REPLACE, 'baz',
'http://baz2', null, null);
remoteConfigFile = remotesDir.get_child('baz.conf');
remoteConfig = GLib.KeyFile.new();
remoteConfig.load_from_file(remoteConfigFile.get_path(),
GLib.KeyFileFlags.NONE);
assertEquals(remoteConfig.get_value('remote "baz"', 'url'), 'http://baz2');
try {
remoteConfig.get_string_list('remote "baz"', 'branches');
throw new Error('baz remote should not have branches option');
} catch (e) {
if (!(e.matches(GLib.KeyFileError, GLib.KeyFileError.KEY_NOT_FOUND)))
throw e;
}

print("ok replace-remote-in-config-dir");

// Test replacing remote options in config file. This should remove the
// branches setting above
repo.remote_change(null, OSTree.RepoRemoteChange.REPLACE, 'bar',
'http://bar2', null, null);
repoConfig = repo.get_config();
assertEquals(repoConfig.get_value('remote "bar"', 'url'), 'http://bar2');
try {
repoConfig.get_string_list('remote "bar"', 'branches');
throw new Error('bar remote should not have branches option');
} catch (e) {
if (!(e.matches(GLib.KeyFileError, GLib.KeyFileError.KEY_NOT_FOUND)))
throw e;
}

print("ok replace-remote-in-config-file");