Re: [PATCH v2] attr: teach "--attr-source=<tree>" global option to "git"
- From
Christian Couder <christian.couder@gmail.com>
- Date
- Apr 27, 2023, 14:02 UTC
- Message-ID
- <CAP8UFD1YG-NnHbG4kBmh7L=O3wVnYKHCd94xD6Up_+AwK1ABPQ@mail.gmail.com>
- In-Reply-To
- <pull.1470.v2.git.git.1679936543320.gitgitgadget@gmail.com>
On Mon, Mar 27, 2023 at 7:44 PM John Cai via GitGitGadget <gitgitgadget@gmail.com> wrote:
Show 5 quoted lines
> 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.
Show 7 quoted lines
> 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.
Show 15 quoted lines
> [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:
Show 9 quoted lines
> - 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
Show 8 quoted lines
> --- 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.
Show 9 quoted lines
> 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...
Show 5 quoted lines
> + 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.
Show 10 quoted lines
> +}
> +
> +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; > +} > +
Show 17 quoted lines
> --- 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!