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

Prune only branch #1127

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@cgwalters
Member

cgwalters commented Sep 1, 2017

No description provided.

@cgwalters

This comment has been minimized.

Member

cgwalters commented Sep 1, 2017

Depends: #1124

@rh-atomic-bot

This comment has been minimized.

Collaborator

rh-atomic-bot commented Sep 1, 2017

☔️ The latest upstream changes (presumably 75f24b3) made this pull request unmergeable. Please resolve the merge conflicts.

@jlebon

This comment has been minimized.

Member

jlebon commented Sep 5, 2017

This needs a rebase.

@cgwalters cgwalters force-pushed the cgwalters:prune-only-branch branch from 50525a2 to e34f7fd Sep 7, 2017

@cgwalters

This comment has been minimized.

Member

cgwalters commented Sep 7, 2017

Rebased! 🏄

find ${repo}/objects -name '*.commit' | wc -l > commitcount
assert_file_has_content commitcount '^'${count}'$'
rm -f commitcount

This comment has been minimized.

@jlebon

jlebon Sep 8, 2017

Member

How about:

[[ $(find ... | wc -l) == ${count} ]]

?

* 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))

This comment has been minimized.

@jlebon

jlebon Sep 8, 2017

Member

GLNX_HASH_TABLE_FOREACH ?

/* Ensure the specified branch exists */
if (!ostree_repo_resolve_rev (repo, ref, FALSE, &commit, error))
return FALSE;
g_hash_table_add (only_branches_set, *iter);

This comment has been minimized.

@jlebon

jlebon Sep 8, 2017

Member

s/*iter/ref/ ?

for (char **iter = opt_only_branches; iter && *iter; iter++)
{
const char *ref = *iter;
g_autofree char *commit = NULL;

This comment has been minimized.

@jlebon

jlebon Sep 8, 2017

Member

We can just pass NULL right?

@@ -49,6 +50,7 @@ static GOptionEntry options[] = {
{ "keep-younger-than", 0, 0, G_OPTION_ARG_STRING, &opt_keep_younger_than, "Prune all commits older than the specified date", "DATE" },
{ "static-deltas-only", 0, 0, G_OPTION_ARG_NONE, &opt_static_deltas_only, "Change the behavior of delete-commit and keep-younger-than to prune only static deltas" },
{ "retain-branch-depth", 0, 0, G_OPTION_ARG_STRING_ARRAY, &opt_retain_branch_depth, "Additionally retain BRANCH=DEPTH commits", "BRANCH=DEPTH" },
{ "only-branch", 0, 0, G_OPTION_ARG_STRING_ARRAY, &opt_only_branches, "Only prune BRANCH (may be specified multiple times)", "BRANCH" },

This comment has been minimized.

@jlebon

jlebon Sep 8, 2017

Member

Have you thought about making these positional args, e.g. just ostree prune foo bar? Seems more natural.

Hmm, although git prune foo bar has the opposite meaning:

<head>...
    In addition to objects reachable from any of our references, keep objects reachable from listed <head>s.

which I find odd, but if in case people are used to that, I'm fine with leaving it as a switch!

This comment has been minimized.

@cgwalters

cgwalters Sep 12, 2017

Member

Mmm...not sure about promoting this to the toplevel quite yet.

{
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));

This comment has been minimized.

@jlebon

jlebon Sep 8, 2017

Member

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.

cgwalters added some commits Sep 1, 2017

bin/prune: Add --only-branch
In 5c94098 / #646 we
added `--retain-branch-depth`; this adds a symmetric
`--only-branch` for the case where a repo owner just
wants to prune a specific branch.

The implementation here is pretty straightforward; we
just walk all refs and inject the equivalent of
`--retain-branch-depth=$ref=-1` if they're *not* in
`--only-branch`.

Closes: #1115
tests/prune: Factor out a helper for counting commits in the repo
We had lots of duplicates; prep for adding more tests.

@cgwalters cgwalters force-pushed the cgwalters:prune-only-branch branch from e34f7fd to 8ec80eb Sep 12, 2017

@cgwalters

This comment has been minimized.

Member

cgwalters commented Sep 12, 2017

Fixups ⬆️

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");

This comment has been minimized.

@jlebon

jlebon Sep 14, 2017

Member

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");
      }
  }

This comment has been minimized.

@cgwalters

cgwalters Sep 15, 2017

Member

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.

This comment has been minimized.

@jlebon

jlebon Sep 15, 2017

Member

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.
+       */

This comment has been minimized.

@cgwalters

cgwalters Sep 15, 2017

Member

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 ⬇️

@jlebon

This comment has been minimized.

Member

jlebon commented Sep 18, 2017

@jlebon jlebon added homu/approved and removed reviewed labels Sep 18, 2017

@rh-atomic-bot

This comment has been minimized.

Collaborator

rh-atomic-bot commented Sep 18, 2017

⌛️ Testing commit 03aeae7 with merge ab20049...

rh-atomic-bot added a commit that referenced this pull request Sep 18, 2017

bin/prune: Add --only-branch
In 5c94098 / #646 we
added `--retain-branch-depth`; this adds a symmetric
`--only-branch` for the case where a repo owner just
wants to prune a specific branch.

The implementation here is pretty straightforward; we
just walk all refs and inject the equivalent of
`--retain-branch-depth=$ref=-1` if they're *not* in
`--only-branch`.

Closes: #1115

Closes: #1127
Approved by: jlebon
@rh-atomic-bot

This comment has been minimized.

Collaborator

rh-atomic-bot commented Sep 18, 2017

☀️ Test successful - status-atomicjenkins
Approved by: jlebon
Pushing ab20049 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment