| Message ID | 20210104162128.95281-2-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: move should_prune_worktree() to worktree.c The subject is a bit confusing since this function already lives in `worktree.c` (albeit within the `builtin` directory). An alternate way to word this might be: worktree: libify and publish should_prune_worktree() or just: worktree: libify should_prune_worktree() > As part of teaching "git worktree list" to annotate worktree that is a > candidate for pruning, let's move should_prune_worktree() from > builtin/worktree.c to worktree.c in order to make part of the worktree > public API. Good. > This function will be used by another API function, implemented in the > next patch that will accept a pointer to "worktree" structure directly > making it easier to implement the "prunable" annotations. Okay, though I'm not sure this paragraph adds value since the reader understands implicitly from the first paragraph that this is the reason this patch is libifying the function. (I'd probably drop this paragraph if I was composing the message.) > should_prune_worktree() knows how to select the given worktree for pruning > based on an expiration date, however the expiration value is stored on a > global variable and it is not local to the function. In order to move the > function, teach should_prune_worktree() to take the expiration date as an > argument. Rather than "on a global variable", it would be more accurate to say "static file-scope variable". > diff --git a/worktree.c b/worktree.c > @@ -700,3 +700,76 @@ void repair_worktree_at_path(const char *path, > +/* > + * 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 > + * cannot be determined. Caller is responsible for freeing returned path. > + */ > +int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath, timestamp_t expire) The function documentation which was moved here (worktree.c) from builtin/worktree.c is duplicated in the header worktree.h. Having it in the header is good. Having it here in the source file doesn't necessarily hurt, but I worry about it becoming out-of-sync with the copy in the header. For that reason, it might make sense to keep it only in the header. All of the above review comments are very minor and several of them are subjective, thus not necessarily worth a re-roll.
On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva <rafaeloliveira.cs@gmail.com> wrote: > diff --git a/worktree.h b/worktree.h > @@ -73,6 +73,16 @@ int is_main_worktree(const struct worktree *wt); > +/* > + * 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 > + * cannot be determined. Caller is responsible for freeing returned path. > + */ > +int should_prune_worktree(const char *id, > + struct strbuf *reason, > + char **wtpath, > + timestamp_t expire); A few more comments... It would be good to update the documentation to explain what `expire` is since it's not necessarily obvious. The documentation could also be tweaked to say that the worktree's path is returned in `wtpath` rather than saying only that it is returned. If you choose to make these changes, they should be probably done in a separate patch from the patch which moves the code. This is a very minor issue, not necessarily worth a re-roll. Now that this is a public function, it might make sense for `wtpath` to be optional; if the caller is not interested in it, then NULL would be passed in for this argument. The implementation of should_prune_worktree() would need to be updated to check that `wtpath` is not NULL before assigning to it. This change, too, would be done in a separate patch. However, this is just a "would be nice to have" item, not at all required, and I don't think it should be added to this series since it's not needed by anything in this series, thus would just be noise (wasting your time and reviewer time). It's just something we can keep in mind for the future.
On Wed, Jan 6, 2021 at 1:55 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva > <rafaeloliveira.cs@gmail.com> wrote: > > +/* > > + * 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 > > + * cannot be determined. Caller is responsible for freeing returned path. > > + */ > > It would be good to update the documentation to explain what `expire` > is since it's not necessarily obvious. The documentation could also be > tweaked to say that the worktree's path is returned in `wtpath` rather > than saying only that it is returned. If you choose to make these > changes, they should be probably done in a separate patch from the > patch which moves the code. This is a very minor issue, not > necessarily worth a re-roll. On second thought, adding a patch just to make a minor update to the function comment is probably overkill. It should be okay to do it in this patch. Mention the change in the commit to alert reviewers.
Thank you for the detailed review. Eric Sunshine writes: > On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva > <rafaeloliveira.cs@gmail.com> wrote: >> worktree: move should_prune_worktree() to worktree.c > > The subject is a bit confusing since this function already lives in > `worktree.c` (albeit within the `builtin` directory). An alternate way > to word this might be: > > worktree: libify and publish should_prune_worktree() > > or just: > > worktree: libify should_prune_worktree() > yes, I think these seems clearer. I will drop the current subject and use one of these on the next revision. >> This function will be used by another API function, implemented in the >> next patch that will accept a pointer to "worktree" structure directly >> making it easier to implement the "prunable" annotations. > > Okay, though I'm not sure this paragraph adds value since the reader > understands implicitly from the first paragraph that this is the > reason this patch is libifying the function. (I'd probably drop this > paragraph if I was composing the message.) > Make sense. >> should_prune_worktree() knows how to select the given worktree for pruning >> based on an expiration date, however the expiration value is stored on a >> global variable and it is not local to the function. In order to move the >> function, teach should_prune_worktree() to take the expiration date as an >> argument. > > Rather than "on a global variable", it would be more accurate to say > "static file-scope variable". > Good point. I will definitely change that to be more accurate on the next revision. >> diff --git a/worktree.c b/worktree.c >> @@ -700,3 +700,76 @@ void repair_worktree_at_path(const char *path, >> +/* >> + * 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 >> + * cannot be determined. Caller is responsible for freeing returned path. >> + */ >> +int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath, timestamp_t expire) > > The function documentation which was moved here (worktree.c) from > builtin/worktree.c is duplicated in the header worktree.h. Having it > in the header is good. Having it here in the source file doesn't > necessarily hurt, but I worry about it becoming out-of-sync with the > copy in the header. For that reason, it might make sense to keep it > only in the header. > > All of the above review comments are very minor and several of them > are subjective, thus not necessarily worth a re-roll. Make sense as well.
Eric Sunshine writes: > On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva > <rafaeloliveira.cs@gmail.com> wrote: >> diff --git a/worktree.h b/worktree.h >> @@ -73,6 +73,16 @@ int is_main_worktree(const struct worktree *wt); >> +/* >> + * 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 >> + * cannot be determined. Caller is responsible for freeing returned path. >> + */ >> +int should_prune_worktree(const char *id, >> + struct strbuf *reason, >> + char **wtpath, >> + timestamp_t expire); > > A few more comments... > > It would be good to update the documentation to explain what `expire` > is since it's not necessarily obvious. The documentation could also be > tweaked to say that the worktree's path is returned in `wtpath` rather > than saying only that it is returned. If you choose to make these > changes, they should be probably done in a separate patch from the > patch which moves the code. This is a very minor issue, not > necessarily worth a re-roll. > Good idea. I will include these changes on the next revision. specially the documentation about the `expire` field. > > Now that this is a public function, it might make sense for `wtpath` > to be optional; if the caller is not interested in it, then NULL would > be passed in for this argument. The implementation of > should_prune_worktree() would need to be updated to check that > `wtpath` is not NULL before assigning to it. This change, too, would > be done in a separate patch. However, this is just a "would be nice to > have" item, not at all required, and I don't think it should be added > to this series since it's not needed by anything in this series, thus > would just be noise (wasting your time and reviewer time). It's just > something we can keep in mind for the future. Agreed. These seems like reasonable changes and might not be good fit for this patches but definitely something that we can follow up after it.
diff --git a/builtin/worktree.c b/builtin/worktree.c index 197fd24a55..eeb3ffaed0 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -67,79 +67,6 @@ static void delete_worktrees_dir_if_empty(void) rmdir(git_path("worktrees")); /* ignore failed removal */ } -/* - * 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 - * cannot be determined. Caller is responsible for freeing returned path. - */ -static int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath) -{ - struct stat st; - char *path; - int fd; - size_t len; - ssize_t read_result; - - *wtpath = NULL; - if (!is_directory(git_path("worktrees/%s", id))) { - strbuf_addstr(reason, _("not a valid directory")); - return 1; - } - if (file_exists(git_path("worktrees/%s/locked", id))) - return 0; - if (stat(git_path("worktrees/%s/gitdir", id), &st)) { - strbuf_addstr(reason, _("gitdir file does not exist")); - return 1; - } - fd = open(git_path("worktrees/%s/gitdir", id), O_RDONLY); - if (fd < 0) { - strbuf_addf(reason, _("unable to read gitdir file (%s)"), - strerror(errno)); - return 1; - } - len = xsize_t(st.st_size); - path = xmallocz(len); - - read_result = read_in_full(fd, path, len); - if (read_result < 0) { - strbuf_addf(reason, _("unable to read gitdir file (%s)"), - strerror(errno)); - close(fd); - free(path); - return 1; - } - close(fd); - - if (read_result != len) { - strbuf_addf(reason, - _("short read (expected %"PRIuMAX" bytes, read %"PRIuMAX")"), - (uintmax_t)len, (uintmax_t)read_result); - free(path); - return 1; - } - while (len && (path[len - 1] == '\n' || path[len - 1] == '\r')) - len--; - if (!len) { - strbuf_addstr(reason, _("invalid gitdir file")); - free(path); - return 1; - } - path[len] = '\0'; - if (!file_exists(path)) { - if (stat(git_path("worktrees/%s/index", id), &st) || - st.st_mtime <= expire) { - strbuf_addstr(reason, _("gitdir file points to non-existent location")); - free(path); - return 1; - } else { - *wtpath = path; - return 0; - } - } - *wtpath = path; - return 0; -} - static void prune_worktree(const char *id, const char *reason) { if (show_only || verbose) @@ -195,7 +122,7 @@ static void prune_worktrees(void) if (is_dot_or_dotdot(d->d_name)) continue; strbuf_reset(&reason); - if (should_prune_worktree(d->d_name, &reason, &path)) + if (should_prune_worktree(d->d_name, &reason, &path, expire)) prune_worktree(d->d_name, reason.buf); else if (path) string_list_append(&kept, path)->util = xstrdup(d->d_name); diff --git a/worktree.c b/worktree.c index f84ceae87d..5764b0dc7c 100644 --- a/worktree.c +++ b/worktree.c @@ -700,3 +700,76 @@ void repair_worktree_at_path(const char *path, strbuf_release(&realdotgit); strbuf_release(&dotgit); } + +/* + * 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 + * cannot be determined. Caller is responsible for freeing returned path. + */ +int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath, timestamp_t expire) +{ + struct stat st; + char *path; + int fd; + size_t len; + ssize_t read_result; + + *wtpath = NULL; + if (!is_directory(git_path("worktrees/%s", id))) { + strbuf_addstr(reason, _("not a valid directory")); + return 1; + } + if (file_exists(git_path("worktrees/%s/locked", id))) + return 0; + if (stat(git_path("worktrees/%s/gitdir", id), &st)) { + strbuf_addstr(reason, _("gitdir file does not exist")); + return 1; + } + fd = open(git_path("worktrees/%s/gitdir", id), O_RDONLY); + if (fd < 0) { + strbuf_addf(reason, _("unable to read gitdir file (%s)"), + strerror(errno)); + return 1; + } + len = xsize_t(st.st_size); + path = xmallocz(len); + + read_result = read_in_full(fd, path, len); + if (read_result < 0) { + strbuf_addf(reason, _("unable to read gitdir file (%s)"), + strerror(errno)); + close(fd); + free(path); + return 1; + } + close(fd); + + if (read_result != len) { + strbuf_addf(reason, + _("short read (expected %"PRIuMAX" bytes, read %"PRIuMAX")"), + (uintmax_t)len, (uintmax_t)read_result); + free(path); + return 1; + } + while (len && (path[len - 1] == '\n' || path[len - 1] == '\r')) + len--; + if (!len) { + strbuf_addstr(reason, _("invalid gitdir file")); + free(path); + return 1; + } + path[len] = '\0'; + if (!file_exists(path)) { + if (stat(git_path("worktrees/%s/index", id), &st) || + st.st_mtime <= expire) { + strbuf_addstr(reason, _("gitdir file points to non-existent location")); + free(path); + return 1; + } else { + *wtpath = path; + return 0; + } + } + *wtpath = path; + return 0; +} diff --git a/worktree.h b/worktree.h index f38e6fd5a2..e5f4320725 100644 --- a/worktree.h +++ b/worktree.h @@ -73,6 +73,16 @@ int is_main_worktree(const struct worktree *wt); */ const char *worktree_lock_reason(struct worktree *wt); +/* + * 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 + * cannot be determined. Caller is responsible for freeing returned path. + */ +int should_prune_worktree(const char *id, + struct strbuf *reason, + char **wtpath, + timestamp_t expire); + #define WT_VALIDATE_WORKTREE_MISSING_OK (1 << 0) /*
As part of teaching "git worktree list" to annotate worktree that is a candidate for pruning, let's move should_prune_worktree() from builtin/worktree.c to worktree.c in order to make part of the worktree public API. This function will be used by another API function, implemented in the next patch that will accept a pointer to "worktree" structure directly making it easier to implement the "prunable" annotations. should_prune_worktree() knows how to select the given worktree for pruning based on an expiration date, however the expiration value is stored on a global variable and it is not local to the function. In order to move the function, teach should_prune_worktree() to take the expiration date as an argument. Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com> --- builtin/worktree.c | 75 +--------------------------------------------- worktree.c | 73 ++++++++++++++++++++++++++++++++++++++++++++ worktree.h | 10 +++++++ 3 files changed, 84 insertions(+), 74 deletions(-)