attr: teach "--attr-source=<tree>" global option to "git" #1470
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
john-cai
force-pushed
the
jc/attr-source-git-flag
branch
2 times, most recently
from
bbe342a
to
ec04719
Compare
|
/preview |
|
Preview email sent as pull.1470.git.git.1679108493001.gitgitgadget@gmail.com |
|
/submit |
|
Submitted as pull.1470.git.git.1679109928556.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
/preview |
|
Preview email sent as pull.1470.v2.git.git.1679928613672.gitgitgadget@gmail.com |
|
/submit |
|
Error: ec04719 was already submitted |
john-cai
force-pushed
the
jc/attr-source-git-flag
branch
from
ec04719
to
0667b47
Compare
|
/submit |
|
Submitted as pull.1470.v2.git.git.1679936543320.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> This version is the same as v1. Just changed the author to Junio as he
> contributed most of the code.
Heh, I do not need an extra credit. The most important missing
piece I punted (i.e. the environment passing and receiving) was done
by you that is a larger contribution anyway ;-)
I am kind-a surprised that we haven't queued the earlier one. Is
this ready to ship?
Thanks. |
|
On the Git mailing list, John Cai wrote (reply to this): Hi Junio,
On 27 Mar 2023, at 14:29, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> This version is the same as v1. Just changed the author to Junio as he
>> contributed most of the code.
>
> Heh, I do not need an extra credit. The most important missing
> piece I punted (i.e. the environment passing and receiving) was done
> by you that is a larger contribution anyway ;-)
Given that the main idea was yours and most of the code minus the test was as well, I
thought it made the most sense.
>
> I am kind-a surprised that we haven't queued the earlier one. Is
> this ready to ship?
I believe so, though a review wouldn't hurt--especially on the environment
variable passing and receiving piece.
>
> Thanks.
thanks!
John |
|
On the Git mailing list, Christian Couder wrote (reply to this): 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! |
|
User |
|
On the Git mailing list, Christian Couder wrote (reply to this): On Thu, Apr 27, 2023 at 4:02 PM Christian Couder
<christian.couder@gmail.com> wrote:
> > - 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
I wanted to say that if you take ownership of the patch as Junio
suggested, I think it should be Ok either way. |
john-cai
force-pushed
the
jc/attr-source-git-flag
branch
2 times, most recently
from
08d32ff
to
ff8d3a0
Compare
|
/preview |
|
Preview email sent as pull.1470.v3.git.git.1682706963577.gitgitgadget@gmail.com |
|
/submit |
|
Submitted as pull.1470.v3.git.git.1682707848916.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Christian Couder wrote (reply to this): > +--attr-source=<tree-ish>::
> -+ Read gitattributes from <tree-ish> instead of the worktree.
> ++ Read gitattributes from <tree-ish> instead of the worktree. See
> ++ linkgit:gitrevisions[7].
I think it's more sensible to link to gitattributes(5) instead of
gitrevisions(7)
> +static const char *default_attr_source_tree_object_name;
> +
> +void set_git_attr_source(const char *tree_object_name)
> +{
> + default_attr_source_tree_object_name = xstrdup(tree_object_name);
> +}
> +
> +
One empty line is enough here.
> +static void compute_default_attr_source(struct object_id *attr_source)
> +{
> + int from_env = 0;
> +
> + if (!default_attr_source_tree_object_name) {
> + default_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE);
> + from_env = 1;
> + }
> +
> + if (!default_attr_source_tree_object_name || !is_null_oid(attr_source))
> + return;
> +
> + if (repo_get_oid_treeish(the_repository, default_attr_source_tree_object_name, attr_source)) {
> + if (from_env)
> + die(_("bad --attr-source object"));
> + else
> + die(_("bad GIT_ATTR_SOURCE"));
I think it would be better to have just the following instead of the 4
lines above:
die(_("invalid tree object from --attr-source flag or GIT_ATTR_SOURCE
env variable"));
as a bad GIT_ATTR_SOURCE in a subprocess could come from a bad
--attr-source in the main process.
And this way the from_env variable is not needed.
> + }
> +}
The rest looks good to me, thanks! |
john-cai
force-pushed
the
jc/attr-source-git-flag
branch
from
ff8d3a0
to
e41f931
Compare
|
/preview |
|
Preview email sent as pull.1470.v4.git.git.1682821537302.gitgitgadget@gmail.com |
|
On the Git mailing list, John Cai wrote (reply to this): Hi Christian
On 28 Apr 2023, at 16:22, Christian Couder wrote:
>> +--attr-source=<tree-ish>::
>> -+ Read gitattributes from <tree-ish> instead of the worktree.
>> ++ Read gitattributes from <tree-ish> instead of the worktree. See
>> ++ linkgit:gitrevisions[7].
>
> I think it's more sensible to link to gitattributes(5) instead of
> gitrevisions(7)
oops, I forgot to push up the version with this fix. Thanks for catching it.
>
>> +static const char *default_attr_source_tree_object_name;
>> +
>> +void set_git_attr_source(const char *tree_object_name)
>> +{
>> + default_attr_source_tree_object_name = xstrdup(tree_object_name);
>> +}
>> +
>> +
>
> One empty line is enough here.
noted.
>
>> +static void compute_default_attr_source(struct object_id *attr_source)
>> +{
>> + int from_env = 0;
>> +
>> + if (!default_attr_source_tree_object_name) {
>> + default_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE);
>> + from_env = 1;
>> + }
>> +
>> + if (!default_attr_source_tree_object_name || !is_null_oid(attr_source))
>> + return;
>> +
>> + if (repo_get_oid_treeish(the_repository, default_attr_source_tree_object_name, attr_source)) {
>> + if (from_env)
>> + die(_("bad --attr-source object"));
>> + else
>> + die(_("bad GIT_ATTR_SOURCE"));
>
> I think it would be better to have just the following instead of the 4
> lines above:
>
> die(_("invalid tree object from --attr-source flag or GIT_ATTR_SOURCE
> env variable"));
>
> as a bad GIT_ATTR_SOURCE in a subprocess could come from a bad
> --attr-source in the main process.
>
> And this way the from_env variable is not needed.
sounds good
>
>> + }
>> +}
>
> The rest looks good to me, thanks!
thanks,
John |
|
/submit |
|
This patch series was integrated into seen via b4eaae1. |
|
On the Git mailing list, Christian Couder wrote (reply to this): On Wed, May 3, 2023 at 10:09 PM John Cai via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> @@ -314,6 +315,21 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
> } else {
> exit(list_cmds(cmd));
> }
> + } else if (!strcmp(cmd, "--attr-source")) {
> + if (*argc < 2) {
> + fprintf(stderr, _("no attribute source given for --attr-source\n" ));
> + usage(git_usage_string);
> + }
> + setenv(GIT_ATTR_SOURCE_ENVIRONMENT, (*argv)[1], 1);
> + if (envchanged)
> + *envchanged = 1;
> + (*argv)++;
> + (*argc)--;
> + } else if (skip_prefix(cmd, "--attr-source=", &cmd)) {
> + set_git_attr_source(cmd);
> + setenv(GIT_ATTR_SOURCE_ENVIRONMENT, (*argv)[1], 1);
It seems to me that for other options "cmd" is used here instead of
"(*argv)[1]" as the second argument to setenv(), for example:
setenv(GIT_SUPER_PREFIX_ENVIRONMENT, cmd, 1);
> + if (envchanged)
> + *envchanged = 1;
> } else {
> fprintf(stderr, _("unknown option: %s\n"), cmd);
> usage(git_usage_string); |
Earlier, 47cfc9b (attr: add flag `--source` to work with tree-ish, 2023-01-14) taught "git check-attr" the "--source=<tree>" option to allow it to read attribute files from a tree-ish, but did so only for the command. Just like "check-attr" users wanted a way to use attributes from a tree-ish and not from the working tree files, users of other commands (like "git diff") would benefit from the same. 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 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> Signed-off-by: Junio C Hamano <gitster@pobox.com>
john-cai
force-pushed
the
jc/attr-source-git-flag
branch
from
b3b696b
to
8389cc1
Compare
|
This patch series was integrated into seen via 5cdb22e. |
|
There was a status update in the "Cooking" section about the branch "git --attr-source=<tree> cmd $args" is a new way to have any command to read attributes not from the working tree but from the given tree object. Expecting a hopefully final minor reroll. cf. <CAP8UFD1AuFWWC=iAe0duhpSsw9HnA-tcpV2F3NGT+089aY60Ow@mail.gmail.com> source: <pull.1470.v5.git.git.1683144574158.gitgitgadget@gmail.com> |
|
/submit |
|
Submitted as pull.1470.v6.git.git.1683346530487.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via d6ed063. |
|
This patch series was integrated into seen via 55f190e. |
|
This patch series was integrated into seen via 74e5879. |
|
This patch series was integrated into seen via e7263cc. |
|
This patch series was integrated into seen via 83f55ea. |
|
This patch series was integrated into seen via 9e509f8. |
|
There was a status update in the "Cooking" section about the branch "git --attr-source=<tree> cmd $args" is a new way to have any command to read attributes not from the working tree but from the given tree object. Will merge to 'next'. source: <pull.1470.v6.git.git.1683346530487.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via aa20c8e. |
|
This patch series was integrated into next via cb94f4f. |
|
This patch series was integrated into seen via 781f990. |
|
There was a status update in the "Cooking" section about the branch "git --attr-source=<tree> cmd $args" is a new way to have any command to read attributes not from the working tree but from the given tree object. Will merge to 'master'. source: <pull.1470.v6.git.git.1683346530487.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via 58c879c. |
|
This patch series was integrated into seen via a46bcd5. |
|
This patch series was integrated into seen via e9043f4. |
|
There was a status update in the "Cooking" section about the branch "git --attr-source=3D<tree> cmd $args" is a new way to have any command to read attributes not from the working tree but from the given tree object. Will merge to 'master'. source: <pull.1470.v6.git.git.1683346530487.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via 67a3b2b. |
|
This patch series was integrated into master via 67a3b2b. |
|
This patch series was integrated into next via 67a3b2b. |
|
Closed via 67a3b2b. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[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.
Changes since v5:
Changes since v4:
Changes since v3:
Changes since v2:
cc: Christian Couder christian.couder@gmail.com