Re: [PATCH 2/2] Speed up is_git_command() by checking early for internal commands
On Mon, Dec 30, 2013 at 10:07 PM, Sebastian Schuberth
wrote:
> Since 2dce956 is_git_command() was a bit slow as it does file I/O in the
> call to list_commands_in_dir(). Avoid the file I/O by adding an early
> check for internal commands.
>
> Signed-off-by: Sebastian Schuberth
> ---
> builtin/help.c | 5 ++
> git.c | 242
> ++---
> 2 files changed, 132 insertions(+), 115 deletions(-)
>
> diff --git a/builtin/help.c b/builtin/help.c
> index b6fc15e..1f0261e 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -284,10 +284,15 @@ static int git_help_config(const char *var, const char
> *value, void *cb)
> return git_default_config(var, value, cb);
> }
>
> +extern int is_internal_command(const char *s);
> +
Starting the new year in keeping with the fine tradition of asking
people who add stuff to clean up what others left behind, I would
suggest moving all the code related to internal commands (or maybe all
commands) in a new pair of files like "internal-cmds.{c,h}". This way
git.c and builtin/help.c could include internal-cmds.h and you
wouldn't need such extern declaration.
Happy new year everyone,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] Speed up is_git_command() by checking early for internal commands
On 02.01.2014 09:51, Christian Couder wrote:
diff --git a/builtin/help.c b/builtin/help.c
index b6fc15e..1f0261e 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -284,10 +284,15 @@ static int git_help_config(const char *var, const char
*value, void *cb)
return git_default_config(var, value, cb);
}
+extern int is_internal_command(const char *s);
+
Starting the new year in keeping with the fine tradition of asking
people who add stuff to clean up what others left behind, I would
suggest moving all the code related to internal commands (or maybe all
commands) in a new pair of files like "internal-cmds.{c,h}". This way
git.c and builtin/help.c could include internal-cmds.h and you
wouldn't need such extern declaration.
Wouldn't the existing builtin.h be a more appropriate for this? (And
create a builtin.c for the implementations.)
Also, I start to realize that it's a bit unfortunate that we seem to use
the terms "builtin" and "internal command" interchangeably. I'll
probably add a patch to address this.
--
Sebastian Schuberth
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/4]
This is the second iteration of the patches in http://www.spinics.net/lists/git/msg222428.html http://www.spinics.net/lists/git/msg222429.html which * adds a commit to use the term "builtin" instead of "internal command", * also modifies the docs accordingly, * moves the is_builtin() declaration to the existing builtin.h, * finally moves all builtin-related definitions to a new builtin.c file. Sebastian Schuberth (4): Consistently use the term "builtin" instead of "internal command" Call load_command_list() only when it is needed Speed up is_git_command() by checking early for internal commands Move builtin-related implementations to a new builtin.c file Documentation/technical/api-builtin.txt | 4 +- Makefile| 1 + builtin.c | 225 ++ builtin.h | 23 +++ builtin/help.c | 6 +- git.c | 238 +--- 6 files changed, 262 insertions(+), 235 deletions(-) create mode 100644 builtin.c -- 1.8.3-mingw-1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/4] Speed up is_git_command() by checking early for internal commands
Since 2dce956 is_git_command() is a bit slow as it does file I/O in the
call to list_commands_in_dir(). Avoid the file I/O by adding an early
check for internal commands.
Signed-off-by: Sebastian Schuberth
---
Documentation/technical/api-builtin.txt | 4 +-
builtin.h | 2 +
builtin/help.c | 3 +
git.c | 242 +---
4 files changed, 134 insertions(+), 117 deletions(-)
diff --git a/Documentation/technical/api-builtin.txt
b/Documentation/technical/api-builtin.txt
index 150a02a..e3d6e7a 100644
--- a/Documentation/technical/api-builtin.txt
+++ b/Documentation/technical/api-builtin.txt
@@ -14,8 +14,8 @@ Git:
. Add the external declaration for the function to `builtin.h`.
-. Add the command to `commands[]` table in `handle_builtin()`,
- defined in `git.c`. The entry should look like:
+. Add the command to the `commands[]` table defined in `git.c`.
+ The entry should look like:
{ "foo", cmd_foo, },
+
diff --git a/builtin.h b/builtin.h
index d4afbfe..c47c110 100644
--- a/builtin.h
+++ b/builtin.h
@@ -27,6 +27,8 @@ extern int fmt_merge_msg(struct strbuf *in, struct strbuf
*out,
extern int textconv_object(const char *path, unsigned mode, const unsigned
char *sha1, int sha1_valid, char **buf, unsigned long *buf_size);
+extern int is_builtin(const char *s);
+
extern int cmd_add(int argc, const char **argv, const char *prefix);
extern int cmd_annotate(int argc, const char **argv, const char *prefix);
extern int cmd_apply(int argc, const char **argv, const char *prefix);
diff --git a/builtin/help.c b/builtin/help.c
index b6fc15e..1fdefeb 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -288,6 +288,9 @@ static struct cmdnames main_cmds, other_cmds;
static int is_git_command(const char *s)
{
+ if (is_builtin(s))
+ return 1;
+
load_command_list("git-", &main_cmds, &other_cmds);
return is_in_cmdlist(&main_cmds, s) ||
is_in_cmdlist(&other_cmds, s);
diff --git a/git.c b/git.c
index 89ab5d7..bba4378 100644
--- a/git.c
+++ b/git.c
@@ -332,124 +332,136 @@ static int run_builtin(struct cmd_struct *p, int argc,
const char **argv)
return 0;
}
+static struct cmd_struct commands[] = {
+ { "add", cmd_add, RUN_SETUP | NEED_WORK_TREE },
+ { "annotate", cmd_annotate, RUN_SETUP },
+ { "apply", cmd_apply, RUN_SETUP_GENTLY },
+ { "archive", cmd_archive },
+ { "bisect--helper", cmd_bisect__helper, RUN_SETUP },
+ { "blame", cmd_blame, RUN_SETUP },
+ { "branch", cmd_branch, RUN_SETUP },
+ { "bundle", cmd_bundle, RUN_SETUP_GENTLY },
+ { "cat-file", cmd_cat_file, RUN_SETUP },
+ { "check-attr", cmd_check_attr, RUN_SETUP },
+ { "check-ignore", cmd_check_ignore, RUN_SETUP | NEED_WORK_TREE },
+ { "check-mailmap", cmd_check_mailmap, RUN_SETUP },
+ { "check-ref-format", cmd_check_ref_format },
+ { "checkout", cmd_checkout, RUN_SETUP | NEED_WORK_TREE },
+ { "checkout-index", cmd_checkout_index,
+ RUN_SETUP | NEED_WORK_TREE},
+ { "cherry", cmd_cherry, RUN_SETUP },
+ { "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },
+ { "clean", cmd_clean, RUN_SETUP | NEED_WORK_TREE },
+ { "clone", cmd_clone },
+ { "column", cmd_column, RUN_SETUP_GENTLY },
+ { "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
+ { "commit-tree", cmd_commit_tree, RUN_SETUP },
+ { "config", cmd_config, RUN_SETUP_GENTLY },
+ { "count-objects", cmd_count_objects, RUN_SETUP },
+ { "credential", cmd_credential, RUN_SETUP_GENTLY },
+ { "describe", cmd_describe, RUN_SETUP },
+ { "diff", cmd_diff },
+ { "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE },
+ { "diff-index", cmd_diff_index, RUN_SETUP },
+ { "diff-tree", cmd_diff_tree, RUN_SETUP },
+ { "fast-export", cmd_fast_export, RUN_SETUP },
+ { "fetch", cmd_fetch, RUN_SETUP },
+ { "fetch-pack", cmd_fetch_pack, RUN_SETUP },
+ { "fmt-merge-msg", cmd_fmt_merge_msg, RUN_SETUP },
+ { "for-each-ref", cmd_for_each_ref, RUN_SETUP },
+ { "format-patch", cmd_format_patch, RUN_SETUP },
+ { "fsck", cmd_fsck, RUN_SETUP },
+ { "fsck-objects", cmd_fsck, RUN_SETUP },
+ { "gc", cmd_gc, RUN_SETUP },
+ { "get-tar-commit-id", cmd_get_tar_commit_id },
+ { "grep", cmd_grep, RUN_SETUP_GENTLY },
+ { "hash-object", cmd_hash_object },
+ { "help", cmd_help },
+ { "index-pack", cmd_index_pack, RUN_SETUP_GENTLY },
+ { "init", cmd_init_db },
+ { "init-db", cmd_init_db },
+ { "log", cmd_log, RUN_SETUP },
+ { "ls-files", cmd_ls_files, RUN_SETUP },
+ { "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
+ { "ls-tree", cmd_ls_tree, RUN_SETUP },
+ { "mailinfo", cmd_mailinfo },
+ { "mailsplit", cmd_mailsplit },
+
[PATCH v2 2/4] Call load_command_list() only when it is needed
This avoids list_commands_in_dir() being called when not needed which is
quite slow due to file I/O in order to list matching files in a directory.
Signed-off-by: Sebastian Schuberth
---
builtin/help.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/builtin/help.c b/builtin/help.c
index cc17e67..b6fc15e 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -288,6 +288,7 @@ static struct cmdnames main_cmds, other_cmds;
static int is_git_command(const char *s)
{
+ load_command_list("git-", &main_cmds, &other_cmds);
return is_in_cmdlist(&main_cmds, s) ||
is_in_cmdlist(&other_cmds, s);
}
@@ -449,7 +450,6 @@ int cmd_help(int argc, const char **argv, const char
*prefix)
int nongit;
const char *alias;
enum help_format parsed_help_format;
- load_command_list("git-", &main_cmds, &other_cmds);
argc = parse_options(argc, argv, prefix, builtin_help_options,
builtin_help_usage, 0);
@@ -458,6 +458,7 @@ int cmd_help(int argc, const char **argv, const char
*prefix)
if (show_all) {
git_config(git_help_config, NULL);
printf(_("usage: %s%s"), _(git_usage_string), "\n\n");
+ load_command_list("git-", &main_cmds, &other_cmds);
list_commands(colopts, &main_cmds, &other_cmds);
}
--
1.8.3-mingw-1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/4] Consistently use the term "builtin" instead of "internal command"
Signed-off-by: Sebastian Schuberth
---
Documentation/technical/api-builtin.txt | 2 +-
git.c | 14 +++---
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/Documentation/technical/api-builtin.txt
b/Documentation/technical/api-builtin.txt
index f3c1357..150a02a 100644
--- a/Documentation/technical/api-builtin.txt
+++ b/Documentation/technical/api-builtin.txt
@@ -14,7 +14,7 @@ Git:
. Add the external declaration for the function to `builtin.h`.
-. Add the command to `commands[]` table in `handle_internal_command()`,
+. Add the command to `commands[]` table in `handle_builtin()`,
defined in `git.c`. The entry should look like:
{ "foo", cmd_foo, },
diff --git a/git.c b/git.c
index 3799514..89ab5d7 100644
--- a/git.c
+++ b/git.c
@@ -332,7 +332,7 @@ static int run_builtin(struct cmd_struct *p, int argc,
const char **argv)
return 0;
}
-static void handle_internal_command(int argc, const char **argv)
+static void handle_builtin(int argc, const char **argv)
{
const char *cmd = argv[0];
static struct cmd_struct commands[] = {
@@ -517,8 +517,8 @@ static int run_argv(int *argcp, const char ***argv)
int done_alias = 0;
while (1) {
- /* See if it's an internal command */
- handle_internal_command(*argcp, *argv);
+ /* See if it's a builtin */
+ handle_builtin(*argcp, *argv);
/* .. then try the external ones */
execv_dashed_external(*argv);
@@ -563,14 +563,14 @@ int main(int argc, char **av)
* - cannot execute it externally (since it would just do
*the same thing over again)
*
-* So we just directly call the internal command handler, and
-* die if that one cannot handle it.
+* So we just directly call the builtin handler, and die if
+* that one cannot handle it.
*/
if (starts_with(cmd, "git-")) {
cmd += 4;
argv[0] = cmd;
- handle_internal_command(argc, argv);
- die("cannot handle %s internally", cmd);
+ handle_builtin(argc, argv);
+ die("cannot handle %s as a builtin", cmd);
}
/* Look for flags.. */
--
1.8.3-mingw-1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 4/4] Move builtin-related implementations to a new builtin.c file
Signed-off-by: Sebastian Schuberth
---
Documentation/technical/api-builtin.txt | 2 +-
Makefile| 1 +
builtin.c | 225 ++
builtin.h | 21 +++
git.c | 238
5 files changed, 248 insertions(+), 239 deletions(-)
create mode 100644 builtin.c
diff --git a/Documentation/technical/api-builtin.txt
b/Documentation/technical/api-builtin.txt
index e3d6e7a..d1d946c 100644
--- a/Documentation/technical/api-builtin.txt
+++ b/Documentation/technical/api-builtin.txt
@@ -14,7 +14,7 @@ Git:
. Add the external declaration for the function to `builtin.h`.
-. Add the command to the `commands[]` table defined in `git.c`.
+. Add the command to the `commands[]` table defined in `builtin.c`.
The entry should look like:
{ "foo", cmd_foo, },
diff --git a/Makefile b/Makefile
index b4af1e2..2d947e8 100644
--- a/Makefile
+++ b/Makefile
@@ -763,6 +763,7 @@ LIB_OBJS += base85.o
LIB_OBJS += bisect.o
LIB_OBJS += blob.o
LIB_OBJS += branch.o
+LIB_OBJS += builtin.o
LIB_OBJS += bulk-checkin.o
LIB_OBJS += bundle.o
LIB_OBJS += cache-tree.o
diff --git a/builtin.c b/builtin.c
new file mode 100644
index 000..6bdeb7c
--- /dev/null
+++ b/builtin.c
@@ -0,0 +1,225 @@
+#include "builtin.h"
+
+static struct cmd_struct commands[] = {
+ { "add", cmd_add, RUN_SETUP | NEED_WORK_TREE },
+ { "annotate", cmd_annotate, RUN_SETUP },
+ { "apply", cmd_apply, RUN_SETUP_GENTLY },
+ { "archive", cmd_archive },
+ { "bisect--helper", cmd_bisect__helper, RUN_SETUP },
+ { "blame", cmd_blame, RUN_SETUP },
+ { "branch", cmd_branch, RUN_SETUP },
+ { "bundle", cmd_bundle, RUN_SETUP_GENTLY },
+ { "cat-file", cmd_cat_file, RUN_SETUP },
+ { "check-attr", cmd_check_attr, RUN_SETUP },
+ { "check-ignore", cmd_check_ignore, RUN_SETUP | NEED_WORK_TREE },
+ { "check-mailmap", cmd_check_mailmap, RUN_SETUP },
+ { "check-ref-format", cmd_check_ref_format },
+ { "checkout", cmd_checkout, RUN_SETUP | NEED_WORK_TREE },
+ { "checkout-index", cmd_checkout_index,
+ RUN_SETUP | NEED_WORK_TREE},
+ { "cherry", cmd_cherry, RUN_SETUP },
+ { "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },
+ { "clean", cmd_clean, RUN_SETUP | NEED_WORK_TREE },
+ { "clone", cmd_clone },
+ { "column", cmd_column, RUN_SETUP_GENTLY },
+ { "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
+ { "commit-tree", cmd_commit_tree, RUN_SETUP },
+ { "config", cmd_config, RUN_SETUP_GENTLY },
+ { "count-objects", cmd_count_objects, RUN_SETUP },
+ { "credential", cmd_credential, RUN_SETUP_GENTLY },
+ { "describe", cmd_describe, RUN_SETUP },
+ { "diff", cmd_diff },
+ { "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE },
+ { "diff-index", cmd_diff_index, RUN_SETUP },
+ { "diff-tree", cmd_diff_tree, RUN_SETUP },
+ { "fast-export", cmd_fast_export, RUN_SETUP },
+ { "fetch", cmd_fetch, RUN_SETUP },
+ { "fetch-pack", cmd_fetch_pack, RUN_SETUP },
+ { "fmt-merge-msg", cmd_fmt_merge_msg, RUN_SETUP },
+ { "for-each-ref", cmd_for_each_ref, RUN_SETUP },
+ { "format-patch", cmd_format_patch, RUN_SETUP },
+ { "fsck", cmd_fsck, RUN_SETUP },
+ { "fsck-objects", cmd_fsck, RUN_SETUP },
+ { "gc", cmd_gc, RUN_SETUP },
+ { "get-tar-commit-id", cmd_get_tar_commit_id },
+ { "grep", cmd_grep, RUN_SETUP_GENTLY },
+ { "hash-object", cmd_hash_object },
+ { "help", cmd_help },
+ { "index-pack", cmd_index_pack, RUN_SETUP_GENTLY },
+ { "init", cmd_init_db },
+ { "init-db", cmd_init_db },
+ { "log", cmd_log, RUN_SETUP },
+ { "ls-files", cmd_ls_files, RUN_SETUP },
+ { "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
+ { "ls-tree", cmd_ls_tree, RUN_SETUP },
+ { "mailinfo", cmd_mailinfo },
+ { "mailsplit", cmd_mailsplit },
+ { "merge", cmd_merge, RUN_SETUP | NEED_WORK_TREE },
+ { "merge-base", cmd_merge_base, RUN_SETUP },
+ { "merge-file", cmd_merge_file, RUN_SETUP_GENTLY },
+ { "merge-index", cmd_merge_index, RUN_SETUP },
+ { "merge-ours", cmd_merge_ours, RUN_SETUP },
+ { "merge-recursive", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
+ { "merge-recursive-ours", cmd_merge_recursive, RUN_SETUP |
NEED_WORK_TREE },
+ { "merge-recursive-theirs", cmd_merge_recursive, RUN_SETUP |
NEED_WORK_TREE },
+ { "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
+ { "merge-tree", cmd_merge_tree, RUN_SETUP },
+ { "mktag", cmd_mktag, RUN_SETUP },
+ { "mktree", cmd_mktree, RUN_SETUP },
+ { "mv", cmd_mv, RUN_SETUP | NEED_WORK_TREE },
+ { "name-rev", cmd_name_rev, RUN_SETUP },
+ { "notes", cmd_notes, RUN_SETUP },
[no subject]
help -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Fix safe_create_leading_directories() for Windows
See https://github.com/msysgit/git/pull/80.
Signed-off-by: Johannes Schindelin
Signed-off-by: Sebastian Schuberth
---
sha1_file.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 760dd60..2114c58 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -110,12 +110,15 @@ int safe_create_leading_directories(char *path)
char *pos = path + offset_1st_component(path);
struct stat st;
- while (pos) {
- pos = strchr(pos, '/');
- if (!pos)
- break;
- while (*++pos == '/')
- ;
+ while (*pos) {
+ while (!is_dir_sep(*pos)) {
+ ++pos;
+ if (!*pos)
+ break;
+ }
+ /* skip consecutive directory separators */
+ while (is_dir_sep(*pos))
+ ++pos;
if (!*pos)
break;
*--pos = '\0';
--
1.8.4.msysgit.0.1.ge705bba.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix safe_create_leading_directories() for Windows
Hi, On Thu, 2 Jan 2014, Sebastian Schuberth wrote: > See https://github.com/msysgit/git/pull/80. Thanks Sebastian! However, since the git.git project is not comfortable with the concept of pull requests (which is why you submitted this patch via mail), I believe that we have to explain the rationale in the commit message. So here goes my attempt: -- snip -- On Linux, we can get away with assuming that the directory separator is a forward slash, but that is wrong in general. For that purpose, the is_dir_sep() function was introduced a long time ago. By using it in safe_create_leading_directories(), we proof said function for use on platforms where the directory separator is different from Linux'. -- snap -- Hmm? Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC] blame: new option to better handle merged cherry-picks
Allows to disable the git blame optimization of assuming that if there is a
parent of a merge commit that has the exactly same file content, then
only this parent is to be looked at.
This optimization, while being faster in the usual case, means that in
the case of cherry-picks the blamed commit depends on which other commits
touched a file.
If for example one commit A modified both files b and c. And there are
commits B and C, B only modifies file b and C only modifies file c
(so that no conflicts happen), and assume A is cherry-picked as A'
and the two branches then merged:
--o-B---A
\ \
---C---A'--M---
Then without this new option git blame blames the A|A' changes of
file b to A while blaming the changes of c to A'.
With the new option --no-parent-shortcut it blames both changes to the
same commit.
Signed-off-by: Bernhard R. Link
---
Documentation/blame-options.txt | 6 ++
builtin/blame.c | 5 -
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index 0cebc4f..55dd12b 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -48,6 +48,12 @@ include::line-range-format.txt[]
Show the result incrementally in a format designed for
machine consumption.
+--no-parent-shortcut::
+ Always look at all parents of a merge and do not shortcut
+ to the first parent with no changes to the file looked at.
+ This takes more time but produces more reliable results
+ if branches with cherry-picked commits were merged.
+
--encoding=::
Specifies the encoding used to output author names
and commit summaries. Setting it to `none` makes blame
diff --git a/builtin/blame.c b/builtin/blame.c
index 4916eb2..dab2c36 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -45,6 +45,7 @@ static int incremental;
static int xdl_opts;
static int abbrev = -1;
static int no_whole_file_rename;
+static int no_parent_shortcut;
static enum date_mode blame_date_mode = DATE_ISO8601;
static size_t blame_date_width;
@@ -1248,7 +1249,8 @@ static void pass_blame(struct scoreboard *sb, struct
origin *origin, int opt)
porigin = find(sb, p, origin);
if (!porigin)
continue;
- if (!hashcmp(porigin->blob_sha1, origin->blob_sha1)) {
+ if (!no_parent_shortcut &&
+ !hashcmp(porigin->blob_sha1, origin->blob_sha1)) {
pass_whole_blame(sb, origin, porigin);
origin_decref(porigin);
goto finish;
@@ -2247,6 +2249,7 @@ int cmd_blame(int argc, const char **argv, const char
*prefix)
static const char *contents_from = NULL;
static const struct option options[] = {
OPT_BOOL(0, "incremental", &incremental, N_("Show blame entries
as we find them, incrementally")),
+ OPT_BOOL(0, "no-parent-shortcut", &no_parent_shortcut,
N_("Don't take shortcuts in some merges but handle cherry-picks better")),
OPT_BOOL('b', NULL, &blank_boundary, N_("Show blank SHA-1 for
boundary commits (Default: off)")),
OPT_BOOL(0, "root", &show_root, N_("Do not treat root commits
as boundaries (Default: off)")),
OPT_BOOL(0, "show-stats", &show_stats, N_("Show work cost
statistics")),
--
1.8.5.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: aborted 'git fetch' leaves workspace unusable
stephen_le...@stephe-leake.org writes: > However, in this case, even running the fetch was a mistake; I would > have prefered that it leave FETCH_HEAD in its previous state. I think the clearing of leftover FETCH_HEAD is one of the early things "git fetch" does, unless "--append" is in effect. I haven't looked at the code for a long time, but it may be possible to move the logic of doing so around so that this clearing is done as lazily as possible. I however suspect that such a change may have fallouts on other people who are writing tools like yours; they may be depending on seeing FETCH_HEAD cleared after a failed fetch, and be surprised to see a stale contents after they (attempt to) run "git fetch" in it. So it is not so clear if it is a good thing to change the behaviour of "git fetch" not to touch FETCH_HEAD upon a failure. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix safe_create_leading_directories() for Windows
On 02.01.2014 18:33, Johannes Schindelin wrote: -- snip -- On Linux, we can get away with assuming that the directory separator is a forward slash, but that is wrong in general. For that purpose, the is_dir_sep() function was introduced a long time ago. By using it in safe_create_leading_directories(), we proof said function for use on platforms where the directory separator is different from Linux'. -- snap -- While I'd be fine with this, I do not think we really need it. As you say, is_dir_sep() has been introduced a long time ago, so people should be aware of it, and it should also be immediately clear from the diff why using it is better than hard-coding '/'. That said, I see any further explanations on top of the commit message title is an added bonus, and as "just" a bonus a link to a pull request should be fine. You don't need to understand or appreciate the concept of pull requests in order to follow the link and read the text in there. -- Sebastian Schuberth -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix safe_create_leading_directories() for Windows
On Thu, Jan 02, 2014 at 07:11:42PM +0100, Sebastian Schuberth wrote: > On 02.01.2014 18:33, Johannes Schindelin wrote: > > > -- snip -- > > On Linux, we can get away with assuming that the directory separator is a > > forward slash, but that is wrong in general. For that purpose, the > > is_dir_sep() function was introduced a long time ago. By using it in > > safe_create_leading_directories(), we proof said function for use on > > platforms where the directory separator is different from Linux'. > > -- snap -- > > While I'd be fine with this, I do not think we really need it. As you > say, is_dir_sep() has been introduced a long time ago, so people should > be aware of it, and it should also be immediately clear from the diff > why using it is better than hard-coding '/'. > > That said, I see any further explanations on top of the commit message > title is an added bonus, and as "just" a bonus a link to a pull request > should be fine. You don't need to understand or appreciate the concept > of pull requests in order to follow the link and read the text in there. The commit message serves as an historical record of why a change was made; depending on an external service to provide this information when it can quite easily be included in the commit itself lessens the value of the commit message. If you look at other commits in git.git you will see that there is a strong preference for summarising the discussion and rationale for a commit in its message. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] Introduce git submodule add|update --attach
Thanks for the comments, my replies below. Before, a couple of general questions: - I'm also writing some tests, should I commit them together with the feature patch? - to determine the attached/detached state I did this: head_detached= if test "$(rev-parse --abbrev-ref HEAD)" = "HEAD" then head_detached="true" fi Is this correct? 2013/12/31 Phil Hord > > On Sun, Dec 29, 2013 at 8:49 PM, Francesco Pretto wrote: > [...] > > > > When updating, using the '--attach' switch or operating in a repository with > > 'submodule..attach' set to 'true' will: > > - checkout a branch with an attached HEAD if the repository was just cloned; > > - perform a fast-forward only merge of changes if it's a 'checkout' update, > > keeping the HEAD attached; > > - reattach the HEAD prior performing a 'merge', 'rebase' or '!command' > > update > > operation if the HEAD was found detached. > > I need to understand this "reattach the HEAD" case better. Can you > give some examples of the expected behavior when merge, rebase or > !command is encountered? > Thanks for pointing this out, actually my patch was a bit lacking at this. Reattaching the HEAD prior to merge, rebase or "!command" would have caused just a *silent* "git checkout "", possibly leaving orphaned commits forgotten. My plans for the patch are now to implement this safer and, IMO, intuitive behavior: let set say we have a submodule "mod" with the HEAD detached at orphaned commit . Let say "origin/" is at commit . Let say I set "submodule.mod.attach" to "true" or I run "git submodule update" with the "--attach" switch. The expected behavior for submodule "mod" would be (pseudocode): git checkout if "merge" and "head_detached" then git merge case: "merge": git merge "rebase": git rebase "!": if "rebase" and "head_detached" then git merge So, in both "merge|rebase" cases we merge back orphaned commits with a "git merge", but the effect will be a merge or a rebase depending of the ordering of the main "update" operation. We can't assume a merge or rebase operation in the case of !command so we let the responsibility of merging back orphaned commits to the user. Sounds good? > > > + > > +--attach:: > > + This option is only valid for the add and update commands. Cause the > > Grammar: 'Causes the result' > Ok. > > > + result of an add or update operation to be an attached HEAD. In the > > + update command , if `submodule..branch` is not set, it will > > typo: space before comma. > Ok. > > Also, the pronoun "it" here is unclear to me. Does this convey the > correct meaning? > >In the update operation, the branch named by 'submodule..branch' is >checked out as the new HEAD of the submodule repository. If >'submodule..branch' is not set, the 'master' branch is > checked out as the >new HEAD of the submodule. > Sounds good to me. > > > + default to `master`. Note: for the update command `--attach` also > > + implies `--remote`. > > + > > +--detach:: > > + This option is only valid for the update command. Cause the result > > Grammar: 'Causes the result' > Ok. > > > + of the update operation to be forcedly a detached HEAD. > > "Forcedly" is a bit strong, maybe, slightly misplaced, and not a word, > besides. How's this, instead: > >Forces the result of the update operation to be a detached HEAD in > the submodule. Sounds good to me. > > > submodule..update:: > > Defines what to do when the submodule is updated by the > > superproject. > > - If 'checkout' (the default), the new commit specified in the > > - superproject will be checked out in the submodule on a detached > > HEAD. > > + If 'checkout' (the default), the new commit (or the branch, when > > using > > + the '--attach' switch or the 'submodule..attach' property is > > set > > + to 'true' during an update operation) specified in the superproject > > will > > + be checked out in the submodule. > > IMHO, this wording is overcomplicated by this change. How about: > >If 'checkout' (the default), the new commit specified in the > superproject >(or branch, with '--attach') will be checked out in the submodule. > Sounds good to me. > > > If 'rebase', the current branch of the submodule will be rebased > > onto > > the commit specified in the superproject. If 'merge', the commit > > specified in the superproject will be merged into the current branch > > Does the 'merge', 'rebase' and '!command' description need to be > updated, too? Here and above it seems to still suggest the old > behavior is kept when --attach is used. > You are right, I'll improve those. Also I think the documentation was a bit innacurate because, with the current git features state, it's possible merge changes keeping the HEAD detached, and that's what happen. > > > @@ -54,6 +56,11 @@ submodule..branch:: > > If
Re: [PATCH] drop unnecessary copying in credential_ask_one
Jeff King writes:
> ... But the test suite, of course, always uses askpass because it
> cannot rely on accessing a terminal (we'd have to do some magic with
> lib-terminal, I think).
>
> So it doesn't detect the problem in your patch, but I wonder if it is
> worth applying the patch below anyway, as it makes the test suite
> slightly more robust.
Sounds like a good first step in the right direction. Thanks.
> -- >8 --
> Subject: use distinct username/password for http auth tests
>
> The httpd server we set up to test git's http client code
> knows about a single account, in which both the username and
> password are "user@host" (the unusual use of the "@" here is
> to verify that we handle the character correctly when URL
> escaped).
>
> This means that we may miss a certain class of errors in
> which the username and password are mixed up internally by
> git. We can make our tests more robust by having distinct
> values for the username and password.
>
> In addition to tweaking the server passwd file and the
> client URL, we must teach the "askpass" harness to accept
> multiple values. As a bonus, this makes the setup of some
> tests more obvious; when we are expecting git to ask
> only about the password, we can seed the username askpass
> response with a bogus value.
>
> Signed-off-by: Jeff King
> ---
> t/lib-httpd.sh| 15 ---
> t/lib-httpd/passwd| 2 +-
> t/t5540-http-push.sh | 4 ++--
> t/t5541-http-push.sh | 6 +++---
> t/t5550-http-fetch.sh | 10 +-
> t/t5551-http-fetch.sh | 6 +++---
> 6 files changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
> index c470784..bfdff2a 100644
> --- a/t/lib-httpd.sh
> +++ b/t/lib-httpd.sh
> @@ -129,7 +129,7 @@ prepare_httpd() {
> HTTPD_DEST=127.0.0.1:$LIB_HTTPD_PORT
> HTTPD_URL=$HTTPD_PROTO://$HTTPD_DEST
> HTTPD_URL_USER=$HTTPD_PROTO://user%40host@$HTTPD_DEST
> - HTTPD_URL_USER_PASS=$HTTPD_PROTO://user%40host:user%40host@$HTTPD_DEST
> + HTTPD_URL_USER_PASS=$HTTPD_PROTO://user%40host:pass%40host@$HTTPD_DEST
>
> if test -n "$LIB_HTTPD_DAV" -o -n "$LIB_HTTPD_SVN"
> then
> @@ -217,7 +217,15 @@ setup_askpass_helper() {
> test_expect_success 'setup askpass helper' '
> write_script "$TRASH_DIRECTORY/askpass" <<-\EOF &&
> echo >>"$TRASH_DIRECTORY/askpass-query" "askpass: $*" &&
> - cat "$TRASH_DIRECTORY/askpass-response"
> + case "$*" in
> + *Username*)
> + what=user
> + ;;
> + *Password*)
> + what=pass
> + ;;
> + esac &&
> + cat "$TRASH_DIRECTORY/askpass-$what"
> EOF
> GIT_ASKPASS="$TRASH_DIRECTORY/askpass" &&
> export GIT_ASKPASS &&
> @@ -227,7 +235,8 @@ setup_askpass_helper() {
>
> set_askpass() {
> >"$TRASH_DIRECTORY/askpass-query" &&
> - echo "$*" >"$TRASH_DIRECTORY/askpass-response"
> + echo "$1" >"$TRASH_DIRECTORY/askpass-user" &&
> + echo "$2" >"$TRASH_DIRECTORY/askpass-pass"
> }
>
> expect_askpass() {
> diff --git a/t/lib-httpd/passwd b/t/lib-httpd/passwd
> index f2fbcad..99a34d6 100644
> --- a/t/lib-httpd/passwd
> +++ b/t/lib-httpd/passwd
> @@ -1 +1 @@
> -user@host:nKpa8pZUHx/ic
> +user@host:xb4E8pqD81KQs
> diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
> index 01d0d95..5b0198c 100755
> --- a/t/t5540-http-push.sh
> +++ b/t/t5540-http-push.sh
> @@ -154,7 +154,7 @@ test_http_push_nonff
> "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git \
>
> test_expect_success 'push to password-protected repository (user in URL)' '
> test_commit pw-user &&
> - set_askpass user@host &&
> + set_askpass user@host pass@host &&
> git push "$HTTPD_URL_USER/auth/dumb/test_repo.git" HEAD &&
> git rev-parse --verify HEAD >expect &&
> git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/test_repo.git" \
> @@ -168,7 +168,7 @@ test_expect_failure 'user was prompted only once for
> password' '
>
> test_expect_failure 'push to password-protected repository (no user in URL)'
> '
> test_commit pw-nouser &&
> - set_askpass user@host &&
> + set_askpass user@host pass@host &&
> git push "$HTTPD_URL/auth/dumb/test_repo.git" HEAD &&
> expect_askpass both user@host
> git rev-parse --verify HEAD >expect &&
> diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
> index 470ac54..bfd241e 100755
> --- a/t/t5541-http-push.sh
> +++ b/t/t5541-http-push.sh
> @@ -274,7 +274,7 @@ test_expect_success 'push over smart http with auth' '
> cd "$ROOT_PATH/test_repo_clone" &&
> echo push-auth-test >expect &&
> test_commit push-auth-test &&
> - set_askpass user@host &&
> + set_askpass user@host pass@host &&
> git push "$HTTPD_URL"/auth/smart/test_repo.git &&
> git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git" \
> lo
Re: [PATCH v2 3/4] Speed up is_git_command() by checking early for internal commands
Sebastian Schuberth writes:
> Since 2dce956 is_git_command() is a bit slow as it does file I/O in the
> call to list_commands_in_dir(). Avoid the file I/O by adding an early
> check for internal commands.
I think it is a good thing to check with the list of built-in's
first, but in order to see if one name we already have at hand is a
git command, I have to wonder if it is the best we can do to collect
all the possible command names with load_command_list() and check
the membership.
There are two callers of is_in_cmdlist(), and the way they use the
function looks both very backwards.
- builtin/help.c has a user supplied string that it wants to see if
it is a git command (either on-disk in exec-path or a builtin),
either to see if an alias in a config is really in effect, or to
see if it is a command (i.e. invoke 'man' with git-) or a
concept (i.e. invoke 'man' with git). It feels that it
should be a lot less work to just check with the builtin table
and stat( + ) for either of these purposes (on
Windows you may have to append .exe before checking and may also
have to check with .com so you may have to do more than one
stat(), but still).
Even though help is not a performance critical codepath,
optimizing this further so that we do not load the full set of
commands unnecessarily may be in line with the theme of your
series, I think.
- builtin/merge.c is the same, but it is conceptually even worse.
It has the end-user supplied string and wants to see if it is a
valid strategy. If the user wants to use a custom strategy, a
single stat() to make sure if it exists should suffice, and the
error codepath should load the command list to present the names
of available ones in the error message.
Based on the above observation, I have a feeling that we will be
better off to aim for getting rid of is_in_cmdlist(), reimplement
is_git_command() to check with builtin and then do a stat (or two)
inside the exec-path, and update builtin/merge.c codepath to use
is_git_command() to see if the given name is indeed a strategy.
So in short, I very much like the part that moves the built-in
command list out of the main() function and makes is_git_command()
use it, but I think the primary implementation of is_git_command()
to check if the given string names an on-disk command is still not
very nice.
Thanks.
> Signed-off-by: Sebastian Schuberth
> ---
> Documentation/technical/api-builtin.txt | 4 +-
> builtin.h | 2 +
> builtin/help.c | 3 +
> git.c | 242
> +---
> 4 files changed, 134 insertions(+), 117 deletions(-)
>
> diff --git a/Documentation/technical/api-builtin.txt
> b/Documentation/technical/api-builtin.txt
> index 150a02a..e3d6e7a 100644
> --- a/Documentation/technical/api-builtin.txt
> +++ b/Documentation/technical/api-builtin.txt
> @@ -14,8 +14,8 @@ Git:
>
> . Add the external declaration for the function to `builtin.h`.
>
> -. Add the command to `commands[]` table in `handle_builtin()`,
> - defined in `git.c`. The entry should look like:
> +. Add the command to the `commands[]` table defined in `git.c`.
> + The entry should look like:
>
> { "foo", cmd_foo, },
> +
> diff --git a/builtin.h b/builtin.h
> index d4afbfe..c47c110 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -27,6 +27,8 @@ extern int fmt_merge_msg(struct strbuf *in, struct strbuf
> *out,
>
> extern int textconv_object(const char *path, unsigned mode, const unsigned
> char *sha1, int sha1_valid, char **buf, unsigned long *buf_size);
>
> +extern int is_builtin(const char *s);
> +
> extern int cmd_add(int argc, const char **argv, const char *prefix);
> extern int cmd_annotate(int argc, const char **argv, const char *prefix);
> extern int cmd_apply(int argc, const char **argv, const char *prefix);
> diff --git a/builtin/help.c b/builtin/help.c
> index b6fc15e..1fdefeb 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -288,6 +288,9 @@ static struct cmdnames main_cmds, other_cmds;
>
> static int is_git_command(const char *s)
> {
> + if (is_builtin(s))
> + return 1;
> +
> load_command_list("git-", &main_cmds, &other_cmds);
> return is_in_cmdlist(&main_cmds, s) ||
> is_in_cmdlist(&other_cmds, s);
> diff --git a/git.c b/git.c
> index 89ab5d7..bba4378 100644
> --- a/git.c
> +++ b/git.c
> @@ -332,124 +332,136 @@ static int run_builtin(struct cmd_struct *p, int
> argc, const char **argv)
> return 0;
> }
>
> +static struct cmd_struct commands[] = {
> + { "add", cmd_add, RUN_SETUP | NEED_WORK_TREE },
> + { "annotate", cmd_annotate, RUN_SETUP },
> + { "apply", cmd_apply, RUN_SETUP_GENTLY },
> + { "archive", cmd_archive },
> + { "bisect--helper", cmd_bisect__helper, RUN_SETUP },
> + { "blame", cmd_blame, RUN_SETUP },
> + { "branch", cmd_branch, RUN_SETUP },
> +
Re: [PATCH v2 4/4] Move builtin-related implementations to a new builtin.c file
Sebastian Schuberth writes:
> Signed-off-by: Sebastian Schuberth
> ---
> Documentation/technical/api-builtin.txt | 2 +-
> Makefile| 1 +
> builtin.c | 225 ++
> builtin.h | 21 +++
> git.c | 238
>
> 5 files changed, 248 insertions(+), 239 deletions(-)
> create mode 100644 builtin.c
I'm sorry but I do not see a point in this.
It is not like builtin.c can be used outside the context of the main
Git program, and many helper functions you moved out of git.c that
used to be static want to be called from other places.
> diff --git a/Documentation/technical/api-builtin.txt
> b/Documentation/technical/api-builtin.txt
> index e3d6e7a..d1d946c 100644
> --- a/Documentation/technical/api-builtin.txt
> +++ b/Documentation/technical/api-builtin.txt
> @@ -14,7 +14,7 @@ Git:
>
> . Add the external declaration for the function to `builtin.h`.
>
> -. Add the command to the `commands[]` table defined in `git.c`.
> +. Add the command to the `commands[]` table defined in `builtin.c`.
>The entry should look like:
>
> { "foo", cmd_foo, },
> diff --git a/Makefile b/Makefile
> index b4af1e2..2d947e8 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -763,6 +763,7 @@ LIB_OBJS += base85.o
> LIB_OBJS += bisect.o
> LIB_OBJS += blob.o
> LIB_OBJS += branch.o
> +LIB_OBJS += builtin.o
> LIB_OBJS += bulk-checkin.o
> LIB_OBJS += bundle.o
> LIB_OBJS += cache-tree.o
> diff --git a/builtin.c b/builtin.c
> new file mode 100644
> index 000..6bdeb7c
> --- /dev/null
> +++ b/builtin.c
> @@ -0,0 +1,225 @@
> +#include "builtin.h"
> +
> +static struct cmd_struct commands[] = {
> + { "add", cmd_add, RUN_SETUP | NEED_WORK_TREE },
> + { "annotate", cmd_annotate, RUN_SETUP },
> + { "apply", cmd_apply, RUN_SETUP_GENTLY },
> + { "archive", cmd_archive },
> + { "bisect--helper", cmd_bisect__helper, RUN_SETUP },
> + { "blame", cmd_blame, RUN_SETUP },
> + { "branch", cmd_branch, RUN_SETUP },
> + { "bundle", cmd_bundle, RUN_SETUP_GENTLY },
> + { "cat-file", cmd_cat_file, RUN_SETUP },
> + { "check-attr", cmd_check_attr, RUN_SETUP },
> + { "check-ignore", cmd_check_ignore, RUN_SETUP | NEED_WORK_TREE },
> + { "check-mailmap", cmd_check_mailmap, RUN_SETUP },
> + { "check-ref-format", cmd_check_ref_format },
> + { "checkout", cmd_checkout, RUN_SETUP | NEED_WORK_TREE },
> + { "checkout-index", cmd_checkout_index,
> + RUN_SETUP | NEED_WORK_TREE},
> + { "cherry", cmd_cherry, RUN_SETUP },
> + { "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },
> + { "clean", cmd_clean, RUN_SETUP | NEED_WORK_TREE },
> + { "clone", cmd_clone },
> + { "column", cmd_column, RUN_SETUP_GENTLY },
> + { "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
> + { "commit-tree", cmd_commit_tree, RUN_SETUP },
> + { "config", cmd_config, RUN_SETUP_GENTLY },
> + { "count-objects", cmd_count_objects, RUN_SETUP },
> + { "credential", cmd_credential, RUN_SETUP_GENTLY },
> + { "describe", cmd_describe, RUN_SETUP },
> + { "diff", cmd_diff },
> + { "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE },
> + { "diff-index", cmd_diff_index, RUN_SETUP },
> + { "diff-tree", cmd_diff_tree, RUN_SETUP },
> + { "fast-export", cmd_fast_export, RUN_SETUP },
> + { "fetch", cmd_fetch, RUN_SETUP },
> + { "fetch-pack", cmd_fetch_pack, RUN_SETUP },
> + { "fmt-merge-msg", cmd_fmt_merge_msg, RUN_SETUP },
> + { "for-each-ref", cmd_for_each_ref, RUN_SETUP },
> + { "format-patch", cmd_format_patch, RUN_SETUP },
> + { "fsck", cmd_fsck, RUN_SETUP },
> + { "fsck-objects", cmd_fsck, RUN_SETUP },
> + { "gc", cmd_gc, RUN_SETUP },
> + { "get-tar-commit-id", cmd_get_tar_commit_id },
> + { "grep", cmd_grep, RUN_SETUP_GENTLY },
> + { "hash-object", cmd_hash_object },
> + { "help", cmd_help },
> + { "index-pack", cmd_index_pack, RUN_SETUP_GENTLY },
> + { "init", cmd_init_db },
> + { "init-db", cmd_init_db },
> + { "log", cmd_log, RUN_SETUP },
> + { "ls-files", cmd_ls_files, RUN_SETUP },
> + { "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
> + { "ls-tree", cmd_ls_tree, RUN_SETUP },
> + { "mailinfo", cmd_mailinfo },
> + { "mailsplit", cmd_mailsplit },
> + { "merge", cmd_merge, RUN_SETUP | NEED_WORK_TREE },
> + { "merge-base", cmd_merge_base, RUN_SETUP },
> + { "merge-file", cmd_merge_file, RUN_SETUP_GENTLY },
> + { "merge-index", cmd_merge_index, RUN_SETUP },
> + { "merge-ours", cmd_merge_ours, RUN_SETUP },
> + { "merge-recursive", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
> + { "merge-recursive-ours", cmd_merge_recursive, RUN_SETUP |
> NEED_WORK_TREE },
> + { "merge-recursive-theirs", cmd_merge_recursive, RUN_SETUP |
> NEED_WORK_TREE },
> +
Re: [PATCH] Fix safe_create_leading_directories() for Windows
Johannes Schindelin writes: > Hi, > > On Thu, 2 Jan 2014, Sebastian Schuberth wrote: > >> See https://github.com/msysgit/git/pull/80. > > Thanks Sebastian! > > However, since the git.git project is not comfortable with the concept of > pull requests (which is why you submitted this patch via mail), I believe > that we have to explain the rationale in the commit message. So here goes > my attempt: Thanks; the conclusion is correct --- you need a good commit message in the recorded history. That does not have anything to do with integrating with pulling from subsystem maintainers, which we regularly do. > On Linux, we can get away with assuming that the directory separator is a > forward slash, but that is wrong in general. For that purpose, the > is_dir_sep() function was introduced a long time ago. By using it in > safe_create_leading_directories(), we proof said function for use on > platforms where the directory separator is different from Linux'. Perhaps with s|Linux|POSIX|, but more importantly, was there a specific error scenario that triggered this change? My quick reading of "git grep" suggests that the callsites of this function all assume that they are to use slashes as directory separators, and it may be that it is a bug in the specific caller that throws a backslash-separated paths to it. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] Introduce git submodule add|update --attach
Francesco Pretto writes: > by default "git submodule" performs its add or update operations on a detached > HEAD. This works well when using an existing full-fledged/indipendent project > as > the submodule, as there's less frequent need to update it or commit back > changes. When the submodule is actually a large portion of shareable code > between different projects, and the superproject needs to track very closely > the evolution of the submodule (or the other way around), I feel more > confortable > to reattach the HEAD of the submodule with an existing branch. I may be missing some fundamental assumption in your mind when you did this change, but in a workflow where somebody wants submodule checkout to be on branches (as opposed to detached), wouldn't it make more sense not to detach in the first place, rather than introducing yet another option to "re-attach"? The documentation of "submodule update" seems to say that its "merge" and "rebase" modes do not detach in the first place (and it alludes to "--checkout" but it is unclear what it does purely from the documentation, as "git submodule --help" does not even list it as one of the options). And if there is a good reason why detaching to update and then (perhaps after verifying the result or cleaning it up? I dunno what the expected use case is, so I am purely guessing) attaching the result to a specific branch in separate steps, does it make sense to give "--attach" option to "update" in the first place? That makes the whole thing into a single step, not giving the user a chance to do anything in between, which I am guessing is the whole point of your not using the existing "do not detach, work on a branch" modes. Puzzled... -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] blame: new option to better handle merged cherry-picks
"Bernhard R. Link" writes:
> Allows to disable the git blame optimization of assuming that if there is a
> parent of a merge commit that has the exactly same file content, then
> only this parent is to be looked at.
I think this is what we usually call --full-history in "git log"
family, but more importantly, I do not think this is solving a valid
problem.
> This optimization, while being faster in the usual case, means that in
> the case of cherry-picks the blamed commit depends on which other commits
> touched a file.
>
> If for example one commit A modified both files b and c. And there are
> commits B and C, B only modifies file b and C only modifies file c
> (so that no conflicts happen), and assume A is cherry-picked as A'
> and the two branches then merged:
>
> --o-B---A
>\ \
> ---C---A'--M---
So the contents of b at M is as the same as in A, so following 'b'
will see A and B changed that path, which is correct.
The contents of c at M is? It is different from A because at A c
lacks the change made to it at C. The merged result at M would
match C in A', no? So following 'c' will see A' and C changed that
path, no?
So what is wrong about it? If the original history were like this
instead, and A' were a cherry-pick of A, then what should happen?
> --o-B---A'
>\ \
> ---C---A---M---
Don't we want to see c blamed the same way?
Also, when handling a merge, we have to handle parents sequencially,
checking the difference between M with its first parent first, and
then passing blame for the remaining common lines to the remaining
parents. If you flip the order of parents of M when you merge A and
A' in your original history, and with your patch, what would you
see when you blame c? Wouldn't it notice that M:c is identical to c
in its first parent (now A') and pass the whole blame to A' anyway
with or without your change?
> Then without this new option git blame blames the A|A' changes of
> file b to A while blaming the changes of c to A'.
> With the new option --no-parent-shortcut it blames both changes to the
> same commit.
>
> Signed-off-by: Bernhard R. Link
> ---
> Documentation/blame-options.txt | 6 ++
> builtin/blame.c | 5 -
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
> index 0cebc4f..55dd12b 100644
> --- a/Documentation/blame-options.txt
> +++ b/Documentation/blame-options.txt
> @@ -48,6 +48,12 @@ include::line-range-format.txt[]
> Show the result incrementally in a format designed for
> machine consumption.
>
> +--no-parent-shortcut::
> + Always look at all parents of a merge and do not shortcut
> + to the first parent with no changes to the file looked at.
> + This takes more time but produces more reliable results
> + if branches with cherry-picked commits were merged.
> +
> --encoding=::
> Specifies the encoding used to output author names
> and commit summaries. Setting it to `none` makes blame
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 4916eb2..dab2c36 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -45,6 +45,7 @@ static int incremental;
> static int xdl_opts;
> static int abbrev = -1;
> static int no_whole_file_rename;
> +static int no_parent_shortcut;
>
> static enum date_mode blame_date_mode = DATE_ISO8601;
> static size_t blame_date_width;
> @@ -1248,7 +1249,8 @@ static void pass_blame(struct scoreboard *sb, struct
> origin *origin, int opt)
> porigin = find(sb, p, origin);
> if (!porigin)
> continue;
> - if (!hashcmp(porigin->blob_sha1, origin->blob_sha1)) {
> + if (!no_parent_shortcut &&
> + !hashcmp(porigin->blob_sha1, origin->blob_sha1)) {
> pass_whole_blame(sb, origin, porigin);
> origin_decref(porigin);
> goto finish;
> @@ -2247,6 +2249,7 @@ int cmd_blame(int argc, const char **argv, const char
> *prefix)
> static const char *contents_from = NULL;
> static const struct option options[] = {
> OPT_BOOL(0, "incremental", &incremental, N_("Show blame entries
> as we find them, incrementally")),
> + OPT_BOOL(0, "no-parent-shortcut", &no_parent_shortcut,
> N_("Don't take shortcuts in some merges but handle cherry-picks better")),
> OPT_BOOL('b', NULL, &blank_boundary, N_("Show blank SHA-1 for
> boundary commits (Default: off)")),
> OPT_BOOL(0, "root", &show_root, N_("Do not treat root commits
> as boundaries (Default: off)")),
> OPT_BOOL(0, "show-stats", &show_stats, N_("Show work cost
> statistics")),
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo i
Re: [PATCH v2 1/4] Consistently use the term "builtin" instead of "internal command"
Hi,
Sebastian Schuberth wrote:
[...]
> --- a/Documentation/technical/api-builtin.txt
> +++ b/Documentation/technical/api-builtin.txt
> @@ -14,7 +14,7 @@ Git:
>
> . Add the external declaration for the function to `builtin.h`.
>
> -. Add the command to `commands[]` table in `handle_internal_command()`,
> +. Add the command to `commands[]` table in `handle_builtin()`,
Makes sense. Using consistent jargon makes for easier reading.
[...]
> +++ b/git.c
[...]
> @@ -563,14 +563,14 @@ int main(int argc, char **av)
[...]
> if (starts_with(cmd, "git-")) {
> cmd += 4;
> argv[0] = cmd;
> - handle_internal_command(argc, argv);
> + handle_builtin(argc, argv);
> - die("cannot handle %s internally", cmd);
> + die("cannot handle %s as a builtin", cmd);
I think this makes the user-visible message less clear.
Before when the user had a stale git-whatever link lingering in
gitexecdir, git would say
fatal: cannot handle whatever internally
which tells me git was asked to handle the whatever command internally
and was unable to. Afterward, it becomes
fatal: cannot handle whatever as a builtin
which requires that I learn the jargon use of "builtin" as a noun.
busybox's analogous message is "applet not found". It's less likely
to come up when using git because it requires having a stray link to
"git". A message like
$ git whatever
fatal: whatever: no such built-in command
would just leave me wondering "I never claimed it was built-in; what's
going on?" I think it would be simplest to keep it as
$ git whatever
fatal: cannot handle "whatever" internally
which at least makes it clear that this is a low-level error.
The rest of the patch looks good.
Thanks,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix safe_create_leading_directories() for Windows
On 02.01.2014 19:18, John Keeping wrote: That said, I see any further explanations on top of the commit message title is an added bonus, and as "just" a bonus a link to a pull request should be fine. You don't need to understand or appreciate the concept of pull requests in order to follow the link and read the text in there. The commit message serves as an historical record of why a change was made; depending on an external service to provide this information when it can quite easily be included in the commit itself lessens the value of the commit message. Sure. My point just was that IMHO the commit does not *depend* on the information provided in the link; for me the commit was simply self-evident, and I just added link as optional information, not to replace any inline text that I would have written otherwise. -- Sebastian Schuberth -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix safe_create_leading_directories() for Windows
Hi Junio, On Thu, 2 Jan 2014, Junio C Hamano wrote: > Johannes Schindelin writes: > > > On Thu, 2 Jan 2014, Sebastian Schuberth wrote: > > > >> See https://github.com/msysgit/git/pull/80. > > > > Thanks Sebastian! > > > > However, since the git.git project is not comfortable with the concept > > of pull requests (which is why you submitted this patch via mail), I > > believe that we have to explain the rationale in the commit message. > > So here goes my attempt: > > Thanks; the conclusion is correct --- you need a good commit message in > the recorded history. That does not have anything to do with > integrating with pulling from subsystem maintainers, which we regularly > do. I should have pointed out that I was talking about the concept of pull requests, not the concept of accepting pull requests from dedicated subsystem maintainers. > > On Linux, we can get away with assuming that the directory separator > > is a forward slash, but that is wrong in general. For that purpose, > > the is_dir_sep() function was introduced a long time ago. By using it > > in safe_create_leading_directories(), we proof said function for use > > on platforms where the directory separator is different from Linux'. > > Perhaps with s|Linux|POSIX|, No, for two reasons: * OpenVMS supports POSIX, yes? And OpenVMS does not have the forward slash as directory separator but instead the dot, yes? * it would be dishonest. The reason is not that we looked at the POSIX standard when we determined how to implement safe_create_leading_directories(), but at Linux. > but more importantly, was there a specific error scenario that triggered > this change? Yes, the concrete use case that triggered the pull request whose change we did not accept but instead would prefer Sebastian's patch is where a user called git clone C:\directory in cmd.exe. Ciao, Johannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix safe_create_leading_directories() for Windows
Hi Sebastian, On Thu, 2 Jan 2014, Sebastian Schuberth wrote: > On 02.01.2014 18:33, Johannes Schindelin wrote: > > > -- snip -- > > On Linux, we can get away with assuming that the directory separator is a > > forward slash, but that is wrong in general. For that purpose, the > > is_dir_sep() function was introduced a long time ago. By using it in > > safe_create_leading_directories(), we proof said function for use on > > platforms where the directory separator is different from Linux'. > > -- snap -- > > While I'd be fine with this, I do not think we really need it. I also would have been fine with your commit message. But I knew Junio wouldn't be. > As you say, is_dir_sep() has been introduced a long time ago, so people > should be aware of it, and it should also be immediately clear from the > diff why using it is better than hard-coding '/'. > > That said, I see any further explanations on top of the commit message > title is an added bonus, and as "just" a bonus a link to a pull request > should be fine. You don't need to understand or appreciate the concept > of pull requests in order to follow the link and read the text in there. Well, you and I both know how easy GitHub's pull request made things for us as well as for contributors. I really cannot thank Erik enough for bullying me into using and accepting them. But the commit messages of the merged pull requests are not exactly stellar when you want the repositories to be self-contained, which is however how I remember things are expected in git.git. Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Dec 2013, #05; Thu, 26)
On Fri, Dec 27, 2013 at 5:13 PM, Junio C Hamano wrote: > Eric Sunshine writes: > >> On Thu, Dec 26, 2013 at 4:08 PM, Junio C Hamano wrote: >>> [New Topics] >> >> Would $gmane/239575 [1] be of interest for "New Topics"? >> >> [1]: http://article.gmane.org/gmane.comp.version-control.git/239575/ > > Actually I was planning to scoop it up directly to master but forgot > to do so. Make sense. > Running "git diff maint pu -- name-hash.c" shows that we have added > a comment that mentions index_name_exists---that needs to be > adjusted, too, by the way. Oops, yes, I had noticed that too when testing atop 'pu' but then forgot about it when preparing the patch for submission on 'master'. I'm not sure how to move forward with this now that kb/fast-hashmap, with which it has a textual conflict, has graduated to 'next'. Should this become a two-patch series with one for scooping directly to 'master' and one for 'next' to sit atop kb/fast-hashmap? (But how will the textual conflict be handled?) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix safe_create_leading_directories() for Windows
On 02.01.2014 20:55, Junio C Hamano wrote: > Thanks; the conclusion is correct --- you need a good commit > message in the recorded history. That does not have anything to do > with integrating with pulling from subsystem maintainers, which we > regularly do. I'll send a v2 which adds a proper commits message inline. > Perhaps with s|Linux|POSIX|, but more importantly, was there a > specific error scenario that triggered this change? Yes, cloning to a non-existent directory with Windows paths fails like: fatal: could not create work tree dir 'c:\foo\bar'.: No such file or directory Seems like the path to clone to is taken as-is from argv in cmd_clone(). So maybe another solution would be to replace all backslashes with forward slashes already there? -- Sebastian Schuberth -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] Fix safe_create_leading_directories() for Windows
When cloning to a directory "C:\foo\bar" from Windows' cmd.exe where "foo"
does not exist yet, Git would throw an error like
fatal: could not create work tree dir 'c:\foo\bar'.: No such file or directory
Fix this by not hard-coding a platform specific directory separator into
safe_create_leading_directories().
Signed-off-by: Johannes Schindelin
Signed-off-by: Sebastian Schuberth
---
sha1_file.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 760dd60..2114c58 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -110,12 +110,15 @@ int safe_create_leading_directories(char *path)
char *pos = path + offset_1st_component(path);
struct stat st;
- while (pos) {
- pos = strchr(pos, '/');
- if (!pos)
- break;
- while (*++pos == '/')
- ;
+ while (*pos) {
+ while (!is_dir_sep(*pos)) {
+ ++pos;
+ if (!*pos)
+ break;
+ }
+ /* skip consecutive directory separators */
+ while (is_dir_sep(*pos))
+ ++pos;
if (!*pos)
break;
*--pos = '\0';
--
1.8.4.msysgit.0.1.ge705bba.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] Move builtin-related implementations to a new builtin.c file
On Thu, Jan 2, 2014 at 8:43 PM, Junio C Hamano wrote: >> Documentation/technical/api-builtin.txt | 2 +- >> Makefile| 1 + >> builtin.c | 225 ++ >> builtin.h | 21 +++ >> git.c | 238 >> >> 5 files changed, 248 insertions(+), 239 deletions(-) >> create mode 100644 builtin.c > > I'm sorry but I do not see a point in this. > > It is not like builtin.c can be used outside the context of the main > Git program, and many helper functions you moved out of git.c that > used to be static want to be called from other places. I've added this commit because Christian suggested so in [1], and also because it has always bothered me that the Git project does not define a function in a file named after the header file that declares the function. Anyway, I've made this the last commit in the series on purpose, I can just drop it in the next re-roll. [1] http://www.spinics.net/lists/git/msg222452.html. -- Sebastian Schuberth -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] Consistently use the term "builtin" instead of "internal command"
On Thu, Jan 2, 2014 at 9:31 PM, Jonathan Nieder wrote: > would just leave me wondering "I never claimed it was built-in; what's > going on?" I think it would be simplest to keep it as > > $ git whatever > fatal: cannot handle "whatever" internally > > which at least makes it clear that this is a low-level error. Right, I'll change this in a re-roll (using single-quotes for the command name). > The rest of the patch looks good. Thanks for the review. -- Sebastian Schuberth -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix safe_create_leading_directories() for Windows
Sebastian Schuberth writes: > On 02.01.2014 20:55, Junio C Hamano wrote: > >> Thanks; the conclusion is correct --- you need a good commit >> message in the recorded history. That does not have anything to do >> with integrating with pulling from subsystem maintainers, which we >> regularly do. > > I'll send a v2 which adds a proper commits message inline. > >> Perhaps with s|Linux|POSIX|, but more importantly, was there a >> specific error scenario that triggered this change? > > Yes, cloning to a non-existent directory with Windows paths fails like: > > fatal: could not create work tree dir 'c:\foo\bar'.: No such file or directory OK. That was why I wanted to see (and Dscho correctly guessed) a good description in the proposed log message to see what problem the change is trying to address, so that we can judge if the change is tackling the right problem. > Seems like the path to clone to is taken as-is from argv in > cmd_clone(). So maybe another solution would be to replace all > backslashes with forward slashes already there? That sounds like a workable alternative, and it might even be a preferable solution but if and only if clone's use of the function to create paths that lead to a new working tree location is the only (ab)use of the function. That was what I meant when I said "it may be that it is a bug in the specific caller". AFAIK, the function was meant to be used only on paths we internally generate, and the paths we internally generate all are slash separated, in line with how paths are stored in the index. If we are going to change the meaning of the function so that it can now take any random path in platform-specific convention that may be incompatible with the internal notion of paths Git has (i.e. what is passed to safe_create_leading_directories() may have to be massaged into a slash-separated form before it can be used in the index and parsed to be stuffed into trees), it is fine to do so as long as all the codepaths understands the new world order, but my earlier "git grep" hits did not tell me that such a change is warranted. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Fix safe_create_leading_directories() for Windows
Hi, On Thu, 2 Jan 2014, Sebastian Schuberth wrote: > When cloning to a directory "C:\foo\bar" from Windows' cmd.exe where "foo" > does not exist yet, Git would throw an error like > > fatal: could not create work tree dir 'c:\foo\bar'.: No such file or directory > > Fix this by not hard-coding a platform specific directory separator into > safe_create_leading_directories(). > > Signed-off-by: Johannes Schindelin > Signed-off-by: Sebastian Schuberth It would be nice to have links to the original discussion, but I guess that that would not be accepted. Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Dec 2013, #05; Thu, 26)
Eric Sunshine writes: > On Fri, Dec 27, 2013 at 5:13 PM, Junio C Hamano wrote: >> Eric Sunshine writes: >> >>> On Thu, Dec 26, 2013 at 4:08 PM, Junio C Hamano wrote: [New Topics] >>> >>> Would $gmane/239575 [1] be of interest for "New Topics"? >>> >>> [1]: http://article.gmane.org/gmane.comp.version-control.git/239575/ >> >> Actually I was planning to scoop it up directly to master but forgot >> to do so. > > Make sense. > >> Running "git diff maint pu -- name-hash.c" shows that we have added >> a comment that mentions index_name_exists---that needs to be >> adjusted, too, by the way. > > Oops, yes, I had noticed that too when testing atop 'pu' but then > forgot about it when preparing the patch for submission on 'master'. > > I'm not sure how to move forward with this now that kb/fast-hashmap, > with which it has a textual conflict, has graduated to 'next'. Should > this become a two-patch series with one for scooping directly to > 'master' and one for 'next' to sit atop kb/fast-hashmap? (But how will > the textual conflict be handled?) I have a feeling that a small unused helper function is not a huge breakage that needs to be immediately fixed, so a single patch as a clean-up on top of whatever is cooking on 'next' should be the best approach, I would think. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] blame: new option to better handle merged cherry-picks
* Junio C Hamano [140102 21:29]: > > This optimization, while being faster in the usual case, means that in > > the case of cherry-picks the blamed commit depends on which other commits > > touched a file. > > > > If for example one commit A modified both files b and c. And there are > > commits B and C, B only modifies file b and C only modifies file c > > (so that no conflicts happen), and assume A is cherry-picked as A' > > and the two branches then merged: > > > > --o-B---A > >\ \ > > ---C---A'--M--- > > So the contents of b at M is as the same as in A, so following 'b' > will see A and B changed that path, which is correct. > > The contents of c at M is? It is different from A because at A c > lacks the change made to it at C. The merged result at M would > match C in A', no? So following 'c' will see A' and C changed that > path, no? > > So what is wrong about it? It's not wrong (that's why I do not suggest to change the default behaviour), but it's inconsistent and can be a bit confusing to have either the one or the other commit blamed depending on whether some file was touched or not. The history I'm a bit more concerned is something like (with ... being unrelated commits not touching B or C): --o-...---A--...---B---...-- \\ ---...---A'--...---C---...---M--- Here having B or C touching b or c determines which of A or A' is blamed for which part of the patch. It's even enough to have: --...---A'--...---B---...-- /\ ---o---...---A-----M--- To have the A/A' changes of c to be attributed to A while the b changes are attributed to A'. I.e. you have a master branch that has commit A, which is also cherry-picked to some previously forked side-branch. Once that side-branch is merged back, parts of the change are attributed to A' if they are in a file that is not touched otherwise in the main branch. > Also, when handling a merge, we have to handle parents sequencially, > checking the difference between M with its first parent first, and > then passing blame for the remaining common lines to the remaining > parents. If you flip the order of parents of M when you merge A and > A' in your original history, and with your patch, what would you > see when you blame c? Wouldn't it notice that M:c is identical to c > in its first parent (now A') and pass the whole blame to A' anyway > with or without your change? When giving git-blame the new option introduced with my patch, only the order of parents determines which commit is blamed. Without the option (i.e. the currently only possible behaviour) which commit is blamed depends what else touches other parts of the file. If both branches make modifications to the file (or if there is any merge conflict resolution in the merge) then the bahaviour with or without the option are the same. But in the example with one commit B touching also b and one commit C touching also c, there is (without the new option) always one part of the cherry-picked commit is blamed on the original and one on the cherry-picked, no matter how you order the parents. (While by having your mainline always the most leftward parent, with the the new option you always get those commit blamed that is the "first one this was introduced to mainline".) Bernhard R. Link -- F8AC 04D5 0B9B 064B 3383 C3DA AFFC 96D1 151D FFDC -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix safe_create_leading_directories() for Windows
Hi Junio, On Thu, 2 Jan 2014, Junio C Hamano wrote: > If we are going to change the meaning of the function so that it can > now take any random path in platform-specific convention Note that nothing in the function name or documentation suggests otherwise. > that may be incompatible with the internal notion of paths Git has (i.e. > what is passed to safe_create_leading_directories() may have to be > massaged into a slash-separated form before it can be used in the index The safe_create_leading_directories() function never interacts with the index, so you find me quite puzzled as to your objection. > and parsed to be stuffed into trees), it is fine to do so as long as all > the codepaths understands the new world order, but my earlier "git grep" > hits did not tell me that such a change is warranted. You call safe_create_leading_directories() with an argument that is supposed to be the final path, correct? So what exactly is wrong with safe_create_leading_directories() creating all the directories necessary so that we can write to the path afterwards, *even* if that path is interpreted in a platform-dependent manner (as one would actually expect it to)? Last time I checked we did not make a fuss about safe_create_leading_directories() interpreting the argument in a case-insensitive fashion on certain setups, either. So it is not exactly a new thing that the paths are interpreted in a platform-dependent manner. Ciao, Johannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Fix safe_create_leading_directories() for Windows
Sebastian Schuberth writes:
> When cloning to a directory "C:\foo\bar" from Windows' cmd.exe where "foo"
> does not exist yet, Git would throw an error like
>
> fatal: could not create work tree dir 'c:\foo\bar'.: No such file or directory
>
> Fix this by not hard-coding a platform specific directory separator into
> safe_create_leading_directories().
>
> Signed-off-by: Johannes Schindelin
> Signed-off-by: Sebastian Schuberth
> ---
> sha1_file.c | 15 +--
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 760dd60..2114c58 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -110,12 +110,15 @@ int safe_create_leading_directories(char *path)
> char *pos = path + offset_1st_component(path);
> struct stat st;
>
> - while (pos) {
> - pos = strchr(pos, '/');
> - if (!pos)
> - break;
> - while (*++pos == '/')
> - ;
> + while (*pos) {
> + while (!is_dir_sep(*pos)) {
> + ++pos;
> + if (!*pos)
> + break;
> + }
> + /* skip consecutive directory separators */
> + while (is_dir_sep(*pos))
> + ++pos;
> if (!*pos)
> break;
> *--pos = '\0';
This has a bit of conflict with another topic in flight; I think I
resolved it correctly, but please double check. The following is
how it would apply on top of 'pu'.
sha1_file.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 131ca97..9e686eb 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -113,11 +113,12 @@ int safe_create_leading_directories(char *path)
while (!retval && next_component) {
struct stat st;
- char *slash = strchr(next_component, '/');
-
- if (!slash)
+ char *slash = next_component;
+ while (!is_dir_sep(*slash))
+ ++slash;
+ if (!*slash)
return 0;
- while (*(slash + 1) == '/')
+ while (is_dir_sep(*(slash + 1)))
slash++;
next_component = slash + 1;
if (!*next_component)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Dec 2013, #05; Thu, 26)
On Thu, Jan 2, 2014 at 4:11 PM, Junio C Hamano wrote: > Eric Sunshine writes: > >> On Fri, Dec 27, 2013 at 5:13 PM, Junio C Hamano wrote: >>> Eric Sunshine writes: >>> On Thu, Dec 26, 2013 at 4:08 PM, Junio C Hamano wrote: > [New Topics] Would $gmane/239575 [1] be of interest for "New Topics"? [1]: http://article.gmane.org/gmane.comp.version-control.git/239575/ >>> >>> Actually I was planning to scoop it up directly to master but forgot >>> to do so. >> >> Make sense. >> >>> Running "git diff maint pu -- name-hash.c" shows that we have added >>> a comment that mentions index_name_exists---that needs to be >>> adjusted, too, by the way. >> >> Oops, yes, I had noticed that too when testing atop 'pu' but then >> forgot about it when preparing the patch for submission on 'master'. >> >> I'm not sure how to move forward with this now that kb/fast-hashmap, >> with which it has a textual conflict, has graduated to 'next'. Should >> this become a two-patch series with one for scooping directly to >> 'master' and one for 'next' to sit atop kb/fast-hashmap? (But how will >> the textual conflict be handled?) > > I have a feeling that a small unused helper function is not a huge > breakage that needs to be immediately fixed, so a single patch as a > clean-up on top of whatever is cooking on 'next' should be the best > approach, I would think. Sounds good. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] blame: new option to better handle merged cherry-picks
"Bernhard R. Link" writes: > When giving git-blame the new option introduced with my patch, only > the order of parents determines which commit is blamed. Without > the option (i.e. the currently only possible behaviour) which commit > is blamed depends what else touches other parts of the file. I am trying to figure out why that difference matters, in other words, when using the new option is actually useful. You give the command a scenario that can be solved in two equally valid ways (blaming to either A or A' is equally valid), and sometimes the command gives the identical result with or without the new option, and some other times the user gets a different but an equally valid result (but after traversing more history spending more cycles). I am not sure what problem the new option solves. I am trying to come up with an easy-to-understand explanation to the end users: "If you want to see blame's result with the property X, use this option---it may have to spend extra cycles, but the property X is so desirable that it may be worth it". And I am having a hard time understanding what that X is. > But in the example with one commit B touching also b and one commit C > touching also c, there is (without the new option) always one part > of the cherry-picked commit is blamed on the original and one on the > cherry-picked, no matter how you order the parents. Yeah, the cherry-picked one will introduce the same change as the one that was cherry-picked, so if you look at the end result and ask "where did _this_ line come from?", there are two equally plausible candidates, as "blame" output can give only one answer to each line. I still do not see why the one that is picked with the new option is better. At best, it looks to me that it is saying "running with this option may (or may not) give a different answer, so run the command with and without it and see which one you like", which does not sound too useful to the end users. That is where my confusion comes from. > (While by having your mainline always the most leftward parent, with > the the new option you always get those commit blamed that is the > "first one this was introduced to mainline".) Yes, I vaguely recall we talked about adding --first-parent option to the command in the past. I do not remember what came out of that discussion. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Fix safe_create_leading_directories() for Windows
Junio C Hamano writes:
> This has a bit of conflict with another topic in flight; I think I
> resolved it correctly, but please double check. The following is
> how it would apply on top of 'pu'.
>
> sha1_file.c | 9 +
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 131ca97..9e686eb 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -113,11 +113,12 @@ int safe_create_leading_directories(char *path)
>
> while (!retval && next_component) {
> struct stat st;
> - char *slash = strchr(next_component, '/');
> -
> - if (!slash)
> + char *slash = next_component;
> + while (!is_dir_sep(*slash))
Gaah; we need to check for the end of string here, i.e.
while (*slash && !is_dir_sep(*slash))
will be what I'll queue on 'pu' for today.
> + ++slash;
> + if (!*slash)
> return 0;
> - while (*(slash + 1) == '/')
> + while (is_dir_sep(*(slash + 1)))
> slash++;
> next_component = slash + 1;
> if (!*next_component)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] name-hash: retire unused index_name_exists()
db5360f3f496 (name-hash: refactor polymorphic index_name_exists();
2013-09-17) split index_name_exists() into index_file_exists() and
index_dir_exists() but retained index_name_exists() as a thin wrapper
to avoid disturbing possible in-flight topics. Since this change
landed in 'master' some time ago and there are no in-flight topics
referencing index_name_exists(), retire it.
Signed-off-by: Eric Sunshine
---
The only difference from v1 [1] is that a comment added by
kb/fast-hashmap in 'next' referencing obsolete index_name_exists()
is also adjusted.
[1]: http://article.gmane.org/gmane.comp.version-control.git/239575/
cache.h | 2 --
name-hash.c | 9 +
2 files changed, 1 insertion(+), 10 deletions(-)
diff --git a/cache.h b/cache.h
index 2a21fbc..d0d1f2b 100644
--- a/cache.h
+++ b/cache.h
@@ -316,7 +316,6 @@ extern void free_name_hash(struct index_state *istate);
#define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st),
(options))
#define cache_dir_exists(name, namelen) index_dir_exists(&the_index, (name),
(namelen))
#define cache_file_exists(name, namelen, igncase)
index_file_exists(&the_index, (name), (namelen), (igncase))
-#define cache_name_exists(name, namelen, igncase)
index_name_exists(&the_index, (name), (namelen), (igncase))
#define cache_name_is_other(name, namelen) index_name_is_other(&the_index,
(name), (namelen))
#define resolve_undo_clear() resolve_undo_clear_index(&the_index)
#define unmerge_cache_entry_at(at) unmerge_index_entry_at(&the_index, at)
@@ -466,7 +465,6 @@ extern int unmerged_index(const struct index_state *);
extern int verify_path(const char *path);
extern struct cache_entry *index_dir_exists(struct index_state *istate, const
char *name, int namelen);
extern struct cache_entry *index_file_exists(struct index_state *istate, const
char *name, int namelen, int igncase);
-extern struct cache_entry *index_name_exists(struct index_state *istate, const
char *name, int namelen, int igncase);
extern int index_name_pos(const struct index_state *, const char *name, int
namelen);
#define ADD_CACHE_OK_TO_ADD 1 /* Ok to add */
#define ADD_CACHE_OK_TO_REPLACE 2 /* Ok to replace file/directory */
diff --git a/name-hash.c b/name-hash.c
index 9a3bd3f..97444d0 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -115,7 +115,7 @@ static int cache_entry_cmp(const struct cache_entry *ce1,
{
/*
* For remove_name_hash, find the exact entry (pointer equality); for
-* index_name_exists, find all entries with matching hash code and
+* index_file_exists, find all entries with matching hash code and
* decide whether the entry matches in same_name.
*/
return remove ? !(ce1 == ce2) : 0;
@@ -227,13 +227,6 @@ struct cache_entry *index_file_exists(struct index_state
*istate, const char *na
return NULL;
}
-struct cache_entry *index_name_exists(struct index_state *istate, const char
*name, int namelen, int icase)
-{
- if (namelen > 0 && name[namelen - 1] == '/')
- return index_dir_exists(istate, name, namelen - 1);
- return index_file_exists(istate, name, namelen, icase);
-}
-
void free_name_hash(struct index_state *istate)
{
if (!istate->name_hash_initialized)
--
1.8.3.2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Fix safe_create_leading_directories() for Windows
Junio C Hamano writes:
> Junio C Hamano writes:
>
>> This has a bit of conflict with another topic in flight; I think I
>> resolved it correctly, but please double check. The following is
>> how it would apply on top of 'pu'.
>>
>> sha1_file.c | 9 +
>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/sha1_file.c b/sha1_file.c
>> index 131ca97..9e686eb 100644
>> --- a/sha1_file.c
>> +++ b/sha1_file.c
>> @@ -113,11 +113,12 @@ int safe_create_leading_directories(char *path)
>>
>> while (!retval && next_component) {
>> struct stat st;
>> -char *slash = strchr(next_component, '/');
>> -
>> -if (!slash)
>> +char *slash = next_component;
>> +while (!is_dir_sep(*slash))
>
> Gaah; we need to check for the end of string here, i.e.
>
> while (*slash && !is_dir_sep(*slash))
>
> will be what I'll queue on 'pu' for today.
>
>> +++slash;
>> +if (!*slash)
>> return 0;
>> -while (*(slash + 1) == '/')
>> +while (is_dir_sep(*(slash + 1)))
>> slash++;
>> next_component = slash + 1;
>> if (!*next_component)
Another thing I noticed (but I won't fix it up myself today, as I am
deep into today's integration cycle already) is that we temporarily
replace the slash with NUL and then restore them before we return,
but the restoration is done with the slash. If we were to go in the
direction of this patch, you may want to update that one to use
whatever dir-sep was used in the input string.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] t0000 cleanups
Jeff King wrote: > On Mon, Dec 30, 2013 at 10:51:25AM -0800, Jonathan Nieder wrote: >> These scratch areas for sub-tests should be under the t >> trash directory, but because the TEST_OUTPUT_DIRECTORY >> setting from the toplevel test leaks [...] > This is not exactly true. The TEST_OUTPUT_DIRECTORY setting does not > leak. t sets $TEST_DIRECTORY (which it must, so the sub-scripts can > find test-lib.sh and friends), and then TEST_OUTPUT_DIRECTORY uses that > as a default if it is not explicitly set. So I should have said something like the following instead: These scratch areas for sub-tests should be under the t trash directory, but because TEST_OUTPUT_DIRECTORY defaults to TEST_DIRECTORY which is exported to help sub-tests find test-lib.sh, the sub-test trash directories are created under the toplevel t/ directory instead. Because some of the sub-tests simulate failures, their trash directories are kept around. Fix it by explicitly setting TEST_OUTPUT_DIRECTORY appropriately for sub-tests. Thanks for catching it. Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] t0000 cleanups
Jonathan Nieder writes: > Jeff King wrote: >> On Mon, Dec 30, 2013 at 10:51:25AM -0800, Jonathan Nieder wrote: > >>> These scratch areas for sub-tests should be under the t >>> trash directory, but because the TEST_OUTPUT_DIRECTORY >>> setting from the toplevel test leaks > [...] >> This is not exactly true. The TEST_OUTPUT_DIRECTORY setting does not >> leak. t sets $TEST_DIRECTORY (which it must, so the sub-scripts can >> find test-lib.sh and friends), and then TEST_OUTPUT_DIRECTORY uses that >> as a default if it is not explicitly set. > > So I should have said something like the following instead: > > These scratch areas for sub-tests should be under the t trash > directory, but because TEST_OUTPUT_DIRECTORY defaults to > TEST_DIRECTORY which is exported to help sub-tests find test-lib.sh, > the sub-test trash directories are created under the toplevel t/ > directory instead. Because some of the sub-tests simulate failures, > their trash directories are kept around. I had a private rewrite queued already, but the above is easier to read, so I'll replace it with this. Thanks. > > Fix it by explicitly setting TEST_OUTPUT_DIRECTORY appropriately > for sub-tests. > > Thanks for catching it. > > Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] Introduce git submodule add|update --attach
2014/1/2 Junio C Hamano : > Francesco Pretto writes: > >> by default "git submodule" performs its add or update operations on a >> detached >> HEAD. This works well when using an existing full-fledged/indipendent >> project as >> the submodule, as there's less frequent need to update it or commit back >> changes. When the submodule is actually a large portion of shareable code >> between different projects, and the superproject needs to track very closely >> the evolution of the submodule (or the other way around), I feel more >> confortable >> to reattach the HEAD of the submodule with an existing branch. > > I may be missing some fundamental assumption in your mind when you > did this change, but in a workflow where somebody wants submodule > checkout to be on branches (as opposed to detached), wouldn't it > make more sense not to detach in the first place, rather than > introducing yet another option to "re-attach"? The documentation of > "submodule update" seems to say that its "merge" and "rebase" modes > do not detach in the first place (and it alludes to "--checkout" but > it is unclear what it does purely from the documentation, as "git > submodule --help" does not even list it as one of the options). > Thanks for commenting: because of more checking I just spotted some unuseful code in my patch in cmd_add() function. In short: it seems to me "git submodule update" doesn't allow to work with attached an HEAD (unless it is *manually* attached, as I regularly do). My feeling is the documentation of "merge", "rebase" update commands is inaccurate and doesn't really reflect what happen when adding a submodule with "add" or cloning it for the first time with "update". You can look at the following conditionals in the current code in git-submodule.sh: ## cmd_add() case "$branch" in '') git checkout -f -q ;; ?*) git checkout -f -q -B "$branch" "origin/$branch" ;; esac ## cmd_update() # Is this something we just cloned? case ";$cloned_modules;" in *";$name;"*) # then there is no local change to integrate update_module= ;; esac This means that the "add" command will always checkout an *attached* HEAD but at the first clone of a different user the checkout will resolve in "git checkout ", always producing a *detached* HEAD and resulting in inconsistent HEAD state between who added the submodule with "add" and who cloned it with "update". Subsequent "merge" or "rebase" operations won't change this fact, the HEAD will remain detached. The following test case confirms this, unless I did something wrong: - parentdir=$(pwd -P) submodurl1=$(pwd -P)/repo1 submodurl2=$(pwd -P)/repo2 repourl=$(pwd -P)/repo # Create repo to be added with submodules cd $parentdir mkdir repo cd repo git init git config receive.denyCurrentBranch ignore echo a >a git add a git commit -m "repo commit 1" # Create repo repo1 to be used as submodule cd $parentdir mkdir repo1 cd repo1 git init git config receive.denyCurrentBranch ignore echo a >a git add a git commit -m "repo1 commit 1" # Create repo repo2 to be used as submodule cd $parentdir mkdir repo2 cd repo2 git init git config receive.denyCurrentBranch ignore echo a >a git add a git commit -m "repo2 commit 1" # Clone repo to test "git submodule update" cd $parentdir git clone "$repourl" repoclone # ## Adding submodule with update "rebase", not specifying a # cd $parentdir/repo git submodule add "$submodurl1" submod1 git config -f .gitmodules submodule.submod1.ignore all git config -f .gitmodules submodule.submod1.update rebase git commit -m "Added submodule" ## repo/submod1 has an attached HEAD ## cd $parentdir/repoclone git pull git submodule init git submodule update ## repoclone/submod1 has a detached HEAD --> note the inconsistency ## # ## Adding submodule with update "rebase", specifying a # cd $parentdir/repo git submodule add --branch master "$submodurl2" submod2 git config -f .gitmodules submodule.submod2.ignore all git config -f .gitmodules submodule.submod2.update rebase git add . git commit -m "Added submodule" ## repo/submod2 has a attached HEAD ## cd $parentdir/repoclone git pull git submodule init git submodule update --remote ## repoclone/submod2 has a detached HEAD --> note the inconsistency ## # ## Adding something to submod2 and test update "rebase" on repoclone # cd $parentdir/repo1 echo b >b git add b git commit -m "repo1 commit 2" cd $parentdir/repoclone git submodule update --remote ## repoclone/submod1 has still a detached HEAD ## - > And if there is a good reason why detaching to update and then > (perhaps after verifying the result or cleaning it up? I dunno what > the expected use case is, so I am purely guessing) attaching the > result to a specific branch in separate steps, does it make sense to > give "--attach" option to "update" in the first place? Th
Re: [PATCH 2/4] completion: introduce __gitcomp_2 ()
Ramkumar Ramachandra writes:
> There are situations where two classes of completions possible. For
> example
>
> branch.
>
> should try to complete
>
> branch.master.
> branch.autosetupmerge
> branch.autosetuprebase
>
> The first candidate has the suffix ".", and the second/ third candidates
> have the suffix " ". To facilitate completions of this kind, create a
> variation of __gitcomp_nl () that accepts two sets of arguments and two
> independent suffixes.
That sounds like a reasonable issue to address, but I do not quite
get why you need a new helper to do this.
If the original only knows to throw "branch." + branch names +
trailing dot into COMPREPLY[] and does so by calling gitcomp_nl,
isn't it the matter of making another call to gitcomp_nl just after
the existing call to stuff branch.autosetup* with trailing SP to
append them to COMPREPLY[]?
Ahh, is that because the eventual call to __gitcompadd() starts the
iteration starting from zero, essentially forbidding you to
incrementally adding to COMPREPLY[] from multiple callers, even
though it is called comp "add" not "replace with this single thing"?
What I am wondering is if a cleaner solution that can be reused by
later needs that may have more than two data sources (or more than
two suffixes) might be to create a variant of __gitcomp_nl that does
not clear existing entries in COMPREPLY[] array, add a helper to
clear the array, which would make the existing one to:
__gitcomp_nl () {
__gitcomp_clear
__gitcomp_nl_append "$@"
}
and then complete branch.* using two calls to __gitcomp_*, letting
the first one clear and later one(s) accumulate:
__gitcomp_nl "$(__git_heads)" "$pfx" "$cur_" "."
__gitcomp_nl_append $"autosetupmerge\nautosetuprebase\n" "$pfx" "$cur_"
" "
Will queue as-is.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] completion: fix branch.autosetup(merge|rebase)
Ramkumar Ramachandra writes:
> branch.*.*)
> local pfx="${cur%.*}." cur_="${cur##*.}"
> + if [ "$pfx" == "branch.autosetupmerge." ] ||
> + [ "$pfx" == "branch.autosetuprebase." ]; then
> + return
> + fi
> __gitcomp "remote pushremote merge mergeoptions rebase" "$pfx"
> "$cur_"
> return
> ;;
I do not quite understand this change.
If we are looking at "branch.autosetupmerge." followed by something,
who typed that final dot? If you are working on a topic about
auto-setup-merge and named your branch "autosetupmerge", don't you
want to be able to configure various aspect of that branch via
branch.autosetupmerge.{remote,merge} etc., just like you can do so
for your "topic" branch via branch.topic.{remote,merge} etc.,
regardless of your use of "autosetupmerge" option across branches?
Besides, it smells fishy to me that you need to enumerate and
special case these two here, and then you have to repeat them below
in a separate case arm.
> branch.*)
> local pfx="${cur%.*}." cur_="${cur#*.}"
> - __gitcomp_nl "$(__git_heads)" "$pfx" "$cur_" "."
> + __gitcomp_2 "$(__git_heads)" "
> + autosetupmerge autosetuprebase
> + " "$pfx" "$cur_" "."
> return
> ;;
> guitool.*.*)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gc: notice gc processes run by other users
On Dec 31, 2013, at 04:07, Kyle J. McKay wrote:
Since 64a99eb4 git gc refuses to run without the --force option if
another gc process on the same repository is already running.
However, if the repository is shared and user A runs git gc on the
repository and while that gc is still running user B runs git gc on
the same repository the gc process run by user A will not be noticed
and the gc run by user B will go ahead and run.
The problem is that the kill(pid, 0) test fails with an EPERM error
since user B is not allowed to signal processes owned by user A
(unless user B is root).
Update the test to recognize an EPERM error as meaning the process
exists and another gc should not be run (unless --force is given).
Oops, sorry, forgot sign off, here's my sign off:
Signed-off-by: Kyle J. McKay
---
I suggest this be included in maint as others may also have expected
the
shared repository, different user gc scenario to be caught by the new
code when in fact it's not without this patch.
builtin/gc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index c14190f8..25f2237c 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -222,7 +222,7 @@ static const char *lock_repo_for_gc(int force,
pid_t* ret_pid)
time(NULL) - st.st_mtime <= 12 * 3600 &&
fscanf(fp, "%"PRIuMAX" %127c", &pid, locking_host) == 2
&&
/* be gentle to concurrent "gc" on remote hosts */
- (strcmp(locking_host, my_host) || !kill(pid, 0));
+ (strcmp(locking_host, my_host) || !kill(pid, 0) || errno ==
EPERM);
if (fp != NULL)
fclose(fp);
if (should_exit) {
--
1.8.5.2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gc: notice gc processes run by other users
Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] Introduce git submodule add|update --attach
2014/1/3 Francesco Pretto : > Concluding, my point is that at the current state submodules in git > seem to be flawed because of the inconsistent HEAD state between "add" > and "update" users. With my patch applied the attached HEAD behavior > would be fully supported. At some point "git submodule add" (without > the "--attached" switch) could be also modified to produce a detached > HEAD by default, removing any remaining inconsistency. > Also consider that ultimately my desired behavior for submodules is the following: 1) I'd like cloning users to have an attached HEAD by default when doing "git submodule update"; 2) when using an attached HEAD, I'd like "git submodule update" to also imply "--remote". An alternative approach would be, for example, make "git submodule update" honor the current documentation about the behavior of "merge" and "rebase" update operations, if confirmed to be wrong, really keeping the HEAD attached. That would be a breaking change, but at least it would reflect currently documented behavior. I prefer my original approach, because it adds more feasibility and it's not breaking in the short time, but I'm open to that and other solutions. Please let me know, what do you think about it. Thank you, Francesco -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] t0000 cleanups
On Thu, Jan 02, 2014 at 02:28:33PM -0800, Jonathan Nieder wrote: > > This is not exactly true. The TEST_OUTPUT_DIRECTORY setting does not > > leak. t sets $TEST_DIRECTORY (which it must, so the sub-scripts can > > find test-lib.sh and friends), and then TEST_OUTPUT_DIRECTORY uses that > > as a default if it is not explicitly set. > > So I should have said something like the following instead: > [...] Yes, looks good to me. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V3 2/2] fetch --prune: Run prune before fetching
When we have a remote-tracking branch named "frotz/nitfol" from a
previous fetch, and the upstream now has a branch named "frotz". Prior
to this patch fetch would fail to remove "frotz/nitfol" with a "git
fetch --prune" from the upstream. git would inform the user to use "git
remote prune" to fix the problem.
This patch changes the way "fetch --prune" works by moving the pruning
operation before the fetching operation. Instead of warning the user of
a conflict, it autmatically fixes it.
Signed-off-by: Tom Miller
Tested-by: Thomas Rast
Acked-by: Junio C Hamano
---
I did change the commit message according to Junio's suggestion in the
first patch.
builtin/fetch.c | 10 +-
t/t5510-fetch.sh | 14 ++
2 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 1b81cf9..09825c8 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -863,11 +863,6 @@ static int do_fetch(struct transport *transport,
if (tags == TAGS_DEFAULT && autotags)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
- if (fetch_refs(transport, ref_map)) {
- free_refs(ref_map);
- retcode = 1;
- goto cleanup;
- }
if (prune) {
/*
* We only prune based on refspecs specified
@@ -883,6 +878,11 @@ static int do_fetch(struct transport *transport,
transport->url);
}
}
+ if (fetch_refs(transport, ref_map)) {
+ free_refs(ref_map);
+ retcode = 1;
+ goto cleanup;
+ }
free_refs(ref_map);
/* if neither --no-tags nor --tags was specified, do automated tag
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 87e896d..12674ac 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -626,4 +626,18 @@ test_expect_success 'fetch --prune prints the remotes url'
'
test_cmp expect actual
'
+test_expect_success 'branchname D/F conflict resolved by --prune' '
+ git branch dir/file &&
+ git clone . prune-df-conflict &&
+ git branch -D dir/file &&
+ git branch dir &&
+ (
+ cd prune-df-conflict &&
+ git fetch --prune &&
+ git rev-parse origin/dir >../actual
+ ) &&
+ git rev-parse dir >expect &&
+ test_cmp expect actual
+'
+
test_done
--
1.8.5.2.231.g4834e63.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V3 1/2] fetch --prune: Always print header url
If "fetch --prune" is run with no new refs to fetch, but it has refs
to prune. Then, the header url is not printed as it would if there were
new refs to fetch.
Output before this patch:
$ git fetch --prune remote-with-no-new-refs
x [deleted] (none) -> origin/world
Output after this patch:
$ git fetch --prune remote-with-no-new-refs
From https://github.com/git/git
x [deleted] (none) -> origin/test
Signed-off-by: Tom Miller
---
I decided it is not worth writing a function to format the header url
that is printed by fetch. Instead, I will duplicate the formatting logic
for now because I plan on following up this patch set with another patch
to stop striping the trailing "/" and ".git" from the url. When that
patch is finished it will remove the majority of the duplicated logic.
builtin/fetch.c | 32 +++-
t/t5510-fetch.sh | 12
2 files changed, 39 insertions(+), 5 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 1e7d617..1b81cf9 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -44,6 +44,7 @@ static struct transport *gtransport;
static struct transport *gsecondary;
static const char *submodule_prefix = "";
static const char *recurse_submodules_default;
+static int shown_url = 0;
static int option_parse_recurse_submodules(const struct option *opt,
const char *arg, int unset)
@@ -535,7 +536,7 @@ static int store_updated_refs(const char *raw_url, const
char *remote_name,
{
FILE *fp;
struct commit *commit;
- int url_len, i, shown_url = 0, rc = 0;
+ int url_len, i, rc = 0;
struct strbuf note = STRBUF_INIT;
const char *what, *kind;
struct ref *rm;
@@ -708,17 +709,36 @@ static int fetch_refs(struct transport *transport, struct
ref *ref_map)
return ret;
}
-static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map)
+static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map,
+ const char *raw_url)
{
- int result = 0;
+ int url_len, i, result = 0;
struct ref *ref, *stale_refs = get_stale_heads(refs, ref_count,
ref_map);
+ char *url;
const char *dangling_msg = dry_run
? _(" (%s will become dangling)")
: _(" (%s has become dangling)");
+ if (raw_url)
+ url = transport_anonymize_url(raw_url);
+ else
+ url = xstrdup("foreign");
+
+ url_len = strlen(url);
+ for (i = url_len - 1; url[i] == '/' && 0 <= i; i--)
+ ;
+
+ url_len = i + 1;
+ if (4 < i && !strncmp(".git", url + i - 3, 4))
+ url_len = i - 3;
+
for (ref = stale_refs; ref; ref = ref->next) {
if (!dry_run)
result |= delete_ref(ref->name, NULL, 0);
+ if (verbosity >= 0 && !shown_url) {
+ fprintf(stderr, _("From %.*s\n"), url_len, url);
+ shown_url = 1;
+ }
if (verbosity >= 0) {
fprintf(stderr, " x %-*s %-*s -> %s\n",
TRANSPORT_SUMMARY(_("[deleted]")),
@@ -726,6 +746,7 @@ static int prune_refs(struct refspec *refs, int ref_count,
struct ref *ref_map)
warn_dangling_symref(stderr, dangling_msg, ref->name);
}
}
+ free(url);
free_refs(stale_refs);
return result;
}
@@ -854,11 +875,12 @@ static int do_fetch(struct transport *transport,
* don't care whether --tags was specified.
*/
if (ref_count) {
- prune_refs(refs, ref_count, ref_map);
+ prune_refs(refs, ref_count, ref_map, transport->url);
} else {
prune_refs(transport->remote->fetch,
transport->remote->fetch_refspec_nr,
- ref_map);
+ ref_map,
+ transport->url);
}
}
free_refs(ref_map);
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 5d4581d..87e896d 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -614,4 +614,16 @@ test_expect_success 'all boundary commits are excluded' '
test_bundle_object_count .git/objects/pack/pack-${pack##pack}.pack 3
'
+test_expect_success 'fetch --prune prints the remotes url' '
+ git branch goodbye &&
+ git clone . only-prunes &&
+ git branch -D goodbye &&
+ (
+ cd only-prunes &&
+ git fetch --prune origin 2>&1 | head -n1 >../actual
+ ) &&
+ echo "From ${D}/." >expect &&
+ test_cmp expect actual
+'
+
test_done
--
1.8.5.2.231.g4834e63.dirty
--
To unsubscribe from this list: send the line "un
Re: aborted 'git fetch' leaves workspace unusable
Junio C Hamano writes: > stephen_le...@stephe-leake.org writes: > >> However, in this case, even running the fetch was a mistake; I would >> have prefered that it leave FETCH_HEAD in its previous state. > > I think the clearing of leftover FETCH_HEAD is one of the early > things "git fetch" does, unless "--append" is in effect. I haven't > looked at the code for a long time, but it may be possible to move > the logic of doing so around so that this clearing is done as lazily > as possible. > > I however suspect that such a change may have fallouts on other > people who are writing tools like yours; they may be depending on > seeing FETCH_HEAD cleared after a failed fetch, and be surprised to > see a stale contents after they (attempt to) run "git fetch" in it. > > So it is not so clear if it is a good thing to change the behaviour > of "git fetch" not to touch FETCH_HEAD upon a failure. Ok; backwards compatibility is important. Perhaps FETCH_HEAD could be copied to FETCH_HEAD_prev or some such, to allow recovering in an error case? -- -- Stephe -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] completion: fix branch.autosetup(merge|rebase)
Junio C Hamano wrote:
> If we are looking at "branch.autosetupmerge." followed by something,
> who typed that final dot?
I admit that it's a very unlikely case. The user did:
$ branch.autosetupmer
hit backspace to delete the trailing space, inserted a dot, and hit again.
> If you are working on a topic about
> auto-setup-merge and named your branch "autosetupmerge", don't you
> want to be able to configure various aspect of that branch via
> branch.autosetupmerge.{remote,merge} etc., just like you can do so
> for your "topic" branch via branch.topic.{remote,merge} etc.,
> regardless of your use of "autosetupmerge" option across branches?
My reasoning was that being correct was more important that being
complete. So, if by some horrible chance, the user names her branch
"autosetupmerge", we don't aid her in completions.
> Besides, it smells fishy to me that you need to enumerate and
> special case these two here, and then you have to repeat them below
> in a separate case arm.
I'm not too irked about correctness in this odd case; seeing that you
aren't either, I'll resubmit the series without this hunk (+ the hunk
in remote.pushdefault).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] completion: introduce __gitcomp_2 ()
Junio C Hamano wrote: > __gitcomp_nl "$(__git_heads)" "$pfx" "$cur_" "." > __gitcomp_nl_append $"autosetupmerge\nautosetuprebase\n" "$pfx" > "$cur_" " " This is not a bad idea at all. I'm just afraid that we might be leaving open ends: What happens if the $pfx isn't the same in both cases? Who keeps track of the index "i" of COMPREPLY (it's currently a local variable)? If we make it global, doesn't every function that deals with COMPREPLY be careful to reset it? More importantly, can you see a usecase for more than two completion classes? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
