* Re: [PATCH v2 1/5] gpg-interface, tag: add GPG_VERIFY_QUIET flag
From: Lukas Pühringer @ 2016-09-27 18:37 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, Santiago Torres, git@vger.kernel.org, Jeff King,
Eric Sunshine, walters
In-Reply-To: <CAGZ79kYitOd5-Cxr0VzhRH70B=z2gh1q2aB5xiwe+HwHuU7dYg@mail.gmail.com>
I made it a habit to use ‘ue’ instead of ‘ü' outside of German speaking countries and in coding. It makes my life easier.
But thanks for the hint.
Lukas
> On Sep 27, 2016, at 2:31 PM, Stefan Beller <sbeller@google.com> wrote:
>
> On Tue, Sep 27, 2016 at 11:25 AM, Lukas Pühringer
> <luk.puehringer@gmail.com> wrote:
>> Good, I will change it to 'Lukas Puehringer' then, when we send you the updated batch of patches, that address your latest comments.
>
> No need to stay full ASCII. German umlauts are fine.
> (See `git shortlog -s` for all the contributor names, there are also
> other alphabets in use)
>
> Stefan
^ permalink raw reply
* Re: [PATCH v2 1/5] gpg-interface, tag: add GPG_VERIFY_QUIET flag
From: Stefan Beller @ 2016-09-27 18:31 UTC (permalink / raw)
To: Lukas Pühringer
Cc: Junio C Hamano, Santiago Torres, git@vger.kernel.org, Jeff King,
Eric Sunshine, walters
In-Reply-To: <3D863101-CA73-4718-A1E7-A1664A5BDB58@gmail.com>
On Tue, Sep 27, 2016 at 11:25 AM, Lukas Pühringer
<luk.puehringer@gmail.com> wrote:
> Good, I will change it to 'Lukas Puehringer' then, when we send you the updated batch of patches, that address your latest comments.
No need to stay full ASCII. German umlauts are fine.
(See `git shortlog -s` for all the contributor names, there are also
other alphabets in use)
Stefan
^ permalink raw reply
* Re: [PATCH 2/4 v4] ls-files: optionally recurse into submodules
From: Junio C Hamano @ 2016-09-27 18:29 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <1474930003-83750-3-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> Allow ls-files to recognize submodules in order to retrieve a list of
> files from a repository's submodules. This is done by forking off a
> process to recursively call ls-files on all submodules. Use top-level
> --submodule_prefix option to pass a path to the submodule which it can
> use to prepend to output or pathspec matching logic.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
> Documentation/git-ls-files.txt | 7 +-
> builtin/ls-files.c | 143 ++++++++++++++++++++++++---------
> git.c | 2 +-
> t/t3007-ls-files-recurse-submodules.sh | 100 +++++++++++++++++++++++
> 4 files changed, 212 insertions(+), 40 deletions(-)
> create mode 100755 t/t3007-ls-files-recurse-submodules.sh
>
> diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> index 0d933ac..446209e 100644
> --- a/Documentation/git-ls-files.txt
> +++ b/Documentation/git-ls-files.txt
> @@ -18,7 +18,8 @@ SYNOPSIS
> [--exclude-per-directory=<file>]
> [--exclude-standard]
> [--error-unmatch] [--with-tree=<tree-ish>]
> - [--full-name] [--abbrev] [--] [<file>...]
> + [--full-name] [--recurse-submodules]
> + [--abbrev] [--] [<file>...]
>
> DESCRIPTION
> -----------
> @@ -137,6 +138,10 @@ a space) at the start of each line:
> option forces paths to be output relative to the project
> top directory.
>
> +--recurse-submodules::
> + Recursively calls ls-files on each submodule in the repository.
> + Currently there is only support for the --cached mode.
> +
> --abbrev[=<n>]::
> Instead of showing the full 40-byte hexadecimal object
> lines, show only a partial prefix.
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 00ea91a..d4bfc60 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -14,6 +14,7 @@
> #include "resolve-undo.h"
> #include "string-list.h"
> #include "pathspec.h"
> +#include "run-command.h"
>
> static int abbrev;
> static int show_deleted;
> @@ -28,6 +29,8 @@ static int show_valid_bit;
> static int line_terminator = '\n';
> static int debug_mode;
> static int show_eol;
> +static int recurse_submodules;
> +static const char *submodule_prefix;
I would have expected this to added to environment.c in the previous
step, but it is OK--I'd imagine you'd grab this from the environment
and carrying a piece of information from git.c to here by setenv()
followed by getenv() feels somewhat roundabout, though.
> static const char *prefix;
> static int max_prefix_len;
> @@ -68,6 +71,21 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path)
> static void write_name(const char *name)
> {
> /*
> + * NEEDSWORK: To make this thread-safe, full_name would have to be owned
> + * by the caller.
As Peff mentioned in his review in another thread, a large number of
functions in git are not reentrant, and I do not think we would want
to give the impression that those missing a warning are safe to use.
Other than that, this step looks OK. 3/4 and later would be a lot
more fun to review ;-)
^ permalink raw reply
* 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
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