Re: cherry-pick --no-commit does not work well with --continue in case of conflicts

[Junio C Hamano]

Ilya Kantor  writes:

> P.S. I assume, `cherry-pick -n ` is meant to merge given
> commits' changes into the current working directory and the index,
> without making new commits, for any given set of commits, hope that's right?

Hmph.

One step in cherry-pick should refuse to start when HEAD and the index
does not match, even though it is perfectly OK if the working tree
files do not match the index, as long as such local changes do not
interfere with the change the cherry-pick tries to bring in.

The requirement for the index to be clean wrt the HEAD is
fundamental.  When any merge-y operation like cherry-pick, apply -3,
checkout -m, etc., happens, we would want to

 * store the cleanly automerged contents to the index

 * store common-ancestor, ours and theirs for conflicted merge to
   the stages in the index.

and being able to safely say "git reset" (not "reset --hard") to
bring the index back to the state before the merge-y operation has
started.  Not noticing a dirty index and starting a step in cherry-pick
means you cannot tell cleanly automerged paths from paths you had
modified in the index _before_ the step started.

And if you have a range that consists of two commits and
successfully did "cherry-pick -n" on the first one, because the
command is not committing, these cleanly merged paths will be
modified in the index.  Then the next step to pick the second commit
may conflict---after that, you lose the result of the first pick
from the index as some changes from the second step is already
intermixed with the result from the first step in the index.

So, no.  I do not think it makes sense to feed multiple commits to
"cherry-pick -n".

Re: [PATCH 8/8] fetch: stop clobbering existing tags without --force

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> > Tags need not be pointing at commits so there is no way to
> > guarantee "fast-forward" anyway.

The observation the above statement makes is not incorrect per-se,
but it does not justify "anything goes".  "nothing is allowed unless
forced" is equally a logical consequence of the observation.

> That comment and the rest of the history of "fetch" shows that the
> "+" (--force) part of refpecs was only conceived for branch updates,
> while tags have accepted any changes from upstream unconditionally and
> clobbered the local tag object. Changing this behavior has been
> discussed as early as 2011[1].

Thanks for a pointer.  We didn't keep reflog on tags as we wanted
tags to be fixed points and made --tags a refspec without leading
'+' because we didn't want this local clobbering.  I'd say it is
just a buggy implementation, and we should just implement a simple
rule "refs/tags/* is never updated unless forced".

> I the current behavior doesn't make sense, it easily results in local

s/I the/To me, the/, or s/I the/The/.

> tags accidentally being clobbered. Ideally we'd namespace our tags
> per-remote, but as with my 97716d217c ("fetch: add a --prune-tags
> option and fetch.pruneTags config", 2018-02-09) it's easier to work
> around the current implementation than to fix the root cause,

I do not think they are the same problem.

You can have refs/remote/$name/v1.0 and have look-up rules to peek
at various places in refs/* hierarchy for v1.0, and you may have
*solved* the "oops I overwrote and the meaning of v1.0 suddenly
changed" issue, but if you fetched to a location in refs/* that has
higher precedence, then "oops, the meaning of v1.0 suddenly changed"
issue itself is *not* solved at all.

> so this
> implements suggestion #1 from [1], "fetch" now only clobbers the tag
> if either "+" is provided as part of the refspec, or if "--force" is
> provided on the command-line.

Good.  Regardless of the issue of separate namespace that is
overlayed at the look-up time, this makes tons of sense.

> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> index 8631e365f4..5b4fc36866 100644
> --- a/Documentation/fetch-options.txt
> +++ b/Documentation/fetch-options.txt
> @@ -49,11 +49,16 @@ endif::git-pull[]
>  
>  -f::
>  --force::
> - When 'git fetch' is used with `:`
> - refspec, it refuses to update the local branch
> - `` unless the remote branch `` it
> - fetches is a descendant of ``.  This option
> - overrides that check.
> + When 'git fetch' is used with `:` refspec it might

Nice to see attention to the detail here.  s/might/may/, I would
say, though.

> + refuse to update the local branch as discussed

> diff --git a/Documentation/pull-fetch-param.txt 
> b/Documentation/pull-fetch-param.txt
> index c579793af5..672e8bc1c0 100644
> --- a/Documentation/pull-fetch-param.txt
> +++ b/Documentation/pull-fetch-param.txt
> @@ -32,12 +32,22 @@ name.
>  `tag ` means the same as `refs/tags/:refs/tags/`;
>  it requests fetching everything up to the given tag.
>  +
> -The remote ref that matches 
> -is fetched, and if  is not empty string, the local
> -ref that matches it is fast-forwarded using .
> -If the optional plus `+` is used, the local ref
> -is updated even if it does not result in a fast-forward
> -update.
> +The remote ref that matches  is fetched, and if  is not
> +empty string, an attempt is made to update the local ref that matches
> +it.
> ++
> +Whether that update is allowed is confusingly not the inverse of
> +whether a server will accept a push as described in the `...`
> +section of linkgit:git-push[1]. If it's a commit under `refs/heads/*`
> +only fast-forwards are allowed,

Perhaps correct.  It is unclear what happens when it is fetching
non-commit to refs/heads/* in the above sentence.

> but unlike what linkgit:git-push[1]
> +will accept clobbering any ref pointing to blobs, trees etc. in any
> +other namespace will be accepted, but commits in any ref
> +namespace. ...

I cannot quite parse this.

> +... Those apply the same fast-forward rule.

Who are "Those"?  refs/poo/*?

> +... An exception to
> +this is that as of Git version 2.18 any object under `refs/tags/*` is
> +protected from updates.

OK.

> +If the optional plus `+` is used, the local ref is updated if the

Tighten "is used" to claify that you are talking about the '+'
prefix that signals a forced push/fetch.  We do not want to hear
from people who complain their "git fetch origin master+" does not
work.

> - OPT__FORCE(, N_("force overwrite of local branch"), 0),
> + OPT__FORCE(, N_("force overwrite of local reference"), 0),

Good.  This is long overdue.

Re: [PATCH 5/8] push doc: correct lies about how push refspecs work

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

>  +
>  The  is often the name of the branch you would want to push, but
> -it can be any arbitrary "SHA-1 expression", such as `master~4` or
> -`HEAD` (see linkgit:gitrevisions[7]).
> +it can be any arbitrary "SHA-1 expression" referring to a branch, such
> +as `master~4` or `HEAD` (see linkgit:gitrevisions[7]). It can also
> +refer to tag objects, trees or blobs if the  is outside of
> +`refs/heads/*`.

I think the addition of "referring to a branch" here is an opposite
of an improvement.  The fact  can name any object (if outside
the refs/heads/) or any commit (otherwise) is stressed with the
added "It can also ...", which is great, but neither "master~4" nor
"HEAD" refer to a branch (they refer to a commit in terms relative
to a branch and a (psuedo)ref, respectively).  And a "SHA-1 expression"
that uses branch tips as a starting point (e.g. master~4 is "start
at the tip of master and go backwards by 4 steps) is *not* special
here.  You can spell your  side as "v2.17.0^0" for example, and
it does not refer to any branch.

> @@ -74,12 +76,24 @@ without any `` on the command line.  Otherwise, 
> missing
>  `:` means to update the same ref as the ``.
>  +
>  The object referenced by  is used to update the  reference
> -on the remote side.  By default this is only allowed if  is not
> -a tag (annotated or lightweight), and then only if it can fast-forward
> -.  By having the optional leading `+`, you can tell Git to update
> -the  ref even if it is not allowed by default (e.g., it is not a
> -fast-forward.)  This does *not* attempt to merge  into .  See
> -EXAMPLES below for details.
> +on the remote side. Whether this is allowed depends on what where in

s/what where/where/, I think.

> +`refs/*` the  reference lives. The `refs/heads/*` namespace will
> +only accept commit objects, and then only they can be
> +fast-forwarded. ...

Nicely clarified.  Excellent.

> + The `refs/tags/*` namespace will accept any kind of
> +object, but there commit objects are known as lightweight tags, and
> +any changes to them and others types of objects will be
> +rejected. ...

with s/, but there commit objects are known as lightweght tags/ the
sentence does not change any meaning?  An early part of the paragraph
made readers anticipate that they hear rules for what can go where,
and "refs/tags/ ref that point at a commit is called lightweight tag",
while it is not an incorrect statement per-se, does not belong to
these "rules".  Unless the discussing of the rules immediately
follows involves (or becomes easier to read if we use the term)
"lightweight tags", it probably is better to drop it.

> +... Finally and most confusingly, it's possible to push any type
> +of object to any namespace outside of `refs/{tags,heads}/*`, but these
> +will be treated as branches, even in the case where a tag object is
> +pushed.

I sense a confused writer, not a confusing behaviour being described
here.  If refs/poo/* is "treated as branches", because of what you
earlier said, you shouldn't be able to push a tag object in the
first place.

If refs/poo/* is meant to be lawless land where anything goes, then
saying "will be treated as branches" does not help readers.

> +... That tag object will be overwritten by another tag object (or
> +commit!) without `--force` if the new tag happens to point to a commit
> +that's a fast-forward of the commit it replaces.

If I pretend that I didn't see the "treated as branches", I fully
agree with the above description and refs/poo/* being a world
governed by random rules, and I do not think I'd be too opposed to
change it to "anything goes".  I do not think I'd be too opposed to
change it to "nothing is allowed unless forced", either, though.

> +By having the optional leading `+`, you can tell Git to update the
> + ref even if it is not allowed by its respective namespace
> +clobbering rules (e.g., it is not a fast-forward. in the case of
> +`refs/heads/*` updates) This does *not* attempt to merge  into
> +.  See EXAMPLES below for details.

Excellent.  

s/leading `+`/& to a refspec (or using "--force" command line option)/

Thanks.

Re: [PATCH v2] mailmap: update brian m. carlson's email address

[Kaartic Sivaraam]

On Tuesday 08 May 2018 07:28 AM, brian m. carlson wrote:
> An earlier change, cdb6b5ac (".mailmap: Combine more (name, email) to
> individual persons", 2013-08-12), noted that there were two name
> spellings and two email addresses and mapped the crustytoothpaste.net
> address to the crustytoothpaste.ath.cx address.  The latter is an older,
> obsolete address, while the former is current, so switch the order of
> the addresses so that git log displays the correct address.
> 
Just to be sure, you're meaning the use of `git log --use-mailmap` when
you mean `git log` in the log message. Am I right? Or did you mean `git
shortlog` ?

I'm asking this because I think the `git log` output doesn't consider
the mailmap file by default.

-- 
Sivaraam

QUOTE:

“The most valuable person on any team is the person who makes everyone
else on the team more valuable, not the person who knows the most.”

  - Joel Spolsky


Sivaraam?

You possibly might have noticed that my signature recently changed from
'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the
new signature to be better for several reasons one of which is that the
former signature has a lot of ambiguities in the place I live as it is a
common name (NOTE: it's not a common spelling, just a common name). So,
I switched signatures before it's too late.

That said, I won't mind you calling me 'Kaartic' if you like it [of
course ;-)]. You can always call me using either of the names.


KIND NOTE TO THE NATIVE ENGLISH SPEAKER:

As I'm not a native English speaker myself, there might be mistaeks in
my usage of English. I apologise for any mistakes that I make.

It would be "helpful" if you take the time to point out the mistakes.

It would be "super helpful" if you could provide suggestions about how
to correct those mistakes.

Thanks in advance!



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 4/8] push tests: assert re-pushing annotated tags

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> Change the test that asserts that lightweight tags can only be
> clobbered by a force-push to check do the same tests for annotated
> tags.
>
> There used to be less exhaustive tests for this with the code added in
> 40eff17999 ("push: require force for annotated tags", 2012-11-29), but
> Junio removed them in 256b9d70a4 ("push: fix "refs/tags/ hierarchy
> cannot be updated without --force"", 2013-01-16) while fixing some of
> the behavior around tag pushing.
>
> That change left us without any coverage asserting that pushing and
> clobbering annotated tags worked as intended.  There was no reason to
> suspect that the receive machinery wouldn't behave the same way with
> annotated tags, but now we know for sure.

Hmm, I am not sure if annotated tag T1 on commit C1 should be called
to "fast-forward to annotated tag T2 on commit C2" when C2 is a
descendant of C1.  Tag is meant to be a non-moving anchor point, so
it may make sense not to allow "fast-forwarding hence it is OK to
replace" that is typical for branch heads, which are meant to move
"forward".

But let's move on and keep reading, at least temporarily assuming
that "fast-forwarding" annotated tags makes sense.  Under that
assumption, this patch makes perfect sense to ensure lightweight and
annotated tags behave the same.

Thanks.

Re: [PATCH v6 10/13] help: use command-list.txt for the source of guides

[Junio C Hamano]

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

> The help command currently hard codes the list of guides and their
> summary in C. Let's move this list to command-list.txt. This lets us
> extract summary lines from Documentation/git*.txt. This also
> potentially lets us list guides in git.txt, but I'll leave that for
> now.
> ---

Nice.

Missing sign-off.

Re: [PATCH v6 08/13] git: support --list-cmds=list-

[Junio C Hamano]

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

> This allows us to select any group of commands by a category defined
> in command-list.txt. This is an internal/hidden option so we don't
> have to be picky about the category name or worried about exposing too
> much.
>
> This will be used later by git-completion.bash to retrieve certain
> command groups.
> ---

Missing sign-off.

>
> diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
> index 015eef2804..bfd8ef0671 100755
> --- a/generate-cmdlist.sh
> +++ b/generate-cmdlist.sh
> @@ -45,6 +45,21 @@ define_categories() {
>   test "$bit" -gt 32 && die "Urgh.. too many categories?"
>  }
>  
> +define_category_names() {

Style.

>  print_command_list() {

Ditto (but needs fixing in an earlier step).

> diff --git a/git.c b/git.c
> index 3c032d01fc..67f3e20ae9 100644
> --- a/git.c
> +++ b/git.c
> @@ -53,6 +53,13 @@ static int list_cmds(const char *spec)
>   list_all_main_cmds();
>   else if (len == 6 && !strncmp(spec, "others", 6))
>   list_all_other_cmds();
> + else if (len > 5 && !strncmp(spec, "list-", 5)) {

Earlier I asked to have a small helper to avoid the constant length that
has to go together with a constant string, e.g.

has_prefix(spec, len, "others")

but this new one may make it a bit tricky.

else if (has_prefix(spec, len, "others")
...
else if (has_prefix(spec, len, "list-") &&
spec[strlen("list-")] != '\0' ) {
...

does look a bit ugly.  Others may be able to help with better ideas.

Thansk.

Re: [GSoC] A blog about 'git stash' project

[Taylor Blau]

On Fri, May 04, 2018 at 12:48:21AM +0300, Paul-Sebastian Ungureanu wrote:
> Hello everybody,
>
> The community bonding period started. It is well known that for a greater
> rate of success, it is recommended to send weekly reports regarding project
> status. But, what would be a good form for such a report? I, for one, think
> that starting a blog is one of the best options because all the content will
> be stored in one place. Without further ado, I would like you to present my
> blog [1].

Hi Paul, and welcome to Git! I am looking forward to reading your
patches and any additional writing posted on your blog.

> Any feedback is greatly appreciated! Thank you!

Do you have a RSS feed that I can consume in a feed reader?

> [1]
> https://ungps.github.io/
>
> Best regards,
> Paul Ungureanu

Thanks,
Taylor

Re: [PATCH v6 05/13] git.c: convert --list-* to --list-cmds=*

[Junio C Hamano]

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

> Even if these are hidden options, let's make them a bit more generic
> since we're introducing more listing types shortly. The code is
> structured to allow combining multiple listing types together because
> we will soon add more types the 'builtins'.
>
> 'parseopt' remains separate because it has separate (SPC) to match
> git-completion.bash needs and will not combine with others.
> ---

Missing sign-off.

> +static int list_cmds(const char *spec)
> +{
> + while (*spec) {
> + const char *sep = strchrnul(spec, ',');
> + int len = sep - spec;
> +
> + if (len == 8 && !strncmp(spec, "builtins", 8))
> + list_builtins(0, '\n');

This is the origin of ugliness we see in later steps that follow the
same

if (len == strlen(constS) && !strncmp(spec, constS, strlen(constS))

pattern added here.  Could we have a small helper that takes len,
spec, and constS to abstract "8" away?

Re: [PATCH v2 02/18] Add a new builtin: branch-diff

[Jeff King]

On Mon, May 07, 2018 at 11:44:29PM -0400, Jeff King wrote:

> On Mon, May 07, 2018 at 03:24:59PM -0700, Stefan Beller wrote:
> 
> > Hence I propose "git range-diff", similar to topic-diff, that
> > was proposed earlier.
> > 
> > * it "diffs ranges" of commits.
> > * it can also deal with out-of-git things like patch series,
> >   but that is a mere by product and may not be desired.
> >   Just like git-diff can also compare two files outside a git
> >   repo, that would not be a good use case.
> >   Keep the name Git-centric!
> > * it autocompletes well.
> 
> FWIW, I like this by far of all of the suggested names.

I hit "send" before I had a chance to expound. ;)

The thing that I really like about it is that it names the _concept_.
If I were writing a manual page describing what this output is, I would
call it a "range diff". And naturally, the command to generate range
diffs is "git range-diff".

I think "git diff --range" would also be OK, but IMHO it's useful to
keep the "git diff" family as always comparing end-points.

-Peff

Re: [PATCH v6 02/13] generate-cmds.sh: export all commands to command-list.h

[Junio C Hamano]

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

> The current generate-cmds.sh generates just enough to print "git help"
> output. That is, it only extracts help text for common commands.
>
> The script is now updated to extract help text for all commands and
> keep command classification a new file, command-list.h. This will be
> useful later:
>
> - "git help -a" could print a short summary of all commands instead of
>   just the common ones.
>
> - "git" could produce a list of commands of one or more category. One
>   of its use is to reduce another command classification embedded in
>   git-completion.bash.
>
> The new file can be generated but is not used anywhere yet. The plan
> is we migrate away from common-cmds.h. Then we can kill off
> common-cmds.h build rules and generation code (and also delete
> duplicate content in command-list.h which we keep for now to not mess
> generate-cmds.sh up too much).
>
> PS. The new fixed column requirement on command-list.txt is
> technically not needed. But it helps simplify the code a bit at this
> stage. We could lift this restriction later if we want to.
> ---

Missing sign-off.

> diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
> index 31b6d886cb..c9fd524760 100755
> --- a/generate-cmdlist.sh
> +++ b/generate-cmdlist.sh
> @@ -1,5 +1,27 @@
>  #!/bin/sh
>  
> +die () {
> + echo "$@" >&2
> + exit 1
> +}
> +
> +command_list () {
> + sed '1,/^### command list/d;/^#/d' "$1"
> +}
> +
> +get_categories() {

Here and define_categories() below lack SP before (), and all others
do.  Be consistent.

> + tr ' ' '\n'|
> + grep -v '^$' |
> + sort |
> + uniq
> +}
> +
> +category_list () {
> + command_list "$1" |
> + cut -c 40- |
> + get_categories
> +}
> +
>  get_synopsis () {
>   sed -n '
>   /^NAME/,/'"$1"'/H
> @@ -10,14 +32,51 @@ get_synopsis () {
>   }' "Documentation/$1.txt"
>  }
>  
> +define_categories() {
> + echo
> + echo "/* Command categories */"
> +...
>  };
> +"
> +if [ -z "$2" ]

if test -z "$2"

Re: [PATCH v2 02/18] Add a new builtin: branch-diff

[Jeff King]

On Mon, May 07, 2018 at 03:24:59PM -0700, Stefan Beller wrote:

> Hence I propose "git range-diff", similar to topic-diff, that
> was proposed earlier.
> 
> * it "diffs ranges" of commits.
> * it can also deal with out-of-git things like patch series,
>   but that is a mere by product and may not be desired.
>   Just like git-diff can also compare two files outside a git
>   repo, that would not be a good use case.
>   Keep the name Git-centric!
> * it autocompletes well.

FWIW, I like this by far of all of the suggested names.

-Peff

Re: [PATCH v2] completion: reduce overhead of clearing cached --options

[Junio C Hamano]

Todd Zullinger  writes:

> Hi Matthew,
>
> Matthew Coleman wrote:
>> I haven't seen any discussion about this recently. What
>> are the chances of getting it merged? I'd like to see this
>> included in 2.18.
>
> Junio's last few "What's cooking" updates have mentioned it:
>
>> * sg/completion-clear-cached (2018-04-18) 1 commit
>>   (merged to 'next' on 2018-04-25 at 9178da6c3d)
>>  + completion: reduce overhead of clearing cached --options
>> 
>>  The completion script (in contrib/) learned to clear cached list of
>>  command line options upon dot-sourcing it again in a more efficient
>>  way.
>> 
>>  Will merge to 'master'.
>
> I imagine it will show up on master sometime soon, in time
> for 2.18.0.

Yup, I do not foresee at this moment why that shouldn't be the case.

Thanks.

Re: main url for linking to git source?

[Junio C Hamano]

Konstantin Ryabitsev  writes:

> On Tue, May 08, 2018 at 01:51:30AM +, brian m. carlson wrote:
>>
>> I think I would also prefer a list of available repositories over a
>> hard-coded choice.  It may be that some places (say, Australia) have
>> better bandwidth to one over the other, and users will be able to have a
>> better experience with certain mirrors.
>> 
>> While I'm sympathetic to the idea of referring to kernel.org because
>> it's open-source and non-profit, users outside of North America are
>> likely to have a less stellar experience with its mirrors, since they're
>> all in North America.
>
> I'm a bit worried that I'll come across as some kind of annoying pedant,
> but git.kernel.org is actually 6 different systems available in US,
> Europe, Hong Kong, and Australia. :)
>
> We use geodns to map users to the nearest server (I know, GeoDNS is not
> the best, but it's what we have for free).

Thanks for an update---it is nice to make those who are discussing
this topic to be aware of these facts.  Very much appreciated.

Re: [PATCH v2] mailmap: update brian m. carlson's email address

[Junio C Hamano]

"brian m. carlson"  writes:

> An earlier change, cdb6b5ac (".mailmap: Combine more (name, email) to
> individual persons", 2013-08-12), noted that there were two name
> spellings and two email addresses and mapped the crustytoothpaste.net
> address to the crustytoothpaste.ath.cx address.  The latter is an older,
> obsolete address, while the former is current, so switch the order of
> the addresses so that git log displays the correct address.
>
> Signed-off-by: brian m. carlson 
> ---
> I intentionally avoided the use of the first person here, because I
> wasn't sure what the preference of the list was on that.  Hopefully it
> reads naturally and isn't awkward.
>
> If not, I can send a v3.

Nah, the updated text is perfect.  Thanks, will replace.

>
>  .mailmap | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/.mailmap b/.mailmap
> index 7c71e88ea5..df7cf6313c 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -25,8 +25,8 @@ Ben Walton  
>  Benoit Sigoure  
>  Bernt Hansen  
>  Brandon Casey  
> -brian m. carlson  Brian M. Carlson 
> 
> -brian m. carlson  
> 
> +brian m. carlson  Brian M. Carlson 
> 
> +brian m. carlson  
> 
>  Bryan Larsen  
>  Bryan Larsen  
>  Cheng Renquan 

Re: [PATCH 3/8] push tests: add more testing for forced tag pushing

[Junio C Hamano]

Junio C Hamano  writes:

> I couldn't quite get what you meant by "(but not the other way
> around)".  Did you mean
>
>   $ git push --force ../child2 refs/tags/*:refs/tags/*
>
> should not become non-forcing version because of the (lack of)
> prefix on the refspec does not trump the --force command line
> option?  If so, making
>
>   $ git push --no-force ../child2 +refs/tags/*:refs/tags/*
>
> not to force would make things more consistent, I suspect, i.e. we
> can simply declare that presence or absense of '+' prefix in the
> refspec determines the forced-ness of the push/fetch when there is
> no command line option to decide it, but an explicit command line
> option will always override it.  
>
> Am I missing something obvious?

And of course I am missing the fact that --force and --no-force
controls a single boolean.  If it controled a tristate (unspecified,
false, true), then what I wrote above makes tons of sense, but that
is not the reality.  "git push --no-force" is saying the the same as
"git push", and its primarily reason for being there is to countermand
a "--force" that appears earlier on the command line for whatever
reason, e.g.

$ alias push='git push --force'
$ push --no-force ../child2 ...

So what you said in this patch 100%  makes sense.

Re: main url for linking to git source?

[Konstantin Ryabitsev]

On Tue, May 08, 2018 at 01:51:30AM +, brian m. carlson wrote:
> On Mon, May 07, 2018 at 11:15:46AM -0700, Stefan Beller wrote:
> > There I would try to mirror Junios list of "public repositories"
> > https://git-blame.blogspot.com/p/git-public-repositories.html
> > without officially endorsing one over another.
> 
> I think I would also prefer a list of available repositories over a
> hard-coded choice.  It may be that some places (say, Australia) have
> better bandwidth to one over the other, and users will be able to have a
> better experience with certain mirrors.
> 
> While I'm sympathetic to the idea of referring to kernel.org because
> it's open-source and non-profit, users outside of North America are
> likely to have a less stellar experience with its mirrors, since they're
> all in North America.

I'm a bit worried that I'll come across as some kind of annoying pedant,
but git.kernel.org is actually 6 different systems available in US,
Europe, Hong Kong, and Australia. :)

We use geodns to map users to the nearest server (I know, GeoDNS is not
the best, but it's what we have for free).

-K



signature.asc
Description: PGP signature

Re: [PATCH 3/8] push tests: add more testing for forced tag pushing

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> Improve the tests added in dbfeddb12e ("push: require force for refs
> under refs/tags/", 2012-11-29) to assert that the same behavior
> applies various forms other refspecs, and that "+" in a refspec will
> override the "--no-force" option (but not the other way around).

For some reason this fell out of my radar; sorry about that.

I like the general idea to ensure non-ff pushes are rejected, unless
forced, to update a light-weight tag with another.  I am unsure what
should happen when trying to update a light-weight tag with an
object with different type (or vice versa), and haven't read in this
series what your opinion is yet.  Let's read on and see how it goes.

I have a moderately strong preference that

$ git push --no-force child2

with a configured refspec

[remote "child2"]
url = ../child2
push = +refs/tags/*:refs/tags/*

should behave as a non-forced push (regardless of the refs hierarchy
involved, not limited to tags/).  I have a mild preference against

$ git push --no-force ../child2 +refs/tags/*:refs/tags/*

that forces, just because command line options look a lot more
explicit than the prefix '+', and choosing it not to force would
make it consistent with the desired behaviour for configured forcing
refspec.

I couldn't quite get what you meant by "(but not the other way
around)".  Did you mean

$ git push --force ../child2 refs/tags/*:refs/tags/*

should not become non-forcing version because of the (lack of)
prefix on the refspec does not trump the --force command line
option?  If so, making

$ git push --no-force ../child2 +refs/tags/*:refs/tags/*

not to force would make things more consistent, I suspect, i.e. we
can simply declare that presence or absense of '+' prefix in the
refspec determines the forced-ness of the push/fetch when there is
no command line option to decide it, but an explicit command line
option will always override it.  

Am I missing something obvious?

> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
>  t/t5516-fetch-push.sh | 12 +++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 15c8d5a734..c9a2011915 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -981,7 +981,17 @@ test_expect_success 'push requires --force to update 
> lightweight tag' '
>   git push --force ../child2 Tag &&
>   git tag -f Tag HEAD~ &&
>   test_must_fail git push ../child2 Tag &&
> - git push --force ../child2 Tag
> + git push --force ../child2 Tag &&
> + git tag -f Tag &&
> + test_must_fail git push ../child2 "refs/tags/*:refs/tags/*" &&
> + git push --force ../child2 "refs/tags/*:refs/tags/*" &&
> + git tag -f Tag HEAD~ &&
> + git push ../child2 "+refs/tags/*:refs/tags/*" &&
> + git tag -f Tag &&
> + git push --no-force ../child2 "+refs/tags/*:refs/tags/*" &&
> + git tag -f Tag HEAD~ &&
> + test_must_fail git push ../child2 tag Tag &&
> + git push --force ../child2 tag Tag
>   )
>  '

Re: [GSoC] Yet another blog series about the GSoC

[Pratik Karki]

On Tue, May 8, 2018 at 12:20 AM, Stefan Beller  wrote:
> Hi Pratik,
Hi Stefan,

> On Sat, May 5, 2018 at 5:24 AM, Pratik Karki  wrote:
>> On Sat, May 5, 2018 at 5:11 PM, Alban Gruin  wrote:
>>> Hi everybody,
>>>
>>> as my fellow students, I started a blog series about my GSoC project[1].
>>> First, I wanted to post them directly on the mailing list, but a blog
>>> allows me to edit the content easily if needed.
>>>
>>> Any feedback is welcome!
>>>
>>> [1] https://blog.pa1ch.fr/posts/2018/05/05/en/gsoc2018-week-1.html
>>>
>>> Cheers,
>>> Alban Gruin
>
> Thanks for blogging about GSoC!
>
>> Nice post. Great job, Alban.
>> Just a little typo I found out there: hazardous -> hasardous.
>
> I would think hazardous is correct, both in British as well as
> American English, I cannot speak for more.

I believe I kept the notation wrong in the mail I sent earlier.
Actually, Alban had hasardous typo. I corrected him.
I guess the notation should have been: hasardous -> hazardous.
Sorry. :-)

Cheers,
Pratik Karki

Re: [PATCH v2] completion: reduce overhead of clearing cached --options

[Todd Zullinger]

Hi Matthew,

Matthew Coleman wrote:
> I haven't seen any discussion about this recently. What
> are the chances of getting it merged? I'd like to see this
> included in 2.18.

Junio's last few "What's cooking" updates have mentioned it:

> * sg/completion-clear-cached (2018-04-18) 1 commit
>   (merged to 'next' on 2018-04-25 at 9178da6c3d)
>  + completion: reduce overhead of clearing cached --options
> 
>  The completion script (in contrib/) learned to clear cached list of
>  command line options upon dot-sourcing it again in a more efficient
>  way.
> 
>  Will merge to 'master'.

I imagine it will show up on master sometime soon, in time
for 2.18.0.

-- 
Todd
~~
I expected times like this -- but never thought they'd be so bad, so
long, and so frequent.
-- Demotivators (www.despair.com)

Re: [PATCH v2 12/18] branch-diff: use color for the commit pairs

[Todd Zullinger]

Hi Johannes,

Johannes Schindelin wrote:
> Hi Todd,
> 
> On Sat, 5 May 2018, Todd Zullinger wrote:
> 
>>> @@ -430,6 +451,8 @@ int cmd_branch_diff(int argc, const char **argv, const 
>>> char *prefix)
>>> struct string_list branch1 = STRING_LIST_INIT_DUP;
>>> struct string_list branch2 = STRING_LIST_INIT_DUP;
>>>  
>>> +   git_diff_basic_config("diff.color.frag", "magenta", NULL);
>>> +
>>> diff_setup();
>>> diffopt.output_format = DIFF_FORMAT_PATCH;
>>> diffopt.flags.suppress_diff_headers = 1;
>> 
>> Should this also (or only) check color.diff.frag?
> 
> This code is not querying diff.color.frag, it is setting it. Without
> any way to override it.
> 
> Having thought about it longer, and triggered by Peff's suggestion to
> decouple the "reverse" part from the actual color, I fixed this by
> 
> - *not* setting .frag to magenta,
> 
> - using the reverse method also to mark outer *hunk headers* (not only the
>   outer -/+ markers).
> 
> - actually calling git_diff_ui_config()...

Excellent.  That seems to work nicely now, respecting the
color.diff. config.

> The current work in progress can be pulled as `branch-diff` from
> https://github.com/dscho/git, if I could ask you to test?

While the colors and 'branch --diff' usage seem to work
nicely, I found that with 4ac3413cc8 ("branch-diff: left-pad
patch numbers", 2018-05-05), 'git branch' itself is broken.

Running 'git branch' creates a branch named 'branch'.
Calling 'git branch --list' shows only 'branch' as the only
branch.

I didn't look too closely, but I'm guessing that the argv
handling is leaving the 'branch' argument in place where it
should be stripped?

This unsurprisingly breaks a large number of tests. :)

Thanks,

-- 
Todd
~~
A common mistake people make when trying to design something
completely foolproof is to underestimate the ingenuity of complete
fools.
-- Douglas Adams

[PATCH v2] mailmap: update brian m. carlson's email address

[brian m. carlson]

An earlier change, cdb6b5ac (".mailmap: Combine more (name, email) to
individual persons", 2013-08-12), noted that there were two name
spellings and two email addresses and mapped the crustytoothpaste.net
address to the crustytoothpaste.ath.cx address.  The latter is an older,
obsolete address, while the former is current, so switch the order of
the addresses so that git log displays the correct address.

Signed-off-by: brian m. carlson 
---
I intentionally avoided the use of the first person here, because I
wasn't sure what the preference of the list was on that.  Hopefully it
reads naturally and isn't awkward.

If not, I can send a v3.

 .mailmap | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/.mailmap b/.mailmap
index 7c71e88ea5..df7cf6313c 100644
--- a/.mailmap
+++ b/.mailmap
@@ -25,8 +25,8 @@ Ben Walton  
 Benoit Sigoure  
 Bernt Hansen  
 Brandon Casey  
-brian m. carlson  Brian M. Carlson 

-brian m. carlson  

+brian m. carlson  Brian M. Carlson 

+brian m. carlson  

 Bryan Larsen  
 Bryan Larsen  
 Cheng Renquan 

Re: main url for linking to git source?

[brian m. carlson]

On Mon, May 07, 2018 at 11:15:46AM -0700, Stefan Beller wrote:
> There I would try to mirror Junios list of "public repositories"
> https://git-blame.blogspot.com/p/git-public-repositories.html
> without officially endorsing one over another.

I think I would also prefer a list of available repositories over a
hard-coded choice.  It may be that some places (say, Australia) have
better bandwidth to one over the other, and users will be able to have a
better experience with certain mirrors.

While I'm sympathetic to the idea of referring to kernel.org because
it's open-source and non-profit, users outside of North America are
likely to have a less stellar experience with its mirrors, since they're
all in North America.

I would suggest that whatever option we choose, we only specify HTTPS or
SSH (i.e., encrypted) protocols.  Encryption is cheap, and we have lots
of options meeting that criterion.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH 2/2] Documentation: render revisions correctly under Asciidoctor

[brian m. carlson]

On Mon, May 07, 2018 at 06:11:43AM +0200, Martin Ågren wrote:
> On 6 May 2018 at 22:42, brian m. carlson  wrote:
> > When creating a literal block from an indented block without any sort of
> > delimiters, Asciidoctor strips off all leading whitespace, resulting in
> > a misrendered chart.  Use an explicit literal block to indicate to
> > Asciidoctor that we want to keep the leading whitespace.
> 
> Excellent. These two patches fix my original problem and seem like the
> obviously correct approach (in hindsight ;-) ). I wonder if the diagrams
> earlier in the file should be tackled also. Right now, one has a huge
> number of dots instead of the four you added; the other has none. They
> do render fine though, so that'd only be about consistency in the
> original .txt-file.

Yeah, the number of dots has to be at least four, but can be any
matching number.

Since this patch fixes the present issue, I'd like to leave it as it is
and run through a cleanup series a bit later that catches all the
literal indented blocks and marks them explicitly to avoid this issue in
the future.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH] alloc.c: replace alloc by mempool

[Junio C Hamano]

Stefan Beller  writes:

> The replacement with mem-pool might be easier than making sure
> that alloc.c has no globals and handles allocations per repository
> correctly. It would make the sb/object-store-alloc series shorter than
> it currently is, and maybe easier to review the code.
>
> However now that sb/object-store-alloc is rerolled with keeping
> the logic of alloc.c and not replacing it with mem-pool, the only
> reason would be ease of maintainability by less code

That is sensible considertation; the patch should be sold as such,
instead of with an empty log message as if the reasoning is obvious
to everybody without being told ;-)

Re: What's cooking in git.git (May 2018, #01; Mon, 7)

[Junio C Hamano]

Elijah Newren  writes:

>>  Rename detection logic in "diff" family that is used in "merge" has
>>  learned to guess when all of x/a, x/b and x/c have moved to z/a,
>>  z/b and z/c, it is likely that x/d added in the meantime would also
>>  want to move to z/d by taking the hint that the entire directory
>>  'x' moved to 'z'.  Incidentally, this avoids updating a file in the
>>  working tree after a (non-trivial) merge whose result matches what
>>  our side originally had.
>
> Thanks for adding a comment about the fix for the unnecessary update.
> However, you've dropped a comment from the original release note about
> the other fix this series also provides, namely, "A bug causing dirty
> files involved in a rename to be overwritten during merge has also
> been fixed as part of this work."

Thanks again.

Re: What's cooking in git.git (May 2018, #01; Mon, 7)

[Junio C Hamano]

Elijah Newren  writes:

> I'm actually curious what the "Will merge to 'master'" and "Will merge
> to 'next'" are intended to mean in general.  I thought it meant that
> the topic would be merged the following week barring further
> discussion, but that hasn't always been the case ...

These have always been "eventually in some future", not "next
week".

A topic with potentially high impact tends to cook in 'next' longer
than a trivial typo fix, so that it has better chance of getting
stopped by the community dogfooding of 'next' before getting merged
to 'master' and bite the general public.

And the commit-graph thing certainly is a bigger beast I'd feel
safer if people tested heavily while it is in 'next' before it hits
'master'.

Re: [PATCH] alloc.c: replace alloc by mempool

[Stefan Beller]

On Mon, May 7, 2018 at 5:37 PM, Junio C Hamano  wrote:
> Duy Nguyen  writes:
>
>>> So I think we could just replace it for now and optimize again later, if it
>>> turns out to be a problem. I think the easiest optimisation is to increase
>>> the allocation size of having a lot more objects per mp_block.
>>
>> Yeah. I also tested this from a different angle: memory overhead. For
>> 2M objects with one mp_block containing 1024 objects (same setting as
>> alloc.c), the overhead (not counting malloc() internal overhead) is
>> 46KB and we don't have any extra overhead due to padding between
>> objects. This is true for all struct blob, commit, tree and tag. This
>> is really good. alloc.c has zero overhead when measured this way but
>> 46KB is practically zero to me.
>
> Thanks.
>
> The above in short sounds like arguing "replacing alloc.c internal
> with mempool incurs negligible memory overhead and performance
> degradation, but that can be optimized later".  It was unclear to me
> why such a replacement needs to happen in the first place, though.

The replacement with mem-pool might be easier than making sure
that alloc.c has no globals and handles allocations per repository
correctly. It would make the sb/object-store-alloc series shorter than
it currently is, and maybe easier to review the code.

However now that sb/object-store-alloc is rerolled with keeping
the logic of alloc.c and not replacing it with mem-pool, the only
reason would be ease of maintainability by less code
On the other hand we have different implementations of
lists, vectors and hashmaps, so having different memory
allocators doesn't hurt.

> Without such a code churn, there won't be extra overhead or need to
> fix it up later, no?

The original motivation is to get the object store on a per-repository
basis. It might be cheaper to use an existing 'objectified' code instead
of doing that again.

Re: [PATCH] alloc.c: replace alloc by mempool

[Junio C Hamano]

Duy Nguyen  writes:

>> So I think we could just replace it for now and optimize again later, if it
>> turns out to be a problem. I think the easiest optimisation is to increase
>> the allocation size of having a lot more objects per mp_block.
>
> Yeah. I also tested this from a different angle: memory overhead. For
> 2M objects with one mp_block containing 1024 objects (same setting as
> alloc.c), the overhead (not counting malloc() internal overhead) is
> 46KB and we don't have any extra overhead due to padding between
> objects. This is true for all struct blob, commit, tree and tag. This
> is really good. alloc.c has zero overhead when measured this way but
> 46KB is practically zero to me.

Thanks.

The above in short sounds like arguing "replacing alloc.c internal
with mempool incurs negligible memory overhead and performance
degradation, but that can be optimized later".  It was unclear to me
why such a replacement needs to happen in the first place, though.
Without such a code churn, there won't be extra overhead or need to
fix it up later, no?

Re: [PATCH v2 02/18] Add a new builtin: branch-diff

[Junio C Hamano]

Jeff King  writes:

> One of the things I don't like about "git branch --diff" is that this
> feature is not _just_ about branches at all.

I actually wouldn't be that much against the word "branch" in
"branch-diff" on the ground that we are typically not feeding
branches to the command (we are feeding two ranges, and one endpoint
of each range typically gets expressed using branch name), as we
have a precedent in "show-branch", for example, that often takes
branches but does not have to.

> It's really "diff-history" or something, I think. That's not very
> catchy, but I think the best name would imply that it was diffing a set
> of commits (so even "diff-commit" would not be right, because that again
> sounds like endpoints).

Sure.  This should't be a submode "--diff" of "git branch" just like
it shouldn't be a submode of "git commit" only because it is about
comparing two sets of commits.  "diff" is about comparing two
endpoints, and not about comparing two sets.  "log" is the closest
thing, if we really want to coerce it into an existing set of
commands, as it is about a set of commits, but it does not do
multiple sets, let alone comparing them.

"branch-diff" was just a good as "diff-history", except that both of
them may irritate command line completion users.  I do not think I
care too much about which exact command name it gets, but I think it
is a bad idea to tacked it to an existing command as a submode that
does unrelated thing to what the main command does.  So from that
point of view, "branch-diff" and "diff-history" are equally good
being a distinct command, and equally bad sharing prefix with common
existing command.

Re: [PATCH 2/2] builtin/grep.c: teach '-o', '--only-matching' to 'git-grep'

[Taylor Blau]

On Sat, May 05, 2018 at 03:36:12AM -0400, Eric Sunshine wrote:
> On Sat, May 5, 2018 at 12:03 AM, Taylor Blau  wrote:
> > Teach GNU grep(1)'s '-o' ('--only-matching') to 'git-grep'. This option
> > prints only the matching components of each line. It writes multiple
> > lines if more than one match exists on a given line.
> >
> > For example:
> >
> >   $ git grep -on --column --heading git -- README.md | head -3
> >   README.md
> >   15:56:git
> >   18:20:git
> >
> > By using show_line_header(), 'git grep --only-matching' correctly
> > respects the '--header' option:
>
> What is the '--header' option? I don't see it used in any example.

I think '--header' is a typo for '--heading', which is used in the
following example.

> >   $ git grep -on --column --heading git -- README.md | head -4
> >   README.md
> >   15:56:git
> >   18:20:git
> >   19:16:git
>
> How does this example differ from the earlier example (other than
> showing 4 lines of output rather than 3)?

Ack. I clipped from my terminal what I meant to be the seocnd
example, and pasted it in for both examples. They are meant to be as
follows:

  1. 'git grep' without heading, showing the full line prefix, and
  2. 'git grep' with heading, showing the file heading with '--heading'.

The later has '| head -n4' on the end to include 3+1 lines (3 matches, 1
heading) whereas the former has '| head -n3' to include 3 lines (3
matches, no heading).

I have updated my patch locally to reflect this.


Thanks,
Taylor

Re: [PATCH 1/2] grep.c: extract show_line_header()

[Taylor Blau]

On Sat, May 05, 2018 at 03:30:51AM -0400, Eric Sunshine wrote:
> On Sat, May 5, 2018 at 12:03 AM, Taylor Blau  wrote:
> > Teach 'git-grep(1)' how to print a line header multiple times from
> > show_line() in preparation for it learning '--only-matching'.
> >
> > show_line_header() comprises of the code in show_line() executed in
>
> s/of//

Ack -- thanks for pointing that out :-).

> > order to produce a line header. It is a one-for-one extraction of the
> > existing implementation.
> >
> > For now, show_line_header() provides no benefit over the change before
> > this patch. The following patch will call conditionally call
>
> s/call conditionally call/conditionally call/

Double ack. Thanks again :-).


Thanks,
Taylor

Re: [PATCH v4 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'

[Taylor Blau]

On Mon, May 07, 2018 at 11:13:12PM +0900, Junio C Hamano wrote:
> Taylor Blau  writes:
>
> > diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
> > index 18b494731f..5409a24399 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]
> >[-l | --files-with-matches] [-L | --files-without-match]
> >[(-O | --open-files-in-pager) []]
> >[-z | --null]
> > @@ -169,6 +169,9 @@ providing this option will cause it to die.
> >  --line-number::
> > Prefix the line number to matching lines.
> >
> > +--column::
> > +   Prefix the 1-indexed column number of the first match on non-context 
> > lines.
> > +
>
> Two questions.
>
>  - It is fine that the leftmost column is 1, but what does this
>number count?  The number of bytes on the same line before the
>first byte of the hit (plus 1)?  The display width of the initial
>non-matching part of the line (plus 1) on a fixed-width terminal?
>The number of "characters"?  Something else?

The count is the byte offset from the 1-index (which is the beginning of
the line, as you noted).

Incidentally, Peff and I chatted briefly offline about this, and agree
that it makes the most sense, since (1) Vim treats it correctly, and (2)
we can't be sure of options like display width, character count, etc.,
without knowing the character encoding.

Nonetheless, other folks in this thread seem to be curious about this
as well. I'll add it to the documentation for --column in
Documentation/git-grep.txt.

>  - Does --column combined with -v make any sense?  If not, shouldn't
>the command error out when both are given at the same time?

I hadn't thought of this. They do not work together, since 'git grep -v
--column' would require us to either (1) not output the column number,
or (2) output '0', or some other non-value.

I think that both (1) and (2) require callers to complicate their
scripts to understand either approach. As such, I think that we should
die() here, and add a test in t7810 to ensure that that's indeed what
happens.

Does this seem sensible to include in v5?


Thanks,
Taylor

Re: [PATCH] mailmap: update brian m. carlson's email address

[brian m. carlson]

On Mon, May 07, 2018 at 12:25:09PM -0700, Stefan Beller wrote:
> brian,
> 
> sorry to break you there. I was the author of the patch Junio found, 
> organizing
> the .mailmap file was one of my starter contributions. I recall asking all the
> people if they were ok with it their names combined in different spellings
> 94b410bba86 (.mailmap: Map email addresses to names, 2013-07-12)
> f4f49e2258a (.mailmap: Combine more (email, name) to individual
> persons, 2013-07-14)
> and I vaguley recall asking you about capitalization of your name there
> and you preferred the lower case name, but apparently I never asked you
> about the preferred email address.

Not a problem.  I hadn't until very recently completely understood the
format of the .mailmap file (in that the correct items are always to be
on the left), so I didn't think anything of it when reviewing the
commit.  Perhaps I should come up with a patch to the documentation to
make it easier for past me to understand.

I only recently realized that with my transition to a separate mail
server (in September), the crustytoothpaste.ath.cx address doesn't
actually work, and when perusing the output of git log, realized that it
was the wrong way around.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH] mailmap: update brian m. carlson's email address

[brian m. carlson]

On Mon, May 07, 2018 at 12:37:05PM +0900, Junio C Hamano wrote:
> I initially reacted to "was reversed" with "yikes, did we break the
> mailmap reader and we need to update the file?", but apparently that
> is not what this patch is about.  I think what is happening here is
> that cdb6b5ac (".mailmap: Combine more (name, email) to individual
> persons", 2013-08-12) removed
> 
> -Brian M. Carlson 
> 
> and then added these two lines
> 
> +brian m. carlson  Brian M. Carlson 
> 
> +brian m. carlson  
> 
> 
> where *.net address did not come from any other entry for you in the
> file.  I guess the author of the patch saw that you were sending
> your messages from the .net address and tried to help by unifying
> the two addresses, without knowing your preference and recorded two
> reversed entries.
> 
> Will queue as-is for now, but if you want to update the log message
> I do not mind taking a reroll.

I can reroll with a less alarming commit message, sure.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH 06/28] t0000: annotate with SHA1 prerequisite

[brian m. carlson]

On Mon, May 07, 2018 at 12:24:13PM +0200, Martin Ågren wrote:
> This could be more centralized at the top of the file, more likely to be
> noticed during a `make test` and easier to adapt in the future. Maybe
> something like this at the top of the file:
> 
> if hash for storage is SHA-1 then
> knowntree=7bb943559a305bdd6bdee2cef6e5df2413c3d30a
> path0=f87290f8eb2cbbea7857214459a0739927eab154
> 
> else
> skip_all='unknown hash, but you really need to expand this test'
> # or even error out!
> fi

As I mentioned in an earlier email, I plan to set an environment
variable for the algorithms in use and then do something like:

  test "$tree" = "$(test-tool hash-helper --output known-tree)"

where "known-tree" is some key we can use to look up the SHA-1 or
NewHash value, and we've specified we want the output format (as opposed
to input format or on-disk format).

> When we add NewHash, we get to add an "else if". Otherwise, we'd be
> plugging in NewHash without testing some very basic stuff.
> 
> Ok, so `skip_all` is quite hard, but skipping certain basic tests also
> feels really shaky. Adding a new hash algo without adapting this test
> should be a no-go, IMHO.

I agree that this test needs to be updated for NewHash, but since we
haven't decided what that's going to be, it makes sense during
development to still test the rest of the code if possible so that we
know what does and doesn't work.

This is a first step in making it obvious what doesn't work, and when we
know what the data is supposed to be, we can adjust it by fixing the
tests so that it works in all cases.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH v4 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'

[Taylor Blau]

On Sun, May 06, 2018 at 07:56:42PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Sat, May 05 2018, Taylor Blau wrote:
>
>
> > +   test_expect_success "grep -w $L (with --{line,column}-number)" '
>
> It's now --column in v4 but this still refers to v3 --column-number.

Thanks, I certainly missed this one.


Thanks,
Taylor

Re: [PATCH v2 02/18] Add a new builtin: branch-diff

[Igor Djordjevic]

Hi Stefan,

On 08/05/2018 00:24, Stefan Beller wrote:
> 
> > List, rename, delete -- all these seem more as basic CRUD operations,
> > where comparison is a more complex one. And not to get me wrong - I
> > could see "branch diff" being part of "branch", but not really when
> > "diff" already exists as a separate thing, already doing quite some
> > (but still diff related, and configurable) stuff.
> 
> If we go with "branch --diff", because it has the CRUD operations already
> there for branches, I might ask for "remote --diff" to diff two remotes. ;)
> (That command "remote --diff" would not make any sense, would it?)

I`m not sure if this is a reply to me or in general, and whether you 
support what I sad, or argue against it...? Because what you`re 
saying was (or at least should have been) my exact point there :)

> > Basically, what you (conceptually) call "two versions of the same
> > branch", I simply call "two branches" (from usage standpoint).
> 
> If I diff 2 (topic) branches, which are based on a different version
> from upstream, then I see changes from commits that I don't care
> about, but this tool explicitly excludes them. Instead it includes
> the ordering of the commits as well as its commit messages to
> the diff.

Here, I was merely pointing out that you still need to provide two 
branch heads - which might be expected to resemble "two versions of 
the same topic", but they are still (just) "two branches" in Git world.

> > And you may have a branch that got split, or more of them that got
> > unified, so defining "previous branch version" may not be that
> > straightforward - it`s really just "two commit ranges" (as man page
> > defines it in general), with "two versions of a patch series" only
> > being the most common/expected use case of the former.
> >
> > Finally, if user picks two totally unrelated "branches" to compare,
> > he won`t get a really useful diff - but it`s the same as if he would
> > compare two totally unrelated commits (where tree state massively
> > changed in between, or having unrelated histories, even).
> 
> I used just that, but narrowed down the comparison to one file
> instead of the whole tree.

Again, not sure if this should support the argument, or argue against 
it? :) My point was that there might be other use cases (as you seem 
to have supported now), and as "diff" is pretty forgiving, might be 
"diff branch" should be as well.

> > With something like `git diff --branch ...` you
> > would get yet another "diff look", useful for use case in question
> > here.
> 
> Personally I think this patch series should neither extend git-diff
> nor git-branch.
> 
> It should not extend git-diff, because currently git-diff can diff
> tree-ishs (and does that very well) and comparing to
> worktree/index.

Hmm, are you saying that `git diff` actually has a too generic name 
for its (more specific) purpose?

> It should also not extend git-branch, as that command is for
> CRUD operations that you hinted at earlier (Earlier I proposed
> git-remote --diff for diffing two remote, which makes no sense,
> another one might be git-worktree, which also just does CRUD
> for worktrees. It would be a bad idea to have "git worktree --diff")

Agreed here.

> Hence I propose "git range-diff", similar to topic-diff, that
> was proposed earlier.

I find it strange that we already have both "diff" and "diff-something" 
commands, and yet you still propose "something-diff" naming pattern 
instead (but I guess it`s mainly because of auto-complete concerns).

Please forgive my lack of code base familiarity, but from what I`ve 
seen so far, and at least from end-user perspective, I may rather expect 
`git diff-range` as low level implementation, and possibly exposed 
through `git diff --range` (with a nice single letter abbreviation?).

> * it "diffs ranges" of commits.

Thus "diff-range", as your description says itself :) ("range-diff" 
might sound like it "ranges diffs"...?)

> * it can also deal with out-of-git things like patch series,
>   but that is a mere by product and may not be desired.
>   Just like git-diff can also compare two files outside a git
>   repo, that would not be a good use case.

Hmm, so still follows `git diff` in general... `git diff --range`? :D

> * it autocompletes well.

Only here I`m not sure if something like `git diff --range` (with 
accompanying single letter option) would be considered "auto-complete 
friendly", or not?

Regards, Buga

Re: [PATCH v4 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'

[Taylor Blau]

On Sat, May 05, 2018 at 08:15:03AM +0200, Duy Nguyen wrote:
> On Sat, May 5, 2018 at 4:43 AM, Taylor Blau  wrote:
> > Teach 'git-grep(1)' a new option, '--column', to show the column
> > number of the first match on a non-context line.
>
> Why? Or put it another way, what is this option used for? Only
> git-jump? (which should also be mentioned here if true)

Good question. My primary intention is giving
'contrib/git-jump/git-jump' the information it needs in order to tell
$EDITOR how to seek to the relevant position within a line.

I have amended this patch to include the relevant bits, and will attach
in v5.


Thanks,
Taylor

Re: [PATCH v4 7/7] contrib/git-jump/git-jump: jump to match column in addition to line

[Taylor Blau]

On Sun, May 06, 2018 at 08:03:01PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Sun, May 06 2018, Martin Ågren wrote:
>
> > On 5 May 2018 at 04:43, Taylor Blau  wrote:
> >> Take advantage of 'git-grep(1)''s new option, '--column' in order to
> >> teach Peff's 'git-jump' script how to jump to the correct column for any
> >> given match.
> >>
> >> 'git-grep(1)''s output is in the correct format for Vim's jump list, so
> >> no additional cleanup is necessary.
> >
> >> diff --git a/contrib/git-jump/README b/contrib/git-jump/README
> >> index 4484bda410..7630e16854 100644
> >
> >>  # use the silver searcher for git jump grep
> >> -git config jump.grepCmd "ag --column"
> >> +git config jump.grepCmd "ag"
> >
> > I think this change originates from Ævar's comment that it "also makes
> > sense to update the example added in 007d06aa57 [...] which seems to
> > have added jump.grepCmd as a workaround for not having this" [1].
> >
> > Somehow though, this approach seems a bit backwards to me. I do believe
> > that your series reduces the reasons for using `jump.grepCmd`, but
> > regressing this example usage of `jump.grepCmd` seems a bit hostile. If
> > someone wants to use `ag`, wouldn't we want to hint that they will
> > probably want to use `--column`?
> >
> > Is there some other `ag --column --foo` that we can give, where `--foo`
> > is not yet in `git grep`? ;-)
> >
> > Martin
> >
> > [1] https://public-inbox.org/git/874lk2e4he@evledraar.gmail.com/
>
> Yeah it doesn't make sense to drop --column here, FWIW what I had in
> mind was something closer to:

Thanks; I wasn't quite clear on what you had suggested in [1], so the
attached diff is very helpful.

> diff --git a/contrib/git-jump/README b/contrib/git-jump/README
> index 4484bda410..357f79371a 100644
> --- a/contrib/git-jump/README
> +++ b/contrib/git-jump/README
> @@ -25,6 +25,13 @@ git-jump will feed this to the editor:
>  foo.c:2: printf("hello word!\n");
>  ---
>
> +Or, when running 'git jump grep' column numbers will also be emitted,
> +e.g. `git jump grep "hello"' would return:
> +
> +---
> +foo.c:2:10: printf("hello word!\n");
> +---
> +
>  Obviously this trivial case isn't that interesting; you could just open
>  `foo.c` yourself. But when you have many changes scattered across a
>  project, you can use the editor's support to "jump" from point to point.
>
> I.e. let's note what the output format is now like for 'grep', and no
> need to change the jump.grepCmd.

Applied (mostly) the above patch to my copy, and will attach as part of
v5.

> The above patch may be incorrect when it comes to the line numbe /
> column number / format, I just wrote that by hand.

Yes; the only thing that was wrong was the column number. The "w" is in
the 10th 1-indexed column, and 'git grep --column' uses 1-indexed
columns.

Thanks,
Taylor

Re: [PATCH 01/28] t/test-lib: add an SHA1 prerequisite

[brian m. carlson]

On Mon, May 07, 2018 at 12:10:39PM +0200, Martin Ågren wrote:
> On 7 May 2018 at 01:17, brian m. carlson  wrote:
> > Add an SHA1 prerequisite to annotate both of these types of tests and
> > disable them when we're using a different hash.  In the future, we can
> > create versions of these tests which handle both SHA-1 and NewHash.
> 
> Minor nit: s/can/can and should/

I agree with that sentiment.  I can change that.

> > +
> > +# SHA1 is a test if the hash algorithm in use is SHA-1.  This is both for 
> > tests
> > +# which will not work with other hash algorithms and tests that work but 
> > don't
> > +# test anything meaningful (e.g. special values which cause short 
> > collisions).
> > +test_lazy_prereq SHA1 '
> > +   test $(git hash-object /dev/null) = 
> > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
> > +'
> 
> So SHA1 means roughly "git hash-object uses SHA-1, so supposedly
> everything on disk is SHA-1." I could imagine one or two different
> meanings: "Git was compiled with support for SHA-1 [oids]."

Currently it means that, yes.  It may specialize to mean, "git emits
SHA-1 output, regardless of the format on disk."  See cases 1 and 2
below.

> Do we actually need more SHA-1-related prereqs, at least long-term, in
> which case we would want to find a more specific name for this one now?
> Is this SHA1_STORAGE, or some much better name than that?

We may.  The transition plan anticipates several states:

1. Store data in NewHash, but input and output are SHA-1.
2. Store data in NewHash; output is SHA-1; input can be either.
3. Store data and output in NewHash; input can be either.
4. All NewHash.

At this point, I'm working on getting the tests to handle case 4, as
that's actually the easiest to fix (because if things are wrong, the
code tends to segfault).  Case 1 will be next, in which case, we may
need SHA1_STORAGE or such.

I plan to make the SHA1 prerequisite go away or at least be far less
used in a few releases.  Once we know what NewHash is going to be, it's
pretty easy to write tooling and translation tables that do the
conversion for most tests, and a test helper can simply emit the right
output for whichever algorithm we're using in that situation, whether
that's the on-disk algorithm, the input algorithm, or the output
algorithm.

> I am thinking for example about a repo with NewHash that gets pushed to
> and fetched from a SHA-1 server, see hash-function-transition.txt, goal
> 1b. We'd want to always test that SHA-1-related functionality in git.
> (But only until the day when someone defines a prereq such as "SHA1" to
> be able to test a git that was compiled without any traces of SHA-1
> whatsoever.)

I anticipate that by the time we get to that point, the SHA1
prerequisite will be long gone and can be reused for that purpose,
should we need it.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH 8/8] gpg-interface: handle alternative signature types

[brian m. carlson]

On Mon, May 07, 2018 at 05:45:00AM -0400, Jeff King wrote:
> Isn't that basically what this patch is, though? Or at least a step in
> that direction? For generic signing support, you need:
> 
>   1. A way to tell Git to recognize that a signature exists, and what
>  type it is.
> 
>   2. A way to tell Git how to invoke the signing tool.
> 
> Let me discuss (2) first.  In your git-sign-* world, then (2) requires
> us to define a set interface to those helpers, including which action to
> perform, which key to use, etc. And then the logic is inside the helper
> to translate that to the tool's interface.
> 
> The direction I anticipated for this patch was more like:
> 
>  - for now, we just piggy-back on gpg's interface for interacting with
>sub-programs. That makes gpgsm Just Work, and it means that you can
>plug in any other tool by writing a wrapper which converts from gpg
>options to the tool's interface. I.e., gpg's "-bsau" becomes the
>lingua franca, rather than us inventing a new one.
> 
>  - the config schema leaves room for adding new properties to each tool.
>So eventually we could support other option microformats by adding
>signingtool.foo.interface = "signify" or whatever.
> 
>That still leaves room if we want to design our own helper interface.
>One thing we could do that this patch doesn't is require the user to
>explicitly ask for "interface = gpg" (and set it by default for the
>gpg tool stanza). And then leave it as an error if you have a tool
>config that doesn't specify its interface type, which leaves room for
>us later to make that default our generic interface.
> 
> Getting back to (1), how do we tell Git to recognize a signature? I
> think we have to use config here, since it would not know to invoke a
> helper without recognizing the type (and we certainly do not want to
> speculatively invoke each helper saying "do you understand this?" for
> efficiency reasons).

I think my main objection to this series is that it is generic in a way
that isn't necessarily useful.  We know there are essentially only two
formats of PEM-style signatures: OpenPGP and CMS[0].  Even if there are
more, they aren't intrinsically useful, because our codebase can only
handle GnuPG-style tools, and those are the only formats GnuPG-style
tools really support (although, as you point out, other tools could
mimic the interface).

I think if we aren't going to implement some sort of interface that's
generically useful for all signing tools, it would be better to simply
say that we support gpg and gpgsm and have signingtool.gpg.program and
signingtool.gpgsm.program and hard-code the logic for those two formats.
That way we don't have a generic interface that's really only useful for
PEM-style tools, when we know it likely won't be useful for other tools
as well.  We can add a more generic interface when we have more varied
tools to support and we know more about what the requirements will be.

This doesn't address Junio's concern about whether adding CMS support is
the right direction to go.  I personally think OpenPGP is the right
direction for most open-source projects, but I know some companies want
to use CMS internally and I'm not intrinsically opposed to that[1].
That decision is ultimately up to Junio, though.

[0] I'm ignoring the original PEM, which specifies MD2 and MD5,
algorithms that nobody should be using these days.
[1] I would welcome, though, if one could configure only one type of
signature verification by, say, setting the signing program to
/bin/false in the config.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

[PATCH v2 09/13] alloc: add repository argument to alloc_report

[Stefan Beller]

This is a small mechanical change; it doesn't change the
implementation to handle repositories other than the_repository yet.
Use a macro to catch callers passing a repository other than
the_repository at compile time.

Signed-off-by: Stefan Beller 
---
 alloc.c | 2 +-
 cache.h | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/alloc.c b/alloc.c
index f031ce422d9..28b85b22144 100644
--- a/alloc.c
+++ b/alloc.c
@@ -105,7 +105,7 @@ static void report(const char *name, unsigned int count, 
size_t size)
 #define REPORT(name, type) \
 report(#name, name##_state.count, name##_state.count * sizeof(type) >> 10)
 
-void alloc_report(void)
+void alloc_report_the_repository(void)
 {
REPORT(blob, struct blob);
REPORT(tree, struct tree);
diff --git a/cache.h b/cache.h
index 2d60359a964..01cc207d218 100644
--- a/cache.h
+++ b/cache.h
@@ -1774,7 +1774,8 @@ extern void *alloc_commit_node_the_repository(void);
 extern void *alloc_tag_node_the_repository(void);
 #define alloc_object_node(r) alloc_object_node_##r()
 extern void *alloc_object_node_the_repository(void);
-extern void alloc_report(void);
+#define alloc_report(r) alloc_report_##r()
+extern void alloc_report_the_repository(void);
 extern unsigned int alloc_commit_index(void);
 
 /* pkt-line.c */
-- 
2.17.0.255.g8bfb7c0704

[PATCH v2 01/13] repository: introduce parsed objects field

[Stefan Beller]

Convert the existing global cache for parsed objects (obj_hash) into
repository-specific parsed object caches. Existing code that uses
obj_hash are modified to use the parsed object cache of
the_repository; future patches will use the parsed object caches of
other repositories.

Another future use case for a pool of objects is ease of memory management
in revision walking: If we can free the rev-list related memory early in
pack-objects (e.g. part of repack operation) then it could lower memory
pressure significantly when running on large repos. While this has been
discussed on the mailing list lately, this series doesn't implement this.

Signed-off-by: Stefan Beller 
---
 object.c | 63 +---
 object.h |  8 +++
 repository.c |  7 ++
 repository.h | 13 ++-
 4 files changed, 67 insertions(+), 24 deletions(-)

diff --git a/object.c b/object.c
index 5044d08e96c..f7c624a7ba6 100644
--- a/object.c
+++ b/object.c
@@ -8,17 +8,14 @@
 #include "object-store.h"
 #include "packfile.h"
 
-static struct object **obj_hash;
-static int nr_objs, obj_hash_size;
-
 unsigned int get_max_object_index(void)
 {
-   return obj_hash_size;
+   return the_repository->parsed_objects->obj_hash_size;
 }
 
 struct object *get_indexed_object(unsigned int idx)
 {
-   return obj_hash[idx];
+   return the_repository->parsed_objects->obj_hash[idx];
 }
 
 static const char *object_type_strings[] = {
@@ -90,15 +87,16 @@ struct object *lookup_object(const unsigned char *sha1)
unsigned int i, first;
struct object *obj;
 
-   if (!obj_hash)
+   if (!the_repository->parsed_objects->obj_hash)
return NULL;
 
-   first = i = hash_obj(sha1, obj_hash_size);
-   while ((obj = obj_hash[i]) != NULL) {
+   first = i = hash_obj(sha1,
+the_repository->parsed_objects->obj_hash_size);
+   while ((obj = the_repository->parsed_objects->obj_hash[i]) != NULL) {
if (!hashcmp(sha1, obj->oid.hash))
break;
i++;
-   if (i == obj_hash_size)
+   if (i == the_repository->parsed_objects->obj_hash_size)
i = 0;
}
if (obj && i != first) {
@@ -107,7 +105,8 @@ struct object *lookup_object(const unsigned char *sha1)
 * that we do not need to walk the hash table the next
 * time we look for it.
 */
-   SWAP(obj_hash[i], obj_hash[first]);
+   SWAP(the_repository->parsed_objects->obj_hash[i],
+the_repository->parsed_objects->obj_hash[first]);
}
return obj;
 }
@@ -124,19 +123,19 @@ static void grow_object_hash(void)
 * Note that this size must always be power-of-2 to match hash_obj
 * above.
 */
-   int new_hash_size = obj_hash_size < 32 ? 32 : 2 * obj_hash_size;
+   int new_hash_size = the_repository->parsed_objects->obj_hash_size < 32 
? 32 : 2 * the_repository->parsed_objects->obj_hash_size;
struct object **new_hash;
 
new_hash = xcalloc(new_hash_size, sizeof(struct object *));
-   for (i = 0; i < obj_hash_size; i++) {
-   struct object *obj = obj_hash[i];
+   for (i = 0; i < the_repository->parsed_objects->obj_hash_size; i++) {
+   struct object *obj = 
the_repository->parsed_objects->obj_hash[i];
if (!obj)
continue;
insert_obj_hash(obj, new_hash, new_hash_size);
}
-   free(obj_hash);
-   obj_hash = new_hash;
-   obj_hash_size = new_hash_size;
+   free(the_repository->parsed_objects->obj_hash);
+   the_repository->parsed_objects->obj_hash = new_hash;
+   the_repository->parsed_objects->obj_hash_size = new_hash_size;
 }
 
 void *create_object(const unsigned char *sha1, void *o)
@@ -147,11 +146,12 @@ void *create_object(const unsigned char *sha1, void *o)
obj->flags = 0;
hashcpy(obj->oid.hash, sha1);
 
-   if (obj_hash_size - 1 <= nr_objs * 2)
+   if (the_repository->parsed_objects->obj_hash_size - 1 <= 
the_repository->parsed_objects->nr_objs * 2)
grow_object_hash();
 
-   insert_obj_hash(obj, obj_hash, obj_hash_size);
-   nr_objs++;
+   insert_obj_hash(obj, the_repository->parsed_objects->obj_hash,
+   the_repository->parsed_objects->obj_hash_size);
+   the_repository->parsed_objects->nr_objs++;
return obj;
 }
 
@@ -431,8 +431,8 @@ void clear_object_flags(unsigned flags)
 {
int i;
 
-   for (i=0; i < obj_hash_size; i++) {
-   struct object *obj = obj_hash[i];
+   for (i=0; i < the_repository->parsed_objects->obj_hash_size; i++) {
+   struct object *obj = 
the_repository->parsed_objects->obj_hash[i];
if (obj)
obj->flags &= ~flags;
}

[PATCH v2 07/13] alloc: add repository argument to alloc_tag_node

[Stefan Beller]

This is a small mechanical change; it doesn't change the
implementation to handle repositories other than the_repository yet.
Use a macro to catch callers passing a repository other than
the_repository at compile time.

Signed-off-by: Stefan Beller 
---
 alloc.c | 2 +-
 cache.h | 3 ++-
 tag.c   | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/alloc.c b/alloc.c
index 9e2b897ec1d..290250e3595 100644
--- a/alloc.c
+++ b/alloc.c
@@ -65,7 +65,7 @@ void *alloc_tree_node_the_repository(void)
 }
 
 static struct alloc_state tag_state;
-void *alloc_tag_node(void)
+void *alloc_tag_node_the_repository(void)
 {
struct tag *t = alloc_node(_state, sizeof(struct tag));
t->object.type = OBJ_TAG;
diff --git a/cache.h b/cache.h
index bf6e8c87d83..32f340cde59 100644
--- a/cache.h
+++ b/cache.h
@@ -1770,7 +1770,8 @@ extern void *alloc_blob_node_the_repository(void);
 extern void *alloc_tree_node_the_repository(void);
 #define alloc_commit_node(r) alloc_commit_node_##r()
 extern void *alloc_commit_node_the_repository(void);
-extern void *alloc_tag_node(void);
+#define alloc_tag_node(r) alloc_tag_node_##r()
+extern void *alloc_tag_node_the_repository(void);
 extern void *alloc_object_node(void);
 extern void alloc_report(void);
 extern unsigned int alloc_commit_index(void);
diff --git a/tag.c b/tag.c
index 7150b759d66..02ef4eaafc0 100644
--- a/tag.c
+++ b/tag.c
@@ -94,7 +94,7 @@ struct tag *lookup_tag(const struct object_id *oid)
struct object *obj = lookup_object(oid->hash);
if (!obj)
return create_object(the_repository, oid->hash,
-alloc_tag_node());
+alloc_tag_node(the_repository));
return object_as_type(obj, OBJ_TAG, 0);
 }
 
-- 
2.17.0.255.g8bfb7c0704

[PATCH v2 06/13] alloc: add repository argument to alloc_commit_node

[Stefan Beller]

This is a small mechanical change; it doesn't change the
implementation to handle repositories other than the_repository yet.
Use a macro to catch callers passing a repository other than
the_repository at compile time.

Signed-off-by: Stefan Beller 
---
 alloc.c   | 2 +-
 blame.c   | 2 +-
 cache.h   | 3 ++-
 commit.c  | 2 +-
 merge-recursive.c | 2 +-
 5 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/alloc.c b/alloc.c
index 2c8d1430758..9e2b897ec1d 100644
--- a/alloc.c
+++ b/alloc.c
@@ -88,7 +88,7 @@ unsigned int alloc_commit_index(void)
return count++;
 }
 
-void *alloc_commit_node(void)
+void *alloc_commit_node_the_repository(void)
 {
struct commit *c = alloc_node(_state, sizeof(struct commit));
c->object.type = OBJ_COMMIT;
diff --git a/blame.c b/blame.c
index dfa24473dc6..ba9b18e7542 100644
--- a/blame.c
+++ b/blame.c
@@ -161,7 +161,7 @@ static struct commit *fake_working_tree_commit(struct 
diff_options *opt,
 
read_cache();
time();
-   commit = alloc_commit_node();
+   commit = alloc_commit_node(the_repository);
commit->object.parsed = 1;
commit->date = now;
parent_tail = >parents;
diff --git a/cache.h b/cache.h
index 1717d07a2c5..bf6e8c87d83 100644
--- a/cache.h
+++ b/cache.h
@@ -1768,7 +1768,8 @@ void encode_85(char *buf, const unsigned char *data, int 
bytes);
 extern void *alloc_blob_node_the_repository(void);
 #define alloc_tree_node(r) alloc_tree_node_##r()
 extern void *alloc_tree_node_the_repository(void);
-extern void *alloc_commit_node(void);
+#define alloc_commit_node(r) alloc_commit_node_##r()
+extern void *alloc_commit_node_the_repository(void);
 extern void *alloc_tag_node(void);
 extern void *alloc_object_node(void);
 extern void alloc_report(void);
diff --git a/commit.c b/commit.c
index 9106acf0aad..a9a43e79bae 100644
--- a/commit.c
+++ b/commit.c
@@ -51,7 +51,7 @@ struct commit *lookup_commit(const struct object_id *oid)
struct object *obj = lookup_object(oid->hash);
if (!obj)
return create_object(the_repository, oid->hash,
-alloc_commit_node());
+alloc_commit_node(the_repository));
return object_as_type(obj, OBJ_COMMIT, 0);
 }
 
diff --git a/merge-recursive.c b/merge-recursive.c
index 0c0d48624da..6dac8908648 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -98,7 +98,7 @@ static struct tree *shift_tree_object(struct tree *one, 
struct tree *two,
 
 static struct commit *make_virtual_commit(struct tree *tree, const char 
*comment)
 {
-   struct commit *commit = alloc_commit_node();
+   struct commit *commit = alloc_commit_node(the_repository);
 
set_merge_remote_desc(commit, comment, (struct object *)commit);
commit->tree = tree;
-- 
2.17.0.255.g8bfb7c0704

[PATCH v2 05/13] alloc: add repository argument to alloc_tree_node

[Stefan Beller]

This is a small mechanical change; it doesn't change the
implementation to handle repositories other than the_repository yet.
Use a macro to catch callers passing a repository other than
the_repository at compile time.

Signed-off-by: Stefan Beller 
---
 alloc.c | 2 +-
 cache.h | 3 ++-
 tree.c  | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/alloc.c b/alloc.c
index 6c5c376a25a..2c8d1430758 100644
--- a/alloc.c
+++ b/alloc.c
@@ -57,7 +57,7 @@ void *alloc_blob_node_the_repository(void)
 }
 
 static struct alloc_state tree_state;
-void *alloc_tree_node(void)
+void *alloc_tree_node_the_repository(void)
 {
struct tree *t = alloc_node(_state, sizeof(struct tree));
t->object.type = OBJ_TREE;
diff --git a/cache.h b/cache.h
index 2258e611275..1717d07a2c5 100644
--- a/cache.h
+++ b/cache.h
@@ -1766,7 +1766,8 @@ void encode_85(char *buf, const unsigned char *data, int 
bytes);
 /* alloc.c */
 #define alloc_blob_node(r) alloc_blob_node_##r()
 extern void *alloc_blob_node_the_repository(void);
-extern void *alloc_tree_node(void);
+#define alloc_tree_node(r) alloc_tree_node_##r()
+extern void *alloc_tree_node_the_repository(void);
 extern void *alloc_commit_node(void);
 extern void *alloc_tag_node(void);
 extern void *alloc_object_node(void);
diff --git a/tree.c b/tree.c
index 63730e3fb46..58cf19b4fa8 100644
--- a/tree.c
+++ b/tree.c
@@ -197,7 +197,7 @@ struct tree *lookup_tree(const struct object_id *oid)
struct object *obj = lookup_object(oid->hash);
if (!obj)
return create_object(the_repository, oid->hash,
-alloc_tree_node());
+alloc_tree_node(the_repository));
return object_as_type(obj, OBJ_TREE, 0);
 }
 
-- 
2.17.0.255.g8bfb7c0704

[PATCH v2 13/13] alloc: allow arbitrary repositories for alloc functions

[Stefan Beller]

We have to convert all of the alloc functions at once, because alloc_report
uses a funky macro for reporting. It is better for the sake of mechanical
conversion to convert multiple functions at once rather than changing the
structure of the reporting function.

We record all memory allocation in alloc.c, and free them in
clear_alloc_state, which is called for all repositories except
the_repository.

Signed-off-by: Stefan Beller 
---
 alloc.c   | 63 +--
 alloc.h   | 15 +++
 blame.c   |  1 +
 blob.c|  1 +
 cache.h   | 16 
 commit.c  |  1 +
 merge-recursive.c |  1 +
 object.c  | 34 +++--
 object.h  |  8 ++
 tag.c |  1 +
 tree.c|  1 +
 11 files changed, 100 insertions(+), 42 deletions(-)
 create mode 100644 alloc.h

diff --git a/alloc.c b/alloc.c
index 277dadd221b..cbdbbc1dd2d 100644
--- a/alloc.c
+++ b/alloc.c
@@ -4,8 +4,7 @@
  * Copyright (C) 2006 Linus Torvalds
  *
  * The standard malloc/free wastes too much space for objects, partly because
- * it maintains all the allocation infrastructure (which isn't needed, since
- * we never free an object descriptor anyway), but even more because it ends
+ * it maintains all the allocation infrastructure, but even more because it 
ends
  * up with maximal alignment because it doesn't know what the object alignment
  * for the new allocation is.
  */
@@ -15,6 +14,7 @@
 #include "tree.h"
 #include "commit.h"
 #include "tag.h"
+#include "alloc.h"
 
 #define BLOCKING 1024
 
@@ -30,8 +30,25 @@ struct alloc_state {
int count; /* total number of nodes allocated */
int nr;/* number of nodes left in current allocation */
void *p;   /* first free node in current allocation */
+
+   /* bookkeeping of allocations */
+   void **slabs;
+   int slab_nr, slab_alloc;
 };
 
+void *allocate_alloc_state(void)
+{
+   return xcalloc(1, sizeof(struct alloc_state));
+}
+
+void clear_alloc_state(struct alloc_state *s)
+{
+   while (s->slab_nr > 0) {
+   s->slab_nr--;
+   free(s->slabs[s->slab_nr]);
+   }
+}
+
 static inline void *alloc_node(struct alloc_state *s, size_t node_size)
 {
void *ret;
@@ -39,60 +56,57 @@ static inline void *alloc_node(struct alloc_state *s, 
size_t node_size)
if (!s->nr) {
s->nr = BLOCKING;
s->p = xmalloc(BLOCKING * node_size);
+
+   ALLOC_GROW(s->slabs, s->slab_nr + 1, s->slab_alloc);
+   s->slabs[s->slab_nr++] = s->p;
}
s->nr--;
s->count++;
ret = s->p;
s->p = (char *)s->p + node_size;
memset(ret, 0, node_size);
+
return ret;
 }
 
-static struct alloc_state blob_state;
-void *alloc_blob_node_the_repository(void)
+void *alloc_blob_node(struct repository *r)
 {
-   struct blob *b = alloc_node(_state, sizeof(struct blob));
+   struct blob *b = alloc_node(r->parsed_objects->blob_state, 
sizeof(struct blob));
b->object.type = OBJ_BLOB;
return b;
 }
 
-static struct alloc_state tree_state;
-void *alloc_tree_node_the_repository(void)
+void *alloc_tree_node(struct repository *r)
 {
-   struct tree *t = alloc_node(_state, sizeof(struct tree));
+   struct tree *t = alloc_node(r->parsed_objects->tree_state, 
sizeof(struct tree));
t->object.type = OBJ_TREE;
return t;
 }
 
-static struct alloc_state tag_state;
-void *alloc_tag_node_the_repository(void)
+void *alloc_tag_node(struct repository *r)
 {
-   struct tag *t = alloc_node(_state, sizeof(struct tag));
+   struct tag *t = alloc_node(r->parsed_objects->tag_state, sizeof(struct 
tag));
t->object.type = OBJ_TAG;
return t;
 }
 
-static struct alloc_state object_state;
-void *alloc_object_node_the_repository(void)
+void *alloc_object_node(struct repository *r)
 {
-   struct object *obj = alloc_node(_state, sizeof(union 
any_object));
+   struct object *obj = alloc_node(r->parsed_objects->object_state, 
sizeof(union any_object));
obj->type = OBJ_NONE;
return obj;
 }
 
-static struct alloc_state commit_state;
-
-unsigned int alloc_commit_index_the_repository(void)
+unsigned int alloc_commit_index(struct repository *r)
 {
-   static unsigned int count;
-   return count++;
+   return r->parsed_objects->commit_count++;
 }
 
-void *alloc_commit_node_the_repository(void)
+void *alloc_commit_node(struct repository *r)
 {
-   struct commit *c = alloc_node(_state, sizeof(struct commit));
+   struct commit *c = alloc_node(r->parsed_objects->commit_state, 
sizeof(struct commit));
c->object.type = OBJ_COMMIT;
-   c->index = alloc_commit_index(the_repository);
+   c->index = alloc_commit_index(r);
return c;
 }
 
@@ -103,9 +117,10 @@ static void report(const char *name, unsigned int count, 
size_t size)
 }

[PATCH v2 02/13] object: add repository argument to create_object

[Stefan Beller]

Add a repository argument to allow the callers of create_object
to be more specific about which repository to act on. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 blob.c   | 4 +++-
 commit.c | 3 ++-
 object.c | 5 +++--
 object.h | 3 ++-
 tag.c| 3 ++-
 tree.c   | 3 ++-
 6 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/blob.c b/blob.c
index fa2ab4f7a74..85c2143f299 100644
--- a/blob.c
+++ b/blob.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "blob.h"
+#include "repository.h"
 
 const char *blob_type = "blob";
 
@@ -7,7 +8,8 @@ struct blob *lookup_blob(const struct object_id *oid)
 {
struct object *obj = lookup_object(oid->hash);
if (!obj)
-   return create_object(oid->hash, alloc_blob_node());
+   return create_object(the_repository, oid->hash,
+alloc_blob_node());
return object_as_type(obj, OBJ_BLOB, 0);
 }
 
diff --git a/commit.c b/commit.c
index ca474a7c112..9106acf0aad 100644
--- a/commit.c
+++ b/commit.c
@@ -50,7 +50,8 @@ struct commit *lookup_commit(const struct object_id *oid)
 {
struct object *obj = lookup_object(oid->hash);
if (!obj)
-   return create_object(oid->hash, alloc_commit_node());
+   return create_object(the_repository, oid->hash,
+alloc_commit_node());
return object_as_type(obj, OBJ_COMMIT, 0);
 }
 
diff --git a/object.c b/object.c
index f7c624a7ba6..2de029275bc 100644
--- a/object.c
+++ b/object.c
@@ -138,7 +138,7 @@ static void grow_object_hash(void)
the_repository->parsed_objects->obj_hash_size = new_hash_size;
 }
 
-void *create_object(const unsigned char *sha1, void *o)
+void *create_object_the_repository(const unsigned char *sha1, void *o)
 {
struct object *obj = o;
 
@@ -178,7 +178,8 @@ struct object *lookup_unknown_object(const unsigned char 
*sha1)
 {
struct object *obj = lookup_object(sha1);
if (!obj)
-   obj = create_object(sha1, alloc_object_node());
+   obj = create_object(the_repository, sha1,
+   alloc_object_node());
return obj;
 }
 
diff --git a/object.h b/object.h
index cecda7da370..2cb0b241083 100644
--- a/object.h
+++ b/object.h
@@ -93,7 +93,8 @@ extern struct object *get_indexed_object(unsigned int);
  */
 struct object *lookup_object(const unsigned char *sha1);
 
-extern void *create_object(const unsigned char *sha1, void *obj);
+#define create_object(r, s, o) create_object_##r(s, o)
+extern void *create_object_the_repository(const unsigned char *sha1, void 
*obj);
 
 void *object_as_type(struct object *obj, enum object_type type, int quiet);
 
diff --git a/tag.c b/tag.c
index 3d37c1bd251..7150b759d66 100644
--- a/tag.c
+++ b/tag.c
@@ -93,7 +93,8 @@ struct tag *lookup_tag(const struct object_id *oid)
 {
struct object *obj = lookup_object(oid->hash);
if (!obj)
-   return create_object(oid->hash, alloc_tag_node());
+   return create_object(the_repository, oid->hash,
+alloc_tag_node());
return object_as_type(obj, OBJ_TAG, 0);
 }
 
diff --git a/tree.c b/tree.c
index 1c68ea586bd..63730e3fb46 100644
--- a/tree.c
+++ b/tree.c
@@ -196,7 +196,8 @@ struct tree *lookup_tree(const struct object_id *oid)
 {
struct object *obj = lookup_object(oid->hash);
if (!obj)
-   return create_object(oid->hash, alloc_tree_node());
+   return create_object(the_repository, oid->hash,
+alloc_tree_node());
return object_as_type(obj, OBJ_TREE, 0);
 }
 
-- 
2.17.0.255.g8bfb7c0704

[PATCH v2 11/13] object: allow grow_object_hash to handle arbitrary repositories

[Stefan Beller]

Reviewed-by: Jonathan Tan 
Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 object.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/object.c b/object.c
index a365a910859..0fcd6f6df42 100644
--- a/object.c
+++ b/object.c
@@ -116,27 +116,27 @@ struct object *lookup_object(const unsigned char *sha1)
  * power of 2 (but at least 32).  Copy the existing values to the new
  * hash map.
  */
-#define grow_object_hash(r) grow_object_hash_##r()
-static void grow_object_hash_the_repository(void)
+static void grow_object_hash(struct repository *r)
 {
int i;
/*
 * Note that this size must always be power-of-2 to match hash_obj
 * above.
 */
-   int new_hash_size = the_repository->parsed_objects->obj_hash_size < 32 
? 32 : 2 * the_repository->parsed_objects->obj_hash_size;
+   int new_hash_size = r->parsed_objects->obj_hash_size < 32 ? 32 : 2 * 
r->parsed_objects->obj_hash_size;
struct object **new_hash;
 
new_hash = xcalloc(new_hash_size, sizeof(struct object *));
-   for (i = 0; i < the_repository->parsed_objects->obj_hash_size; i++) {
-   struct object *obj = 
the_repository->parsed_objects->obj_hash[i];
+   for (i = 0; i < r->parsed_objects->obj_hash_size; i++) {
+   struct object *obj = r->parsed_objects->obj_hash[i];
+
if (!obj)
continue;
insert_obj_hash(obj, new_hash, new_hash_size);
}
-   free(the_repository->parsed_objects->obj_hash);
-   the_repository->parsed_objects->obj_hash = new_hash;
-   the_repository->parsed_objects->obj_hash_size = new_hash_size;
+   free(r->parsed_objects->obj_hash);
+   r->parsed_objects->obj_hash = new_hash;
+   r->parsed_objects->obj_hash_size = new_hash_size;
 }
 
 void *create_object_the_repository(const unsigned char *sha1, void *o)
-- 
2.17.0.255.g8bfb7c0704

[PATCH v2 08/13] alloc: add repository argument to alloc_object_node

[Stefan Beller]

This is a small mechanical change; it doesn't change the
implementation to handle repositories other than the_repository yet.
Use a macro to catch callers passing a repository other than
the_repository at compile time.

Signed-off-by: Stefan Beller 
---
 alloc.c  | 2 +-
 cache.h  | 3 ++-
 object.c | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/alloc.c b/alloc.c
index 290250e3595..f031ce422d9 100644
--- a/alloc.c
+++ b/alloc.c
@@ -73,7 +73,7 @@ void *alloc_tag_node_the_repository(void)
 }
 
 static struct alloc_state object_state;
-void *alloc_object_node(void)
+void *alloc_object_node_the_repository(void)
 {
struct object *obj = alloc_node(_state, sizeof(union 
any_object));
obj->type = OBJ_NONE;
diff --git a/cache.h b/cache.h
index 32f340cde59..2d60359a964 100644
--- a/cache.h
+++ b/cache.h
@@ -1772,7 +1772,8 @@ extern void *alloc_tree_node_the_repository(void);
 extern void *alloc_commit_node_the_repository(void);
 #define alloc_tag_node(r) alloc_tag_node_##r()
 extern void *alloc_tag_node_the_repository(void);
-extern void *alloc_object_node(void);
+#define alloc_object_node(r) alloc_object_node_##r()
+extern void *alloc_object_node_the_repository(void);
 extern void alloc_report(void);
 extern unsigned int alloc_commit_index(void);
 
diff --git a/object.c b/object.c
index 91edc30770c..b8c3f923c51 100644
--- a/object.c
+++ b/object.c
@@ -180,7 +180,7 @@ struct object *lookup_unknown_object(const unsigned char 
*sha1)
struct object *obj = lookup_object(sha1);
if (!obj)
obj = create_object(the_repository, sha1,
-   alloc_object_node());
+   alloc_object_node(the_repository));
return obj;
 }
 
-- 
2.17.0.255.g8bfb7c0704

[PATCH v2 00/13] object store: alloc

[Stefan Beller]

v2:
* I decided to stick with alloc.c and not migrate it to the mem-pool for now.
  The reasoning for that is that mem-pool.h would introduce some alignment
  waste, which I really did not want to.
* renamed to struct parsed_object_pool;
* free'd the additional memory of trees and commits.
* do not special case the_repository for allocation purposes
* corrected commit messages
* I used the (soon to be renamed?) branch-diff tool to attach a diff below.
  (I still need to get used to that format, I find an interdiff of the
   branches easier to read, but that would not yield the commit messages)
   
   

v1:
This applies on top of sb/oid-object-info and is the logical continuum of
the series that it builds on; this brings the object store into more of
Gits code, removing global state, such that reasoning about the state of
the in-memory representation of the repository is easier.

My original plan was to convert lookup_commit_graft as the next series,
which would be similar to lookup_replace_object, as in sb/object-store-replace.
The grafts and shallow mechanism are very close to each other, such that
they need to be converted at the same time, both depending on the
"parsed object store" that is introduced in this commit.

The next series will then convert code in {object/blob/tree/commit/tag}.c
hopefully finishing the lookup_* functions.

I also debated if it is worth converting alloc.c via this patch series
or if it might make more sense to use the new mem-pool by Jameson[1].

I vaguely wonder about the performance impact, as the object allocation
code seemed to be relevant in the past.

[1] https://public-inbox.org/git/20180430153122.243976-1-jam...@microsoft.com/

Any comments welcome,
Thanks,
Stefan

Jonathan Nieder (1):
  object: add repository argument to grow_object_hash

Stefan Beller (12):
  repository: introduce parsed objects field
  object: add repository argument to create_object
  alloc: add repository argument to alloc_blob_node
  alloc: add repository argument to alloc_tree_node
  alloc: add repository argument to alloc_commit_node
  alloc: add repository argument to alloc_tag_node
  alloc: add repository argument to alloc_object_node
  alloc: add repository argument to alloc_report
  alloc: add repository argument to alloc_commit_index
  object: allow grow_object_hash to handle arbitrary repositories
  object: allow create_object to handle arbitrary repositories
  alloc: allow arbitrary repositories for alloc functions

 alloc.c   |  63 +---
 alloc.h   |  15 +++
 blame.c   |   3 +-
 blob.c|   5 ++-
 cache.h   |   9 
 commit.c  |   4 +-
 merge-recursive.c |   3 +-
 object.c  | 105 +-
 object.h  |  18 +++-
 repository.c  |   7 
 repository.h  |  13 +-
 tag.c |   4 +-
 tree.c|   4 +-
 13 files changed, 184 insertions(+), 69 deletions(-)
 create mode 100644 alloc.h


1:  94a4aa2a825 ! 1:  c40aae31a1e repository: introduce object parser field
@@ -1,23 +1,20 @@
 Author: Stefan Beller 
 
-repository: introduce object parser field
+repository: introduce parsed objects field
 
-Git's object access code can be thought of as containing two layers:
-the raw object store provides access to raw object content, while the
-higher level obj_hash code parses raw objects and keeps track of
-parenthood and other object relationships using 'struct object'.
-Keeping these layers separate should make it easier to find relevant
-functions and to change the implementation of one without having to
-touch the other.
+Convert the existing global cache for parsed objects (obj_hash) into
+repository-specific parsed object caches. Existing code that uses
+obj_hash are modified to use the parsed object cache of
+the_repository; future patches will use the parsed object caches of
+other repositories.
 
-Add an object_parser field to 'struct repository' to prepare obj_hash
-to be handled per repository.  Callers still only use the_repository
-for now --- later patches will adapt them to handle arbitrary
-repositories.
+Another future use case for a pool of objects is ease of memory 
management
+in revision walking: If we can free the rev-list related memory early 
in
+pack-objects (e.g. part of repack operation) then it could lower memory
+pressure significantly when running on large repos. While this has been
+discussed on the mailing list lately, this series doesn't implement 
this.
 
-Signed-off-by: Jonathan Nieder 
 Signed-off-by: Stefan Beller 
-Signed-off-by: Junio C Hamano 
 
 diff --git a/object.c b/object.c
 --- 

[PATCH v2 12/13] object: allow create_object to handle arbitrary repositories

[Stefan Beller]

Reviewed-by: Jonathan Tan 
Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 object.c | 12 ++--
 object.h |  3 +--
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/object.c b/object.c
index 0fcd6f6df42..49b952e9299 100644
--- a/object.c
+++ b/object.c
@@ -139,7 +139,7 @@ static void grow_object_hash(struct repository *r)
r->parsed_objects->obj_hash_size = new_hash_size;
 }
 
-void *create_object_the_repository(const unsigned char *sha1, void *o)
+void *create_object(struct repository *r, const unsigned char *sha1, void *o)
 {
struct object *obj = o;
 
@@ -147,12 +147,12 @@ void *create_object_the_repository(const unsigned char 
*sha1, void *o)
obj->flags = 0;
hashcpy(obj->oid.hash, sha1);
 
-   if (the_repository->parsed_objects->obj_hash_size - 1 <= 
the_repository->parsed_objects->nr_objs * 2)
-   grow_object_hash(the_repository);
+   if (r->parsed_objects->obj_hash_size - 1 <= r->parsed_objects->nr_objs 
* 2)
+   grow_object_hash(r);
 
-   insert_obj_hash(obj, the_repository->parsed_objects->obj_hash,
-   the_repository->parsed_objects->obj_hash_size);
-   the_repository->parsed_objects->nr_objs++;
+   insert_obj_hash(obj, r->parsed_objects->obj_hash,
+   r->parsed_objects->obj_hash_size);
+   r->parsed_objects->nr_objs++;
return obj;
 }
 
diff --git a/object.h b/object.h
index 2cb0b241083..b41d7a3accb 100644
--- a/object.h
+++ b/object.h
@@ -93,8 +93,7 @@ extern struct object *get_indexed_object(unsigned int);
  */
 struct object *lookup_object(const unsigned char *sha1);
 
-#define create_object(r, s, o) create_object_##r(s, o)
-extern void *create_object_the_repository(const unsigned char *sha1, void 
*obj);
+extern void *create_object(struct repository *r, const unsigned char *sha1, 
void *obj);
 
 void *object_as_type(struct object *obj, enum object_type type, int quiet);
 
-- 
2.17.0.255.g8bfb7c0704

[PATCH v2 10/13] alloc: add repository argument to alloc_commit_index

[Stefan Beller]

This is a small mechanical change; it doesn't change the
implementation to handle repositories other than the_repository yet.
Use a macro to catch callers passing a repository other than
the_repository at compile time.

Signed-off-by: Stefan Beller 
---
 alloc.c  | 4 ++--
 cache.h  | 3 ++-
 object.c | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/alloc.c b/alloc.c
index 28b85b22144..277dadd221b 100644
--- a/alloc.c
+++ b/alloc.c
@@ -82,7 +82,7 @@ void *alloc_object_node_the_repository(void)
 
 static struct alloc_state commit_state;
 
-unsigned int alloc_commit_index(void)
+unsigned int alloc_commit_index_the_repository(void)
 {
static unsigned int count;
return count++;
@@ -92,7 +92,7 @@ void *alloc_commit_node_the_repository(void)
 {
struct commit *c = alloc_node(_state, sizeof(struct commit));
c->object.type = OBJ_COMMIT;
-   c->index = alloc_commit_index();
+   c->index = alloc_commit_index(the_repository);
return c;
 }
 
diff --git a/cache.h b/cache.h
index 01cc207d218..0e6c5dd5639 100644
--- a/cache.h
+++ b/cache.h
@@ -1776,7 +1776,8 @@ extern void *alloc_tag_node_the_repository(void);
 extern void *alloc_object_node_the_repository(void);
 #define alloc_report(r) alloc_report_##r()
 extern void alloc_report_the_repository(void);
-extern unsigned int alloc_commit_index(void);
+#define alloc_commit_index(r) alloc_commit_index_##r()
+extern unsigned int alloc_commit_index_the_repository(void);
 
 /* pkt-line.c */
 void packet_trace_identity(const char *prog);
diff --git a/object.c b/object.c
index b8c3f923c51..a365a910859 100644
--- a/object.c
+++ b/object.c
@@ -162,7 +162,7 @@ void *object_as_type(struct object *obj, enum object_type 
type, int quiet)
return obj;
else if (obj->type == OBJ_NONE) {
if (type == OBJ_COMMIT)
-   ((struct commit *)obj)->index = alloc_commit_index();
+   ((struct commit *)obj)->index = 
alloc_commit_index(the_repository);
obj->type = type;
return obj;
}
-- 
2.17.0.255.g8bfb7c0704

[PATCH v2 04/13] alloc: add repository argument to alloc_blob_node

[Stefan Beller]

This is a small mechanical change; it doesn't change the
implementation to handle repositories other than the_repository yet.
Use a macro to catch callers passing a repository other than
the_repository at compile time.

Signed-off-by: Stefan Beller 
---
 alloc.c | 2 +-
 blob.c  | 2 +-
 cache.h | 3 ++-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/alloc.c b/alloc.c
index 12afadfacdd..6c5c376a25a 100644
--- a/alloc.c
+++ b/alloc.c
@@ -49,7 +49,7 @@ static inline void *alloc_node(struct alloc_state *s, size_t 
node_size)
 }
 
 static struct alloc_state blob_state;
-void *alloc_blob_node(void)
+void *alloc_blob_node_the_repository(void)
 {
struct blob *b = alloc_node(_state, sizeof(struct blob));
b->object.type = OBJ_BLOB;
diff --git a/blob.c b/blob.c
index 85c2143f299..9e64f301895 100644
--- a/blob.c
+++ b/blob.c
@@ -9,7 +9,7 @@ struct blob *lookup_blob(const struct object_id *oid)
struct object *obj = lookup_object(oid->hash);
if (!obj)
return create_object(the_repository, oid->hash,
-alloc_blob_node());
+alloc_blob_node(the_repository));
return object_as_type(obj, OBJ_BLOB, 0);
 }
 
diff --git a/cache.h b/cache.h
index 3a4d80e92bf..2258e611275 100644
--- a/cache.h
+++ b/cache.h
@@ -1764,7 +1764,8 @@ int decode_85(char *dst, const char *line, int linelen);
 void encode_85(char *buf, const unsigned char *data, int bytes);
 
 /* alloc.c */
-extern void *alloc_blob_node(void);
+#define alloc_blob_node(r) alloc_blob_node_##r()
+extern void *alloc_blob_node_the_repository(void);
 extern void *alloc_tree_node(void);
 extern void *alloc_commit_node(void);
 extern void *alloc_tag_node(void);
-- 
2.17.0.255.g8bfb7c0704

[PATCH v2 03/13] object: add repository argument to grow_object_hash

[Stefan Beller]

From: Jonathan Nieder 

Add a repository argument to allow the caller of grow_object_hash to
be more specific about which repository to handle. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 object.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/object.c b/object.c
index 2de029275bc..91edc30770c 100644
--- a/object.c
+++ b/object.c
@@ -116,7 +116,8 @@ struct object *lookup_object(const unsigned char *sha1)
  * power of 2 (but at least 32).  Copy the existing values to the new
  * hash map.
  */
-static void grow_object_hash(void)
+#define grow_object_hash(r) grow_object_hash_##r()
+static void grow_object_hash_the_repository(void)
 {
int i;
/*
@@ -147,7 +148,7 @@ void *create_object_the_repository(const unsigned char 
*sha1, void *o)
hashcpy(obj->oid.hash, sha1);
 
if (the_repository->parsed_objects->obj_hash_size - 1 <= 
the_repository->parsed_objects->nr_objs * 2)
-   grow_object_hash();
+   grow_object_hash(the_repository);
 
insert_obj_hash(obj, the_repository->parsed_objects->obj_hash,
the_repository->parsed_objects->obj_hash_size);
-- 
2.17.0.255.g8bfb7c0704

Re: [PATCH v2 02/18] Add a new builtin: branch-diff

[Stefan Beller]

On Mon, May 7, 2018 at 3:05 PM, Igor Djordjevic
 wrote:

> List, rename, delete -- all these seem more as basic CRUD operations,
> where comparison is a more complex one. And not to get me wrong - I
> could see "branch diff" being part of "branch", but not really when
> "diff" already exists as a separate thing, already doing quite some
> (but still diff related, and configurable) stuff.

If we go with "branch --diff", because it has the CRUD operations already
there for branches, I might ask for "remote --diff" to diff two remotes. ;)
(That command "remote --diff" would not make any sense, would it?)

> Basically, what you (conceptually) call "two versions of the same
> branch", I simply call "two branches" (from usage standpoint).

If I diff 2 (topic) branches, which are based on a different version
from upstream, then I see changes from commits that I don't care
about, but this tool explicitly excludes them. Instead it includes
the ordering of the commits as well as its commit messages to
the diff.

So I would not say this tool "diffs two branches", as that is understood
as "diffing the trees, where each of the two branches points two",
whereas this tool diffs a patch series, or if you give Git-ranges,
then it would produce such a patch series in memory.


> And you may have a branch that got split, or more of them that got
> unified, so defining "previous branch version" may not be that
> straightforward - it`s really just "two commit ranges" (as man page
> defines it in general), with "two versions of a patch series" only
> being the most common/expected use case of the former.
>
> Finally, if user picks two totally unrelated "branches" to compare,
> he won`t get a really useful diff - but it`s the same as if he would
> compare two totally unrelated commits (where tree state massively
> changed in between, or having unrelated histories, even).

I used just that, but narrowed down the comparison to one file
instead of the whole tree.

> With something like `git diff --branch ...` you
> would get yet another "diff look", useful for use case in question
> here.

Personally I think this patch series should neither extend git-diff
nor git-branch.

It should not extend git-diff, because currently git-diff can diff
tree-ishs (and does that very well) and comparing to
worktree/index.

It should also not extend git-branch, as that command is for
CRUD operations that you hinted at earlier (Earlier I proposed
git-remote --diff for diffing two remote, which makes no sense,
another one might be git-worktree, which also just does CRUD
for worktrees. It would be a bad idea to have "git worktree --diff")

Hence I propose "git range-diff", similar to topic-diff, that
was proposed earlier.

* it "diffs ranges" of commits.
* it can also deal with out-of-git things like patch series,
  but that is a mere by product and may not be desired.
  Just like git-diff can also compare two files outside a git
  repo, that would not be a good use case.
  Keep the name Git-centric!
* it autocompletes well.

Stefan

Re: [PATCH v2 02/18] Add a new builtin: branch-diff

[Igor Djordjevic]

Hi Dscho,

On 07/05/2018 03:34, Johannes Schindelin wrote:
> 
> > > I think Todd's idea to shift it from a full-blown builtin to a cmdmode
> > > of `branch` makes tons of sense.
> >
> > I don`t know, I still find it a bit strange that in order to "diff
> > something", you go to "something" and tell it to "diff itself" - not
> > because it`s a weird concept (OOP, anyone? :]), but because we already
> > have "diff" command that can accept different things, thus just teaching
> > it to accept additional "something" (branch, in this case), seems more
> > natural (to me) - "branch diff" being just another "diff" mode of
> > operation.
> 
> You also have to call `git branch` to list branches. And to rename
> branches. And to delete them. So why not also compare them at the same
> time?

Maybe because we already have a command that specifically does 
comparison? :)

List, rename, delete -- all these seem more as basic CRUD operations, 
where comparison is a more complex one. And not to get me wrong - I 
could see "branch diff" being part of "branch", but not really when 
"diff" already exists as a separate thing, already doing quite some 
(but still diff related, and configurable) stuff.

> > What about that side thought you left out from my original message,
> > making it `git diff --branch` instead?
> 
> I really did not like this, as all of the `git diff` options really are
> about comparing two revisions, not two *sets* of revisions.

I see what you mean, but I would argue this being a deliberate user 
choice here, like picking a diff "strategy" - I`d say it still utterly 
does compare two revisions (branch tips, in this case), just putting 
focus on comparing revisions that lead to them (branch history), 
instead of just files found in them (branch files).

> Further, if I put my unsuspecting user hat on, I would ask myself how you
> can compare branches with one another? That is what I would expect `git
> diff --branch` to do, not to compare two versions of *the same* branch.

I totally agree with you here, and thus I have a question - what 
determines "two versions of *the same* branch"? :) Do you still 
explicitly provide both "old" and "new" version branch tips?

I see "multiple versions of the same branch" more as a conceptual 
model, and not something Git is aware of (I think?) - BUT, even if it 
was, I don`t see why this should be a(n artificial) restriction?

Basically, what you (conceptually) call "two versions of the same 
branch", I simply call "two branches" (from usage standpoint).

And you may have a branch that got split, or more of them that got 
unified, so defining "previous branch version" may not be that 
straightforward - it`s really just "two commit ranges" (as man page 
defines it in general), with "two versions of a patch series" only 
being the most common/expected use case of the former.

Finally, if user picks two totally unrelated "branches" to compare, 
he won`t get a really useful diff - but it`s the same as if he would 
compare two totally unrelated commits (where tree state massively 
changed in between, or having unrelated histories, even).

Besides, while I might still not be much into the matter, but isn`t 
"branch" in Git just a pointer to revision? Being so, there is really 
no such thing as "branch" in terms of being a specific (sub)set of 
revisions (commits), other then "everything from branch head/pointer 
to root commit" (in general).

Yes, we do perceive "a branch" being a specific set of topic related 
commits, but which *exact* commits we are interested in ("branch" lower 
bounds) may differ in regards to what we aim for - how far do we consider 
one branch to reach in the past depends solely on the use case.

> So `git diff --branch` does not at all convey the same to me as `git
> branch --diff`, and I find that the latter does match better what this
> patch series tries to achieve.

I agree with the first part, but it seems to me your finding is 
biased due to your (expected) use case.

> > But if "branch diff" is considered to be too special-cased mode of
> > "diff" so that supporting it from `diff` itself would make it feel
> > awkward in both usage and maintenance (in terms of many other regular
> > `diff` specific options being unsupported), I guess I would understand
> > having it outside `diff` altogether (and implemented as proposed `git
> > branch --diff`, or something)... for the time being, at least :)
> 
> The branch diff is not even a special-cased mode of diff. It is *way* more
> complicated than that. It tries to find 1:1 correspondences between *sets*
> of commits, and then only outputs a "sort" of a diff between the commits
> that correspond with each other. I say "sort" of a diff because that diff
> does not look like `git diff  ` at all!

But there is not only one `git diff  ` looks, it 
depends on other options (like --name-status, for example), which is 
my point exactly :)

With something like `git diff --branch ...` you 
would get yet 

Re: [PATCH v2 02/18] Add a new builtin: branch-diff

[Igor Djordjevic]

On 07/05/2018 09:48, Jeff King wrote:
> 
> > > Let's, please, not fall into the trap of polluting git-branch with
> > > utterly unrelated functionality, as has happened a few times with
> > > other Git commands. Let's especially not do so merely for the sake of
> > > tab-completion. git-branch is for branch management; it's not for
> > > diff'ing.
> >
> > I totally disagree. `git branch` is *the* command to work with branches.
> > Yes, you can manage branches. But you can also list them. And now you can
> > also compare them.
> 
> One of the things I don't like about "git branch --diff" is that this
> feature is not _just_ about branches at all. E.g., I could do:
> 
>   git tbdiff HEAD~10 HEAD~5 foo
> 
> Or even:
> 
>   git tbdiff v2.16.0 v2.17.0 my-rewritten-v2.17.0
> 
> Those arguments really are just commitishes, not necessarily branches.
> One of the current interface rules for "git branch" is that the branch
> names we hand it are interpreted _exactly_ as branch names. You cannot
> "git branch -m v2.16.0", and there is no ambiguity in "git branch -d
> foo" if "foo" is both a tag and a branch.
> 
> But this new mode does not fit the pattern at all.
> 
> If we were to attach this to an existing command, I think it has more to
> do with "diff" than "branch". But I'm not sure we want to overload
> "diff" either (which has traditionally been about two endpoints, and
> does not really traverse at all, though arguably "foo...bar" is a bit of
> a cheat :) ).
> 
> > > Of the suggestions thus far, Junio's git-topic-diff seems the least
> > > worse, and doesn't suffer from tab-completion problems.
> >
> > Except that this is too limited a view.
> 
> Right, I agree with you. Topic branches are the intended use, but that's
> not what it _does_, and obviously it can be applied in other cases. So
> since "branch" is too specific, I think "topic branch" is even more so.
> 
> It's really "diff-history" or something, I think. That's not very
> catchy, but I think the best name would imply that it was diffing a set
> of commits (so even "diff-commit" would not be right, because that again
> sounds like endpoints).

This is exactly what I feel as well, thanks for concise and 
to-the-point spelling out.

>From user interface perspective, I would expect something like this 
to be possible (and natural):

(1) git diff topic-v1...topic-v2
(2) git diff --branch topic-v1...topic-v2

(1) is what we are all familiar with, providing a diff between two 
revisions with focus on file changes, where (2) shifts focus to 
history changes.

It`s all still a comparison between two revisions (pointed to by 
"topic-v1" and "topic-v2" branch heads in this specific example), but 
it differs in what we are comparing - (1) set of files contained in 
endpoints, or (2) set of revisions contained in (or "leading to") 
endpoints.

Hmm... what about `git diff --history`? :/ It does seem more "true" 
to what it does, though I still like `git diff --branch` more 
(catchier, indeed).

Regards, Buga

Re: [PATCH 4/5] lock_file: make function-local locks non-static

[Martin Ågren]

On 7 May 2018 at 17:24, Duy Nguyen  wrote:
> On Sun, May 6, 2018 at 9:32 PM, Martin Ågren  wrote:
>> On 6 May 2018 at 19:42, Duy Nguyen  wrote:
>> Thank you Duy for your comments. How about I write the commit message
>> like so:
>
> +Jeff. Since he made it possible to remove lock file from the global
> linked list, he probably knows well what to check when switching from
> a static lock file to a stack-local one.
>
>>
>>   After 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05),
>>   we can have lockfiles on the stack. These `struct lock_file`s are local
>>   to their respective functions and we can drop their staticness.
>>
>>   Each of these users either commits or rolls back the lock in every
>>   codepath, with these possible exceptions:
>>
>> * We bail using a call to `die()` or `exit()`. The lock will be
>>   cleaned up automatically.
>>
>> * We return early from a function `cmd_foo()` in builtin/, i.e., we
>>   are just about to exit. The lock will be cleaned up automatically.
>
> There are also signals which can be caught and run on its own stack (I
> think) so whatever variable on the current stack should be safe, I
> guess.

Right, that could also happen.

>>   If I have missed some codepath where we do not exit, yet leave a locked
>>   lock around, that was so also before this patch. If we would later
>>   re-enter the same function, then before this patch, we would be retaking
>>   a lock for the very same `struct lock_file`, which feels awkward, but to
>>   the best of my reading has well-defined behavior. Whereas after this
>>   patch, we would attempt to take the lock with a completely fresh `struct
>>   lock_file`. In both cases, the result would simply be that the lock can
>>   not be taken, which is a situation we already handle.
>
> There is a difference here, if the lock is not released properly,
> previously the lockfile is still untouched. If it's on stack, it may
> be overwritten which can corrupt the linked list to get to the next
> lock file.  (and this is about calling the function in question just
> _once_ not the second time).

One thing I can try to make clearer is that no-one is holding on to the
address of a `struct lock_file`. Not anyone, anywhere. So whether that
location gets filled with random junk after the function has returned is
irrelevant when it comes to the eventual cleaning up. The only thing
that anyone is keeping track of is the *temp*file. The `struct
lock_file` is just a fancy wrapper for keeping a pointer to a tempfile.
The tempfile infrastructure keeps track of the tempfiles directly,
without tracking any lockfiles.

Maybe it is a bit optimistic to tackle all of these at one fell swoop.
Fiddling with lockfiles makes people nervous, for good reasons. I could
go for the ones where it is easy to see that we always clean things up,
then handle the less trivial ones in a second run-over, or not at all.

Martin

Re: [PATCH 00/13] object store: alloc

[Stefan Beller]

Hi Junio,

On Mon, May 7, 2018 at 7:05 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> This applies on top of sb/oid-object-info and is the logical continuum of
>> the series that it builds on; this brings the object store into more of
>> Gits code, removing global state, such that reasoning about the state of
>> the in-memory representation of the repository is easier.
>
> I am not sure how well this topic is done, but I've queued the
> following patch at the tip of the topic to make it compile after
> getting merged to integration branches (curiously, the topic by
> itself compiled file for whatever reason).

Thanks for the fixup; I will include it with other fixes in a reroll.

The investigation why it would not compile is not found in alloc.c
but in 1da1580e4c2 (Makefile: detect compiler and enable more
warnings in DEVELOPER=1, 2018-04-14), which enabled
-Werror=missing-prototypes, that requires a prototype which
is found in alloc.h

Thanks,
Stefan

Re: [PATCH v2 02/18] Add a new builtin: branch-diff

[Stefan Beller]

On Mon, May 7, 2018 at 8:28 AM, Duy Nguyen  wrote:

>>> I do hear you. Especially since I hate `git cherry` every single time that
>>> I try to tab-complete `git cherry-pick`.
>>
>> Me too. :)
>
> Just so you know I'm also not happy with that "git cherry". Since I'm
> updating git-completion.bash in this area and we got 3 "me too" votes
> (four if we count Szeder in another thread), I'm going to implementing
> something to at least let you exclude "cherry" from the completion
> list if you want.

And another "me too" here.

Dearest

[Mrs.Renee Shine]

Dearest,

I am Sister Renee Shine, I contacted you regards to the total sum of(3
million USD) which I signed and donated in your custody as the
beneficiary,I was diagnosed with the cancer of the esophagus it has
defied all forms of medical treatment and at this point i have few
weeks to live according to my Doctor,I had promise God Almighty that i
will use my fortune to bless the poor in the work of the Lord.

I am hopefully trusting in your faithfulness that you shall make use
of the fund for the very purpose to host the charities and the less
privileged as soon as the Bank Transferred it into your account, I
want you to always remember that this is the work of God, The 30% of
the money will be for you and your family, I will provide you with the
Bank (E-mail  Address) where I made the Deposit,  I wait your soon
reply as a matter of urgent so I can detail you properly now that i am
alive.

Please reply with my private email address:(mrs.reneesh...@gmail.com)

Remain blessed

Sister Renee Shine

Re: [PATCH] mailmap: update brian m. carlson's email address

[Stefan Beller]

On Sun, May 6, 2018 at 8:37 PM, Junio C Hamano  wrote:
> "brian m. carlson"  writes:
>
>> The order of addresses in the mailmap file was reversed, leading to git
>> preferring the crustytoothpaste.ath.cx address, which is obsolete, over
>> the crustytoothpaste.net address, which is current.  Switch the order of
>> the addresses so that git log displays the correct address.
>>
>> Signed-off-by: brian m. carlson 
>> ---
>
> I initially reacted to "was reversed" with "yikes, did we break the
> mailmap reader and we need to update the file?", but apparently that
> is not what this patch is about.  I think what is happening here is
> that cdb6b5ac (".mailmap: Combine more (name, email) to individual
> persons", 2013-08-12) removed
>
> -Brian M. Carlson 
>
> and then added these two lines
>
> +brian m. carlson  Brian M. Carlson 
> 
> +brian m. carlson  
> 
>
> where *.net address did not come from any other entry for you in the
> file.  I guess the author of the patch saw that you were sending
> your messages from the .net address and tried to help by unifying
> the two addresses, without knowing your preference and recorded two
> reversed entries.
>
> Will queue as-is for now, but if you want to update the log message
> I do not mind taking a reroll.

brian,

sorry to break you there. I was the author of the patch Junio found, organizing
the .mailmap file was one of my starter contributions. I recall asking all the
people if they were ok with it their names combined in different spellings
94b410bba86 (.mailmap: Map email addresses to names, 2013-07-12)
f4f49e2258a (.mailmap: Combine more (email, name) to individual
persons, 2013-07-14)
and I vaguley recall asking you about capitalization of your name there
and you preferred the lower case name, but apparently I never asked you
about the preferred email address.

Sorry and thanks for fixing,
Stefan

Re: Weak option parsing in `git submodule`

[Stefan Beller]

On Sun, May 6, 2018 at 11:10 AM, Kaartic Sivaraam
 wrote:
> I recently faced the consequence of the weak option parsing done by in
> `git submodule`. Initially tried to add a new submodule using the `git
> submodule add` sub-command in the following way,
>
> $ git submodule add ./foo/bar
>
> This cloned the submodule into a new directory named 'bar' in the
> present directory. This was initially confusing as I expected `git
> submodule` to use the actual directory itself as it resides inside a
> sub-directory the main project and thus avoiding the creation of a new
> clone. Then I came to know that `git submodule` wasn't smart enough yet
> to identify this, by reading the documentation. Then, after realizing
> that I would have to specify the path in the command to avoid the
> redundant clone, I corrected the command without consulting the
> documentation (thoroughly) to,
>
> $ git submodule add ./foo/bar --path ./foo/bar
>
> expecting that the path needs to be specified after an argument. This
> is what triggered the weird consequence of weak option parsing. The
> output I got for the above command was:
>
> The following path is ignored by one of your .gitignore files:
> --path
> Use -f if you really want to add it.

Yuck, that is bad UX.

> That really confused me pretty much (being a person who doesn't know
> much about how `git submodule` works). Instead of complaining about an
> inexistent option '--path', it considers '--path' to be the 
> argument which seems to be bad. It doesn't even complain about the
> extra argument. I also dug to find the reason behind this totally weird
> message. It seems to be a consequence of the following lousy check
> ($sm_path is the normalized  argument):
>
> if test -z "$force" &&
> ! git add --dry-run --ignore-missing --no-warn-embedded-repo 
> "$sm_path" > /dev/null 2>&1
> then
> eval_gettextln "The following path is ignored by one of your 
> .gitignore files:
> \$sm_path
> Use -f if you really want to add it." >&2
> exit 1
> fi
>
> The lack of checking for the reason behind why `git add` fails seems to
> be the reason behind that weird message.

(from the man page)
git submodule [--quiet] add [] [--]  []

When options are given after  or  we can count
the arguments and know something is up. (The number of arguments
must be 1 or 2. If it is 3 or above, something fishy is going on), which
I would suggest as a first step.

> Ways to fix this:
>
> 1. Fix this particular issue by adding a '--' after the '--no-warn-
> embedded-repo' option in the above check. But that would also
> require that we allow other parts of the script to accept weird
> paths such as '--path'. Not so useful/helpful.
>
> 2. Check for the actual return value of `git add` in the check and
> act accordingly. Also, check if there are unnecessary arguments for
> `submodule add`.

The second part of this suggestion seems to me as the way to go.
Do you want to write a patch?

> 3. Tighten option parsing in the `git-submodule` script to ensure
> this doesn't happen in the long term and helps users to get more
> relevant error messages.
>
> I find 3 to be a long term solution but not sure if it's worth trying
> as there are efforts towards porting `git submodule` to C. So, I guess
> we should at least do 2 as a short-term solution.

Yeah, bringing it to C, would be nice as a long term solution instead
of piling up more and more shell features. :)

Thanks for such a well written bug report with suggested bug fixes. :)
Stefan

Re: [PATCH 2/5] refs.c: do not die if locking fails in `write_pseudoref()`

[Stefan Beller]

+cc Michael, who did extensive work in the refs.c code.

On Sun, May 6, 2018 at 7:10 AM, Martin Ågren  wrote:
> If we could not take the lock, we add an error to the `strbuf err` and
> return. However, this code is dead. The reason is that we take the lock
> using `LOCK_DIE_ON_ERROR`. Drop the flag to allow our more gentle
> error-handling to actually kick in.
>
> We could instead just drop the dead code and die here. But everything is
> prepared for gently propagating the error, so let's do that instead.

This looks good to me.

> There is similar dead code in `delete_pseudoref()`, but let's save that
> for the next patch.
>
> While at it, make the lock non-static.

We seem to have a lot of static lockfiles in the code base. IIRC that
was due to some technicality of the lockfiles, as they would also
be cleaned up atexit() and for that it had to be static(?)

Maybe mention why it was static and why we can drop the static
now? Given that you found these answers in a reply below, this is
Reviewed-by: Stefan Beller 

Thanks,
Stefan

>
> Signed-off-by: Martin Ågren 
> ---
>  refs.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 8b7a77fe5e..8c50b8b139 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -644,7 +644,7 @@ static int write_pseudoref(const char *pseudoref, const 
> struct object_id *oid,
>  {
> const char *filename;
> int fd;
> -   static struct lock_file lock;
> +   struct lock_file lock = LOCK_INIT;
> struct strbuf buf = STRBUF_INIT;
> int ret = -1;
>
> @@ -654,8 +654,7 @@ static int write_pseudoref(const char *pseudoref, const 
> struct object_id *oid,
> strbuf_addf(, "%s\n", oid_to_hex(oid));
>
> filename = git_path("%s", pseudoref);
> -   fd = hold_lock_file_for_update_timeout(, filename,
> -  LOCK_DIE_ON_ERROR,
> +   fd = hold_lock_file_for_update_timeout(, filename, 0,
>
> get_files_ref_lock_timeout_ms());
> if (fd < 0) {
> strbuf_addf(err, "could not open '%s' for writing: %s",
> --
> 2.17.0.411.g9fd64c8e46
>

Re: [PATCH v2 00/18] Add `branch-diff`, a `tbdiff` lookalike

[Elijah Newren]

On Mon, May 7, 2018 at 10:50 AM, SZEDER Gábor  wrote:
>> 2) Your completion commands for branch-diff will only complete one
>> revision range, not two.  e.g.
>> git branch-diff origin/master..my-topic@{2} origin/master..my-top
>> won't complete "my-topic" as I'd expect.
>
> It does complete two revision ranges, but if you want to look at
> reflogs, then you must escape the opening curly brace.  I'm not sure
> why, but apparently after the unescaped '{' Bash thinks that it's a
> new command, and doesn't even call our completion functions anymore.
> It's not specific to the completion of 'branch-diff', or even to our
> completion script.  I don't think we can do anything about it.

Ah, indeed.  Thanks for the pointer.

Re: [GSoC] Yet another blog series about the GSoC

[Stefan Beller]

Hi Pratik,

On Sat, May 5, 2018 at 5:24 AM, Pratik Karki  wrote:
> On Sat, May 5, 2018 at 5:11 PM, Alban Gruin  wrote:
>> Hi everybody,
>>
>> as my fellow students, I started a blog series about my GSoC project[1].
>> First, I wanted to post them directly on the mailing list, but a blog
>> allows me to edit the content easily if needed.
>>
>> Any feedback is welcome!
>>
>> [1] https://blog.pa1ch.fr/posts/2018/05/05/en/gsoc2018-week-1.html
>>
>> Cheers,
>> Alban Gruin

Thanks for blogging about GSoC!

> Nice post. Great job, Alban.
> Just a little typo I found out there: hazardous -> hasardous.

I would think hazardous is correct, both in British as well as
American English, I cannot speak for more.

Thanks!
Stefan

Re: [PATCH 2/5] refs.c: do not die if locking fails in `write_pseudoref()`

[David Turner]

On May 6, 2018 9:56:31 AM MDT, "Martin Ågren"  wrote:
>On 6 May 2018 at 17:48, David Turner  wrote:
>> On Sun, 2018-05-06 at 16:10 +0200, Martin Ågren wrote:
>>> While at it, make the lock non-static.
>
>> Re making the lock static, I wonder about the following case:
>>
>>   if (read_ref(pseudoref, _old_oid))
>>
>> die("could not read ref '%s'", pseudoref);
>>
>> I think this calls exit(), and then atexit tries to clean up the lock
>> files.  But since lock is no longer static, the stack may have been
>> destroyed (I don't actually know whether this is true, so maybe
>someone
>> else does).
>
>Right. After commit 076aa2cbda (tempfile: auto-allocate tempfiles on
>heap, 2017-09-05) this is safe though. Quite a few locks have already
>been moved to the stack, e.g., in 14bca6c63c (sequencer: make lockfiles
>non-static, 2018-02-27) and 02ae242fdd (checkout-index: simplify
>locking
>logic, 2017-10-05).  I could add a note to the commit message to make
>this clear, like "After 076aa2cbda, locks no longer need to be static."

I am going to reply now to keep the thread moving, but I am on my phone with 
bad connectivity (few cell towers in Bears Ears), so I can't really check the 
code. Feel free to disregard if I am still wrong.

I saw that patch, but I thought the new logic required that cleanup funtions be 
called before the lock goes out of scope.

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: main url for linking to git source?

[Stefan Beller]

Hi Jeff,

On Sun, May 6, 2018 at 11:37 PM, Jeff King  wrote:
> The git-scm.com site currently links to https://github.com/git/git for
> the (non-tarball) source code. Somebody raised the question[1] of
> whether it should point to kernel.org instead. Do people find one
> interface more or less pleasing than the other? Do we want to prefer
> kernel.org as more "official" or less commercial?
>
> I could see arguments both ways, so I thought I'd take a straw poll of
> what people on the list think.

That PR is changing quite a few places, so in classic Git community
fashion, I advise to break it up into more patches. ;)

Junios reply below focuses on the URL passed to git-clone, which
is only found at https://git-scm.com/downloads (?)

There I would try to mirror Junios list of "public repositories"
https://git-blame.blogspot.com/p/git-public-repositories.html
without officially endorsing one over another.

For those links that link to web pages, I am ok with any of the
hosting providers, maybe even taking the one with the prettiest
web page. Maybe we want to reword those sections to rely
more on indirection, e.g. "get the source[link to the source page],
checkout the next branch", without the quick web link to a web page
showing the 'next' tree. Any of the pages with the 'next' tree
do not really help for the purpose of spotting which development
is currently happening. Maybe a "log --merges" would be better.
Something like https://kernel.googlesource.com/pub/scm/git/git/+log/next
(Not that I am endorsing this link over others. I just happen to know
that this comes close to what I propose having there)

Kernel.org might feel a bit more official than the others because
Linus started it there? And given that it is a non profit, I feel
better to link to them over any other commercial entity.

Hope that helps,
Stefan

[PATCH v6 03/13] help: use command-list.h for common command list

[Nguyễn Thái Ngọc Duy]

The previous commit added code generation for all_cmd_desc[] which
includes almost everything we need to generate common command list.
Convert help code to use that array instead and drop common_cmds[] array.

The description of each common command group is removed from
command-list.txt. This keeps this file format simpler. common-cmds.h
will not be generated correctly after this change due to the
command-list.txt format change. But it does not matter and
common-cmds.h will be removed.
---
 Makefile|   4 +-
 command-list.txt|  10 ---
 generate-cmdlist.sh |   4 +-
 help.c  | 145 +---
 t/t0012-help.sh |   9 +++
 5 files changed, 122 insertions(+), 50 deletions(-)

diff --git a/Makefile b/Makefile
index 2a8913ea21..5c58b0b692 100644
--- a/Makefile
+++ b/Makefile
@@ -1914,9 +1914,9 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
$(filter %.o,$^) $(LIBS)
 
-help.sp help.s help.o: common-cmds.h
+help.sp help.s help.o: common-cmds.h command-list.h
 
-builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h GIT-PREFIX
+builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h command-list.h 
GIT-PREFIX
 builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
diff --git a/command-list.txt b/command-list.txt
index 786536aba0..3bd23201a6 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -1,13 +1,3 @@
-# common commands are grouped by themes
-# these groups are output by 'git help' in the order declared here.
-# map each common command in the command list to one of these groups.
-### common groups (do not change this line)
-init start a working area (see also: git help tutorial)
-worktree work on the current change (see also: git help everyday)
-info examine the history and state (see also: git help revisions)
-history  grow, mark and tweak your common history
-remote   collaborate (see also: git help workflows)
-
 ### command list (do not change this line, also do not change alignment)
 # command name  category [category] [category]
 git-add mainporcelain   worktree
diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
index c9fd524760..93de8e8f59 100755
--- a/generate-cmdlist.sh
+++ b/generate-cmdlist.sh
@@ -6,7 +6,7 @@ die () {
 }
 
 command_list () {
-   sed '1,/^### command list/d;/^#/d' "$1"
+   grep -v '^#' "$1"
 }
 
 get_categories() {
@@ -65,7 +65,7 @@ echo "/* Automatically generated by generate-cmdlist.sh */
 struct cmdname_help {
const char *name;
const char *help;
-   uint32_t group;
+   uint32_t category;
 };
 "
 if [ -z "$2" ]
diff --git a/help.c b/help.c
index 60071a9bea..2d6a3157f8 100644
--- a/help.c
+++ b/help.c
@@ -5,13 +5,114 @@
 #include "run-command.h"
 #include "levenshtein.h"
 #include "help.h"
-#include "common-cmds.h"
+#include "command-list.h"
 #include "string-list.h"
 #include "column.h"
 #include "version.h"
 #include "refs.h"
 #include "parse-options.h"
 
+struct category_description {
+   uint32_t category;
+   const char *desc;
+};
+static uint32_t common_mask =
+   CAT_init | CAT_worktree | CAT_info |
+   CAT_history | CAT_remote;
+static struct category_description common_categories[] = {
+   { CAT_init, N_("start a working area (see also: git help tutorial)") },
+   { CAT_worktree, N_("work on the current change (see also: git help 
everyday)") },
+   { CAT_info, N_("examine the history and state (see also: git help 
revisions)") },
+   { CAT_history, N_("grow, mark and tweak your common history") },
+   { CAT_remote, N_("collaborate (see also: git help workflows)") },
+   { 0, NULL }
+};
+
+static const char *drop_prefix(const char *name)
+{
+   const char *new_name;
+
+   if (skip_prefix(name, "git-", _name))
+   return new_name;
+   return name;
+
+}
+
+static void extract_cmds(struct cmdname_help **p_cmds, uint32_t mask)
+{
+   int i, nr = 0;
+   struct cmdname_help *cmds;
+
+   if (ARRAY_SIZE(command_list) == 0)
+   BUG("empty command_list[] is a sign of broken 
generate-cmdlist.sh");
+
+   ALLOC_ARRAY(cmds, ARRAY_SIZE(command_list) + 1);
+
+   for (i = 0; i < ARRAY_SIZE(command_list); i++) {
+   const struct cmdname_help *cmd = command_list + i;
+
+   if (!(cmd->category & mask))
+   continue;
+
+   cmds[nr] = *cmd;
+   cmds[nr].name = drop_prefix(cmd->name);
+
+   nr++;
+   }
+   cmds[nr].name = NULL;
+   *p_cmds = cmds;
+}
+
+static void print_command_list(const struct cmdname_help *cmds,
+  uint32_t mask, int longest)
+{
+   int i;
+
+   for (i = 0; 

[PATCH v6 08/13] git: support --list-cmds=list-

[Nguyễn Thái Ngọc Duy]

This allows us to select any group of commands by a category defined
in command-list.txt. This is an internal/hidden option so we don't
have to be picky about the category name or worried about exposing too
much.

This will be used later by git-completion.bash to retrieve certain
command groups.
---
 generate-cmdlist.sh | 17 +
 git.c   |  7 +++
 help.c  | 23 +++
 help.h  |  2 ++
 4 files changed, 49 insertions(+)

diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
index 015eef2804..bfd8ef0671 100755
--- a/generate-cmdlist.sh
+++ b/generate-cmdlist.sh
@@ -45,6 +45,21 @@ define_categories() {
test "$bit" -gt 32 && die "Urgh.. too many categories?"
 }
 
+define_category_names() {
+   echo
+   echo "/* Category names */"
+   echo "static const char *category_names[] = {"
+   bit=0
+   category_list "$1" |
+   while read cat
+   do
+   echo "  \"$cat\", /* (1UL << $bit) */"
+   bit=$(($bit+1))
+   done
+   echo "  NULL"
+   echo "};"
+}
+
 print_command_list() {
echo "static struct cmdname_help command_list[] = {"
 
@@ -70,4 +85,6 @@ struct cmdname_help {
 "
 define_categories "$1"
 echo
+define_category_names "$1"
+echo
 print_command_list "$1"
diff --git a/git.c b/git.c
index 3c032d01fc..67f3e20ae9 100644
--- a/git.c
+++ b/git.c
@@ -53,6 +53,13 @@ static int list_cmds(const char *spec)
list_all_main_cmds();
else if (len == 6 && !strncmp(spec, "others", 6))
list_all_other_cmds();
+   else if (len > 5 && !strncmp(spec, "list-", 5)) {
+   struct strbuf sb = STRBUF_INIT;
+
+   strbuf_add(, spec + 5, len - 5);
+   list_cmds_by_category(, sb.buf);
+   strbuf_release();
+   }
else
die(_("unsupported command listing type '%s'"), spec);
spec += len;
diff --git a/help.c b/help.c
index d5ce9dfcbb..1117f7d1d1 100644
--- a/help.c
+++ b/help.c
@@ -329,6 +329,29 @@ void list_all_other_cmds(struct string_list *list)
clean_cmdnames(_cmds);
 }
 
+void list_cmds_by_category(struct string_list *list,
+  const char *cat)
+{
+   int i, n = ARRAY_SIZE(command_list);
+   uint32_t cat_id = 0;
+
+   for (i = 0; category_names[i]; i++) {
+   if (!strcmp(cat, category_names[i])) {
+   cat_id = 1UL << i;
+   break;
+   }
+   }
+   if (!cat_id)
+   die(_("unsupported command listing type '%s'"), cat);
+
+   for (i = 0; i < n; i++) {
+   struct cmdname_help *cmd = command_list + i;
+
+   if (cmd->category & cat_id)
+   string_list_append(list, drop_prefix(cmd->name));
+   }
+}
+
 int is_in_cmdlist(struct cmdnames *c, const char *s)
 {
int i;
diff --git a/help.h b/help.h
index 97e6c0965e..734bba32d3 100644
--- a/help.h
+++ b/help.h
@@ -21,6 +21,8 @@ static inline void mput_char(char c, unsigned int num)
 extern void list_common_cmds_help(void);
 extern void list_all_main_cmds(struct string_list *list);
 extern void list_all_other_cmds(struct string_list *list);
+extern void list_cmds_by_category(struct string_list *list,
+ const char *category);
 extern const char *help_unknown_cmd(const char *cmd);
 extern void load_command_list(const char *prefix,
  struct cmdnames *main_cmds,
-- 
2.17.0.705.g3525833791

[PATCH v6 11/13] command-list.txt: documentation and guide line

[Nguyễn Thái Ngọc Duy]

This is intended to help anybody who needs to update command-list.txt.
It gives a brief introduction of all attributes a command can take.
---
 command-list.txt | 44 
 1 file changed, 44 insertions(+)

diff --git a/command-list.txt b/command-list.txt
index 99ddc231c1..9c70c69193 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -1,3 +1,47 @@
+# Command classification list
+# ---
+# All supported commands, builtin or external, must be described in
+# here. This info is used to list commands in various places. Each
+# command is on one line followed by one or more attributes.
+#
+# The first attribute group is mandatory and indicates the command
+# type. This group includes:
+#
+#   mainporcelain
+#   ancillarymanipulators
+#   ancillaryinterrogators
+#   foreignscminterface
+#   plumbingmanipulators
+#   plumbinginterrogators
+#   synchingrepositories
+#   synchelpers
+#   purehelpers
+#
+# The type names are self explanatory. But if you want to see what
+# command belongs to what group to get a better picture, have a look
+# at "git" man page, "GIT COMMANDS" section.
+#
+# Commands of type mainporcelain can also optionally have one of these
+# attributes:
+#
+#   init
+#   worktree
+#   info
+#   history
+#   remote
+#
+# These commands are considered "common" and will show up in "git
+# help" output in groups. Uncommon porcelain commands must not
+# specify any of these attributes.
+#
+# "complete" attribute is used to mark that the command should be
+# completable by git-completion.bash. Note that by default,
+# mainporcelain commands are completable so you don't need this
+# attribute.
+#
+# While not true commands, guides are also specified here, which can
+# only have "guide" attribute and nothing else.
+#
 ### command list (do not change this line, also do not change alignment)
 # command name  category [category] [category]
 git-add mainporcelain   worktree
-- 
2.17.0.705.g3525833791

[PATCH v6 10/13] help: use command-list.txt for the source of guides

[Nguyễn Thái Ngọc Duy]

The help command currently hard codes the list of guides and their
summary in C. Let's move this list to command-list.txt. This lets us
extract summary lines from Documentation/git*.txt. This also
potentially lets us list guides in git.txt, but I'll leave that for
now.
---
 Documentation/gitattributes.txt|  2 +-
 Documentation/gitmodules.txt   |  2 +-
 Documentation/gitrevisions.txt |  2 +-
 Makefile   |  2 +-
 builtin/help.c | 32 --
 command-list.txt   | 16 +
 contrib/completion/git-completion.bash | 15 
 help.c | 21 +
 help.h |  1 +
 t/t0012-help.sh|  6 +
 10 files changed, 54 insertions(+), 45 deletions(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 1094fe2b5b..083c2f380d 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -3,7 +3,7 @@ gitattributes(5)
 
 NAME
 
-gitattributes - defining attributes per path
+gitattributes - Defining attributes per path
 
 SYNOPSIS
 
diff --git a/Documentation/gitmodules.txt b/Documentation/gitmodules.txt
index db5d47eb19..4d63def206 100644
--- a/Documentation/gitmodules.txt
+++ b/Documentation/gitmodules.txt
@@ -3,7 +3,7 @@ gitmodules(5)
 
 NAME
 
-gitmodules - defining submodule properties
+gitmodules - Defining submodule properties
 
 SYNOPSIS
 
diff --git a/Documentation/gitrevisions.txt b/Documentation/gitrevisions.txt
index 27dec5b91d..1f6cceaefb 100644
--- a/Documentation/gitrevisions.txt
+++ b/Documentation/gitrevisions.txt
@@ -3,7 +3,7 @@ gitrevisions(7)
 
 NAME
 
-gitrevisions - specifying revisions and ranges for Git
+gitrevisions - Specifying revisions and ranges for Git
 
 SYNOPSIS
 
diff --git a/Makefile b/Makefile
index a60a78ee67..1efb751e46 100644
--- a/Makefile
+++ b/Makefile
@@ -1937,7 +1937,7 @@ $(BUILT_INS): git$X
 
 command-list.h: generate-cmdlist.sh command-list.txt
 
-command-list.h: $(wildcard Documentation/git-*.txt)
+command-list.h: $(wildcard Documentation/git*.txt)
$(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh command-list.txt >$@+ 
&& mv $@+ $@
 
 SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
diff --git a/builtin/help.c b/builtin/help.c
index 0e0af8426a..5727fb5e51 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -402,38 +402,6 @@ static void show_html_page(const char *git_cmd)
open_html(page_path.buf);
 }
 
-static struct {
-   const char *name;
-   const char *help;
-} common_guides[] = {
-   { "attributes", N_("Defining attributes per path") },
-   { "everyday", N_("Everyday Git With 20 Commands Or So") },
-   { "glossary", N_("A Git glossary") },
-   { "ignore", N_("Specifies intentionally untracked files to ignore") },
-   { "modules", N_("Defining submodule properties") },
-   { "revisions", N_("Specifying revisions and ranges for Git") },
-   { "tutorial", N_("A tutorial introduction to Git (for version 1.5.1 or 
newer)") },
-   { "workflows", N_("An overview of recommended workflows with Git") },
-};
-
-static void list_common_guides_help(void)
-{
-   int i, longest = 0;
-
-   for (i = 0; i < ARRAY_SIZE(common_guides); i++) {
-   if (longest < strlen(common_guides[i].name))
-   longest = strlen(common_guides[i].name);
-   }
-
-   puts(_("The common Git guides are:\n"));
-   for (i = 0; i < ARRAY_SIZE(common_guides); i++) {
-   printf("   %s   ", common_guides[i].name);
-   mput_char(' ', longest - strlen(common_guides[i].name));
-   puts(_(common_guides[i].help));
-   }
-   putchar('\n');
-}
-
 static const char *check_git_cmd(const char* cmd)
 {
char *alias;
diff --git a/command-list.txt b/command-list.txt
index 3bd23201a6..99ddc231c1 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -139,3 +139,19 @@ gitweb  
ancillaryinterrogators
 git-whatchanged ancillaryinterrogators
 git-worktreemainporcelain
 git-write-tree  plumbingmanipulators
+gitattributes   guide
+gitcli  guide
+gitcore-tutorialguide
+gitcvs-migrationguide
+gitdiffcore guide
+giteveryday guide
+gitglossary guide
+githooksguide
+gitignore   guide
+gitmodules  guide
+gitnamespaces   guide
+gitrepository-layoutguide
+gitrevisionsguide
+gittutorial-2   guide

[PATCH v6 13/13] completion: allow to customize the completable command list

[Nguyễn Thái Ngọc Duy]

By default we show porcelain, external commands and a couple others
that are also popular. If you are not happy with this list, you can
now customize it. See the big comment block for details.

PS. perhaps I should make aliases a group too, which makes it possible
to _not_ complete aliases by omitting this special group in
$GIT_COMPLETION_CMD_GROUPS
---
 contrib/completion/git-completion.bash | 35 ++
 git.c  |  2 ++
 help.c | 33 
 help.h |  1 +
 4 files changed, 71 insertions(+)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 908692ea52..0fd29803d5 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -38,6 +38,38 @@
 #
 # When set to "1", do not include "DWIM" suggestions in git-checkout
 # completion (e.g., completing "foo" when "origin/foo" exists).
+#
+#   GIT_COMPLETION_CMD_GROUPS
+#
+# When set, "git --list-cmds=$GIT_COMPLETION_CMD_GROUPS" will be
+# used to get the list of completable commands. The default is
+# "mainporcelain,others,list-complete" (in English: all porcelain
+# commands and external ones are included. Certain non-porcelain
+# commands are also marked for completion in command-list.txt).
+# You could for example complete all commands with
+#
+# GIT_COMPLETION_CMD_GROUPS=main,others
+#
+# Or you could go with defaults add some extra commands specified
+# in the configuration variable completion.commands [1] with
+#
+# GIT_COMPLETION_CMD_GROUPS=mainporcelain,others,list-complete,config
+#
+# Or go completely custom group with
+#
+# GIT_COMPLETION_CMD_GROUPS=config
+#
+# Or you could even play with other command categories found in
+# command-list.txt.
+#
+# [1] Note that completion.commands should not be per-repository
+# since the command list is generated once and cached.
+#
+# completion.commands could be used to exclude commands as
+# well.  If a command in this list begins with '-', then it
+# will be excluded from the list of commands gathered by the
+# groups specified before "config" in
+# $GIT_COMPLETION_CMD_GROUPS.
 
 case "$COMP_WORDBREAKS" in
 *:*) : great ;;
@@ -840,6 +872,9 @@ __git_commands () {
if test -n "$GIT_TESTING_PORCELAIN_COMMAND_LIST"
then
printf "%s" "$GIT_TESTING_PORCELAIN_COMMAND_LIST"
+   elif test -n "$GIT_COMPLETION_CMD_GROUPS"
+   then
+   git --list-cmds="$GIT_COMPLETION_CMD_GROUPS"
else
git --list-cmds=list-mainporcelain,others,list-complete
fi
diff --git a/git.c b/git.c
index 67f3e20ae9..fd08911e11 100644
--- a/git.c
+++ b/git.c
@@ -53,6 +53,8 @@ static int list_cmds(const char *spec)
list_all_main_cmds();
else if (len == 6 && !strncmp(spec, "others", 6))
list_all_other_cmds();
+   else if (len == 6 && !strncmp(spec, "config", 6))
+   list_cmds_by_config();
else if (len > 5 && !strncmp(spec, "list-", 5)) {
struct strbuf sb = STRBUF_INIT;
 
diff --git a/help.c b/help.c
index 23924dd300..abf87205b2 100644
--- a/help.c
+++ b/help.c
@@ -366,6 +366,39 @@ void list_cmds_by_category(struct string_list *list,
}
 }
 
+void list_cmds_by_config(struct string_list *list)
+{
+   const char *cmd_list;
+
+   /*
+* There's no actual repository setup at this point (and even
+* if there is, we don't really care; only global config
+* matters). If we accidentally set up a repository, it's ok
+* too since the caller (git --list-cmds=) should exit shortly
+* anyway.
+*/
+   if (git_config_get_string_const("completion.commands", _list))
+   return;
+
+   string_list_sort(list);
+   string_list_remove_duplicates(list, 0);
+
+   while (*cmd_list) {
+   struct strbuf sb = STRBUF_INIT;
+   const char *p = strchrnul(cmd_list, ' ');
+
+   strbuf_add(, cmd_list, p - cmd_list);
+   if (*cmd_list == '-')
+   string_list_remove(list, cmd_list + 1, 0);
+   else
+   string_list_insert(list, sb.buf);
+   strbuf_release();
+   while (*p == ' ')
+   p++;
+   cmd_list = p;
+   }
+}
+
 void list_common_guides_help(void)
 {
struct category_description catdesc[] = {
diff --git a/help.h b/help.h
index b2293e99be..3b38292a1b 100644
--- a/help.h
+++ b/help.h
@@ -26,6 +26,7 @@ extern void list_all_main_cmds(struct string_list *list);
 extern void list_all_other_cmds(struct string_list 

[PATCH v6 12/13] completion: let git provide the completable command list

[Nguyễn Thái Ngọc Duy]

Instead of maintaining a separate list of command classification,
which often could go out of date, let's centralize the information
back in git.

While the function in git-completion.bash implies "list porcelain
commands", that's not exactly what it does. It gets all commands (aka
--list-cmds=main,others) then exclude certain non-porcelain ones. We
could almost recreate this list two lists list-mainporcelain and
others. The non-porcelain-but-included-anyway is added by the third
category list-complete.

list-complete does not recreate exactly the command list before this
patch though. The following commands are not part of neither
list-mainporcelain nor list-complete and as a result no longer
completes:

- annotate obsolete, discouraged to use
- difftool-helper  not an end user command
- filter-branchnot often used
- get-tar-commit-idnot often used
- imap-sendnot often used
- interpreter-trailers not for interactive use
- lost-found   obsolete
- p4   too short and probably not often used (*)
- peek-remote  deprecated
- svn  same category as p4 (*)
- tar-tree obsolete
- verify-commitnot often used

Note that the current completion script incorrectly classifies
filter-branch as porcelain and t9902 tests this behavior. We keep it
this way in t9902 because this test does not really care which
particular command is porcelain or plubmbing, they're just names.

(*) to be fair, send-email command which is in the same
foreignscminterface group as svn and p4 does get completion, just
because it's used by git and kernel development. So maybe should
include them.
---
 command-list.txt   |  37 
 contrib/completion/git-completion.bash | 117 ++---
 t/t9902-completion.sh  |   5 +-
 3 files changed, 48 insertions(+), 111 deletions(-)

diff --git a/command-list.txt b/command-list.txt
index 9c70c69193..3e21ddfcfb 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -47,11 +47,11 @@
 git-add mainporcelain   worktree
 git-am  mainporcelain
 git-annotateancillaryinterrogators
-git-apply   plumbingmanipulators
+git-apply   plumbingmanipulators
complete
 git-archimport  foreignscminterface
 git-archive mainporcelain
 git-bisect  mainporcelain   info
-git-blame   ancillaryinterrogators
+git-blame   ancillaryinterrogators  
complete
 git-branch  mainporcelain   history
 git-bundle  mainporcelain
 git-cat-fileplumbinginterrogators
@@ -61,7 +61,7 @@ git-check-mailmap   purehelpers
 git-checkoutmainporcelain   history
 git-checkout-index  plumbingmanipulators
 git-check-ref-formatpurehelpers
-git-cherry  ancillaryinterrogators
+git-cherry  ancillaryinterrogators  
complete
 git-cherry-pick mainporcelain
 git-citool  mainporcelain
 git-clean   mainporcelain
@@ -69,7 +69,7 @@ git-clone   mainporcelain 
  init
 git-column  purehelpers
 git-commit  mainporcelain   history
 git-commit-tree plumbingmanipulators
-git-config  ancillarymanipulators
+git-config  ancillarymanipulators   
complete
 git-count-objects   ancillaryinterrogators
 git-credential  purehelpers
 git-credential-cachepurehelpers
@@ -83,7 +83,7 @@ git-diffmainporcelain 
  history
 git-diff-files  plumbinginterrogators
 git-diff-index  plumbinginterrogators
 git-diff-tree   plumbinginterrogators
-git-difftoolancillaryinterrogators
+git-difftoolancillaryinterrogators  
complete
 git-fast-export ancillarymanipulators
 git-fast-import ancillarymanipulators
 git-fetch   mainporcelain   remote
@@ -92,20 +92,20 @@ git-filter-branch   
ancillarymanipulators
 git-fmt-merge-msg   purehelpers
 git-for-each-refplumbinginterrogators
 git-format-patchmainporcelain
-git-fsck   

[PATCH v6 02/13] generate-cmds.sh: export all commands to command-list.h

[Nguyễn Thái Ngọc Duy]

The current generate-cmds.sh generates just enough to print "git help"
output. That is, it only extracts help text for common commands.

The script is now updated to extract help text for all commands and
keep command classification a new file, command-list.h. This will be
useful later:

- "git help -a" could print a short summary of all commands instead of
  just the common ones.

- "git" could produce a list of commands of one or more category. One
  of its use is to reduce another command classification embedded in
  git-completion.bash.

The new file can be generated but is not used anywhere yet. The plan
is we migrate away from common-cmds.h. Then we can kill off
common-cmds.h build rules and generation code (and also delete
duplicate content in command-list.h which we keep for now to not mess
generate-cmds.sh up too much).

PS. The new fixed column requirement on command-list.txt is
technically not needed. But it helps simplify the code a bit at this
stage. We could lift this restriction later if we want to.
---
 .gitignore  |  1 +
 Makefile| 13 ++---
 command-list.txt|  4 +--
 generate-cmdlist.sh | 67 ++---
 4 files changed, 75 insertions(+), 10 deletions(-)

diff --git a/.gitignore b/.gitignore
index 833ef3b0b7..d4c3914167 100644
--- a/.gitignore
+++ b/.gitignore
@@ -180,6 +180,7 @@
 /gitweb/static/gitweb.js
 /gitweb/static/gitweb.min.*
 /common-cmds.h
+/command-list.h
 *.tar.gz
 *.dsc
 *.deb
diff --git a/Makefile b/Makefile
index f181687250..2a8913ea21 100644
--- a/Makefile
+++ b/Makefile
@@ -757,7 +757,7 @@ LIB_FILE = libgit.a
 XDIFF_LIB = xdiff/lib.a
 VCSSVN_LIB = vcs-svn/lib.a
 
-GENERATED_H += common-cmds.h
+GENERATED_H += common-cmds.h command-list.h
 
 LIB_H = $(shell $(FIND) . \
-name .git -prune -o \
@@ -1938,6 +1938,11 @@ $(BUILT_INS): git$X
 common-cmds.h: generate-cmdlist.sh command-list.txt
 
 common-cmds.h: $(wildcard Documentation/git-*.txt)
+   $(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh command-list.txt COMMON 
>$@+ && mv $@+ $@
+
+command-list.h: generate-cmdlist.sh command-list.txt
+
+command-list.h: $(wildcard Documentation/git-*.txt)
$(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh command-list.txt >$@+ 
&& mv $@+ $@
 
 SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
@@ -2148,7 +2153,7 @@ else
 # Dependencies on header files, for platforms that do not support
 # the gcc -MMD option.
 #
-# Dependencies on automatically generated headers such as common-cmds.h
+# Dependencies on automatically generated headers such as common-cmds.h or 
command-list.h
 # should _not_ be included here, since they are necessary even when
 # building an object for the first time.
 
@@ -2527,7 +2532,7 @@ sparse: $(SP_OBJ)
 style:
git clang-format --style file --diff --extensions c,h
 
-check: common-cmds.h
+check: common-cmds.h command-list.h
@if sparse; \
then \
echo >&2 "Use 'make sparse' instead"; \
@@ -2775,7 +2780,7 @@ clean: profile-clean coverage-clean
$(RM) $(TEST_PROGRAMS) $(NO_INSTALL)
$(RM) -r bin-wrappers $(dep_dirs)
$(RM) -r po/build/
-   $(RM) *.pyc *.pyo */*.pyc */*.pyo common-cmds.h $(ETAGS_TARGET) tags 
cscope*
+   $(RM) *.pyc *.pyo */*.pyc */*.pyo common-cmds.h command-list.h 
$(ETAGS_TARGET) tags cscope*
$(RM) -r $(GIT_TARNAME) .doc-tmp-dir
$(RM) $(GIT_TARNAME).tar.gz git-core_$(GIT_VERSION)-*.tar.gz
$(RM) $(htmldocs).tar.gz $(manpages).tar.gz
diff --git a/command-list.txt b/command-list.txt
index a1fad28fd8..786536aba0 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -8,8 +8,8 @@ info examine the history and state (see also: git help 
revisions)
 history  grow, mark and tweak your common history
 remote   collaborate (see also: git help workflows)
 
-### command list (do not change this line)
-# command name  category [deprecated] [common]
+### command list (do not change this line, also do not change alignment)
+# command name  category [category] [category]
 git-add mainporcelain   worktree
 git-am  mainporcelain
 git-annotateancillaryinterrogators
diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
index 31b6d886cb..c9fd524760 100755
--- a/generate-cmdlist.sh
+++ b/generate-cmdlist.sh
@@ -1,5 +1,27 @@
 #!/bin/sh
 
+die () {
+   echo "$@" >&2
+   exit 1
+}
+
+command_list () {
+   sed '1,/^### command list/d;/^#/d' "$1"
+}
+
+get_categories() {
+   tr ' ' '\n'|
+   grep -v '^$' |
+   sort |
+   uniq
+}
+
+category_list () {
+   command_list "$1" |
+   cut -c 40- |
+   get_categories
+}
+
 get_synopsis () {
sed -n '
/^NAME/,/'"$1"'/H
@@ -10,14 +32,51 @@ get_synopsis () {
}' "Documentation/$1.txt"
 }
 
+define_categories() {
+   

[PATCH v6 04/13] Remove common-cmds.h

[Nguyễn Thái Ngọc Duy]

After the last patch, common-cmds.h is no longer used (and it was
actually broken). Remove all related code. command-list.h will take
its place from now on.
---
 .gitignore  |  1 -
 Makefile| 17 ++---
 generate-cmdlist.sh | 46 +++--
 3 files changed, 9 insertions(+), 55 deletions(-)

diff --git a/.gitignore b/.gitignore
index d4c3914167..0836083992 100644
--- a/.gitignore
+++ b/.gitignore
@@ -179,7 +179,6 @@
 /gitweb/gitweb.cgi
 /gitweb/static/gitweb.js
 /gitweb/static/gitweb.min.*
-/common-cmds.h
 /command-list.h
 *.tar.gz
 *.dsc
diff --git a/Makefile b/Makefile
index 5c58b0b692..a60a78ee67 100644
--- a/Makefile
+++ b/Makefile
@@ -757,7 +757,7 @@ LIB_FILE = libgit.a
 XDIFF_LIB = xdiff/lib.a
 VCSSVN_LIB = vcs-svn/lib.a
 
-GENERATED_H += common-cmds.h command-list.h
+GENERATED_H += command-list.h
 
 LIB_H = $(shell $(FIND) . \
-name .git -prune -o \
@@ -1914,9 +1914,9 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
$(filter %.o,$^) $(LIBS)
 
-help.sp help.s help.o: common-cmds.h command-list.h
+help.sp help.s help.o: command-list.h
 
-builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h command-list.h 
GIT-PREFIX
+builtin/help.sp builtin/help.s builtin/help.o: command-list.h GIT-PREFIX
 builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
@@ -1935,11 +1935,6 @@ $(BUILT_INS): git$X
ln -s $< $@ 2>/dev/null || \
cp $< $@
 
-common-cmds.h: generate-cmdlist.sh command-list.txt
-
-common-cmds.h: $(wildcard Documentation/git-*.txt)
-   $(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh command-list.txt COMMON 
>$@+ && mv $@+ $@
-
 command-list.h: generate-cmdlist.sh command-list.txt
 
 command-list.h: $(wildcard Documentation/git-*.txt)
@@ -2153,7 +2148,7 @@ else
 # Dependencies on header files, for platforms that do not support
 # the gcc -MMD option.
 #
-# Dependencies on automatically generated headers such as common-cmds.h or 
command-list.h
+# Dependencies on automatically generated headers such as command-list.h
 # should _not_ be included here, since they are necessary even when
 # building an object for the first time.
 
@@ -2532,7 +2527,7 @@ sparse: $(SP_OBJ)
 style:
git clang-format --style file --diff --extensions c,h
 
-check: common-cmds.h command-list.h
+check: command-list.h
@if sparse; \
then \
echo >&2 "Use 'make sparse' instead"; \
@@ -2780,7 +2775,7 @@ clean: profile-clean coverage-clean
$(RM) $(TEST_PROGRAMS) $(NO_INSTALL)
$(RM) -r bin-wrappers $(dep_dirs)
$(RM) -r po/build/
-   $(RM) *.pyc *.pyo */*.pyc */*.pyo common-cmds.h command-list.h 
$(ETAGS_TARGET) tags cscope*
+   $(RM) *.pyc *.pyo */*.pyc */*.pyo command-list.h $(ETAGS_TARGET) tags 
cscope*
$(RM) -r $(GIT_TARNAME) .doc-tmp-dir
$(RM) $(GIT_TARNAME).tar.gz git-core_$(GIT_VERSION)-*.tar.gz
$(RM) $(htmldocs).tar.gz $(manpages).tar.gz
diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
index 93de8e8f59..015eef2804 100755
--- a/generate-cmdlist.sh
+++ b/generate-cmdlist.sh
@@ -68,46 +68,6 @@ struct cmdname_help {
uint32_t category;
 };
 "
-if [ -z "$2" ]
-then
-   define_categories "$1"
-   echo
-   print_command_list "$1"
-   exit 0
-fi
-
-echo "static const char *common_cmd_groups[] = {"
-
-grps=grps$$.tmp
-match=match$$.tmp
-trap "rm -f '$grps' '$match'" 0 1 2 3 15
-
-sed -n '
-   1,/^### common groups/b
-   /^### command list/q
-   /^#/b
-   /^[ ]*$/b
-   h;s/^[^ ][^ ]*[ ][  ]*\(.*\)/   N_("\1"),/p
-   g;s/^\([^   ][^ ]*\)[   ].*/\1/w '$grps'
-   ' "$1"
-printf '};\n\n'
-
-n=0
-substnum=
-while read grp
-do
-   echo "^git-..*[ ]$grp"
-   substnum="$substnum${substnum:+;}s/[]$grp/$n/"
-   n=$(($n+1))
-done <"$grps" >"$match"
-
-printf 'static struct cmdname_help common_cmds[] = {\n'
-grep -f "$match" "$1" |
-sed 's/^git-//' |
-sort |
-while read cmd tags
-do
-   tag=$(echo "$tags" | sed "$substnum; s/[^0-9]//g")
-   echo "  {\"$cmd\", $(get_synopsis git-$cmd), $tag},"
-done
-echo "};"
+define_categories "$1"
+echo
+print_command_list "$1"
-- 
2.17.0.705.g3525833791

[PATCH v6 07/13] completion: implement and use --list-cmds=main,others

[Nguyễn Thái Ngọc Duy]

Instead of parsing "git help -a" output, which is tricky to get right,
less elegant and also slow, make git provide the list in a
machine-friendly form. This adds two separate listing types, main and
others, instead of just "all" for more flexibility.
---
 contrib/completion/git-completion.bash |  2 +-
 git.c  |  4 
 help.c | 32 ++
 help.h |  4 
 4 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 3556838759..62ca8641f4 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -839,7 +839,7 @@ __git_commands () {
then
printf "%s" "${GIT_TESTING_COMMAND_COMPLETION}"
else
-   git help -a|egrep '^  [a-zA-Z0-9]'
+   git --list-cmds=main,others
fi
 }
 
diff --git a/git.c b/git.c
index a0477c459e..3c032d01fc 100644
--- a/git.c
+++ b/git.c
@@ -49,6 +49,10 @@ static int list_cmds(const char *spec)
 
if (len == 8 && !strncmp(spec, "builtins", 8))
list_builtins(, 0);
+   else if (len == 4 && !strncmp(spec, "main", 4))
+   list_all_main_cmds();
+   else if (len == 6 && !strncmp(spec, "others", 6))
+   list_all_other_cmds();
else
die(_("unsupported command listing type '%s'"), spec);
spec += len;
diff --git a/help.c b/help.c
index 2d6a3157f8..d5ce9dfcbb 100644
--- a/help.c
+++ b/help.c
@@ -297,6 +297,38 @@ void list_common_cmds_help(void)
print_cmd_by_category(common_categories);
 }
 
+void list_all_main_cmds(struct string_list *list)
+{
+   struct cmdnames main_cmds, other_cmds;
+   int i;
+
+   memset(_cmds, 0, sizeof(main_cmds));
+   memset(_cmds, 0, sizeof(other_cmds));
+   load_command_list("git-", _cmds, _cmds);
+
+   for (i = 0; i < main_cmds.cnt; i++)
+   string_list_append(list, main_cmds.names[i]->name);
+
+   clean_cmdnames(_cmds);
+   clean_cmdnames(_cmds);
+}
+
+void list_all_other_cmds(struct string_list *list)
+{
+   struct cmdnames main_cmds, other_cmds;
+   int i;
+
+   memset(_cmds, 0, sizeof(main_cmds));
+   memset(_cmds, 0, sizeof(other_cmds));
+   load_command_list("git-", _cmds, _cmds);
+
+   for (i = 0; i < other_cmds.cnt; i++)
+   string_list_append(list, other_cmds.names[i]->name);
+
+   clean_cmdnames(_cmds);
+   clean_cmdnames(_cmds);
+}
+
 int is_in_cmdlist(struct cmdnames *c, const char *s)
 {
int i;
diff --git a/help.h b/help.h
index b21d7c94e8..97e6c0965e 100644
--- a/help.h
+++ b/help.h
@@ -1,6 +1,8 @@
 #ifndef HELP_H
 #define HELP_H
 
+struct string_list;
+
 struct cmdnames {
int alloc;
int cnt;
@@ -17,6 +19,8 @@ static inline void mput_char(char c, unsigned int num)
 }
 
 extern void list_common_cmds_help(void);
+extern void list_all_main_cmds(struct string_list *list);
+extern void list_all_other_cmds(struct string_list *list);
 extern const char *help_unknown_cmd(const char *cmd);
 extern void load_command_list(const char *prefix,
  struct cmdnames *main_cmds,
-- 
2.17.0.705.g3525833791

[PATCH v6 05/13] git.c: convert --list-* to --list-cmds=*

[Nguyễn Thái Ngọc Duy]

Even if these are hidden options, let's make them a bit more generic
since we're introducing more listing types shortly. The code is
structured to allow combining multiple listing types together because
we will soon add more types the 'builtins'.

'parseopt' remains separate because it has separate (SPC) to match
git-completion.bash needs and will not combine with others.
---
 contrib/completion/git-completion.bash |  2 +-
 git.c  | 30 --
 t/t0012-help.sh|  2 +-
 3 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index a757073945..3556838759 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3049,7 +3049,7 @@ __git_complete_common () {
 __git_cmds_with_parseopt_helper=
 __git_support_parseopt_helper () {
test -n "$__git_cmds_with_parseopt_helper" ||
-   __git_cmds_with_parseopt_helper="$(__git 
--list-parseopt-builtins)"
+   __git_cmds_with_parseopt_helper="$(__git --list-cmds=parseopt)"
 
case " $__git_cmds_with_parseopt_helper " in
*" $1 "*)
diff --git a/git.c b/git.c
index 3a89893712..b2842a22e2 100644
--- a/git.c
+++ b/git.c
@@ -38,6 +38,23 @@ static int use_pager = -1;
 
 static void list_builtins(unsigned int exclude_option, char sep);
 
+static int list_cmds(const char *spec)
+{
+   while (*spec) {
+   const char *sep = strchrnul(spec, ',');
+   int len = sep - spec;
+
+   if (len == 8 && !strncmp(spec, "builtins", 8))
+   list_builtins(0, '\n');
+   else
+   die(_("unsupported command listing type '%s'"), spec);
+   spec += len;
+   if (*spec == ',')
+   spec++;
+   }
+   return 0;
+}
+
 static void commit_pager_choice(void) {
switch (use_pager) {
case 0:
@@ -223,12 +240,13 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
}
(*argv)++;
(*argc)--;
-   } else if (!strcmp(cmd, "--list-builtins")) {
-   list_builtins(0, '\n');
-   exit(0);
-   } else if (!strcmp(cmd, "--list-parseopt-builtins")) {
-   list_builtins(NO_PARSEOPT, ' ');
-   exit(0);
+   } else if (skip_prefix(cmd, "--list-cmds=", )) {
+   if (!strcmp(cmd, "parseopt")) {
+   list_builtins(NO_PARSEOPT, ' ');
+   exit(0);
+   } else {
+   exit(list_cmds(cmd));
+   }
} else {
fprintf(stderr, _("unknown option: %s\n"), cmd);
usage(git_usage_string);
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index c096f33505..3c91a9024a 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -59,7 +59,7 @@ test_expect_success 'git help' '
 '
 
 test_expect_success 'generate builtin list' '
-   git --list-builtins >builtins
+   git --list-cmds=builtins >builtins
 '
 
 while read builtin
-- 
2.17.0.705.g3525833791

[PATCH v6 06/13] git --list-cmds: collect command list in a string_list

[Nguyễn Thái Ngọc Duy]

Instead of printing the command directly one by one, keep them in a
list and print at the end. This allows more modification before we
print out (e.g. sorting, removing duplicates or even excluding some
items).
---
 git.c | 22 +-
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/git.c b/git.c
index b2842a22e2..a0477c459e 100644
--- a/git.c
+++ b/git.c
@@ -36,22 +36,28 @@ const char git_more_info_string[] =
 
 static int use_pager = -1;
 
-static void list_builtins(unsigned int exclude_option, char sep);
+static void list_builtins(struct string_list *list, unsigned int 
exclude_option);
 
 static int list_cmds(const char *spec)
 {
+   struct string_list list = STRING_LIST_INIT_DUP;
+   int i;
+
while (*spec) {
const char *sep = strchrnul(spec, ',');
int len = sep - spec;
 
if (len == 8 && !strncmp(spec, "builtins", 8))
-   list_builtins(0, '\n');
+   list_builtins(, 0);
else
die(_("unsupported command listing type '%s'"), spec);
spec += len;
if (*spec == ',')
spec++;
}
+   for (i = 0; i < list.nr; i++)
+   puts(list.items[i].string);
+   string_list_clear(, 0);
return 0;
 }
 
@@ -242,7 +248,13 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
(*argc)--;
} else if (skip_prefix(cmd, "--list-cmds=", )) {
if (!strcmp(cmd, "parseopt")) {
-   list_builtins(NO_PARSEOPT, ' ');
+   struct string_list list = STRING_LIST_INIT_DUP;
+   int i;
+
+   list_builtins(, NO_PARSEOPT);
+   for (i = 0; i < list.nr; i++)
+   printf("%s ", list.items[i].string);
+   string_list_clear(, 0);
exit(0);
} else {
exit(list_cmds(cmd));
@@ -526,14 +538,14 @@ int is_builtin(const char *s)
return !!get_builtin(s);
 }
 
-static void list_builtins(unsigned int exclude_option, char sep)
+static void list_builtins(struct string_list *out, unsigned int exclude_option)
 {
int i;
for (i = 0; i < ARRAY_SIZE(commands); i++) {
if (exclude_option &&
(commands[i].option & exclude_option))
continue;
-   printf("%s%c", commands[i].cmd, sep);
+   string_list_append(out, commands[i].cmd);
}
 }
 
-- 
2.17.0.705.g3525833791

[PATCH v6 09/13] help: add "-a --verbose" to list all commands with synopsis

[Nguyễn Thái Ngọc Duy]

This lists all recognized commands [1] by category. The group order
follows closely git.txt.

[1] We may actually show commands that are not built (e.g. if you set
NO_PERL you don't have git-instaweb but it's still listed here). I
ignore the problem because on Linux a git package could be split
anyway. The "git-core" package may not contain git-instaweb even if
it's built because it may end up in a separate package. We can't know
anyway.
---
 Documentation/git-help.txt |  4 +++-
 builtin/help.c |  7 +++
 help.c | 16 
 help.h |  2 ++
 t/t0012-help.sh|  9 +
 5 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index 40d328a4b3..a40fc38d8b 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt
@@ -8,7 +8,7 @@ git-help - Display help information about Git
 SYNOPSIS
 
 [verse]
-'git help' [-a|--all] [-g|--guide]
+'git help' [-a|--all [--verbose]] [-g|--guide]
   [-i|--info|-m|--man|-w|--web] [COMMAND|GUIDE]
 
 DESCRIPTION
@@ -42,6 +42,8 @@ OPTIONS
 --all::
Prints all the available commands on the standard output. This
option overrides any given command or guide name.
+   When used with `--verbose` print description for all recognized
+   commands.
 
 -g::
 --guides::
diff --git a/builtin/help.c b/builtin/help.c
index 598867cfea..0e0af8426a 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -36,6 +36,7 @@ static const char *html_path;
 
 static int show_all = 0;
 static int show_guides = 0;
+static int verbose;
 static unsigned int colopts;
 static enum help_format help_format = HELP_FORMAT_NONE;
 static int exclude_guides;
@@ -48,6 +49,7 @@ static struct option builtin_help_options[] = {
HELP_FORMAT_WEB),
OPT_SET_INT('i', "info", _format, N_("show info page"),
HELP_FORMAT_INFO),
+   OPT__VERBOSE(, N_("print command description")),
OPT_END(),
 };
 
@@ -463,6 +465,11 @@ int cmd_help(int argc, const char **argv, const char 
*prefix)
 
if (show_all) {
git_config(git_help_config, NULL);
+   if (verbose) {
+   setup_pager();
+   list_all_cmds_help();
+   return 0;
+   }
printf(_("usage: %s%s"), _(git_usage_string), "\n\n");
load_command_list("git-", _cmds, _cmds);
list_commands(colopts, _cmds, _cmds);
diff --git a/help.c b/help.c
index 1117f7d1d1..c7df1d2338 100644
--- a/help.c
+++ b/help.c
@@ -27,6 +27,17 @@ static struct category_description common_categories[] = {
{ CAT_remote, N_("collaborate (see also: git help workflows)") },
{ 0, NULL }
 };
+static struct category_description main_categories[] = {
+   { CAT_mainporcelain, N_("Main Porcelain Commands") },
+   { CAT_ancillarymanipulators, N_("Ancillary Commands / Manipulators") },
+   { CAT_ancillaryinterrogators, N_("Ancillary Commands / Interrogators") 
},
+   { CAT_foreignscminterface, N_("Interacting with Others") },
+   { CAT_plumbingmanipulators, N_("Low-level Commands / Manipulators") },
+   { CAT_plumbinginterrogators, N_("Low-level Commands / Interrogators") },
+   { CAT_synchingrepositories, N_("Low-level Commands / Synching 
Repositories") },
+   { CAT_purehelpers, N_("Low-level Commands / Internal Helpers") },
+   { 0, NULL }
+};
 
 static const char *drop_prefix(const char *name)
 {
@@ -352,6 +363,11 @@ void list_cmds_by_category(struct string_list *list,
}
 }
 
+void list_all_cmds_help(void)
+{
+   print_cmd_by_category(main_categories);
+}
+
 int is_in_cmdlist(struct cmdnames *c, const char *s)
 {
int i;
diff --git a/help.h b/help.h
index 734bba32d3..40917fc38c 100644
--- a/help.h
+++ b/help.h
@@ -19,6 +19,8 @@ static inline void mput_char(char c, unsigned int num)
 }
 
 extern void list_common_cmds_help(void);
+extern void list_all_cmds_help(void);
+
 extern void list_all_main_cmds(struct string_list *list);
 extern void list_all_other_cmds(struct string_list *list);
 extern void list_cmds_by_category(struct string_list *list,
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 3c91a9024a..060df24c2d 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
+'
+
 test_expect_success "works for commands and guides by default" '
configure_help &&
git help status &&
-- 
2.17.0.705.g3525833791

[PATCH v6 00/13] Keep all info in command-list.txt in git binary

[Nguyễn Thái Ngọc Duy]

v6 is now "feature complete". v6 adds

- documentation in command-list.txt so people know how to update it
- support for config key completion.commands, which consists
  of extra commands. It could also be used for excluding some
  commands.

Interdiff

diff --git a/command-list.txt b/command-list.txt
index 40776b9587..3e21ddfcfb 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -1,3 +1,47 @@
+# Command classification list
+# ---
+# All supported commands, builtin or external, must be described in
+# here. This info is used to list commands in various places. Each
+# command is on one line followed by one or more attributes.
+#
+# The first attribute group is mandatory and indicates the command
+# type. This group includes:
+#
+#   mainporcelain
+#   ancillarymanipulators
+#   ancillaryinterrogators
+#   foreignscminterface
+#   plumbingmanipulators
+#   plumbinginterrogators
+#   synchingrepositories
+#   synchelpers
+#   purehelpers
+#
+# The type names are self explanatory. But if you want to see what
+# command belongs to what group to get a better picture, have a look
+# at "git" man page, "GIT COMMANDS" section.
+#
+# Commands of type mainporcelain can also optionally have one of these
+# attributes:
+#
+#   init
+#   worktree
+#   info
+#   history
+#   remote
+#
+# These commands are considered "common" and will show up in "git
+# help" output in groups. Uncommon porcelain commands must not
+# specify any of these attributes.
+#
+# "complete" attribute is used to mark that the command should be
+# completable by git-completion.bash. Note that by default,
+# mainporcelain commands are completable so you don't need this
+# attribute.
+#
+# While not true commands, guides are also specified here, which can
+# only have "guide" attribute and nothing else.
+#
 ### command list (do not change this line, also do not change alignment)
 # command name  category [category] [category]
 git-add mainporcelain   worktree
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 908692ea52..0fd29803d5 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -38,6 +38,38 @@
 #
 # When set to "1", do not include "DWIM" suggestions in git-checkout
 # completion (e.g., completing "foo" when "origin/foo" exists).
+#
+#   GIT_COMPLETION_CMD_GROUPS
+#
+# When set, "git --list-cmds=$GIT_COMPLETION_CMD_GROUPS" will be
+# used to get the list of completable commands. The default is
+# "mainporcelain,others,list-complete" (in English: all porcelain
+# commands and external ones are included. Certain non-porcelain
+# commands are also marked for completion in command-list.txt).
+# You could for example complete all commands with
+#
+# GIT_COMPLETION_CMD_GROUPS=main,others
+#
+# Or you could go with defaults add some extra commands specified
+# in the configuration variable completion.commands [1] with
+#
+# GIT_COMPLETION_CMD_GROUPS=mainporcelain,others,list-complete,config
+#
+# Or go completely custom group with
+#
+# GIT_COMPLETION_CMD_GROUPS=config
+#
+# Or you could even play with other command categories found in
+# command-list.txt.
+#
+# [1] Note that completion.commands should not be per-repository
+# since the command list is generated once and cached.
+#
+# completion.commands could be used to exclude commands as
+# well.  If a command in this list begins with '-', then it
+# will be excluded from the list of commands gathered by the
+# groups specified before "config" in
+# $GIT_COMPLETION_CMD_GROUPS.
 
 case "$COMP_WORDBREAKS" in
 *:*) : great ;;
@@ -840,6 +872,9 @@ __git_commands () {
if test -n "$GIT_TESTING_PORCELAIN_COMMAND_LIST"
then
printf "%s" "$GIT_TESTING_PORCELAIN_COMMAND_LIST"
+   elif test -n "$GIT_COMPLETION_CMD_GROUPS"
+   then
+   git --list-cmds="$GIT_COMPLETION_CMD_GROUPS"
else
git --list-cmds=list-mainporcelain,others,list-complete
fi
diff --git a/git.c b/git.c
index 1c8b0c93e1..fd08911e11 100644
--- a/git.c
+++ b/git.c
@@ -36,27 +36,30 @@ const char git_more_info_string[] =
 
 static int use_pager = -1;
 
-static void list_builtins(unsigned int exclude_option, char sep);
+static void list_builtins(struct string_list *list, unsigned int 
exclude_option);
 
 static int list_cmds(const char *spec)
 {
+   struct string_list list = STRING_LIST_INIT_DUP;
+   int i;
+
while (*spec) {
const char *sep = strchrnul(spec, ',');
int len = sep - spec;
 
if (len == 8 && !strncmp(spec, "builtins", 8))
-   list_builtins(0, '\n');
-   else if (len == 8 && 

[PATCH v6 01/13] generate-cmds.sh: factor out synopsis extract code

[Nguyễn Thái Ngọc Duy]

This makes it easier to reuse the same code in another place (very
soon).
---
 generate-cmdlist.sh | 18 +++---
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
index eeea4b67ea..31b6d886cb 100755
--- a/generate-cmdlist.sh
+++ b/generate-cmdlist.sh
@@ -1,5 +1,15 @@
 #!/bin/sh
 
+get_synopsis () {
+   sed -n '
+   /^NAME/,/'"$1"'/H
+   ${
+   x
+   s/.*'"$1"' - \(.*\)/N_("\1")/
+   p
+   }' "Documentation/$1.txt"
+}
+
 echo "/* Automatically generated by generate-cmdlist.sh */
 struct cmdname_help {
char name[16];
@@ -39,12 +49,6 @@ sort |
 while read cmd tags
 do
tag=$(echo "$tags" | sed "$substnum; s/[^0-9]//g")
-   sed -n '
-   /^NAME/,/git-'"$cmd"'/H
-   ${
-   x
-   s/.*git-'"$cmd"' - \(.*\)/  {"'"$cmd"'", N_("\1"), 
'$tag'},/
-   p
-   }' "Documentation/git-$cmd.txt"
+   echo "  {\"$cmd\", $(get_synopsis git-$cmd), $tag},"
 done
 echo "};"
-- 
2.17.0.705.g3525833791

Re: [PATCH v2 00/18] Add `branch-diff`, a `tbdiff` lookalike

[SZEDER Gábor]

> 2) Your completion commands for branch-diff will only complete one
> revision range, not two.  e.g.
> git branch-diff origin/master..my-topic@{2} origin/master..my-top
> won't complete "my-topic" as I'd expect.

It does complete two revision ranges, but if you want to look at
reflogs, then you must escape the opening curly brace.  I'm not sure
why, but apparently after the unescaped '{' Bash thinks that it's a
new command, and doesn't even call our completion functions anymore.
It's not specific to the completion of 'branch-diff', or even to our
completion script.  I don't think we can do anything about it.

Re: [PATCH v2 00/18] Add `branch-diff`, a `tbdiff` lookalike

[Elijah Newren]

Hi Dscho,

On Sat, May 5, 2018 at 1:03 PM, Johannes Schindelin
 wrote:
> Hi Elijah,
>
> On Fri, 4 May 2018, Elijah Newren wrote:
>

>> - tbdiff aligned output columns better when there were more than 9
>> patches (I'll comment more on patch 09/18)
>
> I added a new patch to align the patch numbers specifically. I considered
> squashing it into 9/18, but decided against it: it will make it easier to
> read through the rationale when calling `git annotate` on those lines.

Awesome, thanks.


>> Also, I don't have bash-completion for either tbdiff or branch-diff.
>> :-(  But I saw some discussion on the v1 patches about how this gets
>> handled...  :-)
>
> Oh? Does 18/18 not work for you?
> https://public-inbox.org/git/71698f11835311c103aae565a2a761d10f4676b9.1525448066.git.johannes.schinde...@gmx.de/


It looks like it does work, in part, there were just two issues:

1) I apparently wasn't using all the nice improvements from the
completion script in my locally built git, but was instead still using
the one associated with my system-installed (and much older) git.
(Oops, my bad.)


2) Your completion commands for branch-diff will only complete one
revision range, not two.  e.g.
git branch-diff origin/master..my-topic@{2} origin/master..my-top
won't complete "my-topic" as I'd expect.


Elijah

Re: [GSoC] A blog about 'git stash' project

[Paul-Sebastian Ungureanu]

Hello,

On 06.05.2018 16:22, Kaartic Sivaraam wrote:

The blog looks pretty well written. I also read your proposal. It also
seems to be pretty much well written. I like the way you explain things.
Particularly, you seem to be explaining the problem and the way you're
about to approach it well. The plan seems pretty good.


Thank you a lot!


I just thought of suggesting one thing which might possibly be
redundant. I think you're aware of the fact that the Git project has
Travis-CI builds enabled[1] which you could take advantage of to ensure
your changes pass in various text environments.

If you're interested in testing your changes (which I suspect you are),
you might also be interested in 'git-test'[2], a tool built by Michael
Haggerty. Unlike the Travis-CI tests which test only the tip of the
change, 'git-test' would help you ensure that every single commit you
make doesn't break the test suite (which is both a nice thing and is
expected here).


I heard of it and used it a couple of times (for the micro-project and 
some other patch).



Sorry for the off-topic info about the tests in this mail :-)


You shouldn't be sorry. Nothing was off-topic; I found everything you 
said to be helpful! Every feedback is welcomed!



Hope you're able to achieve your goal as planned and have a great time
during this summer of code!


Thank you one more time! I know it will be a good summer.

Best,
Paul Ungureanu

Re: What's cooking in git.git (May 2018, #01; Mon, 7)

[Elijah Newren]

Hi,

On Mon, May 7, 2018 at 8:20 AM, Derrick Stolee  wrote:
> On 5/7/2018 10:58 AM, Junio C Hamano wrote:

>>   Will merge to 'master'.
>
> These have been queued for master for a few weeks. Is anything delaying
> them?

I'm actually curious what the "Will merge to 'master'" and "Will merge
to 'next'" are intended to mean in general.  I thought it meant that
the topic would be merged the following week barring further
discussion, but that hasn't always been the case and a quick look at
origin/todo suggests it's not at all uncommon for my assumption to be
wrong -- but that leaves me wondering what the intent actually is.  In
particular, I'm curious if there is anything I'm unaware of that I
should be doing that I'm not but which would help make topics
(particularly the big ones) work more smoothly.

Thanks,
Elijah

Re: What's cooking in git.git (May 2018, #01; Mon, 7)

[Elijah Newren]

Hi Junio,

On Mon, May 7, 2018 at 7:58 AM, Junio C Hamano  wrote:
> * en/rename-directory-detection-reboot (2018-04-25) 36 commits
>  - merge-recursive: fix check for skipability of working tree updates
>  - merge-recursive: make "Auto-merging" comment show for other merges
>  - merge-recursive: fix remainder of was_dirty() to use original index
>  - merge-recursive: fix was_tracked() to quit lying with some renamed paths
>  - t6046: testcases checking whether updates can be skipped in a merge
>  - merge-recursive: avoid triggering add_cacheinfo error with dirty mod
>  - merge-recursive: move more is_dirty handling to merge_content
>  - merge-recursive: improve add_cacheinfo error handling
>  - merge-recursive: avoid spurious rename/rename conflict from dir renames
>  - directory rename detection: new testcases showcasing a pair of bugs
>  - merge-recursive: fix remaining directory rename + dirty overwrite cases
>  - merge-recursive: fix overwriting dirty files involved in renames
>  - merge-recursive: avoid clobbering untracked files with directory renames
>  - merge-recursive: apply necessary modifications for directory renames
>  - merge-recursive: when comparing files, don't include trees
>  - merge-recursive: check for file level conflicts then get new name
>  - merge-recursive: add computation of collisions due to dir rename & merging
>  - merge-recursive: check for directory level conflicts
>  - merge-recursive: add get_directory_renames()
>  - merge-recursive: make a helper function for cleanup for handle_renames
>  - merge-recursive: split out code for determining diff_filepairs
>  - merge-recursive: make !o->detect_rename codepath more obvious
>  - merge-recursive: fix leaks of allocated renames and diff_filepairs
>  - merge-recursive: introduce new functions to handle rename logic
>  - merge-recursive: move the get_renames() function
>  - directory rename detection: tests for handling overwriting dirty files
>  - directory rename detection: tests for handling overwriting untracked files
>  - directory rename detection: miscellaneous testcases to complete coverage
>  - directory rename detection: testcases exploring possibly suboptimal merges
>  - directory rename detection: more involved edge/corner testcases
>  - directory rename detection: testcases checking which side did the rename
>  - directory rename detection: files/directories in the way of some renames
>  - directory rename detection: partially renamed directory testcase/discussion
>  - directory rename detection: testcases to avoid taking detection too far
>  - directory rename detection: directory splitting testcases
>  - directory rename detection: basic testcases
>  (this branch is used by bp/merge-rename-config.)
>
>  Rename detection logic in "diff" family that is used in "merge" has
>  learned to guess when all of x/a, x/b and x/c have moved to z/a,
>  z/b and z/c, it is likely that x/d added in the meantime would also
>  want to move to z/d by taking the hint that the entire directory
>  'x' moved to 'z'.  Incidentally, this avoids updating a file in the
>  working tree after a (non-trivial) merge whose result matches what
>  our side originally had.

Thanks for adding a comment about the fix for the unnecessary update.
However, you've dropped a comment from the original release note about
the other fix this series also provides, namely, "A bug causing dirty
files involved in a rename to be overwritten during merge has also
been fixed as part of this work."  Was this intentional?

[PATCH] fixup! merge-recursive: add get_directory_renames()

[Elijah Newren]

---
 merge-recursive.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 5f42c677d5..9b9a4b8213 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1851,7 +1851,7 @@ static struct hashmap *get_directory_renames(struct 
diff_queue_struct *pairs,
 * renames, finding out how often each directory rename pair
 * possibility occurs.
 */
-   dir_renames = xmalloc(sizeof(struct hashmap));
+   dir_renames = xmalloc(sizeof(*dir_renames));
dir_rename_init(dir_renames);
for (i = 0; i < pairs->nr; ++i) {
struct string_list_item *item;
@@ -1871,7 +1871,7 @@ static struct hashmap *get_directory_renames(struct 
diff_queue_struct *pairs,
 
entry = dir_rename_find_entry(dir_renames, old_dir);
if (!entry) {
-   entry = xmalloc(sizeof(struct dir_rename_entry));
+   entry = xmalloc(sizeof(*entry));
dir_rename_entry_init(entry, old_dir);
hashmap_put(dir_renames, entry);
} else {
-- 
2.16.0.32.gc5b761fb27.dirty

Re: [PATCH v2 02/18] Add a new builtin: branch-diff

[Duy Nguyen]

On Mon, May 7, 2018 at 9:50 AM, Jeff King  wrote:
> On Sat, May 05, 2018 at 11:57:26PM +0200, Johannes Schindelin wrote:
>
>> > It feels really petty complaining about the name, but I just want to
>> > raise the point, since it will never be easier to change than right now.
>>
>> I do hear you. Especially since I hate `git cherry` every single time that
>> I try to tab-complete `git cherry-pick`.
>
> Me too. :)

Just so you know I'm also not happy with that "git cherry". Since I'm
updating git-completion.bash in this area and we got 3 "me too" votes
(four if we count Szeder in another thread), I'm going to implementing
something to at least let you exclude "cherry" from the completion
list if you want.
-- 
Duy

Re: [PATCH 4/5] lock_file: make function-local locks non-static

[Duy Nguyen]

On Sun, May 6, 2018 at 9:32 PM, Martin Ågren  wrote:
> On 6 May 2018 at 19:42, Duy Nguyen  wrote:
>> On Sun, May 6, 2018 at 7:26 PM, Duy Nguyen  wrote:
>>> On Sun, May 6, 2018 at 4:10 PM, Martin Ågren  wrote:
 These `struct lock_file`s are local to their respective functions and we
 can drop their staticness.
>
 -   static struct lock_file lock;
 +   struct lock_file lock = LOCK_INIT;
>>>
>>> Is it really safe to do this? I vaguely remember something about
>>> (global) linked list and signal handling which could trigger any time
>>> and probably at atexit() time too (i.e. die()). You don't want to
>>> depend on stack-based variables in that case.
>>
>> So I dug in a bit more about this. The original implementation does
>> not allow stack-based lock files at all in 415e96c8b7 ([PATCH]
>> Implement git-checkout-cache -u to update stat information in the
>> cache. - 2005-05-15). The situation has changed since 422a21c6a0
>> (tempfile: remove deactivated list entries - 2017-09-05). At the end
>> of that second commit, Jeff mentioned "We can clean them up
>> individually" which I guess is what these patches do. Though I do not
>> know if we need to make sure to call "release" function or something/
>> Either way you need more explanation and assurance than just "we can
>> drop their staticness" in the commit mesage.
>
> Thank you Duy for your comments. How about I write the commit message
> like so:

+Jeff. Since he made it possible to remove lock file from the global
linked list, he probably knows well what to check when switching from
a static lock file to a stack-local one.

>
>   After 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05),
>   we can have lockfiles on the stack. These `struct lock_file`s are local
>   to their respective functions and we can drop their staticness.
>
>   Each of these users either commits or rolls back the lock in every
>   codepath, with these possible exceptions:
>
> * We bail using a call to `die()` or `exit()`. The lock will be
>   cleaned up automatically.
>
> * We return early from a function `cmd_foo()` in builtin/, i.e., we
>   are just about to exit. The lock will be cleaned up automatically.

There are also signals which can be caught and run on its own stack (I
think) so whatever variable on the current stack should be safe, I
guess.

>   If I have missed some codepath where we do not exit, yet leave a locked
>   lock around, that was so also before this patch. If we would later
>   re-enter the same function, then before this patch, we would be retaking
>   a lock for the very same `struct lock_file`, which feels awkward, but to
>   the best of my reading has well-defined behavior. Whereas after this
>   patch, we would attempt to take the lock with a completely fresh `struct
>   lock_file`. In both cases, the result would simply be that the lock can
>   not be taken, which is a situation we already handle.

There is a difference here, if the lock is not released properly,
previously the lockfile is still untouched. If it's on stack, it may
be overwritten which can corrupt the linked list to get to the next
lock file.  (and this is about calling the function in question just
_once_ not the second time).
-- 
Duy

Re: Is support for 10.8 dropped?

[Igor Korot]

Hi guys,
I am going to try and build the OpenSSL and Git locally.
Search brought me here:
https://wiki.openssl.org/index.php/Compilation_and_Installation#OS_X.

Which options do I absolutely have to have for configuring OpenSSL?
Should I exclude "no--ssl2"? "no-ssl3"?

Thank you.


On Mon, Apr 23, 2018 at 6:16 PM, Perry Hutchison  wrote:
> Igor Korot  wrote:
>> This laptop is old and doesn't have too big of a hard drive.
>> And I'm trying to create a big program
>
> Building OpenSSL via homebrew or MacPorts would likely take less
> space than building all of git that way, but if even that is
> too much perhaps it is time to consider moving the development
> environment onto an external hard drive (via USB, eSATA, or even
> FireWire) -- the presumption being that the development environment
> may not need to be lugged around when using the laptop as a laptop.

Re: What's cooking in git.git (May 2018, #01; Mon, 7)

[Derrick Stolee]

On 5/7/2018 10:58 AM, Junio C Hamano wrote:

* ds/generation-numbers (2018-05-02) 11 commits
  - commit-graph.txt: update design document
  - merge: check config before loading commits
  - commit: use generation number in remove_redundant()
  - commit: add short-circuit to paint_down_to_common()
  - commit: use generation numbers for in_merge_bases()
  - ref-filter: use generation number for --contains
  - commit-graph: always load commit-graph information
  - commit: use generations in paint_down_to_common()
  - commit-graph: compute generation numbers
  - commit: add generation number to struct commmit
  - ref-filter: fix outdated comment on in_commit_list
  (this branch uses ds/commit-graph and ds/lazy-load-trees.)

  A recently added "commit-graph" datafile has learned to store
  pre-computed generation numbers to speed up the decisions to stop
  history traversal.

  Is this ready for 'next'?


I see that you squashed the fix from [1], so I think this is ready to 
go. Thanks!


[1] 
https://public-inbox.org/git/1cfe38f6-925b-d36b-53ae-6b586eed1...@gmail.com/



* ds/lazy-load-trees (2018-05-02) 6 commits
   (merged to 'next' on 2018-05-02 at d54016d9e3)
  + coccinelle: avoid wrong transformation suggestions from commit.cocci
   (merged to 'next' on 2018-04-25 at b90813f421)
  + commit-graph: lazy-load trees for commits
  + treewide: replace maybe_tree with accessor methods
  + commit: create get_commit_tree() method
  + treewide: rename tree to maybe_tree
  + Merge branch 'bw/c-plus-plus' into ds/lazy-load-trees
  (this branch is used by ds/generation-numbers; uses ds/commit-graph.)

  The code has been taught to use the duplicated information stored
  in the commit-graph file to learn the tree object name for a commit
  to avoid opening and parsing the commit object when it makes sense
  to do so.

  Will merge to 'master'.


* ds/commit-graph (2018-04-11) 16 commits
   (merged to 'next' on 2018-04-25 at 18af3d28d9)
  + commit-graph: implement "--append" option
  + commit-graph: build graph from starting commits
  + commit-graph: read only from specific pack-indexes
  + commit: integrate commit graph with commit parsing
  + commit-graph: close under reachability
  + commit-graph: add core.commitGraph setting
  + commit-graph: implement git commit-graph read
  + commit-graph: implement git-commit-graph write
  + commit-graph: implement write_commit_graph()
  + commit-graph: create git-commit-graph builtin
  + graph: add commit graph design document
  + commit-graph: add format document
  + csum-file: refactor finalize_hashfile() method
  + csum-file: rename hashclose() to finalize_hashfile()
  + Merge branch 'jk/cached-commit-buffer' into HEAD
  + Merge branch 'jt/binsearch-with-fanout' into HEAD
  (this branch is used by ds/generation-numbers and ds/lazy-load-trees.)

  Precompute and store information necessary for ancestry traversal
  in a separate file to optimize graph walking.

  Will merge to 'master'.


These have been queued for master for a few weeks. Is anything delaying 
them? I'd love to see the community dogfood this feature by running the 
following on their local repos:


    git config core.commitGraph true
    git show-ref -s | git commit-graph write --stdin-commits

Thanks,
-Stolee

Re: [PATCH 8/8] gpg-interface: handle alternative signature types

[Junio C Hamano]

Jeff King  writes:

> On Tue, Apr 17, 2018 at 12:12:12AM +, brian m. carlson wrote:
>
>> > That argues more strongly that we would regret unless we make the
>> > end-user configuration to at least the whole string (which later can
>> > be promoted to "a pattern that matches the whole string"), not just
>> > the part after mandatory "-BEGIN ", methinks.
>> 
>> Yeah, I think this patch set is "add gpgsm support", which I can see as
>> a valuable goal in and of itself, but I'm not sure the attempt to make
>> it generic is in the right place.  If we want to be truly generic, the
>> way to do that is to invoke a helper based on signature type (e.g.
>> git-sign-gpg, git-sign-gpgsm, git-sign-signify) to do the signing and
>> verification.  We need not ship these helpers ourselves; interested
>> third-parties can provide them, and we can add configuration to match
>> against regexes for non-built-in types (which is required for many other
>> formats).
>
> Isn't that basically what this patch is, though? Or at least a step in
> that direction?

I think that is what Brian is saying, too (and if so I would also
agree).  It probably is a good step.  It is just the feature may be
sold under a wrong (or, overly wide) label, perhaps?

Re: [PATCH v2 02/18] Add a new builtin: branch-diff

[Junio C Hamano]

Johannes Schindelin  writes:

> It would be easy to introduce, but I am wary about its usefulness.
> Unless you re-generate the branch from patches (which I guess you do a
> lot, but I don't), you are likely to compare incomplete patch series: say,
> when you call `git rebase -i` to reword 05/18's commit message, your
> command will only compare 05--18 of the patch series.

Well that is exactly the point of that "..@{1} @{1}..", which turned
out to be very useful in practice at least for me when I am updating
a topic with "rebase -i", and then reviewing what I did with tbdiff.

I do not want 01-04 in the above case as I already know I did not
touch them.

Re: [PATCH v2] completion: reduce overhead of clearing cached --options

[Matthew Coleman]

I haven't seen any discussion about this recently. What are the chances of 
getting it merged? I'd like to see this included in 2.18.

>> To get the names of all '$__git_builtin_*' variables caching --options
>> of builtin commands in order to unset them, 8b0eaa41f2 (completion:
>> clear cached --options when sourcing the completion script,
>> 2018-03-22) runs a 'set |sed s///' pipeline.  This works both in Bash
>> and in ZSH, but has a higher than necessary overhead with the extra
>> processes.
>> 
>> In Bash we can do better: run the 'compgen -v __gitcomp_builtin_'
>> builtin command, which lists the same variables, but without a
>> pipeline and 'sed' it can do so with lower overhead.
>> ZSH will still continue to run that pipeline.
>> 
>> This change also happens to work around an issue in the default Bash
>> ...
>> Updated the commit message to explicitly mention that ZSH is
>> unaffected.  The patch is the same.
> 
> Thanks.

What's cooking in git.git (May 2018, #01; Mon, 7)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.  The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[New Topics]

* tb/test-apfs-utf8-normalization (2018-05-02) 1 commit
 - test: correct detection of UTF8_NFD_TO_NFC for APFS

 A test to see if the filesystem normalizes UTF-8 filename has been
 updated to check what we need to know in a more direct way, i.e. a
 path created in NFC form can be accessed with NFD form (or vice
 versa) to cope with APFS as well as HFS.

 Will merge to 'next'.


* ab/get-short-oid (2018-05-02) 12 commits
 - get_short_oid: document & warn if we ignore the type selector
 - config doc: document core.disambiguate
 - get_short_oid / peel_onion: ^{commit} should be commit, not committish
 - get_short_oid / peel_onion: ^{tree} should be tree, not treeish
 - get_short_oid: learn to disambiguate by ^{blob}
 - get_short_oid: learn to disambiguate by ^{tag}
 - get_short_oid: sort ambiguous objects by type, then SHA-1
 - sha1-name.c: move around the collect_ambiguous() function
 - cache.h: add comment explaining the order in object_type
 - git-p4: change "commitish" typo to "committish"
 - sha1-array.h: align function arguments
 - sha1-name.c: remove stray newline


* ah/misc-doc-updates (2018-05-06) 7 commits
 - doc: normalize [--options] to [options] in git-diff
 - doc: add note about shell quoting to revision.txt
 - git-svn: remove ''--add-author-from' for 'commit-diff'
 - doc: add '-d' and '-o' for 'git push'
 - doc: clarify ignore rules for git ls-files
 - doc: align 'diff --no-index' in text and synopsis
 - doc: improve formatting in githooks.txt

 Misc doc fixes.

 Will merge to 'next'.


* bc/format-patch-cover-no-attach (2018-05-02) 1 commit
 - format-patch: make cover letters always text/plain

 "git format-patch --cover --attach" created a broken MIME multipart
 message for the cover letter, which has been fixed by keeping the
 cover letter as plain text file.

 Will merge to 'next'.


* bp/test-drop-caches (2018-05-04) 1 commit
 - test-drop-caches: simplify delay loading of NtSetSystemInformation

 Code simplification.

 Will merge to 'next'.


* cc/perf-bisect (2018-05-06) 1 commit
 - perf/bisect_run_script: disable codespeed

 Performance test updates.

 Will merge to 'next'.


* cf/submodule-progress-dissociate (2018-05-04) 3 commits
 - submodule: add --dissociate option to add/update commands
 - submodule: add --progress option to add command
 - submodule: clean up subsititions in script

 "git submodule update" and "git submodule add" supported the
 "--reference" option to borrow objects from a neighbouring local
 repository like "git clone" does, but lacked the more recent
 invention "--dissociate".  Also "git submodule add" has been taught
 to take the "--progress" option.

 Is this ready for 'next'?  Should "git submodule -h" list the new
 options in its short help?


* dd/send-email-reedit (2018-05-06) 1 commit
 - git-send-email: allow re-editing of message

 "git send-email" can sometimes offer confirmation dialog "Send this
 email?" with choices 'Yes', 'No', 'Quit', and 'All'.  A new action
 'Edit' has been added to this dialog's choice.
 
 Waiting briefly for an ack or two.
 cf. 


* em/status-rename-config (2018-05-06) 1 commit
 - wt-status: use settings from git_diff_ui_config

 "git status" learned to pay attention to UI related diff
 configuration variables such as diff.renames.

 Will merge to 'next'.


* jm/cache-entry-from-mem-pool (2018-05-02) 5 commits
 - block alloc: add validations around cache_entry lifecyle
 - block alloc: allocate cache entries from mem_pool
 - mem-pool: fill out functionality
 - block alloc: add lifecycle APIs for cache_entry structs
 - read-cache: teach refresh_cache_entry() to take istate

 For a large tree, the index needs to hold many cache entries
 allocated on heap.  These cache entries are now allocated out of a
 dedicated memory pool to amortize malloc(3) overhead.

 Needs review.  
 Is the "caller always knows which pool an entry came from and calls
 the right kind of free" a feasible approach?


* js/branch-diff (2018-05-06) 18 commits
 - completion: support branch-diff
 - branch-diff: add a man page
 - branch-diff --dual-color: work around bogus white-space warning
 - branch-diff: offer to dual-color the diffs
 - diff: add an internal option to dual-color diffs of diffs
 - color: provide inverted colors, too
 - branch-diff: use color for the commit pairs
 - branch-diff: add tests
 - branch-diff: do not show "function names" in hunk headers
 - branch-diff: adjust the output of the commit pairs
 - 

Re: [RFC] Other chunks for commit-graph, part 1 - Bloom filters, topo order, etc.

[Derrick Stolee]

On 5/4/2018 3:40 PM, Jakub Narebski wrote:

Hello,

With early parts of commit-graph feature (ds/commit-graph and
ds/lazy-load-trees) close to being merged into "master", see
https://public-inbox.org/git/xmqq4ljtz87g@gitster-ct.c.googlers.com/
I think it would be good idea to think what other data could be added
there to make Git even faster.


Before thinking about adding more data to the commit-graph, I think 
instead we need to finish taking advantage of the data that is already 
there. This means landing the generation number patch [1] (I think this 
is close, so I'll send a v6 this week if there is no new feedback.) and 
the auto-compute patch [2] (this could use more feedback, but I'll send 
a v1 based on the RFC feedback if no one chimes in).


[1] 
https://public-inbox.org/git/20180501124652.155781-1-dsto...@microsoft.com/

    [PATCH v5 00/11] Compute and consume generation numbers

[2] 
https://public-inbox.org/git/20180417181028.198397-1-dsto...@microsoft.com/

    [RFC PATCH 00/12] Integrate commit-graph into 'fsck' and 'gc'

The big wins remaining from this data are `git tag --merged` and `git 
log --graph`. The `tag` scenario is probably easier: this can be done by 
replacing the revision-walk underlying the call to use 
paint_down_to_common() instead. Requires adding an external method to 
commit.c, but not too much code.


The tougher challenge is `git log --graph`. The revision walk machinery 
currently uses two precompute phases before iterating results to the 
pager: limit_list() and sort_in_topological_order(); these correspond to 
two phases of Kahn's algorithm for topo-sort (compute in-degrees, then 
walk by peeling commits with in-degree zero). This requires O(N) time, 
where N is the number of reachable commits. Instead, we could make this 
be O(W) time to output one page of results, where W is (roughly) the 
number of reachable commits with generation number above the last 
reported result.


In order to take advantage of this approach, the two phases of Kahn's 
algorithm need to be done in-line with reporting results to the pager. 
This means keeping two queues: one is a priority queue by generation 
number that computes in-degrees, the other is a priority queue (by 
commit-date or a visit-order value to do the --topo-order priority) that 
peels the in-degree-zero commits (and decrements the in-degree of their 
parents). I have not begun this refactoring effort because appears 
complicated to me, and it will be hard to tease out the logic without 
affecting other consumers of the revision-walk machinery.


I would love it if someone picked up the `git log --graph` task, since 
it will be a few weeks before I have the time to focus on it.


Without completing the benefits we get from generation numbers, these 
investigations into other reachability indexes will be incomplete as 
they are comparing benefits without all consumers taking advantage of a 
reachability index.


[...]

Bloom filter for changed paths
--

The goal of this chunk is to speed up checking if the file or directory
was changed in given commit, for queries such as "git log -- " or
"git blame ".  This is something that according to "Git Merge
contributor summit notes" [2] is already present in VSTS (Visual Studio
Team Services - the server counterpart of GVFS: Git Virtual File System)
at Microsoft:

AV> - VSTS adds bloom filters to know which paths have changed on the commit
AV> - tree-same check in the bloom filter is fast; speeds up file history checks
AV> - might be useful in the client as well, since limited-traversal is common
AV> - if the file history is _very_ sparse, then bloom filter is useful
AV> - but needs pre-compute, so useful to do once
AV> - first make the client do it, then think about how to serve it centrally

[2]: 
https://public-inbox.org/git/alpine.DEB.2.20.1803091557510.23109@alexmv-linux/

I think it was what Derrick Stolee was talking about at the end of his
part of "Making Git for Windows" presentation at Git Merge 2018:
https://youtu.be/oOMzi983Qmw?t=1835

This was also mentioned in subthread of "Re: [PATCH v2 0/4] Lazy-load
trees when reading commit-graph", starting from [3]
[3]: https://public-inbox.org/git/86y3hyeu6c@gmail.com/


Again, the benefits of Bloom filters should only be measured after 
already taking advantage of a reachability index during `git log`. 
However, you could get performance benefits from Bloom filters in a 
normal `git log` (no topo-order).


The tricky part about this feature is that the decisions we made in our 
C# implementation for the VSTS Git server may be very different than the 
needs for the C implementation of the Git client. Questions like "how do 
we handle merge commits?" may have different answers, which can only be 
discovered by implementing the feature.


(The answer for VSTS is that we only store Bloom filters containing the 
list of changed paths against the first parent. The second parent 
frequently has 

Re: [PATCH] tests: introduce test_unset_prereq, for debugging

[Junio C Hamano]

SZEDER Gábor  writes:

>> For convenience, the following two methods are now supported ways to
>> pretend that a prereq is not met:
>> 
>>  test_set_prereq !GPG
>> 
>> and
>> 
>>  test_unset_prereq GPG
>
> I'm not sure this is the right way to do this.
>
> I wanted to run the whole test suite with all GPG tests skipped the
> other day.  With this 'test_unset_prereq' I would have to modify all
> test scripts containing tests depending on the GPG prereq and add
> 'test_unset_prereq GPG', right?

Excellent point.  This won't cover that use case well.

That does not mean this change is useless.  If you are focusing on
developing a single test script, you can afford to tentatively add
these set/unset, just like you tentatively debug with echo's ;-)

But I find your SKIP_PREREQS an excellent idea.

It may want to be FORCE_PREREQS that lets you pretend a prereq is
not satisfied on your machine even when it actually is, and also
lets you pretend a prereq is satisfied on your machine even when it
is not.  SKIP_PREREQS would only do the first half, which would be
sufficient most of the time, though.

> I think we would be better served by an environment variable similar
> to $GIT_SKIP_TESTS, e.g. $GIT_SKIP_PREREQS, to list all the prereqs
> that should be skipped even if they were met.