[PATCH 7/9] completion: drop the hard coded list of config vars

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

The new help option --config-for-completion is a machine friendlier
version of --config where all the placeholders and wildcards are
dropped, leaving only the good, completable prefixes for
git-completion.bash to consume.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/help.c |  13 +-
 contrib/completion/git-completion.bash | 334 +
 help.c |  30 ++-
 help.h |   2 +-
 4 files changed, 47 insertions(+), 332 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index a1153cf473..fbd2b0238a 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -45,7 +45,8 @@ static struct option builtin_help_options[] = {
OPT_BOOL('a', "all", _all, N_("print all available commands")),
OPT_HIDDEN_BOOL(0, "exclude-guides", _guides, N_("exclude 
guides")),
OPT_BOOL('g', "guides", _guides, N_("print list of useful 
guides")),
-   OPT_BOOL('c', "config", _config, N_("print list recognized config 
variables")),
+   OPT_SET_INT('c', "config", _config, N_("print list recognized 
config variables"), 1),
+   OPT_SET_INT_F(0, "config-for-completion", _config, "", 2, 
PARSE_OPT_HIDDEN),
OPT_SET_INT('m', "man", _format, N_("show man page"), 
HELP_FORMAT_MAN),
OPT_SET_INT('w', "web", _format, N_("show manual in web browser"),
HELP_FORMAT_WEB),
@@ -446,9 +447,13 @@ int cmd_help(int argc, const char **argv, const char 
*prefix)
}
 
if (show_config) {
-   setup_pager();
-   list_config_help();
-   printf("\n%s\n", _("'git help config' for more information"));
+   int for_human = show_config == 1;
+
+   if (for_human)
+   setup_pager();
+   list_config_help(for_human);
+   if (for_human)
+   printf("\n%s\n", _("'git help config' for more 
information"));
return 0;
}
 
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index f7ca9a4d4f..8d3257c4de 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2074,6 +2074,13 @@ __git_config_get_set_variables ()
__git config $config_file --name-only --list
 }
 
+__git_config_vars=
+__git_compute_config_vars ()
+{
+   test -n "$__git_config_vars" ||
+   __git_config_vars="$(git help --config-for-completion | sort | uniq)"
+}
+
 _git_config ()
 {
case "$prev" in
@@ -2232,331 +2239,8 @@ _git_config ()
return
;;
esac
-   __gitcomp "
-   add.ignoreErrors
-   advice.amWorkDir
-   advice.commitBeforeMerge
-   advice.detachedHead
-   advice.implicitIdentity
-   advice.pushAlreadyExists
-   advice.pushFetchFirst
-   advice.pushNeedsForce
-   advice.pushNonFFCurrent
-   advice.pushNonFFMatching
-   advice.pushUpdateRejected
-   advice.resolveConflict
-   advice.rmHints
-   advice.statusHints
-   advice.statusUoption
-   advice.ignoredHook
-   alias.
-   am.keepcr
-   am.threeWay
-   apply.ignorewhitespace
-   apply.whitespace
-   branch.autosetupmerge
-   branch.autosetuprebase
-   browser.
-   clean.requireForce
-   color.branch
-   color.branch.current
-   color.branch.local
-   color.branch.plain
-   color.branch.remote
-   color.decorate.HEAD
-   color.decorate.branch
-   color.decorate.remoteBranch
-   color.decorate.stash
-   color.decorate.tag
-   color.diff
-   color.diff.commit
-   color.diff.frag
-   color.diff.func
-   color.diff.meta
-   color.diff.new
-   color.diff.old
-   color.diff.plain
-   color.diff.whitespace
-   color.grep
-   color.grep.context
-   color.grep.filename
-   color.grep.function
-   color.grep.linenumber
-   color.grep.match
-   color.grep.selected
-   color.grep.separator
-   color.interactive
-   color.interactive.error
-   color.interactive.header
-   color.interactive.help
-   color.interactive.prompt
-   color.pager
-   color.showbranch
-   color.status
-   color.status.added
-   color.status.changed
-   color.status.header
-   color.status.localBranch
-   color.status.nobranch
-   

[PATCH 6/9] am: move advice.amWorkDir parsing back to advice.c

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

The only benefit from this move (apart from cleaner code) is that
advice.amWorkDir should now show up in `git help --config`. There
should be no regression since advice config is always read by the
git_default_config().

While at there, use advise() like other code. We now get "hint: "
prefix and the output is stderr instead of stdout (which is also the
reason for the test update because stderr is checked in a following
test and the extra advice can fail it).

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 advice.c  | 2 ++
 advice.h  | 1 +
 builtin/am.c  | 5 +
 t/t4254-am-corrupt.sh | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/advice.c b/advice.c
index d8ea93637a..d300491a6f 100644
--- a/advice.c
+++ b/advice.c
@@ -16,6 +16,7 @@ int advice_implicit_identity = 1;
 int advice_detached_head = 1;
 int advice_set_upstream_failure = 1;
 int advice_object_name_warning = 1;
+int advice_amworkdir = 1;
 int advice_rm_hints = 1;
 int advice_add_embedded_repo = 1;
 int advice_ignored_hook = 1;
@@ -39,6 +40,7 @@ static struct {
{ "detachedHead", _detached_head },
{ "setupStreamFailure", _set_upstream_failure },
{ "objectNameWarning", _object_name_warning },
+   { "amWorkDir", _amworkdir },
{ "rmHints", _rm_hints },
{ "addEmbeddedRepo", _add_embedded_repo },
{ "ignoredHook", _ignored_hook },
diff --git a/advice.h b/advice.h
index 70568fa792..7555385aa5 100644
--- a/advice.h
+++ b/advice.h
@@ -17,6 +17,7 @@ extern int advice_implicit_identity;
 extern int advice_detached_head;
 extern int advice_set_upstream_failure;
 extern int advice_object_name_warning;
+extern int advice_amworkdir;
 extern int advice_rm_hints;
 extern int advice_add_embedded_repo;
 extern int advice_ignored_hook;
diff --git a/builtin/am.c b/builtin/am.c
index 9c82603f70..03e5870c62 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1827,15 +1827,12 @@ static void am_run(struct am_state *state, int resume)
}
 
if (apply_status) {
-   int advice_amworkdir = 1;
 
printf_ln(_("Patch failed at %s %.*s"), msgnum(state),
linelen(state->msg), state->msg);
 
-   git_config_get_bool("advice.amworkdir", 
_amworkdir);
-
if (advice_amworkdir)
-   printf_ln(_("Use 'git am --show-current-patch' 
to see the failed patch"));
+   advise(_("Use 'git am --show-current-patch' to 
see the failed patch"));
 
die_user_resolve(state);
}
diff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh
index 168739c721..fd3bdbfe2c 100755
--- a/t/t4254-am-corrupt.sh
+++ b/t/t4254-am-corrupt.sh
@@ -25,7 +25,7 @@ test_expect_success setup '
 #   fatal: unable to write file '(null)' mode 100644: Bad address
 # Also, it had the unwanted side-effect of deleting f.
 test_expect_success 'try to apply corrupted patch' '
-   test_must_fail git am bad-patch.diff 2>actual
+   test_must_fail git -c advice.amWorkDir=false am bad-patch.diff 2>actual
 '
 
 test_expect_success 'compare diagnostic; ensure file is still here' '
-- 
2.17.0.705.g3525833791

[PATCH 3/9] fsck: factor out msg_id_info[] lazy initialization code

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

This array will be used by some other function than parse_msg_id() in
the following commit. Factor out this prep code so it could be called
from that one.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 fsck.c | 38 ++
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/fsck.c b/fsck.c
index 9218c2a643..f2534abd44 100644
--- a/fsck.c
+++ b/fsck.c
@@ -84,26 +84,32 @@ static struct {
 };
 #undef MSG_ID
 
-static int parse_msg_id(const char *text)
+static void prepare_msg_ids(void)
 {
int i;
 
-   if (!msg_id_info[0].downcased) {
-   /* convert id_string to lower case, without underscores. */
-   for (i = 0; i < FSCK_MSG_MAX; i++) {
-   const char *p = msg_id_info[i].id_string;
-   int len = strlen(p);
-   char *q = xmalloc(len);
-
-   msg_id_info[i].downcased = q;
-   while (*p)
-   if (*p == '_')
-   p++;
-   else
-   *(q)++ = tolower(*(p)++);
-   *q = '\0';
-   }
+   /* convert id_string to lower case, without underscores. */
+   for (i = 0; i < FSCK_MSG_MAX; i++) {
+   const char *p = msg_id_info[i].id_string;
+   int len = strlen(p);
+   char *q = xmalloc(len);
+
+   msg_id_info[i].downcased = q;
+   while (*p)
+   if (*p == '_')
+   p++;
+   else
+   *(q)++ = tolower(*(p)++);
+   *q = '\0';
}
+}
+
+static int parse_msg_id(const char *text)
+{
+   int i;
+
+   if (!msg_id_info[0].downcased)
+   prepare_msg_ids();
 
for (i = 0; i < FSCK_MSG_MAX; i++)
if (!strcmp(text, msg_id_info[i].downcased))
-- 
2.17.0.705.g3525833791

[PATCH 1/9] Add and use generic name->id mapping code for color slot parsing

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

Instead of hard coding the name-to-id mapping in C code, keep it in an
array and use a common function to do the parsing. This reduces code
and also allows us to list all possible color slots later.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/branch.c | 28 +
 builtin/clean.c  | 28 +
 builtin/commit.c | 33 ++
 config.c | 13 
 config.h |  4 
 diff.c   | 53 +++-
 log-tree.c   | 35 
 7 files changed, 82 insertions(+), 112 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 5bd2a0dd48..b41f332589 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -55,26 +55,18 @@ enum color_branch {
BRANCH_COLOR_UPSTREAM = 5
 };
 
+static const char *color_branch_slots[] = {
+   [BRANCH_COLOR_RESET]= "reset",
+   [BRANCH_COLOR_PLAIN]= "plain",
+   [BRANCH_COLOR_REMOTE]   = "remote",
+   [BRANCH_COLOR_LOCAL]= "local",
+   [BRANCH_COLOR_CURRENT]  = "current",
+   [BRANCH_COLOR_UPSTREAM] = "upstream",
+};
+
 static struct string_list output = STRING_LIST_INIT_DUP;
 static unsigned int colopts;
 
-static int parse_branch_color_slot(const char *slot)
-{
-   if (!strcasecmp(slot, "plain"))
-   return BRANCH_COLOR_PLAIN;
-   if (!strcasecmp(slot, "reset"))
-   return BRANCH_COLOR_RESET;
-   if (!strcasecmp(slot, "remote"))
-   return BRANCH_COLOR_REMOTE;
-   if (!strcasecmp(slot, "local"))
-   return BRANCH_COLOR_LOCAL;
-   if (!strcasecmp(slot, "current"))
-   return BRANCH_COLOR_CURRENT;
-   if (!strcasecmp(slot, "upstream"))
-   return BRANCH_COLOR_UPSTREAM;
-   return -1;
-}
-
 static int git_branch_config(const char *var, const char *value, void *cb)
 {
const char *slot_name;
@@ -86,7 +78,7 @@ static int git_branch_config(const char *var, const char 
*value, void *cb)
return 0;
}
if (skip_prefix(var, "color.branch.", _name)) {
-   int slot = parse_branch_color_slot(slot_name);
+   int slot = LOOKUP_CONFIG(color_branch_slots, slot_name);
if (slot < 0)
return 0;
if (!value)
diff --git a/builtin/clean.c b/builtin/clean.c
index fad533a0a7..0ccd95e693 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -42,6 +42,15 @@ enum color_clean {
CLEAN_COLOR_ERROR = 5
 };
 
+static const char *color_interactive_slots[] = {
+   [CLEAN_COLOR_ERROR]  = "error",
+   [CLEAN_COLOR_HEADER] = "header",
+   [CLEAN_COLOR_HELP]   = "help",
+   [CLEAN_COLOR_PLAIN]  = "plain",
+   [CLEAN_COLOR_PROMPT] = "prompt",
+   [CLEAN_COLOR_RESET]  = "reset",
+};
+
 static int clean_use_color = -1;
 static char clean_colors[][COLOR_MAXLEN] = {
[CLEAN_COLOR_ERROR] = GIT_COLOR_BOLD_RED,
@@ -82,23 +91,6 @@ struct menu_stuff {
void *stuff;
 };
 
-static int parse_clean_color_slot(const char *var)
-{
-   if (!strcasecmp(var, "reset"))
-   return CLEAN_COLOR_RESET;
-   if (!strcasecmp(var, "plain"))
-   return CLEAN_COLOR_PLAIN;
-   if (!strcasecmp(var, "prompt"))
-   return CLEAN_COLOR_PROMPT;
-   if (!strcasecmp(var, "header"))
-   return CLEAN_COLOR_HEADER;
-   if (!strcasecmp(var, "help"))
-   return CLEAN_COLOR_HELP;
-   if (!strcasecmp(var, "error"))
-   return CLEAN_COLOR_ERROR;
-   return -1;
-}
-
 static int git_clean_config(const char *var, const char *value, void *cb)
 {
const char *slot_name;
@@ -113,7 +105,7 @@ static int git_clean_config(const char *var, const char 
*value, void *cb)
return 0;
}
if (skip_prefix(var, "color.interactive.", _name)) {
-   int slot = parse_clean_color_slot(slot_name);
+   int slot = LOOKUP_CONFIG(color_interactive_slots, slot_name);
if (slot < 0)
return 0;
if (!value)
diff --git a/builtin/commit.c b/builtin/commit.c
index 37fcb55ab0..bee5825bd2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -66,6 +66,18 @@ N_("If you wish to skip this commit, use:\n"
 "Then \"git cherry-pick --continue\" will resume cherry-picking\n"
 "the remaining commits.\n");
 
+static const char *color_status_slots[] = {
+   [WT_STATUS_HEADER]= "header",
+   [WT_STATUS_UPDATED]   = "updated",
+   [WT_STATUS_CHANGED]   = "changed",
+   [WT_STATUS_UNTRACKED] = "untracked",
+   [WT_STATUS_NOBRANCH]  = "noBranch",
+   [WT_STATUS_UNMERGED]  = "unmerged",
+   [WT_STATUS_LOCAL_BRANCH]  = "localBranch",
+   [WT_STATUS_REMOTE_BRANCH] = "remoteBranch",
+   [WT_STATUS_ONBRANCH]  = "branch",
+};
+
 static const char 

[PATCH v2] add status config and command line options for rename detection

[Ben Peart]

After performing a merge that has conflicts, git status will by default attempt
to detect renames which causes many objects to be examined.  In a virtualized
repo, those objects do not exist locally so the rename logic triggers them to be
fetched from the server. This results in the status call taking hours to
complete on very large repos.  Even in a small repo (the GVFS repo) turning off
break and rename detection has a significant impact:

git status --no-renames:
31 secs., 105 loose object downloads

git status --no-breaks
7 secs., 17 loose object downloads

git status --no-breaks --no-renames
1 sec., 1 loose object download

Add a new config status.renames setting to enable turning off rename detection
during status.  This setting will default to the value of diff.renames.

Add a new config status.renamelimit setting to to enable bounding the time spent
finding out inexact renames during status.  This setting will default to the
value of diff.renamelimit.

Add status --no-renames command line option that enables overriding the config
setting from the command line. Add --find-renames[=] to enable detecting
renames and optionally setting the similarity index from the command line.

Note: I removed the --no-breaks command line option from the original patch as
it will no longer be needed once the default has been changed [1] to turn it 
off.

[1] 
https://public-inbox.org/git/20180430093421.27551-2-eckhard.s.ma...@gmail.com/

Original-Patch-by: Alejandro Pauly 
Signed-off-by: Ben Peart 
---

Notes:
Base Ref: master
Web-Diff: https://github.com/benpeart/git/commit/823212725b
Checkout: git fetch https://github.com/benpeart/git status-renames-v2 && 
git checkout 823212725b

### Interdiff (v1..v2):

diff --git a/Documentation/config.txt b/Documentation/config.txt
index b79b83c587..9c8eca05b1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3126,7 +3126,8 @@ status.renameLimit::
 status.renames::
Whether and how Git detects renames.  If set to "false",
rename detection is disabled. If set to "true", basic rename
-   detection is enabled.  Defaults to the value of diff.renames.
+   detection is enabled.  If set to "copies" or "copy", Git will
+   detect copies, as well.  Defaults to the value of diff.renames.

 status.showStash::
If set to true, linkgit:git-status[1] will display the number of
diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index c16e27e63d..c4467ffb98 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -135,6 +135,16 @@ ignored, then the directory is not shown, but all 
contents are shown.
Display or do not display detailed ahead/behind counts for the
branch relative to its upstream branch.  Defaults to true.

+--renames::
+--no-renames::
+   Turn on/off rename detection regardless of user configuration.
+   See also linkgit:git-diff[1] `--no-renames`.
+
+--find-renames[=]::
+   Turn on rename detection, optionally setting the similarity
+   threshold.
+   See also linkgit:git-diff[1] `--find-renames`.
+
 ...::
See the 'pathspec' entry in linkgit:gitglossary[7].

diff --git a/builtin/commit.c b/builtin/commit.c
index a545096ddd..db886277f4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -109,10 +109,6 @@ static int have_option_m;
 static struct strbuf message = STRBUF_INIT;

 static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
-static int diff_detect_rename = -1;
-static int status_detect_rename = -1;
-static int diff_rename_limit = -1;
-static int status_rename_limit = -1;

 static int opt_parse_porcelain(const struct option *opt, const char *arg, 
int unset)
 {
@@ -1274,19 +1270,21 @@ static int git_status_config(const char *k, const 
char *v, void *cb)
return 0;
}
if (!strcmp(k, "diff.renamelimit")) {
-   diff_rename_limit = git_config_int(k, v);
+   if (s->rename_limit == -1)
+   s->rename_limit = git_config_int(k, v);
return 0;
}
if (!strcmp(k, "status.renamelimit")) {
-   status_rename_limit = git_config_int(k, v);
+   s->rename_limit = git_config_int(k, v);
return 0;
}
if (!strcmp(k, "diff.renames")) {
-   diff_detect_rename = git_config_rename(k, v);
+   if (s->detect_rename == -1)
+   s->detect_rename = git_config_rename(k, v);
return 0;
}
if (!strcmp(k, "status.renames")) {
-   status_detect_rename = git_config_rename(k, v);
+   s->detect_rename = git_config_rename(k, v);
return 0;
}
return 

Re: Regression in patch add?

[Oliver Joseph Ash]

You found the problem Phillip! My editor was trimming trailing white space, 
which breaks the context line.

I had tried to use an alternative editor to account for any editor specific 
behaviour, but it turns out both the editors I tested in were doing this!

I suspect this change in behaviour will effect a lot of users? If so, it would 
be good if `git add -p` allowed for this behaviour, in the same way `git apply` 
does.

Meanwhile, I can easily configure my editor not to do this for `*.diff` files.

Thanks for your help, Phillip and Martin!

Mahmoud, does this also explain your problem as per your original post?

[PATCH 2/2] t5516-fetch-push: fix broken &&-chain

[SZEDER Gábor]

b2dc968e60 (t5516: refactor oddball tests, 2008-11-07) accidentaly
broke the &&-chain in the test 'push does not update local refs on
failure', but since it was in a subshell, chain-lint couldn't notice
it.

Signed-off-by: SZEDER Gábor 
---
 t/t5516-fetch-push.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 832b79ad40..3e8940eee5 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -612,7 +612,7 @@ test_expect_success 'push does not update local refs on 
failure' '
chmod +x testrepo/.git/hooks/pre-receive &&
(
cd child &&
-   git pull .. master
+   git pull .. master &&
test_must_fail git push &&
test $(git rev-parse master) != \
$(git rev-parse remotes/origin/master)
-- 
2.17.0.756.gcf614c5aff

[PATCH 1/2] t5516-fetch-push: fix 'push with dry-run' test

[SZEDER Gábor]

In a while-at-it cleanup replacing a 'cd dir && <...> && cd ..' with a
subshell, commit 28391a80a9 (receive-pack: allow deletion of corrupt
refs, 2007-11-29) also moved the assignment of the $old_commit
variable to that subshell.  This variable, however, is used outside of
that subshell as a parameter of check_push_result(), to check that a
ref still points to the commit where it is supposed to.  With the
variable remaining unset outside the subshell check_push_result()
doesn't perform that check at all.

Use 'git -C  cmd...', so we don't need to change directory, and
thus don't need the subshell either when setting $old_commit.

Furthermore, change check_push_result() to require at least three
parameters (the repo, the oid, and at least one ref), so it will catch
similar issues earlier should they ever arise.

Signed-off-by: SZEDER Gábor 
---
 t/t5516-fetch-push.sh | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 82239138d5..832b79ad40 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -94,6 +94,9 @@ mk_child() {
 }
 
 check_push_result () {
+   test $# -ge 3 ||
+   error "bug in the test script: check_push_result requires at least 3 
parameters"
+
repo_name="$1"
shift
 
@@ -553,10 +556,7 @@ test_expect_success 'branch.*.pushremote config order is 
irrelevant' '
 test_expect_success 'push with dry-run' '
 
mk_test testrepo heads/master &&
-   (
-   cd testrepo &&
-   old_commit=$(git show-ref -s --verify refs/heads/master)
-   ) &&
+   old_commit=$(git -C testrepo show-ref -s --verify refs/heads/master) &&
git push --dry-run testrepo : &&
check_push_result testrepo $old_commit heads/master
 '
-- 
2.17.0.756.gcf614c5aff

[PATCH] t5310-pack-bitmaps: make JGit tests work with GIT_TEST_SPLIT_INDEX

[SZEDER Gábor]

The two JGit tests 'we can read jgit bitmaps' and 'jgit can read our
bitmaps' in 't5310-pack-bitmaps.sh' fail when run with
GIT_TEST_SPLIT_INDEX=YesPlease.  Both tests create a clone of the test
repository to check bitmap interoperability with JGit.  With split
indexes enabled the index in the clone repositories contains the
'link' extension, which JGit doesn't support and, consequently, an
exception aborts it:

  <...>
  org.eclipse.jgit.api.errors.JGitInternalException: DIRC extension 'link' not 
supported by this version.
  at org.eclipse.jgit.dircache.DirCache.readFrom(DirCache.java:562)
  <...>

Since testing bitmaps doesn't need a worktree in the first place,
let's just create bare clones for the two JGit tests, so the cloned
won't have an index, and these two tests can be executed even with
split index enabled.

Signed-off-by: SZEDER Gábor 
---
 t/t5310-pack-bitmaps.sh | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index f6d600fd82..423c0a475f 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -264,9 +264,9 @@ test_expect_success 'pack with missing parent' '
 '
 
 test_expect_success JGIT 'we can read jgit bitmaps' '
-   git clone . compat-jgit &&
+   git clone --bare . compat-jgit.git &&
(
-   cd compat-jgit &&
+   cd compat-jgit.git &&
rm -f .git/objects/pack/*.bitmap &&
jgit gc &&
git rev-list --test-bitmap HEAD
@@ -274,9 +274,9 @@ test_expect_success JGIT 'we can read jgit bitmaps' '
 '
 
 test_expect_success JGIT 'jgit can read our bitmaps' '
-   git clone . compat-us &&
+   git clone --bare . compat-us.git &&
(
-   cd compat-us &&
+   cd compat-us.git &&
git repack -adb &&
# jgit gc will barf if it does not like our bitmaps
jgit gc
-- 
2.17.0.756.gcf614c5aff

Re: Regression in patch add?

[Martin Ågren]

On 10 May 2018 at 15:16, Oliver Joseph Ash  wrote:
> (Apologies, I accidentally sent this as a reply to the original post, instead 
> of your email. I'm new to this!)

(No worries.) ;-)

>> does your test involve unusual file systems, funny characters in filenames, 
>> ..? You are on some sort of Linux, right?
>
> I'm running macOS 10.13.4. I don't have any unusual file system setup, as far 
> as I'm aware. The filename in my test case is simply `foo`.

I'm not too familiar with Mac, unfortunately, but let's see..

> I tried the steps you suggested: on git 2.17.0, saving the patch, editing it, 
> and applying it, and it succeeded.
>
>> should now show bar2 in the first hunk and bar1 in the second hunk, just 
>> like your edited test.patch.
>
> That was the case, although I had to remove the `--check` flag from `git 
> apply`.

Hmm, you mean that `git apply --check test.patch` failed? With error
messages? Or, you had to remove the --check flag in order for the patch
to actually be applied on disk? I would guess it's the latter, but just
to be clear.

>> How comfortable are you with building Git from the sources?
>
> I've never done it before, but I assume it's well documented, so I'm willing 
> to give it a shot!
>
> Happy to try any steps to debug this! Although I'm a bit surprised no-one 
> else can reproduce it with the same version of Git, which makes it seem less 
> likely this could be a bug, and more likely it's something in my setup.

Where do the git 2.17.0 and 2.16.2 come from that you have been testing?
Homebrew? Apple? (Ple

So you should be able to do `git clone https://github.com/git/git.git`
and read INSTALL. It might be useful to start with `git checkout
v2.17.0` to make sure you're testing roughly the same thing as before.

As for obtaining the dependencies, since I'm not familiar with Mac, I
cannot give any good hints.

I see now that Phillip has replied with a good guess. Let's hope he
has managed to circle in on what's causing your problem.

Martin

Re: Regression in patch add?

[Phillip Wood]

On 10/05/18 13:17, Martin Ågren wrote:
> 
> On 10 May 2018 at 12:41, Oliver Joseph Ash  wrote:
>> I just ran into a similar problem: 
>> https://stackoverflow.com/questions/50258565/git-editing-hunks-fails-when-file-has-other-hunks
>>
>> I can reproduce on 2.17.0. The issue doesn't occur on 2.16.2, however.
>>
>> Is this a bug?
> 
> I would think so. Thanks for finding this thread. To keep history
> around, it would be nice to have your reproduction recipe on the list,
> not just on stackoverflow. That said, I cannot reproduce on v2.17.0
> using your recipe. I suspect there is something quite interesting going
> on here, considering how trivial your edit is.

Thanks Oliver for posting an example that we can test, that said I can't
reproduce it on Linux if the hunk is edited correctly. However if I
remove the leading space from the empty line between 'baz' and 'foo'
then I get the same error as you. Perhaps your editor is stripping
trailing white space? If so that will lead to problems when editing
diffs as the leading space is needed for apply to know that it's an
empty context line.

For the mailing list the hunk in question looks like
@@ -1,5 +1,5 @@
 foo
-bar
+bar1
 baz

 foo

I've tried using 'git apply --recount --cached' directly and was
surprised to see that it accepts the patch with the broken context line.
In 2.17.0 'add -p' no longer uses the --recount option, instead it
counts the patch it's self but stops counting when it runs out of lines
starting with [- +], this explains the difference from earlier versions.
It seems it's not uncommon for editors to strip the space from empty
context lines so maybe 'add -p' should take that into account when
recounting patches. I'm about to go off line for a couple of weeks so it
will probably be next month before I'm able to put a patch together
(assuming Junio agrees we should support broken hunks)

Best Wishes

Phillip


> As a shot in the dark, does your test involve unusual file systems,
> funny characters in filenames, ..? You are on some sort of Linux, right?
> 
> The first thing to try out might be something like
> 
> $ # create the initial file as before, with "bar"
> $ # git add, git commit ...
> $ # do the "change bar to bar1" everywhere
> $ git diff >test-patch
> $ git reset --hard
> $ # edit the *FIRST* hunk in test.patch like before (bar1 -> bar2)
> $ git apply --check test.patch && echo "ok..."
> $ git apply test.patch
> 
> Does that succeed at all?
> 
> $ git diff
> 
> should now show bar2 in the first hunk and bar1 in the second hunk,
> just like your edited test.patch.
> 
> If that works, it would seem that the problem is with `git add -p`, and
> how it is generating the patches for `git apply`. I have some ideas
> about how to debug from there, but ... How comfortable are you with
> building Git from the sources? Or with temporarily fiddling around with
> your Git installation? (git-add--interactive is a Perl script, so it
> would be possible to edit it in place to emit various debug
> information. That has potential for messing up royally, though.)
> 
> Martin
> 

Re: Regression in patch add?

[Oliver Joseph Ash]

(Apologies, I accidentally sent this as a reply to the original post, instead 
of your email. I'm new to this!)

> does your test involve unusual file systems, funny characters in filenames, 
> ..? You are on some sort of Linux, right?

I'm running macOS 10.13.4. I don't have any unusual file system setup, as far 
as I'm aware. The filename in my test case is simply `foo`.

I tried the steps you suggested: on git 2.17.0, saving the patch, editing it, 
and applying it, and it succeeded.

> should now show bar2 in the first hunk and bar1 in the second hunk, just like 
> your edited test.patch.

That was the case, although I had to remove the `--check` flag from `git apply`.

> How comfortable are you with building Git from the sources?

I've never done it before, but I assume it's well documented, so I'm willing to 
give it a shot!

Happy to try any steps to debug this! Although I'm a bit surprised no-one else 
can reproduce it with the same version of Git, which makes it seem less likely 
this could be a bug, and more likely it's something in my setup.

Re: Regression in patch add?

[Oliver Joseph Ash]

> does your test involve unusual file systems, funny characters in filenames, 
> ..? You are on some sort of Linux, right?

I'm running macOS 10.13.4. I don't have any unusual file system setup, as far 
as I'm aware. The filename in my test case is simply `foo`.

I tried the steps you suggested: on git 2.17.0, saving the patch, editing it, 
and applying it, and it succeeded.

> should now show bar2 in the first hunk and bar1 in the second hunk, just like 
> your edited test.patch.

That was the case, although I had to remove the `--check` flag from `git apply`.

> How comfortable are you with building Git from the sources?

I've never done it before, but I assume it's well documented, so I'm willing to 
give it a shot!

Happy to try any steps to debug this! Although I'm a bit surprised no-one else 
can reproduce it with the same version of Git, which makes it seem less likely 
this could be a bug, and more likely it's something in my setup.

Re: [PATCH v4 6/6] get_short_oid: document & warn if we ignore the type selector

[Martin Ågren]

On 10 May 2018 at 14:43, Ævar Arnfjörð Bjarmason  wrote:
> The SHA1 prefix 06fa currently matches no blobs in git.git. When
> disambiguating short SHA1s we've been quietly ignoring the user's type
> selector as a fallback mechanism, this was intentionally added in
> 1ffa26c461 ("get_short_sha1: list ambiguous objects on error",
> 2016-09-26).
>
> I think that behavior makes sense, it's not very useful to just show
> nothing because a preference has been expressed via core.disambiguate,
> but it's bad that we're quietly doing this. The user might thing that
> we just didn't understand what e.g 06fa^{blob} meant.
>
> Now we'll instead print a warning if no objects of the requested type
> were found:
>
> $ git rev-parse 06fa^{blob}
> error: short SHA1 06fa is ambiguous
> hint: The candidates are:
> [... no blobs listed ...]
> warning: Your hint (via core.disambiguate or peel syntax) was ignored, we 
> fell
> back to showing all object types since no object of the requested type
> matched the provide short SHA1 06fa

s/ignored, we/ignored. We/? IMHO, it would read easier.

s/provide short/provided short/

Also: s/SHA1/object id/? That said, you add the warning. The error
message is already there and you are simply following its "SHA1".

Martin

Re: [PATCH 0/1] Fix UX issue with commit-graph.lock

[Derrick Stolee]

On 5/10/2018 3:00 AM, Junio C Hamano wrote:

Derrick Stolee  writes:


We use the lockfile API to avoid multiple Git processes from writing to
the commit-graph file in the .git/objects/info directory. In some cases,
this directory may not exist, so we check for its existence.

The existing code does the following when acquiring the lock:

1. Try to acquire the lock.
2. If it fails, try to create the .git/object/info directory.
3. Try to acquire the lock, failing if necessary.

The problem is that if the lockfile exists, then the mkdir fails, giving
an error that doesn't help the user:

   "fatal: cannot mkdir .git/objects/info: File exists"

Isn't a better immediate fix to make the second step pay attention
to errno?  If mkdir() failed due to EEXIST, then we know we tried to
aquire the lock already, so we can die with an appropriate message.

That way, we can keep the "optimize for the normal case" that the
approach to assume object/info/ directory is already there, instead
of always checking its existence which is almost always true
beforehand.


This "optimize for the normal case" is why the existing code is 
organized the way it is.


Since this code is only for writing a commit-graph file, this "check the 
directory first" option is a very small portion of the full time to 
write the file, so the "optimization" has very little effect, 
relatively. My personal opinion is to make the code cleaner when the 
performance difference is negligible.


I'm willing to concede this point and use the steps you suggest below, 
if we think this is the best way forward.



Also, can't we tell why we failed to acquire the lock at step #1?
Do we only get a NULL that says "I am not telling you why, but we
failed to lock"?


To tell why we failed to acquire the lock, we could inspect "errno". 
However, this requires whitebox knowledge of both the lockfile API and 
the tempfile API to know that the last system call to set errno was an 
open() or adjust_shared_perm(). To cleanly make decisions based on the 
reason the lock failed to acquire, I think we would need to modify the 
lockfile and tempfile APIs to return a failure reason. This could be 
done by passing an `int *reason`, but the extra noise in these APIs is 
likely not worth the change.




What I am getting at is that the ideal sequence
would be more like:
 1. Try to acquire the lock.
 2-a. if #1 succeeds, we are happy. ignore the rest and return
  the lock.
 2-b. if #1 failed because object/info/ did not exist,
  mkdir() it, and die if we cannot, saying "cannot mkdir".
 if mkdir() succeeds, jump t 3.
 2-c. if #1 failed but that is not due to missing object/info/,
 die saying "cannot lock".
 3. Try to acquire the lock.
 4-a. if #3 succeeds, we are happy.ignore the rest and return
  the lock.
 4-b. die saying "cannot lock".



Thanks,
-Stolee

[PATCH v4 2/6] sha1-array.h: align function arguments

[Ævar Arnfjörð Bjarmason]

The arguments weren't lined up with the opening parenthesis. Fixes up
code added in aae0caf19e ("sha1-array.h: align function arguments",
2018-04-30).

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 sha1-array.c | 4 ++--
 sha1-array.h | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/sha1-array.c b/sha1-array.c
index 838b3bf847..466a926aa3 100644
--- a/sha1-array.c
+++ b/sha1-array.c
@@ -42,8 +42,8 @@ void oid_array_clear(struct oid_array *array)
 }
 
 int oid_array_for_each_unique(struct oid_array *array,
-   for_each_oid_fn fn,
-   void *data)
+ for_each_oid_fn fn,
+ void *data)
 {
int i;
 
diff --git a/sha1-array.h b/sha1-array.h
index 04b0756334..1e1d24b009 100644
--- a/sha1-array.h
+++ b/sha1-array.h
@@ -17,7 +17,7 @@ void oid_array_clear(struct oid_array *array);
 typedef int (*for_each_oid_fn)(const struct object_id *oid,
   void *data);
 int oid_array_for_each_unique(struct oid_array *array,
-  for_each_oid_fn fn,
-  void *data);
+ for_each_oid_fn fn,
+ void *data);
 
 #endif /* SHA1_ARRAY_H */
-- 
2.17.0.410.g4ac3413cc8

[PATCH v4 6/6] get_short_oid: document & warn if we ignore the type selector

[Ævar Arnfjörð Bjarmason]

The SHA1 prefix 06fa currently matches no blobs in git.git. When
disambiguating short SHA1s we've been quietly ignoring the user's type
selector as a fallback mechanism, this was intentionally added in
1ffa26c461 ("get_short_sha1: list ambiguous objects on error",
2016-09-26).

I think that behavior makes sense, it's not very useful to just show
nothing because a preference has been expressed via core.disambiguate,
but it's bad that we're quietly doing this. The user might thing that
we just didn't understand what e.g 06fa^{blob} meant.

Now we'll instead print a warning if no objects of the requested type
were found:

$ git rev-parse 06fa^{blob}
error: short SHA1 06fa is ambiguous
hint: The candidates are:
[... no blobs listed ...]
warning: Your hint (via core.disambiguate or peel syntax) was ignored, we 
fell
back to showing all object types since no object of the requested type
matched the provide short SHA1 06fa

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 sha1-name.c | 11 ++-
 t/t1512-rev-parse-disambiguation.sh |  5 -
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/sha1-name.c b/sha1-name.c
index 46d8b1afa6..df33cc2dba 100644
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -438,6 +438,7 @@ static int get_short_oid(const char *name, int len, struct 
object_id *oid,
 
if (!quietly && (status == SHORT_NAME_AMBIGUOUS)) {
struct oid_array collect = OID_ARRAY_INIT;
+   int ignored_hint = 0;
 
error(_("short SHA1 %s is ambiguous"), ds.hex_pfx);
 
@@ -447,8 +448,10 @@ static int get_short_oid(const char *name, int len, struct 
object_id *oid,
 * that case, we still want to show them, so disable the hint
 * function entirely.
 */
-   if (!ds.ambiguous)
+   if (!ds.ambiguous) {
ds.fn = NULL;
+   ignored_hint = 1;
+   }
 
advise(_("The candidates are:"));
for_each_abbrev(ds.hex_pfx, collect_ambiguous, );
@@ -457,6 +460,12 @@ static int get_short_oid(const char *name, int len, struct 
object_id *oid,
if (oid_array_for_each(, show_ambiguous_object, ))
BUG("show_ambiguous_object shouldn't return non-zero");
oid_array_clear();
+
+   if (ignored_hint) {
+   warning(_("Your hint (via core.disambiguate or peel 
syntax) was ignored, we fell\n"
+ "back to showing all object types since no 
object of the requested type\n"
+ "matched the provide short SHA1 %s"), 
ds.hex_pfx);
+   }
}
 
return status;
diff --git a/t/t1512-rev-parse-disambiguation.sh 
b/t/t1512-rev-parse-disambiguation.sh
index 2701462041..1f06c1e61f 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -344,7 +344,10 @@ test_expect_success C_LOCALE_OUTPUT 'failed type-selector 
still shows hint' '
echo 872 | git hash-object --stdin -w &&
test_must_fail git rev-parse ee3d^{commit} 2>stderr &&
grep ^hint: stderr >hints &&
-   test_line_count = 3 hints
+   test_line_count = 3 hints &&
+   grep ^warning stderr >warnings &&
+   grep -q "Your hint.*was ignored" warnings &&
+   grep -q "the provide short SHA1 ee3d" stderr
 '
 
 test_expect_success 'core.disambiguate config can prefer types' '
-- 
2.17.0.410.g4ac3413cc8

[PATCH v4 1/6] sha1-name.c: remove stray newline

[Ævar Arnfjörð Bjarmason]

This stray newline was accidentally introduced in
d2b7d9c7ed ("sha1_name: convert disambiguate_hint_fn to take
object_id", 2017-03-26).

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 sha1-name.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/sha1-name.c b/sha1-name.c
index 5b93bf8da3..cd3b133aae 100644
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -346,7 +346,6 @@ static int show_ambiguous_object(const struct object_id 
*oid, void *data)
struct strbuf desc = STRBUF_INIT;
int type;
 
-
if (ds->fn && !ds->fn(oid, ds->cb_data))
return 0;
 
-- 
2.17.0.410.g4ac3413cc8

[PATCH v4 4/6] sha1-name.c: move around the collect_ambiguous() function

[Ævar Arnfjörð Bjarmason]

A subsequent change will make use of this static function in the
get_short_oid() function, which is defined above where the
collect_ambiguous() function is now. Without this we'd then have a
compilation error due to a forward declaration.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 sha1-name.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/sha1-name.c b/sha1-name.c
index cd3b133aae..9d7bbd3e96 100644
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -372,6 +372,12 @@ static int show_ambiguous_object(const struct object_id 
*oid, void *data)
return 0;
 }
 
+static int collect_ambiguous(const struct object_id *oid, void *data)
+{
+   oid_array_append(data, oid);
+   return 0;
+}
+
 static int get_short_oid(const char *name, int len, struct object_id *oid,
  unsigned flags)
 {
@@ -421,12 +427,6 @@ static int get_short_oid(const char *name, int len, struct 
object_id *oid,
return status;
 }
 
-static int collect_ambiguous(const struct object_id *oid, void *data)
-{
-   oid_array_append(data, oid);
-   return 0;
-}
-
 int for_each_abbrev(const char *prefix, each_abbrev_fn fn, void *cb_data)
 {
struct oid_array collect = OID_ARRAY_INIT;
-- 
2.17.0.410.g4ac3413cc8

[PATCH v4 3/6] git-p4: change "commitish" typo to "committish"

[Ævar Arnfjörð Bjarmason]

This was the only occurrence of "commitish" in the tree, but as the
log will reveal we've had others in the past. Fixes up code added in
00ad6e3182 ("git-p4: work with a detached head", 2015-11-21).

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 git-p4.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 7bb9cadc69..1afa87cd9d 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2099,11 +2099,11 @@ class P4Submit(Command, P4UserMap):
 
 commits = []
 if self.master:
-commitish = self.master
+committish = self.master
 else:
-commitish = 'HEAD'
+committish = 'HEAD'
 
-for line in read_pipe_lines(["git", "rev-list", "--no-merges", 
"%s..%s" % (self.origin, commitish)]):
+for line in read_pipe_lines(["git", "rev-list", "--no-merges", 
"%s..%s" % (self.origin, committish)]):
 commits.append(line.strip())
 commits.reverse()
 
-- 
2.17.0.410.g4ac3413cc8

[PATCH v4 0/6] get_short_oid UI improvements

[Ævar Arnfjörð Bjarmason]

This is like v3 except all the patches to the peel syntax & docs have
been dropped, which were controversial.

I think it's worthwhile to re-work that, but I don't have time for
that now, so I'm submitting this. Maybe I'll have time in the future
to re-work the rest, but then I can base it on top of this.

Ævar Arnfjörð Bjarmason (6):
  sha1-name.c: remove stray newline
  sha1-array.h: align function arguments
  git-p4: change "commitish" typo to "committish"
  sha1-name.c: move around the collect_ambiguous() function
  get_short_oid: sort ambiguous objects by type, then SHA-1
  get_short_oid: document & warn if we ignore the type selector

 Documentation/technical/api-oid-array.txt | 17 ---
 git-p4.py |  6 +--
 sha1-array.c  | 21 +++-
 sha1-array.h  |  7 ++-
 sha1-name.c   | 61 +++
 t/t1512-rev-parse-disambiguation.sh   | 26 +-
 6 files changed, 115 insertions(+), 23 deletions(-)

-- 
2.17.0.410.g4ac3413cc8

[PATCH v4 5/6] get_short_oid: sort ambiguous objects by type, then SHA-1

[Ævar Arnfjörð Bjarmason]

Change the output emitted when an ambiguous object is encountered so
that we show tags first, then commits, followed by trees, and finally
blobs. Within each type we show objects in hashcmp() order. Before
this change the objects were only ordered by hashcmp().

The reason for doing this is that the output looks better as a result,
e.g. the v2.17.0 tag before this change on "git show e8f2" would
display:

hint: The candidates are:
hint:   e8f2093055 tree
hint:   e8f21caf94 commit 2013-06-24 - bash prompt: print unique detached 
HEAD abbreviated object name
hint:   e8f21d02f7 blob
hint:   e8f21d577c blob
hint:   e8f25a3a50 tree
hint:   e8f26250fa commit 2017-02-03 - Merge pull request #996 from 
jeffhostetler/jeffhostetler/register_rename_src
hint:   e8f2650052 tag v2.17.0
hint:   e8f2867228 blob
hint:   e8f28d537c tree
hint:   e8f2a35526 blob
hint:   e8f2bc0c06 commit 2015-05-10 - Documentation: note behavior for 
multiple remote.url entries
hint:   e8f2cf6ec0 tree

Now we'll instead show:

hint:   e8f2650052 tag v2.17.0
hint:   e8f21caf94 commit 2013-06-24 - bash prompt: print unique detached 
HEAD abbreviated object name
hint:   e8f26250fa commit 2017-02-03 - Merge pull request #996 from 
jeffhostetler/jeffhostetler/register_rename_src
hint:   e8f2bc0c06 commit 2015-05-10 - Documentation: note behavior for 
multiple remote.url entries
hint:   e8f2093055 tree
hint:   e8f25a3a50 tree
hint:   e8f28d537c tree
hint:   e8f2cf6ec0 tree
hint:   e8f21d02f7 blob
hint:   e8f21d577c blob
hint:   e8f2867228 blob
hint:   e8f2a35526 blob

Since we show the commit data in the output that's nicely aligned once
we sort by object type. The decision to show tags before commits is
pretty arbitrary. I don't want to order by object_type since there
tags come last after blobs, which doesn't make sense if we want to
show the most important things first.

I could display them after commits, but it's much less likely that
we'll display a tag, so if there is one it makes sense to show it
prominently at the top.

A note on the implementation: Derrick rightly pointed out[1] that
we're bending over backwards here in get_short_oid() to first
de-duplicate the list, and then emit it, but could simply do it in one
step.

The reason for that is that oid_array_for_each_unique() doesn't
actually require that the array be sorted by oid_array_sort(), it just
needs to be sorted in some order that guarantees that all objects with
the same ID are adjacent to one another, which (barring a hash
collision, which'll be someone else's problem) the sort_ambiguous()
function does.

I agree that would be simpler for this code, and had forgotten why I
initially wrote it like this[2]. But on further reflection I think
it's better to do more work here just so we're not underhandedly using
the oid-array API where we lie about the list being sorted. That would
break any subsequent use of oid_array_lookup() in subtle ways.

I could get around that by hacking the API itself to support this
use-case and documenting it, which I did as a WIP patch in [3], but I
think it's too much code smell just for this one call site. It's
simpler for the API to just introduce a oid_array_for_each() function
to eagerly spew out the list without sorting or de-duplication, and
then do the de-duplication and sorting in two passes.

1. https://public-inbox.org/git/20180501130318.58251-1-dsto...@microsoft.com/
2. https://public-inbox.org/git/876047ze9v@evledraar.gmail.com/
3. https://public-inbox.org/git/874ljrzctc@evledraar.gmail.com/

Helped-by: Derrick Stolee 
Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/technical/api-oid-array.txt | 17 +++
 sha1-array.c  | 17 +++
 sha1-array.h  |  3 ++
 sha1-name.c   | 37 ++-
 t/t1512-rev-parse-disambiguation.sh   | 21 +
 5 files changed, 88 insertions(+), 7 deletions(-)

diff --git a/Documentation/technical/api-oid-array.txt 
b/Documentation/technical/api-oid-array.txt
index b0c11f868d..94b529722c 100644
--- a/Documentation/technical/api-oid-array.txt
+++ b/Documentation/technical/api-oid-array.txt
@@ -35,13 +35,18 @@ Functions
Free all memory associated with the array and return it to the
initial, empty state.
 
+`oid_array_for_each`::
+   Iterate over each element of the list, executing the callback
+   function for each one. Does not sort the list, so any custom
+   hash order is retained. If the callback returns a non-zero
+   value, the iteration ends immediately and the callback's
+   return is propagated; otherwise, 0 is returned.
+
 `oid_array_for_each_unique`::
-   Efficiently iterate over each unique element of the list,
-   executing the callback function for each one. If the array is
-   not 

Re: Regression in patch add?

[Martin Ågren]

On 10 May 2018 at 12:41, Oliver Joseph Ash  wrote:
> I just ran into a similar problem: 
> https://stackoverflow.com/questions/50258565/git-editing-hunks-fails-when-file-has-other-hunks
>
> I can reproduce on 2.17.0. The issue doesn't occur on 2.16.2, however.
>
> Is this a bug?

I would think so. Thanks for finding this thread. To keep history
around, it would be nice to have your reproduction recipe on the list,
not just on stackoverflow. That said, I cannot reproduce on v2.17.0
using your recipe. I suspect there is something quite interesting going
on here, considering how trivial your edit is.

As a shot in the dark, does your test involve unusual file systems,
funny characters in filenames, ..? You are on some sort of Linux, right?

The first thing to try out might be something like

$ # create the initial file as before, with "bar"
$ # git add, git commit ...
$ # do the "change bar to bar1" everywhere
$ git diff >test-patch
$ git reset --hard
$ # edit the *FIRST* hunk in test.patch like before (bar1 -> bar2)
$ git apply --check test.patch && echo "ok..."
$ git apply test.patch

Does that succeed at all?

$ git diff

should now show bar2 in the first hunk and bar1 in the second hunk,
just like your edited test.patch.

If that works, it would seem that the problem is with `git add -p`, and
how it is generating the patches for `git apply`. I have some ideas
about how to debug from there, but ... How comfortable are you with
building Git from the sources? Or with temporarily fiddling around with
your Git installation? (git-add--interactive is a Perl script, so it
would be possible to edit it in place to emit various debug
information. That has potential for messing up royally, though.)

Martin

Re: [PATCH 2/2] replace-object.c: remove the_repository from prepare_replace_object

[Jeff King]

On Thu, May 10, 2018 at 07:23:13PM +0900, Junio C Hamano wrote:

> This one was doing
> 
>   ptr = xmalloc(sizeof(*another_ptr))
> 
> and it was OK because ptr and another_ptr happened to be of the same
> type.  I wonder if we are making it safer, or making it more obscure
> to seasoned C programmers, if we introduced a pair of helper macros,
> perhaps like these:
> 
>   #define ALLOCATE(ptr) (ptr) = xmalloc(sizeof(*(ptr)))
>   #define CALLOCATE(ptr,cnt) (ptr) = xcalloc((cnt), sizeof(*(ptr)))

I've often wondered that, too. It's the natural endgame of the
ALLOC_ARRAY() road we've been going down.

-Peff

Re: Generate more revenue from Git

[Michal Sapozhnikov]

Hi,

I am writing with the hope of talking to the appropriate person who handles the
app's monetization.
If it makes sense to have a call, let me know how your schedule looks.

Best Regards,
-- 
Michal Sapozhnikov | Business Manager, Luminati SDK | +972-50-2826778 | Skype:
live:michals_43
http://luminati.io/sdk

On 03-May-18 09:58, 7d (by eremind) wrote:
> 
> Hello,
> 
> Following up on my last email,
> 
> It would be great to setup a call this week.
> 
> Looking forward to your response.
> 
> Best Regards,
> 

Re: [PATCH] fast-export: avoid NULL pointer arithmetic

[Junio C Hamano]

René Scharfe  writes:

> The standard says about uintptr_t that "any valid pointer to void can
> be converted to this type, then converted back to pointer to void, and
> the result will compare equal to the original pointer".  So void * ->
> uintptr_t -> void * is a proper roundtrip, but that doesn't imply that
> casting arbitrary uintptr_t values to void * would be lossless.
>
> I don't know an architecture where this would bite us, but I wonder if
> there is a cleaner way.  Perhaps changing the type of the decoration
> member of struct decoration_entry in decorate.h to uintptr_t?

In order to ensure "void * -> uintptr_t -> void *" roundtrip holds,
the implementation would guarantee that uintptr_t is wider than
void*, so what you suggest technically makes sense.  We should be
able to store any pointer in the field.  And we should be able to
store any value of an unsigned integral type that is narrower than
uintptr_t.

But it somehow feels backwards in spirit to me, as the reason why we
use "void *" there in the decoration field is because we expect that
we'd have a pointer to some struture most of the time, and we have
to occasionally store a small integer there.  So I'd naively expect
that

uint32_t mark = 23;
de->decoration = (void *)mark;

would be a good way to store mark #23 in the field and

uint32_t mark;
mark = (typeof(mark))de->decoration;

would be a good way to read it off of the "void *" field.  Of
course, this assume that (void *) is at least as wide as 32-bit and
it also ignores the standard ;-)

This is an unrelated tangent but the mark-to-ptr() and ptr-to-mark()
implementations feel wasteful, especially when we worry about 32-bit
archs.  A naive platform implementation of

(uint32_t *)mark - (uint32_t *)NULL;

would be ((uintptr_t)mark) / 4, i.e. the de->decoration field will
always have two LSB clear and only utilize top 30-bit to represent
the value of mark.

Re: Regression in patch add?

[Oliver Joseph Ash]

I just ran into a similar problem: 
https://stackoverflow.com/questions/50258565/git-editing-hunks-fails-when-file-has-other-hunks

I can reproduce on 2.17.0. The issue doesn't occur on 2.16.2, however.

Is this a bug?

Re: [PATCH 2/2] replace-object.c: remove the_repository from prepare_replace_object

[Junio C Hamano]

Stefan Beller  writes:

> This was missed in 5982da9d2ce (replace-object: allow
> prepare_replace_object to handle arbitrary repositories, 2018-04-11)
>
> Technically the code works correctly as the replace_map is the same
> size in different repositories, however it is hard to read. So convert
> the code to the familiar pattern of dereferencing the pointer that we
> assign in the sizeof itself.

;-)

We say

ptr = xmalloc(sizeof(*ptr))

is better because 

ptr = xmalloc(sizeof(typeof(*ptr)))

is easy to go stale unless we actually use typeof and instead say a
concrete type like "struct oidmap".

This one was doing

ptr = xmalloc(sizeof(*another_ptr))

and it was OK because ptr and another_ptr happened to be of the same
type.  I wonder if we are making it safer, or making it more obscure
to seasoned C programmers, if we introduced a pair of helper macros,
perhaps like these:

#define ALLOCATE(ptr) (ptr) = xmalloc(sizeof(*(ptr)))
#define CALLOCATE(ptr,cnt) (ptr) = xcalloc((cnt), sizeof(*(ptr)))

The change looks obviously good.  Will queue.

Thanks.

Re: [PATCH 1/2] object.c: free replace map in raw_object_store_clear

[Junio C Hamano]

Stefan Beller  writes:

> The replace map for objects was missed to free in the object store in
> the conversion of 174774cd519 (Merge branch 'sb/object-store-replace',
> 2018-05-08)

I need a bit of clarification wrt the above.  The above makes it
sound like the merge needed a semantic conflict resolution (e.g. one
side turned replace_map into a pointer while the other side added a
place where the containing structure is freed and now the merge
result needs to free the pointer in the new place that frees the
containing structure, but the merge forgot to do so).  Is that what
is going on?

Or is this just a simple "the topic that ends at 174774cd519^2 had
this leak that needs to be fixed by this patch; instead of rerolling
this is an incremental, because the topic has already been merged to
'master' and it is too late now"?

Looking at this patch in the context of the side branch (instead of
in the merged result) already makes sense to me, so I am guessing it
is the latter (i.e. not a botched merge that missed semantic
conflicts), in which case the proposed log message is a bit too
alarming and points readers in a wrong direction.  Shouldn't it
point at, say, c1274495 ("replace-object: eliminate replace objects
prepared flag", 2018-04-11) that turned the oidmap instance into a
pointer in raw_object_store?

Thanks.

>
> Signed-off-by: Stefan Beller 
> ---
>  object.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/object.c b/object.c
> index 9d5b10d5a20..ff28f90c5ef 100644
> --- a/object.c
> +++ b/object.c
> @@ -497,6 +497,7 @@ void raw_object_store_clear(struct raw_object_store *o)
>  {
>   FREE_AND_NULL(o->objectdir);
>   FREE_AND_NULL(o->alternate_db);
> + FREE_AND_NULL(o->replace_map);
>  
>   free_alt_odbs(o);
>   o->alt_odb_tail = NULL;

[PATCH] completion: add diff --color-moved and config diff.colorMoved

[John Marshall]

Complete the --color-moved option wherever we complete --diff-algorithm.

Signed-off-by: John Marshall 
---
Complete this recently-added option in a slightly over-the-top number of
places. Patch based on the maint branch.

Cheers,

John

 contrib/completion/git-completion.bash | 24 ++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index b09c8a236..4e09aebd0 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1383,9 +1383,12 @@ __git_diff_algorithms="myers minimal patience histogram"

 __git_diff_submodule_formats="diff log short"

+__git_diff_colormoveds="no default plain zebra dimmed_zebra"
+
 __git_diff_common_options="--stat --numstat --shortstat --summary
-   --patch-with-stat --name-only --name-status --color
-   --no-color --color-words --no-renames --check
+   --patch-with-stat --name-only --name-status
+   --color --no-color --color-moved --no-color-moved
+   --color-words --no-renames --check
--full-index --binary --abbrev --diff-filter=
--find-copies-harder --ignore-cr-at-eol
--text --ignore-space-at-eol --ignore-space-change
@@ -1406,6 +1409,10 @@ _git_diff ()
__git_has_doubledash && return

case "$cur" in
+   --color-moved=*)
+   __gitcomp "$__git_diff_colormoveds" "" "${cur##--color-moved=}"
+   return
+   ;;
--diff-algorithm=*)
__gitcomp "$__git_diff_algorithms" "" 
"${cur##--diff-algorithm=}"
return
@@ -1702,6 +1709,10 @@ _git_log ()
__gitcomp "full short no" "" "${cur##--decorate=}"
return
;;
+   --color-moved=*)
+   __gitcomp "$__git_diff_colormoveds" "" "${cur##--color-moved=}"
+   return
+   ;;
--diff-algorithm=*)
__gitcomp "$__git_diff_algorithms" "" 
"${cur##--diff-algorithm=}"
return
@@ -2165,6 +2176,10 @@ _git_config ()
"
return
;;
+   diff.color[Mm]oved)
+   __gitcomp "false true $__git_diff_colormoveds"
+   return
+   ;;
diff.submodule)
__gitcomp "log short"
return
@@ -2393,6 +2408,7 @@ _git_config ()
credential.username
credentialCache.ignoreSIGHUP
diff.autorefreshindex
+   diff.colorMoved
diff.external
diff.ignoreSubmodules
diff.mnemonicprefix
@@ -2741,6 +2757,10 @@ _git_show ()
" "" "${cur#*=}"
return
;;
+   --color-moved=*)
+   __gitcomp "$__git_diff_colormoveds" "" "${cur##--color-moved=}"
+   return
+   ;;
--diff-algorithm=*)
__gitcomp "$__git_diff_algorithms" "" 
"${cur##--diff-algorithm=}"
return
--
2.17.0


[University of Glasgow: The Times Scottish University of the Year 2018]

Re: [PATCH 2/2] git-credential-netrc: accept gpg option

[Junio C Hamano]

Luis Marsano  writes:

> git-credential-netrc was hardcoded to decrypt with 'gpg' regardless of the 
> gpg.program option
> this now uses the gpg command option if set, else, the gpg.program option set 
> in the git repository or global configuration, else defaults to 'gpg'
> for git-credential-netrc

These lines are way overlong.  Wrap at around 72-78 cols, perhaps.
Complete each sentence with a full-stop.

> - use Git.pm for repository and global option queries
> - add -g|--gpg command option & document it in command usage
> - test repository & command options
> - support unicode

There are other changes that are not explained/justified here, I
think.

 - Instead of ALLCAPS as a placeholder for a command line argument in
   the help text, use , because doing so is better due
   to such and such reasons.

I think it is good to consistently do so, but it is unclear why
ALLCAPS is bad and  is better.  That needs to be
explained.

 - Replace three-dots in the help text with U+2026 to punish those
   who are still using unicode-inapable terminal in this century.

I do not think this part of the patch is a good idea at all, but
perhaps I misunderstood the reason behind this change you had in
mind (as you did not explain it in the proposed log message).

> @@ -62,27 +69,31 @@ if ($options{help}) {
>  
>   print <  
> -$0 [-f AUTHFILE1] [-f AUTHFILEN] [-d] [-v] [-k] get
> +$0 [(-f )…] [-g ] [-d] [-v] [-k] get

Is this a desired change, or unwanted change left in the patch by
accident?


> -...and if you want lots of debugging info:
> +…and if you want lots of debugging info:

Is this a desired change, or unwanted change left in the patch by
accident?

>  
>git config credential.helper '$shortname -f AUTHFILE -d'
>  
> -...or to see the files opened and data found:
> +…or to see the files opened and data found:
>  

Ditto.

>git config credential.helper '$shortname -f AUTHFILE -v'
>  
> -Only "get" mode is supported by this credential helper.  It opens every 
> AUTHFILE
> +Only "get" mode is supported by this credential helper.  It opens every 
> 
>  and looks for the first entry that matches the requested search criteria:
>  
>   'port|protocol':
> @@ -120,7 +131,7 @@ host=github.com
>  protocol=https
>  username=tzz
>  
> -this credential helper will look for the first entry in every AUTHFILE that
> +this credential helper will look for the first entry in every  that
>  matches
>  
>  machine github.com port https login tzz
> @@ -129,7 +140,7 @@ OR
>  
>  machine github.com protocol https login tzz
>  
> -OR... etc. acceptable tokens as listed above.  Any unknown tokens are
> +OR… etc. acceptable tokens as listed above.  Any unknown tokens are

Ditto.

>  # Credentials must get a parameter, so die if it's missing.
> -die "Syntax: $0 [-f AUTHFILE1] [-f AUTHFILEN] [-d] get" unless defined $mode;
> +die "Syntax: $0 [(-f )…] [-d] get" unless defined $mode;

Ditto.

Re: [PATCH 1/2] git-credential-netrc: adapt to test framework for git

[Junio C Hamano]

Luis Marsano  writes:

> until this change, git-credential-netrc did not test in a repository
> this change reuses the main test framework, which provides such tests
> specific changes

Sorry, but I cannot quite parse what the above is trying to say.

> - switch to Test::More module
> - use File::Basename & File::Spec::Functions
>
> Signed-off-by: Luis Marsano 
> Acked-by: Ted Zlatanov 
> ---
>  contrib/credential/netrc/Makefile |  4 +-
>  .../netrc/t-git-credential-netrc.sh   | 31 
>  contrib/credential/netrc/test.pl  | 73 ---
>  3 files changed, 78 insertions(+), 30 deletions(-)
>  create mode 100755 contrib/credential/netrc/t-git-credential-netrc.sh

Will queue, but may need to make the log message
readable/understandable.

Thanks.

Re: [PATCH] repository: fix free problem with repo_clear(the_repository)

[Junio C Hamano]

Stefan Beller  writes:

> So this would go with the latest sb/object-store-alloc ?
>
> My impression was that we never call repo_clear() on
> the_repository, which would allow us to special case
> the_repository further just as I did in v2 of that series[1] by
> having static allocations for certain objects in case of \
> the_repository.
>
> [1] https://public-inbox.org/git/20180501213403.14643-14-sbel...@google.com/
>
> We could just deal with all the exceptions, but that makes repo_clear
> ugly IMHO.

So perhaps

 void repo_clear(...)
 {
+   if (repo == the_repository)
+   BUG("repo_clear() called on the repository");
...

or something?

Re: [PATCH] fast-export: avoid NULL pointer arithmetic

[René Scharfe]

Am 09.05.2018 um 23:06 schrieb René Scharfe:
> Clang 6 reports the following warning, which is turned into an error in a
> DEVELOPER build:
> 
>   builtin/fast-export.c:162:28: error: performing pointer arithmetic on a 
> null pointer has undefined behavior [-Werror,-Wnull-pointer-arithmetic]
>   return ((uint32_t *)NULL) + mark;
>  ~~ ^
>   1 error generated.
> 
> The compiler is correct, and the error message speaks for itself.  There
> is no need for any undefined operation -- just cast mark to void * or
> uint32_t after an intermediate cast to uintptr_t.  That encodes the
> integer value into a pointer and later decodes it as intended.

Having thought about it a bit more I have to say: That seems to work,
but it's not portable.  

The standard says about uintptr_t that "any valid pointer to void can
be converted to this type, then converted back to pointer to void, and
the result will compare equal to the original pointer".  So void * ->
uintptr_t -> void * is a proper roundtrip, but that doesn't imply that
casting arbitrary uintptr_t values to void * would be lossless.

I don't know an architecture where this would bite us, but I wonder if
there is a cleaner way.  Perhaps changing the type of the decoration
member of struct decoration_entry in decorate.h to uintptr_t?

> While at it remove an outdated comment -- intptr_t has been used since
> ffe659f94d (parse-options: make some arguments optional, add callbacks),
> committed in October 2007.
> 
> Signed-off-by: Rene Scharfe 
> ---
>   builtin/fast-export.c | 7 +++
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index 530df12f05..fa556a3c93 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -156,15 +156,14 @@ static void anonymize_path(struct strbuf *out, const 
> char *path,
>   }
>   }
>   
> -/* Since intptr_t is C99, we do not use it here */
> -static inline uint32_t *mark_to_ptr(uint32_t mark)
> +static inline void *mark_to_ptr(uint32_t mark)
>   {
> - return ((uint32_t *)NULL) + mark;
> + return (void *)(uintptr_t)mark;
>   }
>   
>   static inline uint32_t ptr_to_mark(void * mark)
>   {
> - return (uint32_t *)mark - (uint32_t *)NULL;
> + return (uint32_t)(uintptr_t)mark;
>   }
>   
>   static inline void mark_object(struct object *object, uint32_t mark)
> 

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

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

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

[1] We may actually show commands that are not built (e.g. if you set
NO_PERL you don't have git-instaweb but it's still listed here). I
ignore the problem because on Linux a git package could be split
anyway. The "git-core" package may not contain git-instaweb even if
it's built because it may end up in a separate package. We can't know
anyway.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git-help.txt |  4 +++-
 builtin/help.c |  7 +++
 help.c | 16 
 help.h |  2 ++
 t/t0012-help.sh|  9 +
 5 files changed, 37 insertions(+), 1 deletion(-)

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

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

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

This is intended to help anybody who needs to update command-list.txt.
It gives a brief introduction of all attributes a command can take.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 command-list.txt | 44 
 1 file changed, 44 insertions(+)

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

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

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

Instead of parsing "git help -a" output, which is tricky to get right,
less elegant and also slow, make git provide the list in a
machine-friendly form. This adds two separate listing types, main and
others, instead of just "all" for more flexibility.

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

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

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

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

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

'parseopt' remains separate because it has separate (SPC) to match
git-completion.bash needs and will not combine with others.

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

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

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

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

Instead of printing the command directly one by one, keep them in a
list and print at the end. This allows more modification before we
print out (e.g. sorting, removing duplicates or even excluding some
items).

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 git.c | 22 +-
 1 file changed, 17 insertions(+), 5 deletions(-)

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

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

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

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

This will be used later by git-completion.bash to retrieve certain
command groups.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 generate-cmdlist.sh | 17 +
 git.c   |  7 +++
 help.c  | 23 +++
 help.h  |  2 ++
 4 files changed, 49 insertions(+)

diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
index 3bcc1ee57d..8d6d8b45ce 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 10907f7266..4d5b8a9931 100644
--- a/git.c
+++ b/git.c
@@ -60,6 +60,13 @@ static int list_cmds(const char *spec)
list_all_main_cmds();
else if (match_token(spec, len, "others"))
list_all_other_cmds();
+   else if (len > 5 && !strncmp(spec, "list-", 5)) {
+   struct strbuf sb = STRBUF_INIT;
+
+   strbuf_add(, spec + 5, len - 5);
+   list_cmds_by_category(, sb.buf);
+   strbuf_release();
+   }
else
die(_("unsupported command listing type '%s'"), spec);
spec += len;
diff --git a/help.c b/help.c
index d5ce9dfcbb..1117f7d1d1 100644
--- a/help.c
+++ b/help.c
@@ -329,6 +329,29 @@ void list_all_other_cmds(struct string_list *list)
clean_cmdnames(_cmds);
 }
 
+void list_cmds_by_category(struct string_list *list,
+  const char *cat)
+{
+   int i, n = ARRAY_SIZE(command_list);
+   uint32_t cat_id = 0;
+
+   for (i = 0; category_names[i]; i++) {
+   if (!strcmp(cat, category_names[i])) {
+   cat_id = 1UL << i;
+   break;
+   }
+   }
+   if (!cat_id)
+   die(_("unsupported command listing type '%s'"), cat);
+
+   for (i = 0; i < n; i++) {
+   struct cmdname_help *cmd = command_list + i;
+
+   if (cmd->category & cat_id)
+   string_list_append(list, drop_prefix(cmd->name));
+   }
+}
+
 int is_in_cmdlist(struct cmdnames *c, const char *s)
 {
int i;
diff --git a/help.h b/help.h
index 97e6c0965e..734bba32d3 100644
--- a/help.h
+++ b/help.h
@@ -21,6 +21,8 @@ static inline void mput_char(char c, unsigned int num)
 extern void list_common_cmds_help(void);
 extern void list_all_main_cmds(struct string_list *list);
 extern void list_all_other_cmds(struct string_list *list);
+extern void list_cmds_by_category(struct string_list *list,
+ const char *category);
 extern const char *help_unknown_cmd(const char *cmd);
 extern void load_command_list(const char *prefix,
  struct cmdnames *main_cmds,
-- 
2.17.0.705.g3525833791

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

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

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

The description of each common command group is removed from
command-list.txt. This keeps this file format simpler. common-cmds.h
will not be generated correctly after this change due to the
command-list.txt format change. But it does not matter and
common-cmds.h will be removed.

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

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

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

After the last patch, common-cmds.h is no longer used (and it was
actually broken). Remove all related code. command-list.h will take
its place from now on.

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

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

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

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

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

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

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

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

PS. The new fixed column requirement on command-list.txt is
technically not needed. But it helps simplify the code a bit at this
stage. We could lift this restriction later if we want to.

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 f181687250..2a8913ea21 100644
--- a/Makefile
+++ b/Makefile
@@ -757,7 +757,7 @@ LIB_FILE = libgit.a
 XDIFF_LIB = xdiff/lib.a
 VCSSVN_LIB = vcs-svn/lib.a
 
-GENERATED_H += common-cmds.h
+GENERATED_H += common-cmds.h command-list.h
 
 LIB_H = $(shell $(FIND) . \
-name .git -prune -o \
@@ -1938,6 +1938,11 @@ $(BUILT_INS): git$X
 common-cmds.h: generate-cmdlist.sh command-list.txt
 
 common-cmds.h: $(wildcard Documentation/git-*.txt)
+   $(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh command-list.txt COMMON 
>$@+ && mv $@+ $@
+
+command-list.h: generate-cmdlist.sh command-list.txt
+
+command-list.h: $(wildcard Documentation/git-*.txt)
$(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh command-list.txt >$@+ 
&& mv $@+ $@
 
 SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
@@ -2148,7 +2153,7 @@ else
 # Dependencies on header files, for platforms that do not support
 # the gcc -MMD option.
 #
-# Dependencies on automatically generated headers such as common-cmds.h
+# Dependencies on automatically generated headers such as common-cmds.h or 
command-list.h
 # should _not_ be included here, since they are necessary even when
 # building an object for the first time.
 
@@ -2527,7 +2532,7 @@ sparse: $(SP_OBJ)
 style:
git clang-format --style file --diff --extensions c,h
 
-check: common-cmds.h
+check: common-cmds.h command-list.h
@if sparse; \
then \
echo >&2 "Use 'make sparse' instead"; \
@@ -2775,7 +2780,7 @@ clean: profile-clean coverage-clean
$(RM) $(TEST_PROGRAMS) $(NO_INSTALL)
$(RM) -r bin-wrappers $(dep_dirs)
$(RM) -r po/build/
-   $(RM) *.pyc *.pyo */*.pyc */*.pyo common-cmds.h $(ETAGS_TARGET) tags 
cscope*
+   $(RM) *.pyc *.pyo */*.pyc */*.pyo common-cmds.h command-list.h 
$(ETAGS_TARGET) tags cscope*
$(RM) -r $(GIT_TARNAME) .doc-tmp-dir
$(RM) $(GIT_TARNAME).tar.gz git-core_$(GIT_VERSION)-*.tar.gz
$(RM) $(htmldocs).tar.gz $(manpages).tar.gz
diff --git a/command-list.txt b/command-list.txt
index a1fad28fd8..786536aba0 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -8,8 +8,8 @@ info examine the history and state (see also: git help 
revisions)
 history  grow, mark and tweak your common history
 remote   collaborate (see also: git help workflows)
 
-### command list (do not change this line)
-# command name  category [deprecated] [common]
+### command list (do not change this line, also do not change alignment)
+# command name  category [category] [category]
 git-add mainporcelain   worktree
 git-am  mainporcelain
 git-annotateancillaryinterrogators
diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
index 31b6d886cb..870d3b626a 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 v7 12/13] completion: let git provide the completable command list

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

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

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

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

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

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

(*) to be fair, send-email command which is in the same
foreignscminterface group as svn and p4 does get completion, just
because it's used by git and kernel development. So maybe should
include them.

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

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

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

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

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 | 21 +
 help.h |  1 +
 t/t0012-help.sh|  6 +
 10 files changed, 54 insertions(+), 45 deletions(-)

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

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

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

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

PS. perhaps I should make aliases a group too, which makes it possible
to _not_ complete aliases by omitting this special group in
$GIT_COMPLETION_CMD_GROUPS

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/config.txt   | 10 
 contrib/completion/git-completion.bash | 28 +-
 git.c  |  2 ++
 help.c | 33 ++
 help.h |  1 +
 5 files changed, 73 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2659153cb3..91f7eaed7b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1343,6 +1343,16 @@ credential..*::
 credentialCache.ignoreSIGHUP::
Tell git-credential-cache--daemon to ignore SIGHUP, instead of quitting.
 
+completion.commands::
+   This is only used by git-completion.bash to add or remove
+   commands from the complete list. Normally only porcelain
+   commands and a few select others are in the complete list. You
+   can add more commands, separated by space, in this
+   variable. Prefixing the command with '-' will remove it from
+   the existing list.
++
+This variable should not be per-repository.
+
 include::diff-config.txt[]
 
 difftool..path::
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 908692ea52..f7ca9a4d4f 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -38,6 +38,29 @@
 #
 # When set to "1", do not include "DWIM" suggestions in git-checkout
 # completion (e.g., completing "foo" when "origin/foo" exists).
+#
+#   GIT_COMPLETION_CMD_GROUPS
+#
+# When set, "git --list-cmds=$GIT_COMPLETION_CMD_GROUPS" will be
+# used to get the list of completable commands. The default is
+# "mainporcelain,others,list-complete" (in English: all porcelain
+# commands and external ones are included. Certain non-porcelain
+# commands are also marked for completion in command-list.txt).
+# You could for example complete all commands with
+#
+# GIT_COMPLETION_CMD_GROUPS=main,others
+#
+# Or you could go with main porcelain only and extra commands in
+# the configuration variable completion.commands with
+#
+# GIT_COMPLETION_CMD_GROUPS=mainporcelain,config
+#
+# Or go completely custom group with
+#
+# GIT_COMPLETION_CMD_GROUPS=config
+#
+# Or you could even play with other command categories found in
+# command-list.txt.
 
 case "$COMP_WORDBREAKS" in
 *:*) : great ;;
@@ -840,8 +863,11 @@ __git_commands () {
if test -n "$GIT_TESTING_PORCELAIN_COMMAND_LIST"
then
printf "%s" "$GIT_TESTING_PORCELAIN_COMMAND_LIST"
+   elif test -n "$GIT_COMPLETION_CMD_GROUPS"
+   then
+   git --list-cmds="$GIT_COMPLETION_CMD_GROUPS"
else
-   git --list-cmds=list-mainporcelain,others,list-complete
+   git 
--list-cmds=list-mainporcelain,others,list-complete,config
fi
;;
all)
diff --git a/git.c b/git.c
index 4d5b8a9931..ea4feedd0b 100644
--- a/git.c
+++ b/git.c
@@ -60,6 +60,8 @@ static int list_cmds(const char *spec)
list_all_main_cmds();
else if (match_token(spec, len, "others"))
list_all_other_cmds();
+   else if (match_token(spec, len, "config"))
+   list_cmds_by_config();
else if (len > 5 && !strncmp(spec, "list-", 5)) {
struct strbuf sb = STRBUF_INIT;
 
diff --git a/help.c b/help.c
index 23924dd300..abf87205b2 100644
--- a/help.c
+++ b/help.c
@@ -366,6 +366,39 @@ void list_cmds_by_category(struct string_list *list,
}
 }
 
+void list_cmds_by_config(struct string_list *list)
+{
+   const char *cmd_list;
+
+   /*
+* There's no actual repository setup at this point (and even
+* if there is, we don't really care; only global config
+* matters). If we accidentally set up a repository, it's ok
+* too since the caller (git --list-cmds=) should exit shortly
+* anyway.
+*/
+   if (git_config_get_string_const("completion.commands", _list))
+   return;
+
+   string_list_sort(list);
+   string_list_remove_duplicates(list, 0);
+
+   while (*cmd_list) {
+   struct strbuf sb = STRBUF_INIT;
+   const char *p = strchrnul(cmd_list, ' ');
+
+   strbuf_add(, cmd_list, p - cmd_list);
+   if (*cmd_list == '-')
+   

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

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

v7 is mostly code cleanup plus one more change:

git-completion.bash now always checks completion.commands config key.
This makes it much more convenient to customize the command complete
list. You only have to fall back to setting $GIT_COMPLETION_CMD_GROUPS
when you have very special needs.

Interdiff

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2659153cb3..91f7eaed7b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1343,6 +1343,16 @@ credential..*::
 credentialCache.ignoreSIGHUP::
Tell git-credential-cache--daemon to ignore SIGHUP, instead of quitting.
 
+completion.commands::
+   This is only used by git-completion.bash to add or remove
+   commands from the complete list. Normally only porcelain
+   commands and a few select others are in the complete list. You
+   can add more commands, separated by space, in this
+   variable. Prefixing a command with '-' will remove it from
+   the existing list.
++
+This variable should not be per-repository.
+
 include::diff-config.txt[]
 
 difftool..path::
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 0fd29803d5..f7ca9a4d4f 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -50,10 +50,10 @@
 #
 # GIT_COMPLETION_CMD_GROUPS=main,others
 #
-# Or you could go with defaults add some extra commands specified
-# in the configuration variable completion.commands [1] with
+# Or you could go with main porcelain only and extra commands in
+# the configuration variable completion.commands with
 #
-# GIT_COMPLETION_CMD_GROUPS=mainporcelain,others,list-complete,config
+# GIT_COMPLETION_CMD_GROUPS=mainporcelain,config
 #
 # Or go completely custom group with
 #
@@ -61,15 +61,6 @@
 #
 # Or you could even play with other command categories found in
 # command-list.txt.
-#
-# [1] Note that completion.commands should not be per-repository
-# since the command list is generated once and cached.
-#
-# completion.commands could be used to exclude commands as
-# well.  If a command in this list begins with '-', then it
-# will be excluded from the list of commands gathered by the
-# groups specified before "config" in
-# $GIT_COMPLETION_CMD_GROUPS.
 
 case "$COMP_WORDBREAKS" in
 *:*) : great ;;
@@ -876,7 +867,7 @@ __git_commands () {
then
git --list-cmds="$GIT_COMPLETION_CMD_GROUPS"
else
-   git --list-cmds=list-mainporcelain,others,list-complete
+   git 
--list-cmds=list-mainporcelain,others,list-complete,config
fi
;;
all)
diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
index bfd8ef0671..8d6d8b45ce 100755
--- a/generate-cmdlist.sh
+++ b/generate-cmdlist.sh
@@ -9,7 +9,7 @@ command_list () {
grep -v '^#' "$1"
 }
 
-get_categories() {
+get_categories () {
tr ' ' '\n'|
grep -v '^$' |
sort |
@@ -32,7 +32,7 @@ get_synopsis () {
}' "Documentation/$1.txt"
 }
 
-define_categories() {
+define_categories () {
echo
echo "/* Command categories */"
bit=0
@@ -45,7 +45,7 @@ define_categories() {
test "$bit" -gt 32 && die "Urgh.. too many categories?"
 }
 
-define_category_names() {
+define_category_names () {
echo
echo "/* Category names */"
echo "static const char *category_names[] = {"
@@ -60,7 +60,7 @@ define_category_names() {
echo "};"
 }
 
-print_command_list() {
+print_command_list () {
echo "static struct cmdname_help command_list[] = {"
 
command_list "$1" |
diff --git a/git.c b/git.c
index fd08911e11..ea4feedd0b 100644
--- a/git.c
+++ b/git.c
@@ -38,6 +38,13 @@ static int use_pager = -1;
 
 static void list_builtins(struct string_list *list, unsigned int 
exclude_option);
 
+static int match_token(const char *spec, int len, const char *token)
+{
+   int token_len = strlen(token);
+
+   return len == token_len && !strncmp(spec, token, token_len);
+}
+
 static int list_cmds(const char *spec)
 {
struct string_list list = STRING_LIST_INIT_DUP;
@@ -47,13 +54,13 @@ static int list_cmds(const char *spec)
const char *sep = strchrnul(spec, ',');
int len = sep - spec;
 
-   if (len == 8 && !strncmp(spec, "builtins", 8))
+   if (match_token(spec, len, "builtins"))
list_builtins(, 0);
-   else if (len == 4 && !strncmp(spec, "main", 4))
+   else if (match_token(spec, len, "main"))
list_all_main_cmds();
-   else if (len == 6 && !strncmp(spec, "others", 6))
+   else if (match_token(spec, len, "others"))
list_all_other_cmds();
-   else if (len == 6 && 

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

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

This makes it easier to reuse the same code in another place (very
soon).

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

Re: [PATCH v2 0/5] getting rid of most "static struct lock_file"s

[Martin Ågren]

On 10 May 2018 at 08:01, Junio C Hamano  wrote:
> Jeff King  writes:
>
>> I don't think it's worth re-rolling, but one thing to think about for
>> future cleanups: you split the patches by touched area, not by
>> functionality. So the first three patches have a "while we're here..."
>> that has to explain why dropping the "static" is the right thing over
>> and over. If you instead did the error-handling fixes independently
>> first, then you could lump the "static" cleanups together with one
>> explanation (possibly even just as part of the 4th patch).
>
> Thanks Peff for a good pice of advice.  I agree with the assessment
> after reading the series through (includng "not worth rerolling"
> part).

Right. In the first version, the while-at-its were really while-at-its
-- and it turned out it needed some motivation. So, in the reroll, I
focused on expanding the commit messages. Any benefit from making
patches four and five somewhat smaller definitely got lost in the blown
up first three commit messages. Thanks for pointing it out.

Martin

Re: [PATCH 0/4] doc: cleaning up instances of \--

[Jeff King]

On Wed, Apr 18, 2018 at 01:24:55PM +0900, Junio C Hamano wrote:

> Martin Ågren  writes:
> 
> > This is a patch series to convert \-- to -- in our documentation. The
> > first patch is a reiteration of 1c262bb7b2 (doc: convert \--option to
> > --option, 2015-05-13) to fix some instances that have appeared since.
> > The other three patches deal with standalone "\--" which we can't
> > always turn into "--" since it can be rendered as an em dash.
> 
> All looked sensible.  As you mentioned in [2/4], "\--::" that is
> part of an enumulation appear in documentation for about a dozen
> commands after the series, but I do not think we can avoid it.
> 
> One thing that makes me wonder related to these patches is if a
> newer AsciiDoc we assume lets us do without {litdd} macro.  This
> series and our earlier effort like 1c262bb7 ("doc: convert \--option
> to --option", 2015-05-13) mentions that "\--word" is less pleasant
> on the eyes than "--word", but the ugliness "two{litdd}words" has
> over "two--words" is far worse than that, so...

I think many cases that use {litdd} would be better off using literal
backticks anyway (e.g., git-add.txt mentions the filename
`git-add--interactive.perl`).

There are certainly a few that can't, though (e.g., config.txt uses
linkgit:git-web{litdd}browse[1]).  I agree that "\--" is less ugly there
(and seems to work on my modern asciidoc). There's some history on the
litdd versus "\--" choice in 565e135a1e (Documentation: quote
double-dash for AsciiDoc, 2011-06-29). That in turn references the
2839478774 (Work around em-dash handling in newer AsciiDoc, 2010-08-23),
but I wouldn't be surprised if all of that is now obsolete with our
AsciiDoc 8+ requirement.

-Peff

PS Late review, I know, but the patches look good to me. :)

Re: [PATCH 0/1] Fix UX issue with commit-graph.lock

[Junio C Hamano]

Derrick Stolee  writes:

> We use the lockfile API to avoid multiple Git processes from writing to
> the commit-graph file in the .git/objects/info directory. In some cases,
> this directory may not exist, so we check for its existence.
>
> The existing code does the following when acquiring the lock:
>
> 1. Try to acquire the lock.
> 2. If it fails, try to create the .git/object/info directory.
> 3. Try to acquire the lock, failing if necessary.
>
> The problem is that if the lockfile exists, then the mkdir fails, giving
> an error that doesn't help the user:
>
>   "fatal: cannot mkdir .git/objects/info: File exists"

Isn't a better immediate fix to make the second step pay attention
to errno?  If mkdir() failed due to EEXIST, then we know we tried to
aquire the lock already, so we can die with an appropriate message.

That way, we can keep the "optimize for the normal case" that the
approach to assume object/info/ directory is already there, instead
of always checking its existence which is almost always true
beforehand.

Also, can't we tell why we failed to acquire the lock at step #1?
Do we only get a NULL that says "I am not telling you why, but we
failed to lock"?  What I am getting at is that the ideal sequence
would be more like:

1. Try to acquire the lock.
2-a. if #1 succeeds, we are happy. ignore the rest and return
 the lock.
2-b. if #1 failed because object/info/ did not exist,
 mkdir() it, and die if we cannot, saying "cannot mkdir".
 if mkdir() succeeds, jump t 3.
2-c. if #1 failed but that is not due to missing object/info/,
 die saying "cannot lock".
3. Try to acquire the lock.
4-a. if #3 succeeds, we are happy.ignore the rest and return
 the lock.
4-b. die saying "cannot lock".

Re: [PATCH v3 09/12] get_short_oid / peel_onion: ^{tree} should be tree, not treeish

[Jeff King]

On Thu, May 10, 2018 at 01:21:19PM +0900, Junio C Hamano wrote:

> When diagnosing such an error, we would give hints.  The hint would
> show possible objects that the user could have meant with X.  The
> ^{} suffix given to it may be used to limit the hints to
> subset of the objects that the user could have meant with X;
> e.g. when there is an object of each of type blob, tree, commit, and
> tag, whose name begins with , the short and ambiguous prefix
>  could mean any of these four objects, but when given with
> suffix, e.g. ^{tree}, it makes useless for the hint to include
> the blob object, as it can never peel down to a tree object.
> 
> If the tag whose name begins with  in this example points
> directly to a blob, excluding that tag from the hint would make the
> hint more useful.  I do not offhand know what the code does right
> now.  I wouldn't call it a bug if such a tag is included in the
> hint, but if a change stops such a tag from getting included, I
> would call such a change an improvement.

I actually wondered this while writing an earlier response, and so I
happen to know: when we are looking for a treeish, the disambiguator
will actually peel a candidate tag and only accept one that peels to a
tree or commit. So we would omit the tag-to-blob entirely from
consideration (both as a candidate for ambiguity, and in the hint list).

-Peff

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

[Junio C Hamano]

René Scharfe  writes:

> Am 10.05.2018 um 02:04 schrieb Junio C Hamano:
> ...
>>  $ git grep -v second
>>  $ git grep --not -e second
>> 
>> may hit all lines in this message (except for the obvious two
>> lines), but we cannot say which column we found a hit.  I am
>> wondering if it is too grave a sin to report "the whole line is what
>> satisfied the criteria given" and say the match lies at column #1.

And if we are planning to use this to implement '-o', then I'd
suggest that we'd say the matched part of the line is the whole
thing (i.e. so is column #1, eo is at the eol).

>> By doing so, obviously we can sidestep the whole "this mode is
>> sometimes incompatible" and "I need to compute a lot to see if the
>> given expression is compatible or not" issues.
>
> FWIW, Silver Searcher 2.1.0 does just that:
>
>   $ echo a | ag --column -v b
>   1:a
>
> ripgrep 0.8.1 as well:
>
>   $ echo a | rg --column -v b
>   1:1:a

Thanks for additional datapoints.

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

[Jeff King]

On Wed, May 09, 2018 at 07:00:10PM -0700, Taylor Blau wrote:

> >  - they test with context (-C3) for us. It looks like GNU grep omits
> >context lines with "-o", but we show a bunch of blank lines. This is
> >I guess a bug (though it seems kind of an odd combination to specify
> >in the first place)
> 
> I'm torn on what to do here. Currently, with -C3, I get a bunch of lines
> like:
> 
>   -
> 
> Which I think is technically _right_, but I agree that it is certainly
> an odd combination of things to give to 'git grep'. I think that we
> could either:
> 
>   1. Continue outputting blank lines,
>   2. Ignore '-C' with '-o', or
>   3. die() when given this combination.
> 
> I think that (1) is the most "correct" (for some definition), so I'm
> inclined to adopt that approach. I suppose that (2) is closer to what
> GNU grep offers, and that is the point of this series, so perhaps it
> makes sense to pick that instead.
> 
> I'll go with (2) for now, but I would appreciate others' thoughts before
> sending a subsequent re-roll of this series.

We talked about this off-list, so I want to summarize here for the
archive.

It turns out that the GNU grep behavior isn't universal.  Here's what I
get:

  $ grep -o -C3 the README.md
  the
  the
  the
  the
  the
  the
  the
  the
  --
  the
  the
  the
  the
  the
  the
  the
  the
  the
  the
  the
  the
  the
  the

So that's not _quite_ ignoring -C. It's still showing the "--", which
indicates that the first chunk are all matches within 6 lines of each
other (so their context is melded into a single hunk), but it omits the
lines entirely. Which at least seems like it could be _plausibly_
useful.

BSD grep seems to actually show the context lines. Which is more
information, but if you want to actually see the context, why are you
using "-o" in the first place?

So my general feeling is that disallowing the combination is probably
fine, because it's a vaguely crazy thing to ask for. But that GNU grep's
behavior is the most likely to actually be useful. The BSD behavior
seems more like "this is what we happen to produce" to me.

And it should be pretty easy to reproduce the GNU grep behavior by just
not outputting anything in show_line().

-Peff

Re: [PATCH 4/4] submodule: port submodule subcommand 'foreach' from shell to C

[Junio C Hamano]

Stefan Beller  writes:

> +static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
> +void *cb_data)
> +{
> + struct cb_foreach *info = cb_data;
> + const char *path = list_item->name;
> + const struct object_id *ce_oid = _item->oid;
> +
> + const struct submodule *sub;
> + struct child_process cp = CHILD_PROCESS_INIT;
> + char *displaypath;
> +
> + displaypath = get_submodule_displaypath(path, info->prefix);
> +
> + sub = submodule_from_path(the_repository, _oid, path);
> +
> + if (!sub)
> + die(_("No url found for submodule path '%s' in .gitmodules"),
> + displaypath);
> +
> + if (!is_submodule_populated_gently(path, NULL))
> + goto cleanup;
> +
> + prepare_submodule_repo_env(_array);
> +
> + /*
> + * For the purpose of executing  in the submodule,
> + * separate shell is used for the purpose of running the
> + * child process.
> + */

Micronit: this multi-line comment is indented in a funny way.

> + cp.use_shell = 1;
> + cp.dir = path;
> +
> + /*
> + * NEEDSWORK: the command currently has access to the variables $name,
> + * $sm_path, $displaypath, $sha1 and $toplevel only when the command
> + * contains a single argument. This is done for maintaining a faithful
> + * translation from shell script.
> + */

Same micronit.

The scripted version does 'eval "$1"', so $1 could be something like 

for-each 'echo "$name:$sm_path:$displaypath:$sha1:$toplevel"'

and it can see any variable, not just these 5 (i.e. we could have
fed e.g. $wt_prefix and $mode to the above 'echo' and with the
scripted version the script would have learned their values; with
this version it no longer does, but only these 5 are part of the
documented API, so we choose not to consider it a regression).

> + if (info->argc == 1) {
> + char *toplevel = xgetcwd();
> + struct strbuf sb = STRBUF_INIT;
> +
> + argv_array_pushf(_array, "name=%s", sub->name);
> + argv_array_pushf(_array, "sm_path=%s", path);
> + argv_array_pushf(_array, "displaypath=%s", displaypath);
> + argv_array_pushf(_array, "sha1=%s",
> + oid_to_hex(ce_oid));
> + argv_array_pushf(_array, "toplevel=%s", toplevel);
> +
> + /*
> + * Since the path variable was accessible from the script
> + * before porting, it is also made available after porting.
> + * The environment variable "PATH" has a very special purpose
> + * on windows. And since environment variables are
> + * case-insensitive in windows, it interferes with the
> + * existing PATH variable. Hence, to avoid that, we expose
> + * path via the args argv_array and not via env_array.
> + */
> + sq_quote_buf(, path);
> + argv_array_pushf(, "path=%s; %s",
> +  sb.buf, info->argv[0]);

OK, so we do the equivalent of

name=... sm_path=... displaypath=... sha1=... toplevel=... \
sh -c 'path=...; echo "$name:$sm_path:..."'

when doing

for-each 'echo "$name:$sm_path:..."'

with parts denoted with ... correctly quoted as necessary.  I guess
it would be the best we could do.

I myself do not know if it is true that bash ported to Windows won't
get confused with the above "we use path (all lowercase) only as a
pure shell variable without exporting it ourselves"; I'd trust those
who are more familiar with the platform to raise objections and
suggest a better alternative if it is not the case.  

Thanks for the (malformatted;-) leading comment to highlight why the
'path' variable alone is treated differently from the others.

> + strbuf_release();
> + free(toplevel);
> + } else {
> + argv_array_pushv(, info->argv);
> + }
> +
> + if (!info->quiet)
> + printf(_("Entering '%s'\n"), displaypath);
> +
> + if (info->argv[0] && run_command())
> + die(_("run_command returned non-zero status for %s\n."),
> + displaypath);
> +
> + if (info->recursive) {
> + struct child_process cpr = CHILD_PROCESS_INIT;
> +
> + cpr.git_cmd = 1;
> + cpr.dir = path;
> + prepare_submodule_repo_env(_array);
> +
> + argv_array_pushl(, "--super-prefix", NULL);
> + argv_array_pushf(, "%s/", displaypath);
> + argv_array_pushl(, "submodule--helper", "foreach", 
> "--recursive",
> + NULL);
> +
> + if (info->quiet)
> + argv_array_push(, "--quiet");
> +
> + argv_array_pushv(, info->argv);
> +
> + if (run_command())
> + die(_("run_command returned non-zero status while"

Re: [PATCH] repository: fix free problem with repo_clear(the_repository)

[Duy Nguyen]

On Wed, May 9, 2018 at 8:00 PM, Duy Nguyen  wrote:
> discard_index(repo->index);
> if (repo->index != _index)
> FREE_AND_NULL(repo->index);
>
>> What is your use case of repo_clear(the_repository)?
>
> No actual use case right now. See [1] for the code that triggered
> this. I do want to free some memory in pack-objects and repo_clear()
> _might_ be the one. I'm not sure yet.

Another use case for repo_clear(the_repository) is "git worktree
move". Part of the reason I did not support moving the main repository
with that command is because I wanted to shut down every access  to
$MAIN_WORK_TREE/.git before moving $MAIN_WORK_TREE. It's probably not
a problem for linux/mac platforms to move files (on the same file
system [1]) with file descriptors still open, but I'm pretty sure it
won't work on Windows. If repo_clear() does its job well, I should be
able to safely move $GIT_WORK_TREE after that.

[1] if we move across file systems then another set of problems arise:
if file descriptors remain open, writing to those will not affect the
new copies in the target. We do not support moving across filesystems
yet, but we should not shut that door now.
-- 
Duy

[PATCH v2] repository: fix free problem with repo_clear(the_repository)

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

the_repository is special. One of the special things about it is that
it does not allocate a new index_state object like submodules but
points to the global the_index variable instead. As a global variable,
the_index cannot be free()'d.

Add an exception for this in repo_clear(). In the future perhaps we
would be able to allocate the_repository's index on heap too. Then we
can revert this.

the_repository->index remains pointed to a clean the_index even after
repo_clear() so that it could still be used next time (e.g. in a crazy
use case where a dev switches repo in the same process).

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 v2 keeps the_repository->index pointed to the_index even after cleared

 repository.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/repository.c b/repository.c
index a4848c1bd0..4143073f89 100644
--- a/repository.c
+++ b/repository.c
@@ -238,7 +238,8 @@ void repo_clear(struct repository *repo)
 
if (repo->index) {
discard_index(repo->index);
-   FREE_AND_NULL(repo->index);
+   if (repo->index != _index)
+   FREE_AND_NULL(repo->index);
}
 }
 
-- 
2.17.0.705.g3525833791

Re: [PATCH v2 0/5] getting rid of most "static struct lock_file"s

[Junio C Hamano]

Jeff King  writes:

> I don't think it's worth re-rolling, but one thing to think about for
> future cleanups: you split the patches by touched area, not by
> functionality. So the first three patches have a "while we're here..."
> that has to explain why dropping the "static" is the right thing over
> and over. If you instead did the error-handling fixes independently
> first, then you could lump the "static" cleanups together with one
> explanation (possibly even just as part of the 4th patch).

Thanks Peff for a good pice of advice.  I agree with the assessment
after reading the series through (includng "not worth rerolling"
part).

Thanks, Martin.