Re: [PATCH 2/2] Setup working tree in describe

From: Sebastian Staudt <koraktor@gmail.com>
Date: Jan 26, 2019, 14:07 UTC
Message-ID: <CA+xP2SaJYD+OKC18y0zz2V6A9J8rAB0-cxMnrDmWt==8v2_uBA@mail.gmail.com>
In-Reply-To: <CACsJy8Df6XM55ExRfCjpUQsv2Vm0cOwrSAyNf6net__uztMOGQ@mail.gmail.com>

Am Sa., 26. Jan. 2019 um 12:01 Uhr schrieb Duy Nguyen <pclouds@gmail.com>:

Show 32 quoted lines

>
> On Sat, Jan 26, 2019 at 5:44 PM Sebastian Staudt <koraktor@gmail.com> wrote:
> >
> > This ensures the given working tree is used for --dirty and --broken.
> >
> > Signed-off-by: Sebastian Staudt <koraktor@gmail.com>
> > ---
> >  builtin/describe.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/builtin/describe.c b/builtin/describe.c
> > index cc118448ee..ba1a0b199b 100644
> > --- a/builtin/describe.c
> > +++ b/builtin/describe.c
> > @@ -601,6 +601,8 @@ int cmd_describe(int argc, const char **argv,
> > const char *prefix)
> >      if (!hashmap_get_size(&names) && !always)
> >          die(_("No names found, cannot describe anything."));
> >
> > +    setup_work_tree();
>
> This forces worktree's presence in all cases and will die() if
> worktree is not available. You need to check if broken or dirty is set
> and only call this function in that case.
>
> Though in my opinion it's better to call before we need it in the "if
> (broke)" and "else if (dirty)" code blocks. That way you don't even
> need to check if it's "dirty" or "broken". Does "broken" really need
> this though? If it runs "git diff-index" separately, that command
> should handle this setup_work_tree() already, or we may need to fix it
> there, not here.
>

Thanks for your feedback. Are you sure that it will fail without a working tree? Is it even possible to have *no* working tree?

I already tested this with some real life examples, e.g.

    git --git-dir /some/path/.git describe

From inside and outside of other repositories. I didn‘t hit any errors so far.

Show 11 quoted lines

> > +
> >      if (argc == 0) {
> >          if (broken) {
> >              struct child_process cp = CHILD_PROCESS_INIT;
> > --
> > 2.20.1
>
>
>
> --
> Duy

Best regards,
    Sebastian

Am Sa., 26. Jan. 2019 um 12:01 Uhr schrieb Duy Nguyen <pclouds@gmail.com>:

Show 43 quoted lines

>
> On Sat, Jan 26, 2019 at 5:44 PM Sebastian Staudt <koraktor@gmail.com> wrote:
> >
> > This ensures the given working tree is used for --dirty and --broken.
> >
> > Signed-off-by: Sebastian Staudt <koraktor@gmail.com>
> > ---
> >  builtin/describe.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/builtin/describe.c b/builtin/describe.c
> > index cc118448ee..ba1a0b199b 100644
> > --- a/builtin/describe.c
> > +++ b/builtin/describe.c
> > @@ -601,6 +601,8 @@ int cmd_describe(int argc, const char **argv,
> > const char *prefix)
> >      if (!hashmap_get_size(&names) && !always)
> >          die(_("No names found, cannot describe anything."));
> >
> > +    setup_work_tree();
>
> This forces worktree's presence in all cases and will die() if
> worktree is not available. You need to check if broken or dirty is set
> and only call this function in that case.
>
> Though in my opinion it's better to call before we need it in the "if
> (broke)" and "else if (dirty)" code blocks. That way you don't even
> need to check if it's "dirty" or "broken". Does "broken" really need
> this though? If it runs "git diff-index" separately, that command
> should handle this setup_work_tree() already, or we may need to fix it
> there, not here.
>
> > +
> >      if (argc == 0) {
> >          if (broken) {
> >              struct child_process cp = CHILD_PROCESS_INIT;
> > --
> > 2.20.1
>
>
>
> --
> Duy