Re: [PATCH/RFC] completion: complete all possible -no-

[Eric Sunshine]

On Tue, Apr 17, 2018 at 2:13 PM, Nguyễn Thái Ngọc Duy  wrote:
> The problem with completing --no- form is that the number of
> completable options now usually doubles, taking precious screen space
> and also making it hard to find the option you want.
>
> So the other half of this patch, the part in git-completion.bash, is
> to uncomplete --no- options. When you do "git checkout --",
> instead of displaying all --no- options, this patch simply displays
> one item: the --no- prefix. If you do "git checkout --no-" then
> all negative options are displayed. This helps reduce completable
> options quite efficiently.
>
> After all this "git checkout --" now looks like this
>
> > ~/w/git $ git co --
> --conflict=   --orphan=
> --detach  --ours
> --ignore-other-worktrees  --patch
> --ignore-skip-worktree-bits   --progress
> --merge   --quiet
> --no- --recurse-submodules
> --no-detach   --theirs
> --no-quiet--track
> --no-track

I haven't looked at the implementation, so this may be an entirely
stupid suggestion, but would it be possible to instead render the
completions as?

% git checkout --
--[no-]conflict=   --[no-]patch
--[no-]detach  --[no-]progress
--[no-]ignore-other-worktrees  --[no-]quiet
--[no-]ignore-skip-worktree-bits   --[no-]recurse-submodules
--[no-]merge   --theirs
--[no-]orphan= --[no-]track
--ours

This would address the problem of the --no-* options taking double the
screen space.

It's also more intuitive than that lone and somewhat weird-looking
"--no-" suggestion.

Re: [PATCH v7 0/4] worktree: teach "add" to check out existing branches

[Eric Sunshine]

On Sun, Apr 15, 2018 at 4:29 PM, Thomas Gummerer  wrote:
> The main change once again in this series is the user interface.  It
> feels like we went in a complete circle here now, as this iteration is
> bringing the "Preparing ..." line back (although in a slightly
> different form than the original), and is moving away from printing
> it's own "HEAD is now at..." line again.  This also means we don't
> need the new hidden option to 'git reset' anymore, which is nice.

I'm glad to see the proposed hidden git-reset option go away, and am
likewise happy to see that worktree no longer wants to print "HEAD is
now at" itself. I'm much more pleased with the direction this series
is now taking than in earlier rounds. It's also much simpler, which is
a nice plus.

> I do like the new UI more than what we had in the last round (which I
> already liked more than the original UI) :)
>
> To demonstrate the UI updates, let's compare the new UI and the old UI
> again.  This is the same commands as used for the demonstration in the
> last iteration, so please have a look at 
> <20180331151804.30380-1-t.gumme...@gmail.com>
> for an example of what it looked like after the last round.

Thanks for presenting examples of the new UI under various conditions.
Like you, I find the new "Preparing..." message superior to and much
more useful than the original.

Aside from the problem pointed out in my review of 2/4 in which it
incorrectly shows "detached HEAD" for "git worktree add wt
existing-local-branch", I think this series is just about ready, and
hope to see it land with the next re-roll.

Thanks for working on it.

Re: [PATCH v7 2/4] worktree: improve message when creating a new worktree

[Eric Sunshine]

On Sun, Apr 15, 2018 at 4:29 PM, Thomas Gummerer  wrote:
> Currently 'git worktree add' produces output like the following:
>
> Preparing ../foo (identifier foo)
> HEAD is now at 26da330922 
> [...]
> Instead of this message, print a message that gives the user a bit more
> detail of what exactly 'git worktree' is doing.  There are various dwim
> modes which are perform some magic under the hood, which should be
> helpful to users.  Just from the output of the command it is not always
> visible to users what exactly has happened.
>
> Help the users a bit more by modifying the "Preparing ..." message and
> adding some additional information of what 'git worktree add' did under
> the hood, while not displaying the identifier anymore.
>
> Currently this ends up in three different cases:
>
>   - 'git worktree add -b ...' or 'git worktree add ' [...]
>
>   - 'git worktree add -B ...', which may either create a new branch if
> the branch with the given name does not exist yet, or resets an
> existing branch to the current HEAD, or the commit-ish given.
> Depending on which action is taken, we'll end up with the following
> output:
>
>   Preparing worktree (resetting branch 'next' (was at caa68db14))
>   HEAD is now at 26da330922 

The (...) embedded inside another (...) is ugly and hard to read.
Better perhaps:

Preparing worktree (resetting branch 'next'; was at caa68db14)

Not necessarily worth a re-roll. It would be nice to see this series
land; perhaps this can be tweaked later.

> or:
>
>   Preparing worktree (new branch '')
>   HEAD is now at 26da330922 
>
>   - 'git worktree add --detach' or 'git worktree add  ',
> both of which create a new worktree with a detached HEAD, for which
> we will print the following output:
>
>   Preparing worktree (detached HEAD 26da330922)
>   HEAD is now at 26da330922 

This is inaccurate, isn't it? Certainly, specifying something like
"origin/floop" for  ends up detached:

% git worktree add w1 origin/floop
...
% git worktree list
/proj fe0a9eaf31 [master]
/proj/w1  b46fe60e1d (detached HEAD)

but specifying an existing local branch (say "wip") does not end up detached:

% git worktree add w2 wip
...
% git worktree list
/proj fe0a9eaf31 [master]
/proj/w1  b46fe60e1d (detached HEAD)
/proj/w2  820ed2a513 [wip]

> Additionally currently the "Preparing ..." line is printed to stderr,
> while the "HEAD is now at ..." line is printed to stdout by 'git reset
> --hard', which is used internally by 'git worktree add'.  Fix this
> inconsistency by printing the "Preparing ..." message to stdout as
> well.  As "Preparing ..." is not an error, stdout also seems like the
> more appropriate output stream.
>
> Helped-by: Eric Sunshine 
> Signed-off-by: Thomas Gummerer 

Re: Git enhancement request - checkout (clone) set modified dates to commit date

[Jacob Keller]

On Sun, Apr 22, 2018 at 5:23 PM, Andrew Wolfe  wrote:
> Kevin, thanks for your feedback.
>
> You have a reasonable point, because usually you don't put the outputs of a 
> build into version control, but the build script checks them for being 
> current.
>
> On the other hand, when you change branches, your existing output directories 
> are worthless problems anyway — even if you have all the interdependencies 
> correct.  Because of this, I'm inclined to consider this use case as 
> intrinsically hazardous.  When I do a checkout, I always want to purge all 
> the intermediate and end targets regardless.

Not every build has this problem, and certainly I think some of the
most common build software would not (Make). It's fairly easy to fix
this by using a git hook to update files post checkout (you can look
up the timestamp of each file's commit time, or any other time and use
touch to do this yourself).

Thanks,
Jake

Re: [PATCH v2 2/6] grep.c: take column number as argument to show_line()

[Eric Sunshine]

On Sun, Apr 22, 2018 at 9:17 PM, Taylor Blau  wrote:
> On Sun, Apr 22, 2018 at 08:16:12PM -0400, Eric Sunshine wrote:
>> One way to achieve a more coherent patch series would be to build the
>> machinery first and then expose it to the user in various ways. Also,
>> each patch which implements some user-facing functionality should also
>> document that functionality. For instance, a more understandable
>> series might be structured something like this:
>>
>>   1. grep: match_line: expose matched column
>>   2. grep: extend grep_opt to allow showing matched column
>>   3. grep: display column number of first match
>>   4. builtin/grep: add --column-number option
>>   5. grep: add configuration variables to show matched column
>>   6. contrib/git-jump: jump to match column in addition to line
>
> I appreciate the suggestion, and did not take it as harsh. I think that
> the existing structure is OK, but I think that I am biased since I'm the
> author of the series. I have given your proposed structure a shot and I
> think that it should improve the readability of this series for others
> on the list as well.
>
> To avoid sending more email than is necessary, I have pushed a draft of
> this series to my copy of Git, and would greatly appreciate your
> feedback on whether the structure of these patches is sensible. If they
> all seem OK to you, I'll happily push v3.
>
> For your consideration: https://github.com/ttaylorr/git/compare/tb/grep-colno

Thanks. Perhaps not surprisingly, I find the restructured patch series
easier to follow and review. Each patch has a very specific purpose,
thus demands lower review-time cognitive load than with the old
structure.

One important issue I noticed is that patch 3/7 neglects to update
grep.c:init_grep_defaults() to initialize opt.color_columnno.

Looking at the tests again, are you gaining anything by placing them
inside that for-loop? (Genuine question.)

Re: [PATCH v2 2/6] grep.c: take column number as argument to show_line()

[Taylor Blau]

On Sun, Apr 22, 2018 at 08:16:12PM -0400, Eric Sunshine wrote:
> On Sun, Apr 22, 2018 at 4:47 PM, Taylor Blau  wrote:
> > show_line() currently receives the line number within the
> > 'grep_opt->buf' in order to determine which line number to display. In
> > order to display information about the matching column number--if
> > requested--we must additionally take in that information.
> >
> > To do so, we extend the signature of show_line() to take in an
> > additional unsigned "cno". "cno" is either:
> >
> >   * A 1-indexed column number of the first match on the given line, or
> >   * 0, if the column number is irrelevant (when displaying a function
> > name, context lines, etc).
>
> This information about how 'cno' is interpreted seems important enough
> to have as an in-code comment somewhere. Unfortunately, this patch
> never actually uses 'cno', so it's hard to add such a comment to
> non-existent code. In fact, the granularity of this patch feels wrong;
> it seems to exist for some purpose but, at the same time, is a
> do-nothing patch.
>
> This issue illustrates a larger problem with how this patch series is
> structured overall. In his review, Ævar suggested collapsing several
> patches into one, but the problems go deeper than that when you have
> patches which implement some bit of functionality but don't document
> that functionality until some later step which exposes some other bit
> of functionality, and so forth. As a reviewer, I expect a patch series
> to hold my hand and lead me on a straightforward journey from building
> blocks to final product, but this series tends to jump around without
> apparent logic.
>
> One way to achieve a more coherent patch series would be to build the
> machinery first and then expose it to the user in various ways. Also,
> each patch which implements some user-facing functionality should also
> document that functionality. For instance, a more understandable
> series might be structured something like this:
>
>   1. grep: match_line: expose matched column
>   2. grep: extend grep_opt to allow showing matched column
>   3. grep: display column number of first match
>   4. builtin/grep: add --column-number option
>   5. grep: add configuration variables to show matched column
>   6. contrib/git-jump: jump to match column in addition to line
>
> There may be fewer or more patches than shown here (I believe Ævar
> suggested a cleanup patch), but this should give the general idea.
> Patches 4 and 5 might also be swapped if that seems more logical.
>
> (Sorry if any of the above sounds harsh; it's not meant to be, but is
> intended to be constructive.)

I appreciate the suggestion, and did not take it as harsh. I think that
the existing structure is OK, but I think that I am biased since I'm the
author of the series. I have given your proposed structure a shot and I
think that it should improve the readability of this series for others
on the list as well.

To avoid sending more email than is necessary, I have pushed a draft of
this series to my copy of Git, and would greatly appreciate your
feedback on whether the structure of these patches is sensible. If they
all seem OK to you, I'll happily push v3.

For your consideration: https://github.com/ttaylorr/git/compare/tb/grep-colno

> > We additionally modify all calls to show_line() in order to pass the new
> > required argument.
>
> Nit: No need to state the obvious; this final sentence could easily be 
> dropped.

Fair; I have removed this from the patch.


Thanks,
Taylor

Re: [PATCH v2 5/6] builtin/grep.c: show column numbers via --column-number

[Taylor Blau]

On Sun, Apr 22, 2018 at 08:32:28PM -0400, Eric Sunshine wrote:
> On Sun, Apr 22, 2018 at 4:47 PM, Taylor Blau  wrote:
> > This commit teaches 'git-grep(1)' a new option, '--column-number'. This
> > option builds upon previous commits to show the column number of the
> > first match on a non-context line.
>
> Imperative mood (and dropping unnecessary "builds upon previous"):
>
> Teach 'git-grep(1)' a new option '--column-number' which shows the
> column number of the first match on a non-context line.

Thanks. I am not used to writing in this mood, but have amended my
patches locally to conform to your proposed layout and have reworded
each to be in the imperative mood.

> > For example:
> >
> >   $ ./git-grep -n --column-number foo | head -n3
> >   .clang-format:51:14:# myFunction(foo, bar, baz);
> >   .clang-format:64:7:# int foo();
> >   .clang-format:75:8:# void foo()
> >
> > Now that configuration variables such as grep.columnNumber and
> > color.grep.columnNumber have a visible effect, we document them in this
> > patch as well.
>
> As mentioned in my review of patch 2, document the configuration
> variables in the patch which introduces them.

Thanks again, I have moved this introduction to the relevant patch.

> > While we're at it, change color.grep.linenumber to color.grep.lineNumber
> > to match the casing of nearby variables.
> >
> > Signed-off-by: Taylor Blau 
> > ---
> > diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> > @@ -99,6 +99,28 @@ do
> > +   test_expect_success "grep -w $L" '
> > +   ...
> > +   '
> > +
> > +   test_expect_success "grep -w $L" '
> > +   ...
> > +   '
> > +
> > test_expect_success "grep -w $L" '
>
> I realize that several existing tests in this script are already
> guilty of this sin, but please give each new test a distinct title
> reflective of what it is actually testing in order to make it easier
> to correlate failed test output with the actual test code.

:-). I have changed this locally to indicate which is which in the hopes
that it will provide more clarity should these tests fail at any point.


Thanks,
Taylor

Re: [PATCH v2 4/6] grep.c: display column number of first match

[Taylor Blau]

On Sun, Apr 22, 2018 at 08:24:55PM -0400, Eric Sunshine wrote:
> On Sun, Apr 22, 2018 at 4:47 PM, Taylor Blau  wrote:
> > Building upon our work in the previous commit to add members 'columnnum'
> > and 'color_columno' to 'grep_opt', we teach show_line() how to respect
> > those options.
> >
> > When requested, show_line() will display the column number of the first
> > match on a non-context line. show_line() differentiates between context
> > and non-context lines through the '&& cno' check. 'cno' will be equal to
> > zero if and only if show_line() is invoked on a context line. It will be
> > a non-zero value otherwise.
>
> This interpretation of 'cno' seems important enough to deserve an
> in-code comment. (And, you may be able to drop some of the same
> information from the commit message if it merely duplicates the
> in-code comment.)

Thanks, I agree that the interpretation of 'cno' is subtle and should
not require a 'git blame' to understand. I have adopted your proposed
commit structure [1] and moved this description from the patch message
into grep.c as a comment above the relevant conditional.

Thanks,
Taylor

Re: [PATCH v2 3/6] grep.[ch]: teach columnnum, color_columnno to grep_opt

[Taylor Blau]

On Sun, Apr 22, 2018 at 08:21:33PM -0400, Eric Sunshine wrote:
> On Sun, Apr 22, 2018 at 7:24 PM, Taylor Blau  wrote:
> > On Sun, Apr 22, 2018 at 11:42:48PM +0200, Ęvar Arnfjörš Bjarmason wrote:
> >> On Sun, Apr 22 2018, Taylor Blau wrote:
> >> > In preparation of adding --column-number to 'git-grep(1)', we extend
> >> > grep_opt to take in the requisite new members.
> >>
> >> Just a nit: Makes sense to refer to these camel-cased in docs & commit
> >> messages.
> >
> > Could you clarify which? I am not sure if you mean --column-number,
> > 'git-grep(1)', or grep_opt.
>
> I think Ævar was referring to this bit from the commit message (which
> unfortunately got snipped in his review):
>
> We additionally teach the 'grep.columnnumber' and
> 'color.grep.columnnumber' configuration variables to configure
> showing and coloring the column number, respectively. (These
> options remain undocumented until 'git-grep(1)' learns the
> --column option in a forthcoming commit.)
>
> He was suggesting camel-casing 'grep.columnNumber' and
> 'color.grep.columNumber' even in the commit message.

Thanks for the clarification, Eric. I have amended my copy of this
patch to correctly case those variables.

Thanks,
Taylor

Re: Git enhancement request - checkout (clone) set modified dates to commit date

[Andrew Wolfe]

Kevin, thanks for your feedback.

You have a reasonable point, because usually you don't put the outputs of a 
build into version control, but the build script checks them for being current.

On the other hand, when you change branches, your existing output directories 
are worthless problems anyway — even if you have all the interdependencies 
correct.  Because of this, I'm inclined to consider this use case as 
intrinsically hazardous.  When I do a checkout, I always want to purge all the 
intermediate and end targets regardless.

When doing a build, it's often useful to put the current commit/branch into the 
output directories as documentation; I usually do this in my build scripts.  
This also makes it simple to detect when the branch is changed and clean the 
outputs.

- Andrew

> On Apr 22, 2018, at 3:59 PM, Kevin Daudt  wrote:
> 
> On Sun, Apr 22, 2018 at 03:01:10PM -0400, Andrew Wolfe wrote:
>> Hi Brian,
>> 
>> Not completely sure what you're saying.  If the files on master are
>> not changed, the changed files' commit timestamps will be older than
>> the branch commit timestamps.
>> 
>> However, if I check out master after committing to a branch, the
>> modifications will necessarily disappear because they haven't been
>> committed to master.  Instead, under my proposal, each will get the
>> timestamp of its prior commit.
>> 
> 
> Say I build the project while on a certain branch. Then I checkout a
> different branch and build again. You would expect that the targets of
> every source file that have changed are rebuilt.
> 
> When you would restore the timestamp back to the commit timestamp, the
> timestamp will be older then the target, and make will not rebuild it,
> leaving you with outdated build targets.

Re: [PATCH v2 5/6] builtin/grep.c: show column numbers via --column-number

[Eric Sunshine]

On Sun, Apr 22, 2018 at 4:47 PM, Taylor Blau  wrote:
> This commit teaches 'git-grep(1)' a new option, '--column-number'. This
> option builds upon previous commits to show the column number of the
> first match on a non-context line.

Imperative mood (and dropping unnecessary "builds upon previous"):

Teach 'git-grep(1)' a new option '--column-number' which shows the
column number of the first match on a non-context line.

> For example:
>
>   $ ./git-grep -n --column-number foo | head -n3
>   .clang-format:51:14:# myFunction(foo, bar, baz);
>   .clang-format:64:7:# int foo();
>   .clang-format:75:8:# void foo()
>
> Now that configuration variables such as grep.columnNumber and
> color.grep.columnNumber have a visible effect, we document them in this
> patch as well.

As mentioned in my review of patch 2, document the configuration
variables in the patch which introduces them.

> While we're at it, change color.grep.linenumber to color.grep.lineNumber
> to match the casing of nearby variables.
>
> Signed-off-by: Taylor Blau 
> ---
> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> @@ -99,6 +99,28 @@ do
> +   test_expect_success "grep -w $L" '
> +   ...
> +   '
> +
> +   test_expect_success "grep -w $L" '
> +   ...
> +   '
> +
> test_expect_success "grep -w $L" '

I realize that several existing tests in this script are already
guilty of this sin, but please give each new test a distinct title
reflective of what it is actually testing in order to make it easier
to correlate failed test output with the actual test code.

Re: [PATCH v2 4/6] grep.c: display column number of first match

[Eric Sunshine]

On Sun, Apr 22, 2018 at 4:47 PM, Taylor Blau  wrote:
> Building upon our work in the previous commit to add members 'columnnum'
> and 'color_columno' to 'grep_opt', we teach show_line() how to respect
> those options.
>
> When requested, show_line() will display the column number of the first
> match on a non-context line. show_line() differentiates between context
> and non-context lines through the '&& cno' check. 'cno' will be equal to
> zero if and only if show_line() is invoked on a context line. It will be
> a non-zero value otherwise.

This interpretation of 'cno' seems important enough to deserve an
in-code comment. (And, you may be able to drop some of the same
information from the commit message if it merely duplicates the
in-code comment.)

> Signed-off-by: Taylor Blau 
> ---
> diff --git a/grep.c b/grep.c
> @@ -1404,6 +1404,12 @@ static void show_line(struct grep_opt *opt, char *bol, 
> char *eol,
> +   if (opt->columnnum && cno) {
> +   char buf[32];
> +   xsnprintf(buf, sizeof(buf), "%d", cno);
> +   output_color(opt, buf, strlen(buf), opt->color_columnno);
> +   output_sep(opt, sign);
> +   }

Re: [PATCH v2 3/6] grep.[ch]: teach columnnum, color_columnno to grep_opt

[Eric Sunshine]

On Sun, Apr 22, 2018 at 7:24 PM, Taylor Blau  wrote:
> On Sun, Apr 22, 2018 at 11:42:48PM +0200, Ęvar Arnfjörš Bjarmason wrote:
>> On Sun, Apr 22 2018, Taylor Blau wrote:
>> > In preparation of adding --column-number to 'git-grep(1)', we extend
>> > grep_opt to take in the requisite new members.
>>
>> Just a nit: Makes sense to refer to these camel-cased in docs & commit
>> messages.
>
> Could you clarify which? I am not sure if you mean --column-number,
> 'git-grep(1)', or grep_opt.

I think Ævar was referring to this bit from the commit message (which
unfortunately got snipped in his review):

We additionally teach the 'grep.columnnumber' and
'color.grep.columnnumber' configuration variables to configure
showing and coloring the column number, respectively. (These
options remain undocumented until 'git-grep(1)' learns the
--column option in a forthcoming commit.)

He was suggesting camel-casing 'grep.columnNumber' and
'color.grep.columNumber' even in the commit message.

Re: [PATCH v2 2/6] grep.c: take column number as argument to show_line()

[Eric Sunshine]

On Sun, Apr 22, 2018 at 4:47 PM, Taylor Blau  wrote:
> show_line() currently receives the line number within the
> 'grep_opt->buf' in order to determine which line number to display. In
> order to display information about the matching column number--if
> requested--we must additionally take in that information.
>
> To do so, we extend the signature of show_line() to take in an
> additional unsigned "cno". "cno" is either:
>
>   * A 1-indexed column number of the first match on the given line, or
>   * 0, if the column number is irrelevant (when displaying a function
> name, context lines, etc).

This information about how 'cno' is interpreted seems important enough
to have as an in-code comment somewhere. Unfortunately, this patch
never actually uses 'cno', so it's hard to add such a comment to
non-existent code. In fact, the granularity of this patch feels wrong;
it seems to exist for some purpose but, at the same time, is a
do-nothing patch.

This issue illustrates a larger problem with how this patch series is
structured overall. In his review, Ævar suggested collapsing several
patches into one, but the problems go deeper than that when you have
patches which implement some bit of functionality but don't document
that functionality until some later step which exposes some other bit
of functionality, and so forth. As a reviewer, I expect a patch series
to hold my hand and lead me on a straightforward journey from building
blocks to final product, but this series tends to jump around without
apparent logic.

One way to achieve a more coherent patch series would be to build the
machinery first and then expose it to the user in various ways. Also,
each patch which implements some user-facing functionality should also
document that functionality. For instance, a more understandable
series might be structured something like this:

  1. grep: match_line: expose matched column
  2. grep: extend grep_opt to allow showing matched column
  3. grep: display column number of first match
  4. builtin/grep: add --column-number option
  5. grep: add configuration variables to show matched column
  6. contrib/git-jump: jump to match column in addition to line

There may be fewer or more patches than shown here (I believe Ævar
suggested a cleanup patch), but this should give the general idea.
Patches 4 and 5 might also be swapped if that seems more logical.

(Sorry if any of the above sounds harsh; it's not meant to be, but is
intended to be constructive.)

> We additionally modify all calls to show_line() in order to pass the new
> required argument.

Nit: No need to state the obvious; this final sentence could easily be dropped.

> Signed-off-by: Taylor Blau 

Re: What's cooking in git.git (Apr 2018, #02; Tue, 17)

[Taylor Blau]

On Mon, Apr 23, 2018 at 08:33:11AM +0900, Junio C Hamano wrote:
> Taylor Blau  writes:
>
> > I noticed that tb/config-default, however, only landed two commits:
> >
> >   - builtin/config: introduce `color` type specifier
> >   - config.c: introduce 'git_config_color' to parse ANSI colors
> >
> > but not:
> >
> >   - builtin/config: introduce `--default`
>
> Whenever something like this happens, especially the patch was
> original sent more than several days ago, it is helpful to give the
> message-id of what you are referring to to identify and retrieve it.
> More often than not, it wasn't explicitly rejected but was simply
> dropped, either by mistake or got delayed in delivery and got
> forgotten.
>
> Also for a pair of small series like these, when rerolling the
> preparatory series for the final round, it is helpful to also send
> the other series that depends on the preparatory one at the same
> time, even if the latter hasn't changed since the last time.
>
> Thanks.

Thanks, these are both helpful to know, and I will be sure to include
the relevant bits in the future.

For now, the Message-ID that I was referring to is:
20180410001826.GB67209@syl.local. [1]


Thanks,
Taylor

[1]: https://public-inbox.org/git/20180410001826.GB67209@syl.local/

Re: [PATCH v2 0/6] Teach '--column-number' to 'git-grep(1)'

[Taylor Blau]

On Mon, Apr 23, 2018 at 08:28:14AM +0900, Junio C Hamano wrote:
> Taylor Blau  writes:
>
> > diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> > index 0cf654824d..7349c7fadc 100755
> > --- a/t/t7810-grep.sh
> > +++ b/t/t7810-grep.sh
> > @@ -106,7 +106,7 @@ do
> > echo ${HC}file:5:foo mmap bar_mmap
> > echo ${HC}file:14:foo_mmap bar mmap baz
> > } >expected &&
> > -   git -c grep.linenumber=false grep -m -w -e mmap $H >actual &&
> > +   git grep --column-number -w -e mmap $H >actual &&
> > test_cmp expected actual
> > '
> >
> > @@ -117,7 +117,7 @@ do
> > echo ${HC}file:4:5:foo mmap bar_mmap
> > echo ${HC}file:5:14:foo_mmap bar mmap baz
> > } >expected &&
> > -   git -c grep.linenumber=false grep -n -m -w -e mmap $H >actual &&
> > +   git grep -n --column-number -w -e mmap $H >actual &&
> > test_cmp expected actual
> > '
>
> It seems that these two used to be "even when it is configured not
> to show linenumber, with -n it is shown and without -n it is not,
> when the new --column-number feature forces the command to show the
> "filename plus colon plus location info plus coon" header.  I'm
> guessing that the reason why these got changed this round is because
> the old way was found too defensive (perhaps nobody sets
> grep.linenumber in .git/config in the tests that come before these)?

Sort of. My aim with these new tests is to ensure that git-grep(1) works
with:

  - '--line-number'
  - '--column-number'
  - '--line-number' and '--column-number' together

It seemed unrelated to be testing the various permutations of
grep.linenumber and --line-number along with the above three so I
removed these in order to focus only on the above three.

Do you think that it is worth testing --column-number with and without
grep.columnnumber as above?


Thanks,
Taylor

Re: What's cooking in git.git (Apr 2018, #02; Tue, 17)

[Junio C Hamano]

Taylor Blau  writes:

> I noticed that tb/config-default, however, only landed two commits:
>
>   - builtin/config: introduce `color` type specifier
>   - config.c: introduce 'git_config_color' to parse ANSI colors
>
> but not:
>
>   - builtin/config: introduce `--default`

Whenever something like this happens, especially the patch was
original sent more than several days ago, it is helpful to give the
message-id of what you are referring to to identify and retrieve it.
More often than not, it wasn't explicitly rejected but was simply
dropped, either by mistake or got delayed in delivery and got
forgotten.

Also for a pair of small series like these, when rerolling the
preparatory series for the final round, it is helpful to also send
the other series that depends on the preparatory one at the same
time, even if the latter hasn't changed since the last time.

Thanks.

Re: [PATCH v2 1/6] grep.c: take regmatch_t as argument in match_line()

[Taylor Blau]

On Sun, Apr 22, 2018 at 07:14:58PM -0400, Eric Sunshine wrote:
> On Sun, Apr 22, 2018 at 4:47 PM, Taylor Blau  wrote:
> > In a subsequent patch, we teach show_line() to optionally include the
> > column number of the first match on each matched line.
> >
> > The regmatch_t involved in match_line() and match_one_pattern() both
> > contain this information (via regmatch_t->rm_so), but their current
> > implementation throws this stack variable away at the end of the call.
> >
> > Instead, let's teach match_line() to take in a 'regmatch_t *' so that
> > callers can inspect the result of their calls. This will prove useful in
> > a subsequent commit when a caller will forward on information from the
> > regmatch_t into show_line (as described above).
> >
> > The return condition remains unchanged, therefore the only change
> > required of callers is the addition of a single argument.
>
> Is 'rm_so' the only piece of information which callers will ever want
> to extract from 'regmatch_t'? If so, this patch's approach might be
> overly broad, unnecessarily exposing too much of match_lines()'s
> internal implementation. An alternative would be to narrow the
> interface and limit exposure by passing in an 'int *matched_col' or
> some such.
>
> (Not necessarily a big deal, but something to consider.)

Thanks; I do not think that this is overly broad, although I suppose
that it does bind us to the implementation of regmatch_t. The definition
of regmatch_t, however, is fairly minimal:

typedef struct
{
regoff_t rm_so;  /* Byte offset from string's start to 
substring's start.  */
regoff_t rm_eo;  /* Byte offset from string's start to 
substring's end.  */
} regmatch_t;

I can imagine that other callers would be interested in the other piece
of information, 'rm_eo', as well. Therefore, I think it makes sense to
provide access to it along with 'rm_so'.

I think that if we are concerned with being bound to regmatch_t, then we
could typedef a new struct that contains a 'start' and 'end' unsigned,
but I think that this extra effort would not be fruitful, since the
majority of grep.c assumes regmatch_t (and friends).

> > Signed-off-by: Taylor Blau 
> > ---
> > diff --git a/grep.c b/grep.c
> > @@ -1299,17 +1299,17 @@ static int match_expr(struct grep_opt *opt, char 
> > *bol, char *eol,
> >  static int match_line(struct grep_opt *opt, char *bol, char *eol,
> > - enum grep_context ctx, int collect_hits)
> > + regmatch_t *match, enum grep_context ctx,
> > + int collect_hits)
> >  {
> > struct grep_pat *p;
> > -   regmatch_t match;
> >
> > if (opt->extended)
> > return match_expr(opt, bol, eol, ctx, collect_hits);
>
> The new 'match' argument has no impact in the 'opt->extended' case.
> Perhaps this deserves calling out in the commit message.

I have noted this in the commit message, which I'll include in the next
re-roll.

Thanks,
Taylor

Re: [PATCH v2 0/6] Teach '--column-number' to 'git-grep(1)'

[Junio C Hamano]

Taylor Blau  writes:

>   * Removed '-m' as an alias for '--column-number', per René's
> suggestion [1].
>
>   * Fix some incorrect spelling of 'columnnumber'.
>
>   * Change casing of 'color.grep.linenumber' to 'color.grep.lineNumber'
> to be consistent with 'color.grep.columnNumber'. This is an
> unrelated change, and one which I am happy to drop from this series.
> It was suggested by Martin in [2].

All sounds like good updates.

> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index 0cf654824d..7349c7fadc 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -106,7 +106,7 @@ do
>   echo ${HC}file:5:foo mmap bar_mmap
>   echo ${HC}file:14:foo_mmap bar mmap baz
>   } >expected &&
> - git -c grep.linenumber=false grep -m -w -e mmap $H >actual &&
> + git grep --column-number -w -e mmap $H >actual &&
>   test_cmp expected actual
>   '
>
> @@ -117,7 +117,7 @@ do
>   echo ${HC}file:4:5:foo mmap bar_mmap
>   echo ${HC}file:5:14:foo_mmap bar mmap baz
>   } >expected &&
> - git -c grep.linenumber=false grep -n -m -w -e mmap $H >actual &&
> + git grep -n --column-number -w -e mmap $H >actual &&
>   test_cmp expected actual
>   '

It seems that these two used to be "even when it is configured not
to show linenumber, with -n it is shown and without -n it is not,
when the new --column-number feature forces the command to show the
"filename plus colon plus location info plus coon" header.  I'm
guessing that the reason why these got changed this round is because
the old way was found too defensive (perhaps nobody sets
grep.linenumber in .git/config in the tests that come before these)?

Re: [PATCH v2 6/6] contrib/git-jump/git-jump: use column number when grep-ing

[Taylor Blau]

On Sun, Apr 22, 2018 at 11:49:39PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Sun, Apr 22 2018, Taylor Blau wrote:
>
> > This patch adds the '--column-number' synonym '-m' to the default
> > grep command so that callers are brought to the correct line _and_
> > column of each matched location.
> > [...]
> > diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
> > index 80ab0590bc..2706963690 100755
> > --- a/contrib/git-jump/git-jump
> > +++ b/contrib/git-jump/git-jump
> > @@ -52,7 +52,7 @@ mode_merge() {
> >  # editor shows them to us in the status bar.
> >  mode_grep() {
> > cmd=$(git config jump.grepCmd)
> > -   test -n "$cmd" || cmd="git grep -n"
> > +   test -n "$cmd" || cmd="git grep -n -m"
> > $cmd "$@" |
> > perl -pe '
> > s/[ \t]+/ /g;
>
> So this re-roll doesn't have the alias -m anymore, but this makes use of
> it. Seems you just forgot to update this from v1, unless I'm missing
> something while skimming this...

Ack; another good catch. I have updated this in my copy and will include
it in v3.

Thanks,
Taylor

Re: [PATCH v2 5/6] builtin/grep.c: show column numbers via --column-number

[Taylor Blau]

On Sun, Apr 22, 2018 at 11:48:53PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Sun, Apr 22 2018, Taylor Blau wrote:
>
> I think this part though...
>
> > While we're at it, change color.grep.linenumber to color.grep.lineNumber
> > to match the casing of nearby variables.
> > [...]
> > -`linenumber`;;
> > +`lineNumber`;;
>
> Makes sense as its own patch at the beginning of the series, since it's
> just related cleanup.

Thanks, I have adjusted this change in my copy and will attach it in a
subsequent re-roll.

> > +`columnNumber`;;
> > +   column number prefix (when using `--column-number`)
>
> Here you're using --column-number...
>
> > +grep.columnNumber::
> > +   If set to true, enable `-m` option by default.
>
> ...But not here. This needs to be updated
>
> > +grep.columnNumber::
> > +   If set to true, enable `-m` option by default.
> > +
>
> ...ditto.

Fixed all of these, thanks for pointing them out :-).

> > +--column-number::
> > +   Prefix the 1-indexed column number of the first match on non-context 
> > lines.
> > +
> > [...]
> > OPT_GROUP(""),
> > OPT_BOOL('n', "line-number", , N_("show line 
> > numbers")),
> > +   OPT_BOOL(0, "column-number", , N_("show column 
> > numbers")),
>
> Maybe "show first matching column"? I.e. the main docs say "just shows
> the first", but this seems to give a different impression.

I settled on "show column number of first match", and have noted its use
for callers like git-jump in the documentation.

Thanks,
Taylor

Re: [PATCH v2 3/6] grep.[ch]: teach columnnum, color_columnno to grep_opt

[Taylor Blau]

On Sun, Apr 22, 2018 at 11:42:48PM +0200, Ævar Arnfjörð Bjarmason wrote:
> On Sun, Apr 22 2018, Taylor Blau wrote:
>
> > In preparation of adding --column-number to 'git-grep(1)', we extend
> > grep_opt to take in the requisite new members.
>
> Just a nit: Makes sense to refer to these camel-cased in docs & commit
> messages.

Could you clarify which? I am not sure if you mean --column-number,
'git-grep(1)', or grep_opt.


Thanks,
Taylor

Re: [PATCH v2 1/6] grep.c: take regmatch_t as argument in match_line()

[Eric Sunshine]

On Sun, Apr 22, 2018 at 4:47 PM, Taylor Blau  wrote:
> In a subsequent patch, we teach show_line() to optionally include the
> column number of the first match on each matched line.
>
> The regmatch_t involved in match_line() and match_one_pattern() both
> contain this information (via regmatch_t->rm_so), but their current
> implementation throws this stack variable away at the end of the call.
>
> Instead, let's teach match_line() to take in a 'regmatch_t *' so that
> callers can inspect the result of their calls. This will prove useful in
> a subsequent commit when a caller will forward on information from the
> regmatch_t into show_line (as described above).
>
> The return condition remains unchanged, therefore the only change
> required of callers is the addition of a single argument.

Is 'rm_so' the only piece of information which callers will ever want
to extract from 'regmatch_t'? If so, this patch's approach might be
overly broad, unnecessarily exposing too much of match_lines()'s
internal implementation. An alternative would be to narrow the
interface and limit exposure by passing in an 'int *matched_col' or
some such.

(Not necessarily a big deal, but something to consider.)

> Signed-off-by: Taylor Blau 
> ---
> diff --git a/grep.c b/grep.c
> @@ -1299,17 +1299,17 @@ static int match_expr(struct grep_opt *opt, char 
> *bol, char *eol,
>  static int match_line(struct grep_opt *opt, char *bol, char *eol,
> - enum grep_context ctx, int collect_hits)
> + regmatch_t *match, enum grep_context ctx,
> + int collect_hits)
>  {
> struct grep_pat *p;
> -   regmatch_t match;
>
> if (opt->extended)
> return match_expr(opt, bol, eol, ctx, collect_hits);

The new 'match' argument has no impact in the 'opt->extended' case.
Perhaps this deserves calling out in the commit message.

> /* we do not call with collect_hits without being extended */
> for (p = opt->pattern_list; p; p = p->next) {
> -   if (match_one_pattern(p, bol, eol, ctx, , 0))
> +   if (match_one_pattern(p, bol, eol, ctx, match, 0))
> return 1;
> }
> return 0;

Re: [PATCH v2 6/6] contrib/git-jump/git-jump: use column number when grep-ing

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 22 2018, Taylor Blau wrote:

> This patch adds the '--column-number' synonym '-m' to the default
> grep command so that callers are brought to the correct line _and_
> column of each matched location.
> [...]
> diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
> index 80ab0590bc..2706963690 100755
> --- a/contrib/git-jump/git-jump
> +++ b/contrib/git-jump/git-jump
> @@ -52,7 +52,7 @@ mode_merge() {
>  # editor shows them to us in the status bar.
>  mode_grep() {
>   cmd=$(git config jump.grepCmd)
> - test -n "$cmd" || cmd="git grep -n"
> + test -n "$cmd" || cmd="git grep -n -m"
>   $cmd "$@" |
>   perl -pe '
>   s/[ \t]+/ /g;

So this re-roll doesn't have the alias -m anymore, but this makes use of
it. Seems you just forgot to update this from v1, unless I'm missing
something while skimming this...

Re: [PATCH v2 5/6] builtin/grep.c: show column numbers via --column-number

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 22 2018, Taylor Blau wrote:

I think this part though...

> While we're at it, change color.grep.linenumber to color.grep.lineNumber
> to match the casing of nearby variables.
> [...]
> -`linenumber`;;
> +`lineNumber`;;

Makes sense as its own patch at the beginning of the series, since it's
just related cleanup.

> +`columnNumber`;;
> + column number prefix (when using `--column-number`)

Here you're using --column-number...

> +grep.columnNumber::
> + If set to true, enable `-m` option by default.

...But not here. This needs to be updated

> +grep.columnNumber::
> + If set to true, enable `-m` option by default.
> +

...ditto.

> +--column-number::
> + Prefix the 1-indexed column number of the first match on non-context 
> lines.
> +
> [...]
>   OPT_GROUP(""),
>   OPT_BOOL('n', "line-number", , N_("show line 
> numbers")),
> + OPT_BOOL(0, "column-number", , N_("show column 
> numbers")),

Maybe "show first matching column"? I.e. the main docs say "just shows
the first", but this seems to give a different impression.

It would also be nice if the docs briefly explained what this is for,
i.e. the git-jump use-case.

Re: [PATCH v2 3/6] grep.[ch]: teach columnnum, color_columnno to grep_opt

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 22 2018, Taylor Blau wrote:

I think [345]/6 would make much more sense as just one patch. Comments
on them to follow...

> In preparation of adding --column-number to 'git-grep(1)', we extend
> grep_opt to take in the requisite new members.

Just a nit: Makes sense to refer to these camel-cased in docs & commit
messages.

> diff --git a/grep.c b/grep.c
> [...]

All of the boilerplate looks fine.

Some mutt(1) patches and scripts

[Taylor Blau]

Hi,

I have spent more time contributing to the Git list lately, and as such
have grown a number of patches and scripts that have been useful for my
workflow. I am interested in sharing them here in the hopes that they
will be useful for others as well :-).

My workflow is as follows:

  1. Write some commits.

  2. Prepare them with 'git mail' (a wrapper over 'git-format-patch(1)').

  3. Edit the cover letter template, and look them over in Mutt before
 sending them to the list.

I suspect that (2) and (3) are somewhat unconventional. My 'git mail'
script, in particular, has been useful to me. The contents looks
(basically) as follows [1]:

  mbox=$(mktemp)
  git format-patch --stdout $@ >"$mbox" && mutt -f "$mbox"

This has been useful in not having to move around many *.patch files in
an out of a directory. I enjoy looking at a series as a thread in
mutt(1) rather than as individual files in $EDITOR.

Mutt is not particularly keen to resend email so I have had to teach it
a few tricks:

  1. Macros "b" and "B" to resend and force-resend the highlighted
 message. [2]

macro index,pager b ":set edit_headers=yes:set 
edit_headers=no"
macro index,pager B ":set 
editor=true:set editor=$EDITOR"

  2. A patch to not destroy the original Message-ID header when
 resending email. Mutt (sensibly) does this by default, but it is
 not suitable for my workflow, as when I edit the cover letter
 template the Message-ID changes and subsequent patches are sent in
 response to a non-existent message.

 I have patched Mutt to remove this behavior, and (since I work on
 macOS) set up a Homebrew tap to install mutt with
 `--with-retain-messageid'. [3]

Thanks,
Taylor

[1]: https://github.com/ttaylorr/dotfiles/blob/work-gh/bin/git-mail
[2]: https://github.com/ttaylorr/dotfiles/blob/work-gh/mutt/.muttrc#L43-L44
[3]: https://github.com/ttaylorr/homebrew-mutt

[PATCH v2 3/6] grep.[ch]: teach columnnum, color_columnno to grep_opt

[Taylor Blau]

In preparation of adding --column-number to 'git-grep(1)', we extend
grep_opt to take in the requisite new members.

We additionally teach the 'grep.columnnumber' and
'color.grep.columnnumber' configuration variables to configure showing
and coloring the column number, respectively. (These options remain
undocumented until 'git-grep(1)' learns the --column option in a
forthcoming commit.)

Signed-off-by: Taylor Blau 
---
 grep.c | 8 
 grep.h | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/grep.c b/grep.c
index 29bc799ecf..922ab92eff 100644
--- a/grep.c
+++ b/grep.c
@@ -95,6 +95,10 @@ int grep_config(const char *var, const char *value, void *cb)
opt->linenum = git_config_bool(var, value);
return 0;
}
+   if (!strcmp(var, "grep.columnnumber")) {
+   opt->columnnum = git_config_bool(var, value);
+   return 0;
+   }
 
if (!strcmp(var, "grep.fullname")) {
opt->relative = !git_config_bool(var, value);
@@ -111,6 +115,8 @@ int grep_config(const char *var, const char *value, void 
*cb)
color = opt->color_function;
else if (!strcmp(var, "color.grep.linenumber"))
color = opt->color_lineno;
+   else if (!strcmp(var, "color.grep.columnnumber"))
+   color = opt->color_columnno;
else if (!strcmp(var, "color.grep.matchcontext"))
color = opt->color_match_context;
else if (!strcmp(var, "color.grep.matchselected"))
@@ -155,6 +161,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)
opt->extended_regexp_option = def->extended_regexp_option;
opt->pattern_type_option = def->pattern_type_option;
opt->linenum = def->linenum;
+   opt->columnnum = def->columnnum;
opt->max_depth = def->max_depth;
opt->pathname = def->pathname;
opt->relative = def->relative;
@@ -164,6 +171,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)
color_set(opt->color_filename, def->color_filename);
color_set(opt->color_function, def->color_function);
color_set(opt->color_lineno, def->color_lineno);
+   color_set(opt->color_columnno, def->color_columnno);
color_set(opt->color_match_context, def->color_match_context);
color_set(opt->color_match_selected, def->color_match_selected);
color_set(opt->color_selected, def->color_selected);
diff --git a/grep.h b/grep.h
index 399381c908..08a0b391c5 100644
--- a/grep.h
+++ b/grep.h
@@ -127,6 +127,7 @@ struct grep_opt {
int prefix_length;
regex_t regexp;
int linenum;
+   int columnnum;
int invert;
int ignore_case;
int status_only;
@@ -159,6 +160,7 @@ struct grep_opt {
char color_filename[COLOR_MAXLEN];
char color_function[COLOR_MAXLEN];
char color_lineno[COLOR_MAXLEN];
+   char color_columnno[COLOR_MAXLEN];
char color_match_context[COLOR_MAXLEN];
char color_match_selected[COLOR_MAXLEN];
char color_selected[COLOR_MAXLEN];
-- 
2.17.0

[PATCH v2 2/6] grep.c: take column number as argument to show_line()

[Taylor Blau]

show_line() currently receives the line number within the
'grep_opt->buf' in order to determine which line number to display. In
order to display information about the matching column number--if
requested--we must additionally take in that information.

To do so, we extend the signature of show_line() to take in an
additional unsigned "cno". "cno" is either:

  * A 1-indexed column number of the first match on the given line, or
  * 0, if the column number is irrelevant (when displaying a function
name, context lines, etc).

We additionally modify all calls to show_line() in order to pass the new
required argument.

Signed-off-by: Taylor Blau 
---
 grep.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/grep.c b/grep.c
index 1c25782355..29bc799ecf 100644
--- a/grep.c
+++ b/grep.c
@@ -1361,7 +1361,7 @@ static int next_match(struct grep_opt *opt, char *bol, 
char *eol,
 }
 
 static void show_line(struct grep_opt *opt, char *bol, char *eol,
- const char *name, unsigned lno, char sign)
+ const char *name, unsigned lno, unsigned cno, char sign)
 {
int rest = eol - bol;
const char *match_color, *line_color = NULL;
@@ -1501,7 +1501,7 @@ static void show_funcname_line(struct grep_opt *opt, 
struct grep_source *gs,
break;
 
if (match_funcname(opt, gs, bol, eol)) {
-   show_line(opt, bol, eol, gs->name, lno, '=');
+   show_line(opt, bol, eol, gs->name, lno, 0, '=');
break;
}
}
@@ -1566,7 +1566,7 @@ static void show_pre_context(struct grep_opt *opt, struct 
grep_source *gs,
 
while (*eol != '\n')
eol++;
-   show_line(opt, bol, eol, gs->name, cur, sign);
+   show_line(opt, bol, eol, gs->name, cur, 0, sign);
bol = eol + 1;
cur++;
}
@@ -1830,7 +1830,7 @@ static int grep_source_1(struct grep_opt *opt, struct 
grep_source *gs, int colle
show_pre_context(opt, gs, bol, eol, lno);
else if (opt->funcname)
show_funcname_line(opt, gs, bol, lno);
-   show_line(opt, bol, eol, gs->name, lno, ':');
+   show_line(opt, bol, eol, gs->name, lno, match.rm_so+1, 
':');
last_hit = lno;
if (opt->funcbody)
show_function = 1;
@@ -1859,7 +1859,7 @@ static int grep_source_1(struct grep_opt *opt, struct 
grep_source *gs, int colle
/* If the last hit is within the post context,
 * we need to show this line.
 */
-   show_line(opt, bol, eol, gs->name, lno, '-');
+   show_line(opt, bol, eol, gs->name, lno, match.rm_so+1, 
'-');
}
 
next_line:
-- 
2.17.0

[PATCH v2 1/6] grep.c: take regmatch_t as argument in match_line()

[Taylor Blau]

In a subsequent patch, we teach show_line() to optionally include the
column number of the first match on each matched line.

The regmatch_t involved in match_line() and match_one_pattern() both
contain this information (via regmatch_t->rm_so), but their current
implementation throws this stack variable away at the end of the call.

Instead, let's teach match_line() to take in a 'regmatch_t *' so that
callers can inspect the result of their calls. This will prove useful in
a subsequent commit when a caller will forward on information from the
regmatch_t into show_line (as described above).

The return condition remains unchanged, therefore the only change
required of callers is the addition of a single argument.

Signed-off-by: Taylor Blau 
---
 grep.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/grep.c b/grep.c
index 65b90c10a3..1c25782355 100644
--- a/grep.c
+++ b/grep.c
@@ -1299,17 +1299,17 @@ static int match_expr(struct grep_opt *opt, char *bol, 
char *eol,
 }
 
 static int match_line(struct grep_opt *opt, char *bol, char *eol,
- enum grep_context ctx, int collect_hits)
+ regmatch_t *match, enum grep_context ctx,
+ int collect_hits)
 {
struct grep_pat *p;
-   regmatch_t match;
 
if (opt->extended)
return match_expr(opt, bol, eol, ctx, collect_hits);
 
/* we do not call with collect_hits without being extended */
for (p = opt->pattern_list; p; p = p->next) {
-   if (match_one_pattern(p, bol, eol, ctx, , 0))
+   if (match_one_pattern(p, bol, eol, ctx, match, 0))
return 1;
}
return 0;
@@ -1699,6 +1699,7 @@ static int grep_source_1(struct grep_opt *opt, struct 
grep_source *gs, int colle
int try_lookahead = 0;
int show_function = 0;
struct userdiff_driver *textconv = NULL;
+   regmatch_t match;
enum grep_context ctx = GREP_CONTEXT_HEAD;
xdemitconf_t xecfg;
 
@@ -1788,7 +1789,7 @@ static int grep_source_1(struct grep_opt *opt, struct 
grep_source *gs, int colle
if ((ctx == GREP_CONTEXT_HEAD) && (eol == bol))
ctx = GREP_CONTEXT_BODY;
 
-   hit = match_line(opt, bol, eol, ctx, collect_hits);
+   hit = match_line(opt, bol, eol, , ctx, collect_hits);
*eol = ch;
 
if (collect_hits)
-- 
2.17.0

[PATCH v2 5/6] builtin/grep.c: show column numbers via --column-number

[Taylor Blau]

This commit teaches 'git-grep(1)' a new option, '--column-number'. This
option builds upon previous commits to show the column number of the
first match on a non-context line.

For example:

  $ ./git-grep -n --column-number foo | head -n3
  .clang-format:51:14:# myFunction(foo, bar, baz);
  .clang-format:64:7:# int foo();
  .clang-format:75:8:# void foo()

Now that configuration variables such as grep.columnNumber and
color.grep.columnNumber have a visible effect, we document them in this
patch as well.

While we're at it, change color.grep.linenumber to color.grep.lineNumber
to match the casing of nearby variables.

Signed-off-by: Taylor Blau 
---
 Documentation/config.txt   |  7 ++-
 Documentation/git-grep.txt |  8 +++-
 builtin/grep.c |  1 +
 t/t7810-grep.sh| 22 ++
 4 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2659153cb3..1645fcf2ae 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1157,8 +1157,10 @@ color.grep.::
filename prefix (when not using `-h`)
 `function`;;
function name lines (when using `-p`)
-`linenumber`;;
+`lineNumber`;;
line number prefix (when using `-n`)
+`columnNumber`;;
+   column number prefix (when using `--column-number`)
 `match`;;
matching text (same as setting `matchContext` and `matchSelected`)
 `matchContext`;;
@@ -1708,6 +1710,9 @@ gitweb.snapshot::
 grep.lineNumber::
If set to true, enable `-n` option by default.
 
+grep.columnNumber::
+   If set to true, enable `-m` option by default.
+
 grep.patternType::
Set the default matching behavior. Using a value of 'basic', 'extended',
'fixed', or 'perl' will enable the `--basic-regexp`, 
`--extended-regexp`,
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 18b494731f..b75a039768 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -13,7 +13,7 @@ SYNOPSIS
   [-v | --invert-match] [-h|-H] [--full-name]
   [-E | --extended-regexp] [-G | --basic-regexp]
   [-P | --perl-regexp]
-  [-F | --fixed-strings] [-n | --line-number]
+  [-F | --fixed-strings] [-n | --line-number] [--column-number]
   [-l | --files-with-matches] [-L | --files-without-match]
   [(-O | --open-files-in-pager) []]
   [-z | --null]
@@ -44,6 +44,9 @@ CONFIGURATION
 grep.lineNumber::
If set to true, enable `-n` option by default.
 
+grep.columnNumber::
+   If set to true, enable `-m` option by default.
+
 grep.patternType::
Set the default matching behavior. Using a value of 'basic', 'extended',
'fixed', or 'perl' will enable the `--basic-regexp`, 
`--extended-regexp`,
@@ -169,6 +172,9 @@ providing this option will cause it to die.
 --line-number::
Prefix the line number to matching lines.
 
+--column-number::
+   Prefix the 1-indexed column number of the first match on non-context 
lines.
+
 -l::
 --files-with-matches::
 --name-only::
diff --git a/builtin/grep.c b/builtin/grep.c
index 5f32d2ce84..23ce97f998 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -829,6 +829,7 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
GREP_PATTERN_TYPE_PCRE),
OPT_GROUP(""),
OPT_BOOL('n', "line-number", , N_("show line 
numbers")),
+   OPT_BOOL(0, "column-number", , N_("show column 
numbers")),
OPT_NEGBIT('h', NULL, , N_("don't show 
filenames"), 1),
OPT_BIT('H', NULL, , N_("show filenames"), 1),
OPT_NEGBIT(0, "full-name", ,
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 1797f632a3..7349c7fadc 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -99,6 +99,28 @@ do
test_cmp expected actual
'
 
+   test_expect_success "grep -w $L" '
+   {
+   echo ${HC}file:5:foo mmap bar
+   echo ${HC}file:14:foo_mmap bar mmap
+   echo ${HC}file:5:foo mmap bar_mmap
+   echo ${HC}file:14:foo_mmap bar mmap baz
+   } >expected &&
+   git grep --column-number -w -e mmap $H >actual &&
+   test_cmp expected actual
+   '
+
+   test_expect_success "grep -w $L" '
+   {
+   echo ${HC}file:1:5:foo mmap bar
+   echo ${HC}file:3:14:foo_mmap bar mmap
+   echo ${HC}file:4:5:foo mmap bar_mmap
+   echo ${HC}file:5:14:foo_mmap bar mmap baz
+   } >expected &&
+   git grep -n --column-number -w -e mmap $H >actual &&
+   test_cmp expected actual
+   '
+
test_expect_success "grep -w $L" '
{
echo ${HC}file:1:foo mmap bar
-- 
2.17.0

[PATCH v2 4/6] grep.c: display column number of first match

[Taylor Blau]

Building upon our work in the previous commit to add members 'columnnum'
and 'color_columno' to 'grep_opt', we teach show_line() how to respect
those options.

When requested, show_line() will display the column number of the first
match on a non-context line. show_line() differentiates between context
and non-context lines through the '&& cno' check. 'cno' will be equal to
zero if and only if show_line() is invoked on a context line. It will be
a non-zero value otherwise.

Signed-off-by: Taylor Blau 
---
 grep.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/grep.c b/grep.c
index 922ab92eff..23250e60d0 100644
--- a/grep.c
+++ b/grep.c
@@ -1404,6 +1404,12 @@ static void show_line(struct grep_opt *opt, char *bol, 
char *eol,
output_color(opt, buf, strlen(buf), opt->color_lineno);
output_sep(opt, sign);
}
+   if (opt->columnnum && cno) {
+   char buf[32];
+   xsnprintf(buf, sizeof(buf), "%d", cno);
+   output_color(opt, buf, strlen(buf), opt->color_columnno);
+   output_sep(opt, sign);
+   }
if (opt->color) {
regmatch_t match;
enum grep_context ctx = GREP_CONTEXT_BODY;
-- 
2.17.0

[PATCH v2 6/6] contrib/git-jump/git-jump: use column number when grep-ing

[Taylor Blau]

This patch adds the '--column-number' synonym '-m' to the default
grep command so that callers are brought to the correct line _and_
column of each matched location.

Signed-off-by: Taylor Blau 
---
 contrib/git-jump/git-jump | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
index 80ab0590bc..2706963690 100755
--- a/contrib/git-jump/git-jump
+++ b/contrib/git-jump/git-jump
@@ -52,7 +52,7 @@ mode_merge() {
 # editor shows them to us in the status bar.
 mode_grep() {
cmd=$(git config jump.grepCmd)
-   test -n "$cmd" || cmd="git grep -n"
+   test -n "$cmd" || cmd="git grep -n -m"
$cmd "$@" |
perl -pe '
s/[ \t]+/ /g;
-- 
2.17.0

[PATCH v2 0/6] Teach '--column-number' to 'git-grep(1)'

[Taylor Blau]

Hi,

Attached is a re-roll of the series to add --column-number to
'git-grep(1)'.

Since last time, I have changed the following (an inter-diff is
available below for easier consumption):

  * Removed '-m' as an alias for '--column-number', per René's
suggestion [1].

  * Fix some incorrect spelling of 'columnnumber'.

  * Change casing of 'color.grep.linenumber' to 'color.grep.lineNumber'
to be consistent with 'color.grep.columnNumber'. This is an
unrelated change, and one which I am happy to drop from this series.
It was suggested by Martin in [2].

Thanks in advance for your second round of review :-).


Thanks,
Taylor

[1]: https://public-inbox.org/git/cef29224-718f-21e9-0242-8bcd8e9c2...@web.de/
[2]: 
https://public-inbox.org/git/can0hesp_bgqkf26g4tdow6wpsvr2cew6eqf3ajtkcv5pou_...@mail.gmail.com/

Taylor Blau (6):
  grep.c: take regmatch_t as argument in match_line()
  grep.c: take column number as argument to show_line()
  grep.[ch]: teach columnnum, color_columnno to grep_opt
  grep.c: display column number of first match
  builtin/grep.c: show column numbers via --column-number
  contrib/git-jump/git-jump: use column number when grep-ing

 Documentation/config.txt   |  7 ++-
 Documentation/git-grep.txt |  8 +++-
 builtin/grep.c |  1 +
 contrib/git-jump/git-jump  |  2 +-
 grep.c | 33 -
 grep.h |  2 ++
 t/t7810-grep.sh| 22 ++
 7 files changed, 63 insertions(+), 12 deletions(-)

Inter-diff (since v1):

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 02fd4b662b..1645fcf2ae 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1157,10 +1157,10 @@ color.grep.::
filename prefix (when not using `-h`)
 `function`;;
function name lines (when using `-p`)
-`linenumber`;;
+`lineNumber`;;
line number prefix (when using `-n`)
-`columnnumber`;;
-   column number prefix (when using `-m`)
+`columnNumber`;;
+   column number prefix (when using `--column-number`)
 `match`;;
matching text (same as setting `matchContext` and `matchSelected`)
 `matchContext`;;
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index dd90f74ded..b75a039768 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -13,7 +13,7 @@ SYNOPSIS
   [-v | --invert-match] [-h|-H] [--full-name]
   [-E | --extended-regexp] [-G | --basic-regexp]
   [-P | --perl-regexp]
-  [-F | --fixed-strings] [-n | --line-number] [-m | --column-number]
+  [-F | --fixed-strings] [-n | --line-number] [--column-number]
   [-l | --files-with-matches] [-L | --files-without-match]
   [(-O | --open-files-in-pager) []]
   [-z | --null]
@@ -172,7 +172,6 @@ providing this option will cause it to die.
 --line-number::
Prefix the line number to matching lines.

--m::
 --column-number::
Prefix the 1-indexed column number of the first match on non-context 
lines.

diff --git a/builtin/grep.c b/builtin/grep.c
index faa65abab5..23ce97f998 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -829,7 +829,7 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
GREP_PATTERN_TYPE_PCRE),
OPT_GROUP(""),
OPT_BOOL('n', "line-number", , N_("show line 
numbers")),
-   OPT_BOOL('m', "column-number", , N_("show column 
numbers")),
+   OPT_BOOL(0, "column-number", , N_("show column 
numbers")),
OPT_NEGBIT('h', NULL, , N_("don't show 
filenames"), 1),
OPT_BIT('H', NULL, , N_("show filenames"), 1),
OPT_NEGBIT(0, "full-name", ,
diff --git a/grep.c b/grep.c
index 5aeb893263..23250e60d0 100644
--- a/grep.c
+++ b/grep.c
@@ -115,7 +115,7 @@ int grep_config(const char *var, const char *value, void 
*cb)
color = opt->color_function;
else if (!strcmp(var, "color.grep.linenumber"))
color = opt->color_lineno;
-   else if (!strcmp(var, "color.grep.columnumber"))
+   else if (!strcmp(var, "color.grep.columnnumber"))
color = opt->color_columnno;
else if (!strcmp(var, "color.grep.matchcontext"))
color = opt->color_match_context;
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 0cf654824d..7349c7fadc 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -106,7 +106,7 @@ do
echo ${HC}file:5:foo mmap bar_mmap
echo ${HC}file:14:foo_mmap bar mmap baz
} >expected &&
-   git -c grep.linenumber=false grep -m -w -e mmap $H >actual &&
+   git grep --column-number -w -e mmap $H >actual &&
test_cmp expected actual
'

@@ -117,7 +117,7 @@ do
echo ${HC}file:4:5:foo mmap bar_mmap
echo ${HC}file:5:14:foo_mmap 

Re: [PATCH v2 1/2] completion: stop showing 'save' for stash by default

[Thomas Gummerer]

On 04/20, Duy Nguyen wrote:
> On Fri, Apr 20, 2018 at 1:25 AM, Thomas Gummerer  wrote:
> > The 'save' subcommand in git stash has been deprecated in
> > fd2ebf14db ("stash: mark "git stash save" deprecated in the man page",
> > 2017-10-22).
> >
> > Stop showing it when the users enters 'git stash ' or 'git stash
> > s'.  Keep showing it however when the user enters 'git stash sa'
> > or any more characters of the 'save' subcommand.
> 
> I don't think this is worth it. You only save two keystrokes for 've'
> and already waste one on .

I think the main reason for keeping the completion is not actually
saving keystrokes, but rather not giving the users the false
impression that we removed the command, or that something is wrong
with it, without properly warning them before.  That was the main
reason given in [1] why the completion is useful even for short
commands such as 'rm'.

So while I had the same impression before re-reading that thread, I
think keeping the completion for 'git stash sa' is the right
thing to do, and we can remove it some time after we started warning
about 'git stash save' being deprecated.

[1]: 
<01020160a0004473-277c3d7c-4e3b-4c50-9d44-4a106f37f1d9-000...@eu-west-1.amazonses.com>

> -- 
> Duy

Re: What's cooking in git.git (Apr 2018, #02; Tue, 17)

[Taylor Blau]

On Tue, Apr 17, 2018 at 03:07:46PM +0900, Junio C Hamano wrote:
> * tb/config-type (2018-04-10) 2 commits
>  - builtin/config.c: support `--type=` as preferred alias for `--`
>  - builtin/config.c: treat type specifiers singularly
>  (this branch is used by tb/config-default.)
>
>  The "git config" command uses separate options e.g. "--int",
>  "--bool", etc. to specify what type the caller wants the value to
>  be interpreted as.  A new "--type=" option has been
>  introduced, which would make it cleaner to define new types.
>
>  Expecting a final reroll.
>  cf. <20180411034941.GA63158@syl.local>
>  This looked more or less ready, IIRC
>
>
> * tb/config-default (2018-04-10) 3 commits
>  - builtin/config: introduce `color` type specifier
>  - config.c: introduce 'git_config_color' to parse ANSI colors
>  - builtin/config: introduce `--default`
>  (this branch uses tb/config-type.)
>
>  "git config --get" learned the "--default" option, to help the
>  calling script.  Building on top of the tb/config-type topic, the
>  "git config" learns "--type=color" type.  Taken together, you can
>  do things like "git config --get foo.color --default blue" and get
>  the ANSI color sequence for the color given to foo.color variable,
>  or "blue" if the variable does not exist.
>
>  Will wait on the tb/config-type topic.

Hi Junio,

Thank you for picking up both of these topics. I sent a final reroll to
tb/config-type, which I see has made it successfully to 'pu'.

I noticed that tb/config-default, however, only landed two commits:

  - builtin/config: introduce `color` type specifier
  - config.c: introduce 'git_config_color' to parse ANSI colors

but not:

  - builtin/config: introduce `--default`

As such, I am able to build on 'pu', but the `--default` option is
missing:

  ~/g/git (pu) $ ./git-config --type=color --default red core.doesntexist
  error: unknown option `default'

Please advise.


Thanks,
Taylor

Re: Git enhancement request - checkout (clone) set modified dates to commit date

[Kevin Daudt]

On Sun, Apr 22, 2018 at 03:01:10PM -0400, Andrew Wolfe wrote:
> Hi Brian,
> 
> Not completely sure what you're saying.  If the files on master are
> not changed, the changed files' commit timestamps will be older than
> the branch commit timestamps.
> 
> However, if I check out master after committing to a branch, the
> modifications will necessarily disappear because they haven't been
> committed to master.  Instead, under my proposal, each will get the
> timestamp of its prior commit.
> 

Say I build the project while on a certain branch. Then I checkout a
different branch and build again. You would expect that the targets of
every source file that have changed are rebuilt.

When you would restore the timestamp back to the commit timestamp, the
timestamp will be older then the target, and make will not rebuild it,
leaving you with outdated build targets.

Re: Git enhancement request - checkout (clone) set modified dates to commit date

[Andrew Wolfe]

Hi Brian,

Not completely sure what you're saying.  If the files on master are not 
changed, the changed files' commit timestamps will be older than the branch 
commit timestamps.

However, if I check out master after committing to a branch, the modifications 
will necessarily disappear because they haven't been committed to master.  
Instead, under my proposal, each will get the timestamp of its prior commit.

If you're doing a merge, it will entail a commit and, again, the modified files 
will be newer.

I don't think your use case breaks my proposal.

- Andrew Wolfe

> On Apr 22, 2018, at 2:09 PM, brian m. carlson  
> wrote:
> 
> On Sun, Apr 22, 2018 at 01:18:10PM -0400, Andrew Wolfe wrote:
>> I would like to propose that the checkout process set the create and 
>> modification times of a file to the timestamp at which a file was committed.
> 
> The reason Git doesn't do this is pretty simple: make and various other
> tools do rebuilds depending on timestamps.
> 
> If I create a branch off master and make some commits, then switch back
> to master, I will want the changed files to have their timestamps
> updated to be newer so that a make on master will rebuild dependencies
> based on those files.  If I set the files to the commit timestamp, those
> files would be set to the timestamp of master, which is older than my
> new branch, and make wouldn't work properly.
> 
> There are some cases where people want the behavior you requested, such
> as for reproducible builds, and in such cases, you can use a
> post-checkout hook to set timestamps with touch.
> -- 
> brian m. carlson: Houston, Texas, US
> OpenPGP: https://keybase.io/bk2204

Re: RFC: How should we handle un-deleted remote branches?

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 22 2018, Andreas Heiduk wrote:

> Am 22.04.2018 um 13:17 schrieb Ævar Arnfjörð Bjarmason:
>>
>> On Sun, Apr 22 2018, Andreas Heiduk wrote:
>>
>>> Am 20.04.2018 um 14:14 schrieb Ævar Arnfjörð Bjarmason:
 But this is a possible work-around:

 git init /tmp/empty.git
 git remote add avar file:///tmp/empty.git
 git remote prune avar
 git remote remove avar
>>>
>>> This won't do it also?
>>>
>>> git remote prune origin
>>
>> Yes, in this particular case, but that's just emergent behavior in how
>> we handle refspec prunign, and the fact that it "works" is arguably a
>> bug in "prune". i.e. this:
>
> Its not emergent because "origin" is the other remote responsible for
> that ref and cleaning stuff "belonging" to the remote is the job
> description (I'm arguing from a user's perspective, not as a git-developer).

Right, I should have said something closer to "inconsistent", at least
from the user's perspective. I.e. "remove" doesn't delete your branches
because another ref "owns" them, but "prune" will happily clobber
another remote's refs without warning.

Maybe we're happy to keep that, pruning is a bit of an oddity already,
see the "Git has a default disposition of keeping[...]" docs and the
rest of the "PRUNING" section I added to git-fetch in 2.17.0, but this
is osmething worth keeping in mind.

>>
>> (
>> rm -rf /tmp/git &&
>> git clone --bare --mirror g...@github.com:git/git.git /tmp/git &&
>> cd /tmp/git &&
>> git remote add avar g...@github.com:avar/git.git &&
>> git remote add peff g...@github.com:peff/git.git &&
>> git fetch --all &&
>> git remote remove avar &&
>> git remote prune origin
>> )
>>
>> Will delete all the avar/* and peff/* branches, even though I still have
>> a "peff" remote.
>
> Exactly my point: When you are in a the bad situation of "shared
> responsibility", then there is no easy and always correct way out,
> because there are uncountable possible situations.
>
> To give another, slightly modified example expanding the problem space:
>
> (
> rm -rf /tmp/git &&
> git clone --bare --mirror https://github.com/git/git.git /tmp/git &&
> cd /tmp/git &&
> git remote add github https://github.com/avar/git.git &&
> git fetch github &&
> git fetch origin &&
> # note new fetches for "github/master" using with "(forced update)"
> )
>
> For ... reasons the first repo publishes some references like
>
>   github/maint
>   github/master
>   github/pu
>
> So when this repo is mirrored AND another, suitably named remote is
> added then there will be also namespace conflicts. You can call
>
> git fetch github
> git fetch origin
>
> in a loop and most likely each fetch will update the same refs, always
> toggling between two states.
>
> So: not only "remote remove" and "remote prune" are at stake but every
> command handling remote references.

Right, there's certainly a lot of insanity you can create with
overlapping refs, but to bring this thread around a bit, the edge cases
I'm interested in addressing are those where "git remote "
silently doesn't do its job, or does it too eagerly, resulting in a
hard-to-repair repo state.

> How should "git remote remove github" work in both situations? Remove
> the refs/remotes/github/master & co? remove them only if the last fetch
> was for "github" but not when the last update was for "origin"? Should
> "git fetch" use the same logic?

Until we move to some other ref store implementation that namespaces
things properly, the best we can do is simply to assume that
refs/remotes// is owned by the  remote for the purposes of
remove/prune etc.

That'll of course leave open this edge case you're pointing out where
you're mirroring another remote into refs/* and it creates a
remote// branch, but I think it's sufficient for the purposes of
sane UI to just document that fetching into refs/* creates such caveats.

> So it seems better to me to avoid that bad situation altogether. Don't
> allow overlapping/conflicting refspecs when adding a new remote. Using
> *your* last examples both
>
>> git remote add avar g...@github.com:avar/git.git &&
>> git remote add peff g...@github.com:peff/git.git &&
>
> should have failed and hence the "prune" problems won't exist. Same for
> my example.

I think this is a non-started. There's plenty of legitimate reasons to
have overlapping refspecs, e.g. the GitLab case where they're creating a
"geo" remote which is a mirror of other refs they push to.

Even if that wasn't the case, it would be very fragile to solve these
cases by disallowing adding such remotes, since users can edit the
config file, we'd need to detect it on the fly.

>>> Possible 5):
>>>
>>> Don't fix "git remote remove" but "git remote add" to complain that its
>>> ref-namespace is already occupied by some other remote. Add "--force"
>>> for the experts.
>>
>> Indeed, that's 

[PATCH 2/2] walker: drop fields of `struct walker` which are always 1

[Martin Ågren]

After the previous commit, both users of `struct walker` set `get_tree`,
`get_history` and `get_all` to 1. Drop those fields and simplify the
walker implementation accordingly.

Let's hope that any out-of-tree users will not mind this change. They
should notice that the compilation fails as they try to set these
fields. (If they do not set them, note that `get_http_walker()` leaves
them undefined, so the behavior will have been undefined all the time.)

Signed-off-by: Martin Ågren 
---
 walker.h  |  3 ---
 http-fetch.c  |  3 ---
 remote-curl.c |  3 ---
 walker.c  | 19 ---
 4 files changed, 8 insertions(+), 20 deletions(-)

diff --git a/walker.h b/walker.h
index a869013e85..6d8ae00e5b 100644
--- a/walker.h
+++ b/walker.h
@@ -9,9 +9,6 @@ struct walker {
void (*prefetch)(struct walker *, unsigned char *sha1);
int (*fetch)(struct walker *, unsigned char *sha1);
void (*cleanup)(struct walker *);
-   int get_tree;
-   int get_history;
-   int get_all;
int get_verbosely;
int get_recover;
 
diff --git a/http-fetch.c b/http-fetch.c
index a1564f5a41..7b855d3349 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -56,9 +56,6 @@ int cmd_main(int argc, const char **argv)
 
http_init(NULL, url, 0);
walker = get_http_walker(url);
-   walker->get_tree = 1;
-   walker->get_history = 1;
-   walker->get_all = 1;
walker->get_verbosely = get_verbosely;
walker->get_recover = get_recover;
 
diff --git a/remote-curl.c b/remote-curl.c
index a7c4c9b5ff..dd86a6c4fa 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -797,9 +797,6 @@ static int fetch_dumb(int nr_heads, struct ref **to_fetch)
targets[i] = xstrdup(oid_to_hex(_fetch[i]->old_oid));
 
walker = get_http_walker(url.buf);
-   walker->get_all = 1;
-   walker->get_tree = 1;
-   walker->get_history = 1;
walker->get_verbosely = options.verbosity >= 3;
walker->get_recover = 0;
ret = walker_fetch(walker, nr_heads, targets, NULL, NULL);
diff --git a/walker.c b/walker.c
index dffb9c8e37..8eb7db7a52 100644
--- a/walker.c
+++ b/walker.c
@@ -72,6 +72,8 @@ static struct commit_list *complete = NULL;
 
 static int process_commit(struct walker *walker, struct commit *commit)
 {
+   struct commit_list *parents;
+
if (parse_commit(commit))
return -1;
 
@@ -86,19 +88,14 @@ static int process_commit(struct walker *walker, struct 
commit *commit)
 
walker_say(walker, "walk %s\n", oid_to_hex(>object.oid));
 
-   if (walker->get_tree) {
-   if (process(walker, >tree->object))
+   if (process(walker, >tree->object))
+   return -1;
+
+   for (parents = commit->parents; parents; parents = parents->next) {
+   if (process(walker, >item->object))
return -1;
-   if (!walker->get_all)
-   walker->get_tree = 0;
-   }
-   if (walker->get_history) {
-   struct commit_list *parents = commit->parents;
-   for (; parents; parents = parents->next) {
-   if (process(walker, >item->object))
-   return -1;
-   }
}
+
return 0;
 }
 
-- 
2.17.0

[PATCH 1/2] http-fetch: make `-a` standard behaviour

[Martin Ågren]

This is a follow-up to a6c786fce8 (Mark http-fetch without -a as
deprecated, 2011-08-23). For more than six years, we have been warning
when `-a` is not provided, and the documentation has been saying that
`-a` will become the default.

It is a bit unclear what "default" means here. There is no such thing as
`http-fetch --no-a`. But according to my searches, no-one has been
asking on the mailing list how they should silence the warning and
prepare for overriding the flipped default. So let's assume that
everybody is happy with `-a`. They should be, since not using it may
break the repo in such a way that Git itself is unable to fix it.

Always behave as if `-a` was given. Since `-a` implies `-c` (get commit
objects) and `-t` (get trees), all three options are now unnecessary.
Document all of these as historical artefacts that have no effect.

Leave no-op code for handling these options in http-fetch.c. The
options-handling is currently rather loose. If someone tightens it, we
will not want these ignored options to accidentally turn into hard
errors.

Since `-a` was the only safe and sane usage and we have been pushing
people towards it for a long time, refrain from warning when it is used
"unnecessarily" now. Similarly, do not add anything scary-looking to the
man-page about how it will be removed in the future. We can always do so
later. (It is not like we are in desperate need of freeing up
one-letter arguments.)

Signed-off-by: Martin Ågren 
---
 Documentation/git-http-fetch.txt | 13 +
 t/t5550-http-fetch-dumb.sh   | 11 +++
 http-fetch.c | 18 +++---
 3 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/Documentation/git-http-fetch.txt b/Documentation/git-http-fetch.txt
index 21a33d2c41..666b042679 100644
--- a/Documentation/git-http-fetch.txt
+++ b/Documentation/git-http-fetch.txt
@@ -15,8 +15,9 @@ DESCRIPTION
 ---
 Downloads a remote Git repository via HTTP.
 
-*NOTE*: use of this command without -a is deprecated.  The -a
-behaviour will become the default in a future release.
+This command always gets all objects. Historically, there were three options
+`-a`, `-c` and `-t` for choosing which objects to download. They are now
+silently ignored.
 
 OPTIONS
 ---
@@ -24,12 +25,8 @@ commit-id::
 Either the hash or the filename under [URL]/refs/ to
 pull.
 
--c::
-   Get the commit objects.
--t::
-   Get trees associated with the commit objects.
--a::
-   Get all the objects.
+-a, -c, -t::
+   These options are ignored for historical reasons.
 -v::
Report what is downloaded.
 
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index 8552184e74..6d7d88ccc9 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -169,6 +169,17 @@ test_expect_success 'fetch changes via manual http-fetch' '
test_cmp file clone2/file
 '
 
+test_expect_success 'manual http-fetch without -a works just as well' '
+   cp -R clone-tmpl clone3 &&
+
+   HEAD=$(git rev-parse --verify HEAD) &&
+   (cd clone3 &&
+git http-fetch -w heads/master-new $HEAD $(git config 
remote.origin.url) &&
+git checkout master-new &&
+test $HEAD = $(git rev-parse --verify HEAD)) &&
+   test_cmp file clone3/file
+'
+
 test_expect_success 'http remote detects correct HEAD' '
git push public master:other &&
(cd clone &&
diff --git a/http-fetch.c b/http-fetch.c
index 8af380050c..a1564f5a41 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -17,21 +17,13 @@ int cmd_main(int argc, const char **argv)
char *url = NULL;
int arg = 1;
int rc = 0;
-   int get_tree = 0;
-   int get_history = 0;
-   int get_all = 0;
int get_verbosely = 0;
int get_recover = 0;
 
while (arg < argc && argv[arg][0] == '-') {
if (argv[arg][1] == 't') {
-   get_tree = 1;
} else if (argv[arg][1] == 'c') {
-   get_history = 1;
} else if (argv[arg][1] == 'a') {
-   get_all = 1;
-   get_tree = 1;
-   get_history = 1;
} else if (argv[arg][1] == 'v') {
get_verbosely = 1;
} else if (argv[arg][1] == 'w') {
@@ -55,10 +47,6 @@ int cmd_main(int argc, const char **argv)
commits = 1;
}
 
-   if (get_all == 0)
-   warning("http-fetch: use without -a is deprecated.\n"
-   "In a future release, -a will become the default.");
-
if (argv[arg])
str_end_url_with_slash(argv[arg], );
 
@@ -68,9 +56,9 @@ int cmd_main(int argc, const char **argv)
 
http_init(NULL, url, 0);
walker = get_http_walker(url);
-   walker->get_tree = get_tree;
-   walker->get_history = get_history;
-   walker->get_all = get_all;
+   

Re: Git enhancement request - checkout (clone) set modified dates to commit date

[brian m. carlson]

On Sun, Apr 22, 2018 at 01:18:10PM -0400, Andrew Wolfe wrote:
> I would like to propose that the checkout process set the create and 
> modification times of a file to the timestamp at which a file was committed.

The reason Git doesn't do this is pretty simple: make and various other
tools do rebuilds depending on timestamps.

If I create a branch off master and make some commits, then switch back
to master, I will want the changed files to have their timestamps
updated to be newer so that a make on master will rebuild dependencies
based on those files.  If I set the files to the commit timestamp, those
files would be set to the timestamp of master, which is older than my
new branch, and make wouldn't work properly.

There are some cases where people want the behavior you requested, such
as for reproducible builds, and in such cases, you can use a
post-checkout hook to set timestamps with touch.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: Is support for 10.8 dropped?

[brian m. carlson]

On Sun, Apr 22, 2018 at 12:10:20AM -0700, Perry Hutchison wrote:
> Eric Sunshine  wrote:
> > On Sun, Apr 22, 2018 at 1:15 AM, Igor Korot  wrote:
> > > MyMac:git-2.17.0 igorkorot$ cat config.mak
> > > NO_GETTEXT=Yes
> > > NO_OPENSSL=Yes
> > >
> > > MyMac:dbhandler igorkorot$ /Users/igorkorot/git-2.17.0/git pull
> > > fatal: unable to access
> > > 'https://github.com/oneeyeman1/dbhandler.git/': error:1407742E:SSL
> > > routines:SSL23_GET_SERVER_HELLO:tlsv1 alert protocol version
> > > MyMac:dbhandler igorkorot$
> >
> > Try re-building with OpenSSL enabled (remove NO_OPENSSL from
> > config.make). You may need to build/install OpenSSL yourself to get
> > this to work.
> 
> Explanation:  "tlsv1 alert protocol version" means that the server
> requires its clients to use a newer version of TLS than what was
> used in the local build of git.

I think the issue here is that you're using a crypto library which may
only support TLS 1.0 on 10.8[0].  GitHub requires TLS 1.2 as of
recently.  So this isn't really a problem with Git, so much as it's an
incompatibility between the version of the crypto library you're using
and GitHub.

I expect that due to the PCI DSS rules prohibiting new deployment of TLS
1.0, you'll continue to run into this issue more and more unless you
upgrade to an OS or crypto library that supports TLS 1.2.  As of June
30, TLS 1.0 will be pretty much dead on the Internet.

[0] I surmised this from https://www.ssllabs.com/ssltest/clients.html,
but I don't use macOS so can't speak for certain.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Git enhancement request - checkout (clone) set modified dates to commit date

[Andrew Wolfe]

Hello,

there are several timestamps in the lifecycle of a modification to a file in 
Git:

• file write timestamp
• git add timestamp
• git commit timestamp
• git push timestamp
• git merge timestamp
• git checkout timestamp

Right now when I check out/clone a repository, all the files have the checkout 
timestamp as

• ctime - creation time for the file
• mtime - modification time for the file
• atime - las access time for the file

Not only does this force more work for timestamp-based build programs, it also 
deprives me, as a developer, of a visual 'file blame' that could be very useful 
in spotting changes without having to do git log over and over.

I would like to propose that the checkout process set the create and 
modification times of a file to the timestamp at which a file was committed.

When repository servers have different clocks - which is normal - each 
clone/merge/push should record the time offset.  Each timestamp on each commit 
should be corrected to the repository's specific time, and that should be a 
marking on the history.

Sincere regards,

Andrew Wolfe

Re: [PATCH v8 06/16] sequencer: introduce the `merge` command

[Phillip Wood]

On 21/04/18 16:56, Phillip Wood wrote:
> On 21/04/18 11:33, Johannes Schindelin wrote:
>> This patch is part of the effort to reimplement `--preserve-merges` with
>> a substantially improved design, a design that has been developed in the
>> Git for Windows project to maintain the dozens of Windows-specific patch
>> series on top of upstream Git.
>>
>> The previous patch implemented the `label` and `reset` commands to label
>> commits and to reset to labeled commits. This patch adds the `merge`
>> command, with the following syntax:
> 
> The two patches seem to have been fused together in this series.
> 
> If the reset command fails because it would overwrite untracked files it
> says
> 
> error: Untracked working tree file 'b' would be overwritten by merge.
> 
> Followed by the hint to edit the todo file. Saying 'merge' rather
> 'reset' is possibly confusing to users. Perhaps it could call
> setup_unpack_trees_porcelain(), though that would need to be extended to
> handle 'reset'.


> Also it currently refuses to overwrite ignored files
> which is either annoying or safe depending on one's point of view.

Looking at the existing code this is consistent with (most) of the rest
of the sequencer. The code to fast-forward commits will overwrite
ignored files, and I think the initial checkout will as well but the
rest (picking commits and the new merge command) will not.

> Best Wishes
> 
> Phillip
> 
>>
>> merge [-C ]  # 
>>
>> The  parameter in this instance is the *original* merge commit,
>> whose author and message will be used for the merge commit that is about
>> to be created.
>>
>> The  parameter refers to the (possibly rewritten) revision to
>> merge. Let's see an example of a todo list:
>>
>> label onto
>>
>> # Branch abc
>> reset onto
>> pick deadbeef Hello, world!
>> label abc
>>
>> reset onto
>> pick cafecafe And now for something completely different
>> merge -C baaabaaa abc # Merge the branch 'abc' into master
>>
>> To edit the merge commit's message (a "reword" for merges, if you will),
>> use `-c` (lower-case) instead of `-C`; this convention was borrowed from
>> `git commit` that also supports `-c` and `-C` with similar meanings.
>>
>> To create *new* merges, i.e. without copying the commit message from an
>> existing commit, simply omit the `-C ` parameter (which will
>> open an editor for the merge message):
>>
>> merge abc
>>
>> This comes in handy when splitting a branch into two or more branches.
>>
>> Note: this patch only adds support for recursive merges, to keep things
>> simple. Support for octopus merges will be added later in a separate
>> patch series, support for merges using strategies other than the
>> recursive merge is left for the future.
>>
>> Signed-off-by: Johannes Schindelin 
>> ---
>>   git-rebase--interactive.sh |   6 +
>>   sequencer.c    | 407 -
>>   2 files changed, 406 insertions(+), 7 deletions(-)
>>
>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> index e1b865f43f2..ccd5254d1c9 100644
>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -162,6 +162,12 @@ s, squash  = use commit, but meld into
>> previous commit
>>   f, fixup  = like \"squash\", but discard this commit's log
>> message
>>   x, exec  = run command (the rest of the line) using shell
>>   d, drop  = remove commit
>> +l, label  = label current HEAD with a name
>> +t, reset  = reset HEAD to a label
>> +m, merge [-C  | -c ]  [# ]
>> +.   create a merge commit using the original merge commit's
>> +.   message (or the oneline, if no original merge commit was
>> +.   specified). Use -c  to reword the commit message.
>>     These lines can be re-ordered; they are executed from top to bottom.
>>   " | git stripspace --comment-lines >>"$todo"
>> diff --git a/sequencer.c b/sequencer.c
>> index 01443e0f245..35fcacbdf0f 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -23,6 +23,8 @@
>>   #include "hashmap.h"
>>   #include "notes-utils.h"
>>   #include "sigchain.h"
>> +#include "unpack-trees.h"
>> +#include "worktree.h"
>>     #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>>   @@ -120,6 +122,13 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha,
>> "rebase-merge/stopped-sha")
>>   static GIT_PATH_FUNC(rebase_path_rewritten_list,
>> "rebase-merge/rewritten-list")
>>   static GIT_PATH_FUNC(rebase_path_rewritten_pending,
>>   "rebase-merge/rewritten-pending")
>> +
>> +/*
>> + * The path of the file listing refs that need to be deleted after
>> the rebase
>> + * finishes. This is used by the `label` command to record the need
>> for cleanup.
>> + */
>> +static GIT_PATH_FUNC(rebase_path_refs_to_delete,
>> "rebase-merge/refs-to-delete")
>> +
>>   /*
>>    * The following files are written by git-rebase just after parsing the
>>    * command-line (and are only consumed, not modified, by the
>> sequencer).
>> @@ -244,18 +253,34 @@ 

Re: [PATCH v3 0/6] Keep all info in command-list.txt in git binary

[Ramsay Jones]

On 22/04/18 17:12, Duy Nguyen wrote:
> On Sun, Apr 22, 2018 at 5:58 PM, Ramsay Jones
>  wrote:
 I think you need to try a little harder than this! ;-)
>>>
>>> Yeah. I did think about grepping the output but decided not to because
>>> of gettext poison stuff and column output in "git help". If we do want
>>> to test this, how about I extend --list-cmds= option to take a few
>>> more parameters? --list-cmds=common would output all common commands,
>>> --list-cmds= does the same for other command category. This
>>> way we can verify without worrying about text formatting, paging or
>>> translation.
>>
>> Hmm, my immediate reaction would be to prefer my simple tests.
>> Yes, they are not exactly rigorous and they would be affected
>> by changing the help formatting, but they are effective. ;-)
>>
>> [I don't think the formatting would change that often, or at
>> all - whoever submits that patch would get to update the tests!]
> 
> Hmm.. for non-column output that's true. "git help" with column output
> should probably fine as well because even though we add more and more
> commands every month, these are not marked common (and unlikely so).
> So yeah I agree.
> 
>> What did you think about adding the BUG() checks?
> 
> I was thinking if there was a way to fail the build after running
> ./generate-cmds.sh and generating empty output but it does not sound
> easy to do. So yeah, BUG() checks sound good.

Yeah, failing the build would be preferable, but I didn't get
that far. ;-)

[BTW, I just applied your patch series to the 'next' branch
(I just couldn't be bothered to revert your last series from
the 'pu' branch, etc.) and, as expected, everything built OK,
passed the test-suite and 'git help' is working just fine.]

Thanks!

ATB,
Ramsay Jones

Re: [PATCH v3 0/6] Keep all info in command-list.txt in git binary

[Duy Nguyen]

On Sun, Apr 22, 2018 at 5:58 PM, Ramsay Jones
 wrote:
>>> I think you need to try a little harder than this! ;-)
>>
>> Yeah. I did think about grepping the output but decided not to because
>> of gettext poison stuff and column output in "git help". If we do want
>> to test this, how about I extend --list-cmds= option to take a few
>> more parameters? --list-cmds=common would output all common commands,
>> --list-cmds= does the same for other command category. This
>> way we can verify without worrying about text formatting, paging or
>> translation.
>
> Hmm, my immediate reaction would be to prefer my simple tests.
> Yes, they are not exactly rigorous and they would be affected
> by changing the help formatting, but they are effective. ;-)
>
> [I don't think the formatting would change that often, or at
> all - whoever submits that patch would get to update the tests!]

Hmm.. for non-column output that's true. "git help" with column output
should probably fine as well because even though we add more and more
commands every month, these are not marked common (and unlikely so).
So yeah I agree.

> What did you think about adding the BUG() checks?

I was thinking if there was a way to fail the build after running
./generate-cmds.sh and generating empty output but it does not sound
easy to do. So yeah, BUG() checks sound good.

-- 
Duy

Re: [PATCH v8 06/16] sequencer: introduce the `merge` command

[Philip Oakley]

From: "Johannes Schindelin" 

This patch is part of the effort to reimplement `--preserve-merges` with
a substantially improved design, a design that has been developed in the
Git for Windows project to maintain the dozens of Windows-specific patch
series on top of upstream Git.

The previous patch implemented the `label` and `reset` commands to label


The previous patch was [Patch 05/16] git-rebase--interactive: clarify 
arguments, so this statement doesn't appear to be true. Has a patch been 
missed or re-ordered? Or should it be simply "This patch implements" ? 
Likewise the patch subject would be updated.



commits and to reset to labeled commits. This patch adds the `merge`


s/adds/also adds/ ?


command, with the following syntax:

merge [-C ]  # 

The  parameter in this instance is the *original* merge commit,
whose author and message will be used for the merge commit that is about
to be created.

The  parameter refers to the (possibly rewritten) revision to
merge. Let's see an example of a todo list:


The example ought to also note that `label onto` is to
`# label current HEAD with a name`, seeing as this is the first occurance.
It may be obvious in retrospect, but not at first reading.


label onto

# Branch abc
reset onto


Is this reset strictly necessary. We are already there @head.


pick deadbeef Hello, world!
label abc

reset onto
pick cafecafe And now for something completely different
merge -C baaabaaa abc # Merge the branch 'abc' into master

To edit the merge commit's message (a "reword" for merges, if you will),
use `-c` (lower-case) instead of `-C`; this convention was borrowed from
`git commit` that also supports `-c` and `-C` with similar meanings.

To create *new* merges, i.e. without copying the commit message from an
existing commit, simply omit the `-C ` parameter (which will
open an editor for the merge message):

merge abc

This comes in handy when splitting a branch into two or more branches.

Note: this patch only adds support for recursive merges, to keep things
simple. Support for octopus merges will be added later in a separate
patch series, support for merges using strategies other than the
recursive merge is left for the future.

Signed-off-by: Johannes Schindelin 
---
git-rebase--interactive.sh |   6 +
sequencer.c| 407 -
2 files changed, 406 insertions(+), 7 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index e1b865f43f2..ccd5254d1c9 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -162,6 +162,12 @@ s, squash  = use commit, but meld into 
previous commit

f, fixup  = like \"squash\", but discard this commit's log message
x, exec  = run command (the rest of the line) using shell
d, drop  = remove commit
+l, label  = label current HEAD with a name
+t, reset  = reset HEAD to a label
+m, merge [-C  | -c ]  [# ]
+.   create a merge commit using the original merge commit's
+.   message (or the oneline, if no original merge commit was
+.   specified). Use -c  to reword the commit message.

These lines can be re-ordered; they are executed from top to bottom.
" | git stripspace --comment-lines >>"$todo"
diff --git a/sequencer.c b/sequencer.c
index 01443e0f245..35fcacbdf0f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -23,6 +23,8 @@
#include "hashmap.h"
#include "notes-utils.h"
#include "sigchain.h"
+#include "unpack-trees.h"
+#include "worktree.h"

#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"

@@ -120,6 +122,13 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha, 
"rebase-merge/stopped-sha")
static GIT_PATH_FUNC(rebase_path_rewritten_list, 
"rebase-merge/rewritten-list")

static GIT_PATH_FUNC(rebase_path_rewritten_pending,
 "rebase-merge/rewritten-pending")
+
+/*
+ * The path of the file listing refs that need to be deleted after the 
rebase
+ * finishes. This is used by the `label` command to record the need for 
cleanup.

+ */
+static GIT_PATH_FUNC(rebase_path_refs_to_delete, 
"rebase-merge/refs-to-delete")

+
/*
 * The following files are written by git-rebase just after parsing the
 * command-line (and are only consumed, not modified, by the sequencer).
@@ -244,18 +253,34 @@ static const char *gpg_sign_opt_quoted(struct 
replay_opts *opts)


int sequencer_remove_state(struct replay_opts *opts)
{
- struct strbuf dir = STRBUF_INIT;
+ struct strbuf buf = STRBUF_INIT;
 int i;

+ if (is_rebase_i(opts) &&
+ strbuf_read_file(, rebase_path_refs_to_delete(), 0) > 0) {
+ char *p = buf.buf;
+ while (*p) {
+ char *eol = strchr(p, '\n');
+ if (eol)
+ *eol = '\0';
+ if (delete_ref("(rebase -i) cleanup", p, NULL, 0) < 0)
+ warning(_("could not delete '%s'"), p);
+ if (!eol)
+ break;
+ p = eol + 1;
+ }
+ }
+
 free(opts->gpg_sign);
 free(opts->strategy);
 for (i = 0; i < opts->xopts_nr; i++)
 free(opts->xopts[i]);
 free(opts->xopts);

- strbuf_addstr(, get_dir(opts));
- remove_dir_recursively(, 0);
- 

Re: [PATCH v3 0/6] Keep all info in command-list.txt in git binary

[Ramsay Jones]

On 22/04/18 16:22, Duy Nguyen wrote:
> On Sun, Apr 22, 2018 at 4:45 PM, Ramsay Jones
>  wrote:
>>
>>
>> On 21/04/18 17:56, Duy Nguyen wrote:
>>> On Sat, Apr 21, 2018 at 06:54:08PM +0200, Nguyễn Thái Ngọc Duy wrote:
 Changes:

 - remove the deprecated column in command-list.txt. My change break it
   anyway if anyone uses it.
 - fix up failed tests that I marked in the RFC and kinda forgot about it.
 - fix bashisms in generate-cmdlist.sh
 - fix segfaul in "git help"
>>>
>>> Sorry I forgot the interdiff
>>>
>> [snip]
>>
>>> diff --git a/t/t0012-help.sh b/t/t0012-help.sh
>>> index fd2a7f27dc..53208ab20e 100755
>>> --- a/t/t0012-help.sh
>>> +++ b/t/t0012-help.sh
>>> @@ -25,6 +25,15 @@ test_expect_success "setup" '
>>>   EOF
>>>  '
>>>
>>> +# make sure to exercise these code paths, the output is a bit tricky
>>> +# to verify
>>> +test_expect_success 'basic help commands' '
>>> + git help >/dev/null &&
>>> + git help -a >/dev/null &&
>>> + git help -g >/dev/null &&
>>> + git help -av >/dev/null
>>> +'
>>> +
>> I think you need to try a little harder than this! ;-)
> 
> Yeah. I did think about grepping the output but decided not to because
> of gettext poison stuff and column output in "git help". If we do want
> to test this, how about I extend --list-cmds= option to take a few
> more parameters? --list-cmds=common would output all common commands,
> --list-cmds= does the same for other command category. This
> way we can verify without worrying about text formatting, paging or
> translation.

Hmm, my immediate reaction would be to prefer my simple tests.
Yes, they are not exactly rigorous and they would be affected 
by changing the help formatting, but they are effective. ;-)

[I don't think the formatting would change that often, or at
all - whoever submits that patch would get to update the tests!]

What did you think about adding the BUG() checks?

ATB,
Ramsay Jones

Re: [PATCH 3/3] Avoid multiple PREFIX definitions

[Philip Oakley]

From: "Johannes Schindelin" 

From: Philip Oakley 

The short and sweet PREFIX can be confused when used in many places.

Rename both usages to better describe their purpose. EXEC_CMD_PREFIX is
used in full to disambiguate it from the nearby GIT_EXEC_PATH.


@dcsho; Thanks for keeping up with this and all your work. LGTM Philip.



The PREFIX in sideband.c, while nominally independant of the exec_cmd
PREFIX, does reside within libgit[1], so the definitions would clash
when taken together with a PREFIX given on the command line for use by
exec_cmd.c.

Noticed when compiling Git for Windows using MSVC/Visual Studio [1] which
reports the conflict beteeen the command line definition and the
definition in sideband.c within the libgit project.

[1] the libgit functions are brought into a single sub-project
within the Visual Studio construction script provided in contrib,
and hence uses a single command for both exec_cmd.c and sideband.c.

Signed-off-by: Philip Oakley 
Signed-off-by: Johannes Schindelin 
---
Makefile   |  2 +-
exec-cmd.c |  4 ++--
sideband.c | 10 +-
3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/Makefile b/Makefile
index 111e93d3bea..49cec672242 100644
--- a/Makefile
+++ b/Makefile
@@ -2271,7 +2271,7 @@ exec-cmd.sp exec-cmd.s exec-cmd.o: EXTRA_CPPFLAGS =
\
 '-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \
 '-DGIT_LOCALE_PATH="$(localedir_relative_SQ)"' \
 '-DBINDIR="$(bindir_relative_SQ)"' \
- '-DPREFIX="$(prefix_SQ)"'
+ '-DFALLBACK_RUNTIME_PREFIX="$(prefix_SQ)"'

builtin/init-db.sp builtin/init-db.s builtin/init-db.o: GIT-PREFIX
builtin/init-db.sp builtin/init-db.s builtin/init-db.o: EXTRA_CPPFLAGS = \
diff --git a/exec-cmd.c b/exec-cmd.c
index 3b0a039083a..02d31ee8971 100644
--- a/exec-cmd.c
+++ b/exec-cmd.c
@@ -48,7 +48,7 @@ static const char *system_prefix(void)
 !(prefix = strip_path_suffix(executable_dirname, GIT_EXEC_PATH)) &&
 !(prefix = strip_path_suffix(executable_dirname, BINDIR)) &&
 !(prefix = strip_path_suffix(executable_dirname, "git"))) {
- prefix = PREFIX;
+ prefix = FALLBACK_RUNTIME_PREFIX;
 trace_printf("RUNTIME_PREFIX requested, "
 "but prefix computation failed.  "
 "Using static fallback '%s'.\n", prefix);
@@ -243,7 +243,7 @@ void git_resolve_executable_dir(const char *argv0)
 */
static const char *system_prefix(void)
{
- return PREFIX;
+ return FALLBACK_RUNTIME_PREFIX;
}

/*
diff --git a/sideband.c b/sideband.c
index 6d7f943e438..325bf0e974a 100644
--- a/sideband.c
+++ b/sideband.c
@@ -13,7 +13,7 @@
 * the remote died unexpectedly.  A flush() concludes the stream.
 */

-#define PREFIX "remote: "
+#define DISPLAY_PREFIX "remote: "

#define ANSI_SUFFIX "\033[K"
#define DUMB_SUFFIX ""
@@ -49,7 +49,7 @@ int recv_sideband(const char *me, int in_stream, int
out)
 switch (band) {
 case 3:
 strbuf_addf(, "%s%s%s", outbuf.len ? "\n" : "",
- PREFIX, buf + 1);
+ DISPLAY_PREFIX, buf + 1);
 retval = SIDEBAND_REMOTE_ERROR;
 break;
 case 2:
@@ -67,7 +67,7 @@ int recv_sideband(const char *me, int in_stream, int
out)
 int linelen = brk - b;

 if (!outbuf.len)
- strbuf_addstr(, PREFIX);
+ strbuf_addstr(, DISPLAY_PREFIX);
 if (linelen > 0) {
 strbuf_addf(, "%.*s%s%c",
 linelen, b, suffix, *brk);
@@ -81,8 +81,8 @@ int recv_sideband(const char *me, int in_stream, int
out)
 }

 if (*b)
- strbuf_addf(, "%s%s",
- outbuf.len ? "" : PREFIX, b);
+ strbuf_addf(, "%s%s", outbuf.len ?
+ "" : DISPLAY_PREFIX, b);
 break;
 case 1:
 write_or_die(out, buf + 1, len);
--
2.17.0.windows.1.15.gaa56ade3205

Re: [PATCH v3 0/6] Keep all info in command-list.txt in git binary

[Duy Nguyen]

On Sun, Apr 22, 2018 at 4:45 PM, Ramsay Jones
 wrote:
>
>
> On 21/04/18 17:56, Duy Nguyen wrote:
>> On Sat, Apr 21, 2018 at 06:54:08PM +0200, Nguyễn Thái Ngọc Duy wrote:
>>> Changes:
>>>
>>> - remove the deprecated column in command-list.txt. My change break it
>>>   anyway if anyone uses it.
>>> - fix up failed tests that I marked in the RFC and kinda forgot about it.
>>> - fix bashisms in generate-cmdlist.sh
>>> - fix segfaul in "git help"
>>
>> Sorry I forgot the interdiff
>>
> [snip]
>
>> diff --git a/t/t0012-help.sh b/t/t0012-help.sh
>> index fd2a7f27dc..53208ab20e 100755
>> --- a/t/t0012-help.sh
>> +++ b/t/t0012-help.sh
>> @@ -25,6 +25,15 @@ test_expect_success "setup" '
>>   EOF
>>  '
>>
>> +# make sure to exercise these code paths, the output is a bit tricky
>> +# to verify
>> +test_expect_success 'basic help commands' '
>> + git help >/dev/null &&
>> + git help -a >/dev/null &&
>> + git help -g >/dev/null &&
>> + git help -av >/dev/null
>> +'
>> +
> I think you need to try a little harder than this! ;-)

Yeah. I did think about grepping the output but decided not to because
of gettext poison stuff and column output in "git help". If we do want
to test this, how about I extend --list-cmds= option to take a few
more parameters? --list-cmds=common would output all common commands,
--list-cmds= does the same for other command category. This
way we can verify without worrying about text formatting, paging or
translation.
-- 
Duy

Re: [PATCH v3 09/11] technical/shallow: describe the relationship with replace refs

[Philip Oakley]

From: "Johannes Schindelin" 

Now that grafts are deprecated, we should start to assume that readers
have no idea what grafts are. So it makes more sense to describe the
"shallow" feature in terms of replace refs.

Suggested-by: Eric Sunshine 
Signed-off-by: Johannes Schindelin 
---
Documentation/technical/shallow.txt | 19 +++
1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/Documentation/technical/shallow.txt 
b/Documentation/technical/shallow.txt

index 5183b154229..b3ff23c25f6 100644
--- a/Documentation/technical/shallow.txt
+++ b/Documentation/technical/shallow.txt
@@ -9,14 +9,17 @@ these commits have no parents.
*

The basic idea is to write the SHA-1s of shallow commits into
-$GIT_DIR/shallow, and handle its contents like the contents
-of $GIT_DIR/info/grafts (with the difference that shallow
-cannot contain parent information).
-
-This information is stored in a new file instead of grafts, or
-even the config, since the user should not touch that file
-at all (even throughout development of the shallow clone, it
-was never manually edited!).
+$GIT_DIR/shallow, and handle its contents similar to replace
+refs (with the difference that shallow does not actually
+create those replace refs) and


If grafts are deprecated, why not alse get rid of this mention and simply 
leave the 'what it does' part.


  very much like the 
deprecated

+graft file (with



  the difference that shallow commits will
+always have their parents grafted away, not replaced by

s/their parents grafted away/no parents/ (rather than being replaced..)


+different parents).
+
+This information is stored in a special-purpose file because the
+user should not touch that file at all (even throughout
+development of the shallow clone, it was never manually
+edited!).

Each line contains exactly one SHA-1. When read, a commit_graft
will be constructed, which has nr_parent < 0 to make it easier
--
2.17.0.windows.1.15.gaa56ade3205

Re: [PATCH v8 09/16] rebase: introduce the --rebase-merges option

[Philip Oakley]

From: "Johannes Schindelin" 

Once upon a time, this here developer thought: wouldn't it be nice if,
say, Git for Windows' patches on top of core Git could be represented as
a thicket of branches, and be rebased on top of core Git in order to
maintain a cherry-pick'able set of patch series?

The original attempt to answer this was: git rebase --preserve-merges.

However, that experiment was never intended as an interactive option,
and it only piggy-backed on git rebase --interactive because that
command's implementation looked already very, very familiar: it was
designed by the same person who designed --preserve-merges: yours truly.

Some time later, some other developer (I am looking at you, Andreas!
;-)) decided that it would be a good idea to allow --preserve-merges to
be combined with --interactive (with caveats!) and the Git maintainer
(well, the interim Git maintainer during Junio's absence, that is)
agreed, and that is when the glamor of the --preserve-merges design
started to fall apart rather quickly and unglamorously.

The reason? In --preserve-merges mode, the parents of a merge commit (or
for that matter, of *any* commit) were not stated explicitly, but were
*implied* by the commit name passed to the `pick` command.

This made it impossible, for example, to reorder commits. Not to mention
to flatten the branch topology or, deity forbid, to split topic branches


Aside: The idea of a "flattened" topology is, to my mind, not actually
defined though may be understood by devs working in the area. Hopefully it's
going away as a term, though the new 'cousins' will need clarification
(there's no dot notation for that area of topology).


into two.

Alas, these shortcomings also prevented that mode (whose original
purpose was to serve Git for Windows' needs, with the additional hope
that it may be useful to others, too) from serving Git for Windows'
needs.

Five years later, when it became really untenable to have one unwieldy,
big hodge-podge patch series of partly related, partly unrelated patches
in Git for Windows that was rebased onto core Git's tags from time to
time (earning the undeserved wrath of the developer of the ill-fated
git-remote-hg series that first obsoleted Git for Windows' competing
approach, only to be abandoned without maintainer later) was really
untenable, the "Git garden shears" were born [*1*/*2*]: a script,
piggy-backing on top of the interactive rebase, that would first
determine the branch topology of the patches to be rebased, create a
pseudo todo list for further editing, transform the result into a real
todo list (making heavy use of the `exec` command to "implement" the
missing todo list commands) and finally recreate the patch series on
top of the new base commit.

That was in 2013. And it took about three weeks to come up with the
design and implement it as an out-of-tree script. Needless to say, the
implementation needed quite a few years to stabilize, all the while the
design itself proved itself sound.

With this patch, the goodness of the Git garden shears comes to `git
rebase -i` itself. Passing the `--rebase-merges` option will generate
a todo list that can be understood readily, and where it is obvious
how to reorder commits. New branches can be introduced by inserting
`label` commands and calling `merge `. And once this mode will
have become stable and universally accepted, we can deprecate the design
mistake that was `--preserve-merges`.

Link *1*:
https://github.com/msysgit/msysgit/blob/master/share/msysGit/shears.sh
Link *2*:
https://github.com/git-for-windows/build-extra/blob/master/shears.sh

Signed-off-by: Johannes Schindelin 
---
Documentation/git-rebase.txt   |  20 ++-
contrib/completion/git-completion.bash |   2 +-
git-rebase--interactive.sh |   1 +
git-rebase.sh  |   6 +
t/t3430-rebase-merges.sh   | 179 +
5 files changed, 206 insertions(+), 2 deletions(-)
create mode 100755 t/t3430-rebase-merges.sh

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 3277ca14327..34e0f6a69c1 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -378,6 +378,23 @@ The commit list format can be changed by setting the
configuration option
rebase.instructionFormat.  A customized instruction format will
automatically
have the long commit hash prepended to the format.

+-r::
+--rebase-merges::
+ By default, a rebase will simply drop merge commits and only rebase
+ the non-merge commits. With this option, it will try to preserve
+ the branching structure within the commits that are to be rebased,
+ by recreating the merge commits. If a merge commit resolved any merge
+ or contained manual amendments, then they will have to be re-applied
+ manually.
++
+This mode is similar in spirit to `--preserve-merges`, but in contrast to
+that option works well in interactive rebases: commits can 

Re: [PATCH v3 0/6] Keep all info in command-list.txt in git binary

[Ramsay Jones]

On 21/04/18 17:56, Duy Nguyen wrote:
> On Sat, Apr 21, 2018 at 06:54:08PM +0200, Nguyễn Thái Ngọc Duy wrote:
>> Changes:
>>
>> - remove the deprecated column in command-list.txt. My change break it
>>   anyway if anyone uses it.
>> - fix up failed tests that I marked in the RFC and kinda forgot about it.
>> - fix bashisms in generate-cmdlist.sh
>> - fix segfaul in "git help"
> 
> Sorry I forgot the interdiff
> 
[snip]

> diff --git a/t/t0012-help.sh b/t/t0012-help.sh
> index fd2a7f27dc..53208ab20e 100755
> --- a/t/t0012-help.sh
> +++ b/t/t0012-help.sh
> @@ -25,6 +25,15 @@ test_expect_success "setup" '
>   EOF
>  '
>  
> +# make sure to exercise these code paths, the output is a bit tricky
> +# to verify
> +test_expect_success 'basic help commands' '
> + git help >/dev/null &&
> + git help -a >/dev/null &&
> + git help -g >/dev/null &&
> + git help -av >/dev/null
> +'
> +
I think you need to try a little harder than this! ;-)

This test would not have noticed the recent failure (what annoyed me was
that git build without error and then the test-suite sailed through with
nary a squeak of complaint). Viz:

  $ ./git help
  usage: git [--version] [--help] [-C ] [-c =]
 [--exec-path[=]] [--html-path] [--man-path] [--info-path]
 [-p | --paginate | --no-pager] [--no-replace-objects] [--bare]
 [--git-dir=] [--work-tree=] [--namespace=]
  []
  
  These are common Git commands used in various situations:
  
  'git help -a' and 'git help -g' list available subcommands and some
  concept guides. See 'git help ' or 'git help '
  to read about a specific subcommand or concept.
  $ echo $?
  0
  $ 

  $ ./git help -g
  The common Git guides are:
  
  
  'git help -a' and 'git help -g' list available subcommands and some
  concept guides. See 'git help ' or 'git help '
  to read about a specific subcommand or concept.
  $ echo $?
  0
  $ 

  $ ./git help -av
  Main Porcelain Commands
  
  
  Ancillary Commands / Manipulators
  
  
  Ancillary Commands / Interrogators
  
  
  Interacting with Others
  
  
  Low-level Commands / Manipulators
  
  
  Low-level Commands / Interrogators
  
  
  Low-level Commands / Synching Repositories
  
  
  Low-level Commands / Internal Helpers
  
  $ echo $?
  0
  $ 
 
I started to add some tests, like so:

  diff --git a/t/t0012-help.sh b/t/t0012-help.sh
  index fd2a7f27d..7e10c2862 100755
  --- a/t/t0012-help.sh
  +++ b/t/t0012-help.sh
  @@ -49,6 +49,22 @@ test_expect_success "--help does not work for guides" "
  test_i18ncmp expect actual
   "
   
  +test_expect_success 'git help' '
  +   git help >help.output &&
  +   test_i18ngrep "^   clone  " help.output &&
  +   test_i18ngrep "^   add" help.output &&
  +   test_i18ngrep "^   log" help.output &&
  +   test_i18ngrep "^   commit " help.output &&
  +   test_i18ngrep "^   fetch  " help.output
  +'
  +
  +test_expect_success 'git help -g' '
  +   git help -g >help.output &&
  +   test_i18ngrep "^   attributes " help.output &&
  +   test_i18ngrep "^   everyday   " help.output &&
  +   test_i18ngrep "^   tutorial   " help.output
  +'
  +
   test_expect_success 'generate builtin list' '
  git --list-cmds=builtins >builtins
   '

These tests, while not rigorous, did at least correctly catch the bad
build for me. I was about to add the obvious one for 'git help -av',
when I decided to catch the problem 'at source', viz:

  diff --git a/help.c b/help.c
  index a47f7e2c4..13790bb54 100644
  --- a/help.c
  +++ b/help.c
  @@ -196,6 +196,9 @@ static void extract_common_cmds(struct cmdname_help 
**p_common_cmds,
int i, nr = 0;
struct cmdname_help *common_cmds;
   
  + if (ARRAY_SIZE(command_list) == 0)
  + BUG("empty command_list");
  +
ALLOC_ARRAY(common_cmds, ARRAY_SIZE(command_list));
   
for (i = 0; i < ARRAY_SIZE(command_list); i++) {
  @@ -279,6 +282,9 @@ void list_porcelain_cmds(void)
int i, nr = ARRAY_SIZE(command_list);
struct cmdname_help *cmds = command_list;
   
  + if (nr == 0)
  + BUG("empty command_list");
  +
for (i = 0; i < nr; i++) {
if (cmds[i].category != CAT_mainporcelain)
continue;
  @@ -321,6 +327,9 @@ void list_common_guides_help(void)
int nr = ARRAY_SIZE(command_list);
struct cmdname_help *cmds = command_list;
   
  + if (nr == 0)
  + BUG("empty command_list");
  +
QSORT(cmds, nr, cmd_category_cmp);
   
for (i = 0; i < nr; i++) {
  @@ -343,6 +352,9 @@ void list_all_cmds_help(void)
int nr = ARRAY_SIZE(command_list);
struct cmdname_help *cmds = command_list;
   
  + if (nr == 0)
  + BUG("empty command_list");
  +
for (i = 0; i < nr; i++) {
struct cmdname_help *cmd = cmds + i;
   
  
This had a very dramatic effect on the test-suite, since every single
test file 

Re: [PATCH v8 09/16] rebase: introduce the --rebase-merges option

[Philip Oakley]

From: "Johannes Schindelin" 

Once upon a time, this here developer thought: wouldn't it be nice if,
say, Git for Windows' patches on top of core Git could be represented as
a thicket of branches, and be rebased on top of core Git in order to
maintain a cherry-pick'able set of patch series?

The original attempt to answer this was: git rebase --preserve-merges.

However, that experiment was never intended as an interactive option,
and it only piggy-backed on git rebase --interactive because that
command's implementation looked already very, very familiar: it was
designed by the same person who designed --preserve-merges: yours truly.

Some time later, some other developer (I am looking at you, Andreas!
;-)) decided that it would be a good idea to allow --preserve-merges to
be combined with --interactive (with caveats!) and the Git maintainer
(well, the interim Git maintainer during Junio's absence, that is)
agreed, and that is when the glamor of the --preserve-merges design
started to fall apart rather quickly and unglamorously.

The reason? In --preserve-merges mode, the parents of a merge commit (or
for that matter, of *any* commit) were not stated explicitly, but were
*implied* by the commit name passed to the `pick` command.

Aside: I think this para should be extracted to the --preserve-merges 
documentation to highlight what it does / why it is 'wrong' (not what would 
be expected in some case). It may also need to discuss the (figurative) 
Cousins vs. Siblings distinction [merge of branches external, or internal, 
to the rebase.


"In --preserve-merges, the commit being selected for merging is implied by 
the commit name  passed to the `pick` command (i.e. of the original merge 
commit), not that of the rebased version of that parent."


A similar issue occurs with (figuratively) '--ancestry-path --first parent' 
searches which lacks the alternate '--lead parent' post-walk selection. [1]. 
I don't think there is a dot notation to select the merge cousins, nor merge 
siblings either A.,B ? (that's dot-comma ;-)



This made it impossible, for example, to reorder commits. Not to mention
to flatten the branch topology or, deity forbid, to split topic branches
into two.

Alas, these shortcomings also prevented that mode (whose original
purpose was to serve Git for Windows' needs, with the additional hope
that it may be useful to others, too) from serving Git for Windows'
needs.

Five years later, when it became really untenable to have one unwieldy,
big hodge-podge patch series of partly related, partly unrelated patches
in Git for Windows that was rebased onto core Git's tags from time to
time (earning the undeserved wrath of the developer of the ill-fated
git-remote-hg series that first obsoleted Git for Windows' competing
approach, only to be abandoned without maintainer later) was really
untenable, the "Git garden shears" were born [*1*/*2*]: a script,
piggy-backing on top of the interactive rebase, that would first
determine the branch topology of the patches to be rebased, create a
pseudo todo list for further editing, transform the result into a real
todo list (making heavy use of the `exec` command to "implement" the
missing todo list commands) and finally recreate the patch series on
top of the new base commit.

That was in 2013. And it took about three weeks to come up with the
design and implement it as an out-of-tree script. Needless to say, the
implementation needed quite a few years to stabilize, all the while the
design itself proved itself sound.

With this patch, the goodness of the Git garden shears comes to `git
rebase -i` itself. Passing the `--rebase-merges` option will generate
a todo list that can be understood readily, and where it is obvious
how to reorder commits. New branches can be introduced by inserting
`label` commands and calling `merge `. And once this mode will
have become stable and universally accepted, we can deprecate the design
mistake that was `--preserve-merges`.

Link *1*:
https://github.com/msysgit/msysgit/blob/master/share/msysGit/shears.sh
Link *2*:
https://github.com/git-for-windows/build-extra/blob/master/shears.sh

Signed-off-by: Johannes Schindelin 
---
Documentation/git-rebase.txt   |  20 ++-
contrib/completion/git-completion.bash |   2 +-
git-rebase--interactive.sh |   1 +
git-rebase.sh  |   6 +
t/t3430-rebase-merges.sh   | 179 +
5 files changed, 206 insertions(+), 2 deletions(-)
create mode 100755 t/t3430-rebase-merges.sh

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 3277ca14327..34e0f6a69c1 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -378,6 +378,23 @@ The commit list format can be changed by setting the 
configuration option
rebase.instructionFormat.  A customized instruction format will 
automatically

have the long commit hash prepended to the 

Re: RFC: How should we handle un-deleted remote branches?

[Andreas Heiduk]

Am 22.04.2018 um 13:17 schrieb Ævar Arnfjörð Bjarmason:
> 
> On Sun, Apr 22 2018, Andreas Heiduk wrote:
> 
>> Am 20.04.2018 um 14:14 schrieb Ævar Arnfjörð Bjarmason:
>>> But this is a possible work-around:
>>>
>>> git init /tmp/empty.git
>>> git remote add avar file:///tmp/empty.git
>>> git remote prune avar
>>> git remote remove avar
>>
>> This won't do it also?
>>
>>  git remote prune origin
> 
> Yes, in this particular case, but that's just emergent behavior in how
> we handle refspec prunign, and the fact that it "works" is arguably a
> bug in "prune". i.e. this:

Its not emergent because "origin" is the other remote responsible for
that ref and cleaning stuff "belonging" to the remote is the job
description (I'm arguing from a user's perspective, not as a git-developer).

> 
> (
> rm -rf /tmp/git &&
> git clone --bare --mirror g...@github.com:git/git.git /tmp/git &&
> cd /tmp/git &&
> git remote add avar g...@github.com:avar/git.git &&
> git remote add peff g...@github.com:peff/git.git &&
> git fetch --all &&
> git remote remove avar &&
> git remote prune origin
> )
> 
> Will delete all the avar/* and peff/* branches, even though I still have
> a "peff" remote.

Exactly my point: When you are in a the bad situation of "shared
responsibility", then there is no easy and always correct way out,
because there are uncountable possible situations.

To give another, slightly modified example expanding the problem space:

(
rm -rf /tmp/git &&
git clone --bare --mirror https://github.com/git/git.git /tmp/git &&
cd /tmp/git &&
git remote add github https://github.com/avar/git.git &&
git fetch github &&
git fetch origin &&
# note new fetches for "github/master" using with "(forced update)"
)

For ... reasons the first repo publishes some references like

github/maint
github/master
github/pu

So when this repo is mirrored AND another, suitably named remote is
added then there will be also namespace conflicts. You can call

git fetch github
git fetch origin

in a loop and most likely each fetch will update the same refs, always
toggling between two states.

So: not only "remote remove" and "remote prune" are at stake but every
command handling remote references.

How should "git remote remove github" work in both situations? Remove
the refs/remotes/github/master & co? remove them only if the last fetch
was for "github" but not when the last update was for "origin"? Should
"git fetch" use the same logic?

So it seems better to me to avoid that bad situation altogether. Don't
allow overlapping/conflicting refspecs when adding a new remote. Using
*your* last examples both

> git remote add avar g...@github.com:avar/git.git &&
> git remote add peff g...@github.com:peff/git.git &&

should have failed and hence the "prune" problems won't exist. Same for
my example.

>> Possible 5):
>>
>>  Don't fix "git remote remove" but "git remote add" to complain that its
>> ref-namespace is already occupied by some other remote. Add "--force"
>> for the experts.
> 
> Indeed, that's another bug here, i.e. in the above example:
> 
> git remote remove peff && # won't delete peff/ branches
> git remote add peff g...@github.com:peff/git.git
> 
> Will happily add the "peff" remote again, even though as you point out
> it could be an entirely different remote.

Ummm. That was not my point. My is: "git clone --mirror" uses a refspec

fetch = +refs/*:refs/*

and hence "occupies" the complete "refs/*" namespace. So adding another
remote (for the first time or for the second time is as irrelevant as
the url) will use

fetch = +refs/heads/*:refs/remotes/peff/*

and now the "refs/remotes/peff/*" part is in conflict with "refs/*" from
above. The conflict exists not only for "prune" or "remove" but also for
"fetch", "rename" (didn't check) .

This kind of conflict should not be allowed right from the start - when
the first "git remote add peff..." is executed. Then prune, remove AND
fetch would be OK.

Re: [PATCH v8 08/16] rebase-helper --make-script: introduce a flag to rebase merges

[Philip Oakley]

From: "Johannes Schindelin" 

Sorry for the very late in the series comments..


The sequencer just learned new commands intended to recreate branch
structure (similar in spirit to --preserve-merges, but with a
substantially less-broken design).

Let's allow the rebase--helper to generate todo lists making use of
these commands, triggered by the new --rebase-merges option. For a
commit topology like this (where the HEAD points to C):

- A - B - C
\   /
  D

the generated todo list would look like this:

# branch D
pick 0123 A
label branch-point
pick 1234 D
label D

reset branch-point
pick 2345 B
merge -C 3456 D # C

To keep things simple, we first only implement support for merge commits
with exactly two parents, leaving support for octopus merges to a later
patch series.


For the first time reader this (below) isn't as obvious as may be thought.
maybe we should be a little more explicit here.


As a special, hard-coded label, all merge-rebasing todo lists start with
the command `label onto`


.. which labels the start point head with the name 'onto' ...

Maybe even:
"All merge-rebasing todo lists start with, as a convenience, a hard-coded
`label onto` line which will label the start point's head" ...


   so that we can later always refer to the revision
onto which everything is rebased.

Signed-off-by: Johannes Schindelin 
---
builtin/rebase--helper.c |   4 +-
sequencer.c  | 351 ++-
sequencer.h  |   1 +
3 files changed, 353 insertions(+), 3 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index ad074705bb5..781782e7272 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -12,7 +12,7 @@ static const char * const builtin_rebase_helper_usage[]
= {
int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
{
 struct replay_opts opts = REPLAY_OPTS_INIT;
- unsigned flags = 0, keep_empty = 0;
+ unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
 int abbreviate_commands = 0;
 enum {
 CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
@@ -24,6 +24,7 @@ int cmd_rebase__helper(int argc, const char **argv,
const char *prefix)
 OPT_BOOL(0, "keep-empty", _empty, N_("keep empty commits")),
 OPT_BOOL(0, "allow-empty-message", _empty_message,
 N_("allow commits with empty messages")),
+ OPT_BOOL(0, "rebase-merges", _merges, N_("rebase merge
commits")),
 OPT_CMDMODE(0, "continue", , N_("continue rebase"),
 CONTINUE),
 OPT_CMDMODE(0, "abort", , N_("abort rebase"),
@@ -57,6 +58,7 @@ int cmd_rebase__helper(int argc, const char **argv,
const char *prefix)

 flags |= keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
 flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
+ flags |= rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
 flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;

 if (command == CONTINUE && argc == 1)
diff --git a/sequencer.c b/sequencer.c
index 5944d3a34eb..1e17a11ca32 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -25,6 +25,8 @@
#include "sigchain.h"
#include "unpack-trees.h"
#include "worktree.h"
+#include "oidmap.h"
+#include "oidset.h"

#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"

@@ -3436,6 +3438,343 @@ void append_signoff(struct strbuf *msgbuf, int
ignore_footer, unsigned flag)
 strbuf_release();
}

+struct labels_entry {
+ struct hashmap_entry entry;
+ char label[FLEX_ARRAY];
+};
+
+static int labels_cmp(const void *fndata, const struct labels_entry *a,
+   const struct labels_entry *b, const void *key)
+{
+ return key ? strcmp(a->label, key) : strcmp(a->label, b->label);
+}
+
+struct string_entry {
+ struct oidmap_entry entry;
+ char string[FLEX_ARRAY];
+};
+
+struct label_state {
+ struct oidmap commit2label;
+ struct hashmap labels;
+ struct strbuf buf;
+};
+
+static const char *label_oid(struct object_id *oid, const char *label,
+  struct label_state *state)
+{
+ struct labels_entry *labels_entry;
+ struct string_entry *string_entry;
+ struct object_id dummy;
+ size_t len;
+ int i;
+
+ string_entry = oidmap_get(>commit2label, oid);
+ if (string_entry)
+ return string_entry->string;
+
+ /*
+ * For "uninteresting" commits, i.e. commits that are not to be
+ * rebased, and which can therefore not be labeled, we use a unique
+ * abbreviation of the commit name. This is slightly more complicated
+ * than calling find_unique_abbrev() because we also need to make
+ * sure that the abbreviation does not conflict with any other
+ * label.
+ *
+ * We disallow "interesting" commits to be labeled by a string that
+ * is a valid full-length hash, to ensure that we always can find an
+ * abbreviation for any uninteresting commit's names that does not
+ * clash with any other label.
+ */
+ if (!label) {
+ char *p;
+
+ strbuf_reset(>buf);
+ strbuf_grow(>buf, GIT_SHA1_HEXSZ);
+ label = p = state->buf.buf;
+
+ find_unique_abbrev_r(p, oid, default_abbrev);
+
+ /*
+ * We may need to extend the 

Re: [RFC PATCH v10 32.5/36] unpack_trees: fix memory corruption with split_index when src != dst

[Duy Nguyen]

On Sat, Apr 21, 2018 at 9:37 PM, Elijah Newren  wrote:
> Currently, all callers of unpack_trees() set o->src_index == o->dst_index.
> Since we create a temporary index in o->result, then discard o->dst_index
> and overwrite it with o->result, when o->src_index == o->dst_index it is
> safe to just reuse o->src_index's split_index for o->result.  However,
> o->src_index and o->dst_index are specified separately in order to allow
> callers to have these be different.  In such a case, reusing
> o->src_index's split_index for o->result will cause the split_index to be
> shared.  If either index then has entries replaced or removed, it will
> result in the other index referring to free()'d memory.
>
> Signed-off-by: Elijah Newren 
> ---
>
> I still haven't wrapped my head around the split_index stuff entirely, so
> it's possible that
>
>   - the performance optimization isn't even valid when src == dst.  Could
> the original index be different enough from the result that we don't
> want its split_index?

This really depends on the use case of course. But when git checkout
is used for switching branches, unpack-trees will be used and unless
you switch between to vastly different branches, the updated entries
may be small compared to the entire index that sharing is still good.
If the result index is so different that it results in a huge index
file anyway, I believe we have code to recreate a new shared index to
keep its size down next time.

>   - there's a better, more performant fix or there is some way to actually
> share a split_index between two independent index_state objects.

A cleaner way of doing this would be something to the line [1]

move_index_extensions(>result, o->dst_index);

near the end of this function. This could be where we compare the
result index with the source index's shared file and see if it's worth
keeping the shared index or not. Shared index is designed to work with
huge index files though, any operations that go through all index
entries will usually not be cheap. But at least it's safer.

> However, with this fix, all the tests pass both normally and under
> GIT_TEST_SPLIT_INDEX=DareISayYes.  Without this patch, when
> GIT_TEST_SPLIT_INDEX is set, my directory rename detection series will fail
> several tests, as reported by SZEDER.

Yes, the change looks good.

[1] To me the second parameter should be src_index, not dst_index.
We're copying entries from _source_ index to "result" and we should
also copy extensions from the source index. That line happens to work
only when dst_index is the same as src_index, which is the common use
case so far.

>  unpack-trees.c | 15 +--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 79fd97074e..b670415d4c 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1284,9 +1284,20 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
> struct unpack_trees_options
> o->result.timestamp.sec = o->src_index->timestamp.sec;
> o->result.timestamp.nsec = o->src_index->timestamp.nsec;
> o->result.version = o->src_index->version;
> -   o->result.split_index = o->src_index->split_index;
> -   if (o->result.split_index)
> +   if (!o->src_index->split_index) {
> +   o->result.split_index = NULL;
> +   } else if (o->src_index == o->dst_index) {
> +   /*
> +* o->dst_index (and thus o->src_index) will be discarded
> +* and overwritten with o->result at the end of this function,
> +* so just use src_index's split_index to avoid having to
> +* create a new one.
> +*/
> +   o->result.split_index = o->src_index->split_index;
> o->result.split_index->refcount++;
> +   } else {
> +   o->result.split_index = init_split_index(>result);
> +   }
> hashcpy(o->result.sha1, o->src_index->sha1);
> o->merge_size = len;
> mark_all_ce_unused(o->src_index);
> --
> 2.17.0.296.gaac25b4b81
>



-- 
Duy

Re: [PATCH v1 1/2] merge: Add merge.renames config setting

[Eckhard Maaß]

On Fri, Apr 20, 2018 at 11:34:25AM -0700, Elijah Newren wrote:
> Sorry, I think I wasn't being clear.  The documentation for the config
> options for e.g. diff.renameLimit, fetch.prune, log.abbrevCommit, and
> merge.ff all mention the equivalent command line parameters.  Your
> patch doesn't do that for merge.renames, but I think it would be
> helpful if it did.

I wonder here what the relation to the diff.* options should be in this
regard anyway. There is already diff.renames. Naively, I would assume
that these options are in sync, that is you control the behavior of both
the normal diff family like git show and git merge. The reasoning, at
least for me, is to keep consistency between the outcome of rename
detection while merging and a later simple "git show MERGE_BASE..HEAD".
I would expect those to give me the same style of rename detection.

Hence, I would like to use diff.renames and maybe enhance this option to
also carry the score in backward compatible way (or introduce a second
configuration option?). Is this idea going in a good direction? If yes,
I will try to submit a patch for this.

Ah, by the way: for people that have not touched diff.renames there will
be no visible change in how Git behaves - the default for diff.renames
is a rename with 50% score with is the same for merge. So it will only
change if one has tweaked diff.renames already. But I wonder if one does
that and expect the merge to use a different rename detection anyway.

Greetings,
Eckhard

Re: [PATCH v8 06/16] sequencer: introduce the `merge` command

[Philip Oakley]

From: "Johannes Schindelin" 

This patch is part of the effort to reimplement `--preserve-merges` with
a substantially improved design, a design that has been developed in the
Git for Windows project to maintain the dozens of Windows-specific patch
series on top of upstream Git.

The previous patch implemented the `label` and `reset` commands to label


The previous patch was [Patch 05/16] git-rebase--interactive: clarify
arguments, so this statement doesn't appear to be true. Has a patch been
missed or re-ordered? Or should it be simply "This patch implements" ?
Likewise the patch subject would be updated.


commits and to reset to labeled commits. This patch adds the `merge`


s/adds/also adds/ ?


command, with the following syntax:

merge [-C ]  # 

The  parameter in this instance is the *original* merge commit,
whose author and message will be used for the merge commit that is about
to be created.

The  parameter refers to the (possibly rewritten) revision to
merge. Let's see an example of a todo list:


The example ought to also note that `label onto` is to
`# label current HEAD with a name`, seeing as this is the first occurance.
It may be obvious in retrospect, but not at first reading.


label onto

# Branch abc
reset onto


Is this reset strictly necessary. We are already there @head.


pick deadbeef Hello, world!
label abc

reset onto
pick cafecafe And now for something completely different
merge -C baaabaaa abc # Merge the branch 'abc' into master

To edit the merge commit's message (a "reword" for merges, if you will),
use `-c` (lower-case) instead of `-C`; this convention was borrowed from
`git commit` that also supports `-c` and `-C` with similar meanings.

To create *new* merges, i.e. without copying the commit message from an
existing commit, simply omit the `-C ` parameter (which will
open an editor for the merge message):

merge abc

This comes in handy when splitting a branch into two or more branches.

Note: this patch only adds support for recursive merges, to keep things
simple. Support for octopus merges will be added later in a separate
patch series, support for merges using strategies other than the
recursive merge is left for the future.

Signed-off-by: Johannes Schindelin 
---
git-rebase--interactive.sh |   6 +
sequencer.c| 407 -
2 files changed, 406 insertions(+), 7 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index e1b865f43f2..ccd5254d1c9 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -162,6 +162,12 @@ s, squash  = use commit, but meld into
previous commit
f, fixup  = like \"squash\", but discard this commit's log message
x, exec  = run command (the rest of the line) using shell
d, drop  = remove commit
+l, label  = label current HEAD with a name
+t, reset  = reset HEAD to a label
+m, merge [-C  | -c ]  [# ]
+.   create a merge commit using the original merge commit's
+.   message (or the oneline, if no original merge commit was
+.   specified). Use -c  to reword the commit message.

These lines can be re-ordered; they are executed from top to bottom.
" | git stripspace --comment-lines >>"$todo"
diff --git a/sequencer.c b/sequencer.c
index 01443e0f245..35fcacbdf0f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -23,6 +23,8 @@
#include "hashmap.h"
#include "notes-utils.h"
#include "sigchain.h"
+#include "unpack-trees.h"
+#include "worktree.h"

#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"

@@ -120,6 +122,13 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha,
"rebase-merge/stopped-sha")
static GIT_PATH_FUNC(rebase_path_rewritten_list,
"rebase-merge/rewritten-list")
static GIT_PATH_FUNC(rebase_path_rewritten_pending,
 "rebase-merge/rewritten-pending")
+
+/*
+ * The path of the file listing refs that need to be deleted after the
rebase
+ * finishes. This is used by the `label` command to record the need for
cleanup.
+ */
+static GIT_PATH_FUNC(rebase_path_refs_to_delete,
"rebase-merge/refs-to-delete")
+
/*
 * The following files are written by git-rebase just after parsing the
 * command-line (and are only consumed, not modified, by the sequencer).
@@ -244,18 +253,34 @@ static const char *gpg_sign_opt_quoted(struct
replay_opts *opts)

int sequencer_remove_state(struct replay_opts *opts)
{
- struct strbuf dir = STRBUF_INIT;
+ struct strbuf buf = STRBUF_INIT;
 int i;

+ if (is_rebase_i(opts) &&
+ strbuf_read_file(, rebase_path_refs_to_delete(), 0) > 0) {
+ char *p = buf.buf;
+ while (*p) {
+ char *eol = strchr(p, '\n');
+ if (eol)
+ *eol = '\0';
+ if (delete_ref("(rebase -i) cleanup", p, NULL, 0) < 0)
+ warning(_("could not delete '%s'"), p);
+ if (!eol)
+ break;
+ p = eol + 1;
+ }
+ }
+
 free(opts->gpg_sign);
 free(opts->strategy);
 for (i = 0; i < opts->xopts_nr; i++)
 free(opts->xopts[i]);
 free(opts->xopts);

- strbuf_addstr(, get_dir(opts));
- remove_dir_recursively(, 0);
- strbuf_release();
+ 

Re: RFC: How should we handle un-deleted remote branches?

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 22 2018, Andreas Heiduk wrote:

> Am 20.04.2018 um 14:14 schrieb Ævar Arnfjörð Bjarmason:
>> But this is a possible work-around:
>>
>> git init /tmp/empty.git
>> git remote add avar file:///tmp/empty.git
>> git remote prune avar
>> git remote remove avar
>
> This won't do it also?
>
>   git remote prune origin

Yes, in this particular case, but that's just emergent behavior in how
we handle refspec prunign, and the fact that it "works" is arguably a
bug in "prune". i.e. this:

(
rm -rf /tmp/git &&
git clone --bare --mirror g...@github.com:git/git.git /tmp/git &&
cd /tmp/git &&
git remote add avar g...@github.com:avar/git.git &&
git remote add peff g...@github.com:peff/git.git &&
git fetch --all &&
git remote remove avar &&
git remote prune origin
)

Will delete all the avar/* and peff/* branches, even though I still have
a "peff" remote.

IOW the guarding logic we have in add_branch_for_removal() for not
deleting the branches of other remotes isn't in the corresponding
"prune" function, and that's a bug.

In the specific example I picked "git remote prune origin" just so
happens to do the right thing since I have no other active remote, and
there *is* an alive remote so I can "prune" against it, but it doesn't
help in the general case. In my case I have a remote URL for a git
server called "upstream" that doesn't exist anymore (but as noted, I can
fake it with an empty repo...)>


>> I started to patch this, but I'm not sure what to do here. we could do
>> some combination of:
>>
>>  0. Just document the current behavior and leave it.
>>
>>  1. Dig further down to see what other remotes reference these refs, and
>> just ignore any refspecs that don't explicitly reference
>> refs/remotes//*.
>>
>> I.e. isn't the intention here to preserve a case where you have two
>> URLs for the same effective remote, not whene you have something
>> like a --mirror refspec? Unfortunately I can't ask the original
>> author :(
>>
>>  2. Warn about each ref we didn't delete, or at least warn saying
>> there's undeleted refs under refs/remotes//*.
>>
>>  3. Make 'git remote remove --force-deletion ' (or whatever the
>> flag is called) be a thing. But unless we do the next item this
>> won't be useful.
>>
>>  4. Make 'git remote prune ' work in cases where we don't have a
>> remote called  anymore, just falling back to deleting
>> refs/remotes/. In this case 'git remote remove
>> --force-deletion ' would also do the same thing.
>
> Possible 5):
>
>   Don't fix "git remote remove" but "git remote add" to complain that its
> ref-namespace is already occupied by some other remote. Add "--force"
> for the experts.

Indeed, that's another bug here, i.e. in the above example:

git remote remove peff && # won't delete peff/ branches
git remote add peff g...@github.com:peff/git.git

Will happily add the "peff" remote again, even though as you point out
it could be an entirely different remote.

Re: RFC: How should we handle un-deleted remote branches?

[Andreas Heiduk]

Am 20.04.2018 um 14:14 schrieb Ævar Arnfjörð Bjarmason:
> But this is a possible work-around:
> 
> git init /tmp/empty.git
> git remote add avar file:///tmp/empty.git
> git remote prune avar
> git remote remove avar

This won't do it also?

git remote prune origin


> I started to patch this, but I'm not sure what to do here. we could do
> some combination of:
> 
>  0. Just document the current behavior and leave it.
> 
>  1. Dig further down to see what other remotes reference these refs, and
> just ignore any refspecs that don't explicitly reference
> refs/remotes//*.
> 
> I.e. isn't the intention here to preserve a case where you have two
> URLs for the same effective remote, not whene you have something
> like a --mirror refspec? Unfortunately I can't ask the original
> author :(
> 
>  2. Warn about each ref we didn't delete, or at least warn saying
> there's undeleted refs under refs/remotes//*.
> 
>  3. Make 'git remote remove --force-deletion ' (or whatever the
> flag is called) be a thing. But unless we do the next item this
> won't be useful.
> 
>  4. Make 'git remote prune ' work in cases where we don't have a
> remote called  anymore, just falling back to deleting
> refs/remotes/. In this case 'git remote remove
> --force-deletion ' would also do the same thing.

Possible 5):

Don't fix "git remote remove" but "git remote add" to complain that its
ref-namespace is already occupied by some other remote. Add "--force"
for the experts.

Re: Is support for 10.8 dropped?

[Perry Hutchison]

Eric Sunshine  wrote:
> On Sun, Apr 22, 2018 at 1:15 AM, Igor Korot  wrote:
> > MyMac:git-2.17.0 igorkorot$ cat config.mak
> > NO_GETTEXT=Yes
> > NO_OPENSSL=Yes
> >
> > MyMac:dbhandler igorkorot$ /Users/igorkorot/git-2.17.0/git pull
> > fatal: unable to access
> > 'https://github.com/oneeyeman1/dbhandler.git/': error:1407742E:SSL
> > routines:SSL23_GET_SERVER_HELLO:tlsv1 alert protocol version
> > MyMac:dbhandler igorkorot$
>
> Try re-building with OpenSSL enabled (remove NO_OPENSSL from
> config.make). You may need to build/install OpenSSL yourself to get
> this to work.

Explanation:  "tlsv1 alert protocol version" means that the server
requires its clients to use a newer version of TLS than what was
used in the local build of git.