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
sschube...@gmail.com 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 sschube...@gmail.com
---
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 sschube...@gmail.com
---
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, options },
+
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 },
+ { merge, cmd_merge, RUN_SETUP | NEED_WORK_TREE },
+ { merge-base,
[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 sschube...@gmail.com
---
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 sschube...@gmail.com
---
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, options },
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 sschube...@gmail.com
---
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, options },
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 },
+ { pack-objects, cmd_pack_objects, RUN_SETUP },
+ { pack-redundant, cmd_pack_redundant,
[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 johannes.schinde...@gmx.de
Signed-off-by: Sebastian Schuberth sschube...@gmail.com
---
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
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 phil.h...@gmail.com On Sun, Dec 29, 2013 at 8:49 PM, Francesco Pretto cez...@gmail.com wrote: [...] When updating, using the '--attach' switch or operating in a repository with 'submodule.name.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 branch, 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 orphaned-sha1. Let say origin/branch is at commit origin-sha1. 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 branch if merge and head_detached then git merge orphaned-sha1 case: merge: git merge origin-sha1 rebase: git rebase origin-sha1 !command: command origin-sha1 if rebase and head_detached then git merge orphaned-sha1 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.name.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.name.branch' is checked out as the new HEAD of the submodule repository. If 'submodule.name.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.name.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.name.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.name.branch:: If
Re: [PATCH] drop unnecessary copying in credential_ask_one
Jeff King p...@peff.net 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 p...@peff.net
---
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 \
log -1 --format=%s actual
@@ -286,7 +286,7 @@ test_expect_success 'push to auth-only-for-push repo' '
cd $ROOT_PATH/test_repo_clone
echo push-half-auth expect
test_commit
Re: [PATCH v2 3/4] Speed up is_git_command() by checking early for internal commands
Sebastian Schuberth sschube...@gmail.com 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-string) or a
concept (i.e. invoke 'man' with gitstring). It feels that it
should be a lot less work to just check with the builtin table
and stat(exec-path + string) 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 sschube...@gmail.com
---
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, options },
+
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,
Re: [PATCH v2 4/4] Move builtin-related implementations to a new builtin.c file
Sebastian Schuberth sschube...@gmail.com writes:
Signed-off-by: Sebastian Schuberth sschube...@gmail.com
---
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, options },
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,
Re: [PATCH] Fix safe_create_leading_directories() for Windows
Johannes Schindelin johannes.schinde...@gmx.de 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 cez...@gmail.com 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 brl+...@mail.brlink.eu 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 brl...@debian.org
---
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=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 info at http://vger.kernel.org/majordomo-info.html
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 johannes.schinde...@gmx.de 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 URL 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 v2 1/4] Consistently use the term builtin instead of internal command
On Thu, Jan 2, 2014 at 9:31 PM, Jonathan Nieder jrnie...@gmail.com 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 sschube...@gmail.com 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 johannes.schinde...@gmx.de Signed-off-by: Sebastian Schuberth sschube...@gmail.com 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 sunsh...@sunshineco.com writes: On Fri, Dec 27, 2013 at 5:13 PM, Junio C Hamano gits...@pobox.com wrote: Eric Sunshine sunsh...@sunshineco.com writes: On Thu, Dec 26, 2013 at 4:08 PM, Junio C Hamano gits...@pobox.com 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 gits...@pobox.com [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 sschube...@gmail.com 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 johannes.schinde...@gmx.de
Signed-off-by: Sebastian Schuberth sschube...@gmail.com
---
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 gits...@pobox.com wrote: Eric Sunshine sunsh...@sunshineco.com writes: On Fri, Dec 27, 2013 at 5:13 PM, Junio C Hamano gits...@pobox.com wrote: Eric Sunshine sunsh...@sunshineco.com writes: On Thu, Dec 26, 2013 at 4:08 PM, Junio C Hamano gits...@pobox.com 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 brl+...@mail.brlink.eu 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 gits...@pobox.com 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 sunsh...@sunshineco.com
---
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 gits...@pobox.com writes:
Junio C Hamano gits...@pobox.com 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 jrnie...@gmail.com 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 gits...@pobox.com: Francesco Pretto cez...@gmail.com 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 sha1, 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 branch # 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 branch # 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? That makes the whole thing into a single step, not giving the user a
Re: [PATCH 2/4] completion: introduce __gitcomp_2 ()
Ramkumar Ramachandra artag...@gmail.com writes:
There are situations where two classes of completions possible. For
example
branch.TAB
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 artag...@gmail.com 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 mack...@gmail.com
---
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
[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 jacker...@gmail.com
Tested-by: Thomas Rast t...@thomasrast.ch
Acked-by: Junio C Hamano gits...@pobox.com
---
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 jacker...@gmail.com
---
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 21 | 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 unsubscribe git in
the body of a message to
Re: aborted 'git fetch' leaves workspace unusable
Junio C Hamano gits...@pobox.com 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.autosetupmerTAB
hit backspace to delete the trailing space, inserted a dot, and hit TAB 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
