Web lists-archives.com

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




Am Sa., 26. Jan. 2019 um 12:01 Uhr schrieb Duy Nguyen <pclouds@xxxxxxxxx>:
>
> On Sat, Jan 26, 2019 at 5:44 PM Sebastian Staudt <koraktor@xxxxxxxxx> wrote:
> >
> > This ensures the given working tree is used for --dirty and --broken.
> >
> > Signed-off-by: Sebastian Staudt <koraktor@xxxxxxxxx>
> > ---
> >  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.

> > +
> >      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@xxxxxxxxx>:
>
> On Sat, Jan 26, 2019 at 5:44 PM Sebastian Staudt <koraktor@xxxxxxxxx> wrote:
> >
> > This ensures the given working tree is used for --dirty and --broken.
> >
> > Signed-off-by: Sebastian Staudt <koraktor@xxxxxxxxx>
> > ---
> >  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