Re: Is detecting endianness at compile-time unworkable?
Junio C Hamano wrote: > Well, having said all that, I do not think I personally mind if > ./configure learned to include a "compile small program and run it > to determine byte order on the build machine" as part of "we make a > reasonable effort" as long as it cleanly excludes cross building > case (and the result is made overridable just in case we misdetect > the "cross-ness" of the build). No need to run the program for cross-compiles, grepping a object file seems to work for autoconf: git clone https://git.sv.gnu.org/git/autoconf.git $PAGER autoconf/lib/autoconf/c.m4 # look for "BIGenDianSyS"
Re: [PATCH] t1404: increase core.packedRefsTimeout to avoid occasional test failure
On Wed, Aug 1, 2018 at 1:39 AM Jonathan Nieder wrote: > SZEDER Gábor wrote: > > > While 3secs timeout seems plenty, and indeed is sufficient in most > > cases, on rare occasions it's just not quite enough: I saw this test > > fail in Travis CI build jobs two, maybe three times because 'git > > update-ref' timed out. > > I suspect these tests will fail with valgrind (just because valgrind > makes things super slow). Would it be safe to use timeout=-1? I don't know. Travis CI has a time limit of about 45mins for the whole build job (including installing dependencies and compilation), and any sensible automated build system must have a similar time limit, so it would be fine to wait indefinitely in such an environment, though in case of a timeout we'd lose failure reports of failed tests, if there are any. OTOH, my (and I guess most devs') test runs don't have such a time limit, so I'm reluctant to change it to wait indefinitely. But then again, waiting indefinitely for a lock file is not all that different from messing up something and creating an endless loop or a deadlock...
Re: [PATCH v2 0/4] fix "rebase -i --root" corrupting root commit
Hi Eric, On Tue, Jul 31, 2018 at 12:33 AM, Eric Sunshine wrote: > This is a re-roll of [1] which fixes sequencer bugs resulting in commit > object corruption when "rebase -i --root" swaps in a new commit as root. > Unfortunately, those bugs made it into v2.18.0 and have already > corrupted at least one repository (a local project of mine). Patches 3/4 > and 4/4 are new. > > v1 fixed these bugs: > > * trailing garbage on the commit's "author" header > > * extra trailing digit on "author" header's timezone (caused by two > separate bugs) > > v2 fixes those same bugs, plus: > > * eliminates a bogus "@" prepended to the "author" header timestamp > which renders the header corrupt > > * takes care to validate information coming from > "rebase-merge/author-script" before incorporating it into the "author" > header since that file may be hand-edited, and bogus hand-edited > values could corrupt the commit object. > Does this also fix losing the initial commit if it is empty? Given git init ; git commit -m 'Initial commit' --allow-empty ; touch file.txt ; git add file.txt ; git commit -m 'Add file.txt' ; git rebase --root I would expect there to be 2 commits but the first one has disappeared. (This usually happens with "git rebase -i --root" early on in a new project.) Cheers, Hilco
Re: [PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted
On Wed, Aug 01, 2018 at 12:19:42AM +, brian m. carlson wrote: > On Tue, Jul 31, 2018 at 10:05:22PM +0200, Vojtech Myslivec wrote: > > Hello, > > > > me and my colleague are struggling with automation of verifying git > > repositories and we have encountered that git verify-commit and > > verify-tag accepts untrusted signatures and exit successfully. > > I don't have strong feelings on your change one way or the other, but > for automation it may be useful to use the --raw flag, which gives you > the raw gpg output and much greater control. For example, you can > require that a subkey is or is not used or require certain algorithms. > > I will say that most signatures are untrusted in my experience, so > unless people are using TOFU mode or making local signatures, git will > exit nonzero for most signatures. I think the current status is to exit > on a good signature, even if it isn't necessarily a valid signature. > > I'm interested to hear others' thoughts on this. I'd find it odd that we deviate from the gpg behavior, that returns 0 when verifyng an untrusted signatures. Tooling around gpg is generally difficult for this reason, but using the raw output should be enough to discard signatures with untrusted keys. Another alternative is to use a keyring with trusted keys *only* and disable fetching keys from hkp servers. This way signature verification should fail. Thanks, -Santiago. > -- > brian m. carlson: Houston, Texas, US > OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: git merge -s subtree seems to be broken.
Am 31.07.2018 um 17:50 schrieb Jeff King: > On Tue, Jul 31, 2018 at 11:03:17AM -0400, George Shammas wrote: > >> Bisecting around, this might be the commit that introduced the breakage. >> >> https://github.com/git/git/commit/d8febde >> >> I really hope that it hasn't been broken for 5 years and I am just doing >> something wrong. > > Unfortunately, I think it has been broken for five years. I don't remember this change at all. :-( Sorry for the trouble, everyone. I should feel ashamed, but I'm only staring in bewilderment. René
Re: git merge -s subtree seems to be broken.
Am 31.07.2018 um 23:06 schrieb Junio C Hamano: > Jeff King writes: > >> On Tue, Jul 31, 2018 at 01:23:04PM -0400, Jeff King wrote: >> ... >> So here it is fixed, and with a commit message. I'm not happy to omit a >> regression test, but I actually couldn't come up with a minimal one that >> tickled the problem, because we're playing around with heuristics. How about something like this? (squashable) --- t/t6029-merge-subtree.sh | 28 1 file changed, 28 insertions(+) diff --git a/t/t6029-merge-subtree.sh b/t/t6029-merge-subtree.sh index 3e692454a7..474a850de6 100755 --- a/t/t6029-merge-subtree.sh +++ b/t/t6029-merge-subtree.sh @@ -29,6 +29,34 @@ test_expect_success 'subtree available and works like recursive' ' ' +test_expect_success 'setup branch sub' ' + git checkout --orphan sub && + git rm -rf . && + test_commit foo +' + +test_expect_success 'setup branch main' ' + git checkout -b main master && + git merge -s ours --no-commit --allow-unrelated-histories sub && + git read-tree --prefix=dir/ -u sub && + git commit -m "initial merge of sub into main" && + test_path_is_file dir/foo.t && + test_path_is_file hello +' + +test_expect_success 'update branch sub' ' + git checkout sub && + test_commit bar +' + +test_expect_success 'update branch main' ' + git checkout main && + git merge -s subtree sub -m "second merge of sub into main" && + test_path_is_file dir/bar.t && + test_path_is_file dir/foo.t && + test_path_is_file hello +' + test_expect_success 'setup' ' mkdir git-gui && cd git-gui && -- 2.18.0
Re: [PATCH] remote: prefer exact matches when using refspecs
Jonathan Tan writes:
> This looks good to me. I've checked that refname_match (and
> branch_merge_matches(), which returns the result of refname_match()
> directly) is only used in "if" contexts, so making it return a value
> other than 1 is fine.
Yes, the log message should say that existing callers only care if
the returned value is 0 or not (i.e. if we have any match).
> I would initialize best_score to INT_MAX to avoid needing the
> "best_score < 0" comparison, but don't feel strongly about it.
If we want to lose that "have we seen any possible result?" check, I
think defining (ARRAY_SIZE(ref_rev_parse_rules) - p) as the score,
so that the "full path" gets score 6 (or whatever) and "some remote
tracking name" (like "origin->refs/remotes/origin/HEAD") gets score
of 1 (smallest but true) may make more sense. Then, start the best
score at 0 and every time we get a score strictly better than the
best so far, we overwrite the best. That way, we can even lose the
"did we get any positive score?" check, too, and making the
condition in the inner loop quite simple, i.e.
int best_score = 0;
...
for (ref = refs; ref; ref = ref->next) {
int score = refname_match(name, ref->name);
if (best_score < score) {
best_match = ref;
best_score = score;
}
}
We need a commit log message (hopefully we can lift most parts from
your patch in this thread) and a test update to ensure that the same
precedence order used as ref resolution (i.e. tags get higher prio
over branches, etc.) in addition to the test you had in your patch.
Thanks.
Re: [PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted
On Tue, Jul 31, 2018 at 10:05:22PM +0200, Vojtech Myslivec wrote: > Hello, > > me and my colleague are struggling with automation of verifying git > repositories and we have encountered that git verify-commit and > verify-tag accepts untrusted signatures and exit successfully. I don't have strong feelings on your change one way or the other, but for automation it may be useful to use the --raw flag, which gives you the raw gpg output and much greater control. For example, you can require that a subkey is or is not used or require certain algorithms. I will say that most signatures are untrusted in my experience, so unless people are using TOFU mode or making local signatures, git will exit nonzero for most signatures. I think the current status is to exit on a good signature, even if it isn't necessarily a valid signature. I'm interested to hear others' thoughts on this. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [GSoC] [PATCH v5 0/3] rebase: rewrite rebase in C
Hi Junio, During recent development, I found out that `v5` has some issues and shouldn't be merged into `next`. I implemented more options and ran a couple of regression tests from which I figured out that certain choices I made in those commits need to be reconsidered. During recent development, my working branch `wip-rebase` has passing `t3400` and for which I have made some changes to the code already in v5. Cheers, Pratik
Re: [PATCH] t1404: increase core.packedRefsTimeout to avoid occasional test failure
Hi, SZEDER Gábor wrote: > While 3secs timeout seems plenty, and indeed is sufficient in most > cases, on rare occasions it's just not quite enough: I saw this test > fail in Travis CI build jobs two, maybe three times because 'git > update-ref' timed out. I suspect these tests will fail with valgrind (just because valgrind makes things super slow). Would it be safe to use timeout=-1? Thanks, Jonathan
Re: [PATCH] remote: prefer exact matches when using refspecs
> That is, something like this, perhaps. The resulting behaviour
> should match how "git rev-parse X" would give precedence to tag X
> over branch X by going this route. What do you think?
[snip]
> static const struct ref *find_ref_by_name_abbrev(const struct ref *refs,
> const char *name)
> {
> const struct ref *ref;
> + const struct ref *best_match = NULL;
> + int best_score = -1;
> +
> for (ref = refs; ref; ref = ref->next) {
> - if (refname_match(name, ref->name))
> - return ref;
> + int score = refname_match(name, ref->name);
> +
> + if ((score && (best_score < 0 || score < best_score))) {
> + best_match = ref;
> + best_score = score;
> + }
> }
> - return NULL;
> + return best_match;
> }
This looks good to me. I've checked that refname_match (and
branch_merge_matches(), which returns the result of refname_match()
directly) is only used in "if" contexts, so making it return a value
other than 1 is fine.
I would initialize best_score to INT_MAX to avoid needing the
"best_score < 0" comparison, but don't feel strongly about it.
[PATCH] t1404: increase core.packedRefsTimeout to avoid occasional test failure
The test 'no bogus intermediate values during delete' in
't1404-update-ref-errors.sh', added in 6a2a7736d8 (t1404: demonstrate
two problems with reference transactions, 2017-09-08), tries to catch
undesirable side effects of deleting a ref, both loose and packed, in
a transaction. To do so it is holding the packed refs file locked
when it starts 'git update-ref -d' in the background with a 3secs
'core.packedRefsTimeout' value. After performing a few checks it is
then supposed to unlock the packed refs file before the background
'git update-ref's attempt to acquire the lock times out.
While 3secs timeout seems plenty, and indeed is sufficient in most
cases, on rare occasions it's just not quite enough: I saw this test
fail in Travis CI build jobs two, maybe three times because 'git
update-ref' timed out.
Increase that timeout by an order of magnitude to 30s to make such an
occasional failure even more improbable. This won't make the test run
any longer under normal circumstances, because 'git update-ref' will
acquire the lock and resume execution as soon as it can. And if it
turns out that even this increased timeout is still not enough, then
there are most likely bigger problems, e.g. the Travis CI build job
will exceed its time limit anyway, or the lockfile module is broken.
Signed-off-by: SZEDER Gábor
---
t/t1404-update-ref-errors.sh | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh
index 3a887b5113..372f0b1fbb 100755
--- a/t/t1404-update-ref-errors.sh
+++ b/t/t1404-update-ref-errors.sh
@@ -559,9 +559,9 @@ test_expect_success 'no bogus intermediate values during
delete' '
{
# Note: the following command is intentionally run in the
# background. We increase the timeout so that `update-ref`
- # attempts to acquire the `packed-refs` lock for longer than
- # it takes for us to do the check then delete it:
- git -c core.packedrefstimeout=3000 update-ref -d $prefix/foo &
+ # attempts to acquire the `packed-refs` lock for much longer
+ # than it takes for us to do the check then delete it:
+ git -c core.packedrefstimeout=3 update-ref -d $prefix/foo &
} &&
pid2=$! &&
# Give update-ref plenty of time to get to the point where it tries
--
2.18.0.408.g42635c01bc
Re: [PATCH] transport: report refs only if transport does
> What leaves me even more confused is that the entire log message > does not make it clear what the end-user observable problem the > patch is trying to solve. > > Is this "we sometimes follow and sometimes fail to follow refs while > fetching"? Does it affect all protocol versions and transports, or > only just selected few (and if so which ones)? Normally I would respond by creating a new patch with the answer in its commit message, but I'm now not sure about whether it's better to revert back to the non-"fetched_refs" API entirely (as I explained in the reply to Peff I just sent [1]), so I'll answer your questions here for now: - Yes. We fail to follow when we fetch at least one ref that is up-to-date and one ref that is not, and when we're using the "fetch" command in a remote helper (for example, HTTP protocol v0). - I haven't checked exhaustively, but as far as I know, affects HTTP protocol v0, and does not affect anything using connect or stateless-connect (e.g. HTTP protocol v2, ssh). When I create a new patch, I'll also include these answers in its commit message. [1] https://public-inbox.org/git/20180731232343.184463-1-jonathanta...@google.com/
Re: [PATCH] transport: report refs only if transport does
> On Mon, Jul 30, 2018 at 03:56:01PM -0700, Jonathan Tan wrote:
>
> > Commit 989b8c4452 ("fetch-pack: put shallow info in output parameter",
> > 2018-06-28) allows transports to report the refs that they have fetched
> > in a new out-parameter "fetched_refs". If they do so,
> > transport_fetch_refs() makes this information available to its caller.
> >
> > Because transport_fetch_refs() filters the refs sent to the transport,
> > it cannot just report the transport's result directly, but first needs
> > to readd the excluded refs, pretending that they are fetched. However,
> > this results in a wrong result if the transport did not report the refs
> > that they have fetched in "fetched_refs" - the excluded refs would be
> > added and reported, presenting an incomplete picture to the caller.
>
> This part leaves me confused. If we are not fetching them, then why do
> we need to pretend that they are fetched?
The short answer is that we need:
(1) the complete list of refs that was passed to
transport_fetch_refs(),
(2) with shallow information (REF_STATUS_REJECT_SHALLOW set if
relevant), and
(3) with updated OIDs if ref-in-want was used.
The fetched_refs out param already fulfils (2) and (3), and this patch
makes it fulfil (1). As for calling them fetched_refs, perhaps that is a
misnomer, but they do appear in FETCH_HEAD even though they are not
truly fetched.
Which raises the question...if completeness is so important, why not
reuse the input list of refs and document that transport_fetch_refs()
can mutate the input list? You ask the same question below, so I'll put
the answer after quoting your paragraph.
> I think I am showing my lack of understanding about the reason for this
> whole "return the fetched refs" scheme from 989b8c4452, and probably
> reading the rest of that series would make it more clear. But from the
> perspective of somebody digging into history and finding just this
> commit, it probably needs to lay out a little more of the reasoning.
I think it's because 989b8c4452 is based on my earlier work [1] which
also had a fetched_refs out param. Its main reason is to enable the
invoker of transport_fetch_refs() to specify ref patterns (as you can
see in a later commit in the same patch set [2]) - and if we specify
patterns, the invoker of transport_fetch_refs() needs the resulting refs
(which are provided through fetched_refs).
In the version that made it to master, however, there was some debate
about whether ref patterns need to be allowed. In the end, ref patterns
were not allowed [3], but the fetched_refs out param was still left in.
I think that reverting the API might work, but am on the fence about it.
It would reduce the number of questions about the code (and would
probably automatically fix the issue that I was fixing in the first
place), but if we were to revert the API and then decide that we do want
ref patterns in "want-ref" (or expand transport_fetch_refs in some
similar way), we would need to revert our revert, causing code churn.
[1]
https://public-inbox.org/git/86a128c5fb710a41791e7183207c4d64889f9307.1485381677.git.jonathanta...@google.com/
[2]
https://public-inbox.org/git/eef2b77d88df0db08e4a1505b06e0af2d40143d5.1485381677.git.jonathanta...@google.com/
[3] https://public-inbox.org/git/20180620213235.10952-1-bmw...@google.com/
[PATCH] travis-ci: include the trash directories of failed tests in the trace log
The trash directory of a failed test might contain invaluable
information about the cause of the failure, but we have no access to
the trash directories of Travis CI build jobs. The only feedback we
get from there is the build job's trace log, so...
Modify 'ci/print-test-failures.sh' to create a tar.gz archive of the
trash directory of each failed test, encode that archive with base64,
and print the resulting block of ASCII text, so it gets embedded in
the trace log. Furthermore, run tests with '--immediate' to
faithfully preserve the failed state.
Extracting the trash directories from the trace log turned out to be a
bit of a hassle, partly because of the size of these logs (usually
resulting in several hundreds or even thousands of lines of
base64-encoded text), and partly because these logs have CRLF, CRCRLF
and occasionally even CRCRCRLF line endings, which cause 'base64 -d'
from coreutils to complain about "invalid input". For convenience add
a small script 'ci/util/extract-trash-dirs.sh', which will extract and
unpack all base64-encoded trash directories embedded in the log fed to
its standard input, and include an example command to be copy-pasted
into a terminal to do it all at the end of the failure report.
A few of our tests create sizeable trash directories, so limit the
size of each included base64-encoded block, let's say, to 1MB. And
just in case something fundamental gets broken and a lot of tests fail
at once, don't include trash directories when the combined size of the
included base64-encoded blocks would exceed 1MB.
Signed-off-by: SZEDER Gábor
---
Notes:
This is an improved version of the PoC mentioned some months ago at:
https://public-inbox.org/git/20180122182717.21539-1-szeder@gmail.com/
I'm still not sure whether this is too clever or too ugly, or both,
but it did actually prove to be useful since then: it's very likely
that we wouldn't have 2f3cbcd8c5 (tests: make forging GPG signed
commits and tags more robust, 2018-06-04) without it.
The output looks like this; with this patch on top of yesterday's
'pu' (6f49f36eba), which happens to have multiple test failures:
https://travis-ci.org/szeder/git/jobs/410471312#L3010
(Note: the Travis CI webpage will likely fold up the failure output
before you could take a closer look at it; then click on the line
containing '$ ci/print-test-failures.sh' and scroll down for a while.)
ci/lib-travisci.sh| 2 +-
ci/print-test-failures.sh | 55 +--
ci/util/extract-trash-dirs.sh | 50 +++
3 files changed, 104 insertions(+), 3 deletions(-)
create mode 100755 ci/util/extract-trash-dirs.sh
diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
index ceecc889ca..06970f7213 100755
--- a/ci/lib-travisci.sh
+++ b/ci/lib-travisci.sh
@@ -97,7 +97,7 @@ fi
export DEVELOPER=1
export DEFAULT_TEST_TARGET=prove
export GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
-export GIT_TEST_OPTS="--verbose-log -x"
+export GIT_TEST_OPTS="--verbose-log -x --immediate"
export GIT_TEST_CLONE_2GB=YesPlease
if [ "$jobname" = linux-gcc ]; then
export CC=gcc-8
diff --git a/ci/print-test-failures.sh b/ci/print-test-failures.sh
index 4f261ddc01..d55460a212 100755
--- a/ci/print-test-failures.sh
+++ b/ci/print-test-failures.sh
@@ -8,13 +8,24 @@
# Tracing executed commands would produce too much noise in the loop below.
set +x
-if ! ls t/test-results/*.exit >/dev/null 2>/dev/null
+cd t/
+
+if ! ls test-results/*.exit >/dev/null 2>/dev/null
then
echo "Build job failed before the tests could have been run"
exit
fi
-for TEST_EXIT in t/test-results/*.exit
+case "$jobname" in
+osx-clang|osx-gcc)
+ # base64 in OSX doesn't wrap its output at 76 columns by
+ # default, but prints a single, very long line.
+ base64_opts="-b 76"
+ ;;
+esac
+
+combined_trash_size=0
+for TEST_EXIT in test-results/*.exit
do
if [ "$(cat "$TEST_EXIT")" != "0" ]
then
@@ -23,5 +34,45 @@ do
echo "$(tput setaf 1)${TEST_OUT}...$(tput sgr0)"
echo
""
cat "${TEST_OUT}"
+
+ test_name="${TEST_EXIT%.exit}"
+ test_name="${test_name##*/}"
+ trash_dir="trash directory.$test_name"
+ trash_tgz_b64="trash.$test_name.base64"
+ if [ -d "$trash_dir" ]
+ then
+ tar czp "$trash_dir" |base64 $base64_opts
>"$trash_tgz_b64"
+
+ trash_size=$(wc -c <"$trash_tgz_b64")
+ if [ $trash_size -gt 1048576 ]
+ then
+ # larger than 1MB
+ echo "$(tput setaf 1)Didn't include the trash
directory of '$test_name' in the trace log, it's too big$(tput sgr0)"
+
Re: [PATCH] remote: prefer exact matches when using refspecs
Junio C Hamano writes:
> In order to resolve this correctly with the precedence rules, I
> think you need to make refname_match() return the precedence number
> (e.g. give 1 to "%.*s", 2 to "refs/%.*s", etc., using the index in
> ref_rev_parse_rules[] array), and make this loop keep track of the
> "best" match paying attention to the returned precedence.
That is, something like this, perhaps. The resulting behaviour
should match how "git rev-parse X" would give precedence to tag X
over branch X by going this route. What do you think?
refs.c | 8 +++-
remote.c | 13 ++---
2 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/refs.c b/refs.c
index 457fb78057..fd1a7f7478 100644
--- a/refs.c
+++ b/refs.c
@@ -495,11 +495,9 @@ int refname_match(const char *abbrev_name, const char
*full_name)
const char **p;
const int abbrev_name_len = strlen(abbrev_name);
- for (p = ref_rev_parse_rules; *p; p++) {
- if (!strcmp(full_name, mkpath(*p, abbrev_name_len,
abbrev_name))) {
- return 1;
- }
- }
+ for (p = ref_rev_parse_rules; *p; p++)
+ if (!strcmp(full_name, mkpath(*p, abbrev_name_len,
abbrev_name)))
+ return (p - ref_rev_parse_rules) + 1;
return 0;
}
diff --git a/remote.c b/remote.c
index 86e6098774..ed2f80e45c 100644
--- a/remote.c
+++ b/remote.c
@@ -1689,11 +1689,18 @@ static struct ref *get_expanded_map(const struct ref
*remote_refs,
static const struct ref *find_ref_by_name_abbrev(const struct ref *refs, const
char *name)
{
const struct ref *ref;
+ const struct ref *best_match = NULL;
+ int best_score = -1;
+
for (ref = refs; ref; ref = ref->next) {
- if (refname_match(name, ref->name))
- return ref;
+ int score = refname_match(name, ref->name);
+
+ if ((score && (best_score < 0 || score < best_score))) {
+ best_match = ref;
+ best_score = score;
+ }
}
- return NULL;
+ return best_match;
}
struct ref *get_remote_ref(const struct ref *remote_refs, const char *name)
Re: [PATCH] remote: prefer exact matches when using refspecs
Jonathan Tan writes:
> When matching a non-wildcard LHS of a refspec against a list of refs,
> find_ref_by_name_abbrev() returns the first ref that matches using the
> DWIM rules used by refname_match() in refs.c, even if an exact match
> occurs later in the list of refs.
When you have refs/heads/refs/heads/s and refs/heads/s, and if you
ask for refs/heads/s, you want that exact match (i.e. the latter) to
take precedence over DWIMmed refs/heads/refs/heads/s.
What is unfortunate is that ref_rev_parse_rules[] array already
expresses that preference by listing the "fullname" choice "%.*s"
before other DWIM choices like "refs/heads/%.*s".
Now we iterate over refs we say from ls-remote output, and with the
updated one, the logic _manually_ gives the precedence to the first
entry in ref_rev_parse_rules[], so in that "I have a branch s and
also another branch refs/heads/s" case, that may happen to work, but
would the updated code do the right thing when you have entries that
can match, say, both second and third entry in the rules[] and
tiebreak correctly the same way? Say you ask for "tags/T" when
there are "refs/tags/T" and "refs/heads/tags/T" at the same time in
the refs linked list. None of the ref on refs list trigger
!strcmp() as there is no exact mqatch, and refname_match() would say
"Yeah I see a match" when checking "refs/heads/tags/T" and say it is
the best match. Then it finds "refs/tags/T" also on the refs list
and finds it also matches user-supplied "tags/T".
In order to resolve this correctly with the precedence rules, I
think you need to make refname_match() return the precedence number
(e.g. give 1 to "%.*s", 2 to "refs/%.*s", etc., using the index in
ref_rev_parse_rules[] array), and make this loop keep track of the
"best" match paying attention to the returned precedence.
> This causes unexpected behavior when (for example) fetching using the
> refspec "refs/heads/s:" from a remote with both
> "refs/heads/refs/heads/s" and "refs/heads/s". (Even if the former was
> inadvertently created, one would still expect the latter to be fetched.)
>
> This problem has only been observed when the desired ref comes after the
> undesired ref in alphabetical order. However, for completeness, the test
> in this patch also checks what happens when the desired ref comes first
> alphabetically.
>
> Signed-off-by: Jonathan Tan
> ---
> remote.c | 7 +--
> t/t5510-fetch.sh | 28
> 2 files changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/remote.c b/remote.c
> index 3fd43453f..eeffe3488 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1687,12 +1687,15 @@ static struct ref *get_expanded_map(const struct ref
> *remote_refs,
>
> static const struct ref *find_ref_by_name_abbrev(const struct ref *refs,
> const char *name)
> {
> + const struct ref *best_match = NULL;
> const struct ref *ref;
> for (ref = refs; ref; ref = ref->next) {
> - if (refname_match(name, ref->name))
> + if (!strcmp(name, ref->name))
> return ref;
> + if (refname_match(name, ref->name))
> + best_match = ref;
> }
> - return NULL;
> + return best_match;
> }
>
> struct ref *get_remote_ref(const struct ref *remote_refs, const char *name)
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index e402aee6a..da88f35f0 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -535,6 +535,34 @@ test_expect_success "should be able to fetch with
> duplicate refspecs" '
> )
> '
>
> +test_expect_success 'LHS of refspec prefers exact matches' '
> + mkdir lhs-exact &&
> + (
> + cd lhs-exact &&
> + git init server &&
> + test_commit -C server unwanted &&
> + test_commit -C server wanted &&
> +
> + git init client &&
> +
> + # Check a name coming after "refs" alphabetically ...
> + git -C server update-ref refs/heads/s wanted &&
> + git -C server update-ref refs/heads/refs/heads/s unwanted &&
> + git -C client fetch ../server refs/heads/s:refs/heads/checkthis
> &&
> + git -C server rev-parse wanted >expect &&
> + git -C client rev-parse checkthis >actual &&
> + test_cmp expect actual &&
> +
> + # ... and one before.
> + git -C server update-ref refs/heads/q wanted &&
> + git -C server update-ref refs/heads/refs/heads/q unwanted &&
> + git -C client fetch ../server refs/heads/q:refs/heads/checkthis
> &&
> + git -C server rev-parse wanted >expect &&
> + git -C client rev-parse checkthis >actual &&
> + test_cmp expect actual
> + )
> +'
> +
> # configured prune tests
>
> set_config_tristate () {
Re: [PATCH v2 2/2] sequencer: fix quoting in write_author_script
On Tue, Jul 31, 2018 at 7:15 AM Phillip Wood wrote:
> Single quotes should be escaped as \' not \\'. Note that this only
> affects authors that contain a single quote and then only external
> scripts that read the author script and users whose git is upgraded from
> the shell version of rebase -i while rebase was stopped. This is because
> the parsing in read_env_script() expected the broken version and for
> some reason sq_dequote() called by read_author_ident() seems to handle
> the broken quoting correctly.
Is the:
...for some reason sq_dequote() called by read_author_ident()
seems to handle the broken quoting correctly.
bit outdated? We know now from patch 2/4 of my series[1] that
read_author_ident() wasn't handling it correctly at all. It was merely
ignoring the return value from sq_dequote() and using whatever broken
value came back from it.
[1]:
https://public-inbox.org/git/20180731073331.40007-3-sunsh...@sunshineco.com/
> Helped-by: Johannes Schindelin
> Signed-off-by: Phillip Wood
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -664,14 +664,25 @@ static int write_author_script(const char *message)
> static int read_env_script(struct argv_array *env)
> {
> if (strbuf_read_file(, rebase_path_author_script(), 256) <= 0)
> return -1;
This is not a problem introduced by this patch, but since
strbuf_read_file() doesn't guarantee that memory hasn't been allocated
when it returns an error, this is leaking.
> + /*
> +* write_author_script() used to fail to terminate the GIT_AUTHOR_DATE
> +* line with a "'" and also escaped "'" incorrectly as "'''"
> rather
> +* than "'\\''". We check for the terminating "'" on the last line to
> +* see how "'" has been escaped in case git was upgraded while rebase
> +* was stopped.
> +*/
> + sq_bug = script.len && script.buf[script.len - 2] != '\'';
I think you need to be checking 'script.len > 1', not just
'script.len', otherwise you might access memory outside the allocated
buffer.
This is a very "delicate" check, assuming that a hand-edited file
won't end with, say, an extra newline. I wonder if this level of
backward-compatibility is overkill for such an unlikely case.
> for (p = script.buf; *p; p++)
> - if (skip_prefix(p, "'''", (const char **)))
> + if (sq_bug && skip_prefix(p, "'''", ))
> + strbuf_splice(, p - script.buf, p2 - p, "'",
> 1);
> + else if (skip_prefix(p, "'\\''", ))
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> @@ -75,6 +75,22 @@ test_expect_success 'rebase --keep-empty' '
> +test_expect_success 'rebase -i writes correct author-script' '
> + test_when_finished "test_might_fail git rebase --abort" &&
> + git checkout -b author-with-sq master &&
> + GIT_AUTHOR_NAME="Auth O$SQ R" git commit --allow-empty -m with-sq &&
> + set_fake_editor &&
> + FAKE_LINES="edit 1" git rebase -ki HEAD^ &&
Hmph, -k doesn't seem to be documented in git-rebase.txt. Is it needed here?
Re: [PATCH] transport: report refs only if transport does
Jeff King writes:
> On Mon, Jul 30, 2018 at 03:56:01PM -0700, Jonathan Tan wrote:
>
>> Commit 989b8c4452 ("fetch-pack: put shallow info in output parameter",
>> 2018-06-28) allows transports to report the refs that they have fetched
>> in a new out-parameter "fetched_refs". If they do so,
>> transport_fetch_refs() makes this information available to its caller.
>>
>> Because transport_fetch_refs() filters the refs sent to the transport,
>> it cannot just report the transport's result directly, but first needs
>> to readd the excluded refs, pretending that they are fetched. However,
>> this results in a wrong result if the transport did not report the refs
>> that they have fetched in "fetched_refs" - the excluded refs would be
>> added and reported, presenting an incomplete picture to the caller.
>
> This part leaves me confused. If we are not fetching them, then why do
> we need to pretend that they are fetched?
What leaves me even more confused is that the entire log message
does not make it clear what the end-user observable problem the
patch is trying to solve.
Is this "we sometimes follow and sometimes fail to follow refs while
fetching"? Does it affect all protocol versions and transports, or
only just selected few (and if so which ones)?
In minds of those who reported an issue and wrote the fix, the issue
may be fresh, but let's write the commit log message for ourselves 6
months down the road.
Thanks.
Re: [PATCH] remote: prefer exact matches when using refspecs
Hi,
Jonathan Tan wrote:
> When matching a non-wildcard LHS of a refspec against a list of refs,
> find_ref_by_name_abbrev() returns the first ref that matches using the
> DWIM rules used by refname_match() in refs.c, even if an exact match
> occurs later in the list of refs.
>
> This causes unexpected behavior when (for example) fetching using the
> refspec "refs/heads/s:" from a remote with both
> "refs/heads/refs/heads/s" and "refs/heads/s". (Even if the former was
> inadvertently created, one would still expect the latter to be fetched.)
>
> This problem has only been observed when the desired ref comes after the
> undesired ref in alphabetical order. However, for completeness, the test
> in this patch also checks what happens when the desired ref comes first
> alphabetically.
>
> Signed-off-by: Jonathan Tan
> ---
> remote.c | 7 +--
> t/t5510-fetch.sh | 28
> 2 files changed, 33 insertions(+), 2 deletions(-)
Very clear analysis --- thank you.
> --- a/remote.c
> +++ b/remote.c
> @@ -1687,12 +1687,15 @@ static struct ref *get_expanded_map(const struct ref
> *remote_refs,
>
> static const struct ref *find_ref_by_name_abbrev(const struct ref *refs,
> const char *name)
> {
> + const struct ref *best_match = NULL;
> const struct ref *ref;
> for (ref = refs; ref; ref = ref->next) {
> - if (refname_match(name, ref->name))
> + if (!strcmp(name, ref->name))
> return ref;
> + if (refname_match(name, ref->name))
Should this check be
if (!best_match && refname_match(name, ref->name))
? Otherwise, this would make us prefer the last ref instead of the
first (which probably doesn't matter but would be an unintended
behavior change).
> + best_match = ref;
> }
> - return NULL;
> + return best_match;
> }
>
> struct ref *get_remote_ref(const struct ref *remote_refs, const char *name)
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index e402aee6a..da88f35f0 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -535,6 +535,34 @@ test_expect_success "should be able to fetch with
> duplicate refspecs" '
> )
> '
>
> +test_expect_success 'LHS of refspec prefers exact matches' '
Nice.
With or without the suggested tweak,
Reviewed-by: Jonathan Nieder
Thanks for a pleasant read.
> + mkdir lhs-exact &&
> + (
> + cd lhs-exact &&
> + git init server &&
> + test_commit -C server unwanted &&
> + test_commit -C server wanted &&
> +
> + git init client &&
> +
> + # Check a name coming after "refs" alphabetically ...
> + git -C server update-ref refs/heads/s wanted &&
> + git -C server update-ref refs/heads/refs/heads/s unwanted &&
> + git -C client fetch ../server refs/heads/s:refs/heads/checkthis
> &&
> + git -C server rev-parse wanted >expect &&
> + git -C client rev-parse checkthis >actual &&
> + test_cmp expect actual &&
> +
> + # ... and one before.
> + git -C server update-ref refs/heads/q wanted &&
> + git -C server update-ref refs/heads/refs/heads/q unwanted &&
> + git -C client fetch ../server refs/heads/q:refs/heads/checkthis
> &&
> + git -C server rev-parse wanted >expect &&
> + git -C client rev-parse checkthis >actual &&
> + test_cmp expect actual
> + )
> +'
> +
> # configured prune tests
>
> set_config_tristate () {
[PATCH] remote: prefer exact matches when using refspecs
When matching a non-wildcard LHS of a refspec against a list of refs,
find_ref_by_name_abbrev() returns the first ref that matches using the
DWIM rules used by refname_match() in refs.c, even if an exact match
occurs later in the list of refs.
This causes unexpected behavior when (for example) fetching using the
refspec "refs/heads/s:" from a remote with both
"refs/heads/refs/heads/s" and "refs/heads/s". (Even if the former was
inadvertently created, one would still expect the latter to be fetched.)
This problem has only been observed when the desired ref comes after the
undesired ref in alphabetical order. However, for completeness, the test
in this patch also checks what happens when the desired ref comes first
alphabetically.
Signed-off-by: Jonathan Tan
---
remote.c | 7 +--
t/t5510-fetch.sh | 28
2 files changed, 33 insertions(+), 2 deletions(-)
diff --git a/remote.c b/remote.c
index 3fd43453f..eeffe3488 100644
--- a/remote.c
+++ b/remote.c
@@ -1687,12 +1687,15 @@ static struct ref *get_expanded_map(const struct ref
*remote_refs,
static const struct ref *find_ref_by_name_abbrev(const struct ref *refs, const
char *name)
{
+ const struct ref *best_match = NULL;
const struct ref *ref;
for (ref = refs; ref; ref = ref->next) {
- if (refname_match(name, ref->name))
+ if (!strcmp(name, ref->name))
return ref;
+ if (refname_match(name, ref->name))
+ best_match = ref;
}
- return NULL;
+ return best_match;
}
struct ref *get_remote_ref(const struct ref *remote_refs, const char *name)
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index e402aee6a..da88f35f0 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -535,6 +535,34 @@ test_expect_success "should be able to fetch with
duplicate refspecs" '
)
'
+test_expect_success 'LHS of refspec prefers exact matches' '
+ mkdir lhs-exact &&
+ (
+ cd lhs-exact &&
+ git init server &&
+ test_commit -C server unwanted &&
+ test_commit -C server wanted &&
+
+ git init client &&
+
+ # Check a name coming after "refs" alphabetically ...
+ git -C server update-ref refs/heads/s wanted &&
+ git -C server update-ref refs/heads/refs/heads/s unwanted &&
+ git -C client fetch ../server refs/heads/s:refs/heads/checkthis
&&
+ git -C server rev-parse wanted >expect &&
+ git -C client rev-parse checkthis >actual &&
+ test_cmp expect actual &&
+
+ # ... and one before.
+ git -C server update-ref refs/heads/q wanted &&
+ git -C server update-ref refs/heads/refs/heads/q unwanted &&
+ git -C client fetch ../server refs/heads/q:refs/heads/checkthis
&&
+ git -C server rev-parse wanted >expect &&
+ git -C client rev-parse checkthis >actual &&
+ test_cmp expect actual
+ )
+'
+
# configured prune tests
set_config_tristate () {
--
2.18.0.345.g5c9ce644c3-goog
Re: git merge -s subtree seems to be broken.
Jeff King writes: > On Tue, Jul 31, 2018 at 01:23:04PM -0400, Jeff King wrote: > ... > So here it is fixed, and with a commit message. I'm not happy to omit a > regression test, but I actually couldn't come up with a minimal one that > tickled the problem, because we're playing around with heuristics. So I > compensated by probably over-explaining in the commit message. But Have you tried to apply the message yourself? I'll fix it up but the hint to answer that question is in two extra pair of scissors. > clearly this is not a well-tested code path given the length of time > between introducing and detecting the bug. Thanks for writing it up. The patch itself still looks correct, too.
Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems
Jeff King writes: >> Presumably we are already in an error codepath, so if it is >> absolutely necessary, then we can issue a lstat() to grab the inum >> for the path we are about to create, iterate over the previously >> checked out paths issuing lstat() and see which one yields the same >> inum, to find the one who is the culprit. > > Yes, this is the cleverness I was missing in my earlier response. > > So it seems do-able, and I like that this incurs no cost in the > non-error case. Not so fast, unfortunately. I suspect that some filesystems do not give us inum that we can use for that "identity" purpose, and they tend to be the ones with the case smashing characteristics where we need this code in the error path the most X-<.
Re: [PATCH 2/8] t3206: add color test for range-diff --dual-color
Stefan Beller writes: > The 'expect'ed outcome has been taken by running the 'range-diff | decode'. > > Signed-off-by: Stefan Beller > --- > t/t3206-range-diff.sh | 39 +++ > 1 file changed, 39 insertions(+) > > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh > index 2237c7f4af9..019724e61a0 100755 > --- a/t/t3206-range-diff.sh > +++ b/t/t3206-range-diff.sh > @@ -142,4 +142,43 @@ test_expect_success 'changed message' ' > test_cmp expected actual > ' > > +test_expect_success 'dual-coloring' ' > + cat >expect <<-\EOF && It is a good idea to use something like "sed -e 's/^|//'" instead of "cat" here; that way allows you to mark the left-edge of the data with "|", for a test vector like this one that has a line that would otherwise violate the whitespace style rules. An inferiour alternative would be to add .gitaddtibute entry to make this file exempt from indent-with-tab rule, but even in this 40-line block there only is one line that requires such a workaround, and it won't help the initial application of this patch to get modified when applied with "am --whitespace=fix". > + 1: a4b = 1: f686024 s/5/A/ > + 2: f51d370 ! 2: > 4ab067d s/4/A/ > + @@ -2,6 +2,8 @@ > + > + s/4/A/ > + > + +Also a silly comment here! > + + > + diff --git a/file b/file > + --- a/file > + +++ b/file > + 3: 0559556 ! 3: > b9cb956 s/11/B/ > + @@ -10,7 +10,7 @@ > + 9 > + 10 > + -11 > + -+BB > + ++B > + 12 > + 13 > + 14 > + 4: d966c5c ! 4: > 8add5f1 s/12/B/ > + @@ -8,7 +8,7 @@ > + @@ > + 9 > + 10 > + - BB > + + B > + -12 > + +B > + 13 > + EOF > + git range-diff changed...changed-message --color --dual-color > >actual.raw && > + test_decode_color >actual + test_cmp expect actual > +' > + > test_done
Re: [PATCH v2 1/2] sequencer: handle errors in read_author_ident()
On Tue, Jul 31, 2018 at 7:15 AM Phillip Wood wrote:
> The calling code treated NULL as a valid return value, so fix this by
> returning and integer and passing in a parameter to receive the author.
It might be difficult for future readers (those who didn't follow the
discussion) to understand how/why NULL is not sufficient to signal an
error. Perhaps incorporating the explanation from your email[1] which
discussed that the author name, email, and/or date might change
unexpectedly would be sufficient. This excerpt from [1] might be a
good starting point:
... the caller does not treat NULL as an error, so this will
change the date and potentially the author of the commit
... [which] does corrupt the author data compared to its
expected value.
[1]:
https://public-inbox.org/git/c80cf729-1bbe-10f5-6837-b074d371b...@talktalk.net/
> Signed-off-by: Phillip Wood
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -701,57 +701,58 @@ static char *get_author(const char *message)
> -static const char *read_author_ident(struct strbuf *buf)
> +static int read_author_ident(char **author)
So, the caller is now responsible for freeing the string placed in
'author'. Okay.
> {
> - if (strbuf_read_file(buf, rebase_path_author_script(), 256) <= 0)
> - return NULL;
> + if (strbuf_read_file(, rebase_path_author_script(), 256) <= 0)
> + return -1;
I think you need to strbuf_release() in this error path since
strbuf_read_file() doesn't guarantee that the strbuf hasn't been
partially populated when it returns an error. (That is, this is
leaking.)
> /* dequote values and construct ident line in-place */
Ugh, this comment should have been adjusted in my series. A minor
matter, though, which can be tweaked later.
> /* validate date since fmt_ident() will die() on bad value */
> if (parse_date(val[2], )){
> - warning(_("invalid date format '%s' in '%s'"),
> + error(_("invalid date format '%s' in '%s'"),
> val[2], rebase_path_author_script());
> strbuf_release();
> - return NULL;
> + strbuf_release();
> + return -1;
You were careful to print the error, which references a value from
'buf', before destroying 'buf'. Good.
(A simplifying alternative would have been to not print the actual
value and instead say generally that "the date" was bad. Not a big
deal.)
> }
> - strbuf_swap(buf, );
> - strbuf_release();
> - return buf->buf;
> + *author = strbuf_detach(, NULL);
And, 'author' is only assigned when 0 is returned, so the caller only
has to free(author) upon success. Fine.
> + strbuf_release();
> + return 0;
> }
>
> static const char staged_changes_advice[] =
> @@ -794,12 +795,14 @@ static int run_git_commit(const char *defmsg, struct
> replay_opts *opts,
> - struct strbuf msg = STRBUF_INIT, script = STRBUF_INIT;
> - const char *author = is_rebase_i(opts) ?
> - read_author_ident() : NULL;
> + struct strbuf msg = STRBUF_INIT;
> + char *author = NULL;
> struct object_id root_commit, *cache_tree_oid;
> int res = 0;
>
> + if (is_rebase_i(opts) && read_author_ident())
> + return -1;
Logic looks correct, and it's nice to see that you went with 'return
-1' rather than die(), especially since the caller of run_git_commit()
is already able to handle -1.
Re: git merge -s subtree seems to be broken.
On Tue, Jul 31, 2018 at 03:52:26PM -0400, George Shammas wrote: > This is the fastest I ever seen an open source project respond to an issue > I reported. Thanks for being awesome! You're welcome. My speed is an inverse to how embarrassingly long we carried the bug for. ;) > > Signed-off-by: Jeff King > > --- > > match-trees.c | 43 ++- > > 1 file changed, 26 insertions(+), 17 deletions(-) Sorry, I meant to actually add a: Reported-by: George Shammas here. -Peff
Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems
On Tue, Jul 31, 2018 at 01:12:14PM -0700, Junio C Hamano wrote: > Jeff King writes: > > > On Tue, Jul 31, 2018 at 12:12:58PM -0700, Junio C Hamano wrote: > > ... > >> collapses two (or more) paths if we go that way. We only need to > >> report "we tried to check out X but it seems your filesystem equates > >> something else that is also in the project to X". > > > > Heh. See my similar suggestion in: > > > > https://public-inbox.org/git/20180728095659.ga21...@sigill.intra.peff.net/ > > > > and the response from Duy. > > Yes, but is there a reason why we need to report what that > "something else" is? I don't think it's strictly necessary, but it probably makes things easier for the user. That said... > Presumably we are already in an error codepath, so if it is > absolutely necessary, then we can issue a lstat() to grab the inum > for the path we are about to create, iterate over the previously > checked out paths issuing lstat() and see which one yields the same > inum, to find the one who is the culprit. Yes, this is the cleverness I was missing in my earlier response. So it seems do-able, and I like that this incurs no cost in the non-error case. -Peff
Re: [PATCH 2/2] Highlight keywords in remote sideband output.
On Tue, Jul 31, 2018 at 1:37 PM Han-Wen Nienhuys wrote:
> Highlight keywords in remote sideband output.
Prefix with the module you're touching, don't capitalize, and drop the
period. Perhaps:
sideband: highlight keywords in remote sideband output
> The highlighting is done on the client-side. Supported keywords are
> "error", "warning", "hint" and "success".
>
> The colorization is controlled with the config setting "color.remote".
What's the motivation for this change? The commit message should say
something about that and give an explanation of why this is done
client-side rather than server-side.
> Co-authored-by: Duy Nguyen
Helped-by: is more typical.
> Signed-off-by: Han-Wen Nienhuys
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -1229,6 +1229,15 @@ color.push::
> +color.remote::
> + A boolean to enable/disable colored remote output. If unset,
> + then the value of `color.ui` is used (`auto` by default).
If this is "boolean", what does "auto" mean? Perhaps update the
description to better match other color-related options.
> diff --git a/sideband.c b/sideband.c
> @@ -1,6 +1,97 @@
> +void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
> +{
> + int i;
> +
> + load_sideband_colors();
> + if (!want_color_stderr(sideband_use_color)) {
> + strbuf_add(dest, src, n);
> + return;
> + }
Can load_sideband_colors() be moved below the !want_color_stderr() conditional?
> +
> + while (isspace(*src)) {
> + strbuf_addch(dest, *src);
> + src++;
> + n--;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(keywords); i++) {
> + struct kwtable* p = keywords + i;
> + int len = strlen(p->keyword);
Would it make sense to precompute each keyword length so you don't
have to recompute them repeatedly, or is that premature optimization?
> + if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len]))
> {
So, the strncasecmp() is checking if one of the recognized keywords is
at the 'src' position, and the !isalnum() ensures that you won't pick
up something of which the keyword is merely a prefix. For instance,
you won't mistakenly highlight "successful". It also works correctly
when 'len' happens to reference the end-of-string NUL. Okay.
> + strbuf_addstr(dest, p->color);
> + strbuf_add(dest, src, len);
> + strbuf_addstr(dest, GIT_COLOR_RESET);
> + n -= len;
> + src += len;
> + break;
> + }
So, despite the explanation in the commit message, this function isn't
_generally_ highlighting keywords in the sideband. Instead, it is
highlighting a keyword only if it finds it at the start of string
(ignoring whitespace). Perhaps the commit message could be more clear
about that.
A natural follow-on question is whether strings are fed to this
function one line at a time or if the incoming string may have
embedded newlines (in which case, you might need to find a prefix
following a newline, as well?).
> + }
> +
> + strbuf_add(dest, src, n);
> +}
Re: [PATCH 2/3] config: fix case sensitive subsection names on writing
Stefan Beller writes: > A use reported a submodule issue regarding strange case indentation > issues, but it could be boiled down to the following test case: > > $ git init test && cd test > $ git config foo."Bar".key test > $ git config foo."bar".key test > $ tail -n 3 .git/config > [foo "Bar"] > key = test > key = test > > Sub sections are case sensitive and we have a test for correctly reading > them. However we do not have a test for writing out config correctly with > case sensitive subsection names, which is why this went unnoticed in > 6ae996f2acf (git_config_set: make use of the config parser's event > stream, 2018-04-09) Am I correct to understand that this patch is a "FIX" for breakage introduced by that commit? The phrasing is not helping me to pick a good base to queue these patches on. Thanks.
[PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted
Hello, me and my colleague are struggling with automation of verifying git repositories and we have encountered that git verify-commit and verify-tag accepts untrusted signatures and exit successfully. We have done some investigation of the GPG verification changes in git repository which I includes in this patch message. GPG results `TRUST_NEVER` and `TRUST_UNDEFINED` in raw output is treated as untrusted in git (U) and should not be accepted in verify-commit and verify-tag command. In 434060ec6d verify-tag and verify-commit was centralized into check_signature function and good (G) and untrusted (U) signatures were marked as valid and exited successfully. In this commit it is incorrectly stated that this behavior is adopted from older verify-tag function however original verify-tag behavior was to return exit code from gpg process itself (removed in a4cc18f29). Also rejecting untrusted (U) signature is the pull/merge with --verify-signatures behavior (defined in builtin/merge.c cmd_merge function and presented in eb307ae7bb). The behavior of merge/pull --verify-signatures and verify-commit/verify-tag should be the same. With regards, Vojtech Myslivec and Karel Koci From c9c7b555da284c4f67fe36dc95d592644089544a Mon Sep 17 00:00:00 2001 From: Vojtech Myslivec Date: Tue, 31 Jul 2018 20:32:32 +0200 Subject: [PATCH] gpg-interface: Do not accept untrusted signatures In 434060ec6d verify-tag and verify-commit was centralized into check_signature function and good (G) and untrusted (U) signatures were marked as valid and exited successfully. In this commit it is incorrectly stated that this behavior is adopted from older verify-tag function however original verify-tag behavior was to return exit code from gpg process itself (removed in a4cc18f29). Also rejecting untrusted (U) signature is the pull/merge with --verify-signatures behavior (defined in builtin/merge.c cmd_merge function and presented in eb307ae7bb). The behavior of merge/pull --verify-signatures and verify-commit/verify-tag should be the same. --- gpg-interface.c | 2 +- t/t7030-verify-tag.sh| 4 ++-- t/t7510-signed-commit.sh | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/gpg-interface.c b/gpg-interface.c index 09ddfbc26..83adc7d12 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -86,7 +86,7 @@ int check_signature(const char *payload, size_t plen, const char *signature, strbuf_release(_status); strbuf_release(_output); - return sigc->result != 'G' && sigc->result != 'U'; + return sigc->result != 'G'; } void print_signature_buffer(const struct signature_check *sigc, unsigned flags) diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh index 291a1e2b0..d6f77c443 100755 --- a/t/t7030-verify-tag.sh +++ b/t/t7030-verify-tag.sh @@ -63,7 +63,7 @@ test_expect_success GPG 'verify and show signatures' ' ( for tag in eighth-signed-alt do - git verify-tag $tag 2>actual && + test_must_fail git verify-tag $tag 2>actual && grep "Good signature from" actual && ! grep "BAD signature from" actual && grep "not certified" actual && @@ -103,7 +103,7 @@ test_expect_success GPG 'verify signatures with --raw' ' ( for tag in eighth-signed-alt do - git verify-tag --raw $tag 2>actual && + test_must_fail git verify-tag --raw $tag 2>actual && grep "GOODSIG" actual && ! grep "BADSIG" actual && grep "TRUST_UNDEFINED" actual && diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh index 6e2015ed9..5cb388cb6 100755 --- a/t/t7510-signed-commit.sh +++ b/t/t7510-signed-commit.sh @@ -89,8 +89,8 @@ test_expect_success GPG 'verify and show signatures' ' ) ' -test_expect_success GPG 'verify-commit exits success on untrusted signature' ' - git verify-commit eighth-signed-alt 2>actual && +test_expect_success GPG 'verify-commit exits unsuccessfully on untrusted signature' ' + test_must_fail git verify-commit eighth-signed-alt 2>actual && grep "Good signature from" actual && ! grep "BAD signature from" actual && grep "not certified" actual @@ -118,7 +118,7 @@ test_expect_success GPG 'verify signatures with --raw' ' ( for commit in eighth-signed-alt do - git verify-commit --raw $commit 2>actual && + test_must_fail git verify-commit --raw $commit 2>actual && grep "GOODSIG" actual && ! grep "BADSIG" actual && grep "TRUST_UNDEFINED" actual && -- 2.18.0 signature.asc Description: OpenPGP digital signature
Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems
Jeff King writes: > On Tue, Jul 31, 2018 at 12:12:58PM -0700, Junio C Hamano wrote: > ... >> collapses two (or more) paths if we go that way. We only need to >> report "we tried to check out X but it seems your filesystem equates >> something else that is also in the project to X". > > Heh. See my similar suggestion in: > > https://public-inbox.org/git/20180728095659.ga21...@sigill.intra.peff.net/ > > and the response from Duy. Yes, but is there a reason why we need to report what that "something else" is? Presumably we are already in an error codepath, so if it is absolutely necessary, then we can issue a lstat() to grab the inum for the path we are about to create, iterate over the previously checked out paths issuing lstat() and see which one yields the same inum, to find the one who is the culprit.
Re: Is detecting endianness at compile-time unworkable?
On 31/07/2018 16:25, Ævar Arnfjörð Bjarmason wrote: ...the real trick is using these macros outside of GCC / glibc and on older GCC versions. See the github link above, you basically end up with a whitelist of how it looks on different systems / compilers. Sometimes both are defined, sometimes only both etc. It can be done, but as that code shows it's somewhat complex macro soup to get right. FYI - the gcc I was using is 4.7.4. And, the reason I suggest the test for both not being defined is so that 'make' stops and whoever is running make just sets one or the other. Let them 'file a bug' When they come with a compiler that does not work - and find out what could be used. For example, _AIX is the same as _BIG_ENDIAN. In the meantime, the code to test is simple. Either one of _BIG_ENDIAN or _LITTLE_ENDIAN is provided by the compiler or the builder supplies one of the two using CFLAGS. I assume there is also a "undefine" flag, maybe -U - so hopefully a -U and a -D combination could be used for cross-compiling. re: my mailer blocking things - it would only be for this list, as other lists come through with no extra work from me. At least I am not aware of anything special I could do.
Re: [PATCH v2] checkout: optimize "git checkout -b "
Ben Peart writes: > The biggest change in this version was suggested in feedback to the last > patch. I have turned on the optimzation by default if sparse-checkout is > not on so that most users do not have to set anything and they will get the > benefit of the optimization. Sounds like a good thing to do. If we missed something in the logic of skip_merge_working_tree(), the breakage may affect more unsuspecting people, which may or may not be a bad thing---at least it would allow us to notice it sooner. > + When set to true, "git checkout -b " will not update the > + skip-worktree bit in the index nor add/remove files in the working > + directory to reflect the current sparse checkout settings. This reads a lot clearer, at least to me, than the documentation update in the previous round, by speaking in terms of what the user-visible side effect of this setting would be. Nicely done. Thanks.
Are you sure?
I have a Businesss Proposal for you, Can you do it? If yes please get back to me for more details.
Re: [PATCH 1/1] Add the `p4-pre-submit` hook
On 31 July 2018 at 16:40, Junio C Hamano wrote: > Luke Diamand writes: > >> I think there is an error in the test harness. >> >> On 31 July 2018 at 10:46, SZEDER Gábor wrote: + test_must_fail git-p4 submit --dry-run >errs 2>&1 &&> + ! grep "Would apply" err >> >> It writes to the file "errs" but then looks for the message in "err". >> >> Luke > > Sigh. Thanks for spotting, both of you. > > Here is what I'd locally squash in. If there is anything else, I'd > rather see a refreshed final one sent by the author. > > Thanks. > > t/t9800-git-p4-basic.sh | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh > index 2b7baa95d2..729cd25770 100755 > --- a/t/t9800-git-p4-basic.sh > +++ b/t/t9800-git-p4-basic.sh > @@ -274,19 +274,19 @@ test_expect_success 'run hook p4-pre-submit before > submit' ' > git add hello.txt && > git commit -m "add hello.txt" && > git config git-p4.skipSubmitEdit true && > - git-p4 submit --dry-run >out && > + git p4 submit --dry-run >out && > grep "Would apply" out && > mkdir -p .git/hooks && > write_script .git/hooks/p4-pre-submit <<-\EOF && > exit 0 > EOF > - git-p4 submit --dry-run >out && > + git p4 submit --dry-run >out && > grep "Would apply" out && > write_script .git/hooks/p4-pre-submit <<-\EOF && > exit 1 > EOF > - test_must_fail git-p4 submit --dry-run >errs 2>&1 && > - ! grep "Would apply" err > + test_must_fail git p4 submit --dry-run >errs 2>&1 && > + ! grep "Would apply" errs > ) > ' That set of deltas works for me. Sorry, I should have run the tests myself earlier and I would have picked up on this.
Re: git merge -s subtree seems to be broken.
This is the fastest I ever seen an open source project respond to an issue
I reported. Thanks for being awesome!
On Tue, Jul 31, 2018 at 3:05 PM Jeff King wrote:
> On Tue, Jul 31, 2018 at 01:23:04PM -0400, Jeff King wrote:
>
> > On Tue, Jul 31, 2018 at 10:17:15AM -0700, Junio C Hamano wrote:
> >
> > > Jeff King writes:
> > >
> > > > +...
> > > > + } else if (cmp > 0) {
> > > > /* path2 does not appear in one */
> > > > + score += score_missing(two.entry.mode,
> two.entry.path);
> > > > + update_tree_entry();
> > > > + continue;
> > > > + } if (oidcmp(one.entry.oid, two.entry.oid)) {
> > >
> > > As the earlier ones do the "continue at the end of the block", this
> > > does not affect the correctness, but I think you either meant "else if"
> > > or a fresh "if/else" that is disconnected from the previous if/else
> if/...
> > > chain.
> >
> > Yes, thanks. I actually started to write it without the "continue" at
> > all, and a big "else" that checked the "we have both" case. But I backed
> > that out (in favor of a smaller diff), and forgot to add back in the
> > "else if".
>
> So here it is fixed, and with a commit message. I'm not happy to omit a
> regression test, but I actually couldn't come up with a minimal one that
> tickled the problem, because we're playing around with heuristics. So I
> compensated by probably over-explaining in the commit message. But
> clearly this is not a well-tested code path given the length of time
> between introducing and detecting the bug.
>
> -- >8 --
> Subject: [PATCH] score_trees(): fix iteration over trees with missing
> entries
>
> In score_trees(), we walk over two sorted trees to find
> which entries are missing or have different content between
> the two. So if we have two trees with these entries:
>
> one two
> --- ---
> a a
> b c
> c d
>
> we'd expect the loop to:
>
> - compare "a" to "a"
>
> - compare "b" to "c"; because these are sorted lists, we
> know that the second tree does not have "b"
>
> - compare "c" to "c"
>
> - compare "d" to end-of-list; we know that the first tree
> does not have "d"
>
> And prior to d8febde370 (match-trees: simplify score_trees()
> using tree_entry(), 2013-03-24) that worked. But after that
> commit, we mistakenly increment the tree pointers for every
> loop iteration, even when we've processed the entry for only
> one side. As a result, we end up doing this:
>
> - compare "a" to "a"
>
> - compare "b" to "c"; we know that we do not have "b", but
> we still increment both tree pointers; at this point
> we're out of sync and all further comparisons are wrong
>
> - compare "c" to "d" and mistakenly claim that the second
> tree does not have "c"
>
> - exit the loop, mistakenly not realizing that the first
> tree does not have "d"
>
> So contrary to the claim in d8febde370, we really do need to
> manually use update_tree_entry(), because advancing the tree
> pointer depends on the entry comparison.
>
> That means we must stop using tree_entry() to access each
> entry, since it auto-advances the pointer. Instead:
>
> - we'll use tree_desc.size directly to know if there's
> anything left to look at (which is what tree_entry() was
> doing under the hood)
>
> - rather than do an extra struct assignment to "e1" and
> "e2", we can just access the "entry" field of tree_desc
> directly
>
> That makes us a little more intimate with the tree_desc
> code, but that's not uncommon for its callers.
>
> There's no regression test here, as it's a little tricky to
> trigger this with a minimal example. The user-visible effect
> is that the heuristics fail to correlate two trees that
> should be. But in a minimal example, there aren't a lot of
> other trees to match, so we often end up doing the right
> thing anyway.
>
> A real-world example (from the original bug report) is:
>
> -- >8 --
> git init repo
> cd repo
>
> echo init >file
> git add file
> git commit -m init
>
> git remote add tig https://github.com/jonas/tig.git
> git fetch tig
> git merge -s ours --no-commit --allow-unrelated-histories tig-2.3.0
> git read-tree --prefix=src/ -u tig-2.3.0
> git commit -m 'get upstream tig-2.3.0'
>
> echo update >file
> git commit -a -m update
>
> git merge -s subtree tig-2.4.0
> -- 8< --
>
> Before this patch, we fail to realize that the tig-2.4.0
> content should go into the "src" directory.
>
> Signed-off-by: Jeff King
> ---
> match-trees.c | 43 ++-
> 1 file changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/match-trees.c b/match-trees.c
> index 4cdeff53e1..37653308d3 100644
> --- a/match-trees.c
> +++ b/match-trees.c
> @@ -83,34 +83,43 @@ static int score_trees(const struct object_id *hash1,
> const struct object_id *ha
> int score = 0;
>
> for (;;) {
> - struct name_entry e1, e2;
> - int
Re: [PATCH 2/2] Highlight keywords in remote sideband output.
Han-Wen Nienhuys writes:
> The highlighting is done on the client-side. Supported keywords are
> "error", "warning", "hint" and "success".
>
> The colorization is controlled with the config setting "color.remote".
>
> Co-authored-by: Duy Nguyen
> Signed-off-by: Han-Wen Nienhuys
Thanks. I'll squash the following in while queuing, though.
* maybe_colorize_sideband() does not have outside caller; make it
static to avoid missing-prototype error that breaks compilation.
* correct space-before-tab whitespace style violation.
* use write_script.
* a test script must be executable to avoid triggering test-lint.
* avoid overlong lines in the test.
* no SP between redirection operator and its target.
Other than that, the result looks good to me. So that others can
eyeball the result once more, I'll keep it in 'pu' for a few days,
and if nothing else comes up, hopefully the topic can be merged to
'next' after that.
diff --git a/sideband.c b/sideband.c
index 0d67583ec5..be4635446c 100644
--- a/sideband.c
+++ b/sideband.c
@@ -60,12 +60,12 @@ void list_config_color_sideband_slots(struct string_list
*list, const char *pref
* Optionally highlight some keywords in remote output if they appear at the
* start of the line.
*/
-void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
+static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int
n)
{
int i;
load_sideband_colors();
- if (!want_color_stderr(sideband_use_color)) {
+ if (!want_color_stderr(sideband_use_color)) {
strbuf_add(dest, src, n);
return;
}
diff --git a/t/t5409-colorize-remote-messages.sh
b/t/t5409-colorize-remote-messages.sh
old mode 100644
new mode 100755
index 4e1bd421ff..4547ec95b8
--- a/t/t5409-colorize-remote-messages.sh
+++ b/t/t5409-colorize-remote-messages.sh
@@ -6,27 +6,27 @@ test_description='remote messages are colorized on the client'
test_expect_success 'setup' '
mkdir .git/hooks &&
- cat << EOF > .git/hooks/update &&
-#!/bin/sh
-echo error: error
-echo hint: hint
-echo success: success
-echo warning: warning
-echo prefixerror: error
-exit 0
-EOF
- chmod +x .git/hooks/update &&
+ write_script .git/hooks/update <<-\EOF &&
+ echo error: error
+ echo hint: hint
+ echo success: success
+ echo warning: warning
+ echo prefixerror: error
+ exit 0
+ EOF
+
echo 1 >file &&
git add file &&
git commit -m 1 &&
git clone . child &&
cd child &&
- echo 2 > file &&
+ echo 2 >file &&
git commit -a -m 2
'
test_expect_success 'push' '
- git -c color.remote=always push -f origin HEAD:refs/heads/newbranch
2>output &&
+ git -c color.remote=always \
+ push -f origin HEAD:refs/heads/newbranch 2>output &&
test_decode_color decoded &&
grep "error:" decoded &&
grep "hint:" decoded &&
@@ -36,7 +36,8 @@ test_expect_success 'push' '
'
test_expect_success 'push with customized color' '
- git -c color.remote=always -c color.remote.error=white push -f origin
HEAD:refs/heads/newbranch2 2>output &&
+ git -c color.remote=always -c color.remote.error=white \
+ push -f origin HEAD:refs/heads/newbranch2 2>output &&
test_decode_color decoded &&
grep "error:" decoded &&
grep "hint:" decoded &&
Re: Git clone and case sensitivity
On 7/29/2018 5:28 AM, Jeff King wrote: On Sun, Jul 29, 2018 at 07:26:41AM +0200, Duy Nguyen wrote: strcasecmp() will only catch a subset of the cases. We really need to follow the same folding rules that the filesystem would. True. But that's how we handle case insensitivity internally. If a filesytem has more sophisticated folding rules then git will not work well on that one anyway. Hrm. Yeah, I guess that's the best we can do for the actual in-memory checks. Everything else depends on doing an actual filesystem operation, and our icase stuff kicks in way before then. I was mostly thinking of HFS+ utf8 normalization weirdness, but I guess people are accustomed to that by now. For the case of clone, I actually wonder if we could detect during the checkout step that a file already exists. Since we know that the directory we started with was empty, then if it does, either: - there's some funny case-folding going on that means two paths in the repository map to the same name in the filesystem; or - somebody else is writing to the directory at the same time as us This is exactly what my first patch does (minus the sparse checkout part). Right, sorry, I should have read that one more carefully. But without knowing the exact folding rules, I don't think we can locate this "somebody else" who wrote the first path. So if N paths are treated the same by this filesystem, we could only report N-1 of them. If we want to report just one path when this happens though, then this works quite well. Hmm. Since most such systems are case-preserving, would it be possible to report the name of the existing file? Doing it via opendir/readdir is hacky, and anyway puts the burden on us to find the matching name. Doing it via fstat() on the opened file doesn't work because at that the filesystem has resolved the name to an inode. So yeah, perhaps strcasecmp() is the best we can do (I do agree that being able to mention all of the conflicting names is a benefit). I guess we should be using fspathcmp(), though, in case it later learns to be smarter. -Peff As has already been mentioned, this gets into weird territory really fast, between case folding, final space/dot on windows, utf8 NFC/NFD weirdness on the mac, utf8 invisible chars on the mac, long/short names on windows, and etc. And that's just for filenames. Things really get weird if directory names have these ambiguities. Perhaps just print the problematic paths (where the collision is detected) and let the user decide how to correct them. Perhaps we could have a separate tool that could scan the index or commit for potential conflicts and warn them in advance (granted, it might not be perfect and may report a few false positives). Forcing them into a sparse-checkout situation might be over their skill level. Jeff
Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems
On Tue, Jul 31, 2018 at 12:12:58PM -0700, Junio C Hamano wrote: > Elijah Newren writes: > > > Is it worth attempting to also warn about paths that only differ in > > UTF-normalization on relevant MacOS systems? > > I hate to bring up a totally different approach this late in the > party, but I wonder if it makes more sense to take advantage of > "clone" being a command that starts from an empty working tree. > > builtin/clone.c::checkout() drives a single-tree unpack_trees(), > using oneway_merge() as its callback and at the end, eventually > unpack_trees.c:check_updates() will call into checkout_entry() > to perform the usual "unlink and then create" dance. > > I wonder if it makes sense to introduce a new option to tell the > machinery to report when the final checkout_entry() notices that it > needed to remove the working tree file to make room (perhaps that > bit would go in "struct unpack_trees_options"). In the initial > checkout codepath for a freshly cloned repository, that would only > happen when your tree has two (or more) paths that gets smashed > by case insensitive or UTF-normalizing filesystem, and the code we > would maintain do not have to care how exactly the filesystem > collapses two (or more) paths if we go that way. We only need to > report "we tried to check out X but it seems your filesystem equates > something else that is also in the project to X". Heh. See my similar suggestion in: https://public-inbox.org/git/20180728095659.ga21...@sigill.intra.peff.net/ and the response from Duy. -Peff
Re: [GSoC][PATCH v5 00/20] rebase -i: rewrite in C
Alban Gruin writes:
> This patch series rewrite the interactive rebase from shell to C.
Thanks.
> It is based on ffc6fa0e39 ("Fourth batch for 2.19 cycle", 2018-07-24).
> The v4 was based on b7bd9486 ("Third batch for 2.19 cycle", 2018-07-18).
> I wanted to make sure my series works well with 'bb/pedantic',
> 'jk/empty-pick-fix', and 'as/sequencer-customizable-comment-char', as
> they modified sequencer.c.
It is a good practice to keey an eye on other topics in flight to
make sure you play well with others.
What you can do better in a case like this is to apply the patches
on the same base as v4 and then trial merge the result into the
newer base of your choice (e.g. ffc6fa0e39), and also apply the same
patches on top of the same newer base. If
(1) the application to the old base goes cleanly,
(2) the trial merge goes cleanly, and
(3) the result of the trial merge exactly matches the tree as applying
the patches on the newer base
then it is preferrable not to rebase but keep the old base as the
previous round to avoid needless churn. For a new development like
this (as opposed to "fix for long standing bugs"), keeping an old
and tested base does not matter too much, but it is a good
discipline to get into to hold your base steady.
The patches looked all good and applied cleanly. Will queue and
wait for a few days to see if anybody spots something glaringly
wrong (I expect none) and then merge it to 'next'.
Thanks.
Re: [PATCH] transport: report refs only if transport does
On Mon, Jul 30, 2018 at 03:56:01PM -0700, Jonathan Tan wrote:
> Commit 989b8c4452 ("fetch-pack: put shallow info in output parameter",
> 2018-06-28) allows transports to report the refs that they have fetched
> in a new out-parameter "fetched_refs". If they do so,
> transport_fetch_refs() makes this information available to its caller.
>
> Because transport_fetch_refs() filters the refs sent to the transport,
> it cannot just report the transport's result directly, but first needs
> to readd the excluded refs, pretending that they are fetched. However,
> this results in a wrong result if the transport did not report the refs
> that they have fetched in "fetched_refs" - the excluded refs would be
> added and reported, presenting an incomplete picture to the caller.
This part leaves me confused. If we are not fetching them, then why do
we need to pretend that they are fetched?
I think I am showing my lack of understanding about the reason for this
whole "return the fetched refs" scheme from 989b8c4452, and probably
reading the rest of that series would make it more clear. But from the
perspective of somebody digging into history and finding just this
commit, it probably needs to lay out a little more of the reasoning.
> Thanks for the reproduction recipe, Peff. Here's a fix. It can be
> reproduced with something using a remote helper's fetch command (and not
> using "connect" or "stateless-connect"), fetching at least one ref that
> requires a ref update and at least one that does not (as you can see
> from the included test).
Ah, that explains why I couldn't reproduce it with another repository; I
was using a direct git-upload-pack fetch, which wouldn't trigger the
remote helper code.
> diff --git a/transport.c b/transport.c
> index fdd813f684..2a2415d79c 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1230,17 +1230,18 @@ int transport_fetch_refs(struct transport *transport,
> struct ref *refs,
> struct ref **heads = NULL;
> struct ref *nop_head = NULL, **nop_tail = _head;
> struct ref *rm;
> + struct ref *fetched_by_transport = NULL;
>
> for (rm = refs; rm; rm = rm->next) {
> nr_refs++;
> if (rm->peer_ref &&
> !is_null_oid(>old_oid) &&
> !oidcmp(>peer_ref->old_oid, >old_oid)) {
> - /*
> - * These need to be reported as fetched, but we don't
> - * actually need to fetch them.
> - */
> if (fetched_refs) {
> + /*
> + * These may need to be reported as fetched,
> + * but we don't actually need to fetch them.
> + */
So it's really this comment that leaves me the most puzzled.
> @@ -1264,10 +1265,25 @@ int transport_fetch_refs(struct transport *transport,
> struct ref *refs,
> heads[nr_heads++] = rm;
> }
>
> - rc = transport->vtable->fetch(transport, nr_heads, heads, fetched_refs);
> - if (fetched_refs && nop_head) {
> - *nop_tail = *fetched_refs;
> - *fetched_refs = nop_head;
> + rc = transport->vtable->fetch(transport, nr_heads, heads,
> + fetched_refs ? _by_transport :
> NULL);
> + if (fetched_refs) {
> + if (fetched_by_transport) {
> + /*
> + * The transport reported its fetched refs. Pretend
> + * that we also fetched the ones that we didn't need to
> + * fetch.
> + */
> + *nop_tail = fetched_by_transport;
> + *fetched_refs = nop_head;
> + } else if (!fetched_by_transport) {
> + /*
> + * The transport didn't report its fetched refs, so
> + * this function will not report them either. We have
> + * no use for nop_head.
> + */
> + free_refs(nop_head);
> + }
This part makes sense to me based on the description (and on the
assumption that reporting those nop refs is useful in the first place ;)
).
So I think your fix here is probably the right thing, but I'm just left
confused by the background a bit.
-Peff
Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems
Elijah Newren writes: > Is it worth attempting to also warn about paths that only differ in > UTF-normalization on relevant MacOS systems? I hate to bring up a totally different approach this late in the party, but I wonder if it makes more sense to take advantage of "clone" being a command that starts from an empty working tree. builtin/clone.c::checkout() drives a single-tree unpack_trees(), using oneway_merge() as its callback and at the end, eventually unpack_trees.c:check_updates() will call into checkout_entry() to perform the usual "unlink and then create" dance. I wonder if it makes sense to introduce a new option to tell the machinery to report when the final checkout_entry() notices that it needed to remove the working tree file to make room (perhaps that bit would go in "struct unpack_trees_options"). In the initial checkout codepath for a freshly cloned repository, that would only happen when your tree has two (or more) paths that gets smashed by case insensitive or UTF-normalizing filesystem, and the code we would maintain do not have to care how exactly the filesystem collapses two (or more) paths if we go that way. We only need to report "we tried to check out X but it seems your filesystem equates something else that is also in the project to X".
Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems
Nguyễn Thái Ngọc Duy writes: > Another thing we probably should do is catch in "git checkout" too, > not just "git clone" since your linux/unix colleage colleague may > accidentally add some files that your mac/windows machine is not very > happy with. Then you would catch it not in checkout but in add, no?
Re: [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Tue, Jul 31, 2018 at 02:55:51PM -0400, Eric Sunshine wrote: > > I hesitate to make any suggestion here, as I think we may have passed > > a point of useful cost/benefit in sinking more time into this script. > > But...is switching to awk or perl an option? Our test suite already > > depends on having a vanilla perl, so I don't think it would be a new > > dependency. And it would give you actual data structures. > > It would, and I did consider it, however, I was very concerned about > startup cost (launch time) with heavyweight perl considering that it > would have to be run for _every_ test. With 13000+ tests, that cost > was a very real concern, especially for Windows users, but even for > MacOS users (such as myself, for which the full test suite already > takes probably close to 30 minutes to run, even on a ram drive). So, I > wanted something very lightweight (and deliberately used that word in > the commit message), and 'sed' seemed the lightest-weight of the > bunch. Both perl and sed seem about the same on my system (sometimes one is faster than the other, and sometimes vice versa). However, I expect for Windows the problem is not how big the child executable is, but running a child process at all. I might be wrong, though. > 'awk' might be about as lightweight as 'sed', and it may even be > possible to coerce it into handling the task (since the linter's job > is primarily just a bunch of regex matching with very little > "manipulating"). v1 of the linter was somewhat simpler and didn't deal > with these more complex cases, such as nested here-docs. v1 also did > rather more "manipulating" of the script since the result was meant to > be run by the shell. When it came time to implement v2, which detects > broken &&-chains itself by textual inspection, most of the > functionality (coming from v1) was already implemented in 'sed', so > 'awk' never really came up as a candidate since rewriting the script > from scratch in 'awk' didn't seem like a good idea. (And, at the time > v2 was started, I didn't know that these more complex cases would > arise.) So, 'awk' might be a viable alternative, and perhaps I'll take > a stab at it for fun at some point (or not), but I don't think there's > a pressing need right now. Yeah, I agree with that. -Peff
Re: git merge -s subtree seems to be broken.
On Tue, Jul 31, 2018 at 01:23:04PM -0400, Jeff King wrote:
> On Tue, Jul 31, 2018 at 10:17:15AM -0700, Junio C Hamano wrote:
>
> > Jeff King writes:
> >
> > > +...
> > > + } else if (cmp > 0) {
> > > /* path2 does not appear in one */
> > > + score += score_missing(two.entry.mode, two.entry.path);
> > > + update_tree_entry();
> > > + continue;
> > > + } if (oidcmp(one.entry.oid, two.entry.oid)) {
> >
> > As the earlier ones do the "continue at the end of the block", this
> > does not affect the correctness, but I think you either meant "else if"
> > or a fresh "if/else" that is disconnected from the previous if/else if/...
> > chain.
>
> Yes, thanks. I actually started to write it without the "continue" at
> all, and a big "else" that checked the "we have both" case. But I backed
> that out (in favor of a smaller diff), and forgot to add back in the
> "else if".
So here it is fixed, and with a commit message. I'm not happy to omit a
regression test, but I actually couldn't come up with a minimal one that
tickled the problem, because we're playing around with heuristics. So I
compensated by probably over-explaining in the commit message. But
clearly this is not a well-tested code path given the length of time
between introducing and detecting the bug.
-- >8 --
Subject: [PATCH] score_trees(): fix iteration over trees with missing entries
In score_trees(), we walk over two sorted trees to find
which entries are missing or have different content between
the two. So if we have two trees with these entries:
one two
--- ---
a a
b c
c d
we'd expect the loop to:
- compare "a" to "a"
- compare "b" to "c"; because these are sorted lists, we
know that the second tree does not have "b"
- compare "c" to "c"
- compare "d" to end-of-list; we know that the first tree
does not have "d"
And prior to d8febde370 (match-trees: simplify score_trees()
using tree_entry(), 2013-03-24) that worked. But after that
commit, we mistakenly increment the tree pointers for every
loop iteration, even when we've processed the entry for only
one side. As a result, we end up doing this:
- compare "a" to "a"
- compare "b" to "c"; we know that we do not have "b", but
we still increment both tree pointers; at this point
we're out of sync and all further comparisons are wrong
- compare "c" to "d" and mistakenly claim that the second
tree does not have "c"
- exit the loop, mistakenly not realizing that the first
tree does not have "d"
So contrary to the claim in d8febde370, we really do need to
manually use update_tree_entry(), because advancing the tree
pointer depends on the entry comparison.
That means we must stop using tree_entry() to access each
entry, since it auto-advances the pointer. Instead:
- we'll use tree_desc.size directly to know if there's
anything left to look at (which is what tree_entry() was
doing under the hood)
- rather than do an extra struct assignment to "e1" and
"e2", we can just access the "entry" field of tree_desc
directly
That makes us a little more intimate with the tree_desc
code, but that's not uncommon for its callers.
There's no regression test here, as it's a little tricky to
trigger this with a minimal example. The user-visible effect
is that the heuristics fail to correlate two trees that
should be. But in a minimal example, there aren't a lot of
other trees to match, so we often end up doing the right
thing anyway.
A real-world example (from the original bug report) is:
-- >8 --
git init repo
cd repo
echo init >file
git add file
git commit -m init
git remote add tig https://github.com/jonas/tig.git
git fetch tig
git merge -s ours --no-commit --allow-unrelated-histories tig-2.3.0
git read-tree --prefix=src/ -u tig-2.3.0
git commit -m 'get upstream tig-2.3.0'
echo update >file
git commit -a -m update
git merge -s subtree tig-2.4.0
-- 8< --
Before this patch, we fail to realize that the tig-2.4.0
content should go into the "src" directory.
Signed-off-by: Jeff King
---
match-trees.c | 43 ++-
1 file changed, 26 insertions(+), 17 deletions(-)
diff --git a/match-trees.c b/match-trees.c
index 4cdeff53e1..37653308d3 100644
--- a/match-trees.c
+++ b/match-trees.c
@@ -83,34 +83,43 @@ static int score_trees(const struct object_id *hash1, const
struct object_id *ha
int score = 0;
for (;;) {
- struct name_entry e1, e2;
- int got_entry_from_one = tree_entry(, );
- int got_entry_from_two = tree_entry(, );
int cmp;
- if (got_entry_from_one && got_entry_from_two)
- cmp = base_name_entries_compare(, );
- else if (got_entry_from_one)
+ if (one.size && two.size)
+ cmp = base_name_entries_compare(, );
+ else if
Re: [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Tue, Jul 31, 2018 at 8:50 AM Jeff King wrote: > On Mon, Jul 30, 2018 at 05:38:06PM -0400, Eric Sunshine wrote: > > I considered that, but it doesn't handle nested here-docs, which we > > actually have in the test suite. For instance, from t9300-fast-import: > > [...] > > Nesting could be handled easily enough either by stashing away the > > opening tag and matching against it later _or_ by doing recursive > > here-doc folding, however, 'sed' isn't a proper programming language > > and can't be coerced into doing either of those. (And, it was tricky > > enough just getting it to handle the nested case with a limited set of > > recognized tag names, without having to explicitly handle every > > combination of those names nested inside one another.) > > I hesitate to make any suggestion here, as I think we may have passed > a point of useful cost/benefit in sinking more time into this script. > But...is switching to awk or perl an option? Our test suite already > depends on having a vanilla perl, so I don't think it would be a new > dependency. And it would give you actual data structures. It would, and I did consider it, however, I was very concerned about startup cost (launch time) with heavyweight perl considering that it would have to be run for _every_ test. With 13000+ tests, that cost was a very real concern, especially for Windows users, but even for MacOS users (such as myself, for which the full test suite already takes probably close to 30 minutes to run, even on a ram drive). So, I wanted something very lightweight (and deliberately used that word in the commit message), and 'sed' seemed the lightest-weight of the bunch. 'awk' might be about as lightweight as 'sed', and it may even be possible to coerce it into handling the task (since the linter's job is primarily just a bunch of regex matching with very little "manipulating"). v1 of the linter was somewhat simpler and didn't deal with these more complex cases, such as nested here-docs. v1 also did rather more "manipulating" of the script since the result was meant to be run by the shell. When it came time to implement v2, which detects broken &&-chains itself by textual inspection, most of the functionality (coming from v1) was already implemented in 'sed', so 'awk' never really came up as a candidate since rewriting the script from scratch in 'awk' didn't seem like a good idea. (And, at the time v2 was started, I didn't know that these more complex cases would arise.) So, 'awk' might be a viable alternative, and perhaps I'll take a stab at it for fun at some point (or not), but I don't think there's a pressing need right now.
Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems
On Mon, Jul 30, 2018 at 8:27 AM, Nguyễn Thái Ngọc Duy wrote:
> Paths that only differ in case work fine in a case-sensitive
> filesystems, but if those repos are cloned in a case-insensitive one,
> you'll get problems. The first thing to notice is "git status" will
> never be clean with no indication what's exactly is "dirty".
"what" rather than "what's"?
> This patch helps the situation a bit by pointing out the problem at
> clone time. I have not suggested any way to work around or fix this
> problem. But I guess we could probably have a section in
> Documentation/ dedicated to this problem and point there instead of
> a long advice in this warning.
>
> Another thing we probably should do is catch in "git checkout" too,
> not just "git clone" since your linux/unix colleage colleague may
drop "colleage", keep "colleague"?
> accidentally add some files that your mac/windows machine is not very
> happy with. But then there's another problem, once the problem is
> known, we probably should stop spamming this warning at every
> checkout, but how?
Good questions. I have no answers.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> builtin/clone.c | 41 +
> 1 file changed, 41 insertions(+)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 5c439f1394..32738c2737 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -711,6 +711,33 @@ static void update_head(const struct ref *our, const
> struct ref *remote,
> }
> }
>
> +static void find_duplicate_icase_entries(struct index_state *istate,
> +struct string_list *dup)
> +{
> + struct string_list list = STRING_LIST_INIT_NODUP;
> + int i;
> +
> + for (i = 0; i < istate->cache_nr; i++)
> + string_list_append(, istate->cache[i]->name);
> +
> + list.cmp = fspathcmp;
> + string_list_sort();
So, you sort the list by fspathcmp to get the entries that differ in
case only next to each other. Makes sense...
> +
> + for (i = 1; i < list.nr; i++) {
> + const char *cur = list.items[i].string;
> + const char *prev = list.items[i - 1].string;
> +
> + if (dup->nr &&
> + !fspathcmp(cur, dup->items[dup->nr - 1].string)) {
> + string_list_append(dup, cur);
If we have at least one duplicate in dup (and currently we'd have to
have at least two), and we now hit yet another (i.e. a third or
fourth...) way of spelling the same path, then we add it.
> + } else if (!fspathcmp(cur, prev)) {
> + string_list_append(dup, prev);
> + string_list_append(dup, cur);
> + }
...otherwise, if we find a duplicate, we add both spellings to the dup list.
> + }
> + string_list_clear(, 0);
> +}
> +
> static int checkout(int submodule_progress)
> {
> struct object_id oid;
> @@ -761,6 +788,20 @@ static int checkout(int submodule_progress)
> if (write_locked_index(_index, _file, COMMIT_LOCK))
> die(_("unable to write new index file"));
>
> + if (ignore_case) {
> + struct string_list dup = STRING_LIST_INIT_DUP;
> + int i;
> +
> + find_duplicate_icase_entries(_index, );
> + if (dup.nr) {
> + warning(_("the following paths in this repository
> only differ in case and will\n"
Perhaps I'm being excessively pedantic, but what if there are multiple
pairs of paths differing in case? E.g. if someone has readme.txt,
README.txt, foo.txt, and FOO.txt, you'll list all four files but
readme.txt and foo.txt do not only differ in case.
Maybe something like "...only differ in case from another path and
will... " or is that too verbose and annoying?
> + "cause problems because you have cloned it
> on an case-insensitive filesytem:\n"));
> + for (i = 0; i < dup.nr; i++)
> + fprintf(stderr, "\t%s\n",
> dup.items[i].string);
> + }
> + string_list_clear(, 0);
> + }
> +
> err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
>oid_to_hex(), "1", NULL);
Is it worth attempting to also warn about paths that only differ in
UTF-normalization on relevant MacOS systems?
Re: [PATCH/RFC] clone: report duplicate entries on case-insensitive filesystems
On Mon, Jul 30, 2018 at 05:27:55PM +0200, Nguyễn Thái Ngọc Duy wrote:
> Paths that only differ in case work fine in a case-sensitive
> filesystems, but if those repos are cloned in a case-insensitive one,
> you'll get problems. The first thing to notice is "git status" will
> never be clean with no indication what's exactly is "dirty".
>
> This patch helps the situation a bit by pointing out the problem at
> clone time. I have not suggested any way to work around or fix this
> problem. But I guess we could probably have a section in
> Documentation/ dedicated to this problem and point there instead of
> a long advice in this warning.
>
> Another thing we probably should do is catch in "git checkout" too,
> not just "git clone" since your linux/unix colleage colleague may
> accidentally add some files that your mac/windows machine is not very
> happy with. But then there's another problem, once the problem is
> known, we probably should stop spamming this warning at every
> checkout, but how?
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> builtin/clone.c | 41 +
> 1 file changed, 41 insertions(+)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 5c439f1394..32738c2737 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -711,6 +711,33 @@ static void update_head(const struct ref *our, const
> struct ref *remote,
> }
> }
>
> +static void find_duplicate_icase_entries(struct index_state *istate,
> + struct string_list *dup)
> +{
> + struct string_list list = STRING_LIST_INIT_NODUP;
> + int i;
> +
> + for (i = 0; i < istate->cache_nr; i++)
> + string_list_append(, istate->cache[i]->name);
> +
> + list.cmp = fspathcmp;
> + string_list_sort();
> +
> + for (i = 1; i < list.nr; i++) {
> + const char *cur = list.items[i].string;
> + const char *prev = list.items[i - 1].string;
> +
> + if (dup->nr &&
> + !fspathcmp(cur, dup->items[dup->nr - 1].string)) {
> + string_list_append(dup, cur);
> + } else if (!fspathcmp(cur, prev)) {
> + string_list_append(dup, prev);
> + string_list_append(dup, cur);
> + }
> + }
> + string_list_clear(, 0);
> +}
> +
> static int checkout(int submodule_progress)
> {
> struct object_id oid;
> @@ -761,6 +788,20 @@ static int checkout(int submodule_progress)
> if (write_locked_index(_index, _file, COMMIT_LOCK))
> die(_("unable to write new index file"));
>
> + if (ignore_case) {
> + struct string_list dup = STRING_LIST_INIT_DUP;
> + int i;
> +
> + find_duplicate_icase_entries(_index, );
> + if (dup.nr) {
> + warning(_("the following paths in this repository only
> differ in case and will\n"
> + "cause problems because you have cloned it on
> an case-insensitive filesytem:\n"));
Thanks for the patch.
I wonder if we can tell the users more about the "problems"
and how to avoid them, or to live with them.
This is more loud thinking:
"The following paths only differ in case\n"
"One a case-insensitive file system only one at a time can be present\n"
"You may rename one like this:\n"
"git checkout && git mv .1\n"
> + fprintf(stderr, "\t%s\n", dup.items[i].string);
Another question:
Do we need any quote_path() here ?
(This may be overkill, since typically the repos with conflicting names
only use ASCII.)
> + }
> + string_list_clear(, 0);
> + }
> +
> err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
> oid_to_hex(), "1", NULL);
>
> --
> 2.18.0.656.gda699b98b3
>
Re: Question on range-diff and notes.displayref
Elijah Newren writes: > Should git notes show up in a range-diff? I happened to have > notes.displayref=refs/notes/amlog > set in my git.git repo, and saw the below in my range-diff: > > On Tue, Jul 31, 2018 at 10:12 AM, Elijah Newren wrote: >> 1: 4a1c9c3368 ! 1: 00f94a8b41 t1015: demonstrate directory/file conflict >> recovery failures >> @@ -14,7 +14,6 @@ >> >> Signed-off-by: Elijah Newren >> Signed-off-by: Junio C Hamano >> -Message-Id: <20180713163331.22446-2-new...@gmail.com> >> >> diff --git a/t/t1015-read-index-unmerged.sh >> b/t/t1015-read-index-unmerged.sh >> new file mode 100755 >> 2: e105e8bfbd ! 2: d3b8d7edb6 read-cache: fix directory/file conflict >> handling in read_index_unmerged() >> @@ -59,7 +59,6 @@ >> >> Signed-off-by: Elijah Newren >> Signed-off-by: Junio C Hamano >> -Message-Id: <20180713163331.22446-3-new...@gmail.com> >> > > > Maybe this is expected or wanted (tbdiff also shows the git notes for > what it's worth), but it seemed somewhat surprising to me. I'd rather > not see such "differences" displayed for the patch series that I'm > submitting, but perhaps others see it differently? I was surprised to see that, too, but if you have it configured I would understand the behaviour. Of course, the value of showing the Message-Id: in this particular case is dubious (by definition you won't have them on the side that you have not posted), but if you are comparing two iterations that have been queued (e.g. by keeping the old tips of the topic branch unpruned by holding onto historical 'origin/pu' using reflog), it may become useful when doing a "Wait, is that really my bogosity? Let's go see the list archive using the message ID---ah, something was suggested in the review and the maintainer attempted to pick it up and squash it in, which he botched".
Re: [PATCH v2 10/10] fetch: stop clobbering existing tags without --force
Ævar Arnfjörð Bjarmason writes: > diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt > index 97d3217df9..5b624caf58 100644 > --- a/Documentation/fetch-options.txt > +++ b/Documentation/fetch-options.txt > @@ -49,11 +49,16 @@ endif::git-pull[] > > -f:: > --force:: > - When 'git fetch' is used with `:` > - refspec, it refuses to update the local branch > - `` unless the remote branch `` it > - fetches is a descendant of ``. This option > - overrides that check. > + When 'git fetch' is used with `:` refspec it may > + refuse to update the local branch as discussed > +ifdef::git-pull[] > + in the `` part of the linkgit:git-fetch[1] > + documentation. > +endif::git-pull[] > +ifndef::git-pull[] > + in the `` part below. > +endif::git-pull[] > + This option overrides that check. Ah, that's tricky. I could not locate "the `` part" in Documentation/git-fetch.txt and was scratching my head, but it comes from pull-fetch-param.txt by inclusion ;-) > diff --git a/Documentation/pull-fetch-param.txt > b/Documentation/pull-fetch-param.txt > index f1fb08dc68..acb8e1a4f0 100644 > --- a/Documentation/pull-fetch-param.txt > +++ b/Documentation/pull-fetch-param.txt > @@ -33,11 +33,21 @@ name. > it requests fetching everything up to the given tag. > + > The remote ref that matches > -is fetched, and if is not an empty string, the local > -ref that matches it is fast-forwarded using . > -If the optional plus `+` is used, the local ref > -is updated even if it does not result in a fast-forward > -update. > +is fetched, and if is not an empty string, an attempt > +is made to update the local ref that matches it. > ++ > +Whether that update is allowed without `--force` depends on the ref > +namespace it's being fetched to, and the type of object being > +fetched. If it's a commit under `refs/heads/*` only fast-forwards are > +allowed. > ++ > +By having the optional leading `+` to a refspec (or using `--force` > +command line option) you can tell Git to update the local ref even if > +it is not allowed by its respective namespace clobbering rules. The above two paragraphs imply that I can "fetch +blob:refs/heads/master" to cause havoc locally? > +Before Git version 2.19 tag objects under `refs/tags/*` would not be > +protected from updates, but since then the `+` (or `--force`) syntax > +is required to clobber them. I think that is a good change; it belongs more to the b/c notes in the release notes; while I do not think it is wrong describe "it used to be that way" just after a drastic change in the immediate past, we shouldn't carry that forever, so perhaps we can leave a "NEEDSWORK: remove the 'it used to be this way' in 2020" comment around here?
Re: [PATCH] negotiator/skipping: skip commits during fetch
> > +fetch.negotiationAlgorithm::
> > + Control how information about the commits in the local repository is
> > + sent when negotiating the contents of the packfile to be sent by the
> > + server. Set to "skipping" to use an algorithm that skips commits in an
> > + effort to converge faster, but may result in a larger-than-necessary
> > + packfile; any other value instructs Git to use the default algorithm
> > + that never skips commits (unless the server has acknowledged it or one
> > + of its descendants).
> > +
>
> ...let's instead document that there's just the values "skipping" and
> "default", and say "default" is provided by default, and perhaps change
> the code to warn about anything that isn't those two.
>
> Then we're not painting ourselves into a corner by needing to break a
> promise in the docs ("any other value instructs Git to use the default")
> if we add a new one of these, and aren't silently falling back on the
> default if we add new-fancy-algo the user's version doesn't know about.
My intention was to allow future versions of Git to introduce more
algorithms, but have older versions of Git still work even if a
repository is configured to use a newer algorithm. But your suggestion
is reasonable too.
> Now, running that "git fetch --all" takes ages, and I know why. It's
> because the in the negotiation for "git fetch some/small-repo" I'm
> emitting hundreds of thousands of "have" lines for SHA1s found in other
> unrelated repos, only to get a NAK for all of them.
>
> One way to fix that with this facility would be to have some way to pass
> in arguments, similar to what we have for merge drivers, so I can say
> "just emit 'have' lines for stuff found in this branch". The most
> pathological cases are when I'm fetching a remote that has one commit,
> and I'm desperately trying to find something in common by asking if the
> remote has hundreds of K of commits it has no chance of having.
>
> Or there may be some smarter way to do this, what do you think?
Well, there is already a commit in "next" that does this :-)
3390e42adb ("fetch-pack: support negotiation tip whitelist", 2018-07-03)
[GSoC][PATCH v5 20/20] rebase -i: move rebase--helper modes to rebase--interactive
This moves the rebase--helper modes still used by
git-rebase--preserve-merges.sh (`--shorten-ids`, `--expand-ids`,
`--check-todo-list`, `--rearrange-squash` and `--add-exec-commands`) to
rebase--interactive.c.
git-rebase--preserve-merges.sh is modified accordingly, and
rebase--helper.c is removed as it is useless now.
Signed-off-by: Alban Gruin
---
.gitignore | 1 -
Makefile | 1 -
builtin.h | 1 -
builtin/rebase--helper.c | 226 -
builtin/rebase--interactive.c | 27 +++-
git-rebase--preserve-merges.sh | 10 +-
git.c | 1 -
7 files changed, 31 insertions(+), 236 deletions(-)
delete mode 100644 builtin/rebase--helper.c
diff --git a/.gitignore b/.gitignore
index 3284a1e9b1..406f26d050 100644
--- a/.gitignore
+++ b/.gitignore
@@ -116,7 +116,6 @@
/git-read-tree
/git-rebase
/git-rebase--am
-/git-rebase--helper
/git-rebase--interactive
/git-rebase--merge
/git-rebase--preserve-merges
diff --git a/Makefile b/Makefile
index 584834726d..ca3a0888dd 100644
--- a/Makefile
+++ b/Makefile
@@ -1059,7 +1059,6 @@ BUILTIN_OBJS += builtin/prune.o
BUILTIN_OBJS += builtin/pull.o
BUILTIN_OBJS += builtin/push.o
BUILTIN_OBJS += builtin/read-tree.o
-BUILTIN_OBJS += builtin/rebase--helper.o
BUILTIN_OBJS += builtin/rebase--interactive.o
BUILTIN_OBJS += builtin/receive-pack.o
BUILTIN_OBJS += builtin/reflog.o
diff --git a/builtin.h b/builtin.h
index ed89b4f495..7feb689d87 100644
--- a/builtin.h
+++ b/builtin.h
@@ -203,7 +203,6 @@ extern int cmd_pull(int argc, const char **argv, const char
*prefix);
extern int cmd_push(int argc, const char **argv, const char *prefix);
extern int cmd_read_tree(int argc, const char **argv, const char *prefix);
extern int cmd_rebase__interactive(int argc, const char **argv, const char
*prefix);
-extern int cmd_rebase__helper(int argc, const char **argv, const char *prefix);
extern int cmd_receive_pack(int argc, const char **argv, const char *prefix);
extern int cmd_reflog(int argc, const char **argv, const char *prefix);
extern int cmd_remote(int argc, const char **argv, const char *prefix);
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
deleted file mode 100644
index ac21e8e06e..00
--- a/builtin/rebase--helper.c
+++ /dev/null
@@ -1,226 +0,0 @@
-#include "builtin.h"
-#include "cache.h"
-#include "config.h"
-#include "parse-options.h"
-#include "sequencer.h"
-#include "rebase-interactive.h"
-#include "argv-array.h"
-#include "refs.h"
-#include "rerere.h"
-#include "alias.h"
-
-static GIT_PATH_FUNC(path_state_dir, "rebase-merge/")
-static GIT_PATH_FUNC(path_squash_onto, "rebase-merge/squash-onto")
-static GIT_PATH_FUNC(path_interactive, "rebase-merge/interactive")
-
-static int get_revision_ranges(const char *upstream, const char *onto,
- const char **head_hash,
- char **revisions, char **shortrevisions)
-{
- const char *base_rev = upstream ? upstream : onto;
- struct object_id orig_head;
-
- if (get_oid("HEAD", _head))
- return error(_("no HEAD?"));
-
- *head_hash = find_unique_abbrev(_head, GIT_MAX_HEXSZ);
-
- if (revisions)
- *revisions = xstrfmt("%s...%s", base_rev, *head_hash);
- if (shortrevisions) {
- const char *shorthead;
-
- shorthead = find_unique_abbrev(_head, DEFAULT_ABBREV);
-
- if (upstream) {
- const char *shortrev;
- struct object_id rev_oid;
-
- get_oid(base_rev, _oid);
- shortrev = find_unique_abbrev(_oid, DEFAULT_ABBREV);
-
- *shortrevisions = xstrfmt("%s..%s", shortrev,
shorthead);
- } else
- *shortrevisions = xstrdup(shorthead);
- }
-
- return 0;
-}
-
-static int init_basic_state(struct replay_opts *opts, const char *head_name,
- const char *onto, const char *orig_head)
-{
- FILE *interactive;
-
- if (!is_directory(path_state_dir()) &&
mkdir_in_gitdir(path_state_dir()))
- return error_errno(_("could not create temporary %s"),
path_state_dir());
-
- delete_reflog("REBASE_HEAD");
-
- interactive = fopen(path_interactive(), "w");
- if (!interactive)
- return error_errno(_("could not mark as interactive"));
- fclose(interactive);
-
- return write_basic_state(opts, head_name, onto, orig_head);
-}
-
-static const char * const builtin_rebase_helper_usage[] = {
- N_("git rebase--helper []"),
- NULL
-};
-
-int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
-{
- struct replay_opts opts = REPLAY_OPTS_INIT;
- unsigned flags = 0, keep_empty = 0, rebase_merges = 0, autosquash = 0;
- int abbreviate_commands = 0, rebase_cousins = -1, ret;
- const
[GSoC][PATCH v5 13/20] rebase -i: implement the logic to initialize $revisions in C
This rewrites the part of init_revisions_and_shortrevisions() needed by
`--make-script` from shell to C. The new version is called
get_revision_ranges(), and is a static function inside of
rebase--helper.c. As this does not initialize $shortrevision, the
original shell version is not yet stripped.
Unlike init_revisions_and_shortrevisions(), get_revision_ranges()
doesn’t write $squash_onto to the state directory, it’s done by the
handler of `--make-script` instead.
Finally, this drops the $revision argument passed to `--make-script` in
git-rebase--interactive.sh, and rebase--helper is changed accordingly.
Signed-off-by: Alban Gruin
---
No changes since v4.
builtin/rebase--helper.c | 56 --
git-rebase--interactive.sh | 4 ++-
2 files changed, 56 insertions(+), 4 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 6085527b2b..15fa556f65 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -4,6 +4,25 @@
#include "parse-options.h"
#include "sequencer.h"
#include "rebase-interactive.h"
+#include "argv-array.h"
+
+static GIT_PATH_FUNC(path_squash_onto, "rebase-merge/squash-onto")
+
+static int get_revision_ranges(const char *upstream, const char *onto,
+ const char **head_hash,
+ char **revisions)
+{
+ const char *base_rev = upstream ? upstream : onto;
+ struct object_id orig_head;
+
+ if (get_oid("HEAD", _head))
+ return error(_("no HEAD?"));
+
+ *head_hash = find_unique_abbrev(_head, GIT_MAX_HEXSZ);
+ *revisions = xstrfmt("%s...%s", base_rev, *head_hash);
+
+ return 0;
+}
static const char * const builtin_rebase_helper_usage[] = {
N_("git rebase--helper []"),
@@ -14,7 +33,9 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
{
struct replay_opts opts = REPLAY_OPTS_INIT;
unsigned flags = 0, keep_empty = 0, rebase_merges = 0, autosquash = 0;
- int abbreviate_commands = 0, rebase_cousins = -1;
+ int abbreviate_commands = 0, rebase_cousins = -1, ret;
+ const char *head_hash = NULL, *onto = NULL, *restrict_revision = NULL,
+ *squash_onto = NULL, *upstream = NULL;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, REARRANGE_SQUASH, ADD_EXEC, EDIT_TODO,
PREPARE_BRANCH,
@@ -54,6 +75,13 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
N_("prepare the branch to be rebased"),
PREPARE_BRANCH),
OPT_CMDMODE(0, "complete-action", ,
N_("complete the action"), COMPLETE_ACTION),
+ OPT_STRING(0, "onto", , N_("onto"), N_("onto")),
+ OPT_STRING(0, "restrict-revision", _revision,
+ N_("restrict-revision"), N_("restrict revision")),
+ OPT_STRING(0, "squash-onto", _onto, N_("squash-onto"),
+ N_("squash onto")),
+ OPT_STRING(0, "upstream", , N_("upstream"),
+ N_("the upstream commit")),
OPT_END()
};
@@ -81,8 +109,30 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
return !!sequencer_continue();
if (command == ABORT && argc == 1)
return !!sequencer_remove_state();
- if (command == MAKE_SCRIPT && argc > 1)
- return !!sequencer_make_script(stdout, argc, argv, flags);
+ if (command == MAKE_SCRIPT && argc == 1) {
+ char *revisions = NULL;
+ struct argv_array make_script_args = ARGV_ARRAY_INIT;
+
+ if (!upstream && squash_onto)
+ write_file(path_squash_onto(), "%s\n", squash_onto);
+
+ ret = get_revision_ranges(upstream, onto, _hash,
);
+ if (ret)
+ return ret;
+
+ argv_array_pushl(_script_args, "", revisions, NULL);
+ if (restrict_revision)
+ argv_array_push(_script_args, restrict_revision);
+
+ ret = sequencer_make_script(stdout,
+ make_script_args.argc,
make_script_args.argv,
+ flags);
+
+ free(revisions);
+ argv_array_clear(_script_args);
+
+ return !!ret;
+ }
if ((command == SHORTEN_OIDS || command == EXPAND_OIDS) && argc == 1)
return !!transform_todos(flags);
if (command == CHECK_TODO_LIST && argc == 1)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 0d66c0f8b8..4ca47aed1e 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -92,7 +92,9 @@ git_rebase__interactive () {
git rebase--helper --make-script ${keep_empty:+--keep-empty} \
[GSoC][PATCH v5 14/20] rebase -i: rewrite the rest of init_revisions_and_shortrevisions() in C
This rewrites the part of init_revisions_and_shortrevisions() needed by
`--complete-action` (which initialize $shortrevisions) from shell to C.
When `upstream` is empty, it means that the user launched a `rebase
--root`, and `onto` contains the ID of an empty commit. As a range
between an empty commit and `head` is not really meaningful, `onto` is
not used to initialize `shortrevisions` in this case.
The corresponding arguments passed to `--complete-action` are then
dropped, and init_revisions_and_shortrevisions() is stripped from
git-rebase--interactive.sh
Signed-off-by: Alban Gruin
---
No changes since v4.
builtin/rebase--helper.c | 40 --
git-rebase--interactive.sh | 27 -
2 files changed, 38 insertions(+), 29 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 15fa556f65..c4a4c5cfbb 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -10,7 +10,7 @@ static GIT_PATH_FUNC(path_squash_onto,
"rebase-merge/squash-onto")
static int get_revision_ranges(const char *upstream, const char *onto,
const char **head_hash,
- char **revisions)
+ char **revisions, char **shortrevisions)
{
const char *base_rev = upstream ? upstream : onto;
struct object_id orig_head;
@@ -19,7 +19,25 @@ static int get_revision_ranges(const char *upstream, const
char *onto,
return error(_("no HEAD?"));
*head_hash = find_unique_abbrev(_head, GIT_MAX_HEXSZ);
- *revisions = xstrfmt("%s...%s", base_rev, *head_hash);
+
+ if (revisions)
+ *revisions = xstrfmt("%s...%s", base_rev, *head_hash);
+ if (shortrevisions) {
+ const char *shorthead;
+
+ shorthead = find_unique_abbrev(_head, DEFAULT_ABBREV);
+
+ if (upstream) {
+ const char *shortrev;
+ struct object_id rev_oid;
+
+ get_oid(base_rev, _oid);
+ shortrev = find_unique_abbrev(_oid, DEFAULT_ABBREV);
+
+ *shortrevisions = xstrfmt("%s..%s", shortrev,
shorthead);
+ } else
+ *shortrevisions = xstrdup(shorthead);
+ }
return 0;
}
@@ -116,7 +134,7 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
if (!upstream && squash_onto)
write_file(path_squash_onto(), "%s\n", squash_onto);
- ret = get_revision_ranges(upstream, onto, _hash,
);
+ ret = get_revision_ranges(upstream, onto, _hash,
, NULL);
if (ret)
return ret;
@@ -145,9 +163,19 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
return !!edit_todo_list(flags);
if (command == PREPARE_BRANCH && argc == 2)
return !!prepare_branch_to_be_rebased(, argv[1]);
- if (command == COMPLETE_ACTION && argc == 6)
- return !!complete_action(, flags, argv[1], argv[2],
argv[3],
-argv[4], argv[5], autosquash);
+ if (command == COMPLETE_ACTION && argc == 3) {
+ char *shortrevisions = NULL;
+
+ ret = get_revision_ranges(upstream, onto, _hash, NULL,
);
+ if (ret)
+ return ret;
+
+ ret = complete_action(, flags, shortrevisions, argv[1],
onto,
+ head_hash, argv[2], autosquash);
+
+ free(shortrevisions);
+ return !!ret;
+ }
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 4ca47aed1e..08e9a21c2f 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -60,23 +60,6 @@ init_basic_state () {
write_basic_state
}
-init_revisions_and_shortrevisions () {
- shorthead=$(git rev-parse --short $orig_head)
- shortonto=$(git rev-parse --short $onto)
- if test -z "$rebase_root"
- # this is now equivalent to ! -z "$upstream"
- then
- shortupstream=$(git rev-parse --short $upstream)
- revisions=$upstream...$orig_head
- shortrevisions=$shortupstream..$shorthead
- else
- revisions=$onto...$orig_head
- shortrevisions=$shorthead
- test -z "$squash_onto" ||
- echo "$squash_onto" >"$state_dir"/squash-onto
- fi
-}
-
git_rebase__interactive () {
initiate_action "$action"
ret=$?
@@ -87,8 +70,6 @@ git_rebase__interactive () {
git rebase--helper --prepare-branch "$switch_to" ${verbose:+--verbose}
init_basic_state
- init_revisions_and_shortrevisions
-
git rebase--helper --make-script
[GSoC][PATCH v5 18/20] rebase--interactive2: rewrite the submodes of interactive rebase in C
This rewrites the submodes of interactive rebase (`--continue`,
`--skip`, `--edit-todo`, and `--show-current-patch`) in C.
git-rebase.sh is then modified to call directly git-rebase--interactive2
instead of git-rebase--interactive.sh.
Signed-off-by: Alban Gruin
---
No changes since v4.
builtin/rebase--interactive2.c | 47 +++---
git-rebase.sh | 45 +---
2 files changed, 84 insertions(+), 8 deletions(-)
diff --git a/builtin/rebase--interactive2.c b/builtin/rebase--interactive2.c
index e89ef71499..53b4f7483d 100644
--- a/builtin/rebase--interactive2.c
+++ b/builtin/rebase--interactive2.c
@@ -134,11 +134,14 @@ int cmd_rebase__interactive(int argc, const char **argv,
const char *prefix)
{
struct replay_opts opts = REPLAY_OPTS_INIT;
unsigned flags = 0, keep_empty = 0, rebase_merges = 0, autosquash = 0;
- int abbreviate_commands = 0, rebase_cousins = -1;
+ int abbreviate_commands = 0, rebase_cousins = -1, ret = 0;
const char *onto = NULL, *onto_name = NULL, *restrict_revision = NULL,
*squash_onto = NULL, *upstream = NULL, *head_name = NULL,
*switch_to = NULL, *cmd = NULL;
char *raw_strategies = NULL;
+ enum {
+ NONE = 0, CONTINUE, SKIP, EDIT_TODO, SHOW_CURRENT_PATCH
+ } command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
OPT_BOOL(0, "keep-empty", _empty, N_("keep empty
commits")),
@@ -151,6 +154,13 @@ int cmd_rebase__interactive(int argc, const char **argv,
const char *prefix)
N_("move commits that begin with squash!/fixup!")),
OPT_BOOL(0, "signoff", , N_("sign commits")),
OPT__VERBOSE(, N_("be verbose")),
+ OPT_CMDMODE(0, "continue", , N_("continue rebase"),
+ CONTINUE),
+ OPT_CMDMODE(0, "skip", , N_("skip commit"), SKIP),
+ OPT_CMDMODE(0, "edit-todo", , N_("edit the todo list"),
+ EDIT_TODO),
+ OPT_CMDMODE(0, "show-current-patch", , N_("show the
current patch"),
+ SHOW_CURRENT_PATCH),
OPT_STRING(0, "onto", , N_("onto"), N_("onto")),
OPT_STRING(0, "restrict-revision", _revision,
N_("restrict-revision"), N_("restrict revision")),
@@ -194,7 +204,36 @@ int cmd_rebase__interactive(int argc, const char **argv,
const char *prefix)
warning(_("--[no-]rebase-cousins has no effect without "
"--rebase-merges"));
- return !!do_interactive_rebase(, flags, switch_to, upstream, onto,
- onto_name, squash_onto, head_name,
restrict_revision,
- raw_strategies, cmd, autosquash);
+ switch (command) {
+ case NONE:
+ ret = do_interactive_rebase(, flags, switch_to, upstream,
onto,
+ onto_name, squash_onto, head_name,
restrict_revision,
+ raw_strategies, cmd, autosquash);
+ break;
+ case SKIP: {
+ struct string_list merge_rr = STRING_LIST_INIT_DUP;
+
+ rerere_clear(_rr);
+ /* fallthrough */
+ case CONTINUE:
+ ret = sequencer_continue();
+ break;
+ }
+ case EDIT_TODO:
+ ret = edit_todo_list(flags);
+ break;
+ case SHOW_CURRENT_PATCH: {
+ struct child_process cmd = CHILD_PROCESS_INIT;
+
+ cmd.git_cmd = 1;
+ argv_array_pushl(, "show", "REBASE_HEAD", "--", NULL);
+ ret = run_command();
+
+ break;
+ }
+ default:
+ BUG("invalid command '%d'", command);
+ }
+
+ return !!ret;
}
diff --git a/git-rebase.sh b/git-rebase.sh
index 51a6db7daa..d5950a3012 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -200,19 +200,56 @@ finish_rebase () {
rm -rf "$state_dir"
}
+run_interactive () {
+ GIT_CHERRY_PICK_HELP="$resolvemsg"
+ export GIT_CHERRY_PICK_HELP
+
+ test -n "$keep_empty" && keep_empty="--keep-empty"
+ test -n "$rebase_merges" && rebase_merges="--rebase-merges"
+ test -n "$rebase_cousins" && rebase_cousins="--rebase-cousins"
+ test -n "$autosquash" && autosquash="--autosquash"
+ test -n "$verbose" && verbose="--verbose"
+ test -n "$force_rebase" && force_rebase="--no-ff"
+ test -n "$restrict_revisions" && \
+ restrict_revisions="--restrict-revisions=^$restrict_revisions"
+ test -n "$upstream" && upstream="--upstream=$upstream"
+ test -n "$onto" && onto="--onto=$onto"
+ test -n "$squash_onto" && squash_onto="--squash-onto=$squash_onto"
+ test -n "$onto_name" &&
[GSoC][PATCH v5 16/20] rebase -i: rewrite init_basic_state() in C
This rewrites init_basic_state() from shell to C. The call to
write_basic_state() in cmd_rebase__helper() is replaced by a call to the
new function.
The shell version is then stripped from git-rebase--interactive.sh.
Signed-off-by: Alban Gruin
---
No changes since v4.
builtin/rebase--helper.c | 23 ++-
git-rebase--interactive.sh | 9 -
2 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 06fe3c018b..ac21e8e06e 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -5,10 +5,13 @@
#include "sequencer.h"
#include "rebase-interactive.h"
#include "argv-array.h"
+#include "refs.h"
#include "rerere.h"
#include "alias.h"
+static GIT_PATH_FUNC(path_state_dir, "rebase-merge/")
static GIT_PATH_FUNC(path_squash_onto, "rebase-merge/squash-onto")
+static GIT_PATH_FUNC(path_interactive, "rebase-merge/interactive")
static int get_revision_ranges(const char *upstream, const char *onto,
const char **head_hash,
@@ -44,6 +47,24 @@ static int get_revision_ranges(const char *upstream, const
char *onto,
return 0;
}
+static int init_basic_state(struct replay_opts *opts, const char *head_name,
+ const char *onto, const char *orig_head)
+{
+ FILE *interactive;
+
+ if (!is_directory(path_state_dir()) &&
mkdir_in_gitdir(path_state_dir()))
+ return error_errno(_("could not create temporary %s"),
path_state_dir());
+
+ delete_reflog("REBASE_HEAD");
+
+ interactive = fopen(path_interactive(), "w");
+ if (!interactive)
+ return error_errno(_("could not mark as interactive"));
+ fclose(interactive);
+
+ return write_basic_state(opts, head_name, onto, orig_head);
+}
+
static const char * const builtin_rebase_helper_usage[] = {
N_("git rebase--helper []"),
NULL
@@ -198,7 +219,7 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
if (ret)
return ret;
- return !!write_basic_state(, head_name, onto, head_hash);
+ return !!init_basic_state(, head_name, onto, head_hash);
}
usage_with_options(builtin_rebase_helper_usage, options);
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 6367da66e2..761be95ed1 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -51,14 +51,6 @@ initiate_action () {
esac
}
-init_basic_state () {
- orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")"
- mkdir -p "$state_dir" || die "$(eval_gettext "Could not create
temporary \$state_dir")"
- rm -f "$(git rev-parse --git-path REBASE_HEAD)"
-
- : > "$state_dir"/interactive || die "$(gettext "Could not mark as
interactive")"
-}
-
git_rebase__interactive () {
initiate_action "$action"
ret=$?
@@ -67,7 +59,6 @@ git_rebase__interactive () {
fi
git rebase--helper --prepare-branch "$switch_to" ${verbose:+--verbose}
- init_basic_state
git rebase--helper --init-basic-state ${upstream:+--upstream
"$upstream"} \
${onto:+--onto "$onto"} ${head_name:+--head-name "$head_name"} \
--
2.18.0
[GSoC][PATCH v5 19/20] rebase -i: remove git-rebase--interactive.sh
This removes git-rebase--interactive.sh, as its functionnality has been
replaced by git-rebase--interactive2.
git-rebase--interactive2.c is then renamed to git-rebase--interactive.c.
Signed-off-by: Alban Gruin
---
No changes since v4.
.gitignore| 1 -
Makefile | 4 +-
...--interactive2.c => rebase--interactive.c} | 0
git-rebase--interactive.sh| 84 ---
git-rebase.sh | 2 +-
git.c | 2 +-
6 files changed, 3 insertions(+), 90 deletions(-)
rename builtin/{rebase--interactive2.c => rebase--interactive.c} (100%)
delete mode 100644 git-rebase--interactive.sh
diff --git a/.gitignore b/.gitignore
index 404c9a8472..3284a1e9b1 100644
--- a/.gitignore
+++ b/.gitignore
@@ -118,7 +118,6 @@
/git-rebase--am
/git-rebase--helper
/git-rebase--interactive
-/git-rebase--interactive2
/git-rebase--merge
/git-rebase--preserve-merges
/git-receive-pack
diff --git a/Makefile b/Makefile
index 71f8f45fe5..584834726d 100644
--- a/Makefile
+++ b/Makefile
@@ -619,7 +619,6 @@ SCRIPT_SH += git-web--browse.sh
SCRIPT_LIB += git-mergetool--lib
SCRIPT_LIB += git-parse-remote
SCRIPT_LIB += git-rebase--am
-SCRIPT_LIB += git-rebase--interactive
SCRIPT_LIB += git-rebase--preserve-merges
SCRIPT_LIB += git-rebase--merge
SCRIPT_LIB += git-sh-setup
@@ -1060,8 +1059,8 @@ BUILTIN_OBJS += builtin/prune.o
BUILTIN_OBJS += builtin/pull.o
BUILTIN_OBJS += builtin/push.o
BUILTIN_OBJS += builtin/read-tree.o
-BUILTIN_OBJS += builtin/rebase--interactive2.o
BUILTIN_OBJS += builtin/rebase--helper.o
+BUILTIN_OBJS += builtin/rebase--interactive.o
BUILTIN_OBJS += builtin/receive-pack.o
BUILTIN_OBJS += builtin/reflog.o
BUILTIN_OBJS += builtin/remote.o
@@ -2400,7 +2399,6 @@ XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --language=Perl \
LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
LOCALIZED_SH = $(SCRIPT_SH)
LOCALIZED_SH += git-parse-remote.sh
-LOCALIZED_SH += git-rebase--interactive.sh
LOCALIZED_SH += git-rebase--preserve-merges.sh
LOCALIZED_SH += git-sh-setup.sh
LOCALIZED_PERL = $(SCRIPT_PERL)
diff --git a/builtin/rebase--interactive2.c b/builtin/rebase--interactive.c
similarity index 100%
rename from builtin/rebase--interactive2.c
rename to builtin/rebase--interactive.c
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
deleted file mode 100644
index e87d708a4d..00
--- a/git-rebase--interactive.sh
+++ /dev/null
@@ -1,84 +0,0 @@
-# This shell script fragment is sourced by git-rebase to implement
-# its interactive mode. "git rebase --interactive" makes it easy
-# to fix up commits in the middle of a series and rearrange commits.
-#
-# Copyright (c) 2006 Johannes E. Schindelin
-#
-# The original idea comes from Eric W. Biederman, in
-# https://public-inbox.org/git/m1odwkyuf5.fsf...@ebiederm.dsl.xmission.com/
-#
-# 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.
-todo="$state_dir"/git-rebase-todo
-
-GIT_CHERRY_PICK_HELP="$resolvemsg"
-export GIT_CHERRY_PICK_HELP
-
-# Initiate an action. If the cannot be any
-# further action it may exec a command
-# or exit and not return.
-#
-# TODO: Consider a cleaner return model so it
-# never exits and always return 0 if process
-# is complete.
-#
-# Parameter 1 is the action to initiate.
-#
-# Returns 0 if the action was able to complete
-# and if 1 if further processing is required.
-initiate_action () {
- case "$1" in
- continue)
- exec git rebase--helper ${force_rebase:+--no-ff}
$allow_empty_message \
---continue
- ;;
- skip)
- git rerere clear
- exec git rebase--helper ${force_rebase:+--no-ff}
$allow_empty_message \
---continue
- ;;
- edit-todo)
- exec git rebase--helper --edit-todo
- ;;
- show-current-patch)
- exec git show REBASE_HEAD --
- ;;
- *)
- return 1 # continue
- ;;
- esac
-}
-
-git_rebase__interactive () {
- initiate_action "$action"
- ret=$?
- if test $ret = 0; then
- return 0
- fi
-
- test -n "$keep_empty" && keep_empty="--keep-empty"
- test -n "$rebase_merges" && rebase_merges="--rebase-merges"
- test -n "$rebase_cousins" && rebase_cousins="--rebase-cousins"
- test -n "$autosquash" && autosquash="--autosquash"
- test -n "$verbose" && verbose="--verbose"
- test -n "$force_rebase" && force_rebase="--no-ff"
- test -n "$restrict_revisions" &&
restrict_revisions="--restrict-revisions=^$restrict_revisions"
- test -n "$upstream" &&
[GSoC][PATCH v5 17/20] rebase -i: implement the main part of interactive rebase as a builtin
This rewrites the part of interactive rebase which initializes the
basic state, make the script and complete the action, as a buitin, named
git-rebase--interactive2 for now. Others modes (`--continue`,
`--edit-todo`, etc.) will be rewritten in the next commit.
git-rebase--interactive.sh is modified to call git-rebase--interactive2
instead of git-rebase--helper.
Signed-off-by: Alban Gruin
---
No changes since v4.
.gitignore | 1 +
Makefile | 1 +
builtin.h | 1 +
builtin/rebase--interactive2.c | 200 +
git-rebase--interactive.sh | 41 +++
git.c | 1 +
6 files changed, 226 insertions(+), 19 deletions(-)
create mode 100644 builtin/rebase--interactive2.c
diff --git a/.gitignore b/.gitignore
index 3284a1e9b1..404c9a8472 100644
--- a/.gitignore
+++ b/.gitignore
@@ -118,6 +118,7 @@
/git-rebase--am
/git-rebase--helper
/git-rebase--interactive
+/git-rebase--interactive2
/git-rebase--merge
/git-rebase--preserve-merges
/git-receive-pack
diff --git a/Makefile b/Makefile
index 909a687857..71f8f45fe5 100644
--- a/Makefile
+++ b/Makefile
@@ -1060,6 +1060,7 @@ BUILTIN_OBJS += builtin/prune.o
BUILTIN_OBJS += builtin/pull.o
BUILTIN_OBJS += builtin/push.o
BUILTIN_OBJS += builtin/read-tree.o
+BUILTIN_OBJS += builtin/rebase--interactive2.o
BUILTIN_OBJS += builtin/rebase--helper.o
BUILTIN_OBJS += builtin/receive-pack.o
BUILTIN_OBJS += builtin/reflog.o
diff --git a/builtin.h b/builtin.h
index 0362f1ce25..ed89b4f495 100644
--- a/builtin.h
+++ b/builtin.h
@@ -202,6 +202,7 @@ extern int cmd_prune_packed(int argc, const char **argv,
const char *prefix);
extern int cmd_pull(int argc, const char **argv, const char *prefix);
extern int cmd_push(int argc, const char **argv, const char *prefix);
extern int cmd_read_tree(int argc, const char **argv, const char *prefix);
+extern int cmd_rebase__interactive(int argc, const char **argv, const char
*prefix);
extern int cmd_rebase__helper(int argc, const char **argv, const char *prefix);
extern int cmd_receive_pack(int argc, const char **argv, const char *prefix);
extern int cmd_reflog(int argc, const char **argv, const char *prefix);
diff --git a/builtin/rebase--interactive2.c b/builtin/rebase--interactive2.c
new file mode 100644
index 00..e89ef71499
--- /dev/null
+++ b/builtin/rebase--interactive2.c
@@ -0,0 +1,200 @@
+#include "builtin.h"
+#include "cache.h"
+#include "config.h"
+#include "parse-options.h"
+#include "sequencer.h"
+#include "rebase-interactive.h"
+#include "argv-array.h"
+#include "refs.h"
+#include "rerere.h"
+#include "run-command.h"
+
+static GIT_PATH_FUNC(path_state_dir, "rebase-merge/")
+static GIT_PATH_FUNC(path_squash_onto, "rebase-merge/squash-onto")
+static GIT_PATH_FUNC(path_interactive, "rebase-merge/interactive")
+
+static int get_revision_ranges(const char *upstream, const char *onto,
+ const char **head_hash,
+ char **revisions, char **shortrevisions)
+{
+ const char *base_rev = upstream ? upstream : onto, *shorthead;
+ struct object_id orig_head;
+
+ if (get_oid("HEAD", _head))
+ return error(_("no HEAD?"));
+
+ *head_hash = find_unique_abbrev(_head, GIT_MAX_HEXSZ);
+ *revisions = xstrfmt("%s...%s", base_rev, *head_hash);
+
+ shorthead = find_unique_abbrev(_head, DEFAULT_ABBREV);
+
+ if (upstream) {
+ const char *shortrev;
+ struct object_id rev_oid;
+
+ get_oid(base_rev, _oid);
+ shortrev = find_unique_abbrev(_oid, DEFAULT_ABBREV);
+
+ *shortrevisions = xstrfmt("%s..%s", shortrev, shorthead);
+ } else
+ *shortrevisions = xstrdup(shorthead);
+
+ return 0;
+}
+
+static int init_basic_state(struct replay_opts *opts, const char *head_name,
+ const char *onto, const char *orig_head)
+{
+ FILE *interactive;
+
+ if (!is_directory(path_state_dir()) &&
mkdir_in_gitdir(path_state_dir()))
+ return error_errno(_("could not create temporary %s"),
path_state_dir());
+
+ delete_reflog("REBASE_HEAD");
+
+ interactive = fopen(path_interactive(), "w");
+ if (!interactive)
+ return error_errno(_("could not mark as interactive"));
+ fclose(interactive);
+
+ return write_basic_state(opts, head_name, onto, orig_head);
+}
+
+static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
+const char *switch_to, const char *upstream,
+const char *onto, const char *onto_name,
+const char *squash_onto, const char *head_name,
+const char *restrict_revision, char
*raw_strategies,
+const char *cmd, unsigned autosquash)
+{
+ int ret;
+
[GSoC][PATCH v5 15/20] rebase -i: rewrite write_basic_state() in C
This rewrites write_basic_state() from git-rebase.sh in C. This is the
first step in the conversion of init_basic_state(), hence the mode in
rebase--helper.c is called INIT_BASIC_STATE. init_basic_state() will be
converted in the next commit.
The part of read_strategy_opts() that parses the stategy options is
moved to a new function to allow its use in rebase--helper.c.
Finally, the call to write_basic_state() is removed from
git-rebase--interactive.sh, replaced by a call to `--init-basic-state`.
Signed-off-by: Alban Gruin
---
builtin/rebase--helper.c | 28 +-
git-rebase--interactive.sh | 7 +++-
sequencer.c| 77 --
sequencer.h| 4 ++
4 files changed, 102 insertions(+), 14 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index c4a4c5cfbb..06fe3c018b 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -5,6 +5,8 @@
#include "sequencer.h"
#include "rebase-interactive.h"
#include "argv-array.h"
+#include "rerere.h"
+#include "alias.h"
static GIT_PATH_FUNC(path_squash_onto, "rebase-merge/squash-onto")
@@ -53,11 +55,12 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
unsigned flags = 0, keep_empty = 0, rebase_merges = 0, autosquash = 0;
int abbreviate_commands = 0, rebase_cousins = -1, ret;
const char *head_hash = NULL, *onto = NULL, *restrict_revision = NULL,
- *squash_onto = NULL, *upstream = NULL;
+ *squash_onto = NULL, *upstream = NULL, *head_name = NULL;
+ char *raw_strategies = NULL;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, REARRANGE_SQUASH, ADD_EXEC, EDIT_TODO,
PREPARE_BRANCH,
- COMPLETE_ACTION
+ COMPLETE_ACTION, INIT_BASIC_STATE
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -69,6 +72,7 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
N_("keep original branch points of cousins")),
OPT_BOOL(0, "autosquash", ,
N_("move commits thas begin with squash!/fixup!")),
+ OPT_BOOL(0, "signoff", , N_("sign commits")),
OPT__VERBOSE(, N_("be verbose")),
OPT_CMDMODE(0, "continue", , N_("continue rebase"),
CONTINUE),
@@ -93,6 +97,8 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
N_("prepare the branch to be rebased"),
PREPARE_BRANCH),
OPT_CMDMODE(0, "complete-action", ,
N_("complete the action"), COMPLETE_ACTION),
+ OPT_CMDMODE(0, "init-basic-state", ,
+ N_("initialise the rebase state"),
INIT_BASIC_STATE),
OPT_STRING(0, "onto", , N_("onto"), N_("onto")),
OPT_STRING(0, "restrict-revision", _revision,
N_("restrict-revision"), N_("restrict revision")),
@@ -100,6 +106,14 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
N_("squash onto")),
OPT_STRING(0, "upstream", , N_("upstream"),
N_("the upstream commit")),
+ OPT_STRING(0, "head-name", _name, N_("head-name"),
N_("head name")),
+ OPT_STRING('S', "gpg-sign", _sign, N_("gpg-sign"),
+ N_("GPG-sign commits")),
+ OPT_STRING(0, "strategy", , N_("strategy"),
+ N_("rebase strategy")),
+ OPT_STRING(0, "strategy-opts", _strategies,
N_("strategy-opts"),
+ N_("strategy options")),
+ OPT_RERERE_AUTOUPDATE(_rerere_auto),
OPT_END()
};
@@ -176,6 +190,16 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
free(shortrevisions);
return !!ret;
}
+ if (command == INIT_BASIC_STATE) {
+ if (raw_strategies)
+ parse_strategy_opts(, raw_strategies);
+
+ ret = get_revision_ranges(upstream, onto, _hash, NULL,
NULL);
+ if (ret)
+ return ret;
+
+ return !!write_basic_state(, head_name, onto, head_hash);
+ }
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 08e9a21c2f..6367da66e2 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -57,7 +57,6 @@ init_basic_state () {
rm -f "$(git rev-parse --git-path REBASE_HEAD)"
: > "$state_dir"/interactive || die "$(gettext "Could not mark as
interactive")"
- write_basic_state
}
[GSoC][PATCH v5 08/20] sequencer: refactor append_todo_help() to write its message to a buffer
This refactors append_todo_help() to write its message to a buffer
instead of the todo-list. This is needed for the rewrite of
complete_action(), which will come after the next commit.
As rebase--helper still needs the file manipulation part of
append_todo_help(), it is extracted to a temporary function,
append_todo_help_to_file(). This function will go away after the
rewrite of complete_action().
Signed-off-by: Alban Gruin
---
No changes since v4.
builtin/rebase--helper.c | 2 +-
rebase-interactive.c | 45
rebase-interactive.h | 4 +++-
3 files changed, 36 insertions(+), 15 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 7d9426d23c..313092c465 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -97,7 +97,7 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
if (command == ADD_EXEC && argc == 2)
return !!sequencer_add_exec_commands(argv[1]);
if (command == APPEND_TODO_HELP && argc == 1)
- return !!append_todo_help(0, keep_empty);
+ return !!append_todo_help_to_file(0, keep_empty);
if (command == EDIT_TODO && argc == 1)
return !!edit_todo_list(flags);
if (command == PREPARE_BRANCH && argc == 2)
diff --git a/rebase-interactive.c b/rebase-interactive.c
index 403ecbf3c9..d8b9465597 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -4,11 +4,9 @@
#include "sequencer.h"
#include "strbuf.h"
-int append_todo_help(unsigned edit_todo, unsigned keep_empty)
+void append_todo_help(unsigned edit_todo, unsigned keep_empty,
+ struct strbuf *buf)
{
- struct strbuf buf = STRBUF_INIT;
- FILE *todo;
- int ret;
const char *msg = _("\nCommands:\n"
"p, pick = use commit\n"
"r, reword = use commit, but edit the commit message\n"
@@ -26,11 +24,7 @@ int append_todo_help(unsigned edit_todo, unsigned keep_empty)
"\n"
"These lines can be re-ordered; they are executed from top to bottom.\n");
- todo = fopen_or_warn(rebase_path_todo(), "a");
- if (!todo)
- return 1;
-
- strbuf_add_commented_lines(, msg, strlen(msg));
+ strbuf_add_commented_lines(buf, msg, strlen(msg));
if (get_missing_commit_check_level() == MISSING_COMMIT_CHECK_ERROR)
msg = _("\nDo not remove any line. Use 'drop' "
@@ -39,7 +33,7 @@ int append_todo_help(unsigned edit_todo, unsigned keep_empty)
msg = _("\nIf you remove a line here "
"THAT COMMIT WILL BE LOST.\n");
- strbuf_add_commented_lines(, msg, strlen(msg));
+ strbuf_add_commented_lines(buf, msg, strlen(msg));
if (edit_todo)
msg = _("\nYou are editing the todo file "
@@ -50,12 +44,25 @@ int append_todo_help(unsigned edit_todo, unsigned
keep_empty)
msg = _("\nHowever, if you remove everything, "
"the rebase will be aborted.\n\n");
- strbuf_add_commented_lines(, msg, strlen(msg));
+ strbuf_add_commented_lines(buf, msg, strlen(msg));
if (!keep_empty) {
msg = _("Note that empty commits are commented out");
- strbuf_add_commented_lines(, msg, strlen(msg));
+ strbuf_add_commented_lines(buf, msg, strlen(msg));
}
+}
+
+int append_todo_help_to_file(unsigned edit_todo, unsigned keep_empty)
+{
+ struct strbuf buf = STRBUF_INIT;
+ FILE *todo;
+ int ret;
+
+ todo = fopen_or_warn(rebase_path_todo(), "a");
+ if (!todo)
+ return 1;
+
+ append_todo_help(edit_todo, keep_empty, );
ret = fputs(buf.buf, todo);
if (ret < 0)
@@ -88,7 +95,19 @@ int edit_todo_list(unsigned flags)
strbuf_release();
transform_todos(flags | TODO_LIST_SHORTEN_IDS);
- append_todo_help(1, 0);
+
+ strbuf_read_file(, todo_file, 0);
+ append_todo_help(1, 0, );
+
+ todo = fopen_or_warn(todo_file, "w");
+ if (!todo) {
+ strbuf_release();
+ return 1;
+ }
+
+ strbuf_write(, todo);
+ fclose(todo);
+ strbuf_release();
if (launch_sequence_editor(todo_file, NULL, NULL))
return 1;
diff --git a/rebase-interactive.h b/rebase-interactive.h
index 155219e742..d33f3176b7 100644
--- a/rebase-interactive.h
+++ b/rebase-interactive.h
@@ -1,7 +1,9 @@
#ifndef REBASE_INTERACTIVE_H
#define REBASE_INTERACTIVE_H
-int append_todo_help(unsigned edit_todo, unsigned keep_empty);
+void append_todo_help(unsigned edit_todo, unsigned keep_empty,
+ struct strbuf *buf);
+int append_todo_help_to_file(unsigned edit_todo, unsigned keep_empty);
int edit_todo_list(unsigned flags);
#endif
--
2.18.0
[GSoC][PATCH v5 10/20] t3404: todo list with commented-out commands only aborts
If the todo list generated by `--make-script` is empty, complete_action() writes a noop, but if it has only commented-out commands, it will abort with the message "Nothing to do", and does not launch the editor. This adds a new test to ensure that complete_action() behaves this way. Signed-off-by: Alban Gruin --- No changes since v4. t/t3404-rebase-interactive.sh | 10 ++ 1 file changed, 10 insertions(+) diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 01616901bd..496d88d7d6 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -75,6 +75,16 @@ test_expect_success 'rebase --keep-empty' ' test_line_count = 6 actual ' +cat > expect&1 && + test_i18ncmp expect actual +' + test_expect_success 'rebase -i with the exec command' ' git checkout master && ( -- 2.18.0
[GSoC][PATCH v5 07/20] rebase -i: rewrite checkout_onto() in C
This rewrites checkout_onto() from shell to C.
A new command (“checkout-onto”) is added to rebase--helper.c. The shell
version is then stripped.
Signed-off-by: Alban Gruin
---
No changes since v4.
builtin/rebase--helper.c | 7 ++-
git-rebase--interactive.sh | 25 -
sequencer.c| 19 +++
sequencer.h| 3 +++
4 files changed, 32 insertions(+), 22 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 0e76dadba6..7d9426d23c 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -18,7 +18,8 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
- ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, PREPARE_BRANCH
+ ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, PREPARE_BRANCH,
+ CHECKOUT_ONTO
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -54,6 +55,8 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
EDIT_TODO),
OPT_CMDMODE(0, "prepare-branch", ,
N_("prepare the branch to be rebased"),
PREPARE_BRANCH),
+ OPT_CMDMODE(0, "checkout-onto", ,
+ N_("checkout a commit"), CHECKOUT_ONTO),
OPT_END()
};
@@ -99,5 +102,7 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
return !!edit_todo_list(flags);
if (command == PREPARE_BRANCH && argc == 2)
return !!prepare_branch_to_be_rebased(, argv[1]);
+ if (command == CHECKOUT_ONTO && argc == 4)
+ return !!checkout_onto(, argv[1], argv[2], argv[3]);
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 77e972bb6c..b68f108f28 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -28,17 +28,6 @@ case "$comment_char" in
;;
esac
-orig_reflog_action="$GIT_REFLOG_ACTION"
-
-comment_for_reflog () {
- case "$orig_reflog_action" in
- ''|rebase*)
- GIT_REFLOG_ACTION="rebase -i ($1)"
- export GIT_REFLOG_ACTION
- ;;
- esac
-}
-
die_abort () {
apply_autostash
rm -rf "$state_dir"
@@ -70,14 +59,6 @@ collapse_todo_ids() {
git rebase--helper --shorten-ids
}
-# Switch to the branch in $into and notify it in the reflog
-checkout_onto () {
- comment_for_reflog start
- GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
- output git checkout $onto || die_abort "$(gettext "could not detach
HEAD")"
- git update-ref ORIG_HEAD $orig_head
-}
-
get_missing_commit_check_level () {
check_level=$(git config --get rebase.missingCommitsCheck)
check_level=${check_level:-ignore}
@@ -176,7 +157,8 @@ EOF
git rebase--helper --check-todo-list || {
ret=$?
- checkout_onto
+ git rebase--helper --checkout-onto "$onto_name" "$onto" \
+ "$orig_head" ${verbose:+--verbose}
exit $ret
}
@@ -186,7 +168,8 @@ EOF
onto="$(git rebase--helper --skip-unnecessary-picks)" ||
die "Could not skip unnecessary pick commands"
- checkout_onto
+ git rebase--helper --checkout-onto "$onto_name" "$onto" "$orig_head" \
+ ${verbose:+--verbose}
require_clean_work_tree "rebase"
exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
--continue
diff --git a/sequencer.c b/sequencer.c
index 52949f38b3..4285247810 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3171,6 +3171,25 @@ int prepare_branch_to_be_rebased(struct replay_opts
*opts, const char *commit)
return 0;
}
+int checkout_onto(struct replay_opts *opts,
+ const char *onto_name, const char *onto,
+ const char *orig_head)
+{
+ struct object_id oid;
+ const char *action = reflog_message(opts, "start", "checkout %s",
onto_name);
+
+ if (get_oid(orig_head, ))
+ return error(_("%s: not a valid OID"), orig_head);
+
+ if (run_git_checkout(opts, onto, action)) {
+ apply_autostash(opts);
+ sequencer_remove_state(opts);
+ return error(_("could not detach HEAD"));
+ }
+
+ return update_ref(NULL, "ORIG_HEAD", , NULL, 0,
UPDATE_REFS_MSG_ON_ERR);
+}
+
static const char rescheduled_advice[] =
N_("Could not execute the todo command\n"
"\n"
diff --git a/sequencer.h b/sequencer.h
index 93da713fe2..11a5334612 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -107,6 +107,9 @@ void
[GSoC][PATCH v5 12/20] rebase -i: remove unused modes and functions
This removes the modes `--skip-unnecessary-picks`, `--append-todo-help`,
and `--checkout-onto` from rebase--helper.c, the functions of
git-rebase--interactive.sh that were rendered useless by the rewrite of
complete_action(), and append_todo_help_to_file() from
rebase-interactive.c.
skip_unnecessary_picks() and checkout_onto() becomes static, as they are
only used inside of the sequencer.
Signed-off-by: Alban Gruin
---
No changes since v4.
builtin/rebase--helper.c | 23 ++
git-rebase--interactive.sh | 50 --
rebase-interactive.c | 22 -
rebase-interactive.h | 1 -
sequencer.c| 8 +++---
sequencer.h| 4 ---
6 files changed, 6 insertions(+), 102 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index d7fa5a5062..6085527b2b 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -17,9 +17,8 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
int abbreviate_commands = 0, rebase_cousins = -1;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
- CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
- ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, PREPARE_BRANCH,
- CHECKOUT_ONTO, COMPLETE_ACTION
+ CHECK_TODO_LIST, REARRANGE_SQUASH, ADD_EXEC, EDIT_TODO,
PREPARE_BRANCH,
+ COMPLETE_ACTION
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -44,21 +43,15 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
N_("expand commit ids in the todo list"), EXPAND_OIDS),
OPT_CMDMODE(0, "check-todo-list", ,
N_("check the todo list"), CHECK_TODO_LIST),
- OPT_CMDMODE(0, "skip-unnecessary-picks", ,
- N_("skip unnecessary picks"), SKIP_UNNECESSARY_PICKS),
OPT_CMDMODE(0, "rearrange-squash", ,
N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
OPT_CMDMODE(0, "add-exec-commands", ,
N_("insert exec commands in todo list"), ADD_EXEC),
- OPT_CMDMODE(0, "append-todo-help", ,
- N_("insert the help in the todo list"),
APPEND_TODO_HELP),
OPT_CMDMODE(0, "edit-todo", ,
N_("edit the todo list during an interactive
rebase"),
EDIT_TODO),
OPT_CMDMODE(0, "prepare-branch", ,
N_("prepare the branch to be rebased"),
PREPARE_BRANCH),
- OPT_CMDMODE(0, "checkout-onto", ,
- N_("checkout a commit"), CHECKOUT_ONTO),
OPT_CMDMODE(0, "complete-action", ,
N_("complete the action"), COMPLETE_ACTION),
OPT_END()
@@ -94,26 +87,14 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
return !!transform_todos(flags);
if (command == CHECK_TODO_LIST && argc == 1)
return !!check_todo_list();
- if (command == SKIP_UNNECESSARY_PICKS && argc == 1) {
- struct object_id oid;
- int ret = skip_unnecessary_picks();
-
- if (!ret)
- puts(oid_to_hex());
- return !!ret;
- }
if (command == REARRANGE_SQUASH && argc == 1)
return !!rearrange_squash();
if (command == ADD_EXEC && argc == 2)
return !!sequencer_add_exec_commands(argv[1]);
- if (command == APPEND_TODO_HELP && argc == 1)
- return !!append_todo_help_to_file(0, keep_empty);
if (command == EDIT_TODO && argc == 1)
return !!edit_todo_list(flags);
if (command == PREPARE_BRANCH && argc == 2)
return !!prepare_branch_to_be_rebased(, argv[1]);
- if (command == CHECKOUT_ONTO && argc == 4)
- return !!checkout_onto(, argv[1], argv[2], argv[3]);
if (command == COMPLETE_ACTION && argc == 6)
return !!complete_action(, flags, argv[1], argv[2],
argv[3],
argv[4], argv[5], autosquash);
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 59dc4888a6..0d66c0f8b8 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -16,56 +16,6 @@ todo="$state_dir"/git-rebase-todo
GIT_CHERRY_PICK_HELP="$resolvemsg"
export GIT_CHERRY_PICK_HELP
-comment_char=$(git config --get core.commentchar 2>/dev/null)
-case "$comment_char" in
-'' | auto)
- comment_char="#"
- ;;
-?)
- ;;
-*)
- comment_char=$(echo "$comment_char" | cut -c1)
- ;;
-esac
-
-die_abort () {
- apply_autostash
- rm -rf "$state_dir"
[GSoC][PATCH v5 11/20] rebase -i: rewrite complete_action() in C
This rewrites complete_action() from shell to C.
A new mode is added to rebase--helper (`--complete-action`), as well as
a new flag (`--autosquash`).
Finally, complete_action() is stripped from git-rebase--interactive.sh.
The original complete_action() would return the code 2 when the todo
list contained no actions. This was a special case for rebase -i and
-p; git-rebase.sh would then apply the autostash, delete the state
directory, and die with the message "Nothing to do". This cleanup is
rewritten in C instead of returning 2. As rebase -i no longer returns
2, the comment describing this behaviour in git-rebase.sh is updated to
reflect this change.
The first check might seem useless as we write "noop" to the todo list
if it is empty. Actually, the todo list might contain commented
commands (ie. empty commits). In this case, complete_action() won’t
write "noop", and will abort without starting the editor.
Signed-off-by: Alban Gruin
---
No changes since v4.
builtin/rebase--helper.c | 12 -
git-rebase--interactive.sh | 53 ++--
git-rebase.sh | 2 +-
sequencer.c| 99 ++
sequencer.h| 4 ++
5 files changed, 118 insertions(+), 52 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index bed3dd2b95..d7fa5a5062 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -13,13 +13,13 @@ static const char * const builtin_rebase_helper_usage[] = {
int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
{
struct replay_opts opts = REPLAY_OPTS_INIT;
- unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
+ unsigned flags = 0, keep_empty = 0, rebase_merges = 0, autosquash = 0;
int abbreviate_commands = 0, rebase_cousins = -1;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, PREPARE_BRANCH,
- CHECKOUT_ONTO
+ CHECKOUT_ONTO, COMPLETE_ACTION
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -29,6 +29,8 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
OPT_BOOL(0, "rebase-merges", _merges, N_("rebase merge
commits")),
OPT_BOOL(0, "rebase-cousins", _cousins,
N_("keep original branch points of cousins")),
+ OPT_BOOL(0, "autosquash", ,
+N_("move commits thas begin with squash!/fixup!")),
OPT__VERBOSE(, N_("be verbose")),
OPT_CMDMODE(0, "continue", , N_("continue rebase"),
CONTINUE),
@@ -57,6 +59,8 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
N_("prepare the branch to be rebased"),
PREPARE_BRANCH),
OPT_CMDMODE(0, "checkout-onto", ,
N_("checkout a commit"), CHECKOUT_ONTO),
+ OPT_CMDMODE(0, "complete-action", ,
+ N_("complete the action"), COMPLETE_ACTION),
OPT_END()
};
@@ -110,5 +114,9 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
return !!prepare_branch_to_be_rebased(, argv[1]);
if (command == CHECKOUT_ONTO && argc == 4)
return !!checkout_onto(, argv[1], argv[2], argv[3]);
+ if (command == COMPLETE_ACTION && argc == 6)
+ return !!complete_action(, flags, argv[1], argv[2],
argv[3],
+argv[4], argv[5], autosquash);
+
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index b68f108f28..59dc4888a6 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -127,54 +127,6 @@ init_revisions_and_shortrevisions () {
fi
}
-complete_action() {
- test -s "$todo" || echo noop >> "$todo"
- test -z "$autosquash" || git rebase--helper --rearrange-squash || exit
- test -n "$cmd" && git rebase--helper --add-exec-commands "$cmd"
-
- todocount=$(git stripspace --strip-comments <"$todo" | wc -l)
- todocount=${todocount##* }
-
-cat >>"$todo" <"$todo" ||
die "$(gettext "Could not generate todo list")"
- complete_action
+ exec git rebase--helper --complete-action "$shortrevisions"
"$onto_name" \
+ "$shortonto" "$orig_head" "$cmd" $allow_empty_message \
+ ${autosquash:+--autosquash} ${keep_empty:+--keep-empty} \
+ ${verbose:+--verbose} ${force_rebase:+--no-ff}
}
diff --git a/git-rebase.sh b/git-rebase.sh
index 7973447645..51a6db7daa 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -219,7
[GSoC][PATCH v5 09/20] sequencer: change the way skip_unnecessary_picks() returns its result
Instead of skip_unnecessary_picks() printing its result to stdout, it
returns it into a struct object_id, as the rewrite of complete_action()
(to come in the next commit) will need it.
rebase--helper then is modified to fit this change.
Signed-off-by: Alban Gruin
---
No changes since v4.
builtin/rebase--helper.c | 10 --
sequencer.c | 13 ++---
sequencer.h | 2 +-
3 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 313092c465..bed3dd2b95 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -90,8 +90,14 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
return !!transform_todos(flags);
if (command == CHECK_TODO_LIST && argc == 1)
return !!check_todo_list();
- if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
- return !!skip_unnecessary_picks();
+ if (command == SKIP_UNNECESSARY_PICKS && argc == 1) {
+ struct object_id oid;
+ int ret = skip_unnecessary_picks();
+
+ if (!ret)
+ puts(oid_to_hex());
+ return !!ret;
+ }
if (command == REARRANGE_SQUASH && argc == 1)
return !!rearrange_squash();
if (command == ADD_EXEC && argc == 2)
diff --git a/sequencer.c b/sequencer.c
index 4285247810..a2e04b9eca 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4420,17 +4420,17 @@ static int rewrite_file(const char *path, const char
*buf, size_t len)
}
/* skip picking commits whose parents are unchanged */
-int skip_unnecessary_picks(void)
+int skip_unnecessary_picks(struct object_id *output_oid)
{
const char *todo_file = rebase_path_todo();
struct strbuf buf = STRBUF_INIT;
struct todo_list todo_list = TODO_LIST_INIT;
- struct object_id onto_oid, *oid = _oid, *parent_oid;
+ struct object_id *parent_oid;
int fd, i;
if (!read_oneliner(, rebase_path_onto(), 0))
return error(_("could not read 'onto'"));
- if (get_oid(buf.buf, _oid)) {
+ if (get_oid(buf.buf, output_oid)) {
strbuf_release();
return error(_("need a HEAD to fixup"));
}
@@ -4460,9 +4460,9 @@ int skip_unnecessary_picks(void)
if (item->commit->parents->next)
break; /* merge commit */
parent_oid = >commit->parents->item->object.oid;
- if (hashcmp(parent_oid->hash, oid->hash))
+ if (hashcmp(parent_oid->hash, output_oid->hash))
break;
- oid = >commit->object.oid;
+ oidcpy(output_oid, >commit->object.oid);
}
if (i > 0) {
int offset = get_item_line_offset(_list, i);
@@ -4491,11 +4491,10 @@ int skip_unnecessary_picks(void)
todo_list.current = i;
if (is_fixup(peek_command(_list, 0)))
- record_in_rewritten(oid, peek_command(_list, 0));
+ record_in_rewritten(output_oid,
peek_command(_list, 0));
}
todo_list_release(_list);
- printf("%s\n", oid_to_hex(oid));
return 0;
}
diff --git a/sequencer.h b/sequencer.h
index 11a5334612..f11dabfd65 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -88,7 +88,7 @@ int sequencer_add_exec_commands(const char *command);
int transform_todos(unsigned flags);
enum missing_commit_check_level get_missing_commit_check_level(void);
int check_todo_list(void);
-int skip_unnecessary_picks(void);
+int skip_unnecessary_picks(struct object_id *output_oid);
int rearrange_squash(void);
extern const char sign_off_header[];
--
2.18.0
[GSoC][PATCH v5 01/20] sequencer: make two functions and an enum from sequencer.c public
This makes rebase_path_todo(), get_missing_commit_check_level() and the
enum check_level accessible outside sequencer.c, renames check_level to
missing_commit_check_level, and prefixes its value names by
MISSING_COMMIT_ to avoid namespace pollution.
This function and this enum will eventually be moved to
rebase-interactive.c and become static again, so no special attention
was given to the naming.
This will be needed for the rewrite of append_todo_help() from shell to
C, as it will be in a new library source file, rebase-interactive.c.
Signed-off-by: Alban Gruin
---
No changes since v4.
sequencer.c | 22 +-
sequencer.h | 8
2 files changed, 17 insertions(+), 13 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 16c1411054..8eff526584 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -52,7 +52,7 @@ static GIT_PATH_FUNC(rebase_path, "rebase-merge")
* 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")
+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
@@ -4249,24 +4249,20 @@ int transform_todos(unsigned flags)
return i;
}
-enum check_level {
- CHECK_IGNORE = 0, CHECK_WARN, CHECK_ERROR
-};
-
-static enum check_level get_missing_commit_check_level(void)
+enum missing_commit_check_level get_missing_commit_check_level(void)
{
const char *value;
if (git_config_get_value("rebase.missingcommitscheck", ) ||
!strcasecmp("ignore", value))
- return CHECK_IGNORE;
+ return MISSING_COMMIT_CHECK_IGNORE;
if (!strcasecmp("warn", value))
- return CHECK_WARN;
+ return MISSING_COMMIT_CHECK_WARN;
if (!strcasecmp("error", value))
- return CHECK_ERROR;
+ return MISSING_COMMIT_CHECK_ERROR;
warning(_("unrecognized setting %s for option "
"rebase.missingCommitsCheck. Ignoring."), value);
- return CHECK_IGNORE;
+ return MISSING_COMMIT_CHECK_IGNORE;
}
define_commit_slab(commit_seen, unsigned char);
@@ -4278,7 +4274,7 @@ define_commit_slab(commit_seen, unsigned char);
*/
int check_todo_list(void)
{
- enum check_level check_level = get_missing_commit_check_level();
+ enum missing_commit_check_level check_level =
get_missing_commit_check_level();
struct strbuf todo_file = STRBUF_INIT;
struct todo_list todo_list = TODO_LIST_INIT;
struct strbuf missing = STRBUF_INIT;
@@ -4295,7 +4291,7 @@ int check_todo_list(void)
advise_to_edit_todo = res =
parse_insn_buffer(todo_list.buf.buf, _list);
- if (res || check_level == CHECK_IGNORE)
+ if (res || check_level == MISSING_COMMIT_CHECK_IGNORE)
goto leave_check;
/* Mark the commits in git-rebase-todo as seen */
@@ -4330,7 +4326,7 @@ int check_todo_list(void)
if (!missing.len)
goto leave_check;
- if (check_level == CHECK_ERROR)
+ if (check_level == MISSING_COMMIT_CHECK_ERROR)
advise_to_edit_todo = res = 1;
fprintf(stderr,
diff --git a/sequencer.h b/sequencer.h
index c5787c6b56..ffab798f1e 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -3,6 +3,7 @@
const char *git_path_commit_editmsg(void);
const char *git_path_seq_dir(void);
+const char *rebase_path_todo(void);
#define APPEND_SIGNOFF_DEDUP (1u << 0)
@@ -57,6 +58,12 @@ struct replay_opts {
};
#define REPLAY_OPTS_INIT { .action = -1, .current_fixups = STRBUF_INIT }
+enum missing_commit_check_level {
+ MISSING_COMMIT_CHECK_IGNORE = 0,
+ MISSING_COMMIT_CHECK_WARN,
+ MISSING_COMMIT_CHECK_ERROR
+};
+
/* Call this to setup defaults before parsing command line options */
void sequencer_init_config(struct replay_opts *opts);
int sequencer_pick_revisions(struct replay_opts *opts);
@@ -79,6 +86,7 @@ int sequencer_make_script(FILE *out, int argc, const char
**argv,
int sequencer_add_exec_commands(const char *command);
int transform_todos(unsigned flags);
+enum missing_commit_check_level get_missing_commit_check_level(void);
int check_todo_list(void);
int skip_unnecessary_picks(void);
int rearrange_squash(void);
--
2.18.0
[GSoC][PATCH v5 05/20] sequencer: add a new function to silence a command, except if it fails
This adds a new function, run_command_silent_on_success(), to
redirect the stdout and stderr of a command to a strbuf, and then to run
that command. This strbuf is printed only if the command fails. It is
functionnaly similar to output() from git-rebase.sh.
run_git_commit() is then refactored to use of
run_command_silent_on_success().
Signed-off-by: Alban Gruin
---
No changes since v4.
sequencer.c | 51 +--
1 file changed, 25 insertions(+), 26 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 8eff526584..6d87f5ae6a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -769,6 +769,23 @@ N_("you have staged changes in your working tree\n"
#define VERIFY_MSG (1<<4)
#define CREATE_ROOT_COMMIT (1<<5)
+static int run_command_silent_on_success(struct child_process *cmd)
+{
+ struct strbuf buf = STRBUF_INIT;
+ int rc;
+
+ cmd->stdout_to_stderr = 1;
+ rc = pipe_command(cmd,
+ NULL, 0,
+ NULL, 0,
+ , 0);
+
+ if (rc)
+ fputs(buf.buf, stderr);
+ strbuf_release();
+ return rc;
+}
+
/*
* If we are cherry-pick, and if the merge did not result in
* hand-editing, we will hit this commit and inherit the original
@@ -823,18 +840,11 @@ static int run_git_commit(const char *defmsg, struct
replay_opts *opts,
cmd.git_cmd = 1;
- if (is_rebase_i(opts)) {
- if (!(flags & EDIT_MSG)) {
- cmd.stdout_to_stderr = 1;
- cmd.err = -1;
- }
-
- if (read_env_script(_array)) {
- const char *gpg_opt = gpg_sign_opt_quoted(opts);
+ if (is_rebase_i(opts) && read_env_script(_array)) {
+ const char *gpg_opt = gpg_sign_opt_quoted(opts);
- return error(_(staged_changes_advice),
-gpg_opt, gpg_opt);
- }
+ return error(_(staged_changes_advice),
+gpg_opt, gpg_opt);
}
argv_array_push(, "commit");
@@ -864,21 +874,10 @@ static int run_git_commit(const char *defmsg, struct
replay_opts *opts,
if (opts->allow_empty_message)
argv_array_push(, "--allow-empty-message");
- if (cmd.err == -1) {
- /* hide stderr on success */
- struct strbuf buf = STRBUF_INIT;
- int rc = pipe_command(,
- NULL, 0,
- /* stdout is already redirected */
- NULL, 0,
- , 0);
- if (rc)
- fputs(buf.buf, stderr);
- strbuf_release();
- return rc;
- }
-
- return run_command();
+ if (is_rebase_i(opts) && !(flags & EDIT_MSG))
+ return run_command_silent_on_success();
+ else
+ return run_command();
}
static int rest_is_empty(const struct strbuf *sb, int start)
--
2.18.0
[GSoC][PATCH v5 00/20] rebase -i: rewrite in C
This patch series rewrite the interactive rebase from shell to C.
It is based on ffc6fa0e39 ("Fourth batch for 2.19 cycle", 2018-07-24).
The v4 was based on b7bd9486 ("Third batch for 2.19 cycle", 2018-07-18).
I wanted to make sure my series works well with 'bb/pedantic',
'jk/empty-pick-fix', and 'as/sequencer-customizable-comment-char', as
they modified sequencer.c.
Changes since v4:
- [15/20] Add a newline char to $state_dir/quiet in write_basic_state(), even
if $GIT_QUIET is not set
- [20/20] Remove the declaration of cmd_rebase__helper() in builtin.h
Alban Gruin (20):
sequencer: make two functions and an enum from sequencer.c public
rebase -i: rewrite append_todo_help() in C
editor: add a function to launch the sequence editor
rebase -i: rewrite the edit-todo functionality in C
sequencer: add a new function to silence a command, except if it fails
rebase -i: rewrite setup_reflog_action() in C
rebase -i: rewrite checkout_onto() in C
sequencer: refactor append_todo_help() to write its message to a
buffer
sequencer: change the way skip_unnecessary_picks() returns its result
t3404: todo list with commented-out commands only aborts
rebase -i: rewrite complete_action() in C
rebase -i: remove unused modes and functions
rebase -i: implement the logic to initialize $revisions in C
rebase -i: rewrite the rest of init_revisions_and_shortrevisions() in
C
rebase -i: rewrite write_basic_state() in C
rebase -i: rewrite init_basic_state() in C
rebase -i: implement the main part of interactive rebase as a builtin
rebase--interactive2: rewrite the submodes of interactive rebase in C
rebase -i: remove git-rebase--interactive.sh
rebase -i: move rebase--helper modes to rebase--interactive
.gitignore | 1 -
Makefile | 5 +-
builtin.h | 2 +-
builtin/rebase--helper.c | 88 --
builtin/rebase--interactive.c | 264
cache.h| 1 +
editor.c | 27 ++-
git-rebase--interactive.sh | 283 --
git-rebase--preserve-merges.sh | 10 +-
git-rebase.sh | 47 -
git.c | 2 +-
rebase-interactive.c | 96 ++
rebase-interactive.h | 8 +
sequencer.c| 311 +++--
sequencer.h| 19 +-
strbuf.h | 2 +
t/t3404-rebase-interactive.sh | 10 ++
17 files changed, 729 insertions(+), 447 deletions(-)
delete mode 100644 builtin/rebase--helper.c
create mode 100644 builtin/rebase--interactive.c
delete mode 100644 git-rebase--interactive.sh
create mode 100644 rebase-interactive.c
create mode 100644 rebase-interactive.h
--
2.18.0
[GSoC][PATCH v5 02/20] rebase -i: rewrite append_todo_help() in C
This rewrites append_todo_help() from shell to C. It also incorporates
some parts of initiate_action() and complete_action() that also write
help texts to the todo file.
This also introduces the source file rebase-interactive.c. This file
will contain functions necessary for interactive rebase that are too
specific for the sequencer, and is part of libgit.a.
Two flags are added to rebase--helper.c: one to call
append_todo_help() (`--append-todo-help`), and another one to tell
append_todo_help() to write the help text suited for the edit-todo
mode (`--write-edit-todo`).
Finally, append_todo_help() is removed from git-rebase--interactive.sh
to use `rebase--helper --append-todo-help` instead.
Signed-off-by: Alban Gruin
---
No changes since v4.
Makefile | 1 +
builtin/rebase--helper.c | 11 --
git-rebase--interactive.sh | 52 ++---
rebase-interactive.c | 68 ++
rebase-interactive.h | 6
5 files changed, 86 insertions(+), 52 deletions(-)
create mode 100644 rebase-interactive.c
create mode 100644 rebase-interactive.h
diff --git a/Makefile b/Makefile
index 08e5c54549..909a687857 100644
--- a/Makefile
+++ b/Makefile
@@ -922,6 +922,7 @@ LIB_OBJS += protocol.o
LIB_OBJS += quote.o
LIB_OBJS += reachable.o
LIB_OBJS += read-cache.o
+LIB_OBJS += rebase-interactive.o
LIB_OBJS += reflog-walk.o
LIB_OBJS += refs.o
LIB_OBJS += refs/files-backend.o
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index f7c2a5fdc8..05e73e71d4 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -3,6 +3,7 @@
#include "config.h"
#include "parse-options.h"
#include "sequencer.h"
+#include "rebase-interactive.h"
static const char * const builtin_rebase_helper_usage[] = {
N_("git rebase--helper []"),
@@ -12,12 +13,12 @@ static const char * const builtin_rebase_helper_usage[] = {
int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
{
struct replay_opts opts = REPLAY_OPTS_INIT;
- unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
+ unsigned flags = 0, keep_empty = 0, rebase_merges = 0, write_edit_todo
= 0;
int abbreviate_commands = 0, rebase_cousins = -1;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
- ADD_EXEC
+ ADD_EXEC, APPEND_TODO_HELP
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -27,6 +28,8 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
OPT_BOOL(0, "rebase-merges", _merges, N_("rebase merge
commits")),
OPT_BOOL(0, "rebase-cousins", _cousins,
N_("keep original branch points of cousins")),
+ OPT_BOOL(0, "write-edit-todo", _edit_todo,
+N_("append the edit-todo message to the todo-list")),
OPT_CMDMODE(0, "continue", , N_("continue rebase"),
CONTINUE),
OPT_CMDMODE(0, "abort", , N_("abort rebase"),
@@ -45,6 +48,8 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
OPT_CMDMODE(0, "add-exec-commands", ,
N_("insert exec commands in todo list"), ADD_EXEC),
+ OPT_CMDMODE(0, "append-todo-help", ,
+ N_("insert the help in the todo list"),
APPEND_TODO_HELP),
OPT_END()
};
@@ -84,5 +89,7 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
return !!rearrange_squash();
if (command == ADD_EXEC && argc == 2)
return !!sequencer_add_exec_commands(argv[1]);
+ if (command == APPEND_TODO_HELP && argc == 1)
+ return !!append_todo_help(write_edit_todo, keep_empty);
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 299ded2137..94c23a7af2 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -39,38 +39,6 @@ comment_for_reflog () {
esac
}
-append_todo_help () {
- gettext "
-Commands:
-p, pick = use commit
-r, reword = use commit, but edit the commit message
-e, edit = use commit, but stop for amending
-s, squash = use commit, but meld into previous commit
-f, fixup = like \"squash\", but discard this commit's log message
-x, exec = run command (the rest of the line) using shell
-d, drop = remove commit
-l, label = label current HEAD with a name
-t, reset = reset HEAD to a label
-m, merge [-C | -c ] [# ]
-. create a merge commit using the original merge commit's
-. message (or the oneline, if no original
[GSoC][PATCH v5 06/20] rebase -i: rewrite setup_reflog_action() in C
This rewrites (the misnamed) setup_reflog_action() from shell to C. The
new version is called prepare_branch_to_be_rebased().
A new command is added to rebase--helper.c, “checkout-base”, as well as
a new flag, “verbose”, to avoid silencing the output of the checkout
operation called by checkout_base_commit().
The function `run_git_checkout()` will also be used in the next commit,
therefore its code is not part of `checkout_base_commit()`.
The shell version is then stripped in favour of a call to the helper.
As $GIT_REFLOG_ACTION is no longer set at the first call of
checkout_onto(), a call to comment_for_reflog() is added at the
beginning of this function.
Signed-off-by: Alban Gruin
---
No changes since v4.
builtin/rebase--helper.c | 7 ++-
git-rebase--interactive.sh | 16 ++--
sequencer.c| 30 ++
sequencer.h| 2 ++
4 files changed, 40 insertions(+), 15 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 731a64971d..0e76dadba6 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -18,7 +18,7 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
- ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO
+ ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, PREPARE_BRANCH
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -28,6 +28,7 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
OPT_BOOL(0, "rebase-merges", _merges, N_("rebase merge
commits")),
OPT_BOOL(0, "rebase-cousins", _cousins,
N_("keep original branch points of cousins")),
+ OPT__VERBOSE(, N_("be verbose")),
OPT_CMDMODE(0, "continue", , N_("continue rebase"),
CONTINUE),
OPT_CMDMODE(0, "abort", , N_("abort rebase"),
@@ -51,6 +52,8 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
OPT_CMDMODE(0, "edit-todo", ,
N_("edit the todo list during an interactive
rebase"),
EDIT_TODO),
+ OPT_CMDMODE(0, "prepare-branch", ,
+ N_("prepare the branch to be rebased"),
PREPARE_BRANCH),
OPT_END()
};
@@ -94,5 +97,7 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
return !!append_todo_help(0, keep_empty);
if (command == EDIT_TODO && argc == 1)
return !!edit_todo_list(flags);
+ if (command == PREPARE_BRANCH && argc == 2)
+ return !!prepare_branch_to_be_rebased(, argv[1]);
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2defe607f4..77e972bb6c 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -72,6 +72,7 @@ collapse_todo_ids() {
# Switch to the branch in $into and notify it in the reflog
checkout_onto () {
+ comment_for_reflog start
GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
output git checkout $onto || die_abort "$(gettext "could not detach
HEAD")"
git update-ref ORIG_HEAD $orig_head
@@ -119,19 +120,6 @@ initiate_action () {
esac
}
-setup_reflog_action () {
- comment_for_reflog start
-
- if test ! -z "$switch_to"
- then
- GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
- output git checkout "$switch_to" -- ||
- die "$(eval_gettext "Could not checkout \$switch_to")"
-
- comment_for_reflog start
- fi
-}
-
init_basic_state () {
orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")"
mkdir -p "$state_dir" || die "$(eval_gettext "Could not create
temporary \$state_dir")"
@@ -211,7 +199,7 @@ git_rebase__interactive () {
return 0
fi
- setup_reflog_action
+ git rebase--helper --prepare-branch "$switch_to" ${verbose:+--verbose}
init_basic_state
init_revisions_and_shortrevisions
diff --git a/sequencer.c b/sequencer.c
index 6d87f5ae6a..52949f38b3 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3141,6 +3141,36 @@ static const char *reflog_message(struct replay_opts
*opts,
return buf.buf;
}
+static int run_git_checkout(struct replay_opts *opts, const char *commit,
+ const char *action)
+{
+ struct child_process cmd = CHILD_PROCESS_INIT;
+
+ cmd.git_cmd = 1;
+
+ argv_array_push(, "checkout");
+ argv_array_push(, commit);
+ argv_array_pushf(_array,
[GSoC][PATCH v5 04/20] rebase -i: rewrite the edit-todo functionality in C
This rewrites the edit-todo functionality from shell to C.
To achieve that, a new command mode, `edit-todo`, is added, and the
`write-edit-todo` flag is removed, as the shell script does not need to
write the edit todo help message to the todo list anymore.
The shell version is then stripped in favour of a call to the helper.
Signed-off-by: Alban Gruin
---
No changes since v4.
builtin/rebase--helper.c | 13 -
git-rebase--interactive.sh | 11 +--
rebase-interactive.c | 31 +++
rebase-interactive.h | 1 +
4 files changed, 41 insertions(+), 15 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 05e73e71d4..731a64971d 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -13,12 +13,12 @@ static const char * const builtin_rebase_helper_usage[] = {
int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
{
struct replay_opts opts = REPLAY_OPTS_INIT;
- unsigned flags = 0, keep_empty = 0, rebase_merges = 0, write_edit_todo
= 0;
+ unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
int abbreviate_commands = 0, rebase_cousins = -1;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
- ADD_EXEC, APPEND_TODO_HELP
+ ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -28,8 +28,6 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
OPT_BOOL(0, "rebase-merges", _merges, N_("rebase merge
commits")),
OPT_BOOL(0, "rebase-cousins", _cousins,
N_("keep original branch points of cousins")),
- OPT_BOOL(0, "write-edit-todo", _edit_todo,
-N_("append the edit-todo message to the todo-list")),
OPT_CMDMODE(0, "continue", , N_("continue rebase"),
CONTINUE),
OPT_CMDMODE(0, "abort", , N_("abort rebase"),
@@ -50,6 +48,9 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
N_("insert exec commands in todo list"), ADD_EXEC),
OPT_CMDMODE(0, "append-todo-help", ,
N_("insert the help in the todo list"),
APPEND_TODO_HELP),
+ OPT_CMDMODE(0, "edit-todo", ,
+ N_("edit the todo list during an interactive
rebase"),
+ EDIT_TODO),
OPT_END()
};
@@ -90,6 +91,8 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
if (command == ADD_EXEC && argc == 2)
return !!sequencer_add_exec_commands(argv[1]);
if (command == APPEND_TODO_HELP && argc == 1)
- return !!append_todo_help(write_edit_todo, keep_empty);
+ return !!append_todo_help(0, keep_empty);
+ if (command == EDIT_TODO && argc == 1)
+ return !!edit_todo_list(flags);
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 94c23a7af2..2defe607f4 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -108,16 +108,7 @@ initiate_action () {
--continue
;;
edit-todo)
- git stripspace --strip-comments <"$todo" >"$todo".new
- mv -f "$todo".new "$todo"
- collapse_todo_ids
- git rebase--helper --append-todo-help --write-edit-todo
-
- git_sequence_editor "$todo" ||
- die "$(gettext "Could not execute editor")"
- expand_todo_ids
-
- exit
+ exec git rebase--helper --edit-todo
;;
show-current-patch)
exec git show REBASE_HEAD --
diff --git a/rebase-interactive.c b/rebase-interactive.c
index d7996bc8d9..403ecbf3c9 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -66,3 +66,34 @@ int append_todo_help(unsigned edit_todo, unsigned keep_empty)
return ret;
}
+
+int edit_todo_list(unsigned flags)
+{
+ struct strbuf buf = STRBUF_INIT;
+ const char *todo_file = rebase_path_todo();
+ FILE *todo;
+
+ if (strbuf_read_file(, todo_file, 0) < 0)
+ return error_errno(_("could not read '%s'."), todo_file);
+
+ strbuf_stripspace(, 1);
+ todo = fopen_or_warn(todo_file, "w");
+ if (!todo) {
+ strbuf_release();
+ return 1;
+ }
+
+ strbuf_write(, todo);
+ fclose(todo);
+ strbuf_release();
+
+ transform_todos(flags | TODO_LIST_SHORTEN_IDS);
+ append_todo_help(1, 0);
+
+ if
[GSoC][PATCH v5 03/20] editor: add a function to launch the sequence editor
As part of the rewrite of interactive rebase, the sequencer will need to
open the sequence editor to allow the user to edit the todo list.
Instead of duplicating the existing launch_editor() function, this
refactors it to a new function, launch_specified_editor(), which takes
the editor as a parameter, in addition to the path, the buffer and the
environment variables. launch_sequence_editor() is then added to launch
the sequence editor.
Signed-off-by: Alban Gruin
---
No changes since v4.
cache.h | 1 +
editor.c | 27 +--
strbuf.h | 2 ++
3 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/cache.h b/cache.h
index 8b447652a7..d70ae49ca2 100644
--- a/cache.h
+++ b/cache.h
@@ -1409,6 +1409,7 @@ extern const char *fmt_name(const char *name, const char
*email);
extern const char *ident_default_name(void);
extern const char *ident_default_email(void);
extern const char *git_editor(void);
+extern const char *git_sequence_editor(void);
extern const char *git_pager(int stdout_is_tty);
extern int is_terminal_dumb(void);
extern int git_ident_config(const char *, const char *, void *);
diff --git a/editor.c b/editor.c
index 9a9b4e12d1..c985eee1f9 100644
--- a/editor.c
+++ b/editor.c
@@ -1,4 +1,5 @@
#include "cache.h"
+#include "config.h"
#include "strbuf.h"
#include "run-command.h"
#include "sigchain.h"
@@ -34,10 +35,21 @@ const char *git_editor(void)
return editor;
}
-int launch_editor(const char *path, struct strbuf *buffer, const char *const
*env)
+const char *git_sequence_editor(void)
{
- const char *editor = git_editor();
+ const char *editor = getenv("GIT_SEQUENCE_EDITOR");
+
+ if (!editor)
+ git_config_get_string_const("sequence.editor", );
+ if (!editor)
+ editor = git_editor();
+ return editor;
+}
+
+static int launch_specified_editor(const char *editor, const char *path,
+ struct strbuf *buffer, const char *const
*env)
+{
if (!editor)
return error("Terminal is dumb, but EDITOR unset");
@@ -95,3 +107,14 @@ int launch_editor(const char *path, struct strbuf *buffer,
const char *const *en
return error_errno("could not read file '%s'", path);
return 0;
}
+
+int launch_editor(const char *path, struct strbuf *buffer, const char *const
*env)
+{
+ return launch_specified_editor(git_editor(), path, buffer, env);
+}
+
+int launch_sequence_editor(const char *path, struct strbuf *buffer,
+ const char *const *env)
+{
+ return launch_specified_editor(git_sequence_editor(), path, buffer,
env);
+}
diff --git a/strbuf.h b/strbuf.h
index 60a35aef16..66da9822fd 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -575,6 +575,8 @@ extern void strbuf_add_unique_abbrev(struct strbuf *sb,
* file's contents are not read into the buffer upon completion.
*/
extern int launch_editor(const char *path, struct strbuf *buffer, const char
*const *env);
+extern int launch_sequence_editor(const char *path, struct strbuf *buffer,
+ const char *const *env);
extern void strbuf_add_lines(struct strbuf *sb, const char *prefix, const char
*buf, size_t size);
--
2.18.0
Re: [PATCH v2 08/10] fetch tests: add a test clobbering tag behavior
Ævar Arnfjörð Bjarmason writes:
> The test suite only incidentally (and unintentionally) tested for the
> current behavior of eager tag clobbering on "fetch". This follow-up to
> the previous "push tests: assert re-pushing annotated tags" change
> tests for it explicitly.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
> t/t5516-fetch-push.sh | 24
> 1 file changed, 24 insertions(+)
>
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 1331a8de08..8912312be7 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1011,6 +1011,30 @@ test_force_push_tag () {
> test_force_push_tag "lightweight tag" "-f"
> test_force_push_tag "annotated tag" "-f -a -m'msg'"
>
> +test_force_fetch_tag () {
> + tag_type_description=$1
> + tag_args=$2
> +
> + test_expect_success "fetch will clobber an existing
> $tag_type_description" "
> + mk_test testrepo heads/master &&
> + mk_child testrepo child1 &&
> + mk_child testrepo child2 &&
> + (
> + cd testrepo &&
> + git tag Tag &&
> + git -C ../child1 fetch origin tag Tag &&
> + >file1 &&
> + git add file1 &&
> + git commit -m 'file1' &&
> + git tag $tag_args Tag &&
> + git -C ../child1 fetch origin tag Tag
> + )
> + "
> +}
> +
> +test_force_fetch_tag "lightweight tag" "-f"
> +test_force_fetch_tag "annotated tag" "-f -a -m'msg'"
I do not think that the single quotes around msg on the second one
does what you want them to. In "git tag $tag_args Tag" there is no
eval. You have the same for the push side, which can be seen in the
precontext of this hunk.
I somehow thought that you switched to using testTag for some
reason in an earlier step. Shouldn't we be calling this Tag
also testTag?
> +
> test_expect_success 'push --porcelain' '
> mk_empty testrepo &&
> echo >.git/foo "To testrepo" &&
Re: [PATCH] DO-NOT-MERGE: write and read commit-graph always
On Wed, Jul 18, 2018 at 8:22 AM, Derrick Stolee wrote: > The following test fails because the repo has ambiguous merge-bases, and > the commit-graph changes the walk order so we select a different one. > This alters the resulting merge from the expected result. > > t6024-recursive-merge.sh, Test 4 > > The tests above are made to pass by deleting the commit-graph file > before the necessary steps. I know you meant for these to not be merged, but perhaps the test in t6024 could be made to be less stringent about order of merge bases. In particular, instead of expecting a certain sha1 to be at stage 2 and a different one to be at stage 3, it could just check that both shas appear in the `git ls-files --stage` output. > diff --git a/t/t6024-recursive-merge.sh b/t/t6024-recursive-merge.sh > index 3f59e58dfb..cec10983cd 100755 > --- a/t/t6024-recursive-merge.sh > +++ b/t/t6024-recursive-merge.sh > @@ -61,6 +61,7 @@ GIT_AUTHOR_DATE="2006-12-12 23:00:08" git commit -m F > ' > > test_expect_success "combined merge conflicts" " > + rm -rf .git/objects/info/commit-graph && > test_must_fail git merge -m final G > " > > -- > 2.18.0.118.gd4f65b8d14
Re: [PATCH v2 06/10] push doc: correct lies about how push refspecs work
Ævar Arnfjörð Bjarmason writes:
> The is often the name of the branch you would want to push, but
> -it can be any arbitrary "SHA-1 expression", such as `master~4` or
> -`HEAD` (see linkgit:gitrevisions[7]).
> +it can be any arbitrary expression to a commit, such as `master~4` or
> +`HEAD` (see linkgit:gitrevisions[7]). It can also refer to tag
> +objects, trees or blobs if the is outside of `refs/heads/*`.
"It can also refer to..." is a good addition, but do you really want
to make it part of this series to change/deprecate "SHA-1 expression"
(which would certainly involve discussion on "then what to call them
instead, now we are trying to refrain from saying SHA-1?")?
> +on the remote side. Whether this is allowed depends on where in
> +`refs/*` the reference lives. The `refs/heads/*` namespace will
> +only accept commit objects, and then only they can be
> +fast-forwarded. The `refs/tags/*` namespace will accept any kind of
> +object, and any changes to them and others types of objects will be
> +rejected. Finally, it's possible to push any type of object to any
> +namespace outside of `refs/{tags,heads}/*`,
All sound correct.
> but these will be treated
> +as branches for the purposes of whether `--force` is required, even in
> +the case where a tag object is pushed.
I am not sure what "will be treated as branches" exactly means.
Does it mean "as if they were in refs/heads/* hierarchy?" Or
something else?
> That tag object will be
> +overwritten by another tag object (or commit!) without `--force` if
> +the new tag happens to point to a commit that's a fast-forward of the
> +commit it replaces.
Yup, and that is something we want to fix with a later part of this
series.
> +By having the optional leading `+` to a refspec (or using `--force`
> +command line option) you can tell Git to update the ref even if
> +it is not allowed by its respective namespace clobbering rules (e.g.,
> +it is not a fast-forward. in the case of `refs/heads/*` updates).
This gives an impression that with "--force" you can put non-commit
inside refs/heads/* hierarchy. Is that correct (if so we probably
would want to fix that behaviour)?
> +This
> +does *not* attempt to merge into . See EXAMPLES below for
> +details.
That is not wrong per-se, but would normal people expect a merge to
happen upon pushing on the other side, I wonder?
Thanks for cleaning up our longstanding mess.
[PATCH 2/2] Highlight keywords in remote sideband output.
The highlighting is done on the client-side. Supported keywords are
"error", "warning", "hint" and "success".
The colorization is controlled with the config setting "color.remote".
Co-authored-by: Duy Nguyen
Signed-off-by: Han-Wen Nienhuys
---
Documentation/config.txt| 9 +++
help.c | 1 +
help.h | 1 +
sideband.c | 113 +---
t/t5409-colorize-remote-messages.sh | 47
5 files changed, 162 insertions(+), 9 deletions(-)
create mode 100644 t/t5409-colorize-remote-messages.sh
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 43b2de7b5..0783323be 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1229,6 +1229,15 @@ color.push::
color.push.error::
Use customized color for push errors.
+color.remote::
+ A boolean to enable/disable colored remote output. If unset,
+ then the value of `color.ui` is used (`auto` by default).
+
+color.remote.::
+ Use customized color for each remote keywords. `` may be
+ `hint`, `warning`, `success` or `error` which match the
+ corresponding keyword.
+
color.showBranch::
A boolean to enable/disable color in the output of
linkgit:git-show-branch[1]. May be set to `always`,
diff --git a/help.c b/help.c
index 3ebf0568d..b6cafcfc0 100644
--- a/help.c
+++ b/help.c
@@ -425,6 +425,7 @@ void list_config_help(int for_human)
{ "color.diff", "", list_config_color_diff_slots },
{ "color.grep", "", list_config_color_grep_slots },
{ "color.interactive", "",
list_config_color_interactive_slots },
+ { "color.remote", "", list_config_color_sideband_slots },
{ "color.status", "", list_config_color_status_slots },
{ "fsck", "", list_config_fsck_msg_ids },
{ "receive.fsck", "", list_config_fsck_msg_ids },
diff --git a/help.h b/help.h
index f8b15323a..9eab6a3f8 100644
--- a/help.h
+++ b/help.h
@@ -83,6 +83,7 @@ void list_config_color_diff_slots(struct string_list *list,
const char *prefix);
void list_config_color_grep_slots(struct string_list *list, const char
*prefix);
void list_config_color_interactive_slots(struct string_list *list, const char
*prefix);
void list_config_color_status_slots(struct string_list *list, const char
*prefix);
+void list_config_color_sideband_slots(struct string_list *list, const char
*prefix);
void list_config_fsck_msg_ids(struct string_list *list, const char *prefix);
#endif /* HELP_H */
diff --git a/sideband.c b/sideband.c
index 325bf0e97..0d67583ec 100644
--- a/sideband.c
+++ b/sideband.c
@@ -1,6 +1,97 @@
#include "cache.h"
+#include "color.h"
+#include "config.h"
#include "pkt-line.h"
#include "sideband.h"
+#include "help.h"
+
+struct kwtable {
+ /*
+* We use keyword as config key so it can't contain funny chars like
+* spaces. When we do that, we need a separate field for slot name in
+* load_sideband_colors().
+*/
+ const char *keyword;
+ char color[COLOR_MAXLEN];
+};
+
+static struct kwtable keywords[] = {
+ { "hint", GIT_COLOR_YELLOW },
+ { "warning",GIT_COLOR_BOLD_YELLOW },
+ { "success",GIT_COLOR_BOLD_GREEN },
+ { "error", GIT_COLOR_BOLD_RED },
+};
+
+static int sideband_use_color = -1;
+
+static void load_sideband_colors(void)
+{
+ const char *key = "color.remote";
+ struct strbuf sb = STRBUF_INIT;
+ char *value;
+ int i;
+
+ if (sideband_use_color >= 0)
+ return;
+
+ if (!git_config_get_string(key, ))
+ sideband_use_color = git_config_colorbool(key, value);
+
+ for (i = 0; i < ARRAY_SIZE(keywords); i++) {
+ strbuf_reset();
+ strbuf_addf(, "%s.%s", key, keywords[i].keyword);
+ if (git_config_get_string(sb.buf, ))
+ continue;
+ if (color_parse(value, keywords[i].color))
+ die(_("expecting a color: %s"), value);
+ }
+ strbuf_release();
+}
+
+void list_config_color_sideband_slots(struct string_list *list, const char
*prefix)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(keywords); i++)
+ list_config_item(list, prefix, keywords[i].keyword);
+}
+
+/*
+ * Optionally highlight some keywords in remote output if they appear at the
+ * start of the line.
+ */
+void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
+{
+ int i;
+
+ load_sideband_colors();
+ if (!want_color_stderr(sideband_use_color)) {
+ strbuf_add(dest, src, n);
+ return;
+ }
+
+ while (isspace(*src)) {
+ strbuf_addch(dest, *src);
+ src++;
+ n--;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(keywords); i++) {
+ struct
[PATCH 1/2] Document git config getter return value.
---
config.h | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/config.h b/config.h
index b95bb7649..d39256eb1 100644
--- a/config.h
+++ b/config.h
@@ -178,10 +178,16 @@ struct config_set {
};
extern void git_configset_init(struct config_set *cs);
-extern int git_configset_add_file(struct config_set *cs, const char *filename);
-extern int git_configset_get_value(struct config_set *cs, const char *key,
const char **value);
+
extern const struct string_list *git_configset_get_value_multi(struct
config_set *cs, const char *key);
extern void git_configset_clear(struct config_set *cs);
+
+/*
+ * The int return values in the functions is 1 if not found, 0 if found,
leaving
+ * the found value in teh 'dest' pointer.
+ */
+extern int git_configset_add_file(struct config_set *cs, const char *filename);
+extern int git_configset_get_value(struct config_set *cs, const char *key,
const char **dest);
extern int git_configset_get_string_const(struct config_set *cs, const char
*key, const char **dest);
extern int git_configset_get_string(struct config_set *cs, const char *key,
char **dest);
extern int git_configset_get_int(struct config_set *cs, const char *key, int
*dest);
--
2.18.0.345.g5c9ce644c3-goog
[PATCH 0/2 v3] Highlight keywords in remote sideband output.
squash in Duy's patch Han-Wen Nienhuys (2): Document git config getter return value. Highlight keywords in remote sideband output. Documentation/config.txt| 9 +++ config.h| 10 ++- help.c | 1 + help.h | 1 + sideband.c | 113 +--- t/t5409-colorize-remote-messages.sh | 47 6 files changed, 170 insertions(+), 11 deletions(-) create mode 100644 t/t5409-colorize-remote-messages.sh -- 2.18.0.345.g5c9ce644c3-goog
Re: [PATCH v2 0/4] Speed up unpack_trees()
On 7/31/2018 12:50 PM, Ben Peart wrote: On 7/31/2018 11:31 AM, Duy Nguyen wrote: In the performance game of whack-a-mole, that call to repair cache-tree is now looking quite expensive... Yeah and I think we can whack that mole too. I did some measurement. Best case possible, we just need to scan through two indexes (one with many good cache-tree, one with no cache-tree), compare and copy cache-tree over. The scanning takes like 1% time of current repair step and I suspect it's the hashing that takes most of the time. Of course real world won't have such nice numbers, but I guess we could maybe half cache-tree update/repair time. I have some great profiling tools available so will take a look at this next and see exactly where the time is being spent. Good instincts. In cache_tree_update, the heavy hitter is definitely hash_object_file followed by has_object_file. NameInc %Inc + git!cache_tree_update 12.4 4,935 |+ git!update_one11.8 4,706 | + git!update_one 11.8 4,706 | + git!hash_object_file 6.1 2,406 | + git!has_object_file 2.0813 | + OTHER <> 0.5203 | + git!strbuf_addf 0.4155 | + git!strbuf_release 0.4143 | + git!strbuf_add 0.3121 | + OTHER <> 0.2 93 | + git!strbuf_grow 0.1 25
Re: [GSoC][PATCH v4] fixup! rebase -i: rewrite write_basic_state() in C
Junio C Hamano writes: > As the number of his or her own topics each contributor needs to > keep track of by definition is the number of all topics I need to s/is the/is smaller than the/; Sorry for the noise X-<. > take care of, I do not want to have to keep track of things myself > more than necessary, which would result in missed fixups and delayed > updates.
Re: git merge -s subtree seems to be broken.
On Tue, Jul 31, 2018 at 10:17:15AM -0700, Junio C Hamano wrote:
> Jeff King writes:
>
> > +...
> > + } else if (cmp > 0) {
> > /* path2 does not appear in one */
> > + score += score_missing(two.entry.mode, two.entry.path);
> > + update_tree_entry();
> > + continue;
> > + } if (oidcmp(one.entry.oid, two.entry.oid)) {
>
> As the earlier ones do the "continue at the end of the block", this
> does not affect the correctness, but I think you either meant "else if"
> or a fresh "if/else" that is disconnected from the previous if/else if/...
> chain.
Yes, thanks. I actually started to write it without the "continue" at
all, and a big "else" that checked the "we have both" case. But I backed
that out (in favor of a smaller diff), and forgot to add back in the
"else if".
-Peff
Question on range-diff and notes.displayref
Should git notes show up in a range-diff? I happened to have notes.displayref=refs/notes/amlog set in my git.git repo, and saw the below in my range-diff: On Tue, Jul 31, 2018 at 10:12 AM, Elijah Newren wrote: > 1: 4a1c9c3368 ! 1: 00f94a8b41 t1015: demonstrate directory/file conflict > recovery failures > @@ -14,7 +14,6 @@ > > Signed-off-by: Elijah Newren > Signed-off-by: Junio C Hamano > -Message-Id: <20180713163331.22446-2-new...@gmail.com> > > diff --git a/t/t1015-read-index-unmerged.sh > b/t/t1015-read-index-unmerged.sh > new file mode 100755 > 2: e105e8bfbd ! 2: d3b8d7edb6 read-cache: fix directory/file conflict > handling in read_index_unmerged() > @@ -59,7 +59,6 @@ > > Signed-off-by: Elijah Newren > Signed-off-by: Junio C Hamano > -Message-Id: <20180713163331.22446-3-new...@gmail.com> > Maybe this is expected or wanted (tbdiff also shows the git notes for what it's worth), but it seemed somewhat surprising to me. I'd rather not see such "differences" displayed for the patch series that I'm submitting, but perhaps others see it differently? Elijah
Re: git merge -s subtree seems to be broken.
Jeff King writes:
> +...
> + } else if (cmp > 0) {
> /* path2 does not appear in one */
> + score += score_missing(two.entry.mode, two.entry.path);
> + update_tree_entry();
> + continue;
> + } if (oidcmp(one.entry.oid, two.entry.oid)) {
As the earlier ones do the "continue at the end of the block", this
does not affect the correctness, but I think you either meant "else if"
or a fresh "if/else" that is disconnected from the previous if/else if/...
chain.
> /* they are different */
> ...
> + score += score_differs(one.entry.mode, two.entry.mode,
> +one.entry.path);
> + } else {
[PATCH v3 2/2] read-cache: fix directory/file conflict handling in read_index_unmerged()
read_index_unmerged() has two intended purposes:
* return 1 if there are any unmerged entries, 0 otherwise
* drops any higher-stage entries down to stage #0
There are several callers of read_index_unmerged() that check the return
value to see if it is non-zero, all of which then die() if that condition
is met. For these callers, dropping higher-stage entries down to stage #0
is a waste of resources, and returning immediately on first unmerged entry
would be better. But it's probably only a very minor difference and isn't
the focus of this series.
The remaining callers ignore the return value and call this function for
the side effect of dropping higher-stage entries down to stage #0. As
mentioned in commit e11d7b596970 ("'reset --merge': fix unmerged case",
2009-12-31),
The _only_ reason we want to keep a previously unmerged entry in the
index at stage #0 is so that we don't forget the fact that we have
corresponding file in the work tree in order to be able to remove it
when the tree we are resetting to does not have the path.
In fact, prior to commit d1a43f2aa4bf ("reset --hard/read-tree --reset -u:
remove unmerged new paths", 2008-10-15), read_index_unmerged() did just
remove unmerged entries from the cache immediately but that had the
unwanted effect of leaving around new untracked files in the tree from
aborted merges.
So, that's the intended purpose of this function. The problem is that
when directory/files conflicts are present, trying to add the file to the
index at stage 0 fails (because there is still a directory in the way),
and the function returns early with a -1 return code to signify the error.
As noted above, none of the callers who want the drop-to-stage-0 behavior
check the return status, though, so this means all remaining unmerged
entries remain in the index and the callers proceed assuming otherwise.
Users then see errors of the form:
error: 'DIR-OR-FILE' appears as both a file and as a directory
error: DIR-OR-FILE: cannot drop to stage #0
and potentially also messages about other unmerged entries which came
lexicographically later than whatever pathname was both a file and a
directory. Google finds a few hits searching for those messages,
suggesting there were probably a couple people who hit this besides me.
Luckily, calling `git reset --hard` multiple times would workaround
this bug.
Since the whole purpose here is to just put the entry *temporarily* into
the index so that any associated file in the working copy can be removed,
we can just skip the DFCHECK and allow both the file and directory to
appear in the index. The temporary simultaneous appearance of the
directory and file entries in the index will be removed by the callers
by calling unpack_trees(), which excludes these unmerged entries marked
with CE_CONFLICTED flag from the resulting index, before they attempt to
write the index anywhere.
Signed-off-by: Elijah Newren
Signed-off-by: Junio C Hamano
---
read-cache.c | 13 -
t/t1015-read-index-unmerged.sh | 8
t/t6020-merge-df.sh | 3 ---
t/t6042-merge-rename-corner-cases.sh | 2 --
4 files changed, 12 insertions(+), 14 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index 372588260e..666d295a5a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2632,10 +2632,13 @@ int write_locked_index(struct index_state *istate,
struct lock_file *lock,
/*
* Read the index file that is potentially unmerged into given
- * index_state, dropping any unmerged entries. Returns true if
- * the index is unmerged. Callers who want to refuse to work
- * from an unmerged state can call this and check its return value,
- * instead of calling read_cache().
+ * index_state, dropping any unmerged entries to stage #0 (potentially
+ * resulting in a path appearing as both a file and a directory in the
+ * index; the caller is responsible to clear out the extra entries
+ * before writing the index to a tree). Returns true if the index is
+ * unmerged. Callers who want to refuse to work from an unmerged
+ * state can call this and check its return value, instead of calling
+ * read_cache().
*/
int read_index_unmerged(struct index_state *istate)
{
@@ -2658,7 +2661,7 @@ int read_index_unmerged(struct index_state *istate)
new_ce->ce_flags = create_ce_flags(0) | CE_CONFLICTED;
new_ce->ce_namelen = len;
new_ce->ce_mode = ce->ce_mode;
- if (add_index_entry(istate, new_ce, 0))
+ if (add_index_entry(istate, new_ce, ADD_CACHE_SKIP_DFCHECK))
return error("%s: cannot drop to stage #0",
new_ce->name);
}
diff --git a/t/t1015-read-index-unmerged.sh b/t/t1015-read-index-unmerged.sh
index 32ef6bdcfa..55d22da32c 100755
--- a/t/t1015-read-index-unmerged.sh
+++ b/t/t1015-read-index-unmerged.sh
@@ -30,7 +30,7 @@ test_expect_success 'setup modify/delete
[PATCH v3 0/2] Address recovery failures with directory/file conflicts
This patch series fixes several "recovery" commands that outright fail or do not fully recover when directory-file conflicts are present. This includes: * git read-tree --reset HEAD * git am --skip * git am --abort * git merge --abort (or git reset --merge) * git reset --hard Changes since v2 (full range-diff below): - Backported to maint (there were some textual conflicts in t6042 due to the merging of en/merge-recursive-tests to master), because of this comment from Junio's what's cooking email: "This may have to be rebased on an older maintenance track before moving forward." Elijah Newren (2): t1015: demonstrate directory/file conflict recovery failures read-cache: fix directory/file conflict handling in read_index_unmerged() read-cache.c | 13 +-- t/t1015-read-index-unmerged.sh | 123 +++ t/t6020-merge-df.sh | 3 - t/t6042-merge-rename-corner-cases.sh | 2 - 4 files changed, 131 insertions(+), 10 deletions(-) create mode 100755 t/t1015-read-index-unmerged.sh 1: 4a1c9c3368 ! 1: 00f94a8b41 t1015: demonstrate directory/file conflict recovery failures @@ -14,7 +14,6 @@ Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano -Message-Id: <20180713163331.22446-2-new...@gmail.com> diff --git a/t/t1015-read-index-unmerged.sh b/t/t1015-read-index-unmerged.sh new file mode 100755 2: e105e8bfbd ! 2: d3b8d7edb6 read-cache: fix directory/file conflict handling in read_index_unmerged() @@ -59,7 +59,6 @@ Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano -Message-Id: <20180713163331.22446-3-new...@gmail.com> diff --git a/read-cache.c b/read-cache.c --- a/read-cache.c @@ -150,10 +149,18 @@ --- a/t/t6042-merge-rename-corner-cases.sh +++ b/t/t6042-merge-rename-corner-cases.sh @@ - ( - cd rename-directory-1 && + ' + + test_expect_success 'rename/directory conflict + clean content merge' ' +- git reset --hard && + git reset --hard && + git clean -fdqx && -- git reset --hard && - git reset --hard && - git clean -fdqx && +@@ + ' + + test_expect_success 'rename/directory conflict + content merge conflict' ' +- git reset --hard && + git reset --hard && + git clean -fdqx && -- 2.18.0.2.gf4c50c7885
[PATCH v3 1/2] t1015: demonstrate directory/file conflict recovery failures
Several "recovery" commands outright fail or do not fully recover
when directory-file conflicts are present. This includes:
* git read-tree --reset HEAD
* git am --skip
* git am --abort
* git merge --abort
* git reset --hard
Add testcases documenting these shortcomings.
Signed-off-by: Elijah Newren
Signed-off-by: Junio C Hamano
---
t/t1015-read-index-unmerged.sh | 123 +
1 file changed, 123 insertions(+)
create mode 100755 t/t1015-read-index-unmerged.sh
diff --git a/t/t1015-read-index-unmerged.sh b/t/t1015-read-index-unmerged.sh
new file mode 100755
index 00..32ef6bdcfa
--- /dev/null
+++ b/t/t1015-read-index-unmerged.sh
@@ -0,0 +1,123 @@
+#!/bin/sh
+
+test_description='Test various callers of read_index_unmerged'
+. ./test-lib.sh
+
+test_expect_success 'setup modify/delete + directory/file conflict' '
+ test_create_repo df_plus_modify_delete &&
+ (
+ cd df_plus_modify_delete &&
+
+ test_write_lines a b c d e f g h >letters &&
+ git add letters &&
+ git commit -m initial &&
+
+ git checkout -b modify &&
+ # Throw in letters.txt for sorting order fun
+ # ("letters.txt" sorts between "letters" and "letters/file")
+ echo i >>letters &&
+ echo "version 2" >letters.txt &&
+ git add letters letters.txt &&
+ git commit -m modified &&
+
+ git checkout -b delete HEAD^ &&
+ git rm letters &&
+ mkdir letters &&
+ >letters/file &&
+ echo "version 1" >letters.txt &&
+ git add letters letters.txt &&
+ git commit -m deleted
+ )
+'
+
+test_expect_failure 'read-tree --reset cleans unmerged entries' '
+ test_when_finished "git -C df_plus_modify_delete clean -f" &&
+ test_when_finished "git -C df_plus_modify_delete reset --hard" &&
+ (
+ cd df_plus_modify_delete &&
+
+ git checkout delete^0 &&
+ test_must_fail git merge modify &&
+
+ git read-tree --reset HEAD &&
+ git ls-files -u >conflicts &&
+ test_must_be_empty conflicts
+ )
+'
+
+test_expect_failure 'One reset --hard cleans unmerged entries' '
+ test_when_finished "git -C df_plus_modify_delete clean -f" &&
+ test_when_finished "git -C df_plus_modify_delete reset --hard" &&
+ (
+ cd df_plus_modify_delete &&
+
+ git checkout delete^0 &&
+ test_must_fail git merge modify &&
+
+ git reset --hard &&
+ test_path_is_missing .git/MERGE_HEAD &&
+ git ls-files -u >conflicts &&
+ test_must_be_empty conflicts
+ )
+'
+
+test_expect_success 'setup directory/file conflict + simple edit/edit' '
+ test_create_repo df_plus_edit_edit &&
+ (
+ cd df_plus_edit_edit &&
+
+ test_seq 1 10 >numbers &&
+ git add numbers &&
+ git commit -m initial &&
+
+ git checkout -b d-edit &&
+ mkdir foo &&
+ echo content >foo/bar &&
+ git add foo &&
+ echo 11 >>numbers &&
+ git add numbers &&
+ git commit -m "directory and edit" &&
+
+ git checkout -b f-edit d-edit^1 &&
+ echo content >foo &&
+ git add foo &&
+ echo eleven >>numbers &&
+ git add numbers &&
+ git commit -m "file and edit"
+ )
+'
+
+test_expect_failure 'git merge --abort succeeds despite D/F conflict' '
+ test_when_finished "git -C df_plus_edit_edit clean -f" &&
+ test_when_finished "git -C df_plus_edit_edit reset --hard" &&
+ (
+ cd df_plus_edit_edit &&
+
+ git checkout f-edit^0 &&
+ test_must_fail git merge d-edit^0 &&
+
+ git merge --abort &&
+ test_path_is_missing .git/MERGE_HEAD &&
+ git ls-files -u >conflicts &&
+ test_must_be_empty conflicts
+ )
+'
+
+test_expect_failure 'git am --skip succeeds despite D/F conflict' '
+ test_when_finished "git -C df_plus_edit_edit clean -f" &&
+ test_when_finished "git -C df_plus_edit_edit reset --hard" &&
+ (
+ cd df_plus_edit_edit &&
+
+ git checkout f-edit^0 &&
+ git format-patch -1 d-edit &&
+ test_must_fail git am -3 0001*.patch &&
+
+ git am --skip &&
+ test_path_is_missing .git/rebase-apply &&
+ git ls-files -u >conflicts &&
+ test_must_be_empty conflicts
+ )
+'
+
+test_done
--
2.18.0.2.gf4c50c7885
[PATCH v2] checkout: optimize "git checkout -b "
From: Ben Peart
Skip merging the commit, updating the index and working directory if and
only if we are creating a new branch via "git checkout -b ."
Any other checkout options will still go through the former code path.
If sparse_checkout is on, require the user to manually opt in to this
optimzed behavior by setting the config setting checkout.optimizeNewBranch
to true as we will no longer update the skip-worktree bit in the index, nor
add/remove files in the working directory to reflect the current sparse
checkout settings.
Signed-off-by: Ben Peart
---
The biggest change in this version was suggested in feedback to the last
patch. I have turned on the optimzation by default if sparse-checkout is
not on so that most users do not have to set anything and they will get the
benefit of the optimization.
Because users that use sparse checkout are probably doing so because they
have a large working directory, they stand to benefit the most from this
optimization. To enable them to benefit, I added a "checkout.optimizeNewBranch"
config setting that allows them to opt-in to this optimization if they are
willing to accept the change in behavior. I updated the documentation which
should make it clear exactly what the change in behavior is.
Notes:
Base Ref: master
Web-Diff: https://github.com/benpeart/git/commit/5ea167fe90
Checkout: git fetch https://github.com/benpeart/git checkout-b-v2 && git
checkout 5ea167fe90
### Patches
Documentation/config.txt | 5 ++
builtin/checkout.c | 113 +--
t/t1090-sparse-checkout-scope.sh | 14
3 files changed, 128 insertions(+), 4 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 43b2de7b5f..acf81143d4 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1101,6 +1101,11 @@ browser..path::
browse HTML help (see `-w` option in linkgit:git-help[1]) or a
working repository in gitweb (see linkgit:git-instaweb[1]).
+checkout.optimizeNewBranch
+ When set to true, "git checkout -b " will not update the
+ skip-worktree bit in the index nor add/remove files in the working
+ directory to reflect the current sparse checkout settings.
+
clean.requireForce::
A boolean to make git-clean do nothing unless given -f,
-i or -n. Defaults to true.
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 28627650cd..991b71a341 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -24,6 +24,8 @@
#include "submodule-config.h"
#include "submodule.h"
+static int checkout_optimize_new_branch;
+
static const char * const checkout_usage[] = {
N_("git checkout [] "),
N_("git checkout [] [] -- ..."),
@@ -41,6 +43,10 @@ struct checkout_opts {
int ignore_skipworktree;
int ignore_other_worktrees;
int show_progress;
+ /*
+* If new checkout options are added, skip_merge_working_tree
+* should be updated accordingly.
+*/
const char *new_branch;
const char *new_branch_force;
@@ -471,6 +477,98 @@ static void setup_branch_path(struct branch_info *branch)
branch->path = strbuf_detach(, NULL);
}
+/*
+ * Skip merging the trees, updating the index and working directory if and
+ * only if we are creating a new branch via "git checkout -b ."
+ */
+static int skip_merge_working_tree(const struct checkout_opts *opts,
+ const struct branch_info *old_branch_info,
+ const struct branch_info *new_branch_info)
+{
+ /*
+* Do the merge if sparse checkout is on and the user has not opted in
+* to the optimized behavior
+*/
+ if (core_apply_sparse_checkout && !checkout_optimize_new_branch)
+ return 0;
+
+ /*
+* We must do the merge if we are actually moving to a new commit.
+*/
+ if (!old_branch_info->commit || !new_branch_info->commit ||
+ oidcmp(_branch_info->commit->object.oid,
_branch_info->commit->object.oid))
+ return 0;
+
+ /*
+* opts->patch_mode cannot be used with switching branches so is
+* not tested here
+*/
+
+ /*
+* opts->quiet only impacts output so doesn't require a merge
+*/
+
+ /*
+* Honor the explicit request for a three-way merge or to throw away
+* local changes
+*/
+ if (opts->merge || opts->force)
+ return 0;
+
+ /*
+* --detach is documented as "updating the index and the files in the
+* working tree" but this optimization skips those steps so fall through
+* to the regular code path.
+*/
+ if (opts->force_detach)
+ return 0;
+
+ /*
+* opts->writeout_stage cannot be used with switching branches so is
+* not tested here
+*/
+
+ /*
+* Honor the explicit ignore requests
+*/
+
Re: [PATCH] DO-NOT-MERGE: write and read commit-graph always
Stefan Beller writes: >> I wonder though if all those changes to the testsuite shouldn't be >> merged. > > I think Stolee doesn't want this to be merged after rereading > subject and the commit message. Yes, I understand that, and for the most part I agree with it. This commit main purpose is to thoroughly exercise the new feature. But I think that changes to the testsuite could have been extracted into separate commit, and merged (and only those changes). It could serve as note about the intent of the test. Well, perhaps after encapsulating it in some function with a good name... ;-) Anyway, this is a minor insignificant thing. -- Jakub Narębski
Re: [PATCH v2 0/4] Speed up unpack_trees()
On 7/31/2018 11:31 AM, Duy Nguyen wrote: On Mon, Jul 30, 2018 at 8:10 PM Ben Peart wrote: I ran "git checkout" on a large repo and averaged the results of 3 runs. This clearly demonstrates the benefit of the optimized unpack_trees() as even the final "diff-index" is essentially a 3rd call to unpack_trees(). baselinenew -- 0.535510167 0.556558733 s: read cache .git/index 0.3057373 0.3147105 s: initialize name hash 0.0184082 0.023558433 s: preload index 0.086910967 0.089085967 s: refresh index 7.889590767 2.191554433 s: unpack trees 0.120760833 0.131941267 s: update worktree after a merge 2.2583504 2.572663167 s: repair cache-tree 0.8916137 0.959495233 s: write index, changed mask = 28 3.405199233 0.2710663 s: unpack trees 0.000999667 0.0021554 s: update worktree after a merge 3.4063306 0.273318333 s: diff-index 16.9524923 9.462943133 s: git command: 'c:\git-sdk-64\usr\src\git\git.exe' checkout The first call to unpack_trees() saves 72% The 2nd and 3rd call save 92% By the 3rd I guess you meant "diff-index" line. I think it's the same with the second call. diff-index triggers the second unpack-trees but there's no indent here and it's misleading to read this as diff-index and unpack-trees execute one after the other. Total time savings for the entire command was 44% Wow.. I guess you have more trees since I could only save 30% on gcc.git. Yes, with over 500K trees, this optimization really pays off for us. I can't wait to see how this works out in the wild (vs my "lab" based performance testing). Thank you! I definitely owe you lunch. :) In the performance game of whack-a-mole, that call to repair cache-tree is now looking quite expensive... Yeah and I think we can whack that mole too. I did some measurement. Best case possible, we just need to scan through two indexes (one with many good cache-tree, one with no cache-tree), compare and copy cache-tree over. The scanning takes like 1% time of current repair step and I suspect it's the hashing that takes most of the time. Of course real world won't have such nice numbers, but I guess we could maybe half cache-tree update/repair time. I have some great profiling tools available so will take a look at this next and see exactly where the time is being spent.
First test of t5552 fails on Windows
I'm testing origin/next on Windows with a few other topics on top. The first test fails like this. Do you see what is wrong? Where should I start looking? Is it perhaps that upload-pack is responding too soon so that fetch does not send 'have c1'? this is the console output ...(truncated)... ++ git -C client config fetch.negotiationalgorithm skipping +++ pwd +++ builtin pwd -W +++ pwd +++ builtin pwd -W ++ GIT_TRACE_PACKET='D:/Src/mingw-git/t/trash directory.t5552-skipping-fetch-negotiator/trace' ++ git -C client fetch 'D:/Src/mingw-git/t/trash directory.t5552-skipping-fetch-negotiator/server' warning: no common commits remote: Enumerating objects: 3, done. remote: Counting objects: 100% (3/3), done. remote: Total 3 (delta 0), reused 0 (delta 0) Unpacking objects: 100% (3/3), done. >From D:/Src/mingw-git/t/trash directory.t5552-skipping-fetch-negotiator/server * branchHEAD -> FETCH_HEAD ++ have_sent c7 c5 c2 c1 ++ test 4 -ne 0 +++ git -C client rev-parse c7 ++ grep 'fetch> have 9b13844ba1d52a28bb9487107b41cce9916b74c9' trace packet:fetch> have 9b13844ba1d52a28bb9487107b41cce9916b74c9 ++ test 0 -ne 0 ++ shift ++ test 3 -ne 0 +++ git -C client rev-parse c5 ++ grep 'fetch> have 0abab022ac7e07f16265106cf36faf7cb5d87ab3' trace packet:fetch> have 0abab022ac7e07f16265106cf36faf7cb5d87ab3 ++ test 0 -ne 0 ++ shift ++ test 2 -ne 0 +++ git -C client rev-parse c2 ++ grep 'fetch> have 0d7e994c092abbb0a21e7d243114efa5ba452b8c' trace packet:fetch> have 0d7e994c092abbb0a21e7d243114efa5ba452b8c ++ test 0 -ne 0 ++ shift ++ test 1 -ne 0 +++ git -C client rev-parse c1 ++ grep 'fetch> have 9e22a6c1b441ee1bcd54b8da801261ba8b15eac9' trace ++ test 1 -ne 0 +++ git -C client rev-parse c1 ++ echo 'No have 9e22a6c1b441ee1bcd54b8da801261ba8b15eac9 (c1)' No have 9e22a6c1b441ee1bcd54b8da801261ba8b15eac9 (c1) ++ return 1 error: last command exited with $?=1 not ok 1 - commits with no parents are sent regardless of skip distance # # git init server && # test_commit -C server to_fetch && # # git init client && # for i in $(seq 7) # do # test_commit -C client c$i # done && # # # We send: "c7" (skip 1) "c5" (skip 2) "c2" (skip 4). After that, since # # "c1" has no parent, it is still sent as "have" even though it would # # normally be skipped. # test_config -C client fetch.negotiationalgorithm skipping && # GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" && # have_sent c7 c5 c2 c1 && # have_not_sent c6 c4 c3 # trace file packet: upload-pack> 92dc17da106e837e66a07e4815c474bf4fe99dce HEAD\0multi_ack thin-pack side-band side-band-64k ofs-delta shallow deepen-since deepen-not deepen-relative no-progress include-tag multi_ack_detailed symref=HEAD:refs/heads/master agent=git/2.18.0.721.g75e0872d82 packet:fetch< 92dc17da106e837e66a07e4815c474bf4fe99dce HEAD\0multi_ack thin-pack side-band side-band-64k ofs-delta shallow deepen-since deepen-not deepen-relative no-progress include-tag multi_ack_detailed symref=HEAD:refs/heads/master agent=git/2.18.0.721.g75e0872d82 packet: upload-pack> 92dc17da106e837e66a07e4815c474bf4fe99dce refs/heads/master packet: upload-pack> 92dc17da106e837e66a07e4815c474bf4fe99dce refs/tags/to_fetch packet:fetch< 92dc17da106e837e66a07e4815c474bf4fe99dce refs/heads/master packet: upload-pack> 7da106e837e66a07e4815c474bf4fe99dce refs/tags/to_fetch packet:fetch< packet:fetch> want 92dc17da106e837e66a07e4815c474bf4fe99dce multi_ack_detailed side-band-64k thin-pack ofs-delta deepen-since deepen-not agent=git/2.18.0.721.g75e0872d82 packet:fetch> packet:fetch> have 9b13844ba1d52a28bb9487107b41cce9916b74c9 packet:fetch> have 0abab022ac7e07f16265106cf36faf7cb5d87ab3 packet:fetch> have 0d7e994c092abbb0a21e7d243114efa5ba452b8c packet: upload-pack< want 92dc17da106e837e66a07e4815c474bf4fe99dce multi_ack_detailed side-band-64k thin-pack ofs-delta deepen-since deepen-not agent=git/2.18.0.721.g75e0872d82 packet:fetch> done packet: upload-pack< packet: upload-pack< have 9b13844ba1d52a28bb9487107b41cce9916b74c9 packet: upload-pack< have 0abab022ac7e07f16265106cf36faf7cb5d87ab3 packet: upload-pack< have 0d7e994c092abbb0a21e7d243114efa5ba452b8c packet: upload-pack< have 9e22a6c1b441ee1bcd54b8da801261ba8b15eac9 packet: upload-pack< done packet: upload-pack> NAK packet:fetch< NAK packet: sideband< \2Enumerating objects: 3, done.Counting objects: 33% (1/3) \15Counting objects: 66% (2/3) \15Counting objects: 100% (3/3) \15Co packet: sideband< \2unting objects: 100% (3/3), done.Total 3 (delta 0), reused 0 (delta 0) packet: sideband< PACK ... packet: upload-pack> packet: sideband<
Re: [PATCH 2/2] refs: switch for_each_replace_ref back to use a ref_store
On Tue, Jul 31, 2018 at 2:41 AM Stefan Beller wrote: > > Taking a step back, was there anything that prompted these patches? > > I am flailing around on how to approach the ref store and the repository: > * I dislike having to pass a repository 'r' twice. (current situation after > patch 1. That patch itself is part of Stolees larger series to address > commit graphs and replace refs, so we will have that one way or another) > * So I sent out some RFC patches to have the_repository in the ref store > and pass the repo through to all the call backs to make it easy for > users inside the callback to do basic things like looking up commits. > * both Duy (on list) and Brandon (privately) expressed their dislike for > having the refs API bloated with the repository, as the repository is > not needed per se in the ref store. > * After some reflection I agreed with their concerns, which let me > to re-examine the refs API: all but a few select functions take a > ref_store as the first argument (or imply to work on the ref store > in the_repository, then neither a repo nor a ref store argument is > there) Since I'm the one who added the refs_* variants (which take ref_store as the first argument). There's one thing that I should have done but did not: making each_ref_fn takes the ref store. If a callback is given a refname and wants to do something about it (other that just printing it), chances are you need the same ref-store that triggers the callback and you should not need to pass a separate ref-store around by yourself because you would have the same "passing twice" problem that you disliked. This is more obvious with refs_for_each_reflog() because you will very likely want to parse the ref from the callback. Then, even ref store code needs access to object database and I don't think we want to pass a pair of "struct repository *", "struct ref_store *" in every API. We know the ref store has to be associated with one repository and we do save that information (notice that ref_store_init_fn takes gitdir and the "files" backend does save it). Once refs code is adapted to struct repository, I think it will take a 'struct repository *' instead of the gitdir string and store the pointer to the repository too for internal use. Then if a ref callback needs access to the same repository, we could just provide this repo via refs api. Since callbacks should already have access to the ref store (preferably without having to carrying it via cb_data), it has access to the repository as well and you don't need to explicitly pass the repository. > * I want to bring back the cleanliness of the API, which is to take a > ref store when needed instead of the repository, which is rather > bloated. -- Duy;
Re: git merge -s subtree seems to be broken.
On Tue, Jul 31, 2018 at 08:53:23AM -0700, Junio C Hamano wrote:
> George Shammas writes:
>
> > Bisecting around, this might be the commit that introduced the breakage.
> >
> > https://github.com/git/git/commit/d8febde
>
> Interesting. I've never used the "-s subtree" strategy without
> "-Xsubtree=..." to explicitly tell where the thing should go for a
> long time, so I am not surprised if I did not notice if an update to
> the heuristics made long time ago had affected tree matching.
>
> d8febde3 ("match-trees: simplify score_trees() using tree_entry()",
> 2013-03-24) does touch the area that may affect the subtree matching
> behaviour.
>
> Because it is an update to heuristics, and as such, we need to be
> careful when saying it is or is not "broken". Some heuristics may
> work better with your particular case, and may do worse with other
> cases.
>
> But from the log message description, it looks like it was meant to
> be a no-op simplification rewrite that should not affect the outcome,
> so it is a bit surprising.
Yeah, this is definitely not "well, the heuristic changed a bit". It's
just broken. This fixes it, but we should probably add a test.
diff --git a/match-trees.c b/match-trees.c
index 4cdeff53e1..730fff4cfb 100644
--- a/match-trees.c
+++ b/match-trees.c
@@ -83,34 +83,40 @@ static int score_trees(const struct object_id *hash1, const
struct object_id *ha
int score = 0;
for (;;) {
- struct name_entry e1, e2;
- int got_entry_from_one = tree_entry(, );
- int got_entry_from_two = tree_entry(, );
int cmp;
- if (got_entry_from_one && got_entry_from_two)
- cmp = base_name_entries_compare(, );
- else if (got_entry_from_one)
+ if (one.size && two.size)
+ cmp = base_name_entries_compare(, );
+ else if (one.size)
/* two lacks this entry */
cmp = -1;
- else if (got_entry_from_two)
+ else if (two.size)
/* two has more entries */
cmp = 1;
else
break;
- if (cmp < 0)
+ if (cmp < 0) {
/* path1 does not appear in two */
- score += score_missing(e1.mode, e1.path);
- else if (cmp > 0)
+ score += score_missing(one.entry.mode, one.entry.path);
+ update_tree_entry();
+ continue;
+ } else if (cmp > 0) {
/* path2 does not appear in one */
- score += score_missing(e2.mode, e2.path);
- else if (oidcmp(e1.oid, e2.oid))
+ score += score_missing(two.entry.mode, two.entry.path);
+ update_tree_entry();
+ continue;
+ } if (oidcmp(one.entry.oid, two.entry.oid)) {
/* they are different */
- score += score_differs(e1.mode, e2.mode, e1.path);
- else
+ score += score_differs(one.entry.mode, two.entry.mode,
+ one.entry.path);
+ } else {
/* same subtree or blob */
- score += score_matches(e1.mode, e2.mode, e1.path);
+ score += score_matches(one.entry.mode, two.entry.mode,
+ one.entry.path);
+ }
+ update_tree_entry();
+ update_tree_entry();
}
free(one_buf);
free(two_buf);
Re: [PATCH v2 0/2] Preserve skip_worktree bit in merges when necessary
On Fri, Jul 27, 2018 at 5:59 AM, Ben Peart wrote: > Sending this update as Elijah is on vacation. This only updates the test > case based on feedback from the list. Thanks! One less thing for me to catch up on. :-)
Re: [PATCH/RFC] Color merge conflicts
On Mon, Jul 30, 2018 at 10:40 AM, Stefan Beller wrote: > On Mon, Jul 30, 2018 at 9:00 AM Nguyễn Thái Ngọc Duy > wrote: >> >> One of the things I notice when watching a normal git user face a >> merge conflicts is the output is very verbose (especially when there >> are multiple conflicts) and it's hard to spot the important parts to >> start resolving conflicts unless you know what to look for. > > I usually go by git-status, but I am not the watched normal user, > hopefully. Maybe we want to run git-status after a failed merge > for the user, too, though? I'm a little worried the git status output may be long enough that they miss all the conflict messages and don't even think to scroll back and look at them. Since not all conflict types are nicely representable in git status output, that could be problematic. (e.g. renames aren't recorded in the index in any fashion, so git status can't tell you a conflict was from a rename/delete or rename/rename(1to2), for example; it's possible that information may be important in helping the user track down why the working directory ended up the way it did to help them resolve the conflict.) If that extra information that is currently only reported in these conflict messages were recorded somewhere -- either the index or the working tree -- then it wouldn't be as much of a risk to hide it behind git status. In fact, it would seem safer and nicer in general because users already run the risk of losing those conflict messages. >> This is the start to hopefully help that a bit. One thing I'm not sure >> is what to color because that affects the config keys we use for >> this. If we have to color different things, it's best to go >> "color.merge." but if this is the only place to color, that slot >> thing looks over-engineered. >> >> Perhaps another piece to color is the conflicted path? Maybe. But on >> the other hand, I don't think we want a chameleon output, just enough >> visual cues to go from one conflict to the next... > > I would think we would want to highlight the type of conflict as well, > e.g. > >CONFLICT> \ >(rename/rename): \ > Rename a -> b in branch foo \ > rename a ->c in bar > > but then again, this patch is better than not doing any colors. > > I wonder if we want to have certain colors associated with certain > types of merge conflicts, e.g. anything involving a rename would > be yellow, any conflict with deletion to be dark red, regular merge > conflicts blue (and at the same time we could introduce coloring > of conflict markers to also be blue) Providing extra hints might be good, but we'd need to flesh out this idea to avoid edge and corner cases... What do you do with mixtures, e.g. rename/delete conflicts? Average the colors? Colorize the "rename" differently than the "delete"? Also, by "regular conflicts" I think you mean the normal content-based conflict markers we stick in files, rather than path-based conflicts (like modify/delete or rename/add). However, something could be both -- e.g. for an add/add conflict we two-way merge the files so we have both a path conflict (both sides added a file with that name) and a content or "regular" conflict (most the files were the same but they differ on these lines...). Things could get even crazier with e.g. rename/add/delete or rename/rename(2to1)/delete. So, if you want to colorize regular (or content) conflicts differently than path-based ones, what do you do when you have both types present? Does one win? Do we print multiple conflict messages? Something else? > (I am just making up colors, not seriously suggesting them) > > I like the idea! > > Thanks, > Stefan
Re: git merge -s subtree seems to be broken.
Jeff King writes: > The problem introduced in that commit is that each iteration through the > loop advances the tree pointers. Ah, indeed. The original used tree_entry_extract() and update_tree_entry() separately, but the update does tree_entry() on both sides. > So the assertion in that commit message that "the calls to > update_tree_entry() are not needed any more" is just wrong. We have > decide whether to call it based on the "cmp" value. Yup.
Re: [GSoC][PATCH v4] fixup! rebase -i: rewrite write_basic_state() in C
Alban Gruin writes: >> Hmph, from reading your other message >> >> >> https://public-inbox.org/git/dce8c99b-51e9-4ed1-8ae4-28049cb6e...@gmail.com/ >> >> I got an impression that a rerolled version is coming anyway. Is >> this fix so urgent that it needs tobe squashed in in the meantime >> and cannot wait? >> > > I wanted to reroll it first, but the only changes would have been this > fix and Ramsay’s patch. I was advised to send a fixup! patch instead. > > I can send a reroll if you want, but it won’t have any more changes. Then please do send an updated one with v$n incremented. As the number of his or her own topics each contributor needs to keep track of by definition is the number of all topics I need to take care of, I do not want to have to keep track of things myself more than necessary, which would result in missed fixups and delayed updates.
Re: [PATCH] refspec: allow @ on the left-hand side of refspecs
On 07/30, brian m. carlson wrote: > On Mon, Jul 30, 2018 at 10:50:51AM -0700, Brandon Williams wrote: > > On 07/29, brian m. carlson wrote: > > > The object ID parsing machinery is aware of "@" as a synonym for "HEAD" > > > and this is documented accordingly in gitrevisions(7). The push > > > documentation describes the source portion of a refspec as "any > > > arbitrary 'SHA-1 expression'"; however, "@" is not allowed on the > > > left-hand side of a refspec, since we attempt to check for it being a > > > valid ref name and fail (since it is not). > > > > > > Teach the refspec machinery about this alias and silently substitute > > > "HEAD" when we see "@". This handles the fact that HEAD is a symref and > > > preserves its special behavior. We need not handle other arbitrary > > > object ID expressions (such as "@^") when pushing because the revision > > > machinery already handles that for us. > > > > So this claims that using "@^" should work despite not accounting for it > > explicitly or am I misreading? Unless I'm mistaken, it looks like we > > don't really support arbitrary rev syntax in refspecs since "HEAD^" > > doesn't work either. > > Correct, it does indeed work, at least for me: > > genre ok % git push castro HEAD^:refs/heads/temp > Total 0 (delta 0), reused 0 (delta 0) > To https://git.crustytoothpaste.net/git/bmc/git.git > * [new branch]HEAD^ -> temp > > genre ok % git push castro @^:refs/heads/temp > Total 0 (delta 0), reused 0 (delta 0) > To https://git.crustytoothpaste.net/git/bmc/git.git > * [new branch]@^ -> temp > > Note that in this case, I had to specify a full ref since it didn't > exist on the remote and the left side wasn't a ref name. That's what I was missing, a full refspec! Thanks for the illustration. > > Now it doesn't work for fetches, only pushes. Only the left side of a > push refspec can be an arbitrary expression. > -- > brian m. carlson: Houston, Texas, US > OpenPGP: https://keybase.io/bk2204 -- Brandon Williams
Re: [GSoC][PATCH v4] fixup! rebase -i: rewrite write_basic_state() in C
Hi Junio, Le 31/07/2018 à 17:23, Junio C Hamano a écrit : > Alban Gruin writes: > >> As pointed out by SZEDER Gábor, git-rebase.sh wrote to to 'quiet' with >> an `echo`: >> >> echo "$GIT_QUIET" > "$state_dir/quiet" >> >> This mean that even if $GIT_QUIET is empty, a newline is written to >> quiet. The rewrite of write_basic_state() changed this behaviour, which >> could lead to problems. This patch changes the rewritten version to >> behave like the shell version. >> >> Signed-off-by: Alban Gruin >> --- >> Hi Junio, could you apply this patch on top of ag/rebase-i-in-c, please? > > Hmph, from reading your other message > > https://public-inbox.org/git/dce8c99b-51e9-4ed1-8ae4-28049cb6e...@gmail.com/ > > I got an impression that a rerolled version is coming anyway. Is > this fix so urgent that it needs tobe squashed in in the meantime > and cannot wait? > I wanted to reroll it first, but the only changes would have been this fix and Ramsay’s patch. I was advised to send a fixup! patch instead. I can send a reroll if you want, but it won’t have any more changes. Cheers, Alban >> >> sequencer.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/sequencer.c b/sequencer.c >> index d257903db0..0d41e82953 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -2332,7 +2332,7 @@ int write_basic_state(struct replay_opts *opts, const >> char *head_name, >> if (quiet) >> write_file(rebase_path_quiet(), "%s\n", quiet); >> else >> -write_file(rebase_path_quiet(), ""); >> +write_file(rebase_path_quiet(), "\n"); >> >> if (opts->verbose) >> write_file(rebase_path_verbose(), "");
Re: git merge -s subtree seems to be broken.
While debugging this, I did try -X subtree=src/ however the effect was the
same.
On Tue, Jul 31, 2018 at 11:53 AM Junio C Hamano wrote:
> George Shammas writes:
>
> > Bisecting around, this might be the commit that introduced the breakage.
> >
> > https://github.com/git/git/commit/d8febde
>
> Interesting. I've never used the "-s subtree" strategy without
> "-Xsubtree=..." to explicitly tell where the thing should go for a
> long time, so I am not surprised if I did not notice if an update to
> the heuristics made long time ago had affected tree matching.
>
> d8febde3 ("match-trees: simplify score_trees() using tree_entry()",
> 2013-03-24) does touch the area that may affect the subtree matching
> behaviour.
>
> Because it is an update to heuristics, and as such, we need to be
> careful when saying it is or is not "broken". Some heuristics may
> work better with your particular case, and may do worse with other
> cases.
>
> But from the log message description, it looks like it was meant to
> be a no-op simplification rewrite that should not affect the outcome,
> so it is a bit surprising.
>
Re: git merge -s subtree seems to be broken.
George Shammas writes:
> Bisecting around, this might be the commit that introduced the breakage.
>
> https://github.com/git/git/commit/d8febde
Interesting. I've never used the "-s subtree" strategy without
"-Xsubtree=..." to explicitly tell where the thing should go for a
long time, so I am not surprised if I did not notice if an update to
the heuristics made long time ago had affected tree matching.
d8febde3 ("match-trees: simplify score_trees() using tree_entry()",
2013-03-24) does touch the area that may affect the subtree matching
behaviour.
Because it is an update to heuristics, and as such, we need to be
careful when saying it is or is not "broken". Some heuristics may
work better with your particular case, and may do worse with other
cases.
But from the log message description, it looks like it was meant to
be a no-op simplification rewrite that should not affect the outcome,
so it is a bit surprising.
