Re: [PATCH] format-patch: clear UNINTERESTING flag before prepare_bases
Hi, Junio
Could you help review this patch?
Thanks,
Xiaolong
On 06/04, Xiaolong Ye wrote:
>When users specify the commit range with 'Z..C' pattern for format-patch, all
>the parents of Z (including Z) would be marked as UNINTERESTING which would
>prevent revision walk in prepare_bases from getting the prerequisite commits,
>thus `git format-patch --base Z..C` won't be able to generate
>the list of prerequisite patch ids. Clear UNINTERESTING flag with
>clear_object_flags solves this issue.
>
>Reported-by: Eduardo Habkost
>Signed-off-by: Xiaolong Ye
>---
> builtin/log.c | 1 +
> t/t4014-format-patch.sh | 6 --
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
>diff --git a/builtin/log.c b/builtin/log.c
>index 4686f68594..01993de6fe 100644
>--- a/builtin/log.c
>+++ b/builtin/log.c
>@@ -1746,6 +1746,7 @@ int cmd_format_patch(int argc, const char **argv, const
>char *prefix)
> if (base_commit || base_auto) {
> struct commit *base = get_base_commit(base_commit, list, nr);
> reset_revision_walk();
>+ clear_object_flags(UNINTERESTING);
> prepare_bases(, base, list, nr);
> }
>
>diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
>index 028d5507a6..53880da7bb 100755
>--- a/t/t4014-format-patch.sh
>+++ b/t/t4014-format-patch.sh
>@@ -1554,13 +1554,15 @@ test_expect_success 'format-patch -o overrides
>format.outputDirectory' '
>
> test_expect_success 'format-patch --base' '
> git checkout side &&
>- git format-patch --stdout --base=HEAD~3 -1 | tail -n 7 >actual &&
>+ git format-patch --stdout --base=HEAD~3 -1 | tail -n 7 >actual1 &&
>+ git format-patch --stdout --base=HEAD~3 HEAD~.. | tail -n 7 >actual2 &&
> echo >expected &&
> echo "base-commit: $(git rev-parse HEAD~3)" >>expected &&
> echo "prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id
> --stable | awk "{print \$1}")" >>expected &&
> echo "prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id
> --stable | awk "{print \$1}")" >>expected &&
> signature >> expected &&
>- test_cmp expected actual
>+ test_cmp expected actual1 &&
>+ test_cmp expected actual2
> '
>
> test_expect_success 'format-patch --base errors out when base commit is in
> revision list' '
>--
>2.16.GIT
>
[GIT PULL] Korean l10n updates for Git 2.18.0
Hi Junio, The following changes since commit fd8cb379022fc6f5c6d71d12d10c9388b9f5841c: l10n: zh_CN: for git v2.18.0 l10n round 1 to 3 (2018-06-18 00:31:45 +0800) are available in the Git repository at: git://github.com/git-l10n/git-po tags/l10n-2.18.0-rnd3.1 for you to fetch changes up to 4898dd2513360bd0cb32ca67ca07c70787c81399: l10n: ko.po: Update Korean translation (2018-06-19 02:19:42 +0900) Merge Korean translation for l10n of Git 2.18.0 round 3 Changwoo Ryu (1): l10n: ko.po: Update Korean translation po/TEAMS |4 +- po/ko.po | 6083 -- 2 files changed, 3553 insertions(+), 2534 deletions(-) -- Jiang Xin
Re: want option to git-rebase
Hi,
Ian Jackson wrote[1]:
> git-rebase leaves entries like this in the reflog:
>
> c15f4d5391 HEAD@{33}: rebase: checkout
> c15f4d5391ff07a718431aca68a73e672fe8870e
>
> It would be nice if there were an option to control this message.
> Particularly, when another tool invokes git-rebase, the other tool may
> specify an interesting --onto, and there is no way to record any
> information about that --onto commit.
>
> git-rebase already has a -m option, so I suggest
> --reason=
>
> It doesn't matter much exactly how the provided string is used.
> Any of the following would be good IMO:
>
> rebase start:
>
> I think:
> rebase: checkout c15f4d5391ff07a718431aca68a73e672fe8870e
> would be rather cumbersome.
>From git(1):
GIT_REFLOG_ACTION
When a ref is updated, reflog entries are created to keep
track of the reason why the ref was updated (which is
typically the name of the high-level command that updated the
ref), in addition to the old and new values of the ref. A
scripted Porcelain command can use set_reflog_action helper
function in git-sh-setup to set its name to this variable when
it is invoked as the top level command by the end user, to be
recorded in the body of the reflog.
"git rebase" sets this itself, so it doesn't solve your problem.
Can you say more about what your tool does? I'm wondering if it would
make sense for it to use lower-level commands where GIT_REFLOG_ACTION
applies, instead of the more user-facing git rebase.
Thanks,
Jonathan
[1] https://bugs.debian.org/901805
[PATCH 2/3] submodule: ensure core.worktree is set after update
Signed-off-by: Stefan Beller
---
builtin/submodule--helper.c | 24
git-submodule.sh| 5 +
2 files changed, 29 insertions(+)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index bd250ca2164..dffc55ed8ee 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1860,6 +1860,29 @@ static int check_name(int argc, const char **argv, const
char *prefix)
return 0;
}
+static int connect_gitdir_workingtree(int argc, const char **argv, const char
*prefix)
+{
+ struct strbuf sb = STRBUF_INIT;
+ const char *name, *path;
+ char *sm_gitdir;
+
+ if (argc != 3)
+ BUG("submodule--helper connect-gitdir-workingtree
");
+
+ name = argv[1];
+ path = argv[2];
+
+ strbuf_addf(, "%s/modules/%s", get_git_dir(), name);
+ sm_gitdir = absolute_pathdup(sb.buf);
+
+ connect_work_tree_and_git_dir(path, sm_gitdir, 0);
+
+ strbuf_release();
+ free(sm_gitdir);
+
+ return 0;
+}
+
#define SUPPORT_SUPER_PREFIX (1<<0)
struct cmd_struct {
@@ -1873,6 +1896,7 @@ static struct cmd_struct commands[] = {
{"name", module_name, 0},
{"clone", module_clone, 0},
{"update-clone", update_clone, 0},
+ {"connect-gitdir-workingtree", connect_gitdir_workingtree, 0},
{"relative-path", resolve_relative_path, 0},
{"resolve-relative-url", resolve_relative_url, 0},
{"resolve-relative-url-test", resolve_relative_url_test, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index 78073cd87d1..6bd0db02b33 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -615,6 +615,11 @@ cmd_update()
die "$(eval_gettext "Unable to find current
\${remote_name}/\${branch} revision in submodule path '\$sm_path'")"
fi
+ if ! $(git config -f "$(git rev-parse
--git-common-dir)/modules/$name/config" core.worktree) 2>/dev/null
+ then
+ git submodule--helper connect-gitdir-workingtree
"$name" "$sm_path"
+ fi
+
if test "$subsha1" != "$sha1" || test -n "$force"
then
subforce=$force
--
2.18.0.rc1.244.gcf134e6275-goog
[PATCH 0/3] submodules with no working tree shall unset core.worktree
v2: * fixed quoting issues for white space'd names/paths v1: The first patch teaches checkout/reset (with --recurse-submodules) to unset the core.worktree config when the new state of the superprojects working tree doesn't contain the submodules working tree. The last patch is teaching "git submodule deinit" to unset the core.worktree setting as well. It turned out this one is tricky, as for that we also have to set it in the counter part, such as "submodule update". Thanks, Stefan Stefan Beller (3): submodule: unset core.worktree if no working tree is present submodule: ensure core.worktree is set after update submodule deinit: unset core.worktree builtin/submodule--helper.c | 26 ++ git-submodule.sh| 5 + submodule.c | 14 ++ submodule.h | 2 ++ t/lib-submodule-update.sh | 5 +++-- t/t7400-submodule-basic.sh | 5 + 6 files changed, 55 insertions(+), 2 deletions(-) -- 2.18.0.rc1.244.gcf134e6275-goog
[PATCH 3/3] submodule deinit: unset core.worktree
When a submodule is deinit'd, the working tree is gone, so the setting of
core.worktree is bogus. Unset it.
Signed-off-by: Stefan Beller
---
builtin/submodule--helper.c | 2 ++
t/lib-submodule-update.sh | 2 +-
t/t7400-submodule-basic.sh | 5 +
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index dffc55ed8ee..19480902681 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -980,6 +980,8 @@ static void deinit_submodule(const char *path, const char
*prefix,
if (!(flags & OPT_QUIET))
printf(format, displaypath);
+ submodule_unset_core_worktree(sub);
+
strbuf_release(_rm);
}
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 12cd4e9233e..aa5ac03325a 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -235,7 +235,7 @@ reset_work_tree_to_interested () {
then
mkdir -p submodule_update/.git/modules/sub1/modules &&
cp -r submodule_update_repo/.git/modules/sub1/modules/sub2
submodule_update/.git/modules/sub1/modules/sub2
- GIT_WORK_TREE=. git -C
submodule_update/.git/modules/sub1/modules/sub2 config --unset core.worktree
+ # core.worktree is unset for sub2 as it is not checked out
fi &&
# indicate we are interested in the submodule:
git -C submodule_update config submodule.sub1.url "bogus" &&
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 812db137b8d..48fd14fae6e 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -993,6 +993,11 @@ test_expect_success 'submodule deinit should remove the
whole submodule section
rmdir init
'
+test_expect_success 'submodule deinit should unset core.worktree' '
+ test_path_is_file .git/modules/example/config &&
+ test_must_fail git config -f .git/modules/example/config core.worktree
+'
+
test_expect_success 'submodule deinit from subdirectory' '
git submodule update --init &&
git config submodule.example.foo bar &&
--
2.18.0.rc1.244.gcf134e6275-goog
[PATCH 1/3] submodule: unset core.worktree if no working tree is present
When a submodules work tree is removed, we should unset its core.worktree
setting as the worktree is no longer present. This is not just in line
with the conceptual view of submodules, but it fixes an inconvenience
for looking at submodules that are not checked out:
git clone --recurse-submodules git://github.com/git/git && cd git &&
git checkout --recurse-submodules v2.13.0
git -C .git/modules/sha1collisiondetection log
fatal: cannot chdir to '../../../sha1collisiondetection': \
No such file or directory
With this patch applied, the final call to git log works instead of dying
in its setup, as the checkout will unset the core.worktree setting such
that following log will be run in a bare repository.
This patch covers all commands that are in the unpack machinery, i.e.
checkout, read-tree, reset. A follow up patch will address
"git submodule deinit", which will also make use of the new function
submodule_unset_core_worktree(), which is why we expose it in this patch.
Signed-off-by: Stefan Beller
---
submodule.c | 14 ++
submodule.h | 2 ++
t/lib-submodule-update.sh | 3 ++-
3 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/submodule.c b/submodule.c
index 939d6870ecd..e127c074b04 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1532,6 +1532,18 @@ int bad_to_remove_submodule(const char *path, unsigned
flags)
return ret;
}
+void submodule_unset_core_worktree(const struct submodule *sub)
+{
+ char *config_path = xstrfmt("%s/modules/%s/config",
+ get_git_common_dir(), sub->name);
+
+ if (git_config_set_in_file_gently(config_path, "core.worktree", NULL))
+ warning(_("Could not unset core.worktree setting in submodule
'%s'"),
+ sub->path);
+
+ free(config_path);
+}
+
static const char *get_super_prefix_or_empty(void)
{
const char *s = get_super_prefix();
@@ -1697,6 +1709,8 @@ int submodule_move_head(const char *path,
if (is_empty_dir(path))
rmdir_or_warn(path);
+
+ submodule_unset_core_worktree(sub);
}
}
out:
diff --git a/submodule.h b/submodule.h
index 7856b8a0b3d..4644683e6cb 100644
--- a/submodule.h
+++ b/submodule.h
@@ -121,6 +121,8 @@ extern int submodule_move_head(const char *path,
const char *new_head,
unsigned flags);
+void submodule_unset_core_worktree(const struct submodule *sub);
+
/*
* Prepare the "env_array" parameter of a "struct child_process" for executing
* a submodule by clearing any repo-specific environment variables, but
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 1f38a85371a..12cd4e9233e 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -709,7 +709,8 @@ test_submodule_recursing_with_args_common() {
git branch -t remove_sub1 origin/remove_sub1 &&
$command remove_sub1 &&
test_superproject_content origin/remove_sub1 &&
- ! test -e sub1
+ ! test -e sub1 &&
+ test_must_fail git config -f .git/modules/sub1/config
core.worktree
)
'
# ... absorbing a .git directory along the way.
--
2.18.0.rc1.244.gcf134e6275-goog
[PATCH 4/7] grep.c: display column number of first match
To prepare for 'git grep' learning '--column', teach grep.c's
show_line() how to show the column of the first match on non-context
lines.
Signed-off-by: Taylor Blau
---
grep.c | 30 +-
1 file changed, 25 insertions(+), 5 deletions(-)
diff --git a/grep.c b/grep.c
index 33f8572e6d..9f5b00a471 100644
--- a/grep.c
+++ b/grep.c
@@ -1381,7 +1381,7 @@ static int next_match(struct grep_opt *opt, char *bol,
char *eol,
}
static void show_line(struct grep_opt *opt, char *bol, char *eol,
- const char *name, unsigned lno, char sign)
+ const char *name, unsigned lno, unsigned cno, char sign)
{
int rest = eol - bol;
const char *match_color, *line_color = NULL;
@@ -1416,6 +1416,17 @@ static void show_line(struct grep_opt *opt, char *bol,
char *eol,
output_color(opt, buf, strlen(buf), opt->color_lineno);
output_sep(opt, sign);
}
+ /*
+* Treat 'cno' as the 1-indexed offset from the start of a non-context
+* line to its first match. Otherwise, 'cno' is 0 indicating that we are
+* being called with a context line.
+*/
+ if (opt->columnnum && cno) {
+ char buf[32];
+ xsnprintf(buf, sizeof(buf), "%d", cno);
+ output_color(opt, buf, strlen(buf), opt->color_columnno);
+ output_sep(opt, sign);
+ }
if (opt->color) {
regmatch_t match;
enum grep_context ctx = GREP_CONTEXT_BODY;
@@ -1521,7 +1532,7 @@ static void show_funcname_line(struct grep_opt *opt,
struct grep_source *gs,
break;
if (match_funcname(opt, gs, bol, eol)) {
- show_line(opt, bol, eol, gs->name, lno, '=');
+ show_line(opt, bol, eol, gs->name, lno, 0, '=');
break;
}
}
@@ -1586,7 +1597,7 @@ static void show_pre_context(struct grep_opt *opt, struct
grep_source *gs,
while (*eol != '\n')
eol++;
- show_line(opt, bol, eol, gs->name, cur, sign);
+ show_line(opt, bol, eol, gs->name, cur, 0, sign);
bol = eol + 1;
cur++;
}
@@ -1785,6 +1796,7 @@ static int grep_source_1(struct grep_opt *opt, struct
grep_source *gs, int colle
while (left) {
char *eol, ch;
int hit;
+ ssize_t cno;
ssize_t col = -1, icol = -1;
/*
@@ -1850,7 +1862,15 @@ static int grep_source_1(struct grep_opt *opt, struct
grep_source *gs, int colle
show_pre_context(opt, gs, bol, eol, lno);
else if (opt->funcname)
show_funcname_line(opt, gs, bol, lno);
- show_line(opt, bol, eol, gs->name, lno, ':');
+ cno = opt->invert ? icol : col;
+ if (cno < 0) {
+ /*
+* A negative cno means that there was no match.
+* Clamp to the beginning of the line.
+*/
+ cno = 0;
+ }
+ show_line(opt, bol, eol, gs->name, lno, cno + 1, ':');
last_hit = lno;
if (opt->funcbody)
show_function = 1;
@@ -1879,7 +1899,7 @@ static int grep_source_1(struct grep_opt *opt, struct
grep_source *gs, int colle
/* If the last hit is within the post context,
* we need to show this line.
*/
- show_line(opt, bol, eol, gs->name, lno, '-');
+ show_line(opt, bol, eol, gs->name, lno, col + 1, '-');
}
next_line:
--
2.17.0.582.gccdcbd54c
[PATCH 6/7] grep.c: add configuration variables to show matched option
To support git-grep(1)'s new option, '--column', document and teach
grep.c how to interpret relevant configuration options, similar to those
associated with '--line-number'.
Signed-off-by: Taylor Blau
---
Documentation/config.txt | 5 +
Documentation/git-grep.txt | 3 +++
grep.c | 6 ++
3 files changed, 14 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 58fde4daea..e4cbed3078 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1183,6 +1183,8 @@ color.grep.::
function name lines (when using `-p`)
`lineNumber`;;
line number prefix (when using `-n`)
+`column`;;
+ column number prefix (when using `--column`)
`match`;;
matching text (same as setting `matchContext` and `matchSelected`)
`matchContext`;;
@@ -1797,6 +1799,9 @@ gitweb.snapshot::
grep.lineNumber::
If set to true, enable `-n` option by default.
+grep.column::
+ If set to true, enable the `--column` option by default.
+
grep.patternType::
Set the default matching behavior. Using a value of 'basic', 'extended',
'fixed', or 'perl' will enable the `--basic-regexp`,
`--extended-regexp`,
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 31dc0392a6..0de3493b80 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -44,6 +44,9 @@ CONFIGURATION
grep.lineNumber::
If set to true, enable `-n` option by default.
+grep.column::
+ If set to true, enable the `--column` option by default.
+
grep.patternType::
Set the default matching behavior. Using a value of 'basic', 'extended',
'fixed', or 'perl' will enable the `--basic-regexp`,
`--extended-regexp`,
diff --git a/grep.c b/grep.c
index 9f5b00a471..8ffa94c791 100644
--- a/grep.c
+++ b/grep.c
@@ -96,6 +96,10 @@ int grep_config(const char *var, const char *value, void *cb)
opt->linenum = git_config_bool(var, value);
return 0;
}
+ if (!strcmp(var, "grep.column")) {
+ opt->columnnum = git_config_bool(var, value);
+ return 0;
+ }
if (!strcmp(var, "grep.fullname")) {
opt->relative = !git_config_bool(var, value);
@@ -112,6 +116,8 @@ int grep_config(const char *var, const char *value, void
*cb)
color = opt->color_function;
else if (!strcmp(var, "color.grep.linenumber"))
color = opt->color_lineno;
+ else if (!strcmp(var, "color.grep.column"))
+ color = opt->color_columnno;
else if (!strcmp(var, "color.grep.matchcontext"))
color = opt->color_match_context;
else if (!strcmp(var, "color.grep.matchselected"))
--
2.17.0.582.gccdcbd54c
[PATCH 2/7] grep.c: expose {,inverted} match column in match_line()
When calling match_line(), callers presently cannot determine the
relative offset of the match because match_line() discards the
'regmatch_t' that contains this information.
Instead, teach match_line() to take in two 'ssize_t's. Fill the first
with the offset of the match produced by the given expression. If
extended, fill the later with the offset of the match produced as if
--invert were given.
For instance, matching "--not -e x" on this line produces a columnar
offset of 0, (i.e., the whole line does not contain an x), but "--invert
--not -e -x" will fill the later ssize_t of the column containing an
"x", because this expression is semantically equivalent to "-e x".
To determine the column for the inverted and non-inverted case, do the
following:
- If matching an atom, the non-inverted column is as given from
match_one_pattern(), and the inverted column is unset.
- If matching a --not, the inverted column and non-inverted column swap.
- If matching an --and, or --or, the non-inverted column is the
minimum of the two children, with the exception that --or is
short-circuiting. For instance, if we match "-e a --or -e b" on a
line that contains both "a" and "b" (and "b" comes first), the match
column will hold "a", since we inspected the left child first, and
short-circuited over the right child.
This patch will become useful when we later pick between the two new
results in order to display the column number of the first match on a
line with --column.
Co-authored-by: Jeff King
Signed-off-by: Taylor Blau
---
grep.c | 56 ++--
1 file changed, 38 insertions(+), 18 deletions(-)
diff --git a/grep.c b/grep.c
index 45ec7e636c..19c782aa9d 100644
--- a/grep.c
+++ b/grep.c
@@ -1249,10 +1249,10 @@ static int match_one_pattern(struct grep_pat *p, char
*bol, char *eol,
}
static int match_expr_eval(struct grep_expr *x, char *bol, char *eol,
- enum grep_context ctx, int collect_hits)
+ enum grep_context ctx, ssize_t *col,
+ ssize_t *icol, int collect_hits)
{
int h = 0;
- regmatch_t match;
if (!x)
die("Not a valid grep expression");
@@ -1261,25 +1261,39 @@ static int match_expr_eval(struct grep_expr *x, char
*bol, char *eol,
h = 1;
break;
case GREP_NODE_ATOM:
- h = match_one_pattern(x->u.atom, bol, eol, ctx, , 0);
+ {
+ regmatch_t tmp;
+ h = match_one_pattern(x->u.atom, bol, eol, ctx,
+ , 0);
+ if (h && (*col < 0 || tmp.rm_so < *col))
+ *col = tmp.rm_so;
+ }
break;
case GREP_NODE_NOT:
- h = !match_expr_eval(x->u.unary, bol, eol, ctx, 0);
+ /*
+* Upon visiting a GREP_NODE_NOT, imatch and match become
+* swapped.
+*/
+ h = !match_expr_eval(x->u.unary, bol, eol, ctx, icol, col, 0);
break;
case GREP_NODE_AND:
- if (!match_expr_eval(x->u.binary.left, bol, eol, ctx, 0))
+ if (!match_expr_eval(x->u.binary.left, bol, eol, ctx, col,
+icol, 0))
return 0;
- h = match_expr_eval(x->u.binary.right, bol, eol, ctx, 0);
+ h = match_expr_eval(x->u.binary.right, bol, eol, ctx, col,
+ icol, 0);
break;
case GREP_NODE_OR:
if (!collect_hits)
- return (match_expr_eval(x->u.binary.left,
- bol, eol, ctx, 0) ||
- match_expr_eval(x->u.binary.right,
- bol, eol, ctx, 0));
- h = match_expr_eval(x->u.binary.left, bol, eol, ctx, 0);
+ return (match_expr_eval(x->u.binary.left, bol, eol, ctx,
+ col, icol, 0) ||
+ match_expr_eval(x->u.binary.right, bol, eol,
+ ctx, col, icol, 0));
+ h = match_expr_eval(x->u.binary.left, bol, eol, ctx, col,
+ icol, 0);
x->u.binary.left->hit |= h;
- h |= match_expr_eval(x->u.binary.right, bol, eol, ctx, 1);
+ h |= match_expr_eval(x->u.binary.right, bol, eol, ctx, col,
+icol, 1);
break;
default:
die("Unexpected node type (internal error) %d", x->node);
@@ -1290,25 +1304,30 @@ static int match_expr_eval(struct grep_expr *x, char
*bol, char *eol,
}
static int match_expr(struct grep_opt *opt, char *bol, char
[PATCH 7/7] contrib/git-jump/git-jump: jump to exact location
Take advantage of 'git-grep(1)''s new option, '--column' in order to
teach Peff's 'git-jump' script how to jump to the correct column for any
given match.
'git-grep(1)''s output is in the correct format for Vim's jump list, so
no additional cleanup is necessary.
Signed-off-by: Taylor Blau
---
contrib/git-jump/README | 12 ++--
contrib/git-jump/git-jump | 2 +-
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/contrib/git-jump/README b/contrib/git-jump/README
index 4484bda410..2f618a7f97 100644
--- a/contrib/git-jump/README
+++ b/contrib/git-jump/README
@@ -25,6 +25,13 @@ git-jump will feed this to the editor:
foo.c:2: printf("hello word!\n");
---
+Or, when running 'git jump grep', column numbers will also be emitted,
+e.g. `git jump grep "hello"` would return:
+
+---
+foo.c:2:9: printf("hello word!\n");
+---
+
Obviously this trivial case isn't that interesting; you could just open
`foo.c` yourself. But when you have many changes scattered across a
project, you can use the editor's support to "jump" from point to point.
@@ -35,7 +42,8 @@ Git-jump can generate four types of interesting lists:
2. The beginning of any merge conflict markers.
- 3. Any grep matches.
+ 3. Any grep matches, including the column of the first match on a
+ line.
4. Any whitespace errors detected by `git diff --check`.
@@ -82,7 +90,7 @@ which does something similar to `git jump grep`. However, it
is limited
to positioning the cursor to the correct line in only the first file,
leaving you to locate subsequent hits in that file or other files using
the editor or pager. By contrast, git-jump provides the editor with a
-complete list of files and line numbers for each match.
+complete list of files, lines, and a column number for each match.
Limitations
diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
index 80ab0590bc..931b0fe3a9 100755
--- a/contrib/git-jump/git-jump
+++ b/contrib/git-jump/git-jump
@@ -52,7 +52,7 @@ mode_merge() {
# editor shows them to us in the status bar.
mode_grep() {
cmd=$(git config jump.grepCmd)
- test -n "$cmd" || cmd="git grep -n"
+ test -n "$cmd" || cmd="git grep -n --column"
$cmd "$@" |
perl -pe '
s/[ \t]+/ /g;
--
2.17.0.582.gccdcbd54c
[PATCH 1/7] Documentation/config.txt: camel-case lineNumber for consistency
lineNumber has casing that is inconsistent with surrounding options, like color.grep.matchContext, and color.grep.matchSelected. Re-case this documentation in order to be consistent with the text around it, and to ensure that new entries are consistent, too. Signed-off-by: Taylor Blau --- Documentation/config.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index ab641bf5a9..58fde4daea 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1181,7 +1181,7 @@ color.grep.:: filename prefix (when not using `-h`) `function`;; function name lines (when using `-p`) -`linenumber`;; +`lineNumber`;; line number prefix (when using `-n`) `match`;; matching text (same as setting `matchContext` and `matchSelected`) -- 2.17.0.582.gccdcbd54c
[PATCH 3/7] grep.[ch]: extend grep_opt to allow showing matched column
To support showing the matched column when calling 'git-grep(1)', teach
'grep_opt' the normal set of options to configure the default behavior
and colorization of this feature.
Signed-off-by: Taylor Blau
---
grep.c | 3 +++
grep.h | 2 ++
2 files changed, 5 insertions(+)
diff --git a/grep.c b/grep.c
index 19c782aa9d..33f8572e6d 100644
--- a/grep.c
+++ b/grep.c
@@ -46,6 +46,7 @@ void init_grep_defaults(void)
color_set(opt->color_filename, "");
color_set(opt->color_function, "");
color_set(opt->color_lineno, "");
+ color_set(opt->color_columnno, "");
color_set(opt->color_match_context, GIT_COLOR_BOLD_RED);
color_set(opt->color_match_selected, GIT_COLOR_BOLD_RED);
color_set(opt->color_selected, "");
@@ -155,6 +156,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)
opt->extended_regexp_option = def->extended_regexp_option;
opt->pattern_type_option = def->pattern_type_option;
opt->linenum = def->linenum;
+ opt->columnnum = def->columnnum;
opt->max_depth = def->max_depth;
opt->pathname = def->pathname;
opt->relative = def->relative;
@@ -164,6 +166,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)
color_set(opt->color_filename, def->color_filename);
color_set(opt->color_function, def->color_function);
color_set(opt->color_lineno, def->color_lineno);
+ color_set(opt->color_columnno, def->color_columnno);
color_set(opt->color_match_context, def->color_match_context);
color_set(opt->color_match_selected, def->color_match_selected);
color_set(opt->color_selected, def->color_selected);
diff --git a/grep.h b/grep.h
index 399381c908..08a0b391c5 100644
--- a/grep.h
+++ b/grep.h
@@ -127,6 +127,7 @@ struct grep_opt {
int prefix_length;
regex_t regexp;
int linenum;
+ int columnnum;
int invert;
int ignore_case;
int status_only;
@@ -159,6 +160,7 @@ struct grep_opt {
char color_filename[COLOR_MAXLEN];
char color_function[COLOR_MAXLEN];
char color_lineno[COLOR_MAXLEN];
+ char color_columnno[COLOR_MAXLEN];
char color_match_context[COLOR_MAXLEN];
char color_match_selected[COLOR_MAXLEN];
char color_selected[COLOR_MAXLEN];
--
2.17.0.582.gccdcbd54c
[PATCH 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'
Teach 'git-grep(1)' a new option, '--column', to show the column
number of the first match on a non-context line. This makes it possible
to teach 'contrib/git-jump/git-jump' how to seek to the first matching
position of a grep match in your editor, and allows similar additional
scripting capabilities.
For example:
$ git grep -n --column foo | head -n3
.clang-format:51:14:# myFunction(foo, bar, baz);
.clang-format:64:7:# int foo();
.clang-format:75:8:# void foo()
Signed-off-by: Taylor Blau
---
Documentation/git-grep.txt | 6 +++-
builtin/grep.c | 1 +
t/t7810-grep.sh| 63 ++
3 files changed, 69 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 312409a607..31dc0392a6 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -13,7 +13,7 @@ SYNOPSIS
[-v | --invert-match] [-h|-H] [--full-name]
[-E | --extended-regexp] [-G | --basic-regexp]
[-P | --perl-regexp]
- [-F | --fixed-strings] [-n | --line-number]
+ [-F | --fixed-strings] [-n | --line-number] [--column]
[-l | --files-with-matches] [-L | --files-without-match]
[(-O | --open-files-in-pager) []]
[-z | --null]
@@ -169,6 +169,10 @@ providing this option will cause it to die.
--line-number::
Prefix the line number to matching lines.
+--column::
+ Prefix the 1-indexed byte-offset of the first match from the start of
the
+ matching line.
+
-l::
--files-with-matches::
--name-only::
diff --git a/builtin/grep.c b/builtin/grep.c
index ee753a403e..61bcaf6e58 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -828,6 +828,7 @@ int cmd_grep(int argc, const char **argv, const char
*prefix)
GREP_PATTERN_TYPE_PCRE),
OPT_GROUP(""),
OPT_BOOL('n', "line-number", , N_("show line
numbers")),
+ OPT_BOOL(0, "column", , N_("show column number of
first match")),
OPT_NEGBIT('h', NULL, , N_("don't show
filenames"), 1),
OPT_BIT('H', NULL, , N_("show filenames"), 1),
OPT_NEGBIT(0, "full-name", ,
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 1797f632a3..daaf7b4c44 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -99,6 +99,69 @@ do
test_cmp expected actual
'
+ test_expect_success "grep -w $L (with --column)" '
+ {
+ echo ${HC}file:5:foo mmap bar
+ echo ${HC}file:14:foo_mmap bar mmap
+ echo ${HC}file:5:foo mmap bar_mmap
+ echo ${HC}file:14:foo_mmap bar mmap baz
+ } >expected &&
+ git grep --column -w -e mmap $H >actual &&
+ test_cmp expected actual
+ '
+
+ test_expect_success "grep -w $L (with --column, extended)" '
+ {
+ echo ${HC}file:14:foo_mmap bar mmap
+ echo ${HC}file:19:foo_mmap bar mmap baz
+ } >expected &&
+ git grep --column -w -e mmap$ --or -e baz $H >actual &&
+ test_cmp expected actual
+ '
+
+ test_expect_success "grep -w $L (with --column, --invert)" '
+ {
+ echo ${HC}file:1:foo mmap bar
+ echo ${HC}file:1:foo_mmap bar
+ echo ${HC}file:1:foo_mmap bar mmap
+ echo ${HC}file:1:foo mmap bar_mmap
+ } >expected &&
+ git grep --column --invert -w -e baz $H -- file >actual &&
+ test_cmp expected actual
+ '
+
+ test_expect_success "grep $L (with --column, --invert, extended)" '
+ {
+ echo ${HC}hello_world:6:HeLLo_world
+ } >expected &&
+ git grep --column --invert -e ll --or --not -e _ $H --
hello_world \
+ >actual &&
+ test_cmp expected actual
+ '
+
+ test_expect_success "grep -w $L (with --column, -C)" '
+ {
+ echo ${HC}file:5:foo mmap bar
+ echo ${HC}file-foo_mmap bar
+ echo ${HC}file:14:foo_mmap bar mmap
+ echo ${HC}file:5:foo mmap bar_mmap
+ echo ${HC}file:14:foo_mmap bar mmap baz
+ } >expected &&
+ git grep --column -w -C1 -e mmap $H >actual &&
+ test_cmp expected actual
+ '
+
+ test_expect_success "grep -w $L (with --line-number, --column)" '
+ {
+ echo ${HC}file:1:5:foo mmap bar
+ echo ${HC}file:3:14:foo_mmap bar mmap
+ echo ${HC}file:4:5:foo mmap bar_mmap
+ echo ${HC}file:5:14:foo_mmap bar mmap baz
+ } >expected &&
+ git
[PATCH 0/7] grep.c: teach --column to 'git-grep(1)'
Hi,
Attached is a ``fresh start'' of my series to teach 'git grep --column'.
Since the last time I sent this, much has changed, notably the semantics
for deciding which column is the first when given (1) extended
expressions and (2) --invert.
Both (1) and (2) are described in-depth in patch 2/7, but I am happy to
answer more questions should they arise here. Peff and I worked on this
together off-list, and we are both happy with the semantics, and believe
that it covers most reasonable cases.
The notable case that it does _not_ cover is matching the following
line:
a ... b
with the following expression
git grep --column -e b --or -e a
This will produce the column for 'b' rather than the column for 'a',
since we short-circuit an --or when the left child finds a match, in
this case 'b'. So, we break the semantics for this case, at the benefit
of not having to do twice the work.
In the future, I'd like to revisit this, since any performance gains
that we _do_ make in this area are moot when we rescan all lines in
show_line() with --color. A path forward, I imagine, would look like a
list of regmatch_t's, or a set of locations in the expression tree, such
that we could either enumerate the list or walk the tree in order to
colorize the line. But, I think for now that is #leftoverbits.
Thanks especially to the last round of reviewers for their detailed
feedback, and I hope that starting in a new series will be OK. I figure
that enough has changed that I'd rather not clutter an already busy
thread.
Thanks,
Taylor
Taylor Blau (7):
Documentation/config.txt: camel-case lineNumber for consistency
grep.c: expose {,inverted} match column in match_line()
grep.[ch]: extend grep_opt to allow showing matched column
grep.c: display column number of first match
builtin/grep.c: add '--column' option to 'git-grep(1)'
grep.c: add configuration variables to show matched option
contrib/git-jump/git-jump: jump to exact location
Documentation/config.txt | 7 ++-
Documentation/git-grep.txt | 9 +++-
builtin/grep.c | 1 +
contrib/git-jump/README| 12 -
contrib/git-jump/git-jump | 2 +-
grep.c | 95 +-
grep.h | 2 +
t/t7810-grep.sh| 63 +
8 files changed, 163 insertions(+), 28 deletions(-)
--
2.17.0.582.gccdcbd54c
[PATCH] t7400: encapsulate setup code in test_expect_success
When running t7400 in a shell you observe more output than expected: ... ok 8 - setup - hide init subdirectory ok 9 - setup - repository to add submodules to ok 10 - submodule add [master (root-commit) d79ce16] one Author: A U Thor 1 file changed, 1 insertion(+) create mode 100644 one.t ok 11 - redirected submodule add does not show progress ok 12 - redirected submodule add --progress does show progress ok 13 - submodule add to .gitignored path fails ... Fix the output by encapsulating the setup code in test_expect_success Signed-off-by: Stefan Beller --- t/t7400-submodule-basic.sh | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index 2f532529b82..812db137b8d 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -126,8 +126,10 @@ test_expect_success 'submodule add' ' test_cmp empty untracked ' -test_create_repo parent && -test_commit -C parent one +test_expect_success 'setup parent and one repository' ' + test_create_repo parent && + test_commit -C parent one +' test_expect_success 'redirected submodule add does not show progress' ' git -C addtest submodule add "file://$submodurl/parent" submod-redirected \ -- 2.18.0.rc1.244.gcf134e6275-goog
Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
> Jonathan Tan writes: > > >> Wouldn't that allow us not having to advertise the whole tags > >> namespace only to implement the tag following? > > > > Yes, it would, but as far as I can tell, it would add an extra burden on > > the server to walk all refs requested in the ls-refs call (in order to > > determine which tags to send back in the response). Also, this walk must > > be done before any negotiation (since this is a ls-refs call),... > > My comment was that I doubt the "must be done" part of the above. > How would refs-in-want be responded where client-supplied "I want > 'master' branch---I am not asking for the exact object the first > server I contacted said where the 'master' is at" gets turned into > "So the final value of 'master' among these servers that are not > quite in sync is this" by the one that gives you the pack, not > necessarily the one that responds to ls-refs upon initial contact? > Can't we do something similar, i.e. let the client say "I want tags > that refer to new objects you are going to send me, I do not know > what they are offhand" and the server that actually gives you the > pack to say "here are the tags I ended up including"? The > "include-tag" process to generate pack with extra objects (i.e. the > tags that point at packed objects) has to involve walking for > reachabliity anyway, so as long as the feature is supported, > somebody has to do the work, and if you want to cut down the > transfer cost of the refs/tags/* enumeration, it needs to happen on > the server end, no? Ah, I think I see. There are these possible worlds: (1) the current world (2) no ref-in-want, and upload-pack sends tag information as part of its response to ls-refs (3) no ref-in-want, but upload-pack can send ref information right before the packfile (4) ref-in-want, and upload-pack will send ref information right before the packfile I was only thinking about (2) and (4), but I think you are talking about (3). Yes, that would work, although I don't think it's worth the protocol churn to do (3) then (4), especially since we already have ref-in-want patches sent to the mailing list - but I should have discussed this option in my previous e-mails too. > Or perhaps v2 fetch should implement the automated tag following > without using include-tag and instead as an extended feature of > ref-in-want. I think that is merely giving a different name to the > same idea outlined above, though ;-) Instead of not using include-tag, I would define include-tag in the presence of want-refs to also include the refs, but I agree with this solution.
Re: What's cooking in git.git (Jun 2018, #05; Mon, 18)
Hi Jacob, > * sb/diff-color-move-more (2018-05-21) 8 commits > (merged to 'next' on 2018-05-24 at 45f3fb7975) > + diff: color-moved white space handling options imply color-moved > + diff.c: add --color-moved-ignore-space-delta option > + diff.c: decouple white space treatment from move detection algorithm > + diff.c: add a blocks mode for moved code detection > + diff.c: adjust hash function signature to match hashmap expectation > + diff.c: do not pass diff options as keydata to hashmap > + xdiff/xdiffi.c: remove unneeded function declarations > + xdiff/xdiff.h: remove unused flags > > "git diff --color-moved" feature has further been tweaked. > > Will kick back to 'pu'. > cf. FYI: I have this series still cooking internally, but it is not ready again for prime time, as I still need to debug a corner case. The code found at [1] is improved over this series here as the options for detecting moved code and its coloring are decoupled; having more tests. I just had not enough time to resend this one. [1] https://github.com/stefanbeller/git/tree/color-moved-only Thanks, Stefan
[PATCH] t3404: check root commit in 'rebase -i --root reword root commit'
When testing a reworded root commit, ensure that the squash-onto commit which is created and amended is still the root commit. Suggested-by: Phillip Wood Helped-by: Johannes Schindelin Signed-off-by: Todd Zullinger --- Hi Johannes, Johannes Schindelin wrote: >On Mon, 18 Jun 2018, Todd Zullinger wrote: >> Phillip Wood wrote: >>> On 15/06/18 05:31, Johannes Schindelin via GitGitGadget wrote: From: Todd Zullinger +test_expect_failure 'rebase -i --root reword root commit' ' + test_when_finished "test_might_fail git rebase --abort" && + git checkout -b reword-root-branch master && + set_fake_editor && + FAKE_LINES="reword 1 2" FAKE_COMMIT_MESSAGE="A changed" \ + git rebase -i --root && + git show HEAD^ | grep "A changed" >>> >>> I wonder if it should also check that HEAD^ is the root commit, to make >>> sure that the squash-onto commit that's created and then amended has >>> been squashed onto. >> >> Hmm, is that something which other tests don't cover or an >> issue that could affect 'rebase -i --root' with reword >> differently than other 'rebase -i' commands? >> >> I admit I'm not well-versed in the rebase -i tests and I >> focused only on creating a test which demonstrated the bug I >> noticed. > > I think we should test this here, to make sure it is tested, and it should > be as easy as: > >test -z "$(git show -s --format=%p HEAD^)" > > Hopefully you beat me to it, otherwise I will try to take care of this > tomorrow. With luck, this will save you a few minutes, assuming the commit message is reasonable (or can be improved with help from Phillip and others). :) Or Junio may just squash this onto js/rebase-i-root-fix. Thanks. t/t3404-rebase-interactive.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index e500d7c320..352a52e59d 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -977,7 +977,8 @@ test_expect_success 'rebase -i --root reword root commit' ' set_fake_editor && FAKE_LINES="reword 1 2" FAKE_COMMIT_MESSAGE="A changed" \ git rebase -i --root && - git show HEAD^ | grep "A changed" + git show HEAD^ | grep "A changed" && + test -z "$(git show -s --format=%p HEAD^)" ' test_expect_success C_LOCALE_OUTPUT 'rebase --edit-todo does not work on non-interactive rebase' ' -- Todd ~~ Anyone who is capable of getting themselves made President should on no account be allowed to do the job. -- Douglas Adams, "The Hitchhiker's Guide to the Galaxy"
Re: [GSoC][PATCH 2/3] rebase -i: rewrite setup_reflog_action() in C
On Mon, Jun 18, 2018 at 6:19 AM Alban Gruin wrote:
>
> This rewrites setup_reflog_action() from shell to C.
>
> A new command is added to rebase--helper.c, “setup-reflog”, as such as a
> new flag, “verbose”, to silence the output of the checkout operation
> called by setup_reflog_action().
>
> The shell version is then stripped in favour of a call to the helper. As
> $GIT_REFLOG_ACTION is not longer set at the first call of
> checkout_onto(), a call to comment_for_reflog() is added at the
> beginning of this function.
>
> Signed-off-by: Alban Gruin
> ---
> builtin/rebase--helper.c | 9 +++--
> git-rebase--interactive.sh | 16 ++--
> sequencer.c| 31 +++
> sequencer.h| 3 +++
> 4 files changed, 43 insertions(+), 16 deletions(-)
>
> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> index d2990b210..d677fb663 100644
> --- a/builtin/rebase--helper.c
> +++ b/builtin/rebase--helper.c
> @@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] =
> {
> int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
> {
> struct replay_opts opts = REPLAY_OPTS_INIT;
> - unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
> + unsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0;
> int abbreviate_commands = 0, rebase_cousins = -1;
> enum {
> CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
> CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
> - ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO
> + ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, SETUP_REFLOG
> } command = 0;
> struct option options[] = {
> OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
> @@ -27,6 +27,7 @@ int cmd_rebase__helper(int argc, const char **argv, const
> char *prefix)
> OPT_BOOL(0, "rebase-merges", _merges, N_("rebase merge
> commits")),
> OPT_BOOL(0, "rebase-cousins", _cousins,
> N_("keep original branch points of cousins")),
> + OPT_BOOL(0, "verbose", , N_("verbose")),
verbose is quite a popular flag name, such that the option parsing
dedicated it its own macro OPT__VERBOSE.
> +int setup_reflog_action(struct replay_opts *opts, const char *commit,
> + int verbose)
> +{
> + const char *action;
> +
> + if (commit && *commit) {
While this is defensive programming (checking the pointer before dereferencing
it, the first condition (commit == NULL) should never be false here,
as the caller
checks for argc == 2 ? Maybe we could move the logic of the whole
condition there
if (command == SETUP_REFLOG && argc == 2 && *argv[1])
return !!setup_reflog_action(, argv[1], verbose);
as then we could loose the outer conditional here.
Re: [PATCH 1/2] rebase --root: demonstrate a bug while amending rootcommit messages
Hi Todd, On Mon, 18 Jun 2018, Todd Zullinger wrote: > Phillip Wood wrote: > > On 15/06/18 05:31, Johannes Schindelin via GitGitGadget wrote: > >> > >> From: Todd Zullinger > >> > >> +test_expect_failure 'rebase -i --root reword root commit' ' > >> + test_when_finished "test_might_fail git rebase --abort" && > >> + git checkout -b reword-root-branch master && > >> + set_fake_editor && > >> + FAKE_LINES="reword 1 2" FAKE_COMMIT_MESSAGE="A changed" \ > >> + git rebase -i --root && > >> + git show HEAD^ | grep "A changed" > > > > I wonder if it should also check that HEAD^ is the root commit, to make > > sure that the squash-onto commit that's created and then amended has > > been squashed onto. > > Hmm, is that something which other tests don't cover or an > issue that could affect 'rebase -i --root' with reword > differently than other 'rebase -i' commands? > > I admit I'm not well-versed in the rebase -i tests and I > focused only on creating a test which demonstrated the bug I > noticed. I think we should test this here, to make sure it is tested, and it should be as easy as: test -z "$(git show -s --format=%p HEAD^)" Hopefully you beat me to it, otherwise I will try to take care of this tomorrow. Ciao, Dscho
Re: What's cooking in git.git (Jun 2018, #05; Mon, 18)
Hi Junio, Junio C Hamano wrote: > * tz/cred-netrc-cleanup (2018-06-18) 3 commits > - git-credential-netrc: fix exit status when tests fail > - git-credential-netrc: use in-tree Git.pm for tests > - git-credential-netrc: minor whitespace cleanup in test script > > Build and test procedure for netrc credential helper (in contrib/) > has been updated. It looks like 1/4 from that series didn't make it into the tz/cred-netrc-cleanup branch: git-credential-netrc: make "all" default target of Makefile, which is in <20180613031040.3109-2-...@pobox.com>. Thanks, -- Todd ~~ I used to think the brain was the most advanced part of the body. Then I realized, look what's telling me that. -- Emo Phillips
Re: [PATCH v2] sequencer: do not squash 'reword' commits when we hit conflicts
Hi Phillip,
On Mon, 18 Jun 2018, Phillip Wood wrote:
> On 17/06/18 20:28, Johannes Schindelin wrote:
> >
> > On Sun, 17 Jun 2018, Phillip Wood wrote:
> >
> >> On 17/06/18 06:37, Elijah Newren wrote:
> >>> Ever since commit 18633e1a22 ("rebase -i: use the rebase--helper
> >>> builtin", 2017-02-09), when a commit marked as 'reword' in an
> >>> interactive rebase has conflicts and fails to apply, when the rebase
> >>> is resumed that commit will be squashed into its parent with its
> >>> commit message taken.
> >>>
> >>> The issue can be understood better by looking at commit 56dc3ab04b
> >>> ("sequencer (rebase -i): implement the 'edit' command", 2017-01-02),
> >>> which introduced error_with_patch() for the edit command. For the
> >>> edit command, it needs to stop the rebase whether or not the patch
> >>> applies cleanly. If the patch does apply cleanly, then when it
> >>> resumes it knows it needs to amend all changes into the previous
> >>> commit. If it does not apply cleanly, then the changes should not
> >>> be amended. Thus, it passes !res (success of applying the 'edit'
> >>> commit) to error_with_patch() for the to_amend flag.
> >>>
> >>> The problematic line of code actually came from commit 04efc8b57c
> >>> ("sequencer (rebase -i): implement the 'reword' command", 2017-01-02).
> >>> Note that to get to this point in the code:
> >>>* !!res (i.e. patch application failed)
> >>>* item->command < TODO_SQUASH
> >>>* item->command != TODO_EDIT
> >>>* !is_fixup(item->command) [i.e. not squash or fixup]
> >>> So that means this can only be a failed patch application that is
> >>> either a pick, revert, or reword. For any of those cases we want a
> >>> new commit, so we should not set the to_amend flag.
> >>
> >> Unfortunately I'm not sure it's that simple. Looking and do_pick()
> >> sometimes reword amends HEAD and sometimes it does not. In the
> >> "normal" case then the commit is picked and committed with '--edit'.
> >> However when fast-forwarding the code fast forwards to the commit to
> >> be reworded and then amends it. If the root commit is being reworded
> >> it takes the same code path. While these cases cannot fail with
> >> conflicts, it is possible for the user to cancel the commit or for
> >> them to fail due to collisions with untracked files.
> >>
> >> If I remember correctly the shell version always picks the commit and
> >> then calls 'git commit --amend' afterwards which is less efficient
> >> but consistent.
> >>
> >> I'm afraid I don't have a simple solution for fixing this, as
> >> currently pick_commits() does not know if the commit was called with
> >> AMEND_MSG, I guess that means adding some kind of flag for do_pick()
> >> to set.
> >
> > Oh, you're right, the fast-forwarding path would pose a problem. I
> > think there is an easy way to resolve this, though: in the case that
> > we do want to amend the to-be-reworded commit, we simply have to see
> > whether HEAD points to the very same commit mentioned in the `reword`
> > command:
>
> That's clever, I think to get it to work for rewording the root commit,
> it will need to do something like comparing HEAD to squash-onto as well.
... because squash-onto is a fresh, empty root commit (to be "amended"
when a non-root commit is to be picked as a new root commit). Good point.
> > -- snip --
> > diff --git a/sequencer.c b/sequencer.c
> > index 2dad7041960..99d33d4e063 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -3691,10 +3691,22 @@ static int pick_commits(struct todo_list
> > *todo_list, struct replay_opts *opts)
> > intend_to_amend();
> > return error_failed_squash(item->commit,
> > opts,
> > item->arg_len, item->arg);
> > - } else if (res && is_rebase_i(opts) && item->commit)
> > + } else if (res && is_rebase_i(opts) &&
> > item->commit) {
> > + int to_amend = 0;
> > +
> > + if (item->command == TODO_REWORD) {
> > + struct object_id head;
> > +
> > + if (!get_oid("HEAD", ) &&
> > + !oidcmp(>commit->object.oid,
> > + ))
> > + to_amend = 1;
This would now become
if (!get_oid("HEAD", ) &&
(!oidcmp(>commit->object.oid,
) ||
(opts->have_squash_onto &&
!oidcmp(>squash_onto,
to_amend = 1;
This is awfully indented, so a better idea would
What's cooking in git.git (Jun 2018, #05; Mon, 18)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them. As -rc2 has slipped by a few days, the final is also slipping by a couple of days to give the last-minute fixes. Hopefully we can see the final one within 48 hours ;-) You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to "master"] * ab/cred-netrc-no-autodie (2018-06-13) 1 commit (merged to 'next' on 2018-06-14 at 68171b82a7) + git-credential-netrc: remove use of "autodie" Hotfix for contrib/ stuff broken by this cycle. * es/make-no-iconv (2018-06-15) 1 commit (merged to 'next' on 2018-06-18 at b53e9933c9) + Makefile: make NO_ICONV really mean "no iconv" "make NO_ICONV=NoThanks" did not override NEEDS_LIBICONV (i.e. linkage of -lintl, -liconv, etc. that are platform-specific tweaks), which has been corrected. * jk/ewah-bounds-check (2018-06-18) 2 commits (merged to 'next' on 2018-06-18 at bf606be1bb) + ewah: adjust callers of ewah_read_mmap() + ewah_read_mmap: bounds-check mmap reads (this branch is used by ds/ewah-cleanup.) The code to read compressed bitmap was not careful to avoid reading past the end of the file, which has been corrected. * jl/zlib-restore-nul-termination (2018-06-13) 1 commit (merged to 'next' on 2018-06-14 at 3fa108363e) + packfile: correct zlib buffer handling Make zlib inflate codepath more robust against versions of zlib that clobber unused portion of outbuf. * js/rebase-i-root-fix (2018-06-18) 2 commits (merged to 'next' on 2018-06-18 at a6a1cf01d5) + rebase --root: fix amending root commit messages + rebase --root: demonstrate a bug while amending root commit messages A regression to "rebase -i --root" introduced during this cycle has been fixed. * km/doc-workflows-typofix (2018-06-12) 1 commit (merged to 'next' on 2018-06-13 at 21e6a8e67b) + gitworkflows: fix grammar in 'Merge upwards' rule Typofix. * ks/branch-set-upstream (2018-06-18) 1 commit (merged to 'next' on 2018-06-18 at 83b0b87013) + t3200: clarify description of --set-upstream test A test title has been reworded to clarify it. * ld/git-p4-updates (2018-06-12) 6 commits (merged to 'next' on 2018-06-13 at 4f7e24b3c4) + git-p4: auto-size the block + git-p4: narrow the scope of exceptions caught when parsing an int + git-p4: raise exceptions from p4CmdList based on error from p4 server + git-p4: better error reporting when p4 fails + git-p4: add option to disable syncing of p4/master with p4 + git-p4: disable-rebase: allow setting this via configuration (this branch uses rm/p4-submit-with-commit-option.) "git p4" updates. * mw/doc-merge-enumfix (2018-06-14) 1 commit (merged to 'next' on 2018-06-14 at 7074d6d48e) + doc: update the order of the syntax `git merge --continue` Fix old merge glitch in Documentation during v2.13-rc0 era. * rd/comment-typofix-in-sha1-file (2018-06-04) 1 commit (merged to 'next' on 2018-06-13 at 38ef825556) + sha1-file.c: correct $GITDIR to $GIT_DIR in a comment In code comment typofix * rd/diff-options-typofix (2018-06-11) 1 commit (merged to 'next' on 2018-06-13 at a5aa58fa1b) + diff-options.txt: fix minor typos, font inconsistencies, in docs Typofix. * rd/doc-remote-tracking-with-hyphen (2018-06-13) 1 commit (merged to 'next' on 2018-06-14 at 013aa6912e) + Use hyphenated "remote-tracking branch" (docs and comments) Doc update. * rm/p4-submit-with-commit-option (2018-06-12) 1 commit (merged to 'next' on 2018-06-13 at d3a272c733) + git-p4: add options --commit and --disable-rebase (this branch is used by ld/git-p4-updates.) "git p4" updates. * sb/blame-color (2018-06-14) 1 commit (merged to 'next' on 2018-06-14 at f8cd824d4d) + blame: release string_list after use in parse_color_fields() Leakfix. * sg/t7406-chain-fix (2018-06-18) 1 commit (merged to 'next' on 2018-06-18 at 816d976ea6) + t7406-submodule-update: fix broken &&-chains Test fix. -- [New Topics] * en/rename-directory-detection-reboot (2018-06-18) 1 commit (merged to 'next' on 2018-06-18 at 95c454d3f4) + merge-recursive: use xstrdup() instead of fixed buffer Newly added codepath in merge-recursive had potential buffer overrun, which has been fixed. Will merge to 'master'. * tz/cred-netrc-cleanup (2018-06-18) 3 commits - git-credential-netrc: fix exit status when tests fail - git-credential-netrc: use in-tree Git.pm for tests - git-credential-netrc: minor whitespace cleanup in test script Build and test procedure for netrc credential helper (in contrib/) has been updated.
Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
Jonathan Tan writes: >> Wouldn't that allow us not having to advertise the whole tags >> namespace only to implement the tag following? > > Yes, it would, but as far as I can tell, it would add an extra burden on > the server to walk all refs requested in the ls-refs call (in order to > determine which tags to send back in the response). Also, this walk must > be done before any negotiation (since this is a ls-refs call),... My comment was that I doubt the "must be done" part of the above. How would refs-in-want be responded where client-supplied "I want 'master' branch---I am not asking for the exact object the first server I contacted said where the 'master' is at" gets turned into "So the final value of 'master' among these servers that are not quite in sync is this" by the one that gives you the pack, not necessarily the one that responds to ls-refs upon initial contact? Can't we do something similar, i.e. let the client say "I want tags that refer to new objects you are going to send me, I do not know what they are offhand" and the server that actually gives you the pack to say "here are the tags I ended up including"? The "include-tag" process to generate pack with extra objects (i.e. the tags that point at packed objects) has to involve walking for reachabliity anyway, so as long as the feature is supported, somebody has to do the work, and if you want to cut down the transfer cost of the refs/tags/* enumeration, it needs to happen on the server end, no? Or perhaps v2 fetch should implement the automated tag following without using include-tag and instead as an extended feature of ref-in-want. I think that is merely giving a different name to the same idea outlined above, though ;-)
Re: OAuth2 support in git?
On Mon, Jun 18, 2018 at 08:53:27AM -0700, Junio C Hamano wrote: > > Yeah, that will work for some cases. A few places it might not: > > > > - some people may want to provide this only in response to a 401 > > > > - some tokens may need to be refreshed, which would require interacting > >with a credential helper to do the rest of the oauth conversation > > > > - there's no good way to hide your token in secure storage (versus > >sticking it on the command-line or in a config file). > > And all of these three are what you get for free by building on the > credential helper framework, after extending it a bit so that the > filled credential structure can tell the http code to show it to the > other side as a bearer token, not a password or password hash. The > helper is asked to supply the auth material only after 401, which > covers both the first and the second points, and then keeping the > auth material in-core (e.g. cache--daemon) would be more secure > which covers the third point. Am I following you correctly? Yes, exactly. Even if the credential protocol itself doesn't learn about this feature, even a config option for "treat password as token to send via bearer" would help. The "how" of sending the token isn't secret, just the token itself. So everything else can just pretend it's a password (it's a little funny because I think there isn't a matching username, but you could probably get by with an empty one). That's all just off the top of my head without digging back into the code, nor running any experiments, of course. There may be some gotchas. :) -Peff
Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
> Jonathan Tan writes: > > > When performing tag following, in addition to using the server's > > "include-tag" capability to send tag objects (and emulating it if the > > server does not support that capability), "git fetch" relies upon the > > presence of refs/tags/* entries in the initial ref advertisement to > > locally create refs pointing to the aforementioned tag objects. When > > using protocol v2, refs/tags/* entries in the initial ref advertisement > > may be suppressed by a ref-prefix argument, leading to the tag object > > being downloaded, but the ref not being created. > > I wonder if it is reasonable to define the protocol v2 semantics of > "include-tag" request a bit differently. > > Unlike v0 protocol where the client iterates though the advertised > refs and see if objects at the tip of them, even if they weren't > what the client initially wanted to fetch, to find these unsolicited > followed tag objects, allow the server to say, "I've included some > objects you did not explicitly ask for, and here are the refs/tags/* > names you should place on them", somewhat similar to the way how the > ref-in-want thing would work (i.e. the client does not really ask > for just objects and decides what name to place on them---instead it > lets the server to make part of the decision). > > Wouldn't that allow us not having to advertise the whole tags > namespace only to implement the tag following? Yes, it would, but as far as I can tell, it would add an extra burden on the server to walk all refs requested in the ls-refs call (in order to determine which tags to send back in the response). Also, this walk must be done before any negotiation (since this is a ls-refs call), so the walk is done all the way to the commits without any parents (and so an overestimate of tags might be sent). It might be better to have this functionality with ref-in-want, not only because of its conceptual similarity, but because ref-in-want provides a way for us to send refs at packfile generation time. So it is more efficient (since the server has to iterate through all the refs anyway to include objects for include-tag, it can probably just remember which refs caused objects to be added and return them), and also, the list of tags will not include any tags that point to non-sent objects. So my current inclination is to let v2 "include-tag" have the same semantics as v0 "include-tag", and only provide automatic sending of the ref itself when we have ref-in-want.
Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
Jonathan Tan writes: > When performing tag following, in addition to using the server's > "include-tag" capability to send tag objects (and emulating it if the > server does not support that capability), "git fetch" relies upon the > presence of refs/tags/* entries in the initial ref advertisement to > locally create refs pointing to the aforementioned tag objects. When > using protocol v2, refs/tags/* entries in the initial ref advertisement > may be suppressed by a ref-prefix argument, leading to the tag object > being downloaded, but the ref not being created. I wonder if it is reasonable to define the protocol v2 semantics of "include-tag" request a bit differently. Unlike v0 protocol where the client iterates though the advertised refs and see if objects at the tip of them, even if they weren't what the client initially wanted to fetch, to find these unsolicited followed tag objects, allow the server to say, "I've included some objects you did not explicitly ask for, and here are the refs/tags/* names you should place on them", somewhat similar to the way how the ref-in-want thing would work (i.e. the client does not really ask for just objects and decides what name to place on them---instead it lets the server to make part of the decision). Wouldn't that allow us not having to advertise the whole tags namespace only to implement the tag following?
Re: [PATCH 03/23] midx: add midx builtin
On 6/18/2018 3:55 PM, Stefan Beller wrote: But as this is plumbing and users should not need to worry about it this is optional, I would think. The design document is also in 'Documentation/technical' instead of just 'Documentation/'. Do we have a pattern of linking to the technical documents? Apparently we do (and I was not aware of it): $ git -C Documentation/ grep link:technical git-credential.txt:23:link:technical/api-credentials.html[the Git credential API] for more git.txt:839:link:technical/api-index.html[Git API documentation]. gitcredentials.txt:184:link:technical/api-credentials.html[credentials API] for details. technical/http-protocol.txt:517:link:technical/pack-protocol.html technical/http-protocol.txt:518:link:technical/protocol-capabilities.html user-manual.txt:3220:found in link:technical/pack-format.html[pack format]. Thanks! I'll add some links.
Re: [PATCH 03/23] midx: add midx builtin
> > But as this is plumbing and users should not need to worry about it > > this is optional, I would think. > > The design document is also in 'Documentation/technical' instead of just > 'Documentation/'. Do we have a pattern of linking to the technical > documents? Apparently we do (and I was not aware of it): $ git -C Documentation/ grep link:technical git-credential.txt:23:link:technical/api-credentials.html[the Git credential API] for more git.txt:839:link:technical/api-index.html[Git API documentation]. gitcredentials.txt:184:link:technical/api-credentials.html[credentials API] for details. technical/http-protocol.txt:517:link:technical/pack-protocol.html technical/http-protocol.txt:518:link:technical/protocol-capabilities.html user-manual.txt:3220:found in link:technical/pack-format.html[pack format].
Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
On 06/18, Jonathan Tan wrote:
>
> This would cause different behavior. For example, if I run "git fetch
> refs/heads/master refs/tags/foo", I would expect tag following to work
> even if the tag's name does not start with refs/tags/foo.
I understand that some people *may* want tag following, but really its
confusing from an end user's standpoint. You can fetch a particular ref
and end up with a bunch of tags that you didn't want. Aside from that
my biggest issue is with performance. The ref filtering was added to
cut down on the number of refs sent from the server, now we're basically
adding them all back no matter what (unless a user goes and flips the
default to not fetch tags).
I think we're probably going to need some people other than us to
comment on how this should be handled.
>
> > > @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport
> > > *transport,
> > > refspec_ref_prefixes(>remote->fetch,
> > > _prefixes);
> > >
> > > if (ref_prefixes.argc &&
> > > - (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
> > > + (tags == TAGS_SET || tags == TAGS_DEFAULT)) {
> >
> > Oh, I see. This always asks for refs/tags/ whereas before we only
> > asked for them if there were *no* refspec given. Maybe we can
> > change this to
> >
> > refspec_any_item_contains("refs/tags/")
> >
> > instead of always asking for the tags?
> > (that function would need to be written; I guess for a short term bugfix
> > this is good enough)
>
> Same answer as above.
>
> > How is the tag following documented (i.e. when is the user least
> > surprised that we do not tag-follow all and unconditionally)?
>
> It's documented under the --no-tags option in the man page for git
> fetch. I'm not sure what you mean by the user being surprised.
--
Brandon Williams
Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
> okay. Thinking long term, we may want to introduce a capability that
> can filter the tag space, e.g. "want-refs-since refs/tags/*"
> as a client directive as then the client only asks for new (newly
> created/appearing) tags instead of all tags.
I don't think refs normally have a created/modified time.
> > since tag refs are sent by
> > default now, the test has been switched to using branch refs instead.
>
> That mismatches what I would expect to read below.
> I would expect the client to always ask for refs/tags/* now and
> instead of the server just giving them.
I don't understand why there is a mismatch, so I'll just answer all your
questions. Yes, the client indeed always asks for refs/tags/* now.
By the server just giving them, do you mean (1) giving them based on
what is being fetched, or (2) giving them as if refs/tags/* was sent? If
(1), this is possible, but requires the server to precompute all the
commits that would be sent (and this would be an overestimate, since
negotiation has not yet taken place). If (2), I think it's better to let
the client control this (since the client could just as easily want
--no-tags, then sending tags would be wasted).
> Oh, the problem is in that other test to restrict the refs/tags
> to *not* be sent?
The problem is that refs/tags/* is sent by default. Maybe adding
--no-tags would work (I haven't tried it, though), but I think it's
clearer for the test to just operate on branches, than to have --no-tags
and let the reader wonder what --no-tags has to do with what's being
tested.
> Maybe we can only ask for refs/tags/* if we do not have any
> "refs/tags/" on the CLI: if I invoke "git fetch refs/tags/v1"
> I would not get an advertisement for refs/tags/v2 but if I omit
> all tags from the refspec, I'd get all its advertisements (v1+v2)
This would cause different behavior. For example, if I run "git fetch
refs/heads/master refs/tags/foo", I would expect tag following to work
even if the tag's name does not start with refs/tags/foo.
> > @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport
> > *transport,
> > refspec_ref_prefixes(>remote->fetch,
> > _prefixes);
> >
> > if (ref_prefixes.argc &&
> > - (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
> > + (tags == TAGS_SET || tags == TAGS_DEFAULT)) {
>
> Oh, I see. This always asks for refs/tags/ whereas before we only
> asked for them if there were *no* refspec given. Maybe we can
> change this to
>
> refspec_any_item_contains("refs/tags/")
>
> instead of always asking for the tags?
> (that function would need to be written; I guess for a short term bugfix
> this is good enough)
Same answer as above.
> How is the tag following documented (i.e. when is the user least
> surprised that we do not tag-follow all and unconditionally)?
It's documented under the --no-tags option in the man page for git
fetch. I'm not sure what you mean by the user being surprised.
Re: [PATCH 02/23] midx: add midx format details to pack-format.txt
> >> + (C + 1) * 12 bytes providing the chunk offsets: > >> + First 4 bytes describe chunk id. Value 0 is a terminating > >> label. > >> + Other 8 bytes provide offset in current file for chunk to > >> start. > >> + (Chunks are provided in file-order, so you can infer the length > >> + using the next chunk position if necessary.) > > It is so nice to have the header also have 12 bytes, so it fits right into > > the > > lookup table. So an alternative point of view: > > > >If a chunk needs to store more than 8 bytes, we'll have an offset after > >the first 4 bytes that describe the chunk, otherwise you can store the 8 > > bytes > >of information directly after the 4 bytes. > > "MIDX" is a special chunk and must come first (does it?) and only once > >as it contains the version number. > > This sounds feasible, but unnecessarily complicated. I don't think any > other chunk will be this small. I was just writing it as a way to test if I really understood what you said in the doc, not as a suggestion to incorporate it. Stefan
Re: [PATCH 03/23] midx: add midx builtin
On 6/11/2018 5:02 PM, Stefan Beller wrote: Hi Derrick, On Thu, Jun 7, 2018 at 7:03 AM Derrick Stolee wrote: This new 'git midx' builtin will be the plumbing access for writing, reading, and checking multi-pack-index (MIDX) files. The initial implementation is a no-op. Let's talk about the name for a second: .idx files are written by git-index-pack or as part of git-pack-objects (which just calls write_idx_file as part of finish_tmp_packfile), and the name actually suggests it writes the index files. I have a hard time understanding what the git-midx command does[1]. With both commit graph as well as multi index we introduce a command that is centered around that concept (similar to git-remote or git-config that are centered around a concept, that is closely resembled by a file), but for indexes for packs it was integrated differently into Git. So I am not sure if I want to suggest to integrate it into the packfile commands as that doesn't really fit. But maybe we can have a name that is human readable instead of the file suffix? Maybe git multi-pack-index ? I suppose that eventually this command is not really used by users as it will be used by other porcelain commands in the background or even as part of repack/gc so I am not worried about a long name, but I'd be more worried about understandability. I'll use "git multi-pack-index" in v2. I'll keep "midx.c" in the root, though, if that is OK. [1] While these names are not perfect for the layman, it is okay? I am sure you are aware of https://git-man-page-generator.lokaltog.net/ I was not, and enjoyed that quite a bit. Thanks, -Stolee new file mode 100644 index 00..2bd886f1a2 --- /dev/null +++ b/Documentation/git-midx.txt @@ -0,0 +1,29 @@ +git-midx(1) + + +NAME + +git-midx - Write and verify multi-pack-indexes (MIDX files). The reading is done as part of all other commands. I like to think the 'read' verb is a subset of "verify" because we are checking for information about the MIDX, and mostly for tests or debugging. + + +SYNOPSIS + +[verse] +'git midx' [--object-dir ] + +DESCRIPTION +--- +Write or verify a MIDX file. + +OPTIONS +--- + +--object-dir :: + Use given directory for the location of Git objects. We check + /packs/multi-pack-index for the current MIDX file, and + /packs for the pack-files to index. + + Maybe we could have a SEE ALSO section that points at the explanation of multi index files? (c.f. man git-submodule that has a SEE ALSO gitsubmodules(7), gitmodules(5) explaining concepts(7) and the file(5)) But as this is plumbing and users should not need to worry about it this is optional, I would think. The design document is also in 'Documentation/technical' instead of just 'Documentation/'. Do we have a pattern of linking to the technical documents? Thanks, -Stolee
Re: [PATCH v2 1/2] t5702: test fetch with multiple refspecs at a time
On Mon, Jun 18, 2018 at 12:15 PM Jonathan Tan wrote: > > On Mon, Jun 18, 2018 at 11:30 AM, Stefan Beller wrote: > >> +test_expect_success 'fetch supports various ways of have lines' ' > >> + rm -rf server client trace && > > > > Can we move these deletions to test_when_finished of the previous(?) test > > as well as have them here in a test_when_finished line? > > I think that deleting them when necessary makes it more explicit, and > this also supports tests where a repository is set up and then used > over multiple tests (e.g. having separate "setup X" and "use X" > tests). and it is necessary at the end of each test, so that we minimize the dependencies between tests. In an ideal world you could run any one test in the file alone and would still pass. :) > > > This test is precise and easy to understand; the patch is > > Reviewed-by: Stefan Beller > > (considering the test_when_finished comment as > > an optional nit) > > Thanks. Do you have any comments about the performance issue that > Brandon brought up? Is that https://public-inbox.org/git/20180605212829.gg158...@google.com/ ? (otherwise I'd be missing the performance issue) For now I'd filter out tags only if a specific tag is requested (i.e. the user given refspec contains "refs/tags/" -> no tag following) and then later I'd try out a date(?) based capability that asks for new tags only. Note that you can create old tags by backdating them, so we'd have to ask the server for any update in its reflog for refs/tags...
Re: [PATCH 0/2] rebase --root: fix `reword` on a root commit
Todd Zullinger writes:
>> Offhand it is not clear from the proposed log message where the
>> original breakage happened, but if this is to fix a regression
>> between v2.17.0 and v2.18.0, then let's have it. As -rc2 slipped
>> for a few days, it is reasonable to delay the final by a couple of
>> days as well, if only to give the last minute fixes and translators
>> reasonable time to breathe.
>
> Perhaps replacing the first paragraph with this would make
> it clearer?
>
> Since 21d0764c82 ("rebase -i --root: let the sequencer handle even the
> initial part", 2018-05-04), when splitting a repository, running `git
> rebase -i --root` to reword the initial commit, Git dies with
>
> Alternately, a similar note could be added at the end.
>
> This regression was recently introduced in 21d0764c82 ("rebase -i
> --root: let the sequencer handle even the initial part", 2018-05-04).
These certainly are ways to require one less hop to the readers than
the original ;-) Having said that, I've already merged it down to
'next' and want to have these in 'master' before final, so no need
to further fix-up the log message anymore.
Thanks.
Re: [PATCH 03/23] midx: add midx builtin
On 6/7/2018 1:20 PM, Duy Nguyen wrote:
On Thu, Jun 7, 2018 at 4:03 PM, Derrick Stolee wrote:
diff --git a/Documentation/git-midx.txt b/Documentation/git-midx.txt
new file mode 100644
index 00..2bd886f1a2
--- /dev/null
+++ b/Documentation/git-midx.txt
@@ -0,0 +1,29 @@
+git-midx(1)
+
+
+NAME
+
+git-midx - Write and verify multi-pack-indexes (MIDX files).
No full stop. This head line is collected automatically with others
and its having a full stop while the rest does not looks strange/
diff --git a/builtin/midx.c b/builtin/midx.c
new file mode 100644
index 00..59ea92178f
--- /dev/null
+++ b/builtin/midx.c
@@ -0,0 +1,38 @@
+#include "builtin.h"
+#include "cache.h"
+#include "config.h"
+#include "git-compat-util.h"
You only need either cache.h or git-compat-util.h. If cache.h is here,
git-compat-util can be removed.
+#include "parse-options.h"
+
+static char const * const builtin_midx_usage[] ={
+ N_("git midx [--object-dir ]"),
+ NULL
+};
+
+static struct opts_midx {
+ const char *object_dir;
+} opts;
+
+int cmd_midx(int argc, const char **argv, const char *prefix)
+{
+ static struct option builtin_midx_options[] = {
+ { OPTION_STRING, 0, "object-dir", _dir,
For paths (including dir), OPTION_FILENAME may be a better option to
handle correctly when the command is run in a subdir. See df217ed643
(parse-opts: add OPT_FILENAME and transition builtins - 2009-05-23)
for more info.
Thanks for the pointer!
+ N_("dir"),
+ N_("The object directory containing set of packfile and pack-index
pairs.") },
Other help strings do not have full stop either (I only checked a
couple commands though)
Also, doesn't OPT_STRING() work here too (if you avoid OPTION_FILENAME
for some reason)?
+ OPT_END(),
+ };
+
+ if (argc == 2 && !strcmp(argv[1], "-h"))
+ usage_with_options(builtin_midx_usage, builtin_midx_options);
+
+ git_config(git_default_config, NULL);
+
+ argc = parse_options(argc, argv, prefix,
+builtin_midx_options,
+builtin_midx_usage, 0);
+
+ if (!opts.object_dir)
+ opts.object_dir = get_object_directory();
+
+ return 0;
+}
diff --git a/git.c b/git.c
index c2f48d53dd..400fadd677 100644
--- a/git.c
+++ b/git.c
@@ -503,6 +503,7 @@ static struct cmd_struct commands[] = {
{ "merge-recursive-theirs", cmd_merge_recursive, RUN_SETUP |
NEED_WORK_TREE | NO_PARSEOPT },
{ "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE |
NO_PARSEOPT },
{ "merge-tree", cmd_merge_tree, RUN_SETUP | NO_PARSEOPT },
+ { "midx", cmd_midx, RUN_SETUP },
If it's a plumbing and can take an --object-dir, then I don't think
you should require it to run in a repo (with RUN_SETUP).
RUN_SETUP_GENTLY may be better. You could even leave it empty here and
only call setup_git_directory() only when --object-dir is not set.
I agree. Good point. This could be run to maintain an alternate without
any .git folder.
{ "mktag", cmd_mktag, RUN_SETUP | NO_PARSEOPT },
{ "mktree", cmd_mktree, RUN_SETUP },
{ "mv", cmd_mv, RUN_SETUP | NEED_WORK_TREE },
Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
On Tue, Jun 5, 2018 at 2:41 PM Jonathan Tan wrote:
>
> When performing tag following, in addition to using the server's
> "include-tag" capability to send tag objects (and emulating it if the
> server does not support that capability), "git fetch" relies upon the
> presence of refs/tags/* entries in the initial ref advertisement to
> locally create refs pointing to the aforementioned tag objects. When
> using protocol v2, refs/tags/* entries in the initial ref advertisement
> may be suppressed by a ref-prefix argument, leading to the tag object
> being downloaded, but the ref not being created.
>
> Commit dcc73cf7ff ("fetch: generate ref-prefixes when using a configured
> refspec", 2018-05-18) ensured that "refs/tags/" is always sent as a ref
> prefix when "git fetch" is invoked with no refspecs, but not when "git
> fetch" is invoked with refspecs. Extend that functionality to make it
> work in both situations.
okay. Thinking long term, we may want to introduce a capability that
can filter the tag space, e.g. "want-refs-since refs/tags/*"
as a client directive as then the client only asks for new (newly
created/appearing) tags instead of all tags.
> This also necessitates a change another test which tested ref
> advertisement filtering using tag refs -
sounds plausible.
> since tag refs are sent by
> default now, the test has been switched to using branch refs instead.
That mismatches what I would expect to read below.
I would expect the client to always ask for refs/tags/* now and
instead of the server just giving them.
Oh, the problem is in that other test to restrict the refs/tags
to *not* be sent?
Maybe we can only ask for refs/tags/* if we do not have any
"refs/tags/" on the CLI: if I invoke "git fetch refs/tags/v1"
I would not get an advertisement for refs/tags/v2 but if I omit
all tags from the refspec, I'd get all its advertisements (v1+v2)
> Signed-off-by: Jonathan Tan
> ---
> builtin/fetch.c| 2 +-
> t/t5702-protocol-v2.sh | 24 +---
> 2 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index ea5b9669a..1f447f1e8 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport
> *transport,
> refspec_ref_prefixes(>remote->fetch,
> _prefixes);
>
> if (ref_prefixes.argc &&
> - (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
> + (tags == TAGS_SET || tags == TAGS_DEFAULT)) {
Oh, I see. This always asks for refs/tags/ whereas before we only
asked for them if there were *no* refspec given. Maybe we can
change this to
refspec_any_item_contains("refs/tags/")
instead of always asking for the tags?
(that function would need to be written; I guess for a short term bugfix
this is good enough)
How is the tag following documented (i.e. when is the user least
surprised that we do not tag-follow all and unconditionally)?
> argv_array_push(_prefixes, "refs/tags/");
> }
>
> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index 261e82b0f..b31b6d8d3 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -204,6 +204,7 @@ test_expect_success 'ref advertisment is filtered during
> fetch using protocol v2
> test_when_finished "rm -f log" &&
>
> test_commit -C file_parent three &&
> + git -C file_parent branch unwanted-branch three &&
>
> GIT_TRACE_PACKET="$(pwd)/log" git -C file_child -c protocol.version=2
> \
> fetch origin master &&
> @@ -212,9 +213,8 @@ test_expect_success 'ref advertisment is filtered during
> fetch using protocol v2
> git -C file_parent log -1 --format=%s >expect &&
> test_cmp expect actual &&
>
> - ! grep "refs/tags/one" log &&
> - ! grep "refs/tags/two" log &&
> - ! grep "refs/tags/three" log
> + grep "refs/heads/master" log &&
> + ! grep "refs/heads/unwanted-branch" log
> '
That makes sense so far.
>
> test_expect_success 'server-options are sent when fetching' '
> @@ -406,6 +406,24 @@ test_expect_success 'fetch supports various ways of have
> lines' '
> $(git -C server rev-parse completely-unrelated)
> '
>
> +test_expect_success 'fetch supports include-tag and tag following' '
> + rm -rf server client trace &&
test_when_finished ;)
> + git init server &&
> +
> + test_commit -C server to_fetch &&
> + git -C server tag -a annotated_tag -m message &&
> +
> + git init client &&
> + GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \
> + fetch "$(pwd)/server" to_fetch:to_fetch &&
> +
> + grep "fetch> ref-prefix to_fetch" trace &&
> + grep "fetch> ref-prefix refs/tags/" trace &&
> + grep "fetch> include-tag" trace &&
> +
> + git -C client cat-file -e $(git -C client rev-parse annotated_tag)
> +'
Makes sense.
Re: [PATCH v2 1/2] t5702: test fetch with multiple refspecs at a time
On Mon, Jun 18, 2018 at 11:30 AM, Stefan Beller wrote: >> +test_expect_success 'fetch supports various ways of have lines' ' >> + rm -rf server client trace && > > Can we move these deletions to test_when_finished of the previous(?) test > as well as have them here in a test_when_finished line? I think that deleting them when necessary makes it more explicit, and this also supports tests where a repository is set up and then used over multiple tests (e.g. having separate "setup X" and "use X" tests). > This test is precise and easy to understand; the patch is > Reviewed-by: Stefan Beller > (considering the test_when_finished comment as > an optional nit) Thanks. Do you have any comments about the performance issue that Brandon brought up?
Re: [PATCH 02/23] midx: add midx format details to pack-format.txt
On 6/11/2018 3:19 PM, Stefan Beller wrote:
Hi Derrick,
On Thu, Jun 7, 2018 at 7:03 AM Derrick Stolee wrote:
The multi-pack-index (MIDX) feature generalizes the existing pack-
index (IDX) feature by indexing objects across multiple pack-files.
Describe the basic file format, using a 12-byte header followed by
a lookup table for a list of "chunks" which will be described later.
The file ends with a footer containing a checksum using the hash
algorithm.
The header allows later versions to create breaking changes by
advancing the version number. We can also change the hash algorithm
using a different version value.
We will add the individual chunk format information as we introduce
the code that writes that information.
Signed-off-by: Derrick Stolee
---
Documentation/technical/pack-format.txt | 49 +
1 file changed, 49 insertions(+)
diff --git a/Documentation/technical/pack-format.txt
b/Documentation/technical/pack-format.txt
index 70a99fd142..17666b4bfc 100644
--- a/Documentation/technical/pack-format.txt
+++ b/Documentation/technical/pack-format.txt
@@ -252,3 +252,52 @@ Pack file entry: <+
corresponding packfile.
20-byte SHA-1-checksum of all of the above.
+
+== midx-*.midx files have the following format:
+
+The meta-index files refer to multiple pack-files and loose objects.
So is it meta or multi?
Good catch. We were calling this the meta-index internally before
changing to "multi-pack-index" (helps to not change the acronym).
+In order to allow extensions that add extra data to the MIDX, we organize
+the body into "chunks" and provide a lookup table at the beginning of the
+body. The header includes certain length values, such as the number of packs,
+the number of base MIDX files, hash lengths and types.
+
+All 4-byte numbers are in network order.
+
+HEADER:
+
+ 4-byte signature:
+ The signature is: {'M', 'I', 'D', 'X'}
+
+ 1-byte version number:
+ Git only writes or recognizes version 1
+
+ 1-byte Object Id Version
+ Git only writes or recognizes verion 1 (SHA-1)
s/verion/version/
+ 1-byte number (C) of "chunks"
+
+ 1-byte number (I) of base multi-pack-index files:
+ This value is currently always zero.
Oh? Are meta-index and multi-index files different things?
Not intended to be different things, but this number is related to
making the feature incremental.
+ 4-byte number (P) of pack files
+
+CHUNK LOOKUP:
+
+ (C + 1) * 12 bytes providing the chunk offsets:
+ First 4 bytes describe chunk id. Value 0 is a terminating label.
+ Other 8 bytes provide offset in current file for chunk to start.
+ (Chunks are provided in file-order, so you can infer the length
+ using the next chunk position if necessary.)
It is so nice to have the header also have 12 bytes, so it fits right into the
lookup table. So an alternative point of view:
If a chunk needs to store more than 8 bytes, we'll have an offset after
the first 4 bytes that describe the chunk, otherwise you can store the 8
bytes
of information directly after the 4 bytes.
"MIDX" is a special chunk and must come first (does it?) and only once
as it contains the version number.
This sounds feasible, but unnecessarily complicated. I don't think any
other chunk will be this small.
+ The remaining data in the body is described one chunk at a time, and
+ these chunks may be given in any order. Chunks are required unless
+ otherwise specified.
+
+CHUNK DATA:
+
+ (This section intentionally left incomplete.)
+
+TRAILER:
+
+ H-byte HASH-checksum of all of the above.
This means we have to rehash the whole file for updating its contents.
okay.
Re: [PATCH 15/15] cache.h: flip NO_THE_INDEX_COMPATIBILITY_MACROS switch
On 06/16, Nguyễn Thái Ngọc Duy wrote: > From now on, by default index compat macros are off because they could > hide the_index dependency. Only those in builtin can use it (and even > so should be avoided if possible). This is awesome! Now there aren't any compat macros left in the lib code and the next set of patches has a good base to work from the eliminate the_index (which I assume based on other mails from you will become part of the new USE_THE_INDEX_COMPATIBILITY_MACROS macro once its eliminated from the lib code). > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > attr.c | 1 - > builtin/add.c| 1 + > builtin/am.c | 1 + > builtin/check-attr.c | 1 + > builtin/check-ignore.c | 1 + > builtin/checkout-index.c | 1 + > builtin/checkout.c | 1 + > builtin/clean.c | 1 + > builtin/commit.c | 1 + > builtin/describe.c | 1 + > builtin/diff-files.c | 1 + > builtin/diff-index.c | 1 + > builtin/diff-tree.c | 1 + > builtin/diff.c | 1 + > builtin/fsck.c | 1 + > builtin/ls-files.c | 1 - > builtin/merge-index.c| 1 + > builtin/merge-ours.c | 1 + > builtin/merge.c | 1 + > builtin/mv.c | 1 + > builtin/pull.c | 1 + > builtin/read-tree.c | 1 + > builtin/reset.c | 1 + > builtin/rev-parse.c | 1 + > builtin/rm.c | 1 + > builtin/submodule--helper.c | 1 + > builtin/update-index.c | 1 + > cache.h | 2 +- > convert.c| 1 - > dir.c| 1 - > name-hash.c | 1 - > pathspec.c | 1 - > read-cache.c | 1 - > submodule.c | 1 - > t/helper/test-dump-untracked-cache.c | 1 + > t/helper/test-tool.h | 2 ++ > tree.c | 1 - > unpack-trees.c | 1 - > 38 files changed, 29 insertions(+), 11 deletions(-) > > diff --git a/attr.c b/attr.c > index 067fb9e0c0..d16625661d 100644 > --- a/attr.c > +++ b/attr.c > @@ -7,7 +7,6 @@ > * an insanely large number of attributes. > */ > > -#define NO_THE_INDEX_COMPATIBILITY_MACROS > #include "cache.h" > #include "config.h" > #include "exec-cmd.h" > diff --git a/builtin/add.c b/builtin/add.c > index 8a155dd41e..b93193c3fe 100644 > --- a/builtin/add.c > +++ b/builtin/add.c > @@ -3,6 +3,7 @@ > * > * Copyright (C) 2006 Linus Torvalds > */ > +#define USE_THE_INDEX_COMPATIBILITY_MACROS > #include "cache.h" > #include "config.h" > #include "builtin.h" > diff --git a/builtin/am.c b/builtin/am.c > index 2fc2d1e82c..28af59d183 100644 > --- a/builtin/am.c > +++ b/builtin/am.c > @@ -3,6 +3,7 @@ > * > * Based on git-am.sh by Junio C Hamano. > */ > +#define USE_THE_INDEX_COMPATIBILITY_MACROS > #include "cache.h" > #include "config.h" > #include "builtin.h" > diff --git a/builtin/check-attr.c b/builtin/check-attr.c > index 91444dc044..d9cebd5382 100644 > --- a/builtin/check-attr.c > +++ b/builtin/check-attr.c > @@ -1,3 +1,4 @@ > +#define USE_THE_INDEX_COMPATIBILITY_MACROS > #include "builtin.h" > #include "cache.h" > #include "config.h" > diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c > index ec9a959e08..599097304b 100644 > --- a/builtin/check-ignore.c > +++ b/builtin/check-ignore.c > @@ -1,3 +1,4 @@ > +#define USE_THE_INDEX_COMPATIBILITY_MACROS > #include "builtin.h" > #include "cache.h" > #include "config.h" > diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c > index a730f6a1aa..e38ad42dbf 100644 > --- a/builtin/checkout-index.c > +++ b/builtin/checkout-index.c > @@ -4,6 +4,7 @@ > * Copyright (C) 2005 Linus Torvalds > * > */ > +#define USE_THE_INDEX_COMPATIBILITY_MACROS > #include "builtin.h" > #include "config.h" > #include "lockfile.h" > diff --git a/builtin/checkout.c b/builtin/checkout.c > index 2e1d2376d2..2250611407 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -1,3 +1,4 @@ > +#define USE_THE_INDEX_COMPATIBILITY_MACROS > #include "builtin.h" > #include "config.h" > #include "checkout.h" > diff --git a/builtin/clean.c b/builtin/clean.c > index fad533a0a7..2258379ecc 100644 > --- a/builtin/clean.c > +++ b/builtin/clean.c > @@ -6,6 +6,7 @@ > * Based on git-clean.sh by Pavel Roskin > */ > > +#define USE_THE_INDEX_COMPATIBILITY_MACROS > #include "builtin.h" > #include "cache.h" > #include "config.h" > diff --git a/builtin/commit.c b/builtin/commit.c > index a842fea666..98a5b4a8c8 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@
Re: [GSoC] GSoC with git, week 7
Hello! On 18.06.2018 20:34, Alban Gruin wrote: Hi, I published a new blog post here: https://blog.pa1ch.fr/posts/2018/06/18/en/gsoc2018-week-7.html It’s shorter than the last one, but feel free to tell me what you think about it! :) Cheers, Alban Good job! My blog is also up: https://ungps.github.io/ Best, Paul
Re: [PATCH 01/23] midx: add design document
On 6/11/2018 3:04 PM, Stefan Beller wrote: On Thu, Jun 7, 2018 at 7:03 AM Derrick Stolee wrote: Signed-off-by: Derrick Stolee --- Documentation/technical/midx.txt | 109 +++ 1 file changed, 109 insertions(+) create mode 100644 Documentation/technical/midx.txt diff --git a/Documentation/technical/midx.txt b/Documentation/technical/midx.txt new file mode 100644 index 00..789f410d71 --- /dev/null +++ b/Documentation/technical/midx.txt @@ -0,0 +1,109 @@ +Multi-Pack-Index (MIDX) Design Notes + + +The Git object directory contains a 'pack' directory containing +packfiles (with suffix ".pack") and pack-indexes (with suffix +".idx"). The pack-indexes provide a way to lookup objects and +navigate to their offset within the pack, but these must come +in pairs with the packfiles. This pairing depends on the file +names, as the pack-index differs only in suffix with its pack- +file. While the pack-indexes provide fast lookup per packfile, +this performance degrades as the number of packfiles increases, +because abbreviations need to inspect every packfile and we are +more likely to have a miss on our most-recently-used packfile. +For some large repositories, repacking into a single packfile +is not feasible due to storage space or excessive repack times. This leads to the question how MIDX will cope with large repos or a large number of packs. As it is just an index and not a pack itself, I guess it is smaller by some orders of magnitude, such that it is ok for now. The MIDX file is only slightly larger than the union of the IDX files for those packfiles. +The multi-pack-index (MIDX for short) stores a list of objects +and their offsets into multiple packfiles. It contains: + +- A list of packfile names. +- A sorted list of object IDs. +- A list of metadata for the ith object ID including: + - A value j referring to the jth packfile. + - An offset within the jth packfile for the object. +- If large offsets are required, we use another list of large + offsets similar to version 2 pack-indexes. + +Thus, we can provide O(log N) lookup time for any number +of packfiles. This sounds great for the lookup case! Though that is for the repo-read case. Let's read on how the dynamics of a repository are dealt with, e.g. integrating new packs into the MIDX, or how we deal with objects in multiple packs. + +Design Details +-- + +- The MIDX is stored in a file named 'multi-pack-index' in the + .git/objects/pack directory. This could be stored in the pack + directory of an alternate. It refers only to packfiles in that + same directory. So there is one and only one multi pack index? That makes the case of preparing the next MIDX that contains more pack references more interesting, as then we have to atomically update that file. There is only one, but we can make the file incremental without changing this name similar to how the split index works. +- The core.midx config setting must be on to consume MIDX files. Looking through current config options, I would rename this to a more suggestive name. I searched for the core.idx counterpart that enables idx files -- it turns out that is named pack.indexVersion. So maybe pack.MultiIndex ? That could start out as a boolean as in this series and then evolve into a version number or such later. I'll use that name and rename this file to Documentation/technical/multi-pack-index.txt +- The file format includes parameters for the object ID hash + function, so a future change of hash algorithm does not require + a change in format. + +- The MIDX keeps only one record per object ID. If an object appears + in multiple packfiles, then the MIDX selects the copy in the most- + recently modified packfile. Okay. That answers the question from above. Though this is just the tie breaking decision and not a hard limitation? (i.e. we could change this this later to that pack that has e.g. shortest delta chain for that object or such) This is a soft requirement. It is an easy thing to track at the moment. We can compute the MIDX without opening a packfile, for instance. +- If there exist packfiles in the pack directory not registered in + the MIDX, then those packfiles are loaded into the `packed_git` + list and `packed_git_mru` cache. Not sure I understand the implications of this? Does that mean we first look at the multi index and if an object is not found, we'll search linearly through all packs that are not part of the MIDX? That would require the MIDX to be kepot up to date reasonably to be useful. If you add a packfile to the pack directory, you can immediately start consuming it. You do not need to wait for the MIDX to be updated. The more asynchronous these auxiliary data structures (MIDX, commit-graph) can be, the better. This is in direct contrast to the reachability bitmap which is useless without its corresponding packfile. +- The
Re: [PATCH 00/15] Kill the_index part 1, expose it
On 06/17, Duy Nguyen wrote: > On Sun, Jun 17, 2018 at 9:02 AM Elijah Newren wrote: > > > > On Fri, Jun 15, 2018 at 10:41 PM, Nguyễn Thái Ngọc Duy > > wrote: > > > This is the beginning of the end of the_index. The problem with > > > the_index is it lets library code anywhere access it freely. This is > > > not good because from high level you may not realize that the_index is > > > being used while you don't want to touch index at all, or you want to > > > use a different index instead. > > > > > > This is a long series, 86 patches [1], so I'm going to split and > > > submit it in 15-20 patches at a time. The first two parts are trivial > > > though and could be safely fast tracked if needed. > > > > You post this small little patch about unpack-trees.c, mentioning you > > don't know if it's even correct, and bait me into reviewing it and > > then spring on me that it's actually nearly 100 patches that need > > review... Very sneaky. ;-) > > To be fair, it's all Brandon's fault. If he didn't kick the_index out > of dir.c, it would not conflict with one of my out-of-tree patches in > unpack-trees.c, catch my attention and make me go down this rabbit > hole :D Haha well this is something I've wanted to do for over a year now, glad you've decided to run with it :) I guess I've gotten pretty good at getting people to go down rabbit holes. First Stefan with the object store refactoring and now you with the index stuff. All because I wanted git to be more object oriented and have less global state ;) -- Brandon Williams
Re: [PATCH v2 1/2] t5702: test fetch with multiple refspecs at a time
On Tue, Jun 5, 2018 at 2:40 PM Jonathan Tan wrote:
>
> Extend the protocol v2 tests to also test fetches with multiple refspecs
> specified. This also covers the previously uncovered cases of fetching
> with prefix matching and fetching by SHA-1.
>
> Signed-off-by: Jonathan Tan
> ---
> t/t5702-protocol-v2.sh | 47 ++
> 1 file changed, 47 insertions(+)
>
> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index a4fe6508b..261e82b0f 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -359,6 +359,53 @@ test_expect_success 'default refspec is used to filter
> ref when fetchcing' '
> grep "ref-prefix refs/tags/" log
> '
>
> +test_expect_success 'fetch supports various ways of have lines' '
> + rm -rf server client trace &&
Can we move these deletions to test_when_finished of the previous(?) test
as well as have them here in a test_when_finished line?
> + git init server &&
> + test_commit -C server dwim &&
> + TREE=$(git -C server rev-parse HEAD^{tree}) &&
> + git -C server tag exact \
> + $(git -C server commit-tree -m a "$TREE") &&
> + git -C server tag dwim-unwanted \
> + $(git -C server commit-tree -m b "$TREE") &&
> + git -C server tag exact-unwanted \
> + $(git -C server commit-tree -m c "$TREE") &&
> + git -C server tag prefix1 \
> + $(git -C server commit-tree -m d "$TREE") &&
> + git -C server tag prefix2 \
> + $(git -C server commit-tree -m e "$TREE") &&
> + git -C server tag fetch-by-sha1 \
> + $(git -C server commit-tree -m f "$TREE") &&
> + git -C server tag completely-unrelated \
> + $(git -C server commit-tree -m g "$TREE") &&
So you create different commits all with the same tree; no parents;
this seems to convey nicely that this is about testing tags.
> + git init client &&
> + GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \
> + fetch "file://$(pwd)/server" \
> + dwim \
> + refs/tags/exact \
> + refs/tags/prefix*:refs/tags/prefix* \
> + "$(git -C server rev-parse fetch-by-sha1)" &&
> +
> + # Ensure that the appropriate prefixes are sent (using a sample)
> + grep "fetch> ref-prefix dwim" trace &&
> + grep "fetch> ref-prefix refs/heads/dwim" trace &&
> + grep "fetch> ref-prefix refs/tags/prefix" trace &&
> +
> + # Ensure that the correct objects are returned
> + git -C client cat-file -e $(git -C server rev-parse dwim) &&
> + git -C client cat-file -e $(git -C server rev-parse exact) &&
> + git -C client cat-file -e $(git -C server rev-parse prefix1) &&
> + git -C client cat-file -e $(git -C server rev-parse prefix2) &&
> + git -C client cat-file -e $(git -C server rev-parse fetch-by-sha1) &&
> + test_must_fail git -C client cat-file -e \
> + $(git -C server rev-parse dwim-unwanted) &&
> + test_must_fail git -C client cat-file -e \
> + $(git -C server rev-parse exact-unwanted) &&
> + test_must_fail git -C client cat-file -e \
> + $(git -C server rev-parse completely-unrelated)
> +'
This test is precise and easy to understand; the patch is
Reviewed-by: Stefan Beller
(considering the test_when_finished comment as
an optional nit)
Re: Adding nested repository with slash adds files instead of gitlink
On 06/18, Duy Nguyen wrote: > On Mon, Jun 18, 2018 at 1:23 PM Heiko Voigt wrote: > > > > Hi, > > > > I just discovered that when you have a slash at the end of a nested > > repository, the files contained in the repository get added instead of > > the gitlink. > > > > I found this when I was adding a submodule and wanted to commit a small > > change before that. You get the slash by using tab autocompletion. > > > > Here is a recipe to reproduce: > > > > mkdir test > > cd test; git init > > touch a; git add a; git commit -m a > > mkdir ../test.git; (cd ../test.git; git init --bare) > > git remote add origin ../test.git > > git push origin master > > git submodule add ../test.git submodule > > git reset > > git add submodule/ > > > > Now instead of just submodule gitlink there is an entry for submodule/a > > in the index. > > > > I just thought I put this out there. Will have a look if I find the time > > to cook up a proper testcase and investigate. > > This sounds like the submodule specific code in pathspec.c, which has > been replaced with something else in bw/pathspec-sans-the-index. If > you have time, try a version without those changes (e.g. v2.13 or > before) to see if it's a possible culprit. I just tested this with v2.13 and saw the same issue. I don't actually think this ever worked in the way you want it to Heiko. Maybe git add needs to be taught to be more intelligent when trying to add a submodule which doesn't exist in the index. -- Brandon Williams
Re: [PATCH v2 0/8] ref-in-want
On 06/15, Junio C Hamano wrote: > Brandon Williams writes: > > > Changes in v2: > > * issuing a want-ref line to a ref which doesn't exist is just ignored. > > * fixed some typos > > Do I lock some (logical) prerequisite changes? On 2.18-rc2 they > apply cleanly but the series fails its own test. No this is an error I made in this version of the series which another reviewer pointed out, I have a local v3 which addresses this (by removing the test since it isn't necessary anymore). Sorry for the mistake :) > > t5703-upload-pack-ref-in-want.sh > expecting success: > test-pkt-line pack >in <<-EOF && > command=fetch > 0001 > no-progress > want-ref refs/heads/non-existent > done > > EOF > > test_must_fail git serve --stateless-rpc 2>out grep "unknown ref" out > > test_must_fail: command succeeded: git serve --stateless-rpc > not ok 3 - invalid want-ref line -- Brandon Williams
Re: [PATCH] t7406-submodule-update: fix broken &&-chains
On Sat, Jun 16, 2018 at 1:33 PM SZEDER Gábor wrote: > > Three tests in 't7406-submodule-update' contain broken &&-chains, but > since they are all in subshells, chain-lint couldn't notice them. > > Signed-off-by: SZEDER Gábor This looks good to me; Thanks for spotting and fixing it, Stefan
Re: need for `git submodule update` over `git pull --recurse-submodules`?
On Sun, Jun 17, 2018 at 8:41 PM Shriramana Sharma wrote: > Do I need to execute any `git submodule` commands separately even if I > do `git pull --recurse-submodules`? Ideally you don't need "git submodule" commands any more, the rest of git is slowly converging to have builtin submodule functionality. > All I want is to stay in sync with > the cloned repo. That should just work with "pull --recurse" > But if so what is the use of `git submodule update --recursive` or > `git submodule update --remote --rebase` or such, which is somewhat > confusing to me I'm sorry to say. The git-submodule command was the first command implemented that dealt with submodules. In the beginning there was no "git pull --recurse" but the only way was to run "git submodule update" to change the state of submodules. Now there are better ways to do that, such as the recursive pull. Hope that helps, Stefan
[GSoC] GSoC with git, week 7
Hi, I published a new blog post here: https://blog.pa1ch.fr/posts/2018/06/18/en/gsoc2018-week-7.html It’s shorter than the last one, but feel free to tell me what you think about it! :) Cheers, Alban
Re: [GSoC][PATCH 1/3] sequencer: add a new function to silence a command, except if it fails.
Hi Christian, Le 18/06/2018 à 18:26, Christian Couder a écrit : > Hi Alban, > > On Mon, Jun 18, 2018 at 3:18 PM, Alban Gruin wrote: >> This adds a new function, run_command_silent_if_successful(), > > He re the function is called run_command_silent_if_successful()... > >> to >> redirect the stdout and stderr of a command to a strbuf, and then to run >> that command. This strbuf is printed only if the command fails. It is >> functionnaly similar to output() from git-rebase.sh. >> >> run_git_commit() is then refactored to use of >> run_command_silent_if_successful(). > > ...here also... > > [...] > >> +static int run_command_silent_on_success(struct child_process *cmd) > > ...but here it is called run_command_silent_on_success(). > > Thanks, > Christian. > Oops, my bad. I will fix this in a reroll. Cheers, Alban
Re: [GSoC][PATCH 2/3] rebase -i: rewrite setup_reflog_action() in C
Hi Phillip, Le 18/06/2018 à 17:34, Phillip Wood a écrit : > On 18/06/18 14:18, Alban Gruin wrote: >> This rewrites setup_reflog_action() from shell to C. >> >> A new command is added to rebase--helper.c, “setup-reflog”, as such as a >> new flag, “verbose”, to silence the output of the checkout operation >> called by setup_reflog_action(). > > I'm having difficulty understanding what that means, surely the verbose > flag is to stop the output from being silenced. > I reversed the meaning of the flag, I will correct it in a reroll. > I'm not that keen on the naming in this patch, if it's only a staging > post and will be removed it probably doesn't matter too much. I can see > why you've based the function and flag names on the shell version, but > the C version is not setting up the reflog it is checking out the branch > we're rebasing. --checkout-base or something like that would be clearer. > > Also the name of the function that does the checkout is tied to checking > out the base revision, but then reused in the next patch to checkout the > 'onto' commit. As such I think it would be clearer if it was called > run_git_checkout() or something like that. > Right. > One further thought - how easy would it be to refactor the code in > do_reset() to handle the checkouts in this patch series, rather than > having to fork 'git checkout' > Good remark, I did not notice do_reset(). I intend to refactor and optimize after I have done (most of) the conversion, so this goes into my todo-list, but it does not really seem difficult at first sight. Cheers, Alban
Re: [GSoC][PATCH 3/3] rebase -i: rewrite checkout_onto() in C
Hi Phillip,
Le 18/06/2018 à 18:09, Phillip Wood a écrit :
>> + if (get_oid(orig_head, ))
>> + die(_("%s: not a valid OID"), orig_head);
>
> If this code is going to live long-term in sequencer.c then it would be
> better not to die, but return an error instead as it's part of libgit.
>
Right, I will fix this.
Cheers,
Alban
Re: [PATCH 1/2] rebase --root: demonstrate a bug while amending rootcommit messages
Hi Phillip, Phillip Wood wrote: > On 15/06/18 05:31, Johannes Schindelin via GitGitGadget wrote: >> >> From: Todd Zullinger >> >> +test_expect_failure 'rebase -i --root reword root commit' ' >> +test_when_finished "test_might_fail git rebase --abort" && >> +git checkout -b reword-root-branch master && >> +set_fake_editor && >> +FAKE_LINES="reword 1 2" FAKE_COMMIT_MESSAGE="A changed" \ >> +git rebase -i --root && >> +git show HEAD^ | grep "A changed" > > I wonder if it should also check that HEAD^ is the root commit, to make > sure that the squash-onto commit that's created and then amended has > been squashed onto. Hmm, is that something which other tests don't cover or an issue that could affect 'rebase -i --root' with reword differently than other 'rebase -i' commands? I admit I'm not well-versed in the rebase -i tests and I focused only on creating a test which demonstrated the bug I noticed. -- Todd ~~ The average woman would rather be beautiful than smart because the average man can see better than he can think.
Re: [GSoC][PATCH 1/3] sequencer: add a new function to silence a command, except if it fails.
Hi Phillip, Le 18/06/2018 à 17:26, Phillip Wood a écrit : > Hi Alban > > On 18/06/18 14:18, Alban Gruin wrote: >> This adds a new function, run_command_silent_if_successful(), to >> redirect the stdout and stderr of a command to a strbuf, and then to run >> that command. This strbuf is printed only if the command fails. It is >> functionnaly similar to output() from git-rebase.sh. > > s/functionnaly/functionally/ > > It's not quite the same though because the shell versions handles > --verbose where as here the caller has to put that check in every call > site. I wonder if it would simplify the callers if the C version did the > --verbose handling it's self. > That’s what I did first, but removed it because I thought it would be less confusing. I’m fine with this solution, though. Do you want me to do this instead? Cheers, Alban
Re: [PATCH 0/2] rebase --root: fix `reword` on a root commit
Hi,
Junio C Hamano wrote:
> Todd Zullinger writes:
>
>> Hi Johannes,
>>
>> Johannes Schindelin via GitGitGadget wrote:
>>> From: GitGitGadget
>>>
>>> Todd Zullinger reported this bug in
>>> https://public-inbox.org/git/20180615043111.gs3...@zaya.teonanacatl.net/:
>>> when calling git rebase --root and trying to reword the
>>> root commit's message, a BUG is reported.
>>>
>>> This fixes that.
>>>
>>> IMO the bug fix is trivial enough to qualify for inclusion into v2.18.0,
>>> still.
>>
>> It does indeed fix the issue. I agree it would be nice to
>> see it in 2.18.0. As a fix for a minor regression
>> introduced in this cycle, that seems reasonable.
>
> Offhand it is not clear from the proposed log message where the
> original breakage happened, but if this is to fix a regression
> between v2.17.0 and v2.18.0, then let's have it. As -rc2 slipped
> for a few days, it is reasonable to delay the final by a couple of
> days as well, if only to give the last minute fixes and translators
> reasonable time to breathe.
Perhaps replacing the first paragraph with this would make
it clearer?
Since 21d0764c82 ("rebase -i --root: let the sequencer handle even the
initial part", 2018-05-04), when splitting a repository, running `git
rebase -i --root` to reword the initial commit, Git dies with
Alternately, a similar note could be added at the end.
This regression was recently introduced in 21d0764c82 ("rebase -i
--root: let the sequencer handle even the initial part", 2018-05-04).
--
Todd
Re: [GSoC][PATCH 1/3] sequencer: add a new function to silence a command, except if it fails.
Hi Alban, On Mon, Jun 18, 2018 at 3:18 PM, Alban Gruin wrote: > This adds a new function, run_command_silent_if_successful(), He re the function is called run_command_silent_if_successful()... > to > redirect the stdout and stderr of a command to a strbuf, and then to run > that command. This strbuf is printed only if the command fails. It is > functionnaly similar to output() from git-rebase.sh. > > run_git_commit() is then refactored to use of > run_command_silent_if_successful(). ...here also... [...] > +static int run_command_silent_on_success(struct child_process *cmd) ...but here it is called run_command_silent_on_success(). Thanks, Christian.
Re: [PATCH 0/2] rebase --root: fix `reword` on a root commit
Todd Zullinger writes: > Hi Johannes, > > Johannes Schindelin via GitGitGadget wrote: >> From: GitGitGadget >> >> Todd Zullinger reported this bug in >> https://public-inbox.org/git/20180615043111.gs3...@zaya.teonanacatl.net/: >> when calling git rebase --root and trying to reword the >> root commit's message, a BUG is reported. >> >> This fixes that. >> >> IMO the bug fix is trivial enough to qualify for inclusion into v2.18.0, >> still. > > It does indeed fix the issue. I agree it would be nice to > see it in 2.18.0. As a fix for a minor regression > introduced in this cycle, that seems reasonable. Offhand it is not clear from the proposed log message where the original breakage happened, but if this is to fix a regression between v2.17.0 and v2.18.0, then let's have it. As -rc2 slipped for a few days, it is reasonable to delay the final by a couple of days as well, if only to give the last minute fixes and translators reasonable time to breathe. Thanks. > >> Johannes Schindelin (1): >> rebase --root: fix amending root commit messages >> >> Todd Zullinger (1): >> rebase --root: demonstrate a bug while amending root commit messages >> >> sequencer.c | 2 +- >> t/t3404-rebase-interactive.sh | 9 + >> 2 files changed, 10 insertions(+), 1 deletion(-) >> >> >> base-commit: 68372c88794aba15f853542008cda39def768372
Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv"
Eric Sunshine writes: > On Sun, Jun 17, 2018 at 1:32 PM Kaartic Sivaraam > wrote: >> On Friday 15 June 2018 01:13 PM, Eric Sunshine wrote: >> > On Fri, Jun 15, 2018 at 2:58 AM Simon Ruderich wrote: >> >> Should we put the part about MacOS's make into the commit >> >> message? Seems like relevant information for future readers. >> > >> > No. The bit of commentary mentioning MacOS's very old 'make' was just >> > talking about a possible alternate way of implementing the change. >> > That alternative was not chosen, so talking about old 'make' in the >> > commit message would be confusing for readers. >> >> Interesting. Documentation/SubmittinPatches reads: >> >> The body should provide a meaningful commit message, which: >> >> . alternate solutions considered but discarded, if any. >> >> The consensus has changed, maybe? In which case, should we remove that >> statement from there? > > Whether or not to talk about alternate solutions in the commit message > is a judgment call. Same for deciding what belongs in the commit > message proper and what belongs in the "commentary" section of a > patch. A patch author should strive to convey the problem succinctly > in the commit message, to not overload the reader with unnecessary (or > confusing) information, while, at the same time, not be sparing with > information which is genuinely needed to understand the problem and > solution. > > Often, this can be done without talking about alternatives; often even > without spelling out the solution in detail or at all since the > solution may be "obvious", given a well-written problem description. > Complex cases, or cases in which multiple solutions may be or seem > valid, on the other hand, might warrant talking about those alternate > solutions, so we probably don't want to drop that bullet point. > Perhaps, instead, it can be re-worded a bit to make it sound something > other than mandatory (but I can't think of a good way to phrase it; > maybe you can?). Yup, "if any" is a bad thing to say, as it does not set the bar for that "any" random garbage idea. A phrase like "when appropriate" is a relatively safe but mostly useless cop-out, as these guidelines are written primarily for those who don't yet have proper yardsticks to gauge what is appropriate and what isn't. I think it maybe better to either drop it or make it a sample way to do the second point, i.e. if there are seemingly valid alternative which may entice readers, explaining why the alternative does not work well and the solution you chose works better *is* a good way to justify the way you chose in your change. Off the top of my head, something like this? I am not very happy with the text, though. Documentation/SubmittingPatches | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index 2488544407..4294d0f068 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -125,10 +125,12 @@ The body should provide a meaningful commit message, which: . explains the problem the change tries to solve, i.e. what is wrong with the current code without the change. -. justifies the way the change solves the problem, i.e. why the - result with the change is better. +. justifies the way the change solves the problem, i.e. why the result + with the change is better (e.g. explaining the reason why an + seemingly obvious alternative does not work but the solution in the + patch does may be a good way to illustrate the nature of the problem + and how your approach fits it better). -. alternate solutions considered but discarded, if any. [[imperative-mood]] Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
Re: [GSoC][PATCH 3/3] rebase -i: rewrite checkout_onto() in C
Hi Alban
On 18/06/18 14:18, Alban Gruin wrote:
This rewrites checkout_onto() from shell to C.
A new command (“checkout-onto”) is added to rebase--helper.c. The shell
version is then stripped.
Signed-off-by: Alban Gruin
---
builtin/rebase--helper.c | 7 ++-
git-rebase--interactive.sh | 25 -
sequencer.c| 19 +++
sequencer.h| 3 +++
4 files changed, 32 insertions(+), 22 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index d677fb663..f9fffba96 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -17,7 +17,8 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
- ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, SETUP_REFLOG
+ ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, SETUP_REFLOG,
+ CHECKOUT_ONTO
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -53,6 +54,8 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
EDIT_TODO),
OPT_CMDMODE(0, "setup-reflog", ,
N_("setup the reflog action"), SETUP_REFLOG),
+ OPT_CMDMODE(0, "checkout-onto", ,
+ N_("checkout a commit"), CHECKOUT_ONTO),
OPT_END()
};
@@ -98,5 +101,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
return !!edit_todo_list(flags);
if (command == SETUP_REFLOG && argc == 2)
return !!setup_reflog_action(, argv[1], verbose);
+ if (command == CHECKOUT_ONTO && argc == 4)
+ return !!checkout_onto(, argv[1], argv[2], argv[3],
verbose);
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 048bbf041..0ae053291 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -28,17 +28,6 @@ case "$comment_char" in
;;
esac
-orig_reflog_action="$GIT_REFLOG_ACTION"
-
-comment_for_reflog () {
- case "$orig_reflog_action" in
- ''|rebase*)
- GIT_REFLOG_ACTION="rebase -i ($1)"
- export GIT_REFLOG_ACTION
- ;;
- esac
-}
-
die_abort () {
apply_autostash
rm -rf "$state_dir"
@@ -70,14 +59,6 @@ collapse_todo_ids() {
git rebase--helper --shorten-ids
}
-# Switch to the branch in $into and notify it in the reflog
-checkout_onto () {
- comment_for_reflog start
- GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
- output git checkout $onto || die_abort "$(gettext "could not detach
HEAD")"
- git update-ref ORIG_HEAD $orig_head
-}
-
get_missing_commit_check_level () {
check_level=$(git config --get rebase.missingCommitsCheck)
check_level=${check_level:-ignore}
@@ -176,7 +157,8 @@ EOF
git rebase--helper --check-todo-list || {
ret=$?
- checkout_onto
+ git rebase--helper --checkout-onto "$onto_name" "$onto" \
+ "$orig_head" ${verbose:+--verbose}
exit $ret
}
@@ -186,7 +168,8 @@ EOF
onto="$(git rebase--helper --skip-unnecessary-picks)" ||
die "Could not skip unnecessary pick commands"
- checkout_onto
+ git rebase--helper --checkout-onto "$onto_name" "$onto" "$orig_head" \
+ ${verbose:+--verbose}
require_clean_work_tree "rebase"
exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
--continue
diff --git a/sequencer.c b/sequencer.c
index 4bfe29c7b..d149cbf92 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3162,6 +3162,25 @@ int setup_reflog_action(struct replay_opts *opts, const
char *commit,
return 0;
}
+int checkout_onto(struct replay_opts *opts,
+ const char *onto_name, const char *onto,
+ const char *orig_head, unsigned verbose)
+{
+ struct object_id oid;
+ const char *action = reflog_message(opts, "start", "checkout %s",
onto_name);
+
+ if (get_oid(orig_head, ))
+ die(_("%s: not a valid OID"), orig_head);
If this code is going to live long-term in sequencer.c then it would be
better not to die, but return an error instead as it's part of libgit.
Best Wishes
Phillip
+
+ if (checkout_base_commit(opts, onto, verbose, action)) {
+ apply_autostash(opts);
+ sequencer_remove_state(opts);
+ die(_("could not detach HEAD"));
+ }
+
+ return update_ref(NULL, "ORIG_HEAD", , NULL, 0,
UPDATE_REFS_MSG_ON_ERR);
+}
+
static const char
Re: Adding nested repository with slash adds files instead of gitlink
On Mon, Jun 18, 2018 at 01:19:19PM +0200, Heiko Voigt wrote: Now with cc to the mailing list. > Hi, > > I just discovered that when you have a slash at the end of a nested > repository, the files contained in the repository get added instead of > the gitlink. > > I found this when I was adding a submodule and wanted to commit a small > change before that. You get the slash by using tab autocompletion. > > Here is a recipe to reproduce: > > mkdir test > cd test; git init > touch a; git add a; git commit -m a > mkdir ../test.git; (cd ../test.git; git init --bare) > git remote add origin ../test.git > git push origin master > git submodule add ../test.git submodule > git reset > git add submodule/ > > Now instead of just submodule gitlink there is an entry for submodule/a > in the index. > > I just thought I put this out there. Will have a look if I find the time > to cook up a proper testcase and investigate. > > Cheers Heiko This has been the case as far as I can remember, and is basically lore in the #git irc channel). This can also be reproduced by just cloning a repo inside another repo and running `git add path/`.
Re: OAuth2 support in git?
Jeff King writes: > On Sun, Jun 17, 2018 at 01:37:24PM +0200, Johannes Schindelin wrote: > >> > If it's just a custom Authorization header, we should be able to support >> > it with existing curl versions without _too_ much effort. >> >> Indeed. Because it is already implemented: >> >> git -c http.extraheader="Authorization: Bearer ..." ... >> >> To make this a *little* safer, you can use http..extraheader. > > Yeah, that will work for some cases. A few places it might not: > > - some people may want to provide this only in response to a 401 > > - some tokens may need to be refreshed, which would require interacting >with a credential helper to do the rest of the oauth conversation > > - there's no good way to hide your token in secure storage (versus >sticking it on the command-line or in a config file). And all of these three are what you get for free by building on the credential helper framework, after extending it a bit so that the filled credential structure can tell the http code to show it to the other side as a bearer token, not a password or password hash. The helper is asked to supply the auth material only after 401, which covers both the first and the second points, and then keeping the auth material in-core (e.g. cache--daemon) would be more secure which covers the third point. Am I following you correctly? Thanks.
Re: [PATCH v2] sequencer: do not squash 'reword' commits when we hit conflicts
Phillip Wood writes: >> Oh, you're right, the fast-forwarding path would pose a problem. I think >> there is an easy way to resolve this, though: in the case that we do want >> to amend the to-be-reworded commit, we simply have to see whether HEAD >> points to the very same commit mentioned in the `reword` command: > > That's clever, I think to get it to work for rewording the root commit, > it will need to do something like comparing HEAD to squash-onto as well. So, for the second attempt by Elijah, Dscho said it is good while you noticed a problem with ff, to which Dscho agreed is a better approach, and you extended it which brings us here. I'll leave the thread hanging here while I pick other leftover bits for 2.18 final and then will come back. Hopefully by that time I'll find the final version of the patch to conclude this topic that I can apply on top of 2.18.0 ;-) Thanks.
Re: [GSoC][PATCH 2/3] rebase -i: rewrite setup_reflog_action() in C
On 18/06/18 14:18, Alban Gruin wrote:
This rewrites setup_reflog_action() from shell to C.
A new command is added to rebase--helper.c, “setup-reflog”, as such as a
new flag, “verbose”, to silence the output of the checkout operation
called by setup_reflog_action().
I'm having difficulty understanding what that means, surely the verbose
flag is to stop the output from being silenced.
I'm not that keen on the naming in this patch, if it's only a staging
post and will be removed it probably doesn't matter too much. I can see
why you've based the function and flag names on the shell version, but
the C version is not setting up the reflog it is checking out the branch
we're rebasing. --checkout-base or something like that would be clearer.
Also the name of the function that does the checkout is tied to checking
out the base revision, but then reused in the next patch to checkout the
'onto' commit. As such I think it would be clearer if it was called
run_git_checkout() or something like that.
One further thought - how easy would it be to refactor the code in
do_reset() to handle the checkouts in this patch series, rather than
having to fork 'git checkout'
Best wishes
Phillip
The shell version is then stripped in favour of a call to the helper. As
$GIT_REFLOG_ACTION is not longer set at the first call of
checkout_onto(), a call to comment_for_reflog() is added at the
beginning of this function.
Signed-off-by: Alban Gruin
---
builtin/rebase--helper.c | 9 +++--
git-rebase--interactive.sh | 16 ++--
sequencer.c| 31 +++
sequencer.h| 3 +++
4 files changed, 43 insertions(+), 16 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index d2990b210..d677fb663 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] = {
int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
{
struct replay_opts opts = REPLAY_OPTS_INIT;
- unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
+ unsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0;
int abbreviate_commands = 0, rebase_cousins = -1;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
- ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO
+ ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, SETUP_REFLOG
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -27,6 +27,7 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
OPT_BOOL(0, "rebase-merges", _merges, N_("rebase merge
commits")),
OPT_BOOL(0, "rebase-cousins", _cousins,
N_("keep original branch points of cousins")),
+ OPT_BOOL(0, "verbose", , N_("verbose")),
OPT_CMDMODE(0, "continue", , N_("continue rebase"),
CONTINUE),
OPT_CMDMODE(0, "abort", , N_("abort rebase"),
@@ -50,6 +51,8 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
OPT_CMDMODE(0, "edit-todo", ,
N_("edit the todo list during an interactive
rebase"),
EDIT_TODO),
+ OPT_CMDMODE(0, "setup-reflog", ,
+ N_("setup the reflog action"), SETUP_REFLOG),
OPT_END()
};
@@ -93,5 +96,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
return !!append_todo_help(0, keep_empty);
if (command == EDIT_TODO && argc == 1)
return !!edit_todo_list(flags);
+ if (command == SETUP_REFLOG && argc == 2)
+ return !!setup_reflog_action(, argv[1], verbose);
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2defe607f..048bbf041 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -72,6 +72,7 @@ collapse_todo_ids() {
# Switch to the branch in $into and notify it in the reflog
checkout_onto () {
+ comment_for_reflog start
GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
output git checkout $onto || die_abort "$(gettext "could not detach
HEAD")"
git update-ref ORIG_HEAD $orig_head
@@ -119,19 +120,6 @@ initiate_action () {
esac
}
-setup_reflog_action () {
- comment_for_reflog start
-
- if test ! -z "$switch_to"
- then
- GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
- output git checkout "$switch_to" -- ||
- die "$(eval_gettext "Could not checkout \$switch_to")"
Re: [GSoC][PATCH 1/3] sequencer: add a new function to silence a command, except if it fails.
Hi Alban
On 18/06/18 14:18, Alban Gruin wrote:
This adds a new function, run_command_silent_if_successful(), to
redirect the stdout and stderr of a command to a strbuf, and then to run
that command. This strbuf is printed only if the command fails. It is
functionnaly similar to output() from git-rebase.sh.
s/functionnaly/functionally/
It's not quite the same though because the shell versions handles
--verbose where as here the caller has to put that check in every call
site. I wonder if it would simplify the callers if the C version did the
--verbose handling it's self.
Best Wishes
Phillip
run_git_commit() is then refactored to use of
run_command_silent_if_successful().
Signed-off-by: Alban Gruin
---
sequencer.c | 53 +++--
1 file changed, 27 insertions(+), 26 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 7cc76332e..3437673d2 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -766,6 +766,25 @@ N_("you have staged changes in your working tree\n"
#define VERIFY_MSG (1<<4)
#define CREATE_ROOT_COMMIT (1<<5)
+static int run_command_silent_on_success(struct child_process *cmd)
+{
+ struct strbuf buf = STRBUF_INIT;
+ int rc;
+
+ /* hide stderr on success */
+ cmd->stdout_to_stderr = 1;
+ rc = pipe_command(cmd,
+ NULL, 0,
+ /* stdout is already redirected */
+ NULL, 0,
+ , 0);
+
+ if (rc)
+ fputs(buf.buf, stderr);
+ strbuf_release();
+ return rc;
+}
+
/*
* If we are cherry-pick, and if the merge did not result in
* hand-editing, we will hit this commit and inherit the original
@@ -820,18 +839,11 @@ static int run_git_commit(const char *defmsg, struct
replay_opts *opts,
cmd.git_cmd = 1;
- if (is_rebase_i(opts)) {
- if (!(flags & EDIT_MSG)) {
- cmd.stdout_to_stderr = 1;
- cmd.err = -1;
- }
+ if (is_rebase_i(opts) && read_env_script(_array)) {
+ const char *gpg_opt = gpg_sign_opt_quoted(opts);
- if (read_env_script(_array)) {
- const char *gpg_opt = gpg_sign_opt_quoted(opts);
-
- return error(_(staged_changes_advice),
-gpg_opt, gpg_opt);
- }
+ return error(_(staged_changes_advice),
+gpg_opt, gpg_opt);
}
argv_array_push(, "commit");
@@ -861,21 +873,10 @@ static int run_git_commit(const char *defmsg, struct
replay_opts *opts,
if (opts->allow_empty_message)
argv_array_push(, "--allow-empty-message");
- if (cmd.err == -1) {
- /* hide stderr on success */
- struct strbuf buf = STRBUF_INIT;
- int rc = pipe_command(,
- NULL, 0,
- /* stdout is already redirected */
- NULL, 0,
- , 0);
- if (rc)
- fputs(buf.buf, stderr);
- strbuf_release();
- return rc;
- }
-
- return run_command();
+ if (is_rebase_i(opts) && !(flags & EDIT_MSG))
+ return run_command_silent_on_success();
+ else
+ return run_command();
}
static int rest_is_empty(const struct strbuf *sb, int start)
Re: Adding nested repository with slash adds files instead of gitlink
On Mon, Jun 18, 2018 at 1:23 PM Heiko Voigt wrote: > > Hi, > > I just discovered that when you have a slash at the end of a nested > repository, the files contained in the repository get added instead of > the gitlink. > > I found this when I was adding a submodule and wanted to commit a small > change before that. You get the slash by using tab autocompletion. > > Here is a recipe to reproduce: > > mkdir test > cd test; git init > touch a; git add a; git commit -m a > mkdir ../test.git; (cd ../test.git; git init --bare) > git remote add origin ../test.git > git push origin master > git submodule add ../test.git submodule > git reset > git add submodule/ > > Now instead of just submodule gitlink there is an entry for submodule/a > in the index. > > I just thought I put this out there. Will have a look if I find the time > to cook up a proper testcase and investigate. This sounds like the submodule specific code in pathspec.c, which has been replaced with something else in bw/pathspec-sans-the-index. If you have time, try a version without those changes (e.g. v2.13 or before) to see if it's a possible culprit. > Cheers Heiko -- Duy
Re: Git diff --no-index --no-prefix output loses leading slash in paths
This is a feature request; sorry for the confusion. My guess is that it's a corner case that was not considered due to the default prefixing. > On Jun 18, 2018, at 10:59 AM, Duy Nguyen wrote: > > On Mon, Jun 18, 2018 at 4:36 PM George King wrote: >> >> As of 2.17.1, `git diff --no-index --no-prefix relative/path /absolute/path` >> produces the following: > > I checked as far back as v1.4.0 and git behaved the same way too. What > version did it work for you? Or is this not a regression, rather a > feature request? > >> diff --git relative/path absolute/path >> index XXX..YYY ZZ >> --- relative/path >> +++ absolute/path >> >> The leading slash on `absolute/path` is lost. This is unfortunate; my use >> case is a diff highlighter that parses and reformats paths so that code >> editors can autodetect them and link to the files. >> >> Would the maintainers please consider fixing the output to preserve absolute >> paths? >> >> Thank you, >> George King >> > -- > Duy
Re: Git diff --no-index --no-prefix output loses leading slash in paths
On Mon, Jun 18, 2018 at 4:36 PM George King wrote: > > As of 2.17.1, `git diff --no-index --no-prefix relative/path /absolute/path` > produces the following: I checked as far back as v1.4.0 and git behaved the same way too. What version did it work for you? Or is this not a regression, rather a feature request? > diff --git relative/path absolute/path > index XXX..YYY ZZ > --- relative/path > +++ absolute/path > > The leading slash on `absolute/path` is lost. This is unfortunate; my use > case is a diff highlighter that parses and reformats paths so that code > editors can autodetect them and link to the files. > > Would the maintainers please consider fixing the output to preserve absolute > paths? > > Thank you, > George King > -- Duy
Git diff --no-index --no-prefix output loses leading slash in paths
As of 2.17.1, `git diff --no-index --no-prefix relative/path /absolute/path` produces the following: diff --git relative/path absolute/path index XXX..YYY ZZ --- relative/path +++ absolute/path The leading slash on `absolute/path` is lost. This is unfortunate; my use case is a diff highlighter that parses and reformats paths so that code editors can autodetect them and link to the files. Would the maintainers please consider fixing the output to preserve absolute paths? Thank you, George King
RE: fatal: could not reset submodule index
> That is true; submodule.recurse is not affecting git clone. > This was a design decision once it was introduced, as the git clone might be > too large. Maybe we need to revisit that decision and just clone the > submodules if submodule.recurse is set. > > The problem that this bug report highlights, is the following: > Each branch has its own .gitmodules file and they can be all different, > however at clone time we only clone submodules from the currently active > branch. > > So you could get your test case to pass with > git clone --branch with-submodule main clone5 > Thank you for your input Stefan. But here is our situation : 7 different projects in separate repositories Big parts of the code is similar between the projects We want to extract these similar portions in separate repositories and add them as submodules (maybe about 10 of them). We would do this progressively, adding one submodule at a time. We need to support older versions so we have to keep branches that will not have submodules. Since we need to checkout these branches regularly, we would want to have submodule.recurse enabled. With this setup, every time a submodule is added in one of the projects, each developer would get the "fatal: could not reset submodule index" error and would be unable to checkout master branch.
[GSoC][PATCH 3/3] rebase -i: rewrite checkout_onto() in C
This rewrites checkout_onto() from shell to C.
A new command (“checkout-onto”) is added to rebase--helper.c. The shell
version is then stripped.
Signed-off-by: Alban Gruin
---
builtin/rebase--helper.c | 7 ++-
git-rebase--interactive.sh | 25 -
sequencer.c| 19 +++
sequencer.h| 3 +++
4 files changed, 32 insertions(+), 22 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index d677fb663..f9fffba96 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -17,7 +17,8 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
- ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, SETUP_REFLOG
+ ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, SETUP_REFLOG,
+ CHECKOUT_ONTO
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -53,6 +54,8 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
EDIT_TODO),
OPT_CMDMODE(0, "setup-reflog", ,
N_("setup the reflog action"), SETUP_REFLOG),
+ OPT_CMDMODE(0, "checkout-onto", ,
+ N_("checkout a commit"), CHECKOUT_ONTO),
OPT_END()
};
@@ -98,5 +101,7 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
return !!edit_todo_list(flags);
if (command == SETUP_REFLOG && argc == 2)
return !!setup_reflog_action(, argv[1], verbose);
+ if (command == CHECKOUT_ONTO && argc == 4)
+ return !!checkout_onto(, argv[1], argv[2], argv[3],
verbose);
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 048bbf041..0ae053291 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -28,17 +28,6 @@ case "$comment_char" in
;;
esac
-orig_reflog_action="$GIT_REFLOG_ACTION"
-
-comment_for_reflog () {
- case "$orig_reflog_action" in
- ''|rebase*)
- GIT_REFLOG_ACTION="rebase -i ($1)"
- export GIT_REFLOG_ACTION
- ;;
- esac
-}
-
die_abort () {
apply_autostash
rm -rf "$state_dir"
@@ -70,14 +59,6 @@ collapse_todo_ids() {
git rebase--helper --shorten-ids
}
-# Switch to the branch in $into and notify it in the reflog
-checkout_onto () {
- comment_for_reflog start
- GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
- output git checkout $onto || die_abort "$(gettext "could not detach
HEAD")"
- git update-ref ORIG_HEAD $orig_head
-}
-
get_missing_commit_check_level () {
check_level=$(git config --get rebase.missingCommitsCheck)
check_level=${check_level:-ignore}
@@ -176,7 +157,8 @@ EOF
git rebase--helper --check-todo-list || {
ret=$?
- checkout_onto
+ git rebase--helper --checkout-onto "$onto_name" "$onto" \
+ "$orig_head" ${verbose:+--verbose}
exit $ret
}
@@ -186,7 +168,8 @@ EOF
onto="$(git rebase--helper --skip-unnecessary-picks)" ||
die "Could not skip unnecessary pick commands"
- checkout_onto
+ git rebase--helper --checkout-onto "$onto_name" "$onto" "$orig_head" \
+ ${verbose:+--verbose}
require_clean_work_tree "rebase"
exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
--continue
diff --git a/sequencer.c b/sequencer.c
index 4bfe29c7b..d149cbf92 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3162,6 +3162,25 @@ int setup_reflog_action(struct replay_opts *opts, const
char *commit,
return 0;
}
+int checkout_onto(struct replay_opts *opts,
+ const char *onto_name, const char *onto,
+ const char *orig_head, unsigned verbose)
+{
+ struct object_id oid;
+ const char *action = reflog_message(opts, "start", "checkout %s",
onto_name);
+
+ if (get_oid(orig_head, ))
+ die(_("%s: not a valid OID"), orig_head);
+
+ if (checkout_base_commit(opts, onto, verbose, action)) {
+ apply_autostash(opts);
+ sequencer_remove_state(opts);
+ die(_("could not detach HEAD"));
+ }
+
+ return update_ref(NULL, "ORIG_HEAD", , NULL, 0,
UPDATE_REFS_MSG_ON_ERR);
+}
+
static const char rescheduled_advice[] =
N_("Could not execute the todo command\n"
"\n"
diff --git a/sequencer.h b/sequencer.h
index 55e4057d8..9899d90fc 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -102,6 +102,9 @@ void commit_post_rewrite(const struct commit
[GSoC][PATCH 2/3] rebase -i: rewrite setup_reflog_action() in C
This rewrites setup_reflog_action() from shell to C.
A new command is added to rebase--helper.c, “setup-reflog”, as such as a
new flag, “verbose”, to silence the output of the checkout operation
called by setup_reflog_action().
The shell version is then stripped in favour of a call to the helper. As
$GIT_REFLOG_ACTION is not longer set at the first call of
checkout_onto(), a call to comment_for_reflog() is added at the
beginning of this function.
Signed-off-by: Alban Gruin
---
builtin/rebase--helper.c | 9 +++--
git-rebase--interactive.sh | 16 ++--
sequencer.c| 31 +++
sequencer.h| 3 +++
4 files changed, 43 insertions(+), 16 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index d2990b210..d677fb663 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] = {
int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
{
struct replay_opts opts = REPLAY_OPTS_INIT;
- unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
+ unsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0;
int abbreviate_commands = 0, rebase_cousins = -1;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
- ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO
+ ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, SETUP_REFLOG
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -27,6 +27,7 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
OPT_BOOL(0, "rebase-merges", _merges, N_("rebase merge
commits")),
OPT_BOOL(0, "rebase-cousins", _cousins,
N_("keep original branch points of cousins")),
+ OPT_BOOL(0, "verbose", , N_("verbose")),
OPT_CMDMODE(0, "continue", , N_("continue rebase"),
CONTINUE),
OPT_CMDMODE(0, "abort", , N_("abort rebase"),
@@ -50,6 +51,8 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
OPT_CMDMODE(0, "edit-todo", ,
N_("edit the todo list during an interactive
rebase"),
EDIT_TODO),
+ OPT_CMDMODE(0, "setup-reflog", ,
+ N_("setup the reflog action"), SETUP_REFLOG),
OPT_END()
};
@@ -93,5 +96,7 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
return !!append_todo_help(0, keep_empty);
if (command == EDIT_TODO && argc == 1)
return !!edit_todo_list(flags);
+ if (command == SETUP_REFLOG && argc == 2)
+ return !!setup_reflog_action(, argv[1], verbose);
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2defe607f..048bbf041 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -72,6 +72,7 @@ collapse_todo_ids() {
# Switch to the branch in $into and notify it in the reflog
checkout_onto () {
+ comment_for_reflog start
GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
output git checkout $onto || die_abort "$(gettext "could not detach
HEAD")"
git update-ref ORIG_HEAD $orig_head
@@ -119,19 +120,6 @@ initiate_action () {
esac
}
-setup_reflog_action () {
- comment_for_reflog start
-
- if test ! -z "$switch_to"
- then
- GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
- output git checkout "$switch_to" -- ||
- die "$(eval_gettext "Could not checkout \$switch_to")"
-
- comment_for_reflog start
- fi
-}
-
init_basic_state () {
orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")"
mkdir -p "$state_dir" || die "$(eval_gettext "Could not create
temporary \$state_dir")"
@@ -211,7 +199,7 @@ git_rebase__interactive () {
return 0
fi
- setup_reflog_action
+ git rebase--helper --setup-reflog "$switch_to" ${verbose:+--verbose}
init_basic_state
init_revisions_and_shortrevisions
diff --git a/sequencer.c b/sequencer.c
index 3437673d2..4bfe29c7b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3131,6 +3131,37 @@ static const char *reflog_message(struct replay_opts
*opts,
return buf.buf;
}
+static int checkout_base_commit(struct replay_opts *opts, const char *commit,
+ int verbose, const char *action)
+{
+ struct child_process cmd = CHILD_PROCESS_INIT;
+
+ cmd.git_cmd = 1;
+
+ argv_array_push(,
[GSoC][PATCH 0/3] rebase -i: rewrite reflog operations in C
This patch series rewrites the reflog operations from shell to C. This is part of the effort to rewrite interactive rebase in C. The first commit is dedicated to creating a function to silence a command, as the sequencer will do in several places with these patches. This branch is based on ag/rebase-i-rewrite-todo, and does not conflict with pu (as of 2018-06-18). Alban Gruin (3): sequencer: add a new function to silence a command, except if it fails. rebase -i: rewrite setup_reflog_action() in C rebase -i: rewrite checkout_onto() in C builtin/rebase--helper.c | 14 +- git-rebase--interactive.sh | 39 +++-- sequencer.c| 103 + sequencer.h| 6 +++ 4 files changed, 100 insertions(+), 62 deletions(-) -- 2.16.4
[GSoC][PATCH 1/3] sequencer: add a new function to silence a command, except if it fails.
This adds a new function, run_command_silent_if_successful(), to
redirect the stdout and stderr of a command to a strbuf, and then to run
that command. This strbuf is printed only if the command fails. It is
functionnaly similar to output() from git-rebase.sh.
run_git_commit() is then refactored to use of
run_command_silent_if_successful().
Signed-off-by: Alban Gruin
---
sequencer.c | 53 +++--
1 file changed, 27 insertions(+), 26 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 7cc76332e..3437673d2 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -766,6 +766,25 @@ N_("you have staged changes in your working tree\n"
#define VERIFY_MSG (1<<4)
#define CREATE_ROOT_COMMIT (1<<5)
+static int run_command_silent_on_success(struct child_process *cmd)
+{
+ struct strbuf buf = STRBUF_INIT;
+ int rc;
+
+ /* hide stderr on success */
+ cmd->stdout_to_stderr = 1;
+ rc = pipe_command(cmd,
+ NULL, 0,
+ /* stdout is already redirected */
+ NULL, 0,
+ , 0);
+
+ if (rc)
+ fputs(buf.buf, stderr);
+ strbuf_release();
+ return rc;
+}
+
/*
* If we are cherry-pick, and if the merge did not result in
* hand-editing, we will hit this commit and inherit the original
@@ -820,18 +839,11 @@ static int run_git_commit(const char *defmsg, struct
replay_opts *opts,
cmd.git_cmd = 1;
- if (is_rebase_i(opts)) {
- if (!(flags & EDIT_MSG)) {
- cmd.stdout_to_stderr = 1;
- cmd.err = -1;
- }
+ if (is_rebase_i(opts) && read_env_script(_array)) {
+ const char *gpg_opt = gpg_sign_opt_quoted(opts);
- if (read_env_script(_array)) {
- const char *gpg_opt = gpg_sign_opt_quoted(opts);
-
- return error(_(staged_changes_advice),
-gpg_opt, gpg_opt);
- }
+ return error(_(staged_changes_advice),
+gpg_opt, gpg_opt);
}
argv_array_push(, "commit");
@@ -861,21 +873,10 @@ static int run_git_commit(const char *defmsg, struct
replay_opts *opts,
if (opts->allow_empty_message)
argv_array_push(, "--allow-empty-message");
- if (cmd.err == -1) {
- /* hide stderr on success */
- struct strbuf buf = STRBUF_INIT;
- int rc = pipe_command(,
- NULL, 0,
- /* stdout is already redirected */
- NULL, 0,
- , 0);
- if (rc)
- fputs(buf.buf, stderr);
- strbuf_release();
- return rc;
- }
-
- return run_command();
+ if (is_rebase_i(opts) && !(flags & EDIT_MSG))
+ return run_command_silent_on_success();
+ else
+ return run_command();
}
static int rest_is_empty(const struct strbuf *sb, int start)
--
2.16.4
Re: Re :Re: [PATCHv3 0/1] git-p4 unshelve
On 16 June 2018 at 10:58, merlo...@yahoo.fr wrote: > Yes Luke, my colleagues and I, we care ! Our repository is p4 (choice of the > high management, sigh...). Some of us are working natively on p4, but others > like me are working on git through git-p4. We often want to share pieces of > codes to check compilation on misc platforms/configs, and shelve/unshelve > mechanism is commonly used between nativ p4 users. > For git-p4 users we have a temporary hack to unshelve, but it often fails to > apply the whole p4 diff, and we end up finishing stuff by hand, checking > line by line, sigh... Without speaking about diffs that are accross several > local workspaces. > Hopefully it is on small shelved p4 change lists for the moment, but we > cannot deploy on a larger scale. > Please continue your hard work on unshelve stuff. > > For your last remark, the members of our team often need to work on non > synchro revisions, but still need to share via shelve/unshelve, and I am > sure we will have conflits as you describe, leading unshelve to fail. > Automatic fast import would save us the need to stop our current work, sync > with p4 and launch a 1h compilation, before we could unshelve... So yes we > need it ! OK, I'll give it a go, in my copious free time :-) Luke
Adding nested repository with slash adds files instead of gitlink
Hi, I just discovered that when you have a slash at the end of a nested repository, the files contained in the repository get added instead of the gitlink. I found this when I was adding a submodule and wanted to commit a small change before that. You get the slash by using tab autocompletion. Here is a recipe to reproduce: mkdir test cd test; git init touch a; git add a; git commit -m a mkdir ../test.git; (cd ../test.git; git init --bare) git remote add origin ../test.git git push origin master git submodule add ../test.git submodule git reset git add submodule/ Now instead of just submodule gitlink there is an entry for submodule/a in the index. I just thought I put this out there. Will have a look if I find the time to cook up a proper testcase and investigate. Cheers Heiko
[PATCH] RelNotes 2.18: minor fix to entry about dynamically loading completions
It was not "newer versions of bash" but newer versions of bash-completion that made commit 085e2ee0e6 (completion: load completion file for external subcommand, 2018-04-29) both necessary and possible. Update the corresponding RelNotes entry accordingly. Signed-off-by: SZEDER Gábor --- Documentation/RelNotes/2.18.0.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/RelNotes/2.18.0.txt b/Documentation/RelNotes/2.18.0.txt index 7c59bd92fb..4d9c038b07 100644 --- a/Documentation/RelNotes/2.18.0.txt +++ b/Documentation/RelNotes/2.18.0.txt @@ -104,7 +104,7 @@ UI, Workflows & Features * The command line completion mechanism (in contrib/) learned to load custom completion file for "git $command" where $command is a custom "git-$command" that the end user has on the $PATH when using - newer version of bash. + newer version of bash-completion. * "git send-email" can sometimes offer confirmation dialog "Send this email?" with choices 'Yes', 'No', 'Quit', and 'All'. A new action -- 2.18.0.rc0.207.ga6211da864
Re: [PATCH 1/2] rebase --root: demonstrate a bug while amending rootcommit messages
Hi Todd/Johannes On 15/06/18 05:31, Johannes Schindelin via GitGitGadget wrote: > > From: Todd Zullinger > > When splitting a repository, running `git rebase -i --root` to reword > the initial commit, Git dies with > > BUG: sequencer.c:795: root commit without message. > > Signed-off-by: Todd Zullinger > Signed-off-by: Johannes Schindelin > --- > t/t3404-rebase-interactive.sh | 9 + > 1 file changed, 9 insertions(+) > > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index c65826dda..ca94c688d 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -971,6 +971,15 @@ test_expect_success 'rebase -i --root fixup root commit' > ' > test 0 = $(git cat-file commit HEAD | grep -c ^parent\ ) > ' > > +test_expect_failure 'rebase -i --root reword root commit' ' > + test_when_finished "test_might_fail git rebase --abort" && > + git checkout -b reword-root-branch master && > + set_fake_editor && > + FAKE_LINES="reword 1 2" FAKE_COMMIT_MESSAGE="A changed" \ > + git rebase -i --root && > + git show HEAD^ | grep "A changed" I wonder if it should also check that HEAD^ is the root commit, to make sure that the squash-onto commit that's created and then amended has been squashed onto. Best Wishes Phillip > +' > + > test_expect_success C_LOCALE_OUTPUT 'rebase --edit-todo does not work on > non-interactive rebase' ' > git reset --hard && > git checkout conflict-branch && >
Re: [PATCH v2] sequencer: do not squash 'reword' commits when we hit conflicts
Hi Johannes
On 17/06/18 20:28, Johannes Schindelin wrote:
> Hi Phillip,
>
> On Sun, 17 Jun 2018, Phillip Wood wrote:
>
>> On 17/06/18 06:37, Elijah Newren wrote:
>>> Ever since commit 18633e1a22 ("rebase -i: use the rebase--helper builtin",
>>> 2017-02-09), when a commit marked as 'reword' in an interactive rebase
>>> has conflicts and fails to apply, when the rebase is resumed that commit
>>> will be squashed into its parent with its commit message taken.
>>>
>>> The issue can be understood better by looking at commit 56dc3ab04b
>>> ("sequencer (rebase -i): implement the 'edit' command", 2017-01-02), which
>>> introduced error_with_patch() for the edit command. For the edit command,
>>> it needs to stop the rebase whether or not the patch applies cleanly. If
>>> the patch does apply cleanly, then when it resumes it knows it needs to
>>> amend all changes into the previous commit. If it does not apply cleanly,
>>> then the changes should not be amended. Thus, it passes !res (success of
>>> applying the 'edit' commit) to error_with_patch() for the to_amend flag.
>>>
>>> The problematic line of code actually came from commit 04efc8b57c
>>> ("sequencer (rebase -i): implement the 'reword' command", 2017-01-02).
>>> Note that to get to this point in the code:
>>>* !!res (i.e. patch application failed)
>>>* item->command < TODO_SQUASH
>>>* item->command != TODO_EDIT
>>>* !is_fixup(item->command) [i.e. not squash or fixup]
>>> So that means this can only be a failed patch application that is either a
>>> pick, revert, or reword. For any of those cases we want a new commit, so
>>> we should not set the to_amend flag.
>>
>> Unfortunately I'm not sure it's that simple. Looking and do_pick() sometimes
>> reword amends HEAD and sometimes it does not. In the "normal" case then the
>> commit is picked and committed with '--edit'. However when fast-forwarding
>> the
>> code fast forwards to the commit to be reworded and then amends it. If the
>> root commit is being reworded it takes the same code path. While these cases
>> cannot fail with conflicts, it is possible for the user to cancel the commit
>> or for them to fail due to collisions with untracked files.
>>
>> If I remember correctly the shell version always picks the commit and then
>> calls 'git commit --amend' afterwards which is less efficient but consistent.
>>
>> I'm afraid I don't have a simple solution for fixing this, as currently
>> pick_commits() does not know if the commit was called with AMEND_MSG, I guess
>> that means adding some kind of flag for do_pick() to set.
>
> Oh, you're right, the fast-forwarding path would pose a problem. I think
> there is an easy way to resolve this, though: in the case that we do want
> to amend the to-be-reworded commit, we simply have to see whether HEAD
> points to the very same commit mentioned in the `reword` command:
That's clever, I think to get it to work for rewording the root commit,
it will need to do something like comparing HEAD to squash-onto as well.
> -- snip --
> diff --git a/sequencer.c b/sequencer.c
> index 2dad7041960..99d33d4e063 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3691,10 +3691,22 @@ static int pick_commits(struct todo_list
> *todo_list, struct replay_opts *opts)
> intend_to_amend();
> return error_failed_squash(item->commit, opts,
> item->arg_len, item->arg);
> - } else if (res && is_rebase_i(opts) && item->commit)
> + } else if (res && is_rebase_i(opts) && item->commit) {
> + int to_amend = 0;
> +
> + if (item->command == TODO_REWORD) {
> + struct object_id head;
> +
> + if (!get_oid("HEAD", ) &&
> + !oidcmp(>commit->object.oid,
> + ))
> + to_amend = 1;
> + }
> +
> return res | error_with_patch(item->commit,
> item->arg, item->arg_len, opts, res,
> - item->command == TODO_REWORD);
> + to_amend);
> + }
> } else if (item->command == TODO_EXEC) {
> char *end_of_arg = (char *)(item->arg +
> item->arg_len);
> int saved = *end_of_arg;
> -- snap --
>
> Note that
>
> - this patch is only compile-tested, and
>
> - it is on top of my sequencer-shears branch thicket, so it might not
> apply cleanly to master, and
>
> - it could probably use a comment what we are doing here (see whether we
> wanted to amend a fast-forwarded commit).
Yes that would be helpful for future
Draft of Git Rev News edition 40
Hi, A draft of a new Git Rev News edition is available here: https://github.com/git/git.github.io/blob/master/rev_news/drafts/edition-40.md Everyone is welcome to contribute in any section either by editing the above page on GitHub and sending a pull request, or by commenting on this GitHub issue: https://github.com/git/git.github.io/issues/295 You can also reply to this email. In general all kinds of contribution, for example proofreading, suggestions for articles or links, help on the issues in GitHub, and so on, are very much appreciated. This month it would be nice to have a small summary, or maybe just a number of links, related to the security release on May 29th, and the same related to the GitHub acquisition by Microsoft. Thanks in advance! I tried to cc everyone who appears in this edition, but maybe I missed some people, sorry about that. Jakub, Markus, Gabriel and me plan to publish this edition on Wednesday June 20th. Thanks, Christian.
