Re: Any way to make git-log to enumerate commits?
Konstantin Khomoutov writes:
>> I do not see why the "name each rev relative to HEAD" formatting
>> option cannot produce HEAD^2~2 etc.
>> ...
> My reading was that the OP explicitly wanted to just glance at a single
> integer number and use it right away in a subsequent rebase command.
>
> I mean, use one's own short memory instead of copying and pasting.
As everybody pointed out, a single integer number will fundamentally
unworkable with distributed nature of Git that inherently makes the
history with forks and merges. Besides, there is no way to feed
"git log" with "a single integer number", without which "making
git-log to enumerate" would not be all that useful ("git show
HEAD~4" works but "git show 4" does not).
All of these name-rev based suggestions were about using anchoring
point with memorable name plus a short path to reach from there,
which I think is the closest thing to "a single integer number" and
still is usable for the exact purpose. "HEAD~48^2" on an
integration branch would be "the tip of the side branch that was
merged some 48 commits ago", for example.
Re: Any way to make git-log to enumerate commits?
On Thu, Dec 06, 2018 at 09:31:36AM +0900, Junio C Hamano wrote: > >> It would be great if git-log has a formatting option to insert an > >> index of the current commit since HEAD. > >> > >> It would allow after quitting the git-log to immediately fire up "git > >> rebase -i HEAD~index" instead of "git rebase -i > >> go-copy-paste-this-long-number-id". > > > > This may have little sense in a general case as the history maintained > > by Git is a graph, not a single line. Hence your prospective approach > > would only work for cases like `git log` called with the > > "--first-parent" command-line option. > > I do not see why the "name each rev relative to HEAD" formatting > option cannot produce HEAD^2~2 etc. > > It would be similar to "git log | git name-rev --stdin" but I do not > offhand recall if we had a way to tell name-rev to use only HEAD as > the anchoring point. My reading was that the OP explicitly wanted to just glance at a single integer number and use it right away in a subsequent rebase command. I mean, use one's own short memory instead of copying and pasting. The way I decided to format the reference in my sketch script — using HEAD~ — is just a byproduct of the fact I was aware both of the "gitrevisions" manual page and the fact `git name-rev` exists (though I regretfully was not aware it's able to process a stream of `git log`). Hence while getting fancy names for revisions would be technically correct but less error-prone for retyping from memory ;-)
Re: Any way to make git-log to enumerate commits?
Konstantin Khomoutov writes: > On Wed, Dec 05, 2018 at 05:22:14PM +0300, Konstantin Kharlamov wrote: > >> It would be great if git-log has a formatting option to insert an >> index of the current commit since HEAD. >> >> It would allow after quitting the git-log to immediately fire up "git >> rebase -i HEAD~index" instead of "git rebase -i >> go-copy-paste-this-long-number-id". > > This may have little sense in a general case as the history maintained > by Git is a graph, not a single line. Hence your prospective approach > would only work for cases like `git log` called with the > "--first-parent" command-line option. I do not see why the "name each rev relative to HEAD" formatting option cannot produce HEAD^2~2 etc. It would be similar to "git log | git name-rev --stdin" but I do not offhand recall if we had a way to tell name-rev to use only HEAD as the anchoring point.
Re: Any way to make git-log to enumerate commits?
On Dez 05 2018, Elijah Newren wrote: > Or, just use name-rev so it works with non-linear histories too: > > git log | git name-rev --refs=$(git symbolic-ref HEAD) --stdin | less That wouldn't work for a detached HEAD, though, and you need to use --no-abbrev. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different."
Re: Any way to make git-log to enumerate commits?
On Wed, Dec 5, 2018 at 6:56 AM Konstantin Khomoutov wrote:
>
> On Wed, Dec 05, 2018 at 05:22:14PM +0300, Konstantin Kharlamov wrote:
>
> > It would be great if git-log has a formatting option to insert an
> > index of the current commit since HEAD.
> >
> > It would allow after quitting the git-log to immediately fire up "git
> > rebase -i HEAD~index" instead of "git rebase -i
> > go-copy-paste-this-long-number-id".
>
> This may have little sense in a general case as the history maintained
> by Git is a graph, not a single line. Hence your prospective approach
> would only work for cases like `git log` called with the
> "--first-parent" command-line option.
>
> Still, for a simple approach you may code it right away yourself.
> Say, let's create an alias:
>
> $ git config alias.foo '!git log "$@" --pretty=oneline --source | {
> n=0;
> while read sha ref rest; do
> printf "%s\t%s~%s\t%s\n" "$sha" "$ref" $n "$rest"
> n=$((n+1))
> done
> }'
>
> Now calling `git foo --abbrev=commit` would output something like
>
> 9be8e297dHEAD~7 Frobincated fizzle
>
> where "7" is what you're looking for.
>
> A more roubst solution may need to use the `git rev-list` command.
Or, just use name-rev so it works with non-linear histories too:
git log | git name-rev --refs=$(git symbolic-ref HEAD) --stdin | less
That'll add things like "master~7" for stuff in the first parent
history and output like "master~7^2~3" for commits on side-branches,
either of which can be fed to other git commands.
Re: Any way to make git-log to enumerate commits?
On Wed, Dec 05, 2018 at 05:22:14PM +0300, Konstantin Kharlamov wrote:
> It would be great if git-log has a formatting option to insert an
> index of the current commit since HEAD.
>
> It would allow after quitting the git-log to immediately fire up "git
> rebase -i HEAD~index" instead of "git rebase -i
> go-copy-paste-this-long-number-id".
This may have little sense in a general case as the history maintained
by Git is a graph, not a single line. Hence your prospective approach
would only work for cases like `git log` called with the
"--first-parent" command-line option.
Still, for a simple approach you may code it right away yourself.
Say, let's create an alias:
$ git config alias.foo '!git log "$@" --pretty=oneline --source | {
n=0;
while read sha ref rest; do
printf "%s\t%s~%s\t%s\n" "$sha" "$ref" $n "$rest"
n=$((n+1))
done
}'
Now calling `git foo --abbrev=commit` would output something like
9be8e297dHEAD~7 Frobincated fizzle
where "7" is what you're looking for.
A more roubst solution may need to use the `git rev-list` command.
Any way to make git-log to enumerate commits?
It would be great if git-log has a formatting option to insert an index of the current commit since HEAD. It would allow after quitting the git-log to immediately fire up "git rebase -i HEAD~index" instead of "git rebase -i go-copy-paste-this-long-number-id".
[PATCH v3 04/14] checkout: make "opts" in cmd_checkout() a pointer
"opts" will soon be moved out of cmd_checkout(). To keep changes in
that patch smaller, convert "opts" to a pointer and keep the real
thing behind "real_opts".
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/checkout.c | 109 +++--
1 file changed, 55 insertions(+), 54 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 1887c996c6..1b19328d0a 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1236,76 +1236,77 @@ static int checkout_branch(struct checkout_opts *opts,
int cmd_checkout(int argc, const char **argv, const char *prefix)
{
- struct checkout_opts opts;
+ struct checkout_opts real_opts;
+ struct checkout_opts *opts = _opts;
struct branch_info new_branch_info;
char *conflict_style = NULL;
int dwim_new_local_branch = 1;
int dwim_remotes_matched = 0;
struct option options[] = {
- OPT__QUIET(, N_("suppress progress reporting")),
- OPT_STRING('b', NULL, _branch, N_("branch"),
+ OPT__QUIET(>quiet, N_("suppress progress reporting")),
+ OPT_STRING('b', NULL, >new_branch, N_("branch"),
N_("create and checkout a new branch")),
- OPT_STRING('B', NULL, _branch_force, N_("branch"),
+ OPT_STRING('B', NULL, >new_branch_force, N_("branch"),
N_("create/reset and checkout a branch")),
- OPT_BOOL('l', NULL, _branch_log, N_("create reflog for
new branch")),
- OPT_BOOL(0, "detach", _detach, N_("detach HEAD at
named commit")),
- OPT_SET_INT('t', "track", , N_("set upstream info
for new branch"),
+ OPT_BOOL('l', NULL, >new_branch_log, N_("create reflog
for new branch")),
+ OPT_BOOL(0, "detach", >force_detach, N_("detach HEAD at
named commit")),
+ OPT_SET_INT('t', "track", >track, N_("set upstream info
for new branch"),
BRANCH_TRACK_EXPLICIT),
- OPT_STRING(0, "orphan", _orphan_branch,
N_("new-branch"), N_("new unparented branch")),
- OPT_SET_INT_F('2', "ours", _stage,
+ OPT_STRING(0, "orphan", >new_orphan_branch,
N_("new-branch"), N_("new unparented branch")),
+ OPT_SET_INT_F('2', "ours", >writeout_stage,
N_("checkout our version for unmerged files"),
2, PARSE_OPT_NONEG),
- OPT_SET_INT_F('3', "theirs", _stage,
+ OPT_SET_INT_F('3', "theirs", >writeout_stage,
N_("checkout their version for unmerged files"),
3, PARSE_OPT_NONEG),
- OPT__FORCE(, N_("force checkout (throw away local
modifications)"),
+ OPT__FORCE(>force, N_("force checkout (throw away local
modifications)"),
PARSE_OPT_NOCOMPLETE),
- OPT_BOOL('m', "merge", , N_("perform a 3-way merge
with the new branch")),
- OPT_BOOL_F(0, "overwrite-ignore", _ignore,
+ OPT_BOOL('m', "merge", >merge, N_("perform a 3-way merge
with the new branch")),
+ OPT_BOOL_F(0, "overwrite-ignore", >overwrite_ignore,
N_("update ignored files (default)"),
PARSE_OPT_NOCOMPLETE),
OPT_STRING(0, "conflict", _style, N_("style"),
N_("conflict style (merge or diff3)")),
- OPT_BOOL('p', "patch", _mode, N_("select hunks
interactively")),
- OPT_BOOL(0, "ignore-skip-worktree-bits",
_skipworktree,
+ OPT_BOOL('p', "patch", >patch_mode, N_("select hunks
interactively")),
+ OPT_BOOL(0, "ignore-skip-worktree-bits",
>ignore_skipworktree,
N_("do not limit pathspecs to sparse entries only")),
OPT_HIDDEN_BOOL(0, "guess", _new_local_branch,
N_("second guess 'git checkout
'")),
- OPT_BOOL(0, "ignore-other-worktrees",
_other_worktrees,
+ OPT_BOOL(0, "ignore-other-worktrees",
>ignore_other_worktrees,
N_("do not check if another worktree is holding the
given ref")),
{ OPTION_CALLBACK, 0, "recurse-submodules", NULL,
"checkout", "control recursive updating of
submodules",
PARSE_OPT_OPTARG,
option_parse_recurse_submodules_worktree_updater },
- OPT_BOOL(0, "progress", _progress, N_("force progress
reporting")),
+ OPT_BOOL(0, "progress", >show_progress, N_("force
progress reporting")),
OPT_END(),
};
- memset(, 0, sizeof(opts));
+ memset(opts, 0, sizeof(*opts));
memset(_branch_info, 0, sizeof(new_branch_info));
- opts.overwrite_ignore = 1;
- opts.prefix = prefix;
-
[PATCH v3 13/14] restore-files: make pathspec mandatory
"git restore-files" without arguments does not make much sense when
it's about restoring files (what files now?). We could default to
either
git restore-files .
or
git restore-files :/
Neither is intuitive. Make the user always give pathspec, force the
user to think the scope of restore they want.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/checkout.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 7ff9951818..961a90b1c0 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -59,6 +59,7 @@ struct checkout_opts {
int accept_ref;
int accept_pathspec;
int switch_branch_doing_nothing_not_ok;
+ int empty_pathspec_ok;
/*
* If new checkout options are added, skip_merge_working_tree
@@ -1436,6 +1437,9 @@ static int checkout_main(int argc, const char **argv,
const char *prefix,
die(_("reference is not a tree: %s"),
opts->from_treeish);
}
+ if (opts->accept_pathspec && !opts->empty_pathspec_ok && !argc)
+ die(_("pathspec is required"));
+
if (argc) {
parse_pathspec(>pathspec, 0,
opts->patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0,
@@ -1515,6 +1519,7 @@ int cmd_checkout(int argc, const char **argv, const char
*prefix)
opts.accept_ref = 1;
opts.accept_pathspec = 1;
opts.implicit_detach = 1;
+ opts.empty_pathspec_ok = 1;
options = parse_options_dup(checkout_options);
options = add_common_options(, options);
@@ -1570,6 +1575,7 @@ int cmd_restore_files(int argc, const char **argv, const
char *prefix)
memset(, 0, sizeof(opts));
opts.accept_ref = 0;
opts.accept_pathspec = 1;
+ opts.empty_pathspec_ok = 0;
options = parse_options_dup(restore_options);
options = add_common_options(, options);
--
2.20.0.rc1.380.g3eb999425c.dirty
[PATCH 1/5] t/README: make the 'Skipping tests' section less confusing
I added this section in b5500d16cd ("t/README: Add a section about
skipping tests", 2010-07-02), but apparently didn't notice that there
was an existing "Skipping Tests" section added in
fbd458a3f6 ("t/README: Add 'Skipping Tests' section below 'Running
Tests'", 2008-06-20).
Then since 20873f45e7 ("t/README: Document the do's and don'ts of
tests", 2010-07-02) and 0445e6f0a1 ("test-lib: '--run' to run only
specific tests", 2014-04-30) we've started to refer to "Skipping
tests" or "Skipping Tests" sections in prose elsewhere.
Let's clean up this confusion by renaming the section, and while we're
at it improve the example. Usually we don't use the PERL prerequisite,
and we should accurately describe why you'd want to use prerequisites,
as opposed to GIT_SKIP_TESTS. So let's say "Tests that depend[...]"
instead of "If you need to skip tests[...]" here.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
t/README | 22 +++---
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/t/README b/t/README
index 28711cc508..b6ec28f634 100644
--- a/t/README
+++ b/t/README
@@ -494,7 +494,7 @@ And here are the "don'ts:"
The harness will catch this as a programming error of the test.
Use test_done instead if you need to stop the tests early (see
- "Skipping tests" below).
+ "Using test prerequisites" below).
- Don't use '! git cmd' when you want to make sure the git command
exits with failure in a controlled way by calling "die()". Instead,
@@ -587,28 +587,28 @@ And here are the "don'ts:"
it'll complain if anything is amiss.
-Skipping tests
---
+Using test prerequisites
+
-If you need to skip tests you should do so by using the three-arg form
-of the test_* functions (see the "Test harness library" section
-below), e.g.:
+Tests that depend on something which may not be present on the system
+should use the three-arg form of the test_* functions (see the "Test
+harness library" section below), e.g.:
-test_expect_success PERL 'I need Perl' '
-perl -e "hlagh() if unf_unf()"
+test_expect_success SYMLINKS 'setup' '
+ln -s target link
'
The advantage of skipping tests like this is that platforms that don't
-have the PERL and other optional dependencies get an indication of how
+have the SYMLINKS and other optional dependencies get an indication of how
many tests they're missing.
If the test code is too hairy for that (i.e. does a lot of setup work
outside test assertions) you can also skip all remaining tests by
setting skip_all and immediately call test_done:
- if ! test_have_prereq PERL
+ if ! test_have_prereq SYMLINKS
then
- skip_all='skipping perl interface tests, perl not available'
+ skip_all="skipping symlink tests, the filesystem doesn't support it"
test_done
fi
--
2.20.0.rc1.379.g1dd7ef354c
[PATCH v2 2/7] checkout: make "opts" in cmd_checkout() a pointer
"opts" will soon be moved out of cmd_checkout(). To keep changes in
that patch smaller, convert "opts" to a pointer and keep the real
thing behind "real_opts".
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/checkout.c | 109 +++--
1 file changed, 55 insertions(+), 54 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index acdafc6e4c..31245c1eb4 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1225,76 +1225,77 @@ static int checkout_branch(struct checkout_opts *opts,
int cmd_checkout(int argc, const char **argv, const char *prefix)
{
- struct checkout_opts opts;
+ struct checkout_opts real_opts;
+ struct checkout_opts *opts = _opts;
struct branch_info new_branch_info;
char *conflict_style = NULL;
int dwim_new_local_branch = 1;
int dwim_remotes_matched = 0;
struct option options[] = {
- OPT__QUIET(, N_("suppress progress reporting")),
- OPT_STRING('b', NULL, _branch, N_("branch"),
+ OPT__QUIET(>quiet, N_("suppress progress reporting")),
+ OPT_STRING('b', NULL, >new_branch, N_("branch"),
N_("create and checkout a new branch")),
- OPT_STRING('B', NULL, _branch_force, N_("branch"),
+ OPT_STRING('B', NULL, >new_branch_force, N_("branch"),
N_("create/reset and checkout a branch")),
- OPT_BOOL('l', NULL, _branch_log, N_("create reflog for
new branch")),
- OPT_BOOL(0, "detach", _detach, N_("detach HEAD at
named commit")),
- OPT_SET_INT('t', "track", , N_("set upstream info
for new branch"),
+ OPT_BOOL('l', NULL, >new_branch_log, N_("create reflog
for new branch")),
+ OPT_BOOL(0, "detach", >force_detach, N_("detach HEAD at
named commit")),
+ OPT_SET_INT('t', "track", >track, N_("set upstream info
for new branch"),
BRANCH_TRACK_EXPLICIT),
- OPT_STRING(0, "orphan", _orphan_branch,
N_("new-branch"), N_("new unparented branch")),
- OPT_SET_INT_F('2', "ours", _stage,
+ OPT_STRING(0, "orphan", >new_orphan_branch,
N_("new-branch"), N_("new unparented branch")),
+ OPT_SET_INT_F('2', "ours", >writeout_stage,
N_("checkout our version for unmerged files"),
2, PARSE_OPT_NONEG),
- OPT_SET_INT_F('3', "theirs", _stage,
+ OPT_SET_INT_F('3', "theirs", >writeout_stage,
N_("checkout their version for unmerged files"),
3, PARSE_OPT_NONEG),
- OPT__FORCE(, N_("force checkout (throw away local
modifications)"),
+ OPT__FORCE(>force, N_("force checkout (throw away local
modifications)"),
PARSE_OPT_NOCOMPLETE),
- OPT_BOOL('m', "merge", , N_("perform a 3-way merge
with the new branch")),
- OPT_BOOL_F(0, "overwrite-ignore", _ignore,
+ OPT_BOOL('m', "merge", >merge, N_("perform a 3-way merge
with the new branch")),
+ OPT_BOOL_F(0, "overwrite-ignore", >overwrite_ignore,
N_("update ignored files (default)"),
PARSE_OPT_NOCOMPLETE),
OPT_STRING(0, "conflict", _style, N_("style"),
N_("conflict style (merge or diff3)")),
- OPT_BOOL('p', "patch", _mode, N_("select hunks
interactively")),
- OPT_BOOL(0, "ignore-skip-worktree-bits",
_skipworktree,
+ OPT_BOOL('p', "patch", >patch_mode, N_("select hunks
interactively")),
+ OPT_BOOL(0, "ignore-skip-worktree-bits",
>ignore_skipworktree,
N_("do not limit pathspecs to sparse entries only")),
OPT_HIDDEN_BOOL(0, "guess", _new_local_branch,
N_("second guess 'git checkout
'")),
- OPT_BOOL(0, "ignore-other-worktrees",
_other_worktrees,
+ OPT_BOOL(0, "ignore-other-worktrees",
>ignore_other_worktrees,
N_("do not check if another worktree is holding the
given ref")),
{ OPTION_CALLBACK, 0, "recurse-submodules", NULL,
"checkout", "control recursive updating of
submodules",
PARSE_OPT_OPTARG,
option_parse_recurse_submodules_worktree_updater },
- OPT_BOOL(0, "progress", _progress, N_("force progress
reporting")),
+ OPT_BOOL(0, "progress", >show_progress, N_("force
progress reporting")),
OPT_END(),
};
- memset(, 0, sizeof(opts));
+ memset(opts, 0, sizeof(*opts));
memset(_branch_info, 0, sizeof(new_branch_info));
- opts.overwrite_ignore = 1;
- opts.prefix = prefix;
-
Re: [PATCH v3 3/8] refs: new ref types to make per-worktree refs visible to all worktrees
Ævar Arnfjörð Bjarmason writes: > On Sun, Oct 21 2018, Nguyễn Thái Ngọc Duy wrote: > > This change has a regression in 2.20: > >> [...] >> static void files_reflog_path(struct files_ref_store *refs, >>struct strbuf *sb, >>const char *refname) >> @@ -158,6 +178,9 @@ static void files_reflog_path(struct files_ref_store >> *refs, >> case REF_TYPE_PSEUDOREF: >> strbuf_addf(sb, "%s/logs/%s", refs->gitdir, refname); >> break; >> +case REF_TYPE_OTHER_PSEUDOREF: >> +case REF_TYPE_MAIN_PSEUDOREF: >> +return files_reflog_path_other_worktrees(refs, sb, refname); >> case REF_TYPE_NORMAL: >> strbuf_addf(sb, "%s/logs/%s", refs->gitcommondir, refname); >> break; > > SunCC on Solaris hard errors on this: > > "refs/files-backend.c", line 183: void function cannot return value > > Needs to be files...(); break; instead. True. The caller itself returns "void", so it would be nice if this were a mere warning() from practical usabliity's point of view, though ;-)
Re: [PATCH v3 3/8] refs: new ref types to make per-worktree refs visible to all worktrees
On Sun, Oct 21 2018, Nguyễn Thái Ngọc Duy wrote: This change has a regression in 2.20: > [...] > static void files_reflog_path(struct files_ref_store *refs, > struct strbuf *sb, > const char *refname) > @@ -158,6 +178,9 @@ static void files_reflog_path(struct files_ref_store > *refs, > case REF_TYPE_PSEUDOREF: > strbuf_addf(sb, "%s/logs/%s", refs->gitdir, refname); > break; > + case REF_TYPE_OTHER_PSEUDOREF: > + case REF_TYPE_MAIN_PSEUDOREF: > + return files_reflog_path_other_worktrees(refs, sb, refname); > case REF_TYPE_NORMAL: > strbuf_addf(sb, "%s/logs/%s", refs->gitcommondir, refname); > break; SunCC on Solaris hard errors on this: "refs/files-backend.c", line 183: void function cannot return value Needs to be files...(); break; instead.
[PATCH v11 18/22] stash: make push -q quiet
There is a change in behaviour with this commit. When there was
no initial commit, the shell version of stash would still display
a message. This commit makes `push` to not display any message if
`--quiet` or `-q` is specified. Add tests for `--quiet`.
Signed-off-by: Paul-Sebastian Ungureanu
---
builtin/stash--helper.c | 56 ++---
t/t3903-stash.sh| 23 +
2 files changed, 59 insertions(+), 20 deletions(-)
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 8683c662fc..0dd5dbade6 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -971,7 +971,7 @@ static int save_untracked_files(struct stash_info *info,
struct strbuf *msg,
}
static int stash_patch(struct stash_info *info, struct pathspec ps,
- struct strbuf *out_patch)
+ struct strbuf *out_patch, int quiet)
{
int ret = 0;
struct strbuf out = STRBUF_INIT;
@@ -1024,7 +1024,8 @@ static int stash_patch(struct stash_info *info, struct
pathspec ps,
}
if (!out_patch->len) {
- fprintf_ln(stderr, _("No changes selected"));
+ if (!quiet)
+ fprintf_ln(stderr, _("No changes selected"));
ret = 1;
}
@@ -1102,7 +1103,8 @@ static int stash_working_tree(struct stash_info *info,
struct pathspec ps)
static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf,
int include_untracked, int patch_mode,
- struct stash_info *info, struct strbuf *patch)
+ struct stash_info *info, struct strbuf *patch,
+ int quiet)
{
int ret = 0;
int flags = 0;
@@ -1120,7 +1122,9 @@ static int do_create_stash(struct pathspec ps, struct
strbuf *stash_msg_buf,
refresh_cache(REFRESH_QUIET);
if (get_oid("HEAD", >b_commit)) {
- fprintf_ln(stderr, _("You do not have the initial commit yet"));
+ if (!quiet)
+ fprintf_ln(stderr, _("You do not have "
+"the initial commit yet"));
ret = -1;
goto done;
} else {
@@ -1145,7 +1149,9 @@ static int do_create_stash(struct pathspec ps, struct
strbuf *stash_msg_buf,
if (write_cache_as_tree(>i_tree, 0, NULL) ||
commit_tree(commit_tree_label.buf, commit_tree_label.len,
>i_tree, parents, >i_commit, NULL, NULL)) {
- fprintf_ln(stderr, _("Cannot save the current index state"));
+ if (!quiet)
+ fprintf_ln(stderr, _("Cannot save the current "
+"index state"));
ret = -1;
goto done;
}
@@ -1153,26 +1159,29 @@ static int do_create_stash(struct pathspec ps, struct
strbuf *stash_msg_buf,
if (include_untracked && get_untracked_files(ps, include_untracked,
_files)) {
if (save_untracked_files(info, , untracked_files)) {
- fprintf_ln(stderr, _("Cannot save "
-"the untracked files"));
+ if (!quiet)
+ fprintf_ln(stderr, _("Cannot save "
+"the untracked files"));
ret = -1;
goto done;
}
untracked_commit_option = 1;
}
if (patch_mode) {
- ret = stash_patch(info, ps, patch);
+ ret = stash_patch(info, ps, patch, quiet);
if (ret < 0) {
- fprintf_ln(stderr, _("Cannot save the current "
-"worktree state"));
+ if (!quiet)
+ fprintf_ln(stderr, _("Cannot save the current "
+"worktree state"));
goto done;
} else if (ret > 0) {
goto done;
}
} else {
if (stash_working_tree(info, ps)) {
- fprintf_ln(stderr, _("Cannot save the current "
-"worktree state"));
+ if (!quiet)
+ fprintf_ln(stderr, _("Cannot save the current "
+"worktree state"));
ret = -1;
goto done;
}
@@ -1198,7 +1207,9 @@ static int do_create_stash(struct pathspec ps, struct
strbuf *stash_msg_buf,
if (commit_tree(stash_msg_buf->buf, stash_msg_buf->len, >w_tree,
parents, >w_commit, NULL, NULL)) {
-
Re: [PATCH 4/5] index: make index.threads=true enable ieot and eoie
On 11/20/2018 1:14 AM, Jonathan Nieder wrote: If a user explicitly sets [index] threads = true to read the index using multiple threads, ensure that index writes include the offset table by default to make that possible. This ensures that the user's intent of turning on threading is respected. In other words, permit the following configurations: - index.threads and index.recordOffsetTable unspecified: do not write the offset table yet (to avoid alarming the user with "ignoring IEOT extension" messages when an older version of Git accesses the repository) but do make use of multiple threads to read the index if the supporting offset table is present. This can also be requested explicitly by setting index.threads=true, 0, or >1 and index.recordOffsetTable=false. - index.threads=false or 1: do not write the offset table, and do not make use of the offset table. One can set index.recordOffsetTable=false as well, to be more explicit. - index.threads=true, 0, or >1 and index.recordOffsetTable unspecified: write the offset table and make use of threads at read time. This can also be requested by setting index.threads=true, 0, >1, or unspecified and index.recordOffsetTable=true. Fortunately the complication is temporary: once most Git installations have upgraded to a version with support for the IEOT and EOIE extensions, we can flip the defaults for index.recordEndOfIndexEntries and index.recordOffsetTable to true and eliminate the settings. This looks good. I think this provides good default behavior while enabling fine grained control to those who want/need it. I'm looking forward to the day when we can turn it back on by default so that people can take advantage of the speed improvements.
Re: [PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure
On Tue, Nov 20, 2018 at 12:25:38PM +0100, SZEDER Gábor wrote: > > but we do not > > usually bother to do so for our helper functions (like test_cmp). > > test_i18ngrep() does since your 03aa3783f2 (t: send verbose > test-helper output to fd 4, 2018-02-22). Oh, indeed. Usually I find myself forgetting about patches I worked on from 5 years ago. Forgetting about one from 9 months ago is a new low. :) > > I don't think it would ever hurt, > > though. Should we be doing that for all the others, too? > > I think we should, and you agreed: > > https://public-inbox.org/git/20180303065648.ga17...@sigill.intra.peff.net/ > > but then went travelling, and then the whole thing got forgotten. Stupid vacation. :) OK, yes, I reaffirm my agreement that this is the right direction, then. Sorry for my lack of memory. -Peff
Re: [PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure
On Mon, Nov 19, 2018 at 02:49:20PM -0500, Jeff King wrote: > On Mon, Nov 19, 2018 at 02:28:18PM +0100, SZEDER Gábor wrote: > > > The 'test_cmp_rev' helper is merely a wrapper around 'test_cmp' > > checking the output of two 'git rev-parse' commands, which means that > > its output on failure is not particularly informative, as it's > > basically two OIDs with a bit of extra clutter of the diff header, but > > without any indication of which two revisions have caused the failure: > > > > --- expect.rev 2018-11-17 14:02:11.569747033 + > > +++ actual.rev 2018-11-17 14:02:11.569747033 + > > @@ -1 +1 @@ > > -d79ce1670bdcb76e6d1da2ae095e890ccb326ae9 > > +139b20d8e6c5b496de61f033f642d0e3dbff528d > > > > It also pollutes the test repo with these two intermediate files, > > though that doesn't seem to cause any complications in our current > > tests (meaning that I couldn't find any tests that have to work around > > the presence of these files by explicitly removing or ignoring them). > > > > Enhance 'test_cmp_rev' to provide a more useful output on failure with > > less clutter: > > > > error: two revisions point to different objects: > > 'HEAD^': d79ce1670bdcb76e6d1da2ae095e890ccb326ae9 > > 'extra': 139b20d8e6c5b496de61f033f642d0e3dbff528d > > > > Doing so is more convenient when storing the OIDs outputted by 'git > > rev-parse' in a local variable each, which, as a bonus, won't pollute > > the repository with intermediate files. > > > > While at it, also ensure that 'test_cmp_rev' is invoked with the right > > number of parameters, namely two. > > This is an improvement, in my opinion (and I agree that using your new > BUG for this last part would be better still). It also saves a process > in the common case. But then it adds two subshells to the common case right away... I'm not sure which is worse on Windows, where it matters the most. > One question: > > > + else > > + local r1 r2 > > + r1=$(git rev-parse --verify "$1") && > > + r2=$(git rev-parse --verify "$2") && > > + if test "$r1" != "$r2" > > + then > > + cat >&4 <<-EOF > > + error: two revisions point to different objects: > > + '$1': $r1 > > + '$2': $r2 > > + EOF > > + return 1 > > + fi > > Why does this cat go to descriptor 4? I get why you'd want it to (it's > meant for the user's eyes, and that's where 4 goes), Exactly. > but we do not > usually bother to do so for our helper functions (like test_cmp). test_i18ngrep() does since your 03aa3783f2 (t: send verbose test-helper output to fd 4, 2018-02-22). > I don't think it matters usually in practice, because nobody tries to > capture the stderr of test_cmp, etc. See 54ce2e9be9 (t9400-git-cvsserver-server: don't rely on the output of 'test_cmp', 2018-03-08) for a fun example, I remember it caused a spike in WTF/minutes :) > I don't think it would ever hurt, > though. Should we be doing that for all the others, too? I think we should, and you agreed: https://public-inbox.org/git/20180303065648.ga17...@sigill.intra.peff.net/ but then went travelling, and then the whole thing got forgotten.
[PATCH 4/5] index: make index.threads=true enable ieot and eoie
If a user explicitly sets
[index]
threads = true
to read the index using multiple threads, ensure that index writes
include the offset table by default to make that possible. This
ensures that the user's intent of turning on threading is respected.
In other words, permit the following configurations:
- index.threads and index.recordOffsetTable unspecified: do not write
the offset table yet (to avoid alarming the user with "ignoring IEOT
extension" messages when an older version of Git accesses the
repository) but do make use of multiple threads to read the index if
the supporting offset table is present.
This can also be requested explicitly by setting index.threads=true,
0, or >1 and index.recordOffsetTable=false.
- index.threads=false or 1: do not write the offset table, and do not
make use of the offset table.
One can set index.recordOffsetTable=false as well, to be more
explicit.
- index.threads=true, 0, or >1 and index.recordOffsetTable unspecified:
write the offset table and make use of threads at read time.
This can also be requested by setting index.threads=true, 0, >1, or
unspecified and index.recordOffsetTable=true.
Fortunately the complication is temporary: once most Git installations
have upgraded to a version with support for the IEOT and EOIE
extensions, we can flip the defaults for index.recordEndOfIndexEntries
and index.recordOffsetTable to true and eliminate the settings.
Helped-by: Ben Peart
Signed-off-by: Jonathan Nieder
---
New, based on Ben Peart's feedback. Turned out simpler than I feared
--- thanks, Ben, for pushing for this.
Documentation/config/index.txt | 6 --
config.c | 17 ++---
config.h | 2 +-
read-cache.c | 23 +--
4 files changed, 32 insertions(+), 16 deletions(-)
diff --git a/Documentation/config/index.txt b/Documentation/config/index.txt
index de44183235..f181503041 100644
--- a/Documentation/config/index.txt
+++ b/Documentation/config/index.txt
@@ -3,14 +3,16 @@ index.recordEndOfIndexEntries::
Entry" section. This reduces index load time on multiprocessor
machines but produces a message "ignoring EOIE extension" when
reading the index using Git versions before 2.20. Defaults to
- 'false'.
+ 'true' if index.threads has been explicitly enabled, 'false'
+ otherwise.
index.recordOffsetTable::
Specifies whether the index file should include an "Index Entry
Offset Table" section. This reduces index load time on
multiprocessor machines but produces a message "ignoring IEOT
extension" when reading the index using Git versions before 2.20.
- Defaults to 'false'.
+ Defaults to 'true' if index.threads has been explicitly enabled,
+ 'false' otherwise.
index.threads::
Specifies the number of threads to spawn when loading the index.
diff --git a/config.c b/config.c
index 04286f7717..ff521eb27a 100644
--- a/config.c
+++ b/config.c
@@ -2294,22 +2294,25 @@ int git_config_get_fsmonitor(void)
return 0;
}
-int git_config_get_index_threads(void)
+int git_config_get_index_threads(int *dest)
{
- int is_bool, val = 0;
+ int is_bool, val;
val = git_env_ulong("GIT_TEST_INDEX_THREADS", 0);
- if (val)
- return val;
+ if (val) {
+ *dest = val;
+ return 0;
+ }
if (!git_config_get_bool_or_int("index.threads", _bool, )) {
if (is_bool)
- return val ? 0 : 1;
+ *dest = val ? 0 : 1;
else
- return val;
+ *dest = val;
+ return 0;
}
- return 0; /* auto */
+ return 1;
}
NORETURN
diff --git a/config.h b/config.h
index a06027e69b..ee5d3fa7b4 100644
--- a/config.h
+++ b/config.h
@@ -246,11 +246,11 @@ extern int git_config_get_bool(const char *key, int
*dest);
extern int git_config_get_bool_or_int(const char *key, int *is_bool, int
*dest);
extern int git_config_get_maybe_bool(const char *key, int *dest);
extern int git_config_get_pathname(const char *key, const char **dest);
+extern int git_config_get_index_threads(int *dest);
extern int git_config_get_untracked_cache(void);
extern int git_config_get_split_index(void);
extern int git_config_get_max_percent_split_change(void);
extern int git_config_get_fsmonitor(void);
-extern int git_config_get_index_threads(void);
/* This dies if the configured or default date is in the future */
extern int git_config_get_expiry(const char *key, const char **output);
diff --git a/read-cache.c b/read-cache.c
index 83d24357a6..002ed2c1e4 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2176,7 +2176,8 @@ int do_read_index(struct index_state *istate, const char
*path,
Re: [PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure
On Mon, Nov 19, 2018 at 02:28:18PM +0100, SZEDER Gábor wrote: > The 'test_cmp_rev' helper is merely a wrapper around 'test_cmp' > checking the output of two 'git rev-parse' commands, which means that > its output on failure is not particularly informative, as it's > basically two OIDs with a bit of extra clutter of the diff header, but > without any indication of which two revisions have caused the failure: > > --- expect.rev 2018-11-17 14:02:11.569747033 + > +++ actual.rev 2018-11-17 14:02:11.569747033 + > @@ -1 +1 @@ > -d79ce1670bdcb76e6d1da2ae095e890ccb326ae9 > +139b20d8e6c5b496de61f033f642d0e3dbff528d > > It also pollutes the test repo with these two intermediate files, > though that doesn't seem to cause any complications in our current > tests (meaning that I couldn't find any tests that have to work around > the presence of these files by explicitly removing or ignoring them). > > Enhance 'test_cmp_rev' to provide a more useful output on failure with > less clutter: > > error: two revisions point to different objects: > 'HEAD^': d79ce1670bdcb76e6d1da2ae095e890ccb326ae9 > 'extra': 139b20d8e6c5b496de61f033f642d0e3dbff528d > > Doing so is more convenient when storing the OIDs outputted by 'git > rev-parse' in a local variable each, which, as a bonus, won't pollute > the repository with intermediate files. > > While at it, also ensure that 'test_cmp_rev' is invoked with the right > number of parameters, namely two. This is an improvement, in my opinion (and I agree that using your new BUG for this last part would be better still). It also saves a process in the common case. One question: > + else > + local r1 r2 > + r1=$(git rev-parse --verify "$1") && > + r2=$(git rev-parse --verify "$2") && > + if test "$r1" != "$r2" > + then > + cat >&4 <<-EOF > + error: two revisions point to different objects: > + '$1': $r1 > + '$2': $r2 > + EOF > + return 1 > + fi Why does this cat go to descriptor 4? I get why you'd want it to (it's meant for the user's eyes, and that's where 4 goes), but we do not usually bother to do so for our helper functions (like test_cmp). I don't think it matters usually in practice, because nobody tries to capture the stderr of test_cmp, etc. I don't think it would ever hurt, though. Should we be doing that for all the others, too? -Peff
Re: [PATCH 0/5] Make :(attr) pathspec work with "git log"
On Mon, Nov 19, 2018 at 12:42 PM Ævar Arnfjörð Bjarmason wrote: > > > On Sun, Nov 18 2018, Nguyễn Thái Ngọc Duy wrote: > > > When :(attr) was added, it supported one of the two main pathspec > > matching functions, the one that works on a list of paths. The other > > one works on a tree, tree_entry_interesting(), which gets :(attr) > > support in this series. > > > > With this, "git grep -- :(attr)" or "git log :(attr)" > > will not abort with BUG() anymore. > > > > But this also reveals an interesting thing: even though we walk on a > > tree, we check attributes from _worktree_ (and optionally fall back to > > the index). This is how attributes are implemented since forever. I > > think this is not a big deal if we communicate clearly with the user. > > But otherwise, this series can be scraped, as reading attributes from > > a specific tree could be a lot of work. > > > > The main patch is the last one. The others are just to open a path to > > pass "struct index_state *" down to tree_entry_interesting(). This may > > become standard procedure because we don't want to stick the_index (or > > the_repository) here and there. > > Another side-note (this thread is turning into my personal blog at this > point...) I found an old related thread: > https://public-inbox.org/git/20170509225219.gb106...@google.com/ > > So this series fixes 1/2 of the issues noted there, but git-ls-tree will > still die with the same error. If you mean BUG(), not it does not. There are just a couple more places besides tree_entry_interesting() and match_pathspec() that need to be aware of all the magic things. ls-tree is one of those but it's well guarded and will display this instead $ git ls-tree HEAD ':(attr:abc=def)' fatal: :(attr:abc=def): pathspec magic not supported by this command: 'attr' If you mean making ls-tree support :(attr) and other stuff, it's not going to happen in near future. It may be nice to switch to tree_entry_interesting() in this command, but if it was easy to do I would have done it years ago :P -- Duy
Re: [PATCH 0/5] Make :(attr) pathspec work with "git log"
On Mon, Nov 19, 2018 at 12:16 PM Ævar Arnfjörð Bjarmason wrote: > As an aside, how do you do the inverse of matching for an attribute by > value? I.e.: > > $ git ls-files | wc -l; git ls-files ':(attr:diff=perl)' | wc -l > 3522 > 65 > > I'd like something gives me all files that don't match diff=perl, > i.e. 3522-65 = 3457 files, or what I'd get if I constructed such a match > manually with excludes: > > $ git ls-files $(grep diff=perl .gitattributes | cut -d ' ' -f1 | sed > 's!^!:(exclude)!') | wc -l > 3457 > > From my reading of parse_pathspec_attr_match() and match_attrs() this > isn't possible and I'd need to support ':(attr:diff!=perl)' via a new > MATCH_NOT_VALUE mode. But I wanted to make sure I wasn't missing some > subtlety, i.e. that this was implemented already via some other feature. > > I thought I could do: > > git ls-files ':(exclude):(attr:diff=perl)' > > But we don't support chaining like that, and this would only exclude a > file that's actually called ":(attr:diff=perl)". I.e. created via > something like "touch ':(attr:diff=perl)'". I think we allow :(exclude,attr:diff=perl) which should "exclude all paths that have diff=perl attribute". It's actually tested in t6135 for ls-files (but I didn't add the same test for 'git grep' because I was so confident it would work; I'll work on that). -- Duy
Re: [PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure
On Mon, Nov 19, 2018 at 02:28:18PM +0100, SZEDER Gábor wrote:
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index d158c8d0bf..fc84db67a1 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -854,9 +854,23 @@ test_must_be_empty () {
>
> # Tests that its two parameters refer to the same revision
> test_cmp_rev () {
> - git rev-parse --verify "$1" >expect.rev &&
> - git rev-parse --verify "$2" >actual.rev &&
> - test_cmp expect.rev actual.rev
> + if test $# != 2
> + then
> + error "bug in the test script: test_cmp_rev requires two
> revisions, but got $#"
And this here is another "bug in the test script" error that should be
turned into:
BUG "test_cmp_rev requires two revisions, but got $#"
See: https://public-inbox.org/git/20181119131326.2435-1-szeder@gmail.com/
[PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure
The 'test_cmp_rev' helper is merely a wrapper around 'test_cmp'
checking the output of two 'git rev-parse' commands, which means that
its output on failure is not particularly informative, as it's
basically two OIDs with a bit of extra clutter of the diff header, but
without any indication of which two revisions have caused the failure:
--- expect.rev 2018-11-17 14:02:11.569747033 +
+++ actual.rev 2018-11-17 14:02:11.569747033 +
@@ -1 +1 @@
-d79ce1670bdcb76e6d1da2ae095e890ccb326ae9
+139b20d8e6c5b496de61f033f642d0e3dbff528d
It also pollutes the test repo with these two intermediate files,
though that doesn't seem to cause any complications in our current
tests (meaning that I couldn't find any tests that have to work around
the presence of these files by explicitly removing or ignoring them).
Enhance 'test_cmp_rev' to provide a more useful output on failure with
less clutter:
error: two revisions point to different objects:
'HEAD^': d79ce1670bdcb76e6d1da2ae095e890ccb326ae9
'extra': 139b20d8e6c5b496de61f033f642d0e3dbff528d
Doing so is more convenient when storing the OIDs outputted by 'git
rev-parse' in a local variable each, which, as a bonus, won't pollute
the repository with intermediate files.
While at it, also ensure that 'test_cmp_rev' is invoked with the right
number of parameters, namely two.
Signed-off-by: SZEDER Gábor
---
t/test-lib-functions.sh | 20 +---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index d158c8d0bf..fc84db67a1 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -854,9 +854,23 @@ test_must_be_empty () {
# Tests that its two parameters refer to the same revision
test_cmp_rev () {
- git rev-parse --verify "$1" >expect.rev &&
- git rev-parse --verify "$2" >actual.rev &&
- test_cmp expect.rev actual.rev
+ if test $# != 2
+ then
+ error "bug in the test script: test_cmp_rev requires two
revisions, but got $#"
+ else
+ local r1 r2
+ r1=$(git rev-parse --verify "$1") &&
+ r2=$(git rev-parse --verify "$2") &&
+ if test "$r1" != "$r2"
+ then
+ cat >&4 <<-EOF
+ error: two revisions point to different objects:
+ '$1': $r1
+ '$2': $r2
+ EOF
+ return 1
+ fi
+ fi
}
# Print a sequence of integers in increasing order, either with
--
2.20.0.rc0.134.gf0022f8e60
Re: [PATCH 0/5] Make :(attr) pathspec work with "git log"
On Sun, Nov 18, 2018 at 08:51:52PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > But this also reveals an interesting thing: even though we walk on a
> > tree, we check attributes from _worktree_ (and optionally fall back to
> > the index). This is how attributes are implemented since forever. I
> > think this is not a big deal if we communicate clearly with the user.
> > But otherwise, this series can be scraped, as reading attributes from
> > a specific tree could be a lot of work.
>
> I'm very happy to see this implemented, and I think the behavior
> described here is the right way to go. E.g. in git.git we have diff=perl
> entries in .gitattributes. It would suck if:
>
> git log ':(attr:diff=perl)'
>
> Would only list commits as far as 20460635a8 (".gitattributes: use the
> "perl" differ for Perl", 2018-04-26), since that's when we stop having
> that attribute. Ditto for wanting to run "grep" on e.g. perl files in
> 2.12.0.
>
> I have also run into cases where I want to use a .gitattributes file
> from a specific commit. E.g. when writing pre-receive hooks where I've
> wanted the .gitattributes of the commit being pushed to configure
> something about it. But as you note this isn't supported at all.
>
> But a concern is whether we should be making :(attr:*) behave like this
> for now. Are we going to regret it later? I don't think so, I think
> wanting to use the current working tree's / index's is the most sane
> default, and if we get the ability to read it from revisions as we
> e.g. walk the log it would make most sense to just call that
> :(treeattr:*) or something like that.
I think that ship already sailed with the fact that "git log -p" will
show diffs using the worktree attrs. I agree that it would sometimes be
nice to specify attributes from a particular tree, but at this point the
default probably needs to remain as it is.
-Peff
Re: [PATCH 0/5] Make :(attr) pathspec work with "git log"
On Sun, Nov 18 2018, Nguyễn Thái Ngọc Duy wrote: > When :(attr) was added, it supported one of the two main pathspec > matching functions, the one that works on a list of paths. The other > one works on a tree, tree_entry_interesting(), which gets :(attr) > support in this series. > > With this, "git grep -- :(attr)" or "git log :(attr)" > will not abort with BUG() anymore. > > But this also reveals an interesting thing: even though we walk on a > tree, we check attributes from _worktree_ (and optionally fall back to > the index). This is how attributes are implemented since forever. I > think this is not a big deal if we communicate clearly with the user. > But otherwise, this series can be scraped, as reading attributes from > a specific tree could be a lot of work. > > The main patch is the last one. The others are just to open a path to > pass "struct index_state *" down to tree_entry_interesting(). This may > become standard procedure because we don't want to stick the_index (or > the_repository) here and there. Another side-note (this thread is turning into my personal blog at this point...) I found an old related thread: https://public-inbox.org/git/20170509225219.gb106...@google.com/ So this series fixes 1/2 of the issues noted there, but git-ls-tree will still die with the same error.
Re: [PATCH 0/5] Make :(attr) pathspec work with "git log"
On Sun, Nov 18 2018, Ævar Arnfjörð Bjarmason wrote:
> On Sun, Nov 18 2018, Nguyễn Thái Ngọc Duy wrote:
>
>> When :(attr) was added, it supported one of the two main pathspec
>> matching functions, the one that works on a list of paths. The other
>> one works on a tree, tree_entry_interesting(), which gets :(attr)
>> support in this series.
>>
>> With this, "git grep -- :(attr)" or "git log :(attr)"
>> will not abort with BUG() anymore.
>>
>> But this also reveals an interesting thing: even though we walk on a
>> tree, we check attributes from _worktree_ (and optionally fall back to
>> the index). This is how attributes are implemented since forever. I
>> think this is not a big deal if we communicate clearly with the user.
>> But otherwise, this series can be scraped, as reading attributes from
>> a specific tree could be a lot of work.
>
> I'm very happy to see this implemented, and I think the behavior
> described here is the right way to go. E.g. in git.git we have diff=perl
> entries in .gitattributes. It would suck if:
>
> git log ':(attr:diff=perl)'
>
> Would only list commits as far as 20460635a8 (".gitattributes: use the
> "perl" differ for Perl", 2018-04-26), since that's when we stop having
> that attribute. Ditto for wanting to run "grep" on e.g. perl files in
> 2.12.0.
>
> I have also run into cases where I want to use a .gitattributes file
> from a specific commit. E.g. when writing pre-receive hooks where I've
> wanted the .gitattributes of the commit being pushed to configure
> something about it. But as you note this isn't supported at all.
>
> But a concern is whether we should be making :(attr:*) behave like this
> for now. Are we going to regret it later? I don't think so, I think
> wanting to use the current working tree's / index's is the most sane
> default, and if we get the ability to read it from revisions as we
> e.g. walk the log it would make most sense to just call that
> :(treeattr:*) or something like that.
As an aside, how do you do the inverse of matching for an attribute by
value? I.e.:
$ git ls-files | wc -l; git ls-files ':(attr:diff=perl)' | wc -l
3522
65
I'd like something gives me all files that don't match diff=perl,
i.e. 3522-65 = 3457 files, or what I'd get if I constructed such a match
manually with excludes:
$ git ls-files $(grep diff=perl .gitattributes | cut -d ' ' -f1 | sed
's!^!:(exclude)!') | wc -l
3457
>From my reading of parse_pathspec_attr_match() and match_attrs() this
isn't possible and I'd need to support ':(attr:diff!=perl)' via a new
MATCH_NOT_VALUE mode. But I wanted to make sure I wasn't missing some
subtlety, i.e. that this was implemented already via some other feature.
I thought I could do:
git ls-files ':(exclude):(attr:diff=perl)'
But we don't support chaining like that, and this would only exclude a
file that's actually called ":(attr:diff=perl)". I.e. created via
something like "touch ':(attr:diff=perl)'".
Re: [PATCH 0/5] Make :(attr) pathspec work with "git log"
Nguyễn Thái Ngọc Duy writes: > But this also reveals an interesting thing: even though we walk on a > tree, we check attributes from _worktree_ (and optionally fall back to > the index). This is how attributes are implemented since forever. I > think this is not a big deal if we communicate clearly with the user. This is pretty much deliberately designed to be so; the set of the attributes in HEAD may be stale but by taking the contents from the working tree the user can work it around. > The main patch is the last one. The others are just to open a path to > pass "struct index_state *" down to tree_entry_interesting(). This may > become standard procedure because we don't want to stick the_index (or > the_repository) here and there. Yup.
Re: [PATCH 0/5] Make :(attr) pathspec work with "git log"
On Sun, Nov 18 2018, Nguyễn Thái Ngọc Duy wrote:
> When :(attr) was added, it supported one of the two main pathspec
> matching functions, the one that works on a list of paths. The other
> one works on a tree, tree_entry_interesting(), which gets :(attr)
> support in this series.
>
> With this, "git grep -- :(attr)" or "git log :(attr)"
> will not abort with BUG() anymore.
>
> But this also reveals an interesting thing: even though we walk on a
> tree, we check attributes from _worktree_ (and optionally fall back to
> the index). This is how attributes are implemented since forever. I
> think this is not a big deal if we communicate clearly with the user.
> But otherwise, this series can be scraped, as reading attributes from
> a specific tree could be a lot of work.
I'm very happy to see this implemented, and I think the behavior
described here is the right way to go. E.g. in git.git we have diff=perl
entries in .gitattributes. It would suck if:
git log ':(attr:diff=perl)'
Would only list commits as far as 20460635a8 (".gitattributes: use the
"perl" differ for Perl", 2018-04-26), since that's when we stop having
that attribute. Ditto for wanting to run "grep" on e.g. perl files in
2.12.0.
I have also run into cases where I want to use a .gitattributes file
from a specific commit. E.g. when writing pre-receive hooks where I've
wanted the .gitattributes of the commit being pushed to configure
something about it. But as you note this isn't supported at all.
But a concern is whether we should be making :(attr:*) behave like this
for now. Are we going to regret it later? I don't think so, I think
wanting to use the current working tree's / index's is the most sane
default, and if we get the ability to read it from revisions as we
e.g. walk the log it would make most sense to just call that
:(treeattr:*) or something like that.
[PATCH v3] read-cache: make the split index obey umask settings
Make the split index write out its .git/sharedindex_* files with the
same permissions as .git/index. This only changes the behavior when
core.sharedRepository isn't set, i.e. the user's umask settings will
be respected.
This hasn't been the case ever since the split index was originally
implemented in c18b80a0e8 ("update-index: new options to
enable/disable split index mode", 2014-06-13). A mkstemp()-like
function has always been used to create it. First mkstemp() itself,
and then later our own mkstemp()-like in
f6ecc62dbf ("write_shared_index(): use tempfile module", 2015-08-10)
A related bug was fixed in df801f3f9f ("read-cache: use shared perms
when writing shared index", 2017-06-25). Since then the split index
has respected core.sharedRepository.
However, using that setting should not be required simply to make git
obey the user's umask setting. It's intended for the use-case of
overriding whatever that umask is set to. This fixes cases where the
user has e.g. set his umask to 022 on a shared server in anticipation
of other user's needing to run "status", "log" etc. in his repository.
Signed-off-by: Ævar Arnfjörð Bjarmason
Signed-off-by: Christian Couder
Signed-off-by: Junio C Hamano
---
Here it is with a rewritten commit message & adjusted comment as
discussed in the v2 discussion.
read-cache.c | 3 ++-
t/t1700-split-index.sh | 20
2 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/read-cache.c b/read-cache.c
index 4ca81286c0..e7e77e780b 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -3150,7 +3150,8 @@ int write_locked_index(struct index_state *istate, struct
lock_file *lock,
struct tempfile *temp;
int saved_errno;
- temp = mks_tempfile(git_path("sharedindex_XX"));
+ /* Same initial permissions as the main .git/index file */
+ temp = mks_tempfile_sm(git_path("sharedindex_XX"), 0, 0666);
if (!temp) {
oidclr(>base_oid);
ret = do_write_locked_index(istate, lock, flags);
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 2ac47aa0e4..fa1d3d468b 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -381,6 +381,26 @@ test_expect_success 'check splitIndex.sharedIndexExpire
set to "never" and "now"
test $(ls .git/sharedindex.* | wc -l) -le 2
'
+test_expect_success POSIXPERM 'same mode for index & split index' '
+ git init same-mode &&
+ (
+ cd same-mode &&
+ test_commit A &&
+ test_modebits .git/index >index_mode &&
+ test_must_fail git config core.sharedRepository &&
+ git -c core.splitIndex=true status &&
+ shared=$(ls .git/sharedindex.*) &&
+ case "$shared" in
+ *" "*)
+ # we have more than one???
+ false ;;
+ *)
+ test_modebits "$shared" >split_index_mode &&
+ test_cmp index_mode split_index_mode ;;
+ esac
+ )
+'
+
while read -r mode modebits
do
test_expect_success POSIXPERM "split index respects
core.sharedrepository $mode" '
--
2.19.1.1182.g4ecb1133ce
[PATCH 2/5] tree-walk.c: make tree_entry_interesting() take an index
In order to support :(attr) when matching pathspec on a tree,
tree_entry_interesting() needs to take an index (because
git_check_attr() needs it). This is the preparation step for it. This
also makes it clearer what index we fall back to when looking up
attributes during an unpack-trees operation: the source index.
This also fixes revs->pruning.repo initialization that should have
been done in 2abf350385 (revision.c: remove implicit dependency on
the_index - 2018-09-21). Without it, skip_uninteresting() will
dereference a NULL pointer through this call chain
get_revision(revs)
get_revision_internal
get_revision_1
try_to_simplify_commit
rev_compare_tree
diff_tree_oid(..., >pruning)
ll_diff_tree_oid
diff_tree_paths
ll_diff_tree
skip_uninteresting
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/grep.c | 3 ++-
builtin/merge-tree.c | 2 +-
list-objects.c | 3 ++-
revision.c | 1 +
tree-diff.c | 3 ++-
tree-walk.c | 22 ++
tree-walk.h | 10 ++
tree.c | 3 ++-
unpack-trees.c | 6 +++---
9 files changed, 33 insertions(+), 20 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 56e4a11052..f6e086c287 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -568,7 +568,8 @@ static int grep_tree(struct grep_opt *opt, const struct
pathspec *pathspec,
if (match != all_entries_interesting) {
strbuf_addstr(, base->buf + tn_len);
- match = tree_entry_interesting(, ,
+ match = tree_entry_interesting(repo->index,
+ , ,
0, pathspec);
strbuf_setlen(, name_base_len);
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 70f6fc9167..4984b7e12e 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -346,7 +346,7 @@ static void merge_trees(struct tree_desc t[3], const char
*base)
setup_traverse_info(, base);
info.fn = threeway_callback;
- traverse_trees(3, t, );
+ traverse_trees(_index, 3, t, );
}
static void *get_tree_descriptor(struct tree_desc *desc, const char *rev)
diff --git a/list-objects.c b/list-objects.c
index c41cc80db5..63c395d9c2 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -113,7 +113,8 @@ static void process_tree_contents(struct traversal_context
*ctx,
while (tree_entry(, )) {
if (match != all_entries_interesting) {
- match = tree_entry_interesting(, base, 0,
+ match = tree_entry_interesting(ctx->revs->repo->index,
+ , base, 0,
>revs->diffopt.pathspec);
if (match == all_entries_not_interesting)
break;
diff --git a/revision.c b/revision.c
index bdd3e7c9f1..7ced1ee02b 100644
--- a/revision.c
+++ b/revision.c
@@ -1459,6 +1459,7 @@ void repo_init_revisions(struct repository *r,
revs->abbrev = DEFAULT_ABBREV;
revs->ignore_merges = 1;
revs->simplify_history = 1;
+ revs->pruning.repo = r;
revs->pruning.flags.recursive = 1;
revs->pruning.flags.quick = 1;
revs->pruning.add_remove = file_add_remove;
diff --git a/tree-diff.c b/tree-diff.c
index 0e54324610..34ee3b13b8 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -299,7 +299,8 @@ static void skip_uninteresting(struct tree_desc *t, struct
strbuf *base,
enum interesting match;
while (t->size) {
- match = tree_entry_interesting(>entry, base, 0,
>pathspec);
+ match = tree_entry_interesting(opt->repo->index, >entry,
+ base, 0, >pathspec);
if (match) {
if (match == all_entries_not_interesting)
t->size = 0;
diff --git a/tree-walk.c b/tree-walk.c
index 79bafbd1a2..517bcdecf9 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -365,7 +365,8 @@ static void free_extended_entry(struct tree_desc_x *t)
}
}
-static inline int prune_traversal(struct name_entry *e,
+static inline int prune_traversal(struct index_state *istate,
+ struct name_entry *e,
struct traverse_info *info,
struct strbuf *base,
int still_interesting)
@@ -374,10 +375,13 @@ static inline int prune_traversal(struct name_entry *e,
return 2;
if (still_interesting < 0)
return still_interesting;
- return tree_entry_interesting(e, base, 0, info->pathspec);
+ return tree_entry_interesting(istate, e, base,
+ 0, info->pathspec);
}
-int
[PATCH 0/5] Make :(attr) pathspec work with "git log"
When :(attr) was added, it supported one of the two main pathspec matching functions, the one that works on a list of paths. The other one works on a tree, tree_entry_interesting(), which gets :(attr) support in this series. With this, "git grep -- :(attr)" or "git log :(attr)" will not abort with BUG() anymore. But this also reveals an interesting thing: even though we walk on a tree, we check attributes from _worktree_ (and optionally fall back to the index). This is how attributes are implemented since forever. I think this is not a big deal if we communicate clearly with the user. But otherwise, this series can be scraped, as reading attributes from a specific tree could be a lot of work. The main patch is the last one. The others are just to open a path to pass "struct index_state *" down to tree_entry_interesting(). This may become standard procedure because we don't want to stick the_index (or the_repository) here and there. Nguyễn Thái Ngọc Duy (5): tree.c: make read_tree*() take 'struct repository *' tree-walk.c: make tree_entry_interesting() take an index pathspec.h: clean up "extern" in function declarations dir.c: move, rename and export match_attrs() tree-walk: support :(attr) matching Documentation/glossary-content.txt | 2 + archive.c | 6 +- builtin/checkout.c | 3 +- builtin/grep.c | 3 +- builtin/log.c | 5 +- builtin/ls-files.c | 2 +- builtin/ls-tree.c | 3 +- builtin/merge-tree.c | 2 +- dir.c | 41 +- list-objects.c | 3 +- merge-recursive.c | 3 +- pathspec.c | 38 + pathspec.h | 27 + revision.c | 1 + t/t6135-pathspec-with-attrs.sh | 58 ++- tree-diff.c| 3 +- tree-walk.c| 89 ++ tree-walk.h| 10 ++-- tree.c | 21 --- tree.h | 18 +++--- unpack-trees.c | 6 +- 21 files changed, 235 insertions(+), 109 deletions(-) -- 2.19.1.1327.g328c130451.dirty
[PATCH 1/5] tree.c: make read_tree*() take 'struct repository *'
These functions call tree_entry_interesting() which will soon require
a 'struct index_state *' to be passed in. Instead of just changing the
function signature to take an index, update to take a repo instead
because these functions do need object database access.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
archive.c | 6 --
builtin/checkout.c | 3 ++-
builtin/log.c | 5 +++--
builtin/ls-files.c | 2 +-
builtin/ls-tree.c | 3 ++-
merge-recursive.c | 3 ++-
tree.c | 18 ++
tree.h | 18 +++---
8 files changed, 35 insertions(+), 23 deletions(-)
diff --git a/archive.c b/archive.c
index fd556c28e4..bfa9cc20c9 100644
--- a/archive.c
+++ b/archive.c
@@ -285,7 +285,8 @@ int write_archive_entries(struct archiver_args *args,
git_attr_set_direction(GIT_ATTR_INDEX);
}
- err = read_tree_recursive(args->tree, "", 0, 0, >pathspec,
+ err = read_tree_recursive(args->repo, args->tree, "",
+ 0, 0, >pathspec,
queue_or_write_archive_entry,
);
if (err == READ_TREE_RECURSIVE)
@@ -346,7 +347,8 @@ static int path_exists(struct archiver_args *args, const
char *path)
ctx.args = args;
parse_pathspec(, 0, 0, "", paths);
ctx.pathspec.recursive = 1;
- ret = read_tree_recursive(args->tree, "", 0, 0, ,
+ ret = read_tree_recursive(args->repo, args->tree, "",
+ 0, 0, ,
reject_entry, );
clear_pathspec();
return ret != 0;
diff --git a/builtin/checkout.c b/builtin/checkout.c
index acdafc6e4c..c9dda8e82e 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -115,7 +115,8 @@ static int update_some(const struct object_id *oid, struct
strbuf *base,
static int read_tree_some(struct tree *tree, const struct pathspec *pathspec)
{
- read_tree_recursive(tree, "", 0, 0, pathspec, update_some, NULL);
+ read_tree_recursive(the_repository, tree, "", 0, 0,
+ pathspec, update_some, NULL);
/* update the index with the given tree's info
* for all args, expanding wildcards, and exit
diff --git a/builtin/log.c b/builtin/log.c
index 061d4fd864..d493fa915e 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -639,8 +639,9 @@ int cmd_show(int argc, const char **argv, const char
*prefix)
diff_get_color_opt(,
DIFF_COMMIT),
name,
diff_get_color_opt(,
DIFF_RESET));
- read_tree_recursive((struct tree *)o, "", 0, 0,
_all,
- show_tree_object, rev.diffopt.file);
+ read_tree_recursive(the_repository, (struct tree *)o,
"",
+ 0, 0, _all, show_tree_object,
+ rev.diffopt.file);
rev.shown_one = 1;
break;
case OBJ_COMMIT:
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 7f9919a362..8db0cccbf3 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -441,7 +441,7 @@ void overlay_tree_on_index(struct index_state *istate,
PATHSPEC_PREFER_CWD, prefix, matchbuf);
} else
memset(, 0, sizeof(pathspec));
- if (read_tree(tree, 1, , istate))
+ if (read_tree(the_repository, tree, 1, , istate))
die("unable to read tree entries %s", tree_name);
for (i = 0; i < istate->cache_nr; i++) {
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index fe3b952cb3..6855f7fea5 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -185,5 +185,6 @@ int cmd_ls_tree(int argc, const char **argv, const char
*prefix)
tree = parse_tree_indirect();
if (!tree)
die("not a tree object");
- return !!read_tree_recursive(tree, "", 0, 0, , show_tree,
NULL);
+ return !!read_tree_recursive(the_repository, tree, "", 0, 0,
+, show_tree, NULL);
}
diff --git a/merge-recursive.c b/merge-recursive.c
index acc2f64a4e..b9467f5ecf 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -463,7 +463,8 @@ static void get_files_dirs(struct merge_options *o, struct
tree *tree)
{
struct pathspec match_all;
memset(_all, 0, sizeof(match_all));
- read_tree_recursive(tree, "", 0, 0, _all, save_files_dirs, o);
+ read_tree_recursive(the_repository, tree, "", 0, 0,
+ _all, save_files_dirs, o);
}
static int get_tree_entry_if_blob(const struct object_id *tree,
diff --git a/tree.c b/tree.c
index 215d3fdc7c..ecd8c8ac29 100644
--- a/tree.c
+++ b/tree.c
@@ -60,7 +60,8 @@ static int read_one_entry_quick(const struct object_id *oid,
struct strbuf *base
[PATCH 0/1 v3] make stash work if user.name and user.email are not configured
Changes since v2: * squash patch 1/2 and patch 2/2 into a single patch * modify first part of test when there is valid ident present: create a stash, grab %an and %ae out of the resulting commit object and compare to original ident Slavica Djukic (1): stash: tolerate missing user identity git-stash.sh | 17 + t/t3903-stash.sh | 28 2 files changed, 45 insertions(+) -- 2.19.1.1052.gd166e6afe
[PATCH v2 0/2] [Outreachy] make stash work if user.name and user.email are not configured
Changes since v1: *extend test to check whether git stash executes under valid ident (and not under fallback one) when there is such present *add prepare_fallback_ident() function to git-stash.sh to provide fallback identity Slavica Djukic (2): [Outreachy] t3903-stash: test without configured user.name and user.email [Outreachy] stash: tolerate missing user identity git-stash.sh | 17 + t/t3903-stash.sh | 23 +++ 2 files changed, 40 insertions(+) -- 2.19.1.1052.gd166e6afe
[PATCH v6 06/12] t: make the sha1 test-tool helper generic
Since we're going to have multiple hash algorithms to test, it makes
sense to share as much of the test code as possible. Convert the sha1
helper for the test-tool to be generic and move it out into its own
module. This will allow us to share most of this code with our NewHash
implementation.
Signed-off-by: brian m. carlson
---
Makefile | 1 +
t/helper/{test-sha1.c => test-hash.c} | 19 +-
t/helper/test-sha1.c | 52 +--
t/helper/test-tool.h | 2 ++
4 files changed, 14 insertions(+), 60 deletions(-)
copy t/helper/{test-sha1.c => test-hash.c} (65%)
diff --git a/Makefile b/Makefile
index 016fdcdb81..daf0b198ec 100644
--- a/Makefile
+++ b/Makefile
@@ -724,6 +724,7 @@ TEST_BUILTINS_OBJS += test-dump-split-index.o
TEST_BUILTINS_OBJS += test-dump-untracked-cache.o
TEST_BUILTINS_OBJS += test-example-decorate.o
TEST_BUILTINS_OBJS += test-genrandom.o
+TEST_BUILTINS_OBJS += test-hash.o
TEST_BUILTINS_OBJS += test-hashmap.o
TEST_BUILTINS_OBJS += test-index-version.o
TEST_BUILTINS_OBJS += test-json-writer.o
diff --git a/t/helper/test-sha1.c b/t/helper/test-hash.c
similarity index 65%
copy from t/helper/test-sha1.c
copy to t/helper/test-hash.c
index 1ba0675c75..0a31de66f3 100644
--- a/t/helper/test-sha1.c
+++ b/t/helper/test-hash.c
@@ -1,13 +1,14 @@
#include "test-tool.h"
#include "cache.h"
-int cmd__sha1(int ac, const char **av)
+int cmd_hash_impl(int ac, const char **av, int algo)
{
- git_SHA_CTX ctx;
- unsigned char sha1[20];
+ git_hash_ctx ctx;
+ unsigned char hash[GIT_MAX_HEXSZ];
unsigned bufsz = 8192;
int binary = 0;
char *buffer;
+ const struct git_hash_algo *algop = _algos[algo];
if (ac == 2) {
if (!strcmp(av[1], "-b"))
@@ -26,7 +27,7 @@ int cmd__sha1(int ac, const char **av)
die("OOPS");
}
- git_SHA1_Init();
+ algop->init_fn();
while (1) {
ssize_t sz, this_sz;
@@ -38,20 +39,20 @@ int cmd__sha1(int ac, const char **av)
if (sz == 0)
break;
if (sz < 0)
- die_errno("test-sha1");
+ die_errno("test-hash");
this_sz += sz;
cp += sz;
room -= sz;
}
if (this_sz == 0)
break;
- git_SHA1_Update(, buffer, this_sz);
+ algop->update_fn(, buffer, this_sz);
}
- git_SHA1_Final(sha1, );
+ algop->final_fn(hash, );
if (binary)
- fwrite(sha1, 1, 20, stdout);
+ fwrite(hash, 1, algop->rawsz, stdout);
else
- puts(sha1_to_hex(sha1));
+ puts(hash_to_hex_algop(hash, algop));
exit(0);
}
diff --git a/t/helper/test-sha1.c b/t/helper/test-sha1.c
index 1ba0675c75..d860c387c3 100644
--- a/t/helper/test-sha1.c
+++ b/t/helper/test-sha1.c
@@ -3,55 +3,5 @@
int cmd__sha1(int ac, const char **av)
{
- git_SHA_CTX ctx;
- unsigned char sha1[20];
- unsigned bufsz = 8192;
- int binary = 0;
- char *buffer;
-
- if (ac == 2) {
- if (!strcmp(av[1], "-b"))
- binary = 1;
- else
- bufsz = strtoul(av[1], NULL, 10) * 1024 * 1024;
- }
-
- if (!bufsz)
- bufsz = 8192;
-
- while ((buffer = malloc(bufsz)) == NULL) {
- fprintf(stderr, "bufsz %u is too big, halving...\n", bufsz);
- bufsz /= 2;
- if (bufsz < 1024)
- die("OOPS");
- }
-
- git_SHA1_Init();
-
- while (1) {
- ssize_t sz, this_sz;
- char *cp = buffer;
- unsigned room = bufsz;
- this_sz = 0;
- while (room) {
- sz = xread(0, cp, room);
- if (sz == 0)
- break;
- if (sz < 0)
- die_errno("test-sha1");
- this_sz += sz;
- cp += sz;
- room -= sz;
- }
- if (this_sz == 0)
- break;
- git_SHA1_Update(, buffer, this_sz);
- }
- git_SHA1_Final(sha1, );
-
- if (binary)
- fwrite(sha1, 1, 20, stdout);
- else
- puts(sha1_to_hex(sha1));
- exit(0);
+ return cmd_hash_impl(ac, av, GIT_HASH_SHA1);
}
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 042f12464b..b5b7712a1d 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -51,4 +51,6 @@ int cmd__windows_named_pipe(int argc, const char **argv);
#endif
int cmd__write_cache(int argc, const char **argv);
[PATCH v6 04/12] cache: make hashcmp and hasheq work with larger hashes
In 183a638b7d ("hashcmp: assert constant hash size", 2018-08-23), we
modified hashcmp to assert that the hash size was always 20 to help it
optimize and inline calls to memcmp. In a future series, we replaced
many calls to hashcmp and oidcmp with calls to hasheq and oideq to
improve inlining further.
However, we want to support hash algorithms other than SHA-1, namely
SHA-256. When doing so, we must handle the case where these values are
32 bytes long as well as 20. Adjust hashcmp to handle two cases:
20-byte matches, and maximum-size matches. Therefore, when we include
SHA-256, we'll automatically handle it properly, while at the same time
teaching the compiler that there are only two possible options to
consider. This will allow the compiler to write the most efficient
possible code.
Copy similar code into hasheq and perform an identical transformation.
At least with GCC 8.2.0, making hasheq defer to hashcmp when there are
two branches prevents the compiler from inlining the comparison, while
the code in this patch is inlined properly. Add a comment to avoid an
accidental performance regression from well-intentioned refactoring.
Signed-off-by: brian m. carlson
---
cache.h | 22 --
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/cache.h b/cache.h
index cb7268deea..8607878dbc 100644
--- a/cache.h
+++ b/cache.h
@@ -1028,16 +1028,12 @@ extern const struct object_id null_oid;
static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)
{
/*
-* This is a temporary optimization hack. By asserting the size here,
-* we let the compiler know that it's always going to be 20, which lets
-* it turn this fixed-size memcmp into a few inline instructions.
-*
-* This will need to be extended or ripped out when we learn about
-* hashes of different sizes.
+* Teach the compiler that there are only two possibilities of hash size
+* here, so that it can optimize for this case as much as possible.
*/
- if (the_hash_algo->rawsz != 20)
- BUG("hash size not yet supported by hashcmp");
- return memcmp(sha1, sha2, the_hash_algo->rawsz);
+ if (the_hash_algo->rawsz == GIT_MAX_RAWSZ)
+ return memcmp(sha1, sha2, GIT_MAX_RAWSZ);
+ return memcmp(sha1, sha2, GIT_SHA1_RAWSZ);
}
static inline int oidcmp(const struct object_id *oid1, const struct object_id
*oid2)
@@ -1047,7 +1043,13 @@ static inline int oidcmp(const struct object_id *oid1,
const struct object_id *o
static inline int hasheq(const unsigned char *sha1, const unsigned char *sha2)
{
- return !hashcmp(sha1, sha2);
+ /*
+* We write this here instead of deferring to hashcmp so that the
+* compiler can properly inline it and avoid calling memcmp.
+*/
+ if (the_hash_algo->rawsz == GIT_MAX_RAWSZ)
+ return !memcmp(sha1, sha2, GIT_MAX_RAWSZ);
+ return !memcmp(sha1, sha2, GIT_SHA1_RAWSZ);
}
static inline int oideq(const struct object_id *oid1, const struct object_id
*oid2)
[PATCH 22/23] path.h: make REPO_GIT_PATH_FUNC repository agnostic
git_pathdup uses the_repository internally, but the macro
REPO_GIT_PATH_FUNC is specifically made for arbitrary repositories.
Switch to repo_git_path which works on arbitrary repositories.
Signed-off-by: Stefan Beller
---
path.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/path.h b/path.h
index b654ea8ff5..651e6157fc 100644
--- a/path.h
+++ b/path.h
@@ -165,7 +165,7 @@ extern void report_linked_checkout_garbage(void);
const char *git_path_##var(struct repository *r) \
{ \
if (!r->cached_paths.var) \
- r->cached_paths.var = git_pathdup(filename); \
+ r->cached_paths.var = repo_git_path(r, filename); \
return r->cached_paths.var; \
}
--
2.19.1.1215.g8438c0b245-goog
[PATCH v5 3/3] range-diff: make diff option behavior (e.g. --stat) consistent
Make the behavior when diff options (e.g. "--stat") are passed
consistent with how "diff" behaves.
Before 73a834e9e2 ("range-diff: relieve callers of low-level
configuration burden", 2018-07-22) running range-diff with "--stat"
would produce stat output and the diff output, as opposed to how
"diff" behaves where once "--stat" is specified "--patch" also needs
to be provided to emit the patch output.
As noted in a previous change ("range-diff doc: add a section about
output stability", 2018-11-07) the "--stat" output with "range-diff"
is useless at the moment.
But we should behave consistently with "diff" in anticipation of such
output being useful in the future, because it would make for confusing
UI if "diff" and "range-diff" behaved differently when it came to how
they interpret diff options.
The new behavior is also consistent with the existing documentation
added in ba931edd28 ("range-diff: populate the man page",
2018-08-13). See "[...]also accepts the regular diff options[...]" in
git-range-diff(1).
Signed-off-by: Ævar Arnfjörð Bjarmason
---
range-diff.c | 3 ++-
t/t3206-range-diff.sh | 23 ---
2 files changed, 2 insertions(+), 24 deletions(-)
diff --git a/range-diff.c b/range-diff.c
index ec937008d0..767af8c5bb 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -461,7 +461,8 @@ int show_range_diff(const char *range1, const char *range2,
struct strbuf indent = STRBUF_INIT;
memcpy(, diffopt, sizeof(opts));
- opts.output_format |= DIFF_FORMAT_PATCH;
+ if (!opts.output_format)
+ opts.output_format = DIFF_FORMAT_PATCH;
opts.flags.suppress_diff_headers = 1;
opts.flags.dual_color_diffed_diffs = dual_color;
opts.output_prefix = output_prefix_cb;
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index ab44e085d5..e497c1358f 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -134,43 +134,20 @@ test_expect_success 'changed commit with --no-patch diff
option' '
'
test_expect_success 'changed commit with --stat diff option' '
- four_spaces="" &&
git range-diff --no-color --stat topic...changed >actual &&
cat >expected <<-EOF &&
1: 4de457d = 1: a4b s/5/A/
a => b | 0
1 file changed, 0 insertions(+), 0 deletions(-)
- $four_spaces
2: fccce22 = 2: f51d370 s/4/A/
a => b | 0
1 file changed, 0 insertions(+), 0 deletions(-)
- $four_spaces
3: 147e64e ! 3: 0559556 s/11/B/
a => b | 0
1 file changed, 0 insertions(+), 0 deletions(-)
- $four_spaces
- @@ -10,7 +10,7 @@
- 9
- 10
--11
- -+B
- ++BB
- 12
- 13
- 14
4: a63e992 ! 4: d966c5c s/12/B/
a => b | 0
1 file changed, 0 insertions(+), 0 deletions(-)
- $four_spaces
- @@ -8,7 +8,7 @@
-@@
- 9
- 10
- - B
- + BB
--12
-+B
- 13
EOF
test_cmp expected actual
'
--
2.19.1.1182.g4ecb1133ce
Re: [PATCH v4 3/3] range-diff: make diff option behavior (e.g. --stat) consistent
Ævar Arnfjörð Bjarmason writes: > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh > index ab44e085d5..9352f65280 100755 > --- a/t/t3206-range-diff.sh > +++ b/t/t3206-range-diff.sh > @@ -140,37 +140,15 @@ test_expect_success 'changed commit with --stat diff > option' ' > 1: 4de457d = 1: a4b s/5/A/ >a => b | 0 >1 file changed, 0 insertions(+), 0 deletions(-) > - $four_spaces This removes all references to four_spaces=" " assigned at the very beginning of this test piece, so that assignment should also go, no? > 2: fccce22 = 2: f51d370 s/4/A/ >a => b | 0 >1 file changed, 0 insertions(+), 0 deletions(-) > - $four_spaces > 3: 147e64e ! 3: 0559556 s/11/B/ >a => b | 0 >1 file changed, 0 insertions(+), 0 deletions(-) > - $four_spaces > - @@ -10,7 +10,7 @@ > - 9 > - 10 > - -11 > - -+B > - ++BB > - 12 > - 13 > - 14 > 4: a63e992 ! 4: d966c5c s/12/B/ >a => b | 0 >1 file changed, 0 insertions(+), 0 deletions(-) > - $four_spaces > - @@ -8,7 +8,7 @@ > - @@ > - 9 > - 10 > - - B > - + BB > - -12 > - +B > - 13 > EOF > test_cmp expected actual > '
Re: [PATCH v4 3/3] range-diff: make diff option behavior (e.g. --stat) consistent
Eric Sunshine writes: > On Fri, Nov 9, 2018 at 5:18 AM Ævar Arnfjörð Bjarmason > wrote: >> Make the behavior when diff options (e.g. "--stat") are passed >> consistent with how "diff" behaves. >> [...] >> Signed-off-by: Ævar Arnfjörð Bjarmason >> --- >> diff --git a/range-diff.c b/range-diff.c >> @@ -453,7 +453,8 @@ int show_range_diff(const char *range1, const char >> *range2, >> - opts.output_format |= DIFF_FORMAT_PATCH; >> + if (!opts.output_format) >> + opts.output_format |= DIFF_FORMAT_PATCH; > > I think this can just be '=' now instead of '|=' (to avoid confusing > the reader, even if it's functionally equivalent). Hmph, could the condition in the future change to - if (!opts.output_format) + if (! (opts.output_format & DIFF_FORMAT_MASK)) opts.output_format |= DIFF_FORMAT_PATCH if we ever gain a new "output_format" bit that does not affect how we show the diff in a major way, and that is on by default? If so, I think "|=" is more future-proof. Otherwise, "=" is indeed more clear way to spell the intention.
Re: [PATCH v4 3/3] range-diff: make diff option behavior (e.g. --stat) consistent
On Fri, Nov 9, 2018 at 5:18 AM Ævar Arnfjörð Bjarmason wrote: > Make the behavior when diff options (e.g. "--stat") are passed > consistent with how "diff" behaves. > [...] > Signed-off-by: Ævar Arnfjörð Bjarmason > --- > diff --git a/range-diff.c b/range-diff.c > @@ -453,7 +453,8 @@ int show_range_diff(const char *range1, const char > *range2, > - opts.output_format |= DIFF_FORMAT_PATCH; > + if (!opts.output_format) > + opts.output_format |= DIFF_FORMAT_PATCH; I think this can just be '=' now instead of '|=' (to avoid confusing the reader, even if it's functionally equivalent).
Re: [PATCH v4 3/3] range-diff: make diff option behavior (e.g. --stat) consistent
On Friday, November 9, 2018 3:18:03 AM MST Ævar Arnfjörð Bjarmason wrote: > > But we should behave consistently with "diff" in anticipation of such > output being useful in the future, because it would make for confusing > UI if two "diff" and "range-diff" behaved differently when it came to 's/ two//' > how they interpret diff options. >
[PATCH v4 3/3] range-diff: make diff option behavior (e.g. --stat) consistent
Make the behavior when diff options (e.g. "--stat") are passed
consistent with how "diff" behaves.
Before 73a834e9e2 ("range-diff: relieve callers of low-level
configuration burden", 2018-07-22) running range-diff with "--stat"
would produce stat output and the diff output, as opposed to how
"diff" behaves where once "--stat" is specified "--patch" also needs
to be provided to emit the patch output.
As noted in a previous change ("range-diff doc: add a section about
output stability", 2018-11-07) the "--stat" output with "range-diff"
is useless at the moment.
But we should behave consistently with "diff" in anticipation of such
output being useful in the future, because it would make for confusing
UI if two "diff" and "range-diff" behaved differently when it came to
how they interpret diff options.
The new behavior is also consistent with the existing documentation
added in ba931edd28 ("range-diff: populate the man page",
2018-08-13). See "[...]also accepts the regular diff options[...]" in
git-range-diff(1).
Signed-off-by: Ævar Arnfjörð Bjarmason
---
range-diff.c | 3 ++-
t/t3206-range-diff.sh | 22 --
2 files changed, 2 insertions(+), 23 deletions(-)
diff --git a/range-diff.c b/range-diff.c
index ea317f92f9..72bde281f3 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -453,7 +453,8 @@ int show_range_diff(const char *range1, const char *range2,
struct strbuf indent = STRBUF_INIT;
memcpy(, diffopt, sizeof(opts));
- opts.output_format |= DIFF_FORMAT_PATCH;
+ if (!opts.output_format)
+ opts.output_format |= DIFF_FORMAT_PATCH;
opts.flags.suppress_diff_headers = 1;
opts.flags.dual_color_diffed_diffs = dual_color;
opts.output_prefix = output_prefix_cb;
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index ab44e085d5..9352f65280 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -140,37 +140,15 @@ test_expect_success 'changed commit with --stat diff
option' '
1: 4de457d = 1: a4b s/5/A/
a => b | 0
1 file changed, 0 insertions(+), 0 deletions(-)
- $four_spaces
2: fccce22 = 2: f51d370 s/4/A/
a => b | 0
1 file changed, 0 insertions(+), 0 deletions(-)
- $four_spaces
3: 147e64e ! 3: 0559556 s/11/B/
a => b | 0
1 file changed, 0 insertions(+), 0 deletions(-)
- $four_spaces
- @@ -10,7 +10,7 @@
- 9
- 10
--11
- -+B
- ++BB
- 12
- 13
- 14
4: a63e992 ! 4: d966c5c s/12/B/
a => b | 0
1 file changed, 0 insertions(+), 0 deletions(-)
- $four_spaces
- @@ -8,7 +8,7 @@
-@@
- 9
- 10
- - B
- + BB
--12
-+B
- 13
EOF
test_cmp expected actual
'
--
2.19.1.1182.g4ecb1133ce
[PATCH v3 02/16] sequencer: make the todo_list structure public
This makes the structures todo_list and todo_item, and the functions
todo_list_release() and parse_insn_buffer(), accessible outside of
sequencer.c.
Signed-off-by: Alban Gruin
---
sequencer.c | 67 +
sequencer.h | 49 +++
2 files changed, 60 insertions(+), 56 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 07cc91d8db..7adbeaa27d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1443,32 +1443,6 @@ static int allow_empty(struct replay_opts *opts, struct
commit *commit)
return 1;
}
-/*
- * Note that ordering matters in this enum. Not only must it match the mapping
- * below, it is also divided into several sections that matter. When adding
- * new commands, make sure you add it in the right section.
- */
-enum todo_command {
- /* commands that handle commits */
- TODO_PICK = 0,
- TODO_REVERT,
- TODO_EDIT,
- TODO_REWORD,
- TODO_FIXUP,
- TODO_SQUASH,
- /* commands that do something else than handling a single commit */
- TODO_EXEC,
- TODO_BREAK,
- TODO_LABEL,
- TODO_RESET,
- TODO_MERGE,
- /* commands that do nothing but are counted for reporting progress */
- TODO_NOOP,
- TODO_DROP,
- /* comments (not counted for reporting progress) */
- TODO_COMMENT
-};
-
static struct {
char c;
const char *str;
@@ -1939,26 +1913,7 @@ enum todo_item_flags {
TODO_EDIT_MERGE_MSG = 1
};
-struct todo_item {
- enum todo_command command;
- struct commit *commit;
- unsigned int flags;
- const char *arg;
- int arg_len;
- size_t offset_in_buf;
-};
-
-struct todo_list {
- struct strbuf buf;
- struct todo_item *items;
- int nr, alloc, current;
- int done_nr, total_nr;
- struct stat_data stat;
-};
-
-#define TODO_LIST_INIT { STRBUF_INIT }
-
-static void todo_list_release(struct todo_list *todo_list)
+void todo_list_release(struct todo_list *todo_list)
{
strbuf_release(_list->buf);
FREE_AND_NULL(todo_list->items);
@@ -2060,7 +2015,7 @@ static int parse_insn_line(struct todo_item *item, const
char *bol, char *eol)
return !item->commit;
}
-static int parse_insn_buffer(char *buf, struct todo_list *todo_list)
+int todo_list_parse_insn_buffer(char *buf, struct todo_list *todo_list)
{
struct todo_item *item;
char *p = buf, *next_p;
@@ -2158,7 +2113,7 @@ static int read_populate_todo(struct todo_list *todo_list,
return error(_("could not stat '%s'"), todo_file);
fill_stat_data(_list->stat, );
- res = parse_insn_buffer(todo_list->buf.buf, todo_list);
+ res = todo_list_parse_insn_buffer(todo_list->buf.buf, todo_list);
if (res) {
if (is_rebase_i(opts))
return error(_("please fix this using "
@@ -2189,7 +2144,7 @@ static int read_populate_todo(struct todo_list *todo_list,
FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w");
if (strbuf_read_file(, rebase_path_done(), 0) > 0 &&
- !parse_insn_buffer(done.buf.buf, ))
+ !todo_list_parse_insn_buffer(done.buf.buf,
))
todo_list->done_nr = count_commands();
else
todo_list->done_nr = 0;
@@ -4454,7 +4409,7 @@ int sequencer_add_exec_commands(const char *commands)
if (strbuf_read_file(_list.buf, todo_file, 0) < 0)
return error(_("could not read '%s'."), todo_file);
- if (parse_insn_buffer(todo_list.buf.buf, _list)) {
+ if (todo_list_parse_insn_buffer(todo_list.buf.buf, _list)) {
todo_list_release(_list);
return error(_("unusable todo list: '%s'"), todo_file);
}
@@ -4510,7 +4465,7 @@ int transform_todos(unsigned flags)
if (strbuf_read_file(_list.buf, todo_file, 0) < 0)
return error(_("could not read '%s'."), todo_file);
- if (parse_insn_buffer(todo_list.buf.buf, _list)) {
+ if (todo_list_parse_insn_buffer(todo_list.buf.buf, _list)) {
todo_list_release(_list);
return error(_("unusable todo list: '%s'"), todo_file);
}
@@ -4596,7 +4551,7 @@ int check_todo_list(void)
goto leave_check;
}
advise_to_edit_todo = res =
- parse_insn_buffer(todo_list.buf.buf, _list);
+ todo_list_parse_insn_buffer(todo_list.buf.buf, _list);
if (res || check_level == MISSING_COMMIT_CHECK_IGNORE)
goto leave_check;
@@ -4615,7 +4570,7 @@ int check_todo_list(void)
goto leave_check;
}
strbuf_release(_file);
- res = !!parse_insn_buffer(todo_list.buf.buf, _list);
[PATCH v3 08/16] sequencer: make sequencer_make_script() write its script to a strbuf
This makes sequencer_make_script() write its script to a strbuf (ie. the
buffer of a todo_list) instead of a FILE. This reduce the amount of
read/write made by rebase interactive.
Signed-off-by: Alban Gruin
---
builtin/rebase--interactive.c | 13 +++-
sequencer.c | 38 ---
sequencer.h | 2 +-
3 files changed, 26 insertions(+), 27 deletions(-)
diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
index a6d83a684e..c740a7dd5d 100644
--- a/builtin/rebase--interactive.c
+++ b/builtin/rebase--interactive.c
@@ -71,7 +71,8 @@ static int do_interactive_rebase(struct replay_opts *opts,
unsigned flags,
const char *head_hash = NULL;
char *revisions = NULL, *shortrevisions = NULL;
struct argv_array make_script_args = ARGV_ARRAY_INIT;
- FILE *todo_list;
+ FILE *todo_list_file;
+ struct todo_list todo_list = TODO_LIST_INIT;
if (prepare_branch_to_be_rebased(opts, switch_to))
return -1;
@@ -93,8 +94,8 @@ static int do_interactive_rebase(struct replay_opts *opts,
unsigned flags,
if (!upstream && squash_onto)
write_file(path_squash_onto(), "%s\n", squash_onto);
- todo_list = fopen(rebase_path_todo(), "w");
- if (!todo_list) {
+ todo_list_file = fopen(rebase_path_todo(), "w");
+ if (!todo_list_file) {
free(revisions);
free(shortrevisions);
@@ -105,10 +106,11 @@ static int do_interactive_rebase(struct replay_opts
*opts, unsigned flags,
if (restrict_revision)
argv_array_push(_script_args, restrict_revision);
- ret = sequencer_make_script(todo_list,
+ ret = sequencer_make_script(_list.buf,
make_script_args.argc,
make_script_args.argv,
flags);
- fclose(todo_list);
+ fputs(todo_list.buf.buf, todo_list_file);
+ fclose(todo_list_file);
if (ret)
error(_("could not generate todo list"));
@@ -120,6 +122,7 @@ static int do_interactive_rebase(struct replay_opts *opts,
unsigned flags,
free(revisions);
free(shortrevisions);
+ todo_list_release(_list);
argv_array_clear(_script_args);
return ret;
diff --git a/sequencer.c b/sequencer.c
index fce97e5f11..3389a753b6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4109,7 +4109,7 @@ static const char *label_oid(struct object_id *oid, const
char *label,
}
static int make_script_with_merges(struct pretty_print_context *pp,
- struct rev_info *revs, FILE *out,
+ struct rev_info *revs, struct strbuf *out,
unsigned flags)
{
int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
@@ -4254,7 +4254,7 @@ static int make_script_with_merges(struct
pretty_print_context *pp,
* gathering commits not yet shown, reversing the list on the fly,
* then outputting that list (labeling revisions as needed).
*/
- fprintf(out, "%s onto\n", cmd_label);
+ strbuf_addf(out, "%s onto\n", cmd_label);
for (iter = tips; iter; iter = iter->next) {
struct commit_list *list = NULL, *iter2;
@@ -4264,9 +4264,9 @@ static int make_script_with_merges(struct
pretty_print_context *pp,
entry = oidmap_get(, >object.oid);
if (entry)
- fprintf(out, "\n%c Branch %s\n", comment_line_char,
entry->string);
+ strbuf_addf(out, "\n%c Branch %s\n", comment_line_char,
entry->string);
else
- fprintf(out, "\n");
+ strbuf_addch(out, '\n');
while (oidset_contains(, >object.oid) &&
!oidset_contains(, >object.oid)) {
@@ -4279,8 +4279,8 @@ static int make_script_with_merges(struct
pretty_print_context *pp,
}
if (!commit)
- fprintf(out, "%s %s\n", cmd_reset,
- rebase_cousins ? "onto" : "[new root]");
+ strbuf_addf(out, "%s %s\n", cmd_reset,
+ rebase_cousins ? "onto" : "[new root]");
else {
const char *to = NULL;
@@ -4293,12 +4293,12 @@ static int make_script_with_merges(struct
pretty_print_context *pp,
);
if (!to || !strcmp(to, "onto"))
- fprintf(out, "%s onto\n", cmd_reset);
+ strbuf_addf(out, "%s onto\n", cmd_reset);
else {
strbuf_reset();
pretty_print_commit(pp, commit, );
- fprintf(out, "%s %s # %s\n",
-
[PATCH v4 0/2] i18n: make GETTEXT_POISON a runtime option
Addresses all the feedback against v3. Includes a patch by Junio
sitting in "pu" (and I fixed the grammar error Eric pointed out in its
commit message).
Range-diff:
1: cc24ba8de8 ! 1: 2210ee8bd9 i18n: make GETTEXT_POISON a runtime option
@@ -34,11 +34,11 @@
Notes on the implementation:
- * We still compile a dedicated GETTEXT_POISON build in Travis CI.
- This is probably the wrong thing to do and should be followed-up
- with something similar to ae59a4e44f ("travis: run tests with
- GIT_TEST_SPLIT_INDEX", 2018-01-07) to re-use an existing test setup
- for running in the GIT_TEST_GETTEXT_POISON mode.
+ * We still compile a dedicated GETTEXT_POISON build in Travis
+ CI. Perhaps this should be revisited and integrated into the
+ "linux-gcc" build, see ae59a4e44f ("travis: run tests with
+ GIT_TEST_SPLIT_INDEX", 2018-01-07) for prior art in that area. Then
+ again maybe not, see [2].
* We now skip a test in t-basic.sh under
GIT_TEST_GETTEXT_POISON=YesPlease that wasn't skipped before. This
@@ -56,12 +56,22 @@
so we populate the "poison_requested" variable in a codepath that's
won't suffer from that race condition.
-See also [3] for more on the motivation behind this patch, and the
+ * We error out in the Makefile if you're still saying
+ GETTEXT_POISON=YesPlease to prompt users to change their
+ invocation.
+
+ * We should not print out poisoned messages during the test
+ initialization itself to keep it more readable, so the test library
+ hides the variable if set in $GIT_TEST_GETTEXT_POISON_ORIG during
+ setup. See [3].
+
+See also [4] for more on the motivation behind this patch, and the
history of the GETTEXT_POISON facility.
1. https://public-inbox.org/git/871s8gd32p@evledraar.gmail.com/
-2. https://public-inbox.org/git/87woq7b58k@evledraar.gmail.com/
-3. https://public-inbox.org/git/878t2pd6yu@evledraar.gmail.com/
+2. https://public-inbox.org/git/20181102163725.gy30...@szeder.dev/
+3.
https://public-inbox.org/git/20181022202241.18629-2-szeder@gmail.com/
+4. https://public-inbox.org/git/878t2pd6yu@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason
@@ -181,7 +191,7 @@
#else
static inline void git_setup_gettext(void)
{
-+ use_gettext_poison();; /* getenv() reentrancy paranoia */
++ use_gettext_poison(); /* getenv() reentrancy paranoia */
}
static inline int gettext_width(const char *s)
{
@@ -392,8 +402,32 @@
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@
+ TZ=UTC
+ export LANG LC_ALL PAGER TZ
+ EDITOR=:
++
++# GIT_TEST_GETTEXT_POISON should not influence git commands executed
++# during initialization of test-lib and the test repo. Back it up,
++# unset and then restore after initialization is finished.
++if test -n "$GIT_TEST_GETTEXT_POISON"
++then
++ GIT_TEST_GETTEXT_POISON_ORIG=$GIT_TEST_GETTEXT_POISON
++ unset GIT_TEST_GETTEXT_POISON
++fi
++
+ # A call to "unset" with no arguments causes at least Solaris 10
+ # /usr/xpg4/bin/sh and /bin/ksh to bail out. So keep the unsets
+ # deriving from the command substitution clustered with the other
+@@
+ test -n "$USE_LIBPCRE2" && test_set_prereq LIBPCRE2
test -z "$NO_GETTEXT" && test_set_prereq GETTEXT
++if test -n "$GIT_TEST_GETTEXT_POISON_ORIG"
++then
++ GIT_TEST_GETTEXT_POISON=$GIT_TEST_GETTEXT_POISON_ORIG
++ unset GIT_TEST_GETTEXT_POISON_ORIG
++fi
++
# Can we rely on git's output in the C locale?
-if test -n "$GETTEXT_POISON"
+if test -z "$GIT_TEST_GETTEXT_POISON"
-: -- > 2: a6948d936a Makefile: ease dynamic-gettext-poison transition
Junio C Hamano (1):
Makefile: ease dynamic-gettext-poison transition
Ævar Arnfjörð Bjarmason (1):
i18n: make GETTEXT_POISON a runtime option
.travis.yml | 2 +-
Makefile | 8 +---
ci/lib-travisci.sh| 4 ++--
gettext.c | 11 +++
gettext.h | 9 +++--
git-sh-i18n.sh| 2 +-
po/README | 13 -
t/README | 6 ++
t/lib-gettext.sh | 2 +-
t/t-basic.sh | 2 +-
t/t0205-gettext-poison.sh | 8 +---
t/t3406-rebase-message.sh | 2 +-
t/t7201-co.sh | 6 +++---
t/t9902-completion.sh | 3 ++-
t/test-lib-functions.sh | 8
t/test-lib.sh | 22 +-
16 files changed, 59 insertions(+), 49 deletions(-)
--
2.19.1.930.g4563a0d9d0
[PATCH v4 1/2] i18n: make GETTEXT_POISON a runtime option
YMLINK_HEAD
endif
ifdef GETTEXT_POISON
- BASIC_CFLAGS += -DGETTEXT_POISON
+$(error The GETTEXT_POISON option has been removed in favor of runtime
GIT_TEST_GETTEXT_POISON. See t/README!)
endif
ifdef NO_GETTEXT
BASIC_CFLAGS += -DNO_GETTEXT
@@ -2603,7 +2598,6 @@ ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT
@echo GIT_TEST_CMP_USE_COPIED_CONTEXT=YesPlease >>$@+
endif
@echo NO_GETTEXT=\''$(subst ','\'',$(subst ','\'',$(NO_GETTEXT)))'\'
>>$@+
- @echo GETTEXT_POISON=\''$(subst ','\'',$(subst
','\'',$(GETTEXT_POISON)))'\' >>$@+
ifdef GIT_PERF_REPEAT_COUNT
@echo GIT_PERF_REPEAT_COUNT=\''$(subst ','\'',$(subst
','\'',$(GIT_PERF_REPEAT_COUNT)))'\' >>$@+
endif
diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
index 06970f7213..69dff4d1ec 100755
--- a/ci/lib-travisci.sh
+++ b/ci/lib-travisci.sh
@@ -123,7 +123,7 @@ osx-clang|osx-gcc)
# Travis CI OS X
export GIT_SKIP_TESTS="t9810 t9816"
;;
-GETTEXT_POISON)
- export GETTEXT_POISON=YesPlease
+GIT_TEST_GETTEXT_POISON)
+ export GIT_TEST_GETTEXT_POISON=YesPlease
;;
esac
diff --git a/gettext.c b/gettext.c
index 7272771c8e..d4021d690c 100644
--- a/gettext.c
+++ b/gettext.c
@@ -7,6 +7,7 @@
#include "gettext.h"
#include "strbuf.h"
#include "utf8.h"
+#include "config.h"
#ifndef NO_GETTEXT
# include
@@ -46,15 +47,15 @@ const char *get_preferred_languages(void)
return NULL;
}
-#ifdef GETTEXT_POISON
int use_gettext_poison(void)
{
static int poison_requested = -1;
- if (poison_requested == -1)
- poison_requested = getenv("GIT_GETTEXT_POISON") ? 1 : 0;
+ if (poison_requested == -1) {
+ const char *v = getenv("GIT_TEST_GETTEXT_POISON");
+ poison_requested = v && strlen(v) ? 1 : 0;
+ }
return poison_requested;
}
-#endif
#ifndef NO_GETTEXT
static int test_vsnprintf(const char *fmt, ...)
@@ -164,6 +165,8 @@ void git_setup_gettext(void)
if (!podir)
podir = p = system_path(GIT_LOCALE_PATH);
+ use_gettext_poison(); /* getenv() reentrancy paranoia */
+
if (!is_directory(podir)) {
free(p);
return;
diff --git a/gettext.h b/gettext.h
index 7eee64a34f..71255e503e 100644
--- a/gettext.h
+++ b/gettext.h
@@ -28,12 +28,15 @@
#define FORMAT_PRESERVING(n) __attribute__((format_arg(n)))
+extern int use_gettext_poison(void);
+
#ifndef NO_GETTEXT
extern void git_setup_gettext(void);
extern int gettext_width(const char *s);
#else
static inline void git_setup_gettext(void)
{
+ use_gettext_poison(); /* getenv() reentrancy paranoia */
}
static inline int gettext_width(const char *s)
{
@@ -41,12 +44,6 @@ static inline int gettext_width(const char *s)
}
#endif
-#ifdef GETTEXT_POISON
-extern int use_gettext_poison(void);
-#else
-#define use_gettext_poison() 0
-#endif
-
static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)
{
if (!*msgid)
diff --git a/git-sh-i18n.sh b/git-sh-i18n.sh
index 9d065fb4bf..e1d917fd27 100644
--- a/git-sh-i18n.sh
+++ b/git-sh-i18n.sh
@@ -17,7 +17,7 @@ export TEXTDOMAINDIR
# First decide what scheme to use...
GIT_INTERNAL_GETTEXT_SH_SCHEME=fallthrough
-if test -n "$GIT_GETTEXT_POISON"
+if test -n "$GIT_TEST_GETTEXT_POISON"
then
GIT_INTERNAL_GETTEXT_SH_SCHEME=poison
elif test -n "@@USE_GETTEXT_SCHEME@@"
diff --git a/po/README b/po/README
index fef4c0f0b5..aa704ffcb7 100644
--- a/po/README
+++ b/po/README
@@ -289,16 +289,11 @@ something in the test suite might still depend on the US
English
version of the strings, e.g. to grep some error message or other
output.
-To smoke out issues like these Git can be compiled with gettext poison
-support, at the top-level:
+To smoke out issues like these, Git tested with a translation mode that
+emits gibberish on every call to gettext. To use it run the test suite
+with it, e.g.:
-make GETTEXT_POISON=YesPlease
-
-That'll give you a git which emits gibberish on every call to
-gettext. It's obviously not meant to be installed, but you should run
-the test suite with it:
-
-cd t && prove -j 9 ./t[0-9]*.sh
+cd t && GIT_TEST_GETTEXT_POISON=YesPlease prove -j 9 ./t[0-9]*.sh
If tests break with it you should inspect them manually and see if
what you're translating is sane, i.e. that you're not translating
diff --git a/t/README b/t/README
index 2e9bef2852..bb2db49615 100644
--- a/t/README
+++ b/t/README
@@ -302,6 +302,12 @@ that cannot be easily covered by a few specific test
cases. These
could be enabled by running the test suite with correct GIT_TEST_
environment set.
+GIT_TEST_GETTEXT_POISON= turns all strings marked for
+translation into gibberish if non-empty (think "test -n"). Used for
+spotting those tests that need to be marked with a C_LOCALE_OUT
Re: [PATCH v3] i18n: make GETTEXT_POISON a runtime option
On Thu, Nov 08, 2018 at 09:26:19PM +0100, Ævar Arnfjörð Bjarmason wrote:
> On Fri, Nov 02 2018, SZEDER Gábor wrote:
> >> * We error out in the Makefile if you're still saying
> >>GETTEXT_POISON=YesPlease.
> >>
> >>This makes more sense than just making it a synonym since now this
> >>also needs to be defined at runtime.
> >
> > OK, I think erroring out is better than silently ignoring
> > GETTEXT_POISON=YesPlease. However, the commit message only mentions
> > that GETTEXT_POISON is gone, but perhaps this should be mentioned
> > there as well.
>
> Will mention.
With the benefit of hindsight gained from looking into a failing
GETTEXT_POISON build [1], I have to agree with Junio that flat-out
erroring out when GETTEXT_POISON=YesPlease is set is really a hassle
[2].
[1] https://public-inbox.org/git/20181107130950.ga30...@szeder.dev/
(the failure is not related to this patch)
[2] https://public-inbox.org/git/xmqqpnvg8d5z@gitster-ct.c.googlers.com/
> >> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> >> index 78d8c3783b..2f42b3653c 100644
> >> --- a/t/test-lib-functions.sh
> >> +++ b/t/test-lib-functions.sh
> >> @@ -755,16 +755,16 @@ test_cmp_bin() {
> >>
> >> # Use this instead of test_cmp to compare files that contain expected and
> >> # actual output from git commands that can be translated. When running
> >> -# under GETTEXT_POISON this pretends that the command produced expected
> >> +# under GIT_TEST_GETTEXT_POISON this pretends that the command produced
> >> expected
> >> # results.
> >> test_i18ncmp () {
> >> - test -n "$GETTEXT_POISON" || test_cmp "$@"
> >> + ! test_have_prereq C_LOCALE_OUTPUT || test_cmp "$@"
> >> }
> >
> >> test_i18ngrep () {
> >>eval "last_arg=\${$#}"
> >> @@ -779,7 +779,7 @@ test_i18ngrep () {
> >>error "bug in the test script: too few parameters to
> >> test_i18ngrep"
> >>fi
> >>
> >> - if test -n "$GETTEXT_POISON"
> >> + if test_have_prereq !C_LOCALE_OUTPUT
> >
> > Why do these two helper functions call test_have_prereq (a function
> > that doesn't even fit on my screen) instead of a simple
> >
> > test -n "$GIT_TEST_GETTEXT_POISON"
>
> I'm going to keep the call to test_have_prereq, it's consistent with
> other use of the helper in the test lib, and doesn't confuse the reader
> by giving the impression that we're in some really early setup where we
> haven't set the prereq at this point (we have).
Using the prereq in the first place is what confused (and is still
confusing...) the reader.
Re: [PATCH v3] i18n: make GETTEXT_POISON a runtime option
On Fri, Nov 02 2018, SZEDER Gábor wrote:
> On Thu, Nov 01, 2018 at 07:31:15PM +, Ævar Arnfjörð Bjarmason wrote:
>> Change the GETTEXT_POISON compile-time + runtime GIT_GETTEXT_POISON
>> test parameter to only be a GIT_TEST_GETTEXT_POISON=
>> runtime parameter, to be consistent with other parameters documented
>> in "Running tests with special setups" in t/README.
>>
>> When I added GETTEXT_POISON in bb946bba76 ("i18n: add GETTEXT_POISON
>> to simulate unfriendly translator", 2011-02-22) I was concerned with
>> ensuring that the _() function would get constant folded if NO_GETTEXT
>> was defined, and likewise that GETTEXT_POISON would be compiled out
>> unless it was defined.
>>
>> But as the benchmark in my [1] shows doing a one-off runtime
>> getenv("GIT_TEST_[...]") is trivial, and since GETTEXT_POISON was
>> originally added the GIT_TEST_* env variables have become the common
>> idiom for turning on special test setups.
>>
>> So change GETTEXT_POISON to work the same way. Now the
>> GETTEXT_POISON=YesPlease compile-time option is gone, and running the
>> tests with GIT_TEST_GETTEXT_POISON=[YesPlease|] can be toggled on/off
>> without recompiling.
>>
>> This allows for conditionally amending tests to test with/without
>> poison, similar to what 859fdc0c3c ("commit-graph: define
>> GIT_TEST_COMMIT_GRAPH", 2018-08-29) did for GIT_TEST_COMMIT_GRAPH. Do
>> some of that, now we e.g. always run the t0205-gettext-poison.sh test.
>>
>> I did enough there to remove the GETTEXT_POISON prerequisite, but its
>> inverse C_LOCALE_OUTPUT is still around, and surely some tests using
>> it can be converted to e.g. always set GIT_TEST_GETTEXT_POISON=.
>>
>> Notes on the implementation:
>>
>> * We still compile a dedicated GETTEXT_POISON build in Travis CI.
>>This is probably the wrong thing to do and should be followed-up
>>with something similar to ae59a4e44f ("travis: run tests with
>>GIT_TEST_SPLIT_INDEX", 2018-01-07) to re-use an existing test setup
>>for running in the GIT_TEST_GETTEXT_POISON mode.
>
> I'm of two minds about this. Sure, now it's not a compile time
> option, so we can spare a dedicated compilation, and sparing resources
> is good, of course.
>
> However, checking test failures in the 'linux-gcc' build job is always
> a bit of a hassle, because it's not enough to look at the output of
> the failed test, but I also have to check which one of the two test
> runs failed (i.e. the "regular" or one with all the GIT_TEST_* knobs
> turned on). Adding a second test run with GIT_TEST_GETTEXT_POISON
> enabled to an other build job (e.g. 'linux-clang') would then bring
> this hassle to that build job as well.
>
> Furthermore, occasionally a build job is slower than usual (whatever
> the reason might be), which can be an issue when running the test
> suite twice, as it can exceed the time limit.
>
> Anyway, we can think about this later.
>
> In any case, GIT_TEST_GETTEXT_POISON definitely should not be enabled
> in the same "catch-all" test run with all other GIT_TEST_* variables,
> because it would mess up any translated error messages that might pop
> up in a test failure, and then we wouldn't have any idea about what
> went wrong.
Will clarify this in v3. I.e. "let's think about this...".
>> * We now skip a test in t-basic.sh under
>>GIT_TEST_GETTEXT_POISON=YesPlease that wasn't skipped before. This
>>test relies on C locale output, but due to an edge case in how the
>>previous implementation of GETTEXT_POISON worked (reading it from
>>GIT-BUILD-OPTIONS) wasn't enabling poison correctly. Now it does,
>>and needs to be skipped.
>>
>> * The getenv() function is not reentrant, so out of paranoia about
>>code of the form:
>>
>>printf(_("%s"), getenv("some-env"));
>>
>>call use_gettext_poison() in our early setup in git_setup_gettext()
>>so we populate the "poison_requested" variable in a codepath that's
>>won't suffer from that race condition.
>>
>> See also [3] for more on the motivation behind this patch, and the
>> history of the GETTEXT_POISON facility.
>>
>> 1. https://public-inbox.org/git/871s8gd32p@evledraar.gmail.com/
>> 2. https://public-inbox.org/git/87woq7b58k@evledraar.gmail.com/
>> 3. https://public-inbox.org/git/878t2pd6yu@evledraar.gmail.com/
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason
>> ---
>>
>> Now:
>>
>> * The new i18n helper is gone. We just use "test -n" semantics for
>>$GIT_TEST_GETTEXT_POISON
>>
>> * We error out in the Makefile if you're still saying
>>GETTEXT_POISON=YesPlease.
>>
>>This makes more sense than just making it a synonym since now this
>>also needs to be defined at runtime.
>
> OK, I think erroring out is better than silently ignoring
> GETTEXT_POISON=YesPlease. However, the commit message only mentions
> that GETTEXT_POISON is gone, but perhaps this should be mentioned
> there as well.
Will mention.
>> * The caveat with avoiding test_lazy_prereq is gone (although
Re: [PATCH v3] i18n: make GETTEXT_POISON a runtime option
On Wed, Nov 7, 2018 at 10:24 PM Junio C Hamano wrote: > Makefile: ease dynamic-gettext-poison transition > > Earlier we made the entire build to fail when GETTEXT_POISON=Yes is > given to make, to notify those who did not notice that text poisoning > is now a runtime behaviour. > > It turns out that this too irritating for those who need to build s/too/is &/ > and test different versions of Git that cross the boundary between > history with and without this topic to switch between two > environment variables. Demote the error to a warning, so that you > can say something like > > make GETTEXT_POISON=Yes GIT_TEST_GETTEXT_POISON test > > during the transition period, without having to worry about whether > exact version you are testing has or does not have this topic. > > Signed-off-by: Junio C Hamano
Re: [PATCH] branch: make --show-current use already resolved HEAD
I did something that resulted in the mailing list not being cc'd. Apologies to Junio and Daniels for the double send. :( On Thu, Nov 08, 2018 at 10:11:02AM +0900, Junio C Hamano wrote: > I'd prefer to see scriptors avoid using "git branch", too. > > Unlike end-user facing documentation where we promise "we do X and > will continue to do so because of Y" to the readers, the log message > is primarily for recording the original motivation of the change, so > that we can later learn "we did X back then because we thought Y". > When we want to revise X, we revisit if the reason Y is still valid. > > So in that sense, the door to "break" the scriptability is still > open. > Over at #git, commit messages are sometimes consulted to disambiguate or clarify certain details. Often the documentation is correct but people dispute over interpretations. If someone came asking if `git branch` is parsable, I would advise against and direct them to the plumbing or format alternative. But if someone came over with a link to this commit asking the same question, I suspect the answer would be: it's probably safe to parse the output of this specific option because the commit says so. Thanks for clarifying this is wrong. > > > > static const char *head; > > static struct object_id head_oid; > > +static int head_flags = 0; > > You've eliminated the "now unnecessary" helper and do everything > inside cmd_branch(), so perhaps this can be made function local, no? > I was not sure if these 3 lines were global intentionally or if it was just an artifact from the past. Since it looks like the latter, I'll make them local. -- Rafael Ascensão
Re: [PATCH v3] i18n: make GETTEXT_POISON a runtime option
Ævar Arnfjörð Bjarmason writes: > * We error out in the Makefile if you're still saying >GETTEXT_POISON=YesPlease. I expected this would be irritating, but it turns out it is worse than mere irritation but is a severe hinderance to affect my performance, as I (and my bots) keep building and testing different versions of Git under different configurations. I know it was done to help those who only ever build a single track at a time and mostly moving forward, but I'm very much tempted to remove this part, perhaps demote it to a warning of some sort. > ifdef GETTEXT_POISON > - BASIC_CFLAGS += -DGETTEXT_POISON > +$(error The GETTEXT_POISON option has been removed in favor of runtime > GIT_TEST_GETTEXT_POISON. See t/README!) > endif -- >8 -- Makefile: ease dynamic-gettext-poison transition Earlier we made the entire build to fail when GETTEXT_POISON=Yes is given to make, to notify those who did not notice that text poisoning is now a runtime behaviour. It turns out that this too irritating for those who need to build and test different versions of Git that cross the boundary between history with and without this topic to switch between two environment variables. Demote the error to a warning, so that you can say something like make GETTEXT_POISON=Yes GIT_TEST_GETTEXT_POISON test during the transition period, without having to worry about whether exact version you are testing has or does not have this topic. Signed-off-by: Junio C Hamano --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index f3a9995e50..6b492f44a6 100644 --- a/Makefile +++ b/Makefile @@ -1447,7 +1447,7 @@ ifdef NO_SYMLINK_HEAD BASIC_CFLAGS += -DNO_SYMLINK_HEAD endif ifdef GETTEXT_POISON -$(error The GETTEXT_POISON option has been removed in favor of runtime GIT_TEST_GETTEXT_POISON. See t/README!) +$(warning The GETTEXT_POISON option has been removed in favor of runtime GIT_TEST_GETTEXT_POISON. See t/README!) endif ifdef NO_GETTEXT BASIC_CFLAGS += -DNO_GETTEXT
Re: [PATCH] branch: make --show-current use already resolved HEAD
Rafael Ascensão writes:
> print_current_branch_name() tries to resolve HEAD and die() when it
> doesn't resolve it successfully. But the conditions being tested are
> always unreachable because early in branch:cmd_branch() the same logic
> is performed.
>
> Eliminate the duplicate and unreachable code, and update the current
> logic to the more reliable check for the detached head.
Nice.
> I still think the mention about scripting should be removed from the
> original commit message, leaving it open to being taught other tricks
> like --verbose that aren't necessarily script-friendly.
I'd prefer to see scriptors avoid using "git branch", too.
Unlike end-user facing documentation where we promise "we do X and
will continue to do so because of Y" to the readers, the log message
is primarily for recording the original motivation of the change, so
that we can later learn "we did X back then because we thought Y".
When we want to revise X, we revisit if the reason Y is still valid.
So in that sense, the door to "break" the scriptability is still
open.
> But the main goal of this patch is to just bring some attention to this,
> as I mentioned it in a previous thread but it got lost.
This idea of yours seems to lead to a better implementation, and
indeed "got lost" is a good way to describe what happened---I do not
recall seeing it, for example. Thanks for bringing it back.
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 46f91dc06d..1c51d0a8ca 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -38,6 +38,7 @@ static const char * const builtin_branch_usage[] = {
>
> static const char *head;
> static struct object_id head_oid;
> +static int head_flags = 0;
You've eliminated the "now unnecessary" helper and do everything
inside cmd_branch(), so perhaps this can be made function local, no?
> @@ -668,10 +654,10 @@ int cmd_branch(int argc, const char **argv, const char
> *prefix)
>
> track = git_branch_track;
>
> - head = resolve_refdup("HEAD", 0, _oid, NULL);
> + head = resolve_refdup("HEAD", 0, _oid, _flags);
> if (!head)
> die(_("Failed to resolve HEAD as a valid ref."));
> - if (!strcmp(head, "HEAD"))
> + if (!(head_flags & REF_ISSYMREF))
> filter.detached = 1;
Nice to see we can reuse the resolve_refdup() we already have.
> else if (!skip_prefix(head, "refs/heads/", ))
> die(_("HEAD not found below refs/heads!"));
> @@ -716,7 +702,8 @@ int cmd_branch(int argc, const char **argv, const char
> *prefix)
> die(_("branch name required"));
> return delete_branches(argc, argv, delete > 1, filter.kind,
> quiet);
> } else if (show_current) {
> - print_current_branch_name();
> + if (!filter.detached)
> + puts(head);
Ah, I wondered why we do not have to skip-prefix, but it is already
done for us when we validated that an attached HEAD points at a
local branch. Good.
> return 0;
> } else if (list) {
> /* git branch --local also shows HEAD when it is detached */
[PATCH] branch: make --show-current use already resolved HEAD
print_current_branch_name() tries to resolve HEAD and die() when it
doesn't resolve it successfully. But the conditions being tested are
always unreachable because early in branch:cmd_branch() the same logic
is performed.
Eliminate the duplicate and unreachable code, and update the current
logic to the more reliable check for the detached head.
Signed-off-by: Rafael Ascensão
---
This patch is meant to be either applied or squashed on top of the
current series.
I am basing the claims of it being more reliable of what Junio suggested
on a previous iteration of this series:
https://public-inbox.org/git/xmqq4ldtgehs@gitster-ct.c.googlers.com/
But the main goal of this patch is to just bring some attention to this,
as I mentioned it in a previous thread but it got lost. After asking on
#git-devel, the suggestion was to send it as an incremental patch. So
here it is. :)
I still think the mention about scripting should be removed from the
original commit message, leaving it open to being taught other tricks
like --verbose that aren't necessarily script-friendly.
Cheers
builtin/branch.c | 23 +--
1 file changed, 5 insertions(+), 18 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 46f91dc06d..1c51d0a8ca 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -38,6 +38,7 @@ static const char * const builtin_branch_usage[] = {
static const char *head;
static struct object_id head_oid;
+static int head_flags = 0;
static int branch_use_color = -1;
static char branch_colors[][COLOR_MAXLEN] = {
@@ -443,21 +444,6 @@ static void print_ref_list(struct ref_filter *filter,
struct ref_sorting *sortin
free(to_free);
}
-static void print_current_branch_name(void)
-{
- int flags;
- const char *refname = resolve_ref_unsafe("HEAD", 0, NULL, );
- const char *shortname;
- if (!refname)
- die(_("could not resolve HEAD"));
- else if (!(flags & REF_ISSYMREF))
- return;
- else if (skip_prefix(refname, "refs/heads/", ))
- puts(shortname);
- else
- die(_("HEAD (%s) points outside of refs/heads/"), refname);
-}
-
static void reject_rebase_or_bisect_branch(const char *target)
{
struct worktree **worktrees = get_worktrees(0);
@@ -668,10 +654,10 @@ int cmd_branch(int argc, const char **argv, const char
*prefix)
track = git_branch_track;
- head = resolve_refdup("HEAD", 0, _oid, NULL);
+ head = resolve_refdup("HEAD", 0, _oid, _flags);
if (!head)
die(_("Failed to resolve HEAD as a valid ref."));
- if (!strcmp(head, "HEAD"))
+ if (!(head_flags & REF_ISSYMREF))
filter.detached = 1;
else if (!skip_prefix(head, "refs/heads/", ))
die(_("HEAD not found below refs/heads!"));
@@ -716,7 +702,8 @@ int cmd_branch(int argc, const char **argv, const char
*prefix)
die(_("branch name required"));
return delete_branches(argc, argv, delete > 1, filter.kind,
quiet);
} else if (show_current) {
- print_current_branch_name();
+ if (!filter.detached)
+ puts(head);
return 0;
} else if (list) {
/* git branch --local also shows HEAD when it is detached */
--
2.19.1
Re: [PATCH] multi-pack-index: make code -Wunused-parameter clean
On 11/3/2018 10:27 PM, Jeff King wrote:
On Sat, Nov 03, 2018 at 05:49:57PM -0700, Carlo Marcelo Arenas Belón wrote:
introduced in 662148c435 ("midx: write object offsets", 2018-07-12)
but included on all previous versions as well.
midx.c:713:54: warning: unused parameter 'nr_objects' [-Wunused-parameter]
likely an oversight as the information needed to iterate over is
embedded in nr_large_offset
I've been preparing a series to make the whole code base compile with
-Wunused-parameter, and I handled this case a bit differently.
-- >8 --
Subject: [PATCH] midx: double-check large object write loop
The write_midx_large_offsets() function takes an array of object
entries, the number of entries in the array (nr_objects), and the number
of entries with large offsets (nr_large_offset). But we never actually
use nr_objects; instead we keep walking down the array and counting down
nr_large_offset until we've seen all of the large entries.
This is correct, but we can be a bit more defensive. If there were ever
a mismatch between nr_large_offset and the actual set of large-offset
objects, we'd walk off the end of the array.
Since we know the size of the array, we can use nr_objects to make sure
we don't walk too far.
Signed-off-by: Jeff King
Thanks, both, for catching this. I prefer the approach that adds defenses.
Reviewed-by: Derrick Stolee
---
midx.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/midx.c b/midx.c
index 4fac0cd08a..ecd583666a 100644
--- a/midx.c
+++ b/midx.c
@@ -712,12 +712,18 @@ static size_t write_midx_object_offsets(struct hashfile
*f, int large_offset_nee
static size_t write_midx_large_offsets(struct hashfile *f, uint32_t
nr_large_offset,
struct pack_midx_entry *objects,
uint32_t nr_objects)
{
- struct pack_midx_entry *list = objects;
+ struct pack_midx_entry *list = objects, *end = objects + nr_objects;
size_t written = 0;
while (nr_large_offset) {
- struct pack_midx_entry *obj = list++;
- uint64_t offset = obj->offset;
+ struct pack_midx_entry *obj;
+ uint64_t offset;
+
+ if (list >= end)
+ BUG("too many large-offset objects");
+
+ obj = list++;
+ offset = obj->offset;
if (!(offset >> 31))
continue;
[PATCH v5 06/12] t: make the sha1 test-tool helper generic
Since we're going to have multiple hash algorithms to test, it makes
sense to share as much of the test code as possible. Convert the sha1
helper for the test-tool to be generic and move it out into its own
module. This will allow us to share most of this code with our NewHash
implementation.
Signed-off-by: brian m. carlson
---
Makefile | 1 +
t/helper/{test-sha1.c => test-hash.c} | 19 +-
t/helper/test-sha1.c | 52 +--
t/helper/test-tool.h | 2 ++
4 files changed, 14 insertions(+), 60 deletions(-)
copy t/helper/{test-sha1.c => test-hash.c} (65%)
diff --git a/Makefile b/Makefile
index d18ab0fe78..81dc9ac819 100644
--- a/Makefile
+++ b/Makefile
@@ -714,6 +714,7 @@ TEST_BUILTINS_OBJS += test-dump-split-index.o
TEST_BUILTINS_OBJS += test-dump-untracked-cache.o
TEST_BUILTINS_OBJS += test-example-decorate.o
TEST_BUILTINS_OBJS += test-genrandom.o
+TEST_BUILTINS_OBJS += test-hash.o
TEST_BUILTINS_OBJS += test-hashmap.o
TEST_BUILTINS_OBJS += test-index-version.o
TEST_BUILTINS_OBJS += test-json-writer.o
diff --git a/t/helper/test-sha1.c b/t/helper/test-hash.c
similarity index 65%
copy from t/helper/test-sha1.c
copy to t/helper/test-hash.c
index 1ba0675c75..0a31de66f3 100644
--- a/t/helper/test-sha1.c
+++ b/t/helper/test-hash.c
@@ -1,13 +1,14 @@
#include "test-tool.h"
#include "cache.h"
-int cmd__sha1(int ac, const char **av)
+int cmd_hash_impl(int ac, const char **av, int algo)
{
- git_SHA_CTX ctx;
- unsigned char sha1[20];
+ git_hash_ctx ctx;
+ unsigned char hash[GIT_MAX_HEXSZ];
unsigned bufsz = 8192;
int binary = 0;
char *buffer;
+ const struct git_hash_algo *algop = _algos[algo];
if (ac == 2) {
if (!strcmp(av[1], "-b"))
@@ -26,7 +27,7 @@ int cmd__sha1(int ac, const char **av)
die("OOPS");
}
- git_SHA1_Init();
+ algop->init_fn();
while (1) {
ssize_t sz, this_sz;
@@ -38,20 +39,20 @@ int cmd__sha1(int ac, const char **av)
if (sz == 0)
break;
if (sz < 0)
- die_errno("test-sha1");
+ die_errno("test-hash");
this_sz += sz;
cp += sz;
room -= sz;
}
if (this_sz == 0)
break;
- git_SHA1_Update(, buffer, this_sz);
+ algop->update_fn(, buffer, this_sz);
}
- git_SHA1_Final(sha1, );
+ algop->final_fn(hash, );
if (binary)
- fwrite(sha1, 1, 20, stdout);
+ fwrite(hash, 1, algop->rawsz, stdout);
else
- puts(sha1_to_hex(sha1));
+ puts(hash_to_hex_algop(hash, algop));
exit(0);
}
diff --git a/t/helper/test-sha1.c b/t/helper/test-sha1.c
index 1ba0675c75..d860c387c3 100644
--- a/t/helper/test-sha1.c
+++ b/t/helper/test-sha1.c
@@ -3,55 +3,5 @@
int cmd__sha1(int ac, const char **av)
{
- git_SHA_CTX ctx;
- unsigned char sha1[20];
- unsigned bufsz = 8192;
- int binary = 0;
- char *buffer;
-
- if (ac == 2) {
- if (!strcmp(av[1], "-b"))
- binary = 1;
- else
- bufsz = strtoul(av[1], NULL, 10) * 1024 * 1024;
- }
-
- if (!bufsz)
- bufsz = 8192;
-
- while ((buffer = malloc(bufsz)) == NULL) {
- fprintf(stderr, "bufsz %u is too big, halving...\n", bufsz);
- bufsz /= 2;
- if (bufsz < 1024)
- die("OOPS");
- }
-
- git_SHA1_Init();
-
- while (1) {
- ssize_t sz, this_sz;
- char *cp = buffer;
- unsigned room = bufsz;
- this_sz = 0;
- while (room) {
- sz = xread(0, cp, room);
- if (sz == 0)
- break;
- if (sz < 0)
- die_errno("test-sha1");
- this_sz += sz;
- cp += sz;
- room -= sz;
- }
- if (this_sz == 0)
- break;
- git_SHA1_Update(, buffer, this_sz);
- }
- git_SHA1_Final(sha1, );
-
- if (binary)
- fwrite(sha1, 1, 20, stdout);
- else
- puts(sha1_to_hex(sha1));
- exit(0);
+ return cmd_hash_impl(ac, av, GIT_HASH_SHA1);
}
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index e4890566da..29ac7b0b0d 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -50,4 +50,6 @@ int cmd__windows_named_pipe(int argc, const char **argv);
#endif
int cmd__write_cache(int argc, const char **argv);
[PATCH v5 04/12] cache: make hashcmp and hasheq work with larger hashes
In 183a638b7d ("hashcmp: assert constant hash size", 2018-08-23), we
modified hashcmp to assert that the hash size was always 20 to help it
optimize and inline calls to memcmp. In a future series, we replaced
many calls to hashcmp and oidcmp with calls to hasheq and oideq to
improve inlining further.
However, we want to support hash algorithms other than SHA-1, namely
SHA-256. When doing so, we must handle the case where these values are
32 bytes long as well as 20. Adjust hashcmp to handle two cases:
20-byte matches, and maximum-size matches. Therefore, when we include
SHA-256, we'll automatically handle it properly, while at the same time
teaching the compiler that there are only two possible options to
consider. This will allow the compiler to write the most efficient
possible code.
Copy similar code into hasheq and perform an identical transformation.
At least with GCC 8.2.0, making hasheq defer to hashcmp when there are
two branches prevents the compiler from inlining the comparison, while
the code in this patch is inlined properly. Add a comment to avoid an
accidental performance regression from well-intentioned refactoring.
Signed-off-by: brian m. carlson
---
cache.h | 22 --
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/cache.h b/cache.h
index 51580c4b77..bab8e8964f 100644
--- a/cache.h
+++ b/cache.h
@@ -1027,16 +1027,12 @@ extern const struct object_id null_oid;
static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)
{
/*
-* This is a temporary optimization hack. By asserting the size here,
-* we let the compiler know that it's always going to be 20, which lets
-* it turn this fixed-size memcmp into a few inline instructions.
-*
-* This will need to be extended or ripped out when we learn about
-* hashes of different sizes.
+* Teach the compiler that there are only two possibilities of hash size
+* here, so that it can optimize for this case as much as possible.
*/
- if (the_hash_algo->rawsz != 20)
- BUG("hash size not yet supported by hashcmp");
- return memcmp(sha1, sha2, the_hash_algo->rawsz);
+ if (the_hash_algo->rawsz == GIT_MAX_RAWSZ)
+ return memcmp(sha1, sha2, GIT_MAX_RAWSZ);
+ return memcmp(sha1, sha2, GIT_SHA1_RAWSZ);
}
static inline int oidcmp(const struct object_id *oid1, const struct object_id
*oid2)
@@ -1046,7 +1042,13 @@ static inline int oidcmp(const struct object_id *oid1,
const struct object_id *o
static inline int hasheq(const unsigned char *sha1, const unsigned char *sha2)
{
- return !hashcmp(sha1, sha2);
+ /*
+* We write this here instead of deferring to hashcmp so that the
+* compiler can properly inline it and avoid calling memcmp.
+*/
+ if (the_hash_algo->rawsz == GIT_MAX_RAWSZ)
+ return !memcmp(sha1, sha2, GIT_MAX_RAWSZ);
+ return !memcmp(sha1, sha2, GIT_SHA1_RAWSZ);
}
static inline int oideq(const struct object_id *oid1, const struct object_id
*oid2)
Re: [PATCH] multi-pack-index: make code -Wunused-parameter clean
On Sat, Nov 03, 2018 at 05:49:57PM -0700, Carlo Marcelo Arenas Belón wrote:
> introduced in 662148c435 ("midx: write object offsets", 2018-07-12)
> but included on all previous versions as well.
>
> midx.c:713:54: warning: unused parameter 'nr_objects' [-Wunused-parameter]
>
> likely an oversight as the information needed to iterate over is
> embedded in nr_large_offset
I've been preparing a series to make the whole code base compile with
-Wunused-parameter, and I handled this case a bit differently.
-- >8 --
Subject: [PATCH] midx: double-check large object write loop
The write_midx_large_offsets() function takes an array of object
entries, the number of entries in the array (nr_objects), and the number
of entries with large offsets (nr_large_offset). But we never actually
use nr_objects; instead we keep walking down the array and counting down
nr_large_offset until we've seen all of the large entries.
This is correct, but we can be a bit more defensive. If there were ever
a mismatch between nr_large_offset and the actual set of large-offset
objects, we'd walk off the end of the array.
Since we know the size of the array, we can use nr_objects to make sure
we don't walk too far.
Signed-off-by: Jeff King
---
midx.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/midx.c b/midx.c
index 4fac0cd08a..ecd583666a 100644
--- a/midx.c
+++ b/midx.c
@@ -712,12 +712,18 @@ static size_t write_midx_object_offsets(struct hashfile
*f, int large_offset_nee
static size_t write_midx_large_offsets(struct hashfile *f, uint32_t
nr_large_offset,
struct pack_midx_entry *objects,
uint32_t nr_objects)
{
- struct pack_midx_entry *list = objects;
+ struct pack_midx_entry *list = objects, *end = objects + nr_objects;
size_t written = 0;
while (nr_large_offset) {
- struct pack_midx_entry *obj = list++;
- uint64_t offset = obj->offset;
+ struct pack_midx_entry *obj;
+ uint64_t offset;
+
+ if (list >= end)
+ BUG("too many large-offset objects");
+
+ obj = list++;
+ offset = obj->offset;
if (!(offset >> 31))
continue;
--
2.19.1.1352.g60f3b1a4c2
[PATCH] multi-pack-index: make code -Wunused-parameter clean
introduced in 662148c435 ("midx: write object offsets", 2018-07-12)
but included on all previous versions as well.
midx.c:713:54: warning: unused parameter 'nr_objects' [-Wunused-parameter]
likely an oversight as the information needed to iterate over is
embedded in nr_large_offset
Signed-off-by: Carlo Marcelo Arenas Belón
---
midx.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/midx.c b/midx.c
index 4fac0cd08a..a2c17e3108 100644
--- a/midx.c
+++ b/midx.c
@@ -710,7 +710,7 @@ static size_t write_midx_object_offsets(struct hashfile *f,
int large_offset_nee
}
static size_t write_midx_large_offsets(struct hashfile *f, uint32_t
nr_large_offset,
- struct pack_midx_entry *objects,
uint32_t nr_objects)
+ struct pack_midx_entry *objects)
{
struct pack_midx_entry *list = objects;
size_t written = 0;
@@ -880,7 +880,7 @@ int write_midx_file(const char *object_dir)
break;
case MIDX_CHUNKID_LARGEOFFSETS:
- written += write_midx_large_offsets(f,
num_large_offsets, entries, nr_entries);
+ written += write_midx_large_offsets(f,
num_large_offsets, entries);
break;
default:
--
2.19.1.816.gcd69ec8cd
Re: [PATCH v3] i18n: make GETTEXT_POISON a runtime option
On Thu, Nov 01, 2018 at 07:31:15PM +, Ævar Arnfjörð Bjarmason wrote:
> Change the GETTEXT_POISON compile-time + runtime GIT_GETTEXT_POISON
> test parameter to only be a GIT_TEST_GETTEXT_POISON=
> runtime parameter, to be consistent with other parameters documented
> in "Running tests with special setups" in t/README.
>
> When I added GETTEXT_POISON in bb946bba76 ("i18n: add GETTEXT_POISON
> to simulate unfriendly translator", 2011-02-22) I was concerned with
> ensuring that the _() function would get constant folded if NO_GETTEXT
> was defined, and likewise that GETTEXT_POISON would be compiled out
> unless it was defined.
>
> But as the benchmark in my [1] shows doing a one-off runtime
> getenv("GIT_TEST_[...]") is trivial, and since GETTEXT_POISON was
> originally added the GIT_TEST_* env variables have become the common
> idiom for turning on special test setups.
>
> So change GETTEXT_POISON to work the same way. Now the
> GETTEXT_POISON=YesPlease compile-time option is gone, and running the
> tests with GIT_TEST_GETTEXT_POISON=[YesPlease|] can be toggled on/off
> without recompiling.
>
> This allows for conditionally amending tests to test with/without
> poison, similar to what 859fdc0c3c ("commit-graph: define
> GIT_TEST_COMMIT_GRAPH", 2018-08-29) did for GIT_TEST_COMMIT_GRAPH. Do
> some of that, now we e.g. always run the t0205-gettext-poison.sh test.
>
> I did enough there to remove the GETTEXT_POISON prerequisite, but its
> inverse C_LOCALE_OUTPUT is still around, and surely some tests using
> it can be converted to e.g. always set GIT_TEST_GETTEXT_POISON=.
>
> Notes on the implementation:
>
> * We still compile a dedicated GETTEXT_POISON build in Travis CI.
>This is probably the wrong thing to do and should be followed-up
>with something similar to ae59a4e44f ("travis: run tests with
>GIT_TEST_SPLIT_INDEX", 2018-01-07) to re-use an existing test setup
>for running in the GIT_TEST_GETTEXT_POISON mode.
I'm of two minds about this. Sure, now it's not a compile time
option, so we can spare a dedicated compilation, and sparing resources
is good, of course.
However, checking test failures in the 'linux-gcc' build job is always
a bit of a hassle, because it's not enough to look at the output of
the failed test, but I also have to check which one of the two test
runs failed (i.e. the "regular" or one with all the GIT_TEST_* knobs
turned on). Adding a second test run with GIT_TEST_GETTEXT_POISON
enabled to an other build job (e.g. 'linux-clang') would then bring
this hassle to that build job as well.
Furthermore, occasionally a build job is slower than usual (whatever
the reason might be), which can be an issue when running the test
suite twice, as it can exceed the time limit.
Anyway, we can think about this later.
In any case, GIT_TEST_GETTEXT_POISON definitely should not be enabled
in the same "catch-all" test run with all other GIT_TEST_* variables,
because it would mess up any translated error messages that might pop
up in a test failure, and then we wouldn't have any idea about what
went wrong.
> * We now skip a test in t-basic.sh under
>GIT_TEST_GETTEXT_POISON=YesPlease that wasn't skipped before. This
>test relies on C locale output, but due to an edge case in how the
>previous implementation of GETTEXT_POISON worked (reading it from
>GIT-BUILD-OPTIONS) wasn't enabling poison correctly. Now it does,
>and needs to be skipped.
>
> * The getenv() function is not reentrant, so out of paranoia about
>code of the form:
>
>printf(_("%s"), getenv("some-env"));
>
>call use_gettext_poison() in our early setup in git_setup_gettext()
>so we populate the "poison_requested" variable in a codepath that's
>won't suffer from that race condition.
>
> See also [3] for more on the motivation behind this patch, and the
> history of the GETTEXT_POISON facility.
>
> 1. https://public-inbox.org/git/871s8gd32p@evledraar.gmail.com/
> 2. https://public-inbox.org/git/87woq7b58k@evledraar.gmail.com/
> 3. https://public-inbox.org/git/878t2pd6yu@evledraar.gmail.com/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
>
> Now:
>
> * The new i18n helper is gone. We just use "test -n" semantics for
>$GIT_TEST_GETTEXT_POISON
>
> * We error out in the Makefile if you're still saying
>GETTEXT_POISON=YesPlease.
>
>This makes more sense than just making it a synonym since now this
>also needs to be defined at runtime.
OK, I think erroring out is better than silently ignoring
GETTEXT_POISON=YesPlease. However, the commit message only mentions
that GETTEXT_POISON is gone, but perhaps this should be mentioned
there as well.
> * The caveat with avoiding test_lazy_prereq is gone (although there's
>still some unrelated bug there worth looking into).
>
> * We call use_gettext_poison() really early to avoid any reentrancy
>issue with getenv().
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>
Re: [PATCH 0/3] Make add_missing_tags() linear
On 11/2/2018 10:58 AM, Elijah Newren wrote: On Thu, Nov 1, 2018 at 12:02 PM Derrick Stolee wrote: On 11/1/2018 2:57 PM, Elijah Newren wrote: On Thu, Nov 1, 2018 at 5:32 AM Derrick Stolee wrote: No rush. I'd just like to understand how removing the commit-graph file can make the new algorithm faster. Putting a similar count in the old algorithm would involve giving a count for every call to in_merge_bases_many(), which would be very noisy. $ time git push --dry-run --follow-tags /home/newren/repo-mirror count: 92912 To /home/newren/repo-mirror * [new branch] test5 -> test5 real0m3.024s user0m2.752s sys0m0.320s Is the above test with or without the commit-graph file? Can you run it in the other mode, too? I'd like to see if the "count" value changes when the only difference is the presence of a commit-graph file. I apologize for providing misleading information earlier; this was an apples to oranges comparison. Here's what I did: git clone coworker.bundle coworker-repo cd coworker-repo time git push --dry-run --follow-tags /home/newren/repo-mirror git config core.commitgraph true git config gc.writecommitgraph true git gc time git push --dry-run --follow-tags /home/newren/nucleus-mirror I figured I had just done a fresh clone, so surely the gc wouldn't do anything other than write the .git/objects/info/commit-graph file. However, the original bundle contained many references outside of refs/heads/ and refs/tags/: $ git bundle list-heads ../coworker.bundle | grep -v -e refs/heads/ -e refs/tags/ -e HEAD | wc -l 2396 These other refs apparently referred to objects not otherwise referenced in refs/heads/ and refs/tags/, and caused the gc to explode lots of loose objects: $ git count-objects -v count: 147604 size: 856416 in-pack: 1180692 packs: 1 size-pack: 796143 prune-packable: 0 garbage: 0 size-garbage: 0 The slowdown with commit-graph was entirely due to there being lots of loose objects (147K of them). If I add a git-prune before doing the timing with commit-graph, then the timing with commit-graph is faster than the run without a commit-graph. Sorry for the wild goose chase. And thanks for the fixes; get_reachable_subset() makes things much faster even without a commit-graph, and the commit-graph just improves it more. :-) Thanks for double-checking! It's good to have confidence that this is a good algorithm to use. Thanks, -Stolee
Re: [PATCH 0/3] Make add_missing_tags() linear
On Thu, Nov 1, 2018 at 12:02 PM Derrick Stolee wrote: > > On 11/1/2018 2:57 PM, Elijah Newren wrote: > > On Thu, Nov 1, 2018 at 5:32 AM Derrick Stolee wrote: > >> No rush. I'd just like to understand how removing the commit-graph file > >> can make the new algorithm faster. Putting a similar count in the old > >> algorithm would involve giving a count for every call to > >> in_merge_bases_many(), which would be very noisy. > > $ time git push --dry-run --follow-tags /home/newren/repo-mirror > > count: 92912 > > To /home/newren/repo-mirror > > * [new branch] test5 -> test5 > > > > real0m3.024s > > user0m2.752s > > sys0m0.320s > > Is the above test with or without the commit-graph file? Can you run it > in the other mode, too? I'd like to see if the "count" value changes > when the only difference is the presence of a commit-graph file. I apologize for providing misleading information earlier; this was an apples to oranges comparison. Here's what I did: git clone coworker.bundle coworker-repo cd coworker-repo time git push --dry-run --follow-tags /home/newren/repo-mirror git config core.commitgraph true git config gc.writecommitgraph true git gc time git push --dry-run --follow-tags /home/newren/nucleus-mirror I figured I had just done a fresh clone, so surely the gc wouldn't do anything other than write the .git/objects/info/commit-graph file. However, the original bundle contained many references outside of refs/heads/ and refs/tags/: $ git bundle list-heads ../coworker.bundle | grep -v -e refs/heads/ -e refs/tags/ -e HEAD | wc -l 2396 These other refs apparently referred to objects not otherwise referenced in refs/heads/ and refs/tags/, and caused the gc to explode lots of loose objects: $ git count-objects -v count: 147604 size: 856416 in-pack: 1180692 packs: 1 size-pack: 796143 prune-packable: 0 garbage: 0 size-garbage: 0 The slowdown with commit-graph was entirely due to there being lots of loose objects (147K of them). If I add a git-prune before doing the timing with commit-graph, then the timing with commit-graph is faster than the run without a commit-graph. Sorry for the wild goose chase. And thanks for the fixes; get_reachable_subset() makes things much faster even without a commit-graph, and the commit-graph just improves it more. :-)
[PATCH v2 0/3] Make add_missing_tags() linear
As reported earlier [1], the add_missing_tags() method in remote.c has
quadratic performance. Some of that performance is curbed due to the
generation-number cutoff in in_merge_bases_many(). However, that fix doesn't
help users without a commit-graph, and it can still be painful if that
cutoff is sufficiently low compared to the tags we are using for
reachability testing.
Add a new method in commit-reach.c called get_reachable_subset() which does
a many-to-many reachability test. Starting at the 'from' commits, walk until
the generation is below the smallest generation in the 'to' commits, or all
'to' commits have been discovered. This performs only one commit walk for
the entire add_missing_tags() method, giving linear performance in the worst
case.
Tests are added in t6600-test-reach.sh to ensure get_reachable_subset()
works independently of its application in add_missing_tags().
Thanks, -Stolee
[1]
https://public-inbox.org/git/cabpp-becpsoxudovjbdg_3w9wus102rw+e+qpmd4g3qyd-q...@mail.gmail.com/
Derrick Stolee (3):
commit-reach: implement get_reachable_subset
test-reach: test get_reachable_subset
remote: make add_missing_tags() linear
commit-reach.c| 70 +++
commit-reach.h| 13
remote.c | 34 -
t/helper/test-reach.c | 34 ++---
t/t6600-test-reach.sh | 52
5 files changed, 198 insertions(+), 5 deletions(-)
base-commit: c670b1f876521c9f7cd40184bf7ed05aad843433
Published-As:
https://github.com/gitgitgadget/git/releases/tags/pr-60%2Fderrickstolee%2Fadd-missing-tags-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-60/derrickstolee/add-missing-tags-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/60
Range-diff vs v1:
1: 4c0c5c9143 ! 1: 9e570603bd commit-reach: implement get_reachable_subset
@@ -49,7 +49,7 @@
+
+struct commit_list *get_reachable_subset(struct commit **from, int
nr_from,
+ struct commit **to, int nr_to,
-+ int reachable_flag)
++ unsigned int reachable_flag)
+{
+ struct commit **item;
+ struct commit *current;
@@ -129,9 +129,12 @@
+ * Return a list of commits containing the commits in the 'to' array
+ * that are reachable from at least one commit in the 'from' array.
+ * Also add the given 'flag' to each of the commits in the returned list.
++ *
++ * This method uses the PARENT1 and PARENT2 flags during its operation,
++ * so be sure these flags are not set before calling the method.
+ */
+struct commit_list *get_reachable_subset(struct commit **from, int
nr_from,
+ struct commit **to, int nr_to,
-+ int reachable_flag);
++ unsigned int reachable_flag);
+
#endif
2: 382f4f4a5b = 2: 52e847b928 test-reach: test get_reachable_subset
3: ecbed3de5c = 3: dfaceb162f remote: make add_missing_tags() linear
--
gitgitgadget
[PATCH v2 3/3] remote: make add_missing_tags() linear
From: Derrick Stolee
The add_missing_tags() method currently has quadratic behavior.
This is due to a linear number (based on number of tags T) of
calls to in_merge_bases_many, which has linear performance (based
on number of commits C in the repository).
Replace this O(T * C) algorithm with an O(T + C) algorithm by
using get_reachable_subset(). We ignore the return list and focus
instead on the reachable_flag assigned to the commits we care
about, because we need to interact with the tag ref and not just
the commit object.
Signed-off-by: Derrick Stolee
---
remote.c | 34 +-
1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/remote.c b/remote.c
index 81f4f01b00..b850f2feb3 100644
--- a/remote.c
+++ b/remote.c
@@ -1205,9 +1205,36 @@ static void add_missing_tags(struct ref *src, struct ref
**dst, struct ref ***ds
* sent to the other side.
*/
if (sent_tips.nr) {
+ const int reachable_flag = 1;
+ struct commit_list *found_commits;
+ struct commit **src_commits;
+ int nr_src_commits = 0, alloc_src_commits = 16;
+ ALLOC_ARRAY(src_commits, alloc_src_commits);
+
for_each_string_list_item(item, _tag) {
struct ref *ref = item->util;
+ struct commit *commit;
+
+ if (is_null_oid(>new_oid))
+ continue;
+ commit = lookup_commit_reference_gently(the_repository,
+ >new_oid,
+ 1);
+ if (!commit)
+ /* not pushing a commit, which is not an error
*/
+ continue;
+
+ ALLOC_GROW(src_commits, nr_src_commits + 1,
alloc_src_commits);
+ src_commits[nr_src_commits++] = commit;
+ }
+
+ found_commits = get_reachable_subset(sent_tips.tip,
sent_tips.nr,
+src_commits,
nr_src_commits,
+reachable_flag);
+
+ for_each_string_list_item(item, _tag) {
struct ref *dst_ref;
+ struct ref *ref = item->util;
struct commit *commit;
if (is_null_oid(>new_oid))
@@ -1223,7 +1250,7 @@ static void add_missing_tags(struct ref *src, struct ref
**dst, struct ref ***ds
* Is this tag, which they do not have, reachable from
* any of the commits we are sending?
*/
- if (!in_merge_bases_many(commit, sent_tips.nr,
sent_tips.tip))
+ if (!(commit->object.flags & reachable_flag))
continue;
/* Add it in */
@@ -1231,7 +1258,12 @@ static void add_missing_tags(struct ref *src, struct ref
**dst, struct ref ***ds
oidcpy(_ref->new_oid, >new_oid);
dst_ref->peer_ref = copy_ref(ref);
}
+
+ clear_commit_marks_many(nr_src_commits, src_commits,
reachable_flag);
+ free(src_commits);
+ free_commit_list(found_commits);
}
+
string_list_clear(_tag, 0);
free(sent_tips.tip);
}
--
gitgitgadget
Re: [PATCH v3] i18n: make GETTEXT_POISON a runtime option
Ævar Arnfjörð Bjarmason writes:
> Change the GETTEXT_POISON compile-time + runtime GIT_GETTEXT_POISON
> test parameter to only be a GIT_TEST_GETTEXT_POISON=
> runtime parameter, to be consistent with other parameters documented
> in "Running tests with special setups" in t/README.
> ...
> #ifndef NO_GETTEXT
> extern void git_setup_gettext(void);
> extern int gettext_width(const char *s);
> #else
> static inline void git_setup_gettext(void)
> {
> + use_gettext_poison();; /* getenv() reentrancy paranoia */
> }
Too many "case/esac" statement in shell scripting?
[PATCH v3] i18n: make GETTEXT_POISON a runtime option
as
# a filter to have gitweb.js minified.
#
@@ -1450,7 +1445,7 @@ ifdef NO_SYMLINK_HEAD
BASIC_CFLAGS += -DNO_SYMLINK_HEAD
endif
ifdef GETTEXT_POISON
- BASIC_CFLAGS += -DGETTEXT_POISON
+$(error The GETTEXT_POISON option has been removed in favor of runtime
GIT_TEST_GETTEXT_POISON. See t/README!)
endif
ifdef NO_GETTEXT
BASIC_CFLAGS += -DNO_GETTEXT
@@ -2602,7 +2597,6 @@ ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT
@echo GIT_TEST_CMP_USE_COPIED_CONTEXT=YesPlease >>$@+
endif
@echo NO_GETTEXT=\''$(subst ','\'',$(subst ','\'',$(NO_GETTEXT)))'\'
>>$@+
- @echo GETTEXT_POISON=\''$(subst ','\'',$(subst
','\'',$(GETTEXT_POISON)))'\' >>$@+
ifdef GIT_PERF_REPEAT_COUNT
@echo GIT_PERF_REPEAT_COUNT=\''$(subst ','\'',$(subst
','\'',$(GIT_PERF_REPEAT_COUNT)))'\' >>$@+
endif
diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
index 06970f7213..69dff4d1ec 100755
--- a/ci/lib-travisci.sh
+++ b/ci/lib-travisci.sh
@@ -123,7 +123,7 @@ osx-clang|osx-gcc)
# Travis CI OS X
export GIT_SKIP_TESTS="t9810 t9816"
;;
-GETTEXT_POISON)
- export GETTEXT_POISON=YesPlease
+GIT_TEST_GETTEXT_POISON)
+ export GIT_TEST_GETTEXT_POISON=YesPlease
;;
esac
diff --git a/gettext.c b/gettext.c
index 7272771c8e..d4021d690c 100644
--- a/gettext.c
+++ b/gettext.c
@@ -7,6 +7,7 @@
#include "gettext.h"
#include "strbuf.h"
#include "utf8.h"
+#include "config.h"
#ifndef NO_GETTEXT
# include
@@ -46,15 +47,15 @@ const char *get_preferred_languages(void)
return NULL;
}
-#ifdef GETTEXT_POISON
int use_gettext_poison(void)
{
static int poison_requested = -1;
- if (poison_requested == -1)
- poison_requested = getenv("GIT_GETTEXT_POISON") ? 1 : 0;
+ if (poison_requested == -1) {
+ const char *v = getenv("GIT_TEST_GETTEXT_POISON");
+ poison_requested = v && strlen(v) ? 1 : 0;
+ }
return poison_requested;
}
-#endif
#ifndef NO_GETTEXT
static int test_vsnprintf(const char *fmt, ...)
@@ -164,6 +165,8 @@ void git_setup_gettext(void)
if (!podir)
podir = p = system_path(GIT_LOCALE_PATH);
+ use_gettext_poison(); /* getenv() reentrancy paranoia */
+
if (!is_directory(podir)) {
free(p);
return;
diff --git a/gettext.h b/gettext.h
index 7eee64a34f..43d991a2df 100644
--- a/gettext.h
+++ b/gettext.h
@@ -28,12 +28,15 @@
#define FORMAT_PRESERVING(n) __attribute__((format_arg(n)))
+extern int use_gettext_poison(void);
+
#ifndef NO_GETTEXT
extern void git_setup_gettext(void);
extern int gettext_width(const char *s);
#else
static inline void git_setup_gettext(void)
{
+ use_gettext_poison();; /* getenv() reentrancy paranoia */
}
static inline int gettext_width(const char *s)
{
@@ -41,12 +44,6 @@ static inline int gettext_width(const char *s)
}
#endif
-#ifdef GETTEXT_POISON
-extern int use_gettext_poison(void);
-#else
-#define use_gettext_poison() 0
-#endif
-
static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)
{
if (!*msgid)
diff --git a/git-sh-i18n.sh b/git-sh-i18n.sh
index 9d065fb4bf..e1d917fd27 100644
--- a/git-sh-i18n.sh
+++ b/git-sh-i18n.sh
@@ -17,7 +17,7 @@ export TEXTDOMAINDIR
# First decide what scheme to use...
GIT_INTERNAL_GETTEXT_SH_SCHEME=fallthrough
-if test -n "$GIT_GETTEXT_POISON"
+if test -n "$GIT_TEST_GETTEXT_POISON"
then
GIT_INTERNAL_GETTEXT_SH_SCHEME=poison
elif test -n "@@USE_GETTEXT_SCHEME@@"
diff --git a/po/README b/po/README
index fef4c0f0b5..aa704ffcb7 100644
--- a/po/README
+++ b/po/README
@@ -289,16 +289,11 @@ something in the test suite might still depend on the US
English
version of the strings, e.g. to grep some error message or other
output.
-To smoke out issues like these Git can be compiled with gettext poison
-support, at the top-level:
+To smoke out issues like these, Git tested with a translation mode that
+emits gibberish on every call to gettext. To use it run the test suite
+with it, e.g.:
-make GETTEXT_POISON=YesPlease
-
-That'll give you a git which emits gibberish on every call to
-gettext. It's obviously not meant to be installed, but you should run
-the test suite with it:
-
-cd t && prove -j 9 ./t[0-9]*.sh
+cd t && GIT_TEST_GETTEXT_POISON=YesPlease prove -j 9 ./t[0-9]*.sh
If tests break with it you should inspect them manually and see if
what you're translating is sane, i.e. that you're not translating
diff --git a/t/README b/t/README
index 8847489640..25c4ba3419 100644
--- a/t/README
+++ b/t/README
@@ -301,6 +301,12 @@ that cannot be easily covered by a few specific test
cases. These
could be enabled by running the test suite with correct GIT_TEST_
environment set.
+GIT_TEST_GETTEXT_POISON= turns all strings marked for
+translation into gibberish if non
Re: [PATCH 0/3] Make add_missing_tags() linear
On 11/1/2018 2:57 PM, Elijah Newren wrote: On Thu, Nov 1, 2018 at 5:32 AM Derrick Stolee wrote: No rush. I'd just like to understand how removing the commit-graph file can make the new algorithm faster. Putting a similar count in the old algorithm would involve giving a count for every call to in_merge_bases_many(), which would be very noisy. $ time git push --dry-run --follow-tags /home/newren/repo-mirror count: 92912 To /home/newren/repo-mirror * [new branch] test5 -> test5 real0m3.024s user0m2.752s sys0m0.320s Is the above test with or without the commit-graph file? Can you run it in the other mode, too? I'd like to see if the "count" value changes when the only difference is the presence of a commit-graph file. Thanks, -Stolee
Re: [PATCH 0/3] Make add_missing_tags() linear
On Thu, Nov 1, 2018 at 5:32 AM Derrick Stolee wrote: > On 11/1/2018 2:52 AM, Elijah Newren wrote: > > On Wed, Oct 31, 2018 at 5:05 AM Derrick Stolee wrote: > >> On 10/31/2018 2:04 AM, Elijah Newren wrote: > >>> > >>> On the original repo where the topic was brought up, with commit-graph > >>> NOT turned on and using origin/master, I see: > >>> > >>> $ time git push --dry-run --follow-tags /home/newren/repo-mirror > >>> To /home/newren/repo-mirror > >>> * [new branch] test5 -> test5 > >>> > >>> real 1m20.081s > >>> user 1m19.688s > >>> sys 0m0.292s > >>> > >>> Merging this series in, I now get: > >>> > >>> $ time git push --dry-run --follow-tags /home/newren/repo-mirror > >>> To /home/newren/repo-mirror > >>> * [new branch] test5 -> test5 > >>> > >>> real 0m2.857s > >>> user 0m2.580s > >>> sys 0m0.328s > >>> > >>> which provides a very nice speedup. > >>> > >>> Oddly enough, if I _also_ do the following: > >>> $ git config core.commitgraph true > >>> $ git config gc.writecommitgraph true > >>> $ git gc > >>> > >>> then my timing actually slows down just slightly: > >>> $ time git push --follow-tags --dry-run /home/newren/repo-mirror > >>> To /home/newren/repo-mirror > >>> * [new branch] test5 -> test5 > >>> > >>> real 0m3.027s > >>> user 0m2.696s > >>> sys 0m0.400s > >> So you say that the commit-graph is off in the 2.8s case, but not here > >> in the 3.1s case? I would expect _at minimum_ that the cost of parsing > >> commits would have a speedup in the commit-graph case. There may be > >> something else going on here, since you are timing a `push` event that > >> is doing more than the current walk. > >> > >>> (run-to-run variation seems pretty consistent, < .1s variation, so > >>> this difference is just enough to notice.) I wouldn't be that > >>> surprised if that means there's some really old tags with very small > >>> generation numbers, meaning it's not gaining anything in this special > >>> case from the commit-graph, but it does pay the cost of loading the > >>> commit-graph. > >> While you have this test environment, do you mind applying the diff > >> below and re-running the tests? It will output a count for how many > >> commits are walked by the algorithm. This should help us determine if > >> this is another case where generation numbers are worse than commit-date, > >> or if there is something else going on. Thanks! > > I can do that, but wouldn't you want a similar patch for the old > > get_merge_bases_many() in order to compare? Does an absolute number > > help by itself? > > It's going to have to be tomorrow, though; not enough time tonight. > > No rush. I'd just like to understand how removing the commit-graph file > can make the new algorithm faster. Putting a similar count in the old > algorithm would involve giving a count for every call to > in_merge_bases_many(), which would be very noisy. $ time git push --dry-run --follow-tags /home/newren/repo-mirror count: 92912 To /home/newren/repo-mirror * [new branch] test5 -> test5 real0m3.024s user0m2.752s sys0m0.320s Also: $ git rev-list --count HEAD 55764 $ git rev-list --count --all 91820 Seems a little odd to me that count is greater than `git rev-list --count --all`. However, the fact that they are close in magnitude isn't surprising since I went digging for the commit with smallest generation number not found in the upstream repo, and found: $ git ls-remote /home/newren/repo-mirror/ | grep refs/tags/v0.2.0; echo $? 1 $ git rev-list --count refs/tags/v0.2.0 4 $ git rev-list --count refs/tags/v0.2.0 ^HEAD 4 So, the commit-graph can only help us avoid parsing 3 or so commits, but we have to parse the 5M .git/objects/info/commit-graph file, and then for every parse_commit() call we need to bsearch_graph() for the commit.My theory is that parsing the commit-graph file and binary searching it for commits is relatively fast, but that the time is just big enough to measure and notice, while avoiding parsing 3 more commits is a negligible time savings. To me, I'm thinking this is one of those bizarre corner cases where the commit-graph is almost imperceptibly slower than without the commit-graph. (And it is a very weird repo; someone repeatedly filter-branched lots of small independent repos into a monorepo, but didn't always push everything and didn't clean out all old stuff.) But if you still see weird stuff you want to dig into further (maybe the 92912 > 91820 bit?), I'm happy to try out other stuff.
[PATCH v5 7/7] t6012: make rev-list tests more interesting
As we are working to rewrite some of the revision-walk machinery,
there could easily be some interesting interactions between the
options that force topological constraints (--topo-order,
--date-order, and --author-date-order) along with specifying a
path.
Add extra tests to t6012-rev-list-simplify.sh to add coverage of
these interactions. To ensure interesting things occur, alter the
repo data shape to have different orders depending on topo-, date-,
or author-date-order.
When testing using GIT_TEST_COMMIT_GRAPH, this assists in covering
the new logic for topo-order walks using generation numbers. The
extra tests can be added indepently.
Signed-off-by: Derrick Stolee
---
t/t6012-rev-list-simplify.sh | 45
1 file changed, 36 insertions(+), 9 deletions(-)
diff --git a/t/t6012-rev-list-simplify.sh b/t/t6012-rev-list-simplify.sh
index b5a1190ffe..a10f0df02b 100755
--- a/t/t6012-rev-list-simplify.sh
+++ b/t/t6012-rev-list-simplify.sh
@@ -12,6 +12,22 @@ unnote () {
git name-rev --tags --stdin | sed -e "s|$OID_REGEX (tags/\([^)]*\)) |\1
|g"
}
+#
+# Create a test repo with interesting commit graph:
+#
+# A--B--G--H--I--K--L
+# \ \ / /
+# \ \ / /
+#C--E---F J
+#\_/
+#
+# The commits are laid out from left-to-right starting with
+# the root commit A and terminating at the tip commit L.
+#
+# There are a few places where we adjust the commit date or
+# author date to make the --topo-order, --date-order, and
+# --author-date-order flags produce different output.
+
test_expect_success setup '
echo "Hi there" >file &&
echo "initial" >lost &&
@@ -21,10 +37,18 @@ test_expect_success setup '
git branch other-branch &&
+ git symbolic-ref HEAD refs/heads/unrelated &&
+ git rm -f "*" &&
+ echo "Unrelated branch" >side &&
+ git add side &&
+ test_tick && git commit -m "Side root" &&
+ note J &&
+ git checkout master &&
+
echo "Hello" >file &&
echo "second" >lost &&
git add file lost &&
- test_tick && git commit -m "Modified file and lost" &&
+ test_tick && GIT_AUTHOR_DATE=$(($test_tick + 120)) git commit -m
"Modified file and lost" &&
note B &&
git checkout other-branch &&
@@ -63,13 +87,6 @@ test_expect_success setup '
test_tick && git commit -a -m "Final change" &&
note I &&
- git symbolic-ref HEAD refs/heads/unrelated &&
- git rm -f "*" &&
- echo "Unrelated branch" >side &&
- git add side &&
- test_tick && git commit -m "Side root" &&
- note J &&
-
git checkout master &&
test_tick && git merge --allow-unrelated-histories -m "Coolest"
unrelated &&
note K &&
@@ -103,14 +120,24 @@ check_result () {
check_outcome success "$@"
}
-check_result 'L K J I H G F E D C B A' --full-history
+check_result 'L K J I H F E D C G B A' --full-history --topo-order
+check_result 'L K I H G F E D C B J A' --full-history
+check_result 'L K I H G F E D C B J A' --full-history --date-order
+check_result 'L K I H G F E D B C J A' --full-history --author-date-order
check_result 'K I H E C B A' --full-history -- file
check_result 'K I H E C B A' --full-history --topo-order -- file
check_result 'K I H E C B A' --full-history --date-order -- file
+check_result 'K I H E B C A' --full-history --author-date-order -- file
check_result 'I E C B A' --simplify-merges -- file
+check_result 'I E C B A' --simplify-merges --topo-order -- file
+check_result 'I E C B A' --simplify-merges --date-order -- file
+check_result 'I E B C A' --simplify-merges --author-date-order -- file
check_result 'I B A' -- file
check_result 'I B A' --topo-order -- file
+check_result 'I B A' --date-order -- file
+check_result 'I B A' --author-date-order -- file
check_result 'H' --first-parent -- another-file
+check_result 'H' --first-parent --topo-order -- another-file
check_result 'E C B A' --full-history E -- lost
test_expect_success 'full history simplification without parent' '
--
2.19.1.542.gc4df23f792
Re: [PATCH 0/3] Make add_missing_tags() linear
On 11/1/2018 2:52 AM, Elijah Newren wrote: On Wed, Oct 31, 2018 at 5:05 AM Derrick Stolee wrote: On 10/31/2018 2:04 AM, Elijah Newren wrote: On the original repo where the topic was brought up, with commit-graph NOT turned on and using origin/master, I see: $ time git push --dry-run --follow-tags /home/newren/repo-mirror To /home/newren/repo-mirror * [new branch] test5 -> test5 real 1m20.081s user 1m19.688s sys 0m0.292s Merging this series in, I now get: $ time git push --dry-run --follow-tags /home/newren/repo-mirror To /home/newren/repo-mirror * [new branch] test5 -> test5 real 0m2.857s user 0m2.580s sys 0m0.328s which provides a very nice speedup. Oddly enough, if I _also_ do the following: $ git config core.commitgraph true $ git config gc.writecommitgraph true $ git gc then my timing actually slows down just slightly: $ time git push --follow-tags --dry-run /home/newren/repo-mirror To /home/newren/repo-mirror * [new branch] test5 -> test5 real 0m3.027s user 0m2.696s sys 0m0.400s So you say that the commit-graph is off in the 2.8s case, but not here in the 3.1s case? I would expect _at minimum_ that the cost of parsing commits would have a speedup in the commit-graph case. There may be something else going on here, since you are timing a `push` event that is doing more than the current walk. (run-to-run variation seems pretty consistent, < .1s variation, so this difference is just enough to notice.) I wouldn't be that surprised if that means there's some really old tags with very small generation numbers, meaning it's not gaining anything in this special case from the commit-graph, but it does pay the cost of loading the commit-graph. While you have this test environment, do you mind applying the diff below and re-running the tests? It will output a count for how many commits are walked by the algorithm. This should help us determine if this is another case where generation numbers are worse than commit-date, or if there is something else going on. Thanks! I can do that, but wouldn't you want a similar patch for the old get_merge_bases_many() in order to compare? Does an absolute number help by itself? It's going to have to be tomorrow, though; not enough time tonight. No rush. I'd just like to understand how removing the commit-graph file can make the new algorithm faster. Putting a similar count in the old algorithm would involve giving a count for every call to in_merge_bases_many(), which would be very noisy. Thanks! -Stolee
[PATCH 0/3] [Outreachy] make stash work if user.name and user.email are not configured
Enhancement request that ask for 'git stash' to work even if 'user.name' and 'user.email' are not configured. Due to an implementation detail, git-stash undesirably requires 'user.name' and 'user.email' to be set, but shouldn't. Slavica Djukic(3): [Outreachy] t3903-stash: test without configured user.name and user.email [Outreachy] ident: introduce set_fallback_ident() function [Outreachy] stash: use set_fallback_ident() function builtin/stash.c | 1 + cache.h | 1 + ident.c | 17 + t/t3903-stash.sh | 15 +++ 4 files changed, 34 insertions(+) -- 2.19.1.windows.1
Re: [PATCH 0/3] Make add_missing_tags() linear
On Wed, Oct 31, 2018 at 5:05 AM Derrick Stolee wrote: > > On 10/31/2018 2:04 AM, Elijah Newren wrote: > > On Tue, Oct 30, 2018 at 7:16 AM Derrick Stolee via GitGitGadget > > wrote: > >> > >> As reported earlier [1], the add_missing_tags() method in remote.c has > >> quadratic performance. Some of that performance is curbed due to the > >> generation-number cutoff in in_merge_bases_many(). However, that fix > >> doesn't > >> help users without a commit-graph, and it can still be painful if that > >> cutoff is sufficiently low compared to the tags we are using for > >> reachability testing. > >> > >> Add a new method in commit-reach.c called get_reachable_subset() which does > >> a many-to-many reachability test. Starting at the 'from' commits, walk > >> until > >> the generation is below the smallest generation in the 'to' commits, or all > >> 'to' commits have been discovered. This performs only one commit walk for > >> the entire add_missing_tags() method, giving linear performance in the > >> worst > >> case. > >> > >> Tests are added in t6600-test-reach.sh to ensure get_reachable_subset() > >> works independently of its application in add_missing_tags(). > > > > On the original repo where the topic was brought up, with commit-graph > > NOT turned on and using origin/master, I see: > > > > $ time git push --dry-run --follow-tags /home/newren/repo-mirror > > To /home/newren/repo-mirror > > * [new branch] test5 -> test5 > > > > real 1m20.081s > > user 1m19.688s > > sys 0m0.292s > > > > Merging this series in, I now get: > > > > $ time git push --dry-run --follow-tags /home/newren/repo-mirror > > To /home/newren/repo-mirror > > * [new branch] test5 -> test5 > > > > real 0m2.857s > > user 0m2.580s > > sys 0m0.328s > > > > which provides a very nice speedup. > > > > Oddly enough, if I _also_ do the following: > > $ git config core.commitgraph true > > $ git config gc.writecommitgraph true > > $ git gc > > > > then my timing actually slows down just slightly: > > $ time git push --follow-tags --dry-run /home/newren/repo-mirror > > To /home/newren/repo-mirror > > * [new branch] test5 -> test5 > > > > real 0m3.027s > > user 0m2.696s > > sys 0m0.400s > > So you say that the commit-graph is off in the 2.8s case, but not here > in the 3.1s case? I would expect _at minimum_ that the cost of parsing > commits would have a speedup in the commit-graph case. There may be > something else going on here, since you are timing a `push` event that > is doing more than the current walk. > > > (run-to-run variation seems pretty consistent, < .1s variation, so > > this difference is just enough to notice.) I wouldn't be that > > surprised if that means there's some really old tags with very small > > generation numbers, meaning it's not gaining anything in this special > > case from the commit-graph, but it does pay the cost of loading the > > commit-graph. > > While you have this test environment, do you mind applying the diff > below and re-running the tests? It will output a count for how many > commits are walked by the algorithm. This should help us determine if > this is another case where generation numbers are worse than commit-date, > or if there is something else going on. Thanks! I can do that, but wouldn't you want a similar patch for the old get_merge_bases_many() in order to compare? Does an absolute number help by itself? It's going to have to be tomorrow, though; not enough time tonight.
[PATCH 0/1] Make compat/poll safer on Windows
This is yet another piece from the Git for Windows cake. It avoids a wrap-around in the poll emulation on Windows that occurs every 49 days. Steve Hoelzer (1): poll: use GetTickCount64() to avoid wrap-around issues compat/poll/poll.c | 10 +++--- 1 file changed, 7 insertions(+), 3 deletions(-) base-commit: 4ede3d42dfb57f9a41ac96a1f216c62eb7566cc2 Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-64%2Fdscho%2Fmingw-safer-compat-poll-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-64/dscho/mingw-safer-compat-poll-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/64 -- gitgitgadget
Re: [PATCH 0/3] Make add_missing_tags() linear
On 10/31/2018 2:04 AM, Elijah Newren wrote:
> On Tue, Oct 30, 2018 at 7:16 AM Derrick Stolee via GitGitGadget
> wrote:
>>
>> As reported earlier [1], the add_missing_tags() method in remote.c has
>> quadratic performance. Some of that performance is curbed due to the
>> generation-number cutoff in in_merge_bases_many(). However, that fix doesn't
>> help users without a commit-graph, and it can still be painful if that
>> cutoff is sufficiently low compared to the tags we are using for
>> reachability testing.
>>
>> Add a new method in commit-reach.c called get_reachable_subset() which does
>> a many-to-many reachability test. Starting at the 'from' commits, walk until
>> the generation is below the smallest generation in the 'to' commits, or all
>> 'to' commits have been discovered. This performs only one commit walk for
>> the entire add_missing_tags() method, giving linear performance in the worst
>> case.
>>
>> Tests are added in t6600-test-reach.sh to ensure get_reachable_subset()
>> works independently of its application in add_missing_tags().
>
> On the original repo where the topic was brought up, with commit-graph
> NOT turned on and using origin/master, I see:
>
> $ time git push --dry-run --follow-tags /home/newren/repo-mirror
> To /home/newren/repo-mirror
> * [new branch] test5 -> test5
>
> real 1m20.081s
> user 1m19.688s
> sys 0m0.292s
>
> Merging this series in, I now get:
>
> $ time git push --dry-run --follow-tags /home/newren/repo-mirror
> To /home/newren/repo-mirror
> * [new branch] test5 -> test5
>
> real 0m2.857s
> user 0m2.580s
> sys 0m0.328s
>
> which provides a very nice speedup.
>
> Oddly enough, if I _also_ do the following:
> $ git config core.commitgraph true
> $ git config gc.writecommitgraph true
> $ git gc
>
> then my timing actually slows down just slightly:
> $ time git push --follow-tags --dry-run /home/newren/repo-mirror
> To /home/newren/repo-mirror
> * [new branch] test5 -> test5
>
> real 0m3.027s
> user 0m2.696s
> sys 0m0.400s
So you say that the commit-graph is off in the 2.8s case, but not here
in the 3.1s case? I would expect _at minimum_ that the cost of parsing
commits would have a speedup in the commit-graph case. There may be
something else going on here, since you are timing a `push` event that
is doing more than the current walk.
> (run-to-run variation seems pretty consistent, < .1s variation, so
> this difference is just enough to notice.) I wouldn't be that
> surprised if that means there's some really old tags with very small
> generation numbers, meaning it's not gaining anything in this special
> case from the commit-graph, but it does pay the cost of loading the
> commit-graph.
While you have this test environment, do you mind applying the diff
below and re-running the tests? It will output a count for how many
commits are walked by the algorithm. This should help us determine if
this is another case where generation numbers are worse than commit-date,
or if there is something else going on. Thanks!
-->8--
>From 2115e7dcafb2770455b7b4793f90edc2254bad97 Mon Sep 17 00:00:00 2001
From: Derrick Stolee
Date: Wed, 31 Oct 2018 11:40:50 +
Subject: [PATCH] DO-NOT-MERGE: count commits in get_reachable_subset
Signed-off-by: Derrick Stolee
---
commit-reach.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/commit-reach.c b/commit-reach.c
index a98532ecc8..b512461cf7 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -700,6 +700,7 @@ struct commit_list *get_reachable_subset(struct commit
**from, int nr_from,
struct commit **from_last = from + nr_from;
uint32_t min_generation = GENERATION_NUMBER_INFINITY;
int num_to_find = 0;
+ int count = 0;
struct prio_queue queue = { compare_commits_by_gen_then_commit_date };
@@ -729,6 +730,8 @@ struct commit_list *get_reachable_subset(struct commit
**from, int nr_from,
while (num_to_find && (current = prio_queue_get()) != NULL) {
struct commit_list *parents;
+ count++;
+
if (current->object.flags & PARENT1) {
current->object.flags &= ~PARENT1;
current->object.flags |= reachable_flag;
@@ -755,6 +758,8 @@ struct commit_list *get_reachable_subset(struct commit
**from, int nr_from,
clear_commit_marks_many(nr_to, to, PARENT1);
clear_commit_marks_many(nr_from, from, PARENT2);
+ fprintf(stderr, "count: %d\n", count);
+
return found_commits;
}
--
2.19.1.542.gc4df23f792
Re: [PATCH 0/3] Make add_missing_tags() linear
On Tue, Oct 30, 2018 at 7:16 AM Derrick Stolee via GitGitGadget wrote: > > As reported earlier [1], the add_missing_tags() method in remote.c has > quadratic performance. Some of that performance is curbed due to the > generation-number cutoff in in_merge_bases_many(). However, that fix doesn't > help users without a commit-graph, and it can still be painful if that > cutoff is sufficiently low compared to the tags we are using for > reachability testing. > > Add a new method in commit-reach.c called get_reachable_subset() which does > a many-to-many reachability test. Starting at the 'from' commits, walk until > the generation is below the smallest generation in the 'to' commits, or all > 'to' commits have been discovered. This performs only one commit walk for > the entire add_missing_tags() method, giving linear performance in the worst > case. > > Tests are added in t6600-test-reach.sh to ensure get_reachable_subset() > works independently of its application in add_missing_tags(). On the original repo where the topic was brought up, with commit-graph NOT turned on and using origin/master, I see: $ time git push --dry-run --follow-tags /home/newren/repo-mirror To /home/newren/repo-mirror * [new branch] test5 -> test5 real 1m20.081s user 1m19.688s sys 0m0.292s Merging this series in, I now get: $ time git push --dry-run --follow-tags /home/newren/repo-mirror To /home/newren/repo-mirror * [new branch] test5 -> test5 real 0m2.857s user 0m2.580s sys 0m0.328s which provides a very nice speedup. Oddly enough, if I _also_ do the following: $ git config core.commitgraph true $ git config gc.writecommitgraph true $ git gc then my timing actually slows down just slightly: $ time git push --follow-tags --dry-run /home/newren/repo-mirror To /home/newren/repo-mirror * [new branch] test5 -> test5 real 0m3.027s user 0m2.696s sys 0m0.400s (run-to-run variation seems pretty consistent, < .1s variation, so this difference is just enough to notice.) I wouldn't be that surprised if that means there's some really old tags with very small generation numbers, meaning it's not gaining anything in this special case from the commit-graph, but it does pay the cost of loading the commit-graph. Anyway, looks good in my testing. Thanks much for working on this!
Re: [PATCH 0/3] Make add_missing_tags() linear
"Derrick Stolee via GitGitGadget" writes: > Add a new method in commit-reach.c called get_reachable_subset() which does > a many-to-many reachability test. Starting at the 'from' commits, walk until > the generation is below the smallest generation in the 'to' commits, or all > 'to' commits have been discovered. This performs only one commit walk for > the entire add_missing_tags() method, giving linear performance in the worst > case. ;-) I think in_merge_bases_many() was an attempt to do one half of this (i.e. it checks only one 'to' against main 'from' to see if any of them reach). I wonder why we didn't extend it to multiple 'to' back when we invented it. In any case, good to see this code optimized. Thanks.
[PATCH 22/24] commit: make free_commit_buffer and release_commit_memory repository agnostic
Pass the object pool to free_commit_buffer and release_commit_memory,
such that we can eliminate access to 'the_repository'.
Also remove the TODO in release_commit_memory, as commit->util was
removed in 9d2c97016f (commit.h: delete 'util' field in struct commit,
2018-05-19)
Signed-off-by: Stefan Beller
---
builtin/fsck.c | 3 ++-
builtin/log.c | 6 --
builtin/rev-list.c | 3 ++-
commit.c | 9 -
commit.h | 4 ++--
object.c | 2 +-
6 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 06eb421720..c476ac6983 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -382,7 +382,8 @@ static int fsck_obj(struct object *obj, void *buffer,
unsigned long size)
if (obj->type == OBJ_TREE)
free_tree_buffer((struct tree *)obj);
if (obj->type == OBJ_COMMIT)
- free_commit_buffer((struct commit *)obj);
+ free_commit_buffer(the_repository->parsed_objects,
+ (struct commit *)obj);
return err;
}
diff --git a/builtin/log.c b/builtin/log.c
index 061d4fd864..64c2649c7c 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -395,7 +395,8 @@ static int cmd_log_walk(struct rev_info *rev)
* We may show a given commit multiple times when
* walking the reflogs.
*/
- free_commit_buffer(commit);
+ free_commit_buffer(the_repository->parsed_objects,
+ commit);
free_commit_list(commit->parents);
commit->parents = NULL;
}
@@ -1922,7 +1923,8 @@ int cmd_format_patch(int argc, const char **argv, const
char *prefix)
open_next_file(rev.numbered_files ? NULL : commit, NULL,
, quiet))
die(_("Failed to create output files"));
shown = log_tree_commit(, commit);
- free_commit_buffer(commit);
+ free_commit_buffer(the_repository->parsed_objects,
+ commit);
/* We put one extra blank line between formatted
* patches and this flag is used by log-tree code
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 5064d08e1b..a8867f0c4b 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -197,7 +197,8 @@ static void finish_commit(struct commit *commit, void *data)
free_commit_list(commit->parents);
commit->parents = NULL;
}
- free_commit_buffer(commit);
+ free_commit_buffer(the_repository->parsed_objects,
+ commit);
}
static inline void finish_object__ma(struct object *obj)
diff --git a/commit.c b/commit.c
index 7d2f3a9a93..4fe74aa4bc 100644
--- a/commit.c
+++ b/commit.c
@@ -328,10 +328,10 @@ void repo_unuse_commit_buffer(struct repository *r,
free((void *)buffer);
}
-void free_commit_buffer(struct commit *commit)
+void free_commit_buffer(struct parsed_object_pool *pool, struct commit *commit)
{
struct commit_buffer *v = buffer_slab_peek(
- the_repository->parsed_objects->buffer_slab, commit);
+ pool->buffer_slab, commit);
if (v) {
FREE_AND_NULL(v->buffer);
v->size = 0;
@@ -354,13 +354,12 @@ struct object_id *get_commit_tree_oid(const struct commit
*commit)
return _commit_tree(commit)->object.oid;
}
-void release_commit_memory(struct commit *c)
+void release_commit_memory(struct parsed_object_pool *pool, struct commit *c)
{
c->maybe_tree = NULL;
c->index = 0;
- free_commit_buffer(c);
+ free_commit_buffer(pool, c);
free_commit_list(c->parents);
- /* TODO: what about commit->util? */
c->object.parsed = 0;
}
diff --git a/commit.h b/commit.h
index 2e6b799b26..d2779a23f6 100644
--- a/commit.h
+++ b/commit.h
@@ -140,7 +140,7 @@ void repo_unuse_commit_buffer(struct repository *r,
/*
* Free any cached object buffer associated with the commit.
*/
-void free_commit_buffer(struct commit *);
+void free_commit_buffer(struct parsed_object_pool *pool, struct commit *);
struct tree *get_commit_tree(const struct commit *);
struct object_id *get_commit_tree_oid(const struct commit *);
@@ -149,7 +149,7 @@ struct object_id *get_commit_tree_oid(const struct commit
*);
* Release memory related to a commit, including the parent list and
* any cached object buffer.
*/
-void release_commit_memory(struct commit *c);
+void release_commit_memory(struct parsed_object_pool *pool, struct commit *c);
/*
* Disassociate any cached object buffer from the commit, but do not free it.
diff --git a/object.c b/object.c
index 003f870484..c4170d2d0c 100644
--- a/object.c
+++ b/object.c
@@ -540,7 +540,7 @@ void parsed_object_pool_clear(struct parsed_object_pool
[PATCH 23/24] path.h: make REPO_GIT_PATH_FUNC repository agnostic
git_pathdup uses the_repository internally, but the macro
REPO_GIT_PATH_FUNC is specifically made for arbitrary repositories.
Switch to repo_git_path which works on arbitrary repositories.
Signed-off-by: Stefan Beller
---
path.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/path.h b/path.h
index b654ea8ff5..651e6157fc 100644
--- a/path.h
+++ b/path.h
@@ -165,7 +165,7 @@ extern void report_linked_checkout_garbage(void);
const char *git_path_##var(struct repository *r) \
{ \
if (!r->cached_paths.var) \
- r->cached_paths.var = git_pathdup(filename); \
+ r->cached_paths.var = repo_git_path(r, filename); \
return r->cached_paths.var; \
}
--
2.19.1.930.g4563a0d9d0-goog
[PATCH 3/3] remote: make add_missing_tags() linear
From: Derrick Stolee
The add_missing_tags() method currently has quadratic behavior.
This is due to a linear number (based on number of tags T) of
calls to in_merge_bases_many, which has linear performance (based
on number of commits C in the repository).
Replace this O(T * C) algorithm with an O(T + C) algorithm by
using get_reachable_subset(). We ignore the return list and focus
instead on the reachable_flag assigned to the commits we care
about, because we need to interact with the tag ref and not just
the commit object.
Signed-off-by: Derrick Stolee
---
remote.c | 34 +-
1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/remote.c b/remote.c
index 81f4f01b0..b850f2feb 100644
--- a/remote.c
+++ b/remote.c
@@ -1205,9 +1205,36 @@ static void add_missing_tags(struct ref *src, struct ref
**dst, struct ref ***ds
* sent to the other side.
*/
if (sent_tips.nr) {
+ const int reachable_flag = 1;
+ struct commit_list *found_commits;
+ struct commit **src_commits;
+ int nr_src_commits = 0, alloc_src_commits = 16;
+ ALLOC_ARRAY(src_commits, alloc_src_commits);
+
for_each_string_list_item(item, _tag) {
struct ref *ref = item->util;
+ struct commit *commit;
+
+ if (is_null_oid(>new_oid))
+ continue;
+ commit = lookup_commit_reference_gently(the_repository,
+ >new_oid,
+ 1);
+ if (!commit)
+ /* not pushing a commit, which is not an error
*/
+ continue;
+
+ ALLOC_GROW(src_commits, nr_src_commits + 1,
alloc_src_commits);
+ src_commits[nr_src_commits++] = commit;
+ }
+
+ found_commits = get_reachable_subset(sent_tips.tip,
sent_tips.nr,
+src_commits,
nr_src_commits,
+reachable_flag);
+
+ for_each_string_list_item(item, _tag) {
struct ref *dst_ref;
+ struct ref *ref = item->util;
struct commit *commit;
if (is_null_oid(>new_oid))
@@ -1223,7 +1250,7 @@ static void add_missing_tags(struct ref *src, struct ref
**dst, struct ref ***ds
* Is this tag, which they do not have, reachable from
* any of the commits we are sending?
*/
- if (!in_merge_bases_many(commit, sent_tips.nr,
sent_tips.tip))
+ if (!(commit->object.flags & reachable_flag))
continue;
/* Add it in */
@@ -1231,7 +1258,12 @@ static void add_missing_tags(struct ref *src, struct ref
**dst, struct ref ***ds
oidcpy(_ref->new_oid, >new_oid);
dst_ref->peer_ref = copy_ref(ref);
}
+
+ clear_commit_marks_many(nr_src_commits, src_commits,
reachable_flag);
+ free(src_commits);
+ free_commit_list(found_commits);
}
+
string_list_clear(_tag, 0);
free(sent_tips.tip);
}
--
gitgitgadget
[PATCH 0/3] Make add_missing_tags() linear
As reported earlier [1], the add_missing_tags() method in remote.c has quadratic performance. Some of that performance is curbed due to the generation-number cutoff in in_merge_bases_many(). However, that fix doesn't help users without a commit-graph, and it can still be painful if that cutoff is sufficiently low compared to the tags we are using for reachability testing. Add a new method in commit-reach.c called get_reachable_subset() which does a many-to-many reachability test. Starting at the 'from' commits, walk until the generation is below the smallest generation in the 'to' commits, or all 'to' commits have been discovered. This performs only one commit walk for the entire add_missing_tags() method, giving linear performance in the worst case. Tests are added in t6600-test-reach.sh to ensure get_reachable_subset() works independently of its application in add_missing_tags(). Thanks, -Stolee [1] https://public-inbox.org/git/cabpp-becpsoxudovjbdg_3w9wus102rw+e+qpmd4g3qyd-q...@mail.gmail.com/ Derrick Stolee (3): commit-reach: implement get_reachable_subset test-reach: test get_reachable_subset remote: make add_missing_tags() linear commit-reach.c| 70 +++ commit-reach.h| 10 +++ remote.c | 34 - t/helper/test-reach.c | 34 ++--- t/t6600-test-reach.sh | 52 5 files changed, 195 insertions(+), 5 deletions(-) base-commit: c670b1f876521c9f7cd40184bf7ed05aad843433 Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-60%2Fderrickstolee%2Fadd-missing-tags-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-60/derrickstolee/add-missing-tags-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/60 -- gitgitgadget
[PATCH v2 08/16] sequencer: make sequencer_make_script() write its script to a strbuf
This makes sequencer_make_script() write its script to a strbuf (ie. the
buffer of a todo_list) instead of a FILE. This reduce the amount of
read/write made by rebase interactive.
Signed-off-by: Alban Gruin
---
builtin/rebase--interactive.c | 13 +++-
sequencer.c | 38 ---
sequencer.h | 2 +-
3 files changed, 26 insertions(+), 27 deletions(-)
diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
index f827e53f05..eef1ff2e83 100644
--- a/builtin/rebase--interactive.c
+++ b/builtin/rebase--interactive.c
@@ -71,7 +71,8 @@ static int do_interactive_rebase(struct replay_opts *opts,
unsigned flags,
const char *head_hash = NULL;
char *revisions = NULL, *shortrevisions = NULL;
struct argv_array make_script_args = ARGV_ARRAY_INIT;
- FILE *todo_list;
+ FILE *todo_list_file;
+ struct todo_list todo_list = TODO_LIST_INIT;
if (prepare_branch_to_be_rebased(opts, switch_to))
return -1;
@@ -93,8 +94,8 @@ static int do_interactive_rebase(struct replay_opts *opts,
unsigned flags,
if (!upstream && squash_onto)
write_file(path_squash_onto(), "%s\n", squash_onto);
- todo_list = fopen(rebase_path_todo(), "w");
- if (!todo_list) {
+ todo_list_file = fopen(rebase_path_todo(), "w");
+ if (!todo_list_file) {
free(revisions);
free(shortrevisions);
@@ -105,10 +106,11 @@ static int do_interactive_rebase(struct replay_opts
*opts, unsigned flags,
if (restrict_revision)
argv_array_push(_script_args, restrict_revision);
- ret = sequencer_make_script(todo_list,
+ ret = sequencer_make_script(_list.buf,
make_script_args.argc,
make_script_args.argv,
flags);
- fclose(todo_list);
+ fputs(todo_list.buf.buf, todo_list_file);
+ fclose(todo_list_file);
if (ret)
error(_("could not generate todo list"));
@@ -120,6 +122,7 @@ static int do_interactive_rebase(struct replay_opts *opts,
unsigned flags,
free(revisions);
free(shortrevisions);
+ todo_list_release(_list);
argv_array_clear(_script_args);
return ret;
diff --git a/sequencer.c b/sequencer.c
index 09e32f3e5a..94167588a2 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3927,7 +3927,7 @@ static const char *label_oid(struct object_id *oid, const
char *label,
}
static int make_script_with_merges(struct pretty_print_context *pp,
- struct rev_info *revs, FILE *out,
+ struct rev_info *revs, struct strbuf *out,
unsigned flags)
{
int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
@@ -4076,7 +4076,7 @@ static int make_script_with_merges(struct
pretty_print_context *pp,
* gathering commits not yet shown, reversing the list on the fly,
* then outputting that list (labeling revisions as needed).
*/
- fprintf(out, "%s onto\n", cmd_label);
+ strbuf_addf(out, "%s onto\n", cmd_label);
for (iter = tips; iter; iter = iter->next) {
struct commit_list *list = NULL, *iter2;
@@ -4086,9 +4086,9 @@ static int make_script_with_merges(struct
pretty_print_context *pp,
entry = oidmap_get(, >object.oid);
if (entry)
- fprintf(out, "\n# Branch %s\n", entry->string);
+ strbuf_addf(out, "\n%c Branch %s\n", comment_line_char,
entry->string);
else
- fprintf(out, "\n");
+ strbuf_addch(out, '\n');
while (oidset_contains(, >object.oid) &&
!oidset_contains(, >object.oid)) {
@@ -4101,8 +4101,8 @@ static int make_script_with_merges(struct
pretty_print_context *pp,
}
if (!commit)
- fprintf(out, "%s %s\n", cmd_reset,
- rebase_cousins ? "onto" : "[new root]");
+ strbuf_addf(out, "%s %s\n", cmd_reset,
+ rebase_cousins ? "onto" : "[new root]");
else {
const char *to = NULL;
@@ -4115,12 +4115,12 @@ static int make_script_with_merges(struct
pretty_print_context *pp,
);
if (!to || !strcmp(to, "onto"))
- fprintf(out, "%s onto\n", cmd_reset);
+ strbuf_addf(out, "%s onto\n", cmd_reset);
else {
strbuf_reset();
pretty_print_commit(pp, commit, );
- fprintf(out, "%s %s # %s\n",
- cmd_reset,
[PATCH v2 02/16] sequencer: make the todo_list structure public
This makes the structures todo_list and todo_item, and the functions
todo_list_release() and parse_insn_buffer(), accessible outside of
sequencer.c.
Signed-off-by: Alban Gruin
---
No changes since v1.
sequencer.c | 66 +
sequencer.h | 48 ++
2 files changed, 59 insertions(+), 55 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 9c8bd3f632..f791729271 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1401,31 +1401,6 @@ static int allow_empty(struct replay_opts *opts, struct
commit *commit)
return 1;
}
-/*
- * Note that ordering matters in this enum. Not only must it match the mapping
- * below, it is also divided into several sections that matter. When adding
- * new commands, make sure you add it in the right section.
- */
-enum todo_command {
- /* commands that handle commits */
- TODO_PICK = 0,
- TODO_REVERT,
- TODO_EDIT,
- TODO_REWORD,
- TODO_FIXUP,
- TODO_SQUASH,
- /* commands that do something else than handling a single commit */
- TODO_EXEC,
- TODO_LABEL,
- TODO_RESET,
- TODO_MERGE,
- /* commands that do nothing but are counted for reporting progress */
- TODO_NOOP,
- TODO_DROP,
- /* comments (not counted for reporting progress) */
- TODO_COMMENT
-};
-
static struct {
char c;
const char *str;
@@ -1897,26 +1872,7 @@ enum todo_item_flags {
TODO_EDIT_MERGE_MSG = 1
};
-struct todo_item {
- enum todo_command command;
- struct commit *commit;
- unsigned int flags;
- const char *arg;
- int arg_len;
- size_t offset_in_buf;
-};
-
-struct todo_list {
- struct strbuf buf;
- struct todo_item *items;
- int nr, alloc, current;
- int done_nr, total_nr;
- struct stat_data stat;
-};
-
-#define TODO_LIST_INIT { STRBUF_INIT }
-
-static void todo_list_release(struct todo_list *todo_list)
+void todo_list_release(struct todo_list *todo_list)
{
strbuf_release(_list->buf);
FREE_AND_NULL(todo_list->items);
@@ -2017,7 +1973,7 @@ static int parse_insn_line(struct todo_item *item, const
char *bol, char *eol)
return !item->commit;
}
-static int parse_insn_buffer(char *buf, struct todo_list *todo_list)
+int todo_list_parse_insn_buffer(char *buf, struct todo_list *todo_list)
{
struct todo_item *item;
char *p = buf, *next_p;
@@ -2115,7 +2071,7 @@ static int read_populate_todo(struct todo_list *todo_list,
return error(_("could not stat '%s'"), todo_file);
fill_stat_data(_list->stat, );
- res = parse_insn_buffer(todo_list->buf.buf, todo_list);
+ res = todo_list_parse_insn_buffer(todo_list->buf.buf, todo_list);
if (res) {
if (is_rebase_i(opts))
return error(_("please fix this using "
@@ -2146,7 +2102,7 @@ static int read_populate_todo(struct todo_list *todo_list,
FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w");
if (strbuf_read_file(, rebase_path_done(), 0) > 0 &&
- !parse_insn_buffer(done.buf.buf, ))
+ !todo_list_parse_insn_buffer(done.buf.buf,
))
todo_list->done_nr = count_commands();
else
todo_list->done_nr = 0;
@@ -4276,7 +4232,7 @@ int sequencer_add_exec_commands(const char *commands)
if (strbuf_read_file(_list.buf, todo_file, 0) < 0)
return error(_("could not read '%s'."), todo_file);
- if (parse_insn_buffer(todo_list.buf.buf, _list)) {
+ if (todo_list_parse_insn_buffer(todo_list.buf.buf, _list)) {
todo_list_release(_list);
return error(_("unusable todo list: '%s'"), todo_file);
}
@@ -4311,7 +4267,7 @@ int transform_todos(unsigned flags)
if (strbuf_read_file(_list.buf, todo_file, 0) < 0)
return error(_("could not read '%s'."), todo_file);
- if (parse_insn_buffer(todo_list.buf.buf, _list)) {
+ if (todo_list_parse_insn_buffer(todo_list.buf.buf, _list)) {
todo_list_release(_list);
return error(_("unusable todo list: '%s'"), todo_file);
}
@@ -4397,7 +4353,7 @@ int check_todo_list(void)
goto leave_check;
}
advise_to_edit_todo = res =
- parse_insn_buffer(todo_list.buf.buf, _list);
+ todo_list_parse_insn_buffer(todo_list.buf.buf, _list);
if (res || check_level == MISSING_COMMIT_CHECK_IGNORE)
goto leave_check;
@@ -4416,7 +4372,7 @@ int check_todo_list(void)
goto leave_check;
}
strbuf_release(_file);
- res = !!parse_insn_buffer(todo_list.buf.buf, _list);
Re: [PATCH] i18n: make GETTEXT_POISON a runtime option
On Sat, Oct 27 2018, Jeff King wrote: > On Fri, Oct 26, 2018 at 09:20:56PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> >> I was thinking: >> >> >> >> $ git var -e GIT_WHATEVER_ENV >> >> >> >> [-e for environment]. >> >> >> >> ... but that is really no different than git-config. ;-) >> > >> > Actually, "git var" already does pull bits from the environment. It >> > doesn't know about all of the type-specific parsing that git-config >> > does, but it might be a reasonable path forward to teach it that. (But I >> > still think we should do nothing for now and see how often this comes >> > up). >> >> For myself / Junio picking this up: Does that mean you've read v2 and >> think it's OK to pick up in its current form? I think it is, just >> looking for some Acks on that since it's not in the latest "What's >> Cooking". > > I'm not sure if you're asking whether I looked at the rest of the patch. > If so, then no, not really (so no objection, but I also did not review > it). Yeah I'm fishing for a more general review than just the problem of how we turn an env variable into a boolean, so if you or anyone else is up for it it would be most welcome :)
Re: [PATCH] i18n: make GETTEXT_POISON a runtime option
On Fri, Oct 26, 2018 at 09:20:56PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> I was thinking: > >> > >> $ git var -e GIT_WHATEVER_ENV > >> > >> [-e for environment]. > >> > >> ... but that is really no different than git-config. ;-) > > > > Actually, "git var" already does pull bits from the environment. It > > doesn't know about all of the type-specific parsing that git-config > > does, but it might be a reasonable path forward to teach it that. (But I > > still think we should do nothing for now and see how often this comes > > up). > > For myself / Junio picking this up: Does that mean you've read v2 and > think it's OK to pick up in its current form? I think it is, just > looking for some Acks on that since it's not in the latest "What's > Cooking". I'm not sure if you're asking whether I looked at the rest of the patch. If so, then no, not really (so no objection, but I also did not review it). -Peff
Re: [PATCH] i18n: make GETTEXT_POISON a runtime option
On Thu, Oct 25 2018, Jeff King wrote: > On Thu, Oct 25, 2018 at 02:24:41AM +0100, Ramsay Jones wrote: > >> >> Yeah, my thinko. The latter would be closer to what this patch >> >> wants to have, but obviously the former would be more flexible and >> >> useful in wider context. Both have the "Huh?" factor---what they >> >> are doing has little to do with "config", but I did not think of a >> >> better kitchen-sink (and our default kitchen-sink "rev-parse" is >> >> even further than "config", I would think, for this one). >> > >> > Heh, I thought through the exact sequence in your paragraph when writing >> > my other message. That's probably a good sign that we should probably >> > not pursue this further unless we see the use case come up again a few >> > more times (and if we do, then consider "config" the least-bad place to >> > do it). >> >> I was thinking: >> >> $ git var -e GIT_WHATEVER_ENV >> >> [-e for environment]. >> >> ... but that is really no different than git-config. ;-) > > Actually, "git var" already does pull bits from the environment. It > doesn't know about all of the type-specific parsing that git-config > does, but it might be a reasonable path forward to teach it that. (But I > still think we should do nothing for now and see how often this comes > up). For myself / Junio picking this up: Does that mean you've read v2 and think it's OK to pick up in its current form? I think it is, just looking for some Acks on that since it's not in the latest "What's Cooking".
Re: [PATCH v7 00/10] Make submodules work if .gitmodules is not checked out
On Thu, Oct 25, 2018 at 6:59 PM Junio C Hamano wrote: > > Stefan Beller writes: > > >> In this series I am addressing the comments by Stefan Beller about the > >> tests in patch 9. > >> > >> If the new tests look OK, I'd say we try moving the series to "next" and > >> see what happens? > > > > Sounds good to me. > > Which means (1) the plan sounds OK but I didn't look at these new > tests or (2) the new tests look OK and I am happy to see this go to > 'next'? I looked at the tests and found it a pleasant read, so I think the plan of merging it to next and seeing what will happen is a good one. Stefan
Re: [PATCH v7 00/10] Make submodules work if .gitmodules is not checked out
Stefan Beller writes: >> In this series I am addressing the comments by Stefan Beller about the >> tests in patch 9. >> >> If the new tests look OK, I'd say we try moving the series to "next" and >> see what happens? > > Sounds good to me. Which means (1) the plan sounds OK but I didn't look at these new tests or (2) the new tests look OK and I am happy to see this go to 'next'? tbdiff tells me that 9/10 is the only patch different from the previous round, and I vaguely recall that the other patches looked OK to me (even though I admit I only skimmed them quickly). Thanks.
Re: [PATCH] i18n: make GETTEXT_POISON a runtime option
On Thu, Oct 25, 2018 at 02:24:41AM +0100, Ramsay Jones wrote: > >> Yeah, my thinko. The latter would be closer to what this patch > >> wants to have, but obviously the former would be more flexible and > >> useful in wider context. Both have the "Huh?" factor---what they > >> are doing has little to do with "config", but I did not think of a > >> better kitchen-sink (and our default kitchen-sink "rev-parse" is > >> even further than "config", I would think, for this one). > > > > Heh, I thought through the exact sequence in your paragraph when writing > > my other message. That's probably a good sign that we should probably > > not pursue this further unless we see the use case come up again a few > > more times (and if we do, then consider "config" the least-bad place to > > do it). > > I was thinking: > > $ git var -e GIT_WHATEVER_ENV > > [-e for environment]. > > ... but that is really no different than git-config. ;-) Actually, "git var" already does pull bits from the environment. It doesn't know about all of the type-specific parsing that git-config does, but it might be a reasonable path forward to teach it that. (But I still think we should do nothing for now and see how often this comes up). -Peff
Re: [PATCH v7 00/10] Make submodules work if .gitmodules is not checked out
> In this series I am addressing the comments by Stefan Beller about the > tests in patch 9. > > If the new tests look OK, I'd say we try moving the series to "next" and > see what happens? Sounds good to me. Thanks, Stefan
[PATCH v7 00/10] Make submodules work if .gitmodules is not checked out
Hi, this series teaches git to try and read the .gitmodules file from the index (:.gitmodules) or from the current branch (HEAD:.gitmodules) when the file is not readily available in the working tree. This can be used, together with sparse checkouts, to enable submodule usage with programs like vcsh[1] which manage multiple repositories with their working trees sharing the same path. [1] https://github.com/RichiH/vcsh In the previous series there were some comments about not using the enum in patch 8, but I decided to leave the code as it was as I still think that it make sense to use an enum there, and have the default value unnamed; during the discussion I pointed out that other code in git do the same. In this series I am addressing the comments by Stefan Beller about the tests in patch 9. If the new tests look OK, I'd say we try moving the series to "next" and see what happens? I am available to address any further concerns in follow-up patches. v6 of the series is here: https://public-inbox.org/git/20181005130601.15879-1-...@ao2.it/ v5 of the series is here: https://public-inbox.org/git/20180917140940.3839-1-...@ao2.it/ v4 of the series is here: https://public-inbox.org/git/20180824132951.8000-1-...@ao2.it/ v3 of the series is here: https://public-inbox.org/git/20180814110525.17801-1-...@ao2.it/ v2 of the series is here: https://public-inbox.org/git/20180802134634.10300-1-...@ao2.it/ v1 of the series, with some background info, is here: https://public-inbox.org/git/20180514105823.8378-1-...@ao2.it/ Changes since v6: * Renamed t7416-submodule-sparse-gitmodules.sh to t7418-submodule-sparse-gitmodules.sh because t7416 was already taken. This has been already taken care of by Junio in "pu". * Improved tests in t7418: now, instead of just testing the return value of "git submodule ..." commands when .gitmodules is not in the working tree, the actual use case is checked in each test, with pre- and post-conditions. Thank you, Antonio Antonio Ospite (10): submodule: add a print_config_from_gitmodules() helper submodule: factor out a config_set_in_gitmodules_file_gently function t7411: merge tests 5 and 6 t7411: be nicer to future tests and really clean things up submodule--helper: add a new 'config' subcommand submodule: use the 'submodule--helper config' command t7506: clean up .gitmodules properly before setting up new scenario submodule: add a helper to check if it is safe to write to .gitmodules submodule: support reading .gitmodules when it's not in the working tree t/helper: add test-submodule-nested-repo-config Makefile | 1 + builtin/grep.c | 17 ++- builtin/submodule--helper.c | 40 ++ cache.h | 2 + git-submodule.sh | 13 +- submodule-config.c | 68 - submodule-config.h | 2 + submodule.c | 28 +++- submodule.h | 1 + t/helper/test-submodule-nested-repo-config.c | 30 t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/t7411-submodule-config.sh | 141 +-- t/t7418-submodule-sparse-gitmodules.sh | 122 t/t7506-status-submodule.sh | 3 +- t/t7814-grep-recurse-submodules.sh | 16 +++ 16 files changed, 454 insertions(+), 32 deletions(-) create mode 100644 t/helper/test-submodule-nested-repo-config.c create mode 100755 t/t7418-submodule-sparse-gitmodules.sh -- Antonio Ospite https://ao2.it https://twitter.com/ao2it A: Because it messes up the order in which people normally read text. See http://en.wikipedia.org/wiki/Posting_style Q: Why is top-posting such a bad thing?
Re: [PATCH v6 00/10] Make submodules work if .gitmodules is not checked out
On Thu, 25 Oct 2018 17:40:47 +0900 Junio C Hamano wrote: > Antonio Ospite writes: > > > this series teaches git to try and read the .gitmodules file from the > > index (:.gitmodules) or from the current branch (HEAD:.gitmodules) when > > the file is not readily available in the working tree. > > What you said in [*1*] the discussion on [09/10] sounded like you > are preparing an update of the series, so the topic is marked as > "Expecting a reroll" in the recent "What's cooking" report. At > least one topic now depends on the enhancement this topic makes, so > I'd like to know what the current status and ETA of the reroll would > be, in order to sort-of act as a traffic cop. > Hi Junio, I can send a v7 later today. It will only contain the improvements to 7416-submodule-sparse-gitmodules.sh as discussed in [*1*], it won't contain changes to patch 8 as motivated in https://public-inbox.org/git/20181008143709.dfcc845ab393c9caea660...@ao2.it/ I will also leave patch 10 unchanged, improvements can be made in follow-up patches. BTW, what is the new topic which depends on this one? Thank you, Antonio > [Reference] > > *1* > http://public-inbox.org/git/20181010205645.e1529eff9099805029b1d...@ao2.it/ -- Antonio Ospite https://ao2.it https://twitter.com/ao2it A: Because it messes up the order in which people normally read text. See http://en.wikipedia.org/wiki/Posting_style Q: Why is top-posting such a bad thing?
Re: [PATCH v6 00/10] Make submodules work if .gitmodules is not checked out
Antonio Ospite writes: > this series teaches git to try and read the .gitmodules file from the > index (:.gitmodules) or from the current branch (HEAD:.gitmodules) when > the file is not readily available in the working tree. What you said in [*1*] the discussion on [09/10] sounded like you are preparing an update of the series, so the topic is marked as "Expecting a reroll" in the recent "What's cooking" report. At least one topic now depends on the enhancement this topic makes, so I'd like to know what the current status and ETA of the reroll would be, in order to sort-of act as a traffic cop. Your answer could even be "I have been too busy, and I do not think an update will come for some time"---in other words, I do not mean to tell you to drop other things and work on this instead. If you are too busy, I can even see if other stakeholders (e.g. Stefan, whose topic now depends on this series) can take it over and update it after re-reading the discussion on the latest round. Thanks. [Reference] *1* http://public-inbox.org/git/20181010205645.e1529eff9099805029b1d...@ao2.it/
[PATCH v4 06/12] t: make the sha1 test-tool helper generic
Since we're going to have multiple hash algorithms to test, it makes
sense to share as much of the test code as possible. Convert the sha1
helper for the test-tool to be generic and move it out into its own
module. This will allow us to share most of this code with our NewHash
implementation.
Signed-off-by: brian m. carlson
---
Makefile | 1 +
t/helper/{test-sha1.c => test-hash.c} | 19 +-
t/helper/test-sha1.c | 52 +--
t/helper/test-tool.h | 2 ++
4 files changed, 14 insertions(+), 60 deletions(-)
copy t/helper/{test-sha1.c => test-hash.c} (65%)
diff --git a/Makefile b/Makefile
index d18ab0fe78..81dc9ac819 100644
--- a/Makefile
+++ b/Makefile
@@ -714,6 +714,7 @@ TEST_BUILTINS_OBJS += test-dump-split-index.o
TEST_BUILTINS_OBJS += test-dump-untracked-cache.o
TEST_BUILTINS_OBJS += test-example-decorate.o
TEST_BUILTINS_OBJS += test-genrandom.o
+TEST_BUILTINS_OBJS += test-hash.o
TEST_BUILTINS_OBJS += test-hashmap.o
TEST_BUILTINS_OBJS += test-index-version.o
TEST_BUILTINS_OBJS += test-json-writer.o
diff --git a/t/helper/test-sha1.c b/t/helper/test-hash.c
similarity index 65%
copy from t/helper/test-sha1.c
copy to t/helper/test-hash.c
index 1ba0675c75..0a31de66f3 100644
--- a/t/helper/test-sha1.c
+++ b/t/helper/test-hash.c
@@ -1,13 +1,14 @@
#include "test-tool.h"
#include "cache.h"
-int cmd__sha1(int ac, const char **av)
+int cmd_hash_impl(int ac, const char **av, int algo)
{
- git_SHA_CTX ctx;
- unsigned char sha1[20];
+ git_hash_ctx ctx;
+ unsigned char hash[GIT_MAX_HEXSZ];
unsigned bufsz = 8192;
int binary = 0;
char *buffer;
+ const struct git_hash_algo *algop = _algos[algo];
if (ac == 2) {
if (!strcmp(av[1], "-b"))
@@ -26,7 +27,7 @@ int cmd__sha1(int ac, const char **av)
die("OOPS");
}
- git_SHA1_Init();
+ algop->init_fn();
while (1) {
ssize_t sz, this_sz;
@@ -38,20 +39,20 @@ int cmd__sha1(int ac, const char **av)
if (sz == 0)
break;
if (sz < 0)
- die_errno("test-sha1");
+ die_errno("test-hash");
this_sz += sz;
cp += sz;
room -= sz;
}
if (this_sz == 0)
break;
- git_SHA1_Update(, buffer, this_sz);
+ algop->update_fn(, buffer, this_sz);
}
- git_SHA1_Final(sha1, );
+ algop->final_fn(hash, );
if (binary)
- fwrite(sha1, 1, 20, stdout);
+ fwrite(hash, 1, algop->rawsz, stdout);
else
- puts(sha1_to_hex(sha1));
+ puts(hash_to_hex_algop(hash, algop));
exit(0);
}
diff --git a/t/helper/test-sha1.c b/t/helper/test-sha1.c
index 1ba0675c75..d860c387c3 100644
--- a/t/helper/test-sha1.c
+++ b/t/helper/test-sha1.c
@@ -3,55 +3,5 @@
int cmd__sha1(int ac, const char **av)
{
- git_SHA_CTX ctx;
- unsigned char sha1[20];
- unsigned bufsz = 8192;
- int binary = 0;
- char *buffer;
-
- if (ac == 2) {
- if (!strcmp(av[1], "-b"))
- binary = 1;
- else
- bufsz = strtoul(av[1], NULL, 10) * 1024 * 1024;
- }
-
- if (!bufsz)
- bufsz = 8192;
-
- while ((buffer = malloc(bufsz)) == NULL) {
- fprintf(stderr, "bufsz %u is too big, halving...\n", bufsz);
- bufsz /= 2;
- if (bufsz < 1024)
- die("OOPS");
- }
-
- git_SHA1_Init();
-
- while (1) {
- ssize_t sz, this_sz;
- char *cp = buffer;
- unsigned room = bufsz;
- this_sz = 0;
- while (room) {
- sz = xread(0, cp, room);
- if (sz == 0)
- break;
- if (sz < 0)
- die_errno("test-sha1");
- this_sz += sz;
- cp += sz;
- room -= sz;
- }
- if (this_sz == 0)
- break;
- git_SHA1_Update(, buffer, this_sz);
- }
- git_SHA1_Final(sha1, );
-
- if (binary)
- fwrite(sha1, 1, 20, stdout);
- else
- puts(sha1_to_hex(sha1));
- exit(0);
+ return cmd_hash_impl(ac, av, GIT_HASH_SHA1);
}
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index e4890566da..29ac7b0b0d 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -50,4 +50,6 @@ int cmd__windows_named_pipe(int argc, const char **argv);
#endif
int cmd__write_cache(int argc, const char **argv);
[PATCH v4 04/12] cache: make hashcmp and hasheq work with larger hashes
In 183a638b7d ("hashcmp: assert constant hash size", 2018-08-23), we
modified hashcmp to assert that the hash size was always 20 to help it
optimize and inline calls to memcmp. In a future series, we replaced
many calls to hashcmp and oidcmp with calls to hasheq and oideq to
improve inlining further.
However, we want to support hash algorithms other than SHA-1, namely
SHA-256. When doing so, we must handle the case where these values are
32 bytes long as well as 20. Adjust hashcmp to handle two cases:
20-byte matches, and maximum-size matches. Therefore, when we include
SHA-256, we'll automatically handle it properly, while at the same time
teaching the compiler that there are only two possible options to
consider. This will allow the compiler to write the most efficient
possible code.
Copy similar code into hasheq and perform an identical transformation.
At least with GCC 8.2.0, making hasheq defer to hashcmp when there are
two branches prevents the compiler from inlining the comparison, while
the code in this patch is inlined properly. Add a comment to avoid an
accidental performance regression from well-intentioned refactoring.
Signed-off-by: brian m. carlson
---
cache.h | 22 --
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/cache.h b/cache.h
index 51580c4b77..bab8e8964f 100644
--- a/cache.h
+++ b/cache.h
@@ -1027,16 +1027,12 @@ extern const struct object_id null_oid;
static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)
{
/*
-* This is a temporary optimization hack. By asserting the size here,
-* we let the compiler know that it's always going to be 20, which lets
-* it turn this fixed-size memcmp into a few inline instructions.
-*
-* This will need to be extended or ripped out when we learn about
-* hashes of different sizes.
+* Teach the compiler that there are only two possibilities of hash size
+* here, so that it can optimize for this case as much as possible.
*/
- if (the_hash_algo->rawsz != 20)
- BUG("hash size not yet supported by hashcmp");
- return memcmp(sha1, sha2, the_hash_algo->rawsz);
+ if (the_hash_algo->rawsz == GIT_MAX_RAWSZ)
+ return memcmp(sha1, sha2, GIT_MAX_RAWSZ);
+ return memcmp(sha1, sha2, GIT_SHA1_RAWSZ);
}
static inline int oidcmp(const struct object_id *oid1, const struct object_id
*oid2)
@@ -1046,7 +1042,13 @@ static inline int oidcmp(const struct object_id *oid1,
const struct object_id *o
static inline int hasheq(const unsigned char *sha1, const unsigned char *sha2)
{
- return !hashcmp(sha1, sha2);
+ /*
+* We write this here instead of deferring to hashcmp so that the
+* compiler can properly inline it and avoid calling memcmp.
+*/
+ if (the_hash_algo->rawsz == GIT_MAX_RAWSZ)
+ return !memcmp(sha1, sha2, GIT_MAX_RAWSZ);
+ return !memcmp(sha1, sha2, GIT_SHA1_RAWSZ);
}
static inline int oideq(const struct object_id *oid1, const struct object_id
*oid2)
Re: [PATCH] i18n: make GETTEXT_POISON a runtime option
On 25/10/2018 02:09, Jeff King wrote: > On Thu, Oct 25, 2018 at 10:00:31AM +0900, Junio C Hamano wrote: > >> Jeff King writes: >> >>> but then you lose the default handling. I think if we added a new >>> option, it would either be: >>> >>> # interpret a value directly; use default on empty, I guess? >>> git config --default=false --type=bool --interpret-value >>> "$GIT_WHATEVER_ENV" >>> >>> or >>> >>> # less flexible, but the --default semantics are more obvious >>> git config --default=false --type=bool --get-env GIT_WHATEVER_ENV >> >> Yeah, my thinko. The latter would be closer to what this patch >> wants to have, but obviously the former would be more flexible and >> useful in wider context. Both have the "Huh?" factor---what they >> are doing has little to do with "config", but I did not think of a >> better kitchen-sink (and our default kitchen-sink "rev-parse" is >> even further than "config", I would think, for this one). > > Heh, I thought through the exact sequence in your paragraph when writing > my other message. That's probably a good sign that we should probably > not pursue this further unless we see the use case come up again a few > more times (and if we do, then consider "config" the least-bad place to > do it). I was thinking: $ git var -e GIT_WHATEVER_ENV [-e for environment]. ... but that is really no different than git-config. ;-) ATB, Ramsay Jones
Re: [PATCH] i18n: make GETTEXT_POISON a runtime option
On Thu, Oct 25, 2018 at 10:00:31AM +0900, Junio C Hamano wrote: > Jeff King writes: > > > but then you lose the default handling. I think if we added a new > > option, it would either be: > > > > # interpret a value directly; use default on empty, I guess? > > git config --default=false --type=bool --interpret-value > > "$GIT_WHATEVER_ENV" > > > > or > > > > # less flexible, but the --default semantics are more obvious > > git config --default=false --type=bool --get-env GIT_WHATEVER_ENV > > Yeah, my thinko. The latter would be closer to what this patch > wants to have, but obviously the former would be more flexible and > useful in wider context. Both have the "Huh?" factor---what they > are doing has little to do with "config", but I did not think of a > better kitchen-sink (and our default kitchen-sink "rev-parse" is > even further than "config", I would think, for this one). Heh, I thought through the exact sequence in your paragraph when writing my other message. That's probably a good sign that we should probably not pursue this further unless we see the use case come up again a few more times (and if we do, then consider "config" the least-bad place to do it). -Peff
Re: [PATCH] i18n: make GETTEXT_POISON a runtime option
Jeff King writes: > but then you lose the default handling. I think if we added a new > option, it would either be: > > # interpret a value directly; use default on empty, I guess? > git config --default=false --type=bool --interpret-value "$GIT_WHATEVER_ENV" > > or > > # less flexible, but the --default semantics are more obvious > git config --default=false --type=bool --get-env GIT_WHATEVER_ENV Yeah, my thinko. The latter would be closer to what this patch wants to have, but obviously the former would be more flexible and useful in wider context. Both have the "Huh?" factor---what they are doing has little to do with "config", but I did not think of a better kitchen-sink (and our default kitchen-sink "rev-parse" is even further than "config", I would think, for this one).
[PATCH v2] i18n: make GETTEXT_POISON a runtime option
ot doing:
>>
>>test_lazy_prereq GETTEXT_POISON 'test-tool gettext-poison'
>>test_lazy_prereq C_LOCALE_OUTPUT '! test-tool gettext-poison'
>>
>>In test-lib.sh is because there's some interpolation problem with
>>that syntax which makes t6040-tracking-info.sh fail. Hence using
>>the simpler test_set_prereq.
>
> s/In/in/, as you haven't finished the sentence yet. But more
> importantly, what is this "some interpolation problem"? Are you
> saying that test_lazy_prereq implementation is somehow broken and
> cannot take certain strings? If so, perhaps we want to fix that,
> and people other than you can help to do so, once you let them know
> what the problem is.
I haven't fixed (don't know what the fix is) this issue, but the new
version elaborates on what the error is.
>> See also
>> https://public-inbox.org/git/871s8gd32p@evledraar.gmail.com/ for
>> more discussion.
>
> "See also [*1*] for more discussion" as you've already have
> reference below.
Thanks, mispasted E-Mail ID. Fixed.
>>
>> 1. https://public-inbox.org/git/871s8gd32p@evledraar.gmail.com/
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason
>> ---
>>
>> Here's a polished version of that. I think it makes sense to queue
>> this up before any other refactoring of GETTEXT_POISON, and some patch
>> to unconditionally preserve format specifiers as I suggested upthread
>> could go on top of this.
>> ...
>> +int cmd_sh_i18n__helper(int argc, const char **argv, const char *prefix)
>> +{
>> +int poison = -1;
>> +struct option options[] = {
>> +OPT_BOOL(0, "git-test-gettext-poison", ,
>> + N_("is GIT_TEST_GETTEXT_POISON in effect?")),
>> +OPT_END()
>> +};
>> +
>> +argc = parse_options(argc, argv, NULL, options,
>> + builtin_sh_i18n_helper_usage,
>> PARSE_OPT_KEEP_ARGV0);
>> +
>> +if (poison != -1)
>> +return !git_env_bool("GIT_TEST_GETTEXT_POISON", 0);
>
> Hmm? If "--[no-]git-test-gettext-poison" is given, poison is either
> 0 or 1, and we return the value we read from the environment? What
> convoluted way to implement the option is that, or is there anything
> subtle that I am not getting?
>
> If the "default" parameter to git_env_bool() were poison, and then
> the option was renamed to "--get-git-text-gettext-poison", then I
> sort of understand the code, though (it's like "git config" with
> "--default" option).
Yeah this didn't make much sense. The test helper is now gone, and the
main helper doesn't use git_env_bool() but the utility function in
gettext.h.
> But if there is nothing subtle, it may make sense to implement this
> as an opt-cmdmode instead.
You mean something that works like "helper foo" instead of an option
via "helper --foo"? Seemed easier just to use the opt parse mechanism.
>> diff --git a/po/README b/po/README
>> index fef4c0f0b5..dba46c4a40 100644
>> --- a/po/README
>> +++ b/po/README
>> @@ -289,16 +289,11 @@ something in the test suite might still depend on the
>> US English
>> version of the strings, e.g. to grep some error message or other
>> output.
>>
>> -To smoke out issues like these Git can be compiled with gettext poison
>> -support, at the top-level:
>> +To smoke out issues like these Git tested with a translation mode that
>> +emits gibberish on every call to gettext. To use it run the test suite
>> +with it, e.g.:
>
> s/these Git tested/these, Git can be tested/; even though the comma
> is not a new issue you introduced.
Fixed. Range-diff of the whole thing below:
1: c93bf2f23f ! 1: 7bcb95a82d i18n: make GETTEXT_POISON a runtime option
@@ -24,19 +24,23 @@
This allows for conditionally amending tests to test with/without
poison, similar to what 859fdc0c3c ("commit-graph: define
-GIT_TEST_COMMIT_GRAPH", 2018-08-29) did for GIT_TEST_COMMIT_GRAPH, but
-this patch doesn't change any of the existing tests to work like that.
+GIT_TEST_COMMIT_GRAPH", 2018-08-29) did for GIT_TEST_COMMIT_GRAPH. Do
+some of that, now we e.g. always run the t0205-gettext-poison.sh test.
+
+I did enough there to remove the GETTEXT_POISON prerequisite, but its
+inverse C_LOCALE_OUTPUT is still around, and surely some tests using
+it can be converted to e.g. always set GIT_TEST_GETTEXT_POISON=false.
Notes on the implementation:
- * The only reason we need a new "git-sh-i18n--helper
Re: [PATCH] i18n: make GETTEXT_POISON a runtime option
On Wed, Oct 24, 2018 at 02:45:49PM +0900, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason writes: > > > Notes on the implementation: > > > > * The only reason we need a new "git-sh-i18n--helper" and the > >corresponding "test-tool gettext-poison" is to expose > >git_env_bool() to shellscripts, since git-sh-i18n and friends need > >to inspect the $GIT_TEST_GETTEXT_POISON variable. > > > >We only call these if $GIT_TEST_GETTEXT_POISON is set, or in the > >test suite, and this code can go away for non-test code once the > >last i18n-using shellscript is rewritten in C. > > Makes me wonder if we want to teach "git config" a new "--env-bool" > option that can be used by third-party scripts. Or would it be just > the matter of writing > > git config --default=false --type=bool "$GIT_WHATEVER_ENV" > > in these third-party scripts and we do not need to add such a thing? The config command only takes names, not values. So you'd have to write something like: git -c env.bool="$GIT_WHATEVER_ENV" config --type=bool env.bool but then you lose the default handling. I think if we added a new option, it would either be: # interpret a value directly; use default on empty, I guess? git config --default=false --type=bool --interpret-value "$GIT_WHATEVER_ENV" or # less flexible, but the --default semantics are more obvious git config --default=false --type=bool --get-env GIT_WHATEVER_ENV -Peff
Re: [PATCH] i18n: make GETTEXT_POISON a runtime option
Ævar Arnfjörð Bjarmason writes:
> Notes on the implementation:
>
> * The only reason we need a new "git-sh-i18n--helper" and the
>corresponding "test-tool gettext-poison" is to expose
>git_env_bool() to shellscripts, since git-sh-i18n and friends need
>to inspect the $GIT_TEST_GETTEXT_POISON variable.
>
>We only call these if $GIT_TEST_GETTEXT_POISON is set, or in the
>test suite, and this code can go away for non-test code once the
>last i18n-using shellscript is rewritten in C.
Makes me wonder if we want to teach "git config" a new "--env-bool"
option that can be used by third-party scripts. Or would it be just
the matter of writing
git config --default=false --type=bool "$GIT_WHATEVER_ENV"
in these third-party scripts and we do not need to add such a thing?
> * The reason for not doing:
>
>test_lazy_prereq GETTEXT_POISON 'test-tool gettext-poison'
>test_lazy_prereq C_LOCALE_OUTPUT '! test-tool gettext-poison'
>
>In test-lib.sh is because there's some interpolation problem with
>that syntax which makes t6040-tracking-info.sh fail. Hence using
>the simpler test_set_prereq.
s/In/in/, as you haven't finished the sentence yet. But more
importantly, what is this "some interpolation problem"? Are you
saying that test_lazy_prereq implementation is somehow broken and
cannot take certain strings? If so, perhaps we want to fix that,
and people other than you can help to do so, once you let them know
what the problem is.
> See also
> https://public-inbox.org/git/871s8gd32p@evledraar.gmail.com/ for
> more discussion.
"See also [*1*] for more discussion" as you've already have
reference below.
>
> 1. https://public-inbox.org/git/871s8gd32p@evledraar.gmail.com/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
>
> Here's a polished version of that. I think it makes sense to queue
> this up before any other refactoring of GETTEXT_POISON, and some patch
> to unconditionally preserve format specifiers as I suggested upthread
> could go on top of this.
> ...
> +int cmd_sh_i18n__helper(int argc, const char **argv, const char *prefix)
> +{
> + int poison = -1;
> + struct option options[] = {
> + OPT_BOOL(0, "git-test-gettext-poison", ,
> + N_("is GIT_TEST_GETTEXT_POISON in effect?")),
> + OPT_END()
> + };
> +
> + argc = parse_options(argc, argv, NULL, options,
> + builtin_sh_i18n_helper_usage,
> PARSE_OPT_KEEP_ARGV0);
> +
> + if (poison != -1)
> + return !git_env_bool("GIT_TEST_GETTEXT_POISON", 0);
Hmm? If "--[no-]git-test-gettext-poison" is given, poison is either
0 or 1, and we return the value we read from the environment? What
convoluted way to implement the option is that, or is there anything
subtle that I am not getting?
If the "default" parameter to git_env_bool() were poison, and then
the option was renamed to "--get-git-text-gettext-poison", then I
sort of understand the code, though (it's like "git config" with
"--default" option).
But if there is nothing subtle, it may make sense to implement this
as an opt-cmdmode instead.
> diff --git a/po/README b/po/README
> index fef4c0f0b5..dba46c4a40 100644
> --- a/po/README
> +++ b/po/README
> @@ -289,16 +289,11 @@ something in the test suite might still depend on the
> US English
> version of the strings, e.g. to grep some error message or other
> output.
>
> -To smoke out issues like these Git can be compiled with gettext poison
> -support, at the top-level:
> +To smoke out issues like these Git tested with a translation mode that
> +emits gibberish on every call to gettext. To use it run the test suite
> +with it, e.g.:
s/these Git tested/these, Git can be tested/; even though the comma
is not a new issue you introduced.
