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.

Sign up for GitHub

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

Prune only branch #1127

Closed
wants to merge 5 commits into from
Closed

Prune only branch #1127

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b0b7c7f
tests/prune: Factor out a helper for counting commits in the repo
cgwalters Sep 1, 2017
65258d3
bin/prune: Add --only-branch
cgwalters Sep 1, 2017
f962e2c
fixup! tests/prune: Factor out a helper for counting commits in the repo
cgwalters Sep 12, 2017
8ec80eb
fixup! bin/prune: Add --only-branch
cgwalters Sep 12, 2017
03aeae7
fixup! bin/prune: Add --only-branch
cgwalters Sep 15, 2017
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
11 changes: 5 additions & 6 deletions src/ostree/ot-builtin-prune.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,20 +255,19 @@ ostree_builtin_prune (int argc, char **argv, GCancellable *cancellable, GError *
for (char **iter = opt_only_branches; iter && *iter; iter++)
{
const char *ref = *iter;
g_autofree char *commit = NULL;
/* Ensure the specified branch exists */
if (!ostree_repo_resolve_rev (repo, ref, FALSE, &commit, error))
if (!ostree_repo_resolve_rev (repo, ref, FALSE, NULL, error))
return FALSE;
g_hash_table_add (only_branches_set, *iter);
if (g_hash_table_contains (retain_branch_depth, ref))
return glnx_throw (error, "--retain-branch-depth conflicts with --only-branch");
Copy link
Member

Choose a reason for hiding this comment

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

We want this bit down there no? I.e. we do want to be able to specify --retain-branch-depth=my-ref=0 --only-branch my-ref, right?

I was targeting the g_hash_table_insert below instead:

GLNX_HASH_TABLE_FOREACH (all_refs, const char *, ref)
  {
    if (!g_hash_table_contains (only_branches_set, ref))
      {
        if (!g_hash_table_insert (retain_branch_depth, g_strdup (ref), GINT_TO_POINTER ((int)-1)))
          return glnx_throw (error, "--retain-branch-depth conflicts with --only-branch");
      }
  }

Copy link
Member Author

@cgwalters cgwalters Sep 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Hum...so a high level design I had here was that with --only-branch one just uses the global --depth. In other words: --retain-branch-depth=my-ref=0 --only-branch my-ref is equivalent to --refs-only --only-branch my-ref right?

I'm not quite sure when one would want to combine --only-branch with different depths.

Copy link
Member

Choose a reason for hiding this comment

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

Don't we want to support e.g. --only-branch foo --only-branch bar ? So then you have to resort to --retain-branch-depth if you want them pruned at different depths, no? Well, they can just use separate invocations I suppose (or use the API directly). Though it seems easy enough to support. Otherwise, we should just error out if any --retain-branch-depth is passed at all when --only-branch is given I'd say. And also, remove the related comment block:

+      /* Process --only-branch. Note this combines with --retain-branch-depth; one
+       * could do e.g.:
+       *  * --only-branch exampleos/x86_64/foo
+       *  * --retain-branch-depth exampleos/x86_64/foo=0
+       * to prune exampleos/x86_64/foo to just the latest commit.
+       */

Copy link
Member Author

@cgwalters cgwalters Sep 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I think your original comment here was right - I don't see a reason not to support combining the two, even with different branches. Thanks for sticking with this...another fixup ⬇️

g_hash_table_add (only_branches_set, (char*)ref);
}

/* Iterate over all refs, add equivalent of --retain-branch-depth=$ref=-1
* if the ref isn't in --only-branch set.
*/
g_hash_table_iter_init (&hash_iter, all_refs);
while (g_hash_table_iter_next (&hash_iter, &key, &value))
GLNX_HASH_TABLE_FOREACH (all_refs, const char *, ref)
{
const char *ref = key;
if (!g_hash_table_contains (only_branches_set, ref))
g_hash_table_insert (retain_branch_depth, g_strdup (ref), GINT_TO_POINTER ((int)-1));
Copy link
Member

Choose a reason for hiding this comment

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

Should we be erroring in case the ref is already in the hash table? E.g. someone typing --only-branch foo --retain-branch-depth=bar=5 might not understand what the command is doing.

}
Expand Down