* Re: [PATCH 1/2] Feature Request: user defined suffix for temp files created by git-mergetool
From: David Aguilar @ 2016-10-13 5:13 UTC (permalink / raw)
To: Josef Ridky; +Cc: git
In-Reply-To: <1911899288.2172724.1475757782111.JavaMail.zimbra@redhat.com>
On Thu, Oct 06, 2016 at 08:43:02AM -0400, Josef Ridky wrote:
> This is the first of two variant for request to add option to change
> suffix of name of temporary files generated by git mergetool. This
> change is requested for cases, when is git mergetool used for local
> comparision between two version of same package during package rebase.
>
> Signed-off-by: Josef Ridky <jridky@redhat.com>
> ---
> Documentation/git-mergetool.txt | 7 ++++++-
> git-mergetool.sh | 23 ++++++++++++++++++-----
> 2 files changed, 24 insertions(+), 6 deletions(-)
While I do like that this variant only uses a single flag, I was
curious whether it would make sense for us to change our
defaults to OLD/NEW? I'm thinking "no" since it's totally legit
to merge "old" branches so I'll stop there.
What I really wanted to mention was...
If the patch does not update t/t7610-mergetool.sh then there is
no guarantee that my clumsy fingers won't break the new feature
in the future ;-) So please make sure to update the tests once
we decide on the final direction. It makes sense we wouldn't
want to update them just yet (in this patch) since this is still
RFC, but the final one should include it.
I'm still leaning towards environment variables personally,
and would have no qualms against taking a patch that teaches it
to support environment variables as long as it adds a test case
for the new feature.
Thanks for sticking with it, Josef!
cheers,
--
David
^ permalink raw reply
* Re: [PATCH v2 2/2] Feature Request: user defined suffix for temp files created by git-mergetool
From: David Aguilar @ 2016-10-13 4:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Josef Ridky
In-Reply-To: <xmqqpon54fe5.fsf@gitster.mtv.corp.google.com>
On Wed, Oct 12, 2016 at 10:59:46AM -0700, Junio C Hamano wrote:
> Josef Ridky <jridky@redhat.com> writes:
>
> > This is update of the second variant for request to add option to change
> > suffix of name of temporary files generated by git mergetool. This
> > change is requested for cases, when is git mergetool used for local
> > comparison between two version of same package during package rebase.
> >
> > Signed-off-by: Josef Ridky <jridky@redhat.com>
> > ---
>
> David, what do you think?
>
> I don't think you were ever CC'ed on any of the messages in
> this thread, and I don't think you've commented on the topic. The
> thread begins here:
>
> https://public-inbox.org/git/1329039097.128066.1475476591437.JavaMail.zimbra@redhat.com/
>
>
> In any case, I suggest update to the log message to something like
> this:
>
> Subject: mergetool: allow custom naming for temporary files
>
> A front-end program that is spawned by "git mergetool" is given
> three temporary files (e.g. it may get "x_LOCAL.txt",
> "x_REMOTE.txt", and "x_BASE.txt" while merging "x.txt").
>
> Custom wrappers to "git mergetool" benefits if they are allowed
> to rename these hardcoded suffixes to match the workflow they
> implement. For example, they may be used to compare and merge
> two versions that is available locally, and OLD/NEW may be more
> appropriate than LOCAL/REMOTE in such a context.
>
> primarily because "the second variant" is meaningless thing to say
> in our long term history, when the first variant never was recorded
> there. Josef may want to elaborate more on the latter paragraph.
I do see why this can be helpful but what makes me reluctant is,
> > +'git mergetool' [--tool=<tool>] [-y | --[no-]prompt] [--local=<name>] [--remote=<name>] [--backup=<name>] [--base=<name>] [<file>...]
We're parking on 4 new flags that are really all about changing
one small aspect of the program.
I don't typically like going overboard with environment
variables but for this use case they seem to be a good fit.
It's already been expressed that the intention is for these to
be supplied by some other tool, and environment variables have
the nice property that you can update all your tools without
needing to touch anything except the environment.
Another nice thing is that the the user can choose to set it
globally, or to set it local to specific tools.
Another reason why I think it's a good fit is,
> +# Can be changed by user
> +LOCAL_NAME='LOCAL'
> +BASE_NAME='BASE'
> +BACKUP_NAME='BACKUP'
> +REMOTE_NAME='REMOTE'
These lines would become simply,
LOCAL_NAME=${GIT_MERGETOOL_LOCAL_NAME:-LOCAL}
BASE_NAME=${GIT_MERGETOOL_BASE_NAME:-BASE}
BACKUP_NAME=${GIT_MERGETOOL_BACKUP_NAME:-BACKUP}
REMOTE_NAME=${GIT_MERGETOOL_REMOTE_NAME:-REMOTE}
and we wouldn't need any other change besides this part:
> - BACKUP="$MERGETOOL_TMPDIR/${BASE}_BACKUP_$$$ext"
> - LOCAL="$MERGETOOL_TMPDIR/${BASE}_LOCAL_$$$ext"
> - REMOTE="$MERGETOOL_TMPDIR/${BASE}_REMOTE_$$$ext"
> - BASE="$MERGETOOL_TMPDIR/${BASE}_BASE_$$$ext"
> + BACKUP="$MERGETOOL_TMPDIR/${BASE}_${BACKUP_NAME}_$$$ext"
> + LOCAL="$MERGETOOL_TMPDIR/${BASE}_${LOCAL_NAME}_$$$ext"
> + REMOTE="$MERGETOOL_TMPDIR/${BASE}_${REMOTE_NAME}_$$$ext"
> + BASE="$MERGETOOL_TMPDIR/${BASE}_${BASE_NAME}_$$$ext"
Then, just update the documentation to mention the environment
variables instead of the flags and we're all set.
We also won't need this part if we go down that route:
> +# sanity check after parsing command line
> +case "" in
> +"$LOCAL_NAME"|"$REMOTE_NAME"|"$BASE_NAME"|"$BACKUP_NAME")
> + die "You cannot set any of --local/remote/base/backup to empty."
> + ;;
> +esac
> +
> +case "$LOCAL_NAME" in
> +"$REMOTE_NAME"|"$BASE_NAME"|"$BACKUP_NAME")
> + die "You cannot set any of --remote/base/backup to same as --local."
> + ;;
> +esac
> +
> +case "$REMOTE_NAME" in
> +"$LOCAL_NAME"|"$BASE_NAME"|"$BACKUP_NAME")
> + die "You cannot set any of --local/base/backup to same as --remote."
> + ;;
> +esac
> +
> +case "$BASE_NAME" in
> +"$LOCAL_NAME"|"$REMOTE_NAME"|"$BACKUP_NAME")
> + die "You cannot set any of --local/remote/backup to same as --base."
> + ;;
> +esac
> +
> +case "$BACKUP_NAME" in
> +"$LOCAL_NAME"|"$REMOTE_NAME"|"$BASE_NAME")
> + die "You cannot set any of --local/remote/base to same as --backup."
> + ;;
> +esac
What do you think?
--
David
^ permalink raw reply
* Re: problem with git worktree and git svn
From: Eric Wong @ 2016-10-13 1:52 UTC (permalink / raw)
To: Mathieu Arnold; +Cc: Duy Nguyen, Stefan Beller, git
In-Reply-To: <CAGZ79kZo5W1r0s26G3foB7caP6+u66mdzqzyneqXBX_B7A0RKg@mail.gmail.com>
Stefan Beller <sbeller@google.com> wrote:
> On Wed, Oct 12, 2016 at 7:45 AM, Mathieu Arnold <mat@freebsd.org> wrote:
>
> > I discovered git worktree earlier this week, and I found it a great
> > asset to be able to have more than one branch of my worktree accessible
> > at the same time...
> >
> > Anyway, back to my problem, the way git-svn works, is that it looks for
> > a directory named "svn" in its gitdir and if it is not present, decide
> > the repository is using git-svn version 1 (whatever that is) and goes to
> > parse all the revisions to recreate the svn directory.
> > So I can only use git svn commands in my main worktree, the one with the
> > real gitdir.
Right, I haven't updated git-svn to be worktree-aware, yet, but
a work-in-progress is below
> > To fix that, all I had to do is to add a symlink named svn in each
> > worktree's gitdir and pointing to ../../svn.
>
> For some definition of fix. ;)
> Sure it fixes your local setup now, but would we want to use that as well here?
> My gut reaction:
>
> * not all platforms know symlinks
> * IIRC there is some worktree magic that tells you the "main" dir,
> so if that was used in git-svn instead it should "just work".
>
> >
> > I think all that needs to happen is that when adding a new worktree, if
> > the main git directory has a "svn" directory, add a symlink to it in the
> > worktree's gitdir.
I'm fairly sure the worktree C code should not care about how
git-svn works, but rather git-svn should be made aware of
worktree bits...
I haven't studied worktrees much nor do I use git-svn much,
but the following seems to work for fetch/rebase/dcommit:
------------8<-----------
Subject: [PATCH] git-svn: WIP worktree awareness
---
perl/Git/SVN.pm | 29 ++++++++++++++++++++---------
perl/Git/SVN/Migration.pm | 23 ++++++++++++-----------
2 files changed, 32 insertions(+), 20 deletions(-)
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 018beb8..267cc09 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -807,10 +807,20 @@ sub get_fetch_range {
(++$min, $max);
}
+sub svn_dir {
+ my $git_dir = scalar @_ ? $_[0] : $ENV{GIT_DIR};
+ my $common = $ENV{GIT_COMMON_DIR} || "$git_dir/commondir";
+ $git_dir .= '/'.::file_to_s($common) if -e $common;
+ my $svn_dir = $git_dir . '/svn';
+ $svn_dir =~ tr!/!/!s;
+ $svn_dir;
+}
+
sub tmp_config {
my (@args) = @_;
- my $old_def_config = "$ENV{GIT_DIR}/svn/config";
- my $config = "$ENV{GIT_DIR}/svn/.metadata";
+ my $svn_dir = svn_dir();
+ my $old_def_config = "$svn_dir/config";
+ my $config = "$svn_dir/.metadata";
if (! -f $config && -f $old_def_config) {
rename $old_def_config, $config or
die "Failed rename $old_def_config => $config: $!\n";
@@ -1671,7 +1681,7 @@ sub tie_for_persistent_memoization {
return if $memoized;
$memoized = 1;
- my $cache_path = "$ENV{GIT_DIR}/svn/.caches/";
+ my $cache_path = svn_dir() . '/.caches/';
mkpath([$cache_path]) unless -d $cache_path;
my %lookup_svn_merge_cache;
@@ -1712,7 +1722,7 @@ sub tie_for_persistent_memoization {
sub clear_memoized_mergeinfo_caches {
die "Only call this method in non-memoized context" if ($memoized);
- my $cache_path = "$ENV{GIT_DIR}/svn/.caches/";
+ my $cache_path = svn_dir() . '/.caches/';
return unless -d $cache_path;
for my $cache_file (("$cache_path/lookup_svn_merge",
@@ -2446,12 +2456,13 @@ sub _new {
"refs/remotes/$prefix$default_ref_id";
}
$_[1] = $repo_id;
- my $dir = "$ENV{GIT_DIR}/svn/$ref_id";
+ my $svn_dir = svn_dir();
+ my $dir = "$svn_dir/$ref_id";
- # Older repos imported by us used $GIT_DIR/svn/foo instead of
- # $GIT_DIR/svn/refs/remotes/foo when tracking refs/remotes/foo
+ # Older repos imported by us used $svn_dir/foo instead of
+ # $svn_dir/refs/remotes/foo when tracking refs/remotes/foo
if ($ref_id =~ m{^refs/remotes/(.+)}) {
- my $old_dir = "$ENV{GIT_DIR}/svn/$1";
+ my $old_dir = "$svn_dir/$1";
if (-d $old_dir && ! -d $dir) {
$dir = $old_dir;
}
@@ -2461,7 +2472,7 @@ sub _new {
mkpath([$dir]);
my $obj = bless {
ref_id => $ref_id, dir => $dir, index => "$dir/index",
- config => "$ENV{GIT_DIR}/svn/config",
+ config => "$svn_dir/config",
map_root => "$dir/.rev_map", repo_id => $repo_id }, $class;
# Ensure it gets canonicalized
diff --git a/perl/Git/SVN/Migration.pm b/perl/Git/SVN/Migration.pm
index cf6ffa7..887fd93 100644
--- a/perl/Git/SVN/Migration.pm
+++ b/perl/Git/SVN/Migration.pm
@@ -45,6 +45,7 @@ use Git qw(
command_output_pipe
command_close_pipe
);
+use Git::SVN;
sub migrate_from_v0 {
my $git_dir = $ENV{GIT_DIR};
@@ -82,7 +83,7 @@ sub migrate_from_v1 {
my $git_dir = $ENV{GIT_DIR};
my $migrated = 0;
return $migrated unless -d $git_dir;
- my $svn_dir = "$git_dir/svn";
+ my $svn_dir = Git::SVN::svn_dir($git_dir);
# just in case somebody used 'svn' as their $id at some point...
return $migrated if -d $svn_dir && ! -f "$svn_dir/info/url";
@@ -100,24 +101,23 @@ sub migrate_from_v1 {
next unless -f "$git_dir/$x/info/url";
my $u = eval { ::file_to_s("$git_dir/$x/info/url") };
next unless $u;
- my $dn = dirname("$git_dir/svn/$x");
+ my $dn = dirname("$svn_dir/$x");
mkpath([$dn]) unless -d $dn;
if ($x eq 'svn') { # they used 'svn' as GIT_SVN_ID:
- mkpath(["$git_dir/svn/svn"]);
+ mkpath(["$svn_dir/svn"]);
print STDERR " - $git_dir/$x/info => ",
- "$git_dir/svn/$x/info\n";
- rename "$git_dir/$x/info", "$git_dir/svn/$x/info" or
+ "$svn_dir/$x/info\n";
+ rename "$git_dir/$x/info", "$svn_dir/$x/info" or
croak "$!: $x";
# don't worry too much about these, they probably
# don't exist with repos this old (save for index,
# and we can easily regenerate that)
foreach my $f (qw/unhandled.log index .rev_db/) {
- rename "$git_dir/$x/$f", "$git_dir/svn/$x/$f";
+ rename "$git_dir/$x/$f", "$svn_dir/$x/$f";
}
} else {
- print STDERR " - $git_dir/$x => $git_dir/svn/$x\n";
- rename "$git_dir/$x", "$git_dir/svn/$x" or
- croak "$!: $x";
+ print STDERR " - $git_dir/$x => $svn_dir/$x\n";
+ rename "$git_dir/$x", "$svn_dir/$x" or croak "$!: $x";
}
$migrated++;
}
@@ -139,9 +139,10 @@ sub read_old_urls {
push @dir, $_;
}
}
+ my $svn_dir = Git::SVN::svn_dir();
foreach (@dir) {
my $x = $_;
- $x =~ s!^\Q$ENV{GIT_DIR}\E/svn/!!o;
+ $x =~ s!^\Q$svn_dir\E/!!o;
read_old_urls($l_map, $x, $_);
}
}
@@ -150,7 +151,7 @@ sub migrate_from_v2 {
my @cfg = command(qw/config -l/);
return if grep /^svn-remote\..+\.url=/, @cfg;
my %l_map;
- read_old_urls(\%l_map, '', "$ENV{GIT_DIR}/svn");
+ read_old_urls(\%l_map, '', Git::SVN::svn_dir());
my $migrated = 0;
require Git::SVN;
--
EW
^ permalink raw reply related
* Re: Huge performance bottleneck reading packs
From: Jeff King @ 2016-10-12 23:47 UTC (permalink / raw)
To: Vegard Nossum
Cc: git, Quentin Casasnovas, Shawn Pearce,
Nguyễn Thái Ngọc Duy, Junio C Hamano
In-Reply-To: <20161012231807.syockv2emrsjf55r@sigill.intra.peff.net>
On Wed, Oct 12, 2016 at 07:18:07PM -0400, Jeff King wrote:
> Also, is it possible to make the repository in question available? I
> might be able to reproduce based on your description, but it would save
> time if I could directly run gdb on your example.
I tried this by making a bunch of packs in linux.git (my standard "this
is pretty big" repo), like so:
for i in $(seq 1000); do
git rev-list --objects HEAD~$((i+1))..HEAD~$i |
git pack-objects --delta-base-offset .git/objects/pack/pack
done
and then doing a 25,000-object fetch from upstream (no significance to
the number; that's just how far behind upstream I happened to be).
However, I didn't notice any regression. In fact, it was much _slower_
than v1.9.0, because that older version didn't have threaded index-pack.
If you can't share the repo directly, can you tell us more about your
fetch? How many objects are in your repository? How many objects are
fetched? How many refs are there on the remote side?
-Peff
^ permalink raw reply
* [PATCH v2 6/6] trailer: support values folded to multiple lines
From: Jonathan Tan @ 2016-10-12 23:40 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, peff, christian.couder
In-Reply-To: <cover.1476314576.git.jonathantanmy@google.com>
Currently, interpret-trailers requires that a trailer be only on 1 line.
For example:
a: first line
second line
would be interpreted as one trailer line followed by one non-trailer line.
Make interpret-trailers support RFC 822-style folding, treating those
lines as one logical trailer.
---
Documentation/git-interpret-trailers.txt | 7 +-
t/t7513-interpret-trailers.sh | 139 +++++++++++++++++++++++++++++++
trailer.c | 22 +++--
3 files changed, 160 insertions(+), 8 deletions(-)
diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index c480da6..cfec636 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -57,11 +57,12 @@ minus signs start the patch part of the message.
When reading trailers, there can be whitespaces before and after the
token, the separator and the value. There can also be whitespaces
-inside the token and the value.
+inside the token and the value. The value may be split over multiple lines with
+each subsequent line starting with whitespace, like the "folding" in RFC 822.
Note that 'trailers' do not follow and are not intended to follow many
-rules for RFC 822 headers. For example they do not follow the line
-folding rules, the encoding rules and probably many other rules.
+rules for RFC 822 headers. For example they do not follow
+the encoding rules and probably many other rules.
OPTIONS
-------
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 7f5cd2a..31db699 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -157,6 +157,145 @@ test_expect_success 'with non-trailer lines only' '
test_cmp expected actual
'
+test_expect_success 'multiline field treated as atomic for placement' '
+ q_to_tab >patch <<-\EOF &&
+
+ another: trailer
+ name: value on
+ Qmultiple lines
+ another: trailer
+ EOF
+ q_to_tab >expected <<-\EOF &&
+
+ another: trailer
+ name: value on
+ Qmultiple lines
+ name: value
+ another: trailer
+ EOF
+ test_config trailer.name.where after &&
+ git interpret-trailers --trailer "name: value" patch >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'multiline field treated as atomic for replacement' '
+ q_to_tab >patch <<-\EOF &&
+
+ another: trailer
+ name: value on
+ Qmultiple lines
+ another: trailer
+ EOF
+ q_to_tab >expected <<-\EOF &&
+
+ another: trailer
+ another: trailer
+ name: value
+ EOF
+ test_config trailer.name.ifexists replace &&
+ git interpret-trailers --trailer "name: value" patch >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'multiline field treated as atomic for difference check' '
+ q_to_tab >patch <<-\EOF &&
+
+ another: trailer
+ name: first line
+ Qsecond line
+ another: trailer
+ EOF
+ test_config trailer.name.ifexists addIfDifferent &&
+
+ q_to_tab >trailer <<-\EOF &&
+ name: first line
+ Qsecond line
+ EOF
+ q_to_tab >expected <<-\EOF &&
+
+ another: trailer
+ name: first line
+ Qsecond line
+ another: trailer
+ EOF
+ git interpret-trailers --trailer "$(cat trailer)" patch >actual &&
+ test_cmp expected actual &&
+
+ q_to_tab >trailer <<-\EOF &&
+ name: first line
+ QQQQQsecond line
+ EOF
+ q_to_tab >expected <<-\EOF &&
+
+ another: trailer
+ name: first line
+ Qsecond line
+ another: trailer
+ name: first line
+ QQQQQsecond line
+ EOF
+ git interpret-trailers --trailer "$(cat trailer)" patch >actual &&
+ test_cmp expected actual &&
+
+ q_to_tab >trailer <<-\EOF &&
+ name: first line *DIFFERENT*
+ Qsecond line
+ EOF
+ q_to_tab >expected <<-\EOF &&
+
+ another: trailer
+ name: first line
+ Qsecond line
+ another: trailer
+ name: first line *DIFFERENT*
+ Qsecond line
+ EOF
+ git interpret-trailers --trailer "$(cat trailer)" patch >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'multiline field treated as atomic for neighbor check' '
+ q_to_tab >patch <<-\EOF &&
+
+ another: trailer
+ name: first line
+ Qsecond line
+ another: trailer
+ EOF
+ test_config trailer.name.where after &&
+ test_config trailer.name.ifexists addIfDifferentNeighbor &&
+
+ q_to_tab >trailer <<-\EOF &&
+ name: first line
+ Qsecond line
+ EOF
+ q_to_tab >expected <<-\EOF &&
+
+ another: trailer
+ name: first line
+ Qsecond line
+ another: trailer
+ EOF
+ git interpret-trailers --trailer "$(cat trailer)" patch >actual &&
+ test_cmp expected actual &&
+
+ q_to_tab >trailer <<-\EOF &&
+ name: first line
+ QQQQQsecond line
+ EOF
+ q_to_tab >expected <<-\EOF &&
+
+ another: trailer
+ name: first line
+ Qsecond line
+ name: first line
+ QQQQQsecond line
+ another: trailer
+ EOF
+ git interpret-trailers --trailer "$(cat trailer)" patch >actual &&
+ test_cmp expected actual
+'
+
test_expect_success 'with config setup' '
git config trailer.ack.key "Acked-by: " &&
cat >expected <<-\EOF &&
diff --git a/trailer.c b/trailer.c
index d6dfc7a..ef276e6 100644
--- a/trailer.c
+++ b/trailer.c
@@ -248,7 +248,7 @@ static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg
if (arg_tok->value && arg_tok->value[0]) {
arg = arg_tok->value;
} else {
- if (in_tok && in_tok->value)
+ if (in_tok)
arg = xstrdup(in_tok->value);
else
arg = xstrdup("");
@@ -622,12 +622,14 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val,
return 0;
}
-static void add_trailer_item(struct list_head *head, char *tok, char *val)
+static struct trailer_item *add_trailer_item(struct list_head *head, char *tok,
+ char *val)
{
struct trailer_item *new = xcalloc(sizeof(*new), 1);
new->token = tok;
new->value = val;
list_add_tail(&new->list, head);
+ return new;
}
static void add_arg_item(struct list_head *arg_head, char *tok, char *val,
@@ -781,6 +783,7 @@ static int process_input_file(FILE *outfile,
int patch_start, trailer_start, trailer_end, i;
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
+ struct trailer_item *last = NULL;
/* Get the line count */
while (lines[count])
@@ -798,16 +801,25 @@ static int process_input_file(FILE *outfile,
/* Parse trailer lines */
for (i = trailer_start; i < trailer_end; i++) {
+ if (last && isspace(lines[i]->buf[0])) {
+ struct strbuf sb = STRBUF_INIT;
+ strbuf_addf(&sb, "%s\n%s", last->value, lines[i]->buf);
+ strbuf_strip_suffix(&sb, "\n");
+ free(last->value);
+ last->value = strbuf_detach(&sb, NULL);
+ continue;
+ }
if (!parse_trailer(&tok, &val, NULL, lines[i]->buf, 0))
- add_trailer_item(head,
- strbuf_detach(&tok, NULL),
- strbuf_detach(&val, NULL));
+ last = add_trailer_item(head,
+ strbuf_detach(&tok, NULL),
+ strbuf_detach(&val, NULL));
else {
strbuf_addbuf(&val, lines[i]);
strbuf_strip_suffix(&val, "\n");
add_trailer_item(head,
NULL,
strbuf_detach(&val, NULL));
+ last = NULL;
}
}
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 5/6] trailer: allow non-trailers in trailer block
From: Jonathan Tan @ 2016-10-12 23:40 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, peff, christian.couder
In-Reply-To: <cover.1476314576.git.jonathantanmy@google.com>
Currently, interpret-trailers requires all lines of a trailer block to
be trailers (or comments) - if not it would not identify that block as a
trailer block, and thus create its own trailer block, inserting a blank
line. For example:
echo -e "\na: b\nnot trailer" |
git interpret-trailers --trailer "c: d"
would result in:
a: b
not trailer
c: d
Relax the definition of a trailer block to only require 1 trailer, so
that trailers can be directly added to such blocks, resulting in:
a: b
not trailer
c: d
This allows arbitrary lines to be included in trailer blocks, like those
in [1], and still allow interpret-trailers to be used.
This change also makes comments in the trailer block be treated as any
other non-trailer line, preserving them in the output of
interpret-trailers.
[1]
https://kernel.googlesource.com/pub/scm/linux/kernel/git/stable/linux-stable/+/e7d316a02f683864a12389f8808570e37fb90aa3
---
Documentation/git-interpret-trailers.txt | 3 +-
t/t7513-interpret-trailers.sh | 35 +++++++++++++++
trailer.c | 77 ++++++++++++++++++++++----------
3 files changed, 90 insertions(+), 25 deletions(-)
diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 93d1db6..c480da6 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -48,7 +48,8 @@ with only spaces at the end of the commit message part, one blank line
will be added before the new trailer.
Existing trailers are extracted from the input message by looking for
-a group of one or more lines that contain a colon (by default), where
+a group of one or more lines in which at least one line contains a
+colon (by default), where
the group is preceded by one or more empty (or whitespace-only) lines.
The group must either be at the end of the message or be the last
non-whitespace lines before a line that starts with '---'. Such three
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index aee785c..7f5cd2a 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -126,6 +126,37 @@ test_expect_success 'with multiline title in the message' '
test_cmp expected actual
'
+test_expect_success 'with non-trailer lines mixed with trailer lines' '
+ cat >patch <<-\EOF &&
+
+ this: is a trailer
+ this is not a trailer
+ EOF
+ cat >expected <<-\EOF &&
+
+ this: is a trailer
+ this is not a trailer
+ token: value
+ EOF
+ git interpret-trailers --trailer "token: value" patch >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'with non-trailer lines only' '
+ cat >patch <<-\EOF &&
+
+ this is not a trailer
+ EOF
+ cat >expected <<-\EOF &&
+
+ this is not a trailer
+
+ token: value
+ EOF
+ git interpret-trailers --trailer "token: value" patch >actual &&
+ test_cmp expected actual
+'
+
test_expect_success 'with config setup' '
git config trailer.ack.key "Acked-by: " &&
cat >expected <<-\EOF &&
@@ -257,6 +288,8 @@ test_expect_success 'with message that has comments' '
cat >>expected <<-\EOF &&
# comment
+ # other comment
+ # yet another comment
Reviewed-by: Johan
Cc: Peff
# last comment
@@ -286,6 +319,8 @@ test_expect_success 'with message that has an old style conflict block' '
cat >>expected <<-\EOF &&
# comment
+ # other comment
+ # yet another comment
Reviewed-by: Johan
Cc: Peff
# last comment
diff --git a/trailer.c b/trailer.c
index a9ed3f8..d6dfc7a 100644
--- a/trailer.c
+++ b/trailer.c
@@ -27,6 +27,10 @@ static struct conf_info default_conf_info;
struct trailer_item {
struct list_head list;
+ /*
+ * If this is not a trailer line, the line is stored in value
+ * (excluding the terminating newline) and token is NULL.
+ */
char *token;
char *value;
};
@@ -70,9 +74,14 @@ static size_t token_len_without_separator(const char *token, size_t len)
static int same_token(struct trailer_item *a, struct arg_item *b)
{
- size_t a_len = token_len_without_separator(a->token, strlen(a->token));
- size_t b_len = token_len_without_separator(b->token, strlen(b->token));
- size_t min_len = (a_len > b_len) ? b_len : a_len;
+ size_t a_len, b_len, min_len;
+
+ if (!a->token)
+ return 0;
+
+ a_len = token_len_without_separator(a->token, strlen(a->token));
+ b_len = token_len_without_separator(b->token, strlen(b->token));
+ min_len = (a_len > b_len) ? b_len : a_len;
return !strncasecmp(a->token, b->token, min_len);
}
@@ -130,7 +139,14 @@ static char last_non_space_char(const char *s)
static void print_tok_val(FILE *outfile, const char *tok, const char *val)
{
- char c = last_non_space_char(tok);
+ char c;
+
+ if (!tok) {
+ fprintf(outfile, "%s\n", val);
+ return;
+ }
+
+ c = last_non_space_char(tok);
if (!c)
return;
if (strchr(separators, c))
@@ -543,8 +559,16 @@ static int token_matches_item(const char *tok, struct arg_item *item, int tok_le
return item->conf.key ? !strncasecmp(tok, item->conf.key, tok_len) : 0;
}
+/*
+ * Parse the given trailer into token and value parts.
+ *
+ * If the given trailer does not have a separator (and thus cannot be separated
+ * into token and value parts), it is treated as a token (if parse_as_arg) or
+ * as a non-trailer line (if not parse_as_arg).
+ */
static int parse_trailer(struct strbuf *tok, struct strbuf *val,
- const struct conf_info **conf, const char *trailer)
+ const struct conf_info **conf, const char *trailer,
+ int parse_as_arg)
{
size_t len;
struct strbuf seps = STRBUF_INIT;
@@ -557,11 +581,18 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val,
len = strcspn(trailer, seps.buf);
strbuf_release(&seps);
if (len == 0) {
- int l = strlen(trailer);
+ int l;
+ if (!parse_as_arg)
+ return -1;
+
+ l = strlen(trailer);
while (l > 0 && isspace(trailer[l - 1]))
l--;
return error(_("empty trailer token in trailer '%.*s'"), l, trailer);
}
+ if (!parse_as_arg && len == strlen(trailer))
+ return -1;
+
if (len < strlen(trailer)) {
strbuf_add(tok, trailer, len);
strbuf_trim(tok);
@@ -631,7 +662,7 @@ static void process_command_line_args(struct list_head *arg_head,
/* Add an arg item for each trailer on the command line */
for_each_string_list_item(tr, trailers) {
- if (!parse_trailer(&tok, &val, &conf, tr->string))
+ if (!parse_trailer(&tok, &val, &conf, tr->string, 1))
add_arg_item(arg_head,
strbuf_detach(&tok, NULL),
strbuf_detach(&val, NULL),
@@ -683,7 +714,7 @@ static int find_patch_start(struct strbuf **lines, int count)
*/
static int find_trailer_start(struct strbuf **lines, int count)
{
- int start, end_of_title, only_spaces = 1;
+ int start, end_of_title, only_spaces = 1, trailer_found = 0;
/* The first paragraph is the title and cannot be trailers */
for (start = 0; start < count; start++) {
@@ -699,22 +730,17 @@ static int find_trailer_start(struct strbuf **lines, int count)
* for a line with only spaces before lines with one separator.
*/
for (start = count - 1; start >= end_of_title; start--) {
- if (lines[start]->buf[0] == comment_line_char)
- continue;
if (contains_only_spaces(lines[start]->buf)) {
if (only_spaces)
continue;
- return start + 1;
+ return trailer_found ? start + 1 : count;
}
- if (strcspn(lines[start]->buf, separators) < lines[start]->len) {
- if (only_spaces)
- only_spaces = 0;
- continue;
- }
- return count;
+ only_spaces = 0;
+ if (strcspn(lines[start]->buf, separators) < lines[start]->len)
+ trailer_found = 1;
}
- return only_spaces ? count : 0;
+ return count;
}
/* Get the index of the end of the trailers */
@@ -735,11 +761,8 @@ static int find_trailer_end(struct strbuf **lines, int patch_start)
static int has_blank_line_before(struct strbuf **lines, int start)
{
- for (;start >= 0; start--) {
- if (lines[start]->buf[0] == comment_line_char)
- continue;
+ if (start >= 0)
return contains_only_spaces(lines[start]->buf);
- }
return 0;
}
@@ -775,11 +798,17 @@ static int process_input_file(FILE *outfile,
/* Parse trailer lines */
for (i = trailer_start; i < trailer_end; i++) {
- if (lines[i]->buf[0] != comment_line_char &&
- !parse_trailer(&tok, &val, NULL, lines[i]->buf))
+ if (!parse_trailer(&tok, &val, NULL, lines[i]->buf, 0))
add_trailer_item(head,
strbuf_detach(&tok, NULL),
strbuf_detach(&val, NULL));
+ else {
+ strbuf_addbuf(&val, lines[i]);
+ strbuf_strip_suffix(&val, "\n");
+ add_trailer_item(head,
+ NULL,
+ strbuf_detach(&val, NULL));
+ }
}
return trailer_end;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 4/6] trailer: make args have their own struct
From: Jonathan Tan @ 2016-10-12 23:40 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, peff, christian.couder
In-Reply-To: <cover.1476314576.git.jonathantanmy@google.com>
Improve type safety by making arguments (whether from configuration or
from the command line) have their own "struct arg_item" type, separate
from the "struct trailer_item" type used to represent the trailers in
the buffer being manipulated.
This change also prepares "struct trailer_item" to be further
differentiated from "struct arg_item" in future patches.
---
trailer.c | 135 +++++++++++++++++++++++++++++++++++++++-----------------------
1 file changed, 85 insertions(+), 50 deletions(-)
diff --git a/trailer.c b/trailer.c
index 54cc930..a9ed3f8 100644
--- a/trailer.c
+++ b/trailer.c
@@ -29,6 +29,12 @@ struct trailer_item {
struct list_head list;
char *token;
char *value;
+};
+
+struct arg_item {
+ struct list_head list;
+ char *token;
+ char *value;
struct conf_info conf;
};
@@ -62,7 +68,7 @@ static size_t token_len_without_separator(const char *token, size_t len)
return len;
}
-static int same_token(struct trailer_item *a, struct trailer_item *b)
+static int same_token(struct trailer_item *a, struct arg_item *b)
{
size_t a_len = token_len_without_separator(a->token, strlen(a->token));
size_t b_len = token_len_without_separator(b->token, strlen(b->token));
@@ -71,12 +77,12 @@ static int same_token(struct trailer_item *a, struct trailer_item *b)
return !strncasecmp(a->token, b->token, min_len);
}
-static int same_value(struct trailer_item *a, struct trailer_item *b)
+static int same_value(struct trailer_item *a, struct arg_item *b)
{
return !strcasecmp(a->value, b->value);
}
-static int same_trailer(struct trailer_item *a, struct trailer_item *b)
+static int same_trailer(struct trailer_item *a, struct arg_item *b)
{
return same_token(a, b) && same_value(a, b);
}
@@ -98,6 +104,13 @@ static inline void strbuf_replace(struct strbuf *sb, const char *a, const char *
static void free_trailer_item(struct trailer_item *item)
{
+ free(item->token);
+ free(item->value);
+ free(item);
+}
+
+static void free_arg_item(struct arg_item *item)
+{
free(item->conf.name);
free(item->conf.key);
free(item->conf.command);
@@ -137,17 +150,29 @@ static void print_all(FILE *outfile, struct list_head *head, int trim_empty)
}
}
+static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
+{
+ struct trailer_item *new = xcalloc(sizeof(*new), 1);
+ new->token = arg_tok->token;
+ new->value = arg_tok->value;
+ arg_tok->token = arg_tok->value = NULL;
+ free_arg_item(arg_tok);
+ return new;
+}
+
static void add_arg_to_input_list(struct trailer_item *on_tok,
- struct trailer_item *arg_tok)
+ struct arg_item *arg_tok)
{
- if (after_or_end(arg_tok->conf.where))
- list_add(&arg_tok->list, &on_tok->list);
+ int aoe = after_or_end(arg_tok->conf.where);
+ struct trailer_item *to_add = trailer_from_arg(arg_tok);
+ if (aoe)
+ list_add(&to_add->list, &on_tok->list);
else
- list_add_tail(&arg_tok->list, &on_tok->list);
+ list_add_tail(&to_add->list, &on_tok->list);
}
static int check_if_different(struct trailer_item *in_tok,
- struct trailer_item *arg_tok,
+ struct arg_item *arg_tok,
int check_all,
struct list_head *head)
{
@@ -200,7 +225,7 @@ static char *apply_command(const char *command, const char *arg)
return result;
}
-static void apply_item_command(struct trailer_item *in_tok, struct trailer_item *arg_tok)
+static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg_tok)
{
if (arg_tok->conf.command) {
const char *arg;
@@ -218,13 +243,13 @@ static void apply_item_command(struct trailer_item *in_tok, struct trailer_item
}
static void apply_arg_if_exists(struct trailer_item *in_tok,
- struct trailer_item *arg_tok,
+ struct arg_item *arg_tok,
struct trailer_item *on_tok,
struct list_head *head)
{
switch (arg_tok->conf.if_exists) {
case EXISTS_DO_NOTHING:
- free_trailer_item(arg_tok);
+ free_arg_item(arg_tok);
break;
case EXISTS_REPLACE:
apply_item_command(in_tok, arg_tok);
@@ -241,39 +266,41 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,
if (check_if_different(in_tok, arg_tok, 1, head))
add_arg_to_input_list(on_tok, arg_tok);
else
- free_trailer_item(arg_tok);
+ free_arg_item(arg_tok);
break;
case EXISTS_ADD_IF_DIFFERENT_NEIGHBOR:
apply_item_command(in_tok, arg_tok);
if (check_if_different(on_tok, arg_tok, 0, head))
add_arg_to_input_list(on_tok, arg_tok);
else
- free_trailer_item(arg_tok);
+ free_arg_item(arg_tok);
break;
}
}
static void apply_arg_if_missing(struct list_head *head,
- struct trailer_item *arg_tok)
+ struct arg_item *arg_tok)
{
enum action_where where;
+ struct trailer_item *to_add;
switch (arg_tok->conf.if_missing) {
case MISSING_DO_NOTHING:
- free_trailer_item(arg_tok);
+ free_arg_item(arg_tok);
break;
case MISSING_ADD:
where = arg_tok->conf.where;
apply_item_command(NULL, arg_tok);
+ to_add = trailer_from_arg(arg_tok);
if (after_or_end(where))
- list_add_tail(&arg_tok->list, head);
+ list_add_tail(&to_add->list, head);
else
- list_add(&arg_tok->list, head);
+ list_add(&to_add->list, head);
}
}
static int find_same_and_apply_arg(struct list_head *head,
- struct trailer_item *arg_tok)
+ struct arg_item *arg_tok)
{
struct list_head *pos;
struct trailer_item *in_tok;
@@ -306,11 +333,11 @@ static void process_trailers_lists(struct list_head *head,
struct list_head *arg_head)
{
struct list_head *pos, *p;
- struct trailer_item *arg_tok;
+ struct arg_item *arg_tok;
list_for_each_safe(pos, p, arg_head) {
int applied = 0;
- arg_tok = list_entry(pos, struct trailer_item, list);
+ arg_tok = list_entry(pos, struct arg_item, list);
list_del(pos);
@@ -375,20 +402,20 @@ static void duplicate_conf(struct conf_info *dst, const struct conf_info *src)
dst->command = xstrdup(src->command);
}
-static struct trailer_item *get_conf_item(const char *name)
+static struct arg_item *get_conf_item(const char *name)
{
struct list_head *pos;
- struct trailer_item *item;
+ struct arg_item *item;
/* Look up item with same name */
list_for_each(pos, &conf_head) {
- item = list_entry(pos, struct trailer_item, list);
+ item = list_entry(pos, struct arg_item, list);
if (!strcasecmp(item->conf.name, name))
return item;
}
/* Item does not already exists, create it */
- item = xcalloc(sizeof(struct trailer_item), 1);
+ item = xcalloc(sizeof(*item), 1);
duplicate_conf(&item->conf, &default_conf_info);
item->conf.name = xstrdup(name);
@@ -442,7 +469,7 @@ static int git_trailer_default_config(const char *conf_key, const char *value, v
static int git_trailer_config(const char *conf_key, const char *value, void *cb)
{
const char *trailer_item, *variable_name;
- struct trailer_item *item;
+ struct arg_item *item;
struct conf_info *conf;
char *name = NULL;
enum trailer_info_type type;
@@ -500,7 +527,7 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)
return 0;
}
-static const char *token_from_item(struct trailer_item *item, char *tok)
+static const char *token_from_item(struct arg_item *item, char *tok)
{
if (item->conf.key)
return item->conf.key;
@@ -509,7 +536,7 @@ static const char *token_from_item(struct trailer_item *item, char *tok)
return item->conf.name;
}
-static int token_matches_item(const char *tok, struct trailer_item *item, int tok_len)
+static int token_matches_item(const char *tok, struct arg_item *item, int tok_len)
{
if (!strncasecmp(tok, item->conf.name, tok_len))
return 1;
@@ -521,7 +548,7 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val,
{
size_t len;
struct strbuf seps = STRBUF_INIT;
- struct trailer_item *item;
+ struct arg_item *item;
int tok_len;
struct list_head *pos;
@@ -547,12 +574,14 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val,
/* Lookup if the token matches something in the config */
tok_len = token_len_without_separator(tok->buf, tok->len);
- *conf = &default_conf_info;
+ if (conf)
+ *conf = &default_conf_info;
list_for_each(pos, &conf_head) {
- item = list_entry(pos, struct trailer_item, list);
+ item = list_entry(pos, struct arg_item, list);
if (token_matches_item(tok->buf, item, tok_len)) {
char *tok_buf = strbuf_detach(tok, NULL);
- *conf = &item->conf;
+ if (conf)
+ *conf = &item->conf;
strbuf_addstr(tok, token_from_item(item, tok_buf));
free(tok_buf);
break;
@@ -562,43 +591,51 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val,
return 0;
}
-static void add_trailer_item(struct list_head *head, char *tok, char *val,
- const struct conf_info *conf)
+static void add_trailer_item(struct list_head *head, char *tok, char *val)
{
struct trailer_item *new = xcalloc(sizeof(*new), 1);
new->token = tok;
new->value = val;
- duplicate_conf(&new->conf, conf);
list_add_tail(&new->list, head);
}
+static void add_arg_item(struct list_head *arg_head, char *tok, char *val,
+ const struct conf_info *conf)
+{
+ struct arg_item *new = xcalloc(sizeof(*new), 1);
+ new->token = tok;
+ new->value = val;
+ duplicate_conf(&new->conf, conf);
+ list_add_tail(&new->list, arg_head);
+}
+
static void process_command_line_args(struct list_head *arg_head,
struct string_list *trailers)
{
struct string_list_item *tr;
- struct trailer_item *item;
+ struct arg_item *item;
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
const struct conf_info *conf;
struct list_head *pos;
- /* Add a trailer item for each configured trailer with a command */
+ /* Add an arg item for each configured trailer with a command */
list_for_each(pos, &conf_head) {
- item = list_entry(pos, struct trailer_item, list);
+ item = list_entry(pos, struct arg_item, list);
if (item->conf.command)
- add_trailer_item(arg_head,
- xstrdup(token_from_item(item, NULL)),
- xstrdup(""),
- &item->conf);
+ add_arg_item(arg_head,
+ xstrdup(token_from_item(item, NULL)),
+ xstrdup(""),
+ &item->conf);
}
- /* Add a trailer item for each trailer on the command line */
+ /* Add an arg item for each trailer on the command line */
for_each_string_list_item(tr, trailers) {
if (!parse_trailer(&tok, &val, &conf, tr->string))
- add_trailer_item(arg_head,
- strbuf_detach(&tok, NULL),
- strbuf_detach(&val, NULL),
- conf);
+ add_arg_item(arg_head,
+ strbuf_detach(&tok, NULL),
+ strbuf_detach(&val, NULL),
+ conf);
}
}
@@ -721,7 +758,6 @@ static int process_input_file(FILE *outfile,
int patch_start, trailer_start, trailer_end, i;
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
- const struct conf_info *conf;
/* Get the line count */
while (lines[count])
@@ -740,11 +776,10 @@ static int process_input_file(FILE *outfile,
/* Parse trailer lines */
for (i = trailer_start; i < trailer_end; i++) {
if (lines[i]->buf[0] != comment_line_char &&
- !parse_trailer(&tok, &val, &conf, lines[i]->buf))
+ !parse_trailer(&tok, &val, NULL, lines[i]->buf))
add_trailer_item(head,
strbuf_detach(&tok, NULL),
- strbuf_detach(&val, NULL),
- conf);
+ strbuf_detach(&val, NULL));
}
return trailer_end;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 3/6] trailer: streamline trailer item create and add
From: Jonathan Tan @ 2016-10-12 23:40 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, peff, christian.couder
In-Reply-To: <cover.1476314576.git.jonathantanmy@google.com>
Currently, creation and addition (to a list) of trailer items are spread
across multiple functions. Streamline this by only having 2 functions:
one to parse the user-supplied string, and one to add the parsed
information to a list.
---
trailer.c | 130 +++++++++++++++++++++++++++++---------------------------------
1 file changed, 60 insertions(+), 70 deletions(-)
diff --git a/trailer.c b/trailer.c
index 0afa240..54cc930 100644
--- a/trailer.c
+++ b/trailer.c
@@ -500,10 +500,31 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)
return 0;
}
-static int parse_trailer(struct strbuf *tok, struct strbuf *val, const char *trailer)
+static const char *token_from_item(struct trailer_item *item, char *tok)
+{
+ if (item->conf.key)
+ return item->conf.key;
+ if (tok)
+ return tok;
+ return item->conf.name;
+}
+
+static int token_matches_item(const char *tok, struct trailer_item *item, int tok_len)
+{
+ if (!strncasecmp(tok, item->conf.name, tok_len))
+ return 1;
+ return item->conf.key ? !strncasecmp(tok, item->conf.key, tok_len) : 0;
+}
+
+static int parse_trailer(struct strbuf *tok, struct strbuf *val,
+ const struct conf_info **conf, const char *trailer)
{
size_t len;
struct strbuf seps = STRBUF_INIT;
+ struct trailer_item *item;
+ int tok_len;
+ struct list_head *pos;
+
strbuf_addstr(&seps, separators);
strbuf_addch(&seps, '=');
len = strcspn(trailer, seps.buf);
@@ -523,74 +544,31 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val, const char *tra
strbuf_addstr(tok, trailer);
strbuf_trim(tok);
}
- return 0;
-}
-
-static const char *token_from_item(struct trailer_item *item, char *tok)
-{
- if (item->conf.key)
- return item->conf.key;
- if (tok)
- return tok;
- return item->conf.name;
-}
-
-static struct trailer_item *new_trailer_item(struct trailer_item *conf_item,
- char *tok, char *val)
-{
- struct trailer_item *new = xcalloc(sizeof(*new), 1);
- new->value = val ? val : xstrdup("");
-
- if (conf_item) {
- duplicate_conf(&new->conf, &conf_item->conf);
- new->token = xstrdup(token_from_item(conf_item, tok));
- free(tok);
- } else {
- duplicate_conf(&new->conf, &default_conf_info);
- new->token = tok;
- }
-
- return new;
-}
-
-static int token_matches_item(const char *tok, struct trailer_item *item, int tok_len)
-{
- if (!strncasecmp(tok, item->conf.name, tok_len))
- return 1;
- return item->conf.key ? !strncasecmp(tok, item->conf.key, tok_len) : 0;
-}
-
-static struct trailer_item *create_trailer_item(const char *string)
-{
- struct strbuf tok = STRBUF_INIT;
- struct strbuf val = STRBUF_INIT;
- struct trailer_item *item;
- int tok_len;
- struct list_head *pos;
-
- if (parse_trailer(&tok, &val, string))
- return NULL;
-
- tok_len = token_len_without_separator(tok.buf, tok.len);
/* Lookup if the token matches something in the config */
+ tok_len = token_len_without_separator(tok->buf, tok->len);
+ *conf = &default_conf_info;
list_for_each(pos, &conf_head) {
item = list_entry(pos, struct trailer_item, list);
- if (token_matches_item(tok.buf, item, tok_len))
- return new_trailer_item(item,
- strbuf_detach(&tok, NULL),
- strbuf_detach(&val, NULL));
+ if (token_matches_item(tok->buf, item, tok_len)) {
+ char *tok_buf = strbuf_detach(tok, NULL);
+ *conf = &item->conf;
+ strbuf_addstr(tok, token_from_item(item, tok_buf));
+ free(tok_buf);
+ break;
+ }
}
- return new_trailer_item(NULL,
- strbuf_detach(&tok, NULL),
- strbuf_detach(&val, NULL));
+ return 0;
}
-static void add_trailer_item(struct list_head *head, struct trailer_item *new)
+static void add_trailer_item(struct list_head *head, char *tok, char *val,
+ const struct conf_info *conf)
{
- if (!new)
- return;
+ struct trailer_item *new = xcalloc(sizeof(*new), 1);
+ new->token = tok;
+ new->value = val;
+ duplicate_conf(&new->conf, conf);
list_add_tail(&new->list, head);
}
@@ -599,21 +577,28 @@ static void process_command_line_args(struct list_head *arg_head,
{
struct string_list_item *tr;
struct trailer_item *item;
+ struct strbuf tok = STRBUF_INIT;
+ struct strbuf val = STRBUF_INIT;
+ const struct conf_info *conf;
struct list_head *pos;
/* Add a trailer item for each configured trailer with a command */
list_for_each(pos, &conf_head) {
item = list_entry(pos, struct trailer_item, list);
- if (item->conf.command) {
- struct trailer_item *new = new_trailer_item(item, NULL, NULL);
- add_trailer_item(arg_head, new);
- }
+ if (item->conf.command)
+ add_trailer_item(arg_head,
+ xstrdup(token_from_item(item, NULL)),
+ xstrdup(""),
+ &item->conf);
}
/* Add a trailer item for each trailer on the command line */
for_each_string_list_item(tr, trailers) {
- struct trailer_item *new = create_trailer_item(tr->string);
- add_trailer_item(arg_head, new);
+ if (!parse_trailer(&tok, &val, &conf, tr->string))
+ add_trailer_item(arg_head,
+ strbuf_detach(&tok, NULL),
+ strbuf_detach(&val, NULL),
+ conf);
}
}
@@ -734,6 +719,9 @@ static int process_input_file(FILE *outfile,
{
int count = 0;
int patch_start, trailer_start, trailer_end, i;
+ struct strbuf tok = STRBUF_INIT;
+ struct strbuf val = STRBUF_INIT;
+ const struct conf_info *conf;
/* Get the line count */
while (lines[count])
@@ -751,10 +739,12 @@ static int process_input_file(FILE *outfile,
/* Parse trailer lines */
for (i = trailer_start; i < trailer_end; i++) {
- if (lines[i]->buf[0] != comment_line_char) {
- struct trailer_item *new = create_trailer_item(lines[i]->buf);
- add_trailer_item(head, new);
- }
+ if (lines[i]->buf[0] != comment_line_char &&
+ !parse_trailer(&tok, &val, &conf, lines[i]->buf))
+ add_trailer_item(head,
+ strbuf_detach(&tok, NULL),
+ strbuf_detach(&val, NULL),
+ conf);
}
return trailer_end;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 2/6] trailer: use list.h for doubly-linked list
From: Jonathan Tan @ 2016-10-12 23:40 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, peff, christian.couder
In-Reply-To: <cover.1476314576.git.jonathantanmy@google.com>
Replace the existing handwritten implementation of a doubly-linked list
in trailer.c with the functions and macros from list.h. This
significantly simplifies the code.
---
trailer.c | 258 ++++++++++++++++++++++----------------------------------------
1 file changed, 91 insertions(+), 167 deletions(-)
diff --git a/trailer.c b/trailer.c
index 1f191b2..0afa240 100644
--- a/trailer.c
+++ b/trailer.c
@@ -4,6 +4,7 @@
#include "commit.h"
#include "tempfile.h"
#include "trailer.h"
+#include "list.h"
/*
* Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>
*/
@@ -25,19 +26,24 @@ struct conf_info {
static struct conf_info default_conf_info;
struct trailer_item {
- struct trailer_item *previous;
- struct trailer_item *next;
+ struct list_head list;
char *token;
char *value;
struct conf_info conf;
};
-static struct trailer_item *first_conf_item;
+LIST_HEAD(conf_head);
static char *separators = ":";
#define TRAILER_ARG_STRING "$ARG"
+/* Iterate over the elements of the list. */
+#define list_for_each_dir(pos, head, is_reverse) \
+ for (pos = is_reverse ? (head)->prev : (head)->next; \
+ pos != (head); \
+ pos = is_reverse ? pos->prev : pos->next)
+
static int after_or_end(enum action_where where)
{
return (where == WHERE_AFTER) || (where == WHERE_END);
@@ -120,101 +126,49 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
}
-static void print_all(FILE *outfile, struct trailer_item *first, int trim_empty)
+static void print_all(FILE *outfile, struct list_head *head, int trim_empty)
{
+ struct list_head *pos;
struct trailer_item *item;
- for (item = first; item; item = item->next) {
+ list_for_each(pos, head) {
+ item = list_entry(pos, struct trailer_item, list);
if (!trim_empty || strlen(item->value) > 0)
print_tok_val(outfile, item->token, item->value);
}
}
-static void update_last(struct trailer_item **last)
-{
- if (*last)
- while ((*last)->next != NULL)
- *last = (*last)->next;
-}
-
-static void update_first(struct trailer_item **first)
-{
- if (*first)
- while ((*first)->previous != NULL)
- *first = (*first)->previous;
-}
-
static void add_arg_to_input_list(struct trailer_item *on_tok,
- struct trailer_item *arg_tok,
- struct trailer_item **first,
- struct trailer_item **last)
-{
- if (after_or_end(arg_tok->conf.where)) {
- arg_tok->next = on_tok->next;
- on_tok->next = arg_tok;
- arg_tok->previous = on_tok;
- if (arg_tok->next)
- arg_tok->next->previous = arg_tok;
- update_last(last);
- } else {
- arg_tok->previous = on_tok->previous;
- on_tok->previous = arg_tok;
- arg_tok->next = on_tok;
- if (arg_tok->previous)
- arg_tok->previous->next = arg_tok;
- update_first(first);
- }
+ struct trailer_item *arg_tok)
+{
+ if (after_or_end(arg_tok->conf.where))
+ list_add(&arg_tok->list, &on_tok->list);
+ else
+ list_add_tail(&arg_tok->list, &on_tok->list);
}
static int check_if_different(struct trailer_item *in_tok,
struct trailer_item *arg_tok,
- int check_all)
+ int check_all,
+ struct list_head *head)
{
enum action_where where = arg_tok->conf.where;
+ struct list_head *next_head;
do {
- if (!in_tok)
- return 1;
if (same_trailer(in_tok, arg_tok))
return 0;
/*
* if we want to add a trailer after another one,
* we have to check those before this one
*/
- in_tok = after_or_end(where) ? in_tok->previous : in_tok->next;
+ next_head = after_or_end(where) ? in_tok->list.prev
+ : in_tok->list.next;
+ if (next_head == head)
+ break;
+ in_tok = list_entry(next_head, struct trailer_item, list);
} while (check_all);
return 1;
}
-static void remove_from_list(struct trailer_item *item,
- struct trailer_item **first,
- struct trailer_item **last)
-{
- struct trailer_item *next = item->next;
- struct trailer_item *previous = item->previous;
-
- if (next) {
- item->next->previous = previous;
- item->next = NULL;
- } else if (last)
- *last = previous;
-
- if (previous) {
- item->previous->next = next;
- item->previous = NULL;
- } else if (first)
- *first = next;
-}
-
-static struct trailer_item *remove_first(struct trailer_item **first)
-{
- struct trailer_item *item = *first;
- *first = item->next;
- if (item->next) {
- item->next->previous = NULL;
- item->next = NULL;
- }
- return item;
-}
-
static char *apply_command(const char *command, const char *arg)
{
struct strbuf cmd = STRBUF_INIT;
@@ -266,8 +220,7 @@ static void apply_item_command(struct trailer_item *in_tok, struct trailer_item
static void apply_arg_if_exists(struct trailer_item *in_tok,
struct trailer_item *arg_tok,
struct trailer_item *on_tok,
- struct trailer_item **in_tok_first,
- struct trailer_item **in_tok_last)
+ struct list_head *head)
{
switch (arg_tok->conf.if_exists) {
case EXISTS_DO_NOTHING:
@@ -275,40 +228,34 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,
break;
case EXISTS_REPLACE:
apply_item_command(in_tok, arg_tok);
- add_arg_to_input_list(on_tok, arg_tok,
- in_tok_first, in_tok_last);
- remove_from_list(in_tok, in_tok_first, in_tok_last);
+ add_arg_to_input_list(on_tok, arg_tok);
+ list_del(&in_tok->list);
free_trailer_item(in_tok);
break;
case EXISTS_ADD:
apply_item_command(in_tok, arg_tok);
- add_arg_to_input_list(on_tok, arg_tok,
- in_tok_first, in_tok_last);
+ add_arg_to_input_list(on_tok, arg_tok);
break;
case EXISTS_ADD_IF_DIFFERENT:
apply_item_command(in_tok, arg_tok);
- if (check_if_different(in_tok, arg_tok, 1))
- add_arg_to_input_list(on_tok, arg_tok,
- in_tok_first, in_tok_last);
+ if (check_if_different(in_tok, arg_tok, 1, head))
+ add_arg_to_input_list(on_tok, arg_tok);
else
free_trailer_item(arg_tok);
break;
case EXISTS_ADD_IF_DIFFERENT_NEIGHBOR:
apply_item_command(in_tok, arg_tok);
- if (check_if_different(on_tok, arg_tok, 0))
- add_arg_to_input_list(on_tok, arg_tok,
- in_tok_first, in_tok_last);
+ if (check_if_different(on_tok, arg_tok, 0, head))
+ add_arg_to_input_list(on_tok, arg_tok);
else
free_trailer_item(arg_tok);
break;
}
}
-static void apply_arg_if_missing(struct trailer_item **in_tok_first,
- struct trailer_item **in_tok_last,
+static void apply_arg_if_missing(struct list_head *head,
struct trailer_item *arg_tok)
{
- struct trailer_item **in_tok;
enum action_where where;
switch (arg_tok->conf.if_missing) {
@@ -317,68 +264,60 @@ static void apply_arg_if_missing(struct trailer_item **in_tok_first,
break;
case MISSING_ADD:
where = arg_tok->conf.where;
- in_tok = after_or_end(where) ? in_tok_last : in_tok_first;
apply_item_command(NULL, arg_tok);
- if (*in_tok) {
- add_arg_to_input_list(*in_tok, arg_tok,
- in_tok_first, in_tok_last);
- } else {
- *in_tok_first = arg_tok;
- *in_tok_last = arg_tok;
- }
- break;
+ if (after_or_end(where))
+ list_add_tail(&arg_tok->list, head);
+ else
+ list_add(&arg_tok->list, head);
}
}
-static int find_same_and_apply_arg(struct trailer_item **in_tok_first,
- struct trailer_item **in_tok_last,
+static int find_same_and_apply_arg(struct list_head *head,
struct trailer_item *arg_tok)
{
+ struct list_head *pos;
struct trailer_item *in_tok;
struct trailer_item *on_tok;
- struct trailer_item *following_tok;
enum action_where where = arg_tok->conf.where;
int middle = (where == WHERE_AFTER) || (where == WHERE_BEFORE);
int backwards = after_or_end(where);
- struct trailer_item *start_tok = backwards ? *in_tok_last : *in_tok_first;
+ struct trailer_item *start_tok;
- for (in_tok = start_tok; in_tok; in_tok = following_tok) {
- following_tok = backwards ? in_tok->previous : in_tok->next;
+ if (list_empty(head))
+ return 0;
+
+ start_tok = list_entry(backwards ? head->prev : head->next,
+ struct trailer_item,
+ list);
+
+ list_for_each_dir(pos, head, backwards) {
+ in_tok = list_entry(pos, struct trailer_item, list);
if (!same_token(in_tok, arg_tok))
continue;
on_tok = middle ? in_tok : start_tok;
- apply_arg_if_exists(in_tok, arg_tok, on_tok,
- in_tok_first, in_tok_last);
+ apply_arg_if_exists(in_tok, arg_tok, on_tok, head);
return 1;
}
return 0;
}
-static void process_trailers_lists(struct trailer_item **in_tok_first,
- struct trailer_item **in_tok_last,
- struct trailer_item **arg_tok_first)
+static void process_trailers_lists(struct list_head *head,
+ struct list_head *arg_head)
{
+ struct list_head *pos, *p;
struct trailer_item *arg_tok;
- struct trailer_item *next_arg;
-
- if (!*arg_tok_first)
- return;
- for (arg_tok = *arg_tok_first; arg_tok; arg_tok = next_arg) {
+ list_for_each_safe(pos, p, arg_head) {
int applied = 0;
+ arg_tok = list_entry(pos, struct trailer_item, list);
- next_arg = arg_tok->next;
- remove_from_list(arg_tok, arg_tok_first, NULL);
+ list_del(pos);
- applied = find_same_and_apply_arg(in_tok_first,
- in_tok_last,
- arg_tok);
+ applied = find_same_and_apply_arg(head, arg_tok);
if (!applied)
- apply_arg_if_missing(in_tok_first,
- in_tok_last,
- arg_tok);
+ apply_arg_if_missing(head, arg_tok);
}
}
@@ -438,13 +377,12 @@ static void duplicate_conf(struct conf_info *dst, const struct conf_info *src)
static struct trailer_item *get_conf_item(const char *name)
{
+ struct list_head *pos;
struct trailer_item *item;
- struct trailer_item *previous;
/* Look up item with same name */
- for (previous = NULL, item = first_conf_item;
- item;
- previous = item, item = item->next) {
+ list_for_each(pos, &conf_head) {
+ item = list_entry(pos, struct trailer_item, list);
if (!strcasecmp(item->conf.name, name))
return item;
}
@@ -454,12 +392,7 @@ static struct trailer_item *get_conf_item(const char *name)
duplicate_conf(&item->conf, &default_conf_info);
item->conf.name = xstrdup(name);
- if (!previous)
- first_conf_item = item;
- else {
- previous->next = item;
- item->previous = previous;
- }
+ list_add_tail(&item->list, &conf_head);
return item;
}
@@ -633,6 +566,7 @@ static struct trailer_item *create_trailer_item(const char *string)
struct strbuf val = STRBUF_INIT;
struct trailer_item *item;
int tok_len;
+ struct list_head *pos;
if (parse_trailer(&tok, &val, string))
return NULL;
@@ -640,7 +574,8 @@ static struct trailer_item *create_trailer_item(const char *string)
tok_len = token_len_without_separator(tok.buf, tok.len);
/* Lookup if the token matches something in the config */
- for (item = first_conf_item; item; item = item->next) {
+ list_for_each(pos, &conf_head) {
+ item = list_entry(pos, struct trailer_item, list);
if (token_matches_item(tok.buf, item, tok_len))
return new_trailer_item(item,
strbuf_detach(&tok, NULL),
@@ -652,44 +587,34 @@ static struct trailer_item *create_trailer_item(const char *string)
strbuf_detach(&val, NULL));
}
-static void add_trailer_item(struct trailer_item **first,
- struct trailer_item **last,
- struct trailer_item *new)
+static void add_trailer_item(struct list_head *head, struct trailer_item *new)
{
if (!new)
return;
- if (!*last) {
- *first = new;
- *last = new;
- } else {
- (*last)->next = new;
- new->previous = *last;
- *last = new;
- }
+ list_add_tail(&new->list, head);
}
-static struct trailer_item *process_command_line_args(struct string_list *trailers)
+static void process_command_line_args(struct list_head *arg_head,
+ struct string_list *trailers)
{
- struct trailer_item *arg_tok_first = NULL;
- struct trailer_item *arg_tok_last = NULL;
struct string_list_item *tr;
struct trailer_item *item;
+ struct list_head *pos;
/* Add a trailer item for each configured trailer with a command */
- for (item = first_conf_item; item; item = item->next) {
+ list_for_each(pos, &conf_head) {
+ item = list_entry(pos, struct trailer_item, list);
if (item->conf.command) {
struct trailer_item *new = new_trailer_item(item, NULL, NULL);
- add_trailer_item(&arg_tok_first, &arg_tok_last, new);
+ add_trailer_item(arg_head, new);
}
}
/* Add a trailer item for each trailer on the command line */
for_each_string_list_item(tr, trailers) {
struct trailer_item *new = create_trailer_item(tr->string);
- add_trailer_item(&arg_tok_first, &arg_tok_last, new);
+ add_trailer_item(arg_head, new);
}
-
- return arg_tok_first;
}
static struct strbuf **read_input_file(const char *file)
@@ -805,8 +730,7 @@ static void print_lines(FILE *outfile, struct strbuf **lines, int start, int end
static int process_input_file(FILE *outfile,
struct strbuf **lines,
- struct trailer_item **in_tok_first,
- struct trailer_item **in_tok_last)
+ struct list_head *head)
{
int count = 0;
int patch_start, trailer_start, trailer_end, i;
@@ -829,18 +753,19 @@ static int process_input_file(FILE *outfile,
for (i = trailer_start; i < trailer_end; i++) {
if (lines[i]->buf[0] != comment_line_char) {
struct trailer_item *new = create_trailer_item(lines[i]->buf);
- add_trailer_item(in_tok_first, in_tok_last, new);
+ add_trailer_item(head, new);
}
}
return trailer_end;
}
-static void free_all(struct trailer_item **first)
+static void free_all(struct list_head *head)
{
- while (*first) {
- struct trailer_item *item = remove_first(first);
- free_trailer_item(item);
+ struct list_head *pos, *p;
+ list_for_each_safe(pos, p, head) {
+ list_del(pos);
+ free_trailer_item(list_entry(pos, struct trailer_item, list));
}
}
@@ -877,9 +802,8 @@ static FILE *create_in_place_tempfile(const char *file)
void process_trailers(const char *file, int in_place, int trim_empty, struct string_list *trailers)
{
- struct trailer_item *in_tok_first = NULL;
- struct trailer_item *in_tok_last = NULL;
- struct trailer_item *arg_tok_first;
+ LIST_HEAD(head);
+ LIST_HEAD(arg_head);
struct strbuf **lines;
int trailer_end;
FILE *outfile = stdout;
@@ -894,15 +818,15 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
outfile = create_in_place_tempfile(file);
/* Print the lines before the trailers */
- trailer_end = process_input_file(outfile, lines, &in_tok_first, &in_tok_last);
+ trailer_end = process_input_file(outfile, lines, &head);
- arg_tok_first = process_command_line_args(trailers);
+ process_command_line_args(&arg_head, trailers);
- process_trailers_lists(&in_tok_first, &in_tok_last, &arg_tok_first);
+ process_trailers_lists(&head, &arg_head);
- print_all(outfile, in_tok_first, trim_empty);
+ print_all(outfile, &head, trim_empty);
- free_all(&in_tok_first);
+ free_all(&head);
/* Print the lines after the trailers as is */
print_lines(outfile, lines, trailer_end, INT_MAX);
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 1/6] trailer: improve const correctness
From: Jonathan Tan @ 2016-10-12 23:40 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, peff, christian.couder
In-Reply-To: <cover.1476314576.git.jonathantanmy@google.com>
Change "const char *" to "char *" in struct trailer_item and in the
return value of apply_command (since those strings are owned strings).
Change "struct conf_info *" to "const struct conf_info *" (since that
struct is not modified).
---
trailer.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/trailer.c b/trailer.c
index c6ea9ac..1f191b2 100644
--- a/trailer.c
+++ b/trailer.c
@@ -27,8 +27,8 @@ static struct conf_info default_conf_info;
struct trailer_item {
struct trailer_item *previous;
struct trailer_item *next;
- const char *token;
- const char *value;
+ char *token;
+ char *value;
struct conf_info conf;
};
@@ -95,8 +95,8 @@ static void free_trailer_item(struct trailer_item *item)
free(item->conf.name);
free(item->conf.key);
free(item->conf.command);
- free((char *)item->token);
- free((char *)item->value);
+ free(item->token);
+ free(item->value);
free(item);
}
@@ -215,13 +215,13 @@ static struct trailer_item *remove_first(struct trailer_item **first)
return item;
}
-static const char *apply_command(const char *command, const char *arg)
+static char *apply_command(const char *command, const char *arg)
{
struct strbuf cmd = STRBUF_INIT;
struct strbuf buf = STRBUF_INIT;
struct child_process cp = CHILD_PROCESS_INIT;
const char *argv[] = {NULL, NULL};
- const char *result;
+ char *result;
strbuf_addstr(&cmd, command);
if (arg)
@@ -425,7 +425,7 @@ static int set_if_missing(struct conf_info *item, const char *value)
return 0;
}
-static void duplicate_conf(struct conf_info *dst, struct conf_info *src)
+static void duplicate_conf(struct conf_info *dst, const struct conf_info *src)
{
*dst = *src;
if (src->name)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 0/6] allow non-trailers and multiple-line trailers
From: Jonathan Tan @ 2016-10-12 23:40 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, peff, christian.couder
In-Reply-To: <cover.1476232683.git.jonathantanmy@google.com>
Thanks, Peff, for the pointer to list.h. Using list.h does simplify the
code by a similar amount to switching it to a singly-linked list, so I
have done that (replacing my earlier "trailer: use singly-linked list,
not doubly" patch). Another advantage is that I no longer need to change
the algorithm, making for a smaller patch.
(There are some quirks resulting from list.h implementing a circular
list, like needing to pass "head" as a sentinel when iterating from the
middle of the list, but those are minor, and my original singly-linked
list implementation had quirks too anyway like needing to pass a pointer
to the next pointer.)
Updates:
(-> 1/6)
- Added separate patch for const correctness changes
(1/5 -> 2/6)
- Dropped singly-linked list patch, instead replacing existing
doubly-linked list implementation with list.h
(5/5 -> 6/6)
- Used "char *" instead of "struct strbuf"
- Modified test slightly to test whitespace at beginning of line
Jonathan Tan (6):
trailer: improve const correctness
trailer: use list.h for doubly-linked list
trailer: streamline trailer item create and add
trailer: make args have their own struct
trailer: allow non-trailers in trailer block
trailer: support values folded to multiple lines
Documentation/git-interpret-trailers.txt | 10 +-
t/t7513-interpret-trailers.sh | 174 ++++++++++
trailer.c | 538 +++++++++++++++----------------
3 files changed, 444 insertions(+), 278 deletions(-)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply
* Re: [PATCHv3] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-12 23:33 UTC (permalink / raw)
To: Stefan Beller; +Cc: bmwill, git, j6t, jacob.keller
In-Reply-To: <20161012224109.23410-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> @@ -89,15 +114,20 @@ static void setup_check(void)
>
> ------------
> const char *path;
> + struct git_attr_result *result;
>
> setup_check();
> - git_check_attr(path, check);
> + result = git_check_attr(path, check);
This looks stale by a few revisions of the other parts of the patch?
> diff --git a/archive.c b/archive.c
> index 11e3951..15849a8 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -107,10 +107,12 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
> void *context)
> {
> static struct strbuf path = STRBUF_INIT;
> + static struct git_attr_check *check;
> +
> struct archiver_context *c = context;
> struct archiver_args *args = c->args;
> write_archive_entry_fn_t write_entry = c->write_entry;
> - static struct git_attr_check *check;
> + struct git_attr_result result = GIT_ATTR_RESULT_INIT;
> const char *path_without_prefix;
> int err;
>
> @@ -124,12 +126,16 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
> strbuf_addch(&path, '/');
> path_without_prefix = path.buf + args->baselen;
>
> - if (!check)
> - check = git_attr_check_initl("export-ignore", "export-subst", NULL);
> - if (!git_check_attr(path_without_prefix, check)) {
> - if (ATTR_TRUE(check->check[0].value))
> + git_attr_check_initl(&check, "export-ignore", "export-subst", NULL);
> + git_attr_result_init(&result, check);
> +
> + if (!git_check_attr(path_without_prefix, check, &result)) {
> + if (ATTR_TRUE(result.value[0])) {
> + git_attr_result_clear(&result);
> return 0;
> - args->convert = ATTR_TRUE(check->check[1].value);
> + }
> + args->convert = ATTR_TRUE(result.value[1]);
> + git_attr_result_clear(&result);
> }
This is exactly what I meant by "can we avoid alloc/free of result
in leaf function when we _know_ how many attributes we are
interested in already, which is the majority of the case?".
Starting with a simple but unoptimized internal implementation of
the attr subsystem is one thing (which is good). Exposing an API that
cannot be optimally implemented later without changing the callers
is another (which is bad).
By encapsulating each element into "struct git_attr_result", we can
extend the API without changing the API user [*1*].
But I do not think of a way to allow an efficient implementation
later unless the source of the API user somehow says "this user is
only interested in this many attributes", like having this
struct git_attr_result result[2];
(because this caller only wants "ignore" and "subst") on the API
user's side [*2*]. Without such a clue (like the patch above, that
only says "there is a structure called 'result'"), I do not think of
a way to avoid dynamic allocation on the API implementation side.
All the other callers in the patch (pack-objects, convert, ll-merge,
etc.) seem to share the exact same pattern. Each of the leaf
functions knows a fixed set of attributes it is interested in, the
caller iterates over many paths and makes calls to these leaf
functions, and it is a waste to pay alloc/free overhead for each and
every iteration when we know how many elements result needs to
store.
[Footnote]
*1* Would we need a wrapping struct around the array of results? If
that is the case, we may need something ugly like this on the
API user side:
GIT_ATTR_RESULT_TYPE(2) result = {2,};
with something like the following on the API implementation
side:
#define GIT_ATTR_RESULT_TYPE(n) \
struct { \
int num_slots; \
const char *value[n]; \
}
struct git_attr_result {
int num_slots;
const char *value[FLEX_ARRAY];
};
git_attr_result_init(void *result_, struct git_attr_check *check)
{
struct git_attr_result *result = result_;
assert(result->num_slots, check->num_attrs);
...
}
git_check_attr(const char *path,
struct git_attr_check *check,
void *result_)
{
struct git_attr_result *result = result_;
assert(result->num_slots, check->num_attrs);
for (i = 0; i < check_num_attrs; i++)
result->value[i] = ... found value ...;
}
*2* Or the uglier
GIT_ATTR_RESULT_TYPE(2) result = {2,};
I can see why the "check" side would benefit from a structure
that contains an array, but I do not see why "result" side would
want to, so I am hoping that we won't have to do this uglier
variant and just go with the simple "array of resulting values".
^ permalink raw reply
* Re: [PATCH] worktree: allow the main brach of a bare repository to be checked out
From: Dennis Kaarsemaker @ 2016-10-12 19:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, pclouds, mtutty, rappazzo
In-Reply-To: <xmqqfuo14dnr.fsf@gitster.mtv.corp.google.com>
On Wed, 2016-10-12 at 11:37 -0700, Junio C Hamano wrote:
> > ++test_expect_success '"add" default branch of a bare repo' '
>
> Huh?
Copy paste error. And I missed
ok 17 - checkout from a bare repo without "add"
./t2025-worktree-add.sh: 141: ./t2025-worktree-add.sh: +test_expect_success: not found
in the output of 'make test'. Thanks for fixing up!
D.
^ permalink raw reply
* Re: Huge performance bottleneck reading packs
From: Jeff King @ 2016-10-12 23:18 UTC (permalink / raw)
To: Vegard Nossum
Cc: git, Quentin Casasnovas, Shawn Pearce,
Nguyễn Thái Ngọc Duy, Junio C Hamano
In-Reply-To: <20161012230143.5kxcmtityaasra5j@sigill.intra.peff.net>
On Wed, Oct 12, 2016 at 07:01:43PM -0400, Jeff King wrote:
> On Thu, Oct 13, 2016 at 12:30:52AM +0200, Vegard Nossum wrote:
>
> > However, the commit found by 'git blame' above appears just fine to me,
> > I haven't been able to spot a bug in it.
> >
> > A closer inspection reveals the problem to really be that this is an
> > extremely hot path with more than -- holy cow -- 4,106,756,451
> > iterations on the 'packed_git' list for a single 'git fetch' on my
> > repository. I'm guessing the patch above just made the inner loop
> > ever so slightly slower.
> >
> > My .git/objects/pack/ has ~2088 files (1042 idx files, 1042 pack files,
> > and 4 tmp_pack_* files).
>
> Yeah. I agree that the commit you found makes the check a little more
> expensive, but I think the root of the problem is calling
> prepare_packed_git_one many times. This _should_ happen once for each
> pack at program startup, and possibly again if we need to re-scan the
> pack directory to account for racing with a simultaneous repack.
>
> The latter is generally triggered when we fail to look up an object we
> expect to exist. So I'd suspect 45e8a74 (has_sha1_file: re-check pack
> directory before giving up, 2013-08-30) is playing a part. We dealt with
> that to some degree in 0eeb077 (index-pack: avoid excessive re-reading
> of pack directory, 2015-06-09), but it would not surprise me if there is
> another spot that needs similar treatment.
>
> Does the patch below help?
Also, is it possible to make the repository in question available? I
might be able to reproduce based on your description, but it would save
time if I could directly run gdb on your example.
Specifically, I'm interested in the call graph that leads to calling
reprepare_packed_git().
-Peff
^ permalink raw reply
* Re: Formatting problem send_mail in version 2.10.0
From: Jeff King @ 2016-10-12 23:13 UTC (permalink / raw)
To: Junio C Hamano
Cc: Matthieu Moy, Larry Finger, Mathieu Lienard--Mayor, Remi Lespinet,
git
In-Reply-To: <xmqqtwch2srj.fsf@gitster.mtv.corp.google.com>
On Wed, Oct 12, 2016 at 01:53:52PM -0700, Junio C Hamano wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
> >>> If it's not in the body of the message, then where is it?
> >>
> >> This point is clarified in the thread
> >> http://marc.info/?l=linux-wireless&m=147625930203434&w=2, which is
> >> with my upstream maintainer.
> >
> > Which explicitly states that the syntax is not [$number], but # $number,
> > right?
>
> But I do not think that works, either. Let's step back.
>
> People write things like these
>
> Cc: Stable <stable@vger.kernel.org> # 4.8
> Cc: Stable <stable@vger.kernel.org> [4.8+]
>
> in the trailer part in the body of the message. Are these lines
> meant to be usable if they appear as Cc: headers of an outgoing
> piece of e-mail as-is?
I think the answer is pretty clearly no. It's just that historically we
have auto-munged it into something useful. I think the viable options
are basically:
1. Tell people not to do that, and to do something RFC compliant like
"Stable [4.8+]" <stable@vger.kernel.org>. This is a little funny
for git because we otherwise do not require things like
rfc-compliant quoting for our name/email pairs. But it Just Works
without anybody having to write extra code, or worry about corner
cases in parsing.
2. Drop everything after the trailing ">". This gives a valid rfc2822
cc, and people can pick the "# 4.8" from the cc line in the body.
3. Rewrite
A <B@C> D
into
A D <B@C>
regardless of what is in "D". This retains the information in the
rfc2822 cc.
Starting from scratch, I'd say that (2) seems like a good combination of
simplicity and friendliness. But (3) matches what we have done
historically (and still do at least for some values of "D", and
depending on the presence of Mail::Address).
Once we decide on a behavior, it seems like we should be able to apply
it consistently with or without Mail::Address by grabbing the bits after
the final ">".
Larry seems to be against (2), but I'm not sure I understand why pulling
the value from the in-body cc (which gets copied into the commit message
by git-am, too) would be a problem.
-Peff
^ permalink raw reply
* Re: [PATCHv2] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-12 22:57 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Brandon Williams
In-Reply-To: <CAGZ79kaBhXHLDEK0XMLjm3QofmtaGZspA3EEx5x4-qCYY--wZA@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
>> But many callers do not follow that; rather they do
>>
>> loop to iterate over paths {
>> call a helper func to learn attr X for path
>> use the value of attr X
>> }
>>
>> using a callchain that embeds a helper function deep inside, and
>> "check" is kept in the helper, check-attr function is called from
>> there, and "result" is not passed from the caller to the helper
>> (obviously, because it does not exist in the current API). See the
>> callchain that leads down to convert.c::convert_attrs() for a
>> typical example. When converted to the new API, it needs to have a
>> new "result" structure every time it is called, and cannot reuse the
>> one that was used in its previous call.
>
> Why would that be? i.e. I do not understand the reasoning/motivation
> as well as what you propose to change here.
The leaf function may look like
check_eol(const char *path)
{
static struct git_attr_check *check;
initl(&check, "eol", NULL);
git_check_attr(&check, path, result);
return nth_element_in_result(result, 0);
}
and we want "result" to be thread-ready.
A naive and undesired way to put it on stack is like so:
const char *check_eol(const char *path)
{
static struct git_attr_check *check;
struct git_attr_result result = RESULT_INIT;
const char *eol;
initl(&check, "eol", NULL);
init_result(&check, &result);
git_check_attr(&check, path, &result);
eol = nth_element_in_result(&result, 0);
clear_result(&result);
return eol;
}
where your "struct git_attr_result" has a fixed size, and the actual
result array is allocated via ALLOC_GROW() etc. That's overhead
that we do not want. Instead can't we do this?
const char *check_eol(const char *path)
{
static struct git_attr_check *check;
/* we know we only want one */
struct git_attr_result result[1];
const char *eol;
initl(&check, "eol", NULL);
git_check_attr(&check, path, result);
return result[0];
}
That way, we won't be doing ALLOC_GROW() in init_result() or free in
clear_result().
If you use a structure that has pointer to an array (i.e. the "naive
and undesired way" above), you cannot amortize the alloc/free by
making result "static" if you want to be thread-ready.
^ permalink raw reply
* Re: Huge performance bottleneck reading packs
From: Jeff King @ 2016-10-12 23:01 UTC (permalink / raw)
To: Vegard Nossum
Cc: git, Quentin Casasnovas, Shawn Pearce,
Nguyễn Thái Ngọc Duy, Junio C Hamano
In-Reply-To: <ea8db41f-2ea4-b37b-e6f8-1f1d428aea5d@oracle.com>
On Thu, Oct 13, 2016 at 12:30:52AM +0200, Vegard Nossum wrote:
> However, the commit found by 'git blame' above appears just fine to me,
> I haven't been able to spot a bug in it.
>
> A closer inspection reveals the problem to really be that this is an
> extremely hot path with more than -- holy cow -- 4,106,756,451
> iterations on the 'packed_git' list for a single 'git fetch' on my
> repository. I'm guessing the patch above just made the inner loop
> ever so slightly slower.
>
> My .git/objects/pack/ has ~2088 files (1042 idx files, 1042 pack files,
> and 4 tmp_pack_* files).
Yeah. I agree that the commit you found makes the check a little more
expensive, but I think the root of the problem is calling
prepare_packed_git_one many times. This _should_ happen once for each
pack at program startup, and possibly again if we need to re-scan the
pack directory to account for racing with a simultaneous repack.
The latter is generally triggered when we fail to look up an object we
expect to exist. So I'd suspect 45e8a74 (has_sha1_file: re-check pack
directory before giving up, 2013-08-30) is playing a part. We dealt with
that to some degree in 0eeb077 (index-pack: avoid excessive re-reading
of pack directory, 2015-06-09), but it would not surprise me if there is
another spot that needs similar treatment.
Does the patch below help?
diff --git a/builtin/fetch.c b/builtin/fetch.c
index d5329f9..c0f3c2c 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -243,7 +243,7 @@ static void find_non_local_tags(struct transport *transport,
if (ends_with(ref->name, "^{}")) {
if (item && !has_object_file(&ref->old_oid) &&
!will_fetch(head, ref->old_oid.hash) &&
- !has_sha1_file(item->util) &&
+ !has_sha1_file_with_flags(item->util, HAS_SHA1_QUICK) &&
!will_fetch(head, item->util))
item->util = NULL;
item = NULL;
@@ -256,7 +256,8 @@ static void find_non_local_tags(struct transport *transport,
* to check if it is a lightweight tag that we want to
* fetch.
*/
- if (item && !has_sha1_file(item->util) &&
+ if (item &&
+ !has_sha1_file_with_flags(item->util, HAS_SHA1_QUICK) &&
!will_fetch(head, item->util))
item->util = NULL;
@@ -276,7 +277,8 @@ static void find_non_local_tags(struct transport *transport,
* We may have a final lightweight tag that needs to be
* checked to see if it needs fetching.
*/
- if (item && !has_sha1_file(item->util) &&
+ if (item &&
+ !has_sha1_file_with_flags(item->util, HAS_SHA1_QUICK) &&
!will_fetch(head, item->util))
item->util = NULL;
-Peff
^ permalink raw reply related
* Re: [PATCHv2] attr: convert to new threadsafe API
From: Stefan Beller @ 2016-10-12 21:47 UTC (permalink / raw)
To: Jacob Keller
Cc: Johannes Sixt, Junio C Hamano, Git mailing list, Brandon Williams
In-Reply-To: <CA+P7+xqBUT3jUsxciVydO+nRoR+iJygWG=y_ARpiQSs+-kcH2A@mail.gmail.com>
On Wed, Oct 12, 2016 at 2:45 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Wed, Oct 12, 2016 at 1:07 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>>
>> Sigh. DCLP, the Double Checked Locking Pattern. These days, it should be
>> common knowledge among professionals that this naïve version _does_not_work_
>> [1]!
>>
>> I suggest you go without it, then measure, and only *then* optimize if it is
>> a bottleneck. Did I read "we do not expect much contention" somewhere?
>>
>> [1] http://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf C++ centric,
>> but applies to C just as well
>>
>> -- Hannes
>>
>
>
> You know, I always wondered why Linux Kernel code needed memory
> barriers but userspace programs didn't seem to use them.. turns out
> they actually *do* need them for the same exact types of problems...
>
> Thanks,
> Jake
In a former job I made use of them, too. So I am kinda embarrassed.
(I cannot claim I did not know about these patterns and memory
fencing, it just escaped my consciousness).
^ permalink raw reply
* Re: Huge performance bottleneck reading packs
From: Junio C Hamano @ 2016-10-12 22:45 UTC (permalink / raw)
To: Vegard Nossum
Cc: git, Quentin Casasnovas, Shawn Pearce, Jeff King,
Nguyễn Thái Ngọc Duy
In-Reply-To: <ea8db41f-2ea4-b37b-e6f8-1f1d428aea5d@oracle.com>
Vegard Nossum <vegard.nossum@oracle.com> writes:
> A closer inspection reveals the problem to really be that this is an
> extremely hot path with more than -- holy cow -- 4,106,756,451
> iterations on the 'packed_git' list for a single 'git fetch' on my
> repository. I'm guessing the patch above just made the inner loop
> ever so slightly slower.
Very plausible, and this ...
> My .git/objects/pack/ has ~2088 files (1042 idx files, 1042 pack files,
> and 4 tmp_pack_* files).
... may explain why nobody else has seen a difference.
Is there a reason why your repository has that many pack files? Is
automatic GC not working for some reason?
"gc" would try to make sure that you have reasonably low number of
packs, as having too many packs is detrimental for performance for
multiple reasons, including:
* All objects in a single pack expressed in delta format (i.e. only
the difference from another object is stored) must eventually
have another object that its difference is based on recorded in
the full format in the same packfile.
* A single packfile records a single object only once, but it is
normal (and often required because of the point above) that the
same object appears in multiple packfiles.
* Locating of objects from a single packfile uses its .idx file by
binary search of sorted list of object names, which is efficient,
but this cost is multiplied linearly as the number of packs you
have in your repository.
^ permalink raw reply
* [PATCHv3] attr: convert to new threadsafe API
From: Stefan Beller @ 2016-10-12 22:41 UTC (permalink / raw)
To: gitster, bmwill; +Cc: git, j6t, jacob.keller, Stefan Beller
This revamps the API of the attr subsystem to be thread safe.
Before we had the question and its results in one struct type.
The typical usage of the API was
static struct git_attr_check *check;
if (!check)
check = git_attr_check_initl("text", NULL);
git_check_attr(path, check);
act_on(check->value[0]);
This has a couple of issues when it comes to thread safety:
* the initialization is racy in this implementation. To make it
thread safe, we need to acquire a mutex, such that only one
thread is executing the code in git_attr_check_initl.
As we do not want to introduce a mutex at each call site,
this is best done in the attr code. However to do so, we need
to have access to the `check` variable, i.e. the API has to
look like
git_attr_check_initl(struct git_attr_check*, ...);
Then one of the threads calling git_attr_check_initl will
acquire the mutex and init the `check`, while all other threads
will wait on the mutex just to realize they're late to the
party and they'll return with no work done.
* While the check for attributes to be questioned only need to
be initalized once as that part will be read only after its
initialisation, the answer may be different for each path.
Because of that we need to decouple the check and the answer,
such that each thread can obtain an answer for the path it
is currently processing.
This commit changes the API and adds locking in
git_attr_check_initl that provides the thread safety.
The usage of the new API will be:
/*
* The initl call will thread-safely check whether the
* struct git_attr_check has been initialized. We only
* want to do the initialization work once, hence we do
* that work inside a thread safe environment.
*/
static struct git_attr_check *check;
git_attr_check_initl(&check, "text", NULL);
/*
* Resize internal data structures in the result to match
* the size of `check`:
*/
struct git_attr_result *result = GIT_ATTR_RESULT_INIT;
git_attr_result_init(&result, check);
/* Perform the check and act on it: */
git_check_attr(path, check, &result);
act_on(check->value[0]);
/*
* Free the result.
* The check is static, so doesn't require free'ing
*/
git_attr_result_clear(&result);
Signed-off-by: Stefan Beller <sbeller@google.com>
---
* Do not use the Double Check Locking Pattern
* The callers are all thread safe now (i.e. no static result)
* So this is roughly what we want for the first version of the conversion?
Thanks,
Stefan
Documentation/technical/api-gitattributes.txt | 93 +++++++++++++------
archive.c | 18 ++--
attr.c | 125 +++++++++++++++++---------
attr.h | 80 +++++++++++------
builtin/check-attr.c | 31 ++++---
builtin/pack-objects.c | 20 +++--
convert.c | 43 +++++----
ll-merge.c | 28 +++---
userdiff.c | 24 +++--
ws.c | 17 ++--
10 files changed, 314 insertions(+), 165 deletions(-)
diff --git a/Documentation/technical/api-gitattributes.txt b/Documentation/technical/api-gitattributes.txt
index 92fc32a..ac97244 100644
--- a/Documentation/technical/api-gitattributes.txt
+++ b/Documentation/technical/api-gitattributes.txt
@@ -8,6 +8,18 @@ attributes to set of paths.
Data Structure
--------------
+extern struct git_attr *git_attr(const char *);
+
+/*
+ * Return the name of the attribute represented by the argument. The
+ * return value is a pointer to a null-delimited string that is part
+ * of the internal data structure; it should not be modified or freed.
+ */
+extern const char *git_attr_name(const struct git_attr *);
+
+extern int attr_name_valid(const char *name, size_t namelen);
+extern void invalid_attr_name_message(struct strbuf *, const char *, int);
+
`struct git_attr`::
An attribute is an opaque object that is identified by its name.
@@ -16,15 +28,17 @@ Data Structure
of no interest to the calling programs. The name of the
attribute can be retrieved by calling `git_attr_name()`.
-`struct git_attr_check_elem`::
-
- This structure represents one attribute and its value.
-
`struct git_attr_check`::
- This structure represents a collection of `git_attr_check_elem`.
+ This structure represents a collection of `struct git_attrs`.
It is passed to `git_check_attr()` function, specifying the
- attributes to check, and receives their values.
+ attributes to check, and receives their values into a corresponding
+ `struct git_attr_result`.
+
+`struct git_attr_result`::
+
+ This structure represents a collection of results to its
+ corresponding `struct git_attr_check`, that has the same order.
Attribute Values
@@ -53,19 +67,30 @@ value of the attribute for the path.
Querying Specific Attributes
----------------------------
-* Prepare `struct git_attr_check` using git_attr_check_initl()
+* Prepare a `struct git_attr_check` using `git_attr_check_initl()`
function, enumerating the names of attributes whose values you are
interested in, terminated with a NULL pointer. Alternatively, an
- empty `struct git_attr_check` can be prepared by calling
- `git_attr_check_alloc()` function and then attributes you want to
- ask about can be added to it with `git_attr_check_append()`
- function.
-
-* Call `git_check_attr()` to check the attributes for the path.
-
-* Inspect `git_attr_check` structure to see how each of the
- attribute in the array is defined for the path.
-
+ empty `struct git_attr_check` as allocated by git_attr_check_alloc()
+ can be prepared by calling `git_attr_check_alloc()` function and
+ then attributes you want to ask about can be added to it with
+ `git_attr_check_append()` function.
+ `git_attr_check_initl()` is thread safe, i.e. you can call it
+ from different threads at the same time; when check determines
+ the initialzisation is still needed, the threads will use a
+ single global mutex to perform the initialization just once, the
+ others will wait on the the thread to actually perform the
+ initialization.
+
+* Prepare a `struct git_attr_result` using `git_attr_result_init()`
+ for the check struct. The call to initialize the result is not thread
+ safe, because different threads need their own thread local result
+ anyway.
+
+* Call `git_check_attr()` to check the attributes for the path,
+ the returned `git_attr_result` contains the result.
+
+* Inspect the returned `git_attr_result` structure to see how
+ each of the attribute in the array is defined for the path.
Example
-------
@@ -89,15 +114,20 @@ static void setup_check(void)
------------
const char *path;
+ struct git_attr_result *result;
setup_check();
- git_check_attr(path, check);
+ result = git_check_attr(path, check);
------------
-. Act on `.value` member of the result, left in `check->check[]`:
+The `result` must not be free'd as it is owned by the attr subsystem.
+It is reused by the same thread, so a subsequent call to git_check_attr
+in the same thread will overwrite the result.
+
+. Act on `result->value[]`:
------------
- const char *value = check->check[0].value;
+ const char *value = result->value[0];
if (ATTR_TRUE(value)) {
The attribute is Set, by listing only the name of the
@@ -123,6 +153,8 @@ the first step in the above would be different.
static struct git_attr_check *check;
static void setup_check(const char **argv)
{
+ if (check)
+ return; /* already done */
check = git_attr_check_alloc();
while (*argv) {
struct git_attr *attr = git_attr(*argv);
@@ -138,17 +170,20 @@ Querying All Attributes
To get the values of all attributes associated with a file:
-* Prepare an empty `git_attr_check` structure by calling
- `git_attr_check_alloc()`.
+* Setup a local variables on the stack for both the question
+ `struct git_attr_check` as well as the result `struct git_attr_result`.
+ Zero them out via their respective _INIT macro.
-* Call `git_all_attrs()`, which populates the `git_attr_check`
- with the attributes attached to the path.
+* Call `git_all_attrs()`
-* Iterate over the `git_attr_check.check[]` array to examine
- the attribute names and values. The name of the attribute
- described by a `git_attr_check.check[]` object can be retrieved via
- `git_attr_name(check->check[i].attr)`. (Please note that no items
+* Iterate over the `git_attr_check.attr[]` array to examine the
+ attribute names. The name of the attribute described by a
+ `git_attr_check.attr[]` object can be retrieved via
+ `git_attr_name(check->attr[i])`. (Please note that no items
will be returned for unset attributes, so `ATTR_UNSET()` will return
false for all returned `git_array_check` objects.)
+ The respective value for an attribute can be found in the same
+ index position in of `git_attr_result`.
-* Free the `git_array_check` by calling `git_attr_check_free()`.
+* Clear the local variables by calling `git_attr_check_clear()` and
+ `git_attr_result_clear()`.
diff --git a/archive.c b/archive.c
index 11e3951..15849a8 100644
--- a/archive.c
+++ b/archive.c
@@ -107,10 +107,12 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
void *context)
{
static struct strbuf path = STRBUF_INIT;
+ static struct git_attr_check *check;
+
struct archiver_context *c = context;
struct archiver_args *args = c->args;
write_archive_entry_fn_t write_entry = c->write_entry;
- static struct git_attr_check *check;
+ struct git_attr_result result = GIT_ATTR_RESULT_INIT;
const char *path_without_prefix;
int err;
@@ -124,12 +126,16 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
strbuf_addch(&path, '/');
path_without_prefix = path.buf + args->baselen;
- if (!check)
- check = git_attr_check_initl("export-ignore", "export-subst", NULL);
- if (!git_check_attr(path_without_prefix, check)) {
- if (ATTR_TRUE(check->check[0].value))
+ git_attr_check_initl(&check, "export-ignore", "export-subst", NULL);
+ git_attr_result_init(&result, check);
+
+ if (!git_check_attr(path_without_prefix, check, &result)) {
+ if (ATTR_TRUE(result.value[0])) {
+ git_attr_result_clear(&result);
return 0;
- args->convert = ATTR_TRUE(check->check[1].value);
+ }
+ args->convert = ATTR_TRUE(result.value[1]);
+ git_attr_result_clear(&result);
}
if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
diff --git a/attr.c b/attr.c
index 0faa69f..648c4f9 100644
--- a/attr.c
+++ b/attr.c
@@ -14,6 +14,7 @@
#include "dir.h"
#include "utf8.h"
#include "quote.h"
+#include "thread-utils.h"
const char git_attr__true[] = "(builtin)true";
const char git_attr__false[] = "\0(builtin)false";
@@ -46,6 +47,19 @@ struct git_attr {
static int attr_nr;
static struct git_attr *(git_attr_hash[HASHSIZE]);
+#ifndef NO_PTHREADS
+
+static pthread_mutex_t attr_mutex;
+#define attr_lock() pthread_mutex_lock(&attr_mutex)
+#define attr_unlock() pthread_mutex_unlock(&attr_mutex)
+
+#else
+
+#define attr_lock() (void)0
+#define attr_unlock() (void)0
+
+#endif /* NO_PTHREADS */
+
/*
* NEEDSWORK: maybe-real, maybe-macro are not property of
* an attribute, as it depends on what .gitattributes are
@@ -55,6 +69,16 @@ static struct git_attr *(git_attr_hash[HASHSIZE]);
*/
static int cannot_trust_maybe_real;
+/*
+ * Send one or more git_attr_check to git_check_attrs(), and
+ * each 'value' member tells what its value is.
+ * Unset one is returned as NULL.
+ */
+struct git_attr_check_elem {
+ const struct git_attr *attr;
+ const char *value;
+};
+
/* NEEDSWORK: This will become per git_attr_check */
static struct git_attr_check_elem *check_all_attr;
@@ -781,7 +805,7 @@ static int macroexpand_one(int nr, int rem)
static int attr_check_is_dynamic(const struct git_attr_check *check)
{
- return (void *)(check->check) != (void *)(check + 1);
+ return (void *)(check->attr) != (void *)(check + 1);
}
static void empty_attr_check_elems(struct git_attr_check *check)
@@ -799,7 +823,8 @@ static void empty_attr_check_elems(struct git_attr_check *check)
* any and all attributes that are visible are collected in it.
*/
static void collect_some_attrs(const char *path, int pathlen,
- struct git_attr_check *check, int collect_all)
+ struct git_attr_check *check,
+ struct git_attr_result *result, int collect_all)
{
struct attr_stack *stk;
@@ -825,13 +850,11 @@ static void collect_some_attrs(const char *path, int pathlen,
check_all_attr[i].value = ATTR__UNKNOWN;
if (!collect_all && !cannot_trust_maybe_real) {
- struct git_attr_check_elem *celem = check->check;
-
rem = 0;
for (i = 0; i < check->check_nr; i++) {
- if (!celem[i].attr->maybe_real) {
+ if (!check->attr[i]->maybe_real) {
struct git_attr_check_elem *c;
- c = check_all_attr + celem[i].attr->attr_nr;
+ c = check_all_attr + check->attr[i]->attr_nr;
c->value = ATTR__UNSET;
rem++;
}
@@ -845,38 +868,45 @@ static void collect_some_attrs(const char *path, int pathlen,
rem = fill(path, pathlen, basename_offset, stk, rem);
if (collect_all) {
- empty_attr_check_elems(check);
for (i = 0; i < attr_nr; i++) {
const struct git_attr *attr = check_all_attr[i].attr;
const char *value = check_all_attr[i].value;
if (value == ATTR__UNSET || value == ATTR__UNKNOWN)
continue;
- git_attr_check_append(check, attr)->value = value;
+
+ git_attr_check_append(check, attr);
+
+ ALLOC_GROW(result->value,
+ result->check_nr + 1,
+ result->check_alloc);
+ result->value[result->check_nr++] = value;
}
}
}
static int git_check_attrs(const char *path, int pathlen,
- struct git_attr_check *check)
+ struct git_attr_check *check,
+ struct git_attr_result *result)
{
int i;
- struct git_attr_check_elem *celem = check->check;
- collect_some_attrs(path, pathlen, check, 0);
+ collect_some_attrs(path, pathlen, check, result, 0);
for (i = 0; i < check->check_nr; i++) {
- const char *value = check_all_attr[celem[i].attr->attr_nr].value;
+ const char *value = check_all_attr[check->attr[i]->attr_nr].value;
if (value == ATTR__UNKNOWN)
value = ATTR__UNSET;
- celem[i].value = value;
+ result->value[i] = value;
}
return 0;
}
-void git_all_attrs(const char *path, struct git_attr_check *check)
+void git_all_attrs(const char *path,
+ struct git_attr_check *check,
+ struct git_attr_result *result)
{
- collect_some_attrs(path, strlen(path), check, 1);
+ collect_some_attrs(path, strlen(path), check, result, 1);
}
void git_attr_set_direction(enum git_attr_direction new, struct index_state *istate)
@@ -892,36 +922,42 @@ void git_attr_set_direction(enum git_attr_direction new, struct index_state *ist
use_index = istate;
}
-static int git_check_attr_counted(const char *path, int pathlen,
- struct git_attr_check *check)
+int git_check_attr(const char *path,
+ struct git_attr_check *check,
+ struct git_attr_result *result)
{
+ if (result->check_nr != check->check_nr)
+ die("BUG: git_attr_result is not prepared correctly");
check->finalized = 1;
- return git_check_attrs(path, pathlen, check);
+ return git_check_attrs(path, strlen(path), check, result);
}
-int git_check_attr(const char *path, struct git_attr_check *check)
+void git_attr_check_initl(struct git_attr_check **check_,
+ const char *one, ...)
{
- return git_check_attr_counted(path, strlen(path), check);
-}
-
-struct git_attr_check *git_attr_check_initl(const char *one, ...)
-{
- struct git_attr_check *check;
int cnt;
va_list params;
const char *param;
+ struct git_attr_check *check;
+
+ attr_lock();
+ if (*check_) {
+ attr_unlock();
+ return;
+ }
va_start(params, one);
for (cnt = 1; (param = va_arg(params, const char *)) != NULL; cnt++)
;
va_end(params);
+
check = xcalloc(1,
- sizeof(*check) + cnt * sizeof(*(check->check)));
+ sizeof(*check) + cnt * sizeof(*(check->attr)));
check->check_nr = cnt;
check->finalized = 1;
- check->check = (struct git_attr_check_elem *)(check + 1);
+ check->attr = (const struct git_attr **)(check + 1);
- check->check[0].attr = git_attr(one);
+ check->attr[0] = git_attr(one);
va_start(params, one);
for (cnt = 1; cnt < check->check_nr; cnt++) {
struct git_attr *attr;
@@ -932,10 +968,11 @@ struct git_attr_check *git_attr_check_initl(const char *one, ...)
attr = git_attr(param);
if (!attr)
die("BUG: %s: not a valid attribute name", param);
- check->check[cnt].attr = attr;
+ check->attr[cnt] = attr;
}
va_end(params);
- return check;
+ *check_ = check;
+ attr_unlock();
}
struct git_attr_check *git_attr_check_alloc(void)
@@ -943,18 +980,23 @@ struct git_attr_check *git_attr_check_alloc(void)
return xcalloc(1, sizeof(struct git_attr_check));
}
-struct git_attr_check_elem *git_attr_check_append(struct git_attr_check *check,
- const struct git_attr *attr)
+void git_attr_result_init(struct git_attr_result *result,
+ struct git_attr_check *check)
+{
+ ALLOC_GROW(result->value, check->check_nr, result->check_alloc);
+ result->check_nr = check->check_nr;
+}
+
+
+void git_attr_check_append(struct git_attr_check *check,
+ const struct git_attr *attr)
{
- struct git_attr_check_elem *elem;
if (check->finalized)
die("BUG: append after git_attr_check structure is finalized");
if (!attr_check_is_dynamic(check))
die("BUG: appending to a statically initialized git_attr_check");
- ALLOC_GROW(check->check, check->check_nr + 1, check->check_alloc);
- elem = &check->check[check->check_nr++];
- elem->attr = attr;
- return elem;
+ ALLOC_GROW(check->attr, check->check_nr + 1, check->check_alloc);
+ check->attr[check->check_nr++] = attr;
}
void git_attr_check_clear(struct git_attr_check *check)
@@ -962,12 +1004,13 @@ void git_attr_check_clear(struct git_attr_check *check)
empty_attr_check_elems(check);
if (!attr_check_is_dynamic(check))
die("BUG: clearing a statically initialized git_attr_check");
- free(check->check);
+ free(check->attr);
check->check_alloc = 0;
}
-void git_attr_check_free(struct git_attr_check *check)
+void git_attr_result_clear(struct git_attr_result *result)
{
- git_attr_check_clear(check);
- free(check);
+ free(result->value);
+ result->check_nr = 0;
+ result->check_alloc = 0;
}
diff --git a/attr.h b/attr.h
index 163fcd6..620f6f8 100644
--- a/attr.h
+++ b/attr.h
@@ -10,6 +10,13 @@ struct git_attr;
*/
extern struct git_attr *git_attr(const char *);
+/*
+ * Return the name of the attribute represented by the argument. The
+ * return value is a pointer to a null-delimited string that is part
+ * of the internal data structure; it should not be modified or freed.
+ */
+extern const char *git_attr_name(const struct git_attr *);
+
extern int attr_name_valid(const char *name, size_t namelen);
extern void invalid_attr_name_message(struct strbuf *, const char *, int);
@@ -22,44 +29,65 @@ extern const char git_attr__false[];
#define ATTR_FALSE(v) ((v) == git_attr__false)
#define ATTR_UNSET(v) ((v) == NULL)
-/*
- * Send one or more git_attr_check to git_check_attrs(), and
- * each 'value' member tells what its value is.
- * Unset one is returned as NULL.
- */
-struct git_attr_check_elem {
- const struct git_attr *attr;
- const char *value;
-};
-
struct git_attr_check {
int finalized;
int check_nr;
int check_alloc;
- struct git_attr_check_elem *check;
+ const struct git_attr **attr;
};
+#define GIT_ATTR_CHECK_INIT {0, 0, 0, NULL}
-extern struct git_attr_check *git_attr_check_initl(const char *, ...);
-extern int git_check_attr(const char *path, struct git_attr_check *);
+struct git_attr_result {
+ int check_nr;
+ int check_alloc;
+ const char **value;
+};
+#define GIT_ATTR_RESULT_INIT {0, 0, NULL}
+/*
+ * Initialize the `git_attr_check` via one of the following three functions:
+ *
+ * git_attr_check_alloc allocates an empty check,
+ * git_attr_check_append add an attribute to the given git_attr_check
+ * git_attr_result_init prepare the result struct for a given check struct
+ *
+ * git_all_attrs allocates a check and fills in all attributes that
+ * are set for the given path.
+ * git_attr_check_initl takes a pointer to where the check will be initialized,
+ * followed by all attributes that are to be checked.
+ * This makes it potentially thread safe as it could
+ * internally have a mutex for that memory location.
+ * Currently it is not thread safe!
+ */
extern struct git_attr_check *git_attr_check_alloc(void);
-extern struct git_attr_check_elem *git_attr_check_append(struct git_attr_check *, const struct git_attr *);
+extern void git_attr_check_append(struct git_attr_check *,
+ const struct git_attr *);
+extern void git_attr_check_initl(struct git_attr_check **,
+ const char *, ...);
-extern void git_attr_check_clear(struct git_attr_check *);
-extern void git_attr_check_free(struct git_attr_check *);
+extern void git_attr_result_init(struct git_attr_result *,
+ struct git_attr_check *);
-/*
- * Return the name of the attribute represented by the argument. The
- * return value is a pointer to a null-delimited string that is part
- * of the internal data structure; it should not be modified or freed.
- */
-extern const char *git_attr_name(const struct git_attr *);
+extern void git_all_attrs(const char *path,
+ struct git_attr_check *,
+ struct git_attr_result *);
-/*
- * Retrieve all attributes that apply to the specified path.
- * check holds the attributes and their values.
+/* Query a path for its attributes */
+extern int git_check_attr(const char *path,
+ struct git_attr_check *,
+ struct git_attr_result *);
+
+
+
+/**
+ * Free or clear the result struct. The underlying strings are not free'd
+ * as they are globally known.
*/
-void git_all_attrs(const char *path, struct git_attr_check *check);
+extern void git_attr_result_free(struct git_attr_result *);
+extern void git_attr_result_clear(struct git_attr_result *);
+
+extern void git_attr_check_clear(struct git_attr_check *);
+
enum git_attr_direction {
GIT_ATTR_CHECKIN,
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index ec61476..5e78d5d 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -24,13 +24,17 @@ static const struct option check_attr_options[] = {
OPT_END()
};
-static void output_attr(struct git_attr_check *check, const char *file)
+static void output_attr(struct git_attr_check *check,
+ struct git_attr_result *result, const char *file)
{
int j;
int cnt = check->check_nr;
+ if (check->check_nr != result->check_nr)
+ die("BUG: confused check and result internally");
+
for (j = 0; j < cnt; j++) {
- const char *value = check->check[j].value;
+ const char *value = result->value[j];
if (ATTR_TRUE(value))
value = "set";
@@ -44,11 +48,11 @@ static void output_attr(struct git_attr_check *check, const char *file)
"%s%c" /* attrname */
"%s%c" /* attrvalue */,
file, 0,
- git_attr_name(check->check[j].attr), 0, value, 0);
+ git_attr_name(check->attr[j]), 0, value, 0);
} else {
quote_c_style(file, NULL, stdout, 0);
printf(": %s: %s\n",
- git_attr_name(check->check[j].attr), value);
+ git_attr_name(check->attr[j]), value);
}
}
}
@@ -59,16 +63,21 @@ static void check_attr(const char *prefix,
{
char *full_path =
prefix_path(prefix, prefix ? strlen(prefix) : 0, file);
+ struct git_attr_result result = GIT_ATTR_RESULT_INIT;
+ struct git_attr_check local_check = GIT_ATTR_CHECK_INIT;
+
if (check != NULL) {
- if (git_check_attr(full_path, check))
- die("git_check_attr died");
- output_attr(check, file);
+ git_attr_result_init(&result, check);
+ git_check_attr(full_path, check, &result);
} else {
- check = git_attr_check_alloc();
- git_all_attrs(full_path, check);
- output_attr(check, file);
- git_attr_check_free(check);
+ git_all_attrs(full_path, &local_check, &result);
+ check = &local_check;
}
+
+ output_attr(check, &result, file);
+
+ git_attr_result_clear(&result);
+ git_attr_check_clear(&local_check);
free(full_path);
}
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 3918c07..14b764a 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -900,13 +900,19 @@ static int no_try_delta(const char *path)
{
static struct git_attr_check *check;
- if (!check)
- check = git_attr_check_initl("delta", NULL);
- if (git_check_attr(path, check))
- return 0;
- if (ATTR_FALSE(check->check[0].value))
- return 1;
- return 0;
+ struct git_attr_result result = GIT_ATTR_RESULT_INIT;
+ int ret = 0;
+
+ git_attr_check_initl(&check, "delta", NULL);
+ git_attr_result_init(&result, check);
+
+ if (!git_check_attr(path, check, &result)) {
+ if (ATTR_FALSE(result.value[0]))
+ ret = 1;
+ }
+
+ git_attr_result_clear(&result);
+ return ret;
}
/*
diff --git a/convert.c b/convert.c
index bb2435a..01de8ef 100644
--- a/convert.c
+++ b/convert.c
@@ -718,10 +718,8 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,
return 1;
}
-static enum crlf_action git_path_check_crlf(struct git_attr_check_elem *check)
+static enum crlf_action git_path_check_crlf(const char *value)
{
- const char *value = check->value;
-
if (ATTR_TRUE(value))
return CRLF_TEXT;
else if (ATTR_FALSE(value))
@@ -735,10 +733,8 @@ static enum crlf_action git_path_check_crlf(struct git_attr_check_elem *check)
return CRLF_UNDEFINED;
}
-static enum eol git_path_check_eol(struct git_attr_check_elem *check)
+static enum eol git_path_check_eol(const char *value)
{
- const char *value = check->value;
-
if (ATTR_UNSET(value))
;
else if (!strcmp(value, "lf"))
@@ -748,9 +744,8 @@ static enum eol git_path_check_eol(struct git_attr_check_elem *check)
return EOL_UNSET;
}
-static struct convert_driver *git_path_check_convert(struct git_attr_check_elem *check)
+static struct convert_driver *git_path_check_convert(const char *value)
{
- const char *value = check->value;
struct convert_driver *drv;
if (ATTR_TRUE(value) || ATTR_FALSE(value) || ATTR_UNSET(value))
@@ -761,10 +756,8 @@ static struct convert_driver *git_path_check_convert(struct git_attr_check_elem
return NULL;
}
-static int git_path_check_ident(struct git_attr_check_elem *check)
+static int git_path_check_ident(const char *value)
{
- const char *value = check->value;
-
return !!ATTR_TRUE(value);
}
@@ -778,25 +771,29 @@ struct conv_attrs {
static void convert_attrs(struct conv_attrs *ca, const char *path)
{
static struct git_attr_check *check;
+ static int init_user_convert_tail;
- if (!check) {
- check = git_attr_check_initl("crlf", "ident",
- "filter", "eol", "text",
- NULL);
+ struct git_attr_result result = GIT_ATTR_RESULT_INIT;
+
+ git_attr_check_initl(&check, "crlf", "ident", "filter",
+ "eol", "text", NULL);
+ git_attr_result_init(&result, check);
+
+ if (!init_user_convert_tail) {
user_convert_tail = &user_convert;
git_config(read_convert_config, NULL);
+ init_user_convert_tail = 1;
}
- if (!git_check_attr(path, check)) {
- struct git_attr_check_elem *ccheck = check->check;
- ca->crlf_action = git_path_check_crlf(ccheck + 4);
+ if (!git_check_attr(path, check, &result)) {
+ ca->crlf_action = git_path_check_crlf(result.value[4]);
if (ca->crlf_action == CRLF_UNDEFINED)
- ca->crlf_action = git_path_check_crlf(ccheck + 0);
+ ca->crlf_action = git_path_check_crlf(result.value[0]);
ca->attr_action = ca->crlf_action;
- ca->ident = git_path_check_ident(ccheck + 1);
- ca->drv = git_path_check_convert(ccheck + 2);
+ ca->ident = git_path_check_ident(result.value[1]);
+ ca->drv = git_path_check_convert(result.value[2]);
if (ca->crlf_action != CRLF_BINARY) {
- enum eol eol_attr = git_path_check_eol(ccheck + 3);
+ enum eol eol_attr = git_path_check_eol(result.value[3]);
if (ca->crlf_action == CRLF_AUTO && eol_attr == EOL_LF)
ca->crlf_action = CRLF_AUTO_INPUT;
else if (ca->crlf_action == CRLF_AUTO && eol_attr == EOL_CRLF)
@@ -820,6 +817,8 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)
ca->crlf_action = CRLF_AUTO_CRLF;
if (ca->crlf_action == CRLF_UNDEFINED && auto_crlf == AUTO_CRLF_INPUT)
ca->crlf_action = CRLF_AUTO_INPUT;
+
+ git_attr_result_clear(&result);
}
int would_convert_to_git_filter_fd(const char *path)
diff --git a/ll-merge.c b/ll-merge.c
index bc6479c..3abc77a 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -355,6 +355,7 @@ int ll_merge(mmbuffer_t *result_buf,
{
static struct git_attr_check *check;
static const struct ll_merge_options default_opts;
+ static struct git_attr_result result;
const char *ll_driver_name = NULL;
int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
const struct ll_merge_driver *driver;
@@ -368,13 +369,15 @@ int ll_merge(mmbuffer_t *result_buf,
normalize_file(theirs, path);
}
- if (!check)
- check = git_attr_check_initl("merge", "conflict-marker-size", NULL);
+ if (!check) {
+ git_attr_check_initl(&check, "merge", "conflict-marker-size", NULL);
+ git_attr_result_init(&result, check);
+ }
- if (!git_check_attr(path, check)) {
- ll_driver_name = check->check[0].value;
- if (check->check[1].value) {
- marker_size = atoi(check->check[1].value);
+ if (!git_check_attr(path, check, &result)) {
+ ll_driver_name = result.value[0];
+ if (result.value[1]) {
+ marker_size = atoi(result.value[1]);
if (marker_size <= 0)
marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
}
@@ -394,14 +397,19 @@ int ll_merge(mmbuffer_t *result_buf,
int ll_merge_marker_size(const char *path)
{
static struct git_attr_check *check;
+
+ struct git_attr_result result = GIT_ATTR_RESULT_INIT;
int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
- if (!check)
- check = git_attr_check_initl("conflict-marker-size", NULL);
- if (!git_check_attr(path, check) && check->check[0].value) {
- marker_size = atoi(check->check[0].value);
+ git_attr_check_initl(&check, "conflict-marker-size", NULL);
+ git_attr_result_init(&result, check);
+
+ if (!git_check_attr(path, check, &result) && result.value[0]) {
+ marker_size = atoi(result.value[0]);
if (marker_size <= 0)
marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
}
+ git_attr_result_clear(&result);
+
return marker_size;
}
diff --git a/userdiff.c b/userdiff.c
index 46dfd32..62b1ed6 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -264,20 +264,30 @@ struct userdiff_driver *userdiff_find_by_path(const char *path)
{
static struct git_attr_check *check;
- if (!check)
- check = git_attr_check_initl("diff", NULL);
+ struct git_attr_result result = GIT_ATTR_RESULT_INIT;
+ const char *value;
+
if (!path)
return NULL;
- if (git_check_attr(path, check))
+
+ git_attr_check_initl(&check, "diff", NULL);
+ git_attr_result_init(&result, check);
+
+ if (git_check_attr(path, check, &result)) {
+ git_attr_result_clear(&result);
return NULL;
+ }
+
+ value = result.value[0];
+ git_attr_result_clear(&result);
- if (ATTR_TRUE(check->check[0].value))
+ if (ATTR_TRUE(value))
return &driver_true;
- if (ATTR_FALSE(check->check[0].value))
+ if (ATTR_FALSE(value))
return &driver_false;
- if (ATTR_UNSET(check->check[0].value))
+ if (ATTR_UNSET(value))
return NULL;
- return userdiff_find_by_name(check->check[0].value);
+ return userdiff_find_by_name(value);
}
struct userdiff_driver *userdiff_get_textconv(struct userdiff_driver *driver)
diff --git a/ws.c b/ws.c
index bb3270c..5e30fef 100644
--- a/ws.c
+++ b/ws.c
@@ -73,15 +73,20 @@ unsigned parse_whitespace_rule(const char *string)
unsigned whitespace_rule(const char *pathname)
{
- static struct git_attr_check *attr_whitespace_rule;
+ static struct git_attr_check *check;
- if (!attr_whitespace_rule)
- attr_whitespace_rule = git_attr_check_initl("whitespace", NULL);
+ struct git_attr_result result = GIT_ATTR_RESULT_INIT;
+ const char *value;
+ int r;
- if (!git_check_attr(pathname, attr_whitespace_rule)) {
- const char *value;
+ git_attr_check_initl(&check, "whitespace", NULL);
+ git_attr_result_init(&result, check);
- value = attr_whitespace_rule->check[0].value;
+ r = git_check_attr(pathname, check, &result);
+ value = result.value[0];
+ git_attr_result_clear(&result);
+
+ if (!r) {
if (ATTR_TRUE(value)) {
/* true (whitespace) */
unsigned all_rule = ws_tab_width(whitespace_rule_cfg);
--
2.10.1.432.g8a36cd8.dirty
^ permalink raw reply related
* Huge performance bottleneck reading packs
From: Vegard Nossum @ 2016-10-12 22:30 UTC (permalink / raw)
To: git
Cc: Quentin Casasnovas, Shawn Pearce, Jeff King,
Nguyễn Thái Ngọc Duy, Junio C Hamano
Hi all,
I've bisected a performance regression (noticed by Quentin and myself)
which caused a 'git fetch' to go from ~1m30s to ~2m40s:
commit 47bf4b0fc52f3ad5823185a85f5f82325787c84b
Author: Jeff King <peff@peff.net>
Date: Mon Jun 30 13:04:03 2014 -0400
prepare_packed_git_one: refactor duplicate-pack check
Reverting this commit from a recent mainline master brings the time back
down from ~2m24s to ~1m19s.
The bisect log:
v2.8.1 -- 2m41s, 2m50s (bad)
v1.9.0 -- 1m39s, 1m46s (good)
2.3.4.312.gea1fd48 -- 2m40s
2.1.0.18.gc285171 -- 2m42s
2.0.0.140.g6753d8a -- 1m27s
2.0.1.480.g60e2f5a -- 1m34s
2.0.2.631.gad25da0 -- 2m39s
2.0.1.565.ge0a064a -- 1m30s
2.0.1.622.g2e42338 -- 2m29s
2.0.0.rc1.32.g5165dd5 -- 1m30s
2.0.1.607.g5418212 -- 1m32s
2.0.1.7.g6dda4e6 -- 1m28s
2.0.1.619.g6e40947 -- 2m25s
2.0.1.9.g47bf4b0 -- 2m18s
2.0.1.8.gd6cd00c -- 1m36.542s
However, the commit found by 'git blame' above appears just fine to me,
I haven't been able to spot a bug in it.
A closer inspection reveals the problem to really be that this is an
extremely hot path with more than -- holy cow -- 4,106,756,451
iterations on the 'packed_git' list for a single 'git fetch' on my
repository. I'm guessing the patch above just made the inner loop
ever so slightly slower.
My .git/objects/pack/ has ~2088 files (1042 idx files, 1042 pack files,
and 4 tmp_pack_* files).
I am convinced that it is not necessary to rescan the entire pack
directory 11,348 times or do all 4 _BILLION_ memcmp() calls for a single
'git fetch', even for a large repository like mine.
I could try to write a patch to reduce the number of times we rescan the
pack directory. However, I've never even looked at the file before
today, so any hints regarding what would need to be done would be
appreciated.
Thanks,
(Cced some people with changes in the area.)
Vegard
^ permalink raw reply
* Re: [PATCHv2] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-12 21:50 UTC (permalink / raw)
To: Stefan Beller; +Cc: Johannes Sixt, git@vger.kernel.org, Brandon Williams
In-Reply-To: <CAGZ79kb-xKDNG-LC56i8Y-FAaYZwr8zzXuC9snj1PavnQ6cdCA@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> On Wed, Oct 12, 2016 at 2:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Johannes Sixt <j6t@kdbg.org> writes:
>>
>>> Sigh. DCLP, the Double Checked Locking Pattern. ...
>>> I suggest you go without it, then measure, and only *then* optimize if
>>> it is a bottleneck.
>>
>> That comes from me in earlier discussion before the patch, namely in
>> <xmqqeg3m8y6y.fsf@gitster.mtv.corp.google.com>, where I wondered if
>> a cheap check outside the lock may be a possible optimization
>> opportunity, as this is a classic singleton that will not be
>> deinitialized, and once the codepath gets exercised, we would be
>> taking the "nothing to do" route 100% of the time.
>
> Having followed that advice (and internally having a DCLP), I think
> we have Triple Checked Locking Pattern in this patch. Nobody wrote
> a paper on how that would not work, yet. ;)
>
> In the reroll I plan to reduce it to a Single Checked (inside a mutex)
> Locking Pattern, though I would expect that performance (or lack thereof)
> will show then. But let's postpone measuring until we have a working patch.
Oh, that wasn't even an "advice" (read it again). I fully agree
that starting simple would be the way to go.
^ permalink raw reply
* Re: [PATCHv2] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-12 20:59 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Brandon Williams
In-Reply-To: <CAGZ79kaBhXHLDEK0XMLjm3QofmtaGZspA3EEx5x4-qCYY--wZA@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
>>> Well all of the hunks in the patch are not threaded, so they
>>> don't follow a threading pattern, but the static pattern to not be
>>> more expensive than needed.
>>
>> Is it too invasive a change to make them as if they are thread-ready
>> users of API that happen to know their callers are not threading?
>> It would be ideal if we can prepare them so that the way they
>> interact with the attr subsystem will not have to change after this
>> step.
>
> As far as I see the future, we do not need to change those in the future,
> unless we add the threading to the current callers, which is usually a very
> invasive thing?
It does not matter how invasive the thread set-up and teardown that
happens in the callers.
I am talking about the part of _THIS_ code that you are updating,
that interacts with attr API. The way they prepare "check" and
"result", the way they ask questions by calling git_check_attr()
function.
Think of a thread-safe library function (like malloc()). If you
write
func (...) {
buf = malloc(20);
...
free(buf);
}
in a function that happens to be only called in a non-threaded
program today, you do not have to update these calls to malloc(3)
and free(3) when you update the callchain to threadable, right?
That kind of thread-preparedness is what I am trying to see if we
can achieve with this update.
^ permalink raw reply
* Re: [PATCHv2] attr: convert to new threadsafe API
From: Jacob Keller @ 2016-10-12 21:45 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Stefan Beller, Junio C Hamano, Git mailing list, bmwill
In-Reply-To: <44c554b8-7ac1-047d-59f0-b4d5331ed496@kdbg.org>
On Wed, Oct 12, 2016 at 1:07 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>
> Sigh. DCLP, the Double Checked Locking Pattern. These days, it should be
> common knowledge among professionals that this naïve version _does_not_work_
> [1]!
>
> I suggest you go without it, then measure, and only *then* optimize if it is
> a bottleneck. Did I read "we do not expect much contention" somewhere?
>
> [1] http://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf C++ centric,
> but applies to C just as well
>
> -- Hannes
>
You know, I always wondered why Linux Kernel code needed memory
barriers but userspace programs didn't seem to use them.. turns out
they actually *do* need them for the same exact types of problems...
Thanks,
Jake
^ permalink raw reply
* Re: [PATCHv2] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-12 21:44 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Brandon Williams
In-Reply-To: <CAGZ79kaQGs_3dhd-yEzaq=CRXGQhbMuN_7FdFY=FeKZ4scmMeQ@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> On Wed, Oct 12, 2016 at 2:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> This version adds the actual thread safety,
>>> that is promised in the documentation, however it doesn't add any optimization,
>>> i.e. it's a single global lock. But as we do not expect contention, that is fine.
>>
>> Because we have to start _somewhere_, I agree it is a good approach
>> to first try the simplest implementation and then optimize later,
>> but is it an agreed consensus that we do not expect contention?
>
> I agree on that. Did you mean this is obvious to the reader?
I meant to say that "But as we do not expect" sounded like a
justification for the approach based on an unwarranted assumption
that is not even the list concensus. I do not think it is obvious
to the reader that there is no need to worry about contention.
It all is outside the log message, so as long as readers understand
what we meant from this discussion, that is OK.
^ 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