Re: enhancement: support for author.email and author.name in "git config"

[Martin Ågren]

Hi William,

On Thu, 6 Dec 2018 at 19:18, William Hubbs  wrote:
> We are in a situation where we would like to use author information that is
> different from committer information when we commit to certain
> repositories.

[...]

> [...] I would like to propose the addition of author.email and
> author.name settings to the git-config system.
>
> Additionally you could add committer.name and committer.email, but the
> only reason I bring the committer variations up is consistency since I
> see you also have GIT_COMMITTER_NAME and GIT_COMMITTER_EMAIL environment
> variables.

This idea was floated a couple of months ago [1]. Junio seemed to find
the request sensible and outlined a design. No patches materialized, as
far as I know, but that could be because Eric suggested a tool called
direnv. Maybe that would work for you.

[1] 
https://public-inbox.org/git/0f66ad7a-2289-2cce-6533-a27e19945...@rasmusvillemoes.dk/

Martin

[PATCH v2 2/3] RelNotes 2.20: clarify sentence

[Martin Ågren]

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 f4e79c4cfb..1a5bbd2e91 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.gc81af441bb

[PATCH v2 3/3] RelNotes 2.20: drop spurious double quote

[Martin Ågren]

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 1a5bbd2e91..659474f7c3 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.gc81af441bb

[PATCH v2 1/3] RelNotes 2.20: move some items between sections

[Martin Ågren]

Some items that should be in "Performance, Internal Implementation,
Development Support etc." have ended up in "UI, Workflows & Features".
Move them, and do s/uses/use/ while at it.

Signed-off-by: Martin Ågren 
---
 Documentation/RelNotes/2.20.0.txt | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/RelNotes/2.20.0.txt 
b/Documentation/RelNotes/2.20.0.txt
index b1deaf37da..f4e79c4cfb 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
-- 
2.20.0.rc2.1.gc81af441bb

Re: [PATCH 1/3] RelNotes 2.20: move some items between sections

[Martin Ågren]

On Tue, 4 Dec 2018 at 03:23, Junio C Hamano  wrote:
>
> Martin Ågren  writes:
>
> > 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.
>
> I agree with the early half of this change; I think it is OK to
> consider lack of preparation for Travis transition and lack of
> better testing in the maintenance track as bugs, though.

Sure. Here's a resend where patch 1/3 has been simplified accordingly.

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 | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

Range-diff against v1:
1:  d69f63b5f6 ! 1:  961bfc2ad6 RelNotes 2.20: move some items between sections
@@ -3,8 +3,8 @@
 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.
+Development Support etc." have ended up in "UI, Workflows & Features".
+Move them, and do s/uses/use/ while at it.
 
 Signed-off-by: Martin Ågren 
 
@@ -35,33 +35,3 @@
   * 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
-@@
-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
- -
-@@
-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:  eccb7edd08 = 2:  3027a34938 RelNotes 2.20: clarify sentence
3:  78f3043b65 = 3:  a5e2df91b4 RelNotes 2.20: drop spurious double quote
-- 
2.20.0.rc2.1.gc81af441bb

Re: [PATCH v3] range-diff: always pass at least minimal diff options

[Martin Ågren]

On Mon, 3 Dec 2018 at 22:21, Eric Sunshine  wrote:
> [es: retain diff coloring when going to stdout]
>
> Signed-off-by: Martin Ågren 
> Signed-off-by: Eric Sunshine 
> ---
>
> This is a re-roll of Martin's v2[1]. The only difference from v2 is that
> it retains coloring when emitting to the terminal (plus an in-code
> comment was simplified).

Thank you so much for this.

> if (rev->rdiff1) {
> +   /*
> +* Pass minimum required diff-options to range-diff; others
> +* can be added later if deemed desirable.
> +*/

Agreed.

> +   struct diff_options opts;
> +   diff_setup();
> +   opts.file = rev->diffopt.file;
> +   opts.use_color = rev->diffopt.use_color;

Ah, s/0/rev->diffopt.use_color/, well that's obvious.

Thanks!

Martin

Re: [PATCH] rebase docs: fix incorrect format of the section Behavioral Differences

[Martin Ågren]

On Mon, 3 Dec 2018 at 18:35, Johannes Sixt  wrote:
> I actually did not test the result, because I don't have the
> infrastructure.

I've tested with asciidoc and Asciidoctor, html and man-page. Looks
good.

Martin

[PATCH 1/3] RelNotes 2.20: move some items between sections

[Martin Ågren]

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

[Martin Ågren]

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

[Martin Ågren]

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

[Martin Ågren]

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

[PATCH v2] range-diff: always pass at least minimal diff options

[Martin Ågren]

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.file = rev->diffopt.file;
+   opts.use_color = 0;
+   diff_setup_done();
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, );
}
 }
 
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(, _queued_diff, sizeof(diff_queued_diff));
DIFF_QUEUE_CLEAR(_queued_diff);
 
next_commentary_block(opt, NULL);
fprintf_ln(opt->diffopt.file, "%s", opt->rdiff_title);
+   /*
+* (At least for now) we only want to pass down
+   

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)

[Martin Ågren]

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.file = rev->diffopt.file;
+diff_setup_done();
 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, );
 }
 }

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(, _queued_diff, sizeof(diff_queued_diff));
 DIFF_QUEUE_CLEAR(_queued_diff);

 next_commentary_block(opt, NULL);
 fprintf_ln(opt->diffopt.file, "%s&

Re: [PATCH 1/2] git-reset.txt: render tables correctly under Asciidoctor

[Martin Ågren]

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

[PATCH 1/2] git-reset.txt: render tables correctly under Asciidoctor

[Martin Ågren]

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

[Martin Ågren]

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

[Martin Ågren]

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
-to make does not affect frotz.c or filfre.c, so you reve

Re: Broken alignment in git-reset docs

[Martin Ågren]

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

Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C

[Martin Ågren]

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: [PATCH v2 1/1] bundle: cleanup lock files on error

[Martin Ågren]

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, );
> -   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] coccicheck: introduce 'pending' semantic patches

[Martin Ågren]

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] Makefile: add pending semantic patches

[Martin Ågren]

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

Re: [PATCH v3 1/2] range-diff doc: add a section about output stability

[Martin Ågren]

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 v2] sequencer: break out of loop explicitly

[Martin Ågren]

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

[PATCH v2] sequencer: break out of loop explicitly

[Martin Ågren]

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_REPORT_ON_ERROR) < 0)
return -1;
@@ -2849,10 +2849,13 @@ static int do_reset(const char *name, int len, struct 
replay_opts *opts)
}
oidcpy(, >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(_name, "refs/rewritten/%.*s", len, name);
if (get_oid(ref_name.buf, ) &&
-- 
2.19.1.593.gc670b1f876.dirty

Re: [PATCH] sequencer: clarify intention to break out of loop

[Martin Ågren]

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(_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] sequencer: clarify intention to break out of loop

[Martin Ågren]

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_REPORT_ON_ERROR) < 0)
return -1;
@@ -2849,10 +2849,14 @@ static int do_reset(const char *name, int len, struct 
replay_opts *opts)
}
oidcpy(, >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(_name, "refs/rewritten/%.*s", len, name);
if (get_oid(ref_name.buf, ) &&
-- 
2.19.1.593.gc670b1f876.dirty

[PATCH v2 2/3] builtin/commit-graph.c: UNLEAK variables

[Martin Ågren via GitGitGadget]

From: =?UTF-8?q?Martin=20=C3=85gren?= 

`graph_verify()`, `graph_read()` and `graph_write()` do the hard work of
`cmd_commit_graph()`. As soon as these return, so does
`cmd_commit_graph()`.

`strbuf_getline()` may allocate memory in the strbuf, yet return EOF.
We need to release the strbuf or UNLEAK it. Go for the latter since we
are close to returning from `graph_write()`.

`graph_write()` also fails to free the strings in the string list. They
have been added to the list with `strdup_strings` set to 0. We could
flip `strdup_strings` before clearing the list, which is our usual hack
in situations like this. But since we are about to exit, let's just
UNLEAK the whole string list instead.

UNLEAK `graph` in `graph_verify`. While at it, and for consistency,
UNLEAK in `graph_read()` as well, and remove an unnecessary UNLEAK just
before dying.

Signed-off-by: Martin Ågren 
Signed-off-by: Derrick Stolee 
---
 builtin/commit-graph.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index bc0fa9ba52..66f12eb009 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -64,6 +64,7 @@ static int graph_verify(int argc, const char **argv)
if (!graph)
return 0;
 
+   UNLEAK(graph);
return verify_commit_graph(the_repository, graph);
 }
 
@@ -89,10 +90,8 @@ static int graph_read(int argc, const char **argv)
graph_name = get_commit_graph_filename(opts.obj_dir);
graph = load_commit_graph_one(graph_name);
 
-   if (!graph) {
-   UNLEAK(graph_name);
+   if (!graph)
die("graph file %s does not exist", graph_name);
-   }
 
FREE_AND_NULL(graph_name);
 
@@ -115,7 +114,7 @@ static int graph_read(int argc, const char **argv)
printf(" large_edges");
printf("\n");
 
-   free_commit_graph(graph);
+   UNLEAK(graph);
 
return 0;
 }
@@ -166,6 +165,8 @@ static int graph_write(int argc, const char **argv)
pack_indexes = 
if (opts.stdin_commits)
commit_hex = 
+
+   UNLEAK(buf);
}
 
write_commit_graph(opts.obj_dir,
@@ -174,7 +175,7 @@ static int graph_write(int argc, const char **argv)
   opts.append,
   1);
 
-   string_list_clear(, 0);
+   UNLEAK(lines);
return 0;
 }
 
-- 
gitgitgadget

Re: [PATCH 0/2] commit-graph: more leak fixes

[Martin Ågren]

On Wed, 3 Oct 2018 at 18:19, Derrick Stolee  wrote:
> I'm fine with squashing it in with both our sign-offs. It is the same
> idea, it just requires a different set of arguments to hit it. I'll
> adjust the commit message as necessary.

> I'll add your PATCH 2/2 to my v2. Thanks!

Cool, thanks a lot.

Martin

[PATCH 0/2] commit-graph: more leak fixes

[Martin Ågren]

Hi Derrick,

These two patches on top of yours make the test suite (i.e., the subset
of it that I run) leak-free with respect to builtin/commit-graph.c and
commit-graph.c.

The first could be squashed into your patch 1/2. It touches the same
function, but it requires a different usage to trigger, so squashing it
in would require broadening the scope. I understand if you don't want to
do that.

If you want to pick these up as part of your re-roll in any way, shape
or form, go ahead. If not, they can go in separately, either in parallel
or after your series lands. Whatever the destiny of this posting, I'll
follow through as appropriate.

Martin

Martin Ågren (2):
  commit-graph: free `struct packed_git` after closing it
  builtin/commit-graph.c: UNLEAK variables

 builtin/commit-graph.c | 11 ++-
 commit-graph.c |  1 +
 2 files changed, 7 insertions(+), 5 deletions(-)

-- 
2.19.0.329.g76f2f5c1e3

[PATCH 2/2] builtin/commit-graph.c: UNLEAK variables

[Martin Ågren]

`graph_verify()`, `graph_read()` and `graph_write()` do the hard work of
`cmd_commit_graph()`. As soon as these return, so does
`cmd_commit_graph()`.

`strbuf_getline()` may allocate memory in the strbuf, yet return EOF.
We need to release the strbuf or UNLEAK it. Go for the latter since we
are close to returning from `graph_write()`.

`graph_write()` also fails to free the strings in the string list. They
have been added to the list with `strdup_strings` set to 0. We could
flip `strdup_strings` before clearing the list, which is our usual hack
in situations like this. But since we are about to exit, let's just
UNLEAK the whole string list instead.

UNLEAK `graph` in `graph_verify`. While at it, and for consistency,
UNLEAK in `graph_read()` as well, and remove an unnecessary UNLEAK just
before dying.

Signed-off-by: Martin Ågren 
---
 builtin/commit-graph.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index bc0fa9ba52..66f12eb009 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -64,6 +64,7 @@ static int graph_verify(int argc, const char **argv)
if (!graph)
return 0;
 
+   UNLEAK(graph);
return verify_commit_graph(the_repository, graph);
 }
 
@@ -89,10 +90,8 @@ static int graph_read(int argc, const char **argv)
graph_name = get_commit_graph_filename(opts.obj_dir);
graph = load_commit_graph_one(graph_name);
 
-   if (!graph) {
-   UNLEAK(graph_name);
+   if (!graph)
die("graph file %s does not exist", graph_name);
-   }
 
FREE_AND_NULL(graph_name);
 
@@ -115,7 +114,7 @@ static int graph_read(int argc, const char **argv)
printf(" large_edges");
printf("\n");
 
-   free_commit_graph(graph);
+   UNLEAK(graph);
 
return 0;
 }
@@ -166,6 +165,8 @@ static int graph_write(int argc, const char **argv)
pack_indexes = 
if (opts.stdin_commits)
commit_hex = 
+
+   UNLEAK(buf);
}
 
write_commit_graph(opts.obj_dir,
@@ -174,7 +175,7 @@ static int graph_write(int argc, const char **argv)
   opts.append,
   1);
 
-   string_list_clear(, 0);
+   UNLEAK(lines);
return 0;
 }
 
-- 
2.19.0.329.g76f2f5c1e3

[PATCH 1/2] commit-graph: free `struct packed_git` after closing it

[Martin Ågren]

`close_pack(p)` does not free the memory which `p` points to, so follow
up with a call to `free(p)`. All other users of `close_pack()` look ok.

Signed-off-by: Martin Ågren 
---
 commit-graph.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/commit-graph.c b/commit-graph.c
index 3d644fddc0..9b481bcd06 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -766,6 +766,7 @@ void write_commit_graph(const char *obj_dir,
die(_("error opening index for %s"), 
packname.buf);
for_each_object_in_pack(p, add_packed_commits, , 
0);
close_pack(p);
+   free(p);
}
stop_progress();
strbuf_release();
-- 
2.19.0.329.g76f2f5c1e3

Re: [PATCH 3/5] diff --color-moved-ws: fix a memory leak

[Martin Ågren]

On Tue, 2 Oct 2018 at 20:04, Phillip Wood  wrote:
> const struct emitted_diff_symbol *longer =  a->len > b->len ? a : b;
> const struct emitted_diff_symbol *shorter = a->len > b->len ? b : a;
> int d = longer->len - shorter->len;
> +   int ret = !strncmp(longer->line + d, shorter->line, shorter->len);
> +
> +   if (!ret)
> +   return ret;
>
> out->string = xmemdupz(longer->line, d);
> out->current_longer = (a == longer);
>
> -   return !strncmp(longer->line + d, shorter->line, shorter->len);
> +   return ret;
>  }

The caller only cares about 0 vs non-0, so this would also work:

   if (strncmp(...))
   return 0;

   ...

   return 1;

Just a thought, to avoid some "!"s and the new variable `ret`.

Martin

Re: [PATCH 1/2] commit-graph: clean up leaked memory during write

[Martin Ågren]

On Tue, 2 Oct 2018 at 19:59, Stefan Beller  wrote:
> > > +
> > > +   string_list_clear(, 0);
> > >  }
> >
> > Nit: The blank line adds some asymmetry, IMVHO.
>
> I think these blank lines are super common, as in:
>
> {
>   declarations;
>
>   multiple;
>   lines(of);
>   code;
>
>   cleanup;
>   and_frees;
> }
>
> (c.f. display_table in column.c, which I admit to have
> cherry-picked as an example).
>
> While in nit territory, I would rather move the string list init
> into the first block:
>
>   {
> struct string_list list = STRING_LIST_INIT_DUP;
>
> for_each_ref(add_ref_to_list, );
> write_commit_graph(obj_dir, NULL, , append);
>
> string_list_clear(, 0);
>   }

Now this looks very symmetrical. :-)

> > >  void write_commit_graph(const char *obj_dir,
> > > @@ -846,9 +848,11 @@ void write_commit_graph(const char *obj_dir,
> > > compute_generation_numbers(, report_progress);
> > >
> > > graph_name = get_commit_graph_filename(obj_dir);
> > > -   if (safe_create_leading_directories(graph_name))
> > > +   if (safe_create_leading_directories(graph_name)) {
> > > +   UNLEAK(graph_name);
> > > die_errno(_("unable to create leading directories of %s"),
> > >   graph_name);
> > > +   }
> >
> > Do you really need this hunk?
>
> graph_name is produced via xstrfmt in get_commit_graph_filename,
> so it needs to be free'd in any return/exit path.

Agreed. Although I am questioning that `die()` and its siblings count.

> > In my testing with LeakSanitizer and
> > valgrind, I don't need this hunk to be leak-free.
>
>
> > Generally speaking, it
> > seems impossible to UNLEAK when dying, since we don't know what we have
> > allocated higher up in the call-stack.
>
> I do not understand; I thought UNLEAK was specifically for the purpose of
> die() calls without imposing extra overhead; rereading 0e5bba53af
> (add UNLEAK annotation for reducing leak false positives, 2017-09-08)
> doesn't provide an example for prematurely die()ing, only for regular
> program exit.
>
> > [...] With this hunk, I am
> > puzzled and feel uneasy, both about having to UNLEAK before dying and
> > about having to UNLEAK outside of builtin/.
>
> I am not uneasy about an UNLEAK before dying, but about dying outside
> builtin/ in general

Yeah, not dying would be even better (out of scope for this patch).

> (but having a die call accompanied by UNLEAK seems
> to be the right thing). Can you explain the worries you have regarding the
> allocations on the call stack, as xstrfmt is allocating on the heap and we
> only UNLEAK the pointer to that?

I think we agree that leaking things "allocat[ed] on the call stack"
isn't much of a worry. The reason I mentioned the call stack is that
we've got any number of calls behind us on it, and we might have made
all sorts of allocations on the heap, and at this point, we have no
idea about what we should be UNLEAK-ing.

My worry is that one of these would seem to be true:

* UNLEAK is unsuitable for the job. Whenever we have a `die()` as we do
  here, we can UNLEAK the variables we know of, but we can't do anything
  about the allocations we have made higher up the call-chain. Our test
  suite obviously provokes lots of calls to `die()` -- imagine that each
  of those leaves a few leaked allocations behind. We'd have a semi-huge
  number of leaks being reported. While we could mark with UNLEAK to
  reduce that number, we wouldn't be able to bring the number of leaks
  down to anywhere near manageable where we'd be able to find the last
  few true positives.

* We add code with no purpose. In this case, we're not talking a lot of
  lines, but across the code base, if they bring no gain, they are bound
  to provide a negative net value given enough time.

Martin

Re: [PATCH 1/2] commit-graph: clean up leaked memory during write

[Martin Ågren]

On Tue, 2 Oct 2018 at 17:01, Derrick Stolee via GitGitGadget
 wrote:
> diff --git a/commit-graph.c b/commit-graph.c
> index 2a24eb8b5a..7226bd6b58 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -698,6 +698,8 @@ void write_commit_graph_reachable(const char *obj_dir, 
> int append,
> string_list_init(, 1);
> for_each_ref(add_ref_to_list, );
> write_commit_graph(obj_dir, NULL, , append, report_progress);
> +
> +   string_list_clear(, 0);
>  }

Nit: The blank line adds some asymmetry, IMVHO.

>  void write_commit_graph(const char *obj_dir,
> @@ -846,9 +848,11 @@ void write_commit_graph(const char *obj_dir,
> compute_generation_numbers(, report_progress);
>
> graph_name = get_commit_graph_filename(obj_dir);
> -   if (safe_create_leading_directories(graph_name))
> +   if (safe_create_leading_directories(graph_name)) {
> +   UNLEAK(graph_name);
> die_errno(_("unable to create leading directories of %s"),
>   graph_name);
> +   }

Do you really need this hunk? In my testing with LeakSanitizer and
valgrind, I don't need this hunk to be leak-free. Generally speaking, it
seems impossible to UNLEAK when dying, since we don't know what we have
allocated higher up in the call-stack.

Without this hunk, this patch can have my

Reviewed-by: Martin Ågren 

as I've verified the leaks before and after. With this hunk, I am
puzzled and feel uneasy, both about having to UNLEAK before dying and
about having to UNLEAK outside of builtin/.

> +   free(graph_name);
> +   free(commits.list);
> free(oids.list);
> oids.alloc = 0;
> oids.nr = 0;

Both `commits` and `oids` are on the stack here, so cleaning up one more
than the other is a bit asymmetrical. Also, if we try to zero the counts
-- which seems unnecessary to me, but which is not new with this patch --
we should perhaps use `FREE_AND_NULL` too. But personally, I would just
use `free` and leave `nr` and `alloc` at whatever values they happen to
have.

Martin

Re: [PATCH] Documentation/CodingGuidelines: How to document new APIs

[Martin Ågren]

On Fri, 28 Sep 2018 at 18:51, Junio C Hamano  wrote:
> We recommend documenting in the header over documenting near the
> implementation to encourage people to write the docs that are
> readable without peeking at the implemention.

s/implemention/implementation/

> - - When you come up with an API, document it.
> + - When you come up with an API, document it the functions and the

s/it the/the/

> +   structures in the header file that exposes the API to its callers.
> +   Use what is in "strbuf.h" as a model to decide the appropriate tone
> +   in which the description is given, and the level of details to put
> +   in the description.

Martin

[PATCH] t1400: drop debug `echo` to actually execute `test`

[Martin Ågren]

Instead of running `test "foo" = "$(bar)"`, we prefix the whole thing
with `echo`. Comparing to nearby tests makes it clear that this is just
debug leftover. This line has actually been modified four times since it
was introduced in e52290428b (General ref log reading improvements.,
2006-05-19) and the `echo` has always survived. Let's finally drop it.

This script could need some more cleanups. This is just an immediate fix
so that we actually test what we intend to.

All other hits for `git grep "\
---
 t/t1400-update-ref.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 02493f14ba..b72beebe42 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -359,21 +359,21 @@ ld="Thu, 26 May 2005 18:43:00 -0500"
 test_expect_success 'Query "master@{May 25 2005}" (before history)' '
test_when_finished "rm -f o e" &&
git rev-parse --verify "master@{May 25 2005}" >o 2>e &&
test $C = $(cat o) &&
test "warning: Log for '\''master'\'' only goes back to $ed." = "$(cat 
e)"
 '
 test_expect_success 'Query master@{2005-05-25} (before history)' '
test_when_finished "rm -f o e" &&
git rev-parse --verify master@{2005-05-25} >o 2>e &&
test $C = $(cat o) &&
-   echo test "warning: Log for '\''master'\'' only goes back to $ed." = 
"$(cat e)"
+   test "warning: Log for '\''master'\'' only goes back to $ed." = "$(cat 
e)"
 '
 test_expect_success 'Query "master@{May 26 2005 23:31:59}" (1 second before 
history)' '
test_when_finished "rm -f o e" &&
git rev-parse --verify "master@{May 26 2005 23:31:59}" >o 2>e &&
test $C = $(cat o) &&
test "warning: Log for '\''master'\'' only goes back to $ed." = "$(cat 
e)"
 '
 test_expect_success 'Query "master@{May 26 2005 23:32:00}" (exactly history 
start)' '
test_when_finished "rm -f o e" &&
git rev-parse --verify "master@{May 26 2005 23:32:00}" >o 2>e &&
-- 
2.19.0.216.g2d3b1c576c

Re: [PATCH v2 0/4] git-commit-graph.txt: various cleanups

[Martin Ågren]

Hi Derrick

On Thu, 27 Sep 2018 at 21:16, Derrick Stolee  wrote:
> Thanks! This version satisfies my concerns and looks good to me.
>
> Reviewed-by: Derrick Stolee 

Thanks for the spectacularly snappy review. I don't expect commit graphs
to help my use cases a lot, but I still wanted to try them out a little
and stumbled on the `*` lists. Thanks for doing this work!

Martin

[PATCH v2 1/4] git-commit-graph.txt: fix bullet lists

[Martin Ågren]

We have a couple of bullet items which span multiple lines, and where we
have prefixed each line with a `*`. (This might be the result of a text
editor trying to help.) This results in each line being typeset as a
separate bullet item. Drop the extra `*`.

Signed-off-by: Martin Ågren 
---
 Documentation/git-commit-graph.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-commit-graph.txt 
b/Documentation/git-commit-graph.txt
index dececb79d7..f42f2a1481 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -73,7 +73,7 @@ $ git commit-graph write
 
 
 * Write a graph file, extending the current graph file using commits
-* in .
+  in .
 +
 
 $ echo  | git commit-graph write --stdin-packs
@@ -86,7 +86,7 @@ $ git show-ref -s | git commit-graph write --stdin-commits
 
 
 * Write a graph file containing all commits in the current
-* commit-graph file along with those reachable from HEAD.
+  commit-graph file along with those reachable from HEAD.
 +
 
 $ git rev-parse HEAD | git commit-graph write --stdin-commits --append
-- 
2.19.0.216.g2d3b1c576c

[PATCH v2 4/4] Doc: refer to the "commit-graph file" with dash

[Martin Ågren]

The file processed by `git commit-graph` is referred to as the
"commit-graph file", also with a dash. We have a few references to the
"commit graph file", though, without the dash. These occur in
git-commit-graph.txt as well as in Doc/technical/commit-graph.txt. Fix
them.

Do not change the references to the "commit graph" (without "... file")
as a data structure.

Signed-off-by: Martin Ågren 
---
 Documentation/git-commit-graph.txt   | 12 ++--
 Documentation/technical/commit-graph.txt |  8 
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-commit-graph.txt 
b/Documentation/git-commit-graph.txt
index f0a171..624470e198 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -3,7 +3,7 @@ git-commit-graph(1)
 
 NAME
 
-git-commit-graph - Write and verify Git commit graph files
+git-commit-graph - Write and verify Git commit-graph files
 
 
 SYNOPSIS
@@ -17,16 +17,16 @@ SYNOPSIS
 DESCRIPTION
 ---
 
-Manage the serialized commit graph file.
+Manage the serialized commit-graph file.
 
 
 OPTIONS
 ---
 --object-dir::
-   Use given directory for the location of packfiles and commit graph
+   Use given directory for the location of packfiles and commit-graph
file. This parameter exists to specify the location of an alternate
that only has the objects directory, not a full `.git` directory. The
-   commit graph file is expected to be at `/info/commit-graph` and
+   commit-graph file is expected to be at `/info/commit-graph` and
the packfiles are expected to be in `/pack`.
 
 
@@ -34,7 +34,7 @@ COMMANDS
 
 'write'::
 
-Write a commit graph file based on the commits found in packfiles.
+Write a commit-graph file based on the commits found in packfiles.
 +
 With the `--stdin-packs` option, generate the new commit graph by
 walking objects only in the specified pack-indexes. (Cannot be combined
@@ -66,7 +66,7 @@ database. Used to check for corrupted data.
 EXAMPLES
 
 
-* Write a commit graph file for the packed commits in your local `.git`
+* Write a commit-graph file for the packed commits in your local `.git`
   directory.
 +
 
diff --git a/Documentation/technical/commit-graph.txt 
b/Documentation/technical/commit-graph.txt
index c664acbd76..6b7dde011e 100644
--- a/Documentation/technical/commit-graph.txt
+++ b/Documentation/technical/commit-graph.txt
@@ -15,13 +15,13 @@ There are two main costs here:
 1. Decompressing and parsing commits.
 2. Walking the entire graph to satisfy topological order constraints.
 
-The commit graph file is a supplemental data structure that accelerates
+The commit-graph file is a supplemental data structure that accelerates
 commit graph walks. If a user downgrades or disables the 'core.commitGraph'
 config setting, then the existing ODB is sufficient. The file is stored
 as "commit-graph" either in the .git/objects/info directory or in the info
 directory of an alternate.
 
-The commit graph file stores the commit graph structure along with some
+The commit-graph file stores the commit graph structure along with some
 extra metadata to speed up graph walks. By listing commit OIDs in lexi-
 cographic order, we can identify an integer position for each commit and
 refer to the parents of a commit using those integer positions. We use
@@ -103,7 +103,7 @@ that of a parent.
 Design Details
 --
 
-- The commit graph file is stored in a file named 'commit-graph' in the
+- The commit-graph file is stored in a file named 'commit-graph' in the
   .git/objects/info directory. This could be stored in the info directory
   of an alternate.
 
@@ -127,7 +127,7 @@ Future Work
 - 'log --topo-order'
 - 'tag --merged'
 
-- A server could provide a commit graph file as part of the network protocol
+- A server could provide a commit-graph file as part of the network protocol
   to avoid extra calculations by clients. This feature is only of benefit if
   the user is willing to trust the file, because verifying the file is correct
   is as hard as computing it from scratch.
-- 
2.19.0.216.g2d3b1c576c

[PATCH v2 2/4] git-commit-graph.txt: typeset more in monospace

[Martin Ågren]

While we're here, fix an instance of "folder" to be "directory".

Signed-off-by: Martin Ågren 
---
 Documentation/git-commit-graph.txt | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-commit-graph.txt 
b/Documentation/git-commit-graph.txt
index f42f2a1481..6ac610f016 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -25,9 +25,9 @@ OPTIONS
 --object-dir::
Use given directory for the location of packfiles and commit graph
file. This parameter exists to specify the location of an alternate
-   that only has the objects directory, not a full .git directory. The
-   commit graph file is expected to be at /info/commit-graph and
-   the packfiles are expected to be in /pack.
+   that only has the objects directory, not a full `.git` directory. The
+   commit graph file is expected to be at `/info/commit-graph` and
+   the packfiles are expected to be in `/pack`.
 
 
 COMMANDS
@@ -66,14 +66,15 @@ database. Used to check for corrupted data.
 EXAMPLES
 
 
-* Write a commit graph file for the packed commits in your local .git folder.
+* Write a commit graph file for the packed commits in your local `.git`
+  directory.
 +
 
 $ git commit-graph write
 
 
 * Write a graph file, extending the current graph file using commits
-  in .
+  in ``.
 +
 
 $ echo  | git commit-graph write --stdin-packs
@@ -86,7 +87,7 @@ $ git show-ref -s | git commit-graph write --stdin-commits
 
 
 * Write a graph file containing all commits in the current
-  commit-graph file along with those reachable from HEAD.
+  commit-graph file along with those reachable from `HEAD`.
 +
 
 $ git rev-parse HEAD | git commit-graph write --stdin-commits --append
-- 
2.19.0.216.g2d3b1c576c

[PATCH v2 0/4] git-commit-graph.txt: various cleanups

[Martin Ågren]

This v2 starts with the same two patches as v1 did, then goes on to
change "[commit] graph file" to "commit-graph file" with a dash, to
match other instances as well as Derrick's feedback.

Martin Ågren (4):
  git-commit-graph.txt: fix bullet lists
  git-commit-graph.txt: typeset more in monospace
  git-commit-graph.txt: refer to "*commit*-graph file"
  Doc: refer to the "commit-graph file" with dash

 Documentation/git-commit-graph.txt   | 31 
 Documentation/technical/commit-graph.txt |  8 +++---
 2 files changed, 20 insertions(+), 19 deletions(-)

Range-diff against v1:
1:  222721870b = 1:  837ef2f231 git-commit-graph.txt: fix bullet lists
2:  acac5c3584 = 2:  9759a162ca git-commit-graph.txt: typeset more in monospace
3:  65f42c947a ! 3:  759bc886d8 git-commit-graph.txt: refer to "*commit* graph 
file"
@@ -1,17 +1,17 @@
 Author: Martin Ågren 
 
-git-commit-graph.txt: refer to "*commit* graph file"
+git-commit-graph.txt: refer to "*commit*-graph file"
 
-This document sometimes refers to the "commit graph file" as just "the
+This document sometimes refers to the "commit-graph file" as just "the
 graph file". This saves a couple of words here and there at the risk of
 confusion. In particular, the documentation for `git commit-graph read`
 appears to suggest that there are indeed different types of graph 
files.
 
 Let's just write out the full name everywhere.
 
-The full name, by the way, is not the "commit-graph file" with a dash,
-cf. the synopsis. Use the dashless form. (The next commit will fix the
-remaining few instances of the "commit-graph file" in this document.)
+The full name, by the way, is not the dash-less "commit graph file".
+Use the dashed form. (The next commit will fix the remaining few
+instances of the "commit graph file" in this document.)
 
 Signed-off-by: Martin Ågren 
 
@@ -24,7 +24,7 @@
  
 -Read a graph file given by the commit-graph file and output basic
 -details about the graph file. Used for debugging purposes.
-+Read the commit graph file and output basic details about it.
++Read the commit-graph file and output basic details about it.
 +Used for debugging purposes.
  
  'verify'::
@@ -35,7 +35,7 @@
  
 -* Write a graph file, extending the current graph file using commits
 -  in ``.
-+* Write a commit graph file, extending the current commit graph file
++* Write a commit-graph file, extending the current commit-graph file
 +  using commits in ``.
  +
  
@@ -43,16 +43,14 @@
  
  
 -* Write a graph file containing all reachable commits.
-+* Write a commit graph file containing all reachable commits.
++* Write a commit-graph file containing all reachable commits.
  +
  
  $ git show-ref -s | git commit-graph write --stdin-commits
  
  
 -* Write a graph file containing all commits in the current
--  commit-graph file along with those reachable from `HEAD`.
-+* Write a commit graph file containing all commits in the current
-+  commit graph file along with those reachable from `HEAD`.
++* Write a commit-graph file containing all commits in the current
+   commit-graph file along with those reachable from `HEAD`.
  +
  
- $ git rev-parse HEAD | git commit-graph write --stdin-commits --append
4:  fc81147ea4 < -:  -- git-commit-graph.txt: refer to the "commit 
graph file" without dash
-:  -- > 4:  99b64287ec Doc: refer to the "commit-graph file" with dash
-- 
2.19.0.216.g2d3b1c576c

[PATCH v2 3/4] git-commit-graph.txt: refer to "*commit*-graph file"

[Martin Ågren]

This document sometimes refers to the "commit-graph file" as just "the
graph file". This saves a couple of words here and there at the risk of
confusion. In particular, the documentation for `git commit-graph read`
appears to suggest that there are indeed different types of graph files.

Let's just write out the full name everywhere.

The full name, by the way, is not the dash-less "commit graph file".
Use the dashed form. (The next commit will fix the remaining few
instances of the "commit graph file" in this document.)

Signed-off-by: Martin Ågren 
---
 Documentation/git-commit-graph.txt | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-commit-graph.txt 
b/Documentation/git-commit-graph.txt
index 6ac610f016..f0a171 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -54,8 +54,8 @@ existing commit-graph file.
 
 'read'::
 
-Read a graph file given by the commit-graph file and output basic
-details about the graph file. Used for debugging purposes.
+Read the commit-graph file and output basic details about it.
+Used for debugging purposes.
 
 'verify'::
 
@@ -73,20 +73,20 @@ EXAMPLES
 $ git commit-graph write
 
 
-* Write a graph file, extending the current graph file using commits
-  in ``.
+* Write a commit-graph file, extending the current commit-graph file
+  using commits in ``.
 +
 
 $ echo  | git commit-graph write --stdin-packs
 
 
-* Write a graph file containing all reachable commits.
+* Write a commit-graph file containing all reachable commits.
 +
 
 $ git show-ref -s | git commit-graph write --stdin-commits
 
 
-* Write a graph file containing all commits in the current
+* Write a commit-graph file containing all commits in the current
   commit-graph file along with those reachable from `HEAD`.
 +
 
-- 
2.19.0.216.g2d3b1c576c

Re: [PATCH v2] git.txt: mention mailing list archive

[Martin Ågren]

Hey

On Thu, 27 Sep 2018 at 08:37, Jonathan Nieder  wrote:
> Martin Ågren wrote:
>
> > --- a/Documentation/git.txt
> > +++ b/Documentation/git.txt
> > @@ -859,6 +859,9 @@ Reporting Bugs
> >  Report bugs to the Git mailing list  where the
> >  development and maintenance is primarily done.  You do not have to be
> >  subscribed to the list to send a message there.
> > +If you want to check to see if the issue has
> > +been reported already, the list archive can be found at
> > +<https://public-inbox.org/git/> and other places.
>
> Hm.  I think this encourages a behavior that I want to discourage:
> assuming that if a bug has already been reported then there's nothing
> more for the new user to add.

It was my hope that all of these could be inferred from the above text:

"I'll just drop a mail anyway."

"I wonder if there's a known solution to my issue."

"I wonder if this is known and I can provide some more details compared
to the original poster."

"Maybe I can find some thread where I can just say '+1'."

But what a language-lawyer reading says is of course a lot less relevant
than what a fresh pair of eyes (yours) reads out of the text. Thanks.

> Especially because the mailing list is not an issue tracker, this
> would make it too easy for the project to miss important bugs.
>
> Can this say something more neutral, like
>
> See the list archive at https://public-inbox.org/git/ for
> previous bug reports and other discussions.
>
> ?

This doesn't say "*Please* see", but it comes pretty close. Maybe
something like

  If you want to, you can see the list archive at, e.g.,
  <https://public-inbox.org/git/> for bug reports and other discussions.

>  Or if we want to encourage a particular behavior, should we say
> something about "To coordinate with others experiencing the same
> problem" or something else that encourages joining in with the
> thread instead of assuming it's taken care of?

We might also conclude that trying to delicately word-smith something
that doesn't scare off reports is tricky, and we're better off just
avoiding doing anything which might raise someone's bar for reporting an
issue. I'm leaning more and more towards "it's not broken, so don't fix
it"...

Martin

[PATCH v2] git.txt: mention mailing list archive

[Martin Ågren]

In the "Reporting Bugs" section of git(1), we refer to the mailing list,
but we do not give any hint about where the archives might be found. Of
course, any web search engine can be used to try to hunt down whether an
issue is already known. But we can do better by mentioning the archive
at public-inbox. Make sure to phrase this in a way that avoids raising
the bar for reporting.

public-inbox.org/git/ is usually our preferred archive, since it uses
message ids in its permalinks. But it also has a search function right
at the top of each page, and searching gives the most recent hits first.
Searching for some keyword about a bug or regression should pretty
easily reveal whether it has been recently reported.

Helped-by: Junio C Hamano 
Signed-off-by: Martin Ågren 
---
 Thanks Junio and Taylor for thoughts on this. I agree we do not want
 to scare anyone away. I hope this does the trick.

 Documentation/git.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 74a9d7edb4..68393f3235 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -859,6 +859,9 @@ Reporting Bugs
 Report bugs to the Git mailing list  where the
 development and maintenance is primarily done.  You do not have to be
 subscribed to the list to send a message there.
+If you want to check to see if the issue has
+been reported already, the list archive can be found at
+<https://public-inbox.org/git/> and other places.
 
 Issues which are security relevant should be disclosed privately to
 the Git Security mailing list .
-- 
2.19.0.216.g2d3b1c576c

[PATCH] t7005-editor: quote filename to fix whitespace-issue

[Martin Ågren]

From: Alexander Pyhalov 

Commit 4362da078e (t7005-editor: get rid of the SPACES_IN_FILENAMES
prereq, 2018-05-14) removed code for detecting whether spaces in
filenames work. Since we rely on spaces throughout the test suite
("trash directory.t1234-foo"), testing whether we can use the filename
"e space.sh" was redundant and unnecessary.

In simplifying the code, though, this introduced a regression around how
spaces are handled, not in the /name/ of the editor script, but /in/ the
script itself. The script just does `echo space >$1`, where $1 is for
example "/foo/t/trash directory.t7005-editor/.git/COMMIT_EDITMSG".

With most shells, or with Bash in posix mode, $1 will not be subjected
to field splitting. But if we invoke Bash directly, which will happen if
we build Git with SHELL_PATH=/bin/bash, it will detect and complain
about an "ambiguous redirect". More details can be found in [1], thanks
to SZEDER Gábor.

Make sure that the editor script quotes "$1" to remove the ambiguity.

[1] https://public-inbox.org/git/20180926121107.GH27036@localhost/

Signed-off-by: Alexander Pyhalov 
Commit-message-by: Martin Ågren 
Signed-off-by: Martin Ågren 
---
 SZEDER wrote:
 > Let me put on my POSIX-lawyer hat for a moment to explain this :)

 Thanks for an excellent explanation.

 > Sidenote: this test should use the write_script helper to create this
 > editor script.

 Yes. I've punted on that for now.

 Here's my updated commit message as part of a proper patch. Thanks
 Alexander for the analysis and the diff, and thanks Eric and SZEDER for
 getting me on the right track with the commit message.

 t/t7005-editor.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index b2ca77b338..5fcf281dfb 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -112,7 +112,7 @@ do
 done
 
 test_expect_success 'editor with a space' '
-   echo "echo space >\$1" >"e space.sh" &&
+   echo "echo space >\"\$1\"" >"e space.sh" &&
chmod a+x "e space.sh" &&
GIT_EDITOR="./e\ space.sh" git commit --amend &&
test space = "$(git show -s --pretty=format:%s)"
-- 
2.19.0.216.g2d3b1c576c

Re: t7005-editor.sh failure

[Martin Ågren]

On Wed, 26 Sep 2018 at 13:59, Eric Sunshine  wrote:
> This description of the behavior is misleading (actually, actively
> wrong).

Hmm, that's bad, my apologies.

> echo foo bar >cow
> echo >cow foo bar
> echo foo >cow bar
>
> That is, they all create a file named "cow" with content "foo bar".

Somehow I knew that, as in "I've seen that before", but I guess I've
never thought about it long enough to really incorporate it.

> So, in your example:
>
> echo space >/foo/t/trash directory.t7005-editor/.git/COMMIT_EDITMSG
>
> what is actually happening is that it is creating a file named
> "/foo/t/trash" with content "space
> directory.t7005-editor/.git/COMMIT_EDITMSG".

Thanks for clarifying.

> As for the "ambiguous redirect" diagnostic, that seems to be Bash
> trying to be helpful in reporting what is likely a programming error
> (that is, forgetting to double-quote the expansion).

I see that SZEDER has posted some interesting reading. I'll make sure I
understand this better before coming back to this later today.

Thanks
Martin

Re: t7005-editor.sh failure

[Martin Ågren]

On Wed, 26 Sep 2018 at 11:00, Alexander Pyhalov  wrote:
> As for sign-off, do I understand correctly that you just want to know
> that I'm the original author of the code? Yes, it's so.

Right. Plus that you agree that the code (the commit) may be
redistributed basically forever.

> I see this on OpenIndiana in
> https://github.com/OpenIndiana/oi-userland/pull/4456 , when running
> test suite.
> Not sure why it wasn't noticed earlier, as 'trash directory' is used in path.

My first theory was that my shell and that of other developers was
"modern" or "clever" enough to realize that the space belongs to the
filename, so it just takes everything to the end of line. Whereas your
shell would be "dumber". I see now that you have a newer bash than I
do... Maybe this cleverness can be configured (at compile-time?), or
maybe something else is happening.

> execve("/bin/bash", 0x007EA898, 0x007EA960)  argc = 5
> 2655:argv: /bin/bash -c ./e\ space.sh "$@" ./e\ space.sh
> 2655: 
> /export/home/alp/srcs/oi-userland/components/developer/git/build/amd64/t/trash
> directory.t7005-editor/.git/COMMIT_EDITMSG
> 2655:   execve("./e space.sh", 0x005655C8, 0x00564008)  Err#8 ENOEXEC
> ./e space.sh: line 1: $1: ambiguous redirect

> Shell is bash, as you can see (GNU bash, version 4.4.23(1)-release
> (i386-pc-solaris2.11))

I came up with the following commit message. What do you think about it?

t7005-editor: quote filename to fix whitespace-issue

Commit 4362da078e (t7005-editor: get rid of the SPACES_IN_FILENAMES
prereq, 2018-05-14) removed code for detecting whether spaces in
filenames work. Since we rely on spaces throughout the test suite
("trash directory.t1234-foo"), testing whether we can use the filename
"e space.sh" was redundant and unnecessary.

In simplifying the code, though, the commit introduced a regression around
how spaces are handled, not in the /name/ of the script, but /in/ the
script itself. The editor-script created looks like this:

  echo space >$1

We will try to execute something like

  echo space >/foo/t/trash directory.t7005-editor/.git/COMMIT_EDITMSG

Most shells seem to be able to figure out that the filename doesn't end
with "trash" but continues all the way to "COMMIT_EDITMSG", but at least
one shell chokes on this.

Make sure that the editor-script quotes "$1".

Martin

Re: [PATCH] git.txt: mention mailing list archive

[Martin Ågren]

On Thu, 20 Sep 2018 at 21:07, Junio C Hamano  wrote:
>
> Martin Ågren  writes:
>
> > In the "Reporting Bugs" section of git(1), we refer to the mailing list,
> > but we do not give any hint about where the archives might be found.
>
> And why is it a good idea to give that information in Reporting Bugs
> section?  Are we asking the bug reporters to look for similar issues
> in the archive before they send their message?  If so, I think that

Your guess is correct, sorry for forcing you to make one.

> we should be explicit about it, too.  Otherwise, the list archive
> location would look like an irrelevant noise to those who wanted to
> find the address to report bugs to.
>
> For example, we can say something like this:
>
> >  Report bugs to the Git mailing list  where the
> >  development and maintenance is primarily done.  You do not have to be
> >  subscribed to the list to send a message there.
>   +If you want to check to see if the issue has
>   +been reported already, the list archive can be found at
>   +<https://public-inbox.org/git/> and other places.

I think that one reason I avoided spelling out why giving the archive
location was a good thing to do, was that I didn't want to begin a huge
list of "please do this and that", scaring away potential bug reporters.
I think your "If you want to" solves that problem very nicely. I'll wrap
this up later today.

Martin

Re: t7005-editor.sh failure

[Martin Ågren]

Hi Alexander,

Welcome to the list!

On Wed, 26 Sep 2018 at 08:54, Alexander Pyhalov  wrote:
> On updating git to 2.19 we've suddenly got t7005-editor.sh test failures.
> The issue seems to be that generated "e space.sh" file can't handle
> files with spaces.
> Instead of 'echo space >$1' it should be 'echo space > "$1"' or git
> editor fails when gets file with spaces in name.

Thanks for finding, analysing and reporting. I haven't bisected, but I'm
guessing this comes from 4362da078e (t7005-editor: get rid of the
SPACES_IN_FILENAMES prereq, 2018-05-14), which only happens to have to
do with spaces in filenames. But in rewriting the test, it introduced
/another/ instance of spaces-matter-here and didn't quote $1 properly.
Cute. :-)

We try to snuggle the filename to the >redirector, so it would be 'echo
space >"$1"' and similar.

Could we have your sign-off for this? Please see [1] for what that
means. If you want to re-submit as a "proper" patch with commit message
and all, great. If not, I could do it for you, with you as "Author:", if
you just let me know.

By the way, could you say something about which shell or which
environment this bug triggered in? Just so we can better understand how
this snuck past us.

[1] https://github.com/git/git/blob/master/Documentation/SubmittingPatches

Thanks
Martin

Re: [PATCH 3/4] git-commit-graph.txt: refer to "*commit* graph file"

[Martin Ågren]

On Thu, 20 Sep 2018 at 14:50, Derrick Stolee  wrote:
>
> On 9/19/2018 12:30 PM, Martin Ågren wrote:
> > The full name, by the way, is not the "commit-graph file" with a dash,
> > cf. the synopsis. Use the dashless form. (The next commit will fix the
> > remaining few instances of the "commit-graph file" in this document.)
>
> The file is literally at ".git/objects/info/commit-graph" which is why I
> tried to use "commit-graph" everywhere. Why do you think that "commit
> graph" is better?

I noticed the discrepancy between "commit graph file" and "commit-graph
file" and briefly wondered if it was intentional, i.e., if it meant
anything, but the dash vs no dash seemed pretty random to me. In order
to figure out which was (more) correct, I went to the synopsis. But
admittedly, that was quite arbitrary. For all I know, "the commit-graph
file" could be the better choice, grammatically.

There is the file named "commit-graph" as you note, but it might on the
other hand just as well be called "cg.bin". I would probably try to let
the filename "commit-graph" influence the user manual only if we would
have written "cg.bin" instead. For example, if we would talk about how
you might get out of a hole by deleting the "<...>/commit-graph"
("cg.bin") file manually.

But that's certainly not to argue against "the commit-graph file". I'd
be happy to s/commit graph file/commit-graph file/g instead to keep
others from wondering if these are two slightly different things. And
if the concept and the file have the same name, all the better.

If you agree, I'll do that in a v2, where I will also note in the
Options section that `--object-dir` takes a ``.

Martin

Re: [PATCH 1/9] Makefile: add a hdr-check target

[Martin Ågren]

On Wed, 19 Sep 2018 at 22:15, Ramsay Jones  wrote:
> On 19/09/18 18:49, Martin Ågren wrote:
> > On Wed, 19 Sep 2018 at 02:07, Ramsay Jones  
> > wrote:
> >> +GEN_HDRS := command-list.h unicode-width.h
> >
> > Most of the things happening around here are obvious steps towards the
> > end-goal and seem to logically belong here, together. This one though,
> > "generated headers"(?) seems like it is more general in nature, so could
> > perhaps live somewhere else.
>
> Yes, generated headers, but no, not more general. ;-)

> The 'command-list.h' is generated as part of the build
> (and so may or may not be part of the LIB_H macro), whereas
> the unicode-width.h header is only re-generated when someone
> runs the 'contrib/update-unicode/update_unicode.sh' script
> (presumably when a new standard version is announced) and
> commits the result.

Ah, I wasn't aware of how unicode-width.h works, thanks.

> Both headers fail the 'hdr-check', although both generator
> scripts could be 'fixed' so that they passed. I just didn't
> think it was worth the effort - neither header was likely
> to be #included anywhere else.

With the above background, I'd tend to agree.

> I guess I could eliminate
> that macro, absorbing it into EXCEPT_HDRS, if that would
> lead to less confusion ...

I'm just a single data point, so don't trust my experience too much.

> > Actually, we have a variable `GENERATED_H` which seems to try to do more
> > or less the same thing. It lists just one file, though, command-list.h.
>
> Hmm, GENERATED_H seems only to be used by the i18n part of the
> makefile and, as a result of its appearance in LIB_H, sometimes
> results in command-list.h appearing twice in LOCALIZED_C.

Just thinking out loud, I suppose you could use $(GENERATED_H) instead
of hard-coding command-list.h in your patch. But with the background you
explained above, I think there's a good counter-argument to be made:

When we gain new auto-generated headers, we wouldn't actually mind `make
hdr-check` failing. We'd get the opportunity to decide whether making
the new header pass the check is worth it, or whether the correct
solution is to blacklist the auto-generated header. That's probably
better than just blacklisting the new header by default so that we don't
even notice that it has a potential problem.

So I think your approach makes sense. I stumbled on the similarity of
the name to something we already have. Maybe something like

  # Ignore various generated files, rather than updating the
  # generating scripts for little or no benefit.
  GEN_HDRS := ...

or a remark in the commit message, or rolling this into EXCEPT_HDRS, but
I'd be perfectly fine just leaving this as it is. Please trust your own
judgment more than mine.

Thanks
Martin

Re: [PATCH] gc: fix regression in 7b0f229222 impacting --quiet

[Martin Ågren]

On Wed, 19 Sep 2018 at 23:04, Ævar Arnfjörð Bjarmason  wrote:
> Fix a regression in my recent 7b0f229222 ("commit-graph write: add
> progress output", 2018-09-17), the newly added progress output for
> "commit-graph write" didn't check the --quiet option.

s/, t/. T/, perhaps. Maybe also s/did/does/.

> Do so, and add a test asserting that this works as expected. Since the
> TTY perquisite isn't available everywhere let's add a version of this

s/perq/prereq/

> that both requires and doesn't require that. This test might be overly
> specific and will break if new progress output is added, but I think
> it'll serve as a good reminder to test the undertested progress
> mode(s).

> +test_expect_success 'gc --no-quiet' '
> +   git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
> +   ! test -s stdout &&

`test_must_be_empty` for easier debugging?

> +   test_line_count = 1 stderr &&
> +   test_i18ngrep "Computing commit graph generation numbers" stderr
> +'

Re: [PATCH] pack-objects: handle island check for "external" delta base

[Martin Ågren]

On Wed, 19 Sep 2018 at 05:49, Jeff King  wrote:
> This is tricky to do inside a single "if" statement. And
> after the merge in f3504ea3dd (Merge branch
> 'cc/delta-islands', 2018-09-17), that "if" condition is
> already getting pretty unwieldy. So this patch moves the
> logic into a helper function, where we can easily use
> multiple return paths. The result is a bit longer, but the
> logic should be much easier to follow.

> +static int can_reuse_delta(const unsigned char *base_sha1,
> +  struct object_entry *delta,
> +  struct object_entry **base_out)
> +{
> +   struct object_entry *base;
> +
> +   if (!base_sha1)
> +   return 0;

So this corresponds to "if (base_ref &&".

> +   /*
> +* First see if we're already sending the base (or it's explicitly in
> +* our "excluded" list.
> +*/

Missing ')'.

> +   base = packlist_find(_pack, base_sha1, NULL);
> +   if (base) {
> +   if (!in_same_island(>idx.oid, >idx.oid))
> +   return 0;

This logic matches the removed code...

> +   *base_out = base;
> +   return 1;
> +   }
> +
> +   /*
> +* Otherwise, reachability bitmaps may tell us if the receiver has it,
> +* even if it was buried too deep in history to make it into the
> +* packing list.
> +*/
> +   if (thin && bitmap_has_sha1_in_uninteresting(bitmap_git, base_sha1)) {

This matches...

> +   if (use_delta_islands) {
> +   struct object_id base_oid;
> +   hashcpy(base_oid.hash, base_sha1);
> +   if (!in_same_island(>idx.oid, _oid))
> +   return 0;

This does some extra juggling to avoid using `base->idx.oid`, which
would have been the moral equivalent of the original code, but which
won't fly since `base` is NULL.

> +   }
> +   *base_out = NULL;
> +   return 1;
> +   }
> +
> +   return 0;
> +}
> +
>  static void check_object(struct object_entry *entry)
>  {
> unsigned long canonical_size;
> @@ -1556,22 +1607,7 @@ static void check_object(struct object_entry *entry)
> break;
> }
>
> -   if (base_ref && (
> -   (base_entry = packlist_find(_pack, base_ref, NULL)) ||
> -   (thin &&
> -bitmap_has_sha1_in_uninteresting(bitmap_git, base_ref))) 
> &&
> -   in_same_island(>idx.oid, _entry->idx.oid)) {

Yeah, the new function looks much simpler than this. We have

  if (A && (B1 || B2) && C) {.

Knowing what to look for, it can be seen that we can -- under the right
circumstances -- have A and B2, but not B1, and try to evalute C by
dereferencing `base_entry` which will be NULL.

> +   if (can_reuse_delta(base_ref, entry, _entry)) {
> oe_set_type(entry, entry->in_pack_type);
> SET_SIZE(entry, in_pack_size); /* delta size */
> SET_DELTA_SIZE(entry, in_pack_size);

Without being at all familiar with this code, this looks sane to me.
Just had a small nit about the missing closing ')'.

Martin

Re: [PATCH 2/2] git-config.txt: fix 'see: above' note

[Martin Ågren]

Hi Taylor,

On Wed, 19 Sep 2018 at 19:21, Taylor Blau  wrote:
> I could take or leave 2/2, since I usually write ", (see: above)", but
> I'm not sure if that's grammatically correct or not.

Well, I sure ain't no grammar expert too... This is not a patch I feel
strongly about, so I'll be happy to defer to others.

Thanks for reviewing,
Martin

Re: [PATCH 1/9] Makefile: add a hdr-check target

[Martin Ågren]

Hi Ramsay,

On Wed, 19 Sep 2018 at 02:07, Ramsay Jones  wrote:
> @@ -2675,6 +2676,17 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
>  .PHONY: sparse $(SP_OBJ)
>  sparse: $(SP_OBJ)
>
> +GEN_HDRS := command-list.h unicode-width.h

Most of the things happening around here are obvious steps towards the
end-goal and seem to logically belong here, together. This one though,
"generated headers"(?) seems like it is more general in nature, so could
perhaps live somewhere else.

Actually, we have a variable `GENERATED_H` which seems to try to do more
or less the same thing. It lists just one file, though, command-list.h.
And unicode-width.h seems to be tracked in git.git.

Maybe use `GENERATED_H` instead, and list unicode-width.h on the next
line instead? Or am I completely misreading "GEN_HDRS"?

> +EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff%
> +CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))
> +HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))
> +
> +$(HCO): %.hco: %.h FORCE
> +   $(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc 
> $<
> +
> +.PHONY: hdr-check $(HCO)
> +hdr-check: $(HCO)
> +
>  .PHONY: style
>  style:
> git clang-format --style file --diff --extensions c,h

[PATCH] git.txt: mention mailing list archive

[Martin Ågren]

In the "Reporting Bugs" section of git(1), we refer to the mailing list,
but we do not give any hint about where the archives might be found.
Copy the text from README.md on this to give potential reporters an
honest chance of finding out whether their bug has already been
reported.

Signed-off-by: Martin Ågren 
---
 Documentation/git.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 74a9d7edb4..40eaccafb2 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -858,7 +858,9 @@ Reporting Bugs
 
 Report bugs to the Git mailing list  where the
 development and maintenance is primarily done.  You do not have to be
-subscribed to the list to send a message there.
+subscribed to the list to send a message there. The mailing list archives
+are available at <https://public-inbox.org/git/>,
+<http://marc.info/?l=git> and other archival sites.
 
 Issues which are security relevant should be disclosed privately to
 the Git Security mailing list .
-- 
2.19.0.216.g2d3b1c576c

[PATCH 2/2] git-config.txt: fix 'see: above' note

[Martin Ågren]

Rather than saying "(see: above)", drop the colon. Also drop the comma
before this note.

Signed-off-by: Martin Ågren 
---
 Documentation/git-config.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 9d8cea72dd..5e87d82933 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -188,8 +188,8 @@ Valid ``'s include:
 --bool-or-int::
 --path::
 --expiry-date::
-  Historical options for selecting a type specifier. Prefer instead `--type`,
-  (see: above).
+  Historical options for selecting a type specifier. Prefer instead `--type`
+  (see above).
 
 --no-type::
   Un-sets the previously set type specifier (if one was previously set). This
-- 
2.19.0.216.g2d3b1c576c

[PATCH 1/2] Doc: use `--type=bool` instead of `--bool`

[Martin Ågren]

After fb0dc3bac1 (builtin/config.c: support `--type=` as preferred
alias for `--`, 2018-04-18) we have a more modern way of spelling
`--bool`.

Update all instances except those that explicitly document the
"historical options" in git-config.txt. The other old-style
type-specifiers already seem to be gone except for in that list of
historical options.

Tweak the grammar a little in config.txt while we are there.

Signed-off-by: Martin Ågren 
---
 Documentation/config.txt | 2 +-
 Documentation/git-config.txt | 4 ++--
 Documentation/git.txt| 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 112041f407..088cbefecc 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -225,7 +225,7 @@ boolean::
false;; Boolean false literals are `no`, `off`, `false`,
`0` and the empty string.
 +
-When converting value to the canonical form using `--bool` type
+When converting a value to its canonical form using the `--type=bool` type
 specifier, 'git config' will ensure that the output is "true" or
 "false" (spelled in lowercase).
 
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 8e240435be..9d8cea72dd 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -442,9 +442,9 @@ For URLs in `https://weak.example.com`, `http.sslVerify` is 
set to
 false, while it is set to `true` for all others:
 
 
-% git config --bool --get-urlmatch http.sslverify https://good.example.com
+% git config --type=bool --get-urlmatch http.sslverify https://good.example.com
 true
-% git config --bool --get-urlmatch http.sslverify https://weak.example.com
+% git config --type=bool --get-urlmatch http.sslverify https://weak.example.com
 false
 % git config --get-urlmatch http https://weak.example.com
 http.cookieFile /tmp/cookie.txt
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 74a9d7edb4..08e533d62b 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -76,7 +76,7 @@ Note that omitting the `=` in `git -c foo.bar ...` is allowed 
and sets
 `foo.bar` to the boolean true value (just like `[foo]bar` would in a
 config file). Including the equals but with an empty value (like `git -c
 foo.bar= ...`) sets `foo.bar` to the empty string which `git config
---bool` will convert to `false`.
+--type=bool` will convert to `false`.
 
 --exec-path[=]::
Path to wherever your core Git programs are installed.
-- 
2.19.0.216.g2d3b1c576c

[PATCH 4/4] git-commit-graph.txt: refer to the "commit graph file" without dash

[Martin Ågren]

The command is `git commit-graph`, but the file it processes is the
"commit graph file" without a dash. We have a few references to the
"commit-graph file", though. Fix them.

Signed-off-by: Martin Ågren 
---
 Documentation/git-commit-graph.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-commit-graph.txt 
b/Documentation/git-commit-graph.txt
index 55f63d47d9..dd0a53736f 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -50,7 +50,7 @@ commits starting at all refs. (Cannot be combined with 
`--stdin-commits`
 or `--stdin-packs`.)
 +
 With the `--append` option, include all commits that are present in the
-existing commit-graph file.
+existing commit graph file.
 
 'read'::
 
@@ -59,7 +59,7 @@ Used for debugging purposes.
 
 'verify'::
 
-Read the commit-graph file and verify its contents against the object
+Read the commit graph file and verify its contents against the object
 database. Used to check for corrupted data.
 
 
@@ -93,7 +93,7 @@ $ git show-ref -s | git commit-graph write --stdin-commits
 $ git rev-parse HEAD | git commit-graph write --stdin-commits --append
 
 
-* Read basic information from the commit-graph file.
+* Read basic information from the commit graph file.
 +
 
 $ git commit-graph read
-- 
2.19.0.216.g2d3b1c576c

[PATCH 3/4] git-commit-graph.txt: refer to "*commit* graph file"

[Martin Ågren]

This document sometimes refers to the "commit graph file" as just "the
graph file". This saves a couple of words here and there at the risk of
confusion. In particular, the documentation for `git commit-graph read`
appears to suggest that there are indeed different types of graph files.

Let's just write out the full name everywhere.

The full name, by the way, is not the "commit-graph file" with a dash,
cf. the synopsis. Use the dashless form. (The next commit will fix the
remaining few instances of the "commit-graph file" in this document.)

Signed-off-by: Martin Ågren 
---
 Documentation/git-commit-graph.txt | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-commit-graph.txt 
b/Documentation/git-commit-graph.txt
index 6ac610f016..55f63d47d9 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -54,8 +54,8 @@ existing commit-graph file.
 
 'read'::
 
-Read a graph file given by the commit-graph file and output basic
-details about the graph file. Used for debugging purposes.
+Read the commit graph file and output basic details about it.
+Used for debugging purposes.
 
 'verify'::
 
@@ -73,21 +73,21 @@ EXAMPLES
 $ git commit-graph write
 
 
-* Write a graph file, extending the current graph file using commits
-  in ``.
+* Write a commit graph file, extending the current commit graph file
+  using commits in ``.
 +
 
 $ echo  | git commit-graph write --stdin-packs
 
 
-* Write a graph file containing all reachable commits.
+* Write a commit graph file containing all reachable commits.
 +
 
 $ git show-ref -s | git commit-graph write --stdin-commits
 
 
-* Write a graph file containing all commits in the current
-  commit-graph file along with those reachable from `HEAD`.
+* Write a commit graph file containing all commits in the current
+  commit graph file along with those reachable from `HEAD`.
 +
 
 $ git rev-parse HEAD | git commit-graph write --stdin-commits --append
-- 
2.19.0.216.g2d3b1c576c

[PATCH 1/4] git-commit-graph.txt: fix bullet lists

[Martin Ågren]

We have a couple of bullet items which span multiple lines, and where we
have prefixed each line with a `*`. (This might be the result of a text
editor trying to help.) This results in each line being typeset as a
separate bullet item. Drop the extra `*`.

Signed-off-by: Martin Ågren 
---
 Documentation/git-commit-graph.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-commit-graph.txt 
b/Documentation/git-commit-graph.txt
index dececb79d7..f42f2a1481 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -73,7 +73,7 @@ $ git commit-graph write
 
 
 * Write a graph file, extending the current graph file using commits
-* in .
+  in .
 +
 
 $ echo  | git commit-graph write --stdin-packs
@@ -86,7 +86,7 @@ $ git show-ref -s | git commit-graph write --stdin-commits
 
 
 * Write a graph file containing all commits in the current
-* commit-graph file along with those reachable from HEAD.
+  commit-graph file along with those reachable from HEAD.
 +
 
 $ git rev-parse HEAD | git commit-graph write --stdin-commits --append
-- 
2.19.0.216.g2d3b1c576c

[PATCH 2/4] git-commit-graph.txt: typeset more in monospace

[Martin Ågren]

While we're here, fix an instance of "folder" to be "directory".

Signed-off-by: Martin Ågren 
---
 Documentation/git-commit-graph.txt | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-commit-graph.txt 
b/Documentation/git-commit-graph.txt
index f42f2a1481..6ac610f016 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -25,9 +25,9 @@ OPTIONS
 --object-dir::
Use given directory for the location of packfiles and commit graph
file. This parameter exists to specify the location of an alternate
-   that only has the objects directory, not a full .git directory. The
-   commit graph file is expected to be at /info/commit-graph and
-   the packfiles are expected to be in /pack.
+   that only has the objects directory, not a full `.git` directory. The
+   commit graph file is expected to be at `/info/commit-graph` and
+   the packfiles are expected to be in `/pack`.
 
 
 COMMANDS
@@ -66,14 +66,15 @@ database. Used to check for corrupted data.
 EXAMPLES
 
 
-* Write a commit graph file for the packed commits in your local .git folder.
+* Write a commit graph file for the packed commits in your local `.git`
+  directory.
 +
 
 $ git commit-graph write
 
 
 * Write a graph file, extending the current graph file using commits
-  in .
+  in ``.
 +
 
 $ echo  | git commit-graph write --stdin-packs
@@ -86,7 +87,7 @@ $ git show-ref -s | git commit-graph write --stdin-commits
 
 
 * Write a graph file containing all commits in the current
-  commit-graph file along with those reachable from HEAD.
+  commit-graph file along with those reachable from `HEAD`.
 +
 
 $ git rev-parse HEAD | git commit-graph write --stdin-commits --append
-- 
2.19.0.216.g2d3b1c576c

[PATCH 0/4] git-commit-graph.txt: various cleanups

[Martin Ågren]

The first patch is a bug-fix. The second applies some more
`monospace`-ing, which should also be good thing.

The last two patches are based on my understanding that `git
commit-graph` handles the "commit graph file", without a dash. If that's
correct, there might be more such cleanups to be made in other parts of
git.git. If the dash should actually be there, I could do these changes
in the other direction. Or maybe dash-vs-no-dash is not an actual
problem at all...

Martin

Martin Ågren (4):
  git-commit-graph.txt: fix bullet lists
  git-commit-graph.txt: typeset more in monospace
  git-commit-graph.txt: refer to "*commit* graph file"
  git-commit-graph.txt: refer to the "commit graph file" without dash

 Documentation/git-commit-graph.txt | 29 +++--
 1 file changed, 15 insertions(+), 14 deletions(-)

-- 
2.19.0.216.g2d3b1c576c

Re: git silently ignores include directive with single quotes

[Martin Ågren]

Hi Stas

On Sat, 8 Sep 2018 at 21:00, Stas Bekman  wrote:
> [include]
> path = '../.gitconfig'
>
> Notice the single quotes around the filename. When this is the case git
> silently (!) ignores the custom configuration, which is clearly a bug.

Thanks for reporting and describing out your expectations and what you
observed.

Actually, there is a test explicitly testing that 'missing include files
are ignored'. I couldn't find a motivation for this in 9b25a0b52e
(config: add include directive, 2012-02-06).

> The original problem cropped up due to using:
>
>  git config --local include.path '../.gitconfig'
>
> which on linux stripped the single quotes, but on some windows git bash
> emulation it kept them.

Huh, I wouldn't have expected them to be kept. You learn something
new every day...

> What am I suggesting is that git:
>
> (1) should complain if it encounters an invalid configuration and not
> silently ignore it. It took quite some effort and time to figure the
> culprit.

Sounds reasonable to me, but I might be missing something. I'm cc-ing
the original author. Maybe he can recall why he made sure it silently
ignores missing files.

> (2) probably allow the quoted location of the file, but it's much less
> important, as it's easy to rectify once git gives user #1

I don't think this will work. Allowing quoting for just this one item,
or for all? Any and all quoting or just at the first and last character?
What about those config items where quotes might legitimately occur,
i.e., we'd need some escaping? Actually, something like '.gitconfig'
*with* *those* *quotes* is a valid filename on my machine.

Thank you for reporting.

Martin

Re: [PATCH v3 2/4] eoie: add End of Index Entry (EOIE) extension

[Martin Ågren]

On Sat, 8 Sep 2018 at 16:04, Ben Peart  wrote:
> On 9/8/2018 2:29 AM, Martin Ågren wrote:
> > Maybe it all works out, e.g., so that when someone (brian) merges a
> > NewHash and runs the testsuite, this will fail consistently and in a
> > safe way. But I wonder if it would be too hard to avoid the hardcoded 24
> > already now.
>
> I can certainly change this to be:
>
> #define EOIE_SIZE (4 + GIT_SHA1_RAWSZ)
>
> which should (hopefully) make it easier to find this hard coded hash
> length in the sea of hard coded "20" and "160" (bits) littered through
> the codebase.

Yeah, that seems more grep-friendly.

Martin

Re: [PATCH v3 2/4] eoie: add End of Index Entry (EOIE) extension

[Martin Ågren]

On Fri, 7 Sep 2018 at 22:24, Ben Peart  wrote:
> > Ben Peart  writes:

> >> - 160-bit SHA-1 over the extension types and their sizes (but not
> >> their contents).  E.g. if we have "TREE" extension that is N-bytes
> >> long, "REUC" extension that is M-bytes long, followed by "EOIE",
> >> then the hash would be:

> The purpose of the SHA isn't to detect disk corruption (we already have
> a SHA for the entire index that can serve that purpose) but to help
> ensure that this was actually a valid EOIE extension and not a lucky
> random set of bytes. [...]

> >> +#define EOIE_SIZE 24 /* <4-byte offset> + <20-byte hash> */

> >> +the_hash_algo->init_fn();
> >> +while (src_offset < mmap_size - the_hash_algo->rawsz - 
> >> EOIE_SIZE_WITH_HEADER) {
[...]
> >> +the_hash_algo->final_fn(hash, );
> >> +if (hashcmp(hash, (unsigned char *)index))
> >> +return 0;
> >> +
> >> +/* Validate that the extension offsets returned us back to the eoie 
> >> extension. */
> >> +if (src_offset != mmap_size - the_hash_algo->rawsz - 
> >> EOIE_SIZE_WITH_HEADER)
> >> +return 0;

Besides the issue you and Junio discussed with "should we document this
as being SHA-1 or NewHash" (or "the hash algo"), it seems to me that
this implementation is living somewhere between using SHA-1 and "the
hash algo". The hashing uses `the_hash_algo`, but the hash size is
hardcoded at 20 bytes.

Maybe it all works out, e.g., so that when someone (brian) merges a
NewHash and runs the testsuite, this will fail consistently and in a
safe way. But I wonder if it would be too hard to avoid the hardcoded 24
already now.

Martin

Re: [PATCH 2/2] submodule.c: warn about missing submodule commit in recursive actions

[Martin Ågren]

> +   if (repo_submodule_init(, the_repository, path) < 0)
> +   warning(_("Could not get submodule repository for submodule 
> 's'"), path);

Missing "%" in format specifier, so `path` will never be used. Also,
s/C/c/ at the start of the warning.

Thanks for marking with _().

Martin

Re: [PATCH] diff: allow --recurse-submodules as an synonym for --submodule

[Martin Ågren]

On Thu, 6 Sep 2018 at 00:59, Stefan Beller  wrote:
>
>  --submodule[=]::

Maybe drop `--submodule` here ...

> +--recurse-submodules[=]::
> Specify how differences in submodules are shown.  When specifying
> `--submodule=short` the 'short' format is used.  This format just

... and use `--recurse-submodules` here ...

> shows the names of the commits at the beginning and end of the range.

... and mention `--submodule` here as a historical alias? Maybe
deprecate it? I suppose the implementation of the aliasing is easy
enough that we can carry `--submodule` around forever, though.

> diff --git a/diff.c b/diff.c
> index 145cfbae592..d3d5a989bd1 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -5023,6 +5023,8 @@ int diff_opt_parse(struct diff_options *options,
> handle_ignore_submodules_arg(options, arg);
> } else if (skip_to_optional_arg_default(arg, "--submodule", , 
> "log"))
> return parse_submodule_opt(options, arg);
> +   else if (skip_to_optional_arg_default(arg, "--recurse-submodules", 
> , "log"))
> +   return parse_submodule_opt(options, arg);

How about (whitespace-damaged)

} else if (skip_to_optional_arg_default(arg, "--submodule", , "log") ||
   skip_to_optional_arg_default(arg, "--recurse-submodules",
, "log"))
return parse_submodule_opt(options, arg);

to make this future-proof? Sure, they're close enough that one should
notice the two instances, and any future work work would supposedly
happen in `parse_submodule_opt()` or anywhere else but here, but still.

Just a few thoughts.

Martin

Re: [PATCH] Document update for nd/unpack-trees-with-cache-tree

[Martin Ågren]

On Sat, 25 Aug 2018 at 14:22, Nguyễn Thái Ngọc Duy  wrote:
>   * Fast path if we detect that all trees are the same as cache-tree at this
> - * path. We'll walk these trees recursively using cache-tree/index instead of
> - * ODB since already know what these trees contain.
> + * path. We'll walk these trees in an iteractive loop using cache-tree/index
> + * instead of ODB since already know what these trees contain.

s/iteractive/iterative/ (i.e., drop "c")

Not new, but still: s/already/we already/

Martin

Re: [PATCH v2] worktree: add --quiet option

[Martin Ågren]

Hi Eric,

On Thu, 16 Aug 2018 at 10:25, Eric Sunshine  wrote:
> (/me nudges Martin off the fence onto the side of not bothering to
> mention the obvious)

:-)

Thanks for sanity-checking my thoughts. I agree with everything you
wrote in your reply (and, FWIW, your other findings that you sent
separately).

Martin

Re: [PATCH v2] worktree: add --quiet option

[Martin Ågren]

On Wed, 15 Aug 2018 at 22:56, Elia Pinto  wrote:
> Add the '--quiet' option to git worktree,
> as for the other git commands. 'add' is the
> only command affected by it since all other
> commands, except 'list', are currently
> silent by default.

Thanks for a follow-up.

The word "currently" means I can't shake the feeling that Eric has a
very good point in [1]:

  It might make sense to say instead that this is adding a --quiet
  option _in general_, rather than doing so specifically for 'add'.

As an example, if `git worktree move` ever learns to produce some sort
of output, then Eric's approach would mean that such a hypothetical
`move` is buggy until it learns to respect `--quiet`. With your
approach, it would mean that we would get feature requests that `move`
should also respect `--quiet` (which we would then need to redefine in
the documentation) or that it should learn of a `--quiet-move` (which I
do not think is a particularly good UI).

Doing such a patch instead would mean tweaking the commit message
slightly...

  Add the '--quiet' option to git worktree, as for the other git
  commands. Currently, 'add' is the only command affected by it since
  all other commands, except 'list' obviously, are already silent by
  default.

... and the documentation slightly ...

  Suppress feedback messages.

It might make sense adding a comment to the documentation about how this
currently only affects `add`, but such comments do risk going stale. I
am on the fence about whether the documentation needs to say something
about `list` -- who in their right mind would expect `list --quiet` to
actually suppress the list? And if `list` ever learns to give some
feedback messages in addition to the list itself, then we would want
`--quiet` to suppress *those*, I guess.

Others might disagree violently with this, but I wonder whether it makes
sense to add a test to verify that, e.g., `git worktree move --quiet` is
quiet. Such a test would demonstrate that this is your intention, i.e.,
anyone digging into a report on "why does git worktree foo not respect
--quiet?" would be 100% convinced that your intention back in 2018 really
was to respect it everywhere. It seems wasteful to add such a test for
all other modes, but maybe you can identify one which you think has a
higher risk of learning to output some feedback in the future.

To be clear, it is fine for you to disagree with the above! :-)

> }
> -
> -   print_preparing_worktree_line(opts.detach, branch, new_branch, 
> !!new_branch_force);
> +   if (!opts.quiet)
> +   print_preparing_worktree_line(opts.detach, branch, 
> new_branch, !!new_branch_force);

I think that empty line should be kept. Probably not worth a reroll.

Good work!

[1] 
https://public-inbox.org/git/capig+cs-b2yl2felrzs+jw-o5frd1g8kqak7j1qx5prp0oj...@mail.gmail.com/

Martin

Re: [PATCH 10/10] fetch: retry fetching submodules if sha1 were not fetched

[Martin Ågren]

On 9 August 2018 at 00:17, Stefan Beller  wrote:
> Currently when git-fetch is asked to recurse into submodules, it dispatches
> a plain "git-fetch -C " (and some submodule related options
> such as prefix and recusing strategy, but) without any information of the
> remote or the tip that should be fetched.
>
> This works surprisingly well in some workflows (such as using submodules
> as a third party library), while not so well in other scenarios, such
> as in a Gerrit topic-based workflow, that can tie together changes
> (potentially across repositories) on the server side. One of the parts
> of such a Gerrit workflow is to download a change when wanting to examine
> it, and you'd want to have its submodule changes that are in the same
> topic downloaded as well. However these submodule changes reside in their
> own repository in their on ref (refs/changes/).

s/on/own/

> Retry fetching a submodule if the object id that the superproject points
> to, cannot be found.
>
> Note: This is an RFC and doesn't support fetching to FETCH_HEAD yet, but
> only into a local branch. To make fetching into FETCH_HEAD work, we need
> some refactoring in builtin/fetch.c to adjust the calls to
> 'check_for_new_submodule_commits'.
>
> Signed-off-by: Stefan Beller 
> ---

> diff --git a/submodule.c b/submodule.c
> index ec7ea6f8c2d..6cbd0b1a470 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1127,6 +1127,7 @@ struct submodule_parallel_fetch {
> int result;
>
> struct string_list changed_submodule_names;
> +   struct string_list retry;
>  };
>  #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, 
> STRING_LIST_INIT_DUP }

`retry` will effectively be `STRING_LIST_INIT_NODUP`, but making that
explicit would be better and the next addition to the struct would be
easier to get right.

> +retry_next:
> +   retry_it = string_list_pop(>retry);
> +   if (retry_it) {
> +   struct strbuf submodule_prefix = STRBUF_INIT;
> +   const struct submodule *sub =
> +   submodule_from_name(spf->r,
> +   _oid,
> +   retry_it->string);
> +
> +   child_process_init(cp);
> +   cp->dir = get_submodule_git_dir(spf->r, sub->path);
> +   if (!cp->dir)
> +   goto retry_next;

So here you just drop the string list item. Since it's NODUP, and since
the `util` pointers are owned elsewhere(?), this seems fine. Other uses
of `string_list_pop()` might not be so straightforward.

Just a thought, but rather than pop+if+goto, maybe

while ((retry_it = )) {
...
if (!cp->dir) continue;
...
return 1;
}

I haven't commented on any of the submodule stuff, which is probably
where you'd be most interested in comments. I don't use submodules, nor
do I know the code that runs them.. I guess my comments are more "if
those who know something about submodules find this series worthwhile,
you might want to consider my comments as well".

Martin

Re: [PATCH 03/10] sha1-array: provide oid_array_remove_if

[Martin Ågren]

On 9 August 2018 at 00:17, Stefan Beller  wrote:
> +int oid_array_remove_if(struct oid_array *array,
> +   for_each_oid_fn fn,
> +   void *data)
> +{
> +   int i, j;
> +   char *to_remove = xcalloc(array->nr, sizeof(char));

Do you really need this scratch space? Let's see..

> +   /* No oid_array_sort() here! See the api-oid-array.txt docs! */
> +
> +   for (i = 0; i < array->nr; i++) {
> +   int ret = fn(array->oid + i, data);
> +   if (ret)
> +   to_remove[i] = 1;
> +   }
> +
> +   i = 0, j = 0;
> +   while (i < array->nr && j < array->nr) {
> +   while (i < array->nr && !to_remove[i])
> +   i++;
> +   /* i at first marked for deletion or out */
> +   if (j < i)
> +   j = i;
> +   while (j < array->nr && to_remove[j])
> +   j++;
> +   /* j > i; j at first valid after first deletion range or out 
> */
> +   if (i < array->nr && j < array->nr)
> +   oidcpy(>oid[i], >oid[j]);
> +   else if (i >= array->nr)
> +   assert(j >= array->nr);
> +   /* no pruning happened, keep original array->nr */
> +   else if (j >= array->nr)
> +   array->nr = i;
> +   }
> +
> +   free(to_remove);
> +
> +   return 0;
> +}

I can't entirely follow this index-fiddling, but then I haven't had my
morning coffee yet, so please forgive me if this is nonsense. Would it
suffice to let i point out where to place items (starting at the first
item not to keep) and j where to take them from (i.e., the items to
keep, after the initial run)?

> diff --git a/sha1-array.h b/sha1-array.h
> index 232bf950172..151c7ad7f30 100644
> --- a/sha1-array.h
> +++ b/sha1-array.h
> @@ -22,5 +22,8 @@ int oid_array_for_each(struct oid_array *array,
>  int oid_array_for_each_unique(struct oid_array *array,
>   for_each_oid_fn fn,
>   void *data);
> +int oid_array_remove_if(struct oid_array *array,
> +   for_each_oid_fn fn,
> +   void *data);

Maybe some documentation here, but this seems to be following suit. ;-)

Martin

Re: [PATCH 02/10] string-list.h: add string_list_pop function.

[Martin Ågren]

On 9 August 2018 at 00:17, Stefan Beller  wrote:
> A string list can be used as a stack, but should we? A later patch shows
> how useful this will be.
>
> Signed-off-by: Stefan Beller 
> ---
>  string-list.c | 8 
>  string-list.h | 6 ++
>  2 files changed, 14 insertions(+)
>
> diff --git a/string-list.c b/string-list.c
> index 9f651bb4294..ea80afc8a0c 100644
> --- a/string-list.c
> +++ b/string-list.c
> @@ -80,6 +80,14 @@ void string_list_remove(struct string_list *list, const 
> char *string,
> }
>  }
>
> +struct string_list_item *string_list_pop(struct string_list *list)
> +{
> +   if (list->nr == 0)
> +   return 0;

return NULL, not 0.

> +
> +   return >items[--list->nr];
> +}
> +

> +/**
> + * Returns the last item inserted and removes it from the list.
> + * If the list is empty, return NULL.
> + */
> +struct string_list_item *string_list_pop(struct string_list *list);
> +

The memory ownership is now with the caller. That is, the caller needs
to check/know `list->strdup_strings` and know `free_util` to be able to
properly free all memory.

OTOH, the pointer returned by this function is only guaranteed to be
valid until you start inserting into the list (well, you can do one
insertion per pop without worrying, but that's quite detailed
implementation knowledge).

Maybe these caveats should be documented, or is a hint that this
`_pop()` is not so nice to have after all, but let's see what happens in
the later patches...

Martin

Re: [PATCH] worktree: add --quiet option

[Martin Ågren]

Hi Elia

On 7 August 2018 at 15:21, Elia Pinto  wrote:
> Add the '--quiet' option to git worktree add,
> as for the other git commands.
>
> Signed-off-by: Elia Pinto 
> ---
>  Documentation/git-worktree.txt |  4 +++-
>  builtin/worktree.c | 11 +--
>  2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index 9c26be40f..508cde55c 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -115,7 +115,9 @@ Unlock a working tree, allowing it to be pruned, moved or 
> deleted.
>
>  OPTIONS
>  ---
> -

Grepping through Documentation/, it is clear that we sometimes have a
blank line here, sometimes not. I'm not sure what to make of that.

> +-q::
> +--quiet::
> +   With 'add', suppress feedback messages.
>  -f::

But I do think that for consistency, we'd prefer a blank line before `-f::`.

Both the commit message and this documentation makes me wonder if this
focuses on "add" because it's the only subcommand where `--quiet` makes
sense, conceptually, or because this is where you happen to need it
personally, or due to some other $reason. Could you say something more
about this?

I'm not a worktree power-user, so please forgive my ignorance...

> @@ -315,6 +316,9 @@ static int add_worktree(const char *path, const char 
> *refname,
> cp.argv = NULL;
> argv_array_clear();
> argv_array_pushl(, "reset", "--hard", NULL);
> +   if (opts->quiet)
> +   argv_array_push(, "--quiet");
> +   printf("%s\n","soo qia");

This last line looks like debug cruft.

> @@ -437,6 +441,7 @@ static int add(int ac, const char **av, const char 
> *prefix)
> OPT_BOOL(0, "detach", , N_("detach HEAD at named 
> commit")),
> OPT_BOOL(0, "checkout", , N_("populate the new 
> working tree")),
> OPT_BOOL(0, "lock", _locked, N_("keep the new 
> working tree locked")),
> +   OPT__QUIET(, N_("suppress progress reporting")),

This matches other users. Good.

I did some simple testing and this appears to be quite quiet, modulo
the "soo qia" that I already mentioned. Could you add a test to
demonstrate the quietness and to keep it from regressing? Something like
`git worktree add ../foo >out && test_must_be_empty out" in e.g.,
t2025-worktree-add.sh might do the trick (capture stderr as well?).

Hope this helps
Martin

Re: [PATCH 0/2] Simple fixes to t7406

[Martin Ågren]

On 6 August 2018 at 17:25, Elijah Newren  wrote:
> Changes since v1:
>   - Simplify multiple tests using diff --name-only instead of diff --raw;
> these tests are only interested in which file was modified anyway.
> (Suggested by Junio)
>   - Avoid putting git commands upstream of a pipe, since we don't run under
> set -o pipefail (Suggested by Eric)
>   - Avoid using test_must_fail for system binaries (Pointed out by
> both Eric and Junio)
>
> Elijah Newren (2):

I'm not sure what's up with the count of 2 vs 3.

>   t7406: simplify by using diff --name-only instead of diff --raw
>   t7406: avoid having git commands upstream of a pipe
>   t7406: make a test_must_fail call fail for the right reason

The subject of the final patch doesn't quite match its content anymore.
The problematic test_must_fail is dropped, so it can no longer fail.
Maybe something like "t7406: fix call that was failing for the wrong
reason", or just s/test_must_fail// in what you have.

Martin

Re: [PATCH 4/8] gpg-interface: introduce an abstraction for multiple gpg formats

[Martin Ågren]

Hi Henning,

On 3 July 2018 at 14:38, Henning Schild  wrote:
> Create a struct that holds the format details for the supported formats.
> At the moment that is still just "PGP". This commit prepares for the
> introduction of more formats, that might use other programs and match
> other signatures.
>
> Signed-off-by: Henning Schild 

Welcome to the mailing list! :-)

I'll just comment on a few thoughts I had while skimming this.

>  static char *configured_signing_key;
> -static const char *gpg_format = "PGP";
> -static const char *gpg_program = "gpg";
> +struct gpg_format_data {
> +   const char *format;
> +   const char *program;
> +   const char *extra_args_verify[1];
> +   const char *sigs[2];
> +};
>
>  #define PGP_SIGNATURE "-BEGIN PGP SIGNATURE-"
>  #define PGP_MESSAGE "-BEGIN PGP MESSAGE-"
>
> +enum gpgformats { PGP_FMT };
> +struct gpg_format_data gpg_formats[] = {
> +   { .format = "PGP", .program = "gpg",
> + .extra_args_verify = { "--keyid-format=long", },
> + .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },
> +   },
> +};

I think those trailing commas are ok now, but I'm not sure...

I had the same thought about designated initializers. Those should be ok
now, c.f. cbc0f81d96 (strbuf: use designated initializers in STRBUF_INIT,
2017-07-10) and a73b3680c4 (Add and use generic name->id mapping code
for color slot parsing, 2018-05-26).

> +static const char *gpg_format = "PGP";
> +
> +static struct gpg_format_data *get_format_data(void)
> +{
> +   int i;
> +   for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> +   if (!strcmp(gpg_formats[i].format, gpg_format))
> +   return gpg_formats + i;
> +   assert(0);

This might be better written as `BUG("bad gpg_format '%s'",
gpg_format);` or something like that.

(It's not supposed to be triggered, not even by invalid data from the
user, right?)

> if (!strcmp(var, "gpg.format")) {
> -   if (!strcmp(value, "PGP"))

This line was added in patch 3. It errors out precisely when gpg.format
is "PGP", no? That this doesn't break the whole series is explained by
1) it being removed in this patch 4, and 2) there being no tests. It
makes me wonder if something like patch 5 (test gpg.format) could be
part of patch 3, both with negative ("= malformed") and positive ("=
PGP") tests?

> +   j = 0;
> +   for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> +   if (!strcmp(value, gpg_formats[i].format)) {
> +   j++;
> +   break;
> +   }
> +   if (!j)
> return error("malformed value for %s: %s", var, 
> value);

`if (i == ARRAY_SIZE(gpg_formats))` and drop `j`?

Or check whether `get_format_data()` returns NULL? Hmm, well you can't,
since it takes its "input" from a global variable...

If you want to keep that global nature, the duplication of search-logic
could perhaps be avoided by having a helper function for returning the
index of a gpg_format (or -1).

> return git_config_string(_format, var, value);
> }

Martin

Re: [PATCH 20/20] abbrev: add a core.validateAbbrev setting

[Martin Ågren]

On 9 June 2018 at 00:41, Ævar Arnfjörð Bjarmason  wrote:

> Instead of trying really hard to find an unambiguous SHA-1 we can with
> core.validateAbbrev=false, and preferably combined with the new
> support for relative core.abbrev values (such as +4) unconditionally
> print a short SHA-1 without doing any disambiguation check. I.e. it

This first paragraph read weirdly the first time. On the second attempt
I knew how to structure it and got it right. It might be easier to read
if the part about core.appreb=+4 were in a separate second sentence.

That last "it" is "the combination of these two configs", right?

> allows for picking a trade-off between performance, and the odds that
> future or remote (or current and local) short SHA-1 will be ambiguous.

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index abf07be7b6..df31d1351f 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -925,6 +925,49 @@ means to add or subtract N characters from the SHA-1 
> that Git would
>  otherwise print, this allows for producing more future-proof SHA-1s
>  for use within a given project, while adjusting the value for the
>  current approximate number of objects.
> ++
> +This is especially useful in combination with the
> +`core.validateAbbrev` setting, or to get more future-proof hashes to
> +reference in the future in a repository whose number of objects is
> +expected to grow.

Maybe s/validateAbbrev/validateAbbrev = false/?

> +
> +core.validateAbbrev::
> +   If set to false (true by default) don't do any validation when
> +   printing abbreviated object names to see if they're really
> +   unique. This makes printing objects more performant at the
> +   cost of potentially printing object names that aren't unique
> +   within the current repository.

Good. I understand why I'd want to use it, and why not.

> ++
> +When printing abbreviated object names Git needs to look through the
> +local object store. This is an `O(log N)` operation assuming all the
> +objects are in a single pack file, but `X * O(log N)` given `X` pack
> +files, which can get expensive on some larger repositories.

This might be very close to too much information.

> ++
> +This setting changes that to `O(1)`, but with the trade-off that
> +depending on the value of `core.abbrev` we may be printing abbreviated
> +hashes that collide. Too see how likely this is, try running:
> ++
> +---
> +git log --all --pretty=format:%h --abbrev=4 | perl -nE 'chomp; say length' | 
> sort | uniq -c | sort -nr
> +---
> ++
> +This shows how many commits were found at each abbreviation length. On
> +linux.git in June 2018 this shows a bit more than 750,000 commits,
> +with just 4 needing 11 characters to be fully abbreviated, and the
> +default heuristic picks a length of 12.

These last few paragraphs seem like too much to me.

> ++
> +Even without `core.validateAbbrev=false` the results abbreviation
> +already a bit of a probability game. They're guaranteed at the moment
> +of generation, but as more objects are added, ambiguities may be
> +introduced. Likewise, what's unambiguous for you may not be for
> +somebody else you're communicating with, if they have their own clone.

This seems more useful.

> ++
> +Therefore the default of `core.validateAbbrev=true` may not save you
> +in practice if you're sharing the SHA-1 or noting it now to use after
> +a `git fetch`. You may be better off setting `core.abbrev` to
> +e.g. `+2` to add 2 extra characters to the SHA-1, and possibly combine
> +that with `core.validateAbbrev=false` to get a reasonable trade-off
> +between safety and performance.

Makes sense. As before, I'd suggest s/SHA-1/object ID/.

I do wonder if this documentation could be a bit less verbose without
sacrificing too much correctness and clarity. No brilliant suggestions
on how to do that, sorry.

Martin

Re: [PATCH 19/20] abbrev: support relative abbrev values

[Martin Ågren]

On 9 June 2018 at 00:41, Ævar Arnfjörð Bjarmason  wrote:
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index ab641bf5a9..abf07be7b6 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -919,6 +919,12 @@ core.abbrev::
> in your repository, which hopefully is enough for
> abbreviated object names to stay unique for some time.
> The minimum length is 4.
> ++
> +This can also be set to relative values such as `+2` or `-2`, which
> +means to add or subtract N characters from the SHA-1 that Git would
> +otherwise print, this allows for producing more future-proof SHA-1s
> +for use within a given project, while adjusting the value for the
> +current approximate number of objects.

How about s/, this/. This/ to break it up a little? Also, you write "+2"
and "-2" but then "N". Unify it?

Also, I'd suggest s/SHA-1/object ID/ to be future-proof.

> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index f466600972..f1114a7b8d 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -384,6 +384,9 @@ endif::git-format-patch[]
> independent of the `--full-index` option above, which controls
> the diff-patch output format.  Non default number of
> digits can be specified with `--abbrev=`.
> ++
> +Can also be set to a relative value, see `core.abbrev` in
> +linkgit:git-diff[1].

Good. You then add this paragraph to lots of other places...

> diff --git a/config.c b/config.c
> index 12f762ad92..cd95c6bdfb 100644
> --- a/config.c
> +++ b/config.c
> @@ -1151,6 +1151,17 @@ static int git_default_core_config(const char *var, 
> const char *value)
> return config_error_nonbool(var);
> if (!strcasecmp(value, "auto")) {
> default_abbrev = -1;
> +   } else if (*value == '+' || *value == '-') {
> +   int relative = git_config_int(var, value);
> +   if (relative == 0)
> +   die(_("bad core.abbrev value %s. "

Trailing period? Same below.

> + "relative values must be non-zero"),
> +   value);
> +   if (abs(relative) > GIT_SHA1_HEXSZ)
> +   die(_("bad core.abbrev value %s. "
> + "impossibly out of range"),
> +   value);
> +   default_abbrev_relative = relative;
> } else {
> int abbrev = git_config_int(var, value);
> if (abbrev < minimum_abbrev || abbrev > 40)
> diff --git a/diff.c b/diff.c
> index e0141cfbc0..f7861b8472 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4801,16 +4801,28 @@ int diff_opt_parse(struct diff_options *options,
> else if (!strcmp(arg, "--abbrev"))
> options->abbrev = DEFAULT_ABBREV;
> else if (skip_prefix(arg, "--abbrev=", )) {
> +   int v;
> char *end;
> if (!strcmp(arg, ""))
> die("--abbrev expects a value, got '%s'", arg);
> -   options->abbrev = strtoul(arg, , 10);
> +   v = strtoul(arg, , 10);
> if (*end)
> die("--abbrev expects a numerical value, got '%s'", 
> arg);
> -   if (options->abbrev < MINIMUM_ABBREV) {
> +   if (*arg == '+' || *arg == '-') {
> +   if (v == 0) {
> +   die("relative abbrev must be non-zero");
> +   } else if (abs(v) > the_hash_algo->hexsz) {
> +   die("relative abbrev impossibly out of 
> range");
> +   } else {
> +   default_abbrev_relative = v;
> +   options->abbrev = DEFAULT_ABBREV;
> +   }
> +   } else if (v < MINIMUM_ABBREV) {
> options->abbrev = MINIMUM_ABBREV;
> -   } else if (the_hash_algo->hexsz < options->abbrev) {
> +   } else if (the_hash_algo->hexsz < v) {
> options->abbrev = the_hash_algo->hexsz;
> +   } else {
> +   options->abbrev = v;

I've cut out a few instances of more-or-less repeated code. Any
possibility of extracting this into a helper? Maybe after you've done
the preparatory work of unifying these sites. Or as part of it, i.e.,
"let's switch this spot to use the helper; that makes it stricter in
this-and-that sense".

These can't all be entirely unified, I guess, but maybe "mostly"?

Martin

Re: [PATCH 17/20] abbrev: unify the handling of empty values

[Martin Ågren]

On 9 June 2018 at 16:24, Martin Ågren  wrote:
> On 9 June 2018 at 00:41, Ævar Arnfjörð Bjarmason  wrote:
>> For no good reason the --abbrev= command-line option was less strict
>> than the core.abbrev config option, which came down to the latter
>> using git_config_int() which rejects an empty string, but the rest of
>> the parsing using strtoul() which will convert it to 0.
>
> It will still be less strict in that it accepts trailing garbage, e.g.,
> `--abbrev=7a`. Probably ok to leave it at that in this series, but
> possibly useful to mention here that this only makes these options "less
> differently strict".

Hmpf, please ignore. That's what I get for looking at a few patches,
taking a break, picking it up again and completely forgetting what's
going on...

Re: [PATCH 17/20] abbrev: unify the handling of empty values

[Martin Ågren]

On 9 June 2018 at 00:41, Ævar Arnfjörð Bjarmason  wrote:
> For no good reason the --abbrev= command-line option was less strict
> than the core.abbrev config option, which came down to the latter
> using git_config_int() which rejects an empty string, but the rest of
> the parsing using strtoul() which will convert it to 0.

It will still be less strict in that it accepts trailing garbage, e.g.,
`--abbrev=7a`. Probably ok to leave it at that in this series, but
possibly useful to mention here that this only makes these options "less
differently strict".

I also notice that the documentation of `--abbrev` starts with "Instead
of showing the full 40-byte hexadecimal object name" which doesn't seem
right. I get much shorter IDs and I can't see that I'd have any
configuration causing this. Anyway, that might not be anything this
series needs to do anything about.

> +   if (!strcmp(arg, ""))
> +   die("--abbrev expects a value, got '%s'", arg);

> +   if (!strcmp(arg, ""))
> +   return opterror(opt, "expects a value", 0);

> +   if (!strcmp(optarg, ""))
> +   die("--abbrev expects a value, got '%s'", optarg);

> +   test_must_fail git branch -v --abbrev= 2>stderr &&
> +   test_i18ngrep "expects a value" stderr &&

> +   test_must_fail git log --abbrev= -1 --pretty=format:%h 2>stderr &&
> +   test_i18ngrep "expects a value" stderr &&

> +   test_must_fail git diff --raw --abbrev= HEAD~ 2>stderr &&
> +   test_i18ngrep "expects a value" stderr &&

Mismatch re i18n-ed or not between implementations and tests?

Martin

Re: [PATCH 09/20] abbrev tests: test for "git-log" behavior

[Martin Ågren]

On 9 June 2018 at 11:56, Ævar Arnfjörð Bjarmason  wrote:
>
> On Sat, Jun 09 2018, Martin Ågren wrote:
>
>> On 9 June 2018 at 00:41, Ævar Arnfjörð Bjarmason  wrote:
>>> The "log" family of commands does its own parsing for --abbrev in
>>> revision.c, so having dedicated tests for it makes sense.
>>
>>> +for i in $(test_seq 4 40)
>>
>> I've just been skimming so might have missed something, but I see
>> several instances of this construct, and I wonder what this brute-force
>> approach really buys us. An alternative would be, e.g., "for i in 4 23
>> 40". That is, min/max and some arbitrary number in between (odd because
>> the others are even).
>>
>> Of course, we might have a bug which magically happens for the number 9,
>> but I'd expect us to test for that only if we have some reason to
>> believe that number 9 is indeed magical.
>
> Good point, I'll change this in v2, or at least guard it with
> EXPENSIVE. I hacked it up like this while exhaustively testing things
> during development, and discovered some edge cases (e.g. "0" is special
> sometimes).

Ah, "useful during hacking" explains why you did it like this. Of your
two approaches, I'd probably favour "make it cheaper" over "mark it as
EXPENSIVE". Nothing I feel strongly about.

>> Also, 40 is of course tied to SHA-1. You could perhaps define a variable
>> at the top of this file to simplify a future generalization. (Same for
>> 39/41 which are related to 40.)
>
> I forgot to note this in the commit message, but I intentionally didn't
> guard this test with the SHA1 prereq, there's nothing per-se specific to
> SHA-1 here, it's not a given that whatever our NewHash is that we won't
> use 40 characters, and the rest of the magic constants like 4 and 7 is
> something we're likely to retain with NewHash.

I'd tend to agree about not marking this SHA1.

> Although maybe we should expose GIT_SHA1_HEXSZ to the test suite.

It seems like brian's "test_translate"-approach [1] would be a good
choice of tool for this. That is, you'd just define something at the top
of this file for now, then once that tool is in place, a one-line change
could get "hexsz" from `test_translate` instead.

[1] 
https://public-inbox.org/git/20180604235229.279814-2-sand...@crustytoothpaste.net/

Martin

Re: [PATCH] fsck: avoid looking at NULL blob->object

[Martin Ågren]

On 9 June 2018 at 10:32, Jeff King  wrote:
> Except it _does_ do one non-trivial thing, which is call the
> report() function, which wants us to pass a pointer to a
> "struct object". Which we don't have (we have only a "struct
> object_id"). So we erroneously passed the NULL object, which

s/passed/dereferenced/? Probably doesn't affect the fix though.

> ends up segfaulting.

> blob = lookup_blob(oid);
> if (!blob) {
> -   ret |= report(options, >object,
> +   struct object *obj = lookup_unknown_object(oid->hash);
> +   ret |= report(options, obj,
>   FSCK_MSG_GITMODULES_BLOB,
>   "non-blob found at .gitmodules");

Re: [PATCH 09/20] abbrev tests: test for "git-log" behavior

[Martin Ågren]

On 9 June 2018 at 00:41, Ævar Arnfjörð Bjarmason  wrote:
> The "log" family of commands does its own parsing for --abbrev in
> revision.c, so having dedicated tests for it makes sense.

> +for i in $(test_seq 4 40)

I've just been skimming so might have missed something, but I see
several instances of this construct, and I wonder what this brute-force
approach really buys us. An alternative would be, e.g., "for i in 4 23
40". That is, min/max and some arbitrary number in between (odd because
the others are even).

Of course, we might have a bug which magically happens for the number 9,
but I'd expect us to test for that only if we have some reason to
believe that number 9 is indeed magical.

Also, 40 is of course tied to SHA-1. You could perhaps define a variable
at the top of this file to simplify a future generalization. (Same for
39/41 which are related to 40.)

Martin

Re: [PATCH 0/3] refspec: refactor & fix free() behavior

[Martin Ågren]

On 5 June 2018 at 21:58, Brandon Williams  wrote:
> On 06/05, Ævar Arnfjörð Bjarmason wrote:
>> Since Martin & Brandon both liked this direction I've fixed it
>> up.
>>
>> Martin: I didn't want to be the author of the actual fix for the bug
>> you found, so I rewrote your commit in 3/3. The diff is different, and
>> I slightly modified the 3rd paragraph of the commit message & added my
>> sign-off, but otherwise it's the same.
>
> Thanks for writing up a proper patch series for this fix.  I liked
> breaking up your diff into two different patches to make it clear that
> all callers of refpsec_item_init relying on dieing.

Me too.

>> Martin Ågren (1):
>>   refspec: initalize `refspec_item` in `valid_fetch_refspec()`

I was a bit surprised at first that this wasn't a "while at it" in the
second patch, but on second thought, it does make sense to keep this
separate. Thanks for picking this up and polishing it.

Just noticed: s/initalize/initialize/. That would be my fault...

Martin

Re: [PATCH] refspec: initalize `refspec_item` in `valid_fetch_refspec()`

[Martin Ågren]

On 4 June 2018 at 23:55, Ævar Arnfjörð Bjarmason  wrote:

> I think this makes more sense instead of this fix:
[...]
> -void refspec_item_init(struct refspec_item *item, const char *refspec, int 
> fetch)
> +int refspec_item_init(struct refspec_item *item, const char *refspec, int 
> fetch)
>  {
> memset(item, 0, sizeof(*item));
> +   int ret = parse_refspec(item, refspec, fetch);
> +   return ret;
> +}

Nit: Declaration after statement. Intriguingly, you do use a `ret`
variable, so I suspect you sort of thought about it but left the final
cleaning up for later. ;-)

> -void refspec_item_init(struct refspec_item *item, const char *refspec, int 
> fetch);
> +int refspec_item_init(struct refspec_item *item, const char *refspec, int 
> fetch);
> +void refspec_item_init_or_die(struct refspec_item *item, const char 
> *refspec, int fetch);
>  void refspec_item_clear(struct refspec_item *item);
>  void refspec_init(struct refspec *rs, int fetch);
>  void refspec_append(struct refspec *rs, const char *refspec);
>
> I.e. let's fix the bug, but with this admittedly more verbose fix we're
> left with exactly two memset() in refspec.c, one for each type of struct
> that's initialized by the API.
>
> The reason this is difficult now is because the current API conflates
> the init function with an init_or_die, which is what most callers want,
> so let's just split those concerns up. Then we're left with one init
> function that does the memset.

I didn't test this, but it looks sane in general IMHO, and should fix
the issue I saw. Should we be worried that someone might already depend
on `refspec_item_init()` to die, and we'll silently break that
assumption?

Martin

[PATCH] refspec: initalize `refspec_item` in `valid_fetch_refspec()`

[Martin Ågren]

We allocate a `struct refspec_item` on the stack without initializing
it. In particular, its `dst` and `src` members will contain some random
data from the stack. When we later call `refspec_item_clear()`, it will
call `free()` on those pointers. So if the call to `parse_refspec()` did
not assign to them, we will be freeing some random "pointers". This is
undefined behavior.

To the best of my understanding, this cannot currently be triggered by
user-provided data. And for what it's worth, the test-suite does not
trigger this with SANITIZE=address. It can be provoked by calling
`valid_fetch_refspec(":*")`.

Zero the struct, as is done in other users of `struct refspec_item`.

Signed-off-by: Martin Ågren 
---
I found some time to look into this. It does not seem to be a
user-visible bug, so not particularly critical.

 refspec.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/refspec.c b/refspec.c
index ada7854f7a..7dd7e361e5 100644
--- a/refspec.c
+++ b/refspec.c
@@ -189,7 +189,10 @@ void refspec_clear(struct refspec *rs)
 int valid_fetch_refspec(const char *fetch_refspec_str)
 {
struct refspec_item refspec;
-   int ret = parse_refspec(, fetch_refspec_str, REFSPEC_FETCH);
+   int ret;
+
+   memset(, 0, sizeof(refspec));
+   ret = parse_refspec(, fetch_refspec_str, REFSPEC_FETCH);
refspec_item_clear();
return ret;
 }
-- 
2.18.0.rc0.43.gb85e7bcbff

Re: [PATCH v2 05/36] refspec: convert valid_fetch_refspec to use parse_refspec

[Martin Ågren]

Hi Brandon,

On 17 May 2018 at 00:57, Brandon Williams  wrote:
> Convert 'valid_fetch_refspec()' to use the new 'parse_refspec()'
> function to only parse a single refspec and eliminate an allocation.
>
> Signed-off-by: Brandon Williams 
> ---
>  refspec.c | 17 -
>  refspec.h |  3 ++-
>  2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/refspec.c b/refspec.c
> index af9d0d4b3..ab37b5ba1 100644
> --- a/refspec.c
> +++ b/refspec.c
> @@ -146,15 +146,6 @@ static struct refspec_item *parse_refspec_internal(int 
> nr_refspec, const char **
> die("Invalid refspec '%s'", refspec[i]);
>  }
>
> -int valid_fetch_refspec(const char *fetch_refspec_str)
> -{
> -   struct refspec_item *refspec;
> -
> -   refspec = parse_refspec_internal(1, _refspec_str, 1, 1);
> -   free_refspec(1, refspec);
> -   return !!refspec;
> -}
> -
>  struct refspec_item *parse_fetch_refspec(int nr_refspec, const char 
> **refspec)
>  {
> return parse_refspec_internal(nr_refspec, refspec, 1, 0);
> @@ -242,3 +233,11 @@ void refspec_clear(struct refspec *rs)
>
> rs->fetch = 0;
>  }
> +
> +int valid_fetch_refspec(const char *fetch_refspec_str)
> +{
> +   struct refspec_item refspec;
> +   int ret = parse_refspec(, fetch_refspec_str, REFSPEC_FETCH);
> +   refspec_item_clear();
> +   return ret;
> +}

My compiler warned about this function. The `dst` and `src` pointers
will equal some random data on the stack, then they may or may not be
assigned to, then we will call `free()` on them.

At least I *think* that we "may or may not" assign to them. I don't have
much or any time to really dig into this right now unfortunately.

I suppose this could use a REFSPEC_ITEM_INIT, or a memset inside
`parse_refspec()`, but I am very unfamiliar with this code.

Martin

Re: [RFC PATCH 1/3] usage: extract `prefix_suffix_lines()` from `advise()`

[Martin Ågren]

On 30 May 2018 at 08:00, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> Jeff King  writes:
>>
>>> But most importantly, it means we could eventually colorize errors, too,
>>> where we are not allowed to allocate.
>>>
>>> So perhaps:
>>>
>>>   void report_lines(FILE *out,
>>> const char *color, const char *color_reset,
>>>  const char *prefix, const char *msg);
>>>
>>> or something?
>>
>> Sounds good to me.  And if you hate the repeated "error:" prefix
>> that makes the prefix on the second and subsequent lines included in
>> cutting and pasting, we could use the two-prefix idea elsewhere in
>> the thread, too.

(That also gets rid of the minor strangeness of my series to introduce an
overly broad `suffix` and use it only for resetting color, not actually
for giving any textual suffix.)

> If we do not want duplicate prefix, another approach is to leave
> everything including alignment up to the translators.  For example,
>
> error(_("the first line of error.\n"
> "   ... and the second."));
>
[...]
> with these entries for that hypothetical "shout in caps" language.
>
> msgid "error: "
> msgstr "ERRORX: "
> msgid "the first line of error.\n   ... and the second."
> msgstr"THE FIRST LINE OF ERROR.\n... AND THE SECOND."
>
> So I do not think the two-prefix idea is necessary (and if people
> prefer to have repeated prefix, that can also be done perfectly well
> within this scheme---the second and subsequent message needs to
> duplicate "error:" at the beginning, which is sort of ugly, but the
> leading spaces for alignment we see above already knows how wide
> "error:" and its translation is, so it is not that much worse
> anyway).

Thanks for lots of good input. Doing the indenting in the error does
mean that to indent properly, the translator needs to know that it is an
error, and not a warning. Or one could say that they would need to know
the amount of indentation anyway so that they can know where to wrap
optimally.

Another consequence is that if we can emit a certain string as either,
e.g., a warning or an error depending on the context, we need to address
that. Using contexts, of course. Thank you for the hint about that. (I
have not checked if we actually have such "this is a warning or an
error"-strings currently.)

Somehow it feels slightly cleaner to me, at least on first thought, to
try to decouple the indenting from the translating and line-wrapping.
But as noted above, the indenting does affect how the line-wrapping
should/may be done.

I won't have as much time as I'd like for tinkering with this the next
week, or even weeks. Hopefully when I do get around to it, I will have
processed all the very good input I have received and turn that into
something good.

Thanks,
Martin

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Martin Ågren]

On 29 May 2018 at 23:32, Jeff King  wrote:
> On Mon, May 28, 2018 at 06:25:18PM +0900, Junio C Hamano wrote:
>
>> This shows the typical effect of this series, which (I subjectively
>> think) gives us a more pleasant end-user experience.
>
> Heh, that is one of the cases that I found most ugly when I looked into
> this earlier (and in particular, because I think it makes cut-and-paste
> a little harder).
>
> More discussion in:
>
>   
> https://public-inbox.org/git/2017040758.yyfsc3r3spqpi...@sigill.intra.peff.net/
>
> and down-thread.

Thanks for the pointer. I had missed that thread entirely.

Martin

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Martin Ågren]

On 29 May 2018 at 17:50, Duy Nguyen  wrote:
> On Tue, May 29, 2018 at 6:49 AM, Martin Ågren  wrote:
>> On 28 May 2018 at 23:45, Junio C Hamano  wrote:
>>> Duy Nguyen  writes:
>>>
>>>>>> +error:   sub/added
>>>>>> +error:   sub/addedtoo
>>>>>> +error: Please move or remove them before you switch branches.
>>>>>>  Aborting
>>>>>>  EOF
>>>>>
>>>>> This shows the typical effect of this series, which (I subjectively
>>>>> think) gives us a more pleasant end-user experience.
>>>>
>>>> Also, very subjectively, I'm torn about this. To me, just one
>>>> "error/warning/fatal" at the start of the first paragraph feels much
>>>> better. If we have to somehow mark the second paragraph that "this is
>>>> also part of the error message" then it's probably better to rephrase.
>>
>> Would you feel the same about "hint: "? We already do prefix all the
>> lines there. It seems to we we should probably do the same for "hint: "
>> as for "warning: ", whatever we decide is right.
>
> It may depend on context. Let's look at the commit that introduces
> this "hint:" prefix, 38ef61cfde (advice: Introduce
> error_resolve_conflict - 2011-08-04). The example in the commit
> message shows the hint paragraph sandwiched by an error and a fatal
> one:
>
>   error: 'commit' is not possible because you have unmerged files.
>   hint: Fix them up in the work tree ...
>   hint: ...
>   fatal: Exiting because of an unresolved conflict.
>
> I think in this case (dense paragraphs of different message types) yes
> it might make sense to prefix lines with "hint:". But when there's
> only one type of message like the "error" part quoted at the top, it
> feels too verbose to have error: prefix everywhere.

Hmm, that's interesting. Deciding based on what has already been output
seems feasible, although it sounds like the potential target of infinite
tweaking of some central logic for deciding which way to go. Or, lots of
various places to try and make consistent. I am tempted towards
indenting with spaces in v2, and to leave "hint: " alone as an outlier.
(It always was one. :-/ ) I'll keep your feedback in mind.

Thanks,
Martin

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Martin Ågren]

On 29 May 2018 at 07:50, Junio C Hamano  wrote:
> Martin Ågren  writes:
>
>>>  - allow callers to align 1st prefix (e.g. "error: ") with the
>>>leading indent for the second and subsequent lines by passing the
>>>second prefix with appropriate display width.
>>
>> I suspect this second prefix would always consist of
>> strlen(first_prefix) spaces? We should be able to construct it on the
>> fly, without any need for manual counting and human mistakes.
>
> I wouldn't be so bold to claim that, given especially that we are
> talking about i18n/l10n where byte count, character count and
> display width are all different even on a terminal with fixed-width
> font.

You are of course correct. I should have my morning tea before typing.

About the _("\t")-approach that you mentioned up-thread. It would allow
a translator to adjust all the indentations for a particular language.
To be clear, what you mean is _(" " /* 9 spaces */) to align
nicely with "warning: ", which is the longest English string. Then the
translator would translate the nine spaces and all of "fatal:   " and
others to padded strings, all of the same length (not necessarily nine).
Correct?

That approach seems a bit shaky, if nothing else because we may one day
similarly want to use nine "translated" spaces in some other context. Or
maybe this is actually i18n-best-practices.

It also means that shorter prefixes are somewhat arbitrarily padded,
just because there exists a longer prefix that some other code path may
want to use. OTOH, if a "warning: " is followed by an "error:   ", both
lines/blocks would have the same indentation, which might perhaps be
(slightly) preferable.

Martin

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Martin Ågren]

On 28 May 2018 at 23:45, Junio C Hamano  wrote:
> Duy Nguyen  writes:
>
 +error:   sub/added
 +error:   sub/addedtoo
 +error: Please move or remove them before you switch branches.
  Aborting
  EOF
>>>
>>> This shows the typical effect of this series, which (I subjectively
>>> think) gives us a more pleasant end-user experience.
>>
>> Also, very subjectively, I'm torn about this. To me, just one
>> "error/warning/fatal" at the start of the first paragraph feels much
>> better. If we have to somehow mark the second paragraph that "this is
>> also part of the error message" then it's probably better to rephrase.

Would you feel the same about "hint: "? We already do prefix all the
lines there. It seems to we we should probably do the same for "hint: "
as for "warning: ", whatever we decide is right.

> I personally can go either way.  If you prefer less noisy route, we
> could change the function signature of vreportf() to take a prefix
> for the first line and another prefix for the remaining lines and
> pass that through down to the "split and print with prefix" helper.
>
> That way, we can
>
>  - allow callers to align 1st prefix (e.g. "error: ") with the
>leading indent for the second and subsequent lines by passing the
>second prefix with appropriate display width.

I suspect this second prefix would always consist of
strlen(first_prefix) spaces? We should be able to construct it on the
fly, without any need for manual counting and human mistakes.

>
>  - allow translators to grow or shrink number of lines a given
>message takes, and to decide where in the translated string to
>wrap lines.
>
> Even though step 3/3 may become a bit awkward (the second prefix
> would most likely be only whitespace, and you'd need to write
> something silly like _("\t")), we can still keep the alignment if we
> wanted to.

Thanks both for your comments. I'll see what I can come up with.

Martin

[RFC PATCH 3/3] usage: translate the "error: "-prefix and others

[Martin Ågren]

Translate the "error: ", "fatal: ", "usage: " and "warning: " prefixes
that we use for reporting that kind of information.

Do not translate "BUG: ". We tend to prefer the messages themselves to
be non-translated (and they're not supposed to ever appear anyway) so it
makes sense to let the prefix be nontranslated, too.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
This change breaks several tests under GETTEXT_POISON, e.g. t6030 which
does this:

git bisect skip > my_bisect_log.txt 2>&1 &&
grep "warning" my_bisect_log.txt

 usage.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/usage.c b/usage.c
index 6a5669922f..7709cb22e7 100644
--- a/usage.c
+++ b/usage.c
@@ -39,24 +39,24 @@ void vreportf(const char *prefix, const char *err, va_list 
params)
 
 static NORETURN void usage_builtin(const char *err, va_list params)
 {
-   vreportf("usage: ", err, params);
+   vreportf(_("usage: "), err, params);
exit(129);
 }
 
 static NORETURN void die_builtin(const char *err, va_list params)
 {
-   vreportf("fatal: ", err, params);
+   vreportf(_("fatal: "), err, params);
exit(128);
 }
 
 static void error_builtin(const char *err, va_list params)
 {
-   vreportf("error: ", err, params);
+   vreportf(_("error: "), err, params);
 }
 
 static void warn_builtin(const char *warn, va_list params)
 {
-   vreportf("warning: ", warn, params);
+   vreportf(_("warning: "), warn, params);
 }
 
 static int die_is_recursing_builtin(void)
-- 
2.17.0.1181.g093e983b05

[RFC PATCH 1/3] usage: extract `prefix_suffix_lines()` from `advise()`

[Martin Ågren]

advice.c contains a useful code snippet which takes a multi-line string
and prints the lines, prefixing and suffixing each line with two
constant strings. This was originally added in 23cb5bf3b3 (i18n of
multi-line advice messages, 2011-12-22) to produce such output:

hint: some multi-line advice
hint: prefixed with "hint: "

The prefix is actually colored after 960786e761 (push: colorize errors,
2018-04-21) and each line has a suffix for resetting the color.

The next commit will teach the same "prefix all the lines"-trick to the
code that produces, e.g., "warning: "-messages. In preparation for that,
extract the code for printing the individual lines and expose it through
git-compat-util.h.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
I'm open for suggestions on the naming of `prefix_suffix_lines()`...

 git-compat-util.h |  8 
 advice.c  | 18 --
 usage.c   | 18 ++
 3 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index f9e4c5f9bc..23445f7ab9 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -415,6 +415,14 @@ static inline char *git_find_last_dir_sep(const char *path)
 struct strbuf;
 
 /* General helper functions */
+
+/*
+ * Write the message to the file, prefixing and suffixing
+ * each line with `prefix` resp. `suffix`.
+ */
+void prefix_suffix_lines(FILE *f, const char *prefix,
+const char *message, const char *suffix);
+
 extern void vreportf(const char *prefix, const char *err, va_list params);
 extern NORETURN void usage(const char *err);
 extern NORETURN void usagef(const char *err, ...) __attribute__((format 
(printf, 1, 2)));
diff --git a/advice.c b/advice.c
index 370a56d054..ffb29e7ef4 100644
--- a/advice.c
+++ b/advice.c
@@ -79,24 +79,22 @@ static struct {
 
 void advise(const char *advice, ...)
 {
+   struct strbuf prefix = STRBUF_INIT;
struct strbuf buf = STRBUF_INIT;
va_list params;
-   const char *cp, *np;
+
+   strbuf_addf(, _("%shint: "),
+   advise_get_color(ADVICE_COLOR_HINT));
 
va_start(params, advice);
strbuf_vaddf(, advice, params);
va_end(params);
 
-   for (cp = buf.buf; *cp; cp = np) {
-   np = strchrnul(cp, '\n');
-   fprintf(stderr, _("%shint: %.*s%s\n"),
-   advise_get_color(ADVICE_COLOR_HINT),
-   (int)(np - cp), cp,
-   advise_get_color(ADVICE_COLOR_RESET));
-   if (*np)
-   np++;
-   }
+   prefix_suffix_lines(stderr, prefix.buf, buf.buf,
+   advise_get_color(ADVICE_COLOR_RESET));
+
strbuf_release();
+   strbuf_release();
 }
 
 int git_default_advice_config(const char *var, const char *value)
diff --git a/usage.c b/usage.c
index cdd534c9df..80f9c1d14b 100644
--- a/usage.c
+++ b/usage.c
@@ -6,6 +6,24 @@
 #include "git-compat-util.h"
 #include "cache.h"
 
+void prefix_suffix_lines(FILE *f,
+const char *prefix,
+const char *message,
+const char *suffix)
+{
+   const char *cp, *np;
+
+   for (cp = message; *cp; cp = np) {
+   np = strchrnul(cp, '\n');
+   fprintf(f, "%s%.*s%s\n",
+   prefix,
+   (int)(np - cp), cp,
+   suffix);
+   if (*np)
+   np++;
+   }
+}
+
 void vreportf(const char *prefix, const char *err, va_list params)
 {
char msg[4096];
-- 
2.17.0.1181.g093e983b05

[RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Martin Ågren]

Teach `vreportf()` to prefix all lines with the given prefix, not only
the first line. This matches how "hint: " is being shown, and affects
"error: ", "fatal: ", "usage: ", "warning: " and "BUG: " (as well as any
out-of-tree and future users).

Note that we need to adjust quite a few tests as a result of this
change. All of these changes are because we obviously need to prefix
more lines in various "expect"-files -- except for one instance of a
trailing empty line that disappears with this commit (see t7609). This
is a very minor change, and arguably a good one.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
Looking back at this, I wonder if the opposite approach would be better,
that is, making `advise()` use `vreportf()` after teaching the latter
the multi-line trick.

 t/t1011-read-tree-sparse-checkout.sh |  6 ++---
 t/t1506-rev-parse-diagnosis.sh   |  2 +-
 t/t1600-index.sh |  6 ++---
 t/t3600-rm.sh| 36 -
 t/t5512-ls-remote.sh |  6 ++---
 t/t7607-merge-overwrite.sh   |  6 ++---
 t/t7609-merge-co-error-msgs.sh   | 39 ++--
 usage.c  |  2 +-
 8 files changed, 51 insertions(+), 52 deletions(-)

diff --git a/t/t1011-read-tree-sparse-checkout.sh 
b/t/t1011-read-tree-sparse-checkout.sh
index 0c6f48f302..31b0702e6c 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -243,9 +243,9 @@ test_expect_success 'print errors when failed to update 
worktree' '
test_must_fail git checkout top 2>actual &&
cat >expected <<\EOF &&
 error: The following untracked working tree files would be overwritten by 
checkout:
-   sub/added
-   sub/addedtoo
-Please move or remove them before you switch branches.
+error: sub/added
+error: sub/addedtoo
+error: Please move or remove them before you switch branches.
 Aborting
 EOF
test_i18ncmp expected actual
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index 4ee009da66..80d35087b7 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -11,7 +11,7 @@ test_did_you_mean ()
sq="'" &&
cat >expected <<-EOF &&
fatal: Path '$2$3' $4, but not ${5:-$sq$3$sq}.
-   Did you mean '$1:$2$3'${2:+ aka $sq$1:./$3$sq}?
+   fatal: Did you mean '$1:$2$3'${2:+ aka $sq$1:./$3$sq}?
EOF
test_cmp expected error
 }
diff --git a/t/t1600-index.sh b/t/t1600-index.sh
index c4422312f4..39a707c94a 100755
--- a/t/t1600-index.sh
+++ b/t/t1600-index.sh
@@ -16,7 +16,7 @@ test_expect_success 'bogus GIT_INDEX_VERSION issues warning' '
git add a 2>&1 | sed "s/[0-9]//" >actual.err &&
sed -e "s/ Z$/ /" <<-\EOF >expect.err &&
warning: GIT_INDEX_VERSION set, but the value is 
invalid.
-   Using version Z
+   warning: Using version Z
EOF
test_i18ncmp expect.err actual.err
)
@@ -30,7 +30,7 @@ test_expect_success 'out of bounds GIT_INDEX_VERSION issues 
warning' '
git add a 2>&1 | sed "s/[0-9]//" >actual.err &&
sed -e "s/ Z$/ /" <<-\EOF >expect.err &&
warning: GIT_INDEX_VERSION set, but the value is 
invalid.
-   Using version Z
+   warning: Using version Z
EOF
test_i18ncmp expect.err actual.err
)
@@ -54,7 +54,7 @@ test_expect_success 'out of bounds index.version issues 
warning' '
git add a 2>&1 | sed "s/[0-9]//" >actual.err &&
sed -e "s/ Z$/ /" <<-\EOF >expect.err &&
warning: index.version set, but the value is invalid.
-   Using version Z
+   warning: Using version Z
EOF
test_i18ncmp expect.err actual.err
)
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index b8fbdefcdc..cd4a10df2d 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -771,10 +771,10 @@ test_expect_success 'setup for testing rm messages' '
 test_expect_success 'rm files with different staged content' '
cat >expect <<-\EOF &&
error: the following files have staged content different from both the
-   file and the HEAD:
-   bar.txt
-   foo.txt
-   (use -f to force removal)
+   error: file and the HEAD:
+   error: bar.txt
+   error: foo.txt
+   error: (use -f to force removal)
EOF
echo content1 >foo.txt &&

[RFC PATCH 0/3] usage: prefix all lines in `vreportf()`, not just the first

[Martin Ågren]

On 25 May 2018 at 11:14, Junio C Hamano <gits...@pobox.com> wrote:
> Junio C Hamano <gits...@pobox.com> writes:
>
>>> +warning("the '-l' option is an alias for 
>>> '--create-reflog' and");
>>> +warning("has no effect in list mode. This option will 
>>> soon be");
>>> +warning("removed and you should omit it (or use 
>>> '--list' instead).");
>
> By the way, this is one of these times when I feel that we should
> have a better multi-line message support in die/error/warning/info
> functions.  Ideally, I should be able to write
[...]
> warning(_("the '-l' option is an alias for '--create-reflog' and\n"
>   "has no effect in list mode, This option will soon be\n"
>   "removed and you should omit it (or use '--list' 
> instead)."));
[...]
> and warning() would:
>
>  - do the sprintf formatting thing as necessary to prepare a long multi-line
>message;
>
>  - chomp that into lines at '\n' boundary; and
>
>  - give each of these lines with _("warning: ") prefixed.
>
> That way, translators can choose to make the resulting message to
> different number of lines from the original easily.

How about something like this? The first two patches implement the
above three points, except for the translation of "warning: ".

The third patch is the main reason this is marked RFC. It translates
"warning: " and similar, and breaks quite a few tests under
GETTEXT_POISON since we grep for, e.g., "warning" on stderr. I could
annotate those tests, but since I'm running out of time anyway, I
thought I'd post this as a heads-up of "I'm looking into this".

I do wonder: If our tests rely on grepping for "warning" (for pretty
good reasons), how many scripts out there do something similar (for
maybe-not-so-good reasons, but still)? Do we want to avoid breaking
them?

Also left to do is to convert any existing lego-ing users to the
single-string form that Junio wished for above.

Martin

Martin Ågren (3):
  usage: extract `prefix_suffix_lines()` from `advise()`
  usage: prefix all lines in `vreportf()`, not just the first
  usage: translate the "error: "-prefix and others

 t/t1011-read-tree-sparse-checkout.sh |  6 ++---
 t/t1506-rev-parse-diagnosis.sh   |  2 +-
 t/t1600-index.sh |  6 ++---
 t/t3600-rm.sh| 36 -
 t/t5512-ls-remote.sh |  6 ++---
 t/t7607-merge-overwrite.sh   |  6 ++---
 t/t7609-merge-co-error-msgs.sh   | 39 ++--
 git-compat-util.h|  8 ++
 advice.c | 18 ++---
 usage.c  | 28 
 10 files changed, 89 insertions(+), 66 deletions(-)

-- 
2.17.0.1181.g093e983b05

Re: [PATCH v5 0/4] unpack_trees_options: free messages when done

[Martin Ågren]

On 22 May 2018 at 04:54, Junio C Hamano  wrote:
> Junio C Hamano  writes:

>> Hmph, this unfortunately depends on 'next', which means we cannot
>> merge it down to 'maint' later to fix these leaks.  I guess it is
>> not a huge deal, though.  We've lived with these message leaks for
>> quite some time now and earth still kept rotating ;-)
>
> Oh, what was I thinking.  This, just like its previous rounds, is on
> top of bp/merge-rename-config^0 and it is expected *not* to be
> mergeable to 'maint' (or 'master', for that matter, at least not
> yet).

Right. The reason it depends on that topic is the user in
merge-recursive.c. Other than patch 2 and a small part of patch 4, this
should be mergeable to 'master' (as I recall) and probably also to
'maint'. I suppose this series could have been done as three patches to
fix all users except one, then one or two patches to fix
merge-recursive.c.

That would have allowed merging the first part of the series to 'maint'.
(Maybe not to fix the leaking as such, but to keep 'maint' more up to
date with 'master' for easier merging of other topics?) If you'd prefer
an ordering like that (now and/or in the future), just let me know.

Martin

Re: [PATCH] regex: do not call `regfree()` if compilation fails

[Martin Ågren]

On 22 May 2018 at 04:20, Eric Sunshine <sunsh...@sunshineco.com> wrote:
> On Mon, May 21, 2018 at 2:43 PM, Stefan Beller <sbel...@google.com> wrote:
>> On Sun, May 20, 2018 at 3:50 AM, Martin Ågren <martin.ag...@gmail.com> wrote:
>>> It is apparently undefined behavior to call `regfree()` on a regex where
>>> `regcomp()` failed. [...]
>>>
>>> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
>>> @@ -215,7 +215,6 @@ static void regcomp_or_die(regex_t *regex, const char 
>>> *needle, int cflags)
>>> /* The POSIX.2 people are surely sick */
>>> char errbuf[1024];
>>> regerror(err, regex, errbuf, 1024);
>>> -   regfree(regex);
>>> die("invalid regex: %s", errbuf);
>>
>> While the commit message is very clear why we supposedly introduce a leak 
>> here,
>> it is hard to be found from the source code (as we only delete code
>> there, so digging
>> for history is not obvious), so maybe
>>
>>  /* regfree(regex) is invalid here */
>>
>> instead?
>
> The commit message doesn't say that we are supposedly introducing a
> leak (and, indeed, no resources should have been allocated to the
> 'regex' upon failed compile). It's saying that removing this call
> potentially avoids a crash under some implementations.
>
> Given that the very next line is die(), and that the function name has
> "_or_die" in it, I'm not sure that an in-code comment about regfree()
> being invalid upon failed compile would be all that helpful; indeed,
> it could be confusing, causing the reader to wonder why that is
> significant if we're just dying anyhow. I find that the patch, as is,
> clarifies rather than muddles the situation.

Like Eric, I feel that the possible leak here is somewhat uninteresting,
since the next line will die. That said, I seem to recall from my
grepping around earlier that we have other users where we return
with a failure instead of dying.

Any clarifying comments in such code would be a separate patch to me. I
also do not immediately see the need for adding such a comment in those
places. We can do that once we verify that we actually do leak (I would
expect that to happen only in some implementations, and I think there is
a fair chance that we will never encounter such an implementation.)

Martin