* Re: [PATCH v8 00/11] Git filter protocol
From: Junio C Hamano @ 2016-09-29 21:27 UTC (permalink / raw)
To: Lars Schneider
Cc: Torsten Bögershausen, git, Jeff King, Stefan Beller,
Jakub Narębski, Martin-Louis Bright, ramsay
In-Reply-To: <1A8A9127-4DF9-44AD-9497-F8A630AB1193@gmail.com>
Lars Schneider <larsxschneider@gmail.com> writes:
> We discussed that issue in v4 and v6:
> http://public-inbox.org/git/20160803225313.pk3tfe5ovz4y3i7l@sigill.intra.peff.net/
> http://public-inbox.org/git/xmqqbn0a3wy3.fsf@gitster.mtv.corp.google.com/
>
> My impression was that you don't want Git to wait for the filter process.
> If Git waits for the filter process - how long should Git wait?
I am not sure where you got that impression. I did say that I do
not want Git to _KILL_ my filter process. That does not mean I want
Git to go away without waiting for me.
If the filter process refuses to die forever when Git told it to
shutdown (by closing the pipe to it, for example), that filter
process is simply buggy. I think we want users to become aware of
that, instead of Git leaving it behind, which essentially is to
sweep the problem under the rug.
I agree with what Peff said elsewhere in the thread; if a filter
process wants to take time to clean things up while letting Git
proceed, it can do its own process management, but I think it is
sensible for Git to wait the filter process it directly spawned.
^ permalink raw reply
* Re: [PATCH v2 02/11] i18n: add--interactive: mark simple here documents for translation
From: Jakub Narębski @ 2016-09-29 21:27 UTC (permalink / raw)
To: Junio C Hamano, Vasco Almeida
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
David Aguilar
In-Reply-To: <xmqqfuoihc1m.fsf@gitster.mtv.corp.google.com>
W dniu 29.09.2016 o 19:05, Junio C Hamano pisze:
> Vasco Almeida <vascomalmeida@sapo.pt> writes:
>
>> On the other hand, would it make sense to translate these commands? If
>> so, we would mark for translation the commands name of @cmd in
>> main_loop().
>>
>> sub main_loop {
>> - my @cmd = ([ 'status', \&status_cmd, ],
>> - [ 'update', \&update_cmd, ],
>> - [ 'revert', \&revert_cmd, ],
>> - [ 'add untracked', \&add_untracked_cmd, ],
>> - [ 'patch', \&patch_update_cmd, ],
>> - [ 'diff', \&diff_cmd, ],
>> - [ 'quit', \&quit_cmd, ],
>> - [ 'help', \&help_cmd, ],
>> + my @cmd = ([ __('status'), \&status_cmd, ],
>> + [ __('update'), \&update_cmd, ],
>> + [ __('revert'), \&revert_cmd, ],
>> + [ __('add untracked'), \&add_untracked_cmd, ],
>> + [ __('patch'), \&patch_update_cmd, ],
>> + [ __('diff'), \&diff_cmd, ],
>> + [ __('quit'), \&quit_cmd, ],
>> + [ __('help'), \&help_cmd, ],
>
> I don't know offhand. If the code to prompt and accept the command
> given by the user can take the translated word (or a prefix of it),
> theoretically I would say it could be made to work, but to me it is
> dubious the benefit outweighs its downsides. It would make teaching
> Git and troubleshooting over the phone harder, I would guess.
>
> A: "Hi, I am in a 'git add -i' session."
> B: "Give 's' at the prompt."
> A: "My Git does not seem to take 's' as a valid command."
> B: "What? I've never seen that problem."
> ... back and forth wastes 10 minutes ...
> A: "By the way, I am running Git in Portuguese."
Also, for one-letter commands to work (there is setting where you
don't even need to press enter, IIRC) all those translations would
have to be chosen to begin with different letter, isn't it?
Best,
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH v2 02/11] i18n: add--interactive: mark simple here documents for translation
From: Junio C Hamano @ 2016-09-29 21:31 UTC (permalink / raw)
To: Jakub Narębski
Cc: Vasco Almeida, git, Jiang Xin,
Ævar Arnfjörð Bjarmason, David Aguilar
In-Reply-To: <07371844-7fde-5b7f-b9e1-7db1a54fdbb5@gmail.com>
Jakub Narębski <jnareb@gmail.com> writes:
> W dniu 29.09.2016 o 19:05, Junio C Hamano pisze:
>> Vasco Almeida <vascomalmeida@sapo.pt> writes:
>>
>>> On the other hand, would it make sense to translate these commands? If
>>> so, we would mark for translation the commands name of @cmd in
>>> main_loop().
>>>
>>> sub main_loop {
>>> - my @cmd = ([ 'status', \&status_cmd, ],
>>> - [ 'update', \&update_cmd, ],
>>> - [ 'revert', \&revert_cmd, ],
>>> - [ 'add untracked', \&add_untracked_cmd, ],
>>> - [ 'patch', \&patch_update_cmd, ],
>>> - [ 'diff', \&diff_cmd, ],
>>> - [ 'quit', \&quit_cmd, ],
>>> - [ 'help', \&help_cmd, ],
>>> + my @cmd = ([ __('status'), \&status_cmd, ],
>>> + [ __('update'), \&update_cmd, ],
>>> + [ __('revert'), \&revert_cmd, ],
>>> + [ __('add untracked'), \&add_untracked_cmd, ],
>>> + [ __('patch'), \&patch_update_cmd, ],
>>> + [ __('diff'), \&diff_cmd, ],
>>> + [ __('quit'), \&quit_cmd, ],
>>> + [ __('help'), \&help_cmd, ],
>>
>> I don't know offhand. If the code to prompt and accept the command
>> given by the user can take the translated word (or a prefix of it),
>> theoretically I would say it could be made to work, but to me it is
>> dubious the benefit outweighs its downsides. It would make teaching
>> Git and troubleshooting over the phone harder, I would guess.
>>
>> A: "Hi, I am in a 'git add -i' session."
>> B: "Give 's' at the prompt."
>> A: "My Git does not seem to take 's' as a valid command."
>> B: "What? I've never seen that problem."
>> ... back and forth wastes 10 minutes ...
>> A: "By the way, I am running Git in Portuguese."
>
> Also, for one-letter commands to work (there is setting where you
> don't even need to press enter, IIRC) all those translations would
> have to be chosen to begin with different letter, isn't it?
The original was written with an explicit expectation that these
command words will not be translated adn chose words that do not
share the first letter exactly for that reason.
Having said that, if somebody is willing to i18n the command words,
I'd expect that the command line prompt interaction would be updated
to take the unique prefix instead of the "first byte", and if that
happens, I think the resulting system would at least be internally
consistent.
It is still dubious to me if the benefit of i18n outweighs its
downsides, though.
^ permalink raw reply
* [PATCH v6 0/4] recursive support for ls-files
From: Brandon Williams @ 2016-09-29 21:48 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, peff, gitster
In-Reply-To: <1475099443-145608-1-git-send-email-bmwill@google.com>
Minor fixes per the comments on version 5.
Brandon Williams (4):
git: make super-prefix option
ls-files: optionally recurse into submodules
ls-files: pass through safe options for --recurse-submodules
ls-files: add pathspec matching for submodules
Documentation/git-ls-files.txt | 7 +-
Documentation/git.txt | 6 +
builtin/ls-files.c | 202 ++++++++++++++++++++++++-------
cache.h | 2 +
dir.c | 46 +++++++-
dir.h | 4 +
environment.c | 10 ++
git.c | 28 ++++-
t/t3007-ls-files-recurse-submodules.sh | 209 +++++++++++++++++++++++++++++++++
9 files changed, 470 insertions(+), 44 deletions(-)
create mode 100755 t/t3007-ls-files-recurse-submodules.sh
--
2.10.0
^ permalink raw reply
* [PATCH v6 1/4] git: make super-prefix option
From: Brandon Williams @ 2016-09-29 21:48 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, peff, gitster
In-Reply-To: <1475185723-36871-1-git-send-email-bmwill@google.com>
Add a super-prefix environment variable 'GIT_INTERNAL_SUPER_PREFIX'
which can be used to specify a path from above a repository down to its
root. When such a super-prefix is specified, the paths reported by Git
are prefixed with it to make them relative to that directory "above".
The paths given by the user on the command line
(e.g. "git subcmd --output-file=path/to/a/file" and pathspecs) are taken
relative to the directory "above" to match.
The immediate use of this option is by commands which have a
--recurse-submodule option in order to give context to submodules about
how they were invoked. This option is currently only allowed for
builtins which support a super-prefix.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/git.txt | 6 ++++++
cache.h | 2 ++
environment.c | 10 ++++++++++
git.c | 26 ++++++++++++++++++++++++++
4 files changed, 44 insertions(+)
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 7913fc2..2188ae6 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -13,6 +13,7 @@ SYNOPSIS
[--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
[-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
[--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
+ [--super-prefix=<path>]
<command> [<args>]
DESCRIPTION
@@ -601,6 +602,11 @@ foo.bar= ...`) sets `foo.bar` to the empty string.
details. Equivalent to setting the `GIT_NAMESPACE` environment
variable.
+--super-prefix=<path>::
+ Currently for internal use only. Set a prefix which gives a path from
+ above a repository down to its root. One use is to give submodules
+ context about the superproject that invoked it.
+
--bare::
Treat the repository as a bare repository. If GIT_DIR
environment is not set, it is set to the current working
diff --git a/cache.h b/cache.h
index 3556326..8cf495d 100644
--- a/cache.h
+++ b/cache.h
@@ -408,6 +408,7 @@ static inline enum object_type object_type(unsigned int mode)
#define GIT_NAMESPACE_ENVIRONMENT "GIT_NAMESPACE"
#define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
#define GIT_PREFIX_ENVIRONMENT "GIT_PREFIX"
+#define GIT_SUPER_PREFIX_ENVIRONMENT "GIT_INTERNAL_SUPER_PREFIX"
#define DEFAULT_GIT_DIR_ENVIRONMENT ".git"
#define DB_ENVIRONMENT "GIT_OBJECT_DIRECTORY"
#define INDEX_ENVIRONMENT "GIT_INDEX_FILE"
@@ -468,6 +469,7 @@ extern int get_common_dir_noenv(struct strbuf *sb, const char *gitdir);
extern int get_common_dir(struct strbuf *sb, const char *gitdir);
extern const char *get_git_namespace(void);
extern const char *strip_namespace(const char *namespaced_ref);
+extern const char *get_super_prefix(void);
extern const char *get_git_work_tree(void);
/*
diff --git a/environment.c b/environment.c
index ca72464..13f3d70 100644
--- a/environment.c
+++ b/environment.c
@@ -100,6 +100,8 @@ static char *work_tree;
static const char *namespace;
static size_t namespace_len;
+static const char *super_prefix;
+
static const char *git_dir, *git_common_dir;
static char *git_object_dir, *git_index_file, *git_graft_file;
int git_db_env, git_index_env, git_graft_env, git_common_dir_env;
@@ -120,6 +122,7 @@ const char * const local_repo_env[] = {
NO_REPLACE_OBJECTS_ENVIRONMENT,
GIT_REPLACE_REF_BASE_ENVIRONMENT,
GIT_PREFIX_ENVIRONMENT,
+ GIT_SUPER_PREFIX_ENVIRONMENT,
GIT_SHALLOW_FILE_ENVIRONMENT,
GIT_COMMON_DIR_ENVIRONMENT,
NULL
@@ -222,6 +225,13 @@ const char *strip_namespace(const char *namespaced_ref)
return namespaced_ref + namespace_len;
}
+const char *get_super_prefix(void)
+{
+ if (!super_prefix)
+ super_prefix = getenv(GIT_SUPER_PREFIX_ENVIRONMENT);
+ return super_prefix;
+}
+
static int git_work_tree_initialized;
/*
diff --git a/git.c b/git.c
index 1c61151..f756b62 100644
--- a/git.c
+++ b/git.c
@@ -164,6 +164,20 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
setenv(GIT_WORK_TREE_ENVIRONMENT, cmd, 1);
if (envchanged)
*envchanged = 1;
+ } else if (!strcmp(cmd, "--super-prefix")) {
+ if (*argc < 2) {
+ fprintf(stderr, "No prefix given for --super-prefix.\n" );
+ usage(git_usage_string);
+ }
+ setenv(GIT_SUPER_PREFIX_ENVIRONMENT, (*argv)[1], 1);
+ if (envchanged)
+ *envchanged = 1;
+ (*argv)++;
+ (*argc)--;
+ } else if (skip_prefix(cmd, "--super-prefix=", &cmd)) {
+ setenv(GIT_SUPER_PREFIX_ENVIRONMENT, cmd, 1);
+ if (envchanged)
+ *envchanged = 1;
} else if (!strcmp(cmd, "--bare")) {
char *cwd = xgetcwd();
is_bare_repository_cfg = 1;
@@ -310,6 +324,7 @@ static int handle_alias(int *argcp, const char ***argv)
* RUN_SETUP for reading from the configuration file.
*/
#define NEED_WORK_TREE (1<<3)
+#define SUPPORT_SUPER_PREFIX (1<<4)
struct cmd_struct {
const char *cmd;
@@ -344,6 +359,13 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
}
commit_pager_choice();
+ if (!help && get_super_prefix()) {
+ if (!(p->option & SUPPORT_SUPER_PREFIX))
+ die("%s doesn't support --super-prefix", p->cmd);
+ if (prefix)
+ die("can't use --super-prefix from a subdirectory");
+ }
+
if (!help && p->option & NEED_WORK_TREE)
setup_work_tree();
@@ -558,6 +580,10 @@ static void execv_dashed_external(const char **argv)
const char *tmp;
int status;
+ if (get_super_prefix()) {
+ die("%s doesn't support --super-prefix", argv[0]);
+ }
+
if (use_pager == -1)
use_pager = check_pager_config(argv[0]);
commit_pager_choice();
--
2.10.0
^ permalink raw reply related
* [PATCH v6 2/4] ls-files: optionally recurse into submodules
From: Brandon Williams @ 2016-09-29 21:48 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, peff, gitster
In-Reply-To: <1475185723-36871-1-git-send-email-bmwill@google.com>
Allow ls-files to recognize submodules in order to retrieve a list of
files from a repository's submodules. This is done by forking off a
process to recursively call ls-files on all submodules. Use top-level
--super-prefix option to pass a path to the submodule which it can
use to prepend to output or pathspec matching logic.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/git-ls-files.txt | 8 +-
builtin/ls-files.c | 138 ++++++++++++++++++++++++---------
git.c | 2 +-
t/t3007-ls-files-recurse-submodules.sh | 100 ++++++++++++++++++++++++
4 files changed, 208 insertions(+), 40 deletions(-)
create mode 100755 t/t3007-ls-files-recurse-submodules.sh
diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 0d933ac..ea01d45 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -18,7 +18,8 @@ SYNOPSIS
[--exclude-per-directory=<file>]
[--exclude-standard]
[--error-unmatch] [--with-tree=<tree-ish>]
- [--full-name] [--abbrev] [--] [<file>...]
+ [--full-name] [--recurse-submodules]
+ [--abbrev] [--] [<file>...]
DESCRIPTION
-----------
@@ -137,6 +138,11 @@ a space) at the start of each line:
option forces paths to be output relative to the project
top directory.
+--recurse-submodules::
+ Recursively calls ls-files on each submodule in the repository.
+ Currently there is only support for the --cached mode without a
+ pathspec.
+
--abbrev[=<n>]::
Instead of showing the full 40-byte hexadecimal object
lines, show only a partial prefix.
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 00ea91a..63befed 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -14,6 +14,7 @@
#include "resolve-undo.h"
#include "string-list.h"
#include "pathspec.h"
+#include "run-command.h"
static int abbrev;
static int show_deleted;
@@ -28,8 +29,10 @@ static int show_valid_bit;
static int line_terminator = '\n';
static int debug_mode;
static int show_eol;
+static int recurse_submodules;
static const char *prefix;
+static const char *super_prefix;
static int max_prefix_len;
static int prefix_len;
static struct pathspec pathspec;
@@ -68,11 +71,24 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path)
static void write_name(const char *name)
{
/*
+ * Prepend the super_prefix to name to construct the full_name to be
+ * written.
+ */
+ struct strbuf full_name = STRBUF_INIT;
+ if (super_prefix) {
+ strbuf_addstr(&full_name, super_prefix);
+ strbuf_addstr(&full_name, name);
+ name = full_name.buf;
+ }
+
+ /*
* With "--full-name", prefix_len=0; this caller needs to pass
* an empty string in that case (a NULL is good for "").
*/
write_name_quoted_relative(name, prefix_len ? prefix : NULL,
stdout, line_terminator);
+
+ strbuf_release(&full_name);
}
static void show_dir_entry(const char *tag, struct dir_entry *ent)
@@ -152,55 +168,84 @@ static void show_killed_files(struct dir_struct *dir)
}
}
+/**
+ * Recursively call ls-files on a submodule
+ */
+static void show_gitlink(const struct cache_entry *ce)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+ int status;
+
+ argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
+ super_prefix ? super_prefix : "",
+ ce->name);
+ argv_array_push(&cp.args, "ls-files");
+ argv_array_push(&cp.args, "--recurse-submodules");
+
+ cp.git_cmd = 1;
+ cp.dir = ce->name;
+ status = run_command(&cp);
+ if (status)
+ exit(status);
+}
+
static void show_ce_entry(const char *tag, const struct cache_entry *ce)
{
+ struct strbuf name = STRBUF_INIT;
int len = max_prefix_len;
+ if (super_prefix)
+ strbuf_addstr(&name, super_prefix);
+ strbuf_addstr(&name, ce->name);
if (len >= ce_namelen(ce))
die("git ls-files: internal error - cache entry not superset of prefix");
- if (!match_pathspec(&pathspec, ce->name, ce_namelen(ce),
- len, ps_matched,
- S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)))
- return;
+ if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {
+ show_gitlink(ce);
+ } else if (match_pathspec(&pathspec, name.buf, name.len,
+ len, ps_matched,
+ S_ISDIR(ce->ce_mode) ||
+ S_ISGITLINK(ce->ce_mode))) {
+ if (tag && *tag && show_valid_bit &&
+ (ce->ce_flags & CE_VALID)) {
+ static char alttag[4];
+ memcpy(alttag, tag, 3);
+ if (isalpha(tag[0]))
+ alttag[0] = tolower(tag[0]);
+ else if (tag[0] == '?')
+ alttag[0] = '!';
+ else {
+ alttag[0] = 'v';
+ alttag[1] = tag[0];
+ alttag[2] = ' ';
+ alttag[3] = 0;
+ }
+ tag = alttag;
+ }
- if (tag && *tag && show_valid_bit &&
- (ce->ce_flags & CE_VALID)) {
- static char alttag[4];
- memcpy(alttag, tag, 3);
- if (isalpha(tag[0]))
- alttag[0] = tolower(tag[0]);
- else if (tag[0] == '?')
- alttag[0] = '!';
- else {
- alttag[0] = 'v';
- alttag[1] = tag[0];
- alttag[2] = ' ';
- alttag[3] = 0;
+ if (!show_stage) {
+ fputs(tag, stdout);
+ } else {
+ printf("%s%06o %s %d\t",
+ tag,
+ ce->ce_mode,
+ find_unique_abbrev(ce->sha1,abbrev),
+ ce_stage(ce));
+ }
+ write_eolinfo(ce, ce->name);
+ write_name(ce->name);
+ if (debug_mode) {
+ const struct stat_data *sd = &ce->ce_stat_data;
+
+ printf(" ctime: %d:%d\n", sd->sd_ctime.sec, sd->sd_ctime.nsec);
+ printf(" mtime: %d:%d\n", sd->sd_mtime.sec, sd->sd_mtime.nsec);
+ printf(" dev: %d\tino: %d\n", sd->sd_dev, sd->sd_ino);
+ printf(" uid: %d\tgid: %d\n", sd->sd_uid, sd->sd_gid);
+ printf(" size: %d\tflags: %x\n", sd->sd_size, ce->ce_flags);
}
- tag = alttag;
}
- if (!show_stage) {
- fputs(tag, stdout);
- } else {
- printf("%s%06o %s %d\t",
- tag,
- ce->ce_mode,
- find_unique_abbrev(ce->sha1,abbrev),
- ce_stage(ce));
- }
- write_eolinfo(ce, ce->name);
- write_name(ce->name);
- if (debug_mode) {
- const struct stat_data *sd = &ce->ce_stat_data;
-
- printf(" ctime: %d:%d\n", sd->sd_ctime.sec, sd->sd_ctime.nsec);
- printf(" mtime: %d:%d\n", sd->sd_mtime.sec, sd->sd_mtime.nsec);
- printf(" dev: %d\tino: %d\n", sd->sd_dev, sd->sd_ino);
- printf(" uid: %d\tgid: %d\n", sd->sd_uid, sd->sd_gid);
- printf(" size: %d\tflags: %x\n", sd->sd_size, ce->ce_flags);
- }
+ strbuf_release(&name);
}
static void show_ru_info(void)
@@ -468,6 +513,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
{ OPTION_SET_INT, 0, "full-name", &prefix_len, NULL,
N_("make the output relative to the project top directory"),
PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL },
+ OPT_BOOL(0, "recurse-submodules", &recurse_submodules,
+ N_("recurse through submodules")),
OPT_BOOL(0, "error-unmatch", &error_unmatch,
N_("if any <file> is not in the index, treat this as an error")),
OPT_STRING(0, "with-tree", &with_tree, N_("tree-ish"),
@@ -484,6 +531,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
prefix = cmd_prefix;
if (prefix)
prefix_len = strlen(prefix);
+ super_prefix = get_super_prefix();
git_config(git_default_config, NULL);
if (read_cache() < 0)
@@ -519,6 +567,20 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
if (require_work_tree && !is_inside_work_tree())
setup_work_tree();
+ if (recurse_submodules &&
+ (show_stage || show_deleted || show_others || show_unmerged ||
+ show_killed || show_modified || show_resolve_undo ||
+ show_valid_bit || show_tag || show_eol || with_tree ||
+ (line_terminator == '\0')))
+ die("ls-files --recurse-submodules unsupported mode");
+
+ if (recurse_submodules && error_unmatch)
+ die("ls-files --recurse-submodules does not support "
+ "--error-unmatch");
+
+ if (recurse_submodules && argc)
+ die("ls-files --recurse-submodules does not support pathspec");
+
parse_pathspec(&pathspec, 0,
PATHSPEC_PREFER_CWD |
PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
diff --git a/git.c b/git.c
index f756b62..1bbd5ae 100644
--- a/git.c
+++ b/git.c
@@ -443,7 +443,7 @@ static struct cmd_struct commands[] = {
{ "init-db", cmd_init_db },
{ "interpret-trailers", cmd_interpret_trailers, RUN_SETUP_GENTLY },
{ "log", cmd_log, RUN_SETUP },
- { "ls-files", cmd_ls_files, RUN_SETUP },
+ { "ls-files", cmd_ls_files, RUN_SETUP | SUPPORT_SUPER_PREFIX },
{ "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
{ "ls-tree", cmd_ls_tree, RUN_SETUP },
{ "mailinfo", cmd_mailinfo },
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
new file mode 100755
index 0000000..b5a53c3
--- /dev/null
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -0,0 +1,100 @@
+#!/bin/sh
+
+test_description='Test ls-files recurse-submodules feature
+
+This test verifies the recurse-submodules feature correctly lists files from
+submodules.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup directory structure and submodules' '
+ echo a >a &&
+ mkdir b &&
+ echo b >b/b &&
+ git add a b &&
+ git commit -m "add a and b" &&
+ git init submodule &&
+ echo c >submodule/c &&
+ git -C submodule add c &&
+ git -C submodule commit -m "add c" &&
+ git submodule add ./submodule &&
+ git commit -m "added submodule"
+'
+
+test_expect_success 'ls-files correctly outputs files in submodule' '
+ cat >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ submodule/c
+ EOF
+
+ git ls-files --recurse-submodules >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'ls-files does not output files not added to a repo' '
+ cat >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ submodule/c
+ EOF
+
+ echo a >not_added &&
+ echo b >b/not_added &&
+ echo c >submodule/not_added &&
+ git ls-files --recurse-submodules >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'ls-files recurses more than 1 level' '
+ cat >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ submodule/.gitmodules
+ submodule/c
+ submodule/subsub/d
+ EOF
+
+ git init submodule/subsub &&
+ echo d >submodule/subsub/d &&
+ git -C submodule/subsub add d &&
+ git -C submodule/subsub commit -m "add d" &&
+ git -C submodule submodule add ./subsub &&
+ git -C submodule commit -m "added subsub" &&
+ git ls-files --recurse-submodules >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules does not support using path arguments' '
+ test_must_fail git ls-files --recurse-submodules b 2>actual &&
+ test_i18ngrep "does not support pathspec" actual
+'
+
+test_expect_success '--recurse-submodules does not support --error-unmatch' '
+ test_must_fail git ls-files --recurse-submodules --error-unmatch 2>actual &&
+ test_i18ngrep "does not support --error-unmatch" actual
+'
+
+test_incompatible_with_recurse_submodules () {
+ test_expect_success "--recurse-submodules and $1 are incompatible" "
+ test_must_fail git ls-files --recurse-submodules $1 2>actual &&
+ test_i18ngrep 'unsupported mode' actual
+ "
+}
+
+test_incompatible_with_recurse_submodules -z
+test_incompatible_with_recurse_submodules -v
+test_incompatible_with_recurse_submodules -t
+test_incompatible_with_recurse_submodules --deleted
+test_incompatible_with_recurse_submodules --modified
+test_incompatible_with_recurse_submodules --others
+test_incompatible_with_recurse_submodules --stage
+test_incompatible_with_recurse_submodules --killed
+test_incompatible_with_recurse_submodules --unmerged
+test_incompatible_with_recurse_submodules --eol
+
+test_done
--
2.10.0
^ permalink raw reply related
* [PATCH v6 3/4] ls-files: pass through safe options for --recurse-submodules
From: Brandon Williams @ 2016-09-29 21:48 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, peff, gitster
In-Reply-To: <1475185723-36871-1-git-send-email-bmwill@google.com>
Pass through some known-safe options when recursing into submodules.
(--cached, --stage, -v, -t, -z, --debug, --eol)
Other options are compiled into an argv_array but if an unsafe option is
given the caller will be errored out.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
builtin/ls-files.c | 51 ++++++++++++++++++++++++++++++++--
t/t3007-ls-files-recurse-submodules.sh | 17 ++++++++----
2 files changed, 60 insertions(+), 8 deletions(-)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 63befed..6f744ef 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -30,6 +30,7 @@ static int line_terminator = '\n';
static int debug_mode;
static int show_eol;
static int recurse_submodules;
+static struct argv_array submodules_options = ARGV_ARRAY_INIT;
static const char *prefix;
static const char *super_prefix;
@@ -168,6 +169,45 @@ static void show_killed_files(struct dir_struct *dir)
}
}
+/*
+ * Compile an argv_array with all of the options supported by --recurse_submodules
+ */
+static void compile_submodule_options(const struct dir_struct *dir, int show_tag)
+{
+ if (line_terminator == '\0')
+ argv_array_push(&submodules_options, "-z");
+ if (show_tag)
+ argv_array_push(&submodules_options, "-t");
+ if (show_valid_bit)
+ argv_array_push(&submodules_options, "-v");
+ if (show_cached)
+ argv_array_push(&submodules_options, "--cached");
+ if (show_deleted)
+ argv_array_push(&submodules_options, "--deleted");
+ if (show_modified)
+ argv_array_push(&submodules_options, "--modified");
+ if (show_others)
+ argv_array_push(&submodules_options, "--others");
+ if (dir->flags & DIR_SHOW_IGNORED)
+ argv_array_push(&submodules_options, "--ignored");
+ if (show_stage)
+ argv_array_push(&submodules_options, "--stage");
+ if (show_killed)
+ argv_array_push(&submodules_options, "--killed");
+ if (dir->flags & DIR_SHOW_OTHER_DIRECTORIES)
+ argv_array_push(&submodules_options, "--directory");
+ if (!(dir->flags & DIR_SHOW_OTHER_DIRECTORIES))
+ argv_array_push(&submodules_options, "--empty-directory");
+ if (show_unmerged)
+ argv_array_push(&submodules_options, "--unmerged");
+ if (show_resolve_undo)
+ argv_array_push(&submodules_options, "--resolve-undo");
+ if (show_eol)
+ argv_array_push(&submodules_options, "--eol");
+ if (debug_mode)
+ argv_array_push(&submodules_options, "--debug");
+}
+
/**
* Recursively call ls-files on a submodule
*/
@@ -182,6 +222,9 @@ static void show_gitlink(const struct cache_entry *ce)
argv_array_push(&cp.args, "ls-files");
argv_array_push(&cp.args, "--recurse-submodules");
+ /* add supported options */
+ argv_array_pushv(&cp.args, submodules_options.argv);
+
cp.git_cmd = 1;
cp.dir = ce->name;
status = run_command(&cp);
@@ -567,11 +610,13 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
if (require_work_tree && !is_inside_work_tree())
setup_work_tree();
+ if (recurse_submodules)
+ compile_submodule_options(&dir, show_tag);
+
if (recurse_submodules &&
- (show_stage || show_deleted || show_others || show_unmerged ||
+ (show_deleted || show_others || show_unmerged ||
show_killed || show_modified || show_resolve_undo ||
- show_valid_bit || show_tag || show_eol || with_tree ||
- (line_terminator == '\0')))
+ with_tree))
die("ls-files --recurse-submodules unsupported mode");
if (recurse_submodules && error_unmatch)
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index b5a53c3..e76fa30 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -34,6 +34,18 @@ test_expect_success 'ls-files correctly outputs files in submodule' '
test_cmp expect actual
'
+test_expect_success 'ls-files correctly outputs files in submodule with -z' '
+ lf_to_nul >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ submodule/c
+ EOF
+
+ git ls-files --recurse-submodules -z >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'ls-files does not output files not added to a repo' '
cat >expect <<-\EOF &&
.gitmodules
@@ -86,15 +98,10 @@ test_incompatible_with_recurse_submodules () {
"
}
-test_incompatible_with_recurse_submodules -z
-test_incompatible_with_recurse_submodules -v
-test_incompatible_with_recurse_submodules -t
test_incompatible_with_recurse_submodules --deleted
test_incompatible_with_recurse_submodules --modified
test_incompatible_with_recurse_submodules --others
-test_incompatible_with_recurse_submodules --stage
test_incompatible_with_recurse_submodules --killed
test_incompatible_with_recurse_submodules --unmerged
-test_incompatible_with_recurse_submodules --eol
test_done
--
2.10.0
^ permalink raw reply related
* [PATCH v6 4/4] ls-files: add pathspec matching for submodules
From: Brandon Williams @ 2016-09-29 21:48 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, peff, gitster
In-Reply-To: <1475185723-36871-1-git-send-email-bmwill@google.com>
Pathspecs can be a bit tricky when trying to apply them to submodules.
The main challenge is that the pathspecs will be with respect to the
superproject and not with respect to paths in the submodule. The
approach this patch takes is to pass in the identical pathspec from the
superproject to the submodule in addition to the submodule-prefix, which
is the path from the root of the superproject to the submodule, and then
we can compare an entry in the submodule prepended with the
submodule-prefix to the pathspec in order to determine if there is a
match.
This patch also permits the pathspec logic to perform a prefix match against
submodules since a pathspec could refer to a file inside of a submodule.
Due to limitations in the wildmatch logic, a prefix match is only done
literally. If any wildcard character is encountered we'll simply punt
and produce a false positive match. More accurate matching will be done
once inside the submodule. This is due to the superproject not knowing
what files could exist in the submodule.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/git-ls-files.txt | 3 +-
builtin/ls-files.c | 27 +++++++--
dir.c | 46 +++++++++++++-
dir.h | 4 ++
t/t3007-ls-files-recurse-submodules.sh | 108 ++++++++++++++++++++++++++++++++-
5 files changed, 175 insertions(+), 13 deletions(-)
diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index ea01d45..51ec9a1 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -140,8 +140,7 @@ a space) at the start of each line:
--recurse-submodules::
Recursively calls ls-files on each submodule in the repository.
- Currently there is only support for the --cached mode without a
- pathspec.
+ Currently there is only support for the --cached.
--abbrev[=<n>]::
Instead of showing the full 40-byte hexadecimal object
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 6f744ef..82ec811 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -215,6 +215,7 @@ static void show_gitlink(const struct cache_entry *ce)
{
struct child_process cp = CHILD_PROCESS_INIT;
int status;
+ int i;
argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
super_prefix ? super_prefix : "",
@@ -225,6 +226,15 @@ static void show_gitlink(const struct cache_entry *ce)
/* add supported options */
argv_array_pushv(&cp.args, submodules_options.argv);
+ /*
+ * Pass in the original pathspec args. The submodule will be
+ * responsible for prepending the 'submodule_prefix' prior to comparing
+ * against the pathspec for matches.
+ */
+ argv_array_push(&cp.args, "--");
+ for (i = 0; i < pathspec.nr; i++)
+ argv_array_push(&cp.args, pathspec.items[i].original);
+
cp.git_cmd = 1;
cp.dir = ce->name;
status = run_command(&cp);
@@ -243,7 +253,8 @@ static void show_ce_entry(const char *tag, const struct cache_entry *ce)
if (len >= ce_namelen(ce))
die("git ls-files: internal error - cache entry not superset of prefix");
- if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {
+ if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&
+ submodule_path_match(&pathspec, name.buf, ps_matched)) {
show_gitlink(ce);
} else if (match_pathspec(&pathspec, name.buf, name.len,
len, ps_matched,
@@ -623,16 +634,20 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
die("ls-files --recurse-submodules does not support "
"--error-unmatch");
- if (recurse_submodules && argc)
- die("ls-files --recurse-submodules does not support pathspec");
-
parse_pathspec(&pathspec, 0,
PATHSPEC_PREFER_CWD |
PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
prefix, argv);
- /* Find common prefix for all pathspec's */
- max_prefix = common_prefix(&pathspec);
+ /*
+ * Find common prefix for all pathspec's
+ * This is used as a performance optimization which unfortunately cannot
+ * be done when recursing into submodules
+ */
+ if (recurse_submodules)
+ max_prefix = NULL;
+ else
+ max_prefix = common_prefix(&pathspec);
max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
/* Treat unmatching pathspec elements as errors */
diff --git a/dir.c b/dir.c
index 0ea235f..28e9736 100644
--- a/dir.c
+++ b/dir.c
@@ -207,8 +207,9 @@ int within_depth(const char *name, int namelen,
return 1;
}
-#define DO_MATCH_EXCLUDE 1
-#define DO_MATCH_DIRECTORY 2
+#define DO_MATCH_EXCLUDE (1<<0)
+#define DO_MATCH_DIRECTORY (1<<1)
+#define DO_MATCH_SUBMODULE (1<<2)
/*
* Does 'match' match the given name?
@@ -283,6 +284,32 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,
item->nowildcard_len - prefix))
return MATCHED_FNMATCH;
+ /* Perform checks to see if "name" is a super set of the pathspec */
+ if (flags & DO_MATCH_SUBMODULE) {
+ /* name is a literal prefix of the pathspec */
+ if ((namelen < matchlen) &&
+ (match[namelen] == '/') &&
+ !ps_strncmp(item, match, name, namelen))
+ return MATCHED_RECURSIVELY;
+
+ /* name" doesn't match up to the first wild character */
+ if (item->nowildcard_len < item->len &&
+ ps_strncmp(item, match, name,
+ item->nowildcard_len - prefix))
+ return 0;
+
+ /*
+ * Here is where we would perform a wildmatch to check if
+ * "name" can be matched as a directory (or a prefix) against
+ * the pathspec. Since wildmatch doesn't have this capability
+ * at the present we have to punt and say that it is a match,
+ * potentially returning a false positive
+ * The submodules themselves will be able to perform more
+ * accurate matching to determine if the pathspec matches.
+ */
+ return MATCHED_RECURSIVELY;
+ }
+
return 0;
}
@@ -386,6 +413,21 @@ int match_pathspec(const struct pathspec *ps,
return negative ? 0 : positive;
}
+/**
+ * Check if a submodule is a superset of the pathspec
+ */
+int submodule_path_match(const struct pathspec *ps,
+ const char *submodule_name,
+ char *seen)
+{
+ int matched = do_match_pathspec(ps, submodule_name,
+ strlen(submodule_name),
+ 0, seen,
+ DO_MATCH_DIRECTORY |
+ DO_MATCH_SUBMODULE);
+ return matched;
+}
+
int report_path_error(const char *ps_matched,
const struct pathspec *pathspec,
const char *prefix)
diff --git a/dir.h b/dir.h
index da1a858..97c83bb 100644
--- a/dir.h
+++ b/dir.h
@@ -304,6 +304,10 @@ extern int git_fnmatch(const struct pathspec_item *item,
const char *pattern, const char *string,
int prefix);
+extern int submodule_path_match(const struct pathspec *ps,
+ const char *submodule_name,
+ char *seen);
+
static inline int ce_path_match(const struct cache_entry *ce,
const struct pathspec *pathspec,
char *seen)
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index e76fa30..5475855 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -81,9 +81,111 @@ test_expect_success 'ls-files recurses more than 1 level' '
test_cmp expect actual
'
-test_expect_success '--recurse-submodules does not support using path arguments' '
- test_must_fail git ls-files --recurse-submodules b 2>actual &&
- test_i18ngrep "does not support pathspec" actual
+test_expect_success '--recurse-submodules and pathspecs setup' '
+ echo e >submodule/subsub/e.txt &&
+ git -C submodule/subsub add e.txt &&
+ git -C submodule/subsub commit -m "adding e.txt" &&
+ echo f >submodule/f.TXT &&
+ echo g >submodule/g.txt &&
+ git -C submodule add f.TXT g.txt &&
+ git -C submodule commit -m "add f and g" &&
+ echo h >h.txt &&
+ mkdir sib &&
+ echo sib >sib/file &&
+ git add h.txt sib/file &&
+ git commit -m "add h and sib/file" &&
+ git init sub &&
+ echo sub >sub/file &&
+ git -C sub add file &&
+ git -C sub commit -m "add file" &&
+ git submodule add ./sub &&
+ git commit -m "added sub" &&
+
+ cat >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ h.txt
+ sib/file
+ sub/file
+ submodule/.gitmodules
+ submodule/c
+ submodule/f.TXT
+ submodule/g.txt
+ submodule/subsub/d
+ submodule/subsub/e.txt
+ EOF
+
+ git ls-files --recurse-submodules >actual &&
+ test_cmp expect actual &&
+ cat actual &&
+ git ls-files --recurse-submodules "*" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+ cat >expect <<-\EOF &&
+ h.txt
+ submodule/g.txt
+ submodule/subsub/e.txt
+ EOF
+
+ git ls-files --recurse-submodules "*.txt" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+ cat >expect <<-\EOF &&
+ h.txt
+ submodule/f.TXT
+ submodule/g.txt
+ submodule/subsub/e.txt
+ EOF
+
+ git ls-files --recurse-submodules ":(icase)*.txt" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+ cat >expect <<-\EOF &&
+ h.txt
+ submodule/f.TXT
+ submodule/g.txt
+ EOF
+
+ git ls-files --recurse-submodules ":(icase)*.txt" ":(exclude)submodule/subsub/*" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+ cat >expect <<-\EOF &&
+ sub/file
+ EOF
+
+ git ls-files --recurse-submodules "sub" >actual &&
+ test_cmp expect actual &&
+ git ls-files --recurse-submodules "sub/" >actual &&
+ test_cmp expect actual &&
+ git ls-files --recurse-submodules "sub/file" >actual &&
+ test_cmp expect actual &&
+ git ls-files --recurse-submodules "su*/file" >actual &&
+ test_cmp expect actual &&
+ git ls-files --recurse-submodules "su?/file" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+ cat >expect <<-\EOF &&
+ sib/file
+ sub/file
+ EOF
+
+ git ls-files --recurse-submodules "s??/file" >actual &&
+ test_cmp expect actual &&
+ git ls-files --recurse-submodules "s???file" >actual &&
+ test_cmp expect actual &&
+ git ls-files --recurse-submodules "s*file" >actual &&
+ test_cmp expect actual
'
test_expect_success '--recurse-submodules does not support --error-unmatch' '
--
2.10.0
^ permalink raw reply related
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Linus Torvalds @ 2016-09-29 21:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Johannes Sixt, Git Mailing List
In-Reply-To: <xmqqwphuebhd.fsf@gitster.mtv.corp.google.com>
[-- Attachment #1: Type: text/plain, Size: 623 bytes --]
On Thu, Sep 29, 2016 at 12:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> I think that is a reasonable way to go.
>
> #define DEFAULT_ABBREV get_default_abbrev()
>
> would help.
So something like this that replaces the previous patch?
Somebody should really double-check my heuristics, to see that I did
the pack counting etc right. It doesn't do alternate loose file
counting at all, and maybe it could matter. The advantage of the
previous patch was that it got the object counting right almost
automatically, this actually has its own new object counting code and
maybe I screwed it up.
Linus
[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 2090 bytes --]
cache.h | 3 ++-
environment.c | 2 +-
sha1_file.c | 43 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 46 insertions(+), 2 deletions(-)
diff --git a/cache.h b/cache.h
index 6e33f2f28..a022e1bd2 100644
--- a/cache.h
+++ b/cache.h
@@ -1186,8 +1186,9 @@ static inline int hex2chr(const char *s)
}
/* Convert to/from hex/sha1 representation */
+extern int get_default_abbrev(void);
#define MINIMUM_ABBREV minimum_abbrev
-#define DEFAULT_ABBREV default_abbrev
+#define DEFAULT_ABBREV get_default_abbrev()
struct object_context {
unsigned char tree[20];
diff --git a/environment.c b/environment.c
index c1442df9a..fd6681e46 100644
--- a/environment.c
+++ b/environment.c
@@ -16,7 +16,7 @@ int trust_executable_bit = 1;
int trust_ctime = 1;
int check_stat = 1;
int has_symlinks = 1;
-int minimum_abbrev = 4, default_abbrev = 7;
+int minimum_abbrev = 4, default_abbrev = -1;
int ignore_case;
int assume_unchanged;
int prefer_symlink_refs;
diff --git a/sha1_file.c b/sha1_file.c
index ca149a607..28ba04b65 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3720,3 +3720,46 @@ int for_each_packed_object(each_packed_object_fn cb, void *data, unsigned flags)
}
return r ? r : pack_errors;
}
+
+static int init_default_abbrev(void)
+{
+ unsigned long count = 0;
+ struct packed_git *p;
+ struct strbuf buf = STRBUF_INIT;
+ DIR *dir;
+ char *name;
+ int ret;
+
+ prepare_packed_git();
+ for (p = packed_git; p; p = p->next) {
+ if (open_pack_index(p))
+ continue;
+ count += p->num_objects;
+ }
+
+ strbuf_addstr(&buf, get_object_directory());
+ strbuf_addstr(&buf, "/42/");
+ name = strbuf_detach(&buf, NULL);
+ dir = opendir(name);
+ free(name);
+ if (dir) {
+ struct dirent *de;
+ while ((de = readdir(dir)) != NULL) {
+ count += 256;
+ }
+ closedir(dir);
+ }
+ for (ret = 7; ret < 15; ret++) {
+ unsigned long expect_collision = 1ul << (ret * 2);
+ if (count < expect_collision)
+ break;
+ }
+ return ret;
+}
+
+int get_default_abbrev(void)
+{
+ if (default_abbrev < 0)
+ default_abbrev = init_default_abbrev();
+ return default_abbrev;
+}
^ permalink raw reply related
* Re: [RFC/PATCH 0/2] place cherry pick line below commit title
From: Junio C Hamano @ 2016-09-29 21:56 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, Christian Couder
In-Reply-To: <cover.1475176070.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> This is somewhat of a follow-up to my previous e-mail with subject
> "[PATCH] sequencer: support folding in rfc2822 footer" [1], in which I
> proposed relaxing the definition of a commit message footer to allow
> multiple-line field bodies (as described in RFC2822), but its strictness
> was deemed deliberate.
It does not necessarily mean we can never change it when we did
something deliberately, though. With a good enough justification,
and with a transitition plan if the backward incompatibility is
severe enough to warrant one, we can change things.
I vaguely recall that there were some discussion on the definition
of "what's a trailer line" with folks from the kernel land, perhaps
while discussing the interpret-trailers topic. IIRC, when somebody
passes an improved version along, the resulting message's trailer
block may look like this:
Signed-off-by: Original Author <original@author.xz>
[fixed typo in the variable names]
Signed-off-by: Somebhody Else <somebody@else.xz>
and an obvious "wish" of theirs was to treat not just RFC2822-like
"a line that begins with token followed by a colon" but also these
short comments as part of the trailer block. Your original wish in
[*1*] is to also treat "a line that begin with a whitespace that
follows a line that begins with token followed by a colon" as part
of the trailer block and I personally think that is a reasonable
thing to wish for, too.
I recall that I was somewhat surprised and dissapointed to see no
change to interpret-trailers when you tried [*1*], which was really
about improving the definition of what the trailer block is, by the
way.
In any case, if we want to improve what the trailer block is, we
would certainly need to make sure what is inserted by "cherry-pick -x"
is also considered as part of the trailer block, so it may be necessary
to change it to "Cherry-picked-from: ..." while doing so. I dunno.
> Below is a patch set that allows placing the "cherry picked from" line
> without taking into account the definition of a commit message footer.
> For example, "git cherry-pick -x" (with the appropriate configuration
> variable or argument) would, to this commit message:
>
> commit title
>
> This is an explanatory paragraph.
>
> Footer: foo
>
> place the "(cherry picked from ...)" line below "commit title".
>
> Would this be better?
It is not immediately obvious what such a change buys us. Wouldn't
the current code place that line below "Footer: foo"? I cannot
think of any reason why anybody would want to place "cherry-picked
from" immediately below the title and before the first line of the
body.
[Footnotes]
*1* http://public-inbox.org/git/1472846322-5592-1-git-send-email-jonathantanmy@google.com/
^ permalink raw reply
* Re: [PATCH 1/3] add QSORT
From: Junio C Hamano @ 2016-09-29 22:36 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List
In-Reply-To: <67bddc37-4ee2-fef0-c852-e32645421e4c@web.de>
René Scharfe <l.s.r@web.de> writes:
> Add the macro QSORT, a convenient wrapper for qsort(3) that infers the
> size of the array elements and supports the convention of initializing
> empty arrays with a NULL pointer, which we use in some places.
>
> Calling qsort(3) directly with a NULL pointer is undefined -- even with
> an element count of zero -- and allows the compiler to optimize away any
> following NULL checks. Using the macro avoids such surprises.
>
> Add a semantic patch as well to demonstrate the macro's usage and to
> automate the transformation of trivial cases.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> contrib/coccinelle/qsort.cocci | 19 +++++++++++++++++++
> git-compat-util.h | 8 ++++++++
> 2 files changed, 27 insertions(+)
> create mode 100644 contrib/coccinelle/qsort.cocci
The direct calls to qsort(3) that this series leaves behind are
interesting.
1. builtin/index-pack.c has this:
if (1 < opts->anomaly_nr)
qsort(opts->anomaly, opts->anomaly_nr, sizeof(uint32_t), cmp_uint32);
where opts->anomaly is coming from pack.h:
struct pack_idx_option {
unsigned flags;
...
int anomaly_alloc, anomaly_nr;
uint32_t *anomaly;
};
I cannot quite see how the automated conversion misses it? It's not
like base and nmemb are type-restricted in the rule (they are both
just "expression"s).
2. builtin/shortlog.c has this:
qsort(log->list.items, log->list.nr, sizeof(struct string_list_item),
log->summary ? compare_by_counter : compare_by_list);
where log->list is coming from shortlog.h:
struct shortlog {
struct string_list list;
};
and string-list.h says:
struct string_list {
struct string_list_item *items;
unsigned int nr, alloc;
...
};
which seems to be a good candidate for this rule:
type T;
T *base;
expression nmemb, compar;
@@
- qsort(base, nmemb, sizeof(T), compar);
+ QSORT(base, nmemb, compar);
if we take "T == struct string_list_item".
3. builtin/show-branch.c does this:
qsort(ref_name + bottom, top - bottom, sizeof(ref_name[0]),
compare_ref_name);
where ref_name[] is a file-scope global:
static char *ref_name[MAX_REVS + 1];
and top and bottom are plain integers. The sizeof() does not take
the size of *base, so it is understandable that this does not get
automatically converted.
It seems that some calls to this function _could_ send the same top
and bottom, asking for 0 element array to be sorted, by the way.
Thanks for an amusing read.
^ permalink raw reply
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Junio C Hamano @ 2016-09-29 23:13 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Jeff King, Johannes Sixt, Git Mailing List
In-Reply-To: <CA+55aFyVEQ+8TBBUm5KG9APtd9wy8cp_mRO=3nj12DXZNLAC9A@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> Somebody should really double-check my heuristics, to see that I did
> the pack counting etc right. It doesn't do alternate loose file
> counting at all, and maybe it could matter. The advantage of the
> previous patch was that it got the object counting right almost
> automatically, this actually has its own new object counting code and
> maybe I screwed it up.
One thing that worries me is if we are ready to start accessing the
object store in all codepaths when we ask for DEFAULT_ABBREV. The
worries are twofold:
(1) Do we do the right thing if object store is not available to
us? Some commands can be run outside repository, and if our
call to prepare_packed_git() or loose object iteration barfed
in some way, that would introduce a regression.
(2) Is calling prepare_packed_git() too early interfere with how
the commands expect its own prepare_packed_git() work? That
is, if a command has this sequence, "ask DEFAULT_ABBREV,
arrange things, and then call prepare_packed_git()", and the
existing "arrange things" step had something that causes a new
pack to become eligible to be read by prepare_packed_git(),
like adding to the list of alternate object stores, its own
prepare_packed_git() will now become a no-op.
I browsed through "tig grep DEFAULT_ABBREV \*.c" and it seems that
in majority of the hits, we not just are ready to start accessing,
but already have an object or two, which must have come from an
already open object store, so they are OK. Especially the ones that
use it as the last argument to find_unique_abbrev() are OK as we are
about to open the object store to do the computation.
There are very early ones in the program startup sequence in the
following functions, but I do not think of a reason why our new and
early call to prepare_packed_git() might be problematic, given that
all of them require us to have an access to the repository (i.e.
this change cannot introduce a regression where a command used to
work outside a repository but barf when prepare_packed_git() is
called early):
- builtin/describe.c
- builtin/rev-list.c
- builtin/rev-parse.c
I thought that the one in diff.c might be problematic when the "git
diff" command is run outside a repository with the "--no-index"
option, but it appears that init_default_abbrev() seems to be OK
when run outside a repository.
There is one in parse-options-cb.c that is used to parse the --abbrev
command line option. This might cause a cosmetic problem but when
the user is asking for an abbreviation, it is expected that we will
have an access to the object store anyway, so it may be OK.
I am sorry that none of the above is about your math ;-) I suck at
math so I won't comment.
^ permalink raw reply
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Junio C Hamano @ 2016-09-29 23:20 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Jeff King, Johannes Sixt, Git Mailing List
In-Reply-To: <xmqqbmz6cna5.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>> The advantage of the
>> previous patch was that it got the object counting right almost
>> automatically, this actually has its own new object counting code and
>> maybe I screwed it up.
I guess another advantage of your original approach was that it
delayed the counting to the very last minute, so the things that
worried me in my previous response were automatically made
non-issues.
^ permalink raw reply
* Re: [PATCH 1/3] add QSORT
From: René Scharfe @ 2016-09-29 23:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <xmqqponmcp07.fsf@gitster.mtv.corp.google.com>
Am 30.09.2016 um 00:36 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> Add the macro QSORT, a convenient wrapper for qsort(3) that infers the
>> size of the array elements and supports the convention of initializing
>> empty arrays with a NULL pointer, which we use in some places.
>>
>> Calling qsort(3) directly with a NULL pointer is undefined -- even with
>> an element count of zero -- and allows the compiler to optimize away any
>> following NULL checks. Using the macro avoids such surprises.
>>
>> Add a semantic patch as well to demonstrate the macro's usage and to
>> automate the transformation of trivial cases.
>>
>> Signed-off-by: Rene Scharfe <l.s.r@web.de>
>> ---
>> contrib/coccinelle/qsort.cocci | 19 +++++++++++++++++++
>> git-compat-util.h | 8 ++++++++
>> 2 files changed, 27 insertions(+)
>> create mode 100644 contrib/coccinelle/qsort.cocci
>
> The direct calls to qsort(3) that this series leaves behind are
> interesting.
>
> 1. builtin/index-pack.c has this:
>
> if (1 < opts->anomaly_nr)
> qsort(opts->anomaly, opts->anomaly_nr, sizeof(uint32_t), cmp_uint32);
>
> where opts->anomaly is coming from pack.h:
>
> struct pack_idx_option {
> unsigned flags;
> ...
> int anomaly_alloc, anomaly_nr;
> uint32_t *anomaly;
> };
>
> I cannot quite see how the automated conversion misses it? It's not
> like base and nmemb are type-restricted in the rule (they are both
> just "expression"s).
>
> 2. builtin/shortlog.c has this:
>
> qsort(log->list.items, log->list.nr, sizeof(struct string_list_item),
> log->summary ? compare_by_counter : compare_by_list);
>
> where log->list is coming from shortlog.h:
>
> struct shortlog {
> struct string_list list;
> };
>
> and string-list.h says:
>
> struct string_list {
> struct string_list_item *items;
> unsigned int nr, alloc;
> ...
> };
>
> which seems to be a good candidate for this rule:
>
> type T;
> T *base;
> expression nmemb, compar;
> @@
> - qsort(base, nmemb, sizeof(T), compar);
> + QSORT(base, nmemb, compar);
>
> if we take "T == struct string_list_item".
Transformations for these two are generated if we pass --all-includes
to spatch. So let's do that.
-- >8 --
Subject: [PATCH] coccicheck: use --all-includes by default
Add a make variable, SPATCH_FLAGS, for specifying flags for spatch, and
set it to --all-includes by default. This option lets it consider
header files which would otherwise be ignored. That's important for
some rules that rely on type information. It doubles the duration of
coccicheck, however.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
Makefile | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index 1aad150..d15bf8d 100644
--- a/Makefile
+++ b/Makefile
@@ -467,6 +467,7 @@ SPATCH = spatch
export TCL_PATH TCLTK_PATH
SPARSE_FLAGS =
+SPATCH_FLAGS = --all-includes
@@ -2314,7 +2315,7 @@ C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ))
%.cocci.patch: %.cocci $(C_SOURCES)
@echo ' ' SPATCH $<; \
for f in $(C_SOURCES); do \
- $(SPATCH) --sp-file $< $$f; \
+ $(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS); \
done >$@ 2>$@.log; \
if test -s $@; \
then \
--
2.10.0
^ permalink raw reply related
* Re: Impossible to change working directory
From: Junio C Hamano @ 2016-09-29 23:24 UTC (permalink / raw)
To: Sebastian Feldmann; +Cc: git
In-Reply-To: <A99D7D06-8214-4C09-BCB2-E4FCE75FF317@sebastian-feldmann.info>
Sebastian Feldmann <sf@sebastian-feldmann.info> writes:
> the script fails because changing the current working directory fails.
> If I echo the current working directory it always echoes the root repository path
>
> Is this expected behavior?
Yes, we always go to the top before doing anything.
If you echo environment variables Git may set up for you, what do
you see? Do you see GIT_PREFIX that you can use to tell where you
came from, or something like that?
^ permalink raw reply
* Re: [PATCH 1/3] add QSORT
From: René Scharfe @ 2016-09-29 23:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <eeb2791e-c09b-b7bc-f8e8-336d4f3906b8@web.de>
Am 30.09.2016 um 01:21 schrieb René Scharfe:
> Am 30.09.2016 um 00:36 schrieb Junio C Hamano:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>> Add the macro QSORT, a convenient wrapper for qsort(3) that infers the
>>> size of the array elements and supports the convention of initializing
>>> empty arrays with a NULL pointer, which we use in some places.
>>>
>>> Calling qsort(3) directly with a NULL pointer is undefined -- even with
>>> an element count of zero -- and allows the compiler to optimize away any
>>> following NULL checks. Using the macro avoids such surprises.
>>>
>>> Add a semantic patch as well to demonstrate the macro's usage and to
>>> automate the transformation of trivial cases.
>>>
>>> Signed-off-by: Rene Scharfe <l.s.r@web.de>
>>> ---
>>> contrib/coccinelle/qsort.cocci | 19 +++++++++++++++++++
>>> git-compat-util.h | 8 ++++++++
>>> 2 files changed, 27 insertions(+)
>>> create mode 100644 contrib/coccinelle/qsort.cocci
>>
>> The direct calls to qsort(3) that this series leaves behind are
>> interesting.
>>
>> 1. builtin/index-pack.c has this:
>>
>> if (1 < opts->anomaly_nr)
>> qsort(opts->anomaly, opts->anomaly_nr, sizeof(uint32_t), cmp_uint32);
>>
>> where opts->anomaly is coming from pack.h:
>>
>> struct pack_idx_option {
>> unsigned flags;
>> ...
>> int anomaly_alloc, anomaly_nr;
>> uint32_t *anomaly;
>> };
>>
>> I cannot quite see how the automated conversion misses it? It's not
>> like base and nmemb are type-restricted in the rule (they are both
>> just "expression"s).
>>
>> 2. builtin/shortlog.c has this:
>>
>> qsort(log->list.items, log->list.nr, sizeof(struct string_list_item),
>> log->summary ? compare_by_counter : compare_by_list);
>>
>> where log->list is coming from shortlog.h:
>>
>> struct shortlog {
>> struct string_list list;
>> };
>>
>> and string-list.h says:
>>
>> struct string_list {
>> struct string_list_item *items;
>> unsigned int nr, alloc;
>> ...
>> };
>>
>> which seems to be a good candidate for this rule:
>>
>> type T;
>> T *base;
>> expression nmemb, compar;
>> @@
>> - qsort(base, nmemb, sizeof(T), compar);
>> + QSORT(base, nmemb, compar);
>>
>> if we take "T == struct string_list_item".
>
> Transformations for these two are generated if we pass --all-includes
> to spatch. So let's do that.
And here's the result:
-- >8 --
Subject: [PATCH] use QSORT, part 2
Convert two more qsort(3) calls to QSORT to reduce code size and for
better safety and consistency.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
Squashable.
builtin/index-pack.c | 3 +--
builtin/shortlog.c | 2 +-
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 7657d0a..0a27bab 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1531,8 +1531,7 @@ static void read_v2_anomalous_offsets(struct packed_git *p,
opts->anomaly[opts->anomaly_nr++] = ntohl(idx2[off * 2 + 1]);
}
- if (1 < opts->anomaly_nr)
- qsort(opts->anomaly, opts->anomaly_nr, sizeof(uint32_t), cmp_uint32);
+ QSORT(opts->anomaly, opts->anomaly_nr, cmp_uint32);
}
static void read_idx_option(struct pack_idx_option *opts, const char *pack_name)
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 25fa8a6..ba0e115 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -308,7 +308,7 @@ void shortlog_output(struct shortlog *log)
struct strbuf sb = STRBUF_INIT;
if (log->sort_by_number)
- qsort(log->list.items, log->list.nr, sizeof(struct string_list_item),
+ QSORT(log->list.items, log->list.nr,
log->summary ? compare_by_counter : compare_by_list);
for (i = 0; i < log->list.nr; i++) {
const struct string_list_item *item = &log->list.items[i];
--
2.10.0
^ permalink raw reply related
* Re: [PATCH v2] http: Control GSSAPI credential delegation.
From: brian m. carlson @ 2016-09-29 23:53 UTC (permalink / raw)
To: Petr Stodulka; +Cc: git
In-Reply-To: <1475085694-10056-1-git-send-email-pstodulk@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 1100 bytes --]
On Wed, Sep 28, 2016 at 08:01:34PM +0200, Petr Stodulka wrote:
> Delegation of credentials is disabled by default in libcurl since
> version 7.21.7 due to security vulnerability CVE-2011-2192. Which
> makes troubles with GSS/kerberos authentication when delegation
> of credentials is required. This can be changed with option
> CURLOPT_GSSAPI_DELEGATION in libcurl with set expected parameter
> since libcurl version 7.22.0.
>
> This patch provides new configuration variable http.delegation
> which corresponds to curl parameter "--delegation" (see man 1 curl).
>
> The following values are supported:
>
> * none (default).
> * policy
> * always
I don't personally use Kerberos delegation with Git, but I don't see any
problems with this patch. It preserves the security properties of the
current behavior, and I think adding "policy" as an option to allow
per-realm configuration is a good idea.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v6 3/4] ls-files: pass through safe options for --recurse-submodules
From: Junio C Hamano @ 2016-09-30 0:14 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, peff
In-Reply-To: <1475185723-36871-4-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> +static void compile_submodule_options(const struct dir_struct *dir, int show_tag)
> +{
> + if (line_terminator == '\0')
> + argv_array_push(&submodules_options, "-z");
> + if (show_tag)
> + argv_array_push(&submodules_options, "-t");
> + if (show_valid_bit)
> + argv_array_push(&submodules_options, "-v");
> + if (show_cached)
> + argv_array_push(&submodules_options, "--cached");
> + if (show_deleted)
> + argv_array_push(&submodules_options, "--deleted");
> + if (show_modified)
> + argv_array_push(&submodules_options, "--modified");
> + if (show_others)
> + argv_array_push(&submodules_options, "--others");
> + if (dir->flags & DIR_SHOW_IGNORED)
> + argv_array_push(&submodules_options, "--ignored");
> + if (show_stage)
> + argv_array_push(&submodules_options, "--stage");
> + if (show_killed)
> + argv_array_push(&submodules_options, "--killed");
> + if (dir->flags & DIR_SHOW_OTHER_DIRECTORIES)
> + argv_array_push(&submodules_options, "--directory");
> + if (!(dir->flags & DIR_SHOW_OTHER_DIRECTORIES))
> + argv_array_push(&submodules_options, "--empty-directory");
> + if (show_unmerged)
> + argv_array_push(&submodules_options, "--unmerged");
> + if (show_resolve_undo)
> + argv_array_push(&submodules_options, "--resolve-undo");
> + if (show_eol)
> + argv_array_push(&submodules_options, "--eol");
> + if (debug_mode)
> + argv_array_push(&submodules_options, "--debug");
> +}
With this and 4/4 applied, the documentation still says "--cached"
is the only supported option.
Does it really make sense to pass all of these? I understand "-z"
and I suspect things like "-t" and "-v" that affect "how" things are
shown may also happen to work, but I am not sure how much it makes
sense for options that affect "what" things are shown.
What does it even mean to ask for say "--unmerged" to be shown, for
example, from the superproject? Recurse into submodules whose cache
entries in the index of the superproject are unmerged, or something
else?
I am inclined to say that it is probably better to keep the
"--cached only" as documented, at least on the "what are shown"
side.
Thanks.
^ permalink raw reply
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Linus Torvalds @ 2016-09-30 0:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Johannes Sixt, Git Mailing List
In-Reply-To: <xmqqbmz6cna5.fsf@gitster.mtv.corp.google.com>
On Thu, Sep 29, 2016 at 4:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> One thing that worries me is if we are ready to start accessing the
> object store in all codepaths when we ask for DEFAULT_ABBREV.
Yes. That was my main worry too. I also looked at just doing an explicit
if (abbrev_commit && default_abbrev < 0)
default_abbrev = get_default_abbrev();
and in many ways that would be nicer exactly because the point where
this happens is then explicit, instead of being hidden behind that
macro that may end up being done in random places.
But it wasn't entirely obvious which all paths would need that
initialization either, so on the whole it was very much a "six of one,
half a dozen of the other" thing.
As you say, my original patch had neither of those issues. It just
stupidly re-did the loop over and over, and maybe the right thing to
do is to have that original code, but just short-circuit the "over and
over" behavior by just resetting default_abbrev to the value we do
find.
Linus
^ permalink raw reply
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Linus Torvalds @ 2016-09-30 0:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Johannes Sixt, Git Mailing List
In-Reply-To: <CA+55aFysvNc4p_nFcV=edctCizJBJtDjFOHJa-YYgVZQgBZfiA@mail.gmail.com>
On Thu, Sep 29, 2016 at 5:20 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> As you say, my original patch had neither of those issues.
To be fair, my original patch had a different worry that I didn't
bother with: what if one of the _other_ callers of "get_short_sha1()"
passed in -1 to it. I only handled the -1 case in th eone path care
about in that first RFC for testing. So I'm *not* suggesting you
should apply my first version,, It has issues too.
Let me see if I can massage my first hacky RFC test-patch into
something more reliable.
Linus
^ permalink raw reply
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Linus Torvalds @ 2016-09-30 0:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Johannes Sixt, Git Mailing List
In-Reply-To: <CA+55aFyXxQSygO-gqevLZDjuggOaHs7HsRO=P6GhpC3GStqwvQ@mail.gmail.com>
On Thu, Sep 29, 2016 at 5:28 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> To be fair, my original patch had a different worry that I didn't
> bother with: what if one of the _other_ callers of "get_short_sha1()"
> passed in -1 to it. I only handled the -1 case in th eone path care
> about in that first RFC for testing. So I'm *not* suggesting you
> should apply my first version,, It has issues too.
Actually, all the other cases seem to be "parse a SHA1 with a known
length", so they really don't have a negative length. So this seems
ok, and is easier to verify than the "what all contexts might use
DEFAULT_ABBREV" thing. There's only a few callers, and it's a static
function so it's easy to check it locally in sha1_name.c.
Linus
^ permalink raw reply
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Linus Torvalds @ 2016-09-30 1:01 UTC (permalink / raw)
To: Mike Hommey; +Cc: Junio C Hamano, Johannes Sixt, Git Mailing List, Jeff King
In-Reply-To: <20160930005638.almd66ralshknoxa@glandium.org>
On Thu, Sep 29, 2016 at 5:56 PM, Mike Hommey <mh@glandium.org> wrote:
>
> OTOH, how often does one refer to trees or blobs with abbreviated sha1s?
> Most of the time, you'd use abbreviated sha1s for commits. And the number
> of commits in git and the kernel repositories are much lower than the
> number of overall objects.
See that whole other discussion about this. I agree. If we only ever
worried about just commits, the abbreviation length wouldn't need to
be grown nearly as aggressively. The current default would still be
wrong for the kernel, but it wouldn't be as noticeably wrong, and
updating it to 8 or 9 would be fine.
That said, people argued against that too. We *do* end up having
abbreviated SHA1's for blobs in the diff index. When I said that _I_
neer use it, somebody piped up to say that they do.
So I'd rather just keep the existing semantics (a hash is a hash is a
hash), and just abbreviate at a sufficient point that we don't have to
worry too much about disambiguating further by object type.
Linus
^ permalink raw reply
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Linus Torvalds @ 2016-09-30 1:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Johannes Sixt, Git Mailing List
In-Reply-To: <CA+55aFxsfxvDQqi2M3TUVvAHUx3Qm1hHQ4DMyzXzN6V2v7o-3A@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1470 bytes --]
On Thu, Sep 29, 2016 at 5:57 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Actually, all the other cases seem to be "parse a SHA1 with a known
> length", so they really don't have a negative length. So this seems
> ok, and is easier to verify than the "what all contexts might use
> DEFAULT_ABBREV" thing. There's only a few callers, and it's a static
> function so it's easy to check it locally in sha1_name.c.
Here's my original patch with just a tiny change that instead of
starting the automatic guessing at 7 each time, it starts at
"default_automatic_abbrev", which is initialized to 7.
The difference is that if we decide that "oh, that was too small, need
to repeat", we also update that "default_automatic_abbrev" value, so
that we won't start at the number that we now know was too small.
So it still loops over the abbrev values, but now it only loops a
couple of times.
I actually verified the performance impact by doing
time git rev-list --abbrev-commit HEAD > /dev/null
on the kernel git tree, and it does actually matter. With my original
patch, we wasted a noticeable amount of time on just the extra
looping, with this it's down to the same performance as just doing it
once at init time (it's about 12s vs 9s on my laptop).
So this patch may actually be "production ready" apart from the fact
that some tests still fail (at least t2027-worktree-list.sh) because
of different short SHA1 cases.
Linus
[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 3393 bytes --]
cache.h | 1 +
environment.c | 2 +-
sha1_name.c | 26 +++++++++++++++++++++++++-
3 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/cache.h b/cache.h
index 6e33f2f28..d2da6d186 100644
--- a/cache.h
+++ b/cache.h
@@ -1207,6 +1207,7 @@ struct object_context {
#define GET_SHA1_TREEISH 020
#define GET_SHA1_BLOB 040
#define GET_SHA1_FOLLOW_SYMLINKS 0100
+#define GET_SHA1_AUTOMATIC 0200
#define GET_SHA1_ONLY_TO_DIE 04000
#define GET_SHA1_DISAMBIGUATORS \
diff --git a/environment.c b/environment.c
index c1442df9a..fd6681e46 100644
--- a/environment.c
+++ b/environment.c
@@ -16,7 +16,7 @@ int trust_executable_bit = 1;
int trust_ctime = 1;
int check_stat = 1;
int has_symlinks = 1;
-int minimum_abbrev = 4, default_abbrev = 7;
+int minimum_abbrev = 4, default_abbrev = -1;
int ignore_case;
int assume_unchanged;
int prefer_symlink_refs;
diff --git a/sha1_name.c b/sha1_name.c
index 3b647fd7c..1003c96ea 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -15,6 +15,7 @@ typedef int (*disambiguate_hint_fn)(const unsigned char *, void *);
struct disambiguate_state {
int len; /* length of prefix in hex chars */
+ unsigned int nrobjects;
char hex_pfx[GIT_SHA1_HEXSZ + 1];
unsigned char bin_pfx[GIT_SHA1_RAWSZ];
@@ -118,6 +119,12 @@ static void find_short_object_filename(struct disambiguate_state *ds)
if (strlen(de->d_name) != 38)
continue;
+
+ // We only look at the one subdirectory, and we assume
+ // each subdirectory is roughly similar, so each object
+ // we find probably has 255 other objects in the other
+ // fan-out directories
+ ds->nrobjects += 256;
if (memcmp(de->d_name, ds->hex_pfx + 2, ds->len - 2))
continue;
memcpy(hex + 2, de->d_name, 38);
@@ -151,6 +158,7 @@ static void unique_in_pack(struct packed_git *p,
open_pack_index(p);
num = p->num_objects;
+ ds->nrobjects += num;
last = num;
while (first < last) {
uint32_t mid = (first + last) / 2;
@@ -380,6 +388,9 @@ static int show_ambiguous_object(const unsigned char *sha1, void *data)
return 0;
}
+// Why seven? That's our historical default before the automatic abbreviation
+static int default_automatic_abbrev = 7;
+
static int get_short_sha1(const char *name, int len, unsigned char *sha1,
unsigned flags)
{
@@ -426,6 +437,14 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
for_each_abbrev(ds.hex_pfx, show_ambiguous_object, &ds);
}
+ if (len < 16 && !status && (flags & GET_SHA1_AUTOMATIC)) {
+ unsigned int expect_collision = 1 << (len * 2);
+ if (ds.nrobjects > expect_collision) {
+ default_automatic_abbrev = len+1;
+ return SHORT_NAME_AMBIGUOUS;
+ }
+ }
+
return status;
}
@@ -458,14 +477,19 @@ int for_each_abbrev(const char *prefix, each_abbrev_fn fn, void *cb_data)
int find_unique_abbrev_r(char *hex, const unsigned char *sha1, int len)
{
int status, exists;
+ int flags = GET_SHA1_QUIETLY;
+ if (len < 0) {
+ flags |= GET_SHA1_AUTOMATIC;
+ len = default_automatic_abbrev;
+ }
sha1_to_hex_r(hex, sha1);
if (len == 40 || !len)
return 40;
exists = has_sha1_file(sha1);
while (len < 40) {
unsigned char sha1_ret[20];
- status = get_short_sha1(hex, len, sha1_ret, GET_SHA1_QUIETLY);
+ status = get_short_sha1(hex, len, sha1_ret, flags);
if (exists
? !status
: status == SHORT_NAME_NOT_FOUND) {
^ permalink raw reply related
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Mike Hommey @ 2016-09-30 0:56 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Junio C Hamano, Johannes Sixt, Git Mailing List, Jeff King
In-Reply-To: <CA+55aFx9Utm9yDZceks+5q9c8ydc2QMYshWwJ0G0GHWWLwSsXQ@mail.gmail.com>
On Thu, Sep 29, 2016 at 12:06:23PM -0700, Linus Torvalds wrote:
> On Thu, Sep 29, 2016 at 11:55 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > For the kernel, just the *math* right now actually gives 12
> > characters. For current git it actually seems to say that 8 is the
> > correct number. For small projects, you'll still see 7.
>
> Sorry, the git number is 9, not 8. The reason is that git has roughly
> 212k objects, and 9 hex digits gets expected collisions at about 256k
> objects.
>
> So the logic means that we'll see 7 hex digits for projects with less
> than 16k objects, 8 hex digits if there are less than 64k objects, and
> 9 hex digits for projects like git that currently have fewer than 256k
> objects.
>
> But git itself might not be *that* far from going to 10 hex digits
> with my patch.
>
> The kernel uses 12 he digits because the collision math says that's
> the right thing for a project with between 4M and 16M objects (with
> the kernel being at 5M).
OTOH, how often does one refer to trees or blobs with abbreviated sha1s?
Most of the time, you'd use abbreviated sha1s for commits. And the number
of commits in git and the kernel repositories are much lower than the
number of overall objects.
rev-list --all --count on the git repo gives me 46790. On the kernel, it
gives 618078.
Now, the interesting thing is looking at the *actual* collisions in
those spaces.
At 9 digits, there's only one commit collision in the kernel repo:
45f014c5264f5e68ef0e51b36f4ef5ede3d18397
45f014c52eef022873b19d6a20eb0ec9668f2b09
And two commit collisions at 8 digits in the git repo:
1536dd9c1df0b7167b139f6666080cc4774ef63f
1536dd9c61b5582cf079999057cb715dd6dc6620
2e6e3e82ee36b3e1bec1db8db24817270080424e
2e6e3e829f3759823d70e7af511bc04cd05ad0af
At 7 digits, there are 5 actual commit collisions in the git repo and
718 in the kernel repo only one of those collisions involve more than 2
commits.
Mike
^ permalink raw reply
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Junio C Hamano @ 2016-09-30 3:54 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Jeff King, Johannes Sixt, Git Mailing List
In-Reply-To: <CA+55aFyHn0Q-qPq4dPEJ7X_4jf5UbsVw2vE-4LoWYbPn6gS10g@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> So this patch may actually be "production ready" apart from the fact
> that some tests still fail (at least t2027-worktree-list.sh) because
> of different short SHA1 cases.
t2027 has at least two problems.
* "git worktree" does not read the core.abbrev configuration,
without a recent fix in jc/worktree-config, i.e. d49028e6
("worktree: honor configuration variables", 2016-09-26).
* The script uses "git rev-parse --short HEAD"; I suspect that it
says "ah, default_abbrev is -1 and minimum_abbrev is 4, so let's
try abbreviating to 4 hexdigits".
The first failure in t3203 seems to come from the same issue in
"rev-parse --short".
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox