| Message ID | 20210104162128.95281-3-rafaeloliveira.cs@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | teach `worktree list` verbose mode and prunable annotations | expand |
On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva <rafaeloliveira.cs@gmail.com> wrote: > worktree: implement worktree_prune_reason() wrapper We might be able to give the reader more useful information in the subject by explaining a bit more the goal of this patch. Perhaps something like this: worktree: teach worktree to lazy-load "prunable" reason > The should_prune_worktree() machinery is used by the "prune" command to > identify whether a worktree is a candidate for pruning. This function > however, is not prepared to work directly with "struct worktree" and > refactoring is required not only on the function itself, but also also > changing get_worktrees() to return non-valid worktrees and address the > changes in all "worktree" sub commands. > > Instead let's implement worktree_prune_reason() that accepts > "struct worktree" and uses should_prune_worktree() and returns whether > the given worktree is a candidate for pruning. As the "list" sub command > already uses a list of "struct worktree", this allow to simply check if > the working tree prunable by passing the structure directly without the > others parameters. Everything through "not prepared to work directly with `struct worktree`" makes sense when explaining why you are adding this wrapper function, however, most of what follows is describing an aborted idea about how you originally intended to implement this. The bit about get_worktrees() not being able to return non-valid worktrees is certainly an important limitation in the overall scheme of things, but doesn't really help to sell the change made by this patch, and it probably confuses the reader who didn't also read the cover-letter, especially since this patch does nothing to help that situation. It also isn't really necessary to talk about `git worktree list` at this stage since this patch stands on its own by fleshing out the API without having to cite a specific client. > Also, let's add prune_reason field to the worktree structure that will > store the reason why the worktree can be pruned that is returned by > should_prune_worktree() when such reason is available. In my opinion, this is the real reason this patch exists, thus should be the focus of the commit message. All the description above this paragraph can likely be dropped. Taking the above comments into account, perhaps the entire commit message could be collapsed to something like this: worktree: teach worktree to lazy-load "prunable" reason 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. > diff --git a/worktree.c b/worktree.c > @@ -15,6 +15,7 @@ void free_worktrees(struct worktree **worktrees) > free(worktrees[i]->lock_reason); > + free(worktrees[i]->prune_reason); > free(worktrees[i]); Remembering to free the prune-reason. Good. > @@ -245,6 +246,24 @@ const char *worktree_lock_reason(struct worktree *wt) > +const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire) > +{ > + if (!is_main_worktree(wt)) { > + char *path; > + struct strbuf reason = STRBUF_INIT; > + > + if (should_prune_worktree(wt->id, &reason, &path, expire)) > + wt->prune_reason = strbuf_detach(&reason, NULL); > + else > + wt->prune_reason = NULL; > + > + free(path); > + strbuf_release(&reason); > + } > + > + return wt->prune_reason; > +} A couple observations... I realize you patterned this after worktree_lock_reason(), however, it is more common in this codebase to return early from the function for conditions such as `is_main_worktree(wt)` which don't require any additional processing. One reason is that doing so allows us to lose an indentation level. Another is that it is easier to reason about the rest of the function if we get the simple cases out of the way early, such that we don't have to think about them again while reading the remainder of the code. If I'm not mistaken, the intention here was to cache `prune_reason` for reuse, however, this function just overwrites it each time it's called for a particular worktree, thus providing no caching and leaking the previously-retrieved reason as well. To fix this, I think you need to add a private `prune_reason_valid` member to `struct worktree` (similar to `lock_reason_valid`) and check it before calling should_prune_worktree(). Taking the above comments into account, perhaps it should be written like this: 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; free(path); strbuf_release(&reason); > diff --git a/worktree.h b/worktree.h > @@ -11,6 +11,7 @@ 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 */ As noted above, we also probably need a new `prune_reason_valid` member, similar to the existing `lock_reason_valid`. > @@ -73,6 +74,12 @@ int is_main_worktree(const struct worktree *wt); > +/* > + * Return the reason string if the given worktree should be pruned > + * or NULL otherwise. > + */ > +const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire); The documentation should also talk about `expire` since its purpose and meaning is unclear. Nit: I realize that you patterned this description after the one for worktree_lock_reason(), but it's a bit unclear. Perhaps rephrasing it like this would help: Return the reason the worktree should be pruned, otherwise NULL if it should not be pruned.
Eric Sunshine writes: > On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva > <rafaeloliveira.cs@gmail.com> wrote: >> worktree: implement worktree_prune_reason() wrapper > > We might be able to give the reader more useful information in the > subject by explaining a bit more the goal of this patch. Perhaps > something like this: > > worktree: teach worktree to lazy-load "prunable" reason > yeah, that's sounds better. will change it on the next revision. >> The should_prune_worktree() machinery is used by the "prune" command to >> identify whether a worktree is a candidate for pruning. This function >> however, is not prepared to work directly with "struct worktree" and >> refactoring is required not only on the function itself, but also also >> changing get_worktrees() to return non-valid worktrees and address the >> changes in all "worktree" sub commands. >> >> Instead let's implement worktree_prune_reason() that accepts >> "struct worktree" and uses should_prune_worktree() and returns whether >> the given worktree is a candidate for pruning. As the "list" sub command >> already uses a list of "struct worktree", this allow to simply check if >> the working tree prunable by passing the structure directly without the >> others parameters. > > Everything through "not prepared to work directly with `struct > worktree`" makes sense when explaining why you are adding this wrapper > function, however, most of what follows is describing an aborted idea > about how you originally intended to implement this. The bit about > get_worktrees() not being able to return non-valid worktrees is > certainly an important limitation in the overall scheme of things, but > doesn't really help to sell the change made by this patch, and it > probably confuses the reader who didn't also read the cover-letter, > especially since this patch does nothing to help that situation. > > It also isn't really necessary to talk about `git worktree list` at > this stage since this patch stands on its own by fleshing out the API > without having to cite a specific client. > Make sense. >> Also, let's add prune_reason field to the worktree structure that will >> store the reason why the worktree can be pruned that is returned by >> should_prune_worktree() when such reason is available. > > In my opinion, this is the real reason this patch exists, thus should > be the focus of the commit message. All the description above this > paragraph can likely be dropped. > > Taking the above comments into account, perhaps the entire commit > message could be collapsed to something like this: > > worktree: teach worktree to lazy-load "prunable" reason > > 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. > Interesting point. Rewording the commit message like this seems better, and as you mentioned this patch can stands on its own and can be more clear about what the commit is introducing to the code instead of all the previous message trying to explain why this exists. Thank you for suggesting this commit message. I will revise and change on the next revision. >> diff --git a/worktree.c b/worktree.c >> @@ -15,6 +15,7 @@ void free_worktrees(struct worktree **worktrees) >> free(worktrees[i]->lock_reason); >> + free(worktrees[i]->prune_reason); >> free(worktrees[i]); > > Remembering to free the prune-reason. Good. > >> @@ -245,6 +246,24 @@ const char *worktree_lock_reason(struct worktree *wt) >> +const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire) >> +{ >> + if (!is_main_worktree(wt)) { >> + char *path; >> + struct strbuf reason = STRBUF_INIT; >> + >> + if (should_prune_worktree(wt->id, &reason, &path, expire)) >> + wt->prune_reason = strbuf_detach(&reason, NULL); >> + else >> + wt->prune_reason = NULL; >> + >> + free(path); >> + strbuf_release(&reason); >> + } >> + >> + return wt->prune_reason; >> +} > > A couple observations... > > I realize you patterned this after worktree_lock_reason(), however, it > is more common in this codebase to return early from the function for > conditions such as `is_main_worktree(wt)` which don't require any > additional processing. One reason is that doing so allows us to lose > an indentation level. Another is that it is easier to reason about the > rest of the function if we get the simple cases out of the way early, > such that we don't have to think about them again while reading the > remainder of the code. > > If I'm not mistaken, the intention here was to cache `prune_reason` > for reuse, however, this function just overwrites it each time it's > called for a particular worktree, thus providing no caching and > leaking the previously-retrieved reason as well. To fix this, I think > you need to add a private `prune_reason_valid` member to `struct > worktree` (similar to `lock_reason_valid`) and check it before calling > should_prune_worktree(). > Good point. I totally missed the caching aspect of the implementation and I greed about getting the simple case out earlier in order to make it simple to reason about it. > Taking the above comments into account, perhaps it should be written like this: > > 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; > free(path); > strbuf_release(&reason); > Thanks. I will add this suggestion in the next revision. >> diff --git a/worktree.h b/worktree.h >> @@ -11,6 +11,7 @@ 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 */ > > As noted above, we also probably need a new `prune_reason_valid` > member, similar to the existing `lock_reason_valid`. > Indeed. will add on the next revision. >> @@ -73,6 +74,12 @@ int is_main_worktree(const struct worktree *wt); >> +/* >> + * Return the reason string if the given worktree should be pruned >> + * or NULL otherwise. >> + */ >> +const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire); > > The documentation should also talk about `expire` since its purpose > and meaning is unclear. > Good point. I missed adding the message here for the `expire` parameter, will add it. > Nit: I realize that you patterned this description after the one for > worktree_lock_reason(), but it's a bit unclear. Perhaps rephrasing it > like this would help: > > Return the reason the worktree should be pruned, otherwise > NULL if it should not be pruned. Also make sense to rephrase like this. Thank you for this review will address all this changes on the v2.
diff --git a/worktree.c b/worktree.c index 5764b0dc7c..ee14db3ab5 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,24 @@ const char *worktree_lock_reason(struct worktree *wt) return wt->lock_reason; } +const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire) +{ + if (!is_main_worktree(wt)) { + char *path; + struct strbuf reason = STRBUF_INIT; + + if (should_prune_worktree(wt->id, &reason, &path, expire)) + wt->prune_reason = strbuf_detach(&reason, NULL); + else + wt->prune_reason = NULL; + + free(path); + strbuf_release(&reason); + } + + 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 e5f4320725..24ded0f3c6 100644 --- a/worktree.h +++ b/worktree.h @@ -11,6 +11,7 @@ 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; @@ -73,6 +74,12 @@ 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 + * or NULL otherwise. + */ +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, or NULL if it
The should_prune_worktree() machinery is used by the "prune" command to identify whether a worktree is a candidate for pruning. This function however, is not prepared to work directly with "struct worktree" and refactoring is required not only on the function itself, but also also changing get_worktrees() to return non-valid worktrees and address the changes in all "worktree" sub commands. Instead let's implement worktree_prune_reason() that accepts "struct worktree" and uses should_prune_worktree() and returns whether the given worktree is a candidate for pruning. As the "list" sub command already uses a list of "struct worktree", this allow to simply check if the working tree prunable by passing the structure directly without the others parameters. Also, let's add prune_reason field to the worktree structure that will store the reason why the worktree can be pruned that is returned by should_prune_worktree() when such reason is available. Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com> --- worktree.c | 19 +++++++++++++++++++ worktree.h | 7 +++++++ 2 files changed, 26 insertions(+)