* our company's open source award for Git
From: Frank Graffagnino @ 2016-09-23 1:01 UTC (permalink / raw)
To: git
Not sure if this is the appropriate mailing list or not. If not, I apologize.
Our company (METECS) decided this year to try and give back to the
open source communities that help us do our work. We had a vote for
our 2016 METECS Open Source Award and Git came in 2nd place! So we
will be making a donation to Git at the SFC for $300 soon. We hope
that you all put it to good use.
Other winners this year were Blender and the Free Software Foundation.
Thanks to all of the people who put in work to make Git such a great
tool. We appreciate you.
Frank Graffagnino
METECS
^ permalink raw reply
* error
From: Luciano Schillagi @ 2016-09-22 23:02 UTC (permalink / raw)
To: git
Hi,
please, what should I do to fix this error? thanks
Luko ~ $ git init
error: malformed value for push.default: aguas
error: Must be one of nothing, matching, simple, upstream or current.
fatal: bad config variable 'push.default' in file '/Users/imac/.gitconfig' at line 16
-bash: __git_ps1: command not found
^ permalink raw reply
* Re: [PATCH v2] gitweb: use highlight's shebang detection
From: Jakub Narębski @ 2016-09-22 22:50 UTC (permalink / raw)
To: Ian Kelling, git
In-Reply-To: <20160921221856.27830-1-ian@iankelling.org>
W dniu 22.09.2016 o 00:18, Ian Kelling napisał:
> The highlight binary can detect language by shebang when we can't tell
> the syntax type by the name of the file. In that case, pass the blob
> to "highlight --force" and the resulting html will have markup for
> highlighting if the language was detected.
This description feels a bit convoluted. Perhaps something like this:
The "highlight" binary can, in some cases, determine the language type
by the means of file contents, for example the shebang in the first line
for some scripting languages. Make use of this autodetection for files
which syntax is not known by gitweb. In that case, pass the blob
contents to "highlight --force"; the parameter is needed to make it
always generate HTML output (which includes HTML-escaping).
Also, we might want to have the information about performance of this
solution either in the commit message, or in commit comments.
>
> Document the feature and improve syntax highlight documentation, add
> test to ensure gitweb doesn't crash when language detection is used,
All right.
> and remove an unused parameter from gitweb_check_feature().
First, that is guess_file_syntax(), not gitweb_check_feature().
Second, this change could be made into independent patch, for example
preparatory one.
>
> Signed-off-by: Ian Kelling <ian@iankelling.org>
> ---
> Documentation/gitweb.conf.txt | 21 ++++++++++++++-------
> gitweb/gitweb.perl | 14 +++++++-------
> t/t9500-gitweb-standalone-no-errors.sh | 8 ++++++++
> 3 files changed, 29 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
> index a79e350..e632089 100644
> --- a/Documentation/gitweb.conf.txt
> +++ b/Documentation/gitweb.conf.txt
> @@ -246,13 +246,20 @@ $highlight_bin::
> Note that 'highlight' feature must be set for gitweb to actually
> use syntax highlighting.
> +
> -*NOTE*: if you want to add support for new file type (supported by
> -"highlight" but not used by gitweb), you need to modify `%highlight_ext`
> -or `%highlight_basename`, depending on whether you detect type of file
> -based on extension (for example "sh") or on its basename (for example
> -"Makefile"). The keys of these hashes are extension and basename,
> -respectively, and value for given key is name of syntax to be passed via
> -`--syntax <syntax>` to highlighter.
> +*NOTE*: for a file to be highlighted, its syntax type must be detected
> +and that syntax must be supported by "highlight". The default syntax
> +detection is minimal, and there are many supported syntax types with no
> +detection by default. There are three options for adding syntax
> +detection. The first and second priority are `%highlight_basename` and
> +`%highlight_ext`, which detect based on basename (the full filename, for
> +example "Makefile") and extension (for example "sh"). The keys of these
> +hashes are the basename and extension, respectively, and the value for a
> +given key is the name of the syntax to be passed via `--syntax <syntax>`
> +to "highlight". The last priority is the "highlight" configuration of
> +`Shebang` regular expressions to detect the language based on the first
> +line in the file, (for example, matching the line "#!/bin/bash"). See
> +the highlight documentation and the default config at
> +/etc/highlight/filetypes.conf for more details.
> +
I think the rewrite is a bit more readable.
> For example if repositories you are hosting use "phtml" extension for
> PHP files, and you want to have correct syntax-highlighting for those
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 33d701d..44094f4 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3913,7 +3913,7 @@ sub blob_contenttype {
> # guess file syntax for syntax highlighting; return undef if no highlighting
> # the name of syntax can (in the future) depend on syntax highlighter used
> sub guess_file_syntax {
> - my ($highlight, $mimetype, $file_name) = @_;
> + my ($highlight, $file_name) = @_;
Right.
> return undef unless ($highlight && defined $file_name);
> my $basename = basename($file_name, '.in');
> return $highlight_basename{$basename}
> @@ -3931,15 +3931,16 @@ sub guess_file_syntax {
> # or return original FD if no highlighting
> sub run_highlighter {
> my ($fd, $highlight, $syntax) = @_;
> - return $fd unless ($highlight && defined $syntax);
> + return $fd unless ($highlight);
Run highlighter if it is defined, even if gitweb doesn't know syntax, right.
>
> close $fd;
> + my $syntax_arg = (defined $syntax) ? "--syntax $syntax" : "--force";
> open $fd, quote_command(git_cmd(), "cat-file", "blob", $hash)." | ".
> quote_command($^X, '-CO', '-MEncode=decode,FB_DEFAULT', '-pse',
> '$_ = decode($fe, $_, FB_DEFAULT) if !utf8::decode($_);',
> '--', "-fe=$fallback_encoding")." | ".
> quote_command($highlight_bin).
> - " --replace-tabs=8 --fragment --syntax $syntax |"
> + " --replace-tabs=8 --fragment $syntax_arg |"
Use '--force' if syntax is unknown, right.
> or die_error(500, "Couldn't open file or run syntax highlighter");
> return $fd;
> }
> @@ -7062,9 +7063,8 @@ sub git_blob {
> $have_blame &&= ($mimetype =~ m!^text/!);
>
> my $highlight = gitweb_check_feature('highlight');
> - my $syntax = guess_file_syntax($highlight, $mimetype, $file_name);
> - $fd = run_highlighter($fd, $highlight, $syntax)
> - if $syntax;
> + my $syntax = guess_file_syntax($highlight, $file_name);
> + $fd = run_highlighter($fd, $highlight, $syntax);
Remove unused parameter from callsite, *and* run highlighter even if we
don't know syntax.
>
> git_header_html(undef, $expires);
> my $formats_nav = '';
> @@ -7117,7 +7117,7 @@ sub git_blob {
> $line = untabify($line);
> printf qq!<div class="pre"><a id="l%i" href="%s#l%i" class="linenr">%4i</a> %s</div>\n!,
> $nr, esc_attr(href(-replay => 1)), $nr, $nr,
> - $syntax ? sanitize($line) : esc_html($line, -nbsp=>1);
> + $highlight ? sanitize($line) : esc_html($line, -nbsp=>1);
This is a bit of code duplication / sync from run_highlighter(), but
it is not your fault; it was there (and I don't know how to improve it).
> }
> }
> close $fd
> diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
> index e94b2f1..576db6d 100755
> --- a/t/t9500-gitweb-standalone-no-errors.sh
> +++ b/t/t9500-gitweb-standalone-no-errors.sh
Nice.
> @@ -709,6 +709,14 @@ test_expect_success HIGHLIGHT \
> git commit -m "Add test.sh" &&
> gitweb_run "p=.git;a=blob;f=test.sh"'
>
> +test_expect_success HIGHLIGHT \
> + 'syntax highlighting (highlighter language autodetection)' \
> + 'git config gitweb.highlight yes &&
Modern way would be
+ 'test_config gitweb.highlight yes &&
but other tests in this file do not use it.
> + echo "#!/usr/bin/ruby" > test &&
Preferred style would be
+ echo "#!/usr/bin/ruby" >test &&
but other tests in this file do not use it.
Sidenote: why Ruby, and not sh / bash, Perl or Python?
> + git add test &&
> + git commit -m "Add test" &&
> + gitweb_run "p=.git;a=blob;f=test"'
> +
> # ----------------------------------------------------------------------
> # forks of projects
>
>
Thank you for your work.
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH v2 2/2] mailinfo: unescape quoted-pair in header fields
From: Junio C Hamano @ 2016-09-22 22:17 UTC (permalink / raw)
To: Jeff King; +Cc: Kevin Daudt, git, Swift Geek
In-Reply-To: <20160921110934.f6eu2dz6i2mlpa45@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Sep 19, 2016 at 08:54:40PM +0200, Kevin Daudt wrote:
>
>> + ...
>> + while ((c = *in++) != 0) {
>> + if (take_next_literally) {
>> + take_next_literally = 0;
>> + } else {
>> [...]
>> + }
>> +
>> + strbuf_addch(line, c);
>> + }
>> +}
>
> It needs to `free(in)` at the end of the function.
Ehh, in has been incremented and is pointing at the terminating NUL
there, so it would be more like
char *to_free, *in;
to_free = strbuf_detach(line, NULL);
in = to_free;
...
while ((c = *in++)) {
...
}
free(to_free);
I would think ;-).
^ permalink raw reply
* Re: What's cooking in git.git (Sep 2016, #05; Mon, 19)
From: Junio C Hamano @ 2016-09-22 22:14 UTC (permalink / raw)
To: Jeff King; +Cc: Kevin Daudt, git
In-Reply-To: <20160922064931.ganuwswlnom6nzya@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Sep 21, 2016 at 07:45:50PM +0200, Kevin Daudt wrote:
>
>> On Wed, Sep 21, 2016 at 10:36:57AM -0700, Junio C Hamano wrote:
>> > Kevin Daudt <me@ikke.info> writes:
>> >
>> > > On Mon, Sep 19, 2016 at 04:30:34PM -0700, Junio C Hamano wrote:
>> > >>
>> > >> * kd/mailinfo-quoted-string (2016-09-19) 2 commits
>> > >> - mailinfo: unescape quoted-pair in header fields
>> > >> - t5100-mailinfo: replace common path prefix with variable
>> > >
>> > > Is this good enough, or do you want me to look into the feedback from
>> > > jeff?
>> >
>> > If you are talking about the simplified loop that deliberately sets
>> > a rule that is looser than RFC, yes, I'd like to see you at least
>> > consider the pros and cons of his approach, which looked nicer to my
>> > brief reading of it.
>> >
>> > It is perfectly OK by me (it may not be so if you ask Peff) if you
>> > decide that your version is better after doing so, though.
>>
>> Alright, I'll look into it.
>
> Thanks. I am OK if we do not use my simplified version, but I think
> there were some issues I noted with your last version.
Yup, even some automated tool noticed a new leak ;-)
^ permalink raw reply
* Re: [PATCH 6/6] builtin/tag: add --format argument for tag -v
From: Junio C Hamano @ 2016-09-22 21:23 UTC (permalink / raw)
To: santiago; +Cc: git, peff, sunshine, walters, Lukas P, Lukas Puehringer
In-Reply-To: <20160922185317.349-7-santiago@nyu.edu>
santiago@nyu.edu writes:
> @@ -425,8 +431,11 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> 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')
> + if (cmdmode == 'v') {
> + if (fmt_pretty)
> + verify_ref_format(fmt_pretty);
> return for_each_tag_name(argv, verify_tag);
> + }
OK, you said something about for_each_ref() in an earlier commit,
but what you meant was this one, which takes each_tag_name_fn.
The function for_each_tag_name(), the type each_tag_name_fn, and the
function of that type verify_tag(), are ALL file-scope static in
this single file, builtin/tag.c. It seems to me that it is not
necessary to make the format string global at all.
> @@ -425,8 +431,11 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> 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')
> + if (cmdmode == 'v') {
> + if (fmt_pretty)
> + verify_ref_format(fmt_pretty);
> return for_each_tag_name(argv, verify_tag);
> + }
There are minor implementation and design issues I spotted, but
overall I think the feature the series attempts to add may be a good
thing to have.
Thanks.
^ permalink raw reply
* Re: [PATCH 5/6] builtin/verify-tag: Add --format to verify-tag
From: Junio C Hamano @ 2016-09-22 21:16 UTC (permalink / raw)
To: santiago; +Cc: git, peff, sunshine, walters
In-Reply-To: <20160922185317.349-6-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 7a1121b..319d469 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
> };
>
> +char *fmt_pretty;
Does this have to be extern? I do not think so; prepend "static "
in front of it.
> 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))
> + if (verify_and_format_tag(sha1, name, fmt_pretty, flags))
OK. The callchain from here is
verify_and_format_tag()
-> run_gpg_verify()
-> print_signature_buffer()
so not cramming QUIET into the flags parameter that is already
passed is cumbersome. As I said in my earlier review, it would make
more sense to have the conditional NOT in print_signature_buffer()
but in its caller, but it still is OK to add GPG_VERIFY_QUIET bit
to the flag, which you would check in run_gpg_verify() to decide not
to call print_signature_buffer().
^ permalink raw reply
* Re: Limitiations of git rebase --preserve-merges --interactive
From: Stefan Beller @ 2016-09-22 21:08 UTC (permalink / raw)
To: Anatoly Borodin, Johannes Schindelin; +Cc: git@vger.kernel.org
In-Reply-To: <ns1gr4$pjs$1@blaine.gmane.org>
On Thu, Sep 22, 2016 at 2:01 PM, Anatoly Borodin
<anatoly.borodin@gmail.com> wrote:
> Hi Stefan,
>
> I've also done some archaeology and found that the original version of
> the merge preserving code was written by Johannes Schindelin
> <Johannes.Schindelin@gmx.de>, see e.g.
I think it would be helpful if you'd cc those folks involved, not just
the mailing list.
^ permalink raw reply
* Re: Limitiations of git rebase --preserve-merges --interactive
From: Anatoly Borodin @ 2016-09-22 21:04 UTC (permalink / raw)
To: git
In-Reply-To: <20160922194848.GB6641@ikke.info>
Hi Kevin,
Kevin Daudt <me@ikke.info> wrote:
> Changing the order, or dropping commits might then give unexpected
> results.
The question that Stefan has is rather "what is *supposed* to work /
give *expected* results?". Some stuff can be found in the tests
(t/t*rebase*preserve*), but maybe there is more?
--
Mit freundlichen Grüßen,
Anatoly Borodin
^ permalink raw reply
* Re: Limitiations of git rebase --preserve-merges --interactive
From: Anatoly Borodin @ 2016-09-22 21:01 UTC (permalink / raw)
To: git
In-Reply-To: <1mtveu4.19lvgi1c0hmhaM%lists@haller-berlin.de>
Hi Stefan,
I've also done some archaeology and found that the original version of
the merge preserving code was written by Johannes Schindelin
<Johannes.Schindelin@gmx.de>, see e.g.
f09c9b8c5ff9d8a15499b09ccd6c3e7b3c76af77
There were also some big discussion threads in 2007-2008 regarding a
better mechanism to "mark" or "tag" the heads of rebased branches, but I
haven't seen consensus and development in that direction after that.
PS There are also some pieces of "what should work" in these tests:
t/t3409-rebase-preserve-merges.sh*
t/t3410-rebase-preserve-dropped-merges.sh*
t/t3411-rebase-preserve-around-merges.sh*
t/t3414-rebase-preserve-onto.sh*
--
Mit freundlichen Grüßen,
Anatoly Borodin
^ permalink raw reply
* Re: [PATCH tg/add-chmod+x-fix 2/2] t3700-add: protect one --chmod=+x test with POSIXPERM
From: Thomas Gummerer @ 2016-09-22 21:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Git Mailing List
In-Reply-To: <xmqq60pp6jor.fsf@gitster.mtv.corp.google.com>
On 09/21, Junio C Hamano wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
>
> > But I came to a different conclusion as I said in a message that
> > crossed yours. I hope Thomas can pick up the baton again.
Sorry for not getting back earlier, my git time is quite limited
unfortunately.
> Yeah, our mails crossed, apparently, and I do agree with your
> reasoning. How about this, then?
Thanks, both the reasoning and the patch below make sense to me.
> -- >8 --
> From: Johannes Sixt <j6t@kdbg.org>
> Date: Tue, 20 Sep 2016 08:18:25 +0200
> Subject: [PATCH] t3700-add: do not check working tree file mode without POSIXPERM
>
> A recently introduced test checks the result of 'git status' after
> setting the executable bit on a file. This check does not yield the
> expected result when the filesystem does not support the executable
> bit.
>
> What we care about is that a file added with "--chmod=+x" has
> executable bit in the index and that "--chmod=+x" (or any other
> options for that matter) does not muck with working tree files.
> The former is tested by other existing tests, so let's check the
> latter more explicitly and only under POSIXPERM prerequisite.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> t/t3700-add.sh | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> index 16ab2da..924a266 100755
> --- a/t/t3700-add.sh
> +++ b/t/t3700-add.sh
> @@ -361,13 +361,11 @@ test_expect_success 'git add --chmod=[+-]x changes index with already added file
> test_mode_in_index 100644 xfoo3
> '
>
> -test_expect_success 'file status is changed after git add --chmod=+x' '
> - echo "AM foo4" >expected &&
> +test_expect_success POSIXPERM 'git add --chmod=[+-]x does not change the working tree' '
> echo foo >foo4 &&
> git add foo4 &&
> git add --chmod=+x foo4 &&
> - git status -s foo4 >actual &&
> - test_cmp expected actual
> + ! test -x foo4
> '
>
> test_expect_success 'no file status change if no pathspec is given' '
> --
> 2.10.0-515-g9036219
>
--
Thomas
^ permalink raw reply
* Re: [PATCH 4/6] tag: add format specifier to gpg_verify_tag
From: Junio C Hamano @ 2016-09-22 20:58 UTC (permalink / raw)
To: santiago; +Cc: git, peff, sunshine, walters, Lukas P, Lukas Puehringer
In-Reply-To: <20160922185317.349-5-santiago@nyu.edu>
santiago@nyu.edu writes:
> Calling functions for gpg_verify_tag() may desire to print relevant
> information about the header for further verification. Add an optional
> format argument to print any desired information after GPG verification.
> diff --git a/builtin/tag.c b/builtin/tag.c
> index dbf271f..94ed8a2 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -106,7 +106,7 @@ static int delete_tag(const char *name, const char *ref,
> static int verify_tag(const char *name, const char *ref,
> const unsigned char *sha1)
> {
> - return gpg_verify_tag(sha1, name, GPG_VERIFY_VERBOSE);
> + return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE);
> }
>
> static int do_sign(struct strbuf *buffer)
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> index 99f8148..7a1121b 100644
> --- a/builtin/verify-tag.c
> +++ b/builtin/verify-tag.c
> @@ -51,8 +51,10 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
> const char *name = argv[i++];
> if (get_sha1(name, sha1))
> had_error = !!error("tag '%s' not found.", name);
> - else if (gpg_verify_tag(sha1, name, flags))
> - had_error = 1;
> + else {
> + if (verify_and_format_tag(sha1, name, NULL, flags))
> + had_error = 1;
> + }
Revert the unnecessary reformatting here.
> @@ -56,6 +57,15 @@ int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report,
> ret = run_gpg_verify(buf, size, flags);
>
> free(buf);
> +
> + if (fmt_pretty) {
> + struct ref_array_item *ref_item;
> + ref_item = new_ref_item(name_to_report, sha1, 0);
> + ref_item->kind = FILTER_REFS_TAGS;
> + show_ref_item(ref_item, fmt_pretty, 0);
> + free_ref_item(ref_item);
> + }
I haven't seen 5/6 and 6/6, but if this is the only user of the 3/6,
it would be much better to have a single function to format a ref
exported from ref-filter.[ch] so that this one can say
if (fmt_pretty)
format_ref(name_to_report, sha1, FILTER_REFS_TAGS);
or something like that, instead of doing three that will always be
used together in quick succession in the above pattern.
^ permalink raw reply
* Re: Limitiations of git rebase --preserve-merges --interactive
From: Stefan Beller @ 2016-09-22 20:54 UTC (permalink / raw)
To: Kevin Daudt, lists; +Cc: Anatoly Borodin, git@vger.kernel.org, Jonathan Nieder
In-Reply-To: <20160922194848.GB6641@ikke.info>
On Thu, Sep 22, 2016 at 12:48 PM, Kevin Daudt <me@ikke.info> wrote:
> On Thu, Sep 22, 2016 at 07:33:11PM +0000, Anatoly Borodin wrote:
>> Hi Stefan,
>>
>> this section was added to the manual in the commit
>> cddb42d2c58a9de9b2b5ef68817778e7afaace3e by "Jonathan Nieder"
>> <jrnieder@gmail.com> 6 years ago. Maybe he remembers better?
>>
>
> Just to make it clear, this section explicitly talks about 'bugs' with
> preserve-merges and interactive rebase. Without the --preserve-merges
> option, those operations works as expected.
>
> The reason, as that section explains, is that it's not possible to store
> the merge structure in the flat todo list. I assume this means git
> internally remembers where the merge commit was, and then restores it
> while rebasing.
>
> Changing the order, or dropping commits might then give unexpected
> results.
>
The commit message may help as well:
rebase -i -p: document shortcomings
The rebase --preserve-merges facility presents a list of commits
in its instruction sheet and uses a separate table to keep
track of their parents. Unfortunately, in practice this means
that with -p after most attempts to rearrange patches, some
commits have the "wrong" parent and the resulting history is
rarely what the caller expected.
Yes, it would be nice to fix that. But first, add a warning to the
manual to help the uninitiated understand what is going on.
^ permalink raw reply
* Re: [PATCH 3/6] ref-filter: Expose wrappers for ref_item functions
From: Junio C Hamano @ 2016-09-22 20:50 UTC (permalink / raw)
To: santiago; +Cc: git, peff, sunshine, walters, Lukas P, Lukas Puehringer
In-Reply-To: <20160922185317.349-4-santiago@nyu.edu>
santiago@nyu.edu writes:
> From: Lukas P <luk.puehringer@gmail.com>
>
> Ref-filter functions are useful for printing git object information
> without a format specifier. However, some functions may not want to use
> a complete ref-array, and just a single item instead. Expose
> create/show/free functions for ref_array_items through wrappers around
> the original functions.
>
> Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
> ---
> ref-filter.c | 20 ++++++++++++++++++++
> ref-filter.h | 10 ++++++++++
> 2 files changed, 30 insertions(+)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 9adbb8a..b013799 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1329,6 +1329,14 @@ static struct ref_array_item *new_ref_array_item(const char *refname,
> return ref;
> }
>
> +/* Wrapper: Create ref_array_item w/o referencing container in function name */
> +struct ref_array_item *new_ref_item(const char *refname,
> + const unsigned char *objectname,
> + int flag)
> +{
> + return new_ref_array_item(refname, objectname, flag);
> +}
Why? As a public function name, new_ref_item() is a horrible one,
as there are other structures about "ref" elsewhere in the system.
If a new caller needs to be able to get a new ref_array_item, you
are better off just exposing it, not an ill-named wrapper.
> static int filter_ref_kind(struct ref_filter *filter, const char *refname)
> {
> unsigned int i;
> @@ -1426,6 +1434,12 @@ static void free_array_item(struct ref_array_item *item)
> free(item);
> }
>
> +/* Wrapper: Free ref_array_item w/o referencing container in function name */
> +void free_ref_item(struct ref_array_item *ref_item)
> +{
> + free_array_item(ref_item);
> +}
Again, why? free_array_item() is a horrible name for a public
function, and it is OK to rename it to free_ref_array_item() while
giving external callers an access to it, so that their names are
descriptive enough to convey that they are about ref_array_item
structure used in ref-filter API while at the same time making it
clear to readers that the two functions with related names are
indeed related.
> @@ -1637,6 +1651,12 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
> putchar('\n');
> }
>
> +/* Wrapper: Show ref_array_item w/o referencing container in function name */
> +void show_ref_item(struct ref_array_item *ref_item, const char *format, int quote_style)
> +{
> + show_ref_array_item(ref_item, format, quote_style);
> +}
Ditto.
^ permalink raw reply
* Re: [PATCH 2/6] gpg-interface: add GPG_VERIFY_QUIET flag
From: Junio C Hamano @ 2016-09-22 20:44 UTC (permalink / raw)
To: santiago; +Cc: git, peff, sunshine, walters, Lukas P, Lukas Puehringer
In-Reply-To: <20160922185317.349-3-santiago@nyu.edu>
santiago@nyu.edu writes:
> From: Lukas P <luk.puehringer@gmail.com>
Please match this with S-o-b: below.
>
> Functions that print git object information may require that the
> gpg-interface functions be silent. Add a GPG_VERIFY_QUIET to prevent
> functions such as `print_signature_buffer` from printing any output and
> only return whether signature verification passed or not.
>
> Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
> ---
> gpg-interface.c | 3 +++
> gpg-interface.h | 1 +
> 2 files changed, 4 insertions(+)
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 8672eda..b82bc50 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -88,6 +88,9 @@ int check_signature(const char *payload, size_t plen, const char *signature,
>
> void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
> {
> + if (flags & GPG_VERIFY_QUIET)
> + return;
> +
> const char *output = flags & GPG_VERIFY_RAW ?
> sigc->gpg_status : sigc->gpg_output;
This has only two callsites, which both know what flags they are
passing. Doesn't it make more sense to drop this patch (and
possibly addition of GPG_VERIFY_QUIET flag as well) and teach them
not to call this function in the first place?
> diff --git a/gpg-interface.h b/gpg-interface.h
> index ea68885..85dc982 100644
> --- a/gpg-interface.h
> +++ b/gpg-interface.h
> @@ -3,6 +3,7 @@
>
> #define GPG_VERIFY_VERBOSE 1
> #define GPG_VERIFY_RAW 2
> +#define GPG_VERIFY_QUIET 4
>
> struct signature_check {
> char *payload;
^ permalink raw reply
* Re: Request for large repo clone on slow intermittent connections
From: Kevin Daudt @ 2016-09-22 20:27 UTC (permalink / raw)
To: Aaron Gray; +Cc: Git Mailing List
In-Reply-To: <CANkmNDe7y8oYRDDz4uTUso0SaDn+jSGs=q3XTzKuavSWhnP4kg@mail.gmail.com>
On Thu, Sep 22, 2016 at 12:54:57PM +0100, Aaron Gray wrote:
> I am having problems cloning a 2.1GB repo from googlesource
>
> C:\Users\Aaron Gray\GitHub>git clone
> https://chromium.googlesource.com/chromium/chromium
> Cloning into 'chromium'...
> remote: Sending approximately 2.11 GiB ...
> error: fatal: The remote end hung up unexpectedly MiB | 2.74 MiB/s
> fatal: RPC failed; curl 56 SSL read:
> error:00000000:lib(0):func(0):reason(0), errno 10054
> early EOF
> fatal: index-pack failed
>
> I am repeatedly getting the same result on a 36MBit connection
>
> Hoping for a soulution.
>
> Regards,
>
> Aaron
Nothing much that can be done with a flaky connection. What git does
support are so called bundle files. These basically contain history that
you can import. Such a bundle can be offered through a resumable
transport, such as http.
I'll send you a link to a bundle for the chromium project.
^ permalink raw reply
* Re: [PATCH 2/6] gpg-interface: add GPG_VERIFY_QUIET flag
From: Junio C Hamano @ 2016-09-22 20:25 UTC (permalink / raw)
To: santiago; +Cc: git, peff, sunshine, walters, Lukas P, Lukas Puehringer
In-Reply-To: <20160922185317.349-3-santiago@nyu.edu>
santiago@nyu.edu writes:
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 8672eda..b82bc50 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -88,6 +88,9 @@ int check_signature(const char *payload, size_t plen, const char *signature,
>
> void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
> {
> + if (flags & GPG_VERIFY_QUIET)
> + return;
> +
> const char *output = flags & GPG_VERIFY_RAW ?
> sigc->gpg_status : sigc->gpg_output;
This will not compile with -Werror=declaration-after-statement.
^ permalink raw reply
* Re: [PATCH 1/6] builtin/tag: move format specifier to global var
From: Junio C Hamano @ 2016-09-22 20:23 UTC (permalink / raw)
To: santiago; +Cc: git, peff, sunshine, walters
In-Reply-To: <20160922185317.349-2-santiago@nyu.edu>
santiago@nyu.edu writes:
> From: Santiago Torres <santiago@nyu.edu>
>
> The format specifier will be likely used in other functions throughout
> git tag. One likely candidate to require format strings in the future is
> the gpg_verify_tag function. However, changing the signature of
> functions such as for_each_ref or verify_tag would be quite burdensome.
I do not understand the above excuse. for-each-ref takes a
callback data pointer exactly because it wants you to be able to
extend what data the callback function gets without changing its
signature. builtin/tag.c::verify_tag() is a helper static to the
file--why should it be "burdensome" to change it to fit your needs?
Adding technical debt by going backwards is never a good idea
especially done to add a new feature that is not desperately needed.
^ permalink raw reply
* Re: [PATCH] fetch-pack: do not reset in_vain on non-novel acks
From: Junio C Hamano @ 2016-09-22 20:05 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
In-Reply-To: <5a258c5dbed0683760e2ffb1bd6a1749ea66b2d5.1474568670.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> The MAX_IN_VAIN mechanism was introduced in commit f061e5f ("fetch-pack:
> give up after getting too many "ack continue"", 2006-05-24) to stop ref
> negotiation if a number of consecutive "have"s have been sent with no
> corresponding new acks. A use case (as described in that commit) is the
> scenario in which the local repository has more roots than the remote
> repository.
To those who know what the mechanism is about, the above is
sufficient to refresh their memory, but to others, a brief
explanation of _why_ it is a good idea to stop is needed to
understand what you are trying to achieve with this change.
It may help to add something like "This will stop the client to dig
too deep in an irrelevant side branch in vain without ever finding a
common ancestor." before "A use case is ...", perhaps?
By the way, you made me run "git show -W f061e5f" and then compare
it with "less fetch-pack.c"; I am kind of surprised to see that
find_common() has grown quite a bit over the years.
> However, during a negotiation in which stateless RPCs are used,
> MAX_IN_VAIN will (almost) never trigger (in the more-roots scenario
> above and others) because in each new request, the client has to inform
> the server of objects it already has and knows the server has (to remind
> the server of the state), which the server then acks.
Hmph. So the problem you are trying to solve is that the current
code sees that the other side said 'yeah, that is a common commit'
by giving us ACK common, and resets the in_vain counter, when in
fact we haven't made _any_ progress at that point.
> Make fetch-pack only consider novel acks (acks for objects for which the
> client has never received an ack before in this session) as new acks for
> the purpose of MAX_IN_VAIN.
Makes sense.
Just a hint, because you are relatively new to the project.
Whenever you are tempted to say "In other words...", "That
means...", or further elaborte in parentheses, it pays to stop and
think if you can do without whatever you said before that. In the
above paragraph and in the comment in the patch, a newly invented
term "novel ack" is used exactly once, and because it is a newly
invented word, you need to explain what you want it to mean, but
there is no need to do so. "Make fetch-pack only consider acks for
objects for which no earlier acks have been seen ..." is equally
readable and does not burden the readers with "Ah, the author
introduced a new term 'novel ack', so I need to remember that this
is the definition of the word when I see it mentioned next time".
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> fetch-pack.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 85e77af..1141e3c 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -428,10 +428,18 @@ static int find_common(struct fetch_pack_args *args,
> const char *hex = sha1_to_hex(result_sha1);
> packet_buf_write(&req_buf, "have %s\n", hex);
> state_len = req_buf.len;
> - }
> + /*
> + * Reset in_vain because this
> + * ack is a novel ack (that is,
> + * an ack for this commit has
> + * not been seen).
> + */
Side note. Having to wrap the multi-line comment like this is a
sign that the loop got a bit too big to fit in brain. We may want
to see if there is way to reduce the complexity by introducing a
helper function or something.
> + in_vain = 0;
> + } else if (!args->stateless_rpc
> + || ack != ACK_common)
> + in_vain = 0;
It is a bit hard to read this hunk without pre-context. The
original reads like so:
...
case ACK_common:
case ACK_ready:
case ACK_continue: {
struct commit *commit =
lookup_commit(result_sha1);
if (!commit)
die("invalid commit %s", sha1_to_hex(result_sha1));
if (args->stateless_rpc
&& ack == ACK_common
&& !(commit->object.flags & COMMON)) {
Here, they told us that this is a common ancestor by giving us "ACK
common", and this is not a response to our attempt to prime a new
incarnation of stateless server. It is curious that only ACK_common
is checked, but it is OK because --stateless requires multi-ack and
ACK_continue is not used.
/* We need to replay the have for this object
* on the next RPC request so the peer knows
* it is in common with us.
*/
const char *hex = sha1_to_hex(result_sha1);
packet_buf_write(&req_buf, "have %s\n", hex);
state_len = req_buf.len;
And we store it away so that the next found will start with these
objects as "have" to remind the other side where we were.
> + in_vain = 0;
And at this point, you reset in_vain counter with your change.
Which makes sense. This is a newly discovered common one, i.e. we
are making progress.
- }
> + } else if (!args->stateless_rpc
> + || ack != ACK_common)
> + in_vain = 0;
And you add an else clause here to reset in_vain counter, which we
used to unconditionally do, when stateless is not in use, or when we
are doing stateless and got something other than "ACK common". The
latter is to make sure that "ACK common" for commits we have already
known are common do not count as making progress.
Makes perfect sense to me.
mark_common(commit, 0, 1);
retval = 0;
- in_vain = 0;
And you remove the unconditional reset.
got_continue = 1;
if (ack == ACK_ready) {
clear_prio_queue(&rev_list);
got_ready = 1;
}
break;
}
}
Everything looks good to me and well thought-out.
Thanks.
^ permalink raw reply
* Re: Limitiations of git rebase --preserve-merges --interactive
From: Kevin Daudt @ 2016-09-22 19:48 UTC (permalink / raw)
To: Anatoly Borodin; +Cc: git
In-Reply-To: <ns1bln$2ej$1@blaine.gmane.org>
On Thu, Sep 22, 2016 at 07:33:11PM +0000, Anatoly Borodin wrote:
> Hi Stefan,
>
> this section was added to the manual in the commit
> cddb42d2c58a9de9b2b5ef68817778e7afaace3e by "Jonathan Nieder"
> <jrnieder@gmail.com> 6 years ago. Maybe he remembers better?
>
Just to make it clear, this section explicitly talks about 'bugs' with
preserve-merges and interactive rebase. Without the --preserve-merges
option, those operations works as expected.
The reason, as that section explains, is that it's not possible to store
the merge structure in the flat todo list. I assume this means git
internally remembers where the merge commit was, and then restores it
while rebasing.
Changing the order, or dropping commits might then give unexpected
results.
^ permalink raw reply
* Re: Limitiations of git rebase --preserve-merges --interactive
From: Anatoly Borodin @ 2016-09-22 19:33 UTC (permalink / raw)
To: git
In-Reply-To: <1mtveu4.19lvgi1c0hmhaM%lists@haller-berlin.de>
Hi Stefan,
this section was added to the manual in the commit
cddb42d2c58a9de9b2b5ef68817778e7afaace3e by "Jonathan Nieder"
<jrnieder@gmail.com> 6 years ago. Maybe he remembers better?
--
Mit freundlichen Grüßen,
Anatoly Borodin
^ permalink raw reply
* Re: [PATCH] do not reset in_vain on non-novel acks
From: Junio C Hamano @ 2016-09-22 19:20 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
In-Reply-To: <cover.1474568670.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> This is regarding the packfile negotiation in fetch-pack. If there is a
> concern that MAX_IN_VAIN would be hit too early (as a consequence of the
> patch below), I'm currently investigating the possibility of improving
> the negotiation ability of the client side further (for example, by
> prioritizing refs or heads instead of merely prioritizing by date in the
> priority queue of objects), but I thought I'd send the patch out first
> anyway to see what others think.
>
> Jonathan Tan (1):
> fetch-pack: do not reset in_vain on non-novel acks
>
> fetch-pack.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
Just a hint, because you are relatively new to the project. It
usually is not very productive to have a cover letter to a single
patch. Your cover letter either ends up being useless, or ends up
costing you time by having to repeat what you write for the patch
anyway (and making others to read it twice).
Below the "---" line of the single patch is often a better place to
tell a backstory of the patch if you need to.
^ permalink raw reply
* Re: .gitignore does not ignore Makefile
From: Junio C Hamano @ 2016-09-22 19:16 UTC (permalink / raw)
To: Timur Tabi; +Cc: Kevin Daudt, git
In-Reply-To: <57E4267B.1050507@codeaurora.org>
Timur Tabi <timur@codeaurora.org> writes:
> So .gitignore only ignores new files, not modified ones?
It is determines if an untracked file should be considered by "git
add" to add it or ignore it.
^ permalink raw reply
* Bug? Short command line options
From: Anatoly Borodin @ 2016-09-22 19:03 UTC (permalink / raw)
To: git
Hi All,
is there a good reason why
git fetch -vpnf
works like
git fetch -v -p -n -f
and
git commit -avem msg
works like
git commit -a -v -e -m msg
etc etc, but
git log -wWp
says
fatal: unrecognized argument: -wWp
?
--
Mit freundlichen Grüßen,
Anatoly Borodin
^ permalink raw reply
* Re: [RFC/PATCH 0/6] Add --format to tag verification
From: Stefan Beller @ 2016-09-22 19:01 UTC (permalink / raw)
To: Santiago Torres
Cc: git@vger.kernel.org, Junio C Hamano, Jeff King, Eric Sunshine,
walters
In-Reply-To: <20160922185317.349-1-santiago@nyu.edu>
On Thu, Sep 22, 2016 at 11:53 AM, <santiago@nyu.edu> wrote:
>
> P.S. Gmane seems to be broken for git after it was rebooted. Should we ping
> them about it?
I think most of the git developers have moved on and reference emails by
message id. An archive of all messages of the mailing list is found at
public-inbox.org/git/
(You can git-clone it to have a distributed copy of the whole archive)
public-inbox.org/git/<message-id>/
public-inbox.org/git/<message-id>/raw
is a good point to link to.
However, feel free to ping gmane. :)
Thanks,
Stefan
^ 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