Re: Bug: format-patch MIME boundary not added to cover letter when attach enabled

[Junio C Hamano]

"brian m. carlson"  writes:

> On Sun, Apr 29, 2018 at 09:40:13PM -0400, Patrick Hemmer wrote:
>> When you use `git format-patch --cover-letter --attach`, the cover
>> letter does not have the trailing MIME boundary. RFC2046 states that the
>> last part must be followed by a closing boundary. This causes some email
>> clients (Thunderbird in my case) to discard the message body.
>> This is experienced with git 2.16.3.
>
> Thanks for reporting this.  I can confirm this with a reasonably recent
> next.  Let me see if I can come up with a patch.

Thanks.  It is true that the current output from the tool is corrupt
mime multi-part, and we need to do something about it.

I however have to wonder if it even makes sense for --cover to pay
attention to --attach and produce the cover template that has "BLURB
HERE" etc.  in a multi-part format.  Shouldn't we be making a simple
plain text file instead?

What's cooking in git.git (Apr 2018, #04; Mon, 30)

[Junio C Hamano]

What's cooking in git.git (Apr 2018, #04; Mon, 30)
--

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]

* cc/perf-aggregate-unknown-option (2018-04-26) 1 commit
 - perf/aggregate: use Getopt::Long for option parsing

 Perf-test helper updates.

 Will merge to 'next'.


* ab/perl-python-attrs (2018-04-27) 3 commits
 - .gitattributes: add a diff driver for Python
 - .gitattributes: use the "perl" differ for Perl
 - .gitattributes: add *.pl extension for Perl

 We learned that our source files with ".pl" and ".py" extensions
 are Perl and Python files respectively and changes to them are
 better viewed as such with appropriate diff drivers.

 Will merge to 'next'.


* is/parsing-line-range (2018-04-27) 2 commits
 . log: prevent error if line range ends past end of file
 . blame: prevent error if range ends past end of file

 Parsing of -L[][,[]] parameters "git blame" and "git log"
 take has been tweaked.

 Seems to break a few tests.


* js/test-unset-prereq (2018-04-30) 1 commit
 - tests: introduce test_unset_prereq, for debugging

 Test debugging aid.

 Will merge to 'next'.

--
[Stalled]

* ld/p4-unshelve (2018-02-22) 1 commit
 - git-p4: add unshelve command

 "git p4" learned to "unshelve" shelved commit from P4.

 Will hold, perhaps drop and use format-change that uses a proper "diff".
 cf. 


* av/fsmonitor-updates (2018-01-04) 6 commits
 - fsmonitor: use fsmonitor data in `git diff`
 - fsmonitor: remove debugging lines from t/t7519-status-fsmonitor.sh
 - fsmonitor: make output of test-dump-fsmonitor more concise
 - fsmonitor: update helper tool, now that flags are filled later
 - fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid
 - dir.c: update comments to match argument name

 Code clean-up on fsmonitor integration, plus optional utilization
 of the fsmonitor data in diff-files.

 Waiting for an update.
 cf. 



* pb/bisect-helper-2 (2017-10-28) 8 commits
 - t6030: make various test to pass GETTEXT_POISON tests
 - bisect--helper: `bisect_start` shell function partially in C
 - bisect--helper: `get_terms` & `bisect_terms` shell function in C
 - bisect--helper: `bisect_next_check` shell function in C
 - bisect--helper: `check_and_set_terms` shell function in C
 - wrapper: move is_empty_file() and rename it as is_empty_or_missing_file()
 - bisect--helper: `bisect_write` shell function in C
 - bisect--helper: `bisect_reset` shell function in C

 Expecting a reroll.
 cf. 
<0102015f5e5ee171-f30f4868-886f-47a1-a4e4-b4936afc545d-000...@eu-west-1.amazonses.com>


* mk/http-backend-content-length (2017-11-27) 4 commits
 - SQUASH???
 - t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases
 - SQUASH???
 - http-backend: respect CONTENT_LENGTH as specified by rfc3875

 The http-backend (used for smart-http transport) used to slurp the
 whole input until EOF, without paying attention to CONTENT_LENGTH
 that is supplied in the environment and instead expecting the Web
 server to close the input stream.  This has been fixed.

 Expecting a reroll.
 Suggested fixes to be used when rerolling is queued, but I'd
 prefer _not_ squashing them myself.

 Also, it may be too complex solution for the problem.
 cf. <20171204171308.ga13...@sigill.intra.peff.net>


* jk/drop-ancient-curl (2017-08-09) 5 commits
 - http: #error on too-old curl
 - curl: remove ifdef'd code never used with curl >=7.19.4
 - http: drop support for curl < 7.19.4
 - http: drop support for curl < 7.16.0
 - http: drop support for curl < 7.11.1

 Some code in http.c that has bitrot is being removed.

 Expecting a reroll.


* mk/use-size-t-in-zlib (2017-08-10) 1 commit
 . zlib.c: use size_t for size

 The wrapper to call into zlib followed our long tradition to use
 "unsigned long" for sizes of regions in memory, which have been
 updated to use "size_t".

 Needs resurrecting by making sure the fix is good and still applies
 (or adjusted to today's codebase).

--
[Cooking]

* fg/completion-external (2018-04-30) 1 commit
 - completion: load completion file for external subcommand

 The command line completion mechanism (in contrib/) learned to load
 custom completion file for "git $command" where $command is a
 custom "git-$command" that the end user has on the $PATH when using
 newer version of bash.


* 

Re: [PATCH 6/6] Convert remaining die*(BUG) messages

[Eric Sunshine]

On Sun, Apr 29, 2018 at 6:19 PM, Johannes Schindelin
 wrote:
> These were not caught by the previous commit, as they did not match the
> regular expression.
>
> Signed-off-by: Johannes Schindelin 
> ---
> diff --git a/submodule.c b/submodule.c
> @@ -2043,7 +2043,7 @@ const char *get_superproject_working_tree(void)
> if (super_sub_len > cwd_len ||
> strcmp([cwd_len - super_sub_len], super_sub))
> -   die (_("BUG: returned path string doesn't match 
> cwd?"));
> +   BUG("returned path string doesn't match cwd?");

This message used to be localizable but no longer is, which makes
sense since it's not intended as a user-facing error message but
rather is meant for Git developers. Good.

Re: Bug: format-patch MIME boundary not added to cover letter when attach enabled

[brian m. carlson]

On Sun, Apr 29, 2018 at 09:40:13PM -0400, Patrick Hemmer wrote:
> When you use `git format-patch --cover-letter --attach`, the cover
> letter does not have the trailing MIME boundary. RFC2046 states that the
> last part must be followed by a closing boundary. This causes some email
> clients (Thunderbird in my case) to discard the message body.
> This is experienced with git 2.16.3.

Thanks for reporting this.  I can confirm this with a reasonably recent
next.  Let me see if I can come up with a patch.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Bug: format-patch MIME boundary not added to cover letter when attach enabled

[Patrick Hemmer]

When you use `git format-patch --cover-letter --attach`, the cover
letter does not have the trailing MIME boundary. RFC2046 states that the
last part must be followed by a closing boundary. This causes some email
clients (Thunderbird in my case) to discard the message body.
This is experienced with git 2.16.3.

For example:

$ git format-patch --cover-letter --attach --root -o /tmp/out
/tmp/out/-cover-letter.patch
/tmp/out/0001-hello-world.patch

$ cat /tmp/out/-cover-letter.patch
>From a25ac88e6216131e8b000335d32bb99d4e5185ac Mon Sep 17 00:00:00 2001
From: Patrick Hemmer 
Date: Sun, 29 Apr 2018 21:26:45 -0400
Subject: [PATCH 0/1] *** SUBJECT HERE ***
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="2.16.3"

This is a multi-part message in MIME format.
--2.16.3
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit


*** BLURB HERE ***

Patrick Hemmer (1):
  hello world

-- 
2.16.3

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

[Junio C Hamano]

Johannes Schindelin  writes:

> While working on the --convert-graft-file test, I missed that I was
> relying on the GPG prereq, by using output of test cases that were only
> run under that prereq.
>
> For debugging, it was really convenient to force that prereq to be
> unmet, but there was no easy way to do that. So I came up with a way,
> and this patch reflects the cleaned-up version of that way.
>
> 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

Very nice new feature, and a great description ;-)

> Signed-off-by: Johannes Schindelin 
> ---
> Published-As: https://github.com/dscho/git/releases/tag/test-unset-prereq-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git test-unset-prereq-v1
>  t/test-lib-functions.sh | 14 +-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 7d620bf2a9a..76cd6630f29 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -278,8 +278,20 @@ write_script () {
>  # The single parameter is the prerequisite tag (a simple word, in all
>  # capital letters by convention).
>  
> +test_unset_prereq () {
> + ! test_have_prereq "$1" ||
> + satisfied_prereq="${satisfied_prereq% $1 *} ${satisfied_prereq#* $1 }"
> +}
> +
>  test_set_prereq () {
> - satisfied_prereq="$satisfied_prereq$1 "
> + case "$1" in
> + !*)
> + test_unset_prereq "${1#!}"
> + ;;
> + *)
> + satisfied_prereq="$satisfied_prereq$1 "
> + ;;
> + esac
>  }
>  satisfied_prereq=" "
>  lazily_testable_prereq= lazily_tested_prereq=
>
> base-commit: 1f1cddd558b54bb0ce19c8ace353fd07b758510d

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

[Junio C Hamano]

Thomas Gummerer  writes:

> On 04/27, Eric Sunshine wrote:
>> On Tue, Apr 24, 2018 at 5:56 PM, Thomas Gummerer  
>> wrote:
>> > Thanks Eric for the review and the suggestions on the previous round.
>> >
>> > Changes since the previous round:
>> >
>> > - UNLEAK new_branch after it was xstrndup'd
>> > - update the commit message of 2/4 according to Eric's suggestions
>> > - make force_new_branch a boolean flag in
>> >   print_preparing_worktree_line, instead of using it as the branch
>> >   name.  Instead use new_branch as the new branch name everywhere in
>> >   that function.
>> > - get rid of the ckeckout_existing_branch flag
>> 
>> Thanks. I did another full review of all the patches and played around
>> with the new functionality again for good measure. Everything looked
>> good, and I think the patches are now ready to graduate.
>
> Thanks for all your review work on this series!

Thanks, both.

Re: What's cooking in git.git (Apr 2018, #03; Wed, 25)

[Junio C Hamano]

Eric Sunshine  writes:

> On Wed, Apr 25, 2018 at 4:37 AM, Junio C Hamano  wrote:
>> * tg/worktree-add-existing-branch (2018-04-25) 4 commits
>>  - worktree: teach "add" to check out existing branches
>>  - worktree: factor out dwim_branch function
>>  - worktree: improve message when creating a new worktree
>>  - worktree: remove extra members from struct add_opts
>>
>>  "git worktree add" learned to check out an existing branch.
>>
>>  Is this ready for 'next'?
>
> I just gave v9 (which you have queued) a final full review and found
> it satisfactory (and gave my Reviewed-by:), so yes, it seems ready for
> 'next'.

Thanks, both.

Re: [RFC PATCH] checkout: Force matching mtime between files

[Junio C Hamano]

Duy Nguyen  writes:

> Target revision should be available in the index. But this gives me an
> idea to another thing that bugs me: sending the list to the hook means
> I have to deal with separator (\n or NUL?) or escaping. This mentions
> of index makes me take a different direction. I could produce a small
> index that contains just what is modified, then you can retrieve
> whatever info you want with `git ls-files` or even `git show` after
> pointing $GIT_INDEX_FILE to it.

That's somewhere in between a tail wagging the dog and a hammer
looking like a solution even when you have a screw.  By passing a
temporary index, you may be able to claim that you are feeding the
necessary information without corruption and in a readable and
native format of Git, and make it up to the reader to grab the paths
out of it, but

 (1) the contents, and probably the cached stat information, in that
 temporary index is duplicated and wasted; you know from the
 time you design this thing that all you want to convey is a
 list of paths.

 (2) it is totally unclear who is responsible for cleaning the
 temporary file up.

 (3) the recipient must walk and carefully grab the path, certainly
 has to "deal with separator (\n or NUL?) or escaping" anyway,
 especially if the reason you use a temporary index is because
 "they can use ls-files on it".  They need to read from ls-files
 anyway, so that is no better than feeding ls-files compatible
 input from the standard input of the hook script.

no?

Re: [RFC PATCH] checkout: Force matching mtime between files

[Junio C Hamano]

Elijah Newren  writes:

> Here's a crazy idea -- maybe instead of a list of pathspecs you just
> provide the timestamp of when git checkout started.  Then the hook
> could walk the tree, find all files with modification times at least
> that late, and modify them all back to the the timestamp of when the
> git checkout started.
>
> Would that be enough?  Is that too crazy?
>
> Sure, people could concurrently edit a file or run another program
> that modified files, but if you're doing that you're already playing
> race games with whether your next incremental build is going to be
> able to be correct.  (Some (annoying) IDEs explicitly lock you out
> from editing files during a build to attempt to avoid this very
> problem.)
>
> That does leave one other caveat: If people intentionally do really
> weird stuff with having files with modification timestamps far in the
> future.  However, it seems likely that the group of people doing that,
> if non-zero in number, is likely to be dis-joint with the group of
> folks that want this special
> uniform-timestamp-across-files-in-a-checkout behavior.

These two groups may share the same degree of insanity ;-)

But the single timestamp idea certainly sounds workable, except that
care must be taken to make sure we really grab the fs timestamp (it
is not uncommon for ">F; stat F" to yield quite different time from
"date", when the filesystem is on a remote box).

[PATCH 6/6] Convert remaining die*(BUG) messages

[Johannes Schindelin]

These were not caught by the previous commit, as they did not match the
regular expression.

Signed-off-by: Johannes Schindelin 
---
 git-compat-util.h | 2 +-
 pathspec.c| 2 +-
 submodule.c   | 2 +-
 vcs-svn/fast_export.c | 6 --
 4 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 07e383257b4..3a7216f5313 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1052,7 +1052,7 @@ int git_qsort_s(void *base, size_t nmemb, size_t size,
 
 #define QSORT_S(base, n, compar, ctx) do { \
if (qsort_s((base), (n), sizeof(*(base)), compar, ctx)) \
-   die("BUG: qsort_s() failed");   \
+   BUG("qsort_s() failed");\
 } while (0)
 
 #ifndef REG_STARTEND
diff --git a/pathspec.c b/pathspec.c
index a637a6d15c0..27cd6067860 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -486,7 +486,7 @@ static void init_pathspec_item(struct pathspec_item *item, 
unsigned flags,
/* sanity checks, pathspec matchers assume these are sane */
if (item->nowildcard_len > item->len ||
item->prefix > item->len) {
-   die ("BUG: error initializing pathspec_item");
+   BUG("error initializing pathspec_item");
}
 }
 
diff --git a/submodule.c b/submodule.c
index 733db441714..c282fa8fe51 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2043,7 +2043,7 @@ const char *get_superproject_working_tree(void)
 
if (super_sub_len > cwd_len ||
strcmp([cwd_len - super_sub_len], super_sub))
-   die (_("BUG: returned path string doesn't match cwd?"));
+   BUG("returned path string doesn't match cwd?");
 
super_wt = xstrdup(cwd);
super_wt[cwd_len - super_sub_len] = '\0';
diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index 3fd047a8b82..b5b8913cb0f 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -320,7 +320,8 @@ const char *fast_export_read_path(const char *path, 
uint32_t *mode_out)
err = fast_export_ls(path, mode_out, );
if (err) {
if (errno != ENOENT)
-   die_errno("BUG: unexpected fast_export_ls error");
+   BUG("unexpected fast_export_ls error: %s",
+   strerror(errno));
/* Treat missing paths as directories. */
*mode_out = S_IFDIR;
return NULL;
@@ -338,7 +339,8 @@ void fast_export_copy(uint32_t revision, const char *src, 
const char *dst)
err = fast_export_ls_rev(revision, src, , );
if (err) {
if (errno != ENOENT)
-   die_errno("BUG: unexpected fast_export_ls_rev error");
+   BUG("unexpected fast_export_ls_rev error: %s",
+   strerror(errno));
fast_export_delete(dst);
return;
}
-- 
2.17.0.windows.1.36.gdf4ca5fb72a

[PATCH 5/6] Replace all die("BUG: ...") calls by BUG() ones

[Johannes Schindelin]

In d8193743e08 (usage.c: add BUG() function, 2017-05-12), a new macro
was introduced to use for reporting bugs instead of die(). It was then
subsequently used to convert one single caller in 588a538ae55
(setup_git_env: convert die("BUG") to BUG(), 2017-05-12).

The cover letter of the patch series containing this patch
(cf 20170513032414.mfrwabt4hovuj...@sigill.intra.peff.net) is not
terribly clear why only one call site was converted, or what the plan
is for other, similar calls to die() to report bugs.

Let's just convert all remaining ones in one fell swoop.

This trick was performed by this invocation:

sed -i 's/die("BUG: /BUG("/g' $(git grep -l 'die("BUG' \*.c)

Signed-off-by: Johannes Schindelin 
---
 apply.c  |  4 ++--
 archive-tar.c|  2 +-
 attr.c   | 10 +-
 blame.c  |  2 +-
 builtin/am.c | 20 +--
 builtin/branch.c |  2 +-
 builtin/cat-file.c   |  4 ++--
 builtin/clone.c  |  2 +-
 builtin/commit.c |  2 +-
 builtin/config.c |  2 +-
 builtin/fast-export.c|  2 +-
 builtin/fsck.c   |  2 +-
 builtin/index-pack.c |  4 ++--
 builtin/init-db.c|  2 +-
 builtin/ls-files.c   |  2 +-
 builtin/notes.c  |  8 
 builtin/pack-objects.c   |  4 ++--
 builtin/pull.c   |  2 +-
 builtin/receive-pack.c   |  2 +-
 builtin/replace.c|  2 +-
 builtin/update-index.c   |  2 +-
 bulk-checkin.c   |  2 +-
 color.c  |  4 ++--
 column.c |  2 +-
 config.c | 12 +--
 date.c   |  2 +-
 diff.c   | 12 +--
 dir-iterator.c   |  2 +-
 grep.c   | 16 +++
 http.c   |  8 
 imap-send.c  |  2 +-
 lockfile.c   |  2 +-
 mailinfo.c   |  2 +-
 merge-recursive.c| 12 +--
 notes-merge.c|  4 ++--
 pack-bitmap-write.c  |  2 +-
 pack-bitmap.c|  6 +++---
 pack-objects.c   |  2 +-
 packfile.c   |  6 +++---
 pathspec.c   | 10 +-
 pkt-line.c   |  2 +-
 prio-queue.c |  2 +-
 ref-filter.c |  4 ++--
 refs.c   | 34 
 remote.c |  2 +-
 revision.c   |  4 ++--
 run-command.c| 10 +-
 setup.c  |  4 ++--
 sha1-lookup.c|  2 +-
 sha1-name.c  |  4 ++--
 shallow.c|  6 +++---
 sigchain.c   |  2 +-
 strbuf.c |  4 ++--
 submodule.c  |  6 +++---
 t/helper/test-example-decorate.c | 16 +++
 tmp-objdir.c |  2 +-
 trailer.c|  6 +++---
 transport.c  |  4 ++--
 unpack-trees.c   |  2 +-
 worktree.c   |  2 +-
 wrapper.c|  4 ++--
 wt-status.c  | 20 +--
 zlib.c   |  4 ++--
 63 files changed, 168 insertions(+), 168 deletions(-)

diff --git a/apply.c b/apply.c
index 7e5792c996f..3866adbc97f 100644
--- a/apply.c
+++ b/apply.c
@@ -2375,7 +2375,7 @@ static void update_pre_post_images(struct image *preimage,
if (postlen
? postlen < new_buf - postimage->buf
: postimage->len < new_buf - postimage->buf)
-   die("BUG: caller miscounted postlen: asked %d, orig = %d, used 
= %d",
+   BUG("caller miscounted postlen: asked %d, orig = %d, used = %d",
(int)postlen, (int) postimage->len, (int)(new_buf - 
postimage->buf));
 
/* Fix the length of the whole thing */
@@ -3509,7 +3509,7 @@ static int load_current(struct apply_state *state,
unsigned mode = patch->new_mode;
 
if (!patch->is_new)
-   die("BUG: patch to %s is not a creation", patch->old_name);
+   BUG("patch to %s is not a creation", patch->old_name);
 
pos = cache_name_pos(name, strlen(name));
if (pos < 0)
diff --git a/archive-tar.c b/archive-tar.c
index 3563bcb9f26..61a1a2547cc 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -441,7 +441,7 @@ static int write_tar_filter_archive(const struct archiver 
*ar,
int r;
 
if (!ar->data)
-   die("BUG: tar-filter archiver called with no filter defined");
+   BUG("tar-filter archiver 

[PATCH 3/6] refs/*: report bugs using the BUG() macro

[Johannes Schindelin]

We just prepared t1406 to be okay with BUG reports resulting in SIGABRT
instead of a regular exit code indicating failure. This commit now makes
it so: by calling BUG() (which eventually calls `abort()`), we no longer
exit with code 128 but instead throw that signal.

This trick was performed by this invocation:

sed -i 's/die("BUG: /BUG("/' $(git grep -l 'die("BUG' refs/\*.c)

Signed-off-by: Johannes Schindelin 
---
 refs/files-backend.c  | 20 ++--
 refs/iterator.c   |  6 +++---
 refs/packed-backend.c | 16 
 refs/ref-cache.c  |  2 +-
 4 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index a92a2aa8213..332da47edd9 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -125,7 +125,7 @@ static void files_assert_main_repository(struct 
files_ref_store *refs,
if (refs->store_flags & REF_STORE_MAIN)
return;
 
-   die("BUG: operation %s only allowed for main ref store", caller);
+   BUG("operation %s only allowed for main ref store", caller);
 }
 
 /*
@@ -141,13 +141,13 @@ static struct files_ref_store *files_downcast(struct 
ref_store *ref_store,
struct files_ref_store *refs;
 
if (ref_store->be != _be_files)
-   die("BUG: ref_store is type \"%s\" not \"files\" in %s",
+   BUG("ref_store is type \"%s\" not \"files\" in %s",
ref_store->be->name, caller);
 
refs = (struct files_ref_store *)ref_store;
 
if ((refs->store_flags & required_flags) != required_flags)
-   die("BUG: operation %s requires abilities 0x%x, but only have 
0x%x",
+   BUG("operation %s requires abilities 0x%x, but only have 0x%x",
caller, required_flags, refs->store_flags);
 
return refs;
@@ -166,7 +166,7 @@ static void files_reflog_path(struct files_ref_store *refs,
strbuf_addf(sb, "%s/logs/%s", refs->gitcommondir, refname);
break;
default:
-   die("BUG: unknown ref type %d of ref %s",
+   BUG("unknown ref type %d of ref %s",
ref_type(refname), refname);
}
 }
@@ -184,7 +184,7 @@ static void files_ref_path(struct files_ref_store *refs,
strbuf_addf(sb, "%s/%s", refs->gitcommondir, refname);
break;
default:
-   die("BUG: unknown ref type %d of ref %s",
+   BUG("unknown ref type %d of ref %s",
ref_type(refname), refname);
}
 }
@@ -2010,7 +2010,7 @@ static int files_for_each_reflog_ent_reverse(struct 
ref_store *ref_store,
 
}
if (!ret && sb.len)
-   die("BUG: reverse reflog parser had leftover data");
+   BUG("reverse reflog parser had leftover data");
 
fclose(logfp);
strbuf_release();
@@ -2088,7 +2088,7 @@ static int files_reflog_iterator_advance(struct 
ref_iterator *ref_iterator)
 static int files_reflog_iterator_peel(struct ref_iterator *ref_iterator,
   struct object_id *peeled)
 {
-   die("BUG: ref_iterator_peel() called for reflog_iterator");
+   BUG("ref_iterator_peel() called for reflog_iterator");
 }
 
 static int files_reflog_iterator_abort(struct ref_iterator *ref_iterator)
@@ -2873,7 +2873,7 @@ static int files_initial_transaction_commit(struct 
ref_store *ref_store,
assert(err);
 
if (transaction->state != REF_TRANSACTION_OPEN)
-   die("BUG: commit called for transaction that is not open");
+   BUG("commit called for transaction that is not open");
 
/* Fail if a refname appears more than once in the transaction: */
for (i = 0; i < transaction->nr; i++)
@@ -2899,7 +2899,7 @@ static int files_initial_transaction_commit(struct 
ref_store *ref_store,
 */
if (refs_for_each_rawref(>base, ref_present,
 _refnames))
-   die("BUG: initial ref transaction called with existing refs");
+   BUG("initial ref transaction called with existing refs");
 
packed_transaction = 
ref_store_transaction_begin(refs->packed_ref_store, err);
if (!packed_transaction) {
@@ -2912,7 +2912,7 @@ static int files_initial_transaction_commit(struct 
ref_store *ref_store,
 
if ((update->flags & REF_HAVE_OLD) &&
!is_null_oid(>old_oid))
-   die("BUG: initial ref transaction with old_sha1 set");
+   BUG("initial ref transaction with old_sha1 set");
if (refs_verify_refname_available(>base, update->refname,
  _refnames, NULL,
  err)) {
diff --git a/refs/iterator.c b/refs/iterator.c
index bd35da4e622..2ac91ac3401 100644
--- a/refs/iterator.c
+++ b/refs/iterator.c
@@ -54,7 

Re: [PATCH v4 05/10] commit-graph: always load commit-graph information

[Jakub Narebski]

[Forgot about one thing]

Derrick Stolee  writes:

> Create new load_commit_graph_info() method to fill in the information
> for a commit that exists only in the commit-graph file.

The above sentence is a bit hard to parse because of ambiguity: is it
"the information" that exists only in the commit-graph file, or "a
commit" that exists only in the commit-graph file?

>  Call it from
> parse_commit_buffer() after loading the other commit information from
> the given buffer. Only fill this information when specified by the
> 'check_graph' parameter.
>
> Signed-off-by: Derrick Stolee 

-- 
Jakub Narębski

[PATCH 4/6] run-command: use BUG() to report bugs, not die()

[Johannes Schindelin]

The slightly misleading name die_bug() of the function intended to
report a bug is actually called always, and only reports a bug if the
passed-in parameter `err` is non-zero.

It uses die_errno() to report the bug, to helpfully include the error
message corresponding to `err`.

However, as these messages indicate bugs, we really should use BUG().
And as BUG() is a macro to be able to report the exact file and line
number, we need to convert die_bug() to a macro instead of only
replacing the die_errno() by a call to BUG().

While at it, use a name more indicative of the purpose: CHECK_BUG().

Signed-off-by: Johannes Schindelin 
---
 run-command.c | 23 ++-
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/run-command.c b/run-command.c
index 12c94c1dbe5..0ad6f135d5a 100644
--- a/run-command.c
+++ b/run-command.c
@@ -471,15 +471,12 @@ struct atfork_state {
sigset_t old;
 };
 
-#ifndef NO_PTHREADS
-static void bug_die(int err, const char *msg)
-{
-   if (err) {
-   errno = err;
-   die_errno("BUG: %s", msg);
-   }
-}
-#endif
+#define CHECK_BUG(err, msg) \
+   do { \
+   int e = (err); \
+   if (e) \
+   BUG("%s: %s", msg, strerror(e)); \
+   } while(0)
 
 static void atfork_prepare(struct atfork_state *as)
 {
@@ -491,9 +488,9 @@ static void atfork_prepare(struct atfork_state *as)
if (sigprocmask(SIG_SETMASK, , >old))
die_errno("sigprocmask");
 #else
-   bug_die(pthread_sigmask(SIG_SETMASK, , >old),
+   CHECK_BUG(pthread_sigmask(SIG_SETMASK, , >old),
"blocking all signals");
-   bug_die(pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, >cs),
+   CHECK_BUG(pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, >cs),
"disabling cancellation");
 #endif
 }
@@ -504,9 +501,9 @@ static void atfork_parent(struct atfork_state *as)
if (sigprocmask(SIG_SETMASK, >old, NULL))
die_errno("sigprocmask");
 #else
-   bug_die(pthread_setcancelstate(as->cs, NULL),
+   CHECK_BUG(pthread_setcancelstate(as->cs, NULL),
"re-enabling cancellation");
-   bug_die(pthread_sigmask(SIG_SETMASK, >old, NULL),
+   CHECK_BUG(pthread_sigmask(SIG_SETMASK, >old, NULL),
"restoring signal mask");
 #endif
 }
-- 
2.17.0.windows.1.36.gdf4ca5fb72a

[PATCH 2/6] t1406: prepare for the refs code to fail with BUG()

[Johannes Schindelin]

t1406 specifically verifies that certain code paths fail with a BUG: ...
message.

In the upcoming commit, we will convert that message to be generated via
BUG() instead of die("BUG: ..."), which implies SIGABRT instead of a
regular exit code.

Signed-off-by: Johannes Schindelin 
---
 t/t1406-submodule-ref-store.sh | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh
index e093782cc37..0ea3457cae3 100755
--- a/t/t1406-submodule-ref-store.sh
+++ b/t/t1406-submodule-ref-store.sh
@@ -16,7 +16,7 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'pack_refs() not allowed' '
-   test_must_fail $RUN pack-refs 3
+   test_must_fail ok=sigabrt $RUN pack-refs 3
 '
 
 test_expect_success 'peel_ref(new-tag)' '
@@ -27,15 +27,18 @@ test_expect_success 'peel_ref(new-tag)' '
 '
 
 test_expect_success 'create_symref() not allowed' '
-   test_must_fail $RUN create-symref FOO refs/heads/master nothing
+   test_must_fail ok=sigabrt \
+   $RUN create-symref FOO refs/heads/master nothing
 '
 
 test_expect_success 'delete_refs() not allowed' '
-   test_must_fail $RUN delete-refs 0 nothing FOO refs/tags/new-tag
+   test_must_fail ok=sigabrt \
+   $RUN delete-refs 0 nothing FOO refs/tags/new-tag
 '
 
 test_expect_success 'rename_refs() not allowed' '
-   test_must_fail $RUN rename-ref refs/heads/master refs/heads/new-master
+   test_must_fail ok=sigabrt \
+   $RUN rename-ref refs/heads/master refs/heads/new-master
 '
 
 test_expect_success 'for_each_ref(refs/heads/)' '
@@ -91,11 +94,11 @@ test_expect_success 'reflog_exists(HEAD)' '
 '
 
 test_expect_success 'delete_reflog() not allowed' '
-   test_must_fail $RUN delete-reflog HEAD
+   test_must_fail ok=sigabrt $RUN delete-reflog HEAD
 '
 
 test_expect_success 'create-reflog() not allowed' '
-   test_must_fail $RUN create-reflog HEAD 1
+   test_must_fail ok=sigabrt $RUN create-reflog HEAD 1
 '
 
 test_done
-- 
2.17.0.windows.1.36.gdf4ca5fb72a

[PATCH 1/6] test_must_fail: support ok=sigabrt

[Johannes Schindelin]

In the upcoming patch, we will prepare t1406 to handle the conversion of
refs/files-backend.c to call BUG() instead of die("BUG: ..."). This will
require handling SIGABRT as valid failure case.

Signed-off-by: Johannes Schindelin 
---
 t/test-lib-functions.sh | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 7d620bf2a9a..926aefd1551 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -616,7 +616,7 @@ list_contains () {
 #   ok=[,<...>]:
 # Don't treat an exit caused by the given signal as error.
 # Multiple signals can be specified as a comma separated list.
-# Currently recognized signal names are: sigpipe, success.
+# Currently recognized signal names are: sigabrt, sigpipe, success.
 # (Don't use 'success', use 'test_might_fail' instead.)
 
 test_must_fail () {
@@ -636,6 +636,9 @@ test_must_fail () {
echo >&4 "test_must_fail: command succeeded: $*"
return 1
elif test_match_signal 13 $exit_code && list_contains "$_test_ok" 
sigpipe
+   then
+   return 0
+   elif test_match_signal 6 $exit_code && list_contains "$_test_ok" sigabrt
then
return 0
elif test $exit_code -gt 129 && test $exit_code -le 192
-- 
2.17.0.windows.1.36.gdf4ca5fb72a

[PATCH 0/6] Finish the conversion from die("BUG: ...") to BUG()

[Johannes Schindelin]

The BUG() macro was introduced in this patch series:
https://public-inbox.org/git/20170513032414.mfrwabt4hovuj...@sigill.intra.peff.net

The second patch in that series converted one caller from die("BUG: ")
to use the BUG() macro.

It seems that there was no concrete plan to address the same issue in
the rest of the code base.

This patch series tries to do that.

Note: I separated out 4/6 ("refs/*: report bugs using the BUG() macro")
from 5/6 ("Replace all die("BUG: ...") to keep it cuddled with the patch
2/6 that prepares t1406 for this change of refs/' behavior.

Note also: I would be very surprised if the monster commit 5/6 ("Replace
all die("BUG: ...") calls by BUG() ones") would apply cleanly on `pu` (I
develop this on top of `master`).

For that reason, the commit message contains the precise Unix shell
invocation (GNU sed semantics, not BSD sed ones, because I know that the
Git maintainer as well as the author of the patch introducing BUG() both
use Linux and not macOS or any other platform that would offer a BSD
sed). It should be straight-forward to handle merge
conflicts/non-applying patches by simply re-running that command.


Johannes Schindelin (6):
  test_must_fail: support ok=sigabrt
  t1406: prepare for the refs code to fail with BUG()
  refs/*: report bugs using the BUG() macro
  run-command: use BUG() to report bugs, not die()
  Replace all die("BUG: ...") calls by BUG() ones
  Convert remaining die*(BUG) messages

 apply.c  |  4 ++--
 archive-tar.c|  2 +-
 attr.c   | 10 +-
 blame.c  |  2 +-
 builtin/am.c | 20 +--
 builtin/branch.c |  2 +-
 builtin/cat-file.c   |  4 ++--
 builtin/clone.c  |  2 +-
 builtin/commit.c |  2 +-
 builtin/config.c |  2 +-
 builtin/fast-export.c|  2 +-
 builtin/fsck.c   |  2 +-
 builtin/index-pack.c |  4 ++--
 builtin/init-db.c|  2 +-
 builtin/ls-files.c   |  2 +-
 builtin/notes.c  |  8 
 builtin/pack-objects.c   |  4 ++--
 builtin/pull.c   |  2 +-
 builtin/receive-pack.c   |  2 +-
 builtin/replace.c|  2 +-
 builtin/update-index.c   |  2 +-
 bulk-checkin.c   |  2 +-
 color.c  |  4 ++--
 column.c |  2 +-
 config.c | 12 +--
 date.c   |  2 +-
 diff.c   | 12 +--
 dir-iterator.c   |  2 +-
 git-compat-util.h|  2 +-
 grep.c   | 16 +++
 http.c   |  8 
 imap-send.c  |  2 +-
 lockfile.c   |  2 +-
 mailinfo.c   |  2 +-
 merge-recursive.c| 12 +--
 notes-merge.c|  4 ++--
 pack-bitmap-write.c  |  2 +-
 pack-bitmap.c|  6 +++---
 pack-objects.c   |  2 +-
 packfile.c   |  6 +++---
 pathspec.c   | 12 +--
 pkt-line.c   |  2 +-
 prio-queue.c |  2 +-
 ref-filter.c |  4 ++--
 refs.c   | 34 
 refs/files-backend.c | 20 +--
 refs/iterator.c  |  6 +++---
 refs/packed-backend.c| 16 +++
 refs/ref-cache.c |  2 +-
 remote.c |  2 +-
 revision.c   |  4 ++--
 run-command.c| 33 ++-
 setup.c  |  4 ++--
 sha1-lookup.c|  2 +-
 sha1-name.c  |  4 ++--
 shallow.c|  6 +++---
 sigchain.c   |  2 +-
 strbuf.c |  4 ++--
 submodule.c  |  8 
 t/helper/test-example-decorate.c | 16 +++
 t/t1406-submodule-ref-store.sh   | 15 --
 t/test-lib-functions.sh  |  5 -
 tmp-objdir.c |  2 +-
 trailer.c|  6 +++---
 transport.c  |  4 ++--
 unpack-trees.c   |  2 +-
 vcs-svn/fast_export.c|  6 --
 worktree.c   |  2 +-
 wrapper.c|  4 ++--
 wt-status.c  | 20 +--
 zlib.c   |  4 ++--
 71 files changed, 220 insertions(+), 215 deletions(-)


base-commit: 1f1cddd558b54bb0ce19c8ace353fd07b758510d
Published-As: https://github.com/dscho/git/releases/tag/use-bug-macro-v1
Fetch-It-Via: git fetch https://github.com/dscho/git use-bug-macro-v1
-- 

Re: [PATCH v4 05/10] commit-graph: always load commit-graph information

[Jakub Narebski]

Derrick Stolee  writes:

> Most code paths load commits using lookup_commit() and then
> parse_commit().

And this automatically loads commit graph if needed, thanks to changes
in parse_commit_gently(), which parse_commit() uses.

> In some cases, including some branch lookups, the commit
> is parsed using parse_object_buffer() which side-steps parse_commit() in
> favor of parse_commit_buffer().

I guess the problem is that we cannot just add parse_commit_in_graph() 
like we did in parse_commit_gently(), for some reason?  Like for example
that parse_commit_gently() uses parse_commit_buffer() - which could have
been handled by moving parse_commit_in_graph() down the call chain from
parse_commit_gently() to parse_commit_buffer()... if not the fact that
check_commit() also uses parse_commit_buffer(), but it does not want to
load commit graph.  Am I right?

>
> With generation numbers in the commit-graph, we need to ensure that any
> commit that exists in the commit-graph file has its generation number
> loaded.

Is it generation number, or generation number and position in commit
graph?

>
> Create new load_commit_graph_info() method to fill in the information
> for a commit that exists only in the commit-graph file. Call it from
> parse_commit_buffer() after loading the other commit information from
> the given buffer. Only fill this information when specified by the
> 'check_graph' parameter.

I think this commit would be easier to review if it was split into pure
refactoring part (extracting fill_commit_graph_info() and
find_commit_in_graph()).  On the other hand the refactoring was needed
to reduce code duplication betweem existing parse_commit_in_graph() and
new load_commit_graph_info() functions.

I guess that the difference between parse_commit_in_graph() and
load_commit_graph_info() is that the former cares only about having just
enough information that is needed for parse_commit_gently() - and does
not load graph data if commit is parsed, while the latter is about
loading commit-graph data like generation numbers.

>
> Signed-off-by: Derrick Stolee 
> ---
>  commit-graph.c | 45 ++---
>  commit-graph.h |  8 
>  commit.c   |  7 +--
>  commit.h   |  2 +-
>  object.c   |  2 +-
>  sha1_file.c|  2 +-
>  6 files changed, 46 insertions(+), 20 deletions(-)

I wonder if it would be possible to add tests for this feature, for
example that commit-graph is read when it should (including those branch
lookups), and is not read when the feature should be disabled.

But the only way to test it I can think of is a stupid one: create
invalid commit graph, and check that git fails as expected (trying to
read said malformed file), and does not fail if commit graph feature is
disabled.

>

Let me reorder files (BTW, is there a way for Git to put *.h files
before *.c files in diff?) for easier review:

> diff --git a/commit-graph.h b/commit-graph.h
> index 260a468e73..96cccb10f3 100644
> --- a/commit-graph.h
> +++ b/commit-graph.h
> @@ -17,6 +17,14 @@ char *get_commit_graph_filename(const char *obj_dir);
>   */
>  int parse_commit_in_graph(struct commit *item);
>  
> +/*
> + * It is possible that we loaded commit contents from the commit buffer,
> + * but we also want to ensure the commit-graph content is correctly
> + * checked and filled. Fill the graph_pos and generation members of
> + * the given commit.
> + */
> +void load_commit_graph_info(struct commit *item);
> +
>  struct tree *get_commit_tree_in_graph(const struct commit *c);
>  
>  struct commit_graph {
> diff --git a/commit-graph.c b/commit-graph.c
> index 047fa9fca5..aebd242def 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -245,6 +245,12 @@ static struct commit_list **insert_parent_or_die(struct 
> commit_graph *g,
>   return _list_insert(c, pptr)->next;
>  }
>  
> +static void fill_commit_graph_info(struct commit *item, struct commit_graph 
> *g, uint32_t pos)
> +{
> + const unsigned char *commit_data = g->chunk_commit_data + 
> GRAPH_DATA_WIDTH * pos;
> + item->generation = get_be32(commit_data + g->hash_len + 8) >> 2;
> +}

The comment in the header file commit-graph.h talks about filling
graph_pos and generation members of the given commit, but I don't see
filling graph_pos member here.

Sidenote: it is a tiny little bit strange to see symbolic constants like
GRAPH_DATA_WIDTH near using magic values such as 8 and 2.

> +
>  static int fill_commit_in_graph(struct commit *item, struct commit_graph *g, 
> uint32_t pos)
>  {
>   uint32_t edge_value;
> @@ -292,31 +298,40 @@ static int fill_commit_in_graph(struct commit *item, 
> struct commit_graph *g, uin
>   return 1;
>  }
>  
> +static int find_commit_in_graph(struct commit *item, struct commit_graph *g, 
> uint32_t *pos)
> +{
> + if (item->graph_pos != COMMIT_NOT_FROM_GRAPH) {
> + *pos = item->graph_pos;
> + return 1;
> 

Re: [PATCH 2/6] sequencer: learn about the special "fake root commit" handling

[Stefan Beller]

Hi Johannes,

thanks for taking your time to explain things. It shows I am not
familiar with the rebase code, yet.

>
> Having said that, *this* time round, what we need to do is actually very
> similar to what builtin/am.c's read_author_script() does (even if we
> cannot use it as-is: it populates part of a `struct am_state`). I'll have
> to look into this more closely.

Heh, so my rambling was worth it. Thanks for looking into that.


>> This part could be prefixed with
>> /* un-escape text: turn \\ into ' and remove single quotes. */
>
> If could be prefixed that way, but it would be incorrect. We never turn \\
> into '. What we do here (and I do not want to repeat in a comment what the
> code does): we dequote what we previously quoted using single quotes. So
> we use the fact that we know that the value is of the form 'abc', or if it
> contains single quotes: 'this has '\''single'\'' quotes'.

Is there a helper 'dequote' somewhere?

>> > +/* Does this command create a (non-merge) commit? */
>> > +static int is_pick_or_similar(enum todo_command command)
>> > +{
>> > +   return command <= TODO_SQUASH;
>> > +}
>>
>> This code looks scary.
[...]

> The code already does things like that, by testing `command <
> TODO_COMMENT`.

ok great. So that is a widely used pattern for enum todo_command,
such that rearranging their order would break a lot. (Hence people will
find out quickly when doing so).

> But I guess your concerns would go away if I made this a big honking
> switch() statement that lists *explicitly* what should be considered "pick
> or similar"?

I did not spell that out as producing lots of lines of code is not the
primary goal, but understandability is.

And it looked like it would just pick the first section, so I thought about
some different approaches, either by making the enum todo_command
a lot more complex than an enum (an array of structs?) or adding
a new parallel array, that contains additional information for each
value at the given index, e.g.

static int is_pick_or_similar(enum todo_command command)
{
return todo_command_flags[value] & TODO_CMDS_IS_PICKING;
}

but all approaches I had were more complicated, such that the additional
verbosity would not be enough of a trade off.

I think this function was only scary as I was not familiar with the rebase
code as that employs similar patterns already.

>> > +   if (is_fixup(command))
>> > +   return error(_("cannot fixup root 
>> > commit"));
>>
>> I would expect you also cannot squash into root commit?
>
> In sequencer.c, `is_fixup()` really means "is it a fixup or a squash?". So
> yes, you are correct that we also cannot squash into a root commit.
>
> However, a squash is the same as a fixup with the only difference being
> that the squash lets you edit the final commit message (and does not
> comment out the squash commit's message, in contrast to fixup).
>
> Is it really worth adding an ugly line break in the middle of the error()
> statement just to say "cannot fixup/squash into root commit"? I'd rather
> not.

Makes sense,

Thanks for your patience,
Stefan

Re: [PATCH v3] unpack_trees: fix breakage when o->src_index != o->dst_index

[Johannes Schindelin]

Hi Duy,

On Sun, 29 Apr 2018, Duy Nguyen wrote:

> On Tue, Apr 24, 2018 at 8:50 AM, Elijah Newren  wrote:
> > Currently, all callers of unpack_trees() set o->src_index == o->dst_index.
> > The code in unpack_trees() does not correctly handle them being different.
> > There are two separate issues:
> >
> > First, there is the possibility of memory corruption.  Since
> > unpack_trees() creates a temporary index in o->result and then discards
> > o->dst_index and overwrites it with o->result, in the special case that
> > o->src_index == o->dst_index, it is safe to just reuse o->src_index's
> > split_index for o->result.  However, when src and dst are different,
> > reusing o->src_index's split_index for o->result will cause the
> > split_index to be shared.  If either index then has entries replaced or
> > removed, it will result in the other index referring to free()'d memory.
> >
> > Second, we can drop the index extensions.  Previously, we were moving
> > index extensions from o->dst_index to o->result.  Since o->src_index is
> > the one that will have the necessary extensions (o->dst_index is likely to
> > be a new index temporary index created to store the results), we should be
> > moving the index extensions from there.
> >
> > Signed-off-by: Elijah Newren 
> > ---
> >
> > Differences from v2:
> >   - Don't NULLify src_index until we're done using it
> >   - Actually built and tested[1]
> >
> > But it now passes the testsuite on both linux and mac[2], and I even 
> > re-merged
> > all 53288 merge commits in linux.git (with a merge of this patch together 
> > with
> > the directory rename detection series) for good measure.  [Only 7 commits
> > showed a difference, all due to directory rename detection kicking in.]
> >
> > [1] Turns out that getting all fancy with an m4.10xlarge and nice levels of
> > parallelization are great until you realize that your new setup omitted a
> > critical step, leaving you running a slightly stale version of git 
> > instead...
> > :-(
> >
> > [2] Actually, I get two test failures on mac from t0050-filesystem.sh, both
> > with unicode normalization tests, but those two tests fail before my changes
> > too.  All the other tests pass.
> >
> >  unpack-trees.c | 19 +++
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/unpack-trees.c b/unpack-trees.c
> > index e73745051e..49526d70aa 100644
> > --- a/unpack-trees.c
> > +++ b/unpack-trees.c
> > @@ -1284,9 +1284,20 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
> > struct unpack_trees_options
> > o->result.timestamp.sec = o->src_index->timestamp.sec;
> > o->result.timestamp.nsec = o->src_index->timestamp.nsec;
> > o->result.version = o->src_index->version;
> > -   o->result.split_index = o->src_index->split_index;
> > -   if (o->result.split_index)
> > +   if (!o->src_index->split_index) {
> > +   o->result.split_index = NULL;
> > +   } else if (o->src_index == o->dst_index) {
> > +   /*
> > +* o->dst_index (and thus o->src_index) will be discarded
> > +* and overwritten with o->result at the end of this 
> > function,
> > +* so just use src_index's split_index to avoid having to
> > +* create a new one.
> > +*/
> > +   o->result.split_index = o->src_index->split_index;
> > o->result.split_index->refcount++;
> > +   } else {
> > +   o->result.split_index = init_split_index(>result);
> > +   }
> > hashcpy(o->result.sha1, o->src_index->sha1);
> > o->merge_size = len;
> > mark_all_ce_unused(o->src_index);
> > @@ -1401,7 +1412,6 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
> > struct unpack_trees_options
> > }
> > }
> >
> > -   o->src_index = NULL;
> > ret = check_updates(o) ? (-2) : 0;
> > if (o->dst_index) {
> > if (!ret) {
> > @@ -1412,12 +1422,13 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
> > struct unpack_trees_options
> >   WRITE_TREE_SILENT |
> >   WRITE_TREE_REPAIR);
> > }
> > -   move_index_extensions(>result, o->dst_index);
> > +   move_index_extensions(>result, o->src_index);
> 
> While this looks like the right thing to do on paper, I believe it's
> actually broken for a specific case of untracked cache. In short,
> please do not touch this line. I will send a patch to revert
> edf3b90553 (unpack-trees: preserve index extensions - 2017-05-08),
> which essentially deletes this line, with proper explanation and
> perhaps a test if I could come up with one.
> 
> When we update the index, we depend on the fact that all updates must
> invalidate the right untracked cache correctly. In this unpack
> operations, we 

[PATCH 0/8] "git fetch" should not clobber existing tags without --force

[Ævar Arnfjörð Bjarmason]

On Fri, Apr 27 2018, Ævar Arnfjörð Bjarmason wrote:

> On Tue, Apr 24, 2018 at 9:57 PM, Wink Saville  wrote:
>> If have a repository with a tag "v1.0.0" and I add a remote repository
>> which also has a tag "v1.0.0" tag is overwritten.
>
> I feel like this thread has gotten somewhat side-tracked by the valid
> discussion about whether we should have remote tracking tags, but the
> much easier thing to fix is that the "+" prefix for refs/tags/* means
> nothing.
> [...]

This patch series implements that simpler way of digging ourselves out
of the immediate hole that we're clobbering tags by default without
the --force option.

I'm not 100% happy about this, but I think sans stuff that comes up in
review it's in principle ready for inclusion, stuff I wished I'd done
but have left for later:

 * Write a gitrefspec(5) man page, now we have how they work, and how
   they work on push/pull scattered over two docs, which before this
   are in conflict with one another.

 * Have much more exhaustive tests, I started trying to integrate this
   with the much more exhaustive tag pruning tests in my
   https://github.com/avar/git/tree/refspec-support-+-in-tags but gave
   up because the various interaction with those tests is messy,
   e.g. if we fail a tag update we don't prune as the existing tests
   assert, because the whole ref transaction fails.

 * 05/08 notes how the semantics of whether something needs a --force
   are really confusing because the rules are different depending on
   the ref namespace you're pushing into. We should probably build on
   top of this and e.g. refuse to clobber tags outside of refs/tags/*.

 * Should we do better to mitigate this breaking stuff for existing
   users who really are expecting their tags to be clobbered? Maybe by
   adding a --force-tags option (which wouldn't clobber branches), or
   have some config option either enable the old behavior, or make
   this opt-in?

Ævar Arnfjörð Bjarmason (8):
  push tests: remove redundant 'git push' invocation
  push tests: fix logic error in "push" test assertion
  push tests: add more testing for forced tag pushing
  push tests: assert re-pushing annotated tags
  push doc: correct lies about how push refspecs work
  fetch tests: correct a comment "remove it" -> "remove them"
  fetch tests: add a test clobbering tag behavior
  fetch: stop clobbering existing tags without --force

 Documentation/fetch-options.txt| 15 --
 Documentation/git-push.txt | 30 ---
 Documentation/gitrevisions.txt |  7 +--
 Documentation/pull-fetch-param.txt | 22 +---
 builtin/fetch.c| 20 +---
 t/t5510-fetch.sh   |  2 +-
 t/t5516-fetch-push.sh  | 82 ++
 t/t5612-clone-refspec.sh   |  4 +-
 8 files changed, 130 insertions(+), 52 deletions(-)

-- 
2.17.0.290.gded63e768a

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

[Ævar Arnfjörð Bjarmason]

Change "fetch" to treat "+" in refspecs (aka --force) to mean we
should clobber a local tag of the same name.

This changes the long-standing behavior of "fetch" added in
853a3697dc ("[PATCH] Multi-head fetch.", 2005-08-20), before this
change all tag fetches effectively had --force enabled. The original
rationale in that change was:

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

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

I the current behavior doesn't make sense, it easily results in local
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, 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.

This also makes it nicely symmetrical with how "tag" itself
works. We'll now refuse to clobber any existing tags unless "--force"
is supplied, whether that clobbering would happen by clobbering a
local tag with "tag", or by fetching it from the remote with "fetch".

It's still not at all nicely symmetrical with how "git push" works, as
discussed in the updated pull-fetch-param.txt documentation, but this
change brings them more into line with one another. I don't think
there's any reason "fetch" couldn't fully converge with the behavior
used by "push", but that's a topic for another change.

One of the tests added in 31b808a032 ("clone --single: limit the fetch
refspec to fetched branch", 2012-09-20) is being changed to use
--force where a clone would clobber a tag. This changes nothing about
the existing behavior of the test.

1. https://public-inbox.org/git/2023221658.ga22...@sigill.intra.peff.net/

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/fetch-options.txt| 15 ++-
 Documentation/pull-fetch-param.txt | 22 --
 builtin/fetch.c| 20 +---
 t/t5516-fetch-push.sh  |  5 +++--
 t/t5612-clone-refspec.sh   |  4 ++--
 5 files changed, 44 insertions(+), 22 deletions(-)

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
+   refuse to update the local branch as discussed
+ifdef::git-pull[]
+   in the `` part of the linkgit:git-fetch[1]
+   documentation.
+endif::git-pull[]
+ifndef::git-pull[]
+   in the `` part below.
+endif::git-pull[]
+   This option overrides that check.
 
 -k::
 --keep::
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, 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. Those apply the same fast-forward rule. An exception to
+this is that as of Git version 2.18 any object under `refs/tags/*` is
+protected from updates.
++
+If the optional plus `+` is used, the local ref is updated if the
+update would have otherwise been rejected.
 +
 [NOTE]
 When the remote branch you want to fetch is known to
diff --git a/builtin/fetch.c b/builtin/fetch.c
index dcdfc66f09..e3a44b582a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -126,7 +126,7 @@ 

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

[Ævar Arnfjörð Bjarmason]

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

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
)
 '
 
-- 
2.17.0.290.gded63e768a

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

[Ævar Arnfjörð Bjarmason]

There's complex rules governing whether a push is allowed to take
place depending on whether we're pushing to refs/heads/*, refs/tags/*
or refs/not-that/*. See is_branch() in refs.c, and the various
assertions in refs/files-backend.c. (e.g. "trying to write non-commit
object %s to branch '%s'").

This documentation has never been quite correct, but went downhill
after dbfeddb12e ("push: require force for refs under refs/tags/",
2012-11-29) when we started claiming that  couldn't be a tag
object, which is incorrect. After some of the logic in that patch was
changed in 256b9d70a4 ("push: fix "refs/tags/ hierarchy cannot be
updated without --force"", 2013-01-16) the docs weren't updated, and
we've had some version of documentation that confused whether 
was a tag or not with whether  would accept either an annotated
tag object or the commit it points to.

This makes the intro somewhat more verbose & complex, perhaps we
should have a shorter description here and split the full complexity
into a dedicated section. Very few users will find themselves needing
to e.g. push blobs or trees to refs/custom-namespace/* (or blobs or
trees at all), and that could be covered separately as an advanced
topic.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/git-push.txt | 30 ++
 Documentation/gitrevisions.txt |  7 ---
 2 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 5b08302fc2..806c3d8c65 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -60,8 +60,10 @@ OPTIONS[[OPTIONS]]
by a colon `:`, followed by the destination ref .
 +
 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/*`.
 +
 The  tells which ref on the remote side is updated with this
 push. Arbitrary expressions cannot be used here, an actual ref must
@@ -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
+`refs/*` the  reference lives. The `refs/heads/*` namespace will
+only accept commit objects, and then only they can be
+fast-forwarded. 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. 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. 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.
++
+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.
 +
 `tag ` means the same as `refs/tags/:refs/tags/`.
 +
diff --git a/Documentation/gitrevisions.txt b/Documentation/gitrevisions.txt
index 27dec5b91d..1b79cf1634 100644
--- a/Documentation/gitrevisions.txt
+++ b/Documentation/gitrevisions.txt
@@ -19,9 +19,10 @@ walk the revision graph (such as linkgit:git-log[1]), all 
commits which are
 reachable from that commit. For commands that walk the revision graph one can
 also specify a range of revisions explicitly.
 
-In addition, some Git commands (such as linkgit:git-show[1]) also take
-revision parameters which denote other objects than commits, e.g. blobs
-("files") or trees ("directories of files").
+In addition, some Git commands (such as linkgit:git-show[1] and
+linkgit:git-push[1]) can also take revision parameters which denote
+other objects than commits, e.g. blobs ("files") or trees
+("directories of files").
 
 include::revisions.txt[]
 
-- 
2.17.0.290.gded63e768a

[PATCH 7/8] fetch tests: add a test clobbering tag behavior

[Ævar Arnfjörð Bjarmason]

The test suite only incidentally (and unintentionally) tested for the
current behavior of eager tag clobbering on "fetch". This follow-up to
the previous "push tests: assert re-pushing annotated tags" change
tests for it explicitly.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/t5516-fetch-push.sh | 24 
 1 file changed, 24 insertions(+)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 71fc902062..9cf14c5cc1 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1003,6 +1003,30 @@ test_force_push_tag () {
 test_force_push_tag "lightweight tag" "-f"
 test_force_push_tag "annotated tag" "-f -a -m'msg'"
 
+test_force_fetch_tag () {
+   tag_type_description=$1
+   tag_args=$2
+
+   test_expect_success "fetch will clobber an existing 
$tag_type_description" "
+   mk_test testrepo heads/master &&
+   mk_child testrepo child1 &&
+   mk_child testrepo child2 &&
+   (
+   cd testrepo &&
+   git tag Tag &&
+   git -C ../child1 fetch origin tag Tag &&
+   >file1 &&
+   git add file1 &&
+   git commit -m 'file1' &&
+   git tag $tag_args Tag &&
+   git -C ../child1 fetch origin tag Tag
+   )
+   "
+}
+
+test_force_fetch_tag "lightweight tag" "-f"
+test_force_fetch_tag "annotated tag" "-f -a -m'msg'"
+
 test_expect_success 'push --porcelain' '
mk_empty testrepo &&
echo >.git/foo  "To testrepo" &&
-- 
2.17.0.290.gded63e768a

[PATCH 6/8] fetch tests: correct a comment "remove it" -> "remove them"

[Ævar Arnfjörð Bjarmason]

Correct a comment referring to the removal of just the branch to also
refer to the tag. This should have been changed in my
ca3065e7e7 ("fetch tests: add a tag to be deleted to the pruning
tests", 2018-02-09) when the tag deletion was added, but I missed it
at the time.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/t5510-fetch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index ae5a530a2d..9bd2783521 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -613,7 +613,7 @@ test_configured_prune_type () {
git rev-parse --verify refs/tags/newtag
) &&
 
-   # now remove it
+   # now remove them
git branch -d newbranch &&
git tag -d newtag &&
 
-- 
2.17.0.290.gded63e768a

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

[Ævar Arnfjörð Bjarmason]

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.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/t5516-fetch-push.sh | 66 ---
 1 file changed, 37 insertions(+), 29 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index c9a2011915..71fc902062 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -965,35 +965,43 @@ test_expect_success 'push into aliased refs 
(inconsistent)' '
)
 '
 
-test_expect_success 'push requires --force to update lightweight tag' '
-   mk_test testrepo heads/master &&
-   mk_child testrepo child1 &&
-   mk_child testrepo child2 &&
-   (
-   cd child1 &&
-   git tag Tag &&
-   git push ../child2 Tag &&
-   >file1 &&
-   git add file1 &&
-   git commit -m "file1" &&
-   git tag -f Tag &&
-   test_must_fail git push ../child2 Tag &&
-   git push --force ../child2 Tag &&
-   git tag -f Tag HEAD~ &&
-   test_must_fail git push ../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
-   )
-'
+test_force_push_tag () {
+   tag_type_description=$1
+   tag_args=$2
+
+   test_expect_success "push requires --force to update 
$tag_type_description" "
+   mk_test testrepo heads/master &&
+   mk_child testrepo child1 &&
+   mk_child testrepo child2 &&
+   (
+   cd child1 &&
+   git tag Tag &&
+   git push ../child2 Tag &&
+   >file1 &&
+   git add file1 &&
+   git commit -m 'file1' &&
+   git tag $tag_args Tag &&
+   test_must_fail git push ../child2 Tag &&
+   git push --force ../child2 Tag &&
+   git tag $tag_args Tag HEAD~ &&
+   test_must_fail git push ../child2 Tag &&
+   git push --force ../child2 Tag &&
+   git tag $tag_args Tag &&
+   test_must_fail git push ../child2 
'refs/tags/*:refs/tags/*' &&
+   git push --force ../child2 'refs/tags/*:refs/tags/*' &&
+   git tag $tag_args Tag HEAD~ &&
+   git push ../child2 '+refs/tags/*:refs/tags/*' &&
+   git tag $tag_args Tag &
+   git push --no-force ../child2 
'+refs/tags/*:refs/tags/*' &&
+   git tag $tag_args Tag HEAD~ &&
+   test_must_fail git push ../child2 tag Tag &&
+   git push --force ../child2 tag Tag
+   )
+   "
+}
+
+test_force_push_tag "lightweight tag" "-f"
+test_force_push_tag "annotated tag" "-f -a -m'msg'"
 
 test_expect_success 'push --porcelain' '
mk_empty testrepo &&
-- 
2.17.0.290.gded63e768a

[PATCH 1/8] push tests: remove redundant 'git push' invocation

[Ævar Arnfjörð Bjarmason]

Remove an invocation of 'git push' that's exactly the same as the one
on the preceding line. This was seemingly added by mistake in
dbfeddb12e ("push: require force for refs under refs/tags/",
2012-11-29) and doesn't affect the result of the test, the second
"push" was a no-op as there was nothing new to push.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/t5516-fetch-push.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 82239138d5..7b5a553398 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -973,7 +973,6 @@ test_expect_success 'push requires --force to update 
lightweight tag' '
cd child1 &&
git tag Tag &&
git push ../child2 Tag &&
-   git push ../child2 Tag &&
>file1 &&
git add file1 &&
git commit -m "file1" &&
-- 
2.17.0.290.gded63e768a

[PATCH 2/8] push tests: fix logic error in "push" test assertion

[Ævar Arnfjörð Bjarmason]

Fix a logic error that's been here since this test was added in
dbfeddb12e ("push: require force for refs under refs/tags/",
2012-11-29).

The intent of this test is to force-create a new tag pointing to
HEAD~, and then assert that pushing it doesn't work without --force.

Instead, the code was not creating a new tag at all, and then failing
to push the previous tag for the unrelated reason of providing a
refspec that doesn't make any sense.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/t5516-fetch-push.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 7b5a553398..15c8d5a734 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -979,8 +979,8 @@ test_expect_success 'push requires --force to update 
lightweight tag' '
git tag -f Tag &&
test_must_fail git push ../child2 Tag &&
git push --force ../child2 Tag &&
-   git tag -f Tag &&
-   test_must_fail git push ../child2 Tag HEAD~ &&
+   git tag -f Tag HEAD~ &&
+   test_must_fail git push ../child2 Tag &&
git push --force ../child2 Tag
)
 '
-- 
2.17.0.290.gded63e768a

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

[Duy Nguyen]

Phillip (and others) the changes in this patch make "git help -g" now
lists a lot more guides than just the "common" one as advertised (see
below for the exact list). The man page for "git help -g" also
mentions that it would list "useful" guides, not all guides. But we
have no way to list all guides as far as I can tell.

I guess we have two options forward:

- keep "help -g" to common guide (we can tag common guides in
command-list.txt) and add a new option to list all guides ("help
-ag"?)
- reword the man page to make "help -g" list all guides

I'm ok with either direction. What's your preference?

For comparison, this is the new output

The common Git guides are:
   attributes  Defining attributes per path
   cli Git command-line interface and conventions
   core-tutorial   A Git core tutorial for developers
   cvs-migration   Git for CVS users
   diffcoreTweaking diff output
   everydayA useful minimum set of commands for Everyday
Git
   glossaryA Git Glossary
   hooks   Hooks used by Git
   ignore  Specifies intentionally untracked files to
ignore
   modules Defining submodule properties
   namespaces  Git namespaces
   repository-layout   Git Repository Layout
   revisions   Specifying revisions and ranges for Git
   tutorialA tutorial introduction to Git
   tutorial-2  A tutorial introduction to Git: part two
   workflows   An overview of recommended workflows with Git

compared to the old version

The common Git guides are:

   attributes   Defining attributes per path
   everyday Everyday Git With 20 Commands Or So
   glossary A Git glossary
   ignore   Specifies intentionally untracked files to ignore
   modules  Defining submodule properties
   revisionsSpecifying revisions and ranges for Git
   tutorial A tutorial introduction to Git (for version 1.5.1 or
newer)
   workflowsAn overview of recommended workflows with Git




On Sun, Apr 29, 2018 at 8:18 PM, Nguyễn Thái Ngọc Duy  wrote:
> 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.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  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 | 18 ---
>  help.h |  1 +
>  t/t0012-help.sh|  6 +
>  10 files changed, 52 insertions(+), 44 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 71b5b594cd..18696e35b0 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1939,7 +1939,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 83a7d73afe..b58e8d5f6a 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 

[PATCH v5 03/10] 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.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 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 bb29470f88..ab67150e68 100644
--- a/Makefile
+++ b/Makefile
@@ -1916,9 +1916,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 a4feef2ffe..bf2738e9ef 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 

[PATCH v5 08/10] 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.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git-help.txt |  4 +++-
 builtin/help.c |  7 +++
 help.c | 16 
 help.h |  1 +
 t/t0012-help.sh|  9 +
 5 files changed, 36 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 2d51071429..83a7d73afe 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 156dde1bea..f9da0214f1 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)
 {
@@ -351,6 +362,11 @@ void list_cmds_by_category(const char *cat)
}
 }
 
+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 752368840a..090d46ba01 100644
--- a/help.h
+++ b/help.h
@@ -17,6 +17,7 @@ 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(void);
 extern void list_all_other_cmds(void);
 extern void list_cmds_by_category(const char *category);
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.664.g8924eee37a

[PATCH v5 09/10] 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.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 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 | 18 ---
 help.h |  1 +
 t/t0012-help.sh|  6 +
 10 files changed, 52 insertions(+), 44 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 71b5b594cd..18696e35b0 100644
--- a/Makefile
+++ b/Makefile
@@ -1939,7 +1939,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 83a7d73afe..b58e8d5f6a 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
+gitrevisions

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

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

I think v5 is getting close to what I wanted to achieve from the RFC
version (I skip v4 since I sent out v4/wip and another v4 may confuse
people).

Interdiff is too large to be helpful, but the summary of changes
compared to v3 is:

- common-cmds.h is renamed to command-list.h
- the common group description is moved from command-list.txt to
  help.c to simplify command-list.txt format
- generate-cmds.sh supports multiple categories per command, a new one
  "complete" is added to aid git-completion.bash
- multiple --list-cmds options is replaced with
  --list-cmds=[,...]. This allows easier group
  customization in git-completion.bash (not happens yet)
- __git_list_all_commands() and __git_list_porcelain_commands() for
  backward compatibility
- "git help " completion also makes use of guide list in
  command-list.txt
- better tests from Ramsay

There is one sticky point yet about the guides. I'll pull Phillip in
and explain more in the relevant patch.

Nguyễn Thái Ngọc Duy (10):
  generate-cmds.sh: factor out synopsis extract code
  generate-cmds.sh: export all commands to command-list.h
  help: use command-list.h for common command list
  Remove common-cmds.h
  git.c: convert --list-*builtins to --list-cmds=*
  completion: implement and use --list-cmds=main,others
  git: support --list-cmds=list-
  help: add "-a --verbose" to list all commands with synopsis
  help: use command-list.txt for the source of guides
  completion: let git provide the completable command list

 .gitignore |   2 +-
 Documentation/git-help.txt |   4 +-
 Documentation/gitattributes.txt|   2 +-
 Documentation/gitmodules.txt   |   2 +-
 Documentation/gitrevisions.txt |   2 +-
 Makefile   |  16 +-
 builtin/help.c |  39 +
 command-list.txt   |  67 
 contrib/completion/git-completion.bash | 134 +---
 generate-cmdlist.sh| 126 ++-
 git.c  |  38 -
 help.c | 209 +
 help.h |   5 +
 t/t0012-help.sh|  26 ++-
 t/t9902-completion.sh  |   5 +-
 15 files changed, 426 insertions(+), 251 deletions(-)

-- 
2.17.0.664.g8924eee37a

[PATCH v5 02/10] 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.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 .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 50da82b016..bb29470f88 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 \
@@ -1940,6 +1940,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):\
@@ -2150,7 +2155,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.
 
@@ -2529,7 +2534,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"; \
@@ -2777,7 +2782,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 () {
}' 

[PATCH v5 07/10] 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.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 generate-cmdlist.sh | 17 +
 git.c   |  7 +++
 help.c  | 22 ++
 help.h  |  1 +
 4 files changed, 47 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 da161a74ae..93927e34c4 100644
--- a/git.c
+++ b/git.c
@@ -52,6 +52,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 71bc001570..156dde1bea 100644
--- a/help.c
+++ b/help.c
@@ -329,6 +329,28 @@ void list_all_other_cmds(void)
clean_cmdnames(_cmds);
 }
 
+void list_cmds_by_category(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)
+   puts(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 30e165773e..752368840a 100644
--- a/help.h
+++ b/help.h
@@ -19,6 +19,7 @@ static inline void mput_char(char c, unsigned int num)
 extern void list_common_cmds_help(void);
 extern void list_all_main_cmds(void);
 extern void list_all_other_cmds(void);
+extern void list_cmds_by_category(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.664.g8924eee37a

[PATCH v5 04/10] 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.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 .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 ab67150e68..71b5b594cd 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 \
@@ -1916,9 +1916,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)"' \
@@ -1937,11 +1937,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)
@@ -2155,7 +2150,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.
 
@@ -2534,7 +2529,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"; \
@@ -2782,7 +2777,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.664.g8924eee37a

[PATCH v5 05/10] git.c: convert --list-*builtins 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.

This also allows combining multiple listing types, which is usually
now (for combining parseopt and builtins) but future types will
benefit from this.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 contrib/completion/git-completion.bash |  2 +-
 git.c  | 27 --
 t/t0012-help.sh|  2 +-
 3 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 01dd9ff07a..d7b448fd94 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3044,7 +3044,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 f598fae7b7..ea9bbfb6a3 100644
--- a/git.c
+++ b/git.c
@@ -38,6 +38,25 @@ 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 if (len == 8 && !strncmp(spec, "parseopt", 8))
+   list_builtins(NO_PARSEOPT, ' ');
+   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 +242,8 @@ 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=", )) {
+   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.664.g8924eee37a

[PATCH v5 06/10] 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.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 contrib/completion/git-completion.bash |  2 +-
 git.c  |  4 
 help.c | 32 ++
 help.h |  2 ++
 4 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index d7b448fd94..77cfb8a20b 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -834,7 +834,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 ea9bbfb6a3..da161a74ae 100644
--- a/git.c
+++ b/git.c
@@ -48,6 +48,10 @@ static int list_cmds(const char *spec)
list_builtins(0, '\n');
else if (len == 8 && !strncmp(spec, "parseopt", 8))
list_builtins(NO_PARSEOPT, ' ');
+   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 bf2738e9ef..71bc001570 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(void)
+{
+   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++)
+   puts(main_cmds.names[i]->name);
+
+   clean_cmdnames(_cmds);
+   clean_cmdnames(_cmds);
+}
+
+void list_all_other_cmds(void)
+{
+   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++)
+   puts(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..30e165773e 100644
--- a/help.h
+++ b/help.h
@@ -17,6 +17,8 @@ static inline void mput_char(char c, unsigned int num)
 }
 
 extern void list_common_cmds_help(void);
+extern void list_all_main_cmds(void);
+extern void list_all_other_cmds(void);
 extern const char *help_unknown_cmd(const char *cmd);
 extern void load_command_list(const char *prefix,
  struct cmdnames *main_cmds,
-- 
2.17.0.664.g8924eee37a

[PATCH v5 10/10] 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.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 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 99ddc231c1..40776b9587 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -3,11 +3,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
@@ -17,7 +17,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
@@ -25,7 +25,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
@@ -39,7 +39,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
@@ -48,20 +48,20 @@ git-filter-branch   
ancillarymanipulators
 git-fmt-merge-msg   purehelpers
 git-for-each-refplumbinginterrogators
 git-format-patch   

[PATCH v5 01/10] 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).

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 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.664.g8924eee37a

Re: [PATCH v3] unpack_trees: fix breakage when o->src_index != o->dst_index

[Duy Nguyen]

On Tue, Apr 24, 2018 at 8:50 AM, Elijah Newren  wrote:
> Currently, all callers of unpack_trees() set o->src_index == o->dst_index.
> The code in unpack_trees() does not correctly handle them being different.
> There are two separate issues:
>
> First, there is the possibility of memory corruption.  Since
> unpack_trees() creates a temporary index in o->result and then discards
> o->dst_index and overwrites it with o->result, in the special case that
> o->src_index == o->dst_index, it is safe to just reuse o->src_index's
> split_index for o->result.  However, when src and dst are different,
> reusing o->src_index's split_index for o->result will cause the
> split_index to be shared.  If either index then has entries replaced or
> removed, it will result in the other index referring to free()'d memory.
>
> Second, we can drop the index extensions.  Previously, we were moving
> index extensions from o->dst_index to o->result.  Since o->src_index is
> the one that will have the necessary extensions (o->dst_index is likely to
> be a new index temporary index created to store the results), we should be
> moving the index extensions from there.
>
> Signed-off-by: Elijah Newren 
> ---
>
> Differences from v2:
>   - Don't NULLify src_index until we're done using it
>   - Actually built and tested[1]
>
> But it now passes the testsuite on both linux and mac[2], and I even re-merged
> all 53288 merge commits in linux.git (with a merge of this patch together with
> the directory rename detection series) for good measure.  [Only 7 commits
> showed a difference, all due to directory rename detection kicking in.]
>
> [1] Turns out that getting all fancy with an m4.10xlarge and nice levels of
> parallelization are great until you realize that your new setup omitted a
> critical step, leaving you running a slightly stale version of git instead...
> :-(
>
> [2] Actually, I get two test failures on mac from t0050-filesystem.sh, both
> with unicode normalization tests, but those two tests fail before my changes
> too.  All the other tests pass.
>
>  unpack-trees.c | 19 +++
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index e73745051e..49526d70aa 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1284,9 +1284,20 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
> struct unpack_trees_options
> o->result.timestamp.sec = o->src_index->timestamp.sec;
> o->result.timestamp.nsec = o->src_index->timestamp.nsec;
> o->result.version = o->src_index->version;
> -   o->result.split_index = o->src_index->split_index;
> -   if (o->result.split_index)
> +   if (!o->src_index->split_index) {
> +   o->result.split_index = NULL;
> +   } else if (o->src_index == o->dst_index) {
> +   /*
> +* o->dst_index (and thus o->src_index) will be discarded
> +* and overwritten with o->result at the end of this function,
> +* so just use src_index's split_index to avoid having to
> +* create a new one.
> +*/
> +   o->result.split_index = o->src_index->split_index;
> o->result.split_index->refcount++;
> +   } else {
> +   o->result.split_index = init_split_index(>result);
> +   }
> hashcpy(o->result.sha1, o->src_index->sha1);
> o->merge_size = len;
> mark_all_ce_unused(o->src_index);
> @@ -1401,7 +1412,6 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
> struct unpack_trees_options
> }
> }
>
> -   o->src_index = NULL;
> ret = check_updates(o) ? (-2) : 0;
> if (o->dst_index) {
> if (!ret) {
> @@ -1412,12 +1422,13 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
> struct unpack_trees_options
>   WRITE_TREE_SILENT |
>   WRITE_TREE_REPAIR);
> }
> -   move_index_extensions(>result, o->dst_index);
> +   move_index_extensions(>result, o->src_index);

While this looks like the right thing to do on paper, I believe it's
actually broken for a specific case of untracked cache. In short,
please do not touch this line. I will send a patch to revert
edf3b90553 (unpack-trees: preserve index extensions - 2017-05-08),
which essentially deletes this line, with proper explanation and
perhaps a test if I could come up with one.

When we update the index, we depend on the fact that all updates must
invalidate the right untracked cache correctly. In this unpack
operations, we start copying entries over from src to result. Since
'result' (at least from the beginning) does not have an untracked
cache, it has nothing to invalidate when we copy entries over. By the
time we have done preparing 'result', what's recorded in src's (or
dst's for 

[PATCH v3 0/1] completion: dynamic completion loading

[Florian Gamböck]

In this small patch I want to introduce a way to dynamically load completion 
scripts for external subcommands.

A few years ago, you would put a completion script (which defines a Bash 
function _git_foo for a custom git subcommand foo) into 
/etc/bash_completion.d, or save it somewhere in your $HOME and source it 
manually in your .bashrc.

Starting with bash-completion v2.0 (or, to be absolutely exact, the preview 
versions started at v1.90), completion scripts are not sourced at bash startup 
anymore. Rather, when typing in a command and trigger completion by pressing 
the TAB key, the new bash-completion's main script looks for a script with the 
same name as the typed command in the completion directory, sources it, and 
provides the completions defined in this script.

However, that only works for top level commands. After a completion script has 
been found, the logic is delegated to this script. This means, that the 
completion of subcommands must be handled by the corresponding completion 
script.

As it is now, git is perfectly able to call custom completion functions, iff 
they are already defined when calling the git completion. With my patch, git 
uses an already defined loader function of bash-completion (or continues 
silently if this function is not found), loads an external completion script, 
and provides its completions.

An example for an external completion script would be 
/usr/share/bash-completion/completions/git-foo for a git subcommand foo.

Changes since v2:

-   Replaced __load_completion with _completion_loader, which was introduced 
way before the former and should therefor be available even in older 
distributions

-   Updated commit message with explanations from Szeder Gábor

Changes since v1 (RFC):

-   Re-arranged if-fi statement to increase readability (suggested by Junio C 
Hamano)

-   Re-worded commit message to include the exakt version of bashcomp that 
introduced dynamic loading (suggested by Stefan Beller)

Florian Gamböck (1):
  completion: load completion file for external subcommand

 contrib/completion/git-completion.bash | 10 ++
 1 file changed, 10 insertions(+)

-- 
2.16.1

[PATCH v3 1/1] completion: load completion file for external subcommand

[Florian Gamböck]

Adding external subcommands to Git is as easy as to put an executable
file git-foo into PATH. Packaging such subcommands for a Linux
distribution can be achieved by unpacking the executable into /usr/bin
of the user's system. Adding system-wide completion scripts for new
subcommands, however, can be a bit tricky.

Since bash-completion started to use dynamical loading of completion
scripts since v1.90 (preview of v2.0), it is no longer sufficient to
drop a completion script of a subcommand into the standard completions
path, /usr/share/bash-completion/completions, since this script will not
be loaded if called as a git subcommand.

For example, look at https://bugs.gentoo.org/544722. To give a short
summary: The popular git-flow subcommand provides a completion script,
which gets installed as /usr/share/bash-completion/completions/git-flow.

If you now type into a Bash shell:

git flow 

You will not get any completions, because bash-completion only loads
completions for git and git has no idea that git-flow is defined in
another file. You have to load this script manually or trigger the
dynamic loader with:

git-flow  # Please notice the dash instead of whitespace

This will not complete anything either, because it only defines a Bash
function, without generating completions. But now the correct completion
script has been loaded and the first command can use the completions.

So, the goal is now to teach the git completion script to consider the
possibility of external completion scripts for subcommands, but of
course without breaking current workflows.

I think the easiest method is to use a function that was defined by
bash-completion v1.90, namely _completion_loader. It will take care of
loading the correct script if present. Afterwards, the git completion
script behaves as usual.

_completion_loader was introduced in commit 20c05b43 of bash-completion
(https://github.com/scop/bash-completion.git) back in 2011, so it should
be available in even older LTS distributions. This function searches for
external completion scripts not only in the default path
/usr/share/bash-completion/completions, but also in the user's home
directory via $XDG_DATA_HOME and in a user specified directory via
$BASH_COMPLETION_USER_DIR.

The only "drawback" (if it even can be called as such) is, that if
_completion_loader does not find a completion script, it automatically
registers a minimal function for basic path completion. In practice,
however, this will not matter, because in this case the given command is
a git command in its dashed form, e.g. 'git-diff-index', and those have
been deprecated for a long time.

This way we can leverage bash-completion's dynamic loading for git
subcommands and make it easier for developers to distribute custom
completion scripts.

Signed-off-by: Florian Gamböck 
---
 contrib/completion/git-completion.bash | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index b09c8a236..604ba2b03 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3096,12 +3096,22 @@ __git_main ()
fi
 
local completion_func="_git_${command//-/_}"
+   if ! declare -f $completion_func >/dev/null 2>/dev/null &&
+   declare -f _completion_loader >/dev/null 2>/dev/null
+   then
+   _completion_loader "git-$command"
+   fi
declare -f $completion_func >/dev/null 2>/dev/null && $completion_func 
&& return
 
local expansion=$(__git_aliased_command "$command")
if [ -n "$expansion" ]; then
words[1]=$expansion
completion_func="_git_${expansion//-/_}"
+   if ! declare -f $completion_func >/dev/null 2>/dev/null &&
+   declare -f _completion_loader >/dev/null 2>/dev/null
+   then
+   _completion_loader "git-$expansion"
+   fi
declare -f $completion_func >/dev/null 2>/dev/null && 
$completion_func
fi
 }
-- 
2.16.1

Re: [PATCH v4/wip 02/12] generate-cmds.sh: export all commands to command-list.h

[Duy Nguyen]

On Wed, Apr 25, 2018 at 8:07 PM, Eric Sunshine  wrote:
> On Wed, Apr 25, 2018 at 12:30 PM, Nguyễn Thái Ngọc Duy
>  wrote:
>> 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:
>> [...]
>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>> ---
>> diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
>> @@ -12,14 +34,51 @@ get_synopsis () {
>> +define_categories() {
>> +   echo
>> +   echo "/* Command categories */"
>> +   bit=0
>> +   category_list "$1" |
>> +   while read cat
>> +   do
>> +   echo "#define CAT_$cat (1UL << $bit)"
>> +   bit=$(($bit+1))
>> +   done
>> +   test "$bit" -gt 32 && die "Urgh.. too many categories?"
>
> Should this be '-ge' rather than '-gt'?

After we print "1UL << 31" we increment it to 32 and exit the loop,
then it's still within limits, -ge would incorrectly complain
-- 
Duy

Proposal

[Zeliha Omer faruk]

Hello Dear

Greetings to you, please I have a very important business proposal for our
mutual benefit, please let me know if you are interested.

Best Regards,
Miss. Zeliha ömer Faruk
Caddesi Kristal Kule Binasi
No:215

Re: [PATCH v4 04/10] commit: use generations in paint_down_to_common()

[Jakub Narebski]

Derrick Stolee  writes:

> Define compare_commits_by_gen_then_commit_date(), which uses generation
> numbers as a primary comparison and commit date to break ties (or as a
> comparison when both commits do not have computed generation numbers).

All right, this looks reasonable thing to do when we have access to
commit generation numbers..

> Since the commit-graph file is closed under reachability, we know that
> all commits in the file have generation at most GENERATION_NUMBER_MAX
> which is less than GENERATION_NUMBER_INFINITY.

Thus the condition that if B is reachable from A, then gen(A) >= gen(B),
even if they have generation numbers _INFINITY, _MAX or _ZERO.

We use generation numbers, if possible, to choose closest commit; if
not, we use dates.

>
> This change does not affect the number of commits that are walked during
> the execution of paint_down_to_common(), only the order that those
> commits are inspected. In the case that commit dates violate topological
> order (i.e. a parent is "newer" than a child), the previous code could
> walk a commit twice: if a commit is reached with the PARENT1 bit, but
> later is re-visited with the PARENT2 bit, then that PARENT2 bit must be
> propagated to its parents. Using generation numbers avoids this extra
> effort, even if it is somewhat rare.

Actually the ordering of commits walked does not affect the correctness
of the result.  Better ordering means that commits do not need to be
walked twice; I think it would be possible to craft repository in which
unlucky clock skew would lead to depth-first walk of commits later part
of walk would mark STALE.

Pedantry aside, I think it is a good description of analysis of change
results.

>
> Signed-off-by: Derrick Stolee 
> ---
>  commit.c | 20 +++-
>  commit.h |  1 +
>  2 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/commit.c b/commit.c
> index 711f674c18..4d00b0a1d6 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -640,6 +640,24 @@ static int compare_commits_by_author_date(const void 
> *a_, const void *b_,
>   return 0;
>  }
>  
> +int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, 
> void *unused)
> +{
> + const struct commit *a = a_, *b = b_;
> +
> + /* newer commits first */

To be pedantic, larger generation number does not necessary mean that
commit was created later (is newer), only that it is on longer chain
since common ancestor or root commit.

> + if (a->generation < b->generation)
> + return 1;
> + else if (a->generation > b->generation)
> + return -1;

If the commit-graph feature is not available, or is disabled, all
commits would have the same generation number (_INFINITY), then this
block would be always practically no-op.

This is not very costly: 2 access to data which should be in cache, and
1 to 2 comparison operations.  But I wonder if we wouldn't want to avoid
this nano-cost if possible...

> +
> + /* use date as a heuristic when generations are equal */
> + if (a->date < b->date)
> + return 1;
> + else if (a->date > b->date)
> + return -1;
> + return 0;

The above is the same code as in compare_commits_by_commit_date(), but
there it is with "newer commits with larger date first" as comment
instead.

All right: we need inlining for speed.

> +}
> +
>  int compare_commits_by_commit_date(const void *a_, const void *b_, void 
> *unused)
>  {
>   const struct commit *a = a_, *b = b_;
> @@ -789,7 +807,7 @@ static int queue_has_nonstale(struct prio_queue *queue)
>  /* all input commits in one and twos[] must have been parsed! */
>  static struct commit_list *paint_down_to_common(struct commit *one, int n, 
> struct commit **twos)
>  {
> - struct prio_queue queue = { compare_commits_by_commit_date };
> + struct prio_queue queue = { compare_commits_by_gen_then_commit_date };

I wonder if it would be worth it to avoid comparing by generation
numbers without commit graph data:

  + struct prio_queue queue;
  [...]
  + if (commit_graph)
  + queue.compare = compare_commits_by_gen_then_commit_date;
  + else
  + queue.compare = compare_commits_by_commit_date;

Or something like that.  But perhaps this nano-optimization is not worth
it (it is not that complicated, though).


Sidenote: when I searched for compare_commits_by_commit_date use, I have
noticed that it is used, I think as heuristics, for packfile creation in
upload-pack.c and fetch-pack.c.  Would they similarly improve with
compare_commits_by_gen_then_commit_date?

This is of course not something that this commit, or this patch series,
needs to address now.

>   struct commit_list *result = NULL;
>   int i;
>  
> diff --git a/commit.h b/commit.h
> index aac3b8c56f..64436ff44e 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -341,6 +341,7 @@ extern int remove_signature(struct strbuf *buf);
>  extern int 

Re: [PATCH v2 1/1] completion: load completion file for external subcommand

[Florian Gamböck]

On 2018-04-29 15:08, SZEDER Gábor wrote:
On Sun, Apr 29, 2018 at 1:15 PM, Florian Gamböck  
wrote:
I sense a problem here. If I have a directory with a file xyzfoobar 
in it, and I type `git xyz`, with no defined subcommand that starts 
with these letters, then minimal bashcomp would give me `git 
xyzfoobar`, which can of course not execute. This can be unintuitive 
for users, as in: "If it can't be executed correctly, then why does 
it even suggest such a completion?"


I'm not sure I understand the problem.  After 'git xyz' (note 
there is no space between 'xyz' and ) we try to complete the name 
of a git command, not options of a git command.  This means:


 - At this point we don't look for a _git_xyz() function, so we'll 
 return from __git_main() before even reaching the piece of code your 
 patch modifies.


 - There are (presumably) no commands starting with 'xyz', so we don't 
 list any commands.  Bash will then fall back to its own filename 
 completion, and that's where that 'xyzfoobar' will come from.  It has 
 been behaving like this basically since forever.


And after 'git xyz ' (this time with space) we already complete 
the next word, not 'xyz'.


You are absolutely right! I don't know what my brain was making up here, 
I am sorry. The minimal completion will come up regardless if no valid 
completion can be found. I think I mixed up the meaning of $cword in 
__git_main. It is correct, if I want to complete `git xyz`, then my 
patch is never reached.


I think all you need to do is run a 
s/__load_completion/_completion_loader/ on your patch and update the 
commit message with relevant bits from the above discussion.


I can do that, no problem. But prior to that I want to be sure that 
you are okay with the above mentioned drawback. Will the behavior be 
acceptable in this case? Or should we try to somehow "undo" the 
minimal completion afterwards?


As explained above, I don't think there is any drawback here.  Or at 
least not any new drawback that your patch is introducing.  Or I'm 
completely missing your point; certainly a possibility, it's early 
Sunday afternoon, after all :)


Okay, then I'll prepare the next round. Thank you very much for your 
helpful feedback!

Re: [PATCH v2 1/1] completion: load completion file for external subcommand

[SZEDER Gábor]

On Sun, Apr 29, 2018 at 1:15 PM, Florian Gamböck  wrote:
> On 2018-04-25 16:40, SZEDER Gábor wrote:
>>
>> In my previous emails I overlooked the _completion_loader() helper
>> function.
>>
>> It seems that this function does almost exactly what we want.  It was
>> introduced along with dynamic completion loading back in 20c05b43, so it's
>> available for us even in older LTS/Enterprise releases.  Since cad3abfc it's
>> a wrapper around __load_completion() and thus benefits from all the
>> improvements, notably searching for completion scripts in a user-specified
>> directory ($BASH_COMPLETION_USER_DIR) or in the user's home directory
>> ($XDG_DATA_HOME or ~/.local/...) as well.  It loads the matching completion
>> script, but does not call the completion function unconditionally.
>
>
> Sounds good so far.
>
>> The "almost" refers to he case when _completion_loader() can't find a
>> completion script with a matching name to load, and then registers the
>> _minimal() completion function for the given command to do basic path
>> completion as fallback.  I don't think this matters in practice, because in
>> this case the given command is a git command in its dashed form, e.g.
>> 'git-diff-index', and those have been deprecated for a long time.
>
>
> I sense a problem here. If I have a directory with a file xyzfoobar in it,
> and I type `git xyz`, with no defined subcommand that starts with these
> letters, then minimal bashcomp would give me `git xyzfoobar`, which can of
> course not execute. This can be unintuitive for users, as in: "If it can't
> be executed correctly, then why does it even suggest such a completion?"

I'm not sure I understand the problem.  After 'git xyz' (note
there is no space between 'xyz' and ) we try to complete the name
of a git command, not options of a git command.  This means:

  - At this point we don't look for a _git_xyz() function, so we'll
return from __git_main() before even reaching the piece of code
your patch modifies.

  - There are (presumably) no commands starting with 'xyz', so we
don't list any commands.  Bash will then fall back to its own
filename completion, and that's where that 'xyzfoobar' will come
from.  It has been behaving like this basically since forever.

And after 'git xyz ' (this time with space) we already complete
the next word, not 'xyz'.

>> I think all you need to do is run a
>> s/__load_completion/_completion_loader/ on your patch and update the commit
>> message with relevant bits from the above discussion.
>
>
> I can do that, no problem. But prior to that I want to be sure that you are
> okay with the above mentioned drawback. Will the behavior be acceptable in
> this case? Or should we try to somehow "undo" the minimal completion
> afterwards?

As explained above, I don't think there is any drawback here.  Or at
least not any new drawback that your patch is introducing.  Or I'm
completely missing your point; certainly a possibility, it's early
Sunday afternoon, after all :)

Re: [PATCH v5 0/5] Convert some stash functionality to a builtin

[Johannes Schindelin]

Hi,

On Sun, 29 Apr 2018, Paul-Sebastian Ungureanu wrote:

> > > Since there seems to be interest from GSOC students who want to
> > > work on converting builtins, I figured I should finish what I
> > > have that works now so they could build on top of it.
> 
> First of all, I must thank you for submitting this series of patches. It is a
> great starting point to convert 'git stash'.
> 
> I would like to continue your work on "git stash" if that is fine with you.

So you mean you would like to take custody of Joel's patches and address
the reviews so far, then re-submit?

Ciao,
Dscho

P.S.: Yes, I think I know that you want to do that (as we discussed on
IRC), but I just wanted to clarify on the mailing list, to avoid
misunderstandings ;-).

Re: [PATCH 3/6] rebase -i --root: let the sequencer handle even the initial part

[Johannes Schindelin]

Hi Stefan,

On Sat, 28 Apr 2018, Stefan Beller wrote:

> On Fri, Apr 27, 2018 at 3:31 PM, Johannes Schindelin
>  wrote:
> > In this developer's earlier attempt to accelerate interactive rebases by
> > converting large parts from Unix shell script into portable, performant
> > C, the --root handling was specifically excluded (to simplify the task a
> > little bit; it still took over a year to get that reduced set of patches
> > into Git proper).
> >
> > This patch ties up that loose end: now only --preserve-merges uses the
> > slow Unix shell script implementation to perform the interactive rebase.
> >
> > As the rebase--helper reports progress to stderr (unlike the scripted
> > interactive rebase, which reports it to stdout, of all places), we have
> > to adjust a couple of tests that did not expect that for `git rebase -i
> > --root`.
> >
> > This patch fixes -- at long last! -- the really old bug reported in
> > 6a6bc5bdc4d (add tests for rebasing root, 2013-06-06) that rebasing with
> > --root *always* rewrote the root commit, even if there were no changes.
> >
> > The bug still persists in --preserve-merges mode, of course, but that
> > mode will be deprecated as soon as the new --rebase-merges mode
> > stabilizes, anyway.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  git-rebase--interactive.sh|  4 +++-
> >  t/t3404-rebase-interactive.sh | 19 +--
> >  t/t3421-rebase-topology-linear.sh |  6 +++---
> >  3 files changed, 19 insertions(+), 10 deletions(-)
> >
> > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> > index cbf44f86482..2f4941d0fc9 100644
> > --- a/git-rebase--interactive.sh
> > +++ b/git-rebase--interactive.sh
> > @@ -894,6 +894,8 @@ init_revisions_and_shortrevisions () {
> > else
> > revisions=$onto...$orig_head
> > shortrevisions=$shorthead
> > +   test -z "$squash_onto" ||
> > +   echo "$squash_onto" >"$state_dir"/squash-onto
> > fi
> >  }
> >
> > @@ -948,7 +950,7 @@ EOF
> > die "Could not skip unnecessary pick commands"
> >
> > checkout_onto
> > -   if test -z "$rebase_root" && test ! -d "$rewritten"
> > +   if test ! -d "$rewritten"
> 
> I have the impression this is the line that is really well
> explained in the commit message ("migrate to rebase
> helper even when there is $rebase_root set")
> 
> The rest of the patch is covered as "a couple of places
> where we adjust stdout to stderr"?

Correct.

Thanks for reviewing!
Dscho

Re: [PATCH 2/6] sequencer: learn about the special "fake root commit" handling

[Johannes Schindelin]

Hi Stefan,

On Sat, 28 Apr 2018, Stefan Beller wrote:

> On Fri, Apr 27, 2018 at 3:31 PM, Johannes Schindelin
>  wrote:
> > When an interactive rebase wants to recreate a root commit, it
> > - first creates a new, empty root commit,
> > - checks it out,
> > - converts the next `pick` command so that it amends the empty root
> >   commit
> >
> > Introduce support in the sequencer to handle such an empty root commit,
> > by looking for the file /rebase-merge/squash-onto; if it exists
> > and contains a commit name, the sequencer will compare the HEAD to said
> > root commit, and if identical, a new root commit will be created.
> >
> > While converting scripted code into proper, portable C, we also do away
> > with the old "amend with an empty commit message, then cherry-pick
> > without committing, then amend again" dance and replace it with code
> > that uses the internal API properly to do exactly what we want: create a
> > new root commit.
> >
> > To keep the implementation simple, we always spawn `git commit` to create
> > new root commits.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  sequencer.c | 104 ++--
> >  sequencer.h |   4 ++
> >  2 files changed, 105 insertions(+), 3 deletions(-)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index 90c8218aa9a..fc124596b53 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -125,6 +125,12 @@ static GIT_PATH_FUNC(rebase_path_rewritten_list, 
> > "rebase-merge/rewritten-list")
> >  static GIT_PATH_FUNC(rebase_path_rewritten_pending,
> > "rebase-merge/rewritten-pending")
> >
> > +/*
> > + * The path of the file containig the OID of the "squash onto" commit, i.e.
> > + * the dummy commit used for `reset [new root]`.
> > + */
> > +static GIT_PATH_FUNC(rebase_path_squash_onto, "rebase-merge/squash-onto")
> > +
> >  /*
> >   * The path of the file listing refs that need to be deleted after the 
> > rebase
> >   * finishes. This is used by the `label` command to record the need for 
> > cleanup.
> > @@ -470,7 +476,8 @@ static int fast_forward_to(const struct object_id *to, 
> > const struct object_id *f
> > transaction = ref_transaction_begin();
> > if (!transaction ||
> > ref_transaction_update(transaction, "HEAD",
> > -  to, unborn ? _oid : from,
> > +  to, unborn && !is_rebase_i(opts) ?
> > +  _oid : from,
> >0, sb.buf, ) ||
> > ref_transaction_commit(transaction, )) {
> > ref_transaction_free(transaction);
> > @@ -692,6 +699,42 @@ static char *get_author(const char *message)
> > return NULL;
> >  }
> >
> > +static const char *read_author_ident(struct strbuf *buf)
> 
> This seems to be the counter part of write_author_script(*msg),
> would it make sense to either rename this to read_author_script
> or rename the counter part to write_author_ident ?

It is not really *the* counterpart of write_author_script(). There are
many conceivable counterparts, one of them already exists and is called
read_env_script(). They serve different purposes, even if both read the
author-script file, and they parse things in a fundamentally different
way. I had already pointed out something along those lines in the review
of the patch introducing the read_env_script() and Junio did not believe
me. To make sure, it was my fault that I failed to make a compelling
enough argument. I am glad that I now have proof that I was right: just
because you read the same file, for a similar purpose, does not
necessarily imply that you can share code for those purposes. All you can
share is the name of the input file.

Having said that, *this* time round, what we need to do is actually very
similar to what builtin/am.c's read_author_script() does (even if we
cannot use it as-is: it populates part of a `struct am_state`). I'll have
to look into this more closely.

> > +{
> > +   char *p, *p2;
> > +
> > +   if (strbuf_read_file(buf, rebase_path_author_script(), 256) <= 0)
> 
> The 256 is a hint for read_file how to size the buffer initially.
> If not given it defaults to 8k, which presumably is too much for
> an author identity.

That is a correct reading of the code's intent.

> > +   for (p = buf->buf; *p; p++)
> > +   if (skip_prefix(p, "'''", (const char **)))
> > +   strbuf_splice(buf, p - buf->buf, p2 - p, "'", 1);
> > +   else if (*p == '\'')
> > +   strbuf_splice(buf, p-- - buf->buf, 1, "", 0);
> 
> This part could be prefixed with
> /* un-escape text: turn \\ into ' and remove single quotes. */

If could be prefixed that way, but it would be incorrect. We never turn \\
into '. What we do here (and I do not want to repeat in a comment what the
code does): we dequote what we previously 

Re: [PATCH v2 1/1] completion: load completion file for external subcommand

[Florian Gamböck]

On 2018-04-25 16:40, SZEDER Gábor wrote:
In my previous emails I overlooked the _completion_loader() helper 
function.


It seems that this function does almost exactly what we want.  It was 
introduced along with dynamic completion loading back in 20c05b43, so 
it's available for us even in older LTS/Enterprise releases.  Since 
cad3abfc it's a wrapper around __load_completion() and thus benefits 
from all the improvements, notably searching for completion scripts in 
a user-specified directory ($BASH_COMPLETION_USER_DIR) or in the 
user's home directory ($XDG_DATA_HOME or ~/.local/...) as well.  It 
loads the matching completion script, but does not call the completion 
function unconditionally.


Sounds good so far.

The "almost" refers to he case when _completion_loader() can't find a 
completion script with a matching name to load, and then registers the 
_minimal() completion function for the given command to do basic path 
completion as fallback.  I don't think this matters in practice, 
because in this case the given command is a git command in its dashed 
form, e.g. 'git-diff-index', and those have been deprecated for a long 
time.


I sense a problem here. If I have a directory with a file xyzfoobar in 
it, and I type `git xyz`, with no defined subcommand that starts with 
these letters, then minimal bashcomp would give me `git xyzfoobar`, 
which can of course not execute. This can be unintuitive for users, as 
in: "If it can't be executed correctly, then why does it even suggest 
such a completion?"


I think all you need to do is run a 
s/__load_completion/_completion_loader/ on your patch and update the 
commit message with relevant bits from the above discussion.


I can do that, no problem. But prior to that I want to be sure that you 
are okay with the above mentioned drawback. Will the behavior be 
acceptable in this case? Or should we try to somehow "undo" the minimal 
completion afterwards?

Re: [PATCH v4 03/10] commit-graph: compute generation numbers

[Jakub Narebski]

Derrick Stolee  writes:

> While preparing commits to be written into a commit-graph file, compute
> the generation numbers using a depth-first strategy.

Sidenote: for generation numbers it does not matter if we use
depth-first or breadth-first strategy, but it is more natural to use
depth-first search because generation numbers need post-order processing
(parents before child).

>
> The only commits that are walked in this depth-first search are those
> without a precomputed generation number. Thus, computation time will be
> relative to the number of new commits to the commit-graph file.

A question: what happens if the existing commit graph is from older
version of git and has _ZERO for generation numbers?

Answer: I see that we treat both _INFINITY (not in commit-graph) and
_ZERO (in commit graph but not computed) as not computed generation
numbers.  All right.

>
> If a computed generation number would exceed GENERATION_NUMBER_MAX, then
> use GENERATION_NUMBER_MAX instead.

All right, though I guess this would remain theoretical for a long
while.

We don't have any way of testing this, at least not without recompiling
Git with lower value of GENERATION_NUMBER_MAX -- which means not
automatically, isn't it?

>
> Signed-off-by: Derrick Stolee 
> ---
>  commit-graph.c | 45 +
>  1 file changed, 45 insertions(+)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 9ad21c3ffb..047fa9fca5 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -439,6 +439,9 @@ static void write_graph_chunk_data(struct hashfile *f, 
> int hash_len,
>   else
>   packedDate[0] = 0;
>  
> + if ((*list)->generation != GENERATION_NUMBER_INFINITY)
> + packedDate[0] |= htonl((*list)->generation << 2);
> +

If we stumble upon commit marked as "not in commit-graph" while writing
commit graph, it is a BUG(), isn't it?

(Problem noticed by Junio.)

It is a bit strange to me that the code uses get_be32 for reading, but
htonl for writing.  Is Git tested on non little-endian machines, like
big-endian ppc64 or s390x, or on mixed-endian machines (or
selectable-endian machines with data endianness set to non
little-endian, like ia64)?  If not, could we use for example openSUSE
Build Service (https://build.opensuse.org/) for this?

>   packedDate[1] = htonl((*list)->date);
>   hashwrite(f, packedDate, 8);
>  
> @@ -571,6 +574,46 @@ static void close_reachable(struct packed_oid_list *oids)
>   }
>  }
>  
> +static void compute_generation_numbers(struct commit** commits,
> +int nr_commits)
> +{
> + int i;
> + struct commit_list *list = NULL;

All right, commit_list will work as stack.

> +
> + for (i = 0; i < nr_commits; i++) {
> + if (commits[i]->generation != GENERATION_NUMBER_INFINITY &&
> + commits[i]->generation != GENERATION_NUMBER_ZERO)
> + continue;

All right, we consider _INFINITY and _SERO as not computed.  If
generation number is computed (by 'recursion' or from commit graph), we
(re)use it.  This means that generation number calculation is
incremental, as intended -- good.

> +
> + commit_list_insert(commits[i], );

Start depth-first walks from commits given.

> + while (list) {
> + struct commit *current = list->item;
> + struct commit_list *parent;
> + int all_parents_computed = 1;

Here all_parents_computed is a boolean flag.  I see that it is easier to
start with assumption that all parents will have computed generation
numbers.

> + uint32_t max_generation = 0;

The generation number value of 0 functions as sentinel; generation
numbers start from 1.  Not that it matters much, as lowest possible
generation number is 1, and we could have started from that value.

> +
> + for (parent = current->parents; parent; parent = 
> parent->next) {
> + if (parent->item->generation == 
> GENERATION_NUMBER_INFINITY ||
> + parent->item->generation == 
> GENERATION_NUMBER_ZERO) {
> + all_parents_computed = 0;
> + commit_list_insert(parent->item, );
> + break;

If some parent doesn't have generation number calculated, we add it to
stack (and break out of loop because it is depth-first walk), and mark
this situation.  All right.

> + } else if (parent->item->generation > 
> max_generation) {
> + max_generation = 
> parent->item->generation;

Otherwise, update max_generation.  All right.

> + }
> + }
> +
> + if (all_parents_computed) {
> +