Re: [PATCH v2] attr: teach "--attr-source=<tree>" global option to "git"

[Christian Couder <christian.couder@gmail.com>]

On Mon, Mar 27, 2023 at 7:44 PM John Cai via GitGitGadget
<gitgitgadget@gmail.com> wrote:

> Undo most of the UI change the commit made, while keeping the
> internal logic to read attributes from a given tree-ish.  Expose the
> internal logic via a new "--attr-source=<tree>" command line option
> given to "git", so that it can be used with any git command that
> runs internally.

I am not sure what are the git commands that run internally. You mean
any builtin command? Actually from the sentence below it looks like it
means any command running as part of the main git process.

> Additionally, add an environment variable GIT_ATTR_SOURCE that is set
> when --attr-source is passed in, so that subprocesses use the same value
> for the attributes source tree.
>
> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
>     attr: teach "--attr-source=" global option to "git"

Not sure you can do something about it but it looks like "<tree>" has
been removed after "--attr-source=" in the above sentence. It might be
that the commit subject wasn't properly quoted in the tool chain and
mistaken for an HTML or XML tag.

>     [1] aimed to allow gitattributes to be read from bare repositories when
>     running git-diff(1). Through discussion, a more general solution emerged
>     (represented by this patch), which allows the attribute machinery to
>     read attributes from a source passed in through a git flag.
>
>     This version is the same as v1. Just changed the author to Junio as he
>     contributed most of the code.
>
>      1. https://lore.kernel.org/git/pull.1459.git.git.1678758818.gitgitgadget@gmail.com/
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1470%2Fjohn-cai%2Fjc%2Fattr-source-git-flag-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1470/john-cai/jc/attr-source-git-flag-v2
> Pull-Request: https://github.com/git/git/pull/1470
>
> Range-diff vs v1:

>      -    Add an environment variable GIT_ATTR_SOURCE that is set when
>      -    --attr-source is passed in, so that subprocesses use the same value for
>      -    the attributes source tree.
>      +    Additionally, add an environment variable GIT_ATTR_SOURCE that is set
>      +    when --attr-source is passed in, so that subprocesses use the same value
>      +    for the attributes source tree.
>
>      -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
>           Signed-off-by: John Cai <johncai86@gmail.com>

If the patch is from Junio, I think you should keep his Signed-off-by. If you

> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -212,6 +212,9 @@ If you just want to run git as if it was started in `<path>` then use
>         nohelpers (exclude helper commands), alias and config
>         (retrieve command list from config variable completion.commands)
>
> +--attr-source=<tree-ish>::
> +       Read gitattributes from <tree-ish> instead of the worktree.

Nit: maybe something like "See gitattributes(5)." could help beginners here.

> diff --git a/attr.c b/attr.c
> index 657ee52229e..2539309b92f 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -1166,11 +1166,43 @@ static void collect_some_attrs(struct index_state *istate,
>         fill(path, pathlen, basename_offset, check->stack, check->all_attrs, rem);
>  }
>
> +static const char *default_attr_source_tree_object_name;

I think we are trying to avoid global variables, but I guess there is
no infrastructure yet to put these kinds of variables into a global
struct.

> +static void compute_default_attr_source(struct object_id *attr_source)
> +{
> +       if (!default_attr_source_tree_object_name)
> +               default_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE);

I wonder what happens if the env variable is set to an empty string,
instead of unset...

> +       if (!default_attr_source_tree_object_name || !is_null_oid(attr_source))
> +               return;
> +
> +       if (get_oid_treeish(default_attr_source_tree_object_name, attr_source))
> +               die(_("bad --attr-source object"));

... so it seems that in the case of an empty string, we will just die
with "bad --attr-source object". That doesn't seem very user friendly
to me as users might not know or remember about the GIT_ATTR_SOURCE
variable when they get the error.

> +}
> +
> +static struct object_id *default_attr_source(void)
> +{
> +       static struct object_id attr_source;
> +
> +       if (is_null_oid(&attr_source))
> +               compute_default_attr_source(&attr_source);
> +       if (is_null_oid(&attr_source))
> +               return NULL;

It looks like is_null_oid(&attr_source) can happen when
default_attr_source_tree_object_name is NULL, Ok.

> +       return &attr_source;
> +}
> +


> --- a/git.c
> +++ b/git.c
> @@ -5,6 +5,7 @@
>  #include "run-command.h"
>  #include "alias.h"
>  #include "replace-object.h"
> +#include "attr.h"
>  #include "shallow.h"
>
>  #define RUN_SETUP              (1<<0)
> @@ -308,6 +309,9 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>                         } else {
>                                 exit(list_cmds(cmd));
>                         }
> +               } else if (skip_prefix(cmd, "--attr-source=", &cmd)) {
> +                       set_git_attr_source(cmd);
> +                       setenv(GIT_ATTR_SOURCE, (*argv)[1], 1);

This seems a strange to me as it looks like other similar options
require code like the following:

               } else if (!strcmp(cmd, "--super-prefix")) {
                       if (*argc < 2) {
                               fprintf(stderr, _("no prefix given for
--super-prefix\n" ));
                               usage(git_usage_string);
                       }
                       setenv(GIT_SUPER_PREFIX_ENVIRONMENT, (*argv)[1], 1);
                       if (envchanged)
                               *envchanged = 1;
                       (*argv)++;
                       (*argc)--;
               } else if (skip_prefix(cmd, "--super-prefix=", &cmd)) {
                       setenv(GIT_SUPER_PREFIX_ENVIRONMENT, cmd, 1);
                       if (envchanged)
                               *envchanged = 1;

>                 } else {
>                         fprintf(stderr, _("unknown option: %s\n"), cmd);
>                         usage(git_usage_string);

I haven't taken a look at the tests but the rest of the code looks good to me.

Thanks!