Jeff King <peff@xxxxxxxx> writes: > On Tue, Jan 29, 2019 at 06:18:58AM +0100, Sebastian Staudt wrote: > >> Subject: Re: [PATCH v3 2/3] Setup working tree in describe > > We usually write subjects as "area: do some thing" which is a little > easier when scanning big lists of "git log --oneline". > > I think it's key, too, that we only do this for the --dirty case, not > always. So maybe: > > describe: setup working tree for --dirty > > or something? Thanks as always for being an excellent reviewer who not just reviews but also gives good suggestions. >> This ensures the given working tree is used for --dirty. > > There's been a lot of digging and discussion on the list about what > happens if we don't do this. Could we summarize it here? > > Perhaps: > > We don't use NEED_WORK_TREE when running the git-describe builtin, > since you should be able to describe a commit even in a bare > repository. However, the --dirty flag does need a working tree. Since > we don't call setup_work_tree(), it uses whatever directory we happen > to be in. That's unlikely to match our index, meaning we'd say "dirty" > even when the real working tree is clean. > > We can fix that by calling setup_work_tree() once we know that the > user has asked for --dirty. > >> The implementation of --broken uses diff-index which calls >> setup_work_tree() itself. > > If I hadn't just read the rest of the thread, I'd probably wonder why we > are talking about --broken at all. Maybe: > > The --broken option similarly needs a working tree. But because the > current implementation calls an external diff-index to do the work, > we don't have to bother setting up the working tree in the > git-describe process. > >> diff --git a/builtin/describe.c b/builtin/describe.c >> index cc118448ee..b5b7abdc8f 100644 >> --- a/builtin/describe.c >> +++ b/builtin/describe.c >> @@ -629,6 +629,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix) >> struct argv_array args = ARGV_ARRAY_INIT; >> int fd, result; >> >> + setup_work_tree(); >> read_cache(); >> refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, >> NULL, NULL, NULL); > > The patch itself looks good. :) > > -Peff
