* Re: .gitignore does not ignore Makefile
From: Jakub Narębski @ 2016-09-23 22:29 UTC (permalink / raw)
To: Junio C Hamano, Kevin Daudt; +Cc: Timur Tabi, git
In-Reply-To: <xmqqy42j4wp9.fsf@gitster.mtv.corp.google.com>
W dniu 22.09.2016 o 20:26, Junio C Hamano napisał:
> Kevin Daudt <me@ikke.info> writes:
>
>> Often people advise tricks like `git update-index --assume-unchanges
>> <file>`, but this does not work as expected. It's merely a promise to
>> git that this file does not change (and hence, git will not check if
>> this file has changed when doing git status), but command that try to
>> change this file will abort saying that the file has changed.
>
> It actually is even worse. As the user promised Git that the <file>
> will not be modified and will be kept the same as the version in the
> index, Git reserves the right to _overwrite_ it with the version in
> the index anytime when it is convenient to do so, removing whatever
> local change the user had despite the promise to Git. The "abort
> saying that the file has changed" is merely various codepaths in the
> current implementation trying to be extra nice.
There is a trick that works almost as 'ignore changes' for tracked
files, namely `git update-index --skip-worktree <file>`. From the
documentation:
Skip-worktree bit
~~~~~~~~~~~~~~~~~
Skip-worktree bit can be defined in one (long) sentence: When
reading an entry, if it is marked as skip-worktree, then Git
pretends its working directory version is up to date and read
the index version instead.
[...] Writing is not affected by this bit, content safety is still
first priority. [...]
It works quite well; the only problem is that `git stash` would
not stash away your changes, and you would need to unmark such
file before saving a stash.
With --assume-unchanged used for ignoring changes to tracked files,
you can quite easily lose your work because you are lying to Git.
Note also that in Git classic "ignored" implies unimportant.
--
Jakub Narębski
^ permalink raw reply
* Re: Limitiations of git rebase --preserve-merges --interactive
From: Stefan Beller @ 2016-09-23 22:26 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Stefan Haller, Kevin Daudt, Anatoly Borodin, git@vger.kernel.org,
Jonathan Nieder
In-Reply-To: <alpine.DEB.2.20.1609232305590.129229@virtualbox>
On Fri, Sep 23, 2016 at 2:13 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Stefan,
>
> On Fri, 23 Sep 2016, Stefan Haller wrote:
>
>> Stefan Beller <sbeller@google.com> wrote:
>>
>> > 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.
>>
>> Thanks, but all of this still talks about the issues in very generic
>> terms ("most attempts to rearrange patches"). I'm interested in more
>> details as to exactly what kind of attempts do or don't work. In
>> particular, I'm interested in fixup/squash commands (without reordering
>> anything else), or dropping (non-merge) commits.
>>
>> I could of course experiment with these and try to find out myself, but
>> I was hoping someone would just know the answer off the top of their
>> head, saving me some time.
>
> The fundamental problem here is the underlying design of bolting on the
> "recreate a merge" functionality onto the "pick" command.
>
> That is, if you try to rebase non-linear commit history, it will still
> generate a linear list of "pick <commit-name>" lines, as if it were
> linear, except that it will include the merge commits, too.
Which on a more fundamental design level would be ok.
(C.f. your shell history is a linear list of git commands, but it
deals just fine
with non linear DAGSs)
>
> It then will try to guess what you want to do by recording which commit
> was rewritten as which commit. And when it encounters a "pick" with a
> merge commit, it will try to merge the *rewritten* commit.
Instead of guessing we'd need to differentiate between "pick" and "pickmerge",
whereas the later describes creating commits with more than one parent (i.e.
the prior pick line).
I could imagine the "pickmerge" to list all additional parents (The
first parent being
the previously picked commit) via symbolic naming:
pick 1234affe implement foo
pickmerge 3456feed origin/js/new-feature-1 # Merge origin/js/new-feature-1
pick 45678ead implement feature-2
The "pickmerge" would have first the merge tips, and then the old
subject line after
a # character.
>
> In other words, the design does not allow for changing the tip of any
> merged branch. Not reordering, not dropping.
I see how the current design is problematic as there is no argument
possible that
allows the user to correct the wrong guess.
>
> And I do not think that there is a way to fix that design. That is why I
> came up with the Git garden shears (see the link I sent elsewhere in this
> thread).
I'll look into that.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v3 2/2] gitweb: use highlight's shebang detection
From: Jakub Narębski @ 2016-09-23 22:15 UTC (permalink / raw)
To: Ian Kelling, git
In-Reply-To: <20160923090846.3086-2-ian@iankelling.org>
W dniu 23.09.2016 o 11:08, Ian Kelling napisał:
> 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).
Right.
>
> Although we now run highlight on files which do not end up highlighted,
> performance is virtually unaffected because when we call highlight, we
> also call sanitize() instead of esc_html(), which is significantly
> slower.
This paragraph is a bit unclear, for example it is not obvious what
"..., which is significantly slower" refers to: sanitize() or esc_html().
I think it would be better to write:
Although we now run highlight on files which do not end up highlighted,
performance is virtually unaffected because when we call highlight, it
is used for escaping HTML. In the case that highlight is used, gitweb
calls sanitize() instead of esc_html(), and the latter is significantly
slower (it does more, being roughly a superset of sanitize()).
> After curling blob view of unhighlighted large and small text
> files of perl code and license text 100 times each on a local
> Apache/2.4.23 (Debian) instance, it's logs indicate +-1% difference in
> request time for all file types.
Also, "curling" is not the word I would like to see. I would say:
Simple benchmark comparing performance of 'blob' view of files without
syntax highlighting in gitweb before and after this change indicates
±1% difference in request time for all file types. Benchmark was
performed on local instance on Debian, using Apache/2.4.23 web server
and CGI/PSGI/FCGI/mod_perl.
^^^^^^^^^^^^^^^^^^^^^^--- select one
Or something like that; I'm not sure how detailed this should be.
But it is nice to have such benchmark in the commit message.
Anyway I think that adding yet another configuration toggle for selecting
whether to use "highlight" syntax autodetection or not would be just an
unnecessary complication.
Note that the performance loss might be quite higher on MS Windows, with
its higher cost of fork. But then they probably do not configure
server-side highligher anyway.
>
> Document the feature and improve syntax highlight documentation, add
> test to ensure gitweb doesn't crash when language detection is used.
Good.
>
> Signed-off-by: Ian Kelling <ian@iankelling.org>
> ---
> Documentation/gitweb.conf.txt | 21 ++++++++++++++-------
> gitweb/gitweb.perl | 10 +++++-----
> t/t9500-gitweb-standalone-no-errors.sh | 8 ++++++++
> 3 files changed, 27 insertions(+), 12 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::
We should probably say what does it mean to be "highlight"[1] compatible,
but it is outside of scope for this patch, and I think also out of scope
of this series.
> 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.
All right. I guess /etc/highlight/filetypes.conf is the standard location?
> +
> 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 6cb4280..44094f4 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -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);
>
> 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 |"
> or die_error(500, "Couldn't open file or run syntax highlighter");
> return $fd;
> }
All right, nice and understandable.
> @@ -7063,8 +7064,7 @@ sub git_blob {
>
> my $highlight = gitweb_check_feature('highlight');
> my $syntax = guess_file_syntax($highlight, $file_name);
> - $fd = run_highlighter($fd, $highlight, $syntax)
> - if $syntax;
> + $fd = run_highlighter($fd, $highlight, $syntax);
>
> git_header_html(undef, $expires);
> my $formats_nav = '';
Good, run unconditionally.
> @@ -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);
> }
Good, use highlighter if possible, not only if syntax is known
and highlighter is turned on.
Nice and easy to understand after earlier change.
> }
> close $fd
> diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
> index e94b2f1..6d06ed9 100755
> --- a/t/t9500-gitweb-standalone-no-errors.sh
> +++ b/t/t9500-gitweb-standalone-no-errors.sh
> @@ -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 &&
> + echo "#!/usr/bin/perl" > test &&
> + git add test &&
> + git commit -m "Add test" &&
> + gitweb_run "p=.git;a=blob;f=test"'
Nice.
> +
> # ----------------------------------------------------------------------
> # forks of projects
>
>
^ permalink raw reply
* Re: Limitiations of git rebase --preserve-merges --interactive
From: Johannes Schindelin @ 2016-09-23 21:13 UTC (permalink / raw)
To: Stefan Haller
Cc: Stefan Beller, Kevin Daudt, Anatoly Borodin, git@vger.kernel.org,
Jonathan Nieder
In-Reply-To: <1mu0zrn.cs3kf31kttpjkM%lists@haller-berlin.de>
Hi Stefan,
On Fri, 23 Sep 2016, Stefan Haller wrote:
> Stefan Beller <sbeller@google.com> wrote:
>
> > 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.
>
> Thanks, but all of this still talks about the issues in very generic
> terms ("most attempts to rearrange patches"). I'm interested in more
> details as to exactly what kind of attempts do or don't work. In
> particular, I'm interested in fixup/squash commands (without reordering
> anything else), or dropping (non-merge) commits.
>
> I could of course experiment with these and try to find out myself, but
> I was hoping someone would just know the answer off the top of their
> head, saving me some time.
The fundamental problem here is the underlying design of bolting on the
"recreate a merge" functionality onto the "pick" command.
That is, if you try to rebase non-linear commit history, it will still
generate a linear list of "pick <commit-name>" lines, as if it were
linear, except that it will include the merge commits, too.
It then will try to guess what you want to do by recording which commit
was rewritten as which commit. And when it encounters a "pick" with a
merge commit, it will try to merge the *rewritten* commit.
In other words, the design does not allow for changing the tip of any
merged branch. Not reordering, not dropping.
And I do not think that there is a way to fix that design. That is why I
came up with the Git garden shears (see the link I sent elsewhere in this
thread).
Ciao,
Johannes
^ permalink raw reply
* Re: Limitiations of git rebase --preserve-merges --interactive
From: Johannes Schindelin @ 2016-09-23 21:04 UTC (permalink / raw)
To: Stefan Beller; +Cc: Anatoly Borodin, Stefan Haller, git@vger.kernel.org
In-Reply-To: <CAGZ79kadpe5wG0gSpk-oasxCGuUp+3bSv-A4vhyFC+y0oL-+ww@mail.gmail.com>
Hi,
On Thu, 22 Sep 2016, Stefan Beller wrote:
> 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.
Indeed. It is quite tedious to re-Cc: people. Anatoly, please do not force
me to put in that work in the future.
Also: I mentioned recently that I am not happy with the original design,
either, and that I came up with a new design that I intend to port to the
rebase--helper, once it is included in an official Git version:
https://public-inbox.org/git/alpine.DEB.2.20.1609111027330.129229@virtualbox/
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH 2/2 v2] ls-files: add pathspec matching for submodules
From: Brandon Williams @ 2016-09-23 20:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqbmze1kzt.fsf@gitster.mtv.corp.google.com>
On Fri, Sep 23, 2016 at 12:20 PM, Junio C Hamano <gitster@pobox.com> wrote:
> There is an interesting observation around this code. Note that it
> is just something to keep in mind, even though I think we are in no
> position to solve this within the scope of this series, or in fact I
> am not sure if there is anything to "fix".
>
> The expectation here is that the leading part of pathspec elements
> contain path components above and outside the current working tree,
> e.g. in a superproject with a submodule at "sub/", the end-user may
> have said from the top of the superproject
>
> A saving grace is that "s*b/file" in this case is what the end-user
> is giving us, not something we internally generated. So we can
> simply blame the end user, saying "what --recurse-submodules does is
> to (conceptually) flatten the indices of submodules into the index
> of the superproject and show the entries that match your pathspec.
> Because you gave us 's*b/file', which does match 's*b/oob/file',
> that is what you get."
>
> ;-)
Yeah I've been thinking a bit about that as well. To me, it is
incredibly silly to
have a wildcard character in a filename (its unfortunate that its
allowed). We can
easily do as you suggest and simply blame the user and if they do have wildcard
characters in their filenames they would just need to force the
pathspec code to
do checks literally (using the appropriate pathspec magic). This
would just limit their
ability to use actual wildcards in their pathspecs, ie they have to
pick wildcards in their
filenames or the ability to do wildmatching.
-Brandon
^ permalink raw reply
* Re: What's cooking in git.git (Sep 2016, #06; Wed, 21)
From: Johannes Schindelin @ 2016-09-23 20:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqk2e46da3.fsf@gitster.mtv.corp.google.com>
Hi,
On Wed, 21 Sep 2016, Junio C Hamano wrote:
> * rt/rebase-i-broken-insn-advise (2016-09-07) 1 commit
> - rebase -i: improve advice on bad instruction lines
>
> When "git rebase -i" is given a broken instruction, it told the
> user to fix it with "--edit-todo", but didn't say what the step
> after that was (i.e. "--continue").
>
> Will hold.
> Dscho's "rebase -i" hopefully will become available in 'pu', by
> which time an equivalent of this fix would be ported to C. This is
> queued merely as a reminder.
Porting the fix was surprisingly easy:
-- snipsnap --
[PATCH] fixup! rebase -i: check for missing commits in the rebase--helper
---
sequencer.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 8f27524..386d16e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2567,9 +2567,10 @@ int check_todo_list(void)
if (raise_error)
fprintf(stderr,
- _("You can fix this with 'git rebase --edit-todo'.\n"
- "Or you can abort the rebase with 'git rebase"
- " --abort'.\n"));
+ _("You can fix this with 'git rebase --edit-todo' "
+ "and then run 'git rebase --continue'.\n"
+ "Or you can abort the rebase with 'git rebase"
+ " --abort'.\n"));
return res;
}
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* Re: [PATCH v3 1/2] gitweb: remove unused function parameter
From: Junio C Hamano @ 2016-09-23 19:57 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Ian Kelling, git
In-Reply-To: <4a616b43-cd22-e2fe-6085-a65c48687560@gmail.com>
Jakub Narębski <jnareb@gmail.com> writes:
> I think it would be better to be more descriptive, and say:
>
> Subject: [PATCH v3 1/2] gitweb: remove unused parameter from guess_file_syntax()
> Acked-by: Jakub Narębski <jnareb@gmail.com>
Thanks.
^ permalink raw reply
* Re: [PATCH v3 1/2] gitweb: remove unused function parameter
From: Jakub Narębski @ 2016-09-23 19:44 UTC (permalink / raw)
To: Ian Kelling, git
In-Reply-To: <20160923090846.3086-1-ian@iankelling.org>
W dniu 23.09.2016 o 11:08, Ian Kelling napisał:
>
> Subject: [PATCH v3 1/2] gitweb: remove unused function parameter
I think it would be better to be more descriptive, and say:
Subject: [PATCH v3 1/2] gitweb: remove unused parameter from guess_file_syntax()
But that might be too long...
>
> Signed-off-by: Ian Kelling <ian@iankelling.org>
With, or without this change, it's nice.
Acked-by: Jakub Narębski <jnareb@gmail.com>
> ---
> gitweb/gitweb.perl | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 33d701d..6cb4280 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) = @_;
> return undef unless ($highlight && defined $file_name);
> my $basename = basename($file_name, '.in');
> return $highlight_basename{$basename}
> @@ -7062,7 +7062,7 @@ sub git_blob {
> $have_blame &&= ($mimetype =~ m!^text/!);
>
> my $highlight = gitweb_check_feature('highlight');
> - my $syntax = guess_file_syntax($highlight, $mimetype, $file_name);
> + my $syntax = guess_file_syntax($highlight, $file_name);
> $fd = run_highlighter($fd, $highlight, $syntax)
> if $syntax;
>
>
^ permalink raw reply
* Re: [PATCH v2] fetch-pack: do not reset in_vain on non-novel acks
From: Junio C Hamano @ 2016-09-23 19:40 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
In-Reply-To: <1474652495-26051-1-git-send-email-jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> I tried looking at creating a helper function to reduce both the size
> and the nesting level of the loop, but it seems to me that a helper
> function can't be extracted so easily because the logic is quite
> intertwined with the rest of the function. For example, the "if
> (args->stateless_rpc..." block uses 6 variables from the outer scope:
> args, ack, commit, result_sha1, req_buf, and state_len (and in_vain, but
> this can be the return value of the function). Expanding it wider would
> allow us to make some of those 6 local, but also introduce new ones from
> the outer scope.
Yup, I suspected that much when I wrote the message you are
responding to, but was sort-of hoping that you might come up with a
more clever way to restructure the code. It is OK to leave it
as-is, and let others try making it cleaner ;-).
Thanks.
>
> fetch-pack.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 85e77af..413937e 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -428,10 +428,17 @@ 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 an ack
> + * for this commit has not been
> + * seen.
> + */
> + in_vain = 0;
> + } else if (!args->stateless_rpc
> + || ack != ACK_common)
> + in_vain = 0;
> mark_common(commit, 0, 1);
> retval = 0;
> - in_vain = 0;
> got_continue = 1;
> if (ack == ACK_ready) {
> clear_prio_queue(&rev_list);
^ permalink raw reply
* Re: [PATCH] run-command: async_exit no longer needs to be public
From: Junio C Hamano @ 2016-09-23 19:26 UTC (permalink / raw)
To: Lars Schneider; +Cc: Ramsay Jones, Jeff King, GIT Mailing-list
In-Reply-To: <6DE6D03D-F496-4FAB-866A-BC6802312B43@gmail.com>
Lars Schneider <larsxschneider@gmail.com> writes:
>> I do not offhand know if the topic is otherwise ready as-is, or
>> needs further work. When you need to reroll, you'd also need to
>> fetch from the result of the above from me first and then start your
>> work from it, though, if we go that route.
>
> Sounds good to me!
OK, here is what I queued, then.
-- >8 --
From: Lars Schneider <larsxschneider@gmail.com>
Date: Tue, 20 Sep 2016 21:02:39 +0200
Subject: [PATCH] run-command: move check_pipe() from write_or_die to
run_command
Move check_pipe() to run_command and make it public. This is necessary
to call the function from pkt-line in a subsequent patch.
While at it, make async_exit() static to run_command.c as it is no
longer used from outside.
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
run-command.c | 17 +++++++++++++++--
run-command.h | 2 +-
write_or_die.c | 13 -------------
3 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/run-command.c b/run-command.c
index 5a4dbb6..3269362 100644
--- a/run-command.c
+++ b/run-command.c
@@ -634,7 +634,7 @@ int in_async(void)
return !pthread_equal(main_thread, pthread_self());
}
-void NORETURN async_exit(int code)
+static void NORETURN async_exit(int code)
{
pthread_exit((void *)(intptr_t)code);
}
@@ -684,13 +684,26 @@ int in_async(void)
return process_is_async;
}
-void NORETURN async_exit(int code)
+static void NORETURN async_exit(int code)
{
exit(code);
}
#endif
+void check_pipe(int err)
+{
+ if (err == EPIPE) {
+ if (in_async())
+ async_exit(141);
+
+ signal(SIGPIPE, SIG_DFL);
+ raise(SIGPIPE);
+ /* Should never happen, but just in case... */
+ exit(141);
+ }
+}
+
int start_async(struct async *async)
{
int need_in, need_out;
diff --git a/run-command.h b/run-command.h
index 5066649..cf29a31 100644
--- a/run-command.h
+++ b/run-command.h
@@ -139,7 +139,7 @@ struct async {
int start_async(struct async *async);
int finish_async(struct async *async);
int in_async(void);
-void NORETURN async_exit(int code);
+void check_pipe(int err);
/**
* This callback should initialize the child process and preload the
diff --git a/write_or_die.c b/write_or_die.c
index 0734432..eab8c8d 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -1,19 +1,6 @@
#include "cache.h"
#include "run-command.h"
-static void check_pipe(int err)
-{
- if (err == EPIPE) {
- if (in_async())
- async_exit(141);
-
- signal(SIGPIPE, SIG_DFL);
- raise(SIGPIPE);
- /* Should never happen, but just in case... */
- exit(141);
- }
-}
-
/*
* Some cases use stdio, but want to flush after the write
* to get error handling (and to get better interactive
--
2.10.0-530-g67247c9
^ permalink raw reply related
* Re: Limitiations of git rebase --preserve-merges --interactive
From: Johannes Sixt @ 2016-09-23 19:24 UTC (permalink / raw)
To: Stefan Haller; +Cc: Junio C Hamano, Anatoly Borodin, git
In-Reply-To: <1mu1iqo.1gihw0m1dcenmnM%lists@haller-berlin.de>
Am 23.09.2016 um 17:50 schrieb Stefan Haller:
> And I don't see any tests that do rebase -p -i and actually do something
> interesting with the -i part. So my original question still remains. :-)
-i -p came first. -p without -i was bolted on later.
-- Hannes
^ permalink raw reply
* Re: [PATCH 2/2 v2] ls-files: add pathspec matching for submodules
From: Junio C Hamano @ 2016-09-23 19:20 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <1474500486-101640-1-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> static void show_ce_entry(const char *tag, const struct cache_entry *ce)
> {
> + struct strbuf name = STRBUF_INIT;
> int len = max_prefix_len;
> + if (submodule_prefix)
> + strbuf_addstr(&name, submodule_prefix);
> + strbuf_addstr(&name, ce->name);
> ...
> + } else if (match_pathspec(&pathspec, name.buf, name.len,
> + len, ps_matched,
> + S_ISDIR(ce->ce_mode) ||
> + S_ISGITLINK(ce->ce_mode))) {
There is an interesting observation around this code. Note that it
is just something to keep in mind, even though I think we are in no
position to solve this within the scope of this series, or in fact I
am not sure if there is anything to "fix".
The expectation here is that the leading part of pathspec elements
contain path components above and outside the current working tree,
e.g. in a superproject with a submodule at "sub/", the end-user may
have said from the top of the superproject
git ls-files --recurse-submodules -- sub/file
and the recursing "ls-files" is spawned as
git -C sub ls-files -- sub/file
relaying the pathspec literally.
This does not correctly work if the path to the submodule has
wildcard in it. Imagine that the submodule were at "s*b/". The
recursing invocation would look like:
git -C "s*b" ls-files -- "s*b/file"
Further imagine that the index in the submodule at "s*b" has two
paths in it, i.e.
file
oob/file
The prefix is prepended to them, to turn them into
s*b/file
s*b/oob/file
and I suspect that the pathspec element "s*b/file" would match both
of them.
The pathspec machinery has a provision to prevent a similar gotcha
happening for the "prefix" we internally use. In a sample
repository created like so:
$ git init
$ mkdir -p 's*b/oob' sib
$ >sib/file
$ cd 's*b'
$ >file
$ >oob/file
$ git add .
$ git ls-files -- file
the "ls-files" in the last step gets 's*b/' as the "prefix", and the
pathspec is formed by concatenating "file" to it, but in a special
way. The part that come from the "prefix" is marked not to honor
any wildcard in it, so 's*b/' even though it has an asterisk, it is
forced to match literally, giving only 's*b/file'.
A saving grace is that "s*b/file" in this case is what the end-user
is giving us, not something we internally generated. So we can
simply blame the end user, saying "what --recurse-submodules does is
to (conceptually) flatten the indices of submodules into the index
of the superproject and show the entries that match your pathspec.
Because you gave us 's*b/file', which does match 's*b/oob/file',
that is what you get."
;-)
^ permalink raw reply
* Re: [PATCH] run-command: async_exit no longer needs to be public
From: Lars Schneider @ 2016-09-23 18:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ramsay Jones, Jeff King, GIT Mailing-list
In-Reply-To: <xmqqmviy1qux.fsf@gitster.mtv.corp.google.com>
> On 23 Sep 2016, at 19:13, Junio C Hamano <gitster@pobox.com> wrote:
>
> Lars Schneider <larsxschneider@gmail.com> writes:
>
>>> If you need to re-roll your 'ls/filter-process' branch, could you please
>>> squash this into the relevant commit c42a4cbc ("run-command: move check_pipe()
>>> from write_or_die to run_command", 20-09-2016).
>>>
>>> [Note that commit 9658846c ("write_or_die: handle EPIPE in async threads",
>>> 24-02-2016) introduced async_exit() specifically for use in the implementation
>>> of check_pipe(). Now that you have moved check_pipe() into run-command.c,
>>> it no longer needs to be public.]
>>
>> Hi Ramsay,
>>
>> thanks for noticing this. I actually hope that I don't need another re-roll :-)
>> If I don't re-roll. Should I make a patch with this cleanup or do you
>> take care of it?
>
> I can just squash the the patch you are responding to into c42a4cbc,
> with an additional paragraph "While at it, retire async_exit() as a
> public function as it no longer is called outside run-command API
> implementation", or something like that.
>
> I do not offhand know if the topic is otherwise ready as-is, or
> needs further work. When you need to reroll, you'd also need to
> fetch from the result of the above from me first and then start your
> work from it, though, if we go that route.
Sounds good to me!
Thank you, Junio!
^ permalink raw reply
* Re: [PATCH 2/2 v2] ls-files: add pathspec matching for submodules
From: Junio C Hamano @ 2016-09-23 18:48 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <1474500486-101640-1-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> - /* Find common prefix for all pathspec's */
> - max_prefix = common_prefix(&pathspec);
> + /*
> + * Find common prefix for all pathspec's
> + * This is used as a performance optimization which unfortunately cannot
> + * be done when recursing into submodules
> + */
> + if (recurse_submodules)
> + max_prefix = NULL;
> + else
> + max_prefix = common_prefix(&pathspec);
> max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
This is OK for now, but for a future enhancement, I think we could
do better than this. In a superproject with a submodule at "sub/",
the current implementation of the common_prefix() helper would yield
"sub/a/" when given "sub/a/x" and "sub/a/y" (a pathspec with two
elements), which we want to avoid.
But somebody should be able to notice, before "sub/a/" is given to
max_prefix here, that "sub/" is the leaf level in our repository and
reduce the max_prefix to it. dir.c::common_prefix_len() might be
a place we could do so, but I didn't think about the ramifications
of doing so for other callers of common_prefix() or when we are not
recursing into submodules. Doing it in the caller here, i.e.
max_prefix = common_prefix(&pathspec);
if (recurse_submodules)
max_prefix = chomp_at_submodule_boundary(max_prefix);
is certainly safer.
If the superproject has submodules at "a/b/{sub1,sub2,...}", this
matters more. We do want to notice that we won't have to scan
outside "a/b/" of the index given "a/b/sub1" and "a/b/sub2" as a
pathspec.
The common_prefix_len() function also looks beyond symbolic links,
which is another thing that we may want to think about. In a
repository with a symbolic link "link" pointing somewhere else, when
you give "link/a/x" and "link/a/y" (a pathspec with two elements),
we would get "link/a/" as a common prefix, but we won't find
anything underneath "link" in our index. In such a case, leaving
the common prefix to "link/a/" _might_ allow us to notice that no
pathspec elements can ever match, so not noticing that the common
prefix points beyond a symbolic link might be a feature. I dunno.
^ permalink raw reply
* [PATCH v2] fetch-pack: do not reset in_vain on non-novel acks
From: Jonathan Tan @ 2016-09-23 17:41 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster
In-Reply-To: <xmqqfuor4s4z.fsf@gitster.mtv.corp.google.com>
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. This is to stop the client from digging too deep
in an irrelevant side branch in vain without ever finding a common
ancestor. A use case (as described in that commit) is the scenario in
which the local repository has more roots than the remote repository.
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.
Make fetch-pack only consider, as new acks for the purpose of
MAX_IN_VAIN, acks for objects for which the client has never received an
ack before in this session.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Thanks for your comments - I really appreciate them.
Update from original:
o removed redundant text from commit message and comment in patch
o mentioned stopping the client from digging too deep in the commit
message
I tried looking at creating a helper function to reduce both the size
and the nesting level of the loop, but it seems to me that a helper
function can't be extracted so easily because the logic is quite
intertwined with the rest of the function. For example, the "if
(args->stateless_rpc..." block uses 6 variables from the outer scope:
args, ack, commit, result_sha1, req_buf, and state_len (and in_vain, but
this can be the return value of the function). Expanding it wider would
allow us to make some of those 6 local, but also introduce new ones from
the outer scope.
fetch-pack.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 85e77af..413937e 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -428,10 +428,17 @@ 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 an ack
+ * for this commit has not been
+ * seen.
+ */
+ in_vain = 0;
+ } else if (!args->stateless_rpc
+ || ack != ACK_common)
+ in_vain = 0;
mark_common(commit, 0, 1);
retval = 0;
- in_vain = 0;
got_continue = 1;
if (ack == ACK_ready) {
clear_prio_queue(&rev_list);
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: [PATCH] run-command: async_exit no longer needs to be public
From: Junio C Hamano @ 2016-09-23 17:13 UTC (permalink / raw)
To: Lars Schneider; +Cc: Ramsay Jones, Jeff King, GIT Mailing-list
In-Reply-To: <58164A5E-AC93-48A9-9139-B69CFB854CA8@gmail.com>
Lars Schneider <larsxschneider@gmail.com> writes:
>> If you need to re-roll your 'ls/filter-process' branch, could you please
>> squash this into the relevant commit c42a4cbc ("run-command: move check_pipe()
>> from write_or_die to run_command", 20-09-2016).
>>
>> [Note that commit 9658846c ("write_or_die: handle EPIPE in async threads",
>> 24-02-2016) introduced async_exit() specifically for use in the implementation
>> of check_pipe(). Now that you have moved check_pipe() into run-command.c,
>> it no longer needs to be public.]
>
> Hi Ramsay,
>
> thanks for noticing this. I actually hope that I don't need another re-roll :-)
> If I don't re-roll. Should I make a patch with this cleanup or do you
> take care of it?
I can just squash the the patch you are responding to into c42a4cbc,
with an additional paragraph "While at it, retire async_exit() as a
public function as it no longer is called outside run-command API
implementation", or something like that.
I do not offhand know if the topic is otherwise ready as-is, or
needs further work. When you need to reroll, you'd also need to
fetch from the result of the above from me first and then start your
work from it, though, if we go that route.
^ permalink raw reply
* Re: [RFC PATCH] revision: new rev%n shorthand for rev^n..rev
From: Junio C Hamano @ 2016-09-23 16:43 UTC (permalink / raw)
To: Vegard Nossum; +Cc: git, Santi Béjar, Kevin Bracey, Philip Oakley
In-Reply-To: <20160923105254.10235-1-vegard.nossum@oracle.com>
Vegard Nossum <vegard.nossum@oracle.com> writes:
> I use rev^..rev daily, and I'm surely not the only one. To save typing
> (or copy-pasting, if the rev is long -- like a full SHA-1 or branch name)
> we can make rev% a shorthand for that.
No, we cannot.
'%' is not reserved as a special character that is forbidden in
reference names, and for somebody who has a branch whose name is
'master%', such a change will suddenly make 'master%' mean something
completely different, breaking existing users' repositories.
This is why existing rev^@ and rev^! both use the "^" as the first
character that introduces the "magic" semantics; "^" cannot be a
part of a refname. Also sequences that begin with "^{" and "@{" are
reserved as escape hatches to allow us extend the revision syntax in
the future ("^{" works on history, while "@{" bases its working on
the reflog data).
As "rev^$n" is "nth parent", it may be a possibility to use "rev^-$n"
as a short-hand for "^rev^$n rev".
^ permalink raw reply
* Re: [PATCH 1/2] ls-files: adding support for submodules
From: Stefan Beller @ 2016-09-23 16:34 UTC (permalink / raw)
To: Brandon Williams; +Cc: Jeff King, Junio C Hamano, git@vger.kernel.org
In-Reply-To: <CAKoko1p+M3X_y+DH+65fUDRZjkiZrKE3Z-jjq=RSjqoi7He8pw@mail.gmail.com>
On Fri, Sep 23, 2016 at 9:16 AM, Brandon Williams <bmwill@google.com> wrote:
>> Yeah, a positive "I support this" flag would at least let us correctly
>> flag errors, which is the best we can do. That won't work for
>> non-builtins, but perhaps it is good enough in practice.
>>
>> -Peff
>
>
> So it sounds like we agree that this prefix option should be pushed to
> the top level.
> The question is have we come to a consensus on what we should be
> calling the option?
The option itself is very similar to -C, which changes the directory to the
given argument before executing the git command.
e.g. in git:
git -C builtin ls-files
add.c
...
So for the submodule case we'd want that plus keeping around that prefix,
which makes me wonder if we could just store the argument of -C into a global
and use that when --keep-prefix is given, so you'd do a
git -C path/to/sub --keep-prefix ls-files
path/to/sub/file1
...
maybe --[keep|use]-[path|prefix] ?
You could of course go with a fully independent option, but how
would that work together with -C ?
(first change the dir and then change again while remembering the prefix?
or the other way round?)
> Leave it as submodule-prefix or do we need to come up with a different name?
>
> -Brandon
^ permalink raw reply
* Re: [PATCH 1/2] ls-files: adding support for submodules
From: Brandon Williams @ 2016-09-23 16:16 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Beller, Junio C Hamano, git@vger.kernel.org
In-Reply-To: <20160923060643.3ubr5gn7qczzs2ut@sigill.intra.peff.net>
> Yeah, a positive "I support this" flag would at least let us correctly
> flag errors, which is the best we can do. That won't work for
> non-builtins, but perhaps it is good enough in practice.
>
> -Peff
So it sounds like we agree that this prefix option should be pushed to
the top level.
The question is have we come to a consensus on what we should be
calling the option?
Leave it as submodule-prefix or do we need to come up with a different name?
-Brandon
^ permalink raw reply
* Re: Limitiations of git rebase --preserve-merges --interactive
From: Stefan Haller @ 2016-09-23 15:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Anatoly Borodin, git
In-Reply-To: <xmqqvaxm1vc3.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> wrote:
> lists@haller-berlin.de (Stefan Haller) writes:
>
> > Thanks, this is interesting; I'm having trouble understanding the tests
> > though. Some of them use rebase -p -i, but I don't understand why they
> > use -i, or why that even works in a test (i.e. why it doesn't open an
> > editor).
>
> Upon starting up, tests dot-source t/test-lib.sh file and it
> unsets most of GIT_* environment variables to obtain a stable
> testing environment that is not affected by things that testers
> may have in their environment.
>
> There is EDITOR=: in t/test-lib.sh, which was added in 2006 before
> GIT_EDITOR was invented. That is the one in effect for git
> subcommands that usually interacts with editors during the test,
> unless specific tests further override it with test_set_editor
> helper.
Thanks for the explanation. So this explains why -i works at all here;
it doesn't explain why -i is used in these tests. Unless I'm missing
something, they should all work with just -p.
And I don't see any tests that do rebase -p -i and actually do something
interesting with the -i part. So my original question still remains. :-)
--
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/
^ permalink raw reply
* Re: Limitiations of git rebase --preserve-merges --interactive
From: Junio C Hamano @ 2016-09-23 15:37 UTC (permalink / raw)
To: Stefan Haller; +Cc: Anatoly Borodin, git
In-Reply-To: <1mu13z0.1hye7bq1uqj26sM%lists@haller-berlin.de>
lists@haller-berlin.de (Stefan Haller) writes:
> Thanks, this is interesting; I'm having trouble understanding the tests
> though. Some of them use rebase -p -i, but I don't understand why they
> use -i, or why that even works in a test (i.e. why it doesn't open an
> editor).
Upon starting up, tests dot-source t/test-lib.sh file and it
unsets most of GIT_* environment variables to obtain a stable
testing environment that is not affected by things that testers
may have in their environment.
There is EDITOR=: in t/test-lib.sh, which was added in 2006 before
GIT_EDITOR was invented. That is the one in effect for git
subcommands that usually interacts with editors during the test,
unless specific tests further override it with test_set_editor
helper.
^ permalink raw reply
* Re: [PATCH v2 4/3] init: combine set_git_dir_init() and init_db() into one
From: Junio C Hamano @ 2016-09-23 15:18 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, git, max.nordlund
In-Reply-To: <20160923111206.8596-1-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> I think a separate commit for this is better than combining back to
> 2/3 so we can explain the problem properly (without making 2/3 commit
> message even longer)
>
> Not sure if you want to s/contains/contain/ in 2/3 by yourself or I
> should resend the whole series. Let me know.
OK, I just amended it before applying this on top.
> + flags |= INIT_DB_EXIST_OK;
> + return init_db(git_dir, real_git_dir, template_dir, flags);
I do not think of anything better, but EXIST_OK does not sound
grammatical. "REINIT" is not quite it--we are merely allowing
the function to re-init if there already is a repository. And
OK_TO_REINIT is a bit too long. Let's take the patch as-is for
now.
Thanks.
^ permalink raw reply
* Re: [PATCH 4/6] tag: add format specifier to gpg_verify_tag
From: Santiago Torres @ 2016-09-23 14:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, peff, sunshine, walters, Lukas P, Lukas Puehringer
In-Reply-To: <xmqqponv3b4x.fsf@gitster.mtv.corp.google.com>
[-- Attachment #1: Type: text/plain, Size: 2441 bytes --]
On Thu, Sep 22, 2016 at 01:58:06PM -0700, Junio C Hamano wrote:
> 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.
Oh, this sounds like a better alternative. This would be instead of 0003
right?
Thanks,
-Santiago.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH 5/6] builtin/verify-tag: Add --format to verify-tag
From: Santiago Torres @ 2016-09-23 14:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, peff, sunshine, walters
In-Reply-To: <xmqqintn3aai.fsf@gitster.mtv.corp.google.com>
[-- Attachment #1: Type: text/plain, Size: 2366 bytes --]
On Thu, Sep 22, 2016 at 02:16:21PM -0700, Junio C Hamano wrote:
> 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().
>
Yeah, in retrospect, this sounds like a more reasonable approach than
doing it on gpg-nterface. I'll keep the QUIET bit then.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ 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