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] Add an EditorConfig file

[brian m. carlson]

On Mon, Sep 17, 2018 at 07:18:50PM -0400, Taylor Blau wrote:
> Hi brian,
> 
> Thanks for CC-ing me on this.
> 
> I use editorconfig every day via the configuration in my home directory
> [1], and the Vim plugin editorconfig-vim [2]. It's a great piece of
> software, and I've been using it without any issue since around the
> beginning of 2015.

Yeah, I noticed it in your dotfiles.

> In all *.[ch] files in git.git, I found a total of 817 lines over 79
> characters wide:
> 
>   $ for file in $(git ls-files '**/*.[ch]'); do
>   awk 'length > 79' $f;
> done | wc -l
> 
> So I think that specifying indent_style, and tab_width to be 'tab' and
> '8' respectively is enough. We shouldn't be enforcing a rule about line
> lengths that we are not ourselves consistently following.

I must admit that some of these are my fault.  I think we're moving to
use clang-format more as a tool, so hopefully those become less of an
issue over time.

> Have you thought about including guidelines on COMMIT_EDITMSG? We prefer
> 72 characters per line [3], and this is enforceable via the following:
> 
>   [COMMIT_EDITMSG]
>   max_line_length = 72

That's an interesting thought.  I know different people have different
settings for this: 70, 72, 74.  72 characters is typical for emails, and
since our commits end up as emails, I think standardizing on 72 would
make sense.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH] Add an EditorConfig file

[brian m. carlson]

On Tue, Sep 18, 2018 at 08:00:27PM -0700, Junio C Hamano wrote:
> "brian m. carlson"  writes:
> 
> > To make automatically configuring one's editor easier, provide an
> > EditorConfig file.  This is an INI-style configuration file that can be
> > used to specify editor settings and can be understood by a wide variety
> > of editors.  Some editors include this support natively; others require
> > a plugin.
> 
> Good intentions.  One thing that makes me wonder is how well this
> plays with "make style" and how easy will it be to keep their
> recommendations in sync.  Ideally, if we can generate one from the
> other, or both from a unified source, that would be great.

I think "make style" and the EditorConfig file are complementary.  "make
style" autoformats code into a diff.  I agree that if we always used
clang-format to format code, then this would be a non-issue in the
EditorConfig file, since we'd just tell people to format their code and
be done with it.  However, we don't automatically do that, so I think
this still has value.

(I am having trouble getting make style to work, though, because it
seems to invoke clang-format as a git subcommand, and I don't think that
works.  I may send a patch.)

Even if we did that, we couldn't do it for Perl, because the tidy tool
for Perl, perltidy, produces different output depending on version.  I
expect we'll have little success in trying to standardize on a given
version.

The EditorConfig file applies to a variety of formats and is designed to
set default editor settings (which users *can* override if they choose).
It applies to any file pattern; for example, as Taylor pointed out, we
could use it to recommend 72-character lines in commit messages.

I agree that maintaining more files is a hassle, but personally, I think
it quite unlikely that we're going to change the Git style, so I feel
the maintenance is fairly low.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH 4/4] merge-recursive: rename merge_file_1() and merge_content()

[Stefan Beller]

On Wed, Sep 19, 2018 at 9:15 AM Elijah Newren  wrote:
>
> Summary:
>   merge_file_1()  -> merge_mode_and_contents()
>   merge_content() -> handle_content_merge()
>
> merge_file_1() is a very unhelpful name.  Rename it to
> merge_mode_and_contents() to reflect what it does.
>
> merge_content() calls merge_mode_and_contents() to do the main part of
> its work, but most of this function was about higher level stuff, e.g.
> printing out conflict messages, updating skip_worktree bits, checking
> for ability to avoid updating the working tree or for D/F conflicts
> being in the way, etc.  Since there are several handle_*() functions for
> similar levels of checking and handling in merge-recursive.c (e.g.
> handle_change_delete(), handle_rename_rename_2to1()), let's rename this
> function to handle_content_merge().
>
> Signed-off-by: Elijah Newren 

I looked through the whole series and 1,2,4 are obvious improvements IMHO,
and 3 in itself is not worth it if it weren't for 4 (as now we don't
have neither
_1 and _one functions in this file).

The whole series looks good to me.

Thanks,
Stefan

Re: [PATCH 1/3] git-archimport.1: specify what kind of Arch we're talking about

[frederik]

> I think "Is it a CPU archtecture?" is a red-herring, but between

It wasn't meant to be a red-herring though. Plenty of Linux
distributions have different architecture-specific package repos, and
arch is a common abbreviation for architecture. This only comes up if
you search for the full word e.g. "linux architecture repository"
since other wise Arch Linux dominates all the search results.

A bit of a tangent...

Frederick

> "What is an Arch Repository?" and "What is GNU Arch?" there indeed
> is a vast difference in the quality of information readers would
> get; with the proposed commit log message that ends with "so people
> can find it with search engines", I am reasonably sure this is an
> improvement worth having.
> 
> Thanks.

Re: [PATCH 2/3] git-column.1: clarify initial description, provide examples

[Junio C Hamano]

Frederick Eaton  writes:

> When I read this man page I couldn't figure out what kind of input it
> was referring to, or how input was being put into columns, or where I
> should look for the syntax of the --mode option.
>
> Signed-off-by: Frederick Eaton 
> ---
>  Documentation/git-column.txt | 35 +--
>  1 file changed, 33 insertions(+), 2 deletions(-)

I'll defer this to its primary author.

>
> diff --git a/Documentation/git-column.txt b/Documentation/git-column.txt
> index 03d18465d..5bbb51068 100644
> --- a/Documentation/git-column.txt
> +++ b/Documentation/git-column.txt
> @@ -13,7 +13,10 @@ SYNOPSIS
>  
>  DESCRIPTION
>  ---
> -This command formats its input into multiple columns.
> +This command formats the lines of its standard input into a table with
> +multiple columns. Each input line occupies one cell of the table. It
> +is used internally by other git commands to format output into
> +columns.
>  
>  OPTIONS
>  ---
> @@ -23,7 +26,7 @@ OPTIONS
>  
>  --mode=::
>   Specify layout mode. See configuration variable column.ui for option
> - syntax.
> + syntax (in git-config(1)).
>  
>  --raw-mode=::
>   Same as --mode but take mode encoded as a number. This is mainly used
> @@ -43,6 +46,34 @@ OPTIONS
>  --padding=::
>   The number of spaces between columns. One space by default.
>  
> +EXAMPLES
> +--
> +
> +Format data by columns:
> +
> +$ seq 1 24 | git column --mode=column --padding=5
> +1  4  7  10 13 16 19 22
> +2  5  8  11 14 17 20 23
> +3  6  9  12 15 18 21 24
> +
> +
> +Format data by rows:
> +
> +$ seq 1 21 | git column --mode=row --padding=5
> +1  2  3  4  5  6  7
> +8  9  10 11 12 13 14
> +15 16 17 18 19 20 21
> +
> +
> +List some tags in a table with unequal column widths:
> +
> +$ git tag --list 'v2.4.*' --column=row,dense
> +v2.4.0  v2.4.0-rc0  v2.4.0-rc1  v2.4.0-rc2  v2.4.0-rc3
> +v2.4.1  v2.4.10 v2.4.11 v2.4.12 v2.4.2
> +v2.4.3  v2.4.4  v2.4.5  v2.4.6  v2.4.7
> +v2.4.8  v2.4.9
> +
> +
>  GIT
>  ---
>  Part of the linkgit:git[1] suite

Re: [PATCH 1/3] git-archimport.1: specify what kind of Arch we're talking about

[Junio C Hamano]

Frederick Eaton  writes:

> Is it a CPU architecture? Is it Arch Linux? If you search for "arch
> repository", nothing relevant comes up. Let's call it GNU Arch so
> people can find it with search engines.
>
> Signed-off-by: Frederick Eaton 
> ---
>  Documentation/git-archimport.txt | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

I think "Is it a CPU archtecture?" is a red-herring, but between
"What is an Arch Repository?" and "What is GNU Arch?" there indeed
is a vast difference in the quality of information readers would
get; with the proposed commit log message that ends with "so people
can find it with search engines", I am reasonably sure this is an
improvement worth having.

Thanks.

>
> diff --git a/Documentation/git-archimport.txt 
> b/Documentation/git-archimport.txt
> index ea7065336..a595a0ffe 100644
> --- a/Documentation/git-archimport.txt
> +++ b/Documentation/git-archimport.txt
> @@ -3,7 +3,7 @@ git-archimport(1)
>  
>  NAME
>  
> -git-archimport - Import an Arch repository into Git
> +git-archimport - Import a GNU Arch repository into Git
>  
>  
>  SYNOPSIS
> @@ -14,7 +14,8 @@ SYNOPSIS
>  
>  DESCRIPTION
>  ---
> -Imports a project from one or more Arch repositories. It will follow branches
> +Imports a project from one or more GNU Arch repositories.
> +It will follow branches
>  and repositories within the namespaces defined by the 
>  parameters supplied. If it cannot find the remote branch a merge comes from
>  it will just import it as a regular commit. If it can find it, it will mark 
> it

Re: [PATCH 0/3] some documentation changes from the beginning of the alphabet

[Junio C Hamano]

Frederick Eaton  writes:

> I've never sent patches using git before so I thought it would be
> useful to make a small test.

... eh, welcome ;-)

> I read ./Documentation/SubmittingPatches and I sent these to myself
> and practiced applying them using `git am`, and I also compiled and
> checked the revised manual pages to see that they format correctly.
> Unfortunately it was too late to run 'git diff --check' because I had
> already committed the changes to my repo, but I don't see any
> whitespace highlighted when I run 'git log -p'.

Thanks for being diligent.

> By the way for some reason git-contacts shows more names when I run it
> on the patch hash than when I give it the patch name:
>
> $ ./contrib/contacts/git-contacts 222580cb60ee64f7b81fed64ec8fbfc81952557f
> Sébastien Guimmara 
> Nguyễn Thái Ngọc Duy 
> Eric Sunshine 
> Junio C Hamano 
> $ ./contrib/contacts/git-contacts 
> ./outgoing/0002-git-column.1-clarify-initial-description-provide-exa.patch
> 
> Junio C Hamano 

I've never trusted what git-contacts say, but the latter one
certainly looks strange, as

git log --no-merges Documentation/git-column.txt

makes it clear that I have nothing to do with it ;-).  Perhaps the
tool gives too much credit for Signed-off-by: footer, or something.

bug with git merge-base

[Alexander Mills]

The following command sequence exits with 1, and no stderr

base='remotes/origin/dev';
fork_point="$(git merge-base --fork-point "$base")";

I cannot figure out why it's exiting with 1, but there is no stdout/stderr

-alex

-- 

Alexander D. Mills
¡¡¡ New cell phone number: (415)730-1805 !!!
alexander.d.mi...@gmail.com

www.linkedin.com/pub/alexander-mills/b/7a5/418/

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
> +'

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

[Ævar Arnfjörð Bjarmason]

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.

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
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).

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 builtin/gc.c  |  2 +-
 t/t6500-gc.sh | 21 +
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 06ddd3aea5..91ffb1a803 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -647,7 +647,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 
if (gc_write_commit_graph)
write_commit_graph_reachable(get_object_directory(), 0,
-!daemonized);
+!quiet && !daemonized);
 
if (auto_gc && too_many_loose_objects())
warning(_("There are too many unreachable loose objects; "
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index 818435f04e..6183702c30 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -4,6 +4,7 @@ test_description='basic git gc tests
 '
 
 . ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-terminal.sh
 
 test_expect_success 'setup' '
# do not let the amount of physical memory affects gc
@@ -99,6 +100,26 @@ test_expect_success 'auto gc with too many loose objects 
does not attempt to cre
test_line_count = 2 new # There is one new pack and its .idx
 '
 
+test_expect_success 'gc --no-quiet' '
+   git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
+   ! test -s stdout &&
+   test_line_count = 1 stderr &&
+   test_i18ngrep "Computing commit graph generation numbers" stderr
+'
+
+test_expect_success TTY 'with TTY: gc --no-quiet' '
+   test_terminal git -c gc.writeCommitGraph=true gc --no-quiet >stdout 
2>stderr &&
+   ! test -s stdout &&
+   test_i18ngrep "Enumerating objects" stderr &&
+   test_i18ngrep "Computing commit graph generation numbers" stderr
+'
+
+test_expect_success 'gc --quiet' '
+   git -c gc.writeCommitGraph=true gc --quiet >stdout 2>stderr &&
+   ! test -s stdout &&
+   ! test -s stderr
+'
+
 run_and_wait_for_auto_gc () {
# We read stdout from gc for the side effect of waiting until the
# background gc process exits, closing its fd 9.  Furthermore, the
-- 
2.19.0.444.g18242da7ef

[PATCH 3/3] git-describe.1: clarify that "human readable" is also git-readable

[Frederick Eaton]

The caption uses the term "human readable", but the DESCRIPTION did
not explain this in context.

Signed-off-by: Frederick Eaton 
---
 Documentation/git-describe.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index e027fb8c4..ccdc5f83d 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -18,7 +18,9 @@ The command finds the most recent tag that is reachable from a
 commit.  If the tag points to the commit, then only the tag is
 shown.  Otherwise, it suffixes the tag name with the number of
 additional commits on top of the tagged object and the
-abbreviated object name of the most recent commit.
+abbreviated object name of the most recent commit. The result
+is a "human-readable" object name which can also be used to
+identify the commit to other git commands.
 
 By default (without --all or --tags) `git describe` only shows
 annotated tags.  For more information about creating annotated tags
-- 
2.19.0

[PATCH 2/3] git-column.1: clarify initial description, provide examples

[Frederick Eaton]

When I read this man page I couldn't figure out what kind of input it
was referring to, or how input was being put into columns, or where I
should look for the syntax of the --mode option.

Signed-off-by: Frederick Eaton 
---
 Documentation/git-column.txt | 35 +--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-column.txt b/Documentation/git-column.txt
index 03d18465d..5bbb51068 100644
--- a/Documentation/git-column.txt
+++ b/Documentation/git-column.txt
@@ -13,7 +13,10 @@ SYNOPSIS
 
 DESCRIPTION
 ---
-This command formats its input into multiple columns.
+This command formats the lines of its standard input into a table with
+multiple columns. Each input line occupies one cell of the table. It
+is used internally by other git commands to format output into
+columns.
 
 OPTIONS
 ---
@@ -23,7 +26,7 @@ OPTIONS
 
 --mode=::
Specify layout mode. See configuration variable column.ui for option
-   syntax.
+   syntax (in git-config(1)).
 
 --raw-mode=::
Same as --mode but take mode encoded as a number. This is mainly used
@@ -43,6 +46,34 @@ OPTIONS
 --padding=::
The number of spaces between columns. One space by default.
 
+EXAMPLES
+--
+
+Format data by columns:
+
+$ seq 1 24 | git column --mode=column --padding=5
+1  4  7  10 13 16 19 22
+2  5  8  11 14 17 20 23
+3  6  9  12 15 18 21 24
+
+
+Format data by rows:
+
+$ seq 1 21 | git column --mode=row --padding=5
+1  2  3  4  5  6  7
+8  9  10 11 12 13 14
+15 16 17 18 19 20 21
+
+
+List some tags in a table with unequal column widths:
+
+$ git tag --list 'v2.4.*' --column=row,dense
+v2.4.0  v2.4.0-rc0  v2.4.0-rc1  v2.4.0-rc2  v2.4.0-rc3
+v2.4.1  v2.4.10 v2.4.11 v2.4.12 v2.4.2
+v2.4.3  v2.4.4  v2.4.5  v2.4.6  v2.4.7
+v2.4.8  v2.4.9
+
+
 GIT
 ---
 Part of the linkgit:git[1] suite
-- 
2.19.0

[PATCH 0/3] some documentation changes from the beginning of the alphabet

[Frederick Eaton]

I've never sent patches using git before so I thought it would be
useful to make a small test.

I read ./Documentation/SubmittingPatches and I sent these to myself
and practiced applying them using `git am`, and I also compiled and
checked the revised manual pages to see that they format correctly.
Unfortunately it was too late to run 'git diff --check' because I had
already committed the changes to my repo, but I don't see any
whitespace highlighted when I run 'git log -p'.

I'm sure I'm doing something wrong, so please let me know what it is!

Here's the command I'll use to send the messages (without --dry-run):

git send-email --to "Junio C Hamano " --cc 
git@vger.kernel.org --cc-cmd ./contrib/contacts/git-contacts outgoing/ --dry-run

By the way for some reason git-contacts shows more names when I run it
on the patch hash than when I give it the patch name:

$ ./contrib/contacts/git-contacts 222580cb60ee64f7b81fed64ec8fbfc81952557f
Sébastien Guimmara 
Nguyễn Thái Ngọc Duy 
Eric Sunshine 
Junio C Hamano 
$ ./contrib/contacts/git-contacts 
./outgoing/0002-git-column.1-clarify-initial-description-provide-exa.patch  
  
Junio C Hamano 

Not sure what's going on here, but the changes I propose seem fairly
straightforward...(?)

Frederick Eaton (3):
  git-archimport.1: specify what kind of Arch we're talking about
  git-column.1: clarify initial description, provide examples
  git-describe.1: clarify that "human readable" is also git-readable

 Documentation/git-archimport.txt |  5 +++--
 Documentation/git-column.txt | 35 ++--
 Documentation/git-describe.txt   |  4 +++-
 3 files changed, 39 insertions(+), 5 deletions(-)

-- 
2.19.0

[PATCH 1/3] git-archimport.1: specify what kind of Arch we're talking about

[Frederick Eaton]

Is it a CPU architecture? Is it Arch Linux? If you search for "arch
repository", nothing relevant comes up. Let's call it GNU Arch so
people can find it with search engines.

Signed-off-by: Frederick Eaton 
---
 Documentation/git-archimport.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-archimport.txt b/Documentation/git-archimport.txt
index ea7065336..a595a0ffe 100644
--- a/Documentation/git-archimport.txt
+++ b/Documentation/git-archimport.txt
@@ -3,7 +3,7 @@ git-archimport(1)
 
 NAME
 
-git-archimport - Import an Arch repository into Git
+git-archimport - Import a GNU Arch repository into Git
 
 
 SYNOPSIS
@@ -14,7 +14,8 @@ SYNOPSIS
 
 DESCRIPTION
 ---
-Imports a project from one or more Arch repositories. It will follow branches
+Imports a project from one or more GNU Arch repositories.
+It will follow branches
 and repositories within the namespaces defined by the 
 parameters supplied. If it cannot find the remote branch a merge comes from
 it will just import it as a regular commit. If it can find it, it will mark it
-- 
2.19.0

Re: [PATCH] reflog expire: add progress output

[Ævar Arnfjörð Bjarmason]

On Wed, Sep 19 2018, Jeff King wrote:

> On Wed, Sep 19, 2018 at 07:22:44PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> > Do we have --quiet option or something that needs to completely
>> > suppress this progress thing?
>>
>> Yes. I also see my commit graph process patches sitting in "next" broke
>> the "git gc --quiet" mode, and I'll need to submit something on top
>> (which'll be easy), and submit a v2 on this (pending further
>> comments...).
>>
>> Is there a better way to test that (fake up the file descriptor check)
>> in the tests other than adding getenv("GIT_TEST...") to the progress.c
>> logic?
>
> The progress code doesn't do the isatty() check at all. The caller has
> to do it (and ideally would respect --progress/--no-progress to
> override, along with having --quiet imply --no-progress if such an
> option exists).

Yeah, what I was confused by was testing this with "git gc", and until
my recent commit graph progress patches + this (which I wasn't testing
against) the progress output was all from pack-objects, which checks the
--progress option, and then proceeds to ignore all of that and just
check isatty().

> Once you have all that, then you can test --progress explicitly. If you
> want to check the isatty() handling, you can use test_terminal().

Thanks.

Re: [PATCH v2 1/6] CodingGuidelines: add shell piping guidelines

[Matthew DeVore]

On Wed, Sep 19, 2018 at 5:36 AM Eric Sunshine  wrote:
>
> On Tue, Sep 18, 2018 at 10:11 PM Matthew DeVore  wrote:
> > Yes, it's probably better to add a point about that. Here is the new
> > documentation after applying your suggestions:
> >
> >  - If a piped sequence which spans multiple lines, put each statement
>
> s/which//
Done.

On Wed, Sep 19, 2018 at 7:24 AM Junio C Hamano  wrote:
> The formatting advice to place '|' at the end applies equally to
> '&&' and '||' because these three syntactic elements share exactly
> the same trait: the shell knows you haven't finished speaking when
> it sees them at the end of the line and keeps listening, and humans
> would know that too, so there is no need for explicitly continuing
> the line with backslash.
>
I've reworded the text to indicate the advice applies to && and || as well.

> Organizationally speaking, I wonder if the above about formatting
> would better appear separate from the latter two points that are
> about semantics.
>
I moved the formatting point to right under the point about formatting
if statements, which does seem like a more natural progression.

Here is the new patch to summarize the changes (warning: tabs are mangled):



CodingGuidelines: add shell piping guidelines

Add two guidelines:

 - pipe characters should appear at the end of lines, and not cause
   indentation
 - pipes should be avoided when they swallow exit codes that can
   potentially fail

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 48aa4edfb..6d265327c 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -118,6 +118,24 @@ For shell scripts specifically (not exhaustive):
 do this
 fi

+ - If a command sequence joined with && or || or | spans multiple
+   lines, put each command on a separate line and put && and || and |
+   operators at the end of each line, rather than the start. This
+   means you don't need to use \ to join lines, since the above
+   operators imply the sequence isn't finished.
+
+(incorrect)
+grep blob verify_pack_result \
+| awk -f print_1.awk \
+| sort >actual &&
+...
+
+(correct)
+grep blob verify_pack_result |
+awk -f print_1.awk |
+sort >actual &&
+...
+
  - We prefer "test" over "[ ... ]".

  - We do not write the noiseword "function" in front of shell
@@ -163,6 +181,15 @@ For shell scripts specifically (not exhaustive):

does not have such a problem.

+ - In a piped chain such as "grep blob objects | sort", the exit codes
+   returned by processes besides the last are ignored. This means that
+   if git crashes at the beginning or middle of a chain, it may go
+   undetected. Prefer writing the output of that command to a
+   temporary file with '>' rather than pipe it.
+
+ - The $(git ...) construct also discards git's exit code, so if the
+   goal is to test that particular command, redirect its output to a
+   temporary file rather than wrap it with $( ).

 For C programs:

Re: Access Git ssh on port 8822 ?

[Ævar Arnfjörð Bjarmason]

On Wed, Sep 19 2018, Jeff King wrote:

> On Wed, Sep 19, 2018 at 02:47:09PM -0300, Leonardo Bozzi wrote:
>
>> Good afternoon, I'm trying to set up a git server, but I want to use
>> ssh access to connect clients on my server, but because of a
>> limitation in my internet provider it blocks access from outside on
>> port 22, so I changed the same from ssh to 8822. But when I give the
>> command:
>> 
>> $git remote add origin bo...@bozzi.net:/opt/gitcurso
>> 
>> The server blocks me because I would have to access via port 8822. How
>> do I make the connection correctly?
>
> You have two options:
>
>   1. You can use the more verbose ssh URL syntax, which allows a port
>  number:
>
>git clone ssh://bo...@bozzi.net:8822/opt/gitcurso
>
>   2. You can use a host block in your ~/.ssh/config to set the default
>  port for that host.
>
>{
>  echo "Host bozzi.net"
>echo "Port 8822"
>} >>$HOME/.ssh/config
>
> -Peff

3. GIT_SSH_COMMAND="ssh -p 8822"  git clone bo...@bozzi.net:/opt/gitcurso

Re: [PATCH v2 2/6] test-reach: add run_three_modes method

[Junio C Hamano]

Junio C Hamano  writes:

> SZEDER Gábor  writes:
>
>>> While inspecting this code, I realized that the final test for
>>> 'commit_contains --tag' is silently dropping the '--tag' argument.
>>> It should be quoted to include both.
>>
>> Nit: while quoting the function's arguments does fix the issue, it
>> leaves the tests prone to the same issue in the future.  Wouldn't it
>> be better to use $@ inside the function to refer to all its arguments?
>
> IOW, do it more like this?
>
>>> -test_three_modes () {
>>> +run_three_modes () {
>>> test_when_finished rm -rf .git/objects/info/commit-graph &&
>>> -   test-tool reach $1 actual &&
>>> +   $1 actual &&
>
>   "$@" actual
>
> i.e. treat each parameter as separate things without further getting
> split at $IFS and ...
>
>>> +test_three_modes () {
>>> +   run_three_modes "test-tool reach $1"
>
>   run_three_modes test-tool reach "$1"
>
> ... make sure there three things are sent as separate, by quoting
> "$1" inside dq.
>
> I think that makes sense.

I also noticed that 2/6 made "commti_contains --tag" enclosed in dq
pair for one test, but the next test after it has the identical one.

Here is what I queued in the meantime.

diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh
index 1b18e12a4e..1377849bf8 100755
--- a/t/t6600-test-reach.sh
+++ b/t/t6600-test-reach.sh
@@ -55,18 +55,18 @@ test_expect_success 'setup' '
 
 run_three_modes () {
test_when_finished rm -rf .git/objects/info/commit-graph &&
-   $1 actual &&
+   "$@" actual &&
test_cmp expect actual &&
cp commit-graph-full .git/objects/info/commit-graph &&
-   $1 actual &&
+   "$@" actual &&
test_cmp expect actual &&
cp commit-graph-half .git/objects/info/commit-graph &&
-   $1 actual &&
+   "$@" actual &&
test_cmp expect actual
 }
 
 test_three_modes () {
-   run_three_modes "test-tool reach $1"
+   run_three_modes test-tool reach "$1"
 }
 
 test_expect_success 'ref_newer:miss' '
@@ -223,7 +223,7 @@ test_expect_success 'commit_contains:hit' '
EOF
echo "commit_contains(_,A,X,_):1" >expect &&
test_three_modes commit_contains &&
-   test_three_modes "commit_contains --tag"
+   test_three_modes commit_contains --tag
 '
 
 test_expect_success 'commit_contains:miss' '
-- 
2.19.0-216-g2d3b1c576c

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

[Ramsay Jones]

On 19/09/18 18:49, Martin Ågren wrote:
> 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.

Yes, generated headers, but no, not more general. ;-)

I originally included those headers directly in the
EXCEPT_HDRS macro, along with the list of excluded
directories (which was much longer at one point).

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.

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. I guess I could eliminate
that macro, absorbing it into EXCEPT_HDRS, if that would
lead to less confusion ...

[I suspect the fact that LIB_H (almost always) contains
'command-list.h' has not been noticed ... :-P ]

> 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.

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.
(which is probably not a problem).

ATB,
Ramsay Jones

> 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
> 

Re: [PATCH v2 2/6] test-reach: add run_three_modes method

[Junio C Hamano]

SZEDER Gábor  writes:

>> While inspecting this code, I realized that the final test for
>> 'commit_contains --tag' is silently dropping the '--tag' argument.
>> It should be quoted to include both.
>
> Nit: while quoting the function's arguments does fix the issue, it
> leaves the tests prone to the same issue in the future.  Wouldn't it
> be better to use $@ inside the function to refer to all its arguments?

IOW, do it more like this?

>> -test_three_modes () {
>> +run_three_modes () {
>>  test_when_finished rm -rf .git/objects/info/commit-graph &&
>> -test-tool reach $1 actual &&
>> +$1 actual &&

"$@" actual

i.e. treat each parameter as separate things without further getting
split at $IFS and ...

>> +test_three_modes () {
>> +run_three_modes "test-tool reach $1"

run_three_modes test-tool reach "$1"

... make sure there three things are sent as separate, by quoting
"$1" inside dq.

I think that makes sense.

Re: [PATCH v5 9/9] submodule: support reading .gitmodules when it's not in the working tree

[Junio C Hamano]

SZEDER Gábor  writes:

> it appears that this patch (and its previous versions as well) is
> responsible for triggering occasional test failures in
> 't7814-grep-recurse-submodules.sh', more frequently, about once in
> every ten runs, on macOS on Travis CI, less frequently, about once in
> a couple of hundred runs on Linux on my machine.

I see that among Cc'ed are people who are more familiar with the
submodule code and where it wants to go.  Thanks for a report and
analysis.

> The reason for the failure is memory corruption manifesting in various
> ways: segfault, malloc() or use after free() errors from libc, corrupt
> loose object, invalid ref, bogus output, etc.
> 
> Applying the following patch makes t7814 fail almost every time,
> though sometimes that loop has to iterate over 1000 times until that
> 'git grep' finally fails...  so good luck with debugging ;)
>
> diff --git a/t/t7814-grep-recurse-submodules.sh 
> b/t/t7814-grep-recurse-submodules.sh
> index 7184113b9b..93ae2e8e7c 100755
> --- a/t/t7814-grep-recurse-submodules.sh
> +++ b/t/t7814-grep-recurse-submodules.sh
> @@ -337,6 +337,10 @@ test_expect_success 'grep --recurse-submodules should 
> pass the pattern type alon
>   test_must_fail git -c grep.patternType=fixed grep --recurse-submodules 
> -e "(.|.)[\d]" &&
>  
>   # Basic
> + for i in $(seq 0 2000)
> + do
> + git grep --recurse-submodules 1 >/dev/null || return 1
> + done &&
>   git grep -G --recurse-submodules -e "(.|.)[\d]" >actual &&
>   cat >expect <<-\EOF &&
>   a:(1|2)d(3|4)
>
> On first look I didn't notice anything that is obviously wrong in this
> patch and could be responsible for the memory corruption, but there is
> one thing I found strange, though:
>
>
> On Mon, Sep 17, 2018 at 04:09:40PM +0200, Antonio Ospite wrote:
>> When the .gitmodules file is not available in the working tree, try
>> using the content from the index and from the current branch.
>
> "from the index and from the current branch" of which repository?
>
>> This
>> covers the case when the file is part of the repository but for some
>> reason it is not checked out, for example because of a sparse checkout.
>> 
>> This makes it possible to use at least the 'git submodule' commands
>> which *read* the gitmodules configuration file without fully populating
>> the working tree.
>> 
>> Writing to .gitmodules will still require that the file is checked out,
>> so check for that before calling config_set_in_gitmodules_file_gently.
>> 
>> Add a similar check also in git-submodule.sh::cmd_add() to anticipate
>> the eventual failure of the "git submodule add" command when .gitmodules
>> is not safely writeable; this prevents the command from leaving the
>> repository in a spurious state (e.g. the submodule repository was cloned
>> but .gitmodules was not updated because
>> config_set_in_gitmodules_file_gently failed).
>> 
>> Finally, add t7416-submodule-sparse-gitmodules.sh to verify that reading
>> from .gitmodules succeeds and that writing to it fails when the file is
>> not checked out.
>> 
>> Signed-off-by: Antonio Ospite 
>> ---
>
>> diff --git a/submodule-config.c b/submodule-config.c
>> index 61a555e920..bdb1d0e2c9 100644
>> --- a/submodule-config.c
>> +++ b/submodule-config.c
>
>> @@ -603,8 +604,21 @@ static void submodule_cache_check_init(struct 
>> repository *repo)
>>  static void config_from_gitmodules(config_fn_t fn, struct repository *repo, 
>> void *data)
>>  {
>>  if (repo->worktree) {
>> -char *file = repo_worktree_path(repo, GITMODULES_FILE);
>> -git_config_from_file(fn, file, data);
>> +struct git_config_source config_source = { 0 };
>> +const struct config_options opts = { 0 };
>> +struct object_id oid;
>> +char *file;
>> +
>> +file = repo_worktree_path(repo, GITMODULES_FILE);
>> +if (file_exists(file))
>> +config_source.file = file;
>> +else if (get_oid(GITMODULES_INDEX, ) >= 0)
>> +config_source.blob = GITMODULES_INDEX;
>
> The repository used in t7814 contains nested submodules, which means
> that config_from_gitmodules() is invoked three times.
>
> Now, the first two of those calls look at the superproject and at
> 'submodule', and find the existing files '.../trash
> directory.t7814-grep-recurse-submodules/.gitmodules' and '.../trash
> directory.t7814-grep-recurse-submodules/submodule/.gitmodules',
> respectively.  So far so good.
>
> The third call, however, looks at the nested submodule at
> 'submodule/sub', which doesn't contain a '.gitmodules' file.  So this
> function goes on with the second condition and calls
> get_oid(GITMODULES_INDEX, ), which then appears to find the blob
> in the _superproject's_ index.
>
> I'm no expert on submodules, but my gut feeling says that this can't
> be right.  But if it _is_ right, then I would say that the commit
> message should explain in detail, why it 

Re: [PATCH] add: do not accept pathspec magic 'attr'

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> Commit b0db704652 (pathspec: allow querying for attributes -
> 2017-03-13) adds new pathspec magic 'attr' but only with
> match_pathspec(). "git add" has some pathspec related code that still
> does not know about 'attr' and will bail out:
>
> $ git add ':(attr:foo)'
> fatal: BUG:dir.c:1584: unsupported magic 40
>
> A better solution would be making this code support 'attr'. But I
> don't know how much work is needed (I'm not familiar with this new
> magic). For now, let's simply reject this magic with a friendlier
> message:
>
> $ git add ':(attr:foo)'
> fatal: :(attr:foo): pathspec magic not supported by this command: 'attr'
>
> Reported-by: smau...@sebastianaudet.com
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  Since Brandon is currently unreachable, let's do this for now.

Thanks.  This certainly make it better than the status quo.

Re: [PATCH] reflog expire: add progress output

[Jeff King]

On Wed, Sep 19, 2018 at 07:22:44PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > Do we have --quiet option or something that needs to completely
> > suppress this progress thing?
> 
> Yes. I also see my commit graph process patches sitting in "next" broke
> the "git gc --quiet" mode, and I'll need to submit something on top
> (which'll be easy), and submit a v2 on this (pending further
> comments...).
> 
> Is there a better way to test that (fake up the file descriptor check)
> in the tests other than adding getenv("GIT_TEST...") to the progress.c
> logic?

The progress code doesn't do the isatty() check at all. The caller has
to do it (and ideally would respect --progress/--no-progress to
override, along with having --quiet imply --no-progress if such an
option exists).

Once you have all that, then you can test --progress explicitly. If you
want to check the isatty() handling, you can use test_terminal().

-Peff

Re: Access Git ssh on port 8822 ?

[Leonardo Bozzi]

Ok
   git clone ssh://bo...@bozzi.net:8822/opt/gitcurso

access sucess.



Adm. Leonardo Bozzi
Administrador / Analista de Sistemas
Tel.:  (27) 99988-4576
CRA-ES: 13256


Em qua, 19 de set de 2018 às 14:50, Jeff King  escreveu:
>
> On Wed, Sep 19, 2018 at 02:47:09PM -0300, Leonardo Bozzi wrote:
>
> > Good afternoon, I'm trying to set up a git server, but I want to use
> > ssh access to connect clients on my server, but because of a
> > limitation in my internet provider it blocks access from outside on
> > port 22, so I changed the same from ssh to 8822. But when I give the
> > command:
> >
> > $git remote add origin bo...@bozzi.net:/opt/gitcurso
> >
> > The server blocks me because I would have to access via port 8822. How
> > do I make the connection correctly?
>
> You have two options:
>
>   1. You can use the more verbose ssh URL syntax, which allows a port
>  number:
>
>git clone ssh://bo...@bozzi.net:8822/opt/gitcurso
>
>   2. You can use a host block in your ~/.ssh/config to set the default
>  port for that host.
>
>{
>  echo "Host bozzi.net"
>  echo "Port 8822"
>} >>$HOME/.ssh/config
>
> -Peff

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

[Jeff King]

On Wed, Sep 19, 2018 at 08:34:05PM +0200, Martin Ågren wrote:

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

Oops, yes.

> > +   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.

Yeah, this is the actual bug-fix.

I wasn't happy about having to write in_same_island() twice, but writing
it the other way ended up pretty nasty, too. Something like:

  struct object_id ext_oid;
  struct object_id *base_oid;

  base = packlist_find(_pack, base_sha1, NULL);
  if (base) {
base_oid = >idx.oid;
*base_out = base;
  }
  else if (thin && bitmap_has_sha1_in_uninteresting(bitmap_git, base_sha1)) {
hashcpy(ext_oid.hash, base_sha1);
base_oid = _oid;
*base_out = NULL;
  } else {
return 0;
  }

  if (use_island_marks && !in_same_island(>idx.oid, base_oid))
return 0;

  return 1;

That's less repetitive, but I feel like it's harder to follow which
variables are valid when.

> > +   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 ')'.

Thanks for the review!

-Peff

Re: [PATCH v2 6/6] t9109-git-svn-props.sh: split up several pipes

[Matthew DeVore]

On Wed, Sep 19, 2018 at 5:50 AM Eric Sunshine  wrote:
>
> If so, then please use test_cmp() rather than raw 'cmp' since
> test_cmp() will show the actual difference between the expected and
> actual files, which can be helpful when diagnosing a failing test.
>
All the other testcases in this file use "cmp" and I would prefer to
maintain the consistency in this file. Note that the non-stylistic
fixes (breaking up pipes) in this test were originally not the goal of
this patchset, so changing this entire file to use test_cmp (or even
just a single testcase) seems like it's just getting even farther away
from the original goal of making pipe placement consistent in the code
base.

> Rather than escaping "$" with backslash, a cleaner fix would be to
> change the double quotes around the test body to single quotes. Those
> double quotes weren't needed anyhow since there are no variable
> interpolations in the body. Single quotes would make that obvious at a
> glance in addition to avoiding unexpected behavior in the future (like
> $1, $2, etc. being interpolated at the wrong time). Single quotes
> would also make the test more idiomatic and consistent with the bulk
> of other tests in the suite. If you do go the route of swapping
> quotes, please be sure to mention the change in the commit message.
Done. I thought of that earlier and thought that I should use double
quotes for consistency with the surrounding code, but then I saw that
there were testcases farther away in the same file that used single
quotes.

Here is the new commit message:

t9109: don't swallow Git errors upstream of pipes

'git ... | foo' will mask any errors or crashes in git, so split up such
pipes in this file.

One testcase uses several separate pipe sequences in a row which are
awkward to split up. Wrap the split-up pipe in a function so the
awkwardness is not repeated. Also change that testcase's surrounding
quotes from double to single to avoid premature string interpolation.

Signed-off-by: Matthew DeVore 

>
> Thanks.
Thank you!

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: Access Git ssh on port 8822 ?

[Jeff King]

On Wed, Sep 19, 2018 at 02:47:09PM -0300, Leonardo Bozzi wrote:

> Good afternoon, I'm trying to set up a git server, but I want to use
> ssh access to connect clients on my server, but because of a
> limitation in my internet provider it blocks access from outside on
> port 22, so I changed the same from ssh to 8822. But when I give the
> command:
> 
> $git remote add origin bo...@bozzi.net:/opt/gitcurso
> 
> The server blocks me because I would have to access via port 8822. How
> do I make the connection correctly?

You have two options:

  1. You can use the more verbose ssh URL syntax, which allows a port
 number:

   git clone ssh://bo...@bozzi.net:8822/opt/gitcurso

  2. You can use a host block in your ~/.ssh/config to set the default
 port for that host.

   {
 echo "Host bozzi.net"
 echo "Port 8822"
   } >>$HOME/.ssh/config

-Peff

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

Access Git ssh on port 8822 ?

[Leonardo Bozzi]

Good afternoon, I'm trying to set up a git server, but I want to use
ssh access to connect clients on my server, but because of a
limitation in my internet provider it blocks access from outside on
port 22, so I changed the same from ssh to 8822. But when I give the
command:

$git remote add origin bo...@bozzi.net:/opt/gitcurso

The server blocks me because I would have to access via port 8822. How
do I make the connection correctly?

I'm Debian user.

Re: [PATCH v2 4/6] tests: Add linter check for pipe placement style

[Matthew DeVore]

On Mon, Sep 17, 2018 at 5:34 PM Eric Sunshine  wrote:
>
> On Mon, Sep 17, 2018 at 6:25 PM Matthew DeVore  wrote:
> > tests: Add linter check for pipe placement style
>
> Until now, the various "lint" checks have been for genuine portability
> problems (except perhaps 'test-lint-duplicates'). This new lint check
> makes style violations worthy of failing "make test". Is the indeed
> the direction we want to go? (Genuine question. I can formulate
> arguments for either side.)
>
> > ---
> > diff --git a/t/Makefile b/t/Makefile
> > @@ -101,6 +101,16 @@ test-lint-filenames:
> > +test-lint-pipes:
> > +   @# Do not use \ to join lines when the next line starts with a
> > +   @# pipe. Instead, end the prior line with the pipe, and allow that 
> > to
> > +   @# join the lines implicitly.
> > +   @bad="$$(${PERL_PATH} -n0e 'm/(\n[^\n|]+\\\n[\t ]+\|[^\n]*)/ and \
> > + print qq{$$ARGV:$$1\n\n}' $(T))"; \
> > +   test -z "$$bad" || { \
> > +   printf >&2 "pipe at start of line in file(s):\n%s\n" 
> > "$$bad"; \
> > +   exit 1; }
>
> If we're going in the direction of linting style violations, then
> maybe generalize this by calling it "test-lint-style" rather than
> "test-lint-pipes", and perhaps move the body of the test to a new
> script check-shell-style.pl (or something), much as portability
> violations are housed in check-non-portable-shell.pl.

I agree with moving this code to a separate file. I also notice that
there is logic in a file called t/chainlint.sed which normalizes
scripts for the purpose of lint checks (e.g. it removes heredocs) and
I ought to be re-using this logic in order to make the new lint check
robust.

I think I'd like to shelve this particular commit and come back to it
once I have the time and I'm more familiar with the code base. It is
not critical to this patchset.

Re: [PATCH] reflog expire: add progress output

[Ævar Arnfjörð Bjarmason]

On Wed, Sep 19 2018, Duy Nguyen wrote:

> On Wed, Sep 19, 2018 at 4:23 PM Ævar Arnfjörð Bjarmason
>  wrote:
>> Before this change the "git reflog expire" command didn't report any
>> progress.
>
> I love these progress additions you've been pushing lately :)
>
>> diff --git a/builtin/reflog.c b/builtin/reflog.c
>> index 3acef5a0ab..d3075ee75a 100644
>> --- a/builtin/reflog.c
>> +++ b/builtin/reflog.c
>> @@ -10,6 +10,7 @@
>>  #include "diff.h"
>>  #include "revision.h"
>>  #include "reachable.h"
>> +#include "progress.h"
>>
>>  /* NEEDSWORK: switch to using parse_options */
>>  static const char reflog_expire_usage[] =
>> @@ -225,14 +226,20 @@ static void mark_reachable(struct 
>> expire_reflog_policy_cb *cb)
>> struct commit_list *pending;
>> timestamp_t expire_limit = cb->mark_limit;
>> struct commit_list *leftover = NULL;
>> +   struct progress *progress = NULL;
>> +   int i = 0;
>>
>> for (pending = cb->mark_list; pending; pending = pending->next)
>> pending->item->object.flags &= ~REACHABLE;
>>
>> pending = cb->mark_list;
>> +   progress = start_delayed_progress(
>> +   _("Marking unreachable commits in reflog for expiry"), 0);
>
> Maybe just "Searching expired reflog entries" or something like that.
> It's not technically as accurate, but I think it's easier to
> understand by by new people.

Or "Pruning reflog entries"? I was aiming for some combination of a)
making it clear what we're doing (pruning stuff) b) that we're doing it
on a subset of the counter of the (very large) values we're showing.

So the current one has "a" && "b", "Searching..." has neither, and
"Pruning..." has "a" but not "b".

Maybe making a && b clear isn't that important, but I'm currently
leaning towards keeping the current one because it's not *that* long and
makes things clearer to the user.

> Do we have --quiet option or something that needs to completely
> suppress this progress thing?

Yes. I also see my commit graph process patches sitting in "next" broke
the "git gc --quiet" mode, and I'll need to submit something on top
(which'll be easy), and submit a v2 on this (pending further
comments...).

Is there a better way to test that (fake up the file descriptor check)
in the tests other than adding getenv("GIT_TEST...") to the progress.c
logic?

>> while (pending) {
>> struct commit_list *parent;
>> struct commit *commit = pop_commit();
>> +
>> +   display_progress(progress, ++i);
>
> maybe rename it to commit_count or something and leave "i" for
> temporary/short lived usage.

Good point. Willdo.

>> if (commit->object.flags & REACHABLE)
>> continue;
>> if (parse_commit(commit))
>> @@ -253,6 +260,7 @@ static void mark_reachable(struct 
>> expire_reflog_policy_cb *cb)
>> }
>> }
>> cb->mark_list = leftover;
>> +   stop_progress();
>>  }
>>
>>  static int unreachable(struct expire_reflog_policy_cb *cb, struct commit 
>> *commit, struct object_id *oid)
>> --
>> 2.19.0.444.g18242da7ef
>>

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

[Taylor Blau]

Hi Martin,

On Wed, Sep 19, 2018 at 06:38:19PM +0200, Martin Ågren wrote:
> Rather than saying "(see: above)", drop the colon. Also drop the comma
> before this note.
>
> Signed-off-by: Martin Ågren 

Thanks for both of these. I should have at least taken care of 1/2
myself, but I am appreciative of you doing it, too :-).

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.

But, either approach is fine with me, so both of these have my:

  Reviewed-by: Taylor Blau 

Thanks,
Taylor

[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 ,
+ 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: [PATCH] reflog expire: add progress output

[Duy Nguyen]

On Wed, Sep 19, 2018 at 4:23 PM Ævar Arnfjörð Bjarmason
 wrote:
> Before this change the "git reflog expire" command didn't report any
> progress.

I love these progress additions you've been pushing lately :)

> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 3acef5a0ab..d3075ee75a 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -10,6 +10,7 @@
>  #include "diff.h"
>  #include "revision.h"
>  #include "reachable.h"
> +#include "progress.h"
>
>  /* NEEDSWORK: switch to using parse_options */
>  static const char reflog_expire_usage[] =
> @@ -225,14 +226,20 @@ static void mark_reachable(struct 
> expire_reflog_policy_cb *cb)
> struct commit_list *pending;
> timestamp_t expire_limit = cb->mark_limit;
> struct commit_list *leftover = NULL;
> +   struct progress *progress = NULL;
> +   int i = 0;
>
> for (pending = cb->mark_list; pending; pending = pending->next)
> pending->item->object.flags &= ~REACHABLE;
>
> pending = cb->mark_list;
> +   progress = start_delayed_progress(
> +   _("Marking unreachable commits in reflog for expiry"), 0);

Maybe just "Searching expired reflog entries" or something like that.
It's not technically as accurate, but I think it's easier to
understand by by new people.

Do we have --quiet option or something that needs to completely
suppress this progress thing?

> while (pending) {
> struct commit_list *parent;
> struct commit *commit = pop_commit();
> +
> +   display_progress(progress, ++i);

maybe rename it to commit_count or something and leave "i" for
temporary/short lived usage.

> if (commit->object.flags & REACHABLE)
> continue;
> if (parse_commit(commit))
> @@ -253,6 +260,7 @@ static void mark_reachable(struct expire_reflog_policy_cb 
> *cb)
> }
> }
> cb->mark_list = leftover;
> +   stop_progress();
>  }
>
>  static int unreachable(struct expire_reflog_policy_cb *cb, struct commit 
> *commit, struct object_id *oid)
> --
> 2.19.0.444.g18242da7ef
>
-- 
Duy

Re: [PATCH v2 0/1] Make 'git commit' not accidentally lose staged content

[Jeff King]

On Wed, Sep 19, 2018 at 06:12:23PM +0200, Duy Nguyen wrote:

> On Wed, Sep 19, 2018 at 1:19 AM Jeff King  wrote:
> >
> > On Tue, Sep 18, 2018 at 12:36:06PM -0700, Jacob Keller wrote:
> >
> > > > I like that, too. It's a little more costly just because it may involve
> > > > object-db writes, but I think in most cases it would be fine. I almost
> > > > always "git stash" away discarded changes these days instead of "git
> > > > reset --hard", because it effectively provides this kind of log.
> > >
> > > Obviously we do eventually turn the index into a tree, which is used
> > > by the commit. Would it be possible to simply somehow store these
> > > trees, and have commands which blow the tree away simply instead, save
> > > it? I'm not sure how costly that is.
> >
> > For an up-to-date index with the cache-tree extension, this should be
> > pretty cheap.  Even for a partially-staged index, where we already have
> > the blob in the object database, it should just incur the tree
> > computation (and parts you didn't touch would hopefully have an
> > up-to-date cache-tree).
> 
> Just FYI. I wanted to get at least pruning support in place before
> posting another RFC. But right now I'm going without cache-tree.
> Whenever a file is updated, the _blob_ hashes are recorded in the
> index log (one line per update, same as reflog format) and the
> description part of the line is the updated path.
> 
> This is simpler to do, and we can still reconstruct a tree/commit in
> memory if we need to (but not whole tree snapshots; but I don't think
> that would be very useful). But maybe cache-tree approach is better.
> We will see.

Oh, interesting. Sort of a hybrid approach. :) That makes sense, and I
don't see any reason it wouldn't work.

-Peff

[PATCH 4/4] merge-recursive: rename merge_file_1() and merge_content()

[Elijah Newren]

Summary:
  merge_file_1()  -> merge_mode_and_contents()
  merge_content() -> handle_content_merge()

merge_file_1() is a very unhelpful name.  Rename it to
merge_mode_and_contents() to reflect what it does.

merge_content() calls merge_mode_and_contents() to do the main part of
its work, but most of this function was about higher level stuff, e.g.
printing out conflict messages, updating skip_worktree bits, checking
for ability to avoid updating the working tree or for D/F conflicts
being in the way, etc.  Since there are several handle_*() functions for
similar levels of checking and handling in merge-recursive.c (e.g.
handle_change_delete(), handle_rename_rename_2to1()), let's rename this
function to handle_content_merge().

Signed-off-by: Elijah Newren 
---
 merge-recursive.c | 66 ---
 1 file changed, 34 insertions(+), 32 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 2654a8a485..5206d6cfb6 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1274,14 +1274,14 @@ static int merge_submodule(struct merge_options *o,
return 0;
 }
 
-static int merge_file_1(struct merge_options *o,
-   const struct diff_filespec *one,
-   const struct diff_filespec *a,
-   const struct diff_filespec *b,
-   const char *filename,
-   const char *branch1,
-   const char *branch2,
-   struct merge_file_info *result)
+static int merge_mode_and_contents(struct merge_options *o,
+  const struct diff_filespec *one,
+  const struct diff_filespec *a,
+  const struct diff_filespec *b,
+  const char *filename,
+  const char *branch1,
+  const char *branch2,
+  struct merge_file_info *result)
 {
result->merge = 0;
result->clean = 1;
@@ -1609,8 +1609,8 @@ static int handle_rename_rename_1to2(struct merge_options 
*o,
struct merge_file_info mfi;
struct diff_filespec other;
struct diff_filespec *add;
-   if (merge_file_1(o, one, a, b, one->path,
-ci->branch1, ci->branch2, ))
+   if (merge_mode_and_contents(o, one, a, b, one->path,
+   ci->branch1, ci->branch2, ))
return -1;
 
/*
@@ -1676,10 +1676,10 @@ static int handle_rename_rename_2to1(struct 
merge_options *o,
 
path_side_1_desc = xstrfmt("%s (was %s)", path, a->path);
path_side_2_desc = xstrfmt("%s (was %s)", path, b->path);
-   if (merge_file_1(o, a, c1, >ren1_other, path_side_1_desc,
-o->branch1, o->branch2, _c1) ||
-   merge_file_1(o, b, >ren2_other, c2, path_side_2_desc,
-o->branch1, o->branch2, _c2))
+   if (merge_mode_and_contents(o, a, c1, >ren1_other, path_side_1_desc,
+   o->branch1, o->branch2, _c1) ||
+   merge_mode_and_contents(o, b, >ren2_other, c2, path_side_2_desc,
+   o->branch1, o->branch2, _c2))
return -1;
free(path_side_1_desc);
free(path_side_2_desc);
@@ -2723,9 +2723,9 @@ static int process_renames(struct merge_options *o,
b.mode = dst_other.mode;
b.path = one.path;
 
-   if (merge_file_1(o, , , , 
ren1_dst,
-branch1, branch2,
-)) {
+   if (merge_mode_and_contents(o, , 
, , ren1_dst,
+   branch1, 
branch2,
+   )) {
clean_merge = -1;
goto cleanup_and_return;
}
@@ -2975,13 +2975,13 @@ static int handle_modify_delete(struct merge_options *o,
_("modify"), _("modified"));
 }
 
-static int merge_content(struct merge_options *o,
-const char *path,
-int is_dirty,
-struct object_id *o_oid, int o_mode,
-struct object_id *a_oid, int a_mode,
-struct object_id *b_oid, int b_mode,
-struct rename_conflict_info *rename_conflict_info)
+static int handle_content_merge(struct merge_options *o,
+   const char *path,
+   

[PATCH 1/4] merge-recursive: set paths correctly when three-way merging content

[Elijah Newren]

merge_3way() has code to mark different sides of the conflict with info
about where the content comes from.  If the names of the files involved
match, it simply uses the branch name.  If the names of the files do not
match, it uses branchname:filename.  Unfortunately, merge_content()
previously always called it with one.path = a.path = b.path.  Granted,
it didn't have other path information available to it for years, but
that was corrected by passing rename_conflict_info in commit
3c217c077a86 ("merge-recursive: Provide more info in conflict markers
with file renames", 2011-08-11).  In that commit, instead of just fixing
the bug with the pathnames, it created fake branch names incorporating
both the branch name and file name.

This "fake branch" workaround was extended further when I pulled that
logic out into a special function in commit dac4741554e7
("merge-recursive: Create function for merging with branchname:file
markers", 2011-08-11), and a number of other sites outside of
merge_content() have been added which call into that.  However, this
Rube-Goldberg-esque setup is not merely duplicate code and unnecessary
work, it also risked having other callsites invoke it in a way that
would result in markers of the form branchname:filename:filename (i.e.
with the filename repeated).

Fix this whole mess by:
  - setting one.path, a.path, and b.path appropriately
  - calling merge_file_1() directly
  - deleting the merge_file_special_markers() workaround wrapper

Signed-off-by: Elijah Newren 
---
 merge-recursive.c | 49 +--
 1 file changed, 9 insertions(+), 40 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 45a163c555..99a7ac5ec7 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1366,35 +1366,6 @@ static int merge_file_1(struct merge_options *o,
return 0;
 }
 
-static int merge_file_special_markers(struct merge_options *o,
- const struct diff_filespec *one,
- const struct diff_filespec *a,
- const struct diff_filespec *b,
- const char *target_filename,
- const char *branch1,
- const char *filename1,
- const char *branch2,
- const char *filename2,
- struct merge_file_info *mfi)
-{
-   char *side1 = NULL;
-   char *side2 = NULL;
-   int ret;
-
-   if (filename1)
-   side1 = xstrfmt("%s:%s", branch1, filename1);
-   if (filename2)
-   side2 = xstrfmt("%s:%s", branch2, filename2);
-
-   ret = merge_file_1(o, one, a, b, target_filename,
-  side1 ? side1 : branch1,
-  side2 ? side2 : branch2, mfi);
-
-   free(side1);
-   free(side2);
-   return ret;
-}
-
 static int merge_file_one(struct merge_options *o,
  const char *path,
  const struct object_id *o_oid, int o_mode,
@@ -1729,14 +1700,10 @@ static int handle_rename_rename_2to1(struct 
merge_options *o,
 
path_side_1_desc = xstrfmt("%s (was %s)", path, a->path);
path_side_2_desc = xstrfmt("%s (was %s)", path, b->path);
-   if (merge_file_special_markers(o, a, c1, >ren1_other,
-  path_side_1_desc,
-  o->branch1, c1->path,
-  o->branch2, ci->ren1_other.path, 
_c1) ||
-   merge_file_special_markers(o, b, >ren2_other, c2,
-  path_side_2_desc,
-  o->branch1, ci->ren2_other.path,
-  o->branch2, c2->path, _c2))
+   if (merge_file_1(o, a, c1, >ren1_other, path_side_1_desc,
+o->branch1, o->branch2, _c1) ||
+   merge_file_1(o, b, >ren2_other, c2, path_side_2_desc,
+o->branch1, o->branch2, _c2))
return -1;
free(path_side_1_desc);
free(path_side_2_desc);
@@ -3059,14 +3026,16 @@ static int merge_content(struct merge_options *o,
path2 = (rename_conflict_info->pair2 ||
 o->branch2 == rename_conflict_info->branch1) ?
pair1->two->path : pair1->one->path;
+   one.path = pair1->one->path;
+   a.path = (char *)path1;
+   b.path = (char *)path2;
 
if (dir_in_way(path, !o->call_depth,
   S_ISGITLINK(pair1->two->mode)))
df_conflict_remains = 1;
}
-   if (merge_file_special_markers(o, , , , path,
-  o->branch1, path1,
-  o->branch2, path2, ))
+   if 

[PATCH 0/4] Cleanup of merge_*() functions in merge-recursive

[Elijah Newren]

While working on a series to make file collision conflict handling
consistent, I discovered a latent bug in merge_content()...and several
opportunities for cleanup.  This series fixes that bug, deletes the
merge_file_special_markers() and merge_file_one() functions, and
renames a pair of functions to make them have a better description.

Elijah Newren (4):
  merge-recursive: set paths correctly when three-way merging content
  merge-recursive: avoid wrapper function when unnecessary and wasteful
  merge-recursive: remove final remaining caller of merge_file_one()
  merge-recursive: rename merge_file_1() and merge_content()

 merge-recursive.c | 144 --
 1 file changed, 51 insertions(+), 93 deletions(-)

-- 
2.19.0.12.gc6760fd9a9

[PATCH 2/4] merge-recursive: avoid wrapper function when unnecessary and wasteful

[Elijah Newren]

merge_file_one() is a convenience function taking a bunch of oids and
modes, combining each pair into a diff_filespec, and then calling
merge_file_1().  When we already start with diff_filespec's, we can
just call merge_file_1() directly instead of splitting out the oids
and modes for the wrapper to recombine into what we already had.

Signed-off-by: Elijah Newren 
---
 merge-recursive.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 99a7ac5ec7..9e4e3da672 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1630,10 +1630,7 @@ static int handle_rename_rename_1to2(struct 
merge_options *o,
struct merge_file_info mfi;
struct diff_filespec other;
struct diff_filespec *add;
-   if (merge_file_one(o, one->path,
->oid, one->mode,
->oid, a->mode,
->oid, b->mode,
+   if (merge_file_1(o, one, a, b, one->path,
 ci->branch1, ci->branch2, ))
return -1;
 
-- 
2.19.0.12.gc6760fd9a9

[PATCH 3/4] merge-recursive: remove final remaining caller of merge_file_one()

[Elijah Newren]

The function names merge_file_one() and merge_file_1() aren't
particularly intuitive function names, especially since there is no
associated merge_file() function that these are related to.  The
previous commit showed that merge_file_one() was prone to be called when
merge_file_1() should be, and since it is just a thin wrapper around
merge_file_1() anyway and only has one caller left, let's just remove
merge_file_one() entirely.

(It also turns out that the one remaining caller of merge_file_one()
has very broken code that needs to be completely rewritten, but that's
the subject of a future patch series; for now, we're just translating
it into a merge_file_1() call.)

Signed-off-by: Elijah Newren 
---
 merge-recursive.c | 44 +---
 1 file changed, 17 insertions(+), 27 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 9e4e3da672..2654a8a485 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1366,27 +1366,6 @@ static int merge_file_1(struct merge_options *o,
return 0;
 }
 
-static int merge_file_one(struct merge_options *o,
- const char *path,
- const struct object_id *o_oid, int o_mode,
- const struct object_id *a_oid, int a_mode,
- const struct object_id *b_oid, int b_mode,
- const char *branch1,
- const char *branch2,
- struct merge_file_info *mfi)
-{
-   struct diff_filespec one, a, b;
-
-   one.path = a.path = b.path = (char *)path;
-   oidcpy(, o_oid);
-   one.mode = o_mode;
-   oidcpy(, a_oid);
-   a.mode = a_mode;
-   oidcpy(, b_oid);
-   b.mode = b_mode;
-   return merge_file_1(o, , , , path, branch1, branch2, mfi);
-}
-
 static int handle_rename_via_dir(struct merge_options *o,
 struct diff_filepair *pair,
 const char *rename_branch,
@@ -2730,12 +2709,23 @@ static int process_renames(struct merge_options *o,
   ren1_dst, branch2);
if (o->call_depth) {
struct merge_file_info mfi;
-   if (merge_file_one(o, ren1_dst, 
_oid, 0,
-  
>pair->two->oid,
-  
ren1->pair->two->mode,
-  _other.oid,
-  dst_other.mode,
-  branch1, branch2, 
)) {
+   struct diff_filespec one, a, b;
+
+   oidcpy(, _oid);
+   one.mode = 0;
+   one.path = ren1->pair->two->path;
+
+   oidcpy(, >pair->two->oid);
+   a.mode = ren1->pair->two->mode;
+   a.path = one.path;
+
+   oidcpy(, _other.oid);
+   b.mode = dst_other.mode;
+   b.path = one.path;
+
+   if (merge_file_1(o, , , , 
ren1_dst,
+branch1, branch2,
+)) {
clean_merge = -1;
goto cleanup_and_return;
}
-- 
2.19.0.12.gc6760fd9a9

Re: [PATCH v2 0/1] Make 'git commit' not accidentally lose staged content

[Duy Nguyen]

On Wed, Sep 19, 2018 at 1:19 AM Jeff King  wrote:
>
> On Tue, Sep 18, 2018 at 12:36:06PM -0700, Jacob Keller wrote:
>
> > > I like that, too. It's a little more costly just because it may involve
> > > object-db writes, but I think in most cases it would be fine. I almost
> > > always "git stash" away discarded changes these days instead of "git
> > > reset --hard", because it effectively provides this kind of log.
> >
> > Obviously we do eventually turn the index into a tree, which is used
> > by the commit. Would it be possible to simply somehow store these
> > trees, and have commands which blow the tree away simply instead, save
> > it? I'm not sure how costly that is.
>
> For an up-to-date index with the cache-tree extension, this should be
> pretty cheap.  Even for a partially-staged index, where we already have
> the blob in the object database, it should just incur the tree
> computation (and parts you didn't touch would hopefully have an
> up-to-date cache-tree).

Just FYI. I wanted to get at least pruning support in place before
posting another RFC. But right now I'm going without cache-tree.
Whenever a file is updated, the _blob_ hashes are recorded in the
index log (one line per update, same as reflog format) and the
description part of the line is the updated path.

This is simpler to do, and we can still reconstruct a tree/commit in
memory if we need to (but not whole tree snapshots; but I don't think
that would be very useful). But maybe cache-tree approach is better.
We will see.
-- 
Duy

Re: Git 2.19 ignores default system language in Mac (2.18 or earlier did not)

[Elijah Newren]

On Wed, Sep 19, 2018 at 7:24 AM Vasily Korytov
 wrote:
>
> Hi,
>
> $ locale
> LANG=
> LC_COLLATE="C"
> LC_CTYPE="UTF-8"
> LC_MESSAGES="C"
> LC_MONETARY="C"
> LC_NUMERIC="C"
> LC_TIME="C"
> LC_ALL=
> $ uname -mrs
> Darwin 16.7.0 x86_64
> $ git --version
> git version 2.19.0
>
> In Mac’s system preferences I have three languages: English (default), 
> Russian, Ukrainian.
>
> After update to Git 2.19 Git’s output suddenly appeared in Russian.
> I can use `export LANG=en_US.UTF-8` to switch it to English back, but it’s 
> very weird.
>
> Did not find any related things in changelog, so assuming this is a bug.

According to the thread at
https://public-inbox.org/git/CAPig+cQWW9kibWYKu5oRDgo_Pt4wVmzkqzbTG=ygvwqrcxc...@mail.gmail.com/,
this appears to be a bug in how brew changed its builds of git, and
also affects packages besides git.

Re: [PATCH v2 1/6] CodingGuidelines: add shell piping guidelines

[Junio C Hamano]

Matthew DeVore  writes:

> Yes, it's probably better to add a point about that. Here is the new
> documentation after applying your suggestions:
>
>  - If a piped sequence which spans multiple lines, put each statement
>on a separate line and put pipes on the end of each line, rather
>than the start. This means you don't need to use \ to join lines,
>since | implies a join already.
>
> (incorrect)
> grep blob verify_pack_result \
> | awk -f print_1.awk \
> | sort >actual &&
> ...
>
> (correct)
> grep blob verify_pack_result |
> awk -f print_1.awk |
> sort >actual &&
> ...

The formatting advice to place '|' at the end applies equally to
'&&' and '||' because these three syntactic elements share exactly
the same trait: the shell knows you haven't finished speaking when
it sees them at the end of the line and keeps listening, and humans
would know that too, so there is no need for explicitly continuing
the line with backslash.

Organizationally speaking, I wonder if the above about formatting
would better appear separate from the latter two points that are
about semantics.

>  - In a pipe, any exit codes returned by processes besides the last
>are ignored. This means that if git crashes at the beginning or
>middle of a pipe, it may go undetected. Prefer writing the output
>of that command to a temporary file with '>' rather than pipe it.
>
>  - The $(git ...) construct also discards git's exit code, so if the
>goal is to test that particular command, redirect its output to a
>temporary file rather than wrap it with $( ).

Git 2.19 ignores default system language in Mac (2.18 or earlier did not)

[Vasily Korytov]

Hi,

$ locale
LANG=
LC_COLLATE="C"
LC_CTYPE="UTF-8"
LC_MESSAGES="C"
LC_MONETARY="C"
LC_NUMERIC="C"
LC_TIME="C"
LC_ALL=
$ uname -mrs
Darwin 16.7.0 x86_64
$ git --version
git version 2.19.0

In Mac’s system preferences I have three languages: English (default), Russian, 
Ukrainian.

After update to Git 2.19 Git’s output suddenly appeared in Russian.
I can use `export LANG=en_US.UTF-8` to switch it to English back, but it’s very 
weird.

Did not find any related things in changelog, so assuming this is a bug.

[PATCH] reflog expire: add progress output

[Ævar Arnfjörð Bjarmason]

Before this change the "git reflog expire" command didn't report any
progress. This is the second command (after "pack-refs --all --prune")
that the "gc" command will run.

On small repositories like this command won't take long to run, my
test system it takes just under 1 second to run on git.git, but just
around 8 seconds on linux.git, and much longer on the
2015-04-03-1M-git.git[1] large test repository.

Taking so long means that "gc" will appear to hang at the beginning of
its run. It might still do so after this change if the earlier
"pack-refs" command takes a really long time to run, but that'll only
impact repositories with a really large set of refs to pack, and can
be addressed in some future change.

One thing that's bad about this change is that we might *in theory*
print a "Marking unreachable commits in reflog for expiry" message for
each ref with a reflog. This is because the abbreviated callstack
looks like this:

0  mark_reachable at builtin/reflog.c:227
1  in unreachable at builtin/reflog.c:290
2  in should_expire_reflog_ent at builtin/reflog.c:317
3  in expire_reflog_ent at refs/files-backend.c:2956
4  in show_one_reflog_ent at refs/files-backend.c:1879
# This is the last function that has the refname (e.g. "HEAD") available
5  in files_for_each_reflog_ent at refs/files-backend.c:2025
6  in refs_for_each_reflog_ent at refs.c:2066
7  in files_reflog_expire at refs/files-backend.c:3043
8  in refs_reflog_expire at refs.c:2117
9  in reflog_expire at refs.c:2129
# Here's where we collect reflogs to expire, and expire each one
10 in cmd_reflog_expire  at builtin/reflog.c:595

I.e. this progress is being reported for each expired reflog. So if
start_progress() were used instead of start_delayed_progress() we'd
print (e.g. on my git.git) hundreds of these lines.

In practice I haven't been able to make it print anything except one
line. This is because validating the reflogs for these other
branches (not "HEAD") takes such a short amount of time.

That may just be some artifact of the repositories I've tested, but I
suspect It'll be true in general. As the callstack above shows, in
order to guarantee that we don't do that we'd need to pass some
"progress" variable through 10 levels of functions, many of which are
"for_each" callback functions with void* cb_data.

1. https://github.com/avar/2015-04-03-1M-git

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 builtin/reflog.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 3acef5a0ab..d3075ee75a 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -10,6 +10,7 @@
 #include "diff.h"
 #include "revision.h"
 #include "reachable.h"
+#include "progress.h"
 
 /* NEEDSWORK: switch to using parse_options */
 static const char reflog_expire_usage[] =
@@ -225,14 +226,20 @@ static void mark_reachable(struct expire_reflog_policy_cb 
*cb)
struct commit_list *pending;
timestamp_t expire_limit = cb->mark_limit;
struct commit_list *leftover = NULL;
+   struct progress *progress = NULL;
+   int i = 0;
 
for (pending = cb->mark_list; pending; pending = pending->next)
pending->item->object.flags &= ~REACHABLE;
 
pending = cb->mark_list;
+   progress = start_delayed_progress(
+   _("Marking unreachable commits in reflog for expiry"), 0);
while (pending) {
struct commit_list *parent;
struct commit *commit = pop_commit();
+
+   display_progress(progress, ++i);
if (commit->object.flags & REACHABLE)
continue;
if (parse_commit(commit))
@@ -253,6 +260,7 @@ static void mark_reachable(struct expire_reflog_policy_cb 
*cb)
}
}
cb->mark_list = leftover;
+   stop_progress();
 }
 
 static int unreachable(struct expire_reflog_policy_cb *cb, struct commit 
*commit, struct object_id *oid)
-- 
2.19.0.444.g18242da7ef

Proposal

[Ms.Lev]

I wish to discuss a proposal with you, please contact me via email for more 
details immediately.

Re: [PATCH v2 6/6] t9109-git-svn-props.sh: split up several pipes

[Eric Sunshine]

On Tue, Sep 18, 2018 at 10:56 PM Matthew DeVore  wrote:
> On Mon, Sep 17, 2018 at 6:57 PM Eric Sunshine  wrote:
> > On Mon, Sep 17, 2018 at 6:25 PM Matthew DeVore  wrote:
> > > +   cmp - $3
> >
> > This is just wrong. The "-" argument to 'cmp' says to read from
> > standard input, but there is nothing being passed to 'cmp' on standard
> > input anymore now that you're removed the pipe. I'm guessing that you
> > really meant to use "observed" here (and reverse the order of
> > arguments to be consistent with the expect-then-actual idiom).
> > Finally, since these (apparently) might be binary, you can use
> > test_cmp_bin() instead.
> Fixed, except for the test_cmp_bin part. My understanding is that git
> svn propget is supposed to be printing human-readable strings.

If so, then please use test_cmp() rather than raw 'cmp' since
test_cmp() will show the actual difference between the expected and
actual files, which can be helpful when diagnosing a failing test.

> > After this patch, the test is even more broken than appears at first
> > glance since the test body is inside double-quotes. This means that
> > the $1, $2, $3 inside the test_propget() function are getting expanded
> > _before_ the function itself is ever defined, to whatever bogus values
> > $1, $2, $3 hold at that point. I can't see how this could ever have
> > worked (except only appearing to work by pure accident).
> Fixed, and here is the new test:
>
> test_expect_success 'test propget' "
> test_propget () {
> git svn propget \$1 \$2 >actual &&
> cmp \$3 actual
> } &&

Rather than escaping "$" with backslash, a cleaner fix would be to
change the double quotes around the test body to single quotes. Those
double quotes weren't needed anyhow since there are no variable
interpolations in the body. Single quotes would make that obvious at a
glance in addition to avoiding unexpected behavior in the future (like
$1, $2, etc. being interpolated at the wrong time). Single quotes
would also make the test more idiomatic and consistent with the bulk
of other tests in the suite. If you do go the route of swapping
quotes, please be sure to mention the change in the commit message.

Thanks.

Re: [PATCH v2 1/6] CodingGuidelines: add shell piping guidelines

[Eric Sunshine]

On Tue, Sep 18, 2018 at 10:11 PM Matthew DeVore  wrote:
> Yes, it's probably better to add a point about that. Here is the new
> documentation after applying your suggestions:
>
>  - If a piped sequence which spans multiple lines, put each statement

s/which//

>on a separate line and put pipes on the end of each line, rather
>than the start. This means you don't need to use \ to join lines,
>since | implies a join already.
> [...]
>  - In a pipe, any exit codes returned by processes besides the last
>are ignored. This means that if git crashes at the beginning or
>middle of a pipe, it may go undetected. Prefer writing the output
>of that command to a temporary file with '>' rather than pipe it.
>
>  - The $(git ...) construct also discards git's exit code, so if the
>goal is to test that particular command, redirect its output to a
>temporary file rather than wrap it with $( ).

This all sounds better.

[ANNOUNCE] Git Rev News edition 43

[Christian Couder]

Hi everyone,

The 43rd edition of Git Rev News is now published:

  https://git.github.io/rev_news/2018/09/19/edition-43/

Thanks a lot to the contributors: Johannes Schindelin and Luca Milanesio.!

Enjoy,
Christian, Jakub, Markus and Gabriel.