From: Christian Couder <christian.couder@gmail.com>
To: John Cai via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, John Cai <johncai86@gmail.com>,
Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v2] attr: teach "--attr-source=<tree>" global option to "git"
Date: Thu, 27 Apr 2023 16:02:16 +0200 [thread overview]
Message-ID: <CAP8UFD1YG-NnHbG4kBmh7L=O3wVnYKHCd94xD6Up_+AwK1ABPQ@mail.gmail.com> (raw)
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:
> 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!
next prev parent reply other threads:[~2023-04-27 14:02 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-18 3:25 [PATCH] attr: teach "--attr-source=<tree>" global option to "git" John Cai via GitGitGadget
2023-03-27 17:02 ` [PATCH v2] " John Cai via GitGitGadget
2023-03-27 18:29 ` Junio C Hamano
2023-03-27 19:40 ` John Cai
2023-04-27 14:02 ` Christian Couder [this message]
2023-04-27 14:07 ` Christian Couder
2023-04-28 18:50 ` [PATCH v3] " John Cai via GitGitGadget
2023-04-28 20:22 ` Christian Couder
2023-04-30 2:22 ` John Cai
2023-04-30 2:39 ` [PATCH v4] " John Cai via GitGitGadget
2023-05-01 16:21 ` Junio C Hamano
2023-05-03 15:10 ` Christian Couder
2023-05-03 18:40 ` John Cai
2023-05-03 20:09 ` [PATCH v5] " John Cai via GitGitGadget
2023-05-04 9:50 ` Christian Couder
2023-05-06 4:15 ` [PATCH v6] " John Cai via GitGitGadget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAP8UFD1YG-NnHbG4kBmh7L=O3wVnYKHCd94xD6Up_+AwK1ABPQ@mail.gmail.com' \
--to=christian.couder@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=johncai86@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).