* Re: [PATCH v2 1/5] gpg-interface, tag: add GPG_VERIFY_QUIET flag
From: Lukas Pühringer @ 2016-09-27 18:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: santiago, git, peff, sunshine, walters
In-Reply-To: <xmqqponpnqyf.fsf@gitster.mtv.corp.google.com>
Good, I will change it to 'Lukas Puehringer' then, when we send you the updated batch of patches, that address your latest comments.
Thanks,
Lukas
> On Sep 27, 2016, at 2:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Lukas Pühringer <luk.puehringer@gmail.com> writes:
>
>> Thanks for checking. I am fine with Lukas P, unless git prefers
>> full last names. In that case I am fine with changing too.
>
> We do prefer full names, so that it would be consistent with court
> document when you are involved in copyright inflingement case ;-)
^ permalink raw reply
* Re: [PATCH v2 1/5] gpg-interface, tag: add GPG_VERIFY_QUIET flag
From: Junio C Hamano @ 2016-09-27 18:22 UTC (permalink / raw)
To: Lukas Pühringer; +Cc: santiago, git, peff, sunshine, walters
In-Reply-To: <15BEFDC3-81B2-47FC-A213-F37B4E6020D2@gmail.com>
Lukas Pühringer <luk.puehringer@gmail.com> writes:
> Thanks for checking. I am fine with Lukas P, unless git prefers
> full last names. In that case I am fine with changing too.
We do prefer full names, so that it would be consistent with court
document when you are involved in copyright inflingement case ;-)
^ permalink raw reply
* Re: [PATCH 1/4 v4] submodules: make submodule-prefix option
From: Junio C Hamano @ 2016-09-27 18:17 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <1474930003-83750-2-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> +--submodule-prefix=<path>::
> + Set a prefix which gives submodules context about the superproject that
> + invoked it. Only allowed for commands which support submodules.
This, and also the message in die(), uses a phrase "support
submodules", but it is unclear what it exactly means to the end
users and readers.
A "ls-files" that is recursively run as an implementation detail of
the "grep --recurse-submodules" would be taught to support this
option with this series. Who is supporting submodules in that
context?
I'd imagine (close to) 100% of the people would say it is "grep"
that is supporting submodules, not "ls-files", but what this
paragraph and die() message want to express by the phrase "support
submodules" is the fact that "ls-files" knows how to react to
"--submodule-prefix" option.
I'd suggest not to worry too much about this phrasing at this point,
until we figure out exactly how we want to present these to end
users. For now, perhaps drop the second sentence and replace it
with "The end-users are not expected to use this option" or
something like that?
> diff --git a/git.c b/git.c
> index 1c61151..b2b096a 100644
> --- a/git.c
> +++ b/git.c
> @@ -164,6 +164,20 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
> setenv(GIT_WORK_TREE_ENVIRONMENT, cmd, 1);
> if (envchanged)
> *envchanged = 1;
> + } else if (!strcmp(cmd, "--submodule-prefix")) {
> + if (*argc < 2) {
> + fprintf(stderr, "No prefix given for --submodule-prefix.\n" );
> + usage(git_usage_string);
> + }
> + setenv(GIT_SUBMODULE_PREFIX_ENVIRONMENT, (*argv)[1], 1);
> + if (envchanged)
> + *envchanged = 1;
> + (*argv)++;
> + (*argc)--;
> + } else if (skip_prefix(cmd, "--submodule-prefix=", &cmd)) {
> + setenv(GIT_SUBMODULE_PREFIX_ENVIRONMENT, cmd, 1);
> + if (envchanged)
> + *envchanged = 1;
> } else if (!strcmp(cmd, "--bare")) {
> char *cwd = xgetcwd();
> is_bare_repository_cfg = 1;
> @@ -310,6 +324,7 @@ static int handle_alias(int *argcp, const char ***argv)
> * RUN_SETUP for reading from the configuration file.
> */
> #define NEED_WORK_TREE (1<<3)
> +#define SUPPORT_SUBMODULES (1<<4)
>
> struct cmd_struct {
> const char *cmd;
> @@ -344,6 +359,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
> }
> commit_pager_choice();
>
> + if (!help && (getenv(GIT_SUBMODULE_PREFIX_ENVIRONMENT) &&
> + !(p->option & SUPPORT_SUBMODULES)))
> + die("%s doesn't support submodules", p->cmd);
s/submodules/submodule-prefix/ at least.
> if (!help && p->option & NEED_WORK_TREE)
> setup_work_tree();
^ permalink raw reply
* Re: [PATCH v2 1/5] gpg-interface, tag: add GPG_VERIFY_QUIET flag
From: Lukas Pühringer @ 2016-09-27 18:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: santiago, git, peff, sunshine, walters
In-Reply-To: <xmqqbmz9p7m0.fsf@gitster.mtv.corp.google.com>
Thanks for checking. I am fine with Lukas P, unless git prefers full last names. In that case I am fine with changing too.
Best,
Lukas P
> On Sep 27, 2016, at 1:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> santiago@nyu.edu writes:
>
>> From: Lukas P <luk.puehringer@gmail.com>
>>
>> Functions that print git object information may require that the
>> gpg-interface functions be silent. Add GPG_VERIFY_QUIET flag and prevent
>> print_signature_buffer from being called if flag is set.
>>
>> Signed-off-by: Lukas P <luk.puehringer@gmail.com>
>
> Are you and Lukas sure that "Lukas P" is how luk.puehringer wants to
> be known by the world? Just checking.
>
^ permalink raw reply
* Re: [PATCH v5] revision: new rev^-n shorthand for rev^n..rev
From: Junio C Hamano @ 2016-09-27 18:08 UTC (permalink / raw)
To: Vegard Nossum
Cc: git, Santi Béjar, Kevin Bracey, Philip Oakley, Matthieu Moy,
Ramsay Jones, Jakub Narębski, Jeff King
In-Reply-To: <20160927083249.31869-1-vegard.nossum@oracle.com>
Vegard Nossum <vegard.nossum@oracle.com> writes:
> "git log rev^..rev" is commonly used to show all work done on and merged
> from a side branch. This patch introduces a shorthand "rev^-" for this
> and additionally allows "rev^-$n" to mean "reachable from rev, excluding
> what is reachable from the nth parent of rev". For example, for a
> two-parent merge, you can use rev^-2 to get the set of commits which were
> made to the main branch while the topic branch was prepared.
>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
Very nicely done. Thanks for a pleasant read.
Will queue.
^ permalink raw reply
* Re: [PATCH v2 5/5] builtin/tag: add --format argument for tag -v
From: Junio C Hamano @ 2016-09-27 17:50 UTC (permalink / raw)
To: santiago; +Cc: git, peff, sunshine, walters, Lukas P
In-Reply-To: <20160926224233.32702-6-santiago@nyu.edu>
santiago@nyu.edu writes:
> From: Lukas P <luk.puehringer@gmail.com>
>
> Adding --format to git tag -v mutes the default output of the GPG
> verification and instead prints the formatted tag object.
> This allows callers to cross-check the tagname from refs/tags with
> the tagname from the tag object header upon GPG verification.
>
> Caveat: The change adds a format specifier argument to the
> (*each_tag_name_fn) function pointer, i.e. delete_tag now receives this
> too, although it does not need it.
That's an interesting "caveat".
Generally it is a good idea to give an additional opaque pointer to
callback functions of iteration API so that code that uses the
iteration can pass custom data to its callback.
Looking at the way you enhanced each_tag_name_fn, however, you added
a specific argument instead; that is the only reason why you need a
"caveat". If it were "void *", it would have been in line with the
usual practice, not worth mentioning as a "caveat", but could even
be advertised as a feature, replacing the last "Caveat" paragraph
with something like this:
The callback function for for_each_tag_name() didn't allow
callers to pass custom data to their callback functions.
Add a new opaque pointer to each_tag_name_fn's parameter to
allow this.
> Signed-off-by: Lukas P <luk.puehringer@gmail.com>
> ---
> builtin/tag.c | 30 ++++++++++++++++++++----------
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 14f3b48..f53227e 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -24,7 +24,7 @@ static const char * const git_tag_usage[] = {
> N_("git tag -d <tagname>..."),
> N_("git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>]"
> "\n\t\t[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]"),
> - N_("git tag -v <tagname>..."),
> + N_("git tag -v [--format=<format>] <tagname>..."),
> NULL
> };
>
> @@ -66,9 +66,10 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con
> }
>
> typedef int (*each_tag_name_fn)(const char *name, const char *ref,
> - const unsigned char *sha1);
> + const unsigned char *sha1, const char *fmt_pretty);
You'd replace "const char *fmt_pretty" with "void *cb_data" here, and...
>
> -static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
> +static int for_each_tag_name(const char **argv, each_tag_name_fn fn,
> + const char *fmt_pretty)
... also here. Then introduce fmt_pretty as an auto variable in the
function ...
> {
> const char **p;
... by adding this line here:
const char *fmt_pretty = cb_data;
> char ref[PATH_MAX];
> @@ -87,14 +88,14 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
> had_error = 1;
> continue;
> }
> - if (fn(*p, ref, sha1))
> + if (fn(*p, ref, sha1, fmt_pretty))
> had_error = 1;
> }
> return had_error;
> }
>
> static int delete_tag(const char *name, const char *ref,
> - const unsigned char *sha1)
> + const unsigned char *sha1, const char *fmt_pretty)
And this "const char *fmt_pretty" also becomes "void *cb_data"...
> {
> if (delete_ref(ref, sha1, 0))
> return 1;
> @@ -103,9 +104,15 @@ static int delete_tag(const char *name, const char *ref,
> }
>
> static int verify_tag(const char *name, const char *ref,
> - const unsigned char *sha1)
> + const unsigned char *sha1, const char *fmt_pretty)
... and here. Reintroduce fmt_pretty as a name local to the
function by doing the same thing as for_each_tag_name() above.
> {
> - return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE);
> + int flags;
> + flags = GPG_VERIFY_VERBOSE;
> +
> + if (fmt_pretty)
> + flags = GPG_VERIFY_QUIET;
> +
> + return verify_and_format_tag(sha1, name, fmt_pretty, flags);
> }
>
> static int do_sign(struct strbuf *buffer)
> @@ -424,9 +431,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> if (filter.merge_commit)
> die(_("--merged and --no-merged option are only allowed with -l"));
> if (cmdmode == 'd')
> - return for_each_tag_name(argv, delete_tag);
> - if (cmdmode == 'v')
> - return for_each_tag_name(argv, verify_tag);
> + return for_each_tag_name(argv, delete_tag, NULL);
> + if (cmdmode == 'v') {
> + if (format)
> + verify_ref_format(format);
> + return for_each_tag_name(argv, verify_tag, format);
> + }
Thanks.
^ permalink raw reply
* Re: [PATCH] git-gui: Do not reset author details on amend
From: Stefan Beller @ 2016-09-27 17:45 UTC (permalink / raw)
To: Orgad Shaneh; +Cc: Junio C Hamano, Pat Thoyts, git
In-Reply-To: <CAGHpTB+XOxMh=o_dKGx-j2P15zsmcMn0MB7BuKFksOn4O_FXYw@mail.gmail.com>
On Tue, Sep 27, 2016 at 10:23 AM, Orgad Shaneh <orgads@gmail.com> wrote:
> On Tue, Sep 27, 2016 at 7:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Orgad Shaneh <orgads@gmail.com> writes:
>>
>>> On Tue, Sep 27, 2016 at 12:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> Orgad Shaneh <orgads@gmail.com> writes:
>>>>
>>> I actually see that there is a problem with it:
>>> https://github.com/git-for-windows/git/issues/761
>>>
>>> I'll try to revise it and resubmit.
>>
>> Are you by chance volunteering to be git-gui maintainer?
>
> No way, sorry. I don't speak TCL at all. Every change is a true pain... ;)
I considered stepping up as an interim maintainer briefly, but this is
the exact reason
on why I punted.
Looking at e.g. `git diff --stat gitgui-0.19.0..gitgui-0.20.0` (which
is 16 month apart),
the workload seems to be very light, so I would not have concerns
w.r.t. time spent.
Stepping back a bit and asking "What does a maintainer do?" I think knowing the
language very well is not the top point, but rather looking at the design,
maintainability of proposed solutions as well as long term well being
of the project
is what makes a good maintainer.
That said I could step up as a maintainer, but for each patch I"d ask
the contributor to
find a reviewer who knows tcl well, as that is a part that I cannot
cover. And from a
contributors perspective this seems to be discouraging.
Stefan
>
> - Orgad
^ permalink raw reply
* Re: [PATCH v2 4/5] builtin/verify-tag: add --format to verify-tag
From: Junio C Hamano @ 2016-09-27 17:41 UTC (permalink / raw)
To: santiago; +Cc: git, peff, sunshine, walters
In-Reply-To: <20160926224233.32702-5-santiago@nyu.edu>
santiago@nyu.edu writes:
> From: Santiago Torres <santiago@nyu.edu>
>
> Callers of verify-tag may want to cross-check the tagname from refs/tags
> with the tagname from the tag object header upon GPG verification. This
> is to avoid tag refs that point to an incorrect object.
>
> Add a --format parameter to git verify-tag to print the formatted tag
> object header in addition to or instead of the --verbose or --raw GPG
> verification output.
>
> Signed-off-by: Santiago Torres <santiago@nyu.edu>
> ---
> builtin/verify-tag.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> index de10198..a941053 100644
> --- a/builtin/verify-tag.c
> +++ b/builtin/verify-tag.c
> @@ -12,12 +12,15 @@
> #include <signal.h>
> #include "parse-options.h"
> #include "gpg-interface.h"
> +#include "ref-filter.h"
>
> static const char * const verify_tag_usage[] = {
> - N_("git verify-tag [-v | --verbose] <tag>..."),
> + N_("git verify-tag [-v | --verbose] [--format=<format>] <tag>..."),
> NULL
> };
>
> +static char *fmt_pretty;
> +
I'd suggest to remove this, and then ...
> static int git_verify_tag_config(const char *var, const char *value, void *cb)
> {
> int status = git_gpg_config(var, value, cb);
> @@ -33,6 +36,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
... instead add
char *fmt_pretty = NULL;
here, imitating the way "&flags" is handled. You do not need it to
be visible outside the function.
> const struct option verify_tag_options[] = {
> OPT__VERBOSE(&verbose, N_("print tag contents")),
> OPT_BIT(0, "raw", &flags, N_("print raw gpg status output"), GPG_VERIFY_RAW),
> + OPT_STRING( 0 , "format", &fmt_pretty, N_("format"), N_("format to use for the output")),
> OPT_END()
> };
>
> @@ -46,12 +50,17 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
> if (verbose)
> flags |= GPG_VERIFY_VERBOSE;
>
> + if (fmt_pretty) {
> + verify_ref_format(fmt_pretty);
> + flags |= GPG_VERIFY_QUIET;
> + }
> +
> while (i < argc) {
> unsigned char sha1[20];
> const char *name = argv[i++];
> if (get_sha1(name, sha1))
> had_error = !!error("tag '%s' not found.", name);
> - else if (verify_and_format_tag(sha1, name, NULL, flags))
> + else if (verify_and_format_tag(sha1, name, fmt_pretty, flags))
> had_error = 1;
> }
> return had_error;
^ permalink raw reply
* Re: [PATCH v2 1/5] gpg-interface, tag: add GPG_VERIFY_QUIET flag
From: Junio C Hamano @ 2016-09-27 17:36 UTC (permalink / raw)
To: santiago; +Cc: git, peff, sunshine, walters, Lukas P
In-Reply-To: <20160926224233.32702-2-santiago@nyu.edu>
santiago@nyu.edu writes:
> From: Lukas P <luk.puehringer@gmail.com>
>
> Functions that print git object information may require that the
> gpg-interface functions be silent. Add GPG_VERIFY_QUIET flag and prevent
> print_signature_buffer from being called if flag is set.
>
> Signed-off-by: Lukas P <luk.puehringer@gmail.com>
Are you and Lukas sure that "Lukas P" is how luk.puehringer wants to
be known by the world? Just checking.
^ permalink raw reply
* Re: [PATCH v2 2/5] ref-filter: add function to print single ref_array_item
From: Junio C Hamano @ 2016-09-27 17:35 UTC (permalink / raw)
To: santiago; +Cc: git, peff, sunshine, walters, Lukas P
In-Reply-To: <20160926224233.32702-3-santiago@nyu.edu>
santiago@nyu.edu writes:
> From: Lukas P <luk.puehringer@gmail.com>
>
> ref-filter functions are useful for printing git object information
> using a format specifier. However, some other modules may not want to use
> this functionality on a ref-array but only print a single item.
>
> Expose a format_ref function to create, pretty print and free individual
> ref-items.
>
> Signed-off-by: Lukas P <luk.puehringer@gmail.com>
> ---
> +void format_ref(const char *name, const unsigned char *sha1, const char *format,
> + unsigned kind)
> +{
> + struct ref_array_item *ref_item;
> + ref_item = new_ref_array_item(name, sha1, 0);
> + ref_item->kind = kind;
> + show_ref_array_item(ref_item, format, 0);
> + free_array_item(ref_item);
> +}
> +
> /* If no sorting option is given, use refname to sort as default */
> struct ref_sorting *ref_default_sorting(void)
> {
> diff --git a/ref-filter.h b/ref-filter.h
> index 14d435e..1ef7999 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -107,4 +107,8 @@ struct ref_sorting *ref_default_sorting(void);
> /* Function to parse --merged and --no-merged options */
> int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);
>
> +/* Pretty-print a single ref */
> +void format_ref(const char *name, const unsigned char *sha1, const char *format,
> + unsigned kind);
The fact that you felt a need for comment before its name is a
strong sign that the name is not sufficiently descriptive and
understandable for readers to tell what the function is for.
Would pretty_print_ref() or show_ref_pretty() better names, perhaps?
^ permalink raw reply
* Re: [PATCH] gpg-interface: use more status letters
From: Junio C Hamano @ 2016-09-27 17:25 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Alex
In-Reply-To: <06b5e0bfb898c7b3c6a866df5bda0ab4587284c6.1474986563.git.git@drmicha.warpmail.net>
Michael J Gruber <git@drmicha.warpmail.net> writes:
> According to gpg2's doc/DETAILS:
> "For each signature only one of the codes GOODSIG, BADSIG, EXPSIG,
> EXPKEYSIG, REVKEYSIG or ERRSIG will be emitted."
>
> gpg1 ("classic") behaves the same (although doc/DETAILS
> differs).
>
> Currently, we parse gpg's status output for GOODSIG, BADSIG and trust
> information and translate that into status codes G, B, U, N for the %G?
> format specifier.
>
> git-verify-* returns success in the GOODSIG case only. This is somewhat in
> disagreement with gpg, which considers the first 5 of the 6 above as VALIDSIG,
> but we err on the very safe side.
>
> Introduce additional status codes E, X, R for ERRSIG, EXP*SIG, REVKEYSIG
> so that a user of %G? gets more information about the absence of a 'G'
> on first glance.
>
> Reported-by: Alex <agrambot@gmail.com>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
That probably was requested-by, but that's OK.
> I'd be happy to learn are more portable/safer/cooler way to make gpg forget
> that key in the added test...
We seem to set GNUPGHOME to $HOME/gnupg-home-not-used in test-lib.sh
to say "No gnupg keys for you!" for all the tests by default, which
is overriden by the signature tests like 7510. I do not know if
that is more portable/safer/cooler than setting it to /dev/null but
imitating it might be a way for you to push the potential problem
away to other people ;-) If it becomes an issue to set it to a
directory that does not exist with an updated future version of GPG,
this new test will share the same problem with everybody else, and
hopefully the solution would be the same ;-)
Having said that, if GNUPGHOME=/dev/null works for you, that's good
enough for now, so that people on other platforms can test it and
report.
Thanks.
> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
> index 6e839f5..fd22742 100755
> --- a/t/t7510-signed-commit.sh
> +++ b/t/t7510-signed-commit.sh
> @@ -190,7 +190,7 @@ test_expect_success GPG 'show bad signature with custom format' '
> test_cmp expect actual
> '
>
> -test_expect_success GPG 'show unknown signature with custom format' '
> +test_expect_success GPG 'show untrusted signature with custom format' '
> cat >expect <<-\EOF &&
> U
> 61092E85B7227189
> @@ -200,6 +200,15 @@ test_expect_success GPG 'show unknown signature with custom format' '
> test_cmp expect actual
> '
>
> +test_expect_success GPG 'show unknown signature with custom format' '
> + cat >expect <<-\EOF &&
> + E
> + 61092E85B7227189
> + EOF
> + GNUPGHOME=/dev/null git log -1 --format="%G?%n%GK" eighth-signed-alt >actual &&
> + test_cmp expect actual
> +'
> +
> test_expect_success GPG 'show lack of signature with custom format' '
> cat >expect <<-\EOF &&
> N
^ permalink raw reply
* Re: [PATCH] git-gui: Do not reset author details on amend
From: Orgad Shaneh @ 2016-09-27 17:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Pat Thoyts, git
In-Reply-To: <xmqqd1jpqp7p.fsf@gitster.mtv.corp.google.com>
On Tue, Sep 27, 2016 at 7:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Orgad Shaneh <orgads@gmail.com> writes:
>
>> On Tue, Sep 27, 2016 at 12:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Orgad Shaneh <orgads@gmail.com> writes:
>>>
>> I actually see that there is a problem with it:
>> https://github.com/git-for-windows/git/issues/761
>>
>> I'll try to revise it and resubmit.
>
> Are you by chance volunteering to be git-gui maintainer?
No way, sorry. I don't speak TCL at all. Every change is a true pain... ;)
- Orgad
^ permalink raw reply
* [Q] would it be bad to make /etc/gitconfig runtime configurable?
From: Junio C Hamano @ 2016-09-27 17:05 UTC (permalink / raw)
To: git
The subject says it all. Would it be bad if we introduce an
environment variable, GIT_SYSTEM_CONFIG=/etc/gitconfig, that names
an alternative location of the system-wide configuration file?
That would supersede/deprecate GIT_CONFIG_NOSYSTEM that we
introduced primarily so that we can run our tests without getting
affected by the configuration that happens to be effective on the
host that the test is being run.
^ permalink raw reply
* Re: [PATCH v3 3/3] add David Turner's Two Sigma address
From: Junio C Hamano @ 2016-09-27 17:03 UTC (permalink / raw)
To: David Turner; +Cc: git, peff, David Turner
In-Reply-To: <1474989806-5002-3-git-send-email-dturner@twosigma.com>
David Turner <dturner@twosigma.com> writes:
> From: David Turner <novalis@novalis.org>
>
> Signed-off-by: David Turner <novalis@novalis.org>
> ---
> .mailmap | 1 +
> 1 file changed, 1 insertion(+)
Thanks. Queued separately in order to merge to master much earlier
than the tree-fsck topic.
>
> diff --git a/.mailmap b/.mailmap
> index 9441a54..9cc33e9 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -48,6 +48,7 @@ David Kågedal <davidk@lysator.liu.se>
> David Reiss <dreiss@facebook.com> <dreiss@dreiss-vmware.(none)>
> David S. Miller <davem@davemloft.net>
> David Turner <novalis@novalis.org> <dturner@twopensource.com>
> +David Turner <novalis@novalis.org> <dturner@twosigma.com>
> Deskin Miller <deskinm@umich.edu>
> Dirk Süsserott <newsletter@dirk.my1.cc>
> Eric Blake <eblake@redhat.com> <ebb9@byu.net>
^ permalink raw reply
* Re: [PATCH v3 1/3] tree-walk: be more specific about corrupt tree errors
From: Junio C Hamano @ 2016-09-27 16:55 UTC (permalink / raw)
To: David Turner; +Cc: git, peff
In-Reply-To: <1474989806-5002-1-git-send-email-dturner@twosigma.com>
David Turner <dturner@twosigma.com> writes:
> From: Jeff King <peff@peff.net>
>
> When the tree-walker runs into an error, it just calls
> die(), and the message is always "corrupt tree file".
> However, we are actually covering several cases here; let's
> give the user a hint about what happened.
>
> Let's also avoid using the word "corrupt", which makes it
> seem like the data bit-rotted on disk. Our sha1 check would
> already have found that. These errors are ones of data that
> is malformed in the first place.
>
> Signed-off-by: David Turner <dturner@twosigma.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> t/t1007-hash-object.sh | 21 +++++++++++++++++++--
> tree-walk.c | 12 +++++++-----
> 2 files changed, 26 insertions(+), 7 deletions(-)
Nice that we now prepare the test data ourselves without shipping as
part of the source.
^ permalink raw reply
* Re: [PATCH] rev-list-options: clarify the usage of -n/--max-number
From: Junio C Hamano @ 2016-09-27 16:51 UTC (permalink / raw)
To: Pranit Bauva; +Cc: git
In-Reply-To: <010201576bfb6c7d-0b68228f-9503-4dd1-9721-713477fa2596-000000@eu-west-1.amazonses.com>
Pranit Bauva <pranit.bauva@gmail.com> writes:
> -n=<number>, -<number>, --max-number=<number> shows the last n commits
> specified in <number> irrespective of whether --reverse is used or not.
> With --reverse, it just shows the last n commits in reverse order.
I think it is easier to understand if you updated the description of
"--reverse", rather than "-<n>". "rev-list -n $N" that stops after
showing $N commits is something everybody understands. What often
dissapoints some users is that "--reverse" kicks in _after_ what
commits are to be shown are decided.
> Documentation/rev-list-options.txt | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index 7e462d3..6b7c2e5 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -18,7 +18,7 @@ ordering and formatting options, such as `--reverse`.
> -<number>::
> -n <number>::
> --max-count=<number>::
> - Limit the number of commits to output.
> + Limit to last n number of commits to output specified in <number>.
These essentially say the same thing. The original does not mention
where and how <number> is used, but "Limit the number of commits" as
a description for "-<number>" would be understood by anybody halfway
intelligent that the given number is used as that limit, so I do not
think an updated description is making it easier to understand.
There is a paragraph of interest in an earlier part of "Commit
Limiting" section (which is the section "-n" appears in, among other
options):
Note that these are applied before commit
ordering and formatting options, such as `--reverse`.
So the documentation already makes an attempt to avoid confusion
Ruediger saw, i.e. "rev-list traverses, limits the output to N, and
then shows these N commits in reverse" is what it expects readers to
understand, and that it also expects it would lead naturally to
"these N commits are still from the newest part of the history,
hence 'rev-list --reverse -n N' is not how you grab the earliest N".
But apparently the attempt by the current documentation is not
enough. Let's see how it describes the '--reverse' option:
Commit Ordering
~~~~~~~~~~~~~~~
By default, the commits are shown in reverse chronological order.
...
--reverse::
Output the commits in reverse order.
Cannot be combined with `--walk-reflogs`.
Perhaps "Output the commits chosen to be shown (see Commit Limiting
section above) in reverse order." would make it clearer?
^ permalink raw reply
* Re: git 2.9.2: is RUNTIME_PREFIX supposed to work?
From: Junio C Hamano @ 2016-09-27 16:37 UTC (permalink / raw)
To: Paul Smith; +Cc: Git Mailing List
In-Reply-To: <xmqqh991qpyn.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Paul Smith <paul@mad-scientist.net> writes:
>
>> On Mon, 2016-09-26 at 14:57 -0700, Junio C Hamano wrote:
>>> On Mon, Sep 26, 2016 at 2:32 PM, Paul Smith <paul@mad-scientist.net> wrote:
>>> >
>>> > Hi all. I'm trying to create a relocatable installation of Git 2.9.2,
>>> > so I can copy it anywhere and it continues to run without any problem.
>>> > This is on GNU/Linux systems, FWIW.
>>>
>>> I had an impression that the setting was only to support MS Windows.
>>
>> Hm. You may be right. If so that's too bad, because a relocatable Git
>> is very handy even on UNIX systems. Is there a reason for invoking the
>> subcommands by providing the plain command ("fetch", "merge-base") as
>> argv[0], rather than giving the fully-qualified path to a Git command?
>
> I do not think of any reason offhand. It just is that we never
> needed it.
If you are talking about invoking "git-fetch", then there is a very
good reason. Built-in's do not need any actual binary on the
filesystem (they only need "git").
But that does not have any relevance to the part below.
> If you want to add support without making the resulting codebase too
> ugly, without breaking the classic way of installing into a fixed
> locations, and without breaking the existing support of platforms
> that does know the runtime-prefix thing, not just I wouldn't mind
> but I would welcome such an addition ;-)
If you can make runtime-prefix honored on more platforms, that would
be good, though you _might_ have just added another "without" to the
above list: without using full paths e.g. /usr/local/git/bin/git-fetch
unconditionally.
^ permalink raw reply
* Re: [PATCH] git-gui: Do not reset author details on amend
From: Junio C Hamano @ 2016-09-27 16:31 UTC (permalink / raw)
To: Orgad Shaneh; +Cc: Pat Thoyts, git
In-Reply-To: <CAGHpTBLPvQDD4hhMKiOFdhxug-joi-38mNozuzm3=EJCnS9UEA@mail.gmail.com>
Orgad Shaneh <orgads@gmail.com> writes:
> On Tue, Sep 27, 2016 at 12:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Orgad Shaneh <orgads@gmail.com> writes:
>>
> I actually see that there is a problem with it:
> https://github.com/git-for-windows/git/issues/761
>
> I'll try to revise it and resubmit.
Are you by chance volunteering to be git-gui maintainer?
^ permalink raw reply
* Re: [PATCH] xdiff: rename "struct group" to "struct xdlgroup"
From: Stefan Beller @ 2016-09-27 16:22 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Haggerty, git@vger.kernel.org
In-Reply-To: <20160927043733.u3emlanbipu2cn5h@sigill.intra.peff.net>
On Mon, Sep 26, 2016 at 9:37 PM, Jeff King <peff@peff.net> wrote:
> Commit e8adf23 (xdl_change_compact(): introduce the concept
> of a change group, 2016-08-22) added a "struct group" type
> to xdiff/xdiffi.c. But the POSIX system header "grp.h"
> already defines "struct group" (it is part of the getgrnam
> interface). This happens to work because the new type is
> local to xdiffi.c, and the xdiff code includes a relatively
> small set of system headers. But it will break compilation
> if xdiff ever switches to using git-compat-util.h. It can
> also probably cause confusion with tools that look at the
> whole code base, like coccinelle or ctags.
>
> Let's resolve by giving the xdiff variant a scoped name,
> which is closer to other xdiff types anyway (e.g.,
> xdlfile_t, though note that xdiff is fond if typedefs when
> Git usually is not).
Makes sense!
Thanks,
Stefan
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I didn't rename the functions, which have no conflict, but that would
> also be closer to xdiff's usual style. I don't know how far it is worth
> going; maybe this patch is even already too far.
>
> I noticed because I have a patch series which switches xdiff
> to git-compat-util, to try to use the st_* macros there.
>
> xdiff/xdiffi.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
> index 67c1ccc..760fbb6 100644
> --- a/xdiff/xdiffi.c
> +++ b/xdiff/xdiffi.c
> @@ -708,7 +708,7 @@ static int score_cmp(struct split_score *s1, struct split_score *s2)
> * Note that loops that are testing for changed lines in xdf->rchg do not need
> * index bounding since the array is prepared with a zero at position -1 and N.
> */
> -struct group {
> +struct xdlgroup {
> /*
> * The index of the first changed line in the group, or the index of
> * the unchanged line above which the (empty) group is located.
> @@ -725,7 +725,7 @@ struct group {
> /*
> * Initialize g to point at the first group in xdf.
> */
> -static void group_init(xdfile_t *xdf, struct group *g)
> +static void group_init(xdfile_t *xdf, struct xdlgroup *g)
> {
> g->start = g->end = 0;
> while (xdf->rchg[g->end])
> @@ -736,7 +736,7 @@ static void group_init(xdfile_t *xdf, struct group *g)
> * Move g to describe the next (possibly empty) group in xdf and return 0. If g
> * is already at the end of the file, do nothing and return -1.
> */
> -static inline int group_next(xdfile_t *xdf, struct group *g)
> +static inline int group_next(xdfile_t *xdf, struct xdlgroup *g)
> {
> if (g->end == xdf->nrec)
> return -1;
> @@ -752,7 +752,7 @@ static inline int group_next(xdfile_t *xdf, struct group *g)
> * Move g to describe the previous (possibly empty) group in xdf and return 0.
> * If g is already at the beginning of the file, do nothing and return -1.
> */
> -static inline int group_previous(xdfile_t *xdf, struct group *g)
> +static inline int group_previous(xdfile_t *xdf, struct xdlgroup *g)
> {
> if (g->start == 0)
> return -1;
> @@ -769,7 +769,7 @@ static inline int group_previous(xdfile_t *xdf, struct group *g)
> * following group, expand this group to include it. Return 0 on success or -1
> * if g cannot be slid down.
> */
> -static int group_slide_down(xdfile_t *xdf, struct group *g, long flags)
> +static int group_slide_down(xdfile_t *xdf, struct xdlgroup *g, long flags)
> {
> if (g->end < xdf->nrec &&
> recs_match(xdf->recs[g->start], xdf->recs[g->end], flags)) {
> @@ -790,7 +790,7 @@ static int group_slide_down(xdfile_t *xdf, struct group *g, long flags)
> * into a previous group, expand this group to include it. Return 0 on success
> * or -1 if g cannot be slid up.
> */
> -static int group_slide_up(xdfile_t *xdf, struct group *g, long flags)
> +static int group_slide_up(xdfile_t *xdf, struct xdlgroup *g, long flags)
> {
> if (g->start > 0 &&
> recs_match(xdf->recs[g->start - 1], xdf->recs[g->end - 1], flags)) {
> @@ -818,7 +818,7 @@ static void xdl_bug(const char *msg)
> * size.
> */
> int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
> - struct group g, go;
> + struct xdlgroup g, go;
> long earliest_end, end_matching_other;
> long groupsize;
> unsigned int blank_lines;
> --
> 2.10.0.492.g14f803f
^ permalink raw reply
* Re: git 2.9.2: is RUNTIME_PREFIX supposed to work?
From: Junio C Hamano @ 2016-09-27 16:15 UTC (permalink / raw)
To: Paul Smith; +Cc: Git Mailing List
In-Reply-To: <1474982294.3190.32.camel@mad-scientist.net>
Paul Smith <paul@mad-scientist.net> writes:
> On Mon, 2016-09-26 at 14:57 -0700, Junio C Hamano wrote:
>> On Mon, Sep 26, 2016 at 2:32 PM, Paul Smith <paul@mad-scientist.net> wrote:
>> >
>> > Hi all. I'm trying to create a relocatable installation of Git 2.9.2,
>> > so I can copy it anywhere and it continues to run without any problem.
>> > This is on GNU/Linux systems, FWIW.
>>
>> I had an impression that the setting was only to support MS Windows.
>
> Hm. You may be right. If so that's too bad, because a relocatable Git
> is very handy even on UNIX systems. Is there a reason for invoking the
> subcommands by providing the plain command ("fetch", "merge-base") as
> argv[0], rather than giving the fully-qualified path to a Git command?
I do not think of any reason offhand. It just is that we never
needed it.
If you want to add support without making the resulting codebase too
ugly, without breaking the classic way of installing into a fixed
locations, and without breaking the existing support of platforms
that does know the runtime-prefix thing, not just I wouldn't mind
but I would welcome such an addition ;-)
Thanks.
^ permalink raw reply
* Re: git-upload-pack hangs
From: Junio C Hamano @ 2016-09-27 16:02 UTC (permalink / raw)
To: Jason Pyeron; +Cc: git
In-Reply-To: <50DEA1C222E449F59BC8488C921D6F95@black7>
"Jason Pyeron" <jpyeron@pdinc.us> writes:
> This is a very, very first draft.
>
> It is allowing IIS to work right now.
>
> I still need to address chunked issues, where there is no content length (see http://www.gossamer-threads.com/lists/apache/users/373042)
>
> Any comments, sugestions?
Does this have any relation to another thread earlier this year,
which seems to have ended here:
https://public-inbox.org/git/20160401235532.GA27941@sigill.intra.peff.net/
^ permalink raw reply
* Re: Possible integer overflow parsing malformed objects in git 2.10.0
From: Junio C Hamano @ 2016-09-27 15:57 UTC (permalink / raw)
To: Jeff King; +Cc: Gustavo Grieco, git
In-Reply-To: <20160927080755.evlq3sfkyoolixop@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> That being said, the parse_sha1_header() function clearly does not
> detect overflow at all when parsing the size. So on a 32-bit system, you
> end up with:
>
> $ git fsck
> fatal: Out of memory, malloc failed (tried to allocate 4294967141 bytes)
>
> which is not correct, but I'm not sure it's a security problem. Integer
> overflows are an issue if they cause us to under-allocate, and then to
> write more bytes than we allocated. In this case, I would expect
> unpack_sha1_rest() to never write more bytes than the "size" we parsed
> and allocated (and to complain if the number of bytes we get from the
> zlib sequence do not exactly match the claimed size).
>
> So a more interesting example is more like "ULONG_MAX + 5", where we
> would overflow to 5 bytes. And we'd hope that unpack_sha1_rest does not
> ever write more than 5 bytes. From my reading and a few tests with gdb,
> it does not. However, it also does not notice that there were more bytes
> that we didn't use.
>
> So I think there's room for improved diagnosis of bogus situations
> (including integer overflows), but I don't see any actual security bugs.
I agree with the overall conclusion. This does look like an attempt
to throw random fuzz at Git and see if and how it breaks, and in this
particular one Git is simply doing the right thing (and the fault lies
in the way how ASAN is used and how its result was interpreted).
Throwing random fuzz to see what breaks is not a bad thing to do
per-se, but anybody who does so without wearing a black hat needs to
keep two things in mind:
* When a random fuzz attempt does uncover a security issue,
reporting it here on this list is a grossly irresponsible way to
disclose the issue. We have the git-security list for that.
* A random fuzz may stop Git and that may be perfectly legit thing
to happen, e.g. the data may request a large but still valid
amount of memory to be allocated that happens not to fit in the
hardware the fuzz attempt is being run, and xmalloc() may detect
the situation and die, like the above example. False positives
are expected and you want to make sure you cull them before
making your reports. Otherwise, they will unnecessary burden
people who are doing real work, i.e. reproduce and correct
problems that may be security related that are irresponsibly
disclosed here quickly enough to minimize damage.
Thanks.
^ permalink raw reply
* Re: [PATCH v8 11/11] convert: add filter.<driver>.process option
From: Jakub Narębski @ 2016-09-27 15:37 UTC (permalink / raw)
To: Lars Schneider, git
Cc: Jeff King, Junio C Hamano, Stefan Beller, Martin-Louis Bright,
Torsten Bögershausen, Ramsay Jones
In-Reply-To: <20160920190247.82189-12-larsxschneider@gmail.com>
Part second of the review of 11/11.
W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:
> diff --git a/contrib/long-running-filter/example.pl b/contrib/long-running-filter/example.pl
> new file mode 100755
> index 0000000..c13a631
> --- /dev/null
> +++ b/contrib/long-running-filter/example.pl
[...]
> +( packet_txt_read() eq ( 0, "git-filter-client" ) ) || die "bad initialize";
> +( packet_txt_read() eq ( 0, "version=2" ) ) || die "bad version";
> +( packet_bin_read() eq ( 1, "" ) ) || die "bad version end";
What I would like to see here is some kind of packet_read_list()
or packet_txt_read_list() that reads until flush packet or EOF,
and returns list of chomp-ed lines (without LF terminator).
Then you can examine those lines:
my @lines = packet_read_list();
$lines[0] eq "git-filter-client" or die "bad initialization: '$lines[0]'";
grep { $_ eq "version=2" } @lines or die "bad version: version=2 not found";
Note: I have not checked that I got operator precedence right.
> +
> +packet_txt_write("git-filter-server");
> +packet_txt_write("version=2");
Here using packet_write_list() would help us to not forget to
send the flush packet:
packet_write_list(
"git-filter-server",
"version=2"
);
[...]
> diff --git a/convert.c b/convert.c
> index 597f561..bd66257 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -3,6 +3,7 @@
> #include "run-command.h"
> #include "quote.h"
> #include "sigchain.h"
> +#include "pkt-line.h"
>
> /*
> * convert.c - convert a file when checking it out and checking it in.
> @@ -442,7 +443,7 @@ static int filter_buffer_or_fd(int in, int out, void *data)
> return (write_err || status);
> }
>
> -static int apply_filter(const char *path, const char *src, size_t len, int fd,
> +static int apply_single_file_filter(const char *path, const char *src, size_t len, int fd,
> struct strbuf *dst, const char *cmd)
> {
> /*
> @@ -456,12 +457,6 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,
> struct async async;
> struct filter_params params;
>
> - if (!cmd || !*cmd)
> - return 0;
> -
> - if (!dst)
> - return 1;
> -
> memset(&async, 0, sizeof(async));
> async.proc = filter_buffer_or_fd;
> async.data = ¶ms;
I have reordered a few chunks of this patch to make it easier to
see what happens here, and to review this part of patch.
[moved here from further in the patch]
> +static int apply_filter(const char *path, const char *src, size_t len,
> + int fd, struct strbuf *dst, struct convert_driver *drv,
> + const unsigned int wanted_capability)
> +{
> + const char *cmd = NULL;
> +
> + if (!drv)
> + return 0;
> +
> + if (!dst)
> + return 1;
To reduce the size of this patch (which is not yet of the size that
would make vger reject the email :-/), perhaps this split into
apply_single_file_filter() and apply_filter(), without yet adding
apply_multi_file_filter(). The apply_filter() would be semi-simple
wrapper, with the same signature as above.
> +
> + if (!drv->process && (CAP_CLEAN & wanted_capability) && drv->clean)
This is just a very minor nitpicking, but wouldn't it be easier
to read with those checks reordered?
+ if ((wanted_capability & CAP_CLEAN) && !drv->process && drv->clean)
That is: if we want `clean` capability, and `process` is not set,
and there is `clean` filter.
Though the following would also work...
+ if ((wanted_capability & CAP_CLEAN) && !drv->process)
> + cmd = drv->clean;
> + else if (!drv->process && (CAP_SMUDGE & wanted_capability) && drv->smudge)
> + cmd = drv->smudge;
> +
> + if (cmd && *cmd)
... thanks to this check (the 'cmd' part, which we need to check anyway).
'cmd = drv->clean', then 'if (cmd)' is the same as 'if (drv->clean)',
then 'cmd = drv->clean', then 'if (cmd)', isn't it.
Not sure if it would be more readable, or less readable.
CAP_CLEAN and CAP_SMUDGE are, in theory, mutually exclusive. Note that
the above order prefers `smudge` to `clean` if both given, while in other
places we prefer `clean` to `smudge` if both given.
> + return apply_single_file_filter(path, src, len, fd, dst, cmd);
> + else if (drv->process && *drv->process)
> + return apply_multi_file_filter(path, src, len, fd, dst, drv->process, wanted_capability);
> +
> + return 0;
> +}
Nice and clean wrapper.
> +
[moved here from further in the patch]
> @@ -839,7 +1140,7 @@ int would_convert_to_git_filter_fd(const char *path)
> if (!ca.drv->required)
> return 0;
>
> - return apply_filter(path, NULL, 0, -1, NULL, ca.drv->clean);
> + return apply_filter(path, NULL, 0, -1, NULL, ca.drv, CAP_CLEAN);
> }
>
> const char *get_convert_attr_ascii(const char *path)
This would also be a part of move adding apply_single_file_filter()
and converting apply_filter() to wrapper around single / multi file
filters.
> @@ -493,14 +488,317 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,
> return !err;
> }
>
> +#define CAP_CLEAN (1u<<0)
> +#define CAP_SMUDGE (1u<<1)
It's a pity that ANSI C does not include binary constants, like e.g.
modern Perl, that is '0b0001u' and '0b0010u'; we could use hexadecimal
constants '0x01u' and '0x02u', but perhaps the above is more readable,
and as performant.
> +
> +struct cmd2process {
> + struct hashmap_entry ent; /* must be the first member! */
> + unsigned int supported_capabilities;
> + const char *cmd;
> + struct child_process process;
> +};
> +
> +static int cmd_process_map_initialized;
> +static struct hashmap cmd_process_map;
> +
> +static int cmd2process_cmp(const struct cmd2process *e1,
> + const struct cmd2process *e2,
> + const void *unused)
> +{
> + return strcmp(e1->cmd, e2->cmd);
> +}
> +
> +static struct cmd2process *find_multi_file_filter_entry(struct hashmap *hashmap, const char *cmd)
> +{
> + struct cmd2process key;
> + hashmap_entry_init(&key, strhash(cmd));
> + key.cmd = cmd;
> + return hashmap_get(hashmap, &key, NULL);
> +}
All right, basic hashmap for the list of command processes, here
so that we can find correct driver for current file, and reuse it
if it was started already.
I see that git code does not use /* ............ */ etc to separate
subsections / subparts of a file, so I won't ask for it ;-(
> +
> +static void kill_multi_file_filter(struct hashmap *hashmap, struct cmd2process *entry)
> +{
> + if (!entry)
> + return;
> + sigchain_push(SIGPIPE, SIG_IGN);
> + /*
> + * We kill the filter most likely because an error happened already.
> + * That's why we are not interested in any error code here.
> + */
Good explanation.
> + close(entry->process.in);
> + close(entry->process.out);
> + sigchain_pop(SIGPIPE);
> + finish_command(&entry->process);
> + hashmap_remove(hashmap, entry, NULL);
> + free(entry);
> +}
That's more 'kill_and_remove_...', but that would make too long
function name.
Small and readable. Nice.
> +
> +static int packet_write_list(int fd, const char *line, ...)
> +{
> + va_list args;
> + int err;
> + va_start(args, line);
> + for (;;) {
> + if (!line)
> + break;
> + if (strlen(line) > PKTLINE_DATA_MAXLEN)
Here we see that having PKTLINE_DATA_MAXLEN (or LARGE_PACKET_DATA_MAX)
constant is quite useful.
> + return -1;
> + err = packet_write_fmt_gently(fd, "%s\n", line);
I wonder if adding the fact that we are writing text packets
to function name would be worth it. Nah. Also, it is file-local
(static) function.
> + if (err)
> + return err;
> + line = va_arg(args, const char*);
> + }
> + va_end(args);
> + return packet_flush_gently(fd);
> +}
Nice abstraction.
> +
> +static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, const char *cmd)
> +{
> + int err;
> + struct cmd2process *entry;
> + struct child_process *process;
> + const char *argv[] = { cmd, NULL };
> + struct string_list cap_list = STRING_LIST_INIT_NODUP;
> + char *cap_buf;
> + const char *cap_name;
> +
> + entry = xmalloc(sizeof(*entry));
> + hashmap_entry_init(entry, strhash(cmd));
> + entry->cmd = cmd;
> + entry->supported_capabilities = 0;
> + process = &entry->process;
> +
> + child_process_init(process);
> + process->argv = argv;
> + process->use_shell = 1;
> + process->in = -1;
> + process->out = -1;
> +
> + if (start_command(process)) {
> + error("cannot fork to run external filter '%s'", cmd);
> + kill_multi_file_filter(hashmap, entry);
> + return NULL;
> + }
I guess there is a reason why we init hashmap entry, try to start
external process, then kill entry of unable to start, instead of
trying to start external process, and adding hashmap entry when
we succeed?
> +
> + sigchain_push(SIGPIPE, SIG_IGN);
I guess that this is here to handle errors writing to filter
by ourself, isn't it?
> +
> + err = packet_write_list(process->in, "git-filter-client", "version=2", NULL);
> + if (err)
> + goto done;
Ugh, error / exception handling in C.
> +
> + err = strcmp(packet_read_line(process->out, NULL), "git-filter-server");
> + if (err) {
> + error("external filter '%s' does not support long running filter protocol", cmd);
> + goto done;
> + }
> + err = strcmp(packet_read_line(process->out, NULL), "version=2");
> + if (err)
We could have described the error here better.
+ error("external filter '%s' does not support filter protocol version 2", cmd);
But this is probably not necessary; it should be rare to find
filter process that support the protocol halfway right.
> + goto done;
I guess this would get more complicated if/when there is need
for new version of the protocol.
Shouldn't we read flush packet here? Ah, sorry, we know that we
should get only two lines from the `process` filter driver, and
not variable number of lines, so there is no need to flush here.
Disregard my comments about lack of flush packet in the example
of long running filter script. Well, unless the protocol itself
would get adjusted to always use flush packet to terminate set
of metadata lines, even if number of lines is fixed.
> +
> + err = packet_write_list(process->in, "clean=true", "smudge=true", NULL);
So I see that Git sends all capabilities it supports, not only
those that given git command needs (which might be hard to find
out).
If it were possible at this point of code for Git to know, for
example, that it would only do `clean` operation, shouldn't it
write "clean=true", "smudge=false"? ;-PPP
Note that this "=true" is totally spurious. Maybe "capability=clean",
or just "clean" would make a better protocol?
> +
> + for (;;) {
> + cap_buf = packet_read_line(process->out, NULL);
> + if (!cap_buf)
> + break;
> + string_list_split_in_place(&cap_list, cap_buf, '=', 1);
> +
> + if (cap_list.nr != 2 || strcmp(cap_list.items[1].string, "true"))
> + continue;
> +
> + cap_name = cap_list.items[0].string;
> + if (!strcmp(cap_name, "clean")) {
> + entry->supported_capabilities |= CAP_CLEAN;
> + } else if (!strcmp(cap_name, "smudge")) {
> + entry->supported_capabilities |= CAP_SMUDGE;
> + } else {
> + warning(
> + "external filter '%s' requested unsupported filter capability '%s'",
> + cmd, cap_name
> + );
> + }
> +
> + string_list_clear(&cap_list, 0);
> + }
I guess there is a reason why it was not extracted into helper
function?
Well, both because handling of variable-length response, where
multiple lines must be analyzed, happens only once, and also
because returning list of variable-length strings in C is hard
(alloca? string_list?).
> +
> +done:
> + sigchain_pop(SIGPIPE);
> +
> + if (err || errno == EPIPE) {
> + error("initialization for external filter '%s' failed", cmd);
> + kill_multi_file_filter(hashmap, entry);
> + return NULL;
> + }
Good.
> +
> + hashmap_add(hashmap, entry);
> + return entry;
> +}
> +
> +static void read_multi_file_filter_values(int fd, struct strbuf *status) {
This is more
+static void read_multi_file_filter_status(int fd, struct strbuf *status) {
It doesn't read arbitrary values, it examines 'metadata' from
filter for "status=<foo>" lines.
> + struct strbuf **pair;
Shouldn't it be initialized to NULL, like in strbuf_split_buf()
code?
> + char *line;
> + for (;;) {
> + line = packet_read_line(fd, NULL);
> + if (!line)
> + break;
> + pair = strbuf_split_str(line, '=', 2);
Why, oh why, there is no Documentation/technical/api-strbuf.txt?
Well, strbuf.h is really well commented... but perhaps not enough.
> + if (pair[0] && pair[0]->len && pair[1]) {
> + if (!strcmp(pair[0]->buf, "status=")) {
> + strbuf_reset(status);
> + strbuf_addbuf(status, pair[1]);
> + }
So it is last status=<foo> line wins behavior?
> + }
Shouldn't we free 'struct strbuf **pair', maybe allocated by the
strbuf_split_str() function, and reset to NULL?
> + }
> +}
> +
> +static int apply_multi_file_filter(const char *path, const char *src, size_t len,
> + int fd, struct strbuf *dst, const char *cmd,
> + const unsigned int wanted_capability)
> +{
> + int err;
> + struct cmd2process *entry;
> + struct child_process *process;
> + struct stat file_stat;
> + struct strbuf nbuf = STRBUF_INIT;
This name doesn't tell us much, but I guess there is precedence?
> + struct strbuf filter_status = STRBUF_INIT;
> + char *filter_type;
> +
> + if (!cmd_process_map_initialized) {
> + cmd_process_map_initialized = 1;
> + hashmap_init(&cmd_process_map, (hashmap_cmp_fn) cmd2process_cmp, 0);
> + entry = NULL;
> + } else {
> + entry = find_multi_file_filter_entry(&cmd_process_map, cmd);
> + }
> +
> + fflush(NULL);
Why this fflush(NULL) is needed here?
> +
> + if (!entry) {
> + entry = start_multi_file_filter(&cmd_process_map, cmd);
> + if (!entry)
> + return 0;
> + }
> + process = &entry->process;
All right, we start process filter, or get existing instance.
> +
> + if (!(wanted_capability & entry->supported_capabilities))
> + return 0;
If filter doesn't support wanted capability, then Git just
wouldn't filter. Looks good to me.
> +
> + if (CAP_CLEAN & wanted_capability)
> + filter_type = "clean";
> + else if (CAP_SMUDGE & wanted_capability)
> + filter_type = "smudge";
> + else
> + die("unexpected filter type");
This should never happen; we should always request one of those
capabilities, and only one.
> +
> + if (fd >= 0 && !src) {
> + if (fstat(fd, &file_stat) == -1)
> + return 0;
> + len = xsize_t(file_stat.st_size);
> + }
Errr... is it necessary? The protocol no longer provides size=<n>
hint, and neither uses such hint if provided.
> +
> + sigchain_push(SIGPIPE, SIG_IGN);
Right, we want to handle errors ourself.
> +
> + err = strlen(filter_type) > PKTLINE_DATA_MAXLEN;
> + if (err)
> + goto done;
Errr... this should never happen. We control which capabilities
we pass, it can be only "clean" or "smudge", nothing else. Those
would always be shorter than PKTLINE_DATA_MAXLEN.
Never mind that that is "command=smudge\n" etc. that needs to
be shorter that PKTLINE_DATA_MAXLEN!
So, IMHO it should be at most assert, and needs to be corrected
anyway.
> +
> + err = packet_write_fmt_gently(process->in, "command=%s\n", filter_type);
> + if (err)
> + goto done;
> +
> + err = strlen(path) > PKTLINE_DATA_MAXLEN;
Actually
+ err = strlen(path) > PKTLINE_DATA_MAXLEN - strlen("pathname=\n");
This version was chosen in the very unlikely case if
strlen(path) + strlen("pathname=\n") would overflow.
+ err = strlen("pathname=") + strlen(path) + strlen("\n") > PKTLINE_DATA_MAXLEN;
;-)
> + if (err)
> + goto done;
This should never happen, PATH_MAX everywhere is much shorter
than PKTLINE_DATA_MAXLEN / LARGE_PACKET_MAX. Or is it?
Anyway, we should probably explain or warn
error("path name too long: '%s'", path);
Though if length of pathname is of the order of 2^16, I don't
think printing it would help :-)
> +
> + err = packet_write_fmt_gently(process->in, "pathname=%s\n", path);
> + if (err)
> + goto done;
> +
> + err = packet_flush_gently(process->in);
> + if (err)
> + goto done;
All right, this list of values, currently composed of "command=<sth>"
and "pathname=<sth>" - both of which are required, may be variable
length, so we need flush packet.
> +
> + if (fd >= 0)
> + err = write_packetized_from_fd(fd, process->in);
> + else
> + err = write_packetized_from_buf(src, len, process->in);
> + if (err)
> + goto done;
Looks good, and I think it is better if the caller decided rather
that write_packetized(fd, src, len, process->in) deciding.
Note for implementers: write in full, read in full, no streaming
support (Git doesn't start to read filter output until it writes
to filter in full). This is opposed to what `clean` and `smudge`
filters support.
> +
> + read_multi_file_filter_values(process->out, &filter_status);
> + err = strcmp(filter_status.buf, "success");
> + if (err)
> + goto done;
> +
> + err = read_packetized_to_buf(process->out, &nbuf) < 0;
> + if (err)
> + goto done;
> +
> + read_multi_file_filter_values(process->out, &filter_status);
> + err = strcmp(filter_status.buf, "success");
Looks good to me (LGTM).
> +
> +done:
> + sigchain_pop(SIGPIPE);
> +
> + if (err || errno == EPIPE) {
> + if (!strcmp(filter_status.buf, "error")) {
> + /* The filter signaled a problem with the file. */
> + } else if (!strcmp(filter_status.buf, "abort")) {
> + /*
> + * The filter signaled a permanent problem. Don't try to filter
> + * files with the same command for the lifetime of the current
> + * Git process.
> + */
> + entry->supported_capabilities &= ~wanted_capability;
> + } else {
> + /*
> + * Something went wrong with the protocol filter.
> + * Force shutdown and restart if another blob requires filtering!
Is this exclamation mark '!' here necessary?
> + */
> + error("external filter '%s' failed", cmd);
> + kill_multi_file_filter(&cmd_process_map, entry);
> + }
Looks good. Three error conditions: resumable error from filter,
failure of filter (kill, would restart if necessary), and abort.
> + } else {
> + strbuf_swap(dst, &nbuf);
> + }
> + strbuf_release(&nbuf);
> + return !err;
I guess this is for `filter.<driver>.required` to handle correctly
filter error-ing out, or filter failing, while not aborting
if filter simply doesn't support `clean` or `smudge` capability.
> +}
> +
> static struct convert_driver {
> const char *name;
> struct convert_driver *next;
> const char *smudge;
> const char *clean;
> + const char *process;
LGTM.
> int required;
> } *user_convert, **user_convert_tail;
>
[a section of chunk moved up]
> static int read_convert_config(const char *var, const char *value, void *cb)
> {
> const char *key, *name;
> @@ -538,6 +836,9 @@ static int read_convert_config(const char *var, const char *value, void *cb)
> if (!strcmp("clean", key))
> return git_config_string(&drv->clean, var, value);
>
> + if (!strcmp("process", key))
> + return git_config_string(&drv->process, var, value);
> +
LGTM.
> if (!strcmp("required", key)) {
> drv->required = git_config_bool(var, value);
> return 0;
[a chunk of diff moved up]
> @@ -872,18 +1173,12 @@ int convert_to_git(const char *path, const char *src, size_t len,
> struct strbuf *dst, enum safe_crlf checksafe)
> {
> int ret = 0;
> - const char *filter = NULL;
All right, this was (I think) moved into apply_filter()...
> - int required = 0;
...but this looks like just a removal of a temporary variable,
which could have been done in a separate preparatory patch.
> struct conv_attrs ca;
>
> convert_attrs(&ca, path);
> - if (ca.drv) {
> - filter = ca.drv->clean;
> - required = ca.drv->required;
> - }
>
> - ret |= apply_filter(path, src, len, -1, dst, filter);
> - if (!ret && required)
> + ret |= apply_filter(path, src, len, -1, dst, ca.drv, CAP_CLEAN);
> + if (!ret && ca.drv && ca.drv->required)
> die("%s: clean filter '%s' failed", path, ca.drv->name);
Looks good. (And could be a part of patch adding apply_filter()
as wrapper.)
>
> if (ret && dst) {
> @@ -905,9 +1200,9 @@ void convert_to_git_filter_fd(const char *path, int fd, struct strbuf *dst,
> convert_attrs(&ca, path);
>
> assert(ca.drv);
> - assert(ca.drv->clean);
> + assert(ca.drv->clean || ca.drv->process);
Hmmm... asserts. Well, they were here.
>
> - if (!apply_filter(path, NULL, 0, fd, dst, ca.drv->clean))
> + if (!apply_filter(path, NULL, 0, fd, dst, ca.drv, CAP_CLEAN))
> die("%s: clean filter '%s' failed", path, ca.drv->name);
>
Looks good. (And could be a part of patch adding apply_filter()
as wrapper.)
> crlf_to_git(path, dst->buf, dst->len, dst, ca.crlf_action, checksafe);
> @@ -919,15 +1214,9 @@ static int convert_to_working_tree_internal(const char *path, const char *src,
> int normalizing)
> {
> int ret = 0, ret_filter = 0;
> - const char *filter = NULL;
> - int required = 0;
> struct conv_attrs ca;
>
> convert_attrs(&ca, path);
> - if (ca.drv) {
> - filter = ca.drv->smudge;
> - required = ca.drv->required;
> - }
Well, this is the same change as a bit eaelier.
>
> ret |= ident_to_worktree(path, src, len, dst, ca.ident);
> if (ret) {
> @@ -936,9 +1225,10 @@ static int convert_to_working_tree_internal(const char *path, const char *src,
> }
> /*
> * CRLF conversion can be skipped if normalizing, unless there
> - * is a smudge filter. The filter might expect CRLFs.
> + * is a smudge or process filter (even if the process filter doesn't
> + * support smudge). The filters might expect CRLFs.
> */
> - if (filter || !normalizing) {
> + if ((ca.drv && (ca.drv->smudge || ca.drv->process)) || !normalizing) {
> ret |= crlf_to_worktree(path, src, len, dst, ca.crlf_action);
> if (ret) {
> src = dst->buf;
> @@ -946,8 +1236,8 @@ static int convert_to_working_tree_internal(const char *path, const char *src,
> }
> }
>
> - ret_filter = apply_filter(path, src, len, -1, dst, filter);
> - if (!ret_filter && required)
> + ret_filter = apply_filter(path, src, len, -1, dst, ca.drv, CAP_SMUDGE);
> + if (!ret_filter && ca.drv && ca.drv->required)
> die("%s: smudge filter %s failed", path, ca.drv->name);
>
> return ret | ret_filter;
Looks good to me. I understand ca.drv is checked so that ca.drv->required
and/or ca.drv->smudge / ca.drv->clean / ca.drv->process can be safely
checked, isn't it.
> @@ -1399,7 +1689,7 @@ struct stream_filter *get_stream_filter(const char *path, const unsigned char *s
> struct stream_filter *filter = NULL;
>
> convert_attrs(&ca, path);
> - if (ca.drv && (ca.drv->smudge || ca.drv->clean))
> + if (ca.drv && (ca.drv->process || ca.drv->smudge || ca.drv->clean))
LGTM, understandable change.
> return NULL;
>
> if (ca.crlf_action == CRLF_AUTO || ca.crlf_action == CRLF_AUTO_CRLF)
> diff --git a/pkt-line.h b/pkt-line.h
> index 6df8449..3d873f3 100644
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -86,6 +86,7 @@ ssize_t read_packetized_to_buf(int fd_in, struct strbuf *sb_out);
>
> #define DEFAULT_PACKET_MAX 1000
> #define LARGE_PACKET_MAX 65520
> +#define PKTLINE_DATA_MAXLEN (LARGE_PACKET_MAX - 4)
What the... didn't you use PKTLINE_DATA_MAXLEN in one of
earlier patches in this series? How this even...?
> extern char packet_buffer[LARGE_PACKET_MAX];
>
> #endif
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index dc50938..210c4f6 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
I'll stop here, and I'll finish the review later.
To be continued,
--
Jakub Narębski
^ permalink raw reply
* Re: git 2.9.2: is RUNTIME_PREFIX supposed to work?
From: Johannes Schindelin @ 2016-09-27 15:34 UTC (permalink / raw)
To: Paul Smith; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <1474982294.3190.32.camel@mad-scientist.net>
[-- Attachment #1: Type: text/plain, Size: 823 bytes --]
Hi Paul,
On Tue, 27 Sep 2016, Paul Smith wrote:
> On Mon, 2016-09-26 at 14:57 -0700, Junio C Hamano wrote:
> > On Mon, Sep 26, 2016 at 2:32 PM, Paul Smith <paul@mad-scientist.net> wrote:
> > >
> > > Hi all. I'm trying to create a relocatable installation of Git 2.9.2,
> > > so I can copy it anywhere and it continues to run without any problem.
> > > This is on GNU/Linux systems, FWIW.
> >
> > I had an impression that the setting was only to support MS Windows.
>
> Hm. You may be right. If so that's too bad, because a relocatable Git
> is very handy even on UNIX systems.
I see no reason why we have to keep the RUNTIME_PREFIX functional for
Windows only. Paul, how about giving it a try to fix things? I can make
sure that your changes do not break anything on Windows.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH 1/2] utf8: refactor code to decide fallback encoding
From: Junio C Hamano @ 2016-09-27 15:33 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20160927055202.4ucddki3xkns45om@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> But once we introduce other fallbacks, then "utf8 -> latin1" may become
> "UTF-8 -> iso8859-1". A system that knows only "utf8" and "iso8859-1"
> _could_ work if we turned the knobs individually, but won't if we turn
> them both at once. Worse, a system that knows only "UTF-8" and "latin1"
> works now, but would break with your patches.
>
> I'm not convinced it's worth worrying about, though. The existence of
> such a system is theoretical at this point. I'm not even sure how common
> the "know about utf8 but not UTF-8" thing is, or if we were merely being
> overly cautious.
Yeah, I did consider having to try the permutations until it works,
but suspecting that somebody takes "utf8" without taking "UTF-8" is
to pretty much invalidate the basic premise of the existing code,
i.e. spelling it as "UTF-8" is the most likely to work anywhere as
long as UTF-8 is supported, so I stopped worrying about it at that
point.
I'd actually welcome a more generic suggestions we can put in our
documentation so that we can _lose_ the fallback entirely (e.g. "if
your contributor spelled 'utf8' and your system, which does take
'UTF-8', does not like it, then here is what you can do to your
/etc/locale.alias").
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox