[PATCH 2/4] git-init.txt: do not nest open blocks
It appears we try to nest open blocks, but that does not work well with Asciidoctor, which fails to indent the inner block. This causes the inner block to be set as if it had nothing to do with `--shared`. Drop the outer one. Asciidoc renders identically before and after this patch, both man-page and html. Signed-off-by: Martin Ågren --- See https://git-scm.com/docs/git-init#git-init---sharedfalsetrueumaskgroupallworldeverybody0xxx Documentation/git-init.txt | 4 1 file changed, 4 deletions(-) diff --git a/Documentation/git-init.txt b/Documentation/git-init.txt index 3c5a67fb96..057076ca38 100644 --- a/Documentation/git-init.txt +++ b/Documentation/git-init.txt @@ -38,8 +38,6 @@ the repository to another place if --separate-git-dir is given). OPTIONS --- --- - -q:: --quiet:: @@ -111,8 +109,6 @@ into it. If you provide a 'directory', the command is run inside it. If this directory does not exist, it will be created. --- - TEMPLATE DIRECTORY -- -- 2.20.1
[PATCH 4/4] git-status.txt: render tables correctly under Asciidoctor
Asciidoctor removes the indentation of each line in these tables, so the
last lines of each table have a completely broken alignment.
Similar to 379805051d ("Documentation: render revisions correctly under
Asciidoctor", 2018-05-06), use an explicit literal block to indicate
that we want to keep the leading whitespace in the tables.
Because this gives us some extra indentation, we can remove the one that
we have been carrying explicitly. That is, drop the first four spaces of
indentation on each line. With Asciidoc (8.6.10), this results in
identical rendering before and after this commit, both for git-status.1
and git-status.html.
Signed-off-by: Martin Ågren
---
See https://git-scm.com/docs/git-status#_short_format ,
https://git-scm.com/docs/git-status#_branch_headers and
https://git-scm.com/docs/git-status#_changed_tracked_entries .
Documentation/git-status.txt | 162 ++-
1 file changed, 85 insertions(+), 77 deletions(-)
diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index d9f422d560..861d821d7f 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -197,31 +197,33 @@ codes can be interpreted as follows:
Ignored files are not listed, unless `--ignored` option is in effect,
in which case `XY` are `!!`.
-X Y Meaning
--
-[AMD] not updated
-M[ MD] updated in index
-A[ MD] added to index
-Ddeleted from index
-R[ MD] renamed in index
-C[ MD] copied in index
-[MARC] index and work tree matches
-[ MARC] Mwork tree changed since index
-[ MARC] Ddeleted in work tree
-[ D]Rrenamed in work tree
-[ D]Ccopied in work tree
--
-D Dunmerged, both deleted
-A Uunmerged, added by us
-U Dunmerged, deleted by them
-U Aunmerged, added by them
-D Uunmerged, deleted by us
-A Aunmerged, both added
-U Uunmerged, both modified
--
-? ?untracked
-! !ignored
--
+
+X Y Meaning
+-
+[AMD] not updated
+M[ MD] updated in index
+A[ MD] added to index
+Ddeleted from index
+R[ MD] renamed in index
+C[ MD] copied in index
+[MARC] index and work tree matches
+[ MARC] Mwork tree changed since index
+[ MARC] Ddeleted in work tree
+[ D]Rrenamed in work tree
+[ D]Ccopied in work tree
+-
+D Dunmerged, both deleted
+A Uunmerged, added by us
+U Dunmerged, deleted by them
+U Aunmerged, added by them
+D Uunmerged, deleted by us
+A Aunmerged, both added
+U Uunmerged, both modified
+-
+? ?untracked
+! !ignored
+-
+
Submodules have more state and instead report
Mthe submodule has a different HEAD than
@@ -281,14 +283,16 @@ don't recognize.
If `--branch` is given, a series of header lines are printed with
information about the current branch.
-Line Notes
-
-# branch.oid | (initial)Current commit.
-# branch.head | (detached) Current branch.
-# branch.upstream If upstream is set.
-# branch.ab + - If upstream is set and
-the commit is present.
-
+
+Line Notes
+
+# branch.oid | (initial)Current commit.
+# branch.head | (detached) Current branch.
+# branch.upstream If upstream is set.
+# branch.ab + - If upstream is set and
+the commit is present.
+
+
### Changed Tracked Entries
@@ -306,56 +310,60 @@ Renamed or copied entries have the following format:
2
-Field Meaning
-
-A 2 character field containing the staged and
- unstaged XY values described in the short format,
- with unchanged indicated by a "." rather tha
[PATCH 0/4] A few Asciidoctor-fixes
I have (ab)used doc-diff to try to find instances where Asciidoctor and Asciidoc render our documentation differently. (See [1] for details on the hack.) This series fixes the differences that somehow stood out in the diff. Many smaller differences remain. With each patch, I am giving one or a few URLs which show the current Asciidoctor rendering at git-scm.com. With Asciidoc, `doc-diff origin/master HEAD` reports that there is no difference. [1] https://public-inbox.org/git/can0hesob+ac4bvm-vukvpzkxrtw53-d91a55nx5kc7zynxn...@mail.gmail.com/ Martin Ågren (4): git-column.txt: fix section header git-init.txt: do not nest open blocks rev-list-options.txt: do not nest open blocks git-status.txt: render tables correctly under Asciidoctor Documentation/git-column.txt | 2 +- Documentation/git-init.txt | 4 - Documentation/git-status.txt | 162 +++-- Documentation/rev-list-options.txt | 4 - 4 files changed, 86 insertions(+), 86 deletions(-) -- 2.20.1
[PATCH 3/4] rev-list-options.txt: do not nest open blocks
Similar to the previous commit, it appears we try to nest open blocks, which does not work well with Asciidoctor. Drop the outer one. That fixes the indentation similar to the previous patch, and makes us stop rendering a literal '+' before "Under --pretty=oneline ...". Asciidoc renders identically before and after this patch, both man-page and html. Signed-off-by: Martin Ågren --- See https://git-scm.com/docs/git-log#git-log--g and https://git-scm.com/docs/git-rev-list#git-rev-list--g . The indentation doesn't look too bad, but the "+ Under" is clearly wrong. Documentation/rev-list-options.txt | 4 1 file changed, 4 deletions(-) diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt index bab5f50b17..98b538bc77 100644 --- a/Documentation/rev-list-options.txt +++ b/Documentation/rev-list-options.txt @@ -13,8 +13,6 @@ has a line that matches ``), unless otherwise noted. Note that these are applied before commit ordering and formatting options, such as `--reverse`. --- - -:: -n :: --max-count=:: @@ -308,8 +306,6 @@ ifdef::git-rev-list[] `` text will be printed with each progress update. endif::git-rev-list[] --- - History Simplification ~~ -- 2.20.1
[PATCH 1/4] git-column.txt: fix section header
We have too few dashes under "Examples", which causes Asciidoctor to not pick it up as a section header, but to instead just render the dashes literally. This also seems to confuse Asciidoctor about dashes in general around here. It misinterprets the listing blocks in this section, again rendering the dashes literally. Make sure we have as many dashes as we have characters in "Examples". This fixes the section-issue and, somehow, makes the listing blocks render correctly. Asciidoc renders identically before and after this patch, both man-page and html. Signed-off-by: Martin Ågren --- See https://git-scm.com/docs/git-column Documentation/git-column.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/git-column.txt b/Documentation/git-column.txt index 763afabb6d..f58e9c43e6 100644 --- a/Documentation/git-column.txt +++ b/Documentation/git-column.txt @@ -47,7 +47,7 @@ OPTIONS The number of spaces between columns. One space by default. EXAMPLES --- + Format data by columns: -- 2.20.1
Re: [PATCH 1/4] git-column.txt: fix section header
On Sun, 16 Dec 2018 at 11:51, Jeff King wrote: > > On Sat, Dec 15, 2018 at 12:27:39PM +0100, Martin Ågren wrote: > > > We have too few dashes under "Examples", which causes Asciidoctor to not > > pick it up as a section header, but to instead just render the dashes > > literally. This also seems to confuse Asciidoctor about dashes in > > general around here. It misinterprets the listing blocks in this > > section, again rendering the dashes literally. > > > > Make sure we have as many dashes as we have characters in "Examples". > > This fixes the section-issue and, somehow, makes the listing blocks > > render correctly. > > The "somehow" here is that the mismatched dashes appear to be the start > of a code listing block, making the whole example section into one big > listing (which isn't properly closed, generating a warning). Of course. Thanks for pointing it out. I now realize that when I wrote "render the dashes literally", I was wrong. This should be improved in v2. Martin
[PATCH v2 1/3] git-column.txt: fix section header
We have too few dashes under "Examples", which causes Asciidoctor to not pick it up as a section header. Instead, it thinks we are starting a code listing block, which ends up containing the remainder of the document. The result is quite ugly. Make sure we have as many dashes as we have characters in "Examples". Asciidoc renders identically before and after this patch, both man-page and html. Signed-off-by: Martin Ågren --- Documentation/git-column.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/git-column.txt b/Documentation/git-column.txt index 763afabb6d..f58e9c43e6 100644 --- a/Documentation/git-column.txt +++ b/Documentation/git-column.txt @@ -47,7 +47,7 @@ OPTIONS The number of spaces between columns. One space by default. EXAMPLES --- + Format data by columns: -- 2.20.1
[PATCH v2 2/3] Documentation: do not nest open blocks
It appears we try to nest open blocks, but that does not work well with Asciidoctor, which fails to indent the inner blocks. As a result, they do not visually seem to relate (as much) to the preceding paragraph as they should. Drop the outer blocks to fix the rendering of the inner ones. Asciidoc renders identically before and after this patch, both man-pages and html. This also makes Asciidoctor stop rendering a literal '+' before "Under --pretty=oneline ..." in the manuals for git-log and git-rev-list. Signed-off-by: Martin Ågren --- Documentation/git-init.txt | 4 Documentation/rev-list-options.txt | 4 2 files changed, 8 deletions(-) diff --git a/Documentation/git-init.txt b/Documentation/git-init.txt index 3c5a67fb96..057076ca38 100644 --- a/Documentation/git-init.txt +++ b/Documentation/git-init.txt @@ -38,8 +38,6 @@ the repository to another place if --separate-git-dir is given). OPTIONS --- --- - -q:: --quiet:: @@ -111,8 +109,6 @@ into it. If you provide a 'directory', the command is run inside it. If this directory does not exist, it will be created. --- - TEMPLATE DIRECTORY -- diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt index bab5f50b17..98b538bc77 100644 --- a/Documentation/rev-list-options.txt +++ b/Documentation/rev-list-options.txt @@ -13,8 +13,6 @@ has a line that matches ``), unless otherwise noted. Note that these are applied before commit ordering and formatting options, such as `--reverse`. --- - -:: -n :: --max-count=:: @@ -308,8 +306,6 @@ ifdef::git-rev-list[] `` text will be printed with each progress update. endif::git-rev-list[] --- - History Simplification ~~ -- 2.20.1
[PATCH v2 3/3] git-status.txt: render tables correctly under Asciidoctor
Asciidoctor removes the indentation of each line in these tables, so the
last lines of each table have a completely broken alignment.
Similar to 379805051d ("Documentation: render revisions correctly under
Asciidoctor", 2018-05-06), use an explicit literal block to indicate
that we want to keep the leading whitespace in the tables.
Because this gives us some extra indentation, we can remove the one that
we have been carrying explicitly. That is, drop the first four spaces of
indentation on each line. With Asciidoc (8.6.10), this results in
identical rendering before and after this commit, both for git-status.1
and git-status.html.
Signed-off-by: Martin Ågren
---
Documentation/git-status.txt | 162 ++-
1 file changed, 85 insertions(+), 77 deletions(-)
diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index d9f422d560..861d821d7f 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -197,31 +197,33 @@ codes can be interpreted as follows:
Ignored files are not listed, unless `--ignored` option is in effect,
in which case `XY` are `!!`.
-X Y Meaning
--
-[AMD] not updated
-M[ MD] updated in index
-A[ MD] added to index
-Ddeleted from index
-R[ MD] renamed in index
-C[ MD] copied in index
-[MARC] index and work tree matches
-[ MARC] Mwork tree changed since index
-[ MARC] Ddeleted in work tree
-[ D]Rrenamed in work tree
-[ D]Ccopied in work tree
--
-D Dunmerged, both deleted
-A Uunmerged, added by us
-U Dunmerged, deleted by them
-U Aunmerged, added by them
-D Uunmerged, deleted by us
-A Aunmerged, both added
-U Uunmerged, both modified
--
-? ?untracked
-! !ignored
--
+
+X Y Meaning
+-
+[AMD] not updated
+M[ MD] updated in index
+A[ MD] added to index
+Ddeleted from index
+R[ MD] renamed in index
+C[ MD] copied in index
+[MARC] index and work tree matches
+[ MARC] Mwork tree changed since index
+[ MARC] Ddeleted in work tree
+[ D]Rrenamed in work tree
+[ D]Ccopied in work tree
+-
+D Dunmerged, both deleted
+A Uunmerged, added by us
+U Dunmerged, deleted by them
+U Aunmerged, added by them
+D Uunmerged, deleted by us
+A Aunmerged, both added
+U Uunmerged, both modified
+-
+? ?untracked
+! !ignored
+-
+
Submodules have more state and instead report
Mthe submodule has a different HEAD than
@@ -281,14 +283,16 @@ don't recognize.
If `--branch` is given, a series of header lines are printed with
information about the current branch.
-Line Notes
-
-# branch.oid | (initial)Current commit.
-# branch.head | (detached) Current branch.
-# branch.upstream If upstream is set.
-# branch.ab + - If upstream is set and
-the commit is present.
-
+
+Line Notes
+
+# branch.oid | (initial)Current commit.
+# branch.head | (detached) Current branch.
+# branch.upstream If upstream is set.
+# branch.ab + - If upstream is set and
+the commit is present.
+
+
### Changed Tracked Entries
@@ -306,56 +310,60 @@ Renamed or copied entries have the following format:
2
-Field Meaning
-
-A 2 character field containing the staged and
- unstaged XY values described in the short format,
- with unchanged indicated by a "." rather than
- a space.
- A 4 character field describing the submodule state.
- "N..." when the entry is not a submodule.
-
[PATCH v2 0/3] A few Asciidoctor-fixes
This series addresses a few instances where our documentation renders badly in Asciidoctor, compared to Asciidoc. The changes made are exactly the same as in v1 [1], but the first commit message is corrected, and the two patches 2/4 and 3/4 are now just a single patch 2/3. Thanks Peff for your comments on v1. [1] https://public-inbox.org/git/20181215112742.1475882-1-martin.ag...@gmail.com/ Martin Ågren (3): git-column.txt: fix section header Documentation: do not nest open blocks git-status.txt: render tables correctly under Asciidoctor Documentation/git-column.txt | 2 +- Documentation/git-init.txt | 4 - Documentation/git-status.txt | 162 +++-- Documentation/rev-list-options.txt | 4 - 4 files changed, 86 insertions(+), 86 deletions(-) -- 2.20.1
Re: [PATCH 0/4] A few Asciidoctor-fixes
On Sun, 16 Dec 2018 at 11:45, Jeff King wrote: > > On Sat, Dec 15, 2018 at 12:27:38PM +0100, Martin Ågren wrote: > I think the relevant bits from [1] are: > > * Use `make --always-make ... install-man` in doc-diff. > * ./doc-diff -f HEAD HEAD # note -f > * Add empty commit and tweak config.mak > * ./doc-diff HEAD^ HEAD # note no -f Yes, that's it. > To make this easier, it would make sense to me to: > > - teach doc-diff a flag for marking one or both of the endpoints to be > built with asciidoctor versus asciidoc > > - mark the asciidoc/asciidoctor in the directory name. That name > serves as a cache key for avoiding re-doing the work, you should be > able to just: > > ./doc-diff --asciidoctor HEAD HEAD > > and actually build and compare what you want. > > - it sounds from "make --always-make" that our Makefile does not > properly rebuild when we switch from asciidoc to asciidoctor. That Exactly. > might be nice to fix with a mechanism similar to the GIT-BUILD-FLAGS > we use in the top-level Makefile. Agreed on all three points. The last one would supposedly be useful on its own, beyond this doc-diff motivation. Your list seems complete to me in terms of "how could we teach doc-diff to diff asciidoctor vs asciidoc?". For the resulting diff to actually be useful ;-) there are two more outstanding issues that I see: - Headers and footers. Asciidoc (driven by doc-diff) uses some boilerplate values which avoid timestamps and the like. Asciidoctor partly uses different values, partly interprets the ones given differently. - Asciidoctor introduces a space after linkgit:foo , e.g., before punctuation. Both of these are problems in their own right, so they probably shouldn't be suppressed in the resulting diff. But as long as these issues remain, they produce a lot of noise which might hide more interesting (IMHO) differences. Martin
Re: [PATCH] stripspace: allow -s/-c outside git repository
On Mon, 17 Dec 2018 at 22:56, Jonathan Nieder wrote:
> That makes experimenting with the stripspace command unnecessarily
> fussy. Fix it by discovering the git directory gently, as intended
> all along.
> if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
> - setup_git_directory_gently(NULL);
> + setup_git_directory_gently(&nongit);
> git_config(git_default_config, NULL);
> }
Makes me wonder if `setup_git_directory_gently()` should BUG if it
receives NULL. That would require some reshuffling in setup.c, since
`setup_git_directory()` calls out to it with NULL... Just thinking out
loud. In any case, and more importantly, this is the last user providing
NULL except for the one in `setup_git_directory()`.
> diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
> index 5ce47e8af5..0c24a0f9a3 100755
> --- a/t/t0030-stripspace.sh
> +++ b/t/t0030-stripspace.sh
> @@ -430,9 +430,15 @@ test_expect_success '-c with changed comment char' '
> test_expect_success '-c with comment char defined in .git/config' '
> test_config core.commentchar = &&
> printf "= foo\n" >expect &&
> - printf "foo" | (
> - mkdir sub && cd sub && git stripspace -c
> - ) >actual &&
> + rm -fr sub &&
> + mkdir sub &&
> + printf "foo" | git -C sub stripspace -c >actual &&
> + test_cmp expect actual
> +'
A small while-at-it conversion from subshell (with a funny pipe into it)
to `-C sub`. The `rm -fr` invocation is not in the original, so
shouldn't be needed. This one looks safe enough, though it makes me
wonder why you need it. `mkdir -p sub`? Probably not worth a v2.
> +
> +test_expect_success '-c outside git repository' '
> + printf "# foo\n" >expect &&
> + printf "foo" | nongit git stripspace -c >actual &&
> test_cmp expect actual
> '
Nice.
Martin
[PATCH 0/3] setup: add `clear_repository_format()`
Patch 3 adds `clear_repository_format()` to plug a few memory leaks related to `struct repository_format`. In preparation for that, patch 2 tightens up some code which uses a possibly-undefined value from the struct. Patch 1 drops a return value that no-one has ever used. Cc-ing Peff, who introduced `struct repository_format`, and brian, who introduced its `hash_algo` field (see patch 2). Martin Ågren (3): setup: drop return value from `read_repository_format()` setup: do not use invalid `repository_format` setup: add `clear_repository_format()` cache.h | 13 + repository.c | 1 + setup.c | 19 --- 3 files changed, 26 insertions(+), 7 deletions(-) -- 2.20.1
[PATCH 1/3] setup: drop return value from `read_repository_format()`
No-one looks at the return value, so we might as well drop it. It's
still available as `format->version`.
In v1 of what became commit 2cc7c2c737 ("setup: refactor repo format
reading and verification", 2016-03-11), this function actually had
return type "void", but that was changed in v2. Almost three years
later, no-one has used this return value.
Signed-off-by: Martin Ågren
---
I only discovered the full history after writing the patch. Had I known
it from the beginning, maybe I'd have just skipped this step, but I was
sufficiently disturbed by the redundant and unused return value that I
dropped it before working on the actual meat of this series.
cache.h | 7 +++
setup.c | 3 +--
2 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/cache.h b/cache.h
index ca36b44ee0..8b9e592c65 100644
--- a/cache.h
+++ b/cache.h
@@ -974,11 +974,10 @@ struct repository_format {
/*
* Read the repository format characteristics from the config file "path" into
- * "format" struct. Returns the numeric version. On error, -1 is returned,
- * format->version is set to -1, and all other fields in the struct are
- * undefined.
+ * "format" struct. On error, format->version is set to -1, and all other
+ * fields in the struct are undefined.
*/
-int read_repository_format(struct repository_format *format, const char *path);
+void read_repository_format(struct repository_format *format, const char
*path);
/*
* Verify that the repository described by repository_format is something we
diff --git a/setup.c b/setup.c
index 1be5037f12..27747af7a3 100644
--- a/setup.c
+++ b/setup.c
@@ -509,7 +509,7 @@ static int check_repository_format_gently(const char
*gitdir, struct repository_
return 0;
}
-int read_repository_format(struct repository_format *format, const char *path)
+void read_repository_format(struct repository_format *format, const char *path)
{
memset(format, 0, sizeof(*format));
format->version = -1;
@@ -517,7 +517,6 @@ int read_repository_format(struct repository_format
*format, const char *path)
format->hash_algo = GIT_HASH_SHA1;
string_list_init(&format->unknown_extensions, 1);
git_config_from_file(check_repo_format, path, format);
- return format->version;
}
int verify_repository_format(const struct repository_format *format,
--
2.20.1
[PATCH 3/3] setup: add `clear_repository_format()`
After we set up a `struct repository_format`, it owns various pieces of
allocated memory. We then either use those members, because we decide we
want to use the "candidate" repository format, or we discard the
candidate / scratch space. In the first case, we transfer ownership of
the memory to a few global variables. In the latter case, we just
silently drop the struct and end up leaking memory.
Introduce a function `clear_repository_format()` which frees the memory
the struct holds on to. Call it in the code paths where we currently
leak the memory. Also call it in the error path of
`read_repository_format()` to clean up any partial result.
For hygiene, we need to at least set the pointers that we free to NULL.
For future-proofing, let's zero the entire struct instead. It just means
that in the error path of `read_...()` we need to restore the error
sentinel in the `version` field.
We could take this opportunity to stop claiming that all fields except
`version` are undefined in case of an error. On the other hand, having
them defined as zero is not much better than having them undefined. We
could define them to some fallback configuration (`is_bare = -1` and
`hash_algo = GIT_HASH_SHA1`?), but "clear()" and/or "read()" seem like
the wrong places to enforce fallback configurations. Let's leave things
as "undefined" instead to encourage users to check `version`.
Signed-off-by: Martin Ågren
---
The error state can always be defined later. Defining it now, then
trying to backpedal, is probably not so fun. Filling the struct with
non-zero values might help flush out bugs like the one fixed in the
previous patch, but I'm wary of going that far in this patch.
cache.h | 6 ++
repository.c | 1 +
setup.c | 14 ++
3 files changed, 21 insertions(+)
diff --git a/cache.h b/cache.h
index 8b9e592c65..53ac01efa7 100644
--- a/cache.h
+++ b/cache.h
@@ -979,6 +979,12 @@ struct repository_format {
*/
void read_repository_format(struct repository_format *format, const char
*path);
+/*
+ * Free the memory held onto by `format`, but not the struct itself.
+ * (No need to use this after `read_repository_format()` fails.)
+ */
+void clear_repository_format(struct repository_format *format);
+
/*
* Verify that the repository described by repository_format is something we
* can read. If it is, return 0. Otherwise, return -1, and "err" will describe
diff --git a/repository.c b/repository.c
index 5dd1486718..efa9d1d960 100644
--- a/repository.c
+++ b/repository.c
@@ -159,6 +159,7 @@ int repo_init(struct repository *repo,
if (worktree)
repo_set_worktree(repo, worktree);
+ clear_repository_format(&format);
return 0;
error:
diff --git a/setup.c b/setup.c
index 52c3c9d31f..babe5ea156 100644
--- a/setup.c
+++ b/setup.c
@@ -517,6 +517,18 @@ void read_repository_format(struct repository_format
*format, const char *path)
format->hash_algo = GIT_HASH_SHA1;
string_list_init(&format->unknown_extensions, 1);
git_config_from_file(check_repo_format, path, format);
+ if (format->version == -1) {
+ clear_repository_format(format);
+ format->version = -1;
+ }
+}
+
+void clear_repository_format(struct repository_format *format)
+{
+ string_list_clear(&format->unknown_extensions, 0);
+ free(format->work_tree);
+ free(format->partial_clone);
+ memset(format, 0, sizeof(*format));
}
int verify_repository_format(const struct repository_format *format,
@@ -1043,9 +1055,11 @@ int discover_git_directory(struct strbuf *commondir,
strbuf_release(&err);
strbuf_setlen(commondir, commondir_offset);
strbuf_setlen(gitdir, gitdir_offset);
+ clear_repository_format(&candidate);
return -1;
}
+ clear_repository_format(&candidate);
return 0;
}
--
2.20.1
[PATCH 2/3] setup: do not use invalid `repository_format`
If `read_repository_format()` encounters an error, `format->version` will be -1 and all other fields of `format` will be undefined. However, in `setup_git_directory_gently()`, we use `repo_fmt.hash_algo` regardless of the value of `repo_fmt.version`. This can be observed by adding this to the end of `read_repository_format()`: if (format->version == -1) format->hash_algo = 0; /* no-one should peek at this! */ This causes, e.g., "git branch -m q q2 without config should succeed" in t3200 to fail with "fatal: Failed to resolve HEAD as a valid ref." because it has moved .git/config out of the way and is now trying to use a bad hash algorithm. Check that `version` is non-negative before using `hash_algo`. This patch adds no tests, but do note that if we skip this patch, the next patch would cause existing tests to fail as outlined above. Signed-off-by: Martin Ågren --- I fully admit to not understanding all of this setup code, neither in its current incarnation, nor in terms of an ideal end game. This check seems like a good thing to do though. setup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.c b/setup.c index 27747af7a3..52c3c9d31f 100644 --- a/setup.c +++ b/setup.c @@ -1138,7 +1138,7 @@ const char *setup_git_directory_gently(int *nongit_ok) gitdir = DEFAULT_GIT_DIR_ENVIRONMENT; setup_git_env(gitdir); } - if (startup_info->have_repository) + if (startup_info->have_repository && repo_fmt.version > -1) repo_set_hash_algo(the_repository, repo_fmt.hash_algo); } -- 2.20.1
Re: [PATCH 1/3] setup: drop return value from `read_repository_format()`
On Wed, 19 Dec 2018 at 16:27, Jeff King wrote: > > On Tue, Dec 18, 2018 at 08:25:26AM +0100, Martin Ågren wrote: > > > No-one looks at the return value, so we might as well drop it. It's > > still available as `format->version`. > > Hmm. If we have to pick one, I'd say that just returning a sane exit > value would be the more conventional thing to do. But looking at the > callers, many of them want to just pass the struct on to the verify > function. > > That said, there is a long-standing curiosity here that we may want to > consider: read_repository_format() does not distinguish between these > three cases: [snip several valuable insights] > I dunno. This is one of those dark corners of the code where we appear > to do the wrong thing, but nobody seems to have noticed or cared much, > and changing it runs the risk of breaking some obscure cases. I'm not > sure if we should bite the bullet and try to address that, or just back > away slowly and pretend we never looked at it. ;) That was my reaction when I first dug into this. :-/ It's only now that I've been brave enough to even try to dig through the first layer. > FWIW, the patch itself seems fine, and obviously doesn't make anything > worse on its own. The question is just whether we want to do more > cleanup here. Well, if we do want to make more cleanups around here, one of the more obvious ideas is to actually use the return value. If such a cleanup is going to start with a (moral) revert of this patch, then we're probably better off just dropping this patch. Thanks for your thoughtful response Martin
Re: [PATCH 2/3] setup: do not use invalid `repository_format`
On Wed, 19 Dec 2018 at 01:18, brian m. carlson wrote: > I think this change is fine, because we initialize the value in > the_repository elsewhere, and if there's no repository, this should > never have a value other than the default anyway. Thanks, it feels good that this patch matches how you think about the `hash_algo` field. > I looked at the other patches in the series and thought they looked sane > as well. Thanks for a review, I appreciate it. Martin
Re: [PATCH 2/3] setup: do not use invalid `repository_format`
On Wed, 19 Dec 2018 at 16:38, Jeff King wrote: > > On Tue, Dec 18, 2018 at 08:25:27AM +0100, Martin Ågren wrote: > > > Check that `version` is non-negative before using `hash_algo`. > Hmm. It looks like we never set repo_fmt.hash_algo to anything besides > GIT_HASH_SHA1 anyway. I guess the existing field is really just there in > preparation for us eventually respecting extensions.hashAlgorithm (or > whatever it's called). That was my understanding as well. Maybe I should have spelled it out. I think of the diff of this patch as "let's check `foo->valid` before we `use(foo->bar)`", which should only be able to regress in case foo isn't valid. And ... > Given what I said in my previous email about repos with a missing > "version" field, I wondered if this patch would be breaking config like: > > [core] > # no repositoryformatversion! > [extensions] > hashAlgorithm = sha256 > > But I'd argue that: > > 1. That's pretty dumb config that we shouldn't need to support. Even > if we care about handling the missing version for historical repos, > they wouldn't be talking sha256. ... this matches my thinking. > 2. Arguably we should not even look at extensions.* unless we see a > version >= 1. But we do process them as we parse the config file. > This is mostly an oversight, I think. We have to handle them as we > see them, because they may come out of order with respect to the > repositoryformatversion field. But we could put them into a > string_list, and then only process them after we've decided which > version we have. I hadn't thought too much about this. I guess that for some simpler extensions--versions dependencies it would be feasible to first parse everything, then, depending on the version we've identified, forget about any "irrelevant" extensions. Again, nothing I've thought much about, and seems to be safely out of scope for this patch. > So I think your patch is doing the right thing, and won't hurt any real > cases. But (of course) there are more opportunities to clean things up.
Re: [PATCH 3/3] setup: add `clear_repository_format()`
On Wed, 19 Dec 2018 at 16:48, Jeff King wrote:
>
> On Tue, Dec 18, 2018 at 08:25:28AM +0100, Martin Ågren wrote:
>
> > +void clear_repository_format(struct repository_format *format)
> > +{
> > + string_list_clear(&format->unknown_extensions, 0);
> > + free(format->work_tree);
> > + free(format->partial_clone);
> > + memset(format, 0, sizeof(*format));
> > }
>
> For the callers that actually pick the values out, I think it might be a
> little less error-prone if they actually copied the strings and then
> called clear_repository_format(). That avoids leaks of values that they
> didn't know or care about (and the cost of an extra strdup for
> repository setup is not a big deal).
>
> Something like this on top of your patch, I guess (with the idea being
> that functions which return an error would clear the format, but a
> "successful" one would get returned back up the stack to
> setup_git_directory_gently(), which then clears it before returning.
Thanks for the suggestion. I'll ponder 1) how to go about this
robustifying, 2) how to present the result as part of a v2 series.
To Junio on the sidelines in a cast (hope you're feeling better!):
you can expect a v2 of this series.
Martin
Re: [PATCH] stripspace: allow -s/-c outside git repository
On Tue, 18 Dec 2018 at 13:00, Johannes Schindelin wrote: > > Makes me wonder if `setup_git_directory_gently()` should BUG if it > > receives NULL. That would require some reshuffling in setup.c, since > > `setup_git_directory()` calls out to it with NULL... Just thinking out > > loud. In any case, and more importantly, this is the last user providing > > NULL except for the one in `setup_git_directory()`. > > We could rename `setup_git_directory_gently()` to > `setup_git_directory_gently_2()`, make that `static` and then call it from > `setup_git_directory_gently()` and `setup_git_directory()`. Thanks for the hint. I'm currently recuperating from having been lost in a nearby corner of setup.c, so I´ll leave this tightening as a low-hanging fruit for "someone else." Martin
Re: [PATCH 02/10] repository.c: replace hold_locked_index() with repo_hold_locked_index()
On Sat, 5 Jan 2019 at 07:07, Nguyễn Thái Ngọc Duy wrote:
>
> hold_locked_index() assumes the index path at $GIT_DIR/index. This is
> not good for places that take an arbitrary index_state instead of
> the_index, which is basically everywhere except builtin/.
>
> Replace it with repo_hold_locked_index(). hold_locked_index() remains
> as a wrapper around repo_hold_locked_index() to reduce changes in builtin/
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 7c7f98c72c..ddb3230d21 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -8,6 +8,7 @@
> * Clone a repository into a different directory that does not yet exist.
> */
>
> +#define USE_THE_INDEX_COMPATIBILITY_MACROS
I think this should be in patch 10/10.
> diff --git a/cache.h b/cache.h
> index ca36b44ee0..634c9ce325 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -433,6 +433,7 @@ void validate_cache_entries(const struct index_state
> *istate);
> #define unmerge_cache_entry_at(at) unmerge_index_entry_at(&the_index, at)
> #define unmerge_cache(pathspec) unmerge_index(&the_index, pathspec)
> #define read_blob_data_from_cache(path, sz)
> read_blob_data_from_index(&the_index, (path), (sz))
> +#define hold_locked_index(lock_file, flags)
> repo_hold_locked_index(the_repository, (lock_file), (flags))
> #endif
>
> #define TYPE_BITS 3
> @@ -833,7 +834,6 @@ extern struct cache_entry *refresh_cache_entry(struct
> index_state *, struct cach
> */
> extern void update_index_if_able(struct index_state *, struct lock_file *);
>
> -extern int hold_locked_index(struct lock_file *, int);
The reason this moves is it gets protected with "#ifndef
NO_THE_INDEX_COMPATIBILITY_MACROS". Ok.
> -int hold_locked_index(struct lock_file *lk, int lock_flags)
> -{
> - return hold_lock_file_for_update(lk, get_index_file(), lock_flags);
> -}
> +int repo_hold_locked_index(struct repository *repo,
> + struct lock_file *lf,
> + int flags)
> +{
> + return hold_lock_file_for_update(lf, repo->index_file, flags);
> +}
`get_index_file()` BUGs if `the_repository->index_file` is NULL, but
other than that, this looks like a faithful conversion. Do we want to
keep that check here?
Martin
Re: What's cooking in git.git (Jan 2019, #01; Mon, 7)
On Tue, 8 Jan 2019 at 00:34, Junio C Hamano wrote:
> * bc/sha-256 (2018-11-14) 12 commits
> - hash: add an SHA-256 implementation using OpenSSL
> - sha256: add an SHA-256 implementation using libgcrypt
> - Add a base implementation of SHA-256 support
> - commit-graph: convert to using the_hash_algo
> - t/helper: add a test helper to compute hash speed
> - sha1-file: add a constant for hash block size
> - t: make the sha1 test-tool helper generic
> - t: add basic tests for our SHA-1 implementation
> - cache: make hashcmp and hasheq work with larger hashes
> - hex: introduce functions to print arbitrary hashes
> - sha1-file: provide functions to look up hash algorithms
> - sha1-file: rename algorithm to "sha1"
>
> Add sha-256 hash and plug it through the code to allow building Git
> with the "NewHash".
AddressSanitizer barks at current pu (855f98be272f19d16564e) for a
handful of tests.
One example is t5702-protocol-v2.sh. I've tracked this one down to
ce1a82c251 ("Merge branch 'bc/sha-256' into jch", 2019-01-08).
Reverting that merge makes the problem go away, at least in t5702.
Notably bc/sha-256 on its own has no problems with t5702, possibly
because there has been quite some work done on the test script itself
in bc/sha-256..pu.
There are a few failing tests with AddressSanitizer on pu and they might
be caused by different topics. I've got to run, but thought I'd just
mention this one for now. Here's AddressSanitizer's first complaint for
t5702:
==1691823==ERROR: AddressSanitizer: heap-buffer-overflow on address
0x604004f2 at pc 0x004ea0fd bp 0x7ffc53082590 sp
0x7ffc53081d40
READ of size 32 at 0x604004f2 thread T0
#0 0x4ea0fc in __asan_memcpy
llvm/projects/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:23
#1 0x8603ec in oidset_insert oidset.c
#2 0x86c977 in add_promisor_object packfile.c:2129:4
#3 0x86c07a in for_each_object_in_pack packfile.c:2070:7
#4 0x86c535 in for_each_packed_object packfile.c:2095:7
#5 0x86c651 in is_promisor_object packfile.c:2151:4
#6 0x5ae77d in mark_object builtin/fsck.c:173:6
#7 0x5b0824 in mark_object_reachable builtin/fsck.c:200:2
#8 0x5b0824 in fsck_handle_ref builtin/fsck.c:509
#9 0x8e6987 in do_for_each_repo_ref_iterator refs/iterator.c:418:12
#10 0x8cb84f in do_for_each_ref refs.c:1466:9
#11 0x8cb84f in refs_for_each_rawref refs.c:1532
#12 0x8cb84f in for_each_rawref refs.c:1538
#13 0x5acd1f in get_default_heads builtin/fsck.c:524:2
#14 0x5acd1f in cmd_fsck builtin/fsck.c:846
#15 0x52f022 in run_builtin git.c:422:11
#16 0x52d583 in handle_builtin git.c:655:8
#17 0x52c00b in run_argv git.c:709:4
#18 0x52c00b in cmd_main git.c:806
#19 0x6c4569 in main common-main.c:45:9
#20 0x7f5fd6c23b96 in __libc_start_main
/build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310
#21 0x41c799 in _start (git+0x41c799)
0x604004f2 is located 0 bytes to the right of 34-byte region
[0x604004d0,0x604004f2)
allocated by thread T0 here:
#0 0x4eb4cf in malloc
llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:146
#1 0x9fa1db in do_xmalloc wrapper.c:60:8
#2 0x9fa2fd in do_xmallocz wrapper.c:100:8
#3 0x9fa2fd in xmallocz_gently wrapper.c:113
#4 0x86a877 in unpack_compressed_entry packfile.c:1588:11
#5 0x86a02e in unpack_entry packfile.c:1737:11
#6 0x867431 in cache_or_unpack_entry packfile.c:1439:10
#7 0x867431 in packed_object_info packfile.c:1506
#8 0x96b7be in oid_object_info_extended sha1-file.c:1394:10
#9 0x96d7d0 in read_object sha1-file.c:1434:6
#10 0x96d7d0 in read_object_file_extended sha1-file.c:1476
#11 0x85cf40 in repo_read_object_file ./object-store.h:174:9
#12 0x85cf40 in parse_object object.c:273
#13 0x86c752 in add_promisor_object packfile.c:2108:23
#14 0x86c07a in for_each_object_in_pack packfile.c:2070:7
#15 0x86c535 in for_each_packed_object packfile.c:2095:7
#16 0x86c651 in is_promisor_object packfile.c:2151:4
#17 0x5ae77d in mark_object builtin/fsck.c:173:6
#18 0x5b0824 in mark_object_reachable builtin/fsck.c:200:2
#19 0x5b0824 in fsck_handle_ref builtin/fsck.c:509
#20 0x8e6987 in do_for_each_repo_ref_iterator refs/iterator.c:418:12
#21 0x8cb84f in do_for_each_ref refs.c:1466:9
#22 0x8cb84f in refs_for_each_rawref refs.c:1532
#23 0x8cb84f in for_each_rawref refs.c:1538
#24 0x5acd1f in get_default_heads builtin/fsck.c:524:2
#25 0x5acd1f in cmd_fsck builtin/fsck.c:846
#26 0x52f022 in run_builtin git.c:422:11
#27 0x52d583 in handle_builtin git.c:655:8
#28 0x52c00b in run_argv git.c:709:4
#29 0x52c00b in cmd_main git.c:806
#30 0x6c4569 in main common-main.c:45:9
#31 0x7f5fd6c23b96 in __libc_start_main
/build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310
Re: What's cooking in git.git (Jan 2019, #01; Mon, 7)
On Wed, 9 Jan 2019 at 08:37, Martin Ågren wrote: > > On Tue, 8 Jan 2019 at 00:34, Junio C Hamano wrote: > > * bc/sha-256 (2018-11-14) 12 commits > > - hash: add an SHA-256 implementation using OpenSSL > > - sha256: add an SHA-256 implementation using libgcrypt > > - Add a base implementation of SHA-256 support > > - commit-graph: convert to using the_hash_algo > > - t/helper: add a test helper to compute hash speed > > - sha1-file: add a constant for hash block size > > - t: make the sha1 test-tool helper generic > > - t: add basic tests for our SHA-1 implementation > > - cache: make hashcmp and hasheq work with larger hashes > > - hex: introduce functions to print arbitrary hashes > > - sha1-file: provide functions to look up hash algorithms > > - sha1-file: rename algorithm to "sha1" > > > > Add sha-256 hash and plug it through the code to allow building Git > > with the "NewHash". > > AddressSanitizer barks at current pu (855f98be272f19d16564e) for a > handful of tests. > > One example is t5702-protocol-v2.sh. [...] > > ==1691823==ERROR: AddressSanitizer: heap-buffer-overflow on address > 0x604004f2 at pc 0x004ea0fd bp 0x7ffc53082590 sp > 0x7ffc53081d40 > READ of size 32 at 0x604004f2 thread T0 > #0 0x4ea0fc in __asan_memcpy > llvm/projects/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:23 > #1 0x8603ec in oidset_insert oidset.c > #2 0x86c977 in add_promisor_object packfile.c:2129:4 > #3 0x86c07a in for_each_object_in_pack packfile.c:2070:7 > #4 0x86c535 in for_each_packed_object packfile.c:2095:7 > #5 0x86c651 in is_promisor_object packfile.c:2151:4 > 0x604004f2 is located 0 bytes to the right of 34-byte region > [0x604004d0,0x604004f2) > allocated by thread T0 here: > #0 0x4eb4cf in malloc > llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:146 > #1 0x9fa1db in do_xmalloc wrapper.c:60:8 > #2 0x9fa2fd in do_xmallocz wrapper.c:100:8 > #3 0x9fa2fd in xmallocz_gently wrapper.c:113 > #4 0x86a877 in unpack_compressed_entry packfile.c:1588:11 > #5 0x86a02e in unpack_entry packfile.c:1737:11 > #6 0x867431 in cache_or_unpack_entry packfile.c:1439:10 > #7 0x867431 in packed_object_info packfile.c:1506 > #8 0x96b7be in oid_object_info_extended sha1-file.c:1394:10 > #9 0x96d7d0 in read_object sha1-file.c:1434:6 > #10 0x96d7d0 in read_object_file_extended sha1-file.c:1476 > #11 0x85cf40 in repo_read_object_file ./object-store.h:174:9 > #12 0x85cf40 in parse_object object.c:273 > #13 0x86c752 in add_promisor_object packfile.c:2108:23 > #14 0x86c07a in for_each_object_in_pack packfile.c:2070:7 > #15 0x86c535 in for_each_packed_object packfile.c:2095:7 > #16 0x86c651 in is_promisor_object packfile.c:2151:4 I found some more time to look into this. It seems we have a buffer with raw data and we set up a `struct object_id *` pointing into it, at a (supposed) OID value. Then `update_tree_entry_internal()` verifies that the buffer contains sufficiently many bytes, i.e., at least `the_hash_algo->rawsz` (=20). We immediately call `oidset_insert()` which copies an entire struct, i.e., we copy sizeof(struct object_id) (=32) bytes. Which is 12 more than what is known to be safe. For this particular input data, we read outside allocated memory. I can think of three possible approaches: * Allocate with a margin (GIT_MAX_RAWSZ - the_hash_algo->rawsz) where "necessary" (TM). Maybe not so maintainable. * Teach `oidset_insert()` (i.e., khash) to only copy `the_hash_algo->rawsz` bytes. Maybe not so good for performance. * Ignore. I wonder which of these is the least awful, or if there are other ideas. Martin
Re: What's cooking in git.git (Jan 2019, #01; Mon, 7)
On Thu, 10 Jan 2019 at 02:03, brian m. carlson wrote: > > On Wed, Jan 09, 2019 at 10:06:08PM +0100, Martin Ågren wrote: > > i.e., we copy sizeof(struct object_id) (=32) bytes. Which is 12 more > > than what is known to be safe. For this particular input data, we read > > outside allocated memory. > > Anything pointing to a struct object_id has to support at least > GIT_MAX_RAWSZ bytes, and that code doesn't, because it's a tree buffer. > > I ran into this later on in my SHA-256 work and have a series that fixes > the tree-walk code, but it's a bit involved and requires copying the > struct object_id out of the buffer. > > I thought we were going to be triggering this case only with some new > code I was introducing, but apparently somebody else got there first. > As for my series, I'll need to run the testsuite on it, but I'll try to > get it out tonight or at the latest tomorrow if people want to use that > instead. Cool. I should have known that you had something in the pipeline. Thanks for working on this.
[PATCH v2 0/3] setup: add `clear_repository_format()`
This patch series addresses memory leaks related to `struct
repository_format`. Compared to v1 [1], this v2 has dropped the first
patch ("setup: drop return value from `read_repository_format()`") and
added another patch 1/3 (which solves a different problem).
Patch 2/3 is unchanged. Patch 3/3 now calls `clear_...()` not only in
the error paths, but in all paths, as suggested in the review of v1.
Thanks to Peff and brian for reviewing v1.
There's a minor textual conflict with ed/simplify-setup-git-dir. A merge
of these two topics still passes the test suite (and these leaks remain
plugged).
[1]
https://public-inbox.org/git/20181218072528.3870492-1-martin.ag...@gmail.com/#t
Martin Ågren (3):
setup: free old value before setting `work_tree`
setup: do not use invalid `repository_format`
setup: add `clear_repository_format()`
cache.h | 12
builtin/init-db.c | 3 ++-
repository.c | 3 ++-
setup.c | 33 -
worktree.c| 4 +++-
5 files changed, 43 insertions(+), 12 deletions(-)
--
2.20.1
[PATCH v2 1/3] setup: free old value before setting `work_tree`
Before assigning to `data->work_tree` in `read_worktree_config()`, free
any value we might already have picked up, so that we do not leak it.
Signed-off-by: Martin Ågren
---
setup.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/setup.c b/setup.c
index 1be5037f12..bb633942bb 100644
--- a/setup.c
+++ b/setup.c
@@ -411,6 +411,7 @@ static int read_worktree_config(const char *var, const char
*value, void *vdata)
} else if (strcmp(var, "core.worktree") == 0) {
if (!value)
return config_error_nonbool(var);
+ free(data->work_tree);
data->work_tree = xstrdup(value);
}
return 0;
--
2.20.1
[PATCH v2 2/3] setup: do not use invalid `repository_format`
If `read_repository_format()` encounters an error, `format->version` will be -1 and all other fields of `format` will be undefined. However, in `setup_git_directory_gently()`, we use `repo_fmt.hash_algo` regardless of the value of `repo_fmt.version`. This can be observed by adding this to the end of `read_repository_format()`: if (format->version == -1) format->hash_algo = 0; /* no-one should peek at this! */ This causes, e.g., "git branch -m q q2 without config should succeed" in t3200 to fail with "fatal: Failed to resolve HEAD as a valid ref." because it has moved .git/config out of the way and is now trying to use a bad hash algorithm. Check that `version` is non-negative before using `hash_algo`. This patch adds no tests, but do note that if we skip this patch, the next patch would cause existing tests to fail as outlined above. Signed-off-by: Martin Ågren --- setup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.c b/setup.c index bb633942bb..4d3d67c50b 100644 --- a/setup.c +++ b/setup.c @@ -1140,7 +1140,7 @@ const char *setup_git_directory_gently(int *nongit_ok) gitdir = DEFAULT_GIT_DIR_ENVIRONMENT; setup_git_env(gitdir); } - if (startup_info->have_repository) + if (startup_info->have_repository && repo_fmt.version > -1) repo_set_hash_algo(the_repository, repo_fmt.hash_algo); } -- 2.20.1
[PATCH v2 3/3] setup: add `clear_repository_format()`
After we set up a `struct repository_format`, it owns various pieces of
allocated memory. We then either use those members, because we decide we
want to use the "candidate" repository format, or we discard the
candidate / scratch space. In the first case, we transfer ownership of
the memory to a few global variables. In the latter case, we just
silently drop the struct and end up leaking memory.
Fixing these memory leaks is complicated by two issues:
1) As described above, we "steal" the fields of the struct in case of
success.
2) We might end up calling `read_repository_format()` more than once
-- as we enter it a second time, we lose track of our pointers and
leak memory. As a further complication, we do not initialize (zero)
the structs before calling `read_...()` so as we first enter it, we
cannot blindly free the pointers in it.
To free this memory, in light of (1), we must either carefully cover all
error paths but no success paths; or we should stop grabbing the
pointers. To address 2), we must either zero the struct before calling
`read_repository_format()`, or try to keep track of when we should zero
it and when we should first free the memory.
Introduce `REPO_FORMAT_INIT` and `clear_repository_format()` to be used
on each side of `read_repository_format()`. Make the users duplicate the
memory from the structs, rather than copying the pointers.
Call `clear_...()` at the start of `read_...()` instead of just zeroing
the struct. In the error path of the same function, be sure to restore
the error sentinel after we clear it with the rest of the struct.
Signed-off-by: Martin Ågren
---
I do wonder if introducing and depending on `REPOSITORY_FORMAT_INIT`
like this is 100% sane. Out-of-tree users could be in for a nasty
surprise. :-/
cache.h | 12
builtin/init-db.c | 3 ++-
repository.c | 3 ++-
setup.c | 30 ++
worktree.c| 4 +++-
5 files changed, 41 insertions(+), 11 deletions(-)
diff --git a/cache.h b/cache.h
index ca36b44ee0..3ef63d27c4 100644
--- a/cache.h
+++ b/cache.h
@@ -972,6 +972,12 @@ struct repository_format {
struct string_list unknown_extensions;
};
+/**
+ * Always use this to initialize a `struct repository_format`
+ * to a well-defined state before calling `read_repository()`.
+ */
+#define REPOSITORY_FORMAT_INIT { 0 }
+
/*
* Read the repository format characteristics from the config file "path" into
* "format" struct. Returns the numeric version. On error, -1 is returned,
@@ -980,6 +986,12 @@ struct repository_format {
*/
int read_repository_format(struct repository_format *format, const char *path);
+/*
+ * Free the memory held onto by `format`, but not the struct itself.
+ * (No need to use this after `read_repository_format()` fails.)
+ */
+void clear_repository_format(struct repository_format *format);
+
/*
* Verify that the repository described by repository_format is something we
* can read. If it is, return 0. Otherwise, return -1, and "err" will describe
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 41faffd28d..04c60eaad5 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -96,7 +96,7 @@ static void copy_templates(const char *template_dir)
struct strbuf path = STRBUF_INIT;
struct strbuf template_path = STRBUF_INIT;
size_t template_len;
- struct repository_format template_format;
+ struct repository_format template_format = REPOSITORY_FORMAT_INIT;
struct strbuf err = STRBUF_INIT;
DIR *dir;
char *to_free = NULL;
@@ -148,6 +148,7 @@ static void copy_templates(const char *template_dir)
free(to_free);
strbuf_release(&path);
strbuf_release(&template_path);
+ clear_repository_format(&template_format);
}
static int git_init_db_config(const char *k, const char *v, void *cb)
diff --git a/repository.c b/repository.c
index 7b02e1dffa..df88705574 100644
--- a/repository.c
+++ b/repository.c
@@ -148,7 +148,7 @@ int repo_init(struct repository *repo,
const char *gitdir,
const char *worktree)
{
- struct repository_format format;
+ struct repository_format format = REPOSITORY_FORMAT_INIT;
memset(repo, 0, sizeof(*repo));
repo->objects = raw_object_store_new();
@@ -165,6 +165,7 @@ int repo_init(struct repository *repo,
if (worktree)
repo_set_worktree(repo, worktree);
+ clear_repository_format(&format);
return 0;
error:
diff --git a/setup.c b/setup.c
index 4d3d67c50b..70d9007ae5 100644
--- a/setup.c
+++ b/setup.c
@@ -477,7 +477,7 @@ static int check_repository_format_gently(const char
*gitdir, struct repository_
}
repository_format_precious_objects = candidate->precious_objects;
- repository_format_partial_clone = candidate->partial_clone;
+
Re: [PATCH v2 2/3] setup: do not use invalid `repository_format`
On Tue, 15 Jan 2019 at 20:31, Jeff King wrote:
>
> On Mon, Jan 14, 2019 at 07:34:56PM +0100, Martin Ågren wrote:
>
> > This can be observed by adding this to the end of
> > `read_repository_format()`:
> >
> > if (format->version == -1)
> > format->hash_algo = 0; /* no-one should peek at this! */
> >
> > Check that `version` is non-negative before using `hash_algo`.
> I'm still somewhat confused about how this breaks. If we move
> ".git/config" out of the way, then we have no version indicator and
> presumably we should guess GIT_HASH_SHA1. Which is what's happening if
> we fail to call repo_set_hash_algo(), no? In other words, wouldn't
> repo_set_hash_algo() always be a noop in that case?
>
> I get why adding the code snippet above would cause that assumption to
> break, but I am just not sure why we would add that code snippet. ;)
>
> I also get why read_repository_format() doing this in patch 3 would be a
> problem:
>
> + if (format->version == -1) {
> + clear_repository_format(format);
> + format->version = -1;
> + }
>
> but doesn't that point out that clear_repository_format() should be
> setting hash_algo to GIT_HASH_SHA1 as the default (and likewise "bare =
> -1", etc, that is done in that function)?
Something like the below on top of this series (then rebased). (The last
hunk below is a revert of this patch.)
I'd like to think of the situation before this patch above as a
situation where the API promises something and the user uses the API
beyond that. The next patch in this series changes the internals of the
API in a way that is consistent with the promise made, but which ends up
affecting an over-eager user.
What this patch above does is to make the user do what the API promise
allows them to do, i.e., no more shortcuts. What you're saying is, why
isn't the promise stronger? So the user won't have to think as much?
So in particular, why doesn't `clear...()` and the error path in
`read_...()` impose sane, usable defaults? My first concern is that it
means we need to make a stronger promise, which might then be hard to
back away from, if we want to. Maybe we'll never want to...
My second concern is, what should we be falling back to, going forward?
At some point, the hash indicated by `REPOSITORY_FORMAT_INIT` will be
SHA-256. Before that, and as soon as we support both hashes, what if we
pick up SHA-256 before stumbling on some other piece of the config --
should we now reset the struct to indicate SHA-1, or rather keep the
SHA-256 value, which by itself is valid? (The same could be argued now,
for something other than hash functions, but the SHA-1/256 example might
be more obvious in the context of this patch.)
My third worry is that we should then equip `clear_...()` or at least
the error path of `read_...()` with some logic to keep "as much as
possible" of what we've picked up and reset the rest, all the while
making sure we don't end up with something self-contradicting or stupid.
After all, we'll have promised the users that they can ignore any errors
and just run ahead.
Maybe I'm worrying way too much, and I shouldn't be so afraid of making
a stronger promise here and now because of vague slippery-slope thinking.
Thanks for pushing back and forcing me to articulate my thinking.
Martin
diff --git a/cache.h b/cache.h
index 3ef63d27c4..acd86e9f9f 100644
--- a/cache.h
+++ b/cache.h
@@ -974,15 +974,21 @@ struct repository_format {
/**
* Always use this to initialize a `struct repository_format`
- * to a well-defined state before calling `read_repository()`.
+ * to a well-defined, default state before calling
+ * `read_repository()`.
*/
-#define REPOSITORY_FORMAT_INIT { 0 }
+#define REPOSITORY_FORMAT_INIT (struct repository_format){ \
+.version = -1, \
+.is_bare = -1, \
+.hash_algo = GIT_HASH_SHA1, \
+.unknown_extensions = STRING_LIST_INIT_DUP, \
+ }
/*
* Read the repository format characteristics from the config file "path" into
* "format" struct. Returns the numeric version. On error, -1 is returned,
* format->version is set to -1, and all other fields in the struct are
- * undefined.
+ * set to the default configuration (REPOSITORY_FORMAT_INIT).
*/
int read_repository_format(struct repository_format *format, const char *path);
diff --git a/setup.c b/setup.c
index 70d9007ae5..f3ea479ad9 100644
--- a/setup.c
+++ b/setup.c
@@ -511,15 +511,9 @@ static int check_repository_format_gently(const char
*gitdir, struct repository_
int read_repository_format(struct repository_format *format,
Re: [PATCH] Allow usage of --gpg-sign flag in commit-tree builtin.
Hi Brandon,
Welcome to the list! :-)
On Thu, 17 Jan 2019 at 20:30, Brandon Richardson
wrote:
> Subject: [PATCH] Allow usage of --gpg-sign flag in commit-tree builtin.
Good find!
Maybe "commit-tree: handle --gpg-sign" which looks more like what we
see in `git log --oneline builtin/commit-tree.c`.
> Signed-off-by: Brandon Richardson
It could be worthwhile to note in the proposed commit message that this
option is actually documented, since 55ca3f99ae ("commit-tree: add and
document --no-gpg-sign", 2013-12-13), so it's clearly an omission that
it's not implemented.
> Here are the steps I followed to replicate the issue:
>
> mkdir test && cd test
> git init
> echo 'test' > test.txt
> git hash-object -w test.txt
> git update-index --add --cacheinfo 100644 test.txt
> git write-tree
> git commit-tree --gpg-sign -m 'test commit msg'
Have you looked at turning this into a test in the t/ directory?
Grepping a little suggests that t7510-signed-commit.sh might be a good
spot. A test would make the patch more obviously correct, and would make
sure that this doesn't regress in the future.
> - if (skip_prefix(arg, "-S", &sign_commit))
> + if (skip_prefix(arg, "-S", &sign_commit) ||
> + skip_prefix(arg, "--gpg-sign=", &sign_commit) ||
> + skip_prefix(arg, "--gpg-sign", &sign_commit))
> continue;
This would match "--gpg-signfoo", which seems like a bug. The first two
`skip_prefix()` calls look ok. Then maybe
if (!strcmp(arg, "--gpg-sign")) {
sign_commit = "";
continue;
}
That's entirely untested, though.
Thanks for looking into this, rather than just side-stepping it. ;-)
Martin
Re: [PATCH] commit-tree: add missing --gpg-sign flag
Hi Brandon,
On Fri, 18 Jan 2019 at 02:12, Brandon Richardson
wrote:
>
> Add --gpg-sign option in commit-tree, which was documented, but not
> implemented, in 55ca3f99ae.
>
> Signed-off-by: Brandon Richardson
> ---
Thank you for an updated version!
> builtin/commit-tree.c| 8 +++-
> t/t7510-signed-commit.sh | 4 +++-
Ah, a test, nice. :-)
> - if (skip_prefix(arg, "-S", &sign_commit))
> + if(!strcmp(arg, "--gpg-sign")) {
> + skip_prefix(arg, "--gpg-sign", &sign_commit);
I personally find this a bit convoluted, compared to just assigning "".
Maybe there are reasons for doing it this way that I don't see?
> + continue;
> + }
> +
> + if (skip_prefix(arg, "-S", &sign_commit) ||
> + skip_prefix(arg, "--gpg-sign=", &sign_commit))
> continue;
Looks good.
> --- a/t/t7510-signed-commit.sh
> +++ b/t/t7510-signed-commit.sh
> @@ -51,7 +51,9 @@ test_expect_success GPG 'create signed commits' '
> # commit.gpgsign is still on but this must not be signed
> git tag ninth-unsigned $(echo 9 | git commit-tree HEAD^{tree}) &&
> # explicit -S of course must sign.
> - git tag tenth-signed $(echo 9 | git commit-tree -S HEAD^{tree})
> + git tag tenth-signed $(echo 10 | git commit-tree -S HEAD^{tree})
> + # --gpg-sign must sign.
> + git tag eleventh-signed $(echo 11 | git commit-tree --gpg-sign
> HEAD^{tree})
> '
Thanks for providing a test, and thanks for fixing the "9"/"10"
copy-paste error. (You might want to remark "Fix a 9/10 typo while we're
here." in the commit message, especially since that line requires
editing anyway, see next paragraph.)
All of the commands above are suffixed with "&&" which means that the
shell only keeps executing as long as the commands succeed. If any of
those 20-30 commands fails, the shell will jump out of the &&-chain and
land ... at this line you're adding. If that one succeeds, the test will
be reported as succeeding. So please add a "&&" to the "10" line.
All of that said ... if I add the missing "&&" and run your test on the
*old* implementation, it still succeeds. The reason is that I grepped
for the "best" place to put this, and directed you to a part of the test
suite where it might be a bit too easy to copy existing code and end
up with something non-ideal. Sorry about that. :-(
What happens is that git commit-tree reports "fatal: Not a valid object
name --gpg-sign", then we go on to happily execute `git tag
eleventh-signed` where we've just substituted the empty string produced
by git commit-tree. The verifications will succeed, because there's
already a signature there... (BTW, the verifications happen further
down, so you'll want to add "eleventh-signed" to the list of tags
there.)
One way of making the test more robust might be to add a brand new
commit, similar to how it's done earlier in the test.
It's not your fault that you fell into this trap. If you want to look
into making this more robust -- and try running the test before and after
your fix -- great! If you feel it's out of your comfort zone or interest
range, just let me know and I could try to take it from here.
> test_expect_success GPG 'verify and show signatures' '
BTW, here's that test where the signatures are verified.
Martin
Re: [PATCH v2 2/2] commit-graph: don't call write_graph_chunk_large_edges() unnecessarily
On Fri, 18 Jan 2019 at 18:23, SZEDER Gábor wrote: > write_commit_graph() unconditionally invokes > write_graph_chunk_large_edges(), even when it was decided earlier that > that chunk won't be written. Strictly speaking there is no bug here, > because write_graph_chunk_large_edges() won't write anything if it > Don't call write_graph_chunk_large_edges() when that chunk won't be > written to spare an unnecessary iteration over all commits. This commit message (including the one-line subject) needs some s/_large_/_extra_/. > - write_graph_chunk_extra_edges(f, commits.list, commits.nr); > + if (num_extra_edges) > + write_graph_chunk_extra_edges(f, commits.list, commits.nr); Martin
Re: [PATCH v3] commit-tree: add missing --gpg-sign flag
Hi Brandon,
Thanks for a v3.
On Sat, 19 Jan 2019 at 04:36, Brandon Richardson
wrote:
> - if (skip_prefix(arg, "-S", &sign_commit))
> + if(!strcmp(arg, "--gpg-sign")) {
(Same nit as Junio about the missing space after "if".)
> + sign_commit = "";
Nice. ;-)
> + continue;
> + }
> # commit.gpgsign is still on but this must not be signed
> git tag ninth-unsigned $(echo 9 | git commit-tree HEAD^{tree}) &&
> # explicit -S of course must sign.
> - git tag tenth-signed $(echo 9 | git commit-tree -S HEAD^{tree})
> + git tag tenth-signed $(echo 10 | git commit-tree -S HEAD^{tree}) &&
> +
> + # --gpg-sign[=] must sign.
> + echo 11 >file && test_tick && git commit -S -a -m "eleventh signed" &&
> + git tag eleventh-signed &&
> + git commit-tree --gpg-sign -m "twelfth signed" HEAD^{tree} &&
> + git tag twelfth-signed &&
> +git commit-tree --gpg-sign=B7227189 -m "thirteenth signed" HEAD^{tree} &&
> +git tag thirteenth-signed
> '
(These last two lines are not tab-indented, but indented by four spaces.
They were perhaps mangled by some copy-pasting.)
Running this test, we end up with three tags on one commit:
eleventh-signed, twelfth-signed and thirteenth-signed. So as long as
`git commit -S` works when we use the number 11, everything will pass,
and we won't really test what we wanted to test. We will verify that
`git commit-tree` doesn't choke on "--gpg-sign[=foo]", but we won't
verify that it handles it correctly.
(Just recently, it was pointed out on this list that `git log --count`
won't complain about "--count", but won't act on it, either. So such
errors are not unheard of.)
I looked into this test in a bit more detail, and it seems to be quite
hard to get right. Part of the reason is that `git commit-tree` requires
a bit more careful use than `git commit`, but part of it is that the
tests that we already have for `git commit-tree [-S]` right before the
ones you're adding are a bit too loose, IMHO. So they're not ideal for
copy-pasting... I've come up with the patch below, which you might want
to use as a basis for your work.
That is, you could `git am --scissors` this patch on a fresh branch and
`git commit --amend --signoff --no-edit` it (see
Documentation/SubmittingPatches, "forwarding somebody else's patch"),
then base your work on it, e.g., by cherry-picking your v3 commit.
I think you would want to add 2x3 lines of tests (3 for `--gpg-sign`, 3
for `--gpg-sign=...`). That would give you eleventh-signed and
twelfth-signed and you wouldn't need any invocation of `git commit` (so
no thirteenth-signed).
If you're not up for that, just let me know and I could instead rebase
your patch on top of mine and submit both as a v4. I think this has come
along nicely, and now it's really just about having a robust test.
Martin
-- >8 --
Subject: [PATCH] t7510: invoke git as part of &&-chain
If `git commit-tree HEAD^{tree}` fails on us and produces no output on
stdout, we will substitute that empty string and execute `git tag
ninth-unsigned`, i.e., we will tag HEAD rather than a newly created
object. But we are lucky: we have a signature on HEAD, so we should
eventually fail the next test, where we verify that "ninth-unsigned" is
indeed unsigned.
We have a similar problem a few lines later. If `git commit-tree -S`
fails with no output, we will happily tag HEAD as "tenth-signed". Here,
we are not so lucky. The tag ends up on the same commit as
"eighth-signed-alt", and that's a signed commit, so t7510-signed-commit
will pass, despite `git commit-tree -S` failing.
Make these `git commit-tree` invocations a direct part of the &&-chain,
so that we can rely less on luck and set a better example for future
tests modeled after this one. Fix a 9/10 copy/paste error while at it.
Signed-off-by: Martin Ågren
---
t/t7510-signed-commit.sh | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 86d3f93fa2..58f528b98f 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -49,9 +49,13 @@ test_expect_success GPG 'create signed commits' '
git tag eighth-signed-alt &&
# commit.gpgsign is still on but this must not be signed
- git tag ninth-unsigned $(echo 9 | git commit-tree HEAD^{tree}) &&
+ echo 9 | git commit-tree HEAD^{tree} >oid &&
+ test_line_count = 1 oid &&
+ git tag ninth-unsigned $(cat oid) &&
# explicit -S of course must sign.
- git tag tenth-signed $(echo 9 | git commit-tree -S HEAD^{tree})
+ echo 10 | git commit-tree -S HEAD^{tree} >oid &&
+ test_line_count = 1 oid &&
+ git tag tenth-signed $(cat oid)
'
test_expect_success GPG 'verify and show signatures' '
--
2.20.1.98.gecbdaf0899
Re: [PATCH v3] commit-tree: add missing --gpg-sign flag
On Sat, 19 Jan 2019 at 16:46, Martin Ågren wrote:
> # commit.gpgsign is still on but this must not be signed
> - git tag ninth-unsigned $(echo 9 | git commit-tree HEAD^{tree}) &&
> + echo 9 | git commit-tree HEAD^{tree} >oid &&
> + test_line_count = 1 oid &&
> + git tag ninth-unsigned $(cat oid) &&
> # explicit -S of course must sign.
> - git tag tenth-signed $(echo 9 | git commit-tree -S HEAD^{tree})
> + echo 10 | git commit-tree -S HEAD^{tree} >oid &&
> + test_line_count = 1 oid &&
> + git tag tenth-signed $(cat oid)
> '
Or, a bit simpler:
oid=$(echo 10 | git commit-tree -S HEAD^{tree}) &&
git tag tenth-signed "$oid"
Martin
Re: [PATCH v3] commit-tree: add missing --gpg-sign flag
Hi Brandon,
On Sat, 19 Jan 2019 at 19:05, Brandon Richardson
wrote:
> > I looked into this test in a bit more detail, and it seems to be quite
> > hard to get right. Part of the reason is that `git commit-tree` requires
> > a bit more careful use than `git commit`, but part of it is that the
> > tests that we already have for `git commit-tree [-S]` right before the
> > ones you're adding are a bit too loose, IMHO. So they're not ideal for
> > copy-pasting... I've come up with the patch below, which you might want
> > to use as a basis for your work.
> Just finished adding in the changes you suggested, and everything looks
> good on my end. I based my changes on the patch you provided.
>
> > Or, a bit simpler:
> >
> > oid=$(echo 10 | git commit-tree -S HEAD^{tree}) &&
> > git tag tenth-signed "$oid"
>
> Just noticed your latest email. Do you prefer it this way?
I think so, yeah. (But who knows what others might prefer? ;-) )
The use of "" around $oid is perhaps a bit subtle, but not too much so,
I think. The "test_line_count" version was probably a bit too paranoid
and verbose, for no real gain.
> If so, I can amend
> what I have before I submit v4.
>
> When I submit v4, should I submit the patch you created as well, given
> that my changes are based off of it?
I think the cleanest would be to submit a two-patch series, v4.
Alternatively, you could submit only a patch of your own, but it should
then be based directly off of origin/master. So the test in it could
be inspired by my patch, but yours would not have mine as a parent and
the context lines of your patch would look like what is currently in
master. My patch could then go on top of yours, as a "the new tests are
more robust than these old ones; let's rewrite them to the new style".
Thanks
Martin
Re: [PATCH v4 2/2] commit-tree: add missing --gpg-sign flag
Hi Brandon,
On Sun, 20 Jan 2019 at 00:24, Brandon Richardson
wrote:
> # explicit -S of course must sign.
> echo 10 | git commit-tree -S HEAD^{tree} >oid &&
> test_line_count = 1 oid &&
> - git tag tenth-signed $(cat oid)
> + git tag tenth-signed $(cat oid) &&
> +
> + # --gpg-sign[=] must sign.
> + echo 11 | git commit-tree --gpg-sign HEAD^{tree} >oid &&
> + test_line_count = 1 oid &&
> + git tag eleventh-signed $(cat oid) &&
> + echo 12 | git commit-tree --gpg-sign=B7227189 HEAD^{tree} >oid &&
> + test_line_count = 1 oid &&
> + git tag twelfth-signed-alt $(cat oid)
> '
Thank you for following through.
Let's see if there any opinions from others about this more verbose
construction, vs placing the oid in a variable and quoting it. We
obviously went several years without realizing that using $(...) as an
object id risked falling back to HEAD and that a completely broken `git
commit-tree -S` would pass the test. So being over-careful and extra
obvious might very well be the right thing.
> test_expect_success GPG 'verify and show signatures' '
> (
> for commit in initial second merge fourth-signed \
> - fifth-signed sixth-signed seventh-signed tenth-signed
> + fifth-signed sixth-signed seventh-signed tenth-signed
> \
> + eleventh-signed
> do
> git verify-commit $commit &&
> git show --pretty=short --show-signature $commit
> >actual &&
> @@ -82,7 +91,7 @@ test_expect_success GPG 'verify and show signatures' '
> done
> ) &&
> (
> - for commit in eighth-signed-alt
> + for commit in eighth-signed-alt twelfth-signed-alt
> do
> git show --pretty=short --show-signature $commit
> >actual &&
> grep "Good signature from" actual &&
Ah, good catch. I didn't notice that we had a separate for-loop for this
key. This comes from 4baf839fe0 ("t7510: test a commit signed by an
unknown key", 2014-06-16). What we want to test here is something
different, namely that we're using a specific, named key. But FWIW, I
think we're fine, and that we're not abusing the existing difference
between these two loops too much.
Martin
Re: [PATCH v2 2/3] setup: do not use invalid `repository_format`
On Tue, 22 Jan 2019 at 08:07, Jeff King wrote: > > On Thu, Jan 17, 2019 at 07:31:14AM +0100, Martin Ågren wrote: > > > Something like the below on top of this series (then rebased). (The last > > hunk below is a revert of this patch.) > > Yes, that's exactly what I had in mind. Usually our clear() functions > put the struct back into some default state from which it can be used > gain. But the state after clear() here (without the patch below) is > something that nobody is ever expected to look at. > > So in particular, why doesn't `clear...()` and the error path in > > `read_...()` impose sane, usable defaults? My first concern is that it > > means we need to make a stronger promise, which might then be hard to > > back away from, if we want to. Maybe we'll never want to... > > I'm not too worried about that personally. I think the more likely > problem is that the API is misunderstood and misused. ;) Heh. Agreed. :-) > Now if your next question is: "does any caller misuse this as more than > looking at the repo format", I don't know the answer for sure. That > would be worth poking at (or perhaps having just poked yourself, you > might have an idea already). Not really. I've stumbled around a little, but I'll need to do that some more. > For the record, I can live with it either way. There are so many funky > little setup corner cases in the code already, and we don't even really > have a real-world case to dissect at this point. So the right thing may > also just be to finish this patch series as quickly as possible and move > on to something more useful. :) I rebased the "something like this?" into this series yesterday and I think the end result is better, but also that the way there is clearer, mostly because this patch is then gone. I wanted to double-check it tonight and submit it. I'll do that tonight. Thank you for your comments. They're really helpful. Martin
Re: [PATCH v4 2/2] commit-tree: add missing --gpg-sign flag
On Tue, 22 Jan 2019 at 20:07, Junio C Hamano wrote:
>
> Martin Ågren writes:
>
> >> + echo 11 | git commit-tree --gpg-sign HEAD^{tree} >oid &&
> >> + test_line_count = 1 oid &&
> >> + git tag eleventh-signed $(cat oid) &&
> >> +...
> > Let's see if there any opinions from others about this more verbose
> > construction, vs placing the oid in a variable and quoting it. We
> > obviously went several years without realizing that using $(...) as an
> > object id risked falling back to HEAD and that a completely broken `git
> > commit-tree -S` would pass the test. So being over-careful and extra
> > obvious might very well be the right thing.
>
> Sorry, but I am not sure what issue you are worried about. If the
> "commit-tree" command failed in this construct:
>
> oid=$(echo 11 | git commit-tree ...) &&
> git tag eleventh-signed "$oid"
>
> wouldn't the &&-chain break after the assignment of an empty string
> to oid, skip "git tag" and make the whole test fail, with or without
> '$oid" fed to "git tag" quoted?
Yes.
> It is wrong not to quote "$oid" for
> the "git tag" command (the test should not rely on the fact that the
> object names given by "git commit-tree" have no $IFS in them), but
> that is a separate issue.
It'd also protect against a failure mode where we get no output and a
zero exit code. (Maybe that's ridiculous, but we're testing `git
commit-tree -S` here -- plus, I'm lazy, so I'd rather double-quote than
think. ;-) )
But you asked me what issue I worried about... To recap, master has a
test with a one-liner that doesn't bark if you completely drop the
implementation of `git commit-tree -S`. I don't think that's the
worrying that you're puzzled about.
I posted a three-line replacement that verified the exit code and quotes
the output, but which also has a pretty paranoid middle step to verify
that there was precisely one line of output. I then followed up with a
less paranoid two-liner, which avoids some round-tripping, and which
doesn't verify the line count, but which rather assumes that `git tag`
will bark on a bad oid.
I think that last thing is a fair assumption, and that's why I referred
to the three-line test as being over-careful and extra obvious. I'm not
worrying about the quoting as such.
Martin
[PATCH v3 2/2] setup: fix memory leaks with `struct repository_format`
After we set up a `struct repository_format`, it owns various pieces of
allocated memory. We then either use those members, because we decide we
want to use the "candidate" repository format, or we discard the
candidate / scratch space. In the first case, we transfer ownership of
the memory to a few global variables. In the latter case, we just
silently drop the struct and end up leaking memory.
Introduce an initialization macro `REPOSITORY_FORMAT_INIT` and a
function `clear_repository_format()`, to be used on each side of
`read_repository_format()`. To have a clear and simple memory ownership,
let all users of `struct repository_format` duplicate the strings that
they take from it, rather than stealing the pointers.
Call `clear_...()` at the start of `read_...()` instead of just zeroing
the struct, since we sometimes enter the function multiple times. This
means that it is important to initialize the struct before calling
`read_...()`, so document that. Teach `read_...()` to clear the struct
upon an error, so that it is reset to a safe state, and document this.
About that very last point: In `setup_git_directory_gently()`, we look
at `repo_fmt.hash_algo` even if `repo_fmt.version` is -1, which we
weren't actually supposed to do per the API. After this commit, that's
ok.
We inherit the existing code's combining "error" and "no version found".
Both are signalled through `version == -1` and now both cause us to
clear any partial configuration we have picked up. For "extensions.*",
that's fine, since they require a positive version number. For
"core.bare" and "core.worktree", we're already verifying that we have a
non-negative version number before using them.
Signed-off-by: Martin Ågren
---
cache.h | 27 ---
builtin/init-db.c | 3 ++-
repository.c | 3 ++-
setup.c | 32
worktree.c| 4 +++-
5 files changed, 51 insertions(+), 18 deletions(-)
diff --git a/cache.h b/cache.h
index ca36b44ee0..ec7c043412 100644
--- a/cache.h
+++ b/cache.h
@@ -972,14 +972,35 @@ struct repository_format {
struct string_list unknown_extensions;
};
+/*
+ * Always use this to initialize a `struct repository_format`
+ * to a well-defined, default state before calling
+ * `read_repository()`.
+ */
+#define REPOSITORY_FORMAT_INIT \
+{ \
+ .version = -1, \
+ .is_bare = -1, \
+ .hash_algo = GIT_HASH_SHA1, \
+ .unknown_extensions = STRING_LIST_INIT_DUP, \
+}
+
/*
* Read the repository format characteristics from the config file "path" into
- * "format" struct. Returns the numeric version. On error, -1 is returned,
- * format->version is set to -1, and all other fields in the struct are
- * undefined.
+ * "format" struct. Returns the numeric version. On error, or if no version is
+ * found in the configuration, -1 is returned, format->version is set to -1,
+ * and all other fields in the struct are set to the default configuration
+ * (REPOSITORY_FORMAT_INIT). Always initialize the struct using
+ * REPOSITORY_FORMAT_INIT before calling this function.
*/
int read_repository_format(struct repository_format *format, const char *path);
+/*
+ * Free the memory held onto by `format`, but not the struct itself.
+ * (No need to use this after `read_repository_format()` fails.)
+ */
+void clear_repository_format(struct repository_format *format);
+
/*
* Verify that the repository described by repository_format is something we
* can read. If it is, return 0. Otherwise, return -1, and "err" will describe
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 41faffd28d..04c60eaad5 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -96,7 +96,7 @@ static void copy_templates(const char *template_dir)
struct strbuf path = STRBUF_INIT;
struct strbuf template_path = STRBUF_INIT;
size_t template_len;
- struct repository_format template_format;
+ struct repository_format template_format = REPOSITORY_FORMAT_INIT;
struct strbuf err = STRBUF_INIT;
DIR *dir;
char *to_free = NULL;
@@ -148,6 +148,7 @@ static void copy_templates(const char *template_dir)
free(to_free);
strbuf_release(&path);
strbuf_release(&template_path);
+ clear_repository_format(&template_format);
}
static int git_init_db_config(const char *k, const char *v, void *cb)
diff --git a/repository.c b/repository.c
index 7b02e1dffa..df88705574 100644
--- a/repository.c
+++ b/repository.c
@@ -148,7 +148,7 @@ int repo_init(struct repository *repo,
const char *gitdir,
const char *worktree)
{
- struct repository_format format;
+ struct repository_format format = REPOSITORY_FORMAT_INIT;
memset(repo, 0, sizeof(*repo));
repo->objects = raw_obj
[PATCH v3 1/2] setup: free old value before setting `work_tree`
Before assigning to `data->work_tree` in `read_worktree_config()`, free
any value we might already have picked up, so that we do not leak it.
Signed-off-by: Martin Ågren
Signed-off-by: Junio C Hamano
---
setup.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/setup.c b/setup.c
index 1be5037f12..bb633942bb 100644
--- a/setup.c
+++ b/setup.c
@@ -411,6 +411,7 @@ static int read_worktree_config(const char *var, const char
*value, void *vdata)
} else if (strcmp(var, "core.worktree") == 0) {
if (!value)
return config_error_nonbool(var);
+ free(data->work_tree);
data->work_tree = xstrdup(value);
}
return 0;
--
2.20.1.98.gecbdaf0899
[PATCH v3 0/2] setup: fix memory leaks with `struct repository_format`
On Tue, 22 Jan 2019 at 14:34, Martin Ågren wrote: > > On Tue, 22 Jan 2019 at 08:07, Jeff King wrote: > > > For the record, I can live with it either way. There are so many funky > > little setup corner cases in the code already, and we don't even really > > have a real-world case to dissect at this point. So the right thing may > > also just be to finish this patch series as quickly as possible and move > > on to something more useful. :) > > I rebased the "something like this?" into this series yesterday and I > think the end result is better, but also that the way there is clearer, > mostly because this patch is then gone. I wanted to double-check it > tonight and submit it. I'll do that tonight. Here's that reroll. I now reset the entire struct in the error path of `clear_...()`. Thus, the user that is reading `repo_fmt.hash_algo` despite not being supposed to, can keep reading it, now knowing that the value has a default value. I also expanded on the documentation a little to point out that we'll reset to the default struct state if we don't find any "core.repositoryformatversion". Martin Martin Ågren (2): setup: free old value before setting `work_tree` setup: fix memory leaks with `struct repository_format` cache.h | 27 --- builtin/init-db.c | 3 ++- repository.c | 3 ++- setup.c | 33 + worktree.c| 4 +++- 5 files changed, 52 insertions(+), 18 deletions(-) -- 2.20.1.98.gecbdaf0899
Re: [PATCH v3 2/2] setup: fix memory leaks with `struct repository_format`
On Wed, 23 Jan 2019 at 06:57, Jeff King wrote: > > On Tue, Jan 22, 2019 at 10:45:48PM +0100, Martin Ågren wrote: > > > Call `clear_...()` at the start of `read_...()` instead of just zeroing > > the struct, since we sometimes enter the function multiple times. This > > means that it is important to initialize the struct before calling > > `read_...()`, so document that. > > This part is a little counter-intuitive to me. Is anybody ever going to > pass in anything except a struct initialized to REPOSITORY_FORMAT_INIT? I do update all users in git.git, but yeah, out-of-tree users and in-flight topics would segfault. > If so, might it be kinder for read_...() to not assume anything about > the incoming struct, and initialize it from scratch? I.e., not to use > clear() but just do the initialization step? I have some vague memory from going down that route and giving up. Now that I'm looking at it again, I think we can at least try to do something. We can make sure that "external" users that call into setup.c are fine (they'll leak, but won't crash). Out-of-tree users inside setup.c will still be able to trip on this. I don't have much spare time over the next few days, but I'll get to this. Or we could accept that we may leak when we end up calling `read()` multiple times (I could catch all leaks now, but new ones might sneak in after that) and come back to this after X months, when we can perhaps afford to be a bit more aggressive. I guess we could just rename the struct to have the compiler catch out-of-tree users... > A caller which calls read_() multiple times would presumably have an > intervening clear (either their own, or the one done on an error return > from the read function). > > Other than that minor nit, I like the overall shape of this. Thank you. Martin
Re: [PATCH v3 2/2] setup: fix memory leaks with `struct repository_format`
On Thu, 24 Jan 2019 at 01:15, brian m. carlson
wrote:
>
> On Wed, Jan 23, 2019 at 12:57:05AM -0500, Jeff King wrote:
> > > +void clear_repository_format(struct repository_format *format)
> > > +{
> > > + string_list_clear(&format->unknown_extensions, 0);
> > > + free(format->work_tree);
> > > + free(format->partial_clone);
> > > + *format = (struct repository_format)REPOSITORY_FORMAT_INIT;
> > > +}
> >
> > ...this uses that expression not as an initializer, but as a compound
> > literal. That's also C99, but AFAIK it's the first usage in our code
> > base. I don't know if it will cause problems or not.
> > Given how simple it is to fix if it turns out to be a problem, I'm OK
> > including it as a weather balloon.
Thanks for pointing out this potential problem.
> It's my understanding that MSVC doesn't support this construct. If we
> care about supporting MSVC, then we need to write it without the
> compound literal. MSVC doesn't support any C99 feature that is not also
> in C++, unfortunately.
Ok, better play it safe then. Thanks.
Martin
[PATCH] doc-diff: don't `cd_to_toplevel` before calling `usage`
`usage` tries to call $0, which might very well be "./doc-diff", so if
we `cd_to_toplevel` before calling `usage`, we'll end with an error to
the effect of "./doc-diff: not found" rather than a friendly `doc-diff
-h` output. Granted, all of these `usage` calls are in error paths, so
we're about to exit anyway, but the user experience of something like
`(cd Documentation && ./doc-diff)` could be a bit better than
"./doc-diff: not found".
This regressed in ad51743007 ("doc-diff: add --clean mode to remove
temporary working gunk", 2018-08-31) where we moved the call to
`cd_to_toplevel` to much earlier. Move it back to where it was, and
teach the "--clean" code to cd on its own. This way, we only cd once
we've verified the arguments.
Signed-off-by: Martin Ågren
---
Documentation/doc-diff | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/Documentation/doc-diff b/Documentation/doc-diff
index dfd9418778..f820febf8f 100755
--- a/Documentation/doc-diff
+++ b/Documentation/doc-diff
@@ -39,12 +39,11 @@ do
shift
done
-cd_to_toplevel
-tmp=Documentation/tmp-doc-diff
-
if test -n "$clean"
then
test $# -eq 0 || usage
+ cd_to_toplevel
+ tmp=Documentation/tmp-doc-diff
git worktree remove --force "$tmp/worktree" 2>/dev/null
rm -rf "$tmp"
exit 0
@@ -66,6 +65,9 @@ to=$1; shift
from_oid=$(git rev-parse --verify "$from") || exit 1
to_oid=$(git rev-parse --verify "$to") || exit 1
+cd_to_toplevel
+tmp=Documentation/tmp-doc-diff
+
if test -n "$force"
then
rm -rf "$tmp"
--
2.20.1.309.g16a465bc01
Re: [PATCH] doc-diff: don't `cd_to_toplevel` before calling `usage`
On Sun, 3 Feb 2019 at 10:12, Eric Sunshine wrote:
>
> On Sun, Feb 3, 2019 at 4:08 AM Eric Sunshine wrote:
> > I wonder if a more fruitful, longer-term fix which would save us from
> > having to worry about this in the future, would be to make
> > git-sh-setup.sh remember the original $0 before cd_to_toplevel() and
> > then employ the original value when usage() re-execs with the -h
> > option. That would also avoid the slightly ugly repeated
> > cd_to_top_level() and 'tmp' assignment in this patch.
>
> By "original $0", I meant a path which would be suitable for
> re-exec'ing (which wouldn't be the literal original $0). Sorry for the
> confusion.
I thought about this, and I probably should have said something about it
in the commit message. My uneducated guess was that "all" other users are
in $PATH and aren't being called like `./foobar`, but just `foobar`. Or,
they're internal helpers where the caller has already done the grunt
setup work, so their cd-ing is a no-op. I could be very wrong.
To be honest, I wasn't very tempted to risk breaking git-sh-setup
only(?) to help a development helper script like this. But let's see if
I can spend some more time on this...
The only way I'd know how to do something like this would be with
readlink or realpath. Judging by d2addc3b96 ("t7800: readlink may not be
available", 2016-05-31), we can't count on readlink. And I'd expect that
commit to have switched to realpath if THAT were available everywhere.
That commit instead goes for "ls | sed" and notes that "[t]his is no
universal readlink replacement but works in the controlled test
environment well enough."
Ok, so I am not too eager to try and tackle this with fallback
strategies and what-not. What would you say if I punted on this? I could
add something like this to the commit message:
A more general fix would be to teach git-sh-setup to save away the
absolute path for $0 and then use that, instead. I'm not aware of any
portable way of doing that, see, e.g., d2addc3b96 ("t7800: readlink
may not be available", 2016-05-31), so let's just fix this user
instead.
What do you think? Thanks for your comments.
Martin
[PATCH v2] doc-diff: don't `cd_to_toplevel` before calling `usage`
`usage` tries to call $0, which might very well be "./doc-diff", so if
we `cd_to_toplevel` before calling `usage`, we'll end with an error to
the effect of "./doc-diff: not found" rather than a friendly `doc-diff
-h` output. Granted, all of these `usage` calls are in error paths, so
we're about to exit anyway, but the user experience of something like
`(cd Documentation && ./doc-diff)` could be a bit better than
"./doc-diff: not found".
This regressed in ad51743007 ("doc-diff: add --clean mode to remove
temporary working gunk", 2018-08-31) where we moved the call to
`cd_to_toplevel` to much earlier. Move it back to where it was, and
teach the "--clean" code to cd on its own. This way, we only cd once
we've verified the arguments.
A more general fix would be to teach git-sh-setup to save away the
absolute path for $0 and then use that, instead. I'm not aware of any
portable way of doing that, see, e.g., d2addc3b96 ("t7800: readlink
may not be available", 2016-05-31), so let's just fix this user
instead.
Signed-off-by: Martin Ågren
---
> Punting and extending the commit message like that sounds reasonable.
So here's a v2 doing exactly that.
Documentation/doc-diff | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/Documentation/doc-diff b/Documentation/doc-diff
index dfd9418778..f820febf8f 100755
--- a/Documentation/doc-diff
+++ b/Documentation/doc-diff
@@ -39,12 +39,11 @@ do
shift
done
-cd_to_toplevel
-tmp=Documentation/tmp-doc-diff
-
if test -n "$clean"
then
test $# -eq 0 || usage
+ cd_to_toplevel
+ tmp=Documentation/tmp-doc-diff
git worktree remove --force "$tmp/worktree" 2>/dev/null
rm -rf "$tmp"
exit 0
@@ -66,6 +65,9 @@ to=$1; shift
from_oid=$(git rev-parse --verify "$from") || exit 1
to_oid=$(git rev-parse --verify "$to") || exit 1
+cd_to_toplevel
+tmp=Documentation/tmp-doc-diff
+
if test -n "$force"
then
rm -rf "$tmp"
--
2.20.1.309.g16a465bc01
[PATCH v3] doc-diff: don't `cd_to_toplevel`
`usage` tries to call $0, which might very well be "./doc-diff", so if
we `cd_to_toplevel` before calling `usage`, we'll end with an error to
the effect of "./doc-diff: not found" rather than a friendly `doc-diff
-h` output. This regressed in ad51743007 ("doc-diff: add --clean mode to
remove temporary working gunk", 2018-08-31) where we moved the call to
`cd_to_toplevel` to much earlier.
A general fix might be to teach git-sh-setup to save away the absolute
path for $0 and then use that, instead. I'm not aware of any portable
way of doing that, see, e.g., d2addc3b96 ("t7800: readlink may not be
available", 2016-05-31).
An early version of this patch moved `cd_to_toplevel` back to where it
was before ad51743007 and taught the "--clean" code to cd on its own.
But let's try instead to get rid of the cd-ing entirely. We don't really
need it and we can work with absolute paths instead. There's just one
use of $PWD that we need to adjust by simply dropping it.
Suggested-by: Jeff King
Signed-off-by: Martin Ågren
---
Thanks Peff for the suggestions. Trying not to cd at all seems sane to
me. That it allows `./Documentation/doc-diff` is a bonus, I guess, but
you're right that it's probably nothing anyone will use.
I've verified that diffs produced by `./Documentation/doc-diff foo bar`
and `./doc-diff foo bar` are identical, FWIW.
Martin
Documentation/doc-diff | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/Documentation/doc-diff b/Documentation/doc-diff
index dfd9418778..32c83dd26f 100755
--- a/Documentation/doc-diff
+++ b/Documentation/doc-diff
@@ -39,8 +39,7 @@ do
shift
done
-cd_to_toplevel
-tmp=Documentation/tmp-doc-diff
+tmp="$(git rev-parse --show-toplevel)/Documentation/tmp-doc-diff" || exit 1
if test -n "$clean"
then
@@ -109,7 +108,7 @@ render_tree () {
make -j$parallel -C "$tmp/worktree" \
GIT_VERSION=omitted \
SOURCE_DATE_EPOCH=0 \
- DESTDIR="$PWD/$tmp/installed/$1+" \
+ DESTDIR="$tmp/installed/$1+" \
install-man &&
mv "$tmp/installed/$1+" "$tmp/installed/$1"
fi &&
--
2.20.1.390.gb5101f9297
Re: [PATCH v1 0/2] minor asciidoc/tor formatting fixes
On Sat, 30 Mar 2019 at 19:30, Todd Zullinger wrote: > > Just chipping away at the remaining differences between asciidoc and > asciidoctor. > > Todd Zullinger (2): > Documentation/rev-list-options: wrap --date= block with "--" > Documentation/git-status: fix titles in porcelain v2 section Nice. I've tested and diffed across various dimensions. Looks good to me. Martin
Re: [PATCH v3.1 5/6] ci: stick with Asciidoctor v1.5.8 for now
Hi Junio
On Fri, 29 Mar 2019 at 20:52, SZEDER Gábor wrote:
>
> On Fri, Mar 29, 2019 at 01:35:19PM +0100, SZEDER Gábor wrote:
> > The release of Asciidoctor v2.0.0 two days ago broke our documentation
>
> Well, what happened "two days ago" when I sent v2 is now seven days
> ago... Let's just say "recent" instead.
>
>
> --- >8 ---
>
> Subject: ci: stick with Asciidoctor v1.5.8 for now
This was picked up as 28216d13f4 ("ci: stick with Asciidoctor v1.5.8 for
now", 2019-03-29) as part of sg/asciidoctor-in-ci. Actually, all of the
above is included, self-quote, scissors and all.
Martin
Re: [PATCH v1 0/2] minor asciidoc/tor formatting fixes
Hi Todd,
On Fri, 5 Apr 2019 at 03:40, Todd Zullinger wrote:
> > On Sat, 30 Mar 2019 at 19:30, Todd Zullinger wrote:
> >>
> >> Just chipping away at the remaining differences between asciidoc and
> >> asciidoctor.
> >>
> >> Todd Zullinger (2):
> >> Documentation/rev-list-options: wrap --date= block with "--"
> >> Documentation/git-status: fix titles in porcelain v2 section
> There are two other changes I've got queued locally. One in
> git-show-branch.txt removes the last use of {apostrophe}.
> Another in git-svn.txt is a bit of a work-around for a
> difference in the way asciidoc and asciidoctor parse the
> second paragraph in the CONFIGURATION section. That may
> well be an asiidoctor bug, but it seems like one we can
> adjust for without much effort.
The second one looks like it can be fixed by using `*` instead of '\*',
which I think is more correct anyway. I don't know what your local
workaround looks like, but I think a patch like "use backticks
consistently" (both change to them, in a number of places, and add them,
where we currently have nothing) would be a good change by itself, and
we could note that "BTW, this fixes ...". How does that compare to what
you have?
Martin
Re: [PATCH 0/2] a few more minor asciidoc/tor formatting fixes
On Sat, 6 Apr 2019 at 00:51, Todd Zullinger wrote: > Here's what I have currently. I haven't tested this much with > asciidoctor-1.5.x releases (or maybe not at all -- it's been a > week or so since I worked on this). Tested with Asciidoctor 1.5.5. For both patches, AsciiDoctor stumbles before the patch, but not after it. Great. :-) For AsciiDoc 8.6.10, the first patch is a no-op, while the second patch makes the difference it intends to do. I'll follow up with comments on the individual patches. Martin
Re: [PATCH 1/2] Documentation/git-show-branch: drop last use of {apostrophe}
On Sat, 6 Apr 2019 at 00:51, Todd Zullinger wrote:
>
> The {apostrophe} was needed at the time of a521845800 ("Documentation:
> remove stray backslash in show-branch discussion", 2010-08-20). All
> other uses of {apostrophe} were removed in 6cf378f0cb ("docs: stop using
> asciidoc no-inline-literal", 2012-04-26).
>
> Escape only the leading single-quote. This renders properly in asciidoc
> and asciidoctor.
You could perhaps say that "{apostrophe}" renders literally with
Asciidoctor (at least with 1.5.5). Right now, you sort of undersell this
patch. I know from context that you have some Asciidoctor "issue", but
it's not clear from this patch alone.
> ---
>
> Maybe it would be easier to change the example commit messages
> and avoid having to nest single quotes within double quotes? I
> don't know if that's much preferable to escaping only the opening
> single quote.
Yeah, those commit messages are a bit unfortunate from the point of view
of quoting. Avoiding all that nesting would probably improve the reading
experience, but I don't think that should hold up this patch.
> This is another potential parsing bug in asciidoctor. Of course,
> distros will have versions of asciidoctor in place for some time
> which have trouble parsing this doc. Since it's not much work
> for us to adjust the text to work around it, that seemed
> reasonable.
Agreed.
Martin
Re: [PATCH 2/2] Documentation/git-svn: improve asciidoctor compatibility
On Sat, 6 Apr 2019 at 00:51, Todd Zullinger wrote: > > The second paragraph in the CONFIGURATION section intends to emphasize > the word 'must' with bold type. Adjust the formatting slightly to > provide similar results between asciidoc and asciidoctor. I don't think this explains the problem you're solving well enough. It also doesn't make it clear enough, IMHO, how what we're doing here solves that problem. How about something like this? The second paragraph in the CONFIGURATION section intends to emphasize the word 'must' with bold type. It does so by writing it as *must*, and this works fine with AsciiDoc. It usually works great with Asciidoctor, too, but in this particular instance, we have another "*" earlier in the paragraph. We do escape it, and it is rendered literally just like we want it to, but Asciidoctor then ends up tripping on the second (or third) of the asterisks in this paragraph. Since that asterisk is (part of) a literal example, we can set it in monospace, by giving it as `*`. Adjust the whole paragraph in this way. There's lots more monospacing to be done in this document, but since our main motivation is addressing AsciiDoc/Asciidoctor discrepancies like this one, let's just convert this one paragraph. I think what's happening could be related to the fix in the first patch. There, it's ok to explicitly escape only the first '. The second one is matched to it and gets escaped implicitly. Something like that could be happening here, too, just that we don't want it to. (Should we escape the implicit escaping? ...) Just speculation, though. Martin
Re: [PATCH 1/1] t0001: fix on case-insensitive filesystems
Hi Dscho,
On Sat, 8 Jun 2019 at 16:45, Johannes Schindelin via GitGitGadget
wrote:
>
> From: Johannes Schindelin
>
> On a case-insensitive filesystem, such as HFS+ or NTFS, it is possible
> that the idea Bash has of the current directory differs in case from
> what Git thinks it is. That's totally okay, though, and we should not
> expect otherwise.
> +downcase_on_case_insensitive_fs () {
> + test false = "$(git config --get core.filemode)" || return 0
I think it would be worthwhile to add `--type=bool` to this git-config
call. See, e.g., the FILEMODE prereq in t/test-lib.sh. From my
understanding, this check would regress if someone did s/false/no/ in
builtin/init-db.c, so this check is perhaps not as robust as it could
be. (Now, as for *why* someone would do such a change...)
I do wonder if this is the right way to check for a case-insensitive
filesystem. According to git-config(1), this variable tells whether "the
executable bit of files in the working tree is to be honored". I can see
how this property could correlate with the filesystem being
case-insensitive, but from git-config(1), I would have expected
core.ignoreCase to be queried instead.
You're no doubt a lot more familiar with filesystem case-insensitivity and
how it interacts with Git than I am. Any light you could shed on this
would be much appreciated.
> + for f
> + do
> + tr A-Z a-z <"$f" >"$f".downcased &&
> + mv -f "$f".downcased "$f" || return 1
Makes sense. Good error-handling.
> + done
> +}
Cheers
Martin
Re: [GSoC][PATCH v3 1/3] sequencer: add advice for revert
Hi Rohit,
On Thu, 13 Jun 2019 at 19:46, Phillip Wood wrote:
>
> On 13/06/2019 05:05, Rohit Ashiwal wrote:
> > -static int create_seq_dir(void)
> > +static int create_seq_dir(struct repository *r)
> > {
> > - if (file_exists(git_path_seq_dir())) {
> > - error(_("a cherry-pick or revert is already in progress"));
> > - advise(_("try \"git cherry-pick (--continue | --quit |
> > --abort)\""));
> > + enum replay_action action;
> > + const char *in_progress_advice;
> > + const char *in_progress_error = NULL;
The assigning vs not assigning is a bit inconsistent, but that's a very
minor nit, and not why I started replying. Only noticed it just now. :-)
> > + if (!sequencer_get_last_command(r, &action)) {
> > + switch (action) {
> > + case REPLAY_REVERT:
> > + in_progress_error = _("revert is already in
> > progress");
> > + in_progress_advice =
> > + _("try \"git revert (--continue | --abort |
> > --quit)\"");
> > + break;
> > + case REPLAY_PICK:
> > + in_progress_error = _("cherry-pick is already in
> > progress");
> > + in_progress_advice =
> > + _("try \"git cherry-pick (--continue | --abort |
> > --quit)\"");
> > + break;
> > + default:
> > + BUG(_("the control must not reach here"));
>
> This does not need to be translated as BUG() messages are not really for
> users. Everything else looks fine to be now
I agree 100% with Phillip, but I'll also note that "the control must not
reach here" doesn't tell me anything that BUG() doesn't already. That
is, the point of BUG() is to document that, indeed, we shouldn't get
here and to alert if we do anyway.
An obvious alternative would be
BUG("action is neither revert nor pick");
but that doesn't say much more than the code already says quite clearly,
plus it risks getting outdated. I'd probably settle on something like
BUG("unexpected action in create_seq_dir");
which should give us a good clue even if all we have is this message (so
no file, no line number), but I am sure there are other good choices
here. :-)
Thanks Rohit for your work on this. I'm impressed by how you've polished
this series.
Martin
Re: [PATCH] cleanup: fix possible overflow errors in binary search, part 2
On Thu, 13 Jun 2019 at 19:54, René Scharfe wrote: > > Calculating the sum of two array indexes to find the midpoint between > them can overflow, i.e. code like this is unsafe for big arrays: > > mid = (first + last) >> 1; > > Make sure the intermediate value stays within the boundaries instead, > like this: > > mid = first + ((last - first) >> 1); > > The loop condition of the binary search makes sure that 'last' is > always greater than 'first', so this is safe as long as 'first' is > not negative. And that can be verified easily using the pre-context > of each change, except for name-hash.c, so add an assertion to that > effect there. Right, with "safe", one might mean something like "no undefined behavior due to shifting a signed value with the high bit set". Especially since we're worrying about overflows, we're obviously having large values in mind, so we're right to consider the sign bit. But, we're fine as you note. Because we subtract, and `last` doesn't have its sign bit set, and `first` is non-negative and not greater than `last`, the sign bit of `(last - first)` is always zero. So all is well. But maybe we should write `(last - first) / 2` anyway. We could then drop the extra parenthesis, and we would keep future readers (and static analysis?) from wondering whether we might ever be shifting a signed value with the sign bit set. A few spots fewer to audit in the future... Martin
Re: [PATCH] cleanup: fix possible overflow errors in binary search, part 2
On Thu, 13 Jun 2019 at 23:33, René Scharfe wrote: > > Am 13.06.19 um 21:42 schrieb Martin Ågren: > > On Thu, 13 Jun 2019 at 19:54, René Scharfe wrote: > >> Make sure the intermediate value stays within the boundaries instead, > >> like this: > >> > >> mid = first + ((last - first) >> 1); > >> > >> The loop condition of the binary search makes sure that 'last' is > >> always greater than 'first', so this is safe as long as 'first' is > >> not negative. And that can be verified easily using the pre-context > >> of each change, except for name-hash.c, so add an assertion to that > >> effect there. > > So all is well. But maybe we should write `(last - first) / 2` anyway. > > We could then drop the extra parenthesis, and we would keep future > > readers (and static analysis?) from wondering whether we might ever be > > shifting a signed value with the sign bit set. A few spots fewer to > > audit in the future... > > Yes, thought about that. When I saw Clang 8 emitting extra opcodes for > handling the sign for the version with division I decided to restrict > the patch to just do overflow prevention and leave the right shifts in > place.. Ah, ok, I should have known you were on top of it. I guess that means that clang isn't able to figure out we're guaranteed to be working with non-negative numbers. Which I guess means that it can't be sure the shift is safe, but with undefined behavior it has the leeway it needs, so... Martin
Re: [PATCH] doc: fix form -> from typo
Hi Catalin Welcome to the list! On Tue, 25 Jun 2019 at 09:43, Catalin Criste wrote: > @@ -88,7 +88,7 @@ save [-p|--patch] [-k|--[no-]keep-index] > [-u|--include-untracked] [-a|--all] [-q > > This option is deprecated in favour of 'git stash push'. It > differs from "stash push" in that it cannot take pathspecs, > - and any non-option arguments form the message. > + and any non-option arguments from the message. I think this is actually intended as "form". It took me a couple of readings, but what this paragraph wants to say is that any non-option arguments will be used to form (construct) the message. Do you have any suggestions as to how this could be made clearer? There are at least two of us that have stumbled on this. :-) Cheers Martin
Re: [PATCH] doc: fix form -> from typo
On Tue, 25 Jun 2019 at 13:40, Johannes Schindelin wrote: > On Tue, 25 Jun 2019, Martin Ågren wrote: > > Do you have any suggestions as to how this could be made clearer? > > There are at least two of us that have stumbled on this. :-) > > Make that three. > > Maybe something like > > This option is deprecated in favour of 'git stash push'. It > differs from "stash push" in that it cannot take pathspecs; > Instead, all non-option arguments are concatenated to form the stash > message. > > ? Looks good. I would probably avoid the semicolon ";" and in this case just replace it with a period. But it's nothing I feel strongly about. Martin
[PATCH] ref-filter: fix memory leak in `free_array_item()`
We treat the `value` pointer as a pointer to a struct and free its `s`
field. But `value` is in fact an array of structs. As a result, we only
free the first `s` out of `used_atom_cnt`-many and leak the rest. Make
sure we free all items in `value`.
In the caller, `ref_array_clear()`, this means we need to be careful not
to zero `used_atom_cnt` until after we've called `free_array_item()`. We
could move just a single line, but let's keep related things close
together instead, by first handling `array`, then `used_atom`.
Signed-off-by: Martin Ågren
---
ref-filter.c | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 791f0648a6..1c1a2af880 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2105,7 +2105,9 @@ static void free_array_item(struct ref_array_item *item)
{
free((char *)item->symref);
if (item->value) {
- free((char *)item->value->s);
+ int i;
+ for (i = 0; i < used_atom_cnt; i++)
+ free((char *)item->value[i].s);
free(item->value);
}
free(item);
@@ -2116,14 +2118,16 @@ void ref_array_clear(struct ref_array *array)
{
int i;
- for (i = 0; i < used_atom_cnt; i++)
- free((char *)used_atom[i].name);
- FREE_AND_NULL(used_atom);
- used_atom_cnt = 0;
for (i = 0; i < array->nr; i++)
free_array_item(array->items[i]);
FREE_AND_NULL(array->items);
array->nr = array->alloc = 0;
+
+ for (i = 0; i < used_atom_cnt; i++)
+ free((char *)used_atom[i].name);
+ FREE_AND_NULL(used_atom);
+ used_atom_cnt = 0;
+
if (ref_to_worktree_map.worktrees) {
hashmap_free(&(ref_to_worktree_map.map), 1);
free_worktrees(ref_to_worktree_map.worktrees);
--
2.22.0.428.g6d5b264208
Re: [PATCH] log: use mailmap by default
Hi Ariadne, Welcome to the list! On Thu, 11 Jul 2019 at 18:39, Ariadne Conill wrote: > > Hello, > > On Thu, Jul 11, 2019 at 10:19 AM brian m. carlson > wrote: > > > > On 2019-07-11 at 08:19:58, Ariadne Conill wrote: > > > The `git log` command shows the author and committer name recorded in > > > the git repository itself, while other commands respect `.mailmap` > > > by default. I believe this is a bad design: it causes log entries to > > > reflect inaccurate information: anyone who changes their name or > > > e-mail address will not have that change (recorded in mailmap file) > > > reflected when using `git log` by default. > > > > > > Anyone who explicitly wants the current behaviour can clearly request > > > it by setting the `log.mailmap` setting to `false` in their > > > `.gitconfig` file. It would be useful with some tests for this. That would be a nice way to showcase how this is meant to work, and to protect this from being broken in the future. From looking around in t/, it looks like t4203-mailmap.sh could be a good spot. > > While I'm in favor of using the mailmap by default, typically we want a > > way people can override a default setting from the command line. > > > > So in this case, we have "--use-mailmap", but we don't have a > > "--no-use-mailmap" (at least, it's not documented in the manpage). I > > think we'd want to add such an option so that people can set it if they > > need non-default behavior. > > I agree that there is probably some useful reasons to have this > option, so I can add an option that forces mailmap usage off to > supplement the --use-mailmap option. It's no problem. I will > generate a new patch series in a few minutes. There are some tests already in t4203 which test that for example `git log --use-mailmap ...` does the right thing. If your implementation is correct, it should be possible to drop `--use-mailmap` from there and everything should work. It would then be very useful to have tests that `--no-use-mailmap` does the right thing. There's also some testing around the `log.mailmap` config, which you should probably adapt and possibly extend. The documentation for `log.mailmap` will need to be updated, since right now it says (or at least implies) that the default is "false". This looks like one of those very small and simple changes that turn into quite some work to ensure that the tests are all there and that the documentation is up to date. :-) Cheers Martin
Re: [PATCH v4 3/3] tests: rework mailmap tests for git log
On Thu, 11 Jul 2019 at 20:39, Ariadne Conill wrote: > > In order to prove that the --no-use-mailmap option works as expected, > we add a test for it which runs with -c log.mailmap=true to ensure that > the option successfully negates the configured default. I believe that testing with `-c log.mailmap=true` is not doing much -- if we ignored that config entirely, we would still produce the wanted result. I think it's more important to test with "...=false". (Testing something like `-c log.mailmap=false -c log.mailmap=true` would basically just test our config-parsing in general, and we don't need to do that here -- there are other tests for that. Anyway, I digress.) You or others might very well disagree with me, so feel free to wait for a while to see if others chime in. Just so you don't have to change back and forth due to my whims. > Additionally, since --use-mailmap is now the default behaviour, we > remove mentions of --use-mailmap from the tests, since they are > redundant. We also rework some tests to explicitly define the > log.mailmap variable in both true and false states. > > Signed-off-by: Ariadne Conill > --- > t/t4203-mailmap.sh | 49 ++ > 1 file changed, 41 insertions(+), 8 deletions(-) > > diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh > index 43b1522ea2..3d6086ff96 100755 > --- a/t/t4203-mailmap.sh > +++ b/t/t4203-mailmap.sh > @@ -422,8 +422,8 @@ Author: Some Dude > Author: A U Thor > EOF > > -test_expect_success 'Log output with --use-mailmap' ' > - git log --use-mailmap | grep Author >actual && > +test_expect_success 'Log output with mailmap enabled (default)' ' > + git log | grep Author >actual && > test_cmp expect actual > ' It's a bit unfortunate that we're ignoring the exit status of `git log` since that is the exact command we want to test here. I know you're just following suit here, but if you're touching this line /anyway/, it might make sense to rewrite this into git log ... >out && grep Author out >actual && ... > -test_expect_success 'Log output with log.mailmap' ' > +test_expect_success 'Log output with log.mailmap enabled in config' ' > git -c log.mailmap=True log | grep Author >actual && > test_cmp expect actual > ' Then the question is if you should change this line "while at it", or start with a preparatory patch to first just convert all these "git log | grep" to the pattern I showed above, then have a patch 2/2 with the actual change you want to make. I'm sure there are different opinions here about what is right and not. Anyway, I'm not the one to complain if you just ignore all of these "not so optimal" lines that you're not touching anyway. BTW, this is a test where I wonder if it's really worth running. We basically just check that we won't choke completely on this redundant config. > +cat >expect <<\EOF > +Author: CTO > +Author: claus > +Author: santa > +Author: nick2 > +Author: nick2 > +Author: nick1 > +Author: A U Thor > +EOF > + > +test_expect_success 'Log output with log.mailmap disabled in config' ' > + git -c log.mailmap=False log | grep Author >actual && > + test_cmp expect actual > +' Now this test, on the other hand, I really like having! Again, you're just following suit: Nowadays, we try to run things like "cat ...>expect ..." as part of a "test_expect_success" block. Same question about how maybe one should first convert all existing instances. And again, IMHO it's perfectly fine if you ignore the existing ones, but do it the "correct" way for the ones you're adding. > + > cat >expect <<\EOF > Author: Santa Claus > Author: Santa Claus > EOF > > -test_expect_success 'Grep author with --use-mailmap' ' > - git log --use-mailmap --author Santa | grep Author >actual && > +test_expect_success 'Grep author with mailmap enabled (default)' ' > + git log --author Santa | grep Author >actual && > test_cmp expect actual > ' > cat >expect <<\EOF > @@ -456,16 +471,34 @@ Author: Santa Claus > Author: Santa Claus > EOF > > -test_expect_success 'Grep author with log.mailmap' ' > +test_expect_success 'Grep author with log.mailmap enabled' ' > git -c log.mailmap=True log --author Santa | grep Author >actual && > test_cmp expect actual > ' (Again, I kind of wonder what this buys us.) > -test_expect_success 'Only grep replaced author with --use-mailmap' ' > - git log --use-mailmap --author "" >actual && > +test_expect_success 'Grep author with log.mailmap disabled' ' > + git -c log.mailmap=False log --author "" > >actual && > + test_must_be_empty actual > +' Nice. > +test_expect_success 'Grep author with --no-use-mailmap' ' > + git log --no-use-mailmap --author "" > >actual && > test_must_be_empty actual > ' Nice. > +test_expect_success 'Only grep replaced author with mailmap enabled' ' > + git log --author "" >actual && > + test_must_be_empty
Re: Pull vs push messages
On Fri, 12 Jul 2019 at 00:16, brian m. carlson
wrote:
>
> On 2019-07-11 at 21:36:50, Michael Kielstra wrote:
> > Hi all,
> >
> > I noticed that git pull reports "Already up to date." but git push
> > reports "Everything up-to-date". (I'm using git 2.20.1, the latest in
> > the Ubuntu repos.) Just for a consistent user experience, would it be
> > worth standardizing on:
> >
> > Hyphenation (up-to-date vs up to date)?
> > Periods at the end of one-sentence messages?
> > Colloquialisms and tone of voice? "Already up to date." sounds like a
> > terse error message but "Everything up-to-date" sounds like a chatty
> > friend.
>
> I'd be happy to review a patch that changes this, if you think it's
> worth changing. Generally the way things work here is that except for
> obvious bugs, people send patches for things they care about, and then
> other folks will review and make suggestions, or sometimes there won't
> be any interest in a change, and the patch is dropped.
>
> We'd probably want to standardize on "up to date", since that's the
> correct form here according to the Chicago Manual of Style, and drop the
> period, since this isn't a complete sentence.
There's 7560f547e6 ('treewide: correct several "up-to-date" to "up to
date"', 2017-08-23), which changed a few of these, but also explains why
it leaves "Everything up-to-date" unchanged. Whether that assessment is
the One True Way now 2 years later, I'm not the right person to say.
Michael, you can perhaps find some discussion leading up to / about that
patch on https://public-archive.org/git/
Martin
Re: [PATCH v2 1/4] git-merge: Honor pre-merge hook
On Fri, 19 Jul 2019 at 00:57, Josh Steadmon wrote: > --- /dev/null > +++ b/templates/hooks--pre-merge.sample > @@ -0,0 +1,13 @@ > +#!/bin/sh > +# > +# An example hook script to verify what is about to be committed. > +# Called by "git merge" with no arguments. The hook should > +# exit with non-zero status after issuing an appropriate message to > +# stderr if it wants to stop the merge commit. > +# > +# To enable this hook, rename this file to "pre-merge". > + > +. git-sh-setup > +test -x "$GIT_DIR/hooks/pre-commit" && > +exec "$GIT_DIR/hooks/pre-commit" > +: I'm pretty certain that my comment here was with respect to the ":", which made me think "hmmm, won't : always exit successfully?". My slightly less ignorant self of today answers "sure, but we won't always get that far" -- the "exec" is pretty important, and it will cause the current shell to be replaced with the hook invocation. So however the pre-commit hook decides to exit, that'll be our exit status. I retract my comment from back then. Martin
Re: [PATCH v2 3/4] merge: --no-verify to bypass pre-merge hook
On Fri, 19 Jul 2019 at 00:57, Josh Steadmon wrote: > From: Michael J Gruber > > Analogous to commit, introduce a '--no-verify' option which bypasses the > pre-merge hook. The shorthand '-n' is taken by the (non-existing) > '--no-stat' already. I don't understand this "(non-existing)". I realize this message is two years old, maybe lots older still, and I haven't dug. But at least *now*, "-n" seems to behave like "--no-stat", suppressing "--stat", from my simple testing. Martin
Re: [PATCH v2 4/4] t7503: add tests for pre-merge-hook
On Fri, 19 Jul 2019 at 00:57, Josh Steadmon wrote: > +test_expect_success '--no-verify with succeeding hook (merge)' ' > + > + git checkout side && > + git merge --no-verify -m "merge master" master && > + git checkout master > + > +' This test doesn't even try to verify that the hook was actually ignored. That left me puzzled for a while... > +test_expect_success '--no-verify with failing hook (merge)' ' > + > + git checkout side && > + git merge --no-verify -m "merge master" master && > + git checkout master > + > +' ... but this would then (most likely) fail, so we would notice something's wrong. This script seems to me like if it passes 100%, we can be fairly sure we're ok, but if some individual test fails, we shouldn't be surprised if its oneline description is a bit off compared to the bug. Similarly, quite a few tests could pass, despite their oneline description inducing the thought of "but surely, if /that/ were the problem, /those/ tests would fail". Anyway, I realize this is just following the existing approach. I guess you could argue it has served us well for a long time. I would probably prefer seeing the various hunks in this patch being squashed into the relevant commits (1/4 vs 3/4) to make those patches more self-describing. Martin
Re: [PATCH v2 0/4] pre-merge hook
On Fri, 19 Jul 2019 at 01:56, Josh Steadmon wrote: > * Martin's objection on 1/4 that the sample hook would always exit > successfully is (I believe) incorrect. To test this, I ran > "/bin/true && exec test 0 == 1" in a /bin/sh subshell, and it > correctly had a non-zero exit status. I retract my comment on this. It was incorrect, indeed. > git-merge: Honor pre-merge hook Nit: s/H/h/ > merge: do no-verify like commit Nit (maybe just me): this could be patch 1/N, before getting started with the real focus of this series. Martin
Re: [PATCH v2 0/4] pre-merge hook
On Fri, 19 Jul 2019 at 01:56, Josh Steadmon wrote: > This series revives an old suggestion [...] to make merge honor > pre-commit hook or a separate pre-merge hook. Since pre-commit does not > receive any arguments (which the hook could use tell between commit and > merge) and in order to keep existing behaviour (as opposed to the other > patch) this series introduces a pre-merge hook, with a sample hook that > simply calls the pre-commit hook. Argh, I wanted to comment on this in the cover letter, but forgot and just left a bunch of nits. I wonder if we might ever regret naming this "pre-merge" and not, e.g., "pre-merge-commit". There's the pre-rebase hook which runs before git-rebase even starts actually rebasing, so I could well imagine a "pre-merge" hook which would get called before merging starts. I'm also pondering whether there should be an "automatic" in there, but "pre-automatic-merge-commit" looks a bit awkward. Anyway, I'm not even sure an "automatic merge commit" is a well-defined thing. I can figure out what it pretty certainly means, but it's not crystal clear. There might be a need for some more discussion in the added docs for what this new hook is for and how it compares to pre-commit. Right now, the proposed docs suggests they're equivalent in a way, but I think that's a bit confusing -- they are certainly not synonyms, and they'd never both be called, for example. They can be used for the same purpose in different scenarios, sure. I tried coming up with some proposed docs for githooks.txt, but didn't feel I achieved much of value. :-/ Martin
Re: [PATCH v2 4/4] t7503: add tests for pre-merge-hook
On Tue, 30 Jul 2019 at 01:43, Josh Steadmon wrote: > > On 2019.07.29 22:04, Martin Ågren wrote: > > This script seems to me like if it passes 100%, we can be fairly sure > > we're ok, but [...] > Will squash these as you said in V3. Will also think about whether > another test approach would make more sense here. Thinking a bit more about this, this test uses two identical hooks, runs some commands and verifies that "the" hook was run (or not, with --no-verify). If the implementation started calling the wrong hook (pre-commit / pre-merge) or both hooks, we wouldn't notice. Please forgive my braindump below, I'm on the run so I'm just throwing this out there: Perhaps (first do some modernizing of this script, to protect various setup steps, use "write_script", etc, then) make the existing hook a tiny bit pre-commit-specific, e.g., by doing something like "echo pre-commit >>executed-hooks", then at select places check "test_cmp executed-hooks pre-commit" (against "echo pre-commit >pre-commit"), "test_path_is_missing executed-hooks", and so on, coupled with some "test_when_finished 'rm -f executed_hooks'". Then the tests added for this series would use a very similar hook, appending and checking for "pre-merge[-commit]", That should make us fairly certain that we're running precisely the wanted hook, I think. Martin
[PATCH] RelNotes/2.23.0: fix a few typos and other minor issues
Fix the spelling of the new "--no-show-forced-updates" option that "git fetch/pull" learned. Similarly, spell "--function-context" correctly and fix a few typos, grammos and minor mistakes. One of these is also in 2.22.1.txt, so fix it there too. Signed-off-by: Martin Ågren --- Documentation/RelNotes/2.22.1.txt | 2 +- Documentation/RelNotes/2.23.0.txt | 14 +++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/Documentation/RelNotes/2.22.1.txt b/Documentation/RelNotes/2.22.1.txt index 78b2c5ea8a..76dd8fb578 100644 --- a/Documentation/RelNotes/2.22.1.txt +++ b/Documentation/RelNotes/2.22.1.txt @@ -94,7 +94,7 @@ Fixes since v2.22 * The configuration variable rebase.rescheduleFailedExec should be effective only while running an interactive rebase and should not - affect anything when running an non-interactive one, which was not + affect anything when running a non-interactive one, which was not the case. This has been corrected. * "git submodule foreach" did not protect command line options passed diff --git a/Documentation/RelNotes/2.23.0.txt b/Documentation/RelNotes/2.23.0.txt index 19e894a44e..e1b1ce5680 100644 --- a/Documentation/RelNotes/2.23.0.txt +++ b/Documentation/RelNotes/2.23.0.txt @@ -52,7 +52,7 @@ UI, Workflows & Features * "git fetch" and "git pull" reports when a fetch results in non-fast-forward updates to let the user notice unusual situation. - The commands learned "--no-shown-forced-updates" option to disable + The commands learned "--no-show-forced-updates" option to disable this safety feature. * Two new commands "git switch" and "git restore" are introduced to @@ -68,7 +68,7 @@ UI, Workflows & Features * The conditional inclusion mechanism learned to base the choice on the branch the HEAD currently is on. - * "git rev-list --objects" learned with "--no-object-names" option to + * "git rev-list --objects" learned the "--no-object-names" option to squelch the path to the object that is used as a grouping hint for pack-objects. @@ -106,7 +106,7 @@ Performance, Internal Implementation, Development Support etc. no longer be used. * Developer support to emulate unsatisfied prerequisites in tests to - ensure that the remainer of the tests still succeeds when tests + ensure that the remainder of the tests still succeeds when tests with prerequisites are skipped. * "git update-server-info" learned not to rewrite the file with the @@ -121,7 +121,7 @@ Performance, Internal Implementation, Development Support etc. * Prepare use of reachability index in topological walker that works on a range (A..B). - * A new tutorial targetting specifically aspiring git-core + * A new tutorial targeting specifically aspiring git-core developers has been added. * Auto-detect how to tell HP-UX aCC where to use dynamically linked @@ -181,7 +181,7 @@ Fixes since v2.22 the same repository was corrupt, which has been corrected. * The ownership rule for the file descriptor to fast-import remote - backend was mixed up, leading to unrelated file descriptor getting + backend was mixed up, leading to an unrelated file descriptor getting closed, which has been fixed. * A "merge -c" instruction during "git rebase --rebase-merges" should @@ -306,7 +306,7 @@ Fixes since v2.22 * The configuration variable rebase.rescheduleFailedExec should be effective only while running an interactive rebase and should not - affect anything when running an non-interactive one, which was not + affect anything when running a non-interactive one, which was not the case. This has been corrected. * The "git clone" documentation refers to command line options in its @@ -339,7 +339,7 @@ Fixes since v2.22 having to consult the other end, which has been corrected. * The internal diff machinery can be made to read out of bounds while - looking for --funcion-context line in a corner case, which has been + looking for --function-context line in a corner case, which has been corrected. (merge b777f3fd61 jk/xdiff-clamp-funcname-context-index later to maint). -- 2.23.0.rc0
Re: [PATCH] RelNotes/2.23.0: fix a few typos and other minor issues
On Thu, 1 Aug 2019 at 17:57, Junio C Hamano wrote: > > Martin Ågren writes: > > > Fix the spelling of the new "--no-show-forced-updates" option that "git > > fetch/pull" learned. Similarly, spell "--function-context" correctly and > > fix a few typos, grammos and minor mistakes. > > > > One of these is also in 2.22.1.txt, so fix it there too. > > Thanks, but it would have been better for this to be in two > patches. I'll split them up. Oh, right, I suppose you'd *much* prefer a patch that applies on maint for the 2.22.1-fix. It didn't occur to me. Sorry about that. Thanks for adjusting on your end. Martin
Re: [PATCH v3 3/4] git-merge: honor pre-merge-commit hook
On Fri, 2 Aug 2019 at 00:20, Josh Steadmon wrote: > diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt > index 82cd573776..7c4c994858 100644 > --- a/Documentation/githooks.txt > +++ b/Documentation/githooks.txt > @@ -103,6 +103,13 @@ The default 'pre-commit' hook, when enabled--and with the > `hooks.allownonascii` config option unset or set to false--prevents > the use of non-ASCII filenames. > > +pre-merge-commit > + > + > +This hook is invoked by 'git merge' when doing an automatic merge > +commit; it is equivalent to 'pre-commit' for a non-automatic commit > +for a merge. > + I'm not sure everyone understands what an "automatic merge commit" is. (Is it an automatic "merge commit", or an "automatic merge" commit? Or sort of both?) And I'm not sure exactly what to infer from the "equivalence". I happen to know that the statement about the default hook can only be half-carried over. And I'm not sure what to infer from "All the git commit hooks are invoked with the environment variable ...". Is the below suggestion 1) correct, 2) readable? This hook is invoked by linkgit:git-merge[1], and can be bypassed with the `--no-verify` option. It takes no parameters, and is invoked after the merge has been carried out successfully and before obtaining the proposed commit log message to make a commit. Exiting with a non-zero status from this script causes the `git merge` command to abort before creating a commit. The default 'pre-merge-commit' hook, when enabled, runs the 'pre-commit' hook, if the latter is enabled. This hook is invoked with the environment variable `GIT_EDITOR=:` if the command will not bring up an editor to modify the commit message. If the merge cannot be carried out automatically, the conflicts need to be resolved and the result committed separately (see linkgit:git-merge[1]). At that point, this hook will not be executed, but the 'pre-commit' hook will, if it is enabled. (If you use this or something like it, notice how this already mentions `--no-verify`...) > +test_expect_success 'root commit' ' > + echo "root" > file && > + git add file && > + git commit -m "zeroth" && > + git checkout -b side && > + echo "foo" > foo && > + git add foo && > + git commit -m "make it non-ff" && > + git checkout master > +' You got rid of loads of "> file" in patch 1/4, so it seems unfortunate to introduce a few here. ;-) Martin
[PATCH 2/5] t7503: avoid touch when mtime doesn't matter
AFAIU, we avoid using `touch` unless we really do want to update the mtime. Here, we just want to create empty files, so write `>foo` instead. Signed-off-by: Martin Ågren --- ...3-pre-commit-and-pre-merge-commit-hooks.sh | 38 --- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh index 5cc6c98375..a6f1240aa2 100755 --- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh +++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh @@ -43,7 +43,8 @@ test_expect_success 'root commit' ' test_expect_success 'with no hook' ' test_when_finished "rm -f expected_hooks actual_hooks" && - touch expected_hooks actual_hooks && + >expected_hooks && + >actual_hooks && echo "foo" >file && git add file && git commit -m "first" && @@ -52,7 +53,8 @@ test_expect_success 'with no hook' ' test_expect_success 'with no hook (merge)' ' test_when_finished "rm -f expected_hooks actual_hooks" && - touch expected_hooks actual_hooks && + >expected_hooks && + >actual_hooks && git checkout side && git merge -m "merge master" master && git checkout master && @@ -61,7 +63,8 @@ test_expect_success 'with no hook (merge)' ' test_expect_success '--no-verify with no hook' ' test_when_finished "rm -f expected_hooks actual_hooks" && - touch expected_hooks actual_hooks && + >expected_hooks && + >actual_hooks && echo "bar" >file && git add file && git commit --no-verify -m "bar" && @@ -70,7 +73,8 @@ test_expect_success '--no-verify with no hook' ' test_expect_success '--no-verify with no hook (merge)' ' test_when_finished "rm -f expected_hooks actual_hooks" && - touch expected_hooks actual_hooks && + >expected_hooks && + >actual_hooks && git checkout side && git merge --no-verify -m "merge master" master && git checkout master && @@ -80,7 +84,7 @@ test_expect_success '--no-verify with no hook (merge)' ' test_expect_success 'with succeeding hook' ' test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" && ln -s "success.sample" "$PRECOMMIT" && - touch actual_hooks && + >actual_hooks && echo "$PRECOMMIT" >expected_hooks && echo "more" >>file && git add file && @@ -91,7 +95,7 @@ test_expect_success 'with succeeding hook' ' test_expect_success 'with succeeding hook (merge)' ' test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" && ln -s "success.sample" "$PREMERGE" && - touch actual_hooks && + >actual_hooks && echo "$PREMERGE" >expected_hooks && git checkout side && git merge -m "merge master" master && @@ -102,7 +106,8 @@ test_expect_success 'with succeeding hook (merge)' ' test_expect_success '--no-verify with succeeding hook' ' test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" && ln -s "success.sample" "$PRECOMMIT" && - touch expected_hooks actual_hooks && + >expected_hooks && + >actual_hooks && echo "even more" >>file && git add file && git commit --no-verify -m "even more" && @@ -112,7 +117,8 @@ test_expect_success '--no-verify with succeeding hook' ' test_expect_success '--no-verify with succeeding hook (merge)' ' test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" && ln -s "success.sample" "$PREMERGE" && - touch expected_hooks actual_hooks && + >expected_hooks && + >actual_hooks && git checkout side && git merge --no-verify -m "merge master" master && git checkout master && @@ -122,7 +128,7 @@ test_expect_success '--no-verify with
[PATCH 5/5] t7503: test failing merge with both hooks available
With a failing automatic merge, we want to make sure that we *don't* call the pre-merge-commit hook and that we (eventually) *do* call the pre-commit hook. Signed-off-by: Martin Ågren --- ...3-pre-commit-and-pre-merge-commit-hooks.sh | 30 +++ 1 file changed, 30 insertions(+) diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh index f0c73fd58d..4bf359c097 100755 --- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh +++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh @@ -41,6 +41,18 @@ test_expect_success 'root commit' ' git checkout master ' +test_expect_success 'setup conflicting branches' ' + test_when_finished "git checkout master" && + git checkout -b conflicting-a master && + echo a >conflicting && + git add conflicting && + git commit -m conflicting-a && + git checkout -b conflicting-b master && + echo b >conflicting && + git add conflicting && + git commit -m conflicting-b +' + test_expect_success 'with no hook' ' test_when_finished "rm -f actual_hooks" && echo "foo" >file && @@ -93,6 +105,24 @@ test_expect_success 'with succeeding hook (merge)' ' test_cmp expected_hooks actual_hooks ' +test_expect_success 'automatic merge fails; both hooks are available' ' + test_when_finished "rm -f \"$PREMERGE\" \"$PRECOMMIT\"" && + test_when_finished "rm -f expected_hooks actual_hooks" && + test_when_finished "git checkout master" && + ln -s "success.sample" "$PREMERGE" && + ln -s "success.sample" "$PRECOMMIT" && + + git checkout conflicting-a && + test_must_fail git merge -m "merge conflicting-b" conflicting-b && + test_path_is_missing actual_hooks && + + echo "$PRECOMMIT" >expected_hooks && + echo a+b >conflicting && + git add conflicting && + git commit -m "resolve conflict" && + test_cmp expected_hooks actual_hooks +' + test_expect_success '--no-verify with succeeding hook' ' test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" && ln -s "success.sample" "$PRECOMMIT" && -- 2.23.0.rc0.30.g51cf315870
Re: [PATCH v3 0/4] pre-merge-commit hook
[Dropped cc-list the first time around. Apologies to those who receive this twice...] On Fri, 2 Aug 2019 at 00:20, Josh Steadmon wrote: > > This series adds a new pre-merge-commit hook, similar in usage to > pre-commit. It also improves hook testing in t7503, by verifying that > the correct hooks are run or bypassed as expected. I really like those test improvements. Now it should be harder to mess up a future refactoring and run the wrong hook. These hooks are "very related" so I think this is important. I've messed with the test a bit and offer these potential improvements for your consideration. I was lazy and just built this on top of your series -- if you agree to some or all of these, you'll probably need to squash them into a few individual patches. The first four are perhaps more or less a matter of opinion, although I do think that patch 2/5 is based on an opinion shared by others. ;-) Patch 5/5 or something like it seems pretty important to me to make sure that of these two "similar"/"related" hooks with some "backwards-compatibility-and-code-copying-history" around them, we'd better pick the right one when they're both available. Feel free to pick and squash as you see fit. (I don't think it makes sense to have these go in as-are. They really are meant for squashing.) Martin Martin Ågren (5): t7503: use "&&" in "test_when_finished" rather than ";" t7503: avoid touch when mtime doesn't matter t7503: simplify file-juggling t7503: don't create "actual_hooks" for later appending t7503: test failing merge with both hooks available ...3-pre-commit-and-pre-merge-commit-hooks.sh | 84 +++ 1 file changed, 50 insertions(+), 34 deletions(-) -- 2.23.0.rc0.30.g51cf315870
[PATCH 4/5] t7503: don't create "actual_hooks" for later appending
If we fail to call the hook, we won't append to "actual_hooks". "test_cmp" is able to handle a missing file just fine, so these "pre-creations" are mostly confusing. Let's drop them. Signed-off-by: Martin Ågren -- This "pre-creation" does protect against the file already existing, i.e., it could be seen as a "clearing". But since we use "test_when_finished" in this script... Also, these were "touch" before another of my suggestions, so these really were "pre-creating" these files if needed. t/t7503-pre-commit-and-pre-merge-commit-hooks.sh | 4 1 file changed, 4 deletions(-) diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh index 477207cb5c..f0c73fd58d 100755 --- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh +++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh @@ -76,7 +76,6 @@ test_expect_success '--no-verify with no hook (merge)' ' test_expect_success 'with succeeding hook' ' test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" && ln -s "success.sample" "$PRECOMMIT" && - >actual_hooks && echo "$PRECOMMIT" >expected_hooks && echo "more" >>file && git add file && @@ -87,7 +86,6 @@ test_expect_success 'with succeeding hook' ' test_expect_success 'with succeeding hook (merge)' ' test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" && ln -s "success.sample" "$PREMERGE" && - >actual_hooks && echo "$PREMERGE" >expected_hooks && git checkout side && git merge -m "merge master" master && @@ -116,7 +114,6 @@ test_expect_success '--no-verify with succeeding hook (merge)' ' test_expect_success 'with failing hook' ' test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" && ln -s "fail.sample" "$PRECOMMIT" && - >actual_hooks && echo "$PRECOMMIT" >expected_hooks && echo "another" >>file && git add file && @@ -136,7 +133,6 @@ test_expect_success '--no-verify with failing hook' ' test_expect_success 'with failing hook (merge)' ' test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" && ln -s "fail.sample" "$PREMERGE" && - >actual_hooks && echo "$PREMERGE" >expected_hooks && git checkout side && test_must_fail git merge -m "merge master" master && -- 2.23.0.rc0.30.g51cf315870
[PATCH 1/5] t7503: use "&&" in "test_when_finished" rather than ";"
Signed-off-by: Martin Ågren --- t/t7503-pre-commit-and-pre-merge-commit-hooks.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh index 86a375ab3e..5cc6c98375 100755 --- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh +++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh @@ -162,7 +162,7 @@ test_expect_success '--no-verify with failing hook (merge)' ' ' test_expect_success POSIXPERM 'with non-executable hook' ' - test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks; chmod +x \"$HOOKDIR/fail.sample\"" && + test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks && chmod +x \"$HOOKDIR/fail.sample\"" && ln -s "fail.sample" "$PRECOMMIT" && chmod -x "$HOOKDIR/fail.sample" && touch expected_hooks actual_hooks && @@ -173,7 +173,7 @@ test_expect_success POSIXPERM 'with non-executable hook' ' ' test_expect_success POSIXPERM '--no-verify with non-executable hook' ' - test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks; chmod +x \"$HOOKDIR/fail.sample\"" && + test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks && chmod +x \"$HOOKDIR/fail.sample\"" && ln -s "fail.sample" "$PRECOMMIT" && chmod -x "$HOOKDIR/fail.sample" && touch expected_hooks actual_hooks && -- 2.23.0.rc0.30.g51cf315870
[PATCH 3/5] t7503: simplify file-juggling
Rather than creating an empty "expected_hooks" file and another empty file "actual_hooks" (which we want to not be appended to), just verify that "actual_hooks" isn't created. Do still clean up the "actual_hooks" file, so that a failing test won't start affecting later tests too. Signed-off-by: Martin Ågren --- ...3-pre-commit-and-pre-merge-commit-hooks.sh | 60 +++ 1 file changed, 20 insertions(+), 40 deletions(-) diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh index a6f1240aa2..477207cb5c 100755 --- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh +++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh @@ -42,43 +42,35 @@ test_expect_success 'root commit' ' ' test_expect_success 'with no hook' ' - test_when_finished "rm -f expected_hooks actual_hooks" && - >expected_hooks && - >actual_hooks && + test_when_finished "rm -f actual_hooks" && echo "foo" >file && git add file && git commit -m "first" && - test_cmp expected_hooks actual_hooks + test_path_is_missing actual_hooks ' test_expect_success 'with no hook (merge)' ' - test_when_finished "rm -f expected_hooks actual_hooks" && - >expected_hooks && - >actual_hooks && + test_when_finished "rm -f actual_hooks" && git checkout side && git merge -m "merge master" master && git checkout master && - test_cmp expected_hooks actual_hooks + test_path_is_missing actual_hooks ' test_expect_success '--no-verify with no hook' ' - test_when_finished "rm -f expected_hooks actual_hooks" && - >expected_hooks && - >actual_hooks && + test_when_finished "rm -f actual_hooks" && echo "bar" >file && git add file && git commit --no-verify -m "bar" && - test_cmp expected_hooks actual_hooks + test_path_is_missing actual_hooks ' test_expect_success '--no-verify with no hook (merge)' ' - test_when_finished "rm -f expected_hooks actual_hooks" && - >expected_hooks && - >actual_hooks && + test_when_finished "rm -f actual_hooks" && git checkout side && git merge --no-verify -m "merge master" master && git checkout master && - test_cmp expected_hooks actual_hooks + test_path_is_missing actual_hooks ' test_expect_success 'with succeeding hook' ' @@ -104,25 +96,21 @@ test_expect_success 'with succeeding hook (merge)' ' ' test_expect_success '--no-verify with succeeding hook' ' - test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" && + test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" && ln -s "success.sample" "$PRECOMMIT" && - >expected_hooks && - >actual_hooks && echo "even more" >>file && git add file && git commit --no-verify -m "even more" && - test_cmp expected_hooks actual_hooks + test_path_is_missing actual_hooks ' test_expect_success '--no-verify with succeeding hook (merge)' ' - test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" && + test_when_finished "rm -f \"$PREMERGE\" actual_hooks" && ln -s "success.sample" "$PREMERGE" && - >expected_hooks && - >actual_hooks && git checkout side && git merge --no-verify -m "merge master" master && git checkout master && - test_cmp expected_hooks actual_hooks + test_path_is_missing actual_hooks ' test_expect_success 'with failing hook' ' @@ -137,14 +125,12 @@ test_expect_success 'with failing hook' ' ' test_expect_success '--no-verify with failing hook' ' - test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" && + test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" && ln -s "fail.sample" "$PRECOMMIT" && - >expected_hooks && -
Re: [PATCH v3 1/4] t7503: verify proper hook execution
On Fri, 2 Aug 2019 at 00:20, Josh Steadmon wrote: > > t7503 did not verify that the expected hooks actually ran during > testing. Fix that by making the hook scripts write their $0 into a file > so that we can compare actual execution vs. expected execution. It could be argued that this test is perfectly fine as long as there is just one hook to choose from, and as long as we're fine with failing tests being a bit non-obvious to debug. Since we'll soon have two hooks to choose from in this test, we really want to make sure that we're executing the right one, not just "at least one". (No need to reroll, this is just me thinking out loud.) Martin
[PATCH] sequencer: clarify intention to break out of loop
When we find a space, we set `len = i`, which gives us the answer we are
looking for, but which also breaks out of the loop through these steps:
1. `len = i`
2. `i = i + 1`
3. Is `i < len`? No, so break out.
Since `i` is signed, step 2 is undefined if `i` has the value `INT_MAX`.
It can't actually have that value, but that doesn't stop my copy of gcc
7.3.0 from throwing the following:
> sequencer.c:2853:3: error: assuming signed overflow does not occur when
> assuming that (X + c) < X is always false [-Werror=strict-overflow]
>for (i = 0; i < len; i++)
>^~~
That is, the compiler has realized that the code is essentially
evaluating "(len + 1) < len" and that for `len = INT_MAX`, this is
undefined behavior. What it hasn't figured out is that if `i` and `len`
are both `INT_MAX` after step 1, then `len` must have had a value larger
than `INT_MAX` before that step, which it can't have had.
Let's be explicit about breaking out of the loop. This helps the
compiler grok our intention. As a bonus, it might make it (even) more
obvious to human readers that the loop stops at the first space.
While at it, reduce the scope of `i`.
Signed-off-by: Martin Ågren
---
sequencer.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 0c164d5f98..a351638ad9 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2829,7 +2829,7 @@ static int do_reset(const char *name, int len, struct
replay_opts *opts)
struct tree_desc desc;
struct tree *tree;
struct unpack_trees_options unpack_tree_opts;
- int ret = 0, i;
+ int ret = 0;
if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
return -1;
@@ -2849,10 +2849,14 @@ static int do_reset(const char *name, int len, struct
replay_opts *opts)
}
oidcpy(&oid, &opts->squash_onto);
} else {
+ int i;
/* Determine the length of the label */
- for (i = 0; i < len; i++)
- if (isspace(name[i]))
+ for (i = 0; i < len; i++) {
+ if (isspace(name[i])) {
len = i;
+ break;
+ }
+ }
strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
if (get_oid(ref_name.buf, &oid) &&
--
2.19.1.593.gc670b1f876.dirty
Re: [PATCH] sequencer: clarify intention to break out of loop
On Sun, 28 Oct 2018 at 20:01, Eric Sunshine wrote:
>
> On Sun, Oct 28, 2018 at 11:32 AM Martin Ågren wrote:
> > [...]
> > Let's be explicit about breaking out of the loop. This helps the
> > compiler grok our intention. As a bonus, it might make it (even) more
> > obvious to human readers that the loop stops at the first space.
>
> This did come up in review[1,2]. I had a hard time understanding the
> loop because it looked idiomatic but wasn't (and we have preconceived
> notions about behavior of things which look idiomatic).
>
> [1]:
> https://public-inbox.org/git/CAPig+cQbG2s-LrAo9+7C7=dXifbWFJ3SzuNa-QePHDk7egK=j...@mail.gmail.com/
> [2]:
> https://public-inbox.org/git/capig+crju6nixpt2frdwz0x1hmgf1ojvzj3uk2qxege-s7i...@mail.gmail.com/
Hmm, I should have been able to dig those up myself. Thanks for the
pointers.
> > /* Determine the length of the label */
> > + for (i = 0; i < len; i++) {
> > + if (isspace(name[i])) {
> > len = i;
> > + break;
> > + }
> > + }
> > strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
>
> At least for me, this would be more idiomatic if it was coded like this:
>
> for (i = 0; i < len; i++)
> if (isspace(name[i]))
> break;
> strbuf_addf(..., i, name);
>
> or, perhaps (less concise):
>
> for (i = 0; i < len; i++)
> if (isspace(name[i]))
> break;
> len = i;
> strbuf_addf(..., len, name);
This second one is more true to the original in that it updates `len` to
the new, shorter length. Which actually seems to be needed -- towards
the very end of the function, `len` is used, so the first of these
suggestions would change the behavior.
Thanks a lot for a review. I'll wait for any additional comments and
will try a v2 with your second suggestion.
Martin
[PATCH v2] sequencer: break out of loop explicitly
It came up in review [1, 2] that this non-idiomatic loop is a bit tricky.
When we find a space, we set `len = i`, which gives us the answer we are
looking for, but which also breaks out of the loop.
It turns out that this loop can confuse compilers as well. My copy of
gcc 7.3.0 realizes that we are essentially evaluating `(len + 1) < len`
and warns that the behavior is undefined if `len` is `INT_MAX`. (Because
the assignment `len = i` is guaranteed to decrease `len`, such undefined
behavior is not actually possible.)
Rewrite the loop to a more idiomatic variant which doesn't muck with
`len` in the loop body. That should help compilers and human readers
figure out what is going on here. But do note that we need to update
`len` since it is not only used just after this loop (where we could
have used `i` directly), but also later in this function.
While at it, reduce the scope of `i`.
[1]
https://public-inbox.org/git/CAPig+cQbG2s-LrAo9+7C7=dXifbWFJ3SzuNa-QePHDk7egK=j...@mail.gmail.com/
[2]
https://public-inbox.org/git/capig+crju6nixpt2frdwz0x1hmgf1ojvzj3uk2qxege-s7i...@mail.gmail.com/
Helped-by: Eric Sunshine
Signed-off-by: Martin Ågren
---
Thanks for the comments on v1. Based on them, I decided to go for
Eric's option 2, and to make the log message less technical in favor of
"compilers and humans alike can get this wrong".
sequencer.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 0c164d5f98..e7aa4d5020 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2829,7 +2829,7 @@ static int do_reset(const char *name, int len, struct
replay_opts *opts)
struct tree_desc desc;
struct tree *tree;
struct unpack_trees_options unpack_tree_opts;
- int ret = 0, i;
+ int ret = 0;
if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
return -1;
@@ -2849,10 +2849,13 @@ static int do_reset(const char *name, int len, struct
replay_opts *opts)
}
oidcpy(&oid, &opts->squash_onto);
} else {
+ int i;
+
/* Determine the length of the label */
for (i = 0; i < len; i++)
if (isspace(name[i]))
- len = i;
+ break;
+ len = i;
strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
if (get_oid(ref_name.buf, &oid) &&
--
2.19.1.593.gc670b1f876.dirty
Re: [PATCH v2] sequencer: break out of loop explicitly
On Wed, 31 Oct 2018 at 18:28, Eric Sunshine wrote: > > On Wed, Oct 31, 2018 at 10:54 AM Johannes Schindelin > wrote: > > ACK. Thanks for cleaning up after me, > > Looks good to me, as well. Thanks for working on it. Thanks, both of you. Martin
Re: [PATCH v3 1/2] range-diff doc: add a section about output stability
On Wed, 7 Nov 2018 at 13:22, Ævar Arnfjörð Bjarmason wrote: > + > +This is particularly true when passing in diff options. Currently some > +options like `--stat` can as an emergent effect produce output that's > +quite useless in the context of `range-diff`. Future versions of > +`range-diff` may learn to interpret such options in a manner specifc s/specifc/specific/ > +to `range-diff` (e.g. for `--stat` summarizing how the diffstat > +changed).
Re: [PATCH] Makefile: add pending semantic patches
On Thu, 8 Nov 2018 at 21:53, Stefan Beller wrote: > > From: SZEDER Gábor > I haven't followed the original discussion too carefully, so I'll read this like someone new to the topic probably would. A nit, perhaps, but I was genuinely confused at first. The subject is "Makefile: add pending semantic patches", but this patch doesn't add any. It adds Makefile-support for such patches though, and it defines the entire concept of pending semantic patches. How about "coccicheck: introduce 'pending' semantic patches"? > Add a description and place on how to use coccinelle for large refactorings > that happen only once. A bit confused about "and place". Based on my understanding from reading the remainder of this patch, maybe: Teach `make coccicheck` to avoid patches named "*.pending.cocci" and handle them separately in a new `make coccicheck-pending` instead. This means that we can separate "critical" patches from "FYI" patches. The former target can continue causing Travis to fail its static analysis job, while the latter can let us keep an eye on ongoing (pending) transitions without them causing too much fallout. Document the intended use-cases and processes around these two targets. > This directory provides examples of Coccinelle (http://coccinelle.lip6.fr/) > semantic patches that might be useful to developers. > + > +There are two types of semantic patches: > + > + * Using the semantic transformation to check for bad patterns in the code; > + This is what the original target 'make coccicheck' is designed to do and Is it relevant that this was the "original" target? (Genuine question.) > + it is expected that any resulting patch indicates a regression. > + The patches resulting from 'make coccicheck' are small and infrequent, > + so once they are found, they can be sent to the mailing list as per usual. > + > + Example for introducing new patterns: > + 67947c34ae (convert "hashcmp() != 0" to "!hasheq()", 2018-08-28) > + b84c783882 (fsck: s/++i > 1/i++/, 2018-10-24) > + > + Example of fixes using this approach: > + 248f66ed8e (run-command: use strbuf_addstr() for adding a string to > + a strbuf, 2018-03-25) > + f919ffebed (Use MOVE_ARRAY, 2018-01-22) > + > + These types of semantic patches are usually part of testing, c.f. > + 0860a7641b (travis-ci: fail if Coccinelle static analysis found something > + to transform, 2018-07-23) Very nicely described, nice with the examples to quickly give a feeling about how/when to use this. > + * Using semantic transformations in large scale refactorings throughout > + the code base. > + > + When applying the semantic patch into a real patch, sending it to the > + mailing list in the usual way, such a patch would be expected to have a > + lot of textual and semantic conflicts as such large scale refactorings > + change function signatures that are used widely in the code base. > + A textual conflict would arise if surrounding code near any call of such > + function changes. A semantic conflict arises when other patch series in > + flight introduce calls to such functions. OK, I'm with you. > + So to aid these large scale refactorings, semantic patches can be used, > + using the process as follows: > + > + 1) Figure out what kind of large scale refactoring we need > + -> This is usually done by another unrelated series. "This"? The figuring out, or the refactoring? Also, "unrelated"? > + 2) Create the sematic patch needed for the large scale refactoring s/sematic/semantic/ > + and store it in contrib/coccinelle/*.pending.cocci > + -> The suffix containing 'pending' is important to differentiate > + this case from the other use case of checking for bad patterns. Good. > + 3) Apply the semantic patch only partially, where needed for the patch > series > + that motivates the large scale refactoring and then build that series > + on top of it. > + By applying the semantic patch only partially where needed, the series > + is less likely to conflict with other series in flight. > + To make it possible to apply the semantic patch partially, there needs > + to be mechanism for backwards compatibility to keep those places > working > + where the semantic patch is not applied. This can be done via a > + wrapper function that has the exact name and signature as the function > + to be changed. > + > + 4) Send the series as usual, including only the needed parts of the > + large scale refactoring Trailing period. OK, I think I get it, but I wonder if this might not work equally well or better under certain circumstances: - introduce new API - add pending semantic patch - convert quiet areas to use the new API On the other hand, listing all possible flows might be needlessly limiting. I guess it boils down to this: "Create a pending semantic patch. Make sure the old way of doing things sti
Re: [PATCH] coccicheck: introduce 'pending' semantic patches
On Sat, 10 Nov 2018 at 01:10, Stefan Beller wrote: > I dialed back on the workflow, as we may want to explore it first > before writing it down. Makes sense. FWIW, this iteration looks good to me. Martin
Re: [PATCH v2 1/1] bundle: cleanup lock files on error
On Wed, 14 Nov 2018 at 16:26, Gaël Lhez via GitGitGadget
wrote:
> However, the `.lock` file was still open and on Windows that means
> that it could not be deleted properly. This patch fixes that issue.
Hmmm, doesn't the tempfile machinery remove the lock file when we die?
> ref_count = write_bundle_refs(bundle_fd, &revs);
> - if (!ref_count)
> - die(_("Refusing to create empty bundle."));
> - else if (ref_count < 0)
> + if (ref_count <= 0) {
> + if (!ref_count)
> + error(_("Refusing to create empty bundle."));
> goto err;
> + }
One less `die()` in libgit -- nice.
> +test_expect_success 'try to create a bundle with empty ref count' '
> + test_expect_code 1 git bundle create foobar.bundle master..master
> +'
This tries to make sure that we don't just die, but that we exit in a
"controlled" way with exit code 1. After reading the log message, I had
perhaps expected something like
test_must_fail git bundle [...] &&
test_path_is_missing foobar.bundle.lock
That relies on magically knowing the ".lock" suffix, but my point is
that for me (on Linux), this alternative test passes both before and
after the patch. Before, because tempfile.c cleans up; after, because
bundle.c does so. Doesn't this happen on Windows too? What am I missing?
Martin
Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C
On Fri, 23 Nov 2018 at 11:13, Johannes Schindelin
wrote:
> On Mon, 30 Oct 2017, Pranit Bauva wrote:
>
> > On Fri, Oct 27, 2017 at 10:58 PM, Martin Ågren
> > wrote:
> > > On 27 October 2017 at 17:06, Pranit Bauva wrote:
> > >> +static void free_terms(struct bisect_terms *terms)
> > >> +{
> > >> + if (!terms->term_good)
> > >> + free((void *) terms->term_good);
> > >> + if (!terms->term_bad)
> > >> + free((void *) terms->term_bad);
> > >> +}
> > > You leave the pointers dangling, but you're ok for now since this is the
> > > last thing that happens in `cmd_bisect__helper()`. Your later patches
> > > add more users, but they're also ok, since they immediately assign new
> > > values.
> > >
> > > In case you (and others) find it useful, the below is a patch I've been
> > > sitting on for a while as part of a series to plug various memory-leaks.
> > > `FREE_AND_NULL_CONST()` would be useful in precisely these situations.
> >
> > Honestly, I wouldn't be the best person to judge this.
>
> Git's source code implicitly assumes that any `const` pointer refers to
> memory owned by another code path. It is therefore probably not a good
> idea to encourage `free()`ing a `const` pointer.
Yeah, I never submitted that patch as part of a real series. I remember
having a funky feeling about it, and whatever use-case I had for this
macro, I managed to solve the memory leak in some other way in a
rewrite.
Thanks for a sanity check.
Martin
Re: Broken alignment in git-reset docs
On Wed, 28 Nov 2018 at 12:42, Paweł Samoraj wrote:
>
> Hi!
> The git-reset documentation page section which is accessible via URL
> https://git-scm.com/docs/git-reset#_discussion is not looking good.
>
[snip]
>
> The web archive has got a snapshot from 2014-06-28 when it was ok
> (https://web.archive.org/web/20140628170155/http://git-scm.com/docs/git-reset).
Hi Paweł
Thanks for the report. The sources have not changed since 2010, so this
is most likely because something in the build has changed. It's probably
that git-scm.com has switched to using Asciidoctor (as opposed to
Asciidoc). The correct fix could be something like 379805051d
("Documentation: render revisions correctly under Asciidoctor",
2018-05-06).
Do you feel like attempting a patch against git.git? The hard part of
that is probably to get all the build dependencies in place, in
particular to be able to test with both Asciidoc and Asciidoctor. See
USE_ASCIIDOCTOR in Documentation/Makefile. If you're not up for it, no
problem, I should be able to get to this later today.
Thanks again for the report.
Martin
[PATCH 1/2] git-reset.txt: render tables correctly under Asciidoctor
Asciidoctor removes the indentation of each line in these tables, so the
last lines of each table have a completely broken alignment.
Similar to 379805051d ("Documentation: render revisions correctly under
Asciidoctor", 2018-05-06), use an explicit literal block to indicate
that we want to keep the leading whitespace in the tables.
Because this gives us some extra indentation, we can remove the one that
we have been carrying explicitly. That is, drop the first six spaces of
indentation on each line. With Asciidoc (8.6.10), this results in
identical rendering before and after this commit, both for git-reset.1
and git-reset.html.
Reported-by: Paweł Samoraj
Signed-off-by: Martin Ågren
---
Documentation/git-reset.txt | 140
1 file changed, 78 insertions(+), 62 deletions(-)
diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 2dac95c71a..7c925e3a29 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -365,53 +365,65 @@ index in state B. It resets (i.e. moves) the HEAD (i.e.
the tip of
the current branch, if you are on one) to "target" (which has the file
in state D).
- working index HEAD target working index HEAD
-
- A B CD --soft A B D
- --mixed A D D
- --hard D D D
- --merge (disallowed)
- --keep (disallowed)
-
- working index HEAD target working index HEAD
-
- A B CC --soft A B C
- --mixed A C C
- --hard C C C
- --merge (disallowed)
- --keep A C C
-
- working index HEAD target working index HEAD
-
- B B CD --soft B B D
- --mixed B D D
- --hard D D D
- --merge D D D
- --keep (disallowed)
-
- working index HEAD target working index HEAD
-
- B B CC --soft B B C
- --mixed B C C
- --hard C C C
- --merge C C C
- --keep B C C
-
- working index HEAD target working index HEAD
-
- B C CD --soft B C D
- --mixed B D D
- --hard D D D
- --merge (disallowed)
- --keep (disallowed)
-
- working index HEAD target working index HEAD
-
- B C CC --soft B C C
- --mixed B C C
- --hard C C C
- --merge B C C
- --keep B C C
+
+working index HEAD target working index HEAD
+
+ A B CD --soft A B D
+ --mixed A D D
+ --hard D D D
+ --merge (disallowed)
+ --keep (disallowed)
+
+
+
+working index HEAD target working index HEAD
+
+ A B CC --soft A B C
+ --mixed A C C
+ --hard C C C
+ --merge (disallowed)
+ --keep A C C
+
+
+
+working index HEAD target working index HEAD
+
+ B B CD --soft B B D
+ --mixed B D D
+ --hard D D D
+ --merge D D D
+ --keep (disallowed)
+
+
+
+working index HEAD target working index HEAD
+
+ B B CC --soft B B C
+ --mixed B C C
+ --ha
[PATCH 0/2] Re: Broken alignment in git-reset docs
On Wed, 28 Nov 2018 at 13:02, Martin Ågren wrote:
>
> On Wed, 28 Nov 2018 at 12:42, Paweł Samoraj wrote:
> >
> > The git-reset documentation page section which is accessible via URL
> > https://git-scm.com/docs/git-reset#_discussion is not looking good.
>
> [...] The correct fix could be something like 379805051d
> ("Documentation: render revisions correctly under Asciidoctor",
> 2018-05-06).
Turns out it probably is, so here's a proposed fix, followed by a patch
to typeset more of this document in monospace. That should also make
things prettier, but not in such a dramatic way as the first patch.
This is obviously not 2.20-material. About where to queue this, I had
expected this to depend on 743e63f3ed ("Documentation: use 8-space tabs
with Asciidoctor", 2018-05-06) just like 379805051d does for proper
rendering, but from my testing, somehow it doesn't.
Paweł, I'm hoping this fix should be in v2.21 in a few months and then
eventually trickle down to git-scm. Thanks again for reporting this.
Martin Ågren (2):
git-reset.txt: render tables correctly under Asciidoctor
git-reset.txt: render literal examples as monospace
Documentation/git-reset.txt | 277 +++-
1 file changed, 147 insertions(+), 130 deletions(-)
--
2.20.0.rc1.8.g46351a2c6f
[PATCH 2/2] git-reset.txt: render literal examples as monospace
Large parts of this document do not use `backticks` around literal examples such as branch names (`topic/wip`), git usages, `HEAD` and `` so they render as ordinary text. Fix that. Signed-off-by: Martin Ågren --- Documentation/git-reset.txt | 131 ++-- 1 file changed, 66 insertions(+), 65 deletions(-) diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt index 7c925e3a29..9f69ae8b69 100644 --- a/Documentation/git-reset.txt +++ b/Documentation/git-reset.txt @@ -14,14 +14,14 @@ SYNOPSIS DESCRIPTION --- -In the first and second form, copy entries from to the index. -In the third form, set the current branch head (HEAD) to , optionally -modifying index and working tree to match. The / defaults -to HEAD in all forms. +In the first and second form, copy entries from `` to the index. +In the third form, set the current branch head (`HEAD`) to ``, +optionally modifying index and working tree to match. +The ``/`` defaults to `HEAD` in all forms. 'git reset' [-q] [] [--] ...:: - This form resets the index entries for all to their - state at . (It does not affect the working tree or + This form resets the index entries for all `` to their + state at ``. (It does not affect the working tree or the current branch.) + This means that `git reset ` is the opposite of `git add @@ -36,7 +36,7 @@ working tree in one go. 'git reset' (--patch | -p) [] [--] [...]:: Interactively select hunks in the difference between the index - and (defaults to HEAD). The chosen hunks are applied + and `` (defaults to `HEAD`). The chosen hunks are applied in reverse to the index. + This means that `git reset -p` is the opposite of `git add -p`, i.e. @@ -44,16 +44,16 @@ you can use it to selectively reset hunks. See the ``Interactive Mode'' section of linkgit:git-add[1] to learn how to operate the `--patch` mode. 'git reset' [] []:: - This form resets the current branch head to and - possibly updates the index (resetting it to the tree of ) and - the working tree depending on . If is omitted, - defaults to "--mixed". The must be one of the following: + This form resets the current branch head to `` and + possibly updates the index (resetting it to the tree of ``) and + the working tree depending on ``. If `` is omitted, + defaults to `--mixed`. The `` must be one of the following: + -- --soft:: Does not touch the index file or the working tree at all (but - resets the head to , just like all modes do). This leaves - all your changed files "Changes to be committed", as 'git status' + resets the head to ``, just like all modes do). This leaves + all your changed files "Changes to be committed", as `git status` would put it. --mixed:: @@ -66,24 +66,24 @@ linkgit:git-add[1]). --hard:: Resets the index and working tree. Any changes to tracked files in the - working tree since are discarded. + working tree since `` are discarded. --merge:: Resets the index and updates the files in the working tree that are - different between and HEAD, but keeps those which are + different between `` and `HEAD`, but keeps those which are different between the index and working tree (i.e. which have changes which have not been added). - If a file that is different between and the index has unstaged - changes, reset is aborted. + If a file that is different between `` and the index has + unstaged changes, reset is aborted. + -In other words, --merge does something like a 'git read-tree -u -m ', +In other words, `--merge` does something like a `git read-tree -u -m `, but carries forward unmerged index entries. --keep:: Resets index entries and updates files in the working tree that are - different between and HEAD. - If a file that is different between and HEAD has local changes, - reset is aborted. + different between `` and `HEAD`. + If a file that is different between `` and `HEAD` has local + changes, reset is aborted. -- If you want to undo a commit other than the latest on a branch, @@ -116,15 +116,15 @@ $ git pull git://info.example.com/ nitfol <4> + <1> You are happily working on something, and find the changes in these files are in good order. You do not want to see them -when you run "git diff", because you plan to work on other files +when you run `git diff`, because you plan to work on other files and changes with these files are distracting. <2> Somebody asks you to pull, and the changes sound worthy of merging. <3> However, you already dirtied the index (i.e. your index does -not match the HEAD commit). But you know the pull you are going -
Re: [PATCH 1/2] git-reset.txt: render tables correctly under Asciidoctor
On Wed, 28 Nov 2018 at 20:45, Ævar Arnfjörð Bjarmason wrote: > On Wed, Nov 28 2018, Martin Ågren wrote: > > > Asciidoctor removes the indentation of each line in these tables, so the > > last lines of each table have a completely broken alignment. > > Earlier I was trying to get the Documentation/doc-diff script to diff > the asciidoc and asciidoctor docs without much success (hadn't used it > before, just hacking the Makefile to turn on asciidoctor yielded syntax > errors or something). > > Is something like that a thing we could make into a regression test? Interesting idea. I just tried a gross hack: * Use `make --always-make ... install-man` in doc-diff. * ./doc-diff -f HEAD HEAD # note -f * Add empty commit and tweak config.mak * ./doc-diff HEAD^ HEAD # note no -f There are lots of irrelevant differences in the headers and footers, which is a bit unfortunate. Also, lots of annoying differences originating in Asciidoctor's inclination to render a space after linkgit:foo . There are those differences themselves, obviously, but also follow-on differences such as entire paragraphs that wrap differently. I could spot a few things that should be fixable on our side, but on a quick skimming, I didn't spot too many "huge" differences, which feels good. The one which this patch fixes, obviously, and there's some work to do in git-status.txt and git-column.txt (at least). Tacking on `--stat` to the call to `git diff --no-index` singles out git-config.txt, but it seems like lots of small or maybe even irrelevant differences, plus lots of spaces around linkgit: , as already mentioned. Martin
Re: [PATCH] format-patch: do not let its diff-options affect --range-diff (was Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options)
On Fri, 30 Nov 2018 at 10:32, Eric Sunshine wrote:
>
> On Thu, Nov 29, 2018 at 11:27 PM Junio C Hamano wrote:
> > Junio C Hamano writes:
> > So how about doing this on top of 'master' instead? As this leaks
> > *no* information wrt how range-diff machinery should behave from the
> > format-patch side by not passing any diffopt, as long as the new
> > code I added to show_range_diff() comes up with a reasonable default
> > diffopts (for which I really would appreciate extra sets of eyes to
> > make sure), this change by definition cannot be wrong (famous last
> > words).
> Another benefit of calling show_range_diff() directly is that when
> "git format-patch --stdout --range-diff=..." is sent to the terminal,
> the range-diff gets colored output for free. I'm pleased to see that
> that still works after this change.
(If the patch below makes any sense to you and you know more about this
diff/color thing, the fourth bullet in the log message below might
interest you.)
> > diff --git a/range-diff.h b/range-diff.h
> > @@ -5,6 +5,11 @@
> > +/*
> > + * Compare series of commmits in RANGE1 and RANGE2, and emit to the
> > + * standard output. NULL can be passed to DIFFOPT to use the built-in
> > + * default.
> > + */
>
> It is more correct to say that the range-diff is emitted to
> diffopt->file (which may be stdout).
This seems to be an important remark. There's a pretty bad regression
here since when `diffopt` is NULL, we've lost our original, intended
`diffopt->file` and the range-diff ends up on stdout.
Here's my whitespace-damaged WIP. I would be able to pick this up again
in about 6h, but anyone is more than welcome to pick this up and run
with it in the meantime.
This is not a corner of the code that I'm particularly familiar with...
-->8--
Subject: [PATCH] range-diff: always pass at least minimal diff options
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Commit d8981c3f88 ("format-patch: do not let its diff-options affect
--range-diff", 2018-11-30) taught `show_range_diff()` to accept a
NULL-pointer as an indication that it should use its own "reasonable
default". That fixed a regression from a5170794 ("Merge branch
'ab/range-diff-no-patch'", 2018-11-18), but unfortunately it introduced
a regression of its own.
In particular, it means we forget the `file` member of the diff options,
so rather than placing a range-diff in the cover-letter, we write it to
stdout. In order to fix this, rewrite the two callers adjusted by
d8981c3f88 to create a "dummy" set of diff options where they only fill
in which file to use.
A couple of remarks about this commit:
* No tests. The change in builtin/log.c has been tested manually, the
one in log-tree.c not at all, other than by running existing tests.
* I have not convinced myself 100% that there aren't other things that
are just as important as `file` to pass down.
* `show_range_diff()` can still take NULL, although that is now dead
code. If something like this here commit is deemed the proper fix
for this, that code path could also go, either as part of this
commit, or separately, once we've cut 2.20.
* The range-diff is written colored regardless of destination, i.e.,
when written to a file, it contains garbage.
Signed-off-by: Martin Ågren
---
builtin/log.c | 12 +++-
log-tree.c| 12 +++-
2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index 5ac18e2848..0609e41ae5 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1094,9 +1094,19 @@ static void make_cover_letter(struct rev_info
*rev, int use_stdout,
}
if (rev->rdiff1) {
+/**
+ * (At least for now) we only want to pass down
+ * the file handle where we want the range-diff
+ * to appear. Avoid any other diff options until
+ * we know how we want to handle them.
+ */
+struct diff_options opts;
+diff_setup(&opts);
+opts.file = rev->diffopt.file;
+diff_setup_done(&opts);
fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
show_range_diff(rev->rdiff1, rev->rdiff2,
-rev->creation_factor, 1, NULL);
+rev->creation_factor, 1, &opts);
}
}
diff --git a/log-tree.c b/log-tree.c
index b243779a0b..bc355a4e91 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -755,14 +755,24 @@ void show_log(struct rev_info *opt)
if (cmit_fmt_is_mail(ctx.fmt) && opt->rdiff1) {
struct diff_queue_struct dq;
+struct diff_options opts;
memcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));
DIFF_QUEUE_CLEAR(&diff_
[PATCH v2] range-diff: always pass at least minimal diff options
Commit d8981c3f88 ("format-patch: do not let its diff-options affect
--range-diff", 2018-11-30) taught `show_range_diff()` to accept a
NULL-pointer as an indication that it should use its own "reasonable
default". That fixed a regression from a5170794 ("Merge branch
'ab/range-diff-no-patch'", 2018-11-18), but unfortunately it introduced
a regression of its own.
In particular, it means we forget the `file` member of the diff options,
so rather than placing a range-diff in the cover-letter, we write it to
stdout. In order to fix this, rewrite the two callers adjusted by
d8981c3f88 to instead create a "dummy" set of diff options where they
only fill in which file to use.
Plus, turn off coloring to make sure we don't write any color codes.
Maybe we could do `opts.use_color = opts.file != stdout`, but for now,
I'd much rather always write uncolored output than write color codes
where there shouldn't be any.
Modify and extend the existing tests to try and verify that the right
contents end up in the right place.
Don't revert `show_range_diff()`, i.e., let it keep accepting NULL.
Rather than removing what is dead code and figuring out it isn't
actually dead and we've broken 2.20, just leave it for now.
Signed-off-by: Martin Ågren
---
Here's another attempt at fixing this recent regression.
t/t3206-range-diff.sh | 20 +---
builtin/log.c | 13 -
log-tree.c| 13 -
3 files changed, 37 insertions(+), 9 deletions(-)
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index e497c1358f..048feaf6dd 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -248,18 +248,24 @@ test_expect_success 'dual-coloring' '
for prev in topic master..topic
do
test_expect_success "format-patch --range-diff=$prev" '
- git format-patch --stdout --cover-letter --range-diff=$prev \
+ git format-patch --cover-letter --range-diff=$prev \
master..unmodified >actual &&
- grep "= 1: .* s/5/A" actual &&
- grep "= 2: .* s/4/A" actual &&
- grep "= 3: .* s/11/B" actual &&
- grep "= 4: .* s/12/B" actual
+ test_when_finished "rm 000?-*" &&
+ test_line_count = 5 actual &&
+ test_i18ngrep "^Range-diff:$" -* &&
+ grep "= 1: .* s/5/A" -* &&
+ grep "= 2: .* s/4/A" -* &&
+ grep "= 3: .* s/11/B" -* &&
+ grep "= 4: .* s/12/B" -*
'
done
test_expect_success 'format-patch --range-diff as commentary' '
- git format-patch --stdout --range-diff=HEAD~1 HEAD~1 >actual &&
- test_i18ngrep "^Range-diff:$" actual
+ git format-patch --range-diff=HEAD~1 HEAD~1 >actual &&
+ test_when_finished "rm 0001-*" &&
+ test_line_count = 1 actual &&
+ test_i18ngrep "^Range-diff:$" 0001-* &&
+ grep "> 1: .* new message" 0001-*
'
test_done
diff --git a/builtin/log.c b/builtin/log.c
index 5ac18e2848..e42487b46d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1094,9 +1094,20 @@ static void make_cover_letter(struct rev_info *rev, int
use_stdout,
}
if (rev->rdiff1) {
+ /*
+* (At least for now) we only want to pass down
+* the file handle where we want the range-diff
+* to appear. Avoid any other diff options until
+* we know how we want to handle them.
+*/
+ struct diff_options opts;
+ diff_setup(&opts);
+ opts.file = rev->diffopt.file;
+ opts.use_color = 0;
+ diff_setup_done(&opts);
fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
show_range_diff(rev->rdiff1, rev->rdiff2,
- rev->creation_factor, 1, NULL);
+ rev->creation_factor, 1, &opts);
}
}
diff --git a/log-tree.c b/log-tree.c
index b243779a0b..fd79a3ec37 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -755,14 +755,25 @@ void show_log(struct rev_info *opt)
if (cmit_fmt_is_mail(ctx.fmt) && opt->rdiff1) {
struct diff_queue_struct dq;
+ struct diff_options opts;
memcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));
DIFF_QUEUE_CLEAR(&diff_queued_diff);
next_commentary_block(opt, NULL);
fprintf_ln(opt-&
[PATCH 1/3] RelNotes 2.20: move some items between sections
Some items that should be in "Performance, Internal Implementation, Development Support etc." have ended up in "UI, Workflows & Features" and "Fixes since v2.19". Move them, and do s/uses/use/ while at it. Signed-off-by: Martin Ågren --- Documentation/RelNotes/2.20.0.txt | 26 +- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/Documentation/RelNotes/2.20.0.txt b/Documentation/RelNotes/2.20.0.txt index b1deaf37da..e5ab8cc609 100644 --- a/Documentation/RelNotes/2.20.0.txt +++ b/Documentation/RelNotes/2.20.0.txt @@ -137,11 +137,6 @@ UI, Workflows & Features command line, or setting sendemail.suppresscc configuration variable to "misc-by", can be used to disable this behaviour. - * Developer builds now uses -Wunused-function compilation option. - - * One of our CI tests to run with "unusual/experimental/random" - settings now also uses commit-graph and midx. - * "git mergetool" learned to take the "--[no-]gui" option, just like "git difftool" does. @@ -185,6 +180,11 @@ UI, Workflows & Features Performance, Internal Implementation, Development Support etc. + * Developer builds now use -Wunused-function compilation option. + + * One of our CI tests to run with "unusual/experimental/random" + settings now also uses commit-graph and midx. + * When there are too many packfiles in a repository (which is not recommended), looking up an object in these would require consulting many pack .idx files; a new mechanism to have a single @@ -387,6 +387,14 @@ Performance, Internal Implementation, Development Support etc. two classes to ease code migration process has been proposed and its support has been added to the Makefile. + * The "container" mode of TravisCI is going away. Our .travis.yml + file is getting prepared for the transition. + (merge 32ee384be8 ss/travis-ci-force-vm-mode later to maint). + + * Our test scripts can now take the '-V' option as a synonym for the + '--verbose-log' option. + (merge a5f52c6dab sg/test-verbose-log later to maint). + Fixes since v2.19 - @@ -544,14 +552,6 @@ Fixes since v2.19 didn't make much sense. This has been corrected. (merge 669b1d2aae md/exclude-promisor-objects-fix later to maint). - * The "container" mode of TravisCI is going away. Our .travis.yml - file is getting prepared for the transition. - (merge 32ee384be8 ss/travis-ci-force-vm-mode later to maint). - - * Our test scripts can now take the '-V' option as a synonym for the - '--verbose-log' option. - (merge a5f52c6dab sg/test-verbose-log later to maint). - * A regression in Git 2.12 era made "git fsck" fall into an infinite loop while processing truncated loose objects. (merge 18ad13e5b2 jk/detect-truncated-zlib-input later to maint). -- 2.20.0.rc2.1.gfcc5f94f1e
[PATCH 2/3] RelNotes 2.20: clarify sentence
I had to read this sentence a few times to understand it. Let's try to clarify it. Signed-off-by: Martin Ågren --- Documentation/RelNotes/2.20.0.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/RelNotes/2.20.0.txt b/Documentation/RelNotes/2.20.0.txt index e5ab8cc609..201135d80c 100644 --- a/Documentation/RelNotes/2.20.0.txt +++ b/Documentation/RelNotes/2.20.0.txt @@ -305,7 +305,7 @@ Performance, Internal Implementation, Development Support etc. * The overly large Documentation/config.txt file have been split into million little pieces. This potentially allows each individual piece - included into the manual page of the command it affects more easily. + to be included into the manual page of the command it affects more easily. * Replace three string-list instances used as look-up tables in "git fetch" with hashmaps. -- 2.20.0.rc2.1.gfcc5f94f1e
[PATCH 0/3] Re: [ANNOUNCE] Git v2.20.0-rc2
Hi Junio, > A release candidate Git v2.20.0-rc2 is now available for testing > at the usual places. It is comprised of 934 non-merge commits > since v2.19.0, contributed by 76 people, 25 of which are new faces. Here are a few suggested tweaks after reading the draft release notes. Nothing critical. Martin Martin Ågren (3): RelNotes 2.20: move some items between sections RelNotes 2.20: clarify sentence RelNotes 2.20: drop spurious double quote Documentation/RelNotes/2.20.0.txt | 30 +++--- 1 file changed, 15 insertions(+), 15 deletions(-) -- 2.20.0.rc2.1.gfcc5f94f1e
[PATCH 3/3] RelNotes 2.20: drop spurious double quote
We have three double-quote characters, which is one too many or too few. Dropping the last one seems to match the original intention best. Signed-off-by: Martin Ågren --- Documentation/RelNotes/2.20.0.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/RelNotes/2.20.0.txt b/Documentation/RelNotes/2.20.0.txt index 201135d80c..e71fe3dee1 100644 --- a/Documentation/RelNotes/2.20.0.txt +++ b/Documentation/RelNotes/2.20.0.txt @@ -578,7 +578,7 @@ Fixes since v2.19 * "git rev-parse --exclude=* --branches --branches" (i.e. first saying "add only things that do not match '*' out of all branches" - and then adding all branches, without any exclusion this time") + and then adding all branches, without any exclusion this time) worked as expected, but "--exclude=* --all --all" did not work the same way, which has been fixed. (merge 5221048092 ag/rev-parse-all-exclude-fix later to maint). -- 2.20.0.rc2.1.gfcc5f94f1e
