| Message ID | 20210117234244.95106-3-rafaeloliveira.cs@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | teach `worktree list` verbose mode and prunable annotations | expand |
On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva <rafaeloliveira.cs@gmail.com> wrote: > Add worktree_prune_reason() to allow a caller to discover whether a > worktree is prunable and the reason that it is, much like > worktree_lock_reason() indicates whether a worktree is locked and the > reason for the lock. As with worktree_lock_reason(), retrieve the > prunable reason lazily and cache it in the `worktree` structure. > > Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com> > --- > diff --git a/worktree.c b/worktree.c > @@ -245,6 +246,25 @@ const char *worktree_lock_reason(struct worktree *wt) > +const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire) > +{ > + struct strbuf reason = STRBUF_INIT; > + char *path; The `path` variable is uninitialized... > + if (should_prune_worktree(wt->id, &reason, &path, expire)) > + wt->prune_reason = strbuf_detach(&reason, NULL); ...but the very first thing should_prune_worktree() does is unconditionally set it to NULL, so it's either NULL or an allocated string regardless of the value returned by should_prune_worktree()... > + strbuf_release(&reason); > + free(path); ...which makes the behavior of `free(path)` deterministic. Good. I'm on the fence about whether or not we should initialize `path` to NULL: char *path = NULL; just to make it easier for the next person to reason about it without having to dig into the implementation of should_prune_worktree(). This is a really minor point, though, not worth a re-roll.
Eric Sunshine writes: > On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva > <rafaeloliveira.cs@gmail.com> wrote: >> Add worktree_prune_reason() to allow a caller to discover whether a >> worktree is prunable and the reason that it is, much like >> worktree_lock_reason() indicates whether a worktree is locked and the >> reason for the lock. As with worktree_lock_reason(), retrieve the >> prunable reason lazily and cache it in the `worktree` structure. >> >> Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com> >> --- >> diff --git a/worktree.c b/worktree.c >> @@ -245,6 +246,25 @@ const char *worktree_lock_reason(struct worktree *wt) >> +const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire) >> +{ >> + struct strbuf reason = STRBUF_INIT; >> + char *path; > > The `path` variable is uninitialized... > >> + if (should_prune_worktree(wt->id, &reason, &path, expire)) >> + wt->prune_reason = strbuf_detach(&reason, NULL); > > ...but the very first thing should_prune_worktree() does is > unconditionally set it to NULL, so it's either NULL or an allocated > string regardless of the value returned by should_prune_worktree()... > >> + strbuf_release(&reason); >> + free(path); > > ...which makes the behavior of `free(path)` deterministic. Good. > > I'm on the fence about whether or not we should initialize `path` to NULL: > > char *path = NULL; > > just to make it easier for the next person to reason about it without > having to dig into the implementation of should_prune_worktree(). This > is a really minor point, though, not worth a re-roll. This is a good point and totally agreed. I'm going to include this change in the next revision.
diff --git a/worktree.c b/worktree.c index 8ae019af79..474ed46562 100644 --- a/worktree.c +++ b/worktree.c @@ -15,6 +15,7 @@ void free_worktrees(struct worktree **worktrees) free(worktrees[i]->id); free(worktrees[i]->head_ref); free(worktrees[i]->lock_reason); + free(worktrees[i]->prune_reason); free(worktrees[i]); } free (worktrees); @@ -245,6 +246,25 @@ const char *worktree_lock_reason(struct worktree *wt) return wt->lock_reason; } +const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire) +{ + struct strbuf reason = STRBUF_INIT; + char *path; + + if (is_main_worktree(wt)) + return NULL; + if (wt->prune_reason_valid) + return wt->prune_reason; + + if (should_prune_worktree(wt->id, &reason, &path, expire)) + wt->prune_reason = strbuf_detach(&reason, NULL); + wt->prune_reason_valid = 1; + + strbuf_release(&reason); + free(path); + return wt->prune_reason; +} + /* convenient wrapper to deal with NULL strbuf */ static void strbuf_addf_gently(struct strbuf *buf, const char *fmt, ...) { diff --git a/worktree.h b/worktree.h index 818e1491c7..8b7c408132 100644 --- a/worktree.h +++ b/worktree.h @@ -11,11 +11,13 @@ struct worktree { char *id; char *head_ref; /* NULL if HEAD is broken or detached */ char *lock_reason; /* private - use worktree_lock_reason */ + char *prune_reason; /* private - use worktree_prune_reason */ struct object_id head_oid; int is_detached; int is_bare; int is_current; int lock_reason_valid; /* private */ + int prune_reason_valid; /* private */ }; /* @@ -73,6 +75,13 @@ int is_main_worktree(const struct worktree *wt); */ const char *worktree_lock_reason(struct worktree *wt); +/* + * Return the reason string if the given worktree should be pruned, otherwise + * NULL if it should not be pruned. `expire` defines a grace period to prune + * the worktree when its path does not exist. + */ +const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire); + /* * Return true if worktree entry should be pruned, along with the reason for * pruning. Otherwise, return false and the worktree's path in `wtpath`, or
Add worktree_prune_reason() to allow a caller to discover whether a worktree is prunable and the reason that it is, much like worktree_lock_reason() indicates whether a worktree is locked and the reason for the lock. As with worktree_lock_reason(), retrieve the prunable reason lazily and cache it in the `worktree` structure. Helped-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com> --- worktree.c | 20 ++++++++++++++++++++ worktree.h | 9 +++++++++ 2 files changed, 29 insertions(+)