Re: [RFC/PATCH] merge: Add '--continue' option as a synonym for 'git commit'
On Mon, Dec 12, 2016 at 10:02 PM, Markus Hitter wrote:
> Am 12.12.2016 um 09:34 schrieb Chris Packham:
>> Teach 'git merge' the --continue option which allows 'continuing' a
>> merge by completing it. The traditional way of completing a merge after
>> resolving conflicts is to use 'git commit'. Now with commands like 'git
>> rebase' and 'git cherry-pick' having a '--continue' option adding such
>> an option to 'git merge' presents a consistent UI.
>
> Like.
>
> While Junio is entirely right that this is redundant, the inner workings of
> Git are just voodoo for a (guessed) 95% of users out there, so a consistent
> UI is important.
>
>> DESCRIPTION
>> ---
>> @@ -61,6 +62,9 @@ reconstruct the original (pre-merge) changes. Therefore:
>> discouraged: while possible, it may leave you in a state that is hard to
>> back out of in the case of a conflict.
>>
>> +The fourth syntax ("`git merge --continue`") can only be run after the
>> +merge has resulted in conflicts. 'git merge --continue' will take the
>> +currently staged changes and complete the merge.
>
> I think this should mention the equivalence to 'git commit'.
>
It is mentioned in the OPTIONS section where the --continue option is
documented. I could move it here but the OPTIONS section is where the
--abort synonym also has a reference to git reset --merge.
>
> Markus
>
> --
> - - - - - - - - - - - - - - - - - - -
> Dipl. Ing. (FH) Markus Hitter
> http://www.jump-ing.de/
log: range_set_append: Assertion failed with -L:funcname:
Hi all, I encountered the following assertion failure when running in a local clone of the https://github.com/nodejs/node.git repository: $ git log -L:writeFileSync:lib/fs.js -L:appendFileSync:lib/fs.js aa67b1^..aa67b1 git: line-log.c:71: range_set_append: Assertion `rs->nr == 0 || rs->ranges[rs->nr-1].end <= a' failed. Aborted I was able to provoke the same assertion failure in the git repository with the following: $ git log -L:print_one_push_status:transport.c -L:transport_summary_width:transport.c f9db0c^..f9db0c (FWIW, f9db0c is a merge commit while aa67b1 is not.) I was able to reproduce the error with git built from 13b8f6 (the commit which introduced git log -L:pattern:file) as well as current next (91ed5b). The backtrace from next is attached. Any assistance to avoid or fix the error would be greatly appreciated. Thanks, Kevin P.S. Please CC me in replies as I am not subscribed to this ML. -- Kevin Locke | ke...@kevinlocke.name| XMPP: ke...@kevinlocke.name | https://kevinlocke.name | IRC: kevinoid on freenode #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:58 #1 0x76fa040a in __GI_abort () at abort.c:89 #2 0x76f97e47 in __assert_fail_base (fmt=, assertion=assertion@entry=0x55754400 "rs->nr == 0 || rs->ranges[rs->nr-1].end <= a", file=file@entry=0x557543eb "line-log.c", line=line@entry=71, function=function@entry=0x55754730 <__PRETTY_FUNCTION__.29201> "range_set_append") at assert.c:92 #3 0x76f97ef2 in __GI___assert_fail ( assertion=0x55754400 "rs->nr == 0 || rs->ranges[rs->nr-1].end <= a", file=0x557543eb "line-log.c", line=71, function=0x55754730 <__PRETTY_FUNCTION__.29201> "range_set_append") at assert.c:101 #4 0x556677dd in range_set_append (rs=0x7fffd320, a=421, b=434) at line-log.c:71 #5 0x556686f8 in range_set_shift_diff (out=0x7fffd320, rs=0x7fffd330, diff=0x7fffd3a0) at line-log.c:442 #6 0x556687aa in range_set_map_across_diff (out=0x7fffd3c0, rs=0x55ad9698, diff=0x7fffd3a0, touched_out=0x7fffd410) at line-log.c:465 #7 0x5566a33e in process_diff_filepair (rev=0x7fffd5f0, pair=0x55a60830, range=0x55ad9680, diff_out=0x7fffd410) at line-log.c:1036 #8 0x5566a4ec in process_all_files (range_out=0x55a5ba10, rev=0x7fffd5f0, queue=0x55a078a0, range=0x55a347a0) at line-log.c:1076 #9 0x5566a852 in process_ranges_merge_commit (rev=0x7fffd5f0, commit=0x55a19f70, range=0x55a347a0) at line-log.c:1158 #10 0x5566aa37 in process_ranges_arbitrary_commit (rev=0x7fffd5f0, commit=0x55a19f70) at line-log.c:1202 #11 0x5566ab4b in line_log_filter (rev=0x7fffd5f0) at line-log.c:1236 #12 0x556c59f1 in prepare_revision_walk (revs=0x7fffd5f0) at revision.c:2773 #13 0x555ac8a9 in cmd_log_walk (rev=0x7fffd5f0) at builtin/log.c:347 #14 0x555ad9e1 in cmd_log (argc=4, argv=0x7fffe0d8, prefix=0x0) at builtin/log.c:682 #15 0x55566079 in run_builtin (p=0x559b5ba0 , argc=4, argv=0x7fffe0d8) at git.c:373 #16 0x555662fa in handle_builtin (argc=4, argv=0x7fffe0d8) at git.c:572 #17 0x55566534 in cmd_main (argc=4, argv=0x7fffe0d8) at git.c:677 #18 0x555fd6bf in main (argc=4, argv=0x7fffe0d8) at common-main.c:40
[PATCHv2 1/2] merge: Add '--continue' option as a synonym for 'git commit'
Teach 'git merge' the --continue option which allows 'continuing' a
merge by completing it. The traditional way of completing a merge after
resolving conflicts is to use 'git commit'. Now with commands like 'git
rebase' and 'git cherry-pick' having a '--continue' option adding such
an option to 'git merge' presents a consistent UI.
Signed-off-by: Chris Packham
---
Notes:
Changes in v2:
- add --continue to builtin_merge_usage
- verify that no other arguments are present when --continue is used.
- add basic test
Documentation/git-merge.txt | 13 -
builtin/merge.c | 22 +-
t/t7600-merge.sh| 8
3 files changed, 41 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index b758d5556..765b0f26e 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -15,6 +15,7 @@ SYNOPSIS
[--[no-]rerere-autoupdate] [-m ] [...]
'git merge' HEAD ...
'git merge' --abort
+'git merge' --continue
DESCRIPTION
---
@@ -61,6 +62,9 @@ reconstruct the original (pre-merge) changes. Therefore:
discouraged: while possible, it may leave you in a state that is hard to
back out of in the case of a conflict.
+The fourth syntax ("`git merge --continue`") can only be run after the
+merge has resulted in conflicts. 'git merge --continue' will take the
+currently staged changes and complete the merge.
OPTIONS
---
@@ -99,6 +103,12 @@ commit or stash your changes before running 'git merge'.
'git merge --abort' is equivalent to 'git reset --merge' when
`MERGE_HEAD` is present.
+--continue::
+ Take the currently staged changes and complete the merge.
++
+'git merge --continue' is equivalent to 'git commit' when
+`MERGE_HEAD` is present.
+
...::
Commits, usually other branch heads, to merge into our branch.
Specifying more than one commit will create a merge with
@@ -277,7 +287,8 @@ After seeing a conflict, you can do two things:
* Resolve the conflicts. Git will mark the conflicts in
the working tree. Edit the files into shape and
- 'git add' them to the index. Use 'git commit' to seal the deal.
+ 'git add' them to the index. Use 'git merge --continue' to seal the
+ deal.
You can work through the conflict with a number of tools:
diff --git a/builtin/merge.c b/builtin/merge.c
index b65eeaa87..379685223 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -46,6 +46,7 @@ static const char * const builtin_merge_usage[] = {
N_("git merge [] [...]"),
N_("git merge [] HEAD "),
N_("git merge --abort"),
+ N_("git merge --continue"),
NULL
};
@@ -65,6 +66,7 @@ static int option_renormalize;
static int verbosity;
static int allow_rerere_auto;
static int abort_current_merge;
+static int continue_current_merge;
static int allow_unrelated_histories;
static int show_progress = -1;
static int default_to_upstream = 1;
@@ -223,6 +225,8 @@ static struct option builtin_merge_options[] = {
OPT__VERBOSITY(&verbosity),
OPT_BOOL(0, "abort", &abort_current_merge,
N_("abort the current in-progress merge")),
+ OPT_BOOL(0, "continue", &continue_current_merge,
+ N_("continue the current in-progress merge")),
OPT_BOOL(0, "allow-unrelated-histories", &allow_unrelated_histories,
N_("allow merging unrelated histories")),
OPT_SET_INT(0, "progress", &show_progress, N_("force progress
reporting"), 1),
@@ -739,7 +743,7 @@ static void abort_commit(struct commit_list *remoteheads,
const char *err_msg)
if (err_msg)
error("%s", err_msg);
fprintf(stderr,
- _("Not committing merge; use 'git commit' to complete the
merge.\n"));
+ _("Not committing merge; use 'git merge --continue' to complete
the merge.\n"));
write_merge_state(remoteheads);
exit(1);
}
@@ -1166,6 +1170,22 @@ int cmd_merge(int argc, const char **argv, const char
*prefix)
goto done;
}
+ if (continue_current_merge) {
+ int nargc = 1;
+ const char *nargv[] = {"commit", NULL};
+
+ if (argc)
+ usage_msg_opt("--continue expects no arguments",
+ builtin_merge_usage, builtin_merge_options);
+
+ if (!file_exists(git_path_merge_head()))
+ die(_("There is no merge in progress (MERGE_HEAD
missing)."));
+
+ /* Invoke 'git commit' */
+ ret = cmd_commit(nargc, nargv, prefix);
+ goto done;
+ }
+
if (read_cache_unmerged())
die_resolve_conflict("merge");
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 85248a14b..44b34ef3a 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -154,6 +154,7 @@ test_expect_success 'test option parsing' '
test_must_fail git
[PATCHv2 2/2] completion: add --continue option for merge
Add 'git merge --continue' option when completing. Signed-off-by: Chris Packham --- Notes: Changes in v2: - new. contrib/completion/git-completion.bash | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 21016bf8d..1f97ffae1 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -1552,7 +1552,7 @@ _git_merge () case "$cur" in --*) __gitcomp "$__git_merge_options - --rerere-autoupdate --no-rerere-autoupdate --abort" + --rerere-autoupdate --no-rerere-autoupdate --abort --continue" return esac __gitcomp_nl "$(__git_refs)" -- 2.11.0.24.ge6920cf
[RFC/PATCH] Makefile: add cppcheck target
Add cppcheck target to Makefile. Cppcheck is a static
analysis tool for C/C++ code. Cppcheck primarily detects
the types of bugs that the compilers normally do not detect.
It is an useful target for doing QA analysis.
Based-on-patch-by: Elia Pinto
Signed-off-by: Chris Packham
---
I had been playing with cppcheck for some other projects and happened to
notice [1] in the archives. This is my attempt to resolve the feedback
that Junio made at the time.
In terms of errors that are actually reported there are only a few
$ make cppcheck
cppcheck --force --quiet --inline-suppr .
[compat/nedmalloc/malloc.c.h:4093]: (error) Possible null pointer dereference:
sp
[compat/nedmalloc/malloc.c.h:4106]: (error) Possible null pointer dereference:
sp
[compat/nedmalloc/nedmalloc.c:551]: (error) Expression
'*(&p.mycache)=TlsAlloc(),TLS_OUT_OF_INDEXES==*(&p.mycache)' depends on order
of evaluation of side effects
[compat/regex/regcomp.c:3086]: (error) Memory leak: sbcset
[compat/regex/regcomp.c:3634]: (error) Memory leak: sbcset
[compat/regex/regcomp.c:3086]: (error) Memory leak: mbcset
[compat/regex/regcomp.c:3634]: (error) Memory leak: mbcset
[compat/regex/regcomp.c:2802]: (error) Uninitialized variable: table_size
[compat/regex/regcomp.c:2805]: (error) Uninitialized variable: table_size
[compat/regex/regcomp.c:532]: (error) Memory leak: fastmap
[t/t4051/appended1.c:3]: (error) Invalid number of character '{' when these
macros are defined: ''.
[t/t4051/appended2.c:35]: (error) Invalid number of character '{' when these
macros are defined: ''.
The last 2 are just false positives from test data. I haven't looked
into any of the others.
I've also provisioned for enabling extra checks by passing CPPCHECK_ADD
in the make invocation.
$ make cppcheck CPPCHECK_ADD=--enable=all
... lots of output
[1] -
http://public-inbox.org/git/1390993371-2431-1-git-send-email-gitter.spi...@gmail.com/#t
Makefile | 4
1 file changed, 4 insertions(+)
diff --git a/Makefile b/Makefile
index f53fcc90d..8b5976d88 100644
--- a/Makefile
+++ b/Makefile
@@ -2635,3 +2635,7 @@ cover_db: coverage-report
cover_db_html: cover_db
cover -report html -outputdir cover_db_html cover_db
+.PHONY: cppcheck
+
+cppcheck:
+ cppcheck --force --quiet --inline-suppr $(CPPCHECK_ADD) .
--
2.11.0.24.ge6920cf
Re: [RFC/PATCH] Makefile: add cppcheck target
On Tue, Dec 13, 2016 at 10:22 PM, Chris Packham wrote:
> Add cppcheck target to Makefile. Cppcheck is a static
> analysis tool for C/C++ code. Cppcheck primarily detects
> the types of bugs that the compilers normally do not detect.
> It is an useful target for doing QA analysis.
>
> Based-on-patch-by: Elia Pinto
> Signed-off-by: Chris Packham
> ---
> I had been playing with cppcheck for some other projects and happened to
> notice [1] in the archives. This is my attempt to resolve the feedback
> that Junio made at the time.
>
> In terms of errors that are actually reported there are only a few
>
> $ make cppcheck
> cppcheck --force --quiet --inline-suppr .
> [compat/nedmalloc/malloc.c.h:4093]: (error) Possible null pointer
> dereference: sp
> [compat/nedmalloc/malloc.c.h:4106]: (error) Possible null pointer
> dereference: sp
> [compat/nedmalloc/nedmalloc.c:551]: (error) Expression
> '*(&p.mycache)=TlsAlloc(),TLS_OUT_OF_INDEXES==*(&p.mycache)' depends on order
> of evaluation of side effects
> [compat/regex/regcomp.c:3086]: (error) Memory leak: sbcset
> [compat/regex/regcomp.c:3634]: (error) Memory leak: sbcset
> [compat/regex/regcomp.c:3086]: (error) Memory leak: mbcset
> [compat/regex/regcomp.c:3634]: (error) Memory leak: mbcset
> [compat/regex/regcomp.c:2802]: (error) Uninitialized variable: table_size
> [compat/regex/regcomp.c:2805]: (error) Uninitialized variable: table_size
> [compat/regex/regcomp.c:532]: (error) Memory leak: fastmap
> [t/t4051/appended1.c:3]: (error) Invalid number of character '{' when these
> macros are defined: ''.
> [t/t4051/appended2.c:35]: (error) Invalid number of character '{' when these
> macros are defined: ''.
>
> The last 2 are just false positives from test data. I haven't looked
> into any of the others.
>
> I've also provisioned for enabling extra checks by passing CPPCHECK_ADD
> in the make invocation.
>
> $ make cppcheck CPPCHECK_ADD=--enable=all
> ... lots of output
>
> [1] -
> http://public-inbox.org/git/1390993371-2431-1-git-send-email-gitter.spi...@gmail.com/#t
>
> Makefile | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index f53fcc90d..8b5976d88 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2635,3 +2635,7 @@ cover_db: coverage-report
> cover_db_html: cover_db
> cover -report html -outputdir cover_db_html cover_db
>
> +.PHONY: cppcheck
> +
> +cppcheck:
> + cppcheck --force --quiet --inline-suppr $(CPPCHECK_ADD) .
If I'm permitted a little GNU make-ism the following might make
CPPCHECK_ADD a bit more usable
+ cppcheck --force --quiet --inline-suppr $(if
$(CPPCHECK_ADD),--enable=$(CPPCHECK_ADD)) .
Which would take us from
$ make cppcheck CPPCHECK_ADD=--enable=all
to
$ make cppcheck CPPCHECK_ADD=all
Re: [RFC/PATCH] Makefile: add cppcheck target
Am 13.12.2016 um 10:32 schrieb Chris Packham:
> On Tue, Dec 13, 2016 at 10:22 PM, Chris Packham
> wrote:
>> Add cppcheck target to Makefile. Cppcheck is a static
>> analysis tool for C/C++ code. Cppcheck primarily detects
>> the types of bugs that the compilers normally do not detect.
>> It is an useful target for doing QA analysis.
>>
>> Based-on-patch-by: Elia Pinto
>> Signed-off-by: Chris Packham
>> ---
>> I had been playing with cppcheck for some other projects and happened to
>> notice [1] in the archives. This is my attempt to resolve the feedback
>> that Junio made at the time.
>>
>> In terms of errors that are actually reported there are only a few
>>
>> $ make cppcheck
>> cppcheck --force --quiet --inline-suppr .
>> [compat/nedmalloc/malloc.c.h:4093]: (error) Possible null pointer
>> dereference: sp
>> [compat/nedmalloc/malloc.c.h:4106]: (error) Possible null pointer
>> dereference: sp
>> [compat/nedmalloc/nedmalloc.c:551]: (error) Expression
>> '*(&p.mycache)=TlsAlloc(),TLS_OUT_OF_INDEXES==*(&p.mycache)' depends on
>> order of evaluation of side effects
>> [compat/regex/regcomp.c:3086]: (error) Memory leak: sbcset
>> [compat/regex/regcomp.c:3634]: (error) Memory leak: sbcset
>> [compat/regex/regcomp.c:3086]: (error) Memory leak: mbcset
>> [compat/regex/regcomp.c:3634]: (error) Memory leak: mbcset
>> [compat/regex/regcomp.c:2802]: (error) Uninitialized variable: table_size
>> [compat/regex/regcomp.c:2805]: (error) Uninitialized variable: table_size
>> [compat/regex/regcomp.c:532]: (error) Memory leak: fastmap
>> [t/t4051/appended1.c:3]: (error) Invalid number of character '{' when these
>> macros are defined: ''.
>> [t/t4051/appended2.c:35]: (error) Invalid number of character '{' when these
>> macros are defined: ''.
>>
>> The last 2 are just false positives from test data. I haven't looked
>> into any of the others.
>>
>> I've also provisioned for enabling extra checks by passing CPPCHECK_ADD
>> in the make invocation.
>>
>> $ make cppcheck CPPCHECK_ADD=--enable=all
>> ... lots of output
>>
>> [1] -
>> http://public-inbox.org/git/1390993371-2431-1-git-send-email-gitter.spi...@gmail.com/#t
>>
>> Makefile | 4
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/Makefile b/Makefile
>> index f53fcc90d..8b5976d88 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -2635,3 +2635,7 @@ cover_db: coverage-report
>> cover_db_html: cover_db
>> cover -report html -outputdir cover_db_html cover_db
>>
>> +.PHONY: cppcheck
>> +
>> +cppcheck:
>> + cppcheck --force --quiet --inline-suppr $(CPPCHECK_ADD) .
>
> If I'm permitted a little GNU make-ism the following might make
> CPPCHECK_ADD a bit more usable
>
> + cppcheck --force --quiet --inline-suppr $(if
> $(CPPCHECK_ADD),--enable=$(CPPCHECK_ADD)) .
>
> Which would take us from
>
> $ make cppcheck CPPCHECK_ADD=--enable=all
>
> to
>
> $ make cppcheck CPPCHECK_ADD=all
Hhhmmmbut this allows for only specifying options to '--enable'.
The other version is much more flexible (i.e. allows for other complete options
as well).
Just my 0,02€
Stefan
--
/dev/random says: I'd love to, but I prefer to remain an enigma.
python -c "print
'73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')"
GPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9 9666 829B 49C5 9221 27AF
Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines
A Sáb, 10-12-2016 às 14:09 -0800, Junio C Hamano escreveu:
> We only update comment_line_char from the default "#" when the
> configured value is a single-byte character and we ignore incorrect
> values in the configuration file. So I think the patch you sent is
> correct after all.
I am still not sure what version do we prefer.
Check whether core.commentchar is a single character. If not, use '#'
as the $comment_line_char.
+sub get_comment_line_char {
+ my $comment_line_char = config("core.commentchar") || '#';
+ $comment_line_char = '#' if ($comment_line_char eq 'auto');
+ $comment_line_char = '#' if (length($comment_line_char) != 1);
+ return $comment_line_char;
+}
Check whether core.commentchar is a single character. If not, use the
first character of the core.commentchar value, mirroring the "rebase
-i" behavior introduced recently.
+sub get_comment_line_char {
+ my $comment_line_char = config("core.commentchar") || '#';
+ $comment_line_char = '#' if ($comment_line_char eq 'auto');
+ if (length($comment_line_char) != 1) {
+ # use first character
+ $comment_line_char = substr($comment_line_char, 0, 1);
+ }
+ return $comment_line_char;
+}
Or akin to what I had in the first patch related to handling 'auto'
value of core.commentchar configuration variable:
+sub get_comment_line_char {
+ my $comment_line_char = config("core.commentchar") || '#';
+ $comment_line_char = '#' if ($comment_line_char eq 'auto');
+ return $comment_line_char;
+}
Which assumes that the value of core.commentchar configuration variable
is either 'auto' or one single character, or the variable is not
defined.
Re: [PATCHv2] git-p4: support git worktrees
On Sun, Dec 11, 2016 at 2:19 PM, Luke Diamand wrote: > On 10 December 2016 at 21:57, Luke Diamand wrote: >> git-p4 would attempt to find the git directory using >> its own specific code, which did not know about git >> worktrees. This caused git operations to fail needlessly. >> >> Rework it to use "git rev-parse --git-dir" instead, which >> knows about worktrees. > > Actually this doesn't work as well as the original version. "git > rev-parse --git-dir" won't go and find the ".git" subdirectory. The > previous version would go looking for it, so this would introduce a > regression. Maybe git rev-parse --git-path HEAD, then strip out the /HEAD part? -- Duy
Re: [PATCH 1/2] alternates: accept double-quoted paths
On Tue, Dec 13, 2016 at 2:52 AM, Jeff King wrote: > Instead, let's treat names as unquoted unless they begin > with a double-quote, in which case they are interpreted via > our usual C-stylke quoting rules. This also breaks > backwards-compatibility, but in a smaller way: it only > matters if your file has a double-quote as the very _first_ > character in the path (whereas an escape character is a > problem anywhere in the path). It's also consistent with > many other parts of git, which accept either a bare pathname > or a double-quoted one, and the sender can choose to quote > or not as required. At least attr has the same problem and is going the same direction [1]. Cool. (I actually thought the patch was in and evidence that this kind of backward compatibility breaking was ok, turns out the patch has stayed around for years) [1] http://public-inbox.org/git/%3c20161110203428.30512-18-sbel...@google.com%3E/ -- Duy
Re: [PATCH 2/2] tmp-objdir: quote paths we add to alternates
On Mon, Dec 12, 2016 at 09:53:11PM +0100, Johannes Sixt wrote:
> > The appropriate check there would be ";" anyway, but I am not
> > sure _that_ is allowed in paths, either.
>
> That was also my line of thought. I tested earlier today whether a file name
> can have ";", and the OS did not reject it bluntly. Let me see whether I can
> adjust the test case for Windows.
The naive conversion is just:
diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh
index 6275ec807..e195e168b 100755
--- a/t/t5547-push-quarantine.sh
+++ b/t/t5547-push-quarantine.sh
@@ -33,8 +33,13 @@ test_expect_success 'rejected objects are removed' '
test_cmp expect actual
'
-# MINGW does not allow colons in pathnames in the first place
-test_expect_success !MINGW 'push to repo path with colon' '
+if test_have_prereq MINGW
+then
+ path_sep=';'
+else
+ path_sep=':'
+fi
+test_expect_success 'push to repo path with (semi-)colon separator' '
# The interesting failure case here is when the
# receiving end cannot access its original object directory,
# so make it likely for us to generate a delta by having
@@ -43,13 +48,13 @@ test_expect_success !MINGW 'push to repo path with colon' '
test-genrandom foo 4096 >file.bin &&
git add file.bin &&
git commit -m bin &&
- git clone --bare . xxx:yyy.git &&
+ git clone --bare . xxx${path_sep}yyy.git &&
echo change >>file.bin &&
git commit -am change &&
# Note that we have to use the full path here, or it gets confused
# with the ssh host:path syntax.
- git push "$PWD/xxx:yyy.git" HEAD
+ git push "$PWD/xxx${path_sep}yyy.git" HEAD
'
test_done
Does that work?
-Peff
Re: [PATCH 0/2] handling alternates paths with colons
On Mon, Dec 12, 2016 at 02:37:08PM -0800, Junio C Hamano wrote: > Jeff King writes: > > > So here are patches that do that. It kicks in only when the first > > character of a path is a double-quote, and then expects the usual > > C-style quoting. > > The quote being per delimited component is what makes this fifth > approach a huge win. > > All sane components on a list-valued environment are still honored > and an insane component that has either a colon in the middle or > begins with a double-quote gets quoted. As long as nobody used a > path that begins with a double-quote as an element in such a > list-valued environment (and they cannot be, as using a non-absolute > path as an element does not make much sense), this will be safe, and > a path with a colon didn't work with Git unaware of the new quoting > rule anyway. Nice. We do support non-absolute paths, both in alternates files and environment variables. It's a nice feature if you want to have a relocatable family of shared repositories. I'd imagine that most cases start with "../", though. -Peff
Re: [PATCH 1/2] alternates: accept double-quoted paths
On Tue, Dec 13, 2016 at 06:30:15PM +0700, Duy Nguyen wrote: > On Tue, Dec 13, 2016 at 2:52 AM, Jeff King wrote: > > Instead, let's treat names as unquoted unless they begin > > with a double-quote, in which case they are interpreted via > > our usual C-stylke quoting rules. This also breaks > > backwards-compatibility, but in a smaller way: it only > > matters if your file has a double-quote as the very _first_ > > character in the path (whereas an escape character is a > > problem anywhere in the path). It's also consistent with > > many other parts of git, which accept either a bare pathname > > or a double-quoted one, and the sender can choose to quote > > or not as required. > > At least attr has the same problem and is going the same direction > [1]. Cool. (I actually thought the patch was in and evidence that this > kind of backward compatibility breaking was ok, turns out the patch > has stayed around for years) > > [1] > http://public-inbox.org/git/%3c20161110203428.30512-18-sbel...@google.com%3E/ Thanks for digging that up. As soon as I came up with the idea[1], I wanted to use the attr code as an example of a similar problem and solution, but I couldn't find it in the code. Which makes sense if it wasn't merged. I do think it's a pretty reasonable approach in general, and would be OK for the attributes code. -Peff [1] One could argue that I did not come up with the idea at all, but rather just remembered that somebody else had done so. :)
Re: [PATCHv2 1/2] merge: Add '--continue' option as a synonym for 'git commit'
On Tue, Dec 13, 2016 at 09:48:58PM +1300, Chris Packham wrote:
> + if (continue_current_merge) {
> + int nargc = 1;
> + const char *nargv[] = {"commit", NULL};
> +
> + if (argc)
> + usage_msg_opt("--continue expects no arguments",
> + builtin_merge_usage, builtin_merge_options);
This checks that we don't have:
git merge --continue foobar
but still allows:
git merge --continue --some-option
because parse_options() decrements argc.
It would be insane to check individually which options might have been
set. But I wonder if we could do something like:
int orig_argc = argc;
...
argc = parse_options(argc, argv, ...);
if (continue_current_merge) {
if (orig_argc != 1) /* maybe 2, to account for argv[0] ? */
usage_msg_opt("--continue expects no arguments", ...);
}
That gets trickier if there ever is an option that's OK to use with
--continue. We might want to forward along "--quiet", for example. On
the other hand, we silently ignore it now, so maybe it is better to
complain and then let --quiet get added later if somebody cares.
Whatever we do here, I think "--abort" should get the same treatment
(probably as a separate patch).
> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index 85248a14b..44b34ef3a 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -154,6 +154,7 @@ test_expect_success 'test option parsing' '
> test_must_fail git merge -s foobar c1 &&
> test_must_fail git merge -s=foobar c1 &&
> test_must_fail git merge -m &&
> + test_must_fail git merge --continue foobar &&
> test_must_fail git merge
> '
Your tests look good, though obviously if you check for options above,
that should be covered in this test.
-Peff
Re: [RFC/PATCH] Makefile: add cppcheck target
On Tue, Dec 13, 2016 at 10:22:25PM +1300, Chris Packham wrote:
> $ make cppcheck
> cppcheck --force --quiet --inline-suppr .
> [compat/nedmalloc/malloc.c.h:4093]: (error) Possible null pointer
> dereference: sp
> [compat/nedmalloc/malloc.c.h:4106]: (error) Possible null pointer
> dereference: sp
> [compat/nedmalloc/nedmalloc.c:551]: (error) Expression
> '*(&p.mycache)=TlsAlloc(),TLS_OUT_OF_INDEXES==*(&p.mycache)' depends on order
> of evaluation of side effects
> [compat/regex/regcomp.c:3086]: (error) Memory leak: sbcset
> [compat/regex/regcomp.c:3634]: (error) Memory leak: sbcset
> [compat/regex/regcomp.c:3086]: (error) Memory leak: mbcset
> [compat/regex/regcomp.c:3634]: (error) Memory leak: mbcset
> [compat/regex/regcomp.c:2802]: (error) Uninitialized variable: table_size
> [compat/regex/regcomp.c:2805]: (error) Uninitialized variable: table_size
> [compat/regex/regcomp.c:532]: (error) Memory leak: fastmap
> [t/t4051/appended1.c:3]: (error) Invalid number of character '{' when these
> macros are defined: ''.
> [t/t4051/appended2.c:35]: (error) Invalid number of character '{' when these
> macros are defined: ''.
>
> The last 2 are just false positives from test data. I haven't looked
> into any of the others.
I think these last two are a good sign that we need to be feeding the
list of source files to cppcheck. I tried your patch and it also started
looking in t/perf/build, which are old versions of git built to serve
the performance-testing suite.
See the way that the "tags" target is handled for a possible approach.
My main complaint with any static checker is how we can handle false
positives. I think our use of "-Wall -Werror" is successful because it's
not too hard to keep the normal state to zero warnings. Looking at the
output of cppcheck on my system (which is different than on yours!), I
do see a few real problems, but many false positives, too.
Unfortunately, one of the false positives is:
int foo = foo;
to silence -Wuninitialized, which causes cppcheck to complain that "foo"
is uninitialized. I'm worried we will end up with two static checkers
fighting each other, and no good way to please both.
-Peff
Re: [RFC/PATCH] Makefile: add cppcheck target
On Tue, Dec 13, 2016 at 07:15:10AM -0500, Jeff King wrote: > I think these last two are a good sign that we need to be feeding the > list of source files to cppcheck. I tried your patch and it also started > looking in t/perf/build, which are old versions of git built to serve > the performance-testing suite. > > See the way that the "tags" target is handled for a possible approach. Maybe something like this: diff --git a/Makefile b/Makefile index 8b5976d88..e7684ae63 100644 --- a/Makefile +++ b/Makefile @@ -2638,4 +2638,6 @@ cover_db_html: cover_db .PHONY: cppcheck cppcheck: - cppcheck --force --quiet --inline-suppr $(CPPCHECK_ADD) . + $(FIND_SOURCE_FILES) |\ + grep -v ^t/t |\ + xargs cppcheck --force --quiet --inline-suppr $(CPPCHECK_ADD) > My main complaint with any static checker is how we can handle false > positives. [...] Here's what that generates on my machine, with annotations: [builtin/am.c:766]: (error) Resource leak: in Correct. [builtin/notes.c:260]: (error) Memory pointed to by 'buf' is freed twice. [builtin/notes.c:264]: (error) Memory pointed to by 'buf' is freed twice. Nope. It appears not to understand that die() does not return. [builtin/rev-list.c:391]: (error) Uninitialized variable: reaches [builtin/rev-list.c:391]: (error) Uninitialized variable: all These are "int foo = foo" (which is ugly, and maybe we can get rid of if compilers have gotten smart enough to figure it out). [compat/nedmalloc/malloc.c.h:4646]: (error) Memory leak: mem Hard to tell, but I think this is wrong and is confused by pointer arithmetic on the malloc chunks. [compat/regex/regcomp.c:3086]: (error) Memory leak: sbcset Nope, this return is hit only when sbcset is NULL. The tool is presumably confused by a conditional hidden inside a macro. [compat/regex/regcomp.c:3634]: (error) Memory leak: sbcset [compat/regex/regcomp.c:3086]: (error) Memory leak: mbcset [compat/regex/regcomp.c:3634]: (error) Memory leak: mbcset I didn't look at these, but presumably similar. [compat/regex/regcomp.c:2802]: (error) Uninitialized variable: table_size [compat/regex/regcomp.c:2805]: (error) Uninitialized variable: table_size Not sure, but it looks like this function declares another inline function inside its scope, and that confuses the tool. [compat/regex/regcomp.c:532]: (error) Memory leak: fastmap Nope. Tool seems confused by hiding free() in a macro. [contrib/examples/builtin-fetch--tool.c:420]: (error) Uninitialized variable: lrr_count [contrib/examples/builtin-fetch--tool.c:427]: (error) Uninitialized variable: lrr_list More "int foo = foo". Might be worth omitting contrib/examples (or possibly contrib/ entirely) from the check. [t/helper/test-hashmap.c:125]: (error) Memory leak: entries [t/helper/test-hashmap.c:125]: (error) Memory leak: hashes Correct (but unimportant). So I think it is capable of finding real problems, but I think we'd need some way of squelching false positives, preferably in a way that carries forward as the code changes (so not just saying "foo.c:1234 is a false positive", which will break when it becomes "foo.c:1235"). -Peff
Re: [PATCH 1/4] doc: add articles (grammar)
Thank you for reviewing this series, Philip. Philip Oakley writes: > This looks good to me. I'll add this header: Acked-by: Philip Oakley To the commit message of this patch in the next review round (version 2 of the patch series). Let me know if I should add another header, or do something different than this. -- Kristoffer Haugsbakk
Re: [PATCH 3/4] doc: make the intent of sentence clearer
Philip Oakley writes: > Looks like a reasonable emphasis to me I'll add this header: Acked-by: Philip Oakley To the commit message of this patch in the next review round (version 2 of the patch series). Let me know if I should add another header, or do something different than this. -- Kristoffer Haugsbakk
[PATCH] builtin/commit.c: convert trivial snprintf calls to xsnprintf
With the commits f2f02675 and 5096d490 we have been converted in some files the
call from snprintf/sprintf/strcpy to xsnprintf. This patch supersedes the
snprintf with several calls that make use of the heap rather than fixed
length buffers (eg. PATH_MAX) that may be incorrect on some systems.
Signed-off-by: Elia Pinto
---
builtin/commit.c | 32 +++-
1 file changed, 15 insertions(+), 17 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 0ed634b26..37228330c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -960,15 +960,16 @@ static int prepare_to_commit(const char *index_file,
const char *prefix,
return 0;
if (use_editor) {
- char index[PATH_MAX];
- const char *env[2] = { NULL };
- env[0] = index;
- snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
- if (launch_editor(git_path_commit_editmsg(), NULL, env)) {
+ struct argv_array env = ARGV_ARRAY_INIT;
+
+ argv_array_pushf(&env, "GIT_INDEX_FILE=%s", index_file);
+ if (launch_editor(git_path_commit_editmsg(), NULL, env.argv)) {
fprintf(stderr,
_("Please supply the message using either -m or -F
option.\n"));
+ argv_array_clear(&env);
exit(1);
}
+ argv_array_clear(&env);
}
if (!no_verify &&
@@ -1525,12 +1526,10 @@ static int git_commit_config(const char *k, const char
*v, void *cb)
static int run_rewrite_hook(const unsigned char *oldsha1,
const unsigned char *newsha1)
{
- /* oldsha1 SP newsha1 LF NUL */
- static char buf[2*40 + 3];
+ char *buf;
struct child_process proc = CHILD_PROCESS_INIT;
const char *argv[3];
int code;
- size_t n;
argv[0] = find_hook("post-rewrite");
if (!argv[0])
@@ -1546,34 +1545,33 @@ static int run_rewrite_hook(const unsigned char
*oldsha1,
code = start_command(&proc);
if (code)
return code;
- n = snprintf(buf, sizeof(buf), "%s %s\n",
-sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
+ buf = xstrfmt("%s %s\n", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
sigchain_push(SIGPIPE, SIG_IGN);
- write_in_full(proc.in, buf, n);
+ write_in_full(proc.in, buf, strlen(buf));
close(proc.in);
+ free(buf);
sigchain_pop(SIGPIPE);
return finish_command(&proc);
}
int run_commit_hook(int editor_is_used, const char *index_file, const char
*name, ...)
{
- const char *hook_env[3] = { NULL };
- char index[PATH_MAX];
+ struct argv_array hook_env = ARGV_ARRAY_INIT;
va_list args;
int ret;
- snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
- hook_env[0] = index;
+ argv_array_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file);
/*
* Let the hook know that no editor will be launched.
*/
if (!editor_is_used)
- hook_env[1] = "GIT_EDITOR=:";
+ argv_array_push(&hook_env, "GIT_EDITOR=:");
va_start(args, name);
- ret = run_hook_ve(hook_env, name, args);
+ ret = run_hook_ve(hook_env.argv,name, args);
va_end(args);
+ argv_array_clear(&hook_env);
return ret;
}
--
2.11.0.153.gacc9cba
Re: [PATCH] builtin/commit.c: convert trivial snprintf calls to xsnprintf
On Tue, Dec 13, 2016 at 01:27:17PM +, Elia Pinto wrote:
> With the commits f2f02675 and 5096d490 we have been converted in some files
> the call from snprintf/sprintf/strcpy to xsnprintf. This patch supersedes the
> snprintf with several calls that make use of the heap rather than fixed
> length buffers (eg. PATH_MAX) that may be incorrect on some systems.
I had trouble parsing this, because it leads with mention of commits
that _aren't_ doing the same thing we are doing here. I think the
argument you want to make is basically:
1. snprintf is bad because it may silently truncate results if we're
wrong.
2. In cases where we use PATH_MAX, we'd want to handle larger paths
anyway, so we should switch to dynamic allocation.
3. In other cases where we know the input is bounded to a certain
length we could use xsnprintf(), which asserts that we don't
truncate. But by switching to dynamic allocation, we can avoid
dealing with magic numbers in the code.
I'd actually consider splitting cases (2) and (3) into separate patches.
Even though the end result is the same, the reasoning is quite
different.
As far as the patch itself goes, they all look like improvements to me.
The extra allocations shouldn't matter because these are all heavy
operations already.
A few minor nits:
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 0ed634b26..37228330c 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -960,15 +960,16 @@ static int prepare_to_commit(const char *index_file,
> const char *prefix,
> return 0;
>
> if (use_editor) {
> - char index[PATH_MAX];
> - const char *env[2] = { NULL };
> - env[0] = index;
> - snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
> - if (launch_editor(git_path_commit_editmsg(), NULL, env)) {
> + struct argv_array env = ARGV_ARRAY_INIT;
> +
> + argv_array_pushf(&env, "GIT_INDEX_FILE=%s", index_file);
> + if (launch_editor(git_path_commit_editmsg(), NULL, env.argv)) {
> fprintf(stderr,
> _("Please supply the message using either -m or -F
> option.\n"));
> + argv_array_clear(&env);
> exit(1);
> }
> + argv_array_clear(&env);
I'd generally skip the clear() right before exiting, though I saw
somebody disagree with me recently on that. I wonder if we should
decide as a project on whether it is worth doing or not.
> @@ -1525,12 +1526,10 @@ static int git_commit_config(const char *k, const
> char *v, void *cb)
> static int run_rewrite_hook(const unsigned char *oldsha1,
> const unsigned char *newsha1)
> {
> - /* oldsha1 SP newsha1 LF NUL */
> - static char buf[2*40 + 3];
> + char *buf;
> struct child_process proc = CHILD_PROCESS_INIT;
> const char *argv[3];
> int code;
> - size_t n;
>
> argv[0] = find_hook("post-rewrite");
> if (!argv[0])
> @@ -1546,34 +1545,33 @@ static int run_rewrite_hook(const unsigned char
> *oldsha1,
> code = start_command(&proc);
> if (code)
> return code;
> - n = snprintf(buf, sizeof(buf), "%s %s\n",
> - sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
> + buf = xstrfmt("%s %s\n", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
> sigchain_push(SIGPIPE, SIG_IGN);
> - write_in_full(proc.in, buf, n);
> + write_in_full(proc.in, buf, strlen(buf));
> close(proc.in);
> + free(buf);
Any time you care about the length of the result, I'd generally use an
actual strbuf instead of xstrfmt. The extra strlen isn't a big deal
here, but it somehow seems simpler to me. It probably doesn't matter
much either way, though.
> int run_commit_hook(int editor_is_used, const char *index_file, const char
> *name, ...)
> {
> - const char *hook_env[3] = { NULL };
> - char index[PATH_MAX];
> + struct argv_array hook_env = ARGV_ARRAY_INIT;
> va_list args;
> int ret;
>
> - snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
> - hook_env[0] = index;
> + argv_array_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file);
>
> /*
>* Let the hook know that no editor will be launched.
>*/
> if (!editor_is_used)
> - hook_env[1] = "GIT_EDITOR=:";
> + argv_array_push(&hook_env, "GIT_EDITOR=:");
>
> va_start(args, name);
> - ret = run_hook_ve(hook_env, name, args);
> + ret = run_hook_ve(hook_env.argv,name, args);
> va_end(args);
> + argv_array_clear(&hook_env);
Missing whitespace between arguments in the run_hook_ve() call.
-Peff
[PATCH v2 04/34] sequencer (rebase -i): implement the 'exec' command
The 'exec' command is a little special among rebase -i's commands, as it
does *not* have a SHA-1 as first parameter. Instead, everything after the
`exec` command is treated as command-line to execute.
Let's reuse the arg/arg_len fields of the todo_item structure (which hold
the oneline for pick/edit commands) to point to the command-line.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 59 +++
1 file changed, 59 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index 68e2c84803..700b7575ed 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -17,6 +17,7 @@
#include "argv-array.h"
#include "quote.h"
#include "log-tree.h"
+#include "wt-status.h"
#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
@@ -664,6 +665,8 @@ enum todo_command {
TODO_PICK = 0,
TODO_REVERT,
TODO_EDIT,
+ /* commands that do something else than handling a single commit */
+ TODO_EXEC,
/* commands that do nothing but are counted for reporting progress */
TODO_NOOP
};
@@ -672,6 +675,7 @@ static const char *todo_command_strings[] = {
"pick",
"revert",
"edit",
+ "exec",
"noop"
};
@@ -971,6 +975,12 @@ static int parse_insn_line(struct todo_item *item, const
char *bol, char *eol)
return -1;
bol += padding;
+ if (item->command == TODO_EXEC) {
+ item->arg = bol;
+ item->arg_len = (int)(eol - bol);
+ return 0;
+ }
+
end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
saved = *end_of_object_name;
*end_of_object_name = '\0';
@@ -1396,6 +1406,47 @@ static int error_with_patch(struct commit *commit,
return exit_code;
}
+static int do_exec(const char *command_line)
+{
+ const char *child_argv[] = { NULL, NULL };
+ int dirty, status;
+
+ fprintf(stderr, "Executing: %s\n", command_line);
+ child_argv[0] = command_line;
+ status = run_command_v_opt(child_argv, RUN_USING_SHELL);
+
+ /* force re-reading of the cache */
+ if (discard_cache() < 0 || read_cache() < 0)
+ return error(_("could not read index"));
+
+ dirty = require_clean_work_tree("rebase", NULL, 1, 1);
+
+ if (status) {
+ warning(_("execution failed: %s\n%s"
+ "You can fix the problem, and then run\n"
+ "\n"
+ " git rebase --continue\n"
+ "\n"),
+ command_line,
+ dirty ? N_("and made changes to the index and/or the "
+ "working tree\n") : "");
+ if (status == 127)
+ /* command not found */
+ status = 1;
+ }
+ else if (dirty) {
+ warning(_("execution succeeded: %s\nbut "
+ "left changes to the index and/or the working tree\n"
+ "Commit or stash your changes, and then run\n"
+ "\n"
+ " git rebase --continue\n"
+ "\n"), command_line);
+ status = 1;
+ }
+
+ return status;
+}
+
static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
{
int res = 0;
@@ -1425,6 +1476,14 @@ static int pick_commits(struct todo_list *todo_list,
struct replay_opts *opts)
!res);
}
}
+ else if (item->command == TODO_EXEC) {
+ char *end_of_arg = (char *)(item->arg + item->arg_len);
+ int saved = *end_of_arg;
+
+ *end_of_arg = '\0';
+ res = do_exec(item->arg);
+ *end_of_arg = saved;
+ }
else if (item->command != TODO_NOOP)
return error(_("unknown command %d"), item->command);
--
2.11.0.rc3.windows.1
[PATCH v2 05/34] sequencer (rebase -i): learn about the 'verbose' mode
When calling `git rebase -i -v`, the user wants to see some statistics
after the commits were rebased. Let's show some.
The strbuf we use to perform that task will be used for other things
in subsequent commits, hence it is declared and initialized in a wider
scope than strictly needed here.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 22 ++
sequencer.h | 1 +
2 files changed, 23 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index 700b7575ed..1ab50884bd 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -63,6 +63,8 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha,
"rebase-merge/stopped-sha")
* command-line (and are only consumed, not modified, by the sequencer).
*/
static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")
+static GIT_PATH_FUNC(rebase_path_orig_head, "rebase-merge/orig-head")
+static GIT_PATH_FUNC(rebase_path_verbose, "rebase-merge/verbose")
static inline int is_rebase_i(const struct replay_opts *opts)
{
@@ -1121,6 +1123,9 @@ static int read_populate_opts(struct replay_opts *opts)
}
strbuf_release(&buf);
+ if (file_exists(rebase_path_verbose()))
+ opts->verbose = 1;
+
return 0;
}
@@ -1493,9 +1498,26 @@ static int pick_commits(struct todo_list *todo_list,
struct replay_opts *opts)
}
if (is_rebase_i(opts)) {
+ struct strbuf buf = STRBUF_INIT;
+
/* Stopped in the middle, as planned? */
if (todo_list->current < todo_list->nr)
return 0;
+
+ if (opts->verbose) {
+ const char *argv[] = {
+ "diff-tree", "--stat", NULL, NULL
+ };
+
+ if (!read_oneliner(&buf, rebase_path_orig_head(), 0))
+ return error(_("could not read '%s'"),
+ rebase_path_orig_head());
+ strbuf_addstr(&buf, "..HEAD");
+ argv[2] = buf.buf;
+ run_command_v_opt(argv, RUN_GIT_CMD);
+ strbuf_reset(&buf);
+ }
+ strbuf_release(&buf);
}
/*
diff --git a/sequencer.h b/sequencer.h
index cb21cfddee..f885b68395 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -24,6 +24,7 @@ struct replay_opts {
int allow_empty;
int allow_empty_message;
int keep_redundant_commits;
+ int verbose;
int mainline;
--
2.11.0.rc3.windows.1
[PATCH v2 06/34] sequencer (rebase -i): write the 'done' file
In the interactive rebase, commands that were successfully processed are
not simply discarded, but appended to the 'done' file instead. This is
used e.g. to display the current state to the user in the output of
`git status` or the progress.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 23 +++
1 file changed, 23 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index 1ab50884bd..f6e20b142a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -39,6 +39,12 @@ static GIT_PATH_FUNC(rebase_path, "rebase-merge")
*/
static GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
/*
+ * The rebase command lines that have already been processed. A line
+ * is moved here when it is first handled, before any associated user
+ * actions.
+ */
+static GIT_PATH_FUNC(rebase_path_done, "rebase-merge/done")
+/*
* A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
* GIT_AUTHOR_DATE that will be used for the commit that is currently
* being rebased.
@@ -1295,6 +1301,23 @@ static int save_todo(struct todo_list *todo_list, struct
replay_opts *opts)
return error_errno(_("could not write to '%s'"), todo_path);
if (commit_lock_file(&todo_lock) < 0)
return error(_("failed to finalize '%s'."), todo_path);
+
+ if (is_rebase_i(opts)) {
+ const char *done_path = rebase_path_done();
+ int fd = open(done_path, O_CREAT | O_WRONLY | O_APPEND, 0666);
+ int prev_offset = !next ? 0 :
+ todo_list->items[next - 1].offset_in_buf;
+
+ if (fd >= 0 && offset > prev_offset &&
+ write_in_full(fd, todo_list->buf.buf + prev_offset,
+ offset - prev_offset) < 0) {
+ close(fd);
+ return error_errno(_("could not write to '%s'"),
+ done_path);
+ }
+ if (fd >= 0)
+ close(fd);
+ }
return 0;
}
--
2.11.0.rc3.windows.1
[PATCH v2 03/34] sequencer (rebase -i): implement the 'edit' command
This patch is a straight-forward reimplementation of the `edit`
operation of the interactive rebase command.
Well, not *quite* straight-forward: when stopping, the `edit`
command wants to write the `patch` file (which is not only the
patch, but includes the commit message and author information). To
that end, this patch requires the earlier work that taught the
log-tree machinery to respect the `file` setting of
rev_info->diffopt to write to a file stream different than stdout.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 116 ++--
1 file changed, 114 insertions(+), 2 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 1224799286..68e2c84803 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -16,6 +16,7 @@
#include "refs.h"
#include "argv-array.h"
#include "quote.h"
+#include "log-tree.h"
#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
@@ -43,6 +44,20 @@ static GIT_PATH_FUNC(rebase_path_todo,
"rebase-merge/git-rebase-todo")
*/
static GIT_PATH_FUNC(rebase_path_author_script, "rebase-merge/author-script")
/*
+ * When an "edit" rebase command is being processed, the SHA1 of the
+ * commit to be edited is recorded in this file. When "git rebase
+ * --continue" is executed, if there are any staged changes then they
+ * will be amended to the HEAD commit, but only provided the HEAD
+ * commit is still the commit to be edited. When any other rebase
+ * command is processed, this file is deleted.
+ */
+static GIT_PATH_FUNC(rebase_path_amend, "rebase-merge/amend")
+/*
+ * When we stop at a given patch via the "edit" command, this file contains
+ * the long commit name of the corresponding patch.
+ */
+static GIT_PATH_FUNC(rebase_path_stopped_sha, "rebase-merge/stopped-sha")
+/*
* The following files are written by git-rebase just after parsing the
* command-line (and are only consumed, not modified, by the sequencer).
*/
@@ -648,6 +663,7 @@ enum todo_command {
/* commands that handle commits */
TODO_PICK = 0,
TODO_REVERT,
+ TODO_EDIT,
/* commands that do nothing but are counted for reporting progress */
TODO_NOOP
};
@@ -655,6 +671,7 @@ enum todo_command {
static const char *todo_command_strings[] = {
"pick",
"revert",
+ "edit",
"noop"
};
@@ -1301,9 +1318,87 @@ static int save_opts(struct replay_opts *opts)
return res;
}
+static int make_patch(struct commit *commit, struct replay_opts *opts)
+{
+ struct strbuf buf = STRBUF_INIT;
+ struct rev_info log_tree_opt;
+ const char *commit_buffer = get_commit_buffer(commit, NULL), *subject,
*p;
+ int res = 0;
+
+ p = short_commit_name(commit);
+ if (write_message(p, strlen(p), rebase_path_stopped_sha(), 1) < 0)
+ return -1;
+
+ strbuf_addf(&buf, "%s/patch", get_dir(opts));
+ memset(&log_tree_opt, 0, sizeof(log_tree_opt));
+ init_revisions(&log_tree_opt, NULL);
+ log_tree_opt.abbrev = 0;
+ log_tree_opt.diff = 1;
+ log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;
+ log_tree_opt.disable_stdin = 1;
+ log_tree_opt.no_commit_id = 1;
+ log_tree_opt.diffopt.file = fopen(buf.buf, "w");
+ log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;
+ if (!log_tree_opt.diffopt.file)
+ res |= error_errno(_("could not open '%s'"), buf.buf);
+ else {
+ res |= log_tree_commit(&log_tree_opt, commit);
+ fclose(log_tree_opt.diffopt.file);
+ }
+ strbuf_reset(&buf);
+
+ strbuf_addf(&buf, "%s/message", get_dir(opts));
+ if (!file_exists(buf.buf)) {
+ find_commit_subject(commit_buffer, &subject);
+ res |= write_message(subject, strlen(subject), buf.buf, 1);
+ unuse_commit_buffer(commit, commit_buffer);
+ }
+ strbuf_release(&buf);
+
+ return res;
+}
+
+static int intend_to_amend(void)
+{
+ unsigned char head[20];
+ char *p;
+
+ if (get_sha1("HEAD", head))
+ return error(_("cannot read HEAD"));
+
+ p = sha1_to_hex(head);
+ return write_message(p, strlen(p), rebase_path_amend(), 1);
+}
+
+static int error_with_patch(struct commit *commit,
+ const char *subject, int subject_len,
+ struct replay_opts *opts, int exit_code, int to_amend)
+{
+ if (make_patch(commit, opts))
+ return -1;
+
+ if (to_amend) {
+ if (intend_to_amend())
+ return -1;
+
+ fprintf(stderr, "You can amend the commit now, with\n"
+ "\n"
+ " git commit --amend %s\n"
+ "\n"
+ "Once you are satisfied with your changes, run\n"
+ "\n"
+ " git rebase --continue\n", gpg_sign_opt_quoted(opts));
+ }
+ else if (exit_code)
+ fprintf(stderr,
[PATCH v2 27/34] sequencer (rebase -i): differentiate between comments and 'noop'
In the upcoming patch, we will support rebase -i's progress
reporting. The progress skips comments but counts 'noop's.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 1f314b2743..63f6f25ced 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -770,7 +770,9 @@ enum todo_command {
TODO_EXEC,
/* commands that do nothing but are counted for reporting progress */
TODO_NOOP,
- TODO_DROP
+ TODO_DROP,
+ /* comments (not counted for reporting progress) */
+ TODO_COMMENT
};
static struct {
@@ -785,12 +787,13 @@ static struct {
{ 's', "squash" },
{ 'x', "exec" },
{ 0, "noop" },
- { 'd', "drop" }
+ { 'd', "drop" },
+ { 0, NULL }
};
static const char *command_to_string(const enum todo_command command)
{
- if ((size_t)command < ARRAY_SIZE(todo_command_info))
+ if (command < TODO_COMMENT)
return todo_command_info[command].str;
die("Unknown command: %d", command);
}
@@ -1237,14 +1240,14 @@ static int parse_insn_line(struct todo_item *item,
const char *bol, char *eol)
bol += strspn(bol, " \t");
if (bol == eol || *bol == '\r' || *bol == comment_line_char) {
- item->command = TODO_NOOP;
+ item->command = TODO_COMMENT;
item->commit = NULL;
item->arg = bol;
item->arg_len = eol - bol;
return 0;
}
- for (i = 0; i < ARRAY_SIZE(todo_command_info); i++)
+ for (i = 0; i < TODO_COMMENT; i++)
if (skip_prefix(bol, todo_command_info[i].str, &bol)) {
item->command = i;
break;
@@ -1254,7 +1257,7 @@ static int parse_insn_line(struct todo_item *item, const
char *bol, char *eol)
item->command = i;
break;
}
- if (i >= ARRAY_SIZE(todo_command_info))
+ if (i >= TODO_COMMENT)
return -1;
if (item->command == TODO_NOOP) {
--
2.11.0.rc3.windows.1
[PATCH v2 07/34] sequencer (rebase -i): add support for the 'fixup' and 'squash' commands
This is a huge patch, and at the same time a huge step forward to
execute the performance-critical parts of the interactive rebase in a
builtin command.
Since 'fixup' and 'squash' are not only similar, but also need to know
about each other (we want to reduce a series of fixups/squashes into a
single, final commit message edit, from the user's point of view), we
really have to implement them both at the same time.
Most of the actual work is done by the existing code path that already
handles the "pick" and the "edit" commands; We added support for other
features (e.g. to amend the commit message) in the patches leading up to
this one, yet there are still quite a few bits in this patch that simply
would not make sense as individual patches (such as: determining whether
there was anything to "fix up" in the "todo" script, etc).
In theory, it would be possible to reuse the fast-forward code path also
for the fixup and the squash code paths, but in practice this would make
the code less readable. The end result cannot be fast-forwarded anyway,
therefore let's just extend the cherry-picking code path for now.
Since the sequencer parses the entire `git-rebase-todo` script in one go,
fixup or squash commands without a preceding pick can be reported early
(in git-rebase--interactive, we could only report such errors just before
executing the fixup/squash).
Signed-off-by: Johannes Schindelin
---
sequencer.c | 232 +---
1 file changed, 222 insertions(+), 10 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index f6e20b142a..271c21581d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -45,6 +45,35 @@ static GIT_PATH_FUNC(rebase_path_todo,
"rebase-merge/git-rebase-todo")
*/
static GIT_PATH_FUNC(rebase_path_done, "rebase-merge/done")
/*
+ * The commit message that is planned to be used for any changes that
+ * need to be committed following a user interaction.
+ */
+static GIT_PATH_FUNC(rebase_path_message, "rebase-merge/message")
+/*
+ * The file into which is accumulated the suggested commit message for
+ * squash/fixup commands. When the first of a series of squash/fixups
+ * is seen, the file is created and the commit message from the
+ * previous commit and from the first squash/fixup commit are written
+ * to it. The commit message for each subsequent squash/fixup commit
+ * is appended to the file as it is processed.
+ *
+ * The first line of the file is of the form
+ * # This is a combination of $count commits.
+ * where $count is the number of commits whose messages have been
+ * written to the file so far (including the initial "pick" commit).
+ * Each time that a commit message is processed, this line is read and
+ * updated. It is deleted just before the combined commit is made.
+ */
+static GIT_PATH_FUNC(rebase_path_squash_msg, "rebase-merge/message-squash")
+/*
+ * If the current series of squash/fixups has not yet included a squash
+ * command, then this file exists and holds the commit message of the
+ * original "pick" commit. (If the series ends without a "squash"
+ * command, then this can be used as the commit message of the combined
+ * commit without opening the editor.)
+ */
+static GIT_PATH_FUNC(rebase_path_fixup_msg, "rebase-merge/message-fixup")
+/*
* A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
* GIT_AUTHOR_DATE that will be used for the commit that is currently
* being rebased.
@@ -673,6 +702,8 @@ enum todo_command {
TODO_PICK = 0,
TODO_REVERT,
TODO_EDIT,
+ TODO_FIXUP,
+ TODO_SQUASH,
/* commands that do something else than handling a single commit */
TODO_EXEC,
/* commands that do nothing but are counted for reporting progress */
@@ -683,6 +714,8 @@ static const char *todo_command_strings[] = {
"pick",
"revert",
"edit",
+ "fixup",
+ "squash",
"exec",
"noop"
};
@@ -694,16 +727,119 @@ static const char *command_to_string(const enum
todo_command command)
die("Unknown command: %d", command);
}
+static int is_fixup(enum todo_command command)
+{
+ return command == TODO_FIXUP || command == TODO_SQUASH;
+}
+
+static int update_squash_messages(enum todo_command command,
+ struct commit *commit, struct replay_opts *opts)
+{
+ struct strbuf buf = STRBUF_INIT;
+ int count, res;
+ const char *message, *body;
+
+ if (file_exists(rebase_path_squash_msg())) {
+ char *p, *p2;
+
+ if (strbuf_read_file(&buf, rebase_path_squash_msg(), 2048) <= 0)
+ return error(_("could not read '%s'"),
+ rebase_path_squash_msg());
+
+ if (buf.buf[0] != comment_line_char ||
+ !skip_prefix(buf.buf + 1, " This is a combination of ",
+(const char **)&p))
+ return error(_("unexpected 1st line of squash message:"
[PATCH v2 12/34] sequencer (rebase -i): skip some revert/cherry-pick specific code path
When a cherry-pick continues without a "todo script", the intention is
simply to pick a single commit.
However, when an interactive rebase is continued without a "todo
script", it means that the last command has been completed and that we
now need to clean up.
This commit guards the revert/cherry-pick specific steps so that they
are not executed in rebase -i mode.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 25 ++---
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index abffaf3b40..4ceb6f3ac5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1882,25 +1882,28 @@ int sequencer_continue(struct replay_opts *opts)
if (commit_staged_changes(opts))
return -1;
}
- if (!file_exists(get_todo_path(opts)))
+ else if (!file_exists(get_todo_path(opts)))
return continue_single_pick();
if (read_populate_opts(opts))
return -1;
if ((res = read_populate_todo(&todo_list, opts)))
goto release_todo_list;
- /* Verify that the conflict has been resolved */
- if (file_exists(git_path_cherry_pick_head()) ||
- file_exists(git_path_revert_head())) {
- res = continue_single_pick();
- if (res)
+ if (!is_rebase_i(opts)) {
+ /* Verify that the conflict has been resolved */
+ if (file_exists(git_path_cherry_pick_head()) ||
+ file_exists(git_path_revert_head())) {
+ res = continue_single_pick();
+ if (res)
+ goto release_todo_list;
+ }
+ if (index_differs_from("HEAD", 0, 0)) {
+ res = error_dirty_index(opts);
goto release_todo_list;
+ }
+ todo_list.current++;
}
- if (index_differs_from("HEAD", 0, 0)) {
- res = error_dirty_index(opts);
- goto release_todo_list;
- }
- todo_list.current++;
+
res = pick_commits(&todo_list, opts);
release_todo_list:
todo_list_release(&todo_list);
--
2.11.0.rc3.windows.1
[PATCH v2 14/34] sequencer (rebase -i): update refs after a successful rebase
An interactive rebase operates on a detached HEAD (to keep the reflog
of the original branch relatively clean), and updates the branch only
at the end.
Now that the sequencer learns to perform interactive rebases, it also
needs to learn the trick to update the branch before removing the
directory containing the state of the interactive rebase.
We introduce a new head_ref variable in a wider scope than necessary at
the moment, to allow for a later patch that prints out "Successfully
rebased and updated ".
Signed-off-by: Johannes Schindelin
---
sequencer.c | 32 +++-
1 file changed, 31 insertions(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index a6625e765d..a4e9b326ba 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -100,6 +100,8 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha,
"rebase-merge/stopped-sha")
static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")
static GIT_PATH_FUNC(rebase_path_orig_head, "rebase-merge/orig-head")
static GIT_PATH_FUNC(rebase_path_verbose, "rebase-merge/verbose")
+static GIT_PATH_FUNC(rebase_path_head_name, "rebase-merge/head-name")
+static GIT_PATH_FUNC(rebase_path_onto, "rebase-merge/onto")
static inline int is_rebase_i(const struct replay_opts *opts)
{
@@ -1793,12 +1795,39 @@ static int pick_commits(struct todo_list *todo_list,
struct replay_opts *opts)
}
if (is_rebase_i(opts)) {
- struct strbuf buf = STRBUF_INIT;
+ struct strbuf head_ref = STRBUF_INIT, buf = STRBUF_INIT;
/* Stopped in the middle, as planned? */
if (todo_list->current < todo_list->nr)
return 0;
+ if (read_oneliner(&head_ref, rebase_path_head_name(), 0) &&
+ starts_with(head_ref.buf, "refs/")) {
+ unsigned char head[20], orig[20];
+
+ if (get_sha1("HEAD", head))
+ return error(_("cannot read HEAD"));
+ if (!read_oneliner(&buf, rebase_path_orig_head(), 0) ||
+ get_sha1_hex(buf.buf, orig))
+ return error(_("could not read orig-head"));
+ strbuf_addf(&buf, "rebase -i (finish): %s onto ",
+ head_ref.buf);
+ if (!read_oneliner(&buf, rebase_path_onto(), 0))
+ return error(_("could not read 'onto'"));
+ if (update_ref(buf.buf, head_ref.buf, head, orig,
+ REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))
+ return error(_("could not update %s"),
+ head_ref.buf);
+ strbuf_reset(&buf);
+ strbuf_addf(&buf,
+ "rebase -i (finish): returning to %s",
+ head_ref.buf);
+ if (create_symref("HEAD", head_ref.buf, buf.buf))
+ return error(_("could not update HEAD to %s"),
+ head_ref.buf);
+ strbuf_reset(&buf);
+ }
+
if (opts->verbose) {
const char *argv[] = {
"diff-tree", "--stat", NULL, NULL
@@ -1813,6 +1842,7 @@ static int pick_commits(struct todo_list *todo_list,
struct replay_opts *opts)
strbuf_reset(&buf);
}
strbuf_release(&buf);
+ strbuf_release(&head_ref);
}
/*
--
2.11.0.rc3.windows.1
[PATCH v2 26/34] sequencer (rebase -i): implement the 'drop' command
The parsing part of a 'drop' command is almost identical to parsing a
'pick', while the operation is the same as that of a 'noop'.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 03256b5b1d..1f314b2743 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -769,7 +769,8 @@ enum todo_command {
/* commands that do something else than handling a single commit */
TODO_EXEC,
/* commands that do nothing but are counted for reporting progress */
- TODO_NOOP
+ TODO_NOOP,
+ TODO_DROP
};
static struct {
@@ -783,7 +784,8 @@ static struct {
{ 'f', "fixup" },
{ 's', "squash" },
{ 'x', "exec" },
- { 0, "noop" }
+ { 0, "noop" },
+ { 'd', "drop" }
};
static const char *command_to_string(const enum todo_command command)
@@ -1317,7 +1319,7 @@ static int parse_insn_buffer(char *buf, struct todo_list
*todo_list)
else if (is_fixup(item->command))
return error(_("cannot '%s' without a previous commit"),
command_to_string(item->command));
- else if (item->command != TODO_NOOP)
+ else if (item->command < TODO_NOOP)
fixup_okay = 1;
}
@@ -1827,7 +1829,7 @@ static enum todo_command peek_command(struct todo_list
*todo_list, int offset)
int i;
for (i = todo_list->current + offset; i < todo_list->nr; i++)
- if (todo_list->items[i].command != TODO_NOOP)
+ if (todo_list->items[i].command < TODO_NOOP)
return todo_list->items[i].command;
return -1;
@@ -1960,7 +1962,7 @@ static int pick_commits(struct todo_list *todo_list,
struct replay_opts *opts)
res = do_exec(item->arg);
*end_of_arg = saved;
}
- else if (item->command != TODO_NOOP)
+ else if (item->command < TODO_NOOP)
return error(_("unknown command %d"), item->command);
todo_list->current++;
--
2.11.0.rc3.windows.1
[PATCH v2 15/34] sequencer (rebase -i): leave a patch upon error
When doing an interactive rebase, we want to leave a 'patch' file for
further inspection by the user (even if we never tried to actually apply
that patch, since we're cherry-picking instead).
Signed-off-by: Johannes Schindelin
---
sequencer.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index a4e9b326ba..4361fe0e94 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1777,6 +1777,9 @@ static int pick_commits(struct todo_list *todo_list,
struct replay_opts *opts)
return error_failed_squash(item->commit, opts,
item->arg_len, item->arg);
}
+ else if (res && is_rebase_i(opts))
+ return res | error_with_patch(item->commit,
+ item->arg, item->arg_len, opts, res, 0);
}
else if (item->command == TODO_EXEC) {
char *end_of_arg = (char *)(item->arg + item->arg_len);
--
2.11.0.rc3.windows.1
[PATCH v2 13/34] sequencer (rebase -i): the todo can be empty when continuing
When the last command of an interactive rebase fails, the user needs to
resolve the problem and then continue the interactive rebase. Naturally,
the todo script is empty by then. So let's not complain about that!
To that end, let's move that test out of the function that parses the
todo script, and into the more high-level function read_populate_todo().
This is also necessary by now because the lower-level parse_insn_buffer()
has no idea whether we are performing an interactive rebase or not.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 4ceb6f3ac5..a6625e765d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1255,8 +1255,7 @@ static int parse_insn_buffer(char *buf, struct todo_list
*todo_list)
else if (item->command != TODO_NOOP)
fixup_okay = 1;
}
- if (!todo_list->nr)
- return error(_("no commits parsed."));
+
return res;
}
@@ -1280,6 +1279,10 @@ static int read_populate_todo(struct todo_list
*todo_list,
if (res)
return error(_("unusable instruction sheet: '%s'"), todo_file);
+ if (!todo_list->nr &&
+ (!is_rebase_i(opts) || !file_exists(rebase_path_done(
+ return error(_("no commits parsed."));
+
if (!is_rebase_i(opts)) {
enum todo_command valid =
opts->action == REPLAY_PICK ? TODO_PICK : TODO_REVERT;
--
2.11.0.rc3.windows.1
[PATCH v2 25/34] sequencer (rebase -i): allow rescheduling commands
The interactive rebase has the very special magic that a cherry-pick
that exits with a status different from 0 and 1 signifies a failure to
even record that a cherry-pick was started.
This can happen e.g. when a fast-forward fails because it would
overwrite untracked files.
In that case, we must reschedule the command that we thought we already
had at least started successfully.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index a67ecec961..03256b5b1d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1922,6 +1922,12 @@ static int pick_commits(struct todo_list *todo_list,
struct replay_opts *opts)
1);
res = do_pick_commit(item->command, item->commit,
opts, is_final_fixup(todo_list));
+ if (is_rebase_i(opts) && res < 0) {
+ /* Reschedule */
+ todo_list->current--;
+ if (save_todo(todo_list, opts))
+ return -1;
+ }
if (item->command == TODO_EDIT) {
struct commit *commit = item->commit;
if (!res)
--
2.11.0.rc3.windows.1
[PATCH v2 23/34] sequencer (rebase -i): respect the rebase.autostash setting
Git's `rebase` command inspects the `rebase.autostash` config setting
to determine whether it should stash any uncommitted changes before
rebasing and re-apply them afterwards.
As we introduce more bits and pieces to let the sequencer act as
interactive rebase's backend, here is the part that adds support for
the autostash feature.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 43 +++
1 file changed, 43 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index cafd945e83..c11eceb70b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -111,6 +111,7 @@ static GIT_PATH_FUNC(rebase_path_orig_head,
"rebase-merge/orig-head")
static GIT_PATH_FUNC(rebase_path_verbose, "rebase-merge/verbose")
static GIT_PATH_FUNC(rebase_path_head_name, "rebase-merge/head-name")
static GIT_PATH_FUNC(rebase_path_onto, "rebase-merge/onto")
+static GIT_PATH_FUNC(rebase_path_autostash, "rebase-merge/autostash")
static inline int is_rebase_i(const struct replay_opts *opts)
{
@@ -1808,6 +1809,47 @@ static enum todo_command peek_command(struct todo_list
*todo_list, int offset)
return -1;
}
+static int apply_autostash(struct replay_opts *opts)
+{
+ struct strbuf stash_sha1 = STRBUF_INIT;
+ struct child_process child = CHILD_PROCESS_INIT;
+ int ret = 0;
+
+ if (!read_oneliner(&stash_sha1, rebase_path_autostash(), 1)) {
+ strbuf_release(&stash_sha1);
+ return 0;
+ }
+ strbuf_trim(&stash_sha1);
+
+ child.git_cmd = 1;
+ argv_array_push(&child.args, "stash");
+ argv_array_push(&child.args, "apply");
+ argv_array_push(&child.args, stash_sha1.buf);
+ if (!run_command(&child))
+ printf(_("Applied autostash."));
+ else {
+ struct child_process store = CHILD_PROCESS_INIT;
+
+ store.git_cmd = 1;
+ argv_array_push(&store.args, "stash");
+ argv_array_push(&store.args, "store");
+ argv_array_push(&store.args, "-m");
+ argv_array_push(&store.args, "autostash");
+ argv_array_push(&store.args, "-q");
+ argv_array_push(&store.args, stash_sha1.buf);
+ if (run_command(&store))
+ ret = error(_("cannot store %s"), stash_sha1.buf);
+ else
+ printf(_("Applying autostash resulted in conflicts.\n"
+ "Your changes are safe in the stash.\n"
+ "You can run \"git stash pop\" or"
+ " \"git stash drop\" at any time.\n"));
+ }
+
+ strbuf_release(&stash_sha1);
+ return ret;
+}
+
static const char *reflog_message(struct replay_opts *opts,
const char *sub_action, const char *fmt, ...)
{
@@ -1970,6 +2012,7 @@ static int pick_commits(struct todo_list *todo_list,
struct replay_opts *opts)
run_command(&hook);
}
}
+ apply_autostash(opts);
strbuf_release(&buf);
strbuf_release(&head_ref);
--
2.11.0.rc3.windows.1
[PATCH v2 28/34] run_command_opt(): optionally hide stderr when the command succeeds
This will be needed to hide the output of `git commit` when the
sequencer handles an interactive rebase's script.
Signed-off-by: Johannes Schindelin
---
run-command.c | 23 +++
run-command.h | 1 +
2 files changed, 24 insertions(+)
diff --git a/run-command.c b/run-command.c
index ca905a9e80..5bb957afdd 100644
--- a/run-command.c
+++ b/run-command.c
@@ -589,6 +589,29 @@ int run_command_v_opt_cd_env(const char **argv, int opt,
const char *dir, const
cmd.clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;
cmd.dir = dir;
cmd.env = env;
+
+ if (opt & RUN_HIDE_STDERR_ON_SUCCESS) {
+ struct strbuf buf = STRBUF_INIT;
+ int res;
+
+ cmd.err = -1;
+ if (start_command(&cmd) < 0)
+ return -1;
+
+ if (strbuf_read(&buf, cmd.err, 0) < 0) {
+ close(cmd.err);
+ finish_command(&cmd); /* throw away exit code */
+ return -1;
+ }
+
+ close(cmd.err);
+ res = finish_command(&cmd);
+ if (res)
+ fputs(buf.buf, stderr);
+ strbuf_release(&buf);
+ return res;
+ }
+
return run_command(&cmd);
}
diff --git a/run-command.h b/run-command.h
index dd1c78c28d..65a21ddd4e 100644
--- a/run-command.h
+++ b/run-command.h
@@ -72,6 +72,7 @@ extern int run_hook_ve(const char *const *env, const char
*name, va_list args);
#define RUN_SILENT_EXEC_FAILURE 8
#define RUN_USING_SHELL 16
#define RUN_CLEAN_ON_EXIT 32
+#define RUN_HIDE_STDERR_ON_SUCCESS 64
int run_command_v_opt(const char **argv, int opt);
/*
--
2.11.0.rc3.windows.1
[PATCH v2 24/34] sequencer (rebase -i): respect strategy/strategy_opts settings
The sequencer already has an idea about using different merge
strategies. We just piggy-back on top of that, using rebase -i's
own settings, when running the sequencer in interactive rebase mode.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 26 +-
1 file changed, 25 insertions(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index c11eceb70b..a67ecec961 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -112,6 +112,8 @@ static GIT_PATH_FUNC(rebase_path_verbose,
"rebase-merge/verbose")
static GIT_PATH_FUNC(rebase_path_head_name, "rebase-merge/head-name")
static GIT_PATH_FUNC(rebase_path_onto, "rebase-merge/onto")
static GIT_PATH_FUNC(rebase_path_autostash, "rebase-merge/autostash")
+static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")
+static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
static inline int is_rebase_i(const struct replay_opts *opts)
{
@@ -1408,6 +1410,26 @@ static int populate_opts_cb(const char *key, const char
*value, void *data)
return 0;
}
+static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf)
+{
+ int i;
+
+ strbuf_reset(buf);
+ if (!read_oneliner(buf, rebase_path_strategy(), 0))
+ return;
+ opts->strategy = strbuf_detach(buf, NULL);
+ if (!read_oneliner(buf, rebase_path_strategy_opts(), 0))
+ return;
+
+ opts->xopts_nr = split_cmdline(buf->buf, (const char ***)&opts->xopts);
+ for (i = 0; i < opts->xopts_nr; i++) {
+ const char *arg = opts->xopts[i];
+
+ skip_prefix(arg, "--", &arg);
+ opts->xopts[i] = xstrdup(arg);
+ }
+}
+
static int read_populate_opts(struct replay_opts *opts)
{
if (is_rebase_i(opts)) {
@@ -1421,11 +1443,13 @@ static int read_populate_opts(struct replay_opts *opts)
opts->gpg_sign = xstrdup(buf.buf + 2);
}
}
- strbuf_release(&buf);
if (file_exists(rebase_path_verbose()))
opts->verbose = 1;
+ read_strategy_opts(opts, &buf);
+ strbuf_release(&buf);
+
return 0;
}
--
2.11.0.rc3.windows.1
[PATCH v2 18/34] sequencer (rebase -i): refactor setting the reflog message
This makes the code DRYer, with the obvious benefit that we can enhance
the code further in a single place.
We can also reuse the functionality elsewhere by calling this new
function.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 33 ++---
1 file changed, 26 insertions(+), 7 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 33fb36dcbe..d20efad562 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1750,6 +1750,26 @@ static int is_final_fixup(struct todo_list *todo_list)
return 1;
}
+static const char *reflog_message(struct replay_opts *opts,
+ const char *sub_action, const char *fmt, ...)
+{
+ va_list ap;
+ static struct strbuf buf = STRBUF_INIT;
+
+ va_start(ap, fmt);
+ strbuf_reset(&buf);
+ strbuf_addstr(&buf, action_name(opts));
+ if (sub_action)
+ strbuf_addf(&buf, " (%s)", sub_action);
+ if (fmt) {
+ strbuf_addstr(&buf, ": ");
+ strbuf_vaddf(&buf, fmt, ap);
+ }
+ va_end(ap);
+
+ return buf.buf;
+}
+
static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
{
int res = 0;
@@ -1820,6 +1840,7 @@ static int pick_commits(struct todo_list *todo_list,
struct replay_opts *opts)
if (read_oneliner(&head_ref, rebase_path_head_name(), 0) &&
starts_with(head_ref.buf, "refs/")) {
+ const char *msg;
unsigned char head[20], orig[20];
if (get_sha1("HEAD", head))
@@ -1827,19 +1848,17 @@ static int pick_commits(struct todo_list *todo_list,
struct replay_opts *opts)
if (!read_oneliner(&buf, rebase_path_orig_head(), 0) ||
get_sha1_hex(buf.buf, orig))
return error(_("could not read orig-head"));
- strbuf_addf(&buf, "rebase -i (finish): %s onto ",
- head_ref.buf);
if (!read_oneliner(&buf, rebase_path_onto(), 0))
return error(_("could not read 'onto'"));
- if (update_ref(buf.buf, head_ref.buf, head, orig,
+ msg = reflog_message(opts, "finish", "%s onto %s",
+ head_ref.buf, buf.buf);
+ if (update_ref(msg, head_ref.buf, head, orig,
REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))
return error(_("could not update %s"),
head_ref.buf);
- strbuf_reset(&buf);
- strbuf_addf(&buf,
- "rebase -i (finish): returning to %s",
+ msg = reflog_message(opts, "finish", "returning to %s",
head_ref.buf);
- if (create_symref("HEAD", head_ref.buf, buf.buf))
+ if (create_symref("HEAD", head_ref.buf, msg))
return error(_("could not update HEAD to %s"),
head_ref.buf);
strbuf_reset(&buf);
--
2.11.0.rc3.windows.1
[PATCH v2 21/34] sequencer (rebase -i): record interrupted commits in rewritten, too
When continuing after a `pick` command failed, we want that commit
to show up in the rewritten-list (and its notes to be rewritten), too.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index 7ab533abd9..0233999389 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2052,6 +2052,15 @@ int sequencer_continue(struct replay_opts *opts)
}
todo_list.current++;
}
+ else if (file_exists(rebase_path_stopped_sha())) {
+ struct strbuf buf = STRBUF_INIT;
+ struct object_id oid;
+
+ if (read_oneliner(&buf, rebase_path_stopped_sha(), 1) &&
+ !get_sha1_committish(buf.buf, oid.hash))
+ record_in_rewritten(&oid, peek_command(&todo_list, 0));
+ strbuf_release(&buf);
+ }
res = pick_commits(&todo_list, opts);
release_todo_list:
--
2.11.0.rc3.windows.1
[PATCH v2 19/34] sequencer (rebase -i): set the reflog message consistently
We already used the same reflog message as the scripted version of rebase
-i when finishing. With this commit, we do that also for all the commands
before that.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 4
1 file changed, 4 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index d20efad562..118696f6d3 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1792,6 +1792,10 @@ static int pick_commits(struct todo_list *todo_list,
struct replay_opts *opts)
unlink(rebase_path_amend());
}
if (item->command <= TODO_SQUASH) {
+ if (is_rebase_i(opts))
+ setenv("GIT_REFLOG_ACTION", reflog_message(opts,
+ command_to_string(item->command), NULL),
+ 1);
res = do_pick_commit(item->command, item->commit,
opts, is_final_fixup(todo_list));
if (item->command == TODO_EDIT) {
--
2.11.0.rc3.windows.1
[PATCH v2 32/34] sequencer (rebase -i): show the progress
The interactive rebase keeps the user informed about its progress.
If the sequencer wants to do the grunt work of the interactive
rebase, it also needs to show that progress.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 32
1 file changed, 32 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index 498dd028d1..35d5ef4ef6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1221,6 +1221,7 @@ struct todo_list {
struct strbuf buf;
struct todo_item *items;
int nr, alloc, current;
+ int done_nr, total_nr;
};
#define TODO_LIST_INIT { STRBUF_INIT }
@@ -1338,6 +1339,17 @@ static int parse_insn_buffer(char *buf, struct todo_list
*todo_list)
return res;
}
+static int count_commands(struct todo_list *todo_list)
+{
+ int count = 0, i;
+
+ for (i = 0; i < todo_list->nr; i++)
+ if (todo_list->items[i].command != TODO_COMMENT)
+ count++;
+
+ return count;
+}
+
static int read_populate_todo(struct todo_list *todo_list,
struct replay_opts *opts)
{
@@ -1380,6 +1392,21 @@ static int read_populate_todo(struct todo_list
*todo_list,
return error(_("cannot revert during a
cherry-pick."));
}
+ if (is_rebase_i(opts)) {
+ struct todo_list done = TODO_LIST_INIT;
+
+ if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&
+ !parse_insn_buffer(done.buf.buf, &done))
+ todo_list->done_nr = count_commands(&done);
+ else
+ todo_list->done_nr = 0;
+
+ todo_list->total_nr = todo_list->done_nr
+ + count_commands(todo_list);
+
+ todo_list_release(&done);
+ }
+
return 0;
}
@@ -1928,6 +1955,11 @@ static int pick_commits(struct todo_list *todo_list,
struct replay_opts *opts)
if (save_todo(todo_list, opts))
return -1;
if (is_rebase_i(opts)) {
+ if (item->command != TODO_COMMENT)
+ fprintf(stderr, "Rebasing (%d/%d)%s",
+ ++(todo_list->done_nr),
+ todo_list->total_nr,
+ opts->verbose ? "\n" : "\r");
unlink(rebase_path_message());
unlink(rebase_path_author_script());
unlink(rebase_path_stopped_sha());
--
2.11.0.rc3.windows.1
[PATCH v2 20/34] sequencer (rebase -i): copy commit notes at end
When rebasing commits that have commit notes attached, the interactive
rebase rewrites those notes faithfully at the end. The sequencer must
do this, too, if it wishes to do interactive rebase's job.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 76 +
1 file changed, 76 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index 118696f6d3..7ab533abd9 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -94,6 +94,15 @@ static GIT_PATH_FUNC(rebase_path_amend, "rebase-merge/amend")
*/
static GIT_PATH_FUNC(rebase_path_stopped_sha, "rebase-merge/stopped-sha")
/*
+ * For the post-rewrite hook, we make a list of rewritten commits and
+ * their new sha1s. The rewritten-pending list keeps the sha1s of
+ * commits that have been processed, but not committed yet,
+ * e.g. because they are waiting for a 'squash' command.
+ */
+static GIT_PATH_FUNC(rebase_path_rewritten_list, "rebase-merge/rewritten-list")
+static GIT_PATH_FUNC(rebase_path_rewritten_pending,
+ "rebase-merge/rewritten-pending")
+/*
* The following files are written by git-rebase just after parsing the
* command-line (and are only consumed, not modified, by the sequencer).
*/
@@ -883,6 +892,44 @@ static int update_squash_messages(enum todo_command
command,
return res;
}
+static void flush_rewritten_pending(void) {
+ struct strbuf buf = STRBUF_INIT;
+ unsigned char newsha1[20];
+ FILE *out;
+
+ if (strbuf_read_file(&buf, rebase_path_rewritten_pending(), 82) > 0 &&
+ !get_sha1("HEAD", newsha1) &&
+ (out = fopen(rebase_path_rewritten_list(), "a"))) {
+ char *bol = buf.buf, *eol;
+
+ while (*bol) {
+ eol = strchrnul(bol, '\n');
+ fprintf(out, "%.*s %s\n", (int)(eol - bol),
+ bol, sha1_to_hex(newsha1));
+ if (!*eol)
+ break;
+ bol = eol + 1;
+ }
+ fclose(out);
+ unlink(rebase_path_rewritten_pending());
+ }
+ strbuf_release(&buf);
+}
+
+static void record_in_rewritten(struct object_id *oid,
+ enum todo_command next_command) {
+ FILE *out = fopen(rebase_path_rewritten_pending(), "a");
+
+ if (!out)
+ return;
+
+ fprintf(out, "%s\n", oid_to_hex(oid));
+ fclose(out);
+
+ if (!is_fixup(next_command))
+ flush_rewritten_pending();
+}
+
static int do_pick_commit(enum todo_command command, struct commit *commit,
struct replay_opts *opts, int final_fixup)
{
@@ -1750,6 +1797,17 @@ static int is_final_fixup(struct todo_list *todo_list)
return 1;
}
+static enum todo_command peek_command(struct todo_list *todo_list, int offset)
+{
+ int i;
+
+ for (i = todo_list->current + offset; i < todo_list->nr; i++)
+ if (todo_list->items[i].command != TODO_NOOP)
+ return todo_list->items[i].command;
+
+ return -1;
+}
+
static const char *reflog_message(struct replay_opts *opts,
const char *sub_action, const char *fmt, ...)
{
@@ -1808,6 +1866,9 @@ static int pick_commits(struct todo_list *todo_list,
struct replay_opts *opts)
item->arg, item->arg_len, opts, res,
!res);
}
+ if (is_rebase_i(opts) && !res)
+ record_in_rewritten(&item->commit->object.oid,
+ peek_command(todo_list, 1));
if (res && is_fixup(item->command)) {
if (res == 1)
intend_to_amend();
@@ -1837,6 +1898,7 @@ static int pick_commits(struct todo_list *todo_list,
struct replay_opts *opts)
if (is_rebase_i(opts)) {
struct strbuf head_ref = STRBUF_INIT, buf = STRBUF_INIT;
+ struct stat st;
/* Stopped in the middle, as planned? */
if (todo_list->current < todo_list->nr)
@@ -1881,6 +1943,20 @@ static int pick_commits(struct todo_list *todo_list,
struct replay_opts *opts)
run_command_v_opt(argv, RUN_GIT_CMD);
strbuf_reset(&buf);
}
+ flush_rewritten_pending();
+ if (!stat(rebase_path_rewritten_list(), &st) &&
+ st.st_size > 0) {
+ struct child_process child = CHILD_PROCESS_INIT;
+
+ child.in = open(rebase_path_rewritten_list(), O_RDONLY);
+ child.git_cmd = 1;
+ argv_array_push(&child.args, "notes");
+ argv_array_push(&child.args, "copy");
+ argv_array_push(&child.args,
[PATCH v2 22/34] sequencer (rebase -i): run the post-rewrite hook, if needed
Signed-off-by: Johannes Schindelin
---
sequencer.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index 0233999389..cafd945e83 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1947,6 +1947,8 @@ static int pick_commits(struct todo_list *todo_list,
struct replay_opts *opts)
if (!stat(rebase_path_rewritten_list(), &st) &&
st.st_size > 0) {
struct child_process child = CHILD_PROCESS_INIT;
+ const char *post_rewrite_hook =
+ find_hook("post-rewrite");
child.in = open(rebase_path_rewritten_list(), O_RDONLY);
child.git_cmd = 1;
@@ -1955,6 +1957,18 @@ static int pick_commits(struct todo_list *todo_list,
struct replay_opts *opts)
argv_array_push(&child.args, "--for-rewrite=rebase");
/* we don't care if this copying failed */
run_command(&child);
+
+ if (post_rewrite_hook) {
+ struct child_process hook = CHILD_PROCESS_INIT;
+
+ hook.in = open(rebase_path_rewritten_list(),
+ O_RDONLY);
+ hook.stdout_to_stderr = 1;
+ argv_array_push(&hook.args, post_rewrite_hook);
+ argv_array_push(&hook.args, "rebase");
+ /* we don't care if this hook failed */
+ run_command(&hook);
+ }
}
strbuf_release(&buf);
--
2.11.0.rc3.windows.1
[PATCH v2 16/34] sequencer (rebase -i): implement the 'reword' command
This is now trivial, as all the building blocks are in place: all we need
to do is to flip the "edit" switch when committing.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 4361fe0e94..5a9972fec3 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -751,6 +751,7 @@ enum todo_command {
TODO_PICK = 0,
TODO_REVERT,
TODO_EDIT,
+ TODO_REWORD,
TODO_FIXUP,
TODO_SQUASH,
/* commands that do something else than handling a single commit */
@@ -766,6 +767,7 @@ static struct {
{ 'p', "pick" },
{ 0, "revert" },
{ 'e', "edit" },
+ { 'r', "reword" },
{ 'f', "fixup" },
{ 's', "squash" },
{ 'x', "exec" },
@@ -1001,7 +1003,9 @@ static int do_pick_commit(enum todo_command command,
struct commit *commit,
}
}
- if (is_fixup(command)) {
+ if (command == TODO_REWORD)
+ edit = 1;
+ else if (is_fixup(command)) {
if (update_squash_messages(command, commit, opts))
return -1;
amend = 1;
@@ -1779,7 +1783,8 @@ static int pick_commits(struct todo_list *todo_list,
struct replay_opts *opts)
}
else if (res && is_rebase_i(opts))
return res | error_with_patch(item->commit,
- item->arg, item->arg_len, opts, res, 0);
+ item->arg, item->arg_len, opts, res,
+ item->command == TODO_REWORD);
}
else if (item->command == TODO_EXEC) {
char *end_of_arg = (char *)(item->arg + item->arg_len);
--
2.11.0.rc3.windows.1
[PATCH v2 17/34] sequencer (rebase -i): allow fast-forwarding for edit/reword
The sequencer already knew how to fast-forward instead of
cherry-picking, if possible.
We want to continue to do this, of course, but in case of the 'reword'
command, we will need to call `git commit` after fast-forwarding.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 25 +
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 5a9972fec3..33fb36dcbe 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -893,7 +893,7 @@ static int do_pick_commit(enum todo_command command, struct
commit *commit,
const char *base_label, *next_label;
struct commit_message msg = { NULL, NULL, NULL, NULL };
struct strbuf msgbuf = STRBUF_INIT;
- int res, unborn = 0, amend = 0, allow;
+ int res, unborn = 0, amend = 0, allow = 0;
if (opts->no_commit) {
/*
@@ -939,11 +939,23 @@ static int do_pick_commit(enum todo_command command,
struct commit *commit,
else
parent = commit->parents->item;
+ if (get_message(commit, &msg) != 0)
+ return error(_("cannot get commit message for %s"),
+ oid_to_hex(&commit->object.oid));
+
if (opts->allow_ff && !is_fixup(command) &&
((parent && !hashcmp(parent->object.oid.hash, head)) ||
-(!parent && unborn)))
- return fast_forward_to(commit->object.oid.hash, head, unborn,
opts);
-
+(!parent && unborn))) {
+ if (is_rebase_i(opts))
+ write_author_script(msg.message);
+ res = fast_forward_to(commit->object.oid.hash, head, unborn,
+ opts);
+ if (res || command != TODO_REWORD)
+ goto leave;
+ edit = amend = 1;
+ msg_file = NULL;
+ goto fast_forward_edit;
+ }
if (parent && parse_commit(parent) < 0)
/* TRANSLATORS: The first %s will be a "todo" command like
"revert" or "pick", the second %s a SHA1. */
@@ -951,10 +963,6 @@ static int do_pick_commit(enum todo_command command,
struct commit *commit,
command_to_string(command),
oid_to_hex(&parent->object.oid));
- if (get_message(commit, &msg) != 0)
- return error(_("cannot get commit message for %s"),
- oid_to_hex(&commit->object.oid));
-
/*
* "commit" is an existing commit. We would want to apply
* the difference it introduces since its first parent "prev"
@@ -1084,6 +1092,7 @@ static int do_pick_commit(enum todo_command command,
struct commit *commit,
goto leave;
}
if (!opts->no_commit)
+fast_forward_edit:
res = run_git_commit(msg_file, opts, allow, edit, amend,
cleanup_commit_message);
--
2.11.0.rc3.windows.1
[PATCH v2 02/34] sequencer (rebase -i): implement the 'noop' command
The 'noop' command is probably the most boring of all rebase -i commands
to support in the sequencer.
Which makes it an excellent candidate for this first stab to add support
for rebase -i's commands to the sequencer.
For the moment, let's also treat empty lines and commented-out lines as
'noop'; We will refine that handling later in this patch series.
To make it easier to identify "classes" of todo_commands (such as:
determine whether a command is pick-like, i.e. handles a single commit),
let's enforce a certain order of said commands.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 35 ---
1 file changed, 32 insertions(+), 3 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 21cfdacd06..1224799286 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -639,14 +639,23 @@ static int allow_empty(struct replay_opts *opts, struct
commit *commit)
return 1;
}
+/*
+ * Note that ordering matters in this enum. Not only must it match the mapping
+ * below, it is also divided into several sections that matter. When adding
+ * new commands, make sure you add it in the right section.
+ */
enum todo_command {
+ /* commands that handle commits */
TODO_PICK = 0,
- TODO_REVERT
+ TODO_REVERT,
+ /* commands that do nothing but are counted for reporting progress */
+ TODO_NOOP
};
static const char *todo_command_strings[] = {
"pick",
- "revert"
+ "revert",
+ "noop"
};
static const char *command_to_string(const enum todo_command command)
@@ -916,6 +925,14 @@ static int parse_insn_line(struct todo_item *item, const
char *bol, char *eol)
/* left-trim */
bol += strspn(bol, " \t");
+ if (bol == eol || *bol == '\r' || *bol == comment_line_char) {
+ item->command = TODO_NOOP;
+ item->commit = NULL;
+ item->arg = bol;
+ item->arg_len = eol - bol;
+ return 0;
+ }
+
for (i = 0; i < ARRAY_SIZE(todo_command_strings); i++)
if (skip_prefix(bol, todo_command_strings[i], &bol)) {
item->command = i;
@@ -924,6 +941,13 @@ static int parse_insn_line(struct todo_item *item, const
char *bol, char *eol)
if (i >= ARRAY_SIZE(todo_command_strings))
return -1;
+ if (item->command == TODO_NOOP) {
+ item->commit = NULL;
+ item->arg = bol;
+ item->arg_len = eol - bol;
+ return 0;
+ }
+
/* Eat up extra spaces/ tabs before object name */
padding = strspn(bol, " \t");
if (!padding)
@@ -1292,7 +1316,12 @@ static int pick_commits(struct todo_list *todo_list,
struct replay_opts *opts)
struct todo_item *item = todo_list->items + todo_list->current;
if (save_todo(todo_list, opts))
return -1;
- res = do_pick_commit(item->command, item->commit, opts);
+ if (item->command <= TODO_REVERT)
+ res = do_pick_commit(item->command, item->commit,
+ opts);
+ else if (item->command != TODO_NOOP)
+ return error(_("unknown command %d"), item->command);
+
todo_list->current++;
if (res)
return res;
--
2.11.0.rc3.windows.1
[PATCH v2 01/34] sequencer: support a new action: 'interactive rebase'
This patch introduces a new action for the sequencer. It really does not
do a whole lot of its own right now, but lays the ground work for
patches to come. The intention, of course, is to finally make the
sequencer the work horse of the interactive rebase (the original idea
behind the "sequencer" concept).
Signed-off-by: Johannes Schindelin
---
sequencer.c | 33 +
sequencer.h | 3 ++-
2 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 30b10ba143..21cfdacd06 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -28,6 +28,14 @@ static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
+static GIT_PATH_FUNC(rebase_path, "rebase-merge")
+/*
+ * The file containing rebase commands, comments, and empty lines.
+ * This file is created by "git rebase -i" then edited by the user. As
+ * the lines are processed, they are removed from the front of this
+ * file and written to the tail of 'done'.
+ */
+static GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
/*
* A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
* GIT_AUTHOR_DATE that will be used for the commit that is currently
@@ -40,19 +48,22 @@ static GIT_PATH_FUNC(rebase_path_author_script,
"rebase-merge/author-script")
*/
static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")
-/* We will introduce the 'interactive rebase' mode later */
static inline int is_rebase_i(const struct replay_opts *opts)
{
- return 0;
+ return opts->action == REPLAY_INTERACTIVE_REBASE;
}
static const char *get_dir(const struct replay_opts *opts)
{
+ if (is_rebase_i(opts))
+ return rebase_path();
return git_path_seq_dir();
}
static const char *get_todo_path(const struct replay_opts *opts)
{
+ if (is_rebase_i(opts))
+ return rebase_path_todo();
return git_path_todo_file();
}
@@ -168,7 +179,15 @@ int sequencer_remove_state(struct replay_opts *opts)
static const char *action_name(const struct replay_opts *opts)
{
- return opts->action == REPLAY_REVERT ? N_("revert") : N_("cherry-pick");
+ switch (opts->action) {
+ case REPLAY_REVERT:
+ return N_("revert");
+ case REPLAY_PICK:
+ return N_("cherry-pick");
+ case REPLAY_INTERACTIVE_REBASE:
+ return N_("rebase -i");
+ }
+ die(_("Unknown action: %d"), opts->action);
}
struct commit_message {
@@ -395,7 +414,10 @@ static int do_recursive_merge(struct commit *base, struct
commit *next,
if (active_cache_changed &&
write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
- /* TRANSLATORS: %s will be "revert" or "cherry-pick" */
+ /*
+* TRANSLATORS: %s will be "revert", "cherry-pick" or
+* "rebase -i".
+*/
return error(_("%s: Unable to write new index file"),
_(action_name(opts)));
rollback_lock_file(&index_lock);
@@ -1204,6 +1226,9 @@ static int save_todo(struct todo_list *todo_list, struct
replay_opts *opts)
const char *todo_path = get_todo_path(opts);
int next = todo_list->current, offset, fd;
+ if (is_rebase_i(opts))
+ next++;
+
fd = hold_lock_file_for_update(&todo_lock, todo_path, 0);
if (fd < 0)
return error_errno(_("could not lock '%s'"), todo_path);
diff --git a/sequencer.h b/sequencer.h
index 7a513c576b..cb21cfddee 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -7,7 +7,8 @@ const char *git_path_seq_dir(void);
enum replay_action {
REPLAY_REVERT,
- REPLAY_PICK
+ REPLAY_PICK,
+ REPLAY_INTERACTIVE_REBASE
};
struct replay_opts {
--
2.11.0.rc3.windows.1
[PATCH v2 00/34] Teach the sequencer to act as rebase -i's backend
This marks the count down to '3': two more patch series after this (really tiny ones) and we have a faster rebase -i. The idea of this patch series is to teach the sequencer to understand all of the commands in `git-rebase-todo` scripts, to execute them and to behave pretty much very the same as `git rebase -i --continue` when called with the newly-introduced REPLAY_INTERACTIVE_REBASE mode. Most of these patches should be pretty much straight-forward. When not, I tried to make a point of describing enough background in the commit message. Please feel free to point out where my explanations fall short. Note that even after this patch series is applied, rebase -i is still unaffected. It will require the next patch series which introduces the rebase--helper that essentially implements `git rebase -i --continue` by calling the sequencer with the appropriate options. The final patch series will move a couple of pre- and post-processing steps into the rebase--helper/sequencer (such as expanding/shrinking the SHA-1s, reordering the fixup!/squash! lines, etc). This might sound like a mere add-on, but it is essential for the speed improvements: those stupid little processing steps really dominated the execution time in my tests. Apart from mostly cosmetic patches (and the occasional odd bug that I fixed promptly), I used these patches since mid May to perform all of my interactive rebases. In mid June, I had the idea to teach rebase -i to run *both* scripted rebase and rebase--helper and to cross-validate the results. This slowed down all my interactive rebases since, but helped me catch three rather obscure bugs (e.g. that git commit --fixup unfolds long onelines and rebase -i still finds the correct original commit). This is all only to say that I am rather confident that the current code does the job. Since sending out v1, I integrated all of these patch series into Git for Windows v2.10.0, where they have been live ever since, and used by myself (also in a Linux VM, as Git for Windows' master branch always builds also on Linux and passes the test suite, too). Just to reiterate why I do all this: it speeds up the interactive rebase substantially. Even with a not yet fully builtin rebase -i, but just the part after the user edited the `git-rebase-todo` script. The performance test I introduced to demonstrate this (p3404) shows a speed-up of +380% here (i.e. roughly 5x), from ~8.8 seconds to ~1.8 seconds. This is on Windows, where the performance impact of avoiding shell scripting is most noticable. On MacOSX and on Linux, the speed-up is less pronounced, but still noticable, at least if you trust Travis CI, which I abused to perform that test for me. Check for yourself (searching for "3404.2") here: https://travis-ci.org/git/git/builds/156295227. According to those logs, p3404 is speeded up from ~0.45 seconds to ~0.12 seconds on Linux (read: about 3.5x) and from ~1.7 seconds to ~0.5 seconds on MacOSX (read: almost 4x). Please note that the interdiff vs v1 is only of limited use: too many things changed in the meantime, in particular the prepare-sequencer branch that went through a couple of iterations before it found its way into git.git's master branch. So please take the interdiff with a mountain range of salt. Changes since v1: - some grammar touch-ups. - simplified determining the command string in walk_revs_populate_todo(). - removed the beautiful ordinal logic (to print out "1st", "2nd", "3rd" etc) and made things consistent with the current `rebase -i`. - while at it, marked more messages for translation. - added code-comments to clarify the order, and the sections, of the todo_command enum. - replaced one error(..., strerror(...)) to an error_errno(...). - downcased error messages - marked error messages for translation - adjusted the patches to account for sequencer_entrust() having been removed from the prepare-sequencer patch series by request of Junio. - moved the introduction of write_message_gently() into the patch introducing its first usage, i.e. the support for the 'edit' command. - adjusted some indentations - prevented an write_in_full() from being called after a failed open() - inserted a few forgotten strbuf_release() calls Johannes Schindelin (34): sequencer: support a new action: 'interactive rebase' sequencer (rebase -i): implement the 'noop' command sequencer (rebase -i): implement the 'edit' command sequencer (rebase -i): implement the 'exec' command sequencer (rebase -i): learn about the 'verbose' mode sequencer (rebase -i): write the 'done' file sequencer (rebase -i): add support for the 'fixup' and 'squash' commands sequencer (rebase -i): implement the short commands sequencer (rebase -i): write an author-script file sequencer (rebase -i): allow continuing with staged changes sequencer (rebase -i): remove CHERRY_PICK_HEAD when no longer needed sequencer (rebase -i): skip some revert/cherry-pick specific code path seque
[PATCH v2 08/34] sequencer (rebase -i): implement the short commands
For users' convenience, most rebase commands can be abbreviated, e.g.
'p' instead of 'pick' and 'x' instead of 'exec'. Let's teach the
sequencer to handle those abbreviated commands just fine.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 36 ++--
1 file changed, 22 insertions(+), 14 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 271c21581d..e443f4765d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -710,20 +710,23 @@ enum todo_command {
TODO_NOOP
};
-static const char *todo_command_strings[] = {
- "pick",
- "revert",
- "edit",
- "fixup",
- "squash",
- "exec",
- "noop"
+static struct {
+ char c;
+ const char *str;
+} todo_command_info[] = {
+ { 'p', "pick" },
+ { 0, "revert" },
+ { 'e', "edit" },
+ { 'f', "fixup" },
+ { 's', "squash" },
+ { 'x', "exec" },
+ { 0, "noop" }
};
static const char *command_to_string(const enum todo_command command)
{
- if ((size_t)command < ARRAY_SIZE(todo_command_strings))
- return todo_command_strings[command];
+ if ((size_t)command < ARRAY_SIZE(todo_command_info))
+ return todo_command_info[command].str;
die("Unknown command: %d", command);
}
@@ -1125,12 +1128,17 @@ static int parse_insn_line(struct todo_item *item,
const char *bol, char *eol)
return 0;
}
- for (i = 0; i < ARRAY_SIZE(todo_command_strings); i++)
- if (skip_prefix(bol, todo_command_strings[i], &bol)) {
+ for (i = 0; i < ARRAY_SIZE(todo_command_info); i++)
+ if (skip_prefix(bol, todo_command_info[i].str, &bol)) {
item->command = i;
break;
}
- if (i >= ARRAY_SIZE(todo_command_strings))
+ else if (bol[1] == ' ' && *bol == todo_command_info[i].c) {
+ bol++;
+ item->command = i;
+ break;
+ }
+ if (i >= ARRAY_SIZE(todo_command_info))
return -1;
if (item->command == TODO_NOOP) {
@@ -1325,7 +1333,7 @@ static int walk_revs_populate_todo(struct todo_list
*todo_list,
{
enum todo_command command = opts->action == REPLAY_PICK ?
TODO_PICK : TODO_REVERT;
- const char *command_string = todo_command_strings[command];
+ const char *command_string = todo_command_info[command].str;
struct commit *commit;
if (prepare_revs(opts))
--
2.11.0.rc3.windows.1
[PATCH v2 11/34] sequencer (rebase -i): remove CHERRY_PICK_HEAD when no longer needed
The scripted version of the interactive rebase already does that.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index 855d3ba503..abffaf3b40 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1835,8 +1835,13 @@ static int commit_staged_changes(struct replay_opts
*opts)
if (has_unstaged_changes(1))
return error(_("cannot rebase: You have unstaged changes."));
- if (!has_uncommitted_changes(0))
+ if (!has_uncommitted_changes(0)) {
+ const char *cherry_pick_head = git_path("CHERRY_PICK_HEAD");
+
+ if (file_exists(cherry_pick_head) && unlink(cherry_pick_head))
+ return error(_("could not remove CHERRY_PICK_HEAD"));
return 0;
+ }
if (file_exists(rebase_path_amend())) {
struct strbuf rev = STRBUF_INIT;
--
2.11.0.rc3.windows.1
[PATCH v2 09/34] sequencer (rebase -i): write an author-script file
When the interactive rebase aborts, it writes out an author-script file
to record the author information for the current commit. As we are about
to teach the sequencer how to perform the actions behind an interactive
rebase, it needs to write those author-script files, too.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 51 ++-
1 file changed, 50 insertions(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index e443f4765d..80469b6954 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -515,6 +515,53 @@ static int is_index_unchanged(void)
return !hashcmp(active_cache_tree->sha1,
head_commit->tree->object.oid.hash);
}
+static int write_author_script(const char *message)
+{
+ struct strbuf buf = STRBUF_INIT;
+ const char *eol;
+ int res;
+
+ for (;;)
+ if (!*message || starts_with(message, "\n")) {
+missing_author:
+ /* Missing 'author' line? */
+ unlink(rebase_path_author_script());
+ return 0;
+ }
+ else if (skip_prefix(message, "author ", &message))
+ break;
+ else if ((eol = strchr(message, '\n')))
+ message = eol + 1;
+ else
+ goto missing_author;
+
+ strbuf_addstr(&buf, "GIT_AUTHOR_NAME='");
+ while (*message && *message != '\n' && *message != '\r')
+ if (skip_prefix(message, " <", &message))
+ break;
+ else if (*message != '\'')
+ strbuf_addch(&buf, *(message++));
+ else
+ strbuf_addf(&buf, "'%c'", *(message++));
+ strbuf_addstr(&buf, "'\nGIT_AUTHOR_EMAIL='");
+ while (*message && *message != '\n' && *message != '\r')
+ if (skip_prefix(message, "> ", &message))
+ break;
+ else if (*message != '\'')
+ strbuf_addch(&buf, *(message++));
+ else
+ strbuf_addf(&buf, "'%c'", *(message++));
+ strbuf_addstr(&buf, "'\nGIT_AUTHOR_DATE='@");
+ while (*message && *message != '\n' && *message != '\r')
+ if (*message != '\'')
+ strbuf_addch(&buf, *(message++));
+ else
+ strbuf_addf(&buf, "'%c'", *(message++));
+ res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);
+ strbuf_release(&buf);
+ return res;
+}
+
/*
* Read the author-script file into an environment block, ready for use in
* run_command(), that can be free()d afterwards.
@@ -974,7 +1021,9 @@ static int do_pick_commit(enum todo_command command,
struct commit *commit,
}
}
- if (!opts->strategy || !strcmp(opts->strategy, "recursive") || command
== TODO_REVERT) {
+ if (is_rebase_i(opts) && write_author_script(msg.message) < 0)
+ res = -1;
+ else if (!opts->strategy || !strcmp(opts->strategy, "recursive") ||
command == TODO_REVERT) {
res = do_recursive_merge(base, next, base_label, next_label,
head, &msgbuf, opts);
if (res < 0)
--
2.11.0.rc3.windows.1
[PATCH v2 10/34] sequencer (rebase -i): allow continuing with staged changes
When an interactive rebase is interrupted, the user may stage changes
before continuing, and we need to commit those changes in that case.
Please note that the nested "if" added to the sequencer_continue() is
not combined into a single "if" because it will be extended with an
"else" clause in a later patch in this patch series.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 40
1 file changed, 40 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index 80469b6954..855d3ba503 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1829,6 +1829,42 @@ static int continue_single_pick(void)
return run_command_v_opt(argv, RUN_GIT_CMD);
}
+static int commit_staged_changes(struct replay_opts *opts)
+{
+ int amend = 0;
+
+ if (has_unstaged_changes(1))
+ return error(_("cannot rebase: You have unstaged changes."));
+ if (!has_uncommitted_changes(0))
+ return 0;
+
+ if (file_exists(rebase_path_amend())) {
+ struct strbuf rev = STRBUF_INIT;
+ unsigned char head[20], to_amend[20];
+
+ if (get_sha1("HEAD", head))
+ return error(_("cannot amend non-existing commit"));
+ if (!read_oneliner(&rev, rebase_path_amend(), 0))
+ return error(_("invalid file: '%s'"),
rebase_path_amend());
+ if (get_sha1_hex(rev.buf, to_amend))
+ return error(_("invalid contents: '%s'"),
+ rebase_path_amend());
+ if (hashcmp(head, to_amend))
+ return error(_("\nYou have uncommitted changes in your "
+ "working tree. Please, commit them\n"
+ "first and then run 'git rebase "
+ "--continue' again."));
+
+ strbuf_release(&rev);
+ amend = 1;
+ }
+
+ if (run_git_commit(rebase_path_message(), opts, 1, 1, amend, 0))
+ return error(_("could not commit staged changes."));
+ unlink(rebase_path_amend());
+ return 0;
+}
+
int sequencer_continue(struct replay_opts *opts)
{
struct todo_list todo_list = TODO_LIST_INIT;
@@ -1837,6 +1873,10 @@ int sequencer_continue(struct replay_opts *opts)
if (read_and_refresh_cache(opts))
return -1;
+ if (is_rebase_i(opts)) {
+ if (commit_staged_changes(opts))
+ return -1;
+ }
if (!file_exists(get_todo_path(opts)))
return continue_single_pick();
if (read_populate_opts(opts))
--
2.11.0.rc3.windows.1
[PATCH v2 29/34] sequencer (rebase -i): show only failed `git commit`'s output
This is the behavior of the shell script version of the interactive
rebase, by using the `output` function defined in `git-rebase.sh`.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 63f6f25ced..dfa4fab98b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -647,10 +647,15 @@ static int run_git_commit(const char *defmsg, struct
replay_opts *opts,
{
char **env = NULL;
struct argv_array array;
- int rc;
+ int opt = RUN_GIT_CMD, rc;
const char *value;
if (is_rebase_i(opts)) {
+ if (!edit) {
+ opt |= RUN_COMMAND_STDOUT_TO_STDERR;
+ opt |= RUN_HIDE_STDERR_ON_SUCCESS;
+ }
+
env = read_author_script();
if (!env) {
const char *gpg_opt = gpg_sign_opt_quoted(opts);
@@ -687,7 +692,7 @@ static int run_git_commit(const char *defmsg, struct
replay_opts *opts,
if (opts->allow_empty_message)
argv_array_push(&array, "--allow-empty-message");
- rc = run_command_v_opt_cd_env(array.argv, RUN_GIT_CMD, NULL,
+ rc = run_command_v_opt_cd_env(array.argv, opt, NULL,
(const char *const *)env);
argv_array_clear(&array);
free(env);
--
2.11.0.rc3.windows.1
[PATCH v2 30/34] sequencer (rebase -i): show only failed cherry-picks' output
This is the behavior of the shell script version of the interactive rebase, by using the `output` function defined in `git-rebase.sh`. Signed-off-by: Johannes Schindelin --- sequencer.c | 4 1 file changed, 4 insertions(+) diff --git a/sequencer.c b/sequencer.c index dfa4fab98b..edc213a2c8 100644 --- a/sequencer.c +++ b/sequencer.c @@ -464,6 +464,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next, o.ancestor = base ? base_label : "(empty tree)"; o.branch1 = "HEAD"; o.branch2 = next ? next_label : "(empty tree)"; + if (is_rebase_i(opts)) + o.buffer_output = 2; head_tree = parse_tree_indirect(head); next_tree = next ? next->tree : empty_tree(); @@ -475,6 +477,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next, clean = merge_trees(&o, head_tree, next_tree, base_tree, &result); + if (is_rebase_i(opts) && clean <= 0) + fputs(o.obuf.buf, stdout); strbuf_release(&o.obuf); if (clean < 0) return clean; -- 2.11.0.rc3.windows.1
Re: What's cooking in git.git (Dec 2016, #02; Mon, 12)
Hi Junio, On Mon, 12 Dec 2016, Junio C Hamano wrote: > * js/mingw-isatty (2016-12-11) 1 commit > (merged to 'next' on 2016-12-12 at 60c1da6676) > + mingw: intercept isatty() to handle /dev/null as Git expects it > > We often decide if a session is interactive by checking if the > standard I/O streams are connected to a TTY, but isatty() emulation > on Windows incorrectly returned true if it is used on NUL (i.e. an > equivalent to /dev/null). This has been fixed. I'd like to suggest a reword: we did not use an isatty() emulation, but Windows' own _isatty() function that simply has different semantics than what Git expected. *Now* we have an isatty() emulation that wraps _isatty() and emulates the behavior expected by Git. Thanks, Dscho
[PATCH v2 33/34] sequencer (rebase -i): write the progress into files
For the benefit of e.g. the shell prompt, the interactive rebase not
only displays the progress for the user to see, but also writes it into
the msgnum/end files in the state directory.
Teach the sequencer this new trick.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 30 +++---
1 file changed, 27 insertions(+), 3 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 35d5ef4ef6..cb5e7f35fc 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -45,6 +45,16 @@ static GIT_PATH_FUNC(rebase_path_todo,
"rebase-merge/git-rebase-todo")
*/
static GIT_PATH_FUNC(rebase_path_done, "rebase-merge/done")
/*
+ * The file to keep track of how many commands were already processed (e.g.
+ * for the prompt).
+ */
+static GIT_PATH_FUNC(rebase_path_msgnum, "rebase-merge/msgnum");
+/*
+ * The file to keep track of how many commands are to be processed in total
+ * (e.g. for the prompt).
+ */
+static GIT_PATH_FUNC(rebase_path_msgtotal, "rebase-merge/end");
+/*
* The commit message that is planned to be used for any changes that
* need to be committed following a user interaction.
*/
@@ -1394,6 +1404,7 @@ static int read_populate_todo(struct todo_list *todo_list,
if (is_rebase_i(opts)) {
struct todo_list done = TODO_LIST_INIT;
+ FILE *f = fopen(rebase_path_msgtotal(), "w");
if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&
!parse_insn_buffer(done.buf.buf, &done))
@@ -1403,8 +1414,12 @@ static int read_populate_todo(struct todo_list
*todo_list,
todo_list->total_nr = todo_list->done_nr
+ count_commands(todo_list);
-
todo_list_release(&done);
+
+ if (f) {
+ fprintf(f, "%d\n", todo_list->total_nr);
+ fclose(f);
+ }
}
return 0;
@@ -1955,11 +1970,20 @@ static int pick_commits(struct todo_list *todo_list,
struct replay_opts *opts)
if (save_todo(todo_list, opts))
return -1;
if (is_rebase_i(opts)) {
- if (item->command != TODO_COMMENT)
+ if (item->command != TODO_COMMENT) {
+ FILE *f = fopen(rebase_path_msgnum(), "w");
+
+ todo_list->done_nr++;
+
+ if (f) {
+ fprintf(f, "%d\n", todo_list->done_nr);
+ fclose(f);
+ }
fprintf(stderr, "Rebasing (%d/%d)%s",
- ++(todo_list->done_nr),
+ todo_list->done_nr,
todo_list->total_nr,
opts->verbose ? "\n" : "\r");
+ }
unlink(rebase_path_message());
unlink(rebase_path_author_script());
unlink(rebase_path_stopped_sha());
--
2.11.0.rc3.windows.1
[PATCH v2 34/34] sequencer (rebase -i): write out the final message
The shell script version of the interactive rebase has a very specific final message. Teach the sequencer to print the same. Signed-off-by: Johannes Schindelin --- sequencer.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sequencer.c b/sequencer.c index cb5e7f35fc..41be4cde16 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2118,6 +2118,9 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts) } apply_autostash(opts); + fprintf(stderr, "Successfully rebased and updated %s.\n", + head_ref.buf); + strbuf_release(&buf); strbuf_release(&head_ref); } -- 2.11.0.rc3.windows.1
[PATCH v2 31/34] sequencer (rebase -i): suggest --edit-todo upon unknown command
This is the same behavior as known from `git rebase -i`.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index edc213a2c8..498dd028d1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1355,8 +1355,12 @@ static int read_populate_todo(struct todo_list
*todo_list,
close(fd);
res = parse_insn_buffer(todo_list->buf.buf, todo_list);
- if (res)
+ if (res) {
+ if (is_rebase_i(opts))
+ return error(_("please fix this using "
+ "'git rebase --edit-todo'."));
return error(_("unusable instruction sheet: '%s'"), todo_file);
+ }
if (!todo_list->nr &&
(!is_rebase_i(opts) || !file_exists(rebase_path_done(
--
2.11.0.rc3.windows.1
Re: What's cooking in git.git (Dec 2016, #02; Mon, 12)
Hi Junio, On Mon, 12 Dec 2016, Junio C Hamano wrote: > * jc/bundle (2016-03-03) 6 commits > - index-pack: --clone-bundle option > - Merge branch 'jc/index-pack' into jc/bundle > - bundle v3: the beginning > - bundle: keep a copy of bundle file name in the in-core bundle header > - bundle: plug resource leak > - bundle doc: 'verify' is not about verifying the bundle > > The beginning of "split bundle", which could be one of the > ingredients to allow "git clone" traffic off of the core server > network to CDN. > > While I think it would make it easier for people to experiment and > build on if the topic is merged to 'next', I am at the same time a > bit reluctant to merge an unproven new topic that introduces a new > file format, which we may end up having to support til the end of > time. It is likely that to support a "prime clone from CDN", it > would need a lot more than just "these are the heads and the pack > data is over there", so this may not be sufficient. > > Will discard. You could mark it as experimental, subject to change, and merge it to `next` safely. Ciao, Dscho
Re: What's cooking in git.git (Dec 2016, #02; Mon, 12)
Johannes Schindelin writes: > Hi Junio, > > On Mon, 12 Dec 2016, Junio C Hamano wrote: > >> * js/mingw-isatty (2016-12-11) 1 commit >> (merged to 'next' on 2016-12-12 at 60c1da6676) >> + mingw: intercept isatty() to handle /dev/null as Git expects it >> >> We often decide if a session is interactive by checking if the >> standard I/O streams are connected to a TTY, but isatty() emulation >> on Windows incorrectly returned true if it is used on NUL (i.e. an >> equivalent to /dev/null). This has been fixed. > > I'd like to suggest a reword: we did not use an isatty() emulation, but > Windows' own _isatty() function that simply has different semantics than > what Git expected. *Now* we have an isatty() emulation that wraps > _isatty() and emulates the behavior expected by Git. Thanks for a comment. One of the things that the new code does with the fix is this: +/* In this file, we actually want to use Windows' own isatty(). */ +#undef isatty + which undoes "#define isatty winansi_isatty" that other code uses, so that the implementation of winansi_isatty() can say isatty() and get what people usually get when they say "isatty()" on Windows. Before or after that patch, there is no "#define isatty _isatty" in our codebase. I take all of the above to mean that Windows does give us isatty() function (not a macro--as otherwise it won't become available to us again by "#undef isatty"), that in turn internally calls what it calls _isatty() that says true for NUL? Following the above reasoning, I meant "whatever you get when you write isatty() on Windows" by "isatty() emulation on Windows" in the paragraph you are commenting on. I didn't say "what was written by Git for Windows folks to emulate isatty()" or "what was given by MS development tools", as that distinction is immaterial and does not change the value of the fix.
Re: What's cooking in git.git (Dec 2016, #02; Mon, 12)
Johannes Schindelin writes: >> While I think it would make it easier for people to experiment and >> build on if the topic is merged to 'next', I am at the same time a >> bit reluctant to merge an unproven new topic that introduces a new >> file format, which we may end up having to support til the end of >> time. It is likely that to support a "prime clone from CDN", it >> would need a lot more than just "these are the heads and the pack >> data is over there", so this may not be sufficient. >> >> Will discard. > > You could mark it as experimental, subject to change, and merge it to > `next` safely. Are you planning, or do you know somebody who plans to use that code soonish? Otherwise I'd prefer to drop it---at this point, the series is merely "just because we can", not "because we need it to further improve this or that".
Re: [RFC/PATCH v3 00/16] Add initial experimental external ODB support
On Wed, Nov 30, 2016 at 11:36 PM, Junio C Hamano wrote: > Christian Couder writes: > >> For now there should be one odb ref per blob. Each ref name should be >> refs/odbs// where is the sha1 of the blob stored >> in the external odb named . >> >> These odb refs should all point to a blob that should be stored in the >> Git repository and contain information about the blob stored in the >> external odb. This information can be specific to the external odb. >> The repos can then share this information using commands like: >> >> `git fetch origin "refs/odbs//*:refs/odbs//*"` > > Unless this is designed to serve only a handful of blobs, I cannot > see how this design would scale successfully. I notice you wrote > "For now" at the beginning, but what is the envisioned way this will > evolve in the future? In general I think that having a lot of refs is really a big problem right now in Git as many big organizations using Git are facing this problem in one form or another. So I think that support for a big number of refs is a separate and important problem that should and hopefully will be solved. My preferred way to solve it would be with something like Shawn's RefTree. I think it would also help regarding other problems like speeding up git protocol, tracking patch series (see git-series discussions), tools like https://www.softwareheritage.org/, ... If the "big number of refs" problem is not solved and many refs in refs/odbs// is a problem, it's possible to have just one ref in refs/odbs// that points to a blob that contains a list (maybe a json list with information attached to each item) of the blobs stored in the external odb. Though I think it would be more complex to edit/parse this list than to deal with many refs in refs/odbs//.
Re: [RFC/PATCH v3 00/16] Add initial experimental external ODB support
On Sat, Dec 3, 2016 at 7:47 PM, Lars Schneider wrote: > >> On 30 Nov 2016, at 22:04, Christian Couder >> wrote: >> >> Goal >> >> >> Git can store its objects only in the form of loose objects in >> separate files or packed objects in a pack file. >> >> To be able to better handle some kind of objects, for example big >> blobs, it would be nice if Git could store its objects in other object >> databases (ODB). > > This is a great goal. I really hope we can use that to solve the > pain points in the current Git <--> GitLFS integration! Yeah, I hope it will help too. > Thanks for working on this! > > Minor nit: I feel the term "other" could be more expressive. Plus > "database" might confuse people. What do you think about > "External Object Storage" or something? In the current Git code, "DB" is already used a lot. For example in cache.h there is: #define DB_ENVIRONMENT "GIT_OBJECT_DIRECTORY" #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES" #define INIT_DB_QUIET 0x0001 #define INIT_DB_EXIST_OK 0x0002 extern int init_db(const char *git_dir, const char *real_git_dir, const char *template_dir, unsigned int flags); [...] >> - " get ": the command should then read from the >> external ODB the content of the object corresponding to and >> output it on stdout. >> >> - " put ": the command should then read >> from stdin an object and store it in the external ODB. > > Based on my experience with Git clean/smudge filters I think this kind > of single shot protocol will be a performance bottleneck as soon as > people store more than >1000 files in the external ODB. > Maybe you can reuse my "filter process protocol" (edcc858) here? Yeah, I would like to do reuse your "filter process protocol" as much as possible to improve this in the future. >> * Transfer >> >> To tranfer information about the blobs stored in external ODB, some >> special refs, called "odb ref", similar as replace refs, are used. >> >> For now there should be one odb ref per blob. Each ref name should be >> refs/odbs// where is the sha1 of the blob stored >> in the external odb named . >> >> These odb refs should all point to a blob that should be stored in the >> Git repository and contain information about the blob stored in the >> external odb. This information can be specific to the external odb. >> The repos can then share this information using commands like: >> >> `git fetch origin "refs/odbs//*:refs/odbs//*"` > > The "odbref" would point to a blob and the blob could contain anything, > right? E.g. it could contain an existing GitLFS pointer, right? > > version https://git-lfs.github.com/spec/v1 > oid sha256:4d7a214614ab2935c943f9e0ff69d22eadbb8f32b1258daaa5e2ca24d17e2393 > size 12345 Yes, but I think that the sha1 should be added. So yes, it could easily be made compatible with git LFS. >> Design discussion about performance >> ~~~ >> >> Yeah, it is not efficient to fork/exec a command to just read or write >> one object to or from the external ODB. Batch calls and/or using a >> daemon and/or RPC should be used instead to be able to store regular >> objects in an external ODB. But for now the external ODB would be all >> about really big files, where the cost of a fork+exec should not >> matter much. If we later want to extend usage of external ODBs, yeah >> we will probably need to design other mechanisms. > > I think we should leverage the learnings from GitLFS as much as possible. > My learnings are: > > (1) Fork/exec per object won't work. People have lots and lots of content > that is not suited for Git (e.g. integration test data, images, ...). I agree that it will not work for many people, but look at how git LFS evolved. It first started without a good solution for those people, and then you provided a much better solution to them. So I am a bit reluctant to work on a complex solution reusing your "filter process protocol" work right away. > (2) We need a good UI. I think it would be great if the average user would > not even need to know about ODB. Moving files explicitly with a "put" > command seems unpractical to me. GitLFS tracks files via filename and > that has a number of drawbacks, too. Do you see a way to define a > customizable metric such as "move all files to ODB X that are gzip > compressed larger than Y"? I think these should be defined in the config and attributes files. It could also be possible to implement a "want" command (in the same way as the "get", "put" and "have" commands) to ask the e-odb helper if it wants to store a specific blob. >> Future work >> ~~~ >> >> I think that the odb refs don't prevent a regular fetch or push from >> wanting to send the objects that are managed by an external odb. So I >> am interested in suggestions about this problem. I will take a look at >> previous discussions and how other mechanisms (shallow clone, bundle >> v3, ...) handle this. > > If the ODB config
Re: git add -p with new file
On Mon, Dec 12, 2016 at 09:31:03PM +0100, Stephan Beyer wrote: > I am also a "git add -p"-only user (except for new files and merges). > However, I usually keep a lot of untracked files in my repositories. > Files that I do not (git)ignore because I want to see them when I type > "git status". > > Hence, the imagination only that "git add -p" starts to ask me for each > untracked file feels like a lot of annoying "n" presses. I could imagine > that it is okay-ish when it asks about the untracked files *after* all > tracked paths have been processed (I guess this has been proposed > before), so that I can safely quit. Yeah, this is the "some people might be annoyed" that I mentioned originally. If your workflow leaves a lot of untracked files that you don't care about it, then I think you'd want this feature disabled entirely via a config option (or vice versa, that it would only be enabled by config option). > I am also not really sure what problem this feature is trying to solve. > If the "problem"(?) is that it should act more like "git add" instead of > "git add -u", for whatever reason, this may be fine (but the > configuration option is a must-have then). I think the problem is just that "add -p" does not give the whole story of what you might want to do before making a commit. > > I'd also probably add interactive.showUntracked to make the whole thing > > optional (but I think it would be OK to default it to on). > Hm, "interactive.showUntracked" is a confusing name because "git add -i" > (interactive) already handles untracked files. Sure, that was just meant for illustration. I agree there's probably a better name. -Peff
Re: [PATCH 6/6] rm: add absorb a submodules git dir before deletion
On Mon, Dec 12, 2016 at 7:28 PM, brian m. carlson wrote: > On Mon, Dec 12, 2016 at 05:40:55PM -0800, Stefan Beller wrote: >> When deleting a submodule we need to keep the actual git directory around, >> such that we do not lose local changes in there and at a later checkout >> of the submodule we don't need to clone it again. >> >> Implement `depopulate_submodule`, that migrates the git directory before >> deletion of a submodule and afterwards the equivalent of "rm -rf", which >> is already found in entry.c, so expose that and for clarity add a suffix >> "_or_dir" to it. > > I think you might have meant "_or_die" here. indeed, will fix in a reroll. Thanks for the review!
Re: [PATCH 0/6] git-rm absorbs submodule git directory before deletion
On Mon, Dec 12, 2016 at 11:28 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> The "checkout --recurse-submodules" series got too large to comfortably send >> it out for review, so I had to break it up into smaller series'; this is the >> first subseries, but it makes sense on its own. >> >> This series teaches git-rm to absorb the git directory of a submodule instead >> of failing and complaining about the git directory preventing deletion. >> >> It applies on origin/sb/submodule-embed-gitdir. > > Thanks. I probably should rename the topic again with s/embed/absorb/; I mostly renamed it in the hope to settle our dispute what embedding means. ;) So in case we want to further discuss on the name of the function, we should do that before doing actual work such as renaming. Note that sb/t3600-cleanup is part of this series now, (The first commit of that series is in patch 6/6 of this series, and patch 5 is the modernization effort.) Thanks, Stefan
Re: [PATCHv2 1/2] merge: Add '--continue' option as a synonym for 'git commit'
Chris Packham writes:
> +'git merge' --continue
>
> DESCRIPTION
> ---
> @@ -61,6 +62,9 @@ reconstruct the original (pre-merge) changes. Therefore:
> discouraged: while possible, it may leave you in a state that is hard to
> back out of in the case of a conflict.
>
> +The fourth syntax ("`git merge --continue`") can only be run after the
> +merge has resulted in conflicts.
OK. I can see that the code refuses if there is no MERGE_HEAD, so
"can only be run" is ensured correctly.
> 'git merge --continue' will take the
> +currently staged changes and complete the merge.
For Git-savvy folks, this may be sufficient to tell that they are
expected to resolve conflicts in the working tree and register the
resolusion by doing "git add" before running "git merge --continue",
but I wonder if that is clear enough for new readers.
The same comment applies to the option description below. I suspect
that it is better to remove the last sentence above, leaving "4th
one can be run only with MERGE_HEAD" here, and enhance the
explanation in the option description (see below).
> OPTIONS
> ---
> @@ -99,6 +103,12 @@ commit or stash your changes before running 'git merge'.
> 'git merge --abort' is equivalent to 'git reset --merge' when
> `MERGE_HEAD` is present.
>
> +--continue::
> + Take the currently staged changes and complete the merge.
> ++
> +'git merge --continue' is equivalent to 'git commit' when
> +`MERGE_HEAD` is present.
> +
These two sentences are even more technical and unfriendly to new
readers, I am afraid. How about giving a hint by referring to an
existing section, like this?
--continue::
After a "git merge" stops due to conflicts you can conclude
the merge by running "git merge --continue" (see "How to
resolve conflicts" section below).
> @@ -277,7 +287,8 @@ After seeing a conflict, you can do two things:
>
> * Resolve the conflicts. Git will mark the conflicts in
> the working tree. Edit the files into shape and
> - 'git add' them to the index. Use 'git commit' to seal the deal.
> + 'git add' them to the index. Use 'git merge --continue' to seal the
> + deal.
Why do we want to make it harder to discover "git commit" here?
I would understand:
... Use 'git commit' to conclude (you can also say 'git
merge --continue').
though. After all, we are merely introducing a synonym for those
who want to type more. There is no plan to deprecate the use of
'git commit', which is a perfectly reasonable way to conclude an
interrupted merge, that has worked well for us for the past 10 years
and still works.
> @@ -65,6 +66,7 @@ static int option_renormalize;
> static int verbosity;
> static int allow_rerere_auto;
> static int abort_current_merge;
> +static int continue_current_merge;
> static int allow_unrelated_histories;
> static int show_progress = -1;
> static int default_to_upstream = 1;
> @@ -223,6 +225,8 @@ static struct option builtin_merge_options[] = {
> OPT__VERBOSITY(&verbosity),
> OPT_BOOL(0, "abort", &abort_current_merge,
> N_("abort the current in-progress merge")),
> + OPT_BOOL(0, "continue", &continue_current_merge,
> + N_("continue the current in-progress merge")),
> OPT_BOOL(0, "allow-unrelated-histories", &allow_unrelated_histories,
>N_("allow merging unrelated histories")),
> OPT_SET_INT(0, "progress", &show_progress, N_("force progress
> reporting"), 1),
> @@ -739,7 +743,7 @@ static void abort_commit(struct commit_list *remoteheads,
> const char *err_msg)
> if (err_msg)
> error("%s", err_msg);
> fprintf(stderr,
> - _("Not committing merge; use 'git commit' to complete the
> merge.\n"));
> + _("Not committing merge; use 'git merge --continue' to complete
> the merge.\n"));
Likewise. I do not see a need to change this one at all.
> @@ -1166,6 +1170,22 @@ int cmd_merge(int argc, const char **argv, const char
> *prefix)
> goto done;
> }
>
> + if (continue_current_merge) {
> + int nargc = 1;
> + const char *nargv[] = {"commit", NULL};
> +
> + if (argc)
> + usage_msg_opt("--continue expects no arguments",
> + builtin_merge_usage, builtin_merge_options);
Peff already commented on "what about other options?", and I think
his "check the number of args before parse-options ran to ensure
that the '--abort' or '--continue' was the only thing" is probably
a workable hack.
The "right" way to fix it would be way too involved to be worth for
just this single change (and also fixing "--abort"). Just thinking
aloud:
* Update parse-options API to:
- extend "struct option" with one field that holds what "command
modes" the option the "struct option" describes is incompatible
with.
- make parse_options() to keep track of the set of command modes
that are
RE: [PATCH 6/6] rm: add absorb a submodules git dir before deletion
> -Original Message-
> From: Stefan Beller [mailto:sbel...@google.com]
> Sent: Monday, December 12, 2016 8:41 PM
> To: gits...@pobox.com
> Cc: git@vger.kernel.org; David Turner; bmw...@google.com; Stefan Beller
> Subject: [PATCH 6/6] rm: add absorb a submodules git dir before deletion
>
> When deleting a submodule we need to keep the actual git directory around,
Nit: comma after submodule.
> - strbuf_reset(&buf);
> - strbuf_addstr(&buf, path);
> - if (!remove_dir_recursively(&buf, 0)) {
> - removed = 1;
> - if
> (!remove_path_from_gitmodules(path))
> - gitmodules_modified = 1;
> - strbuf_release(&buf);
> - continue;
> - } else if (!file_exists(path))
> - /* Submodule was removed by
> user */
> - if
> (!remove_path_from_gitmodules(path))
> - gitmodules_modified = 1;
> + if (file_exists(path))
> + depopulate_submodule(path);
> + removed = 1;
> + if (!remove_path_from_gitmodules(path))
> + gitmodules_modified = 1;
> + continue;
> /* Fallthrough and let remove_path()
> fail.
> */
It seems odd to have a continue right before a comment that says "Fallthrough".
Re: [PATCH 1/2] alternates: accept double-quoted paths
Duy Nguyen writes: > At least attr has the same problem and is going the same direction > [1]. Cool. (I actually thought the patch was in and evidence that this > kind of backward compatibility breaking was ok, turns out the patch > has stayed around for years) > > [1] > http://public-inbox.org/git/%3c20161110203428.30512-18-sbel...@google.com%3E/ Seeing that again, I see this in the proposed log message: Full pattern must be quoted. So 'pat"t"ern attr' will give exactly 'pat"t"ern', not 'pattern'. I cannot tell what it is trying to say. The code suggests something quite different, i.e. a line like this "pat\"t\"ern" attr would tell us that a path that matches the pattern pat"t"ern is given 'attr'. The log message may need to be cleaned up. The update by that commit to the documentation looks alright, thoguh.
Re: [PATCH 2/2] tmp-objdir: quote paths we add to alternates
Jeff King writes:
> The naive conversion is just:
> ...
> -# MINGW does not allow colons in pathnames in the first place
> -test_expect_success !MINGW 'push to repo path with colon' '
> +if test_have_prereq MINGW
> +then
> + path_sep=';'
> +else
> + path_sep=':'
> +fi
> ...
> - git clone --bare . xxx:yyy.git &&
> + git clone --bare . xxx${path_sep}yyy.git &&
Don't you want to dq the whole thing to prevent the shell from
splitting this into two commands at ';'? The other one below is OK.
>
> echo change >>file.bin &&
> git commit -am change &&
> # Note that we have to use the full path here, or it gets confused
> # with the ssh host:path syntax.
> - git push "$PWD/xxx:yyy.git" HEAD
> + git push "$PWD/xxx${path_sep}yyy.git" HEAD
> '
>
> test_done
>
> Does that work?
>
> -Peff
Re: [PATCH 0/2] handling alternates paths with colons
Jeff King writes: > On Mon, Dec 12, 2016 at 02:37:08PM -0800, Junio C Hamano wrote: > >> Jeff King writes: >> >> > So here are patches that do that. It kicks in only when the first >> > character of a path is a double-quote, and then expects the usual >> > C-style quoting. >> >> The quote being per delimited component is what makes this fifth >> approach a huge win. >> >> All sane components on a list-valued environment are still honored >> and an insane component that has either a colon in the middle or >> begins with a double-quote gets quoted. As long as nobody used a >> path that begins with a double-quote as an element in such a >> list-valued environment (and they cannot be, as using a non-absolute >> path as an element does not make much sense), this will be safe, and >> a path with a colon didn't work with Git unaware of the new quoting >> rule anyway. Nice. > > We do support non-absolute paths, both in alternates files and > environment variables. It's a nice feature if you want to have a > relocatable family of shared repositories. I'd imagine that most cases > start with "../", though. Yes. I was only talking about the environment variable, as you can tell from my mention of "colon" there.
Re: [PATCH 1/4] doc: add articles (grammar)
Kristoffer Haugsbakk writes: > Thank you for reviewing this series, Philip. > > Philip Oakley writes: > >> This looks good to me. > > I'll add this header: > > Acked-by: Philip Oakley > > To the commit message of this patch in the next review round (version 2 > of the patch series). > > Let me know if I should add another header, or do something different > than this. I was planning to merge all four from you as-is to 'next' today, though. Should I wait?
Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines
Vasco Almeida writes: >> We only update comment_line_char from the default "#" when the >> configured value is a single-byte character and we ignore incorrect >> values in the configuration file. So I think the patch you sent is >> correct after all. > > I am still not sure what version do we prefer. > > Check whether core.commentchar is a single character. If not, use '#' > as the $comment_line_char. This, plus special casing "auto". Picking the first byte is inconsistent with the current practice (the paragraph you quoted above), I think.
Re: [PATCH 2/2] tmp-objdir: quote paths we add to alternates
On Tue, Dec 13, 2016 at 10:10:04AM -0800, Junio C Hamano wrote:
> > - git clone --bare . xxx:yyy.git &&
> > + git clone --bare . xxx${path_sep}yyy.git &&
>
> Don't you want to dq the whole thing to prevent the shell from
> splitting this into two commands at ';'? The other one below is OK.
After expansion, I don't think the shell will do any further processing
except for whitespace splitting. E.g.:
$ debug() { for i in "$@"; do echo "got $i"; done; }
$ sep=';'
$ space=' '
$ debug foo${sep}bar
got foo;bar
$ debug foo${space}bar
got foo
got bar
I don't mind quoting it to make it more obvious, though.
-Peff
Re: [PATCH 0/2] handling alternates paths with colons
On Tue, Dec 13, 2016 at 10:10:54AM -0800, Junio C Hamano wrote: > > We do support non-absolute paths, both in alternates files and > > environment variables. It's a nice feature if you want to have a > > relocatable family of shared repositories. I'd imagine that most cases > > start with "../", though. > > Yes. I was only talking about the environment variable, as you can > tell from my mention of "colon" there. Right, but we also support relative paths via environment variables. I don't think that changes the conclusion though. I'm not convinced relative paths via the environment aren't slightly insane in the first place, given the discussion in 37a95862c (alternates: re-allow relative paths from environment, 2016-11-07). And they'd probably start with "../" as well. -Peff
Re: [PATCH 0/2] handling alternates paths with colons
Jeff King writes: > Right, but we also support relative paths via environment variables. I > don't think that changes the conclusion though. I'm not convinced > relative paths via the environment aren't slightly insane in the first > place, Sorry, a triple negation is above my head. "relative paths in env aren't insane" == "using relative paths is OK" and you are not convinced that it is the case, so you are saying that you think relative paths in env is not something that is sensible? > given the discussion in 37a95862c (alternates: re-allow relative > paths from environment, 2016-11-07). And they'd probably start with > "../" as well. Yeah. In any case, there is a potential regression but the chance is miniscule---the user has to have been using a relative path that begins with a double-quote in the environment or in alternates file.
Re: git add -p with new file
Jeff King writes: >> I am also not really sure what problem this feature is trying to solve. >> If the "problem"(?) is that it should act more like "git add" instead of >> "git add -u", for whatever reason, this may be fine (but the >> configuration option is a must-have then). > > I think the problem is just that "add -p" does not give the whole story > of what you might want to do before making a commit. The same is shared by "git diff [HEAD]", by the way. It is beyond me why people use "add -p", not "git diff [HEAD]", for the final sanity check before committing. Perhaps the latter is not advertised well enough? "add -p" does not even page so it is not very useful way to check what is being added if you are adding a new file (unless you are doing a toy example to add a 7-line file). >> > I'd also probably add interactive.showUntracked to make the whole thing >> > optional (but I think it would be OK to default it to on). >> Hm, "interactive.showUntracked" is a confusing name because "git add -i" >> (interactive) already handles untracked files. > > Sure, that was just meant for illustration. I agree there's probably a > better name. "interactive.*" is not a sensible hierarchy to use, because things other than "add" can go interactive. addPatch.showUntracked?
Re: [PATCH 0/2] handling alternates paths with colons
On Tue, Dec 13, 2016 at 10:42:39AM -0800, Junio C Hamano wrote: > Jeff King writes: > > > Right, but we also support relative paths via environment variables. I > > don't think that changes the conclusion though. I'm not convinced > > relative paths via the environment aren't slightly insane in the first > > place, > > Sorry, a triple negation is above my head. "relative paths in env > aren't insane" == "using relative paths is OK" and you are not > convinced that it is the case, so you are saying that you think > relative paths in env is not something that is sensible? I think relative paths in env have very flaky semantics which makes them inadvisable to use in general. That being said, when we broke even those flaky semantics somebody complained. So I consider a feature we _do_ support, but not one I would recommend to people. -Peff
Re: [PATCH 0/6] git-rm absorbs submodule git directory before deletion
Stefan Beller writes: > On Mon, Dec 12, 2016 at 11:28 PM, Junio C Hamano wrote: >> Stefan Beller writes: >> >>> The "checkout --recurse-submodules" series got too large to comfortably send >>> it out for review, so I had to break it up into smaller series'; this is the >>> first subseries, but it makes sense on its own. >>> >>> This series teaches git-rm to absorb the git directory of a submodule >>> instead >>> of failing and complaining about the git directory preventing deletion. >>> >>> It applies on origin/sb/submodule-embed-gitdir. >> >> Thanks. I probably should rename the topic again with s/embed/absorb/; > > I mostly renamed it in the hope to settle our dispute what embedding means. ;) I do not think there is no dispute about what embedding means. A submodule whose .git is inside its working tree has its repository embedded. What we had trouble settling on was what to call the operation to undo the embedding, unentangling its repository out of the working tree. I'd still vote for unembed if you want a name to be nominated. > Note that sb/t3600-cleanup is part of this series now, > (The first commit of that series is in patch 6/6 of this series, and patch 5 > is > the modernization effort.) Well, "t3600: remove useless redirect" has been in 'next' already; do you mean that you want me to queue this series on top of that topic?
Re: git add -p with new file
On Tue, Dec 13, 2016 at 10:48:07AM -0800, Junio C Hamano wrote: > > I think the problem is just that "add -p" does not give the whole story > > of what you might want to do before making a commit. > > The same is shared by "git diff [HEAD]", by the way. It is beyond > me why people use "add -p", not "git diff [HEAD]", for the final > sanity check before committing. > > Perhaps the latter is not advertised well enough? "add -p" does not > even page so it is not very useful way to check what is being added > if you are adding a new file (unless you are doing a toy example to > add a 7-line file). I use "add -p" routinely for my final add-and-sanity-check, and it is certainly not because I don't know about "git diff". I think it's just nice to break it into bite-size chunks and sort them into "yes, OK" or "no, not yet" bins. The lack of paging isn't usually a problem, because this "add -p" is useful precisely when you have a lot of little changes spread across the code base. I'd probably also run "git diff" if I wanted to look at something bigger. And I routinely use "git status", too, to see the full state of my tree. To me they are all tools in the toolbox, and I can pick the one that works best in any given situation, or that I just feel like using that day. > >> Hm, "interactive.showUntracked" is a confusing name because "git add -i" > >> (interactive) already handles untracked files. > > > > Sure, that was just meant for illustration. I agree there's probably a > > better name. > > "interactive.*" is not a sensible hierarchy to use, because things > other than "add" can go interactive. > > addPatch.showUntracked? Hmm, I wonder if interactive.diffFilter was mis-named then. The description is written in such a way as to cover other possible interactive commands showing a diff. It might be possible to do the same here: come up with a general option that _could_ cover new situations, but right now just applies here. Or maybe it would be too confusing. TBH, I wasn't all that concerned with the name yet. Whoever writes the patch can figure it out, and _then_ we can all bikeshed it. :) -Peff
Re: [PATCH] builtin/commit.c: convert trivial snprintf calls to xsnprintf
Jeff King writes:
>> +argv_array_pushf(&env, "GIT_INDEX_FILE=%s", index_file);
>> +if (launch_editor(git_path_commit_editmsg(), NULL, env.argv)) {
>> fprintf(stderr,
>> _("Please supply the message using either -m or -F
>> option.\n"));
>> +argv_array_clear(&env);
>> exit(1);
>> }
>> +argv_array_clear(&env);
>
> I'd generally skip the clear() right before exiting, though I saw
> somebody disagree with me recently on that. I wonder if we should
> decide as a project on whether it is worth doing or not.
I'd say it is a responsibility of the person who turns exit(1) to
return -1 to ensure the code does not leak.
>> @@ -1525,12 +1526,10 @@ static int git_commit_config(const char *k, const
>> char *v, void *cb)
>> static int run_rewrite_hook(const unsigned char *oldsha1,
>> const unsigned char *newsha1)
>> {
>> -/* oldsha1 SP newsha1 LF NUL */
>> -static char buf[2*40 + 3];
>> +char *buf;
>> struct child_process proc = CHILD_PROCESS_INIT;
>> const char *argv[3];
>> int code;
>> -size_t n;
>>
>> argv[0] = find_hook("post-rewrite");
>> if (!argv[0])
>> @@ -1546,34 +1545,33 @@ static int run_rewrite_hook(const unsigned char
>> *oldsha1,
>> code = start_command(&proc);
>> if (code)
>> return code;
>> -n = snprintf(buf, sizeof(buf), "%s %s\n",
>> - sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
>> +buf = xstrfmt("%s %s\n", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
>> sigchain_push(SIGPIPE, SIG_IGN);
>> -write_in_full(proc.in, buf, n);
>> +write_in_full(proc.in, buf, strlen(buf));
>> close(proc.in);
>> +free(buf);
>
> Any time you care about the length of the result, I'd generally use an
> actual strbuf instead of xstrfmt. The extra strlen isn't a big deal
> here, but it somehow seems simpler to me. It probably doesn't matter
> much either way, though.
Your justification for this extra allocation was that it is a
heavy-weight operation. While I agree that the runtime cost of
allocation and deallocation does not matter, I would be a bit
worried about extra cognitive burden to programmers. They did not
have to worry about leaking because they are writing a fixed length
string. Now they do, whether they use xstrfmt() or struct strbuf.
When they need to update what they write, they do have to remember
to adjust the size of the "fixed string", and the original is not
free from the "programmers' cognitive cost" point of view, of
course. Probably use of strbuf/xstrfmt is an overall win.
And of course, I think strbuf is more appropriate if you have to do
strlen().
Re: [PATCH 0/6] git-rm absorbs submodule git directory before deletion
On Tue, Dec 13, 2016 at 10:53 AM, Junio C Hamano wrote: > Stefan Beller writes: > >> On Mon, Dec 12, 2016 at 11:28 PM, Junio C Hamano wrote: >>> Stefan Beller writes: >>> The "checkout --recurse-submodules" series got too large to comfortably send it out for review, so I had to break it up into smaller series'; this is the first subseries, but it makes sense on its own. This series teaches git-rm to absorb the git directory of a submodule instead of failing and complaining about the git directory preventing deletion. It applies on origin/sb/submodule-embed-gitdir. >>> >>> Thanks. I probably should rename the topic again with s/embed/absorb/; >> >> I mostly renamed it in the hope to settle our dispute what embedding means. >> ;) > > I do not think there is no dispute about what embedding means. double negative: You think we have a slight dispute here. > A > submodule whose .git is inside its working tree has its repository > embedded. > > What we had trouble settling on was what to call the operation to > undo the embedding, unentangling its repository out of the working > tree. I'd still vote for unembed if you want a name to be nominated. So I can redo the series with two commands "git submodule [un]embed". For me "unembed" == "absorb", such that we could also go with absorb into superproject <-> embed into worktree > >> Note that sb/t3600-cleanup is part of this series now, >> (The first commit of that series is in patch 6/6 of this series, and patch 5 >> is >> the modernization effort.) > > Well, "t3600: remove useless redirect" has been in 'next' already; > do you mean that you want me to queue this series on top of that > topic? > I need to reroll this series any way; the reroll will be on top of that. Thanks, Stefan
[PATCH 3/2] t5547-push-quarantine: run the path separator test on Windows, too
To perform the test case on Windows in a way that corresponds to the
POSIX version, inject the semicolon in a directory name.
Typically, an absolute POSIX style path, such as the one in $PWD, is
translated into a Windows style path by bash when it invokes git.exe.
However, the presence of the semicolon suppresses this translation;
but the untranslated POSIX style path is useless for git.exe.
Therefore, instead of $PWD pass the Windows style path that $(pwd)
produces.
Signed-off-by: Johannes Sixt
---
Am 12.12.2016 um 20:53 schrieb Jeff King:
> Johannes, please let me know if I am wrong about skipping the test on
> !MINGW. The appropriate check there would be ";" anyway, but I am not
> sure _that_ is allowed in paths, either.
Here is a version for Windows. I'd prefer this patch on top instead
of squashing it into yours to keep the $PWD vs. $(pwd) explanation.
The result is the same as yours in all practical matters; but this
version I have already tested.
t/t5547-push-quarantine.sh | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh
index 6275ec807b..af9fcd833a 100755
--- a/t/t5547-push-quarantine.sh
+++ b/t/t5547-push-quarantine.sh
@@ -33,8 +33,7 @@ test_expect_success 'rejected objects are removed' '
test_cmp expect actual
'
-# MINGW does not allow colons in pathnames in the first place
-test_expect_success !MINGW 'push to repo path with colon' '
+test_expect_success 'push to repo path with path separator (colon)' '
# The interesting failure case here is when the
# receiving end cannot access its original object directory,
# so make it likely for us to generate a delta by having
@@ -43,13 +42,20 @@ test_expect_success !MINGW 'push to repo path with colon' '
test-genrandom foo 4096 >file.bin &&
git add file.bin &&
git commit -m bin &&
- git clone --bare . xxx:yyy.git &&
+
+ if test_have_prereq MINGW
+ then
+ pathsep=";"
+ else
+ pathsep=":"
+ fi &&
+ git clone --bare . "xxx${pathsep}yyy.git" &&
echo change >>file.bin &&
git commit -am change &&
# Note that we have to use the full path here, or it gets confused
# with the ssh host:path syntax.
- git push "$PWD/xxx:yyy.git" HEAD
+ git push "$(pwd)/xxx${pathsep}yyy.git" HEAD
'
test_done
--
2.11.0.55.g6a4dbb1
Re: [PATCH] builtin/commit.c: convert trivial snprintf calls to xsnprintf
On Tue, Dec 13, 2016 at 11:03:53AM -0800, Junio C Hamano wrote:
> >> @@ -1525,12 +1526,10 @@ static int git_commit_config(const char *k, const
> >> char *v, void *cb)
> >> static int run_rewrite_hook(const unsigned char *oldsha1,
> >>const unsigned char *newsha1)
> >> {
> >> - /* oldsha1 SP newsha1 LF NUL */
> >> - static char buf[2*40 + 3];
> >> + char *buf;
> >>struct child_process proc = CHILD_PROCESS_INIT;
> >>const char *argv[3];
> >>int code;
> >> - size_t n;
> >>
> >>argv[0] = find_hook("post-rewrite");
> >>if (!argv[0])
> >> @@ -1546,34 +1545,33 @@ static int run_rewrite_hook(const unsigned char
> >> *oldsha1,
> >>code = start_command(&proc);
> >>if (code)
> >>return code;
> >> - n = snprintf(buf, sizeof(buf), "%s %s\n",
> >> - sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
> >> + buf = xstrfmt("%s %s\n", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
> >>sigchain_push(SIGPIPE, SIG_IGN);
> >> - write_in_full(proc.in, buf, n);
> >> + write_in_full(proc.in, buf, strlen(buf));
> >>close(proc.in);
> >> + free(buf);
> >
> > Any time you care about the length of the result, I'd generally use an
> > actual strbuf instead of xstrfmt. The extra strlen isn't a big deal
> > here, but it somehow seems simpler to me. It probably doesn't matter
> > much either way, though.
>
> Your justification for this extra allocation was that it is a
> heavy-weight operation. While I agree that the runtime cost of
> allocation and deallocation does not matter, I would be a bit
> worried about extra cognitive burden to programmers. They did not
> have to worry about leaking because they are writing a fixed length
> string. Now they do, whether they use xstrfmt() or struct strbuf.
> When they need to update what they write, they do have to remember
> to adjust the size of the "fixed string", and the original is not
> free from the "programmers' cognitive cost" point of view, of
> course. Probably use of strbuf/xstrfmt is an overall win.
So I think you are agreeing, but I have a minor nit to pick. :)
The fact that the extra allocation will not hurt performance is
_necessary_, but not _sufficient_. So it's not a justification in
itself, only something we have to check before proceeding.
The only justification here is that magic numbers like "2*40 + 3" are
confusing and a potential maintenance burden. And that's why I suggested
splitting this one out from the other two (whose justification is
"PATH_MAX is sometimes too small").
I agree with you that it's a tradeoff between "magic numbers" versus
"having to free resources". In my opinion it's a net improvement, but I
think it would also be reasonable to switch to xsnprintf() here. Then
the programmer has an automatic check that the buffer size is
sufficient.
-Peff
Re: [PATCH 0/6] git-rm absorbs submodule git directory before deletion
Stefan Beller writes: >> I do not think there is no dispute about what embedding means. > > double negative: You think we have a slight dispute here. Sorry, I do not think there is any dispute on that. >> A >> submodule whose .git is inside its working tree has its repository >> embedded. >> >> What we had trouble settling on was what to call the operation to >> undo the embedding, unentangling its repository out of the working >> tree. I'd still vote for unembed if you want a name to be nominated. > > So I can redo the series with two commands "git submodule [un]embed". > > For me "unembed" == "absorb", such that we could also go with > absorb into superproject <-> embed into worktree With us agreeing that "embed" is about something is _IN_ submodule working tree, unembed would naturally be something becomes OUTSIDE the same thing (i.e. "submodule working tree"). However, if you introduce "absorb", we suddenly need to talk about a different thing, i.e. "superproject's .git/modules", that is doing the absorption. That is why I suggest "unembed" over "absorb".
Re: git add -p with new file
Jeff King writes: >> Perhaps the latter is not advertised well enough? "add -p" does not >> even page so it is not very useful way to check what is being added >> if you are adding a new file (unless you are doing a toy example to >> add a 7-line file). > > I use "add -p" routinely for my final add-and-sanity-check,... > ... To me they are all tools in the toolbox, and I can pick the one that > works best in any given situation, or that I just feel like using that > day. Oh, there is no question about that. I was just pointing out that "add -p" is not the "one that works best" when dealing with a path that is not yet even in the index.
Re: [PATCH 3/2] t5547-push-quarantine: run the path separator test on Windows, too
On Tue, Dec 13, 2016 at 08:09:31PM +0100, Johannes Sixt wrote: > Am 12.12.2016 um 20:53 schrieb Jeff King: > > Johannes, please let me know if I am wrong about skipping the test on > > !MINGW. The appropriate check there would be ";" anyway, but I am not > > sure _that_ is allowed in paths, either. > > Here is a version for Windows. I'd prefer this patch on top instead > of squashing it into yours to keep the $PWD vs. $(pwd) explanation. > > The result is the same as yours in all practical matters; but this > version I have already tested. Yeah, I'm happy to have this on top. The patch itself looks obviously correct. Thanks! -Peff
Re: What's cooking in git.git (Dec 2016, #02; Mon, 12)
Hi Junio, On Mon, 12 Dec 2016, Junio C Hamano wrote: > * bw/grep-recurse-submodules (2016-11-22) 6 commits > - grep: search history of moved submodules > - grep: enable recurse-submodules to work on objects > - grep: optionally recurse into submodules > - grep: add submodules as a grep source type > - submodules: load gitmodules file from commit sha1 > - submodules: add helper functions to determine presence of submodules > > "git grep" learns to optionally recurse into submodules > > Has anybody else seen t7814 being flakey with this series? It is not flakey for me, it fails consistently on Windows. This is the output with -i -v -x (sorry, I won't have time this week to do anything about it, but maybe it helps identify the root cause): -- snipsnap -- Initialized empty Git repository in C:/git-sdk-64/usr/src/git/wip3/t/trash directory.t7814-grep-recurse-submodules/.git/ expecting success: echo "foobar" >a && mkdir b && echo "bar" >b/b && git add a b && git commit -m "add a and b" && git init submodule && echo "foobar" >submodule/a && git -C submodule add a && git -C submodule commit -m "add a" && git submodule add ./submodule && git commit -m "added submodule" ++ echo foobar ++ mkdir b ++ echo bar ++ git add a b ++ git commit -m 'add a and b' [master (root-commit) 6a17548] add a and b Author: A U Thor 2 files changed, 2 insertions(+) create mode 100644 a create mode 100644 b/b ++ git init submodule Initialized empty Git repository in C:/git-sdk-64/usr/src/git/wip3/t/trash directory.t7814-grep-recurse-submodules/submodule/.git/ ++ echo foobar ++ git -C submodule add a ++ git -C submodule commit -m 'add a' [master (root-commit) 081a998] add a Author: A U Thor 1 file changed, 1 insertion(+) create mode 100644 a ++ git submodule add ./submodule Adding existing repo at 'submodule' to the index ++ git commit -m 'added submodule' [master 0c0fdd0] added submodule Author: A U Thor 2 files changed, 4 insertions(+) create mode 100644 .gitmodules create mode 16 submodule + test_eval_ret_=0 + want_trace + test t = t + test t = t + set +x ok 1 - setup directory structure and submodule expecting success: cat >expect <<-\EOF && a:foobar b/b:bar submodule/a:foobar EOF git grep -e "bar" --recurse-submodules >actual && test_cmp expect actual ++ cat ++ git grep -e bar --recurse-submodules ++ test_cmp expect actual ++ mingw_test_cmp expect actual ++ local test_cmp_a= test_cmp_b= ++ local stdin_for_diff= ++ test -s expect ++ test -s actual ++ mingw_read_file_strip_cr_ test_cmp_a ++ local line ++ : ++ IFS=$'\r' ++ read -r -d ' ' line ++ line='a:foobar ' ++ eval 'test_cmp_a=$test_cmp_a$line' +++ test_cmp_a='a:foobar ' ++ : ++ IFS=$'\r' ++ read -r -d ' ' line ++ line='b/b:bar ' ++ eval 'test_cmp_a=$test_cmp_a$line' +++ test_cmp_a='a:foobar b/b:bar ' ++ : ++ IFS=$'\r' ++ read -r -d ' ' line ++ line='submodule/a:foobar ' ++ eval 'test_cmp_a=$test_cmp_a$line' +++ test_cmp_a='a:foobar b/b:bar submodule/a:foobar ' ++ : ++ IFS=$'\r' ++ read -r -d ' ' line ++ test -z '' ++ break ++ mingw_read_file_strip_cr_ test_cmp_b ++ local line ++ : ++ IFS=$'\r' ++ read -r -d ' ' line ++ line='a:foobar ' ++ eval 'test_cmp_b=$test_cmp_b$line' +++ test_cmp_b='a:foobar ' ++ : ++ IFS=$'\r' ++ read -r -d ' ' line ++ line='b/b:bar ' ++ eval 'test_cmp_b=$test_cmp_b$line' +++ test_cmp_b='a:foobar b/b:bar ' ++ : ++ IFS=$'\r' ++ read -r -d ' ' line ++ line='submodule/a:foobar ' ++ eval 'test_cmp_b=$test_cmp_b$line' +++ test_cmp_b='a:foobar b/b:bar submodule/a:foobar ' ++ : ++ IFS=$'\r' ++ read -r -d ' ' line ++ test -z '' ++ break ++ test -n 'a:foobar b/b:bar submodule/a:foobar ' ++ test -n 'a:foobar b/b:bar submodule/a:foobar ' ++ test 'a:foobar b/b:bar submodule/a:foobar ' = 'a:foobar b/b:bar submodule/a:foobar ' + test_eval_ret_=0 + want_trace + test t = t + test t = t + set +x ok 2 - grep correctly finds patterns in a submodule expecting success: cat >expect <<-\EOF && submodule/a:foobar EOF git grep -e. --recurse-submodules -- submodule >actual && test_cmp expect actual ++ cat ++ git grep -e. --recurse-submodules -- submodule ++ test_cmp expect actual ++ mingw_test_cmp expect actual ++ local test_cmp_a= test_cmp_b= ++ local stdin_for_diff= ++ test -s expect ++ test -s actual ++ mingw_read_file_strip_cr_ test_cmp_a ++ local line ++ : ++ IFS=$'\r' ++ read -r -d ' ' line ++ line='submodule/a:foobar ' ++ eval 'test_cmp_a=$test_cmp_a$line' +++ test_cmp_a='submodule/a:foobar ' ++ : ++ IFS=$'\r' ++ read -r -d ' ' line ++ test -z '' ++ break ++ mingw_read_file_strip_cr_ test_cmp_b ++ local line ++ : ++ IFS=$'\r' ++ read -r -d ' ' line ++ line='submodule/a:foobar ' ++ eval 'test_cmp_b=$test_cmp_b$line' +++ test_cmp_b='submodule/a:foobar ' ++ : ++ IFS=$'\r' ++ read -r -d ' ' line ++ test -z '' ++ break ++ test -n 'submodule/a:foobar ' ++ test
Re: [PATCH 3/2] t5547-push-quarantine: run the path separator test on Windows, too
Johannes Sixt writes:
> To perform the test case on Windows in a way that corresponds to the
> POSIX version, inject the semicolon in a directory name.
>
> Typically, an absolute POSIX style path, such as the one in $PWD, is
> translated into a Windows style path by bash when it invokes git.exe.
> However, the presence of the semicolon suppresses this translation;
> but the untranslated POSIX style path is useless for git.exe.
> Therefore, instead of $PWD pass the Windows style path that $(pwd)
> produces.
>
> Signed-off-by: Johannes Sixt
> ---
> Am 12.12.2016 um 20:53 schrieb Jeff King:
>> Johannes, please let me know if I am wrong about skipping the test on
>> !MINGW. The appropriate check there would be ";" anyway, but I am not
>> sure _that_ is allowed in paths, either.
>
> Here is a version for Windows. I'd prefer this patch on top instead
> of squashing it into yours to keep the $PWD vs. $(pwd) explanation.
>
> The result is the same as yours in all practical matters; but this
> version I have already tested.
Will queue (I would wait for peff@ to say "OK", but I suspect he
would be OK in this case).
Thanks.
> t/t5547-push-quarantine.sh | 14 ++
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh
> index 6275ec807b..af9fcd833a 100755
> --- a/t/t5547-push-quarantine.sh
> +++ b/t/t5547-push-quarantine.sh
> @@ -33,8 +33,7 @@ test_expect_success 'rejected objects are removed' '
> test_cmp expect actual
> '
>
> -# MINGW does not allow colons in pathnames in the first place
> -test_expect_success !MINGW 'push to repo path with colon' '
> +test_expect_success 'push to repo path with path separator (colon)' '
> # The interesting failure case here is when the
> # receiving end cannot access its original object directory,
> # so make it likely for us to generate a delta by having
> @@ -43,13 +42,20 @@ test_expect_success !MINGW 'push to repo path with colon'
> '
> test-genrandom foo 4096 >file.bin &&
> git add file.bin &&
> git commit -m bin &&
> - git clone --bare . xxx:yyy.git &&
> +
> + if test_have_prereq MINGW
> + then
> + pathsep=";"
> + else
> + pathsep=":"
> + fi &&
> + git clone --bare . "xxx${pathsep}yyy.git" &&
>
> echo change >>file.bin &&
> git commit -am change &&
> # Note that we have to use the full path here, or it gets confused
> # with the ssh host:path syntax.
> - git push "$PWD/xxx:yyy.git" HEAD
> + git push "$(pwd)/xxx${pathsep}yyy.git" HEAD
> '
>
> test_done
Re: What's cooking in git.git (Dec 2016, #02; Mon, 12)
Johannes Schindelin writes:
> ok 13 - grep tree and more pathspecs
>
> expecting success:
> git init parent &&
> test_when_finished "rm -rf parent" &&
> echo "foobar" >"parent/fi:le" &&
> git -C parent add "fi:le" &&
> git -C parent commit -m "add fi:le" &&
> ...
> test_cmp expect actual
>
> ++ git init parent
> Initialized empty Git repository in C:/git-sdk-64/usr/src/git/wip3/t/trash
> directory.t7814-grep-recurse-submodules/parent/.git/
> ++ test_when_finished 'rm -rf parent'
> ++ test 0 = 0
> ++ test_cleanup='{ rm -rf parent
> } && (exit "$eval_ret"); eval_ret=$?; :'
> ++ echo foobar
> ++ git -C parent add fi:le
> fatal: pathspec 'fi:le' did not match any files
I think !MINGW prereq is missing?
git add -p with unmerged files (was: git add -p with new file)
Hi, While we're on the topic that "git add -p" should behave like the "normal" "git add" (not "git add -u"): what about unmerged changes? When I have merge conflicts, I almost always use my aliases "edit-unmerged" and "add-unmerged": $ git config --global --list | grep unmerged alias.list-unmerged=diff --name-only --diff-filter=U alias.edit-unmerged=!vim `git list-unmerged` alias.add-unmerged=!git add `git list-unmerged` alias.reset-unmerged=!uf=`git list-unmerged`; git reset HEAD $uf; git checkout -- $uf The "add-unmerged" alias is always a little scary because I'd rather like to check the changes with the "git add -p" workflow I am used to. Opinions? Best Stephan
Re: [PATCH 0/6] git-rm absorbs submodule git directory before deletion
On Tue, Dec 13, 2016 at 11:13 AM, Stefan Beller wrote: > On Tue, Dec 13, 2016 at 11:11 AM, Junio C Hamano wrote: >> Stefan Beller writes: >> I do not think there is no dispute about what embedding means. >>> >>> double negative: You think we have a slight dispute here. >> >> Sorry, I do not think there is any dispute on that. >> A submodule whose .git is inside its working tree has its repository embedded. What we had trouble settling on was what to call the operation to undo the embedding, unentangling its repository out of the working tree. I'd still vote for unembed if you want a name to be nominated. >>> >>> So I can redo the series with two commands "git submodule [un]embed". >>> >>> For me "unembed" == "absorb", such that we could also go with >>> absorb into superproject <-> embed into worktree >> >> With us agreeing that "embed" is about something is _IN_ submodule >> working tree, unembed would naturally be something becomes OUTSIDE >> the same thing (i.e. "submodule working tree"). I do not agree, yet. So I thought about this for a while. The standard in Git is to have the .git directory inside the working tree, which is why you are convinced that embedded means the .git is in the working tree, because you approach this discussion as the Git maintainer, spending only little time on submodule related stuff. The desired standard for submodules is to have the git dir inside the superprojects git dir (since 501770e, Aug 2011, Move git-dir for submodules), which is why I think an "embedded submodule git dir" is inside the superproject already. I think both views are legit, and we would want to choose the one that users find most intuitive (and I think there will be users that find either viewpoint intuitive). So when you have typed "git submodule ", I wonder if a user would assume a submodule-centric mindset of how submodules ought to work or if they still look at a submodule as its own git repo that just happens to be embedded into the superproject. I guess the latter is the case, so embedding is actually inside the working tree and un-embedding is the relocation to the superproject.
Re: [PATCH v2 00/34] Teach the sequencer to act as rebase -i's backend
Johannes Schindelin writes: > This marks the count down to '3': two more patch series after this > (really tiny ones) and we have a faster rebase -i. Nice. > Apart from mostly cosmetic patches (and the occasional odd bug that I > fixed promptly), I used these patches since mid May to perform all of my > interactive rebases. In mid June, I had the idea to teach rebase -i to > run *both* scripted rebase and rebase--helper and to cross-validate the > results. This slowed down all my interactive rebases since, but helped > me catch three rather obscure bugs (e.g. that git commit --fixup unfolds > long onelines and rebase -i still finds the correct original commit). > ... > Please note that the interdiff vs v1 is only of limited use: too many > things changed in the meantime, in particular the prepare-sequencer > branch that went through a couple of iterations before it found its way > into git.git's master branch. So please take the interdiff with a > mountain range of salt. > ... > Changes since v1: > ... > - removed the beautiful ordinal logic (to print out "1st", "2nd", "3rd" > etc) and made things consistent with the current `rebase -i`. It was removed because it was too Anglo-centric and unusable in i18n context, no? Judging from the list above, interdiff are pretty much all cosmetic and that is why you say it is only of limited use, I guess. ... goes and reads the remainder and finds that these were ... all minor changes, mostly cosmetic, with a helper function ... refactored out or two and things of that nature. It is actually a good thing. We do not want to see it deviate too drastically from what you have been testing for some months. Thanks.
Re: [PATCH 0/6] git-rm absorbs submodule git directory before deletion
On Tue, Dec 13, 2016 at 11:11 AM, Junio C Hamano wrote: > Stefan Beller writes: > >>> I do not think there is no dispute about what embedding means. >> >> double negative: You think we have a slight dispute here. > > Sorry, I do not think there is any dispute on that. > >>> A >>> submodule whose .git is inside its working tree has its repository >>> embedded. >>> >>> What we had trouble settling on was what to call the operation to >>> undo the embedding, unentangling its repository out of the working >>> tree. I'd still vote for unembed if you want a name to be nominated. >> >> So I can redo the series with two commands "git submodule [un]embed". >> >> For me "unembed" == "absorb", such that we could also go with >> absorb into superproject <-> embed into worktree > > With us agreeing that "embed" is about something is _IN_ submodule > working tree, unembed would naturally be something becomes OUTSIDE > the same thing (i.e. "submodule working tree"). However, if you > introduce "absorb", we suddenly need to talk about a different > thing, i.e. "superproject's .git/modules", that is doing the > absorption. That is why I suggest "unembed" over "absorb". ok, I will take unembed then. We could also go with more command line options such as "embed --reverse" or such, but that is not as nice I'd think.
Re: [PATCH 0/6] git-rm absorbs submodule git directory before deletion
Stefan Beller writes: > The desired standard for submodules is to have the git dir inside the > superprojects git dir (since 501770e, Aug 2011, Move git-dir for > submodules), which is why I think an "embedded submodule git dir" > is inside the superproject already. Think how you start a new submodule. What are the steps you take before you say "git submodule add"? And where does .git for the submodule live at that point? With the current system, you as the submodule originator need to do something different to make your working tree of the superproject match what the others who clone from your public repository. And comparing the two layout, the one originally held by the submodule originator has .git embedded in the working tree, no? All of the above is coming from "submodule" centric mindset. It just is not centric to those who follow what others originated.
Re: git add -p with unmerged files (was: git add -p with new file)
On Tue, Dec 13, 2016 at 08:21:59PM +0100, Stephan Beyer wrote:
> While we're on the topic that "git add -p" should behave like the
> "normal" "git add" (not "git add -u"): what about unmerged changes?
I agree that's a related part of the workflow, though the implementation
is a bit harder.
> When I have merge conflicts, I almost always use my aliases
> "edit-unmerged" and "add-unmerged":
>
> $ git config --global --list | grep unmerged
> alias.list-unmerged=diff --name-only --diff-filter=U
> alias.edit-unmerged=!vim `git list-unmerged`
You might like contrib/git-jump for that, which makes it easier to go to
the specific spots within files.
When "git jump merge" produces no hits, I know I've dealt with all of
the conflicts (textual ones, anyway). I do often want to run "git add
-p" then to review the changes before staging.
I think what is most helpful there is probably "git diff HEAD" to see
what the merge is bringing in (or the cherry-pick, or "am", or
whatever). If I wanted "add -p" to do anything, I think it would be to
act as if stage 2 ("ours", which should be the same as what is in HEAD)
was already in the index. I.e., show the diff against that, apply any
hunks we select, and store the whole thing as stage 0, losing the
unmerged bit.
When you select all hunks, this is equivalent to "git add unmerge-file".
If you choose only a subset of hunks, it leaves the unselected ones for
you to examine later via "git diff". And if you choose none, it should
probably leave unmerged.
That's just a scheme I thought up, though. I've never actually tried it
in practice.
-Peff
Re: [PATCH 0/6] git-rm absorbs submodule git directory before deletion
Stefan Beller writes: > I guess the latter is the case, so embedding is actually inside > the working tree and un-embedding is the relocation to the > superproject. Another reason why I personally see a .git in each submodule working tree is "embedded" has nothing to do with Git. It is an analogy I feel (perhaps it is just me) with the word "embedded reporters in warzone". These people are spread around, assigned to units to report from places closer to the front line and being closer to the leaf of the hierarchy, as opposed to be assigned to a more central place like HQ to do their reporting.
Re: git add -p with unmerged files
Stephan Beyer writes: > While we're on the topic that "git add -p" should behave like the > "normal" "git add" (not "git add -u"): what about unmerged changes? > > > When I have merge conflicts, I almost always use my aliases > "edit-unmerged" and "add-unmerged": > > $ git config --global --list | grep unmerged > alias.list-unmerged=diff --name-only --diff-filter=U > alias.edit-unmerged=!vim `git list-unmerged` > alias.add-unmerged=!git add `git list-unmerged` > alias.reset-unmerged=!uf=`git list-unmerged`; git reset HEAD $uf; git > checkout -- $uf > > The "add-unmerged" alias is always a little scary because I'd rather > like to check the changes with the "git add -p" workflow I am used to. > > Opinions? For this, you would NEVER want to use "add -p" to pick and choose. By definition, while you are in conflicted merge, the path that had conflicts before you started the merge-y operation (be it "pull", "am -3", or "cherry-pick") did not have any change since HEAD, and "pick this hunk, drop that hunk" cannot be correct for the conflict resolution. "git diff" while conflicted will highlight what conflicted by showing the three-way diff (similar to "diff --cc" on a merge result) and after conflict is resolved you can view "diff HEAD" on the path to see what the merge brought in.
Re: [RFC/PATCH v3 00/16] Add initial experimental external ODB support
Christian Couder writes: > In general I think that having a lot of refs is really a big problem > right now in Git as many big organizations using Git are facing this > problem in one form or another. > So I think that support for a big number of refs is a separate and > important problem that should and hopefully will be solved. But you do not have to make it worse. Is "refs" a good match for the problem you are solving? Or is it merely an expedient thing to use? I think it is the latter, judging by your mentioning RefTree. Whatever mechanism we choose, that will be carved into stone in users' repositories and you'd end up having to support it, and devise the migration path out of it if the initial selection is too problematic. That is why people (not just me) pointed out upfront that using refs for this purose would not scale.
Re: [PATCH 0/6] git-rm absorbs submodule git directory before deletion
On Tue, Dec 13, 2016 at 11:47 AM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> The desired standard for submodules is to have the git dir inside the
>> superprojects git dir (since 501770e, Aug 2011, Move git-dir for
>> submodules), which is why I think an "embedded submodule git dir"
>> is inside the superproject already.
>
> Think how you start a new submodule. What are the steps you take
> before you say "git submodule add"? And where does .git for the
> submodule live at that point?
Well there is no way to directly start a submodule
(e.g. "git submodule create"), such that there has to be one repo
that actually has the git dir inside the working tree.
If the submodule exists already somewhere, there are 2 ways to do it
("git submodule add " or "git clone && git submodule add")
which lead to different outcomes, where the .git resides.
> With the current system, you as the submodule originator need to do
> something different to make your working tree of the superproject
> match what the others who clone from your public repository.
>
> And comparing the two layout, the one originally held by the
> submodule originator has .git embedded in the working tree, no?
When starting a new submodule repo, yes, the git dir is inside
the working tree.
> All of the above is coming from "submodule" centric mindset. It
> just is not centric to those who follow what others originated.
ok.
> Another reason why I personally see a .git in each submodule working
> tree is "embedded" has nothing to do with Git. It is an analogy I
> feel (perhaps it is just me) with the word "embedded reporters in
> warzone". These people are spread around, assigned to units to
> report from places closer to the front line and being closer to the
> leaf of the hierarchy, as opposed to be assigned to a more central
> place like HQ to do their reporting.
I talked to people in the office and got a heavy rejection on the
the work "embedded" here for another reason:
"Does it put the submodule on an embedded device?
What does embedded even mean?
The end user is super confused"
So I think we should not use embed or un-embed one way or the other.
Instead we need to have another name.
I think absorb is ok-ish, as "git submodule absorb" hints that the
superproject absorbs something from the submodule.
So I will reroll it with "absorb" fixing some nits pointed out by David?
Re: [PATCH 2/2] tmp-objdir: quote paths we add to alternates
Jeff King writes:
> On Tue, Dec 13, 2016 at 10:10:04AM -0800, Junio C Hamano wrote:
>
>> > - git clone --bare . xxx:yyy.git &&
>> > + git clone --bare . xxx${path_sep}yyy.git &&
>>
>> Don't you want to dq the whole thing to prevent the shell from
>> splitting this into two commands at ';'? The other one below is OK.
>
> After expansion, I don't think the shell will do any further processing
> except for whitespace splitting. E.g.:
Ah, my mistake. Staring at too many `eval`s does it to me.
Thanks.
Re: [PATCH 0/6] git-rm absorbs submodule git directory before deletion
On Tue, Dec 13, 2016 at 12:09 PM, Stefan Beller wrote: > > So I will reroll it with "absorb" fixing some nits pointed out by David? I got confused there, Davids nits are for this series, the absorb series itself doesn't seem to have nits. So I'll just reroll this series on top of the currently sb/submodule-embed-gitdir (which you originally noted to be better renamed to submodule-absorb-gitdir) merged with t3600-cleanup. Thanks, Stefan
